Bryan, I assume you sign-off on this patch?
Bryan S Rosenburg wrote:
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
_______________________________________________
general mailing list
[email protected]
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
