Hi Marc,
On 12/10/20 11:16 AM, Marc Zyngier wrote:
> 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.
s/imnplementation/implementation
>>>
>>> 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@xxxxxxxxxx>
>>> ---
>>> 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).
Yes, I was looking at the code trying to understand how the undef can happen.
>
>> 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...
Makes sense, and I do consider this approach to be consistent with how we handle
PMU reset. The patch looks alright to me:
Reviewed-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
Thanks,
Alex
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
