From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43114) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XYuom-00023z-8I for qemu-devel@nongnu.org; Tue, 30 Sep 2014 06:38:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XYuof-0001X7-Vj for qemu-devel@nongnu.org; Tue, 30 Sep 2014 06:38:04 -0400 Received: from smtp.citrix.com ([66.165.176.89]:14896) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XYuof-0001Uo-Ml for qemu-devel@nongnu.org; Tue, 30 Sep 2014 06:37:57 -0400 Date: Tue, 30 Sep 2014 11:35:44 +0100 From: Stefano Stabellini In-Reply-To: <5429FA0F.4050905@terremark.com> 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: Don Slutz 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 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. > > > > + 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 :-) 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. > > 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: Tue, 30 Sep 2014 11:35:44 +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: <5429FA0F.4050905@terremark.com> 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: Don Slutz 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 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. > > > > + 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 :-) 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. > > 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 >