From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [RFC][PATCH v2 1/1] Add IOREQ_TYPE_VMWARE_PORT Date: Thu, 02 Oct 2014 23:23:31 +0100 Message-ID: <542DD063.2090400@citrix.com> References: <1412274977-6098-1-git-send-email-dslutz@verizon.com> <1412274977-6098-2-git-send-email-dslutz@verizon.com> <542DB6A5.7080207@citrix.com> <542DCA02.10304@terremark.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <542DCA02.10304@terremark.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Don Slutz , xen-devel@lists.xen.org Cc: Keir Fraser , Ian Campbell , Jan Beulich List-Id: xen-devel@lists.xenproject.org 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 >>> --- >>> 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 =3D current; >>> + struct hvm_vcpu_io *vio =3D &curr->arch.hvm_vcpu.hvm_io; >>> + vmware_ioreq_t p =3D { >>> + .type =3D IOREQ_TYPE_VMWARE_PORT, >>> + .addr =3D addr, >>> + .size =3D size, >>> + .dir =3D dir, >>> + .eax =3D regs->rax, >>> + .ebx =3D regs->rbx, >>> + .ecx =3D regs->rcx, >>> + .edx =3D regs->rdx, >>> + .esi =3D regs->rsi, >>> + .edi =3D regs->rdi, >>> + }; >>> + ioreq_t *pp =3D (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 > To: Don Slutz > CC: Stefano Stabellini , > qemu-devel@nongnu.org, xen-devel@lists.xensource.com, Alexander Graf > , Andreas F=E4rber , Anthony Liguori > , Marcel Apfelbaum , Markus > Armbruster , Michael S. Tsirkin > > > > 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 =3D { >> > > > + .type =3D IOREQ_TYPE_PIO, >> > > > + .addr =3D (uint16_t)req->size, >> > > > + .size =3D 4, >> > > > + .dir =3D IOREQ_READ, >> > > > + .df =3D 0, >> > > > + .data_is_ptr =3D 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