Re: [PATCH v2 12/12] xen/iommu: smmu: Add Xen specific code to be able to use the driver

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

On 28/01/15 10:31, Ian Campbell wrote:
> On Tue, 2015-01-27 at 17:34 +0000, Stefano Stabellini wrote:
>> On Tue, 27 Jan 2015, Julien Grall wrote:
>>> On 27/01/15 16:46, Stefano Stabellini wrote:
>>>> On Fri, 16 Jan 2015, Julien Grall wrote:
>>>>> The main goal is to modify as little the Linux code to be able to port
>>>>> easily new feature added in Linux repo for the driver.
>>>>
>>>> Agreed.
>>>>
>>>>
>>>>> To achieve that we:
>>>>>     - Add helpers to Linux function not implemented on Xen
>>>>
>>>> Good idea, I would take it further and move the helpers to a separate
>>>> file that smmu.c #includes. Or it could work the other way around: you
>>>> could move the original code to smmu-linux.c that get #included into
>>>> smmu.c
>>>
>>> We would have to modify smmu-linux.c for disabling some functions and/or
>>> modify them.
>>>
>>> Although for the later, there is only a couple functions modified.
>>>
>>>>
>>>>>     - Add callbacks used by Xen to do our own stuff and call Linux ones
>>>>>     - Only modify when required the code which comes from Linux. If so a
>>>>>     comment has been added with /* Xen: ... */ explaining why it's
>>>>>     necessary.
>>>>
>>>> I wonder if it makes sense to keep your changes in patch format and
>>>> apply the patch at run time as part of the make system.
>>>>
>>>> Of course you would need to be careful to keep the patched smmu.c
>>>> separate from the original, otherwise it would get very confusing when
>>>> people works on their changes.
>>>
>>> IHMO, it makes more difficult to review and work with it. Although it
>>> may be easier for porting change from Linux in the future.
>>
>> Yes, the reason to do that would be to make it as easy as possible to
>> update it in the future.  Ian, do you have an opinion on this?
> 
> Build time application of patches is a nightmare, we should definitely
> not do that.

I though about it during the night. It would be the best approach to
keep in sync with Linux more quickly.

It would also made clear what are our modifications and what was
upstreamed in Linux (assuming someone want to port/test our changes).

> IMHO the best way to approach this sort of thing is to separate it from
> the main body of imported code as much as possible, i.e. by patching in
> a minimal set of hooks the implementations of which can be segregated
> away from the imported code, even if that makes for a slightly less
> clean design overall.

It's already the case, if you look the code. The number of modifications
in the Linux code is very tiny compare to the number of changes (about
50 lines changes).

> Looking briefly at the patch:
>       * Can things like supporting larger address spaces not be
>         upstreamed?

I'm not sure which one you are talking about. But Linux is use separate
page table by device. Until now they were able to support only a certain
number of address bits.

I know they are reworking the SMMU drivers for 3.20. I haven't yet take
a look to it.


>       * Can we not avoid the #if 0 in many cases by just leaving them
>         and making it so the compiler can statically eliminate the call
>         (i.e. with expressions which expand to if(0)?

No, most of this code is self-contained in callbacks that we don't use.

>       * The fixups for differing configurations could be done all at
>         once in a xen hook called at the end of arm_smmu_device_reset
>         which applies our delta (overriding what the upstream code had
>         done)

No, because we may for a slight moment allow the device to bypass the
SMMU and therefore mess up the real memory.

>       * etc

I believe the most of my changes in the Linux code are justified.

There is a certain limit about using Linux drivers. They are moving fast
(at least on the ARM IOMMU sides). It will never be possible to have an
nearly exact (99%) match with Linux.

Regards,

-- 
Julien Grall