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

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

On 10/15/2013 11:00 AM, Ian Campbell wrote:
> On Tue, 2013-10-15 at 00:11 +0100, Julien Grall wrote:
>> 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.
> 
> Hrm this seems to imply that either you should name the node
> <architecture>,<core> *or* you should call it cpu and have a
> compatibility property <architecture>,<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.
> 
> Yes.
> 
> I suppose we could try /proc/device-tree and fall back to /proc/cpuinfo
> (which should at least get us v7 vs v8) and finally fallback to a
> hardcoded value based on the architecture the tools are compiled for
> (which is lame, but better than nothing?).

Reading the value from /proc/device-tree is a bit complicate. You need
to parse each directory to find a CPU.

>>> - Try it with armv7
>>
>> The previous point will be the same for the timer and the GIC.
> 
> Yes.
> 
>>> +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.
> 
> OK. The reason we have this for dom0 is that we can't control whether
> the host DTB has interrupt-parent or not?

Right. Some device tree have a simple-bus where all nodes live and also
the interrupt-property

/ {

  smb {
    compatible = "simple,bus"
    interrupt-parent = &gic
    ...
  }
}

During dom0 DT creation, Xen will add the node in / which may not have
the right property.

>>> +    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?
> 
> I suppose we ought to, since Xen is the one implementing the actual
> functionality. I notice that even Xen itself isn't using those #defines
> (nothing is AFAICT!)

It's used in traps.c via in the macro PSCI and in domain_build.c when
the PSCI node is creating.

>>> +    //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?
> 
> It's an arbitrary value which we picked when we defined our guest
> virtual platform. It's "random" in the same way as the address of the
> UART picked by an SoC designer is.
> 
> It should be a #define for sure though.

If you choose to hardcode this address, can you add a TODO? So we won't
forget later.

-- 
Julien Grall