lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Thu, 17 Sep 2015 19:03:37 +0300
From:	"Michael S. Tsirkin" <mst@...hat.com>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	Bjorn Helgaas <bhelgaas@...gle.com>, linux-kernel@...r.kernel.org,
	Fam Zheng <famz@...hat.com>,
	Yinghai Lu <yhlu.kernel.send@...il.com>,
	Ulrich Obergfell <uobergfe@...hat.com>,
	Rusty Russell <rusty@...tcorp.com.au>,
	linux-pci@...r.kernel.org,
	virtualization@...ts.linux-foundation.org
Subject: Re: [PATCH v7] pci: quirk to skip msi disable on shutdown

On Thu, Sep 17, 2015 at 10:49:06AM -0500, Eric W. Biederman wrote:
> Bjorn Helgaas <bhelgaas@...gle.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@...hat.com>
> >> Cc: Bjorn Helgaas <bhelgaas@...gle.com>
> >> Cc: Yinghai Lu <yhlu.kernel.send@...il.com>
> >> Cc: Ulrich Obergfell <uobergfe@...hat.com>
> >> Cc: Rusty Russell <rusty@...tcorp.com.au>
> >> Cc: "Eric W. Biederman" <ebiederm@...ssion.com>
> >> Signed-off-by: Michael S. Tsirkin <mst@...hat.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.

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@...wei.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists