[PATCH 0/3] arm64: Avoid overlapping EFI regions

[PATCH 0/3] arm64: Avoid overlapping EFI regions

[Catalin Marinas]

The aim of this series is to avoid the pud/pmd splitting in the arm64
create_pgd_mapping() code. AFAICT, efi_create_mapping() was the last
user to generate overlapping mappings. If I'm mistaken and this is not
the last user, we have to fix the rest as well.

The EFI memory map on my Juno looks like this:

[    0.000000] efi: Processing EFI memory map:
[    0.000000] efi:   0x000008000000-0x00000bffffff [Memory Mapped I/O  |RUN|  |  |  |  |  |  |   |  |  |  |UC]
[    0.000000] efi:   0x00001c170000-0x00001c170fff [Memory Mapped I/O  |RUN|  |  |  |  |  |  |   |  |  |  |UC]
[    0.000000] efi:   0x000080000000-0x00008007ffff [Conventional Memory|   |  |  |  |  |  |  |   |WB|WT|WC|UC]
[    0.000000] efi:   0x000080080000-0x000080d8ffff [Loader Data        |   |  |  |  |  |  |  |   |WB|WT|WC|UC]
[    0.000000] efi:   0x000080d90000-0x00009fdfffff [Conventional Memory|   |  |  |  |  |  |  |   |WB|WT|WC|UC]
[    0.000000] efi:   0x00009fe00000-0x00009fe0ffff [Loader Data        |   |  |  |  |  |  |  |   |WB|WT|WC|UC]
[    0.000000] efi:   0x00009fe10000-0x0000dfffffff [Conventional Memory|   |  |  |  |  |  |  |   |WB|WT|WC|UC]
[    0.000000] efi:   0x0000e00f0000-0x0000febd5fff [Conventional Memory|   |  |  |  |  |  |  |   |WB|WT|WC|UC]
[    0.000000] efi:   0x0000febd6000-0x0000febd9fff [ACPI Reclaim Memory|   |  |  |  |  |  |  |   |WB|WT|WC|UC]*
[    0.000000] efi:   0x0000febda000-0x0000febdafff [ACPI Memory NVS    |   |  |  |  |  |  |  |   |WB|WT|WC|UC]*
[    0.000000] efi:   0x0000febdb000-0x0000febdcfff [ACPI Reclaim Memory|   |  |  |  |  |  |  |   |WB|WT|WC|UC]*
[    0.000000] efi:   0x0000febdd000-0x0000feffffff [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC]
[    0.000000] efi:   0x000880000000-0x0009f9644fff [Conventional Memory|   |  |  |  |  |  |  |   |WB|WT|WC|UC]
[    0.000000] efi:   0x0009f9645000-0x0009f9646fff [Loader Data        |   |  |  |  |  |  |  |   |WB|WT|WC|UC]
[    0.000000] efi:   0x0009f9647000-0x0009fa356fff [Loader Code        |   |  |  |  |  |  |  |   |WB|WT|WC|UC]
[    0.000000] efi:   0x0009fa357000-0x0009faf6efff [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC]
[    0.000000] efi:   0x0009faf6f000-0x0009fafa9fff [Runtime Data       |RUN|  |  |  |  |  |  |   |WB|WT|WC|UC]*
[    0.000000] efi:   0x0009fafaa000-0x0009ff2b1fff [Conventional Memory|   |  |  |  |  |  |  |   |WB|WT|WC|UC]
[    0.000000] efi:   0x0009ff2b2000-0x0009ffb70fff [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC]
[    0.000000] efi:   0x0009ffb71000-0x0009ffb89fff [Conventional Memory|   |  |  |  |  |  |  |   |WB|WT|WC|UC]
[    0.000000] efi:   0x0009ffb8a000-0x0009ffb8dfff [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC]
[    0.000000] efi:   0x0009ffb8e000-0x0009ffb8efff [Conventional Memory|   |  |  |  |  |  |  |   |WB|WT|WC|UC]
[    0.000000] efi:   0x0009ffb8f000-0x0009ffdddfff [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC]
[    0.000000] efi:   0x0009ffdde000-0x0009ffe5ffff [Conventional Memory|   |  |  |  |  |  |  |   |WB|WT|WC|UC]
[    0.000000] efi:   0x0009ffe60000-0x0009ffe76fff [Loader Data        |   |  |  |  |  |  |  |   |WB|WT|WC|UC]
[    0.000000] efi:   0x0009ffe77000-0x0009fff6dfff [Boot Code          |   |  |  |  |  |  |  |   |WB|WT|WC|UC]
[    0.000000] efi:   0x0009fff6e000-0x0009fffaefff [Runtime Code       |RUN|  |  |  |  |  |  |   |WB|WT|WC|UC]*
[    0.000000] efi:   0x0009fffaf000-0x0009ffffefff [Runtime Data       |RUN|  |  |  |  |  |  |   |WB|WT|WC|UC]*
[    0.000000] efi:   0x0009fffff000-0x0009ffffffff [Boot Data          |   |  |  |  |  |  |  |   |WB|WT|WC|UC]

Catalin Marinas (3):
  efi: Introduce *_continue efi_memory_desc iterators
  arm64: efi: Ensure efi_create_mapping() does not map overlapping
    regions
  arm64: mm: Remove split_p*d() functions

 arch/arm64/kernel/efi.c | 22 +++++++++++++++++++---
 arch/arm64/mm/mmu.c     | 47 ++++-------------------------------------------
 include/linux/efi.h     | 19 +++++++++++++++++--
 3 files changed, 40 insertions(+), 48 deletions(-)

[PATCH 1/3] efi: Introduce *_continue efi_memory_desc iterators

[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 <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
Signed-off-by: Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org>
---
 include/linux/efi.h | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/include/linux/efi.h b/include/linux/efi.h
index c2db3ca22217..4b0d880f1cd7 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -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)				   \
+	((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))
 
 /**
  * for_each_efi_memory_desc - iterate over descriptors in efi.memmap
@@ -1017,6 +1022,16 @@ extern int efi_memattr_apply_permissions(struct mm_struct *mm,
 #define for_each_efi_memory_desc(md) \
 	for_each_efi_memory_desc_in_map(&efi.memmap, md)
 
+/* Continue iterating through an efi_memory_map */
+#define for_each_efi_memory_desc_in_map_continue(m, md)			   \
+	for ((md) = efi_memory_desc_next_entry_map(m, md);		   \
+	     (md);							   \
+	     (md) = efi_memory_desc_next_entry_map(m, md))
+
+/* Continue iterating through efi.memmap */
+#define for_each_efi_memory_desc_continue(md) \
+	for_each_efi_memory_desc_in_map_continue(&efi.memmap, md)
+
 /*
  * Format an EFI memory descriptor's type and attributes to a user-provided
  * character buffer, as per snprintf(), and return the buffer.

[PATCH 2/3] arm64: efi: Ensure efi_create_mapping() does not map overlapping regions

[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 <catalin.marinas-5wv7dgnIgG8@public.gmane.org>
---
 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;
+		break;
+	}
+
+	if (length)
+		create_pgd_mapping(mm, md->phys_addr, md->virt_addr, length,
+				   __pgprot(prot_val | PTE_NG));
 	return 0;
 }
 

[PATCH 3/3] arm64: mm: Remove split_p*d() functions

[Catalin Marinas]

Since the efi_create_mapping() no longer generates overlapping regions
and being the last user of the split_p*d code, remove these functions
and the corresponding TLBI.

Signed-off-by: Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org>
---
 arch/arm64/mm/mmu.c | 47 ++++-------------------------------------------
 1 file changed, 4 insertions(+), 43 deletions(-)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index c31f796f33ac..00a9e3f64ea9 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -134,24 +134,6 @@ void __strict_break_before_make(pte_t old_pte, pte_t new_pte, const char *info)
 }
 #endif
 
-/*
- * remap a PMD into pages
- */
-static void split_pmd(pmd_t *pmd, pte_t *pte)
-{
-	unsigned long pfn = pmd_pfn(*pmd);
-	int i = 0;
-
-	do {
-		/*
-		 * Need to have the least restrictive permissions available
-		 * permissions will be fixed up later
-		 */
-		set_pte(pte, pfn_pte(pfn, PAGE_KERNEL_EXEC));
-		pfn++;
-	} while (pte++, i++, i < PTRS_PER_PTE);
-}
-
 static void alloc_init_pte(pmd_t *pmd, unsigned long addr,
 				  unsigned long end, unsigned long pfn,
 				  pgprot_t prot,
@@ -159,15 +141,13 @@ static void alloc_init_pte(pmd_t *pmd, unsigned long addr,
 {
 	pte_t *pte;
 
-	if (pmd_none(*pmd) || pmd_sect(*pmd)) {
+	BUG_ON(pmd_sect(*pmd));
+	if (pmd_none(*pmd)) {
 		phys_addr_t pte_phys;
 		BUG_ON(!pgtable_alloc);
 		pte_phys = pgtable_alloc();
 		pte = pte_set_fixmap(pte_phys);
-		if (pmd_sect(*pmd))
-			split_pmd(pmd, pte);
 		__pmd_populate(pmd, pte_phys, PMD_TYPE_TABLE);
-		flush_tlb_all();
 		pte_clear_fixmap();
 	}
 	BUG_ON(pmd_bad(*pmd));
@@ -181,18 +161,6 @@ static void alloc_init_pte(pmd_t *pmd, unsigned long addr,
 	pte_clear_fixmap();
 }
 
-static void split_pud(pud_t *old_pud, pmd_t *pmd)
-{
-	unsigned long addr = pud_pfn(*old_pud) << PAGE_SHIFT;
-	pgprot_t prot = __pgprot(pud_val(*old_pud) ^ addr);
-	int i = 0;
-
-	do {
-		set_pmd(pmd, __pmd(addr | pgprot_val(prot)));
-		addr += PMD_SIZE;
-	} while (pmd++, i++, i < PTRS_PER_PMD);
-}
-
 #ifdef CONFIG_DEBUG_PAGEALLOC
 static bool block_mappings_allowed(phys_addr_t (*pgtable_alloc)(void))
 {
@@ -223,20 +191,13 @@ static void alloc_init_pmd(pud_t *pud, unsigned long addr, unsigned long end,
 	/*
 	 * Check for initial section mappings in the pgd/pud and remove them.
 	 */
-	if (pud_none(*pud) || pud_sect(*pud)) {
+	BUG_ON(pud_sect(*pud));
+	if (pud_none(*pud)) {
 		phys_addr_t pmd_phys;
 		BUG_ON(!pgtable_alloc);
 		pmd_phys = pgtable_alloc();
 		pmd = pmd_set_fixmap(pmd_phys);
-		if (pud_sect(*pud)) {
-			/*
-			 * need to have the 1G of mappings continue to be
-			 * present
-			 */
-			split_pud(pud, pmd);
-		}
 		__pud_populate(pud, pmd_phys, PUD_TYPE_TABLE);
-		flush_tlb_all();
 		pmd_clear_fixmap();
 	}
 	BUG_ON(pud_bad(*pud));

Re: [PATCH 1/3] efi: Introduce *_continue efi_memory_desc iterators

[Mark Rutland]

Hi Catalin,

On Tue, May 31, 2016 at 04:14:30PM +0100, Catalin Marinas wrote:
> 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 <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> Signed-off-by: Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org>
> ---
>  include/linux/efi.h | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index c2db3ca22217..4b0d880f1cd7 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -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)				   \
> +	((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,
Mark.

[1] git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git urgent
[2] http://lkml.kernel.org/r/1464690224-4503-1-git-send-email-matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org

Re: [PATCH 1/3] efi: Introduce *_continue efi_memory_desc iterators

[Catalin Marinas]

On Wed, Jun 01, 2016 at 11:34:47AM +0100, Mark Rutland wrote:
> On Tue, May 31, 2016 at 04:14:30PM +0100, Catalin Marinas wrote:
> > 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 <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> > Signed-off-by: Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org>
> > ---
> >  include/linux/efi.h | 19 +++++++++++++++++--
> >  1 file changed, 17 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index c2db3ca22217..4b0d880f1cd7 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -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)				   \
> > +	((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.

-- 
Catalin

Re: [PATCH 1/3] efi: Introduce *_continue efi_memory_desc iterators

[Matt Fleming]

On Wed, 01 Jun, at 11:43:26AM, Catalin Marinas wrote:
> 
> 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?

Re: [PATCH 2/3] arm64: efi: Ensure efi_create_mapping() does not map overlapping regions

[Matt Fleming]

On Tue, 31 May, at 04:14:31PM, Catalin Marinas wrote:
> 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 <catalin.marinas-5wv7dgnIgG8@public.gmane.org>
> ---
>  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;
> +		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?

Re: [PATCH 1/3] efi: Introduce *_continue efi_memory_desc iterators

[Catalin Marinas]

On Thu, Jun 02, 2016 at 03:36:50PM +0100, Matt Fleming wrote:
> On Wed, 01 Jun, at 11:43:26AM, Catalin Marinas wrote:
> > 
> > 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?

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.

-- 
Catalin

Re: [PATCH 1/3] efi: Introduce *_continue efi_memory_desc iterators

[Jeremy Linton]

On 06/02/2016 11:29 AM, Catalin Marinas wrote:
> On Thu, Jun 02, 2016 at 03:36:50PM +0100, Matt Fleming wrote:
>> On Wed, 01 Jun, at 11:43:26AM, Catalin Marinas wrote:
>>>
>>> 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?
>
> 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!

Re: [PATCH 2/3] arm64: efi: Ensure efi_create_mapping() does not map overlapping regions

[Catalin Marinas]

On Thu, Jun 02, 2016 at 03:52:46PM +0100, Matt Fleming wrote:
> On Tue, 31 May, at 04:14:31PM, Catalin Marinas wrote:
> > 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 <catalin.marinas-5wv7dgnIgG8@public.gmane.org>
> > ---
> >  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;
> > +		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).

-- 
Catalin

Re: [PATCH 1/3] efi: Introduce *_continue efi_memory_desc iterators

[Catalin Marinas]

On Thu, Jun 02, 2016 at 11:31:14AM -0500, Jeremy Linton wrote:
> On 06/02/2016 11:29 AM, Catalin Marinas wrote:
> >On Thu, Jun 02, 2016 at 03:36:50PM +0100, Matt Fleming wrote:
> >>On Wed, 01 Jun, at 11:43:26AM, Catalin Marinas wrote:
> >>>
> >>>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?
> >
> >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).

-- 
Catalin

RE: [PATCH 1/3] efi: Introduce *_continue efi_memory_desc iterators

[Jeremy Linton]

|-----Original Message-----
|From: Catalin Marinas [mailto:catalin.marinas-5wv7dgnIgG8@public.gmane.org]
|Sent: Thursday, June 02, 2016 12:12 PM
|To: Jeremy Linton
|Cc: Matt Fleming; Mark Rutland; linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Will Deacon;
|linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
|Subject: Re: [PATCH 1/3] efi: Introduce *_continue efi_memory_desc
|iterators
|
|On Thu, Jun 02, 2016 at 11:31:14AM -0500, Jeremy Linton wrote:
|> On 06/02/2016 11:29 AM, Catalin Marinas wrote:
|> >On Thu, Jun 02, 2016 at 03:36:50PM +0100, Matt Fleming wrote:
|> >>On Wed, 01 Jun, at 11:43:26AM, Catalin Marinas wrote:
|> >>>
|> >>>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?
|> >
|> >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).

Sure. Most of the delay on posting them generally comes down to how many machines I try to boot them on with a given kernel, which is related to how many ACPI issues I happen to have.


IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH 1/3] efi: Introduce *_continue efi_memory_desc iterators

[Matt Fleming]

On Thu, 02 Jun, at 06:11:52PM, Catalin Marinas wrote:
> 
> 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).

Sure, this works for me. Thanks Catalin.

Re: [PATCH 2/3] arm64: efi: Ensure efi_create_mapping() does not map overlapping regions

[Matt Fleming]

On Thu, 02 Jun, at 05:56:21PM, Catalin Marinas wrote:
> 
> 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).

Yeah, figures. Could you mention this in the changelog or in a code
comment?

Re: [PATCH 2/3] arm64: efi: Ensure efi_create_mapping() does not map overlapping regions

[Ard Biesheuvel]

On 31 May 2016 at 17:14, Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
> 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.

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. The problem that
was flagged before is failure to do break-before-make, based on the
observation that a valid mapping (of the page that is shared between
two regions) is overwritten with another valid mapping. However, since
the 64K (or 16K) page that is shared between the two regions is mapped
with the exact same attributes (we ignore the permissions in that
case), the old and the new valid mappings are identical, which I don't
think should be a problem.

So, if it is allowed by the architecture to overwrite a page table
entry with the same value, perhaps we could educate the
break-before-make debug routines (if we merged those, I didn't check)
to allow that. And if we want to make absolutely sure that we don't
inadvertently map more than 32 MB worth of runtime services code or
data on a 16 KB pages kernel using block mappings, we could borrow the
PT debug logic to force mapping regions that are not aligned to
PAGE_SIZE down to pages in all cases. But I don't think we need the
logic in this patch.

Another comment below.

> Signed-off-by: Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org>
> ---
>  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.

> +               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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] arm64: efi: Ensure efi_create_mapping() does not map overlapping regions

[Catalin Marinas]

On Mon, Jun 06, 2016 at 11:43:01AM +0200, Ard Biesheuvel wrote:
> On 31 May 2016 at 17:14, Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
> > 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).

> > Signed-off-by: Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org>
> > ---
> >  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

Re: [PATCH 2/3] arm64: efi: Ensure efi_create_mapping() does not map overlapping regions

[Ard Biesheuvel]

> On 6 jun. 2016, at 19:09, Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
> 
>> On Mon, Jun 06, 2016 at 11:43:01AM +0200, Ard Biesheuvel wrote:
>>> On 31 May 2016 at 17:14, Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
>>> 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).
> 
>>> Signed-off-by: Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org>
>>> ---
>>> 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.
> 

If the current entry is only 4k in size, and starts 56k into the 64k page frame, you subtract 60k, and you end up with a length of -56k--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] arm64: efi: Ensure efi_create_mapping() does not map overlapping regions

[Catalin Marinas]

On Mon, Jun 06, 2016 at 06:09:50PM +0100, Catalin Marinas wrote:
> On Mon, Jun 06, 2016 at 11:43:01AM +0200, Ard Biesheuvel wrote:
> > On 31 May 2016 at 17:14, Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
> > > 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.

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.

-- 
Catalin

Re: [PATCH 2/3] arm64: efi: Ensure efi_create_mapping() does not map overlapping regions

[Ard Biesheuvel]

On 6 June 2016 at 19:42, Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
> On Mon, Jun 06, 2016 at 06:09:50PM +0100, Catalin Marinas wrote:
>> On Mon, Jun 06, 2016 at 11:43:01AM +0200, Ard Biesheuvel wrote:
>> > On 31 May 2016 at 17:14, Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
>> > > 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.
>
> 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.
>

Indeed.

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

Re: [PATCH 2/3] arm64: efi: Ensure efi_create_mapping() does not map overlapping regions

[Catalin Marinas]

(Restarting the thread before I forget the entire discussion)

On Mon, Jun 06, 2016 at 11:18:14PM +0200, Ard Biesheuvel wrote:
> >> > On 31 May 2016 at 17:14, Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
> >> > > 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;
[...]
> 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.

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

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

-- 
Catalin

Re: [PATCH 2/3] arm64: efi: Ensure efi_create_mapping() does not map overlapping regions

[Ard Biesheuvel]

On 28 June 2016 at 18:05, Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
> (Restarting the thread before I forget the entire discussion)
>
> On Mon, Jun 06, 2016 at 11:18:14PM +0200, Ard Biesheuvel wrote:
>> >> > On 31 May 2016 at 17:14, Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
>> >> > > 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;
> [...]
>> 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.

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

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

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

Indeed. So we could reuse the logic that imposes that to prevent
creating regions that are subject to splitting later on. That way,
there is no need to worry about the code running twice in case the
Memory Attributes table is exposed by the firmware.

Re: [PATCH 2/3] arm64: efi: Ensure efi_create_mapping() does not map overlapping regions

[Catalin Marinas]

On Tue, Jun 28, 2016 at 06:12:22PM +0200, Ard Biesheuvel wrote:
> On 28 June 2016 at 18:05, Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
> > (Restarting the thread before I forget the entire discussion)
> >
> > On Mon, Jun 06, 2016 at 11:18:14PM +0200, Ard Biesheuvel wrote:
> >> >> > On 31 May 2016 at 17:14, Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
> >> >> > > 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;
> > [...]
> >> 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.
> 
> >> 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.
> 
> 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.

-- 
Catalin

Re: [PATCH 2/3] arm64: efi: Ensure efi_create_mapping() does not map overlapping regions

[Ard Biesheuvel]

On 29 June 2016 at 11:39, Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
> On Tue, Jun 28, 2016 at 06:12:22PM +0200, Ard Biesheuvel wrote:
>> On 28 June 2016 at 18:05, Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
>> > (Restarting the thread before I forget the entire discussion)
>> >
>> > On Mon, Jun 06, 2016 at 11:18:14PM +0200, Ard Biesheuvel wrote:
>> >> >> > On 31 May 2016 at 17:14, Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
>> >> >> > > 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;
>> > [...]
>> >> 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.
>>
>> >> 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.
>>
>> 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.
>

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.

Re: [PATCH 2/3] arm64: efi: Ensure efi_create_mapping() does not map overlapping regions

[Catalin Marinas]

On Wed, Jun 29, 2016 at 12:03:16PM +0200, Ard Biesheuvel wrote:
> On 29 June 2016 at 11:39, Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
> > On Tue, Jun 28, 2016 at 06:12:22PM +0200, Ard Biesheuvel wrote:
> >> On 28 June 2016 at 18:05, Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
> >> > (Restarting the thread before I forget the entire discussion)
> >> >
> >> > On Mon, Jun 06, 2016 at 11:18:14PM +0200, Ard Biesheuvel wrote:
> >> >> >> > On 31 May 2016 at 17:14, Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
> >> >> >> > > 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;
> >> > [...]
> >> >> 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.
> >>
> >> >> 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.
> >>
> >> 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.
> 
> 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.

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

-- 
Catalin

Re: [PATCH 2/3] arm64: efi: Ensure efi_create_mapping() does not map overlapping regions

[Ard Biesheuvel]

On 29 June 2016 at 12:50, Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
> On Wed, Jun 29, 2016 at 12:03:16PM +0200, Ard Biesheuvel wrote:
>> On 29 June 2016 at 11:39, Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
>> > On Tue, Jun 28, 2016 at 06:12:22PM +0200, Ard Biesheuvel wrote:
>> >> On 28 June 2016 at 18:05, Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
>> >> > (Restarting the thread before I forget the entire discussion)
>> >> >
>> >> > On Mon, Jun 06, 2016 at 11:18:14PM +0200, Ard Biesheuvel wrote:
>> >> >> >> > On 31 May 2016 at 17:14, Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
>> >> >> >> > > 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;
>> >> > [...]
>> >> >> 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.
>> >>
>> >> >> 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.
>> >>
>> >> 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.
>>
>> 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.
>
>> 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.

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.

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

Re: [PATCH 2/3] arm64: efi: Ensure efi_create_mapping() does not map overlapping regions

[Catalin Marinas]

On Wed, Jun 29, 2016 at 01:03:34PM +0200, Ard Biesheuvel wrote:
> On 29 June 2016 at 12:50, Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
> > On Wed, Jun 29, 2016 at 12:03:16PM +0200, Ard Biesheuvel wrote:
> >> On 29 June 2016 at 11:39, Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
> >> > On Tue, Jun 28, 2016 at 06:12:22PM +0200, Ard Biesheuvel wrote:
> >> >> On 28 June 2016 at 18:05, Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
> >> >> > On Mon, Jun 06, 2016 at 11:18:14PM +0200, Ard Biesheuvel wrote:
> >> >> >> 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.
> >> >>
> >> >> >> 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.
> >> >>
> >> >> 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.
> >>
> >> 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.
> >
> >> 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.
> 
> 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.

I assume there isn't even a workaround if the EFI maps are broken in
this respect. But we still need to gracefully handle it and avoid a
potential kernel panic (like some BUG_ON in the arm64 page table
creation code).

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

There wouldn't be any issue, we already do this in other cases like
mark_rodata_ro().

-- 
Catalin