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

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

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