Hi Marc,
On Wed, May 31, 2023 at 12:24 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> On Tue, 30 May 2023 19:02:03 +0100,
> Jing Zhang <jingzhangos@xxxxxxxxxx> wrote:
>
> [...]
>
> > > > +/*
> > > > + * Set the guest's ID registers with ID_SANITISED() to the host's sanitized value.
> > > > + */
> > > > +void kvm_arm_init_id_regs(struct kvm *kvm)
> > > > +{
> > > > + const struct sys_reg_desc *idreg;
> > > > + struct sys_reg_params params;
> > > > + u32 id;
> > > > +
> > > > + /* Find the first idreg (SYS_ID_PFR0_EL1) in sys_reg_descs. */
> > > > + id = SYS_ID_PFR0_EL1;
> > > > + params = encoding_to_params(id);
> > > > + idreg = find_reg(¶ms, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
> > > > + if (WARN_ON(!idreg))
> > > > + return;
> > >
> > > What is this trying to guard against? Not finding ID_PFR0_EL1 in the
> > > sysreg table? But this says nothing about the following registers (all
> > > 55 of them), so why do we need to special-case this one?
> > Here is to find the first idreg in the array and warn that no idregs
> > found in the array with the assumption that ID_PFR0_EL1 is the first
> > one defined and if it is not found, then no other idregs are defined
> > either.
>
> I didn't make my point clear. What we have is a purely static array.
> Why should we perform such a test on every single VM creation? Any
> structural validation should only happen once, at KVM init time.
>
> > Another way is to go through all the regs in array sys_reg_descs and
> > do the initialization if it is a idreg.
>
> That'd be a waste of precious cycles.
>
> This WARN_ON()+early return should go, but the rest is fine.
>
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
That makes sense. We don't have to do the check on every single VM
creation. And the code to find the ID_PFR0_EL1 should not be done for
every VM creation either.
I'll use a static variable to save the pointer to ID_PFR0_EL1 and do
the check and search in kvm_sys_reg_table_init().
Thanks,
Jing
