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

From: Oliver Upton <oliver.upton@linux.dev>
To: Marc Zyngier <maz@kernel.org>
Cc: kvmarm@lists.linux.dev, kvm@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	Catalin Marinas <catalin.marinas@arm.com>,
	Eric Auger <eric.auger@redhat.com>,
	Mark Brown <broonie@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Will Deacon <will@kernel.org>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	Andre Przywara <andre.przywara@arm.com>,
	Chase Conklin <chase.conklin@arm.com>,
	Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>,
	Darren Hart <darren@os.amperecomputing.com>,
	Miguel Luis <miguel.luis@oracle.com>,
	James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Zenghui Yu <yuzenghui@huawei.com>
Subject: Re: [PATCH v2 14/26] KVM: arm64: nv: Add trap forwarding infrastructure
Date: Mon, 31 Jul 2023 10:02:47 -0700	[thread overview]
Message-ID: <ZMfpN4nTEBpbvHBc@linux.dev> (raw)
In-Reply-To: <87fs57qdm2.wl-maz@kernel.org>

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@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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel