diff mbox series

[v3,09/10] nvme: add handling for app_tag

Message ID 20240823103811.2421-11-anuj20.g@samsung.com (mailing list archive)
State New
Headers show
Series Read/Write with meta/integrity | expand

Commit Message

Anuj Gupta Aug. 23, 2024, 10:38 a.m. UTC
From: Kanchan Joshi <joshi.k@samsung.com>

With user integrity buffer, there is a way to specify the app_tag.
Set the corresponding protocol specific flags and send the app_tag down.

Signed-off-by: Anuj Gupta <anuj20.g@samsung.com>
Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
---
 drivers/nvme/host/core.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Christoph Hellwig Aug. 24, 2024, 8:49 a.m. UTC | #1
Maybe name this "add support for passin on the application tag" ?

> +static void nvme_set_app_tag(struct nvme_command *cmnd, u16 apptag)
> +{
> +	cmnd->rw.apptag = cpu_to_le16(apptag);
> +	/* use 0xfff as mask so that apptag is used in entirety */
> +	cmnd->rw.appmask = cpu_to_le16(0xffff);

Can you throw in a patch to rename these field to match the field names
used in the specification?  I also don't think the comment is all that
useful.

> +}
> +
>  static void nvme_set_ref_tag(struct nvme_ns *ns, struct nvme_command *cmnd,
>  			      struct request *req)
>  {
> @@ -1010,6 +1017,11 @@ static inline blk_status_t nvme_setup_rw(struct nvme_ns *ns,
>  				control |= NVME_RW_APPEND_PIREMAP;
>  			nvme_set_ref_tag(ns, cmnd, req);
>  		}
> +		if (bio_integrity_flagged(req->bio, BIP_CHECK_APPTAG)) {
> +			control |= NVME_RW_PRINFO_PRCHK_APP;
> +			nvme_set_app_tag(cmnd,
> +					 bio_integrity(req->bio)->app_tag);

Passing the request to nvme_set_app_tag would probably be a bit cleaner.
Martin K. Petersen Aug. 29, 2024, 3 a.m. UTC | #2
Anuj,

> With user integrity buffer, there is a way to specify the app_tag. Set
> the corresponding protocol specific flags and send the app_tag down.

This assumes the app tag is the same for every block in the I/O? That's
not how it's typically used (to the extent that it is used at all due to
the value 0xffff acting as escape).
Kanchan Joshi Aug. 29, 2024, 10:18 a.m. UTC | #3
On 8/29/2024 8:30 AM, Martin K. Petersen wrote:
> 
> Anuj,
> 
>> With user integrity buffer, there is a way to specify the app_tag. Set
>> the corresponding protocol specific flags and send the app_tag down.
> 
> This assumes the app tag is the same for every block in the I/O? That's
> not how it's typically used (to the extent that it is used at all due to
> the value 0xffff acting as escape).
> 

NVMe spec allows only one value to be passed in each read/write command 
(LBAT for write, and ELBAT for read). And that's what controller checks 
for the entire block range covered by one command.
So per-io tag rather than per-block tag. The integrity buffer creator is 
supposed to put the same application-tag for each block if it is sending 
multi-block IO.
Martin K. Petersen Sept. 13, 2024, 2:05 a.m. UTC | #4
Kanchan,

>> This assumes the app tag is the same for every block in the I/O?
>> That's not how it's typically used (to the extent that it is used at
>> all due to the value 0xffff acting as escape).
>> 
>
> NVMe spec allows only one value to be passed in each read/write
> command (LBAT for write, and ELBAT for read). And that's what
> controller checks for the entire block range covered by one command.
> So per-io tag rather than per-block tag. The integrity buffer creator
> is supposed to put the same application-tag for each block if it is
> sending multi-block IO.

I am OK with that approach as long as the mask is only applied when
checking is enabled.

I.e. I don't have a use case for checking less than the full app tag.
But almost all users of PI I am aware of depend on being able to put
different values in the app tag for different blocks in the I/O when
checking is disabled.
diff mbox series

Patch

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index d4c366df8f12..af6940ec6e3c 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -871,6 +871,13 @@  static blk_status_t nvme_setup_discard(struct nvme_ns *ns, struct request *req,
 	return BLK_STS_OK;
 }
 
+static void nvme_set_app_tag(struct nvme_command *cmnd, u16 apptag)
+{
+	cmnd->rw.apptag = cpu_to_le16(apptag);
+	/* use 0xfff as mask so that apptag is used in entirety */
+	cmnd->rw.appmask = cpu_to_le16(0xffff);
+}
+
 static void nvme_set_ref_tag(struct nvme_ns *ns, struct nvme_command *cmnd,
 			      struct request *req)
 {
@@ -1010,6 +1017,11 @@  static inline blk_status_t nvme_setup_rw(struct nvme_ns *ns,
 				control |= NVME_RW_APPEND_PIREMAP;
 			nvme_set_ref_tag(ns, cmnd, req);
 		}
+		if (bio_integrity_flagged(req->bio, BIP_CHECK_APPTAG)) {
+			control |= NVME_RW_PRINFO_PRCHK_APP;
+			nvme_set_app_tag(cmnd,
+					 bio_integrity(req->bio)->app_tag);
+		}
 	}
 
 	cmnd->rw.control = cpu_to_le16(control);