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

[Luca Fancellu <luca.fancellu@xxxxxxx>]

> On 1 Oct 2021, at 12:02, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 30.09.2021 16:28, Luca Fancellu wrote:
>> --- 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;
> 
> Are these two changes really still needed?

Yes you are right, I will revert back them.

> 
>> @@ -1285,14 +1286,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
>> *SystemTable)
>>             efi_bs->FreePool(name.w);
>>         }
>> 
>> -        if ( !name.s )
>> -            blexit(L"No Dom0 kernel image specified.");
>> -
>>         efi_arch_cfg_file_early(loaded_image, dir_handle, section.s);
>> 
>> -        option_str = split_string(name.s);
>> +        if ( name.s )
>> +            option_str = split_string(name.s);
> 
>        option_str = name.s ? split_string(name.s) : NULL;

I will use your suggestion above so I don’t have to initialise it.

> 
> would be the less intrusive change (eliminating the need to add an
> initialized for option_str). Or if you really want to stick to your
> model, then please at the same time at least move option_str into
> the more narrow scope.
> 
>> @@ -1361,12 +1361,30 @@ 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);
>>     }
>> 
>> +#ifdef CONFIG_HAS_DEVICE_TREE
>> +    /* Get the number of boot modules specified on the DT or an error (<0) 
>> */
>> +    dt_modules_found = efi_arch_check_dt_boot(dir_handle);
>> +#endif
> 
> So I had asked to add a stub enclosed in such an #ifdef, to avoid the
> #ifdef here. I may be willing to let you keep things as you have them
> now, but I'd like to understand why you've picked that different
> approach despite the prior discussion.

There must be a misunderstanding, your message in the v3 was:

"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.”

So I thought you wanted me to remove the stub in x86 (since it doesn’t support 
DT)
and put this call under #ifdef so it won’t be compiled for arch not supporting 
DT.


> 
>> +    dir_handle->Close(dir_handle);
>> +
>> +    if ( dt_modules_found < 0 )
>> +        /* efi_arch_check_dt_boot throws some error */
>> +        blexit(L"Error processing boot modules on DT.");
>> +
>> +    /*
>> +     * Check if a proper configuration is provided to start Xen:
>> +     *  - Dom0 specified (minimum required)
>> +     *  - Dom0 and DomU(s) specified
>> +     *  - DomU(s) specified
>> +     */
> 
> May I suggest to shorten the three bullet points to "At least one
> of Dom0 or DomU(s) specified"?

Sure I will change to:

/* Check if at least one of Dom0 or DomU(s) is specified */

> 
>> +    if ( !dt_modules_found && !kernel.addr )
>> +        blexit(L"No Dom0 kernel image specified.");
> 
> And may I also ask to alter the text here, to be less confusing to
> dom0less folks? E.g. "No initial domain kernel specified"?

Yes I will change that.

Cheers,
Luca

> 
> Jan
>