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

[Luca Fancellu <luca.fancellu@arm.com>]

> On 24 Sep 2021, at 15:02, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 22.09.2021 16:13, Luca Fancellu wrote:
>> +static unsigned int __init allocate_dom0less_file(EFI_FILE_HANDLE dir_handle,
>> +                                                  const char *name,
>> +                                                  unsigned int name_len)
>> +{
>> +    dom0less_module_name* file_name;
>> +    union string module_name;
>> +    unsigned int ret_idx;
>> +
>> +    /*
>> +     * Check if there is any space left for a domU module, the variable
>> +     * dom0less_modules_available is updated each time we use read_file(...)
>> +     * successfully.
>> +     */
>> +    if ( !dom0less_modules_available )
>> +        blexit(L"No space left for domU modules");
>> +
>> +    module_name.s = (char*) name;
> 
> Unfortunately there are too many style issues in these Arm additions to
> really enumerate; I'd like to ask that you go through yourself with
> ./CODING_STYLE, surrounding code, and review comments on earlier patches
> of yours in mind. This cast stands out, though: I'm pretty sure you were
> told before that casts are often dangerous and hence should be avoided
> whenever (easily) possible. There was a prior case where union string
> was used in a similar way, not all that long ago. Hence why it now has
> a "const char *" member. (That's still somewhat risky, but imo way
> better than a cast.)

Hi Jan,

Yes I will use the .cs member, I will have also a better look on the patch to
find the style issues.

> 
>> @@ -1361,12 +1360,21 @@ 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);
>>     }
>> 
>> +    /*
>> +     * Check if a proper configuration is provided to start Xen:
>> +     *  - Dom0 specified (minimum required)
>> +     *  - Dom0 and DomU(s) specified
>> +     *  - DomU(s) specified
>> +     */
>> +    if ( !efi_arch_check_dom0less_boot(dir_handle) && !kernel.addr )
>> +        blexit(L"No Dom0 kernel image specified.");
>> +
>> +    dir_handle->Close(dir_handle);
> 
> So far I was under the impression that handles and alike need closing
> before calling Exit(), to prevent resource leaks. While I will admit
> that likely there are more (pre-existing) affected paths, I think that
> - if that understanding of mine is correct - it would be nice to avoid
> adding yet more instances.

Ok sure, I will close the handle before the blexit.

Cheers,
Luca

> 
> Jan
>