Re: [PATCH RFC 14/15] libxl: build a device tree for ARM guests

[Julien Grall <julien.grall@linaro.org>]

On 10/07/2013 05:40 PM, Ian Campbell wrote:
> Uses xc_dom_devicetree_mem which was just added. The call to this needs to be
> carefully sequenced to be after xc_dom_parse_image (so we can tell which kind
> of guest we are building, although we don't use this yet) and before
> xc_dom_mem_init which tries to decide where to place the FDT in guest RAM.
>
> Removes libxl_noarch which would only have been used by IA64 after this
> change. Remove IA64 as part of this patch.
>
> There is no attempt to expose this as a configuration setting for the user.
>
> Includes a debug hook to dump the dtb to a file for inspection.
>
> TODO:
> - Hardcoded armv8 bits need abstracting. Perhaps e.g. read CPU compatiblity
>    node from sysfs?

I'm wondering if it's usefull to have the property compatible. From 
Linux documentation:
   So under /cpus, you are supposed to create a node for every CPU on
   the machine. There is no specific restriction on the name of the
   CPU, though it's common to call it <architecture>,<core>. For
   example, Apple uses PowerPC,G5 while IBM uses PowerPC,970FX.
   However, the Generic Names convention suggests that it would be
   better to simply use 'cpu' for each cpu node and use the compatible
   property to identify the specific cpu core.

Linux doesn't seems to retrieve the compatible node and Freebsd also.
The only way to read this property is from /proc/device-tree. But this 
directory exists only if CONFIG_PROC_DEVICETREE=y.

> - Try it with armv7

The previous point will be the same for the timer and the GIC.

> - Debug hook should be #ifdef DEBUG?
> - Various values (e.g. GIC base addresses, evtchn PPI) should be passed
>    down to the hypervisor by some mechanism I've not decided on yet. Perhaps
>    domctl but maybe better would be via the HVM save format to more easily
>    support migration of the settings?
> - TODOs and //comments in the code
   - Add initrd support?

>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

[..]

> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> new file mode 100644
> index 0000000..913741d
> --- /dev/null
> +++ b/tools/libxl/libxl_arm.c

[..]

> +static int fdt_property_interrupts(libxl__gc *gc, void *fdt,
> +                                   gic_interrupt_t *intr,
> +                                   unsigned num_irq)
> +{
> +    int res;
> +
> +    res = fdt_property(fdt, "interrupts", intr, sizeof (intr[0]) * num_irq);
> +    if ( res )
> +        return res;
> +
> +    res = fdt_property_cell(fdt, "interrupt-parent", PHANDLE_GIC);
> +
> +    return res;
> +}

You don't need this function if the interrupt-parent properties is set 
in the root node. Which is the case below in make_root_properties.

[..]

> +static int make_cpus_node(libxl__gc *gc, void *fdt, int nr_cpus)
> +{
> +    int res, i;
> +
> +    res = fdt_begin_node(fdt, "cpus");
> +    if ( res )
> +        return res;
> +
> +    res = fdt_property_cell(fdt, "#address-cells", 1);
> +    if ( res )
> +        return res;
> +
> +    res = fdt_property_cell(fdt, "#size-cells", 0);
> +    if ( res )
> +        return res;
> +
> +    for (i = 0; i < nr_cpus; i++) {
> +        char name[strlen("cpu@") + 2 + 1];
> +
> +        if ( snprintf(name, sizeof(name)-1, "cpu@%d", i) < 0 )
> +        {
> +            perror("snprintf");
> +            exit(1);
> +        }
> +
> +        res = fdt_begin_node(fdt, name);
> +        if ( res )
> +            return res;
> +
> +        res = fdt_property_string(fdt, "device_type", "cpu");
> +        if ( res )
> +            return res;
> +
> +        res = fdt_property_compat(gc, fdt, 1, "arm,armv8");
> +        if ( res )
> +            return res;
> +
> +        res = fdt_property_string(fdt, "enable-method", "psci");

#ifdef CONFIG_ARM64 and if it's a 64 bit domain?

> +        if ( res )
> +            return res;
> +
> +        res = fdt_property_regs(gc, fdt, 1, 0, 1, (uint64_t)i);
> +        if ( res )
> +            return res;
> +
> +        res = fdt_end_node(fdt);
> +        if ( res )
> +            return res;
> +
> +    }
> +
> +    res = fdt_end_node(fdt);
> +
> +    return res;
> +}
> +
> +static int make_psci_node(libxl__gc *gc, void *fdt)
> +{
> +    int res;
> +
> +    res = fdt_begin_node(fdt, "psci");
> +    if ( res )
> +        return res;
> +
> +    res = fdt_property_compat(gc, fdt, 1, "arm,psci");
> +    if ( res )
> +        return res;
> +
> +    res = fdt_property_string(fdt, "method", "hvc");
> +    if ( res )
> +        return res;
> +
> +    res = fdt_property_cell(fdt, "cpu_off", 0x1);
> +    if ( res )
> +        return res;
> +
> +    res = fdt_property_cell(fdt, "cpu_on", 0x2);
> +    if ( res )
> +        return res;

Can we export include/asm-arm/psci.h and reuse the value here?

> +
> +    res = fdt_end_node(fdt);
> +
> +    return res;
> +}

[..]

> +static int make_hypervisor_node(libxl__gc *gc, void *fdt,
> +                                const libxl_version_info *vers)
> +{
> +    int res;
> +    gic_interrupt_t intr;
> +
> +    /* See linux Documentation/devicetree/bindings/arm/xen.txt */
> +    res = fdt_begin_node(fdt, "hypervisor");
> +    if ( res )
> +        return res;
> +
> +    res = fdt_property_compat(gc, fdt, 2,
> +                              GCSPRINTF("xen,xen-%d.%d",
> +                                        vers->xen_version_major,
> +                                        vers->xen_version_minor),
> +                              "xen,xen");
> +    if ( res )
> +        return res;
> +
> +    //DPRINT("  Grant table range: 0xb0000000-0x20000\n");
> +    /* reg 0 is grant table space */
> +    res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS,
> +                            1,
> +                            (uint64_t)0xb0000000,

I still don't know where this value comes from... if it's a random 
value, can we autogenerate it?

-- 
Julien Grall