arm64: Avoid overlapping EFI regions

The for_each_efi_memory_desc_continue() macro and corresponding "_in_map" allow iterating over an efi_memory_map from a given position. For code reuse between the existing iterator and the _continue variant, this patch also introduces efi_memory_desc_next_entry_map().

Cc Matt Fleming <[email protected]>

Signed-off-by Catalin Marinas <[email protected]>

---
 include/linux/efi.h | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

include/linux/efi.h +2 -0

index c2db3ca22217..4b0d880f1cd7 100644
@@ -1002,11 +1002,16 @@ extern int efi_memattr_init(void);
 extern int efi_memattr_apply_permissions(struct mm_struct *mm,
 					 efi_memattr_perm_setter fn);
 
+/* Find next entry in an efi_memory_map or NULL if md is last */
+#define efi_memory_desc_next_entry_map(m, md)				   \

Quoted 19 lines · Catalin Marinas

The for_each_efi_memory_desc_continue() macro and corresponding "_in_map" allow iterating over an efi_memory_map from a given position. For code reuse between the existing iterator and the _continue variant, this patch also introduces efi_memory_desc_next_entry_map().

Cc Matt Fleming <[email protected]>

Signed-off-by Catalin Marinas <[email protected]>

---
 include/linux/efi.h | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

include/linux/efi.h +0 -0

index c2db3ca22217..4b0d880f1cd7 100644
@@ -1002,11 +1002,16 @@ extern int efi_memattr_init(void);
 extern int efi_memattr_apply_permissions(struct mm_struct *mm,
 					 efi_memattr_perm_setter fn);
 

include/linux/efi.h extern int efi_memattr_init(void); · line 1014

Quoted 12 lines

include/linux/efi.h +7 -2

+/* Find next entry in an efi_memory_map or NULL if md is last */
+#define efi_memory_desc_next_entry_map(m, md)				   \
+	((md) == (efi_memory_desc_t *)((m)->map_end - (m)->desc_size)	   \
+	 ? NULL : (void *)(md) + (m)->desc_size)
+
 /* Iterate through an efi_memory_map */
 #define for_each_efi_memory_desc_in_map(m, md)				   \
 	for ((md) = (m)->map;						   \
-	     (md) <= (efi_memory_desc_t *)((m)->map_end - (m)->desc_size); \
-	     (md) = (void *)(md) + (m)->desc_size)
+	     (md);							   \
+	     (md) = efi_memory_desc_next_entry_map(m, md))

As a heads-up, this will conflict with the efi/urgent branch [1], due to commit ee92562e33c516dd ("efi: Fix for_each_efi_memory_desc_in_map() for empty memmaps"). A pull went out for that yesterday [2].

Thanks for the heads up. I'll rebase the patches after -rc2 but anyway I plan to merge them in 4.8 via the arm64 tree, including the patch above if Matt acks it. So there is enough time to fix the conflicts.

Quoted 4 lines · Catalin Marinas

Thanks for the heads up. I'll rebase the patches after -rc2 but anyway I plan to merge them in 4.8 via the arm64 tree, including the patch above if Matt acks it. So there is enough time to fix the conflicts.

I'd really prefer it if you didn't take this series through the arm64 tree, unless there are existing dependencies, of course.

There were a number of conflicts in 3 different maintainer trees in linux-next before the most recent merge window opened, and extra work was required to sort that out. I want to avoid that in the future, so expect me to be more insistent about keeping EFI patches in the EFI tree than I have been in the past.

Do arm64 prerequisites exist for this series? Or are you planning on adding dependencies on top?

Quoted 15 lines · Matt Fleming

Quoted 4 lines · Catalin Marinas

Thanks for the heads up. I'll rebase the patches after -rc2 but anyway I plan to merge them in 4.8 via the arm64 tree, including the patch above if Matt acks it. So there is enough time to fix the conflicts.

I'd really prefer it if you didn't take this series through the arm64 tree, unless there are existing dependencies, of course.

There were a number of conflicts in 3 different maintainer trees in linux-next before the most recent merge window opened, and extra work was required to sort that out. I want to avoid that in the future, so expect me to be more insistent about keeping EFI patches in the EFI tree than I have been in the past.

Quoted 2 lines

Do arm64 prerequisites exist for this series? Or are you planning on adding dependencies on top?

I don't know when Jeremy plans to submit his contiguous PTE patches (we rejected them on the grounds of unnecessary huge page splitting, hence this series). If he's not targeting 4.8, I'm happy for the patches to go in via the EFI tree.

Quoted 19 lines · Catalin Marinas

Quoted 17 lines · Matt Fleming

Quoted 4 lines · Catalin Marinas

Thanks for the heads up. I'll rebase the patches after -rc2 but anyway I plan to merge them in 4.8 via the arm64 tree, including the patch above if Matt acks it. So there is enough time to fix the conflicts.

I'd really prefer it if you didn't take this series through the arm64 tree, unless there are existing dependencies, of course.

There were a number of conflicts in 3 different maintainer trees in linux-next before the most recent merge window opened, and extra work was required to sort that out. I want to avoid that in the future, so expect me to be more insistent about keeping EFI patches in the EFI tree than I have been in the past.

Do arm64 prerequisites exist for this series? Or are you planning on adding dependencies on top?

Quoted 4 lines

I don't know when Jeremy plans to submit his contiguous PTE patches (we rejected them on the grounds of unnecessary huge page splitting, hence this series). If he's not targeting 4.8, I'm happy for the patches to go in via the EFI tree.

I can re-post them again for 4.8. They seem to get simpler every time!

I think you should post them fairly soon so that we get a chance to review them and ideally push into next around -rc4.

Regarding these efi/arm64 patches, I can push them to a branch (once the conflicting efi/urgent patches hit upstream, I guess around -rc2) that Matt can pull into the EFI tree and I can also pull in the arm64 tree *if* we are to merge the Jeremy's patches.

Jeremy, you can base your patches on such branch (I'll follow up with the details if that's ok with Matt).

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.

Signed-off-by Catalin Marinas <[email protected]>

---
 arch/arm64/kernel/efi.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

arch/arm64/kernel/efi.c +12 -3

index 78f52488f9ff..0d5753c31c7f 100644
@@ -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))

Quoted 39 lines · Catalin Marinas

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.

Signed-off-by Catalin Marinas <[email protected]>

---
 arch/arm64/kernel/efi.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

arch/arm64/kernel/efi.c +12 -3

index 78f52488f9ff..0d5753c31c7f 100644
@@ -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))

arch/arm64/kernel/efi.c struct screen_info screen_info __section(.data); · line 87

Quoted 10 lines

arch/arm64/kernel/efi.c +7 -0

+			length -= (md->phys_addr + length) & ~PAGE_MASK;
+		break;
+	}
+
+	if (length)
+		create_pgd_mapping(mm, md->phys_addr, md->virt_addr, length,
+				   __pgprot(prot_val | PTE_NG));
 	return 0;
 }
 

Is this mapping in chunks scheme required because of the EFI Properties Table restriction whereby relative offsets between regions must be maintained?

Because if that's not the reason, I'm wondering why you can't simply update efi_get_virtmap() to align the virtual addresses to 64K?

Ard to confirm but I think the reason is the relative offset between code and data regions that must be preserved. For example, on Juno I get:

[    0.000000] efi:   0x0009fff6e000-0x0009fffaefff [Runtime Code       |RUN|  |  |  |  |  |  |   |WB|WT|WC|UC]*
[    0.000000] efi:   0x0009fffaf000-0x0009ffffefff [Runtime Data       |RUN|  |  |  |  |  |  |   |WB|WT|WC|UC]*

Since the code may assume relative loads from the data section, we need to preserve this offset (which doesn't seem 64KB aligned).

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.

Signed-off-by Catalin Marinas <[email protected]>

---
 arch/arm64/kernel/efi.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

arch/arm64/kernel/efi.c +3 -3

index 78f52488f9ff..0d5753c31c7f 100644
@@ -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));
+       /*

arch/arm64/kernel/efi.c +6 -1

+               break;
+       }
+
+       if (length)
+               create_pgd_mapping(mm, md->phys_addr, md->virt_addr, length,
+                                  __pgprot(prot_val | PTE_NG));
        return 0;
 }
 
--

To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 6 lines · Catalin Marinas

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.

Quoted 3 lines

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.

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.

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.

Quoted 27 lines

Signed-off-by Catalin Marinas <[email protected]>

---
 arch/arm64/kernel/efi.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

arch/arm64/kernel/efi.c +10 -3

index 78f52488f9ff..0d5753c31c7f 100644
@@ -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))

arch/arm64/kernel/efi.c struct screen_info screen_info __section(.data); · line 78

Quoted 3 lines

arch/arm64/kernel/efi.c +3 -0

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

Quoted 9 lines · Ard Biesheuvel

Quoted 7 lines · Catalin Marinas

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.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 7 lines

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.

Quoted 3 lines

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 27 lines

Quoted 27 lines

Signed-off-by Catalin Marinas <[email protected]>

---
arch/arm64/kernel/efi.c | 22 +++++++++++++++++++---

1 file changed, 19 insertions(+), 3 deletions(-)

arch/arm64/kernel/efi.c +10 -3

index 78f52488f9ff..0d5753c31c7f 100644
@@ -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))

Quoted 8 lines

arch/arm64/kernel/efi.c struct screen_info screen_info __section(.data); · line 78

Quoted 3 lines

arch/arm64/kernel/efi.c +3 -0

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

Quoted 23 lines · Ard Biesheuvel

Quoted 22 lines · Catalin Marinas

arch/arm64/kernel/efi.c +10 -3

index 78f52488f9ff..0d5753c31c7f 100644
@@ -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))

Quoted 8 lines

arch/arm64/kernel/efi.c struct screen_info screen_info __section(.data); · line 78

Quoted 3 lines

arch/arm64/kernel/efi.c +3 -0

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

Quoted 33 lines · Catalin Marinas

Quoted 23 lines · Ard Biesheuvel

Quoted 22 lines · Catalin Marinas

arch/arm64/kernel/efi.c +10 -3

index 78f52488f9ff..0d5753c31c7f 100644
@@ -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))

Quoted 8 lines

arch/arm64/kernel/efi.c struct screen_info screen_info __section(.data); · line 78

Quoted 3 lines

arch/arm64/kernel/efi.c +3 -0

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

Quoted 5 lines

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.

I think I get it now. If the md we are currently mapping is within a 64KB page *and* phys_addr unaligned, length can wrap through 0. I will update the patch.

Quoted 26 lines

Quoted 26 lines

Quoted 26 lines · Catalin Marinas

Quoted 25 lines

arch/arm64/kernel/efi.c +13 -3

index 78f52488f9ff..0d5753c31c7f 100644
@@ -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;

Another thing I failed to mention is that the new Memory Attributes table support may map all of the RuntimeServicesCode regions a second time, but with a higher granularity, using RO for .text and .rodata and NX for .data and .bss (and the PE/COFF header).

Due to the higher
granularity, regions that were mapped using the contiguous bit the first time around may be split into smaller regions. Your current code does not address that case.

I wonder how the PT debug code deals with this, and whether there is anything we can reuse to inhibit cont and block mappings

(Restarting the thread before I forget the entire discussion)

Quoted 26 lines · Ard Biesheuvel

Quoted 26 lines

Quoted 26 lines

Quoted 26 lines · Catalin Marinas

Quoted 25 lines

arch/arm64/kernel/efi.c +13 -3

index 78f52488f9ff..0d5753c31c7f 100644
@@ -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;

[...]

Quoted 4 lines

Another thing I failed to mention is that the new Memory Attributes table support may map all of the RuntimeServicesCode regions a second time, but with a higher granularity, using RO for .text and .rodata and NX for .data and .bss (and the PE/COFF header).

Can this not be done in a single go without multiple passes? That's what we did for the core arm64 code, the only one left being EFI run-time mappings.

Quoted 4 lines

Due to the higher
granularity, regions that were mapped using the contiguous bit the first time around may be split into smaller regions. Your current code does not address that case.

If the above doesn't work, the only solution would be to permanently map these ranges as individual pages, no large blocks.

Quoted 2 lines

I wonder how the PT debug code deals with this, and whether there is anything we can reuse to inhibit cont and block mappings

Do you mean DEBUG_PAGEALLOC? When enabled, it defaults to single page mappings for the kernel, no sections.

Quoted 30 lines · Catalin Marinas

(Restarting the thread before I forget the entire discussion)

Quoted 26 lines · Ard Biesheuvel

Quoted 26 lines

Quoted 26 lines

Quoted 26 lines · Catalin Marinas

Quoted 25 lines

arch/arm64/kernel/efi.c +13 -3

index 78f52488f9ff..0d5753c31c7f 100644
@@ -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;

[...]

Quoted 8 lines

Quoted 4 lines

Another thing I failed to mention is that the new Memory Attributes table support may map all of the RuntimeServicesCode regions a second time, but with a higher granularity, using RO for .text and .rodata and NX for .data and .bss (and the PE/COFF header).

Can this not be done in a single go without multiple passes? That's what we did for the core arm64 code, the only one left being EFI run-time mappings.

Well, we probably could, but it is far from trivial.

Quoted 5 lines

Quoted 4 lines

Due to the higher
granularity, regions that were mapped using the contiguous bit the first time around may be split into smaller regions. Your current code does not address that case.

Quoted 2 lines

If the above doesn't work, the only solution would be to permanently map these ranges as individual pages, no large blocks.

That is not unreasonable, since regions >2MB are unusual.

Quoted 50 lines · Ard Biesheuvel

Quoted 30 lines · Catalin Marinas

(Restarting the thread before I forget the entire discussion)

Quoted 26 lines · Ard Biesheuvel

Quoted 26 lines

Quoted 26 lines

Quoted 26 lines · Catalin Marinas

Quoted 25 lines

arch/arm64/kernel/efi.c +13 -3

index 78f52488f9ff..0d5753c31c7f 100644
@@ -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;

[...]

Quoted 8 lines

Quoted 4 lines

Another thing I failed to mention is that the new Memory Attributes table support may map all of the RuntimeServicesCode regions a second time, but with a higher granularity, using RO for .text and .rodata and NX for .data and .bss (and the PE/COFF header).

Can this not be done in a single go without multiple passes? That's what we did for the core arm64 code, the only one left being EFI run-time mappings.

Well, we probably could, but it is far from trivial.

Quoted 7 lines

Quoted 4 lines

Due to the higher
granularity, regions that were mapped using the contiguous bit the first time around may be split into smaller regions. Your current code does not address that case.

If the above doesn't work, the only solution would be to permanently map these ranges as individual pages, no large blocks.

Quoted 1 line

That is not unreasonable, since regions >2MB are unusual.

We'll have the contiguous bit supported at some point and we won't be able to use it for EFI run-time mappings. But I don't think that's essential, minor improvement on a non-critical path.

I'll post some patches to always use PAGE_SIZE granularity for EFI run-time mappings.

Quoted 57 lines · Catalin Marinas

Quoted 47 lines · Ard Biesheuvel

Quoted 30 lines · Catalin Marinas

(Restarting the thread before I forget the entire discussion)

Quoted 26 lines · Ard Biesheuvel

Quoted 26 lines

Quoted 26 lines

Quoted 26 lines · Catalin Marinas

Quoted 25 lines

arch/arm64/kernel/efi.c +13 -3

index 78f52488f9ff..0d5753c31c7f 100644
@@ -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;

[...]

Quoted 8 lines

Quoted 4 lines

Another thing I failed to mention is that the new Memory Attributes table support may map all of the RuntimeServicesCode regions a second time, but with a higher granularity, using RO for .text and .rodata and NX for .data and .bss (and the PE/COFF header).

Can this not be done in a single go without multiple passes? That's what we did for the core arm64 code, the only one left being EFI run-time mappings.

Well, we probably could, but it is far from trivial.

Quoted 5 lines

Quoted 4 lines

Due to the higher
granularity, regions that were mapped using the contiguous bit the first time around may be split into smaller regions. Your current code does not address that case.

Quoted 4 lines

Quoted 2 lines

If the above doesn't work, the only solution would be to permanently map these ranges as individual pages, no large blocks.

That is not unreasonable, since regions >2MB are unusual.

We'll have the contiguous bit supported at some point and we won't be able to use it for EFI run-time mappings. But I don't think that's essential, minor improvement on a non-critical path.

Quoted 2 lines

I'll post some patches to always use PAGE_SIZE granularity for EFI run-time mappings.

Given that contiguous bit mappings only affect the TLB footprint, I'd be more concerned about not using block mappings for EfiMemoryMappedIo regions (since they may cover fairly sizable NOR flashes like the 64 MB one QEMU mach-virt exposes).

So I would recommend to only use PAGE_SIZE granularity for EfiRuntimeServicesCode and EfiRuntimeServicesData regions, since those are the only ones that can be expected to appear in the Memory Attributes table, and all other regions will only be mapped a single time.

Quoted 58 lines · Ard Biesheuvel

Quoted 57 lines · Catalin Marinas

Quoted 47 lines · Ard Biesheuvel

Quoted 30 lines · Catalin Marinas

(Restarting the thread before I forget the entire discussion)

Quoted 26 lines · Ard Biesheuvel

Quoted 26 lines

Quoted 26 lines

Quoted 26 lines · Catalin Marinas

Quoted 25 lines

arch/arm64/kernel/efi.c +13 -3

index 78f52488f9ff..0d5753c31c7f 100644
@@ -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;

[...]

Quoted 8 lines

Quoted 4 lines

Another thing I failed to mention is that the new Memory Attributes table support may map all of the RuntimeServicesCode regions a second time, but with a higher granularity, using RO for .text and .rodata and NX for .data and .bss (and the PE/COFF header).

Can this not be done in a single go without multiple passes? That's what we did for the core arm64 code, the only one left being EFI run-time mappings.

Well, we probably could, but it is far from trivial.

Quoted 5 lines

Quoted 4 lines

Due to the higher
granularity, regions that were mapped using the contiguous bit the first time around may be split into smaller regions. Your current code does not address that case.

Quoted 4 lines

Quoted 2 lines

If the above doesn't work, the only solution would be to permanently map these ranges as individual pages, no large blocks.

That is not unreasonable, since regions >2MB are unusual.

We'll have the contiguous bit supported at some point and we won't be able to use it for EFI run-time mappings. But I don't think that's essential, minor improvement on a non-critical path.

Quoted 7 lines

Quoted 2 lines

I'll post some patches to always use PAGE_SIZE granularity for EFI run-time mappings.

Given that contiguous bit mappings only affect the TLB footprint, I'd be more concerned about not using block mappings for EfiMemoryMappedIo regions (since they may cover fairly sizable NOR flashes like the 64 MB one QEMU mach-virt exposes).

Good point.

Quoted 5 lines

So I would recommend to only use PAGE_SIZE granularity for EfiRuntimeServicesCode and EfiRuntimeServicesData regions, since those are the only ones that can be expected to appear in the Memory Attributes table, and all other regions will only be mapped a single time.

Is there a possibility that EfiMemoryMappedIo share the same 64K page with EfiRuntimeServicesCode? If it does, it won't help much with avoiding splitting.

Unless I keep a combination of these series
(checking the end/start overlap) with a forced page-only mapping for EfiRuntimeServicesCode/Data.

Quoted 46 lines · Catalin Marinas

Quoted 29 lines · Ard Biesheuvel

Quoted 28 lines · Catalin Marinas

Quoted 18 lines · Ard Biesheuvel

Quoted 9 lines · Catalin Marinas

Quoted 4 lines · Ard Biesheuvel

Another thing I failed to mention is that the new Memory Attributes table support may map all of the RuntimeServicesCode regions a second time, but with a higher granularity, using RO for .text and .rodata and NX for .data and .bss (and the PE/COFF header).

Can this not be done in a single go without multiple passes? That's what we did for the core arm64 code, the only one left being EFI run-time mappings.

Well, we probably could, but it is far from trivial.

Quoted 5 lines

Quoted 4 lines

Due to the higher
granularity, regions that were mapped using the contiguous bit the first time around may be split into smaller regions. Your current code does not address that case.

Quoted 4 lines

Quoted 2 lines

If the above doesn't work, the only solution would be to permanently map these ranges as individual pages, no large blocks.

That is not unreasonable, since regions >2MB are unusual.

We'll have the contiguous bit supported at some point and we won't be able to use it for EFI run-time mappings. But I don't think that's essential, minor improvement on a non-critical path.

Quoted 7 lines

Quoted 2 lines

I'll post some patches to always use PAGE_SIZE granularity for EFI run-time mappings.

Given that contiguous bit mappings only affect the TLB footprint, I'd be more concerned about not using block mappings for EfiMemoryMappedIo regions (since they may cover fairly sizable NOR flashes like the 64 MB one QEMU mach-virt exposes).

Good point.

Quoted 5 lines

So I would recommend to only use PAGE_SIZE granularity for EfiRuntimeServicesCode and EfiRuntimeServicesData regions, since those are the only ones that can be expected to appear in the Memory Attributes table, and all other regions will only be mapped a single time.

Quoted 3 lines

Is there a possibility that EfiMemoryMappedIo share the same 64K page with EfiRuntimeServicesCode? If it does, it won't help much with avoiding splitting.

The spec does not allow it, and it would also imply that memory and !memory share a 64 KB page frame in the hardware, which seems highly unlikely as well.

Quoted 3 lines

Unless I keep a combination of these series
(checking the end/start overlap) with a forced page-only mapping for EfiRuntimeServicesCode/Data.

If we get rid of the splitting, the only 'issue' that remains is that the page frame shared between two adjacent unaligned regions is mapped twice (but the current code will always map them with the same attribute)

So back to my question I posed a couple of posts ago: if the UEFI page tables were live at this time (which they are not), could it ever be a problem that a page table entry is rewritten with the exact same value it had before (but without bbm?) If not, I think we could educate the debug routines to allow this case (since it needs to read the entry to check the valid bit anyway, if it needs to be strict about break before make)