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

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

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