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

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Thu, Jul 11, 2024 at 03:54:12AM -0400, Michael S. Tsirkin wrote:
> On Thu, Jul 11, 2024 at 12:29:19PM +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 Virito common config
> 
> avoid misspellings please.
> 

Sure.

> > 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>
> > ---
> >  transport-pci.tex | 125 +++++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 97 insertions(+), 28 deletions(-)
> > 
> > diff --git a/transport-pci.tex b/transport-pci.tex
> > index a5c6719..f8e6ccd 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}
> > @@ -838,7 +838,7 @@ \subsubsection{Legacy Interfaces: A Note on PCI Device Layout}\label{sec:Virtio
> >  \hline
> >  \end{tabularx}
> >  
> > -If MSI-X is enabled for the device, two additional fields
> > +If MSI/MSI-X is enabled for the device, two additional fields
> >  immediately follow this header:
> >  
> >  \begin{tabular}{ |l||l|l| }
> > @@ -847,14 +847,14 @@ \subsubsection{Legacy Interfaces: A Note on PCI Device Layout}\label{sec:Virtio
> >  \hline
> >  Read/Write & R+W            & R+W    \\
> >  \hline
> > -Purpose (MSI-X) & \field{config_msix_vector}  & \field{queue_msix_vector} \\
> > +Purpose (MSI/MSI-X) & \field{config_msix_vector}  & \field{queue_msix_vector} \\
> >  \hline
> >  \end{tabular}
> >  
> > -Note: When MSI-X capability is enabled, device-specific configuration starts at
> > -byte offset 24 in virtio common configuration structure. When MSI-X capability is not
> > +Note: When MSI/MSI-X capability is enabled, device-specific configuration starts at
> > +byte offset 24 in virtio common configuration structure. When MSI/MSI-X capability is not
> >  enabled, device-specific configuration starts at byte offset 20 in virtio
> > -header.  ie. once you enable MSI-X on the device, the other fields move.
> > +header.  ie. once you enable MSI/MSI-X on the device, the other fields move.
> >  If you turn it off again, they move back!
> >  
> >  Any device-specific configuration space immediately follows
> 
> 
> Legacy is legacy. It is there to document compatibility to existing
> drivers and hypervisors.  No changes, besides bugfixes to the legacy
> interface should be accepted.
> 

Ok.

> 
> > @@ -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).
> 
> 
> Here do we want to also document fallback from MSI to INTx?
> 

I think it is not required, since it says that the driver call fallback to
either MSI or INTX. And the MSI section documents the immediate fallback to
INTX.

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

[...]

> > +\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
> 
> Looks like a lot of text duplicated from MSI-X, can't we
> avoid doing that?
> 

Well, I initially tried to add MSI to the existing MSI-X sections, but that
looked messy. And all of the wordings are applicable to MSI as well. That's why
I kept them.

> 
> > @@ -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 number
> > +  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 number.
> 
> if we go with "the MSI-X Table entry" we should also do "the MSI
> vector".  We are inconsistent in this unfortunately, but at least not
> inside the same sentence.
> Applies elsewhere, too.
> 

Sure.

- Mani

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