| Message ID | 20240823103811.2421-11-anuj20.g@samsung.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,01/10] block: define set of integrity flags to be inherited by cloned bip | expand |
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.
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).
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.
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 --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);