From: Don Slutz <dslutz@verizon.com>
To: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Keir Fraser <keir@xen.org>,
Ian Campbell <Ian.Campbell@citrix.com>,
Andrew Cooper <andrew.cooper3@citrix.com>,
Don Slutz <dslutz@verizon.com>,
xen-devel@lists.xen.org, Jan Beulich <jbeulich@suse.com>
Subject: Re: [RFC][PATCH v2 1/1] Add IOREQ_TYPE_VMWARE_PORT
Date: Fri, 03 Oct 2014 15:27:20 -0400 [thread overview]
Message-ID: <542EF898.5010107@terremark.com> (raw)
In-Reply-To: <alpine.DEB.2.02.1410031052390.17038@kaball.uk.xensource.com>
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 <dslutz@verizon.com>
Date: Fri, 3 Oct 2014 15:15:11 -0400
Subject: [PATCH] Add union_ioreq_t
Signed-off-by: Don Slutz <dslutz@verizon.com>
---
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
next prev parent reply other threads:[~2014-10-03 19:27 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-10-02 18:36 [RFC][PATCH v2 0/1] Add support for Xen access to vmport Don Slutz
2014-10-02 18:36 ` [RFC][PATCH v2 1/1] Add IOREQ_TYPE_VMWARE_PORT Don Slutz
2014-10-02 20:33 ` Andrew Cooper
2014-10-02 21:56 ` Don Slutz
2014-10-02 22:23 ` Andrew Cooper
2014-10-03 9:47 ` Stefano Stabellini
2014-10-03 9:51 ` Ian Campbell
2014-10-03 9:54 ` Stefano Stabellini
2014-10-03 19:27 ` Don Slutz [this message]
2014-10-06 7:55 ` Jan Beulich
2014-10-06 9:21 ` Stefano Stabellini
2014-10-06 9:39 ` Jan Beulich
2014-10-06 19:51 ` Don Slutz
2014-10-07 8:05 ` Jan Beulich
2014-10-06 9:54 ` Paul Durrant
2014-10-03 9:29 ` Paul Durrant
2014-10-03 19:44 ` Don Slutz
2014-10-03 9:46 ` Paul Durrant
2014-10-03 19:56 ` Don Slutz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=542EF898.5010107@terremark.com \
--to=dslutz@verizon.com \
--cc=Ian.Campbell@citrix.com \
--cc=andrew.cooper3@citrix.com \
--cc=jbeulich@suse.com \
--cc=keir@xen.org \
--cc=stefano.stabellini@eu.citrix.com \
--cc=xen-devel@lists.xen.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).