Re: [PATCH] ARM: mm: fix no-MMU ZERO_PAGE() implementation

[Arnd Bergmann]

On Tue, Oct 18, 2022, at 1:37 AM, Giulio Benetti wrote:
> Actually in no-MMU SoCs(i.e. i.MXRT) ZERO_PAGE(vaddr) expands to
> ```
> virt_to_page(0)
> ```
> that in order expands to:
> ```
> pfn_to_page(virt_to_pfn(0))
> ```
> and then virt_to_pfn(0) to:
> ```
> #define virt_to_pfn(0) \
> 	((((unsigned long)(0) - PAGE_OFFSET) >> PAGE_SHIFT) + \
> 	 PHYS_PFN_OFFSET)
> ```
> where PAGE_OFFSET and PHYS_PFN_OFFSET are the DRAM offset(0x80000000) and
> PAGE_SHIFT is 12. This way we obtain 16MB(0x01000000) summed to the base of
> DRAM(0x80000000).
> When ZERO_PAGE(0) is then used, for example in bio_add_page(), the page
> gets an address that is out of DRAM bounds.
> So instead of using fake virtual page 0 let's allocate a dedicated
> zero_page during paging_init() and assign it to a global 'struct page *
> empty_zero_page' the same way mmu.c does. Then let's move ZERO_PAGE()
> definition to the top of pgtable.h to be in common between mmu.c and
> nommu.c.
>
> Signed-off-by: Giulio Benetti <giulio.benetti@xxxxxxxxxxxxxxxxxxxxxx>

Maybe mention commit dc068f462179 ("m68knommu: set ZERO_PAGE() to the
allocated zeroed page") for the commit that fixed this first,
as well as the previous discussion at
https://lore.kernel.org/linux-m68k/2a462b23-5b8e-bbf4-ec7d-778434a3b9d7@xxxxxxxxxx/T/#m1266ceb63ad140743174d6b3070364d3c9a5179b

It looks like we dropped the ball on this when it came up last.
I'm also not sure when we started requiring this, any idea?
I can see that microblaze-nommu used BUG() in ZERO_PAGE(), so at
whenever microblaze last worked, we clearly did not call it.

> +#ifndef __ASSEMBLY__
> +/*
> + * ZERO_PAGE is a global shared page that is always zero: used
> + * for zero-mapped memory areas etc..
> + */
> +extern struct page *empty_zero_page;
> +#define ZERO_PAGE(vaddr)	(empty_zero_page)
> +#endif

In addition to your fix, I see that arm is the only architecture
that defines 'empty_zero_page' as a pointer to the page, when
everything else just makes it a pointer to the data itself,
or an 'extern char empty_zero_page[]' array, which we may want
to change for consistency.

There are three references to empty_zero_page in architecture
independent code, and while we don't seem to use any of them
on Arm, they would clearly be wrong if we did:

drivers/acpi/scan.c:#define INVALID_ACPI_HANDLE ((acpi_handle)empty_zero_page)
drivers/spi/spi-fsl-cpm.c:      mspi->dma_dummy_tx = dma_map_single(dev, empty_zero_page, PAGE_SIZE,
include/linux/raid/pq.h:# define raid6_empty_zero_page empty_zero_page

     Arnd