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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.07.15 at 20:09, <edmund.h.white@xxxxxxxxx> wrote:
> @@ -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;

curr

> +    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;
> +    }

Pointless braces.

> @@ -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;

The two changes don't fit together (and continue to look pointless
considering that the helper unconditionally returns
X86EMUL_EXCEPTION): != X86EMUL_OKAY doesn't mean
== X86EMUL_EXCEPTION.

> --- 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;
>  
> -        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 */

Case labels indented the same as the containing switch() please.

> +            case 0xd4: /* vmfunc */
> +                generate_exception_if(
> +                    (lock_prefix |
> +                    rep_prefix() |
> +                    (vex.pfx == vex_66)),
> +                    EXC_UD, -1);

FWIW, while Andrew pointed out that this doesn't match the doc, I
suppose it's the doc that's wrong here, so I would be inclined to
suggest keeping it as is. What I don't like though is the formatting -
why does this need to span across 5 lines?

> +                fail_if(ops->vmfunc == NULL);
> +                if ( (rc = ops->vmfunc(ctxt) != X86EMUL_OKAY) )
> +                    goto done;
> +                break;
> +            default:
> +                goto continue_grp7;
>          }
> +        break;
>  
> +continue_grp7:

Labels indented by at least one space please.

Jan


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