On Tue, Jan 18, 2022 at 11:12:43AM +0800, Zhu, Lingshan wrote:
>
>
> 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 8bit 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 deassert the
> interrupt.
> In this way, driver read of ISR status causes the device to deassert 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 MSIX 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 MSIX capability is enabled:
> 1. If config_msix_vector is not NO_VECTOR, request the appropriate MSIX interrupt message for
> the device, config_msix_vector sets the MSIX 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 MSIX 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.
>
> Thanks
>
No, we actually need that for bypass. Only config interrupts set ISR.
I will need to send a patch, people are still confused.
> 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
>
>
>