From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [RFC][PATCH v2 1/1] Add IOREQ_TYPE_VMWARE_PORT Date: Fri, 03 Oct 2014 15:27:20 -0400 Message-ID: <542EF898.5010107@terremark.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> <542DD063.2090400@citrix.com> <1412329912.423.23.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini Cc: Keir Fraser , Ian Campbell , Andrew Cooper , Don Slutz , xen-devel@lists.xen.org, Jan Beulich List-Id: xen-devel@lists.xenproject.org On 10/03/14 05:54, Stefano Stabellini wrote: > On Fri, 3 Oct 2014, Ian Campbell wrote: >> On Fri, 2014-10-03 at 10:47 +0100, Stefano Stabellini wrote: >>> The issue with a union is compatibility with older QEMU versions: we can >>> introduce the union and retain compatibility only if we use anonymous >>> unions. However I seem to recall Jan arguing against anonymous unions >>> in public interfaces in past. >> The canonical headers in xen/include/public are supposed to be strict >> ANSI C and anonymous unions are a gcc extension. >> >> However no-one is obliged to use this copy and several projects >> (including Linux, *BSD and others) take copies and modify them to suite >> their local coding styles/conventions etc. That could include using >> anonymous unions if that is preferable. I'm not sure if that helps you >> here though (since the issue AIUI is with existing qemu releases...) > Right, it doesn't help. > Even upstream QEMU still builds against external Xen header files. Here is a union that as far as I know is ANSI C and avoids a cast to a different type. But it is a lot of changes just to avoid this. Should I spend the time to make it part of this? From 84b4f8eb944d436b4e47e4024ebefbbe4460cd32 Mon Sep 17 00:00:00 2001 From: Don Slutz Date: Fri, 3 Oct 2014 15:15:11 -0400 Subject: [PATCH] Add union_ioreq_t Signed-off-by: Don Slutz --- xen/arch/x86/hvm/emulate.c | 90 +++++++++++++++++++++--------------------- xen/arch/x86/hvm/hvm.c | 73 +++++++++++++++++----------------- xen/arch/x86/hvm/io.c | 27 ++++++------- xen/include/asm-x86/hvm/hvm.h | 2 +- xen/include/asm-x86/hvm/io.h | 2 +- xen/include/public/hvm/ioreq.h | 11 ++++++ 6 files changed, 109 insertions(+), 96 deletions(-) diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index 5f8c551..cee8a37 100644 --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -55,20 +55,18 @@ int hvmemul_do_vmport(uint16_t addr, int size, int dir, { struct vcpu *curr = current; struct hvm_vcpu_io *vio = &curr->arch.hvm_vcpu.hvm_io; - vmware_ioreq_t p = { - .type = IOREQ_TYPE_VMWARE_PORT, - .addr = addr, - .size = size, - .dir = dir, - .eax = regs->rax, - .ebx = regs->rbx, - .ecx = regs->rcx, - .edx = regs->rdx, - .esi = regs->rsi, - .edi = regs->rdi, + union_ioreq_t up = { + .vreq.type = IOREQ_TYPE_VMWARE_PORT, + .vreq.addr = addr, + .vreq.size = size, + .vreq.dir = dir, + .vreq.eax = regs->rax, + .vreq.ebx = regs->rbx, + .vreq.ecx = regs->rcx, + .vreq.edx = regs->rdx, + .vreq.esi = regs->rsi, + .vreq.edi = regs->rdi, }; - ioreq_t *pp = (ioreq_t *)&p; - ioreq_t op; BUILD_BUG_ON(sizeof(ioreq_t) != sizeof(vmware_ioreq_t)); BUILD_BUG_ON(offsetof(ioreq_t, type) != offsetof(vmware_ioreq_t, type)); @@ -104,21 +102,25 @@ int hvmemul_do_vmport(uint16_t addr, int size, int dir, } else { - if ( !hvm_send_assist_req(pp) ) + if ( !hvm_send_assist_req(&up) ) vio->io_state = HVMIO_none; return X86EMUL_RETRY; } finish_access_vmport: - memset(&op, 0, sizeof(op)); - op.dir = dir; - op.addr = (uint16_t)regs->rdx; - op.data_is_ptr = 0; - op.data = vio->io_data; - hvmtrace_io_assist(0, &op); + { + ioreq_t op = { + .type = IOREQ_TYPE_PIO, + .dir = dir, + .addr = addr, + .data_is_ptr = 0, + .data = vio->io_data, + }; + + hvmtrace_io_assist(0, &op); + } memcpy(®s->rax, &vio->io_data, vio->io_size); - return X86EMUL_OKAY; } @@ -128,14 +130,14 @@ static int hvmemul_do_io( { struct vcpu *curr = current; struct hvm_vcpu_io *vio; - ioreq_t p = { - .type = is_mmio ? IOREQ_TYPE_COPY : IOREQ_TYPE_PIO, - .addr = addr, - .size = size, - .dir = dir, - .df = df, - .data = ram_gpa, - .data_is_ptr = (p_data == NULL), + union_ioreq_t up = { + .oreq.type = is_mmio ? IOREQ_TYPE_COPY : IOREQ_TYPE_PIO, + .oreq.addr = addr, + .oreq.size = size, + .oreq.dir = dir, + .oreq.df = df, + .oreq.data = ram_gpa, + .oreq.data_is_ptr = (p_data == NULL), }; unsigned long ram_gfn = paddr_to_pfn(ram_gpa); p2m_type_t p2mt; @@ -173,15 +175,15 @@ static int hvmemul_do_io( return X86EMUL_UNHANDLEABLE; } - if ( !p.data_is_ptr && (dir == IOREQ_WRITE) ) + if ( !up.oreq.data_is_ptr && (dir == IOREQ_WRITE) ) { - memcpy(&p.data, p_data, size); + memcpy(&up.oreq.data, p_data, size); p_data = NULL; } vio = &curr->arch.hvm_vcpu.hvm_io; - if ( is_mmio && !p.data_is_ptr ) + if ( is_mmio && !up.oreq.data_is_ptr ) { /* Part of a multi-cycle read or write? */ if ( dir == IOREQ_WRITE ) @@ -225,7 +227,7 @@ static int hvmemul_do_io( goto finish_access; case HVMIO_dispatched: /* May have to wait for previous cycle of a multi-write to complete. */ - if ( is_mmio && !p.data_is_ptr && (dir == IOREQ_WRITE) && + if ( is_mmio && !up.oreq.data_is_ptr && (dir == IOREQ_WRITE) && (addr == (vio->mmio_large_write_pa + vio->mmio_large_write_bytes)) ) { @@ -258,31 +260,31 @@ static int hvmemul_do_io( if ( vio->mmio_retrying ) *reps = 1; - p.count = *reps; + up.oreq.count = *reps; if ( dir == IOREQ_WRITE ) - hvmtrace_io_assist(is_mmio, &p); + hvmtrace_io_assist(is_mmio, &up.oreq); if ( is_mmio ) { - rc = hvm_mmio_intercept(&p); + rc = hvm_mmio_intercept(&up.oreq); if ( rc == X86EMUL_UNHANDLEABLE ) - rc = hvm_buffered_io_intercept(&p); + rc = hvm_buffered_io_intercept(&up.oreq); } else { - rc = hvm_portio_intercept(&p); + rc = hvm_portio_intercept(&up.oreq); } switch ( rc ) { case X86EMUL_OKAY: case X86EMUL_RETRY: - *reps = p.count; - p.state = STATE_IORESP_READY; + *reps = up.oreq.count; + up.oreq.state = STATE_IORESP_READY; if ( !vio->mmio_retry ) { - hvm_io_assist(&p); + hvm_io_assist(&up); vio->io_state = HVMIO_none; } else @@ -299,7 +301,7 @@ static int hvmemul_do_io( else { rc = X86EMUL_RETRY; - if ( !hvm_send_assist_req(&p) ) + if ( !hvm_send_assist_req(&up) ) vio->io_state = HVMIO_none; else if ( p_data == NULL ) rc = X86EMUL_OKAY; @@ -318,12 +320,12 @@ static int hvmemul_do_io( finish_access: if ( dir == IOREQ_READ ) - hvmtrace_io_assist(is_mmio, &p); + hvmtrace_io_assist(is_mmio, &up.oreq); if ( p_data != NULL ) memcpy(p_data, &vio->io_data, size); - if ( is_mmio && !p.data_is_ptr ) + if ( is_mmio && !up.oreq.data_is_ptr ) { /* Part of a multi-cycle read or write? */ if ( dir == IOREQ_WRITE ) diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 83a7000..d1f3e52 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -372,9 +372,9 @@ void hvm_migrate_pirqs(struct vcpu *v) spin_unlock(&d->event_lock); } -static ioreq_t *get_ioreq(struct hvm_ioreq_server *s, struct vcpu *v) +static union_ioreq_t *get_ioreq(struct hvm_ioreq_server *s, struct vcpu *v) { - shared_iopage_t *p = s->ioreq.va; + union_shared_iopage_t *p = s->ioreq.va; ASSERT((v == current) || !vcpu_runnable(v)); ASSERT(p != NULL); @@ -391,34 +391,35 @@ bool_t hvm_io_pending(struct vcpu *v) &d->arch.hvm_domain.ioreq_server.list, list_entry ) { - ioreq_t *p = get_ioreq(s, v); + union_ioreq_t *up = get_ioreq(s, v); - if ( p->state != STATE_IOREQ_NONE ) + if ( up->oreq.state != STATE_IOREQ_NONE ) return 1; } return 0; } -static bool_t hvm_wait_for_io(struct hvm_ioreq_vcpu *sv, ioreq_t *p) +static bool_t hvm_wait_for_io(struct hvm_ioreq_vcpu *sv, + union_ioreq_t *up) { - /* NB. Optimised for common case (p->state == STATE_IOREQ_NONE). */ - while ( p->state != STATE_IOREQ_NONE ) + /* NB. Optimised for common case (up->oreq.state == STATE_IOREQ_NONE). */ + while ( up->oreq.state != STATE_IOREQ_NONE ) { - switch ( p->state ) + switch ( up->oreq.state ) { case STATE_IORESP_READY: /* IORESP_READY -> NONE */ rmb(); /* see IORESP_READY /then/ read contents of ioreq */ - hvm_io_assist(p); + hvm_io_assist(up); break; case STATE_IOREQ_READY: /* IOREQ_{READY,INPROCESS} -> IORESP_READY */ case STATE_IOREQ_INPROCESS: wait_on_xen_event_channel(sv->ioreq_evtchn, - (p->state != STATE_IOREQ_READY) && - (p->state != STATE_IOREQ_INPROCESS)); + (up->oreq.state != STATE_IOREQ_READY) && + (up->oreq.state != STATE_IOREQ_INPROCESS)); break; default: - gdprintk(XENLOG_ERR, "Weird HVM iorequest state %d.\n", p->state); + gdprintk(XENLOG_ERR, "Weird HVM iorequest state %d.\n", up->oreq.state); domain_crash(sv->vcpu->domain); return 0; /* bail */ } @@ -598,9 +599,9 @@ static void hvm_update_ioreq_evtchn(struct hvm_ioreq_server *s, if ( s->ioreq.va != NULL ) { - ioreq_t *p = get_ioreq(s, sv->vcpu); + union_ioreq_t *up = get_ioreq(s, sv->vcpu); - p->vp_eport = sv->ioreq_evtchn; + up->oreq.vp_eport = sv->ioreq_evtchn; } } @@ -2526,27 +2527,27 @@ bool_t hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s, if ( sv->vcpu == curr ) { evtchn_port_t port = sv->ioreq_evtchn; - ioreq_t *p = get_ioreq(s, curr); + union_ioreq_t *up = get_ioreq(s, curr); - if ( unlikely(p->state != STATE_IOREQ_NONE) ) + if ( unlikely(up->oreq.state != STATE_IOREQ_NONE) ) { gdprintk(XENLOG_ERR, "Device model set bad IO state %d.\n", - p->state); + up->oreq.state); goto crash; } - if ( unlikely(p->vp_eport != port) ) + if ( unlikely(up->oreq.vp_eport != port) ) { gdprintk(XENLOG_ERR, "Device model set bad event channel %d.\n", - p->vp_eport); + up->oreq.vp_eport); goto crash; } proto_p->state = STATE_IOREQ_NONE; proto_p->vp_eport = port; - *p = *proto_p; + up->oreq = *proto_p; prepare_wait_on_xen_event_channel(port); @@ -2555,7 +2556,7 @@ bool_t hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s, * contents. prepare_wait_on_xen_event_channel() is an implicit * barrier. */ - p->state = STATE_IOREQ_READY; + up->oreq.state = STATE_IOREQ_READY; notify_via_xen_event_channel(d, port); break; } @@ -2568,44 +2569,44 @@ bool_t hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s, return 0; } -static bool_t hvm_complete_assist_req(ioreq_t *p) +static bool_t hvm_complete_assist_req(union_ioreq_t *up) { - switch ( p->type ) + switch ( up->oreq.type ) { case IOREQ_TYPE_COPY: case IOREQ_TYPE_PIO: - if ( p->dir == IOREQ_READ ) + if ( up->oreq.dir == IOREQ_READ ) { - if ( !p->data_is_ptr ) - p->data = ~0ul; + if ( !up->oreq.data_is_ptr ) + up->oreq.data = ~0ul; else { - int i, step = p->df ? -p->size : p->size; + int i, step = up->oreq.df ? -up->oreq.size : up->oreq.size; uint32_t data = ~0; - for ( i = 0; i < p->count; i++ ) - hvm_copy_to_guest_phys(p->data + step * i, &data, - p->size); + for ( i = 0; i < up->oreq.count; i++ ) + hvm_copy_to_guest_phys(up->oreq.data + step * i, &data, + up->oreq.size); } } /* FALLTHRU */ default: - p->state = STATE_IORESP_READY; - hvm_io_assist(p); + up->oreq.state = STATE_IORESP_READY; + hvm_io_assist(up); break; } return 1; } -bool_t hvm_send_assist_req(ioreq_t *p) +bool_t hvm_send_assist_req(union_ioreq_t *up) { - struct hvm_ioreq_server *s = hvm_select_ioreq_server(current->domain, p); + struct hvm_ioreq_server *s = hvm_select_ioreq_server(current->domain, &up->oreq); if ( !s ) - return hvm_complete_assist_req(p); + return hvm_complete_assist_req(up); - return hvm_send_assist_req_to_ioreq_server(s, p); + return hvm_send_assist_req_to_ioreq_server(s, &up->oreq); } void hvm_broadcast_assist_req(ioreq_t *p) diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c index 5d7d221..a06a507 100644 --- a/xen/arch/x86/hvm/io.c +++ b/xen/arch/x86/hvm/io.c @@ -169,13 +169,13 @@ int handle_pio(uint16_t port, unsigned int size, int dir) return 1; } -void hvm_io_assist(ioreq_t *p) +void hvm_io_assist(union_ioreq_t *up) { struct vcpu *curr = current; struct hvm_vcpu_io *vio = &curr->arch.hvm_vcpu.hvm_io; enum hvm_io_state io_state; - p->state = STATE_IOREQ_NONE; + up->oreq.state = STATE_IOREQ_NONE; io_state = vio->io_state; vio->io_state = HVMIO_none; @@ -184,39 +184,38 @@ void hvm_io_assist(ioreq_t *p) { case HVMIO_awaiting_completion: vio->io_state = HVMIO_completed; - vio->io_data = p->data; + vio->io_data = up->oreq.data; break; case HVMIO_handle_mmio_awaiting_completion: vio->io_state = HVMIO_completed; - vio->io_data = p->data; + vio->io_data = up->oreq.data; (void)handle_mmio(); break; case HVMIO_handle_pio_awaiting_completion: if ( vio->io_size == 4 ) /* Needs zero extension. */ - guest_cpu_user_regs()->rax = (uint32_t)p->data; + guest_cpu_user_regs()->rax = (uint32_t)up->oreq.data; else - memcpy(&guest_cpu_user_regs()->rax, &p->data, vio->io_size); + memcpy(&guest_cpu_user_regs()->rax, &up->oreq.data, vio->io_size); break; case HVMIO_handle_vmport_awaiting_completion: { struct cpu_user_regs *regs = guest_cpu_user_regs(); - vmware_ioreq_t *vp = (vmware_ioreq_t *)p; /* Always zero extension for eax */ - regs->rax = vp->eax; + regs->rax = up->vreq.eax; /* Only change the 32bit part of the register */ - regs->_ebx = vp->ebx; - regs->_ecx = vp->ecx; - regs->_edx = vp->edx; - regs->_esi = vp->esi; - regs->_edi = vp->edi; + regs->_ebx = up->vreq.ebx; + regs->_ecx = up->vreq.ecx; + regs->_edx = up->vreq.edx; + regs->_esi = up->vreq.esi; + regs->_edi = up->vreq.edi; } break; default: break; } - if ( p->state == STATE_IOREQ_NONE ) + if ( up->oreq.state == STATE_IOREQ_NONE ) { msix_write_completion(curr); vcpu_end_shutdown_deferral(curr); diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h index 0910147..844271c 100644 --- a/xen/include/asm-x86/hvm/hvm.h +++ b/xen/include/asm-x86/hvm/hvm.h @@ -228,7 +228,7 @@ int hvm_vcpu_cacheattr_init(struct vcpu *v); void hvm_vcpu_cacheattr_destroy(struct vcpu *v); void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip); -bool_t hvm_send_assist_req(ioreq_t *p); +bool_t hvm_send_assist_req(union_ioreq_t *up); void hvm_broadcast_assist_req(ioreq_t *p); void hvm_get_guest_pat(struct vcpu *v, u64 *guest_pat); diff --git a/xen/include/asm-x86/hvm/io.h b/xen/include/asm-x86/hvm/io.h index d257161..1a183a1 100644 --- a/xen/include/asm-x86/hvm/io.h +++ b/xen/include/asm-x86/hvm/io.h @@ -123,7 +123,7 @@ int handle_mmio_with_translation(unsigned long gva, unsigned long gpfn, struct npfec); int handle_pio(uint16_t port, unsigned int size, int dir); void hvm_interrupt_post(struct vcpu *v, int vector, int type); -void hvm_io_assist(ioreq_t *p); +void hvm_io_assist(union_ioreq_t *p); void hvm_dpci_eoi(struct domain *d, unsigned int guest_irq, const union vioapic_redir_entry *ent); void msix_write_completion(struct vcpu *); diff --git a/xen/include/public/hvm/ioreq.h b/xen/include/public/hvm/ioreq.h index 7d5ba58..d75ce66 100644 --- a/xen/include/public/hvm/ioreq.h +++ b/xen/include/public/hvm/ioreq.h @@ -85,11 +85,22 @@ struct vmware_ioreq { }; typedef struct vmware_ioreq vmware_ioreq_t; +union union_ioreq { + ioreq_t oreq; + vmware_ioreq_t vreq; +}; +typedef union union_ioreq union_ioreq_t; + struct shared_iopage { struct ioreq vcpu_ioreq[1]; }; typedef struct shared_iopage shared_iopage_t; +struct union_shared_iopage { + union union_ioreq vcpu_ioreq[1]; +}; +typedef struct union_shared_iopage union_shared_iopage_t; + struct buf_ioreq { uint8_t type; /* I/O type */ uint8_t pad:1; -- 1.7.11.7 -Don Slutz