[PATCH 0/3] x86: FS/GS base handling adjustments

[PATCH 0/3] x86: FS/GS base handling adjustments

[Jan Beulich]

1: fix GS-base-dirty determination
2: also show FS/GS base addresses when dumping registers
3: avoid FS/GS base reads

Patch 1 is a bug fix which should be strongly considered for 4.10.
Patch 2 has proven helpful in analyzing the original problem, so
would be nice to have upstream rather sooner than later. Patch 3
is a minor performance tweak, which can easily wait until after
4.10.

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


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

[PATCH 1/3] x86: fix GS-base-dirty determination

[Jan Beulich]

load_segments() writes the two MSRs in their "canonical" positions
(GS_BASE for the user base, SHADOW_GS_BASE for the kernel one) and uses
SWAPGS to switch them around if the incoming vCPU is in kernel mode. In
order to not leave a stale kernel address in GS_BASE when the incoming
guest is in user mode, the check on the outgoing vCPU needs to be
dependent upon the mode it is currently in, rather than blindly looking
at the user base.

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

--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1479,7 +1479,8 @@ static void save_segments(struct vcpu *v
         if ( regs->gs & ~3 )
             v->arch.pv_vcpu.gs_base_user = 0;
     }
-    if ( v->arch.pv_vcpu.gs_base_user )
+    if ( v->arch.flags & TF_kernel_mode ? v->arch.pv_vcpu.gs_base_kernel
+                                        : v->arch.pv_vcpu.gs_base_user )
         dirty_segment_mask |= DIRTY_GS_BASE_USER;
 
     this_cpu(dirty_segment_mask) = dirty_segment_mask;




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

[PATCH 2/3] x86: also show FS/GS base addresses when dumping registers

[Jan Beulich]

Their state may be important to figure the reason for a crash. To not
further grow duplicate code, break out a helper function.

I realize that (ab)using the control register array here may not be
considered the nicest solution, but it seems easier (and less overall
overhead) to do so compared to the alternative of introducing another
helper structure.

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

--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -36,6 +36,21 @@ static void print_xen_info(void)
 
 enum context { CTXT_hypervisor, CTXT_pv_guest, CTXT_hvm_guest };
 
+static void read_registers(struct cpu_user_regs *regs, unsigned long crs[8])
+{
+    crs[0] = read_cr0();
+    crs[2] = read_cr2();
+    crs[3] = read_cr3();
+    crs[4] = read_cr4();
+    regs->ds = read_sreg(ds);
+    regs->es = read_sreg(es);
+    regs->fs = read_sreg(fs);
+    regs->gs = read_sreg(gs);
+    crs[5] = rdfsbase();
+    crs[6] = rdgsbase();
+    rdmsrl(MSR_SHADOW_GS_BASE, crs[7]);
+}
+
 static void _show_registers(
     const struct cpu_user_regs *regs, unsigned long crs[8],
     enum context context, const struct vcpu *v)
@@ -74,6 +89,8 @@ static void _show_registers(
     else
         printk("cr0: %016lx   cr4: %016lx\n", crs[0], crs[4]);
     printk("cr3: %016lx   cr2: %016lx\n", crs[3], crs[2]);
+    printk("fsb: %016lx   gsb: %016lx   gss: %016lx\n",
+           crs[5], crs[6], crs[7]);
     printk("ds: %04x   es: %04x   fs: %04x   gs: %04x   "
            "ss: %04x   cs: %04x\n",
            regs->ds, regs->es, regs->fs,
@@ -103,13 +120,18 @@ void show_registers(const struct cpu_use
         fault_regs.es = sreg.sel;
         hvm_get_segment_register(v, x86_seg_fs, &sreg);
         fault_regs.fs = sreg.sel;
+        fault_crs[5] = sreg.base;
         hvm_get_segment_register(v, x86_seg_gs, &sreg);
         fault_regs.gs = sreg.sel;
+        fault_crs[6] = sreg.base;
         hvm_get_segment_register(v, x86_seg_ss, &sreg);
         fault_regs.ss = sreg.sel;
+        fault_crs[7] = hvm_get_shadow_gs_base(v);
     }
     else
     {
+        read_registers(&fault_regs, fault_crs);
+
         if ( guest_mode(regs) )
         {
             context = CTXT_pv_guest;
@@ -120,14 +142,6 @@ void show_registers(const struct cpu_use
             context = CTXT_hypervisor;
             fault_crs[2] = read_cr2();
         }
-
-        fault_crs[0] = read_cr0();
-        fault_crs[3] = read_cr3();
-        fault_crs[4] = read_cr4();
-        fault_regs.ds = read_sreg(ds);
-        fault_regs.es = read_sreg(es);
-        fault_regs.fs = read_sreg(fs);
-        fault_regs.gs = read_sreg(gs);
     }
 
     print_xen_info();
@@ -146,6 +160,7 @@ void show_registers(const struct cpu_use
 void vcpu_show_registers(const struct vcpu *v)
 {
     const struct cpu_user_regs *regs = &v->arch.user_regs;
+    bool_t kernel = guest_kernel_mode(v, regs);
     unsigned long crs[8];
 
     /* Only handle PV guests for now */
@@ -154,10 +169,13 @@ void vcpu_show_registers(const struct vc
 
     crs[0] = v->arch.pv_vcpu.ctrlreg[0];
     crs[2] = arch_get_cr2(v);
-    crs[3] = pagetable_get_paddr(guest_kernel_mode(v, regs) ?
+    crs[3] = pagetable_get_paddr(kernel ?
                                  v->arch.guest_table :
                                  v->arch.guest_table_user);
     crs[4] = v->arch.pv_vcpu.ctrlreg[4];
+    crs[5] = v->arch.pv_vcpu.fs_base;
+    crs[6 + !kernel] = v->arch.pv_vcpu.gs_base_kernel;
+    crs[7 - !kernel] = v->arch.pv_vcpu.gs_base_user;
 
     _show_registers(regs, crs, CTXT_pv_guest, v);
 }
@@ -237,14 +255,7 @@ void do_double_fault(struct cpu_user_reg
     printk("*** DOUBLE FAULT ***\n");
     print_xen_info();
 
-    crs[0] = read_cr0();
-    crs[2] = read_cr2();
-    crs[3] = read_cr3();
-    crs[4] = read_cr4();
-    regs->ds = read_sreg(ds);
-    regs->es = read_sreg(es);
-    regs->fs = read_sreg(fs);
-    regs->gs = read_sreg(gs);
+    read_registers(regs, crs);
 
     printk("CPU:    %d\n", cpu);
     _show_registers(regs, crs, CTXT_hypervisor, NULL);



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

[PATCH 3/3] x86: avoid FS/GS base reads when possible

[Jan Beulich]

They're being zeroed a few lines down when non-null selectors are being
found in the respective registers.

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

--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1449,10 +1449,11 @@ static void save_segments(struct vcpu *v
 
     if ( cpu_has_fsgsbase && !is_pv_32bit_vcpu(v) )
     {
-        v->arch.pv_vcpu.fs_base = __rdfsbase();
+        if ( !(regs->fs & ~3) )
+            v->arch.pv_vcpu.fs_base = __rdfsbase();
         if ( v->arch.flags & TF_kernel_mode )
             v->arch.pv_vcpu.gs_base_kernel = __rdgsbase();
-        else
+        else if ( !(regs->gs & ~3) )
             v->arch.pv_vcpu.gs_base_user = __rdgsbase();
     }
 




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

Re: [PATCH 1/3] x86: fix GS-base-dirty determination

[Andrew Cooper]

On 20/10/17 15:22, Jan Beulich wrote:
> load_segments() writes the two MSRs in their "canonical" positions
> (GS_BASE for the user base, SHADOW_GS_BASE for the kernel one) and uses
> SWAPGS to switch them around if the incoming vCPU is in kernel mode. In
> order to not leave a stale kernel address in GS_BASE when the incoming
> guest is in user mode, the check on the outgoing vCPU needs to be
> dependent upon the mode it is currently in, rather than blindly looking
> at the user base.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

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

Re: [PATCH 2/3] x86: also show FS/GS base addresses when dumping registers

[Andrew Cooper]

On 20/10/17 15:23, Jan Beulich wrote:
> Their state may be important to figure the reason for a crash. To not
> further grow duplicate code, break out a helper function.
>
> I realize that (ab)using the control register array here may not be
> considered the nicest solution, but it seems easier (and less overall
> overhead) to do so compared to the alternative of introducing another
> helper structure.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Printing this information is definitely a good idea.

Reviewed-by Andrew Cooper <andrew.cooper3@citrix.com>, with two
observations.

>
> --- a/xen/arch/x86/x86_64/traps.c
> +++ b/xen/arch/x86/x86_64/traps.c
> @@ -36,6 +36,21 @@ static void print_xen_info(void)
>  
>  enum context { CTXT_hypervisor, CTXT_pv_guest, CTXT_hvm_guest };
>  

/* (ab)use crs[5..7] for fs/gs bases. */

?  A comment to this effect at least makes it clear that this is
intentional.

> +static void read_registers(struct cpu_user_regs *regs, unsigned long crs[8])
> +{
> +    crs[0] = read_cr0();
> +    crs[2] = read_cr2();
> +    crs[3] = read_cr3();
> +    crs[4] = read_cr4();
> +    regs->ds = read_sreg(ds);
> +    regs->es = read_sreg(es);
> +    regs->fs = read_sreg(fs);
> +    regs->gs = read_sreg(gs);
> +    crs[5] = rdfsbase();
> +    crs[6] = rdgsbase();
> +    rdmsrl(MSR_SHADOW_GS_BASE, crs[7]);
> +}
> +
>  static void _show_registers(
>      const struct cpu_user_regs *regs, unsigned long crs[8],
>      enum context context, const struct vcpu *v)
> @@ -146,6 +160,7 @@ void show_registers(const struct cpu_use
>  void vcpu_show_registers(const struct vcpu *v)
>  {
>      const struct cpu_user_regs *regs = &v->arch.user_regs;
> +    bool_t kernel = guest_kernel_mode(v, regs);

Forward porting mishap?

>      unsigned long crs[8];
>  
>      /* Only handle PV guests for now */
>


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

Re: [PATCH 3/3] x86: avoid FS/GS base reads when possible

[Andrew Cooper]

On 20/10/17 15:24, Jan Beulich wrote:
> They're being zeroed a few lines down when non-null selectors are being
> found in the respective registers.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Given that this is within an cpu_has_fsgsbase conditional, there is a
moderate chance that the conditional is more overhead than the
rd{fs,gs}base instruction alone.

ISTR Andy Lutomirsky finding that they were actually very efficient
instructions.

I'm not sure which is the better option here, but I'm not aversed to the
change, so Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1449,10 +1449,11 @@ static void save_segments(struct vcpu *v
>  
>      if ( cpu_has_fsgsbase && !is_pv_32bit_vcpu(v) )
>      {
> -        v->arch.pv_vcpu.fs_base = __rdfsbase();
> +        if ( !(regs->fs & ~3) )
> +            v->arch.pv_vcpu.fs_base = __rdfsbase();
>          if ( v->arch.flags & TF_kernel_mode )
>              v->arch.pv_vcpu.gs_base_kernel = __rdgsbase();
> -        else
> +        else if ( !(regs->gs & ~3) )
>              v->arch.pv_vcpu.gs_base_user = __rdgsbase();
>      }
>  
>
>
>


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

Re: [PATCH 2/3] x86: also show FS/GS base addresses when dumping registers

[Jan Beulich]

>>> On 20.10.17 at 16:56, <andrew.cooper3@citrix.com> wrote:
> On 20/10/17 15:23, Jan Beulich wrote:
>> Their state may be important to figure the reason for a crash. To not
>> further grow duplicate code, break out a helper function.
>>
>> I realize that (ab)using the control register array here may not be
>> considered the nicest solution, but it seems easier (and less overall
>> overhead) to do so compared to the alternative of introducing another
>> helper structure.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Printing this information is definitely a good idea.
> 
> Reviewed-by Andrew Cooper <andrew.cooper3@citrix.com>, with two
> observations.

Thanks.

>> --- a/xen/arch/x86/x86_64/traps.c
>> +++ b/xen/arch/x86/x86_64/traps.c
>> @@ -36,6 +36,21 @@ static void print_xen_info(void)
>>  
>>  enum context { CTXT_hypervisor, CTXT_pv_guest, CTXT_hvm_guest };
>>  
> 
> /* (ab)use crs[5..7] for fs/gs bases. */
> 
> ?  A comment to this effect at least makes it clear that this is
> intentional.

Okay.

>> +static void read_registers(struct cpu_user_regs *regs, unsigned long crs[8])
>> +{
>> +    crs[0] = read_cr0();
>> +    crs[2] = read_cr2();
>> +    crs[3] = read_cr3();
>> +    crs[4] = read_cr4();
>> +    regs->ds = read_sreg(ds);
>> +    regs->es = read_sreg(es);
>> +    regs->fs = read_sreg(fs);
>> +    regs->gs = read_sreg(gs);
>> +    crs[5] = rdfsbase();
>> +    crs[6] = rdgsbase();
>> +    rdmsrl(MSR_SHADOW_GS_BASE, crs[7]);
>> +}
>> +
>>  static void _show_registers(
>>      const struct cpu_user_regs *regs, unsigned long crs[8],
>>      enum context context, const struct vcpu *v)
>> @@ -146,6 +160,7 @@ void show_registers(const struct cpu_use
>>  void vcpu_show_registers(const struct vcpu *v)
>>  {
>>      const struct cpu_user_regs *regs = &v->arch.user_regs;
>> +    bool_t kernel = guest_kernel_mode(v, regs);
> 
> Forward porting mishap?

Oops, indeed.

Jan


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

Re: [PATCH 0/3] x86: FS/GS base handling adjustments

[Julien Grall]

On 20/10/17 15:11, Jan Beulich wrote:
> 1: fix GS-base-dirty determination
> 2: also show FS/GS base addresses when dumping registers
> 3: avoid FS/GS base reads
> 
> Patch 1 is a bug fix which should be strongly considered for 4.10.
> Patch 2 has proven helpful in analyzing the original problem, so
> would be nice to have upstream rather sooner than later. Patch 3
> is a minor performance tweak, which can easily wait until after
> 4.10.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

For the 3 patches:

Release-acked-by: Julien Grall <julien.grall@linaro.org>

Cheers,

-- 
Julien Grall

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