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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 28 Jan 2015, Julien Grall wrote:
> 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).

I agree.  Anybody could upgrade the smmu driver at any time, if we did
it that way.

I don't think that it would be a nightmare, if we think carefully about
the changes to the build system and we make sure that the patched driver
is updated appropriately.

In fact in xen-unstable we already do this with the current stubdom
patches. The stubdom build system is certainly not a great example to
follow, but it is not the patching that causes problems.

Of course the patch (if any) should be a small as possible following the
same principles that Ian wrote below.


> > 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
> 

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