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

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> CC: Kevin Tian <kevin.tian@xxxxxxxxx>
> CC: Juergen Gross <jgross@xxxxxxxx>
> 
> 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@xxxxxxxxx>

> 
> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel