[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 04/16] xen: Add is_vmware_port_enabled
On 09/17/14 11:56, Boris Ostrovsky wrote: > > On 09/16/2014 08:08 AM, Slutz, Donald Christopher wrote: >> On 09/12/14 09:08, Boris Ostrovsky wrote: >>> 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 <public/sched.h> >>>> #include <asm/hvm/vpt.h> >>>> #include <asm/hvm/trace.h> >>>> +#include <asm/hvm/vmport.h> >>>> #include <asm/hap.h> >>>> #include <asm/apic.h> >>>> #include <asm/debugger.h> >>>> @@ -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. >>> >> A key part that you appear to have missed is that >> __get_instruction_length_from_list() uses gdprintk(XENLOG_WARNING,... >> but VMPORT_DBG_LOG is only available in debug=y builds. So the new >> last argument is used to control this. Since this change includes >> enabling #GP vmexits, it is now possible for ring 3 users to generate at >> large volume of these which with gdprintk() can flood the console. > > Would it be possible to decide where and whether to print the warning > inside __get_instruction_length_from_list() as opposed to passing a > new parameter? E.g. if vmware_port_enabled is set and list includes > IN/OUT and possibly something else? > It could be. However the test is complex and not something I am willing to be part of this patch. So the only thing I have come up with is using #ifndef NDEBUG around the gdprintk(). This leaves the hvm_inject_hw_exception(TRAP_gp_fault, 0); Which is not correct in this case. Now are far as I can tell, calling this twice does the wrong thing for this case. I.E. turns it into a double fault. For #GP I need to call it with vmcb->exitinfo1 instead of 0. (Note: v5 posted before I got this.) I just spent some time checking and found out that even with the cpu reporting: (XEN) - Next-RIP Saved on #VMEXIT svm_nextrip_insn_length(v) is 0. (hyper-0-21-54:~>cat /proc/cpuinfo processor : 0 vendor_id : AuthenticAMD cpu family : 21 model : 2 model name : AMD Opteron(tm) Processor 4365 EE stepping : 0 microcode : 0x600081c ...) Which means to me that by using __get_instruction_length_from_list() I am reading the instruction bytes 2 times. Once in __get_instruction_length_from_list() and once in vmport_gp_check(). This is not too bad since the rate of these is low. But this does suggest to me that the change to using __get_instruction_length_from_list() was not the best. Having a routine that both checks the instruction and reports on which one was found would be much better. So do I go with v5, or dropping the use of __get_instruction_length_from_list() in a v6? (The coding of the new routine will take some time.) -Don Slutz > -boris > >> >> >>> 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. >>> >> I will change the check in vmport_gp_check on is_vmware_port_enabled >> into an ASSERT() so both SVM and VMX will be covered. >> >>>> + >>>> + 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); >>>> + } >>>> +} >>>> + >>> . > _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |