[PATCH v1 0/2] x86/cpuid: Use AMD's NullSelectorClearsBase CPUID bit

[PATCH v1 0/2] x86/cpuid: Use AMD's NullSelectorClearsBase CPUID bit

[Jane Malalane]

Jane Malalane (2):
  x86/cpuid: Expose NullSelectorClearsBase CPUID bit to guests
  x86/cpuid: Detect null segment behaviour on Zen2 CPUs

 tools/libs/light/libxl_cpuid.c              |  1 +
 tools/misc/xen-cpuid.c                      |  1 +
 xen/arch/x86/cpu/amd.c                      | 18 ++++++++++++++++++
 xen/arch/x86/cpu/cpu.h                      |  1 +
 xen/arch/x86/cpu/hygon.c                    |  5 +++++
 xen/include/asm-x86/cpufeature.h            |  1 +
 xen/include/public/arch-x86/cpufeatureset.h |  1 +
 7 files changed, 28 insertions(+)

-- 
2.11.0

[PATCH v1 1/2] x86/cpuid: Expose NullSelectorClearsBase CPUID bit to guests

[Jane Malalane]

AMD Zen3 adds the NullSelectorClearsBase bit to indicate that loading
a NULL segment selector zeroes the base and limit fields, as well as
just attributes.

Expose bit to all guests.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jane Malalane <jane.malalane@citrix.com>
---
CC: Wei Liu <wl@xen.org>
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: "Roger Pau Monné" <roger.pau@citrix.com>
CC: Pu Wen <puwen@hygon.cn>
CC: Andy Lutomirski <luto@kernel.org>
---
 tools/libs/light/libxl_cpuid.c              | 1 +
 tools/misc/xen-cpuid.c                      | 1 +
 xen/include/public/arch-x86/cpufeatureset.h | 1 +
 3 files changed, 3 insertions(+)

diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
index b2c673841a..d667c36f31 100644
--- a/tools/libs/light/libxl_cpuid.c
+++ b/tools/libs/light/libxl_cpuid.c
@@ -289,6 +289,7 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str)
         {"svm_pausefilt",0x8000000a, NA, CPUID_REG_EDX, 10,  1},
 
         {"lfence+",      0x80000021, NA, CPUID_REG_EAX,  2,  1},
+        {"nscb",         0x80000021, NA, CPUID_REG_EAX,  6,  1},
 
         {"maxhvleaf",    0x40000000, NA, CPUID_REG_EAX,  0,  8},
 
diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
index 735bcf8f0e..d79e67ecfb 100644
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -185,6 +185,7 @@ static const char *const str_7a1[32] =
 static const char *const str_e21a[32] =
 {
     [ 2] = "lfence+",
+    [ 6] = "nscb",
 };
 
 static const struct {
diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index 380b51b1b3..e5a7c94c78 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -285,6 +285,7 @@ XEN_CPUFEATURE(FSRCS,        10*32+12) /*A  Fast Short REP CMPSB/SCASB */
 
 /* AMD-defined CPU features, CPUID level 0x80000021.eax, word 11 */
 XEN_CPUFEATURE(LFENCE_DISPATCH,    11*32+ 2) /*A  LFENCE always serializing */
+XEN_CPUFEATURE(NSCB,               11*32+ 6) /*A  Null Selector Clears Base */
 
 #endif /* XEN_CPUFEATURE */
 
-- 
2.11.0

Re: [PATCH v1 1/2] x86/cpuid: Expose NullSelectorClearsBase CPUID bit to guests

[Jan Beulich]

On 06.09.2021 14:00, Jane Malalane wrote:
> AMD Zen3 adds the NullSelectorClearsBase bit to indicate that loading
> a NULL segment selector zeroes the base and limit fields, as well as
> just attributes.
> 
> Expose bit to all guests.
> 
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jane Malalane <jane.malalane@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v1 1/2] x86/cpuid: Expose NullSelectorClearsBase CPUID bit to guests

[Andrew Cooper]

On 06/09/2021 13:00, Jane Malalane wrote:
> diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
> index 380b51b1b3..e5a7c94c78 100644
> --- a/xen/include/public/arch-x86/cpufeatureset.h
> +++ b/xen/include/public/arch-x86/cpufeatureset.h
> @@ -285,6 +285,7 @@ XEN_CPUFEATURE(FSRCS,        10*32+12) /*A  Fast Short REP CMPSB/SCASB */
>  
>  /* AMD-defined CPU features, CPUID level 0x80000021.eax, word 11 */
>  XEN_CPUFEATURE(LFENCE_DISPATCH,    11*32+ 2) /*A  LFENCE always serializing */
> +XEN_CPUFEATURE(NSCB,               11*32+ 6) /*A  Null Selector Clears Base */

On second thoughts, I'm tempted to add " (and limit too)" to the comment.

Can be fixed on commit.

~Andrew

[PATCH v1 2/2] x86/cpuid: Detect null segment behaviour on Zen2 CPUs

[Jane Malalane]

Zen2 CPUs actually have this behaviour, but the CPUID bit couldn't be
introduced into Zen2 due to a lack of leaves. So, it was added in a
new leaf in Zen3. Nonetheless, hypervisors can synthesize the CPUID
bit in software.

So, on Zen2 hardware, Xen probes for NSCB (NullSelectorClearsBit) and
synthesizes the bit.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jane Malalane <jane.malalane@citrix.com>
---
CC: Wei Liu <wl@xen.org>
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: "Roger Pau Monné" <roger.pau@citrix.com>
CC: Pu Wen <puwen@hygon.cn>
CC: Andy Lutomirski <luto@kernel.org>
---
 xen/arch/x86/cpu/amd.c           | 18 ++++++++++++++++++
 xen/arch/x86/cpu/cpu.h           |  1 +
 xen/arch/x86/cpu/hygon.c         |  5 +++++
 xen/include/asm-x86/cpufeature.h |  1 +
 4 files changed, 25 insertions(+)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 2260eef3aa..654f82e2cb 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -681,6 +681,19 @@ void amd_init_lfence(struct cpuinfo_x86 *c)
 			  c->x86_capability);
 }
 
+void detect_zen2_null_seg_behaviour(void)
+{
+	uint64_t base;
+
+	wrmsrl(MSR_FS_BASE, 1);
+	asm volatile ( "mov %0, %%fs" :: "rm" (0) );
+	rdmsrl(MSR_FS_BASE, base);
+
+	if (base == 0)
+		setup_force_cpu_cap(X86_FEATURE_NSCB);
+
+}
+
 static void init_amd(struct cpuinfo_x86 *c)
 {
 	u32 l, h;
@@ -731,6 +744,11 @@ static void init_amd(struct cpuinfo_x86 *c)
 	else /* Implicily "== 0x10 || >= 0x12" by being 64bit. */
 		amd_init_lfence(c);
 
+	/* Probe for NSCB on Zen2 CPUs when not virtualised */
+	if (!cpu_has_hypervisor && !cpu_has_nscb && c == &boot_cpu_data &&
+	    c->x86 == 0x17 && c->x86_model >= 30 && c->x86_model <= 0x5f)
+		detect_zen2_null_seg_behaviour();
+
 	/*
 	 * If the user has explicitly chosen to disable Memory Disambiguation
 	 * to mitigiate Speculative Store Bypass, poke the appropriate MSR.
diff --git a/xen/arch/x86/cpu/cpu.h b/xen/arch/x86/cpu/cpu.h
index 1ac3b2867a..0dd1b762ff 100644
--- a/xen/arch/x86/cpu/cpu.h
+++ b/xen/arch/x86/cpu/cpu.h
@@ -21,3 +21,4 @@ extern bool detect_extended_topology(struct cpuinfo_x86 *c);
 void early_init_amd(struct cpuinfo_x86 *c);
 void amd_log_freq(const struct cpuinfo_x86 *c);
 void amd_init_lfence(struct cpuinfo_x86 *c);
+void detect_zen2_null_seg_behaviour(void);
diff --git a/xen/arch/x86/cpu/hygon.c b/xen/arch/x86/cpu/hygon.c
index 67e23c5df9..232edb0c4d 100644
--- a/xen/arch/x86/cpu/hygon.c
+++ b/xen/arch/x86/cpu/hygon.c
@@ -34,6 +34,11 @@ static void init_hygon(struct cpuinfo_x86 *c)
 
 	amd_init_lfence(c);
 
+	/* Probe for NSCB on Zen2 CPUs when not virtualised */
+	if (!cpu_has_hypervisor && !cpu_has_nscb && c == &boot_cpu_data &&
+	    c->x86 == 0x18 && c->x86_model >= 4)
+		detect_zen2_null_seg_behaviour();
+
 	/*
 	 * If the user has explicitly chosen to disable Memory Disambiguation
 	 * to mitigiate Speculative Store Bypass, poke the appropriate MSR.
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index 5f6b83f71c..4faf9bff29 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -146,6 +146,7 @@
 #define cpu_has_cpuid_faulting  boot_cpu_has(X86_FEATURE_CPUID_FAULTING)
 #define cpu_has_aperfmperf      boot_cpu_has(X86_FEATURE_APERFMPERF)
 #define cpu_has_lfence_dispatch boot_cpu_has(X86_FEATURE_LFENCE_DISPATCH)
+#define cpu_has_nscb            boot_cpu_has(X86_FEATURE_NSCB)
 #define cpu_has_xen_lbr         boot_cpu_has(X86_FEATURE_XEN_LBR)
 #define cpu_has_xen_shstk       boot_cpu_has(X86_FEATURE_XEN_SHSTK)
 
-- 
2.11.0

Re: [PATCH v1 2/2] x86/cpuid: Detect null segment behaviour on Zen2 CPUs

[Jan Beulich]

On 06.09.2021 14:00, Jane Malalane wrote:
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -681,6 +681,19 @@ void amd_init_lfence(struct cpuinfo_x86 *c)
>  			  c->x86_capability);
>  }
>  
> +void detect_zen2_null_seg_behaviour(void)

This can in principle be marked __init.

> +{
> +	uint64_t base;
> +
> +	wrmsrl(MSR_FS_BASE, 1);
> +	asm volatile ( "mov %0, %%fs" :: "rm" (0) );

While I don't strictly mind the "m" part of the constraint to remain
there (in the hope for compilers actually to support this), iirc it's
not useful to have when the value is a constant: Last time I checked,
the compiler would not instantiate an anonymous (stack) variable to
fulfill this constraint (as can be seen when dropping the "r" part of
the constraint).

> @@ -731,6 +744,11 @@ static void init_amd(struct cpuinfo_x86 *c)
>  	else /* Implicily "== 0x10 || >= 0x12" by being 64bit. */
>  		amd_init_lfence(c);
>  
> +	/* Probe for NSCB on Zen2 CPUs when not virtualised */
> +	if (!cpu_has_hypervisor && !cpu_has_nscb && c == &boot_cpu_data &&
> +	    c->x86 == 0x17 && c->x86_model >= 30 && c->x86_model <= 0x5f)

DYM 0x30 here? Or 0x1e? In any event 0x5f should be accompanied by
another hex constant. And it would also help if in the description
you said where these bounds as well as ...

> --- a/xen/arch/x86/cpu/hygon.c
> +++ b/xen/arch/x86/cpu/hygon.c
> @@ -34,6 +34,11 @@ static void init_hygon(struct cpuinfo_x86 *c)
>  
>  	amd_init_lfence(c);
>  
> +	/* Probe for NSCB on Zen2 CPUs when not virtualised */
> +	if (!cpu_has_hypervisor && !cpu_has_nscb && c == &boot_cpu_data &&
> +	    c->x86 == 0x18 && c->x86_model >= 4)

... this one come from.

Jan

Re: [PATCH v1 2/2] x86/cpuid: Detect null segment behaviour on Zen2 CPUs

[Andrew Cooper]

On 06/09/2021 16:17, Jan Beulich wrote:
> On 06.09.2021 14:00, Jane Malalane wrote:
>> --- a/xen/arch/x86/cpu/amd.c
>> +++ b/xen/arch/x86/cpu/amd.c
>> @@ -681,6 +681,19 @@ void amd_init_lfence(struct cpuinfo_x86 *c)
>>  			  c->x86_capability);
>>  }
>>  
>> +void detect_zen2_null_seg_behaviour(void)
> This can in principle be marked __init.
>
>> +{
>> +	uint64_t base;
>> +
>> +	wrmsrl(MSR_FS_BASE, 1);
>> +	asm volatile ( "mov %0, %%fs" :: "rm" (0) );
> While I don't strictly mind the "m" part of the constraint to remain
> there (in the hope for compilers actually to support this), iirc it's
> not useful to have when the value is a constant: Last time I checked,
> the compiler would not instantiate an anonymous (stack) variable to
> fulfill this constraint (as can be seen when dropping the "r" part of
> the constraint).

This is "rm" because it is what we use elsewhere in Xen for selectors,
and because it is the correct constraints based on the legal instruction
encodings.

If you want to work around what you perceive to be bugs in compilers
then submit a independent change yourself.

>> @@ -731,6 +744,11 @@ static void init_amd(struct cpuinfo_x86 *c)
>>  	else /* Implicily "== 0x10 || >= 0x12" by being 64bit. */
>>  		amd_init_lfence(c);
>>  
>> +	/* Probe for NSCB on Zen2 CPUs when not virtualised */
>> +	if (!cpu_has_hypervisor && !cpu_has_nscb && c == &boot_cpu_data &&
>> +	    c->x86 == 0x17 && c->x86_model >= 30 && c->x86_model <= 0x5f)
> DYM 0x30 here?

0x30, although it turns out that some of the mobile Zen2 CPUs exceed
0x60 in terms of model number.

As Zen3 changes the family number to 0x19, I'd just drop the upper bound.

>  Or 0x1e? In any event 0x5f should be accompanied by
> another hex constant. And it would also help if in the description
> you said where these bounds

From talking to people at AMD.

>  as well as ...
>
>> --- a/xen/arch/x86/cpu/hygon.c
>> +++ b/xen/arch/x86/cpu/hygon.c
>> @@ -34,6 +34,11 @@ static void init_hygon(struct cpuinfo_x86 *c)
>>  
>>  	amd_init_lfence(c);
>>  
>> +	/* Probe for NSCB on Zen2 CPUs when not virtualised */
>> +	if (!cpu_has_hypervisor && !cpu_has_nscb && c == &boot_cpu_data &&
>> +	    c->x86 == 0x18 && c->x86_model >= 4)
> ... this one come from.

From talking to people at Hygon.

~Andrew

Re: [PATCH v1 2/2] x86/cpuid: Detect null segment behaviour on Zen2 CPUs

[Jan Beulich]

On 06.09.2021 20:07, Andrew Cooper wrote:
> On 06/09/2021 16:17, Jan Beulich wrote:
>> On 06.09.2021 14:00, Jane Malalane wrote:
>>> --- a/xen/arch/x86/cpu/amd.c
>>> +++ b/xen/arch/x86/cpu/amd.c
>>> @@ -681,6 +681,19 @@ void amd_init_lfence(struct cpuinfo_x86 *c)
>>>  			  c->x86_capability);
>>>  }
>>>  
>>> +void detect_zen2_null_seg_behaviour(void)
>> This can in principle be marked __init.
>>
>>> +{
>>> +	uint64_t base;
>>> +
>>> +	wrmsrl(MSR_FS_BASE, 1);
>>> +	asm volatile ( "mov %0, %%fs" :: "rm" (0) );
>> While I don't strictly mind the "m" part of the constraint to remain
>> there (in the hope for compilers actually to support this), iirc it's
>> not useful to have when the value is a constant: Last time I checked,
>> the compiler would not instantiate an anonymous (stack) variable to
>> fulfill this constraint (as can be seen when dropping the "r" part of
>> the constraint).
> 
> This is "rm" because it is what we use elsewhere in Xen for selectors,
> and because it is the correct constraints based on the legal instruction
> encodings.

grep-ing for "%%[defgs]s" reveals:

efi_arch_post_exit_boot(), svm_ctxt_switch_to(), and
do_set_segment_base() all use just "r". This grep has not produced
any use of "rm". What are you talking about?

> If you want to work around what you perceive to be bugs in compilers
> then submit a independent change yourself.

I don't perceive this as a bug; perhaps a desirable feature. I also
did start my response with "While I don't strictly mind the "m"
part ..." - was this not careful enough to indicate I'm not going
to insist on the change, but I'd prefer it to be made?

>>> @@ -731,6 +744,11 @@ static void init_amd(struct cpuinfo_x86 *c)
>>>  	else /* Implicily "== 0x10 || >= 0x12" by being 64bit. */
>>>  		amd_init_lfence(c);
>>>  
>>> +	/* Probe for NSCB on Zen2 CPUs when not virtualised */
>>> +	if (!cpu_has_hypervisor && !cpu_has_nscb && c == &boot_cpu_data &&
>>> +	    c->x86 == 0x17 && c->x86_model >= 30 && c->x86_model <= 0x5f)
>> DYM 0x30 here?
> 
> 0x30, although it turns out that some of the mobile Zen2 CPUs exceed
> 0x60 in terms of model number.
> 
> As Zen3 changes the family number to 0x19, I'd just drop the upper bound.

Minor note: Even if it didn't, the !cpu_has_nscb would also be enough
to avoid the probing there.

>>  Or 0x1e? In any event 0x5f should be accompanied by
>> another hex constant. And it would also help if in the description
>> you said where these bounds
> 
> From talking to people at AMD.
> 
>>  as well as ...
>>
>>> --- a/xen/arch/x86/cpu/hygon.c
>>> +++ b/xen/arch/x86/cpu/hygon.c
>>> @@ -34,6 +34,11 @@ static void init_hygon(struct cpuinfo_x86 *c)
>>>  
>>>  	amd_init_lfence(c);
>>>  
>>> +	/* Probe for NSCB on Zen2 CPUs when not virtualised */
>>> +	if (!cpu_has_hypervisor && !cpu_has_nscb && c == &boot_cpu_data &&
>>> +	    c->x86 == 0x18 && c->x86_model >= 4)
>> ... this one come from.
> 
> From talking to people at Hygon.

Fair enough, but imo this wants mentioning in the description.

Jan

Re: [PATCH v1 2/2] x86/cpuid: Detect null segment behaviour on Zen2 CPUs

[Andrew Cooper]

On 07/09/2021 07:09, Jan Beulich wrote:
> On 06.09.2021 20:07, Andrew Cooper wrote:
>> On 06/09/2021 16:17, Jan Beulich wrote:
>>> On 06.09.2021 14:00, Jane Malalane wrote:
>>>> --- a/xen/arch/x86/cpu/amd.c
>>>> +++ b/xen/arch/x86/cpu/amd.c
>>>> @@ -681,6 +681,19 @@ void amd_init_lfence(struct cpuinfo_x86 *c)
>>>>  			  c->x86_capability);
>>>>  }
>>>>  
>>>> +void detect_zen2_null_seg_behaviour(void)
>>> This can in principle be marked __init.
>>>
>>>> +{
>>>> +	uint64_t base;
>>>> +
>>>> +	wrmsrl(MSR_FS_BASE, 1);
>>>> +	asm volatile ( "mov %0, %%fs" :: "rm" (0) );
>>> While I don't strictly mind the "m" part of the constraint to remain
>>> there (in the hope for compilers actually to support this), iirc it's
>>> not useful to have when the value is a constant: Last time I checked,
>>> the compiler would not instantiate an anonymous (stack) variable to
>>> fulfill this constraint (as can be seen when dropping the "r" part of
>>> the constraint).
>> This is "rm" because it is what we use elsewhere in Xen for selectors,
>> and because it is the correct constraints based on the legal instruction
>> encodings.
> grep-ing for "%%[defgs]s" reveals:
>
> efi_arch_post_exit_boot(), svm_ctxt_switch_to(),

These are writing multiple selectors in one go, and a register
constraint is the only sane option.

> and do_set_segment_base() all use just "r".

I had missed this one.

>  This grep has not produced
> any use of "rm". What are you talking about?

TRY_LOAD_SEG(), pv_emul_read_descriptor() for both lar and lsl,
do_double_fault() for another lsl, lldt(), ltr().

So ok - not everything, but most.

>
>> If you want to work around what you perceive to be bugs in compilers
>> then submit a independent change yourself.
> I don't perceive this as a bug; perhaps a desirable feature. I also
> did start my response with "While I don't strictly mind the "m"
> part ..." - was this not careful enough to indicate I'm not going
> to insist on the change, but I'd prefer it to be made?

No, because a maintainer saying "I'd prefer this to be changed" is still
an instruction to the submitter to make the change.

But the request is inappropriate.  "Last time I checked, the compiler
would" presumably means you've checked GCC and not Clang, and therefore
any conclusions about the behaviour are incomplete.

Unless there is a real concrete compiler bug to work around, "rm" is the
appropriate constraint to use, all other things being equal.  If the
complier is merely doing something dumb with the flexibility it has been
permitted, then fix the compiler and the problem will resolve itself the
proper way.

>
>>>> @@ -731,6 +744,11 @@ static void init_amd(struct cpuinfo_x86 *c)
>>>>  	else /* Implicily "== 0x10 || >= 0x12" by being 64bit. */
>>>>  		amd_init_lfence(c);
>>>>  
>>>> +	/* Probe for NSCB on Zen2 CPUs when not virtualised */
>>>> +	if (!cpu_has_hypervisor && !cpu_has_nscb && c == &boot_cpu_data &&
>>>> +	    c->x86 == 0x17 && c->x86_model >= 30 && c->x86_model <= 0x5f)
>>> DYM 0x30 here?
>> 0x30, although it turns out that some of the mobile Zen2 CPUs exceed
>> 0x60 in terms of model number.
>>
>> As Zen3 changes the family number to 0x19, I'd just drop the upper bound.
> Minor note: Even if it didn't, the !cpu_has_nscb would also be enough
> to avoid the probing there.

There is actually a problem.  From a non-AMD source, I've found the
Sucor Z+ CPU which is a Fam17h Model 0x50 Zen1.

So instead I'm going to recommend dropping all model checks and just
keeping the family checks.  This will extend the probe function to Zen1
too, but it is once on boot, trivial in terms of complexity, and really
not worth the time/effort it has taken to discover that the model list
wasn't correct to start with.

~Andrew

Re: [PATCH v1 2/2] x86/cpuid: Detect null segment behaviour on Zen2 CPUs

[Jan Beulich]

On 07.09.2021 15:27, Andrew Cooper wrote:
> On 07/09/2021 07:09, Jan Beulich wrote:
>> On 06.09.2021 20:07, Andrew Cooper wrote:
>>> On 06/09/2021 16:17, Jan Beulich wrote:
>>>> On 06.09.2021 14:00, Jane Malalane wrote:
>>>>> --- a/xen/arch/x86/cpu/amd.c
>>>>> +++ b/xen/arch/x86/cpu/amd.c
>>>>> @@ -681,6 +681,19 @@ void amd_init_lfence(struct cpuinfo_x86 *c)
>>>>>  			  c->x86_capability);
>>>>>  }
>>>>>  
>>>>> +void detect_zen2_null_seg_behaviour(void)
>>>> This can in principle be marked __init.
>>>>
>>>>> +{
>>>>> +	uint64_t base;
>>>>> +
>>>>> +	wrmsrl(MSR_FS_BASE, 1);
>>>>> +	asm volatile ( "mov %0, %%fs" :: "rm" (0) );
>>>> While I don't strictly mind the "m" part of the constraint to remain
>>>> there (in the hope for compilers actually to support this), iirc it's
>>>> not useful to have when the value is a constant: Last time I checked,
>>>> the compiler would not instantiate an anonymous (stack) variable to
>>>> fulfill this constraint (as can be seen when dropping the "r" part of
>>>> the constraint).
>>> This is "rm" because it is what we use elsewhere in Xen for selectors,
>>> and because it is the correct constraints based on the legal instruction
>>> encodings.
>> grep-ing for "%%[defgs]s" reveals:
>>
>> efi_arch_post_exit_boot(), svm_ctxt_switch_to(),
> 
> These are writing multiple selectors in one go, and a register
> constraint is the only sane option.
> 
>> and do_set_segment_base() all use just "r".
> 
> I had missed this one.
> 
>>  This grep has not produced
>> any use of "rm". What are you talking about?
> 
> TRY_LOAD_SEG(), pv_emul_read_descriptor() for both lar and lsl,
> do_double_fault() for another lsl, lldt(), ltr().

TRY_LOAD_SEG() and pv_emul_read_descriptor() don't pass constants
as asm() argument values. do_double_fault()'s use of lsl is indeed
an example matching the pattern here. lldt() and ltr() are generic
inline helpers, so validly allow for both because they should not
make assumptions on what the caller passes. Plus "m" there is okay,
because if the caller passes a constant there will be a named
variable (the function parameter), i.e. the compiler does not need
to instantiate any anonymous one.

> So ok - not everything, but most.
> 
>>
>>> If you want to work around what you perceive to be bugs in compilers
>>> then submit a independent change yourself.
>> I don't perceive this as a bug; perhaps a desirable feature. I also
>> did start my response with "While I don't strictly mind the "m"
>> part ..." - was this not careful enough to indicate I'm not going
>> to insist on the change, but I'd prefer it to be made?
> 
> No, because a maintainer saying "I'd prefer this to be changed" is still
> an instruction to the submitter to make the change.

It was a request to _consider_ dropping the m part, yes. But (see
below) now that you've forced me to re-check (I presume you didn't
check yourself, or else I would expect you would have drawn the same
conclusion as I did), I actually feel stronger about this needing
adjustment.

> But the request is inappropriate.  "Last time I checked, the compiler
> would" presumably means you've checked GCC and not Clang, and therefore
> any conclusions about the behaviour are incomplete.

Not really, no. IIRC I did check the version of clang that I have easy
access to. (For gcc I've just now re-checked with 10.x and 11.x.)

> Unless there is a real concrete compiler bug to work around, "rm" is the
> appropriate constraint to use, all other things being equal.  If the
> complier is merely doing something dumb with the flexibility it has been
> permitted, then fix the compiler and the problem will resolve itself the
> proper way.

I disagree. When an asm() constraint permits multiple kinds of values,
dropping one or more of the alternatives should IMO still yield valid
(perhaps sub-optimal) code (IOW every one of the supplied kinds should
be valid). The issue here is that it is not spelled out clearly
whether something like "m" (0) is actually legal. The error messages,
however, suggest this is intended to be illegal: gcc says "memory
input ... is not directly addressable", whereas clang says "invalid
lvalue in asm input for constraint 'm'".

IOW I do think the one case of LSL in do_double_fault() does need
adjusting.

Jan

[PATCH v2 2/2] x86/cpuid: Detect null segment behaviour on Zen2 CPUs

[Jane Malalane]

Zen2 CPUs actually have this behaviour, but the CPUID bit couldn't be
introduced into Zen2 due to a lack of leaves. So, it was added in a
new leaf in Zen3. Nonetheless, hypervisors can synthesize the CPUID
bit in software.

So, Xen probes for NSCB (NullSelectorClearsBit) and
synthesizes the bit, if the behaviour is present.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jane Malalane <jane.malalane@citrix.com>
---
CC: Wei Liu <wl@xen.org>
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: "Roger Pau Monné" <roger.pau@citrix.com>
CC: Pu Wen <puwen@hygon.cn>
CC: Andy Lutomirski <luto@kernel.org>

v2:
 * Mark detect_zen2_null_seg_behaviour() with __init
 * Remove model checks
---
 xen/arch/x86/cpu/amd.c           | 18 ++++++++++++++++++
 xen/arch/x86/cpu/cpu.h           |  1 +
 xen/arch/x86/cpu/hygon.c         |  5 +++++
 xen/include/asm-x86/cpufeature.h |  1 +
 4 files changed, 25 insertions(+)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 2260eef3aa..cb12861481 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -681,6 +681,19 @@ void amd_init_lfence(struct cpuinfo_x86 *c)
 			  c->x86_capability);
 }
 
+void __init detect_zen2_null_seg_behaviour(void)
+{
+	uint64_t base;
+
+	wrmsrl(MSR_FS_BASE, 1);
+	asm volatile ( "mov %0, %%fs" :: "rm" (0) );
+	rdmsrl(MSR_FS_BASE, base);
+
+	if (base == 0)
+		setup_force_cpu_cap(X86_FEATURE_NSCB);
+
+}
+
 static void init_amd(struct cpuinfo_x86 *c)
 {
 	u32 l, h;
@@ -731,6 +744,11 @@ static void init_amd(struct cpuinfo_x86 *c)
 	else /* Implicily "== 0x10 || >= 0x12" by being 64bit. */
 		amd_init_lfence(c);
 
+	/* Probe for NSCB on Zen2 CPUs when not virtualised */
+	if (!cpu_has_hypervisor && !cpu_has_nscb && c == &boot_cpu_data &&
+	    c->x86 == 0x17)
+		detect_zen2_null_seg_behaviour();
+
 	/*
 	 * If the user has explicitly chosen to disable Memory Disambiguation
 	 * to mitigiate Speculative Store Bypass, poke the appropriate MSR.
diff --git a/xen/arch/x86/cpu/cpu.h b/xen/arch/x86/cpu/cpu.h
index 1ac3b2867a..0dd1b762ff 100644
--- a/xen/arch/x86/cpu/cpu.h
+++ b/xen/arch/x86/cpu/cpu.h
@@ -21,3 +21,4 @@ extern bool detect_extended_topology(struct cpuinfo_x86 *c);
 void early_init_amd(struct cpuinfo_x86 *c);
 void amd_log_freq(const struct cpuinfo_x86 *c);
 void amd_init_lfence(struct cpuinfo_x86 *c);
+void detect_zen2_null_seg_behaviour(void);
diff --git a/xen/arch/x86/cpu/hygon.c b/xen/arch/x86/cpu/hygon.c
index 67e23c5df9..d7a04af2bb 100644
--- a/xen/arch/x86/cpu/hygon.c
+++ b/xen/arch/x86/cpu/hygon.c
@@ -34,6 +34,11 @@ static void init_hygon(struct cpuinfo_x86 *c)
 
 	amd_init_lfence(c);
 
+	/* Probe for NSCB on Zen2 CPUs when not virtualised */
+	if (!cpu_has_hypervisor && !cpu_has_nscb && c == &boot_cpu_data &&
+	    c->x86 == 0x18)
+		detect_zen2_null_seg_behaviour();
+
 	/*
 	 * If the user has explicitly chosen to disable Memory Disambiguation
 	 * to mitigiate Speculative Store Bypass, poke the appropriate MSR.
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index 5f6b83f71c..4faf9bff29 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -146,6 +146,7 @@
 #define cpu_has_cpuid_faulting  boot_cpu_has(X86_FEATURE_CPUID_FAULTING)
 #define cpu_has_aperfmperf      boot_cpu_has(X86_FEATURE_APERFMPERF)
 #define cpu_has_lfence_dispatch boot_cpu_has(X86_FEATURE_LFENCE_DISPATCH)
+#define cpu_has_nscb            boot_cpu_has(X86_FEATURE_NSCB)
 #define cpu_has_xen_lbr         boot_cpu_has(X86_FEATURE_XEN_LBR)
 #define cpu_has_xen_shstk       boot_cpu_has(X86_FEATURE_XEN_SHSTK)
 
-- 
2.11.0

Re: [PATCH v2 2/2] x86/cpuid: Detect null segment behaviour on Zen2 CPUs

[Jan Beulich]

On 08.09.2021 10:19, Jane Malalane wrote:
> Zen2 CPUs actually have this behaviour, but the CPUID bit couldn't be
> introduced into Zen2 due to a lack of leaves. So, it was added in a
> new leaf in Zen3. Nonetheless, hypervisors can synthesize the CPUID
> bit in software.

Considering the prior model checks, I understand this isn't all Zen2s?
No matter what the answer, I'd like to ask that the first sentence
start with either "All" or "Some" (or something along these lines).
Which is of course fine to insert while committing, so not need to
send a v3.

> So, Xen probes for NSCB (NullSelectorClearsBit) and
> synthesizes the bit, if the behaviour is present.
> 
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jane Malalane <jane.malalane@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v2 2/2] x86/cpuid: Detect null segment behaviour on Zen2 CPUs

[Andrew Cooper]

On 08/09/2021 13:08, Jan Beulich wrote:
> On 08.09.2021 10:19, Jane Malalane wrote:
>> Zen2 CPUs actually have this behaviour, but the CPUID bit couldn't be
>> introduced into Zen2 due to a lack of leaves. So, it was added in a
>> new leaf in Zen3. Nonetheless, hypervisors can synthesize the CPUID
>> bit in software.
> Considering the prior model checks, I understand this isn't all Zen2s?
> No matter what the answer, I'd like to ask that the first sentence
> start with either "All" or "Some" (or something along these lines).
> Which is of course fine to insert while committing, so not need to
> send a v3.

All Zen2's have this behaviour.

The model checks were trying to avoid running the probe on Zen1, but
"model >= 0x30 && mode != 0x50" is error prone.

There are no faults in the probe, and its fast, so running on all Zen1/2
is better than hoping that we got the model list correct.

~Andrew