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

[Jan Beulich <jbeulich@suse.com>]

On 16.09.2021 13:28, Luca Fancellu wrote:
>> On 16 Sep 2021, at 09:46, Jan Beulich <jbeulich@suse.com> wrote:
>> 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.

I don't think it's reasonable to spell out there every little detail.
You should also take adjacent code into consideration, making yours
match. Issues only arise when there's bad code that you happen to
look at.

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

Correct, which is why this is fine.

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

As you can see from neighboring code, we prefer the shorter forms,
for being easier/shorter to read.

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

It is you who is adding the name.s != NULL part, isn't it?

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

This doesn't match what I would expect - if configuration tells
to boot dom0less, why would an embedded Dom0 kernel matter? I can
see that views might differ here; you will want to write down
somewhere what the intended behavior in such a case is.

Jan