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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8a908ee8-620a-d9c2-734b-5a6402950072@arm.com>
Date: Mon, 5 Feb 2024 12:16:53 +0000
From: James Clark <james.clark@....com>
To: Oliver Upton <oliver.upton@...ux.dev>
Cc: coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org,
 kvmarm@...ts.linux.dev, broonie@...nel.org, maz@...nel.org,
 suzuki.poulose@....com, acme@...nel.org, James Morse <james.morse@....com>,
 Zenghui Yu <yuzenghui@...wei.com>, Catalin Marinas
 <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
 Mike Leach <mike.leach@...aro.org>, Leo Yan <leo.yan@...aro.org>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Anshuman Khandual <anshuman.khandual@....com>, Rob Herring
 <robh@...nel.org>, Miguel Luis <miguel.luis@...cle.com>,
 Jintack Lim <jintack.lim@...aro.org>, Ard Biesheuvel <ardb@...nel.org>,
 Mark Rutland <mark.rutland@....com>, Arnd Bergmann <arnd@...db.de>,
 Vincent Donnefort <vdonnefort@...gle.com>,
 Kristina Martsenko <kristina.martsenko@....com>,
 Fuad Tabba <tabba@...gle.com>, Joey Gouly <joey.gouly@....com>,
 Akihiko Odaki <akihiko.odaki@...nix.com>, Jing Zhang
 <jingzhangos@...gle.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/7] arm64: KVM: Use shared area to pass PMU event
 state to hypervisor



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.