Re: [PATCH v6 2/2] arm/efi: load dom0 modules from DT using UEFI

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.10.2021 02:49, Stefano Stabellini wrote:
> On Tue, 12 Oct 2021, Luca Fancellu wrote:
>>> On 12 Oct 2021, at 02:31, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
>>>
>>> On Mon, 11 Oct 2021, Julien Grall wrote:
>>>> Hi Stefano,
>>>>
>>>> On 11/10/2021 22:24, Stefano Stabellini wrote:
>>>>>> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
>>>>>> index 840728d6c0..076b827bdd 100644
>>>>>> --- a/xen/arch/arm/efi/efi-boot.h
>>>>>> +++ b/xen/arch/arm/efi/efi-boot.h
>>>>>> @@ -713,10 +713,12 @@ static int __init 
>>>>>> handle_module_node(EFI_FILE_HANDLE
>>>>>> dir_handle,
>>>>>>      char mod_string[24]; /* Placeholder for module@ + a 64-bit number +
>>>>>> \0 */
>>>>>>      int uefi_name_len, file_idx, module_compat;
>>>>>>      module_name *file;
>>>>>> +    const char *compat_string = is_domu_module ? "multiboot,module" :
>>>>>> +                                "xen,multiboot-module";
>>>>>>        /* Check if the node is a multiboot,module otherwise return */
>>>>>>      module_compat = fdt_node_check_compatible(fdt, module_node_offset,
>>>>>> -                                              "multiboot,module");
>>>>>> +                                              compat_string);
>>>>>>      if ( module_compat < 0 )
>>>>>>          /* Error while checking the compatible string */
>>>>>>          return ERROR_CHECK_MODULE_COMPAT;
>>>>>
>>>>>
>>>>> Well... not exactly like this because this would stop a normal
>>>>> "multiboot,module" dom0 kernel from being recognized.
>>>>>
>>>>> So we need for domU: only "multiboot,module"
>>>>> For Dom0, either "multiboot,module" or "xen,multiboot-module"
>>>>
>>>> Looking at the history, xen,multiboot-module has been considered as a 
>>>> legacy
>>>> binding since before UEFI was introduced. In fact, without this series, I
>>>> believe, there is limited reasons for the compatible to be present in the 
>>>> DT
>>>> as you would either use grub (which use the new compatible) or xen.cfg (the
>>>> stub will create the node).
>>>>
>>>> So I would argue that this compatible should not be used in combination 
>>>> with
>>>> UEFI and therefore we should not handle it. This would make the code 
>>>> simpler
>>>> :).
>>>
>>
>> Hi Stefano,
>>
>>> What you suggested is a viable option, however ImageBuilder is still
>>> using the "xen,multiboot-module" format somehow today (no idea why) and
>>> we have the following written in docs/misc/arm/device-tree/booting.txt:
>>>
>>>     Xen 4.4 supported a different set of legacy compatible strings
>>>     which remain supported such that systems supporting both 4.4
>>>     and later can use a single DTB.
>>>
>>>     - "xen,multiboot-module" equivalent to "multiboot,module"
>>>     - "xen,linux-zimage"     equivalent to "multiboot,kernel"
>>>     - "xen,linux-initrd"     equivalent to "multiboot,ramdisk"
>>>
>>>     For compatibility with Xen 4.4 the more specific "xen,linux-*"
>>>     names are non-optional and must be included.
>>>
>>> My preference is to avoid breaking compatibility (even with UEFI
>>> booting). The way I suggested above is one way to do it.
>>>
>>> But I don't feel strongly about this at all, I am fine with ignoring
>>> "xen,multiboot-module" in the EFI stub. I can get ImageBuilder fixed
>>> very quickly (I should do that in any case). If we are going to ignore
>>> "xen,multiboot-module" then we probably want to update the text in
>>> docs/misc/arm/device-tree/booting.txt also.
>>
>> The changes to support legacy compatible strings can be done but it will 
>> result in
>> complex code, I would go for Julien suggestion to just drop it for UEFI.
>>
>> I can add a note on docs/misc/arm/device-tree/booting.txt saying that for 
>> UEFI boot
>> the legacy strings are not supported.
>>
>> Something like:
>>
>> --- a/docs/misc/arm/device-tree/booting.txt
>> +++ b/docs/misc/arm/device-tree/booting.txt
>> @@ -51,6 +51,8 @@ Each node contains the following properties:
>>         Xen 4.4 supported a different set of legacy compatible strings
>>         which remain supported such that systems supporting both 4.4
>>         and later can use a single DTB.
>> +       However when booting Xen using UEFI and Device Tree, the legacy 
>> compatible
>> +       strings are not supported.
>>  
>>         - "xen,multiboot-module" equivalent to "multiboot,module"
>>         - "xen,linux-zimage"     equivalent to "multiboot,kernel”
>>
>>
>> What do you think about that?
> 
> Also reading Julien's reply, I am fine with a doc-only change in a
> separate patch.
> 
> Yes, something along those lines is OK.
> 
> So for this patch:
> 
> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>

Then applicable parts
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan