[Qemu-devel] [RFH PATCH 0/2] xen-mapcache: fixes for unlocked memory access

[Qemu-devel] [RFH PATCH 0/2] xen-mapcache: fixes for unlocked memory access

[Paolo Bonzini]

These are small preparations to let Xen support memory access outside
the BQL.  Warning: they are not tested beyond compilation.

Paolo Bonzini (2):
  xen: do not use __-named variables in mapcache
  xen: add a lock for the mapcache

 xen-mapcache.c | 94 +++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 63 insertions(+), 31 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [RFH PATCH 1/2] xen: do not use __-named variables in mapcache

[Paolo Bonzini]

Keep the namespace clean.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 xen-mapcache.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/xen-mapcache.c b/xen-mapcache.c
index 66da1a6..458069b 100644
--- a/xen-mapcache.c
+++ b/xen-mapcache.c
@@ -199,8 +199,8 @@ uint8_t *xen_map_cache(hwaddr phys_addr, hwaddr size,
     MapCacheEntry *entry, *pentry = NULL;
     hwaddr address_index;
     hwaddr address_offset;
-    hwaddr __size = size;
-    hwaddr __test_bit_size = size;
+    hwaddr cache_size = size;
+    hwaddr test_bit_size;
     bool translated = false;
 
 tryagain:
@@ -209,22 +209,22 @@ tryagain:
 
     trace_xen_map_cache(phys_addr);
 
-    /* __test_bit_size is always a multiple of XC_PAGE_SIZE */
+    /* test_bit_size is always a multiple of XC_PAGE_SIZE */
     if (size) {
-        __test_bit_size = size + (phys_addr & (XC_PAGE_SIZE - 1));
+        test_bit_size = size + (phys_addr & (XC_PAGE_SIZE - 1));
 
-        if (__test_bit_size % XC_PAGE_SIZE) {
-            __test_bit_size += XC_PAGE_SIZE - (__test_bit_size % XC_PAGE_SIZE);
+        if (test_bit_size % XC_PAGE_SIZE) {
+            test_bit_size += XC_PAGE_SIZE - (test_bit_size % XC_PAGE_SIZE);
         }
     } else {
-        __test_bit_size = XC_PAGE_SIZE;
+        test_bit_size = XC_PAGE_SIZE;
     }
 
     if (mapcache->last_entry != NULL &&
         mapcache->last_entry->paddr_index == address_index &&
-        !lock && !__size &&
+        !lock && !size &&
         test_bits(address_offset >> XC_PAGE_SHIFT,
-                  __test_bit_size >> XC_PAGE_SHIFT,
+                  test_bit_size >> XC_PAGE_SHIFT,
                   mapcache->last_entry->valid_mapping)) {
         trace_xen_map_cache_return(mapcache->last_entry->vaddr_base + address_offset);
         return mapcache->last_entry->vaddr_base + address_offset;
@@ -232,20 +232,20 @@ tryagain:
 
     /* size is always a multiple of MCACHE_BUCKET_SIZE */
     if (size) {
-        __size = size + address_offset;
-        if (__size % MCACHE_BUCKET_SIZE) {
-            __size += MCACHE_BUCKET_SIZE - (__size % MCACHE_BUCKET_SIZE);
+        cache_size = size + address_offset;
+        if (cache_size % MCACHE_BUCKET_SIZE) {
+            cache_size += MCACHE_BUCKET_SIZE - (cache_size % MCACHE_BUCKET_SIZE);
         }
     } else {
-        __size = MCACHE_BUCKET_SIZE;
+        cache_size = MCACHE_BUCKET_SIZE;
     }
 
     entry = &mapcache->entry[address_index % mapcache->nr_buckets];
 
     while (entry && entry->lock && entry->vaddr_base &&
-            (entry->paddr_index != address_index || entry->size != __size ||
+            (entry->paddr_index != address_index || entry->size != cache_size ||
              !test_bits(address_offset >> XC_PAGE_SHIFT,
-                 __test_bit_size >> XC_PAGE_SHIFT,
+                 test_bit_size >> XC_PAGE_SHIFT,
                  entry->valid_mapping))) {
         pentry = entry;
         entry = entry->next;
@@ -253,19 +253,19 @@ tryagain:
     if (!entry) {
         entry = g_malloc0(sizeof (MapCacheEntry));
         pentry->next = entry;
-        xen_remap_bucket(entry, __size, address_index);
+        xen_remap_bucket(entry, cache_size, address_index);
     } else if (!entry->lock) {
         if (!entry->vaddr_base || entry->paddr_index != address_index ||
-                entry->size != __size ||
+                entry->size != cache_size ||
                 !test_bits(address_offset >> XC_PAGE_SHIFT,
-                    __test_bit_size >> XC_PAGE_SHIFT,
+                    test_bit_size >> XC_PAGE_SHIFT,
                     entry->valid_mapping)) {
-            xen_remap_bucket(entry, __size, address_index);
+            xen_remap_bucket(entry, cache_size, address_index);
         }
     }
 
     if(!test_bits(address_offset >> XC_PAGE_SHIFT,
-                __test_bit_size >> XC_PAGE_SHIFT,
+                test_bit_size >> XC_PAGE_SHIFT,
                 entry->valid_mapping)) {
         mapcache->last_entry = NULL;
         if (!translated && mapcache->phys_offset_to_gaddr) {
-- 
1.8.3.1

[Qemu-devel] [RFH PATCH 2/2] xen: add a lock for the mapcache

[Paolo Bonzini]

Extend the existing dummy mapcache_lock/unlock macros to cover all of
xen-mapcache.c.  This prepares for unlocked memory access, when parts
of exec.c will not be protected by the BQL.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 xen-mapcache.c | 54 +++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 43 insertions(+), 11 deletions(-)

diff --git a/xen-mapcache.c b/xen-mapcache.c
index 458069b..8cefd0c 100644
--- a/xen-mapcache.c
+++ b/xen-mapcache.c
@@ -49,9 +49,6 @@
  */
 #define NON_MCACHE_MEMORY_SIZE (80 * 1024 * 1024)
 
-#define mapcache_lock()   ((void)0)
-#define mapcache_unlock() ((void)0)
-
 typedef struct MapCacheEntry {
     hwaddr paddr_index;
     uint8_t *vaddr_base;
@@ -79,11 +76,22 @@ typedef struct MapCache {
     unsigned int mcache_bucket_shift;
 
     phys_offset_to_gaddr_t phys_offset_to_gaddr;
+    QemuMutex lock;
     void *opaque;
 } MapCache;
 
 static MapCache *mapcache;
 
+static inline void mapcache_lock(void)
+{
+    qemu_mutex_lock(&mapcache->lock);
+}
+
+static inline void mapcache_unlock(void)
+{
+    qemu_mutex_unlock(&mapcache->lock);
+}
+
 static inline int test_bits(int nr, int size, const unsigned long *addr)
 {
     unsigned long res = find_next_zero_bit(addr, size + nr, nr);
@@ -102,6 +110,7 @@ void xen_map_cache_init(phys_offset_to_gaddr_t f, void *opaque)
 
     mapcache->phys_offset_to_gaddr = f;
     mapcache->opaque = opaque;
+    qemu_mutex_init(&mapcache->lock);
 
     QTAILQ_INIT(&mapcache->locked_entries);
 
@@ -193,8 +202,8 @@ static void xen_remap_bucket(MapCacheEntry *entry,
     g_free(err);
 }
 
-uint8_t *xen_map_cache(hwaddr phys_addr, hwaddr size,
-                       uint8_t lock)
+static uint8_t *xen_map_cache_unlocked(hwaddr phys_addr, hwaddr size,
+                                       uint8_t lock)
 {
     MapCacheEntry *entry, *pentry = NULL;
     hwaddr address_index;
@@ -291,14 +300,27 @@ tryagain:
     return mapcache->last_entry->vaddr_base + address_offset;
 }
 
+uint8_t *xen_map_cache(hwaddr phys_addr, hwaddr size,
+                       uint8_t lock)
+{
+    uint8_t *p;
+
+    mapcache_lock();
+    p = xen_map_cache_unlocked(phys_addr, size, lock);
+    mapcache_unlock();
+    return p;
+}
+
 ram_addr_t xen_ram_addr_from_mapcache(void *ptr)
 {
     MapCacheEntry *entry = NULL;
     MapCacheRev *reventry;
     hwaddr paddr_index;
     hwaddr size;
+    ram_addr_t raddr;
     int found = 0;
 
+    mapcache_lock();
     QTAILQ_FOREACH(reventry, &mapcache->locked_entries, next) {
         if (reventry->vaddr_req == ptr) {
             paddr_index = reventry->paddr_index;
@@ -323,13 +345,16 @@ ram_addr_t xen_ram_addr_from_mapcache(void *ptr)
     }
     if (!entry) {
         DPRINTF("Trying to find address %p that is not in the mapcache!\n", ptr);
-        return 0;
+        raddr = 0;
+    } else {
+        raddr = (reventry->paddr_index << MCACHE_BUCKET_SHIFT) +
+             ((unsigned long) ptr - (unsigned long) entry->vaddr_base);
     }
-    return (reventry->paddr_index << MCACHE_BUCKET_SHIFT) +
-        ((unsigned long) ptr - (unsigned long) entry->vaddr_base);
+    mapcache_unlock();
+    return raddr;
 }
 
-void xen_invalidate_map_cache_entry(uint8_t *buffer)
+static void xen_invalidate_map_cache_entry_unlocked(uint8_t *buffer)
 {
     MapCacheEntry *entry = NULL, *pentry = NULL;
     MapCacheRev *reventry;
@@ -383,6 +408,13 @@ void xen_invalidate_map_cache_entry(uint8_t *buffer)
     g_free(entry);
 }
 
+void xen_invalidate_map_cache_entry(uint8_t *buffer)
+{
+    mapcache_lock();
+    xen_invalidate_map_cache_entry_unlocked(buffer);
+    mapcache_unlock();
+}
+
 void xen_invalidate_map_cache(void)
 {
     unsigned long i;
@@ -391,14 +423,14 @@ void xen_invalidate_map_cache(void)
     /* Flush pending AIO before destroying the mapcache */
     bdrv_drain_all();
 
+    mapcache_lock();
+
     QTAILQ_FOREACH(reventry, &mapcache->locked_entries, next) {
         DPRINTF("There should be no locked mappings at this time, "
                 "but "TARGET_FMT_plx" -> %p is present\n",
                 reventry->paddr_index, reventry->vaddr_req);
     }
 
-    mapcache_lock();
-
     for (i = 0; i < mapcache->nr_buckets; i++) {
         MapCacheEntry *entry = &mapcache->entry[i];
 
-- 
1.8.3.1

Re: [Qemu-devel] [RFH PATCH 1/2] xen: do not use __-named variables in mapcache

[Stefano Stabellini]

On Wed, 14 Jan 2015, Paolo Bonzini wrote:
> Keep the namespace clean.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  xen-mapcache.c | 40 ++++++++++++++++++++--------------------
>  1 file changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/xen-mapcache.c b/xen-mapcache.c
> index 66da1a6..458069b 100644
> --- a/xen-mapcache.c
> +++ b/xen-mapcache.c
> @@ -199,8 +199,8 @@ uint8_t *xen_map_cache(hwaddr phys_addr, hwaddr size,
>      MapCacheEntry *entry, *pentry = NULL;
>      hwaddr address_index;
>      hwaddr address_offset;
> -    hwaddr __size = size;
> -    hwaddr __test_bit_size = size;
> +    hwaddr cache_size = size;
> +    hwaddr test_bit_size;
>      bool translated = false;
>  
>  tryagain:
> @@ -209,22 +209,22 @@ tryagain:
>  
>      trace_xen_map_cache(phys_addr);
>  
> -    /* __test_bit_size is always a multiple of XC_PAGE_SIZE */
> +    /* test_bit_size is always a multiple of XC_PAGE_SIZE */
>      if (size) {
> -        __test_bit_size = size + (phys_addr & (XC_PAGE_SIZE - 1));
> +        test_bit_size = size + (phys_addr & (XC_PAGE_SIZE - 1));
>  
> -        if (__test_bit_size % XC_PAGE_SIZE) {
> -            __test_bit_size += XC_PAGE_SIZE - (__test_bit_size % XC_PAGE_SIZE);
> +        if (test_bit_size % XC_PAGE_SIZE) {
> +            test_bit_size += XC_PAGE_SIZE - (test_bit_size % XC_PAGE_SIZE);
>          }
>      } else {
> -        __test_bit_size = XC_PAGE_SIZE;
> +        test_bit_size = XC_PAGE_SIZE;
>      }
>  
>      if (mapcache->last_entry != NULL &&
>          mapcache->last_entry->paddr_index == address_index &&
> -        !lock && !__size &&
> +        !lock && !size &&
>          test_bits(address_offset >> XC_PAGE_SHIFT,
> -                  __test_bit_size >> XC_PAGE_SHIFT,
> +                  test_bit_size >> XC_PAGE_SHIFT,
>                    mapcache->last_entry->valid_mapping)) {
>          trace_xen_map_cache_return(mapcache->last_entry->vaddr_base + address_offset);
>          return mapcache->last_entry->vaddr_base + address_offset;
> @@ -232,20 +232,20 @@ tryagain:
>  
>      /* size is always a multiple of MCACHE_BUCKET_SIZE */
>      if (size) {
> -        __size = size + address_offset;
> -        if (__size % MCACHE_BUCKET_SIZE) {
> -            __size += MCACHE_BUCKET_SIZE - (__size % MCACHE_BUCKET_SIZE);
> +        cache_size = size + address_offset;
> +        if (cache_size % MCACHE_BUCKET_SIZE) {
> +            cache_size += MCACHE_BUCKET_SIZE - (cache_size % MCACHE_BUCKET_SIZE);
>          }
>      } else {
> -        __size = MCACHE_BUCKET_SIZE;
> +        cache_size = MCACHE_BUCKET_SIZE;
>      }
>  
>      entry = &mapcache->entry[address_index % mapcache->nr_buckets];
>  
>      while (entry && entry->lock && entry->vaddr_base &&
> -            (entry->paddr_index != address_index || entry->size != __size ||
> +            (entry->paddr_index != address_index || entry->size != cache_size ||
>               !test_bits(address_offset >> XC_PAGE_SHIFT,
> -                 __test_bit_size >> XC_PAGE_SHIFT,
> +                 test_bit_size >> XC_PAGE_SHIFT,
>                   entry->valid_mapping))) {
>          pentry = entry;
>          entry = entry->next;
> @@ -253,19 +253,19 @@ tryagain:
>      if (!entry) {
>          entry = g_malloc0(sizeof (MapCacheEntry));
>          pentry->next = entry;
> -        xen_remap_bucket(entry, __size, address_index);
> +        xen_remap_bucket(entry, cache_size, address_index);
>      } else if (!entry->lock) {
>          if (!entry->vaddr_base || entry->paddr_index != address_index ||
> -                entry->size != __size ||
> +                entry->size != cache_size ||
>                  !test_bits(address_offset >> XC_PAGE_SHIFT,
> -                    __test_bit_size >> XC_PAGE_SHIFT,
> +                    test_bit_size >> XC_PAGE_SHIFT,
>                      entry->valid_mapping)) {
> -            xen_remap_bucket(entry, __size, address_index);
> +            xen_remap_bucket(entry, cache_size, address_index);
>          }
>      }
>  
>      if(!test_bits(address_offset >> XC_PAGE_SHIFT,
> -                __test_bit_size >> XC_PAGE_SHIFT,
> +                test_bit_size >> XC_PAGE_SHIFT,
>                  entry->valid_mapping)) {
>          mapcache->last_entry = NULL;
>          if (!translated && mapcache->phys_offset_to_gaddr) {
> -- 
> 1.8.3.1
> 
> 

Re: [Qemu-devel] [RFH PATCH 2/2] xen: add a lock for the mapcache

[Stefano Stabellini]

On Wed, 14 Jan 2015, Paolo Bonzini wrote:
> Extend the existing dummy mapcache_lock/unlock macros to cover all of
> xen-mapcache.c.  This prepares for unlocked memory access, when parts
> of exec.c will not be protected by the BQL.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  xen-mapcache.c | 54 +++++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 43 insertions(+), 11 deletions(-)
> 
> diff --git a/xen-mapcache.c b/xen-mapcache.c
> index 458069b..8cefd0c 100644
> --- a/xen-mapcache.c
> +++ b/xen-mapcache.c
> @@ -49,9 +49,6 @@
>   */
>  #define NON_MCACHE_MEMORY_SIZE (80 * 1024 * 1024)
>  
> -#define mapcache_lock()   ((void)0)
> -#define mapcache_unlock() ((void)0)
> -
>  typedef struct MapCacheEntry {
>      hwaddr paddr_index;
>      uint8_t *vaddr_base;
> @@ -79,11 +76,22 @@ typedef struct MapCache {
>      unsigned int mcache_bucket_shift;
>  
>      phys_offset_to_gaddr_t phys_offset_to_gaddr;
> +    QemuMutex lock;
>      void *opaque;
>  } MapCache;
>  
>  static MapCache *mapcache;
>  
> +static inline void mapcache_lock(void)
> +{
> +    qemu_mutex_lock(&mapcache->lock);
> +}
> +
> +static inline void mapcache_unlock(void)
> +{
> +    qemu_mutex_unlock(&mapcache->lock);
> +}
> +
>  static inline int test_bits(int nr, int size, const unsigned long *addr)
>  {
>      unsigned long res = find_next_zero_bit(addr, size + nr, nr);
> @@ -102,6 +110,7 @@ void xen_map_cache_init(phys_offset_to_gaddr_t f, void *opaque)
>  
>      mapcache->phys_offset_to_gaddr = f;
>      mapcache->opaque = opaque;
> +    qemu_mutex_init(&mapcache->lock);
>  
>      QTAILQ_INIT(&mapcache->locked_entries);
>  
> @@ -193,8 +202,8 @@ static void xen_remap_bucket(MapCacheEntry *entry,
>      g_free(err);
>  }
>  
> -uint8_t *xen_map_cache(hwaddr phys_addr, hwaddr size,
> -                       uint8_t lock)
> +static uint8_t *xen_map_cache_unlocked(hwaddr phys_addr, hwaddr size,
> +                                       uint8_t lock)
>  {
>      MapCacheEntry *entry, *pentry = NULL;
>      hwaddr address_index;
> @@ -291,14 +300,27 @@ tryagain:
>      return mapcache->last_entry->vaddr_base + address_offset;
>  }
>  
> +uint8_t *xen_map_cache(hwaddr phys_addr, hwaddr size,
> +                       uint8_t lock)
> +{
> +    uint8_t *p;
> +
> +    mapcache_lock();
> +    p = xen_map_cache_unlocked(phys_addr, size, lock);
> +    mapcache_unlock();
> +    return p;
> +}
> +
>  ram_addr_t xen_ram_addr_from_mapcache(void *ptr)
>  {
>      MapCacheEntry *entry = NULL;
>      MapCacheRev *reventry;
>      hwaddr paddr_index;
>      hwaddr size;
> +    ram_addr_t raddr;
>      int found = 0;
>  
> +    mapcache_lock();
>      QTAILQ_FOREACH(reventry, &mapcache->locked_entries, next) {
>          if (reventry->vaddr_req == ptr) {
>              paddr_index = reventry->paddr_index;
> @@ -323,13 +345,16 @@ ram_addr_t xen_ram_addr_from_mapcache(void *ptr)
>      }
>      if (!entry) {
>          DPRINTF("Trying to find address %p that is not in the mapcache!\n", ptr);
> -        return 0;
> +        raddr = 0;
> +    } else {
> +        raddr = (reventry->paddr_index << MCACHE_BUCKET_SHIFT) +
> +             ((unsigned long) ptr - (unsigned long) entry->vaddr_base);
>      }
> -    return (reventry->paddr_index << MCACHE_BUCKET_SHIFT) +
> -        ((unsigned long) ptr - (unsigned long) entry->vaddr_base);
> +    mapcache_unlock();
> +    return raddr;
>  }
>  
> -void xen_invalidate_map_cache_entry(uint8_t *buffer)
> +static void xen_invalidate_map_cache_entry_unlocked(uint8_t *buffer)
>  {
>      MapCacheEntry *entry = NULL, *pentry = NULL;
>      MapCacheRev *reventry;
> @@ -383,6 +408,13 @@ void xen_invalidate_map_cache_entry(uint8_t *buffer)
>      g_free(entry);
>  }
>  
> +void xen_invalidate_map_cache_entry(uint8_t *buffer)
> +{
> +    mapcache_lock();
> +    xen_invalidate_map_cache_entry_unlocked(buffer);
> +    mapcache_unlock();
> +}
> +
>  void xen_invalidate_map_cache(void)
>  {
>      unsigned long i;
> @@ -391,14 +423,14 @@ void xen_invalidate_map_cache(void)
>      /* Flush pending AIO before destroying the mapcache */
>      bdrv_drain_all();
>  
> +    mapcache_lock();
> +
>      QTAILQ_FOREACH(reventry, &mapcache->locked_entries, next) {
>          DPRINTF("There should be no locked mappings at this time, "
>                  "but "TARGET_FMT_plx" -> %p is present\n",
>                  reventry->paddr_index, reventry->vaddr_req);
>      }
>  
> -    mapcache_lock();
> -
>      for (i = 0; i < mapcache->nr_buckets; i++) {
>          MapCacheEntry *entry = &mapcache->entry[i];
>  
> -- 
> 1.8.3.1
>