Re: [PATCH v2 06/12] VMX: add VMFUNC leaf 0 (EPTP switching) to emulator.

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 22/06/15 19:56, Ed White wrote:
> From: Ravi Sahita <ravi.sahita@intel.com>
>
> Signed-off-by: Ravi Sahita <ravi.sahita@intel.com>
> ---
>  xen/arch/x86/hvm/emulate.c             | 13 +++++++++++--
>  xen/arch/x86/hvm/vmx/vmx.c             | 30 ++++++++++++++++++++++++++++++
>  xen/arch/x86/x86_emulate/x86_emulate.c |  8 ++++++++
>  xen/arch/x86/x86_emulate/x86_emulate.h |  4 ++++
>  xen/include/asm-x86/hvm/hvm.h          |  2 ++
>  5 files changed, 55 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index ac9c9d6..e38a2fe 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1356,6 +1356,13 @@ static int hvmemul_invlpg(
>      return rc;
>  }
>  
> +static int hvmemul_vmfunc(
> +    struct x86_emulate_ctxt *ctxt)
> +{
> +    hvm_funcs.ahvm_vcpu_emulate_vmfunc(ctxt->regs);
> +    return X86EMUL_OKAY;
> +}

ahvm_vcpu_emulate_vmfunc() should return an X86EMUL code.

> +
>  static const struct x86_emulate_ops hvm_emulate_ops = {
>      .read          = hvmemul_read,
>      .insn_fetch    = hvmemul_insn_fetch,
> @@ -1379,7 +1386,8 @@ static const struct x86_emulate_ops hvm_emulate_ops = {
>      .inject_sw_interrupt = hvmemul_inject_sw_interrupt,
>      .get_fpu       = hvmemul_get_fpu,
>      .put_fpu       = hvmemul_put_fpu,
> -    .invlpg        = hvmemul_invlpg
> +    .invlpg        = hvmemul_invlpg,
> +    .vmfunc        = hvmemul_vmfunc,
>  };
>  
>  static const struct x86_emulate_ops hvm_emulate_ops_no_write = {
> @@ -1405,7 +1413,8 @@ static const struct x86_emulate_ops hvm_emulate_ops_no_write = {
>      .inject_sw_interrupt = hvmemul_inject_sw_interrupt,
>      .get_fpu       = hvmemul_get_fpu,
>      .put_fpu       = hvmemul_put_fpu,
> -    .invlpg        = hvmemul_invlpg
> +    .invlpg        = hvmemul_invlpg,
> +    .vmfunc        = hvmemul_vmfunc,
>  };
>  
>  static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt,
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index e8d9c82..ad9e9e4 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -82,6 +82,7 @@ static void vmx_fpu_dirty_intercept(void);
>  static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content);
>  static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content);
>  static void vmx_invlpg_intercept(unsigned long vaddr);
> +static int vmx_vmfunc_intercept(struct cpu_user_regs* regs);

s/* / */

>  
>  uint8_t __read_mostly posted_intr_vector;
>  
> @@ -1826,6 +1827,20 @@ static void vmx_vcpu_update_vmfunc_ve(struct vcpu *v)
>      vmx_vmcs_exit(v);
>  }
>  
> +static bool_t vmx_vcpu_emulate_vmfunc(struct cpu_user_regs *regs)
> +{
> +    bool_t rc = 0;
> +
> +    if ( !cpu_has_vmx_vmfunc && altp2mhvm_active(current->domain) &&
> +         regs->eax == 0 &&
> +         p2m_switch_vcpu_altp2m_by_id(current, (uint16_t)regs->ecx) )

Please latch current at the top of the function.  It is inefficient to
access like this.

> +    {
> +        regs->eip += 3;
> +        rc = 1;
> +    }
> +    return rc;
> +}
> +
>  static bool_t vmx_vcpu_emulate_ve(struct vcpu *v)
>  {
>      bool_t rc = 0;
> @@ -1894,6 +1909,7 @@ static struct hvm_function_table __initdata vmx_function_table = {
>      .msr_read_intercept   = vmx_msr_read_intercept,
>      .msr_write_intercept  = vmx_msr_write_intercept,
>      .invlpg_intercept     = vmx_invlpg_intercept,
> +    .vmfunc_intercept     = vmx_vmfunc_intercept,
>      .handle_cd            = vmx_handle_cd,
>      .set_info_guest       = vmx_set_info_guest,
>      .set_rdtsc_exiting    = vmx_set_rdtsc_exiting,
> @@ -1920,6 +1936,7 @@ static struct hvm_function_table __initdata vmx_function_table = {
>      .ahvm_vcpu_update_eptp = vmx_vcpu_update_eptp,
>      .ahvm_vcpu_update_vmfunc_ve = vmx_vcpu_update_vmfunc_ve,
>      .ahvm_vcpu_emulate_ve = vmx_vcpu_emulate_ve,
> +    .ahvm_vcpu_emulate_vmfunc = vmx_vcpu_emulate_vmfunc,
>  };
>  
>  const struct hvm_function_table * __init start_vmx(void)
> @@ -2091,6 +2108,13 @@ static void vmx_invlpg_intercept(unsigned long vaddr)
>          vpid_sync_vcpu_gva(curr, vaddr);
>  }
>  
> +static int vmx_vmfunc_intercept(struct cpu_user_regs *regs)
> +{
> +    gdprintk(XENLOG_ERR, "Failed guest VMFUNC execution\n");
> +    domain_crash(current->domain);
> +    return X86EMUL_OKAY;
> +}
> +
>  static int vmx_cr_access(unsigned long exit_qualification)
>  {
>      struct vcpu *curr = current;
> @@ -2675,6 +2699,7 @@ void vmx_enter_realmode(struct cpu_user_regs *regs)
>      regs->eflags |= (X86_EFLAGS_VM | X86_EFLAGS_IOPL);
>  }
>  
> +

Spurious whitespace change.

>  static void vmx_vmexit_ud_intercept(struct cpu_user_regs *regs)
>  {
>      struct hvm_emulate_ctxt ctxt;
> @@ -3239,6 +3264,11 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>              update_guest_eip();
>          break;
>  
> +    case EXIT_REASON_VMFUNC:
> +        if ( vmx_vmfunc_intercept(regs) == X86EMUL_OKAY )

This is currently an unconditional failure, and I don't see subsequent
patches which alter vmx_vmfunc_intercept().  Shouldn't
vmx_vmfunc_intercept() switch on eax and optionally call
p2m_switch_vcpu_altp2m_by_id()?

> +            update_guest_eip();
> +        break;
> +
>      case EXIT_REASON_MWAIT_INSTRUCTION:
>      case EXIT_REASON_MONITOR_INSTRUCTION:
>      case EXIT_REASON_GETSEC:
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
> index c017c69..4ae95ce 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -3837,6 +3837,14 @@ x86_emulate(
>              goto rdtsc;
>          }
>  
> +        if (modrm == 0xd4) /* vmfunc */

Style (spaces inside brackets).

~Andrew

> +        {
> +            fail_if(ops->vmfunc == NULL);
> +            if ( (rc = ops->vmfunc(ctxt) != 0) )
> +                goto done;
> +            break;
> +        }
> +
>          switch ( modrm_reg & 7 )
>          {
>          case 0: /* sgdt */
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h
> index 064b8f4..a4d4ec8 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -397,6 +397,10 @@ struct x86_emulate_ops
>          enum x86_segment seg,
>          unsigned long offset,
>          struct x86_emulate_ctxt *ctxt);
> +
> +    /* vmfunc: Emulate VMFUNC via given set of EAX ECX inputs */
> +    int (*vmfunc)(
> +        struct x86_emulate_ctxt *ctxt);
>  };
>  
>  struct cpu_user_regs;
> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
> index 9cd674f..2e33b4f 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -167,6 +167,7 @@ struct hvm_function_table {
>      int (*msr_read_intercept)(unsigned int msr, uint64_t *msr_content);
>      int (*msr_write_intercept)(unsigned int msr, uint64_t msr_content);
>      void (*invlpg_intercept)(unsigned long vaddr);
> +    int (*vmfunc_intercept)(struct cpu_user_regs *regs);
>      void (*handle_cd)(struct vcpu *v, unsigned long value);
>      void (*set_info_guest)(struct vcpu *v);
>      void (*set_rdtsc_exiting)(struct vcpu *v, bool_t);
> @@ -218,6 +219,7 @@ struct hvm_function_table {
>      void (*ahvm_vcpu_update_eptp)(struct vcpu *v);
>      void (*ahvm_vcpu_update_vmfunc_ve)(struct vcpu *v);
>      bool_t (*ahvm_vcpu_emulate_ve)(struct vcpu *v);
> +    bool_t (*ahvm_vcpu_emulate_vmfunc)(struct cpu_user_regs *regs);
>  };
>  
>  extern struct hvm_function_table hvm_funcs;