[PATCH 0/4] i386-efi and x86_64-efi fixes

[PATCH 0/4] i386-efi and x86_64-efi fixes

[Josh Triplett]

The first three patches improve the EFI firmware allocator, fixing some bugs in
the process.

The last patch fixes many compilation-specific and firmware-specific crashes.

To avoid unnecessary patch conflicts, I've included appropriately formatted
ChangeLog entries for each patch separately in each mail, below the commit
messages and above the diffstats.

Josh Triplett (4):
  efi: Fix firmware memory allocation to round to 4k pages, not 1k
  efi: Fix requests to allocate GRUB_MEMORY_AVAILABLE
  efi: Support GRUB_MMAP_MALLOC_LOW in the EFI firmware allocator
  efi: On x86-64, align the stack to a 16-byte boundary as required by
    ABI

 grub-core/kern/x86_64/efi/startup.S |  6 +++++-
 grub-core/mmap/efi/mmap.c           | 23 ++++++++++++++---------
 include/grub/efi/memory.h           |  2 ++
 3 files changed, 21 insertions(+), 10 deletions(-)

-- 
1.8.4.3

[PATCH 1/4] efi: Fix firmware memory allocation to round to 4k pages, not 1k

[Josh Triplett]

EFI memory allocation operates in 4k pages, but the firmware memory
allocator used 0x3ff in several places rather than 0xfff.
---

ChangeLog entry:

2013-11-13  Josh Triplett  <josh@joshtriplett.org>

	* grub-core/mmap/efi/mmap.c (grub_mmap_register): Round up/down to
	  4k page boundaries as expected by firmware rather than 1k
	  boundaries.
	  (grub_mmap_malign_and_register): Likewise.

 grub-core/mmap/efi/mmap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/grub-core/mmap/efi/mmap.c b/grub-core/mmap/efi/mmap.c
index 4f17c8b..a77efe8 100644
--- a/grub-core/mmap/efi/mmap.c
+++ b/grub-core/mmap/efi/mmap.c
@@ -184,8 +184,8 @@ grub_mmap_register (grub_uint64_t start, grub_uint64_t size, int type)
     return 0;
 
   b = grub_efi_system_table->boot_services;
-  address = start & (~0x3ffULL);
-  pages = (end - address  + 0x3ff) >> 12;
+  address = start & (~0xfffULL);
+  pages = (end - address + 0xfff) >> 12;
   status = efi_call_2 (b->free_pages, address, pages);
   if (status != GRUB_EFI_SUCCESS && status != GRUB_EFI_NOT_FOUND)
     {
@@ -263,7 +263,7 @@ grub_mmap_malign_and_register (grub_uint64_t align __attribute__ ((unused)),
   atype = GRUB_EFI_ALLOCATE_ANY_PAGES;
 #endif
 
-  pages = (size + 0x3ff) >> 12;
+  pages = (size + 0xfff) >> 12;
   status = efi_call_4 (b->allocate_pages, atype,
 		       make_efi_memtype (type), pages, &address);
   if (status != GRUB_EFI_SUCCESS)
-- 
1.8.4.3

Re: [PATCH 1/4] efi: Fix firmware memory allocation to round to 4k pages, not 1k

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 325 bytes --]

On 13.11.2013 03:26, Josh Triplett wrote:
> 2013-11-13  Josh Triplett  <josh@joshtriplett.org>
> 
> 	* grub-core/mmap/efi/mmap.c (grub_mmap_register): Round up/down to
> 	  4k page boundaries as expected by firmware rather than 1k
> 	  boundaries.
> 	  (grub_mmap_malign_and_register): Likewise.
Committed, thanks.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]

[PATCH 2/4] efi: Fix requests to allocate GRUB_MEMORY_AVAILABLE

[Josh Triplett]

EFI firmware refuses to allocate memory of type
GRUB_EFI_CONVENTIONAL_MEMORY, because that indicates a block of
available memory that other allocations (or the OS) would then step on.
Map GRUB_MEMORY_AVAILABLE to GRUB_EFI_LOADER_CODE instead.
---

ChangeLog entry:

2013-11-13  Josh Triplett  <josh@joshtriplett.org>

	* grub-core/mmap/efi/mmap.c (make_efi_memtype): Map
	  GRUB_MEMORY_AVAILABLE to GRUB_EFI_LOADER_CODE rather than
	  GRUB_EFI_CONVENTIONAL_MEMORY.  EFI firmware refuses to allocate
	  memory of type GRUB_EFI_CONVENTIONAL_MEMORY, because that
	  indicates a block of available memory that other allocations (or
	  the OS) would then step on.

 grub-core/mmap/efi/mmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/mmap/efi/mmap.c b/grub-core/mmap/efi/mmap.c
index a77efe8..e6cd185 100644
--- a/grub-core/mmap/efi/mmap.c
+++ b/grub-core/mmap/efi/mmap.c
@@ -146,7 +146,7 @@ make_efi_memtype (int type)
       return GRUB_EFI_UNUSABLE_MEMORY;
 
     case GRUB_MEMORY_AVAILABLE:
-      return GRUB_EFI_CONVENTIONAL_MEMORY;
+      return GRUB_EFI_LOADER_CODE;
 
     case GRUB_MEMORY_ACPI:
       return GRUB_EFI_ACPI_RECLAIM_MEMORY;
-- 
1.8.4.3

Re: [PATCH 2/4] efi: Fix requests to allocate GRUB_MEMORY_AVAILABLE

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1522 bytes --]

On 13.11.2013 03:26, Josh Triplett wrote:
> EFI firmware refuses to allocate memory of type
> GRUB_EFI_CONVENTIONAL_MEMORY, because that indicates a block of
> available memory that other allocations (or the OS) would then step on.
> Map GRUB_MEMORY_AVAILABLE to GRUB_EFI_LOADER_CODE instead.
> ---
> 
> ChangeLog entry:
> 
> 2013-11-13  Josh Triplett  <josh@joshtriplett.org>
> 
> 	* grub-core/mmap/efi/mmap.c (make_efi_memtype): Map
> 	  GRUB_MEMORY_AVAILABLE to GRUB_EFI_LOADER_CODE rather than
> 	  GRUB_EFI_CONVENTIONAL_MEMORY.  EFI firmware refuses to allocate
> 	  memory of type GRUB_EFI_CONVENTIONAL_MEMORY, because that
> 	  indicates a block of available memory that other allocations (or
> 	  the OS) would then step on.
> 
"allocating" with type GRUB_MEMORY_AVAILABLE isn't actually allocating
but declaring a new chunk of available memory not discovered by
firmware. Up until now we've never actually needed it
>  grub-core/mmap/efi/mmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/grub-core/mmap/efi/mmap.c b/grub-core/mmap/efi/mmap.c
> index a77efe8..e6cd185 100644
> --- a/grub-core/mmap/efi/mmap.c
> +++ b/grub-core/mmap/efi/mmap.c
> @@ -146,7 +146,7 @@ make_efi_memtype (int type)
>        return GRUB_EFI_UNUSABLE_MEMORY;
>  
>      case GRUB_MEMORY_AVAILABLE:
> -      return GRUB_EFI_CONVENTIONAL_MEMORY;
> +      return GRUB_EFI_LOADER_CODE;
>  
>      case GRUB_MEMORY_ACPI:
>        return GRUB_EFI_ACPI_RECLAIM_MEMORY;
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]

[PATCH 3/4] efi: Support GRUB_MMAP_MALLOC_LOW in the EFI firmware allocator

[Josh Triplett]

EFI supports allocating memory below a specified address; use that to
implement GRUB_MMAP_MALLOC_LOW by requesting memory below 1M.
---

ChangeLog entry:

2013-11-13  Josh Triplett  <josh@joshtriplett.org>

	* include/grub/efi/memory.h (GRUB_MMAP_MALLOC_LOW): Define.
	* grub-core/mmap/efi/mmap.c (grub_mmap_malign_and_register): Add
	  support for GRUB_MMAP_MALLOC_LOW, to allocate memory below 1M via the
	  EFI firmware.

 grub-core/mmap/efi/mmap.c | 15 ++++++++++-----
 include/grub/efi/memory.h |  2 ++
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/grub-core/mmap/efi/mmap.c b/grub-core/mmap/efi/mmap.c
index e6cd185..64ad05c 100644
--- a/grub-core/mmap/efi/mmap.c
+++ b/grub-core/mmap/efi/mmap.c
@@ -239,9 +239,9 @@ void *
 grub_mmap_malign_and_register (grub_uint64_t align __attribute__ ((unused)),
 			       grub_uint64_t size,
 			       int *handle, int type,
-			       int flags __attribute__ ((unused)))
+			       int flags)
 {
-  grub_efi_physical_address_t address;
+  grub_efi_physical_address_t address, max_address;
   grub_efi_boot_services_t *b;
   grub_efi_uintn_t pages;
   grub_efi_status_t status;
@@ -254,13 +254,18 @@ grub_mmap_malign_and_register (grub_uint64_t align __attribute__ ((unused)),
 
   b = grub_efi_system_table->boot_services;
 
-  address = 0xffffffff;
+  if (flags & GRUB_MMAP_MALLOC_LOW)
+      max_address = 0xfffff;
+  else
+      max_address = 0xffffffff;
+  address = max_address;
 
 #if GRUB_TARGET_SIZEOF_VOID_P < 8
   /* Limit the memory access to less than 4GB for 32-bit platforms.  */
   atype = GRUB_EFI_ALLOCATE_MAX_ADDRESS;
 #else
-  atype = GRUB_EFI_ALLOCATE_ANY_PAGES;
+  atype = (flags & GRUB_MMAP_MALLOC_LOW) ? GRUB_EFI_ALLOCATE_MAX_ADDRESS
+                                         : GRUB_EFI_ALLOCATE_ANY_PAGES;
 #endif
 
   pages = (size + 0xfff) >> 12;
@@ -276,7 +281,7 @@ grub_mmap_malign_and_register (grub_uint64_t align __attribute__ ((unused)),
     {
       /* Uggh, the address 0 was allocated... This is too annoying,
 	 so reallocate another one.  */
-      address = 0xffffffff;
+      address = max_address;
       status = efi_call_4 (b->allocate_pages, atype,
 			   make_efi_memtype (type), pages, &address);
       grub_efi_free_pages (0, pages);
diff --git a/include/grub/efi/memory.h b/include/grub/efi/memory.h
index 20526b1..b4940af 100644
--- a/include/grub/efi/memory.h
+++ b/include/grub/efi/memory.h
@@ -24,6 +24,8 @@
 
 #define GRUB_MMAP_REGISTER_BY_FIRMWARE  1
 
+#define GRUB_MMAP_MALLOC_LOW 1
+
 grub_err_t grub_machine_mmap_register (grub_uint64_t start, grub_uint64_t size,
 				       int type, int handle);
 grub_err_t grub_machine_mmap_unregister (int handle);
-- 
1.8.4.3

Re: [PATCH 3/4] efi: Support GRUB_MMAP_MALLOC_LOW in the EFI firmware allocator

[Andrey Borzenkov]

В Tue, 12 Nov 2013 18:26:39 -0800
Josh Triplett <josh@joshtriplett.org> пишет:

> EFI supports allocating memory below a specified address; use that to
> implement GRUB_MMAP_MALLOC_LOW by requesting memory below 1M.

Out of curiosity - why would you need it on EFI? Your patch does not
add any consumer of GRUB_MMAP_MALLOC_LOW and on EFI notion of low
memory should not exist?

> ---
> 
> ChangeLog entry:
> 
> 2013-11-13  Josh Triplett  <josh@joshtriplett.org>
> 
> 	* include/grub/efi/memory.h (GRUB_MMAP_MALLOC_LOW): Define.
> 	* grub-core/mmap/efi/mmap.c (grub_mmap_malign_and_register): Add
> 	  support for GRUB_MMAP_MALLOC_LOW, to allocate memory below 1M via the
> 	  EFI firmware.
> 
>  grub-core/mmap/efi/mmap.c | 15 ++++++++++-----
>  include/grub/efi/memory.h |  2 ++
>  2 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/grub-core/mmap/efi/mmap.c b/grub-core/mmap/efi/mmap.c
> index e6cd185..64ad05c 100644
> --- a/grub-core/mmap/efi/mmap.c
> +++ b/grub-core/mmap/efi/mmap.c
> @@ -239,9 +239,9 @@ void *
>  grub_mmap_malign_and_register (grub_uint64_t align __attribute__ ((unused)),
>  			       grub_uint64_t size,
>  			       int *handle, int type,
> -			       int flags __attribute__ ((unused)))
> +			       int flags)
>  {
> -  grub_efi_physical_address_t address;
> +  grub_efi_physical_address_t address, max_address;
>    grub_efi_boot_services_t *b;
>    grub_efi_uintn_t pages;
>    grub_efi_status_t status;
> @@ -254,13 +254,18 @@ grub_mmap_malign_and_register (grub_uint64_t align __attribute__ ((unused)),
>  
>    b = grub_efi_system_table->boot_services;
>  
> -  address = 0xffffffff;
> +  if (flags & GRUB_MMAP_MALLOC_LOW)
> +      max_address = 0xfffff;
> +  else
> +      max_address = 0xffffffff;
> +  address = max_address;
>  
>  #if GRUB_TARGET_SIZEOF_VOID_P < 8
>    /* Limit the memory access to less than 4GB for 32-bit platforms.  */
>    atype = GRUB_EFI_ALLOCATE_MAX_ADDRESS;
>  #else
> -  atype = GRUB_EFI_ALLOCATE_ANY_PAGES;
> +  atype = (flags & GRUB_MMAP_MALLOC_LOW) ? GRUB_EFI_ALLOCATE_MAX_ADDRESS
> +                                         : GRUB_EFI_ALLOCATE_ANY_PAGES;
>  #endif
>  
>    pages = (size + 0xfff) >> 12;
> @@ -276,7 +281,7 @@ grub_mmap_malign_and_register (grub_uint64_t align __attribute__ ((unused)),
>      {
>        /* Uggh, the address 0 was allocated... This is too annoying,
>  	 so reallocate another one.  */
> -      address = 0xffffffff;
> +      address = max_address;
>        status = efi_call_4 (b->allocate_pages, atype,
>  			   make_efi_memtype (type), pages, &address);
>        grub_efi_free_pages (0, pages);
> diff --git a/include/grub/efi/memory.h b/include/grub/efi/memory.h
> index 20526b1..b4940af 100644
> --- a/include/grub/efi/memory.h
> +++ b/include/grub/efi/memory.h
> @@ -24,6 +24,8 @@
>  
>  #define GRUB_MMAP_REGISTER_BY_FIRMWARE  1
>  
> +#define GRUB_MMAP_MALLOC_LOW 1
> +
>  grub_err_t grub_machine_mmap_register (grub_uint64_t start, grub_uint64_t size,
>  				       int type, int handle);
>  grub_err_t grub_machine_mmap_unregister (int handle);

Re: [PATCH 3/4] efi: Support GRUB_MMAP_MALLOC_LOW in the EFI firmware allocator

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 589 bytes --]

On 13.11.2013 04:59, Andrey Borzenkov wrote:
> В Tue, 12 Nov 2013 18:26:39 -0800
> Josh Triplett <josh@joshtriplett.org> пишет:
> 
>> EFI supports allocating memory below a specified address; use that to
>> implement GRUB_MMAP_MALLOC_LOW by requesting memory below 1M.
> 
> Out of curiosity - why would you need it on EFI? Your patch does not
> add any consumer of GRUB_MMAP_MALLOC_LOW and on EFI notion of low
> memory should not exist?
> 
As discussed on IRC, it's for starting other CPUs, for this you need a
vector in low memory as cpus always start in real mode.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]

Re: [PATCH 3/4] efi: Support GRUB_MMAP_MALLOC_LOW in the EFI firmware allocator

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 3401 bytes --]

On 13.11.2013 03:26, Josh Triplett wrote:
> EFI supports allocating memory below a specified address; use that to
> implement GRUB_MMAP_MALLOC_LOW by requesting memory below 1M.
As discussed on the IRC, using mmap isn't right way to allocate
transient low memory space. This would merit a separate functions
(alloc_low_page and free_low_page) which uses EFI calls on EFI and
returns the scratch address GRUB_MEMORY_MACHINE_SCRATCH_ADDR on other
platforms. These would be subjected to following rules:
- Always gives only 4k
- Consumer has to ensure the transient nature of allocation since low
memory is scarce.
> ---
> 
> ChangeLog entry:
> 
> 2013-11-13  Josh Triplett  <josh@joshtriplett.org>
> 
> 	* include/grub/efi/memory.h (GRUB_MMAP_MALLOC_LOW): Define.
> 	* grub-core/mmap/efi/mmap.c (grub_mmap_malign_and_register): Add
> 	  support for GRUB_MMAP_MALLOC_LOW, to allocate memory below 1M via the
> 	  EFI firmware.
> 
>  grub-core/mmap/efi/mmap.c | 15 ++++++++++-----
>  include/grub/efi/memory.h |  2 ++
>  2 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/grub-core/mmap/efi/mmap.c b/grub-core/mmap/efi/mmap.c
> index e6cd185..64ad05c 100644
> --- a/grub-core/mmap/efi/mmap.c
> +++ b/grub-core/mmap/efi/mmap.c
> @@ -239,9 +239,9 @@ void *
>  grub_mmap_malign_and_register (grub_uint64_t align __attribute__ ((unused)),
>  			       grub_uint64_t size,
>  			       int *handle, int type,
> -			       int flags __attribute__ ((unused)))
> +			       int flags)
>  {
> -  grub_efi_physical_address_t address;
> +  grub_efi_physical_address_t address, max_address;
>    grub_efi_boot_services_t *b;
>    grub_efi_uintn_t pages;
>    grub_efi_status_t status;
> @@ -254,13 +254,18 @@ grub_mmap_malign_and_register (grub_uint64_t align __attribute__ ((unused)),
>  
>    b = grub_efi_system_table->boot_services;
>  
> -  address = 0xffffffff;
> +  if (flags & GRUB_MMAP_MALLOC_LOW)
> +      max_address = 0xfffff;
> +  else
> +      max_address = 0xffffffff;
> +  address = max_address;
>  
>  #if GRUB_TARGET_SIZEOF_VOID_P < 8
>    /* Limit the memory access to less than 4GB for 32-bit platforms.  */
>    atype = GRUB_EFI_ALLOCATE_MAX_ADDRESS;
>  #else
> -  atype = GRUB_EFI_ALLOCATE_ANY_PAGES;
> +  atype = (flags & GRUB_MMAP_MALLOC_LOW) ? GRUB_EFI_ALLOCATE_MAX_ADDRESS
> +                                         : GRUB_EFI_ALLOCATE_ANY_PAGES;
>  #endif
>  
>    pages = (size + 0xfff) >> 12;
> @@ -276,7 +281,7 @@ grub_mmap_malign_and_register (grub_uint64_t align __attribute__ ((unused)),
>      {
>        /* Uggh, the address 0 was allocated... This is too annoying,
>  	 so reallocate another one.  */
> -      address = 0xffffffff;
> +      address = max_address;
>        status = efi_call_4 (b->allocate_pages, atype,
>  			   make_efi_memtype (type), pages, &address);
>        grub_efi_free_pages (0, pages);
> diff --git a/include/grub/efi/memory.h b/include/grub/efi/memory.h
> index 20526b1..b4940af 100644
> --- a/include/grub/efi/memory.h
> +++ b/include/grub/efi/memory.h
> @@ -24,6 +24,8 @@
>  
>  #define GRUB_MMAP_REGISTER_BY_FIRMWARE  1
>  
> +#define GRUB_MMAP_MALLOC_LOW 1
> +
>  grub_err_t grub_machine_mmap_register (grub_uint64_t start, grub_uint64_t size,
>  				       int type, int handle);
>  grub_err_t grub_machine_mmap_unregister (int handle);
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]

[PATCH 4/4] efi: On x86-64, align the stack to a 16-byte boundary as required by ABI

[Josh Triplett]

The x86-64 ABI specification requires a 16-byte-aligned stack.  In some
cases, GCC emits code that assumes this alignment, which crashes if not
aligned.  The EFI firmware is also entitled to assume that stack
alignment without checking, and some firmware does make that assumption.
---

ChangeLog entry:

2013-11-13  Josh Triplett  <josh@joshtriplett.org>

	* grub-core/kern/x86_64/efi/startup.S (_start): Align the stack to a
	  16-byte boundary, as required by the x86-64 ABI, before calling
	  grub_main.  In some cases, GCC emits code that assumes this
	  alignment, which crashes if not aligned.  The EFI firmware is also
	  entitled to assume that stack alignment without checking.

 grub-core/kern/x86_64/efi/startup.S | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/grub-core/kern/x86_64/efi/startup.S b/grub-core/kern/x86_64/efi/startup.S
index f86f019..94bd6ae 100644
--- a/grub-core/kern/x86_64/efi/startup.S
+++ b/grub-core/kern/x86_64/efi/startup.S
@@ -29,7 +29,11 @@ start:
 _start:
 	movq	%rcx, EXT_C(grub_efi_image_handle)(%rip)
 	movq	%rdx, EXT_C(grub_efi_system_table)(%rip)
-
+	mov	%rsp, %rax
+	subq	$8, %rsp
+	and	$~0xf, %rsp
+	mov	%rax, (%rsp)
 	call	EXT_C(grub_main)
+	mov	(%rsp), %rsp
 	ret
 
-- 
1.8.4.3

Re: [PATCH 4/4] efi: On x86-64, align the stack to a 16-byte boundary as required by ABI

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 752 bytes --]

> 
> diff --git a/grub-core/kern/x86_64/efi/startup.S b/grub-core/kern/x86_64/efi/startup.S
> index f86f019..94bd6ae 100644
> --- a/grub-core/kern/x86_64/efi/startup.S
> +++ b/grub-core/kern/x86_64/efi/startup.S
> @@ -29,7 +29,11 @@ start:
>  _start:
>  	movq	%rcx, EXT_C(grub_efi_image_handle)(%rip)
>  	movq	%rdx, EXT_C(grub_efi_system_table)(%rip)
> -
> +	mov	%rsp, %rax
> +	subq	$8, %rsp
> +	and	$~0xf, %rsp
> +	mov	%rax, (%rsp)
>  	call	EXT_C(grub_main)
> +	mov	(%rsp), %rsp
This would be useful if we ever returned but we never do (we call
boot_services->exit instead). Could you instead prepare a patch which
does stack alignment and removed the "ret" putting instead a comment /*
Doesn't return.  */ ?
>  	ret
>  
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]

Re: [PATCH 4/4] efi: On x86-64, align the stack to a 16-byte boundary as required by ABI

[Jordan Justen]

On Tue, Nov 12, 2013 at 6:27 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> The x86-64 ABI specification requires a 16-byte-aligned stack.  In some
> cases, GCC emits code that assumes this alignment, which crashes if not
> aligned.  The EFI firmware is also entitled to assume that stack
> alignment without checking, and some firmware does make that assumption.
> ---
>
> ChangeLog entry:
>
> 2013-11-13  Josh Triplett  <josh@joshtriplett.org>
>
>         * grub-core/kern/x86_64/efi/startup.S (_start): Align the stack to a
>           16-byte boundary, as required by the x86-64 ABI, before calling
>           grub_main.  In some cases, GCC emits code that assumes this
>           alignment, which crashes if not aligned.  The EFI firmware is also
>           entitled to assume that stack alignment without checking.
>
>  grub-core/kern/x86_64/efi/startup.S | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/kern/x86_64/efi/startup.S b/grub-core/kern/x86_64/efi/startup.S
> index f86f019..94bd6ae 100644
> --- a/grub-core/kern/x86_64/efi/startup.S
> +++ b/grub-core/kern/x86_64/efi/startup.S
> @@ -29,7 +29,11 @@ start:
>  _start:
>         movq    %rcx, EXT_C(grub_efi_image_handle)(%rip)
>         movq    %rdx, EXT_C(grub_efi_system_table)(%rip)
> -
> +       mov     %rsp, %rax
> +       subq    $8, %rsp
> +       and     $~0xf, %rsp
> +       mov     %rax, (%rsp)
>         call    EXT_C(grub_main)
> +       mov     (%rsp), %rsp

You can assume that the firmware followed the alignment convention, so
you just need to subtract 8 from the stack before calling, and add it
back after. Since rcx is not an output, how about:
push %rcx
call    EXT_C(grub_main)
pop %rcx

Or, use sub/add. Code might be larger, but would be more readable.

As far as Vladimir's comment about never returning, it seems like it
would be better to keep the path safe. But, either way, the comment
seems like a good idea.

-Jordan

Re: [PATCH 4/4] efi: On x86-64, align the stack to a 16-byte boundary as required by ABI

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 182 bytes --]

On 15.11.2013 08:15, Jordan Justen wrote:
> You can assume that the firmware followed the alignment convention,
Never assume that firmware followed anything unless you have to.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]

Re: [PATCH 4/4] efi: On x86-64, align the stack to a 16-byte boundary as required by ABI

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 482 bytes --]

On 13.11.2013 03:27, Josh Triplett wrote:
> 2013-11-13  Josh Triplett  <josh@joshtriplett.org>
> 
> 	* grub-core/kern/x86_64/efi/startup.S (_start): Align the stack to a
> 	  16-byte boundary, as required by the x86-64 ABI, before calling
> 	  grub_main.  In some cases, GCC emits code that assumes this
> 	  alignment, which crashes if not aligned.  The EFI firmware is also
> 	  entitled to assume that stack alignment without checking.
Committed with changes, thanks.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]