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

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

[Linus Walleij]

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)
 {
 	struct rxe_mr *mr = to_rmr(ibmr);
-	struct page *page = virt_to_page(iova & mr->page_mask);
+	struct page *page = virt_to_page((void *)(iova & mr->page_mask));
 	bool persistent = !!(mr->access & IB_ACCESS_FLUSH_PERSISTENT);
 	int err;
 
@@ -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);
 		va = kmap_local_page(page);
@@ -488,7 +488,7 @@ int rxe_mr_do_atomic_op(struct rxe_mr *mr, u64 iova, int opcode,
 
 	if (mr->ibmr.type == IB_MR_TYPE_DMA) {
 		page_offset = iova & (PAGE_SIZE - 1);
-		page = virt_to_page(iova & PAGE_MASK);
+		page = virt_to_page((void *)(iova & PAGE_MASK));
 	} else {
 		unsigned long index;
 		int err;
@@ -545,7 +545,7 @@ int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, u64 value)
 
 	if (mr->ibmr.type == IB_MR_TYPE_DMA) {
 		page_offset = iova & (PAGE_SIZE - 1);
-		page = virt_to_page(iova & PAGE_MASK);
+		page = virt_to_page((void *)(iova & PAGE_MASK));
 	} else {
 		unsigned long index;
 		int err;
-- 
2.34.1

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

[kernel test robot]

Hi Linus,

I love your patch! Perhaps something to improve:

[auto build test WARNING on rdma/for-next]
[also build test WARNING on linus/master v6.3-rc3 next-20230324]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Linus-Walleij/RDMA-rxe-Pass-a-pointer-to-virt_to_page/20230324-183329
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git for-next
patch link:    https://lore.kernel.org/r/20230324103252.712107-1-linus.walleij%40linaro.org
patch subject: [PATCH] RDMA/rxe: Pass a pointer to virt_to_page()
config: parisc-allyesconfig (https://download.01.org/0day-ci/archive/20230324/202303242000.HmTaa6yB-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/f162d87e28eb5241d1a0e2085b80bfc70f626536
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Linus-Walleij/RDMA-rxe-Pass-a-pointer-to-virt_to_page/20230324-183329
        git checkout f162d87e28eb5241d1a0e2085b80bfc70f626536
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=parisc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=parisc SHELL=/bin/bash drivers/infiniband/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303242000.HmTaa6yB-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from arch/parisc/include/asm/page.h:181,
                    from include/linux/mm_types_task.h:16,
                    from include/linux/mm_types.h:5,
                    from include/linux/mmzone.h:22,
                    from include/linux/gfp.h:7,
                    from include/linux/xarray.h:15,
                    from include/linux/list_lru.h:14,
                    from include/linux/fs.h:13,
                    from include/linux/highmem.h:5,
                    from include/linux/bvec.h:10,
                    from include/linux/blk_types.h:10,
                    from include/linux/bio.h:10,
                    from include/linux/libnvdimm.h:14,
                    from drivers/infiniband/sw/rxe/rxe_mr.c:7:
   drivers/infiniband/sw/rxe/rxe_mr.c: In function 'rxe_set_page':
>> drivers/infiniband/sw/rxe/rxe_mr.c:216:42: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
     216 |         struct page *page = virt_to_page((void *)(iova & mr->page_mask));
         |                                          ^
   include/asm-generic/memory_model.h:18:46: note: in definition of macro '__pfn_to_page'
      18 | #define __pfn_to_page(pfn)      (mem_map + ((pfn) - ARCH_PFN_OFFSET))
         |                                              ^~~
   arch/parisc/include/asm/page.h:179:45: note: in expansion of macro '__pa'
     179 | #define virt_to_page(kaddr)     pfn_to_page(__pa(kaddr) >> PAGE_SHIFT)
         |                                             ^~~~
   drivers/infiniband/sw/rxe/rxe_mr.c:216:29: note: in expansion of macro 'virt_to_page'
     216 |         struct page *page = virt_to_page((void *)(iova & mr->page_mask));
         |                             ^~~~~~~~~~~~
   drivers/infiniband/sw/rxe/rxe_mr.c: In function 'rxe_mr_copy_dma':
   drivers/infiniband/sw/rxe/rxe_mr.c:291:37: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
     291 |                 page = virt_to_page((void *)(iova & mr->page_mask));
         |                                     ^
   include/asm-generic/memory_model.h:18:46: note: in definition of macro '__pfn_to_page'
      18 | #define __pfn_to_page(pfn)      (mem_map + ((pfn) - ARCH_PFN_OFFSET))
         |                                              ^~~
   arch/parisc/include/asm/page.h:179:45: note: in expansion of macro '__pa'
     179 | #define virt_to_page(kaddr)     pfn_to_page(__pa(kaddr) >> PAGE_SHIFT)
         |                                             ^~~~
   drivers/infiniband/sw/rxe/rxe_mr.c:291:24: note: in expansion of macro 'virt_to_page'
     291 |                 page = virt_to_page((void *)(iova & mr->page_mask));
         |                        ^~~~~~~~~~~~
   drivers/infiniband/sw/rxe/rxe_mr.c: In function 'rxe_mr_do_atomic_op':
   drivers/infiniband/sw/rxe/rxe_mr.c:491:37: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
     491 |                 page = virt_to_page((void *)(iova & PAGE_MASK));
         |                                     ^
   include/asm-generic/memory_model.h:18:46: note: in definition of macro '__pfn_to_page'
      18 | #define __pfn_to_page(pfn)      (mem_map + ((pfn) - ARCH_PFN_OFFSET))
         |                                              ^~~
   arch/parisc/include/asm/page.h:179:45: note: in expansion of macro '__pa'
     179 | #define virt_to_page(kaddr)     pfn_to_page(__pa(kaddr) >> PAGE_SHIFT)
         |                                             ^~~~
   drivers/infiniband/sw/rxe/rxe_mr.c:491:24: note: in expansion of macro 'virt_to_page'
     491 |                 page = virt_to_page((void *)(iova & PAGE_MASK));
         |                        ^~~~~~~~~~~~


vim +216 drivers/infiniband/sw/rxe/rxe_mr.c

   212	
   213	static int rxe_set_page(struct ib_mr *ibmr, u64 iova)
   214	{
   215		struct rxe_mr *mr = to_rmr(ibmr);
 > 216		struct page *page = virt_to_page((void *)(iova & mr->page_mask));
   217		bool persistent = !!(mr->access & IB_ACCESS_FLUSH_PERSISTENT);
   218		int err;
   219	
   220		if (persistent && !is_pmem_page(page)) {
   221			rxe_dbg_mr(mr, "Page cannot be persistent\n");
   222			return -EINVAL;
   223		}
   224	
   225		if (unlikely(mr->nbuf == mr->num_buf))
   226			return -ENOMEM;
   227	
   228		err = xa_err(xa_store(&mr->page_list, mr->nbuf, page, GFP_KERNEL));
   229		if (err)
   230			return err;
   231	
   232		mr->nbuf++;
   233		return 0;
   234	}
   235	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

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

[Jason Gunthorpe]

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.


> @@ -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);

> @@ -488,7 +488,7 @@ int rxe_mr_do_atomic_op(struct rxe_mr *mr, u64 iova, int opcode,
>  
>  	if (mr->ibmr.type == IB_MR_TYPE_DMA) {
>  		page_offset = iova & (PAGE_SIZE - 1);
> -		page = virt_to_page(iova & PAGE_MASK);
> +		page = virt_to_page((void *)(iova & PAGE_MASK));

These masks against PAGE_MASK are not needed, virt_to_page does them
already.

>  	} else {
>  		unsigned long index;
>  		int err;
> @@ -545,7 +545,7 @@ int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, u64 value)
>  
>  	if (mr->ibmr.type == IB_MR_TYPE_DMA) {
>  		page_offset = iova & (PAGE_SIZE - 1);
> -		page = virt_to_page(iova & PAGE_MASK);
> +		page = virt_to_page((void *)(iova & PAGE_MASK));

pfn_to_page here as well

Jason

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

[Linus Walleij]

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

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

[Bob Pearson]

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

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

[Jason Gunthorpe]

On Wed, Mar 29, 2023 at 04:28:08PM +0200, Linus Walleij wrote:

> 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?

The physical address dma type IB_MR_TYPE_DMA is rarely used and maybe
nobody ever tested it, at least in a configuration where kva != pa

Then again, maybe I got it wrong and it is still a kva in this case
because it is still ultimately DMA mapped when using IB_MR_TYPE_DMA?

Bob?

Jason

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

[Bob Pearson]

On 3/29/23 18:16, Jason Gunthorpe wrote:
> On Wed, Mar 29, 2023 at 04:28:08PM +0200, Linus Walleij wrote:
> 
>> 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?
> 
> The physical address dma type IB_MR_TYPE_DMA is rarely used and maybe
> nobody ever tested it, at least in a configuration where kva != pa
> 
> Then again, maybe I got it wrong and it is still a kva in this case
> because it is still ultimately DMA mapped when using IB_MR_TYPE_DMA?
> 
> Bob?
> 
> Jason

In the amd64 environment I use to dev and test there is no difference AFAIK.
I have to go on faith that other platforms work but have very little experience.

Bob

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

[Jason Gunthorpe]

On Wed, Mar 29, 2023 at 09:45:45PM -0500, Bob Pearson wrote:
> On 3/29/23 18:16, Jason Gunthorpe wrote:
> > On Wed, Mar 29, 2023 at 04:28:08PM +0200, Linus Walleij wrote:
> > 
> >> 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?
> > 
> > The physical address dma type IB_MR_TYPE_DMA is rarely used and maybe
> > nobody ever tested it, at least in a configuration where kva != pa
> > 
> > Then again, maybe I got it wrong and it is still a kva in this case
> > because it is still ultimately DMA mapped when using IB_MR_TYPE_DMA?
> > 
> > Bob?
> > 
> > Jason
> 
> In the amd64 environment I use to dev and test there is no difference AFAIK.
> I have to go on faith that other platforms work but have very little experience.

Honestly, I would be much happier if the virtual wrappers used
physical not kva :\ But that is too much to ask

So I suspsect I got it wrong and it is a kva still

Jason

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

[Linus Walleij]

On Thu, Mar 30, 2023 at 2:06 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> On Wed, Mar 29, 2023 at 09:45:45PM -0500, Bob Pearson wrote:
> > On 3/29/23 18:16, Jason Gunthorpe wrote:
> > > On Wed, Mar 29, 2023 at 04:28:08PM +0200, Linus Walleij wrote:
> > >
> > >> 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?
> > >
> > > The physical address dma type IB_MR_TYPE_DMA is rarely used and maybe
> > > nobody ever tested it, at least in a configuration where kva != pa
> > >
> > > Then again, maybe I got it wrong and it is still a kva in this case
> > > because it is still ultimately DMA mapped when using IB_MR_TYPE_DMA?
> > >
> > > Bob?
> > >
> > > Jason
> >
> > In the amd64 environment I use to dev and test there is no difference AFAIK.
> > I have to go on faith that other platforms work but have very little experience.
>
> Honestly, I would be much happier if the virtual wrappers used
> physical not kva :\ But that is too much to ask
>
> So I suspsect I got it wrong and it is a kva still

OK I'll respin the first version of the patch but with the quirks making it work
on parisc.

Yours,
Linus Walleij