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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- 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