Alvaro Karsz
2022-Sep-18 16:07 UTC
[PATCH v2] virtio_blk: add SECURE ERASE command support
> 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.
Stefan Hajnoczi
2022-Sep-20 18:10 UTC
[PATCH v2] virtio_blk: add SECURE ERASE command support
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 -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 488 bytes Desc: not available URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20220920/d700db66/attachment.sig>