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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

> @@ -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;

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.

> +    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"?

> +    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"?

Jan