On Thu, Sep 30, 2021 at 4:16 AM Will Deacon [off-list ref] wrote:

On Wed, Sep 29, 2021 at 11:54:55PM -0400, Pasha Tatashin wrote:

quoted

quoted

quoted

+/* Allocates pages for kexec page table */
+static void *kexec_page_alloc(void *arg)
+{
+     struct kimage *kimage = (struct kimage *)arg;
+     struct page *page = kimage_alloc_control_pages(kimage, 0);
+
+     if (!page)
+             return NULL;
+
+     memset(page_address(page), 0, PAGE_SIZE);

Hmm, I think we might be missing barriers here to ensure that the zeroes
are visible to the page-table walker before we plumb the page into the
page-table.

Usually, that's taken care of by the smp_wmb() in __pXX_alloc() but I
can't see that here. Is it hiding?

Based on the comment in __pte_alloc() that smp_wmb() is needed in
order to synchronize pte setup with other cpus prior to making it
visible to them. This is not needed here. First, by the time these
page tables are used the other cpus are offlined (kexec reboot code is
single threaded). Second, we never insert any entry into a page table
that is actively used by any cpu.

I think the comment there is wrong, but the barrier is still necessary.
How else do you guarantee that the page-table walker reads the zeroes from
the memset?

True, good point. We are still safe because we have the following:
cpu_install_ttbr0() is used to load trans_pgd tables both in kexec and
hibernate cases.

cpu_install_ttbr0 has: local_flush_tlb_all()

dsb(nshst);   // Ensure prior page-table updates have completed
__tlbi(vmalle1);  // Invalidate the TLB
dsb(nsh);   // Ensure the TLB invalidation has completed
isb();  // Discard any instructions fetched from the old mapping

Pasha

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel