[RFC PATCH v4 14/26] KVM: arm64: Add consistency checking for frac fields of ID registers

[Reiji Watanabe]

Hi Fuad,

On Mon, Jan 24, 2022 at 9:01 AM Fuad Tabba <tabba at google.com> wrote:
>
> Hi Reiji,
>
> On Thu, Jan 6, 2022 at 4:29 AM Reiji Watanabe <reijiw at google.com> wrote:
> >
> > Feature fractional field of an ID register cannot be simply validated
> > at KVM_SET_ONE_REG because its validity depends on its (main) feature
> > field value, which could be in a different ID register (and might be
> > set later).
> > Validate fractional fields at the first KVM_RUN instead.
> >
> > Signed-off-by: Reiji Watanabe <reijiw at google.com>
> > ---
> >  arch/arm64/include/asm/kvm_host.h |   1 +
> >  arch/arm64/kvm/arm.c              |   3 +
> >  arch/arm64/kvm/sys_regs.c         | 116 +++++++++++++++++++++++++++++-
> >  3 files changed, 117 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index 4509f9e7472d..7b3f86bd6a6b 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -750,6 +750,7 @@ long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
> >
> >  void set_default_id_regs(struct kvm *kvm);
> >  int kvm_set_id_reg_feature(struct kvm *kvm, u32 id, u8 field_shift, u8 fval);
> > +int kvm_id_regs_consistency_check(const struct kvm_vcpu *vcpu);
> >
> >  /* Guest/host FPSIMD coordination helpers */
> >  int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu);
> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > index 5f497a0af254..16fc2ce32069 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -596,6 +596,9 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
> >         if (!kvm_arm_vcpu_is_finalized(vcpu))
> >                 return -EPERM;
> >
> > +       if (!kvm_vm_is_protected(kvm) && kvm_id_regs_consistency_check(vcpu))
> > +               return -EPERM;
> > +
> >         vcpu->arch.has_run_once = true;
> >
> >         kvm_arm_vcpu_init_debug(vcpu);
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index ddbeefc3881c..6adb7b04620c 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -756,9 +756,6 @@ static struct id_reg_info id_aa64pfr0_el1_info = {
> >
> >  static struct id_reg_info id_aa64pfr1_el1_info = {
> >         .sys_reg = SYS_ID_AA64PFR1_EL1,
> > -       .ignore_mask = ARM64_FEATURE_MASK(ID_AA64PFR1_RASFRAC) |
> > -                      ARM64_FEATURE_MASK(ID_AA64PFR1_MPAMFRAC) |
> > -                      ARM64_FEATURE_MASK(ID_AA64PFR1_CSV2FRAC),
> >         .init = init_id_aa64pfr1_el1_info,
> >         .validate = validate_id_aa64pfr1_el1,
> >         .vcpu_mask = vcpu_mask_id_aa64pfr1_el1,
> > @@ -3434,10 +3431,109 @@ int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
> >         return write_demux_regids(uindices);
> >  }
> >
> > +/* ID register's fractional field information with its feature field. */
> > +struct feature_frac {
> > +       u32     id;
> > +       u32     shift;
> > +       u32     frac_id;
> > +       u32     frac_shift;
> > +       u8      frac_ftr_check;
> > +};
>
> frac_ftr_check doesn't seem to be used. Also, it would be easier to
> read if the ordering of the fields match the ordering you initialize
> them below.

Thank you for catching this.
I will remove frac_ftr_check and change the ordering.

>
> > +
> > +static struct feature_frac feature_frac_table[] = {
> > +       {
> > +               .frac_id = SYS_ID_AA64PFR1_EL1,
> > +               .frac_shift = ID_AA64PFR1_RASFRAC_SHIFT,
> > +               .id = SYS_ID_AA64PFR0_EL1,
> > +               .shift = ID_AA64PFR0_RAS_SHIFT,
> > +       },
> > +       {
> > +               .frac_id = SYS_ID_AA64PFR1_EL1,
> > +               .frac_shift = ID_AA64PFR1_MPAMFRAC_SHIFT,
> > +               .id = SYS_ID_AA64PFR0_EL1,
> > +               .shift = ID_AA64PFR0_MPAM_SHIFT,
> > +       },
> > +       {
> > +               .frac_id = SYS_ID_AA64PFR1_EL1,
> > +               .frac_shift = ID_AA64PFR1_CSV2FRAC_SHIFT,
> > +               .id = SYS_ID_AA64PFR0_EL1,
> > +               .shift = ID_AA64PFR0_CSV2_SHIFT,
> > +       },
> > +};
> > +
> > +/*
> > + * Return non-zero if the feature/fractional fields pair are not
> > + * supported. Return zero otherwise.
> > + * This function validates only the fractional feature field,
> > + * and relies on the fact the feature field is validated before
> > + * through arm64_check_features.
> > + */
> > +static int vcpu_id_reg_feature_frac_check(const struct kvm_vcpu *vcpu,
> > +                                         const struct feature_frac *ftr_frac)
> > +{
> > +       const struct id_reg_info *id_reg;
> > +       u32 id;
> > +       u64 val, lim, mask;
> > +
> > +       /* Check if the feature field value is same as the limit */
> > +       id = ftr_frac->id;
> > +       id_reg = GET_ID_REG_INFO(id);
> > +
> > +       mask = (u64)ARM64_FEATURE_FIELD_MASK << ftr_frac->shift;
> > +       val = __read_id_reg(vcpu, id) & mask;
> > +       lim = id_reg ? id_reg->vcpu_limit_val : read_sanitised_ftr_reg(id);
> > +       lim &= mask;
> > +
> > +       if (val != lim)
> > +               /*
> > +                * The feature level is lower than the limit.
> > +                * Any fractional version should be fine.
> > +                */
> > +               return 0;
> > +
> > +       /* Check the fractional feature field */
> > +       id = ftr_frac->frac_id;
> > +       id_reg = GET_ID_REG_INFO(id);
> > +
> > +       mask = (u64)ARM64_FEATURE_FIELD_MASK << ftr_frac->frac_shift;
> > +       val = __read_id_reg(vcpu, id) & mask;
> > +       lim = id_reg ? id_reg->vcpu_limit_val : read_sanitised_ftr_reg(id);
> > +       lim &= mask;
> > +
> > +       if (val == lim)
> > +               /*
> > +                * Both the feature and fractional fields are the same
> > +                * as limit.
> > +                */
> > +               return 0;
> > +
> > +       return arm64_check_features(id, val, lim);
> > +}
> > +
> > +int kvm_id_regs_consistency_check(const struct kvm_vcpu *vcpu)
>
> Nit: considering that this is only checking the fractional fields,
> should the function name reflect that?

Thank you for the suggestion.
I will change the function name to reflect that.
(There were more checks in older version and I forgot to
change the name...)

Thanks,
Reiji