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

[Jan Beulich <jbeulich@suse.com>]

A number of nits, sorry:

On 15.09.2021 16:26, Luca Fancellu wrote:
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -8,9 +8,39 @@
>  #include <asm/setup.h>
>  #include <asm/smp.h>
>  
> +typedef struct {
> +    char* name;

Misplaced *.

> +    int name_len;

Surely this can't go negative? (Same issue elsewhere.)

> +} dom0less_module_name;
> +
> +/*
> + * Binaries will be translated into bootmodules, the maximum number for them is
> + * MAX_MODULES where we should remove a unit for Xen and one for Xen DTB
> + */
> +#define MAX_DOM0LESS_MODULES (MAX_MODULES - 2)
> +static struct file __initdata dom0less_files[MAX_DOM0LESS_MODULES];
> +static dom0less_module_name __initdata dom0less_bin_names[MAX_DOM0LESS_MODULES];
> +static uint32_t __initdata dom0less_modules_available = MAX_DOM0LESS_MODULES;
> +static uint32_t __initdata dom0less_modules_idx = 0;

Please see ./CODING_STYLE for your (ab)use of uint32_t here and
elsewhere.

> +#define ERROR_DOM0LESS_FILE_NOT_FOUND -1

Macros expanding to more than a single token should be suitably
parenthesized at least when the expression can possibly be mistaken
precedence wise (i.e. array[n] is in principle fine without
parentheses, because the meaning won't change no matter how it's
used in an expression).

>  void noreturn efi_xen_start(void *fdt_ptr, uint32_t fdt_size);
>  void __flush_dcache_area(const void *vaddr, unsigned long size);
>  
> +static int __init get_dom0less_file_index(const char* name, int name_len);
> +static uint32_t __init allocate_dom0less_file(EFI_FILE_HANDLE dir_handle,
> +                                              const char* name, int name_len);
> +static void __init handle_dom0less_module_node(EFI_FILE_HANDLE dir_handle,
> +                                               int module_node_offset,
> +                                               int reg_addr_cells,
> +                                               int reg_size_cells);
> +static void __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
> +                                               int domain_node,
> +                                               int addr_cells,
> +                                               int size_cells);
> +static bool __init check_dom0less_efi_boot(EFI_FILE_HANDLE dir_handle);

There are attributes (e.g. __must_check) which belong on the
declarations. __init, however, belongs on the definitions.

> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1134,8 +1134,9 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>      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;
> +    bool dom0less_found = false;
>  
>      __set_bit(EFI_BOOT, &efi_flags);
>      __set_bit(EFI_LOADER, &efi_flags);
> @@ -1285,14 +1286,21 @@ 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);
> +#ifdef CONFIG_ARM
> +        /* dom0less feature is supported only on ARM */
> +        dom0less_found = check_dom0less_efi_boot(dir_handle);
> +#endif
> +
> +        if ( !name.s && !dom0less_found )

Here you (properly ) use !name.s,

> +            blexit(L"No Dom0 kernel image specified.");
> +
> +        if ( name.s != NULL )

Here you then mean to omit the "!= NULL" for consistency and brevity.

> +            option_str = split_string(name.s);
>  
> -        if ( !read_section(loaded_image, L"kernel", &kernel, option_str) )
> +        if ( (!read_section(loaded_image, L"kernel", &kernel, option_str)) &&

Stray parentheses.

> +             (name.s != NULL) )

See above.

I also don't think this logic is quite right: If you're dom0less,
why would you want to look for an embedded Dom0 kernel image?

Jan