RE: [openib-general] Re: [PATCH 21 of 53] ipath - use phys_to_virt instead of bus_to_virt

RE: [openib-general] Re: [PATCH 21 of 53] ipath - use phys_to_virt instead of bus_to_virt

[Caitlin Bestler]

openib-general-bounces@openib.org wrote:
>     Grant> Aren't remote addresses handled differently than local
>     Grant> ones?  ULP has to map local addresses.  We can't map remote
>     Grant> ones (remote host maps it).  The ULP must know the
>     Grant> difference and can tell the lower level driver which is   
> Grant> which. 
> 
> The problem is that RDMA requests have to be handled by the
> low-level driver (or hardware) without any ULP involvement.
> So every device has to handle getting messages like "send me
> XXX bytes of data from address YYY in the memory region
> corresponding to R_Key ZZZ."
> 

True, but how does that constrain the local interfaces by which
the driver is informed of the set of pages that back a given
memory region? The driver must still ultimately provide dma
accessible addresses to the device. RDMA just changes the
timing of the steps, albeit radically, but not what the
steps are.

Re: [openib-general] Re: [PATCH 21 of 53] ipath - use phys_to_virt instead of bus_to_virt

[Roland Dreier]

    Caitlin> True, but how does that constrain the local interfaces by
    Caitlin> which the driver is informed of the set of pages that
    Caitlin> back a given memory region? The driver must still
    Caitlin> ultimately provide dma accessible addresses to the
    Caitlin> device. RDMA just changes the timing of the steps, albeit
    Caitlin> radically, but not what the steps are.

It's only a problem for "reserved L_Key" types of things, where the
device is supposed to just use the address given in a work request
without translating it.  No translation means that work requests have
to contain "bus addresses" -- addresses that are what the device would
put on the bus to access memory.  But if a device needs to simulate
DMA in software, then it really needs a kernel virtual address, not a
bus address.  But it's pretty ugly to have to put that knowledge in
every consumer.

 - R.

[PATCH 21 of 53] ipath - use phys_to_virt instead of bus_to_virt

[Bryan O'Sullivan]

I think Roland already has this patch.

diff -r 201654fe1962 -r 4e0a07d20868 drivers/infiniband/hw/ipath/ipath_keys.c
--- a/drivers/infiniband/hw/ipath/ipath_keys.c	Fri May 12 15:55:28 2006 -0700
+++ b/drivers/infiniband/hw/ipath/ipath_keys.c	Fri May 12 15:55:28 2006 -0700
@@ -126,11 +126,11 @@ int ipath_lkey_ok(struct ipath_lkey_tabl
 	/*
 	 * We use LKEY == zero to mean a physical kmalloc() address.
 	 * This is a bit of a hack since we rely on dma_map_single()
-	 * being reversible by calling bus_to_virt().
+	 * being reversible by calling phys_to_virt().
 	 */
 	if (sge->lkey == 0) {
 		isge->mr = NULL;
-		isge->vaddr = bus_to_virt(sge->addr);
+		isge->vaddr = phys_to_virt(sge->addr);
 		isge->length = sge->length;
 		isge->sge_length = sge->length;
 		ret = 1;

Re: [PATCH 21 of 53] ipath - use phys_to_virt instead of bus_to_virt

[Roland Dreier]

 > I think Roland already has this patch.

 >  	 * This is a bit of a hack since we rely on dma_map_single()
 > -	 * being reversible by calling bus_to_virt().
 > +	 * being reversible by calling phys_to_virt().

Actually I NAK'ed this patch.  It compiles the same thing on x86_64
but makes the source code wrong -- dma_map_single() returns a bus
address, not a physical address.

 - R.

Re: [PATCH 21 of 53] ipath - use phys_to_virt instead of bus_to_virt

[Bryan O'Sullivan]

On Mon, 2006-05-15 at 08:50 -0700, Roland Dreier wrote:

> Actually I NAK'ed this patch.  It compiles the same thing on x86_64
> but makes the source code wrong -- dma_map_single() returns a bus
> address, not a physical address.

As Segher mentioned, bus_to_virt is unportable, so it's definitely the
wrong thing to use.

I don't recall what you suggested instead, but I seem to recall that the
discussion kind of went "oh, right, the layering is all broken".

Any ideas?  Should this turn from a one-liner into a
big-refactor-for-2.6.18 patch?

	<b

Re: [PATCH 21 of 53] ipath - use phys_to_virt instead of bus_to_virt

[Roland Dreier]

    Bryan> As Segher mentioned, bus_to_virt is unportable, so it's
    Bryan> definitely the wrong thing to use.

Yes, but at least it says what you're trying to do.  asm-powerpc's
io.h has this for phys_to_virt:

 *	This function does not handle bus mappings for DMA transfers. In
 *	almost all conceivable cases a device driver should not be using
 *	this function

so replacing bus_to_virt with that is not a step forward.

    Bryan> Any ideas?  Should this turn from a one-liner into a
    Bryan> big-refactor-for-2.6.18 patch?

I don't think there's a quick way to fix this.  What you really want
to do is override the DMA mapping functions for your device so that
you can keep track of the kernel mapping.  powerpc can already do this
(cf the ehca driver), and I think patches to do it on x86-64 are
floating around as part of the "Calgary IOMMU" work.

 - R.

Re: [openib-general] Re: [PATCH 21 of 53] ipath - use phys_to_virt instead of bus_to_virt

[Grant Grundler]

On Mon, May 15, 2006 at 02:28:45PM -0700, Roland Dreier wrote:
>     Bryan> Any ideas?  Should this turn from a one-liner into a
>     Bryan> big-refactor-for-2.6.18 patch?
> 
> I don't think there's a quick way to fix this.  What you really want
> to do is override the DMA mapping functions for your device so that
> you can keep track of the kernel mapping.

Or figure out which openib.org interface has to change so the
original virt addresses that were registered/handed to the ULP
are passed down to the low level interface driver too.
Seems like a more obvious way to fix the problem.
Someone did suggest this already, right?

> (cf the ehca driver), and I think patches to do it on x86-64 are
> floating around as part of the "Calgary IOMMU" work.

parisc has been using dma_ops for several years.
I don't expect dma_ops to become part of generic code.
DMA support is inherently arch specific.
Because of that, I don't look forward to a low level
device driver that is mucking with dma_ops.

hth,
grant

Re: [openib-general] Re: [PATCH 21 of 53] ipath - use phys_to_virt instead of bus_to_virt

[Roland Dreier]

    Grant> Or figure out which openib.org interface has to change so
    Grant> the original virt addresses that were registered/handed to
    Grant> the ULP are passed down to the low level interface driver
    Grant> too.  Seems like a more obvious way to fix the problem.
    Grant> Someone did suggest this already, right?

It's been suggested many times, but no one ever comes up with a way to
handle the fact that RDMA means that addresses come from remote
systems as well as being passed in through an API.

 - R.

Re: [openib-general] Re: [PATCH 21 of 53] ipath - use phys_to_virt instead of bus_to_virt

[Grant Grundler]

On Mon, May 15, 2006 at 04:16:45PM -0700, Roland Dreier wrote:
>     Grant> Or figure out which openib.org interface has to change so
>     Grant> the original virt addresses that were registered/handed to
>     Grant> the ULP are passed down to the low level interface driver
>     Grant> too.  Seems like a more obvious way to fix the problem.
>     Grant> Someone did suggest this already, right?
> 
> It's been suggested many times, but no one ever comes up with a way to
> handle the fact that RDMA means that addresses come from remote
> systems as well as being passed in through an API.

Aren't remote addresses handled differently than local ones?
ULP has to map local addresses.
We can't map remote ones (remote host maps it).
The ULP must know the difference and can tell the lower level
driver which is which.

Sorry, I hope my ignorance of RDMA isn't getting in the way again.

thanks,
grant

Re: [openib-general] Re: [PATCH 21 of 53] ipath - use phys_to_virt instead of bus_to_virt

[Roland Dreier]

    Grant> Aren't remote addresses handled differently than local
    Grant> ones?  ULP has to map local addresses.  We can't map remote
    Grant> ones (remote host maps it).  The ULP must know the
    Grant> difference and can tell the lower level driver which is
    Grant> which.

The problem is that RDMA requests have to be handled by the low-level
driver (or hardware) without any ULP involvement.  So every device has
to handle getting messages like "send me XXX bytes of data from
address YYY in the memory region corresponding to R_Key ZZZ."

 - R.