Re: Re: Re: Re: Re: Re: [PATCH] RDMA/siw: Fix compiler warnings on 32-bit due to u64/pointer abuse
On Mon, Aug 19, 2019 at 09:40:10PM +, Bernard Metzler wrote: > >It is either an iova & lkey pair, or SGE information is inlined into > >the WR ring. > > > In siw, the reference to any type of memory is kept uninterpreted > in the send/receive queue until it gets accessed by a data > transfer. The information on what type of memory is being referenced > is deducted from the local memory key. As said, this step is > being executed only when the actual buffer is to be touched. > All it needs before that translation is to keep the 32bit key + > length and the up to 64bit address in a work queue element within > the send queue. > lkey lookup and memory translation + access validation happens > after the work queue element left the send/receive queue and a > local copy of it is being processed by the kernel driver > during RX or TX operations. > > Inline data is implemented similar to how HW providers do > it - user data are copied immediately into the WR array. I still don't understand how kernel void *'s are getting into WQEs. Jason
Re: Re: Re: Re: Re: Re: [PATCH] RDMA/siw: Fix compiler warnings on 32-bit due to u64/pointer abuse
-"Jason Gunthorpe" wrote: - >To: "Bernard Metzler" >From: "Jason Gunthorpe" >Date: 08/19/2019 08:00PM >Cc: "Geert Uytterhoeven" , "Doug Ledford" >, linux-r...@vger.kernel.org, >linux-kernel@vger.kernel.org >Subject: [EXTERNAL] Re: Re: Re: Re: Re: [PATCH] RDMA/siw: Fix >compiler warnings on 32-bit due to u64/pointer abuse > >On Mon, Aug 19, 2019 at 05:39:04PM +, Bernard Metzler wrote: >> >> >To: "Bernard Metzler" >> >From: "Jason Gunthorpe" >> >Date: 08/19/2019 06:35PM >> >Cc: "Geert Uytterhoeven" , "Doug Ledford" >> >, linux-r...@vger.kernel.org, >> >linux-kernel@vger.kernel.org >> >Subject: [EXTERNAL] Re: Re: Re: Re: Re: [PATCH] RDMA/siw: Fix >> >compiler warnings on 32-bit due to u64/pointer abuse >> > >> >On Mon, Aug 19, 2019 at 04:29:11PM +, Bernard Metzler wrote: >> >> >> >> >To: "Bernard Metzler" >> >> >From: "Jason Gunthorpe" >> >> >Date: 08/19/2019 06:05PM >> >> >Cc: "Geert Uytterhoeven" , "Doug Ledford" >> >> >, linux-r...@vger.kernel.org, >> >> >linux-kernel@vger.kernel.org >> >> >Subject: [EXTERNAL] Re: Re: Re: Re: [PATCH] RDMA/siw: Fix >compiler >> >> >warnings on 32-bit due to u64/pointer abuse >> >> > >> >> >On Mon, Aug 19, 2019 at 03:54:56PM +, Bernard Metzler >wrote: >> >> > >> >> >> Absolutely. But these addresses are conveyed through the >> >> >> API as unsigned 64 during post_send(), and land in the siw >> >> >> send queue as is. During send queue processing, these >addresses >> >> >> must be interpreted according to its context and transformed >> >> >> (casted) back to the callers intention. I frankly do not >> >> >> know what we can do differently... The representation of >> >> >> all addresses as unsigned 64 is given. Sorry for the >confusion. >> >> > >> >> >send work does not have pointers in it, so I'm confused what >this >> >is >> >> >about. Does siw allow userspace to stick an ordinary pointer >for >> >the >> >> >SG list? >> >> >> >> Right a user references a buffer by address and local key it >> >> got during reservation of that buffer. The user can provide any >> >> VA between start of that buffer and registered length. >> > >> >Oh gross, it overloads the IOVA in the WR with a kernel void * ?? >> >> Oh no. The user library writes the buffer address into >> the 64bit address field of the WR. This is nothing siw >> has invented. > >No HW provider sticks pointers into the WR ring. Now siw is a SW only provider. It sits on top of TCP kernel sockets. siw translates any local application buffer reference it gets back into a kvec or page pointer (transmit from), or a virtual address (receive into). This is what the TCP interface wants. In fact, siw cares about physical addresses only since the RDMA (kernel level) user may care about it. It translates those back into something the TCP interface can consume. > >It is either an iova & lkey pair, or SGE information is inlined into >the WR ring. > In siw, the reference to any type of memory is kept uninterpreted in the send/receive queue until it gets accessed by a data transfer. The information on what type of memory is being referenced is deducted from the local memory key. As said, this step is being executed only when the actual buffer is to be touched. All it needs before that translation is to keep the 32bit key + length and the up to 64bit address in a work queue element within the send queue. lkey lookup and memory translation + access validation happens after the work queue element left the send/receive queue and a local copy of it is being processed by the kernel driver during RX or TX operations. Inline data is implemented similar to how HW providers do it - user data are copied immediately into the WR array. >Never, ever, a user or kernel pointer. > >The closest we get to a kernel pointer is with the local dma lkey & >iova == physical memory address. > >> >Why does siw_pbl_get_buffer not return a void *?? >> >> >> I think, in fact, it should be dma_addr_t, since this is >> what PBL's are described with. Makes sense? > >You mean because siw uses dma_virt_ops and can translate a dma_addr_t >back to a pfn? Yes, that would make alot more sense. > >If all conversions went explicitly from a iova & lkey -> dma_addr_t >-> void * in >the kmap then I'd be a lot happier > >Jason > >
Re: Re: Re: Re: Re: [PATCH] RDMA/siw: Fix compiler warnings on 32-bit due to u64/pointer abuse
On Mon, Aug 19, 2019 at 05:39:04PM +, Bernard Metzler wrote: > > >To: "Bernard Metzler" > >From: "Jason Gunthorpe" > >Date: 08/19/2019 06:35PM > >Cc: "Geert Uytterhoeven" , "Doug Ledford" > >, linux-r...@vger.kernel.org, > >linux-kernel@vger.kernel.org > >Subject: [EXTERNAL] Re: Re: Re: Re: Re: [PATCH] RDMA/siw: Fix > >compiler warnings on 32-bit due to u64/pointer abuse > > > >On Mon, Aug 19, 2019 at 04:29:11PM +, Bernard Metzler wrote: > >> > >> >To: "Bernard Metzler" > >> >From: "Jason Gunthorpe" > >> >Date: 08/19/2019 06:05PM > >> >Cc: "Geert Uytterhoeven" , "Doug Ledford" > >> >, linux-r...@vger.kernel.org, > >> >linux-kernel@vger.kernel.org > >> >Subject: [EXTERNAL] Re: Re: Re: Re: [PATCH] RDMA/siw: Fix compiler > >> >warnings on 32-bit due to u64/pointer abuse > >> > > >> >On Mon, Aug 19, 2019 at 03:54:56PM +, Bernard Metzler wrote: > >> > > >> >> Absolutely. But these addresses are conveyed through the > >> >> API as unsigned 64 during post_send(), and land in the siw > >> >> send queue as is. During send queue processing, these addresses > >> >> must be interpreted according to its context and transformed > >> >> (casted) back to the callers intention. I frankly do not > >> >> know what we can do differently... The representation of > >> >> all addresses as unsigned 64 is given. Sorry for the confusion. > >> > > >> >send work does not have pointers in it, so I'm confused what this > >is > >> >about. Does siw allow userspace to stick an ordinary pointer for > >the > >> >SG list? > >> > >> Right a user references a buffer by address and local key it > >> got during reservation of that buffer. The user can provide any > >> VA between start of that buffer and registered length. > > > >Oh gross, it overloads the IOVA in the WR with a kernel void * ?? > > Oh no. The user library writes the buffer address into > the 64bit address field of the WR. This is nothing siw > has invented. No HW provider sticks pointers into the WR ring. It is either an iova & lkey pair, or SGE information is inlined into the WR ring. Never, ever, a user or kernel pointer. The closest we get to a kernel pointer is with the local dma lkey & iova == physical memory address. > >Why does siw_pbl_get_buffer not return a void *?? > > > I think, in fact, it should be dma_addr_t, since this is > what PBL's are described with. Makes sense? You mean because siw uses dma_virt_ops and can translate a dma_addr_t back to a pfn? Yes, that would make alot more sense. If all conversions went explicitly from a iova & lkey -> dma_addr_t -> void * in the kmap then I'd be a lot happier Jason
RE: Re: Re: Re: Re: [PATCH] RDMA/siw: Fix compiler warnings on 32-bit due to u64/pointer abuse
-"Jason Gunthorpe" wrote: - >To: "Bernard Metzler" >From: "Jason Gunthorpe" >Date: 08/19/2019 06:35PM >Cc: "Geert Uytterhoeven" , "Doug Ledford" >, linux-r...@vger.kernel.org, >linux-kernel@vger.kernel.org >Subject: [EXTERNAL] Re: Re: Re: Re: Re: [PATCH] RDMA/siw: Fix >compiler warnings on 32-bit due to u64/pointer abuse > >On Mon, Aug 19, 2019 at 04:29:11PM +, Bernard Metzler wrote: >> >> >To: "Bernard Metzler" >> >From: "Jason Gunthorpe" >> >Date: 08/19/2019 06:05PM >> >Cc: "Geert Uytterhoeven" , "Doug Ledford" >> >, linux-r...@vger.kernel.org, >> >linux-kernel@vger.kernel.org >> >Subject: [EXTERNAL] Re: Re: Re: Re: [PATCH] RDMA/siw: Fix compiler >> >warnings on 32-bit due to u64/pointer abuse >> > >> >On Mon, Aug 19, 2019 at 03:54:56PM +, Bernard Metzler wrote: >> > >> >> Absolutely. But these addresses are conveyed through the >> >> API as unsigned 64 during post_send(), and land in the siw >> >> send queue as is. During send queue processing, these addresses >> >> must be interpreted according to its context and transformed >> >> (casted) back to the callers intention. I frankly do not >> >> know what we can do differently... The representation of >> >> all addresses as unsigned 64 is given. Sorry for the confusion. >> > >> >send work does not have pointers in it, so I'm confused what this >is >> >about. Does siw allow userspace to stick an ordinary pointer for >the >> >SG list? >> >> Right a user references a buffer by address and local key it >> got during reservation of that buffer. The user can provide any >> VA between start of that buffer and registered length. > >Oh gross, it overloads the IOVA in the WR with a kernel void * ?? Oh no. The user library writes the buffer address into the 64bit address field of the WR. This is nothing siw has invented. > >> >The code paths here must be totally different, so there should be >> >different types and functions for each case. >> >> Yes, there is a function to process application memory >(siw_rx_umem), >> to process a kernel PBL (siw_rx_pbl), and one to process kernel >> addresses (siw_rx_kva). Before running that function, the API >> representation of the current SGE gets translated into target >> buffer representation. > >Why does siw_pbl_get_buffer not return a void *?? > I think, in fact, it should be dma_addr_t, since this is what PBL's are described with. Makes sense? >Still looks like two types have been crammed together. > >The kernel can't ever store anything into the user WQE buffer, so I >would think it would copy the buffer to kernel space, transform it >properly and then refer to it as a kernel buffer. Kernel sourced >buffers just skip the transofmration. This is in fact what happens. siw just does not copy immediately during post send, but maintains a mmapped shared send queue. The kernel driver copies the current element from the send queue and processes it. Only then the current element gets transformed into the right buffer representation, since it is not being accessed before. Thanks Bernard. > >JAson > >
Re: Re: Re: Re: Re: [PATCH] RDMA/siw: Fix compiler warnings on 32-bit due to u64/pointer abuse
On Mon, Aug 19, 2019 at 04:29:11PM +, Bernard Metzler wrote: > > >To: "Bernard Metzler" > >From: "Jason Gunthorpe" > >Date: 08/19/2019 06:05PM > >Cc: "Geert Uytterhoeven" , "Doug Ledford" > >, linux-r...@vger.kernel.org, > >linux-kernel@vger.kernel.org > >Subject: [EXTERNAL] Re: Re: Re: Re: [PATCH] RDMA/siw: Fix compiler > >warnings on 32-bit due to u64/pointer abuse > > > >On Mon, Aug 19, 2019 at 03:54:56PM +, Bernard Metzler wrote: > > > >> Absolutely. But these addresses are conveyed through the > >> API as unsigned 64 during post_send(), and land in the siw > >> send queue as is. During send queue processing, these addresses > >> must be interpreted according to its context and transformed > >> (casted) back to the callers intention. I frankly do not > >> know what we can do differently... The representation of > >> all addresses as unsigned 64 is given. Sorry for the confusion. > > > >send work does not have pointers in it, so I'm confused what this is > >about. Does siw allow userspace to stick an ordinary pointer for the > >SG list? > > Right a user references a buffer by address and local key it > got during reservation of that buffer. The user can provide any > VA between start of that buffer and registered length. Oh gross, it overloads the IOVA in the WR with a kernel void * ?? > >The code paths here must be totally different, so there should be > >different types and functions for each case. > > Yes, there is a function to process application memory (siw_rx_umem), > to process a kernel PBL (siw_rx_pbl), and one to process kernel > addresses (siw_rx_kva). Before running that function, the API > representation of the current SGE gets translated into target > buffer representation. Why does siw_pbl_get_buffer not return a void *?? Still looks like two types have been crammed together. The kernel can't ever store anything into the user WQE buffer, so I would think it would copy the buffer to kernel space, transform it properly and then refer to it as a kernel buffer. Kernel sourced buffers just skip the transofmration. JAson
Re: Re: Re: Re: Re: [PATCH] RDMA/siw: Fix compiler warnings on 32-bit due to u64/pointer abuse
-"Jason Gunthorpe" wrote: - >To: "Bernard Metzler" >From: "Jason Gunthorpe" >Date: 08/19/2019 06:05PM >Cc: "Geert Uytterhoeven" , "Doug Ledford" >, linux-r...@vger.kernel.org, >linux-kernel@vger.kernel.org >Subject: [EXTERNAL] Re: Re: Re: Re: [PATCH] RDMA/siw: Fix compiler >warnings on 32-bit due to u64/pointer abuse > >On Mon, Aug 19, 2019 at 03:54:56PM +, Bernard Metzler wrote: > >> Absolutely. But these addresses are conveyed through the >> API as unsigned 64 during post_send(), and land in the siw >> send queue as is. During send queue processing, these addresses >> must be interpreted according to its context and transformed >> (casted) back to the callers intention. I frankly do not >> know what we can do differently... The representation of >> all addresses as unsigned 64 is given. Sorry for the confusion. > >send work does not have pointers in it, so I'm confused what this is >about. Does siw allow userspace to stick an ordinary pointer for the >SG list? Right a user references a buffer by address and local key it got during reservation of that buffer. The user can provide any VA between start of that buffer and registered length. > >The code paths here must be totally different, so there should be >different types and functions for each case. Yes, there is a function to process application memory (siw_rx_umem), to process a kernel PBL (siw_rx_pbl), and one to process kernel addresses (siw_rx_kva). Before running that function, the API representation of the current SGE gets translated into target buffer representation. Thanks and best regards, Bernard.
Re: Re: Re: Re: [PATCH] RDMA/siw: Fix compiler warnings on 32-bit due to u64/pointer abuse
On Mon, Aug 19, 2019 at 03:54:56PM +, Bernard Metzler wrote: > Absolutely. But these addresses are conveyed through the > API as unsigned 64 during post_send(), and land in the siw > send queue as is. During send queue processing, these addresses > must be interpreted according to its context and transformed > (casted) back to the callers intention. I frankly do not > know what we can do differently... The representation of > all addresses as unsigned 64 is given. Sorry for the confusion. send work does not have pointers in it, so I'm confused what this is about. Does siw allow userspace to stick an ordinary pointer for the SG list? The code paths here must be totally different, so there should be different types and functions for each case. Jason
Re: Re: Re: Re: [PATCH] RDMA/siw: Fix compiler warnings on 32-bit due to u64/pointer abuse
-"Jason Gunthorpe" wrote: - >To: "Bernard Metzler" >From: "Jason Gunthorpe" >Date: 08/19/2019 05:07PM >Cc: "Geert Uytterhoeven" , "Doug Ledford" >, linux-r...@vger.kernel.org, >linux-kernel@vger.kernel.org >Subject: [EXTERNAL] Re: Re: Re: [PATCH] RDMA/siw: Fix compiler >warnings on 32-bit due to u64/pointer abuse > >On Mon, Aug 19, 2019 at 02:52:13PM +, Bernard Metzler wrote: >> >> >To: "Bernard Metzler" >> >From: "Jason Gunthorpe" >> >Date: 08/19/2019 04:19PM >> >Cc: "Geert Uytterhoeven" , "Doug Ledford" >> >, linux-r...@vger.kernel.org, >> >linux-kernel@vger.kernel.org >> >Subject: [EXTERNAL] Re: Re: Re: [PATCH] RDMA/siw: Fix compiler >> >warnings on 32-bit due to u64/pointer abuse >> > >> >On Mon, Aug 19, 2019 at 02:15:36PM +, Bernard Metzler wrote: >> >> >> >> >To: "Bernard Metzler" >> >> >From: "Jason Gunthorpe" >> >> >Date: 08/19/2019 03:52PM >> >> >Cc: "Geert Uytterhoeven" , "Doug Ledford" >> >> >, linux-r...@vger.kernel.org, >> >> >linux-kernel@vger.kernel.org >> >> >Subject: [EXTERNAL] Re: Re: [PATCH] RDMA/siw: Fix compiler >> >warnings >> >> >on 32-bit due to u64/pointer abuse >> >> > >> >> >On Mon, Aug 19, 2019 at 01:36:11PM +, Bernard Metzler >wrote: >> >> >> >If the value is really a kernel pointer, then it ought to be >> >> >printed >> >> >> >with %p. We have been getting demanding on this point lately >in >> >> >RDMA >> >> >> >to enforce the ability to keep kernel pointers secret. >> >> >> > >> >> >> >> - wqe->sqe.sge[0].laddr = (u64)&wqe->sqe.sge[1]; >> >> >> >> + wqe->sqe.sge[0].laddr = >> >> >> >> (uintptr_t)&wqe->sqe.sge[1]; >> >> >> > >> >> >> >[..] >> >> >> > >> >> >> >> rv = siw_rx_kva(srx, >> >> >> >> - (void *)(sge->laddr + >> >> >> >> frx->sge_off), >> >> >> >> + (void *)(uintptr_t)(sge->laddr >> >> >> >> + frx->sge_off), >> >> >> >> sge_bytes); >> >> >> > >> >> >> >Bernard, this is nonsense, what is going on here with >> >sge->laddr >> >> >that >> >> >> >it can't be a void *? >> >> >> > >> >> >> siw_sge is defined in siw-abi.h. We make the address u64 to >keep >> >> >the ABI >> >> >> arch independent. >> >> > >> >> >Eh? How does the siw-abi.h store a kernel pointer? Sounds like >> >kernel >> >> >and user types are being mixed. >> >> > >> >> >> >> siw-abi.h defines the work queue elements of a siw send queue. >> >> For user land, the send queue is mmapped. Kernel or user land >> >> clients write to its send queue when posting work >> >> (SGE: buffer address, length, local key). >> > >> >Should have different types.. Don't want to accidently mix a laddr >> >under user control with one under kernel control. >> > >> Well we have an unsigned 64bit for both user and kernel >> application buffer addresses throughout the rdma stack, > >We do not. Kernel addresses are consistenyly void * or dma_addr_t > Absolutely. But these addresses are conveyed through the API as unsigned 64 during post_send(), and land in the siw send queue as is. During send queue processing, these addresses must be interpreted according to its context and transformed (casted) back to the callers intention. I frankly do not know what we can do differently... The representation of all addresses as unsigned 64 is given. Sorry for the confusion. >Most places that consume a data address are using lkeys anyhow, which >does have a lkey & u64, but that u64 is not a application buffer >address, but the IOVA of the lkey, which is very different. > >I really have no idea why siw needs to mix kernel VAs with user >pointers, particularly in wqes... > >Jason > >
