arm/arm64: KVM: Fix and refactor unmap_range
Subsystems (3)
PATCH v3 1/1 arm/arm64: KVM: Fix and refactor unmap_range
unmap_range() was utterly broken, to quote Marc, and broke in all sorts of situations. It was also quite complicated to follow and didn't follow the usual scheme of having a separate iterating function for each level of page tables.
Address this by refactoring the code and introduce a pgd_clear() function.
Tested on TC2 with/without THP and limited testing on the v8 Foundation Model.
---
Changes since v2:
- Don't call clear_p[mu]d_entry for huge pmd's or pud's
- Use phys_addr_t instead of unsigned long longs
- Change invalid 'pid' reference to pud
Changes since v1:
- Don't define custom __unused but reuse __maybe_unused
[ I kept the acked-by and reviewed-by tags because I felt the changes
were small enough to do so - if someone disagrees I'll be happy to remove them, thanks. ]
arch/arm/include/asm/kvm_mmu.h | 12 +++ arch/arm/kvm/mmu.c | 157 +++++++++++++++++++++------------------ arch/arm64/include/asm/kvm_mmu.h | 15 ++++ 3 files changed, 111 insertions(+), 73 deletions(-)
arch/arm/include/asm/kvm_mmu.h +12 -0
@@ -127,6 +127,18 @@ static inline void kvm_set_s2pmd_writable(pmd_t *pmd) (__boundary -1 < (end) -1 )? __boundary: (end); \ }) +static inline bool kvm_page_empty (void *ptr) +{ +struct page *ptr_page =virt_to_page (ptr); +return page_count (ptr_page) ==1 ; +} + + +#define kvm_pte_table_empty (ptep) kvm_page_empty(ptep) +#define kvm_pmd_table_empty (pmdp) kvm_page_empty(pmdp) +#define kvm_pud_table_empty (pudp) (0) + +struct kvm ;#define kvm_flush_dcache_to_poc (a,l) __cpuc_flush_dcache_area((a), (l))
arch/arm/kvm/mmu.c +84 -73
188 lines not on this page · raw message
arch/arm64/include/asm/kvm_mmu.h +15 -0
@@ -125,6 +125,21 @@ static inline void kvm_set_s2pmd_writable(pmd_t *pmd)#define kvm_pud_addr_end (addr, end) pud_addr_end(addr, end)#define kvm_pmd_addr_end (addr, end) pmd_addr_end(addr, end) +static inline bool kvm_page_empty (void *ptr) +{ +struct page *ptr_page =virt_to_page (ptr); +return page_count (ptr_page) ==1 ; +} + +#define kvm_pte_table_empty (ptep) kvm_page_empty(ptep) +#ifndef CONFIG_ARM64_64K_PAGES +#define kvm_pmd_table_empty (pmdp) kvm_page_empty(pmdp) +#else +#define kvm_pmd_table_empty (pmdp) (0) +#endif +#define kvm_pud_table_empty (pudp) (0) + +struct kvm ;#define kvm_flush_dcache_to_poc (a,l) __flush_dcache_area((a), (l))
Signature · 1 line
1.8.5.2
PATCH v3 1/1 arm/arm64: KVM: Fix and refactor unmap_range
Hi Christoffer,
Quoted 8 lines · Christoffer Dall
unmap_range()was utterly broken, to quote Marc, and broke in all sorts of situations. It was also quite complicated to follow and didn't follow the usual scheme of having a separate iterating function for each level of page tables.Address this by refactoring the code and introduce a
pgd_clear()function.
Quoted 8 lines
Tested on TC2 with/without THP and limited testing on the v8 Foundation Model.
Reviewed-by Jungseok Lee <[email protected]>Reviewed-by Mario Smarduch <[email protected]>Acked-by Marc Zyngier <[email protected]>Signed-off-by Christoffer Dall <[email protected]>---
This looks good to me.
One minor comment below (sorry just spotted this now)...
[ ... ]
Quoted 8 lines
arch/arm/kvm/mmu.c +2
-5-static void clear_pte_entry (struct kvm *kvm,pte_t *pte,phys_addr_t addr) +static void unmap_ptes (struct kvm *kvm,pmd_t *pmd, +phys_addr_t addr,phys_addr_t end) { -if (pte_present (*pte)) { -kvm_set_pte (pte,__pte (0 )); -put_page (virt_to_page (pte)); -kvm_tlb_flush_vmid_ipa (kvm, addr);
arch/arm/kvm/mmu.c static void *mmu_memory_cache_alloc(struct kvm_mmu_memory_cache *mc) · line 133
Quoted 10 lines
arch/arm/kvm/mmu.c +9
-1- } +pte_t *pte, *start_pte; +unsigned long long start_addr = addr; + + start_pte = pte =pte_offset_kernel (pmd, addr); + do { + if (!pte_none(*pte)) { + kvm_set_pte(pte, __pte(0)); + put_page(virt_to_page(pte)); + kvm_tlb_flush_vmid_ipa(kvm, addr);
Can this hyp call be expensive if a lot of ptes are being unmapped (for 64K pages we can have 8192 ptes per page)?
If so, can they be batched together?
Cheers,
Signature · 1 line
Steve
PATCH v3 1/1 arm/arm64: KVM: Fix and refactor unmap_range
Quoted 20 lines · Steve Capper
Hi Christoffer,
Quoted 8 lines · Christoffer Dall
unmap_range()was utterly broken, to quote Marc, and broke in all sorts of situations. It was also quite complicated to follow and didn't follow the usual scheme of having a separate iterating function for each level of page tables.Address this by refactoring the code and introduce a
pgd_clear()function.Quoted 8 lines
Tested on TC2 with/without THP and limited testing on the v8 Foundation Model.
Reviewed-by Jungseok Lee <[email protected]>Reviewed-by Mario Smarduch <[email protected]>Acked-by Marc Zyngier <[email protected]>Signed-off-by Christoffer Dall <[email protected]>---
Quoted 4 lines
This looks good to me.
Reviewed-by Steve Capper <[email protected]>
thanks.
Quoted 18 lines
One minor comment below (sorry just spotted this now)...
[ ... ]
Quoted 14 lines
arch/arm/kvm/mmu.c +7
-6-static void clear_pte_entry (struct kvm *kvm,pte_t *pte,phys_addr_t addr) +static void unmap_ptes (struct kvm *kvm,pmd_t *pmd, +phys_addr_t addr,phys_addr_t end) { -if (pte_present (*pte)) { -kvm_set_pte (pte,__pte (0 )); -put_page (virt_to_page (pte)); -kvm_tlb_flush_vmid_ipa (kvm, addr); - } +pte_t *pte, *start_pte; +unsigned long long start_addr = addr; + + start_pte = pte =pte_offset_kernel (pmd, addr); + do {
Quoted 9 lines
arch/arm/kvm/mmu.c static void *mmu_memory_cache_alloc(struct kvm_mmu_memory_cache *mc) · line 133
Quoted 4 lines
arch/arm/kvm/mmu.c +4
-0+if (!pte_none (*pte)) { +kvm_set_pte (pte,__pte (0 )); +put_page (virt_to_page (pte)); +kvm_tlb_flush_vmid_ipa (kvm, addr);Can this hyp call be expensive if a lot of ptes are being unmapped (for 64K pages we can have 8192 ptes per page)?
If so, can they be batched together?
I suppose we could, we would have to add something to flush the entire TLB for that VMID on aarch64 (or that entire range) to do so.
I think it's reasonable to merge this now and apply that as an optimization. Marc?
-Christoffer
PATCH v3 1/1 arm/arm64: KVM: Fix and refactor unmap_range
Quoted 21 lines · Christoffer Dall
Quoted 20 lines · Steve Capper
Hi Christoffer,
Quoted 16 lines · Christoffer Dall
unmap_range()was utterly broken, to quote Marc, and broke in all sorts of situations. It was also quite complicated to follow and didn't follow the usual scheme of having a separate iterating function for each level of page tables.Address this by refactoring the code and introduce a
pgd_clear()function.Tested on TC2 with/without THP and limited testing on the v8 Foundation Model.
Reviewed-by Jungseok Lee <[email protected]>Reviewed-by Mario Smarduch <[email protected]>Acked-by Marc Zyngier <[email protected]>Signed-off-by Christoffer Dall <[email protected]>---
Quoted 33 lines
Quoted 4 lines
This looks good to me.
Reviewed-by Steve Capper <[email protected]>thanks.
Quoted 26 lines
One minor comment below (sorry just spotted this now)...
[ ... ]
Quoted 8 lines
arch/arm/kvm/mmu.c +2
-5-static void clear_pte_entry (struct kvm *kvm,pte_t *pte,phys_addr_t addr) +static void unmap_ptes (struct kvm *kvm,pmd_t *pmd, +phys_addr_t addr,phys_addr_t end) { -if (pte_present (*pte)) { -kvm_set_pte (pte,__pte (0 )); -put_page (virt_to_page (pte)); -kvm_tlb_flush_vmid_ipa (kvm, addr);arch/arm/kvm/mmu.c static void *mmu_memory_cache_alloc(struct kvm_mmu_memory_cache *mc) · line 133
Quoted 10 lines
arch/arm/kvm/mmu.c +9
-1- } +pte_t *pte, *start_pte; +unsigned long long start_addr = addr; + + start_pte = pte =pte_offset_kernel (pmd, addr); + do { + if (!pte_none(*pte)) { + kvm_set_pte(pte, __pte(0)); + put_page(virt_to_page(pte)); + kvm_tlb_flush_vmid_ipa(kvm, addr);Can this hyp call be expensive if a lot of ptes are being unmapped (for 64K pages we can have 8192 ptes per page)?
If so, can they be batched together?
The main problem here is that HYP doesn't have access to these pages at all. They are only mapped at EL1, so HYP cannot parse the PTE table.
Quoted 2 lines
I suppose we could, we would have to add something to flush the entire TLB for that VMID on aarch64 (or that entire range) to do so.
Yes, VMALL would be possible, but we'd have to find out pretty early if we're tearing down the whole address space, or just a single page.
Quoted 2 lines
I think it's reasonable to merge this now and apply that as an optimization. Marc?
Agreed. Batching those require a bit more thoughts and more testing. I remember heuristics about when it was more interesting to switch from single TLBI to ALL, but I'd need to deep dive into my Inbox for this.
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
PATCH v3 1/1 arm/arm64: KVM: Fix and refactor unmap_range
Quoted 65 lines · Marc Zyngier
Quoted 21 lines · Christoffer Dall
Quoted 20 lines · Steve Capper
Hi Christoffer,
Quoted 16 lines · Christoffer Dall
unmap_range()was utterly broken, to quote Marc, and broke in all sorts of situations. It was also quite complicated to follow and didn't follow the usual scheme of having a separate iterating function for each level of page tables.Address this by refactoring the code and introduce a
pgd_clear()function.Tested on TC2 with/without THP and limited testing on the v8 Foundation Model.
Reviewed-by Jungseok Lee <[email protected]>Reviewed-by Mario Smarduch <[email protected]>Acked-by Marc Zyngier <[email protected]>Signed-off-by Christoffer Dall <[email protected]>---
Quoted 33 lines
Quoted 4 lines
This looks good to me.
Reviewed-by Steve Capper <[email protected]>thanks.
Quoted 26 lines
One minor comment below (sorry just spotted this now)...
[ ... ]
Quoted 8 lines
arch/arm/kvm/mmu.c +2
-5-static void clear_pte_entry (struct kvm *kvm,pte_t *pte,phys_addr_t addr) +static void unmap_ptes (struct kvm *kvm,pmd_t *pmd, +phys_addr_t addr,phys_addr_t end) { -if (pte_present (*pte)) { -kvm_set_pte (pte,__pte (0 )); -put_page (virt_to_page (pte)); -kvm_tlb_flush_vmid_ipa (kvm, addr);arch/arm/kvm/mmu.c static void *mmu_memory_cache_alloc(struct kvm_mmu_memory_cache *mc) · line 133
Quoted 10 lines
arch/arm/kvm/mmu.c +9
-1- } +pte_t *pte, *start_pte; +unsigned long long start_addr = addr; + + start_pte = pte =pte_offset_kernel (pmd, addr); + do { + if (!pte_none(*pte)) { + kvm_set_pte(pte, __pte(0)); + put_page(virt_to_page(pte)); + kvm_tlb_flush_vmid_ipa(kvm, addr);Can this hyp call be expensive if a lot of ptes are being unmapped (for 64K pages we can have 8192 ptes per page)?
If so, can they be batched together?The main problem here is that HYP doesn't have access to these pages at all. They are only mapped at EL1, so HYP cannot parse the PTE table.
Quoted 2 lines
I suppose we could, we would have to add something to flush the entire TLB for that VMID on aarch64 (or that entire range) to do so.
Yes, VMALL would be possible, but we'd have to find out pretty early if we're tearing down the whole address space, or just a single page.
Quoted 7 lines
Quoted 2 lines
I think it's reasonable to merge this now and apply that as an optimization. Marc?
Agreed. Batching those require a bit more thoughts and more testing. I remember heuristics about when it was more interesting to switch from single TLBI to ALL, but I'd need to deep dive into my Inbox for this.
Cool, let's try to remember this as a point of investigation and I'll apply this one to -next for now.
-Christoffer