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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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