Re: [PATCH v5 21/22] xen/arm: ITS: Generate ITS node for Dom0

[Julien Grall <julien.grall@citrix.com>]

On 27/07/2015 04:12, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>
> Parse host dt and generate ITS node for Dom0.
> ITS node resides inside GIC node so when GIC node
> is encountered look for ITS node.
>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
> v5: - Moved ITS dt node generation to ITS driver
> v4: - Generate only one ITS node for Dom0
>      - Replace msi-parent references to single its phandle
> ---
>   xen/arch/arm/domain_build.c   |   17 ++++++++++
>   xen/arch/arm/gic-v3-its.c     |   74 +++++++++++++++++++++++++++++++++++++++++
>   xen/arch/arm/gic-v3.c         |   29 ++++++++++++++++
>   xen/arch/arm/gic.c            |   18 ++++++++++
>   xen/include/asm-arm/gic-its.h |    3 ++
>   xen/include/asm-arm/gic.h     |    7 ++++
>   6 files changed, 148 insertions(+)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 8556afd..6b6f013 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -469,6 +469,19 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
>               continue;
>           }
>
> +        /*
> +         * Replace all msi-parent phandle references to single ITS node
> +         * generated for Dom0
> +         */
> +        if ( dt_property_name_is_equal(prop, "msi-parent") )

I think this need more care than replacing every msi-parent without any 
checking.

You need to make sure that the msi-parent points to an ITS MSI just in 
case there is other possibility of MSI.

Furthermore, I would do this a ITS specific callback (gic_rewrite_node 
or smth similar) to avoid replacing msi-parent when it's not necessary. 
I have in mind GICv2M.

> +        {
> +            fdt32_t phandle = gic_get_msi_handle();
> +            DPRINT(" Set msi-parent(ITS) phandle 0x%x\n",phandle);
> +            fdt_property(kinfo->fdt, prop->name, (void *)&phandle,
> +                         sizeof(phandle));
> +            continue;
> +        }
> +
>           res = fdt_property(kinfo->fdt, prop->name, prop_data, prop_len);
>
>           xfree(new_data);
> @@ -875,6 +888,10 @@ static int make_gic_node(const struct domain *d, void *fdt,
>           return res;
>
>       res = fdt_end_node(fdt);
> +    if ( res )
> +        return res;
> +
> +    res = gic_its_hwdom_dt_node(d, node, fdt);

Can you explain why you didn't follow my suggestion to plumb the ITS 
node creation in gic_hwdow_dt_node? IHMO there is no need of new callback.

Furthermore the call is misplaced. You will end up to have a DT looking like

gic {
}

gic-its {
}

rather than

gic {
   gic-its {
   }
}

>       return res;
>   }
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 99f6edc..042c70d 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c

> +int its_make_dt_node(const struct domain *d,
> +                     const struct dt_device_node *node, void *fdt)
> +{
> +    struct its_node *its;
> +    const struct dt_device_node *gic;
> +    const void *compatible = NULL;
> +    u32 len;
> +    __be32 *new_cells, *tmp;
> +    int res = 0;
> +
> +    /* Will pass only first ITS node info */
> +    its = list_first_entry(&its_nodes, struct its_node, entry);
> +    if ( !its )
> +    {
> +        dprintk(XENLOG_ERR, "ITS node not found\n");
> +        return -FDT_ERR_XEN(ENOENT);
> +    }
> +
> +    gic = its->dt_node;
> +
> +    compatible = dt_get_property(gic, "compatible", &len);
> +    if ( !compatible )
> +    {
> +        dprintk(XENLOG_ERR, "Can't find compatible property for the its node\n");
> +        return -FDT_ERR_XEN(ENOENT);
> +    }
> +
> +    res = fdt_begin_node(fdt, "gic-its");
> +    if ( res )
> +        return res;
> +
> +    res = fdt_property(fdt, "compatible", compatible, len);
> +    if ( res )
> +        return res;
> +
> +    res = fdt_property(fdt, "msi-controller", NULL, 0);
> +    if ( res )
> +        return res;
> +
> +    len = dt_cells_to_size(dt_n_addr_cells(node) + dt_n_size_cells(node));
> +
> +    new_cells = xzalloc_bytes(len);
> +    if ( new_cells == NULL )
> +        return -FDT_ERR_XEN(ENOMEM);
> +    tmp = new_cells;
> +
> +    dt_set_range(&tmp, node, its->phys_base, its->phys_size);
> +
> +    res = fdt_property(fdt, "reg", new_cells, len);
> +    xfree(new_cells);
> +
> +    if ( node->phandle )
> +    {
> +        res = fdt_property_cell(fdt, "phandle", node->phandle);
> +        if ( res )
> +            return res;
> +
> +        its_phandle = cpu_to_fdt32(node->phandle);

Why this is set in make_hwdom_dt_node? The ITS phandle may be used 
before we effectively parse the GIC node.

> +    }
> +
> +    res = fdt_end_node(fdt);
> +
> +    return res;
> +}
> +
> +
> +fdt32_t its_get_lpi_handle(void)
> +{
> +    return its_phandle;
> +}
> +
>   static int its_force_quiescent(void __iomem *base)
>   {
>       u32 count = 1000000;   /* 1s */
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 23eb47c..828bf27 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1143,6 +1143,34 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
>       return res;
>   }
>
> +static int gicv3_make_hwdom_its_dt_node(const struct domain *d,
> +                                        const struct dt_device_node *node,
> +                                        void *fdt)
> +{
> +    struct dt_device_node *gic_child;
> +    int res = 0;
> +
> +    static const struct dt_device_match its_matches[] __initconst =
> +    {
> +        DT_MATCH_GIC_ITS,
> +        { /* sentinel */ },
> +    };
> +
> +    dt_for_each_child_node(node, gic_child)
> +    {
> +        if ( gic_child != NULL )
> +        {
> +            if ( dt_match_node(its_matches, gic_child) )
> +            {
> +                res = its_make_dt_node(d, gic_child, fdt);
> +                break;
> +            }
> +        }
> +    }

I already asked on v4, why do you need this loop? The GIC node for the 
DOM0 is recreating from scratch, there is no need of looping on the 
current GIC node...


> +
> +    return res;
> +}
> +
>   static const hw_irq_controller gicv3_host_irq_type = {
>       .typename     = "gic-v3",
>       .startup      = gicv3_irq_startup,
> @@ -1372,6 +1400,7 @@ static const struct gic_hw_operations gicv3_ops = {
>       .read_apr            = gicv3_read_apr,
>       .secondary_init      = gicv3_secondary_cpu_init,
>       .make_hwdom_dt_node  = gicv3_make_hwdom_dt_node,
> +    .make_hwdom_its_dt_node  = gicv3_make_hwdom_its_dt_node,
>       .is_lpi              = gicv3_is_lpi,
>   };
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index f6be0e9..88c1427 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -753,6 +753,15 @@ void __cpuinit init_maintenance_interrupt(void)
>                   "irq-maintenance", NULL);
>   }
>
> +fdt32_t gic_get_msi_handle(void)
> +{
> +#ifdef HAS_GICV3
> +    if ( gic_lpi_supported() )
> +        return its_get_lpi_handle();
> +#endif

If you need this introduce a new callback but don't add #ifdef in the 
common code unless you have a strong argument.

> +    return 0;
> +}
> +

[..]

Regards,

-- 
Julien Grall