Re: [PATCH v4 2/7] arm64: KVM: Use shared area to pass PMU event state to hypervisor

[James Clark <james.clark@arm.com>]

On 02/02/2024 22:00, Oliver Upton wrote:
> On Thu, Jan 04, 2024 at 04:27:02PM +0000, James Clark wrote:
> 
> [...]
> 
>> diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c
>> index c50f8459e4fc..89147a9dc38c 100644
>> --- a/arch/arm64/kvm/hyp/nvhe/switch.c
>> +++ b/arch/arm64/kvm/hyp/nvhe/switch.c
>> @@ -130,13 +130,18 @@ static void __hyp_vgic_restore_state(struct kvm_vcpu *vcpu)
>>  	}
>>  }
>>  
>> +static struct kvm_pmu_events *kvm_nvhe_get_pmu_events(struct kvm_vcpu *vcpu)
>> +{
>> +	return &kvm_host_global_state[vcpu->cpu].pmu_events;
>> +}
>> +
>>  /*
>>   * Disable host events, enable guest events
>>   */
>>  #ifdef CONFIG_HW_PERF_EVENTS
>>  static bool __pmu_switch_to_guest(struct kvm_vcpu *vcpu)
>>  {
>> -	struct kvm_pmu_events *pmu = &vcpu->arch.pmu.events;
>> +	struct kvm_pmu_events *pmu = kvm_nvhe_get_pmu_events(vcpu);
>>  
>>  	if (pmu->events_host)
>>  		write_sysreg(pmu->events_host, pmcntenclr_el0);
>> @@ -152,7 +157,7 @@ static bool __pmu_switch_to_guest(struct kvm_vcpu *vcpu)
>>   */
>>  static void __pmu_switch_to_host(struct kvm_vcpu *vcpu)
>>  {
>> -	struct kvm_pmu_events *pmu = &vcpu->arch.pmu.events;
>> +	struct kvm_pmu_events *pmu = kvm_nvhe_get_pmu_events(vcpu);
>>  
>>  	if (pmu->events_guest)
>>  		write_sysreg(pmu->events_guest, pmcntenclr_el0);
> 
> This now allows the host to program event counters for a protected
> guest. That _might_ be a useful feature behind some debug option, but is
> most definitely *not* something we want to do for pVMs generally.

Unless I'm missing something, using PMUs on protected guests was added
by 722625c6f4c5b ("KVM: arm64: Reenable pmu in Protected Mode"). This
change is just a refactor that will allow us to add the same behavior
for a similar feature (tracing) without adding yet another copy of some
state before the guest switch.

> 
> Do we even need to make this shared data work at all for pKVM? The rest
> of the shared data between pKVM and the kernel is system information,
> which (importantly) doesn't have any guest context in it.
> 

Probably not, Marc actually mentioned on one of the first versions of
that this could be hidden behind a debug flag. To be honest one of the
reasons I didn't do that was because I wasn't sure what the appropriate
debug setting was. NVHE_EL2_DEBUG didn't seem quite right. DEBUG_KERNEL
maybe? Or a new one?

And then I suppose I got distracted by trying to make it have feature
parity with PMUs and forgot about the debug only thing.


> I'm perfectly happy leaving these sorts of features broken for pKVM and
> using the 'normal' way of getting percpu data to the nVHE hypervisor
> otherwise.
> 

I can do that. But do I also disable PMU at the same time in a new
commit? Now that both PMU and tracing is working maybe it would be a
waste to throw that away and hiding it behind an option is better. Or I
can leave the PMU as it is and just keep tracing disabled in pKVM.

I don't mind either way, my main goal was to get exclude/include guest
tracing working for normal VMs. For pKVM I don't have a strong opinion.