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

["Michael S. Tsirkin" <mst@redhat.com>]

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