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

[Giulio Benetti]

Hi Arnd,

On 18/10/22 09:03, Arnd Bergmann wrote:
> 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

Thanks for poiting, I was unaware of this. I'll point it for sure.

> 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?

No to be honest. But in my case I've met ZERO_PAGE() calling in sdhci
driver. And as stated on the ML link above:
```
But I wonder if it's safe for noMMU architectures to go on without a
working ZERO_PAGE(0).  It has uses scattered throughout the tree, in
drivers, fs, crypto and more, and it's not at all obvious (to me) that
they all depend on CONFIG_MMU.
```
And I've found this driver that requires it and probably is not the last
since imxrt support is not complete.

> I can see that microblaze-nommu used BUG() in ZERO_PAGE(), so at
> whenever microblaze last worked, we clearly did not call it.

This probably means that microblaze-nommu doesn't use drivers or other
subsystems that require ZERO_PAGE().

> > +#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.

I was about doing it, but then I tought to move one piece at a time.
But yes, I can modify accordingly. That way we also avoid the early
allocation in pagint_init() since it would be a .bss array.

> 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

For them I can send patches to substitute with PAGE_ZERO(0) correctly 
adapted.

What do you think?

Thanks for reviewing.

Best regards
--
Giulio Benetti
CEO/CTO@Benetti Engineering sas