From 74e1fab6015b584312eb850b7d55b1378f19c6ce Mon Sep 17 00:00:00 2001 From: "kfraser@localhost.localdomain" Date: Fri, 10 Nov 2006 10:30:38 +0000 Subject: [PATCH] [HVM] Clarify ioreq interactions between Xen and qemu-dm. Mainly this involves placing appropriate barriers before updating the shared state variable, and after reading from it. The model is that it is state switches that drive the transfer of data to and fro in the shared ioreq structure. So writers should wmb() before updating the variable; readers should rmb() after seeing a state change. These barriers aren't actually required given the current code structure since the communication flow is really driven by event-channel notifications, which happen to provide a sufficient barrier. However, relying on this for all time and on all architectures seems foolish and the barriers have negligible cost compared with the totoal ioreq round-trip latency. Also the model of communications being driven by the shared-memory state variable more closely matches other inter-domain protocols (where there is usually a shared-memory producer index), and is hence a model we are familiar with and likely to implement correctly. Signed-off-by: Keir Fraser --- tools/ioemu/target-i386-dm/helper2.c | 38 +++++++++++++++++----------- xen/arch/x86/hvm/io.c | 2 ++ xen/arch/x86/hvm/platform.c | 30 ++++++++++++++-------- 3 files changed, 45 insertions(+), 25 deletions(-) diff --git a/tools/ioemu/target-i386-dm/helper2.c b/tools/ioemu/target-i386-dm/helper2.c index 787d6631ac..aa7b042561 100644 --- a/tools/ioemu/target-i386-dm/helper2.c +++ b/tools/ioemu/target-i386-dm/helper2.c @@ -209,18 +209,19 @@ static ioreq_t *__cpu_get_ioreq(int vcpu) req = &(shared_page->vcpu_iodata[vcpu].vp_ioreq); - if (req->state == STATE_IOREQ_READY) { - req->state = STATE_IOREQ_INPROCESS; - rmb(); - return req; + if (req->state != STATE_IOREQ_READY) { + fprintf(logfile, "I/O request not ready: " + "%x, ptr: %x, port: %"PRIx64", " + "data: %"PRIx64", count: %"PRIx64", size: %"PRIx64"\n", + req->state, req->data_is_ptr, req->addr, + req->data, req->count, req->size); + return NULL; } - fprintf(logfile, "False I/O request ... in-service already: " - "%x, ptr: %x, port: %"PRIx64", " - "data: %"PRIx64", count: %"PRIx64", size: %"PRIx64"\n", - req->state, req->data_is_ptr, req->addr, - req->data, req->count, req->size); - return NULL; + rmb(); /* see IOREQ_READY /then/ read contents of ioreq */ + + req->state = STATE_IOREQ_INPROCESS; + return req; } //use poll to get the port notification @@ -504,12 +505,19 @@ void cpu_handle_ioreq(void *opaque) if (req) { __handle_ioreq(env, req); - /* No state change if state = STATE_IORESP_HOOK */ - if (req->state == STATE_IOREQ_INPROCESS) { - req->state = STATE_IORESP_READY; - xc_evtchn_notify(xce_handle, ioreq_local_port[send_vcpu]); - } else + if (req->state != STATE_IOREQ_INPROCESS) { + fprintf(logfile, "Badness in I/O request ... not in service?!: " + "%x, ptr: %x, port: %"PRIx64", " + "data: %"PRIx64", count: %"PRIx64", size: %"PRIx64"\n", + req->state, req->data_is_ptr, req->addr, + req->data, req->count, req->size); destroy_hvm_domain(); + return; + } + + wmb(); /* Update ioreq contents /then/ update state. */ + req->state = STATE_IORESP_READY; + xc_evtchn_notify(xce_handle, ioreq_local_port[send_vcpu]); } } diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c index 0e34be4ebb..0976183bb7 100644 --- a/xen/arch/x86/hvm/io.c +++ b/xen/arch/x86/hvm/io.c @@ -745,6 +745,8 @@ void hvm_io_assist(struct vcpu *v) domain_crash_synchronous(); } + rmb(); /* see IORESP_READY /then/ read contents of ioreq */ + p->state = STATE_IOREQ_NONE; if ( p->type == IOREQ_TYPE_PIO ) diff --git a/xen/arch/x86/hvm/platform.c b/xen/arch/x86/hvm/platform.c index 8246bc706a..0afae0e4d6 100644 --- a/xen/arch/x86/hvm/platform.c +++ b/xen/arch/x86/hvm/platform.c @@ -727,15 +727,20 @@ static void hvm_send_assist_req(struct vcpu *v) ioreq_t *p; p = &get_vio(v->domain, v->vcpu_id)->vp_ioreq; - if ( unlikely(p->state != STATE_IOREQ_NONE) ) { - /* This indicates a bug in the device model. Crash the - domain. */ - printk("Device model set bad IO state %d.\n", p->state); + if ( unlikely(p->state != STATE_IOREQ_NONE) ) + { + /* This indicates a bug in the device model. Crash the domain. */ + gdprintk(XENLOG_ERR, "Device model set bad IO state %d.\n", p->state); domain_crash(v->domain); return; } prepare_wait_on_xen_event_channel(v->arch.hvm_vcpu.xen_port); + + /* + * Following happens /after/ blocking and setting up ioreq contents. + * prepare_wait_on_xen_event_channel() is an implicit barrier. + */ p->state = STATE_IOREQ_READY; notify_via_xen_event_channel(v->arch.hvm_vcpu.xen_port); } @@ -781,10 +786,12 @@ void send_pio_req(unsigned long port, unsigned long count, int size, p->data = shadow_gva_to_gpa(current, value); else p->data = value; /* guest VA == guest PA */ - } else if ( dir == IOREQ_WRITE ) + } + else if ( dir == IOREQ_WRITE ) p->data = value; - if ( hvm_portio_intercept(p) ) { + if ( hvm_portio_intercept(p) ) + { p->state = STATE_IORESP_READY; hvm_io_assist(v); return; @@ -829,15 +836,18 @@ static void send_mmio_req(unsigned char type, unsigned long gpa, p->io_count++; - if (value_is_ptr) { - if (hvm_paging_enabled(v)) + if ( value_is_ptr ) + { + if ( hvm_paging_enabled(v) ) p->data = shadow_gva_to_gpa(v, value); else p->data = value; /* guest VA == guest PA */ - } else + } + else p->data = value; - if ( hvm_mmio_intercept(p) || hvm_buffered_io_intercept(p) ) { + if ( hvm_mmio_intercept(p) || hvm_buffered_io_intercept(p) ) + { p->state = STATE_IORESP_READY; hvm_io_assist(v); return; -- 2.30.2