live migration support for VM dirty log management
Subsystems (2)
PATCH 3/5 live migration support for VM dirty log management
Add support for dirty bitmap management. Wanted to make it generic but function does a couple things different then the x86 version.
--- arch/arm/include/asm/kvm_host.h | 3 ++ arch/arm/kvm/arm.c | 71 ++++++++++++++++++++++++++++++++++++++- arch/arm/kvm/mmu.c | 53 +++++++++++++++++++++++++++++ 3 files changed, 126 insertions(+), 1 deletion(-)
arch/arm/include/asm/kvm_host.h +3 -0
@@ -230,5 +230,8 @@ int kvm_arm_timer_set_reg(struct kvm_vcpu *, u64 regid, u64 value);void kvm_tlb_flush_vm (struct kvm *kvm);void kvm_mmu_slot_remove_write_access (struct kvm *kvm,int slot); +void kvm_mmu_write_protect_pt_masked (struct kvm *kvm, +struct kvm_memory_slot *slot, +gfn_t gfn_offset,unsigned long mask);#endif /* __ARM_KVM_HOST_H__ */
arch/arm/kvm/arm.c +70 -1
79 lines not on this page · raw message
arch/arm/kvm/mmu.c +53 -0
59 lines not on this page · raw message
Signature · 1 line
1.7.9.5
Quoted 32 lines · Mario Smarduch
Add support for dirty bitmap management. Wanted to make it generic but function does a couple things different then the x86 version.
Signed-off-by Mario Smarduch <[email protected]>--- arch/arm/include/asm/kvm_host.h | 3 ++ arch/arm/kvm/arm.c | 71 ++++++++++++++++++++++++++++++++++++++- arch/arm/kvm/mmu.c | 53 +++++++++++++++++++++++++++++ 3 files changed, 126 insertions(+), 1 deletion(-)arch/arm/include/asm/kvm_host.h +3
-0@@ -230,5 +230,8 @@ int kvm_arm_timer_set_reg(struct kvm_vcpu *, u64 regid, u64 value);void kvm_tlb_flush_vm (struct kvm *kvm);void kvm_mmu_slot_remove_write_access (struct kvm *kvm,int slot); +void kvm_mmu_write_protect_pt_masked (struct kvm *kvm, +struct kvm_memory_slot *slot, +gfn_t gfn_offset,unsigned long mask);#endif /* __ARM_KVM_HOST_H__ */ arch/arm/kvm/arm.c +0
-0@@ -785,9 +785,78 @@ long kvm_arch_vcpu_ioctl(struct file *filp, } }
arch/arm/kvm/arm.c long kvm_arch_vcpu_ioctl(struct file *filp, · line 797
Quoted 10 lines
arch/arm/kvm/arm.c +10
-0+ +/** +* kvm_mmu_slot_remove_access - retrieves the log of dirty pages for a memslot. +* It's itteratively during migration to retrieve pages written since +* last call. In the process write protects ptes that are dirty for next +* time, holds the mmu_lock while write protecting dirty pages. +* +* @kvm: The KVM pointer +* @log: Bitmap of dirty pages return. +*/
So let's play the difference game with x86:
arch/arm/kvm/arm.c long kvm_arch_vcpu_ioctl(struct file *filp, · line 806
Quoted 10 lines
arch/arm/kvm/arm.c +7
-1int kvm_vm_ioctl_get_dirty_log (struct kvm *kvm,struct kvm_dirty_log *log) { -return -EINVAL ; +int r; +struct kvm_memory_slot *memslot; +unsigned long n, i; +unsigned long *dirty_bitmap; +unsigned long *dirty_bitmap_buffer; +bool is_dirty = false; +gfn_t offset;
You've moved offset out of the loop.
arch/arm/kvm/arm.c long kvm_arch_vcpu_ioctl(struct file *filp, · line 815
Quoted 8 lines
arch/arm/kvm/arm.c +8
-0+mutex_lock (&kvm->slots_lock); + r = -EINVAL ; + +/* Return with error code will cause migration to abort, this happens +* when initial write protection of VM to manage dirty pages fails +*/ +if (kvm->arch.migration_in_progress ==-1 ) + goto out;
You've added this test. How does x86 cope with the same case?
Quoted 18 lines
arch/arm/kvm/arm.c +18
-0+if (log->slot >=KVM_USER_MEM_SLOTS ) + goto out; + + memslot =id_to_memslot (kvm->memslots, log->slot); + dirty_bitmap = memslot->dirty_bitmap; + + r = -ENOENT ; +if (!dirty_bitmap) + goto out; + + n =kvm_dirty_bitmap_bytes (memslot); + dirty_bitmap_buffer = dirty_bitmap + n /sizeof (long ); +memset (dirty_bitmap_buffer,0 , n); + +spin_lock (&kvm->mmu_lock); +for (i =0 ; i < n /sizeof (long ); i++) { +unsigned long mask; +
arch/arm/kvm/arm.c long kvm_arch_vcpu_ioctl(struct file *filp, · line 844
Quoted 10 lines
arch/arm/kvm/arm.c +10
-0+if (!dirty_bitmap[i]) +continue ; + + is_dirty = true; + offset = i *BITS_PER_LONG ; +kvm_mmu_write_protect_pt_masked (kvm, memslot, offset, + dirty_bitmap[i]); + mask = dirty_bitmap[i]; + dirty_bitmap_buffer[i] = mask; + dirty_bitmap[i] =0 ;
You've expanded the xchg macro, and moved it around.
arch/arm/kvm/arm.c long kvm_arch_vcpu_ioctl(struct file *filp, · line 848
Quoted 4 lines
arch/arm/kvm/arm.c +4
-0+ } + +if (is_dirty) +kvm_tlb_flush_vm (kvm);
This can be easily abstracted to be a kvm_flush_remote_tlbs on x86, and a HW broadcast on ARM.
arch/arm/kvm/arm.c long kvm_arch_vcpu_ioctl(struct file *filp, · line 860
Quoted 12 lines
arch/arm/kvm/arm.c +11
-0+ +spin_unlock (&kvm->mmu_lock); + r = -EFAULT ; + +if (copy_to_user (log->dirty_bitmap, dirty_bitmap_buffer, n)) + goto out; + + r =0 ; +out: +mutex_unlock (&kvm->slots_lock); +return r; }
So only two small differences that can be easily factored out. I stand by my initial analysis, and ask again you move this function to the generic code as a weak symbol. There isn't any value in duplicating it.
Quoted 34 lines
arch/arm/kvm/arm.c +0
-0static int kvm_vm_ioctl_set_device_addr(struct kvm *kvm,arch/arm/kvm/mmu.c +25
-0@@ -773,6 +773,59 @@ void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot)spin_unlock (&kvm->mmu_lock); } + +/** +* kvm_mmu_write_protected_pt_masked - after migration thread write protects +* the entire VM address space itterative calls are made to get diry pages +* as the VM pages are being migrated. New dirty pages may be subset +* of initial WPed VM or new writes faulted in. Here write protect new +* dirty pages again in preparation of next dirty log read. This function is +* called as a result KVM_GET_DIRTY_LOG ioctl, to determine what pages +* need to be migrated. +* 'kvm->mmu_lock' must be held to protect against concurrent modification +* of page tables (2nd stage fault, mmu modifiers, device writes) +* +* @kvm: The KVM pointer +* @slot: The memory slot the dirty log is retrieved for +* @gfn_offset: The gfn offset in memory slot +* @mask: The mask of dirty pages at offset 'gnf_offset in this memory +* slot to be writ protect +*/ + +void kvm_mmu_write_protect_pt_masked (struct kvm *kvm, +struct kvm_memory_slot *slot, +gfn_t gfn_offset,unsigned long mask) +{ +phys_addr_t ipa; +pgd_t *pgdp = kvm->arch.pgd, *pgd;
arch/arm/kvm/mmu.c void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot) · line 810
Quoted 10 lines
arch/arm/kvm/mmu.c +10
-0+pud_t *pud; +pmd_t *pmd; +pte_t *pte, new_pte; + +/* walk set bits in the mask and write protect corresponding pages */ +while (mask) { + ipa = (slot->base_gfn + gfn_offset +__ffs (mask)) <<PAGE_SHIFT ; + pgd = pgdp +pgd_index (ipa); +if (!pgd_present (*pgd)) + goto update_mask;
I think something is wrong in your logic. If there is no PGD, it means a whole 1GB isn't present. Yet you're just clearing one bit from the mask and doing it again. As you're only looking at BITS_PER_LONG contiguous pages at a time, it is likely that the same thing will happen for the other pages, and you're just wasting precious CPU cycles here.
arch/arm/kvm/mmu.c void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot) · line 823
Quoted 13 lines
arch/arm/kvm/mmu.c +13
-0+ pud =pud_offset (pgd, ipa); +if (!pud_present (*pud)) + goto update_mask; + pmd =pmd_offset (pud, ipa); +if (!pmd_present (*pmd)) + goto update_mask; + pte =pte_offset_kernel (pmd, ipa); +if (!pte_present (*pte)) + goto update_mask; +if ((*pte &L_PTE_S2_RDWR ) ==L_PTE_S2_RDONLY ) + goto update_mask; + new_pte =pfn_pte (pte_pfn (*pte),PAGE_S2 ); + *pte = new_pte;
I'd like to see these two lines in a separate function (something like "stage2_mark_pte_ro")...
Quoted 8 lines
arch/arm/kvm/mmu.c +5
-0+update_mask: + mask &= mask -1 ; + } +} +static int user_mem_abort (struct kvm_vcpu *vcpu,phys_addr_t fault_ipa,struct kvm_memory_slot *memslot,unsigned long fault_status)
Signature · 1 line
Jazz is not dead. It just smells funny.
MZ> So let's play the difference game with x86:
Quoted 1 line
int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, struct
kvm_dirty_log
kvm_vm_ioctl_get_dirty_log() is identical now to x86 version moved it to kvm_main.c, to make it generic, it's declared weak. Do I go into x86 and remove that function?
Or leave it to x86 folks to do it?
-----Original Message-----
arch/arm/kvm/arm.c long kvm_arch_vcpu_ioctl(struct file *filp, · line 848
Quoted 6 lines
+ *
arch/arm/kvm/arm.c +4
-0+ } + +if (is_dirty) +kvm_tlb_flush_vm (kvm);
MZ> This can be easily abstracted to be a kvm_flush_remote_tlbs on x86, and a HW broadcast on ARM.
Kvm_tlb_flush_vm() is replaced with kvm_flush_remote_tlbs() I made that function weak and declared a ARM version, in arm mmu.c
The current version sends IPIs to vCPU running the guest, ARMv7+ does not need that. Is that ok?
arch/arm/kvm/mmu.c void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot) · line 810
Quoted 6 lines
arch/arm/kvm/mmu.c +6
-0+/* walk set bits in the mask and write protect corresponding pages */ +while (mask) { + ipa = (slot->base_gfn + gfn_offset +__ffs (mask)) <<PAGE_SHIFT ; + pgd = pgdp +pgd_index (ipa); +if (!pgd_present (*pgd)) + goto update_mask;
MZ> I think something is wrong in your logic. If there is no PGD, it means a whole 1GB isn't present. Yet you're just clearing one bit from the mask and doing it again. As you're only looking at BITS_PER_LONG MZ> contiguous pages at a time, it is likely that the same thing will happen for the other pages, and you're just wasting precious CPU cycles here.
Yes this is grossly inefficient, I updated it to walk ptes only, after first determining if it straddles a pmd. Should mostly be pte walks with maybe one PMD walk but unlikely.
arch/arm/kvm/mmu.c void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot) · line 823
Quoted 2 lines
arch/arm/kvm/mmu.c +2
-0+ new_pte =pfn_pte (pte_pfn (*pte),PAGE_S2 ); + *pte = new_pte;
MZ> I'd like to see these two lines in a separate function (something like "stage2_mark_pte_ro")...
Yes ok.
(emailed from outlook client)
--
Jazz is not dead. It just smells funny.
Quoted 10 lines · Mario Smarduch
MZ> So let's play the difference game with x86:
Quoted 1 line
int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, struct
kvm_dirty_log
kvm_vm_ioctl_get_dirty_log()is identical now to x86 version moved it tokvm_main.c, to make it generic, it's declared weak. Do I go into x86 and remove that function?Or leave it to x86 folks to do it?
No, this must be part of your patch series.
Quoted 8 lines
arch/arm/kvm/arm.c long kvm_arch_vcpu_ioctl(struct file *filp, · line 848
Quoted 4 lines
arch/arm/kvm/arm.c +4
-0+ } + +if (is_dirty) +kvm_tlb_flush_vm (kvm);MZ> This can be easily abstracted to be a
kvm_flush_remote_tlbson MZ> x86, and a HW broadcast on ARM.
Quoted 5 lines
Kvm_tlb_flush_vm()is replaced withkvm_flush_remote_tlbs()I made that function weak and declared a ARM version, in armmmu.cThe current version sends IPIs to vCPU running the guest, ARMv7+ does not need that. Is that ok?
We don't need any of that. You should have akvm_flush_remote_tlb that is essentially a call to kvm_tlb_flush_vmid.
Quoted 16 lines
arch/arm/kvm/mmu.c void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot) · line 810
Quoted 8 lines
arch/arm/kvm/mmu.c +6
-0+/* walk set bits in the mask and write protect corresponding pages */ +while (mask) { + ipa = (slot->base_gfn + gfn_offset +__ffs (mask)) <<PAGE_SHIFT ; + pgd = pgdp +pgd_index (ipa); +if (!pgd_present (*pgd)) + goto update_mask;MZ> I think something is wrong in your logic. If there is no PGD, it MZ> means a whole 1GB isn't present. Yet you're just clearing one bit MZ> from the mask and doing it again. As you're only looking at MZ>
BITS_PER_LONGMZ> contiguous pages at a time, it is likely that MZ> the same thing will happen for the other pages, and you're just MZ> wasting precious CPU cycles here.
Quoted 3 lines
Yes this is grossly inefficient, I updated it to walk ptes only, after first determining if it straddles a pmd.
Should mostly be pte walks with maybe one PMD walk but unlikely.
Yes, that's what I thought.
Quoted 8 lines
arch/arm/kvm/mmu.c void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot) · line 823
Quoted 3 lines
arch/arm/kvm/mmu.c +2
-0+ new_pte =pfn_pte (pte_pfn (*pte),PAGE_S2 ); + *pte = new_pte;MZ> I'd like to see these two lines in a separate function (something MZ> like "
stage2_mark_pte_ro")...Yes ok.
Thanks.
M.
Signature · 1 line
Jazz is not dead. It just smells funny.