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.
