Virtualization - Sep 2022 - [PATCH v2] virtio_blk: add SECURE ERASE command support

On Sun, Sep 18, 2022 at 05:01:53PM +0300, Alvaro Karsz
wrote:
> 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.
sounds good. Add a code comment?
> > 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.
yes but I now see two places that seem to include this logic.
> 
> > 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.
I am not 100% sure. Two options:
1- Add a validate callback and clear VIRTIO_BLK_F_SECURE_ERASE.
2- Alternatively, fail probe.

which is preferable depends on how bad is it if host sets
VIRTIO_BLK_F_SECURE_ERASE but guest does not use it.

-- 
MST

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