[PATCH v2] transport-pci: Add MSI support

[PATCH v2] transport-pci: Add MSI support

[Manivannan Sadhasivam]

MSI is the predecessor of MSI-X that allows PCIe devices to send interrupts
to the host. Compared to MSI-X, MSI supports only a maximum of 32 vectors
per PCIe function. But MSI has been widely supported by the PCIe devices
requiring fewer interrupts such as Modems, WLAN cards etc...

Currently, Virtio spec only documents MSI-X and INTX interrupt mechanisms
for the PCI transport. So if a Virtio device based on PCI transport
supports only MSI, then the driver on the guest will only use INTX for
receiving the interrupts. This is really sub-optimal and affects the
performance of the device. Because with MSI, the device can use one vector
per queue (max of 32 vectors) thus avoiding the overhead associated with a
shared INTX vector.

Hence, add support for MSI to the Virtio PCI transport. MSI support is
added such a way that it reuses the existing infrastructure of MSI-X, like
the config_msix_vector/queue_msix_vector fields of the Virtio common config
structure. This makes it easy for the Virtio drivers to add MSI support
without any disruptive changes.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---

Changes in v2:

* Fixed a spelling mistake in commit message
* Removed update to legacy interface
* Used 'MSI vector' consistently

 transport-pci.tex | 115 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 92 insertions(+), 23 deletions(-)

diff --git a/transport-pci.tex b/transport-pci.tex
index a5c6719..fd92641 100644
--- a/transport-pci.tex
+++ b/transport-pci.tex
@@ -347,7 +347,7 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
         Driver Feature Bits selected by \field{driver_feature_select}.
 
 \item[\field{config_msix_vector}]
-        Set by the driver to the MSI-X vector for configuration change notifications.
+        Set by the driver to the MSI/MSI-X vector for configuration change notifications.
 
 \item[\field{num_queues}]
         The device specifies the maximum number of virtqueues supported here.
@@ -371,7 +371,7 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
         A 0 means the queue is unavailable.
 
 \item[\field{queue_msix_vector}]
-        Set by the driver to the MSI-X vector for virtqueue notifications.
+        Set by the driver to the MSI/MSI-X vector for virtqueue notifications.
 
 \item[\field{queue_enable}]
         The driver uses this to selectively prevent the device from executing requests from this virtqueue.
@@ -631,11 +631,11 @@ \subsubsection{ISR status capability}\label{sec:Virtio Transport Options / Virti
 in \field{ISR status} before sending a device configuration
 change notification to the driver.
 
-If MSI-X capability is disabled, the device MUST set the Queue
+If MSI/MSI-X capability is disabled, the device MUST set the Queue
 Interrupt bit in \field{ISR status} before sending a virtqueue
 notification to the driver.
 
-If MSI-X capability is disabled, the device MUST set the Interrupt Status
+If MSI/MSI-X capability is disabled, the device MUST set the Interrupt Status
 bit in the PCI Status register in the PCI Configuration Header of
 the device to the logical OR of all bits in \field{ISR status} of
 the device.  The device then asserts/deasserts INT\#x interrupts unless masked
@@ -645,7 +645,7 @@ \subsubsection{ISR status capability}\label{sec:Virtio Transport Options / Virti
 
 \drivernormative{\paragraph}{ISR status capability}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / ISR status capability}
 
-If MSI-X capability is enabled, the driver SHOULD NOT access
+If MSI/MSI-X capability is enabled, the driver SHOULD NOT access
 \field{ISR status} upon detecting a Queue Interrupt.
 
 \subsubsection{Device-specific configuration}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Device-specific configuration}
@@ -1017,7 +1017,7 @@ \subsubsection{Device Initialization}\label{sec:Virtio Transport Options / Virti
 \drivernormative{\subparagraph}{MSI-X Vector Configuration}{Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
 
 Driver MUST support device with any MSI-X Table Size 0 to 0x7FF.
-Driver MAY fall back on using INT\#x interrupts for a device
+Driver MAY fall back on using MSI or INT\#x interrupts for a device
 which only supports one MSI-X vector (MSI-X Table Size = 0).
 
 Driver MAY interpret the Table Size as a hint from the device
@@ -1034,6 +1034,75 @@ \subsubsection{Device Initialization}\label{sec:Virtio Transport Options / Virti
 the driver MAY retry mapping with fewer vectors, disable MSI-X
 or report device failure.
 
+\paragraph{MSI Vector Configuration}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI Vector Configuration}
+
+When MSI capability is present and enabled in the device
+(through standard PCI configuration space) \field{config_msix_vector} and \field{queue_msix_vector} are used to map configuration change and queue
+interrupts to MSI vectors. In this case, the ISR Status is unused.
+
+Writing a valid MSI vector, 0 to 0x1F, to
+\field{config_msix_vector}/\field{queue_msix_vector} maps interrupts triggered
+by the configuration change/selected queue events respectively to
+the corresponding MSI vector. To disable interrupts for an
+event type, the driver unmaps this event by writing a special NO_VECTOR
+value:
+
+\begin{lstlisting}
+/* Vector value used to disable MSI for queue */
+#define VIRTIO_MSI_NO_VECTOR            0xffff
+\end{lstlisting}
+
+Note that mapping an event to vector might require device to
+allocate internal device resources, and thus could fail.
+
+\devicenormative{\subparagraph}{MSI Vector Configuration}{Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI Vector Configuration}
+
+A device that has an MSI capability SHOULD support at least 2
+and at most 0x20 MSI vectors.
+Device MUST report the number of vectors supported in
+\field{Multiple Message Capable} field in the MSI Capability as specified in
+\hyperref[intro:PCI]{[PCI]}.
+The device SHOULD restrict the reported MSI Multiple Message Capable field
+to a value that might benefit system performance.
+\begin{note}
+For example, a device which does not expect to send
+interrupts at a high rate might only specify 2 MSI vectors.
+\end{note}
+Device MUST support mapping any event type to any valid
+vector 0 to number of MSI vectors specified in \field{Multiple Message Capable} field.
+Device MUST support unmapping any event type.
+
+The device MUST return vector mapped to a given event,
+(NO_VECTOR if unmapped) on read of \field{config_msix_vector}/\field{queue_msix_vector}.
+The device MUST have all queue and configuration change
+events unmapped upon reset.
+
+Devices SHOULD NOT cause mapping an event to vector to fail
+unless it is impossible for the device to satisfy the mapping
+request.  Devices MUST report mapping
+failures by returning the NO_VECTOR value when the relevant
+\field{config_msix_vector}/\field{queue_msix_vector} field is read.
+
+\drivernormative{\subparagraph}{MSI Vector Configuration}{Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI Vector Configuration}
+
+Driver MUST support device with any MSI vector from 0 to 0x1F.
+Driver MAY fall back on using INT\#x interrupts for a device
+which only supports one MSI vector (MSI Multiple Message Capable = 0).
+
+Driver MAY interpret the Multiple Message Capable field as a hint from the device
+for the suggested number of MSI vectors to use.
+
+Driver MUST NOT attempt to map an event to a vector
+outside the MSI vector supported by the device,
+as reported by \field{Multiple Message Capable} field in the MSI Capability.
+
+After mapping an event to vector, the
+driver MUST verify success by reading the Vector field value: on
+success, the previously written value is returned, and on
+failure, NO_VECTOR is returned. If a mapping failure is detected,
+the driver MAY retry mapping with fewer vectors, disable MSI
+or report device failure.
+
 \paragraph{Virtqueue Configuration}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / Virtqueue Configuration}
 
 As a device can have zero or more virtqueues for bulk data
@@ -1054,10 +1123,10 @@ \subsubsection{Device Initialization}\label{sec:Virtio Transport Options / Virti
 \item Allocate and zero Descriptor Table, Available and Used rings for the
    virtqueue in contiguous physical memory.
 
-\item Optionally, if MSI-X capability is present and enabled on the
+\item Optionally, if MSI/MSI-X capability is present and enabled on the
   device, select a vector to use to request interrupts triggered
-  by virtqueue events. Write the MSI-X Table entry number
-  corresponding to this vector into \field{queue_msix_vector}. Read
+  by virtqueue events. Write the MSI-X Table entry number or MSI vector
+  corresponding to this event into \field{queue_msix_vector}. Read
   \field{queue_msix_vector}: on success, previously written value is
   returned; on failure, NO_VECTOR value is returned.
 \end{enumerate}
@@ -1129,25 +1198,25 @@ \subsubsection{Used Buffer Notifications}\label{sec:Virtio Transport Options / V
 If a used buffer notification is necessary for a virtqueue, the device would typically act as follows:
 
 \begin{itemize}
-  \item If MSI-X capability is disabled:
+  \item If MSI/MSI-X capability is disabled:
     \begin{enumerate}
     \item Set the lower bit of the ISR Status field for the device.
 
     \item Send the appropriate PCI interrupt for the device.
     \end{enumerate}
 
-  \item If MSI-X capability is enabled:
+  \item If MSI/MSI-X capability is enabled:
     \begin{enumerate}
     \item If \field{queue_msix_vector} is not NO_VECTOR,
-      request the appropriate MSI-X interrupt message for the
+      request the appropriate MSI/MSI-X interrupt message for the
       device, \field{queue_msix_vector} sets the MSI-X Table entry
-      number.
+      number or MSI vector.
     \end{enumerate}
 \end{itemize}
 
 \devicenormative{\paragraph}{Used Buffer Notifications}{Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Used Buffer Notifications}
 
-If MSI-X capability is enabled and \field{queue_msix_vector} is
+If MSI/MSI-X capability is enabled and \field{queue_msix_vector} is
 NO_VECTOR for a virtqueue, the device MUST NOT deliver an interrupt
 for that virtqueue.
 
@@ -1157,19 +1226,19 @@ \subsubsection{Notification of Device Configuration Changes}\label{sec:Virtio Tr
 state, as reflected in the device-specific configuration region of the device. In this case:
 
 \begin{itemize}
-  \item If MSI-X capability is disabled:
+  \item If MSI/MSI-X capability is disabled:
     \begin{enumerate}
     \item Set the second lower bit of the ISR Status field for the device.
 
     \item Send the appropriate PCI interrupt for the device.
     \end{enumerate}
 
-  \item If MSI-X capability is enabled:
+  \item If MSI/MSI-X capability is enabled:
     \begin{enumerate}
     \item If \field{config_msix_vector} is not NO_VECTOR,
-      request the appropriate MSI-X interrupt message for the
+      request the appropriate MSI/MSI-X interrupt message for the
       device, \field{config_msix_vector} sets the MSI-X Table entry
-      number.
+      number or MSI vector.
     \end{enumerate}
 \end{itemize}
 
@@ -1178,7 +1247,7 @@ \subsubsection{Notification of Device Configuration Changes}\label{sec:Virtio Tr
 
 \devicenormative{\paragraph}{Notification of Device Configuration Changes}{Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
 
-If MSI-X capability is enabled and \field{config_msix_vector} is
+If MSI/MSI-X capability is enabled and \field{config_msix_vector} is
 NO_VECTOR, the device MUST NOT deliver an interrupt
 for device configuration space changes.
 
@@ -1191,7 +1260,7 @@ \subsubsection{Driver Handling Interrupts}\label{sec:Virtio Transport Options /
 The driver interrupt handler would typically:
 
 \begin{itemize}
-  \item If MSI-X capability is disabled:
+  \item If MSI/MSI-X capability is disabled:
     \begin{itemize}
       \item Read the ISR Status field, which will reset it to zero.
       \item If the lower bit is set:
@@ -1201,14 +1270,14 @@ \subsubsection{Driver Handling Interrupts}\label{sec:Virtio Transport Options /
       \item If the second lower bit is set:
         re-examine the configuration space to see what changed.
     \end{itemize}
-  \item If MSI-X capability is enabled:
+  \item If MSI/MSI-X capability is enabled:
     \begin{itemize}
       \item
-        Look through all virtqueues mapped to that MSI-X vector for the
+        Look through all virtqueues mapped to that MSI/MSI-X vector for the
         device, to see if any progress has been made by the device
         which requires servicing.
       \item
-        If the MSI-X vector is equal to \field{config_msix_vector},
+        If the MSI/MSI-X vector is equal to \field{config_msix_vector},
         re-examine the configuration space to see what changed.
     \end{itemize}
 \end{itemize}
-- 
2.25.1

Re: [PATCH v2] transport-pci: Add MSI support

[Manivannan Sadhasivam]

On Fri, Jul 12, 2024 at 07:31:44PM +0530, Manivannan Sadhasivam wrote:
> MSI is the predecessor of MSI-X that allows PCIe devices to send interrupts
> to the host. Compared to MSI-X, MSI supports only a maximum of 32 vectors
> per PCIe function. But MSI has been widely supported by the PCIe devices
> requiring fewer interrupts such as Modems, WLAN cards etc...
> 
> Currently, Virtio spec only documents MSI-X and INTX interrupt mechanisms
> for the PCI transport. So if a Virtio device based on PCI transport
> supports only MSI, then the driver on the guest will only use INTX for
> receiving the interrupts. This is really sub-optimal and affects the
> performance of the device. Because with MSI, the device can use one vector
> per queue (max of 32 vectors) thus avoiding the overhead associated with a
> shared INTX vector.
> 
> Hence, add support for MSI to the Virtio PCI transport. MSI support is
> added such a way that it reuses the existing infrastructure of MSI-X, like
> the config_msix_vector/queue_msix_vector fields of the Virtio common config
> structure. This makes it easy for the Virtio drivers to add MSI support
> without any disruptive changes.
> 

Gentle ping!

- Mani

> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
> 
> Changes in v2:
> 
> * Fixed a spelling mistake in commit message
> * Removed update to legacy interface
> * Used 'MSI vector' consistently
> 
>  transport-pci.tex | 115 ++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 92 insertions(+), 23 deletions(-)
> 
> diff --git a/transport-pci.tex b/transport-pci.tex
> index a5c6719..fd92641 100644
> --- a/transport-pci.tex
> +++ b/transport-pci.tex
> @@ -347,7 +347,7 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>          Driver Feature Bits selected by \field{driver_feature_select}.
>  
>  \item[\field{config_msix_vector}]
> -        Set by the driver to the MSI-X vector for configuration change notifications.
> +        Set by the driver to the MSI/MSI-X vector for configuration change notifications.
>  
>  \item[\field{num_queues}]
>          The device specifies the maximum number of virtqueues supported here.
> @@ -371,7 +371,7 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>          A 0 means the queue is unavailable.
>  
>  \item[\field{queue_msix_vector}]
> -        Set by the driver to the MSI-X vector for virtqueue notifications.
> +        Set by the driver to the MSI/MSI-X vector for virtqueue notifications.
>  
>  \item[\field{queue_enable}]
>          The driver uses this to selectively prevent the device from executing requests from this virtqueue.
> @@ -631,11 +631,11 @@ \subsubsection{ISR status capability}\label{sec:Virtio Transport Options / Virti
>  in \field{ISR status} before sending a device configuration
>  change notification to the driver.
>  
> -If MSI-X capability is disabled, the device MUST set the Queue
> +If MSI/MSI-X capability is disabled, the device MUST set the Queue
>  Interrupt bit in \field{ISR status} before sending a virtqueue
>  notification to the driver.
>  
> -If MSI-X capability is disabled, the device MUST set the Interrupt Status
> +If MSI/MSI-X capability is disabled, the device MUST set the Interrupt Status
>  bit in the PCI Status register in the PCI Configuration Header of
>  the device to the logical OR of all bits in \field{ISR status} of
>  the device.  The device then asserts/deasserts INT\#x interrupts unless masked
> @@ -645,7 +645,7 @@ \subsubsection{ISR status capability}\label{sec:Virtio Transport Options / Virti
>  
>  \drivernormative{\paragraph}{ISR status capability}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / ISR status capability}
>  
> -If MSI-X capability is enabled, the driver SHOULD NOT access
> +If MSI/MSI-X capability is enabled, the driver SHOULD NOT access
>  \field{ISR status} upon detecting a Queue Interrupt.
>  
>  \subsubsection{Device-specific configuration}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Device-specific configuration}
> @@ -1017,7 +1017,7 @@ \subsubsection{Device Initialization}\label{sec:Virtio Transport Options / Virti
>  \drivernormative{\subparagraph}{MSI-X Vector Configuration}{Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
>  
>  Driver MUST support device with any MSI-X Table Size 0 to 0x7FF.
> -Driver MAY fall back on using INT\#x interrupts for a device
> +Driver MAY fall back on using MSI or INT\#x interrupts for a device
>  which only supports one MSI-X vector (MSI-X Table Size = 0).
>  
>  Driver MAY interpret the Table Size as a hint from the device
> @@ -1034,6 +1034,75 @@ \subsubsection{Device Initialization}\label{sec:Virtio Transport Options / Virti
>  the driver MAY retry mapping with fewer vectors, disable MSI-X
>  or report device failure.
>  
> +\paragraph{MSI Vector Configuration}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI Vector Configuration}
> +
> +When MSI capability is present and enabled in the device
> +(through standard PCI configuration space) \field{config_msix_vector} and \field{queue_msix_vector} are used to map configuration change and queue
> +interrupts to MSI vectors. In this case, the ISR Status is unused.
> +
> +Writing a valid MSI vector, 0 to 0x1F, to
> +\field{config_msix_vector}/\field{queue_msix_vector} maps interrupts triggered
> +by the configuration change/selected queue events respectively to
> +the corresponding MSI vector. To disable interrupts for an
> +event type, the driver unmaps this event by writing a special NO_VECTOR
> +value:
> +
> +\begin{lstlisting}
> +/* Vector value used to disable MSI for queue */
> +#define VIRTIO_MSI_NO_VECTOR            0xffff
> +\end{lstlisting}
> +
> +Note that mapping an event to vector might require device to
> +allocate internal device resources, and thus could fail.
> +
> +\devicenormative{\subparagraph}{MSI Vector Configuration}{Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI Vector Configuration}
> +
> +A device that has an MSI capability SHOULD support at least 2
> +and at most 0x20 MSI vectors.
> +Device MUST report the number of vectors supported in
> +\field{Multiple Message Capable} field in the MSI Capability as specified in
> +\hyperref[intro:PCI]{[PCI]}.
> +The device SHOULD restrict the reported MSI Multiple Message Capable field
> +to a value that might benefit system performance.
> +\begin{note}
> +For example, a device which does not expect to send
> +interrupts at a high rate might only specify 2 MSI vectors.
> +\end{note}
> +Device MUST support mapping any event type to any valid
> +vector 0 to number of MSI vectors specified in \field{Multiple Message Capable} field.
> +Device MUST support unmapping any event type.
> +
> +The device MUST return vector mapped to a given event,
> +(NO_VECTOR if unmapped) on read of \field{config_msix_vector}/\field{queue_msix_vector}.
> +The device MUST have all queue and configuration change
> +events unmapped upon reset.
> +
> +Devices SHOULD NOT cause mapping an event to vector to fail
> +unless it is impossible for the device to satisfy the mapping
> +request.  Devices MUST report mapping
> +failures by returning the NO_VECTOR value when the relevant
> +\field{config_msix_vector}/\field{queue_msix_vector} field is read.
> +
> +\drivernormative{\subparagraph}{MSI Vector Configuration}{Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI Vector Configuration}
> +
> +Driver MUST support device with any MSI vector from 0 to 0x1F.
> +Driver MAY fall back on using INT\#x interrupts for a device
> +which only supports one MSI vector (MSI Multiple Message Capable = 0).
> +
> +Driver MAY interpret the Multiple Message Capable field as a hint from the device
> +for the suggested number of MSI vectors to use.
> +
> +Driver MUST NOT attempt to map an event to a vector
> +outside the MSI vector supported by the device,
> +as reported by \field{Multiple Message Capable} field in the MSI Capability.
> +
> +After mapping an event to vector, the
> +driver MUST verify success by reading the Vector field value: on
> +success, the previously written value is returned, and on
> +failure, NO_VECTOR is returned. If a mapping failure is detected,
> +the driver MAY retry mapping with fewer vectors, disable MSI
> +or report device failure.
> +
>  \paragraph{Virtqueue Configuration}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / Virtqueue Configuration}
>  
>  As a device can have zero or more virtqueues for bulk data
> @@ -1054,10 +1123,10 @@ \subsubsection{Device Initialization}\label{sec:Virtio Transport Options / Virti
>  \item Allocate and zero Descriptor Table, Available and Used rings for the
>     virtqueue in contiguous physical memory.
>  
> -\item Optionally, if MSI-X capability is present and enabled on the
> +\item Optionally, if MSI/MSI-X capability is present and enabled on the
>    device, select a vector to use to request interrupts triggered
> -  by virtqueue events. Write the MSI-X Table entry number
> -  corresponding to this vector into \field{queue_msix_vector}. Read
> +  by virtqueue events. Write the MSI-X Table entry number or MSI vector
> +  corresponding to this event into \field{queue_msix_vector}. Read
>    \field{queue_msix_vector}: on success, previously written value is
>    returned; on failure, NO_VECTOR value is returned.
>  \end{enumerate}
> @@ -1129,25 +1198,25 @@ \subsubsection{Used Buffer Notifications}\label{sec:Virtio Transport Options / V
>  If a used buffer notification is necessary for a virtqueue, the device would typically act as follows:
>  
>  \begin{itemize}
> -  \item If MSI-X capability is disabled:
> +  \item If MSI/MSI-X capability is disabled:
>      \begin{enumerate}
>      \item Set the lower bit of the ISR Status field for the device.
>  
>      \item Send the appropriate PCI interrupt for the device.
>      \end{enumerate}
>  
> -  \item If MSI-X capability is enabled:
> +  \item If MSI/MSI-X capability is enabled:
>      \begin{enumerate}
>      \item If \field{queue_msix_vector} is not NO_VECTOR,
> -      request the appropriate MSI-X interrupt message for the
> +      request the appropriate MSI/MSI-X interrupt message for the
>        device, \field{queue_msix_vector} sets the MSI-X Table entry
> -      number.
> +      number or MSI vector.
>      \end{enumerate}
>  \end{itemize}
>  
>  \devicenormative{\paragraph}{Used Buffer Notifications}{Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Used Buffer Notifications}
>  
> -If MSI-X capability is enabled and \field{queue_msix_vector} is
> +If MSI/MSI-X capability is enabled and \field{queue_msix_vector} is
>  NO_VECTOR for a virtqueue, the device MUST NOT deliver an interrupt
>  for that virtqueue.
>  
> @@ -1157,19 +1226,19 @@ \subsubsection{Notification of Device Configuration Changes}\label{sec:Virtio Tr
>  state, as reflected in the device-specific configuration region of the device. In this case:
>  
>  \begin{itemize}
> -  \item If MSI-X capability is disabled:
> +  \item If MSI/MSI-X capability is disabled:
>      \begin{enumerate}
>      \item Set the second lower bit of the ISR Status field for the device.
>  
>      \item Send the appropriate PCI interrupt for the device.
>      \end{enumerate}
>  
> -  \item If MSI-X capability is enabled:
> +  \item If MSI/MSI-X capability is enabled:
>      \begin{enumerate}
>      \item If \field{config_msix_vector} is not NO_VECTOR,
> -      request the appropriate MSI-X interrupt message for the
> +      request the appropriate MSI/MSI-X interrupt message for the
>        device, \field{config_msix_vector} sets the MSI-X Table entry
> -      number.
> +      number or MSI vector.
>      \end{enumerate}
>  \end{itemize}
>  
> @@ -1178,7 +1247,7 @@ \subsubsection{Notification of Device Configuration Changes}\label{sec:Virtio Tr
>  
>  \devicenormative{\paragraph}{Notification of Device Configuration Changes}{Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
>  
> -If MSI-X capability is enabled and \field{config_msix_vector} is
> +If MSI/MSI-X capability is enabled and \field{config_msix_vector} is
>  NO_VECTOR, the device MUST NOT deliver an interrupt
>  for device configuration space changes.
>  
> @@ -1191,7 +1260,7 @@ \subsubsection{Driver Handling Interrupts}\label{sec:Virtio Transport Options /
>  The driver interrupt handler would typically:
>  
>  \begin{itemize}
> -  \item If MSI-X capability is disabled:
> +  \item If MSI/MSI-X capability is disabled:
>      \begin{itemize}
>        \item Read the ISR Status field, which will reset it to zero.
>        \item If the lower bit is set:
> @@ -1201,14 +1270,14 @@ \subsubsection{Driver Handling Interrupts}\label{sec:Virtio Transport Options /
>        \item If the second lower bit is set:
>          re-examine the configuration space to see what changed.
>      \end{itemize}
> -  \item If MSI-X capability is enabled:
> +  \item If MSI/MSI-X capability is enabled:
>      \begin{itemize}
>        \item
> -        Look through all virtqueues mapped to that MSI-X vector for the
> +        Look through all virtqueues mapped to that MSI/MSI-X vector for the
>          device, to see if any progress has been made by the device
>          which requires servicing.
>        \item
> -        If the MSI-X vector is equal to \field{config_msix_vector},
> +        If the MSI/MSI-X vector is equal to \field{config_msix_vector},
>          re-examine the configuration space to see what changed.
>      \end{itemize}
>  \end{itemize}
> -- 
> 2.25.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2] transport-pci: Add MSI support

[Manivannan Sadhasivam]

On Wed, Jul 24, 2024 at 09:51:43PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Jul 12, 2024 at 07:31:44PM +0530, Manivannan Sadhasivam wrote:
> > MSI is the predecessor of MSI-X that allows PCIe devices to send interrupts
> > to the host. Compared to MSI-X, MSI supports only a maximum of 32 vectors
> > per PCIe function. But MSI has been widely supported by the PCIe devices
> > requiring fewer interrupts such as Modems, WLAN cards etc...
> > 
> > Currently, Virtio spec only documents MSI-X and INTX interrupt mechanisms
> > for the PCI transport. So if a Virtio device based on PCI transport
> > supports only MSI, then the driver on the guest will only use INTX for
> > receiving the interrupts. This is really sub-optimal and affects the
> > performance of the device. Because with MSI, the device can use one vector
> > per queue (max of 32 vectors) thus avoiding the overhead associated with a
> > shared INTX vector.
> > 
> > Hence, add support for MSI to the Virtio PCI transport. MSI support is
> > added such a way that it reuses the existing infrastructure of MSI-X, like
> > the config_msix_vector/queue_msix_vector fields of the Virtio common config
> > structure. This makes it easy for the Virtio drivers to add MSI support
> > without any disruptive changes.
> > 
> 
> Gentle ping!
> 

Ping again. Virtio maintainers, can you please share some feedback?

- Mani

> - Mani
> 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> > 
> > Changes in v2:
> > 
> > * Fixed a spelling mistake in commit message
> > * Removed update to legacy interface
> > * Used 'MSI vector' consistently
> > 
> >  transport-pci.tex | 115 ++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 92 insertions(+), 23 deletions(-)
> > 
> > diff --git a/transport-pci.tex b/transport-pci.tex
> > index a5c6719..fd92641 100644
> > --- a/transport-pci.tex
> > +++ b/transport-pci.tex
> > @@ -347,7 +347,7 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
> >          Driver Feature Bits selected by \field{driver_feature_select}.
> >  
> >  \item[\field{config_msix_vector}]
> > -        Set by the driver to the MSI-X vector for configuration change notifications.
> > +        Set by the driver to the MSI/MSI-X vector for configuration change notifications.
> >  
> >  \item[\field{num_queues}]
> >          The device specifies the maximum number of virtqueues supported here.
> > @@ -371,7 +371,7 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
> >          A 0 means the queue is unavailable.
> >  
> >  \item[\field{queue_msix_vector}]
> > -        Set by the driver to the MSI-X vector for virtqueue notifications.
> > +        Set by the driver to the MSI/MSI-X vector for virtqueue notifications.
> >  
> >  \item[\field{queue_enable}]
> >          The driver uses this to selectively prevent the device from executing requests from this virtqueue.
> > @@ -631,11 +631,11 @@ \subsubsection{ISR status capability}\label{sec:Virtio Transport Options / Virti
> >  in \field{ISR status} before sending a device configuration
> >  change notification to the driver.
> >  
> > -If MSI-X capability is disabled, the device MUST set the Queue
> > +If MSI/MSI-X capability is disabled, the device MUST set the Queue
> >  Interrupt bit in \field{ISR status} before sending a virtqueue
> >  notification to the driver.
> >  
> > -If MSI-X capability is disabled, the device MUST set the Interrupt Status
> > +If MSI/MSI-X capability is disabled, the device MUST set the Interrupt Status
> >  bit in the PCI Status register in the PCI Configuration Header of
> >  the device to the logical OR of all bits in \field{ISR status} of
> >  the device.  The device then asserts/deasserts INT\#x interrupts unless masked
> > @@ -645,7 +645,7 @@ \subsubsection{ISR status capability}\label{sec:Virtio Transport Options / Virti
> >  
> >  \drivernormative{\paragraph}{ISR status capability}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / ISR status capability}
> >  
> > -If MSI-X capability is enabled, the driver SHOULD NOT access
> > +If MSI/MSI-X capability is enabled, the driver SHOULD NOT access
> >  \field{ISR status} upon detecting a Queue Interrupt.
> >  
> >  \subsubsection{Device-specific configuration}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Device-specific configuration}
> > @@ -1017,7 +1017,7 @@ \subsubsection{Device Initialization}\label{sec:Virtio Transport Options / Virti
> >  \drivernormative{\subparagraph}{MSI-X Vector Configuration}{Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
> >  
> >  Driver MUST support device with any MSI-X Table Size 0 to 0x7FF.
> > -Driver MAY fall back on using INT\#x interrupts for a device
> > +Driver MAY fall back on using MSI or INT\#x interrupts for a device
> >  which only supports one MSI-X vector (MSI-X Table Size = 0).
> >  
> >  Driver MAY interpret the Table Size as a hint from the device
> > @@ -1034,6 +1034,75 @@ \subsubsection{Device Initialization}\label{sec:Virtio Transport Options / Virti
> >  the driver MAY retry mapping with fewer vectors, disable MSI-X
> >  or report device failure.
> >  
> > +\paragraph{MSI Vector Configuration}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI Vector Configuration}
> > +
> > +When MSI capability is present and enabled in the device
> > +(through standard PCI configuration space) \field{config_msix_vector} and \field{queue_msix_vector} are used to map configuration change and queue
> > +interrupts to MSI vectors. In this case, the ISR Status is unused.
> > +
> > +Writing a valid MSI vector, 0 to 0x1F, to
> > +\field{config_msix_vector}/\field{queue_msix_vector} maps interrupts triggered
> > +by the configuration change/selected queue events respectively to
> > +the corresponding MSI vector. To disable interrupts for an
> > +event type, the driver unmaps this event by writing a special NO_VECTOR
> > +value:
> > +
> > +\begin{lstlisting}
> > +/* Vector value used to disable MSI for queue */
> > +#define VIRTIO_MSI_NO_VECTOR            0xffff
> > +\end{lstlisting}
> > +
> > +Note that mapping an event to vector might require device to
> > +allocate internal device resources, and thus could fail.
> > +
> > +\devicenormative{\subparagraph}{MSI Vector Configuration}{Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI Vector Configuration}
> > +
> > +A device that has an MSI capability SHOULD support at least 2
> > +and at most 0x20 MSI vectors.
> > +Device MUST report the number of vectors supported in
> > +\field{Multiple Message Capable} field in the MSI Capability as specified in
> > +\hyperref[intro:PCI]{[PCI]}.
> > +The device SHOULD restrict the reported MSI Multiple Message Capable field
> > +to a value that might benefit system performance.
> > +\begin{note}
> > +For example, a device which does not expect to send
> > +interrupts at a high rate might only specify 2 MSI vectors.
> > +\end{note}
> > +Device MUST support mapping any event type to any valid
> > +vector 0 to number of MSI vectors specified in \field{Multiple Message Capable} field.
> > +Device MUST support unmapping any event type.
> > +
> > +The device MUST return vector mapped to a given event,
> > +(NO_VECTOR if unmapped) on read of \field{config_msix_vector}/\field{queue_msix_vector}.
> > +The device MUST have all queue and configuration change
> > +events unmapped upon reset.
> > +
> > +Devices SHOULD NOT cause mapping an event to vector to fail
> > +unless it is impossible for the device to satisfy the mapping
> > +request.  Devices MUST report mapping
> > +failures by returning the NO_VECTOR value when the relevant
> > +\field{config_msix_vector}/\field{queue_msix_vector} field is read.
> > +
> > +\drivernormative{\subparagraph}{MSI Vector Configuration}{Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI Vector Configuration}
> > +
> > +Driver MUST support device with any MSI vector from 0 to 0x1F.
> > +Driver MAY fall back on using INT\#x interrupts for a device
> > +which only supports one MSI vector (MSI Multiple Message Capable = 0).
> > +
> > +Driver MAY interpret the Multiple Message Capable field as a hint from the device
> > +for the suggested number of MSI vectors to use.
> > +
> > +Driver MUST NOT attempt to map an event to a vector
> > +outside the MSI vector supported by the device,
> > +as reported by \field{Multiple Message Capable} field in the MSI Capability.
> > +
> > +After mapping an event to vector, the
> > +driver MUST verify success by reading the Vector field value: on
> > +success, the previously written value is returned, and on
> > +failure, NO_VECTOR is returned. If a mapping failure is detected,
> > +the driver MAY retry mapping with fewer vectors, disable MSI
> > +or report device failure.
> > +
> >  \paragraph{Virtqueue Configuration}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / Virtqueue Configuration}
> >  
> >  As a device can have zero or more virtqueues for bulk data
> > @@ -1054,10 +1123,10 @@ \subsubsection{Device Initialization}\label{sec:Virtio Transport Options / Virti
> >  \item Allocate and zero Descriptor Table, Available and Used rings for the
> >     virtqueue in contiguous physical memory.
> >  
> > -\item Optionally, if MSI-X capability is present and enabled on the
> > +\item Optionally, if MSI/MSI-X capability is present and enabled on the
> >    device, select a vector to use to request interrupts triggered
> > -  by virtqueue events. Write the MSI-X Table entry number
> > -  corresponding to this vector into \field{queue_msix_vector}. Read
> > +  by virtqueue events. Write the MSI-X Table entry number or MSI vector
> > +  corresponding to this event into \field{queue_msix_vector}. Read
> >    \field{queue_msix_vector}: on success, previously written value is
> >    returned; on failure, NO_VECTOR value is returned.
> >  \end{enumerate}
> > @@ -1129,25 +1198,25 @@ \subsubsection{Used Buffer Notifications}\label{sec:Virtio Transport Options / V
> >  If a used buffer notification is necessary for a virtqueue, the device would typically act as follows:
> >  
> >  \begin{itemize}
> > -  \item If MSI-X capability is disabled:
> > +  \item If MSI/MSI-X capability is disabled:
> >      \begin{enumerate}
> >      \item Set the lower bit of the ISR Status field for the device.
> >  
> >      \item Send the appropriate PCI interrupt for the device.
> >      \end{enumerate}
> >  
> > -  \item If MSI-X capability is enabled:
> > +  \item If MSI/MSI-X capability is enabled:
> >      \begin{enumerate}
> >      \item If \field{queue_msix_vector} is not NO_VECTOR,
> > -      request the appropriate MSI-X interrupt message for the
> > +      request the appropriate MSI/MSI-X interrupt message for the
> >        device, \field{queue_msix_vector} sets the MSI-X Table entry
> > -      number.
> > +      number or MSI vector.
> >      \end{enumerate}
> >  \end{itemize}
> >  
> >  \devicenormative{\paragraph}{Used Buffer Notifications}{Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Used Buffer Notifications}
> >  
> > -If MSI-X capability is enabled and \field{queue_msix_vector} is
> > +If MSI/MSI-X capability is enabled and \field{queue_msix_vector} is
> >  NO_VECTOR for a virtqueue, the device MUST NOT deliver an interrupt
> >  for that virtqueue.
> >  
> > @@ -1157,19 +1226,19 @@ \subsubsection{Notification of Device Configuration Changes}\label{sec:Virtio Tr
> >  state, as reflected in the device-specific configuration region of the device. In this case:
> >  
> >  \begin{itemize}
> > -  \item If MSI-X capability is disabled:
> > +  \item If MSI/MSI-X capability is disabled:
> >      \begin{enumerate}
> >      \item Set the second lower bit of the ISR Status field for the device.
> >  
> >      \item Send the appropriate PCI interrupt for the device.
> >      \end{enumerate}
> >  
> > -  \item If MSI-X capability is enabled:
> > +  \item If MSI/MSI-X capability is enabled:
> >      \begin{enumerate}
> >      \item If \field{config_msix_vector} is not NO_VECTOR,
> > -      request the appropriate MSI-X interrupt message for the
> > +      request the appropriate MSI/MSI-X interrupt message for the
> >        device, \field{config_msix_vector} sets the MSI-X Table entry
> > -      number.
> > +      number or MSI vector.
> >      \end{enumerate}
> >  \end{itemize}
> >  
> > @@ -1178,7 +1247,7 @@ \subsubsection{Notification of Device Configuration Changes}\label{sec:Virtio Tr
> >  
> >  \devicenormative{\paragraph}{Notification of Device Configuration Changes}{Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
> >  
> > -If MSI-X capability is enabled and \field{config_msix_vector} is
> > +If MSI/MSI-X capability is enabled and \field{config_msix_vector} is
> >  NO_VECTOR, the device MUST NOT deliver an interrupt
> >  for device configuration space changes.
> >  
> > @@ -1191,7 +1260,7 @@ \subsubsection{Driver Handling Interrupts}\label{sec:Virtio Transport Options /
> >  The driver interrupt handler would typically:
> >  
> >  \begin{itemize}
> > -  \item If MSI-X capability is disabled:
> > +  \item If MSI/MSI-X capability is disabled:
> >      \begin{itemize}
> >        \item Read the ISR Status field, which will reset it to zero.
> >        \item If the lower bit is set:
> > @@ -1201,14 +1270,14 @@ \subsubsection{Driver Handling Interrupts}\label{sec:Virtio Transport Options /
> >        \item If the second lower bit is set:
> >          re-examine the configuration space to see what changed.
> >      \end{itemize}
> > -  \item If MSI-X capability is enabled:
> > +  \item If MSI/MSI-X capability is enabled:
> >      \begin{itemize}
> >        \item
> > -        Look through all virtqueues mapped to that MSI-X vector for the
> > +        Look through all virtqueues mapped to that MSI/MSI-X vector for the
> >          device, to see if any progress has been made by the device
> >          which requires servicing.
> >        \item
> > -        If the MSI-X vector is equal to \field{config_msix_vector},
> > +        If the MSI/MSI-X vector is equal to \field{config_msix_vector},
> >          re-examine the configuration space to see what changed.
> >      \end{itemize}
> >  \end{itemize}
> > -- 
> > 2.25.1
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்

-- 
மணிவண்ணன் சதாசிவம்

RE: [PATCH v2] transport-pci: Add MSI support

[Parav Pandit]

> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Sent: Tuesday, September 3, 2024 11:02 AM
> 
> On Wed, Jul 24, 2024 at 09:51:43PM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Jul 12, 2024 at 07:31:44PM +0530, Manivannan Sadhasivam wrote:
> > > MSI is the predecessor of MSI-X that allows PCIe devices to send
> > > interrupts to the host. Compared to MSI-X, MSI supports only a
> > > maximum of 32 vectors per PCIe function. But MSI has been widely
> > > supported by the PCIe devices requiring fewer interrupts such as
> Modems, WLAN cards etc...
> > >
> > > Currently, Virtio spec only documents MSI-X and INTX interrupt
> > > mechanisms for the PCI transport. So if a Virtio device based on PCI
> > > transport supports only MSI, then the driver on the guest will only
> > > use INTX for receiving the interrupts. This is really sub-optimal
> > > and affects the performance of the device. Because with MSI, the
> > > device can use one vector per queue (max of 32 vectors) thus
> > > avoiding the overhead associated with a shared INTX vector.
> > >
> > > Hence, add support for MSI to the Virtio PCI transport. MSI support
> > > is added such a way that it reuses the existing infrastructure of
> > > MSI-X, like the config_msix_vector/queue_msix_vector fields of the
> > > Virtio common config structure. This makes it easy for the Virtio
> > > drivers to add MSI support without any disruptive changes.
> > >
> >
> > Gentle ping!
> >
> 
> Ping again. Virtio maintainers, can you please share some feedback?
>
Overall looks to me.
A small comment below on single MSI vector.

 
> - Mani
> 
> > - Mani
> >
> > > Signed-off-by: Manivannan Sadhasivam
> > > <manivannan.sadhasivam@linaro.org>
> > > ---
> > >
> > > Changes in v2:
> > >
> > > * Fixed a spelling mistake in commit message
> > > * Removed update to legacy interface
> > > * Used 'MSI vector' consistently
> > >
> > >  transport-pci.tex | 115
> > > ++++++++++++++++++++++++++++++++++++----------
> > >  1 file changed, 92 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/transport-pci.tex b/transport-pci.tex index
> > > a5c6719..fd92641 100644
> > > --- a/transport-pci.tex
> > > +++ b/transport-pci.tex
> > > @@ -347,7 +347,7 @@ \subsubsection{Common configuration structure
> layout}\label{sec:Virtio Transport
> > >          Driver Feature Bits selected by \field{driver_feature_select}.
> > >
> > >  \item[\field{config_msix_vector}]
> > > -        Set by the driver to the MSI-X vector for configuration change
> notifications.
> > > +        Set by the driver to the MSI/MSI-X vector for configuration change
> notifications.
> > >
> > >  \item[\field{num_queues}]
> > >          The device specifies the maximum number of virtqueues supported
> here.
> > > @@ -371,7 +371,7 @@ \subsubsection{Common configuration structure
> layout}\label{sec:Virtio Transport
> > >          A 0 means the queue is unavailable.
> > >
> > >  \item[\field{queue_msix_vector}]
> > > -        Set by the driver to the MSI-X vector for virtqueue notifications.
> > > +        Set by the driver to the MSI/MSI-X vector for virtqueue
> notifications.
> > >
> > >  \item[\field{queue_enable}]
> > >          The driver uses this to selectively prevent the device from executing
> requests from this virtqueue.
> > > @@ -631,11 +631,11 @@ \subsubsection{ISR status
> > > capability}\label{sec:Virtio Transport Options / Virti  in
> > > \field{ISR status} before sending a device configuration  change
> notification to the driver.
> > >
> > > -If MSI-X capability is disabled, the device MUST set the Queue
> > > +If MSI/MSI-X capability is disabled, the device MUST set the Queue
> > >  Interrupt bit in \field{ISR status} before sending a virtqueue
> > > notification to the driver.
> > >
> > > -If MSI-X capability is disabled, the device MUST set the Interrupt
> > > Status
> > > +If MSI/MSI-X capability is disabled, the device MUST set the
> > > +Interrupt Status
> > >  bit in the PCI Status register in the PCI Configuration Header of
> > > the device to the logical OR of all bits in \field{ISR status} of
> > > the device.  The device then asserts/deasserts INT\#x interrupts
> > > unless masked @@ -645,7 +645,7 @@ \subsubsection{ISR status
> > > capability}\label{sec:Virtio Transport Options / Virti
> > >
> > >  \drivernormative{\paragraph}{ISR status capability}{Virtio
> > > Transport Options / Virtio Over PCI Bus / PCI Device Layout / ISR
> > > status capability}
> > >
> > > -If MSI-X capability is enabled, the driver SHOULD NOT access
> > > +If MSI/MSI-X capability is enabled, the driver SHOULD NOT access
> > >  \field{ISR status} upon detecting a Queue Interrupt.
> > >
> > >  \subsubsection{Device-specific configuration}\label{sec:Virtio
> > > Transport Options / Virtio Over PCI Bus / PCI Device Layout /
> > > Device-specific configuration} @@ -1017,7 +1017,7 @@
> > > \subsubsection{Device Initialization}\label{sec:Virtio Transport
> > > Options / Virti  \drivernormative{\subparagraph}{MSI-X Vector
> > > Configuration}{Virtio Transport Options / Virtio Over PCI Bus /
> > > PCI-specific Initialization And Device Operation / Device
> > > Initialization / MSI-X Vector Configuration}
> > >
> > >  Driver MUST support device with any MSI-X Table Size 0 to 0x7FF.
> > > -Driver MAY fall back on using INT\#x interrupts for a device
> > > +Driver MAY fall back on using MSI or INT\#x interrupts for a device
> > >  which only supports one MSI-X vector (MSI-X Table Size = 0).
> > >
> > >  Driver MAY interpret the Table Size as a hint from the device @@
> > > -1034,6 +1034,75 @@ \subsubsection{Device
> > > Initialization}\label{sec:Virtio Transport Options / Virti  the
> > > driver MAY retry mapping with fewer vectors, disable MSI-X  or report
> device failure.
> > >
> > > +\paragraph{MSI Vector Configuration}\label{sec:Virtio Transport
> > > +Options / Virtio Over PCI Bus / PCI-specific Initialization And
> > > +Device Operation / Device Initialization / MSI Vector
> > > +Configuration}
> > > +
> > > +When MSI capability is present and enabled in the device (through
> > > +standard PCI configuration space) \field{config_msix_vector} and
> > > +\field{queue_msix_vector} are used to map configuration change and
> queue interrupts to MSI vectors. In this case, the ISR Status is unused.
> > > +
> > > +Writing a valid MSI vector, 0 to 0x1F, to
> > > +\field{config_msix_vector}/\field{queue_msix_vector} maps
> > > +interrupts triggered by the configuration change/selected queue
> > > +events respectively to the corresponding MSI vector. To disable
> > > +interrupts for an event type, the driver unmaps this event by
> > > +writing a special NO_VECTOR
> > > +value:
> > > +
> > > +\begin{lstlisting}
> > > +/* Vector value used to disable MSI for queue */
> > > +#define VIRTIO_MSI_NO_VECTOR            0xffff
> > > +\end{lstlisting}
> > > +
> > > +Note that mapping an event to vector might require device to
> > > +allocate internal device resources, and thus could fail.
> > > +
> > > +\devicenormative{\subparagraph}{MSI Vector Configuration}{Virtio
> > > +Transport Options / Virtio Over PCI Bus / PCI-specific
> > > +Initialization And Device Operation / Device Initialization / MSI
> > > +Vector Configuration}
> > > +
> > > +A device that has an MSI capability SHOULD support at least 2 and
> > > +at most 0x20 MSI vectors.
> > > +Device MUST report the number of vectors supported in
> > > +\field{Multiple Message Capable} field in the MSI Capability as
> > > +specified in \hyperref[intro:PCI]{[PCI]}.
> > > +The device SHOULD restrict the reported MSI Multiple Message
> > > +Capable field to a value that might benefit system performance.
> > > +\begin{note}
> > > +For example, a device which does not expect to send interrupts at a
> > > +high rate might only specify 2 MSI vectors.
> > > +\end{note}
> > > +Device MUST support mapping any event type to any valid vector 0 to
> > > +number of MSI vectors specified in \field{Multiple Message Capable}
> field.
> > > +Device MUST support unmapping any event type.
> > > +
> > > +The device MUST return vector mapped to a given event, (NO_VECTOR
> > > +if unmapped) on read of
> \field{config_msix_vector}/\field{queue_msix_vector}.
> > > +The device MUST have all queue and configuration change events
> > > +unmapped upon reset.
> > > +
> > > +Devices SHOULD NOT cause mapping an event to vector to fail unless
> > > +it is impossible for the device to satisfy the mapping request.
> > > +Devices MUST report mapping failures by returning the NO_VECTOR
> > > +value when the relevant
> > > +\field{config_msix_vector}/\field{queue_msix_vector} field is read.
> > > +
> > > +\drivernormative{\subparagraph}{MSI Vector Configuration}{Virtio
> > > +Transport Options / Virtio Over PCI Bus / PCI-specific
> > > +Initialization And Device Operation / Device Initialization / MSI
> > > +Vector Configuration}
> > > +
> > > +Driver MUST support device with any MSI vector from 0 to 0x1F.
> > > +Driver MAY fall back on using INT\#x interrupts for a device which
> > > +only supports one MSI vector (MSI Multiple Message Capable = 0).
> > > +
A single MSI (and MSI-X) vector is still far more optimal than INTx due to the inefficiency of the INTx delivery on PCIe transport.
And for sw based devices, it anyway doesnt matter a lot either.

So when a new functionality like MSI is added, it does not need to continue what MSI-X has done.

So I request you to remove this guidance of INTx fallback on single MSI vector.

Rest all looks fine to me.

Re: [PATCH v2] transport-pci: Add MSI support

[Michael S. Tsirkin]

On Mon, Sep 30, 2024 at 05:45:48AM +0000, Parav Pandit wrote:
> 
> 
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Sent: Tuesday, September 3, 2024 11:02 AM
> > 
> > On Wed, Jul 24, 2024 at 09:51:43PM +0530, Manivannan Sadhasivam wrote:
> > > On Fri, Jul 12, 2024 at 07:31:44PM +0530, Manivannan Sadhasivam wrote:
> > > > MSI is the predecessor of MSI-X that allows PCIe devices to send
> > > > interrupts to the host. Compared to MSI-X, MSI supports only a
> > > > maximum of 32 vectors per PCIe function. But MSI has been widely
> > > > supported by the PCIe devices requiring fewer interrupts such as
> > Modems, WLAN cards etc...
> > > >
> > > > Currently, Virtio spec only documents MSI-X and INTX interrupt
> > > > mechanisms for the PCI transport. So if a Virtio device based on PCI
> > > > transport supports only MSI, then the driver on the guest will only
> > > > use INTX for receiving the interrupts. This is really sub-optimal
> > > > and affects the performance of the device. Because with MSI, the
> > > > device can use one vector per queue (max of 32 vectors) thus
> > > > avoiding the overhead associated with a shared INTX vector.
> > > >
> > > > Hence, add support for MSI to the Virtio PCI transport. MSI support
> > > > is added such a way that it reuses the existing infrastructure of
> > > > MSI-X, like the config_msix_vector/queue_msix_vector fields of the
> > > > Virtio common config structure. This makes it easy for the Virtio
> > > > drivers to add MSI support without any disruptive changes.
> > > >
> > >
> > > Gentle ping!
> > >
> > 
> > Ping again. Virtio maintainers, can you please share some feedback?
> >
> Overall looks to me.
> A small comment below on single MSI vector.
> 
>  
> > - Mani
> > 
> > > - Mani
> > >
> > > > Signed-off-by: Manivannan Sadhasivam
> > > > <manivannan.sadhasivam@linaro.org>
> > > > ---
> > > >
> > > > Changes in v2:
> > > >
> > > > * Fixed a spelling mistake in commit message
> > > > * Removed update to legacy interface
> > > > * Used 'MSI vector' consistently
> > > >
> > > >  transport-pci.tex | 115
> > > > ++++++++++++++++++++++++++++++++++++----------
> > > >  1 file changed, 92 insertions(+), 23 deletions(-)
> > > >
> > > > diff --git a/transport-pci.tex b/transport-pci.tex index
> > > > a5c6719..fd92641 100644
> > > > --- a/transport-pci.tex
> > > > +++ b/transport-pci.tex
> > > > @@ -347,7 +347,7 @@ \subsubsection{Common configuration structure
> > layout}\label{sec:Virtio Transport
> > > >          Driver Feature Bits selected by \field{driver_feature_select}.
> > > >
> > > >  \item[\field{config_msix_vector}]
> > > > -        Set by the driver to the MSI-X vector for configuration change
> > notifications.
> > > > +        Set by the driver to the MSI/MSI-X vector for configuration change
> > notifications.
> > > >
> > > >  \item[\field{num_queues}]
> > > >          The device specifies the maximum number of virtqueues supported
> > here.
> > > > @@ -371,7 +371,7 @@ \subsubsection{Common configuration structure
> > layout}\label{sec:Virtio Transport
> > > >          A 0 means the queue is unavailable.
> > > >
> > > >  \item[\field{queue_msix_vector}]
> > > > -        Set by the driver to the MSI-X vector for virtqueue notifications.
> > > > +        Set by the driver to the MSI/MSI-X vector for virtqueue
> > notifications.
> > > >
> > > >  \item[\field{queue_enable}]
> > > >          The driver uses this to selectively prevent the device from executing
> > requests from this virtqueue.
> > > > @@ -631,11 +631,11 @@ \subsubsection{ISR status
> > > > capability}\label{sec:Virtio Transport Options / Virti  in
> > > > \field{ISR status} before sending a device configuration  change
> > notification to the driver.
> > > >
> > > > -If MSI-X capability is disabled, the device MUST set the Queue
> > > > +If MSI/MSI-X capability is disabled, the device MUST set the Queue
> > > >  Interrupt bit in \field{ISR status} before sending a virtqueue
> > > > notification to the driver.
> > > >
> > > > -If MSI-X capability is disabled, the device MUST set the Interrupt
> > > > Status
> > > > +If MSI/MSI-X capability is disabled, the device MUST set the
> > > > +Interrupt Status
> > > >  bit in the PCI Status register in the PCI Configuration Header of
> > > > the device to the logical OR of all bits in \field{ISR status} of
> > > > the device.  The device then asserts/deasserts INT\#x interrupts
> > > > unless masked @@ -645,7 +645,7 @@ \subsubsection{ISR status
> > > > capability}\label{sec:Virtio Transport Options / Virti
> > > >
> > > >  \drivernormative{\paragraph}{ISR status capability}{Virtio
> > > > Transport Options / Virtio Over PCI Bus / PCI Device Layout / ISR
> > > > status capability}
> > > >
> > > > -If MSI-X capability is enabled, the driver SHOULD NOT access
> > > > +If MSI/MSI-X capability is enabled, the driver SHOULD NOT access
> > > >  \field{ISR status} upon detecting a Queue Interrupt.
> > > >
> > > >  \subsubsection{Device-specific configuration}\label{sec:Virtio
> > > > Transport Options / Virtio Over PCI Bus / PCI Device Layout /
> > > > Device-specific configuration} @@ -1017,7 +1017,7 @@
> > > > \subsubsection{Device Initialization}\label{sec:Virtio Transport
> > > > Options / Virti  \drivernormative{\subparagraph}{MSI-X Vector
> > > > Configuration}{Virtio Transport Options / Virtio Over PCI Bus /
> > > > PCI-specific Initialization And Device Operation / Device
> > > > Initialization / MSI-X Vector Configuration}
> > > >
> > > >  Driver MUST support device with any MSI-X Table Size 0 to 0x7FF.
> > > > -Driver MAY fall back on using INT\#x interrupts for a device
> > > > +Driver MAY fall back on using MSI or INT\#x interrupts for a device
> > > >  which only supports one MSI-X vector (MSI-X Table Size = 0).
> > > >
> > > >  Driver MAY interpret the Table Size as a hint from the device @@
> > > > -1034,6 +1034,75 @@ \subsubsection{Device
> > > > Initialization}\label{sec:Virtio Transport Options / Virti  the
> > > > driver MAY retry mapping with fewer vectors, disable MSI-X  or report
> > device failure.
> > > >
> > > > +\paragraph{MSI Vector Configuration}\label{sec:Virtio Transport
> > > > +Options / Virtio Over PCI Bus / PCI-specific Initialization And
> > > > +Device Operation / Device Initialization / MSI Vector
> > > > +Configuration}
> > > > +
> > > > +When MSI capability is present and enabled in the device (through
> > > > +standard PCI configuration space) \field{config_msix_vector} and
> > > > +\field{queue_msix_vector} are used to map configuration change and
> > queue interrupts to MSI vectors. In this case, the ISR Status is unused.
> > > > +
> > > > +Writing a valid MSI vector, 0 to 0x1F, to
> > > > +\field{config_msix_vector}/\field{queue_msix_vector} maps
> > > > +interrupts triggered by the configuration change/selected queue
> > > > +events respectively to the corresponding MSI vector. To disable
> > > > +interrupts for an event type, the driver unmaps this event by
> > > > +writing a special NO_VECTOR
> > > > +value:
> > > > +
> > > > +\begin{lstlisting}
> > > > +/* Vector value used to disable MSI for queue */
> > > > +#define VIRTIO_MSI_NO_VECTOR            0xffff
> > > > +\end{lstlisting}
> > > > +
> > > > +Note that mapping an event to vector might require device to
> > > > +allocate internal device resources, and thus could fail.
> > > > +
> > > > +\devicenormative{\subparagraph}{MSI Vector Configuration}{Virtio
> > > > +Transport Options / Virtio Over PCI Bus / PCI-specific
> > > > +Initialization And Device Operation / Device Initialization / MSI
> > > > +Vector Configuration}
> > > > +
> > > > +A device that has an MSI capability SHOULD support at least 2 and
> > > > +at most 0x20 MSI vectors.
> > > > +Device MUST report the number of vectors supported in
> > > > +\field{Multiple Message Capable} field in the MSI Capability as
> > > > +specified in \hyperref[intro:PCI]{[PCI]}.
> > > > +The device SHOULD restrict the reported MSI Multiple Message
> > > > +Capable field to a value that might benefit system performance.
> > > > +\begin{note}
> > > > +For example, a device which does not expect to send interrupts at a
> > > > +high rate might only specify 2 MSI vectors.
> > > > +\end{note}
> > > > +Device MUST support mapping any event type to any valid vector 0 to
> > > > +number of MSI vectors specified in \field{Multiple Message Capable}
> > field.
> > > > +Device MUST support unmapping any event type.
> > > > +
> > > > +The device MUST return vector mapped to a given event, (NO_VECTOR
> > > > +if unmapped) on read of
> > \field{config_msix_vector}/\field{queue_msix_vector}.
> > > > +The device MUST have all queue and configuration change events
> > > > +unmapped upon reset.
> > > > +
> > > > +Devices SHOULD NOT cause mapping an event to vector to fail unless
> > > > +it is impossible for the device to satisfy the mapping request.
> > > > +Devices MUST report mapping failures by returning the NO_VECTOR
> > > > +value when the relevant
> > > > +\field{config_msix_vector}/\field{queue_msix_vector} field is read.
> > > > +
> > > > +\drivernormative{\subparagraph}{MSI Vector Configuration}{Virtio
> > > > +Transport Options / Virtio Over PCI Bus / PCI-specific
> > > > +Initialization And Device Operation / Device Initialization / MSI
> > > > +Vector Configuration}
> > > > +
> > > > +Driver MUST support device with any MSI vector from 0 to 0x1F.
> > > > +Driver MAY fall back on using INT\#x interrupts for a device which
> > > > +only supports one MSI vector (MSI Multiple Message Capable = 0).
> > > > +
> A single MSI (and MSI-X) vector is still far more optimal than INTx due to the inefficiency of the INTx delivery on PCIe transport.

And lack of support on VFs.

> And for sw based devices, it anyway doesnt matter a lot either.
> 
> So when a new functionality like MSI is added, it does not need to continue what MSI-X has done.
> 
> So I request you to remove this guidance of INTx fallback on single MSI vector.

Let's provide an alternative guidance then.
Device SHOULD implement at least 2 MSI vectors?

> Rest all looks fine to me.

Re: [PATCH v2] transport-pci: Add MSI support

[Zhu Lingshan]

On 9/30/2024 2:30 PM, Michael S. Tsirkin wrote:
> On Mon, Sep 30, 2024 at 05:45:48AM +0000, Parav Pandit wrote:
>>
>>> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>> Sent: Tuesday, September 3, 2024 11:02 AM
>>>
>>> On Wed, Jul 24, 2024 at 09:51:43PM +0530, Manivannan Sadhasivam wrote:
>>>> On Fri, Jul 12, 2024 at 07:31:44PM +0530, Manivannan Sadhasivam wrote:
>>>>> MSI is the predecessor of MSI-X that allows PCIe devices to send
>>>>> interrupts to the host. Compared to MSI-X, MSI supports only a
>>>>> maximum of 32 vectors per PCIe function. But MSI has been widely
>>>>> supported by the PCIe devices requiring fewer interrupts such as
>>> Modems, WLAN cards etc...
>>>>> Currently, Virtio spec only documents MSI-X and INTX interrupt
>>>>> mechanisms for the PCI transport. So if a Virtio device based on PCI
>>>>> transport supports only MSI, then the driver on the guest will only
>>>>> use INTX for receiving the interrupts. This is really sub-optimal
>>>>> and affects the performance of the device. Because with MSI, the
>>>>> device can use one vector per queue (max of 32 vectors) thus
>>>>> avoiding the overhead associated with a shared INTX vector.
>>>>>
>>>>> Hence, add support for MSI to the Virtio PCI transport. MSI support
>>>>> is added such a way that it reuses the existing infrastructure of
>>>>> MSI-X, like the config_msix_vector/queue_msix_vector fields of the
>>>>> Virtio common config structure. This makes it easy for the Virtio
>>>>> drivers to add MSI support without any disruptive changes.
>>>>>
>>>> Gentle ping!
>>>>
>>> Ping again. Virtio maintainers, can you please share some feedback?
>>>
>> Overall looks to me.
>> A small comment below on single MSI vector.
>>
>>  
>>> - Mani
>>>
>>>> - Mani
>>>>
>>>>> Signed-off-by: Manivannan Sadhasivam
>>>>> <manivannan.sadhasivam@linaro.org>
>>>>> ---
>>>>>
>>>>> Changes in v2:
>>>>>
>>>>> * Fixed a spelling mistake in commit message
>>>>> * Removed update to legacy interface
>>>>> * Used 'MSI vector' consistently
>>>>>
>>>>>  transport-pci.tex | 115
>>>>> ++++++++++++++++++++++++++++++++++++----------
>>>>>  1 file changed, 92 insertions(+), 23 deletions(-)
>>>>>
>>>>> diff --git a/transport-pci.tex b/transport-pci.tex index
>>>>> a5c6719..fd92641 100644
>>>>> --- a/transport-pci.tex
>>>>> +++ b/transport-pci.tex
>>>>> @@ -347,7 +347,7 @@ \subsubsection{Common configuration structure
>>> layout}\label{sec:Virtio Transport
>>>>>          Driver Feature Bits selected by \field{driver_feature_select}.
>>>>>
>>>>>  \item[\field{config_msix_vector}]
>>>>> -        Set by the driver to the MSI-X vector for configuration change
>>> notifications.
>>>>> +        Set by the driver to the MSI/MSI-X vector for configuration change
>>> notifications.
>>>>>  \item[\field{num_queues}]
>>>>>          The device specifies the maximum number of virtqueues supported
>>> here.
>>>>> @@ -371,7 +371,7 @@ \subsubsection{Common configuration structure
>>> layout}\label{sec:Virtio Transport
>>>>>          A 0 means the queue is unavailable.
>>>>>
>>>>>  \item[\field{queue_msix_vector}]
>>>>> -        Set by the driver to the MSI-X vector for virtqueue notifications.
>>>>> +        Set by the driver to the MSI/MSI-X vector for virtqueue
>>> notifications.
>>>>>  \item[\field{queue_enable}]
>>>>>          The driver uses this to selectively prevent the device from executing
>>> requests from this virtqueue.
>>>>> @@ -631,11 +631,11 @@ \subsubsection{ISR status
>>>>> capability}\label{sec:Virtio Transport Options / Virti  in
>>>>> \field{ISR status} before sending a device configuration  change
>>> notification to the driver.
>>>>> -If MSI-X capability is disabled, the device MUST set the Queue
>>>>> +If MSI/MSI-X capability is disabled, the device MUST set the Queue
>>>>>  Interrupt bit in \field{ISR status} before sending a virtqueue
>>>>> notification to the driver.
>>>>>
>>>>> -If MSI-X capability is disabled, the device MUST set the Interrupt
>>>>> Status
>>>>> +If MSI/MSI-X capability is disabled, the device MUST set the
>>>>> +Interrupt Status
>>>>>  bit in the PCI Status register in the PCI Configuration Header of
>>>>> the device to the logical OR of all bits in \field{ISR status} of
>>>>> the device.  The device then asserts/deasserts INT\#x interrupts
>>>>> unless masked @@ -645,7 +645,7 @@ \subsubsection{ISR status
>>>>> capability}\label{sec:Virtio Transport Options / Virti
>>>>>
>>>>>  \drivernormative{\paragraph}{ISR status capability}{Virtio
>>>>> Transport Options / Virtio Over PCI Bus / PCI Device Layout / ISR
>>>>> status capability}
>>>>>
>>>>> -If MSI-X capability is enabled, the driver SHOULD NOT access
>>>>> +If MSI/MSI-X capability is enabled, the driver SHOULD NOT access
>>>>>  \field{ISR status} upon detecting a Queue Interrupt.
>>>>>
>>>>>  \subsubsection{Device-specific configuration}\label{sec:Virtio
>>>>> Transport Options / Virtio Over PCI Bus / PCI Device Layout /
>>>>> Device-specific configuration} @@ -1017,7 +1017,7 @@
>>>>> \subsubsection{Device Initialization}\label{sec:Virtio Transport
>>>>> Options / Virti  \drivernormative{\subparagraph}{MSI-X Vector
>>>>> Configuration}{Virtio Transport Options / Virtio Over PCI Bus /
>>>>> PCI-specific Initialization And Device Operation / Device
>>>>> Initialization / MSI-X Vector Configuration}
>>>>>
>>>>>  Driver MUST support device with any MSI-X Table Size 0 to 0x7FF.
>>>>> -Driver MAY fall back on using INT\#x interrupts for a device
>>>>> +Driver MAY fall back on using MSI or INT\#x interrupts for a device
>>>>>  which only supports one MSI-X vector (MSI-X Table Size = 0).
>>>>>
>>>>>  Driver MAY interpret the Table Size as a hint from the device @@
>>>>> -1034,6 +1034,75 @@ \subsubsection{Device
>>>>> Initialization}\label{sec:Virtio Transport Options / Virti  the
>>>>> driver MAY retry mapping with fewer vectors, disable MSI-X  or report
>>> device failure.
>>>>> +\paragraph{MSI Vector Configuration}\label{sec:Virtio Transport
>>>>> +Options / Virtio Over PCI Bus / PCI-specific Initialization And
>>>>> +Device Operation / Device Initialization / MSI Vector
>>>>> +Configuration}
>>>>> +
>>>>> +When MSI capability is present and enabled in the device (through
>>>>> +standard PCI configuration space) \field{config_msix_vector} and
>>>>> +\field{queue_msix_vector} are used to map configuration change and
>>> queue interrupts to MSI vectors. In this case, the ISR Status is unused.
>>>>> +
>>>>> +Writing a valid MSI vector, 0 to 0x1F, to
>>>>> +\field{config_msix_vector}/\field{queue_msix_vector} maps
>>>>> +interrupts triggered by the configuration change/selected queue
>>>>> +events respectively to the corresponding MSI vector. To disable
>>>>> +interrupts for an event type, the driver unmaps this event by
>>>>> +writing a special NO_VECTOR
>>>>> +value:
>>>>> +
>>>>> +\begin{lstlisting}
>>>>> +/* Vector value used to disable MSI for queue */
>>>>> +#define VIRTIO_MSI_NO_VECTOR            0xffff
>>>>> +\end{lstlisting}
>>>>> +
>>>>> +Note that mapping an event to vector might require device to
>>>>> +allocate internal device resources, and thus could fail.
>>>>> +
>>>>> +\devicenormative{\subparagraph}{MSI Vector Configuration}{Virtio
>>>>> +Transport Options / Virtio Over PCI Bus / PCI-specific
>>>>> +Initialization And Device Operation / Device Initialization / MSI
>>>>> +Vector Configuration}
>>>>> +
>>>>> +A device that has an MSI capability SHOULD support at least 2 and
>>>>> +at most 0x20 MSI vectors.
>>>>> +Device MUST report the number of vectors supported in
>>>>> +\field{Multiple Message Capable} field in the MSI Capability as
>>>>> +specified in \hyperref[intro:PCI]{[PCI]}.
>>>>> +The device SHOULD restrict the reported MSI Multiple Message
>>>>> +Capable field to a value that might benefit system performance.
>>>>> +\begin{note}
>>>>> +For example, a device which does not expect to send interrupts at a
>>>>> +high rate might only specify 2 MSI vectors.
>>>>> +\end{note}
>>>>> +Device MUST support mapping any event type to any valid vector 0 to
>>>>> +number of MSI vectors specified in \field{Multiple Message Capable}
>>> field.
>>>>> +Device MUST support unmapping any event type.
>>>>> +
>>>>> +The device MUST return vector mapped to a given event, (NO_VECTOR
>>>>> +if unmapped) on read of
>>> \field{config_msix_vector}/\field{queue_msix_vector}.
>>>>> +The device MUST have all queue and configuration change events
>>>>> +unmapped upon reset.
>>>>> +
>>>>> +Devices SHOULD NOT cause mapping an event to vector to fail unless
>>>>> +it is impossible for the device to satisfy the mapping request.
>>>>> +Devices MUST report mapping failures by returning the NO_VECTOR
>>>>> +value when the relevant
>>>>> +\field{config_msix_vector}/\field{queue_msix_vector} field is read.
>>>>> +
>>>>> +\drivernormative{\subparagraph}{MSI Vector Configuration}{Virtio
>>>>> +Transport Options / Virtio Over PCI Bus / PCI-specific
>>>>> +Initialization And Device Operation / Device Initialization / MSI
>>>>> +Vector Configuration}
>>>>> +
>>>>> +Driver MUST support device with any MSI vector from 0 to 0x1F.
>>>>> +Driver MAY fall back on using INT\#x interrupts for a device which
>>>>> +only supports one MSI vector (MSI Multiple Message Capable = 0).
>>>>> +
>> A single MSI (and MSI-X) vector is still far more optimal than INTx due to the inefficiency of the INTx delivery on PCIe transport.
> And lack of support on VFs.
>
>> And for sw based devices, it anyway doesnt matter a lot either.
>>
>> So when a new functionality like MSI is added, it does not need to continue what MSI-X has done.
>>
>> So I request you to remove this guidance of INTx fallback on single MSI vector.
> Let's provide an alternative guidance then.
> Device SHOULD implement at least 2 MSI vectors?
I think one MSI vector is enough for functionalities, just shared by all interrupts.

Thanks
Zhu Lingshan
>
>> Rest all looks fine to me.
>

RE: [PATCH v2] transport-pci: Add MSI support

[Parav Pandit]

> From: Zhu Lingshan <lingshan.zhu@amd.com>
> Sent: Monday, September 30, 2024 12:13 PM
> 
> On 9/30/2024 2:30 PM, Michael S. Tsirkin wrote:
> > On Mon, Sep 30, 2024 at 05:45:48AM +0000, Parav Pandit wrote:
> >>
> >>> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> >>> Sent: Tuesday, September 3, 2024 11:02 AM
> >>>
> >>> On Wed, Jul 24, 2024 at 09:51:43PM +0530, Manivannan Sadhasivam
> wrote:
> >>>> On Fri, Jul 12, 2024 at 07:31:44PM +0530, Manivannan Sadhasivam
> wrote:
> >>>>> MSI is the predecessor of MSI-X that allows PCIe devices to send
> >>>>> interrupts to the host. Compared to MSI-X, MSI supports only a
> >>>>> maximum of 32 vectors per PCIe function. But MSI has been widely
> >>>>> supported by the PCIe devices requiring fewer interrupts such as
> >>> Modems, WLAN cards etc...
> >>>>> Currently, Virtio spec only documents MSI-X and INTX interrupt
> >>>>> mechanisms for the PCI transport. So if a Virtio device based on
> >>>>> PCI transport supports only MSI, then the driver on the guest will
> >>>>> only use INTX for receiving the interrupts. This is really
> >>>>> sub-optimal and affects the performance of the device. Because
> >>>>> with MSI, the device can use one vector per queue (max of 32
> >>>>> vectors) thus avoiding the overhead associated with a shared INTX
> vector.
> >>>>>
> >>>>> Hence, add support for MSI to the Virtio PCI transport. MSI
> >>>>> support is added such a way that it reuses the existing
> >>>>> infrastructure of MSI-X, like the
> >>>>> config_msix_vector/queue_msix_vector fields of the Virtio common
> >>>>> config structure. This makes it easy for the Virtio drivers to add MSI
> support without any disruptive changes.
> >>>>>
> >>>> Gentle ping!
> >>>>
> >>> Ping again. Virtio maintainers, can you please share some feedback?
> >>>
> >> Overall looks to me.
> >> A small comment below on single MSI vector.
> >>
> >>
> >>> - Mani
> >>>
> >>>> - Mani
> >>>>
> >>>>> Signed-off-by: Manivannan Sadhasivam
> >>>>> <manivannan.sadhasivam@linaro.org>
> >>>>> ---
> >>>>>
> >>>>> Changes in v2:
> >>>>>
> >>>>> * Fixed a spelling mistake in commit message
> >>>>> * Removed update to legacy interface
> >>>>> * Used 'MSI vector' consistently
> >>>>>
> >>>>>  transport-pci.tex | 115
> >>>>> ++++++++++++++++++++++++++++++++++++----------
> >>>>>  1 file changed, 92 insertions(+), 23 deletions(-)
> >>>>>
> >>>>> diff --git a/transport-pci.tex b/transport-pci.tex index
> >>>>> a5c6719..fd92641 100644
> >>>>> --- a/transport-pci.tex
> >>>>> +++ b/transport-pci.tex
> >>>>> @@ -347,7 +347,7 @@ \subsubsection{Common configuration
> structure
> >>> layout}\label{sec:Virtio Transport
> >>>>>          Driver Feature Bits selected by \field{driver_feature_select}.
> >>>>>
> >>>>>  \item[\field{config_msix_vector}]
> >>>>> -        Set by the driver to the MSI-X vector for configuration change
> >>> notifications.
> >>>>> +        Set by the driver to the MSI/MSI-X vector for
> >>>>> + configuration change
> >>> notifications.
> >>>>>  \item[\field{num_queues}]
> >>>>>          The device specifies the maximum number of virtqueues
> >>>>> supported
> >>> here.
> >>>>> @@ -371,7 +371,7 @@ \subsubsection{Common configuration
> structure
> >>> layout}\label{sec:Virtio Transport
> >>>>>          A 0 means the queue is unavailable.
> >>>>>
> >>>>>  \item[\field{queue_msix_vector}]
> >>>>> -        Set by the driver to the MSI-X vector for virtqueue notifications.
> >>>>> +        Set by the driver to the MSI/MSI-X vector for virtqueue
> >>> notifications.
> >>>>>  \item[\field{queue_enable}]
> >>>>>          The driver uses this to selectively prevent the device
> >>>>> from executing
> >>> requests from this virtqueue.
> >>>>> @@ -631,11 +631,11 @@ \subsubsection{ISR status
> >>>>> capability}\label{sec:Virtio Transport Options / Virti  in
> >>>>> \field{ISR status} before sending a device configuration  change
> >>> notification to the driver.
> >>>>> -If MSI-X capability is disabled, the device MUST set the Queue
> >>>>> +If MSI/MSI-X capability is disabled, the device MUST set the
> >>>>> +Queue
> >>>>>  Interrupt bit in \field{ISR status} before sending a virtqueue
> >>>>> notification to the driver.
> >>>>>
> >>>>> -If MSI-X capability is disabled, the device MUST set the
> >>>>> Interrupt Status
> >>>>> +If MSI/MSI-X capability is disabled, the device MUST set the
> >>>>> +Interrupt Status
> >>>>>  bit in the PCI Status register in the PCI Configuration Header of
> >>>>> the device to the logical OR of all bits in \field{ISR status} of
> >>>>> the device.  The device then asserts/deasserts INT\#x interrupts
> >>>>> unless masked @@ -645,7 +645,7 @@ \subsubsection{ISR status
> >>>>> capability}\label{sec:Virtio Transport Options / Virti
> >>>>>
> >>>>>  \drivernormative{\paragraph}{ISR status capability}{Virtio
> >>>>> Transport Options / Virtio Over PCI Bus / PCI Device Layout / ISR
> >>>>> status capability}
> >>>>>
> >>>>> -If MSI-X capability is enabled, the driver SHOULD NOT access
> >>>>> +If MSI/MSI-X capability is enabled, the driver SHOULD NOT access
> >>>>>  \field{ISR status} upon detecting a Queue Interrupt.
> >>>>>
> >>>>>  \subsubsection{Device-specific configuration}\label{sec:Virtio
> >>>>> Transport Options / Virtio Over PCI Bus / PCI Device Layout /
> >>>>> Device-specific configuration} @@ -1017,7 +1017,7 @@
> >>>>> \subsubsection{Device Initialization}\label{sec:Virtio Transport
> >>>>> Options / Virti  \drivernormative{\subparagraph}{MSI-X Vector
> >>>>> Configuration}{Virtio Transport Options / Virtio Over PCI Bus /
> >>>>> PCI-specific Initialization And Device Operation / Device
> >>>>> Initialization / MSI-X Vector Configuration}
> >>>>>
> >>>>>  Driver MUST support device with any MSI-X Table Size 0 to 0x7FF.
> >>>>> -Driver MAY fall back on using INT\#x interrupts for a device
> >>>>> +Driver MAY fall back on using MSI or INT\#x interrupts for a
> >>>>> +device
> >>>>>  which only supports one MSI-X vector (MSI-X Table Size = 0).
> >>>>>
> >>>>>  Driver MAY interpret the Table Size as a hint from the device @@
> >>>>> -1034,6 +1034,75 @@ \subsubsection{Device
> >>>>> Initialization}\label{sec:Virtio Transport Options / Virti  the
> >>>>> driver MAY retry mapping with fewer vectors, disable MSI-X  or
> >>>>> report
> >>> device failure.
> >>>>> +\paragraph{MSI Vector Configuration}\label{sec:Virtio Transport
> >>>>> +Options / Virtio Over PCI Bus / PCI-specific Initialization And
> >>>>> +Device Operation / Device Initialization / MSI Vector
> >>>>> +Configuration}
> >>>>> +
> >>>>> +When MSI capability is present and enabled in the device (through
> >>>>> +standard PCI configuration space) \field{config_msix_vector} and
> >>>>> +\field{queue_msix_vector} are used to map configuration change
> >>>>> +and
> >>> queue interrupts to MSI vectors. In this case, the ISR Status is unused.
> >>>>> +
> >>>>> +Writing a valid MSI vector, 0 to 0x1F, to
> >>>>> +\field{config_msix_vector}/\field{queue_msix_vector} maps
> >>>>> +interrupts triggered by the configuration change/selected queue
> >>>>> +events respectively to the corresponding MSI vector. To disable
> >>>>> +interrupts for an event type, the driver unmaps this event by
> >>>>> +writing a special NO_VECTOR
> >>>>> +value:
> >>>>> +
> >>>>> +\begin{lstlisting}
> >>>>> +/* Vector value used to disable MSI for queue */
> >>>>> +#define VIRTIO_MSI_NO_VECTOR            0xffff
> >>>>> +\end{lstlisting}
> >>>>> +
> >>>>> +Note that mapping an event to vector might require device to
> >>>>> +allocate internal device resources, and thus could fail.
> >>>>> +
> >>>>> +\devicenormative{\subparagraph}{MSI Vector Configuration}{Virtio
> >>>>> +Transport Options / Virtio Over PCI Bus / PCI-specific
> >>>>> +Initialization And Device Operation / Device Initialization / MSI
> >>>>> +Vector Configuration}
> >>>>> +
> >>>>> +A device that has an MSI capability SHOULD support at least 2 and
> >>>>> +at most 0x20 MSI vectors.
> >>>>> +Device MUST report the number of vectors supported in
> >>>>> +\field{Multiple Message Capable} field in the MSI Capability as
> >>>>> +specified in \hyperref[intro:PCI]{[PCI]}.
> >>>>> +The device SHOULD restrict the reported MSI Multiple Message
> >>>>> +Capable field to a value that might benefit system performance.
> >>>>> +\begin{note}
> >>>>> +For example, a device which does not expect to send interrupts at
> >>>>> +a high rate might only specify 2 MSI vectors.
> >>>>> +\end{note}
> >>>>> +Device MUST support mapping any event type to any valid vector 0
> >>>>> +to number of MSI vectors specified in \field{Multiple Message
> >>>>> +Capable}
> >>> field.
> >>>>> +Device MUST support unmapping any event type.
> >>>>> +
> >>>>> +The device MUST return vector mapped to a given event, (NO_VECTOR
> >>>>> +if unmapped) on read of
> >>> \field{config_msix_vector}/\field{queue_msix_vector}.
> >>>>> +The device MUST have all queue and configuration change events
> >>>>> +unmapped upon reset.
> >>>>> +
> >>>>> +Devices SHOULD NOT cause mapping an event to vector to fail
> >>>>> +unless it is impossible for the device to satisfy the mapping request.
> >>>>> +Devices MUST report mapping failures by returning the NO_VECTOR
> >>>>> +value when the relevant
> >>>>> +\field{config_msix_vector}/\field{queue_msix_vector} field is read.
> >>>>> +
> >>>>> +\drivernormative{\subparagraph}{MSI Vector Configuration}{Virtio
> >>>>> +Transport Options / Virtio Over PCI Bus / PCI-specific
> >>>>> +Initialization And Device Operation / Device Initialization / MSI
> >>>>> +Vector Configuration}
> >>>>> +
> >>>>> +Driver MUST support device with any MSI vector from 0 to 0x1F.
> >>>>> +Driver MAY fall back on using INT\#x interrupts for a device
> >>>>> +which only supports one MSI vector (MSI Multiple Message Capable =
> 0).
> >>>>> +
> >> A single MSI (and MSI-X) vector is still far more optimal than INTx due to
> the inefficiency of the INTx delivery on PCIe transport.
> > And lack of support on VFs.
> >
> >> And for sw based devices, it anyway doesnt matter a lot either.
> >>
> >> So when a new functionality like MSI is added, it does not need to continue
> what MSI-X has done.
> >>
> >> So I request you to remove this guidance of INTx fallback on single MSI
> vector.
> > Let's provide an alternative guidance then.
> > Device SHOULD implement at least 2 MSI vectors?
> I think one MSI vector is enough for functionalities, just shared by all
> interrupts.
> 
Functionally MSI vector can work sub-optimally, because now on every VQ notification, the driver will inspect configuration space to discover changes, generating more unwanted PCI traffic.
So, 2 seems a more practical tradeoff across functionality / performance / scale.
And single MSI vector instead of INTx is of course a good recommendation by itself to include as independent line.

> Thanks
> Zhu Lingshan
> >
> >> Rest all looks fine to me.
> >

Re: [PATCH v2] transport-pci: Add MSI support

[Zhu Lingshan]

On 9/30/2024 3:01 PM, Parav Pandit wrote:
>> From: Zhu Lingshan <lingshan.zhu@amd.com>
>> Sent: Monday, September 30, 2024 12:13 PM
>>
>> On 9/30/2024 2:30 PM, Michael S. Tsirkin wrote:
>>> On Mon, Sep 30, 2024 at 05:45:48AM +0000, Parav Pandit wrote:
>>>>> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>>>> Sent: Tuesday, September 3, 2024 11:02 AM
>>>>>
>>>>> On Wed, Jul 24, 2024 at 09:51:43PM +0530, Manivannan Sadhasivam
>> wrote:
>>>>>> On Fri, Jul 12, 2024 at 07:31:44PM +0530, Manivannan Sadhasivam
>> wrote:
>>>>>>> MSI is the predecessor of MSI-X that allows PCIe devices to send
>>>>>>> interrupts to the host. Compared to MSI-X, MSI supports only a
>>>>>>> maximum of 32 vectors per PCIe function. But MSI has been widely
>>>>>>> supported by the PCIe devices requiring fewer interrupts such as
>>>>> Modems, WLAN cards etc...
>>>>>>> Currently, Virtio spec only documents MSI-X and INTX interrupt
>>>>>>> mechanisms for the PCI transport. So if a Virtio device based on
>>>>>>> PCI transport supports only MSI, then the driver on the guest will
>>>>>>> only use INTX for receiving the interrupts. This is really
>>>>>>> sub-optimal and affects the performance of the device. Because
>>>>>>> with MSI, the device can use one vector per queue (max of 32
>>>>>>> vectors) thus avoiding the overhead associated with a shared INTX
>> vector.
>>>>>>> Hence, add support for MSI to the Virtio PCI transport. MSI
>>>>>>> support is added such a way that it reuses the existing
>>>>>>> infrastructure of MSI-X, like the
>>>>>>> config_msix_vector/queue_msix_vector fields of the Virtio common
>>>>>>> config structure. This makes it easy for the Virtio drivers to add MSI
>> support without any disruptive changes.
>>>>>> Gentle ping!
>>>>>>
>>>>> Ping again. Virtio maintainers, can you please share some feedback?
>>>>>
>>>> Overall looks to me.
>>>> A small comment below on single MSI vector.
>>>>
>>>>
>>>>> - Mani
>>>>>
>>>>>> - Mani
>>>>>>
>>>>>>> Signed-off-by: Manivannan Sadhasivam
>>>>>>> <manivannan.sadhasivam@linaro.org>
>>>>>>> ---
>>>>>>>
>>>>>>> Changes in v2:
>>>>>>>
>>>>>>> * Fixed a spelling mistake in commit message
>>>>>>> * Removed update to legacy interface
>>>>>>> * Used 'MSI vector' consistently
>>>>>>>
>>>>>>>  transport-pci.tex | 115
>>>>>>> ++++++++++++++++++++++++++++++++++++----------
>>>>>>>  1 file changed, 92 insertions(+), 23 deletions(-)
>>>>>>>
>>>>>>> diff --git a/transport-pci.tex b/transport-pci.tex index
>>>>>>> a5c6719..fd92641 100644
>>>>>>> --- a/transport-pci.tex
>>>>>>> +++ b/transport-pci.tex
>>>>>>> @@ -347,7 +347,7 @@ \subsubsection{Common configuration
>> structure
>>>>> layout}\label{sec:Virtio Transport
>>>>>>>          Driver Feature Bits selected by \field{driver_feature_select}.
>>>>>>>
>>>>>>>  \item[\field{config_msix_vector}]
>>>>>>> -        Set by the driver to the MSI-X vector for configuration change
>>>>> notifications.
>>>>>>> +        Set by the driver to the MSI/MSI-X vector for
>>>>>>> + configuration change
>>>>> notifications.
>>>>>>>  \item[\field{num_queues}]
>>>>>>>          The device specifies the maximum number of virtqueues
>>>>>>> supported
>>>>> here.
>>>>>>> @@ -371,7 +371,7 @@ \subsubsection{Common configuration
>> structure
>>>>> layout}\label{sec:Virtio Transport
>>>>>>>          A 0 means the queue is unavailable.
>>>>>>>
>>>>>>>  \item[\field{queue_msix_vector}]
>>>>>>> -        Set by the driver to the MSI-X vector for virtqueue notifications.
>>>>>>> +        Set by the driver to the MSI/MSI-X vector for virtqueue
>>>>> notifications.
>>>>>>>  \item[\field{queue_enable}]
>>>>>>>          The driver uses this to selectively prevent the device
>>>>>>> from executing
>>>>> requests from this virtqueue.
>>>>>>> @@ -631,11 +631,11 @@ \subsubsection{ISR status
>>>>>>> capability}\label{sec:Virtio Transport Options / Virti  in
>>>>>>> \field{ISR status} before sending a device configuration  change
>>>>> notification to the driver.
>>>>>>> -If MSI-X capability is disabled, the device MUST set the Queue
>>>>>>> +If MSI/MSI-X capability is disabled, the device MUST set the
>>>>>>> +Queue
>>>>>>>  Interrupt bit in \field{ISR status} before sending a virtqueue
>>>>>>> notification to the driver.
>>>>>>>
>>>>>>> -If MSI-X capability is disabled, the device MUST set the
>>>>>>> Interrupt Status
>>>>>>> +If MSI/MSI-X capability is disabled, the device MUST set the
>>>>>>> +Interrupt Status
>>>>>>>  bit in the PCI Status register in the PCI Configuration Header of
>>>>>>> the device to the logical OR of all bits in \field{ISR status} of
>>>>>>> the device.  The device then asserts/deasserts INT\#x interrupts
>>>>>>> unless masked @@ -645,7 +645,7 @@ \subsubsection{ISR status
>>>>>>> capability}\label{sec:Virtio Transport Options / Virti
>>>>>>>
>>>>>>>  \drivernormative{\paragraph}{ISR status capability}{Virtio
>>>>>>> Transport Options / Virtio Over PCI Bus / PCI Device Layout / ISR
>>>>>>> status capability}
>>>>>>>
>>>>>>> -If MSI-X capability is enabled, the driver SHOULD NOT access
>>>>>>> +If MSI/MSI-X capability is enabled, the driver SHOULD NOT access
>>>>>>>  \field{ISR status} upon detecting a Queue Interrupt.
>>>>>>>
>>>>>>>  \subsubsection{Device-specific configuration}\label{sec:Virtio
>>>>>>> Transport Options / Virtio Over PCI Bus / PCI Device Layout /
>>>>>>> Device-specific configuration} @@ -1017,7 +1017,7 @@
>>>>>>> \subsubsection{Device Initialization}\label{sec:Virtio Transport
>>>>>>> Options / Virti  \drivernormative{\subparagraph}{MSI-X Vector
>>>>>>> Configuration}{Virtio Transport Options / Virtio Over PCI Bus /
>>>>>>> PCI-specific Initialization And Device Operation / Device
>>>>>>> Initialization / MSI-X Vector Configuration}
>>>>>>>
>>>>>>>  Driver MUST support device with any MSI-X Table Size 0 to 0x7FF.
>>>>>>> -Driver MAY fall back on using INT\#x interrupts for a device
>>>>>>> +Driver MAY fall back on using MSI or INT\#x interrupts for a
>>>>>>> +device
>>>>>>>  which only supports one MSI-X vector (MSI-X Table Size = 0).
>>>>>>>
>>>>>>>  Driver MAY interpret the Table Size as a hint from the device @@
>>>>>>> -1034,6 +1034,75 @@ \subsubsection{Device
>>>>>>> Initialization}\label{sec:Virtio Transport Options / Virti  the
>>>>>>> driver MAY retry mapping with fewer vectors, disable MSI-X  or
>>>>>>> report
>>>>> device failure.
>>>>>>> +\paragraph{MSI Vector Configuration}\label{sec:Virtio Transport
>>>>>>> +Options / Virtio Over PCI Bus / PCI-specific Initialization And
>>>>>>> +Device Operation / Device Initialization / MSI Vector
>>>>>>> +Configuration}
>>>>>>> +
>>>>>>> +When MSI capability is present and enabled in the device (through
>>>>>>> +standard PCI configuration space) \field{config_msix_vector} and
>>>>>>> +\field{queue_msix_vector} are used to map configuration change
>>>>>>> +and
>>>>> queue interrupts to MSI vectors. In this case, the ISR Status is unused.
>>>>>>> +
>>>>>>> +Writing a valid MSI vector, 0 to 0x1F, to
>>>>>>> +\field{config_msix_vector}/\field{queue_msix_vector} maps
>>>>>>> +interrupts triggered by the configuration change/selected queue
>>>>>>> +events respectively to the corresponding MSI vector. To disable
>>>>>>> +interrupts for an event type, the driver unmaps this event by
>>>>>>> +writing a special NO_VECTOR
>>>>>>> +value:
>>>>>>> +
>>>>>>> +\begin{lstlisting}
>>>>>>> +/* Vector value used to disable MSI for queue */
>>>>>>> +#define VIRTIO_MSI_NO_VECTOR            0xffff
>>>>>>> +\end{lstlisting}
>>>>>>> +
>>>>>>> +Note that mapping an event to vector might require device to
>>>>>>> +allocate internal device resources, and thus could fail.
>>>>>>> +
>>>>>>> +\devicenormative{\subparagraph}{MSI Vector Configuration}{Virtio
>>>>>>> +Transport Options / Virtio Over PCI Bus / PCI-specific
>>>>>>> +Initialization And Device Operation / Device Initialization / MSI
>>>>>>> +Vector Configuration}
>>>>>>> +
>>>>>>> +A device that has an MSI capability SHOULD support at least 2 and
>>>>>>> +at most 0x20 MSI vectors.
>>>>>>> +Device MUST report the number of vectors supported in
>>>>>>> +\field{Multiple Message Capable} field in the MSI Capability as
>>>>>>> +specified in \hyperref[intro:PCI]{[PCI]}.
>>>>>>> +The device SHOULD restrict the reported MSI Multiple Message
>>>>>>> +Capable field to a value that might benefit system performance.
>>>>>>> +\begin{note}
>>>>>>> +For example, a device which does not expect to send interrupts at
>>>>>>> +a high rate might only specify 2 MSI vectors.
>>>>>>> +\end{note}
>>>>>>> +Device MUST support mapping any event type to any valid vector 0
>>>>>>> +to number of MSI vectors specified in \field{Multiple Message
>>>>>>> +Capable}
>>>>> field.
>>>>>>> +Device MUST support unmapping any event type.
>>>>>>> +
>>>>>>> +The device MUST return vector mapped to a given event, (NO_VECTOR
>>>>>>> +if unmapped) on read of
>>>>> \field{config_msix_vector}/\field{queue_msix_vector}.
>>>>>>> +The device MUST have all queue and configuration change events
>>>>>>> +unmapped upon reset.
>>>>>>> +
>>>>>>> +Devices SHOULD NOT cause mapping an event to vector to fail
>>>>>>> +unless it is impossible for the device to satisfy the mapping request.
>>>>>>> +Devices MUST report mapping failures by returning the NO_VECTOR
>>>>>>> +value when the relevant
>>>>>>> +\field{config_msix_vector}/\field{queue_msix_vector} field is read.
>>>>>>> +
>>>>>>> +\drivernormative{\subparagraph}{MSI Vector Configuration}{Virtio
>>>>>>> +Transport Options / Virtio Over PCI Bus / PCI-specific
>>>>>>> +Initialization And Device Operation / Device Initialization / MSI
>>>>>>> +Vector Configuration}
>>>>>>> +
>>>>>>> +Driver MUST support device with any MSI vector from 0 to 0x1F.
>>>>>>> +Driver MAY fall back on using INT\#x interrupts for a device
>>>>>>> +which only supports one MSI vector (MSI Multiple Message Capable =
>> 0).
>>>>>>> +
>>>> A single MSI (and MSI-X) vector is still far more optimal than INTx due to
>> the inefficiency of the INTx delivery on PCIe transport.
>>> And lack of support on VFs.
>>>
>>>> And for sw based devices, it anyway doesnt matter a lot either.
>>>>
>>>> So when a new functionality like MSI is added, it does not need to continue
>> what MSI-X has done.
>>>> So I request you to remove this guidance of INTx fallback on single MSI
>> vector.
>>> Let's provide an alternative guidance then.
>>> Device SHOULD implement at least 2 MSI vectors?
>> I think one MSI vector is enough for functionalities, just shared by all
>> interrupts.
>>
> Functionally MSI vector can work sub-optimally, because now on every VQ notification, the driver will inspect configuration space to discover changes, generating more unwanted PCI traffic.
when the device notifies the driver, the driver needs to read the vq avail / used index.
But the driver doesn't need to examine all config space when receive vq_notifications, the config space changes are reported through config interrupt.
> So, 2 seems a more practical tradeoff across functionality / performance / scale.
> And single MSI vector instead of INTx is of course a good recommendation by itself to include as independent line.
It should be a minimal requirement, not for optimization
>
>> Thanks
>> Zhu Lingshan
>>>> Rest all looks fine to me.

RE: [PATCH v2] transport-pci: Add MSI support

[Parav Pandit]

> From: Zhu Lingshan <lingshan.zhu@amd.com>
> Sent: Monday, September 30, 2024 12:41 PM
> 
> On 9/30/2024 3:01 PM, Parav Pandit wrote:
> >> From: Zhu Lingshan <lingshan.zhu@amd.com>
> >> Sent: Monday, September 30, 2024 12:13 PM
> >>
> >> On 9/30/2024 2:30 PM, Michael S. Tsirkin wrote:
> >>> On Mon, Sep 30, 2024 at 05:45:48AM +0000, Parav Pandit wrote:
> >>>>> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> >>>>> Sent: Tuesday, September 3, 2024 11:02 AM
> >>>>>
> >>>>> On Wed, Jul 24, 2024 at 09:51:43PM +0530, Manivannan Sadhasivam
> >> wrote:
> >>>>>> On Fri, Jul 12, 2024 at 07:31:44PM +0530, Manivannan Sadhasivam
> >> wrote:
> >>>>>>> MSI is the predecessor of MSI-X that allows PCIe devices to send
> >>>>>>> interrupts to the host. Compared to MSI-X, MSI supports only a
> >>>>>>> maximum of 32 vectors per PCIe function. But MSI has been widely
> >>>>>>> supported by the PCIe devices requiring fewer interrupts such as
> >>>>> Modems, WLAN cards etc...
> >>>>>>> Currently, Virtio spec only documents MSI-X and INTX interrupt
> >>>>>>> mechanisms for the PCI transport. So if a Virtio device based on
> >>>>>>> PCI transport supports only MSI, then the driver on the guest
> >>>>>>> will only use INTX for receiving the interrupts. This is really
> >>>>>>> sub-optimal and affects the performance of the device. Because
> >>>>>>> with MSI, the device can use one vector per queue (max of 32
> >>>>>>> vectors) thus avoiding the overhead associated with a shared
> >>>>>>> INTX
> >> vector.
> >>>>>>> Hence, add support for MSI to the Virtio PCI transport. MSI
> >>>>>>> support is added such a way that it reuses the existing
> >>>>>>> infrastructure of MSI-X, like the
> >>>>>>> config_msix_vector/queue_msix_vector fields of the Virtio common
> >>>>>>> config structure. This makes it easy for the Virtio drivers to
> >>>>>>> add MSI
> >> support without any disruptive changes.
> >>>>>> Gentle ping!
> >>>>>>
> >>>>> Ping again. Virtio maintainers, can you please share some feedback?
> >>>>>
> >>>> Overall looks to me.
> >>>> A small comment below on single MSI vector.
> >>>>
> >>>>
> >>>>> - Mani
> >>>>>
> >>>>>> - Mani
> >>>>>>
> >>>>>>> Signed-off-by: Manivannan Sadhasivam
> >>>>>>> <manivannan.sadhasivam@linaro.org>
> >>>>>>> ---
> >>>>>>>
> >>>>>>> Changes in v2:
> >>>>>>>
> >>>>>>> * Fixed a spelling mistake in commit message
> >>>>>>> * Removed update to legacy interface
> >>>>>>> * Used 'MSI vector' consistently
> >>>>>>>
> >>>>>>>  transport-pci.tex | 115
> >>>>>>> ++++++++++++++++++++++++++++++++++++----------
> >>>>>>>  1 file changed, 92 insertions(+), 23 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/transport-pci.tex b/transport-pci.tex index
> >>>>>>> a5c6719..fd92641 100644
> >>>>>>> --- a/transport-pci.tex
> >>>>>>> +++ b/transport-pci.tex
> >>>>>>> @@ -347,7 +347,7 @@ \subsubsection{Common configuration
> >> structure
> >>>>> layout}\label{sec:Virtio Transport
> >>>>>>>          Driver Feature Bits selected by \field{driver_feature_select}.
> >>>>>>>
> >>>>>>>  \item[\field{config_msix_vector}]
> >>>>>>> -        Set by the driver to the MSI-X vector for configuration change
> >>>>> notifications.
> >>>>>>> +        Set by the driver to the MSI/MSI-X vector for
> >>>>>>> + configuration change
> >>>>> notifications.
> >>>>>>>  \item[\field{num_queues}]
> >>>>>>>          The device specifies the maximum number of virtqueues
> >>>>>>> supported
> >>>>> here.
> >>>>>>> @@ -371,7 +371,7 @@ \subsubsection{Common configuration
> >> structure
> >>>>> layout}\label{sec:Virtio Transport
> >>>>>>>          A 0 means the queue is unavailable.
> >>>>>>>
> >>>>>>>  \item[\field{queue_msix_vector}]
> >>>>>>> -        Set by the driver to the MSI-X vector for virtqueue notifications.
> >>>>>>> +        Set by the driver to the MSI/MSI-X vector for virtqueue
> >>>>> notifications.
> >>>>>>>  \item[\field{queue_enable}]
> >>>>>>>          The driver uses this to selectively prevent the device
> >>>>>>> from executing
> >>>>> requests from this virtqueue.
> >>>>>>> @@ -631,11 +631,11 @@ \subsubsection{ISR status
> >>>>>>> capability}\label{sec:Virtio Transport Options / Virti  in
> >>>>>>> \field{ISR status} before sending a device configuration  change
> >>>>> notification to the driver.
> >>>>>>> -If MSI-X capability is disabled, the device MUST set the Queue
> >>>>>>> +If MSI/MSI-X capability is disabled, the device MUST set the
> >>>>>>> +Queue
> >>>>>>>  Interrupt bit in \field{ISR status} before sending a virtqueue
> >>>>>>> notification to the driver.
> >>>>>>>
> >>>>>>> -If MSI-X capability is disabled, the device MUST set the
> >>>>>>> Interrupt Status
> >>>>>>> +If MSI/MSI-X capability is disabled, the device MUST set the
> >>>>>>> +Interrupt Status
> >>>>>>>  bit in the PCI Status register in the PCI Configuration Header
> >>>>>>> of the device to the logical OR of all bits in \field{ISR
> >>>>>>> status} of the device.  The device then asserts/deasserts INT\#x
> >>>>>>> interrupts unless masked @@ -645,7 +645,7 @@ \subsubsection{ISR
> >>>>>>> status capability}\label{sec:Virtio Transport Options / Virti
> >>>>>>>
> >>>>>>>  \drivernormative{\paragraph}{ISR status capability}{Virtio
> >>>>>>> Transport Options / Virtio Over PCI Bus / PCI Device Layout /
> >>>>>>> ISR status capability}
> >>>>>>>
> >>>>>>> -If MSI-X capability is enabled, the driver SHOULD NOT access
> >>>>>>> +If MSI/MSI-X capability is enabled, the driver SHOULD NOT
> >>>>>>> +access
> >>>>>>>  \field{ISR status} upon detecting a Queue Interrupt.
> >>>>>>>
> >>>>>>>  \subsubsection{Device-specific configuration}\label{sec:Virtio
> >>>>>>> Transport Options / Virtio Over PCI Bus / PCI Device Layout /
> >>>>>>> Device-specific configuration} @@ -1017,7 +1017,7 @@
> >>>>>>> \subsubsection{Device Initialization}\label{sec:Virtio Transport
> >>>>>>> Options / Virti  \drivernormative{\subparagraph}{MSI-X Vector
> >>>>>>> Configuration}{Virtio Transport Options / Virtio Over PCI Bus /
> >>>>>>> PCI-specific Initialization And Device Operation / Device
> >>>>>>> Initialization / MSI-X Vector Configuration}
> >>>>>>>
> >>>>>>>  Driver MUST support device with any MSI-X Table Size 0 to 0x7FF.
> >>>>>>> -Driver MAY fall back on using INT\#x interrupts for a device
> >>>>>>> +Driver MAY fall back on using MSI or INT\#x interrupts for a
> >>>>>>> +device
> >>>>>>>  which only supports one MSI-X vector (MSI-X Table Size = 0).
> >>>>>>>
> >>>>>>>  Driver MAY interpret the Table Size as a hint from the device
> >>>>>>> @@
> >>>>>>> -1034,6 +1034,75 @@ \subsubsection{Device
> >>>>>>> Initialization}\label{sec:Virtio Transport Options / Virti  the
> >>>>>>> driver MAY retry mapping with fewer vectors, disable MSI-X  or
> >>>>>>> report
> >>>>> device failure.
> >>>>>>> +\paragraph{MSI Vector Configuration}\label{sec:Virtio Transport
> >>>>>>> +Options / Virtio Over PCI Bus / PCI-specific Initialization And
> >>>>>>> +Device Operation / Device Initialization / MSI Vector
> >>>>>>> +Configuration}
> >>>>>>> +
> >>>>>>> +When MSI capability is present and enabled in the device
> >>>>>>> +(through standard PCI configuration space)
> >>>>>>> +\field{config_msix_vector} and \field{queue_msix_vector} are
> >>>>>>> +used to map configuration change and
> >>>>> queue interrupts to MSI vectors. In this case, the ISR Status is unused.
> >>>>>>> +
> >>>>>>> +Writing a valid MSI vector, 0 to 0x1F, to
> >>>>>>> +\field{config_msix_vector}/\field{queue_msix_vector} maps
> >>>>>>> +interrupts triggered by the configuration change/selected queue
> >>>>>>> +events respectively to the corresponding MSI vector. To disable
> >>>>>>> +interrupts for an event type, the driver unmaps this event by
> >>>>>>> +writing a special NO_VECTOR
> >>>>>>> +value:
> >>>>>>> +
> >>>>>>> +\begin{lstlisting}
> >>>>>>> +/* Vector value used to disable MSI for queue */
> >>>>>>> +#define VIRTIO_MSI_NO_VECTOR            0xffff
> >>>>>>> +\end{lstlisting}
> >>>>>>> +
> >>>>>>> +Note that mapping an event to vector might require device to
> >>>>>>> +allocate internal device resources, and thus could fail.
> >>>>>>> +
> >>>>>>> +\devicenormative{\subparagraph}{MSI Vector
> >>>>>>> +Configuration}{Virtio Transport Options / Virtio Over PCI Bus /
> >>>>>>> +PCI-specific Initialization And Device Operation / Device
> >>>>>>> +Initialization / MSI Vector Configuration}
> >>>>>>> +
> >>>>>>> +A device that has an MSI capability SHOULD support at least 2
> >>>>>>> +and at most 0x20 MSI vectors.
> >>>>>>> +Device MUST report the number of vectors supported in
> >>>>>>> +\field{Multiple Message Capable} field in the MSI Capability as
> >>>>>>> +specified in \hyperref[intro:PCI]{[PCI]}.
> >>>>>>> +The device SHOULD restrict the reported MSI Multiple Message
> >>>>>>> +Capable field to a value that might benefit system performance.
> >>>>>>> +\begin{note}
> >>>>>>> +For example, a device which does not expect to send interrupts
> >>>>>>> +at a high rate might only specify 2 MSI vectors.
> >>>>>>> +\end{note}
> >>>>>>> +Device MUST support mapping any event type to any valid vector
> >>>>>>> +0 to number of MSI vectors specified in \field{Multiple Message
> >>>>>>> +Capable}
> >>>>> field.
> >>>>>>> +Device MUST support unmapping any event type.
> >>>>>>> +
> >>>>>>> +The device MUST return vector mapped to a given event,
> >>>>>>> +(NO_VECTOR if unmapped) on read of
> >>>>> \field{config_msix_vector}/\field{queue_msix_vector}.
> >>>>>>> +The device MUST have all queue and configuration change events
> >>>>>>> +unmapped upon reset.
> >>>>>>> +
> >>>>>>> +Devices SHOULD NOT cause mapping an event to vector to fail
> >>>>>>> +unless it is impossible for the device to satisfy the mapping request.
> >>>>>>> +Devices MUST report mapping failures by returning the NO_VECTOR
> >>>>>>> +value when the relevant
> >>>>>>> +\field{config_msix_vector}/\field{queue_msix_vector} field is read.
> >>>>>>> +
> >>>>>>> +\drivernormative{\subparagraph}{MSI Vector
> >>>>>>> +Configuration}{Virtio Transport Options / Virtio Over PCI Bus /
> >>>>>>> +PCI-specific Initialization And Device Operation / Device
> >>>>>>> +Initialization / MSI Vector Configuration}
> >>>>>>> +
> >>>>>>> +Driver MUST support device with any MSI vector from 0 to 0x1F.
> >>>>>>> +Driver MAY fall back on using INT\#x interrupts for a device
> >>>>>>> +which only supports one MSI vector (MSI Multiple Message
> >>>>>>> +Capable =
> >> 0).
> >>>>>>> +
> >>>> A single MSI (and MSI-X) vector is still far more optimal than INTx
> >>>> due to
> >> the inefficiency of the INTx delivery on PCIe transport.
> >>> And lack of support on VFs.
> >>>
> >>>> And for sw based devices, it anyway doesnt matter a lot either.
> >>>>
> >>>> So when a new functionality like MSI is added, it does not need to
> >>>> continue
> >> what MSI-X has done.
> >>>> So I request you to remove this guidance of INTx fallback on single
> >>>> MSI
> >> vector.
> >>> Let's provide an alternative guidance then.
> >>> Device SHOULD implement at least 2 MSI vectors?
> >> I think one MSI vector is enough for functionalities, just shared by
> >> all interrupts.
> >>
> > Functionally MSI vector can work sub-optimally, because now on every VQ
> notification, the driver will inspect configuration space to discover changes,
> generating more unwanted PCI traffic.
> when the device notifies the driver, the driver needs to read the vq avail / used
> index.
> But the driver doesn't need to examine all config space when receive
> vq_notifications, the config space changes are reported through config
> interrupt.
Right, but when there is only one vector recommendation, the driver cannot distinguish between vq notification vs config notification.
And it will result in inspecting config space changes.

Driver can implement moderation to not read at < 1msec interval.

So there are few options for driver and device each has trade off.

> > So, 2 seems a more practical tradeoff across functionality / performance /
> scale.
> > And single MSI vector instead of INTx is of course a good recommendation
> by itself to include as independent line.
> It should be a minimal requirement, not for optimization

Re: [PATCH v2] transport-pci: Add MSI support

[Zhu Lingshan]

On 9/30/2024 3:15 PM, Parav Pandit wrote:
>> From: Zhu Lingshan <lingshan.zhu@amd.com>
>> Sent: Monday, September 30, 2024 12:41 PM
>>
>> On 9/30/2024 3:01 PM, Parav Pandit wrote:
>>>> From: Zhu Lingshan <lingshan.zhu@amd.com>
>>>> Sent: Monday, September 30, 2024 12:13 PM
>>>>
>>>> On 9/30/2024 2:30 PM, Michael S. Tsirkin wrote:
>>>>> On Mon, Sep 30, 2024 at 05:45:48AM +0000, Parav Pandit wrote:
>>>>>>> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>>>>>> Sent: Tuesday, September 3, 2024 11:02 AM
>>>>>>>
>>>>>>> On Wed, Jul 24, 2024 at 09:51:43PM +0530, Manivannan Sadhasivam
>>>> wrote:
>>>>>>>> On Fri, Jul 12, 2024 at 07:31:44PM +0530, Manivannan Sadhasivam
>>>> wrote:
>>>>>>>>> MSI is the predecessor of MSI-X that allows PCIe devices to send
>>>>>>>>> interrupts to the host. Compared to MSI-X, MSI supports only a
>>>>>>>>> maximum of 32 vectors per PCIe function. But MSI has been widely
>>>>>>>>> supported by the PCIe devices requiring fewer interrupts such as
>>>>>>> Modems, WLAN cards etc...
>>>>>>>>> Currently, Virtio spec only documents MSI-X and INTX interrupt
>>>>>>>>> mechanisms for the PCI transport. So if a Virtio device based on
>>>>>>>>> PCI transport supports only MSI, then the driver on the guest
>>>>>>>>> will only use INTX for receiving the interrupts. This is really
>>>>>>>>> sub-optimal and affects the performance of the device. Because
>>>>>>>>> with MSI, the device can use one vector per queue (max of 32
>>>>>>>>> vectors) thus avoiding the overhead associated with a shared
>>>>>>>>> INTX
>>>> vector.
>>>>>>>>> Hence, add support for MSI to the Virtio PCI transport. MSI
>>>>>>>>> support is added such a way that it reuses the existing
>>>>>>>>> infrastructure of MSI-X, like the
>>>>>>>>> config_msix_vector/queue_msix_vector fields of the Virtio common
>>>>>>>>> config structure. This makes it easy for the Virtio drivers to
>>>>>>>>> add MSI
>>>> support without any disruptive changes.
>>>>>>>> Gentle ping!
>>>>>>>>
>>>>>>> Ping again. Virtio maintainers, can you please share some feedback?
>>>>>>>
>>>>>> Overall looks to me.
>>>>>> A small comment below on single MSI vector.
>>>>>>
>>>>>>
>>>>>>> - Mani
>>>>>>>
>>>>>>>> - Mani
>>>>>>>>
>>>>>>>>> Signed-off-by: Manivannan Sadhasivam
>>>>>>>>> <manivannan.sadhasivam@linaro.org>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> Changes in v2:
>>>>>>>>>
>>>>>>>>> * Fixed a spelling mistake in commit message
>>>>>>>>> * Removed update to legacy interface
>>>>>>>>> * Used 'MSI vector' consistently
>>>>>>>>>
>>>>>>>>>  transport-pci.tex | 115
>>>>>>>>> ++++++++++++++++++++++++++++++++++++----------
>>>>>>>>>  1 file changed, 92 insertions(+), 23 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/transport-pci.tex b/transport-pci.tex index
>>>>>>>>> a5c6719..fd92641 100644
>>>>>>>>> --- a/transport-pci.tex
>>>>>>>>> +++ b/transport-pci.tex
>>>>>>>>> @@ -347,7 +347,7 @@ \subsubsection{Common configuration
>>>> structure
>>>>>>> layout}\label{sec:Virtio Transport
>>>>>>>>>          Driver Feature Bits selected by \field{driver_feature_select}.
>>>>>>>>>
>>>>>>>>>  \item[\field{config_msix_vector}]
>>>>>>>>> -        Set by the driver to the MSI-X vector for configuration change
>>>>>>> notifications.
>>>>>>>>> +        Set by the driver to the MSI/MSI-X vector for
>>>>>>>>> + configuration change
>>>>>>> notifications.
>>>>>>>>>  \item[\field{num_queues}]
>>>>>>>>>          The device specifies the maximum number of virtqueues
>>>>>>>>> supported
>>>>>>> here.
>>>>>>>>> @@ -371,7 +371,7 @@ \subsubsection{Common configuration
>>>> structure
>>>>>>> layout}\label{sec:Virtio Transport
>>>>>>>>>          A 0 means the queue is unavailable.
>>>>>>>>>
>>>>>>>>>  \item[\field{queue_msix_vector}]
>>>>>>>>> -        Set by the driver to the MSI-X vector for virtqueue notifications.
>>>>>>>>> +        Set by the driver to the MSI/MSI-X vector for virtqueue
>>>>>>> notifications.
>>>>>>>>>  \item[\field{queue_enable}]
>>>>>>>>>          The driver uses this to selectively prevent the device
>>>>>>>>> from executing
>>>>>>> requests from this virtqueue.
>>>>>>>>> @@ -631,11 +631,11 @@ \subsubsection{ISR status
>>>>>>>>> capability}\label{sec:Virtio Transport Options / Virti  in
>>>>>>>>> \field{ISR status} before sending a device configuration  change
>>>>>>> notification to the driver.
>>>>>>>>> -If MSI-X capability is disabled, the device MUST set the Queue
>>>>>>>>> +If MSI/MSI-X capability is disabled, the device MUST set the
>>>>>>>>> +Queue
>>>>>>>>>  Interrupt bit in \field{ISR status} before sending a virtqueue
>>>>>>>>> notification to the driver.
>>>>>>>>>
>>>>>>>>> -If MSI-X capability is disabled, the device MUST set the
>>>>>>>>> Interrupt Status
>>>>>>>>> +If MSI/MSI-X capability is disabled, the device MUST set the
>>>>>>>>> +Interrupt Status
>>>>>>>>>  bit in the PCI Status register in the PCI Configuration Header
>>>>>>>>> of the device to the logical OR of all bits in \field{ISR
>>>>>>>>> status} of the device.  The device then asserts/deasserts INT\#x
>>>>>>>>> interrupts unless masked @@ -645,7 +645,7 @@ \subsubsection{ISR
>>>>>>>>> status capability}\label{sec:Virtio Transport Options / Virti
>>>>>>>>>
>>>>>>>>>  \drivernormative{\paragraph}{ISR status capability}{Virtio
>>>>>>>>> Transport Options / Virtio Over PCI Bus / PCI Device Layout /
>>>>>>>>> ISR status capability}
>>>>>>>>>
>>>>>>>>> -If MSI-X capability is enabled, the driver SHOULD NOT access
>>>>>>>>> +If MSI/MSI-X capability is enabled, the driver SHOULD NOT
>>>>>>>>> +access
>>>>>>>>>  \field{ISR status} upon detecting a Queue Interrupt.
>>>>>>>>>
>>>>>>>>>  \subsubsection{Device-specific configuration}\label{sec:Virtio
>>>>>>>>> Transport Options / Virtio Over PCI Bus / PCI Device Layout /
>>>>>>>>> Device-specific configuration} @@ -1017,7 +1017,7 @@
>>>>>>>>> \subsubsection{Device Initialization}\label{sec:Virtio Transport
>>>>>>>>> Options / Virti  \drivernormative{\subparagraph}{MSI-X Vector
>>>>>>>>> Configuration}{Virtio Transport Options / Virtio Over PCI Bus /
>>>>>>>>> PCI-specific Initialization And Device Operation / Device
>>>>>>>>> Initialization / MSI-X Vector Configuration}
>>>>>>>>>
>>>>>>>>>  Driver MUST support device with any MSI-X Table Size 0 to 0x7FF.
>>>>>>>>> -Driver MAY fall back on using INT\#x interrupts for a device
>>>>>>>>> +Driver MAY fall back on using MSI or INT\#x interrupts for a
>>>>>>>>> +device
>>>>>>>>>  which only supports one MSI-X vector (MSI-X Table Size = 0).
>>>>>>>>>
>>>>>>>>>  Driver MAY interpret the Table Size as a hint from the device
>>>>>>>>> @@
>>>>>>>>> -1034,6 +1034,75 @@ \subsubsection{Device
>>>>>>>>> Initialization}\label{sec:Virtio Transport Options / Virti  the
>>>>>>>>> driver MAY retry mapping with fewer vectors, disable MSI-X  or
>>>>>>>>> report
>>>>>>> device failure.
>>>>>>>>> +\paragraph{MSI Vector Configuration}\label{sec:Virtio Transport
>>>>>>>>> +Options / Virtio Over PCI Bus / PCI-specific Initialization And
>>>>>>>>> +Device Operation / Device Initialization / MSI Vector
>>>>>>>>> +Configuration}
>>>>>>>>> +
>>>>>>>>> +When MSI capability is present and enabled in the device
>>>>>>>>> +(through standard PCI configuration space)
>>>>>>>>> +\field{config_msix_vector} and \field{queue_msix_vector} are
>>>>>>>>> +used to map configuration change and
>>>>>>> queue interrupts to MSI vectors. In this case, the ISR Status is unused.
>>>>>>>>> +
>>>>>>>>> +Writing a valid MSI vector, 0 to 0x1F, to
>>>>>>>>> +\field{config_msix_vector}/\field{queue_msix_vector} maps
>>>>>>>>> +interrupts triggered by the configuration change/selected queue
>>>>>>>>> +events respectively to the corresponding MSI vector. To disable
>>>>>>>>> +interrupts for an event type, the driver unmaps this event by
>>>>>>>>> +writing a special NO_VECTOR
>>>>>>>>> +value:
>>>>>>>>> +
>>>>>>>>> +\begin{lstlisting}
>>>>>>>>> +/* Vector value used to disable MSI for queue */
>>>>>>>>> +#define VIRTIO_MSI_NO_VECTOR            0xffff
>>>>>>>>> +\end{lstlisting}
>>>>>>>>> +
>>>>>>>>> +Note that mapping an event to vector might require device to
>>>>>>>>> +allocate internal device resources, and thus could fail.
>>>>>>>>> +
>>>>>>>>> +\devicenormative{\subparagraph}{MSI Vector
>>>>>>>>> +Configuration}{Virtio Transport Options / Virtio Over PCI Bus /
>>>>>>>>> +PCI-specific Initialization And Device Operation / Device
>>>>>>>>> +Initialization / MSI Vector Configuration}
>>>>>>>>> +
>>>>>>>>> +A device that has an MSI capability SHOULD support at least 2
>>>>>>>>> +and at most 0x20 MSI vectors.
>>>>>>>>> +Device MUST report the number of vectors supported in
>>>>>>>>> +\field{Multiple Message Capable} field in the MSI Capability as
>>>>>>>>> +specified in \hyperref[intro:PCI]{[PCI]}.
>>>>>>>>> +The device SHOULD restrict the reported MSI Multiple Message
>>>>>>>>> +Capable field to a value that might benefit system performance.
>>>>>>>>> +\begin{note}
>>>>>>>>> +For example, a device which does not expect to send interrupts
>>>>>>>>> +at a high rate might only specify 2 MSI vectors.
>>>>>>>>> +\end{note}
>>>>>>>>> +Device MUST support mapping any event type to any valid vector
>>>>>>>>> +0 to number of MSI vectors specified in \field{Multiple Message
>>>>>>>>> +Capable}
>>>>>>> field.
>>>>>>>>> +Device MUST support unmapping any event type.
>>>>>>>>> +
>>>>>>>>> +The device MUST return vector mapped to a given event,
>>>>>>>>> +(NO_VECTOR if unmapped) on read of
>>>>>>> \field{config_msix_vector}/\field{queue_msix_vector}.
>>>>>>>>> +The device MUST have all queue and configuration change events
>>>>>>>>> +unmapped upon reset.
>>>>>>>>> +
>>>>>>>>> +Devices SHOULD NOT cause mapping an event to vector to fail
>>>>>>>>> +unless it is impossible for the device to satisfy the mapping request.
>>>>>>>>> +Devices MUST report mapping failures by returning the NO_VECTOR
>>>>>>>>> +value when the relevant
>>>>>>>>> +\field{config_msix_vector}/\field{queue_msix_vector} field is read.
>>>>>>>>> +
>>>>>>>>> +\drivernormative{\subparagraph}{MSI Vector
>>>>>>>>> +Configuration}{Virtio Transport Options / Virtio Over PCI Bus /
>>>>>>>>> +PCI-specific Initialization And Device Operation / Device
>>>>>>>>> +Initialization / MSI Vector Configuration}
>>>>>>>>> +
>>>>>>>>> +Driver MUST support device with any MSI vector from 0 to 0x1F.
>>>>>>>>> +Driver MAY fall back on using INT\#x interrupts for a device
>>>>>>>>> +which only supports one MSI vector (MSI Multiple Message
>>>>>>>>> +Capable =
>>>> 0).
>>>>>>>>> +
>>>>>> A single MSI (and MSI-X) vector is still far more optimal than INTx
>>>>>> due to
>>>> the inefficiency of the INTx delivery on PCIe transport.
>>>>> And lack of support on VFs.
>>>>>
>>>>>> And for sw based devices, it anyway doesnt matter a lot either.
>>>>>>
>>>>>> So when a new functionality like MSI is added, it does not need to
>>>>>> continue
>>>> what MSI-X has done.
>>>>>> So I request you to remove this guidance of INTx fallback on single
>>>>>> MSI
>>>> vector.
>>>>> Let's provide an alternative guidance then.
>>>>> Device SHOULD implement at least 2 MSI vectors?
>>>> I think one MSI vector is enough for functionalities, just shared by
>>>> all interrupts.
>>>>
>>> Functionally MSI vector can work sub-optimally, because now on every VQ
>> notification, the driver will inspect configuration space to discover changes,
>> generating more unwanted PCI traffic.
>> when the device notifies the driver, the driver needs to read the vq avail / used
>> index.
>> But the driver doesn't need to examine all config space when receive
>> vq_notifications, the config space changes are reported through config
>> interrupt.
> Right, but when there is only one vector recommendation, the driver cannot distinguish between vq notification vs config notification.
> And it will result in inspecting config space changes
That is true. And the driver can check config_generation to determine whether there is a config change.
Only one MSI vector would sure lead to performance overhead. But it is still the minimal requirements.

Additionally, the spec may want to say: Ideally each vq and the config space should have their own MSIX vector,
and should provide at least one MSI vector in its MSI capability.
>
> Driver can implement moderation to not read at < 1msec interval.
>
> So there are few options for driver and device each has trade off.
>
>>> So, 2 seems a more practical tradeoff across functionality / performance /
>> scale.
>>> And single MSI vector instead of INTx is of course a good recommendation
>> by itself to include as independent line.
>> It should be a minimal requirement, not for optimization

Re: [PATCH v2] transport-pci: Add MSI support

[Manivannan Sadhasivam]

On Mon, Sep 30, 2024 at 03:25:31PM +0800, Zhu Lingshan wrote:
> On 9/30/2024 3:15 PM, Parav Pandit wrote:
> >> From: Zhu Lingshan <lingshan.zhu@amd.com>
> >> Sent: Monday, September 30, 2024 12:41 PM
> >>

[...]

> >>>>>>>>> +Driver MUST support device with any MSI vector from 0 to 0x1F.
> >>>>>>>>> +Driver MAY fall back on using INT\#x interrupts for a device
> >>>>>>>>> +which only supports one MSI vector (MSI Multiple Message
> >>>>>>>>> +Capable =
> >>>> 0).
> >>>>>>>>> +
> >>>>>> A single MSI (and MSI-X) vector is still far more optimal than INTx
> >>>>>> due to
> >>>> the inefficiency of the INTx delivery on PCIe transport.
> >>>>> And lack of support on VFs.
> >>>>>
> >>>>>> And for sw based devices, it anyway doesnt matter a lot either.
> >>>>>>
> >>>>>> So when a new functionality like MSI is added, it does not need to
> >>>>>> continue
> >>>> what MSI-X has done.
> >>>>>> So I request you to remove this guidance of INTx fallback on single
> >>>>>> MSI
> >>>> vector.
> >>>>> Let's provide an alternative guidance then.
> >>>>> Device SHOULD implement at least 2 MSI vectors?
> >>>> I think one MSI vector is enough for functionalities, just shared by
> >>>> all interrupts.
> >>>>
> >>> Functionally MSI vector can work sub-optimally, because now on every VQ
> >> notification, the driver will inspect configuration space to discover changes,
> >> generating more unwanted PCI traffic.
> >> when the device notifies the driver, the driver needs to read the vq avail / used
> >> index.
> >> But the driver doesn't need to examine all config space when receive
> >> vq_notifications, the config space changes are reported through config
> >> interrupt.
> > Right, but when there is only one vector recommendation, the driver cannot distinguish between vq notification vs config notification.
> > And it will result in inspecting config space changes
> That is true. And the driver can check config_generation to determine whether there is a config change.
> Only one MSI vector would sure lead to performance overhead. But it is still the minimal requirements.
> 
> Additionally, the spec may want to say: Ideally each vq and the config space should have their own MSIX vector,
> and should provide at least one MSI vector in its MSI capability.
> >
> > Driver can implement moderation to not read at < 1msec interval.
> >
> > So there are few options for driver and device each has trade off.
> >
> >>> So, 2 seems a more practical tradeoff across functionality / performance /
> >> scale.
> >>> And single MSI vector instead of INTx is of course a good recommendation
> >> by itself to include as independent line.
> >> It should be a minimal requirement, not for optimization
> 

Thanks everyone for the comments! I guess the conclusion here is to drop the
INTx fallback (which makes sense to me). And for the minimal requirement of 2,
it is already mentioned in the 'Device requirements' section. But if the
fallback needs to be dropped, then that requirement has to be changed to '1' I
suppose.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2] transport-pci: Add MSI support

[Michael S. Tsirkin]

On Mon, Sep 30, 2024 at 02:42:40PM +0800, Zhu Lingshan wrote:
> On 9/30/2024 2:30 PM, Michael S. Tsirkin wrote:
> > On Mon, Sep 30, 2024 at 05:45:48AM +0000, Parav Pandit wrote:
> >>
> >>> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> >>> Sent: Tuesday, September 3, 2024 11:02 AM
> >>>
> >>> On Wed, Jul 24, 2024 at 09:51:43PM +0530, Manivannan Sadhasivam wrote:
> >>>> On Fri, Jul 12, 2024 at 07:31:44PM +0530, Manivannan Sadhasivam wrote:
> >>>>> MSI is the predecessor of MSI-X that allows PCIe devices to send
> >>>>> interrupts to the host. Compared to MSI-X, MSI supports only a
> >>>>> maximum of 32 vectors per PCIe function. But MSI has been widely
> >>>>> supported by the PCIe devices requiring fewer interrupts such as
> >>> Modems, WLAN cards etc...
> >>>>> Currently, Virtio spec only documents MSI-X and INTX interrupt
> >>>>> mechanisms for the PCI transport. So if a Virtio device based on PCI
> >>>>> transport supports only MSI, then the driver on the guest will only
> >>>>> use INTX for receiving the interrupts. This is really sub-optimal
> >>>>> and affects the performance of the device. Because with MSI, the
> >>>>> device can use one vector per queue (max of 32 vectors) thus
> >>>>> avoiding the overhead associated with a shared INTX vector.
> >>>>>
> >>>>> Hence, add support for MSI to the Virtio PCI transport. MSI support
> >>>>> is added such a way that it reuses the existing infrastructure of
> >>>>> MSI-X, like the config_msix_vector/queue_msix_vector fields of the
> >>>>> Virtio common config structure. This makes it easy for the Virtio
> >>>>> drivers to add MSI support without any disruptive changes.
> >>>>>
> >>>> Gentle ping!
> >>>>
> >>> Ping again. Virtio maintainers, can you please share some feedback?
> >>>
> >> Overall looks to me.
> >> A small comment below on single MSI vector.
> >>
> >>  
> >>> - Mani
> >>>
> >>>> - Mani
> >>>>
> >>>>> Signed-off-by: Manivannan Sadhasivam
> >>>>> <manivannan.sadhasivam@linaro.org>
> >>>>> ---
> >>>>>
> >>>>> Changes in v2:
> >>>>>
> >>>>> * Fixed a spelling mistake in commit message
> >>>>> * Removed update to legacy interface
> >>>>> * Used 'MSI vector' consistently
> >>>>>
> >>>>>  transport-pci.tex | 115
> >>>>> ++++++++++++++++++++++++++++++++++++----------
> >>>>>  1 file changed, 92 insertions(+), 23 deletions(-)
> >>>>>
> >>>>> diff --git a/transport-pci.tex b/transport-pci.tex index
> >>>>> a5c6719..fd92641 100644
> >>>>> --- a/transport-pci.tex
> >>>>> +++ b/transport-pci.tex
> >>>>> @@ -347,7 +347,7 @@ \subsubsection{Common configuration structure
> >>> layout}\label{sec:Virtio Transport
> >>>>>          Driver Feature Bits selected by \field{driver_feature_select}.
> >>>>>
> >>>>>  \item[\field{config_msix_vector}]
> >>>>> -        Set by the driver to the MSI-X vector for configuration change
> >>> notifications.
> >>>>> +        Set by the driver to the MSI/MSI-X vector for configuration change
> >>> notifications.
> >>>>>  \item[\field{num_queues}]
> >>>>>          The device specifies the maximum number of virtqueues supported
> >>> here.
> >>>>> @@ -371,7 +371,7 @@ \subsubsection{Common configuration structure
> >>> layout}\label{sec:Virtio Transport
> >>>>>          A 0 means the queue is unavailable.
> >>>>>
> >>>>>  \item[\field{queue_msix_vector}]
> >>>>> -        Set by the driver to the MSI-X vector for virtqueue notifications.
> >>>>> +        Set by the driver to the MSI/MSI-X vector for virtqueue
> >>> notifications.
> >>>>>  \item[\field{queue_enable}]
> >>>>>          The driver uses this to selectively prevent the device from executing
> >>> requests from this virtqueue.
> >>>>> @@ -631,11 +631,11 @@ \subsubsection{ISR status
> >>>>> capability}\label{sec:Virtio Transport Options / Virti  in
> >>>>> \field{ISR status} before sending a device configuration  change
> >>> notification to the driver.
> >>>>> -If MSI-X capability is disabled, the device MUST set the Queue
> >>>>> +If MSI/MSI-X capability is disabled, the device MUST set the Queue
> >>>>>  Interrupt bit in \field{ISR status} before sending a virtqueue
> >>>>> notification to the driver.
> >>>>>
> >>>>> -If MSI-X capability is disabled, the device MUST set the Interrupt
> >>>>> Status
> >>>>> +If MSI/MSI-X capability is disabled, the device MUST set the
> >>>>> +Interrupt Status
> >>>>>  bit in the PCI Status register in the PCI Configuration Header of
> >>>>> the device to the logical OR of all bits in \field{ISR status} of
> >>>>> the device.  The device then asserts/deasserts INT\#x interrupts
> >>>>> unless masked @@ -645,7 +645,7 @@ \subsubsection{ISR status
> >>>>> capability}\label{sec:Virtio Transport Options / Virti
> >>>>>
> >>>>>  \drivernormative{\paragraph}{ISR status capability}{Virtio
> >>>>> Transport Options / Virtio Over PCI Bus / PCI Device Layout / ISR
> >>>>> status capability}
> >>>>>
> >>>>> -If MSI-X capability is enabled, the driver SHOULD NOT access
> >>>>> +If MSI/MSI-X capability is enabled, the driver SHOULD NOT access
> >>>>>  \field{ISR status} upon detecting a Queue Interrupt.
> >>>>>
> >>>>>  \subsubsection{Device-specific configuration}\label{sec:Virtio
> >>>>> Transport Options / Virtio Over PCI Bus / PCI Device Layout /
> >>>>> Device-specific configuration} @@ -1017,7 +1017,7 @@
> >>>>> \subsubsection{Device Initialization}\label{sec:Virtio Transport
> >>>>> Options / Virti  \drivernormative{\subparagraph}{MSI-X Vector
> >>>>> Configuration}{Virtio Transport Options / Virtio Over PCI Bus /
> >>>>> PCI-specific Initialization And Device Operation / Device
> >>>>> Initialization / MSI-X Vector Configuration}
> >>>>>
> >>>>>  Driver MUST support device with any MSI-X Table Size 0 to 0x7FF.
> >>>>> -Driver MAY fall back on using INT\#x interrupts for a device
> >>>>> +Driver MAY fall back on using MSI or INT\#x interrupts for a device
> >>>>>  which only supports one MSI-X vector (MSI-X Table Size = 0).
> >>>>>
> >>>>>  Driver MAY interpret the Table Size as a hint from the device @@
> >>>>> -1034,6 +1034,75 @@ \subsubsection{Device
> >>>>> Initialization}\label{sec:Virtio Transport Options / Virti  the
> >>>>> driver MAY retry mapping with fewer vectors, disable MSI-X  or report
> >>> device failure.
> >>>>> +\paragraph{MSI Vector Configuration}\label{sec:Virtio Transport
> >>>>> +Options / Virtio Over PCI Bus / PCI-specific Initialization And
> >>>>> +Device Operation / Device Initialization / MSI Vector
> >>>>> +Configuration}
> >>>>> +
> >>>>> +When MSI capability is present and enabled in the device (through
> >>>>> +standard PCI configuration space) \field{config_msix_vector} and
> >>>>> +\field{queue_msix_vector} are used to map configuration change and
> >>> queue interrupts to MSI vectors. In this case, the ISR Status is unused.
> >>>>> +
> >>>>> +Writing a valid MSI vector, 0 to 0x1F, to
> >>>>> +\field{config_msix_vector}/\field{queue_msix_vector} maps
> >>>>> +interrupts triggered by the configuration change/selected queue
> >>>>> +events respectively to the corresponding MSI vector. To disable
> >>>>> +interrupts for an event type, the driver unmaps this event by
> >>>>> +writing a special NO_VECTOR
> >>>>> +value:
> >>>>> +
> >>>>> +\begin{lstlisting}
> >>>>> +/* Vector value used to disable MSI for queue */
> >>>>> +#define VIRTIO_MSI_NO_VECTOR            0xffff
> >>>>> +\end{lstlisting}
> >>>>> +
> >>>>> +Note that mapping an event to vector might require device to
> >>>>> +allocate internal device resources, and thus could fail.
> >>>>> +
> >>>>> +\devicenormative{\subparagraph}{MSI Vector Configuration}{Virtio
> >>>>> +Transport Options / Virtio Over PCI Bus / PCI-specific
> >>>>> +Initialization And Device Operation / Device Initialization / MSI
> >>>>> +Vector Configuration}
> >>>>> +
> >>>>> +A device that has an MSI capability SHOULD support at least 2 and
> >>>>> +at most 0x20 MSI vectors.
> >>>>> +Device MUST report the number of vectors supported in
> >>>>> +\field{Multiple Message Capable} field in the MSI Capability as
> >>>>> +specified in \hyperref[intro:PCI]{[PCI]}.
> >>>>> +The device SHOULD restrict the reported MSI Multiple Message
> >>>>> +Capable field to a value that might benefit system performance.
> >>>>> +\begin{note}
> >>>>> +For example, a device which does not expect to send interrupts at a
> >>>>> +high rate might only specify 2 MSI vectors.
> >>>>> +\end{note}
> >>>>> +Device MUST support mapping any event type to any valid vector 0 to
> >>>>> +number of MSI vectors specified in \field{Multiple Message Capable}
> >>> field.
> >>>>> +Device MUST support unmapping any event type.
> >>>>> +
> >>>>> +The device MUST return vector mapped to a given event, (NO_VECTOR
> >>>>> +if unmapped) on read of
> >>> \field{config_msix_vector}/\field{queue_msix_vector}.
> >>>>> +The device MUST have all queue and configuration change events
> >>>>> +unmapped upon reset.
> >>>>> +
> >>>>> +Devices SHOULD NOT cause mapping an event to vector to fail unless
> >>>>> +it is impossible for the device to satisfy the mapping request.
> >>>>> +Devices MUST report mapping failures by returning the NO_VECTOR
> >>>>> +value when the relevant
> >>>>> +\field{config_msix_vector}/\field{queue_msix_vector} field is read.
> >>>>> +
> >>>>> +\drivernormative{\subparagraph}{MSI Vector Configuration}{Virtio
> >>>>> +Transport Options / Virtio Over PCI Bus / PCI-specific
> >>>>> +Initialization And Device Operation / Device Initialization / MSI
> >>>>> +Vector Configuration}
> >>>>> +
> >>>>> +Driver MUST support device with any MSI vector from 0 to 0x1F.
> >>>>> +Driver MAY fall back on using INT\#x interrupts for a device which
> >>>>> +only supports one MSI vector (MSI Multiple Message Capable = 0).
> >>>>> +
> >> A single MSI (and MSI-X) vector is still far more optimal than INTx due to the inefficiency of the INTx delivery on PCIe transport.
> > And lack of support on VFs.
> >
> >> And for sw based devices, it anyway doesnt matter a lot either.
> >>
> >> So when a new functionality like MSI is added, it does not need to continue what MSI-X has done.
> >>
> >> So I request you to remove this guidance of INTx fallback on single MSI vector.
> > Let's provide an alternative guidance then.
> > Device SHOULD implement at least 2 MSI vectors?
> I think one MSI vector is enough for functionalities, just shared by all interrupts.
> 
> Thanks
> Zhu Lingshan


Within a VM, this forces an vmexit to check ISR on each interrupt.
Not good.

> >
> >> Rest all looks fine to me.
> >

Re: [PATCH v2] transport-pci: Add MSI support

[Zhu Lingshan]

On 9/30/2024 9:53 PM, Michael S. Tsirkin wrote:
> On Mon, Sep 30, 2024 at 02:42:40PM +0800, Zhu Lingshan wrote:
>> On 9/30/2024 2:30 PM, Michael S. Tsirkin wrote:
>>> On Mon, Sep 30, 2024 at 05:45:48AM +0000, Parav Pandit wrote:
>>>>> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>>>> Sent: Tuesday, September 3, 2024 11:02 AM
>>>>>
>>>>> On Wed, Jul 24, 2024 at 09:51:43PM +0530, Manivannan Sadhasivam wrote:
>>>>>> On Fri, Jul 12, 2024 at 07:31:44PM +0530, Manivannan Sadhasivam wrote:
>>>>>>> MSI is the predecessor of MSI-X that allows PCIe devices to send
>>>>>>> interrupts to the host. Compared to MSI-X, MSI supports only a
>>>>>>> maximum of 32 vectors per PCIe function. But MSI has been widely
>>>>>>> supported by the PCIe devices requiring fewer interrupts such as
>>>>> Modems, WLAN cards etc...
>>>>>>> Currently, Virtio spec only documents MSI-X and INTX interrupt
>>>>>>> mechanisms for the PCI transport. So if a Virtio device based on PCI
>>>>>>> transport supports only MSI, then the driver on the guest will only
>>>>>>> use INTX for receiving the interrupts. This is really sub-optimal
>>>>>>> and affects the performance of the device. Because with MSI, the
>>>>>>> device can use one vector per queue (max of 32 vectors) thus
>>>>>>> avoiding the overhead associated with a shared INTX vector.
>>>>>>>
>>>>>>> Hence, add support for MSI to the Virtio PCI transport. MSI support
>>>>>>> is added such a way that it reuses the existing infrastructure of
>>>>>>> MSI-X, like the config_msix_vector/queue_msix_vector fields of the
>>>>>>> Virtio common config structure. This makes it easy for the Virtio
>>>>>>> drivers to add MSI support without any disruptive changes.
>>>>>>>
>>>>>> Gentle ping!
>>>>>>
>>>>> Ping again. Virtio maintainers, can you please share some feedback?
>>>>>
>>>> Overall looks to me.
>>>> A small comment below on single MSI vector.
>>>>
>>>>  
>>>>> - Mani
>>>>>
>>>>>> - Mani
>>>>>>
>>>>>>> Signed-off-by: Manivannan Sadhasivam
>>>>>>> <manivannan.sadhasivam@linaro.org>
>>>>>>> ---
>>>>>>>
>>>>>>> Changes in v2:
>>>>>>>
>>>>>>> * Fixed a spelling mistake in commit message
>>>>>>> * Removed update to legacy interface
>>>>>>> * Used 'MSI vector' consistently
>>>>>>>
>>>>>>>  transport-pci.tex | 115
>>>>>>> ++++++++++++++++++++++++++++++++++++----------
>>>>>>>  1 file changed, 92 insertions(+), 23 deletions(-)
>>>>>>>
>>>>>>> diff --git a/transport-pci.tex b/transport-pci.tex index
>>>>>>> a5c6719..fd92641 100644
>>>>>>> --- a/transport-pci.tex
>>>>>>> +++ b/transport-pci.tex
>>>>>>> @@ -347,7 +347,7 @@ \subsubsection{Common configuration structure
>>>>> layout}\label{sec:Virtio Transport
>>>>>>>          Driver Feature Bits selected by \field{driver_feature_select}.
>>>>>>>
>>>>>>>  \item[\field{config_msix_vector}]
>>>>>>> -        Set by the driver to the MSI-X vector for configuration change
>>>>> notifications.
>>>>>>> +        Set by the driver to the MSI/MSI-X vector for configuration change
>>>>> notifications.
>>>>>>>  \item[\field{num_queues}]
>>>>>>>          The device specifies the maximum number of virtqueues supported
>>>>> here.
>>>>>>> @@ -371,7 +371,7 @@ \subsubsection{Common configuration structure
>>>>> layout}\label{sec:Virtio Transport
>>>>>>>          A 0 means the queue is unavailable.
>>>>>>>
>>>>>>>  \item[\field{queue_msix_vector}]
>>>>>>> -        Set by the driver to the MSI-X vector for virtqueue notifications.
>>>>>>> +        Set by the driver to the MSI/MSI-X vector for virtqueue
>>>>> notifications.
>>>>>>>  \item[\field{queue_enable}]
>>>>>>>          The driver uses this to selectively prevent the device from executing
>>>>> requests from this virtqueue.
>>>>>>> @@ -631,11 +631,11 @@ \subsubsection{ISR status
>>>>>>> capability}\label{sec:Virtio Transport Options / Virti  in
>>>>>>> \field{ISR status} before sending a device configuration  change
>>>>> notification to the driver.
>>>>>>> -If MSI-X capability is disabled, the device MUST set the Queue
>>>>>>> +If MSI/MSI-X capability is disabled, the device MUST set the Queue
>>>>>>>  Interrupt bit in \field{ISR status} before sending a virtqueue
>>>>>>> notification to the driver.
>>>>>>>
>>>>>>> -If MSI-X capability is disabled, the device MUST set the Interrupt
>>>>>>> Status
>>>>>>> +If MSI/MSI-X capability is disabled, the device MUST set the
>>>>>>> +Interrupt Status
>>>>>>>  bit in the PCI Status register in the PCI Configuration Header of
>>>>>>> the device to the logical OR of all bits in \field{ISR status} of
>>>>>>> the device.  The device then asserts/deasserts INT\#x interrupts
>>>>>>> unless masked @@ -645,7 +645,7 @@ \subsubsection{ISR status
>>>>>>> capability}\label{sec:Virtio Transport Options / Virti
>>>>>>>
>>>>>>>  \drivernormative{\paragraph}{ISR status capability}{Virtio
>>>>>>> Transport Options / Virtio Over PCI Bus / PCI Device Layout / ISR
>>>>>>> status capability}
>>>>>>>
>>>>>>> -If MSI-X capability is enabled, the driver SHOULD NOT access
>>>>>>> +If MSI/MSI-X capability is enabled, the driver SHOULD NOT access
>>>>>>>  \field{ISR status} upon detecting a Queue Interrupt.
>>>>>>>
>>>>>>>  \subsubsection{Device-specific configuration}\label{sec:Virtio
>>>>>>> Transport Options / Virtio Over PCI Bus / PCI Device Layout /
>>>>>>> Device-specific configuration} @@ -1017,7 +1017,7 @@
>>>>>>> \subsubsection{Device Initialization}\label{sec:Virtio Transport
>>>>>>> Options / Virti  \drivernormative{\subparagraph}{MSI-X Vector
>>>>>>> Configuration}{Virtio Transport Options / Virtio Over PCI Bus /
>>>>>>> PCI-specific Initialization And Device Operation / Device
>>>>>>> Initialization / MSI-X Vector Configuration}
>>>>>>>
>>>>>>>  Driver MUST support device with any MSI-X Table Size 0 to 0x7FF.
>>>>>>> -Driver MAY fall back on using INT\#x interrupts for a device
>>>>>>> +Driver MAY fall back on using MSI or INT\#x interrupts for a device
>>>>>>>  which only supports one MSI-X vector (MSI-X Table Size = 0).
>>>>>>>
>>>>>>>  Driver MAY interpret the Table Size as a hint from the device @@
>>>>>>> -1034,6 +1034,75 @@ \subsubsection{Device
>>>>>>> Initialization}\label{sec:Virtio Transport Options / Virti  the
>>>>>>> driver MAY retry mapping with fewer vectors, disable MSI-X  or report
>>>>> device failure.
>>>>>>> +\paragraph{MSI Vector Configuration}\label{sec:Virtio Transport
>>>>>>> +Options / Virtio Over PCI Bus / PCI-specific Initialization And
>>>>>>> +Device Operation / Device Initialization / MSI Vector
>>>>>>> +Configuration}
>>>>>>> +
>>>>>>> +When MSI capability is present and enabled in the device (through
>>>>>>> +standard PCI configuration space) \field{config_msix_vector} and
>>>>>>> +\field{queue_msix_vector} are used to map configuration change and
>>>>> queue interrupts to MSI vectors. In this case, the ISR Status is unused.
>>>>>>> +
>>>>>>> +Writing a valid MSI vector, 0 to 0x1F, to
>>>>>>> +\field{config_msix_vector}/\field{queue_msix_vector} maps
>>>>>>> +interrupts triggered by the configuration change/selected queue
>>>>>>> +events respectively to the corresponding MSI vector. To disable
>>>>>>> +interrupts for an event type, the driver unmaps this event by
>>>>>>> +writing a special NO_VECTOR
>>>>>>> +value:
>>>>>>> +
>>>>>>> +\begin{lstlisting}
>>>>>>> +/* Vector value used to disable MSI for queue */
>>>>>>> +#define VIRTIO_MSI_NO_VECTOR            0xffff
>>>>>>> +\end{lstlisting}
>>>>>>> +
>>>>>>> +Note that mapping an event to vector might require device to
>>>>>>> +allocate internal device resources, and thus could fail.
>>>>>>> +
>>>>>>> +\devicenormative{\subparagraph}{MSI Vector Configuration}{Virtio
>>>>>>> +Transport Options / Virtio Over PCI Bus / PCI-specific
>>>>>>> +Initialization And Device Operation / Device Initialization / MSI
>>>>>>> +Vector Configuration}
>>>>>>> +
>>>>>>> +A device that has an MSI capability SHOULD support at least 2 and
>>>>>>> +at most 0x20 MSI vectors.
>>>>>>> +Device MUST report the number of vectors supported in
>>>>>>> +\field{Multiple Message Capable} field in the MSI Capability as
>>>>>>> +specified in \hyperref[intro:PCI]{[PCI]}.
>>>>>>> +The device SHOULD restrict the reported MSI Multiple Message
>>>>>>> +Capable field to a value that might benefit system performance.
>>>>>>> +\begin{note}
>>>>>>> +For example, a device which does not expect to send interrupts at a
>>>>>>> +high rate might only specify 2 MSI vectors.
>>>>>>> +\end{note}
>>>>>>> +Device MUST support mapping any event type to any valid vector 0 to
>>>>>>> +number of MSI vectors specified in \field{Multiple Message Capable}
>>>>> field.
>>>>>>> +Device MUST support unmapping any event type.
>>>>>>> +
>>>>>>> +The device MUST return vector mapped to a given event, (NO_VECTOR
>>>>>>> +if unmapped) on read of
>>>>> \field{config_msix_vector}/\field{queue_msix_vector}.
>>>>>>> +The device MUST have all queue and configuration change events
>>>>>>> +unmapped upon reset.
>>>>>>> +
>>>>>>> +Devices SHOULD NOT cause mapping an event to vector to fail unless
>>>>>>> +it is impossible for the device to satisfy the mapping request.
>>>>>>> +Devices MUST report mapping failures by returning the NO_VECTOR
>>>>>>> +value when the relevant
>>>>>>> +\field{config_msix_vector}/\field{queue_msix_vector} field is read.
>>>>>>> +
>>>>>>> +\drivernormative{\subparagraph}{MSI Vector Configuration}{Virtio
>>>>>>> +Transport Options / Virtio Over PCI Bus / PCI-specific
>>>>>>> +Initialization And Device Operation / Device Initialization / MSI
>>>>>>> +Vector Configuration}
>>>>>>> +
>>>>>>> +Driver MUST support device with any MSI vector from 0 to 0x1F.
>>>>>>> +Driver MAY fall back on using INT\#x interrupts for a device which
>>>>>>> +only supports one MSI vector (MSI Multiple Message Capable = 0).
>>>>>>> +
>>>> A single MSI (and MSI-X) vector is still far more optimal than INTx due to the inefficiency of the INTx delivery on PCIe transport.
>>> And lack of support on VFs.
>>>
>>>> And for sw based devices, it anyway doesnt matter a lot either.
>>>>
>>>> So when a new functionality like MSI is added, it does not need to continue what MSI-X has done.
>>>>
>>>> So I request you to remove this guidance of INTx fallback on single MSI vector.
>>> Let's provide an alternative guidance then.
>>> Device SHOULD implement at least 2 MSI vectors?
>> I think one MSI vector is enough for functionalities, just shared by all interrupts.
>>
>> Thanks
>> Zhu Lingshan
>
> Within a VM, this forces an vmexit to check ISR on each interrupt.
> Not good.
True, but one vector mode is still the minimal requirements even with some
performance overhead. This mode has been verified on ifcvf and virtio should
not fail when the platform can provide only one vector.

Thanks
Zhu Lingshan
>
>>>> Rest all looks fine to me.