[PATCH] x86: Prefer MWAIT over HALT on AMD processors

[PATCH] x86: Prefer MWAIT over HALT on AMD processors

[Wyes Karny]

From: Lewis Caroll <lewis.carroll@amd.com>

Currently in the absence of the cpuidle driver (eg: when global
C-States are disabled in the BIOS or when cpuidle is driver is not
compiled in), the default idle state on AMD Zen processors uses the
HALT instruction even though there is support for MWAIT instruction
which is more efficient than HALT.

The below table represents the exit latency for HALT and MWAIT on AMD
Zen 3 system.
Exit latency is measured by issuing a wakeup (IPI) to other
CPU and measuring how many clock cycles it took to wakeup.
Each iteration measures 10K wakeups by pinning source and
destination.

HALT:

25.0000th percentile  :      1900 ns
50.0000th percentile  :      2000 ns
75.0000th percentile  :      2300 ns
90.0000th percentile  :      2500 ns
95.0000th percentile  :      2600 ns
99.0000th percentile  :      2800 ns
99.5000th percentile  :      3000 ns
99.9000th percentile  :      3400 ns
99.9500th percentile  :      3600 ns
99.9900th percentile  :      5900 ns
  Min latency         :      1700 ns
  Max latency         :      5900 ns
Total Samples      9999

MWAIT:

25.0000th percentile  :      1400 ns
50.0000th percentile  :      1500 ns
75.0000th percentile  :      1700 ns
90.0000th percentile  :      1800 ns
95.0000th percentile  :      1900 ns
99.0000th percentile  :      2300 ns
99.5000th percentile  :      2500 ns
99.9000th percentile  :      3200 ns
99.9500th percentile  :      3500 ns
99.9900th percentile  :      4600 ns
  Min latency         :      1200 ns
  Max latency         :      4600 ns
Total Samples      9997

Improvement: 21.74%

A similar trend is observed on older Zen processors also.

Here we enable MWAIT instruction as the default idle call for AMD
Zen processors which support MWAIT. We retain the existing behaviour
for older processors which depend on HALT.

NOTE: This change only impacts the default idle behaviour in the
absence of cpuidle driver. If the cpuidle driver is present, it
controls the processor idle behaviour.

Fixes: commit b253149b843f ("sched/idle/x86: Restore mwait_idle() to fix boot hangs, to improve power savings and to improve performance")
Signed-off-by: Lewis Caroll <lewis.carroll@amd.com>
Signed-off-by: Wyes Karny <Wyes.Karny@amd.com>
---
 arch/x86/kernel/process.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 81d8ef036637..952d0382354b 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -809,19 +809,34 @@ static void amd_e400_idle(void)
 	raw_local_irq_enable();
 }
 
+/*
+ * Intel and AMD Zen processors allow MWAIT early on.
+ */
+static inline bool early_mwait_supported(const struct cpuinfo_x86 *c)
+{
+	if (c->x86_vendor == X86_VENDOR_INTEL)
+		return true;
+
+	if (c->x86_vendor == X86_VENDOR_AMD && cpu_has(c, X86_FEATURE_ZEN))
+		return true;
+
+	return false;
+}
+
 /*
  * Intel Core2 and older machines prefer MWAIT over HALT for C1.
  * We can't rely on cpuidle installing MWAIT, because it will not load
  * on systems that support only C1 -- so the boot default must be MWAIT.
  *
- * Some AMD machines are the opposite, they depend on using HALT.
+ * Even AMD Zen processors prefer MWAIT over HALT. But older AMD processors
+ * depend on HALT.
  *
  * So for default C1, which is used during boot until cpuidle loads,
- * use MWAIT-C1 on Intel HW that has it, else use HALT.
+ * use MWAIT-C1 on Intel and AMD HW that have it, else use HALT.
  */
 static int prefer_mwait_c1_over_halt(const struct cpuinfo_x86 *c)
 {
-	if (c->x86_vendor != X86_VENDOR_INTEL)
+	if (!early_mwait_supported(c))
 		return 0;
 
 	if (!cpu_has(c, X86_FEATURE_MWAIT) || boot_cpu_has_bug(X86_BUG_MONITOR))
-- 
2.27.0

Re: [PATCH] x86: Prefer MWAIT over HALT on AMD processors

[Borislav Petkov]

On Tue, Apr 05, 2022 at 06:30:21PM +0530, Wyes Karny wrote:
> From: Lewis Caroll <lewis.carroll@amd.com>
> 
> Currently in the absence of the cpuidle driver (eg: when global
> C-States are disabled in the BIOS or when cpuidle is driver is not
> compiled in),

When does that ever happen?

Sounds like a very very niche situation to me...

> Here we enable MWAIT instruction as the default idle call for AMD
> Zen processors which support MWAIT. We retain the existing behaviour
> for older processors which depend on HALT.

Please use passive voice in your commit message: no "we" or "I", etc,
and describe your changes in imperative mood.

Also, pls read section "2) Describe your changes" in
Documentation/process/submitting-patches.rst for more details.

Also, see section "Changelog" in
Documentation/process/maintainer-tip.rst

Bottom line is: personal pronouns are ambiguous in text, especially with
so many parties/companies/etc developing the kernel so let's avoid them
please.

>  static int prefer_mwait_c1_over_halt(const struct cpuinfo_x86 *c)
>  {
> -	if (c->x86_vendor != X86_VENDOR_INTEL)
> +	if (!early_mwait_supported(c))

Isn't it enough to simply do here:

	if (cpu_has(c, X86_FEATURE_ZEN))
		return 1;

        if (c->x86_vendor != X86_VENDOR_INTEL)
                return 0;

	...


-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86: Prefer MWAIT over HALT on AMD processors

[Peter Zijlstra]

On Tue, Apr 05, 2022 at 06:30:21PM +0530, Wyes Karny wrote:
> +static inline bool early_mwait_supported(const struct cpuinfo_x86 *c)
> +{
> +	if (c->x86_vendor == X86_VENDOR_INTEL)
> +		return true;
> +
> +	if (c->x86_vendor == X86_VENDOR_AMD && cpu_has(c, X86_FEATURE_ZEN))

What about Hygon? For some reason you guys don't co-ordinate and we end
up getting endless 'make-same' patches, sometimes separated by years :/

> +		return true;
> +
> +	return false;
> +}

Re: [PATCH] x86: Prefer MWAIT over HALT on AMD processors

[Dave Hansen]

On 4/5/22 07:07, Peter Zijlstra wrote:
> On Tue, Apr 05, 2022 at 06:30:21PM +0530, Wyes Karny wrote:
>> +static inline bool early_mwait_supported(const struct cpuinfo_x86 *c)
>> +{
>> +	if (c->x86_vendor == X86_VENDOR_INTEL)
>> +		return true;
>> +
>> +	if (c->x86_vendor == X86_VENDOR_AMD && cpu_has(c, X86_FEATURE_ZEN))
> What about Hygon? For some reason you guys don't co-ordinate and we end
> up getting endless 'make-same' patches, sometimes separated by years :/

Believe it or not Hygon seems to work OK with this because:

> static void init_hygon(struct cpuinfo_x86 *c)
> {
...
>         set_cpu_cap(c, X86_FEATURE_ZEN);

I do worry a bit though that using X86_FEATURE_ZEN is going to bite us
long-term.  It currently claims to be set for "family 0x17 or above":

> #define X86_FEATURE_ZEN                 ( 7*32+28) /* "" CPU is AMD family 0x17 or above (Zen) */

But then it goes and gets used in side-channel defense:

>         if (!static_cpu_has(X86_FEATURE_ZEN)) {
>                 msr |= ssbd_tif_to_amd_ls_cfg(tifn);
>                 wrmsrl(MSR_AMD64_LS_CFG, msr);
>                 return;
>         }

This seem _bit_ at odds with the commit message (and the AMD SSBD
whitepaper):

>     Add the necessary synchronization logic for AMD family 17H. 

So, is X86_FEATURE_ZEN for family==0x17, or family>=0x17?

RE: [PATCH] x86: Prefer MWAIT over HALT on AMD processors

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Dave Hansen <dave.hansen@intel.com>
> Sent: Tuesday, April 5, 2022 10:10
> To: Peter Zijlstra <peterz@infradead.org>; Karny, Wyes
> <Wyes.Karny@amd.com>
> Cc: linux-kernel@vger.kernel.org; Carroll, Lewis <Lewis.Carroll@amd.com>;
> Limonciello, Mario <Mario.Limonciello@amd.com>; Shenoy, Gautham Ranjal
> <gautham.shenoy@amd.com>; Narayan, Ananth
> <Ananth.Narayan@amd.com>; Rao, Bharata Bhasker <bharata@amd.com>;
> len.brown@intel.com; x86@kernel.org; tglx@linutronix.de;
> mingo@redhat.com; bp@alien8.de; dave.hansen@linux.intel.com;
> hpa@zytor.com; chang.seok.bae@intel.com; keescook@chromium.org;
> metze@samba.org; zhengqi.arch@bytedance.com; mark.rutland@arm.com
> Subject: Re: [PATCH] x86: Prefer MWAIT over HALT on AMD processors
> 
> On 4/5/22 07:07, Peter Zijlstra wrote:
> > On Tue, Apr 05, 2022 at 06:30:21PM +0530, Wyes Karny wrote:
> >> +static inline bool early_mwait_supported(const struct cpuinfo_x86 *c)
> >> +{
> >> +	if (c->x86_vendor == X86_VENDOR_INTEL)
> >> +		return true;
> >> +
> >> +	if (c->x86_vendor == X86_VENDOR_AMD && cpu_has(c,
> X86_FEATURE_ZEN))
> > What about Hygon? For some reason you guys don't co-ordinate and we
> end
> > up getting endless 'make-same' patches, sometimes separated by years :/
> 
> Believe it or not Hygon seems to work OK with this because:
> 
> > static void init_hygon(struct cpuinfo_x86 *c)
> > {
> ...
> >         set_cpu_cap(c, X86_FEATURE_ZEN);
> 
> I do worry a bit though that using X86_FEATURE_ZEN is going to bite us
> long-term.  It currently claims to be set for "family 0x17 or above":
> 
> > #define X86_FEATURE_ZEN                 ( 7*32+28) /* "" CPU is AMD family
> 0x17 or above (Zen) */
> 
> But then it goes and gets used in side-channel defense:
> 
> >         if (!static_cpu_has(X86_FEATURE_ZEN)) {
> >                 msr |= ssbd_tif_to_amd_ls_cfg(tifn);
> >                 wrmsrl(MSR_AMD64_LS_CFG, msr);
> >                 return;
> >         }
> 
> This seem _bit_ at odds with the commit message (and the AMD SSBD
> whitepaper):
> 
> >     Add the necessary synchronization logic for AMD family 17H.
> 
> So, is X86_FEATURE_ZEN for family==0x17, or family>=0x17?

There are Zen family CPUs and APUs from family 0x19.  Perhaps at the
time of the whitepaper there weren't yet though.

Re: [PATCH] x86: Prefer MWAIT over HALT on AMD processors

[Dave Hansen]

On 4/5/22 08:34, Limonciello, Mario wrote:
>>>         if (!static_cpu_has(X86_FEATURE_ZEN)) {
>>>                 msr |= ssbd_tif_to_amd_ls_cfg(tifn);
>>>                 wrmsrl(MSR_AMD64_LS_CFG, msr);
>>>                 return;
>>>         }
>> This seem _bit_ at odds with the commit message (and the AMD SSBD
>> whitepaper):
>>
>>>     Add the necessary synchronization logic for AMD family 17H.
>> So, is X86_FEATURE_ZEN for family==0x17, or family>=0x17?
> There are Zen family CPUs and APUs from family 0x19.  Perhaps at the
> time of the whitepaper there weren't yet though.

Is this a gap in the documentation, then?  Is there some documentation
of the availability of SSBD mitigations on family 0x19?

Anyway, back to MWAIT...  It would be really great to include the
assumptions about what X86_FEATURE_ZEN means in the context of this
patch.  Does this patch *really* mean "Zen microarchitecture" only?

RE: [PATCH] x86: Prefer MWAIT over HALT on AMD processors

[Carroll, Lewis]

[AMD Official Use Only]

Merging some comments from subsequent threads below...

> -----Original Message-----
> From: Borislav Petkov <bp@alien8.de>
> Sent: Tuesday, April 5, 2022 10:06 AM
> To: Karny, Wyes <Wyes.Karny@amd.com>
> Cc: linux-kernel@vger.kernel.org; Carroll, Lewis <Lewis.Carroll@amd.com>;
> Limonciello, Mario <Mario.Limonciello@amd.com>; Shenoy, Gautham Ranjal
> <gautham.shenoy@amd.com>; Narayan, Ananth <Ananth.Narayan@amd.com>; Rao,
> Bharata Bhasker <bharata@amd.com>; len.brown@intel.com; x86@kernel.org;
> tglx@linutronix.de; mingo@redhat.com; dave.hansen@linux.intel.com;
> hpa@zytor.com; peterz@infradead.org; chang.seok.bae@intel.com;
> keescook@chromium.org; metze@samba.org; zhengqi.arch@bytedance.com;
> mark.rutland@arm.com
> Subject: Re: [PATCH] x86: Prefer MWAIT over HALT on AMD processors
> 
> [CAUTION: External Email]
> 
> On Tue, Apr 05, 2022 at 06:30:21PM +0530, Wyes Karny wrote:
> > From: Lewis Caroll <lewis.carroll@amd.com>
> >
> > Currently in the absence of the cpuidle driver (eg: when global
> > C-States are disabled in the BIOS or when cpuidle is driver is not
> > compiled in),
> 
> When does that ever happen?
> 
> Sounds like a very very niche situation to me...
> 

This happens when:
 * User disables global C-states in BIOS
 * User disables cpuidle (e.g. idle=off or processor.max_cstate=0)
 * Kernel does not have CONFIG_ACPI_PROCESSOR_IDLE
Genesis of this patch is customer performance observations.

> > Here we enable MWAIT instruction as the default idle call for AMD Zen
> > processors which support MWAIT. We retain the existing behaviour for
> > older processors which depend on HALT.
> 
> Please use passive voice in your commit message: no "we" or "I", etc, and
> describe your changes in imperative mood.
> 
> Also, pls read section "2) Describe your changes" in
> Documentation/process/submitting-patches.rst for more details.
> 
> Also, see section "Changelog" in
> Documentation/process/maintainer-tip.rst
> 
> Bottom line is: personal pronouns are ambiguous in text, especially with so
> many parties/companies/etc developing the kernel so let's avoid them please.
> 
> >  static int prefer_mwait_c1_over_halt(const struct cpuinfo_x86 *c)  {
> > -     if (c->x86_vendor != X86_VENDOR_INTEL)
> > +     if (!early_mwait_supported(c))
> 
> Isn't it enough to simply do here:
> 
>         if (cpu_has(c, X86_FEATURE_ZEN))
>                 return 1;
> 
>         if (c->x86_vendor != X86_VENDOR_INTEL)
>                 return 0;
> 
>         ...

Yes. We felt the code more readable with the prefer_mwait_c1_over_halt fn.
Hygon CPU init indeed sets X86_FEATURE ZEN.
AMD CPU init sets X86_FEATURE_ZEN for family >= 17h (not only 17h).
Cleanest solution might be a new CPU feature (e.g. X86_PREFER_MWAIT_IDLE) that
gets set appropriately, but that would require touching more files.

> 
> 
> --
> Regards/Gruss,
>     Boris.

Re: [PATCH] x86: Prefer MWAIT over HALT on AMD processors

[Borislav Petkov]

On Tue, Apr 05, 2022 at 08:26:53PM +0000, Carroll, Lewis wrote:
> This happens when:
>  * User disables global C-states in BIOS
>  * User disables cpuidle (e.g. idle=off or processor.max_cstate=0)
>  * Kernel does not have CONFIG_ACPI_PROCESSOR_IDLE

All three or any one of those?

> Genesis of this patch is customer performance observations.

Please add that explanation to the changelog - it is important when
looking back, trying to figure out why this was done.

> Yes. We felt the code more readable with the prefer_mwait_c1_over_halt fn.
> Hygon CPU init indeed sets X86_FEATURE ZEN.
> AMD CPU init sets X86_FEATURE_ZEN for family >= 17h (not only 17h).

Yes, but this new logic you're adding, basically says, use MWAIT on all
Zen uarch CPUs, right?

So why not write exactly that?

The simpler the logic and the clearer the code, the better.

> Cleanest solution might be a new CPU feature (e.g.
> X86_PREFER_MWAIT_IDLE) that gets set appropriately, but that would
> require touching more files.

Yes, I thought about it too.

Not really necessary if what I wrote above fits.

And while you're touching files, pls add that change too:

---
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 73e643ae94b6..c1091f78f104 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -219,7 +219,7 @@
 #define X86_FEATURE_IBRS		( 7*32+25) /* Indirect Branch Restricted Speculation */
 #define X86_FEATURE_IBPB		( 7*32+26) /* Indirect Branch Prediction Barrier */
 #define X86_FEATURE_STIBP		( 7*32+27) /* Single Thread Indirect Branch Predictors */
-#define X86_FEATURE_ZEN			( 7*32+28) /* "" CPU is AMD family 0x17 or above (Zen) */
+#define X86_FEATURE_ZEN			( 7*32+28) /* "" Set on CPUs of the Zen uarch */
 #define X86_FEATURE_L1TF_PTEINV		( 7*32+29) /* "" L1TF workaround PTE inversion */
 #define X86_FEATURE_IBRS_ENHANCED	( 7*32+30) /* Enhanced IBRS */
 #define X86_FEATURE_MSR_IA32_FEAT_CTL	( 7*32+31) /* "" MSR IA32_FEAT_CTL configured */

so that dhansen and peterz are not confused anymore. :-)

Thx.

-- 
Regards/Gruss,
    Boris.

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

RE: [PATCH] x86: Prefer MWAIT over HALT on AMD processors

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Dave Hansen <dave.hansen@intel.com>
> Sent: Tuesday, April 5, 2022 10:47
> To: Limonciello, Mario <Mario.Limonciello@amd.com>; Peter Zijlstra
> <peterz@infradead.org>; Karny, Wyes <Wyes.Karny@amd.com>
> Cc: linux-kernel@vger.kernel.org; Carroll, Lewis <Lewis.Carroll@amd.com>;
> Shenoy, Gautham Ranjal <gautham.shenoy@amd.com>; Narayan, Ananth
> <Ananth.Narayan@amd.com>; Rao, Bharata Bhasker <bharata@amd.com>;
> len.brown@intel.com; x86@kernel.org; tglx@linutronix.de;
> mingo@redhat.com; bp@alien8.de; dave.hansen@linux.intel.com;
> hpa@zytor.com; chang.seok.bae@intel.com; keescook@chromium.org;
> metze@samba.org; zhengqi.arch@bytedance.com; mark.rutland@arm.com
> Subject: Re: [PATCH] x86: Prefer MWAIT over HALT on AMD processors
> 
> On 4/5/22 08:34, Limonciello, Mario wrote:
> >>>         if (!static_cpu_has(X86_FEATURE_ZEN)) {
> >>>                 msr |= ssbd_tif_to_amd_ls_cfg(tifn);
> >>>                 wrmsrl(MSR_AMD64_LS_CFG, msr);
> >>>                 return;
> >>>         }
> >> This seem _bit_ at odds with the commit message (and the AMD SSBD
> >> whitepaper):
> >>
> >>>     Add the necessary synchronization logic for AMD family 17H.
> >> So, is X86_FEATURE_ZEN for family==0x17, or family>=0x17?
> > There are Zen family CPUs and APUs from family 0x19.  Perhaps at the
> > time of the whitepaper there weren't yet though.
> 
> Is this a gap in the documentation, then?  Is there some documentation
> of the availability of SSBD mitigations on family 0x19?

It looks like a misinterpretation of the document.

Notice it mentions in Non-architectural MSRs explicitly:

"some models of family 17h have logic that allow loads to bypass older stores 
but lack the architectural SPEC_CTRL or VIRT_SPEC_CTR"

But that is not for all family 17h nor for family 19h.  You can see earlier in
the document the method to detect presence for SSBD which applies to the
rest of 17h and 19h.  That code in amd_set_core_ssb_state is only used for
one of the mitigation codepaths without MSR support, not for all Zen CPUs.

> 
> Anyway, back to MWAIT...  It would be really great to include the
> assumptions about what X86_FEATURE_ZEN means in the context of this
> patch.  Does this patch *really* mean "Zen microarchitecture" only?

RE: [PATCH] x86: Prefer MWAIT over HALT on AMD processors

[Carroll, Lewis]

[Public]

> -----Original Message-----
> From: Borislav Petkov <bp@alien8.de>
> Sent: Tuesday, April 5, 2022 4:39 PM
> To: Carroll, Lewis <Lewis.Carroll@amd.com>
> Cc: linux-kernel@vger.kernel.org; peterz@infradead.org;
> dave.hansen@linux.intel.com; Karny, Wyes <Wyes.Karny@amd.com>; Limonciello,
> Mario <Mario.Limonciello@amd.com>; Shenoy, Gautham Ranjal
> <gautham.shenoy@amd.com>; Narayan, Ananth <Ananth.Narayan@amd.com>; Rao,
> Bharata Bhasker <bharata@amd.com>; len.brown@intel.com; x86@kernel.org;
> tglx@linutronix.de; mingo@redhat.com; hpa@zytor.com;
> chang.seok.bae@intel.com; keescook@chromium.org; metze@samba.org;
> zhengqi.arch@bytedance.com; mark.rutland@arm.com
> Subject: Re: [PATCH] x86: Prefer MWAIT over HALT on AMD processors
> 
> [CAUTION: External Email]
> 
> On Tue, Apr 05, 2022 at 08:26:53PM +0000, Carroll, Lewis wrote:
> > This happens when:
> >  * User disables global C-states in BIOS
> >  * User disables cpuidle (e.g. idle=off or processor.max_cstate=0)
> >  * Kernel does not have CONFIG_ACPI_PROCESSOR_IDLE
> 
> All three or any one of those?

Just when I thought I was being thorough. Any of the above will block the
cpuidle driver from loading. As will absence of _CST ACPI methods (add that
as a fourth cause). Brings back memories of code comments about a certain
idle driver being created to work around broken BIOSes...

> 
> > Genesis of this patch is customer performance observations.
> 
> Please add that explanation to the changelog - it is important when looking
> back, trying to figure out why this was done.

We will have to see what we can sanitize. The original performance observation
(packet loss in a networking application) led to discovery of lots of cycles
in the various go-to-sleep-via-halt and wake-from-halt-via-IPI functions. Wyes
collected the raw data on the relative idle+wake-up latency and included that
in the commit msg. Think of that delta as the root cause of the performance
regression in this case.

> 
> > Yes. We felt the code more readable with the prefer_mwait_c1_over_halt fn.
> > Hygon CPU init indeed sets X86_FEATURE ZEN.
> > AMD CPU init sets X86_FEATURE_ZEN for family >= 17h (not only 17h).
> 
> Yes, but this new logic you're adding, basically says, use MWAIT on all Zen
> uarch CPUs, right?

Yes we are saying use MWAIT instead of HLT on all known (as of today) Zen
uarch CPUs (AMD >= 17h and Hygon).

> 
> So why not write exactly that?
> 
> The simpler the logic and the clearer the code, the better.
> 
> > Cleanest solution might be a new CPU feature (e.g.
> > X86_PREFER_MWAIT_IDLE) that gets set appropriately, but that would
> > require touching more files.
> 
> Yes, I thought about it too.
> 
> Not really necessary if what I wrote above fits.
> 
> And while you're touching files, pls add that change too:
> 
> ---
> diff --git a/arch/x86/include/asm/cpufeatures.h
> b/arch/x86/include/asm/cpufeatures.h
> index 73e643ae94b6..c1091f78f104 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -219,7 +219,7 @@
>  #define X86_FEATURE_IBRS               ( 7*32+25) /* Indirect Branch
> Restricted Speculation */
>  #define X86_FEATURE_IBPB               ( 7*32+26) /* Indirect Branch
> Prediction Barrier */
>  #define X86_FEATURE_STIBP              ( 7*32+27) /* Single Thread Indirect
> Branch Predictors */
> -#define X86_FEATURE_ZEN                        ( 7*32+28) /* "" CPU is AMD
> family 0x17 or above (Zen) */
> +#define X86_FEATURE_ZEN                        ( 7*32+28) /* "" Set on CPUs
> of the Zen uarch */
>  #define X86_FEATURE_L1TF_PTEINV                ( 7*32+29) /* "" L1TF
> workaround PTE inversion */
>  #define X86_FEATURE_IBRS_ENHANCED      ( 7*32+30) /* Enhanced IBRS */
>  #define X86_FEATURE_MSR_IA32_FEAT_CTL  ( 7*32+31) /* "" MSR IA32_FEAT_CTL
> configured */
> 
> so that dhansen and peterz are not confused anymore. :-)
 
Ack.

> 
> Thx.
> 
> --
> Regards/Gruss,
>     Boris.

RE: [PATCH] x86: Prefer MWAIT over HALT on AMD processors

[Thomas Gleixner]

Mario,

On Tue, Apr 05 2022 at 20:40, Mario Limonciello wrote:

> [Public]

Really useful information that your post is public while some of the
earlier posts in this _public_ thread were marked '[AMD confidential]'.

>> >> This seem _bit_ at odds with the commit message (and the AMD SSBD
>> >> whitepaper):
>> >>
>> >>>     Add the necessary synchronization logic for AMD family 17H.
>> >> So, is X86_FEATURE_ZEN for family==0x17, or family>=0x17?
>> > There are Zen family CPUs and APUs from family 0x19.  Perhaps at the
>> > time of the whitepaper there weren't yet though.
>> 
>> Is this a gap in the documentation, then?  Is there some documentation
>> of the availability of SSBD mitigations on family 0x19?
>
> It looks like a misinterpretation of the document.

Not at all. The document does not mention family 19h at all. So where is
the misinterpretation?

Dave was asking for documentation for family 0x19, right?

> Notice it mentions in Non-architectural MSRs explicitly:
>
> "some models of family 17h have logic that allow loads to bypass older stores 
> but lack the architectural SPEC_CTRL or VIRT_SPEC_CTR"

That's relevant to Dave's question in which way? 

> But that is not for all family 17h nor for family 19h.  You can see earlier in
> the document the method to detect presence for SSBD which applies to the
> rest of 17h and 19h.

We know how to read this document. But this document does not mention
anything else than family 17h. So what are you talking about?

> That code in amd_set_core_ssb_state is only used for one of the
> mitigation codepaths without MSR support, not for all Zen CPUs.

Again, how is that relevant to the legitimate question whether that
document applies to family 17h only or to 17h+ which includes 19h?

We need to make a decison about what X86_FEATURE_ZEN means. Is it that
hard to give an authoritive answer?

Thanks,

        tglx

Re: [PATCH] x86: Prefer MWAIT over HALT on AMD processors

[Wyes Karny]

On 4/5/2022 7:35 PM, Borislav Petkov wrote:
> On Tue, Apr 05, 2022 at 06:30:21PM +0530, Wyes Karny wrote:
>> From: Lewis Caroll <lewis.carroll@amd.com>
>>
>> Currently in the absence of the cpuidle driver (eg: when global
>> C-States are disabled in the BIOS or when cpuidle is driver is not
>> compiled in),
> 
> When does that ever happen?
> 
> Sounds like a very very niche situation to me...
Certain customers prefer to turn off C-States from the BIOS in
low-latency environments.

> 
>> Here we enable MWAIT instruction as the default idle call for AMD
>> Zen processors which support MWAIT. We retain the existing behaviour
>> for older processors which depend on HALT.
> 
> Please use passive voice in your commit message: no "we" or "I", etc,
> and describe your changes in imperative mood.
> 
> Also, pls read section "2) Describe your changes" in
> Documentation/process/submitting-patches.rst for more details.
> 
> Also, see section "Changelog" in
> Documentation/process/maintainer-tip.rst
> 
> Bottom line is: personal pronouns are ambiguous in text, especially with
> so many parties/companies/etc developing the kernel so let's avoid them
> please.

Sure. I'll update this. Thanks.
> 
>>  static int prefer_mwait_c1_over_halt(const struct cpuinfo_x86 *c)
>>  {
>> -	if (c->x86_vendor != X86_VENDOR_INTEL)
>> +	if (!early_mwait_supported(c))
> 
> Isn't it enough to simply do here:
> 
> 	if (cpu_has(c, X86_FEATURE_ZEN))
> 		return 1;
> 
>         if (c->x86_vendor != X86_VENDOR_INTEL)
>                 return 0;
> 
> 	...
> 
> 

If x86_FEATURE_ZEN is set and X86_FEATURE_MWAIT is not set or has X86_BUG_MONITOR
then it won't return correct value.

--
Thanks,
Wyes

Re: [PATCH] x86: Prefer MWAIT over HALT on AMD processors

[Borislav Petkov]

On Wed, Apr 06, 2022 at 11:44:52AM +0530, Wyes Karny wrote:
> If x86_FEATURE_ZEN is set and X86_FEATURE_MWAIT is not set or has
> X86_BUG_MONITOR then it won't return correct value.

Can that ever happen on Zen uarch?

Also, what does this mean: "allow MWAIT early on."?

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86: Prefer MWAIT over HALT on AMD processors

[Borislav Petkov]

On Tue, Apr 05, 2022 at 09:49:27PM +0000, Carroll, Lewis wrote:
> Just when I thought I was being thorough. Any of the above will block the
> cpuidle driver from loading. As will absence of _CST ACPI methods (add that
> as a fourth cause).

Yah, put that all in the text over prefer_mwait_c1_over_halt() pls.

> We will have to see what we can sanitize. The original performance observation
> (packet loss in a networking application) led to discovery of lots of cycles
> in the various go-to-sleep-via-halt and wake-from-halt-via-IPI functions. Wyes
> collected the raw data on the relative idle+wake-up latency and included that
> in the commit msg. Think of that delta as the root cause of the performance
> regression in this case.

You don't have to write novels - just leave enough breadcrumbs so that
people looking at this in the future know *why* this was done.

> Yes we are saying use MWAIT instead of HLT on all known (as of today) Zen
> uarch CPUs (AMD >= 17h and Hygon).

Wyes in his reply from today says that the logic is not that simple so
you folks need to define clearly which cases are we talking about here:

Zen uarch, MWAIT CPUID bit set/clear, MONITOR bug, <other feature bits>,
etc.

Thx.

-- 
Regards/Gruss,
    Boris.

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

RE: [PATCH] x86: Prefer MWAIT over HALT on AMD processors

[Limonciello, Mario]

[Public]

> > [Public]
> 
> Really useful information that your post is public while some of the
> earlier posts in this _public_ thread were marked '[AMD confidential]'.

AMD's mail system adds this tag.  You'll see it in my next reply too.

You might be referring to Lewis' earlier email that had "AMD Official Use Only".
I don't believe anything has been tagged as confidential.

> 
> >> >> This seem _bit_ at odds with the commit message (and the AMD SSBD
> >> >> whitepaper):
> >> >>
> >> >>>     Add the necessary synchronization logic for AMD family 17H.
> >> >> So, is X86_FEATURE_ZEN for family==0x17, or family>=0x17?
> >> > There are Zen family CPUs and APUs from family 0x19.  Perhaps at the
> >> > time of the whitepaper there weren't yet though.
> >>
> >> Is this a gap in the documentation, then?  Is there some documentation
> >> of the availability of SSBD mitigations on family 0x19?
> >
> > It looks like a misinterpretation of the document.
> 
> Not at all. The document does not mention family 19h at all. So where is
> the misinterpretation?
> 
> Dave was asking for documentation for family 0x19, right?
> 
> > Notice it mentions in Non-architectural MSRs explicitly:
> >
> > "some models of family 17h have logic that allow loads to bypass older
> stores
> > but lack the architectural SPEC_CTRL or VIRT_SPEC_CTR"
> 
> That's relevant to Dave's question in which way?

The document is proposing logic that includes saying that if the CPU supports
the architectural MSR then use that.  If it doesn't then do this other stuff.
It specifically mentions that some CPUs in family 17h don't support the architectural
MSR.

The document is *not intended* to be comprehensive list of CPUs that support
It.  Documentation for individual CPUs indicate the availability of the MSR.

> 
> > But that is not for all family 17h nor for family 19h.  You can see earlier in
> > the document the method to detect presence for SSBD which applies to
> the
> > rest of 17h and 19h.
> 
> We know how to read this document. But this document does not mention
> anything else than family 17h. So what are you talking about?
> 
> > That code in amd_set_core_ssb_state is only used for one of the
> > mitigation codepaths without MSR support, not for all Zen CPUs.
> 
> Again, how is that relevant to the legitimate question whether that
> document applies to family 17h only or to 17h+ which includes 19h?

To confirm the availability of the MSR for a particular CPU, you can look
at the PPR for a family 19h CPU.  For example here is family 19h model 01h:
https://www.amd.com/en/support/tech-docs/processor-programming-reference-ppr-for-amd-family-19h-model-01h-revision-b1?msclkid=f5047d01b5b211ec8d619d1385260e2d

> 
> We need to make a decison about what X86_FEATURE_ZEN means. Is it that
> hard to give an authoritive answer?
> 

The comment currently associated with X86_FEATURE_ZEN is appropriate that it
means family 17h or newer.  There is an ask in this thread for clarifying that comment
it's for all CPUs with the Zen microarchitecture, and I believe that will be something
that Wyes will do as a follow up.

Re: [PATCH] x86: Prefer MWAIT over HALT on AMD processors

[Wen Pu]

On 2022/4/5 22:07, Peter Zijlstra wrote:
> On Tue, Apr 05, 2022 at 06:30:21PM +0530, Wyes Karny wrote:
>> +static inline bool early_mwait_supported(const struct cpuinfo_x86 *c)
>> +{
>> +	if (c->x86_vendor == X86_VENDOR_INTEL)
>> +		return true;
>> +
>> +	if (c->x86_vendor == X86_VENDOR_AMD && cpu_has(c, X86_FEATURE_ZEN))
> 
> What about Hygon? For some reason you guys don't co-ordinate and we end
> up getting endless 'make-same' patches, sometimes separated by years :/


Hygon CPU supports MWAIT, so this patch also make sense for Hygon :)
As Dave Hansen and Lewis Caroll mentioned in the following threads,
Hygon has already set the X86_FEATURE_ZEN flag, so it's better to
use X86_FEATURE_ZEN for all Zen uarch CPUs.

>> +		return true;
>> +
>> +	return false;
>> +}

-- 
Regards,
Pu Wen

Re: [PATCH] x86: Prefer MWAIT over HALT on AMD processors

[Dave Hansen]

On 4/6/22 07:23, Limonciello, Mario wrote:
> To confirm the availability of the MSR for a particular CPU, you can look
> at the PPR for a family 19h CPU.  For example here is family 19h model 01h:
> https://www.amd.com/en/support/tech-docs/processor-programming-reference-ppr-for-amd-family-19h-model-01h-revision-b1?msclkid=f5047d01b5b211ec8d619d1385260e2d

I don't want to give you too hard of a time on this.  But, that's not
architecture, that's just telling folks what the implementation is on
*one* CPU model.  Don't get me wrong: these model-specific docs are
great, and I wish Intel published something like that.

But, the code as written depends on behavior for *all* of family 0x19:

>         case 0x17: fallthrough;
>         case 0x19: init_amd_zn(c); break;

So, while the docs for "family 19h model 01h" are *consistent* with this
code, there's also nothing preventing the docs for "family 19h model
02h" from breaking this new MWAIT code.

Now, AMD is full of smart folks that aren't going out of their way to
try and break existing software.  But, actual documentation of the
*architecture* is really preferable to what we have now.  It helps
establish a shared hardware/software contract that keeps both sides honest.

What's missing is something that says:

	All AMD family 0x17 and 0x19 that enumerate support for MWAIT
	also support this "early MWAIT" implementation

That represents a promise from AMD that this can't break in the future.
 It both gives us something unambiguous to write code with *and*
something to help nudge our hardware and microcode colleagues if they
start to do something funny in the future.

Just having that in the changelog would be fine, and a genuine
improvement over what we have now.  Having it one of the
model-independent architecture manuals would be even better.

RE: [PATCH] x86: Prefer MWAIT over HALT on AMD processors

[Limonciello, Mario]

[Public]

> 
> I don't want to give you too hard of a time on this.  But, that's not
> architecture, that's just telling folks what the implementation is on
> *one* CPU model.  Don't get me wrong: these model-specific docs are
> great, and I wish Intel published something like that.
> 
> But, the code as written depends on behavior for *all* of family 0x19:
> 
> >         case 0x17: fallthrough;
> >         case 0x19: init_amd_zn(c); break;
> 
> So, while the docs for "family 19h model 01h" are *consistent* with this
> code, there's also nothing preventing the docs for "family 19h model
> 02h" from breaking this new MWAIT code.
> 
> Now, AMD is full of smart folks that aren't going out of their way to
> try and break existing software.  But, actual documentation of the
> *architecture* is really preferable to what we have now.  It helps
> establish a shared hardware/software contract that keeps both sides honest.

Point taken.  As you're probably aware releasing documentation is never a quick
process, but Lewis and Wyes can take this to the powers that be internally to see
what can be done for the future.

> 
> What's missing is something that says:
> 
> 	All AMD family 0x17 and 0x19 that enumerate support for MWAIT
> 	also support this "early MWAIT" implementation
> 
> That represents a promise from AMD that this can't break in the future.
>  It both gives us something unambiguous to write code with *and*
> something to help nudge our hardware and microcode colleagues if they
> start to do something funny in the future.
> 
> Just having that in the changelog would be fine, and a genuine
> improvement over what we have now.  Having it one of the
> model-independent architecture manuals would be even better.

OK... I think the commit message and/or a comment is doable in the short
term for v2.

RE: [PATCH] x86: Prefer MWAIT over HALT on AMD processors

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Limonciello, Mario
> Sent: Thursday, April 7, 2022 20:24
> To: Dave Hansen <dave.hansen@intel.com>; Thomas Gleixner
> <tglx@linutronix.de>; Peter Zijlstra <peterz@infradead.org>; Karny, Wyes
> <Wyes.Karny@amd.com>
> Cc: linux-kernel@vger.kernel.org; Carroll, Lewis <Lewis.Carroll@amd.com>;
> Shenoy, Gautham Ranjal <gautham.shenoy@amd.com>; Narayan, Ananth
> <Ananth.Narayan@amd.com>; Rao, Bharata Bhasker <bharata@amd.com>;
> len.brown@intel.com; x86@kernel.org; mingo@redhat.com; bp@alien8.de;
> dave.hansen@linux.intel.com; hpa@zytor.com; chang.seok.bae@intel.com;
> keescook@chromium.org; metze@samba.org;
> zhengqi.arch@bytedance.com; mark.rutland@arm.com
> Subject: RE: [PATCH] x86: Prefer MWAIT over HALT on AMD processors
> 
> [Public]
> 
> >
> > I don't want to give you too hard of a time on this.  But, that's not
> > architecture, that's just telling folks what the implementation is on
> > *one* CPU model.  Don't get me wrong: these model-specific docs are
> > great, and I wish Intel published something like that.
> >
> > But, the code as written depends on behavior for *all* of family 0x19:
> >
> > >         case 0x17: fallthrough;
> > >         case 0x19: init_amd_zn(c); break;
> >
> > So, while the docs for "family 19h model 01h" are *consistent* with this
> > code, there's also nothing preventing the docs for "family 19h model
> > 02h" from breaking this new MWAIT code.
> >
> > Now, AMD is full of smart folks that aren't going out of their way to
> > try and break existing software.  But, actual documentation of the
> > *architecture* is really preferable to what we have now.  It helps
> > establish a shared hardware/software contract that keeps both sides
> honest.
> 
> Point taken.  As you're probably aware releasing documentation is never a
> quick
> process, but Lewis and Wyes can take this to the powers that be internally to
> see
> what can be done for the future.

This thread did splinter a bit as it was between speculation control and MWAIT.
So I just wanted to complete the thoughts related to the speculation as that came
up.  Having gone back over what is in the outward facing documentation I did want
to share to you that APM volume 2 page 67-70 does include the details
for speculation control and proper architectural discovery of the MSR.
https://www.amd.com/system/files/TechDocs/24593.pdf

> 
> >
> > What's missing is something that says:
> >
> > 	All AMD family 0x17 and 0x19 that enumerate support for MWAIT
> > 	also support this "early MWAIT" implementation
> >
> > That represents a promise from AMD that this can't break in the future.
> >  It both gives us something unambiguous to write code with *and*
> > something to help nudge our hardware and microcode colleagues if they
> > start to do something funny in the future.
> >
> > Just having that in the changelog would be fine, and a genuine
> > improvement over what we have now.  Having it one of the
> > model-independent architecture manuals would be even better.
> 
> OK... I think the commit message and/or a comment is doable in the short
> term for v2.