[PATCH 0/3] arm64/efi: improve TEXT_OFFSET handling

[PATCH 0/3] arm64/efi: improve TEXT_OFFSET handling

[Ard Biesheuvel]

This is mostly a resend of patches that were circulated a week or 2 ago to
address boot failures that were observed on platforms where the base of DRAM
is occupied by firmware or by UEFI itself.

TEXT_OFFSET is somewhat of a historical artefact, and with Mark Rutland's
patches already queued for 3.17, the region below TEXT_OFFSET is actually no
longer used for early page tables etc. and the intention in the long term is
to get rid of TEXT_OFFSET completely, i.e., set it to 0

The new TEXT_OFFSET fuzzing option (which is recommended for distribution
kernels) already has the potential to break some platforms (such as APM Mustang
booting from UEFI) that assume the value of TEXT_OFFSET is large enough to allow
a small portion of memory at the base of DRAM to be reserved for things like the
SMP holding pen. If the dice roll that produces TEXT_OFFSET happens to be too
low, the kernel will currently fail to boot.

This problem, and other potential issues where the base of DRAM may not be
vacant for the kernel, are addressed by patch #3. This may result in up to 2
megabytes of memory at the low end being wasted, but with TEXT_OFFSET being set
to zero eventually, this cannot be avoided anyway.

Patch #1 was suggested by Mark to fix a failure on APM Mustang identified by
Mark Salter in bringing up the secondaries when the kernel is loaded higher than
the base of DRAM. This may happen after patch #3 is applied, and instead of
aborting, the Image is loaded 2 megs + TEXT_OFFSET bytes above the base of DRAM
rather than just TEXT_OFFSET bytes. Unfortunately, I have not been able to test
this patch myself, so if anyone is in a position to test these patches on a
UEFI APM Mustang, that would be highly appreciated.

Patch #2 ensures that the allocation done by the PE/COFF loader is large enough
so that it can be executed in place. The likelihood of this occurring is not
clear: Tianocore/EDK2 tends to load Image fairly high in memory, but a UEFI
enhanced GRUB or any other arm64 boot protocol aware intermediate loader may
well decide to put the Image in the right place before the stub is executed.

Patch #3 changes the relocation logic so that the Image is relocated to the
lowest available 2 meg boundary + TEXT_OFFSET. This eliminates a failure mode
where we used to abort if the kernel cannot be loaded at its optimal offset, and
moves always moves Image to a suitable location regardless of the existence of
low reservations or the value of TEXT_OFFSET.

Mark Rutland (1):
  arm64: spin-table: handle unmapped cpu-release-addrs

Ard Biesheuvel (2):
  arm64/efi: efistub: cover entire static mem footprint in PE/COFF .text
  arm64/efi: efistub: don't abort if base of DRAM is occupied

 arch/arm64/kernel/efi-stub.c       | 18 ++++++------------
 arch/arm64/kernel/head.S           |  6 +++---
 arch/arm64/kernel/smp_spin_table.c | 21 ++++++++++++++++-----
 3 files changed, 25 insertions(+), 20 deletions(-)

-- 
1.8.3.2

[PATCH 1/3] arm64: spin-table: handle unmapped cpu-release-addrs

[Ard Biesheuvel]

From: Mark Rutland <mark.rutland@arm.com>

In certain cases the cpu-release-addr of a CPU may not fall in the
linear mapping (e.g. when the kernel is loaded above this address due to
the presence of other images in memory). This is problematic for the
spin-table code as it assumes that it can trivially convert a
cpu-release-addr to a valid VA in the linear map.

This patch modifies the spin-table code to use a temporary cached
mapping to write to a given cpu-release-addr, enabling us to support
addresses regardless of whether they are covered by the linear mapping.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm64/kernel/smp_spin_table.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kernel/smp_spin_table.c b/arch/arm64/kernel/smp_spin_table.c
index 0347d38eea29..70181c1bf42d 100644
--- a/arch/arm64/kernel/smp_spin_table.c
+++ b/arch/arm64/kernel/smp_spin_table.c
@@ -20,6 +20,7 @@
 #include <linux/init.h>
 #include <linux/of.h>
 #include <linux/smp.h>
+#include <linux/types.h>
 
 #include <asm/cacheflush.h>
 #include <asm/cpu_ops.h>
@@ -65,12 +66,21 @@ static int smp_spin_table_cpu_init(struct device_node *dn, unsigned int cpu)
 
 static int smp_spin_table_cpu_prepare(unsigned int cpu)
 {
-	void **release_addr;
+	__le64 __iomem *release_addr;
 
 	if (!cpu_release_addr[cpu])
 		return -ENODEV;
 
-	release_addr = __va(cpu_release_addr[cpu]);
+	/*
+	 * The cpu-release-addr may or may not be inside the linear mapping.
+	 * As ioremap_cache will either give us a new mapping or reuse the
+	 * existing linear mapping, we can use it to cover both cases. In
+	 * either case the memory will be MT_NORMAL.
+	 */
+	release_addr = ioremap_cache(cpu_release_addr[cpu],
+				     sizeof(*release_addr));
+	if (!release_addr)
+		return -ENOMEM;
 
 	/*
 	 * We write the release address as LE regardless of the native
@@ -79,15 +89,16 @@ static int smp_spin_table_cpu_prepare(unsigned int cpu)
 	 * boot-loader's endianess before jumping. This is mandated by
 	 * the boot protocol.
 	 */
-	release_addr[0] = (void *) cpu_to_le64(__pa(secondary_holding_pen));
-
-	__flush_dcache_area(release_addr, sizeof(release_addr[0]));
+	writeq_relaxed(__pa(secondary_holding_pen), release_addr);
+	__flush_dcache_area(release_addr, sizeof(*release_addr));
 
 	/*
 	 * Send an event to wake up the secondary CPU.
 	 */
 	sev();
 
+	iounmap(release_addr);
+
 	return 0;
 }
 
-- 
1.8.3.2

[PATCH 2/3] arm64/efi: efistub: cover entire static mem footprint in PE/COFF .text

[Ard Biesheuvel]

The static memory footprint of a kernel Image at boot is larger than the
Image file itself. Things like .bss data and initial page tables are allocated
statically but populated dynamically so their content is not contained in the
Image file.

However, if EFI (or GRUB) has loaded the Image at precisely the desired offset
of base of DRAM + TEXT_OFFSET, the Image will be booted in place, and we have
to make sure that the allocation done by the PE/COFF loader is large enough.

Fix this by growing the PE/COFF .text section to cover the entire static
memory footprint. The part of the section that is not covered by the payload
will be zero initialised by the PE/COFF loader.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/head.S | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 69dafe9621fd..dafc4f2c1ade 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -148,7 +148,7 @@ optional_header:
 	.short	0x20b				// PE32+ format
 	.byte	0x02				// MajorLinkerVersion
 	.byte	0x14				// MinorLinkerVersion
-	.long	_edata - stext			// SizeOfCode
+	.long	_end - stext			// SizeOfCode
 	.long	0				// SizeOfInitializedData
 	.long	0				// SizeOfUninitializedData
 	.long	efi_stub_entry - efi_head	// AddressOfEntryPoint
@@ -166,7 +166,7 @@ extra_header_fields:
 	.short	0				// MinorSubsystemVersion
 	.long	0				// Win32VersionValue
 
-	.long	_edata - efi_head		// SizeOfImage
+	.long	_end - efi_head			// SizeOfImage
 
 	// Everything before the kernel image is considered part of the header
 	.long	stext - efi_head		// SizeOfHeaders
@@ -213,7 +213,7 @@ section_table:
 	.byte	0
 	.byte	0
 	.byte	0        		// end of 0 padding of section name
-	.long	_edata - stext		// VirtualSize
+	.long	_end - stext		// VirtualSize
 	.long	stext - efi_head	// VirtualAddress
 	.long	_edata - stext		// SizeOfRawData
 	.long	stext - efi_head	// PointerToRawData
-- 
1.8.3.2

[PATCH 3/3] arm64/efi: efistub: don't abort if base of DRAM is occupied

[Ard Biesheuvel]

If we cannot relocate the kernel Image to its preferred offset of base of DRAM
plus TEXT_OFFSET, instead relocate it to the lowest available 2 MB boundary plus
TEXT_OFFSET. We may lose a bit of memory at the low end, but we can still
proceed normally otherwise.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/efi-stub.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c
index 60e98a639ac5..460c00c41e57 100644
--- a/arch/arm64/kernel/efi-stub.c
+++ b/arch/arm64/kernel/efi-stub.c
@@ -60,20 +60,16 @@ static efi_status_t handle_kernel_image(efi_system_table_t *sys_table,
 	kernel_size = _edata - _text;
 	if (*image_addr != (dram_base + TEXT_OFFSET)) {
 		kernel_memsize = kernel_size + (_end - _edata);
-		status = efi_relocate_kernel(sys_table, image_addr,
-					     kernel_size, kernel_memsize,
-					     dram_base + TEXT_OFFSET,
-					     PAGE_SIZE);
+		status = efi_low_alloc(sys_table, kernel_memsize + TEXT_OFFSET,
+				       SZ_2M, reserve_addr);
 		if (status != EFI_SUCCESS) {
 			pr_efi_err(sys_table, "Failed to relocate kernel\n");
 			return status;
 		}
-		if (*image_addr != (dram_base + TEXT_OFFSET)) {
-			pr_efi_err(sys_table, "Failed to alloc kernel memory\n");
-			efi_free(sys_table, kernel_memsize, *image_addr);
-			return EFI_ERROR;
-		}
-		*image_size = kernel_memsize;
+		memcpy((void *)*reserve_addr + TEXT_OFFSET, (void *)*image_addr,
+		       kernel_size);
+		*image_addr = *reserve_addr + TEXT_OFFSET;
+		*reserve_size = kernel_memsize + TEXT_OFFSET;
 	}
 
 
-- 
1.8.3.2

[PATCH 1/3] arm64: spin-table: handle unmapped cpu-release-addrs

[Mark Salter]

On Tue, 2014-07-29 at 12:49 +0200, Ard Biesheuvel wrote:
> From: Mark Rutland <mark.rutland@arm.com>
> 
> In certain cases the cpu-release-addr of a CPU may not fall in the
> linear mapping (e.g. when the kernel is loaded above this address due to
> the presence of other images in memory). This is problematic for the
> spin-table code as it assumes that it can trivially convert a
> cpu-release-addr to a valid VA in the linear map.
> 
> This patch modifies the spin-table code to use a temporary cached
> mapping to write to a given cpu-release-addr, enabling us to support
> addresses regardless of whether they are covered by the linear mapping.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> ---
>  arch/arm64/kernel/smp_spin_table.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/kernel/smp_spin_table.c b/arch/arm64/kernel/smp_spin_table.c
> index 0347d38eea29..70181c1bf42d 100644
> --- a/arch/arm64/kernel/smp_spin_table.c
> +++ b/arch/arm64/kernel/smp_spin_table.c
> @@ -20,6 +20,7 @@
>  #include <linux/init.h>
>  #include <linux/of.h>
>  #include <linux/smp.h>
> +#include <linux/types.h>
>  
>  #include <asm/cacheflush.h>
>  #include <asm/cpu_ops.h>
> @@ -65,12 +66,21 @@ static int smp_spin_table_cpu_init(struct device_node *dn, unsigned int cpu)
>  
>  static int smp_spin_table_cpu_prepare(unsigned int cpu)
>  {
> -	void **release_addr;
> +	__le64 __iomem *release_addr;
>  
>  	if (!cpu_release_addr[cpu])
>  		return -ENODEV;
>  
> -	release_addr = __va(cpu_release_addr[cpu]);
> +	/*
> +	 * The cpu-release-addr may or may not be inside the linear mapping.
> +	 * As ioremap_cache will either give us a new mapping or reuse the
> +	 * existing linear mapping, we can use it to cover both cases. In
> +	 * either case the memory will be MT_NORMAL.
> +	 */
> +	release_addr = ioremap_cache(cpu_release_addr[cpu],
> +				     sizeof(*release_addr));
> +	if (!release_addr)
> +		return -ENOMEM;
>  
>  	/*
>  	 * We write the release address as LE regardless of the native
> @@ -79,15 +89,16 @@ static int smp_spin_table_cpu_prepare(unsigned int cpu)
>  	 * boot-loader's endianess before jumping. This is mandated by
>  	 * the boot protocol.
>  	 */
> -	release_addr[0] = (void *) cpu_to_le64(__pa(secondary_holding_pen));
> -
> -	__flush_dcache_area(release_addr, sizeof(release_addr[0]));
> +	writeq_relaxed(__pa(secondary_holding_pen), release_addr);
> +	__flush_dcache_area(release_addr, sizeof(*release_addr));

       __flush_dcache_area((__force void *)release_addr, ... 

to avoid sparse warning.

>  
>  	/*
>  	 * Send an event to wake up the secondary CPU.
>  	 */
>  	sev();
>  
> +	iounmap(release_addr);
> +
>  	return 0;
>  }
>  

[PATCH 1/3] arm64: spin-table: handle unmapped cpu-release-addrs

[Mark Salter]

On Tue, 2014-07-29 at 11:15 -0400, Mark Salter wrote:
> On Tue, 2014-07-29 at 12:49 +0200, Ard Biesheuvel wrote:
> > From: Mark Rutland <mark.rutland@arm.com>
> > 
> > In certain cases the cpu-release-addr of a CPU may not fall in the
> > linear mapping (e.g. when the kernel is loaded above this address due to
> > the presence of other images in memory). This is problematic for the
> > spin-table code as it assumes that it can trivially convert a
> > cpu-release-addr to a valid VA in the linear map.
> > 
> > This patch modifies the spin-table code to use a temporary cached
> > mapping to write to a given cpu-release-addr, enabling us to support
> > addresses regardless of whether they are covered by the linear mapping.
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>

Oops, forgot:

Tested-by: Mark Salter <msalter@redhat.com>

> > ---
> >  arch/arm64/kernel/smp_spin_table.c | 21 ++++++++++++++++-----
> >  1 file changed, 16 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/smp_spin_table.c b/arch/arm64/kernel/smp_spin_table.c
> > index 0347d38eea29..70181c1bf42d 100644
> > --- a/arch/arm64/kernel/smp_spin_table.c
> > +++ b/arch/arm64/kernel/smp_spin_table.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/init.h>
> >  #include <linux/of.h>
> >  #include <linux/smp.h>
> > +#include <linux/types.h>
> >  
> >  #include <asm/cacheflush.h>
> >  #include <asm/cpu_ops.h>
> > @@ -65,12 +66,21 @@ static int smp_spin_table_cpu_init(struct device_node *dn, unsigned int cpu)
> >  
> >  static int smp_spin_table_cpu_prepare(unsigned int cpu)
> >  {
> > -	void **release_addr;
> > +	__le64 __iomem *release_addr;
> >  
> >  	if (!cpu_release_addr[cpu])
> >  		return -ENODEV;
> >  
> > -	release_addr = __va(cpu_release_addr[cpu]);
> > +	/*
> > +	 * The cpu-release-addr may or may not be inside the linear mapping.
> > +	 * As ioremap_cache will either give us a new mapping or reuse the
> > +	 * existing linear mapping, we can use it to cover both cases. In
> > +	 * either case the memory will be MT_NORMAL.
> > +	 */
> > +	release_addr = ioremap_cache(cpu_release_addr[cpu],
> > +				     sizeof(*release_addr));
> > +	if (!release_addr)
> > +		return -ENOMEM;
> >  
> >  	/*
> >  	 * We write the release address as LE regardless of the native
> > @@ -79,15 +89,16 @@ static int smp_spin_table_cpu_prepare(unsigned int cpu)
> >  	 * boot-loader's endianess before jumping. This is mandated by
> >  	 * the boot protocol.
> >  	 */
> > -	release_addr[0] = (void *) cpu_to_le64(__pa(secondary_holding_pen));
> > -
> > -	__flush_dcache_area(release_addr, sizeof(release_addr[0]));
> > +	writeq_relaxed(__pa(secondary_holding_pen), release_addr);
> > +	__flush_dcache_area(release_addr, sizeof(*release_addr));
> 
>        __flush_dcache_area((__force void *)release_addr, ... 
> 
> to avoid sparse warning.
> 
> >  
> >  	/*
> >  	 * Send an event to wake up the secondary CPU.
> >  	 */
> >  	sev();
> >  
> > +	iounmap(release_addr);
> > +
> >  	return 0;
> >  }
> >  
> 

[PATCH 1/3] arm64: spin-table: handle unmapped cpu-release-addrs

[Arnd Bergmann]

On Tuesday 29 July 2014 11:15:45 Mark Salter wrote:
> > -
> > -     __flush_dcache_area(release_addr, sizeof(release_addr[0]));
> > +     writeq_relaxed(__pa(secondary_holding_pen), release_addr);
> > +     __flush_dcache_area(release_addr, sizeof(*release_addr));
> 
>        __flush_dcache_area((__force void *)release_addr, ... 
> 
> to avoid sparse warning.
> 

I think it would be cleaner to drop the __iomem annotation and use vmap()
rather than ioremap(). That requires having a 'struct page' though, which
I'm not sure you have.

	Arnd

[PATCH 3/3] arm64/efi: efistub: don't abort if base of DRAM is occupied

[Mark Salter]

On Tue, 2014-07-29 at 12:49 +0200, Ard Biesheuvel wrote:
> If we cannot relocate the kernel Image to its preferred offset of base of DRAM
> plus TEXT_OFFSET, instead relocate it to the lowest available 2 MB boundary plus
> TEXT_OFFSET. We may lose a bit of memory at the low end, but we can still
> proceed normally otherwise.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/kernel/efi-stub.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)

Tested on Mustang (with loss of 2MB free memory).

> 
> diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c
> index 60e98a639ac5..460c00c41e57 100644
> --- a/arch/arm64/kernel/efi-stub.c
> +++ b/arch/arm64/kernel/efi-stub.c
> @@ -60,20 +60,16 @@ static efi_status_t handle_kernel_image(efi_system_table_t *sys_table,
>  	kernel_size = _edata - _text;
>  	if (*image_addr != (dram_base + TEXT_OFFSET)) {
>  		kernel_memsize = kernel_size + (_end - _edata);
> -		status = efi_relocate_kernel(sys_table, image_addr,
> -					     kernel_size, kernel_memsize,
> -					     dram_base + TEXT_OFFSET,
> -					     PAGE_SIZE);

Can we make efi_relocate_kernel static inline to get rid
of the "defined but unused" warning?

Otherwise:

Acked-by: Mark Salter <msalter@redhat.com>

> +		status = efi_low_alloc(sys_table, kernel_memsize + TEXT_OFFSET,
> +				       SZ_2M, reserve_addr);
>  		if (status != EFI_SUCCESS) {
>  			pr_efi_err(sys_table, "Failed to relocate kernel\n");
>  			return status;
>  		}
> -		if (*image_addr != (dram_base + TEXT_OFFSET)) {
> -			pr_efi_err(sys_table, "Failed to alloc kernel memory\n");
> -			efi_free(sys_table, kernel_memsize, *image_addr);
> -			return EFI_ERROR;
> -		}
> -		*image_size = kernel_memsize;
> +		memcpy((void *)*reserve_addr + TEXT_OFFSET, (void *)*image_addr,
> +		       kernel_size);
> +		*image_addr = *reserve_addr + TEXT_OFFSET;
> +		*reserve_size = kernel_memsize + TEXT_OFFSET;
>  	}
>  
> 

[PATCH 1/3] arm64: spin-table: handle unmapped cpu-release-addrs

[Mark Salter]

On Tue, 2014-07-29 at 17:20 +0200, Arnd Bergmann wrote:
> On Tuesday 29 July 2014 11:15:45 Mark Salter wrote:
> > > -
> > > -     __flush_dcache_area(release_addr, sizeof(release_addr[0]));
> > > +     writeq_relaxed(__pa(secondary_holding_pen), release_addr);
> > > +     __flush_dcache_area(release_addr, sizeof(*release_addr));
> > 
> >        __flush_dcache_area((__force void *)release_addr, ... 
> > 
> > to avoid sparse warning.
> > 
> 
> I think it would be cleaner to drop the __iomem annotation and use vmap()
> rather than ioremap(). That requires having a 'struct page' though, which
> I'm not sure you have.
> 

You won't. If you did have a struct page, then __va() would work.

[PATCH 2/3] arm64/efi: efistub: cover entire static mem footprint in PE/COFF .text

[Mark Salter]

On Tue, 2014-07-29 at 12:49 +0200, Ard Biesheuvel wrote:
> The static memory footprint of a kernel Image at boot is larger than the
> Image file itself. Things like .bss data and initial page tables are allocated
> statically but populated dynamically so their content is not contained in the
> Image file.
> 
> However, if EFI (or GRUB) has loaded the Image at precisely the desired offset
> of base of DRAM + TEXT_OFFSET, the Image will be booted in place, and we have
> to make sure that the allocation done by the PE/COFF loader is large enough.
> 
> Fix this by growing the PE/COFF .text section to cover the entire static
> memory footprint. The part of the section that is not covered by the payload
> will be zero initialised by the PE/COFF loader.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/kernel/head.S | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Acked-by: Mark Salter <msalter@redhat.com>

> 
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index 69dafe9621fd..dafc4f2c1ade 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -148,7 +148,7 @@ optional_header:
>  	.short	0x20b				// PE32+ format
>  	.byte	0x02				// MajorLinkerVersion
>  	.byte	0x14				// MinorLinkerVersion
> -	.long	_edata - stext			// SizeOfCode
> +	.long	_end - stext			// SizeOfCode
>  	.long	0				// SizeOfInitializedData
>  	.long	0				// SizeOfUninitializedData
>  	.long	efi_stub_entry - efi_head	// AddressOfEntryPoint
> @@ -166,7 +166,7 @@ extra_header_fields:
>  	.short	0				// MinorSubsystemVersion
>  	.long	0				// Win32VersionValue
>  
> -	.long	_edata - efi_head		// SizeOfImage
> +	.long	_end - efi_head			// SizeOfImage
>  
>  	// Everything before the kernel image is considered part of the header
>  	.long	stext - efi_head		// SizeOfHeaders
> @@ -213,7 +213,7 @@ section_table:
>  	.byte	0
>  	.byte	0
>  	.byte	0        		// end of 0 padding of section name
> -	.long	_edata - stext		// VirtualSize
> +	.long	_end - stext		// VirtualSize
>  	.long	stext - efi_head	// VirtualAddress
>  	.long	_edata - stext		// SizeOfRawData
>  	.long	stext - efi_head	// PointerToRawData

[PATCH 1/3] arm64: spin-table: handle unmapped cpu-release-addrs

[Arnd Bergmann]

On Tuesday 29 July 2014 11:30:24 Mark Salter wrote:
> On Tue, 2014-07-29 at 17:20 +0200, Arnd Bergmann wrote:
> > On Tuesday 29 July 2014 11:15:45 Mark Salter wrote:
> > > > -
> > > > -     __flush_dcache_area(release_addr, sizeof(release_addr[0]));
> > > > +     writeq_relaxed(__pa(secondary_holding_pen), release_addr);
> > > > +     __flush_dcache_area(release_addr, sizeof(*release_addr));
> > > 
> > >        __flush_dcache_area((__force void *)release_addr, ... 
> > > 
> > > to avoid sparse warning.
> > > 
> > 
> > I think it would be cleaner to drop the __iomem annotation and use vmap()
> > rather than ioremap(). That requires having a 'struct page' though, which
> > I'm not sure you have.
> > 
> 
> You won't. If you did have a struct page, then __va() would work.

Ah, right. I was thinking of highmem, which has a struct page but no virtual
address. However, on arm64 there is obviously no highmem.

	Arnd 

[PATCH 1/3] arm64: spin-table: handle unmapped cpu-release-addrs

[Mark Rutland]

On Tue, Jul 29, 2014 at 04:20:46PM +0100, Arnd Bergmann wrote:
> On Tuesday 29 July 2014 11:15:45 Mark Salter wrote:
> > > -
> > > -     __flush_dcache_area(release_addr, sizeof(release_addr[0]));
> > > +     writeq_relaxed(__pa(secondary_holding_pen), release_addr);
> > > +     __flush_dcache_area(release_addr, sizeof(*release_addr));
> > 
> >        __flush_dcache_area((__force void *)release_addr, ... 
> > 
> > to avoid sparse warning.

Presumably we'd get this for the write_relaxed too?

> > 
> 
> I think it would be cleaner to drop the __iomem annotation and use vmap()
> rather than ioremap(). That requires having a 'struct page' though, which
> I'm not sure you have.

As far as I am aware, we'd only have a struct page for memory falling in
the linear map, so for the cases this patch is actually required we
wouldn't have a struct page.

So it looks like I should just make release_addr a void __iomem *. Then
this line can just be:

__flush_dcache_area(release_addr, 8);

Where we could replace 8 with sizeof(u64), sizeof(__le64), etc if 8 is
too magic.

How does that sound?

Thanks,
Mark.

[PATCH 1/3] arm64: spin-table: handle unmapped cpu-release-addrs

[Arnd Bergmann]

On Tuesday 29 July 2014 17:03:03 Mark Rutland wrote:
> On Tue, Jul 29, 2014 at 04:20:46PM +0100, Arnd Bergmann wrote:
> > On Tuesday 29 July 2014 11:15:45 Mark Salter wrote:
> > > > -
> > > > -     __flush_dcache_area(release_addr, sizeof(release_addr[0]));
> > > > +     writeq_relaxed(__pa(secondary_holding_pen), release_addr);
> > > > +     __flush_dcache_area(release_addr, sizeof(*release_addr));
> > > 
> > >        __flush_dcache_area((__force void *)release_addr, ... 
> > > 
> > > to avoid sparse warning.
> 
> Presumably we'd get this for the write_relaxed too?

writeq_relaxed() actually expects an __iomem  pointer

> > 
> > I think it would be cleaner to drop the __iomem annotation and use vmap()
> > rather than ioremap(). That requires having a 'struct page' though, which
> > I'm not sure you have.
> 
> As far as I am aware, we'd only have a struct page for memory falling in
> the linear map, so for the cases this patch is actually required we
> wouldn't have a struct page.
> 
> So it looks like I should just make release_addr a void __iomem *. Then
> this line can just be:

You mean make it a 'void *' instead of 'void __iomem *', right?

> __flush_dcache_area(release_addr, 8);
> 
> Where we could replace 8 with sizeof(u64), sizeof(__le64), etc if 8 is
> too magic.
> 
> How does that sound?

Not sure where you're getting at. Using a regular pointer sounds fine,
but then you have to cast the result of ioremap and do a manual
cpu_to_le64 conversion on the assignment.

Keeping the iomem annotation will also work,a nd then we only need
the cast in the __flush_dcache_area call.

	Arnd

[PATCH 1/3] arm64: spin-table: handle unmapped cpu-release-addrs

[Mark Rutland]

On Tue, Jul 29, 2014 at 05:13:07PM +0100, Arnd Bergmann wrote:
> On Tuesday 29 July 2014 17:03:03 Mark Rutland wrote:
> > On Tue, Jul 29, 2014 at 04:20:46PM +0100, Arnd Bergmann wrote:
> > > On Tuesday 29 July 2014 11:15:45 Mark Salter wrote:
> > > > > -
> > > > > -     __flush_dcache_area(release_addr, sizeof(release_addr[0]));
> > > > > +     writeq_relaxed(__pa(secondary_holding_pen), release_addr);
> > > > > +     __flush_dcache_area(release_addr, sizeof(*release_addr));
> > > > 
> > > >        __flush_dcache_area((__force void *)release_addr, ... 
> > > > 
> > > > to avoid sparse warning.
> > 
> > Presumably we'd get this for the write_relaxed too?
> 
> writeq_relaxed() actually expects an __iomem  pointer

Ah. I was being thick and thought this was about the underlying type of
release_addr (le64 * vs void *) rather than the __iomem annotation.

> > > 
> > > I think it would be cleaner to drop the __iomem annotation and use vmap()
> > > rather than ioremap(). That requires having a 'struct page' though, which
> > > I'm not sure you have.
> > 
> > As far as I am aware, we'd only have a struct page for memory falling in
> > the linear map, so for the cases this patch is actually required we
> > wouldn't have a struct page.
> > 
> > So it looks like I should just make release_addr a void __iomem *. Then
> > this line can just be:
> 
> You mean make it a 'void *' instead of 'void __iomem *', right?
> 
> > __flush_dcache_area(release_addr, 8);
> > 
> > Where we could replace 8 with sizeof(u64), sizeof(__le64), etc if 8 is
> > too magic.
> > 
> > How does that sound?
> 
> Not sure where you're getting at. Using a regular pointer sounds fine,
> but then you have to cast the result of ioremap and do a manual
> cpu_to_le64 conversion on the assignment.
> 
> Keeping the iomem annotation will also work,a nd then we only need
> the cast in the __flush_dcache_area call.

Sorry, I'd misunderstood the problem and my suggestion was nonsense
deriving from that.

Having the (__force void *) cast in the call to __flush_dcache_area
sounds like the right solution to me.

Thanks,
Mark.

[PATCH 1/3] arm64: spin-table: handle unmapped cpu-release-addrs

[Arnd Bergmann]

On Tuesday 29 July 2014 17:18:52 Mark Rutland wrote:
> Sorry, I'd misunderstood the problem and my suggestion was nonsense
> deriving from that.
> 
> Having the (__force void *) cast in the call to __flush_dcache_area
> sounds like the right solution to me.

Well, there isn't really a good solution here I think, because you
are not really dealing with an iomem pointer after all, just something
a bit like that, but we don't have a 'memremap' function to do what
you really mean.

	Arnd

[PATCH 3/3] arm64/efi: efistub: don't abort if base of DRAM is occupied

[Ard Biesheuvel]

On 29 July 2014 17:29, Mark Salter <msalter@redhat.com> wrote:
> On Tue, 2014-07-29 at 12:49 +0200, Ard Biesheuvel wrote:
>> If we cannot relocate the kernel Image to its preferred offset of base of DRAM
>> plus TEXT_OFFSET, instead relocate it to the lowest available 2 MB boundary plus
>> TEXT_OFFSET. We may lose a bit of memory at the low end, but we can still
>> proceed normally otherwise.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  arch/arm64/kernel/efi-stub.c | 16 ++++++----------
>>  1 file changed, 6 insertions(+), 10 deletions(-)
>
> Tested on Mustang (with loss of 2MB free memory).
>

Great, thanks!

>>
>> diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c
>> index 60e98a639ac5..460c00c41e57 100644
>> --- a/arch/arm64/kernel/efi-stub.c
>> +++ b/arch/arm64/kernel/efi-stub.c
>> @@ -60,20 +60,16 @@ static efi_status_t handle_kernel_image(efi_system_table_t *sys_table,
>>       kernel_size = _edata - _text;
>>       if (*image_addr != (dram_base + TEXT_OFFSET)) {
>>               kernel_memsize = kernel_size + (_end - _edata);
>> -             status = efi_relocate_kernel(sys_table, image_addr,
>> -                                          kernel_size, kernel_memsize,
>> -                                          dram_base + TEXT_OFFSET,
>> -                                          PAGE_SIZE);
>
> Can we make efi_relocate_kernel static inline to get rid
> of the "defined but unused" warning?
>

I have some patches pending in the EFI tree to turn the stub into a
static library, and that already takes care of this issue.

> Otherwise:
>
> Acked-by: Mark Salter <msalter@redhat.com>
>

Cheers,
Ard.



>> +             status = efi_low_alloc(sys_table, kernel_memsize + TEXT_OFFSET,
>> +                                    SZ_2M, reserve_addr);
>>               if (status != EFI_SUCCESS) {
>>                       pr_efi_err(sys_table, "Failed to relocate kernel\n");
>>                       return status;
>>               }
>> -             if (*image_addr != (dram_base + TEXT_OFFSET)) {
>> -                     pr_efi_err(sys_table, "Failed to alloc kernel memory\n");
>> -                     efi_free(sys_table, kernel_memsize, *image_addr);
>> -                     return EFI_ERROR;
>> -             }
>> -             *image_size = kernel_memsize;
>> +             memcpy((void *)*reserve_addr + TEXT_OFFSET, (void *)*image_addr,
>> +                    kernel_size);
>> +             *image_addr = *reserve_addr + TEXT_OFFSET;
>> +             *reserve_size = kernel_memsize + TEXT_OFFSET;
>>       }
>>
>>
>
>

[PATCH 3/3] arm64/efi: efistub: don't abort if base of DRAM is occupied

[Mark Salter]

On Tue, 2014-07-29 at 20:17 +0200, Ard Biesheuvel wrote:
> On 29 July 2014 17:29, Mark Salter <msalter@redhat.com> wrote:
> > On Tue, 2014-07-29 at 12:49 +0200, Ard Biesheuvel wrote:
> >> If we cannot relocate the kernel Image to its preferred offset of base of DRAM
> >> plus TEXT_OFFSET, instead relocate it to the lowest available 2 MB boundary plus
> >> TEXT_OFFSET. We may lose a bit of memory at the low end, but we can still
> >> proceed normally otherwise.
> >>
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >>  arch/arm64/kernel/efi-stub.c | 16 ++++++----------
> >>  1 file changed, 6 insertions(+), 10 deletions(-)
> >
> > Tested on Mustang (with loss of 2MB free memory).
> >
> 
> Great, thanks!
> 
> >>
> >> diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c
> >> index 60e98a639ac5..460c00c41e57 100644
> >> --- a/arch/arm64/kernel/efi-stub.c
> >> +++ b/arch/arm64/kernel/efi-stub.c
> >> @@ -60,20 +60,16 @@ static efi_status_t handle_kernel_image(efi_system_table_t *sys_table,
> >>       kernel_size = _edata - _text;
> >>       if (*image_addr != (dram_base + TEXT_OFFSET)) {
> >>               kernel_memsize = kernel_size + (_end - _edata);
> >> -             status = efi_relocate_kernel(sys_table, image_addr,
> >> -                                          kernel_size, kernel_memsize,
> >> -                                          dram_base + TEXT_OFFSET,
> >> -                                          PAGE_SIZE);
> >
> > Can we make efi_relocate_kernel static inline to get rid
> > of the "defined but unused" warning?
> >
> 
> I have some patches pending in the EFI tree to turn the stub into a
> static library, and that already takes care of this issue.

That's fine if the static library stub patch goes in first. If this
patch goes in first, then let's avoid the warning since it is easy
to do.

> 
> > Otherwise:
> >
> > Acked-by: Mark Salter <msalter@redhat.com>
> >
> 
> Cheers,
> Ard.
> 
> 
> 
> >> +             status = efi_low_alloc(sys_table, kernel_memsize + TEXT_OFFSET,
> >> +                                    SZ_2M, reserve_addr);
> >>               if (status != EFI_SUCCESS) {
> >>                       pr_efi_err(sys_table, "Failed to relocate kernel\n");
> >>                       return status;
> >>               }
> >> -             if (*image_addr != (dram_base + TEXT_OFFSET)) {
> >> -                     pr_efi_err(sys_table, "Failed to alloc kernel memory\n");
> >> -                     efi_free(sys_table, kernel_memsize, *image_addr);
> >> -                     return EFI_ERROR;
> >> -             }
> >> -             *image_size = kernel_memsize;
> >> +             memcpy((void *)*reserve_addr + TEXT_OFFSET, (void *)*image_addr,
> >> +                    kernel_size);
> >> +             *image_addr = *reserve_addr + TEXT_OFFSET;
> >> +             *reserve_size = kernel_memsize + TEXT_OFFSET;
> >>       }
> >>
> >>
> >
> >

[PATCH 3/3] arm64/efi: efistub: don't abort if base of DRAM is occupied

[Ard Biesheuvel]

On 29 July 2014 20:27, Mark Salter <msalter@redhat.com> wrote:
> On Tue, 2014-07-29 at 20:17 +0200, Ard Biesheuvel wrote:
>> On 29 July 2014 17:29, Mark Salter <msalter@redhat.com> wrote:
>> > On Tue, 2014-07-29 at 12:49 +0200, Ard Biesheuvel wrote:
>> >> If we cannot relocate the kernel Image to its preferred offset of base of DRAM
>> >> plus TEXT_OFFSET, instead relocate it to the lowest available 2 MB boundary plus
>> >> TEXT_OFFSET. We may lose a bit of memory at the low end, but we can still
>> >> proceed normally otherwise.
>> >>
>> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >> ---
>> >>  arch/arm64/kernel/efi-stub.c | 16 ++++++----------
>> >>  1 file changed, 6 insertions(+), 10 deletions(-)
>> >
>> > Tested on Mustang (with loss of 2MB free memory).
>> >
>>
>> Great, thanks!
>>
>> >>
>> >> diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c
>> >> index 60e98a639ac5..460c00c41e57 100644
>> >> --- a/arch/arm64/kernel/efi-stub.c
>> >> +++ b/arch/arm64/kernel/efi-stub.c
>> >> @@ -60,20 +60,16 @@ static efi_status_t handle_kernel_image(efi_system_table_t *sys_table,
>> >>       kernel_size = _edata - _text;
>> >>       if (*image_addr != (dram_base + TEXT_OFFSET)) {
>> >>               kernel_memsize = kernel_size + (_end - _edata);
>> >> -             status = efi_relocate_kernel(sys_table, image_addr,
>> >> -                                          kernel_size, kernel_memsize,
>> >> -                                          dram_base + TEXT_OFFSET,
>> >> -                                          PAGE_SIZE);
>> >
>> > Can we make efi_relocate_kernel static inline to get rid
>> > of the "defined but unused" warning?
>> >
>>
>> I have some patches pending in the EFI tree to turn the stub into a
>> static library, and that already takes care of this issue.
>
> That's fine if the static library stub patch goes in first. If this
> patch goes in first, then let's avoid the warning since it is easy
> to do.
>

My idea was to ask Matt to take patches #2 and #3. I may have to fix
them up slightly to apply correctly, but that's fine. Changing
efi_relocate_kernel to static inline would need to go through Matt's
tree as well, so there's probably no point in doing that in any case.

Patch #1 needs to go through the arm64, I guess. This means UEFI boot
on APM Mustang will be broken during the short time between the
x86/tip tree and the arm64 tree being merged for 3.17 (assuming 3.17
is still open). I think we should be able to tolerate that, right?

>>
>> > Otherwise:
>> >
>> > Acked-by: Mark Salter <msalter@redhat.com>
>> >
>>
>> Cheers,
>> Ard.
>>
>>
>>
>> >> +             status = efi_low_alloc(sys_table, kernel_memsize + TEXT_OFFSET,
>> >> +                                    SZ_2M, reserve_addr);
>> >>               if (status != EFI_SUCCESS) {
>> >>                       pr_efi_err(sys_table, "Failed to relocate kernel\n");
>> >>                       return status;
>> >>               }
>> >> -             if (*image_addr != (dram_base + TEXT_OFFSET)) {
>> >> -                     pr_efi_err(sys_table, "Failed to alloc kernel memory\n");
>> >> -                     efi_free(sys_table, kernel_memsize, *image_addr);
>> >> -                     return EFI_ERROR;
>> >> -             }
>> >> -             *image_size = kernel_memsize;
>> >> +             memcpy((void *)*reserve_addr + TEXT_OFFSET, (void *)*image_addr,
>> >> +                    kernel_size);
>> >> +             *image_addr = *reserve_addr + TEXT_OFFSET;
>> >> +             *reserve_size = kernel_memsize + TEXT_OFFSET;
>> >>       }
>> >>
>> >>
>> >
>> >
>
>

[PATCH 3/3] arm64/efi: efistub: don't abort if base of DRAM is occupied

[Mark Salter]

On Tue, 2014-07-29 at 20:46 +0200, Ard Biesheuvel wrote:
> On 29 July 2014 20:27, Mark Salter <msalter@redhat.com> wrote:
> > On Tue, 2014-07-29 at 20:17 +0200, Ard Biesheuvel wrote:
> >> On 29 July 2014 17:29, Mark Salter <msalter@redhat.com> wrote:
> >> > On Tue, 2014-07-29 at 12:49 +0200, Ard Biesheuvel wrote:
> >> >> If we cannot relocate the kernel Image to its preferred offset of base of DRAM
> >> >> plus TEXT_OFFSET, instead relocate it to the lowest available 2 MB boundary plus
> >> >> TEXT_OFFSET. We may lose a bit of memory at the low end, but we can still
> >> >> proceed normally otherwise.
> >> >>
> >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> >> ---
> >> >>  arch/arm64/kernel/efi-stub.c | 16 ++++++----------
> >> >>  1 file changed, 6 insertions(+), 10 deletions(-)
> >> >
> >> > Tested on Mustang (with loss of 2MB free memory).
> >> >
> >>
> >> Great, thanks!
> >>
> >> >>
> >> >> diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c
> >> >> index 60e98a639ac5..460c00c41e57 100644
> >> >> --- a/arch/arm64/kernel/efi-stub.c
> >> >> +++ b/arch/arm64/kernel/efi-stub.c
> >> >> @@ -60,20 +60,16 @@ static efi_status_t handle_kernel_image(efi_system_table_t *sys_table,
> >> >>       kernel_size = _edata - _text;
> >> >>       if (*image_addr != (dram_base + TEXT_OFFSET)) {
> >> >>               kernel_memsize = kernel_size + (_end - _edata);
> >> >> -             status = efi_relocate_kernel(sys_table, image_addr,
> >> >> -                                          kernel_size, kernel_memsize,
> >> >> -                                          dram_base + TEXT_OFFSET,
> >> >> -                                          PAGE_SIZE);
> >> >
> >> > Can we make efi_relocate_kernel static inline to get rid
> >> > of the "defined but unused" warning?
> >> >
> >>
> >> I have some patches pending in the EFI tree to turn the stub into a
> >> static library, and that already takes care of this issue.
> >
> > That's fine if the static library stub patch goes in first. If this
> > patch goes in first, then let's avoid the warning since it is easy
> > to do.
> >
> 
> My idea was to ask Matt to take patches #2 and #3. I may have to fix
> them up slightly to apply correctly, but that's fine. Changing
> efi_relocate_kernel to static inline would need to go through Matt's
> tree as well, so there's probably no point in doing that in any case.

I'm not following. I would say there is no point in not doing that.
You have a patch which causes a build warning. Let's avoid that
unless there's a good reason not too. In this case, it seems easy
enough to avoid unless I'm missing something.

> 
> Patch #1 needs to go through the arm64, I guess. This means UEFI boot
> on APM Mustang will be broken during the short time between the
> x86/tip tree and the arm64 tree being merged for 3.17 (assuming 3.17
> is still open). I think we should be able to tolerate that, right?

Breaking bisect would be really bad. I think all three should be
pulled from the same tree. What's wrong with getting an ack from
Catalin or Will on the first patch and having all three go through
tip? Patch one is needed for EFI boot functionality even if it isn't
specifically EFI code. This won't be the last time this sort of 
situation arises.

> 
> >>
> >> > Otherwise:
> >> >
> >> > Acked-by: Mark Salter <msalter@redhat.com>
> >> >
> >>
> >> Cheers,
> >> Ard.
> >>
> >>
> >>
> >> >> +             status = efi_low_alloc(sys_table, kernel_memsize + TEXT_OFFSET,
> >> >> +                                    SZ_2M, reserve_addr);
> >> >>               if (status != EFI_SUCCESS) {
> >> >>                       pr_efi_err(sys_table, "Failed to relocate kernel\n");
> >> >>                       return status;
> >> >>               }
> >> >> -             if (*image_addr != (dram_base + TEXT_OFFSET)) {
> >> >> -                     pr_efi_err(sys_table, "Failed to alloc kernel memory\n");
> >> >> -                     efi_free(sys_table, kernel_memsize, *image_addr);
> >> >> -                     return EFI_ERROR;
> >> >> -             }
> >> >> -             *image_size = kernel_memsize;
> >> >> +             memcpy((void *)*reserve_addr + TEXT_OFFSET, (void *)*image_addr,
> >> >> +                    kernel_size);
> >> >> +             *image_addr = *reserve_addr + TEXT_OFFSET;
> >> >> +             *reserve_size = kernel_memsize + TEXT_OFFSET;
> >> >>       }
> >> >>
> >> >>
> >> >
> >> >
> >
> >

[PATCH 3/3] arm64/efi: efistub: don't abort if base of DRAM is occupied

[Ard Biesheuvel]

On 29 July 2014 21:20, Mark Salter <msalter@redhat.com> wrote:
> On Tue, 2014-07-29 at 20:46 +0200, Ard Biesheuvel wrote:
>> On 29 July 2014 20:27, Mark Salter <msalter@redhat.com> wrote:
>> > On Tue, 2014-07-29 at 20:17 +0200, Ard Biesheuvel wrote:
>> >> On 29 July 2014 17:29, Mark Salter <msalter@redhat.com> wrote:
>> >> > On Tue, 2014-07-29 at 12:49 +0200, Ard Biesheuvel wrote:
>> >> >> If we cannot relocate the kernel Image to its preferred offset of base of DRAM
>> >> >> plus TEXT_OFFSET, instead relocate it to the lowest available 2 MB boundary plus
>> >> >> TEXT_OFFSET. We may lose a bit of memory at the low end, but we can still
>> >> >> proceed normally otherwise.
>> >> >>
>> >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >> >> ---
>> >> >>  arch/arm64/kernel/efi-stub.c | 16 ++++++----------
>> >> >>  1 file changed, 6 insertions(+), 10 deletions(-)
>> >> >
>> >> > Tested on Mustang (with loss of 2MB free memory).
>> >> >
>> >>
>> >> Great, thanks!
>> >>
>> >> >>
>> >> >> diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c
>> >> >> index 60e98a639ac5..460c00c41e57 100644
>> >> >> --- a/arch/arm64/kernel/efi-stub.c
>> >> >> +++ b/arch/arm64/kernel/efi-stub.c
>> >> >> @@ -60,20 +60,16 @@ static efi_status_t handle_kernel_image(efi_system_table_t *sys_table,
>> >> >>       kernel_size = _edata - _text;
>> >> >>       if (*image_addr != (dram_base + TEXT_OFFSET)) {
>> >> >>               kernel_memsize = kernel_size + (_end - _edata);
>> >> >> -             status = efi_relocate_kernel(sys_table, image_addr,
>> >> >> -                                          kernel_size, kernel_memsize,
>> >> >> -                                          dram_base + TEXT_OFFSET,
>> >> >> -                                          PAGE_SIZE);
>> >> >
>> >> > Can we make efi_relocate_kernel static inline to get rid
>> >> > of the "defined but unused" warning?
>> >> >
>> >>
>> >> I have some patches pending in the EFI tree to turn the stub into a
>> >> static library, and that already takes care of this issue.
>> >
>> > That's fine if the static library stub patch goes in first. If this
>> > patch goes in first, then let's avoid the warning since it is easy
>> > to do.
>> >
>>
>> My idea was to ask Matt to take patches #2 and #3. I may have to fix
>> them up slightly to apply correctly, but that's fine. Changing
>> efi_relocate_kernel to static inline would need to go through Matt's
>> tree as well, so there's probably no point in doing that in any case.
>
> I'm not following. I would say there is no point in not doing that.
> You have a patch which causes a build warning. Let's avoid that
> unless there's a good reason not too. In this case, it seems easy
> enough to avoid unless I'm missing something.
>

Agreed.

The static library patches are already queued up in x86/tip, so if we
add a patch that changes efi_relocate_kernel() in
drivers/firmware/efi, it either goes through efi and comes after the
static lib patches, which makes it redundant, or it goes through arm64
and causes a conflict.

>>
>> Patch #1 needs to go through the arm64, I guess. This means UEFI boot
>> on APM Mustang will be broken during the short time between the
>> x86/tip tree and the arm64 tree being merged for 3.17 (assuming 3.17
>> is still open). I think we should be able to tolerate that, right?
>
> Breaking bisect would be really bad. I think all three should be
> pulled from the same tree. What's wrong with getting an ack from
> Catalin or Will on the first patch and having all three go through
> tip? Patch one is needed for EFI boot functionality even if it isn't
> specifically EFI code. This won't be the last time this sort of
> situation arises.
>

Ah, right, I forgot about bisect. So yes, patch #1 should definitely
go in before #2 and #3, which is indeed easiest if they get merged
through the same tree.
So yes, taking all of these through the EFI tree is indeed the way to go ...

-- 
Ard.


>>
>> >>
>> >> > Otherwise:
>> >> >
>> >> > Acked-by: Mark Salter <msalter@redhat.com>
>> >> >
>> >>
>> >> Cheers,
>> >> Ard.
>> >>
>> >>
>> >>
>> >> >> +             status = efi_low_alloc(sys_table, kernel_memsize + TEXT_OFFSET,
>> >> >> +                                    SZ_2M, reserve_addr);
>> >> >>               if (status != EFI_SUCCESS) {
>> >> >>                       pr_efi_err(sys_table, "Failed to relocate kernel\n");
>> >> >>                       return status;
>> >> >>               }
>> >> >> -             if (*image_addr != (dram_base + TEXT_OFFSET)) {
>> >> >> -                     pr_efi_err(sys_table, "Failed to alloc kernel memory\n");
>> >> >> -                     efi_free(sys_table, kernel_memsize, *image_addr);
>> >> >> -                     return EFI_ERROR;
>> >> >> -             }
>> >> >> -             *image_size = kernel_memsize;
>> >> >> +             memcpy((void *)*reserve_addr + TEXT_OFFSET, (void *)*image_addr,
>> >> >> +                    kernel_size);
>> >> >> +             *image_addr = *reserve_addr + TEXT_OFFSET;
>> >> >> +             *reserve_size = kernel_memsize + TEXT_OFFSET;
>> >> >>       }
>> >> >>
>> >> >>
>> >> >
>> >> >
>> >
>> >
>
>