[PATCH v2] virtio_blk: add SECURE ERASE command support

[PATCH v2] virtio_blk: add SECURE ERASE command support

[Alvaro Karsz]

Support for the VIRTIO_BLK_F_SECURE_ERASE VirtIO feature.

A device that offers this feature can receive VIRTIO_BLK_T_SECURE_ERASE
commands.

A device which supports this feature has the following fields in the
virtio config:

- max_secure_erase_sectors
- max_secure_erase_seg
- secure_erase_sector_alignment

max_secure_erase_sectors and secure_erase_sector_alignment are expressed
in 512-byte units.

Every secure erase command has the following fields:

- sectors: The starting offset in 512-byte units.
- num_sectors: The number of sectors.

Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
---
v2:
	- Set queue max discard segments as the minimum between
	  max_secure_erase_seg and max_discard_seg.
	- Set queue discard granularity as the minimum between
	  secure_erase_sector_alignment and discard_sector_alignment.
---
 drivers/block/virtio_blk.c      | 82 +++++++++++++++++++++++++--------
 include/uapi/linux/virtio_blk.h | 19 ++++++++
 2 files changed, 83 insertions(+), 18 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 30255fcaf18..8a2f00cdf52 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -130,7 +130,7 @@ static int virtblk_add_req(struct virtqueue *vq, struct virtblk_req *vbr)
 	return virtqueue_add_sgs(vq, sgs, num_out, num_in, vbr, GFP_ATOMIC);
 }
 
-static int virtblk_setup_discard_write_zeroes(struct request *req, bool unmap)
+static int virtblk_setup_discard_write_zeroes_erase(struct request *req, bool unmap)
 {
 	unsigned short segments = blk_rq_nr_discard_segments(req);
 	unsigned short n = 0;
@@ -240,6 +240,9 @@ static blk_status_t virtblk_setup_cmd(struct virtio_device *vdev,
 		type = VIRTIO_BLK_T_WRITE_ZEROES;
 		unmap = !(req->cmd_flags & REQ_NOUNMAP);
 		break;
+	case REQ_OP_SECURE_ERASE:
+		type = VIRTIO_BLK_T_SECURE_ERASE;
+		break;
 	case REQ_OP_DRV_IN:
 		type = VIRTIO_BLK_T_GET_ID;
 		break;
@@ -251,8 +254,9 @@ static blk_status_t virtblk_setup_cmd(struct virtio_device *vdev,
 	vbr->out_hdr.type = cpu_to_virtio32(vdev, type);
 	vbr->out_hdr.ioprio = cpu_to_virtio32(vdev, req_get_ioprio(req));
 
-	if (type == VIRTIO_BLK_T_DISCARD || type == VIRTIO_BLK_T_WRITE_ZEROES) {
-		if (virtblk_setup_discard_write_zeroes(req, unmap))
+	if (type == VIRTIO_BLK_T_DISCARD || type == VIRTIO_BLK_T_WRITE_ZEROES ||
+	    type == VIRTIO_BLK_T_SECURE_ERASE) {
+		if (virtblk_setup_discard_write_zeroes_erase(req, unmap))
 			return BLK_STS_RESOURCE;
 	}
 
@@ -889,6 +893,8 @@ static int virtblk_probe(struct virtio_device *vdev)
 	int err, index;
 
 	u32 v, blk_size, max_size, sg_elems, opt_io_size;
+	u32 max_discard_segs = 0;
+	u32 discard_granularity = 0;
 	u16 min_io_size;
 	u8 physical_block_exp, alignment_offset;
 	unsigned int queue_depth;
@@ -1046,27 +1052,14 @@ static int virtblk_probe(struct virtio_device *vdev)
 
 	if (virtio_has_feature(vdev, VIRTIO_BLK_F_DISCARD)) {
 		virtio_cread(vdev, struct virtio_blk_config,
-			     discard_sector_alignment, &v);
-		if (v)
-			q->limits.discard_granularity = v << SECTOR_SHIFT;
-		else
-			q->limits.discard_granularity = blk_size;
+			     discard_sector_alignment, &discard_granularity);
 
 		virtio_cread(vdev, struct virtio_blk_config,
 			     max_discard_sectors, &v);
 		blk_queue_max_discard_sectors(q, v ? v : UINT_MAX);
 
 		virtio_cread(vdev, struct virtio_blk_config, max_discard_seg,
-			     &v);
-
-		/*
-		 * max_discard_seg == 0 is out of spec but we always
-		 * handled it.
-		 */
-		if (!v)
-			v = sg_elems;
-		blk_queue_max_discard_segments(q,
-					       min(v, MAX_DISCARD_SEGMENTS));
+			     &max_discard_segs);
 	}
 
 	if (virtio_has_feature(vdev, VIRTIO_BLK_F_WRITE_ZEROES)) {
@@ -1075,6 +1068,57 @@ static int virtblk_probe(struct virtio_device *vdev)
 		blk_queue_max_write_zeroes_sectors(q, v ? v : UINT_MAX);
 	}
 
+	if (virtio_has_feature(vdev, VIRTIO_BLK_F_SECURE_ERASE)) {
+		/* The discard alignment value should be the minimum between
+		 * secure_erase_sector_alignment and discard_sector_alignment
+		 * (if VIRTIO_BLK_F_DISCARD is negotiated).
+		 */
+		virtio_cread(vdev, struct virtio_blk_config,
+			     secure_erase_sector_alignment, &v);
+		if (v) {
+			if (discard_granularity)
+				discard_granularity = min(discard_granularity, v);
+			else
+				discard_granularity = v;
+		}
+
+		virtio_cread(vdev, struct virtio_blk_config,
+			     max_secure_erase_sectors, &v);
+		blk_queue_max_secure_erase_sectors(q, v ? v : UINT_MAX);
+
+		/* The max discard segments value should be the minimum between
+		 * max_secure_erase_seg and max_discard_seg (if VIRTIO_BLK_F_DISCARD
+		 * is negotiated).
+		 */
+		virtio_cread(vdev, struct virtio_blk_config,
+			     max_secure_erase_seg, &v);
+		if (v) {
+			if (max_discard_segs)
+				max_discard_segs = min(max_discard_segs, v);
+			else
+				max_discard_segs = v;
+		}
+	}
+
+	if (virtio_has_feature(vdev, VIRTIO_BLK_F_DISCARD) ||
+	    virtio_has_feature(vdev, VIRTIO_BLK_F_SECURE_ERASE)) {
+
+		/* max_discard_seg == 0 or max_secure_erase_seg == 0
+		 * are out of spec but we always handled it.
+		 */
+		if (!max_discard_segs)
+			max_discard_segs = sg_elems;
+
+		blk_queue_max_discard_segments(q,
+					       min(max_discard_segs, MAX_DISCARD_SEGMENTS));
+
+		/* If alignemnt is 0, use block size alignment */
+		if (discard_granularity)
+			q->limits.discard_granularity = discard_granularity << SECTOR_SHIFT;
+		else
+			q->limits.discard_granularity = blk_size;
+	}
+
 	virtblk_update_capacity(vblk, false);
 	virtio_device_ready(vdev);
 
@@ -1170,6 +1214,7 @@ static unsigned int features_legacy[] = {
 	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
 	VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
 	VIRTIO_BLK_F_MQ, VIRTIO_BLK_F_DISCARD, VIRTIO_BLK_F_WRITE_ZEROES,
+	VIRTIO_BLK_F_SECURE_ERASE,
 }
 ;
 static unsigned int features[] = {
@@ -1177,6 +1222,7 @@ static unsigned int features[] = {
 	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
 	VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
 	VIRTIO_BLK_F_MQ, VIRTIO_BLK_F_DISCARD, VIRTIO_BLK_F_WRITE_ZEROES,
+	VIRTIO_BLK_F_SECURE_ERASE,
 };
 
 static struct virtio_driver virtio_blk = {
diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h
index d888f013d9f..58e70b24b50 100644
--- a/include/uapi/linux/virtio_blk.h
+++ b/include/uapi/linux/virtio_blk.h
@@ -40,6 +40,7 @@
 #define VIRTIO_BLK_F_MQ		12	/* support more than one vq */
 #define VIRTIO_BLK_F_DISCARD	13	/* DISCARD is supported */
 #define VIRTIO_BLK_F_WRITE_ZEROES	14	/* WRITE ZEROES is supported */
+#define VIRTIO_BLK_F_SECURE_ERASE	16 /* Secure Erase is supported */
 
 /* Legacy feature bits */
 #ifndef VIRTIO_BLK_NO_LEGACY
@@ -121,6 +122,21 @@ struct virtio_blk_config {
 	__u8 write_zeroes_may_unmap;
 
 	__u8 unused1[3];
+
+	/* the next 3 entries are guarded by VIRTIO_BLK_F_SECURE_ERASE */
+	/*
+	 * The maximum secure erase sectors (in 512-byte sectors) for
+	 * one segment.
+	 */
+	__virtio32 max_secure_erase_sectors;
+	/*
+	 * The maximum number of secure erase segments in a
+	 * secure erase command.
+	 */
+	__virtio32 max_secure_erase_seg;
+	/* Secure erase commands must be aligned to this number of sectors. */
+	__virtio32 secure_erase_sector_alignment;
+
 } __attribute__((packed));
 
 /*
@@ -155,6 +171,9 @@ struct virtio_blk_config {
 /* Write zeroes command */
 #define VIRTIO_BLK_T_WRITE_ZEROES	13
 
+/* Secure erase command */
+#define VIRTIO_BLK_T_SECURE_ERASE	14
+
 #ifndef VIRTIO_BLK_NO_LEGACY
 /* Barrier before this op. */
 #define VIRTIO_BLK_T_BARRIER	0x80000000
-- 
2.32.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add SECURE ERASE command support

[Alvaro Karsz]

Is that what you meant Stefan?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add SECURE ERASE command support

[Alvaro Karsz]

Hi,
Anyone have any comments on the patch?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add SECURE ERASE command support

[Michael S. Tsirkin]

On Mon, Aug 29, 2022 at 11:23:13AM +0300, Alvaro Karsz wrote:
> Support for the VIRTIO_BLK_F_SECURE_ERASE VirtIO feature.
> 
> A device that offers this feature can receive VIRTIO_BLK_T_SECURE_ERASE
> commands.
> 
> A device which supports this feature has the following fields in the
> virtio config:
> 
> - max_secure_erase_sectors
> - max_secure_erase_seg
> - secure_erase_sector_alignment
> 
> max_secure_erase_sectors and secure_erase_sector_alignment are expressed
> in 512-byte units.
> 
> Every secure erase command has the following fields:
> 
> - sectors: The starting offset in 512-byte units.
> - num_sectors: The number of sectors.
> 
> Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>

Review from storage maintainers before I queue this?

Thanks!

> ---
> v2:
> 	- Set queue max discard segments as the minimum between
> 	  max_secure_erase_seg and max_discard_seg.
> 	- Set queue discard granularity as the minimum between
> 	  secure_erase_sector_alignment and discard_sector_alignment.
> ---
>  drivers/block/virtio_blk.c      | 82 +++++++++++++++++++++++++--------
>  include/uapi/linux/virtio_blk.h | 19 ++++++++
>  2 files changed, 83 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 30255fcaf18..8a2f00cdf52 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -130,7 +130,7 @@ static int virtblk_add_req(struct virtqueue *vq, struct virtblk_req *vbr)
>  	return virtqueue_add_sgs(vq, sgs, num_out, num_in, vbr, GFP_ATOMIC);
>  }
>  
> -static int virtblk_setup_discard_write_zeroes(struct request *req, bool unmap)
> +static int virtblk_setup_discard_write_zeroes_erase(struct request *req, bool unmap)
>  {
>  	unsigned short segments = blk_rq_nr_discard_segments(req);
>  	unsigned short n = 0;
> @@ -240,6 +240,9 @@ static blk_status_t virtblk_setup_cmd(struct virtio_device *vdev,
>  		type = VIRTIO_BLK_T_WRITE_ZEROES;
>  		unmap = !(req->cmd_flags & REQ_NOUNMAP);
>  		break;
> +	case REQ_OP_SECURE_ERASE:
> +		type = VIRTIO_BLK_T_SECURE_ERASE;
> +		break;
>  	case REQ_OP_DRV_IN:
>  		type = VIRTIO_BLK_T_GET_ID;
>  		break;
> @@ -251,8 +254,9 @@ static blk_status_t virtblk_setup_cmd(struct virtio_device *vdev,
>  	vbr->out_hdr.type = cpu_to_virtio32(vdev, type);
>  	vbr->out_hdr.ioprio = cpu_to_virtio32(vdev, req_get_ioprio(req));
>  
> -	if (type == VIRTIO_BLK_T_DISCARD || type == VIRTIO_BLK_T_WRITE_ZEROES) {
> -		if (virtblk_setup_discard_write_zeroes(req, unmap))
> +	if (type == VIRTIO_BLK_T_DISCARD || type == VIRTIO_BLK_T_WRITE_ZEROES ||
> +	    type == VIRTIO_BLK_T_SECURE_ERASE) {
> +		if (virtblk_setup_discard_write_zeroes_erase(req, unmap))
>  			return BLK_STS_RESOURCE;
>  	}
>  
> @@ -889,6 +893,8 @@ static int virtblk_probe(struct virtio_device *vdev)
>  	int err, index;
>  
>  	u32 v, blk_size, max_size, sg_elems, opt_io_size;
> +	u32 max_discard_segs = 0;
> +	u32 discard_granularity = 0;
>  	u16 min_io_size;
>  	u8 physical_block_exp, alignment_offset;
>  	unsigned int queue_depth;
> @@ -1046,27 +1052,14 @@ static int virtblk_probe(struct virtio_device *vdev)
>  
>  	if (virtio_has_feature(vdev, VIRTIO_BLK_F_DISCARD)) {
>  		virtio_cread(vdev, struct virtio_blk_config,
> -			     discard_sector_alignment, &v);
> -		if (v)
> -			q->limits.discard_granularity = v << SECTOR_SHIFT;
> -		else
> -			q->limits.discard_granularity = blk_size;
> +			     discard_sector_alignment, &discard_granularity);
>  
>  		virtio_cread(vdev, struct virtio_blk_config,
>  			     max_discard_sectors, &v);
>  		blk_queue_max_discard_sectors(q, v ? v : UINT_MAX);
>  
>  		virtio_cread(vdev, struct virtio_blk_config, max_discard_seg,
> -			     &v);
> -
> -		/*
> -		 * max_discard_seg == 0 is out of spec but we always
> -		 * handled it.
> -		 */
> -		if (!v)
> -			v = sg_elems;
> -		blk_queue_max_discard_segments(q,
> -					       min(v, MAX_DISCARD_SEGMENTS));
> +			     &max_discard_segs);
>  	}
>  
>  	if (virtio_has_feature(vdev, VIRTIO_BLK_F_WRITE_ZEROES)) {
> @@ -1075,6 +1068,57 @@ static int virtblk_probe(struct virtio_device *vdev)
>  		blk_queue_max_write_zeroes_sectors(q, v ? v : UINT_MAX);
>  	}
>  
> +	if (virtio_has_feature(vdev, VIRTIO_BLK_F_SECURE_ERASE)) {
> +		/* The discard alignment value should be the minimum between
> +		 * secure_erase_sector_alignment and discard_sector_alignment
> +		 * (if VIRTIO_BLK_F_DISCARD is negotiated).

why minimum?

> +		 */
> +		virtio_cread(vdev, struct virtio_blk_config,
> +			     secure_erase_sector_alignment, &v);
> +		if (v) {
> +			if (discard_granularity)
> +				discard_granularity = min(discard_granularity, v);
> +			else
> +				discard_granularity = v;
> +		}
> +
> +		virtio_cread(vdev, struct virtio_blk_config,
> +			     max_secure_erase_sectors, &v);
> +		blk_queue_max_secure_erase_sectors(q, v ? v : UINT_MAX);
> +
> +		/* The max discard segments value should be the minimum between
> +		 * max_secure_erase_seg and max_discard_seg (if VIRTIO_BLK_F_DISCARD
> +		 * is negotiated).

why is that?

> +		 */
> +		virtio_cread(vdev, struct virtio_blk_config,
> +			     max_secure_erase_seg, &v);
> +		if (v) {
> +			if (max_discard_segs)
> +				max_discard_segs = min(max_discard_segs, v);
> +			else
> +				max_discard_segs = v;

is this logic repeating code from below?

> +		}
> +	}
> +
> +	if (virtio_has_feature(vdev, VIRTIO_BLK_F_DISCARD) ||
> +	    virtio_has_feature(vdev, VIRTIO_BLK_F_SECURE_ERASE)) {
> +
> +		/* max_discard_seg == 0 or max_secure_erase_seg == 0
> +		 * are out of spec but we always handled it.

Always? What's going on here?
which versions handled max_secure_erase_seg == 0?


> +		 */
> +		if (!max_discard_segs)
> +			max_discard_segs = sg_elems;
> +
> +		blk_queue_max_discard_segments(q,
> +					       min(max_discard_segs, MAX_DISCARD_SEGMENTS));
> +
> +		/* If alignemnt is 0, use block size alignment */
> +		if (discard_granularity)
> +			q->limits.discard_granularity = discard_granularity << SECTOR_SHIFT;
> +		else
> +			q->limits.discard_granularity = blk_size;
> +	}
> +
>  	virtblk_update_capacity(vblk, false);
>  	virtio_device_ready(vdev);
>  
> @@ -1170,6 +1214,7 @@ static unsigned int features_legacy[] = {
>  	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
>  	VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
>  	VIRTIO_BLK_F_MQ, VIRTIO_BLK_F_DISCARD, VIRTIO_BLK_F_WRITE_ZEROES,
> +	VIRTIO_BLK_F_SECURE_ERASE,
>  }
>  ;
>  static unsigned int features[] = {
> @@ -1177,6 +1222,7 @@ static unsigned int features[] = {
>  	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
>  	VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
>  	VIRTIO_BLK_F_MQ, VIRTIO_BLK_F_DISCARD, VIRTIO_BLK_F_WRITE_ZEROES,
> +	VIRTIO_BLK_F_SECURE_ERASE,
>  };
>  
>  static struct virtio_driver virtio_blk = {
> diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h
> index d888f013d9f..58e70b24b50 100644
> --- a/include/uapi/linux/virtio_blk.h
> +++ b/include/uapi/linux/virtio_blk.h
> @@ -40,6 +40,7 @@
>  #define VIRTIO_BLK_F_MQ		12	/* support more than one vq */
>  #define VIRTIO_BLK_F_DISCARD	13	/* DISCARD is supported */
>  #define VIRTIO_BLK_F_WRITE_ZEROES	14	/* WRITE ZEROES is supported */
> +#define VIRTIO_BLK_F_SECURE_ERASE	16 /* Secure Erase is supported */
>  
>  /* Legacy feature bits */
>  #ifndef VIRTIO_BLK_NO_LEGACY
> @@ -121,6 +122,21 @@ struct virtio_blk_config {
>  	__u8 write_zeroes_may_unmap;
>  
>  	__u8 unused1[3];
> +
> +	/* the next 3 entries are guarded by VIRTIO_BLK_F_SECURE_ERASE */
> +	/*
> +	 * The maximum secure erase sectors (in 512-byte sectors) for
> +	 * one segment.
> +	 */
> +	__virtio32 max_secure_erase_sectors;
> +	/*
> +	 * The maximum number of secure erase segments in a
> +	 * secure erase command.
> +	 */
> +	__virtio32 max_secure_erase_seg;
> +	/* Secure erase commands must be aligned to this number of sectors. */
> +	__virtio32 secure_erase_sector_alignment;
> +
>  } __attribute__((packed));
>  
>  /*
> @@ -155,6 +171,9 @@ struct virtio_blk_config {
>  /* Write zeroes command */
>  #define VIRTIO_BLK_T_WRITE_ZEROES	13
>  
> +/* Secure erase command */
> +#define VIRTIO_BLK_T_SECURE_ERASE	14
> +
>  #ifndef VIRTIO_BLK_NO_LEGACY
>  /* Barrier before this op. */
>  #define VIRTIO_BLK_T_BARRIER	0x80000000
> -- 
> 2.32.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add SECURE ERASE command support

[Alvaro Karsz]

Thanks for the reply.

> why minimum?

>  why is that?

This was discussed in the previous version
(https://www.spinics.net/lists/linux-virtualization/msg58232.html).
As far as I know, the Linux kernel uses the same "max segments" value
for a discard and a secure erase command.
In the first version, I ignored the max_secure_erase_seg and
secure_erase_sector_alignment config fields (just like
max_write_zeroes_seg and write_zeroes_may_unmap are ignored in the
write zeros command implementation).

It was suggested to use the minimum "max segments" value if both
VIRTIO_BLK_F_SECURE_ERASE and VIRTIO_BLK_F_DISCARD are negotiated.
The same is true for the sector alignment values.

> is this logic repeating code from below?

I'm not sure what you mean.
The idea is:
At this point, the VIRTIO_BLK_F_DISCARD fields were read from the
virtio config (if VIRTIO_BLK_F_DISCARD is negotiated).
If max_discard_segs is 0, VIRTIO_BLK_F_DISCARD is not negotiated (or
set to 0), so we should use the max_secure_erase_seg value as
max_discard_segs.


> Always? What's going on here?
> which versions handled max_secure_erase_seg == 0?

This comment is from the VIRTIO_BLK_F_DISCARD implementation.
I added the max_secure_erase_seg part since I could not find how to
handle the case when max_secure_erase_seg is 0 in the spec.
So, like with the VIRTIO_BLK_F_DISCARD implementation, I'm setting the
value to sg_elems.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add SECURE ERASE command support

[Michael S. Tsirkin]

On Sun, Sep 18, 2022 at 05:01:53PM +0300, Alvaro Karsz wrote:
> Thanks for the reply.
> 
> > why minimum?
> 
> >  why is that?
> 
> This was discussed in the previous version
> (https://www.spinics.net/lists/linux-virtualization/msg58232.html).
> As far as I know, the Linux kernel uses the same "max segments" value
> for a discard and a secure erase command.
> In the first version, I ignored the max_secure_erase_seg and
> secure_erase_sector_alignment config fields (just like
> max_write_zeroes_seg and write_zeroes_may_unmap are ignored in the
> write zeros command implementation).
> 
> It was suggested to use the minimum "max segments" value if both
> VIRTIO_BLK_F_SECURE_ERASE and VIRTIO_BLK_F_DISCARD are negotiated.
> The same is true for the sector alignment values.

sounds good. Add a code comment?

> > is this logic repeating code from below?
> 
> I'm not sure what you mean.
> The idea is:
> At this point, the VIRTIO_BLK_F_DISCARD fields were read from the
> virtio config (if VIRTIO_BLK_F_DISCARD is negotiated).
> If max_discard_segs is 0, VIRTIO_BLK_F_DISCARD is not negotiated (or
> set to 0), so we should use the max_secure_erase_seg value as
> max_discard_segs.

yes but I now see two places that seem to include this logic.

> 
> > Always? What's going on here?
> > which versions handled max_secure_erase_seg == 0?
> 
> This comment is from the VIRTIO_BLK_F_DISCARD implementation.
> I added the max_secure_erase_seg part since I could not find how to
> handle the case when max_secure_erase_seg is 0 in the spec.
> So, like with the VIRTIO_BLK_F_DISCARD implementation, I'm setting the
> value to sg_elems.

I am not 100% sure. Two options:
1- Add a validate callback and clear VIRTIO_BLK_F_SECURE_ERASE.
2- Alternatively, fail probe.

which is preferable depends on how bad is it if host sets
VIRTIO_BLK_F_SECURE_ERASE but guest does not use it.

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add SECURE ERASE command support

[Alvaro Karsz]

> sounds good. Add a code comment?

I will.

>  yes but I now see two places that seem to include this logic.


Yes, this is because the same logic is applied on 2 different pairs.

* secure_erase_sector_alignment and discard_sector_alignment are used
to calculate  q->limits.discard_granularity.
* max_discard_seg and max_secure_erase_seg are used to calculate
max_discard_segments.

> I am not 100% sure. Two options:
> 1- Add a validate callback and clear VIRTIO_BLK_F_SECURE_ERASE.
> 2- Alternatively, fail probe.


Good ideas.
2- Do you think that something like that should be mentioned in the
spec? or should be implementation specific?

How about setting the value to 1? (which is the minimum usable value)

> which is preferable depends on how bad is it if host sets
> VIRTIO_BLK_F_SECURE_ERASE but guest does not use it.


I'm not sure if it is that bad.
If the value is 0, sg_elems is used.
sg_elems is either 1 (if VIRTIO_BLK_F_SEG_MAX is not negotiated), or
seg_max (virtio config).

"""
err = virtio_cread_feature(vdev, VIRTIO_BLK_F_SEG_MAX,
                                          struct virtio_blk_config, seg_max,
                                          &sg_elems);
/* We need at least one SG element, whatever they say. */
if (err || !sg_elems)
         sg_elems = 1;
"""

So the only "danger" that I can think of is if a device negotiates
VIRTIO_BLK_F_SEG_MAX and  VIRTIO_BLK_F_SECURE_ERASE, sets
max_secure_erase_seg to 0 (I'm not sure what is the purpose, since
this is meaningless), and can't handle secure erase commands with
seg_max segments.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add SECURE ERASE command support

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 1753 bytes --]

On Mon, Aug 29, 2022 at 11:23:13AM +0300, Alvaro Karsz wrote:
> @@ -1075,6 +1068,57 @@ static int virtblk_probe(struct virtio_device *vdev)
>  		blk_queue_max_write_zeroes_sectors(q, v ? v : UINT_MAX);
>  	}
>  
> +	if (virtio_has_feature(vdev, VIRTIO_BLK_F_SECURE_ERASE)) {
> +		/* The discard alignment value should be the minimum between
> +		 * secure_erase_sector_alignment and discard_sector_alignment
> +		 * (if VIRTIO_BLK_F_DISCARD is negotiated).
> +		 */
> +		virtio_cread(vdev, struct virtio_blk_config,
> +			     secure_erase_sector_alignment, &v);
> +		if (v) {
> +			if (discard_granularity)
> +				discard_granularity = min(discard_granularity, v);
> +			else
> +				discard_granularity = v;

This can be simplified with min_not_zero().

> +		}
> +
> +		virtio_cread(vdev, struct virtio_blk_config,
> +			     max_secure_erase_sectors, &v);
> +		blk_queue_max_secure_erase_sectors(q, v ? v : UINT_MAX);
> +
> +		/* The max discard segments value should be the minimum between
> +		 * max_secure_erase_seg and max_discard_seg (if VIRTIO_BLK_F_DISCARD
> +		 * is negotiated).
> +		 */
> +		virtio_cread(vdev, struct virtio_blk_config,
> +			     max_secure_erase_seg, &v);
> +		if (v) {
> +			if (max_discard_segs)
> +				max_discard_segs = min(max_discard_segs, v);
> +			else
> +				max_discard_segs = v;

Same here.

> +		}
> +	}
> +
> +	if (virtio_has_feature(vdev, VIRTIO_BLK_F_DISCARD) ||
> +	    virtio_has_feature(vdev, VIRTIO_BLK_F_SECURE_ERASE)) {

It's worth including a comment here that the discard and secure erase
limits are combined because the Linux block layer only has one limit
value. If the block layer supported independent limit values we wouldn't
need to do this.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add SECURE ERASE command support

[Alvaro Karsz]

Thanks for the reply.

> This can be simplified with min_not_zero().

Ok, I will do it in the next version.

> It's worth including a comment here that the discard and secure erase
> limits are combined because the Linux block layer only has one limit
> value. If the block layer supported independent limit values we wouldn't
> need to do this.

Ok.

I'll send a new version once we agree on the max_secure_erase_seg = 0 scenario.
Do you have an opinion on that?
Do you think that using sg_elems as the number of secure erase/discard
segments when the value in the virtio config is 0 is good enough?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add SECURE ERASE command support

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 2105 bytes --]

On Sun, Sep 18, 2022 at 07:07:34PM +0300, Alvaro Karsz wrote:
> > sounds good. Add a code comment?
> 
> I will.
> 
> >  yes but I now see two places that seem to include this logic.
> 
> 
> Yes, this is because the same logic is applied on 2 different pairs.
> 
> * secure_erase_sector_alignment and discard_sector_alignment are used
> to calculate  q->limits.discard_granularity.
> * max_discard_seg and max_secure_erase_seg are used to calculate
> max_discard_segments.
> 
> > I am not 100% sure. Two options:
> > 1- Add a validate callback and clear VIRTIO_BLK_F_SECURE_ERASE.
> > 2- Alternatively, fail probe.
> 
> 
> Good ideas.
> 2- Do you think that something like that should be mentioned in the
> spec? or should be implementation specific?
> 
> How about setting the value to 1? (which is the minimum usable value)
> 
> > which is preferable depends on how bad is it if host sets
> > VIRTIO_BLK_F_SECURE_ERASE but guest does not use it.
> 
> 
> I'm not sure if it is that bad.
> If the value is 0, sg_elems is used.
> sg_elems is either 1 (if VIRTIO_BLK_F_SEG_MAX is not negotiated), or
> seg_max (virtio config).
> 
> """
> err = virtio_cread_feature(vdev, VIRTIO_BLK_F_SEG_MAX,
>                                           struct virtio_blk_config, seg_max,
>                                           &sg_elems);
> /* We need at least one SG element, whatever they say. */
> if (err || !sg_elems)
>          sg_elems = 1;
> """
> 
> So the only "danger" that I can think of is if a device negotiates
> VIRTIO_BLK_F_SEG_MAX and  VIRTIO_BLK_F_SECURE_ERASE, sets
> max_secure_erase_seg to 0 (I'm not sure what is the purpose, since
> this is meaningless), and can't handle secure erase commands with
> seg_max segments.

Given that SECURE ERASE is new and the VIRTIO spec does not define
special behavior for 0, I think the virtio_blk driver should be strict.

There's no need to work around existing broken devices. I would fail
probing the device. This will encourage device implementors to provide a
usable value instead of 0.

Stefan

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add SECURE ERASE command support

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 1018 bytes --]

On Mon, Sep 19, 2022 at 09:09:05PM +0300, Alvaro Karsz wrote:
> Thanks for the reply.
> 
> > This can be simplified with min_not_zero().
> 
> Ok, I will do it in the next version.
> 
> > It's worth including a comment here that the discard and secure erase
> > limits are combined because the Linux block layer only has one limit
> > value. If the block layer supported independent limit values we wouldn't
> > need to do this.
> 
> Ok.
> 
> I'll send a new version once we agree on the max_secure_erase_seg = 0 scenario.
> Do you have an opinion on that?
> Do you think that using sg_elems as the number of secure erase/discard
> segments when the value in the virtio config is 0 is good enough?
> 

Okay, I have replied in the max_secure_erase_seg sub-thread. I think
probing the device should fail if the value is 0. There are no existing
non-compliant devices that we need to be compatible with - let's
encourage device implementors to report usable max_secure_erase_seg
values.

Stefan

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add SECURE ERASE command support

[Alvaro Karsz]

> Okay, I have replied in the max_secure_erase_seg sub-thread. I think
> probing the device should fail if the value is 0. There are no existing
> non-compliant devices that we need to be compatible with - let's
> encourage device implementors to report usable max_secure_erase_seg
> values.

Ok, I will include it in the new version.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add SECURE ERASE command support

[Michael S. Tsirkin]

On Tue, Sep 20, 2022 at 02:11:40PM -0400, Stefan Hajnoczi wrote:
> On Mon, Sep 19, 2022 at 09:09:05PM +0300, Alvaro Karsz wrote:
> > Thanks for the reply.
> > 
> > > This can be simplified with min_not_zero().
> > 
> > Ok, I will do it in the next version.
> > 
> > > It's worth including a comment here that the discard and secure erase
> > > limits are combined because the Linux block layer only has one limit
> > > value. If the block layer supported independent limit values we wouldn't
> > > need to do this.
> > 
> > Ok.
> > 
> > I'll send a new version once we agree on the max_secure_erase_seg = 0 scenario.
> > Do you have an opinion on that?
> > Do you think that using sg_elems as the number of secure erase/discard
> > segments when the value in the virtio config is 0 is good enough?
> > 
> 
> Okay, I have replied in the max_secure_erase_seg sub-thread. I think
> probing the device should fail if the value is 0. There are no existing
> non-compliant devices that we need to be compatible with - let's
> encourage device implementors to report usable max_secure_erase_seg
> values.
> 
> Stefan

I agree, but do we have to fail probe? Are there security concerns
if secure erase functionality is just disabled in this case?


-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_blk: add SECURE ERASE command support

[Michael S. Tsirkin]

On Tue, Sep 20, 2022 at 02:10:37PM -0400, Stefan Hajnoczi wrote:
> On Sun, Sep 18, 2022 at 07:07:34PM +0300, Alvaro Karsz wrote:
> > > sounds good. Add a code comment?
> > 
> > I will.
> > 
> > >  yes but I now see two places that seem to include this logic.
> > 
> > 
> > Yes, this is because the same logic is applied on 2 different pairs.
> > 
> > * secure_erase_sector_alignment and discard_sector_alignment are used
> > to calculate  q->limits.discard_granularity.
> > * max_discard_seg and max_secure_erase_seg are used to calculate
> > max_discard_segments.
> > 
> > > I am not 100% sure. Two options:
> > > 1- Add a validate callback and clear VIRTIO_BLK_F_SECURE_ERASE.
> > > 2- Alternatively, fail probe.
> > 
> > 
> > Good ideas.
> > 2- Do you think that something like that should be mentioned in the
> > spec? or should be implementation specific?
> > 
> > How about setting the value to 1? (which is the minimum usable value)
> > 
> > > which is preferable depends on how bad is it if host sets
> > > VIRTIO_BLK_F_SECURE_ERASE but guest does not use it.
> > 
> > 
> > I'm not sure if it is that bad.
> > If the value is 0, sg_elems is used.
> > sg_elems is either 1 (if VIRTIO_BLK_F_SEG_MAX is not negotiated), or
> > seg_max (virtio config).
> > 
> > """
> > err = virtio_cread_feature(vdev, VIRTIO_BLK_F_SEG_MAX,
> >                                           struct virtio_blk_config, seg_max,
> >                                           &sg_elems);
> > /* We need at least one SG element, whatever they say. */
> > if (err || !sg_elems)
> >          sg_elems = 1;
> > """
> > 
> > So the only "danger" that I can think of is if a device negotiates
> > VIRTIO_BLK_F_SEG_MAX and  VIRTIO_BLK_F_SECURE_ERASE, sets
> > max_secure_erase_seg to 0 (I'm not sure what is the purpose, since
> > this is meaningless), and can't handle secure erase commands with
> > seg_max segments.
> 
> Given that SECURE ERASE is new and the VIRTIO spec does not define
> special behavior for 0, I think the virtio_blk driver should be strict.
> 
> There's no need to work around existing broken devices. I would fail
> probing the device. This will encourage device implementors to provide a
> usable value instead of 0.
> 
> Stefan



What I worry about is that down the road we might want to add
special meaning to currently unused values.
If doing that just clears VIRTIO_BLK_F_SECURE_ERASE then
we have forward compatibility. If it fails probe then we
won't be able to do use these values.

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization