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

From: "Jan Beulich" <JBeulich@suse.com>
To: Ravi Sahita <ravi.sahita@intel.com>
Cc: Tim Deegan <tim@xen.org>, Wei Liu <wei.liu2@citrix.com>,
	George Dunlap <george.dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	Edmund H White <edmund.h.white@intel.com>,
	"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>,
	"tlengyel@novetta.com" <tlengyel@novetta.com>,
	Daniel De Graaf <dgdegra@tycho.nsa.gov>
Subject: Re: [PATCH v4 07/15] VMX: add VMFUNC leaf 0 (EPTP switching) to emulator.
Date: Mon, 13 Jul 2015 08:13:34 +0100	[thread overview]
Message-ID: <55A3813E020000780008FF02@mail.emea.novell.com> (raw)
In-Reply-To: <DBC12B0F5509554280826E40BCDEE8BE54FD2FC1@ORSMSX104.amr.corp.intel.com>

>>> On 11.07.15 at 22:01, <ravi.sahita@intel.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>Sent: Friday, July 10, 2015 2:31 AM
>>
>>>>> On 10.07.15 at 02:52, <edmund.h.white@intel.com> 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