Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
From: Alex Williamson <alex.williamson@redhat.com>
To: Andreas Hartmann <andihartmann@freenet.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
	linux-pci <linux-pci@vger.kernel.org>
Subject: Re: Hard and silent lock up since linux 3.14 with PCIe pass through (vfio)
Date: Thu, 30 Oct 2014 10:58:19 -0600
Message-ID: <1414688299.27420.292.camel@ul30vt.home> (raw)
In-Reply-To: <545268E8.3080107@maya.org>

On Thu, 2014-10-30 at 17:35 +0100, Andreas Hartmann wrote:
> Alex Williamson wrote:
> > On Wed, 2014-10-29 at 20:43 +0100, Andreas Hartmann wrote:
> [...]
> >> Therefore, I never should need pci_save_vc_state and
> >> pci_restore_vc_state. Thus, it should be ok to add "return" at the
> >> beginning of each of these function, true? Then it should work.
> >>
> >> I tested it. It worked.
> >>
> >> But if I'm removing only one of these returns either in
> >> pci_save_vc_state or pci_restore_vc_state, the machine hangs again.
> >>
> >> Therefore, there must be something odd going on in the for loops. Isn't
> >> it possible to add some useful debug code to these loops to see what's
> >> really going on? But the output *must* go to the actual console,
> >> otherwise I can't see it!
> >>
> >>
> >> int pci_save_vc_state(struct pci_dev *dev)
> >> {
> >>         return 0; // must be set
> >>         int i;
> >>
> >>         for (i = 0; i < ARRAY_SIZE(vc_caps); i++) {
> 		   // continue; -> works
> >>                 int pos, ret;
> >>                 struct pci_cap_saved_state *save_state;
> 		   // continue does not work!
> 
> --> Most probably the
> 
>             struct pci_cap_saved_state *save_state;
> 
>     makes the system hang!

We've done nothing more than declare variables there, there's no actual
code.  What happens if you increase the delay after bus reset, edit
drivers/pci/pci.c, find the call to ssleep(1) and change the 1 to a 2,
doubling the delay after reset.  It seems like VC save/restore is just a
scapegoat for the platform already being broken by the bus reset.  Also,
if you have any other card to test in this slot, it would be useful
comparison data to know if we're dealing with an endpoint issue or a bus
issue.

>   ARRAY_SIZE(vc_caps) is 3 and the whole
>     function is called 3 times when starting the vm.

Sounds right.  The array is declared right above these functions and has
entries for VC, VC9, and MFVC types.  VFIO will try to reset the device
when it's initially opened and then QEMU does it twice (for some
reason), so that makes 3.  Thanks,

Alex

> >>
> >>                 pos = pci_find_ext_capability(dev, vc_caps[i].id);
> >>                 if (!pos || !pci_vc_needs_save(dev, vc_caps[i].id))
> >>                         continue;
> > 
> > Take the next logical step, comment out the if here and we'll statically
> > take the continue.  Does it still fail?  If so, move the continue above
> > the call to pci_find_ext_capability(), if not...
> > 
> >>
> >>                 save_state = pci_find_saved_ext_cap(dev, vc_caps[i].id);
> > 
> > If not, add a continue; here.  Unless my pci_vc_needs_save() function is
> > broken, we shouldn't be getting here anyway.
> > 
> >>                 if (!save_state) {
> >>                         dev_err(&dev->dev, "%s buffer not found in %s\n",
> >>                                 vc_caps[i].name, __func__);
> >>                         return -ENOMEM;
> >>                 }
> >>
> >>                 printk("%s doing %s save on %s\n", __func__, vc_caps[i].name, pci_name(dev));
> >>                 ret = pci_vc_do_save_buffer(dev, pos, save_state, true);
> >>                 if (ret) {
> >>                         dev_err(&dev->dev, "%s save unsuccessful %s\n",
> >>                                 vc_caps[i].name, __func__);
> >>                         return ret;
> >>                 }
> >>         }
> >>
> >>         return 0;
> >> }
> >>
> >>
> >> void pci_restore_vc_state(struct pci_dev *dev)
> >> {
> >>         return; // must be set
> >>         int i;
> >>
> >>         for (i = 0; i < ARRAY_SIZE(vc_caps); i++) {
> >>                 int pos;
> >>                 struct pci_cap_saved_state *save_state;
> >>
> >>                 pos = pci_find_ext_capability(dev, vc_caps[i].id);
> >>                 save_state = pci_find_saved_ext_cap(dev, vc_caps[i].id);
> > 
> > This should never find a save_state with the pci_vc_needs_save() patch,
> > so we should always take the branch below.  Comment out the if (... and
> > leave the continue, does the behavior change?  If so, add a continue;
> > line above pci_find_saved_ext_cap(), does it work?  If not, add another
> > continue above pci_find_ext_capability().
> > 
> >>                 if (!save_state || !pos)
> >>                         continue;
> >>
> >>                 printk("%s doing %s restore on %s\n", __func__, vc_caps[i].name, pci_name(dev));
> >>                 pci_vc_do_save_buffer(dev, pos, save_state, false);
> >>         }
> >> }
> > 
> > In the "working" case with Bjorn's patch, are you actually trying to use
> > the device or just testing to see if the system survives reset?  You
> > might at least want to run lspci -xxxx on it after reset to make sure
> > it's really there.  Thanks,
> > 
> > Alex
> > 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html




  reply index

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-09-23 19:03 Andreas Hartmann
2014-09-23 20:07 ` Alex Williamson
2014-09-24 14:54   ` Andreas Hartmann
2014-09-24 17:16     ` Andreas Hartmann
2014-10-10  9:39   ` Andreas Hartmann
2014-10-10 14:37     ` Bjorn Helgaas
2014-10-10 14:49       ` Andreas Hartmann
2014-10-10 15:55         ` Bjorn Helgaas
2014-10-10 16:09           ` Andreas Hartmann
2014-10-10 16:41             ` Bjorn Helgaas
2014-10-10 22:32               ` Andreas Hartmann
2014-10-10 22:54                 ` Bjorn Helgaas
2014-10-11  6:20                   ` Andreas Hartmann
2014-10-15  8:04                     ` Alex Williamson
2014-10-17  1:04                       ` Andreas Hartmann
2014-10-21 21:06                         ` Alex Williamson
2014-10-21 21:32                           ` Alex Williamson
2014-10-22 16:22                             ` Andreas Hartmann
2014-10-22 20:36                               ` Alex Williamson
2014-10-23 16:00                                 ` Andreas Hartmann
2014-10-23 16:33                                   ` Alex Williamson
2014-10-23 17:12                                     ` Andreas Hartmann
2014-10-23 17:33                                     ` Andreas Hartmann
2014-10-23 19:37                                       ` Alex Williamson
2014-10-24 14:21                                         ` Andreas Hartmann
2014-10-25  6:03                                         ` Andreas Hartmann
2014-10-28 21:51                                           ` Alex Williamson
2014-10-29 16:47                                             ` Andreas Hartmann
2014-10-29 17:44                                               ` Alex Williamson
2014-10-29 17:57                                                 ` Andreas Hartmann
2014-10-29 18:16                                                   ` Alex Williamson
2014-10-29 19:43                                                     ` Andreas Hartmann
2014-10-29 20:50                                                       ` Alex Williamson
2014-10-29 21:35                                                         ` Andreas Hartmann
2014-10-30 16:35                                                         ` Andreas Hartmann
2014-10-30 16:58                                                           ` Alex Williamson [this message]
2014-10-30 19:09                                                             ` Andreas Hartmann
2014-10-30 19:45                                                               ` Alex Williamson
2014-10-30 20:21                                                                 ` Andreas Hartmann
2014-10-22 15:34                           ` Andreas Hartmann
2014-10-22 16:02                             ` Alex Williamson
2014-10-22 16:20                               ` Andreas Hartmann

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1414688299.27420.292.camel@ul30vt.home \
    --to=alex.williamson@redhat.com \
    --cc=andihartmann@freenet.de \
    --cc=bhelgaas@google.com \
    --cc=linux-pci@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \
		linux-pci@vger.kernel.org
	public-inbox-index linux-pci

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git