Re: [PATCH v4 3/3] arm/efi: load dom0 modules from DT using UEFI

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.10.2021 16:08, Luca Fancellu wrote:
> 
> 
>> On 1 Oct 2021, at 12:16, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 30.09.2021 16:28, Luca Fancellu wrote:
>>> Add support to load Dom0 boot modules from
>>> the device tree using the xen,uefi-binary property.
>>>
>>> Update documentation about that.
>>>
>>> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
>>
>> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
>> despite ...
>>
>>> @@ -1385,6 +1380,17 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
>>> *SystemTable)
>>>     if ( !dt_modules_found && !kernel.addr )
>>>         blexit(L"No Dom0 kernel image specified.");
>>>
>>> +    /*
>>> +     * The Dom0 kernel can be loaded from the configuration file or by the
>>> +     * device tree through the efi_arch_check_dt_boot function, in this 
>>> stage
>>> +     * verify it.
>>> +     */
>>> +    if ( kernel.addr &&
>>
>> ... me still being a little unhappy with the inconsistent use of the
>> union fields so close together: This one is now consistent with the
>> one visible further up in context, but ...
>>
>>> +         !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,> +       
>>>                                     (void **)&shim_lock)) &&
>>> +         (status = shim_lock->Verify(kernel.ptr, kernel.size)) != 
>>> EFI_SUCCESS )
>>
>> ... is now inconsistent with this use. But yeah - read_file() is
>> even worse in that sense, except that there the different uses are
>> for specific reasons, while here the only requirement is to satisfy
>> shim_lock->Verify().
>>
>> Please feel free to retain my ack in case you decide to use .ptr in
>> all three places.
> 
> Hi Jan,
> 
> Sure I will do the modification you suggested, I will fix also my silly 
> mistake that
> Stefano pointed out.
> 
> Just to be sure, I explain what I will do:
> 
> In the second patch I will change:
> 
>     if ( !dt_modules_found && !kernel.addr )
> 
> To 
> 
>     if ( !dt_modules_found && !kernel.ptr )
> 
> 
> And in this patch I will use:
> 
> if ( kernel.ptr &&
>          !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
>                                            (void **)&shim_lock)) &&
>          (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS 
> )
>         PrintErrMesg(L"Dom0 kernel image could not be verified", status);
> 
> Do you agree on them?

Yes and ...

> Can I retain your ack to this patch doing these changes?

... as previously said, yes.

Jan