From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH v4 07/15] VMX: add VMFUNC leaf 0 (EPTP switching) to emulator. Date: Fri, 10 Jul 2015 10:30:40 +0100 Message-ID: <559FACE0020000780008F53F@mail.emea.novell.com> References: <1436489553-6300-1-git-send-email-edmund.h.white@intel.com> <1436489553-6300-8-git-send-email-edmund.h.white@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1436489553-6300-8-git-send-email-edmund.h.white@intel.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ed White Cc: Tim Deegan , Ravi Sahita , 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 >>> 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. > --- 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). > @@ -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. 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