| Message ID | 20230503171618.2020461-7-jingzhangos@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support writable CPU ID registers from userspace | expand |
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@google.com> > --- > 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
Hi Suraj, On Wed, May 17, 2023 at 3:00 PM Jitindar Singh, Suraj <surajjs@amazon.com> 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@google.com> > > --- > > 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. > > > > > - 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
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@amazon.com> 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@google.com> > > > --- > > > 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
On Thu, 18 May 2023 22:08:15 +0100, "Jitindar Singh, Suraj" <surajjs@amazon.com> wrote: > > 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(). What is "WE"? > > 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. This code is called from *userspace*, and tries to cope with a VM being restored. So we have 3 (not 2 cases): - both the source and the destination have the same level of CSVx support, and all is great in the world - the source has CSVx==0, while the destination has CSVx=1. All good, as we won't be lying to the guest, and the extra mitigation executed by the guest isn't a functional problem. The guest will still see CSVx=0 after migration. - the source has CSVx=1, while the destination has CSVx=0. This isn't an acceptable situation, and we return an error Is that clearer? M.
On Fri, 2023-05-19 at 10:16 +0100, Marc Zyngier 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. > > > > On Thu, 18 May 2023 22:08:15 +0100, > "Jitindar Singh, Suraj" <surajjs@amazon.com> wrote: > > > > 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(). > > What is "WE"? The CPU > > > > > 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. > > This code is called from *userspace*, and tries to cope with a VM > being restored. So we have 3 (not 2 cases): > > - both the source and the destination have the same level of CSVx > support, and all is great in the world > > - the source has CSVx==0, while the destination has CSVx=1. All good, > as we won't be lying to the guest, and the extra mitigation > executed by the guest isn't a functional problem. The guest will > still see CSVx=0 after migration. > > - the source has CSVx=1, while the destination has CSVx=0. This isn't > an acceptable situation, and we return an error > > Is that clearer? Yes thanks, that all makes sense. My initial question was why we needed to enforce the limit essentially twice in set_id_aa64pfr0_el1(). Once with: /* * Allow AA64PFR0_EL1.CSV2 to be set from userspace as long as * it doesn't promise more than what is actually provided (the * guest could otherwise be covered in ectoplasmic residue). */ csv2 = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_EL1_CSV2_SHIFT); if (csv2 > 1 || (csv2 && arm64_get_spectre_v2_state() != SPECTRE_UNAFFECTED)) return -EINVAL; /* Same thing for CSV3 */ csv3 = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_EL1_CSV3_SHIFT); if (csv3 > 1 || (csv3 && arm64_get_meltdown_state() != SPECTRE_UNAFFECTED)) return -EINVAL; And then again with the check in arm64_check_features(). Thanks, Suraj > > M. > > -- > Without deviation from the norm, progress is not possible.
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@google.com> Hi, > --- > 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 ] > diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c > index 64d40aa395be..6cd56c9e6428 100644 > --- a/arch/arm64/kvm/id_regs.c > +++ b/arch/arm64/kvm/id_regs.c > @@ -18,6 +18,86 @@ > > #include "sys_regs.h" > > [ 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); > + } > + I think the virtual GIC check also needs to be moved here from kvm_arm_read_id_reg() if (kvm_vgic_global_state.type == VGIC_V3) { val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_GIC); val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_GIC), 1); } Since the host supports GICv4.1 ID_AA64PFR0_EL1_GIC == 3 when qemu tries to read this register then overwrite it with the same value it read previously an error occurs. ID_AA64PFR0_EL1_GIC == 3 is the "limit" value however this field will read as ID_AA64PFR0_EL1_GIC == 1 when a virtual GICv3 is in use. Thus when qemu tries to set ID_AA64PFR0_EL1_GIC == 1, arm64_check_features() fails as those bits aren't set in id_reg.val meaning that modifications aren't allowed. Additionally this means it's possible to set ID_AA64PFR0_EL1_GIC == 3 from userspace however any reads will see this field as ID_AA64PFR0_EL1_GIC == 1. This all means the smp kvm_unit_tests failed. With the above change they pass. Thanks, Suraj > + 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) [ snip ]
On Sat, 20 May 2023 00:04:53 +0100, "Jitindar Singh, Suraj" <surajjs@amazon.com> wrote: > > On Fri, 2023-05-19 at 10:16 +0100, Marc Zyngier 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. > > > > > > > > On Thu, 18 May 2023 22:08:15 +0100, > > "Jitindar Singh, Suraj" <surajjs@amazon.com> wrote: > > > > > > 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(). > > > > What is "WE"? > > The CPU Hilarious. > > > > > > > > > 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. > > > > This code is called from *userspace*, and tries to cope with a VM > > being restored. So we have 3 (not 2 cases): > > > > - both the source and the destination have the same level of CSVx > > support, and all is great in the world > > > > - the source has CSVx==0, while the destination has CSVx=1. All good, > > as we won't be lying to the guest, and the extra mitigation > > executed by the guest isn't a functional problem. The guest will > > still see CSVx=0 after migration. > > > > - the source has CSVx=1, while the destination has CSVx=0. This isn't > > an acceptable situation, and we return an error > > > > Is that clearer? > > Yes thanks, that all makes sense. > > My initial question was why we needed to enforce the limit essentially > twice in set_id_aa64pfr0_el1(). > > Once with: > /* > * Allow AA64PFR0_EL1.CSV2 to be set from userspace as long as > * it doesn't promise more than what is actually provided (the > * guest could otherwise be covered in ectoplasmic residue). > */ > csv2 = cpuid_feature_extract_unsigned_field(val, > ID_AA64PFR0_EL1_CSV2_SHIFT); > if (csv2 > 1 || > (csv2 && arm64_get_spectre_v2_state() != > SPECTRE_UNAFFECTED)) > return -EINVAL; > > /* Same thing for CSV3 */ > csv3 = cpuid_feature_extract_unsigned_field(val, > ID_AA64PFR0_EL1_CSV3_SHIFT); > if (csv3 > 1 || > (csv3 && arm64_get_meltdown_state() != SPECTRE_UNAFFECTED)) > return -EINVAL; > > And then again with the check in arm64_check_features(). Ah, I see what you mean. Yes, this isn't right. I asked for the generic code to be used in the past, but the old stuff was left in. Which is obviously not great. M.
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h index 6bf013fb110d..dc769c2eb7a4 100644 --- a/arch/arm64/include/asm/cpufeature.h +++ b/arch/arm64/include/asm/cpufeature.h @@ -915,6 +915,7 @@ static inline unsigned int get_vmid_bits(u64 mmfr1) return 8; } +s64 arm64_ftr_safe_value(const struct arm64_ftr_bits *ftrp, s64 new, s64 cur); struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id); extern struct arm64_ftr_override id_aa64mmfr1_override; diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index 2e3e55139777..677ec4fe9f6b 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -791,7 +791,7 @@ static u64 arm64_ftr_set_value(const struct arm64_ftr_bits *ftrp, s64 reg, return reg; } -static s64 arm64_ftr_safe_value(const struct arm64_ftr_bits *ftrp, s64 new, +s64 arm64_ftr_safe_value(const struct arm64_ftr_bits *ftrp, s64 new, s64 cur) { s64 ret = 0; diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c index 64d40aa395be..6cd56c9e6428 100644 --- a/arch/arm64/kvm/id_regs.c +++ b/arch/arm64/kvm/id_regs.c @@ -18,6 +18,86 @@ #include "sys_regs.h" +static s64 kvm_arm64_ftr_safe_value(u32 id, const struct arm64_ftr_bits *ftrp, + s64 new, s64 cur) +{ + struct arm64_ftr_bits kvm_ftr = *ftrp; + + /* Some features have different safe value type in KVM than host features */ + switch (id) { + case SYS_ID_AA64DFR0_EL1: + if (kvm_ftr.shift == ID_AA64DFR0_EL1_PMUVer_SHIFT) + kvm_ftr.type = FTR_LOWER_SAFE; + break; + case SYS_ID_DFR0_EL1: + if (kvm_ftr.shift == ID_DFR0_EL1_PerfMon_SHIFT) + kvm_ftr.type = FTR_LOWER_SAFE; + break; + } + + return arm64_ftr_safe_value(&kvm_ftr, new, cur); +} + +/** + * arm64_check_features() - Check if a feature register value constitutes + * a subset of features indicated by the idreg's KVM sanitised limit. + * + * This function will check if each feature field of @val is the "safe" value + * against idreg's KVM sanitised limit return from reset() callback. + * If a field value in @val is the same as the one in limit, it is always + * considered the safe value regardless For register fields that are not in + * writable, only the value in limit is considered the safe value. + * + * Return: 0 if all the fields are safe. Otherwise, return negative errno. + */ +static int arm64_check_features(struct kvm_vcpu *vcpu, + const struct sys_reg_desc *rd, + u64 val) +{ + const struct arm64_ftr_reg *ftr_reg; + const struct arm64_ftr_bits *ftrp = NULL; + u32 id = reg_to_encoding(rd); + u64 writable_mask = rd->val; + u64 limit = 0; + u64 mask = 0; + + /* For hidden and unallocated idregs without reset, only val = 0 is allowed. */ + if (rd->reset) { + limit = rd->reset(vcpu, rd); + ftr_reg = get_arm64_ftr_reg(id); + if (!ftr_reg) + return -EINVAL; + ftrp = ftr_reg->ftr_bits; + } + + for (; ftrp && ftrp->width; ftrp++) { + s64 f_val, f_lim, safe_val; + u64 ftr_mask; + + ftr_mask = arm64_ftr_mask(ftrp); + if ((ftr_mask & writable_mask) != ftr_mask) + continue; + + f_val = arm64_ftr_value(ftrp, val); + f_lim = arm64_ftr_value(ftrp, limit); + mask |= ftr_mask; + + if (f_val == f_lim) + safe_val = f_val; + else + safe_val = kvm_arm64_ftr_safe_value(id, ftrp, f_val, f_lim); + + if (safe_val != f_val) + return -E2BIG; + } + + /* For fields that are not writable, values in limit are the safe values. */ + if ((val & ~mask) != (limit & ~mask)) + return -E2BIG; + + return 0; +} + static u8 vcpu_pmuver(const struct kvm_vcpu *vcpu) { if (kvm_vcpu_has_pmu(vcpu)) @@ -68,7 +148,6 @@ u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id) case SYS_ID_AA64PFR0_EL1: if (!vcpu_has_sve(vcpu)) val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_SVE); - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_AMU); if (kvm_vgic_global_state.type == VGIC_V3) { val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_GIC); val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_GIC), 1); @@ -95,15 +174,10 @@ u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id) val &= ~ARM64_FEATURE_MASK(ID_AA64ISAR2_EL1_WFxT); break; case SYS_ID_AA64DFR0_EL1: - /* Limit debug to ARMv8.0 */ - val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_DebugVer); - val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_DebugVer), 6); /* Set PMUver to the required version */ val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer); val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer), vcpu_pmuver(vcpu)); - /* Hide SPE from guests */ - val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMSVer); break; case SYS_ID_DFR0_EL1: val &= ~ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon); @@ -167,11 +241,23 @@ static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, u64 val) { - /* This is what we mean by invariant: you can't change it. */ - if (val != read_id_reg(vcpu, rd)) - return -EINVAL; + struct kvm_arch *arch = &vcpu->kvm->arch; + u32 id = reg_to_encoding(rd); + int ret = 0; - return 0; + mutex_lock(&arch->config_lock); + /* Only allow userspace to change the idregs before VM running */ + if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &vcpu->kvm->arch.flags)) { + if (val != read_id_reg(vcpu, rd)) + ret = -EBUSY; + } else { + ret = arm64_check_features(vcpu, rd, val); + if (!ret) + IDREG(vcpu->kvm, id) = val; + } + mutex_unlock(&arch->config_lock); + + return ret; } static unsigned int id_visibility(const struct kvm_vcpu *vcpu, @@ -203,14 +289,40 @@ static unsigned int aa32_id_visibility(const struct kvm_vcpu *vcpu, return id_visibility(vcpu, r); } +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; - 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 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 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; @@ -448,9 +587,13 @@ const struct sys_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = { /* CRm=1 */ AA32_ID_SANITISED(ID_PFR0_EL1), AA32_ID_SANITISED(ID_PFR1_EL1), - { SYS_DESC(SYS_ID_DFR0_EL1), .access = access_id_reg, - .get_user = get_id_reg, .set_user = set_id_dfr0_el1, - .visibility = aa32_id_visibility, }, + { SYS_DESC(SYS_ID_DFR0_EL1), + .access = access_id_reg, + .get_user = get_id_reg, + .set_user = set_id_dfr0_el1, + .visibility = aa32_id_visibility, + .reset = read_sanitised_id_dfr0_el1, + .val = ID_DFR0_EL1_PerfMon_MASK, }, ID_HIDDEN(ID_AFR0_EL1), AA32_ID_SANITISED(ID_MMFR0_EL1), AA32_ID_SANITISED(ID_MMFR1_EL1), @@ -479,8 +622,12 @@ const struct sys_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = { /* AArch64 ID registers */ /* CRm=4 */ - { SYS_DESC(SYS_ID_AA64PFR0_EL1), .access = access_id_reg, - .get_user = get_id_reg, .set_user = set_id_aa64pfr0_el1, }, + { SYS_DESC(SYS_ID_AA64PFR0_EL1), + .access = access_id_reg, + .get_user = get_id_reg, + .set_user = set_id_aa64pfr0_el1, + .reset = read_sanitised_id_aa64pfr0_el1, + .val = ID_AA64PFR0_EL1_CSV2_MASK | ID_AA64PFR0_EL1_CSV3_MASK, }, ID_SANITISED(ID_AA64PFR1_EL1), ID_UNALLOCATED(4, 2), ID_UNALLOCATED(4, 3), @@ -490,8 +637,12 @@ const struct sys_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = { ID_UNALLOCATED(4, 7), /* CRm=5 */ - { SYS_DESC(SYS_ID_AA64DFR0_EL1), .access = access_id_reg, - .get_user = get_id_reg, .set_user = set_id_aa64dfr0_el1, }, + { SYS_DESC(SYS_ID_AA64DFR0_EL1), + .access = access_id_reg, + .get_user = get_id_reg, + .set_user = set_id_aa64dfr0_el1, + .reset = read_sanitised_id_aa64dfr0_el1, + .val = ID_AA64DFR0_EL1_PMUVer_MASK, }, ID_SANITISED(ID_AA64DFR1_EL1), ID_UNALLOCATED(5, 2), ID_UNALLOCATED(5, 3), @@ -563,36 +714,4 @@ void kvm_arm_init_id_regs(struct kvm *kvm) IDREG(kvm, id) = val; } - - /* - * 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. - */ - val = IDREG(kvm, SYS_ID_AA64PFR0_EL1); - - 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); - } - - IDREG(kvm, SYS_ID_AA64PFR0_EL1) = val; - /* - * Initialise the default PMUver before there is a chance to - * create an actual PMU. - */ - val = IDREG(kvm, SYS_ID_AA64DFR0_EL1); - - val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer); - val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer), - kvm_arm_pmu_get_pmuver_limit()); - - IDREG(kvm, SYS_ID_AA64DFR0_EL1) = val; }
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@google.com> --- 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(-)