virtio-comment

 View Only

Re: spec inconsistency: Device Configuration Interrupt bit in ISR status

  • 1.  Re: spec inconsistency: Device Configuration Interrupt bit in ISR status

    Posted 01-18-2022 06:25
    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 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.
    >
    > 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
    >
    >
    >