[Qemu-devel] [PATCH 0/4] xen: don't save/restore the physmap on VM save/restore

[Qemu-devel] [PATCH 0/4] xen: don't save/restore the physmap on VM save/restore

[Igor Druzhinin]

Saving/restoring the physmap to/from xenstore was introduced to
QEMU majorly in order to cover up the VRAM region restore issue.
The sequence of restore operations implies that we should know
the effective guest VRAM address *before* we have the VRAM region
restored (which happens later). Unfortunately, in Xen environment
VRAM memory does actually belong to a guest - not QEMU itself -
which means the position of this region is unknown beforehand and
can't be mapped into QEMU address space immediately.

Previously, recreating xenstore keys, holding the physmap, by the
toolstack helped to get this information in place at the right
moment ready to be consumed by QEMU to map the region properly.
But using xenstore for it has certain disadvantages: toolstack
needs to be aware of these keys and save/restore them accordingly;
accessing xenstore requires extra privileges which hinders QEMU
sandboxing.

The previous attempt to get rid of that was to remember all the
VRAM pointers during QEMU initialization phase and then update
them all at once when an actual foreign mapping is established.
Unfortunately, this approach worked only for VRAM and only for
a predefined set of devices - stdvga and cirrus. QXL and other
possible future devices using a moving emulated MMIO region
would be equally broken.

The new approach leverages xenforeignmemory_map2() call recently
introduced in libxenforeignmemory. It allows to create a dummy
anonymous mapping for QEMU during its initialization and change
it to a real one later during machine state restore.

Igor Druzhinin (4):
  xen: move physmap saving into a separate function
  xen/mapcache: add an ability to create dummy mappings
  xen/mapcache: introduce xen_remap_cache_entry()
  xen: don't use xenstore to save/restore physmap anymore

 configure                     |  18 ++++++
 hw/i386/xen/xen-hvm.c         | 100 ++++++++++++++++++++------------
 hw/i386/xen/xen-mapcache.c    | 129 +++++++++++++++++++++++++++++++++++++++---
 include/hw/xen/xen_common.h   |   8 +++
 include/sysemu/xen-mapcache.h |   6 ++
 5 files changed, 217 insertions(+), 44 deletions(-)

-- 
2.7.4

[PATCH 0/4] xen: don't save/restore the physmap on VM save/restore

[Igor Druzhinin]

Saving/restoring the physmap to/from xenstore was introduced to
QEMU majorly in order to cover up the VRAM region restore issue.
The sequence of restore operations implies that we should know
the effective guest VRAM address *before* we have the VRAM region
restored (which happens later). Unfortunately, in Xen environment
VRAM memory does actually belong to a guest - not QEMU itself -
which means the position of this region is unknown beforehand and
can't be mapped into QEMU address space immediately.

Previously, recreating xenstore keys, holding the physmap, by the
toolstack helped to get this information in place at the right
moment ready to be consumed by QEMU to map the region properly.
But using xenstore for it has certain disadvantages: toolstack
needs to be aware of these keys and save/restore them accordingly;
accessing xenstore requires extra privileges which hinders QEMU
sandboxing.

The previous attempt to get rid of that was to remember all the
VRAM pointers during QEMU initialization phase and then update
them all at once when an actual foreign mapping is established.
Unfortunately, this approach worked only for VRAM and only for
a predefined set of devices - stdvga and cirrus. QXL and other
possible future devices using a moving emulated MMIO region
would be equally broken.

The new approach leverages xenforeignmemory_map2() call recently
introduced in libxenforeignmemory. It allows to create a dummy
anonymous mapping for QEMU during its initialization and change
it to a real one later during machine state restore.

Igor Druzhinin (4):
  xen: move physmap saving into a separate function
  xen/mapcache: add an ability to create dummy mappings
  xen/mapcache: introduce xen_remap_cache_entry()
  xen: don't use xenstore to save/restore physmap anymore

 configure                     |  18 ++++++
 hw/i386/xen/xen-hvm.c         | 100 ++++++++++++++++++++------------
 hw/i386/xen/xen-mapcache.c    | 129 +++++++++++++++++++++++++++++++++++++++---
 include/hw/xen/xen_common.h   |   8 +++
 include/sysemu/xen-mapcache.h |   6 ++
 5 files changed, 217 insertions(+), 44 deletions(-)

-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Qemu-devel] [PATCH 1/4] xen: move physmap saving into a separate function

[Igor Druzhinin]

Non-functional change.

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
 hw/i386/xen/xen-hvm.c | 57 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 31 insertions(+), 26 deletions(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index cffa7e2..d259cf7 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -305,6 +305,36 @@ static hwaddr xen_phys_offset_to_gaddr(hwaddr start_addr,
     return start_addr;
 }
 
+static int xen_save_physmap(XenIOState *state, XenPhysmap *physmap)
+{
+    char path[80], value[17];
+
+    snprintf(path, sizeof(path),
+            "/local/domain/0/device-model/%d/physmap/%"PRIx64"/start_addr",
+            xen_domid, (uint64_t)physmap->phys_offset);
+    snprintf(value, sizeof(value), "%"PRIx64, (uint64_t)physmap->start_addr);
+    if (!xs_write(state->xenstore, 0, path, value, strlen(value))) {
+        return -1;
+    }
+    snprintf(path, sizeof(path),
+            "/local/domain/0/device-model/%d/physmap/%"PRIx64"/size",
+            xen_domid, (uint64_t)physmap->phys_offset);
+    snprintf(value, sizeof(value), "%"PRIx64, (uint64_t)physmap->size);
+    if (!xs_write(state->xenstore, 0, path, value, strlen(value))) {
+        return -1;
+    }
+    if (physmap->name) {
+        snprintf(path, sizeof(path),
+                "/local/domain/0/device-model/%d/physmap/%"PRIx64"/name",
+                xen_domid, (uint64_t)physmap->phys_offset);
+        if (!xs_write(state->xenstore, 0, path,
+                      physmap->name, strlen(physmap->name))) {
+            return -1;
+        }
+    }
+    return 0;
+}
+
 static int xen_add_to_physmap(XenIOState *state,
                               hwaddr start_addr,
                               ram_addr_t size,
@@ -316,7 +346,6 @@ static int xen_add_to_physmap(XenIOState *state,
     XenPhysmap *physmap = NULL;
     hwaddr pfn, start_gpfn;
     hwaddr phys_offset = memory_region_get_ram_addr(mr);
-    char path[80], value[17];
     const char *mr_name;
 
     if (get_physmapping(state, start_addr, size)) {
@@ -368,31 +397,7 @@ go_physmap:
                                    start_addr >> TARGET_PAGE_BITS,
                                    (start_addr + size - 1) >> TARGET_PAGE_BITS,
                                    XEN_DOMCTL_MEM_CACHEATTR_WB);
-
-    snprintf(path, sizeof(path),
-            "/local/domain/0/device-model/%d/physmap/%"PRIx64"/start_addr",
-            xen_domid, (uint64_t)phys_offset);
-    snprintf(value, sizeof(value), "%"PRIx64, (uint64_t)start_addr);
-    if (!xs_write(state->xenstore, 0, path, value, strlen(value))) {
-        return -1;
-    }
-    snprintf(path, sizeof(path),
-            "/local/domain/0/device-model/%d/physmap/%"PRIx64"/size",
-            xen_domid, (uint64_t)phys_offset);
-    snprintf(value, sizeof(value), "%"PRIx64, (uint64_t)size);
-    if (!xs_write(state->xenstore, 0, path, value, strlen(value))) {
-        return -1;
-    }
-    if (mr_name) {
-        snprintf(path, sizeof(path),
-                "/local/domain/0/device-model/%d/physmap/%"PRIx64"/name",
-                xen_domid, (uint64_t)phys_offset);
-        if (!xs_write(state->xenstore, 0, path, mr_name, strlen(mr_name))) {
-            return -1;
-        }
-    }
-
-    return 0;
+    return xen_save_physmap(state, physmap);
 }
 
 static int xen_remove_from_physmap(XenIOState *state,
-- 
2.7.4

[PATCH 1/4] xen: move physmap saving into a separate function

[Igor Druzhinin]

Non-functional change.

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
 hw/i386/xen/xen-hvm.c | 57 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 31 insertions(+), 26 deletions(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index cffa7e2..d259cf7 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -305,6 +305,36 @@ static hwaddr xen_phys_offset_to_gaddr(hwaddr start_addr,
     return start_addr;
 }
 
+static int xen_save_physmap(XenIOState *state, XenPhysmap *physmap)
+{
+    char path[80], value[17];
+
+    snprintf(path, sizeof(path),
+            "/local/domain/0/device-model/%d/physmap/%"PRIx64"/start_addr",
+            xen_domid, (uint64_t)physmap->phys_offset);
+    snprintf(value, sizeof(value), "%"PRIx64, (uint64_t)physmap->start_addr);
+    if (!xs_write(state->xenstore, 0, path, value, strlen(value))) {
+        return -1;
+    }
+    snprintf(path, sizeof(path),
+            "/local/domain/0/device-model/%d/physmap/%"PRIx64"/size",
+            xen_domid, (uint64_t)physmap->phys_offset);
+    snprintf(value, sizeof(value), "%"PRIx64, (uint64_t)physmap->size);
+    if (!xs_write(state->xenstore, 0, path, value, strlen(value))) {
+        return -1;
+    }
+    if (physmap->name) {
+        snprintf(path, sizeof(path),
+                "/local/domain/0/device-model/%d/physmap/%"PRIx64"/name",
+                xen_domid, (uint64_t)physmap->phys_offset);
+        if (!xs_write(state->xenstore, 0, path,
+                      physmap->name, strlen(physmap->name))) {
+            return -1;
+        }
+    }
+    return 0;
+}
+
 static int xen_add_to_physmap(XenIOState *state,
                               hwaddr start_addr,
                               ram_addr_t size,
@@ -316,7 +346,6 @@ static int xen_add_to_physmap(XenIOState *state,
     XenPhysmap *physmap = NULL;
     hwaddr pfn, start_gpfn;
     hwaddr phys_offset = memory_region_get_ram_addr(mr);
-    char path[80], value[17];
     const char *mr_name;
 
     if (get_physmapping(state, start_addr, size)) {
@@ -368,31 +397,7 @@ go_physmap:
                                    start_addr >> TARGET_PAGE_BITS,
                                    (start_addr + size - 1) >> TARGET_PAGE_BITS,
                                    XEN_DOMCTL_MEM_CACHEATTR_WB);
-
-    snprintf(path, sizeof(path),
-            "/local/domain/0/device-model/%d/physmap/%"PRIx64"/start_addr",
-            xen_domid, (uint64_t)phys_offset);
-    snprintf(value, sizeof(value), "%"PRIx64, (uint64_t)start_addr);
-    if (!xs_write(state->xenstore, 0, path, value, strlen(value))) {
-        return -1;
-    }
-    snprintf(path, sizeof(path),
-            "/local/domain/0/device-model/%d/physmap/%"PRIx64"/size",
-            xen_domid, (uint64_t)phys_offset);
-    snprintf(value, sizeof(value), "%"PRIx64, (uint64_t)size);
-    if (!xs_write(state->xenstore, 0, path, value, strlen(value))) {
-        return -1;
-    }
-    if (mr_name) {
-        snprintf(path, sizeof(path),
-                "/local/domain/0/device-model/%d/physmap/%"PRIx64"/name",
-                xen_domid, (uint64_t)phys_offset);
-        if (!xs_write(state->xenstore, 0, path, mr_name, strlen(mr_name))) {
-            return -1;
-        }
-    }
-
-    return 0;
+    return xen_save_physmap(state, physmap);
 }
 
 static int xen_remove_from_physmap(XenIOState *state,
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Qemu-devel] [PATCH 1/4] xen: move physmap saving into a separate function

[Stefano Stabellini]

On Fri, 30 Jun 2017, Igor Druzhinin wrote:
> Non-functional change.
> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  hw/i386/xen/xen-hvm.c | 57 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 31 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> index cffa7e2..d259cf7 100644
> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c
> @@ -305,6 +305,36 @@ static hwaddr xen_phys_offset_to_gaddr(hwaddr start_addr,
>      return start_addr;
>  }
>  
> +static int xen_save_physmap(XenIOState *state, XenPhysmap *physmap)
> +{
> +    char path[80], value[17];
> +
> +    snprintf(path, sizeof(path),
> +            "/local/domain/0/device-model/%d/physmap/%"PRIx64"/start_addr",
> +            xen_domid, (uint64_t)physmap->phys_offset);
> +    snprintf(value, sizeof(value), "%"PRIx64, (uint64_t)physmap->start_addr);
> +    if (!xs_write(state->xenstore, 0, path, value, strlen(value))) {
> +        return -1;
> +    }
> +    snprintf(path, sizeof(path),
> +            "/local/domain/0/device-model/%d/physmap/%"PRIx64"/size",
> +            xen_domid, (uint64_t)physmap->phys_offset);
> +    snprintf(value, sizeof(value), "%"PRIx64, (uint64_t)physmap->size);
> +    if (!xs_write(state->xenstore, 0, path, value, strlen(value))) {
> +        return -1;
> +    }
> +    if (physmap->name) {
> +        snprintf(path, sizeof(path),
> +                "/local/domain/0/device-model/%d/physmap/%"PRIx64"/name",
> +                xen_domid, (uint64_t)physmap->phys_offset);
> +        if (!xs_write(state->xenstore, 0, path,
> +                      physmap->name, strlen(physmap->name))) {
> +            return -1;
> +        }
> +    }
> +    return 0;
> +}
> +
>  static int xen_add_to_physmap(XenIOState *state,
>                                hwaddr start_addr,
>                                ram_addr_t size,
> @@ -316,7 +346,6 @@ static int xen_add_to_physmap(XenIOState *state,
>      XenPhysmap *physmap = NULL;
>      hwaddr pfn, start_gpfn;
>      hwaddr phys_offset = memory_region_get_ram_addr(mr);
> -    char path[80], value[17];
>      const char *mr_name;
>  
>      if (get_physmapping(state, start_addr, size)) {
> @@ -368,31 +397,7 @@ go_physmap:
>                                     start_addr >> TARGET_PAGE_BITS,
>                                     (start_addr + size - 1) >> TARGET_PAGE_BITS,
>                                     XEN_DOMCTL_MEM_CACHEATTR_WB);
> -
> -    snprintf(path, sizeof(path),
> -            "/local/domain/0/device-model/%d/physmap/%"PRIx64"/start_addr",
> -            xen_domid, (uint64_t)phys_offset);
> -    snprintf(value, sizeof(value), "%"PRIx64, (uint64_t)start_addr);
> -    if (!xs_write(state->xenstore, 0, path, value, strlen(value))) {
> -        return -1;
> -    }
> -    snprintf(path, sizeof(path),
> -            "/local/domain/0/device-model/%d/physmap/%"PRIx64"/size",
> -            xen_domid, (uint64_t)phys_offset);
> -    snprintf(value, sizeof(value), "%"PRIx64, (uint64_t)size);
> -    if (!xs_write(state->xenstore, 0, path, value, strlen(value))) {
> -        return -1;
> -    }
> -    if (mr_name) {
> -        snprintf(path, sizeof(path),
> -                "/local/domain/0/device-model/%d/physmap/%"PRIx64"/name",
> -                xen_domid, (uint64_t)phys_offset);
> -        if (!xs_write(state->xenstore, 0, path, mr_name, strlen(mr_name))) {
> -            return -1;
> -        }
> -    }
> -
> -    return 0;
> +    return xen_save_physmap(state, physmap);
>  }
>  
>  static int xen_remove_from_physmap(XenIOState *state,
> -- 
> 2.7.4
> 

Re: [PATCH 1/4] xen: move physmap saving into a separate function

[Stefano Stabellini]

On Fri, 30 Jun 2017, Igor Druzhinin wrote:
> Non-functional change.
> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  hw/i386/xen/xen-hvm.c | 57 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 31 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> index cffa7e2..d259cf7 100644
> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c
> @@ -305,6 +305,36 @@ static hwaddr xen_phys_offset_to_gaddr(hwaddr start_addr,
>      return start_addr;
>  }
>  
> +static int xen_save_physmap(XenIOState *state, XenPhysmap *physmap)
> +{
> +    char path[80], value[17];
> +
> +    snprintf(path, sizeof(path),
> +            "/local/domain/0/device-model/%d/physmap/%"PRIx64"/start_addr",
> +            xen_domid, (uint64_t)physmap->phys_offset);
> +    snprintf(value, sizeof(value), "%"PRIx64, (uint64_t)physmap->start_addr);
> +    if (!xs_write(state->xenstore, 0, path, value, strlen(value))) {
> +        return -1;
> +    }
> +    snprintf(path, sizeof(path),
> +            "/local/domain/0/device-model/%d/physmap/%"PRIx64"/size",
> +            xen_domid, (uint64_t)physmap->phys_offset);
> +    snprintf(value, sizeof(value), "%"PRIx64, (uint64_t)physmap->size);
> +    if (!xs_write(state->xenstore, 0, path, value, strlen(value))) {
> +        return -1;
> +    }
> +    if (physmap->name) {
> +        snprintf(path, sizeof(path),
> +                "/local/domain/0/device-model/%d/physmap/%"PRIx64"/name",
> +                xen_domid, (uint64_t)physmap->phys_offset);
> +        if (!xs_write(state->xenstore, 0, path,
> +                      physmap->name, strlen(physmap->name))) {
> +            return -1;
> +        }
> +    }
> +    return 0;
> +}
> +
>  static int xen_add_to_physmap(XenIOState *state,
>                                hwaddr start_addr,
>                                ram_addr_t size,
> @@ -316,7 +346,6 @@ static int xen_add_to_physmap(XenIOState *state,
>      XenPhysmap *physmap = NULL;
>      hwaddr pfn, start_gpfn;
>      hwaddr phys_offset = memory_region_get_ram_addr(mr);
> -    char path[80], value[17];
>      const char *mr_name;
>  
>      if (get_physmapping(state, start_addr, size)) {
> @@ -368,31 +397,7 @@ go_physmap:
>                                     start_addr >> TARGET_PAGE_BITS,
>                                     (start_addr + size - 1) >> TARGET_PAGE_BITS,
>                                     XEN_DOMCTL_MEM_CACHEATTR_WB);
> -
> -    snprintf(path, sizeof(path),
> -            "/local/domain/0/device-model/%d/physmap/%"PRIx64"/start_addr",
> -            xen_domid, (uint64_t)phys_offset);
> -    snprintf(value, sizeof(value), "%"PRIx64, (uint64_t)start_addr);
> -    if (!xs_write(state->xenstore, 0, path, value, strlen(value))) {
> -        return -1;
> -    }
> -    snprintf(path, sizeof(path),
> -            "/local/domain/0/device-model/%d/physmap/%"PRIx64"/size",
> -            xen_domid, (uint64_t)phys_offset);
> -    snprintf(value, sizeof(value), "%"PRIx64, (uint64_t)size);
> -    if (!xs_write(state->xenstore, 0, path, value, strlen(value))) {
> -        return -1;
> -    }
> -    if (mr_name) {
> -        snprintf(path, sizeof(path),
> -                "/local/domain/0/device-model/%d/physmap/%"PRIx64"/name",
> -                xen_domid, (uint64_t)phys_offset);
> -        if (!xs_write(state->xenstore, 0, path, mr_name, strlen(mr_name))) {
> -            return -1;
> -        }
> -    }
> -
> -    return 0;
> +    return xen_save_physmap(state, physmap);
>  }
>  
>  static int xen_remove_from_physmap(XenIOState *state,
> -- 
> 2.7.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Qemu-devel] [PATCH 2/4] xen/mapcache: add an ability to create dummy mappings

[Igor Druzhinin]

Dummys are simple anonymous mappings that are placed instead
of regular foreign mappings in certain situations when we need
to postpone the actual mapping but still have to give a
memory region to QEMU to play with.

This is planned to be used for restore on Xen.

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
 hw/i386/xen/xen-mapcache.c | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
index e60156c..05050de 100644
--- a/hw/i386/xen/xen-mapcache.c
+++ b/hw/i386/xen/xen-mapcache.c
@@ -150,7 +150,8 @@ void xen_map_cache_init(phys_offset_to_gaddr_t f, void *opaque)
 
 static void xen_remap_bucket(MapCacheEntry *entry,
                              hwaddr size,
-                             hwaddr address_index)
+                             hwaddr address_index,
+                             bool dummy)
 {
     uint8_t *vaddr_base;
     xen_pfn_t *pfns;
@@ -177,11 +178,25 @@ static void xen_remap_bucket(MapCacheEntry *entry,
         pfns[i] = (address_index << (MCACHE_BUCKET_SHIFT-XC_PAGE_SHIFT)) + i;
     }
 
-    vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid, PROT_READ|PROT_WRITE,
-                                      nb_pfn, pfns, err);
-    if (vaddr_base == NULL) {
-        perror("xenforeignmemory_map");
-        exit(-1);
+    if (!dummy) {
+        vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid,
+                                           PROT_READ|PROT_WRITE,
+                                           nb_pfn, pfns, err);
+        if (vaddr_base == NULL) {
+            perror("xenforeignmemory_map");
+            exit(-1);
+        }
+    } else {
+        /*
+         * We create dummy mappings where we are unable to create a foreign
+         * mapping immediately due to certain circumstances (i.e. on resume now)
+         */
+        vaddr_base = mmap(NULL, size, PROT_READ|PROT_WRITE,
+                          MAP_ANON|MAP_SHARED, -1, 0);
+        if (vaddr_base == NULL) {
+            perror("mmap");
+            exit(-1);
+        }
     }
 
     entry->vaddr_base = vaddr_base;
@@ -211,6 +226,7 @@ static uint8_t *xen_map_cache_unlocked(hwaddr phys_addr, hwaddr size,
     hwaddr cache_size = size;
     hwaddr test_bit_size;
     bool translated = false;
+    bool dummy = false;
 
 tryagain:
     address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
@@ -262,14 +278,14 @@ tryagain:
     if (!entry) {
         entry = g_malloc0(sizeof (MapCacheEntry));
         pentry->next = entry;
-        xen_remap_bucket(entry, cache_size, address_index);
+        xen_remap_bucket(entry, cache_size, address_index, dummy);
     } else if (!entry->lock) {
         if (!entry->vaddr_base || entry->paddr_index != address_index ||
                 entry->size != cache_size ||
                 !test_bits(address_offset >> XC_PAGE_SHIFT,
                     test_bit_size >> XC_PAGE_SHIFT,
                     entry->valid_mapping)) {
-            xen_remap_bucket(entry, cache_size, address_index);
+            xen_remap_bucket(entry, cache_size, address_index, dummy);
         }
     }
 
@@ -282,6 +298,10 @@ tryagain:
             translated = true;
             goto tryagain;
         }
+        if (!dummy && runstate_check(RUN_STATE_INMIGRATE)) {
+            dummy = true;
+            goto tryagain;
+        }
         trace_xen_map_cache_return(NULL);
         return NULL;
     }
-- 
2.7.4

[PATCH 2/4] xen/mapcache: add an ability to create dummy mappings

[Igor Druzhinin]

Dummys are simple anonymous mappings that are placed instead
of regular foreign mappings in certain situations when we need
to postpone the actual mapping but still have to give a
memory region to QEMU to play with.

This is planned to be used for restore on Xen.

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
 hw/i386/xen/xen-mapcache.c | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
index e60156c..05050de 100644
--- a/hw/i386/xen/xen-mapcache.c
+++ b/hw/i386/xen/xen-mapcache.c
@@ -150,7 +150,8 @@ void xen_map_cache_init(phys_offset_to_gaddr_t f, void *opaque)
 
 static void xen_remap_bucket(MapCacheEntry *entry,
                              hwaddr size,
-                             hwaddr address_index)
+                             hwaddr address_index,
+                             bool dummy)
 {
     uint8_t *vaddr_base;
     xen_pfn_t *pfns;
@@ -177,11 +178,25 @@ static void xen_remap_bucket(MapCacheEntry *entry,
         pfns[i] = (address_index << (MCACHE_BUCKET_SHIFT-XC_PAGE_SHIFT)) + i;
     }
 
-    vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid, PROT_READ|PROT_WRITE,
-                                      nb_pfn, pfns, err);
-    if (vaddr_base == NULL) {
-        perror("xenforeignmemory_map");
-        exit(-1);
+    if (!dummy) {
+        vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid,
+                                           PROT_READ|PROT_WRITE,
+                                           nb_pfn, pfns, err);
+        if (vaddr_base == NULL) {
+            perror("xenforeignmemory_map");
+            exit(-1);
+        }
+    } else {
+        /*
+         * We create dummy mappings where we are unable to create a foreign
+         * mapping immediately due to certain circumstances (i.e. on resume now)
+         */
+        vaddr_base = mmap(NULL, size, PROT_READ|PROT_WRITE,
+                          MAP_ANON|MAP_SHARED, -1, 0);
+        if (vaddr_base == NULL) {
+            perror("mmap");
+            exit(-1);
+        }
     }
 
     entry->vaddr_base = vaddr_base;
@@ -211,6 +226,7 @@ static uint8_t *xen_map_cache_unlocked(hwaddr phys_addr, hwaddr size,
     hwaddr cache_size = size;
     hwaddr test_bit_size;
     bool translated = false;
+    bool dummy = false;
 
 tryagain:
     address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
@@ -262,14 +278,14 @@ tryagain:
     if (!entry) {
         entry = g_malloc0(sizeof (MapCacheEntry));
         pentry->next = entry;
-        xen_remap_bucket(entry, cache_size, address_index);
+        xen_remap_bucket(entry, cache_size, address_index, dummy);
     } else if (!entry->lock) {
         if (!entry->vaddr_base || entry->paddr_index != address_index ||
                 entry->size != cache_size ||
                 !test_bits(address_offset >> XC_PAGE_SHIFT,
                     test_bit_size >> XC_PAGE_SHIFT,
                     entry->valid_mapping)) {
-            xen_remap_bucket(entry, cache_size, address_index);
+            xen_remap_bucket(entry, cache_size, address_index, dummy);
         }
     }
 
@@ -282,6 +298,10 @@ tryagain:
             translated = true;
             goto tryagain;
         }
+        if (!dummy && runstate_check(RUN_STATE_INMIGRATE)) {
+            dummy = true;
+            goto tryagain;
+        }
         trace_xen_map_cache_return(NULL);
         return NULL;
     }
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Qemu-devel] [PATCH 2/4] xen/mapcache: add an ability to create dummy mappings

[Stefano Stabellini]

On Fri, 30 Jun 2017, Igor Druzhinin wrote:
> Dummys are simple anonymous mappings that are placed instead
> of regular foreign mappings in certain situations when we need
> to postpone the actual mapping but still have to give a
> memory region to QEMU to play with.
> 
> This is planned to be used for restore on Xen.
> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>
> ---
>  hw/i386/xen/xen-mapcache.c | 36 ++++++++++++++++++++++++++++--------
>  1 file changed, 28 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
> index e60156c..05050de 100644
> --- a/hw/i386/xen/xen-mapcache.c
> +++ b/hw/i386/xen/xen-mapcache.c
> @@ -150,7 +150,8 @@ void xen_map_cache_init(phys_offset_to_gaddr_t f, void *opaque)
>  
>  static void xen_remap_bucket(MapCacheEntry *entry,
>                               hwaddr size,
> -                             hwaddr address_index)
> +                             hwaddr address_index,
> +                             bool dummy)
>  {
>      uint8_t *vaddr_base;
>      xen_pfn_t *pfns;
> @@ -177,11 +178,25 @@ static void xen_remap_bucket(MapCacheEntry *entry,
>          pfns[i] = (address_index << (MCACHE_BUCKET_SHIFT-XC_PAGE_SHIFT)) + i;
>      }
>  
> -    vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid, PROT_READ|PROT_WRITE,
> -                                      nb_pfn, pfns, err);
> -    if (vaddr_base == NULL) {
> -        perror("xenforeignmemory_map");
> -        exit(-1);
> +    if (!dummy) {
> +        vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid,
> +                                           PROT_READ|PROT_WRITE,
> +                                           nb_pfn, pfns, err);
> +        if (vaddr_base == NULL) {
> +            perror("xenforeignmemory_map");
> +            exit(-1);
> +        }
> +    } else {
> +        /*
> +         * We create dummy mappings where we are unable to create a foreign
> +         * mapping immediately due to certain circumstances (i.e. on resume now)
> +         */
> +        vaddr_base = mmap(NULL, size, PROT_READ|PROT_WRITE,
> +                          MAP_ANON|MAP_SHARED, -1, 0);
> +        if (vaddr_base == NULL) {
> +            perror("mmap");
> +            exit(-1);
> +        }

For our sanity in debugging this in the future, I think it's best if we
mark this mapcache entry as "dummy". Since we are at it, we could turn
the lock field of MapCacheEntry into a flag field and #define LOCK as
(1<<0) and DUMMY as (1<<1). Please do that as a separate patch.


>      }
>  
>      entry->vaddr_base = vaddr_base;
> @@ -211,6 +226,7 @@ static uint8_t *xen_map_cache_unlocked(hwaddr phys_addr, hwaddr size,
>      hwaddr cache_size = size;
>      hwaddr test_bit_size;
>      bool translated = false;
> +    bool dummy = false;
>  
>  tryagain:
>      address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
> @@ -262,14 +278,14 @@ tryagain:
>      if (!entry) {
>          entry = g_malloc0(sizeof (MapCacheEntry));
>          pentry->next = entry;
> -        xen_remap_bucket(entry, cache_size, address_index);
> +        xen_remap_bucket(entry, cache_size, address_index, dummy);
>      } else if (!entry->lock) {
>          if (!entry->vaddr_base || entry->paddr_index != address_index ||
>                  entry->size != cache_size ||
>                  !test_bits(address_offset >> XC_PAGE_SHIFT,
>                      test_bit_size >> XC_PAGE_SHIFT,
>                      entry->valid_mapping)) {
> -            xen_remap_bucket(entry, cache_size, address_index);
> +            xen_remap_bucket(entry, cache_size, address_index, dummy);
>          }
>      }
>  
> @@ -282,6 +298,10 @@ tryagain:
>              translated = true;
>              goto tryagain;
>          }
> +        if (!dummy && runstate_check(RUN_STATE_INMIGRATE)) {
> +            dummy = true;
> +            goto tryagain;
> +        }
>          trace_xen_map_cache_return(NULL);
>          return NULL;
>      }
> -- 
> 2.7.4
> 

Re: [PATCH 2/4] xen/mapcache: add an ability to create dummy mappings

[Stefano Stabellini]

On Fri, 30 Jun 2017, Igor Druzhinin wrote:
> Dummys are simple anonymous mappings that are placed instead
> of regular foreign mappings in certain situations when we need
> to postpone the actual mapping but still have to give a
> memory region to QEMU to play with.
> 
> This is planned to be used for restore on Xen.
> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>
> ---
>  hw/i386/xen/xen-mapcache.c | 36 ++++++++++++++++++++++++++++--------
>  1 file changed, 28 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
> index e60156c..05050de 100644
> --- a/hw/i386/xen/xen-mapcache.c
> +++ b/hw/i386/xen/xen-mapcache.c
> @@ -150,7 +150,8 @@ void xen_map_cache_init(phys_offset_to_gaddr_t f, void *opaque)
>  
>  static void xen_remap_bucket(MapCacheEntry *entry,
>                               hwaddr size,
> -                             hwaddr address_index)
> +                             hwaddr address_index,
> +                             bool dummy)
>  {
>      uint8_t *vaddr_base;
>      xen_pfn_t *pfns;
> @@ -177,11 +178,25 @@ static void xen_remap_bucket(MapCacheEntry *entry,
>          pfns[i] = (address_index << (MCACHE_BUCKET_SHIFT-XC_PAGE_SHIFT)) + i;
>      }
>  
> -    vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid, PROT_READ|PROT_WRITE,
> -                                      nb_pfn, pfns, err);
> -    if (vaddr_base == NULL) {
> -        perror("xenforeignmemory_map");
> -        exit(-1);
> +    if (!dummy) {
> +        vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid,
> +                                           PROT_READ|PROT_WRITE,
> +                                           nb_pfn, pfns, err);
> +        if (vaddr_base == NULL) {
> +            perror("xenforeignmemory_map");
> +            exit(-1);
> +        }
> +    } else {
> +        /*
> +         * We create dummy mappings where we are unable to create a foreign
> +         * mapping immediately due to certain circumstances (i.e. on resume now)
> +         */
> +        vaddr_base = mmap(NULL, size, PROT_READ|PROT_WRITE,
> +                          MAP_ANON|MAP_SHARED, -1, 0);
> +        if (vaddr_base == NULL) {
> +            perror("mmap");
> +            exit(-1);
> +        }

For our sanity in debugging this in the future, I think it's best if we
mark this mapcache entry as "dummy". Since we are at it, we could turn
the lock field of MapCacheEntry into a flag field and #define LOCK as
(1<<0) and DUMMY as (1<<1). Please do that as a separate patch.


>      }
>  
>      entry->vaddr_base = vaddr_base;
> @@ -211,6 +226,7 @@ static uint8_t *xen_map_cache_unlocked(hwaddr phys_addr, hwaddr size,
>      hwaddr cache_size = size;
>      hwaddr test_bit_size;
>      bool translated = false;
> +    bool dummy = false;
>  
>  tryagain:
>      address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
> @@ -262,14 +278,14 @@ tryagain:
>      if (!entry) {
>          entry = g_malloc0(sizeof (MapCacheEntry));
>          pentry->next = entry;
> -        xen_remap_bucket(entry, cache_size, address_index);
> +        xen_remap_bucket(entry, cache_size, address_index, dummy);
>      } else if (!entry->lock) {
>          if (!entry->vaddr_base || entry->paddr_index != address_index ||
>                  entry->size != cache_size ||
>                  !test_bits(address_offset >> XC_PAGE_SHIFT,
>                      test_bit_size >> XC_PAGE_SHIFT,
>                      entry->valid_mapping)) {
> -            xen_remap_bucket(entry, cache_size, address_index);
> +            xen_remap_bucket(entry, cache_size, address_index, dummy);
>          }
>      }
>  
> @@ -282,6 +298,10 @@ tryagain:
>              translated = true;
>              goto tryagain;
>          }
> +        if (!dummy && runstate_check(RUN_STATE_INMIGRATE)) {
> +            dummy = true;
> +            goto tryagain;
> +        }
>          trace_xen_map_cache_return(NULL);
>          return NULL;
>      }
> -- 
> 2.7.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Qemu-devel] [PATCH 2/4] xen/mapcache: add an ability to create dummy mappings

[Igor Druzhinin]

On 01/07/17 01:06, Stefano Stabellini wrote:
> On Fri, 30 Jun 2017, Igor Druzhinin wrote:
>> Dummys are simple anonymous mappings that are placed instead
>> of regular foreign mappings in certain situations when we need
>> to postpone the actual mapping but still have to give a
>> memory region to QEMU to play with.
>>
>> This is planned to be used for restore on Xen.
>>
>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>>
>> ---
>>  hw/i386/xen/xen-mapcache.c | 36 ++++++++++++++++++++++++++++--------
>>  1 file changed, 28 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
>> index e60156c..05050de 100644
>> --- a/hw/i386/xen/xen-mapcache.c
>> +++ b/hw/i386/xen/xen-mapcache.c
>> @@ -150,7 +150,8 @@ void xen_map_cache_init(phys_offset_to_gaddr_t f, void *opaque)
>>  
>>  static void xen_remap_bucket(MapCacheEntry *entry,
>>                               hwaddr size,
>> -                             hwaddr address_index)
>> +                             hwaddr address_index,
>> +                             bool dummy)
>>  {
>>      uint8_t *vaddr_base;
>>      xen_pfn_t *pfns;
>> @@ -177,11 +178,25 @@ static void xen_remap_bucket(MapCacheEntry *entry,
>>          pfns[i] = (address_index << (MCACHE_BUCKET_SHIFT-XC_PAGE_SHIFT)) + i;
>>      }
>>  
>> -    vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid, PROT_READ|PROT_WRITE,
>> -                                      nb_pfn, pfns, err);
>> -    if (vaddr_base == NULL) {
>> -        perror("xenforeignmemory_map");
>> -        exit(-1);
>> +    if (!dummy) {
>> +        vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid,
>> +                                           PROT_READ|PROT_WRITE,
>> +                                           nb_pfn, pfns, err);
>> +        if (vaddr_base == NULL) {
>> +            perror("xenforeignmemory_map");
>> +            exit(-1);
>> +        }
>> +    } else {
>> +        /*
>> +         * We create dummy mappings where we are unable to create a foreign
>> +         * mapping immediately due to certain circumstances (i.e. on resume now)
>> +         */
>> +        vaddr_base = mmap(NULL, size, PROT_READ|PROT_WRITE,
>> +                          MAP_ANON|MAP_SHARED, -1, 0);
>> +        if (vaddr_base == NULL) {
>> +            perror("mmap");
>> +            exit(-1);
>> +        }
> 
> For our sanity in debugging this in the future, I think it's best if we
> mark this mapcache entry as "dummy". Since we are at it, we could turn
> the lock field of MapCacheEntry into a flag field and #define LOCK as
> (1<<0) and DUMMY as (1<<1). Please do that as a separate patch.
>

Unfortunately, lock field is a reference counter (or at least it looks
like according to the source code). It seems to me that it's technically
possible to have one region locked from several places in QEMU code. For
that reason, I'd like to introduce a separate field - something like
uint8_t flags.

Igor

>>>      }
>>  
>>      entry->vaddr_base = vaddr_base;
>> @@ -211,6 +226,7 @@ static uint8_t *xen_map_cache_unlocked(hwaddr phys_addr, hwaddr size,
>>      hwaddr cache_size = size;
>>      hwaddr test_bit_size;
>>      bool translated = false;
>> +    bool dummy = false;
>>  
>>  tryagain:
>>      address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
>> @@ -262,14 +278,14 @@ tryagain:
>>      if (!entry) {
>>          entry = g_malloc0(sizeof (MapCacheEntry));
>>          pentry->next = entry;
>> -        xen_remap_bucket(entry, cache_size, address_index);
>> +        xen_remap_bucket(entry, cache_size, address_index, dummy);
>>      } else if (!entry->lock) {
>>          if (!entry->vaddr_base || entry->paddr_index != address_index ||
>>                  entry->size != cache_size ||
>>                  !test_bits(address_offset >> XC_PAGE_SHIFT,
>>                      test_bit_size >> XC_PAGE_SHIFT,
>>                      entry->valid_mapping)) {
>> -            xen_remap_bucket(entry, cache_size, address_index);
>> +            xen_remap_bucket(entry, cache_size, address_index, dummy);
>>          }
>>      }
>>  
>> @@ -282,6 +298,10 @@ tryagain:
>>              translated = true;
>>              goto tryagain;
>>          }
>> +        if (!dummy && runstate_check(RUN_STATE_INMIGRATE)) {
>> +            dummy = true;
>> +            goto tryagain;
>> +        }
>>          trace_xen_map_cache_return(NULL);
>>          return NULL;
>>      }
>> -- 
>> 2.7.4
>>

Re: [PATCH 2/4] xen/mapcache: add an ability to create dummy mappings

[Igor Druzhinin]

On 01/07/17 01:06, Stefano Stabellini wrote:
> On Fri, 30 Jun 2017, Igor Druzhinin wrote:
>> Dummys are simple anonymous mappings that are placed instead
>> of regular foreign mappings in certain situations when we need
>> to postpone the actual mapping but still have to give a
>> memory region to QEMU to play with.
>>
>> This is planned to be used for restore on Xen.
>>
>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>>
>> ---
>>  hw/i386/xen/xen-mapcache.c | 36 ++++++++++++++++++++++++++++--------
>>  1 file changed, 28 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
>> index e60156c..05050de 100644
>> --- a/hw/i386/xen/xen-mapcache.c
>> +++ b/hw/i386/xen/xen-mapcache.c
>> @@ -150,7 +150,8 @@ void xen_map_cache_init(phys_offset_to_gaddr_t f, void *opaque)
>>  
>>  static void xen_remap_bucket(MapCacheEntry *entry,
>>                               hwaddr size,
>> -                             hwaddr address_index)
>> +                             hwaddr address_index,
>> +                             bool dummy)
>>  {
>>      uint8_t *vaddr_base;
>>      xen_pfn_t *pfns;
>> @@ -177,11 +178,25 @@ static void xen_remap_bucket(MapCacheEntry *entry,
>>          pfns[i] = (address_index << (MCACHE_BUCKET_SHIFT-XC_PAGE_SHIFT)) + i;
>>      }
>>  
>> -    vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid, PROT_READ|PROT_WRITE,
>> -                                      nb_pfn, pfns, err);
>> -    if (vaddr_base == NULL) {
>> -        perror("xenforeignmemory_map");
>> -        exit(-1);
>> +    if (!dummy) {
>> +        vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid,
>> +                                           PROT_READ|PROT_WRITE,
>> +                                           nb_pfn, pfns, err);
>> +        if (vaddr_base == NULL) {
>> +            perror("xenforeignmemory_map");
>> +            exit(-1);
>> +        }
>> +    } else {
>> +        /*
>> +         * We create dummy mappings where we are unable to create a foreign
>> +         * mapping immediately due to certain circumstances (i.e. on resume now)
>> +         */
>> +        vaddr_base = mmap(NULL, size, PROT_READ|PROT_WRITE,
>> +                          MAP_ANON|MAP_SHARED, -1, 0);
>> +        if (vaddr_base == NULL) {
>> +            perror("mmap");
>> +            exit(-1);
>> +        }
> 
> For our sanity in debugging this in the future, I think it's best if we
> mark this mapcache entry as "dummy". Since we are at it, we could turn
> the lock field of MapCacheEntry into a flag field and #define LOCK as
> (1<<0) and DUMMY as (1<<1). Please do that as a separate patch.
>

Unfortunately, lock field is a reference counter (or at least it looks
like according to the source code). It seems to me that it's technically
possible to have one region locked from several places in QEMU code. For
that reason, I'd like to introduce a separate field - something like
uint8_t flags.

Igor

>>>      }
>>  
>>      entry->vaddr_base = vaddr_base;
>> @@ -211,6 +226,7 @@ static uint8_t *xen_map_cache_unlocked(hwaddr phys_addr, hwaddr size,
>>      hwaddr cache_size = size;
>>      hwaddr test_bit_size;
>>      bool translated = false;
>> +    bool dummy = false;
>>  
>>  tryagain:
>>      address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
>> @@ -262,14 +278,14 @@ tryagain:
>>      if (!entry) {
>>          entry = g_malloc0(sizeof (MapCacheEntry));
>>          pentry->next = entry;
>> -        xen_remap_bucket(entry, cache_size, address_index);
>> +        xen_remap_bucket(entry, cache_size, address_index, dummy);
>>      } else if (!entry->lock) {
>>          if (!entry->vaddr_base || entry->paddr_index != address_index ||
>>                  entry->size != cache_size ||
>>                  !test_bits(address_offset >> XC_PAGE_SHIFT,
>>                      test_bit_size >> XC_PAGE_SHIFT,
>>                      entry->valid_mapping)) {
>> -            xen_remap_bucket(entry, cache_size, address_index);
>> +            xen_remap_bucket(entry, cache_size, address_index, dummy);
>>          }
>>      }
>>  
>> @@ -282,6 +298,10 @@ tryagain:
>>              translated = true;
>>              goto tryagain;
>>          }
>> +        if (!dummy && runstate_check(RUN_STATE_INMIGRATE)) {
>> +            dummy = true;
>> +            goto tryagain;
>> +        }
>>          trace_xen_map_cache_return(NULL);
>>          return NULL;
>>      }
>> -- 
>> 2.7.4
>>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Qemu-devel] [PATCH 2/4] xen/mapcache: add an ability to create dummy mappings

[Stefano Stabellini]

On Mon, 3 Jul 2017, Igor Druzhinin wrote:
> On 01/07/17 01:06, Stefano Stabellini wrote:
> > On Fri, 30 Jun 2017, Igor Druzhinin wrote:
> >> Dummys are simple anonymous mappings that are placed instead
> >> of regular foreign mappings in certain situations when we need
> >> to postpone the actual mapping but still have to give a
> >> memory region to QEMU to play with.
> >>
> >> This is planned to be used for restore on Xen.
> >>
> >> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> >>
> >> ---
> >>  hw/i386/xen/xen-mapcache.c | 36 ++++++++++++++++++++++++++++--------
> >>  1 file changed, 28 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
> >> index e60156c..05050de 100644
> >> --- a/hw/i386/xen/xen-mapcache.c
> >> +++ b/hw/i386/xen/xen-mapcache.c
> >> @@ -150,7 +150,8 @@ void xen_map_cache_init(phys_offset_to_gaddr_t f, void *opaque)
> >>  
> >>  static void xen_remap_bucket(MapCacheEntry *entry,
> >>                               hwaddr size,
> >> -                             hwaddr address_index)
> >> +                             hwaddr address_index,
> >> +                             bool dummy)
> >>  {
> >>      uint8_t *vaddr_base;
> >>      xen_pfn_t *pfns;
> >> @@ -177,11 +178,25 @@ static void xen_remap_bucket(MapCacheEntry *entry,
> >>          pfns[i] = (address_index << (MCACHE_BUCKET_SHIFT-XC_PAGE_SHIFT)) + i;
> >>      }
> >>  
> >> -    vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid, PROT_READ|PROT_WRITE,
> >> -                                      nb_pfn, pfns, err);
> >> -    if (vaddr_base == NULL) {
> >> -        perror("xenforeignmemory_map");
> >> -        exit(-1);
> >> +    if (!dummy) {
> >> +        vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid,
> >> +                                           PROT_READ|PROT_WRITE,
> >> +                                           nb_pfn, pfns, err);
> >> +        if (vaddr_base == NULL) {
> >> +            perror("xenforeignmemory_map");
> >> +            exit(-1);
> >> +        }
> >> +    } else {
> >> +        /*
> >> +         * We create dummy mappings where we are unable to create a foreign
> >> +         * mapping immediately due to certain circumstances (i.e. on resume now)
> >> +         */
> >> +        vaddr_base = mmap(NULL, size, PROT_READ|PROT_WRITE,
> >> +                          MAP_ANON|MAP_SHARED, -1, 0);
> >> +        if (vaddr_base == NULL) {
> >> +            perror("mmap");
> >> +            exit(-1);
> >> +        }
> > 
> > For our sanity in debugging this in the future, I think it's best if we
> > mark this mapcache entry as "dummy". Since we are at it, we could turn
> > the lock field of MapCacheEntry into a flag field and #define LOCK as
> > (1<<0) and DUMMY as (1<<1). Please do that as a separate patch.
> >
> 
> Unfortunately, lock field is a reference counter (or at least it looks
> like according to the source code). It seems to me that it's technically
> possible to have one region locked from several places in QEMU code. For
> that reason, I'd like to introduce a separate field - something like
> uint8_t flags.

Yes, you are right.


> >>>      }
> >>  
> >>      entry->vaddr_base = vaddr_base;
> >> @@ -211,6 +226,7 @@ static uint8_t *xen_map_cache_unlocked(hwaddr phys_addr, hwaddr size,
> >>      hwaddr cache_size = size;
> >>      hwaddr test_bit_size;
> >>      bool translated = false;
> >> +    bool dummy = false;
> >>  
> >>  tryagain:
> >>      address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
> >> @@ -262,14 +278,14 @@ tryagain:
> >>      if (!entry) {
> >>          entry = g_malloc0(sizeof (MapCacheEntry));
> >>          pentry->next = entry;
> >> -        xen_remap_bucket(entry, cache_size, address_index);
> >> +        xen_remap_bucket(entry, cache_size, address_index, dummy);
> >>      } else if (!entry->lock) {
> >>          if (!entry->vaddr_base || entry->paddr_index != address_index ||
> >>                  entry->size != cache_size ||
> >>                  !test_bits(address_offset >> XC_PAGE_SHIFT,
> >>                      test_bit_size >> XC_PAGE_SHIFT,
> >>                      entry->valid_mapping)) {
> >> -            xen_remap_bucket(entry, cache_size, address_index);
> >> +            xen_remap_bucket(entry, cache_size, address_index, dummy);
> >>          }
> >>      }
> >>  
> >> @@ -282,6 +298,10 @@ tryagain:
> >>              translated = true;
> >>              goto tryagain;
> >>          }
> >> +        if (!dummy && runstate_check(RUN_STATE_INMIGRATE)) {
> >> +            dummy = true;
> >> +            goto tryagain;
> >> +        }
> >>          trace_xen_map_cache_return(NULL);
> >>          return NULL;
> >>      }
> >> -- 
> >> 2.7.4
> >>
> 

Re: [PATCH 2/4] xen/mapcache: add an ability to create dummy mappings

[Stefano Stabellini]

On Mon, 3 Jul 2017, Igor Druzhinin wrote:
> On 01/07/17 01:06, Stefano Stabellini wrote:
> > On Fri, 30 Jun 2017, Igor Druzhinin wrote:
> >> Dummys are simple anonymous mappings that are placed instead
> >> of regular foreign mappings in certain situations when we need
> >> to postpone the actual mapping but still have to give a
> >> memory region to QEMU to play with.
> >>
> >> This is planned to be used for restore on Xen.
> >>
> >> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> >>
> >> ---
> >>  hw/i386/xen/xen-mapcache.c | 36 ++++++++++++++++++++++++++++--------
> >>  1 file changed, 28 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
> >> index e60156c..05050de 100644
> >> --- a/hw/i386/xen/xen-mapcache.c
> >> +++ b/hw/i386/xen/xen-mapcache.c
> >> @@ -150,7 +150,8 @@ void xen_map_cache_init(phys_offset_to_gaddr_t f, void *opaque)
> >>  
> >>  static void xen_remap_bucket(MapCacheEntry *entry,
> >>                               hwaddr size,
> >> -                             hwaddr address_index)
> >> +                             hwaddr address_index,
> >> +                             bool dummy)
> >>  {
> >>      uint8_t *vaddr_base;
> >>      xen_pfn_t *pfns;
> >> @@ -177,11 +178,25 @@ static void xen_remap_bucket(MapCacheEntry *entry,
> >>          pfns[i] = (address_index << (MCACHE_BUCKET_SHIFT-XC_PAGE_SHIFT)) + i;
> >>      }
> >>  
> >> -    vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid, PROT_READ|PROT_WRITE,
> >> -                                      nb_pfn, pfns, err);
> >> -    if (vaddr_base == NULL) {
> >> -        perror("xenforeignmemory_map");
> >> -        exit(-1);
> >> +    if (!dummy) {
> >> +        vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid,
> >> +                                           PROT_READ|PROT_WRITE,
> >> +                                           nb_pfn, pfns, err);
> >> +        if (vaddr_base == NULL) {
> >> +            perror("xenforeignmemory_map");
> >> +            exit(-1);
> >> +        }
> >> +    } else {
> >> +        /*
> >> +         * We create dummy mappings where we are unable to create a foreign
> >> +         * mapping immediately due to certain circumstances (i.e. on resume now)
> >> +         */
> >> +        vaddr_base = mmap(NULL, size, PROT_READ|PROT_WRITE,
> >> +                          MAP_ANON|MAP_SHARED, -1, 0);
> >> +        if (vaddr_base == NULL) {
> >> +            perror("mmap");
> >> +            exit(-1);
> >> +        }
> > 
> > For our sanity in debugging this in the future, I think it's best if we
> > mark this mapcache entry as "dummy". Since we are at it, we could turn
> > the lock field of MapCacheEntry into a flag field and #define LOCK as
> > (1<<0) and DUMMY as (1<<1). Please do that as a separate patch.
> >
> 
> Unfortunately, lock field is a reference counter (or at least it looks
> like according to the source code). It seems to me that it's technically
> possible to have one region locked from several places in QEMU code. For
> that reason, I'd like to introduce a separate field - something like
> uint8_t flags.

Yes, you are right.


> >>>      }
> >>  
> >>      entry->vaddr_base = vaddr_base;
> >> @@ -211,6 +226,7 @@ static uint8_t *xen_map_cache_unlocked(hwaddr phys_addr, hwaddr size,
> >>      hwaddr cache_size = size;
> >>      hwaddr test_bit_size;
> >>      bool translated = false;
> >> +    bool dummy = false;
> >>  
> >>  tryagain:
> >>      address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
> >> @@ -262,14 +278,14 @@ tryagain:
> >>      if (!entry) {
> >>          entry = g_malloc0(sizeof (MapCacheEntry));
> >>          pentry->next = entry;
> >> -        xen_remap_bucket(entry, cache_size, address_index);
> >> +        xen_remap_bucket(entry, cache_size, address_index, dummy);
> >>      } else if (!entry->lock) {
> >>          if (!entry->vaddr_base || entry->paddr_index != address_index ||
> >>                  entry->size != cache_size ||
> >>                  !test_bits(address_offset >> XC_PAGE_SHIFT,
> >>                      test_bit_size >> XC_PAGE_SHIFT,
> >>                      entry->valid_mapping)) {
> >> -            xen_remap_bucket(entry, cache_size, address_index);
> >> +            xen_remap_bucket(entry, cache_size, address_index, dummy);
> >>          }
> >>      }
> >>  
> >> @@ -282,6 +298,10 @@ tryagain:
> >>              translated = true;
> >>              goto tryagain;
> >>          }
> >> +        if (!dummy && runstate_check(RUN_STATE_INMIGRATE)) {
> >> +            dummy = true;
> >> +            goto tryagain;
> >> +        }
> >>          trace_xen_map_cache_return(NULL);
> >>          return NULL;
> >>      }
> >> -- 
> >> 2.7.4
> >>
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Qemu-devel] [PATCH 3/4] xen/mapcache: introduce xen_remap_cache_entry()

[Igor Druzhinin]

This new call is trying to update a requested map cache entry
according to the changes in the physmap. The call is searching
for the entry, unmaps it, tries to translate the address and
maps again at the same place. If the mapping is dummy this call
will make it real.

This function makes use of a new xenforeignmemory_map2() call
with extended interface that was recently introduced in
libxenforeignmemory [1].

[1] https://www.mail-archive.com/xen-devel@lists.xen.org/msg113007.html

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
 configure                     |  18 ++++++++
 hw/i386/xen/xen-mapcache.c    | 105 +++++++++++++++++++++++++++++++++++++++---
 include/hw/xen/xen_common.h   |   7 +++
 include/sysemu/xen-mapcache.h |   6 +++
 4 files changed, 130 insertions(+), 6 deletions(-)

diff --git a/configure b/configure
index c571ad1..ad6156b 100755
--- a/configure
+++ b/configure
@@ -2021,6 +2021,24 @@ EOF
     # Xen unstable
     elif
         cat > $TMPC <<EOF &&
+#undef XC_WANT_COMPAT_MAP_FOREIGN_API
+#include <xenforeignmemory.h>
+int main(void) {
+  xenforeignmemory_handle *xfmem;
+
+  xfmem = xenforeignmemory_open(0, 0);
+  xenforeignmemory_map2(xfmem, 0, 0, 0, 0, 0, 0, 0);
+
+  return 0;
+}
+EOF
+        compile_prog "" "$xen_libs -lxendevicemodel $xen_stable_libs"
+      then
+      xen_stable_libs="-lxendevicemodel $xen_stable_libs"
+      xen_ctrl_version=41000
+      xen=yes
+    elif
+        cat > $TMPC <<EOF &&
 #undef XC_WANT_COMPAT_DEVICEMODEL_API
 #define __XEN_TOOLS__
 #include <xendevicemodel.h>
diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
index 05050de..5d8d990 100644
--- a/hw/i386/xen/xen-mapcache.c
+++ b/hw/i386/xen/xen-mapcache.c
@@ -149,6 +149,7 @@ void xen_map_cache_init(phys_offset_to_gaddr_t f, void *opaque)
 }
 
 static void xen_remap_bucket(MapCacheEntry *entry,
+                             void *vaddr,
                              hwaddr size,
                              hwaddr address_index,
                              bool dummy)
@@ -179,11 +180,11 @@ static void xen_remap_bucket(MapCacheEntry *entry,
     }
 
     if (!dummy) {
-        vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid,
-                                           PROT_READ|PROT_WRITE,
+        vaddr_base = xenforeignmemory_map2(xen_fmem, xen_domid, vaddr,
+                                           PROT_READ|PROT_WRITE, 0,
                                            nb_pfn, pfns, err);
         if (vaddr_base == NULL) {
-            perror("xenforeignmemory_map");
+            perror("xenforeignmemory_map2");
             exit(-1);
         }
     } else {
@@ -191,7 +192,7 @@ static void xen_remap_bucket(MapCacheEntry *entry,
          * We create dummy mappings where we are unable to create a foreign
          * mapping immediately due to certain circumstances (i.e. on resume now)
          */
-        vaddr_base = mmap(NULL, size, PROT_READ|PROT_WRITE,
+        vaddr_base = mmap(vaddr, size, PROT_READ|PROT_WRITE,
                           MAP_ANON|MAP_SHARED, -1, 0);
         if (vaddr_base == NULL) {
             perror("mmap");
@@ -278,14 +279,14 @@ tryagain:
     if (!entry) {
         entry = g_malloc0(sizeof (MapCacheEntry));
         pentry->next = entry;
-        xen_remap_bucket(entry, cache_size, address_index, dummy);
+        xen_remap_bucket(entry, NULL, cache_size, address_index, dummy);
     } else if (!entry->lock) {
         if (!entry->vaddr_base || entry->paddr_index != address_index ||
                 entry->size != cache_size ||
                 !test_bits(address_offset >> XC_PAGE_SHIFT,
                     test_bit_size >> XC_PAGE_SHIFT,
                     entry->valid_mapping)) {
-            xen_remap_bucket(entry, cache_size, address_index, dummy);
+            xen_remap_bucket(entry, NULL, cache_size, address_index, dummy);
         }
     }
 
@@ -482,3 +483,95 @@ void xen_invalidate_map_cache(void)
 
     mapcache_unlock();
 }
+
+static uint8_t *xen_remap_cache_entry_unlocked(hwaddr phys_addr, hwaddr size)
+{
+    MapCacheEntry *entry, *pentry = NULL;
+    hwaddr address_index;
+    hwaddr address_offset;
+    hwaddr cache_size = size;
+    hwaddr test_bit_size;
+    void *vaddr = NULL;
+    uint8_t lock;
+
+    address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
+    address_offset = phys_addr & (MCACHE_BUCKET_SIZE - 1);
+
+    /* test_bit_size is always a multiple of XC_PAGE_SIZE */
+    if (size) {
+        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);
+        }
+        cache_size = size + address_offset;
+        if (cache_size % MCACHE_BUCKET_SIZE) {
+            cache_size += MCACHE_BUCKET_SIZE - (cache_size % MCACHE_BUCKET_SIZE);
+        }
+    } else {
+        test_bit_size = XC_PAGE_SIZE;
+        cache_size = MCACHE_BUCKET_SIZE;
+    }
+
+    /* Search for the requested map cache entry to invalidate */
+    entry = &mapcache->entry[address_index % mapcache->nr_buckets];
+    while (entry && !(entry->paddr_index == address_index && entry->size == cache_size)) {
+        pentry = entry;
+        entry = entry->next;
+    }
+    if (!entry) {
+        DPRINTF("Trying to update an entry for %lx that is not in the mapcache!\n", phys_addr);
+        return NULL;
+    }
+
+    vaddr = entry->vaddr_base;
+    lock = entry->lock;
+    if (entry->vaddr_base) {
+        ram_block_notify_remove(entry->vaddr_base, entry->size);
+        if (munmap(entry->vaddr_base, entry->size) != 0) {
+            perror("unmap fails");
+            exit(-1);
+        }
+    }
+    entry->vaddr_base = NULL;
+    entry->lock = 0;
+
+    if (mapcache->phys_offset_to_gaddr) {
+        phys_addr = mapcache->phys_offset_to_gaddr(phys_addr, size, mapcache->opaque);
+
+        address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
+        address_offset = phys_addr & (MCACHE_BUCKET_SIZE - 1);
+    }
+
+    /* Address may have changed so we need to repeat the search */
+    entry = &mapcache->entry[address_index % mapcache->nr_buckets];
+    while (entry && entry->lock && entry->vaddr_base) {
+        pentry = entry;
+        entry = entry->next;
+    }
+    if (!entry) {
+        entry = g_malloc0(sizeof (MapCacheEntry));
+        pentry->next = entry;
+    }
+
+    entry->lock = 0;
+    xen_remap_bucket(entry, vaddr, cache_size, address_index, false);
+    if(!test_bits(address_offset >> XC_PAGE_SHIFT,
+                test_bit_size >> XC_PAGE_SHIFT,
+                entry->valid_mapping)) {
+        DPRINTF("Unable to update an entry for %lx in the mapcache!\n", phys_addr);
+        return NULL;
+    }
+
+    entry->lock = lock;
+    return entry->vaddr_base + address_offset;
+}
+
+uint8_t *xen_remap_cache_entry(hwaddr phys_addr, hwaddr size)
+{
+    uint8_t *p;
+
+    mapcache_lock();
+    p = xen_remap_cache_entry_unlocked(phys_addr, size);
+    mapcache_unlock();
+    return p;
+}
diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index e00ddd7..70a5cad 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -78,6 +78,13 @@ static inline void *xenforeignmemory_map(xc_interface *h, uint32_t dom,
 
 extern xenforeignmemory_handle *xen_fmem;
 
+#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
+
+#define xenforeignmemory_map2(h, d, a, p, f, ps, ar, e) \
+    xenforeignmemory_map(h, d, p, ps, ar, e)
+
+#endif
+
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
 
 typedef xc_interface xendevicemodel_handle;
diff --git a/include/sysemu/xen-mapcache.h b/include/sysemu/xen-mapcache.h
index 01daaad..8c140d0 100644
--- a/include/sysemu/xen-mapcache.h
+++ b/include/sysemu/xen-mapcache.h
@@ -21,6 +21,7 @@ uint8_t *xen_map_cache(hwaddr phys_addr, hwaddr size,
 ram_addr_t xen_ram_addr_from_mapcache(void *ptr);
 void xen_invalidate_map_cache_entry(uint8_t *buffer);
 void xen_invalidate_map_cache(void);
+uint8_t *xen_remap_cache_entry(hwaddr phys_addr, hwaddr size);
 
 #else
 
@@ -50,6 +51,11 @@ static inline void xen_invalidate_map_cache(void)
 {
 }
 
+static inline uint8_t *xen_remap_cache_entry(hwaddr phys_addr, hwaddr size)
+{
+    abort();
+}
+
 #endif
 
 #endif /* XEN_MAPCACHE_H */
-- 
2.7.4

[PATCH 3/4] xen/mapcache: introduce xen_remap_cache_entry()

[Igor Druzhinin]

This new call is trying to update a requested map cache entry
according to the changes in the physmap. The call is searching
for the entry, unmaps it, tries to translate the address and
maps again at the same place. If the mapping is dummy this call
will make it real.

This function makes use of a new xenforeignmemory_map2() call
with extended interface that was recently introduced in
libxenforeignmemory [1].

[1] https://www.mail-archive.com/xen-devel@lists.xen.org/msg113007.html

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
 configure                     |  18 ++++++++
 hw/i386/xen/xen-mapcache.c    | 105 +++++++++++++++++++++++++++++++++++++++---
 include/hw/xen/xen_common.h   |   7 +++
 include/sysemu/xen-mapcache.h |   6 +++
 4 files changed, 130 insertions(+), 6 deletions(-)

diff --git a/configure b/configure
index c571ad1..ad6156b 100755
--- a/configure
+++ b/configure
@@ -2021,6 +2021,24 @@ EOF
     # Xen unstable
     elif
         cat > $TMPC <<EOF &&
+#undef XC_WANT_COMPAT_MAP_FOREIGN_API
+#include <xenforeignmemory.h>
+int main(void) {
+  xenforeignmemory_handle *xfmem;
+
+  xfmem = xenforeignmemory_open(0, 0);
+  xenforeignmemory_map2(xfmem, 0, 0, 0, 0, 0, 0, 0);
+
+  return 0;
+}
+EOF
+        compile_prog "" "$xen_libs -lxendevicemodel $xen_stable_libs"
+      then
+      xen_stable_libs="-lxendevicemodel $xen_stable_libs"
+      xen_ctrl_version=41000
+      xen=yes
+    elif
+        cat > $TMPC <<EOF &&
 #undef XC_WANT_COMPAT_DEVICEMODEL_API
 #define __XEN_TOOLS__
 #include <xendevicemodel.h>
diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
index 05050de..5d8d990 100644
--- a/hw/i386/xen/xen-mapcache.c
+++ b/hw/i386/xen/xen-mapcache.c
@@ -149,6 +149,7 @@ void xen_map_cache_init(phys_offset_to_gaddr_t f, void *opaque)
 }
 
 static void xen_remap_bucket(MapCacheEntry *entry,
+                             void *vaddr,
                              hwaddr size,
                              hwaddr address_index,
                              bool dummy)
@@ -179,11 +180,11 @@ static void xen_remap_bucket(MapCacheEntry *entry,
     }
 
     if (!dummy) {
-        vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid,
-                                           PROT_READ|PROT_WRITE,
+        vaddr_base = xenforeignmemory_map2(xen_fmem, xen_domid, vaddr,
+                                           PROT_READ|PROT_WRITE, 0,
                                            nb_pfn, pfns, err);
         if (vaddr_base == NULL) {
-            perror("xenforeignmemory_map");
+            perror("xenforeignmemory_map2");
             exit(-1);
         }
     } else {
@@ -191,7 +192,7 @@ static void xen_remap_bucket(MapCacheEntry *entry,
          * We create dummy mappings where we are unable to create a foreign
          * mapping immediately due to certain circumstances (i.e. on resume now)
          */
-        vaddr_base = mmap(NULL, size, PROT_READ|PROT_WRITE,
+        vaddr_base = mmap(vaddr, size, PROT_READ|PROT_WRITE,
                           MAP_ANON|MAP_SHARED, -1, 0);
         if (vaddr_base == NULL) {
             perror("mmap");
@@ -278,14 +279,14 @@ tryagain:
     if (!entry) {
         entry = g_malloc0(sizeof (MapCacheEntry));
         pentry->next = entry;
-        xen_remap_bucket(entry, cache_size, address_index, dummy);
+        xen_remap_bucket(entry, NULL, cache_size, address_index, dummy);
     } else if (!entry->lock) {
         if (!entry->vaddr_base || entry->paddr_index != address_index ||
                 entry->size != cache_size ||
                 !test_bits(address_offset >> XC_PAGE_SHIFT,
                     test_bit_size >> XC_PAGE_SHIFT,
                     entry->valid_mapping)) {
-            xen_remap_bucket(entry, cache_size, address_index, dummy);
+            xen_remap_bucket(entry, NULL, cache_size, address_index, dummy);
         }
     }
 
@@ -482,3 +483,95 @@ void xen_invalidate_map_cache(void)
 
     mapcache_unlock();
 }
+
+static uint8_t *xen_remap_cache_entry_unlocked(hwaddr phys_addr, hwaddr size)
+{
+    MapCacheEntry *entry, *pentry = NULL;
+    hwaddr address_index;
+    hwaddr address_offset;
+    hwaddr cache_size = size;
+    hwaddr test_bit_size;
+    void *vaddr = NULL;
+    uint8_t lock;
+
+    address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
+    address_offset = phys_addr & (MCACHE_BUCKET_SIZE - 1);
+
+    /* test_bit_size is always a multiple of XC_PAGE_SIZE */
+    if (size) {
+        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);
+        }
+        cache_size = size + address_offset;
+        if (cache_size % MCACHE_BUCKET_SIZE) {
+            cache_size += MCACHE_BUCKET_SIZE - (cache_size % MCACHE_BUCKET_SIZE);
+        }
+    } else {
+        test_bit_size = XC_PAGE_SIZE;
+        cache_size = MCACHE_BUCKET_SIZE;
+    }
+
+    /* Search for the requested map cache entry to invalidate */
+    entry = &mapcache->entry[address_index % mapcache->nr_buckets];
+    while (entry && !(entry->paddr_index == address_index && entry->size == cache_size)) {
+        pentry = entry;
+        entry = entry->next;
+    }
+    if (!entry) {
+        DPRINTF("Trying to update an entry for %lx that is not in the mapcache!\n", phys_addr);
+        return NULL;
+    }
+
+    vaddr = entry->vaddr_base;
+    lock = entry->lock;
+    if (entry->vaddr_base) {
+        ram_block_notify_remove(entry->vaddr_base, entry->size);
+        if (munmap(entry->vaddr_base, entry->size) != 0) {
+            perror("unmap fails");
+            exit(-1);
+        }
+    }
+    entry->vaddr_base = NULL;
+    entry->lock = 0;
+
+    if (mapcache->phys_offset_to_gaddr) {
+        phys_addr = mapcache->phys_offset_to_gaddr(phys_addr, size, mapcache->opaque);
+
+        address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
+        address_offset = phys_addr & (MCACHE_BUCKET_SIZE - 1);
+    }
+
+    /* Address may have changed so we need to repeat the search */
+    entry = &mapcache->entry[address_index % mapcache->nr_buckets];
+    while (entry && entry->lock && entry->vaddr_base) {
+        pentry = entry;
+        entry = entry->next;
+    }
+    if (!entry) {
+        entry = g_malloc0(sizeof (MapCacheEntry));
+        pentry->next = entry;
+    }
+
+    entry->lock = 0;
+    xen_remap_bucket(entry, vaddr, cache_size, address_index, false);
+    if(!test_bits(address_offset >> XC_PAGE_SHIFT,
+                test_bit_size >> XC_PAGE_SHIFT,
+                entry->valid_mapping)) {
+        DPRINTF("Unable to update an entry for %lx in the mapcache!\n", phys_addr);
+        return NULL;
+    }
+
+    entry->lock = lock;
+    return entry->vaddr_base + address_offset;
+}
+
+uint8_t *xen_remap_cache_entry(hwaddr phys_addr, hwaddr size)
+{
+    uint8_t *p;
+
+    mapcache_lock();
+    p = xen_remap_cache_entry_unlocked(phys_addr, size);
+    mapcache_unlock();
+    return p;
+}
diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index e00ddd7..70a5cad 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -78,6 +78,13 @@ static inline void *xenforeignmemory_map(xc_interface *h, uint32_t dom,
 
 extern xenforeignmemory_handle *xen_fmem;
 
+#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
+
+#define xenforeignmemory_map2(h, d, a, p, f, ps, ar, e) \
+    xenforeignmemory_map(h, d, p, ps, ar, e)
+
+#endif
+
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
 
 typedef xc_interface xendevicemodel_handle;
diff --git a/include/sysemu/xen-mapcache.h b/include/sysemu/xen-mapcache.h
index 01daaad..8c140d0 100644
--- a/include/sysemu/xen-mapcache.h
+++ b/include/sysemu/xen-mapcache.h
@@ -21,6 +21,7 @@ uint8_t *xen_map_cache(hwaddr phys_addr, hwaddr size,
 ram_addr_t xen_ram_addr_from_mapcache(void *ptr);
 void xen_invalidate_map_cache_entry(uint8_t *buffer);
 void xen_invalidate_map_cache(void);
+uint8_t *xen_remap_cache_entry(hwaddr phys_addr, hwaddr size);
 
 #else
 
@@ -50,6 +51,11 @@ static inline void xen_invalidate_map_cache(void)
 {
 }
 
+static inline uint8_t *xen_remap_cache_entry(hwaddr phys_addr, hwaddr size)
+{
+    abort();
+}
+
 #endif
 
 #endif /* XEN_MAPCACHE_H */
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Qemu-devel] [PATCH 3/4] xen/mapcache: introduce xen_remap_cache_entry()

[Stefano Stabellini]

On Fri, 30 Jun 2017, Igor Druzhinin wrote:
> This new call is trying to update a requested map cache entry
> according to the changes in the physmap. The call is searching
> for the entry, unmaps it, tries to translate the address and
> maps again at the same place. If the mapping is dummy this call
> will make it real.
> 
> This function makes use of a new xenforeignmemory_map2() call
> with extended interface that was recently introduced in
> libxenforeignmemory [1].
> 
> [1] https://www.mail-archive.com/xen-devel@lists.xen.org/msg113007.html
> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> ---
>  configure                     |  18 ++++++++
>  hw/i386/xen/xen-mapcache.c    | 105 +++++++++++++++++++++++++++++++++++++++---
>  include/hw/xen/xen_common.h   |   7 +++
>  include/sysemu/xen-mapcache.h |   6 +++
>  4 files changed, 130 insertions(+), 6 deletions(-)
> 
> diff --git a/configure b/configure
> index c571ad1..ad6156b 100755
> --- a/configure
> +++ b/configure
> @@ -2021,6 +2021,24 @@ EOF
>      # Xen unstable
>      elif
>          cat > $TMPC <<EOF &&
> +#undef XC_WANT_COMPAT_MAP_FOREIGN_API
> +#include <xenforeignmemory.h>
> +int main(void) {
> +  xenforeignmemory_handle *xfmem;
> +
> +  xfmem = xenforeignmemory_open(0, 0);
> +  xenforeignmemory_map2(xfmem, 0, 0, 0, 0, 0, 0, 0);
> +
> +  return 0;
> +}
> +EOF
> +        compile_prog "" "$xen_libs -lxendevicemodel $xen_stable_libs"
> +      then
> +      xen_stable_libs="-lxendevicemodel $xen_stable_libs"
> +      xen_ctrl_version=41000
> +      xen=yes
> +    elif
> +        cat > $TMPC <<EOF &&
>  #undef XC_WANT_COMPAT_DEVICEMODEL_API
>  #define __XEN_TOOLS__
>  #include <xendevicemodel.h>
> diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
> index 05050de..5d8d990 100644
> --- a/hw/i386/xen/xen-mapcache.c
> +++ b/hw/i386/xen/xen-mapcache.c
> @@ -149,6 +149,7 @@ void xen_map_cache_init(phys_offset_to_gaddr_t f, void *opaque)
>  }
>  
>  static void xen_remap_bucket(MapCacheEntry *entry,
> +                             void *vaddr,
>                               hwaddr size,
>                               hwaddr address_index,
>                               bool dummy)
> @@ -179,11 +180,11 @@ static void xen_remap_bucket(MapCacheEntry *entry,
>      }
>  
>      if (!dummy) {
> -        vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid,
> -                                           PROT_READ|PROT_WRITE,
> +        vaddr_base = xenforeignmemory_map2(xen_fmem, xen_domid, vaddr,
> +                                           PROT_READ|PROT_WRITE, 0,
>                                             nb_pfn, pfns, err);
>          if (vaddr_base == NULL) {
> -            perror("xenforeignmemory_map");
> +            perror("xenforeignmemory_map2");
>              exit(-1);
>          }
>      } else {
> @@ -191,7 +192,7 @@ static void xen_remap_bucket(MapCacheEntry *entry,
>           * We create dummy mappings where we are unable to create a foreign
>           * mapping immediately due to certain circumstances (i.e. on resume now)
>           */
> -        vaddr_base = mmap(NULL, size, PROT_READ|PROT_WRITE,
> +        vaddr_base = mmap(vaddr, size, PROT_READ|PROT_WRITE,
>                            MAP_ANON|MAP_SHARED, -1, 0);
>          if (vaddr_base == NULL) {
>              perror("mmap");
> @@ -278,14 +279,14 @@ tryagain:
>      if (!entry) {
>          entry = g_malloc0(sizeof (MapCacheEntry));
>          pentry->next = entry;
> -        xen_remap_bucket(entry, cache_size, address_index, dummy);
> +        xen_remap_bucket(entry, NULL, cache_size, address_index, dummy);
>      } else if (!entry->lock) {
>          if (!entry->vaddr_base || entry->paddr_index != address_index ||
>                  entry->size != cache_size ||
>                  !test_bits(address_offset >> XC_PAGE_SHIFT,
>                      test_bit_size >> XC_PAGE_SHIFT,
>                      entry->valid_mapping)) {
> -            xen_remap_bucket(entry, cache_size, address_index, dummy);
> +            xen_remap_bucket(entry, NULL, cache_size, address_index, dummy);
>          }
>      }
>  
> @@ -482,3 +483,95 @@ void xen_invalidate_map_cache(void)
>  
>      mapcache_unlock();
>  }
> +
> +static uint8_t *xen_remap_cache_entry_unlocked(hwaddr phys_addr, hwaddr size)

I think it's best if we use a more descriptive name, such as
xen_replace_dummy_entry to avoid confusion.


> +{
> +    MapCacheEntry *entry, *pentry = NULL;
> +    hwaddr address_index;
> +    hwaddr address_offset;
> +    hwaddr cache_size = size;
> +    hwaddr test_bit_size;
> +    void *vaddr = NULL;
> +    uint8_t lock;
> +
> +    address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
> +    address_offset = phys_addr & (MCACHE_BUCKET_SIZE - 1);
> +
> +    /* test_bit_size is always a multiple of XC_PAGE_SIZE */
> +    if (size) {

There is no need to make xen_remap_cache_entry_unlocked generic: it's
only used with explicitly sized mappings, right? We could assert(!size).


> +        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);
> +        }
> +        cache_size = size + address_offset;
> +        if (cache_size % MCACHE_BUCKET_SIZE) {
> +            cache_size += MCACHE_BUCKET_SIZE - (cache_size % MCACHE_BUCKET_SIZE);
> +        }
> +    } else {
> +        test_bit_size = XC_PAGE_SIZE;
> +        cache_size = MCACHE_BUCKET_SIZE;
> +    }
> +
> +    /* Search for the requested map cache entry to invalidate */
> +    entry = &mapcache->entry[address_index % mapcache->nr_buckets];
> +    while (entry && !(entry->paddr_index == address_index && entry->size == cache_size)) {
> +        pentry = entry;
> +        entry = entry->next;
> +    }
> +    if (!entry) {
> +        DPRINTF("Trying to update an entry for %lx that is not in the mapcache!\n", phys_addr);
> +        return NULL;
> +    }
> +
> +    vaddr = entry->vaddr_base;
> +    lock = entry->lock;
> +    if (entry->vaddr_base) {
> +        ram_block_notify_remove(entry->vaddr_base, entry->size);
> +        if (munmap(entry->vaddr_base, entry->size) != 0) {
> +            perror("unmap fails");
> +            exit(-1);
> +        }
> +    }

Why are we calling ram_block_notify_remove? Isn't the ram_block about to
be remapped with the correct underlying pages?


> +    entry->vaddr_base = NULL;
> +    entry->lock = 0;

Why can't we just keep using this entry as is?


> +    if (mapcache->phys_offset_to_gaddr) {
> +        phys_addr = mapcache->phys_offset_to_gaddr(phys_addr, size, mapcache->opaque);
> +
> +        address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
> +        address_offset = phys_addr & (MCACHE_BUCKET_SIZE - 1);
> +    }

Instead of having this check to find the new address, why don't we just
pass it to xen_remap_cache_entry_unlocked as an argument?


> +    /* Address may have changed so we need to repeat the search */
> +    entry = &mapcache->entry[address_index % mapcache->nr_buckets];
> +    while (entry && entry->lock && entry->vaddr_base) {
> +        pentry = entry;
> +        entry = entry->next;
> +    }
> +    if (!entry) {
> +        entry = g_malloc0(sizeof (MapCacheEntry));
> +        pentry->next = entry;
> +    }

Is it really possible to already have a mapcache entry for the new
phys_addr, which has never been used before? It doesn't look like it.
Also, why are we creating a new entry instead of simply reusing the
previous one?


> +    entry->lock = 0;
> +    xen_remap_bucket(entry, vaddr, cache_size, address_index, false);
> +    if(!test_bits(address_offset >> XC_PAGE_SHIFT,
> +                test_bit_size >> XC_PAGE_SHIFT,
> +                entry->valid_mapping)) {
> +        DPRINTF("Unable to update an entry for %lx in the mapcache!\n", phys_addr);
> +        return NULL;
> +    }
> +
> +    entry->lock = lock;
> +    return entry->vaddr_base + address_offset;
> +}
> +
> +uint8_t *xen_remap_cache_entry(hwaddr phys_addr, hwaddr size)
> +{
> +    uint8_t *p;
> +
> +    mapcache_lock();
> +    p = xen_remap_cache_entry_unlocked(phys_addr, size);
> +    mapcache_unlock();
> +    return p;
> +}
> diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
> index e00ddd7..70a5cad 100644
> --- a/include/hw/xen/xen_common.h
> +++ b/include/hw/xen/xen_common.h
> @@ -78,6 +78,13 @@ static inline void *xenforeignmemory_map(xc_interface *h, uint32_t dom,
>  
>  extern xenforeignmemory_handle *xen_fmem;
>  
> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
> +
> +#define xenforeignmemory_map2(h, d, a, p, f, ps, ar, e) \
> +    xenforeignmemory_map(h, d, p, ps, ar, e)
> +
> +#endif
> +
>  #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
>  
>  typedef xc_interface xendevicemodel_handle;
> diff --git a/include/sysemu/xen-mapcache.h b/include/sysemu/xen-mapcache.h
> index 01daaad..8c140d0 100644
> --- a/include/sysemu/xen-mapcache.h
> +++ b/include/sysemu/xen-mapcache.h
> @@ -21,6 +21,7 @@ uint8_t *xen_map_cache(hwaddr phys_addr, hwaddr size,
>  ram_addr_t xen_ram_addr_from_mapcache(void *ptr);
>  void xen_invalidate_map_cache_entry(uint8_t *buffer);
>  void xen_invalidate_map_cache(void);
> +uint8_t *xen_remap_cache_entry(hwaddr phys_addr, hwaddr size);
>  
>  #else
>  
> @@ -50,6 +51,11 @@ static inline void xen_invalidate_map_cache(void)
>  {
>  }
>  
> +static inline uint8_t *xen_remap_cache_entry(hwaddr phys_addr, hwaddr size)
> +{
> +    abort();
> +}
> +
>  #endif
>  
>  #endif /* XEN_MAPCACHE_H */
> -- 
> 2.7.4
> 

Re: [PATCH 3/4] xen/mapcache: introduce xen_remap_cache_entry()

[Stefano Stabellini]

On Fri, 30 Jun 2017, Igor Druzhinin wrote:
> This new call is trying to update a requested map cache entry
> according to the changes in the physmap. The call is searching
> for the entry, unmaps it, tries to translate the address and
> maps again at the same place. If the mapping is dummy this call
> will make it real.
> 
> This function makes use of a new xenforeignmemory_map2() call
> with extended interface that was recently introduced in
> libxenforeignmemory [1].
> 
> [1] https://www.mail-archive.com/xen-devel@lists.xen.org/msg113007.html
> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> ---
>  configure                     |  18 ++++++++
>  hw/i386/xen/xen-mapcache.c    | 105 +++++++++++++++++++++++++++++++++++++++---
>  include/hw/xen/xen_common.h   |   7 +++
>  include/sysemu/xen-mapcache.h |   6 +++
>  4 files changed, 130 insertions(+), 6 deletions(-)
> 
> diff --git a/configure b/configure
> index c571ad1..ad6156b 100755
> --- a/configure
> +++ b/configure
> @@ -2021,6 +2021,24 @@ EOF
>      # Xen unstable
>      elif
>          cat > $TMPC <<EOF &&
> +#undef XC_WANT_COMPAT_MAP_FOREIGN_API
> +#include <xenforeignmemory.h>
> +int main(void) {
> +  xenforeignmemory_handle *xfmem;
> +
> +  xfmem = xenforeignmemory_open(0, 0);
> +  xenforeignmemory_map2(xfmem, 0, 0, 0, 0, 0, 0, 0);
> +
> +  return 0;
> +}
> +EOF
> +        compile_prog "" "$xen_libs -lxendevicemodel $xen_stable_libs"
> +      then
> +      xen_stable_libs="-lxendevicemodel $xen_stable_libs"
> +      xen_ctrl_version=41000
> +      xen=yes
> +    elif
> +        cat > $TMPC <<EOF &&
>  #undef XC_WANT_COMPAT_DEVICEMODEL_API
>  #define __XEN_TOOLS__
>  #include <xendevicemodel.h>
> diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
> index 05050de..5d8d990 100644
> --- a/hw/i386/xen/xen-mapcache.c
> +++ b/hw/i386/xen/xen-mapcache.c
> @@ -149,6 +149,7 @@ void xen_map_cache_init(phys_offset_to_gaddr_t f, void *opaque)
>  }
>  
>  static void xen_remap_bucket(MapCacheEntry *entry,
> +                             void *vaddr,
>                               hwaddr size,
>                               hwaddr address_index,
>                               bool dummy)
> @@ -179,11 +180,11 @@ static void xen_remap_bucket(MapCacheEntry *entry,
>      }
>  
>      if (!dummy) {
> -        vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid,
> -                                           PROT_READ|PROT_WRITE,
> +        vaddr_base = xenforeignmemory_map2(xen_fmem, xen_domid, vaddr,
> +                                           PROT_READ|PROT_WRITE, 0,
>                                             nb_pfn, pfns, err);
>          if (vaddr_base == NULL) {
> -            perror("xenforeignmemory_map");
> +            perror("xenforeignmemory_map2");
>              exit(-1);
>          }
>      } else {
> @@ -191,7 +192,7 @@ static void xen_remap_bucket(MapCacheEntry *entry,
>           * We create dummy mappings where we are unable to create a foreign
>           * mapping immediately due to certain circumstances (i.e. on resume now)
>           */
> -        vaddr_base = mmap(NULL, size, PROT_READ|PROT_WRITE,
> +        vaddr_base = mmap(vaddr, size, PROT_READ|PROT_WRITE,
>                            MAP_ANON|MAP_SHARED, -1, 0);
>          if (vaddr_base == NULL) {
>              perror("mmap");
> @@ -278,14 +279,14 @@ tryagain:
>      if (!entry) {
>          entry = g_malloc0(sizeof (MapCacheEntry));
>          pentry->next = entry;
> -        xen_remap_bucket(entry, cache_size, address_index, dummy);
> +        xen_remap_bucket(entry, NULL, cache_size, address_index, dummy);
>      } else if (!entry->lock) {
>          if (!entry->vaddr_base || entry->paddr_index != address_index ||
>                  entry->size != cache_size ||
>                  !test_bits(address_offset >> XC_PAGE_SHIFT,
>                      test_bit_size >> XC_PAGE_SHIFT,
>                      entry->valid_mapping)) {
> -            xen_remap_bucket(entry, cache_size, address_index, dummy);
> +            xen_remap_bucket(entry, NULL, cache_size, address_index, dummy);
>          }
>      }
>  
> @@ -482,3 +483,95 @@ void xen_invalidate_map_cache(void)
>  
>      mapcache_unlock();
>  }
> +
> +static uint8_t *xen_remap_cache_entry_unlocked(hwaddr phys_addr, hwaddr size)

I think it's best if we use a more descriptive name, such as
xen_replace_dummy_entry to avoid confusion.


> +{
> +    MapCacheEntry *entry, *pentry = NULL;
> +    hwaddr address_index;
> +    hwaddr address_offset;
> +    hwaddr cache_size = size;
> +    hwaddr test_bit_size;
> +    void *vaddr = NULL;
> +    uint8_t lock;
> +
> +    address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
> +    address_offset = phys_addr & (MCACHE_BUCKET_SIZE - 1);
> +
> +    /* test_bit_size is always a multiple of XC_PAGE_SIZE */
> +    if (size) {

There is no need to make xen_remap_cache_entry_unlocked generic: it's
only used with explicitly sized mappings, right? We could assert(!size).


> +        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);
> +        }
> +        cache_size = size + address_offset;
> +        if (cache_size % MCACHE_BUCKET_SIZE) {
> +            cache_size += MCACHE_BUCKET_SIZE - (cache_size % MCACHE_BUCKET_SIZE);
> +        }
> +    } else {
> +        test_bit_size = XC_PAGE_SIZE;
> +        cache_size = MCACHE_BUCKET_SIZE;
> +    }
> +
> +    /* Search for the requested map cache entry to invalidate */
> +    entry = &mapcache->entry[address_index % mapcache->nr_buckets];
> +    while (entry && !(entry->paddr_index == address_index && entry->size == cache_size)) {
> +        pentry = entry;
> +        entry = entry->next;
> +    }
> +    if (!entry) {
> +        DPRINTF("Trying to update an entry for %lx that is not in the mapcache!\n", phys_addr);
> +        return NULL;
> +    }
> +
> +    vaddr = entry->vaddr_base;
> +    lock = entry->lock;
> +    if (entry->vaddr_base) {
> +        ram_block_notify_remove(entry->vaddr_base, entry->size);
> +        if (munmap(entry->vaddr_base, entry->size) != 0) {
> +            perror("unmap fails");
> +            exit(-1);
> +        }
> +    }

Why are we calling ram_block_notify_remove? Isn't the ram_block about to
be remapped with the correct underlying pages?


> +    entry->vaddr_base = NULL;
> +    entry->lock = 0;

Why can't we just keep using this entry as is?


> +    if (mapcache->phys_offset_to_gaddr) {
> +        phys_addr = mapcache->phys_offset_to_gaddr(phys_addr, size, mapcache->opaque);
> +
> +        address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
> +        address_offset = phys_addr & (MCACHE_BUCKET_SIZE - 1);
> +    }

Instead of having this check to find the new address, why don't we just
pass it to xen_remap_cache_entry_unlocked as an argument?


> +    /* Address may have changed so we need to repeat the search */
> +    entry = &mapcache->entry[address_index % mapcache->nr_buckets];
> +    while (entry && entry->lock && entry->vaddr_base) {
> +        pentry = entry;
> +        entry = entry->next;
> +    }
> +    if (!entry) {
> +        entry = g_malloc0(sizeof (MapCacheEntry));
> +        pentry->next = entry;
> +    }

Is it really possible to already have a mapcache entry for the new
phys_addr, which has never been used before? It doesn't look like it.
Also, why are we creating a new entry instead of simply reusing the
previous one?


> +    entry->lock = 0;
> +    xen_remap_bucket(entry, vaddr, cache_size, address_index, false);
> +    if(!test_bits(address_offset >> XC_PAGE_SHIFT,
> +                test_bit_size >> XC_PAGE_SHIFT,
> +                entry->valid_mapping)) {
> +        DPRINTF("Unable to update an entry for %lx in the mapcache!\n", phys_addr);
> +        return NULL;
> +    }
> +
> +    entry->lock = lock;
> +    return entry->vaddr_base + address_offset;
> +}
> +
> +uint8_t *xen_remap_cache_entry(hwaddr phys_addr, hwaddr size)
> +{
> +    uint8_t *p;
> +
> +    mapcache_lock();
> +    p = xen_remap_cache_entry_unlocked(phys_addr, size);
> +    mapcache_unlock();
> +    return p;
> +}
> diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
> index e00ddd7..70a5cad 100644
> --- a/include/hw/xen/xen_common.h
> +++ b/include/hw/xen/xen_common.h
> @@ -78,6 +78,13 @@ static inline void *xenforeignmemory_map(xc_interface *h, uint32_t dom,
>  
>  extern xenforeignmemory_handle *xen_fmem;
>  
> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
> +
> +#define xenforeignmemory_map2(h, d, a, p, f, ps, ar, e) \
> +    xenforeignmemory_map(h, d, p, ps, ar, e)
> +
> +#endif
> +
>  #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
>  
>  typedef xc_interface xendevicemodel_handle;
> diff --git a/include/sysemu/xen-mapcache.h b/include/sysemu/xen-mapcache.h
> index 01daaad..8c140d0 100644
> --- a/include/sysemu/xen-mapcache.h
> +++ b/include/sysemu/xen-mapcache.h
> @@ -21,6 +21,7 @@ uint8_t *xen_map_cache(hwaddr phys_addr, hwaddr size,
>  ram_addr_t xen_ram_addr_from_mapcache(void *ptr);
>  void xen_invalidate_map_cache_entry(uint8_t *buffer);
>  void xen_invalidate_map_cache(void);
> +uint8_t *xen_remap_cache_entry(hwaddr phys_addr, hwaddr size);
>  
>  #else
>  
> @@ -50,6 +51,11 @@ static inline void xen_invalidate_map_cache(void)
>  {
>  }
>  
> +static inline uint8_t *xen_remap_cache_entry(hwaddr phys_addr, hwaddr size)
> +{
> +    abort();
> +}
> +
>  #endif
>  
>  #endif /* XEN_MAPCACHE_H */
> -- 
> 2.7.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Qemu-devel] [PATCH 3/4] xen/mapcache: introduce xen_remap_cache_entry()

[Igor Druzhinin]

On 01/07/17 01:08, Stefano Stabellini wrote:
> On Fri, 30 Jun 2017, Igor Druzhinin wrote:
>> This new call is trying to update a requested map cache entry
>> according to the changes in the physmap. The call is searching
>> for the entry, unmaps it, tries to translate the address and
>> maps again at the same place. If the mapping is dummy this call
>> will make it real.
>>
>> This function makes use of a new xenforeignmemory_map2() call
>> with extended interface that was recently introduced in
>> libxenforeignmemory [1].
>>
>> [1] https://www.mail-archive.com/xen-devel@lists.xen.org/msg113007.html
>>
>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>> ---
>>  configure                     |  18 ++++++++
>>  hw/i386/xen/xen-mapcache.c    | 105 +++++++++++++++++++++++++++++++++++++++---
>>  include/hw/xen/xen_common.h   |   7 +++
>>  include/sysemu/xen-mapcache.h |   6 +++
>>  4 files changed, 130 insertions(+), 6 deletions(-)
>>
>> diff --git a/configure b/configure
>> index c571ad1..ad6156b 100755
>> --- a/configure
>> +++ b/configure
>> @@ -2021,6 +2021,24 @@ EOF
>>      # Xen unstable
>>      elif
>>          cat > $TMPC <<EOF &&
>> +#undef XC_WANT_COMPAT_MAP_FOREIGN_API
>> +#include <xenforeignmemory.h>
>> +int main(void) {
>> +  xenforeignmemory_handle *xfmem;
>> +
>> +  xfmem = xenforeignmemory_open(0, 0);
>> +  xenforeignmemory_map2(xfmem, 0, 0, 0, 0, 0, 0, 0);
>> +
>> +  return 0;
>> +}
>> +EOF
>> +        compile_prog "" "$xen_libs -lxendevicemodel $xen_stable_libs"
>> +      then
>> +      xen_stable_libs="-lxendevicemodel $xen_stable_libs"
>> +      xen_ctrl_version=41000
>> +      xen=yes
>> +    elif
>> +        cat > $TMPC <<EOF &&
>>  #undef XC_WANT_COMPAT_DEVICEMODEL_API
>>  #define __XEN_TOOLS__
>>  #include <xendevicemodel.h>
>> diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
>> index 05050de..5d8d990 100644
>> --- a/hw/i386/xen/xen-mapcache.c
>> +++ b/hw/i386/xen/xen-mapcache.c
>> @@ -149,6 +149,7 @@ void xen_map_cache_init(phys_offset_to_gaddr_t f, void *opaque)
>>  }
>>  
>>  static void xen_remap_bucket(MapCacheEntry *entry,
>> +                             void *vaddr,
>>                               hwaddr size,
>>                               hwaddr address_index,
>>                               bool dummy)
>> @@ -179,11 +180,11 @@ static void xen_remap_bucket(MapCacheEntry *entry,
>>      }
>>  
>>      if (!dummy) {
>> -        vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid,
>> -                                           PROT_READ|PROT_WRITE,
>> +        vaddr_base = xenforeignmemory_map2(xen_fmem, xen_domid, vaddr,
>> +                                           PROT_READ|PROT_WRITE, 0,
>>                                             nb_pfn, pfns, err);
>>          if (vaddr_base == NULL) {
>> -            perror("xenforeignmemory_map");
>> +            perror("xenforeignmemory_map2");
>>              exit(-1);
>>          }
>>      } else {
>> @@ -191,7 +192,7 @@ static void xen_remap_bucket(MapCacheEntry *entry,
>>           * We create dummy mappings where we are unable to create a foreign
>>           * mapping immediately due to certain circumstances (i.e. on resume now)
>>           */
>> -        vaddr_base = mmap(NULL, size, PROT_READ|PROT_WRITE,
>> +        vaddr_base = mmap(vaddr, size, PROT_READ|PROT_WRITE,
>>                            MAP_ANON|MAP_SHARED, -1, 0);
>>          if (vaddr_base == NULL) {
>>              perror("mmap");
>> @@ -278,14 +279,14 @@ tryagain:
>>      if (!entry) {
>>          entry = g_malloc0(sizeof (MapCacheEntry));
>>          pentry->next = entry;
>> -        xen_remap_bucket(entry, cache_size, address_index, dummy);
>> +        xen_remap_bucket(entry, NULL, cache_size, address_index, dummy);
>>      } else if (!entry->lock) {
>>          if (!entry->vaddr_base || entry->paddr_index != address_index ||
>>                  entry->size != cache_size ||
>>                  !test_bits(address_offset >> XC_PAGE_SHIFT,
>>                      test_bit_size >> XC_PAGE_SHIFT,
>>                      entry->valid_mapping)) {
>> -            xen_remap_bucket(entry, cache_size, address_index, dummy);
>> +            xen_remap_bucket(entry, NULL, cache_size, address_index, dummy);
>>          }
>>      }
>>  
>> @@ -482,3 +483,95 @@ void xen_invalidate_map_cache(void)
>>  
>>      mapcache_unlock();
>>  }
>> +
>> +static uint8_t *xen_remap_cache_entry_unlocked(hwaddr phys_addr, hwaddr size)
> 
> I think it's best if we use a more descriptive name, such as
> xen_replace_dummy_entry to avoid confusion.
> 
> 
>> +{
>> +    MapCacheEntry *entry, *pentry = NULL;
>> +    hwaddr address_index;
>> +    hwaddr address_offset;
>> +    hwaddr cache_size = size;
>> +    hwaddr test_bit_size;
>> +    void *vaddr = NULL;
>> +    uint8_t lock;
>> +
>> +    address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
>> +    address_offset = phys_addr & (MCACHE_BUCKET_SIZE - 1);
>> +
>> +    /* test_bit_size is always a multiple of XC_PAGE_SIZE */
>> +    if (size) {
> 
> There is no need to make xen_remap_cache_entry_unlocked generic: it's
> only used with explicitly sized mappings, right? We could assert(!size).
> 

Sure.

> 
>> +        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);
>> +        }
>> +        cache_size = size + address_offset;
>> +        if (cache_size % MCACHE_BUCKET_SIZE) {
>> +            cache_size += MCACHE_BUCKET_SIZE - (cache_size % MCACHE_BUCKET_SIZE);
>> +        }
>> +    } else {
>> +        test_bit_size = XC_PAGE_SIZE;
>> +        cache_size = MCACHE_BUCKET_SIZE;
>> +    }
>> +
>> +    /* Search for the requested map cache entry to invalidate */
>> +    entry = &mapcache->entry[address_index % mapcache->nr_buckets];
>> +    while (entry && !(entry->paddr_index == address_index && entry->size == cache_size)) {
>> +        pentry = entry;
>> +        entry = entry->next;
>> +    }
>> +    if (!entry) {
>> +        DPRINTF("Trying to update an entry for %lx that is not in the mapcache!\n", phys_addr);
>> +        return NULL;
>> +    }
>> +
>> +    vaddr = entry->vaddr_base;
>> +    lock = entry->lock;
>> +    if (entry->vaddr_base) {
>> +        ram_block_notify_remove(entry->vaddr_base, entry->size);
>> +        if (munmap(entry->vaddr_base, entry->size) != 0) {
>> +            perror("unmap fails");
>> +            exit(-1);
>> +        }
>> +    }
> 
> Why are we calling ram_block_notify_remove? Isn't the ram_block about to
> be remapped with the correct underlying pages?
> 

It's here for symmetry. xen_remap_bucket is going to call
ram_block_notify_add and I didn't want to gate it there. But I probably
should.

> 
>> +    entry->vaddr_base = NULL;
>> +    entry->lock = 0;
> 
> Why can't we just keep using this entry as is?
> 
> 
>> +    if (mapcache->phys_offset_to_gaddr) {
>> +        phys_addr = mapcache->phys_offset_to_gaddr(phys_addr, size, mapcache->opaque);
>> +
>> +        address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
>> +        address_offset = phys_addr & (MCACHE_BUCKET_SIZE - 1);
>> +    }
> 
> Instead of having this check to find the new address, why don't we just
> pass it to xen_remap_cache_entry_unlocked as an argument?
> 

Agree.

> 
>> +    /* Address may have changed so we need to repeat the search */
>> +    entry = &mapcache->entry[address_index % mapcache->nr_buckets];
>> +    while (entry && entry->lock && entry->vaddr_base) {
>> +        pentry = entry;
>> +        entry = entry->next;
>> +    }
>> +    if (!entry) {
>> +        entry = g_malloc0(sizeof (MapCacheEntry));
>> +        pentry->next = entry;
>> +    }
> 
> Is it really possible to already have a mapcache entry for the new
> phys_addr, which has never been used before? It doesn't look like it.
> Also, why are we creating a new entry instead of simply reusing the
> previous one?
> 

I wasn't completely sure it's safe to reuse the previous entry since
it's indexed by its previous physical address. But it looks like that
QEMU is going to use only its old address for referencing. That'll make
the code much simpler.

> 
>> +    entry->lock = 0;
>> +    xen_remap_bucket(entry, vaddr, cache_size, address_index, false);
>> +    if(!test_bits(address_offset >> XC_PAGE_SHIFT,
>> +                test_bit_size >> XC_PAGE_SHIFT,
>> +                entry->valid_mapping)) {
>> +        DPRINTF("Unable to update an entry for %lx in the mapcache!\n", phys_addr);
>> +        return NULL;
>> +    }
>> +
>> +    entry->lock = lock;
>> +    return entry->vaddr_base + address_offset;
>> +}
>> +
>> +uint8_t *xen_remap_cache_entry(hwaddr phys_addr, hwaddr size)
>> +{
>> +    uint8_t *p;
>> +
>> +    mapcache_lock();
>> +    p = xen_remap_cache_entry_unlocked(phys_addr, size);
>> +    mapcache_unlock();
>> +    return p;
>> +}
>> diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
>> index e00ddd7..70a5cad 100644
>> --- a/include/hw/xen/xen_common.h
>> +++ b/include/hw/xen/xen_common.h
>> @@ -78,6 +78,13 @@ static inline void *xenforeignmemory_map(xc_interface *h, uint32_t dom,
>>  
>>  extern xenforeignmemory_handle *xen_fmem;
>>  
>> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
>> +
>> +#define xenforeignmemory_map2(h, d, a, p, f, ps, ar, e) \
>> +    xenforeignmemory_map(h, d, p, ps, ar, e)
>> +
>> +#endif
>> +
>>  #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
>>  
>>  typedef xc_interface xendevicemodel_handle;
>> diff --git a/include/sysemu/xen-mapcache.h b/include/sysemu/xen-mapcache.h
>> index 01daaad..8c140d0 100644
>> --- a/include/sysemu/xen-mapcache.h
>> +++ b/include/sysemu/xen-mapcache.h
>> @@ -21,6 +21,7 @@ uint8_t *xen_map_cache(hwaddr phys_addr, hwaddr size,
>>  ram_addr_t xen_ram_addr_from_mapcache(void *ptr);
>>  void xen_invalidate_map_cache_entry(uint8_t *buffer);
>>  void xen_invalidate_map_cache(void);
>> +uint8_t *xen_remap_cache_entry(hwaddr phys_addr, hwaddr size);
>>  
>>  #else
>>  
>> @@ -50,6 +51,11 @@ static inline void xen_invalidate_map_cache(void)
>>  {
>>  }
>>  
>> +static inline uint8_t *xen_remap_cache_entry(hwaddr phys_addr, hwaddr size)
>> +{
>> +    abort();
>> +}
>> +
>>  #endif
>>  
>>  #endif /* XEN_MAPCACHE_H */
>> -- 
>> 2.7.4
>>

Re: [PATCH 3/4] xen/mapcache: introduce xen_remap_cache_entry()

[Igor Druzhinin]

On 01/07/17 01:08, Stefano Stabellini wrote:
> On Fri, 30 Jun 2017, Igor Druzhinin wrote:
>> This new call is trying to update a requested map cache entry
>> according to the changes in the physmap. The call is searching
>> for the entry, unmaps it, tries to translate the address and
>> maps again at the same place. If the mapping is dummy this call
>> will make it real.
>>
>> This function makes use of a new xenforeignmemory_map2() call
>> with extended interface that was recently introduced in
>> libxenforeignmemory [1].
>>
>> [1] https://www.mail-archive.com/xen-devel@lists.xen.org/msg113007.html
>>
>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>> ---
>>  configure                     |  18 ++++++++
>>  hw/i386/xen/xen-mapcache.c    | 105 +++++++++++++++++++++++++++++++++++++++---
>>  include/hw/xen/xen_common.h   |   7 +++
>>  include/sysemu/xen-mapcache.h |   6 +++
>>  4 files changed, 130 insertions(+), 6 deletions(-)
>>
>> diff --git a/configure b/configure
>> index c571ad1..ad6156b 100755
>> --- a/configure
>> +++ b/configure
>> @@ -2021,6 +2021,24 @@ EOF
>>      # Xen unstable
>>      elif
>>          cat > $TMPC <<EOF &&
>> +#undef XC_WANT_COMPAT_MAP_FOREIGN_API
>> +#include <xenforeignmemory.h>
>> +int main(void) {
>> +  xenforeignmemory_handle *xfmem;
>> +
>> +  xfmem = xenforeignmemory_open(0, 0);
>> +  xenforeignmemory_map2(xfmem, 0, 0, 0, 0, 0, 0, 0);
>> +
>> +  return 0;
>> +}
>> +EOF
>> +        compile_prog "" "$xen_libs -lxendevicemodel $xen_stable_libs"
>> +      then
>> +      xen_stable_libs="-lxendevicemodel $xen_stable_libs"
>> +      xen_ctrl_version=41000
>> +      xen=yes
>> +    elif
>> +        cat > $TMPC <<EOF &&
>>  #undef XC_WANT_COMPAT_DEVICEMODEL_API
>>  #define __XEN_TOOLS__
>>  #include <xendevicemodel.h>
>> diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
>> index 05050de..5d8d990 100644
>> --- a/hw/i386/xen/xen-mapcache.c
>> +++ b/hw/i386/xen/xen-mapcache.c
>> @@ -149,6 +149,7 @@ void xen_map_cache_init(phys_offset_to_gaddr_t f, void *opaque)
>>  }
>>  
>>  static void xen_remap_bucket(MapCacheEntry *entry,
>> +                             void *vaddr,
>>                               hwaddr size,
>>                               hwaddr address_index,
>>                               bool dummy)
>> @@ -179,11 +180,11 @@ static void xen_remap_bucket(MapCacheEntry *entry,
>>      }
>>  
>>      if (!dummy) {
>> -        vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid,
>> -                                           PROT_READ|PROT_WRITE,
>> +        vaddr_base = xenforeignmemory_map2(xen_fmem, xen_domid, vaddr,
>> +                                           PROT_READ|PROT_WRITE, 0,
>>                                             nb_pfn, pfns, err);
>>          if (vaddr_base == NULL) {
>> -            perror("xenforeignmemory_map");
>> +            perror("xenforeignmemory_map2");
>>              exit(-1);
>>          }
>>      } else {
>> @@ -191,7 +192,7 @@ static void xen_remap_bucket(MapCacheEntry *entry,
>>           * We create dummy mappings where we are unable to create a foreign
>>           * mapping immediately due to certain circumstances (i.e. on resume now)
>>           */
>> -        vaddr_base = mmap(NULL, size, PROT_READ|PROT_WRITE,
>> +        vaddr_base = mmap(vaddr, size, PROT_READ|PROT_WRITE,
>>                            MAP_ANON|MAP_SHARED, -1, 0);
>>          if (vaddr_base == NULL) {
>>              perror("mmap");
>> @@ -278,14 +279,14 @@ tryagain:
>>      if (!entry) {
>>          entry = g_malloc0(sizeof (MapCacheEntry));
>>          pentry->next = entry;
>> -        xen_remap_bucket(entry, cache_size, address_index, dummy);
>> +        xen_remap_bucket(entry, NULL, cache_size, address_index, dummy);
>>      } else if (!entry->lock) {
>>          if (!entry->vaddr_base || entry->paddr_index != address_index ||
>>                  entry->size != cache_size ||
>>                  !test_bits(address_offset >> XC_PAGE_SHIFT,
>>                      test_bit_size >> XC_PAGE_SHIFT,
>>                      entry->valid_mapping)) {
>> -            xen_remap_bucket(entry, cache_size, address_index, dummy);
>> +            xen_remap_bucket(entry, NULL, cache_size, address_index, dummy);
>>          }
>>      }
>>  
>> @@ -482,3 +483,95 @@ void xen_invalidate_map_cache(void)
>>  
>>      mapcache_unlock();
>>  }
>> +
>> +static uint8_t *xen_remap_cache_entry_unlocked(hwaddr phys_addr, hwaddr size)
> 
> I think it's best if we use a more descriptive name, such as
> xen_replace_dummy_entry to avoid confusion.
> 
> 
>> +{
>> +    MapCacheEntry *entry, *pentry = NULL;
>> +    hwaddr address_index;
>> +    hwaddr address_offset;
>> +    hwaddr cache_size = size;
>> +    hwaddr test_bit_size;
>> +    void *vaddr = NULL;
>> +    uint8_t lock;
>> +
>> +    address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
>> +    address_offset = phys_addr & (MCACHE_BUCKET_SIZE - 1);
>> +
>> +    /* test_bit_size is always a multiple of XC_PAGE_SIZE */
>> +    if (size) {
> 
> There is no need to make xen_remap_cache_entry_unlocked generic: it's
> only used with explicitly sized mappings, right? We could assert(!size).
> 

Sure.

> 
>> +        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);
>> +        }
>> +        cache_size = size + address_offset;
>> +        if (cache_size % MCACHE_BUCKET_SIZE) {
>> +            cache_size += MCACHE_BUCKET_SIZE - (cache_size % MCACHE_BUCKET_SIZE);
>> +        }
>> +    } else {
>> +        test_bit_size = XC_PAGE_SIZE;
>> +        cache_size = MCACHE_BUCKET_SIZE;
>> +    }
>> +
>> +    /* Search for the requested map cache entry to invalidate */
>> +    entry = &mapcache->entry[address_index % mapcache->nr_buckets];
>> +    while (entry && !(entry->paddr_index == address_index && entry->size == cache_size)) {
>> +        pentry = entry;
>> +        entry = entry->next;
>> +    }
>> +    if (!entry) {
>> +        DPRINTF("Trying to update an entry for %lx that is not in the mapcache!\n", phys_addr);
>> +        return NULL;
>> +    }
>> +
>> +    vaddr = entry->vaddr_base;
>> +    lock = entry->lock;
>> +    if (entry->vaddr_base) {
>> +        ram_block_notify_remove(entry->vaddr_base, entry->size);
>> +        if (munmap(entry->vaddr_base, entry->size) != 0) {
>> +            perror("unmap fails");
>> +            exit(-1);
>> +        }
>> +    }
> 
> Why are we calling ram_block_notify_remove? Isn't the ram_block about to
> be remapped with the correct underlying pages?
> 

It's here for symmetry. xen_remap_bucket is going to call
ram_block_notify_add and I didn't want to gate it there. But I probably
should.

> 
>> +    entry->vaddr_base = NULL;
>> +    entry->lock = 0;
> 
> Why can't we just keep using this entry as is?
> 
> 
>> +    if (mapcache->phys_offset_to_gaddr) {
>> +        phys_addr = mapcache->phys_offset_to_gaddr(phys_addr, size, mapcache->opaque);
>> +
>> +        address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
>> +        address_offset = phys_addr & (MCACHE_BUCKET_SIZE - 1);
>> +    }
> 
> Instead of having this check to find the new address, why don't we just
> pass it to xen_remap_cache_entry_unlocked as an argument?
> 

Agree.

> 
>> +    /* Address may have changed so we need to repeat the search */
>> +    entry = &mapcache->entry[address_index % mapcache->nr_buckets];
>> +    while (entry && entry->lock && entry->vaddr_base) {
>> +        pentry = entry;
>> +        entry = entry->next;
>> +    }
>> +    if (!entry) {
>> +        entry = g_malloc0(sizeof (MapCacheEntry));
>> +        pentry->next = entry;
>> +    }
> 
> Is it really possible to already have a mapcache entry for the new
> phys_addr, which has never been used before? It doesn't look like it.
> Also, why are we creating a new entry instead of simply reusing the
> previous one?
> 

I wasn't completely sure it's safe to reuse the previous entry since
it's indexed by its previous physical address. But it looks like that
QEMU is going to use only its old address for referencing. That'll make
the code much simpler.

> 
>> +    entry->lock = 0;
>> +    xen_remap_bucket(entry, vaddr, cache_size, address_index, false);
>> +    if(!test_bits(address_offset >> XC_PAGE_SHIFT,
>> +                test_bit_size >> XC_PAGE_SHIFT,
>> +                entry->valid_mapping)) {
>> +        DPRINTF("Unable to update an entry for %lx in the mapcache!\n", phys_addr);
>> +        return NULL;
>> +    }
>> +
>> +    entry->lock = lock;
>> +    return entry->vaddr_base + address_offset;
>> +}
>> +
>> +uint8_t *xen_remap_cache_entry(hwaddr phys_addr, hwaddr size)
>> +{
>> +    uint8_t *p;
>> +
>> +    mapcache_lock();
>> +    p = xen_remap_cache_entry_unlocked(phys_addr, size);
>> +    mapcache_unlock();
>> +    return p;
>> +}
>> diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
>> index e00ddd7..70a5cad 100644
>> --- a/include/hw/xen/xen_common.h
>> +++ b/include/hw/xen/xen_common.h
>> @@ -78,6 +78,13 @@ static inline void *xenforeignmemory_map(xc_interface *h, uint32_t dom,
>>  
>>  extern xenforeignmemory_handle *xen_fmem;
>>  
>> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
>> +
>> +#define xenforeignmemory_map2(h, d, a, p, f, ps, ar, e) \
>> +    xenforeignmemory_map(h, d, p, ps, ar, e)
>> +
>> +#endif
>> +
>>  #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
>>  
>>  typedef xc_interface xendevicemodel_handle;
>> diff --git a/include/sysemu/xen-mapcache.h b/include/sysemu/xen-mapcache.h
>> index 01daaad..8c140d0 100644
>> --- a/include/sysemu/xen-mapcache.h
>> +++ b/include/sysemu/xen-mapcache.h
>> @@ -21,6 +21,7 @@ uint8_t *xen_map_cache(hwaddr phys_addr, hwaddr size,
>>  ram_addr_t xen_ram_addr_from_mapcache(void *ptr);
>>  void xen_invalidate_map_cache_entry(uint8_t *buffer);
>>  void xen_invalidate_map_cache(void);
>> +uint8_t *xen_remap_cache_entry(hwaddr phys_addr, hwaddr size);
>>  
>>  #else
>>  
>> @@ -50,6 +51,11 @@ static inline void xen_invalidate_map_cache(void)
>>  {
>>  }
>>  
>> +static inline uint8_t *xen_remap_cache_entry(hwaddr phys_addr, hwaddr size)
>> +{
>> +    abort();
>> +}
>> +
>>  #endif
>>  
>>  #endif /* XEN_MAPCACHE_H */
>> -- 
>> 2.7.4
>>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Qemu-devel] [PATCH 4/4] xen: don't use xenstore to save/restore physmap anymore

[Igor Druzhinin]

If we have a system with xenforeignmemory_map2() implemented
we don't need to save/restore physmap on suspend/restore
anymore. In case we resume a VM without physmap - try to
recreate the physmap during memory region restore phase and
remap map cache entries accordingly. The old code is left
for compatibility reasons.

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
 hw/i386/xen/xen-hvm.c       | 45 ++++++++++++++++++++++++++++++++++-----------
 include/hw/xen/xen_common.h |  1 +
 2 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index d259cf7..1b6a5ce 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -305,6 +305,7 @@ static hwaddr xen_phys_offset_to_gaddr(hwaddr start_addr,
     return start_addr;
 }
 
+#ifdef XEN_COMPAT_PHYSMAP
 static int xen_save_physmap(XenIOState *state, XenPhysmap *physmap)
 {
     char path[80], value[17];
@@ -334,6 +335,12 @@ static int xen_save_physmap(XenIOState *state, XenPhysmap *physmap)
     }
     return 0;
 }
+#else
+static int xen_save_physmap(XenIOState *state, XenPhysmap *physmap)
+{
+    return 0;
+}
+#endif
 
 static int xen_add_to_physmap(XenIOState *state,
                               hwaddr start_addr,
@@ -368,6 +375,26 @@ go_physmap:
     DPRINTF("mapping vram to %"HWADDR_PRIx" - %"HWADDR_PRIx"\n",
             start_addr, start_addr + size);
 
+    mr_name = memory_region_name(mr);
+
+    physmap = g_malloc(sizeof (XenPhysmap));
+
+    physmap->start_addr = start_addr;
+    physmap->size = size;
+    physmap->name = mr_name;
+    physmap->phys_offset = phys_offset;
+
+    QLIST_INSERT_HEAD(&state->physmap, physmap, list);
+
+    if (runstate_check(RUN_STATE_INMIGRATE)) {
+        /* Now when we have a physmap entry we can remap a dummy mapping and change
+         * it to a real one of guest foreign memory. */
+        uint8_t *p = xen_remap_cache_entry(phys_offset, size);
+        assert(p && p == memory_region_get_ram_ptr(mr));
+
+        return 0;
+    }
+
     pfn = phys_offset >> TARGET_PAGE_BITS;
     start_gpfn = start_addr >> TARGET_PAGE_BITS;
     for (i = 0; i < size >> TARGET_PAGE_BITS; i++) {
@@ -382,21 +409,11 @@ go_physmap:
         }
     }
 
-    mr_name = memory_region_name(mr);
-
-    physmap = g_malloc(sizeof (XenPhysmap));
-
-    physmap->start_addr = start_addr;
-    physmap->size = size;
-    physmap->name = mr_name;
-    physmap->phys_offset = phys_offset;
-
-    QLIST_INSERT_HEAD(&state->physmap, physmap, list);
-
     xc_domain_pin_memory_cacheattr(xen_xc, xen_domid,
                                    start_addr >> TARGET_PAGE_BITS,
                                    (start_addr + size - 1) >> TARGET_PAGE_BITS,
                                    XEN_DOMCTL_MEM_CACHEATTR_WB);
+
     return xen_save_physmap(state, physmap);
 }
 
@@ -1158,6 +1175,7 @@ static void xen_exit_notifier(Notifier *n, void *data)
     xs_daemon_close(state->xenstore);
 }
 
+#ifdef XEN_COMPAT_PHYSMAP
 static void xen_read_physmap(XenIOState *state)
 {
     XenPhysmap *physmap = NULL;
@@ -1205,6 +1223,11 @@ static void xen_read_physmap(XenIOState *state)
     }
     free(entries);
 }
+#else
+static void xen_read_physmap(XenIOState *state)
+{
+}
+#endif
 
 static void xen_wakeup_notifier(Notifier *notifier, void *data)
 {
diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 70a5cad..c04c5c9 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -80,6 +80,7 @@ extern xenforeignmemory_handle *xen_fmem;
 
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
 
+#define XEN_COMPAT_PHYSMAP
 #define xenforeignmemory_map2(h, d, a, p, f, ps, ar, e) \
     xenforeignmemory_map(h, d, p, ps, ar, e)
 
-- 
2.7.4

[PATCH 4/4] xen: don't use xenstore to save/restore physmap anymore

[Igor Druzhinin]

If we have a system with xenforeignmemory_map2() implemented
we don't need to save/restore physmap on suspend/restore
anymore. In case we resume a VM without physmap - try to
recreate the physmap during memory region restore phase and
remap map cache entries accordingly. The old code is left
for compatibility reasons.

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
 hw/i386/xen/xen-hvm.c       | 45 ++++++++++++++++++++++++++++++++++-----------
 include/hw/xen/xen_common.h |  1 +
 2 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index d259cf7..1b6a5ce 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -305,6 +305,7 @@ static hwaddr xen_phys_offset_to_gaddr(hwaddr start_addr,
     return start_addr;
 }
 
+#ifdef XEN_COMPAT_PHYSMAP
 static int xen_save_physmap(XenIOState *state, XenPhysmap *physmap)
 {
     char path[80], value[17];
@@ -334,6 +335,12 @@ static int xen_save_physmap(XenIOState *state, XenPhysmap *physmap)
     }
     return 0;
 }
+#else
+static int xen_save_physmap(XenIOState *state, XenPhysmap *physmap)
+{
+    return 0;
+}
+#endif
 
 static int xen_add_to_physmap(XenIOState *state,
                               hwaddr start_addr,
@@ -368,6 +375,26 @@ go_physmap:
     DPRINTF("mapping vram to %"HWADDR_PRIx" - %"HWADDR_PRIx"\n",
             start_addr, start_addr + size);
 
+    mr_name = memory_region_name(mr);
+
+    physmap = g_malloc(sizeof (XenPhysmap));
+
+    physmap->start_addr = start_addr;
+    physmap->size = size;
+    physmap->name = mr_name;
+    physmap->phys_offset = phys_offset;
+
+    QLIST_INSERT_HEAD(&state->physmap, physmap, list);
+
+    if (runstate_check(RUN_STATE_INMIGRATE)) {
+        /* Now when we have a physmap entry we can remap a dummy mapping and change
+         * it to a real one of guest foreign memory. */
+        uint8_t *p = xen_remap_cache_entry(phys_offset, size);
+        assert(p && p == memory_region_get_ram_ptr(mr));
+
+        return 0;
+    }
+
     pfn = phys_offset >> TARGET_PAGE_BITS;
     start_gpfn = start_addr >> TARGET_PAGE_BITS;
     for (i = 0; i < size >> TARGET_PAGE_BITS; i++) {
@@ -382,21 +409,11 @@ go_physmap:
         }
     }
 
-    mr_name = memory_region_name(mr);
-
-    physmap = g_malloc(sizeof (XenPhysmap));
-
-    physmap->start_addr = start_addr;
-    physmap->size = size;
-    physmap->name = mr_name;
-    physmap->phys_offset = phys_offset;
-
-    QLIST_INSERT_HEAD(&state->physmap, physmap, list);
-
     xc_domain_pin_memory_cacheattr(xen_xc, xen_domid,
                                    start_addr >> TARGET_PAGE_BITS,
                                    (start_addr + size - 1) >> TARGET_PAGE_BITS,
                                    XEN_DOMCTL_MEM_CACHEATTR_WB);
+
     return xen_save_physmap(state, physmap);
 }
 
@@ -1158,6 +1175,7 @@ static void xen_exit_notifier(Notifier *n, void *data)
     xs_daemon_close(state->xenstore);
 }
 
+#ifdef XEN_COMPAT_PHYSMAP
 static void xen_read_physmap(XenIOState *state)
 {
     XenPhysmap *physmap = NULL;
@@ -1205,6 +1223,11 @@ static void xen_read_physmap(XenIOState *state)
     }
     free(entries);
 }
+#else
+static void xen_read_physmap(XenIOState *state)
+{
+}
+#endif
 
 static void xen_wakeup_notifier(Notifier *notifier, void *data)
 {
diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 70a5cad..c04c5c9 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -80,6 +80,7 @@ extern xenforeignmemory_handle *xen_fmem;
 
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
 
+#define XEN_COMPAT_PHYSMAP
 #define xenforeignmemory_map2(h, d, a, p, f, ps, ar, e) \
     xenforeignmemory_map(h, d, p, ps, ar, e)
 
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Qemu-devel] [PATCH 4/4] xen: don't use xenstore to save/restore physmap anymore

[Stefano Stabellini]

On Fri, 30 Jun 2017, Igor Druzhinin wrote:
> If we have a system with xenforeignmemory_map2() implemented
> we don't need to save/restore physmap on suspend/restore
> anymore. In case we resume a VM without physmap - try to
> recreate the physmap during memory region restore phase and
> remap map cache entries accordingly. The old code is left
> for compatibility reasons.
> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> ---
>  hw/i386/xen/xen-hvm.c       | 45 ++++++++++++++++++++++++++++++++++-----------
>  include/hw/xen/xen_common.h |  1 +
>  2 files changed, 35 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> index d259cf7..1b6a5ce 100644
> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c
> @@ -305,6 +305,7 @@ static hwaddr xen_phys_offset_to_gaddr(hwaddr start_addr,
>      return start_addr;
>  }
>  
> +#ifdef XEN_COMPAT_PHYSMAP
>  static int xen_save_physmap(XenIOState *state, XenPhysmap *physmap)
>  {
>      char path[80], value[17];
> @@ -334,6 +335,12 @@ static int xen_save_physmap(XenIOState *state, XenPhysmap *physmap)
>      }
>      return 0;
>  }
> +#else
> +static int xen_save_physmap(XenIOState *state, XenPhysmap *physmap)
> +{
> +    return 0;
> +}
> +#endif
>  
>  static int xen_add_to_physmap(XenIOState *state,
>                                hwaddr start_addr,
> @@ -368,6 +375,26 @@ go_physmap:
>      DPRINTF("mapping vram to %"HWADDR_PRIx" - %"HWADDR_PRIx"\n",
>              start_addr, start_addr + size);
>  
> +    mr_name = memory_region_name(mr);
> +
> +    physmap = g_malloc(sizeof (XenPhysmap));
> +
> +    physmap->start_addr = start_addr;
> +    physmap->size = size;
> +    physmap->name = mr_name;
> +    physmap->phys_offset = phys_offset;
> +
> +    QLIST_INSERT_HEAD(&state->physmap, physmap, list);
> +
> +    if (runstate_check(RUN_STATE_INMIGRATE)) {
> +        /* Now when we have a physmap entry we can remap a dummy mapping and change
> +         * it to a real one of guest foreign memory. */
> +        uint8_t *p = xen_remap_cache_entry(phys_offset, size);
> +        assert(p && p == memory_region_get_ram_ptr(mr));

I would just pass start_addr to xen_remap_cache_entry as argument. It
would make things easier. With that, I think we should also be able to
#ifdef xen_phys_offset_to_gaddr and the call to phys_offset_to_gaddr in
xen_map_cache_unlocked, right?  It would make things simpler.


> +        return 0;
> +    }
>      pfn = phys_offset >> TARGET_PAGE_BITS;
>      start_gpfn = start_addr >> TARGET_PAGE_BITS;
>      for (i = 0; i < size >> TARGET_PAGE_BITS; i++) {
> @@ -382,21 +409,11 @@ go_physmap:
>          }
>      }
>  
> -    mr_name = memory_region_name(mr);
> -
> -    physmap = g_malloc(sizeof (XenPhysmap));
> -
> -    physmap->start_addr = start_addr;
> -    physmap->size = size;
> -    physmap->name = mr_name;
> -    physmap->phys_offset = phys_offset;
> -
> -    QLIST_INSERT_HEAD(&state->physmap, physmap, list);
> -
>      xc_domain_pin_memory_cacheattr(xen_xc, xen_domid,
>                                     start_addr >> TARGET_PAGE_BITS,
>                                     (start_addr + size - 1) >> TARGET_PAGE_BITS,
>                                     XEN_DOMCTL_MEM_CACHEATTR_WB);
> +

Spurious change


>      return xen_save_physmap(state, physmap);
>  }
>  
> @@ -1158,6 +1175,7 @@ static void xen_exit_notifier(Notifier *n, void *data)
>      xs_daemon_close(state->xenstore);
>  }
>  
> +#ifdef XEN_COMPAT_PHYSMAP
>  static void xen_read_physmap(XenIOState *state)
>  {
>      XenPhysmap *physmap = NULL;
> @@ -1205,6 +1223,11 @@ static void xen_read_physmap(XenIOState *state)
>      }
>      free(entries);
>  }
> +#else
> +static void xen_read_physmap(XenIOState *state)
> +{
> +}
> +#endif
>  
>  static void xen_wakeup_notifier(Notifier *notifier, void *data)
>  {
> diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
> index 70a5cad..c04c5c9 100644
> --- a/include/hw/xen/xen_common.h
> +++ b/include/hw/xen/xen_common.h
> @@ -80,6 +80,7 @@ extern xenforeignmemory_handle *xen_fmem;
>  
>  #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
>  
> +#define XEN_COMPAT_PHYSMAP
>  #define xenforeignmemory_map2(h, d, a, p, f, ps, ar, e) \
>      xenforeignmemory_map(h, d, p, ps, ar, e)
>  
> -- 
> 2.7.4
> 

Re: [PATCH 4/4] xen: don't use xenstore to save/restore physmap anymore

[Stefano Stabellini]

On Fri, 30 Jun 2017, Igor Druzhinin wrote:
> If we have a system with xenforeignmemory_map2() implemented
> we don't need to save/restore physmap on suspend/restore
> anymore. In case we resume a VM without physmap - try to
> recreate the physmap during memory region restore phase and
> remap map cache entries accordingly. The old code is left
> for compatibility reasons.
> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> ---
>  hw/i386/xen/xen-hvm.c       | 45 ++++++++++++++++++++++++++++++++++-----------
>  include/hw/xen/xen_common.h |  1 +
>  2 files changed, 35 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> index d259cf7..1b6a5ce 100644
> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c
> @@ -305,6 +305,7 @@ static hwaddr xen_phys_offset_to_gaddr(hwaddr start_addr,
>      return start_addr;
>  }
>  
> +#ifdef XEN_COMPAT_PHYSMAP
>  static int xen_save_physmap(XenIOState *state, XenPhysmap *physmap)
>  {
>      char path[80], value[17];
> @@ -334,6 +335,12 @@ static int xen_save_physmap(XenIOState *state, XenPhysmap *physmap)
>      }
>      return 0;
>  }
> +#else
> +static int xen_save_physmap(XenIOState *state, XenPhysmap *physmap)
> +{
> +    return 0;
> +}
> +#endif
>  
>  static int xen_add_to_physmap(XenIOState *state,
>                                hwaddr start_addr,
> @@ -368,6 +375,26 @@ go_physmap:
>      DPRINTF("mapping vram to %"HWADDR_PRIx" - %"HWADDR_PRIx"\n",
>              start_addr, start_addr + size);
>  
> +    mr_name = memory_region_name(mr);
> +
> +    physmap = g_malloc(sizeof (XenPhysmap));
> +
> +    physmap->start_addr = start_addr;
> +    physmap->size = size;
> +    physmap->name = mr_name;
> +    physmap->phys_offset = phys_offset;
> +
> +    QLIST_INSERT_HEAD(&state->physmap, physmap, list);
> +
> +    if (runstate_check(RUN_STATE_INMIGRATE)) {
> +        /* Now when we have a physmap entry we can remap a dummy mapping and change
> +         * it to a real one of guest foreign memory. */
> +        uint8_t *p = xen_remap_cache_entry(phys_offset, size);
> +        assert(p && p == memory_region_get_ram_ptr(mr));

I would just pass start_addr to xen_remap_cache_entry as argument. It
would make things easier. With that, I think we should also be able to
#ifdef xen_phys_offset_to_gaddr and the call to phys_offset_to_gaddr in
xen_map_cache_unlocked, right?  It would make things simpler.


> +        return 0;
> +    }
>      pfn = phys_offset >> TARGET_PAGE_BITS;
>      start_gpfn = start_addr >> TARGET_PAGE_BITS;
>      for (i = 0; i < size >> TARGET_PAGE_BITS; i++) {
> @@ -382,21 +409,11 @@ go_physmap:
>          }
>      }
>  
> -    mr_name = memory_region_name(mr);
> -
> -    physmap = g_malloc(sizeof (XenPhysmap));
> -
> -    physmap->start_addr = start_addr;
> -    physmap->size = size;
> -    physmap->name = mr_name;
> -    physmap->phys_offset = phys_offset;
> -
> -    QLIST_INSERT_HEAD(&state->physmap, physmap, list);
> -
>      xc_domain_pin_memory_cacheattr(xen_xc, xen_domid,
>                                     start_addr >> TARGET_PAGE_BITS,
>                                     (start_addr + size - 1) >> TARGET_PAGE_BITS,
>                                     XEN_DOMCTL_MEM_CACHEATTR_WB);
> +

Spurious change


>      return xen_save_physmap(state, physmap);
>  }
>  
> @@ -1158,6 +1175,7 @@ static void xen_exit_notifier(Notifier *n, void *data)
>      xs_daemon_close(state->xenstore);
>  }
>  
> +#ifdef XEN_COMPAT_PHYSMAP
>  static void xen_read_physmap(XenIOState *state)
>  {
>      XenPhysmap *physmap = NULL;
> @@ -1205,6 +1223,11 @@ static void xen_read_physmap(XenIOState *state)
>      }
>      free(entries);
>  }
> +#else
> +static void xen_read_physmap(XenIOState *state)
> +{
> +}
> +#endif
>  
>  static void xen_wakeup_notifier(Notifier *notifier, void *data)
>  {
> diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
> index 70a5cad..c04c5c9 100644
> --- a/include/hw/xen/xen_common.h
> +++ b/include/hw/xen/xen_common.h
> @@ -80,6 +80,7 @@ extern xenforeignmemory_handle *xen_fmem;
>  
>  #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
>  
> +#define XEN_COMPAT_PHYSMAP
>  #define xenforeignmemory_map2(h, d, a, p, f, ps, ar, e) \
>      xenforeignmemory_map(h, d, p, ps, ar, e)
>  
> -- 
> 2.7.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel