Re: [Xen-devel] [PATCH v3 07/13] VMX: add VMFUNC leaf 0 (EPTP switching) to emulator.

["Sahita, Ravi" <ravi.sahita@xxxxxxxxx>]

-----Original Message-----
From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx] 
Sent: Friday, July 03, 2015 9:40 AM
To: White, Edmund H; xen-devel@xxxxxxxxxxxxx
Cc: Ian Jackson; Jan Beulich; Tim Deegan; Daniel De Graaf; Sahita, Ravi; Wei 
Liu; tlengyel@xxxxxxxxxxx; George Dunlap
Subject: Re: [PATCH v3 07/13] VMX: add VMFUNC leaf 0 (EPTP switching) to 
emulator.

On 01/07/15 19:09, Ed White wrote:
> From: Ravi Sahita <ravi.sahita@xxxxxxxxx>
>
> Signed-off-by: Ravi Sahita <ravi.sahita@xxxxxxxxx>
> ---
>  xen/arch/x86/hvm/emulate.c             | 12 +++++++--
>  xen/arch/x86/hvm/vmx/vmx.c             | 30 +++++++++++++++++++++
>  xen/arch/x86/x86_emulate/x86_emulate.c | 48 
> +++++++++++++++++++++-------------
>  xen/arch/x86/x86_emulate/x86_emulate.h |  4 +++
>  xen/include/asm-x86/hvm/hvm.h          |  2 ++
>  5 files changed, 76 insertions(+), 20 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c 
> index ac9c9d6..157fe78 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1356,6 +1356,12 @@ static int hvmemul_invlpg(
>      return rc;
>  }
>  
> +static int hvmemul_vmfunc(
> +    struct x86_emulate_ctxt *ctxt)
> +{
> +    return hvm_funcs.ap2m_vcpu_emulate_vmfunc(ctxt->regs);
> +}
> +
>  static const struct x86_emulate_ops hvm_emulate_ops = {
>      .read          = hvmemul_read,
>      .insn_fetch    = hvmemul_insn_fetch,
> @@ -1379,7 +1385,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 +1412,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 9585aa3..c6feeae 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);
>  
>  uint8_t __read_mostly posted_intr_vector;
>  
> @@ -1830,6 +1831,20 @@ static void vmx_vcpu_update_vmfunc_ve(struct vcpu *v)
>      vmx_vmcs_exit(v);
>  }
>  
> +static int vmx_vcpu_emulate_vmfunc(struct cpu_user_regs *regs) {
> +    int rc = X86EMUL_EXCEPTION;
> +    struct vcpu *v = current;
> +
> +    if ( !cpu_has_vmx_vmfunc && altp2m_active(v->domain) &&
> +         regs->eax == 0 &&
> +         p2m_switch_vcpu_altp2m_by_id(v, (uint16_t)regs->ecx) )
> +    {
> +        rc = X86EMUL_OKAY;
> +    }

You need a #UD injection at this point.

Ravi> I will keep this function unchanged i.e. returns X86EMUL_EXCEPTION on 
error, which will cause the initiating hvmemul_vmfunc to stage a #UD (this 
staging was in fact missing, and is now fixed - thanks).
Ravi> The #UD is actually injected by the top level routine 
vmx_vmexit_ud_intercept.

> +    return rc;
> +}
> +
>  static bool_t vmx_vcpu_emulate_ve(struct vcpu *v)  {
>      bool_t rc = 0;
> @@ -1898,6 +1913,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,
> @@ -1924,6 +1940,7 @@ static struct hvm_function_table __initdata 
> vmx_function_table = {
>      .ap2m_vcpu_update_eptp = vmx_vcpu_update_eptp,
>      .ap2m_vcpu_update_vmfunc_ve = vmx_vcpu_update_vmfunc_ve,
>      .ap2m_vcpu_emulate_ve = vmx_vcpu_emulate_ve,
> +    .ap2m_vcpu_emulate_vmfunc = vmx_vcpu_emulate_vmfunc,
>  };
>  
>  const struct hvm_function_table * __init start_vmx(void) @@ -2095,6 
> +2112,12 @@ 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");
> +    return X86EMUL_EXCEPTION;
> +}
> +
>  static int vmx_cr_access(unsigned long exit_qualification)  {
>      struct vcpu *curr = current;
> @@ -3245,6 +3268,13 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>              update_guest_eip();
>          break;
>  
> +    case EXIT_REASON_VMFUNC:
> +        if ( vmx_vmfunc_intercept(regs) == X86EMUL_OKAY )
> +            update_guest_eip();
> +        else
> +            hvm_inject_hw_exception(TRAP_invalid_op, 
> HVM_DELIVER_NO_ERROR_CODE);
> +        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..adf64d0 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -3815,28 +3815,40 @@ x86_emulate(
>      case 0x01: /* Grp7 */ {
>          struct segment_register reg;
>          unsigned long base, limit, cr0, cr0w;
> +        uint64_t tsc_aux;

This variable can live inside the rdtscp case, to reduce its scope.

Ravi>Will address

>  
> -        if ( modrm == 0xdf ) /* invlpga */
> +        switch( modrm )
>          {
> -            generate_exception_if(!in_protmode(ctxt, ops), EXC_UD, -1);
> -            generate_exception_if(!mode_ring0(), EXC_GP, 0);
> -            fail_if(ops->invlpg == NULL);
> -            if ( (rc = ops->invlpg(x86_seg_none, truncate_ea(_regs.eax),
> -                                   ctxt)) )
> -                goto done;
> -            break;
> -        }
> -
> -        if ( modrm == 0xf9 ) /* rdtscp */
> -        {
> -            uint64_t tsc_aux;
> -            fail_if(ops->read_msr == NULL);
> -            if ( (rc = ops->read_msr(MSR_TSC_AUX, &tsc_aux, ctxt)) != 0 )
> -                goto done;
> -            _regs.ecx = (uint32_t)tsc_aux;
> -            goto rdtsc;
> +            case 0xdf: /* invlpga AMD */
> +                generate_exception_if(!in_protmode(ctxt, ops), EXC_UD, -1);
> +                generate_exception_if(!mode_ring0(), EXC_GP, 0);
> +                fail_if(ops->invlpg == NULL);
> +                if ( (rc = ops->invlpg(x86_seg_none, truncate_ea(_regs.eax),
> +                                       ctxt)) )
> +                    goto done;
> +                break;
> +            case 0xf9: /* rdtscp */
> +                fail_if(ops->read_msr == NULL);
> +                if ( (rc = ops->read_msr(MSR_TSC_AUX, &tsc_aux, ctxt)) != 0 )
> +                    goto done;
> +                _regs.ecx = (uint32_t)tsc_aux;
> +                goto rdtsc;
> +            case 0xd4: /* vmfunc */
> +                generate_exception_if(
> +                    (lock_prefix |
> +                    rep_prefix() |
> +                    (vex.pfx == vex_66)),
> +                    EXC_UD, -1);

The instruction reference makes no mention of any conditions like this.

Ravi> yes I will note that to be fixed - for now the best documentation I can 
point to is for an instruction in the same encoding group (see XSETBV or XTEST) 
which specifies #UD when prefixes LOCK, 66H, F3H or F2H are used.

The 3 conditions for #UD are being executed in non-root mode, the enable VM 
functions execution control is clear (which is how we would get here in the 
first place), or if eax is is >= 64.
The first needs an has_hvm_container() check, while the second and third can be 
left to ops->vmfunc() to handle.

Ravi>right, the required exec controls and register parameter checks are 
already done by ops->vmfunc(), and regarding the has_hvm_container check - I 
don't think that's needed because ops->vmfunc checks for altp2m being enabled 
and altp2m can be enabled for hvm domains only.

Thanks,
Ravi

~Andrew

> +                fail_if(ops->vmfunc == NULL);
> +                if ( (rc = ops->vmfunc(ctxt) != X86EMUL_OKAY) )
> +                    goto done;
> +                break;
> +            default:
> +                goto continue_grp7;
>          }
> +        break;
>  
> +continue_grp7:
>          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 36f1b74..595b399 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 (*ap2m_vcpu_update_eptp)(struct vcpu *v);
>      void (*ap2m_vcpu_update_vmfunc_ve)(struct vcpu *v);
>      bool_t (*ap2m_vcpu_emulate_ve)(struct vcpu *v);
> +    int (*ap2m_vcpu_emulate_vmfunc)(struct cpu_user_regs *regs);
>  };
>  
>  extern struct hvm_function_table hvm_funcs;


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