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 <sstabell...@kernel.org> 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 <sstabell...@kernel.org>
Then applicable parts Acked-by: Jan Beulich <jbeul...@suse.com> Jan
