[PATCH 1/1] nvme: introduce nvme_transport_sgl_desc structure

[PATCH 1/1] nvme: introduce nvme_transport_sgl_desc structure

[Max Gurtovoy]

Currently the tcp and fc transports use nvme_sgl_desc structure to
describe transport data blocks. Replace it with a new structure that
describes the Transport SGL Data Block according to the NVMe
specification.

Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 drivers/nvme/host/fc.c  |  6 ++----
 drivers/nvme/host/tcp.c |  6 ++----
 include/linux/nvme.h    | 14 +++++++++++---
 3 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index f183f9fa03d0..adcf39a01d14 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2698,13 +2698,11 @@ nvme_fc_start_fcp_op(struct nvme_fc_ctrl *ctrl, struct nvme_fc_queue *queue,
 	 * format SQE DPTR field per FC-NVME rules:
 	 *    type=0x5     Transport SGL Data Block Descriptor
 	 *    subtype=0xA  Transport-specific value
-	 *    address=0
 	 *    length=length of the data series
 	 */
-	sqe->rw.dptr.sgl.type = (NVME_TRANSPORT_SGL_DATA_DESC << 4) |
+	sqe->rw.dptr.tsgl.type = (NVME_TRANSPORT_SGL_DATA_DESC << 4) |
 					NVME_SGL_FMT_TRANSPORT_A;
-	sqe->rw.dptr.sgl.length = cpu_to_le32(data_len);
-	sqe->rw.dptr.sgl.addr = 0;
+	sqe->rw.dptr.tsgl.length = cpu_to_le32(data_len);
 
 	if (!(op->flags & FCOP_FLAGS_AEN)) {
 		ret = nvme_fc_map_data(ctrl, op->rq, op);
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 34f4b3402f7c..c696b962c486 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2143,9 +2143,8 @@ static void nvme_tcp_free_ctrl(struct nvme_ctrl *nctrl)
 
 static void nvme_tcp_set_sg_null(struct nvme_command *c)
 {
-	struct nvme_sgl_desc *sg = &c->common.dptr.sgl;
+	struct nvme_transport_sgl_desc *sg = &c->common.dptr.tsgl;
 
-	sg->addr = 0;
 	sg->length = 0;
 	sg->type = (NVME_TRANSPORT_SGL_DATA_DESC << 4) |
 			NVME_SGL_FMT_TRANSPORT_A;
@@ -2164,9 +2163,8 @@ static void nvme_tcp_set_sg_inline(struct nvme_tcp_queue *queue,
 static void nvme_tcp_set_sg_host_data(struct nvme_command *c,
 		u32 data_len)
 {
-	struct nvme_sgl_desc *sg = &c->common.dptr.sgl;
+	struct nvme_transport_sgl_desc *sg = &c->common.dptr.tsgl;
 
-	sg->addr = 0;
 	sg->length = cpu_to_le32(data_len);
 	sg->type = (NVME_TRANSPORT_SGL_DATA_DESC << 4) |
 			NVME_SGL_FMT_TRANSPORT_A;
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index edcbd60b88b9..ca0a17970c75 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -742,7 +742,7 @@ enum {
  *   @NVME_KEY_SGL_FMT_DATA_DESC:	keyed data block descriptor
  *
  * Transport-specific SGL types:
- *   @NVME_TRANSPORT_SGL_DATA_DESC:	Transport SGL data dlock descriptor
+ *   @NVME_TRANSPORT_SGL_DATA_DESC:	Transport SGL data block descriptor
  */
 enum {
 	NVME_SGL_FMT_DATA_DESC		= 0x00,
@@ -766,13 +766,21 @@ struct nvme_keyed_sgl_desc {
 	__u8	type;
 };
 
+struct nvme_transport_sgl_desc {
+	__u8	rsvd0[8];
+	__le32	length;
+	__u8	rsvd1[3];
+	__u8	type;
+};
+
 union nvme_data_ptr {
 	struct {
 		__le64	prp1;
 		__le64	prp2;
 	};
-	struct nvme_sgl_desc	sgl;
-	struct nvme_keyed_sgl_desc ksgl;
+	struct nvme_sgl_desc		sgl;
+	struct nvme_keyed_sgl_desc	ksgl;
+	struct nvme_transport_sgl_desc	tsgl;
 };
 
 /*
-- 
2.18.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 1/1] nvme: introduce nvme_transport_sgl_desc structure

[Christoph Hellwig]

On Mon, Jul 19, 2021 at 12:53:09PM +0300, Max Gurtovoy wrote:
> Currently the tcp and fc transports use nvme_sgl_desc structure to
> describe transport data blocks. Replace it with a new structure that
> describes the Transport SGL Data Block according to the NVMe
> specification.

What is the point?  Note that we should probably still clear the
reserved space anyway.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 1/1] nvme: introduce nvme_transport_sgl_desc structure

[Max Gurtovoy]

On 7/19/2021 12:57 PM, Christoph Hellwig wrote:
> On Mon, Jul 19, 2021 at 12:53:09PM +0300, Max Gurtovoy wrote:
>> Currently the tcp and fc transports use nvme_sgl_desc structure to
>> describe transport data blocks. Replace it with a new structure that
>> describes the Transport SGL Data Block according to the NVMe
>> specification.
> What is the point?  Note that we should probably still clear the
> reserved space anyway.

The point is to use the correct structure from the spec and not enforce 
some weird casting.

This makes the code more readable.

Reserved space should be treated as for all other reserved fields in the 
spec.



_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 1/1] nvme: introduce nvme_transport_sgl_desc structure

[Sagi Grimberg]

>> Currently the tcp and fc transports use nvme_sgl_desc structure to
>> describe transport data blocks. Replace it with a new structure that
>> describes the Transport SGL Data Block according to the NVMe
>> specification.
> 
> What is the point?

I think its ok to be more aligned to the spec, and have the transport
sgl represented in the code. Don't see any harm in that.

> Note that we should probably still clear the reserved space anyway.

Definitely we should clear it.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 1/1] nvme: introduce nvme_transport_sgl_desc structure

[Max Gurtovoy]

On 7/19/2021 10:58 PM, Sagi Grimberg wrote:
>
>>> Currently the tcp and fc transports use nvme_sgl_desc structure to
>>> describe transport data blocks. Replace it with a new structure that
>>> describes the Transport SGL Data Block according to the NVMe
>>> specification.
>>
>> What is the point?
>
> I think its ok to be more aligned to the spec, and have the transport
> sgl represented in the code. Don't see any harm in that.
>
>> Note that we should probably still clear the reserved space anyway.
>
> Definitely we should clear it.

don't you think it's cleared already ?

if not, lets clear also other rsvd fields while we're here.

Each driver call nvme_setup_cmd before mapping the data and this 
function clears the SQE (if (!(req->rq_flags & RQF_DONTPREP)))..




_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 1/1] nvme: introduce nvme_transport_sgl_desc structure

[Sagi Grimberg]

>>>> Currently the tcp and fc transports use nvme_sgl_desc structure to
>>>> describe transport data blocks. Replace it with a new structure that
>>>> describes the Transport SGL Data Block according to the NVMe
>>>> specification.
>>>
>>> What is the point?
>>
>> I think its ok to be more aligned to the spec, and have the transport
>> sgl represented in the code. Don't see any harm in that.
>>
>>> Note that we should probably still clear the reserved space anyway.
>>
>> Definitely we should clear it.
> 
> don't you think it's cleared already ?
> 
> if not, lets clear also other rsvd fields while we're here.
> 
> Each driver call nvme_setup_cmd before mapping the data and this 
> function clears the SQE (if (!(req->rq_flags & RQF_DONTPREP)))..

Yea, I guess its fine to rely on nvme_setup_cmd for it.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme