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

[Stefan Hajnoczi <stefanha@redhat.com>]

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: signature.asc
Description: PGP signature