LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Borislav Petkov <bp@alien8.de>
To: Jane Malalane <jane.malalane@citrix.com>
Cc: LKML <linux-kernel@vger.kernel.org>,
	x86@kernel.org, Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, "H. Peter Anvin" <hpa@zytor.com>,
	Pu Wen <puwen@hygon.cn>, Paolo Bonzini <pbonzini@redhat.com>,
	Sean Christopherson <seanjc@google.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Yazen Ghannam <Yazen.Ghannam@amd.com>,
	Brijesh Singh <brijesh.singh@amd.com>,
	Huang Rui <ray.huang@amd.com>, Andy Lutomirski <luto@kernel.org>,
	Kim Phillips <kim.phillips@amd.com>,
	stable@vger.kernel.org
Subject: Re: [PATCH] x86/cpu: Fix migration safety with X86_BUG_NULL_SEL
Date: Fri, 1 Oct 2021 16:19:54 +0200	[thread overview]
Message-ID: <YVcZCgOVkCPz1kwO@zn.tnic> (raw)
In-Reply-To: <20211001133349.9825-1-jane.malalane@citrix.com>

On Fri, Oct 01, 2021 at 02:33:49PM +0100, Jane Malalane wrote:
> Subject: Re: [PATCH] x86/cpu: Fix migration safety with X86_BUG_NULL_SEL
> ...
> Currently, Linux probes for X86_BUG_NULL_SEL unconditionally which
> makes it unsafe to migrate in a virtualised environment as the
> properties across the migration pool might differ.

Sorry but you need to explain "migration safety" in greater detail -
we're not all virtualizers.

> Zen3 adds the NullSelectorClearsBase bit to indicate that loading
> a NULL segment selector zeroes the base and limit fields, as well as
> just attributes. Zen2 also has this behaviour but doesn't have the
> NSCB bit.

I'm guessing you can detect Zen2 too, without the CPUID bit?

> When virtualised, NSCB might be cleared for migration safety,
> therefore we must not probe. Always honour the NSCB bit in this case,

Who is "we"?

> as the hypervisor is expected to synthesize it in the Zen2 case.
> 
> Signed-off-by: Jane Malalane <jane.malalane@citrix.com>
> ---
> CC: <x86@kernel.org>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: Borislav Petkov <bp@alien8.de>
> CC: "H. Peter Anvin" <hpa@zytor.com>
> CC: Pu Wen <puwen@hygon.cn>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Sean Christopherson <seanjc@google.com>
> CC: Peter Zijlstra <peterz@infradead.org>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Yazen Ghannam <Yazen.Ghannam@amd.com>
> CC: Brijesh Singh <brijesh.singh@amd.com>
> CC: Huang Rui <ray.huang@amd.com>
> CC: Andy Lutomirski <luto@kernel.org>
> CC: Kim Phillips <kim.phillips@amd.com>
> CC: <stable@vger.kernel.org>
> ---
>  arch/x86/include/asm/cpufeatures.h |  2 +-
>  arch/x86/kernel/cpu/amd.c          | 23 +++++++++++++++++++++++
>  arch/x86/kernel/cpu/common.c       |  6 ++----
>  arch/x86/kernel/cpu/cpu.h          |  1 +
>  arch/x86/kernel/cpu/hygon.c        | 23 +++++++++++++++++++++++
>  5 files changed, 50 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index d0ce5cfd3ac1..f571e4f6fe83 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -96,7 +96,7 @@
>  #define X86_FEATURE_SYSCALL32		( 3*32+14) /* "" syscall in IA32 userspace */
>  #define X86_FEATURE_SYSENTER32		( 3*32+15) /* "" sysenter in IA32 userspace */
>  #define X86_FEATURE_REP_GOOD		( 3*32+16) /* REP microcode works well */
> -/* FREE!                                ( 3*32+17) */
> +#define X86_FEATURE_NSCB		( 3*32+17) /* Null Selector Clears Base */

You don't really need that bit definition - you check CPUID in the
early_init_amd/hygon() functions and that is enough. IOW, based on that
CPUID bit, you can call detect_null_seg_behavior() and set (or not)
X86_BUG_NULL_SEG.

...

> diff --git a/arch/x86/kernel/cpu/hygon.c b/arch/x86/kernel/cpu/hygon.c
> index 6d50136f7ab9..765f1556d964 100644
> --- a/arch/x86/kernel/cpu/hygon.c
> +++ b/arch/x86/kernel/cpu/hygon.c
> @@ -264,6 +264,29 @@ static void early_init_hygon(struct cpuinfo_x86 *c)
>  	if (c->x86_power & BIT(14))
>  		set_cpu_cap(c, X86_FEATURE_RAPL);
>  
> +	/*
> +	 * Zen1 and earlier CPUs don't clear segment base/limits when
> +	 * loading a NULL selector.  This has been designated
> +	 * X86_BUG_NULL_SEG.
> +	 *
> +	 * Zen3 CPUs advertise Null Selector Clears Base in CPUID.
> +	 * Zen2 CPUs also have this behaviour, but no CPUID bit.
> +	 *
> +	 * A hypervisor may sythesize the bit, but may also hide it
> +	 * for migration safety, so we must not probe for model
> +	 * specific behaviour when virtualised.
> +	 */
> +	if (c->extended_cpuid_level >= 0x80000021 &&
> +	    cpuid_eax(0x80000021) & BIT(6))
> +	        set_cpu_cap(c, X86_FEATURE_NSCB);
> +
> +	if (!cpu_has(c, X86_FEATURE_HYPERVISOR) && !cpu_has(c, X86_FEATURE_NSCB) &&
> +	    c->x86 == 0x18)
> +                detect_null_seg_behavior(c);
> +
> +	if (!cpu_has(c, X86_FEATURE_NSCB))
> +		set_cpu_bug(c, X86_BUG_NULL_SEG);

Please integrate scripts/checkpatch.pl into your patch creation
workflow. Some of the warnings/errors *actually* make sense.

I'll show you that there's whitespace damage here:

ERROR: code indent should use tabs where possible
#238: FILE: arch/x86/kernel/cpu/hygon.c:281:
+^I        set_cpu_cap(c, X86_FEATURE_NSCB);$

ERROR: code indent should use tabs where possible
#242: FILE: arch/x86/kernel/cpu/hygon.c:285:
+                detect_null_seg_behavior(c);$

WARNING: please, no spaces at the start of a line
#242: FILE: arch/x86/kernel/cpu/hygon.c:285:
+                detect_null_seg_behavior(c);$

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

  parent reply	other threads:[~2021-10-01 14:20 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-01 13:33 Jane Malalane
2021-10-01 14:07 ` Andrew Cooper
2021-10-01 14:19 ` Borislav Petkov [this message]
2021-10-06 14:15   ` Andrew Cooper
2021-10-06 14:30     ` Borislav Petkov
2021-10-05 20:58 ` Andy Lutomirski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YVcZCgOVkCPz1kwO@zn.tnic \
    --to=bp@alien8.de \
    --cc=Yazen.Ghannam@amd.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=brijesh.singh@amd.com \
    --cc=hpa@zytor.com \
    --cc=jane.malalane@citrix.com \
    --cc=kim.phillips@amd.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=mingo@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=peterz@infradead.org \
    --cc=puwen@hygon.cn \
    --cc=ray.huang@amd.com \
    --cc=seanjc@google.com \
    --cc=stable@vger.kernel.org \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    --subject='Re: [PATCH] x86/cpu: Fix migration safety with X86_BUG_NULL_SEL' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).