Re: [RFC PATCH v2 09/22] xen/arm: its: Add helper functions to decode ITS Command

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

Hi,

On 11/05/2015 15:36, Vijay Kilari wrote:
> On Mon, May 11, 2015 at 7:55 PM, Julien Grall <julien.grall@citrix.com> wrote:
>> On 11/05/15 15:25, Julien Grall wrote:
>>> Hi Vijay,
>>>
>>> On 11/05/15 15:14, Vijay Kilari wrote:
>>>> On Wed, Apr 1, 2015 at 5:10 PM, Ian Campbell <ian.campbell@citrix.com> wrote:
>>>>> On Thu, 2015-03-19 at 20:07 +0530, vijay.kilari@gmail.com wrote:
>>>>>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>>>>>
>>>>>> Add helper functions to decode ITS command
>>>>>> This will be useful for Virtual ITS driver
>>>>>
>>>>> It depends slightly on the answer to the quesiton I asked on patch #6,
>>>>> but in general in Xen we have preferred to define a structure/union
>>>>> overlaying the processor's view of such things and to use access to
>>>>> those fields, see e.g. the hsr decode or the pte stuff.
>>>>
>>>>     This make heavy and simple changes. I prefer to make it a separate
>>>> patch series.
>>>
>>> We are at early stage of the review process, it's the second version and
>>> the design is not yet set in stone. So it's normal to have heavy changes
>>> in the code between 2 versions.
>>>
>>> Although, when you say heavy, it's compare to what? The GICv3 ITS driver?
>>
>> * from Linux?
>
> Yes.

I'm joining Ian's concern on a previous mail [1]. The diff between the 
Linux driver and the Xen driver is very huge. Removing unnecessary code, 
moving code in another file... doesn't help for porting fixes for Linux. 
How would you know if you remove/move the function for good reasons? 
What would you do if you have to bring back a function?

Your previous mail saying "This make heavy and simple changes. I prefer 
to make it a separate patch series." achieved to convince me that you 
don't use the GICv3 ITS driver from Linux for helping backporting fixes 
later (even you if you stated the invert on multiple mails [2], [3]...).

To help backporting, the driver needs to have very small changes. 
Removing/moving code is even too much, the resulting patch won't be 
similar to the Linux one and it will be harder to know whether it's 
valid or not.

When I worked on the SMMU drivers last year, I choose to differ from 
Linux because the page table wasn't shared. The base was from Linux but 
heavily change. The code went in, but few months after we notice it was 
hard to maintain and the API is fairly similar to Linux. So we decided 
to get a driver very close to Linux.

Now for the GICv3 ITS driver... the final design of the ITS in Xen will 
be very different to the Linux one:
	- Command are generated by the vITS and not the ITS driver
         - The Linux ITS is waiting the completion of each command 
before continuing while it's not possible for Xen
         - and so on...

TBH, with these reasons, I don't see how it will be possible to keep the 
Xen drivers synced...

If your driver is not sync to the latest version of Linux (which I guess 
it is). Give a try to take the patches and backport one by one on the 
top of your series. You will be able to see how difficult it is to 
backport on a diverging driver.

Regards,


[1] 
http://lists.xenproject.org/archives/html/xen-devel/2015-04/msg00235.html

[2] 
http://lists.xenproject.org/archives/html/xen-devel/2015-04/msg00201.html

[3] 
http://lists.xenproject.org/archives/html/xen-devel/2015-04/msg00246.html



> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

-- 
Julien Grall