virtio-mmio: Add MSI and different notification address support

PATCH In review Jing Liu 2 participants 8 messages Dec 20, 2019 – Feb 10, 2020

lore

Share & tools

lore

Jing Liu

author

2019-12-20 07:45 UTC #0 lore raw

PATCH 1/2 virtio-mmio: Add MSI and different notification address support

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.

Signed-off-by Jing Liu <[email protected]>

Signed-off-by Chao Peng <[email protected]>

Signed-off-by Zha Bin <[email protected]>

Signed-off-by Liu Jiang <[email protected]>

---
 content.tex  | 191 ++++++++++++++++++++++++++++++++++++++++++++++++-----------
 msi-status.c |   5 ++
 2 files changed, 163 insertions(+), 33 deletions(-)
 create mode 100644 msi-status.c

content.tex +158 ~~-33~~

index d68cfaf..eaaffec 100644
@@ -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.
     \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.
 
-    \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}
+  }
+  \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.
+    See \ref{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation / Device Initialization / MSI Vector Configuration}
+    for how to use this register.
+  }
+  \hline
+  \mmiodreg{MsiAddrLow}{MsiAddrHigh}{MSI 64 bit address}{0x0c4}{0x0c8}{W}{%
+    Writing to these two registers (lower 32 bits of the address to \field{MsiAddrLow},
+    higher 32 bits to \field{MsiAddrHigh}) notifies the device about the
+    MSI address.
+  }
+  \hline
+  \mmioreg{MsiData}{MSI 32 bit data}{0x0cc}{W}{%
+    Writing to this register notifies the device about the MSI data.
+  }
   \hline 
   \mmioreg{ConfigGeneration}{Configuration atomicity value}{0x0fc}{R}{
     Reading from this register returns a value describing a version of the device-specific configuration space (see \field{Config}).

@@ -1783,12 +1806,18 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
 
 The device MUST return 0x74726976 in \field{MagicValue}.
 
-The device MUST return value 0x2 in \field{Version}.
+The device MUST return value 0x3 in \field{Version}.
 
-The device MUST present each event by setting the corresponding bit in \field{InterruptStatus} from the
+When MSI is disabled, the device MUST present each event by setting the
+corresponding bit in \field{InterruptStatus} from the
 moment it takes place, until the driver acknowledges the interrupt
-by writing a corresponding bit mask to the \field{InterruptACK} register.  Bits which
-do not represent events which took place MUST be zero.
+by writing a corresponding bit mask to the \field{InterruptACK} register.
+Bits which do not represent events which took place MUST be zero.
+
+When MSI is enabled, the device MUST NOT set \field{InterruptStatus} and MUST
+ignore \field{InterruptACK}.
+
+Upon reset, the device MUST clear \field{msi_enabled} bit in \field{MsiStatus}.
 
 Upon reset, the device MUST clear all bits in \field{InterruptStatus} and ready bits in the
 \field{QueueReady} register for all queues in the device.

@@ -1814,7 +1843,7 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
 The driver MUST ignore a device with \field{MagicValue} which is not 0x74726976,
 although it MAY report an error.
 
-The driver MUST ignore a device with \field{Version} which is not 0x2,
+The driver MUST ignore a device with \field{Version} which is not 0x3,
 although it MAY report an error.
 
 The driver MUST ignore a device with \field{DeviceID} 0x0,

@@ -1837,7 +1866,12 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
 
 The driver MUST ignore undefined bits in \field{InterruptStatus}.
 
-The driver MUST write a value with a bit mask describing events it handled into \field{InterruptACK} when
+The driver MUST ignore undefined bits in \field{MsiStatus}.
+
+When MSI is enabled, the driver MUST NOT access \field{InterruptStatus} and MUST NOT write to \field{InterruptACK}.
+
+When MSI is disabled, the driver MUST write a value with a bit mask
+describing events it handled into \field{InterruptACK} when
 it finishes handling an interrupt and MUST NOT set any of the undefined bits in the value.
 
 \subsection{MMIO-specific Initialization And Device Operation}\label{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation}

@@ -1858,6 +1892,65 @@ \subsubsection{Device Initialization}\label{sec:Virtio Transport Options / Virti
 Further initialization MUST follow the procedure described in
 \ref{sec:General Initialization And Device Operation / Device Initialization}~\nameref{sec:General Initialization And Device Operation / Device Initialization}.
 
+\paragraph{MSI Vector Configuration}\label{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation / Device Initialization / MSI Vector Configuration}
+The MSI vectors and interrupt events have fixed relationships.
+The first vector is for device configuraiton
+change event, the second vector is for virtqueue 1, the third vector
+is for virtqueue 2 and so on.
+
+The 16-bit \field{MsiCmd} register including command operator and command argument is used
+to configure MSI vectors and set global MSI enable/disable status.
+\begin{lstlisting}
+le16 {
+    cmd_arg : 12;
+    cmd_op : 4;
+};
+\end{lstlisting}
+
+The \field{cmd_op} specifies the detailed command operator defined as follows.
+\begin{lstlisting}
+#define  VIRTIO_MMIO_MSI_CMD_UPDATE           0x1
+#define  VIRTIO_MMIO_MSI_CMD_ENABLE           0x2
+\end{lstlisting}
+
+\drivernormative{\subparagraph}{MSI Vector Configuration}{Virtio Transport Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation / MSI Vector Configuration}
+To configure MSI vector, driver SHOULD firstly notify the MSI address and data by
+writing to \field{MsiAddrLow}, \field{MsiAddrHigh},
+and \field{MsiData}, and immediately follow a \field{MsiCmd} write operation
+using VIRTIO_MMIO_MSI_CMD_UPDATE with corresponding
+virtual queue index as \field{cmd_arg} to device for configuring an event to this MSI vector.
+
+After all MSI vectors are configured, driver SHOULD set global MSI enabled
+by writing to \field{MsiCmd} using VIRTIO_MMIO_MSI_CMD_ENABLE
+with one (0x1) as \field{cmd_arg} to device.
+
+Driver should use VIRTIO_MMIO_MSI_CMD_ENABLE with status zero (0x0) when disabling MSI.
+
+If driver fails to setup any event with a vector,
+it MUST disable MSI by \field{MsiCmd} and use single interrupt for device.
+
+\subsubsection{Notification Address}\label{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation / Notification Address}
+
+The notification location is calculated by notification structure. Driver reads \field{QueueNotify} to get this structure formatted as follows.
+
+\begin{lstlisting}
+le32 {
+        notify_base : 16;
+        notify_multiplier : 16;
+};
+\end{lstlisting}
+
+\field{notify_multiplier} is combined with virtqueue index to derive the Queue Notify address
+within a memory mapped control registers for a virtqueue:
+
+\begin{lstlisting}
+        notify_base + queue_index * notify_multiplier
+\end{lstlisting}
+
+\begin{note}
+The nofication address may be just located at the \field{QueueNotify}.
+\end{note}
+
 \subsubsection{Virtqueue Configuration}\label{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation / Virtqueue Configuration}
 
 The driver will typically initialize the virtual queue in the following way:

@@ -1885,6 +1978,12 @@ \subsubsection{Virtqueue Configuration}\label{sec:Virtio Transport Options / Vir
    \field{QueueDriverLow}/\field{QueueDriverHigh} and
    \field{QueueDeviceLow}/\field{QueueDeviceHigh} register pairs.
 
+\item Write MSI address \field{MsiAddrLow}/\field{MsiAddrHigh},
+MSI data \field{MsiData} and MSI update command \field{MsiCmd} with corresponding
+virtqueue index to update
+MSI configuration for device requesting interrupts triggered by
+virtqueue events.
+
 \item Write 0x1 to \field{QueueReady}.
 \end{enumerate}

@@ -1893,32 +1992,58 @@ \subsubsection{Available Buffer Notifications}\label{sec:Virtio Transport Option
 When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
 the driver sends an available buffer notification to the device by writing
 the 16-bit virtqueue index
-of the queue to be notified to \field{QueueNotify}.
+of the queue to be notified to Queue Notify address.
 
 When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
 the driver sends an available buffer notification to the device by writing
-the following 32-bit value to \field{QueueNotify}:
+the following 32-bit value to Queue Notify address:
 \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 / Notification Address}
+for how to calculate the Queue Notify address.
+
 \subsubsection{Notifications From The Device}\label{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation / Notifications From The Device}
 
-The memory mapped virtio device is using a single, dedicated
+If MSI is enabled, the memory mapped virtio
+device uses appropriate MSI interrupt message
+for configuration change notification and used buffer notification which are
+configured by \field{MsiAddrLow}, \field{MsoAddrHigh} and \field{MsiData}.
+
+If MSI is not enabled, the memory mapped virtio device
+uses a single, dedicated
 interrupt signal, which is asserted when at least one of the
 bits described in the description of \field{InterruptStatus}
-is set. This is how the device sends a used buffer notification
-or a configuration change notification to the device.
+is set.
 
 \drivernormative{\paragraph}{Notifications From The Device}{Virtio Transport Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation / Notifications From The Device}
-After receiving an interrupt, the driver MUST read
-\field{InterruptStatus} to check what caused the interrupt (see the
-register description).  The used buffer notification bit being set
-SHOULD be interpreted as a used buffer notification for each active
-virtqueue.  After the interrupt is handled, the driver MUST acknowledge
-it by writing a bit mask corresponding to the handled events to the
-InterruptACK register.
+A driver MUST handle the case where MSI is disabled, which uses the same interrupt indicating both device configuration
+space change and one or more virtqueues being used.
+
+\subsubsection{Driver Handling Interrupts}\label{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation / Driver Handling Interrupts}
+
+The driver interrupt handler would typically:
+
+\begin{itemize}
+  \item If MSI is enabled:
+    \begin{itemize}
+      \item
+        Figure out the virtqueue mapped to that MSI vector for the
+        device, to see if any progress has been made by the device
+        which requires servicing.
+      \item
+        If the interrupt belongs to configuration space changing signal,
+        re-examine the configuration space to see what changed.
+    \end{itemize}
+  \item If MSI is disabled:
+    \begin{itemize}
+      \item Read \field{InterruptStatus} to check what caused the interrupt.
+      \item Acknowledge the interrupt by writing a bit mask corresponding
+            to the handled events to the InterruptACK register.
+    \end{itemize}
+\end{itemize}
 
 \subsection{Legacy interface}\label{sec:Virtio Transport Options / Virtio Over MMIO / Legacy interface}

msi-status.c new file +5 -0

new file mode 100644
index 0000000..f3bd0ec
@@ -0,0 +1,5 @@
+le16 {
+    max_queue_num : 11;
+    reserved : 4;
+    msi_enabled: 1;
+};

Signature · 1 line

2.7.4

Jing Liu

author

2019-12-20 07:45 UTC #1 lore raw

PATCH 2/2 virtio-mmio: Append version 2 interface

Version 2 needs to be appended together with version 1 as legacy interface since we introduce the latest version.

Signed-off-by Jing Liu <[email protected]>

Signed-off-by Chao Peng <[email protected]>

Signed-off-by Zha Bin <[email protected]>

Signed-off-by Liu Jiang <[email protected]>

---
 content.tex | 123 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 117 insertions(+), 6 deletions(-)

content.tex +117 -6

index eaaffec..faf74de 100644
@@ -2047,18 +2047,129 @@ \subsubsection{Driver Handling Interrupts}\label{sec:Virtio Transport Options /
 
 \subsection{Legacy interface}\label{sec:Virtio Transport Options / Virtio Over MMIO / Legacy interface}
 
-The legacy MMIO transport used page-based addressing, resulting
+\subsubsection{Version 2}\label{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation / Version 2}
+
+The version 2 MMIO transport used single interrupt number and signel notification address,
+resulting in a slightly different control register layout, the device initialization and
+interrupt event configuration procedure from later version.
+
+Table \ref{tab:Virtio Trasport Options / Virtio Over MMIO / Version 2 MMIO Device Register Layout}
+presents control registers layout, omitting
+descriptions of registers which did not change their function
+nor behaviour:
+
+\begin{longtable}{p{0.2\textwidth}p{0.7\textwidth}}
+  \caption {Version 2 MMIO Device Register Layout}
+  \label{tab:Virtio Trasport Options / Virtio Over MMIO / Version 2 MMIO Device Register Layout} \\
+  \hline
+  \mmioreg{Name}{Function}{Offset from base}{Direction}{Description}
+  \hline
+  \hline
+  \endfirsthead
+  \hline
+  \mmioreg{Name}{Function}{Offset from the base}{Direction}{Description}
+  \hline
+  \hline
+  \endhead
+  \endfoot
+  \endlastfoot
+  \mmioreg{MagicValue}{Magic value}{0x000}{R}{}
+  \hline
+  \mmioreg{Version}{Device version number}{0x004}{R}{Device returns value 0x2.}
+  \hline
+  \mmioreg{DeviceID}{Virtio Subsystem Device ID}{0x008}{R}{}
+  \hline
+  \mmioreg{VendorID}{Virtio Subsystem Vendor ID}{0x00c}{R}{}
+  \hline
+  \mmioreg{DeviceFeatures}{Flags representing features the device supports}{0x010}{R}{}
+  \hline
+  \mmioreg{DeviceFeaturesSel}{Device (host) features word selection.}{0x014}{W}{}
+  \hline
+  \mmioreg{DriverFeatures}{Flags representing device features understood and activated by the driver}{0x020}{W}{}
+  \hline
+  \mmioreg{DriverFeaturesSel}{Activated (guest) features word selection}{0x024}{W}{}
+  \hline
+  \mmioreg{QueueSel}{Virtual queue index}{0x030}{W}{}
+  \hline
+  \mmioreg{QueueNumMax}{Maximum virtual queue size}{0x034}{R}{}
+  \hline
+  \mmioreg{QueueNum}{Virtual queue size}{0x038}{W}{}
+  \hline
+  \mmioreg{QueueReady}{Virtual queue ready bit}{0x044}{RW}{}
+  \hline
+  \mmioreg{QueueNotify}{Queue notifier}{0x050}{RW}{%
+    Writing a value to this register notifies the device that
+    there are new buffers to process in a queue.
+
+    When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
+    the value written is the queue index.
+
+    When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
+    the \field{Notification data} value has the following format:
+
+    \lstinputlisting{notifications-le.c}
+
+    See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues / Driver notifications}
+    for the definition of the components.
+  }
+  \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.
+    The following events are possible:
+    \begin{description}
+      \item[Used Buffer Notification] - bit 0 - the interrupt was asserted
+        because the device has used a buffer
+        in at least one of the active virtual queues.
+      \item [Configuration Change Notification] - bit 1 - the interrupt was
+        asserted because the configuration of the device has changed.
+    \end{description}
+  }
+  \hline
+  \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.
+  }
+  \hline
+  \mmioreg{Status}{Device status}{0x070}{RW}{}
+  \hline
+  \mmiodreg{QueueDescLow}{QueueDescHigh}{Virtual queue's Descriptor Area 64 bit long physical address}{0x080}{0x084}{W}{}
+  \hline
+  \mmiodreg{QueueDriverLow}{QueueDriverHigh}{Virtual queue's Driver Area 64 bit long physical address}{0x090}{0x094}{W}{}
+  \hline
+  \mmiodreg{QueueDeviceLow}{QueueDeviceHigh}{Virtual queue's Device Area 64 bit long physical address}{0x0a0}{0x0a4}{W}{}
+  \hline
+  \mmioreg{SHMSel}{Shared memory id}{0x0ac}{W}{}
+  \hline
+  \mmiodreg{SHMLenLow}{SHMLenHigh}{Shared memory region 64 bit long length}{0x0b0}{0x0b4}{R}{}
+  \hline
+  \mmiodreg{SHMBaseLow}{SHMBaseHigh}{Shared memory region 64 bit long physical address}{0x0b8}{0x0bc}{R}{}
+  \hline
+  \mmioreg{ConfigGeneration}{Configuration atomicity value}{0x0fc}{R}{}
+  \hline
+  \mmioreg{Config}{Configuration space}{0x100+}{RW}{}
+  \hline
+\end{longtable}
+
+The device sends a used buffer notification or a configuration change notification
+by a single, deditcated interrupt, which is differed by the bit described in the description of InterruptStatus. That is, after receiving an interrupt, driver uses InterruptStatus to differ the event source and uses InterruptACK register to acknowledge after handling.
+
+The notification mechanism is described in the QueueNotify register.
+
+\subsubsection{Version 1}\label{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation / Version 1}
+The MMIO transport of version 1 used page-based addressing, resulting
 in a slightly different control register layout, the device
-initialization and the virtual queue configuration procedure.
+initialization and the virtual queue configuration procedure from later versions.
 
-Table \ref{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Legacy Register Layout} 
+Table \ref{tab:Virtio Trasport Options / Virtio Over MMIO / Version 1 MMIO Device Register Layout}
 presents control registers layout, omitting
 descriptions of registers which did not change their function
 nor behaviour:
 
 \begin{longtable}{p{0.2\textwidth}p{0.7\textwidth}}
-  \caption {MMIO Device Legacy Register Layout}
-  \label{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Legacy Register Layout} \\
+  \caption {Version 1 MMIO Device Register Layout}
+  \label{tab:Virtio Trasport Options / Virtio Over MMIO / Version 1 MMIO Device Register Layout} \\
   \hline
   \mmioreg{Name}{Function}{Offset from base}{Direction}{Description} 
   \hline

@@ -2193,7 +2304,7 @@ \subsection{Legacy interface}\label{sec:Virtio Transport Options / Virtio Over M
    the \field{QueuePFN} register.
 \end{enumerate}
 
-Notification mechanisms did not change.
+Notification mechanisms are the same as version 2.
 
 \section{Virtio Over Channel I/O}\label{sec:Virtio Transport Options / Virtio Over Channel I/O}

Signature · 1 line

2.7.4

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.

Quoted 12 lines

content.tex +2 -3

     \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.

content.tex \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi · line 1681

Quoted 10 lines

content.tex +4 -4

 
-    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?

Quoted 27 lines

content.tex +5 -6

-    \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}{%

content.tex \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi · line 1768

Quoted 9 lines

content.tex +6 -0

@@ -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).

content.tex \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi · line 1774

Quoted 6 lines

content.tex +6 -0

+  \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.

Liu, Jing2

author

2020-01-09 05:13 UTC #3 lore raw

Quoted 7 lines · Stefan Hajnoczi

Quoted 5 lines · Jing Liu
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...

Hi Stefan,

Thanks for reviewing patches! Glad to see that MSI is welcome.

Quoted 26 lines

Quoted 15 lines
Signed-off-by Jing Liu<[email protected]>
Signed-off-by Chao Peng<[email protected]>
Signed-off-by Zha Bin<[email protected]>
Signed-off-by Liu Jiang<[email protected]>
---
  content.tex  | 191 ++++++++++++++++++++++++++++++++++++++++++++++++-----------
  msi-status.c |   5 ++
  2 files changed, 163 insertions(+), 33 deletions(-)
create mode 100644 msi-status.c
content.tex +0 -0
index d68cfaf..eaaffec 100644
content.tex \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi · line 1602
Quoted 8 lines
content.tex +2 -2
@@ -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:

Quoted 2 lines

       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.

Thanks for the guide.

Quoted 4 lines

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.

Yes, we considered using a feature bit from 24~37 or above, while a concern is that,

the device which uses such bit only represents the behavior of mmio transport layer but not common behavior

of virtio device. Or am I missing some "transport" feature bit range?

Quoted 18 lines

content.tex +4 -5

      \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.

Quoted 6 lines

content.tex \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi · line 1681
Quoted 4 lines
content.tex +2 -2
-    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?

We're trying to define the notification mechanism that, driver MUST read 0x50 to get the notification configuration

and calculate the notify address. The writing case here is that, the notify address is just located here e.g. notify_base=0x50, notify_mul=0.

Quoted 30 lines

content.tex +5 -6

-    \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.
    }

Quoted 8 lines

content.tex \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi · line 1768
Quoted 6 lines
content.tex +6 -0
+  \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?

Originally, we want this 16-bit Read-Only, so we put some RO things together and separate

enable setting command to next register.

Quoted 3 lines

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).

In order to improve performance/save register space, we combine some data together.

For example, combine MSI cmd operator (e.g. enable/disable, vector setup) with argument (e.g. 1/0, queue index).

But it seems we miss the consistency with QueueSel. So do you think if the max queue number should be 32-bit,

which means it must be the same with QueueSel? If so, I guess we need some re-organization. :)

Quoted 10 lines

content.tex \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi · line 1774
Quoted 7 lines
content.tex +6 -0
+  \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.

Read enable/disable state is in 0x0c0.

Thanks,

Jing

Stefan Hajnoczi

2020-01-10 09:52 UTC #4 lore raw

Quoted 48 lines · On Thu, Jan 09, 2020 at 01:13:09PM +0800, Liu, Jing2 wrote:

Quoted 7 lines · Stefan Hajnoczi
Quoted 5 lines · Jing Liu
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...
Hi Stefan,
Thanks for reviewing patches! Glad to see that MSI is welcome.
Quoted 26 lines
Quoted 15 lines
Signed-off-by Jing Liu<[email protected]>
Signed-off-by Chao Peng<[email protected]>
Signed-off-by Zha Bin<[email protected]>
Signed-off-by Liu Jiang<[email protected]>
---
  content.tex  | 191 ++++++++++++++++++++++++++++++++++++++++++++++++-----------
  msi-status.c |   5 ++
  2 files changed, 163 insertions(+), 33 deletions(-)
create mode 100644 msi-status.c
content.tex +0 -0
index d68cfaf..eaaffec 100644
content.tex \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi · line 1602
Quoted 8 lines
content.tex +2 -2
@@ -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:
Quoted 2 lines
       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.
Thanks for the guide.
Quoted 4 lines
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.

Quoted 7 lines

Yes, we considered using a feature bit from 24~37 or above, while a concern is that,
the device which uses such bit only represents the behavior of mmio transport layer but not common behavior
of virtio device. Or am I missing some "transport" feature bit range?

https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-4100006

Feature bit 39 is reserved and can be used.

This is similar to when the SR_IOV (37) feature bit was added for virtio-pci.

Quoted 23 lines

Quoted 14 lines

content.tex +2 -5

      \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.

Quoted 8 lines

content.tex \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi · line 1681
Quoted 6 lines
content.tex +4 -2
+    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?

Quoted 5 lines

We're trying to define the notification mechanism that, driver MUST read 0x50 to get the notification configuration
and calculate the notify address. The writing case here is that, the notify address is just located here e.g. notify_base=0x50, notify_mul=0.

I still don't understand what this means. It's just an English issue and it will become clear if you can rephrase what you're saying.

Quoted 50 lines

Quoted 29 lines

content.tex +5 -6

-    \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.
    }

Quoted 8 lines

content.tex \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi · line 1768
Quoted 6 lines
content.tex +6 -0
+  \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?

Originally, we want this 16-bit Read-Only, so we put some RO things together and separate

enable setting command to next register.

Quoted 3 lines

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).

In order to improve performance/save register space, we combine some data together.

Quoted 8 lines

For example, combine MSI cmd operator (e.g. enable/disable, vector setup) with argument (e.g. 1/0, queue index).
But it seems we miss the consistency with QueueSel. So do you think if the max queue number should be 32-bit,
which means it must be the same with QueueSel? If so, I guess we need some re-organization. :)

I suggest following the 32-bit register size convention unless there is a specific reason why using other register sizes is absolutely necessary.

Quoted 7 lines

content.tex +6 -0

+  \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.

Quoted 5 lines

Quoted 3 lines
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.
Read enable/disable state is in 0x0c0.

The read-only 0xc0 register combines two pieces of information:
1. Global MSI enable/disable state
2. Number of virtqueues (or is "number of MSI vectors" a more accurate term?)

A simpler way of organizing the registers is:
MsiMaxVectors R
MsiState RW

This is simpler because drivers can read the MsiMaxVectors field and treat the value as an integer (no masking required). And a person reading the specification instantly knows how to fetch the MSI enable/disable state when they read the MsiState register description. They don't need to look through all the other registers to find the one that allows them to read the state.

Stefan

Liu, Jing2

author

2020-01-13 11:54 UTC #5 lore raw

Quoted 25 lines

Quoted 25 lines
Quoted 12 lines
content.tex \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi · line 1602
Quoted 9 lines
content.tex +2 -2
@@ -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:
Quoted 2 lines
        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.
Thanks for the guide.
Quoted 4 lines
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.
Yes, we considered using a feature bit from 24~37 or above, while a concern is that,
the device which uses such bit only represents the behavior of mmio transport layer but not common behavior

Quoted 7 lines

Quoted 1 line
of virtio device. Or am I missing some "transport" feature bit range?
https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-4100006
Feature bit 39 is reserved and can be used.
This is similar to when the SR_IOV (37) feature bit was added for virtio-pci.

That's great. Let's try to use bit 39 for MMIO MSI and bit 40 for MMIO notifications capabilities.

Quoted 22 lines

Quoted 10 lines

content.tex +0 -3

       \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.

content.tex \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi · line 1679

Quoted 10 lines

content.tex +6 -4

+  \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?

Quoted 7 lines

Quoted 5 lines
We're trying to define the notification mechanism that, driver MUST read 0x50 to get the notification configuration
and calculate the notify address. The writing case here is that, the notify address is just located here e.g. notify_base=0x50, notify_mul=0.
I still don't understand what this means. It's just an English issue and it will become clear if you can rephrase what you're saying.

Sure, let me try to explain it. :)

The different notification locations are calculated via the structure returned by reading this register.

le32 {
notify_base : 16;
notify_multiplier : 16;
};

location=notify_base + queue_index * notify_multiplier

The location might be the same when mul=0, and furthermore, it might be equal to 0x50 (notify_base=0x50, notify_mul=0) so we make this register W too.

So we said, the register is RW and W is only for such scenario.

Feel free to tell me if it's still confusing.

Quoted 51 lines

Quoted 29 lines

content.tex +5 -6

-    \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.
     }

Quoted 8 lines

content.tex \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi · line 1768
Quoted 6 lines
content.tex +6 -0
+  \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?

Originally, we want this 16-bit Read-Only, so we put some RO things together and separate

enable setting command to next register.

Quoted 3 lines

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).

In order to improve performance/save register space, we combine some data together.

For example, combine MSI cmd operator (e.g. enable/disable, vector setup) with argument (e.g. 1/0, queue index).

Quoted 7 lines

Quoted 5 lines
But it seems we miss the consistency with QueueSel. So do you think if the max queue number should be 32-bit,
which means it must be the same with QueueSel? If so, I guess we need some re-organization. :)
I suggest following the 32-bit register size convention unless there is a specific reason why using other register sizes is absolutely necessary.

Yes, let's keep consistency with QueueSel and re-organize other registers.

I feel concern why Available Buﬀer Notifcations (section describing VIRTIO_F_NOTIFICATION_DATA) makes vq index as 16bit?

Quoted 19 lines

Quoted 10 lines
Quoted 9 lines
content.tex \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi · line 1774
Quoted 6 lines
content.tex +6 -0
+  \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.
Read enable/disable state is in 0x0c0.
The read-only 0xc0 register combines two pieces of information:
1. Global MSI enable/disable state
2. Number of virtqueues (or is "number of MSI vectors" a more accurate
    term?)
A simpler way of organizing the registers is:
MsiMaxVectors R
MsiState RW

Quoted 6 lines

This is simpler because drivers can read the MsiMaxVectors field and treat the value as an integer (no masking required). And a person reading the specification instantly knows how to fetch the MSI enable/disable state when they read the MsiState register description. They don't need to look through all the other registers to find the one that allows them to read the state.

That's a good idea to separate the MsiMaxVectors(32bit) and this also meets the request from Jason/MST's comments.

Thanks!

Jing

Quoted 2 lines

Stefan

Stefan Hajnoczi

2020-01-22 16:56 UTC #6 lore raw

Quoted 42 lines · On Mon, Jan 13, 2020 at 07:54:06PM +0800, Liu, Jing2 wrote:

Quoted 22 lines
Quoted 22 lines
Quoted 22 lines
Quoted 10 lines
       \end{note}
     }
     \hline
content.tex +0 -3
@@ -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.
content.tex \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi · line 1679
Quoted 10 lines
content.tex +6 -4
+  \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?
Quoted 7 lines
Quoted 5 lines
We're trying to define the notification mechanism that, driver MUST read 0x50 to get the notification configuration
and calculate the notify address. The writing case here is that, the notify address is just located here e.g. notify_base=0x50, notify_mul=0.
I still don't understand what this means. It's just an English issue and it will become clear if you can rephrase what you're saying.
Sure, let me try to explain it. :)
The different notification locations are calculated via the structure returned by reading this register.
le32 {
notify_base : 16;
notify_multiplier : 16;
};
location=notify_base + queue_index * notify_multiplier

Quoted 7 lines

The location might be the same when mul=0, and furthermore, it might be equal to 0x50 (notify_base=0x50, notify_mul=0) so we make this register W too.
So we said, the register is RW and W is only for such scenario.
Feel free to tell me if it's still confusing.

I understand now:

Devices that only require a single notify address may set notify_base=0x50 and notify_multiplier=0 to use the Queue Notifier register itself for notifications. In this case the driver writes to Queue Notifier to notify the device that there are new buffers in a virtqueue.

Perhaps you could include this in the text.

Quoted 56 lines

Quoted 56 lines
Quoted 56 lines
Quoted 29 lines
Quoted 29 lines
content.tex +5 -6
-    \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.
     }
Quoted 8 lines
content.tex \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi · line 1768
Quoted 6 lines
content.tex +6 -0
+  \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?
Originally, we want this 16-bit Read-Only, so we put some RO things together and separate
enable setting command to next register.
Quoted 3 lines
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).
In order to improve performance/save register space, we combine some data together.
For example, combine MSI cmd operator (e.g. enable/disable, vector setup) with argument (e.g. 1/0, queue index).
But it seems we miss the consistency with QueueSel. So do you think if the max queue number should be 32-bit,
which means it must be the same with QueueSel? If so, I guess we need some re-organization. :)

Quoted 7 lines

Quoted 2 lines
I suggest following the 32-bit register size convention unless there is a specific reason why using other register sizes is absolutely necessary.
Yes, let's keep consistency with QueueSel and re-organize other registers.
I feel concern why Available Buﬀer Notifcations (section describing VIRTIO_F_NOTIFICATION_DATA) makes vq index as 16bit?

As you mentioned, the valid range of virtqueue numbers is only 16 bits due to non-MMIO parts of the specification using 16 bits.

However, I think it makes sense to stick to the MMIO transport 32-bit register size convention for consistency. Devices just won't support values above 0xffff.

Stefan

Liu, Jing2

author

2020-02-10 09:21 UTC #7 lore raw

Quoted 51 lines · Stefan Hajnoczi

Quoted 41 lines · On Mon, Jan 13, 2020 at 07:54:06PM +0800, Liu, Jing2 wrote:
Quoted 22 lines
Quoted 22 lines
Quoted 22 lines
Quoted 10 lines
        \end{note}
      }
      \hline
content.tex +0 -3
@@ -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.
content.tex \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi · line 1679
Quoted 10 lines
content.tex +6 -4
+  \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?
Quoted 7 lines
Quoted 5 lines
We're trying to define the notification mechanism that, driver MUST read 0x50 to get the notification configuration
and calculate the notify address. The writing case here is that, the notify address is just located here e.g. notify_base=0x50, notify_mul=0.
I still don't understand what this means. It's just an English issue and it will become clear if you can rephrase what you're saying.
Sure, let me try to explain it. :)
The different notification locations are calculated via the structure returned by reading this register.
le32 {
notify_base : 16;
notify_multiplier : 16;
};
location=notify_base + queue_index * notify_multiplier
Quoted 7 lines
The location might be the same when mul=0, and furthermore, it might be equal to 0x50 (notify_base=0x50, notify_mul=0) so we make this register W too.
So we said, the register is RW and W is only for such scenario.
Feel free to tell me if it's still confusing.
I understand now:

Quoted 7 lines

Devices that only require a single notify address may set notify_base=0x50 and notify_multiplier=0 to use the Queue Notifier register itself for notifications. In this case the driver writes to Queue Notifier to notify the device that there are new buffers in a virtqueue.
Perhaps you could include this in the text.

Thanks for the guide. Since v2 was sent out, we'll add such text in later version.

Jing

Quoted 70 lines

Quoted 54 lines
Quoted 54 lines
Quoted 54 lines
Quoted 29 lines
Quoted 29 lines
content.tex +5 -6
-    \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.
      }
Quoted 8 lines
content.tex \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi · line 1768
Quoted 6 lines
content.tex +6 -0
+  \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?
Originally, we want this 16-bit Read-Only, so we put some RO things together and separate
enable setting command to next register.
Quoted 3 lines
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).
In order to improve performance/save register space, we combine some data together.
For example, combine MSI cmd operator (e.g. enable/disable, vector setup) with argument (e.g. 1/0, queue index).
But it seems we miss the consistency with QueueSel. So do you think if the max queue number should be 32-bit,
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
which means it must be the same with QueueSel? If so, I guess we need some re-organization. :)
I suggest following the 32-bit register size convention unless there is a specific reason why using other register sizes is absolutely necessary.
Yes, let's keep consistency with QueueSel and re-organize other registers.
I feel concern why Available Buﬀer Notifcations (section describing VIRTIO_F_NOTIFICATION_DATA) makes vq index as 16bit?
As you mentioned, the valid range of virtqueue numbers is only 16 bits due to non-MMIO parts of the specification using 16 bits.
However, I think it makes sense to stick to the MMIO transport 32-bit register size convention for consistency. Devices just won't support values above 0xffff.
Stefan