Re: [Xen-devel] [PATCH v4 17/24] arm/acpi: Create min DT stub for Dom0

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Sun, 28 Feb 2016, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@xxxxxxxxxx>
> 
> Create a DT for Dom0 for ACPI-case only. DT contains minimal required
> informations such as Dom0 bootargs, initrd, efi description table and
> address of uefi memory table.
> 
> Also port the document of this device tree bindings from Linux.
> 
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Signed-off-by: Naresh Bhat <naresh.bhat@xxxxxxxxxx>
> Signed-off-by: Parth Dixit <parth.dixit@xxxxxxxxxx>
> Signed-off-by: Shannon Zhao <shannon.zhao@xxxxxxxxxx>
> ---
> v4: port the document of this device tree bindings from Linux
> ---
>  docs/misc/arm/device-tree/xen.txt |  58 ++++++++++++++++
>  xen/arch/arm/domain_build.c       | 143 
> ++++++++++++++++++++++++++++++++++++++
>  xen/common/efi/boot.c             |  47 +++++++++++++
>  xen/include/asm-arm/setup.h       |   2 +
>  4 files changed, 250 insertions(+)
>  create mode 100644 docs/misc/arm/device-tree/xen.txt
> 
> diff --git a/docs/misc/arm/device-tree/xen.txt 
> b/docs/misc/arm/device-tree/xen.txt
> new file mode 100644
> index 0000000..6f83f76
> --- /dev/null
> +++ b/docs/misc/arm/device-tree/xen.txt

Calling this file xen.txt inside the Xen repository is not very
meaningful, maybe we should rename it to uefi.txt

Otherwise you can add this info to the existing booting.txt


> @@ -0,0 +1,58 @@
> +* Xen hypervisor device tree bindings
> +
> +Xen ARM virtual platforms shall have a top-level "hypervisor" node with
> +the following properties:
> +
> +- compatible:
> +     compatible = "xen,xen-<version>", "xen,xen";
> +  where <version> is the version of the Xen ABI of the platform.
> +
> +- reg: specifies the base physical address and size of a region in
> +  memory where the grant table should be mapped to, using an
> +  HYPERVISOR_memory_op hypercall. The memory region is large enough to map
> +  the whole grant table (it is larger or equal to gnttab_max_grant_frames()).
> +
> +- interrupts: the interrupt used by Xen to inject event notifications.
> +  A GIC node is also required.
> +
> +To support UEFI on Xen ARM virtual platforms, Xen populates the FDT "uefi" 
> node
> +under /hypervisor with following parameters:
> +
> +________________________________________________________________________________
> +Name                      | Size   | Description
> +================================================================================
> +xen,uefi-system-table     | 64-bit | Guest physical address of the UEFI 
> System
> +                       |        | Table.
> +--------------------------------------------------------------------------------
> +xen,uefi-mmap-start       | 64-bit | Guest physical address of the UEFI 
> memory
> +                       |        | map.
> +--------------------------------------------------------------------------------
> +xen,uefi-mmap-size        | 32-bit | Size in bytes of the UEFI memory map
> +                          |        | pointed to in previous entry.
> +--------------------------------------------------------------------------------
> +xen,uefi-mmap-desc-size   | 32-bit | Size in bytes of each entry in the UEFI
> +                          |        | memory map.
> +--------------------------------------------------------------------------------
> +xen,uefi-mmap-desc-ver    | 32-bit | Version of the mmap descriptor format.
> +--------------------------------------------------------------------------------
> +
> +Example (assuming #address-cells = <2> and #size-cells = <2>):
> +
> +hypervisor {
> +     compatible = "xen,xen-4.3", "xen,xen";
> +     reg = <0 0xb0000000 0 0x20000>;
> +     interrupts = <1 15 0xf08>;
> +     uefi {
> +             xen,uefi-system-table = <0xXXXXXXXX>;
> +             xen,uefi-mmap-start = <0xXXXXXXXX>;
> +             xen,uefi-mmap-size = <0xXXXXXXXX>;
> +             xen,uefi-mmap-desc-size = <0xXXXXXXXX>;
> +             xen,uefi-mmap-desc-ver = <0xXXXXXXXX>;
> +        };
> +};
> +
> +The format and meaning of the "xen,uefi-*" parameters are similar to those in
> +Documentation/arm/uefi.txt, which are provided by the regular UEFI stub. 
> However
                             ^ in Linux                         ^ Linux


> +they differ because they are provided by the Xen hypervisor, together with a 
> set
> +of UEFI runtime services implemented via hypercalls, see
> +http://xenbits.xen.org/docs/unstable/hypercall/x86_64/include,public,platform.h.html.

I think that here is fine to use a simple path within the Xen repo:

xen/include/public/platform.h


> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 083ddd5..4b1f387 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1357,6 +1357,145 @@ static int prepare_dtb(struct domain *d, struct 
> kernel_info *kinfo)
>  }
>  
>  #ifdef CONFIG_ACPI
> +#define ACPI_DOM0_FDT_MIN_SIZE 4096
> +
> +static int make_chosen_node(const struct kernel_info *kinfo,
> +                            struct membank tbl_add[])
> +{
> +    int res;
> +    const char *bootargs = NULL;
> +    const struct bootmodule *mod = kinfo->kernel_bootmodule;
> +    void *fdt = kinfo->fdt;
> +
> +    DPRINT("Create chosen node\n");
> +    res = fdt_begin_node(fdt, "chosen");
> +    if ( res )
> +        return res;
> +
> +    if ( mod && mod->cmdline[0] )
> +    {
> +        bootargs = &mod->cmdline[0];
> +        res = fdt_property(fdt, "bootargs", bootargs, strlen(bootargs) + 1);
> +        if ( res )
> +           return res;
> +    }
> +
> +    /*
> +     * If the bootloader provides an initrd, we must create a placeholder
> +     * for the initrd properties. The values will be replaced later.
> +     */
> +    if ( mod && mod->size )
> +    {
> +        u64 a = 0;
> +        res = fdt_property(kinfo->fdt, "linux,initrd-start", &a, sizeof(a));
> +        if ( res )
> +            return res;
> +
> +        res = fdt_property(kinfo->fdt, "linux,initrd-end", &a, sizeof(a));
> +        if ( res )
> +            return res;
> +    }
> +
> +    res = fdt_end_node(fdt);
> +
> +    return res;
> +}
> +static int acpi_make_hypervisor_node(const struct kernel_info *kinfo,
> +                                     struct membank tbl_add[])
> +{
> +    const char compat[] =
> +        "xen,xen-"__stringify(XEN_VERSION)"."__stringify(XEN_SUBVERSION)"\0"
> +        "xen,xen";
> +    int res;
> +    /* Convenience alias */
> +    void *fdt = kinfo->fdt;
> +
> +    DPRINT("Create hypervisor node\n");
> +
> +    /* See linux Documentation/devicetree/bindings/arm/xen.txt */
> +    res = fdt_begin_node(fdt, "hypervisor");
> +    if ( res )
> +        return res;
> +
> +    /* Cannot use fdt_property_string due to embedded nulls */
> +    res = fdt_property(fdt, "compatible", compat, sizeof(compat));
> +    if ( res )
> +        return res;
> +
> +    res = arm_acpi_make_efi_nodes(fdt, tbl_add);
> +    if ( res )
> +        return res;
> +
> +    res = fdt_end_node(fdt);
> +
> +    return res;
> +}

Please refactor the existing make_hypervisor_node into a shared function
and use that instead.


> +/*
> + * Prepare a minimal DTB for Dom0 which contains bootargs, initrd, memory
> + * information, EFI table.
> + */
> +static int create_acpi_dtb(struct kernel_info *kinfo, struct membank 
> tbl_add[])
> +{
> +    int new_size;
> +    int ret;
> +
> +    DPRINT("Prepare a min DTB for DOM0\n");
> +
> +    /* Allocate min size for DT */
> +    new_size = ACPI_DOM0_FDT_MIN_SIZE;
> +    kinfo->fdt = xmalloc_bytes(new_size);
> +
> +    if ( kinfo->fdt == NULL )
> +        return -ENOMEM;
> +
> +    /* Create a new empty DT for DOM0 */
> +    ret = fdt_create(kinfo->fdt, new_size);
> +    if ( ret < 0 )
> +        goto err;
> +
> +    ret = fdt_finish_reservemap(kinfo->fdt);
> +    if ( ret < 0 )
> +        goto err;
> +
> +    ret = fdt_begin_node(kinfo->fdt, "/");
> +    if ( ret < 0 )
> +        goto err;
> +
> +    ret = fdt_property_cell(kinfo->fdt, "#address-cells", 2);
> +    if ( ret )
> +        return ret;
> +
> +    ret = fdt_property_cell(kinfo->fdt, "#size-cells", 1);
> +    if ( ret )
> +        return ret;
> +
> +    /* Create a chosen node for DOM0 */
> +    ret = make_chosen_node(kinfo, tbl_add);
> +    if ( ret )
> +        goto err;
> +
> +    ret = acpi_make_hypervisor_node(kinfo, tbl_add);
> +    if ( ret )
> +        goto err;
> +
> +    ret = fdt_end_node(kinfo->fdt);
> +    if ( ret < 0 )
> +        goto err;
> +
> +    ret = fdt_finish(kinfo->fdt);
> +    if ( ret < 0 )
> +        goto err;
> +
> +    return 0;
> +
> +  err:
> +    printk("Device tree generation failed (%d).\n", ret);
> +    xfree(kinfo->fdt);
> +    return -EINVAL;
> +}
> +
>  static void acpi_map_other_tables(struct domain *d)
>  {
>      int i;
> @@ -1706,6 +1845,10 @@ static int prepare_acpi(struct domain *d, struct 
> kernel_info *kinfo)
>          return rc;
>      }
>  
> +    rc = create_acpi_dtb(kinfo, tbl_add);
> +    if ( rc != 0 )
> +        return rc;
> +
>      return 0;
>  }
>  #else
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index b8d7409..c0b08ef 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1271,6 +1271,53 @@ void __init acpi_create_efi_mmap_table(paddr_t paddr, 
> paddr_t size,
>      memory_map[offset].NumberOfPages = PFN_UP(size);
>      memory_map[offset].Attribute = EFI_MEMORY_WB;
>  }
> +
> +/* Create place holder for efi values. */
> +int __init arm_acpi_make_efi_nodes(void *fdt, struct membank tbl_add[])
> +{
> +    u64 fdt_val64;
> +    u32 fdt_val32;
> +    int desc_ver = mdesc_ver;
> +    int res;
> +
> +    res = fdt_begin_node(fdt, "uefi");
> +    if ( res )
> +        return res;
> +
> +    fdt_val64 = cpu_to_fdt64(tbl_add[TBL_EFIT].start);
> +    res = fdt_property(fdt, "xen,uefi-system-table",
> +                       &fdt_val64, sizeof(fdt_val64));
> +    if ( res )
> +        return res;
> +
> +    fdt_val64 = cpu_to_fdt64(tbl_add[TBL_MMAP].start);
> +    res = fdt_property(fdt, "xen,uefi-mmap-start",
> +                       &fdt_val64,  sizeof(fdt_val64));
> +    if ( res )
> +        return res;
> +
> +    fdt_val32 = cpu_to_fdt32(tbl_add[TBL_MMAP].size);
> +    res = fdt_property(fdt, "xen,uefi-mmap-size",
> +                       &fdt_val32,  sizeof(fdt_val32));
> +    if ( res )
> +        return res;
> +
> +    fdt_val32 = cpu_to_fdt32(sizeof(EFI_MEMORY_DESCRIPTOR));
> +    res = fdt_property(fdt, "xen,uefi-mmap-desc-size",
> +                         &fdt_val32, sizeof(fdt_val32));
> +    if ( res )
> +        return res;
> +
> +    fdt_val32 = cpu_to_fdt32(desc_ver);
> +    res = fdt_property(fdt, "xen,uefi-mmap-desc-ver",
> +                         &fdt_val32, sizeof(fdt_val32));
> +    if ( res )
> +        return res;
> +
> +    res = fdt_end_node(fdt);
> +
> +    return res;
> +}

This function should not be under /common. It should be in domain_build.c
or arch/arm/efi.


>  #endif
>  
>  #ifndef CONFIG_ARM /* TODO - runtime service support */
> diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
> index af5a038..fdec501 100644
> --- a/xen/include/asm-arm/setup.h
> +++ b/xen/include/asm-arm/setup.h
> @@ -61,6 +61,8 @@ void acpi_create_efi_mmap_table(paddr_t paddr, paddr_t size,
>                                  const struct meminfo *mem,
>                                  struct membank tbl_add[]);
>  
> +int arm_acpi_make_efi_nodes(void *fdt, struct membank tbl_add[]);
> +
>  int construct_dom0(struct domain *d);
>  
>  void discard_initial_modules(void);
> -- 
> 2.0.4
> 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel