Re: [PATCH v8 6/6] KVM: arm64: Refactor writings for PMUVer/CSV2/CSV3

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 2023-05-03 at 17:16 +0000, Jing Zhang wrote:
> Refactor writings for ID_AA64PFR0_EL1.[CSV2|CSV3],
> ID_AA64DFR0_EL1.PMUVer and ID_DFR0_ELF.PerfMon based on utilities
> introduced by ID register descriptor array.
> 
> Signed-off-by: Jing Zhang <jingzhangos@xxxxxxxxxx>
> ---
>  arch/arm64/include/asm/cpufeature.h |   1 +
>  arch/arm64/kernel/cpufeature.c      |   2 +-
>  arch/arm64/kvm/id_regs.c            | 361 ++++++++++++++++++--------
> --
>  3 files changed, 242 insertions(+), 122 deletions(-)
> 
> 

[ SNIP ]

>  
> +static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
> +                                         const struct sys_reg_desc
> *rd)
> +{
> +       u64 val;
> +       u32 id = reg_to_encoding(rd);
> +
> +       val = read_sanitised_ftr_reg(id);
> +       /*
> +        * The default is to expose CSV2 == 1 if the HW isn't
> affected.
> +        * Although this is a per-CPU feature, we make it global
> because
> +        * asymmetric systems are just a nuisance.
> +        *
> +        * Userspace can override this as long as it doesn't promise
> +        * the impossible.
> +        */
> +       if (arm64_get_spectre_v2_state() == SPECTRE_UNAFFECTED) {
> +               val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2);
> +               val |=
> FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2), 1);
> +       }
> +       if (arm64_get_meltdown_state() == SPECTRE_UNAFFECTED) {
> +               val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3);
> +               val |=
> FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3), 1);
> +       }
> +
> +       val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_AMU);
> +
> +       return val;
> +}
> +
>  static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
>                                const struct sys_reg_desc *rd,
>                                u64 val)
>  {
> -       struct kvm_arch *arch = &vcpu->kvm->arch;
> -       u64 sval = val;
>         u8 csv2, csv3;
> -       int ret = 0;
>  
>         /*
>          * Allow AA64PFR0_EL1.CSV2 to be set from userspace as long
> as
> @@ -226,26 +338,30 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu
> *vcpu,
>         if (csv3 > 1 || (csv3 && arm64_get_meltdown_state() !=
> SPECTRE_UNAFFECTED))
>                 return -EINVAL;

Can't we remove the checking of csv[23] here as it will be checked by
arm64_check_features()?

i.e. in arm64_check_features() we will load the "limit" value from the
"reset" function (read_sanitised_id_aa64pfr0_el1()) which has csv[23]
set appropriately and limit it to a safe value basically performing the
same check as we are here.

>  
> -       mutex_lock(&arch->config_lock);
> -       /* We can only differ with CSV[23], and anything else is an
> error */
> -       val ^= read_id_reg(vcpu, rd);
> -       val &= ~(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2) |
> -                ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3));
> -       if (val) {
> -               ret = -EINVAL;
> -               goto out;
> -       }
> +       return set_id_reg(vcpu, rd, val);
> +}
>  
> -       /* Only allow userspace to change the idregs before VM
> running */
> -       if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &vcpu->kvm-
> >arch.flags)) {
> -               if (sval != read_id_reg(vcpu, rd))
> -                       ret = -EBUSY;
> -       } else {
> -               IDREG(vcpu->kvm, reg_to_encoding(rd)) = sval;
> -       }
> -out:
> -       mutex_unlock(&arch->config_lock);
> -       return ret;
> +static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
> +                                         const struct sys_reg_desc
> *rd)
> +{
> +       u64 val;
> +       u32 id = reg_to_encoding(rd);
> +
> +       val = read_sanitised_ftr_reg(id);
> +       /* Limit debug to ARMv8.0 */
> +       val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_DebugVer);
> +       val |=
> FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_DebugVer), 6);
> +       /*
> +        * Initialise the default PMUver before there is a chance to
> +        * create an actual PMU.
> +        */
> +       val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer);
> +       val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer),
> +                         kvm_arm_pmu_get_pmuver_limit());
> +       /* Hide SPE from guests */
> +       val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMSVer);
> +
> +       return val;
>  }
>  
>  static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
> @@ -255,7 +371,6 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu
> *vcpu,
>         struct kvm_arch *arch = &vcpu->kvm->arch;
>         u8 pmuver, host_pmuver;
>         bool valid_pmu;
> -       u64 sval = val;
>         int ret = 0;
>  
>         host_pmuver = kvm_arm_pmu_get_pmuver_limit();
> @@ -277,40 +392,61 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu
> *vcpu,
>                 return -EINVAL;
>  
>         mutex_lock(&arch->config_lock);
> -       /* We can only differ with PMUver, and anything else is an
> error */
> -       val ^= read_id_reg(vcpu, rd);
> -       val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer);
> -       if (val) {
> -               ret = -EINVAL;
> -               goto out;
> -       }
> -
>         /* Only allow userspace to change the idregs before VM
> running */
>         if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &vcpu->kvm-
> >arch.flags)) {
> -               if (sval != read_id_reg(vcpu, rd))
> +               if (val != read_id_reg(vcpu, rd))
>                         ret = -EBUSY;
> -       } else {
> -               if (valid_pmu) {
> -                       val = IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1);
> -                       val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
> -                       val |=
> FIELD_PREP(ID_AA64DFR0_EL1_PMUVer_MASK, pmuver);
> -                       IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) = val;
> -
> -                       val = IDREG(vcpu->kvm, SYS_ID_DFR0_EL1);
> -                       val &= ~ID_DFR0_EL1_PerfMon_MASK;
> -                       val |= FIELD_PREP(ID_DFR0_EL1_PerfMon_MASK,
> pmuver_to_perfmon(pmuver));
> -                       IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) = val;
> -               } else {
> -
>                        assign_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU,
> &vcpu->kvm->arch.flags,
> -                                  pmuver ==
> ID_AA64DFR0_EL1_PMUVer_IMP_DEF);
> -               }
> +               goto out;
> +       }
> +
> +       if (!valid_pmu) {
> +               /*
> +                * Ignore the PMUVer filed in @val. The PMUVer would

Nit s/filed/field

> be determined
> +                * by arch flags bit
> KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU,
> +                */
> +               pmuver = FIELD_GET(ID_AA64DFR0_EL1_PMUVer_MASK,
> +                                  IDREG(vcpu->kvm,
> SYS_ID_AA64DFR0_EL1));
> +               val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
> +               val |= FIELD_PREP(ID_AA64DFR0_EL1_PMUVer_MASK,
> pmuver);
>         }
>  
> +       ret = arm64_check_features(vcpu, rd, val);
> +       if (ret)
> +               goto out;
> +
> +       IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) = val;
> +
> +       val = IDREG(vcpu->kvm, SYS_ID_DFR0_EL1);
> +       val &= ~ID_DFR0_EL1_PerfMon_MASK;
> +       val |= FIELD_PREP(ID_DFR0_EL1_PerfMon_MASK,
> pmuver_to_perfmon(pmuver));
> +       IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) = val;
> +
> +       if (!valid_pmu)
> +               assign_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU, &vcpu-
> >kvm->arch.flags,
> +                          pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF);
> +
>  out:
>         mutex_unlock(&arch->config_lock);
>         return ret;
>  }
>  
> +static u64 read_sanitised_id_dfr0_el1(struct kvm_vcpu *vcpu,
> +                                     const struct sys_reg_desc *rd)
> +{
> +       u64 val;
> +       u32 id = reg_to_encoding(rd);
> +
> +       val = read_sanitised_ftr_reg(id);
> +       /*
> +        * Initialise the default PMUver before there is a chance to
> +        * create an actual PMU.
> +        */
> +       val &= ~ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon);
> +       val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon),
> kvm_arm_pmu_get_pmuver_limit());
> +
> +       return val;
> +}
> +
>  static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
>                            const struct sys_reg_desc *rd,
>                            u64 val)
> @@ -318,7 +454,6 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
>         struct kvm_arch *arch = &vcpu->kvm->arch;
>         u8 perfmon, host_perfmon;
>         bool valid_pmu;
> -       u64 sval = val;
>         int ret = 0;
>  
>         host_perfmon =
> pmuver_to_perfmon(kvm_arm_pmu_get_pmuver_limit());
> @@ -341,35 +476,39 @@ static int set_id_dfr0_el1(struct kvm_vcpu
> *vcpu,
>                 return -EINVAL;
>  
>         mutex_lock(&arch->config_lock);
> -       /* We can only differ with PerfMon, and anything else is an
> error */
> -       val ^= read_id_reg(vcpu, rd);
> -       val &= ~ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon);
> -       if (val) {
> -               ret = -EINVAL;
> -               goto out;
> -       }
> -
>         /* Only allow userspace to change the idregs before VM
> running */
>         if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &vcpu->kvm-
> >arch.flags)) {
> -               if (sval != read_id_reg(vcpu, rd))
> +               if (val != read_id_reg(vcpu, rd))
>                         ret = -EBUSY;
> -       } else {
> -               if (valid_pmu) {
> -                       val = IDREG(vcpu->kvm, SYS_ID_DFR0_EL1);
> -                       val &= ~ID_DFR0_EL1_PerfMon_MASK;
> -                       val |= FIELD_PREP(ID_DFR0_EL1_PerfMon_MASK,
> perfmon);
> -                       IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) = val;
> -
> -                       val = IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1);
> -                       val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
> -                       val |=
> FIELD_PREP(ID_AA64DFR0_EL1_PMUVer_MASK, perfmon_to_pmuver(perfmon));
> -                       IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) = val;
> -               } else {
> -
>                        assign_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU,
> &vcpu->kvm->arch.flags,
> -                                  perfmon ==
> ID_DFR0_EL1_PerfMon_IMPDEF);
> -               }
> +               goto out;
> +       }
> +
> +       if (!valid_pmu) {
> +               /*
> +                * Ignore the PerfMon filed in @val. The PerfMon

Nit s/filed/field

> would be determined
> +                * by arch flags bit
> KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU,
> +                */
> +               perfmon = FIELD_GET(ID_DFR0_EL1_PerfMon_MASK,
> +                                   IDREG(vcpu->kvm,
> SYS_ID_DFR0_EL1));
> +               val &= ~ID_DFR0_EL1_PerfMon_MASK;
> +               val |= FIELD_PREP(ID_DFR0_EL1_PerfMon_MASK, perfmon);
>         }
>  
> +       ret = arm64_check_features(vcpu, rd, val);
> +       if (ret)
> +               goto out;
> +
> +       IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) = val;
> +
> +       val = IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1);
> +       val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
> +       val |= FIELD_PREP(ID_AA64DFR0_EL1_PMUVer_MASK,
> perfmon_to_pmuver(perfmon));
> +       IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) = val;
> +
> +       if (!valid_pmu)
> +               assign_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU, &vcpu-
> >kvm->arch.flags,
> +                          perfmon == ID_DFR0_EL1_PerfMon_IMPDEF);
> +
>  out:
>         mutex_unlock(&arch->config_lock);
>         return ret;

Otherwise looks good!

Thanks,
Suraj




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux