RE: [PATCH 2/2] virtio-blk: support BLKSECDISCARD

["Qi, Yadong" <yadong.qi@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?
> 
Not yet. I will draft a proposal to virtio-comment if no big concern of this patch
From maintainer.
> >
> >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.
> 
Make sense.

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

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

> 
> >
> >         err_status = virtio_blk_handle_discard_write_zeroes(req, &dwz_hdr,
> >-                                                            is_write_zeroes);
> >+                                                            is_write_zeroes,
> >+
> >+ is_secdiscard);


> >
> >+    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)
Yes, BDRV_O_SECDISCARD=(secdiscard=="on") ? 1 : 0;

> 
> Otherwise what is the purpose of the "secdiscard" property?
I cannot find a good method to detect whether host device support BLKSECDISCARD.
So I add this "secdiscard" property to explicitly enable this feature.

Best Regard
Yadong
> 
> Thanks,
> Stefano