Re: [PATCH] RDMA/rxe: Pass a pointer to virt_to_page()

[Bob Pearson <rpearsonhpe@gmail.com>]

On 3/29/23 09:28, Linus Walleij wrote:
> On Fri, Mar 24, 2023 at 3:00 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>> On Fri, Mar 24, 2023 at 11:32:52AM +0100, Linus Walleij wrote:
>>> Like the other calls in this function virt_to_page() expects
>>> a pointer, not an integer.
>>>
>>> However since many architectures implement virt_to_pfn() as
>>> a macro, this function becomes polymorphic and accepts both a
>>> (unsigned long) and a (void *).
>>>
>>> Fix this up with an explicit cast.
>>>
>>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>>> ---
>>>  drivers/infiniband/sw/rxe/rxe_mr.c | 8 ++++----
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
>>> index b10aa1580a64..5c90d83002f0 100644
>>> --- a/drivers/infiniband/sw/rxe/rxe_mr.c
>>> +++ b/drivers/infiniband/sw/rxe/rxe_mr.c
>>> @@ -213,7 +213,7 @@ int rxe_mr_init_fast(int max_pages, struct rxe_mr *mr)
>>>  static int rxe_set_page(struct ib_mr *ibmr, u64 iova)
>>>  {
>>
>> All these functions have the wrong names, they are kva not IOVA.
> 
> This escalated quickly. :D
> 
> I'm trying to do the right thing, I try to fix the technical issues first,
> and I can follow up with a rename patch if you want.
> 
>>> @@ -288,7 +288,7 @@ static void rxe_mr_copy_dma(struct rxe_mr *mr, u64 iova, void *addr,
>>>       u8 *va;
>>
>>>       while (length) {
>>> -             page = virt_to_page(iova & mr->page_mask);
>>> +             page = virt_to_page((void *)(iova & mr->page_mask));
>>>               bytes = min_t(unsigned int, length,
>>>                               PAGE_SIZE - page_offset);
>>
>> This is actually a bug, this function is only called on IB_MR_TYPE_DMA
>> and in that case 'iova' is actually a phys addr
>>
>> So iova should be called phys and the above should be:
>>
>>                 page = pfn_to_page(phys >> PAGE_SHIFT);
> 
> I tried to make a patch fixing all of these up and prepended to v2 of this
> patch (which also needed adjustments).
> 
> This is a bit tricky so bear with me, also I have no idea how to test this
> so hoping for some help there.
> 
> I'm a bit puzzled: could the above code (which exist in
> three instances in the driver) even work as it is? Or is it not used?
> Or is there some failover from DMA to something else that is constantly
> happening?
> 
> Yours,
> Linus Walleij

The driver is a software only emulation of an RDMA device. So there isn't any 'real' DMA going on
just a bunch of memcpy()s.

Bob