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

From: "Michael S. Tsirkin" <mst@redhat.com>
To: Alvaro Karsz <alvaro.karsz@solid-run.com>
Cc: Jens Axboe <axboe@kernel.dk>, Paolo Bonzini <pbonzini@redhat.com>,
	stefanha@redhat.com, virtualization@lists.linux-foundation.org
Subject: Re: [PATCH v2] virtio_blk: add SECURE ERASE command support
Date: Sun, 18 Sep 2022 11:13:51 -0400	[thread overview]
Message-ID: <20220918110951-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <CAJs=3_B4sTo-X9ZkqAZjzLi0EGRNW_jwUtgUBj17p907qVkNoQ@mail.gmail.com>

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization