Add support to load Dom0 boot modules from
the device tree using the uefi,binary property.
Update documentation about that.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
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 | 36 +++++++++++++--
xen/common/efi/boot.c | 12 ++---
4 files changed, 108 insertions(+), 12 deletions(-)
diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 354bb43fe1..e73f6476d4 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.
+- 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 800e67a233..4cebc47a18 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";
+ uefi,binary = "vmlinuz-3.0.31-0.4-xen";
+ bootargs = "[domain 0 command line options]";
+ };
+
+ module@2 {
+ compatible = "multiboot,ramdisk", "multiboot,module";
+ 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
-"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";
+ uefi,binary = "vmlinuz-3.0.31-0.4-xen";
+ bootargs = "[domain 0 command line options]";
+ };
+
+ module@2 {
+ compatible = "multiboot,ramdisk", "multiboot,module";
+ 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";
+ 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 4f7c913f86..df63387136 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -31,8 +31,10 @@ static unsigned int __initdata modules_idx;
#define ERROR_MISSING_DT_PROPERTY (-3)
#define ERROR_RENAME_MODULE_NAME (-4)
#define ERROR_SET_REG_PROPERTY (-5)
+#define ERROR_DOM0_ALREADY_FOUND (-6)
#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);
@@ -45,7 +47,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 bool is_boot_module(int dt_module_offset);
static int handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
int domain_node);
@@ -701,7 +704,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 */
@@ -743,6 +747,24 @@ static int __init handle_module_node(EFI_FILE_HANDLE dir_handle,
return ERROR_SET_REG_PROPERTY;
}
+ if ( !is_domu_module &&
+ (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;
+ }
+
return 0;
}
@@ -799,7 +821,7 @@ static int __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
if ( is_boot_module(module_node) )
{
int ret = handle_module_node(dir_handle, module_node, addr_cells,
- size_cells);
+ size_cells, true);
if ( ret < 0 )
return ret;
}
@@ -809,7 +831,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_arch_check_dt_boot(EFI_FILE_HANDLE dir_handle)
@@ -836,6 +858,12 @@ static int __init efi_arch_check_dt_boot(EFI_FILE_HANDLE dir_handle)
if ( handle_dom0less_domain_node(dir_handle, node) < 0 )
return ERROR_DT_MODULE_DOMU;
}
+ else if ( is_boot_module(node) )
+ {
+ if ( handle_module_node(dir_handle, node, addr_len, size_len,
+ false) < 0 )
+ return ERROR_DT_MODULE_DOM0;
+ }
}
/* Free dom0less file names if any */
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index c8c57fbb54..b221494a06 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1296,11 +1296,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) )
@@ -1372,6 +1367,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
if (dt_module_found < 0)
/* efi_arch_check_dt_boot throws some error */
blexit(L"Error processing boot modules on DT.");
+
+ /* If Dom0 is specified, 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);
/*
* Check if a proper configuration is provided to start Xen:
* - Dom0 specified (minimum required)
--
2.17.1
On Tue, 28 Sep 2021, Luca Fancellu wrote: > Add support to load Dom0 boot modules from > the device tree using the uefi,binary property. > > Update documentation about that. > > Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> It is great how simple this patch is! The patch looks all correct. Only one question: do we need a check to make sure the dom0 ramdisk is not loaded twice? Once via uefi,binary and another time via the config file? In other words... > --- > 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 | 36 +++++++++++++-- > xen/common/efi/boot.c | 12 ++--- > 4 files changed, 108 insertions(+), 12 deletions(-) > > diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt > index 354bb43fe1..e73f6476d4 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. > > +- 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 800e67a233..4cebc47a18 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"; > + uefi,binary = "vmlinuz-3.0.31-0.4-xen"; > + bootargs = "[domain 0 command line options]"; > + }; > + > + module@2 { > + compatible = "multiboot,ramdisk", "multiboot,module"; > + 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 > -"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"; > + uefi,binary = "vmlinuz-3.0.31-0.4-xen"; > + bootargs = "[domain 0 command line options]"; > + }; > + > + module@2 { > + compatible = "multiboot,ramdisk", "multiboot,module"; > + 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"; > + 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 4f7c913f86..df63387136 100644 > --- a/xen/arch/arm/efi/efi-boot.h > +++ b/xen/arch/arm/efi/efi-boot.h > @@ -31,8 +31,10 @@ static unsigned int __initdata modules_idx; > #define ERROR_MISSING_DT_PROPERTY (-3) > #define ERROR_RENAME_MODULE_NAME (-4) > #define ERROR_SET_REG_PROPERTY (-5) > +#define ERROR_DOM0_ALREADY_FOUND (-6) > #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); > @@ -45,7 +47,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 bool is_boot_module(int dt_module_offset); > static int handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle, > int domain_node); > @@ -701,7 +704,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 */ > @@ -743,6 +747,24 @@ static int __init handle_module_node(EFI_FILE_HANDLE dir_handle, > return ERROR_SET_REG_PROPERTY; > } > > + if ( !is_domu_module && > + (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; > + } ... is it necessary to update ramdisk as well or check if it is already set? As far as I can tell it is the only other potential conflict that we could have. > return 0; > } > > @@ -799,7 +821,7 @@ static int __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle, > if ( is_boot_module(module_node) ) > { > int ret = handle_module_node(dir_handle, module_node, addr_cells, > - size_cells); > + size_cells, true); > if ( ret < 0 ) > return ret; > } > @@ -809,7 +831,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_arch_check_dt_boot(EFI_FILE_HANDLE dir_handle) > @@ -836,6 +858,12 @@ static int __init efi_arch_check_dt_boot(EFI_FILE_HANDLE dir_handle) > if ( handle_dom0less_domain_node(dir_handle, node) < 0 ) > return ERROR_DT_MODULE_DOMU; > } > + else if ( is_boot_module(node) ) > + { > + if ( handle_module_node(dir_handle, node, addr_len, size_len, > + false) < 0 ) > + return ERROR_DT_MODULE_DOM0; > + } > } > > /* Free dom0less file names if any */ > diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c > index c8c57fbb54..b221494a06 100644 > --- a/xen/common/efi/boot.c > +++ b/xen/common/efi/boot.c > @@ -1296,11 +1296,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) ) > @@ -1372,6 +1367,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > if (dt_module_found < 0) > /* efi_arch_check_dt_boot throws some error */ > blexit(L"Error processing boot modules on DT."); > + > + /* If Dom0 is specified, 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); > /* > * Check if a proper configuration is provided to start Xen: > * - Dom0 specified (minimum required) > -- > 2.17.1 >
On 28.09.2021 18:32, Luca Fancellu wrote: > --- a/xen/common/efi/boot.c > +++ b/xen/common/efi/boot.c > @@ -1296,11 +1296,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) ) > @@ -1372,6 +1367,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > if (dt_module_found < 0) > /* efi_arch_check_dt_boot throws some error */ > blexit(L"Error processing boot modules on DT."); > + > + /* If Dom0 is specified, 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); Why does this code need moving in the first place? That's (to me at least) not obvious from looking just at the common (i.e. non-Arm-specific) part. Hence I would wish for the comment to be slightly extended to indicate that the kernel image may also have been loaded by <whichever function>. Also, two nits: You've broken indentation here (you've lost a space too much compared to the original code) and ... > /* > * Check if a proper configuration is provided to start Xen: > * - Dom0 specified (minimum required) > ... you will want to insert a blank line not only before, but also after your insertion (or, imo less desirable, neither of the two blank lines). Further I wonder whether your addition wouldn't better be after the code following this comment. And finally I wonder (looking back at the earlier patch) why you use kernel.addr there when kernel.ptr is being used here. Unless there's a reason for the difference, being consistent would be better. Jan
> On 29 Sep 2021, at 09:00, Jan Beulich <jbeulich@suse.com> wrote: > > On 28.09.2021 18:32, Luca Fancellu wrote: >> --- a/xen/common/efi/boot.c >> +++ b/xen/common/efi/boot.c >> @@ -1296,11 +1296,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) ) >> @@ -1372,6 +1367,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) >> if (dt_module_found < 0) >> /* efi_arch_check_dt_boot throws some error */ >> blexit(L"Error processing boot modules on DT."); >> + >> + /* If Dom0 is specified, 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); > > Why does this code need moving in the first place? That's (to me at least) > not obvious from looking just at the common (i.e. non-Arm-specific) part. > Hence I would wish for the comment to be slightly extended to indicate > that the kernel image may also have been loaded by <whichever function>. > Sure I will improve the comment to explain that. > Also, two nits: You've broken indentation here (you've lost a space too > much compared to the original code) and ... Yes sorry for that, we didn’t see it, I will fix it. > >> /* >> * Check if a proper configuration is provided to start Xen: >> * - Dom0 specified (minimum required) >> > > ... you will want to insert a blank line not only before, but also after > your insertion (or, imo less desirable, neither of the two blank lines). > > Further I wonder whether your addition wouldn't better be after the code > following this comment. > > And finally I wonder (looking back at the earlier patch) why you use > kernel.addr there when kernel.ptr is being used here. Unless there's a > reason for the difference, being consistent would be better. I will do all of the above for the v4. Thanks for all the feedbacks. Cheers, Luca > > Jan >
© 2016 - 2024 Red Hat, Inc.