[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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
       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.


Julien Grall



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.