Michael S. Tsirkin
2015-Sep-06 15:32 UTC
[PATCH v7] pci: quirk to skip msi disable on shutdown
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 at redhat.com> Cc: Bjorn Helgaas <bhelgaas at google.com> Cc: Yinghai Lu <yhlu.kernel.send at gmail.com> Cc: Ulrich Obergfell <uobergfe at redhat.com> Cc: Rusty Russell <rusty at rustcorp.com.au> Cc: "Eric W. Biederman" <ebiederm at xmission.com> Signed-off-by: Michael S. Tsirkin <mst at redhat.com> --- 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 at huawei.com> 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 */ + 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
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 at redhat.com> > Cc: Bjorn Helgaas <bhelgaas at google.com> > Cc: Yinghai Lu <yhlu.kernel.send at gmail.com> > Cc: Ulrich Obergfell <uobergfe at redhat.com> > Cc: Rusty Russell <rusty at rustcorp.com.au> > Cc: "Eric W. Biederman" <ebiederm at xmission.com> > Signed-off-by: Michael S. Tsirkin <mst at redhat.com>Eric, what do you think of this? You had many comments on previous versions. 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 at huawei.com> 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
Eric W. Biederman
2015-Sep-17 15:49 UTC
[PATCH v7] pci: quirk to skip msi disable on shutdown
Bjorn Helgaas <bhelgaas at google.com> 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 at redhat.com> >> Cc: Bjorn Helgaas <bhelgaas at google.com> >> Cc: Yinghai Lu <yhlu.kernel.send at gmail.com> >> Cc: Ulrich Obergfell <uobergfe at redhat.com> >> Cc: Rusty Russell <rusty at rustcorp.com.au> >> Cc: "Eric W. Biederman" <ebiederm at xmission.com> >> Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > > 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. I think the hack is better as a quirk. I think the quirk would tend to be better if it was limited to whatever set of hypervisors where this is actually a problem. 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.> 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 at huawei.com> 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
Michael S. Tsirkin
2015-Sep-17 15:56 UTC
[PATCH v7] pci: quirk to skip msi disable on shutdown
On Thu, Sep 17, 2015 at 10:44:24AM -0500, Bjorn Helgaas wrote:> 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 at redhat.com> > > Cc: Bjorn Helgaas <bhelgaas at google.com> > > Cc: Yinghai Lu <yhlu.kernel.send at gmail.com> > > Cc: Ulrich Obergfell <uobergfe at redhat.com> > > Cc: Rusty Russell <rusty at rustcorp.com.au> > > Cc: "Eric W. Biederman" <ebiederm at xmission.com> > > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > > Eric, what do you think of this? You had many comments on previous > versions. > > 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 at huawei.com> 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.Yes but I wanted to point out that bus master needs to work. One of the reasons we don't do this patch for all devices is because some builtin devices might have a broken bus master. How about "Some other means (e.g. disabling bus master) suppress interrupts."> > + 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
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.My understanding is: Linux disables MSI/MSI-X during device shutdown. If the device signals an interrupt after that, it may use INTx. This INTx interrupt is not necessarily spurious. Using INTx to signal an interrupt that occurs when MSI is disabled seems like reasonable behavior for any PCI device. And it doesn't seem related to switching between MSI and non-MSI mode. Yes, the INTx happens *after* disabling MSI, but it is not at all *because* we disabled MSI. So I wouldn't say "they generate spurious interrupts when switching between MSI and non-MSI." Why doesn't virtio-pci just register an INTx handler in addition to an MSI handler? Bjorn
Michael S. Tsirkin
2015-Sep-21 19:42 UTC
[PATCH v7] pci: quirk to skip msi disable on shutdown
On Mon, Sep 21, 2015 at 01:21:47PM -0500, Bjorn Helgaas wrote:> 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. > > My understanding is: > > Linux disables MSI/MSI-X during device shutdown. If the device > signals an interrupt after that, it may use INTx. > > This INTx interrupt is not necessarily spurious. Using INTx to signal an > interrupt that occurs when MSI is disabled seems like reasonable behavior > for any PCI device. > And it doesn't seem related to switching between MSI and non-MSI mode. > Yes, the INTx happens *after* disabling MSI, but it is not at all > *because* we disabled MSI. So I wouldn't say "they generate spurious > interrupts when switching between MSI and non-MSI." > > Why doesn't virtio-pci just register an INTx handler in addition to an MSI > handler? > > BjornThe handler causes an expensive exit to the hypervisor, and the INTx lines are shared with other devices. Seems silly to slow them down just so we can do something that triggers the device bug. The bus master is disabled by that time, if linux can just desist from touching MSI enable device won't send either INTx (because MSI is on) or MSI (because bus master is on) and all will be well. -- MST
Possibly Parallel Threads
- [PATCH v7] pci: quirk to skip msi disable on shutdown
- [PATCH v7] pci: quirk to skip msi disable on shutdown
- [PATCH v7] pci: quirk to skip msi disable on shutdown
- [PATCH v7] pci: quirk to skip msi disable on shutdown
- [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown