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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/02/2017 23:25, Stefano Stabellini wrote:
> On Thu, 2 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@xxxxxxx>
>>
>> ---
>> CC: Jan Beulich <jbeulich@xxxxxxxx>
>> CC: Daniel Kiper <daniel.kiper@xxxxxxxxxx>
>> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>
>>     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.
>> ---
>>  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..8cb4602 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 with efi */
> Do you mean "without"?

IMO, it would be clearer to say /* BSS needs zeroing when booting
without EFI. */

"should" in this case can validly be interpreted as both "we should do
it", and "it should have be done for us".

As for the patch itself, LGTM

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel