Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.7.0-alpha0-1047-g9e4af4ada4-fm-20221005.001-g9e4af4ad
Mime-Version: 1.0
Message-Id: <3fb4afd1-2eea-4a71-a914-f8208b11f9f4@app.fastmail.com>
In-Reply-To: <20221017233700.84918-1-giulio.benetti@benettiengineering.com>
References: <20221017233700.84918-1-giulio.benetti@benettiengineering.com>
Date:   Tue, 18 Oct 2022 09:03:01 +0200
From:   "Arnd Bergmann" <arnd@arndb.de>
To:     "Giulio Benetti" <giulio.benetti@benettiengineering.com>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Cc:     "Russell King" <linux@armlinux.org.uk>,
        "Anshuman Khandual" <anshuman.khandual@arm.com>,
        "Andrew Morton" <akpm@linux-foundation.org>,
        "Kefeng Wang" <wangkefeng.wang@huawei.com>,
        "Russell King" <rmk+kernel@armlinux.org.uk>,
        "Will Deacon" <will@kernel.org>
Subject: Re: [PATCH] ARM: mm: fix no-MMU ZERO_PAGE() implementation
Content-Type: text/plain
Precedence: bulk

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