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

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



>>> On 10.07.15 at 02:52, <edmund.h.white@xxxxxxxxx> wrote:
> @@ -3234,6 +3256,13 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>              update_guest_eip();
>          break;
>  
> +    case EXIT_REASON_VMFUNC:
> +        if ( vmx_vmfunc_intercept(regs) == X86EMUL_EXCEPTION )
> +            hvm_inject_hw_exception(TRAP_invalid_op, 
> HVM_DELIVER_NO_ERROR_CODE);
> +        else
> +            update_guest_eip();
> +        break;

How about X86EMUL_UNHANDLEABLE and X86EMUL_RETRY? As said
before, either get this right, or simply fold the relatively pointless
helper into here.

> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -3816,8 +3816,9 @@ x86_emulate(
>          struct segment_register reg;
>          unsigned long base, limit, cr0, cr0w;
>  
> -        if ( modrm == 0xdf ) /* invlpga */
> +        switch( modrm )
>          {
> +        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);

The diff now looks much better. Yet I don't see why you added "AMD"
to the comment - we don't elsewhere note that certain instructions
are vendor specific (and really which ones are also changes over time,
see RDTSCP for a prominent example).

> @@ -3825,10 +3826,7 @@ x86_emulate(
>                                     ctxt)) )
>                  goto done;
>              break;
> -        }
> -
> -        if ( modrm == 0xf9 ) /* rdtscp */
> -        {
> +        case 0xf9: /* rdtscp */ {
>              uint64_t tsc_aux;
>              fail_if(ops->read_msr == NULL);
>              if ( (rc = ops->read_msr(MSR_TSC_AUX, &tsc_aux, ctxt)) != 0 )
> @@ -3836,7 +3834,19 @@ x86_emulate(
>              _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);
> +            fail_if(ops->vmfunc == NULL);
> +            if ( (rc = ops->vmfunc(ctxt) != X86EMUL_OKAY) )
> +                goto done;
> +            break;
> +        default:
> +            goto continue_grp7;
> +        }
> +        break;
>  
> + continue_grp7:

Already when first looking at this I disliked this label. Looking at it
again, I'd really like to see it gone: RDTSCP handling already ends
in a goto. Since the only VMFUNC currently implemented doesn't
modify any register state either, its handling could end in an
unconditional "goto done" too for now. And INVLPG, not modifying
any register state, could follow suit.

And even if you really wanted to cater for future VMFUNCs to alter
register state, I'd still like this ugliness to be avoided - e.g. by
setting rc to a negative value before the switch and break-ing
afterwards when it's no longer negative.

Jan


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


 


Rackspace

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