Re: [Qemu-devel] [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Don Slutz <dslutz@verizon.com>]

On 10/01/14 05:20, Stefano Stabellini wrote:
> On Wed, 1 Oct 2014, Slutz, Donald Christopher wrote:
>> On 09/30/14 06:35, Stefano Stabellini wrote:
>>> 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.
>>>>>>>
>>>>>>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>>>>>> [...]
>>>>>>
>>>>>>> diff --git a/xen-hvm.c b/xen-hvm.c
>>>>>>> index 05e522c..e1274bb 100644
>>>>>>> --- a/xen-hvm.c
>>>>>>> +++ b/xen-hvm.c
>>>>>>> @@ -857,14 +857,48 @@ static void cpu_handle_ioreq(void *opaque)
>>>>>>>           handle_buffered_iopage(state);
>>>>>>>         if (req) {
>>>>>>> +#ifdef IOREQ_TYPE_VMWARE_PORT
>>>>>> Is there any reason to #ifdef this code?
>>>>>> Couldn't we just always build it in QEMU?
>>>> This allows QEMU 2.2 (or later) to build on a system that has Xen 4.5
>>>> or earlier installed; and work.
>>> I would rather remove the #ifdef from here and add any necessary
>>> compatibility stuff to include/hw/xen/xen_common.h.
>>>
>> Ok, will do.
>>
>>>>>>> +        if (req->type == IOREQ_TYPE_VMWARE_PORT) {
>>>>>> I think it would make more sense to check for IOREQ_TYPE_VMWARE_PORT
>>>>>> from within handle_ioreq.
>>>>>>
>>>> Ok, I can move it.
>>>>
>>>>
>>>>>>> +            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 :-)
>> None taken.
>>
>>> 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.
>>>
>> I can, it is just slower.  This would require 2 new types.  1 for regs to
>> QEMU, 1 for regs from QEMU.  So instead of 1 round trip (Xen to QEMU
>> to Xen), you now have 3 (Xen to QEMU (regs to QEMU) to Xen, Xen to
>> QEMU (PIO) to Xen, Xen to QEMU (regs from QEMU) to Xen).
> This is not an high performance device, is it?

Depends on how you view mouse device speed.

> In any case I agree it would be better to avoid it.
> I wonder if we could send both ioreqs at once from Xen and back from
> QEMU. Or maybe append the registers to IOREQ_TYPE_VMWARE_PORT, changing
> the size of ioreq_t only for this ioreq type.

No idea how do do this since this is an array in a shared page.

> Another maybe simpler possibility would be introducing a union in
> ioreq_t with the registers.
> Anything would be OK for me but I would like to see the registers being
> passes explicitely by Xen rather than "hiding" them into other ioreq
> fields.
>

I considered a union, but this can be a issue with older QEMU
and newer Xen.  Since it appears that you care, I will add a new
struct for this.  The problem is that it is a "union" and so must
match on the common fields.

    -Don Slutz



>>>>>     If any case you should spend a
>>>>> few more words on why you are doing this.
>>>>>
>>>> Sure.  Will add some form of the above as part of the commit message.
>>>>
>>>>>>> +            if (!xen_opaque_env) {
>>>>>>> +                xen_opaque_env = g_malloc(sizeof(CPUX86State));
>>>>>>> +            }
>>>>>>> +            env = xen_opaque_env;
>>>>>>> +            env->regs[R_EAX] = (uint32_t)(req->addr >> 32);
>>>>>>> +            env->regs[R_EBX] = (uint32_t)(req->addr);
>>>>>>> +            env->regs[R_ECX] = req->count;
>>>>>>> +            env->regs[R_EDX] = req->size;
>>>>>>> +            env->regs[R_ESI] = (uint32_t)(req->data >> 32);
>>>>>>> +            env->regs[R_EDI] = (uint32_t)(req->data);
>>>>>>> +            handle_ioreq(&fake_req);
>>>>>>> +            req->addr = ((uint64_t)fake_req.data << 32) |
>>>>>>> +                (uint32_t)env->regs[R_EBX];
>>>>>>> +            req->count = env->regs[R_ECX];
>>>>>>> +            req->size = env->regs[R_EDX];
>>>>>>> +            req->data = ((uint64_t)env->regs[R_ESI] << 32) |
>>>>>>> +                (uint32_t)env->regs[R_EDI];
>>>>>> This code could be moved to a separate helper function called by
>>>>>> handle_ioreq.
>>>>>>
>>>> Ok.
>>>>
>>>>      -Don Slutz
>>>>
>>>>>>> +        } else {
>>>>>>> +            handle_ioreq(req);
>>>>>>> +        }
>>>>>>> +#else
>>>>>>>             handle_ioreq(req);
>>>>>>> +#endif