Re: [PATCH v3 2/3] arm/efi: Use dom0less configuration when using EFI boot

[Luca Fancellu <luca.fancellu@xxxxxxx>]

> On 29 Sep 2021, at 08:50, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 28.09.2021 18:32, Luca Fancellu wrote:
>> --- a/xen/arch/x86/efi/efi-boot.h
>> +++ b/xen/arch/x86/efi/efi-boot.h
>> @@ -678,6 +678,12 @@ static void __init efi_arch_handle_module(const struct 
>> file *file,
>>     efi_bs->FreePool(ptr);
>> }
>> 
>> +static int __init efi_arch_check_dt_boot(EFI_FILE_HANDLE dir_handle)
>> +{
>> +    /* x86 doesn't support device tree boot */
>> +    return 0;
>> +}
> 
> Every time I see this addition I'm getting puzzled. As a result I'm
> afraid I now need to finally ask you to do something about this (and
> I'm sorry for doing so only now). There would better be no notion of
> DT in x86 code, and there would better also not be a need for
> architectures not supporting DT to each supply such a stub. Instead
> I think you want to put this stub in xen/common/efi/boot.c, inside a
> suitable #ifdef.

Sure I will enclose it in #ifdef CONFIG_ARM and remove the x86 stub.

> 
>> --- a/xen/common/efi/boot.c
>> +++ b/xen/common/efi/boot.c
>> @@ -1127,15 +1127,17 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
>> *SystemTable)
>>     static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID;
>>     EFI_LOADED_IMAGE *loaded_image;
>>     EFI_STATUS status;
>> -    unsigned int i, argc;
>> -    CHAR16 **argv, *file_name, *cfg_file_name = NULL, *options = NULL;
>> +    unsigned int i, argc = 0;
>> +    CHAR16 **argv, *file_name = NULL, *cfg_file_name = NULL, *options = 
>> NULL;
>>     UINTN gop_mode = ~0;
>>     EFI_SHIM_LOCK_PROTOCOL *shim_lock;
>>     EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
>>     union string section = { NULL }, name;
>>     bool base_video = false;
>> -    const char *option_str;
>> +    const char *option_str = NULL;
>>     bool use_cfg_file;
>> +    int dt_module_found;
> 
> I think this variable either wants to be bool or be named differently.
> 
>> @@ -1361,12 +1361,26 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
>> *SystemTable)
>>         efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
>>         cfg.addr = 0;
>> 
>> -        dir_handle->Close(dir_handle);
>> -
>>         if ( gop && !base_video )
>>             gop_mode = efi_find_gop_mode(gop, cols, rows, depth);
>>     }
>> 
>> +    dt_module_found = efi_arch_check_dt_boot(dir_handle);
>> +
>> +    dir_handle->Close(dir_handle);
>> +
>> +    if (dt_module_found < 0)
>> +        /* efi_arch_check_dt_boot throws some error */
>> +        blexit(L"Error processing boot modules on DT.");
> 
> For this use, bool would seem appropriate, but ...
> 
>> +    /*
>> +     * Check if a proper configuration is provided to start Xen:
>> +     *  - Dom0 specified (minimum required)
>> +     *  - Dom0 and DomU(s) specified
>> +     *  - DomU(s) specified
>> +     */
>> +    if ( !dt_module_found && !kernel.addr )
>> +        blexit(L"No Dom0 kernel image specified.");
> 
> ... this (and my brief looking at the Arm code) rather suggests a
> count gets returned, and hence it may want renaming instead. Maybe
> simply to dt_modules_found.

Yes that’s a better name, I will also add a comment just above the
efi_arch_check_dt_boot to explain it is returning the number of modules
found in the DT or an error (<0)

> 
> Considering the new conditional I also wonder whether the error
> message can't end up being misleading on Arm (it certainly should
> remain as is on x86).

Do you think that a message like this: “No guest kernel image specified”
can work for both x86 and arm architecture?

> 
> Jan
>