| Message ID | 20211117064359.2362060-5-reijiw@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Make CPU ID registers writable by userspace | expand |
On Wed, 17 Nov 2021 06:43:34 +0000, Reiji Watanabe <reijiw@google.com> wrote: > > This patch adds id_reg_info for ID_AA64PFR0_EL1 to make it writable by > userspace. > > The CSV2/CSV3 fields of the register were already writable and values > that were written for them affected all vCPUs before. Now they only > affect the vCPU. > Return an error if userspace tries to set SVE/GIC field of the register > to a value that conflicts with SVE/GIC configuration for the guest. > SIMD/FP/SVE fields of the requested value are validated according to > Arm ARM. > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > --- > arch/arm64/kvm/sys_regs.c | 159 ++++++++++++++++++++++++-------------- > 1 file changed, 103 insertions(+), 56 deletions(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 1552cd5581b7..35400869067a 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -401,6 +401,92 @@ static void id_reg_info_init(struct id_reg_info *id_reg) > id_reg->init(id_reg); > } > > +#define kvm_has_gic3(kvm) \ > + (irqchip_in_kernel(kvm) && \ > + (kvm)->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) > + > +static int validate_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > + const struct id_reg_info *id_reg, u64 val) > +{ > + int fp, simd; > + bool vcpu_has_sve = vcpu_has_sve(vcpu); > + bool pfr0_has_sve = id_aa64pfr0_sve(val); > + int gic; > + > + simd = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_ASIMD_SHIFT); > + fp = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_FP_SHIFT); > + if (simd != fp) > + return -EINVAL; > + > + /* fp must be supported when sve is supported */ > + if (pfr0_has_sve && (fp < 0)) > + return -EINVAL; > + > + /* Check if there is a conflict with a request via KVM_ARM_VCPU_INIT */ > + if (vcpu_has_sve ^ pfr0_has_sve) > + return -EPERM; > + > + gic = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_GIC_SHIFT); > + if ((gic > 0) ^ kvm_has_gic3(vcpu->kvm)) > + return -EPERM; > + > + return 0; > +} > + > +static void init_id_aa64pfr0_el1_info(struct id_reg_info *id_reg) > +{ > + u64 limit = id_reg->vcpu_limit_val; > + unsigned int gic; > + > + limit &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_AMU); > + if (!system_supports_sve()) > + limit &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_SVE); > + > + /* > + * The default is to expose CSV2 == 1 and CSV3 == 1 if the HW > + * isn't affected. Userspace can override this as long as it > + * doesn't promise the impossible. > + */ > + limit &= ~(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2) | > + ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3)); > + > + if (arm64_get_spectre_v2_state() == SPECTRE_UNAFFECTED) > + limit |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2), 1); > + if (arm64_get_meltdown_state() == SPECTRE_UNAFFECTED) > + limit |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3), 1); > + > + gic = cpuid_feature_extract_unsigned_field(limit, ID_AA64PFR0_GIC_SHIFT); > + if (gic > 1) { > + /* Limit to GICv3.0/4.0 */ > + limit &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_GIC); > + limit |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_GIC), 1); > + } > + id_reg->vcpu_limit_val = limit; > +} > + > +static u64 get_reset_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > + const struct id_reg_info *idr) > +{ > + u64 val = idr->vcpu_limit_val; > + > + if (!vcpu_has_sve(vcpu)) > + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_SVE); > + > + if (!kvm_has_gic3(vcpu->kvm)) > + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_GIC); No. As I said in a previous email, this breaks migration, and advertising a GICv3 CPU interface doesn't mean it is usable (the guest OS must check that it can actually enable ICC_SRE_EL1.SRE -- see what the Linux GICv3 driver does for an example). > + > + return val; > +} > + > +static struct id_reg_info id_aa64pfr0_el1_info = { > + .sys_reg = SYS_ID_AA64PFR0_EL1, > + .ftr_check_types = S_FCT(ID_AA64PFR0_ASIMD_SHIFT, FCT_LOWER_SAFE) | > + S_FCT(ID_AA64PFR0_FP_SHIFT, FCT_LOWER_SAFE), > + .init = init_id_aa64pfr0_el1_info, > + .validate = validate_id_aa64pfr0_el1, > + .get_reset_val = get_reset_id_aa64pfr0_el1, > +}; > + > /* > * An ID register that needs special handling to control the value for the > * guest must have its own id_reg_info in id_reg_info_table. > @@ -409,7 +495,9 @@ static void id_reg_info_init(struct id_reg_info *id_reg) > * validation, etc.) > */ > #define GET_ID_REG_INFO(id) (id_reg_info_table[IDREG_IDX(id)]) > -static struct id_reg_info *id_reg_info_table[KVM_ARM_ID_REG_MAX_NUM] = {}; > +static struct id_reg_info *id_reg_info_table[KVM_ARM_ID_REG_MAX_NUM] = { > + [IDREG_IDX(SYS_ID_AA64PFR0_EL1)] = &id_aa64pfr0_el1_info, > +}; > > static int validate_id_reg(struct kvm_vcpu *vcpu, > const struct sys_reg_desc *rd, u64 val) > @@ -1239,20 +1327,22 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu, > static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id) > { > u64 val = __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id)); > + u64 lim, gic, gic_lim; > + const struct id_reg_info *id_reg; > > switch (id) { > case SYS_ID_AA64PFR0_EL1: > - if (!vcpu_has_sve(vcpu)) > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_SVE); > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_AMU); > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2); > - val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2), (u64)vcpu->kvm->arch.pfr0_csv2); > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3); > - val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3), (u64)vcpu->kvm->arch.pfr0_csv3); > - if (irqchip_in_kernel(vcpu->kvm) && > - vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) { > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_GIC); > - val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_GIC), 1); > + gic = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_GIC_SHIFT); > + if (kvm_has_gic3(vcpu->kvm) && (gic == 0)) { > + /* > + * This is a case where userspace configured gic3 after > + * the vcpu was created, and then it didn't set > + * ID_AA64PFR0_EL1. > + */ Shouldn't that be done at the point where a GICv3 is created, rather than after the fact? Thanks, M.
On Sun, Nov 21, 2021 at 4:37 AM Marc Zyngier <maz@kernel.org> wrote: > > On Wed, 17 Nov 2021 06:43:34 +0000, > Reiji Watanabe <reijiw@google.com> wrote: > > > > This patch adds id_reg_info for ID_AA64PFR0_EL1 to make it writable by > > userspace. > > > > The CSV2/CSV3 fields of the register were already writable and values > > that were written for them affected all vCPUs before. Now they only > > affect the vCPU. > > Return an error if userspace tries to set SVE/GIC field of the register > > to a value that conflicts with SVE/GIC configuration for the guest. > > SIMD/FP/SVE fields of the requested value are validated according to > > Arm ARM. > > > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > > --- > > arch/arm64/kvm/sys_regs.c | 159 ++++++++++++++++++++++++-------------- > > 1 file changed, 103 insertions(+), 56 deletions(-) > > > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > > index 1552cd5581b7..35400869067a 100644 > > --- a/arch/arm64/kvm/sys_regs.c > > +++ b/arch/arm64/kvm/sys_regs.c > > @@ -401,6 +401,92 @@ static void id_reg_info_init(struct id_reg_info *id_reg) > > id_reg->init(id_reg); > > } > > > > +#define kvm_has_gic3(kvm) \ > > + (irqchip_in_kernel(kvm) && \ > > + (kvm)->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) > > + > > +static int validate_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > > + const struct id_reg_info *id_reg, u64 val) > > +{ > > + int fp, simd; > > + bool vcpu_has_sve = vcpu_has_sve(vcpu); > > + bool pfr0_has_sve = id_aa64pfr0_sve(val); > > + int gic; > > + > > + simd = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_ASIMD_SHIFT); > > + fp = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_FP_SHIFT); > > + if (simd != fp) > > + return -EINVAL; > > + > > + /* fp must be supported when sve is supported */ > > + if (pfr0_has_sve && (fp < 0)) > > + return -EINVAL; > > + > > + /* Check if there is a conflict with a request via KVM_ARM_VCPU_INIT */ > > + if (vcpu_has_sve ^ pfr0_has_sve) > > + return -EPERM; > > + > > + gic = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_GIC_SHIFT); > > + if ((gic > 0) ^ kvm_has_gic3(vcpu->kvm)) > > + return -EPERM; > > + > > + return 0; > > +} > > + > > +static void init_id_aa64pfr0_el1_info(struct id_reg_info *id_reg) > > +{ > > + u64 limit = id_reg->vcpu_limit_val; > > + unsigned int gic; > > + > > + limit &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_AMU); > > + if (!system_supports_sve()) > > + limit &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_SVE); > > + > > + /* > > + * The default is to expose CSV2 == 1 and CSV3 == 1 if the HW > > + * isn't affected. Userspace can override this as long as it > > + * doesn't promise the impossible. > > + */ > > + limit &= ~(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2) | > > + ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3)); > > + > > + if (arm64_get_spectre_v2_state() == SPECTRE_UNAFFECTED) > > + limit |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2), 1); > > + if (arm64_get_meltdown_state() == SPECTRE_UNAFFECTED) > > + limit |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3), 1); > > + > > + gic = cpuid_feature_extract_unsigned_field(limit, ID_AA64PFR0_GIC_SHIFT); > > + if (gic > 1) { > > + /* Limit to GICv3.0/4.0 */ > > + limit &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_GIC); > > + limit |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_GIC), 1); > > + } > > + id_reg->vcpu_limit_val = limit; > > +} > > + > > +static u64 get_reset_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > > + const struct id_reg_info *idr) > > +{ > > + u64 val = idr->vcpu_limit_val; > > + > > + if (!vcpu_has_sve(vcpu)) > > + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_SVE); > > + > > + if (!kvm_has_gic3(vcpu->kvm)) > > + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_GIC); > > No. As I said in a previous email, this breaks migration, and > advertising a GICv3 CPU interface doesn't mean it is usable (the guest > OS must check that it can actually enable ICC_SRE_EL1.SRE -- see what > the Linux GICv3 driver does for an example). Yes, I understand. I will remove that code. > > + return val; > > +} > > + > > +static struct id_reg_info id_aa64pfr0_el1_info = { > > + .sys_reg = SYS_ID_AA64PFR0_EL1, > > + .ftr_check_types = S_FCT(ID_AA64PFR0_ASIMD_SHIFT, FCT_LOWER_SAFE) | > > + S_FCT(ID_AA64PFR0_FP_SHIFT, FCT_LOWER_SAFE), > > + .init = init_id_aa64pfr0_el1_info, > > + .validate = validate_id_aa64pfr0_el1, > > + .get_reset_val = get_reset_id_aa64pfr0_el1, > > +}; > > + > > /* > > * An ID register that needs special handling to control the value for the > > * guest must have its own id_reg_info in id_reg_info_table. > > @@ -409,7 +495,9 @@ static void id_reg_info_init(struct id_reg_info *id_reg) > > * validation, etc.) > > */ > > #define GET_ID_REG_INFO(id) (id_reg_info_table[IDREG_IDX(id)]) > > -static struct id_reg_info *id_reg_info_table[KVM_ARM_ID_REG_MAX_NUM] = {}; > > +static struct id_reg_info *id_reg_info_table[KVM_ARM_ID_REG_MAX_NUM] = { > > + [IDREG_IDX(SYS_ID_AA64PFR0_EL1)] = &id_aa64pfr0_el1_info, > > +}; > > > > static int validate_id_reg(struct kvm_vcpu *vcpu, > > const struct sys_reg_desc *rd, u64 val) > > @@ -1239,20 +1327,22 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu, > > static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id) > > { > > u64 val = __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id)); > > + u64 lim, gic, gic_lim; > > + const struct id_reg_info *id_reg; > > > > switch (id) { > > case SYS_ID_AA64PFR0_EL1: > > - if (!vcpu_has_sve(vcpu)) > > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_SVE); > > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_AMU); > > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2); > > - val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2), (u64)vcpu->kvm->arch.pfr0_csv2); > > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3); > > - val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3), (u64)vcpu->kvm->arch.pfr0_csv3); > > - if (irqchip_in_kernel(vcpu->kvm) && > > - vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) { > > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_GIC); > > - val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_GIC), 1); > > + gic = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_GIC_SHIFT); > > + if (kvm_has_gic3(vcpu->kvm) && (gic == 0)) { > > + /* > > + * This is a case where userspace configured gic3 after > > + * the vcpu was created, and then it didn't set > > + * ID_AA64PFR0_EL1. > > + */ > > Shouldn't that be done at the point where a GICv3 is created, rather > than after the fact? I will look into having it done at the point where a GICv3 is created. (I originally chose this way because I wanted to avoid access to other vCPUs' ID registers if possible) Thanks, Reiji
Hi Reiji, On 11/17/21 7:43 AM, Reiji Watanabe wrote: > This patch adds id_reg_info for ID_AA64PFR0_EL1 to make it writable by > userspace. > > The CSV2/CSV3 fields of the register were already writable and values > that were written for them affected all vCPUs before. Now they only > affect the vCPU. > Return an error if userspace tries to set SVE/GIC field of the register > to a value that conflicts with SVE/GIC configuration for the guest. > SIMD/FP/SVE fields of the requested value are validated according to > Arm ARM. > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > --- > arch/arm64/kvm/sys_regs.c | 159 ++++++++++++++++++++++++-------------- > 1 file changed, 103 insertions(+), 56 deletions(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 1552cd5581b7..35400869067a 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -401,6 +401,92 @@ static void id_reg_info_init(struct id_reg_info *id_reg) > id_reg->init(id_reg); > } > > +#define kvm_has_gic3(kvm) \ > + (irqchip_in_kernel(kvm) && \ > + (kvm)->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) you may move this macro to kvm/arm_vgic.h as this may be used in vgic/vgic-v3.c too > + > +static int validate_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > + const struct id_reg_info *id_reg, u64 val) > +{ > + int fp, simd; > + bool vcpu_has_sve = vcpu_has_sve(vcpu); > + bool pfr0_has_sve = id_aa64pfr0_sve(val); > + int gic; > + > + simd = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_ASIMD_SHIFT); > + fp = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_FP_SHIFT); > + if (simd != fp) > + return -EINVAL; > + > + /* fp must be supported when sve is supported */ > + if (pfr0_has_sve && (fp < 0)) > + return -EINVAL; > + > + /* Check if there is a conflict with a request via KVM_ARM_VCPU_INIT */ > + if (vcpu_has_sve ^ pfr0_has_sve) > + return -EPERM; > + > + gic = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_GIC_SHIFT); > + if ((gic > 0) ^ kvm_has_gic3(vcpu->kvm)) > + return -EPERM; Sometimes from a given architecture version, some lower values are not allowed. For instance from ARMv8.5 onlt 1 is permitted for CSV3. Shouldn't we handle that kind of check? > + > + return 0; > +} > + > +static void init_id_aa64pfr0_el1_info(struct id_reg_info *id_reg) > +{ > + u64 limit = id_reg->vcpu_limit_val; > + unsigned int gic; > + > + limit &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_AMU); > + if (!system_supports_sve()) > + limit &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_SVE); > + > + /* > + * The default is to expose CSV2 == 1 and CSV3 == 1 if the HW > + * isn't affected. Userspace can override this as long as it > + * doesn't promise the impossible. > + */ > + limit &= ~(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2) | > + ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3)); > + > + if (arm64_get_spectre_v2_state() == SPECTRE_UNAFFECTED) > + limit |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2), 1); > + if (arm64_get_meltdown_state() == SPECTRE_UNAFFECTED) > + limit |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3), 1); > + > + gic = cpuid_feature_extract_unsigned_field(limit, ID_AA64PFR0_GIC_SHIFT); > + if (gic > 1) { > + /* Limit to GICv3.0/4.0 */ > + limit &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_GIC); > + limit |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_GIC), 1); > + } > + id_reg->vcpu_limit_val = limit; > +} > + > +static u64 get_reset_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > + const struct id_reg_info *idr) > +{ > + u64 val = idr->vcpu_limit_val; > + > + if (!vcpu_has_sve(vcpu)) > + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_SVE); > + > + if (!kvm_has_gic3(vcpu->kvm)) > + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_GIC); > + > + return val; > +} > + > +static struct id_reg_info id_aa64pfr0_el1_info = { > + .sys_reg = SYS_ID_AA64PFR0_EL1, > + .ftr_check_types = S_FCT(ID_AA64PFR0_ASIMD_SHIFT, FCT_LOWER_SAFE) | > + S_FCT(ID_AA64PFR0_FP_SHIFT, FCT_LOWER_SAFE), is it needed as it is the default? > + .init = init_id_aa64pfr0_el1_info, > + .validate = validate_id_aa64pfr0_el1, > + .get_reset_val = get_reset_id_aa64pfr0_el1, > +}; > + > /* > * An ID register that needs special handling to control the value for the > * guest must have its own id_reg_info in id_reg_info_table. > @@ -409,7 +495,9 @@ static void id_reg_info_init(struct id_reg_info *id_reg) > * validation, etc.) > */ > #define GET_ID_REG_INFO(id) (id_reg_info_table[IDREG_IDX(id)]) > -static struct id_reg_info *id_reg_info_table[KVM_ARM_ID_REG_MAX_NUM] = {}; > +static struct id_reg_info *id_reg_info_table[KVM_ARM_ID_REG_MAX_NUM] = { > + [IDREG_IDX(SYS_ID_AA64PFR0_EL1)] = &id_aa64pfr0_el1_info, > +}; > > static int validate_id_reg(struct kvm_vcpu *vcpu, > const struct sys_reg_desc *rd, u64 val) > @@ -1239,20 +1327,22 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu, > static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id) > { > u64 val = __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id)); > + u64 lim, gic, gic_lim; > + const struct id_reg_info *id_reg; > > switch (id) { > case SYS_ID_AA64PFR0_EL1: > - if (!vcpu_has_sve(vcpu)) > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_SVE); > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_AMU); > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2); > - val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2), (u64)vcpu->kvm->arch.pfr0_csv2); > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3); > - val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3), (u64)vcpu->kvm->arch.pfr0_csv3); > - if (irqchip_in_kernel(vcpu->kvm) && > - vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) { > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_GIC); > - val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_GIC), 1); > + gic = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_GIC_SHIFT); > + if (kvm_has_gic3(vcpu->kvm) && (gic == 0)) { > + /* > + * This is a case where userspace configured gic3 after > + * the vcpu was created, and then it didn't set > + * ID_AA64PFR0_EL1. > + */ > + id_reg = GET_ID_REG_INFO(id); > + lim = id_reg->vcpu_limit_val; > + gic_lim = cpuid_feature_extract_unsigned_field(lim, ID_AA64PFR0_GIC_SHIFT); > + val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_GIC), gic_lim); > } > break; > case SYS_ID_AA64PFR1_EL1: > @@ -1373,48 +1463,6 @@ static void reset_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd) > __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id)) = val; > } > > -static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > - const struct sys_reg_desc *rd, > - const struct kvm_one_reg *reg, void __user *uaddr) > -{ > - const u64 id = sys_reg_to_index(rd); > - u8 csv2, csv3; > - int err; > - u64 val; > - > - err = reg_from_user(&val, uaddr, id); > - if (err) > - return err; > - > - /* > - * Allow AA64PFR0_EL1.CSV2 to be set from userspace as long as > - * it doesn't promise more than what is actually provided (the > - * guest could otherwise be covered in ectoplasmic residue). > - */ > - csv2 = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_CSV2_SHIFT); > - if (csv2 > 1 || > - (csv2 && arm64_get_spectre_v2_state() != SPECTRE_UNAFFECTED)) > - return -EINVAL; > - > - /* Same thing for CSV3 */ > - csv3 = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_CSV3_SHIFT); > - 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, false); > - val &= ~((0xFUL << ID_AA64PFR0_CSV2_SHIFT) | > - (0xFUL << ID_AA64PFR0_CSV3_SHIFT)); > - if (val) > - return -EINVAL; > - > - vcpu->kvm->arch.pfr0_csv2 = csv2; > - vcpu->kvm->arch.pfr0_csv3 = csv3 ; > - > - return 0; > -} > - > /* cpufeature ID register user accessors */ > static int __get_id_reg(const struct kvm_vcpu *vcpu, > const struct sys_reg_desc *rd, void __user *uaddr, > @@ -1705,8 +1753,7 @@ 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, }, > + ID_SANITISED(ID_AA64PFR0_EL1), > ID_SANITISED(ID_AA64PFR1_EL1), > ID_UNALLOCATED(4,2), > ID_UNALLOCATED(4,3), > Thanks Eric
Hi Eric, On Thu, Nov 25, 2021 at 7:35 AM Eric Auger <eauger@redhat.com> wrote: > > Hi Reiji, > > On 11/17/21 7:43 AM, Reiji Watanabe wrote: > > This patch adds id_reg_info for ID_AA64PFR0_EL1 to make it writable by > > userspace. > > > > The CSV2/CSV3 fields of the register were already writable and values > > that were written for them affected all vCPUs before. Now they only > > affect the vCPU. > > Return an error if userspace tries to set SVE/GIC field of the register > > to a value that conflicts with SVE/GIC configuration for the guest. > > SIMD/FP/SVE fields of the requested value are validated according to > > Arm ARM. > > > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > > --- > > arch/arm64/kvm/sys_regs.c | 159 ++++++++++++++++++++++++-------------- > > 1 file changed, 103 insertions(+), 56 deletions(-) > > > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > > index 1552cd5581b7..35400869067a 100644 > > --- a/arch/arm64/kvm/sys_regs.c > > +++ b/arch/arm64/kvm/sys_regs.c > > @@ -401,6 +401,92 @@ static void id_reg_info_init(struct id_reg_info *id_reg) > > id_reg->init(id_reg); > > } > > > > +#define kvm_has_gic3(kvm) \ > > + (irqchip_in_kernel(kvm) && \ > > + (kvm)->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) > you may move this macro to kvm/arm_vgic.h as this may be used in > vgic/vgic-v3.c too Thank you for the suggestion. I will move that to kvm/arm_vgic.h. > > + > > +static int validate_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > > + const struct id_reg_info *id_reg, u64 val) > > +{ > > + int fp, simd; > > + bool vcpu_has_sve = vcpu_has_sve(vcpu); > > + bool pfr0_has_sve = id_aa64pfr0_sve(val); > > + int gic; > > + > > + simd = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_ASIMD_SHIFT); > > + fp = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_FP_SHIFT); > > + if (simd != fp) > > + return -EINVAL; > > + > > + /* fp must be supported when sve is supported */ > > + if (pfr0_has_sve && (fp < 0)) > > + return -EINVAL; > > + > > + /* Check if there is a conflict with a request via KVM_ARM_VCPU_INIT */ > > + if (vcpu_has_sve ^ pfr0_has_sve) > > + return -EPERM; > > + > > + gic = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_GIC_SHIFT); > > + if ((gic > 0) ^ kvm_has_gic3(vcpu->kvm)) > > + return -EPERM; > > Sometimes from a given architecture version, some lower values are not > allowed. For instance from ARMv8.5 onlt 1 is permitted for CSV3. > Shouldn't we handle that kind of check? As far as I know, there is no way for guests to identify the architecture revision (e.g. v8.1, v8.2, etc). It might be able to indirectly infer the revision though (from features that are available or etc). > > + > > + return 0; > > +} > > + > > +static void init_id_aa64pfr0_el1_info(struct id_reg_info *id_reg) > > +{ > > + u64 limit = id_reg->vcpu_limit_val; > > + unsigned int gic; > > + > > + limit &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_AMU); > > + if (!system_supports_sve()) > > + limit &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_SVE); > > + > > + /* > > + * The default is to expose CSV2 == 1 and CSV3 == 1 if the HW > > + * isn't affected. Userspace can override this as long as it > > + * doesn't promise the impossible. > > + */ > > + limit &= ~(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2) | > > + ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3)); > > + > > + if (arm64_get_spectre_v2_state() == SPECTRE_UNAFFECTED) > > + limit |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2), 1); > > + if (arm64_get_meltdown_state() == SPECTRE_UNAFFECTED) > > + limit |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3), 1); > > + > > + gic = cpuid_feature_extract_unsigned_field(limit, ID_AA64PFR0_GIC_SHIFT); > > + if (gic > 1) { > > + /* Limit to GICv3.0/4.0 */ > > + limit &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_GIC); > > + limit |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_GIC), 1); > > + } > > + id_reg->vcpu_limit_val = limit; > > +} > > + > > +static u64 get_reset_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > > + const struct id_reg_info *idr) > > +{ > > + u64 val = idr->vcpu_limit_val; > > + > > + if (!vcpu_has_sve(vcpu)) > > + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_SVE); > > + > > + if (!kvm_has_gic3(vcpu->kvm)) > > + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_GIC); > > + > > + return val; > > +} > > + > > +static struct id_reg_info id_aa64pfr0_el1_info = { > > + .sys_reg = SYS_ID_AA64PFR0_EL1, > > + .ftr_check_types = S_FCT(ID_AA64PFR0_ASIMD_SHIFT, FCT_LOWER_SAFE) | > > + S_FCT(ID_AA64PFR0_FP_SHIFT, FCT_LOWER_SAFE), > is it needed as it is the default? > > + .ftr_check_types = S_FCT(ID_AA64PFR0_ASIMD_SHIFT, FCT_LOWER_SAFE) | > > + S_FCT(ID_AA64PFR0_FP_SHIFT, FCT_LOWER_SAFE), > is it needed as it is the default? They are needed because they are signed fields (the default is unsigned and FCT_LOWER_SAFE). Having said that, ftr_check_types itself will be gone in the next version (as arm64_ftr_bits will be used instead). Thanks, Reiji
Hi Reiji, On 11/30/21 2:29 AM, Reiji Watanabe wrote: > Hi Eric, > > On Thu, Nov 25, 2021 at 7:35 AM Eric Auger <eauger@redhat.com> wrote: >> >> Hi Reiji, >> >> On 11/17/21 7:43 AM, Reiji Watanabe wrote: >>> This patch adds id_reg_info for ID_AA64PFR0_EL1 to make it writable by >>> userspace. >>> >>> The CSV2/CSV3 fields of the register were already writable and values >>> that were written for them affected all vCPUs before. Now they only >>> affect the vCPU. >>> Return an error if userspace tries to set SVE/GIC field of the register >>> to a value that conflicts with SVE/GIC configuration for the guest. >>> SIMD/FP/SVE fields of the requested value are validated according to >>> Arm ARM. >>> >>> Signed-off-by: Reiji Watanabe <reijiw@google.com> >>> --- >>> arch/arm64/kvm/sys_regs.c | 159 ++++++++++++++++++++++++-------------- >>> 1 file changed, 103 insertions(+), 56 deletions(-) >>> >>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c >>> index 1552cd5581b7..35400869067a 100644 >>> --- a/arch/arm64/kvm/sys_regs.c >>> +++ b/arch/arm64/kvm/sys_regs.c >>> @@ -401,6 +401,92 @@ static void id_reg_info_init(struct id_reg_info *id_reg) >>> id_reg->init(id_reg); >>> } >>> >>> +#define kvm_has_gic3(kvm) \ >>> + (irqchip_in_kernel(kvm) && \ >>> + (kvm)->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) >> you may move this macro to kvm/arm_vgic.h as this may be used in >> vgic/vgic-v3.c too > > Thank you for the suggestion. I will move that to kvm/arm_vgic.h. > > >>> + >>> +static int validate_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, >>> + const struct id_reg_info *id_reg, u64 val) >>> +{ >>> + int fp, simd; >>> + bool vcpu_has_sve = vcpu_has_sve(vcpu); >>> + bool pfr0_has_sve = id_aa64pfr0_sve(val); >>> + int gic; >>> + >>> + simd = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_ASIMD_SHIFT); >>> + fp = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_FP_SHIFT); >>> + if (simd != fp) >>> + return -EINVAL; >>> + >>> + /* fp must be supported when sve is supported */ >>> + if (pfr0_has_sve && (fp < 0)) >>> + return -EINVAL; >>> + >>> + /* Check if there is a conflict with a request via KVM_ARM_VCPU_INIT */ >>> + if (vcpu_has_sve ^ pfr0_has_sve) >>> + return -EPERM; >>> + >>> + gic = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_GIC_SHIFT); >>> + if ((gic > 0) ^ kvm_has_gic3(vcpu->kvm)) >>> + return -EPERM; >> >> Sometimes from a given architecture version, some lower values are not >> allowed. For instance from ARMv8.5 onlt 1 is permitted for CSV3. >> Shouldn't we handle that kind of check? > > As far as I know, there is no way for guests to identify the > architecture revision (e.g. v8.1, v8.2, etc). It might be able > to indirectly infer the revision though (from features that are > available or etc). OK. That sounds weird to me as we do many checks accross different IDREG settings but we may eventually have a wrong "CPU model" exposed by the user space violating those spec revision minima. Shouldn't we introduce some way for the userspace to provide his requirements? via new VCPU targets for instance? Thanks Eric > > >>> + >>> + return 0; >>> +} >>> + >>> +static void init_id_aa64pfr0_el1_info(struct id_reg_info *id_reg) >>> +{ >>> + u64 limit = id_reg->vcpu_limit_val; >>> + unsigned int gic; >>> + >>> + limit &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_AMU); >>> + if (!system_supports_sve()) >>> + limit &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_SVE); >>> + >>> + /* >>> + * The default is to expose CSV2 == 1 and CSV3 == 1 if the HW >>> + * isn't affected. Userspace can override this as long as it >>> + * doesn't promise the impossible. >>> + */ >>> + limit &= ~(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2) | >>> + ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3)); >>> + >>> + if (arm64_get_spectre_v2_state() == SPECTRE_UNAFFECTED) >>> + limit |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2), 1); >>> + if (arm64_get_meltdown_state() == SPECTRE_UNAFFECTED) >>> + limit |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3), 1); >>> + >>> + gic = cpuid_feature_extract_unsigned_field(limit, ID_AA64PFR0_GIC_SHIFT); >>> + if (gic > 1) { >>> + /* Limit to GICv3.0/4.0 */ >>> + limit &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_GIC); >>> + limit |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_GIC), 1); >>> + } >>> + id_reg->vcpu_limit_val = limit; >>> +} >>> + >>> +static u64 get_reset_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, >>> + const struct id_reg_info *idr) >>> +{ >>> + u64 val = idr->vcpu_limit_val; >>> + >>> + if (!vcpu_has_sve(vcpu)) >>> + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_SVE); >>> + >>> + if (!kvm_has_gic3(vcpu->kvm)) >>> + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_GIC); >>> + >>> + return val; >>> +} >>> + >>> +static struct id_reg_info id_aa64pfr0_el1_info = { >>> + .sys_reg = SYS_ID_AA64PFR0_EL1, >>> + .ftr_check_types = S_FCT(ID_AA64PFR0_ASIMD_SHIFT, FCT_LOWER_SAFE) | >>> + S_FCT(ID_AA64PFR0_FP_SHIFT, FCT_LOWER_SAFE), >> is it needed as it is the default? > >>> + .ftr_check_types = S_FCT(ID_AA64PFR0_ASIMD_SHIFT, FCT_LOWER_SAFE) | >>> + S_FCT(ID_AA64PFR0_FP_SHIFT, FCT_LOWER_SAFE), >> is it needed as it is the default? > > They are needed because they are signed fields (the default is unsigned Ah OK, I did not catch it at first glance while looking at the ARM ARM. Thanks Eric > and FCT_LOWER_SAFE). Having said that, ftr_check_types itself will be > gone in the next version (as arm64_ftr_bits will be used instead). > > Thanks, > Reiji >
Hi Eric, On Thu, Dec 2, 2021 at 5:02 AM Eric Auger <eauger@redhat.com> wrote: > > Hi Reiji, > > On 11/30/21 2:29 AM, Reiji Watanabe wrote: > > Hi Eric, > > > > On Thu, Nov 25, 2021 at 7:35 AM Eric Auger <eauger@redhat.com> wrote: > >> > >> Hi Reiji, > >> > >> On 11/17/21 7:43 AM, Reiji Watanabe wrote: > >>> This patch adds id_reg_info for ID_AA64PFR0_EL1 to make it writable by > >>> userspace. > >>> > >>> The CSV2/CSV3 fields of the register were already writable and values > >>> that were written for them affected all vCPUs before. Now they only > >>> affect the vCPU. > >>> Return an error if userspace tries to set SVE/GIC field of the register > >>> to a value that conflicts with SVE/GIC configuration for the guest. > >>> SIMD/FP/SVE fields of the requested value are validated according to > >>> Arm ARM. > >>> > >>> Signed-off-by: Reiji Watanabe <reijiw@google.com> > >>> --- > >>> arch/arm64/kvm/sys_regs.c | 159 ++++++++++++++++++++++++-------------- > >>> 1 file changed, 103 insertions(+), 56 deletions(-) > >>> > >>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > >>> index 1552cd5581b7..35400869067a 100644 > >>> --- a/arch/arm64/kvm/sys_regs.c > >>> +++ b/arch/arm64/kvm/sys_regs.c > >>> @@ -401,6 +401,92 @@ static void id_reg_info_init(struct id_reg_info *id_reg) > >>> id_reg->init(id_reg); > >>> } > >>> > >>> +#define kvm_has_gic3(kvm) \ > >>> + (irqchip_in_kernel(kvm) && \ > >>> + (kvm)->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) > >> you may move this macro to kvm/arm_vgic.h as this may be used in > >> vgic/vgic-v3.c too > > > > Thank you for the suggestion. I will move that to kvm/arm_vgic.h. > > > > > >>> + > >>> +static int validate_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > >>> + const struct id_reg_info *id_reg, u64 val) > >>> +{ > >>> + int fp, simd; > >>> + bool vcpu_has_sve = vcpu_has_sve(vcpu); > >>> + bool pfr0_has_sve = id_aa64pfr0_sve(val); > >>> + int gic; > >>> + > >>> + simd = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_ASIMD_SHIFT); > >>> + fp = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_FP_SHIFT); > >>> + if (simd != fp) > >>> + return -EINVAL; > >>> + > >>> + /* fp must be supported when sve is supported */ > >>> + if (pfr0_has_sve && (fp < 0)) > >>> + return -EINVAL; > >>> + > >>> + /* Check if there is a conflict with a request via KVM_ARM_VCPU_INIT */ > >>> + if (vcpu_has_sve ^ pfr0_has_sve) > >>> + return -EPERM; > >>> + > >>> + gic = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_GIC_SHIFT); > >>> + if ((gic > 0) ^ kvm_has_gic3(vcpu->kvm)) > >>> + return -EPERM; > >> > >> Sometimes from a given architecture version, some lower values are not > >> allowed. For instance from ARMv8.5 onlt 1 is permitted for CSV3. > >> Shouldn't we handle that kind of check? > > > > As far as I know, there is no way for guests to identify the > > architecture revision (e.g. v8.1, v8.2, etc). It might be able > > to indirectly infer the revision though (from features that are > > available or etc). > > OK. That sounds weird to me as we do many checks accross different IDREG > settings but we may eventually have a wrong "CPU model" exposed by the > user space violating those spec revision minima. Shouldn't we introduce > some way for the userspace to provide his requirements? via new VCPU > targets for instance? Thank you for sharing your thoughts and providing the suggestion ! Does the "new vCPU targets" mean Armv8.0, armv8.1, and so on ? The ID registers' consistency checking in the series is to not promise more to userspace than what KVM (on the host) can provide, and to not expose ID register values that are not supported on any ARM v8 architecture for guests (I think those are what the current KVM is trying to assure). I'm not trying to have KVM provide full consistency checking of ID registers to completely prevent userspace's bugs in setting ID registers. I agree that it's quite possible that userspace exposes such wrong CPU models, and KVM's providing more consistency checking would be nicer in general. But should it be KVM's responsibility to completely prevent such ID register issues due to userspace bugs ? Honestly, I'm a bit reluctant to do that so far yet:) If that is something useful that userspace or we (KVM developers) really want or need, or such userspace issue could affect KVM, I would be happy to add such extra consistency checking though. Thanks, Reiji
Hi Reiji, On 12/4/21 8:59 AM, Reiji Watanabe wrote: > Hi Eric, > > On Thu, Dec 2, 2021 at 5:02 AM Eric Auger <eauger@redhat.com> wrote: >> >> Hi Reiji, >> >> On 11/30/21 2:29 AM, Reiji Watanabe wrote: >>> Hi Eric, >>> >>> On Thu, Nov 25, 2021 at 7:35 AM Eric Auger <eauger@redhat.com> wrote: >>>> >>>> Hi Reiji, >>>> >>>> On 11/17/21 7:43 AM, Reiji Watanabe wrote: >>>>> This patch adds id_reg_info for ID_AA64PFR0_EL1 to make it writable by >>>>> userspace. >>>>> >>>>> The CSV2/CSV3 fields of the register were already writable and values >>>>> that were written for them affected all vCPUs before. Now they only >>>>> affect the vCPU. >>>>> Return an error if userspace tries to set SVE/GIC field of the register >>>>> to a value that conflicts with SVE/GIC configuration for the guest. >>>>> SIMD/FP/SVE fields of the requested value are validated according to >>>>> Arm ARM. >>>>> >>>>> Signed-off-by: Reiji Watanabe <reijiw@google.com> >>>>> --- >>>>> arch/arm64/kvm/sys_regs.c | 159 ++++++++++++++++++++++++-------------- >>>>> 1 file changed, 103 insertions(+), 56 deletions(-) >>>>> >>>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c >>>>> index 1552cd5581b7..35400869067a 100644 >>>>> --- a/arch/arm64/kvm/sys_regs.c >>>>> +++ b/arch/arm64/kvm/sys_regs.c >>>>> @@ -401,6 +401,92 @@ static void id_reg_info_init(struct id_reg_info *id_reg) >>>>> id_reg->init(id_reg); >>>>> } >>>>> >>>>> +#define kvm_has_gic3(kvm) \ >>>>> + (irqchip_in_kernel(kvm) && \ >>>>> + (kvm)->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) >>>> you may move this macro to kvm/arm_vgic.h as this may be used in >>>> vgic/vgic-v3.c too >>> >>> Thank you for the suggestion. I will move that to kvm/arm_vgic.h. >>> >>> >>>>> + >>>>> +static int validate_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, >>>>> + const struct id_reg_info *id_reg, u64 val) >>>>> +{ >>>>> + int fp, simd; >>>>> + bool vcpu_has_sve = vcpu_has_sve(vcpu); >>>>> + bool pfr0_has_sve = id_aa64pfr0_sve(val); >>>>> + int gic; >>>>> + >>>>> + simd = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_ASIMD_SHIFT); >>>>> + fp = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_FP_SHIFT); >>>>> + if (simd != fp) >>>>> + return -EINVAL; >>>>> + >>>>> + /* fp must be supported when sve is supported */ >>>>> + if (pfr0_has_sve && (fp < 0)) >>>>> + return -EINVAL; >>>>> + >>>>> + /* Check if there is a conflict with a request via KVM_ARM_VCPU_INIT */ >>>>> + if (vcpu_has_sve ^ pfr0_has_sve) >>>>> + return -EPERM; >>>>> + >>>>> + gic = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_GIC_SHIFT); >>>>> + if ((gic > 0) ^ kvm_has_gic3(vcpu->kvm)) >>>>> + return -EPERM; >>>> >>>> Sometimes from a given architecture version, some lower values are not >>>> allowed. For instance from ARMv8.5 onlt 1 is permitted for CSV3. >>>> Shouldn't we handle that kind of check? >>> >>> As far as I know, there is no way for guests to identify the >>> architecture revision (e.g. v8.1, v8.2, etc). It might be able >>> to indirectly infer the revision though (from features that are >>> available or etc). >> >> OK. That sounds weird to me as we do many checks accross different IDREG >> settings but we may eventually have a wrong "CPU model" exposed by the >> user space violating those spec revision minima. Shouldn't we introduce >> some way for the userspace to provide his requirements? via new VCPU >> targets for instance? > > Thank you for sharing your thoughts and providing the suggestion ! > > Does the "new vCPU targets" mean Armv8.0, armv8.1, and so on ? Yeah my suggestion probably is not a good idea, ie. introducing such VCPU targets. I was simply confused by the fact we introduce in this series quite intricate consistency checks but given the fact we miss the spec rev information we are not exhaustive in terms of checking. So it is sometimes difficult to review against the spec. > > The ID registers' consistency checking in the series is to not > promise more to userspace than what KVM (on the host) can provide, > and to not expose ID register values that are not supported on > any ARM v8 architecture for guests (I think those are what the > current KVM is trying to assure). I'm not trying to have KVM > provide full consistency checking of ID registers to completely > prevent userspace's bugs in setting ID registers. > > I agree that it's quite possible that userspace exposes such wrong > CPU models, and KVM's providing more consistency checking would be > nicer in general. But should it be KVM's responsibility to completely > prevent such ID register issues due to userspace bugs ? > > Honestly, I'm a bit reluctant to do that so far yet:) understood. I will look at the spec in more details on my next review cycle. Looking forward to reviewing the next version ;-) Thanks Eric > If that is something useful that userspace or we (KVM developers) > really want or need, or such userspace issue could affect KVM, > I would be happy to add such extra consistency checking though. > > Thanks, > Reiji >
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 1552cd5581b7..35400869067a 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -401,6 +401,92 @@ static void id_reg_info_init(struct id_reg_info *id_reg) id_reg->init(id_reg); } +#define kvm_has_gic3(kvm) \ + (irqchip_in_kernel(kvm) && \ + (kvm)->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) + +static int validate_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, + const struct id_reg_info *id_reg, u64 val) +{ + int fp, simd; + bool vcpu_has_sve = vcpu_has_sve(vcpu); + bool pfr0_has_sve = id_aa64pfr0_sve(val); + int gic; + + simd = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_ASIMD_SHIFT); + fp = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_FP_SHIFT); + if (simd != fp) + return -EINVAL; + + /* fp must be supported when sve is supported */ + if (pfr0_has_sve && (fp < 0)) + return -EINVAL; + + /* Check if there is a conflict with a request via KVM_ARM_VCPU_INIT */ + if (vcpu_has_sve ^ pfr0_has_sve) + return -EPERM; + + gic = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_GIC_SHIFT); + if ((gic > 0) ^ kvm_has_gic3(vcpu->kvm)) + return -EPERM; + + return 0; +} + +static void init_id_aa64pfr0_el1_info(struct id_reg_info *id_reg) +{ + u64 limit = id_reg->vcpu_limit_val; + unsigned int gic; + + limit &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_AMU); + if (!system_supports_sve()) + limit &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_SVE); + + /* + * The default is to expose CSV2 == 1 and CSV3 == 1 if the HW + * isn't affected. Userspace can override this as long as it + * doesn't promise the impossible. + */ + limit &= ~(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2) | + ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3)); + + if (arm64_get_spectre_v2_state() == SPECTRE_UNAFFECTED) + limit |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2), 1); + if (arm64_get_meltdown_state() == SPECTRE_UNAFFECTED) + limit |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3), 1); + + gic = cpuid_feature_extract_unsigned_field(limit, ID_AA64PFR0_GIC_SHIFT); + if (gic > 1) { + /* Limit to GICv3.0/4.0 */ + limit &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_GIC); + limit |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_GIC), 1); + } + id_reg->vcpu_limit_val = limit; +} + +static u64 get_reset_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, + const struct id_reg_info *idr) +{ + u64 val = idr->vcpu_limit_val; + + if (!vcpu_has_sve(vcpu)) + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_SVE); + + if (!kvm_has_gic3(vcpu->kvm)) + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_GIC); + + return val; +} + +static struct id_reg_info id_aa64pfr0_el1_info = { + .sys_reg = SYS_ID_AA64PFR0_EL1, + .ftr_check_types = S_FCT(ID_AA64PFR0_ASIMD_SHIFT, FCT_LOWER_SAFE) | + S_FCT(ID_AA64PFR0_FP_SHIFT, FCT_LOWER_SAFE), + .init = init_id_aa64pfr0_el1_info, + .validate = validate_id_aa64pfr0_el1, + .get_reset_val = get_reset_id_aa64pfr0_el1, +}; + /* * An ID register that needs special handling to control the value for the * guest must have its own id_reg_info in id_reg_info_table. @@ -409,7 +495,9 @@ static void id_reg_info_init(struct id_reg_info *id_reg) * validation, etc.) */ #define GET_ID_REG_INFO(id) (id_reg_info_table[IDREG_IDX(id)]) -static struct id_reg_info *id_reg_info_table[KVM_ARM_ID_REG_MAX_NUM] = {}; +static struct id_reg_info *id_reg_info_table[KVM_ARM_ID_REG_MAX_NUM] = { + [IDREG_IDX(SYS_ID_AA64PFR0_EL1)] = &id_aa64pfr0_el1_info, +}; static int validate_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, u64 val) @@ -1239,20 +1327,22 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu, static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id) { u64 val = __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id)); + u64 lim, gic, gic_lim; + const struct id_reg_info *id_reg; switch (id) { case SYS_ID_AA64PFR0_EL1: - if (!vcpu_has_sve(vcpu)) - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_SVE); - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_AMU); - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2); - val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2), (u64)vcpu->kvm->arch.pfr0_csv2); - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3); - val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3), (u64)vcpu->kvm->arch.pfr0_csv3); - if (irqchip_in_kernel(vcpu->kvm) && - vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) { - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_GIC); - val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_GIC), 1); + gic = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_GIC_SHIFT); + if (kvm_has_gic3(vcpu->kvm) && (gic == 0)) { + /* + * This is a case where userspace configured gic3 after + * the vcpu was created, and then it didn't set + * ID_AA64PFR0_EL1. + */ + id_reg = GET_ID_REG_INFO(id); + lim = id_reg->vcpu_limit_val; + gic_lim = cpuid_feature_extract_unsigned_field(lim, ID_AA64PFR0_GIC_SHIFT); + val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_GIC), gic_lim); } break; case SYS_ID_AA64PFR1_EL1: @@ -1373,48 +1463,6 @@ static void reset_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd) __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id)) = val; } -static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, - const struct sys_reg_desc *rd, - const struct kvm_one_reg *reg, void __user *uaddr) -{ - const u64 id = sys_reg_to_index(rd); - u8 csv2, csv3; - int err; - u64 val; - - err = reg_from_user(&val, uaddr, id); - if (err) - return err; - - /* - * Allow AA64PFR0_EL1.CSV2 to be set from userspace as long as - * it doesn't promise more than what is actually provided (the - * guest could otherwise be covered in ectoplasmic residue). - */ - csv2 = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_CSV2_SHIFT); - if (csv2 > 1 || - (csv2 && arm64_get_spectre_v2_state() != SPECTRE_UNAFFECTED)) - return -EINVAL; - - /* Same thing for CSV3 */ - csv3 = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_CSV3_SHIFT); - 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, false); - val &= ~((0xFUL << ID_AA64PFR0_CSV2_SHIFT) | - (0xFUL << ID_AA64PFR0_CSV3_SHIFT)); - if (val) - return -EINVAL; - - vcpu->kvm->arch.pfr0_csv2 = csv2; - vcpu->kvm->arch.pfr0_csv3 = csv3 ; - - return 0; -} - /* cpufeature ID register user accessors */ static int __get_id_reg(const struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, void __user *uaddr, @@ -1705,8 +1753,7 @@ 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, }, + ID_SANITISED(ID_AA64PFR0_EL1), ID_SANITISED(ID_AA64PFR1_EL1), ID_UNALLOCATED(4,2), ID_UNALLOCATED(4,3),
This patch adds id_reg_info for ID_AA64PFR0_EL1 to make it writable by userspace. The CSV2/CSV3 fields of the register were already writable and values that were written for them affected all vCPUs before. Now they only affect the vCPU. Return an error if userspace tries to set SVE/GIC field of the register to a value that conflicts with SVE/GIC configuration for the guest. SIMD/FP/SVE fields of the requested value are validated according to Arm ARM. Signed-off-by: Reiji Watanabe <reijiw@google.com> --- arch/arm64/kvm/sys_regs.c | 159 ++++++++++++++++++++++++-------------- 1 file changed, 103 insertions(+), 56 deletions(-)