| Message ID | 1456771254-17511-19-git-send-email-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Feb 29, 2016 at 7:40 PM, Markus Armbruster <armbru@redhat.com> wrote: > The ivshmem device can either use MSI-X or legacy INTx for interrupts. > > With MSI-X enabled, peer interrupt events trigger an MSI as they > should. But software can still raise INTx via interrupt status and > mask register in BAR 0. This is explicitly prohibited by PCI Local > Bus Specification Revision 3.0, section 6.8.3.3: > > While enabled for MSI or MSI-X operation, a function is prohibited > from using its INTx# pin (if implemented) to request service (MSI, > MSI-X, and INTx# are mutually exclusive). > > Fix the device model to leave INTx alone when using MSI-X. > > Document that we claim to use INTx in config space even when we don't. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > hw/misc/ivshmem.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c > index cfea151..fc37feb 100644 > --- a/hw/misc/ivshmem.c > +++ b/hw/misc/ivshmem.c > @@ -126,6 +126,11 @@ static void ivshmem_update_irq(IVShmemState *s) > PCIDevice *d = PCI_DEVICE(s); > uint32_t isr = s->intrstatus & s->intrmask; > > + /* No INTx with msi=off, whether the guest enabled MSI-X or not */ > + if (ivshmem_has_feature(s, IVSHMEM_MSI)) { > + return; > + } > + > /* don't print ISR resets */ > if (isr) { > IVSHMEM_DPRINTF("Set IRQ to %d (%04x %04x)\n", > @@ -874,6 +879,10 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error **errp) > pci_conf = dev->config; > pci_conf[PCI_COMMAND] = PCI_COMMAND_IO | PCI_COMMAND_MEMORY; > > + /* > + * Note: we don't use INTx with IVSHMEM_MSI at all, so this is a > + * bald-faced lie then. But it's a backwards compatible lie. > + */ > pci_config_set_interrupt_pin(pci_conf, 1); I am not sure how much of a problem this is. Apparently, other devices claim interrupt and msi (ich, hda, pvscsi) Better ask someone more familiar with PCI details. > > memory_region_init_io(&s->ivshmem_mmio, OBJECT(s), &ivshmem_mmio_ops, s, > -- > 2.4.3 > >
On 01/03/2016 18:14, Marc-André Lureau wrote: > > + /* > > + * Note: we don't use INTx with IVSHMEM_MSI at all, so this is a > > + * bald-faced lie then. But it's a backwards compatible lie. > > + */ > > pci_config_set_interrupt_pin(pci_conf, 1); > > I am not sure how much of a problem this is. Apparently, other devices > claim interrupt and msi (ich, hda, pvscsi) > > Better ask someone more familiar with PCI details. The interrupt pin is read-only and just helps the OS figure out which interrupt is routed to intx. If you return early from ivshmem_update_irq if IVSHMEM_MSI, you should skip this line too. I think it's better to leave this line in and check if (msix_enabled(pci_dev)) { return; } in ivshmem_update_irq instead. This matches what xhci does, for example. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 01/03/2016 18:14, Marc-André Lureau wrote: >> > + /* >> > + * Note: we don't use INTx with IVSHMEM_MSI at all, so this is a >> > + * bald-faced lie then. But it's a backwards compatible lie. >> > + */ >> > pci_config_set_interrupt_pin(pci_conf, 1); >> >> I am not sure how much of a problem this is. Apparently, other devices >> claim interrupt and msi (ich, hda, pvscsi) >> >> Better ask someone more familiar with PCI details. > > The interrupt pin is read-only and just helps the OS figure out which > interrupt is routed to intx. If you return early from > ivshmem_update_irq if IVSHMEM_MSI, you should skip this line too. > > I think it's better to leave this line in and check > > if (msix_enabled(pci_dev)) { > return; > } > > in ivshmem_update_irq instead. This matches what xhci does, for example. Yes, but it's not what ivshmem has ever done. In other words, it's a backward-incompatible change. A PCI function declares whether it can do MSI or MSI-X with capabilities. Use of MSI and MSI-X is optional. Software can enable either MSI or MSI-X, both not both. When MSI-X is enabled, the function must signal interrupts via MSI-X. When MSI is enabled, it must signal interrupts via MSI. When neither is enabled, it signals interrupts via INTx *if* it has the pin wired up. PCI Local Bus Specification Revision 3.0, section 6.8 Message Signaled Interrupts: It is recommended that devices implement interrupt pins to provide compatibility in systems that do not support MSI (devices default to interrupt pins). However, it is expected that the need for interrupt pins will diminish over time. Devices that do not support interrupt pins due to pin constraints (rely on polling for device service) may implement messages to increase performance without adding additional pins. Therefore, system configuration software must not assume that a message capable device has an interrupt pin. The xhci device *does* implement this fallback to INTx. For better or worse, fallback to INTx has never been implemented in ivshmem. You can either ask for an INTx-only device (msi=off), or for an MSI-X-only device (msi=on). The latter *cannot* do interrupts until you enable MSI-X. Similarly, the ivshmem-doorbell device introduced later in this series can only do MSI-X, and the ivshmem-plain device cannot do interrupts at all. We could of course implement the fallback in ivshmem, too. It's not quite as simple as making ivshmem_update_irq() do nothing when msix_enabled(), we also have to adapt ivshmem_vector_notify(), update ivshmem-spec.txt, and cover the fallback in the tests. Also limit the change to revision 1 of the device for compatibility. I very much doubt this is worth the trouble. A PCI function declares its INTx use in config space register Interrupt Pin. Ibid., section 6.2.4. Miscellaneous Registers: The Interrupt Pin register tells which interrupt pin the device (or device function) uses. A value of 1 corresponds to INTA#. A value of 2 corresponds to INTB#. A value of 3 corresponds to INTC#. A value of 4 corresponds to INTD#. Devices (or device functions) that do not use an interrupt pin must put a 0 in this register. ivshmem with msi=on should therefore put 0 in this register. It doesn't, but I feel it's better to let it remain bug-compatible. ivshmem-doorbell and ivshmem-plain get it right. Aside: xhci falls back to INTx, and should therefore declare its use of INTx in the Interrupt Pin register, but I can't see where it does.
On 02/03/2016 12:04, Markus Armbruster wrote: > For better or worse, fallback to INTx has never been implemented in > ivshmem. You can either ask for an INTx-only device (msi=off), or for > an MSI-X-only device (msi=on). The latter *cannot* do interrupts until > you enable MSI-X. Aha, now I see what you mean: if (ivshmem_has_feature(s, IVSHMEM_MSI)) { msix_notify(pdev, vector); } else { ivshmem_IntrStatus_write(s, 1); } So I believe your patch is okay. Perhaps you could also change the interrupt pin for new machine types (even without changing the revision), but it's not necessary to do it. > Similarly, the ivshmem-doorbell device introduced later in this series > can only do MSI-X, and the ivshmem-plain device cannot do interrupts at > all. Here: dev->config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin 1 */ Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 02/03/2016 12:04, Markus Armbruster wrote: >> For better or worse, fallback to INTx has never been implemented in >> ivshmem. You can either ask for an INTx-only device (msi=off), or for >> an MSI-X-only device (msi=on). The latter *cannot* do interrupts until >> you enable MSI-X. > > Aha, now I see what you mean: > > if (ivshmem_has_feature(s, IVSHMEM_MSI)) { > msix_notify(pdev, vector); > } else { > ivshmem_IntrStatus_write(s, 1); > } > > So I believe your patch is okay. I can try to explain it a bit better in the comment and/or commit message when I respin. > Perhaps you could also change the > interrupt pin for new machine types (even without changing the > revision), but it's not necessary to do it. I chose not to bother, because after PATCH 34, the non-deprecated devices are all revision 1 (correct Interrupt Pin register). >> Similarly, the ivshmem-doorbell device introduced later in this series >> can only do MSI-X, and the ivshmem-plain device cannot do interrupts at >> all. > > Here: > > dev->config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin 1 */ Ah! I looked only for pci_config_set_interrupt_pin().
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c index cfea151..fc37feb 100644 --- a/hw/misc/ivshmem.c +++ b/hw/misc/ivshmem.c @@ -126,6 +126,11 @@ static void ivshmem_update_irq(IVShmemState *s) PCIDevice *d = PCI_DEVICE(s); uint32_t isr = s->intrstatus & s->intrmask; + /* No INTx with msi=off, whether the guest enabled MSI-X or not */ + if (ivshmem_has_feature(s, IVSHMEM_MSI)) { + return; + } + /* don't print ISR resets */ if (isr) { IVSHMEM_DPRINTF("Set IRQ to %d (%04x %04x)\n", @@ -874,6 +879,10 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error **errp) pci_conf = dev->config; pci_conf[PCI_COMMAND] = PCI_COMMAND_IO | PCI_COMMAND_MEMORY; + /* + * Note: we don't use INTx with IVSHMEM_MSI at all, so this is a + * bald-faced lie then. But it's a backwards compatible lie. + */ pci_config_set_interrupt_pin(pci_conf, 1); memory_region_init_io(&s->ivshmem_mmio, OBJECT(s), &ivshmem_mmio_ops, s,
The ivshmem device can either use MSI-X or legacy INTx for interrupts. With MSI-X enabled, peer interrupt events trigger an MSI as they should. But software can still raise INTx via interrupt status and mask register in BAR 0. This is explicitly prohibited by PCI Local Bus Specification Revision 3.0, section 6.8.3.3: While enabled for MSI or MSI-X operation, a function is prohibited from using its INTx# pin (if implemented) to request service (MSI, MSI-X, and INTx# are mutually exclusive). Fix the device model to leave INTx alone when using MSI-X. Document that we claim to use INTx in config space even when we don't. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- hw/misc/ivshmem.c | 9 +++++++++ 1 file changed, 9 insertions(+)