Re: [PATCH v1] virtio-blk: Add inline encryption support

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 9512 bytes --]

On Tue, Jan 27, 2026 at 10:20:32PM +0800, Linlin Zhang wrote:
> From: linlzhan <quic_linlzhan@quicinc.com>
> 
> Inline encryption on virtio block can only be supported when
> the new feature bit VIRTIO_BLK_F_ICE is negotiated.
> 
> Extend struct virtio_blk_config and struct virtio_blk_req,
> so that crypto capabilities can be got in the frontend and
> encryption metadata can be sent to the backend, together with
> each I/O transaction.

This looks like a Self-Encrypting Drives feature along the lines of TCG
Opal:
https://en.wikipedia.org/wiki/Opal_Storage_Specification

Would it make sense to implement TCG Opal instead of defining a new
interface? That would make this more familiar to users and simplify
integration into existing tools like sedutil and cryptsetup (e.g. by
supporting the <linux/sed-opal.h> ioctl interface).

> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/238
> 
> Change-Id: Ic23b2137e5d9a599d826e06c279f1b614d79abdf
> Signed-off-by: linlzhan <quic_linlzhan@quicinc.com>
> ---
>  device-types/blk/description.tex | 69 ++++++++++++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
> 
> diff --git a/device-types/blk/description.tex b/device-types/blk/description.tex
> index 2712ada..23d8dc0 100644
> --- a/device-types/blk/description.tex
> +++ b/device-types/blk/description.tex
> @@ -66,6 +66,11 @@ \subsection{Feature bits}\label{sec:Device Types / Block Device / Feature bits}
>  	(ZNS). For brevity, these standard documents are referred as "ZBD standards"
>  	from this point on in the text.
>  
> +\item[VIRTIO_BLK_F_ICE(22)] Inline Crypto Extensions are supported. When this
> +     is negotiated, the device exposes crypto characteristics in configuration
> +     space and the driver SHALL provide an extended request header containing a

SHALL, MUST, MAY, etc are only used in the normative sections of the
spec.

Why "SHALL"? Does this mean the device must be prepared to receive
requests without the payload field when VIRTIO_BLK_F_ICE is negotiated?
How should the device behave in that case: fail the request or perform
I/O without crypto?

> +     crypto payload for block I/O.
> +
>  \end{description}
>  
>  \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Block Device / Feature bits / Legacy Interface: Feature bits}
> @@ -128,6 +133,11 @@ \subsection{Device configuration layout}\label{sec:Device Types / Block Device /
>                  u8 model;
>                  u8 unused2[3];
>          } zoned;
> +        struct virtio_blk_crypto_characteristics {
> +                __virtio16 slot_info;
> +                __virtio16 reserved;
> +                __virtio32 capability;
> +        } crypto;
>  };
>  \end{lstlisting}
>  
> @@ -215,6 +225,25 @@ \subsection{Device configuration layout}\label{sec:Device Types / Block Device /
>  terminated by the device with a "zone resources exceeded" error as defined for
>  specific commands later.
>  
> +If the VIRTIO_BLK_F_ICE feature is negotiated, then in
> +\field{virtio_blk_crypto_characteristics},
> +\begin{itemize}
> +\item \field{slot_info} value packs two 8-bits values:
> +    \begin{itemize}
> +        \item Bits~\[15:8] (\emph{max\_slots}): the maximum number of supported
> +            crypto key slots.
> +        \item Bits~\[7:0] (\emph{slot\_offset}): an offset applied to slot numbering.
> +    \end{itemize}
> +\item \field{capability} value packs four 8-bits values:
> +    \begin{itemize}
> +        \item Bits~\[31:24]: crypto algorithm id.
> +        \item Bits~\[23:16]: mask of data unit size.
> +        \item Bits~\[15:8]: crypto key size.
> +        \item Bits~\[7:0]: unused.
> +    \end{itemize}

Why are these fields packed? Configuration Space can have u8 fields.

These fields are not sufficiently documented. Where are the crypto
algorithm ids listed, etc?

How can a device support multiple algorithms? I think Configuration
Space may not be flexible enough for this. You could introduce a
GET_CRYPTO_INFO request type that allows the driver to fetch arrays of
crypto algorithm characteristics.

> +\end{itemize}
> +
> +
>  \subsubsection{Legacy Interface: Device configuration layout}\label{sec:Device Types / Block Device / Device configuration layout / Legacy Interface: Device configuration layout}
>  When using the legacy interface, transitional devices and drivers
>  MUST format the fields in struct virtio_blk_config
> @@ -278,6 +307,10 @@ \subsection{Device Initialization}\label{sec:Device Types / Block Device / Devic
>      \field{zoned} can be read by the driver to determine the zone
>      characteristics of the device. All \field{zoned} fields are read-only.
>  
> +\item If the VIRTIO_BLK_F_ICE feature is negotiated, the fields in
> +    \field{crypto} can be read by the driver to determine the inline crypto
> +    characteristics of the device. All \field{crypto} fields are read-only.
> +
>  \end{enumerate}
>  
>  \drivernormative{\subsubsection}{Device Initialization}{Device Types / Block Device / Device Initialization}
> @@ -317,6 +350,9 @@ \subsection{Device Initialization}\label{sec:Device Types / Block Device / Devic
>      driver SHOULD ignore all other fields in \field{zoned}.
>  \end{itemize}
>  
> +If the VIRTIO_BLK_F_ICE feature is negotiated, then the driver must validate
> +    the max_slots in \field{slot_info} before the slot usage.
> +
>  \devicenormative{\subsubsection}{Device Initialization}{Device Types / Block Device / Device Initialization}
>  
>  Devices SHOULD always offer VIRTIO_BLK_F_FLUSH, and MUST offer it
> @@ -402,6 +438,16 @@ \subsection{Device Initialization}\label{sec:Device Types / Block Device / Devic
>  \item the device MUST initialize padding bytes \field{unused2} to 0.
>  \end{itemize}
>  
> +If the VIRTIO_BLK_F_ICE feature is negotiated, then the fields in \field{cryto}

s/cryto/crypto/

> +struct in the configuration space MUST be set by the device.
> +\begin{itemize}
> +\item the \field{slot_info} field of \field{crypto} MUST be set by the device to a
> +    max_slots in the higher 8 bits and slot_offset in the lower 8 bits.
> +
> +\item the \field{capability} field of \field{crypto} MUST be set by the device
> +    to a crypto capability read from the storage register.
> +\end{itemize}
> +
>  \subsubsection{Legacy Interface: Device Initialization}\label{sec:Device Types / Block Device / Device Initialization / Legacy Interface: Device Initialization}
>  
>  Because legacy devices do not have FEATURES_OK, transitional devices
> @@ -436,6 +482,13 @@ \subsection{Device Operation}\label{sec:Device Types / Block Device / Device Ope
>          le32 type;
>          le32 reserved;
>          le64 sector;
> +        struct virtio_blk_crypto_payload {
> +            u8 slot;
> +            u8 activate;
> +            le16 reserved1;
> +            le32 reserved2;
> +            le64 data_unit_num;
> +        } payload;
>          u8 data[];
>          u8 status;
>  };
> @@ -463,6 +516,20 @@ \subsection{Device Operation}\label{sec:Device Types / Block Device / Device Ope
>  the read or write is to occur. This field is unused and set to 0 for
>  commands other than read, write and some zone operations.
>  
> +The \field{payload} consists of the encryption information for current
> +request. It need to be set by the driver only when the feature VIRTIO_BLK_F_ICE
> +is negotiated.

"set" is ambiguous: does it meaning filling in the fields or does it
mean the fields are only present when VIRTIO_BLK_F_ICE is negotiated
(this distinction is important if other features add more fields after
payload in the future).

The sentence could be reworded:

  It is only present when the VIRTIO_BLK_F_ICE feature is negotiated and
  \field{type} is VIRTIO_BLK_T_IN or VIRTIO_BLK_T_OUT.

(I'm not sure whether DISCARD, WRITE_ZEROES, or SECURE_ERASE also need
the payload field. It seems like GET_ID and GET_LIFETIME do not need the
payload field.)

> +\begin{itemize}
> +\item The \field{slot} filed in \field{payload} indicates the ICE

s/filed/field/

> +  (Inline Crypto Encryption) slot index where the key resides in.

s/where the key resides in/where the key resides/

> +
> +\item The \field{activate} filed in \field{payload} implies this is a

s/filed/field/

> +  encryption request.

Does "encryption" really mean just encryption or does it mean
encryption for writes and decryption for reads?

> +
> +\item The \field{data_unit_num} filed in \field{payload} indicates the

s/filed/field/

> +  starting block of the request.
> +\end{itemize}
> +
>  VIRTIO_BLK_T_IN requests populate \field{data} with the contents of sectors
>  read from the block device (in multiples of 512 bytes).  VIRTIO_BLK_T_OUT
>  requests write the contents of \field{data} to the block device (in multiples
> @@ -912,6 +979,8 @@ \subsection{Device Operation}\label{sec:Device Types / Block Device / Device Ope
>  successfully, failed, or were processed by the device at all if the request
>  failed with VIRTIO_BLK_S_IOERR.
>  
> +A driver MUST set \field{activate} to 0 for a non VIRTIO_BLK_F_ICE request.

Please explicitly list request types where the payload field is present
and where activate is optional.

> +
>  The following requirements only apply if the VIRTIO_BLK_F_ZONED feature is
>  negotiated.
>  
> -- 
> 2.34.1
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]