Re: [Qemu-devel] [Xen-devel] [PATCH v5] xen: don't save/restore the physmap on VM save/restore
On 16/03/17 12:54, Igor Druzhinin wrote: > On 16/03/17 12:26, Anthony PERARD wrote: >> On Wed, Mar 15, 2017 at 04:01:19PM +0000, Igor Druzhinin wrote: >>> 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. >>> >>> The extraneous complexity of having those keys transferred by the >>> toolstack and unnecessary redundancy prompted us to propose a >>> solution which doesn't require any extra data in xenstore. The idea >>> is to defer the VRAM region mapping till the point we actually know >>> the effective address and able to map it. To that end, we initially >>> just skip the mapping request for the framebuffer if we unable to >>> map it now. Then, after the memory region restore phase, we perform >>> the mapping again, this time successfully, and update the VRAM region >>> metadata accordingly. >>> >>> Signed-off-by: Igor Druzhinin <igor.druzhi...@citrix.com> >> >> I've tried to migrate a guest with this patch, but once migrated, the >> screen is black (via VNC, keyboard is working fine). >> >> I haven't try to migrate a guest from QEMU without this patch to a QEMU >> with it. >> > > Hmm. It works for me - I've tried to migrate between identical QEMUs > with this patch on localhost. Save/restore also works fine. > > What do you mean 'the screen is black'? Could you describe your actions > so I could try to reproduce it? Ok. I could track down the issue - starting from v4 the patch doesn't work for cirrus. The reason is that post_load handler is different for cirrus and doesn't call the parent handler from common vga code. I manage to fix it by updating the corresponding handler by duplicating the code. But is it a good solution? Would it be better to have the common handler called in this function instead? > > Igor > > ___ > Xen-devel mailing list > xen-de...@lists.xen.org > https://lists.xen.org/xen-devel >
[Qemu-devel] [PATCH v5] xen: don't save/restore the physmap on VM save/restore
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.
The extraneous complexity of having those keys transferred by the
toolstack and unnecessary redundancy prompted us to propose a
solution which doesn't require any extra data in xenstore. The idea
is to defer the VRAM region mapping till the point we actually know
the effective address and able to map it. To that end, we initially
just skip the mapping request for the framebuffer if we unable to
map it now. Then, after the memory region restore phase, we perform
the mapping again, this time successfully, and update the VRAM region
metadata accordingly.
Signed-off-by: Igor Druzhinin <igor.druzhi...@citrix.com>
---
v5:
* Add an assertion and debug printf
v4:
* Use VGA post_load handler for vram_ptr update
v3:
* Modify qemu_ram_ptr_length similarly with qemu_map_ram_ptr
* Add a comment explaining qemu_map_ram_ptr and qemu_ram_ptr_length
semantic change for Xen
* Dropped some redundant changes
v2:
* Fix some building and coding style issues
---
exec.c | 16 +
hw/display/vga.c | 11 ++
xen-hvm.c| 104 ++-
3 files changed, 46 insertions(+), 85 deletions(-)
diff --git a/exec.c b/exec.c
index aabb035..a1ac8cd 100644
--- a/exec.c
+++ b/exec.c
@@ -2008,6 +2008,14 @@ void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t
addr)
}
block->host = xen_map_cache(block->offset, block->max_length, 1);
+if (block->host == NULL) {
+/* In case we cannot establish the mapping right away we might
+ * still be able to do it later e.g. on a later stage of restore.
+ * We don't touch the block and return NULL here to indicate
+ * that intention.
+ */
+return NULL;
+}
}
return ramblock_ptr(block, addr);
}
@@ -2041,6 +2049,14 @@ static void *qemu_ram_ptr_length(RAMBlock *ram_block,
ram_addr_t addr,
}
block->host = xen_map_cache(block->offset, block->max_length, 1);
+if (block->host == NULL) {
+/* In case we cannot establish the mapping right away we might
+ * still be able to do it later e.g. on a later stage of restore.
+ * We don't touch the block and return NULL here to indicate
+ * that intention.
+ */
+return NULL;
+}
}
return ramblock_ptr(block, addr);
diff --git a/hw/display/vga.c b/hw/display/vga.c
index 69c3e1d..7d85fd8 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -2035,6 +2035,12 @@ static int vga_common_post_load(void *opaque, int
version_id)
{
VGACommonState *s = opaque;
+if (xen_enabled() && !s->vram_ptr) {
+/* update VRAM region pointer in case we've failed
+ * the last time during init phase */
+s->vram_ptr = memory_region_get_ram_ptr(>vram);
+assert(s->vram_ptr);
+}
/* force refresh */
s->graphic_mode = -1;
vbe_update_vgaregs(s);
@@ -2165,6 +2171,11 @@ void vga_common_init(VGACommonState *s, Object *obj,
bool global_vmstate)
vmstate_register_ram(>vram, global_vmstate ? NULL : DEVICE(obj));
xen_register_framebuffer(>vram);
s->vram_ptr = memory_region_get_ram_ptr(>vram);
+/* VRAM pointer might still be NULL here if we are restoring on Xen.
+ We try to get it again later at post-load phase. */
+#ifdef DEBUG_VGA_MEM
+printf("vga: vram ptr: %p\n", s->vram_ptr);
+#endif
s->get_bpp = vga_get_bpp;
s->get_offsets = vga_get_offsets;
s->get_resolution = vga_get_resolution;
diff --git a/xen-hvm.c b/xen-hvm.c
index 5043beb..8bedd9b 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -317,7 +317,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)) {
@@ -340,6 +339,22 @@ 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(
Re: [Qemu-devel] [PATCH v5] xen: don't save/restore the physmap on VM save/restore
On 16/03/17 12:26, Anthony PERARD wrote: > On Wed, Mar 15, 2017 at 04:01:19PM +0000, Igor Druzhinin wrote: >> 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. >> >> The extraneous complexity of having those keys transferred by the >> toolstack and unnecessary redundancy prompted us to propose a >> solution which doesn't require any extra data in xenstore. The idea >> is to defer the VRAM region mapping till the point we actually know >> the effective address and able to map it. To that end, we initially >> just skip the mapping request for the framebuffer if we unable to >> map it now. Then, after the memory region restore phase, we perform >> the mapping again, this time successfully, and update the VRAM region >> metadata accordingly. >> >> Signed-off-by: Igor Druzhinin <igor.druzhi...@citrix.com> > > I've tried to migrate a guest with this patch, but once migrated, the > screen is black (via VNC, keyboard is working fine). > > I haven't try to migrate a guest from QEMU without this patch to a QEMU > with it. > Hmm. It works for me - I've tried to migrate between identical QEMUs with this patch on localhost. Save/restore also works fine. What do you mean 'the screen is black'? Could you describe your actions so I could try to reproduce it? Igor
[Qemu-devel] [PATCH] xen: don't save/restore the physmap on VM save/restore
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.
The extraneous complexity of having those keys transferred by the
toolstack and unnecessary redundancy prompted us to propose a
solution which doesn't require any extra data in xenstore. The idea
is to defer the VRAM region mapping till the point we actually know
the effective address and able to map it. To that end, we initially
only register the pointer to the framebuffer without actual mapping.
Then, during the memory region restore phase, we perform the mapping
of the known address and update the VRAM region metadata (including
previously registered pointer) accordingly.
Signed-off-by: Igor Druzhinin <igor.druzhi...@citrix.com>
---
exec.c | 3 ++
hw/display/vga.c | 2 +-
include/hw/xen/xen.h | 2 +-
xen-hvm.c| 114 ---
4 files changed, 32 insertions(+), 89 deletions(-)
diff --git a/exec.c b/exec.c
index aabb035..5f2809e 100644
--- a/exec.c
+++ b/exec.c
@@ -2008,6 +2008,9 @@ void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t
addr)
}
block->host = xen_map_cache(block->offset, block->max_length, 1);
+if (block->host == NULL) {
+return NULL;
+}
}
return ramblock_ptr(block, addr);
}
diff --git a/hw/display/vga.c b/hw/display/vga.c
index 69c3e1d..be554c2 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -2163,7 +2163,7 @@ void vga_common_init(VGACommonState *s, Object *obj, bool
global_vmstate)
memory_region_init_ram(>vram, obj, "vga.vram", s->vram_size,
_fatal);
vmstate_register_ram(>vram, global_vmstate ? NULL : DEVICE(obj));
-xen_register_framebuffer(>vram);
+xen_register_framebuffer(>vram, >vram_ptr);
s->vram_ptr = memory_region_get_ram_ptr(>vram);
s->get_bpp = vga_get_bpp;
s->get_offsets = vga_get_offsets;
diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index 09c2ce5..3831843 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -45,6 +45,6 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
struct MemoryRegion *mr, Error **errp);
void xen_modified_memory(ram_addr_t start, ram_addr_t length);
-void xen_register_framebuffer(struct MemoryRegion *mr);
+void xen_register_framebuffer(struct MemoryRegion *mr, uint8_t **ptr);
#endif /* QEMU_HW_XEN_H */
diff --git a/xen-hvm.c b/xen-hvm.c
index 5043beb..ea5ed24 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -41,6 +41,7 @@
static MemoryRegion ram_memory, ram_640k, ram_lo, ram_hi;
static MemoryRegion *framebuffer;
+static uint8_t **framebuffer_ptr;
static bool xen_in_migration;
/* Compatibility with older version */
@@ -302,7 +303,6 @@ static hwaddr xen_phys_offset_to_gaddr(hwaddr start_addr,
return physmap->start_addr;
}
}
-
return start_addr;
}
@@ -317,7 +317,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)) {
@@ -340,6 +339,27 @@ 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(>physmap, physmap, list);
+
+if (runstate_check(RUN_STATE_INMIGRATE))
+{
+/* At this point we have a physmap entry for the framebuffer region
+ * established during the restore phase so we can safely update the
+ * registered framebuffer address here. */
+ if (mr == framebuffer)
+*framebuffer_ptr = memory_region_get_ram_ptr(framebuffer);
+return 0;
+}
+
pfn = phys_offset >> TARGET_PAGE_BITS;
start_gpfn = start_addr >> TARGET_PAGE_BITS;
for (i = 0; i < size >> TARGET_PAGE_BITS; i++) {
@@ -350,49 +370,17 @@ go_physmap:
if (rc) {
[Qemu-devel] [PATCH v2] xen: don't save/restore the physmap on VM save/restore
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.
The extraneous complexity of having those keys transferred by the
toolstack and unnecessary redundancy prompted us to propose a
solution which doesn't require any extra data in xenstore. The idea
is to defer the VRAM region mapping till the point we actually know
the effective address and able to map it. To that end, we initially
only register the pointer to the framebuffer without actual mapping.
Then, during the memory region restore phase, we perform the mapping
of the known address and update the VRAM region metadata (including
previously registered pointer) accordingly.
Signed-off-by: Igor Druzhinin <igor.druzhi...@citrix.com>
---
v2:
* Fix some building and coding style issues
---
exec.c | 3 ++
hw/display/vga.c | 2 +-
include/hw/xen/xen.h | 2 +-
xen-hvm-stub.c | 2 +-
xen-hvm.c| 114 ---
5 files changed, 33 insertions(+), 90 deletions(-)
diff --git a/exec.c b/exec.c
index aabb035..5f2809e 100644
--- a/exec.c
+++ b/exec.c
@@ -2008,6 +2008,9 @@ void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t
addr)
}
block->host = xen_map_cache(block->offset, block->max_length, 1);
+if (block->host == NULL) {
+return NULL;
+}
}
return ramblock_ptr(block, addr);
}
diff --git a/hw/display/vga.c b/hw/display/vga.c
index 69c3e1d..be554c2 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -2163,7 +2163,7 @@ void vga_common_init(VGACommonState *s, Object *obj, bool
global_vmstate)
memory_region_init_ram(>vram, obj, "vga.vram", s->vram_size,
_fatal);
vmstate_register_ram(>vram, global_vmstate ? NULL : DEVICE(obj));
-xen_register_framebuffer(>vram);
+xen_register_framebuffer(>vram, >vram_ptr);
s->vram_ptr = memory_region_get_ram_ptr(>vram);
s->get_bpp = vga_get_bpp;
s->get_offsets = vga_get_offsets;
diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index 09c2ce5..3831843 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -45,6 +45,6 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
struct MemoryRegion *mr, Error **errp);
void xen_modified_memory(ram_addr_t start, ram_addr_t length);
-void xen_register_framebuffer(struct MemoryRegion *mr);
+void xen_register_framebuffer(struct MemoryRegion *mr, uint8_t **ptr);
#endif /* QEMU_HW_XEN_H */
diff --git a/xen-hvm-stub.c b/xen-hvm-stub.c
index c500325..c89065e 100644
--- a/xen-hvm-stub.c
+++ b/xen-hvm-stub.c
@@ -46,7 +46,7 @@ qemu_irq *xen_interrupt_controller_init(void)
return NULL;
}
-void xen_register_framebuffer(MemoryRegion *mr)
+void xen_register_framebuffer(MemoryRegion *mr, uint8_t **ptr)
{
}
diff --git a/xen-hvm.c b/xen-hvm.c
index 5043beb..270cd99 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -41,6 +41,7 @@
static MemoryRegion ram_memory, ram_640k, ram_lo, ram_hi;
static MemoryRegion *framebuffer;
+static uint8_t **framebuffer_ptr;
static bool xen_in_migration;
/* Compatibility with older version */
@@ -302,7 +303,6 @@ static hwaddr xen_phys_offset_to_gaddr(hwaddr start_addr,
return physmap->start_addr;
}
}
-
return start_addr;
}
@@ -317,7 +317,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)) {
@@ -340,6 +339,27 @@ 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(>physmap, physmap, list);
+
+if (runstate_check(RUN_STATE_INMIGRATE)) {
+/* At this point we have a physmap entry for the framebuffer region
+ * established during the restore phase so we can safely update the
+
Re: [Qemu-devel] [PATCH v3] xen: don't save/restore the physmap on VM save/restore
On 13/03/17 21:15, Stefano Stabellini wrote:
> On Mon, 13 Mar 2017, Igor Druzhinin wrote:
>> 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.
>>
>> The extraneous complexity of having those keys transferred by the
>> toolstack and unnecessary redundancy prompted us to propose a
>> solution which doesn't require any extra data in xenstore. The idea
>> is to defer the VRAM region mapping till the point we actually know
>> the effective address and able to map it. To that end, we initially
>> only register the pointer to the framebuffer without actual mapping.
>> Then, during the memory region restore phase, we perform the mapping
>> of the known address and update the VRAM region metadata (including
>> previously registered pointer) accordingly.
>>
>> Signed-off-by: Igor Druzhinin <igor.druzhi...@citrix.com>
>
> Let me get this straight. The current sequence is:
>
> - read physmap from xenstore, including vram and rom addresses
> - vga initialization
> - register framebuffer with xen-hvm.c
> - set vram_ptr by mapping the vram region using xen_map_cache
> - rtl8139 initialization
> - map rom files using xen_map_cache
>
> The new sequence would be:
>
> - vga initialization
> - register framebuffer and _ptr with xen-hvm.c
> - set vram_ptr to NULL because we don't know the vram address yet
> - rtl8139 initialization
> - map rom files using xen_map_cache ???
> - the vram address is discovered as part of the savevm file
> - when the vram region is mapped into the guest, set vram_ptr to the right
> value
>
>
> Is that right? If so, why can't we just move the
>
> s->vram_ptr = memory_region_get_ram_ptr(>vram);
>
> line in vga.c to later? It would be better than changing the value of
> vram_ptr behind the scenes. Clearer for the vga maintainers too.
>
Yes, it's one of the possible solutions. Probably would require more
changes in VGA code. But I'll take a look at this.
>
> But my main concern is actually rom files. The current physmap mechanism
> also covers roms, such as the rtl8139 rom, which is used for pxebooting
> from the VM. How do you plan to cover those?
>
Here is an excerpt from xen_add_to_physmap() which clearly indicates
that the only region that we track now is VRAM region.
if (mr == framebuffer && start_addr > 0xb) {
goto go_physmap;
}
return -1;
Maybe I'm missing something?
Igor
>
>> v3:
>> * Modify qemu_ram_ptr_length similarly with qemu_map_ram_ptr
>> * Add a comment explaining qemu_map_ram_ptr and qemu_ram_ptr_length
>> semantic change for Xen
>> * Dropped some redundant changes
>>
>> v2:
>> * Fix some building and coding style issues
>>
>> ---
>> exec.c | 16
>> hw/display/vga.c | 2 +-
>> include/hw/xen/xen.h | 2 +-
>> xen-hvm-stub.c | 2 +-
>> xen-hvm.c| 111
>> ---
>> 5 files changed, 44 insertions(+), 89 deletions(-)
>>
>> diff --git a/exec.c b/exec.c
>> index aabb035..a1ac8cd 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -2008,6 +2008,14 @@ void *qemu_map_ram_ptr(RAMBlock *ram_block,
>> ram_addr_t addr)
>> }
>>
>> block->host = xen_map_cache(block->offset, block->max_length, 1);
>> +if (block->host == NULL) {
>> +/* In case we cannot establish the mapping right away we might
>> + * still be able to do it later e.g. on a later stage of
>> restore.
>> + * We don't touch the block and return NULL here to indicate
>> + * that intention.
>> + */
>> +return NULL;
>> +}
>> }
>> return ramblock_ptr(block, addr);
>> }
>> @@ -2041,6 +2049,14 @@ static void *qemu_ram_ptr_length(RAMBlock *ram_block,
>> ram_addr_t addr,
>>
[Qemu-devel] [PATCH v3] xen: don't save/restore the physmap on VM save/restore
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.
The extraneous complexity of having those keys transferred by the
toolstack and unnecessary redundancy prompted us to propose a
solution which doesn't require any extra data in xenstore. The idea
is to defer the VRAM region mapping till the point we actually know
the effective address and able to map it. To that end, we initially
only register the pointer to the framebuffer without actual mapping.
Then, during the memory region restore phase, we perform the mapping
of the known address and update the VRAM region metadata (including
previously registered pointer) accordingly.
Signed-off-by: Igor Druzhinin <igor.druzhi...@citrix.com>
---
v3:
* Modify qemu_ram_ptr_length similarly with qemu_map_ram_ptr
* Add a comment explaining qemu_map_ram_ptr and qemu_ram_ptr_length
semantic change for Xen
* Dropped some redundant changes
v2:
* Fix some building and coding style issues
---
exec.c | 16
hw/display/vga.c | 2 +-
include/hw/xen/xen.h | 2 +-
xen-hvm-stub.c | 2 +-
xen-hvm.c| 111 ---
5 files changed, 44 insertions(+), 89 deletions(-)
diff --git a/exec.c b/exec.c
index aabb035..a1ac8cd 100644
--- a/exec.c
+++ b/exec.c
@@ -2008,6 +2008,14 @@ void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t
addr)
}
block->host = xen_map_cache(block->offset, block->max_length, 1);
+if (block->host == NULL) {
+/* In case we cannot establish the mapping right away we might
+ * still be able to do it later e.g. on a later stage of restore.
+ * We don't touch the block and return NULL here to indicate
+ * that intention.
+ */
+return NULL;
+}
}
return ramblock_ptr(block, addr);
}
@@ -2041,6 +2049,14 @@ static void *qemu_ram_ptr_length(RAMBlock *ram_block,
ram_addr_t addr,
}
block->host = xen_map_cache(block->offset, block->max_length, 1);
+if (block->host == NULL) {
+/* In case we cannot establish the mapping right away we might
+ * still be able to do it later e.g. on a later stage of restore.
+ * We don't touch the block and return NULL here to indicate
+ * that intention.
+ */
+return NULL;
+}
}
return ramblock_ptr(block, addr);
diff --git a/hw/display/vga.c b/hw/display/vga.c
index 69c3e1d..be554c2 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -2163,7 +2163,7 @@ void vga_common_init(VGACommonState *s, Object *obj, bool
global_vmstate)
memory_region_init_ram(>vram, obj, "vga.vram", s->vram_size,
_fatal);
vmstate_register_ram(>vram, global_vmstate ? NULL : DEVICE(obj));
-xen_register_framebuffer(>vram);
+xen_register_framebuffer(>vram, >vram_ptr);
s->vram_ptr = memory_region_get_ram_ptr(>vram);
s->get_bpp = vga_get_bpp;
s->get_offsets = vga_get_offsets;
diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index 09c2ce5..3831843 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -45,6 +45,6 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
struct MemoryRegion *mr, Error **errp);
void xen_modified_memory(ram_addr_t start, ram_addr_t length);
-void xen_register_framebuffer(struct MemoryRegion *mr);
+void xen_register_framebuffer(struct MemoryRegion *mr, uint8_t **ptr);
#endif /* QEMU_HW_XEN_H */
diff --git a/xen-hvm-stub.c b/xen-hvm-stub.c
index c500325..c89065e 100644
--- a/xen-hvm-stub.c
+++ b/xen-hvm-stub.c
@@ -46,7 +46,7 @@ qemu_irq *xen_interrupt_controller_init(void)
return NULL;
}
-void xen_register_framebuffer(MemoryRegion *mr)
+void xen_register_framebuffer(MemoryRegion *mr, uint8_t **ptr)
{
}
diff --git a/xen-hvm.c b/xen-hvm.c
index 5043beb..221334a 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -41,6 +41,7 @@
static MemoryRegion ram_memory, ram_640k, ram_lo, ram_hi;
static MemoryRegion *framebuffer;
+static uint8_t **framebuffer_ptr;
static bool xen_in_migration;
/* Compatibility with older version */
@@ -317,7 +318,6 @@ static int xen_add_to_physmap(XenIOState *state,
[Qemu-devel] [PATCH v4] xen: don't save/restore the physmap on VM save/restore
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.
The extraneous complexity of having those keys transferred by the
toolstack and unnecessary redundancy prompted us to propose a
solution which doesn't require any extra data in xenstore. The idea
is to defer the VRAM region mapping till the point we actually know
the effective address and able to map it. To that end, we initially
just skip the mapping request for the framebuffer if we unable to
map it now. Then, after the memory region restore phase, we perform
the mapping again, this time successfully, and update the VRAM region
metadata accordingly.
Signed-off-by: Igor Druzhinin <igor.druzhi...@citrix.com>
---
v4:
* Use VGA post_load handler for vram_ptr update
v3:
* Modify qemu_ram_ptr_length similarly with qemu_map_ram_ptr
* Add a comment explaining qemu_map_ram_ptr and qemu_ram_ptr_length
semantic change for Xen
* Dropped some redundant changes
v2:
* Fix some building and coding style issues
---
exec.c | 16 +
hw/display/vga.c | 5 +++
xen-hvm.c| 104 ++-
3 files changed, 40 insertions(+), 85 deletions(-)
diff --git a/exec.c b/exec.c
index aabb035..a1ac8cd 100644
--- a/exec.c
+++ b/exec.c
@@ -2008,6 +2008,14 @@ void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t
addr)
}
block->host = xen_map_cache(block->offset, block->max_length, 1);
+if (block->host == NULL) {
+/* In case we cannot establish the mapping right away we might
+ * still be able to do it later e.g. on a later stage of restore.
+ * We don't touch the block and return NULL here to indicate
+ * that intention.
+ */
+return NULL;
+}
}
return ramblock_ptr(block, addr);
}
@@ -2041,6 +2049,14 @@ static void *qemu_ram_ptr_length(RAMBlock *ram_block,
ram_addr_t addr,
}
block->host = xen_map_cache(block->offset, block->max_length, 1);
+if (block->host == NULL) {
+/* In case we cannot establish the mapping right away we might
+ * still be able to do it later e.g. on a later stage of restore.
+ * We don't touch the block and return NULL here to indicate
+ * that intention.
+ */
+return NULL;
+}
}
return ramblock_ptr(block, addr);
diff --git a/hw/display/vga.c b/hw/display/vga.c
index 69c3e1d..f8aebe3 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -2035,6 +2035,11 @@ static int vga_common_post_load(void *opaque, int
version_id)
{
VGACommonState *s = opaque;
+if (xen_enabled() && !s->vram_ptr) {
+/* update VRAM region pointer in case we've failed
+ * the last time during init phase */
+s->vram_ptr = memory_region_get_ram_ptr(>vram);
+}
/* force refresh */
s->graphic_mode = -1;
vbe_update_vgaregs(s);
diff --git a/xen-hvm.c b/xen-hvm.c
index 5043beb..8bedd9b 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -317,7 +317,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)) {
@@ -340,6 +339,22 @@ 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(>physmap, physmap, list);
+
+if (runstate_check(RUN_STATE_INMIGRATE)) {
+/* If we are migrating the region has been already mapped */
+return 0;
+}
+
pfn = phys_offset >> TARGET_PAGE_BITS;
start_gpfn = start_addr >> TARGET_PAGE_BITS;
for (i = 0; i < size >> TARGET_PAGE_BITS; i++) {
@@ -350,49 +365,17 @@ go_physmap:
if (rc) {
DPRINTF("add_to_physmap MFN %"PRI_xen_pfn" to PFN %"
[Qemu-devel] [PATCH v3 2/4] xen/mapcache: add an ability to create dummy mappings
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.druzhi...@citrix.com>
Reviewed-by: Paul Durrant <paul.durr...@citrix.com>
Reviewed-by: Stefano Stabellini <sstabell...@kernel.org>
---
hw/i386/xen/xen-mapcache.c | 44
1 file changed, 36 insertions(+), 8 deletions(-)
diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
index e60156c..39cb511 100644
--- a/hw/i386/xen/xen-mapcache.c
+++ b/hw/i386/xen/xen-mapcache.c
@@ -53,6 +53,8 @@ typedef struct MapCacheEntry {
uint8_t *vaddr_base;
unsigned long *valid_mapping;
uint8_t lock;
+#define XEN_MAPCACHE_ENTRY_DUMMY (1 << 0)
+uint8_t flags;
hwaddr size;
struct MapCacheEntry *next;
} MapCacheEntry;
@@ -150,7 +152,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 +180,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;
@@ -190,6 +207,12 @@ static void xen_remap_bucket(MapCacheEntry *entry,
entry->valid_mapping = (unsigned long *) g_malloc0(sizeof(unsigned long) *
BITS_TO_LONGS(size >> XC_PAGE_SHIFT));
+if (dummy) {
+entry->flags |= XEN_MAPCACHE_ENTRY_DUMMY;
+} else {
+entry->flags &= ~(XEN_MAPCACHE_ENTRY_DUMMY);
+}
+
ram_block_notify_add(entry->vaddr_base, entry->size);
bitmap_zero(entry->valid_mapping, nb_pfn);
for (i = 0; i < nb_pfn; i++) {
@@ -211,6 +234,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 +286,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 +306,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
[Qemu-devel] [PATCH v3 1/4] xen: move physmap saving into a separate function
Non-functional change.
Signed-off-by: Igor Druzhinin <igor.druzhi...@citrix.com>
Reviewed-by: Stefano Stabellini <sstabell...@kernel.org>
Reviewed-by: Paul Durrant <paul.durr...@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
[Qemu-devel] [PATCH v3 0/4] xen: don't save/restore the physmap on VM save/restore
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. --- Changes in v3: * Patch 3: use dummy flag based checks to gate ram_block_notify_* functions * Patch 3: switch to inline compat function instead of a straight define * Patch 4: add additional XEN_COMPAT_PHYSMAP blocks Changed in v2: * Patch 2: set dummy flag in a new flags field in struct MapCacheEntry * Patch 3: change xen_remap_cache_entry name and signature * Patch 3: gate ram_block_notify_* functions in xen_remap_bucket * Patch 3: rewrite the logic of xen_replace_cache_entry_unlocked to reuse the existing entry instead of allocating a new one * Patch 4: don't use xen_phys_offset_to_gaddr in non-compat mode --- Igor Druzhinin (4): xen: move physmap saving into a separate function xen/mapcache: add an ability to create dummy mappings xen/mapcache: introduce xen_replace_cache_entry() xen: don't use xenstore to save/restore physmap anymore configure | 18 +++ hw/i386/xen/xen-hvm.c | 105 +++- hw/i386/xen/xen-mapcache.c| 121 ++ include/hw/xen/xen_common.h | 15 ++ include/sysemu/xen-mapcache.h | 11 +++- 5 files changed, 222 insertions(+), 48 deletions(-) -- 2.7.4
[Qemu-devel] [PATCH v3 3/4] xen/mapcache: introduce xen_replace_cache_entry()
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 and maps again at the same place using
a new guest address. If the mapping is dummy this call will
make it real.
This function makes use of a new xenforeignmemory_map2() call
with an 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.druzhi...@citrix.com>
---
configure | 18 +
hw/i386/xen/xen-mapcache.c| 85 +++
include/hw/xen/xen_common.h | 14 +++
include/sysemu/xen-mapcache.h | 11 +-
4 files changed, 119 insertions(+), 9 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 <
+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 <
diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
index 39cb511..8bc63e0 100644
--- a/hw/i386/xen/xen-mapcache.c
+++ b/hw/i386/xen/xen-mapcache.c
@@ -151,6 +151,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)
@@ -167,7 +168,9 @@ static void xen_remap_bucket(MapCacheEntry *entry,
err = g_malloc0(nb_pfn * sizeof (int));
if (entry->vaddr_base != NULL) {
-ram_block_notify_remove(entry->vaddr_base, entry->size);
+if (!(entry->flags & XEN_MAPCACHE_ENTRY_DUMMY)) {
+ram_block_notify_remove(entry->vaddr_base, entry->size);
+}
if (munmap(entry->vaddr_base, entry->size) != 0) {
perror("unmap fails");
exit(-1);
@@ -181,11 +184,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 {
@@ -193,7 +196,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");
@@ -201,6 +204,10 @@ static void xen_remap_bucket(MapCacheEntry *entry,
}
}
+if (!(entry->flags & XEN_MAPCACHE_ENTRY_DUMMY)) {
+ram_block_notify_add(vaddr_base, size);
+}
+
entry->vaddr_base = vaddr_base;
entry->paddr_index = address_index;
entry->size = size;
@@ -213,7 +220,6 @@ static void xen_remap_bucket(MapCacheEntry *entry,
entry->flags &= ~(XEN_MAPCACHE_ENTRY_DUMMY);
}
-ram_block_notify_add(entry->vaddr_base, entry->size);
bitmap_zero(entry->valid_mapping, nb_pfn);
for (i = 0; i < nb_pfn; i++) {
if (!err[i]) {
@@ -286,14 +292,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
[Qemu-devel] [PATCH v3 4/4] xen: don't use xenstore to save/restore physmap anymore
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.druzhi...@citrix.com>
---
hw/i386/xen/xen-hvm.c | 48 ++---
hw/i386/xen/xen-mapcache.c | 4
include/hw/xen/xen_common.h | 1 +
3 files changed, 42 insertions(+), 11 deletions(-)
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index d259cf7..d24ca47 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -289,6 +289,7 @@ static XenPhysmap *get_physmapping(XenIOState *state,
return NULL;
}
+#ifdef XEN_COMPAT_PHYSMAP
static hwaddr xen_phys_offset_to_gaddr(hwaddr start_addr,
ram_addr_t size, void
*opaque)
{
@@ -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(>physmap, physmap, list);
+
+if (runstate_check(RUN_STATE_INMIGRATE)) {
+/* Now when we have a physmap entry we can replace a dummy mapping with
+ * a real one of guest foreign memory. */
+uint8_t *p = xen_replace_cache_entry(phys_offset, start_addr, 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,17 +409,6 @@ 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(>physmap, physmap, list);
-
xc_domain_pin_memory_cacheattr(xen_xc, xen_domid,
start_addr >> TARGET_PAGE_BITS,
(start_addr + size - 1) >> TARGET_PAGE_BITS,
@@ -1158,6 +1174,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 +1222,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)
{
@@ -1331,7 +1353,11 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion
**ram_memory)
state->bufioreq_local_port = rc;
/* Init RAM management */
+#ifdef XEN_COMPAT_PHYSMAP
xen_map_cache_init(xen_phys_offset_to_gaddr, state);
+#else
+xen_map_cache_init(NULL, state);
+#endif
xen_ram_init(pcms, ram_size, ram_memory);
qemu_add_vm_change_state_handler(xen_hvm_change_state_handler, state);
diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
index 8bc63e0..84cc4a2 100644
--- a/hw/i386/xen/xen-mapcache.c
+++ b/hw/i386/xen/xen-mapcache.c
@@ -239,7 +239,9 @@ static uint8_t *xen_map_cache_unlocked(hwaddr phys_addr,
hwaddr size,
hwaddr address_offset;
hwaddr cache_size = size;
hwaddr test_bit_size;
+#ifdef XEN_COMPAT_PHYSMAP
bool translated = false;
+#endif
bool dummy = false;
tryagain:
@@ -307,11 +309,13 @@ tryagain:
test_bit_size >> XC_PAGE_SHIFT,
entry->valid_mapping)) {
mapcache->last_entry = NULL;
+#ifdef XEN_COMPAT_PHYSMAP
if (!translated && mapcache->phys_offset_to_gaddr) {
phys_addr = mapcache->phys_offset_to_gaddr(phys_addr, size,
mapcache->opaque);
translated = true;
goto tryagain;
}
+#endif
if (!dummy && runstate_check(RUN_STATE_INMIGRATE)) {
dummy = true;
goto tryagain;
diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index e28ed48..86c7f26 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -80,6
Re: [Qemu-devel] [PULL for-2.10 6/7] xen/mapcache: introduce xen_replace_cache_entry()
On 21/07/17 14:50, Anthony PERARD wrote:
On Tue, Jul 18, 2017 at 03:22:41PM -0700, Stefano Stabellini wrote:
From: Igor Druzhinin <igor.druzhi...@citrix.com>
...
+static uint8_t *xen_replace_cache_entry_unlocked(hwaddr old_phys_addr,
+ hwaddr new_phys_addr,
+ hwaddr size)
+{
+MapCacheEntry *entry;
+hwaddr address_index, address_offset;
+hwaddr test_bit_size, cache_size = size;
+
+address_index = old_phys_addr >> MCACHE_BUCKET_SHIFT;
+address_offset = old_phys_addr & (MCACHE_BUCKET_SIZE - 1);
+
+assert(size);
+/* test_bit_size is always a multiple of XC_PAGE_SIZE */
+test_bit_size = size + (old_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);
+}
+
+entry = >entry[address_index % mapcache->nr_buckets];
+while (entry && !(entry->paddr_index == address_index &&
+ entry->size == cache_size)) {
+entry = entry->next;
+}
+if (!entry) {
+DPRINTF("Trying to update an entry for %lx " \
+"that is not in the mapcache!\n", old_phys_addr);
+return NULL;
+}
+
+address_index = new_phys_addr >> MCACHE_BUCKET_SHIFT;
+address_offset = new_phys_addr & (MCACHE_BUCKET_SIZE - 1);
+
+fprintf(stderr, "Replacing a dummy mapcache entry for %lx with %lx\n",
+old_phys_addr, new_phys_addr);
Looks likes this does not build on 32bits.
in:
http://logs.test-lab.xenproject.org/osstest/logs/112041/build-i386/6.ts-xen-build.log
/home/osstest/build.112041.build-i386/xen/tools/qemu-xen-dir/hw/i386/xen/xen-mapcache.c:
In function 'xen_replace_cache_entry_unlocked':
/home/osstest/build.112041.build-i386/xen/tools/qemu-xen-dir/hw/i386/xen/xen-mapcache.c:539:13:
error: format '%lx' expects argument of type 'long unsigned int', but argument
3 has type 'hwaddr' [-Werror=format=]
old_phys_addr, new_phys_addr);
^
/home/osstest/build.112041.build-i386/xen/tools/qemu-xen-dir/hw/i386/xen/xen-mapcache.c:539:13:
error: format '%lx' expects argument of type 'long unsigned int', but argument
4 has type 'hwaddr' [-Werror=format=]
cc1: all warnings being treated as errors
CC i386-softmmu/target/i386/gdbstub.o
/home/osstest/build.112041.build-i386/xen/tools/qemu-xen-dir/rules.mak:66:
recipe for target 'hw/i386/xen/xen-mapcache.o' failed
+
+xen_remap_bucket(entry, entry->vaddr_base,
+ 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 a mapcache entry for %lx!\n", old_phys_addr);
+return NULL;
+}
+
+return entry->vaddr_base + address_offset;
+}
+
Please, accept the attached patch to fix the issue.
Igor
>From 69a3afa453e283e92ddfd76109b203a20a02524c Mon Sep 17 00:00:00 2001
From: Igor Druzhinin <igor.druzhi...@citrix.com>
Date: Fri, 21 Jul 2017 19:27:41 +0100
Subject: [PATCH] xen: fix compilation on 32-bit hosts
Signed-off-by: Igor Druzhinin <igor.druzhi...@citrix.com>
---
hw/i386/xen/xen-mapcache.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
index 84cc4a2..540406a 100644
--- a/hw/i386/xen/xen-mapcache.c
+++ b/hw/i386/xen/xen-mapcache.c
@@ -529,7 +529,7 @@ static uint8_t *xen_replace_cache_entry_unlocked(hwaddr old_phys_addr,
entry = entry->next;
}
if (!entry) {
-DPRINTF("Trying to update an entry for %lx " \
+DPRINTF("Trying to update an entry for "TARGET_FMT_plx \
"that is not in the mapcache!\n", old_phys_addr);
return NULL;
}
@@ -537,15 +537,16 @@ static uint8_t *xen_replace_cache_entry_unlocked(hwaddr old_phys_addr,
address_index = new_phys_addr >> MCACHE_BUCKET_SHIFT;
address_offset = new_phys_addr & (MCACHE_BUCKET_SIZE - 1);
-fprintf(stderr, "Replacing a dummy mapcache entry for %lx with %lx\n",
-old_phys_addr, new_phys_addr);
+fprintf(stderr, "Replacing a dummy mapcache entry for "TARGET_FMT_plx \
+" with "TARGET_FMT_plx"\n", old_phys_addr, new_phys_addr);
xen_remap_bucket(entry, entry->vaddr_base,
cache_size, address_index, false);
if(!test_bits(address_offset >> XC_PAGE_SHIFT,
test_bit_size >> XC_PAGE_SHI
[Qemu-devel] [PATCH 3/4] xen/mapcache: introduce xen_remap_cache_entry()
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.druzhi...@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 <
+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 <
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 = >entry[ad
[Qemu-devel] [PATCH 2/4] xen/mapcache: add an ability to create dummy mappings
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.druzhi...@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
[Qemu-devel] [PATCH 0/4] xen: don't save/restore the physmap on VM save/restore
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
[Qemu-devel] [PATCH 4/4] xen: don't use xenstore to save/restore physmap anymore
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.druzhi...@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(>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(>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
[Qemu-devel] [PATCH 1/4] xen: move physmap saving into a separate function
Non-functional change.
Signed-off-by: Igor Druzhinin <igor.druzhi...@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
[Qemu-devel] [PATCH v2 4/4] xen: don't use xenstore to save/restore physmap anymore
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.druzhi...@citrix.com>
---
hw/i386/xen/xen-hvm.c | 48 ++---
include/hw/xen/xen_common.h | 1 +
2 files changed, 38 insertions(+), 11 deletions(-)
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index d259cf7..d24ca47 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -289,6 +289,7 @@ static XenPhysmap *get_physmapping(XenIOState *state,
return NULL;
}
+#ifdef XEN_COMPAT_PHYSMAP
static hwaddr xen_phys_offset_to_gaddr(hwaddr start_addr,
ram_addr_t size, void
*opaque)
{
@@ -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(>physmap, physmap, list);
+
+if (runstate_check(RUN_STATE_INMIGRATE)) {
+/* Now when we have a physmap entry we can replace a dummy mapping with
+ * a real one of guest foreign memory. */
+uint8_t *p = xen_replace_cache_entry(phys_offset, start_addr, 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,17 +409,6 @@ 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(>physmap, physmap, list);
-
xc_domain_pin_memory_cacheattr(xen_xc, xen_domid,
start_addr >> TARGET_PAGE_BITS,
(start_addr + size - 1) >> TARGET_PAGE_BITS,
@@ -1158,6 +1174,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 +1222,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)
{
@@ -1331,7 +1353,11 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion
**ram_memory)
state->bufioreq_local_port = rc;
/* Init RAM management */
+#ifdef XEN_COMPAT_PHYSMAP
xen_map_cache_init(xen_phys_offset_to_gaddr, state);
+#else
+xen_map_cache_init(NULL, state);
+#endif
xen_ram_init(pcms, ram_size, ram_memory);
qemu_add_vm_change_state_handler(xen_hvm_change_state_handler, state);
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
[Qemu-devel] [PATCH v2 0/4] xen: don't save/restore the physmap on VM save/restore
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. --- Changed in v2: * Patch 2: set dummy flag in a new flags field in struct MapCacheEntry * Patch 3: change xen_remap_cache_entry name and signature * Patch 3: gate ram_block_notify_* functions in xen_remap_bucket * Patch 3: rewrite the logic of xen_replace_cache_entry_unlocked to reuse the existing entry instead of allocating a new one * Patch 4: don't use xen_phys_offset_to_gaddr in non-compat mode --- Igor Druzhinin (4): xen: move physmap saving into a separate function xen/mapcache: add an ability to create dummy mappings xen/mapcache: introduce xen_replace_cache_entry() xen: don't use xenstore to save/restore physmap anymore configure | 18 +++ hw/i386/xen/xen-hvm.c | 105 ++--- hw/i386/xen/xen-mapcache.c| 107 ++ include/hw/xen/xen_common.h | 8 include/sysemu/xen-mapcache.h | 11 - 5 files changed, 201 insertions(+), 48 deletions(-) -- 2.7.4
Re: [Qemu-devel] [PATCH v2 3/4] xen/mapcache: introduce xen_replace_cache_entry()
On 04/07/17 17:27, Paul Durrant wrote:
>> -Original Message-
>> From: Igor Druzhinin
>> Sent: 04 July 2017 16:48
>> To: xen-de...@lists.xenproject.org; qemu-devel@nongnu.org
>> Cc: Igor Druzhinin <igor.druzhi...@citrix.com>; sstabell...@kernel.org;
>> Anthony Perard <anthony.per...@citrix.com>; Paul Durrant
>> <paul.durr...@citrix.com>; pbonz...@redhat.com
>> Subject: [PATCH v2 3/4] xen/mapcache: introduce
>> xen_replace_cache_entry()
>>
>> 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 and maps again at the same place using
>> a new guest address. If the mapping is dummy this call will
>> make it real.
>>
>> This function makes use of a new xenforeignmemory_map2() call
>> with an extended interface that was recently introduced in
>> libxenforeignmemory [1].
>
> I don't understand how the compat layer works here. If
> xenforeignmemory_map2() is not available then you can't control the placement
> in virtual address space.
>
If it's not 4.10 or newer xenforeignmemory_map2() doesn't exist and is
going to be defined as xenforeignmemory_map(). At the same time
XEN_COMPAT_PHYSMAP is defined and the entry replace function (which
relies on xenforeignmemory_map2 functionality) is never going to be called.
If you mean that I should incorporate this into the description I can do it.
Igor
> Paul
>
>>
>> [1] https://www.mail-archive.com/xen-devel@lists.xen.org/msg113007.html
>>
>> Signed-off-by: Igor Druzhinin <igor.druzhi...@citrix.com>
>> ---
>> configure | 18 ++
>> hw/i386/xen/xen-mapcache.c| 79
>> ++-
>> include/hw/xen/xen_common.h | 7
>> include/sysemu/xen-mapcache.h | 11 +-
>> 4 files changed, 106 insertions(+), 9 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 <> +#undef XC_WANT_COMPAT_MAP_FOREIGN_API
>> +#include
>> +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 <> #undef XC_WANT_COMPAT_DEVICEMODEL_API
>> #define __XEN_TOOLS__
>> #include
>> diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
>> index cd4e746..a988be7 100644
>> --- a/hw/i386/xen/xen-mapcache.c
>> +++ b/hw/i386/xen/xen-mapcache.c
>> @@ -151,6 +151,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)
>> @@ -167,7 +168,9 @@ static void xen_remap_bucket(MapCacheEntry
>> *entry,
>> err = g_malloc0(nb_pfn * sizeof (int));
>>
>> if (entry->vaddr_base != NULL) {
>> -ram_block_notify_remove(entry->vaddr_base, entry->size);
>> +if (entry->vaddr_base != vaddr) {
>> +ram_block_notify_remove(entry->vaddr_base, entry->size);
>> +}
>> if (munmap(entry->vaddr_base, entry->size) != 0) {
>> perror("unmap fails");
>> exit(-1);
>> @@ -181,11 +184,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_map
[Qemu-devel] [PATCH v2 2/4] xen/mapcache: add an ability to create dummy mappings
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.druzhi...@citrix.com>
---
hw/i386/xen/xen-mapcache.c | 40
1 file changed, 32 insertions(+), 8 deletions(-)
diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
index e60156c..cd4e746 100644
--- a/hw/i386/xen/xen-mapcache.c
+++ b/hw/i386/xen/xen-mapcache.c
@@ -53,6 +53,8 @@ typedef struct MapCacheEntry {
uint8_t *vaddr_base;
unsigned long *valid_mapping;
uint8_t lock;
+#define XEN_MAPCACHE_ENTRY_DUMMY (1 << 0)
+uint8_t flags;
hwaddr size;
struct MapCacheEntry *next;
} MapCacheEntry;
@@ -150,7 +152,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 +180,27 @@ 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);
+}
+entry->flags &= ~(XEN_MAPCACHE_ENTRY_DUMMY);
+} 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->flags |= XEN_MAPCACHE_ENTRY_DUMMY;
}
entry->vaddr_base = vaddr_base;
@@ -211,6 +230,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 +282,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 +302,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
[Qemu-devel] [PATCH v2 1/4] xen: move physmap saving into a separate function
Non-functional change.
Signed-off-by: Igor Druzhinin <igor.druzhi...@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
Re: [Qemu-devel] [PATCH v2 3/4] xen/mapcache: introduce xen_replace_cache_entry()
On 04/07/17 17:42, Paul Durrant wrote:
>> -Original Message-
>> From: Igor Druzhinin
>> Sent: 04 July 2017 17:34
>> To: Paul Durrant <paul.durr...@citrix.com>; xen-de...@lists.xenproject.org;
>> qemu-devel@nongnu.org
>> Cc: sstabell...@kernel.org; Anthony Perard <anthony.per...@citrix.com>;
>> pbonz...@redhat.com
>> Subject: Re: [PATCH v2 3/4] xen/mapcache: introduce
>> xen_replace_cache_entry()
>>
>> On 04/07/17 17:27, Paul Durrant wrote:
>>>> -Original Message-
>>>> From: Igor Druzhinin
>>>> Sent: 04 July 2017 16:48
>>>> To: xen-de...@lists.xenproject.org; qemu-devel@nongnu.org
>>>> Cc: Igor Druzhinin <igor.druzhi...@citrix.com>; sstabell...@kernel.org;
>>>> Anthony Perard <anthony.per...@citrix.com>; Paul Durrant
>>>> <paul.durr...@citrix.com>; pbonz...@redhat.com
>>>> Subject: [PATCH v2 3/4] xen/mapcache: introduce
>>>> xen_replace_cache_entry()
>>>>
>>>> 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 and maps again at the same place using
>>>> a new guest address. If the mapping is dummy this call will
>>>> make it real.
>>>>
>>>> This function makes use of a new xenforeignmemory_map2() call
>>>> with an extended interface that was recently introduced in
>>>> libxenforeignmemory [1].
>>>
>>> I don't understand how the compat layer works here. If
>> xenforeignmemory_map2() is not available then you can't control the
>> placement in virtual address space.
>>>
>>
>> If it's not 4.10 or newer xenforeignmemory_map2() doesn't exist and is
>> going to be defined as xenforeignmemory_map(). At the same time
>> XEN_COMPAT_PHYSMAP is defined and the entry replace function (which
>> relies on xenforeignmemory_map2 functionality) is never going to be called.
>>
>> If you mean that I should incorporate this into the description I can do it.
>
> AFAICT XEN_COMPAT_PHYSMAP is not introduced until patch #4 though.
>
> The problem really comes down to defining xenforeignmemory_map2() in terms of
> xenforeignmemory_map(). It basically can't be safely done. Could you define
> xenforeignmemory_map2() as abort() in the compat case instead?
>
xen_replace_cache_entry() is not called in patch #3. Which means it's
safe to use a fallback version (xenforeignmemory_map) in
xen_remap_bucket here.
Igor
> Paul
>
>>
>> Igor
>>
>>> Paul
>>>
>>>>
>>>> [1] https://www.mail-archive.com/xen-
>> de...@lists.xen.org/msg113007.html
>>>>
>>>> Signed-off-by: Igor Druzhinin <igor.druzhi...@citrix.com>
>>>> ---
>>>> configure | 18 ++
>>>> hw/i386/xen/xen-mapcache.c| 79
>>>> ++-
>>>> include/hw/xen/xen_common.h | 7
>>>> include/sysemu/xen-mapcache.h | 11 +-
>>>> 4 files changed, 106 insertions(+), 9 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 <>>> +#undef XC_WANT_COMPAT_MAP_FOREIGN_API
>>>> +#include
>>>> +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 <>>> #undef XC_WANT_COMPAT_DEVICEMODEL_API
>>>> #define __XEN_TOOLS__
>>>> #include
>>>> diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
>>>> index cd4e746..a988be7 100644
>>>> --- a/hw/i386/xen/xen-mapcache.c
>>>> +++ b/hw/i386/xen/xen-mapcache.c
>>>> @@ -151,
Re: [Qemu-devel] [PATCH 2/4] xen/mapcache: add an ability to create dummy mappings
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.druzhi...@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: [Qemu-devel] [PATCH 3/4] xen/mapcache: introduce xen_remap_cache_entry()
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.druzhi...@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 <> +#undef XC_WANT_COMPAT_MAP_FOREIGN_API
>> +#include
>> +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 <> #undef XC_WANT_COMPAT_DEVICEMODEL_API
>> #define __XEN_TOOLS__
>> #include
>> 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);
>> }
>> }
>>
>> @@
[Qemu-devel] [PATCH v2 3/4] xen/mapcache: introduce xen_replace_cache_entry()
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 and maps again at the same place using
a new guest address. If the mapping is dummy this call will
make it real.
This function makes use of a new xenforeignmemory_map2() call
with an 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.druzhi...@citrix.com>
---
configure | 18 ++
hw/i386/xen/xen-mapcache.c| 79 ++-
include/hw/xen/xen_common.h | 7
include/sysemu/xen-mapcache.h | 11 +-
4 files changed, 106 insertions(+), 9 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 <
+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 <
diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
index cd4e746..a988be7 100644
--- a/hw/i386/xen/xen-mapcache.c
+++ b/hw/i386/xen/xen-mapcache.c
@@ -151,6 +151,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)
@@ -167,7 +168,9 @@ static void xen_remap_bucket(MapCacheEntry *entry,
err = g_malloc0(nb_pfn * sizeof (int));
if (entry->vaddr_base != NULL) {
-ram_block_notify_remove(entry->vaddr_base, entry->size);
+if (entry->vaddr_base != vaddr) {
+ram_block_notify_remove(entry->vaddr_base, entry->size);
+}
if (munmap(entry->vaddr_base, entry->size) != 0) {
perror("unmap fails");
exit(-1);
@@ -181,11 +184,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);
}
entry->flags &= ~(XEN_MAPCACHE_ENTRY_DUMMY);
@@ -194,7 +197,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");
@@ -203,13 +206,16 @@ static void xen_remap_bucket(MapCacheEntry *entry,
entry->flags |= XEN_MAPCACHE_ENTRY_DUMMY;
}
+if (entry->vaddr_base == NULL || entry->vaddr_base != vaddr) {
+ram_block_notify_add(vaddr_base, size);
+}
+
entry->vaddr_base = vaddr_base;
entry->paddr_index = address_index;
entry->size = size;
entry->valid_mapping = (unsigned long *) g_malloc0(sizeof(unsigned long) *
BITS_TO_LONGS(size >> XC_PAGE_SHIFT));
-ram_block_notify_add(entry->vaddr_base, entry->size);
bitmap_zero(entry->valid_mapping, nb_pfn);
for (i = 0; i < nb_pfn; i++) {
if (!err[i]) {
@@ -282,14 +288,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);
+
[Qemu-devel] [PATCH] xen-pvdevice: Introduce a simplistic xen-pvdevice save state
This should help to avoid problems with accessing the device after
migration/resume without PV drivers. Older systems will acquire
the new record when migrated which should not change their state for
worse.
Signed-off-by: Igor Druzhinin <igor.druzhi...@citrix.com>
---
hw/i386/xen/xen_pvdevice.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/hw/i386/xen/xen_pvdevice.c b/hw/i386/xen/xen_pvdevice.c
index c093b34..ef22a03 100644
--- a/hw/i386/xen/xen_pvdevice.c
+++ b/hw/i386/xen/xen_pvdevice.c
@@ -71,6 +71,16 @@ static const MemoryRegionOps xen_pv_mmio_ops = {
.endianness = DEVICE_LITTLE_ENDIAN,
};
+static const VMStateDescription vmstate_xen_pvdevice = {
+.name = "xen-pvdevice",
+.version_id = 1,
+.minimum_version_id = 1,
+.fields = (VMStateField[]) {
+VMSTATE_PCI_DEVICE(parent_obj, XenPVDevice),
+VMSTATE_END_OF_LIST()
+}
+};
+
static void xen_pv_realize(PCIDevice *pci_dev, Error **errp)
{
XenPVDevice *d = XEN_PV_DEVICE(pci_dev);
@@ -120,6 +130,7 @@ static void xen_pv_class_init(ObjectClass *klass, void
*data)
k->class_id = PCI_CLASS_SYSTEM_OTHER;
dc->desc = "Xen PV Device";
dc->props = xen_pv_props;
+dc->vmsd = _xen_pvdevice;
}
static const TypeInfo xen_pv_type_info = {
--
2.7.4
[Qemu-devel] [PATCH v2] xen-pvdevice: Introduce a simplistic xen-pvdevice save state
This should help to avoid problems with accessing the device after
migration/resume without PV drivers by migrating its PCI configuration
space state. Without an explicitly defined state record it resets
every time a VM migrates which confuses the OS and makes every
access to xen-pvdevice MMIO region to fail. PV tools enable some
logic to save and restore PCI configuration state from within the VM
every time it migrates which basically hides the issue.
Older systems will acquire the new record when migrated which should
not change their state for worse.
Signed-off-by: Igor Druzhinin <igor.druzhi...@citrix.com>
Reviewed-by: Paul Durrant <paul.durr...@citrix.com>
---
v2: add more concrete info
---
hw/i386/xen/xen_pvdevice.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/hw/i386/xen/xen_pvdevice.c b/hw/i386/xen/xen_pvdevice.c
index f748823..a146f18 100644
--- a/hw/i386/xen/xen_pvdevice.c
+++ b/hw/i386/xen/xen_pvdevice.c
@@ -71,6 +71,16 @@ static const MemoryRegionOps xen_pv_mmio_ops = {
.endianness = DEVICE_LITTLE_ENDIAN,
};
+static const VMStateDescription vmstate_xen_pvdevice = {
+.name = "xen-pvdevice",
+.version_id = 1,
+.minimum_version_id = 1,
+.fields = (VMStateField[]) {
+VMSTATE_PCI_DEVICE(parent_obj, XenPVDevice),
+VMSTATE_END_OF_LIST()
+}
+};
+
static void xen_pv_realize(PCIDevice *pci_dev, Error **errp)
{
XenPVDevice *d = XEN_PV_DEVICE(pci_dev);
@@ -120,6 +130,7 @@ static void xen_pv_class_init(ObjectClass *klass, void
*data)
k->class_id = PCI_CLASS_SYSTEM_OTHER;
dc->desc = "Xen PV Device";
dc->props = xen_pv_props;
+dc->vmsd = _xen_pvdevice;
}
static const TypeInfo xen_pv_type_info = {
--
2.7.4
[Qemu-devel] [PATCH] xen/pt: use address_space_memory object for memory region hooks
Commit 99605175c (xen-pt: Fix PCI devices re-attach failed) introduced a subtle bug. As soon as the guest switches off Bus Mastering on the device it immediately causes all the BARs be unmapped due to the DMA address space of the device being changed. This is undesired behavior because the guest may try to communicate with the device after that which triggers the following errors in the logs: [00:05.0] xen_pt_bar_read: Error: Should not read BAR through QEMU. @0x0200 [00:05.0] xen_pt_bar_write: Error: Should not write BAR through QEMU. @0x0200 The issue that the original patch tried to workaround (uneven number of region_add/del calls on device attach/detach) was fixed in later QEMU versions. Signed-off-by: Igor Druzhinin <igor.druzhi...@citrix.com> Reported-by: Ross Lagerwall <ross.lagerw...@citrix.com> --- hw/xen/xen_pt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c index 9b7a960..e5a6eff 100644 --- a/hw/xen/xen_pt.c +++ b/hw/xen/xen_pt.c @@ -907,7 +907,7 @@ out: } } -memory_listener_register(>memory_listener, >dev.bus_master_as); +memory_listener_register(>memory_listener, _space_memory); memory_listener_register(>io_listener, _space_io); s->listener_set = true; XEN_PT_LOG(d, -- 2.7.4
Re: [Qemu-devel] [PATCH] xen/pt: use address_space_memory object for memory region hooks
On 17/04/18 15:15, Anthony PERARD wrote: > On Fri, Apr 06, 2018 at 10:21:23PM +0100, Igor Druzhinin wrote: >> Commit 99605175c (xen-pt: Fix PCI devices re-attach failed) introduced >> a subtle bug. As soon as the guest switches off Bus Mastering on the >> device it immediately causes all the BARs be unmapped due to the DMA >> address space of the device being changed. This is undesired behavior >> because the guest may try to communicate with the device after that >> which triggers the following errors in the logs: >> >> [00:05.0] xen_pt_bar_read: Error: Should not read BAR through QEMU. >> @0x0200 >> [00:05.0] xen_pt_bar_write: Error: Should not write BAR through QEMU. >> @0x0200 >> >> The issue that the original patch tried to workaround (uneven number of >> region_add/del calls on device attach/detach) was fixed in later QEMU >> versions. > > Do you know when the issue was fixed? > I think it's this commit: commit d25836cafd7508090d211e97acfc0abc5ae88daa Author: Peter Xu <pet...@redhat.com> Date: Mon Jan 22 14:02:44 2018 +0800 memory: do explicit cleanup when remove listeners Igor
Re: [Qemu-devel] [PATCH] xen/pt: use address_space_memory object for memory region hooks
On 17/04/18 15:15, Anthony PERARD wrote: > On Fri, Apr 06, 2018 at 10:21:23PM +0100, Igor Druzhinin wrote: >> Commit 99605175c (xen-pt: Fix PCI devices re-attach failed) introduced >> a subtle bug. As soon as the guest switches off Bus Mastering on the >> device it immediately causes all the BARs be unmapped due to the DMA >> address space of the device being changed. This is undesired behavior >> because the guest may try to communicate with the device after that >> which triggers the following errors in the logs: >> >> [00:05.0] xen_pt_bar_read: Error: Should not read BAR through QEMU. >> @0x0200 >> [00:05.0] xen_pt_bar_write: Error: Should not write BAR through QEMU. >> @0x0200 >> >> The issue that the original patch tried to workaround (uneven number of >> region_add/del calls on device attach/detach) was fixed in later QEMU >> versions. > > Do you know when the issue was fixed? > I haven't tracked down a particular version but the previous behavior of memory_listener_unregister() was to remove the listener from the list without calling the callback. It has changed since then and now the callback is called in listener_del_address_space(). Igor
[Qemu-devel] [PATCH v2] xen/pt: use address_space_memory object for memory region hooks
Commit 99605175c (xen-pt: Fix PCI devices re-attach failed) introduced a subtle bug. As soon as the guest switches off Bus Mastering on the device it immediately causes all the BARs be unmapped due to the DMA address space of the device being changed. This is undesired behavior because the guest may try to communicate with the device after that which triggers the following errors in the logs: [00:05.0] xen_pt_bar_read: Error: Should not read BAR through QEMU. @0x0200 [00:05.0] xen_pt_bar_write: Error: Should not write BAR through QEMU. @0x0200 The issue that the original patch tried to workaround (uneven number of region_add/del calls on device attach/detach) was fixed in d25836cafd (memory: do explicit cleanup when remove listeners). Signed-off-by: Igor Druzhinin <igor.druzhi...@citrix.com> Reported-by: Ross Lagerwall <ross.lagerw...@citrix.com> Acked-by: Anthony PERARD <anthony.per...@citrix.com> --- Changes in v2: * specify the exact fixing commit in the commit message --- hw/xen/xen_pt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c index 9b7a960..e5a6eff 100644 --- a/hw/xen/xen_pt.c +++ b/hw/xen/xen_pt.c @@ -907,7 +907,7 @@ out: } } -memory_listener_register(>memory_listener, >dev.bus_master_as); +memory_listener_register(>memory_listener, _space_memory); memory_listener_register(>io_listener, _space_io); s->listener_set = true; XEN_PT_LOG(d, -- 2.7.4
Re: [Qemu-devel] [PATCH] xen/pt: use address_space_memory object for memory region hooks
ping?
[Qemu-devel] [PATCH] xen/hvm: correct reporting of modified memory under physmap during migration
When global_log_dirty is enabled VRAM modification tracking never
worked correctly. The address that is passed to xen_hvm_modified_memory()
is not the effective PFN but RAM block address which is not the same
for VRAM.
We need to make a translation for this address into PFN using
physmap. Since there is no way to access physmap properly inside
xen_hvm_modified_memory() let's make it a global structure.
Signed-off-by: Igor Druzhinin <igor.druzhi...@citrix.com>
---
hw/i386/xen/xen-hvm.c | 37 +++--
hw/i386/xen/xen-mapcache.c| 2 +-
include/sysemu/xen-mapcache.h | 5 ++---
3 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index cb85541..0cc0124 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -89,6 +89,8 @@ typedef struct XenPhysmap {
QLIST_ENTRY(XenPhysmap) list;
} XenPhysmap;
+static QLIST_HEAD(, XenPhysmap) xen_physmap;
+
typedef struct XenIOState {
ioservid_t ioservid;
shared_iopage_t *shared_page;
@@ -109,7 +111,6 @@ typedef struct XenIOState {
MemoryListener memory_listener;
MemoryListener io_listener;
DeviceListener device_listener;
-QLIST_HEAD(, XenPhysmap) physmap;
hwaddr free_phys_offset;
const XenPhysmap *log_for_dirtybit;
@@ -276,14 +277,13 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
MemoryRegion *mr,
g_free(pfn_list);
}
-static XenPhysmap *get_physmapping(XenIOState *state,
- hwaddr start_addr, ram_addr_t size)
+static XenPhysmap *get_physmapping(hwaddr start_addr, ram_addr_t size)
{
XenPhysmap *physmap = NULL;
start_addr &= TARGET_PAGE_MASK;
-QLIST_FOREACH(physmap, >physmap, list) {
+QLIST_FOREACH(physmap, _physmap, list) {
if (range_covers_byte(physmap->start_addr, physmap->size, start_addr))
{
return physmap;
}
@@ -291,23 +291,21 @@ static XenPhysmap *get_physmapping(XenIOState *state,
return NULL;
}
-#ifdef XEN_COMPAT_PHYSMAP
-static hwaddr xen_phys_offset_to_gaddr(hwaddr start_addr,
- ram_addr_t size, void
*opaque)
+static hwaddr xen_phys_offset_to_gaddr(hwaddr phys_offset, ram_addr_t size)
{
-hwaddr addr = start_addr & TARGET_PAGE_MASK;
-XenIOState *xen_io_state = opaque;
+hwaddr addr = phys_offset & TARGET_PAGE_MASK;
XenPhysmap *physmap = NULL;
-QLIST_FOREACH(physmap, _io_state->physmap, list) {
+QLIST_FOREACH(physmap, _physmap, list) {
if (range_covers_byte(physmap->phys_offset, physmap->size, addr)) {
-return physmap->start_addr;
+return physmap->start_addr + (phys_offset - physmap->phys_offset);
}
}
-return start_addr;
+return phys_offset;
}
+#ifdef XEN_COMPAT_PHYSMAP
static int xen_save_physmap(XenIOState *state, XenPhysmap *physmap)
{
char path[80], value[17];
@@ -357,7 +355,7 @@ static int xen_add_to_physmap(XenIOState *state,
hwaddr phys_offset = memory_region_get_ram_addr(mr);
const char *mr_name;
-if (get_physmapping(state, start_addr, size)) {
+if (get_physmapping(start_addr, size)) {
return 0;
}
if (size <= 0) {
@@ -386,7 +384,7 @@ go_physmap:
physmap->name = mr_name;
physmap->phys_offset = phys_offset;
-QLIST_INSERT_HEAD(>physmap, physmap, list);
+QLIST_INSERT_HEAD(_physmap, physmap, list);
if (runstate_check(RUN_STATE_INMIGRATE)) {
/* Now when we have a physmap entry we can replace a dummy mapping with
@@ -425,7 +423,7 @@ static int xen_remove_from_physmap(XenIOState *state,
XenPhysmap *physmap = NULL;
hwaddr phys_offset = 0;
-physmap = get_physmapping(state, start_addr, size);
+physmap = get_physmapping(start_addr, size);
if (physmap == NULL) {
return -1;
}
@@ -593,7 +591,7 @@ static void xen_sync_dirty_bitmap(XenIOState *state,
int rc, i, j;
const XenPhysmap *physmap = NULL;
-physmap = get_physmapping(state, start_addr, size);
+physmap = get_physmapping(start_addr, size);
if (physmap == NULL) {
/* not handled */
return;
@@ -1219,7 +1217,7 @@ static void xen_read_physmap(XenIOState *state)
xen_domid, entries[i]);
physmap->name = xs_read(state->xenstore, 0, path, );
-QLIST_INSERT_HEAD(>physmap, physmap, list);
+QLIST_INSERT_HEAD(_physmap, physmap, list);
}
free(entries);
}
@@ -1356,7 +1354,6 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion
**ram_memory)
qemu_add_vm_change_state_handler(xen_hvm_change_state_handler, state);
state->memory_listener = xen_memory_listener;
-QLIST_INIT(>physmap);
memory_listener_register(>memory_listener, _space_memory);
state->log_for_dirtybit = NULL;
@@ -1372,6 +1369,8 @@ void xe
[Qemu-devel] [PATCH] block/nvme: call blk_drain in NVMe reset code to avoid lockups
When blk_flush called in NVMe reset path S/C queues are already freed
which means that re-entering AIO handling loop having some IO requests
unfinished will lockup or crash as their SG structures being potentially
reused. Call blk_drain before freeing the queues to avoid this nasty
scenario.
Signed-off-by: Igor Druzhinin
---
hw/block/nvme.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index fc7dacb..cdf836e 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -797,6 +797,8 @@ static void nvme_clear_ctrl(NvmeCtrl *n)
{
int i;
+blk_drain(n->conf.blk);
+
for (i = 0; i < n->num_queues; i++) {
if (n->sq[i] != NULL) {
nvme_free_sq(n->sq[i], n);
--
2.7.4
Re: [Qemu-devel] [PATCH] block/nvme: call blk_drain in NVMe reset code to avoid lockups
On 06/11/2018 12:16, Igor Druzhinin wrote:
> When blk_flush called in NVMe reset path S/C queues are already freed
> which means that re-entering AIO handling loop having some IO requests
> unfinished will lockup or crash as their SG structures being potentially
> reused. Call blk_drain before freeing the queues to avoid this nasty
> scenario.
>
> Signed-off-by: Igor Druzhinin
> ---
> hw/block/nvme.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index fc7dacb..cdf836e 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -797,6 +797,8 @@ static void nvme_clear_ctrl(NvmeCtrl *n)
> {
> int i;
>
> +blk_drain(n->conf.blk);
> +
> for (i = 0; i < n->num_queues; i++) {
> if (n->sq[i] != NULL) {
> nvme_free_sq(n->sq[i], n);
>
ping?
Re: [Qemu-devel] [PATCH] block/nvme: call blk_drain in NVMe reset code to avoid lockups
On 14/11/2018 17:42, Igor Druzhinin wrote:
> On 06/11/2018 12:16, Igor Druzhinin wrote:
>> When blk_flush called in NVMe reset path S/C queues are already freed
>> which means that re-entering AIO handling loop having some IO requests
>> unfinished will lockup or crash as their SG structures being potentially
>> reused. Call blk_drain before freeing the queues to avoid this nasty
>> scenario.
>>
>> Signed-off-by: Igor Druzhinin
>> ---
>> hw/block/nvme.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>> index fc7dacb..cdf836e 100644
>> --- a/hw/block/nvme.c
>> +++ b/hw/block/nvme.c
>> @@ -797,6 +797,8 @@ static void nvme_clear_ctrl(NvmeCtrl *n)
>> {
>> int i;
>>
>> +blk_drain(n->conf.blk);
>> +
>> for (i = 0; i < n->num_queues; i++) {
>> if (n->sq[i] != NULL) {
>> nvme_free_sq(n->sq[i], n);
>>
>
> ping?
>
CC: Paolo Bonzini
Re: [Qemu-devel] Commit 331b51 breaks live migration on FreeBSD/Xen dom0
On 14/03/2019 15:54, Roger Pau Monné wrote: > The log of the incoming QEMU is: > > qemu-system-i386: -serial pty: char device redirected to /dev/pts/4 (label > serial0) > xen: shared page at pfn feff0 > xen: buffered io page at pfn feff1 > xen: buffered io evtchn is 4 > xen_mapcache: xen_map_cache_init, nr_buckets = 8000 size 1572864 > xen_ram_alloc: do not alloc 1f80 bytes of ram at 0 when runstate is > INMIGRATE > xen_ram_alloc: do not alloc 80 bytes of ram at 1f80 when runstate is > INMIGRATE > xen_ram_alloc: do not alloc 1 bytes of ram at 2000 when runstate is > INMIGRATE > xen_ram_alloc: do not alloc 4 bytes of ram at 2001 when runstate is > INMIGRATE > VNC server running on 0.0.0.0:5900 > xen: mapping vram to f000 - f080 > Replacing a dummy mapcache entry for 1f80 with f000 > Assertion failed: (p && p == memory_region_get_ram_ptr(mr)), function > xen_add_to_physmap, file > /usr/ports/sysutils/xen-tools/work/xen-4.11.1/tools/qemu-xen/hw/i386/xen/xen-hvm.c, > line 392. > The change was supposed to be platform independent - it relies on the fact addr argument to mmap() works as expected - mmaps at the specified address. If this argument might be ignored on FreeBSD - that is a problem. Other then that the change was platform independent. You could also manually enable XEN_COMPAT_PHYSMAP while building QEMU (it's now gated on only Xen version) and see if it solves your problem. We may probably keep it enabled on FreeBSD all the time if there is no other way. Could you also print p and memory_region_get_ram_ptr(mr) so we could be sure it's mmap disregarding hint address? Igor
Re: [Qemu-devel] [PATCH v2] xen-mapcache: use MAP_FIXED flag so the mmap address hint is always honored
On 18/03/2019 15:45, Roger Pau Monne wrote:
> Or if it's not possible to honor the hinted address an error is returned
> instead. This makes it easier to spot the actual failure, instead of
> failing later on when the caller of xen_remap_bucket realizes the
> mapping has not been created at the requested address.
>
> Also note that at least on FreeBSD using MAP_FIXED will cause mmap to
> try harder to honor the passed address.
>
> Signed-off-by: Roger Pau Monné
> ---
> Cc: Stefano Stabellini
> Cc: Anthony Perard
> Cc: Paul Durrant
> Cc: Igor Druzhinin
> Cc: Paolo Bonzini
> Cc: Richard Henderson
> Cc: Eduardo Habkost
> Cc: "Michael S. Tsirkin"
> Cc: Marcel Apfelbaum
> Cc: xen-de...@lists.xenproject.org
> ---
> Changes since v1:
> - Use MAP_FIXED for the dummy mmap call also if a specific virtual
>address is requested.
> ---
> hw/i386/xen/xen-mapcache.c | 10 --
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
> index 349f72d00c..23de5517db 100644
> --- a/hw/i386/xen/xen-mapcache.c
> +++ b/hw/i386/xen/xen-mapcache.c
> @@ -185,8 +185,13 @@ static void xen_remap_bucket(MapCacheEntry *entry,
> }
>
> if (!dummy) {
> +/*
> + * If the caller has requested the mapping at a specific address use
> + * MAP_FIXED to make sure it's honored.
> + */
Since the comment now applied to both invocation - could it be moved
outside the if statement then?
Igor
Re: [Qemu-devel] [PATCH v3] xen-mapcache: use MAP_FIXED flag so the mmap address hint is always honored
On 18/03/2019 17:37, Roger Pau Monne wrote:
> Or if it's not possible to honor the hinted address an error is returned
> instead. This makes it easier to spot the actual failure, instead of
> failing later on when the caller of xen_remap_bucket realizes the
> mapping has not been created at the requested address.
>
> Also note that at least on FreeBSD using MAP_FIXED will cause mmap to
> try harder to honor the passed address.
>
> Signed-off-by: Roger Pau Monné
> Reviewed-by: Paul Durrant
> ---
> Cc: Stefano Stabellini
> Cc: Anthony Perard
> Cc: Paul Durrant
> Cc: Igor Druzhinin
> Cc: Paolo Bonzini
> Cc: Richard Henderson
> Cc: Eduardo Habkost
> Cc: "Michael S. Tsirkin"
> Cc: Marcel Apfelbaum
> Cc: xen-de...@lists.xenproject.org
> ---
> Changes since v2:
> - Move comment position.
>
> Changes since v1:
> - Use MAP_FIXED for the dummy mmap call also if a specific virtual
>address is requested.
> ---
> hw/i386/xen/xen-mapcache.c | 10 --
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
> index 349f72d00c..254759f776 100644
> --- a/hw/i386/xen/xen-mapcache.c
> +++ b/hw/i386/xen/xen-mapcache.c
> @@ -184,9 +184,14 @@ static void xen_remap_bucket(MapCacheEntry *entry,
> pfns[i] = (address_index << (MCACHE_BUCKET_SHIFT-XC_PAGE_SHIFT)) + i;
> }
>
> +/*
> + * If the caller has requested the mapping at a specific address use
> + * MAP_FIXED to make sure it's honored.
> + */
> if (!dummy) {
> vaddr_base = xenforeignmemory_map2(xen_fmem, xen_domid, vaddr,
> - PROT_READ | PROT_WRITE, 0,
> + PROT_READ | PROT_WRITE,
> + vaddr ? MAP_FIXED : 0,
> nb_pfn, pfns, err);
> if (vaddr_base == NULL) {
> perror("xenforeignmemory_map2");
> @@ -198,7 +203,8 @@ static void xen_remap_bucket(MapCacheEntry *entry,
> * mapping immediately due to certain circumstances (i.e. on resume
> now)
> */
> vaddr_base = mmap(vaddr, size, PROT_READ | PROT_WRITE,
> - MAP_ANON | MAP_SHARED, -1, 0);
> + MAP_ANON | MAP_SHARED | (vaddr ? MAP_FIXED : 0),
> + -1, 0);
> if (vaddr_base == MAP_FAILED) {
> perror("mmap");
> exit(-1);
>
Reviewed-by: Igor Druzhinin
Igor
Re: [Qemu-devel] [PATCH] xen-mapcache: use MAP_FIXED flag so the mmap address hint is always honored
On 15/03/2019 18:38, Anthony PERARD wrote:
> On Fri, Mar 15, 2019 at 06:14:09PM +, Anthony PERARD wrote:
>> On Fri, Mar 15, 2019 at 09:58:47AM +0100, Roger Pau Monne wrote:
>>> Or if it's not possible to honor the hinted address an error is returned
>>> instead. This makes it easier to spot the actual failure, instead of
>>> failing later on when the caller of xen_remap_bucket realizes the
>>> mapping has not been created at the requested address.
>>>
>>> Also note that at least on FreeBSD using MAP_FIXED will cause mmap to
>>> try harder to honor the passed address.
>>>
>>> Signed-off-by: Roger Pau Monné
>>
>> The patch looks fine, and MAP_FIXED seems to be the expected behavior
>> even on Linux.
>>
>> Acked-by: Anthony PERARD
>>
>>> diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
>>> @@ -185,8 +185,14 @@ static void xen_remap_bucket(MapCacheEntry *entry,
>>> }
>>>
>>> if (!dummy) {
>>> +/*
>>> + * If the caller has requested the mapping at a specific address
>>> use
>>> + * MAP_FIXED to make sure it's honored. Note that with MAP_FIXED at
>>> + * least FreeBSD will try harder to honor the passed address.
>>> + */
>>> vaddr_base = xenforeignmemory_map2(xen_fmem, xen_domid, vaddr,
>>> - PROT_READ | PROT_WRITE, 0,
>>> + PROT_READ | PROT_WRITE,
>>> + vaddr ? MAP_FIXED : 0,
>>> nb_pfn, pfns, err);
>
> I've noticed that there's a mmap call just after which has also vaddr as
> hint, should that second call also have MAP_FIXED as flags?
I think so, yes. The intended usage of xen_remap_bucket() is to create a
mapcache entry - either dummy or real. So this patch fixes the present
problem for real entry now but not for dummy. In future, there might be
xen_remap_bucket() calls to create dummy entries at a predefined
location so, I think, MAP_FIXED should be passed to mmap() call as well.
Igor
[Qemu-devel] [PATCH] xen: cleanup IOREQ server on exit
Device model is supposed to destroy IOREQ server for itself.
Signed-off-by: Igor Druzhinin
---
hw/i386/xen/xen-hvm.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index e8e79e0..30a5948 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1242,6 +1242,8 @@ static void xen_exit_notifier(Notifier *n, void *data)
{
XenIOState *state = container_of(n, XenIOState, exit);
+xen_destroy_ioreq_server(xen_domid, state->ioservid);
+
xenevtchn_close(state->xce_handle);
xs_daemon_close(state->xenstore);
}
--
2.7.4
[PATCH] xen-mapcache: avoid a race on memory map while using MAP_FIXED
When we're replacing the existing mapping there is possibility of a race
on memory map with other threads doing mmap operations - the address being
unmapped/re-mapped could be occupied by another thread in between.
Linux mmap man page recommends keeping the existing mappings in place to
reserve the place and instead utilize the fact that the next mmap operation
with MAP_FIXED flag passed will implicitly destroy the existing mappings
behind the chosen address. This behavior is guaranteed by POSIX / BSD and
therefore is portable.
Note that it wouldn't make the replacement atomic for parallel accesses to
the replaced region - those might still fail with SIGBUS due to
xenforeignmemory_map not being atomic. So we're still not expecting those.
Tested-by: Anthony PERARD
Signed-off-by: Igor Druzhinin
---
hw/i386/xen/xen-mapcache.c | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
index 5b120ed..e82b7dc 100644
--- a/hw/i386/xen/xen-mapcache.c
+++ b/hw/i386/xen/xen-mapcache.c
@@ -171,7 +171,20 @@ static void xen_remap_bucket(MapCacheEntry *entry,
if (!(entry->flags & XEN_MAPCACHE_ENTRY_DUMMY)) {
ram_block_notify_remove(entry->vaddr_base, entry->size);
}
-if (munmap(entry->vaddr_base, entry->size) != 0) {
+
+/*
+ * If an entry is being replaced by another mapping and we're using
+ * MAP_FIXED flag for it - there is possibility of a race for vaddr
+ * address with another thread doing an mmap call itself
+ * (see man 2 mmap). To avoid that we skip explicit unmapping here
+ * and allow the kernel to destroy the previous mappings by replacing
+ * them in mmap call later.
+ *
+ * Non-identical replacements are not allowed therefore.
+ */
+assert(!vaddr || (entry->vaddr_base == vaddr && entry->size == size));
+
+if (!vaddr && munmap(entry->vaddr_base, entry->size) != 0) {
perror("unmap fails");
exit(-1);
}
--
2.7.4
Re: [PATCH] xen-mapcache: avoid a race on memory map while using MAP_FIXED
On 20/04/2021 09:53, Roger Pau Monné wrote: On Tue, Apr 20, 2021 at 04:35:02AM +0100, Igor Druzhinin wrote: When we're replacing the existing mapping there is possibility of a race on memory map with other threads doing mmap operations - the address being unmapped/re-mapped could be occupied by another thread in between. Linux mmap man page recommends keeping the existing mappings in place to reserve the place and instead utilize the fact that the next mmap operation with MAP_FIXED flag passed will implicitly destroy the existing mappings behind the chosen address. This behavior is guaranteed by POSIX / BSD and therefore is portable. Note that it wouldn't make the replacement atomic for parallel accesses to the replaced region - those might still fail with SIGBUS due to xenforeignmemory_map not being atomic. So we're still not expecting those. Tested-by: Anthony PERARD Signed-off-by: Igor Druzhinin Should we add a 'Fixes: 759235653de ...' or similar tag here? I was thinking about it and decided - no. There wasn't a bug here until QEMU introduced usage of iothreads at the state loading phase. Originally this process was totally single-threaded. And it's hard to pinpoint the exact moment when it happened which is also not directly related to the change here. Igor
Re: [PATCH] xen-mapcache: avoid a race on memory map while using MAP_FIXED
On 20/04/2021 04:39, no-re...@patchew.org wrote: Patchew URL: https://patchew.org/QEMU/1618889702-13104-1-git-send-email-igor.druzhi...@citrix.com/ Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 1618889702-13104-1-git-send-email-igor.druzhi...@citrix.com Subject: [PATCH] xen-mapcache: avoid a race on memory map while using MAP_FIXED === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/1618889702-13104-1-git-send-email-igor.druzhi...@citrix.com -> patchew/1618889702-13104-1-git-send-email-igor.druzhi...@citrix.com Switched to a new branch 'test' 3102519 xen-mapcache: avoid a race on memory map while using MAP_FIXED === OUTPUT BEGIN === ERROR: Author email address is mangled by the mailing list #2: Author: Igor Druzhinin via total: 1 errors, 0 warnings, 21 lines checked Anthony, Is there any action required from me here? I don't completely understand how that happened. But I found this: "In some cases the Author: email address in patches submitted to the list gets mangled such that it says John Doe via Qemu-devel This change is a result of workarounds for DMARC policies. Subsystem maintainers accepting patches need to catch these and fix them before sending pull requests, so a checkpatch.pl test is highly desirable." Igor
