| Message ID | 20230928173205.2826598-3-vdonnefort@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Use folio for THP support | expand |
Hi Vincent, On 9/29/23 03:32, Vincent Donnefort wrote: > Since commit cb196ee1ef39 ("mm/huge_memory: convert > do_huge_pmd_anonymous_page() to use vma_alloc_folio()"), transparent > huge pages use folios. It enables us to check efficiently if a page is > mapped by a block simply looking at the folio size. This is saving a > page table walk. > > It is safe to read the folio in this path. We've just increased its > refcount (GUP from __gfn_to_pfn_memslot()). This will prevent attempts > of splitting the huge page. > > Signed-off-by: Vincent Donnefort <vdonnefort@google.com> > > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c > index de5e5148ef5d..69fcbcc7aca5 100644 > --- a/arch/arm64/kvm/mmu.c > +++ b/arch/arm64/kvm/mmu.c > @@ -791,51 +791,6 @@ int create_hyp_exec_mappings(phys_addr_t phys_addr, size_t size, > return 0; > } > > -static struct kvm_pgtable_mm_ops kvm_user_mm_ops = { > - /* We shouldn't need any other callback to walk the PT */ > - .phys_to_virt = kvm_host_va, > -}; > - > -static int get_user_mapping_size(struct kvm *kvm, u64 addr) > -{ > - struct kvm_pgtable pgt = { > - .pgd = (kvm_pteref_t)kvm->mm->pgd, > - .ia_bits = vabits_actual, > - .start_level = (KVM_PGTABLE_MAX_LEVELS - > - CONFIG_PGTABLE_LEVELS), > - .mm_ops = &kvm_user_mm_ops, > - }; > - unsigned long flags; > - kvm_pte_t pte = 0; /* Keep GCC quiet... */ > - u32 level = ~0; > - int ret; > - > - /* > - * Disable IRQs so that we hazard against a concurrent > - * teardown of the userspace page tables (which relies on > - * IPI-ing threads). > - */ > - local_irq_save(flags); > - ret = kvm_pgtable_get_leaf(&pgt, addr, &pte, &level); > - local_irq_restore(flags); > - > - if (ret) > - return ret; > - > - /* > - * Not seeing an error, but not updating level? Something went > - * deeply wrong... > - */ > - if (WARN_ON(level >= KVM_PGTABLE_MAX_LEVELS)) > - return -EFAULT; > - > - /* Oops, the userspace PTs are gone... Replay the fault */ > - if (!kvm_pte_valid(pte)) > - return -EAGAIN; > - > - return BIT(ARM64_HW_PGTABLE_LEVEL_SHIFT(level)); > -} > - > static struct kvm_pgtable_mm_ops kvm_s2_mm_ops = { > .zalloc_page = stage2_memcache_zalloc_page, > .zalloc_pages_exact = kvm_s2_zalloc_pages_exact, > @@ -1274,7 +1229,7 @@ static bool fault_supports_stage2_huge_mapping(struct kvm_memory_slot *memslot, > * > * Returns the size of the mapping. > */ > -static long > +static unsigned long > transparent_hugepage_adjust(struct kvm *kvm, struct kvm_memory_slot *memslot, > unsigned long hva, kvm_pfn_t *pfnp, > phys_addr_t *ipap) > @@ -1287,10 +1242,7 @@ transparent_hugepage_adjust(struct kvm *kvm, struct kvm_memory_slot *memslot, > * block map is contained within the memslot. > */ > if (fault_supports_stage2_huge_mapping(memslot, hva, PMD_SIZE)) { > - int sz = get_user_mapping_size(kvm, hva); > - > - if (sz < 0) > - return sz; > + size_t sz = folio_size(pfn_folio(pfn)); > > if (sz < PMD_SIZE) > return PAGE_SIZE; Is it possible that a tail page is returned from __gfn_to_pfn_memslot()? folio_size() doesn't work for a tail page because order 0 is returned for tail pages. It seems it's possible looking at the following call path. user_mem_abort __gfn_to_pfn_memslot // one page hva_to_pfn hva_to_pfn_fast get_user_page_fast_only get_user_pages_fast_only internal_get_user_pages_fast lockless_pages_from_mm gup_pgd_range // walk page-table for range [start, start + PAGE_SIZE] gup_p4d_range // start needn't be PMD_SIZE aligned gup_pud_range gup_pmd_range gup_huge_pmd nth_page record_subpages > @@ -1385,7 +1337,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, > kvm_pfn_t pfn; > bool logging_active = memslot_is_logging(memslot); > unsigned long fault_level = kvm_vcpu_trap_get_fault_level(vcpu); > - long vma_pagesize, fault_granule; > + unsigned long vma_pagesize, fault_granule; > enum kvm_pgtable_prot prot = KVM_PGTABLE_PROT_R; > struct kvm_pgtable *pgt; > > @@ -1530,11 +1482,6 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, > vma_pagesize = transparent_hugepage_adjust(kvm, memslot, > hva, &pfn, > &fault_ipa); > - > - if (vma_pagesize < 0) { > - ret = vma_pagesize; > - goto out_unlock; > - } > } > > if (fault_status != ESR_ELx_FSC_PERM && !device && kvm_has_mte(kvm)) { Thanks, Gavin
On Fri, Sep 29, 2023 at 05:24:00PM +1000, Gavin Shan wrote: > > + size_t sz = folio_size(pfn_folio(pfn)); > > Is it possible that a tail page is returned from __gfn_to_pfn_memslot()? folio_size() > doesn't work for a tail page because order 0 is returned for tail pages. It seems it's pfn_folio() can't return a tail page. That's the point of folios; they aren't tail pages. They're either a head page or an order-0 page.
Hi Matthew, On 9/29/23 23:07, Matthew Wilcox wrote: > On Fri, Sep 29, 2023 at 05:24:00PM +1000, Gavin Shan wrote: >>> + size_t sz = folio_size(pfn_folio(pfn)); >> >> Is it possible that a tail page is returned from __gfn_to_pfn_memslot()? folio_size() >> doesn't work for a tail page because order 0 is returned for tail pages. It seems it's > > pfn_folio() can't return a tail page. That's the point of folios; they > aren't tail pages. They're either a head page or an order-0 page. > Indeed, page_folio() already returned the head page properly. Thanks a lot for your confirm. Thanks, Gavin
On 9/29/23 03:32, Vincent Donnefort wrote: > Since commit cb196ee1ef39 ("mm/huge_memory: convert > do_huge_pmd_anonymous_page() to use vma_alloc_folio()"), transparent > huge pages use folios. It enables us to check efficiently if a page is > mapped by a block simply looking at the folio size. This is saving a > page table walk. > > It is safe to read the folio in this path. We've just increased its > refcount (GUP from __gfn_to_pfn_memslot()). This will prevent attempts > of splitting the huge page. > > Signed-off-by: Vincent Donnefort <vdonnefort@google.com> > Reviewed-by: Gavin Shan <gshan@redhat.com> > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c > index de5e5148ef5d..69fcbcc7aca5 100644 > --- a/arch/arm64/kvm/mmu.c > +++ b/arch/arm64/kvm/mmu.c > @@ -791,51 +791,6 @@ int create_hyp_exec_mappings(phys_addr_t phys_addr, size_t size, > return 0; > } > > -static struct kvm_pgtable_mm_ops kvm_user_mm_ops = { > - /* We shouldn't need any other callback to walk the PT */ > - .phys_to_virt = kvm_host_va, > -}; > - > -static int get_user_mapping_size(struct kvm *kvm, u64 addr) > -{ > - struct kvm_pgtable pgt = { > - .pgd = (kvm_pteref_t)kvm->mm->pgd, > - .ia_bits = vabits_actual, > - .start_level = (KVM_PGTABLE_MAX_LEVELS - > - CONFIG_PGTABLE_LEVELS), > - .mm_ops = &kvm_user_mm_ops, > - }; > - unsigned long flags; > - kvm_pte_t pte = 0; /* Keep GCC quiet... */ > - u32 level = ~0; > - int ret; > - > - /* > - * Disable IRQs so that we hazard against a concurrent > - * teardown of the userspace page tables (which relies on > - * IPI-ing threads). > - */ > - local_irq_save(flags); > - ret = kvm_pgtable_get_leaf(&pgt, addr, &pte, &level); > - local_irq_restore(flags); > - > - if (ret) > - return ret; > - > - /* > - * Not seeing an error, but not updating level? Something went > - * deeply wrong... > - */ > - if (WARN_ON(level >= KVM_PGTABLE_MAX_LEVELS)) > - return -EFAULT; > - > - /* Oops, the userspace PTs are gone... Replay the fault */ > - if (!kvm_pte_valid(pte)) > - return -EAGAIN; > - > - return BIT(ARM64_HW_PGTABLE_LEVEL_SHIFT(level)); > -} > - > static struct kvm_pgtable_mm_ops kvm_s2_mm_ops = { > .zalloc_page = stage2_memcache_zalloc_page, > .zalloc_pages_exact = kvm_s2_zalloc_pages_exact, > @@ -1274,7 +1229,7 @@ static bool fault_supports_stage2_huge_mapping(struct kvm_memory_slot *memslot, > * > * Returns the size of the mapping. > */ > -static long > +static unsigned long > transparent_hugepage_adjust(struct kvm *kvm, struct kvm_memory_slot *memslot, > unsigned long hva, kvm_pfn_t *pfnp, > phys_addr_t *ipap) > @@ -1287,10 +1242,7 @@ transparent_hugepage_adjust(struct kvm *kvm, struct kvm_memory_slot *memslot, > * block map is contained within the memslot. > */ > if (fault_supports_stage2_huge_mapping(memslot, hva, PMD_SIZE)) { > - int sz = get_user_mapping_size(kvm, hva); > - > - if (sz < 0) > - return sz; > + size_t sz = folio_size(pfn_folio(pfn)); > > if (sz < PMD_SIZE) > return PAGE_SIZE; > @@ -1385,7 +1337,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, > kvm_pfn_t pfn; > bool logging_active = memslot_is_logging(memslot); > unsigned long fault_level = kvm_vcpu_trap_get_fault_level(vcpu); > - long vma_pagesize, fault_granule; > + unsigned long vma_pagesize, fault_granule; > enum kvm_pgtable_prot prot = KVM_PGTABLE_PROT_R; > struct kvm_pgtable *pgt; > > @@ -1530,11 +1482,6 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, > vma_pagesize = transparent_hugepage_adjust(kvm, memslot, > hva, &pfn, > &fault_ipa); > - > - if (vma_pagesize < 0) { > - ret = vma_pagesize; > - goto out_unlock; > - } > } > > if (fault_status != ESR_ELx_FSC_PERM && !device && kvm_has_mte(kvm)) {
On 28/09/2023 18:32, Vincent Donnefort wrote: > Since commit cb196ee1ef39 ("mm/huge_memory: convert > do_huge_pmd_anonymous_page() to use vma_alloc_folio()"), transparent > huge pages use folios. It enables us to check efficiently if a page is > mapped by a block simply looking at the folio size. This is saving a > page table walk. > > It is safe to read the folio in this path. We've just increased its > refcount (GUP from __gfn_to_pfn_memslot()). This will prevent attempts > of splitting the huge page. > > Signed-off-by: Vincent Donnefort <vdonnefort@google.com> > > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c > index de5e5148ef5d..69fcbcc7aca5 100644 > --- a/arch/arm64/kvm/mmu.c > +++ b/arch/arm64/kvm/mmu.c > @@ -791,51 +791,6 @@ int create_hyp_exec_mappings(phys_addr_t phys_addr, size_t size, > return 0; > } > > -static struct kvm_pgtable_mm_ops kvm_user_mm_ops = { > - /* We shouldn't need any other callback to walk the PT */ > - .phys_to_virt = kvm_host_va, > -}; > - > -static int get_user_mapping_size(struct kvm *kvm, u64 addr) > -{ > - struct kvm_pgtable pgt = { > - .pgd = (kvm_pteref_t)kvm->mm->pgd, > - .ia_bits = vabits_actual, > - .start_level = (KVM_PGTABLE_MAX_LEVELS - > - CONFIG_PGTABLE_LEVELS), > - .mm_ops = &kvm_user_mm_ops, > - }; > - unsigned long flags; > - kvm_pte_t pte = 0; /* Keep GCC quiet... */ > - u32 level = ~0; > - int ret; > - > - /* > - * Disable IRQs so that we hazard against a concurrent > - * teardown of the userspace page tables (which relies on > - * IPI-ing threads). > - */ > - local_irq_save(flags); > - ret = kvm_pgtable_get_leaf(&pgt, addr, &pte, &level); > - local_irq_restore(flags); > - > - if (ret) > - return ret; > - > - /* > - * Not seeing an error, but not updating level? Something went > - * deeply wrong... > - */ > - if (WARN_ON(level >= KVM_PGTABLE_MAX_LEVELS)) > - return -EFAULT; > - > - /* Oops, the userspace PTs are gone... Replay the fault */ > - if (!kvm_pte_valid(pte)) > - return -EAGAIN; > - > - return BIT(ARM64_HW_PGTABLE_LEVEL_SHIFT(level)); > -} > - > static struct kvm_pgtable_mm_ops kvm_s2_mm_ops = { > .zalloc_page = stage2_memcache_zalloc_page, > .zalloc_pages_exact = kvm_s2_zalloc_pages_exact, > @@ -1274,7 +1229,7 @@ static bool fault_supports_stage2_huge_mapping(struct kvm_memory_slot *memslot, > * > * Returns the size of the mapping. > */ > -static long > +static unsigned long > transparent_hugepage_adjust(struct kvm *kvm, struct kvm_memory_slot *memslot, > unsigned long hva, kvm_pfn_t *pfnp, > phys_addr_t *ipap) > @@ -1287,10 +1242,7 @@ transparent_hugepage_adjust(struct kvm *kvm, struct kvm_memory_slot *memslot, > * block map is contained within the memslot. > */ > if (fault_supports_stage2_huge_mapping(memslot, hva, PMD_SIZE)) { > - int sz = get_user_mapping_size(kvm, hva); > - > - if (sz < 0) > - return sz; > + size_t sz = folio_size(pfn_folio(pfn)); Hi, Sorry this is an extremely late reply - I just noticed this because Marc mentioned it in another thread. This doesn't look quite right to me; just because you have a folio of a given size, that doesn't mean the whole thing is mapped into this particular address space. For example, you could have a (PMD-sized) THP that gets partially munmapped - the folio is still PMD-sized but only some of it is mapped and should be accessible to the process. Or you could have a large file-backed folio (from a filesystem that supports large folios - e.g. XFS) but the application only mapped part of the file. Perhaps I've misunderstood and those edge cases can't happen here for some reason? Thanks, Ryan > > if (sz < PMD_SIZE) > return PAGE_SIZE; > @@ -1385,7 +1337,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, > kvm_pfn_t pfn; > bool logging_active = memslot_is_logging(memslot); > unsigned long fault_level = kvm_vcpu_trap_get_fault_level(vcpu); > - long vma_pagesize, fault_granule; > + unsigned long vma_pagesize, fault_granule; > enum kvm_pgtable_prot prot = KVM_PGTABLE_PROT_R; > struct kvm_pgtable *pgt; > > @@ -1530,11 +1482,6 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, > vma_pagesize = transparent_hugepage_adjust(kvm, memslot, > hva, &pfn, > &fault_ipa); > - > - if (vma_pagesize < 0) { > - ret = vma_pagesize; > - goto out_unlock; > - } > } > > if (fault_status != ESR_ELx_FSC_PERM && !device && kvm_has_mte(kvm)) {
On Sat, 28 Oct 2023 10:17:17 +0100, Ryan Roberts <ryan.roberts@arm.com> wrote: > > On 28/09/2023 18:32, Vincent Donnefort wrote: > > Since commit cb196ee1ef39 ("mm/huge_memory: convert > > do_huge_pmd_anonymous_page() to use vma_alloc_folio()"), transparent > > huge pages use folios. It enables us to check efficiently if a page is > > mapped by a block simply looking at the folio size. This is saving a > > page table walk. > > > > It is safe to read the folio in this path. We've just increased its > > refcount (GUP from __gfn_to_pfn_memslot()). This will prevent attempts > > of splitting the huge page. > > > > Signed-off-by: Vincent Donnefort <vdonnefort@google.com> > > > > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c > > index de5e5148ef5d..69fcbcc7aca5 100644 > > --- a/arch/arm64/kvm/mmu.c > > +++ b/arch/arm64/kvm/mmu.c > > @@ -791,51 +791,6 @@ int create_hyp_exec_mappings(phys_addr_t phys_addr, size_t size, > > return 0; > > } > > > > -static struct kvm_pgtable_mm_ops kvm_user_mm_ops = { > > - /* We shouldn't need any other callback to walk the PT */ > > - .phys_to_virt = kvm_host_va, > > -}; > > - > > -static int get_user_mapping_size(struct kvm *kvm, u64 addr) > > -{ > > - struct kvm_pgtable pgt = { > > - .pgd = (kvm_pteref_t)kvm->mm->pgd, > > - .ia_bits = vabits_actual, > > - .start_level = (KVM_PGTABLE_MAX_LEVELS - > > - CONFIG_PGTABLE_LEVELS), > > - .mm_ops = &kvm_user_mm_ops, > > - }; > > - unsigned long flags; > > - kvm_pte_t pte = 0; /* Keep GCC quiet... */ > > - u32 level = ~0; > > - int ret; > > - > > - /* > > - * Disable IRQs so that we hazard against a concurrent > > - * teardown of the userspace page tables (which relies on > > - * IPI-ing threads). > > - */ > > - local_irq_save(flags); > > - ret = kvm_pgtable_get_leaf(&pgt, addr, &pte, &level); > > - local_irq_restore(flags); > > - > > - if (ret) > > - return ret; > > - > > - /* > > - * Not seeing an error, but not updating level? Something went > > - * deeply wrong... > > - */ > > - if (WARN_ON(level >= KVM_PGTABLE_MAX_LEVELS)) > > - return -EFAULT; > > - > > - /* Oops, the userspace PTs are gone... Replay the fault */ > > - if (!kvm_pte_valid(pte)) > > - return -EAGAIN; > > - > > - return BIT(ARM64_HW_PGTABLE_LEVEL_SHIFT(level)); > > -} > > - > > static struct kvm_pgtable_mm_ops kvm_s2_mm_ops = { > > .zalloc_page = stage2_memcache_zalloc_page, > > .zalloc_pages_exact = kvm_s2_zalloc_pages_exact, > > @@ -1274,7 +1229,7 @@ static bool fault_supports_stage2_huge_mapping(struct kvm_memory_slot *memslot, > > * > > * Returns the size of the mapping. > > */ > > -static long > > +static unsigned long > > transparent_hugepage_adjust(struct kvm *kvm, struct kvm_memory_slot *memslot, > > unsigned long hva, kvm_pfn_t *pfnp, > > phys_addr_t *ipap) > > @@ -1287,10 +1242,7 @@ transparent_hugepage_adjust(struct kvm *kvm, struct kvm_memory_slot *memslot, > > * block map is contained within the memslot. > > */ > > if (fault_supports_stage2_huge_mapping(memslot, hva, PMD_SIZE)) { > > - int sz = get_user_mapping_size(kvm, hva); > > - > > - if (sz < 0) > > - return sz; > > + size_t sz = folio_size(pfn_folio(pfn)); > > Hi, > > Sorry this is an extremely late reply - I just noticed this because Marc > mentioned it in another thread. > > This doesn't look quite right to me; just because you have a folio of a given > size, that doesn't mean the whole thing is mapped into this particular address > space. For example, you could have a (PMD-sized) THP that gets partially > munmapped - the folio is still PMD-sized but only some of it is mapped and > should be accessible to the process. Or you could have a large file-backed folio > (from a filesystem that supports large folios - e.g. XFS) but the application > only mapped part of the file. > > Perhaps I've misunderstood and those edge cases can't happen here for some reason? I went ahead and applied the following hack to the *current* tree, with this patch: diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c index 482280fe22d7..de365489a62f 100644 --- a/arch/arm64/kvm/mmu.c +++ b/arch/arm64/kvm/mmu.c @@ -1291,6 +1291,10 @@ transparent_hugepage_adjust(struct kvm *kvm, struct kvm_memory_slot *memslot, */ if (fault_supports_stage2_huge_mapping(memslot, hva, PMD_SIZE)) { int sz = get_user_mapping_size(kvm, hva); + size_t fsz = folio_size(pfn_folio(pfn)); + + if (sz != fsz) + pr_err("sz = %d fsz = %ld\n", sz, fsz); if (sz < 0) return sz; and sure enough, I see the check firing under *a lot* of memory pressure: [84567.458803] sz = 4096 fsz = 2097152 [84620.166018] sz = 4096 fsz = 2097152 So indeed, folio_size() doesn't provide what we need. We absolutely need to match what is actually mapped in userspace or things may turn out to be rather ugly should the other pages that are part of the same folio be allocated somewhere else. Is that even possible? M.
[...] > > static struct kvm_pgtable_mm_ops kvm_s2_mm_ops = { > > .zalloc_page = stage2_memcache_zalloc_page, > > .zalloc_pages_exact = kvm_s2_zalloc_pages_exact, > > @@ -1274,7 +1229,7 @@ static bool fault_supports_stage2_huge_mapping(struct kvm_memory_slot *memslot, > > * > > * Returns the size of the mapping. > > */ > > -static long > > +static unsigned long > > transparent_hugepage_adjust(struct kvm *kvm, struct kvm_memory_slot *memslot, > > unsigned long hva, kvm_pfn_t *pfnp, > > phys_addr_t *ipap) > > @@ -1287,10 +1242,7 @@ transparent_hugepage_adjust(struct kvm *kvm, struct kvm_memory_slot *memslot, > > * block map is contained within the memslot. > > */ > > if (fault_supports_stage2_huge_mapping(memslot, hva, PMD_SIZE)) { > > - int sz = get_user_mapping_size(kvm, hva); > > - > > - if (sz < 0) > > - return sz; > > + size_t sz = folio_size(pfn_folio(pfn)); > > Hi, > > Sorry this is an extremely late reply - I just noticed this because Marc > mentioned it in another thread. > > This doesn't look quite right to me; just because you have a folio of a given > size, that doesn't mean the whole thing is mapped into this particular address > space. For example, you could have a (PMD-sized) THP that gets partially > munmapped - the folio is still PMD-sized but only some of it is mapped and > should be accessible to the process. Or you could have a large file-backed folio > (from a filesystem that supports large folios - e.g. XFS) but the application > only mapped part of the file. I thought originally this would break the block and the folio with it, but a quick expriment showed it's not the case. > > Perhaps I've misunderstood and those edge cases can't happen here for some reason? And fault_supports_stage2_huge_mapping() would probably not be enough. we might end-up with a portion unmapped at stage-1 but mapped at stage-2. :-( > > Thanks, > Ryan > >
On 30/10/2023 10:40, Marc Zyngier wrote: > On Sat, 28 Oct 2023 10:17:17 +0100, > Ryan Roberts <ryan.roberts@arm.com> wrote: >> >> On 28/09/2023 18:32, Vincent Donnefort wrote: >>> Since commit cb196ee1ef39 ("mm/huge_memory: convert >>> do_huge_pmd_anonymous_page() to use vma_alloc_folio()"), transparent >>> huge pages use folios. It enables us to check efficiently if a page is >>> mapped by a block simply looking at the folio size. This is saving a >>> page table walk. >>> >>> It is safe to read the folio in this path. We've just increased its >>> refcount (GUP from __gfn_to_pfn_memslot()). This will prevent attempts >>> of splitting the huge page. >>> >>> Signed-off-by: Vincent Donnefort <vdonnefort@google.com> >>> >>> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c >>> index de5e5148ef5d..69fcbcc7aca5 100644 >>> --- a/arch/arm64/kvm/mmu.c >>> +++ b/arch/arm64/kvm/mmu.c >>> @@ -791,51 +791,6 @@ int create_hyp_exec_mappings(phys_addr_t phys_addr, size_t size, >>> return 0; >>> } >>> >>> -static struct kvm_pgtable_mm_ops kvm_user_mm_ops = { >>> - /* We shouldn't need any other callback to walk the PT */ >>> - .phys_to_virt = kvm_host_va, >>> -}; >>> - >>> -static int get_user_mapping_size(struct kvm *kvm, u64 addr) >>> -{ >>> - struct kvm_pgtable pgt = { >>> - .pgd = (kvm_pteref_t)kvm->mm->pgd, >>> - .ia_bits = vabits_actual, >>> - .start_level = (KVM_PGTABLE_MAX_LEVELS - >>> - CONFIG_PGTABLE_LEVELS), >>> - .mm_ops = &kvm_user_mm_ops, >>> - }; >>> - unsigned long flags; >>> - kvm_pte_t pte = 0; /* Keep GCC quiet... */ >>> - u32 level = ~0; >>> - int ret; >>> - >>> - /* >>> - * Disable IRQs so that we hazard against a concurrent >>> - * teardown of the userspace page tables (which relies on >>> - * IPI-ing threads). >>> - */ >>> - local_irq_save(flags); >>> - ret = kvm_pgtable_get_leaf(&pgt, addr, &pte, &level); >>> - local_irq_restore(flags); >>> - >>> - if (ret) >>> - return ret; >>> - >>> - /* >>> - * Not seeing an error, but not updating level? Something went >>> - * deeply wrong... >>> - */ >>> - if (WARN_ON(level >= KVM_PGTABLE_MAX_LEVELS)) >>> - return -EFAULT; >>> - >>> - /* Oops, the userspace PTs are gone... Replay the fault */ >>> - if (!kvm_pte_valid(pte)) >>> - return -EAGAIN; >>> - >>> - return BIT(ARM64_HW_PGTABLE_LEVEL_SHIFT(level)); >>> -} >>> - >>> static struct kvm_pgtable_mm_ops kvm_s2_mm_ops = { >>> .zalloc_page = stage2_memcache_zalloc_page, >>> .zalloc_pages_exact = kvm_s2_zalloc_pages_exact, >>> @@ -1274,7 +1229,7 @@ static bool fault_supports_stage2_huge_mapping(struct kvm_memory_slot *memslot, >>> * >>> * Returns the size of the mapping. >>> */ >>> -static long >>> +static unsigned long >>> transparent_hugepage_adjust(struct kvm *kvm, struct kvm_memory_slot *memslot, >>> unsigned long hva, kvm_pfn_t *pfnp, >>> phys_addr_t *ipap) >>> @@ -1287,10 +1242,7 @@ transparent_hugepage_adjust(struct kvm *kvm, struct kvm_memory_slot *memslot, >>> * block map is contained within the memslot. >>> */ >>> if (fault_supports_stage2_huge_mapping(memslot, hva, PMD_SIZE)) { >>> - int sz = get_user_mapping_size(kvm, hva); >>> - >>> - if (sz < 0) >>> - return sz; >>> + size_t sz = folio_size(pfn_folio(pfn)); >> >> Hi, >> >> Sorry this is an extremely late reply - I just noticed this because Marc >> mentioned it in another thread. >> >> This doesn't look quite right to me; just because you have a folio of a given >> size, that doesn't mean the whole thing is mapped into this particular address >> space. For example, you could have a (PMD-sized) THP that gets partially >> munmapped - the folio is still PMD-sized but only some of it is mapped and >> should be accessible to the process. Or you could have a large file-backed folio >> (from a filesystem that supports large folios - e.g. XFS) but the application >> only mapped part of the file. >> >> Perhaps I've misunderstood and those edge cases can't happen here for some reason? > > I went ahead and applied the following hack to the *current* tree, > with this patch: > > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c > index 482280fe22d7..de365489a62f 100644 > --- a/arch/arm64/kvm/mmu.c > +++ b/arch/arm64/kvm/mmu.c > @@ -1291,6 +1291,10 @@ transparent_hugepage_adjust(struct kvm *kvm, struct kvm_memory_slot *memslot, > */ > if (fault_supports_stage2_huge_mapping(memslot, hva, PMD_SIZE)) { > int sz = get_user_mapping_size(kvm, hva); > + size_t fsz = folio_size(pfn_folio(pfn)); > + > + if (sz != fsz) > + pr_err("sz = %d fsz = %ld\n", sz, fsz); > > if (sz < 0) > return sz; > > and sure enough, I see the check firing under *a lot* of memory > pressure: > > [84567.458803] sz = 4096 fsz = 2097152 > [84620.166018] sz = 4096 fsz = 2097152 > > So indeed, folio_size() doesn't provide what we need. We absolutely > need to match what is actually mapped in userspace or things may turn > out to be rather ugly should the other pages that are part of the same > folio be allocated somewhere else. Is that even possible? Yes, I think so. One such possibility is: 1. user space maps a PMD-sized THP 2. user space does a partial munmap - some of the PMD-sized folio is now PTE-mapped - folio is on the deferred split list 3. user space creates a VM covering this memory - new faultly logic incorrectly determines its a PMD mapping, so PMD maps the whole folio 4. memory pressure causes folios on the deferred split queue to be split - Unless you take an _entire_mapcount ref when you map. But even then, I doubt the deferred split queue will check that because once unamapped it should be impossible to remap that piece of the folio. 5. The "unampped" pages of the folio we just split gets given back to the buddy - I guess if KVM took a ref then the page might not be given back to the buddy which would at least prevent this becoming a security issue 6. Something else allocates that page... A less severe scenario might involve mremap, where part of the THP is mremapped in user space so its not all contiguous, but with your logic, the VM will see it as contiguous. > > M. >
On 30/10/2023 10:47, Vincent Donnefort wrote: > [...] > >>> static struct kvm_pgtable_mm_ops kvm_s2_mm_ops = { >>> .zalloc_page = stage2_memcache_zalloc_page, >>> .zalloc_pages_exact = kvm_s2_zalloc_pages_exact, >>> @@ -1274,7 +1229,7 @@ static bool fault_supports_stage2_huge_mapping(struct kvm_memory_slot *memslot, >>> * >>> * Returns the size of the mapping. >>> */ >>> -static long >>> +static unsigned long >>> transparent_hugepage_adjust(struct kvm *kvm, struct kvm_memory_slot *memslot, >>> unsigned long hva, kvm_pfn_t *pfnp, >>> phys_addr_t *ipap) >>> @@ -1287,10 +1242,7 @@ transparent_hugepage_adjust(struct kvm *kvm, struct kvm_memory_slot *memslot, >>> * block map is contained within the memslot. >>> */ >>> if (fault_supports_stage2_huge_mapping(memslot, hva, PMD_SIZE)) { >>> - int sz = get_user_mapping_size(kvm, hva); >>> - >>> - if (sz < 0) >>> - return sz; >>> + size_t sz = folio_size(pfn_folio(pfn)); >> >> Hi, >> >> Sorry this is an extremely late reply - I just noticed this because Marc >> mentioned it in another thread. >> >> This doesn't look quite right to me; just because you have a folio of a given >> size, that doesn't mean the whole thing is mapped into this particular address >> space. For example, you could have a (PMD-sized) THP that gets partially >> munmapped - the folio is still PMD-sized but only some of it is mapped and >> should be accessible to the process. Or you could have a large file-backed folio >> (from a filesystem that supports large folios - e.g. XFS) but the application >> only mapped part of the file. > > I thought originally this would break the block and the folio with it, but a > quick expriment showed it's not the case. That's not how it works unfortunately. Even if we wanted to do this in the common case (which we might in future for anon memory) there are edge cases where splitting the folio can fail (GUP). And it wouldn't make sense to split for file-backed case since those folios may be mapped into other processes. > >> >> Perhaps I've misunderstood and those edge cases can't happen here for some reason? > > And fault_supports_stage2_huge_mapping() would probably not be enough. we might > end-up with a portion unmapped at stage-1 but mapped at stage-2. :-( Yes. Or even remapped if you consider mremapping a portion of the THP. > >> >> Thanks, >> Ryan >> >>
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c index de5e5148ef5d..69fcbcc7aca5 100644 --- a/arch/arm64/kvm/mmu.c +++ b/arch/arm64/kvm/mmu.c @@ -791,51 +791,6 @@ int create_hyp_exec_mappings(phys_addr_t phys_addr, size_t size, return 0; } -static struct kvm_pgtable_mm_ops kvm_user_mm_ops = { - /* We shouldn't need any other callback to walk the PT */ - .phys_to_virt = kvm_host_va, -}; - -static int get_user_mapping_size(struct kvm *kvm, u64 addr) -{ - struct kvm_pgtable pgt = { - .pgd = (kvm_pteref_t)kvm->mm->pgd, - .ia_bits = vabits_actual, - .start_level = (KVM_PGTABLE_MAX_LEVELS - - CONFIG_PGTABLE_LEVELS), - .mm_ops = &kvm_user_mm_ops, - }; - unsigned long flags; - kvm_pte_t pte = 0; /* Keep GCC quiet... */ - u32 level = ~0; - int ret; - - /* - * Disable IRQs so that we hazard against a concurrent - * teardown of the userspace page tables (which relies on - * IPI-ing threads). - */ - local_irq_save(flags); - ret = kvm_pgtable_get_leaf(&pgt, addr, &pte, &level); - local_irq_restore(flags); - - if (ret) - return ret; - - /* - * Not seeing an error, but not updating level? Something went - * deeply wrong... - */ - if (WARN_ON(level >= KVM_PGTABLE_MAX_LEVELS)) - return -EFAULT; - - /* Oops, the userspace PTs are gone... Replay the fault */ - if (!kvm_pte_valid(pte)) - return -EAGAIN; - - return BIT(ARM64_HW_PGTABLE_LEVEL_SHIFT(level)); -} - static struct kvm_pgtable_mm_ops kvm_s2_mm_ops = { .zalloc_page = stage2_memcache_zalloc_page, .zalloc_pages_exact = kvm_s2_zalloc_pages_exact, @@ -1274,7 +1229,7 @@ static bool fault_supports_stage2_huge_mapping(struct kvm_memory_slot *memslot, * * Returns the size of the mapping. */ -static long +static unsigned long transparent_hugepage_adjust(struct kvm *kvm, struct kvm_memory_slot *memslot, unsigned long hva, kvm_pfn_t *pfnp, phys_addr_t *ipap) @@ -1287,10 +1242,7 @@ transparent_hugepage_adjust(struct kvm *kvm, struct kvm_memory_slot *memslot, * block map is contained within the memslot. */ if (fault_supports_stage2_huge_mapping(memslot, hva, PMD_SIZE)) { - int sz = get_user_mapping_size(kvm, hva); - - if (sz < 0) - return sz; + size_t sz = folio_size(pfn_folio(pfn)); if (sz < PMD_SIZE) return PAGE_SIZE; @@ -1385,7 +1337,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, kvm_pfn_t pfn; bool logging_active = memslot_is_logging(memslot); unsigned long fault_level = kvm_vcpu_trap_get_fault_level(vcpu); - long vma_pagesize, fault_granule; + unsigned long vma_pagesize, fault_granule; enum kvm_pgtable_prot prot = KVM_PGTABLE_PROT_R; struct kvm_pgtable *pgt; @@ -1530,11 +1482,6 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, vma_pagesize = transparent_hugepage_adjust(kvm, memslot, hva, &pfn, &fault_ipa); - - if (vma_pagesize < 0) { - ret = vma_pagesize; - goto out_unlock; - } } if (fault_status != ESR_ELx_FSC_PERM && !device && kvm_has_mte(kvm)) {
Since commit cb196ee1ef39 ("mm/huge_memory: convert do_huge_pmd_anonymous_page() to use vma_alloc_folio()"), transparent huge pages use folios. It enables us to check efficiently if a page is mapped by a block simply looking at the folio size. This is saving a page table walk. It is safe to read the folio in this path. We've just increased its refcount (GUP from __gfn_to_pfn_memslot()). This will prevent attempts of splitting the huge page. Signed-off-by: Vincent Donnefort <vdonnefort@google.com>