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

[Luca Fancellu <luca.fancellu@xxxxxxx>]

> On 29 Sep 2021, at 09:00, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 28.09.2021 18:32, Luca Fancellu wrote:
>> --- a/xen/common/efi/boot.c
>> +++ b/xen/common/efi/boot.c
>> @@ -1296,11 +1296,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
>> *SystemTable)
>>         {
>>             read_file(dir_handle, s2w(&name), &kernel, option_str);
>>             efi_bs->FreePool(name.w);
>> -
>> -            if ( !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);
>>         }
>> 
>>         if ( !read_section(loaded_image, L"ramdisk", &ramdisk, NULL) )
>> @@ -1372,6 +1367,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
>> *SystemTable)
>>     if (dt_module_found < 0)
>>         /* efi_arch_check_dt_boot throws some error */
>>         blexit(L"Error processing boot modules on DT.");
>> +
>> +    /* If Dom0 is specified, verify it */
>> +    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);
> 
> Why does this code need moving in the first place? That's (to me at least)
> not obvious from looking just at the common (i.e. non-Arm-specific) part.
> Hence I would wish for the comment to be slightly extended to indicate
> that the kernel image may also have been loaded by <whichever function>.
> 

Sure I will improve the comment to explain that.

> Also, two nits: You've broken indentation here (you've lost a space too
> much compared to the original code) and ...

Yes sorry for that, we didn’t see it, I will fix it.

> 
>>     /*
>>      * Check if a proper configuration is provided to start Xen:
>>      *  - Dom0 specified (minimum required)
>> 
> 
> ... you will want to insert a blank line not only before, but also after
> your insertion (or, imo less desirable, neither of the two blank lines).
> 
> Further I wonder whether your addition wouldn't better be after the code
> following this comment.
> 
> And finally I wonder (looking back at the earlier patch) why you use
> kernel.addr there when kernel.ptr is being used here. Unless there's a
> reason for the difference, being consistent would be better.

I will do all of the above for the v4.

Thanks for all the feedbacks.

Cheers,
Luca

> 
> Jan
>