Received-SPF: pass (google.com: domain of linux-kernel+bounces-131251-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <67938940-a7b9-4a75-883e-cc49c6097fcc@suse.com>
Date: Thu, 4 Apr 2024 12:44:20 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 4/4] x86/xen: Enumerate NX from CPUID directly
Content-Language: en-US
To: Dave Hansen <dave.hansen@linux.intel.com>, linux-kernel@vger.kernel.org
Cc: tglx@linutronix.de, x86@kernel.org, bp@alien8.de
References: <20240403153508.7328E749@davehans-spike.ostc.intel.com>
 <20240403153514.CC2C9D13@davehans-spike.ostc.intel.com>
From: =?UTF-8?B?SsO8cmdlbiBHcm/Dnw==?= <jgross@suse.com>
In-Reply-To: <20240403153514.CC2C9D13@davehans-spike.ostc.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 03.04.24 17:35, Dave Hansen wrote:
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> 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 <dave.hansen@linux.intel.com>
> Cc: Juergen Gross <jgross@suse.com>
> ---
> 
>   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