Christophe Leroy's on June 11, 2019 3:39 pm: > > > Le 10/06/2019 à 06:38, Nicholas Piggin a écrit : >> For platforms that define HAVE_ARCH_HUGE_VMAP, have vmap allow vmalloc to >> allocate huge pages and map them > > Will this be compatible with Russell's series > https://patchwork.ozlabs.org/patch/1099857/ for the implementation of > STRICT_MODULE_RWX ? > I see that apply_to_page_range() have things like BUG_ON(pud_huge(*pud)); > > Might also be an issue for arm64 as I think Russell's implementation > comes from there. Yeah you're right (and correct about arm64 problem). I'll fix that up. >> +static int vmap_hpages_range(unsigned long start, unsigned long end, >> + pgprot_t prot, struct page **pages, >> + unsigned int page_shift) >> +{ >> + BUG_ON(page_shift != PAGE_SIZE); > > Do we really need a BUG_ON() there ? What happens if this condition is > true ? If it's true then vmap_pages_range would die horribly reading off the end of the pages array thinking they are struct page pointers. I guess it could return failure. >> + return vmap_pages_range(start, end, prot, pages); >> +} >> +#endif >> + >> + >> int is_vmalloc_or_module_addr(const void *x) >> { >> /* >> @@ -462,7 +498,7 @@ struct page *vmalloc_to_page(const void *vmalloc_addr) >> { >> unsigned long addr = (unsigned long) vmalloc_addr; >> struct page *page = NULL; >> - pgd_t *pgd = pgd_offset_k(addr); >> + pgd_t *pgd; >> p4d_t *p4d; >> pud_t *pud; >> pmd_t *pmd; >> @@ -474,27 +510,38 @@ struct page *vmalloc_to_page(const void *vmalloc_addr) >> */ >> VIRTUAL_BUG_ON(!is_vmalloc_or_module_addr(vmalloc_addr)); >> >> + pgd = pgd_offset_k(addr); >> if (pgd_none(*pgd)) >> return NULL; >> + >> p4d = p4d_offset(pgd, addr); >> if (p4d_none(*p4d)) >> return NULL; >> - pud = pud_offset(p4d, addr); >> +#ifdef CONFIG_HAVE_ARCH_HUGE_VMAP > > Do we really need that ifdef ? Won't p4d_large() always return 0 when is > not set ? > Otherwise, could we use IS_ENABLED(CONFIG_HAVE_ARCH_HUGE_VMAP) instead ? > > Same several places below. Possibly some of them are not defined without HAVE_ARCH_HUGE_VMAP I think. I'll try to apply this pattern as much as possible. >> @@ -2541,14 +2590,17 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask, >> pgprot_t prot, int node) >> { >> struct page **pages; >> + unsigned long addr = (unsigned long)area->addr; >> + unsigned long size = get_vm_area_size(area); >> + unsigned int page_shift = area->page_shift; >> + unsigned int shift = page_shift + PAGE_SHIFT; >> unsigned int nr_pages, array_size, i; >> const gfp_t nested_gfp = (gfp_mask & GFP_RECLAIM_MASK) | __GFP_ZERO; >> const gfp_t alloc_mask = gfp_mask | __GFP_NOWARN; >> const gfp_t highmem_mask = (gfp_mask & (GFP_DMA | GFP_DMA32)) ? >> - 0 : >> - __GFP_HIGHMEM; >> + 0 : __GFP_HIGHMEM; > > This patch is already quite big, shouldn't this kind of unrelated > cleanups be in another patch ? Okay, 2 against 1. I'll minimise changes like this. Thanks, Nick
