Hi Marc,
On Mon, Apr 3, 2023 at 5:27 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> On Sun, 02 Apr 2023 19:37:34 +0100,
> Jing Zhang <jingzhangos@xxxxxxxxxx> 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@xxxxxxxxxx>
> > Signed-off-by: Reiji Watanabe <reijiw@xxxxxxxxxx>
> > Signed-off-by: Jing Zhang <jingzhangos@xxxxxxxxxx>
> > ---
> > 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
