... and rdmsr() while here.

Most of these accesses are in fastpaths and do not need serialising behaviour,
but the write side is serialising on all Intel hardware as well as older AMD
hardware.

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/domain.c               | 10 +++++-----
 xen/arch/x86/hvm/vmx/vmx.c          |  4 ++--
 xen/arch/x86/include/asm/fsgsbase.h | 30 +++++++++--------------------
 3 files changed, 16 insertions(+), 28 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 3015aac4adbc..2a9bb87729c8 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1824,9 +1824,9 @@ static void load_segments(struct vcpu *n)
         }
         else
         {
-            wrmsrl(MSR_FS_BASE, n->arch.pv.fs_base);
-            wrmsrl(MSR_GS_BASE, gsb);
-            wrmsrl(MSR_SHADOW_GS_BASE, gss);
+            wrmsrns(MSR_FS_BASE, n->arch.pv.fs_base);
+            wrmsrns(MSR_GS_BASE, gsb);
+            wrmsrns(MSR_SHADOW_GS_BASE, gss);
         }
     }
 
@@ -1951,8 +1951,8 @@ static void save_segments(struct vcpu *v)
         }
         else
         {
-            rdmsrl(MSR_FS_BASE, fs_base);
-            rdmsrl(MSR_GS_BASE, gs_base);
+            fs_base = rdmsr(MSR_FS_BASE);
+            gs_base = rdmsr(MSR_GS_BASE);
         }
 
         v->arch.pv.fs_base = fs_base;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index f97a7746531a..9ba140c4811c 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2733,8 +2733,8 @@ static uint64_t cf_check vmx_get_reg(struct vcpu *v, unsigned int reg)
     case MSR_SHADOW_GS_BASE:
         if ( v != curr )
             return v->arch.hvm.vmx.shadow_gs;
-        rdmsrl(MSR_SHADOW_GS_BASE, val);
-        return val;
+        else
+            return rdmsr(MSR_SHADOW_GS_BASE);
     }
 
     /* Logic which maybe requires remote VMCS acquisition. */
diff --git a/xen/arch/x86/include/asm/fsgsbase.h b/xen/arch/x86/include/asm/fsgsbase.h
index 90d116f3bb54..7a0b623cca8f 100644
--- a/xen/arch/x86/include/asm/fsgsbase.h
+++ b/xen/arch/x86/include/asm/fsgsbase.h
@@ -63,38 +63,26 @@ static inline void __wrgskern(unsigned long base)
 
 static inline unsigned long read_fs_base(void)
 {
-    unsigned long base;
-
     if ( read_cr4() & X86_CR4_FSGSBASE )
         return __rdfsbase();
-
-    rdmsrl(MSR_FS_BASE, base);
-
-    return base;
+    else
+        return rdmsr(MSR_FS_BASE);
 }
 
 static inline unsigned long read_gs_base(void)
 {
-    unsigned long base;
-
     if ( read_cr4() & X86_CR4_FSGSBASE )
         return __rdgsbase();
-
-    rdmsrl(MSR_GS_BASE, base);
-
-    return base;
+    else
+        return rdmsr(MSR_GS_BASE);
 }
 
 static inline unsigned long read_gs_shadow(void)
 {
-    unsigned long base;
-
     if ( read_cr4() & X86_CR4_FSGSBASE )
         return __rdgskern();
-
-    rdmsrl(MSR_SHADOW_GS_BASE, base);
-
-    return base;
+    else
+        return rdmsr(MSR_SHADOW_GS_BASE);
 }
 
 static inline void write_fs_base(unsigned long base)
@@ -102,7 +90,7 @@ static inline void write_fs_base(unsigned long base)
     if ( read_cr4() & X86_CR4_FSGSBASE )
         __wrfsbase(base);
     else
-        wrmsrl(MSR_FS_BASE, base);
+        wrmsrns(MSR_FS_BASE, base);
 }
 
 static inline void write_gs_base(unsigned long base)
@@ -110,7 +98,7 @@ static inline void write_gs_base(unsigned long base)
     if ( read_cr4() & X86_CR4_FSGSBASE )
         __wrgsbase(base);
     else
-        wrmsrl(MSR_GS_BASE, base);
+        wrmsrns(MSR_GS_BASE, base);
 }
 
 static inline void write_gs_shadow(unsigned long base)
@@ -118,7 +106,7 @@ static inline void write_gs_shadow(unsigned long base)
     if ( read_cr4() & X86_CR4_FSGSBASE )
         __wrgskern(base);
     else
-        wrmsrl(MSR_SHADOW_GS_BASE, base);
+        wrmsrns(MSR_SHADOW_GS_BASE, base);
 }
 
 #endif /* X86_FSGSBASE_H */
-- 
2.39.5

On 15.08.2025 22:41, Andrew Cooper wrote:
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2733,8 +2733,8 @@ static uint64_t cf_check vmx_get_reg(struct vcpu *v, unsigned int reg)
>      case MSR_SHADOW_GS_BASE:
>          if ( v != curr )
>              return v->arch.hvm.vmx.shadow_gs;
> -        rdmsrl(MSR_SHADOW_GS_BASE, val);
> -        return val;
> +        else
> +            return rdmsr(MSR_SHADOW_GS_BASE);
>      }

Here and below, can we please do without the pointless "else"? Strictly
speaking in Misra's terms that's "dead code" (things working identically
without), and I'm quite happy that I can now use this argument to
support my personal antipathy to this style of coding. Or else use the
conditional operator in such cases (where applicable).

Jan

On 19/08/2025 2:09 pm, Jan Beulich wrote:
> On 15.08.2025 22:41, Andrew Cooper wrote:
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -2733,8 +2733,8 @@ static uint64_t cf_check vmx_get_reg(struct vcpu *v, unsigned int reg)
>>      case MSR_SHADOW_GS_BASE:
>>          if ( v != curr )
>>              return v->arch.hvm.vmx.shadow_gs;
>> -        rdmsrl(MSR_SHADOW_GS_BASE, val);
>> -        return val;
>> +        else
>> +            return rdmsr(MSR_SHADOW_GS_BASE);
>>      }
> Here and below, can we please do without the pointless "else"? Strictly
> speaking in Misra's terms that's "dead code" (things working identically
> without), and I'm quite happy that I can now use this argument to
> support my personal antipathy to this style of coding. Or else use the
> conditional operator in such cases (where applicable).

No.  I have always, and will always prioritise readability first and
foremost.

I do not agree with your interpretation of MISRA in this case.

~Andrew

On 21.08.2025 22:49, Andrew Cooper wrote:
> On 19/08/2025 2:09 pm, Jan Beulich wrote:
>> On 15.08.2025 22:41, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -2733,8 +2733,8 @@ static uint64_t cf_check vmx_get_reg(struct vcpu *v, unsigned int reg)
>>>      case MSR_SHADOW_GS_BASE:
>>>          if ( v != curr )
>>>              return v->arch.hvm.vmx.shadow_gs;
>>> -        rdmsrl(MSR_SHADOW_GS_BASE, val);
>>> -        return val;
>>> +        else
>>> +            return rdmsr(MSR_SHADOW_GS_BASE);
>>>      }
>> Here and below, can we please do without the pointless "else"? Strictly
>> speaking in Misra's terms that's "dead code" (things working identically
>> without), and I'm quite happy that I can now use this argument to
>> support my personal antipathy to this style of coding. Or else use the
>> conditional operator in such cases (where applicable).
> 
> No.  I have always, and will always prioritise readability first and
> foremost.

But my preference is precisely because of readability. The excess "else"
gives a wrong impression to the reader, when not looking closely enough.
(And I [now] expect you might say the opposite.)

> I do not agree with your interpretation of MISRA in this case.

Something to discuss in a broader group then, I suppose.

Jan