Re: [Xen-devel] [RFC][PATCH v2x prototype 1/1] Add IOREQ_TYPE_VMWARE_PORT

[Don Slutz <dslutz@xxxxxxxxxxx>]

On 10/13/14 09:26, Paul Durrant wrote:
> > -----Original Message-----
> > From: Don Slutz [mailto:dslutz@xxxxxxxxxxx]
> > Sent: 09 October 2014 15:26
> > To: xen-devel@xxxxxxxxxxxxx; Paul Durrant
> > Cc: Jan Beulich; Keir (Xen.org); Ian Campbell; Don Slutz
> > Subject: [RFC][PATCH v2x prototype 1/1] Add IOREQ_TYPE_VMWARE_PORT
> >
> > This adds synchronisation of the 6 vcpu registers (only 32bits of
> > them) that vmport.c needs between Xen and QEMU.
> >
> > This is to avoid a 2nd and 3rd exchange between QEMU and Xen to
> > fetch and put these 6 vcpu registers used by the code in vmport.c
> > and vmmouse.c
> >
> > QEMU patch is named "xen-hvm.c: Add support for Xen access to vmport"
> >
> > Signed-off-by: Don Slutz <dslutz@xxxxxxxxxxx>
> > ---
> > As requested by Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> >
> > Here is a prototype of the QEMU change using a 2nd shared page.
> > I picked adding HVM_PARAM_VMPORT_IOREQ_PFN as the simple and
> > fast way to handle QEMU building on older Xen versions.
> >
> > There is xentrace and debug logging that is TBD for the Xen 4.6
> > submission of this.
...
> > diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> > index c0f47d2..4b8ea8f 100644
> > --- a/xen/arch/x86/hvm/emulate.c
> > +++ b/xen/arch/x86/hvm/emulate.c
> > @@ -52,12 +52,14 @@ static void hvmtrace_io_assist(int is_mmio, ioreq_t
> > *p)
> >
> >   static int hvmemul_do_io(
> >       int is_mmio, paddr_t addr, unsigned long *reps, int size,
> > -    paddr_t ram_gpa, int dir, int df, void *p_data)
> > +    paddr_t ram_gpa, int dir, int df, void *p_data,
> > +    struct cpu_user_regs *regs)
> >   {
> >       struct vcpu *curr = current;
> >       struct hvm_vcpu_io *vio;
> >       ioreq_t p = {
> > -        .type = is_mmio ? IOREQ_TYPE_COPY : IOREQ_TYPE_PIO,
> > +        .type = regs ? IOREQ_TYPE_VMWARE_PORT :
> > +                is_mmio ? IOREQ_TYPE_COPY : IOREQ_TYPE_PIO,
> >           .addr = addr,
> >           .size = size,
> >           .dir = dir,
> > @@ -65,11 +67,15 @@ static int hvmemul_do_io(
> >           .data = ram_gpa,
> >           .data_is_ptr = (p_data == NULL),
> >       };
> > +    vmware_ioreq_t vp;
> > +    vmware_ioreq_t *vpp;
> >       unsigned long ram_gfn = paddr_to_pfn(ram_gpa);
> >       p2m_type_t p2mt;
> >       struct page_info *ram_page;
> >       int rc;
> >
> > +    BUILD_BUG_ON(sizeof(ioreq_t) < sizeof(vmware_ioreq_t));
> > +
> >       /* Check for paged out page */
> >       ram_page = get_page_from_gfn(curr->domain, ram_gfn, &p2mt,
> > P2M_UNSHARE);
> >       if ( p2m_is_paging(p2mt) )
> > @@ -101,7 +107,17 @@ static int hvmemul_do_io(
> >           return X86EMUL_UNHANDLEABLE;
> >       }
> >
> > -    if ( !p.data_is_ptr && (dir == IOREQ_WRITE) )
> > +    if ( regs )
> > +    {
> > +        vpp = &vp;
> > +        p.data = regs->rax;
> > +        vp.ebx = regs->rbx;
> > +        vp.ecx = regs->rcx;
> > +        vp.edx = regs->rdx;
> > +        vp.esi = regs->rsi;
> > +        vp.edi = regs->rdi;
> > +    }
> > +    else if ( !p.data_is_ptr && (dir == IOREQ_WRITE) )
> >       {
> >           memcpy(&p.data, p_data, size);
> >           p_data = NULL;
> > @@ -161,7 +177,19 @@ static int hvmemul_do_io(
> >                   put_page(ram_page);
> >               return X86EMUL_RETRY;
> >           }
> > +    case HVMIO_awaiting_completion:
> > +        if ( regs )
> > +        {
> > +            /* May have to wait for previous cycle of a multi-write to 
> > complete.
> > */
> > +            if ( vio->mmio_retry ) {
> > +                gdprintk(XENLOG_WARNING, "WARNING: mmio_retry
> > io_state=%d?\n", vio->io_state);
> > +                return X86EMUL_RETRY;
> > +            }
> > +            /* These are expected if we get here via hvmemul_do_io() */
> > +            break;
> > +        }
> >       default:
> > +        gdprintk(XENLOG_WARNING, "WARNING: io_state=%d?\n", vio-
> > > io_state);
> >           if ( ram_page )
> >               put_page(ram_page);
> >           return X86EMUL_UNHANDLEABLE;
> > @@ -175,7 +203,7 @@ static int hvmemul_do_io(
> >           return X86EMUL_UNHANDLEABLE;
> >       }
> >
> > -    vio->io_state =
> > +    vio->io_state = regs ? HVMIO_handle_vmport_awaiting_completion :
> >           (p_data == NULL) ? HVMIO_dispatched : HVMIO_awaiting_completion;
> >       vio->io_size = size;
> >
> > @@ -197,6 +225,9 @@ static int hvmemul_do_io(
> >           if ( rc == X86EMUL_UNHANDLEABLE )
> >               rc = hvm_buffered_io_intercept(&p);
> >       }
> > +    else if ( regs ) {
> > +        rc = X86EMUL_UNHANDLEABLE;
> > +    }
> >       else
> >       {
> >           rc = hvm_portio_intercept(&p);
> > @@ -210,7 +241,7 @@ static int hvmemul_do_io(
> >           p.state = STATE_IORESP_READY;
> >           if ( !vio->mmio_retry )
> >           {
> > -            hvm_io_assist(&p);
> > +            hvm_io_assist(&p, vpp);
> >               vio->io_state = HVMIO_none;
> >           }
> >           else
> > @@ -226,13 +257,19 @@ static int hvmemul_do_io(
> >           }
> >           else
> >           {
> > -            rc = X86EMUL_RETRY;
> > -            if ( !hvm_send_assist_req(&p) )
> > +            if ( regs )
> > +                rc = X86EMUL_VMPORT_RETRY;
> > +            else
> > +                rc = X86EMUL_RETRY;
> > +            if ( !hvm_send_assist_req(&p, vpp) )
> >                   vio->io_state = HVMIO_none;
> >               else if ( p_data == NULL )
> >                   rc = X86EMUL_OKAY;
> >           }
> >           break;
> > +    case X86EMUL_VMPORT_RETRY:
> > +        rc = X86EMUL_RETRY;
> > +        break;
> >       default:
> >           BUG();
> >       }
> I still fail to see why you need to make such intrusive modifications to this 
> code.
I am confused.  The code is doing what you say:

>   I would have thought you could add a new ioreq type, as you've done, but not 
> make it 'data bearing'. I.e. you use your new VMPORT_IOREQ_PFN to carry the 
> register values back and forth between Xen and QEMU, but you still issue a 
> 'normal' ioreq_t structure (with your new type) via the 'normal' shared 
> IOREQ_PFN. That way you need do nothing to the majority of the emulation code 
> path - you'd just need to add code t copy the register values into and out of 
> your new shared page at start and completion of I/O.
I did adjust hvmemul_do_io() instead of duplicating it's code.
hvmemul_do_io() cannot be used without adjustment because of
the new type.

I will code this up with a new routine.

    -Don Slutz


>    Paul




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