Re: [RFC PATCH 04/25] KVM: arm64: Introduce struct id_reg_info

[Reiji Watanabe <reijiw@google.com>]

> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -263,6 +263,76 @@ static bool trap_raz_wi(struct kvm_vcpu *vcpu,
> >               return read_zero(vcpu, p);
> >  }
> >
> > +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 sys_val
> > +      * with bits cleared for unsupported features for the guest.
> > +      */
> > +     u64     vcpu_limit_val;
>
> Maybe I'll see a need for both later, but at the moment I'd think we only
> need sys_val with the bits cleared for disabled features.

Uh, yes, sys_val is used in patch-15 and I should have introduced
the field in the patch.  I will fix it in v2.


> > -static int __set_id_reg(const struct kvm_vcpu *vcpu,
> > +static int __set_id_reg(struct kvm_vcpu *vcpu,
> >                       const struct sys_reg_desc *rd, void __user *uaddr,
> >                       bool raz)
> >  {
> >       const u64 id = sys_reg_to_index(rd);
> > +     u32 encoding = reg_to_encoding(rd);
> >       int err;
> >       u64 val;
> >
> > @@ -1252,10 +1327,18 @@ static int __set_id_reg(const struct kvm_vcpu *vcpu,
> >       if (err)
> >               return err;
> >
> > -     /* This is what we mean by invariant: you can't change it. */
> > -     if (val != read_id_reg(vcpu, rd, raz))
> > +     /* Don't allow to change the reg unless the reg has id_reg_info */
> > +     if (val != read_id_reg(vcpu, rd, raz) && !GET_ID_REG_INFO(encoding))
> >               return -EINVAL;
> >
> > +     if (raz)
> > +             return (val == 0) ? 0 : -EINVAL;
>
> This is already covered by the val != read_id_reg(vcpu, rd, raz) check.

Yes, it can simply return 0 for raz case in this patch.
I will fix this in v2.


> > +     err = validate_id_reg(vcpu, rd, val);
> > +     if (err)
> > +             return err;
> > +
> > +     __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(encoding)) = val;
> >       return 0;
> >  }
> >
> > @@ -2818,6 +2901,23 @@ int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
> >       return write_demux_regids(uindices);
> >  }
> >
> > +static void id_reg_info_init_all(void)
> > +{
> > +     int i;
> > +     struct id_reg_info *id_reg;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(id_reg_info_table); i++) {
> > +             id_reg = (struct id_reg_info *)id_reg_info_table[i];
> > +             if (!id_reg)
> > +                     continue;
> > +
> > +             if (id_reg->init)
> > +                     id_reg->init(id_reg);
> > +             else
> > +                     id_reg_info_init(id_reg);
>
> Maybe call id_reg->init(id_reg) from within id_reg_info_init() in case we
> wanted to apply some common id register initialization at some point?

Thank you for the nice suggestion.
That sounds like a better idea. I'll look into fixing it in v2.

Thanks,
Reiji