| Message ID | 20211011181528.17367-3-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm/efi: Add dom0less support to UEFI boot | expand |
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@arm.com> 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" 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;
> On 11 Oct 2021, at 20:53, Stefano Stabellini <sstabellini@kernel.org> 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@arm.com> > 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} > > 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? 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. Cheers, Luca
On Mon, 11 Oct 2021, Luca Fancellu wrote: > > On 11 Oct 2021, at 20:53, Stefano Stabellini <sstabellini@kernel.org> 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@arm.com> > > > > 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;
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@kernel.org> 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@arm.com> > > > > > > > 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"
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 :). Cheers,
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. 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.
> On 12 Oct 2021, at 02:31, Stefano Stabellini <sstabellini@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? Cheers, Luca
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,
On Tue, 12 Oct 2021, Luca Fancellu wrote: > > On 12 Oct 2021, at 02:31, Stefano Stabellini <sstabellini@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 <sstabellini@kernel.org>
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@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 <sstabellini@kernel.org> Then applicable parts Acked-by: Jan Beulich <jbeulich@suse.com> Jan
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@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 <sstabellini@kernel.org> > > Then applicable parts > Acked-by: Jan Beulich <jbeulich@suse.com> Thanks! Patch committed.
diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt index 7258e7e1ec..c6a775f4e8 100644 --- a/docs/misc/arm/device-tree/booting.txt +++ b/docs/misc/arm/device-tree/booting.txt @@ -70,6 +70,14 @@ Each node contains the following properties: priority of this field vs. other mechanisms of specifying the bootargs for the kernel. +- xen,uefi-binary (UEFI boot only) + + String property that specifies the file name to be loaded by the UEFI + boot for this module. If this is specified, there is no need to specify + the reg property because it will be created by the UEFI stub on boot. + This option is needed only when UEFI boot is used, the node needs to be + compatible with multiboot,kernel or multiboot,ramdisk. + Examples ======== diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc index 876cd55005..4abbb5bb82 100644 --- a/docs/misc/efi.pandoc +++ b/docs/misc/efi.pandoc @@ -167,6 +167,28 @@ sbsign \ --output xen.signed.efi \ xen.unified.efi ``` +## UEFI boot and Dom0 modules on ARM + +When booting using UEFI on ARM, it is possible to specify the Dom0 modules +directly from the device tree without using the Xen configuration file, here an +example: + +chosen { + #size-cells = <0x1>; + #address-cells = <0x1>; + xen,xen-bootargs = "[Xen boot arguments]" + + module@1 { + compatible = "multiboot,kernel", "multiboot,module"; + xen,uefi-binary = "vmlinuz-3.0.31-0.4-xen"; + bootargs = "[domain 0 command line options]"; + }; + + module@2 { + compatible = "multiboot,ramdisk", "multiboot,module"; + xen,uefi-binary = "initrd-3.0.31-0.4-xen"; + }; +} ## UEFI boot and dom0less on ARM @@ -326,10 +348,10 @@ chosen { ### Boot Xen, Dom0 and DomU(s) This configuration is a mix of the two configuration above, to boot this one -the configuration file must be processed so the /chosen node must have the -"xen,uefi-cfg-load" property. +the configuration file can be processed or the Dom0 modules can be read from +the device tree. -Here an example: +Here the first example: Xen configuration file: @@ -369,4 +391,40 @@ chosen { }; ``` +Here the second example: + +Device tree: + +``` +chosen { + #size-cells = <0x1>; + #address-cells = <0x1>; + xen,xen-bootargs = "[Xen boot arguments]" + + module@1 { + compatible = "multiboot,kernel", "multiboot,module"; + xen,uefi-binary = "vmlinuz-3.0.31-0.4-xen"; + bootargs = "[domain 0 command line options]"; + }; + + module@2 { + compatible = "multiboot,ramdisk", "multiboot,module"; + xen,uefi-binary = "initrd-3.0.31-0.4-xen"; + }; + + domU1 { + #size-cells = <0x1>; + #address-cells = <0x1>; + compatible = "xen,domain"; + cpus = <0x1>; + memory = <0x0 0xc0000>; + vpl011; + module@1 { + compatible = "multiboot,kernel", "multiboot,module"; + xen,uefi-binary = "Image-domu1.bin"; + bootargs = "console=ttyAMA0 root=/dev/ram0 rw"; + }; + }; +}; +``` diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h index f35e035b22..840728d6c0 100644 --- a/xen/arch/arm/efi/efi-boot.h +++ b/xen/arch/arm/efi/efi-boot.h @@ -32,8 +32,12 @@ static unsigned int __initdata modules_idx; #define ERROR_RENAME_MODULE_NAME (-4) #define ERROR_SET_REG_PROPERTY (-5) #define ERROR_CHECK_MODULE_COMPAT (-6) +#define ERROR_DOM0_ALREADY_FOUND (-7) +#define ERROR_DOM0_RAMDISK_FOUND (-8) +#define ERROR_XSM_ALREADY_FOUND (-9) #define ERROR_DT_MODULE_DOMU (-1) #define ERROR_DT_CHOSEN_NODE (-2) +#define ERROR_DT_MODULE_DOM0 (-3) void noreturn efi_xen_start(void *fdt_ptr, uint32_t fdt_size); void __flush_dcache_area(const void *vaddr, unsigned long size); @@ -46,7 +50,8 @@ static int allocate_module_file(EFI_FILE_HANDLE dir_handle, static int handle_module_node(EFI_FILE_HANDLE dir_handle, int module_node_offset, int reg_addr_cells, - int reg_size_cells); + int reg_size_cells, + bool is_domu_module); static int handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle, int domain_node); static int efi_check_dt_boot(EFI_FILE_HANDLE dir_handle); @@ -701,7 +706,8 @@ static int __init allocate_module_file(EFI_FILE_HANDLE dir_handle, static int __init handle_module_node(EFI_FILE_HANDLE dir_handle, int module_node_offset, int reg_addr_cells, - int reg_size_cells) + int reg_size_cells, + bool is_domu_module) { const void *uefi_name_prop; char mod_string[24]; /* Placeholder for module@ + a 64-bit number + \0 */ @@ -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; } /* Free boot modules file names if any */ diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 7879b93f93..531975326f 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -1302,11 +1302,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) { read_file(dir_handle, s2w(&name), &kernel, option_str); efi_bs->FreePool(name.w); - - if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL, - (void **)&shim_lock)) && - (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS ) - PrintErrMesg(L"Dom0 kernel image could not be verified", status); } if ( !read_section(loaded_image, L"ramdisk", &ramdisk, NULL) ) @@ -1384,6 +1379,17 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) if ( !dt_modules_found && !kernel.ptr ) blexit(L"No initial domain kernel specified."); + /* + * The Dom0 kernel can be loaded from the configuration file or by the + * device tree through the efi_check_dt_boot function, in this stage + * verify it. + */ + if ( kernel.ptr && + !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL, + (void **)&shim_lock)) && + (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS ) + PrintErrMesg(L"Dom0 kernel image could not be verified", status); + efi_arch_edd(); /* XXX Collect EDID info. */
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@arm.com> --- Changes in v6: - given the changes to is_boot_module() in previous patch, a check to avoid declaration of xsm in DT and cfg file is introduced and the call to is_boot_module is removed. Changes in v5: - renamed missing uefi,binary string - used kernel.ptr instead of kernel.addr to be consistent to the surrounding code - Changed a comment referring to efi_arch_check_dt_boot that now is efi_check_dt_boot Changes in v4: - Add check to avoid double definition of Dom0 ramdisk from cfg file and DT - Fix if conditions indentation in boot.c - Moved Dom0 kernel verification code after check for presence for Dom0 or DomU(s) - Changed uefi,binary property to xen,uefi-binary Changes in v3: - new patch --- docs/misc/arm/device-tree/booting.txt | 8 ++++ docs/misc/efi.pandoc | 64 +++++++++++++++++++++++++-- xen/arch/arm/efi/efi-boot.h | 52 ++++++++++++++++++++-- xen/common/efi/boot.c | 16 ++++--- 4 files changed, 128 insertions(+), 12 deletions(-)