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

[Jason Wang <jasowang@redhat.com>]

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 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
>