[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 <keir@xensource.com>

diff --git a/tools/ioemu/target-i386-dm/helper2.c b/tools/ioemu/target-i386-dm/helper2.c
index 787d6631ac29e2009a1245096463e70438c37c13..aa7b042561234c77aecf4450c7e6c454010f879b 100644 (file)
--- 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 0e34be4ebb667d3448ab3720fce046c79f34904f..0976183bb792e958eed5fc92f115153b4ca7e65f 100644 (file)
--- 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 8246bc706a66a40f00006a16afae5dc52a05794d..0afae0e4d6e5b1442a1ad95ae89efbc86e42e86d 100644 (file)
--- 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;