On Wed, 2023-05-17 at 15:55 -0700, Jing Zhang wrote:
> CAUTION: This email originated from outside of the organization. Do
> not click links or open attachments unless you can confirm the sender
> and know the content is safe.
>
>
>
> Hi Suraj,
>
> On Wed, May 17, 2023 at 3:00 PM Jitindar Singh, Suraj
> <surajjs@xxxxxxxxxx> wrote:
> >
> > 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.
> The limit and the check here might be different if like
> arm64_get_meltdown_state() is not SPECTRE_UNAFFECTED.
> i.e. if we remove the check here, theoretically, the csv3 can be set
> a
> value greater 1 if arm64_get_meltdown_state() is not
> SPECTRE_UNAFFECTED.
Reading init_cpu_ftr_reg() is hurting my head :p but don't we have
basically 2 cases here?
1. We are unaffected by spectre|meltdown i.e.
arm64_get_[spectre|meltdown]_v2_state() will return SPECTRE_UNAFFECTED
and we will set the limit to 1 for the csv[1|2] bit fields in
read_sanitised_id_aa64pfr0_el1()
or
2. We ARE affected by spectre|meltdown in which case
arm64_get_[spectre|meltdown]_v2_state() will be
SPECTRE_VULNERABLE|SPECTRE_MITIGATED in which case
read_sanitised_ftr_reg() will return a value with csv[1|2] set to 0
essentially setting the limit for either of these bit fields to 0 in
read_sanitised_id_aa64pfr0_el1().
Are we trying to catch the case where csv[1|2] is 0 on the host but we
are unaffected as detected by e.g. cpuid and that cpu happens to
incorrectly not set csv[1|2] in the ID register but we still want to
allow the guest to see those bits as set?
This isn't really related to the CR as I know this is existing code
which has just been moved around and sorry if I'm missing something
obvious.
Thanks,
Suraj
> >
> > >
> > > - 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
> Will fix.
> >
> > > 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
> Thanks, will fix it.
> >
> > > 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
>
> Thanks,
> Jing
