On Mon, Jun 06, 2016 at 11:43:01AM +0200, Ard Biesheuvel wrote:

On 31 May 2016 at 17:14, Catalin Marinas [off-list ref] wrote:

quoted

Since the EFI page size is 4KB, it is possible for a !4KB page kernel to
align an EFI runtime map boundaries in a way that they can overlap
within the same page. This requires the current create_pgd_mapping()
code to be able to split existing larger mappings when an overlapping
region needs to be mapped.

With this patch, efi_create_mapping() scans the EFI memory map for
overlapping regions and trims the length of the current map to avoid a
large block mapping and subsequent split.

I remember the discussion, but IIRC this was about failure to do
break-before-make in general rather than a problem with splitting in
particular.

In this instance, we don't have break-before-make issue since the pgd
used for EFI runtime services is not active. So it's just about
splitting a larger block and simplifying the arch code.

So first of all, I would like to point out that this failure to do
break-before-make is not a problem in practice, given that the EFI
page tables are not live until much later. The reason we want to fix
this is because the EFI page tables routines are shared with the
swapper, where it /is/ a concern, and there is a desire to make those
routines more strict when it comes to architectural rules regarding
page table manipulation.

Exactly.

Also, the reference to block mappings on !4K pages kernels is a bit
misleading here: such block mappings are at least 32 MB in size, and
we never map runtime code or data regions that big.

Jeremy has some patches in flight to use the contiguous bit on such
mappings. With 64KB x 32 pages, we get a 2MB block. I want to avoid code
splitting such contiguous block as well (which with the contiguous bit
means going back and clearing it).

quoted

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/kernel/efi.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index 78f52488f9ff..0d5753c31c7f 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c

@@ -62,10 +62,26 @@ struct screen_info screen_info __section(.data);
 int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md)
 {
        pteval_t prot_val = create_mapping_protection(md);
+       phys_addr_t length = md->num_pages << EFI_PAGE_SHIFT;
+       efi_memory_desc_t *next = md;

-       create_pgd_mapping(mm, md->phys_addr, md->virt_addr,
-                          md->num_pages << EFI_PAGE_SHIFT,
-                          __pgprot(prot_val | PTE_NG));
+       /*
+        * Search for the next EFI runtime map and check for any overlap with
+        * the current map when aligned to PAGE_SIZE. In such case, defer
+        * mapping the end of the current range until the next
+        * efi_create_mapping() call.
+        */
+       for_each_efi_memory_desc_continue(next) {
+               if (!(next->attribute & EFI_MEMORY_RUNTIME))
+                       continue;
+               if (next->phys_addr < PAGE_ALIGN(md->phys_addr + length))
+                       length -= (md->phys_addr + length) & ~PAGE_MASK;

This looks fishy. We could have more than two runtime regions sharing
a 64 KB page frame, for instance, and the middle regions will have
unaligned start and end addresses, and sizes smaller than 64 KB. This
subtraction may wrap in that case, producing a bogus length.

I don't think I get it. This subtraction is meant to reduce the length
of the current md so that it is page aligned, deferring the mapping of
the last page to the next efi_create_mapping() call. Note that there is
a "break" just after the hunk you quoted, so we only care about the next
runtime map.

-- 
Catalin