From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@linaro.org>
Subject: Re: [PATCH RFC 14/15] libxl: build a device tree for
	ARM guests
Date: Tue, 15 Oct 2013 14:23:09 +0100
Message-ID: <525D41BD.802@linaro.org>
References: <1381163944.21562.129.camel@kazak.uk.xensource.com>
	<1381164001-1446-14-git-send-email-ian.campbell@citrix.com>
	<525C7A28.7060003@linaro.org>
	<1381831218.21901.11.camel@kazak.uk.xensource.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1381831218.21901.11.camel@kazak.uk.xensource.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Ian Campbell <Ian.Campbell@citrix.com>
Cc: stefano.stabellini@eu.citrix.com, tim@xen.org, xen-devel@lists.xen.org
List-Id: xen-devel@lists.xenproject.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