| Message ID | 1486550894-10830-1-git-send-email-julien.grall@arm.com |
|---|---|
| State | New |
| Headers | show |
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
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 >
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) /*
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(-)