Re: [PATCH v5 1/2] mm: make dev_pagemap_mapping_shift() externally visible

[Barret Rhoden]

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