Xen project Mailing List

Re: [Xen-devel] [PATCH v5] Fixes to Xen pciback for 3.17.

Wednesday, August 6, 2014, 9:18:31 PM, you wrote: > On Wed, Aug 06, 2014 at 08:59:59PM +0200, Sander Eikelenboom wrote: >> >> Tuesday, August 5, 2014, 4:04:43 PM, you wrote: >> >> >> > Tuesday, August 5, 2014, 3:49:30 PM, you wrote: >> >> >> On Tue, Aug 05, 2014 at 11:44:33AM +0200, Sander Eikelenboom wrote: >> >>> >> >>> Tuesday, August 5, 2014, 11:31:08 AM, you wrote: >> >>> >> >>> > On 05/08/14 09:44, Sander Eikelenboom wrote: >> >>> >> >> >>> >> Monday, August 4, 2014, 8:43:18 PM, you wrote: >> >>> >> >> >>> >>> On Fri, Aug 01, 2014 at 04:30:05PM +0100, David Vrabel wrote: >> >>> >>>> On 14/07/14 17:18, Konrad Rzeszutek Wilk wrote: >> >>> >>>>> Greg: goto GHK >> >>> >>>>> >> >>> >>>>> This is v5 version of patches to fix some issues in Xen PCIback. >> >>> >>>> >> >>> >>>> Applied to devel/for-linus-3.17. >> >>> >> >> >>> >>> Thank you. >> >>> >>>> >> >>> >>>> I dropped the stable Cc for #2 pending a final decision on whether >> >>> >>>> it >> >>> >>>> really is a stable candidate. >> >>> >> >> >>> >>> OK. >> >>> >>>> >> >>> >>>> David >> >>> >> >> >>> >> Hi Konrad / David, >> >>> >> >> >>> >> This series still lacks a resolution on the sysfs /do_flr /reset, >> >>> >> as a result the pci devices are not reset after shutdown of a guest. >> >>> >> (no more pciback 0000:xx:xx.x: restoring config space at offset xxx) >> >>> >> >> >>> >> So this series now introduces a regression to 3.16, which causes >> >>> >> devices to malfunction >> >>> >> after a guest reboot or after assigning the devices to another guest. >> >>> >> >>> > I don't follow what you're saying. The lack of a device reset for PCI >> >>> > devices with no FLR method isn't a regression as this has never worked. >> >>> > Can you explain in more detail what the regression is and which patch >> >>> > caused it? >> >>> >> >>> I haven't bisected it to a specific patch in this series, >> >>> but this patch series (when pulled on top of 3.16) cause the following: >> >>> >> >>> - Do a system start and HVM guest start >> >>> - HVM guest with pci passthrough, devices work fine >> >>> - shutdown the HVM guest >> >>> - "pciback 0000:xx:xx.x: restoring config space at offset xxx" messages >> >>> do not >> >>> appear anymore when shutting down the HVM guest (as they do with >> >>> vanilla 3.16) >> >>> - Starting the HVM guest again with the same devices passed through. >> >>> - Devices malfunction (for example a USB host controller will fail a >> >>> simple >> >>> "lsusb" >> >>> - And this all works fine on vanilla 3.16. >> >> >> Hm, the only patch that makes code changes is >> >> 63fc5ec97cc54257d1c4ee49ed2131f754a5ff9b >> >> "xen/pciback: Don't deadlock when unbinding." >> >> but it does not change any of that code path. Only figures out whether >> >> to take a lock or not. >> >> > Ok and the do_flr nack by david is unrelated to this part (i didn't check >> > just >> > assumed there could be a connection) >> >> >> I will try it out on my box and see if I can reproduce it. >> >> >> And just to be 100% sure - you are using vanilla Xen? No changes on top >> >> of it? >> >> > Except the fix from jan for the pirq/msi stuff (and an unrelated hpet >> > one), other than that no. >> > If you can't reproduce i will see if i can dive deeper into it tonight ! >> >> Hi Konrad, >> >> It looks like the issues is this part of the change: >> >> --- a/drivers/xen/xen-pciback/pci_stub.c >> +++ b/drivers/xen/xen-pciback/pci_stub.c >> @@ -250,6 +250,8 @@ struct pci_dev *pcistub_get_pci_dev(struct >> xen_pcibk_device *pdev, >> * - 'echo BDF > unbind' with a guest still using it. See pcistub_remove >> * >> * As such we have to be careful. >> + * >> + * To make this easier, the caller has to hold the device lock. >> */ >> void pcistub_put_pci_dev(struct pci_dev *dev) >> { >> @@ -276,11 +278,8 @@ void pcistub_put_pci_dev(struct pci_dev *dev) >> /* Cleanup our device >> * (so it's ready for the next domain) >> */ >> - >> - /* This is OK - we are running from workqueue context >> - * and want to inhibit the user from fiddling with 'reset' >> - */ >> - pci_reset_function(dev); >> + lockdep_assert_held(&dev->dev.mutex); >> + __pci_reset_function_locked(dev); >> pci_restore_state(dev); >> /* This disables the device. */ >> >> More specifically: >> The old "pci_reset_function(dev)" potentially seems to do much more than >> __pci_reset_function_locked(dev). >> >> >> "__pci_reset_function_locked(dev)" only calls "__pci_dev_reset" >> while "pci_reset_function" not only calls pci_dev_reset, but on succes >> it also calls: "pci_dev_save_and_disable" which does a save state etc. >> >> >> So i added a little more debug: >> >> device_lock_assert(&dev->dev); >> ret = __pci_reset_function_locked(dev); >> dev_dbg(&dev->dev, "%s __pci_reset_function_locked:%d >> dev->state_saved:%d\n", __func__, ret, (!dev->state_saved) ? 0 : 1 ); >> pci_restore_state(dev); >> >> And this returns: >> [ 494.570579] pciback 0000:04:00.0: pcistub_put_pci_dev >> __pci_reset_function_locked:0 dev->state_saved:0 >> >> So that confirms there is no saved_state to get restored by >> pci_restore_state(dev) in the next line. >> >> However there seems to be no "locked" variant of the function >> "pci_reset_function" in pci.c that has all the same logic ... > Yup. I've a preliminary patch: Preliminary in the sense: "this should fix it .. needs more testing" ? > diff --git a/drivers/xen/xen-pciback/pci_stub.c > b/drivers/xen/xen-pciback/pci_stub.c > index 1ddd22f..4cb7901 100644 > --- a/drivers/xen/xen-pciback/pci_stub.c > +++ b/drivers/xen/xen-pciback/pci_stub.c > @@ -105,7 +105,7 @@ static void pcistub_device_release(struct kref *kref) > */ > __pci_reset_function_locked(dev); > if (pci_load_and_free_saved_state(dev, &dev_data->pci_saved_state)) > - dev_dbg(&dev->dev, "Could not reload PCI state\n"); > + dev_info(&dev->dev, "Could not reload PCI state\n"); > else > pci_restore_state(dev); > > @@ -257,6 +257,7 @@ void pcistub_put_pci_dev(struct pci_dev *dev) > { > struct pcistub_device *psdev, *found_psdev = NULL; > unsigned long flags; > + struct xen_pcibk_dev_data *dev_data; > > spin_lock_irqsave(&pcistub_devices_lock, flags); > > @@ -278,10 +279,25 @@ void pcistub_put_pci_dev(struct pci_dev *dev) > /* Cleanup our device > * (so it's ready for the next domain) > */ > - device_lock_assert(&dev->dev); > - __pci_reset_function_locked(dev); > - pci_restore_state(dev); > - > + if (pci_load_and_free_saved_state(dev, &dev_data->pci_saved_state)) > + dev_info(&dev->dev, "Could not reload PCI state\n"); > + else { > + device_lock_assert(&dev->dev); > + __pci_reset_function_locked(dev); > + /* > + * The usual sequence is pci_save_state & pci_restore_state > + * but the guest might have messed the config space up. Use > + * the initial configuration (when device was binded to us). > + */ > + pci_restore_state(dev); > + /* > + * The next steps are to reload the configuration for the > + * next time we need to unbind/bind to a guest.. > + */ > + dev_data = pci_get_drvdata(dev); > + pci_save_state(dev); > + dev_data->pci_saved_state = pci_store_saved_state(dev); > + } > /* This disables the device. */ > xen_pcibk_reset_device(dev); > >> >> -- >> Sander >> >> >> Thanks! >> >> >>> >> >>> >> Apart from that .. i can't resist to remind the other issue with >> >>> >> removing pci >> >>> >> devices passed through to HVM guests related to the signaling via >> >>> >> xenstore, >> >>> >> described in: >> >>> >> >> >>> >> http://lists.xen.org/archives/html/xen-devel/2014-07/msg01875.html >> >>> >> >>> > I don't remember seeing you posting a patch...? >> >> >> I was going to, but I think we need to figure out the 'do_flr' mechanism >> >> first. >> >> >>> >> >>> > David >> >>> >> >>> >> >> >> >> >> _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.