|
[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
On Mon, 11 Oct 2021, Stefano Stabellini wrote:
> On Mon, 11 Oct 2021, Luca Fancellu wrote:
> > > On 11 Oct 2021, at 20:53, Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > wrote:
> > >
> > > On Mon, 11 Oct 2021, Luca Fancellu wrote:
> > >> Add support to load Dom0 boot modules from
> > >> the device tree using the xen,uefi-binary property.
> > >>
> > >> Update documentation about that.
> > >>
> > >> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> > >
> >
> > Hi Stefano,
> >
> > > Unfortunately, due to the change to the previous patch, this patch now
> > > has one issue, see below.
> > >
> > >
> > >> @@ -754,6 +760,41 @@ static int __init
> > >> handle_module_node(EFI_FILE_HANDLE dir_handle,
> > >> return ERROR_SET_REG_PROPERTY;
> > >> }
> > >>
> > >> + if ( !is_domu_module )
> > >> + {
> > >> + if ( (fdt_node_check_compatible(fdt, module_node_offset,
> > >> + "multiboot,kernel") == 0) )
> > >> + {
> > >> + /*
> > >> + * This is the Dom0 kernel, wire it to the kernel variable
> > >> because it
> > >> + * will be verified by the shim lock protocol later in the
> > >> common
> > >> + * code.
> > >> + */
> > >> + if ( kernel.addr )
> > >> + {
> > >> + PrintMessage(L"Dom0 kernel already found in cfg file.");
> > >> + return ERROR_DOM0_ALREADY_FOUND;
> > >> + }
> > >> + kernel.need_to_free = false; /* Freed using the module
> > >> array */
> > >> + kernel.addr = file->addr;
> > >> + kernel.size = file->size;
> > >> + }
> > >> + else if ( ramdisk.addr &&
> > >> + (fdt_node_check_compatible(fdt, module_node_offset,
> > >> + "multiboot,ramdisk") == 0)
> > >> )
> > >> + {
> > >> + PrintMessage(L"Dom0 ramdisk already found in cfg file.");
> > >> + return ERROR_DOM0_RAMDISK_FOUND;
> > >> + }
> > >> + else if ( xsm.addr &&
> > >> + (fdt_node_check_compatible(fdt, module_node_offset,
> > >> + "xen,xsm-policy") == 0) )
> > >> + {
> > >> + PrintMessage(L"XSM policy already found in cfg file.");
> > >> + return ERROR_XSM_ALREADY_FOUND;
> > >> + }
> > >> + }
> > >> +
> > >> return 0;
> > >> }
> > >>
> > >> @@ -793,7 +834,7 @@ static int __init
> > >> handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
> > >> module_node = fdt_next_subnode(fdt, module_node) )
> > >> {
> > >> int ret = handle_module_node(dir_handle, module_node, addr_cells,
> > >> - size_cells);
> > >> + size_cells, true);
> > >> if ( ret < 0 )
> > >> return ret;
> > >> }
> > >> @@ -803,7 +844,7 @@ static int __init
> > >> handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
> > >>
> > >> /*
> > >> * This function checks for xen domain nodes under the /chosen node for
> > >> possible
> > >> - * domU guests to be loaded.
> > >> + * dom0 and domU guests to be loaded.
> > >> * Returns the number of modules loaded or a negative number for error.
> > >> */
> > >> static int __init efi_check_dt_boot(EFI_FILE_HANDLE dir_handle)
> > >> @@ -830,6 +871,9 @@ static int __init efi_check_dt_boot(EFI_FILE_HANDLE
> > >> dir_handle)
> > >> if ( handle_dom0less_domain_node(dir_handle, node) < 0 )
> > >> return ERROR_DT_MODULE_DOMU;
> > >> }
> > >> + else if ( handle_module_node(dir_handle, node, addr_len,
> > >> size_len,
> > >> + false) < 0 )
> > >> + return ERROR_DT_MODULE_DOM0;
> > >> }
> > >
> > > handle_module_node comes with a "multiboot,module" compatible check now,
> > > which is fine for dom0less DomU modules, but it is not OK for dom0
> > > modules.
> > >
> > > That is because it is also possible to write the dom0 modules (kernel
> > > and ramdisk) with the following compabile strings:
> > >
> > > /chosen/dom0 compatible "xen,linux-zimage" "xen,multiboot-module"
> > > /chosen/dom0-ramdisk compatible "xen,linux-initrd" "xen,multiboot-module"
> >
> > Oh ok I’m surprised because I think even before I was not managing any
> > module
> > with “xen,multiboot-module”, just any multiboot,{kernel,ramdisk,device-tree}
>
> At least by looking at the code it seemed to work before, although we
> weren't explicitly checking for this case
>
>
> > > They are legacy but we are not meant to break support for them. Also
> > > third party tools might still use them -- I checked and even
> > > ImageBuilder still uses them.
> > >
> > > One way to solve the problem is to make the "multiboot,module"
> > > compatible check at the beginning of handle_module_node conditional on
> > > !is_domu_module.
> > >
> > > Or maybe just ignore compabible errors if !is_domu_module. Something like:
> > >
> > > diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> > > index 840728d6c0..cbfcd55449 100644
> > > --- a/xen/arch/arm/efi/efi-boot.h
> > > +++ b/xen/arch/arm/efi/efi-boot.h
> > > @@ -721,7 +721,7 @@ static int __init handle_module_node(EFI_FILE_HANDLE
> > > dir_handle,
> > > /* Error while checking the compatible string */
> > > return ERROR_CHECK_MODULE_COMPAT;
> > >
> > > - if ( module_compat != 0 )
> > > + if ( is_domu_module && module_compat != 0 )
> > > /* Module is not a multiboot,module */
> > > return 0;
> > >
> >
> > I can be ok with this change but it means that any node under chosen that
> > is not a “xen,domain”
> > will be handled as it is a Dom0 boot module (if it has xen,uefi-binary), is
> > it always true?
>
> Good point. I don't think it is a safe assumption.
>
>
> > Otherwise I can do these changes:
> >
> > --- a/xen/arch/arm/efi/efi-boot.h
> > +++ b/xen/arch/arm/efi/efi-boot.h
> > @@ -721,10 +721,19 @@ static int __init handle_module_node(EFI_FILE_HANDLE
> > dir_handle,
> > /* Error while checking the compatible string */
> > return ERROR_CHECK_MODULE_COMPAT;
> >
> > - if ( module_compat != 0 )
> > + if ( is_domu_module && (module_compat != 0) )
> > /* Module is not a multiboot,module */
> > return 0;
> >
> > + /*
> > + * For Dom0 boot modules can be specified also using the legacy
> > compatible
> > + * xen,multiboot-module
> > + */
> > + if ( !is_domu_module && module_compat &&
> > + (fdt_node_check_compatible(fdt, module_node_offset,
> > + "xen,multiboot-module") != 0) )
> > + return 0;
> > +
> > /* Read xen,uefi-binary property to get the file name. */
> > uefi_name_prop = fdt_getprop(fdt, module_node_offset,
> > "xen,uefi-binary",
> > &uefi_name_len);
> > @@ -763,7 +772,9 @@ static int __init handle_module_node(EFI_FILE_HANDLE
> > dir_handle,
> > if ( !is_domu_module )
> > {
> > if ( (fdt_node_check_compatible(fdt, module_node_offset,
> > - "multiboot,kernel") == 0) )
> > + "multiboot,kernel") == 0) ||
> > + (fdt_node_check_compatible(fdt, module_node_offset,
> > + "xen,linux-zimage") == 0) )
> > {
> > /*
> > * This is the Dom0 kernel, wire it to the kernel variable
> > because it
> > @@ -780,8 +791,10 @@ static int __init handle_module_node(EFI_FILE_HANDLE
> > dir_handle,
> > kernel.size = file->size;
> > }
> > else if ( ramdisk.addr &&
> > - (fdt_node_check_compatible(fdt, module_node_offset,
> > - "multiboot,ramdisk") == 0) )
> > + ((fdt_node_check_compatible(fdt, module_node_offset,
> > + "multiboot,ramdisk") == 0) ||
> > + (fdt_node_check_compatible(fdt, module_node_offset,
> > + "xen,linux-initrd") == 0)) )
> > {
> > PrintMessage(L"Dom0 ramdisk already found in cfg file.");
> > return ERROR_DOM0_RAMDISK_FOUND;
> >
> >
> > I would need to check for “xen,linux-zimage” and “xen,linux-initrd” however
> > to be sure the user is not specifying the kernel and ramdisk twice.
> >
> > Please let me know if you agree or if there is some issue with them.
>
> I have another idea: I don't think we need to actually check for
> "xen,linux-zimage" or "xen,linux-initrd" because I am pretty sure they
> were always used in conjunction with "xen,multiboot-module".
>
> So maybe it is enough to check for:
>
> - for dom0: "xen,multiboot-module"
> - domU: "multiboot,module"
>
>
> E.g.:
>
> 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"
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |