[Xen-devel] xen/arm64: Don't zero BSS when booting using EFI

Message ID 1486550894-10830-1-git-send-email-julien.grall@arm.com
State New
Headers show

Commit Message

Julien Grall Feb. 8, 2017, 10:48 a.m.
Commit 146786b "efi: create efi_enabled()" introduced a variable
efi_flags stored in BSS and used to pass information between the stub
and Xen. However on ARM, BSS is zeroed after the stub has finished to
run and before Xen is started. This means that the bits set in efi_flags
will be lost.

We were not affected before because all the variables used to pass
information between Xen and the stub are living in initdata or data.

Looking at the description of the field SizeOfRawData in the PE/COFF
header (see [1]):

"If this is less than VirtualSize, the remainder of the section is
zero-filled. Because the SizeOfRawData field is rounded but the
VirtualSize field is not, it is possible for SizeOfRawData to be greater
than VirtualSize as well. When a section contains only uninitialized
data, this field should be zero."

Both VirtualSize and SizeOfRawData are correctly set in the header (see
arch/arm/arm64/head.S) so the EFI firmware will zero BSS for us.

Therefore we don't need to zero BSS before running the EFI stub and can
skip the one between the EFI stub and Xen.

To avoid another branch instruction, slightly refactor the code. The
register x26 is allocated to hold whether BSS is skipped. The value will
be:
    - 0 when the code is running on CPU0 and EFI is not used
    - 1 when EFI is used or running on other processor than the boot one.

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/ms680547(v=vs.85).aspx

Signed-off-by: Julien Grall <julien.grall@arm.com>

---
CC: Jan Beulich <jbeulich@suse.com>
CC: Daniel Kiper <daniel.kiper@oracle.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>

    This patch fix ACPI boot on Xen ARM. Without it Xen thinks it is not
    running on EFI and will not try to find the RDSP.

    I did not put the reviewed-by/acked-by tag as there was some request
    to clarify the comments.

    Changes in v2:
        - Update comment in the code
---
 xen/arch/arm/arm64/head.S | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

Comments

Daniel Kiper Feb. 8, 2017, 12:10 p.m. | #1
On Wed, Feb 08, 2017 at 10:48:14AM +0000, Julien Grall wrote:
> Commit 146786b "efi: create efi_enabled()" introduced a variable
> efi_flags stored in BSS and used to pass information between the stub
> and Xen. However on ARM, BSS is zeroed after the stub has finished to
> run and before Xen is started. This means that the bits set in efi_flags
> will be lost.
>
> We were not affected before because all the variables used to pass
> information between Xen and the stub are living in initdata or data.
>
> Looking at the description of the field SizeOfRawData in the PE/COFF
> header (see [1]):
>
> "If this is less than VirtualSize, the remainder of the section is
> zero-filled. Because the SizeOfRawData field is rounded but the
> VirtualSize field is not, it is possible for SizeOfRawData to be greater
> than VirtualSize as well. When a section contains only uninitialized
> data, this field should be zero."
>
> Both VirtualSize and SizeOfRawData are correctly set in the header (see
> arch/arm/arm64/head.S) so the EFI firmware will zero BSS for us.
>
> Therefore we don't need to zero BSS before running the EFI stub and can
> skip the one between the EFI stub and Xen.
>
> To avoid another branch instruction, slightly refactor the code. The
> register x26 is allocated to hold whether BSS is skipped. The value will
> be:
>     - 0 when the code is running on CPU0 and EFI is not used
>     - 1 when EFI is used or running on other processor than the boot one.
>
> [1] https://msdn.microsoft.com/en-us/library/windows/desktop/ms680547(v=vs.85).aspx
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel
Stefano Stabellini Feb. 8, 2017, 11:31 p.m. | #2
On Wed, 8 Feb 2017, Julien Grall wrote:
> Commit 146786b "efi: create efi_enabled()" introduced a variable
> efi_flags stored in BSS and used to pass information between the stub
> and Xen. However on ARM, BSS is zeroed after the stub has finished to
> run and before Xen is started. This means that the bits set in efi_flags
> will be lost.
> 
> We were not affected before because all the variables used to pass
> information between Xen and the stub are living in initdata or data.
> 
> Looking at the description of the field SizeOfRawData in the PE/COFF
> header (see [1]):
> 
> "If this is less than VirtualSize, the remainder of the section is
> zero-filled. Because the SizeOfRawData field is rounded but the
> VirtualSize field is not, it is possible for SizeOfRawData to be greater
> than VirtualSize as well. When a section contains only uninitialized
> data, this field should be zero."
> 
> Both VirtualSize and SizeOfRawData are correctly set in the header (see
> arch/arm/arm64/head.S) so the EFI firmware will zero BSS for us.
> 
> Therefore we don't need to zero BSS before running the EFI stub and can
> skip the one between the EFI stub and Xen.
> 
> To avoid another branch instruction, slightly refactor the code. The
> register x26 is allocated to hold whether BSS is skipped. The value will
> be:
>     - 0 when the code is running on CPU0 and EFI is not used
>     - 1 when EFI is used or running on other processor than the boot one.
> 
> [1] https://msdn.microsoft.com/en-us/library/windows/desktop/ms680547(v=vs.85).aspx
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Daniel Kiper <daniel.kiper@oracle.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> 
>     This patch fix ACPI boot on Xen ARM. Without it Xen thinks it is not
>     running on EFI and will not try to find the RDSP.
> 
>     I did not put the reviewed-by/acked-by tag as there was some request
>     to clarify the comments.
> 
>     Changes in v2:
>         - Update comment in the code
> ---
>  xen/arch/arm/arm64/head.S | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 3f63d2a..3a54356 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -65,7 +65,7 @@
>   *  x23 - UART address
>   *  x24 - cpuid
>   *  x25 - identity map in place
> - *  x26 -
> + *  x26 - skip_zero_bss
>   *  x27 -
>   *  x28 -
>   *  x29 -
> @@ -232,6 +232,10 @@ section_table:
>          .long   0xe0500020       /* Characteristics (section flags) */
>          .align  5
>  real_start:
> +        /* BSS should be zeroed when booting without EFI */
> +        mov   x26, #0                /* x26 := skip_zero_bss */
> +
> +real_start_efi:
>          msr   DAIFSet, 0xf           /* Disable all interrupts */
>  
>          /* Save the bootloader arguments in less-clobberable registers */
> @@ -261,6 +265,8 @@ GLOBAL(init_secondary)
>          sub   x20, x19, x0           /* x20 := phys-offset */
>  
>          mov   x22, #1                /* x22 := is_secondary_cpu */
> +        /* Boot CPU already zero BSS so skip it on secondary CPUs. */
> +        mov   x26, #1                /* X26 := skip_zero_bss */
>  
>  common_start:
>          mov   x24, #0                /* x24 := CPU ID. Initialy zero until we
> @@ -314,8 +320,8 @@ common_start:
>  
>  el2:    PRINT("- Xen starting at EL2 -\r\n")
>  
> -        /* Zero BSS On the boot CPU to avoid nasty surprises */
> -        cbnz  x22, skip_bss
> +        /* Zero BSS only when requested */
> +        cbnz  x26, skip_bss
>  
>          PRINT("- Zero BSS -\r\n")
>          ldr   x0, =__bss_start       /* Load start & end of bss */
> @@ -787,7 +793,16 @@ ENTRY(efi_xen_start)
>          mov   x1, xzr
>          mov   x2, xzr
>          mov   x3, xzr
> -        b     real_start
> +        /*
> +         * The EFI stub and Xen may share some information living in
> +         * BSS. Don't zero BSS to avoid loosing them.
> +         *
> +         * Note that the EFI firmware has already zeroed BSS for us
> +         * before jump into the stub.
> +         */
> +        mov   x26, #1               /* x26 := skip_zero_bss */
> +
> +        b     real_start_efi
>  ENDPROC(efi_xen_start)
>  
>  /*
> -- 
> 1.9.1
>

Patch hide | download patch | download mbox

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 3f63d2a..3a54356 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -65,7 +65,7 @@ 
  *  x23 - UART address
  *  x24 - cpuid
  *  x25 - identity map in place
- *  x26 -
+ *  x26 - skip_zero_bss
  *  x27 -
  *  x28 -
  *  x29 -
@@ -232,6 +232,10 @@  section_table:
         .long   0xe0500020       /* Characteristics (section flags) */
         .align  5
 real_start:
+        /* BSS should be zeroed when booting without EFI */
+        mov   x26, #0                /* x26 := skip_zero_bss */
+
+real_start_efi:
         msr   DAIFSet, 0xf           /* Disable all interrupts */
 
         /* Save the bootloader arguments in less-clobberable registers */
@@ -261,6 +265,8 @@  GLOBAL(init_secondary)
         sub   x20, x19, x0           /* x20 := phys-offset */
 
         mov   x22, #1                /* x22 := is_secondary_cpu */
+        /* Boot CPU already zero BSS so skip it on secondary CPUs. */
+        mov   x26, #1                /* X26 := skip_zero_bss */
 
 common_start:
         mov   x24, #0                /* x24 := CPU ID. Initialy zero until we
@@ -314,8 +320,8 @@  common_start:
 
 el2:    PRINT("- Xen starting at EL2 -\r\n")
 
-        /* Zero BSS On the boot CPU to avoid nasty surprises */
-        cbnz  x22, skip_bss
+        /* Zero BSS only when requested */
+        cbnz  x26, skip_bss
 
         PRINT("- Zero BSS -\r\n")
         ldr   x0, =__bss_start       /* Load start & end of bss */
@@ -787,7 +793,16 @@  ENTRY(efi_xen_start)
         mov   x1, xzr
         mov   x2, xzr
         mov   x3, xzr
-        b     real_start
+        /*
+         * The EFI stub and Xen may share some information living in
+         * BSS. Don't zero BSS to avoid loosing them.
+         *
+         * Note that the EFI firmware has already zeroed BSS for us
+         * before jump into the stub.
+         */
+        mov   x26, #1               /* x26 := skip_zero_bss */
+
+        b     real_start_efi
 ENDPROC(efi_xen_start)
 
 /*