| Message ID | 20230527040236.1875860-1-reijiw@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | KVM: arm64: PMU: Fix PMUVer handling on heterogeneous PMU systems | expand |
On Sat, 27 May 2023 05:02:32 +0100, Reiji Watanabe <reijiw@google.com> wrote: > > This series fixes issues with PMUVer handling for a guest with > PMU configured on heterogeneous PMU systems. > Specifically, it addresses the following two issues. > > [A] The default value of ID_AA64DFR0_EL1.PMUVer of the vCPU is set > to its sanitized value. This could be inappropriate on > heterogeneous PMU systems, as arm64_ftr_bits for PMUVer is defined > as FTR_EXACT with safe_val == 0 (when ID_AA64DFR0_EL1.PMUVer of all > PEs on the host is not uniform, the sanitized value will be 0). Why is this a problem? The CPUs don't implement the same version of the architecture, we don't get a PMU. Why should we try to do anything better? I really don't think we should go out or out way and make the code more complicated for something that doesn't really exist. Or am I missing the problem altogether? Thanks, M.
Hi Marc, On Mon, May 29, 2023 at 02:39:28PM +0100, Marc Zyngier wrote: > On Sat, 27 May 2023 05:02:32 +0100, > Reiji Watanabe <reijiw@google.com> wrote: > > > > This series fixes issues with PMUVer handling for a guest with > > PMU configured on heterogeneous PMU systems. > > Specifically, it addresses the following two issues. > > > > [A] The default value of ID_AA64DFR0_EL1.PMUVer of the vCPU is set > > to its sanitized value. This could be inappropriate on > > heterogeneous PMU systems, as arm64_ftr_bits for PMUVer is defined > > as FTR_EXACT with safe_val == 0 (when ID_AA64DFR0_EL1.PMUVer of all > > PEs on the host is not uniform, the sanitized value will be 0). > > Why is this a problem? The CPUs don't implement the same version of > the architecture, we don't get a PMU. Why should we try to do anything > better? I really don't think we should go out or out way and make the > code more complicated for something that doesn't really exist. Even when the CPUs don't implement the same version of the architecture, if one of them implement PMUv3, KVM advertises KVM_CAP_ARM_PMU_V3, and allows userspace to configure PMU (KVM_ARM_VCPU_PMU_V3) for vCPUs. In this case, although KVM provides PMU emulations for the guest, the guest's ID_AA64DFR0_EL1.PMUVer will be zero. Also, KVM_SET_ONE_REG for ID_AA64DFR0_EL1 will never work for vCPUs with PMU configured on such systems (since KVM also doesn't allow userspace to set the PMUVer to 0 for the vCPUs with PMU configured). I would think either ID_AA64DFR0_EL1.PMUVer for the guest should indicate PMUv3, or KVM should not allow userspace to configure PMU, in this case. This series is a fix for the former, mainly to keep the current behavior of KVM_CAP_ARM_PMU_V3 and KVM_ARM_VCPU_PMU_V3 on such systems, since I wasn't sure if such systems don't really exist :) (Also, I plan to implement a similar fix for PMCR_EL0.N on top of those changes) I could make a fix for the latter instead though. What do you think ? Thank you, Reiji > > Or am I missing the problem altogether? > > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible.
Hey Reiji, On Tue, 30 May 2023 13:53:24 +0100, Reiji Watanabe <reijiw@google.com> wrote: > > Hi Marc, > > On Mon, May 29, 2023 at 02:39:28PM +0100, Marc Zyngier wrote: > > On Sat, 27 May 2023 05:02:32 +0100, > > Reiji Watanabe <reijiw@google.com> wrote: > > > > > > This series fixes issues with PMUVer handling for a guest with > > > PMU configured on heterogeneous PMU systems. > > > Specifically, it addresses the following two issues. > > > > > > [A] The default value of ID_AA64DFR0_EL1.PMUVer of the vCPU is set > > > to its sanitized value. This could be inappropriate on > > > heterogeneous PMU systems, as arm64_ftr_bits for PMUVer is defined > > > as FTR_EXACT with safe_val == 0 (when ID_AA64DFR0_EL1.PMUVer of all > > > PEs on the host is not uniform, the sanitized value will be 0). > > > > Why is this a problem? The CPUs don't implement the same version of > > the architecture, we don't get a PMU. Why should we try to do anything > > better? I really don't think we should go out or out way and make the > > code more complicated for something that doesn't really exist. > > Even when the CPUs don't implement the same version of the architecture, > if one of them implement PMUv3, KVM advertises KVM_CAP_ARM_PMU_V3, > and allows userspace to configure PMU (KVM_ARM_VCPU_PMU_V3) for vCPUs. Ah, I see it now. The kernel will register the PMU even if it decides that advertising it is wrong, and then we pick it up. Great :-/. > In this case, although KVM provides PMU emulations for the guest, > the guest's ID_AA64DFR0_EL1.PMUVer will be zero. Also, > KVM_SET_ONE_REG for ID_AA64DFR0_EL1 will never work for vCPUs > with PMU configured on such systems (since KVM also doesn't allow > userspace to set the PMUVer to 0 for the vCPUs with PMU configured). > > I would think either ID_AA64DFR0_EL1.PMUVer for the guest should > indicate PMUv3, or KVM should not allow userspace to configure PMU, > in this case. My vote is on the latter. Even if a PMU is available, we should rely on the feature exposed by the kernel to decide whether exposing a PMU or not. To be honest, this will affect almost nobody (I only know of a single one, an obscure ARMv8.0+ARMv8.2 system which is very unlikely to ever use KVM). I'm happy to take the responsibility to actively break those. > This series is a fix for the former, mainly to keep the current > behavior of KVM_CAP_ARM_PMU_V3 and KVM_ARM_VCPU_PMU_V3 on such > systems, since I wasn't sure if such systems don't really exist :) > (Also, I plan to implement a similar fix for PMCR_EL0.N on top of > those changes) > > I could make a fix for the latter instead though. What do you think ? I think this would be valuable. Also, didn't you have patches for the EL0 side of the PMU? I've been trying to look for a new version, but couldn't find it... Thanks, M.
Hi Marc, On Thu, Jun 01, 2023 at 06:02:41AM +0100, Marc Zyngier wrote: > Hey Reiji, > > On Tue, 30 May 2023 13:53:24 +0100, > Reiji Watanabe <reijiw@google.com> wrote: > > > > Hi Marc, > > > > On Mon, May 29, 2023 at 02:39:28PM +0100, Marc Zyngier wrote: > > > On Sat, 27 May 2023 05:02:32 +0100, > > > Reiji Watanabe <reijiw@google.com> wrote: > > > > > > > > This series fixes issues with PMUVer handling for a guest with > > > > PMU configured on heterogeneous PMU systems. > > > > Specifically, it addresses the following two issues. > > > > > > > > [A] The default value of ID_AA64DFR0_EL1.PMUVer of the vCPU is set > > > > to its sanitized value. This could be inappropriate on > > > > heterogeneous PMU systems, as arm64_ftr_bits for PMUVer is defined > > > > as FTR_EXACT with safe_val == 0 (when ID_AA64DFR0_EL1.PMUVer of all > > > > PEs on the host is not uniform, the sanitized value will be 0). > > > > > > Why is this a problem? The CPUs don't implement the same version of > > > the architecture, we don't get a PMU. Why should we try to do anything > > > better? I really don't think we should go out or out way and make the > > > code more complicated for something that doesn't really exist. > > > > Even when the CPUs don't implement the same version of the architecture, > > if one of them implement PMUv3, KVM advertises KVM_CAP_ARM_PMU_V3, > > and allows userspace to configure PMU (KVM_ARM_VCPU_PMU_V3) for vCPUs. > > Ah, I see it now. The kernel will register the PMU even if it decides > that advertising it is wrong, and then we pick it up. Great :-/. > > > In this case, although KVM provides PMU emulations for the guest, > > the guest's ID_AA64DFR0_EL1.PMUVer will be zero. Also, > > KVM_SET_ONE_REG for ID_AA64DFR0_EL1 will never work for vCPUs > > with PMU configured on such systems (since KVM also doesn't allow > > userspace to set the PMUVer to 0 for the vCPUs with PMU configured). > > > > I would think either ID_AA64DFR0_EL1.PMUVer for the guest should > > indicate PMUv3, or KVM should not allow userspace to configure PMU, > > in this case. > > My vote is on the latter. Even if a PMU is available, we should rely > on the feature exposed by the kernel to decide whether exposing a PMU > or not. > > To be honest, this will affect almost nobody (I only know of a single > one, an obscure ARMv8.0+ARMv8.2 system which is very unlikely to ever > use KVM). I'm happy to take the responsibility to actively break those. Thank you for the information! Just curious, how about a mix of cores with and without PMU ? (with the same ARMv8.x version) I'm guessing there are very few if any though :) > > > This series is a fix for the former, mainly to keep the current > > behavior of KVM_CAP_ARM_PMU_V3 and KVM_ARM_VCPU_PMU_V3 on such > > systems, since I wasn't sure if such systems don't really exist :) > > (Also, I plan to implement a similar fix for PMCR_EL0.N on top of > > those changes) > > > > I could make a fix for the latter instead though. What do you think ? > > I think this would be valuable. Thank you for the comment! I will go with the latter. > Also, didn't you have patches for the EL0 side of the PMU? I've been > trying to look for a new version, but couldn't find it... While I'm working on fixing the series based on the recent comment from Oliver (https://lore.kernel.org/all/ZG%2Fw95pYjWnMJB62@linux.dev/), I have a new PMU EL0 issue, which blocked my testing of the series. So, I am debugging the new PMU EL0 issue. It appears that arch_perf_update_userpage() defined in drivers/perf/arm_pmuv3.c isn't used, and instead, the weak one in kernel/events/core.c is used. This prevents cap_user_rdpmc (, etc) from being set (This prevented my test program from directly accessing counters). This seems to be caused by the commit 7755cec63ade ("arm64: perf: Move PMUv3 driver to drivers/perf"). I have not yet figured out why the one in arm_pmuv3.c isn't used though (The weak one in core.c seems to take precedence over strong ones under drivers/ somehow...). Anyway, I worked around the new issue for now, and ran the test for my series though. I will post the new version of the EL0 series tomorrow hopefully. Thank you, Reiji > > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible.
On Fri, 02 Jun 2023 06:23:23 +0100, Reiji Watanabe <reijiw@google.com> wrote: > > Hi Marc, > > On Thu, Jun 01, 2023 at 06:02:41AM +0100, Marc Zyngier wrote: > > Hey Reiji, > > > > On Tue, 30 May 2023 13:53:24 +0100, > > Reiji Watanabe <reijiw@google.com> wrote: > > > > > > Hi Marc, > > > > > > On Mon, May 29, 2023 at 02:39:28PM +0100, Marc Zyngier wrote: > > > > On Sat, 27 May 2023 05:02:32 +0100, > > > > Reiji Watanabe <reijiw@google.com> wrote: > > > > > > > > > > This series fixes issues with PMUVer handling for a guest with > > > > > PMU configured on heterogeneous PMU systems. > > > > > Specifically, it addresses the following two issues. > > > > > > > > > > [A] The default value of ID_AA64DFR0_EL1.PMUVer of the vCPU is set > > > > > to its sanitized value. This could be inappropriate on > > > > > heterogeneous PMU systems, as arm64_ftr_bits for PMUVer is defined > > > > > as FTR_EXACT with safe_val == 0 (when ID_AA64DFR0_EL1.PMUVer of all > > > > > PEs on the host is not uniform, the sanitized value will be 0). > > > > > > > > Why is this a problem? The CPUs don't implement the same version of > > > > the architecture, we don't get a PMU. Why should we try to do anything > > > > better? I really don't think we should go out or out way and make the > > > > code more complicated for something that doesn't really exist. > > > > > > Even when the CPUs don't implement the same version of the architecture, > > > if one of them implement PMUv3, KVM advertises KVM_CAP_ARM_PMU_V3, > > > and allows userspace to configure PMU (KVM_ARM_VCPU_PMU_V3) for vCPUs. > > > > Ah, I see it now. The kernel will register the PMU even if it decides > > that advertising it is wrong, and then we pick it up. Great :-/. > > > > > In this case, although KVM provides PMU emulations for the guest, > > > the guest's ID_AA64DFR0_EL1.PMUVer will be zero. Also, > > > KVM_SET_ONE_REG for ID_AA64DFR0_EL1 will never work for vCPUs > > > with PMU configured on such systems (since KVM also doesn't allow > > > userspace to set the PMUVer to 0 for the vCPUs with PMU configured). > > > > > > I would think either ID_AA64DFR0_EL1.PMUVer for the guest should > > > indicate PMUv3, or KVM should not allow userspace to configure PMU, > > > in this case. > > > > My vote is on the latter. Even if a PMU is available, we should rely > > on the feature exposed by the kernel to decide whether exposing a PMU > > or not. > > > > To be honest, this will affect almost nobody (I only know of a single > > one, an obscure ARMv8.0+ARMv8.2 system which is very unlikely to ever > > use KVM). I'm happy to take the responsibility to actively break those. > > Thank you for the information! Just curious, how about a mix of > cores with and without PMU ? (with the same ARMv8.x version) > I'm guessing there are very few if any though :) I don't know of any. Similar things for IMPDEF PMUs. And to be honest, I'd be very tempted to nuke that in KVM as well, because this is one of the worse decision I ever made. > > > This series is a fix for the former, mainly to keep the current > > > behavior of KVM_CAP_ARM_PMU_V3 and KVM_ARM_VCPU_PMU_V3 on such > > > systems, since I wasn't sure if such systems don't really exist :) > > > (Also, I plan to implement a similar fix for PMCR_EL0.N on top of > > > those changes) > > > > > > I could make a fix for the latter instead though. What do you think ? > > > > I think this would be valuable. > > Thank you for the comment! I will go with the latter. Thanks. > > Also, didn't you have patches for the EL0 side of the PMU? I've been > > trying to look for a new version, but couldn't find it... > > While I'm working on fixing the series based on the recent comment from > Oliver (https://lore.kernel.org/all/ZG%2Fw95pYjWnMJB62@linux.dev/), > I have a new PMU EL0 issue, which blocked my testing of the series. > So, I am debugging the new PMU EL0 issue. > > It appears that arch_perf_update_userpage() defined in > drivers/perf/arm_pmuv3.c isn't used, and instead, the weak one in > kernel/events/core.c is used. Wut??? How comes? /me disassembles the kernel: ffff8000082a1ab0 <arch_perf_update_userpage>: ffff8000082a1ab0: d503201f nop ffff8000082a1ab4: d503201f nop ffff8000082a1ab8: d65f03c0 ret ffff8000082a1abc: d503201f nop ffff8000082a1ac0: d503201f nop ffff8000082a1ac4: d503201f nop What the hell is happening here??? > This prevents cap_user_rdpmc (, etc) > from being set (This prevented my test program from directly > accessing counters). This seems to be caused by the commit 7755cec63ade > ("arm64: perf: Move PMUv3 driver to drivers/perf"). It is becoming more puzzling by the minute. > > I have not yet figured out why the one in arm_pmuv3.c isn't used > though (The weak one in core.c seems to take precedence over strong > ones under drivers/ somehow...). > > Anyway, I worked around the new issue for now, and ran the test for > my series though. I will post the new version of the EL0 series > tomorrow hopefully. I have a "fix" for this. It doesn't make any sense, but it seems to work here (GCC 10.2.1 from Debian). Can you please give it a shot? Thanks, M. From 236ac26bd0e03bf2ca3b40471b61a35b02272662 Mon Sep 17 00:00:00 2001 From: Marc Zyngier <maz@kernel.org> Date: Fri, 2 Jun 2023 09:52:25 +0100 Subject: [PATCH] perf/core: Drop __weak attribute on arch-specific prototypes Reiji reports that the arm64 implementation of arch_perf_update_userpage() is now ignored and replaced by the dummy stub in core code. This seems to happen since the PMUv3 driver was moved to driver/perf. As it turns out, dropping the __weak attribute from the *prototype* of the function solves the problem. You're right, this doesn't seem to make much sense. And yet... With this, arm64 is able to enjoy arch_perf_update_userpage() again. And while we're at it, drop the same __weak attribute from the arch_perf_get_page_size() prototype. Reported-by: Reiji Watanabe <reijiw@google.com> Signed-off-by: Marc Zyngier <maz@kernel.org> --- include/linux/perf_event.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h index d5628a7b5eaa..1509aea69a16 100644 --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h @@ -1845,12 +1845,12 @@ int perf_event_exit_cpu(unsigned int cpu); #define perf_event_exit_cpu NULL #endif -extern void __weak arch_perf_update_userpage(struct perf_event *event, - struct perf_event_mmap_page *userpg, - u64 now); +extern void arch_perf_update_userpage(struct perf_event *event, + struct perf_event_mmap_page *userpg, + u64 now); #ifdef CONFIG_MMU -extern __weak u64 arch_perf_get_page_size(struct mm_struct *mm, unsigned long addr); +extern u64 arch_perf_get_page_size(struct mm_struct *mm, unsigned long addr); #endif /*
> > > Also, didn't you have patches for the EL0 side of the PMU? I've been > > > trying to look for a new version, but couldn't find it... > > > > While I'm working on fixing the series based on the recent comment from > > Oliver (https://lore.kernel.org/all/ZG%2Fw95pYjWnMJB62@linux.dev/), > > I have a new PMU EL0 issue, which blocked my testing of the series. > > So, I am debugging the new PMU EL0 issue. > > > > It appears that arch_perf_update_userpage() defined in > > drivers/perf/arm_pmuv3.c isn't used, and instead, the weak one in > > kernel/events/core.c is used. > > Wut??? How comes? /me disassembles the kernel: > > ffff8000082a1ab0 <arch_perf_update_userpage>: > ffff8000082a1ab0: d503201f nop > ffff8000082a1ab4: d503201f nop > ffff8000082a1ab8: d65f03c0 ret > ffff8000082a1abc: d503201f nop > ffff8000082a1ac0: d503201f nop > ffff8000082a1ac4: d503201f nop > > What the hell is happening here??? > > > This prevents cap_user_rdpmc (, etc) > > from being set (This prevented my test program from directly > > accessing counters). This seems to be caused by the commit 7755cec63ade > > ("arm64: perf: Move PMUv3 driver to drivers/perf"). > > It is becoming more puzzling by the minute. > > > > > I have not yet figured out why the one in arm_pmuv3.c isn't used > > though (The weak one in core.c seems to take precedence over strong > > ones under drivers/ somehow...). > > > > Anyway, I worked around the new issue for now, and ran the test for > > my series though. I will post the new version of the EL0 series > > tomorrow hopefully. > > I have a "fix" for this. It doesn't make any sense, but it seems to > work here (GCC 10.2.1 from Debian). Can you please give it a shot? > > Thanks, > > M. > > From 236ac26bd0e03bf2ca3b40471b61a35b02272662 Mon Sep 17 00:00:00 2001 > From: Marc Zyngier <maz@kernel.org> > Date: Fri, 2 Jun 2023 09:52:25 +0100 > Subject: [PATCH] perf/core: Drop __weak attribute on arch-specific prototypes > > Reiji reports that the arm64 implementation of arch_perf_update_userpage() > is now ignored and replaced by the dummy stub in core code. > This seems to happen since the PMUv3 driver was moved to driver/perf. > > As it turns out, dropping the __weak attribute from the *prototype* > of the function solves the problem. You're right, this doesn't seem > to make much sense. And yet... > > With this, arm64 is able to enjoy arch_perf_update_userpage() again. Oh, that's interesting... But, it worked, thank you! (With the patch, the disassembles of the kernel for arch_perf_update_userpage look right, and my EL0 test works fine) > And while we're at it, drop the same __weak attribute from the > arch_perf_get_page_size() prototype. The arch_perf_get_page_size() prototype seems to be unnecessary now (after the commit 8af26be06272 "erf/core: Fix arch_perf_get_page_size()"). So, it appears that we could drop the prototype itself. Thank you, Reiji > > Reported-by: Reiji Watanabe <reijiw@google.com> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > include/linux/perf_event.h | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h > index d5628a7b5eaa..1509aea69a16 100644 > --- a/include/linux/perf_event.h > +++ b/include/linux/perf_event.h > @@ -1845,12 +1845,12 @@ int perf_event_exit_cpu(unsigned int cpu); > #define perf_event_exit_cpu NULL > #endif > > -extern void __weak arch_perf_update_userpage(struct perf_event *event, > - struct perf_event_mmap_page *userpg, > - u64 now); > +extern void arch_perf_update_userpage(struct perf_event *event, > + struct perf_event_mmap_page *userpg, > + u64 now); > > #ifdef CONFIG_MMU > -extern __weak u64 arch_perf_get_page_size(struct mm_struct *mm, unsigned long addr); > +extern u64 arch_perf_get_page_size(struct mm_struct *mm, unsigned long addr); > #endif > > /* > -- > 2.39.2 > > > -- > Without deviation from the norm, progress is not possible.
This series fixes issues with PMUVer handling for a guest with PMU configured on heterogeneous PMU systems. Specifically, it addresses the following two issues. [A] The default value of ID_AA64DFR0_EL1.PMUVer of the vCPU is set to its sanitized value. This could be inappropriate on heterogeneous PMU systems, as arm64_ftr_bits for PMUVer is defined as FTR_EXACT with safe_val == 0 (when ID_AA64DFR0_EL1.PMUVer of all PEs on the host is not uniform, the sanitized value will be 0). [B] KVM uses PMUVer of the PMU hardware that is associated to the guest (kvm->arch.arm_pmu->pmuver) for the guest in some cases, even though userspace might have changed the guest's ID_AA64DFR0_EL1.PMUVer (kvm->arch.dfr0_pmuver.imp). To fix [A], KVM will set the default value of the guest's ID_AA64DFR0_EL1.PMUVer to the PMUVer of the guest's PMU (kvm->arch.arm_pmu->pmuver). To fix [B], KVM will stop using kvm->arch.arm_pmu->pmuver (except for some special cases) and use ID_AA64DFR0_EL1.PMUVer for the guest instead. Patch 1 adds a helper to set a PMU for the guest. This helper will make it easier for the following patches to modify the relevant code. Patch 2 make the default PMU for the guest set on the first vCPU reset. As userspace can get the value of ID_AA64DFR0_EL1 after the initial vCPU reset, this change is to make the default PMUVer value based on the guest's PMU available on the initial vCPU reset. Patch 3 and 4 fix the issue [A] and [B] respectively. The series is based on v6.4-rc3. The patches in this series were originally included as part of [1]. [1] https://lore.kernel.org/all/20230211031506.4159098-1-reijiw@google.com/ Reiji Watanabe (4): KVM: arm64: PMU: Introduce a helper to set the guest's PMU KVM: arm64: PMU: Set the default PMU for the guest on vCPU reset KVM: arm64: PMU: Use PMUVer of the guest's PMU for ID_AA64DFR0.PMUVer KVM: arm64: PMU: Don't use the PMUVer of the PMU set for guest arch/arm64/include/asm/kvm_host.h | 2 + arch/arm64/kvm/arm.c | 6 --- arch/arm64/kvm/pmu-emul.c | 73 +++++++++++++++++++++---------- arch/arm64/kvm/reset.c | 20 ++++++--- arch/arm64/kvm/sys_regs.c | 48 +++++++++++++------- include/kvm/arm_pmu.h | 10 ++++- 6 files changed, 106 insertions(+), 53 deletions(-) base-commit: 44c026a73be8038f03dbdeef028b642880cf1511