| Message ID | 20210922141341.42288-2-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm/efi: Add dom0less support to UEFI boot | expand |
On Wed, 22 Sep 2021, Luca Fancellu wrote: > Introduce the uefi,cfg-load DT property of /chosen > node for ARM whose presence decide whether to force > the load of the UEFI Xen configuration file. > > The logic is that if any multiboot,module is found in > the DT, then the uefi,cfg-load property is used to see > if the UEFI Xen configuration file is needed. > > Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> The patch looks OK, just a couple of minor comments below. > --- > v2 changes: > - Introduced uefi,cfg-load property > - Add documentation about the property > --- > docs/misc/efi.pandoc | 2 ++ > xen/arch/arm/efi/efi-boot.h | 28 +++++++++++++++++++++++----- > 2 files changed, 25 insertions(+), 5 deletions(-) > > diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc > index ac3cd58cae..e289c5e7ba 100644 > --- a/docs/misc/efi.pandoc > +++ b/docs/misc/efi.pandoc > @@ -14,6 +14,8 @@ loaded the modules and describes them in the device tree provided to Xen. If a > bootloader provides a device tree containing modules then any configuration > files are ignored, and the bootloader is responsible for populating all > relevant device tree nodes. > +The property "uefi,cfg-load" can be specified in the /chosen node to force Xen > +to load the configuration file even if multiboot modules are found. I think that, in addition to this, we also need to add the property to docs/misc/arm/device-tree/booting.txt where our "official" device tree bindings are maintained. I would add it below "Command lines" and before "Creating Multiple Domains directly from Xen" maybe as a new chapter. It could be called "Other Options" but other ideas could be valid too. You can say that uefi,cfg-load is a boolean. > Once built, `make install-xen` will place the resulting binary directly into > the EFI boot partition, provided `EFI_VENDOR` is set in the environment (and > diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h > index cf9c37153f..8ceeba4ad1 100644 > --- a/xen/arch/arm/efi/efi-boot.h > +++ b/xen/arch/arm/efi/efi-boot.h > @@ -581,22 +581,40 @@ 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) > { > + bool skip_cfg_file = 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 > - * this case, we do not use a configuration file, and rely on the > - * bootloader to have loaded all required modules and appropriate > - * options. > + * this case, we search for the property uefi,cfg-load in the /chosen node > + * to decide whether to skip the UEFI Xen configuration file or not. > */ > > 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 ) > + > + if ( fdt_node_offset_by_compatible(fdt, 0, "multiboot,module") > 0 ) > + { > + /* Locate chosen node */ > + int node = fdt_subnode_offset(fdt, 0, "chosen"); > + const void *cfg_load_prop; > + int cfg_load_len; > + > + if ( node > 0 ) > + { > + /* Check if uefi,cfg-load property exists */ > + cfg_load_prop = fdt_getprop(fdt, node, "uefi,cfg-load", > + &cfg_load_len); > + if ( !cfg_load_prop ) > + skip_cfg_file = true; > + } > + } > + > + if ( !fdt || !skip_cfg_file ) Just a suggestion, but rather than the double negative, wouldn't it be simpler to define it as bool load_cfg_file = true; ? > { > /* > * 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. > */ Also mention in the commit message that you are taking the opportunity to update this comment do remove "dom0 required". > return true; > } > -- > 2.17.1 >
> On 22 Sep 2021, at 22:19, Stefano Stabellini <sstabellini@kernel.org> wrote: > > On Wed, 22 Sep 2021, Luca Fancellu wrote: >> Introduce the uefi,cfg-load DT property of /chosen >> node for ARM whose presence decide whether to force >> the load of the UEFI Xen configuration file. >> >> The logic is that if any multiboot,module is found in >> the DT, then the uefi,cfg-load property is used to see >> if the UEFI Xen configuration file is needed. >> >> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> > > The patch looks OK, just a couple of minor comments below. > > >> --- >> v2 changes: >> - Introduced uefi,cfg-load property >> - Add documentation about the property >> --- >> docs/misc/efi.pandoc | 2 ++ >> xen/arch/arm/efi/efi-boot.h | 28 +++++++++++++++++++++++----- >> 2 files changed, 25 insertions(+), 5 deletions(-) >> >> diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc >> index ac3cd58cae..e289c5e7ba 100644 >> --- a/docs/misc/efi.pandoc >> +++ b/docs/misc/efi.pandoc >> @@ -14,6 +14,8 @@ loaded the modules and describes them in the device tree provided to Xen. If a >> bootloader provides a device tree containing modules then any configuration >> files are ignored, and the bootloader is responsible for populating all >> relevant device tree nodes. >> +The property "uefi,cfg-load" can be specified in the /chosen node to force Xen >> +to load the configuration file even if multiboot modules are found. > Hi Stefano, > I think that, in addition to this, we also need to add the property to > docs/misc/arm/device-tree/booting.txt where our "official" device tree > bindings are maintained. I would add it below "Command lines" and before > "Creating Multiple Domains directly from Xen" maybe as a new chapter. > It could be called "Other Options" but other ideas could be valid too. > > You can say that uefi,cfg-load is a boolean. Sure, I will add in v3, what about this: UEFI boot and DT ============== When Xen is booted using UEFI, it doesn't read the configuration file if any multiboot module is specified. To force Xen to load the configuration file, the boolean property uefi,cfg-load must be declared in the /chosen node. > >> Once built, `make install-xen` will place the resulting binary directly into >> the EFI boot partition, provided `EFI_VENDOR` is set in the environment (and >> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h >> index cf9c37153f..8ceeba4ad1 100644 >> --- a/xen/arch/arm/efi/efi-boot.h >> +++ b/xen/arch/arm/efi/efi-boot.h >> @@ -581,22 +581,40 @@ 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) >> { >> + bool skip_cfg_file = 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 >> - * this case, we do not use a configuration file, and rely on the >> - * bootloader to have loaded all required modules and appropriate >> - * options. >> + * this case, we search for the property uefi,cfg-load in the /chosen node >> + * to decide whether to skip the UEFI Xen configuration file or not. >> */ >> >> 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 ) >> + >> + if ( fdt_node_offset_by_compatible(fdt, 0, "multiboot,module") > 0 ) >> + { >> + /* Locate chosen node */ >> + int node = fdt_subnode_offset(fdt, 0, "chosen"); >> + const void *cfg_load_prop; >> + int cfg_load_len; >> + >> + if ( node > 0 ) >> + { >> + /* Check if uefi,cfg-load property exists */ >> + cfg_load_prop = fdt_getprop(fdt, node, "uefi,cfg-load", >> + &cfg_load_len); >> + if ( !cfg_load_prop ) >> + skip_cfg_file = true; >> + } >> + } >> + >> + if ( !fdt || !skip_cfg_file ) > > Just a suggestion, but rather than the double negative, wouldn't it be > simpler to define it as > > bool load_cfg_file = true; > > ? Sure I will modify it. > > >> { >> /* >> * 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. >> */ > > Also mention in the commit message that you are taking the opportunity > to update this comment do remove "dom0 required". Yes I will add it in the commit message Cheers, Luca > > >> return true; >> } >> -- >> 2.17.1
diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc index ac3cd58cae..e289c5e7ba 100644 --- a/docs/misc/efi.pandoc +++ b/docs/misc/efi.pandoc @@ -14,6 +14,8 @@ loaded the modules and describes them in the device tree provided to Xen. If a bootloader provides a device tree containing modules then any configuration files are ignored, and the bootloader is responsible for populating all relevant device tree nodes. +The property "uefi,cfg-load" can be specified in the /chosen node to force Xen +to load the configuration file even if multiboot modules are found. Once built, `make install-xen` will place the resulting binary directly into the EFI boot partition, provided `EFI_VENDOR` is set in the environment (and diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h index cf9c37153f..8ceeba4ad1 100644 --- a/xen/arch/arm/efi/efi-boot.h +++ b/xen/arch/arm/efi/efi-boot.h @@ -581,22 +581,40 @@ 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) { + bool skip_cfg_file = 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 - * this case, we do not use a configuration file, and rely on the - * bootloader to have loaded all required modules and appropriate - * options. + * this case, we search for the property uefi,cfg-load in the /chosen node + * to decide whether to skip the UEFI Xen configuration file or not. */ 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 ) + + if ( fdt_node_offset_by_compatible(fdt, 0, "multiboot,module") > 0 ) + { + /* Locate chosen node */ + int node = fdt_subnode_offset(fdt, 0, "chosen"); + const void *cfg_load_prop; + int cfg_load_len; + + if ( node > 0 ) + { + /* Check if uefi,cfg-load property exists */ + cfg_load_prop = fdt_getprop(fdt, node, "uefi,cfg-load", + &cfg_load_len); + if ( !cfg_load_prop ) + skip_cfg_file = true; + } + } + + if ( !fdt || !skip_cfg_file ) { /* * 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; }
Introduce the uefi,cfg-load DT property of /chosen node for ARM whose presence decide whether to force the load of the UEFI Xen configuration file. The logic is that if any multiboot,module is found in the DT, then the uefi,cfg-load property is used to see if the UEFI Xen configuration file is needed. Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- v2 changes: - Introduced uefi,cfg-load property - Add documentation about the property --- docs/misc/efi.pandoc | 2 ++ xen/arch/arm/efi/efi-boot.h | 28 +++++++++++++++++++++++----- 2 files changed, 25 insertions(+), 5 deletions(-)