lkml.org 
[lkml]   [2009]   [May]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [Xen-devel] Re: Where do we stand with the Xen patches?
    From
    On Thu, 21 May 2009 12:03:05 +0100
    Ian Campbell <ijc@hellion.org.uk> wrote:

    > On Thu, 2009-05-21 at 06:39 -0400, FUJITA Tomonori wrote:
    > > On Thu, 21 May 2009 11:28:53 +0100
    > > Ian Campbell <ijc@hellion.org.uk> wrote:
    > >
    > > > +#ifdef CONFIG_PCI_XEN
    > > > +extern int xen_range_needs_mapping(phys_addr_t paddr, size_t size);
    > > > +#else
    > > > +static inline int xen_range_needs_mapping(phys_addr_t paddr, size_t size) { return 0; }
    > > > +#endif
    > >
    > > I know Xen can do something like this but you think that this is
    > > clean?
    >
    > Well, defining a static inline function when a CONFIG option is disabled
    > is fairly idiomatic in the kernel and in general hiding these sorts of
    > things in the headers in this way is preferred to having them in .c
    > files. See e.g. the handling of CONFIG_PRINTK in include/linux/kernel.h
    > or CONFIG_HIGHMEM in include/linux/highmem.h for just two examples out
    > of many.

    Well, I know that it's idiomatic, but placing CONFIG_PCI_XEN in
    arch/{x86|ia64}/include/asm/ is a wrong abstraction to me.


    > > In addition, you also the similar hack in
    > > arch/ia64/include/asm/dma-mapping.h for ia64's dom0 support, I think.
    > >
    > > IMO, your patch just moves the ugly hacks from lib/swiotlb.c to
    > > arch/{x86|ia64}/include/asm/dma-mapping.h.
    >
    > I nearly suggested that for this hook it might actually be preferable to
    > put the one line Xen hook directly into swiotlb.c. I didn't think this
    > suggestion would go down very well though.

    Any arch or Xen specific code should not live in swiotlb.c


    > In any case something along these lines needs to go somewhere. I think
    > you are slightly mischaracterising this as an "ugly hack" -- it is a
    > necessary interface to enable a particular use-case, and it actually has
    > a very small cross section (it's basically five or six lines of code).

    A necessary interface? Sorry, I don't buy it. It's necessary for
    only Xen. And it's not fit well for swiotlb and the architecture
    abstraction.


    > If there was a cleaner way to achieve the same result we would of course
    > go with that. I don't think duplicating swiotlb.c, as has been suggested
    > as the alternative, just for that one hook point makes sense.

    One hook? You need to reread swiotlb.c. There are more. All should go.


    \
     
     \ /
      Last update: 2009-05-21 13:23    [W:0.025 / U:0.100 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site