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

[Luca Fancellu <luca.fancellu@xxxxxxx>]

> On 16 Sep 2021, at 09:46, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> 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 *.

I was looking in the CODING_STYLE and I didn’t found anything that mandates
char *name; instead of char* name; but if you prefer I can change it since I 
have
to do some modification to the patch.

> 
>> +    int name_len;
> 
> Surely this can't go negative? (Same issue elsewhere.)

I will change that to unsigned int.

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

Ok, I will change them to unsigned int

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

I will fix it to be (-1)

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

Ok, I will remove __init from declarations.

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

This is not my code, I just added && !dom0less

> 
>> +            blexit(L"No Dom0 kernel image specified.");
>> +
>> +        if ( name.s != NULL )
> 
> Here you then mean to omit the "!= NULL" for consistency and brevity.

I usually check explicitely pointers with NULL, is it something to be avoided 
in Xen?
There are some industrial coding standards that says to avoid the use of ! 
operator
with pointers. Is it important here to do !name.s instead of the solution above?

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

Will fix

> 
>> +             (name.s != NULL) )
> 
> See above.

Will fix

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

This is common code, that check is not from my patch.

Before this serie, EFI boot requires that a dom0 module was passed, otherwise
the boot was stopped.

This serie instead removes this requirement, letting the boot continue if there 
is no dom0
kernel.

However (as in the old code) if the user embed the dom0 kernel in the image, 
then it is
legit to use it and if there are also other domUs specified by DT, then the 
system will
start the dom0 kernel and the domUs kernel as well.

Cheers,
Luca 


> 
> Jan
>