Re: [PATCH v10 1/5] KVM: arm64: Save ID registers' sanitized value per guest

From: Marc Zyngier <maz@kernel.org>
To: Jing Zhang <jingzhangos@google.com>
Cc: KVM <kvm@vger.kernel.org>, KVMARM <kvmarm@lists.linux.dev>,
	ARMLinux <linux-arm-kernel@lists.infradead.org>,
	Oliver Upton <oupton@google.com>, Will Deacon <will@kernel.org>,
	Paolo Bonzini <pbonzini@redhat.com>,
	James Morse <james.morse@arm.com>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Fuad Tabba <tabba@google.com>, Reiji Watanabe <reijiw@google.com>,
	Raghavendra Rao Ananta <rananta@google.com>
Subject: Re: [PATCH v10 1/5] KVM: arm64: Save ID registers' sanitized value per guest
Date: Wed, 31 May 2023 08:24:09 +0100	[thread overview]
Message-ID: <864jntc6au.wl-maz@kernel.org> (raw)
In-Reply-To: <CAAdAUtinwccyd+URCLjowXq47_LP_SjLNEqm-F9GqycmTZrYuw@mail.gmail.com>

On Tue, 30 May 2023 19:02:03 +0100,
Jing Zhang <jingzhangos@google.com> 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(&params, 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.