ARM: mm: fix no-MMU ZERO_PAGE() implementation

  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@benettiengineering.com>
---
 arch/arm/include/asm/pgtable-nommu.h |  6 ------
 arch/arm/include/asm/pgtable.h       | 16 +++++++++-------
 arch/arm/mm/nommu.c                  | 19 +++++++++++++++++++
 3 files changed, 28 insertions(+), 13 deletions(-)
  

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@benettiengineering.com>

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@google.com/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
  

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@benettiengineering.com>
> 
> 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@google.com/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
  

On Tue, Oct 18, 2022, at 19:44, Giulio Benetti wrote:
> 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
>
>> 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().

To clarify: microblaze-nommu support was removed two years ago,
and probably was already broken for a while before that.

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

Right, it would definitely be a separate patch, but it
can be a series of two patches. We probably wouldn't need to
backport the second patch that turns it into a static allocation.

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

That sounds like a good idea as well.

     Arnd
  

On 18/10/22 20:35, Arnd Bergmann wrote:
> On Tue, Oct 18, 2022, at 19:44, Giulio Benetti wrote:
>> 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
>>
>>> 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().
> 
> To clarify: microblaze-nommu support was removed two years ago,
> and probably was already broken for a while before that.
> 
>>> 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.
> 
> Right, it would definitely be a separate patch, but it
> can be a series of two patches. We probably wouldn't need to
> backport the second patch that turns it into a static allocation.

I've sent the patchset of 2:
https://lore.kernel.org/all/20221018222503.90118-1-giulio.benetti@benettiengineering.com/T/#t

I'm wondering if it makes sense to send a patchset for all those
architectures that have only one zero page. I've seen that for example
loongarch has more than one. But for the others I find the array
approach more linear, with less code all around and a bit faster in term
of code execution(of course really few, but better than nothing) since
that array is in .bss, so it will be zeroed earlier during a long
"memset" where assembly instructions for zeroing 8 bytes at a time are
used. What about this?

>> 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?
> 
> That sounds like a good idea as well.

I've just sent a patchset for this:
https://lore.kernel.org/all/20221018215755.33566-1-giulio.benetti@benettiengineering.com/T/#t

Best regards
  

On Wed, Oct 19, 2022, at 00:32, Giulio Benetti wrote:
> On 18/10/22 20:35, Arnd Bergmann wrote:
>> On Tue, Oct 18, 2022, at 19:44, Giulio Benetti wrote:
>>> On 18/10/22 09:03, Arnd Bergmann wrote:
>>>> 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.
>> 
>> Right, it would definitely be a separate patch, but it
>> can be a series of two patches. We probably wouldn't need to
>> backport the second patch that turns it into a static allocation.
>
> I've sent the patchset of 2:
> https://lore.kernel.org/all/20221018222503.90118-1-giulio.benetti@benettiengineering.com/T/#t
>
> I'm wondering if it makes sense to send a patchset for all those
> architectures that have only one zero page. I've seen that for example
> loongarch has more than one. But for the others I find the array
> approach more linear, with less code all around and a bit faster in term
> of code execution(of course really few, but better than nothing) since
> that array is in .bss, so it will be zeroed earlier during a long
> "memset" where assembly instructions for zeroing 8 bytes at a time are
> used. What about this?

The initial zeroing should not matter at all in terms of performance,
I think the only question is whether one wants a single zero page
to be used everywhere or one per NUMA node to give better locality
for a cache miss.

My guess is that for a system with 4KB pages, all the data
in the zero page are typically available in a CPU cache already,
so it doesn't matter, but it's possible that some machines benefit
from having per-node pages when the page size isn't tiny compared
to the typical cache sizes.

We should probably not touch this for any of the other architectures.

     Arnd
  

On Tue, Oct 18, 2022 at 09:03:01AM +0200, Arnd Bergmann wrote:
> 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.

ARM's implementation is the utterly sensible implementation IMHO.

When the only users in the kernel _were_ ZERO_PAGE() for this, which
is defined to return a struct page pointer, there was no need to make
"empty_zero_page" anything but a struct page pointer, rather than a
runtime translation from an address to a struct page.

IMHO, we should _not_ be exposing empty_zero_page to devices - we
certainly do not want the DMA API performing cache maintenance on
this page since the primary purpose of this page is to fill in
userspace BSS pages that have not been written.

ACPI's use is just to have a cookie for invalid handles, and using
the struct page pointer is good enough.

The only problem one is the RAID6 code, but that is disabled:

/* Set to 1 to use kernel-wide empty_zero_page */
#define RAID6_USE_EMPTY_ZERO_PAGE 0

#if RAID6_USE_EMPTY_ZERO_PAGE
# define raid6_empty_zero_page empty_zero_page
#else
extern const char raid6_empty_zero_page[PAGE_SIZE];
#endif

So, the only one that needs fixing is the SPI usage, which IMHO
is wrong. ARM being different finds what I consider a driver bug.
Good for 32-bit ARM. :)
  

On Wed, Oct 19, 2022, at 11:09, Russell King (Oracle) wrote:

> When the only users in the kernel _were_ ZERO_PAGE() for this, which
> is defined to return a struct page pointer, there was no need to make
> "empty_zero_page" anything but a struct page pointer, rather than a
> runtime translation from an address to a struct page.

Fair enough.

> IMHO, we should _not_ be exposing empty_zero_page to devices - we
> certainly do not want the DMA API performing cache maintenance on
> this page since the primary purpose of this page is to fill in
> userspace BSS pages that have not been written.

It should be easy enough to not expose it by renaming the
symbol to something other than empty_zero_page. That way,
any incorrect users that may come up in the future would
at least result in a build failure instead of runtime
data corruption.

> So, the only one that needs fixing is the SPI usage, which IMHO
> is wrong. ARM being different finds what I consider a driver bug.
> Good for 32-bit ARM. :)

The SPI driver is powerpc specific, so it's also not going to
get hit. 

    Arnd
  

On 19/10/22 09:00, Arnd Bergmann wrote:
> On Wed, Oct 19, 2022, at 00:32, Giulio Benetti wrote:
>> On 18/10/22 20:35, Arnd Bergmann wrote:
>>> On Tue, Oct 18, 2022, at 19:44, Giulio Benetti wrote:
>>>> On 18/10/22 09:03, Arnd Bergmann wrote:
>>>>> 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.
>>>
>>> Right, it would definitely be a separate patch, but it
>>> can be a series of two patches. We probably wouldn't need to
>>> backport the second patch that turns it into a static allocation.
>>
>> I've sent the patchset of 2:
>> https://lore.kernel.org/all/20221018222503.90118-1-giulio.benetti@benettiengineering.com/T/#t
>>
>> I'm wondering if it makes sense to send a patchset for all those
>> architectures that have only one zero page. I've seen that for example
>> loongarch has more than one. But for the others I find the array
>> approach more linear, with less code all around and a bit faster in term
>> of code execution(of course really few, but better than nothing) since
>> that array is in .bss, so it will be zeroed earlier during a long
>> "memset" where assembly instructions for zeroing 8 bytes at a time are
>> used. What about this?
> 
> The initial zeroing should not matter at all in terms of performance,
> I think the only question is whether one wants a single zero page
> to be used everywhere or one per NUMA node to give better locality
> for a cache miss.
> 
> My guess is that for a system with 4KB pages, all the data
> in the zero page are typically available in a CPU cache already,
> so it doesn't matter, but it's possible that some machines benefit
> from having per-node pages when the page size isn't tiny compared
> to the typical cache sizes.
> 
> We should probably not touch this for any of the other architectures.

Ok, thanks for the explanation!

Best regards
  

Hello Russell, Arnd, All,

On 19/10/22 11:09, Russell King (Oracle) wrote:
> On Tue, Oct 18, 2022 at 09:03:01AM +0200, Arnd Bergmann wrote:
>> 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.
> 
> ARM's implementation is the utterly sensible implementation IMHO.
> 
> When the only users in the kernel _were_ ZERO_PAGE() for this, which
> is defined to return a struct page pointer, there was no need to make
> "empty_zero_page" anything but a struct page pointer, rather than a
> runtime translation from an address to a struct page.
> 
> IMHO, we should _not_ be exposing empty_zero_page to devices - we
> certainly do not want the DMA API performing cache maintenance on
> this page since the primary purpose of this page is to fill in
> userspace BSS pages that have not been written.
> 
> ACPI's use is just to have a cookie for invalid handles, and using
> the struct page pointer is good enough.
> 
> The only problem one is the RAID6 code, but that is disabled:
> 
> /* Set to 1 to use kernel-wide empty_zero_page */
> #define RAID6_USE_EMPTY_ZERO_PAGE 0
> 
> #if RAID6_USE_EMPTY_ZERO_PAGE
> # define raid6_empty_zero_page empty_zero_page
> #else
> extern const char raid6_empty_zero_page[PAGE_SIZE];
> #endif

For this I've sent a patch to remove the unused code:
https://lore.kernel.org/all/20221019160407.7550-1-giulio.benetti@benettiengineering.com/

> So, the only one that needs fixing is the SPI usage, which IMHO
> is wrong. ARM being different finds what I consider a driver bug.
> Good for 32-bit ARM. :)

Oh, I've sent a patch for substituting ZERO_PAGE(0) and it's already
been applied to spi's for-next:
https://lore.kernel.org/all/166619141690.565256.8563939546728659746.b4-ty@kernel.org/
So this doesn't break the build but there is still a bug.

Just to understand if I've understood correctly. The correct fix would
be to kzalloc() a dma_dummy_tx buffer and use it in place of
ZERO_PAGE(0), right?

Can you also please point me some link explaining the structure of this
topic? I've already started to read:
https://docs.kernel.org/core-api/index.html#memory-management

Maybe it is enough. I'd appreciate a lot any further links to get into
this if any.

Thank you

Kind regards