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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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.

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.

Looking briefly at the patch:
      * Can things like supporting larger address spaces not be
        upstreamed?
      * 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)?
      * 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)
      * etc

Ian.



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