Re: [PATCH 3/3] x86/boot: Rewrite EFI start part in C

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.09.2024 10:25, Frediano Ziglio wrote:
> On Sun, Sep 15, 2024 at 8:00 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 10.09.2024 18:16, Frediano Ziglio wrote:
>>> -.Lefi_multiboot2_proto:
>>> -        /* Zero EFI SystemTable, EFI ImageHandle addresses and cmdline. */
>>> -        xor     %esi,%esi
>>> -        xor     %edi,%edi
>>> -        xor     %edx,%edx
>>> +        /* Save Multiboot2 pointer on the stack. */
>>> +        push    %rbx
>>
>> %rbx doesn't need preserving around a C function call (which will do
>> so itself if necessary). I understand a 2nd PUSH may be necessary
>> anyway, to keep the stack aligned, yet that then would need
>> commenting differently. Plus as long as we call our own functions
>> only, we don't require such alignment.
> 
> Extended comment.
> 16-byte alignment is also in SystemV ABI, I won't remove it in this series.

Except that we build with -mpreferred-stack-boundary=3, not respecting
the ABI in this regard anyway.

>>> +    {
>>> +        if ( tag->type == MULTIBOOT2_TAG_TYPE_EFI_BS )
>>> +            have_bs = true;
>>> +        else if ( tag->type == MULTIBOOT2_TAG_TYPE_EFI64 )
>>> +            SystemTable = _p(((const multiboot2_tag_efi64_t 
>>> *)tag)->pointer);
>>> +        else if ( tag->type == MULTIBOOT2_TAG_TYPE_EFI64_IH )
>>> +            ImageHandle = _p(((const multiboot2_tag_efi64_ih_t 
>>> *)tag)->pointer);
>>> +        else if ( tag->type == MULTIBOOT2_TAG_TYPE_CMDLINE )
>>> +            cmdline = ((const multiboot2_tag_string_t *)tag)->string;
>>> +    }
>>> +
>>> +    if ( !have_bs )
>>> +        return "ERR: Bootloader shutdown EFI x64 boot services!";
>>> +    if ( !SystemTable )
>>> +        return "ERR: EFI SystemTable is not provided by bootloader!";
>>> +    if ( !ImageHandle )
>>> +        return "ERR: EFI ImageHandle is not provided by bootloader!";
>>> +
>>> +    efi_multiboot2(ImageHandle, SystemTable, cmdline);
>>
>> This being invoked from here now makes me wonder about the (new)
>> function's name and whether a separate new function is actually
>> needed: Can't the new code then be integrated right into
>> efi_multiboot2(), thus eliminating the question on the naming of
>> the function?
> 
> If you are suggesting putting this parsing code inside efi_multiboot2
> in ef-boot.h that would change the behavior, which I would do in a
> different commit.
> Currently, there are 2 different efi_multiboot2 functions, one if
> ms_abi is supported, the other an empty stubs. However, some checks
> and tests are done in both cases (ms_abi supported or not). Also, both
> paths uses SystemTable, so I need to parse MBI2 in any case.

It could be slightly less parsing, but I get your point.

Then, as indicated, the function's name needs to change. The present name
simply fails to account for the important-ish fact that efi_multiboot2()
is (tail-)called.

>>> --- a/xen/arch/x86/efi/stub.c
>>> +++ b/xen/arch/x86/efi/stub.c
>>> @@ -17,7 +17,8 @@
>>>   */
>>>
>>>  void __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle,
>>> -                                    EFI_SYSTEM_TABLE *SystemTable)
>>> +                                    EFI_SYSTEM_TABLE *SystemTable,
>>> +                                    const char *cmdline)
>>>  {
>>>      static const CHAR16 __initconst err[] =
>>>          L"Xen does not have EFI code build in!\r\nSystem halted!\r\n";
>>
>> This, if not a separate change, wants mentioning in the description.
>> It's a related observation that this wasn't properly updated, but
>> nothing that necessarily needs doing here. Question is whether the
>> declaration of the function wouldn't better go into a header now in
>> the first place.
> 
> I had the same though about a header. But currently there's no such
> header, I mean it should be able to be included by both stub.c and
> efi-boot.h which are both x86 only code so it should go in
> xen/arch/x86/ I suppose. Suggestions? Maybe a different solution would
> be to have a xen/arch/x86/efi/efi-boot-stub.h instead of
> xen/arch/x86/efi/stub.c ?

It's not quite the right place, but maybe (ab)using asm/efibind.h would
be slightly better than introducing asm/efi.h just for a single decl?

Jan