On Thu, Sep 17, 2015 at 10:49:06AM -0500, Eric W. Biederman wrote:
> Bjorn Helgaas <bhelgaas@xxxxxxxxxx> writes:
>
> > On Sun, Sep 06, 2015 at 06:32:35PM +0300, Michael S. Tsirkin wrote:
> >> On some hypervisors, virtio devices tend to generate spurious interrupts
> >> when switching between MSI and non-MSI mode. Normally, either MSI or
> >> non-MSI is used and all is well, but during shutdown, linux disables MSI
> >> which then causes an "irq %d: nobody cared" message, with irq being
> >> subsequently disabled.
> >>
> >> Since bus mastering is already disabled at this point, disabling MSI
> >> isn't actually useful for spec compliant devices: MSI interrupts are
> >> in-bus memory writes so disabling Bus Master (which is already done)
> >> disables these as well: after some research, it appears to be there for
> >> the benefit of devices that ignore the bus master bit.
> >>
> >> As it's not clear how common this kind of bug is, this patch simply adds
> >> a quirk, to be set by devices that wish to skip disabling msi on
> >> shutdown, relying on bus master instead.
> >>
> >> We set this quirk in virtio core.
> >>
> >> Reported-by: Fam Zheng <famz@xxxxxxxxxx>
> >> Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> >> Cc: Yinghai Lu <yhlu.kernel.send@xxxxxxxxx>
> >> Cc: Ulrich Obergfell <uobergfe@xxxxxxxxxx>
> >> Cc: Rusty Russell <rusty@xxxxxxxxxxxxxxx>
> >> Cc: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
> >> Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
> >
> > Eric, what do you think of this? You had many comments on previous
> > versions.
>
> I still think it is a hack to avoid actually fixing a driver.
The bug's in the hypervisor though.
> I think the hack is better as a quirk.
You mean put it in drivers/pci/quirks.c?
> I think the quirk would tend to
> be better if it was limited to whatever set of hypervisors where this is
> actually a problem.
I guess we might add a new flag "msi disable at shutdown is safe" in future
hypervisors. Until such hypervisors ship I don't see a way to detect
this.
> And of course given that on any sane configuration we have the irq
> watchdog I don't see what this is fixing in practice.
>
> Other than suggesting this hack become find a way to limit itself to the
> driver that is actually having problems I don't see a way to improve it.
It's already limited: virtio is the only one that sets
PCI_DEV_FLAGS_NO_MSI_SHUTDOWN. Did you notice this?
> > Minor comment on a comment below.
> >
> >> ---
> >>
> >>
> >> changes from v6:
> >> limit changes to virtio only
> >> changes from v5:
> >> rebased on top of pci/msi
> >> fixed commit log, including comments by Bjorn
> >> and adding explanation to address comments/questions by Eric
> >> dropped stable Cc, this patch does not seem to qualify for stable
> >> changes from v4:
> >> Yijing Wang <wangyijing@xxxxxxxxxx> noted that
> >> early fixups rely on pci_msi_off.
> >> Split out the functionality and move off the
> >> required part to run early during pci_device_setup.
> >> Changes from v3:
> >> fix a copy-and-paste error in
> >> pci: drop some duplicate code
> >> other patches are unchanged
> >> drop Cc stable for now
> >> Changes from v2:
> >> move code from probe to device enumeration
> >> add patches to unexport pci_msi_off
> >>
> >>
> >> include/linux/pci.h | 2 ++
> >> drivers/pci/pci-driver.c | 6 ++++--
> >> drivers/virtio/virtio_pci_common.c | 4 ++++
> >> 3 files changed, 10 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/linux/pci.h b/include/linux/pci.h
> >> index 860c751..80f3494 100644
> >> --- a/include/linux/pci.h
> >> +++ b/include/linux/pci.h
> >> @@ -180,6 +180,8 @@ enum pci_dev_flags {
> >> PCI_DEV_FLAGS_NO_BUS_RESET = (__force pci_dev_flags_t) (1 << 6),
> >> /* Do not use PM reset even if device advertises NoSoftRst- */
> >> PCI_DEV_FLAGS_NO_PM_RESET = (__force pci_dev_flags_t) (1 << 7),
> >> + /* Do not disable MSI on shutdown, disable bus master instead */
> >
> > This comment doesn't really match what the patch does. The patch merely
> > does "Do not disable MSI on shutdown." It doesn't "disable bus master
> > instead."
> >
> > Bus master may be disabled elsewhere, but that is independent of the
> > PCI_DEV_FLAGS_NO_MSI_SHUTDOWN flag.
> >
> >> + PCI_DEV_FLAGS_NO_MSI_SHUTDOWN = (__force pci_dev_flags_t) (1 << 8),
> >> };
> >>
> >> enum pci_irq_reroute_variant {
> >> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> >> index 3cb2210..59d9e40 100644
> >> --- a/drivers/pci/pci-driver.c
> >> +++ b/drivers/pci/pci-driver.c
> >> @@ -450,8 +450,10 @@ static void pci_device_shutdown(struct device *dev)
> >>
> >> if (drv && drv->shutdown)
> >> drv->shutdown(pci_dev);
> >> - pci_msi_shutdown(pci_dev);
> >> - pci_msix_shutdown(pci_dev);
> >> + if (!(pci_dev->dev_flags & PCI_DEV_FLAGS_NO_MSI_SHUTDOWN)) {
> >> + pci_msi_shutdown(pci_dev);
> >> + pci_msix_shutdown(pci_dev);
> >> + }
> >>
> >> #ifdef CONFIG_KEXEC
> >> /*
> >> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> >> index 78f804a..26f46c3 100644
> >> --- a/drivers/virtio/virtio_pci_common.c
> >> +++ b/drivers/virtio/virtio_pci_common.c
> >> @@ -528,6 +528,8 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
> >> if (rc)
> >> goto err_register;
> >>
> >> + pci_dev->dev_flags |= PCI_DEV_FLAGS_NO_MSI_SHUTDOWN;
> >> +
> >> return 0;
> >>
> >> err_register:
> >> @@ -546,6 +548,8 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
> >> {
> >> struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
> >>
> >> + pci_dev->dev_flags &= ~PCI_DEV_FLAGS_NO_MSI_SHUTDOWN;
> >> +
> >> unregister_virtio_device(&vp_dev->vdev);
> >>
> >> if (vp_dev->ioaddr)
> >> --
> >> MST
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
