Virtualization - Sep 2022 - [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.

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