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

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

[Marc Zyngier]

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;
+
 	pmcr = read_sysreg(pmcr_el0);
 	/*
 	 * Writable bits of PMCR_EL0 (ARMV8_PMU_PMCR_MASK) are reset to UNKNOWN
-- 
2.29.2


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

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

[Alexandru Elisei]

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. 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?

On the other hand, kvm_pmu_vcpu_reset() is happy to do the reset even if the VCPU
feature isn't set, the host doesn't support a PMU, and before PMCR_EL0 is
initialized. It's up to you, the kvm_arm_support_pmu_v3() check looks consistent
with how PMU reset is handled.

Thanks,
Alex

>  	pmcr = read_sysreg(pmcr_el0);
>  	/*
>  	 * Writable bits of PMCR_EL0 (ARMV8_PMU_PMCR_MASK) are reset to UNKNOWN

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

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

[Marc Zyngier]

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

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

[Alexandru Elisei]

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@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).

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@arm.com>

Thanks,
Alex

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

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

[Qian Cai]

On Thu, 2020-12-10 at 08:30 +0000, 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>

Reverting this commit on the top of today's linux-next fixed a qemu-kvm coredump
issue on TX2 while starting a guest.

- host kernel .config:
https://cailca.coding.net/public/linux/mm/git/files/master/arm64.config

# /usr/libexec/qemu-kvm -name ubuntu-20.04-server-cloudimg -cpu host -smp 2 -m 2g
-drive if=none,format=qcow2,file=./ubuntu-20.04-server-cloudimg.qcow2,id=hd 
-device virtio-scsi -device scsi-hd,drive=hd -cdrom ./ubuntu-20.04-server-cloudimg.iso
-bios /usr/share/AAVMF/AAVMF_CODE.fd -M gic-version=host -nographic 
-nic user,model=virtio,hostfwd=tcp::2222-:22

qemu-kvm: /builddir/build/BUILD/qemu-4.2.0/target/arm/helper.c:1812:
pmevcntr_rawwrite: Assertion `counter < pmu_num_counters(env)' failed.

> ---
>  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;
> +
>  	pmcr = read_sysreg(pmcr_el0);
>  	/*
>  	 * Writable bits of PMCR_EL0 (ARMV8_PMU_PMCR_MASK) are reset to UNKNOWN


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

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

[Marc Zyngier]

On 2021-01-04 15:47, Qian Cai wrote:
> On Thu, 2020-12-10 at 08:30 +0000, 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>
> 
> Reverting this commit on the top of today's linux-next fixed a qemu-kvm 
> coredump
> issue on TX2 while starting a guest.
> 
> - host kernel .config:
> https://cailca.coding.net/public/linux/mm/git/files/master/arm64.config
> 
> # /usr/libexec/qemu-kvm -name ubuntu-20.04-server-cloudimg -cpu host
> -smp 2 -m 2g
> -drive 
> if=none,format=qcow2,file=./ubuntu-20.04-server-cloudimg.qcow2,id=hd
> -device virtio-scsi -device scsi-hd,drive=hd -cdrom
> ./ubuntu-20.04-server-cloudimg.iso
> -bios /usr/share/AAVMF/AAVMF_CODE.fd -M gic-version=host -nographic
> -nic user,model=virtio,hostfwd=tcp::2222-:22
> 
> qemu-kvm: /builddir/build/BUILD/qemu-4.2.0/target/arm/helper.c:1812:
> pmevcntr_rawwrite: Assertion `counter < pmu_num_counters(env)' failed.

You don't have KVM_ARM_PMU selected in your config, so QEMU cannot
access the PMU registers, and no counters are exposed.

         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

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

[Qian Cai]

On Mon, 2021-01-04 at 16:08 +0000, Marc Zyngier wrote:
> On 2021-01-04 15:47, Qian Cai wrote:
> > On Thu, 2020-12-10 at 08:30 +0000, 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>
> > 
> > Reverting this commit on the top of today's linux-next fixed a qemu-kvm 
> > coredump
> > issue on TX2 while starting a guest.
> > 
> > - host kernel .config:
> > https://cailca.coding.net/public/linux/mm/git/files/master/arm64.config
> > 
> > # /usr/libexec/qemu-kvm -name ubuntu-20.04-server-cloudimg -cpu host
> > -smp 2 -m 2g
> > -drive 
> > if=none,format=qcow2,file=./ubuntu-20.04-server-cloudimg.qcow2,id=hd
> > -device virtio-scsi -device scsi-hd,drive=hd -cdrom
> > ./ubuntu-20.04-server-cloudimg.iso
> > -bios /usr/share/AAVMF/AAVMF_CODE.fd -M gic-version=host -nographic
> > -nic user,model=virtio,hostfwd=tcp::2222-:22
> > 
> > qemu-kvm: /builddir/build/BUILD/qemu-4.2.0/target/arm/helper.c:1812:
> > pmevcntr_rawwrite: Assertion `counter < pmu_num_counters(env)' failed.
> 
> You don't have KVM_ARM_PMU selected in your config, so QEMU cannot
> access the PMU registers, and no counters are exposed.

Well, isn't it the rule that don't break the userspace? qemu works fine with
KVM_ARM_PMU=n until this commit.


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

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

[Marc Zyngier]

On 2021-01-04 16:22, Qian Cai wrote:
> On Mon, 2021-01-04 at 16:08 +0000, Marc Zyngier wrote:
>> On 2021-01-04 15:47, Qian Cai wrote:
>> > On Thu, 2020-12-10 at 08:30 +0000, 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>
>> >
>> > Reverting this commit on the top of today's linux-next fixed a qemu-kvm
>> > coredump
>> > issue on TX2 while starting a guest.
>> >
>> > - host kernel .config:
>> > https://cailca.coding.net/public/linux/mm/git/files/master/arm64.config
>> >
>> > # /usr/libexec/qemu-kvm -name ubuntu-20.04-server-cloudimg -cpu host
>> > -smp 2 -m 2g
>> > -drive
>> > if=none,format=qcow2,file=./ubuntu-20.04-server-cloudimg.qcow2,id=hd
>> > -device virtio-scsi -device scsi-hd,drive=hd -cdrom
>> > ./ubuntu-20.04-server-cloudimg.iso
>> > -bios /usr/share/AAVMF/AAVMF_CODE.fd -M gic-version=host -nographic
>> > -nic user,model=virtio,hostfwd=tcp::2222-:22
>> >
>> > qemu-kvm: /builddir/build/BUILD/qemu-4.2.0/target/arm/helper.c:1812:
>> > pmevcntr_rawwrite: Assertion `counter < pmu_num_counters(env)' failed.
>> 
>> You don't have KVM_ARM_PMU selected in your config, so QEMU cannot
>> access the PMU registers, and no counters are exposed.
> 
> Well, isn't it the rule that don't break the userspace? qemu works fine 
> with
> KVM_ARM_PMU=n until this commit.

No, it doesn't "work fine". It gets random data that potentially makes 
no sense,
depending on the HW this runs on.

Now, userspace tells you that your kernel is misconfigured. I see it as
an improvement.

         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

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

[Qian Cai]

On Mon, 2021-01-04 at 16:27 +0000, Marc Zyngier wrote:
> On 2021-01-04 16:22, Qian Cai wrote:
> > On Mon, 2021-01-04 at 16:08 +0000, Marc Zyngier wrote:
> > > On 2021-01-04 15:47, Qian Cai wrote:
> > > > On Thu, 2020-12-10 at 08:30 +0000, 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>
> > > > 
> > > > Reverting this commit on the top of today's linux-next fixed a qemu-kvm
> > > > coredump
> > > > issue on TX2 while starting a guest.
> > > > 
> > > > - host kernel .config:
> > > > https://cailca.coding.net/public/linux/mm/git/files/master/arm64.config
> > > > 
> > > > # /usr/libexec/qemu-kvm -name ubuntu-20.04-server-cloudimg -cpu host
> > > > -smp 2 -m 2g
> > > > -drive
> > > > if=none,format=qcow2,file=./ubuntu-20.04-server-cloudimg.qcow2,id=hd
> > > > -device virtio-scsi -device scsi-hd,drive=hd -cdrom
> > > > ./ubuntu-20.04-server-cloudimg.iso
> > > > -bios /usr/share/AAVMF/AAVMF_CODE.fd -M gic-version=host -nographic
> > > > -nic user,model=virtio,hostfwd=tcp::2222-:22
> > > > 
> > > > qemu-kvm: /builddir/build/BUILD/qemu-4.2.0/target/arm/helper.c:1812:
> > > > pmevcntr_rawwrite: Assertion `counter < pmu_num_counters(env)' failed.
> > > 
> > > You don't have KVM_ARM_PMU selected in your config, so QEMU cannot
> > > access the PMU registers, and no counters are exposed.
> > 
> > Well, isn't it the rule that don't break the userspace? qemu works fine 
> > with
> > KVM_ARM_PMU=n until this commit.
> 
> No, it doesn't "work fine". It gets random data that potentially makes 
> no sense,
> depending on the HW this runs on.
> 
> Now, userspace tells you that your kernel is misconfigured. I see it as
> an improvement.

Marc, do you suggest that CONFIG_KVM=y should select KVM_ARM_PMU=y then?
Otherwise, this is rather difficult for users to figure out and a core dump with
an implicit error message from qemu is not that helpful.


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

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

[Marc Zyngier]

On 2021-01-04 18:20, Qian Cai wrote:
> On Mon, 2021-01-04 at 16:27 +0000, Marc Zyngier wrote:
>> On 2021-01-04 16:22, Qian Cai wrote:
>> > On Mon, 2021-01-04 at 16:08 +0000, Marc Zyngier wrote:
>> > > On 2021-01-04 15:47, Qian Cai wrote:
>> > > > On Thu, 2020-12-10 at 08:30 +0000, 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>
>> > > >
>> > > > Reverting this commit on the top of today's linux-next fixed a qemu-kvm
>> > > > coredump
>> > > > issue on TX2 while starting a guest.
>> > > >
>> > > > - host kernel .config:
>> > > > https://cailca.coding.net/public/linux/mm/git/files/master/arm64.config
>> > > >
>> > > > # /usr/libexec/qemu-kvm -name ubuntu-20.04-server-cloudimg -cpu host
>> > > > -smp 2 -m 2g
>> > > > -drive
>> > > > if=none,format=qcow2,file=./ubuntu-20.04-server-cloudimg.qcow2,id=hd
>> > > > -device virtio-scsi -device scsi-hd,drive=hd -cdrom
>> > > > ./ubuntu-20.04-server-cloudimg.iso
>> > > > -bios /usr/share/AAVMF/AAVMF_CODE.fd -M gic-version=host -nographic
>> > > > -nic user,model=virtio,hostfwd=tcp::2222-:22
>> > > >
>> > > > qemu-kvm: /builddir/build/BUILD/qemu-4.2.0/target/arm/helper.c:1812:
>> > > > pmevcntr_rawwrite: Assertion `counter < pmu_num_counters(env)' failed.
>> > >
>> > > You don't have KVM_ARM_PMU selected in your config, so QEMU cannot
>> > > access the PMU registers, and no counters are exposed.
>> >
>> > Well, isn't it the rule that don't break the userspace? qemu works fine
>> > with
>> > KVM_ARM_PMU=n until this commit.
>> 
>> No, it doesn't "work fine". It gets random data that potentially makes
>> no sense,
>> depending on the HW this runs on.
>> 
>> Now, userspace tells you that your kernel is misconfigured. I see it 
>> as
>> an improvement.
> 
> Marc, do you suggest that CONFIG_KVM=y should select KVM_ARM_PMU=y 
> then?
> Otherwise, this is rather difficult for users to figure out and a core 
> dump with
> an implicit error message from qemu is not that helpful.

What I'm suggesting is this [1], which is to get rid of KVM_ARM_PMU
completely. At least, the kernel configuration will be consistent.

Overall, I think there is an issue with KVM exposing more than it
should to userspace when no PMU is defined, but I don't think that's
the problem you are seeing.

         M.

[1] https://lore.kernel.org/r/20210104172723.2014324-1-maz@kernel.org
-- 
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

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

[Qian Cai]

On Mon, 2021-01-04 at 18:26 +0000, Marc Zyngier wrote:
> What I'm suggesting is this [1], which is to get rid of KVM_ARM_PMU
> completely. At least, the kernel configuration will be consistent.
> 

Do you have a patch for CONFIG_KVM to select HW_PERF_EVENTS then? I could cook
one if not.

> Overall, I think there is an issue with KVM exposing more than it
> should to userspace when no PMU is defined, but I don't think that's
> the problem you are seeing.
> 
>          M.
> 
> [1] https://lore.kernel.org/r/20210104172723.2014324-1-maz@kernel.org


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

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

[Marc Zyngier]

On 2021-01-04 18:42, Qian Cai wrote:
> On Mon, 2021-01-04 at 18:26 +0000, Marc Zyngier wrote:
>> What I'm suggesting is this [1], which is to get rid of KVM_ARM_PMU
>> completely. At least, the kernel configuration will be consistent.
>> 
> 
> Do you have a patch for CONFIG_KVM to select HW_PERF_EVENTS then? I 
> could cook
> one if not.

I don't think there should be such a patch. People do disable
HW_PERF_EVENTS in production in some cases, and we should
honor that. All I am trying to guarantee at the moment is
that the KVM configuration is consistent, as I believe that's
what broke in your particular case.

What needs doing is to hide the PMU registers from userspace
when no PMU is configured, or even available. I'll try and post
something to that effect tomorrow (hey, I'm still officially
on holiday...).

         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