Re: [PATCH] KVM: arm64: Don't access PMCR_EL0 when no PMU is available

From: Marc Zyngier <maz@kernel.org>
To: Alexandru Elisei <alexandru.elisei@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>,
	James Morse <james.morse@arm.com>,
	linux-arm-kernel@lists.infradead.org, kernel-team@android.com,
	kvmarm@lists.cs.columbia.edu,
	Julien Thierry <julien.thierry.kdev@gmail.com>
Subject: Re: [PATCH] KVM: arm64: Don't access PMCR_EL0 when no PMU is available
Date: Thu, 10 Dec 2020 11:16:42 +0000	[thread overview]
Message-ID: <ce2301d5ca78e35cd05aca54b14e141a@kernel.org> (raw)
In-Reply-To: <41fab19e-1e6d-f39b-c0a8-d4a1e54fc9b9@arm.com>

Hi Alex,

Thanks for looking at this.

On 2020-12-10 10:12, Alexandru Elisei wrote:
> Hi Marc,
> 
> On 12/10/20 8:30 AM, Marc Zyngier wrote:
>> We reset the guest's view of PMCR_EL0 unconditionally, based on
>> the host's view of this register. It is however legal for an
>> imnplementation not to provide any PMU, resulting in an UNDEF.
>> 
>> The obvious fix is to skip the reset of this shadow register
>> when no PMU is available, sidestepping the issue entirely.
>> If no PMU is available, the guest is not able to request
>> a virtual PMU anyway, so not doing nothing is the right thing
>> to do!
>> 
>> It is unlikely that this bug can hit any HW implementation
>> though, as they all provide a PMU. It has been found using nested
>> virt with the host KVM not implementing the PMU itself.
>> 
>> Fixes: ab9468340d2bc ("arm64: KVM: Add access handler for PMCR 
>> register")
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>>  arch/arm64/kvm/sys_regs.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index bc15246775d0..6c64d010102b 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -923,6 +923,10 @@ static void reset_pmcr(struct kvm_vcpu *vcpu, 
>> const struct sys_reg_desc *r)
>>  {
>>  	u64 pmcr, val;
>> 
>> +	/* No PMU available, PMCR_EL0 may UNDEF... */
>> +	if (!kvm_arm_support_pmu_v3())
>> +		return;
>> +
> 
> reset_pmcr() is called from kvm_reset_vcpu()->kvm_reset_sys_regs().
> Before calling kvm_reset_sys_regs(), kvm_reset_vcpu() returns -EINVAL
> if the VCPU has the PMUv3 feature but the host doesn't have a PMU.
> 
> It looks to me like the undef can happen only when the VCPU feature
> isn't set and the hardware doesn't have a PMU.

Which is exactly what I describe in the commit message (NV without PMU).

> How about we change
> the test to check for kvm_vcpu_has_pmu() to avoid executing the extra
> instructions, which are not needed because the VM won't have a PMU?

I went down that road initially, and then realised that we need to
backport this as far back as 4.9 (the code was merged in 4.6).
I don't fancy backporting kvm_vcpu_has_pmu() and co...

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel