[PATCH v5 5/6] KVM: arm64: Introduce ID register specific descriptor

[Jing Zhang]

Hi Marc,

On Mon, Apr 3, 2023 at 5:27 AM Marc Zyngier <maz at kernel.org> wrote:
>
> On Sun, 02 Apr 2023 19:37:34 +0100,
> Jing Zhang <jingzhangos at google.com> wrote:
> >
> > Introduce an ID feature register specific descriptor to include ID
> > register specific fields and callbacks besides its corresponding
> > general system register descriptor.
> >
> > No functional change intended.
> >
> > Co-developed-by: Reiji Watanabe <reijiw at google.com>
> > Signed-off-by: Reiji Watanabe <reijiw at google.com>
> > Signed-off-by: Jing Zhang <jingzhangos at google.com>
> > ---
> >  arch/arm64/kvm/id_regs.c  | 233 ++++++++++++++++++++++++++++----------
> >  arch/arm64/kvm/sys_regs.c |   2 +-
> >  arch/arm64/kvm/sys_regs.h |   1 +
> >  3 files changed, 178 insertions(+), 58 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c
> > index e92eacb0ad32..af86001e2686 100644
> > --- a/arch/arm64/kvm/id_regs.c
> > +++ b/arch/arm64/kvm/id_regs.c
> > @@ -18,6 +18,27 @@
> >
> >  #include "sys_regs.h"
> >
> > +struct id_reg_desc {
> > +     const struct sys_reg_desc       reg_desc;
> > +     /*
> > +      * ftr_bits points to the feature bits array defined in cpufeature.c for
> > +      * writable CPU ID feature register.
> > +      */
> > +     const struct arm64_ftr_bits *ftr_bits;
>
> Why do we need to keep this around? we already have all the required
> infrastructure to lookup a full arm64_ftr_reg. So why the added stuff?
You're right. Will use the lookup function.
>
> > +     /*
> > +      * Only bits with 1 are writable from userspace.
> > +      * This mask might not be necessary in the future whenever all ID
> > +      * registers are enabled as writable from userspace.
> > +      */
> > +     const u64 writable_mask;
> > +     /*
> > +      * This function returns the KVM sanitised register value.
> > +      * The value would be the same as the host kernel sanitised value if
> > +      * there is no KVM sanitisation for this id register.
> > +      */
> > +     u64 (*read_kvm_sanitised_reg)(const struct id_reg_desc *idr);
>
> Why can't this function return both the required value and a mask?
> Why can't it live in the sys_reg_desc structure?
>
> Frankly, I don't see a good reason to have this wrapper structure
> which makes things pointlessly complicated and prevent code sharing
> with the rest of the infrastructure.
Sure, will reuse the existing fields in sys_reg_desc structure as you
demonstrated.
>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.
Thanks,
Jing