| Message ID | 20170908172422.rxmhwd2vl6eye2or@docker (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, CC Juergen, Boris and Stefano. On 08/09/17 18:24, Tycho Andersen wrote: > On Fri, Sep 08, 2017 at 12:53:47AM -0700, Christoph Hellwig wrote: >>> +/* >>> + * Lookup the page table entry for a virtual address and return a pointer to >>> + * the entry. Based on x86 tree. >>> + */ >>> +static pte_t *lookup_address(unsigned long addr) >> >> Seems like this should be moved to common arm64 mm code and used by >> kernel_page_present. > > Sounds good, I'll include something like the patch below in the next > series. > > Unfortunately, adding an implementation of lookup_address seems to be > slightly more complicated than necessary, because of the xen piece. We > have to define lookup_address() with the level parameter, but it's not > obvious to me to name the page levels. So for now I've just left it as > a WARN() if someone supplies it. > > It seems like xen still does need this to be defined, because if I > define it without level: > > drivers/xen/xenbus/xenbus_client.c: In function ‘xenbus_unmap_ring_vfree_pv’: > drivers/xen/xenbus/xenbus_client.c:760:4: error: too many arguments to function ‘lookup_address’ > lookup_address(addr, &level)).maddr; > ^~~~~~~~~~~~~~ > In file included from ./arch/arm64/include/asm/page.h:37:0, > from ./include/linux/mmzone.h:20, > from ./include/linux/gfp.h:5, > from ./include/linux/mm.h:9, > from drivers/xen/xenbus/xenbus_client.c:33: > ./arch/arm64/include/asm/pgtable-types.h:67:15: note: declared here > extern pte_t *lookup_address(unsigned long addr); > ^~~~~~~~~~~~~~ > > I've cc-d the xen folks, maybe they can suggest a way to untangle it? > Alternatively, if someone can suggest a good naming scheme for the > page levels, I can just do that. The implementation of lookup_address(...) on ARM for Xen (see include/xen/arm/page.h) is just a BUG(). This is because this code should never be called (only used for x86 PV code). Furthermore, xenbus client does not use at all the level. It is just to cope with the x86 version of lookup_address. So one way to solve the problem would be to introduce xen_lookup_address(addr) that would be implemented as: - on x86 unsigned int level; return lookup_address(addr, &level).maddr; - on ARM BUG(); With that there would be no prototype clash and avoid introducing a level parameter. Cheers,
On 14/09/17 12:41, Julien Grall wrote: > Hi, > > CC Juergen, Boris and Stefano. > > On 08/09/17 18:24, Tycho Andersen wrote: >> On Fri, Sep 08, 2017 at 12:53:47AM -0700, Christoph Hellwig wrote: >>>> +/* >>>> + * Lookup the page table entry for a virtual address and return a >>>> pointer to >>>> + * the entry. Based on x86 tree. >>>> + */ >>>> +static pte_t *lookup_address(unsigned long addr) >>> >>> Seems like this should be moved to common arm64 mm code and used by >>> kernel_page_present. >> >> Sounds good, I'll include something like the patch below in the next >> series. >> >> Unfortunately, adding an implementation of lookup_address seems to be >> slightly more complicated than necessary, because of the xen piece. We >> have to define lookup_address() with the level parameter, but it's not >> obvious to me to name the page levels. So for now I've just left it as >> a WARN() if someone supplies it. >> >> It seems like xen still does need this to be defined, because if I >> define it without level: >> >> drivers/xen/xenbus/xenbus_client.c: In function >> ‘xenbus_unmap_ring_vfree_pv’: >> drivers/xen/xenbus/xenbus_client.c:760:4: error: too many arguments to >> function ‘lookup_address’ >> lookup_address(addr, &level)).maddr; >> ^~~~~~~~~~~~~~ >> In file included from ./arch/arm64/include/asm/page.h:37:0, >> from ./include/linux/mmzone.h:20, >> from ./include/linux/gfp.h:5, >> from ./include/linux/mm.h:9, >> from drivers/xen/xenbus/xenbus_client.c:33: >> ./arch/arm64/include/asm/pgtable-types.h:67:15: note: declared here >> extern pte_t *lookup_address(unsigned long addr); >> ^~~~~~~~~~~~~~ >> >> I've cc-d the xen folks, maybe they can suggest a way to untangle it? >> Alternatively, if someone can suggest a good naming scheme for the >> page levels, I can just do that. > > The implementation of lookup_address(...) on ARM for Xen (see > include/xen/arm/page.h) is just a BUG(). This is because this code > should never be called (only used for x86 PV code). > > Furthermore, xenbus client does not use at all the level. It is just to > cope with the x86 version of lookup_address. > > So one way to solve the problem would be to introduce > xen_lookup_address(addr) that would be implemented as: > - on x86 > unsigned int level; > > return lookup_address(addr, &level).maddr; > - on ARM > BUG(); > > With that there would be no prototype clash and avoid introducing a > level parameter. I'd rather add some #ifdef CONFIG_XEN_PV and remove the *_pv functions from ARM completely this way. I'm sending a patch soon... Juergen
diff --git a/arch/arm64/include/asm/pgtable-types.h b/arch/arm64/include/asm/pgtable-types.h index 345a072b5856..fad3db5a673f 100644 --- a/arch/arm64/include/asm/pgtable-types.h +++ b/arch/arm64/include/asm/pgtable-types.h @@ -64,4 +64,6 @@ typedef struct { pteval_t pgprot; } pgprot_t; #include <asm-generic/5level-fixup.h> #endif +extern pte_t *lookup_address(unsigned long addr, unsigned int *level); + #endif /* __ASM_PGTABLE_TYPES_H */ diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c index a682a0a2a0fa..437a12485873 100644 --- a/arch/arm64/mm/pageattr.c +++ b/arch/arm64/mm/pageattr.c @@ -138,6 +138,32 @@ int set_memory_valid(unsigned long addr, int numpages, int enable) __pgprot(PTE_VALID)); } +pte_t *lookup_address(unsigned long addr, unsigned int *level) +{ + pgd_t *pgd; + pud_t *pud; + pmd_t *pmd; + + if (unlikely(level)) { + WARN(1, "level unused on arm64\n"); + *level = 0; + } + + pgd = pgd_offset_k(addr); + if (pgd_none(*pgd)) + return NULL; + + pud = pud_offset(pgd, addr); + if (pud_none(*pud)) + return NULL; + + pmd = pmd_offset(pud, addr); + if (pmd_none(*pmd)) + return NULL; + + return pte_offset_kernel(pmd, addr); +} + #ifdef CONFIG_DEBUG_PAGEALLOC void __kernel_map_pages(struct page *page, int numpages, int enable) { @@ -156,29 +182,12 @@ void __kernel_map_pages(struct page *page, int numpages, int enable) */ bool kernel_page_present(struct page *page) { - pgd_t *pgd; - pud_t *pud; - pmd_t *pmd; - pte_t *pte; unsigned long addr = (unsigned long)page_address(page); + pte_t *pte = lookup_address(addr); - pgd = pgd_offset_k(addr); - if (pgd_none(*pgd)) - return false; - - pud = pud_offset(pgd, addr); - if (pud_none(*pud)) - return false; - if (pud_sect(*pud)) - return true; - - pmd = pmd_offset(pud, addr); - if (pmd_none(*pmd)) + if (!pte) return false; - if (pmd_sect(*pmd)) - return true; - pte = pte_offset_kernel(pmd, addr); return pte_valid(*pte); } #endif /* CONFIG_HIBERNATION */ diff --git a/include/xen/arm/page.h b/include/xen/arm/page.h index 415dbc6e43fd..6adc2a955340 100644 --- a/include/xen/arm/page.h +++ b/include/xen/arm/page.h @@ -84,16 +84,6 @@ static inline xmaddr_t arbitrary_virt_to_machine(void *vaddr) BUG(); } -/* TODO: this shouldn't be here but it is because the frontend drivers - * are using it (its rolled in headers) even though we won't hit the code path. - * So for right now just punt with this. - */ -static inline pte_t *lookup_address(unsigned long address, unsigned int *level) -{ - BUG(); - return NULL; -} - extern int set_foreign_p2m_mapping(struct gnttab_map_grant_ref *map_ops, struct gnttab_map_grant_ref *kmap_ops, struct page **pages, unsigned int count);