Received-SPF: pass (google.com: domain of linux-kernel+bounces-131671-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Thu, 4 Apr 2024 08:05:52 -0700
In-Reply-To: <20240403153514.CC2C9D13@davehans-spike.ostc.intel.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240403153508.7328E749@davehans-spike.ostc.intel.com> <20240403153514.CC2C9D13@davehans-spike.ostc.intel.com>
Message-ID: <Zg7B0JwJD6sfjVIY@google.com>
Subject: Re: [PATCH 4/4] x86/xen: Enumerate NX from CPUID directly
From: Sean Christopherson <seanjc@google.com>
To: Dave Hansen <dave.hansen@linux.intel.com>
Cc: linux-kernel@vger.kernel.org, jgross@suse.com, tglx@linutronix.de, 
	x86@kernel.org, bp@alien8.de
Content-Type: text/plain; charset="us-ascii"

On Wed, Apr 03, 2024, Dave Hansen wrote:
> +/*
> + * It is too early for boot_cpu_has() and friends to work.
> + * Use CPUID to directly enumerate NX support.
> + */
> +static inline void xen_configure_nx(void)
> +{
> +	bool nx_supported;
> +	u32 eax = 0;
> +
> +	get_cpuid_region_leaf(0x80000001, CPUID_EDX, &eax);
> +
> +	nx_supported = eax & (X86_FEATURE_NX & 31);

Heh, I wasn't looking to review this, but this is wrong, and the fly-by comment
I was going to make is relevant (and got really long once I started typing...).

X86_FEATURE_NX isn't a bit, it's bit position: (1*32+20) & 0x31 == 20.  I.e. the
code would actually need to be

	nx_supported = edx & BIT(X86_FEATURE_NX & 31);

The TL;DR version of comment I came to give, is that KVM has a lot of infrastructure
for manipulating raw CPUID values using X86_FEATURE_* flags, and that I think we
should seriously consider moving much of KVM's infrastructure to core x86. 

KVM has several macros for doing this sort of thing, and we solve the "way shorter
than any helper" conundrum is by pasting tokens, e.g.

	#define feature_bit(name)  __feature_bit(X86_FEATURE_##name)

which yields fairly readable code, e.g. this would be

	nx_supported = edx & feature_bit(NX);

and if you want to go really terse with a macro that is local to a .c file

	#define F feature_bit

though I doubt that's necessary outside of KVM (KVM has ~200 uses in a single
function that computes the support guest CPUID).

Grabbing feature_bit() directly from KVM would be cumbersome, as __feature_bit()
pulls in a _lot_ of dependencies that aren't strictly necessary.  But if you do
do want to add a generic macro, I definitely think it's worth moving KVM's stuff
to common code, because all of the dependencies are compile-time assertions to
guard against incorrect usage.  E.g. using the "& 31" trick on scattered flags
for raw CPUID will give the wrong result, because the bit position for scattered
flags doesn't match the bit position for hardware-defined CPUID.

But if we went a step further, KVM's code can also programatically generate the
inputs to CPUID.  x86_feature_cpuid() returns a cpuid_reg structure, which gives
the function, index, and register:

	struct cpuid_reg {
		u32 function;
		u32 index;
		int reg;
	};

E.g. if we move the KVM stuff to common code, we could have a helper like:

	static __always_inline bool x86_cpuid_has(unsigned int feature)
	{
		struct cpuid_reg cpuid = x86_feature_cpuid(feature);
		u32 val;

		if (!get_cpuid_region_leaf(cpuid.function, cpuid.reg, &val))
			return false;

		return val & __feature_bit(feature);
	}

and then the NX Xen code is more simply

	x86_configure_nx(x86_cpuid_has(X86_FEATURE_NX));

If we wanted to go really crazy, I bet we could figure out a way to use an
alternative-like implementation to automagically redirect boot_cpuid_has() to
a raw CPUID-based check if it's invoked before boot_cpu_data() is populated,
without increasing the code footprint.