Message-Id: <20090522205533P.fujita.tomonori@lab.ntt.co.jp>
Date:	Fri, 22 May 2009 20:55:22 +0900
From:	FUJITA Tomonori <fujita.tomonori@....ntt.co.jp>
To:	Ian.Campbell@...citrix.com, mingo@...e.hu
Cc:	fujita.tomonori@....ntt.co.jp, jeremy@...p.org,
	beckyb@...nel.crashing.org, okir@...e.de, gregkh@...e.de,
	xendevel@...ts.xensource.com, x86@...nel.org,
	linux-kernel@...r.kernel.org
Subject: Re: swiotlb: remove __weak hooks in favour of
	architecture-specific functions

On Fri, 22 May 2009 12:43:16 +0100
Ian Campbell <Ian.Campbell@...citrix.com> wrote:

> On Fri, 2009-05-22 at 07:13 -0400, FUJITA Tomonori wrote:
> > On Thu, 21 May 2009 17:15:21 +0100
> > Ian Campbell <ian.campbell@...rix.com> wrote:
> > Please go with the following way (that I posted yesterday):
> > 
> > http://marc.info/?l=xen-devel&m=124292666214380&w=2
> > 
> > 
> > Export the core feature of swiotlb, managing iotlb buffer and
> > implement the Xen mapping functions.
> 
> I feel that should be a last resort, before we go down that path we
> should try and find a way for us to use the generic code in a clean way
> which makes everyone happy.
>
> We have had several attempts at this and admittedly have not yet come up
> with something that satisfies everyone but I don't really think we have
> gotten to the point of admitting defeat and just duplicating the code.

There should not be much duplication.


> I think the proposal to use a dma_map_range-like function which I sent a
> few minutes ago I think gets us closer to something which satisfies
> everyone's requirements, including yours for a clean abstraction.

Seems you don't understand the point; with dom0, we can't cleanly use
arch/*/include/asm/.

You need to insert Xen's hook like this:


+++ b/arch/x86/include/asm/dma-mapping.h
@@ -309,4 +309,20 @@ static inline void dma_free_coherent(struct device *dev, size_t size,
 		ops->free_coherent(dev, size, vaddr, bus);
 }
 
+static inline dma_addr_t dma_map_range(struct device *dev, u64 mask,
+				       phys_addr_t addr, size_t size)
+{
+	dma_addr_t dma_addr;
+#ifdef CONFIG_XEN
+	extern int xen_range_needs_mapping(phys_addr_t paddr, size_t size);
+	if (xen_pv_domain() && xen_range_needs_mapping(addr, size))
+		return 0;
+#endif
+
+	dma_addr = swiotlb_phys_to_bus(dev, addr);
+	if (dma_addr + size <= mask)
+		return 0;
+	return dma_addr;
+}

Or you need to use a function pointer in a strange way.

--- a/arch/x86/include/asm/dma-mapping.h
+++ b/arch/x86/include/asm/dma-mapping.h
@@ -315,4 +315,13 @@ static inline int is_buffer_dma_capable(struct device *dev, u64 mask,
 	return addr + size <= mask;
 }
 
+extern int (*x86_swiotlb_force_mapping)(phys_addr_t paddr, size_t size);
+
+static inline int swiotlb_force_mapping(phys_addr_t paddr, size_t size)
+{
+	if (x86_swiotlb_force_mapping)
+		return x86_swiotlb_force_mapping(paddr, size);
+	return 0;
+}
+

Or you could invent something more.


As you said in another mail, it's up to the x86 maintainer :

> This case is internal to the x86 arch code and I'd really like to hear
> the x86 maintainer's opinion of the general approach.


But the above code looks really ugly to me.


> >  With that approach, there is not
> > much code duplication and there is no need for ugly hooks for dom0;
> > the phys/bus address conversion and address checking.
> 
> The phys/bus address conversion is also needed for powerpc.
> 
> I think the two address checking functions can be collapsed into a
> single one which satisfies the needs of both Xen and powerpc.
> 
> What dom0 specific "ugly" hooks does that leave? The alloc one? I've
> discussed that in another mail.

See above. POWERPC can use arch/*/include/asm/ cleanly for the
phys/bus address conversion while dom0 can't. That's what I said again
and again.
--
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/

Powered by blists - more mailing lists