> On 29 Sep 2021, at 08:50, Jan Beulich <jbeul...@suse.com> 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
>
