linux-kernel - Re: [RFC PATCH v2 4/4] KVM: arm64: Make guests see only counters they can access

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] [day] [month] [year] [list]
Message-ID: <gsnted051jqc.fsf@coltonlewis-kvm.c.googlers.com>
Date: Mon, 10 Feb 2025 23:08:11 +0000
From: Colton Lewis <coltonlewis@...gle.com>
To: Oliver Upton <oliver.upton@...ux.dev>
Cc: kvm@...r.kernel.org, linux@...linux.org.uk, catalin.marinas@....com, 
	will@...nel.org, maz@...nel.org, joey.gouly@....com, suzuki.poulose@....com, 
	yuzenghui@...wei.com, mark.rutland@....com, pbonzini@...hat.com, 
	shuah@...nel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, kvmarm@...ts.linux.dev, 
	linux-perf-users@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [RFC PATCH v2 4/4] KVM: arm64: Make guests see only counters they
 can access

Hi Oliver, thanks for the review.

Oliver Upton <oliver.upton@...ux.dev> writes:

> On Sat, Feb 08, 2025 at 02:01:11AM +0000, Colton Lewis wrote:
>> The ARM architecture specifies that when MDCR_EL2.HPMN is set, EL1 and
>> EL0, which includes KVM guests, should read that value for PMCR.N.

>> Signed-off-by: Colton Lewis <coltonlewis@...gle.com>
>> ---
>>   arch/arm64/kvm/debug.c                                  | 3 +--
>>   arch/arm64/kvm/pmu-emul.c                               | 8 +++++++-
>>   tools/testing/selftests/kvm/arm64/vpmu_counter_access.c | 2 +-
>>   3 files changed, 9 insertions(+), 4 deletions(-)

>> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
>> index 0e4c805e7e89..7c04db00bf6c 100644
>> --- a/arch/arm64/kvm/debug.c
>> +++ b/arch/arm64/kvm/debug.c
>> @@ -36,8 +36,7 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu  
>> *vcpu)
>>   	 * This also clears MDCR_EL2_E2PB_MASK and MDCR_EL2_E2TB_MASK
>>   	 * to disable guest access to the profiling and trace buffers
>>   	 */
>> -	vcpu->arch.mdcr_el2 = FIELD_PREP(MDCR_EL2_HPMN,
>> -					 *host_data_ptr(nr_event_counters));
>> +	vcpu->arch.mdcr_el2 = FIELD_PREP(MDCR_EL2_HPMN, read_mdcr());

> Please avoid unnecessary accesses to MDCR_EL2 at all costs. This is a
> guaranteed trap to EL2 with nested virt.

I thought it was necessary when I wrote this, but I see it's not after
thinking about it for a while.

The intended value is accessible from vcpu->kvm->arch.arm_pmu.hpmn as
written, but could be somewhere else after addressing your suggestion to
lift all MDCR handling into KVM.

>>   	vcpu->arch.mdcr_el2 |= (MDCR_EL2_TPM |
>>   				MDCR_EL2_TPMS |
>>   				MDCR_EL2_TTRF |
>> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
>> index 6c5950b9ceac..052ce8c721fe 100644
>> --- a/arch/arm64/kvm/pmu-emul.c
>> +++ b/arch/arm64/kvm/pmu-emul.c
>> @@ -993,12 +993,18 @@ static bool pmu_irq_is_valid(struct kvm *kvm, int  
>> irq)
>>   u8 kvm_arm_pmu_get_max_counters(struct kvm *kvm)
>>   {
>>   	struct arm_pmu *arm_pmu = kvm->arch.arm_pmu;
>> +	u8 limit;
>> +
>> +	if (arm_pmu->partitioned)
>> +		limit = arm_pmu->hpmn - 1;
>> +	else
>> +		limit = ARMV8_PMU_MAX_GENERAL_COUNTERS;

>>   	/*
>>   	 * The arm_pmu->cntr_mask considers the fixed counter(s) as well.
>>   	 * Ignore those and return only the general-purpose counters.
>>   	 */
>> -	return bitmap_weight(arm_pmu->cntr_mask,  
>> ARMV8_PMU_MAX_GENERAL_COUNTERS);
>> +	return bitmap_weight(arm_pmu->cntr_mask, limit);
>>   }

> This isn't necessary and is likely to regress the existing behavior.

> When the architecture says the lower ELs should see PMCR_EL0.N have the
> same value as MDCR_EL2.HPMN, what it really means is direct reads from
> hardware will return the value.

> So my expectation would be that a VM using the partitioned PMU
> implementation would never reach any of the *emulated* PMU handlers, as
> we would've disabled the associated traps.

Understood. The change was here to see be able to read PMCR_EL0.N from
inside a VM since I haven't disabled the associated traps yet.

It shouldn't be in this patch.

> The partitioned PMU will not replace the emulated vPMU implementation in
> KVM, so it'd be good to refactor what we have today to make room for
> your work. I think that'd be along the lines of:

>   - Shared code for event filter enforcement and handling of the vPMU
>     overflow IRQ.

>   - Emulated PMU implementation that provides trap handlers for all PMUv3
>     registers and backs into host perf

>   - Partitioned PMU implementation that doesn't rely on traps and instead
>     saves / restores a portion of the PMU that contains the guest
>     context.

> These should be done in separate files, i.e. I do not want to see a
> bunch of inline handling to cope with emulated v. partitioned PMUs.

Agreed.

>>   static void kvm_arm_set_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)
>> diff --git a/tools/testing/selftests/kvm/arm64/vpmu_counter_access.c  
>> b/tools/testing/selftests/kvm/arm64/vpmu_counter_access.c
>> index f16b3b27e32e..b5bc18b7528d 100644
>> --- a/tools/testing/selftests/kvm/arm64/vpmu_counter_access.c
>> +++ b/tools/testing/selftests/kvm/arm64/vpmu_counter_access.c
>> @@ -609,7 +609,7 @@ static void run_pmregs_validity_test(uint64_t pmcr_n)
>>    */
>>   static void run_error_test(uint64_t pmcr_n)
>>   {
>> -	pr_debug("Error test with pmcr_n %lu (larger than the host)\n",  
>> pmcr_n);
>> +	pr_debug("Error test with pmcr_n %lu (larger than the host allows)\n",  
>> pmcr_n);

> NBD for an RFC, but in the future please do selftests changes in a
> separate patch.

Ok, I'll do that in the future.