Re: [RFC][PATCH v2 1/1] Add IOREQ_TYPE_VMWARE_PORT

[Andrew Cooper <andrew.cooper3@citrix.com>]

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@verizon.com>
>>> ---
>>> 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@eu.citrix.com>
> To:     Don Slutz <dslutz@verizon.com>
> CC:     Stefano Stabellini <stefano.stabellini@eu.citrix.com>,
> qemu-devel@nongnu.org, xen-devel@lists.xensource.com, Alexander Graf
> <agraf@suse.de>, Andreas Färber <afaerber@suse.de>, Anthony Liguori
> <aliguori@amazon.com>, Marcel Apfelbaum <marcel.a@redhat.com>, Markus
> Armbruster <armbru@redhat.com>, Michael S. Tsirkin <mst@redhat.com>
>
>
>
> 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.

~Andrew