| Message ID | 20211117064359.2362060-10-reijiw@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Make CPU ID registers writable by userspace | expand |
Hi Reiji, On 11/17/21 7:43 AM, Reiji Watanabe wrote: > When ID_AA64DFR0_EL1.PMUVER or ID_DFR0_EL1.PERFMON is 0xf, which > means IMPLEMENTATION DEFINED PMU supported, KVM unconditionally > expose the value for the guest as it is. Since KVM doesn't support > IMPLEMENTATION DEFINED PMU for the guest, in that case KVM should > exopse 0x0 (PMU is not implemented) instead. s/exopse/expose > > Change cpuid_feature_cap_perfmon_field() to update the field value > to 0x0 when it is 0xf. is it wrong to expose the guest with a Perfmon value of 0xF? Then the guest should not use it as a PMUv3? Eric > > Fixes: 8e35aa642ee4 ("arm64: cpufeature: Extract capped perfmon fields") > Signed-off-by: Reiji Watanabe <reijiw@google.com> > --- > arch/arm64/include/asm/cpufeature.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h > index ef6be92b1921..fd7ad8193827 100644 > --- a/arch/arm64/include/asm/cpufeature.h > +++ b/arch/arm64/include/asm/cpufeature.h > @@ -553,7 +553,7 @@ cpuid_feature_cap_perfmon_field(u64 features, int field, u64 cap) > > /* Treat IMPLEMENTATION DEFINED functionality as unimplemented */ > if (val == ID_AA64DFR0_PMUVER_IMP_DEF) > - val = 0; > + return (features & ~mask); > > if (val > cap) { > features &= ~mask; >
Hi Eric, On Thu, Nov 25, 2021 at 12:30 PM Eric Auger <eauger@redhat.com> wrote: > > Hi Reiji, > > On 11/17/21 7:43 AM, Reiji Watanabe wrote: > > When ID_AA64DFR0_EL1.PMUVER or ID_DFR0_EL1.PERFMON is 0xf, which > > means IMPLEMENTATION DEFINED PMU supported, KVM unconditionally > > expose the value for the guest as it is. Since KVM doesn't support > > IMPLEMENTATION DEFINED PMU for the guest, in that case KVM should > > exopse 0x0 (PMU is not implemented) instead. > s/exopse/expose > > > > Change cpuid_feature_cap_perfmon_field() to update the field value > > to 0x0 when it is 0xf. > is it wrong to expose the guest with a Perfmon value of 0xF? Then the > guest should not use it as a PMUv3? > is it wrong to expose the guest with a Perfmon value of 0xF? Then the > guest should not use it as a PMUv3? For the value 0xf in ID_AA64DFR0_EL1.PMUVER and ID_DFR0_EL1.PERFMON, Arm ARM says: "IMPLEMENTATION DEFINED form of performance monitors supported, PMUv3 not supported." Since the PMU that KVM supports for guests is PMUv3, 0xf shouldn't be exposed to guests (And this patch series doesn't allow userspace to set the fields to 0xf for guests). Thanks, Reiji > > Eric > > > > Fixes: 8e35aa642ee4 ("arm64: cpufeature: Extract capped perfmon fields") > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > > --- > > arch/arm64/include/asm/cpufeature.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h > > index ef6be92b1921..fd7ad8193827 100644 > > --- a/arch/arm64/include/asm/cpufeature.h > > +++ b/arch/arm64/include/asm/cpufeature.h > > @@ -553,7 +553,7 @@ cpuid_feature_cap_perfmon_field(u64 features, int field, u64 cap) > > > > /* Treat IMPLEMENTATION DEFINED functionality as unimplemented */ > > if (val == ID_AA64DFR0_PMUVER_IMP_DEF) > > - val = 0; > > + return (features & ~mask); > > > > if (val > cap) { > > features &= ~mask; > > >
Hi Reiji, On Mon, Nov 29, 2021 at 09:32:02PM -0800, Reiji Watanabe wrote: > Hi Eric, > > On Thu, Nov 25, 2021 at 12:30 PM Eric Auger <eauger@redhat.com> wrote: > > > > Hi Reiji, > > > > On 11/17/21 7:43 AM, Reiji Watanabe wrote: > > > When ID_AA64DFR0_EL1.PMUVER or ID_DFR0_EL1.PERFMON is 0xf, which > > > means IMPLEMENTATION DEFINED PMU supported, KVM unconditionally > > > expose the value for the guest as it is. Since KVM doesn't support > > > IMPLEMENTATION DEFINED PMU for the guest, in that case KVM should > > > exopse 0x0 (PMU is not implemented) instead. > > s/exopse/expose > > > > > > Change cpuid_feature_cap_perfmon_field() to update the field value > > > to 0x0 when it is 0xf. > > is it wrong to expose the guest with a Perfmon value of 0xF? Then the > > guest should not use it as a PMUv3? > > > is it wrong to expose the guest with a Perfmon value of 0xF? Then the > > guest should not use it as a PMUv3? > > For the value 0xf in ID_AA64DFR0_EL1.PMUVER and ID_DFR0_EL1.PERFMON, > Arm ARM says: > "IMPLEMENTATION DEFINED form of performance monitors supported, > PMUv3 not supported." > > Since the PMU that KVM supports for guests is PMUv3, 0xf shouldn't > be exposed to guests (And this patch series doesn't allow userspace > to set the fields to 0xf for guests). While it's true that a value of 0xf means that PMUv3 is not present (both KVM and the PMU driver handle it this way) this is an userspace visible change. Are you sure there isn't software in the wild that relies on this value being 0xf to detect that some non-Arm architected hardware is present? Since both 0 and 0xf are valid values that mean that PMUv3 is not present, I think it's best that both are kept. Thanks, Alex > > Thanks, > Reiji > > > > > Eric > > > > > > Fixes: 8e35aa642ee4 ("arm64: cpufeature: Extract capped perfmon fields") > > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > > > --- > > > arch/arm64/include/asm/cpufeature.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h > > > index ef6be92b1921..fd7ad8193827 100644 > > > --- a/arch/arm64/include/asm/cpufeature.h > > > +++ b/arch/arm64/include/asm/cpufeature.h > > > @@ -553,7 +553,7 @@ cpuid_feature_cap_perfmon_field(u64 features, int field, u64 cap) > > > > > > /* Treat IMPLEMENTATION DEFINED functionality as unimplemented */ > > > if (val == ID_AA64DFR0_PMUVER_IMP_DEF) > > > - val = 0; > > > + return (features & ~mask); > > > > > > if (val > cap) { > > > features &= ~mask; > > > > >
Hi Reiji, On Wed, Dec 01, 2021 at 03:53:18PM +0000, Alexandru Elisei wrote: > Hi Reiji, > > On Mon, Nov 29, 2021 at 09:32:02PM -0800, Reiji Watanabe wrote: > > Hi Eric, > > > > On Thu, Nov 25, 2021 at 12:30 PM Eric Auger <eauger@redhat.com> wrote: > > > > > > Hi Reiji, > > > > > > On 11/17/21 7:43 AM, Reiji Watanabe wrote: > > > > When ID_AA64DFR0_EL1.PMUVER or ID_DFR0_EL1.PERFMON is 0xf, which > > > > means IMPLEMENTATION DEFINED PMU supported, KVM unconditionally > > > > expose the value for the guest as it is. Since KVM doesn't support > > > > IMPLEMENTATION DEFINED PMU for the guest, in that case KVM should > > > > exopse 0x0 (PMU is not implemented) instead. > > > s/exopse/expose > > > > > > > > Change cpuid_feature_cap_perfmon_field() to update the field value > > > > to 0x0 when it is 0xf. > > > is it wrong to expose the guest with a Perfmon value of 0xF? Then the > > > guest should not use it as a PMUv3? > > > > > is it wrong to expose the guest with a Perfmon value of 0xF? Then the > > > guest should not use it as a PMUv3? > > > > For the value 0xf in ID_AA64DFR0_EL1.PMUVER and ID_DFR0_EL1.PERFMON, > > Arm ARM says: > > "IMPLEMENTATION DEFINED form of performance monitors supported, > > PMUv3 not supported." > > > > Since the PMU that KVM supports for guests is PMUv3, 0xf shouldn't > > be exposed to guests (And this patch series doesn't allow userspace > > to set the fields to 0xf for guests). > > While it's true that a value of 0xf means that PMUv3 is not present (both > KVM and the PMU driver handle it this way) this is an userspace visible > change. > > Are you sure there isn't software in the wild that relies on this value > being 0xf to detect that some non-Arm architected hardware is present? > > Since both 0 and 0xf are valid values that mean that PMUv3 is not present, > I think it's best that both are kept. Sorry, somehow I managed to get myself confused and didn't realize that this is only used by KVM. What I said above about the possibility of software existing that pokes IMP DEF registers when PMUVer = 0xf is in fact a good argument for this patch, because KVM injects an undefined exception when a guest tries to access such registers. Thanks, Alex > > Thanks, > Alex > > > > > Thanks, > > Reiji > > > > > > > > Eric > > > > > > > > Fixes: 8e35aa642ee4 ("arm64: cpufeature: Extract capped perfmon fields") > > > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > > > > --- > > > > arch/arm64/include/asm/cpufeature.h | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h > > > > index ef6be92b1921..fd7ad8193827 100644 > > > > --- a/arch/arm64/include/asm/cpufeature.h > > > > +++ b/arch/arm64/include/asm/cpufeature.h > > > > @@ -553,7 +553,7 @@ cpuid_feature_cap_perfmon_field(u64 features, int field, u64 cap) > > > > > > > > /* Treat IMPLEMENTATION DEFINED functionality as unimplemented */ > > > > if (val == ID_AA64DFR0_PMUVER_IMP_DEF) > > > > - val = 0; > > > > + return (features & ~mask); > > > > > > > > if (val > cap) { > > > > features &= ~mask; > > > > > > >
Hi Alex, On Wed, Dec 1, 2021 at 8:09 AM Alexandru Elisei <alexandru.elisei@arm.com> wrote: > > Hi Reiji, > > On Wed, Dec 01, 2021 at 03:53:18PM +0000, Alexandru Elisei wrote: > > Hi Reiji, > > > > On Mon, Nov 29, 2021 at 09:32:02PM -0800, Reiji Watanabe wrote: > > > Hi Eric, > > > > > > On Thu, Nov 25, 2021 at 12:30 PM Eric Auger <eauger@redhat.com> wrote: > > > > > > > > Hi Reiji, > > > > > > > > On 11/17/21 7:43 AM, Reiji Watanabe wrote: > > > > > When ID_AA64DFR0_EL1.PMUVER or ID_DFR0_EL1.PERFMON is 0xf, which > > > > > means IMPLEMENTATION DEFINED PMU supported, KVM unconditionally > > > > > expose the value for the guest as it is. Since KVM doesn't support > > > > > IMPLEMENTATION DEFINED PMU for the guest, in that case KVM should > > > > > exopse 0x0 (PMU is not implemented) instead. > > > > s/exopse/expose > > > > > > > > > > Change cpuid_feature_cap_perfmon_field() to update the field value > > > > > to 0x0 when it is 0xf. > > > > is it wrong to expose the guest with a Perfmon value of 0xF? Then the > > > > guest should not use it as a PMUv3? > > > > > > > is it wrong to expose the guest with a Perfmon value of 0xF? Then the > > > > guest should not use it as a PMUv3? > > > > > > For the value 0xf in ID_AA64DFR0_EL1.PMUVER and ID_DFR0_EL1.PERFMON, > > > Arm ARM says: > > > "IMPLEMENTATION DEFINED form of performance monitors supported, > > > PMUv3 not supported." > > > > > > Since the PMU that KVM supports for guests is PMUv3, 0xf shouldn't > > > be exposed to guests (And this patch series doesn't allow userspace > > > to set the fields to 0xf for guests). > > > > While it's true that a value of 0xf means that PMUv3 is not present (both > > KVM and the PMU driver handle it this way) this is an userspace visible > > change. > > > > Are you sure there isn't software in the wild that relies on this value > > being 0xf to detect that some non-Arm architected hardware is present? > > > > Since both 0 and 0xf are valid values that mean that PMUv3 is not present, > > I think it's best that both are kept. > > Sorry, somehow I managed to get myself confused and didn't realize that > this is only used by KVM. > > What I said above about the possibility of software existing that pokes IMP > DEF registers when PMUVer = 0xf is in fact a good argument for this patch, > because KVM injects an undefined exception when a guest tries to access > such registers. Thank you for your review. My understanding is the same as yours. Thanks, Reiji
Hi Reiji, On 11/30/21 6:32 AM, Reiji Watanabe wrote: > Hi Eric, > > On Thu, Nov 25, 2021 at 12:30 PM Eric Auger <eauger@redhat.com> wrote: >> >> Hi Reiji, >> >> On 11/17/21 7:43 AM, Reiji Watanabe wrote: >>> When ID_AA64DFR0_EL1.PMUVER or ID_DFR0_EL1.PERFMON is 0xf, which >>> means IMPLEMENTATION DEFINED PMU supported, KVM unconditionally >>> expose the value for the guest as it is. Since KVM doesn't support >>> IMPLEMENTATION DEFINED PMU for the guest, in that case KVM should >>> exopse 0x0 (PMU is not implemented) instead. >> s/exopse/expose >>> >>> Change cpuid_feature_cap_perfmon_field() to update the field value >>> to 0x0 when it is 0xf. >> is it wrong to expose the guest with a Perfmon value of 0xF? Then the >> guest should not use it as a PMUv3? > >> is it wrong to expose the guest with a Perfmon value of 0xF? Then the >> guest should not use it as a PMUv3? > > For the value 0xf in ID_AA64DFR0_EL1.PMUVER and ID_DFR0_EL1.PERFMON, > Arm ARM says: > "IMPLEMENTATION DEFINED form of performance monitors supported, > PMUv3 not supported." > > Since the PMU that KVM supports for guests is PMUv3, 0xf shouldn't > be exposed to guests (And this patch series doesn't allow userspace > to set the fields to 0xf for guests). What I don't get is why this isn't detected before (in kvm_reset_vcpu). if the VCPU was initialized with KVM_ARM_VCPU_PMU_V3 can we honor this init request if the host pmu is implementation defined? Thanks Eric > > Thanks, > Reiji > >> >> Eric >>> >>> Fixes: 8e35aa642ee4 ("arm64: cpufeature: Extract capped perfmon fields") >>> Signed-off-by: Reiji Watanabe <reijiw@google.com> >>> --- >>> arch/arm64/include/asm/cpufeature.h | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h >>> index ef6be92b1921..fd7ad8193827 100644 >>> --- a/arch/arm64/include/asm/cpufeature.h >>> +++ b/arch/arm64/include/asm/cpufeature.h >>> @@ -553,7 +553,7 @@ cpuid_feature_cap_perfmon_field(u64 features, int field, u64 cap) >>> >>> /* Treat IMPLEMENTATION DEFINED functionality as unimplemented */ >>> if (val == ID_AA64DFR0_PMUVER_IMP_DEF) >>> - val = 0; >>> + return (features & ~mask); >>> >>> if (val > cap) { >>> features &= ~mask; >>> >> > _______________________________________________ > kvmarm mailing list > kvmarm@lists.cs.columbia.edu > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm >
Hi Eric, On Thu, Dec 2, 2021 at 2:57 AM Eric Auger <eauger@redhat.com> wrote: > > Hi Reiji, > > On 11/30/21 6:32 AM, Reiji Watanabe wrote: > > Hi Eric, > > > > On Thu, Nov 25, 2021 at 12:30 PM Eric Auger <eauger@redhat.com> wrote: > >> > >> Hi Reiji, > >> > >> On 11/17/21 7:43 AM, Reiji Watanabe wrote: > >>> When ID_AA64DFR0_EL1.PMUVER or ID_DFR0_EL1.PERFMON is 0xf, which > >>> means IMPLEMENTATION DEFINED PMU supported, KVM unconditionally > >>> expose the value for the guest as it is. Since KVM doesn't support > >>> IMPLEMENTATION DEFINED PMU for the guest, in that case KVM should > >>> exopse 0x0 (PMU is not implemented) instead. > >> s/exopse/expose > >>> > >>> Change cpuid_feature_cap_perfmon_field() to update the field value > >>> to 0x0 when it is 0xf. > >> is it wrong to expose the guest with a Perfmon value of 0xF? Then the > >> guest should not use it as a PMUv3? > > > >> is it wrong to expose the guest with a Perfmon value of 0xF? Then the > >> guest should not use it as a PMUv3? > > > > For the value 0xf in ID_AA64DFR0_EL1.PMUVER and ID_DFR0_EL1.PERFMON, > > Arm ARM says: > > "IMPLEMENTATION DEFINED form of performance monitors supported, > > PMUv3 not supported." > > > > Since the PMU that KVM supports for guests is PMUv3, 0xf shouldn't > > be exposed to guests (And this patch series doesn't allow userspace > > to set the fields to 0xf for guests). > What I don't get is why this isn't detected before (in kvm_reset_vcpu). > if the VCPU was initialized with KVM_ARM_VCPU_PMU_V3 can we honor this > init request if the host pmu is implementation defined? KVM_ARM_VCPU_INIT with KVM_ARM_VCPU_PMU_V3 will fail in kvm_reset_vcpu() if the host PMU is implementation defined. The AA64DFR0 and DFR0 registers for a vCPU without KVM_ARM_VCPU_PMU_V3 indicates IMPLEMENTATION DEFINED PMU support, which is not correct. Thanks, Reiji
Hi Reiji, On 12/4/21 2:04 AM, Reiji Watanabe wrote: > Hi Eric, > > On Thu, Dec 2, 2021 at 2:57 AM Eric Auger <eauger@redhat.com> wrote: >> >> Hi Reiji, >> >> On 11/30/21 6:32 AM, Reiji Watanabe wrote: >>> Hi Eric, >>> >>> On Thu, Nov 25, 2021 at 12:30 PM Eric Auger <eauger@redhat.com> wrote: >>>> >>>> Hi Reiji, >>>> >>>> On 11/17/21 7:43 AM, Reiji Watanabe wrote: >>>>> When ID_AA64DFR0_EL1.PMUVER or ID_DFR0_EL1.PERFMON is 0xf, which >>>>> means IMPLEMENTATION DEFINED PMU supported, KVM unconditionally >>>>> expose the value for the guest as it is. Since KVM doesn't support >>>>> IMPLEMENTATION DEFINED PMU for the guest, in that case KVM should >>>>> exopse 0x0 (PMU is not implemented) instead. >>>> s/exopse/expose >>>>> >>>>> Change cpuid_feature_cap_perfmon_field() to update the field value >>>>> to 0x0 when it is 0xf. >>>> is it wrong to expose the guest with a Perfmon value of 0xF? Then the >>>> guest should not use it as a PMUv3? >>> >>>> is it wrong to expose the guest with a Perfmon value of 0xF? Then the >>>> guest should not use it as a PMUv3? >>> >>> For the value 0xf in ID_AA64DFR0_EL1.PMUVER and ID_DFR0_EL1.PERFMON, >>> Arm ARM says: >>> "IMPLEMENTATION DEFINED form of performance monitors supported, >>> PMUv3 not supported." >>> >>> Since the PMU that KVM supports for guests is PMUv3, 0xf shouldn't >>> be exposed to guests (And this patch series doesn't allow userspace >>> to set the fields to 0xf for guests). >> What I don't get is why this isn't detected before (in kvm_reset_vcpu). >> if the VCPU was initialized with KVM_ARM_VCPU_PMU_V3 can we honor this >> init request if the host pmu is implementation defined? > > KVM_ARM_VCPU_INIT with KVM_ARM_VCPU_PMU_V3 will fail in > kvm_reset_vcpu() if the host PMU is implementation defined. OK. This was not obvsious to me. if (kvm_vcpu_has_pmu(vcpu) && !kvm_arm_support_pmu_v3()) { ret = -EINVAL; goto out; } kvm_perf_init + if (perf_num_counters() > 0) + static_branch_enable(&kvm_arm_pmu_available); But I believe you ;-), sorry for the noise Eric > > The AA64DFR0 and DFR0 registers for a vCPU without KVM_ARM_VCPU_PMU_V3 > indicates IMPLEMENTATION DEFINED PMU support, which is not correct. > > Thanks, > Reiji >
Hi Eric, On Sat, Dec 4, 2021 at 6:14 AM Eric Auger <eauger@redhat.com> wrote: > > Hi Reiji, > > On 12/4/21 2:04 AM, Reiji Watanabe wrote: > > Hi Eric, > > > > On Thu, Dec 2, 2021 at 2:57 AM Eric Auger <eauger@redhat.com> wrote: > >> > >> Hi Reiji, > >> > >> On 11/30/21 6:32 AM, Reiji Watanabe wrote: > >>> Hi Eric, > >>> > >>> On Thu, Nov 25, 2021 at 12:30 PM Eric Auger <eauger@redhat.com> wrote: > >>>> > >>>> Hi Reiji, > >>>> > >>>> On 11/17/21 7:43 AM, Reiji Watanabe wrote: > >>>>> When ID_AA64DFR0_EL1.PMUVER or ID_DFR0_EL1.PERFMON is 0xf, which > >>>>> means IMPLEMENTATION DEFINED PMU supported, KVM unconditionally > >>>>> expose the value for the guest as it is. Since KVM doesn't support > >>>>> IMPLEMENTATION DEFINED PMU for the guest, in that case KVM should > >>>>> exopse 0x0 (PMU is not implemented) instead. > >>>> s/exopse/expose > >>>>> > >>>>> Change cpuid_feature_cap_perfmon_field() to update the field value > >>>>> to 0x0 when it is 0xf. > >>>> is it wrong to expose the guest with a Perfmon value of 0xF? Then the > >>>> guest should not use it as a PMUv3? > >>> > >>>> is it wrong to expose the guest with a Perfmon value of 0xF? Then the > >>>> guest should not use it as a PMUv3? > >>> > >>> For the value 0xf in ID_AA64DFR0_EL1.PMUVER and ID_DFR0_EL1.PERFMON, > >>> Arm ARM says: > >>> "IMPLEMENTATION DEFINED form of performance monitors supported, > >>> PMUv3 not supported." > >>> > >>> Since the PMU that KVM supports for guests is PMUv3, 0xf shouldn't > >>> be exposed to guests (And this patch series doesn't allow userspace > >>> to set the fields to 0xf for guests). > >> What I don't get is why this isn't detected before (in kvm_reset_vcpu). > >> if the VCPU was initialized with KVM_ARM_VCPU_PMU_V3 can we honor this > >> init request if the host pmu is implementation defined? > > > > KVM_ARM_VCPU_INIT with KVM_ARM_VCPU_PMU_V3 will fail in > > kvm_reset_vcpu() if the host PMU is implementation defined. > > OK. This was not obvsious to me. > > if (kvm_vcpu_has_pmu(vcpu) && !kvm_arm_support_pmu_v3()) { > ret = -EINVAL; > goto out; > } > > kvm_perf_init > + if (perf_num_counters() > 0) > + static_branch_enable(&kvm_arm_pmu_available); > > But I believe you ;-), sorry for the noise Thank you for the review ! I didn't find the code above in v5.16-rc3, which is the base code of this series. So, I'm not sure where the code came from (any kvmarm repository branch ??). What I see in v5.16-rc3 is: ---- int kvm_perf_init(void) { return perf_register_guest_info_callbacks(&kvm_guest_cbs); } void kvm_host_pmu_init(struct arm_pmu *pmu) { if (pmu->pmuver != 0 && pmu->pmuver != ID_AA64DFR0_PMUVER_IMP_DEF && !kvm_arm_support_pmu_v3() && !is_protected_kvm_enabled()) static_branch_enable(&kvm_arm_pmu_available); } ---- And I don't find any other code that enables kvm_arm_pmu_available. Looking at the KVM's PMUV3 support code for guests in v5.16-rc3, if KVM allows userspace to configure KVM_ARM_VCPU_PMU_V3 even with ID_AA64DFR0_PMUVER_IMP_DEF on the host (, which I don't think it does), I think we should fix that to not allow that. (I'm not sure how KVM's PMUV3 support code is implemented in the code that you are looking at though) Thanks, Reiji
Reiji, GMail keeps marking your email as spam, any ideas? On Sun, Dec 5, 2021 at 2:43 AM Reiji Watanabe <reijiw@google.com> wrote: > > Hi Eric, > > On Sat, Dec 4, 2021 at 6:14 AM Eric Auger <eauger@redhat.com> wrote: > > > > Hi Reiji, > > > > On 12/4/21 2:04 AM, Reiji Watanabe wrote: > > > Hi Eric, > > > > > > On Thu, Dec 2, 2021 at 2:57 AM Eric Auger <eauger@redhat.com> wrote: > > >> > > >> Hi Reiji, > > >> > > >> On 11/30/21 6:32 AM, Reiji Watanabe wrote: > > >>> Hi Eric, > > >>> > > >>> On Thu, Nov 25, 2021 at 12:30 PM Eric Auger <eauger@redhat.com> wrote: > > >>>> > > >>>> Hi Reiji, > > >>>> > > >>>> On 11/17/21 7:43 AM, Reiji Watanabe wrote: > > >>>>> When ID_AA64DFR0_EL1.PMUVER or ID_DFR0_EL1.PERFMON is 0xf, which > > >>>>> means IMPLEMENTATION DEFINED PMU supported, KVM unconditionally > > >>>>> expose the value for the guest as it is. Since KVM doesn't support > > >>>>> IMPLEMENTATION DEFINED PMU for the guest, in that case KVM should > > >>>>> exopse 0x0 (PMU is not implemented) instead. > > >>>> s/exopse/expose > > >>>>> > > >>>>> Change cpuid_feature_cap_perfmon_field() to update the field value > > >>>>> to 0x0 when it is 0xf. > > >>>> is it wrong to expose the guest with a Perfmon value of 0xF? Then the > > >>>> guest should not use it as a PMUv3? > > >>> > > >>>> is it wrong to expose the guest with a Perfmon value of 0xF? Then the > > >>>> guest should not use it as a PMUv3? > > >>> > > >>> For the value 0xf in ID_AA64DFR0_EL1.PMUVER and ID_DFR0_EL1.PERFMON, > > >>> Arm ARM says: > > >>> "IMPLEMENTATION DEFINED form of performance monitors supported, > > >>> PMUv3 not supported." > > >>> > > >>> Since the PMU that KVM supports for guests is PMUv3, 0xf shouldn't > > >>> be exposed to guests (And this patch series doesn't allow userspace > > >>> to set the fields to 0xf for guests). > > >> What I don't get is why this isn't detected before (in kvm_reset_vcpu). > > >> if the VCPU was initialized with KVM_ARM_VCPU_PMU_V3 can we honor this > > >> init request if the host pmu is implementation defined? > > > > > > KVM_ARM_VCPU_INIT with KVM_ARM_VCPU_PMU_V3 will fail in > > > kvm_reset_vcpu() if the host PMU is implementation defined. > > > > OK. This was not obvsious to me. > > > > if (kvm_vcpu_has_pmu(vcpu) && !kvm_arm_support_pmu_v3()) { > > ret = -EINVAL; > > goto out; > > } > > > > kvm_perf_init > > + if (perf_num_counters() > 0) > > + static_branch_enable(&kvm_arm_pmu_available); > > > > But I believe you ;-), sorry for the noise > > Thank you for the review ! > > I didn't find the code above in v5.16-rc3, which is the base code of > this series. So, I'm not sure where the code came from (any kvmarm > repository branch ??). > > What I see in v5.16-rc3 is: > ---- > int kvm_perf_init(void) > { > return perf_register_guest_info_callbacks(&kvm_guest_cbs); > } > > void kvm_host_pmu_init(struct arm_pmu *pmu) > { > if (pmu->pmuver != 0 && pmu->pmuver != ID_AA64DFR0_PMUVER_IMP_DEF && > !kvm_arm_support_pmu_v3() && !is_protected_kvm_enabled()) > static_branch_enable(&kvm_arm_pmu_available); > } > ---- > > And I don't find any other code that enables kvm_arm_pmu_available. > > Looking at the KVM's PMUV3 support code for guests in v5.16-rc3, > if KVM allows userspace to configure KVM_ARM_VCPU_PMU_V3 even with > ID_AA64DFR0_PMUVER_IMP_DEF on the host (, which I don't think it does), > I think we should fix that to not allow that. > (I'm not sure how KVM's PMUV3 support code is implemented in the > code that you are looking at though) > > Thanks, > Reiji > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Itaru, No, I have no idea... (I just noticed that I also had a lot of kvmarm emails that were treated as spam...) Thanks, Reiji On Sat, Dec 4, 2021 at 3:38 PM Itaru Kitayama <itaru.kitayama@gmail.com> wrote: > > Reiji, > GMail keeps marking your email as spam, any ideas? > > On Sun, Dec 5, 2021 at 2:43 AM Reiji Watanabe <reijiw@google.com> wrote: > > > > Hi Eric, > > > > On Sat, Dec 4, 2021 at 6:14 AM Eric Auger <eauger@redhat.com> wrote: > > > > > > Hi Reiji, > > > > > > On 12/4/21 2:04 AM, Reiji Watanabe wrote: > > > > Hi Eric, > > > > > > > > On Thu, Dec 2, 2021 at 2:57 AM Eric Auger <eauger@redhat.com> wrote: > > > >> > > > >> Hi Reiji, > > > >> > > > >> On 11/30/21 6:32 AM, Reiji Watanabe wrote: > > > >>> Hi Eric, > > > >>> > > > >>> On Thu, Nov 25, 2021 at 12:30 PM Eric Auger <eauger@redhat.com> wrote: > > > >>>> > > > >>>> Hi Reiji, > > > >>>> > > > >>>> On 11/17/21 7:43 AM, Reiji Watanabe wrote: > > > >>>>> When ID_AA64DFR0_EL1.PMUVER or ID_DFR0_EL1.PERFMON is 0xf, which > > > >>>>> means IMPLEMENTATION DEFINED PMU supported, KVM unconditionally > > > >>>>> expose the value for the guest as it is. Since KVM doesn't support > > > >>>>> IMPLEMENTATION DEFINED PMU for the guest, in that case KVM should > > > >>>>> exopse 0x0 (PMU is not implemented) instead. > > > >>>> s/exopse/expose > > > >>>>> > > > >>>>> Change cpuid_feature_cap_perfmon_field() to update the field value > > > >>>>> to 0x0 when it is 0xf. > > > >>>> is it wrong to expose the guest with a Perfmon value of 0xF? Then the > > > >>>> guest should not use it as a PMUv3? > > > >>> > > > >>>> is it wrong to expose the guest with a Perfmon value of 0xF? Then the > > > >>>> guest should not use it as a PMUv3? > > > >>> > > > >>> For the value 0xf in ID_AA64DFR0_EL1.PMUVER and ID_DFR0_EL1.PERFMON, > > > >>> Arm ARM says: > > > >>> "IMPLEMENTATION DEFINED form of performance monitors supported, > > > >>> PMUv3 not supported." > > > >>> > > > >>> Since the PMU that KVM supports for guests is PMUv3, 0xf shouldn't > > > >>> be exposed to guests (And this patch series doesn't allow userspace > > > >>> to set the fields to 0xf for guests). > > > >> What I don't get is why this isn't detected before (in kvm_reset_vcpu). > > > >> if the VCPU was initialized with KVM_ARM_VCPU_PMU_V3 can we honor this > > > >> init request if the host pmu is implementation defined? > > > > > > > > KVM_ARM_VCPU_INIT with KVM_ARM_VCPU_PMU_V3 will fail in > > > > kvm_reset_vcpu() if the host PMU is implementation defined. > > > > > > OK. This was not obvsious to me. > > > > > > if (kvm_vcpu_has_pmu(vcpu) && !kvm_arm_support_pmu_v3()) { > > > ret = -EINVAL; > > > goto out; > > > } > > > > > > kvm_perf_init > > > + if (perf_num_counters() > 0) > > > + static_branch_enable(&kvm_arm_pmu_available); > > > > > > But I believe you ;-), sorry for the noise > > > > Thank you for the review ! > > > > I didn't find the code above in v5.16-rc3, which is the base code of > > this series. So, I'm not sure where the code came from (any kvmarm > > repository branch ??). > > > > What I see in v5.16-rc3 is: > > ---- > > int kvm_perf_init(void) > > { > > return perf_register_guest_info_callbacks(&kvm_guest_cbs); > > } > > > > void kvm_host_pmu_init(struct arm_pmu *pmu) > > { > > if (pmu->pmuver != 0 && pmu->pmuver != ID_AA64DFR0_PMUVER_IMP_DEF && > > !kvm_arm_support_pmu_v3() && !is_protected_kvm_enabled()) > > static_branch_enable(&kvm_arm_pmu_available); > > } > > ---- > > > > And I don't find any other code that enables kvm_arm_pmu_available. > > > > Looking at the KVM's PMUV3 support code for guests in v5.16-rc3, > > if KVM allows userspace to configure KVM_ARM_VCPU_PMU_V3 even with > > ID_AA64DFR0_PMUVER_IMP_DEF on the host (, which I don't think it does), > > I think we should fix that to not allow that. > > (I'm not sure how KVM's PMUV3 support code is implemented in the > > code that you are looking at though) > > > > Thanks, > > Reiji > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi, On Sat, Dec 04, 2021 at 09:39:59AM -0800, Reiji Watanabe wrote: > Hi Eric, > > On Sat, Dec 4, 2021 at 6:14 AM Eric Auger <eauger@redhat.com> wrote: > > > > Hi Reiji, > > > > On 12/4/21 2:04 AM, Reiji Watanabe wrote: > > > Hi Eric, > > > > > > On Thu, Dec 2, 2021 at 2:57 AM Eric Auger <eauger@redhat.com> wrote: > > >> > > >> Hi Reiji, > > >> > > >> On 11/30/21 6:32 AM, Reiji Watanabe wrote: > > >>> Hi Eric, > > >>> > > >>> On Thu, Nov 25, 2021 at 12:30 PM Eric Auger <eauger@redhat.com> wrote: > > >>>> > > >>>> Hi Reiji, > > >>>> > > >>>> On 11/17/21 7:43 AM, Reiji Watanabe wrote: > > >>>>> When ID_AA64DFR0_EL1.PMUVER or ID_DFR0_EL1.PERFMON is 0xf, which > > >>>>> means IMPLEMENTATION DEFINED PMU supported, KVM unconditionally > > >>>>> expose the value for the guest as it is. Since KVM doesn't support > > >>>>> IMPLEMENTATION DEFINED PMU for the guest, in that case KVM should > > >>>>> exopse 0x0 (PMU is not implemented) instead. > > >>>> s/exopse/expose > > >>>>> > > >>>>> Change cpuid_feature_cap_perfmon_field() to update the field value > > >>>>> to 0x0 when it is 0xf. > > >>>> is it wrong to expose the guest with a Perfmon value of 0xF? Then the > > >>>> guest should not use it as a PMUv3? > > >>> > > >>>> is it wrong to expose the guest with a Perfmon value of 0xF? Then the > > >>>> guest should not use it as a PMUv3? > > >>> > > >>> For the value 0xf in ID_AA64DFR0_EL1.PMUVER and ID_DFR0_EL1.PERFMON, > > >>> Arm ARM says: > > >>> "IMPLEMENTATION DEFINED form of performance monitors supported, > > >>> PMUv3 not supported." > > >>> > > >>> Since the PMU that KVM supports for guests is PMUv3, 0xf shouldn't > > >>> be exposed to guests (And this patch series doesn't allow userspace > > >>> to set the fields to 0xf for guests). > > >> What I don't get is why this isn't detected before (in kvm_reset_vcpu). > > >> if the VCPU was initialized with KVM_ARM_VCPU_PMU_V3 can we honor this > > >> init request if the host pmu is implementation defined? > > > > > > KVM_ARM_VCPU_INIT with KVM_ARM_VCPU_PMU_V3 will fail in > > > kvm_reset_vcpu() if the host PMU is implementation defined. > > > > OK. This was not obvsious to me. > > > > if (kvm_vcpu_has_pmu(vcpu) && !kvm_arm_support_pmu_v3()) { > > ret = -EINVAL; > > goto out; > > } > > > > kvm_perf_init > > + if (perf_num_counters() > 0) > > + static_branch_enable(&kvm_arm_pmu_available); > > > > But I believe you ;-), sorry for the noise > > Thank you for the review ! > > I didn't find the code above in v5.16-rc3, which is the base code of > this series. So, I'm not sure where the code came from (any kvmarm > repository branch ??). > > What I see in v5.16-rc3 is: > ---- > int kvm_perf_init(void) > { > return perf_register_guest_info_callbacks(&kvm_guest_cbs); > } > > void kvm_host_pmu_init(struct arm_pmu *pmu) > { > if (pmu->pmuver != 0 && pmu->pmuver != ID_AA64DFR0_PMUVER_IMP_DEF && > !kvm_arm_support_pmu_v3() && !is_protected_kvm_enabled()) > static_branch_enable(&kvm_arm_pmu_available); > } > ---- > > And I don't find any other code that enables kvm_arm_pmu_available. The code was recently changed (in v5.15 I think), I think Eric is looking at an older version. > > Looking at the KVM's PMUV3 support code for guests in v5.16-rc3, > if KVM allows userspace to configure KVM_ARM_VCPU_PMU_V3 even with > ID_AA64DFR0_PMUVER_IMP_DEF on the host (, which I don't think it does), > I think we should fix that to not allow that. I recently started looking into that too. If there's only one PMU, then the guest won't see the value IMP DEF for PMUVer (userspace cannot set the PMU feature because !kvm_arm_support_pmu_v3()). On heterogeneous systems with multiple PMUs, it gets complicated. I don't have any such hardware, but what I think will happen is that KVM will enable the static branch if there is at least one PMU with PMUVer != IMP_DEF, even if there are other PMUs with PMUVer = IMP_DEF. But read_sanitised_ftr_reg() will always return 0 for the PMUVer field because the field is defined as FTR_EXACT with a safe value of 0 in cpufeature.c. So the guest ends up seeing PMUVer = 0. I'm not sure if this is the case because I'm not familiar with the cpu features code, but I planning to investigate further. Thanks, Alex > (I'm not sure how KVM's PMUV3 support code is implemented in the > code that you are looking at though) > > Thanks, > Reiji
Hi On 12/6/21 10:52 AM, Alexandru Elisei wrote: > Hi, > > On Sat, Dec 04, 2021 at 09:39:59AM -0800, Reiji Watanabe wrote: >> Hi Eric, >> >> On Sat, Dec 4, 2021 at 6:14 AM Eric Auger <eauger@redhat.com> wrote: >>> >>> Hi Reiji, >>> >>> On 12/4/21 2:04 AM, Reiji Watanabe wrote: >>>> Hi Eric, >>>> >>>> On Thu, Dec 2, 2021 at 2:57 AM Eric Auger <eauger@redhat.com> wrote: >>>>> >>>>> Hi Reiji, >>>>> >>>>> On 11/30/21 6:32 AM, Reiji Watanabe wrote: >>>>>> Hi Eric, >>>>>> >>>>>> On Thu, Nov 25, 2021 at 12:30 PM Eric Auger <eauger@redhat.com> wrote: >>>>>>> >>>>>>> Hi Reiji, >>>>>>> >>>>>>> On 11/17/21 7:43 AM, Reiji Watanabe wrote: >>>>>>>> When ID_AA64DFR0_EL1.PMUVER or ID_DFR0_EL1.PERFMON is 0xf, which >>>>>>>> means IMPLEMENTATION DEFINED PMU supported, KVM unconditionally >>>>>>>> expose the value for the guest as it is. Since KVM doesn't support >>>>>>>> IMPLEMENTATION DEFINED PMU for the guest, in that case KVM should >>>>>>>> exopse 0x0 (PMU is not implemented) instead. >>>>>>> s/exopse/expose >>>>>>>> >>>>>>>> Change cpuid_feature_cap_perfmon_field() to update the field value >>>>>>>> to 0x0 when it is 0xf. >>>>>>> is it wrong to expose the guest with a Perfmon value of 0xF? Then the >>>>>>> guest should not use it as a PMUv3? >>>>>> >>>>>>> is it wrong to expose the guest with a Perfmon value of 0xF? Then the >>>>>>> guest should not use it as a PMUv3? >>>>>> >>>>>> For the value 0xf in ID_AA64DFR0_EL1.PMUVER and ID_DFR0_EL1.PERFMON, >>>>>> Arm ARM says: >>>>>> "IMPLEMENTATION DEFINED form of performance monitors supported, >>>>>> PMUv3 not supported." >>>>>> >>>>>> Since the PMU that KVM supports for guests is PMUv3, 0xf shouldn't >>>>>> be exposed to guests (And this patch series doesn't allow userspace >>>>>> to set the fields to 0xf for guests). >>>>> What I don't get is why this isn't detected before (in kvm_reset_vcpu). >>>>> if the VCPU was initialized with KVM_ARM_VCPU_PMU_V3 can we honor this >>>>> init request if the host pmu is implementation defined? >>>> >>>> KVM_ARM_VCPU_INIT with KVM_ARM_VCPU_PMU_V3 will fail in >>>> kvm_reset_vcpu() if the host PMU is implementation defined. >>> >>> OK. This was not obvsious to me. >>> >>> if (kvm_vcpu_has_pmu(vcpu) && !kvm_arm_support_pmu_v3()) { >>> ret = -EINVAL; >>> goto out; >>> } >>> >>> kvm_perf_init >>> + if (perf_num_counters() > 0) >>> + static_branch_enable(&kvm_arm_pmu_available); >>> >>> But I believe you ;-), sorry for the noise >> >> Thank you for the review ! >> >> I didn't find the code above in v5.16-rc3, which is the base code of >> this series. So, I'm not sure where the code came from (any kvmarm >> repository branch ??). >> >> What I see in v5.16-rc3 is: >> ---- >> int kvm_perf_init(void) >> { >> return perf_register_guest_info_callbacks(&kvm_guest_cbs); >> } >> >> void kvm_host_pmu_init(struct arm_pmu *pmu) >> { >> if (pmu->pmuver != 0 && pmu->pmuver != ID_AA64DFR0_PMUVER_IMP_DEF && >> !kvm_arm_support_pmu_v3() && !is_protected_kvm_enabled()) >> static_branch_enable(&kvm_arm_pmu_available); >> } >> ---- >> >> And I don't find any other code that enables kvm_arm_pmu_available. > > The code was recently changed (in v5.15 I think), I think Eric is looking > at an older version. Yes I was "googling" kvm_arm_pmu_available enablement and I missed the kvm_pmu_probe_pmuver() != ID_AA64DFR0_PMUVER_IMP_DEF check addition. So except the heterogenous case reported by Alexandru below, we should be fine. Sorry for the noise. Thanks Eric > >> >> Looking at the KVM's PMUV3 support code for guests in v5.16-rc3, >> if KVM allows userspace to configure KVM_ARM_VCPU_PMU_V3 even with >> ID_AA64DFR0_PMUVER_IMP_DEF on the host (, which I don't think it does), >> I think we should fix that to not allow that. > > I recently started looking into that too. If there's only one PMU, then the > guest won't see the value IMP DEF for PMUVer (userspace cannot set the PMU > feature because !kvm_arm_support_pmu_v3()). > > On heterogeneous systems with multiple PMUs, it gets complicated. I don't > have any such hardware, but what I think will happen is that KVM will > enable the static branch if there is at least one PMU with > PMUVer != IMP_DEF, even if there are other PMUs with PMUVer = IMP_DEF. But > read_sanitised_ftr_reg() will always return 0 for the > PMUVer field because the field is defined as FTR_EXACT with a safe value of > 0 in cpufeature.c. So the guest ends up seeing PMUVer = 0. > > I'm not sure if this is the case because I'm not familiar with the cpu > features code, but I planning to investigate further. > > Thanks, > Alex > >> (I'm not sure how KVM's PMUV3 support code is implemented in the >> code that you are looking at though) >> >> Thanks, >> Reiji >
Hi Eric, On Mon, Dec 6, 2021 at 2:25 AM Eric Auger <eauger@redhat.com> wrote: > > Hi > > On 12/6/21 10:52 AM, Alexandru Elisei wrote: > > Hi, > > > > On Sat, Dec 04, 2021 at 09:39:59AM -0800, Reiji Watanabe wrote: > >> Hi Eric, > >> > >> On Sat, Dec 4, 2021 at 6:14 AM Eric Auger <eauger@redhat.com> wrote: > >>> > >>> Hi Reiji, > >>> > >>> On 12/4/21 2:04 AM, Reiji Watanabe wrote: > >>>> Hi Eric, > >>>> > >>>> On Thu, Dec 2, 2021 at 2:57 AM Eric Auger <eauger@redhat.com> wrote: > >>>>> > >>>>> Hi Reiji, > >>>>> > >>>>> On 11/30/21 6:32 AM, Reiji Watanabe wrote: > >>>>>> Hi Eric, > >>>>>> > >>>>>> On Thu, Nov 25, 2021 at 12:30 PM Eric Auger <eauger@redhat.com> wrote: > >>>>>>> > >>>>>>> Hi Reiji, > >>>>>>> > >>>>>>> On 11/17/21 7:43 AM, Reiji Watanabe wrote: > >>>>>>>> When ID_AA64DFR0_EL1.PMUVER or ID_DFR0_EL1.PERFMON is 0xf, which > >>>>>>>> means IMPLEMENTATION DEFINED PMU supported, KVM unconditionally > >>>>>>>> expose the value for the guest as it is. Since KVM doesn't support > >>>>>>>> IMPLEMENTATION DEFINED PMU for the guest, in that case KVM should > >>>>>>>> exopse 0x0 (PMU is not implemented) instead. > >>>>>>> s/exopse/expose > >>>>>>>> > >>>>>>>> Change cpuid_feature_cap_perfmon_field() to update the field value > >>>>>>>> to 0x0 when it is 0xf. > >>>>>>> is it wrong to expose the guest with a Perfmon value of 0xF? Then the > >>>>>>> guest should not use it as a PMUv3? > >>>>>> > >>>>>>> is it wrong to expose the guest with a Perfmon value of 0xF? Then the > >>>>>>> guest should not use it as a PMUv3? > >>>>>> > >>>>>> For the value 0xf in ID_AA64DFR0_EL1.PMUVER and ID_DFR0_EL1.PERFMON, > >>>>>> Arm ARM says: > >>>>>> "IMPLEMENTATION DEFINED form of performance monitors supported, > >>>>>> PMUv3 not supported." > >>>>>> > >>>>>> Since the PMU that KVM supports for guests is PMUv3, 0xf shouldn't > >>>>>> be exposed to guests (And this patch series doesn't allow userspace > >>>>>> to set the fields to 0xf for guests). > >>>>> What I don't get is why this isn't detected before (in kvm_reset_vcpu). > >>>>> if the VCPU was initialized with KVM_ARM_VCPU_PMU_V3 can we honor this > >>>>> init request if the host pmu is implementation defined? > >>>> > >>>> KVM_ARM_VCPU_INIT with KVM_ARM_VCPU_PMU_V3 will fail in > >>>> kvm_reset_vcpu() if the host PMU is implementation defined. > >>> > >>> OK. This was not obvsious to me. > >>> > >>> if (kvm_vcpu_has_pmu(vcpu) && !kvm_arm_support_pmu_v3()) { > >>> ret = -EINVAL; > >>> goto out; > >>> } > >>> > >>> kvm_perf_init > >>> + if (perf_num_counters() > 0) > >>> + static_branch_enable(&kvm_arm_pmu_available); > >>> > >>> But I believe you ;-), sorry for the noise > >> > >> Thank you for the review ! > >> > >> I didn't find the code above in v5.16-rc3, which is the base code of > >> this series. So, I'm not sure where the code came from (any kvmarm > >> repository branch ??). > >> > >> What I see in v5.16-rc3 is: > >> ---- > >> int kvm_perf_init(void) > >> { > >> return perf_register_guest_info_callbacks(&kvm_guest_cbs); > >> } > >> > >> void kvm_host_pmu_init(struct arm_pmu *pmu) > >> { > >> if (pmu->pmuver != 0 && pmu->pmuver != ID_AA64DFR0_PMUVER_IMP_DEF && > >> !kvm_arm_support_pmu_v3() && !is_protected_kvm_enabled()) > >> static_branch_enable(&kvm_arm_pmu_available); > >> } > >> ---- > >> > >> And I don't find any other code that enables kvm_arm_pmu_available. > > > > The code was recently changed (in v5.15 I think), I think Eric is looking > > at an older version. > > Yes I was "googling" kvm_arm_pmu_available enablement and I missed the > kvm_pmu_probe_pmuver() != ID_AA64DFR0_PMUVER_IMP_DEF check addition. So > except the heterogenous case reported by Alexandru below, we should be > fine. Sorry for the noise. Understood. Thank you for the confirmation. Regards, Reiji > >> > >> Looking at the KVM's PMUV3 support code for guests in v5.16-rc3, > >> if KVM allows userspace to configure KVM_ARM_VCPU_PMU_V3 even with > >> ID_AA64DFR0_PMUVER_IMP_DEF on the host (, which I don't think it does), > >> I think we should fix that to not allow that. > > > > I recently started looking into that too. If there's only one PMU, then the > > guest won't see the value IMP DEF for PMUVer (userspace cannot set the PMU > > feature because !kvm_arm_support_pmu_v3()). > > > > On heterogeneous systems with multiple PMUs, it gets complicated. I don't > > have any such hardware, but what I think will happen is that KVM will > > enable the static branch if there is at least one PMU with > > PMUVer != IMP_DEF, even if there are other PMUs with PMUVer = IMP_DEF. But > > read_sanitised_ftr_reg() will always return 0 for the > > PMUVer field because the field is defined as FTR_EXACT with a safe value of > > 0 in cpufeature.c. So the guest ends up seeing PMUVer = 0. > > > > I'm not sure if this is the case because I'm not familiar with the cpu > > features code, but I planning to investigate further. > > > > Thanks, > > Alex > > > >> (I'm not sure how KVM's PMUV3 support code is implemented in the > >> code that you are looking at though) > >> > >> Thanks, > >> Reiji > > >
Hi Alex, On Mon, Dec 6, 2021 at 1:52 AM Alexandru Elisei <alexandru.elisei@arm.com> wrote: > > Hi, > > On Sat, Dec 04, 2021 at 09:39:59AM -0800, Reiji Watanabe wrote: > > Hi Eric, > > > > On Sat, Dec 4, 2021 at 6:14 AM Eric Auger <eauger@redhat.com> wrote: > > > > > > Hi Reiji, > > > > > > On 12/4/21 2:04 AM, Reiji Watanabe wrote: > > > > Hi Eric, > > > > > > > > On Thu, Dec 2, 2021 at 2:57 AM Eric Auger <eauger@redhat.com> wrote: > > > >> > > > >> Hi Reiji, > > > >> > > > >> On 11/30/21 6:32 AM, Reiji Watanabe wrote: > > > >>> Hi Eric, > > > >>> > > > >>> On Thu, Nov 25, 2021 at 12:30 PM Eric Auger <eauger@redhat.com> wrote: > > > >>>> > > > >>>> Hi Reiji, > > > >>>> > > > >>>> On 11/17/21 7:43 AM, Reiji Watanabe wrote: > > > >>>>> When ID_AA64DFR0_EL1.PMUVER or ID_DFR0_EL1.PERFMON is 0xf, which > > > >>>>> means IMPLEMENTATION DEFINED PMU supported, KVM unconditionally > > > >>>>> expose the value for the guest as it is. Since KVM doesn't support > > > >>>>> IMPLEMENTATION DEFINED PMU for the guest, in that case KVM should > > > >>>>> exopse 0x0 (PMU is not implemented) instead. > > > >>>> s/exopse/expose > > > >>>>> > > > >>>>> Change cpuid_feature_cap_perfmon_field() to update the field value > > > >>>>> to 0x0 when it is 0xf. > > > >>>> is it wrong to expose the guest with a Perfmon value of 0xF? Then the > > > >>>> guest should not use it as a PMUv3? > > > >>> > > > >>>> is it wrong to expose the guest with a Perfmon value of 0xF? Then the > > > >>>> guest should not use it as a PMUv3? > > > >>> > > > >>> For the value 0xf in ID_AA64DFR0_EL1.PMUVER and ID_DFR0_EL1.PERFMON, > > > >>> Arm ARM says: > > > >>> "IMPLEMENTATION DEFINED form of performance monitors supported, > > > >>> PMUv3 not supported." > > > >>> > > > >>> Since the PMU that KVM supports for guests is PMUv3, 0xf shouldn't > > > >>> be exposed to guests (And this patch series doesn't allow userspace > > > >>> to set the fields to 0xf for guests). > > > >> What I don't get is why this isn't detected before (in kvm_reset_vcpu). > > > >> if the VCPU was initialized with KVM_ARM_VCPU_PMU_V3 can we honor this > > > >> init request if the host pmu is implementation defined? > > > > > > > > KVM_ARM_VCPU_INIT with KVM_ARM_VCPU_PMU_V3 will fail in > > > > kvm_reset_vcpu() if the host PMU is implementation defined. > > > > > > OK. This was not obvsious to me. > > > > > > if (kvm_vcpu_has_pmu(vcpu) && !kvm_arm_support_pmu_v3()) { > > > ret = -EINVAL; > > > goto out; > > > } > > > > > > kvm_perf_init > > > + if (perf_num_counters() > 0) > > > + static_branch_enable(&kvm_arm_pmu_available); > > > > > > But I believe you ;-), sorry for the noise > > > > Thank you for the review ! > > > > I didn't find the code above in v5.16-rc3, which is the base code of > > this series. So, I'm not sure where the code came from (any kvmarm > > repository branch ??). > > > > What I see in v5.16-rc3 is: > > ---- > > int kvm_perf_init(void) > > { > > return perf_register_guest_info_callbacks(&kvm_guest_cbs); > > } > > > > void kvm_host_pmu_init(struct arm_pmu *pmu) > > { > > if (pmu->pmuver != 0 && pmu->pmuver != ID_AA64DFR0_PMUVER_IMP_DEF && > > !kvm_arm_support_pmu_v3() && !is_protected_kvm_enabled()) > > static_branch_enable(&kvm_arm_pmu_available); > > } > > ---- > > > > And I don't find any other code that enables kvm_arm_pmu_available. > > The code was recently changed (in v5.15 I think), I think Eric is looking > at an older version. > > > > > Looking at the KVM's PMUV3 support code for guests in v5.16-rc3, > > if KVM allows userspace to configure KVM_ARM_VCPU_PMU_V3 even with > > ID_AA64DFR0_PMUVER_IMP_DEF on the host (, which I don't think it does), > > I think we should fix that to not allow that. > > I recently started looking into that too. If there's only one PMU, then the > guest won't see the value IMP DEF for PMUVer (userspace cannot set the PMU > feature because !kvm_arm_support_pmu_v3()). > > On heterogeneous systems with multiple PMUs, it gets complicated. I don't > have any such hardware, but what I think will happen is that KVM will > enable the static branch if there is at least one PMU with > PMUVer != IMP_DEF, even if there are other PMUs with PMUVer = IMP_DEF. But > read_sanitised_ftr_reg() will always return 0 for the > PMUVer field because the field is defined as FTR_EXACT with a safe value of > 0 in cpufeature.c. So the guest ends up seeing PMUVer = 0. > > I'm not sure if this is the case because I'm not familiar with the cpu > features code, but I planning to investigate further. Thank you for the comment ! Yes, it looks like that KVM will enable the static branch if there is at least one PMU with PMUVer != 0 && PMUVer != IMP_DEF. (then, yes, AA64DFR0.PMUVER will be 0 even for a vCPU that KVM_ARM_VCPU_PMU_V3 is successfully configured for in the case) I will look into it some more. Thanks, Reiji
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h index ef6be92b1921..fd7ad8193827 100644 --- a/arch/arm64/include/asm/cpufeature.h +++ b/arch/arm64/include/asm/cpufeature.h @@ -553,7 +553,7 @@ cpuid_feature_cap_perfmon_field(u64 features, int field, u64 cap) /* Treat IMPLEMENTATION DEFINED functionality as unimplemented */ if (val == ID_AA64DFR0_PMUVER_IMP_DEF) - val = 0; + return (features & ~mask); if (val > cap) { features &= ~mask;
When ID_AA64DFR0_EL1.PMUVER or ID_DFR0_EL1.PERFMON is 0xf, which means IMPLEMENTATION DEFINED PMU supported, KVM unconditionally expose the value for the guest as it is. Since KVM doesn't support IMPLEMENTATION DEFINED PMU for the guest, in that case KVM should exopse 0x0 (PMU is not implemented) instead. Change cpuid_feature_cap_perfmon_field() to update the field value to 0x0 when it is 0xf. Fixes: 8e35aa642ee4 ("arm64: cpufeature: Extract capped perfmon fields") Signed-off-by: Reiji Watanabe <reijiw@google.com> --- arch/arm64/include/asm/cpufeature.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)