Michael S. Tsirkin
2022-Sep-18 13:30 UTC
[PATCH v2] virtio_blk: add SECURE ERASE command support
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 at 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
Alvaro Karsz
2022-Sep-18 14:01 UTC
[PATCH v2] virtio_blk: add SECURE ERASE command support
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.