|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5] Fixes to Xen pciback for 3.17.
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" ?
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 ?)
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |