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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20090521201951I.fujita.tomonori@lab.ntt.co.jp>
Date:	Thu, 21 May 2009 20:19:46 +0900
From:	FUJITA Tomonori <fujita.tomonori@....ntt.co.jp>
To:	ijc@...lion.org.uk
Cc:	fujita.tomonori@....ntt.co.jp, jeremy@...p.org,
	xen-devel@...ts.xensource.com, beckyb@...nel.crashing.org,
	okir@...e.de, x86@...nel.org, linux-kernel@...r.kernel.org,
	mingo@...e.hu, gregkh@...e.de
Subject: Re: [Xen-devel] Re: Where do we stand with the Xen patches?

On Thu, 21 May 2009 12:03:05 +0100
Ian Campbell <ijc@...lion.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@...lion.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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/