Re: [Xen-devel] [PATCH v3 04/16] hypervisor part of add vmware_port to xl.cfg

[Don Slutz <dslutz@xxxxxxxxxxx>]

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 <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
> > +    };
> > +
> > +    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);
> > +    }
> > +}
> > +

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel