[ofa-general] [PATCH 2/3] RDMA/cxgb3: fix page shift calculation in build_phys_page_list()

[Bryan S Rosenburg]

On Mon Jan 21 12:39:36 PST 2008, Steve Wise wrote:
> RDMA/cxgb3: fix page shift calculation in build_phys_page_list()
> 
> The existing logic incorrectly maps this buffer list:
> 
> 0: addr 0x10001000, size 0x1000
> 1: addr 0x10002000, size 0x1000
> 
> To this bogus page list:
> 
> 0: 0x10000000
> 1: 0x10002000
> 
> The shift calculation must also take into account the address of the 
first
> entry masked by the page_mask as well as the last address+size rounded
> up to the next page size.

I think the problem can still occur, even with the patch, if the buffer 
list has just one entry.

A single entry (addr 0x10001000, size 0x2000) will get converted to page 
address 0x10000000 with a page size of 0x4000.  The patch as it stands 
doesn't address the single buffer case, but in fact it allows the 
subsequent single-buffer special case to be eliminated entirely.  Because 
the mask now includes the (page adjusted) starting and ending addresses, 
the general case works for the single buffer case as well:

================================================================================
diff --git a/drivers/infiniband/hw/cxgb3/iwch_mem.c 
b/drivers/infiniband/hw/cxgb3/iwch_mem.c
index 73bfd16..b8797c6 100644
--- a/drivers/infiniband/hw/cxgb3/iwch_mem.c
+++ b/drivers/infiniband/hw/cxgb3/iwch_mem.c
@@ -136,14 +136,8 @@ int build_phys_page_list(struct ib_phys_buf 
*buffer_list,
 
        /* Find largest page shift we can use to cover buffers */
        for (*shift = PAGE_SHIFT; *shift < 27; ++(*shift))
-               if (num_phys_buf > 1) {
-                       if ((1ULL << *shift) & mask)
-                               break;
-               } else
-                       if (1ULL << *shift >=
-                           buffer_list[0].size +
-                           (buffer_list[0].addr & ((1ULL << *shift) - 
1)))
-                               break;
+               if ((1ULL << *shift) & mask)
+                       break;
 
        buffer_list[0].size += buffer_list[0].addr & ((1ULL << *shift) - 
1);
        buffer_list[0].addr &= ~0ull << *shift;
================================================================================

Don't try this without applying Steve's patch first.

Incidentally, I've been tracking down exactly the bug that Steve fixed, 
but in mthca_reg_phys_mr() rather than in the cxgb3 
build_phys_page_list().  I'll submit a patch for mthca, unless someone 
else applies Steve's fix there soon.

- Bryan Rosenburg - IBM Research


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/general/attachments/20080125/84d274fd/attachment.html>