From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Sahita, Ravi" Subject: Re: [PATCH v4 07/15] VMX: add VMFUNC leaf 0 (EPTP switching) to emulator. Date: Sat, 11 Jul 2015 21:25:32 +0000 Message-ID: References: <1436489553-6300-1-git-send-email-edmund.h.white@intel.com> <1436489553-6300-8-git-send-email-edmund.h.white@intel.com> <559FACE0020000780008F53F@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Content-Language: en-US List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , "White, Edmund H" Cc: Tim Deegan , "Sahita, Ravi" , Wei Liu , George Dunlap , Andrew Cooper , Ian Jackson , "xen-devel@lists.xen.org" , "tlengyel@novetta.com" , Daniel De Graaf List-Id: xen-devel@lists.xenproject.org >From: Sahita, Ravi >Sent: Saturday, July 11, 2015 1:01 PM > >>From: Jan Beulich [mailto:JBeulich@suse.com] >>Sent: Friday, July 10, 2015 2:31 AM >> >>>>> On 10.07.15 at 02:52, 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