From: Dave Hansen <[email protected]>
xen_start_kernel() is some of the first C code run "Xen PV" systems.
It precedes things like setup_arch() or processor initialization code
that would be thought of as "very early".
That means that 'boot_cpu_data' is garbage. It has not even
established the utter basics like if the CPU supports the CPUID
instruction. Unfortunately get_cpu_cap() requires this exact
information.
Nevertheless, xen_start_kernel() calls get_cpu_cap(). But it
works out in practice because it's looking for the NX bit which
comes from an extended CPUID leaf that doesn't depend on
c->cpuid_level being set.
Stop calling get_cpu_cap(). Use CPUID directly. Add a little
helper to avoid cluttering up the xen_start_kernel() flow.
This removes the risky, barely-working call to get_cpu_cap().
Note: The '& 31' strips the "capability" word off of an
X86_FEATURE_* value. It's a magic number, but there are
a few of them sparsely scattered around and it's way
shorter than any helper.
Signed-off-by: Dave Hansen <[email protected]>
Cc: Juergen Gross <[email protected]>
---
b/arch/x86/xen/enlighten_pv.c | 19 ++++++++++++++++---
1 file changed, 16 insertions(+), 3 deletions(-)
diff -puN arch/x86/xen/enlighten_pv.c~xen-explain1 arch/x86/xen/enlighten_pv.c
--- a/arch/x86/xen/enlighten_pv.c~xen-explain1 2024-04-02 15:23:00.046913557 -0700
+++ b/arch/x86/xen/enlighten_pv.c 2024-04-02 15:23:00.050913562 -0700
@@ -1306,6 +1306,21 @@ static void __init xen_domu_set_legacy_f
extern void early_xen_iret_patch(void);
+/*
+ * 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);
+ x86_configure_nx(nx_supported);
+}
+
/* First C function to be called on Xen boot */
asmlinkage __visible void __init xen_start_kernel(struct start_info *si)
{
@@ -1369,9 +1384,7 @@ asmlinkage __visible void __init xen_sta
/* Get mfn list */
xen_build_dynamic_phys_to_machine();
- /* Work out if we support NX */
- get_cpu_cap(&boot_cpu_data);
- x86_configure_nx(boot_cpu_has(X86_FEATURE_NX));
+ xen_configure_nx();
/*
* Set up kernel GDT and segment registers, mainly so that
_
On 03.04.24 17:35, Dave Hansen wrote:
> From: Dave Hansen <[email protected]>
>
> xen_start_kernel() is some of the first C code run "Xen PV" systems.
Nit: s/run/run on/
> It precedes things like setup_arch() or processor initialization code
> that would be thought of as "very early".
>
> That means that 'boot_cpu_data' is garbage. It has not even
> established the utter basics like if the CPU supports the CPUID
> instruction. Unfortunately get_cpu_cap() requires this exact
> information.
>
> Nevertheless, xen_start_kernel() calls get_cpu_cap(). But it
> works out in practice because it's looking for the NX bit which
> comes from an extended CPUID leaf that doesn't depend on
> c->cpuid_level being set.
>
> Stop calling get_cpu_cap(). Use CPUID directly. Add a little
> helper to avoid cluttering up the xen_start_kernel() flow.
>
> This removes the risky, barely-working call to get_cpu_cap().
>
> Note: The '& 31' strips the "capability" word off of an
> X86_FEATURE_* value. It's a magic number, but there are
> a few of them sparsely scattered around and it's way
> shorter than any helper.
>
> Signed-off-by: Dave Hansen <[email protected]>
> Cc: Juergen Gross <[email protected]>
> ---
>
> b/arch/x86/xen/enlighten_pv.c | 19 ++++++++++++++++---
> 1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff -puN arch/x86/xen/enlighten_pv.c~xen-explain1 arch/x86/xen/enlighten_pv.c
> --- a/arch/x86/xen/enlighten_pv.c~xen-explain1 2024-04-02 15:23:00.046913557 -0700
> +++ b/arch/x86/xen/enlighten_pv.c 2024-04-02 15:23:00.050913562 -0700
> @@ -1306,6 +1306,21 @@ static void __init xen_domu_set_legacy_f
>
> extern void early_xen_iret_patch(void);
>
> +/*
> + * 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;
I'd prefer to name this variable edx.
> +
> + get_cpuid_region_leaf(0x80000001, CPUID_EDX, &eax);
> +
> + nx_supported = eax & (X86_FEATURE_NX & 31);
> + x86_configure_nx(nx_supported);
> +}
> +
> /* First C function to be called on Xen boot */
> asmlinkage __visible void __init xen_start_kernel(struct start_info *si)
> {
> @@ -1369,9 +1384,7 @@ asmlinkage __visible void __init xen_sta
> /* Get mfn list */
> xen_build_dynamic_phys_to_machine();
>
> - /* Work out if we support NX */
> - get_cpu_cap(&boot_cpu_data);
> - x86_configure_nx(boot_cpu_has(X86_FEATURE_NX));
> + xen_configure_nx();
>
> /*
> * Set up kernel GDT and segment registers, mainly so that
> _
Juergen
On 4/4/24 03:44, Jürgen Groß wrote:
>> @@ -1306,6 +1306,21 @@ static void __init xen_domu_set_legacy_f
>> extern void early_xen_iret_patch(void);
>> +/*
>> + * 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;
>
> I'd prefer to name this variable edx.
>
>> +
>> + get_cpuid_region_leaf(0x80000001, CPUID_EDX, &eax);
Heh. That's a nice way to say it. :)
I'll fix it up.
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.
On Thu, Apr 04, 2024 at 08:05:52AM -0700, Sean Christopherson wrote:
> ...
> and then the NX Xen code is more simply
>
> x86_configure_nx(x86_cpuid_has(X86_FEATURE_NX));
I can't say that I haven't looked at the KVM features code and haven't
thought that, yap, some of the bits we could make generic. And I was
going to even suggest that but then, there's this bigger CPUID feature
rework that has been brewing on the horizon for a loong while now and
where we want to read out the CPUID features once and have everything
else query those instead of everybody doing their own thing...
.. and some of that work is here:
https://lpc.events/event/17/contributions/1511/ ...
and tglx wanted to get this thing moving faster...
So yeah, I think we should finally unify all the feature bits handling
and reading so that there's a single set of APIs which are used by
everybody instead of the crazy "fun" we have now.
And then my hope is that you could kill/merge some of the KVM infra into
generic code.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette