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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 2 Oct 2014, Andrew Cooper wrote:
> On 02/10/2014 22:56, Don Slutz wrote:
> > On 10/02/14 16:33, Andrew Cooper wrote:
> >> On 02/10/14 19:36, Don Slutz wrote:
> >>> Signed-off-by: Don Slutz <dslutz@xxxxxxxxxxx>
> >>> ---
> >>> v2:
> >>>      Fixup usage of hvmtrace_io_assist().
> >>>      VMware only changes the 32bit part of the register.
> >>>         Added vmware_ioreq_t
> >>>
> >>>   xen/arch/x86/hvm/emulate.c        | 72
> >>> +++++++++++++++++++++++++++++++++++++++
> >>>   xen/arch/x86/hvm/io.c             | 19 +++++++++++
> >>>   xen/arch/x86/hvm/vmware/vmport.c  | 24 ++++++++++++-
> >>>   xen/include/asm-x86/hvm/emulate.h |  2 ++
> >>>   xen/include/asm-x86/hvm/vcpu.h    |  1 +
> >>>   xen/include/public/hvm/ioreq.h    | 19 +++++++++++
> >>>   6 files changed, 136 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> >>> index c0f47d2..215f049 100644
> >>> --- a/xen/arch/x86/hvm/emulate.c
> >>> +++ b/xen/arch/x86/hvm/emulate.c
> >>> @@ -50,6 +50,78 @@ static void hvmtrace_io_assist(int is_mmio,
> >>> ioreq_t *p)
> >>>       trace_var(event, 0/*!cycles*/, size, buffer);
> >>>   }
> >>>   +int hvmemul_do_vmport(uint16_t addr, int size, int dir,
> >>> +                      struct cpu_user_regs *regs)
> >>> +{
> >>> +    struct vcpu *curr = current;
> >>> +    struct hvm_vcpu_io *vio = &curr->arch.hvm_vcpu.hvm_io;
> >>> +    vmware_ioreq_t p = {
> >>> +        .type = IOREQ_TYPE_VMWARE_PORT,
> >>> +        .addr = addr,
> >>> +        .size = size,
> >>> +        .dir = dir,
> >>> +        .eax = regs->rax,
> >>> +        .ebx = regs->rbx,
> >>> +        .ecx = regs->rcx,
> >>> +        .edx = regs->rdx,
> >>> +        .esi = regs->rsi,
> >>> +        .edi = regs->rdi,
> >>> +    };
> >>> +    ioreq_t *pp = (ioreq_t *)&p;
> >>> +    ioreq_t op;
> >> Eww.
> >>
> >> Just because the C type system lets you abuse it like this doesn't mean
> >> it is a clever idea to.  Please refer to c/s 15a9f34d1b as an example of
> >> the kinds of bugs it causes.
> >
> > This is a direct result of:
> >
> >
> > Subject:     Re: [PATCH 1/1] xen-hvm.c: Add support for Xen access to
> > vmport
> > Date:     Tue, 30 Sep 2014 11:35:44 +0100
> > From:     Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > To:     Don Slutz <dslutz@xxxxxxxxxxx>
> > CC:     Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>,
> > qemu-devel@xxxxxxxxxx, xen-devel@xxxxxxxxxxxxxxxxxxx, Alexander Graf
> > <agraf@xxxxxxx>, Andreas FÃrber <afaerber@xxxxxxx>, Anthony Liguori
> > <aliguori@xxxxxxxxxx>, Marcel Apfelbaum <marcel.a@xxxxxxxxxx>, Markus
> > Armbruster <armbru@xxxxxxxxxx>, Michael S. Tsirkin <mst@xxxxxxxxxx>
> >
> >
> >
> > On Mon, 29 Sep 2014, Don Slutz wrote:
> >> On 09/29/14 06:25, Stefano Stabellini wrote:
> >> > On Mon, 29 Sep 2014, Stefano Stabellini wrote:
> >> > > On Fri, 26 Sep 2014, Don Slutz wrote:
> >> > > > This adds synchronisation of the vcpu registers
> >> > > > between Xen and QEMU.
> >
> > ...
> >
> >> > > > +            CPUX86State *env;
> >> > > > +            ioreq_t fake_req = {
> >> > > > +                .type = IOREQ_TYPE_PIO,
> >> > > > +                .addr = (uint16_t)req->size,
> >> > > > +                .size = 4,
> >> > > > +                .dir = IOREQ_READ,
> >> > > > +                .df = 0,
> >> > > > +                .data_is_ptr = 0,
> >> > > > +            };
> >> > Why do you need a fake req?
> >>
> >> To transport the 6 VCPU registers (only 32bits of them) that vmport.c
> >> needs to do it's work.
> >>
> >> > Couldn't Xen send the real req instead?
> >>
> >> Yes, but then a 2nd exchange between QEMU and Xen would be needed
> >> to fetch the 6 VCPU registers.  The ways I know of to fetch the VCPU
> >> registers
> >> from Xen, all need many cycles to do their work and return
> >> a lot of data that is not needed.
> >>
> >> The other option that I have considered was to extend the ioreq_t type
> >> to have room for these registers, but that reduces by almost half the
> >> maximum number of VCPUs that are supported (They all live on 1 page).
> >
> > Urgh. Now that I understand the patch better is think it's horrible, no
> > offense :-)
> >
> > Why don't you add another new ioreq type to send out the vcpu state?
> > Something like IOREQ_TYPE_VCPU_SYNC_REGS? You could send it to QEMU
> > before IOREQ_TYPE_VMWARE_PORT. Actually this solution looks very imilar
> > to Alex's suggestion.
> >
> > ...
> >
> >
> > And the ASSERTs below are the attempt to prevent bugs from being added.
> >
> > Sigh.  Too much with both XEN and QEMU in hard freeze.  I may
> > have a way to avoid the cast.
> 
> I put 2 and 2 together to make something close to 4 after sending the
> first email, but unions are the C way of doing things like this.

I think that vmware_ioreq_t is still far from ideal but it is an
improvement over the previous version that was silently abusing the
ioreq_t struct. This version at least does that explicitly.

The issue with a union is compatibility with older QEMU versions: we can
introduce the union and retain compatibility only if we use anonymous
unions.  However I seem to recall Jan arguing against anonymous unions
in public interfaces in past.

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