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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 13 Oct 2021, Jan Beulich wrote:
> 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>

Thanks! Patch committed.