On Fri, 2023-06-02 at 10:15 -0700, Jing Zhang wrote: > Hi Suraj, > > Let's continue the problem here you raised in the v9. > For the SVE example, the problem would be gone by enabling the > writable for SVE field? Since we are going to enable the writable for > all ID regs one by one later. > The double checking for CSV[2|3] is gone in this series. > > Thanks, > Jing Hi Jing, Correct. We need to in theory set the writable bit for all the fields we're manipulating in kvm_arm_read_id_reg(). I think I also ran into issues with the GIC bits. I guess for per vcpu values this raises the question of what it means to set them at a VM level (although I doubt anyone is setting these heterogeneously in practise). Thanks - Suraj > > On Thu, Jun 1, 2023 at 5:51 PM Jing Zhang <jingzhangos@xxxxxxxxxx> > wrote: > > > > Refactor writings for ID_AA64PFR0_EL1.[CSV2|CSV3], > > ID_AA64DFR0_EL1.PMUVer and ID_DFR0_ELF.PerfMon based on utilities > > specific to ID register. > > > > Signed-off-by: Jing Zhang <jingzhangos@xxxxxxxxxx> > > --- > > arch/arm64/include/asm/cpufeature.h | 1 + > > arch/arm64/kernel/cpufeature.c | 2 +- > > arch/arm64/kvm/sys_regs.c | 291 +++++++++++++++++++----- > > ---- > > 3 files changed, 203 insertions(+), 91 deletions(-) > > > > 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 7d7128c65161..3317a7b6deac 100644 > > --- a/arch/arm64/kernel/cpufeature.c > > +++ b/arch/arm64/kernel/cpufeature.c > > @@ -798,7 +798,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/sys_regs.c b/arch/arm64/kvm/sys_regs.c > > index 1a534e0fc4ca..50d4e25f42d3 100644 > > --- a/arch/arm64/kvm/sys_regs.c > > +++ b/arch/arm64/kvm/sys_regs.c > > @@ -41,6 +41,7 @@ > > * 64bit interface. > > */ > > > > +static int set_id_reg(struct kvm_vcpu *vcpu, const struct > > sys_reg_desc *rd, u64 val); > > static u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 > > encoding); > > static u64 sys_reg_to_index(const struct sys_reg_desc *reg); > > > > @@ -1194,6 +1195,86 @@ static bool access_arch_timer(struct > > kvm_vcpu *vcpu, > > return true; > > } > > > > +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)) > > @@ -1231,9 +1312,17 @@ static u8 pmuver_to_perfmon(u8 pmuver) > > } > > } > > > > -static void pmuver_update(struct kvm_vcpu *vcpu, u8 pmuver, bool > > valid_pmu) > > +static int pmuver_update(struct kvm_vcpu *vcpu, > > + const struct sys_reg_desc *rd, > > + u64 val, > > + u8 pmuver, > > + bool valid_pmu) > > { > > - u64 val; > > + int ret; > > + > > + ret = set_id_reg(vcpu, rd, val); > > + if (ret) > > + return ret; > > > > if (valid_pmu) { > > val = IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1); > > @@ -1249,6 +1338,8 @@ static void pmuver_update(struct kvm_vcpu > > *vcpu, u8 pmuver, bool valid_pmu) > > assign_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU, > > &vcpu->kvm->arch.flags, > > pmuver == > > ID_AA64DFR0_EL1_PMUVer_IMP_DEF); > > } > > + > > + return 0; > > } > > > > static u64 general_read_kvm_sanitised_reg(struct kvm_vcpu *vcpu, > > const struct sys_reg_desc *rd) > > @@ -1264,7 +1355,6 @@ static u64 kvm_arm_read_id_reg(const struct > > kvm_vcpu *vcpu, u32 encoding) > > 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); > > @@ -1291,15 +1381,10 @@ static u64 kvm_arm_read_id_reg(const struct > > kvm_vcpu *vcpu, u32 encoding) > > 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); > > @@ -1398,38 +1483,56 @@ static unsigned int sve_visibility(const > > struct kvm_vcpu *vcpu, > > return REG_HIDDEN; > > } > > > > -static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > > - const struct sys_reg_desc *rd, > > - u64 val) > > +static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > > + const struct sys_reg_desc > > *rd) > > { > > - u64 new_val = val; > > - u8 csv2, csv3; > > + u64 val; > > + u32 id = reg_to_encoding(rd); > > > > + val = read_sanitised_ftr_reg(id); > > /* > > - * 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). > > + * 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. > > */ > > - 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; > > + 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); > > + } > > > > - /* 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; > > + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_AMU); > > > > - /* 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) > > - return -EINVAL; > > + return val; > > +} > > > > - IDREG(vcpu->kvm, reg_to_encoding(rd)) = new_val; > > - return 0; > > +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, > > @@ -1457,14 +1560,35 @@ static int set_id_aa64dfr0_el1(struct > > kvm_vcpu *vcpu, > > if (kvm_vcpu_has_pmu(vcpu) != valid_pmu) > > return -EINVAL; > > > > - /* 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) > > - return -EINVAL; > > + if (!valid_pmu) { > > + /* > > + * Ignore the PMUVer field 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); > > + } > > > > - pmuver_update(vcpu, pmuver, valid_pmu); > > - return 0; > > + return pmuver_update(vcpu, rd, val, pmuver, valid_pmu); > > +} > > + > > +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, > > @@ -1493,14 +1617,18 @@ static int set_id_dfr0_el1(struct kvm_vcpu > > *vcpu, > > if (kvm_vcpu_has_pmu(vcpu) != valid_pmu) > > return -EINVAL; > > > > - /* 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) > > - return -EINVAL; > > + if (!valid_pmu) { > > + /* > > + * Ignore the PerfMon field 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); > > + } > > > > - pmuver_update(vcpu, perfmon_to_pmuver(perfmon), valid_pmu); > > - return 0; > > + return pmuver_update(vcpu, rd, val, > > perfmon_to_pmuver(perfmon), valid_pmu); > > } > > > > /* > > @@ -1520,11 +1648,14 @@ 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; > > + u32 id = reg_to_encoding(rd); > > + int ret = 0; > > > > - return 0; > > + ret = arm64_check_features(vcpu, rd, val); > > + if (!ret) > > + IDREG(vcpu->kvm, id) = val; > > + > > + return ret; > > } > > > > static int get_raz_reg(struct kvm_vcpu *vcpu, const struct > > sys_reg_desc *rd, > > @@ -1875,9 +2006,13 @@ static const struct sys_reg_desc > > sys_reg_descs[] = { > > /* 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), > > @@ -1906,8 +2041,12 @@ static const struct sys_reg_desc > > sys_reg_descs[] = { > > > > /* 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_reg, > > + .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), > > @@ -1917,8 +2056,12 @@ static const struct sys_reg_desc > > sys_reg_descs[] = { > > 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), > > @@ -3454,38 +3597,6 @@ void kvm_arm_init_id_regs(struct kvm *kvm) > > idreg++; > > id = reg_to_encoding(idreg); > > } > > - > > - /* > > - * 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; > > } > > > > int __init kvm_sys_reg_table_init(void) > > -- > > 2.41.0.rc0.172.g3f132b7071-goog > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel