[PATCH v2 14/26] KVM: arm64: nv: Add trap forwarding infrastructure

[Oliver Upton]

On Sat, Jul 29, 2023 at 10:19:17AM +0100, Marc Zyngier wrote:
> On Fri, 28 Jul 2023 19:33:31 +0100,
> Oliver Upton <oliver.upton at linux.dev> wrote:
> > Correct me if I'm wrong, but I don't think the compiler is going to
> > whine if any of these bitfields are initialized with a larger value than
> > can be represented... Do you think some BUILD_BUG_ON() is in order to
> > ensure that trap_group fits in ::cgt?
> > 
> > 	BUILD_BUG_ON(__NR_TRAP_IDS__ >= BIT(10));
> 
> Indeed. This might also apply to ::fgt, and I want to add some sanity
> checks to verify that the whole union isn't larger than a 'void *', as
> we rely on that.

Agreed on both.

> > > +/*
> > > + * Map encoding to trap bits for exception reported with EC=0x18.
> > > + * These must only be evaluated when running a nested hypervisor, but
> > > + * that the current context is not a hypervisor context. When the
> > > + * trapped access matches one of the trap controls, the exception is
> > > + * re-injected in the nested hypervisor.
> > > + */
> > > +static const struct encoding_to_trap_config encoding_to_cgt[] __initconst = {
> > > +};
> > > +
> > > +static DEFINE_XARRAY(sr_forward_xa);
> > > +
> > > +static union trap_config get_trap_config(u32 sysreg)
> > > +{
> > > +	return (union trap_config) {
> > > +		.val = xa_to_value(xa_load(&sr_forward_xa, sysreg)),
> > > +	};
> > 
> > Should we be checking for NULL here? AFAICT, the use of sentinel values
> > in the trap_group enum would effectively guarantee each trap_config has
> > a nonzero value.
> 
> if xa_load() returns NULL, xa_to_value() will still give us a 0, which
> is an indication of a sysreg that isn't present in the trap
> configuration. This can happen if we trap something that isn't yet
> supported in NV, which is quite common. This allows us to use features
> on the host without having to immediately write the same support for
> NV guests.

That's fine by me, I couldn't piece it together where a value of 0 is
explicitly handled as having no trap config.

> But this is obviously a temporary situation. At some point, I'll
> become a total bastard and demand that people treat NV as a first
> class citizen. One day ;-).

No time like the present!

--
Thanks,
Oliver