Right now they're inline in {read,write}_gs_shadow(), but we're going to need
to use these elsewhere to support FRED.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/include/asm/fsgsbase.h | 36 ++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/xen/arch/x86/include/asm/fsgsbase.h b/xen/arch/x86/include/asm/fsgsbase.h
index 03e6a85d31ce..90d116f3bb54 100644
--- a/xen/arch/x86/include/asm/fsgsbase.h
+++ b/xen/arch/x86/include/asm/fsgsbase.h
@@ -32,6 +32,17 @@ static inline unsigned long __rdgsbase(void)
     return base;
 }
 
+static inline unsigned long __rdgskern(void)
+{
+    unsigned long base;
+
+    asm_inline volatile ( "swapgs\n\t"
+                          "rdgsbase %0\n\t"
+                          "swapgs" : "=r" (base) );
+
+    return base;
+}
+
 static inline void __wrfsbase(unsigned long base)
 {
     asm volatile ( "wrfsbase %0" :: "r" (base) );
@@ -42,6 +53,14 @@ static inline void __wrgsbase(unsigned long base)
     asm volatile ( "wrgsbase %0" :: "r" (base) );
 }
 
+static inline void __wrgskern(unsigned long base)
+{
+    asm_inline volatile ( "swapgs\n\t"
+                          "wrgsbase %0\n\t"
+                          "swapgs"
+                          :: "r" (base) );
+}
+
 static inline unsigned long read_fs_base(void)
 {
     unsigned long base;
@@ -71,13 +90,9 @@ static inline unsigned long read_gs_shadow(void)
     unsigned long base;
 
     if ( read_cr4() & X86_CR4_FSGSBASE )
-    {
-        asm volatile ( "swapgs" );
-        base = __rdgsbase();
-        asm volatile ( "swapgs" );
-    }
-    else
-        rdmsrl(MSR_SHADOW_GS_BASE, base);
+        return __rdgskern();
+
+    rdmsrl(MSR_SHADOW_GS_BASE, base);
 
     return base;
 }
@@ -101,12 +116,7 @@ static inline void write_gs_base(unsigned long base)
 static inline void write_gs_shadow(unsigned long base)
 {
     if ( read_cr4() & X86_CR4_FSGSBASE )
-    {
-        asm volatile ( "swapgs\n\t"
-                       "wrgsbase %0\n\t"
-                       "swapgs"
-                       :: "r" (base) );
-    }
+        __wrgskern(base);
     else
         wrmsrl(MSR_SHADOW_GS_BASE, base);
 }
-- 
2.39.5

On 15.08.2025 22:41, Andrew Cooper wrote:
> Right now they're inline in {read,write}_gs_shadow(), but we're going to need
> to use these elsewhere to support FRED.

But why "kern"? We're not dealing with GS in kernel / user terms, but in
real / shadow ones. I'm also not quite happy with the double leading
underscores, fwiw.

As to using them elsewhere to support FRED: How can we when they use SWAPGS,
which isn't available under FRED?

> --- a/xen/arch/x86/include/asm/fsgsbase.h
> +++ b/xen/arch/x86/include/asm/fsgsbase.h
> @@ -32,6 +32,17 @@ static inline unsigned long __rdgsbase(void)
>      return base;
>  }
>  
> +static inline unsigned long __rdgskern(void)
> +{
> +    unsigned long base;
> +
> +    asm_inline volatile ( "swapgs\n\t"
> +                          "rdgsbase %0\n\t"
> +                          "swapgs" : "=r" (base) );

Again strictly speaking "=&r", if already you open-code rdgsbase() now.

Jan

On 19/08/2025 2:01 pm, Jan Beulich wrote:
> On 15.08.2025 22:41, Andrew Cooper wrote:
>> Right now they're inline in {read,write}_gs_shadow(), but we're going to need
>> to use these elsewhere to support FRED.
> But why "kern"? We're not dealing with GS in kernel / user terms, but in
> real / shadow ones.

Because it's a common name that also has the property of aligning nicely
when used beside GS_BASE.

But fine, I'll rename it.

>  I'm also not quite happy with the double leading
> underscores, fwiw.

Consistency with the similar logic.

>
>> --- a/xen/arch/x86/include/asm/fsgsbase.h
>> +++ b/xen/arch/x86/include/asm/fsgsbase.h
>> @@ -32,6 +32,17 @@ static inline unsigned long __rdgsbase(void)
>>      return base;
>>  }
>>  
>> +static inline unsigned long __rdgskern(void)
>> +{
>> +    unsigned long base;
>> +
>> +    asm_inline volatile ( "swapgs\n\t"
>> +                          "rdgsbase %0\n\t"
>> +                          "swapgs" : "=r" (base) );
> Again strictly speaking "=&r", if already you open-code rdgsbase() now.

As before, why?   There are no inputs to be clobbered, early or otherwise.

~Andrew

On 21.08.2025 22:20, Andrew Cooper wrote:
> On 19/08/2025 2:01 pm, Jan Beulich wrote:
>> On 15.08.2025 22:41, Andrew Cooper wrote:
>>> Right now they're inline in {read,write}_gs_shadow(), but we're going to need
>>> to use these elsewhere to support FRED.
>> But why "kern"? We're not dealing with GS in kernel / user terms, but in
>> real / shadow ones.
> 
> Because it's a common name that also has the property of aligning nicely
> when used beside GS_BASE.
> 
> But fine, I'll rename it.
> 
>>  I'm also not quite happy with the double leading
>> underscores, fwiw.
> 
> Consistency with the similar logic.

Yet making for more changes later, once we mean to strictly follow the
respective Misra rule.

>>> --- a/xen/arch/x86/include/asm/fsgsbase.h
>>> +++ b/xen/arch/x86/include/asm/fsgsbase.h
>>> @@ -32,6 +32,17 @@ static inline unsigned long __rdgsbase(void)
>>>      return base;
>>>  }
>>>  
>>> +static inline unsigned long __rdgskern(void)
>>> +{
>>> +    unsigned long base;
>>> +
>>> +    asm_inline volatile ( "swapgs\n\t"
>>> +                          "rdgsbase %0\n\t"
>>> +                          "swapgs" : "=r" (base) );
>> Again strictly speaking "=&r", if already you open-code rdgsbase() now.
> 
> As before, why?   There are no inputs to be clobbered, early or otherwise.

Hence why I said "strictly speaking". Inputs or not imo doesn't matter in
such a decision; that merely makes using the early-clobber form benign.
The sole criteria by which I think one ought to go is whether a register
is altered solely by the last (often: only) insn. IOW it's again more to
prevent setting a bad precedent. Here things are easy to see, so a
hypothetical future change isn't at much of a risk of breaking things. The
more complex asm()-s get, the easier it is to overlook a missing early-
clobber when making some change.

Jan