On Sun, Nov 21, 2021 at 10:46 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> On Wed, 17 Nov 2021 06:43:51 +0000,
> Reiji Watanabe <reijiw@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
