Re: [Xen-devel] [PATCH RFC 2/6] xen/arm: hwdom GIC: inherit interrupt parent from host device tree

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

Sorry for the late review, I promise I'll be faster next time :-)

On Mon, 5 Sep 2016, Kyle Temkin wrote:
> From: "Kyle J. Temkin" <temkink@xxxxxxxxxxxx>
> 
> Currently, we don't copy in the interrupt parent from the host device
> tree; and instead let Xen automatically figure it out when generating
> the device tree for the hardware domain.
> 
> In cases where a non-GIC interrupt controller is present, this can lead
> to incorrect assignment of interrupt parents, and throughly confuse the
> hwdom. Instead of letting this fall to chance, pass through the
> phandles.

Let me get this straight: the problem is that domain_build.c doesn't get
the interrupt-parent property from the host device tree, instead it just
generates it by blindly pointing interrupt-parent to the gic. This
happens in fdt_property_interrupts for generated nodes, such as the
hypervisor and timer nodes. Other nodes are typically just copied over
as is, so they are not affected.

This is OK, unless one of these nodes actually need a different
interrupt-parent. However these nodes don't really correspond to regular
devices and shouldn't actually need a different interrupt-parent.
Certainly not the hypervisor, timer, cpus and memory nodes.

This only leaves out the GIC node, and in fact this patch is setting
interrupt-parent from the host value just for the GIC node.

Looking through device trees I found arch/arm/boot/dts/tegra30.dtsi,
which shows exactly this issue:

        intc: interrupt-controller@50041000 {
                compatible = "arm,cortex-a9-gic";
                reg = <0x50041000 0x1000
                       0x50040100 0x0100>;
                interrupt-controller;
                #interrupt-cells = <3>;
                interrupt-parent = <&intc>;
        };

Did I get it right?
If so, please expand the commit message, to include this info. Also add
an in code comment to explain why you are setting interrupt-parent for
the gic node.

With these changes:

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


> Signed-off-by: Kyle Temkin <temkink@xxxxxxxxxxxx>
> ---
>  xen/arch/arm/domain_build.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 35ab08d..52c9a01 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -764,8 +764,8 @@ static int make_gic_node(const struct domain *d, void 
> *fdt,
>  {
>      const struct dt_device_node *gic = dt_interrupt_controller;
>      int res = 0;
> -    const void *addrcells, *sizecells;
> -    u32 addrcells_len, sizecells_len;
> +    const void *addrcells, *sizecells, *iparent;
> +    u32 addrcells_len, sizecells_len, iparent_len;
>  
>      /*
>       * Xen currently supports only a single GIC. Discard any secondary
> @@ -795,6 +795,14 @@ static int make_gic_node(const struct domain *d, void 
> *fdt,
>              return res;
>      }
>  
> +    iparent = dt_get_property(gic, "interrupt-parent", &iparent_len);
> +    if ( iparent )
> +    {
> +        res = fdt_property(fdt, "interrupt-parent", iparent, iparent_len);
> +        if ( res )
> +          return res;
> +    }
>
>      addrcells = dt_get_property(gic, "#address-cells", &addrcells_len);
>      if ( addrcells )
>      {

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