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

[Zhu Lingshan <lingshan.zhu@amd.com>]

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.