[RFC] vfio-pci/migration: Dirty logging of the Memory BAR region?

Commit Message

Zenghui Yu Nov. 15, 2020, 2:31 p.m. UTC

Hi folks,

While trying the new vfio-pci migration on my arm64 server, I noticed an
error at the very beginning:

qemu-system-aarch64: kvm_set_user_memory_region: 
KVM_SET_USER_MEMORY_REGION failed, slot=5, start=0x8000000000, 
size=0x100000: Invalid argument

The reason is that we've registered the Memory BAR region as ram device
region (mr->ram_device is set) and tried to track dirty pages of this
region during migration.  QEMU tries to request tracking of it (via kvm
memory listener's log_start() callback) whilst KVM/arm64 clearly refuses
to do so [1]:

 > int kvm_arch_prepare_memory_region(struct kvm *kvm, ...)
 > {
 > 		/* IO region dirty page logging not allowed */
 > 		if (memslot->flags & KVM_MEM_LOG_DIRTY_PAGES) {
 > 			ret = -EINVAL;
 > 			goto out;
 > 		}
 > }

If I understand things correctly, the Memory BAR region generally
contains internal device-specific registers which shouldn't (and can't)
be tracked by QEMU's dirty logging mechanism.  I've patched QEMU with
the following diff and it seems work for me, but all of these still
require for comments.


      return mask;


Thanks,
Zenghui

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kvm/mmu.c

Comments

Paolo Bonzini Nov. 15, 2020, 3:03 p.m. UTC | #1

On 15/11/20 15:31, Zenghui Yu wrote:
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 71951fe4dc..0958db1a08 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -1806,7 +1806,10 @@ bool memory_region_is_ram_device(MemoryRegion *mr)
>   uint8_t memory_region_get_dirty_log_mask(MemoryRegion *mr)
>   {
>       uint8_t mask = mr->dirty_log_mask;
> -    if (global_dirty_log && (mr->ram_block || 
> memory_region_is_iommu(mr))) {
> +    RAMBlock *rb = mr->ram_block;
> +
> +    if (global_dirty_log && ((rb && qemu_ram_is_migratable(rb)) ||
> +                             memory_region_is_iommu(mr))) {
>           mask |= (1 << DIRTY_MEMORY_MIGRATION);
>       }
>       return mask;

Yes, this makes sense.  Please send it as a patch, thanks!

Paolo

Zenghui Yu Nov. 16, 2020, 8:16 a.m. UTC | #2

On 2020/11/15 23:03, Paolo Bonzini wrote:
> On 15/11/20 15:31, Zenghui Yu wrote:
>> diff --git a/softmmu/memory.c b/softmmu/memory.c
>> index 71951fe4dc..0958db1a08 100644
>> --- a/softmmu/memory.c
>> +++ b/softmmu/memory.c
>> @@ -1806,7 +1806,10 @@ bool memory_region_is_ram_device(MemoryRegion *mr)
>>  uint8_t memory_region_get_dirty_log_mask(MemoryRegion *mr)
>>  {
>>      uint8_t mask = mr->dirty_log_mask;
>> -    if (global_dirty_log && (mr->ram_block || 
>> memory_region_is_iommu(mr))) {
>> +    RAMBlock *rb = mr->ram_block;
>> +
>> +    if (global_dirty_log && ((rb && qemu_ram_is_migratable(rb)) ||
>> +                             memory_region_is_iommu(mr))) {
>>          mask |= (1 << DIRTY_MEMORY_MIGRATION);
>>      }
>>      return mask;
> 
> Yes, this makes sense.  Please send it as a patch, thanks!

Sure, I'm going to write a commit message for it.  Thanks for your
review.


Zenghui

Patch

diff --git a/softmmu/memory.c b/softmmu/memory.c
index 71951fe4dc..0958db1a08 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1806,7 +1806,10 @@  bool memory_region_is_ram_device(MemoryRegion *mr)
  uint8_t memory_region_get_dirty_log_mask(MemoryRegion *mr)
  {
      uint8_t mask = mr->dirty_log_mask;
-    if (global_dirty_log && (mr->ram_block || 
memory_region_is_iommu(mr))) {
+    RAMBlock *rb = mr->ram_block;
+
+    if (global_dirty_log && ((rb && qemu_ram_is_migratable(rb)) ||
+                             memory_region_is_iommu(mr))) {
          mask |= (1 << DIRTY_MEMORY_MIGRATION);
      }