From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v4 04/16] xen: Add is_vmware_port_enabled Date: Fri, 12 Sep 2014 09:08:41 -0400 Message-ID: <5412F059.10003@oracle.com> References: <1410460610-14759-1-git-send-email-dslutz@verizon.com> <1410460610-14759-5-git-send-email-dslutz@verizon.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1410460610-14759-5-git-send-email-dslutz@verizon.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: Don Slutz , xen-devel@lists.xen.org Cc: Kevin Tian , Keir Fraser , Ian Campbell , Stefano Stabellini , Jun Nakajima , Eddie Dong , Ian Jackson , Tim Deegan , George Dunlap , Aravind Gopalakrishnan , Jan Beulich , Andrew Cooper , Suravee Suthikulpanit List-Id: xen-devel@lists.xenproject.org On 09/11/2014 02:36 PM, Don Slutz wrote: > int __get_instruction_length_from_list(struct vcpu *v, > - const enum instruction_index *list, unsigned int list_count) > + const enum instruction_index *list, > + unsigned int list_count, > + bool_t err_rpt) > { > struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb; > unsigned int i, j, inst_len = 0; > @@ -211,10 +222,13 @@ int __get_instruction_length_from_list(struct vcpu *v, > mismatch: ; > } > > - gdprintk(XENLOG_WARNING, > - "%s: Mismatch between expected and actual instruction bytes: " > - "eip = %lx\n", __func__, (unsigned long)vmcb->rip); > - hvm_inject_hw_exception(TRAP_gp_fault, 0); > + if ( err_rpt ) > + { > + gdprintk(XENLOG_WARNING, > + "%s: Mismatch between expected and actual instruction bytes: " > + "eip = %lx\n", __func__, (unsigned long)vmcb->rip); > + hvm_inject_hw_exception(TRAP_gp_fault, 0); > + } > return 0; > > done: > diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c > index b5188e6..9e14d2a 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 > + }; > + > + inst_len = __get_instruction_length_from_list( > + v, list, ARRAY_SIZE(list), 0); I should have asked earlier but I don't think I understand why the last argument here is 0 (and therefore why you have this last argument at all). Because whether or not you are warning in __get_instruction_length_from_list() it will still return 0. And that, in turn, will cause vmport_gp_check() to return an error. And then you will print another warning in VMPORT_LOG. So there is a warning anyway. Second, since this handler appears to be handling #GP only for VMware guest we should make sure that it is not executed for any other guest. You do now condition intercept got #GP for such guests only but I still think having a check here is worth doing. Maybe a BUG() or ASSERT()? The same comments are applicable to VMX code, I suspect. > + > + rc = vmport_gp_check(regs, v, inst_len, inst_addr, > + vmcb->exitinfo1, vmcb->exitinfo2); > + if ( !rc ) > + __update_guest_eip(regs, inst_len); > + else > + { > + VMPORT_DBG_LOG(VMPORT_LOG_GP_UNKNOWN, > + "gp: rc=%d ei1=0x%lx ei2=0x%lx ip=%"PRIx64 > + " (0x%lx,%ld) ax=%"PRIx64" bx=%"PRIx64" cx=%"PRIx64 > + " dx=%"PRIx64" si=%"PRIx64" di=%"PRIx64, rc, > + (unsigned long)vmcb->exitinfo1, > + (unsigned long)vmcb->exitinfo2, regs->rip, inst_addr, > + inst_len, regs->rax, regs->rbx, regs->rcx, regs->rdx, > + regs->rsi, regs->rdi); > + hvm_inject_hw_exception(TRAP_gp_fault, regs->error_code); > + } > +} > + ... > + > +int vmport_gp_check(struct cpu_user_regs *regs, struct vcpu *v, > + unsigned long inst_len, unsigned long inst_addr, > + unsigned long ei1, unsigned long ei2) > +{ > + if ( !v->domain->arch.hvm_domain.is_vmware_port_enabled ) > + return 10; > + > + if ( inst_len && inst_len <= 2 && get_low_bits(regs->rdx) == BDOOR_PORT && > + ei1 == 0 && ei2 == 0 && regs->error_code == 0 && > + (uint32_t)regs->rax == BDOOR_MAGIC ) > + { > + int i = 0; > + uint32_t val; > + uint32_t byte_cnt = 4; > + unsigned char bytes[2]; > + unsigned int fetch_len; > + int frc; > + int rc; > + > + /* > + * Fetch up to the next page break; we'll fetch from the > + * next page later if we have to. > + */ > + fetch_len = min_t(unsigned int, inst_len, > + PAGE_SIZE - (inst_addr & ~PAGE_MASK)); > + frc = hvm_fetch_from_guest_virt_nofault(bytes, inst_addr, fetch_len, > + PFEC_page_present); > + if ( frc != HVMCOPY_okay ) > + { > + gdprintk(XENLOG_WARNING, > + "Bad instruction fetch at %#lx (frc=%d il=%lu fl=%u)\n", > + (unsigned long) inst_addr, frc, inst_len, fetch_len); > + return 11; > + } > + if ( bytes[0] == 0x66 ) /* operand size prefix */ > + { > + byte_cnt = 2; > + i = 1; > + if ( fetch_len != inst_len ) > + { > + frc = hvm_fetch_from_guest_virt_nofault(&bytes[1], > + inst_addr + 1, 1, > + PFEC_page_present); > + if ( frc != HVMCOPY_okay ) > + { > + gdprintk(XENLOG_WARNING, > + "Bad instruction fetch at %#lx + 1 (frc=%d)\n", > + (unsigned long) inst_addr, frc); > + return 12; > + } > + } > + } > + if ( bytes[i] == 0xed ) /* in (%dx),%eax or in (%dx),%ax */ > + { > + rc = vmport_ioport(IOREQ_READ, BDOOR_PORT, byte_cnt, &val); > + VMPORT_DBG_LOG(VMPORT_LOG_GP_VMWARE_AFTER, > + "gp: VMwareIn rc=%d ip=%"PRIx64" byte_cnt=%d ax=%" > + PRIx64" bx=%"PRIx64" cx=%"PRIx64" dx=%"PRIx64 > + " si=%"PRIx64" di=%"PRIx64, rc, > + inst_addr, byte_cnt, regs->rax, regs->rbx, > + regs->rcx, regs->rdx, regs->rsi, regs->rdi); > + return rc; > + } > + else if ( bytes[i] == 0xec ) /* in (%dx),%al */ > + { > + rc = vmport_ioport(IOREQ_READ, BDOOR_PORT, 1, &val); > + VMPORT_DBG_LOG(VMPORT_LOG_GP_VMWARE_AFTER, > + "gp: VMwareIn rc=%d ip=%"PRIx64" byte_cnt=1 ax=%" > + PRIx64" bx=%"PRIx64" cx=%"PRIx64" dx=%"PRIx64 > + " si=%"PRIx64" di=%"PRIx64, rc, > + inst_addr, regs->rax, regs->rbx, regs->rcx, > + regs->rdx, regs->rsi, regs->rdi); > + return rc; > + } > + else if ( bytes[i] == 0xef ) /* out %eax,(%dx) or out %ax,(%dx) */ > + { > + rc = vmport_ioport(IOREQ_WRITE, BDOOR_PORT, byte_cnt, &val); > + VMPORT_DBG_LOG(VMPORT_LOG_GP_VMWARE_AFTER, > + "gp: VMwareOut rc=%d ip=%"PRIx64" byte_cnt=%d ax=%" > + PRIx64" bx=%"PRIx64" cx=%"PRIx64" dx=%"PRIx64 > + " si=%"PRIx64" di=%"PRIx64, rc, > + inst_addr, byte_cnt, regs->rax, regs->rbx, > + regs->rcx, regs->rdx, regs->rsi, regs->rdi); > + return rc; > + } > + else if ( bytes[i] == 0xee ) /* out %al,(%dx) */ > + { > + rc = vmport_ioport(IOREQ_WRITE, BDOOR_PORT, 1, &val); > + VMPORT_DBG_LOG(VMPORT_LOG_GP_VMWARE_AFTER, > + "gp: VMwareOut rc=%d ip=%"PRIx64" byte_cnt=1 ax=%" > + PRIx64" bx=%"PRIx64" cx=%"PRIx64" dx=%"PRIx64 > + " si=%"PRIx64" di=%"PRIx64, rc, > + inst_addr, regs->rax, regs->rbx, regs->rcx, > + regs->rdx, regs->rsi, regs->rdi); > + return rc; > + } > + else > + { > + VMPORT_DBG_LOG(VMPORT_LOG_GP_FAIL_RD_INST, > + "gp: VMware? lip=%"PRIx64"[%d]=>0x%x(%ld) ax=%" > + PRIx64" bx=%"PRIx64" cx=%"PRIx64" dx=%"PRIx64 > + " si=%"PRIx64" di=%"PRIx64, > + inst_addr, i, bytes[i], inst_len, regs->rax, > + regs->rbx, regs->rcx, regs->rdx, regs->rsi, > + regs->rdi); > + return 13; > + } > + } > + return 14; The return values should be defined as macros --- otherwise they look like some magic integers. -boris