| Message ID | 20210915142602.42862-2-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm/efi: Add dom0less support to UEFI boot | expand |
Adding Jan for an opinion on the EFI common code changes. See below. On Wed, 15 Sep 2021, Luca Fancellu wrote: > When Xen is started as EFI application, it is checking > the presence of multiboot,module in the DT, if any is > found, the configuration file is skipped. > Restrict this check to just any multiboot,module that > is a direct child of the /chosen node. > > Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> > --- > xen/arch/arm/efi/efi-boot.h | 30 ++++++++++++++++++++++++++++-- > 1 file changed, 28 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h > index cf9c37153f..5ff626c6a0 100644 > --- a/xen/arch/arm/efi/efi-boot.h > +++ b/xen/arch/arm/efi/efi-boot.h > @@ -581,6 +581,8 @@ static void __init efi_arch_load_addr_check(EFI_LOADED_IMAGE *loaded_image) > > static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable) > { > + int node; > + bool dom0_module_found = false; > /* > * For arm, we may get a device tree from GRUB (or other bootloader) > * that contains modules that have already been loaded into memory. In > @@ -592,11 +594,35 @@ static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable) > fdt = lookup_fdt_config_table(SystemTable); > dtbfile.ptr = fdt; > dtbfile.need_to_free = false; /* Config table memory can't be freed. */ > - if ( !fdt || fdt_node_offset_by_compatible(fdt, 0, "multiboot,module") < 0 ) > + > + /* Locate chosen node */ > + node = fdt_subnode_offset(fdt, 0, "chosen"); > + > + /* Cycle through every node inside chosen having multiboot,module */ > + do { > + int depth = 0; > + node = fdt_node_offset_by_compatible(fdt, node, "multiboot,module"); > + /* > + * If the multiboot,module just found is placed at depth less than 3, > + * it means that it is here: /chosen/<module> so it is a module to > + * start dom0. (root is counted as 0) > + */ > + if ( node > 0 ) > + { > + depth = fdt_node_depth(fdt, node); > + if ( (depth >= 0) && (depth < 3) ) > + { > + dom0_module_found = true; > + break; > + } > + } > + } while(node > 0); It should be possible to enable the uefi,binary bootflow for Dom0 and the Dom0 ramdisk too. It would be nice as we could have a 100% UEFI boot, not dependent on U-Boot, both Dom0 and Dom0less, without the xen.cfg file. It doesn't have to be done now by this series, but it should be possible from a device tree bindings perspective. With that in mind, is this check accurate? This patch is saying that if Dom0 is not present in the device tree, then load xen.cfg. But what if it is a true dom0less configuration? Then we would have no dom0, only dom0less VMs, and we might still not want to load xen.cfg. True dom0less is another one of those configurations that don't have to be enabled now by this series but they should be possible from a device tree bindings perspective. I tried to think of ways to improve this check, for instance searching for a module that doesn't have "uefi,binary" but has the regular "reg" property. If there is one, then we could skip loading xen.cfg. But that doesn't work if we have a UEFI-only true dom0less configuration. So I am thinking that we have no choice but introducing a new property to tell us whether we should or should not load xen.cfg when multiboot,modules are present. Taking inspiration from HyperLaunch, it could be a new node: chosen { cfg { compatible = "xen,uefi-config", "multiboot,module"; uefi,binary = "xen.cfg"; }; }; In efi_arch_use_config_file we would check if there are any nodes compatible with "multiboot,module". If there are none, it returns true. If there are any, and one of them is also compatible "xen,uefi-config", then efi_arch_use_config_file returns true and also the specified configuration filename. If there are nodes compatible to "multiboot,module" but none of them is compatible to "module,uefi-config", then efi_arch_use_config_file returns false. We use the device tree only. I think that would be clearer and more consistent from a specification perspective, but it requires one change in common code: efi_arch_use_config_file would not just return bool but it would also return a filename if found (it could be a pointer parameter to the function). Otherwise, we could add a simple property like the following, without a specific value and without a filename: chosen { xen,uefi-config; }; The presence of xen,uefi-config could mean that Xen should go through the usual guessing game to figure out the right cfg file to load. This would not require any common code changes because efi_arch_use_config_file could simply return bool as it does today. My preference is the xen,uefi-config compatible node, but I think the property would also work. Jan, do you have an opinion on whether efi_arch_use_config_file has to stay as it is today, or would you be open to the possibility of making efi_arch_use_config_file return a filename too?
On 16.09.2021 02:16, Stefano Stabellini wrote: > So I am thinking that we have no choice but introducing a new property > to tell us whether we should or should not load xen.cfg when > multiboot,modules are present. > > Taking inspiration from HyperLaunch, it could be a new node: > > chosen { > cfg { > compatible = "xen,uefi-config", "multiboot,module"; > uefi,binary = "xen.cfg"; > }; > }; > > In efi_arch_use_config_file we would check if there are any nodes > compatible with "multiboot,module". If there are none, it returns true. > > If there are any, and one of them is also compatible "xen,uefi-config", > then efi_arch_use_config_file returns true and also the specified > configuration filename. > > If there are nodes compatible to "multiboot,module" but none of them is > compatible to "module,uefi-config", then efi_arch_use_config_file > returns false. We use the device tree only. > > I think that would be clearer and more consistent from a specification > perspective, but it requires one change in common code: > efi_arch_use_config_file would not just return bool but it would also > return a filename if found (it could be a pointer parameter to the > function). > > > Otherwise, we could add a simple property like the following, without a > specific value and without a filename: > > chosen { > xen,uefi-config; > }; > > The presence of xen,uefi-config could mean that Xen should go through > the usual guessing game to figure out the right cfg file to load. This > would not require any common code changes because > efi_arch_use_config_file could simply return bool as it does today. > > My preference is the xen,uefi-config compatible node, but I think the > property would also work. > > > Jan, do you have an opinion on whether efi_arch_use_config_file has to > stay as it is today, or would you be open to the possibility of making > efi_arch_use_config_file return a filename too? I see no issue with making such a change; my preference would be an altered return type, provided all present cases can be expressed alongside the new one you need. Jan
> On 16 Sep 2021, at 01:16, Stefano Stabellini <sstabellini@kernel.org> wrote: > > Adding Jan for an opinion on the EFI common code changes. See below. > > > On Wed, 15 Sep 2021, Luca Fancellu wrote: >> When Xen is started as EFI application, it is checking >> the presence of multiboot,module in the DT, if any is >> found, the configuration file is skipped. >> Restrict this check to just any multiboot,module that >> is a direct child of the /chosen node. >> >> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> >> --- >> xen/arch/arm/efi/efi-boot.h | 30 ++++++++++++++++++++++++++++-- >> 1 file changed, 28 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h >> index cf9c37153f..5ff626c6a0 100644 >> --- a/xen/arch/arm/efi/efi-boot.h >> +++ b/xen/arch/arm/efi/efi-boot.h >> @@ -581,6 +581,8 @@ static void __init efi_arch_load_addr_check(EFI_LOADED_IMAGE *loaded_image) >> >> static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable) >> { >> + int node; >> + bool dom0_module_found = false; >> /* >> * For arm, we may get a device tree from GRUB (or other bootloader) >> * that contains modules that have already been loaded into memory. In >> @@ -592,11 +594,35 @@ static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable) >> fdt = lookup_fdt_config_table(SystemTable); >> dtbfile.ptr = fdt; >> dtbfile.need_to_free = false; /* Config table memory can't be freed. */ >> - if ( !fdt || fdt_node_offset_by_compatible(fdt, 0, "multiboot,module") < 0 ) >> + >> + /* Locate chosen node */ >> + node = fdt_subnode_offset(fdt, 0, "chosen"); >> + >> + /* Cycle through every node inside chosen having multiboot,module */ >> + do { >> + int depth = 0; >> + node = fdt_node_offset_by_compatible(fdt, node, "multiboot,module"); >> + /* >> + * If the multiboot,module just found is placed at depth less than 3, >> + * it means that it is here: /chosen/<module> so it is a module to >> + * start dom0. (root is counted as 0) >> + */ >> + if ( node > 0 ) >> + { >> + depth = fdt_node_depth(fdt, node); >> + if ( (depth >= 0) && (depth < 3) ) >> + { >> + dom0_module_found = true; >> + break; >> + } >> + } >> + } while(node > 0); > > It should be possible to enable the uefi,binary bootflow for Dom0 and > the Dom0 ramdisk too. It would be nice as we could have a 100% UEFI > boot, not dependent on U-Boot, both Dom0 and Dom0less, without the > xen.cfg file. It doesn't have to be done now by this series, but it > should be possible from a device tree bindings perspective. > > With that in mind, is this check accurate? This patch is saying that if > Dom0 is not present in the device tree, then load xen.cfg. But what if > it is a true dom0less configuration? Then we would have no dom0, only > dom0less VMs, and we might still not want to load xen.cfg. True dom0less > is another one of those configurations that don't have to be enabled now > by this series but they should be possible from a device tree bindings > perspective. > > > I tried to think of ways to improve this check, for instance searching > for a module that doesn't have "uefi,binary" but has the regular "reg" > property. If there is one, then we could skip loading xen.cfg. But that > doesn't work if we have a UEFI-only true dom0less configuration. > > So I am thinking that we have no choice but introducing a new property > to tell us whether we should or should not load xen.cfg when > multiboot,modules are present. > > Taking inspiration from HyperLaunch, it could be a new node: > > chosen { > cfg { > compatible = "xen,uefi-config", "multiboot,module"; > uefi,binary = "xen.cfg"; > }; > }; > > In efi_arch_use_config_file we would check if there are any nodes > compatible with "multiboot,module". If there are none, it returns true. > > If there are any, and one of them is also compatible "xen,uefi-config", > then efi_arch_use_config_file returns true and also the specified > configuration filename. > > If there are nodes compatible to "multiboot,module" but none of them is > compatible to "module,uefi-config", then efi_arch_use_config_file > returns false. We use the device tree only. > > I think that would be clearer and more consistent from a specification > perspective, but it requires one change in common code: > efi_arch_use_config_file would not just return bool but it would also > return a filename if found (it could be a pointer parameter to the > function). > > > Otherwise, we could add a simple property like the following, without a > specific value and without a filename: > > chosen { > xen,uefi-config; > }; > > The presence of xen,uefi-config could mean that Xen should go through > the usual guessing game to figure out the right cfg file to load. This > would not require any common code changes because > efi_arch_use_config_file could simply return bool as it does today. > > My preference is the xen,uefi-config compatible node, but I think the > property would also work. > > > Jan, do you have an opinion on whether efi_arch_use_config_file has to > stay as it is today, or would you be open to the possibility of making > efi_arch_use_config_file return a filename too? Hi Stefano, True dom0less is a configuration that this serie enables: if there is no dom0 kernel specified in the xen.cfg then only the domUs will be loaded and started. The following cases are valid: 1) start only dom0 [dom0 modules in xen.cfg or embedded in Xen image] 2) start only domUs, true dom0less [no dom0 modules in xen.cfg and inside Xen image, domUs on DT] 3) start dom0 and domUs [(dom0 modules in xen.cfg or inside Xen image) and domUs on DT] I don’t understand why we want to add new properties to avoid/not avoid to read the xen.cfg, am I missing something? Cheers, Luca
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h index cf9c37153f..5ff626c6a0 100644 --- a/xen/arch/arm/efi/efi-boot.h +++ b/xen/arch/arm/efi/efi-boot.h @@ -581,6 +581,8 @@ static void __init efi_arch_load_addr_check(EFI_LOADED_IMAGE *loaded_image) static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable) { + int node; + bool dom0_module_found = false; /* * For arm, we may get a device tree from GRUB (or other bootloader) * that contains modules that have already been loaded into memory. In @@ -592,11 +594,35 @@ static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable) fdt = lookup_fdt_config_table(SystemTable); dtbfile.ptr = fdt; dtbfile.need_to_free = false; /* Config table memory can't be freed. */ - if ( !fdt || fdt_node_offset_by_compatible(fdt, 0, "multiboot,module") < 0 ) + + /* Locate chosen node */ + node = fdt_subnode_offset(fdt, 0, "chosen"); + + /* Cycle through every node inside chosen having multiboot,module */ + do { + int depth = 0; + node = fdt_node_offset_by_compatible(fdt, node, "multiboot,module"); + /* + * If the multiboot,module just found is placed at depth less than 3, + * it means that it is here: /chosen/<module> so it is a module to + * start dom0. (root is counted as 0) + */ + if ( node > 0 ) + { + depth = fdt_node_depth(fdt, node); + if ( (depth >= 0) && (depth < 3) ) + { + dom0_module_found = true; + break; + } + } + } while(node > 0); + + if ( !fdt || !dom0_module_found ) { /* * We either have no FDT, or one without modules, so we must have a - * Xen EFI configuration file to specify modules. (dom0 required) + * Xen EFI configuration file to specify modules. */ return true; }
When Xen is started as EFI application, it is checking the presence of multiboot,module in the DT, if any is found, the configuration file is skipped. Restrict this check to just any multiboot,module that is a direct child of the /chosen node. Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- xen/arch/arm/efi/efi-boot.h | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-)