virtio-comment

? 2022/1/18 ??11:12, Zhu, Lingshan ??:
>
>
> On 1/18/2022 11:01 AM, Jason Wang wrote:
>> On Mon, Jan 17, 2022 at 3:57 PM Michael S. Tsirkin<mst@redhat.com> wrote:
>>> On Mon, Jan 17, 2022 at 02:15:55PM +0800, Jason Wang wrote:
>>>> ? 2022/1/14 ??9:39, Michael S. Tsirkin ??:
>>>>> The spec says (v1.1 4.1.4.5 ISR status capability):
>>>>>
>>>>> The VIRTIO_PCI_CAP_ISR_CFG capability refers to at least a single byte, which contains the 8­bit ISR
>>>>> status field to be used for INT#x interrupt handling.
>>>>>
>>>>> and
>>>>>
>>>>> to avoid an extra access, simply reading this register resets it to 0 and causes the device to de­assert the
>>>>> interrupt.
>>>>> In this way, driver read of ISR status causes the device to de­assert an interrupt.
>>>>>
>>>>> See sections 4.1.5.3 and 4.1.5.4 for how this is used.
>>>>>
>>>>> and in 4.1.5.4 Notification of Device Configuration Changes
>>>>>
>>>>> it says:
>>>>>
>>>>> • If MSI­X capability is disabled:
>>>>> 1. Set the second lower bit of the ISR Status field for the device.
>>>>> 2. Send the appropriate PCI interrupt for the device.
>>>>>
>>>>> If MSI­X capability is enabled:
>>>>> 1. If config_msix_vector is not NO_VECTOR, request the appropriate MSI­X interrupt message for
>>>>> the device, config_msix_vector sets the MSI­X Table entry number.
>>>>>
>>>>> all of the above make it looks like VIRTIO_PCI_CAP_ISR_CFG capability is
>>>>> unused with MSIX.
>>>>>
>>>>> This was actually the way the spec was understood by
>>>>> Zhu Lingshan from Intel (Cc'd).
>>>>>
>>>>> However, looking at the conformance statements, one finds out this is
>>>>> not the case:
>>>>>
>>>>> 4.1.4.5.1 Device Requirements: ISR status capability
>>>>>
>>>>>
>>>>> The device MUST present at least one VIRTIO_PCI_CAP_ISR_CFG capability.
>>>>> The device MUST set the Device Configuration Interrupt bit in ISR status before sending a device configu­
>>>>> ration change notification to the driver.
>>>>> If MSI­X capability is disabled, the device MUST set the Queue Interrupt bit in ISR status before sending a
>>>>> virtqueue notification to the driver.
>>>>>
>>>>> which implies that the Device Configuration Interrupt bit is set unconditionally.
>>>>>
>>>>>
>>>>>
>>>>> It is unfortunate that it does not copy this requirement in more places,
>>>>> and that the non-conformance text is incomplete and does not
>>>>> mention the MSI-X usage at all.
>>>>>
>>>>> I propose to extend 4.1.4.5 ISR status capability and
>>>>> 4.1.5.4 Notification of Device Configuration Changes
>>>>> to mention the MSI use.
> I agree to expand ISR cap usage to MSI(MSIX) usage with clear
> descriptions,
> and remove the limitations to MSIX. E.g,:
> 4.1.4.5.2 Driver Requirements: ISR status capability
> If MSI-X capability is enabled, the driver SHOULD NOT access ISR
> status upon detecting a Queue Interrupt.


I guess the idea is only for config MSI not for queue. Otherwise it
would be hard to implement fast irq injection.

Thanks


>
>
> Thanks
>>>> I wonder do we want
>>>>
>>>> 1) mandate ISR bit
>>>>
>>>> or
>>>>
>>>> 2) remove the ISR bit set for MSI mode?
>>>>
>>>> 1) is the current Qemu behavior but seems a little bit contradict with the
>>>> goal of MSI.
>>>>
>>>> Thanks
>>>>
>>>>
>>> I think we want the ISR bit
>> This means there's no chance to use fast irq path but since it's a
>> less frequent operation. It should be fine.
>>
>> Thanks
>>
>>> since otherwise we need to go read
>>> a ton of config space fields to check whether anything changed.
>>>
>>> --
>>> MST
>>>
>