[PATCH for-4.11 v2 0/3] x86: XSA-267 follow-up

[PATCH for-4.11 v2 0/3] x86: XSA-267 follow-up

[Jan Beulich]

The first patch alone should be sufficient to address an issue with the
XSA-267 changes, causing a BUG_ON() to be triggered. The other two
patches would, afaict, each individually also have avoided the issue,
hence I'm also including them here. While the first patch obviously is
a strong candidate for 4.11, the latter two are candidates too imo; at
least I consider them backporting candidates (albeit, to be clear, patch
2 is useful only as far back as we have exception recovery attached
to emulator stubs, i.e. no farther than 4.9).

1: x86/HVM: don't cause #NM to be raised in Xen
2: x86: guard against #NM
3: VMX: check host CR0 before entering guest

Jan



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

[PATCH for-4.11 v2 1/3] x86/HVM: don't cause #NM to be raised in Xen

[Jan Beulich]

The changes for XSA-267 did not touch management of CR0.TS for HVM
guests. In fully eager mode this bit should never be set when
respective vCPU-s are active, or else hvmemul_get_fpu() might leave it
wrongly set, leading to #NM in hypervisor context.

{svm,vmx}_enter() and {svm,vmx}_fpu_dirty_intercept() become unreachable
this way. Explicit {svm,vmx}_fpu_leave() invocations need to be guarded
now.

With no CR0.TS management necessary in fully eager mode, there's also no
need anymore to intercept #NM.

Reported-by: Charles Arnold <carnold@suse.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Correct host_cr0 calculation in construct_vmcs().
---
TBD: A few ASSERT()s could be sprinkled around, but I wasn't sure how
     far to go with this, so I've left them out altogether for now.

--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -574,7 +574,10 @@ void svm_update_guest_cr(struct vcpu *v,
         if ( !(v->arch.hvm_vcpu.guest_cr[0] & X86_CR0_TS) )
         {
             if ( v != current )
-                hw_cr0_mask |= X86_CR0_TS;
+            {
+                if ( !v->arch.fully_eager_fpu )
+                    hw_cr0_mask |= X86_CR0_TS;
+            }
             else if ( vmcb_get_cr0(vmcb) & X86_CR0_TS )
                 svm_fpu_enter(v);
         }
@@ -1083,7 +1086,8 @@ static void svm_ctxt_switch_from(struct
     if ( unlikely((read_efer() & EFER_SVME) == 0) )
         return;
 
-    svm_fpu_leave(v);
+    if ( !v->arch.fully_eager_fpu )
+        svm_fpu_leave(v);
 
     svm_save_dr(v);
     svm_lwp_save(v);
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -180,8 +180,8 @@ static int construct_vmcb(struct vcpu *v
     paging_update_paging_modes(v);
 
     vmcb->_exception_intercepts =
-        HVM_TRAP_MASK
-        | (1U << TRAP_no_device);
+        HVM_TRAP_MASK |
+        (v->arch.fully_eager_fpu ? 0 : (1U << TRAP_no_device));
 
     if ( paging_mode_hap(v->domain) )
     {
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1144,7 +1144,9 @@ static int construct_vmcs(struct vcpu *v
     __vmwrite(HOST_TR_SELECTOR, TSS_ENTRY << 3);
 
     /* Host control registers. */
-    v->arch.hvm_vmx.host_cr0 = read_cr0() | X86_CR0_TS;
+    v->arch.hvm_vmx.host_cr0 = read_cr0() & ~X86_CR0_TS;
+    if ( !v->arch.fully_eager_fpu )
+        v->arch.hvm_vmx.host_cr0 |= X86_CR0_TS;
     __vmwrite(HOST_CR0, v->arch.hvm_vmx.host_cr0);
     __vmwrite(HOST_CR4, mmu_cr4_features);
 
@@ -1223,7 +1225,7 @@ static int construct_vmcs(struct vcpu *v
 
     v->arch.hvm_vmx.exception_bitmap = HVM_TRAP_MASK
               | (paging_mode_hap(d) ? 0 : (1U << TRAP_page_fault))
-              | (1U << TRAP_no_device);
+              | (v->arch.fully_eager_fpu ? 0 : (1U << TRAP_no_device));
     vmx_update_exception_bitmap(v);
 
     v->arch.hvm_vcpu.guest_cr[0] = X86_CR0_PE | X86_CR0_ET;
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -936,7 +936,8 @@ static void vmx_ctxt_switch_from(struct
         vmx_vmcs_reload(v);
     }
 
-    vmx_fpu_leave(v);
+    if ( !v->arch.fully_eager_fpu )
+        vmx_fpu_leave(v);
     vmx_save_guest_msrs(v);
     vmx_restore_host_msrs();
     vmx_save_dr(v);
@@ -1493,7 +1494,10 @@ static void vmx_update_guest_cr(struct v
         if ( !(v->arch.hvm_vcpu.guest_cr[0] & X86_CR0_TS) )
         {
             if ( v != current )
-                hw_cr0_mask |= X86_CR0_TS;
+            {
+                if ( !v->arch.fully_eager_fpu )
+                    hw_cr0_mask |= X86_CR0_TS;
+            }
             else if ( v->arch.hvm_vcpu.hw_cr[0] & X86_CR0_TS )
                 vmx_fpu_enter(v);
         }





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

[PATCH for-4.11 v2 2/3] x86: guard against #NM

[Jan Beulich]

Just in case we still don't get CR0.TS handling right, prevent a host
crash by honoring exception fixups in do_device_not_available(). This
would in particular cover emulator stubs raising #NM.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Add ASSERT_UNREACHABLE().

--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1749,7 +1749,21 @@ void do_device_not_available(struct cpu_
 {
     struct vcpu *curr = current;
 
-    BUG_ON(!guest_mode(regs));
+    if ( !guest_mode(regs) )
+    {
+        unsigned long fixup = search_exception_table(regs);
+
+        dprintk(XENLOG_ERR, "#NM: %p [%ps] -> %p\n",
+                _p(regs->rip), _p(regs->rip), _p(fixup));
+        /*
+         * We mustn't come here, but for release builds have the recovery
+         * logic in place nevertheless.
+         */
+        ASSERT_UNREACHABLE();
+        BUG_ON(!fixup);
+        regs->rip = fixup;
+        return;
+    }
 
     vcpu_restore_fpu_lazy(curr);
 





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

[PATCH for-4.11 v2 3/3] VMX: check host CR0 before entering guest

[Jan Beulich]

While we don't expect CR0 to change behind our backs, cope with this
happening, but other than for CR4 also log a (debug) message.

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

--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1676,7 +1676,7 @@ void vmx_vmentry_failure(void)
 void vmx_do_resume(struct vcpu *v)
 {
     bool_t debug_state;
-    unsigned long host_cr4;
+    unsigned long host_cr4, host_cr0, cr0;
 
     if ( v->arch.hvm_vmx.active_cpu == smp_processor_id() )
         vmx_vmcs_reload(v);
@@ -1732,6 +1732,15 @@ void vmx_do_resume(struct vcpu *v)
     if ( host_cr4 != read_cr4() )
         __vmwrite(HOST_CR4, read_cr4());
 
+    /* Check host CR0 (its value shouldn't have changed). */
+    __vmread(HOST_CR0, &host_cr0);
+    cr0 = read_cr0();
+    if ( host_cr0 != cr0 )
+    {
+        dprintk(XENLOG_ERR, "%pv: CR0 %lx != %lx\n", v, host_cr0, cr0);
+        __vmwrite(HOST_CR0, cr0);
+    }
+
     reset_stack_and_jump(vmx_asm_do_vmentry);
 }
 



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

Re: [PATCH for-4.11 v2 1/3] x86/HVM: don't cause #NM to be raised in Xen

[Andrew Cooper]

On 26/06/18 07:36, Jan Beulich wrote:
> The changes for XSA-267 did not touch management of CR0.TS for HVM
> guests. In fully eager mode this bit should never be set when
> respective vCPU-s are active, or else hvmemul_get_fpu() might leave it
> wrongly set, leading to #NM in hypervisor context.
>
> {svm,vmx}_enter() and {svm,vmx}_fpu_dirty_intercept() become unreachable
> this way. Explicit {svm,vmx}_fpu_leave() invocations need to be guarded
> now.
>
> With no CR0.TS management necessary in fully eager mode, there's also no
> need anymore to intercept #NM.
>
> Reported-by: Charles Arnold <carnold@suse.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

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

Re: [PATCH for-4.11 v2 2/3] x86: guard against #NM

[Andrew Cooper]

On 26/06/18 07:36, Jan Beulich wrote:
> Just in case we still don't get CR0.TS handling right, prevent a host
> crash by honoring exception fixups in do_device_not_available(). This
> would in particular cover emulator stubs raising #NM.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Add ASSERT_UNREACHABLE().
>
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1749,7 +1749,21 @@ void do_device_not_available(struct cpu_
>  {
>      struct vcpu *curr = current;
>  
> -    BUG_ON(!guest_mode(regs));
> +    if ( !guest_mode(regs) )
> +    {
> +        unsigned long fixup = search_exception_table(regs);
> +
> +        dprintk(XENLOG_ERR, "#NM: %p [%ps] -> %p\n",

gprintk() please.  the current vcpu is very likely relevant, and it
would be extremely useful to see this line in release builds where it to
happen.

> +                _p(regs->rip), _p(regs->rip), _p(fixup));
> +        /*
> +         * We mustn't come here, but for release builds have the recovery

"We shouldn't be able to reach here..."

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

> +         * logic in place nevertheless.
> +         */
> +        ASSERT_UNREACHABLE();
> +        BUG_ON(!fixup);
> +        regs->rip = fixup;
> +        return;
> +    }
>  
>      vcpu_restore_fpu_lazy(curr);
>  
>
>
>
>


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

Re: [PATCH for-4.11 v2 2/3] x86: guard against #NM

[Jan Beulich]

>>> On 26.06.18 at 11:41, <andrew.cooper3@citrix.com> wrote:
> On 26/06/18 07:36, Jan Beulich wrote:
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -1749,7 +1749,21 @@ void do_device_not_available(struct cpu_
>>  {
>>      struct vcpu *curr = current;
>>  
>> -    BUG_ON(!guest_mode(regs));
>> +    if ( !guest_mode(regs) )
>> +    {
>> +        unsigned long fixup = search_exception_table(regs);
>> +
>> +        dprintk(XENLOG_ERR, "#NM: %p [%ps] -> %p\n",
> 
> gprintk() please.  the current vcpu is very likely relevant, and it
> would be extremely useful to see this line in release builds where it to
> happen.

Hmm, yes - it's different from other similar log messages then, but I can
see your point.

>> +                _p(regs->rip), _p(regs->rip), _p(fixup));
>> +        /*
>> +         * We mustn't come here, but for release builds have the recovery
> 
> "We shouldn't be able to reach here..."

Well, okay - I did consider this weaker wording, but had opted for the
stronger one then.

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

Thanks.

Jan



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

Re: [PATCH for-4.11 v2 2/3] x86: guard against #NM

[Andrew Cooper]

On 26/06/18 10:57, Jan Beulich wrote:
>
>>> +                _p(regs->rip), _p(regs->rip), _p(fixup));
>>> +        /*
>>> +         * We mustn't come here, but for release builds have the recovery
>> "We shouldn't be able to reach here..."
> Well, okay - I did consider this weaker wording, but had opted for the
> stronger one then.

"mustn't" isn't the intended phrasing here, although I'm having a very
hard time trying to describe the difference vs "shouldn't be able to" in
this context.

By the time people are poking around in this code, the "mustn't" has
clearly been violated, whereas the "shouldn't be able to" is accepting
of the fact that we have got here, and something previously has gone wrong.

I realise this is probably a very unhelpful description.

~Andrew

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

Re: [PATCH for-4.11 v2 3/3] VMX: check host CR0 before entering guest

[Andrew Cooper]

On 26/06/18 07:38, Jan Beulich wrote:
> While we don't expect CR0 to change behind our backs, cope with this
> happening, but other than for CR4 also log a (debug) message.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -1676,7 +1676,7 @@ void vmx_vmentry_failure(void)
>  void vmx_do_resume(struct vcpu *v)
>  {
>      bool_t debug_state;
> -    unsigned long host_cr4;
> +    unsigned long host_cr4, host_cr0, cr0;
>  
>      if ( v->arch.hvm_vmx.active_cpu == smp_processor_id() )
>          vmx_vmcs_reload(v);
> @@ -1732,6 +1732,15 @@ void vmx_do_resume(struct vcpu *v)
>      if ( host_cr4 != read_cr4() )
>          __vmwrite(HOST_CR4, read_cr4());
>  
> +    /* Check host CR0 (its value shouldn't have changed). */
> +    __vmread(HOST_CR0, &host_cr0);
> +    cr0 = read_cr0();

For better or worse, read_cr0() isn't a cached read, so this adds a real
mov from cr0 into the resume path, which is a large overhead for a path
we expect never to take.

Now that we are 64bit, this could possibly be changed, as task switches
can't occur and change TS behind our back (and all guest task switches
are handled by Xen).

I'd also like to consider a point raised by Huawai at XenSummit.  Once
we handle #NM and disable the intercept, clts/stts inside the guest
still cause a vmexit.  In one HPC workload, this accounted for a 40%
performance impact.

On Intel hardware, this can be fixed with the cr0_host/guest mask
setting, similar to the cr4 changes in c/s 40681735502, and on AMD
hardware by making use of GENERAL1_INTERCEPT_CR0_SEL_WRITE in preference
to CR_INTERCEPT_CR0_WRITE.

With those optimisations in place, I don't believe these checks would be
warranted.

~Andrew

> +    if ( host_cr0 != cr0 )
> +    {
> +        dprintk(XENLOG_ERR, "%pv: CR0 %lx != %lx\n", v, host_cr0, cr0);
> +        __vmwrite(HOST_CR0, cr0);
> +    }
> +
>      reset_stack_and_jump(vmx_asm_do_vmentry);
>  }
>  
>
>


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

Re: [PATCH for-4.11 v2 1/3] x86/HVM: don't cause #NM to be raised in Xen

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, June 26, 2018 2:36 PM
> 
> The changes for XSA-267 did not touch management of CR0.TS for HVM
> guests. In fully eager mode this bit should never be set when
> respective vCPU-s are active, or else hvmemul_get_fpu() might leave it
> wrongly set, leading to #NM in hypervisor context.
> 
> {svm,vmx}_enter() and {svm,vmx}_fpu_dirty_intercept() become
> unreachable
> this way. Explicit {svm,vmx}_fpu_leave() invocations need to be guarded
> now.
> 
> With no CR0.TS management necessary in fully eager mode, there's also no
> need anymore to intercept #NM.
> 
> Reported-by: Charles Arnold <carnold@suse.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

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

Re: [PATCH for-4.11 v2 3/3] VMX: check host CR0 before entering guest

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Tuesday, June 26, 2018 6:34 PM
> 
> On 26/06/18 07:38, Jan Beulich wrote:
> > While we don't expect CR0 to change behind our backs, cope with this
> > happening, but other than for CR4 also log a (debug) message.
> >
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >
> > --- a/xen/arch/x86/hvm/vmx/vmcs.c
> > +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> > @@ -1676,7 +1676,7 @@ void vmx_vmentry_failure(void)
> >  void vmx_do_resume(struct vcpu *v)
> >  {
> >      bool_t debug_state;
> > -    unsigned long host_cr4;
> > +    unsigned long host_cr4, host_cr0, cr0;
> >
> >      if ( v->arch.hvm_vmx.active_cpu == smp_processor_id() )
> >          vmx_vmcs_reload(v);
> > @@ -1732,6 +1732,15 @@ void vmx_do_resume(struct vcpu *v)
> >      if ( host_cr4 != read_cr4() )
> >          __vmwrite(HOST_CR4, read_cr4());
> >
> > +    /* Check host CR0 (its value shouldn't have changed). */
> > +    __vmread(HOST_CR0, &host_cr0);
> > +    cr0 = read_cr0();
> 
> For better or worse, read_cr0() isn't a cached read, so this adds a real
> mov from cr0 into the resume path, which is a large overhead for a path
> we expect never to take.
> 
> Now that we are 64bit, this could possibly be changed, as task switches
> can't occur and change TS behind our back (and all guest task switches
> are handled by Xen).
> 
> I'd also like to consider a point raised by Huawai at XenSummit.  Once
> we handle #NM and disable the intercept, clts/stts inside the guest
> still cause a vmexit.  In one HPC workload, this accounted for a 40%
> performance impact.
> 
> On Intel hardware, this can be fixed with the cr0_host/guest mask
> setting, similar to the cr4 changes in c/s 40681735502, and on AMD
> hardware by making use of GENERAL1_INTERCEPT_CR0_SEL_WRITE in
> preference
> to CR_INTERCEPT_CR0_WRITE.
> 
> With those optimisations in place, I don't believe these checks would be
> warranted.
> 

I agree such check adds unnecessary overhead (possibly just do it in
debug build?), but didn't see how your comment invalidates the check.
It's about host CR0 check. why would policy change on guest behavior
impact that part?

Thanks
Kevin
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel