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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.07.15 at 22:01, <ravi.sahita@xxxxxxxxx> wrote:
>> 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.

The reason for this would need to go into ...

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

... such a comment. And yes, I'd be as fine with just a comment as
with the wrapper being folded in.

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

Yes please.

Jan


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