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

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 12/10/2021 02:31, Stefano Stabellini 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
> > :).
> 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.
I understand that from the documentation PoV we claim that the legacy 
bindings is supported in EFI. However, looking at the code, I don't 
think we ever supported them.
Indeed, the function efi_arch_use_config_file() has always only looked 
for nodes contains the compatible "multiboot,module". If there are none 
present, then the stub would require to have a xen.cfg present.
> But I don't feel strongly about this at all, I am fine with ignoring
> "xen,multiboot-module" in the EFI stub.
I think this has always been the case for the past 7 years (or so). This 
leads me to think that nobody ever used UEFI in combination of 
ImageBuilder and therefore I think...
> I can get ImageBuilder fixed
> very quickly (I should do that in any case).
... it is more suitable to fix ImageBuilder over trying to add 
retrospectively a legacy binding in the UEFI stub.
> 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.
I agree the docs probably wants to be updated. Although, I think this 
should happen in a separate patch because this doesn't look a new problem.
Cheers,

--
Julien Grall