[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 2/2] x86/hvm: Corrections to RDTSCP intercept handling



> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> Sent: 30 November 2018 17:07
> To: Xen-devel <xen-devel@xxxxxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Jan Beulich
> <JBeulich@xxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; Roger Pau Monne
> <roger.pau@xxxxxxxxxx>; Paul Durrant <Paul.Durrant@xxxxxxxxxx>; Jun
> Nakajima <jun.nakajima@xxxxxxxxx>; Kevin Tian <kevin.tian@xxxxxxxxx>;
> Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>; Suravee Suthikulpanit
> <suravee.suthikulpanit@xxxxxxx>; Brian Woods <brian.woods@xxxxxxx>;
> Juergen Gross <jgross@xxxxxxxx>
> Subject: [PATCH 2/2] x86/hvm: Corrections to RDTSCP intercept handling
> 
> For both VT-x and SVM, the RDTSCP intercept will trigger if the pipeline
> supports the instruction, but the guest may have not have rdtscp in its
> featureset.  Bring the vmexit handlers in line with the main emulator
> behaviour by optionally handing back #UD.
> 
> Next on the AMD side, if RDTSCP actually ends up being intercepted on a
> debug
> build, we first update regs->rcx, then call __get_instruction_length()
> asking
> for RDTSC.  As the two instructions are different (and indeed, different
> lengths!), __get_instruction_length_from_list() fails and hands back a #GP
> fault.
> 
> This can demonstrated by putting a guest into tsc_mode="always emulate"
> and
> executing an rdtscp instruction:
> 
>   (d1) --- Xen Test Framework ---
>   (d1) Environment: HVM 64bit (Long mode 4 levels)
>   (d1) Test rdtscp
>   (d1) TSC mode 1
>   (XEN) emulate.c:159:d1v0 __get_instruction_length_from_list: Mismatch
> between expected and actual instruction:
>   (XEN) emulate.c:163:d1v0   list[0] val 8, { opc 0xf0031, modrm 0 }, list
> entries: 1
>   (XEN) emulate.c:165:d1v0   rip 0x10475f, nextrip 0x104762, len 3
>   (XEN) Insn_len emulation failed (1): d1v0 64bit @ 0008:0010475f -> 0f 01
> f9 5b 31 ff 31 c0 e9 c4 db ff ff 00 00 00
>   (d1) ******************************
>   (d1) PANIC: Unhandled exception at 0008:000000000010475f
>   (d1) Vec 13 #GP[0000]
>   (d1) ******************************
> 
> First, teach __get_instruction_length() to cope with RDTSCP, and improve
> svm_vmexit_do_rdtsc() to ask for the correct instruction.  Move the regs-
> >rcx
> adjustment into this function to ensure it gets done after we are done
> potentially raising faults.
> 
> Reported-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx>

With one observation...

> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Paul Durrant <paul.durrant@xxxxxxxxxx>
> CC: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> CC: Kevin Tian <kevin.tian@xxxxxxxxx>
> CC: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> CC: Brian Woods <brian.woods@xxxxxxx>
> CC: Juergen Gross <jgross@xxxxxxxx>
> 
> There is a further 4.12 bug/regression here.  For some reason, master and
> staging are now defaulting VMs into an emulated TSC mode.  I have yet to
> figure out why.
> ---
>  xen/arch/x86/hvm/svm/emulate.c        |  1 +
>  xen/arch/x86/hvm/svm/svm.c            | 22 +++++++++++++++++-----
>  xen/arch/x86/hvm/vmx/vmx.c            |  8 ++++++++
>  xen/include/asm-x86/hvm/svm/emulate.h |  1 +
>  4 files changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/emulate.c
> b/xen/arch/x86/hvm/svm/emulate.c
> index 71a1b6e..0290264 100644
> --- a/xen/arch/x86/hvm/svm/emulate.c
> +++ b/xen/arch/x86/hvm/svm/emulate.c
> @@ -78,6 +78,7 @@ static const struct {
>      [INSTR_STGI]    = { X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 4) },
>      [INSTR_CLGI]    = { X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 5) },
>      [INSTR_INVLPGA] = { X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 7) },
> +    [INSTR_RDTSCP]  = { X86EMUL_OPC(0x0f, 0x01), MODRM(3, 7, 1) },
>      [INSTR_INVD]    = { X86EMUL_OPC(0x0f, 0x08) },
>      [INSTR_WBINVD]  = { X86EMUL_OPC(0x0f, 0x09) },
>      [INSTR_WRMSR]   = { X86EMUL_OPC(0x0f, 0x30) },
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index b9a8900..d8d3813 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2279,14 +2279,28 @@ static void svm_vmexit_do_hlt(struct vmcb_struct
> *vmcb,
>      hvm_hlt(regs->eflags);
>  }
> 
> -static void svm_vmexit_do_rdtsc(struct cpu_user_regs *regs)
> +static void svm_vmexit_do_rdtsc(struct cpu_user_regs *regs, bool rdtscp)
>  {
> +    struct vcpu *curr = current;
> +    const struct domain *currd = curr->domain;
> +    enum instruction_index insn = rdtscp ? INSTR_RDTSCP : INSTR_RDTSC;
>      unsigned int inst_len;
> 
> -    if ( (inst_len = __get_instruction_length(current, INSTR_RDTSC)) == 0
> )
> +    if ( rdtscp && !currd->arch.cpuid->extd.rdtscp &&
> +         currd->arch.tsc_mode != TSC_MODE_PVRDTSCP )
> +    {
> +        hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
>          return;
> +    }
> +
> +    if ( (inst_len = __get_instruction_length(curr, insn)) == 0 )
> +        return;
> +
>      __update_guest_eip(regs, inst_len);
> 
> +    if ( rdtscp )
> +        regs->rcx = hvm_msr_tsc_aux(curr);
> +
>      hvm_rdtsc_intercept(regs);
>  }
> 
> @@ -2968,10 +2982,8 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>          break;
> 
>      case VMEXIT_RDTSCP:
> -        regs->rcx = hvm_msr_tsc_aux(v);
> -        /* fall through */
>      case VMEXIT_RDTSC:
> -        svm_vmexit_do_rdtsc(regs);
> +        svm_vmexit_do_rdtsc(regs, exit_reason == VMEXIT_RDTSCP);
>          break;
> 
>      case VMEXIT_MONITOR:
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 365eeb2..a9f9b9b 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3589,6 +3589,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>      unsigned long exit_qualification, exit_reason, idtv_info, intr_info =
> 0;
>      unsigned int vector = 0, mode;
>      struct vcpu *v = current;
> +    struct domain *currd = v->domain;

... following the usual rules, you should now convert all uses of v->domain in 
this function to use currd.

> 
>      __vmread(GUEST_RIP,    &regs->rip);
>      __vmread(GUEST_RSP,    &regs->rsp);
> @@ -3956,6 +3957,13 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>          vmx_invlpg_intercept(exit_qualification);
>          break;
>      case EXIT_REASON_RDTSCP:
> +        if ( !currd->arch.cpuid->extd.rdtscp &&
> +             currd->arch.tsc_mode != TSC_MODE_PVRDTSCP )
> +        {
> +            hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
> +            break;
> +        }
> +
>          regs->rcx = hvm_msr_tsc_aux(v);
>          /* fall through */
>      case EXIT_REASON_RDTSC:
> diff --git a/xen/include/asm-x86/hvm/svm/emulate.h b/xen/include/asm-
> x86/hvm/svm/emulate.h
> index 3de8236..ca92abb 100644
> --- a/xen/include/asm-x86/hvm/svm/emulate.h
> +++ b/xen/include/asm-x86/hvm/svm/emulate.h
> @@ -30,6 +30,7 @@ enum instruction_index {
>      INSTR_HLT,
>      INSTR_INT3,
>      INSTR_RDTSC,
> +    INSTR_RDTSCP,
>      INSTR_PAUSE,
>      INSTR_XSETBV,
>      INSTR_VMRUN,
> --
> 2.1.4

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.