| Message ID | 20210928163209.49611-4-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm/efi: Add dom0less support to UEFI boot | expand |
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 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... Yes I will add a check for it > >> --- >> 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. ramdisk variable can be left unset, we are setting the kernel variable only to reuse the code that verifies the kernel image, however if the user specify the ramdisk in both cfg and device tree we will end up in two multiboot,ramdisk modules being added to the device tree. I will add a check to return an error if ramdisk was declared previously. > > >> 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 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 >
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)
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(-)