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

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Nov 15, 2021 at 12:52:00PM +0800, yadong.qi@intel.com wrote:
> From: Yadong Qi <yadong.qi@intel.com>
> 
> Add new virtio feature: VIRTIO_BLK_F_SECDISCARD.
> Add new virtio command: VIRTIO_BLK_T_SECDISCARD.
> 
> 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.qi@intel.com>
> ---
>  hw/block/virtio-blk.c                       | 26 +++++++++++++++++----
>  include/standard-headers/linux/virtio_blk.h |  4 ++++


Any changes to standard headers need to go to linux and virtio TC.

>  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)
>  {
>      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;
>      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));
>      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);
> +
>      if (virtio_has_feature(s->host_features, VIRTIO_BLK_F_WRITE_ZEROES) &&
>          (!conf->max_write_zeroes_sectors ||
>           conf->max_write_zeroes_sectors > BDRV_REQUEST_MAX_SECTORS)) {
> @@ -1307,6 +1323,8 @@ static Property virtio_blk_properties[] = {
>                       conf.report_discard_granularity, true),
>      DEFINE_PROP_BIT64("write-zeroes", VirtIOBlock, host_features,
>                        VIRTIO_BLK_F_WRITE_ZEROES, true),
> +    DEFINE_PROP_BIT64("secdiscard", VirtIOBlock, host_features,
> +                      VIRTIO_BLK_F_SECDISCARD, false),
>      DEFINE_PROP_UINT32("max-discard-sectors", VirtIOBlock,
>                         conf.max_discard_sectors, BDRV_REQUEST_MAX_SECTORS),
>      DEFINE_PROP_UINT32("max-write-zeroes-sectors", VirtIOBlock,
> diff --git a/include/standard-headers/linux/virtio_blk.h b/include/standard-headers/linux/virtio_blk.h
> index 2dcc90826a..c55a07840c 100644
> --- a/include/standard-headers/linux/virtio_blk.h
> +++ b/include/standard-headers/linux/virtio_blk.h
> @@ -40,6 +40,7 @@
>  #define VIRTIO_BLK_F_MQ		12	/* support more than one vq */
>  #define VIRTIO_BLK_F_DISCARD	13	/* DISCARD is supported */
>  #define VIRTIO_BLK_F_WRITE_ZEROES	14	/* WRITE ZEROES is supported */
> +#define VIRTIO_BLK_F_SECDISCARD	15	/* WRITE ZEROES is supported */

Surely not.


>  
>  /* Legacy feature bits */
>  #ifndef VIRTIO_BLK_NO_LEGACY
> @@ -153,6 +154,9 @@ struct virtio_blk_config {
>  /* Write zeroes command */
>  #define VIRTIO_BLK_T_WRITE_ZEROES	13
>  
> +/* Secure discard command */
> +#define VIRTIO_BLK_T_SECDISCARD	14
> +
>  #ifndef VIRTIO_BLK_NO_LEGACY
>  /* Barrier before this op. */
>  #define VIRTIO_BLK_T_BARRIER	0x80000000
> -- 
> 2.25.1