Re: [PATCH v7] pci: quirk to skip msi disable on shutdown

["Michael S. Tsirkin" <mst@redhat.com>]

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@redhat.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Yinghai Lu <yhlu.kernel.send@gmail.com>
> > Cc: Ulrich Obergfell <uobergfe@redhat.com>
> > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> > Signed-off-by: Michael S. Tsirkin <mst@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@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