Re: [PATCH v2] virtio-blk: add secure discard feature to specification

[Stefan Hajnoczi <stefanha@redhat.com>]

On Wed, Nov 24, 2021 at 10:47:57AM +0800, yadong.qi@intel.com wrote:
> From: Yadong Qi <yadong.qi@intel.com>
> 
> There are user requests to use BLKSECDISCARD on virtio-blk device.

If you respin, please s/BLKSECDISCARD/the Linux BLKSECDISCARD ioctl/
because the VIRTIO community is broader than Linux. Some people might
not know what BLKSECDISCARD is.

> Hence in this proposal, extend virtio-blk protocol to support secure
> discard command.
> 
> Introduced new feature flag and command type:
>     VIRTIO_BLK_F_SECDISCARD
>     VIRTIO_BLK_T_SECDISCARD
> 
> This feature is a passthrough feature on backend because it is hard
> to emulate a secure discard. So virtio-blk will report this feature
> to guest OS if backend device support such kind of feature. And
> when guest OS issues a secure discard command, backend driver will
> passthrough the command to host device blocks.
> 
> Introduced new fileds in virtio_blk_config for secure discard commands
> to separate with discard commands:
> struct virtio_blk_config {
>     ...
>     max_secdiscard_sectors;
>     max_secdiscard_seg;
>     secdiscard_sector_alignment;
>     ...
> };
> 
> v1 -> v2:
> - add separated queue limits for secure discard.
> 
> Signed-off-by: Yadong Qi <yadong.qi@intel.com>
> ---
>  content.tex | 41 +++++++++++++++++++++++++++++++++--------
>  1 file changed, 33 insertions(+), 8 deletions(-)
> 
> diff --git a/content.tex b/content.tex
> index 5d112af..686389b 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -4435,6 +4435,11 @@ \subsection{Feature bits}\label{sec:Device Types / Block Device / Feature bits}
>  
>  \item[VIRTIO_BLK_F_LIFETIME (15)] Device supports providing storage lifetime
>       information.
> +
> +\item[VIRTIO_BLK_F_SECDISCARD (16)] Device can support secure discard command,

For consistency with VIRTIO_BLK_F_LIFETIME wording above:

  Device supports secure discard command,

> +    maximum discard sectors size in \field{max_secdiscard_sectors} and maximum

s/sectors size/sector count/

> +    discard segment number in \field{max_secdiscard_seg}.
> +
>  \end{description}
>  
>  \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Block Device / Feature bits / Legacy Interface: Feature bits}
> @@ -4463,7 +4468,9 @@ \subsection{Device configuration layout}\label{sec:Device Types / Block Device /
>  \field{discard_sector_alignment} are expressed in 512-byte units if the
>  VIRTIO_BLK_F_DISCARD feature bit is negotiated. The \field{max_write_zeroes_sectors}
>  is expressed in 512-byte units if the VIRTIO_BLK_F_WRITE_ZEROES feature
> -bit is negotiated.
> +bit is negotiated. The parameters in the configuration space of the device
> +\field{max_secdiscard_sectors} \field{secdiscard_sector_aligment} are expressed

s/secdiscard_sector_aligment/secdiscard_sector_alignment/

> +in 512-byte units if the VIRTIO_BLK_F_SECDISCARD feature bit is negotiated.
>  
>  \begin{lstlisting}
>  struct virtio_blk_config {
> @@ -4494,6 +4501,9 @@ \subsection{Device configuration layout}\label{sec:Device Types / Block Device /
>          le32 discard_sector_alignment;
>          le32 max_write_zeroes_sectors;
>          le32 max_write_zeroes_seg;
> +        le32 max_secdiscard_sectors;
> +        le32 max_secdiscard_seg;
> +        le32 secdiscard_sector_alignment;
>          u8 write_zeroes_may_unmap;
>          u8 unused1[3];
>  };

Please add new fields at the end of struct virtio_blk_config so the
offset of write_zeroes_may_unmap remains unchanged. This is necessary
for compatibility with existing drivers that are not aware of
VIRTIO_BLK_F_SECDISCARD.

> @@ -4552,6 +4562,13 @@ \subsection{Device Initialization}\label{sec:Device Types / Block Device / Devic
>  \item If the VIRTIO_BLK_F_MQ feature is negotiated, \field{num_queues} field
>      can be read to determine the number of queues.
>  
> +\item If the VIRTIO_BLK_F_SECDISCARD feature is negotiated,
> +    \field{max_secdiscard_sectors} and \field{max_secdiscard_seg} can be read
> +    to determine the maximum secure discard sectors and maximum number of
> +    secure discard segments for the block driver to use.
> +    \field{secdiscard_sector_alignment} can be used by OS when splitting a
> +    request based on alignment.
> +
>  \end{enumerate}
>  
>  \drivernormative{\subsubsection}{Device Initialization}{Device Types / Block Device / Device Initialization}
> @@ -4619,7 +4636,8 @@ \subsection{Device Operation}\label{sec:Device Types / Block Device / Device Ope
>  The type of the request is either a read (VIRTIO_BLK_T_IN), a write
>  (VIRTIO_BLK_T_OUT), a discard (VIRTIO_BLK_T_DISCARD), a write zeroes
>  (VIRTIO_BLK_T_WRITE_ZEROES), a flush (VIRTIO_BLK_T_FLUSH), a get device ID
> -string command (VIRTIO_BLK_T_GET_ID), or a get device lifetime command
> +string command (VIRTIO_BLK_T_GET_ID), a secure discard
> +(VIRTIO_BLK_T_SECDISCARD), or a get device lifetime command
>  (VIRTIO_BLK_T_GET_LIFETIME).

Is the meaning of secure discard widely understood or does it make sense
to define it in more detail?

Attachment: signature.asc
Description: PGP signature