Xen project Mailing List

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

Wednesday, August 6, 2014, 10:17:19 PM, you wrote: > Wednesday, August 6, 2014, 10:09:59 PM, you wrote: >> On Wed, Aug 06, 2014 at 09:47:43PM +0200, Sander Eikelenboom wrote: >>> >>> Wednesday, August 6, 2014, 9:39:16 PM, you wrote: >>> >>> > On Wed, Aug 06, 2014 at 09:25:59PM +0200, Sander Eikelenboom wrote: >>> >> >>> >> 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" ? >>> >>> > This should fix it, albeit the fix has a disastrous flaw. Here is the >>> > proper version: >>> >>> >>> > From 00a5b6e3c9ee2c2d605879bdaebc627fa640b024 Mon Sep 17 00:00:00 2001 >>> > From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> >>> > Date: Wed, 6 Aug 2014 16:21:32 -0400 >>> > Subject: [PATCH] xen/pciback: Restore configuration space when detaching >>> > from >>> > a guest. >>> >>> > The commit 9eea3f7695226f9af9992cebf8e98ac0ad78b277 >>> > "xen/pciback: Don't deadlock when unbinding." was using >>> > the version of pci_reset_function which would lock the device lock. >>> > That is no good as we can dead-lock. As such we swapped to using >>> > the lock-less version and requiring that the callers >>> > of 'pcistub_put_pci_dev' take the device lock. And as such >>> > this bug got exposed. >>> >>> > Using the lock-less version is OK, except that we tried to >>> > use 'pci_restore_state' after the lock-less version of >>> > __pci_reset_function_locked - which won't work as 'state_saved' >>> > is set to false. Said 'state_saved' is a toggle boolean that >>> > is to be used by the sequence of a) pci_save_state/pci_restore_state >>> > or b) pci_load_and_free_saved_state/pci_restore_state. We don't >>> > want to use a) as the guest might have messed up the PCI >>> > configuration space and we want it to revert to the state >>> > when the PCI device was binded to us. Therefore we pick >>> > b) to restore the configuration space. >>> >>> > To still retain the PCI configuration space, we save it once >>> > more and store it on our private copy to be restored when: >>> > - Device is unbinded from pciback >>> > - Device is detached from a guest. >>> >>> > Reported-by: Sander Eikelenboom <linux@xxxxxxxxxxxxxx> >>> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> >>> > --- >>> > drivers/xen/xen-pciback/pci_stub.c | 25 +++++++++++++++++++++---- >>> > 1 files changed, 21 insertions(+), 4 deletions(-) >>> >>> > diff --git a/drivers/xen/xen-pciback/pci_stub.c >>> > b/drivers/xen/xen-pciback/pci_stub.c >>> > index 1ddd22f..8cf7f2b 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); >>> > >>> > @@ -279,9 +280,25 @@ void pcistub_put_pci_dev(struct pci_dev *dev) >>> > * (so it's ready for the next domain) >>> > */ >>> > device_lock_assert(&dev->dev); >>> > - __pci_reset_function_locked(dev); >>> > - pci_restore_state(dev); >>> > - >>> > + dev_data = pci_get_drvdata(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 { >>> > + __pci_reset_function_locked(dev); >>> > + /* >>> > + * The usual sequence is pci_save_state & >>> > pci_restore_state >>> > + * but the guest might have messed the configuration >>> > space up. >>> > + * Use the initial version (when device was binded to us). >>> > + */ >>> > + pci_restore_state(dev); >>> > + /* >>> > + * The next steps are to reload the configuration for the >>> > + * next time we bind & unbind to a guest - or unload from >>> > + * pciback. >>> > + */ >>> > + pci_save_state(dev); >>> > + dev_data->pci_saved_state = pci_store_saved_state(dev); >>> > + } >>> > /* This disables the device. */ >>> > xen_pcibk_reset_device(dev); >>> > >>> >>> >>> Is it save to have "__pci_reset_function_locked(dev)" to be conditional on >>> succes of >>> "pci_load_and_free_saved_state" ? >> It could be redone a bit differently - as in: >> rc = pci_load_and_free_saved_state(..); >> __pci_reset_function_locked(dev); >> if (!rc) { >> pci_restore_state(dev); >> ... >> In which case we will only do the restore state (and save state) when the >> device >> is in expected state. And the reset happens at that point. >>> >>> Or is it safer because you don't reset the device although it's in an >>> unknown >>> state (and resetting it while it's back to dom0 could lead to more problems >>> ?) >> It could very well lead to disaster. I am not exactly sure what the >> ramifications >> are with a device for which we cannot save PCI configuration space - aka - >> extremely >> borked. > If it would .. perhaps you even shouldn't pass it through / seize it, when > you can't save it. > And make it unassignable to other guests / rebindable to dom0 if restore > fails. > Compile is done .. lets test :-) If you like, you may stick on a: Tested-By: Sander Eikelenboom <linux@xxxxxxxxxxxxxx> Thanks for fixing Konrad ! -- Sander _______________________________________________ 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.