| Message ID | 20211117064359.2362060-22-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:51 +0000, Reiji Watanabe <reijiw@google.com> wrote: > > When a CPU feature that is supported on the host is not exposed to > its guest, emulating a real CPU's behavior (by trapping or disabling > guest's using the feature) is generally a desirable behavior (when > it's possible without any or little side effect). > > Introduce feature_config_ctrl structure, which manages feature > information to program configuration register to trap or disable > the feature when the feature is not exposed to the guest, and > functions that uses the structure to activate trapping the feature. > > At present, no feature has feature_config_ctrl yet and the following > patches will add the feature_config_ctrl for several features. > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > --- > arch/arm64/kvm/sys_regs.c | 121 +++++++++++++++++++++++++++++++++++++- > 1 file changed, 120 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 2f96103fc0d2..501de08dacb7 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -376,8 +376,38 @@ static int arm64_check_features(u64 check_types, u64 val, u64 lim) > (cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR1_GPI_SHIFT) >= \ > ID_AA64ISAR1_GPI_IMP_DEF) > > +enum vcpu_config_reg { > + VCPU_HCR_EL2 = 1, > + VCPU_MDCR_EL2, > + VCPU_CPTR_EL2, > +}; > + > +/* > + * Feature information to program configuration register to trap or disable > + * guest's using a feature when the feature is not exposed to the guest. > + */ > +struct feature_config_ctrl { > + /* ID register/field for the feature */ > + u32 ftr_reg; /* ID register */ > + bool ftr_signed; /* Is the feature field signed ? */ > + u8 ftr_shift; /* Field of ID register for the feature */ > + s8 ftr_min; /* Min value that indicate the feature */ > + > + /* > + * Function to check trapping is needed. This is used when the above > + * fields are not enough to determine if trapping is needed. > + */ > + bool (*ftr_need_trap)(struct kvm_vcpu *vcpu); > + > + /* Configuration register information to trap the feature. */ > + enum vcpu_config_reg cfg_reg; /* Configuration register */ > + u64 cfg_mask; /* Field of the configuration register */ > + u64 cfg_val; /* Value that are set for the field */ Although this probably works for the use cases you have in mind, some trap bits are actually working the other way around (clear to trap). So you probably want to turn this into cfg_set and add a cfg_clear for a good measure, dropping cfg_mask in the process. That being said, the current trend is to move to FGT, meaning that a single register is unlikely to cut it in the long run. I'd rather you simply have a configuration function here (and the helper you already have is probably enough). > +}; > + > struct id_reg_info { > u32 sys_reg; /* Register ID */ > + u64 sys_val; /* Sanitized system value */ > > /* > * Limit value of the register for a vcpu. The value is the sanitized > @@ -410,11 +440,15 @@ struct id_reg_info { > /* Return the reset value of the register for the vCPU */ > u64 (*get_reset_val)(struct kvm_vcpu *vcpu, > const struct id_reg_info *id_reg); > + > + /* Information to trap features that are disabled for the guest */ > + const struct feature_config_ctrl *(*trap_features)[]; > }; > > static void id_reg_info_init(struct id_reg_info *id_reg) > { > - id_reg->vcpu_limit_val = read_sanitised_ftr_reg(id_reg->sys_reg); > + id_reg->sys_val = read_sanitised_ftr_reg(id_reg->sys_reg); > + id_reg->vcpu_limit_val = id_reg->sys_val; > if (id_reg->init) > id_reg->init(id_reg); > } > @@ -952,6 +986,47 @@ static int validate_id_reg(struct kvm_vcpu *vcpu, > return err; > } > > +static void feature_trap_activate(struct kvm_vcpu *vcpu, > + const struct feature_config_ctrl *config) > +{ > + u64 *reg_ptr, reg_val; > + > + switch (config->cfg_reg) { > + case VCPU_HCR_EL2: > + reg_ptr = &vcpu->arch.hcr_el2; > + break; > + case VCPU_MDCR_EL2: > + reg_ptr = &vcpu->arch.mdcr_el2; > + break; > + case VCPU_CPTR_EL2: > + reg_ptr = &vcpu->arch.cptr_el2; > + break; > + } > + > + /* Update cfg_mask fields with cfg_val */ > + reg_val = (*reg_ptr & ~config->cfg_mask); > + reg_val |= config->cfg_val; > + *reg_ptr = reg_val; > +} > + > +static inline bool feature_avail(const struct feature_config_ctrl *ctrl, > + u64 id_val) > +{ > + int field_val = cpuid_feature_extract_field(id_val, > + ctrl->ftr_shift, ctrl->ftr_signed); > + > + return (field_val >= ctrl->ftr_min); > +} > + > +static inline bool vcpu_feature_is_available(struct kvm_vcpu *vcpu, > + const struct feature_config_ctrl *ctrl) > +{ > + u64 val; > + > + val = __read_id_reg(vcpu, ctrl->ftr_reg); > + return feature_avail(ctrl, val); > +} > + > /* > * ARMv8.1 mandates at least a trivial LORegion implementation, where all the > * RW registers are RES0 (which we can implement as RAZ/WI). On an ARMv8.0 > @@ -1831,6 +1906,42 @@ static int reg_from_user(u64 *val, const void __user *uaddr, u64 id); > static int reg_to_user(void __user *uaddr, const u64 *val, u64 id); > static u64 sys_reg_to_index(const struct sys_reg_desc *reg); > > +static void id_reg_features_trap_activate(struct kvm_vcpu *vcpu, > + const struct id_reg_info *id_reg) > +{ > + u64 val; > + int i = 0; > + const struct feature_config_ctrl **ctrlp_array, *ctrl; > + > + if (!id_reg || !id_reg->trap_features) > + /* No information to trap a feature */ > + return; > + > + val = __read_id_reg(vcpu, id_reg->sys_reg); > + if (val == id_reg->sys_val) > + /* No feature needs to be trapped (no feature is disabled). */ > + return; > + > + ctrlp_array = *id_reg->trap_features; > + while ((ctrl = ctrlp_array[i++]) != NULL) { > + if (ctrl->ftr_need_trap && ctrl->ftr_need_trap(vcpu)) { > + feature_trap_activate(vcpu, ctrl); > + continue; > + } > + > + if (!feature_avail(ctrl, id_reg->sys_val)) > + /* The feature is not supported on the host. */ > + continue; > + > + if (feature_avail(ctrl, val)) > + /* The feature is enabled for the guest. */ > + continue; > + > + /* The feature is supported but disabled. */ > + feature_trap_activate(vcpu, ctrl); > + } > +} > + > /* Visibility overrides for SVE-specific control registers */ > static unsigned int sve_visibility(const struct kvm_vcpu *vcpu, > const struct sys_reg_desc *rd) > @@ -3457,6 +3568,14 @@ int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices) > return write_demux_regids(uindices); > } > > +void kvm_vcpu_init_traps(struct kvm_vcpu *vcpu) Who is going to call this? At which point? Please document the use constraints on this. > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(id_reg_info_table); i++) > + id_reg_features_trap_activate(vcpu, id_reg_info_table[i]); > +} > + > /* ID register's fractional field information with its feature field. */ > struct feature_frac { > u32 id; > -- > 2.34.0.rc1.387.gb447b232ab-goog > > Thanks, M.
On Sun, Nov 21, 2021 at 10:46 AM Marc Zyngier <maz@kernel.org> wrote: > > On Wed, 17 Nov 2021 06:43:51 +0000, > Reiji Watanabe <reijiw@google.com> wrote: > > > > When a CPU feature that is supported on the host is not exposed to > > its guest, emulating a real CPU's behavior (by trapping or disabling > > guest's using the feature) is generally a desirable behavior (when > > it's possible without any or little side effect). > > > > Introduce feature_config_ctrl structure, which manages feature > > information to program configuration register to trap or disable > > the feature when the feature is not exposed to the guest, and > > functions that uses the structure to activate trapping the feature. > > > > At present, no feature has feature_config_ctrl yet and the following > > patches will add the feature_config_ctrl for several features. > > > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > > --- > > arch/arm64/kvm/sys_regs.c | 121 +++++++++++++++++++++++++++++++++++++- > > 1 file changed, 120 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > > index 2f96103fc0d2..501de08dacb7 100644 > > --- a/arch/arm64/kvm/sys_regs.c > > +++ b/arch/arm64/kvm/sys_regs.c > > @@ -376,8 +376,38 @@ static int arm64_check_features(u64 check_types, u64 val, u64 lim) > > (cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR1_GPI_SHIFT) >= \ > > ID_AA64ISAR1_GPI_IMP_DEF) > > > > +enum vcpu_config_reg { > > + VCPU_HCR_EL2 = 1, > > + VCPU_MDCR_EL2, > > + VCPU_CPTR_EL2, > > +}; > > + > > +/* > > + * Feature information to program configuration register to trap or disable > > + * guest's using a feature when the feature is not exposed to the guest. > > + */ > > +struct feature_config_ctrl { > > + /* ID register/field for the feature */ > > + u32 ftr_reg; /* ID register */ > > + bool ftr_signed; /* Is the feature field signed ? */ > > + u8 ftr_shift; /* Field of ID register for the feature */ > > + s8 ftr_min; /* Min value that indicate the feature */ > > + > > + /* > > + * Function to check trapping is needed. This is used when the above > > + * fields are not enough to determine if trapping is needed. > > + */ > > + bool (*ftr_need_trap)(struct kvm_vcpu *vcpu); > > + > > + /* Configuration register information to trap the feature. */ > > + enum vcpu_config_reg cfg_reg; /* Configuration register */ > > + u64 cfg_mask; /* Field of the configuration register */ > > + u64 cfg_val; /* Value that are set for the field */ > > Although this probably works for the use cases you have in mind, some > trap bits are actually working the other way around (clear to trap). > So you probably want to turn this into cfg_set and add a cfg_clear for > a good measure, dropping cfg_mask in the process. Although I was aware of both of the cases (cfg_clear is implicitly derived from cfg_mask ~ cfg_set), I agree that dropping cfg_mask and adding cfg_clear would be more explicit and nicer. > That being said, the current trend is to move to FGT, meaning that a > single register is unlikely to cut it in the long run. I'd rather you > simply have a configuration function here (and the helper you already > have is probably enough). Thank you for the nice suggestion ! That's a good point (I didn't pay attention to FGT yet). I will look into having a configuration function here. > > +}; > > + > > struct id_reg_info { > > u32 sys_reg; /* Register ID */ > > + u64 sys_val; /* Sanitized system value */ > > > > /* > > * Limit value of the register for a vcpu. The value is the sanitized > > @@ -410,11 +440,15 @@ struct id_reg_info { > > /* Return the reset value of the register for the vCPU */ > > u64 (*get_reset_val)(struct kvm_vcpu *vcpu, > > const struct id_reg_info *id_reg); > > + > > + /* Information to trap features that are disabled for the guest */ > > + const struct feature_config_ctrl *(*trap_features)[]; > > }; > > > > static void id_reg_info_init(struct id_reg_info *id_reg) > > { > > - id_reg->vcpu_limit_val = read_sanitised_ftr_reg(id_reg->sys_reg); > > + id_reg->sys_val = read_sanitised_ftr_reg(id_reg->sys_reg); > > + id_reg->vcpu_limit_val = id_reg->sys_val; > > if (id_reg->init) > > id_reg->init(id_reg); > > } > > @@ -952,6 +986,47 @@ static int validate_id_reg(struct kvm_vcpu *vcpu, > > return err; > > } > > > > +static void feature_trap_activate(struct kvm_vcpu *vcpu, > > + const struct feature_config_ctrl *config) > > +{ > > + u64 *reg_ptr, reg_val; > > + > > + switch (config->cfg_reg) { > > + case VCPU_HCR_EL2: > > + reg_ptr = &vcpu->arch.hcr_el2; > > + break; > > + case VCPU_MDCR_EL2: > > + reg_ptr = &vcpu->arch.mdcr_el2; > > + break; > > + case VCPU_CPTR_EL2: > > + reg_ptr = &vcpu->arch.cptr_el2; > > + break; > > + } > > + > > + /* Update cfg_mask fields with cfg_val */ > > + reg_val = (*reg_ptr & ~config->cfg_mask); > > + reg_val |= config->cfg_val; > > + *reg_ptr = reg_val; > > +} > > + > > +static inline bool feature_avail(const struct feature_config_ctrl *ctrl, > > + u64 id_val) > > +{ > > + int field_val = cpuid_feature_extract_field(id_val, > > + ctrl->ftr_shift, ctrl->ftr_signed); > > + > > + return (field_val >= ctrl->ftr_min); > > +} > > + > > +static inline bool vcpu_feature_is_available(struct kvm_vcpu *vcpu, > > + const struct feature_config_ctrl *ctrl) > > +{ > > + u64 val; > > + > > + val = __read_id_reg(vcpu, ctrl->ftr_reg); > > + return feature_avail(ctrl, val); > > +} > > + > > /* > > * ARMv8.1 mandates at least a trivial LORegion implementation, where all the > > * RW registers are RES0 (which we can implement as RAZ/WI). On an ARMv8.0 > > @@ -1831,6 +1906,42 @@ static int reg_from_user(u64 *val, const void __user *uaddr, u64 id); > > static int reg_to_user(void __user *uaddr, const u64 *val, u64 id); > > static u64 sys_reg_to_index(const struct sys_reg_desc *reg); > > > > +static void id_reg_features_trap_activate(struct kvm_vcpu *vcpu, > > + const struct id_reg_info *id_reg) > > +{ > > + u64 val; > > + int i = 0; > > + const struct feature_config_ctrl **ctrlp_array, *ctrl; > > + > > + if (!id_reg || !id_reg->trap_features) > > + /* No information to trap a feature */ > > + return; > > + > > + val = __read_id_reg(vcpu, id_reg->sys_reg); > > + if (val == id_reg->sys_val) > > + /* No feature needs to be trapped (no feature is disabled). */ > > + return; > > + > > + ctrlp_array = *id_reg->trap_features; > > + while ((ctrl = ctrlp_array[i++]) != NULL) { > > + if (ctrl->ftr_need_trap && ctrl->ftr_need_trap(vcpu)) { > > + feature_trap_activate(vcpu, ctrl); > > + continue; > > + } > > + > > + if (!feature_avail(ctrl, id_reg->sys_val)) > > + /* The feature is not supported on the host. */ > > + continue; > > + > > + if (feature_avail(ctrl, val)) > > + /* The feature is enabled for the guest. */ > > + continue; > > + > > + /* The feature is supported but disabled. */ > > + feature_trap_activate(vcpu, ctrl); > > + } > > +} > > + > > /* Visibility overrides for SVE-specific control registers */ > > static unsigned int sve_visibility(const struct kvm_vcpu *vcpu, > > const struct sys_reg_desc *rd) > > @@ -3457,6 +3568,14 @@ int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices) > > return write_demux_regids(uindices); > > } > > > > +void kvm_vcpu_init_traps(struct kvm_vcpu *vcpu) > > Who is going to call this? At which point? Please document the use > constraints on this. kvm_vcpu_first_run_init() is going to call it (for non-pKVM). I will document the use constraints on that. Thanks, Reiji
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 2f96103fc0d2..501de08dacb7 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -376,8 +376,38 @@ static int arm64_check_features(u64 check_types, u64 val, u64 lim) (cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR1_GPI_SHIFT) >= \ ID_AA64ISAR1_GPI_IMP_DEF) +enum vcpu_config_reg { + VCPU_HCR_EL2 = 1, + VCPU_MDCR_EL2, + VCPU_CPTR_EL2, +}; + +/* + * Feature information to program configuration register to trap or disable + * guest's using a feature when the feature is not exposed to the guest. + */ +struct feature_config_ctrl { + /* ID register/field for the feature */ + u32 ftr_reg; /* ID register */ + bool ftr_signed; /* Is the feature field signed ? */ + u8 ftr_shift; /* Field of ID register for the feature */ + s8 ftr_min; /* Min value that indicate the feature */ + + /* + * Function to check trapping is needed. This is used when the above + * fields are not enough to determine if trapping is needed. + */ + bool (*ftr_need_trap)(struct kvm_vcpu *vcpu); + + /* Configuration register information to trap the feature. */ + enum vcpu_config_reg cfg_reg; /* Configuration register */ + u64 cfg_mask; /* Field of the configuration register */ + u64 cfg_val; /* Value that are set for the field */ +}; + struct id_reg_info { u32 sys_reg; /* Register ID */ + u64 sys_val; /* Sanitized system value */ /* * Limit value of the register for a vcpu. The value is the sanitized @@ -410,11 +440,15 @@ struct id_reg_info { /* Return the reset value of the register for the vCPU */ u64 (*get_reset_val)(struct kvm_vcpu *vcpu, const struct id_reg_info *id_reg); + + /* Information to trap features that are disabled for the guest */ + const struct feature_config_ctrl *(*trap_features)[]; }; static void id_reg_info_init(struct id_reg_info *id_reg) { - id_reg->vcpu_limit_val = read_sanitised_ftr_reg(id_reg->sys_reg); + id_reg->sys_val = read_sanitised_ftr_reg(id_reg->sys_reg); + id_reg->vcpu_limit_val = id_reg->sys_val; if (id_reg->init) id_reg->init(id_reg); } @@ -952,6 +986,47 @@ static int validate_id_reg(struct kvm_vcpu *vcpu, return err; } +static void feature_trap_activate(struct kvm_vcpu *vcpu, + const struct feature_config_ctrl *config) +{ + u64 *reg_ptr, reg_val; + + switch (config->cfg_reg) { + case VCPU_HCR_EL2: + reg_ptr = &vcpu->arch.hcr_el2; + break; + case VCPU_MDCR_EL2: + reg_ptr = &vcpu->arch.mdcr_el2; + break; + case VCPU_CPTR_EL2: + reg_ptr = &vcpu->arch.cptr_el2; + break; + } + + /* Update cfg_mask fields with cfg_val */ + reg_val = (*reg_ptr & ~config->cfg_mask); + reg_val |= config->cfg_val; + *reg_ptr = reg_val; +} + +static inline bool feature_avail(const struct feature_config_ctrl *ctrl, + u64 id_val) +{ + int field_val = cpuid_feature_extract_field(id_val, + ctrl->ftr_shift, ctrl->ftr_signed); + + return (field_val >= ctrl->ftr_min); +} + +static inline bool vcpu_feature_is_available(struct kvm_vcpu *vcpu, + const struct feature_config_ctrl *ctrl) +{ + u64 val; + + val = __read_id_reg(vcpu, ctrl->ftr_reg); + return feature_avail(ctrl, val); +} + /* * ARMv8.1 mandates at least a trivial LORegion implementation, where all the * RW registers are RES0 (which we can implement as RAZ/WI). On an ARMv8.0 @@ -1831,6 +1906,42 @@ static int reg_from_user(u64 *val, const void __user *uaddr, u64 id); static int reg_to_user(void __user *uaddr, const u64 *val, u64 id); static u64 sys_reg_to_index(const struct sys_reg_desc *reg); +static void id_reg_features_trap_activate(struct kvm_vcpu *vcpu, + const struct id_reg_info *id_reg) +{ + u64 val; + int i = 0; + const struct feature_config_ctrl **ctrlp_array, *ctrl; + + if (!id_reg || !id_reg->trap_features) + /* No information to trap a feature */ + return; + + val = __read_id_reg(vcpu, id_reg->sys_reg); + if (val == id_reg->sys_val) + /* No feature needs to be trapped (no feature is disabled). */ + return; + + ctrlp_array = *id_reg->trap_features; + while ((ctrl = ctrlp_array[i++]) != NULL) { + if (ctrl->ftr_need_trap && ctrl->ftr_need_trap(vcpu)) { + feature_trap_activate(vcpu, ctrl); + continue; + } + + if (!feature_avail(ctrl, id_reg->sys_val)) + /* The feature is not supported on the host. */ + continue; + + if (feature_avail(ctrl, val)) + /* The feature is enabled for the guest. */ + continue; + + /* The feature is supported but disabled. */ + feature_trap_activate(vcpu, ctrl); + } +} + /* Visibility overrides for SVE-specific control registers */ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd) @@ -3457,6 +3568,14 @@ int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices) return write_demux_regids(uindices); } +void kvm_vcpu_init_traps(struct kvm_vcpu *vcpu) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(id_reg_info_table); i++) + id_reg_features_trap_activate(vcpu, id_reg_info_table[i]); +} + /* ID register's fractional field information with its feature field. */ struct feature_frac { u32 id;
When a CPU feature that is supported on the host is not exposed to its guest, emulating a real CPU's behavior (by trapping or disabling guest's using the feature) is generally a desirable behavior (when it's possible without any or little side effect). Introduce feature_config_ctrl structure, which manages feature information to program configuration register to trap or disable the feature when the feature is not exposed to the guest, and functions that uses the structure to activate trapping the feature. At present, no feature has feature_config_ctrl yet and the following patches will add the feature_config_ctrl for several features. Signed-off-by: Reiji Watanabe <reijiw@google.com> --- arch/arm64/kvm/sys_regs.c | 121 +++++++++++++++++++++++++++++++++++++- 1 file changed, 120 insertions(+), 1 deletion(-)