Re: [PATCH 2/2] arm/efi: Use dom0less configuration when using EFI boot

[Stefano Stabellini <sstabellini@kernel.org>]

On Thu, 16 Sep 2021, Luca Fancellu wrote:
> > On 16 Sep 2021, at 02:16, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > 
> > On Wed, 15 Sep 2021, Luca Fancellu wrote:
> >> This patch introduces the support for dom0less configuration
> >> when using UEFI boot on ARM, it permits the EFI boot to
> >> continue if no dom0 kernel is specified but at least one domU
> >> is found.
> >> 
> >> Introduce the new property "uefi,binary" for device tree boot
> >> module nodes that are subnode of "xen,domain" compatible nodes.
> >> The property holds a string containing the file name of the
> >> binary that shall be loaded by the uefi loader from the filesystem.
> >> 
> >> Update efi documentation about how to start a dom0less
> >> setup using UEFI
> >> 
> >> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> >> ---
> >> docs/misc/efi.pandoc        |  37 ++++++
> >> xen/arch/arm/efi/efi-boot.h | 244 +++++++++++++++++++++++++++++++++++-
> >> xen/common/efi/boot.c       |  20 ++-
> >> 3 files changed, 294 insertions(+), 7 deletions(-)
> >> 
> >> diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
> >> index ac3cd58cae..db9b3273f8 100644
> >> --- a/docs/misc/efi.pandoc
> >> +++ b/docs/misc/efi.pandoc
> >> @@ -165,3 +165,40 @@ sbsign \
> >> 	--output xen.signed.efi \
> >> 	xen.unified.efi
> >> ```
> >> +
> >> +## UEFI boot and dom0less on ARM
> >> +
> >> +Dom0less feature is supported by ARM and it is possible to use it when Xen is
> >> +started as an EFI application.
> >> +The way to specify the domU domains is by Device Tree as specified in the
> >> +[dom0less](dom0less.html) documentation page under the "Device Tree
> >> +configuration" section, but instead of declaring the reg property in the boot
> >> +module, the user must specify the "uefi,binary" property containing the name
> >> +of the binary file that has to be loaded in memory.
> >> +The UEFI stub will load the binary in memory and it will add the reg property
> >> +accordingly.
> >> +
> >> +An example here:
> >> +
> >> +    domU1 {
> >> +        #address-cells = <1>;
> >> +        #size-cells = <1>;
> >> +        compatible = "xen,domain";
> >> +        memory = <0 0x20000>;
> >> +        cpus = <1>;
> >> +        vpl011;
> >> +
> >> +        module@1 {
> >> +            compatible = "multiboot,kernel", "multiboot,module";
> >> +            uefi,binary = "vmlinuz-3.0.31-0.4-xen";
> >> +            bootargs = "console=ttyAMA0";
> >> +        };
> >> +        module@2 {
> >> +            compatible = "multiboot,ramdisk", "multiboot,module";
> >> +            uefi,binary = "initrd-3.0.31-0.4-xen";
> >> +        };
> >> +        module@3 {
> >> +            compatible = "multiboot,ramdisk", "multiboot,module";
> >> +            uefi,binary = "passthrough.dtb";
> >> +        };
> >> +    };
> > 
> > Can you please also update docs/misc/arm/device-tree/booting.txt ?
> > Either a link to docs/misc/efi.pandoc or a definition of the uefi,binary
> > property (mentioning that it is EFI-only.)
> 
> Yes I will update it.
> 
> > 
> > 
> >> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> >> index 5ff626c6a0..8d7ced70f2 100644
> >> --- a/xen/arch/arm/efi/efi-boot.h
> >> +++ b/xen/arch/arm/efi/efi-boot.h
> >> @@ -8,9 +8,39 @@
> >> #include <asm/setup.h>
> >> #include <asm/smp.h>
> >> 
> >> +typedef struct {
> >> +    char* name;
> >> +    int name_len;
> >> +} dom0less_module_name;
> >> +
> >> +/*
> >> + * Binaries will be translated into bootmodules, the maximum number for them is
> >> + * MAX_MODULES where we should remove a unit for Xen and one for Xen DTB
> >> + */
> >> +#define MAX_DOM0LESS_MODULES (MAX_MODULES - 2)
> >> +static struct file __initdata dom0less_files[MAX_DOM0LESS_MODULES];
> >> +static dom0less_module_name __initdata dom0less_bin_names[MAX_DOM0LESS_MODULES];
> > 
> > I suggest a slightly different model where we don't call AllocatePool to
> > allocate dom0less_module_name.name and instead we just set the pointer
> > directly to the fdt string. There is no risk of the fdt going away at
> > this point so it should be safe to use.
> 
> Yes I thought about this approach but since I was not sure how the DTB behaves when we modify
> It to add the reg property or to modify the module name, then I used this other approach.
> Are you sure that the pointed memory will stay the same after we modify the DTB? My main concern
> was that the DTB structure was going to be modified and the string I was pointing in the DTB memory
> can be relocated elsewhere. 

You are right: fdt_set_name and fdt_set_reg can cause a memmove to be
called, which might change the pointers. Which means we cannot simply
set the char* pointer to the device tree string as it might change.
That's unfortunate. For the lack of a better suggestion, go ahead and
keep AllocatePool/FreePool for the next version.