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

["Michael S. Tsirkin" <mst@redhat.com>]

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