Message-ID: <131731634.20140807000831@eikelenboom.it>
Date:	Thu, 7 Aug 2014 00:08:31 +0200
From:	Sander Eikelenboom <linux@...elenboom.it>
To:	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
CC:	gregkh@...uxfoundation.org, boris.ostrovsky@...cle.com,
	<xen-devel@...ts.xenproject.org>,
	David Vrabel <david.vrabel@...rix.com>,
	<linux-kernel@...r.kernel.org>
Subject: 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@...cle.com>
>>> > 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@...elenboom.it>
>>> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
>>> > ---
>>> >  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@...elenboom.it>

Thanks for fixing Konrad !

--
Sander 





--
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