Re: fine-grained PI control

[Anuj Gupta <anuj20.g@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 3689 bytes --]

On Tue, Jul 09, 2024 at 09:16:04AM +0200, Christoph Hellwig wrote:
> On Mon, Jul 08, 2024 at 11:35:13PM -0400, Martin K. Petersen wrote:
> > I don't like having the BIP_USER_CHECK_FOO flags exposed in the block
> > layer. The io_uring interface obviously needs to expose some flags in
> > the user API. But there should not be a separate set of BIP_USER_* flags
> > bolted onto the side of the existing kernel integrity flags.
> >
> > The bip flags should describe the contents of the integrity buffer and
> > how the hardware needs to interpret and check that information.
> 
> Yes, that was also my review comments.

Hi Christoph, Martin,

I was thinking something like below patch[*] could help us get rid of the
BIP_USER_CHECK_FOO flags, and also driver can now check flags passed by block
layer instead of checking if it's user passthrough data. Haven't plumbed the
scsi side of things, but do you think it can work with scsi? 

Subject: [PATCH] block: introduce BIP_CHECK_GUARD/REFTAG/APPTAG bip_flags

This patch introduces BIP_CHECK_GUARD/REFTAG/APPTAG bip_flags which
indicate how the hardware should check the payload. The driver can now
just rely on block layer flags, and doesn't need to know the integrity
source. Submitter of PI chooses which tags to check. This would also
give us a unified interface for user and kernel generated integrity.

Signed-off-by: Anuj Gupta <anuj20.g@samsung.com>
Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
---
 block/bio-integrity.c         |  5 +++++
 drivers/nvme/host/core.c      | 12 +++---------
 include/linux/bio-integrity.h |  3 +++
 3 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index 8d1fb38f745f..d179b0134e1d 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -443,6 +443,11 @@ bool bio_integrity_prep(struct bio *bio)
 	if (bi->csum_type == BLK_INTEGRITY_CSUM_IP)
 		bip->bip_flags |= BIP_IP_CHECKSUM;
 
+	/* describe what tags to check in payload */
+	if (bi->csum_type)
+		bip->bip_flags |= BIP_CHECK_GUARD;
+	if (bi->flags & BLK_INTEGRITY_REF_TAG)
+		bip->bip_flags |= BIP_CHECK_REFTAG;
 	if (bio_integrity_add_page(bio, virt_to_page(buf), len,
 			offset_in_page(buf)) < len) {
 		printk(KERN_ERR "could not attach integrity payload\n");
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 19917253ba7b..5991f048f394 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1001,19 +1001,13 @@ static inline blk_status_t nvme_setup_rw(struct nvme_ns *ns,
 				return BLK_STS_NOTSUPP;
 			control |= NVME_RW_PRINFO_PRACT;
 		}
-
-		switch (ns->head->pi_type) {
-		case NVME_NS_DPS_PI_TYPE3:
+		if (bio_integrity_flagged(req->bio, BIP_CHECK_GUARD))
 			control |= NVME_RW_PRINFO_PRCHK_GUARD;
-			break;
-		case NVME_NS_DPS_PI_TYPE1:
-		case NVME_NS_DPS_PI_TYPE2:
-			control |= NVME_RW_PRINFO_PRCHK_GUARD |
-					NVME_RW_PRINFO_PRCHK_REF;
+		if (bio_integrity_flagged(req->bio, BIP_CHECK_REFTAG)) {
+			control |= NVME_RW_PRINFO_PRCHK_REF;
 			if (op == nvme_cmd_zone_append)
 				control |= NVME_RW_APPEND_PIREMAP;
 			nvme_set_ref_tag(ns, cmnd, req);
-			break;
 		}
 	}
 
diff --git a/include/linux/bio-integrity.h b/include/linux/bio-integrity.h
index dd831c269e99..a7e3dfc994b0 100644
--- a/include/linux/bio-integrity.h
+++ b/include/linux/bio-integrity.h
@@ -11,6 +11,9 @@ enum bip_flags {
 	BIP_DISK_NOCHECK	= 1 << 3, /* disable disk integrity checking */
 	BIP_IP_CHECKSUM		= 1 << 4, /* IP checksum */
 	BIP_COPY_USER		= 1 << 5, /* Kernel bounce buffer in use */
+	BIP_CHECK_GUARD		= 1 << 6,
+	BIP_CHECK_REFTAG	= 1 << 7,
+	BIP_CHECK_APPTAG	= 1 << 8,
 };
 
 struct bio_integrity_payload {
-- 
2.25.1

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]