Re: [PATCH 1/3] x86/boot: Initialise BSS as soon as possible

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.09.2024 09:44, Frediano Ziglio wrote:
> On Sun, Sep 15, 2024 at 7:20 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 10.09.2024 18:16, Frediano Ziglio wrote:
>>> --- a/xen/arch/x86/boot/head.S
>>> +++ b/xen/arch/x86/boot/head.S
>>> @@ -231,6 +231,27 @@ __efi64_mb2_start:
>>>          /* VGA is not available on EFI platforms. */
>>>          movl   $0,vga_text_buffer(%rip)
>>>
>>> +        /*
>>> +         * Align the stack as UEFI spec requires. Keep it aligned
>>> +         * before efi_multiboot2() call by pushing/popping even
>>> +         * numbers of items on it.
>>> +         */
>>> +        and     $~15,%rsp
>>
>> You don't use the stack below, so it's not clear if/why this needs
>> moving. If it does, please add the missing blank after the comma
>> (like you nicely do everywhere else).
> 
> Fixed the blank. The reason is more clear if you look at the last
> commit in the series, at least for the EFI part. For the BIOS/PVH part
> is less clear, but the rationale is the same. The commit came from a
> larger series where BIOS/PVH is mainly written in C so there is more
> clear.

If there's need to do it _here_, that need wants spelling out in the
description.

>> Apart from this there's the question on the precise placement of
>> the calls - see respective comment on patch 2 (which I needed to
>> look at first to have an opinion here).
> 
> I think you removed too much context, not clear what code you are
> referring to, last hunk above is an "and" instruction.

This comment was on the patch as a whole, and I thought it was clear
that it would refer to the two calls to the new function. The details
of my concern there are, as said, in comments on patch 2.

Jan