From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [PATCH v3 04/16] hypervisor part of add vmware_port to xl.cfg Date: Mon, 08 Sep 2014 13:20:16 -0400 Message-ID: <540DE550.1020700@terremark.com> References: <1410182158-8542-1-git-send-email-dslutz@verizon.com> <1410182158-8542-5-git-send-email-dslutz@verizon.com> <540DC4B6.1000707@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <540DC4B6.1000707@oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Boris Ostrovsky , Don Slutz , xen-devel@lists.xen.org Cc: Kevin Tian , Keir Fraser , Ian Campbell , Stefano Stabellini , Jun Nakajima , Eddie Dong , Ian Jackson , Tim Deegan , Aravind Gopalakrishnan , Jan Beulich , Andrew Cooper , Suravee Suthikulpanit List-Id: xen-devel@lists.xenproject.org On 09/08/14 11:01, Boris Ostrovsky wrote: > On 09/08/2014 09:15 AM, Don Slutz wrote: >> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c >> index b5188e6..12079be 100644 >> --- a/xen/arch/x86/hvm/svm/svm.c >> +++ b/xen/arch/x86/hvm/svm/svm.c >> @@ -59,6 +59,7 @@ >> #include >> #include >> #include >> +#include >> #include >> #include >> #include >> @@ -2065,6 +2066,38 @@ svm_vmexit_do_vmsave(struct vmcb_struct *vmcb, >> return; >> } >> +static void svm_vmexit_gp_intercept(struct cpu_user_regs *regs, >> + struct vcpu *v) >> +{ >> + struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb; >> + unsigned long inst_len; >> + unsigned long inst_addr = svm_rip2pointer(v); >> + int rc; >> + static const enum instruction_index list[] = { >> + INSTR_INL_DX, INSTR_INB_DX, INSTR_OUTL_DX, INSTR_OUTB_DX >> + }; >> + >> + regs->error_code = vmcb->exitinfo1; > > I am not sure this is a good idea. I have a feeling this may mess up > fault reporting in case of double faults (e.g. see AMD volume 2 > section 15.2, Example paragraph). > > Do you really need to save it into regs or can out pass exitinfo1 as > error_code argument directly to vmport_gp_check()? > Nope. I can pass this on. >> + inst_len = __get_instruction_length_from_list( >> + v, list, ARRAY_SIZE(list), 0); >> + >> + rc = vmport_gp_check(regs, v, inst_len, inst_addr, >> vmcb->exitinfo2); > > You probably want to call this only when > __get_instruction_length_from_list() succeeded (i.e. instr_len >0) > This check is inside the vmport_gp_check(). I can extract it to both callers if that makes sense. > What happens when you have a non-VMware-aware guest that performs this > access? Prior to this patch it would get a #GP but now it will > continue happily running, right? This changes user-visible behavior. > It depends on the setting of vmware_port. It will still get a #GP if vmware_port is zero. And yes when the config of a guest is changed to include vmware_port, this changes user-visible behavior. > I wonder whether we should enable #GP intercepts only when we know > that the guest is VMware-aware (which we do as far as I can tell since > we have a config option). > This may make sense. The way the code is now, the only change is how long it takes to get a #GP when vmware_port is zero. I did some looking into enabling and disabling #GP intercepts but did not come up with a simple way at the time. I also thought about it and came to the conclusion and the number of #GPs that a guest takes which is not VMware-aware is very few. So add a lot of complex code seemed to me to be worse. I can look at it again. -Don Slutz > -boris > > >> + if ( !rc ) >> + __update_guest_eip(regs, inst_len); >> + else >> + { >> + VMPORT_DBG_LOG(VMPORT_LOG_GP_UNKNOWN, >> + "gp: rc=%d e2=%lx ec=%lx ip=%"PRIx64" (%ld) >> ax=%"PRIx64 >> + " bx=%"PRIx64" cx=%"PRIx64" dx=%"PRIx64" >> si=%"PRIx64 >> + " di=%"PRIx64, rc, >> + (unsigned long)vmcb->exitinfo2, >> + (unsigned long)regs->error_code, >> + regs->rip, inst_len, regs->rax, regs->rbx, >> regs->rcx, >> + regs->rdx, regs->rsi, regs->rdi); >> + hvm_inject_hw_exception(TRAP_gp_fault, regs->error_code); >> + } >> +} >> + >> >