[Xen-devel] [PATCH] x86/svm: Fix handling of EFLAGS.RF on task switch

[Xen-devel] [PATCH] x86/svm: Fix handling of EFLAGS.RF on task switch

[Andrew Cooper]

VT-x updates RF before vmexit, so eflags written into the outgoing TSS happens
to be correct.  SVM does not update RF before vmexit, and instead provides it
via a bit in exitinfo2.

In practice, needing RF set in the outgoing state occurs when a task gate is
used to handle faults.

Extend hvm_task_switch() with an extra_eflags parameter which gets fed into
the outgoing TSS, and fill it in suitably from the SVM vmexit information.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Juergen Gross <jgross@suse.com>

Kevin: There is no help in the SDM about this.  RF is not mentioned in the
list of state either modified or unmodified by hardware on a task switch
vmexit.  This conclusion has been drawn from looking at the actual VMExit
state given an XTF test poking every corner of TASK_SWITCH VMExits.

Juergen: I know its getting stupidly late in the day, but this, like the
previous fixes, want backporting.  OTOH, the likelihood of not fixing it
causing harm to VMs is minimal, unlike the earlier task switch fixes.
---
 xen/arch/x86/hvm/hvm.c        | 4 ++--
 xen/arch/x86/hvm/svm/svm.c    | 3 ++-
 xen/arch/x86/hvm/vmx/vmx.c    | 3 ++-
 xen/include/asm-x86/hvm/hvm.h | 2 +-
 4 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 7f556171bd..47573f71b8 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2913,7 +2913,7 @@ void hvm_prepare_vm86_tss(struct vcpu *v, uint32_t base, uint32_t limit)
 
 void hvm_task_switch(
     uint16_t tss_sel, enum hvm_task_switch_reason taskswitch_reason,
-    int32_t errcode, unsigned int insn_len)
+    int32_t errcode, unsigned int insn_len, unsigned int extra_eflags)
 {
     struct vcpu *v = current;
     struct cpu_user_regs *regs = guest_cpu_user_regs();
@@ -2988,7 +2988,7 @@ void hvm_task_switch(
         eflags &= ~X86_EFLAGS_NT;
 
     tss.eip    = regs->eip + insn_len;
-    tss.eflags = eflags;
+    tss.eflags = eflags | extra_eflags;
     tss.eax    = regs->eax;
     tss.ecx    = regs->ecx;
     tss.edx    = regs->edx;
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 0fb1908c18..6ae43999ff 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2812,7 +2812,8 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
         if ( (vmcb->exitinfo2 >> 44) & 1 )
             errcode = (uint32_t)vmcb->exitinfo2;
 
-        hvm_task_switch(vmcb->exitinfo1, reason, errcode, insn_len);
+        hvm_task_switch(vmcb->exitinfo1, reason, errcode, insn_len,
+                        (vmcb->exitinfo2 & (1ul << 48)) ? X86_EFLAGS_RF : 0);
         break;
     }
 
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 7450cbe40d..bafc3b30c5 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3963,7 +3963,8 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
         else
              ecode = -1;
 
-        hvm_task_switch(exit_qualification, reasons[source], ecode, inst_len);
+        hvm_task_switch(exit_qualification, reasons[source], ecode, inst_len,
+                        0 /* EFLAGS.RF already updated. */);
         break;
     }
     case EXIT_REASON_CPUID:
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 17fb7efa6e..1d7b66f927 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -296,7 +296,7 @@ void hvm_set_rdtsc_exiting(struct domain *d, bool_t enable);
 enum hvm_task_switch_reason { TSW_jmp, TSW_iret, TSW_call_or_int };
 void hvm_task_switch(
     uint16_t tss_sel, enum hvm_task_switch_reason taskswitch_reason,
-    int32_t errcode, unsigned int insn_len);
+    int32_t errcode, unsigned int insn_len, unsigned int extra_eflags);
 
 enum hvm_access_type {
     hvm_access_insn_fetch,
-- 
2.11.0


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

Re: [Xen-devel] [PATCH] x86/svm: Fix handling of EFLAGS.RF on task switch

[Jan Beulich]

On 03.12.2019 23:30, Andrew Cooper wrote:
> VT-x updates RF before vmexit, so eflags written into the outgoing TSS happens
> to be correct.  SVM does not update RF before vmexit, and instead provides it
> via a bit in exitinfo2.
> 
> In practice, needing RF set in the outgoing state occurs when a task gate is
> used to handle faults.
> 
> Extend hvm_task_switch() with an extra_eflags parameter which gets fed into
> the outgoing TSS, and fill it in suitably from the SVM vmexit information.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

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

Re: [Xen-devel] [PATCH] x86/svm: Fix handling of EFLAGS.RF on task switch

[Tian, Kevin]

> From: Andrew Cooper <andrew.cooper3@citrix.com>
> Sent: Wednesday, December 4, 2019 6:31 AM
> 
> VT-x updates RF before vmexit, so eflags written into the outgoing TSS
> happens
> to be correct.  SVM does not update RF before vmexit, and instead provides
> it
> via a bit in exitinfo2.
> 
> In practice, needing RF set in the outgoing state occurs when a task gate is
> used to handle faults.
> 
> Extend hvm_task_switch() with an extra_eflags parameter which gets fed
> into
> the outgoing TSS, and fill it in suitably from the SVM vmexit information.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> CC: Juergen Gross <jgross@suse.com>
> 
> Kevin: There is no help in the SDM about this.  RF is not mentioned in the
> list of state either modified or unmodified by hardware on a task switch
> vmexit.  This conclusion has been drawn from looking at the actual VMExit
> state given an XTF test poking every corner of TASK_SWITCH VMExits.

Here is what I observed in latest SDM (Oct. 2019):

27.3.3 Saving RIP, RSP, RFLAGS, and SSP
...
With the exception of the resume flag (RF; bit 16), the contents 
of the RFLAGS register is saved into the RFLAGS field. RFLAGS.RF 
is saved as follows:
...
- If the VM exit is caused by a task switch (including one caused 
by a task gate in the IDT), the value saved is that which would 
have been saved in the RFLAGS image in the old task-state 
segment (TSS) had the task switch completed normally without 
exception.
...

Based on that, fox vmx part:

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

> 
> Juergen: I know its getting stupidly late in the day, but this, like the
> previous fixes, want backporting.  OTOH, the likelihood of not fixing it
> causing harm to VMs is minimal, unlike the earlier task switch fixes.
> ---
>  xen/arch/x86/hvm/hvm.c        | 4 ++--
>  xen/arch/x86/hvm/svm/svm.c    | 3 ++-
>  xen/arch/x86/hvm/vmx/vmx.c    | 3 ++-
>  xen/include/asm-x86/hvm/hvm.h | 2 +-
>  4 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 7f556171bd..47573f71b8 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2913,7 +2913,7 @@ void hvm_prepare_vm86_tss(struct vcpu *v,
> uint32_t base, uint32_t limit)
> 
>  void hvm_task_switch(
>      uint16_t tss_sel, enum hvm_task_switch_reason taskswitch_reason,
> -    int32_t errcode, unsigned int insn_len)
> +    int32_t errcode, unsigned int insn_len, unsigned int extra_eflags)
>  {
>      struct vcpu *v = current;
>      struct cpu_user_regs *regs = guest_cpu_user_regs();
> @@ -2988,7 +2988,7 @@ void hvm_task_switch(
>          eflags &= ~X86_EFLAGS_NT;
> 
>      tss.eip    = regs->eip + insn_len;
> -    tss.eflags = eflags;
> +    tss.eflags = eflags | extra_eflags;
>      tss.eax    = regs->eax;
>      tss.ecx    = regs->ecx;
>      tss.edx    = regs->edx;
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 0fb1908c18..6ae43999ff 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2812,7 +2812,8 @@ void svm_vmexit_handler(struct cpu_user_regs
> *regs)
>          if ( (vmcb->exitinfo2 >> 44) & 1 )
>              errcode = (uint32_t)vmcb->exitinfo2;
> 
> -        hvm_task_switch(vmcb->exitinfo1, reason, errcode, insn_len);
> +        hvm_task_switch(vmcb->exitinfo1, reason, errcode, insn_len,
> +                        (vmcb->exitinfo2 & (1ul << 48)) ? X86_EFLAGS_RF : 0);
>          break;
>      }
> 
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 7450cbe40d..bafc3b30c5 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3963,7 +3963,8 @@ void vmx_vmexit_handler(struct cpu_user_regs
> *regs)
>          else
>               ecode = -1;
> 
> -        hvm_task_switch(exit_qualification, reasons[source], ecode, inst_len);
> +        hvm_task_switch(exit_qualification, reasons[source], ecode, inst_len,
> +                        0 /* EFLAGS.RF already updated. */);
>          break;
>      }
>      case EXIT_REASON_CPUID:
> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-
> x86/hvm/hvm.h
> index 17fb7efa6e..1d7b66f927 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -296,7 +296,7 @@ void hvm_set_rdtsc_exiting(struct domain *d, bool_t
> enable);
>  enum hvm_task_switch_reason { TSW_jmp, TSW_iret, TSW_call_or_int };
>  void hvm_task_switch(
>      uint16_t tss_sel, enum hvm_task_switch_reason taskswitch_reason,
> -    int32_t errcode, unsigned int insn_len);
> +    int32_t errcode, unsigned int insn_len, unsigned int extra_eflags);
> 
>  enum hvm_access_type {
>      hvm_access_insn_fetch,
> --
> 2.11.0

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

Re: [Xen-devel] [PATCH] x86/svm: Fix handling of EFLAGS.RF on task switch

[Jürgen Groß]

On 03.12.19 23:30, Andrew Cooper wrote:
> VT-x updates RF before vmexit, so eflags written into the outgoing TSS happens
> to be correct.  SVM does not update RF before vmexit, and instead provides it
> via a bit in exitinfo2.
> 
> In practice, needing RF set in the outgoing state occurs when a task gate is
> used to handle faults.
> 
> Extend hvm_task_switch() with an extra_eflags parameter which gets fed into
> the outgoing TSS, and fill it in suitably from the SVM vmexit information.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

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