From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50205) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XZJ5w-0007FL-44 for qemu-devel@nongnu.org; Wed, 01 Oct 2014 08:33:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XZJ5p-0000KC-Nc for qemu-devel@nongnu.org; Wed, 01 Oct 2014 08:33:23 -0400 Received: from omzsmtpe04.verizonbusiness.com ([199.249.25.207]:10124) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XZJ5p-0000J5-Fv for qemu-devel@nongnu.org; Wed, 01 Oct 2014 08:33:17 -0400 From: Don Slutz Message-ID: <542BF484.3050909@terremark.com> Date: Wed, 01 Oct 2014 08:33:08 -0400 MIME-Version: 1.0 References: <1411757235-29128-1-git-send-email-dslutz@verizon.com> <1411757235-29128-2-git-send-email-dslutz@verizon.com> <5429FA0F.4050905@terremark.com> In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefano Stabellini , "Slutz, Donald Christopher" Cc: "xen-devel@lists.xensource.com" , Marcel Apfelbaum , Markus Armbruster , "Michael S. Tsirkin" , "qemu-devel@nongnu.org" , Alexander Graf , Anthony Liguori , =?windows-1252?Q?Andreas_F=E4rber?= 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 >>>>>> [...] >>>>>> >>>>>>> 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