From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46635) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XZG7B-00033d-EN for qemu-devel@nongnu.org; Wed, 01 Oct 2014 05:22:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XZG76-0007ls-VA for qemu-devel@nongnu.org; Wed, 01 Oct 2014 05:22:29 -0400 Received: from smtp02.citrix.com ([66.165.176.63]:37143) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XZG76-0007lg-Ol for qemu-devel@nongnu.org; Wed, 01 Oct 2014 05:22:24 -0400 Date: Wed, 1 Oct 2014 10:20:16 +0100 From: Stefano Stabellini In-Reply-To: Message-ID: References: <1411757235-29128-1-git-send-email-dslutz@verizon.com> <1411757235-29128-2-git-send-email-dslutz@verizon.com> <5429FA0F.4050905@terremark.com> MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" 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: "Slutz, Donald Christopher" Cc: "xen-devel@lists.xensource.com" , Stefano Stabellini , Markus Armbruster , Marcel Apfelbaum , Alexander Graf , "qemu-devel@nongnu.org" , "Michael S. Tsirkin" , Anthony Liguori , =?UTF-8?Q?Andreas_F=C3=A4rber?= 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? 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. 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. > >>> 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 > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Stabellini Subject: Re: [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport Date: Wed, 1 Oct 2014 10:20:16 +0100 Message-ID: References: <1411757235-29128-1-git-send-email-dslutz@verizon.com> <1411757235-29128-2-git-send-email-dslutz@verizon.com> <5429FA0F.4050905@terremark.com> Mime-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org Sender: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org To: "Slutz, Donald Christopher" Cc: "xen-devel@lists.xensource.com" , Stefano Stabellini , Markus Armbruster , Marcel Apfelbaum , Alexander Graf , "qemu-devel@nongnu.org" , "Michael S. Tsirkin" , Anthony Liguori , =?UTF-8?Q?Andreas_F=C3=A4rber?= List-Id: xen-devel@lists.xenproject.org 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? 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. 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. > >>> 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 >