Re: [Xen-devel] Re: Where do we stand with the Xen patches?

[Ian Campbell <ijc@hellion.org.uk>]

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.

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

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

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.

Ian.

-- 
Ian Campbell
Current Noise: Isis - Altered Course

"For a male and female to live continuously together is...  biologically
speaking, an extremely unnatural condition."
		-- Robert Briffault