From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 1/3] x86: fix FS/GS base handling when using the fsgsbase feature Date: Wed, 5 Feb 2014 15:50:59 +0000 Message-ID: <52F25DE3.4070306@citrix.com> References: <52F25D4202000078001196AB@nat28.tlf.novell.com> <52F25E3F02000078001196E4@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4569693584749241606==" Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WB4kl-00035X-NJ for xen-devel@lists.xenproject.org; Wed, 05 Feb 2014 15:51:08 +0000 In-Reply-To: <52F25E3F02000078001196E4@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: George Dunlap , xen-devel , Keir Fraser List-Id: xen-devel@lists.xenproject.org --===============4569693584749241606== Content-Type: multipart/alternative; boundary="------------020507090806000700020401" --------------020507090806000700020401 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit On 05/02/14 14:52, Jan Beulich wrote: > In that case, due to the respective instructions not being privileged, > we can't rely on our in-memory data to always be correct: While the > guest is running, it may change without us knowing about it. Therefore > we need to > - read the correct values from hardware during context switch out > (save_segments()) > - read the correct values from hardware during RDMSR emulation > - update in-memory values during guest mode change > (toggle_guest_mode()) > > For completeness/consistency, WRMSR emulation is also being switched > to use wr[fg]sbase(). > > Signed-off-by: Jan Beulich Reviewed-by: Andrew Cooper > > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -1251,6 +1251,15 @@ static void save_segments(struct vcpu *v > regs->fs = read_segment_register(fs); > regs->gs = read_segment_register(gs); > > + if ( cpu_has_fsgsbase && !is_pv_32bit_vcpu(v) ) > + { > + v->arch.pv_vcpu.fs_base = __rdfsbase(); > + if ( v->arch.flags & TF_kernel_mode ) > + v->arch.pv_vcpu.gs_base_kernel = __rdgsbase(); > + else > + v->arch.pv_vcpu.gs_base_user = __rdgsbase(); > + } > + > if ( regs->ds ) > dirty_segment_mask |= DIRTY_DS; > > --- a/xen/arch/x86/traps.c > +++ b/xen/arch/x86/traps.c > @@ -2382,15 +2382,13 @@ static int emulate_privileged_op(struct > case MSR_FS_BASE: > if ( is_pv_32on64_vcpu(v) ) > goto fail; > - if ( wrmsr_safe(MSR_FS_BASE, msr_content) ) > - goto fail; > + wrfsbase(msr_content); > v->arch.pv_vcpu.fs_base = msr_content; > break; > case MSR_GS_BASE: > if ( is_pv_32on64_vcpu(v) ) > goto fail; > - if ( wrmsr_safe(MSR_GS_BASE, msr_content) ) > - goto fail; > + wrgsbase(msr_content); > v->arch.pv_vcpu.gs_base_kernel = msr_content; > break; > case MSR_SHADOW_GS_BASE: > @@ -2535,15 +2533,14 @@ static int emulate_privileged_op(struct > case MSR_FS_BASE: > if ( is_pv_32on64_vcpu(v) ) > goto fail; > - regs->eax = v->arch.pv_vcpu.fs_base & 0xFFFFFFFFUL; > - regs->edx = v->arch.pv_vcpu.fs_base >> 32; > - break; > + val = cpu_has_fsgsbase ? __rdfsbase() : v->arch.pv_vcpu.fs_base; > + goto rdmsr_writeback; > case MSR_GS_BASE: > if ( is_pv_32on64_vcpu(v) ) > goto fail; > - regs->eax = v->arch.pv_vcpu.gs_base_kernel & 0xFFFFFFFFUL; > - regs->edx = v->arch.pv_vcpu.gs_base_kernel >> 32; > - break; > + val = cpu_has_fsgsbase ? __rdgsbase() > + : v->arch.pv_vcpu.gs_base_kernel; > + goto rdmsr_writeback; > case MSR_SHADOW_GS_BASE: > if ( is_pv_32on64_vcpu(v) ) > goto fail; > --- a/xen/arch/x86/x86_64/traps.c > +++ b/xen/arch/x86/x86_64/traps.c > @@ -257,6 +257,13 @@ void toggle_guest_mode(struct vcpu *v) > { > if ( is_pv_32bit_vcpu(v) ) > return; > + if ( cpu_has_fsgsbase ) > + { > + if ( v->arch.flags & TF_kernel_mode ) > + v->arch.pv_vcpu.gs_base_kernel = __rdgsbase(); > + else > + v->arch.pv_vcpu.gs_base_user = __rdgsbase(); > + } > v->arch.flags ^= TF_kernel_mode; > asm volatile ( "swapgs" ); > update_cr3(v); > --- a/xen/include/asm-x86/msr.h > +++ b/xen/include/asm-x86/msr.h > @@ -98,34 +98,52 @@ static inline int wrmsr_safe(unsigned in > : "=a" (low), "=d" (high) \ > : "c" (counter)) > > -static inline unsigned long rdfsbase(void) > +static inline unsigned long __rdfsbase(void) > { > unsigned long base; > > - if ( cpu_has_fsgsbase ) > #ifdef HAVE_GAS_FSGSBASE > - asm volatile ( "rdfsbase %0" : "=r" (base) ); > + asm volatile ( "rdfsbase %0" : "=r" (base) ); > #else > - asm volatile ( ".byte 0xf3, 0x48, 0x0f, 0xae, 0xc0" : "=a" (base) ); > + asm volatile ( ".byte 0xf3, 0x48, 0x0f, 0xae, 0xc0" : "=a" (base) ); > #endif > - else > - rdmsrl(MSR_FS_BASE, base); > > return base; > } > > -static inline unsigned long rdgsbase(void) > +static inline unsigned long __rdgsbase(void) > { > unsigned long base; > > - if ( cpu_has_fsgsbase ) > #ifdef HAVE_GAS_FSGSBASE > - asm volatile ( "rdgsbase %0" : "=r" (base) ); > + asm volatile ( "rdgsbase %0" : "=r" (base) ); > #else > - asm volatile ( ".byte 0xf3, 0x48, 0x0f, 0xae, 0xc8" : "=a" (base) ); > + asm volatile ( ".byte 0xf3, 0x48, 0x0f, 0xae, 0xc8" : "=a" (base) ); > #endif > - else > - rdmsrl(MSR_GS_BASE, base); > + > + return base; > +} > + > +static inline unsigned long rdfsbase(void) > +{ > + unsigned long base; > + > + if ( cpu_has_fsgsbase ) > + return __rdfsbase(); > + > + rdmsrl(MSR_FS_BASE, base); > + > + return base; > +} > + > +static inline unsigned long rdgsbase(void) > +{ > + unsigned long base; > + > + if ( cpu_has_fsgsbase ) > + return __rdgsbase(); > + > + rdmsrl(MSR_GS_BASE, base); > > return base; > } > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel --------------020507090806000700020401 Content-Type: text/html; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit
On 05/02/14 14:52, Jan Beulich wrote:
In that case, due to the respective instructions not being privileged,
we can't rely on our in-memory data to always be correct: While the
guest is running, it may change without us knowing about it. Therefore
we need to
- read the correct values from hardware during context switch out
  (save_segments())
- read the correct values from hardware during RDMSR emulation
- update in-memory values during guest mode change
  (toggle_guest_mode())

For completeness/consistency, WRMSR emulation is also being switched
to use wr[fg]sbase().

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

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>


--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1251,6 +1251,15 @@ static void save_segments(struct vcpu *v
     regs->fs = read_segment_register(fs);
     regs->gs = read_segment_register(gs);
 
+    if ( cpu_has_fsgsbase && !is_pv_32bit_vcpu(v) )
+    {
+        v->arch.pv_vcpu.fs_base = __rdfsbase();
+        if ( v->arch.flags & TF_kernel_mode )
+            v->arch.pv_vcpu.gs_base_kernel = __rdgsbase();
+        else
+            v->arch.pv_vcpu.gs_base_user = __rdgsbase();
+    }
+
     if ( regs->ds )
         dirty_segment_mask |= DIRTY_DS;
 
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -2382,15 +2382,13 @@ static int emulate_privileged_op(struct 
         case MSR_FS_BASE:
             if ( is_pv_32on64_vcpu(v) )
                 goto fail;
-            if ( wrmsr_safe(MSR_FS_BASE, msr_content) )
-                goto fail;
+            wrfsbase(msr_content);
             v->arch.pv_vcpu.fs_base = msr_content;
             break;
         case MSR_GS_BASE:
             if ( is_pv_32on64_vcpu(v) )
                 goto fail;
-            if ( wrmsr_safe(MSR_GS_BASE, msr_content) )
-                goto fail;
+            wrgsbase(msr_content);
             v->arch.pv_vcpu.gs_base_kernel = msr_content;
             break;
         case MSR_SHADOW_GS_BASE:
@@ -2535,15 +2533,14 @@ static int emulate_privileged_op(struct 
         case MSR_FS_BASE:
             if ( is_pv_32on64_vcpu(v) )
                 goto fail;
-            regs->eax = v->arch.pv_vcpu.fs_base & 0xFFFFFFFFUL;
-            regs->edx = v->arch.pv_vcpu.fs_base >> 32;
-            break;
+            val = cpu_has_fsgsbase ? __rdfsbase() : v->arch.pv_vcpu.fs_base;
+            goto rdmsr_writeback;
         case MSR_GS_BASE:
             if ( is_pv_32on64_vcpu(v) )
                 goto fail;
-            regs->eax = v->arch.pv_vcpu.gs_base_kernel & 0xFFFFFFFFUL;
-            regs->edx = v->arch.pv_vcpu.gs_base_kernel >> 32;
-            break;
+            val = cpu_has_fsgsbase ? __rdgsbase()
+                                   : v->arch.pv_vcpu.gs_base_kernel;
+            goto rdmsr_writeback;
         case MSR_SHADOW_GS_BASE:
             if ( is_pv_32on64_vcpu(v) )
                 goto fail;
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -257,6 +257,13 @@ void toggle_guest_mode(struct vcpu *v)
 {
     if ( is_pv_32bit_vcpu(v) )
         return;
+    if ( cpu_has_fsgsbase )
+    {
+        if ( v->arch.flags & TF_kernel_mode )
+            v->arch.pv_vcpu.gs_base_kernel = __rdgsbase();
+        else
+            v->arch.pv_vcpu.gs_base_user = __rdgsbase();
+    }
     v->arch.flags ^= TF_kernel_mode;
     asm volatile ( "swapgs" );
     update_cr3(v);
--- a/xen/include/asm-x86/msr.h
+++ b/xen/include/asm-x86/msr.h
@@ -98,34 +98,52 @@ static inline int wrmsr_safe(unsigned in
 			  : "=a" (low), "=d" (high) \
 			  : "c" (counter))
 
-static inline unsigned long rdfsbase(void)
+static inline unsigned long __rdfsbase(void)
 {
     unsigned long base;
 
-    if ( cpu_has_fsgsbase )
 #ifdef HAVE_GAS_FSGSBASE
-        asm volatile ( "rdfsbase %0" : "=r" (base) );
+    asm volatile ( "rdfsbase %0" : "=r" (base) );
 #else
-        asm volatile ( ".byte 0xf3, 0x48, 0x0f, 0xae, 0xc0" : "=a" (base) );
+    asm volatile ( ".byte 0xf3, 0x48, 0x0f, 0xae, 0xc0" : "=a" (base) );
 #endif
-    else
-        rdmsrl(MSR_FS_BASE, base);
 
     return base;
 }
 
-static inline unsigned long rdgsbase(void)
+static inline unsigned long __rdgsbase(void)
 {
     unsigned long base;
 
-    if ( cpu_has_fsgsbase )
 #ifdef HAVE_GAS_FSGSBASE
-        asm volatile ( "rdgsbase %0" : "=r" (base) );
+    asm volatile ( "rdgsbase %0" : "=r" (base) );
 #else
-        asm volatile ( ".byte 0xf3, 0x48, 0x0f, 0xae, 0xc8" : "=a" (base) );
+    asm volatile ( ".byte 0xf3, 0x48, 0x0f, 0xae, 0xc8" : "=a" (base) );
 #endif
-    else
-        rdmsrl(MSR_GS_BASE, base);
+
+    return base;
+}
+
+static inline unsigned long rdfsbase(void)
+{
+    unsigned long base;
+
+    if ( cpu_has_fsgsbase )
+        return __rdfsbase();
+
+    rdmsrl(MSR_FS_BASE, base);
+
+    return base;
+}
+
+static inline unsigned long rdgsbase(void)
+{
+    unsigned long base;
+
+    if ( cpu_has_fsgsbase )
+        return __rdgsbase();
+
+    rdmsrl(MSR_GS_BASE, base);
 
     return base;
 }





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

--------------020507090806000700020401-- --===============4569693584749241606== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============4569693584749241606==--