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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

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).

Jan