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

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

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

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