On 16.09.2024 10:25, Frediano Ziglio wrote:
> On Sun, Sep 15, 2024 at 8:00 AM Jan Beulich <jbeul...@suse.com> 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
