Re: [virtio-dev][PATCH v1 1/2] virtio-mmio: Add MSI and different notification address support

Stefan Hajnoczi
2020-01-06T16:18:51Z

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

On Fri, Dec 20, 2019 at 11:25:03PM +0800, Jing Liu wrote:
> Upgrade virtio-mmio to version 3, with the abilities to support
> MSI interrupt and notification features.
>
> The details of version 2 will be appended as part of legacy interface
> in next patch.

Cool, MSI is useful. Not a full review, but some comments below...

>
> Signed-off-by: Jing Liu <jing2.liu@linux.intel.com>
> Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> Signed-off-by: Zha Bin <zhabin@linux.alibaba.com>
> Signed-off-by: Liu Jiang <gerry@linux.alibaba.com>
> ---
> content.tex | 191 ++++++++++++++++++++++++++++++++++++++++++++++++-----------
> msi-status.c | 5 ++
> 2 files changed, 163 insertions(+), 33 deletions(-)
> create mode 100644 msi-status.c
>
> diff --git a/content.tex b/content.tex
> index d68cfaf..eaaffec 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -1597,9 +1597,9 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
> }
> \hline
> \mmioreg{Version}{Device version number}{0x004}{R}{%
> - 0x2.
> + 0x3.
> \begin{note}
> - Legacy devices (see \ref{sec:Virtio Transport Options / Virtio Over MMIO / Legacy interface}~\nameref{sec:Virtio Transport Options / Virtio Over MMIO / Legacy interface}) used 0x1.
> + Legacy devices (see \ref{sec:Virtio Transport Options / Virtio Over MMIO / Legacy interface}~\nameref{sec:Virtio Transport Options / Virtio Over MMIO / Legacy interface}) used 0x1 or 0x2.

"Legacy devices" refers to pre-VIRTIO 1.0 devices. 0x2 is VIRTIO 1.0
and therefore not "Legacy". I suggest the following wording:

Legacy devices (see \ref{sec:Virtio Transport Options / Virtio Over MMIO / Legacy interface}~\nameref{sec:Virtio Transport Options / Virtio Over MMIO / Legacy interface}) used 0x1.
+ VIRTIO 1.0 and 1.1 used 0x2.

Did you consider using a transport feature bit instead of changing the
device version number? Feature bits allow more graceful compatibility:
old drivers will continue to work with new devices and new drivers will
continue to work with old devices.

> \end{note}
> }
> \hline
> @@ -1671,25 +1671,23 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
> accesses apply to the queue selected by writing to \field{QueueSel}.
> }
> \hline
> - \mmioreg{QueueNotify}{Queue notifier}{0x050}{W}{%
> - Writing a value to this register notifies the device that
> - there are new buffers to process in a queue.
> + \mmioreg{QueueNotify}{Queue notifier}{0x050}{RW}{%
> + Reading from the register returns the virtqueue notification configuration.
>
> - When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
> - the value written is the queue index.
> + See \ref{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation / Notification Address}
> + for the configuration format.
>
> - When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
> - the \field{Notification data} value has the following format:
> + Writing when the notification address calculated by the notification configuration
> + is just located at this register.

I don't understand this sentence. What happens when the driver writes
to this register?

>
> - \lstinputlisting{notifications-le.c}
> -
> - See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues / Driver notifications}
> - for the definition of the components.
> + See \ref{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation / Available Buffer Notifications}
> + to see the notification data format.
> }
> \hline
> \mmioreg{InterruptStatus}{Interrupt status}{0x60}{R}{%
> Reading from this register returns a bit mask of events that
> - caused the device interrupt to be asserted.
> + caused the device interrupt to be asserted. This is only used
> + when MSI is not enabled.
> The following events are possible:
> \begin{description}
> \item[Used Buffer Notification] - bit 0 - the interrupt was asserted
> @@ -1703,7 +1701,7 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
> \mmioreg{InterruptACK}{Interrupt acknowledge}{0x064}{W}{%
> Writing a value with bits set as defined in \field{InterruptStatus}
> to this register notifies the device that events causing
> - the interrupt have been handled.
> + the interrupt have been handled. This is only used when MSI is not enabled.
> }
> \hline
> \mmioreg{Status}{Device status}{0x070}{RW}{%
> @@ -1762,6 +1760,31 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
> \field{SHMSel} is unused) results in a base address of
> 0xffffffffffffffff.
> }
> + \hline
> + \mmioreg{MsiStatus}{MSI status}{0x0c0}{R}{%
> + Reading from this register returns the global MSI enable/disable status and maximum
> + number of virtqueues that device supports.
> + \lstinputlisting{msi-status.c}
> + }

Why is it necessary to combine the number of virtqueues and global
MSI enable/disable into a single 16-bit field?

virtio-mmio uses 32-bit registers. It doesn't try hard to save register
space so it's strange to do it here (11-bit number of virtqueue field
but 32-bit QueueSel field).

> + \hline
> + \mmioreg{MsiCmd}{MSI command}{0x0c2}{W}{%
> + The driver writes to this register with appropriate operators and arguments to
> + execute MSI command to device.
> + Operators supported is setting global MSI enable/disable status
> + and updating MSI configuration to device.

If the global MSI enable/disable state is written in this register,
consider making this register readable too so the global MSI
enable/disable state can be read from it.

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