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

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

On 21/08/2015 16:02, Vijay Kilari wrote:
> On Mon, Aug 17, 2015 at 12:41 PM, Julien Grall <julien.grall@citrix.com> wrote:
>>
>>
>> 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(+)
>>>
> [...]
>>> +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...
>
>    It ensures that ITS node for Dom0 is generated only if dt_host has it.

That's wrong. The ITS node maybe be present in the DT but the ITS driver 
never initialized. Furthermore, we may decide at some point to not 
exposed the ITS to DOM0.

You should introduced a variable to know whether the ITS is used for 
DOM0 or not and not relying on the hw DT. This is pointless and make the 
code harder to read.

Regards,

-- 
Julien Grall