Messages in this thread |  | | Subject | Re: [PATCH v5 1/2] mm: make dev_pagemap_mapping_shift() externally visible | From | Barret Rhoden <> | Date | Mon, 16 Dec 2019 12:59:53 -0500 |
| |
On 12/13/19 12:47 PM, Sean Christopherson wrote: >> +unsigned long dev_pagemap_mapping_shift(unsigned long address, >> + struct mm_struct *mm) >> +{ >> + pgd_t *pgd; >> + p4d_t *p4d; >> + pud_t *pud; >> + pmd_t *pmd; >> + pte_t *pte; >> + >> + pgd = pgd_offset(mm, address); >> + if (!pgd_present(*pgd)) >> + return 0; >> + p4d = p4d_offset(pgd, address); >> + if (!p4d_present(*p4d)) >> + return 0; >> + pud = pud_offset(p4d, address); >> + if (!pud_present(*pud)) >> + return 0; >> + if (pud_devmap(*pud)) >> + return PUD_SHIFT; >> + pmd = pmd_offset(pud, address); >> + if (!pmd_present(*pmd)) >> + return 0; >> + if (pmd_devmap(*pmd)) >> + return PMD_SHIFT; >> + pte = pte_offset_map(pmd, address); >> + if (!pte_present(*pte)) >> + return 0; >> + if (pte_devmap(*pte)) >> + return PAGE_SHIFT; >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(dev_pagemap_mapping_shift); > > This is basically a rehash of lookup_address_in_pgd(), and doesn't provide > exactly what KVM needs. E.g. KVM works with levels instead of shifts, and > it would be nice to provide the pte so that KVM can sanity check that the > pfn from this walk matches the pfn it plans on mapping.
One minor issue is that the levels for lookup_address_in_pgd() and for KVM differ in name, although not in value. lookup uses PG_LEVEL_4K = 1. KVM uses PT_PAGE_TABLE_LEVEL = 1. The enums differ a little too: x86 has a name for a 512G page, etc. It's all in arch/x86.
Does KVM-x86 need its own names for the levels? If not, I could convert the PT_PAGE_TABLE_* stuff to PG_LEVEL_* stuff.
> > Instead of exporting dev_pagemap_mapping_shift(), what about replacing it > with a patch to introduce lookup_address_mm() and export that? > > dev_pagemap_mapping_shift() could then wrap the new helper (if you want),
I might hold off on that for now, since that helper is used outside of x86, and I don't know if 'level' makes sense outside of x86.
Thanks,
Barret
|  |