On Mon, Nov 15, 2021 at 12:52:00PM +0800, yadong...@intel.com wrote:
From: Yadong Qi <yadong...@intel.com>
Add new virtio feature: VIRTIO_BLK_F_SECDISCARD.
Add new virtio command: VIRTIO_BLK_T_SECDISCARD.
Has a proposal been sent out yet to virtio-comment mailing list for
discussing these specification changes?
This feature is disabled by default, it will check the backend
bs->open_flags & BDRV_O_SECDISCARD, enable it if BDRV_O_SECDISCARD
is supported.
Signed-off-by: Yadong Qi <yadong...@intel.com>
---
hw/block/virtio-blk.c | 26 +++++++++++++++++----
include/standard-headers/linux/virtio_blk.h | 4 ++++
2 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index dbc4c5a3cd..7bc3484521 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -536,7 +536,8 @@ static bool virtio_blk_sect_range_ok(VirtIOBlock *dev,
}
static uint8_t virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req,
- struct virtio_blk_discard_write_zeroes *dwz_hdr, bool is_write_zeroes)
+ struct virtio_blk_discard_write_zeroes *dwz_hdr, bool is_write_zeroes,
+ bool is_secdiscard)
Since the function now handles 3 commands, I'm thinking if it's better
to pass the command directly and then make a switch instead of using 2
booleans.
{
VirtIOBlock *s = req->dev;
VirtIODevice *vdev = VIRTIO_DEVICE(s);
@@ -577,8 +578,8 @@ static uint8_t
virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req,
goto err;
}
+ int blk_aio_flags = 0;
Maybe better to move it to the beginning of the function.
if (is_write_zeroes) { /* VIRTIO_BLK_T_WRITE_ZEROES */
- int blk_aio_flags = 0;
if (flags & VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP) {
blk_aio_flags |= BDRV_REQ_MAY_UNMAP;
@@ -600,7 +601,12 @@ static uint8_t
virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req,
goto err;
}
- blk_aio_pdiscard(s->blk, sector << BDRV_SECTOR_BITS, bytes, 0,
+ if (is_secdiscard) {
+ blk_aio_flags |= BDRV_REQ_SECDISCARD;
+ }
+
+ blk_aio_pdiscard(s->blk, sector << BDRV_SECTOR_BITS, bytes,
+ blk_aio_flags,
virtio_blk_discard_write_zeroes_complete, req);
}
@@ -622,6 +628,7 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req,
MultiReqBuffer *mrb)
unsigned out_num = req->elem.out_num;
VirtIOBlock *s = req->dev;
VirtIODevice *vdev = VIRTIO_DEVICE(s);
+ bool is_secdiscard = false;
if (req->elem.out_num < 1 || req->elem.in_num < 1) {
virtio_error(vdev, "virtio-blk missing headers");
@@ -722,6 +729,9 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req,
MultiReqBuffer *mrb)
* VIRTIO_BLK_T_OUT flag set. We masked this flag in the switch statement,
* so we must mask it for these requests, then we will check if it is set.
*/
+ case VIRTIO_BLK_T_SECDISCARD & ~VIRTIO_BLK_T_OUT:
+ is_secdiscard = true;
+ __attribute__((fallthrough));
We can use QEMU_FALLTHROUGH here.
case VIRTIO_BLK_T_DISCARD & ~VIRTIO_BLK_T_OUT:
case VIRTIO_BLK_T_WRITE_ZEROES & ~VIRTIO_BLK_T_OUT:
{
@@ -752,7 +762,8 @@ static int virtio_blk_handle_request(VirtIOBlockReq
*req, MultiReqBuffer *mrb)
}
err_status = virtio_blk_handle_discard_write_zeroes(req, &dwz_hdr,
- is_write_zeroes);
+ is_write_zeroes,
+ is_secdiscard);
if (err_status != VIRTIO_BLK_S_OK) {
virtio_blk_req_complete(req, err_status);
virtio_blk_free_request(req);
@@ -1201,6 +1212,11 @@ static void virtio_blk_device_realize(DeviceState *dev,
Error **errp)
return;
}
+ if (blk_get_flags(conf->conf.blk) & BDRV_O_SECDISCARD)
+ virtio_add_feature(&s->host_features,
VIRTIO_BLK_F_SECDISCARD);
+ else
+ virtio_clear_feature(&s->host_features, VIRTIO_BLK_F_SECDISCARD);
+
IIUC here we set or not the feature if BDRV_O_SECDISCARD is set.
Should we keep it disabled if "secdiscard" is false? (e.g. to avoid
migration problems)
Otherwise what is the purpose of the "secdiscard" property?
Thanks,
Stefano
