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
