On the POWER9 processor, the XIVE interrupt controller can control
interrupt sources using MMIO to trigger events, to EOI or to turn off
the sources. Priority management and interrupt acknowledgment is also
controlled by MMIO in the presenter sub-engine.
These MMIO regions are exposed to guests in QEMU with a set of 'ram
device' memory mappings, similarly to VFIO, and the VMAs are populated
dynamically with the appropriate pages using a fault handler.
But, these regions are an issue for migration. We need to discard the
associated RAMBlocks from the RAM state on the source VM and let the
destination VM rebuild the memory mappings on the new host in the
post_load() operation just before resuming the system.
To achieve this goal, the following introduces a new helper,
ram_block_is_migratable(), which identifies RAMBlocks to discard on
the source. Some checks are also performed on the destination to make
sure nothing invalid was sent.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
I am not sure we want to taker into account patchew complaint :
ERROR: braces {} are necessary for all arms of this statement
#52: FILE: migration/ram.c:203:
+ if (ram_block_is_migratable(block))
[...]
total: 1 errors, 0 warnings, 136 lines checked
migration/ram.c | 52 ++++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 42 insertions(+), 10 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 0e90efa09236..32371950865b 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -188,6 +188,21 @@ void ramblock_recv_bitmap_set_range(RAMBlock *rb, void *host_addr,
}
/*
+ * Identifies RAM blocks which should be discarded from migration. For
+ * the moment, it only applies to blocks backed by a 'ram_device'
+ * memory region.
+ */
+static inline bool ram_block_is_migratable(RAMBlock *block)
+{
+ return !memory_region_is_ram_device(block->mr);
+}
+
+/* Should be holding either ram_list.mutex, or the RCU lock. */
+#define RAMBLOCK_FOREACH_MIGRATABLE(block) \
+ RAMBLOCK_FOREACH(block) \
+ if (ram_block_is_migratable(block))
+
+/*
* An outstanding page request, on the source, having been received
* and queued
*/
@@ -780,6 +795,10 @@ unsigned long migration_bitmap_find_dirty(RAMState *rs, RAMBlock *rb,
unsigned long *bitmap = rb->bmap;
unsigned long next;
+ if (!ram_block_is_migratable(rb)) {
+ return size;
+ }
+
if (rs->ram_bulk_stage && start > 0) {
next = start + 1;
} else {
@@ -825,7 +844,7 @@ uint64_t ram_pagesize_summary(void)
RAMBlock *block;
uint64_t summary = 0;
- RAMBLOCK_FOREACH(block) {
+ RAMBLOCK_FOREACH_MIGRATABLE(block) {
summary |= block->page_size;
}
@@ -849,7 +868,7 @@ static void migration_bitmap_sync(RAMState *rs)
qemu_mutex_lock(&rs->bitmap_mutex);
rcu_read_lock();
- RAMBLOCK_FOREACH(block) {
+ RAMBLOCK_FOREACH_MIGRATABLE(block) {
migration_bitmap_sync_range(rs, block, 0, block->used_length);
}
rcu_read_unlock();
@@ -1499,6 +1518,10 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
size_t pagesize_bits =
qemu_ram_pagesize(pss->block) >> TARGET_PAGE_BITS;
+ if (!ram_block_is_migratable(pss->block)) {
+ return 0;
+ }
+
do {
tmppages = ram_save_target_page(rs, pss, last_stage);
if (tmppages < 0) {
@@ -1587,7 +1610,7 @@ uint64_t ram_bytes_total(void)
uint64_t total = 0;
rcu_read_lock();
- RAMBLOCK_FOREACH(block) {
+ RAMBLOCK_FOREACH_MIGRATABLE(block) {
total += block->used_length;
}
rcu_read_unlock();
@@ -1642,7 +1665,7 @@ static void ram_save_cleanup(void *opaque)
*/
memory_global_dirty_log_stop();
- QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
+ RAMBLOCK_FOREACH_MIGRATABLE(block) {
g_free(block->bmap);
block->bmap = NULL;
g_free(block->unsentmap);
@@ -1705,7 +1728,7 @@ void ram_postcopy_migrated_memory_release(MigrationState *ms)
{
struct RAMBlock *block;
- RAMBLOCK_FOREACH(block) {
+ RAMBLOCK_FOREACH_MIGRATABLE(block) {
unsigned long *bitmap = block->bmap;
unsigned long range = block->used_length >> TARGET_PAGE_BITS;
unsigned long run_start = find_next_zero_bit(bitmap, range, 0);
@@ -1783,7 +1806,7 @@ static int postcopy_each_ram_send_discard(MigrationState *ms)
struct RAMBlock *block;
int ret;
- RAMBLOCK_FOREACH(block) {
+ RAMBLOCK_FOREACH_MIGRATABLE(block) {
PostcopyDiscardState *pds =
postcopy_discard_send_init(ms, block->idstr);
@@ -1991,7 +2014,7 @@ int ram_postcopy_send_discard_bitmap(MigrationState *ms)
rs->last_sent_block = NULL;
rs->last_page = 0;
- QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
+ RAMBLOCK_FOREACH_MIGRATABLE(block) {
unsigned long pages = block->used_length >> TARGET_PAGE_BITS;
unsigned long *bitmap = block->bmap;
unsigned long *unsentmap = block->unsentmap;
@@ -2150,7 +2173,7 @@ static void ram_list_init_bitmaps(void)
/* Skip setting bitmap if there is no RAM */
if (ram_bytes_total()) {
- QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
+ RAMBLOCK_FOREACH_MIGRATABLE(block) {
pages = block->max_length >> TARGET_PAGE_BITS;
block->bmap = bitmap_new(pages);
bitmap_set(block->bmap, 0, pages);
@@ -2226,7 +2249,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
qemu_put_be64(f, ram_bytes_total() | RAM_SAVE_FLAG_MEM_SIZE);
- RAMBLOCK_FOREACH(block) {
+ RAMBLOCK_FOREACH_MIGRATABLE(block) {
qemu_put_byte(f, strlen(block->idstr));
qemu_put_buffer(f, (uint8_t *)block->idstr, strlen(block->idstr));
qemu_put_be64(f, block->used_length);
@@ -2471,6 +2494,11 @@ static inline RAMBlock *ram_block_from_stream(QEMUFile *f, int flags)
return NULL;
}
+ if (!ram_block_is_migratable(block)) {
+ error_report("block %s should not be migrated !", id);
+ return NULL;
+ }
+
return block;
}
@@ -2921,7 +2949,11 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
length = qemu_get_be64(f);
block = qemu_ram_block_by_name(id);
- if (block) {
+ if (block && !ram_block_is_migratable(block)) {
+ error_report("block %s should not be migrated !", id);
+ ret = -EINVAL;
+
+ } else if (block) {
if (length != block->used_length) {
Error *local_err = NULL;
--
2.13.6
On 12 April 2018 at 11:18, Cédric Le Goater <clg@kaod.org> wrote:
> On the POWER9 processor, the XIVE interrupt controller can control
> interrupt sources using MMIO to trigger events, to EOI or to turn off
> the sources. Priority management and interrupt acknowledgment is also
> controlled by MMIO in the presenter sub-engine.
>
> These MMIO regions are exposed to guests in QEMU with a set of 'ram
> device' memory mappings, similarly to VFIO, and the VMAs are populated
> dynamically with the appropriate pages using a fault handler.
>
> But, these regions are an issue for migration. We need to discard the
> associated RAMBlocks from the RAM state on the source VM and let the
> destination VM rebuild the memory mappings on the new host in the
> post_load() operation just before resuming the system.
>
> To achieve this goal, the following introduces a new helper,
> ram_block_is_migratable(), which identifies RAMBlocks to discard on
> the source. Some checks are also performed on the destination to make
> sure nothing invalid was sent.
David suggested on IRC that we would want a flag on the ramblock
for "not migratable", because there are other uses for "don't
migrate this" than just "is this a ram device".
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>
> I am not sure we want to taker into account patchew complaint :
>
> ERROR: braces {} are necessary for all arms of this statement
> #52: FILE: migration/ram.c:203:
> + if (ram_block_is_migratable(block))
> [...]
>
> total: 1 errors, 0 warnings, 136 lines checked
>
> migration/ram.c | 52 ++++++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 42 insertions(+), 10 deletions(-)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 0e90efa09236..32371950865b 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -188,6 +188,21 @@ void ramblock_recv_bitmap_set_range(RAMBlock *rb, void *host_addr,
> }
>
> /*
> + * Identifies RAM blocks which should be discarded from migration. For
> + * the moment, it only applies to blocks backed by a 'ram_device'
> + * memory region.
> + */
> +static inline bool ram_block_is_migratable(RAMBlock *block)
> +{
> + return !memory_region_is_ram_device(block->mr);
> +}
> +
> +/* Should be holding either ram_list.mutex, or the RCU lock. */
> +#define RAMBLOCK_FOREACH_MIGRATABLE(block) \
> + RAMBLOCK_FOREACH(block) \
> + if (ram_block_is_migratable(block))
This will mishandle some uses, like:
if (foo)
RAMBLOCK_FOREACH_MIGRATABLE(block)
stuff;
else
morestuff;
as the if() inside the macro will capture the else clause.
(The lack of braces in the calling code would be against our
coding style, of course, so not very likely.)
Eric, is there a 'standard' trick for this? I thought of
maybe
#define RAMBLOCK_FOREACH_MIGRATABLE(block) \
RAMBLOCK_FOREACH(block) \
if (!ram_block_is_migratable(block)) {} else
?
thanks
-- PMM
* Peter Maydell (peter.maydell@linaro.org) wrote:
> On 12 April 2018 at 11:18, Cédric Le Goater <clg@kaod.org> wrote:
> > On the POWER9 processor, the XIVE interrupt controller can control
> > interrupt sources using MMIO to trigger events, to EOI or to turn off
> > the sources. Priority management and interrupt acknowledgment is also
> > controlled by MMIO in the presenter sub-engine.
> >
> > These MMIO regions are exposed to guests in QEMU with a set of 'ram
> > device' memory mappings, similarly to VFIO, and the VMAs are populated
> > dynamically with the appropriate pages using a fault handler.
> >
> > But, these regions are an issue for migration. We need to discard the
> > associated RAMBlocks from the RAM state on the source VM and let the
> > destination VM rebuild the memory mappings on the new host in the
> > post_load() operation just before resuming the system.
> >
> > To achieve this goal, the following introduces a new helper,
> > ram_block_is_migratable(), which identifies RAMBlocks to discard on
> > the source. Some checks are also performed on the destination to make
> > sure nothing invalid was sent.
>
> David suggested on IRC that we would want a flag on the ramblock
> for "not migratable", because there are other uses for "don't
> migrate this" than just "is this a ram device".
My original suggestion to your series was with a flag, but I'd forgotten
about that by the time I'd made the suggestion to Cédric.
In your case would just adding an extra term to the
ram_block_is_migratable function work, or do you really need a flag?
Dave
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > ---
> >
> > I am not sure we want to taker into account patchew complaint :
> >
> > ERROR: braces {} are necessary for all arms of this statement
> > #52: FILE: migration/ram.c:203:
> > + if (ram_block_is_migratable(block))
> > [...]
> >
> > total: 1 errors, 0 warnings, 136 lines checked
> >
> > migration/ram.c | 52 ++++++++++++++++++++++++++++++++++++++++++----------
> > 1 file changed, 42 insertions(+), 10 deletions(-)
> >
> > diff --git a/migration/ram.c b/migration/ram.c
> > index 0e90efa09236..32371950865b 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -188,6 +188,21 @@ void ramblock_recv_bitmap_set_range(RAMBlock *rb, void *host_addr,
> > }
> >
> > /*
> > + * Identifies RAM blocks which should be discarded from migration. For
> > + * the moment, it only applies to blocks backed by a 'ram_device'
> > + * memory region.
> > + */
> > +static inline bool ram_block_is_migratable(RAMBlock *block)
> > +{
> > + return !memory_region_is_ram_device(block->mr);
> > +}
> > +
> > +/* Should be holding either ram_list.mutex, or the RCU lock. */
> > +#define RAMBLOCK_FOREACH_MIGRATABLE(block) \
> > + RAMBLOCK_FOREACH(block) \
> > + if (ram_block_is_migratable(block))
>
> This will mishandle some uses, like:
>
> if (foo)
> RAMBLOCK_FOREACH_MIGRATABLE(block)
> stuff;
> else
> morestuff;
>
> as the if() inside the macro will capture the else clause.
> (The lack of braces in the calling code would be against our
> coding style, of course, so not very likely.)
>
> Eric, is there a 'standard' trick for this? I thought of
> maybe
>
> #define RAMBLOCK_FOREACH_MIGRATABLE(block) \
> RAMBLOCK_FOREACH(block) \
> if (!ram_block_is_migratable(block)) {} else
>
> ?
>
> thanks
> -- PMM
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 12 April 2018 at 12:53, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > * Peter Maydell (peter.maydell@linaro.org) wrote: >> David suggested on IRC that we would want a flag on the ramblock >> for "not migratable", because there are other uses for "don't >> migrate this" than just "is this a ram device". > > My original suggestion to your series was with a flag, but I'd forgotten > about that by the time I'd made the suggestion to Cédric. > In your case would just adding an extra term to the > ram_block_is_migratable function work, or do you really need a flag? I don't see how else you would identify the ram block that needs to be skipped. Also I think it's just better design to decouple the decision about "should we migrate this ram block" from the migration code itself, and push it up to the code layer that knows it's creating ram blocks that shouldn't be migrated. thanks -- PMM
On 04/12/2018 02:08 PM, Peter Maydell wrote: > On 12 April 2018 at 12:53, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: >> * Peter Maydell (peter.maydell@linaro.org) wrote: >>> David suggested on IRC that we would want a flag on the ramblock >>> for "not migratable", because there are other uses for "don't >>> migrate this" than just "is this a ram device". >> >> My original suggestion to your series was with a flag, but I'd forgotten >> about that by the time I'd made the suggestion to Cédric. >> In your case would just adding an extra term to the >> ram_block_is_migratable function work, or do you really need a flag? > > I don't see how else you would identify the ram block that needs > to be skipped. Also I think it's just better design to decouple > the decision about "should we migrate this ram block" from the > migration code itself, and push it up to the code layer that knows > it's creating ram blocks that shouldn't be migrated. Do you mean adding a new RAMBlock flag RAM_NON_MIGRATABLE in exec.c ? That would require to add an extra bool to the following functions : memory_region_init_ram_ptr() qemu_ram_alloc_from_ptr() qemu_ram_alloc_internal() Would that be ok ? Thanks, C.
On 12 April 2018 at 14:41, Cédric Le Goater <clg@kaod.org> wrote: > On 04/12/2018 02:08 PM, Peter Maydell wrote: >> On 12 April 2018 at 12:53, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: >>> * Peter Maydell (peter.maydell@linaro.org) wrote: >>>> David suggested on IRC that we would want a flag on the ramblock >>>> for "not migratable", because there are other uses for "don't >>>> migrate this" than just "is this a ram device". >>> >>> My original suggestion to your series was with a flag, but I'd forgotten >>> about that by the time I'd made the suggestion to Cédric. >>> In your case would just adding an extra term to the >>> ram_block_is_migratable function work, or do you really need a flag? >> >> I don't see how else you would identify the ram block that needs >> to be skipped. Also I think it's just better design to decouple >> the decision about "should we migrate this ram block" from the >> migration code itself, and push it up to the code layer that knows >> it's creating ram blocks that shouldn't be migrated. > > Do you mean adding a new RAMBlock flag RAM_NON_MIGRATABLE > in exec.c ? That would require to add an extra bool to the > following functions : > > memory_region_init_ram_ptr() > qemu_ram_alloc_from_ptr() > qemu_ram_alloc_internal() > > Would that be ok ? Paolo may have an opinion what the API here should be, but at the MemoryRegion level we already have a mix of functions memory_region_init_foo_nomigrate() and memory_region_init_foo(), which at the moment just control whether we call vmstate_register_ram() or not. Is it possible to hook into that, so that we default to marking RAMBlocks as not-migrated, and when vmstate_register_ram() is called we set the flag to "migrated"? NB: this probably requires careful thought to make sure we don't accidentally break migration compat, but it seems like the cleaner approach. At the moment we do weird things if you migrate a RAM block that hasn't had its ID string set, IIRC, so just not migrating those RAM blocks seems like a better plan than mishandling them. It would also mean that the vmstate_register/unregister_ram() functions did what they claim to do based on the function name, I think. thanks -- PMM
On 12/04/2018 15:51, Peter Maydell wrote: > Paolo may have an opinion what the API here should be, but > at the MemoryRegion level we already have a mix of functions > memory_region_init_foo_nomigrate() and memory_region_init_foo(), > which at the moment just control whether we call > vmstate_register_ram() or not. Is it possible to hook into that, > so that we default to marking RAMBlocks as not-migrated, and > when vmstate_register_ram() is called we set the flag to > "migrated"? Yes, that sounds pretty. Paolo > NB: this probably requires careful thought to make sure we > don't accidentally break migration compat, but it seems like > the cleaner approach. At the moment we do weird things if you > migrate a RAM block that hasn't had its ID string set, IIRC, > so just not migrating those RAM blocks seems like a better > plan than mishandling them. It would also mean that the > vmstate_register/unregister_ram() functions did what they > claim to do based on the function name, I think.
On 04/12/2018 04:11 PM, Paolo Bonzini wrote:
> On 12/04/2018 15:51, Peter Maydell wrote:
>> Paolo may have an opinion what the API here should be, but
>> at the MemoryRegion level we already have a mix of functions
>> memory_region_init_foo_nomigrate() and memory_region_init_foo(),
>> which at the moment just control whether we call
>> vmstate_register_ram() or not. Is it possible to hook into that,
>> so that we default to marking RAMBlocks as not-migrated, and
>> when vmstate_register_ram() is called we set the flag to
>> "migrated"?
>
> Yes, that sounds pretty.
I have added the MIGRATABLE flag un/setting under qemu_ram_un/set_idstr()
which are the only routines called by vmstate_un/register_ram().
Tell me if that's fine or if you'd rather have helpers to
set MIGRATABLE from savem ? below is some code extract.
Thanks,
C.
>> NB: this probably requires careful thought to make sure we
>> don't accidentally break migration compat, but it seems like
>> the cleaner approach. At the moment we do weird things if you
>> migrate a RAM block that hasn't had its ID string set, IIRC,
>> so just not migrating those RAM blocks seems like a better
>> plan than mishandling them. It would also mean that the
>> vmstate_register/unregister_ram() functions did what they
>> claim to do based on the function name, I think.
--- qemu-xive.git.orig/exec.c
+++ qemu-xive.git/exec.c
@@ -104,6 +104,9 @@ static MemoryRegion io_mem_unassigned;
* (Set during postcopy)
*/
#define RAM_UF_ZEROPAGE (1 << 3)
+
+/* RAM can be migrated */
+#define RAM_MIGRATABLE (1 << 4)
#endif
#ifdef TARGET_PAGE_BITS_VARY
@@ -1807,6 +1810,11 @@ void qemu_ram_set_uf_zeroable(RAMBlock *
rb->flags |= RAM_UF_ZEROPAGE;
}
+bool qemu_ram_is_migratable(RAMBlock *rb)
+{
+ return rb->flags & RAM_MIGRATABLE;
+}
+
/* Called with iothread lock held. */
void qemu_ram_set_idstr(RAMBlock *new_block, const char *name, DeviceState *dev)
{
@@ -1823,6 +1831,7 @@ void qemu_ram_set_idstr(RAMBlock *new_bl
}
}
pstrcat(new_block->idstr, sizeof(new_block->idstr), name);
+ new_block->flags |= RAM_MIGRATABLE;
rcu_read_lock();
RAMBLOCK_FOREACH(block) {
@@ -1845,6 +1854,7 @@ void qemu_ram_unset_idstr(RAMBlock *bloc
*/
if (block) {
memset(block->idstr, 0, sizeof(block->idstr));
+ block->flags &= ~RAM_MIGRATABLE;
}
}
On 04/12/2018 06:47 AM, Peter Maydell wrote:
>> /*
>> + * Identifies RAM blocks which should be discarded from migration. For
>> + * the moment, it only applies to blocks backed by a 'ram_device'
>> + * memory region.
>> + */
>> +static inline bool ram_block_is_migratable(RAMBlock *block)
>> +{
>> + return !memory_region_is_ram_device(block->mr);
>> +}
>> +
>> +/* Should be holding either ram_list.mutex, or the RCU lock. */
>> +#define RAMBLOCK_FOREACH_MIGRATABLE(block) \
>> + RAMBLOCK_FOREACH(block) \
>> + if (ram_block_is_migratable(block))
>
> This will mishandle some uses, like:
>
> if (foo)
> RAMBLOCK_FOREACH_MIGRATABLE(block)
> stuff;
> else
> morestuff;
>
> as the if() inside the macro will capture the else clause.
> (The lack of braces in the calling code would be against our
> coding style, of course, so not very likely.)
All existing callers of RAMBLOCK_FOREACH() already supply their own {}
around stuff (same is true for QLIST_FOREACH_RCU(), which is what is
being used under the hood). By the time you correct that to:
if (foo)
RAMBLOCK_FOREACH_MIGRATABLE(block) {
stuff;
}
else
morestuff;
then even though the outer if violates coding standard, the correct
usage of the macro with {} around stuff won't leak that there is a
trailing if.
But yeah, if you're worrying about code that omitted {}, then dealing
with omitted {} in both places is something to think about.
>
> Eric, is there a 'standard' trick for this? I thought of
> maybe
>
> #define RAMBLOCK_FOREACH_MIGRATABLE(block) \
> RAMBLOCK_FOREACH(block) \
> if (!ram_block_is_migratable(block)) {} else
at which point, yes, this is a little bit safer for taking exactly one
statement without risking that a later bare 'else' could match to the
wrong unbraced 'if'. I'm not coming up with any better ideas for
(abusing?) the syntax.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
© 2016 - 2024 Red Hat, Inc.