Re: [Xen-devel] [PATCH v4 2/2] x86/Intel: virtualize support for cpuid faulting

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 17/10/16 19:51, Kyle Huey wrote:
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index 6ed7486..a713ff3 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1544,16 +1544,35 @@ static int hvmemul_wbinvd(
>  
>  static int hvmemul_cpuid(
>      unsigned int *eax,
>      unsigned int *ebx,
>      unsigned int *ecx,
>      unsigned int *edx,
>      struct x86_emulate_ctxt *ctxt)
>  {
> +    /*
> +     * x86_emulate uses this function to query CPU features for its own 
> internal
> +     * use. Make sure we're actually emulating CPUID before emulating CPUID
> +     * faulting.

Looking into this, it is all a complete tangle.

Conceptually, the correct way to do this is to introduce
cpuid_faulting_active() to mirror the existing umip_active().  However,
the read_msr() infrastructure latched a #GP fault behind the back of the
emulator, so doesn't work for speculative reads.

Therefore, I am happy to accept the code in this form, because it looks
like the least bad option available at the moment.  I will see about
fixing it when I do the planned MSR overhaul work.

Otherwise, just a few style corrections.

> +     */
> +    if ( ctxt->opcode == X86EMUL_OPC(0x0f, 0xa2) &&
> +         hvm_check_cpuid_fault(current) ) {

Brace on newline please.

> +        struct hvm_emulate_ctxt *hvmemul_ctxt =
> +            container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
> +
> +        hvmemul_ctxt->exn_pending = 1;
> +        hvmemul_ctxt->trap.vector = TRAP_gp_fault;
> +        hvmemul_ctxt->trap.type = X86_EVENTTYPE_HW_EXCEPTION;
> +        hvmemul_ctxt->trap.error_code = 0;
> +        hvmemul_ctxt->trap.insn_len = 0;
> +
> +        return X86EMUL_EXCEPTION;
> +    }
> +
>      hvm_funcs.cpuid_intercept(eax, ebx, ecx, edx);
>      return X86EMUL_OKAY;
>  }
>  
>  static int hvmemul_inject_hw_exception(
>      uint8_t vector,
>      int32_t error_code,
>      struct x86_emulate_ctxt *ctxt)
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index b9102ce..228c1b9 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2428,16 +2428,21 @@ static void vmx_cpuid_intercept(
>      HVMTRACE_5D (CPUID, input, *eax, *ebx, *ecx, *edx);
>  }
>  
>  static int vmx_do_cpuid(struct cpu_user_regs *regs)
>  {
>      unsigned int eax, ebx, ecx, edx;
>      unsigned int leaf, subleaf;
>  
> +    if ( hvm_check_cpuid_fault(current) ) {

And here please.

> +        hvm_inject_hw_exception(TRAP_gp_fault, 0);
> +        return 1;  /* Don't advance the guest IP! */
> +    }
> +
>      eax = regs->eax;
>      ebx = regs->ebx;
>      ecx = regs->ecx;
>      edx = regs->edx;
>  
>      leaf = regs->eax;
>      subleaf = regs->ecx;
>  
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 293ff8d..12322bd 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1315,16 +1315,24 @@ static int emulate_forced_invalid_op(struct 
> cpu_user_regs *regs)
>      /* We only emulate CPUID. */
>      if ( ( rc = copy_from_user(instr, (char *)eip, sizeof(instr))) != 0 )
>      {
>          propagate_page_fault(eip + sizeof(instr) - rc, 0);
>          return EXCRET_fault_fixed;
>      }
>      if ( memcmp(instr, "\xf\xa2", sizeof(instr)) )
>          return 0;
> +
> +    /* If cpuid faulting is enabled and CPL>0 inject a #GP in place of #UD. 
> */
> +    if ( current->arch.cpuid_fault && !guest_kernel_mode(current, regs) ) {

And here.

~Andrew

> +        regs->eip = eip;
> +        do_guest_trap(TRAP_gp_fault, regs);
> +        return EXCRET_fault_fixed;
> +    }
> +
>      eip += sizeof(instr);
>  
>      pv_cpuid(regs);
>  
>      instruction_done(regs, eip, 0);
>  
>      trace_trap_one_addr(TRC_PV_FORCED_INVALID_OP, regs->eip);
>  
>

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