[Qemu-devel] [PATCH] Add a memory barrier to guest memory access functions

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

The emulated devices can run simultaneously with the guest, so
we need to be careful with ordering of load and stores done by
them to the guest system memory, which need to be observed in
the right order by the guest operating system.

This adds barriers to some standard guest memory access functions
along with a comment explaining the semantics to exec.c

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

So after the discussion with Paolo, I removed the specific accessors,
just used a normal smp_mb() in only two places, cpu_physical_memory_rw
and cpu_physical_memory_map.

I don't see an obvious need to provide a "relaxed" variant of the
later at this stage, a quick grep doesn't seem to show that most cases
where it's used are either not performance sensitive or the barrier
makes sense, but feel free to prove me wrong :-)

If we really want that, my suggestion would be to change the "is_write"
flag into a proper bitmask of direction and relaxed attribute (which we
can use for more attributes in the future if needed). 

Also, we probably want an smp_mb() when shooting MSIs (not LSIs, those
are not ordered, that's why the guest driver needs to do an MMIO read
after an LSI, but MSIs are). I haven't looked at that yet, we can do
it from a separate patch if needed.

 exec.c |   37 +++++++++++++++++++++++++++++++++++++
 1 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/exec.c b/exec.c
index 363ec98..997dbb0 100644
--- a/exec.c
+++ b/exec.c
@@ -16,6 +16,34 @@
  * You should have received a copy of the GNU Lesser General Public
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
+
+/*
+ * Note on memory barriers usage:
+ *
+ * In order for emulated devices "DMA" operations to appear
+ * with a consistent ordering to the guest, we provide some
+ * amount of ordering guarantees:
+ *
+ * cpy_physical_memory_rw() (and all functions using it) is
+ * ordered vs. all previous accesses (it begins with a full
+ * memory barrier)
+ *
+ * This include all the new dma_* accessors.
+ *
+ * The old variants of ld* and st* that have not been convered
+ * to dma_ are not ordered. Users are reponsible for using their
+ * own ordering.
+ *
+ * cpu_physical_memory_map() provides a memory barrier vs. all
+ * previous accesses. There is no implicit barrier on unmap.
+ * If ordering is required between accessed within the map/unmmap
+ * sequence, then it needs to be done explicitely.
+ *
+ * This means that a typical block driver using map/unmap accross
+ * the transfer of a block followed by dma_ writes to signal
+ * completion or interrupt shouldn't require the addition of
+ * explicit barriers.
+ */ 
 #include "config.h"
 #ifdef _WIN32
 #include <windows.h>
@@ -25,6 +53,7 @@
 #endif
 
 #include "qemu-common.h"
+#include "qemu-barrier.h"
 #include "cpu.h"
 #include "tcg.h"
 #include "hw/hw.h"
@@ -3516,6 +3545,10 @@ void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
     target_phys_addr_t page;
     MemoryRegionSection *section;
 
+    /* Provides ordering vs. previous accesses, see comments
+     * at the top of this file */
+    smp_mb();
+
     while (len > 0) {
         page = addr & TARGET_PAGE_MASK;
         l = (page + TARGET_PAGE_SIZE) - addr;
@@ -3713,6 +3746,10 @@ void *cpu_physical_memory_map(target_phys_addr_t addr,
     ram_addr_t rlen;
     void *ret;
 
+    /* Provides ordering vs. previous accesses, see comments
+     * at the top of this file */
+    smp_mb();
+
     while (len > 0) {
         page = addr & TARGET_PAGE_MASK;
         l = (page + TARGET_PAGE_SIZE) - addr;