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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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?).

Or maybe we add a hypercall to get the underlying CPU compatibility node
direct from Xen. It's the sort of thing I guess "xl info" ought to be
able to display I suppose.

> > - 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?

> > +
> > +        res = fdt_property_string(fdt, "enable-method", "psci");
> 
> #ifdef CONFIG_ARM64 and if it's a 64 bit domain?

Yes.

> > +    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!)

> > +    //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.

Hardcoding this for dom0 seems like a mistake though, we need a way to
find a hole in the paddr space...

Ian.


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