[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-dev] [PATCH v2] Add virtio rpmb device specification
On Tue, 30 Jul 2019 21:46:14 +0800
Huang Yang <yang.huang@intel.com> wrote:
> It is a virtio based RPMB (Replay Protected Memory Block) device.
>
> Signed-off-by: Yang Huang <yang.huang@intel.com>
> Reviewed-by: Bing Zhu <bing.zhu@intel.com>
> Reviewed-by: Tomas Winkler <tomas.winkler@intel.com>
>
> v1 -> v2:
> 1. update conformance.
> 2. wordings change:
> first initialization -> first device initialization
> device size -> device capacity
> 3. update Device Operation:
> add more decriptions on write counter, key and write operations.
> ---
> conformance.tex | 19 ++++++++++-
> content.tex | 3 ++
> virtio-rpmb.tex | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 121 insertions(+), 1 deletion(-)
> create mode 100644 virtio-rpmb.tex
>
(...)
> diff --git a/content.tex b/content.tex
> index ee0d7c9..7f54f94 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -2717,6 +2717,8 @@ \chapter{Device Types}\label{sec:Device Types}
> \hline
> 27 & PMEM device \\
> \hline
> +28 & RPMB device \\
Depending on how long it will take to get this into shape, it might be
a good idea to reserve the id separately.
> +\hline
> \end{tabular}
>
> Some of the devices above are unspecified by this document,
> @@ -5677,6 +5679,7 @@ \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
> \input{virtio-input.tex}
> \input{virtio-crypto.tex}
> \input{virtio-vsock.tex}
> +\input{virtio-rpmb.tex}
>
> \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>
> diff --git a/virtio-rpmb.tex b/virtio-rpmb.tex
> new file mode 100644
> index 0000000..aa113bb
> --- /dev/null
> +++ b/virtio-rpmb.tex
> @@ -0,0 +1,100 @@
> +\section{RPMB Device}\label{sec:Device Types / RPMB Device}
> +
> +virtio-rpmb is a virtio based RPMB (Replay Protected Memory Block)
> +device. It is used as a tamper-resistant and anti-replay storage.
> +It supports four command requests including read, write, get write
> +counter and program key. They are placed in the queue.
What about replacing the last two sentences with:
"The device is driven via requests including read, write, get write
counter, and program key, which are submitted via a request queue." ?
> +
> +\subsection{Device ID}\label{sec:Device Types / RPMB Device / Device ID}
> +
> +28
> +
> +\subsection{Virtqueues}\label{sec:Device Types / RPMB Device / Virtqueues}
> +
> +\begin{description}
> +\item[0] requestq
> +\end{description}
> +
> +\subsection{Feature bits}\label{sec:Device Types / RPMB Device / Feature bits}
> +
> +None.
> +
> +\subsection{Device configuration layout}\label{sec:Device Types / RPMB Device / Device configuration layout}
> +
> +None.
> +
> +\devicenormative{\subsection}{Device Initialization}{Device Types / RPMB Device / Device Initialization}
> +
> +\begin{enumerate}
> +\item The virtqueue is initialized.
> +\item The authentication key of device SHOULD NOT be programmed at the first device initialization.
> +\item The device capacity SHOULD be initialized to a multiple of 128 Kbytes and up to 16Mbytes.
> +\end{enumerate}
I would place a slightly more verbose description of how the device
should be initialized into a descriptive section and only leave the
normative statements here.
Also, why SHOULD/SHOULD NOT instead of MUST/MUST NOT? Especially the
device capacity requirements feel like something that should not be too
loosely defined.
Would it make sense to expose the capacity through the config space?
> +
> +\subsection{Device Operation}\label{sec:Device Types / RPMB Device / Device Operation}
> +
> +The operation of a virtio RPMB device is driven by the requests placed on the virtqueue.
> + The type of the request can be program key (VIRTIO_RPMB_REQ_PROGRAM_KEY),
> + get write counter (VIRTIO_RPMB_REQ_GET_WRITE_COUNTER),
> + write (VIRTIO_RPMB_REQ_DATA_WRITE), and read (VIRTIO_RPMB_REQ_DATA_READ).
> + A program key or write request can also combine with a
> + result read (VIRTIO_RPMB_REQ_RESULT_READ) for a returned result.
> +
> +\begin{lstlisting}
> +#define VIRTIO_RPMB_REQ_PROGRAM_KEY 0x0001
> +#define VIRTIO_RPMB_REQ_GET_WRITE_COUNTER 0x0002
> +#define VIRTIO_RPMB_REQ_DATA_WRITE 0x0003
> +#define VIRTIO_RPMB_REQ_DATA_READ 0x0004
> +#define VIRTIO_RPMB_REQ_RESULT_READ 0x0005
> +\end{lstlisting}
What form do the requests that are placed on the queue take?
> +
> +\drivernormative{\subsubsection}{Device Operation}{Device Types / RPMB Device / Device Operation}
> +
> +The driver MUST configure and initialize all virtqueues for the requests received.
But there is only a single virtqueue, isn't there? This requirement
looks a bit odd.
> +
> +\devicenormative{\subsubsection}{Device Operation}{Device Types / RPMB Device / Device Operation}
> +
> +The device provides a simulated RPMB backed by ordinary file or
> + other medium in host. It SHOULD keep consistent behaviors with
> + hardware, including but not limited to:
If this device is supposed to simulate the behaviour of a real device,
the virtio spec needs to point to a specification of that behaviour as
a normative reference. From the information contained here, I would
have no clue how to actually produce a conforming device or driver
implementation.
> +\begin{enumerate}
> +\item The device maintains an authentication key. Once the first
> + successful key programming is performed, the authentication
> + key MUST be kept unchanged during device lifecycle. It cannot
> + be overwritten, erased or read. This key MUST be kept regardless
> + of device reset or reboot.
> +\item The device maintains a monotonic write counter. It MUST be
> + initialized to zero and added by one automatically after each
> + successful write operation. The value cannot be reset. After
> + the counter has reached its maximum value 0xFFFFFFFF, it will
> + not be incremented anymore. This counter MUST be kept regardless
> + of device reset or reboot.
> +\item The RPMB device cannot be successfully accessed until RPMB
> + authentication key is programmed. For any operation (read, write,
> + program key, get write counter) done to virtio RPMB device after
> + authentication key is programmed successfully, the device responds
> + with a MAC calculated by HMAC-SHA with authentication key to driver.
> +\item For write operation, the device MUST compare the writer counter
> + it receives with the one it maintained internally. If the two are
> + not equal, a VIRTIO_RPMB_RES_COUNT_FAILURE SHOULD be returned as
> + the result. The device MUST calculate the MAC using HMAC-SHA. The
> + authentication key acts as an input of the calculation. If the MAC
> + are not equal to the one it received, a VIRTIO_RPMB_RES_AUTH_FAILURE
> + SHOULD be returned as the result.
> +\item
> +\end{enumerate}
I think the spec would again benefit from putting the verbose
description of what the driver needs to do to access the device and how
the device needs to react into a descriptive section, and only keep
some simple normative statements here.
> +
> +One of the below error codes MUST be returned to the driver
> + based on the operation result.
> +
> +\begin{lstlisting}
> +#define VIRTIO_RPMB_RES_OK 0x0000
> +#define VIRTIO_RPMB_RES_GENERAL_FAILURE 0x0001
> +#define VIRTIO_RPMB_RES_AUTH_FAILURE 0x0002
> +#define VIRTIO_RPMB_RES_COUNT_FAILURE 0x0003
> +#define VIRTIO_RPMB_RES_ADDR_FAILURE 0x0004
> +#define VIRTIO_RPMB_RES_WRITE_FAILURE 0x0005
> +#define VIRTIO_RPMB_RES_READ_FAILURE 0x0006
> +#define VIRTIO_RPMB_RES_NO_AUTH_KEY 0x0007
> +#define VIRTIO_RPMB_RES_WRITE_COUNTER_EXPIRED 0x0080
These return codes probably need some more description. Also, how are
they returned?
> +\end{lstlisting}
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]