On Wed, 2023-05-03 at 17:16 +0000, Jing Zhang wrote:
> Refactor writings for ID_AA64PFR0_EL1.[CSV2|CSV3],
> ID_AA64DFR0_EL1.PMUVer and ID_DFR0_ELF.PerfMon based on utilities
> introduced by ID register descriptor array.
>
> Signed-off-by: Jing Zhang <jingzhangos@xxxxxxxxxx>
> ---
> arch/arm64/include/asm/cpufeature.h | 1 +
> arch/arm64/kernel/cpufeature.c | 2 +-
> arch/arm64/kvm/id_regs.c | 361 ++++++++++++++++++--------
> --
> 3 files changed, 242 insertions(+), 122 deletions(-)
>
>
[ SNIP ]
>
> +static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
> + const struct sys_reg_desc
> *rd)
> +{
> + u64 val;
> + u32 id = reg_to_encoding(rd);
> +
> + val = read_sanitised_ftr_reg(id);
> + /*
> + * The default is to expose CSV2 == 1 if the HW isn't
> affected.
> + * Although this is a per-CPU feature, we make it global
> because
> + * asymmetric systems are just a nuisance.
> + *
> + * Userspace can override this as long as it doesn't promise
> + * the impossible.
> + */
> + if (arm64_get_spectre_v2_state() == SPECTRE_UNAFFECTED) {
> + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2);
> + val |=
> FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2), 1);
> + }
> + if (arm64_get_meltdown_state() == SPECTRE_UNAFFECTED) {
> + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3);
> + val |=
> FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3), 1);
> + }
> +
> + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_AMU);
> +
> + return val;
> +}
> +
> static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
> const struct sys_reg_desc *rd,
> u64 val)
> {
> - struct kvm_arch *arch = &vcpu->kvm->arch;
> - u64 sval = val;
> u8 csv2, csv3;
> - int ret = 0;
>
> /*
> * Allow AA64PFR0_EL1.CSV2 to be set from userspace as long
> as
> @@ -226,26 +338,30 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu
> *vcpu,
> if (csv3 > 1 || (csv3 && arm64_get_meltdown_state() !=
> SPECTRE_UNAFFECTED))
> return -EINVAL;
Can't we remove the checking of csv[23] here as it will be checked by
arm64_check_features()?
i.e. in arm64_check_features() we will load the "limit" value from the
"reset" function (read_sanitised_id_aa64pfr0_el1()) which has csv[23]
set appropriately and limit it to a safe value basically performing the
same check as we are here.
>
> - mutex_lock(&arch->config_lock);
> - /* We can only differ with CSV[23], and anything else is an
> error */
> - val ^= read_id_reg(vcpu, rd);
> - val &= ~(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2) |
> - ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3));
> - if (val) {
> - ret = -EINVAL;
> - goto out;
> - }
> + return set_id_reg(vcpu, rd, val);
> +}
>
> - /* Only allow userspace to change the idregs before VM
> running */
> - if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &vcpu->kvm-
> >arch.flags)) {
> - if (sval != read_id_reg(vcpu, rd))
> - ret = -EBUSY;
> - } else {
> - IDREG(vcpu->kvm, reg_to_encoding(rd)) = sval;
> - }
> -out:
> - mutex_unlock(&arch->config_lock);
> - return ret;
> +static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
> + const struct sys_reg_desc
> *rd)
> +{
> + u64 val;
> + u32 id = reg_to_encoding(rd);
> +
> + val = read_sanitised_ftr_reg(id);
> + /* Limit debug to ARMv8.0 */
> + val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_DebugVer);
> + val |=
> FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_DebugVer), 6);
> + /*
> + * Initialise the default PMUver before there is a chance to
> + * create an actual PMU.
> + */
> + val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer);
> + val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer),
> + kvm_arm_pmu_get_pmuver_limit());
> + /* Hide SPE from guests */
> + val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMSVer);
> +
> + return val;
> }
>
> static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
> @@ -255,7 +371,6 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu
> *vcpu,
> struct kvm_arch *arch = &vcpu->kvm->arch;
> u8 pmuver, host_pmuver;
> bool valid_pmu;
> - u64 sval = val;
> int ret = 0;
>
> host_pmuver = kvm_arm_pmu_get_pmuver_limit();
> @@ -277,40 +392,61 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu
> *vcpu,
> return -EINVAL;
>
> mutex_lock(&arch->config_lock);
> - /* We can only differ with PMUver, and anything else is an
> error */
> - val ^= read_id_reg(vcpu, rd);
> - val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer);
> - if (val) {
> - ret = -EINVAL;
> - goto out;
> - }
> -
> /* Only allow userspace to change the idregs before VM
> running */
> if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &vcpu->kvm-
> >arch.flags)) {
> - if (sval != read_id_reg(vcpu, rd))
> + if (val != read_id_reg(vcpu, rd))
> ret = -EBUSY;
> - } else {
> - if (valid_pmu) {
> - val = IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1);
> - val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
> - val |=
> FIELD_PREP(ID_AA64DFR0_EL1_PMUVer_MASK, pmuver);
> - IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) = val;
> -
> - val = IDREG(vcpu->kvm, SYS_ID_DFR0_EL1);
> - val &= ~ID_DFR0_EL1_PerfMon_MASK;
> - val |= FIELD_PREP(ID_DFR0_EL1_PerfMon_MASK,
> pmuver_to_perfmon(pmuver));
> - IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) = val;
> - } else {
> -
> assign_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU,
> &vcpu->kvm->arch.flags,
> - pmuver ==
> ID_AA64DFR0_EL1_PMUVer_IMP_DEF);
> - }
> + goto out;
> + }
> +
> + if (!valid_pmu) {
> + /*
> + * Ignore the PMUVer filed in @val. The PMUVer would
Nit s/filed/field
> be determined
> + * by arch flags bit
> KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU,
> + */
> + pmuver = FIELD_GET(ID_AA64DFR0_EL1_PMUVer_MASK,
> + IDREG(vcpu->kvm,
> SYS_ID_AA64DFR0_EL1));
> + val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
> + val |= FIELD_PREP(ID_AA64DFR0_EL1_PMUVer_MASK,
> pmuver);
> }
>
> + ret = arm64_check_features(vcpu, rd, val);
> + if (ret)
> + goto out;
> +
> + IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) = val;
> +
> + val = IDREG(vcpu->kvm, SYS_ID_DFR0_EL1);
> + val &= ~ID_DFR0_EL1_PerfMon_MASK;
> + val |= FIELD_PREP(ID_DFR0_EL1_PerfMon_MASK,
> pmuver_to_perfmon(pmuver));
> + IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) = val;
> +
> + if (!valid_pmu)
> + assign_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU, &vcpu-
> >kvm->arch.flags,
> + pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF);
> +
> out:
> mutex_unlock(&arch->config_lock);
> return ret;
> }
>
> +static u64 read_sanitised_id_dfr0_el1(struct kvm_vcpu *vcpu,
> + const struct sys_reg_desc *rd)
> +{
> + u64 val;
> + u32 id = reg_to_encoding(rd);
> +
> + val = read_sanitised_ftr_reg(id);
> + /*
> + * Initialise the default PMUver before there is a chance to
> + * create an actual PMU.
> + */
> + val &= ~ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon);
> + val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon),
> kvm_arm_pmu_get_pmuver_limit());
> +
> + return val;
> +}
> +
> static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
> const struct sys_reg_desc *rd,
> u64 val)
> @@ -318,7 +454,6 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
> struct kvm_arch *arch = &vcpu->kvm->arch;
> u8 perfmon, host_perfmon;
> bool valid_pmu;
> - u64 sval = val;
> int ret = 0;
>
> host_perfmon =
> pmuver_to_perfmon(kvm_arm_pmu_get_pmuver_limit());
> @@ -341,35 +476,39 @@ static int set_id_dfr0_el1(struct kvm_vcpu
> *vcpu,
> return -EINVAL;
>
> mutex_lock(&arch->config_lock);
> - /* We can only differ with PerfMon, and anything else is an
> error */
> - val ^= read_id_reg(vcpu, rd);
> - val &= ~ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon);
> - if (val) {
> - ret = -EINVAL;
> - goto out;
> - }
> -
> /* Only allow userspace to change the idregs before VM
> running */
> if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &vcpu->kvm-
> >arch.flags)) {
> - if (sval != read_id_reg(vcpu, rd))
> + if (val != read_id_reg(vcpu, rd))
> ret = -EBUSY;
> - } else {
> - if (valid_pmu) {
> - val = IDREG(vcpu->kvm, SYS_ID_DFR0_EL1);
> - val &= ~ID_DFR0_EL1_PerfMon_MASK;
> - val |= FIELD_PREP(ID_DFR0_EL1_PerfMon_MASK,
> perfmon);
> - IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) = val;
> -
> - val = IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1);
> - val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
> - val |=
> FIELD_PREP(ID_AA64DFR0_EL1_PMUVer_MASK, perfmon_to_pmuver(perfmon));
> - IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) = val;
> - } else {
> -
> assign_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU,
> &vcpu->kvm->arch.flags,
> - perfmon ==
> ID_DFR0_EL1_PerfMon_IMPDEF);
> - }
> + goto out;
> + }
> +
> + if (!valid_pmu) {
> + /*
> + * Ignore the PerfMon filed in @val. The PerfMon
Nit s/filed/field
> would be determined
> + * by arch flags bit
> KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU,
> + */
> + perfmon = FIELD_GET(ID_DFR0_EL1_PerfMon_MASK,
> + IDREG(vcpu->kvm,
> SYS_ID_DFR0_EL1));
> + val &= ~ID_DFR0_EL1_PerfMon_MASK;
> + val |= FIELD_PREP(ID_DFR0_EL1_PerfMon_MASK, perfmon);
> }
>
> + ret = arm64_check_features(vcpu, rd, val);
> + if (ret)
> + goto out;
> +
> + IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) = val;
> +
> + val = IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1);
> + val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
> + val |= FIELD_PREP(ID_AA64DFR0_EL1_PMUVer_MASK,
> perfmon_to_pmuver(perfmon));
> + IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) = val;
> +
> + if (!valid_pmu)
> + assign_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU, &vcpu-
> >kvm->arch.flags,
> + perfmon == ID_DFR0_EL1_PerfMon_IMPDEF);
> +
> out:
> mutex_unlock(&arch->config_lock);
> return ret;
Otherwise looks good!
Thanks,
Suraj
