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

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

>From: Sahita, Ravi
>Sent: Saturday, July 11, 2015 1:01 PM
>
>>From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>>Sent: Friday, July 10, 2015 2:31 AM
>>
>>>>> 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.
>
>Sure I can add the other error conditions but note that they will be handled as
>EXCEPTION. Let me explain the point of the relatively pointless :-) helper was
>to have the interface complete so that if someone in the future wanted to
>handle VMFUNC exits (perhaps for lazily managing EPTP list for nesting
>scenarios) they could do that by extending the vmx_vmfunc_intercept. I can
>also add a comment there - Will that be sufficient? (I'm trying to avoid 
>another
>revision after I revise it to add the other exception conditions as stated)
>
>>
>>> --- 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).
>>
>
>I thought it would be better to specify instructions that are unique to a 
>specific
>CPU.
>But I can remove it.
>
>>> @@ -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.
>>
>
>Sure - no issues with that.
>

On second thoughts, I cannot really use a goto done for these 2 cases since 
that will skip the single-step tracing check that's performed in the existing 
flow.
So I can add a new label entrypoint before the tracing check, or goto writeback 
(with the dst.type switch there being a wasted check), or I can keep the flow 
as is - which would you prefer?

Thanks,
Ravi

>Thanks,
>Ravi
>
>>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