Re: [Xen-devel] [PATCH v4 04/16] xen: Add is_vmware_port_enabled

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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?
-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