Re: [RFC PATCH 00/19] xen/arm: Add ITS support

[Julien Grall <julien.grall@linaro.org>]

Hello Vijay,

On 03/03/2015 03:55, Vijay Kilari wrote:
> On Mon, Mar 2, 2015 at 6:49 PM, Julien Grall <julien.grall@linaro.org> wrote:
>> On 02/03/15 12:30, vijay.kilari@gmail.com wrote:
>>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>>
>>> Add ITS support for arm. Following major features
>>> are supported
>>>   - GICv3 ITS support for arm64 platform
>>>   - Supports only single ITS node
>>
>> Why only one ITS node supported? I though Cavium was using 2 ITS...
>
> I will update for 2 ITS nodes later when NUMA is supported

Why do you speak about NUMA? AFAICT, there is no requirement to support 
NUMA for having multiple ITS...

With multiple ITS support, your vITS emulation will likely heavily 
change. So it would be nice to have this support as soon as possible.

>>
>>>   - LPI descriptors are allocated on-demand
>>>   - Only ITS Dom0 is supported
>>
>> Any plan to support guest?
>
> Yes, I will do it in next version

What is missing to support guest? Only the toolstack part?

>>
>>> Vijaya Kumar K (19):
>>>    xen/arm: add linked list apis
>>>    xen/arm: its: Import GICv3 ITS driver from linux
>>>    xen/arm: its: Port ITS driver to xen
>>
>> A general comment (I haven't read closely the patches). The GICv3 ITS
>> taken from Linux is modified so heavily (rename function, move out code,
>> dropping unused code...) that your assumption in patch #1 [1] is wrong.
>>
>> At the end of this series it would make impossible to backport patch
>> from Linux.
>
>    Most of the code is reused or moved to different file based on Xen
> requirement.
>      - code like msi registered callback (setup_irq & teardown_irq) is of
> no use in Xen. So removed
>      - irq_chip is different in linux
>      - some of the functions like encode of ITS commands can be used in
> virtual ITS
>        driver as well. So have to be moved out to header file
>      - the LPI allocation is moved to virtual ITS driver. We can
> consider it keeping in physical ITS
>        driver but it fits well in virtual ITS driver rather than
> physical ITS driver.

IHMO, moving the code around two files make the code more difficult to 
review because the patch is bigger.

That also means we can't really review the first couple of patches 
because the code will change a lot after.

But the maintainers may be disagree with me...

>>
>> Regards,
>>
>> [1] "This is actual GICv3 ITS driver from Linux. [..] This helps to
>> import any issues found in Linux"
>
> I have kept the linux GICv3 ITS driver in first patch and made incremental
> changes just to have better understanding and incremental approach

But this is doesn't help to backport issue from Linux...

BTW, do you have a tree with all the code?

Regards,

-- 
Julien Grall