Re: [Qemu-devel] [RFC/PATCH] Add a memory barrier to guest memory access functions

[Anthony Liguori <anthony@codemonkey.ws>]

On 05/16/2012 07:52 PM, Benjamin Herrenschmidt wrote:
> 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.
>
> In order to avoid unnecessary overhead on i386, we define a new
> barrier dma_mb() which is a full barrier on powerpc and a nop
> on i386 and x86_64 (see the comment I added in the code).
>
> This barrier is then added to qemu_get_ram_ptr() which is easier
> than sprinkling into all the functions that provide guest
> access and are all more or less open coded.
>
> Signed-off-by: Benjamin Herrenschmidt<benh@kernel.crashing.org>
> ---
>
> Discussion: So the other option is to do it only in
> cpu_physical_memory_rw() and leave the responsibility to use explicit
> barriers to the callers of ld*/st* accessors. For example virtio already
> does it in a few places explicitly.
>
>   exec.c         |   11 +++++++++++
>   qemu-barrier.h |   29 ++++++++++++++++++++++++++---
>   2 files changed, 37 insertions(+), 3 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 40cf52d..fc857b6 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -25,6 +25,7 @@
>   #endif
>
>   #include "qemu-common.h"
> +#include "qemu-barrier.h"
>   #include "cpu.h"
>   #include "tcg.h"
>   #include "hw/hw.h"
> @@ -2794,6 +2795,9 @@ void *qemu_get_ram_ptr(ram_addr_t addr)
>   {
>       RAMBlock *block;
>
> +    /* We ensure ordering for all DMA transactions */
> +    dma_mb();
> +

I get being conservative, but I don't think this makes a lot of sense.  There 
are cases where the return of this function is cached (like the VGA ram area). 
I think it would make more sense if you explicitly put a barrier after write 
operations.

Regards,

Anthony Liguori

>       QLIST_FOREACH(block,&ram_list.blocks, next) {
>           if (addr - block->offset<  block->length) {
>               /* Move this entry to to start of the list.  */
> @@ -2830,6 +2834,9 @@ void *qemu_safe_ram_ptr(ram_addr_t addr)
>   {
>       RAMBlock *block;
>
> +    /* We ensure ordering for all DMA transactions */
> +    dma_mb();
> +
>       QLIST_FOREACH(block,&ram_list.blocks, next) {
>           if (addr - block->offset<  block->length) {
>               if (xen_enabled()) {
> @@ -2861,6 +2868,10 @@ void *qemu_ram_ptr_length(ram_addr_t addr, ram_addr_t *size)
>       if (*size == 0) {
>           return NULL;
>       }
> +
> +    /* We ensure ordering for all DMA transactions */
> +    dma_mb();
> +
>       if (xen_enabled()) {
>           return xen_map_cache(addr, *size, 1);
>       } else {
> diff --git a/qemu-barrier.h b/qemu-barrier.h
> index 7e11197..8c62683 100644
> --- a/qemu-barrier.h
> +++ b/qemu-barrier.h
> @@ -23,7 +23,21 @@
>   #define smp_mb() __sync_synchronize()
>   #else
>   #define smp_mb() asm volatile("lock; addl $0,0(%%esp) " ::: "memory")
> -#endif
> + #endif
> +
> +/*
> + * DMA barrier is used to order accesses from qemu devices to
> + * guest memory, in order to make them appear to the guest in
> + * program order.
> + *
> + * We assume that we never uses non-temporal accesses for such
> + * DMA and so don't need anything other than a compiler barrier
> + *
> + * If some devices use weakly ordered SSE load/store instructions
> + * then those devices will be responsible for using the appropriate
> + * barriers as well.
> + */
> +#define dma_mb()    barrier()
>
>   #elif defined(__x86_64__)
>
> @@ -31,6 +45,9 @@
>   #define smp_rmb()   barrier()
>   #define smp_mb() asm volatile("mfence" ::: "memory")
>
> +/* Same comment as i386 */
> +#define dma_mb()    barrier()
> +
>   #elif defined(_ARCH_PPC)
>
>   /*
> @@ -46,8 +63,13 @@
>   #define smp_rmb()   asm volatile("sync" ::: "memory")
>   #endif
>
> -#define smp_mb()   asm volatile("sync" ::: "memory")
> +#define smp_mb()    asm volatile("sync" ::: "memory")
>
> +/*
> + * We use a full barrier for DMA which encompass the full
> + * requirements of the PCI ordering model.
> + */
> +#define dma_mb()    smp_mb()
>   #else
>
>   /*
> @@ -57,8 +79,9 @@
>    * be overkill for wmb() and rmb().
>    */
>   #define smp_wmb()   __sync_synchronize()
> -#define smp_mb()   __sync_synchronize()
> +#define smp_mb()    __sync_synchronize()
>   #define smp_rmb()   __sync_synchronize()
> +#define dma_rmb()   __sync_synchronize()
>
>   #endif
>
>
>