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

[Luca Fancellu <luca.fancellu@xxxxxxx>]

> 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? Can I retain your ack to this patch doing these changes?

Cheers,
Luca

> 
> Jan
>