| Message ID | 20230402183735.3011540-7-jingzhangos@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support writable CPU ID registers from userspace | expand |
On Sun, 02 Apr 2023 19:37:35 +0100, Jing Zhang <jingzhangos@google.com> 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. > > No functional change intended. > > Co-developed-by: Reiji Watanabe <reijiw@google.com> > Signed-off-by: Reiji Watanabe <reijiw@google.com> > Signed-off-by: Jing Zhang <jingzhangos@google.com> > --- > arch/arm64/include/asm/cpufeature.h | 5 + > arch/arm64/kernel/cpufeature.c | 8 +- > arch/arm64/kvm/id_regs.c | 262 +++++++++++++++++++--------- > arch/arm64/kvm/sys_regs.c | 3 +- > arch/arm64/kvm/sys_regs.h | 2 +- > 5 files changed, 191 insertions(+), 89 deletions(-) > > diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h > index 6bf013fb110d..f17e74afe3e9 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; > @@ -925,6 +926,10 @@ extern struct arm64_ftr_override id_aa64smfr0_override; > extern struct arm64_ftr_override id_aa64isar1_override; > extern struct arm64_ftr_override id_aa64isar2_override; > > +extern const struct arm64_ftr_bits ftr_id_dfr0[]; > +extern const struct arm64_ftr_bits ftr_id_aa64pfr0[]; > +extern const struct arm64_ftr_bits ftr_id_aa64dfr0[]; > + > u32 get_kvm_ipa_limit(void); > void dump_cpu_features(void); > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index c331c49a7d19..5b0e3379e5f8 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -225,7 +225,7 @@ static const struct arm64_ftr_bits ftr_id_aa64isar2[] = { > ARM64_FTR_END, > }; > > -static const struct arm64_ftr_bits ftr_id_aa64pfr0[] = { > +const struct arm64_ftr_bits ftr_id_aa64pfr0[] = { > ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL1_CSV3_SHIFT, 4, 0), > ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL1_CSV2_SHIFT, 4, 0), > ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL1_DIT_SHIFT, 4, 0), > @@ -426,7 +426,7 @@ static const struct arm64_ftr_bits ftr_id_mmfr0[] = { > ARM64_FTR_END, > }; > > -static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = { > +const struct arm64_ftr_bits ftr_id_aa64dfr0[] = { > S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_DoubleLock_SHIFT, 4, 0), > ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_PMSVer_SHIFT, 4, 0), > ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_CTX_CMPs_SHIFT, 4, 0), > @@ -578,7 +578,7 @@ static const struct arm64_ftr_bits ftr_id_pfr2[] = { > ARM64_FTR_END, > }; > > -static const struct arm64_ftr_bits ftr_id_dfr0[] = { > +const struct arm64_ftr_bits ftr_id_dfr0[] = { > /* [31:28] TraceFilt */ > S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_EXACT, ID_DFR0_EL1_PerfMon_SHIFT, 4, 0), > ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_DFR0_EL1_MProfDbg_SHIFT, 4, 0), There really isn't a good reason for exposing any of this. You can readily get to the overarching arm64_ftr_reg. > @@ -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 af86001e2686..395eaf84a0ab 100644 > --- a/arch/arm64/kvm/id_regs.c > +++ b/arch/arm64/kvm/id_regs.c > @@ -39,6 +39,64 @@ struct id_reg_desc { > u64 (*read_kvm_sanitised_reg)(const struct id_reg_desc *idr); > }; > > +static struct id_reg_desc id_reg_descs[]; > + > +/** > + * arm64_check_features() - Check if a feature register value constitutes > + * a subset of features indicated by @limit. > + * > + * @ftrp: Pointer to an array of arm64_ftr_bits. It must be terminated by > + * an item whose width field is zero. > + * @writable_mask: Indicates writable feature bits. > + * @val: The feature register value to check > + * @limit: The limit value of the feature register > + * > + * This function will check if each feature field of @val is the "safe" value > + * against @limit based on @ftrp[], each of which specifies the target field > + * (shift, width), whether or not the field is for a signed value (sign), > + * how the field is determined to be "safe" (type), and the safe value > + * (safe_val) when type == FTR_EXACT (safe_val won't be used by this > + * function when type != FTR_EXACT). Any other fields in arm64_ftr_bits > + * won't be used by this function. If a field value in @val is the same > + * as the one in @limit, it is always considered the safe value regardless > + * of the type. 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(const struct arm64_ftr_bits *ftrp, > + u64 writable_mask, u64 val, u64 limit) > +{ > + u64 mask = 0; > + > + 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 = arm64_ftr_safe_value(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)) > @@ -84,7 +142,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); > @@ -111,15 +168,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); > @@ -178,9 +230,16 @@ 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; > + int ret; > + int id = reg_to_encoding(rd); > + const struct id_reg_desc *idr = &id_reg_descs[IDREG_IDX(id)]; > + > + ret = arm64_check_features(idr->ftr_bits, idr->writable_mask, val, > + idr->read_kvm_sanitised_reg ? idr->read_kvm_sanitised_reg(idr) : 0); > + if (ret) > + return ret; > + > + IDREG(vcpu->kvm, id) = val; > > return 0; > } > @@ -219,12 +278,39 @@ static u64 general_read_kvm_sanitised_reg(const struct id_reg_desc *idr) > return read_sanitised_ftr_reg(reg_to_encoding(&idr->reg_desc)); > } > > +static u64 read_sanitised_id_aa64pfr0_el1(const struct id_reg_desc *idr) > +{ > + u64 val; > + u32 id = reg_to_encoding(&idr->reg_desc); > + > + 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) > { > u8 csv2, csv3; > - u64 sval = val; > > /* > * Allow AA64PFR0_EL1.CSV2 to be set from userspace as long as > @@ -232,26 +318,37 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > * 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)) > + 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)) > + if (csv3 > 1 || (csv3 && arm64_get_meltdown_state() != SPECTRE_UNAFFECTED)) > return -EINVAL; > > - /* 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 set_id_reg(vcpu, rd, val); > +} > > - IDREG(vcpu->kvm, reg_to_encoding(rd)) = sval; > +static u64 read_sanitised_id_aa64dfr0_el1(const struct id_reg_desc *idr) > +{ > + u64 val; > + u32 id = reg_to_encoding(&idr->reg_desc); > > - return 0; > + 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, > @@ -260,6 +357,7 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu, > { > u8 pmuver, host_pmuver; > bool valid_pmu; > + int ret; > > host_pmuver = kvm_arm_pmu_get_pmuver_limit(); > > @@ -279,23 +377,25 @@ 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) { > mutex_lock(&vcpu->kvm->arch.config_lock); > - IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer); > - IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) |= > - FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer), pmuver); > + ret = set_id_reg(vcpu, rd, val); > + if (ret) { > + mutex_unlock(&vcpu->kvm->arch.config_lock); > + return ret; > + } > > IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) &= ~ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon); > IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) |= > FIELD_PREP(ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon), pmuver_to_perfmon(pmuver)); I repeatedly asked for assignments to be *on a single line*. > mutex_unlock(&vcpu->kvm->arch.config_lock); > } else { > + /* 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; > + I asked about this before. Why do we need this? Isn't the whole point of the exercise to have a *unified* way to check for the writable bits? If you still need to open-code that, the whole exercise is a bit pointless, isn't it? Frankly, the whole thing is going the wrong way, starting with the wrapping data structure. We already have most of what we need in sys_reg_desc, and it only takes a bit of imagination to get there. I've spent a couple of hours hacking away at the series, and got rid of it entirely, simply by repurposing exist fields (val for the write mask, reset for the limit value). All I can say is that it compiles. But at least it doesn't reinvent the wheel. M. diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c index 395eaf84a0ab..30f36e0dd12e 100644 --- a/arch/arm64/kvm/id_regs.c +++ b/arch/arm64/kvm/id_regs.c @@ -18,29 +18,6 @@ #include "sys_regs.h" -struct id_reg_desc { - const struct sys_reg_desc reg_desc; - /* - * ftr_bits points to the feature bits array defined in cpufeature.c for - * writable CPU ID feature register. - */ - const struct arm64_ftr_bits *ftr_bits; - /* - * Only bits with 1 are writable from userspace. - * This mask might not be necessary in the future whenever all ID - * registers are enabled as writable from userspace. - */ - const u64 writable_mask; - /* - * This function returns the KVM sanitised register value. - * The value would be the same as the host kernel sanitised value if - * there is no KVM sanitisation for this id register. - */ - u64 (*read_kvm_sanitised_reg)(const struct id_reg_desc *idr); -}; - -static struct id_reg_desc id_reg_descs[]; - /** * arm64_check_features() - Check if a feature register value constitutes * a subset of features indicated by @limit. @@ -64,11 +41,26 @@ static struct id_reg_desc id_reg_descs[]; * * Return: 0 if all the fields are safe. Otherwise, return negative errno. */ -static int arm64_check_features(const struct arm64_ftr_bits *ftrp, - u64 writable_mask, u64 val, u64 limit) +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; + u32 id = reg_to_encoding(rd); + u64 writable_mask = rd->val; + u64 limit = 0; u64 mask = 0; + 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; @@ -230,12 +222,10 @@ 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) { + u32 id = reg_to_encoding(rd); int ret; - int id = reg_to_encoding(rd); - const struct id_reg_desc *idr = &id_reg_descs[IDREG_IDX(id)]; - ret = arm64_check_features(idr->ftr_bits, idr->writable_mask, val, - idr->read_kvm_sanitised_reg ? idr->read_kvm_sanitised_reg(idr) : 0); + ret = arm64_check_features(vcpu, rd, val); if (ret) return ret; @@ -273,15 +263,17 @@ static unsigned int aa32_id_visibility(const struct kvm_vcpu *vcpu, return id_visibility(vcpu, r); } -static u64 general_read_kvm_sanitised_reg(const struct id_reg_desc *idr) +static u64 general_read_kvm_sanitised_reg(struct kvm_vcpu *vcpu, + const struct sys_reg_desc *r) { - return read_sanitised_ftr_reg(reg_to_encoding(&idr->reg_desc)); + return read_sanitised_ftr_reg(reg_to_encoding(r)); } -static u64 read_sanitised_id_aa64pfr0_el1(const struct id_reg_desc *idr) +static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, + const struct sys_reg_desc *r) { u64 val; - u32 id = reg_to_encoding(&idr->reg_desc); + u32 id = reg_to_encoding(r); val = read_sanitised_ftr_reg(id); /* @@ -329,10 +321,11 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, return set_id_reg(vcpu, rd, val); } -static u64 read_sanitised_id_aa64dfr0_el1(const struct id_reg_desc *idr) +static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu, + const struct sys_reg_desc *r) { u64 val; - u32 id = reg_to_encoding(&idr->reg_desc); + u32 id = reg_to_encoding(r); val = read_sanitised_ftr_reg(id); /* Limit debug to ARMv8.0 */ @@ -403,10 +396,11 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu, return 0; } -static u64 read_sanitised_id_dfr0_el1(const struct id_reg_desc *idr) +static u64 read_sanitised_id_dfr0_el1(struct kvm_vcpu *vcpu, + const struct sys_reg_desc *r) { u64 val; - u32 id = reg_to_encoding(&idr->reg_desc); + u32 id = reg_to_encoding(r); val = read_sanitised_ftr_reg(id); /* @@ -473,33 +467,23 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu, } /* sys_reg_desc initialiser for known cpufeature ID registers */ -#define SYS_DESC_SANITISED(name) { \ +#define ID_SANITISED(name) { \ SYS_DESC(SYS_##name), \ .access = access_id_reg, \ .get_user = get_id_reg, \ .set_user = set_id_reg, \ .visibility = id_visibility, \ -} - -#define ID_SANITISED(name) { \ - .reg_desc = SYS_DESC_SANITISED(name), \ - .ftr_bits = NULL, \ - .writable_mask = 0, \ - .read_kvm_sanitised_reg = general_read_kvm_sanitised_reg, \ + .reset = general_read_kvm_sanitised_reg, \ } /* sys_reg_desc initialiser for known cpufeature ID registers */ -#define AA32_ID_SANITISED(name) { \ - .reg_desc = { \ - SYS_DESC(SYS_##name), \ - .access = access_id_reg, \ - .get_user = get_id_reg, \ - .set_user = set_id_reg, \ - .visibility = aa32_id_visibility, \ - }, \ - .ftr_bits = NULL, \ - .writable_mask = 0, \ - .read_kvm_sanitised_reg = general_read_kvm_sanitised_reg, \ +#define AA32_ID_SANITISED(name) { \ + SYS_DESC(SYS_##name), \ + .access = access_id_reg, \ + .get_user = get_id_reg, \ + .set_user = set_id_reg, \ + .visibility = aa32_id_visibility, \ + .reset = general_read_kvm_sanitised_reg, \ } /* @@ -508,16 +492,11 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu, * (1 <= crm < 8, 0 <= Op2 < 8). */ #define ID_UNALLOCATED(crm, op2) { \ - .reg_desc = { \ Op0(3), Op1(0), CRn(0), CRm(crm), Op2(op2), \ .access = access_id_reg, \ .get_user = get_id_reg, \ .set_user = set_id_reg, \ .visibility = raz_visibility \ - }, \ - .ftr_bits = NULL, \ - .writable_mask = 0, \ - .read_kvm_sanitised_reg = NULL, \ } /* @@ -526,19 +505,14 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu, * RAZ for the guest. */ #define ID_HIDDEN(name) { \ - .reg_desc = { \ SYS_DESC(SYS_##name), \ .access = access_id_reg, \ .get_user = get_id_reg, \ .set_user = set_id_reg, \ .visibility = raz_visibility, \ - }, \ - .ftr_bits = NULL, \ - .writable_mask = 0, \ - .read_kvm_sanitised_reg = NULL, \ } -static struct id_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = { +static struct sys_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = { /* * ID regs: all ID_SANITISED() entries here must have corresponding * entries in arm64_ftr_regs[]. @@ -548,15 +522,14 @@ static struct id_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = { /* CRm=1 */ AA32_ID_SANITISED(ID_PFR0_EL1), AA32_ID_SANITISED(ID_PFR1_EL1), - { .reg_desc = { + { 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, }, - .ftr_bits = ftr_id_dfr0, - .writable_mask = ID_DFR0_EL1_PerfMon_MASK, - .read_kvm_sanitised_reg = read_sanitised_id_dfr0_el1, + .visibility = aa32_id_visibility, + .val = ID_DFR0_EL1_PerfMon_MASK, + .reset = read_sanitised_id_dfr0_el1, }, ID_HIDDEN(ID_AFR0_EL1), AA32_ID_SANITISED(ID_MMFR0_EL1), @@ -586,14 +559,13 @@ static struct id_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = { /* AArch64 ID registers */ /* CRm=4 */ - { .reg_desc = { + { SYS_DESC(SYS_ID_AA64PFR0_EL1), .access = access_id_reg, .get_user = get_id_reg, - .set_user = set_id_aa64pfr0_el1, }, - .ftr_bits = ftr_id_aa64pfr0, - .writable_mask = ID_AA64PFR0_EL1_CSV2_MASK | ID_AA64PFR0_EL1_CSV3_MASK, - .read_kvm_sanitised_reg = read_sanitised_id_aa64pfr0_el1, + .set_user = set_id_aa64pfr0_el1, + .val = ID_AA64PFR0_EL1_CSV2_MASK | ID_AA64PFR0_EL1_CSV3_MASK, + .reset = read_sanitised_id_aa64pfr0_el1, }, ID_SANITISED(ID_AA64PFR1_EL1), ID_UNALLOCATED(4, 2), @@ -604,14 +576,13 @@ static struct id_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = { ID_UNALLOCATED(4, 7), /* CRm=5 */ - { .reg_desc = { + { SYS_DESC(SYS_ID_AA64DFR0_EL1), .access = access_id_reg, .get_user = get_id_reg, - .set_user = set_id_aa64dfr0_el1, }, - .ftr_bits = ftr_id_aa64dfr0, - .writable_mask = ID_AA64DFR0_EL1_PMUVer_MASK, - .read_kvm_sanitised_reg = read_sanitised_id_aa64dfr0_el1, + .set_user = set_id_aa64dfr0_el1, + .val = ID_AA64DFR0_EL1_PMUVer_MASK, + .reset = read_sanitised_id_aa64dfr0_el1, }, ID_SANITISED(ID_AA64DFR1_EL1), ID_UNALLOCATED(5, 2), @@ -648,7 +619,7 @@ static const struct sys_reg_desc *id_params_to_desc(struct sys_reg_params *param id = reg_to_encoding(params); if (is_id_reg(id)) - return &id_reg_descs[IDREG_IDX(id)].reg_desc; + return &id_reg_descs[IDREG_IDX(id)]; return NULL; } @@ -742,11 +713,11 @@ bool kvm_arm_idreg_table_init(void) unsigned int i; for (i = 0; i < ARRAY_SIZE(id_reg_descs); i++) { - const struct sys_reg_desc *r = &id_reg_descs[i].reg_desc; + const struct sys_reg_desc *r = &id_reg_descs[i]; if (!is_id_reg(reg_to_encoding(r))) { kvm_err("id_reg table %pS entry %d not set correctly\n", - &id_reg_descs[i].reg_desc, i); + id_reg_descs, i); return false; } } @@ -756,7 +727,7 @@ bool kvm_arm_idreg_table_init(void) int kvm_arm_walk_id_regs(struct kvm_vcpu *vcpu, u64 __user *uind) { - const struct id_reg_desc *i2, *end2; + const struct sys_reg_desc *i2, *end2; unsigned int total = 0; int err; @@ -764,7 +735,7 @@ int kvm_arm_walk_id_regs(struct kvm_vcpu *vcpu, u64 __user *uind) end2 = id_reg_descs + ARRAY_SIZE(id_reg_descs); for (; i2 != end2; i2++) { - err = walk_one_sys_reg(vcpu, &(i2->reg_desc), &uind, &total); + err = walk_one_sys_reg(vcpu, i2, &uind, &total); if (err) return err; } @@ -779,11 +750,11 @@ void kvm_arm_init_id_regs(struct kvm *kvm) u64 val; for (i = 0; i < ARRAY_SIZE(id_reg_descs); i++) { - id = reg_to_encoding(&id_reg_descs[i].reg_desc); + id = reg_to_encoding(&id_reg_descs[i]); val = 0; - if (id_reg_descs[i].read_kvm_sanitised_reg) - val = id_reg_descs[i].read_kvm_sanitised_reg(&id_reg_descs[i]); + if (id_reg_descs[i].reset) + val = id_reg_descs[i].reset(NULL, &id_reg_descs[i]); IDREG(kvm, id) = val; } diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index b90d1d3ad081..c4f498e75315 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -540,10 +540,11 @@ static int get_bvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, return 0; } -static void reset_bvr(struct kvm_vcpu *vcpu, +static u64 reset_bvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd) { vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm] = rd->val; + return rd->val; } static bool trap_bcr(struct kvm_vcpu *vcpu, @@ -576,10 +577,11 @@ static int get_bcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, return 0; } -static void reset_bcr(struct kvm_vcpu *vcpu, +static u64 reset_bcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd) { vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm] = rd->val; + return rd->val; } static bool trap_wvr(struct kvm_vcpu *vcpu, @@ -613,10 +615,11 @@ static int get_wvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, return 0; } -static void reset_wvr(struct kvm_vcpu *vcpu, +static u64 reset_wvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd) { vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm] = rd->val; + return rd->val; } static bool trap_wcr(struct kvm_vcpu *vcpu, @@ -649,25 +652,28 @@ static int get_wcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, return 0; } -static void reset_wcr(struct kvm_vcpu *vcpu, +static u64 reset_wcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd) { vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm] = rd->val; + return rd->val; } -static void reset_amair_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) +static u64 reset_amair_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) { u64 amair = read_sysreg(amair_el1); vcpu_write_sys_reg(vcpu, amair, AMAIR_EL1); + return amair; } -static void reset_actlr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) +static u64 reset_actlr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) { u64 actlr = read_sysreg(actlr_el1); vcpu_write_sys_reg(vcpu, actlr, ACTLR_EL1); + return actlr; } -static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) +static u64 reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) { u64 mpidr; @@ -681,7 +687,9 @@ static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) mpidr = (vcpu->vcpu_id & 0x0f) << MPIDR_LEVEL_SHIFT(0); mpidr |= ((vcpu->vcpu_id >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1); mpidr |= ((vcpu->vcpu_id >> 12) & 0xff) << MPIDR_LEVEL_SHIFT(2); - vcpu_write_sys_reg(vcpu, (1ULL << 31) | mpidr, MPIDR_EL1); + mpidr |= (1ULL << 31); + vcpu_write_sys_reg(vcpu, mpidr, MPIDR_EL1); + return mpidr; } static unsigned int pmu_visibility(const struct kvm_vcpu *vcpu, @@ -693,13 +701,13 @@ static unsigned int pmu_visibility(const struct kvm_vcpu *vcpu, return REG_HIDDEN; } -static void reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) +static u64 reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) { u64 n, mask = BIT(ARMV8_PMU_CYCLE_IDX); /* No PMU available, any PMU reg may UNDEF... */ if (!kvm_arm_support_pmu_v3()) - return; + return 0; n = read_sysreg(pmcr_el0) >> ARMV8_PMU_PMCR_N_SHIFT; n &= ARMV8_PMU_PMCR_N_MASK; @@ -708,33 +716,37 @@ static void reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) reset_unknown(vcpu, r); __vcpu_sys_reg(vcpu, r->reg) &= mask; + return __vcpu_sys_reg(vcpu, r->reg); } -static void reset_pmevcntr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) +static u64 reset_pmevcntr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) { reset_unknown(vcpu, r); __vcpu_sys_reg(vcpu, r->reg) &= GENMASK(31, 0); + return __vcpu_sys_reg(vcpu, r->reg); } -static void reset_pmevtyper(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) +static u64 reset_pmevtyper(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) { reset_unknown(vcpu, r); __vcpu_sys_reg(vcpu, r->reg) &= ARMV8_PMU_EVTYPE_MASK; + return __vcpu_sys_reg(vcpu, r->reg); } -static void reset_pmselr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) +static u64 reset_pmselr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) { reset_unknown(vcpu, r); __vcpu_sys_reg(vcpu, r->reg) &= ARMV8_PMU_COUNTER_MASK; + return __vcpu_sys_reg(vcpu, r->reg); } -static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) +static u64 reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) { u64 pmcr; /* No PMU available, PMCR_EL0 may UNDEF... */ if (!kvm_arm_support_pmu_v3()) - return; + return 0; /* Only preserve PMCR_EL0.N, and reset the rest to 0 */ pmcr = read_sysreg(pmcr_el0) & (ARMV8_PMU_PMCR_N_MASK << ARMV8_PMU_PMCR_N_SHIFT); @@ -742,6 +754,7 @@ static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) pmcr |= ARMV8_PMU_PMCR_LC; __vcpu_sys_reg(vcpu, r->reg) = pmcr; + return __vcpu_sys_reg(vcpu, r->reg); } static bool check_pmu_access_disabled(struct kvm_vcpu *vcpu, u64 flags) @@ -1205,7 +1218,7 @@ static bool access_clidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, * Fabricate a CLIDR_EL1 value instead of using the real value, which can vary * by the physical CPU which the vcpu currently resides in. */ -static void reset_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) +static u64 reset_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) { u64 ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0); u64 clidr; @@ -1253,6 +1266,7 @@ static void reset_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) clidr |= 2 << CLIDR_TTYPE_SHIFT(loc); __vcpu_sys_reg(vcpu, r->reg) = clidr; + return __vcpu_sys_reg(vcpu, r->reg); } static int set_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, @@ -2603,19 +2617,21 @@ id_to_sys_reg_desc(struct kvm_vcpu *vcpu, u64 id, */ #define FUNCTION_INVARIANT(reg) \ - static void get_##reg(struct kvm_vcpu *v, \ + static u64 get_##reg(struct kvm_vcpu *v, \ const struct sys_reg_desc *r) \ { \ ((struct sys_reg_desc *)r)->val = read_sysreg(reg); \ + return ((struct sys_reg_desc *)r)->val; \ } FUNCTION_INVARIANT(midr_el1) FUNCTION_INVARIANT(revidr_el1) FUNCTION_INVARIANT(aidr_el1) -static void get_ctr_el0(struct kvm_vcpu *v, const struct sys_reg_desc *r) +static u64 get_ctr_el0(struct kvm_vcpu *v, const struct sys_reg_desc *r) { ((struct sys_reg_desc *)r)->val = read_sanitised_ftr_reg(SYS_CTR_EL0); + return ((struct sys_reg_desc *)r)->val; } /* ->val is filled in by kvm_sys_reg_table_init() */ diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h index df8d26df93ec..d14d5b41a222 100644 --- a/arch/arm64/kvm/sys_regs.h +++ b/arch/arm64/kvm/sys_regs.h @@ -65,12 +65,12 @@ struct sys_reg_desc { const struct sys_reg_desc *); /* Initialization for vcpu. */ - void (*reset)(struct kvm_vcpu *, const struct sys_reg_desc *); + u64 (*reset)(struct kvm_vcpu *, const struct sys_reg_desc *); /* Index into sys_reg[], or 0 if we don't need to save it. */ int reg; - /* Value (usually reset value) */ + /* Value (usually reset value), or write mask for idregs */ u64 val; /* Custom get/set_user functions, fallback to generic if NULL */ @@ -123,19 +123,21 @@ static inline bool read_zero(struct kvm_vcpu *vcpu, } /* Reset functions */ -static inline void reset_unknown(struct kvm_vcpu *vcpu, +static inline u64 reset_unknown(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) { BUG_ON(!r->reg); BUG_ON(r->reg >= NR_SYS_REGS); __vcpu_sys_reg(vcpu, r->reg) = 0x1de7ec7edbadc0deULL; + return __vcpu_sys_reg(vcpu, r->reg); } -static inline void reset_val(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) +static inline u64 reset_val(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) { BUG_ON(!r->reg); BUG_ON(r->reg >= NR_SYS_REGS); __vcpu_sys_reg(vcpu, r->reg) = r->val; + return __vcpu_sys_reg(vcpu, r->reg); } static inline unsigned int sysreg_visibility(const struct kvm_vcpu *vcpu,
Hi Marc, On Mon, Apr 3, 2023 at 7:55 AM Marc Zyngier <maz@kernel.org> wrote: > > On Sun, 02 Apr 2023 19:37:35 +0100, > Jing Zhang <jingzhangos@google.com> 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. > > > > No functional change intended. > > > > Co-developed-by: Reiji Watanabe <reijiw@google.com> > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > > Signed-off-by: Jing Zhang <jingzhangos@google.com> > > --- > > arch/arm64/include/asm/cpufeature.h | 5 + > > arch/arm64/kernel/cpufeature.c | 8 +- > > arch/arm64/kvm/id_regs.c | 262 +++++++++++++++++++--------- > > arch/arm64/kvm/sys_regs.c | 3 +- > > arch/arm64/kvm/sys_regs.h | 2 +- > > 5 files changed, 191 insertions(+), 89 deletions(-) > > > > diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h > > index 6bf013fb110d..f17e74afe3e9 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; > > @@ -925,6 +926,10 @@ extern struct arm64_ftr_override id_aa64smfr0_override; > > extern struct arm64_ftr_override id_aa64isar1_override; > > extern struct arm64_ftr_override id_aa64isar2_override; > > > > +extern const struct arm64_ftr_bits ftr_id_dfr0[]; > > +extern const struct arm64_ftr_bits ftr_id_aa64pfr0[]; > > +extern const struct arm64_ftr_bits ftr_id_aa64dfr0[]; > > + > > u32 get_kvm_ipa_limit(void); > > void dump_cpu_features(void); > > > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > > index c331c49a7d19..5b0e3379e5f8 100644 > > --- a/arch/arm64/kernel/cpufeature.c > > +++ b/arch/arm64/kernel/cpufeature.c > > @@ -225,7 +225,7 @@ static const struct arm64_ftr_bits ftr_id_aa64isar2[] = { > > ARM64_FTR_END, > > }; > > > > -static const struct arm64_ftr_bits ftr_id_aa64pfr0[] = { > > +const struct arm64_ftr_bits ftr_id_aa64pfr0[] = { > > ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL1_CSV3_SHIFT, 4, 0), > > ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL1_CSV2_SHIFT, 4, 0), > > ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL1_DIT_SHIFT, 4, 0), > > @@ -426,7 +426,7 @@ static const struct arm64_ftr_bits ftr_id_mmfr0[] = { > > ARM64_FTR_END, > > }; > > > > -static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = { > > +const struct arm64_ftr_bits ftr_id_aa64dfr0[] = { > > S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_DoubleLock_SHIFT, 4, 0), > > ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_PMSVer_SHIFT, 4, 0), > > ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_CTX_CMPs_SHIFT, 4, 0), > > @@ -578,7 +578,7 @@ static const struct arm64_ftr_bits ftr_id_pfr2[] = { > > ARM64_FTR_END, > > }; > > > > -static const struct arm64_ftr_bits ftr_id_dfr0[] = { > > +const struct arm64_ftr_bits ftr_id_dfr0[] = { > > /* [31:28] TraceFilt */ > > S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_EXACT, ID_DFR0_EL1_PerfMon_SHIFT, 4, 0), > > ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_DFR0_EL1_MProfDbg_SHIFT, 4, 0), > > There really isn't a good reason for exposing any of this. You can > readily get to the overarching arm64_ftr_reg. Yes, will do. > > > @@ -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 af86001e2686..395eaf84a0ab 100644 > > --- a/arch/arm64/kvm/id_regs.c > > +++ b/arch/arm64/kvm/id_regs.c > > @@ -39,6 +39,64 @@ struct id_reg_desc { > > u64 (*read_kvm_sanitised_reg)(const struct id_reg_desc *idr); > > }; > > > > +static struct id_reg_desc id_reg_descs[]; > > + > > +/** > > + * arm64_check_features() - Check if a feature register value constitutes > > + * a subset of features indicated by @limit. > > + * > > + * @ftrp: Pointer to an array of arm64_ftr_bits. It must be terminated by > > + * an item whose width field is zero. > > + * @writable_mask: Indicates writable feature bits. > > + * @val: The feature register value to check > > + * @limit: The limit value of the feature register > > + * > > + * This function will check if each feature field of @val is the "safe" value > > + * against @limit based on @ftrp[], each of which specifies the target field > > + * (shift, width), whether or not the field is for a signed value (sign), > > + * how the field is determined to be "safe" (type), and the safe value > > + * (safe_val) when type == FTR_EXACT (safe_val won't be used by this > > + * function when type != FTR_EXACT). Any other fields in arm64_ftr_bits > > + * won't be used by this function. If a field value in @val is the same > > + * as the one in @limit, it is always considered the safe value regardless > > + * of the type. 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(const struct arm64_ftr_bits *ftrp, > > + u64 writable_mask, u64 val, u64 limit) > > +{ > > + u64 mask = 0; > > + > > + 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 = arm64_ftr_safe_value(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)) > > @@ -84,7 +142,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); > > @@ -111,15 +168,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); > > @@ -178,9 +230,16 @@ 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; > > + int ret; > > + int id = reg_to_encoding(rd); > > + const struct id_reg_desc *idr = &id_reg_descs[IDREG_IDX(id)]; > > + > > + ret = arm64_check_features(idr->ftr_bits, idr->writable_mask, val, > > + idr->read_kvm_sanitised_reg ? idr->read_kvm_sanitised_reg(idr) : 0); > > + if (ret) > > + return ret; > > + > > + IDREG(vcpu->kvm, id) = val; > > > > return 0; > > } > > @@ -219,12 +278,39 @@ static u64 general_read_kvm_sanitised_reg(const struct id_reg_desc *idr) > > return read_sanitised_ftr_reg(reg_to_encoding(&idr->reg_desc)); > > } > > > > +static u64 read_sanitised_id_aa64pfr0_el1(const struct id_reg_desc *idr) > > +{ > > + u64 val; > > + u32 id = reg_to_encoding(&idr->reg_desc); > > + > > + 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) > > { > > u8 csv2, csv3; > > - u64 sval = val; > > > > /* > > * Allow AA64PFR0_EL1.CSV2 to be set from userspace as long as > > @@ -232,26 +318,37 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > > * 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)) > > + 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)) > > + if (csv3 > 1 || (csv3 && arm64_get_meltdown_state() != SPECTRE_UNAFFECTED)) > > return -EINVAL; > > > > - /* 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 set_id_reg(vcpu, rd, val); > > +} > > > > - IDREG(vcpu->kvm, reg_to_encoding(rd)) = sval; > > +static u64 read_sanitised_id_aa64dfr0_el1(const struct id_reg_desc *idr) > > +{ > > + u64 val; > > + u32 id = reg_to_encoding(&idr->reg_desc); > > > > - return 0; > > + 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, > > @@ -260,6 +357,7 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu, > > { > > u8 pmuver, host_pmuver; > > bool valid_pmu; > > + int ret; > > > > host_pmuver = kvm_arm_pmu_get_pmuver_limit(); > > > > @@ -279,23 +377,25 @@ 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) { > > mutex_lock(&vcpu->kvm->arch.config_lock); > > - IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer); > > - IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) |= > > - FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer), pmuver); > > + ret = set_id_reg(vcpu, rd, val); > > + if (ret) { > > + mutex_unlock(&vcpu->kvm->arch.config_lock); > > + return ret; > > + } > > > > IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) &= ~ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon); > > IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) |= > > FIELD_PREP(ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon), pmuver_to_perfmon(pmuver)); > > I repeatedly asked for assignments to be *on a single line*. Will fix it. > > > mutex_unlock(&vcpu->kvm->arch.config_lock); > > } else { > > + /* 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; > > + > > I asked about this before. Why do we need this? Isn't the whole point > of the exercise to have a *unified* way to check for the writable > bits? If you still need to open-code that, the whole exercise is a > bit pointless, isn't it? Right. Will find a better way. > > Frankly, the whole thing is going the wrong way, starting with the > wrapping data structure. We already have most of what we need in > sys_reg_desc, and it only takes a bit of imagination to get there. > > I've spent a couple of hours hacking away at the series, and got rid > of it entirely, simply by repurposing exist fields (val for the write > mask, reset for the limit value). All I can say is that it compiles. > > But at least it doesn't reinvent the wheel. > > M. > > diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c > index 395eaf84a0ab..30f36e0dd12e 100644 > --- a/arch/arm64/kvm/id_regs.c > +++ b/arch/arm64/kvm/id_regs.c > @@ -18,29 +18,6 @@ > > #include "sys_regs.h" > > -struct id_reg_desc { > - const struct sys_reg_desc reg_desc; > - /* > - * ftr_bits points to the feature bits array defined in cpufeature.c for > - * writable CPU ID feature register. > - */ > - const struct arm64_ftr_bits *ftr_bits; > - /* > - * Only bits with 1 are writable from userspace. > - * This mask might not be necessary in the future whenever all ID > - * registers are enabled as writable from userspace. > - */ > - const u64 writable_mask; > - /* > - * This function returns the KVM sanitised register value. > - * The value would be the same as the host kernel sanitised value if > - * there is no KVM sanitisation for this id register. > - */ > - u64 (*read_kvm_sanitised_reg)(const struct id_reg_desc *idr); > -}; > - > -static struct id_reg_desc id_reg_descs[]; > - > /** > * arm64_check_features() - Check if a feature register value constitutes > * a subset of features indicated by @limit. > @@ -64,11 +41,26 @@ static struct id_reg_desc id_reg_descs[]; > * > * Return: 0 if all the fields are safe. Otherwise, return negative errno. > */ > -static int arm64_check_features(const struct arm64_ftr_bits *ftrp, > - u64 writable_mask, u64 val, u64 limit) > +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; > + u32 id = reg_to_encoding(rd); > + u64 writable_mask = rd->val; > + u64 limit = 0; > u64 mask = 0; > > + 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; > @@ -230,12 +222,10 @@ 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) > { > + u32 id = reg_to_encoding(rd); > int ret; > - int id = reg_to_encoding(rd); > - const struct id_reg_desc *idr = &id_reg_descs[IDREG_IDX(id)]; > > - ret = arm64_check_features(idr->ftr_bits, idr->writable_mask, val, > - idr->read_kvm_sanitised_reg ? idr->read_kvm_sanitised_reg(idr) : 0); > + ret = arm64_check_features(vcpu, rd, val); > if (ret) > return ret; > > @@ -273,15 +263,17 @@ static unsigned int aa32_id_visibility(const struct kvm_vcpu *vcpu, > return id_visibility(vcpu, r); > } > > -static u64 general_read_kvm_sanitised_reg(const struct id_reg_desc *idr) > +static u64 general_read_kvm_sanitised_reg(struct kvm_vcpu *vcpu, > + const struct sys_reg_desc *r) > { > - return read_sanitised_ftr_reg(reg_to_encoding(&idr->reg_desc)); > + return read_sanitised_ftr_reg(reg_to_encoding(r)); > } > > -static u64 read_sanitised_id_aa64pfr0_el1(const struct id_reg_desc *idr) > +static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > + const struct sys_reg_desc *r) > { > u64 val; > - u32 id = reg_to_encoding(&idr->reg_desc); > + u32 id = reg_to_encoding(r); > > val = read_sanitised_ftr_reg(id); > /* > @@ -329,10 +321,11 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > return set_id_reg(vcpu, rd, val); > } > > -static u64 read_sanitised_id_aa64dfr0_el1(const struct id_reg_desc *idr) > +static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu, > + const struct sys_reg_desc *r) > { > u64 val; > - u32 id = reg_to_encoding(&idr->reg_desc); > + u32 id = reg_to_encoding(r); > > val = read_sanitised_ftr_reg(id); > /* Limit debug to ARMv8.0 */ > @@ -403,10 +396,11 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu, > return 0; > } > > -static u64 read_sanitised_id_dfr0_el1(const struct id_reg_desc *idr) > +static u64 read_sanitised_id_dfr0_el1(struct kvm_vcpu *vcpu, > + const struct sys_reg_desc *r) > { > u64 val; > - u32 id = reg_to_encoding(&idr->reg_desc); > + u32 id = reg_to_encoding(r); > > val = read_sanitised_ftr_reg(id); > /* > @@ -473,33 +467,23 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu, > } > > /* sys_reg_desc initialiser for known cpufeature ID registers */ > -#define SYS_DESC_SANITISED(name) { \ > +#define ID_SANITISED(name) { \ > SYS_DESC(SYS_##name), \ > .access = access_id_reg, \ > .get_user = get_id_reg, \ > .set_user = set_id_reg, \ > .visibility = id_visibility, \ > -} > - > -#define ID_SANITISED(name) { \ > - .reg_desc = SYS_DESC_SANITISED(name), \ > - .ftr_bits = NULL, \ > - .writable_mask = 0, \ > - .read_kvm_sanitised_reg = general_read_kvm_sanitised_reg, \ > + .reset = general_read_kvm_sanitised_reg, \ > } > > /* sys_reg_desc initialiser for known cpufeature ID registers */ > -#define AA32_ID_SANITISED(name) { \ > - .reg_desc = { \ > - SYS_DESC(SYS_##name), \ > - .access = access_id_reg, \ > - .get_user = get_id_reg, \ > - .set_user = set_id_reg, \ > - .visibility = aa32_id_visibility, \ > - }, \ > - .ftr_bits = NULL, \ > - .writable_mask = 0, \ > - .read_kvm_sanitised_reg = general_read_kvm_sanitised_reg, \ > +#define AA32_ID_SANITISED(name) { \ > + SYS_DESC(SYS_##name), \ > + .access = access_id_reg, \ > + .get_user = get_id_reg, \ > + .set_user = set_id_reg, \ > + .visibility = aa32_id_visibility, \ > + .reset = general_read_kvm_sanitised_reg, \ > } > > /* > @@ -508,16 +492,11 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu, > * (1 <= crm < 8, 0 <= Op2 < 8). > */ > #define ID_UNALLOCATED(crm, op2) { \ > - .reg_desc = { \ > Op0(3), Op1(0), CRn(0), CRm(crm), Op2(op2), \ > .access = access_id_reg, \ > .get_user = get_id_reg, \ > .set_user = set_id_reg, \ > .visibility = raz_visibility \ > - }, \ > - .ftr_bits = NULL, \ > - .writable_mask = 0, \ > - .read_kvm_sanitised_reg = NULL, \ > } > > /* > @@ -526,19 +505,14 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu, > * RAZ for the guest. > */ > #define ID_HIDDEN(name) { \ > - .reg_desc = { \ > SYS_DESC(SYS_##name), \ > .access = access_id_reg, \ > .get_user = get_id_reg, \ > .set_user = set_id_reg, \ > .visibility = raz_visibility, \ > - }, \ > - .ftr_bits = NULL, \ > - .writable_mask = 0, \ > - .read_kvm_sanitised_reg = NULL, \ > } > > -static struct id_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = { > +static struct sys_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = { > /* > * ID regs: all ID_SANITISED() entries here must have corresponding > * entries in arm64_ftr_regs[]. > @@ -548,15 +522,14 @@ static struct id_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = { > /* CRm=1 */ > AA32_ID_SANITISED(ID_PFR0_EL1), > AA32_ID_SANITISED(ID_PFR1_EL1), > - { .reg_desc = { > + { > 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, }, > - .ftr_bits = ftr_id_dfr0, > - .writable_mask = ID_DFR0_EL1_PerfMon_MASK, > - .read_kvm_sanitised_reg = read_sanitised_id_dfr0_el1, > + .visibility = aa32_id_visibility, > + .val = ID_DFR0_EL1_PerfMon_MASK, > + .reset = read_sanitised_id_dfr0_el1, > }, > ID_HIDDEN(ID_AFR0_EL1), > AA32_ID_SANITISED(ID_MMFR0_EL1), > @@ -586,14 +559,13 @@ static struct id_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = { > > /* AArch64 ID registers */ > /* CRm=4 */ > - { .reg_desc = { > + { > SYS_DESC(SYS_ID_AA64PFR0_EL1), > .access = access_id_reg, > .get_user = get_id_reg, > - .set_user = set_id_aa64pfr0_el1, }, > - .ftr_bits = ftr_id_aa64pfr0, > - .writable_mask = ID_AA64PFR0_EL1_CSV2_MASK | ID_AA64PFR0_EL1_CSV3_MASK, > - .read_kvm_sanitised_reg = read_sanitised_id_aa64pfr0_el1, > + .set_user = set_id_aa64pfr0_el1, > + .val = ID_AA64PFR0_EL1_CSV2_MASK | ID_AA64PFR0_EL1_CSV3_MASK, > + .reset = read_sanitised_id_aa64pfr0_el1, > }, > ID_SANITISED(ID_AA64PFR1_EL1), > ID_UNALLOCATED(4, 2), > @@ -604,14 +576,13 @@ static struct id_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = { > ID_UNALLOCATED(4, 7), > > /* CRm=5 */ > - { .reg_desc = { > + { > SYS_DESC(SYS_ID_AA64DFR0_EL1), > .access = access_id_reg, > .get_user = get_id_reg, > - .set_user = set_id_aa64dfr0_el1, }, > - .ftr_bits = ftr_id_aa64dfr0, > - .writable_mask = ID_AA64DFR0_EL1_PMUVer_MASK, > - .read_kvm_sanitised_reg = read_sanitised_id_aa64dfr0_el1, > + .set_user = set_id_aa64dfr0_el1, > + .val = ID_AA64DFR0_EL1_PMUVer_MASK, > + .reset = read_sanitised_id_aa64dfr0_el1, > }, > ID_SANITISED(ID_AA64DFR1_EL1), > ID_UNALLOCATED(5, 2), > @@ -648,7 +619,7 @@ static const struct sys_reg_desc *id_params_to_desc(struct sys_reg_params *param > > id = reg_to_encoding(params); > if (is_id_reg(id)) > - return &id_reg_descs[IDREG_IDX(id)].reg_desc; > + return &id_reg_descs[IDREG_IDX(id)]; > > return NULL; > } > @@ -742,11 +713,11 @@ bool kvm_arm_idreg_table_init(void) > unsigned int i; > > for (i = 0; i < ARRAY_SIZE(id_reg_descs); i++) { > - const struct sys_reg_desc *r = &id_reg_descs[i].reg_desc; > + const struct sys_reg_desc *r = &id_reg_descs[i]; > > if (!is_id_reg(reg_to_encoding(r))) { > kvm_err("id_reg table %pS entry %d not set correctly\n", > - &id_reg_descs[i].reg_desc, i); > + id_reg_descs, i); > return false; > } > } > @@ -756,7 +727,7 @@ bool kvm_arm_idreg_table_init(void) > > int kvm_arm_walk_id_regs(struct kvm_vcpu *vcpu, u64 __user *uind) > { > - const struct id_reg_desc *i2, *end2; > + const struct sys_reg_desc *i2, *end2; > unsigned int total = 0; > int err; > > @@ -764,7 +735,7 @@ int kvm_arm_walk_id_regs(struct kvm_vcpu *vcpu, u64 __user *uind) > end2 = id_reg_descs + ARRAY_SIZE(id_reg_descs); > > for (; i2 != end2; i2++) { > - err = walk_one_sys_reg(vcpu, &(i2->reg_desc), &uind, &total); > + err = walk_one_sys_reg(vcpu, i2, &uind, &total); > if (err) > return err; > } > @@ -779,11 +750,11 @@ void kvm_arm_init_id_regs(struct kvm *kvm) > u64 val; > > for (i = 0; i < ARRAY_SIZE(id_reg_descs); i++) { > - id = reg_to_encoding(&id_reg_descs[i].reg_desc); > + id = reg_to_encoding(&id_reg_descs[i]); > > val = 0; > - if (id_reg_descs[i].read_kvm_sanitised_reg) > - val = id_reg_descs[i].read_kvm_sanitised_reg(&id_reg_descs[i]); > + if (id_reg_descs[i].reset) > + val = id_reg_descs[i].reset(NULL, &id_reg_descs[i]); > > IDREG(kvm, id) = val; > } > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index b90d1d3ad081..c4f498e75315 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -540,10 +540,11 @@ static int get_bvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > return 0; > } > > -static void reset_bvr(struct kvm_vcpu *vcpu, > +static u64 reset_bvr(struct kvm_vcpu *vcpu, > const struct sys_reg_desc *rd) > { > vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm] = rd->val; > + return rd->val; > } > > static bool trap_bcr(struct kvm_vcpu *vcpu, > @@ -576,10 +577,11 @@ static int get_bcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > return 0; > } > > -static void reset_bcr(struct kvm_vcpu *vcpu, > +static u64 reset_bcr(struct kvm_vcpu *vcpu, > const struct sys_reg_desc *rd) > { > vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm] = rd->val; > + return rd->val; > } > > static bool trap_wvr(struct kvm_vcpu *vcpu, > @@ -613,10 +615,11 @@ static int get_wvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > return 0; > } > > -static void reset_wvr(struct kvm_vcpu *vcpu, > +static u64 reset_wvr(struct kvm_vcpu *vcpu, > const struct sys_reg_desc *rd) > { > vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm] = rd->val; > + return rd->val; > } > > static bool trap_wcr(struct kvm_vcpu *vcpu, > @@ -649,25 +652,28 @@ static int get_wcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > return 0; > } > > -static void reset_wcr(struct kvm_vcpu *vcpu, > +static u64 reset_wcr(struct kvm_vcpu *vcpu, > const struct sys_reg_desc *rd) > { > vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm] = rd->val; > + return rd->val; > } > > -static void reset_amair_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) > +static u64 reset_amair_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) > { > u64 amair = read_sysreg(amair_el1); > vcpu_write_sys_reg(vcpu, amair, AMAIR_EL1); > + return amair; > } > > -static void reset_actlr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) > +static u64 reset_actlr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) > { > u64 actlr = read_sysreg(actlr_el1); > vcpu_write_sys_reg(vcpu, actlr, ACTLR_EL1); > + return actlr; > } > > -static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) > +static u64 reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) > { > u64 mpidr; > > @@ -681,7 +687,9 @@ static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) > mpidr = (vcpu->vcpu_id & 0x0f) << MPIDR_LEVEL_SHIFT(0); > mpidr |= ((vcpu->vcpu_id >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1); > mpidr |= ((vcpu->vcpu_id >> 12) & 0xff) << MPIDR_LEVEL_SHIFT(2); > - vcpu_write_sys_reg(vcpu, (1ULL << 31) | mpidr, MPIDR_EL1); > + mpidr |= (1ULL << 31); > + vcpu_write_sys_reg(vcpu, mpidr, MPIDR_EL1); > + return mpidr; > } > > static unsigned int pmu_visibility(const struct kvm_vcpu *vcpu, > @@ -693,13 +701,13 @@ static unsigned int pmu_visibility(const struct kvm_vcpu *vcpu, > return REG_HIDDEN; > } > > -static void reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) > +static u64 reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) > { > u64 n, mask = BIT(ARMV8_PMU_CYCLE_IDX); > > /* No PMU available, any PMU reg may UNDEF... */ > if (!kvm_arm_support_pmu_v3()) > - return; > + return 0; > > n = read_sysreg(pmcr_el0) >> ARMV8_PMU_PMCR_N_SHIFT; > n &= ARMV8_PMU_PMCR_N_MASK; > @@ -708,33 +716,37 @@ static void reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) > > reset_unknown(vcpu, r); > __vcpu_sys_reg(vcpu, r->reg) &= mask; > + return __vcpu_sys_reg(vcpu, r->reg); > } > > -static void reset_pmevcntr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) > +static u64 reset_pmevcntr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) > { > reset_unknown(vcpu, r); > __vcpu_sys_reg(vcpu, r->reg) &= GENMASK(31, 0); > + return __vcpu_sys_reg(vcpu, r->reg); > } > > -static void reset_pmevtyper(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) > +static u64 reset_pmevtyper(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) > { > reset_unknown(vcpu, r); > __vcpu_sys_reg(vcpu, r->reg) &= ARMV8_PMU_EVTYPE_MASK; > + return __vcpu_sys_reg(vcpu, r->reg); > } > > -static void reset_pmselr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) > +static u64 reset_pmselr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) > { > reset_unknown(vcpu, r); > __vcpu_sys_reg(vcpu, r->reg) &= ARMV8_PMU_COUNTER_MASK; > + return __vcpu_sys_reg(vcpu, r->reg); > } > > -static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) > +static u64 reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) > { > u64 pmcr; > > /* No PMU available, PMCR_EL0 may UNDEF... */ > if (!kvm_arm_support_pmu_v3()) > - return; > + return 0; > > /* Only preserve PMCR_EL0.N, and reset the rest to 0 */ > pmcr = read_sysreg(pmcr_el0) & (ARMV8_PMU_PMCR_N_MASK << ARMV8_PMU_PMCR_N_SHIFT); > @@ -742,6 +754,7 @@ static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) > pmcr |= ARMV8_PMU_PMCR_LC; > > __vcpu_sys_reg(vcpu, r->reg) = pmcr; > + return __vcpu_sys_reg(vcpu, r->reg); > } > > static bool check_pmu_access_disabled(struct kvm_vcpu *vcpu, u64 flags) > @@ -1205,7 +1218,7 @@ static bool access_clidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, > * Fabricate a CLIDR_EL1 value instead of using the real value, which can vary > * by the physical CPU which the vcpu currently resides in. > */ > -static void reset_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) > +static u64 reset_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) > { > u64 ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0); > u64 clidr; > @@ -1253,6 +1266,7 @@ static void reset_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) > clidr |= 2 << CLIDR_TTYPE_SHIFT(loc); > > __vcpu_sys_reg(vcpu, r->reg) = clidr; > + return __vcpu_sys_reg(vcpu, r->reg); > } > > static int set_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > @@ -2603,19 +2617,21 @@ id_to_sys_reg_desc(struct kvm_vcpu *vcpu, u64 id, > */ > > #define FUNCTION_INVARIANT(reg) \ > - static void get_##reg(struct kvm_vcpu *v, \ > + static u64 get_##reg(struct kvm_vcpu *v, \ > const struct sys_reg_desc *r) \ > { \ > ((struct sys_reg_desc *)r)->val = read_sysreg(reg); \ > + return ((struct sys_reg_desc *)r)->val; \ > } > > FUNCTION_INVARIANT(midr_el1) > FUNCTION_INVARIANT(revidr_el1) > FUNCTION_INVARIANT(aidr_el1) > > -static void get_ctr_el0(struct kvm_vcpu *v, const struct sys_reg_desc *r) > +static u64 get_ctr_el0(struct kvm_vcpu *v, const struct sys_reg_desc *r) > { > ((struct sys_reg_desc *)r)->val = read_sanitised_ftr_reg(SYS_CTR_EL0); > + return ((struct sys_reg_desc *)r)->val; > } > > /* ->val is filled in by kvm_sys_reg_table_init() */ > diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h > index df8d26df93ec..d14d5b41a222 100644 > --- a/arch/arm64/kvm/sys_regs.h > +++ b/arch/arm64/kvm/sys_regs.h > @@ -65,12 +65,12 @@ struct sys_reg_desc { > const struct sys_reg_desc *); > > /* Initialization for vcpu. */ > - void (*reset)(struct kvm_vcpu *, const struct sys_reg_desc *); > + u64 (*reset)(struct kvm_vcpu *, const struct sys_reg_desc *); > > /* Index into sys_reg[], or 0 if we don't need to save it. */ > int reg; > > - /* Value (usually reset value) */ > + /* Value (usually reset value), or write mask for idregs */ > u64 val; > > /* Custom get/set_user functions, fallback to generic if NULL */ > @@ -123,19 +123,21 @@ static inline bool read_zero(struct kvm_vcpu *vcpu, > } > > /* Reset functions */ > -static inline void reset_unknown(struct kvm_vcpu *vcpu, > +static inline u64 reset_unknown(struct kvm_vcpu *vcpu, > const struct sys_reg_desc *r) > { > BUG_ON(!r->reg); > BUG_ON(r->reg >= NR_SYS_REGS); > __vcpu_sys_reg(vcpu, r->reg) = 0x1de7ec7edbadc0deULL; > + return __vcpu_sys_reg(vcpu, r->reg); > } > > -static inline void reset_val(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) > +static inline u64 reset_val(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) > { > BUG_ON(!r->reg); > BUG_ON(r->reg >= NR_SYS_REGS); > __vcpu_sys_reg(vcpu, r->reg) = r->val; > + return __vcpu_sys_reg(vcpu, r->reg); > } > > static inline unsigned int sysreg_visibility(const struct kvm_vcpu *vcpu, Really appreciate it. It looks great! Will reimplement based on your suggestion. > > -- > Without deviation from the norm, progress is not possible. Thanks, Jing
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h index 6bf013fb110d..f17e74afe3e9 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; @@ -925,6 +926,10 @@ extern struct arm64_ftr_override id_aa64smfr0_override; extern struct arm64_ftr_override id_aa64isar1_override; extern struct arm64_ftr_override id_aa64isar2_override; +extern const struct arm64_ftr_bits ftr_id_dfr0[]; +extern const struct arm64_ftr_bits ftr_id_aa64pfr0[]; +extern const struct arm64_ftr_bits ftr_id_aa64dfr0[]; + u32 get_kvm_ipa_limit(void); void dump_cpu_features(void); diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index c331c49a7d19..5b0e3379e5f8 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -225,7 +225,7 @@ static const struct arm64_ftr_bits ftr_id_aa64isar2[] = { ARM64_FTR_END, }; -static const struct arm64_ftr_bits ftr_id_aa64pfr0[] = { +const struct arm64_ftr_bits ftr_id_aa64pfr0[] = { ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL1_CSV3_SHIFT, 4, 0), ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL1_CSV2_SHIFT, 4, 0), ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL1_DIT_SHIFT, 4, 0), @@ -426,7 +426,7 @@ static const struct arm64_ftr_bits ftr_id_mmfr0[] = { ARM64_FTR_END, }; -static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = { +const struct arm64_ftr_bits ftr_id_aa64dfr0[] = { S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_DoubleLock_SHIFT, 4, 0), ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_PMSVer_SHIFT, 4, 0), ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_CTX_CMPs_SHIFT, 4, 0), @@ -578,7 +578,7 @@ static const struct arm64_ftr_bits ftr_id_pfr2[] = { ARM64_FTR_END, }; -static const struct arm64_ftr_bits ftr_id_dfr0[] = { +const struct arm64_ftr_bits ftr_id_dfr0[] = { /* [31:28] TraceFilt */ S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_EXACT, ID_DFR0_EL1_PerfMon_SHIFT, 4, 0), ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_DFR0_EL1_MProfDbg_SHIFT, 4, 0), @@ -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 af86001e2686..395eaf84a0ab 100644 --- a/arch/arm64/kvm/id_regs.c +++ b/arch/arm64/kvm/id_regs.c @@ -39,6 +39,64 @@ struct id_reg_desc { u64 (*read_kvm_sanitised_reg)(const struct id_reg_desc *idr); }; +static struct id_reg_desc id_reg_descs[]; + +/** + * arm64_check_features() - Check if a feature register value constitutes + * a subset of features indicated by @limit. + * + * @ftrp: Pointer to an array of arm64_ftr_bits. It must be terminated by + * an item whose width field is zero. + * @writable_mask: Indicates writable feature bits. + * @val: The feature register value to check + * @limit: The limit value of the feature register + * + * This function will check if each feature field of @val is the "safe" value + * against @limit based on @ftrp[], each of which specifies the target field + * (shift, width), whether or not the field is for a signed value (sign), + * how the field is determined to be "safe" (type), and the safe value + * (safe_val) when type == FTR_EXACT (safe_val won't be used by this + * function when type != FTR_EXACT). Any other fields in arm64_ftr_bits + * won't be used by this function. If a field value in @val is the same + * as the one in @limit, it is always considered the safe value regardless + * of the type. 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(const struct arm64_ftr_bits *ftrp, + u64 writable_mask, u64 val, u64 limit) +{ + u64 mask = 0; + + 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 = arm64_ftr_safe_value(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)) @@ -84,7 +142,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); @@ -111,15 +168,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); @@ -178,9 +230,16 @@ 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; + int ret; + int id = reg_to_encoding(rd); + const struct id_reg_desc *idr = &id_reg_descs[IDREG_IDX(id)]; + + ret = arm64_check_features(idr->ftr_bits, idr->writable_mask, val, + idr->read_kvm_sanitised_reg ? idr->read_kvm_sanitised_reg(idr) : 0); + if (ret) + return ret; + + IDREG(vcpu->kvm, id) = val; return 0; } @@ -219,12 +278,39 @@ static u64 general_read_kvm_sanitised_reg(const struct id_reg_desc *idr) return read_sanitised_ftr_reg(reg_to_encoding(&idr->reg_desc)); } +static u64 read_sanitised_id_aa64pfr0_el1(const struct id_reg_desc *idr) +{ + u64 val; + u32 id = reg_to_encoding(&idr->reg_desc); + + 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) { u8 csv2, csv3; - u64 sval = val; /* * Allow AA64PFR0_EL1.CSV2 to be set from userspace as long as @@ -232,26 +318,37 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, * 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)) + 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)) + if (csv3 > 1 || (csv3 && arm64_get_meltdown_state() != SPECTRE_UNAFFECTED)) return -EINVAL; - /* 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 set_id_reg(vcpu, rd, val); +} - IDREG(vcpu->kvm, reg_to_encoding(rd)) = sval; +static u64 read_sanitised_id_aa64dfr0_el1(const struct id_reg_desc *idr) +{ + u64 val; + u32 id = reg_to_encoding(&idr->reg_desc); - return 0; + 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, @@ -260,6 +357,7 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu, { u8 pmuver, host_pmuver; bool valid_pmu; + int ret; host_pmuver = kvm_arm_pmu_get_pmuver_limit(); @@ -279,23 +377,25 @@ 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) { mutex_lock(&vcpu->kvm->arch.config_lock); - IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer); - IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) |= - FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer), pmuver); + ret = set_id_reg(vcpu, rd, val); + if (ret) { + mutex_unlock(&vcpu->kvm->arch.config_lock); + return ret; + } IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) &= ~ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon); IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) |= FIELD_PREP(ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon), pmuver_to_perfmon(pmuver)); mutex_unlock(&vcpu->kvm->arch.config_lock); } else { + /* 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; + assign_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU, &vcpu->kvm->arch.flags, pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF); } @@ -303,12 +403,29 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu, return 0; } +static u64 read_sanitised_id_dfr0_el1(const struct id_reg_desc *idr) +{ + u64 val; + u32 id = reg_to_encoding(&idr->reg_desc); + + 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) { u8 perfmon, host_perfmon; bool valid_pmu; + int ret; host_perfmon = pmuver_to_perfmon(kvm_arm_pmu_get_pmuver_limit()); @@ -329,23 +446,25 @@ 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) { mutex_lock(&vcpu->kvm->arch.config_lock); - IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) &= ~ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon); - IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) |= - FIELD_PREP(ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon), perfmon); + ret = set_id_reg(vcpu, rd, val); + if (ret) { + mutex_unlock(&vcpu->kvm->arch.config_lock); + return ret; + } IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer); IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer), perfmon_to_pmuver(perfmon)); mutex_unlock(&vcpu->kvm->arch.config_lock); } else { + /* 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; + assign_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU, &vcpu->kvm->arch.flags, perfmon == ID_DFR0_EL1_PerfMon_IMPDEF); } @@ -354,14 +473,16 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu, } /* sys_reg_desc initialiser for known cpufeature ID registers */ +#define SYS_DESC_SANITISED(name) { \ + SYS_DESC(SYS_##name), \ + .access = access_id_reg, \ + .get_user = get_id_reg, \ + .set_user = set_id_reg, \ + .visibility = id_visibility, \ +} + #define ID_SANITISED(name) { \ - .reg_desc = { \ - SYS_DESC(SYS_##name), \ - .access = access_id_reg, \ - .get_user = get_id_reg, \ - .set_user = set_id_reg, \ - .visibility = id_visibility, \ - }, \ + .reg_desc = SYS_DESC_SANITISED(name), \ .ftr_bits = NULL, \ .writable_mask = 0, \ .read_kvm_sanitised_reg = general_read_kvm_sanitised_reg, \ @@ -417,7 +538,7 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu, .read_kvm_sanitised_reg = NULL, \ } -static const struct id_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = { +static struct id_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = { /* * ID regs: all ID_SANITISED() entries here must have corresponding * entries in arm64_ftr_regs[]. @@ -433,6 +554,9 @@ static const struct id_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = { .get_user = get_id_reg, .set_user = set_id_dfr0_el1, .visibility = aa32_id_visibility, }, + .ftr_bits = ftr_id_dfr0, + .writable_mask = ID_DFR0_EL1_PerfMon_MASK, + .read_kvm_sanitised_reg = read_sanitised_id_dfr0_el1, }, ID_HIDDEN(ID_AFR0_EL1), AA32_ID_SANITISED(ID_MMFR0_EL1), @@ -467,6 +591,9 @@ static const struct id_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = { .access = access_id_reg, .get_user = get_id_reg, .set_user = set_id_aa64pfr0_el1, }, + .ftr_bits = ftr_id_aa64pfr0, + .writable_mask = ID_AA64PFR0_EL1_CSV2_MASK | ID_AA64PFR0_EL1_CSV3_MASK, + .read_kvm_sanitised_reg = read_sanitised_id_aa64pfr0_el1, }, ID_SANITISED(ID_AA64PFR1_EL1), ID_UNALLOCATED(4, 2), @@ -482,6 +609,9 @@ static const struct id_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = { .access = access_id_reg, .get_user = get_id_reg, .set_user = set_id_aa64dfr0_el1, }, + .ftr_bits = ftr_id_aa64dfr0, + .writable_mask = ID_AA64DFR0_EL1_PMUVer_MASK, + .read_kvm_sanitised_reg = read_sanitised_id_aa64dfr0_el1, }, ID_SANITISED(ID_AA64DFR1_EL1), ID_UNALLOCATED(5, 2), @@ -607,7 +737,7 @@ int kvm_arm_set_id_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) return ret; } -bool kvm_arm_check_idreg_table(void) +bool kvm_arm_idreg_table_init(void) { unsigned int i; @@ -641,10 +771,7 @@ int kvm_arm_walk_id_regs(struct kvm_vcpu *vcpu, u64 __user *uind) return total; } -/* - * Set the guest's ID registers that are defined in id_reg_descs[] - * with ID_SANITISED() to the host's sanitized value. - */ +/* Initialize the guest's ID registers with KVM sanitised values. */ void kvm_arm_init_id_regs(struct kvm *kvm) { int i; @@ -660,33 +787,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. - */ - IDREG(kvm, SYS_ID_AA64DFR0_EL1) &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer); - IDREG(kvm, SYS_ID_AA64DFR0_EL1) |= - FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer), kvm_arm_pmu_get_pmuver_limit()); } diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 1d7fb0acf154..27cff8022754 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -2940,14 +2940,13 @@ int __init kvm_sys_reg_table_init(void) unsigned int i; /* Make sure tables are unique and in order. */ - valid &= kvm_arm_check_idreg_table(); valid &= check_sysreg_table(sys_reg_descs, ARRAY_SIZE(sys_reg_descs), false); valid &= check_sysreg_table(cp14_regs, ARRAY_SIZE(cp14_regs), true); valid &= check_sysreg_table(cp14_64_regs, ARRAY_SIZE(cp14_64_regs), true); valid &= check_sysreg_table(cp15_regs, ARRAY_SIZE(cp15_regs), true); valid &= check_sysreg_table(cp15_64_regs, ARRAY_SIZE(cp15_64_regs), true); valid &= check_sysreg_table(invariant_sys_regs, ARRAY_SIZE(invariant_sys_regs), false); - + valid &= kvm_arm_idreg_table_init(); if (!valid) return -EINVAL; diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h index 15a0a1e2fe99..df8d26df93ec 100644 --- a/arch/arm64/kvm/sys_regs.h +++ b/arch/arm64/kvm/sys_regs.h @@ -246,7 +246,7 @@ int walk_one_sys_reg(const struct kvm_vcpu *vcpu, int emulate_id_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params); int kvm_arm_get_id_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg); int kvm_arm_set_id_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg); -bool kvm_arm_check_idreg_table(void); +bool kvm_arm_idreg_table_init(void); int kvm_arm_walk_id_regs(struct kvm_vcpu *vcpu, u64 __user *uind); u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id);