Here is some context for this strange change request.

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 subengine.

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.

This is the goal of the following proposal. Does it make sense ? It
seems to be working enough to migrate a running guest but there might
be a better, more subtle, approach.

Thanks,

C.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 migration/ram.c | 42 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 0e90efa09236..6404ccd046d8 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -780,6 +780,10 @@ unsigned long migration_bitmap_find_dirty(RAMState *rs, RAMBlock *rb,
     unsigned long *bitmap = rb->bmap;
     unsigned long next;
 
+    if (memory_region_is_ram_device(rb->mr)) {
+        return size;
+    }
+
     if (rs->ram_bulk_stage && start > 0) {
         next = start + 1;
     } else {
@@ -826,6 +830,9 @@ uint64_t ram_pagesize_summary(void)
     uint64_t summary = 0;
 
     RAMBLOCK_FOREACH(block) {
+        if (memory_region_is_ram_device(block->mr)) {
+            continue;
+        }
         summary |= block->page_size;
     }
 
@@ -850,6 +857,9 @@ static void migration_bitmap_sync(RAMState *rs)
     qemu_mutex_lock(&rs->bitmap_mutex);
     rcu_read_lock();
     RAMBLOCK_FOREACH(block) {
+        if (memory_region_is_ram_device(block->mr)) {
+            continue;
+        }
         migration_bitmap_sync_range(rs, block, 0, block->used_length);
     }
     rcu_read_unlock();
@@ -1499,6 +1509,10 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
     size_t pagesize_bits =
         qemu_ram_pagesize(pss->block) >> TARGET_PAGE_BITS;
 
+    if (memory_region_is_ram_device(pss->block->mr)) {
+        return 0;
+    }
+
     do {
         tmppages = ram_save_target_page(rs, pss, last_stage);
         if (tmppages < 0) {
@@ -1588,6 +1602,9 @@ uint64_t ram_bytes_total(void)
 
     rcu_read_lock();
     RAMBLOCK_FOREACH(block) {
+        if (memory_region_is_ram_device(block->mr)) {
+            continue;
+        }
         total += block->used_length;
     }
     rcu_read_unlock();
@@ -1643,6 +1660,9 @@ static void ram_save_cleanup(void *opaque)
     memory_global_dirty_log_stop();
 
     QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
+        if (memory_region_is_ram_device(block->mr)) {
+            continue;
+        }
         g_free(block->bmap);
         block->bmap = NULL;
         g_free(block->unsentmap);
@@ -1710,6 +1730,9 @@ void ram_postcopy_migrated_memory_release(MigrationState *ms)
         unsigned long range = block->used_length >> TARGET_PAGE_BITS;
         unsigned long run_start = find_next_zero_bit(bitmap, range, 0);
 
+        if (memory_region_is_ram_device(block->mr)) {
+            continue;
+        }
         while (run_start < range) {
             unsigned long run_end = find_next_bit(bitmap, range, run_start + 1);
             ram_discard_range(block->idstr, run_start << TARGET_PAGE_BITS,
@@ -1784,8 +1807,13 @@ static int postcopy_each_ram_send_discard(MigrationState *ms)
     int ret;
 
     RAMBLOCK_FOREACH(block) {
-        PostcopyDiscardState *pds =
-            postcopy_discard_send_init(ms, block->idstr);
+        PostcopyDiscardState *pds;
+
+        if (memory_region_is_ram_device(block->mr)) {
+            continue;
+        }
+
+        pds = postcopy_discard_send_init(ms, block->idstr);
 
         /*
          * Postcopy sends chunks of bitmap over the wire, but it
@@ -1996,6 +2024,10 @@ int ram_postcopy_send_discard_bitmap(MigrationState *ms)
         unsigned long *bitmap = block->bmap;
         unsigned long *unsentmap = block->unsentmap;
 
+        if (memory_region_is_ram_device(block->mr)) {
+            continue;
+        }
+
         if (!unsentmap) {
             /* We don't have a safe way to resize the sentmap, so
              * if the bitmap was resized it will be NULL at this
@@ -2151,6 +2183,9 @@ 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) {
+            if (memory_region_is_ram_device(block->mr)) {
+                continue;
+            }
             pages = block->max_length >> TARGET_PAGE_BITS;
             block->bmap = bitmap_new(pages);
             bitmap_set(block->bmap, 0, pages);
@@ -2227,6 +2262,9 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
     qemu_put_be64(f, ram_bytes_total() | RAM_SAVE_FLAG_MEM_SIZE);
 
     RAMBLOCK_FOREACH(block) {
+        if (memory_region_is_ram_device(block->mr)) {
+            continue;
+        }
         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);
-- 
2.13.6

[cc +folks working on vfio-mdev migration]

On Wed, 11 Apr 2018 19:20:14 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> Here is some context for this strange change request.
> 
> 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 subengine.
> 
> 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.
> 
> This is the goal of the following proposal. Does it make sense ? It
> seems to be working enough to migrate a running guest but there might
> be a better, more subtle, approach.

Yulei, is this something you've run into with GVT-g migration?  I don't
see how we can read from or write to ram_device regions in a useful way
during migration anyway, so the change initially looks correct to me.
Thanks,

Alex

> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  migration/ram.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 0e90efa09236..6404ccd046d8 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -780,6 +780,10 @@ unsigned long migration_bitmap_find_dirty(RAMState *rs, RAMBlock *rb,
>      unsigned long *bitmap = rb->bmap;
>      unsigned long next;
>  
> +    if (memory_region_is_ram_device(rb->mr)) {
> +        return size;
> +    }
> +
>      if (rs->ram_bulk_stage && start > 0) {
>          next = start + 1;
>      } else {
> @@ -826,6 +830,9 @@ uint64_t ram_pagesize_summary(void)
>      uint64_t summary = 0;
>  
>      RAMBLOCK_FOREACH(block) {
> +        if (memory_region_is_ram_device(block->mr)) {
> +            continue;
> +        }
>          summary |= block->page_size;
>      }
>  
> @@ -850,6 +857,9 @@ static void migration_bitmap_sync(RAMState *rs)
>      qemu_mutex_lock(&rs->bitmap_mutex);
>      rcu_read_lock();
>      RAMBLOCK_FOREACH(block) {
> +        if (memory_region_is_ram_device(block->mr)) {
> +            continue;
> +        }
>          migration_bitmap_sync_range(rs, block, 0, block->used_length);
>      }
>      rcu_read_unlock();
> @@ -1499,6 +1509,10 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
>      size_t pagesize_bits =
>          qemu_ram_pagesize(pss->block) >> TARGET_PAGE_BITS;
>  
> +    if (memory_region_is_ram_device(pss->block->mr)) {
> +        return 0;
> +    }
> +
>      do {
>          tmppages = ram_save_target_page(rs, pss, last_stage);
>          if (tmppages < 0) {
> @@ -1588,6 +1602,9 @@ uint64_t ram_bytes_total(void)
>  
>      rcu_read_lock();
>      RAMBLOCK_FOREACH(block) {
> +        if (memory_region_is_ram_device(block->mr)) {
> +            continue;
> +        }
>          total += block->used_length;
>      }
>      rcu_read_unlock();
> @@ -1643,6 +1660,9 @@ static void ram_save_cleanup(void *opaque)
>      memory_global_dirty_log_stop();
>  
>      QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
> +        if (memory_region_is_ram_device(block->mr)) {
> +            continue;
> +        }
>          g_free(block->bmap);
>          block->bmap = NULL;
>          g_free(block->unsentmap);
> @@ -1710,6 +1730,9 @@ void ram_postcopy_migrated_memory_release(MigrationState *ms)
>          unsigned long range = block->used_length >> TARGET_PAGE_BITS;
>          unsigned long run_start = find_next_zero_bit(bitmap, range, 0);
>  
> +        if (memory_region_is_ram_device(block->mr)) {
> +            continue;
> +        }
>          while (run_start < range) {
>              unsigned long run_end = find_next_bit(bitmap, range, run_start + 1);
>              ram_discard_range(block->idstr, run_start << TARGET_PAGE_BITS,
> @@ -1784,8 +1807,13 @@ static int postcopy_each_ram_send_discard(MigrationState *ms)
>      int ret;
>  
>      RAMBLOCK_FOREACH(block) {
> -        PostcopyDiscardState *pds =
> -            postcopy_discard_send_init(ms, block->idstr);
> +        PostcopyDiscardState *pds;
> +
> +        if (memory_region_is_ram_device(block->mr)) {
> +            continue;
> +        }
> +
> +        pds = postcopy_discard_send_init(ms, block->idstr);
>  
>          /*
>           * Postcopy sends chunks of bitmap over the wire, but it
> @@ -1996,6 +2024,10 @@ int ram_postcopy_send_discard_bitmap(MigrationState *ms)
>          unsigned long *bitmap = block->bmap;
>          unsigned long *unsentmap = block->unsentmap;
>  
> +        if (memory_region_is_ram_device(block->mr)) {
> +            continue;
> +        }
> +
>          if (!unsentmap) {
>              /* We don't have a safe way to resize the sentmap, so
>               * if the bitmap was resized it will be NULL at this
> @@ -2151,6 +2183,9 @@ 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) {
> +            if (memory_region_is_ram_device(block->mr)) {
> +                continue;
> +            }
>              pages = block->max_length >> TARGET_PAGE_BITS;
>              block->bmap = bitmap_new(pages);
>              bitmap_set(block->bmap, 0, pages);
> @@ -2227,6 +2262,9 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>      qemu_put_be64(f, ram_bytes_total() | RAM_SAVE_FLAG_MEM_SIZE);
>  
>      RAMBLOCK_FOREACH(block) {
> +        if (memory_region_is_ram_device(block->mr)) {
> +            continue;
> +        }
>          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);

On 4/11/2018 11:25 PM, Alex Williamson wrote:
> [cc +folks working on vfio-mdev migration]
> 
> On Wed, 11 Apr 2018 19:20:14 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> Here is some context for this strange change request.
>>
>> 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 subengine.
>>
>> 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.
>>
>> This is the goal of the following proposal. Does it make sense ? It
>> seems to be working enough to migrate a running guest but there might
>> be a better, more subtle, approach.
> 
> Yulei, is this something you've run into with GVT-g migration?  I don't
> see how we can read from or write to ram_device regions in a useful way
> during migration anyway, so the change initially looks correct to me.
> Thanks,
> 

I ran into this problem with vGPU migration. I have very similar patch
in my local branch to test vGPU migration. This patch looks good to me.

Thanks,
Kirti


> Alex
> 
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  migration/ram.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 0e90efa09236..6404ccd046d8 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -780,6 +780,10 @@ unsigned long migration_bitmap_find_dirty(RAMState *rs, RAMBlock *rb,
>>      unsigned long *bitmap = rb->bmap;
>>      unsigned long next;
>>  
>> +    if (memory_region_is_ram_device(rb->mr)) {
>> +        return size;
>> +    }
>> +
>>      if (rs->ram_bulk_stage && start > 0) {
>>          next = start + 1;
>>      } else {
>> @@ -826,6 +830,9 @@ uint64_t ram_pagesize_summary(void)
>>      uint64_t summary = 0;
>>  
>>      RAMBLOCK_FOREACH(block) {
>> +        if (memory_region_is_ram_device(block->mr)) {
>> +            continue;
>> +        }
>>          summary |= block->page_size;
>>      }
>>  
>> @@ -850,6 +857,9 @@ static void migration_bitmap_sync(RAMState *rs)
>>      qemu_mutex_lock(&rs->bitmap_mutex);
>>      rcu_read_lock();
>>      RAMBLOCK_FOREACH(block) {
>> +        if (memory_region_is_ram_device(block->mr)) {
>> +            continue;
>> +        }
>>          migration_bitmap_sync_range(rs, block, 0, block->used_length);
>>      }
>>      rcu_read_unlock();
>> @@ -1499,6 +1509,10 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
>>      size_t pagesize_bits =
>>          qemu_ram_pagesize(pss->block) >> TARGET_PAGE_BITS;
>>  
>> +    if (memory_region_is_ram_device(pss->block->mr)) {
>> +        return 0;
>> +    }
>> +
>>      do {
>>          tmppages = ram_save_target_page(rs, pss, last_stage);
>>          if (tmppages < 0) {
>> @@ -1588,6 +1602,9 @@ uint64_t ram_bytes_total(void)
>>  
>>      rcu_read_lock();
>>      RAMBLOCK_FOREACH(block) {
>> +        if (memory_region_is_ram_device(block->mr)) {
>> +            continue;
>> +        }
>>          total += block->used_length;
>>      }
>>      rcu_read_unlock();
>> @@ -1643,6 +1660,9 @@ static void ram_save_cleanup(void *opaque)
>>      memory_global_dirty_log_stop();
>>  
>>      QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
>> +        if (memory_region_is_ram_device(block->mr)) {
>> +            continue;
>> +        }
>>          g_free(block->bmap);
>>          block->bmap = NULL;
>>          g_free(block->unsentmap);
>> @@ -1710,6 +1730,9 @@ void ram_postcopy_migrated_memory_release(MigrationState *ms)
>>          unsigned long range = block->used_length >> TARGET_PAGE_BITS;
>>          unsigned long run_start = find_next_zero_bit(bitmap, range, 0);
>>  
>> +        if (memory_region_is_ram_device(block->mr)) {
>> +            continue;
>> +        }
>>          while (run_start < range) {
>>              unsigned long run_end = find_next_bit(bitmap, range, run_start + 1);
>>              ram_discard_range(block->idstr, run_start << TARGET_PAGE_BITS,
>> @@ -1784,8 +1807,13 @@ static int postcopy_each_ram_send_discard(MigrationState *ms)
>>      int ret;
>>  
>>      RAMBLOCK_FOREACH(block) {
>> -        PostcopyDiscardState *pds =
>> -            postcopy_discard_send_init(ms, block->idstr);
>> +        PostcopyDiscardState *pds;
>> +
>> +        if (memory_region_is_ram_device(block->mr)) {
>> +            continue;
>> +        }
>> +
>> +        pds = postcopy_discard_send_init(ms, block->idstr);
>>  
>>          /*
>>           * Postcopy sends chunks of bitmap over the wire, but it
>> @@ -1996,6 +2024,10 @@ int ram_postcopy_send_discard_bitmap(MigrationState *ms)
>>          unsigned long *bitmap = block->bmap;
>>          unsigned long *unsentmap = block->unsentmap;
>>  
>> +        if (memory_region_is_ram_device(block->mr)) {
>> +            continue;
>> +        }
>> +
>>          if (!unsentmap) {
>>              /* We don't have a safe way to resize the sentmap, so
>>               * if the bitmap was resized it will be NULL at this
>> @@ -2151,6 +2183,9 @@ 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) {
>> +            if (memory_region_is_ram_device(block->mr)) {
>> +                continue;
>> +            }
>>              pages = block->max_length >> TARGET_PAGE_BITS;
>>              block->bmap = bitmap_new(pages);
>>              bitmap_set(block->bmap, 0, pages);
>> @@ -2227,6 +2262,9 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>>      qemu_put_be64(f, ram_bytes_total() | RAM_SAVE_FLAG_MEM_SIZE);
>>  
>>      RAMBLOCK_FOREACH(block) {
>> +        if (memory_region_is_ram_device(block->mr)) {
>> +            continue;
>> +        }
>>          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);
> 

> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Thursday, April 12, 2018 1:55 AM
> To: Cédric Le Goater <clg@kaod.org>
> Cc: qemu-devel@nongnu.org; Juan Quintela <quintela@redhat.com>; Dr .
> David Alan Gilbert <dgilbert@redhat.com>; David Gibson
> <david@gibson.dropbear.id.au>; Zhang, Yulei <yulei.zhang@intel.com>; Tian,
> Kevin <kevin.tian@intel.com>; joonas.lahtinen@linux.intel.com;
> zhenyuw@linux.intel.com; kwankhede@nvidia.com; Wang, Zhi A
> <zhi.a.wang@intel.com>
> Subject: Re: [Qemu-devel] [RFC PATCH] migration: discard RAMBlocks of type
> ram_device
> 
> [cc +folks working on vfio-mdev migration]
> 
> On Wed, 11 Apr 2018 19:20:14 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
> > Here is some context for this strange change request.
> >
> > 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 subengine.
> >
> > 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.
> >
> > This is the goal of the following proposal. Does it make sense ? It
> > seems to be working enough to migrate a running guest but there might
> > be a better, more subtle, approach.
> 
> Yulei, is this something you've run into with GVT-g migration?  I don't see
> how we can read from or write to ram_device regions in a useful way during
> migration anyway, so the change initially looks correct to me.
> Thanks,
> 
> Alex
> 

Didn't meet such issue before. I think the change will be fine if the vendor driver
handle the reconstruction well on the target side. And I agree with Dave's suggestion,
how about vendor driver report a flag about the mapped region to indicate it can
be registered as migratable memory block or not.   

Yulei
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > ---
> >  migration/ram.c | 42 ++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 40 insertions(+), 2 deletions(-)
> >
> > diff --git a/migration/ram.c b/migration/ram.c index
> > 0e90efa09236..6404ccd046d8 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -780,6 +780,10 @@ unsigned long
> migration_bitmap_find_dirty(RAMState *rs, RAMBlock *rb,
> >      unsigned long *bitmap = rb->bmap;
> >      unsigned long next;
> >
> > +    if (memory_region_is_ram_device(rb->mr)) {
> > +        return size;
> > +    }
> > +
> >      if (rs->ram_bulk_stage && start > 0) {
> >          next = start + 1;
> >      } else {
> > @@ -826,6 +830,9 @@ uint64_t ram_pagesize_summary(void)
> >      uint64_t summary = 0;
> >
> >      RAMBLOCK_FOREACH(block) {
> > +        if (memory_region_is_ram_device(block->mr)) {
> > +            continue;
> > +        }
> >          summary |= block->page_size;
> >      }
> >
> > @@ -850,6 +857,9 @@ static void migration_bitmap_sync(RAMState *rs)
> >      qemu_mutex_lock(&rs->bitmap_mutex);
> >      rcu_read_lock();
> >      RAMBLOCK_FOREACH(block) {
> > +        if (memory_region_is_ram_device(block->mr)) {
> > +            continue;
> > +        }
> >          migration_bitmap_sync_range(rs, block, 0, block->used_length);
> >      }
> >      rcu_read_unlock();
> > @@ -1499,6 +1509,10 @@ static int ram_save_host_page(RAMState *rs,
> PageSearchStatus *pss,
> >      size_t pagesize_bits =
> >          qemu_ram_pagesize(pss->block) >> TARGET_PAGE_BITS;
> >
> > +    if (memory_region_is_ram_device(pss->block->mr)) {
> > +        return 0;
> > +    }
> > +
> >      do {
> >          tmppages = ram_save_target_page(rs, pss, last_stage);
> >          if (tmppages < 0) {
> > @@ -1588,6 +1602,9 @@ uint64_t ram_bytes_total(void)
> >
> >      rcu_read_lock();
> >      RAMBLOCK_FOREACH(block) {
> > +        if (memory_region_is_ram_device(block->mr)) {
> > +            continue;
> > +        }
> >          total += block->used_length;
> >      }
> >      rcu_read_unlock();
> > @@ -1643,6 +1660,9 @@ static void ram_save_cleanup(void *opaque)
> >      memory_global_dirty_log_stop();
> >
> >      QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
> > +        if (memory_region_is_ram_device(block->mr)) {
> > +            continue;
> > +        }
> >          g_free(block->bmap);
> >          block->bmap = NULL;
> >          g_free(block->unsentmap);
> > @@ -1710,6 +1730,9 @@ void
> ram_postcopy_migrated_memory_release(MigrationState *ms)
> >          unsigned long range = block->used_length >> TARGET_PAGE_BITS;
> >          unsigned long run_start = find_next_zero_bit(bitmap, range,
> > 0);
> >
> > +        if (memory_region_is_ram_device(block->mr)) {
> > +            continue;
> > +        }
> >          while (run_start < range) {
> >              unsigned long run_end = find_next_bit(bitmap, range, run_start +
> 1);
> >              ram_discard_range(block->idstr, run_start <<
> > TARGET_PAGE_BITS, @@ -1784,8 +1807,13 @@ static int
> postcopy_each_ram_send_discard(MigrationState *ms)
> >      int ret;
> >
> >      RAMBLOCK_FOREACH(block) {
> > -        PostcopyDiscardState *pds =
> > -            postcopy_discard_send_init(ms, block->idstr);
> > +        PostcopyDiscardState *pds;
> > +
> > +        if (memory_region_is_ram_device(block->mr)) {
> > +            continue;
> > +        }
> > +
> > +        pds = postcopy_discard_send_init(ms, block->idstr);
> >
> >          /*
> >           * Postcopy sends chunks of bitmap over the wire, but it @@
> > -1996,6 +2024,10 @@ int
> ram_postcopy_send_discard_bitmap(MigrationState *ms)
> >          unsigned long *bitmap = block->bmap;
> >          unsigned long *unsentmap = block->unsentmap;
> >
> > +        if (memory_region_is_ram_device(block->mr)) {
> > +            continue;
> > +        }
> > +
> >          if (!unsentmap) {
> >              /* We don't have a safe way to resize the sentmap, so
> >               * if the bitmap was resized it will be NULL at this @@
> > -2151,6 +2183,9 @@ 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) {
> > +            if (memory_region_is_ram_device(block->mr)) {
> > +                continue;
> > +            }
> >              pages = block->max_length >> TARGET_PAGE_BITS;
> >              block->bmap = bitmap_new(pages);
> >              bitmap_set(block->bmap, 0, pages); @@ -2227,6 +2262,9 @@
> > static int ram_save_setup(QEMUFile *f, void *opaque)
> >      qemu_put_be64(f, ram_bytes_total() | RAM_SAVE_FLAG_MEM_SIZE);
> >
> >      RAMBLOCK_FOREACH(block) {
> > +        if (memory_region_is_ram_device(block->mr)) {
> > +            continue;
> > +        }
> >          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);

On Thu, 12 Apr 2018 15:59:24 +0000
"Zhang, Yulei" <yulei.zhang@intel.com> wrote:

> > -----Original Message-----
> > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > Sent: Thursday, April 12, 2018 1:55 AM
> > To: Cédric Le Goater <clg@kaod.org>
> > Cc: qemu-devel@nongnu.org; Juan Quintela <quintela@redhat.com>; Dr .
> > David Alan Gilbert <dgilbert@redhat.com>; David Gibson
> > <david@gibson.dropbear.id.au>; Zhang, Yulei <yulei.zhang@intel.com>; Tian,
> > Kevin <kevin.tian@intel.com>; joonas.lahtinen@linux.intel.com;
> > zhenyuw@linux.intel.com; kwankhede@nvidia.com; Wang, Zhi A
> > <zhi.a.wang@intel.com>
> > Subject: Re: [Qemu-devel] [RFC PATCH] migration: discard RAMBlocks of type
> > ram_device
> > 
> > [cc +folks working on vfio-mdev migration]
> > 
> > On Wed, 11 Apr 2018 19:20:14 +0200
> > Cédric Le Goater <clg@kaod.org> wrote:
> >   
> > > Here is some context for this strange change request.
> > >
> > > 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 subengine.
> > >
> > > 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.
> > >
> > > This is the goal of the following proposal. Does it make sense ? It
> > > seems to be working enough to migrate a running guest but there might
> > > be a better, more subtle, approach.  
> > 
> > Yulei, is this something you've run into with GVT-g migration?  I don't see
> > how we can read from or write to ram_device regions in a useful way during
> > migration anyway, so the change initially looks correct to me.
> > Thanks,
> > 
> > Alex
> >   
> 
> Didn't meet such issue before. I think the change will be fine if the vendor driver
> handle the reconstruction well on the target side. And I agree with Dave's suggestion,
> how about vendor driver report a flag about the mapped region to indicate it can
> be registered as migratable memory block or not.   

"migration" and "memory blocks" are not vfio concepts, you'd need to
come up with flags that actually conveys the device level property of
the region that you're trying to indicate.  I don't see why we'd do
this though, the application of such a flag seems too narrow and it
tarnishes the concept that a vendor driver provides a region, through
which *all* device state is saved and restored.  Thanks,

Alex

On 4/12/2018 9:53 PM, Alex Williamson wrote:
> On Thu, 12 Apr 2018 15:59:24 +0000
> "Zhang, Yulei" <yulei.zhang@intel.com> wrote:
> 
>>> -----Original Message-----
>>> From: Alex Williamson [mailto:alex.williamson@redhat.com]
>>> Sent: Thursday, April 12, 2018 1:55 AM
>>> To: Cédric Le Goater <clg@kaod.org>
>>> Cc: qemu-devel@nongnu.org; Juan Quintela <quintela@redhat.com>; Dr .
>>> David Alan Gilbert <dgilbert@redhat.com>; David Gibson
>>> <david@gibson.dropbear.id.au>; Zhang, Yulei <yulei.zhang@intel.com>; Tian,
>>> Kevin <kevin.tian@intel.com>; joonas.lahtinen@linux.intel.com;
>>> zhenyuw@linux.intel.com; kwankhede@nvidia.com; Wang, Zhi A
>>> <zhi.a.wang@intel.com>
>>> Subject: Re: [Qemu-devel] [RFC PATCH] migration: discard RAMBlocks of type
>>> ram_device
>>>
>>> [cc +folks working on vfio-mdev migration]
>>>
>>> On Wed, 11 Apr 2018 19:20:14 +0200
>>> Cédric Le Goater <clg@kaod.org> wrote:
>>>   
>>>> Here is some context for this strange change request.
>>>>
>>>> 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 subengine.
>>>>
>>>> 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.
>>>>
>>>> This is the goal of the following proposal. Does it make sense ? It
>>>> seems to be working enough to migrate a running guest but there might
>>>> be a better, more subtle, approach.  
>>>
>>> Yulei, is this something you've run into with GVT-g migration?  I don't see
>>> how we can read from or write to ram_device regions in a useful way during
>>> migration anyway, so the change initially looks correct to me.
>>> Thanks,
>>>
>>> Alex
>>>   
>>
>> Didn't meet such issue before. I think the change will be fine if the vendor driver
>> handle the reconstruction well on the target side. And I agree with Dave's suggestion,
>> how about vendor driver report a flag about the mapped region to indicate it can
>> be registered as migratable memory block or not.   
> 
> "migration" and "memory blocks" are not vfio concepts, you'd need to
> come up with flags that actually conveys the device level property of
> the region that you're trying to indicate.  I don't see why we'd do
> this though, the application of such a flag seems too narrow and it
> tarnishes the concept that a vendor driver provides a region, through
> which *all* device state is saved and restored.  Thanks,
> 

I don't think vendor driver need to report region as migratable or not
explicitly.

I hit the issue when MMIO region is mmaped by VFIO, for example BAR1 of
vGPU, that region is marked as ram region.
vfio_region_mmap() -> memory_region_init_ram_device_ptr() -> sets
mr->ram_device = true
Vendor driver specifies which region to be MMAPed, based on that QEMU
can decide to skip that region during migration as in this RFC. Then
vendor driver takes care of restoring it's MMIO region state after
migration.

Thanks,
Kirti

* Cédric Le Goater (clg@kaod.org) wrote:
> Here is some context for this strange change request.
> 
> 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 subengine.
> 
> 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.
> 
> This is the goal of the following proposal. Does it make sense ? It
> seems to be working enough to migrate a running guest but there might
> be a better, more subtle, approach.

If this is always true of RAM devices (which I suspect it is).

Interestingly, your patch comes less than 2 weeks after Lai Jiangshan's
 'add capability to bypass the shared memory'
   https://lists.nongnu.org/archive/html/qemu-devel/2018-03/msg07511.html

which is the only other case I think we've got of someone trying to
avoid transmitting a block.

We should try and merge the two sets to make them consistent; you've
covered some more cases (the other patch wasn't expected to work with
Postcopy anyway).
(At this rate then we can expect another 20 for the year....)

We should probably have:
   1) A     bool is_migratable_block(RAMBlock *)
   2) A RAMBLOCK_FOREACH_MIGRATABLE(block)  macro that is like
RAMBLOCK_FOREACH but does the call to is_migratable_block

then the changes should be mostly pretty tidy.

A sanity check is probably needed on load as well, to give a neat
error if for some reason the source transmits pages to you.

One other thing I notice is your code changes ram_bytes_total(),
where as the other patch avoids it;  I think your code is actually
more correct.

Is there *any* case in existing QEMUs where we migrate ram devices
succesfully, if so we've got to make it backwards compatible; but I
think you're saying there isn't.

Dave


> Thanks,
> 
> C.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  migration/ram.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 0e90efa09236..6404ccd046d8 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -780,6 +780,10 @@ unsigned long migration_bitmap_find_dirty(RAMState *rs, RAMBlock *rb,
>      unsigned long *bitmap = rb->bmap;
>      unsigned long next;
>  
> +    if (memory_region_is_ram_device(rb->mr)) {
> +        return size;
> +    }
> +
>      if (rs->ram_bulk_stage && start > 0) {
>          next = start + 1;
>      } else {
> @@ -826,6 +830,9 @@ uint64_t ram_pagesize_summary(void)
>      uint64_t summary = 0;
>  
>      RAMBLOCK_FOREACH(block) {
> +        if (memory_region_is_ram_device(block->mr)) {
> +            continue;
> +        }
>          summary |= block->page_size;
>      }
>  
> @@ -850,6 +857,9 @@ static void migration_bitmap_sync(RAMState *rs)
>      qemu_mutex_lock(&rs->bitmap_mutex);
>      rcu_read_lock();
>      RAMBLOCK_FOREACH(block) {
> +        if (memory_region_is_ram_device(block->mr)) {
> +            continue;
> +        }
>          migration_bitmap_sync_range(rs, block, 0, block->used_length);
>      }
>      rcu_read_unlock();
> @@ -1499,6 +1509,10 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
>      size_t pagesize_bits =
>          qemu_ram_pagesize(pss->block) >> TARGET_PAGE_BITS;
>  
> +    if (memory_region_is_ram_device(pss->block->mr)) {
> +        return 0;
> +    }
> +

Now we shouldn't actually end up here should we - so I suggest an
error_report and returning -EINVAL.

>      do {
>          tmppages = ram_save_target_page(rs, pss, last_stage);
>          if (tmppages < 0) {
> @@ -1588,6 +1602,9 @@ uint64_t ram_bytes_total(void)
>  
>      rcu_read_lock();
>      RAMBLOCK_FOREACH(block) {
> +        if (memory_region_is_ram_device(block->mr)) {
> +            continue;
> +        }
>          total += block->used_length;
>      }
>      rcu_read_unlock();
> @@ -1643,6 +1660,9 @@ static void ram_save_cleanup(void *opaque)
>      memory_global_dirty_log_stop();
>  
>      QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
> +        if (memory_region_is_ram_device(block->mr)) {
> +            continue;
> +        }
>          g_free(block->bmap);
>          block->bmap = NULL;
>          g_free(block->unsentmap);
> @@ -1710,6 +1730,9 @@ void ram_postcopy_migrated_memory_release(MigrationState *ms)
>          unsigned long range = block->used_length >> TARGET_PAGE_BITS;
>          unsigned long run_start = find_next_zero_bit(bitmap, range, 0);
>  
> +        if (memory_region_is_ram_device(block->mr)) {
> +            continue;
> +        }
>          while (run_start < range) {
>              unsigned long run_end = find_next_bit(bitmap, range, run_start + 1);
>              ram_discard_range(block->idstr, run_start << TARGET_PAGE_BITS,
> @@ -1784,8 +1807,13 @@ static int postcopy_each_ram_send_discard(MigrationState *ms)
>      int ret;
>  
>      RAMBLOCK_FOREACH(block) {
> -        PostcopyDiscardState *pds =
> -            postcopy_discard_send_init(ms, block->idstr);
> +        PostcopyDiscardState *pds;
> +
> +        if (memory_region_is_ram_device(block->mr)) {
> +            continue;
> +        }
> +
> +        pds = postcopy_discard_send_init(ms, block->idstr);
>  
>          /*
>           * Postcopy sends chunks of bitmap over the wire, but it
> @@ -1996,6 +2024,10 @@ int ram_postcopy_send_discard_bitmap(MigrationState *ms)
>          unsigned long *bitmap = block->bmap;
>          unsigned long *unsentmap = block->unsentmap;
>  
> +        if (memory_region_is_ram_device(block->mr)) {
> +            continue;
> +        }
> +
>          if (!unsentmap) {
>              /* We don't have a safe way to resize the sentmap, so
>               * if the bitmap was resized it will be NULL at this
> @@ -2151,6 +2183,9 @@ 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) {
> +            if (memory_region_is_ram_device(block->mr)) {
> +                continue;
> +            }
>              pages = block->max_length >> TARGET_PAGE_BITS;
>              block->bmap = bitmap_new(pages);
>              bitmap_set(block->bmap, 0, pages);
> @@ -2227,6 +2262,9 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>      qemu_put_be64(f, ram_bytes_total() | RAM_SAVE_FLAG_MEM_SIZE);
>  
>      RAMBLOCK_FOREACH(block) {
> +        if (memory_region_is_ram_device(block->mr)) {
> +            continue;
> +        }
>          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);
> -- 
> 2.13.6
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

On 04/11/2018 09:21 PM, Dr. David Alan Gilbert wrote:
> * Cédric Le Goater (clg@kaod.org) wrote:
>> Here is some context for this strange change request.
>>
>> 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 subengine.
>>
>> 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.
>>
>> This is the goal of the following proposal. Does it make sense ? It
>> seems to be working enough to migrate a running guest but there might
>> be a better, more subtle, approach.
> 
> If this is always true of RAM devices (which I suspect it is).
>
> Interestingly, your patch comes less than 2 weeks after Lai Jiangshan's
>  'add capability to bypass the shared memory'
>    https://lists.nongnu.org/archive/html/qemu-devel/2018-03/msg07511.html

I missed that.

> which is the only other case I think we've got of someone trying to
> avoid transmitting a block.
> 
> We should try and merge the two sets to make them consistent; you've
> covered some more cases (the other patch wasn't expected to work with
> Postcopy anyway).
> (At this rate then we can expect another 20 for the year....)
> 
> We should probably have:
>    1) A     bool is_migratable_block(RAMBlock *)
>    2) A RAMBLOCK_FOREACH_MIGRATABLE(block)  macro that is like
> RAMBLOCK_FOREACH but does the call to is_migratable_block
>
> then the changes should be mostly pretty tidy.

yes, indeed, they do.

> A sanity check is probably needed on load as well, to give a neat
> error if for some reason the source transmits pages to you.

OK. 

Would a check on the block migratability at the end of function 
ram_block_from_stream() be enough ? This is called in ram_load()
and ram_load_postcopy()

> One other thing I notice is your code changes ram_bytes_total(),
> where as the other patch avoids it;  I think your code is actually
> more correct.
> 
> Is there *any* case in existing QEMUs where we migrate ram devices
> succesfully, if so we've got to make it backwards compatible; but I
> think you're saying there isn't.

The only RAM devices I know of are the VFIOs.

Thanks,

C.

* Cédric Le Goater (clg@kaod.org) wrote:
> On 04/11/2018 09:21 PM, Dr. David Alan Gilbert wrote:
> > * Cédric Le Goater (clg@kaod.org) wrote:
> >> Here is some context for this strange change request.
> >>
> >> 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 subengine.
> >>
> >> 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.
> >>
> >> This is the goal of the following proposal. Does it make sense ? It
> >> seems to be working enough to migrate a running guest but there might
> >> be a better, more subtle, approach.
> > 
> > If this is always true of RAM devices (which I suspect it is).
> >
> > Interestingly, your patch comes less than 2 weeks after Lai Jiangshan's
> >  'add capability to bypass the shared memory'
> >    https://lists.nongnu.org/archive/html/qemu-devel/2018-03/msg07511.html
> 
> I missed that.
> 
> > which is the only other case I think we've got of someone trying to
> > avoid transmitting a block.
> > 
> > We should try and merge the two sets to make them consistent; you've
> > covered some more cases (the other patch wasn't expected to work with
> > Postcopy anyway).
> > (At this rate then we can expect another 20 for the year....)
> > 
> > We should probably have:
> >    1) A     bool is_migratable_block(RAMBlock *)
> >    2) A RAMBLOCK_FOREACH_MIGRATABLE(block)  macro that is like
> > RAMBLOCK_FOREACH but does the call to is_migratable_block
> >
> > then the changes should be mostly pretty tidy.
> 
> yes, indeed, they do.
> 
> > A sanity check is probably needed on load as well, to give a neat
> > error if for some reason the source transmits pages to you.
> 
> OK. 
> 
> Would a check on the block migratability at the end of function 
> ram_block_from_stream() be enough ? This is called in ram_load()
> and ram_load_postcopy()

Yes I think that's fine.  Maybe also add one in ram_load() in
the     case RAM_SAVE_FLAG_MEM_SIZE:      which happens right at the
start of the migration stream.

> > One other thing I notice is your code changes ram_bytes_total(),
> > where as the other patch avoids it;  I think your code is actually
> > more correct.
> > 
> > Is there *any* case in existing QEMUs where we migrate ram devices
> > succesfully, if so we've got to make it backwards compatible; but I
> > think you're saying there isn't.
> 
> The only RAM devices I know of are the VFIOs.

Great, so we should be OK.

Dave

> Thanks,
> 
> C.
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

On 12 April 2018 at 08:02, Cédric Le Goater <clg@kaod.org> wrote:
> On 04/11/2018 09:21 PM, Dr. David Alan Gilbert wrote:
>> Interestingly, your patch comes less than 2 weeks after Lai Jiangshan's
>>  'add capability to bypass the shared memory'
>>    https://lists.nongnu.org/archive/html/qemu-devel/2018-03/msg07511.html
>
> I missed that.
>
>> which is the only other case I think we've got of someone trying to
>> avoid transmitting a block.
>>
>> We should try and merge the two sets to make them consistent; you've
>> covered some more cases (the other patch wasn't expected to work with
>> Postcopy anyway).
>> (At this rate then we can expect another 20 for the year....)
>>
>> We should probably have:
>>    1) A     bool is_migratable_block(RAMBlock *)
>>    2) A RAMBLOCK_FOREACH_MIGRATABLE(block)  macro that is like
>> RAMBLOCK_FOREACH but does the call to is_migratable_block
>>
>> then the changes should be mostly pretty tidy.

We also want this so we can enable migration when the
"mmio-exec" feature is being used without accidentally
trying to migrate the ramblock used for execute-from-device:
 http://lists.gnu.org/archive/html/qemu-devel/2017-07/msg05237.html

thanks
-- PMM