Patchwork xen/pci: Deal with toolstack missing an 'XenbusStateClosing'.

login
register
mail settings
Submitter Konrad Rzeszutek Wilk
Date June 10, 2013, 9:06 p.m.
Message ID <1370898399-20968-2-git-send-email-konrad.wilk@oracle.com>
Download mbox | patch
Permalink /patch/250371/
State Not Applicable
Headers show

Comments

Konrad Rzeszutek Wilk - June 10, 2013, 9:06 p.m.
There are two tool-stack that can instruct the Xen PCI frontend
and backend to change states: 'xm' (Python code with a daemon),
and 'xl' (C library - does not keep state changes).

With the 'xm', the path to disconnect a PCI device (xm pci-detach
<guest> <BDF>)is:

4(Connected)->7(Reconfiguring*)-> 8(Reconfigured)-> 4(Connected)->5(Closing*).

The * is for states that the tool-stack sets. For 'xl', it is similar:

4(Connected)->7(Reconfiguring*)-> 8(Reconfigured)-> 4(Connected)

Both of them also tear down the XenBus structure, so the backend
state ends up going in the 3(Initialised) and calls pcifront_xenbus_remove.

When a PCI device is plugged in (xm pci-attach <guest> <BDF>)
both of them follow the same pattern:
2(InitWait*), 3(Initialized*), 4(Connected*)->4(Connected).

[xen-pcifront ignores the 2,3 state changes and only acts when
4 (Connected) has been reached]

The problem is that git commit 3d925320e9e2de162bd138bf97816bda8c3f71be
("xen/pcifront: Use Xen-SWIOTLB when initting if required") introduced
a mechanism to initialize the SWIOTLB when the Xen PCI front moves to
Connected state. It also had some aggressive seatbelt code check that
would warn the user if one tried to change to Connected state without
hitting first the Closing state:

 pcifront pci-0: PCI frontend already installed!

However, that code can be relaxed and we can continue on working
even if the frontend is instructed to be the 'Connected' state with
no devices and then gets tickled to be in 'Connected' state again.

In other words, this 4(Connected)->5(Closing)->4(Connected) state
was expected, while 4(Connected)->.... anything but 5(Closing)->4(Connected)
was not. This patch removes that aggressive check and allows
Xen pcifront to work with the 'xl' toolstack.

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
Cc: stable@vger.kernel.org
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/pci/xen-pcifront.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
Jan Beulich - June 11, 2013, 7:29 a.m.
>>> On 10.06.13 at 23:06, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> There are two tool-stack that can instruct the Xen PCI frontend
> and backend to change states: 'xm' (Python code with a daemon),
> and 'xl' (C library - does not keep state changes).
> 
> With the 'xm', the path to disconnect a PCI device (xm pci-detach
> <guest> <BDF>)is:
> 
> 4(Connected)->7(Reconfiguring*)-> 8(Reconfigured)-> 4(Connected)->5(Closing*).
> 
> The * is for states that the tool-stack sets. For 'xl', it is similar:
> 
> 4(Connected)->7(Reconfiguring*)-> 8(Reconfigured)-> 4(Connected)
> 
> Both of them also tear down the XenBus structure, so the backend
> state ends up going in the 3(Initialised) and calls pcifront_xenbus_remove.
> 
> When a PCI device is plugged in (xm pci-attach <guest> <BDF>)
> both of them follow the same pattern:
> 2(InitWait*), 3(Initialized*), 4(Connected*)->4(Connected).
> 
> [xen-pcifront ignores the 2,3 state changes and only acts when
> 4 (Connected) has been reached]
> 
> The problem is that git commit 3d925320e9e2de162bd138bf97816bda8c3f71be
> ("xen/pcifront: Use Xen-SWIOTLB when initting if required") introduced
> a mechanism to initialize the SWIOTLB when the Xen PCI front moves to
> Connected state. It also had some aggressive seatbelt code check that
> would warn the user if one tried to change to Connected state without
> hitting first the Closing state:
> 
>  pcifront pci-0: PCI frontend already installed!
> 
> However, that code can be relaxed and we can continue on working
> even if the frontend is instructed to be the 'Connected' state with
> no devices and then gets tickled to be in 'Connected' state again.
> 
> In other words, this 4(Connected)->5(Closing)->4(Connected) state
> was expected, while 4(Connected)->.... anything but 5(Closing)->4(Connected)
> was not. This patch removes that aggressive check and allows
> Xen pcifront to work with the 'xl' toolstack.

I actually think this shouldn't be worked around here, but fixed in
xl. Any device removed from a guest should be driven towards
the "Closed" state.

Jan

> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: linux-pci@vger.kernel.org 
> Cc: stable@vger.kernel.org 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  drivers/pci/xen-pcifront.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
> index ac99515..cc46e253 100644
> --- a/drivers/pci/xen-pcifront.c
> +++ b/drivers/pci/xen-pcifront.c
> @@ -675,10 +675,9 @@ static int pcifront_connect_and_init_dma(struct 
> pcifront_device *pdev)
>  	if (!pcifront_dev) {
>  		dev_info(&pdev->xdev->dev, "Installing PCI frontend\n");
>  		pcifront_dev = pdev;
> -	} else {
> -		dev_err(&pdev->xdev->dev, "PCI frontend already installed!\n");
> +	} else
>  		err = -EEXIST;
> -	}
> +
>  	spin_unlock(&pcifront_dev_lock);
>  
>  	if (!err && !swiotlb_nr_tbl()) {
> @@ -846,7 +845,7 @@ static int pcifront_try_connect(struct pcifront_device 
> *pdev)
>  		goto out;
>  
>  	err = pcifront_connect_and_init_dma(pdev);
> -	if (err) {
> +	if (err && err != -EEXIST) {
>  		xenbus_dev_fatal(pdev->xdev, err,
>  				 "Error setting up PCI Frontend");
>  		goto out;
> -- 
> 1.8.1.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 



--
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
George Dunlap - June 11, 2013, 9 a.m.
On 06/11/2013 08:29 AM, Jan Beulich wrote:
>>>> On 10.06.13 at 23:06, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
>> There are two tool-stack that can instruct the Xen PCI frontend
>> and backend to change states: 'xm' (Python code with a daemon),
>> and 'xl' (C library - does not keep state changes).
>>
>> With the 'xm', the path to disconnect a PCI device (xm pci-detach
>> <guest> <BDF>)is:
>>
>> 4(Connected)->7(Reconfiguring*)-> 8(Reconfigured)-> 4(Connected)->5(Closing*).
>>
>> The * is for states that the tool-stack sets. For 'xl', it is similar:
>>
>> 4(Connected)->7(Reconfiguring*)-> 8(Reconfigured)-> 4(Connected)
>>
>> Both of them also tear down the XenBus structure, so the backend
>> state ends up going in the 3(Initialised) and calls pcifront_xenbus_remove.
>>
>> When a PCI device is plugged in (xm pci-attach <guest> <BDF>)
>> both of them follow the same pattern:
>> 2(InitWait*), 3(Initialized*), 4(Connected*)->4(Connected).
>>
>> [xen-pcifront ignores the 2,3 state changes and only acts when
>> 4 (Connected) has been reached]
>>
>> The problem is that git commit 3d925320e9e2de162bd138bf97816bda8c3f71be
>> ("xen/pcifront: Use Xen-SWIOTLB when initting if required") introduced
>> a mechanism to initialize the SWIOTLB when the Xen PCI front moves to
>> Connected state. It also had some aggressive seatbelt code check that
>> would warn the user if one tried to change to Connected state without
>> hitting first the Closing state:
>>
>>   pcifront pci-0: PCI frontend already installed!
>>
>> However, that code can be relaxed and we can continue on working
>> even if the frontend is instructed to be the 'Connected' state with
>> no devices and then gets tickled to be in 'Connected' state again.
>>
>> In other words, this 4(Connected)->5(Closing)->4(Connected) state
>> was expected, while 4(Connected)->.... anything but 5(Closing)->4(Connected)
>> was not. This patch removes that aggressive check and allows
>> Xen pcifront to work with the 'xl' toolstack.
>
> I actually think this shouldn't be worked around here, but fixed in
> xl. Any device removed from a guest should be driven towards
> the "Closed" state.

Yeah, that seems pretty obvious to me.  The weird thing is that this 
wasn't noticed before -- does this work in 4.2?  Have you been doing 
this test all along, or has it only broken recently?

I've reproduced it on one of my test boxes; let me see if I can sort it out.

  -George

--
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
Konrad Rzeszutek Wilk - June 11, 2013, 1:03 p.m.
On 6/11/2013 5:00 AM, George Dunlap wrote:
> On 06/11/2013 08:29 AM, Jan Beulich wrote:
>>>>> On 10.06.13 at 23:06, Konrad Rzeszutek Wilk 
>>>>> <konrad.wilk@oracle.com> wrote:
>>> There are two tool-stack that can instruct the Xen PCI frontend
>>> and backend to change states: 'xm' (Python code with a daemon),
>>> and 'xl' (C library - does not keep state changes).
>>>
>>> With the 'xm', the path to disconnect a PCI device (xm pci-detach
>>> <guest> <BDF>)is:
>>>
>>> 4(Connected)->7(Reconfiguring*)-> 8(Reconfigured)-> 
>>> 4(Connected)->5(Closing*).
>>>
>>> The * is for states that the tool-stack sets. For 'xl', it is similar:
>>>
>>> 4(Connected)->7(Reconfiguring*)-> 8(Reconfigured)-> 4(Connected)
>>>
>>> Both of them also tear down the XenBus structure, so the backend
>>> state ends up going in the 3(Initialised) and calls 
>>> pcifront_xenbus_remove.
>>>
>>> When a PCI device is plugged in (xm pci-attach <guest> <BDF>)
>>> both of them follow the same pattern:
>>> 2(InitWait*), 3(Initialized*), 4(Connected*)->4(Connected).
>>>
>>> [xen-pcifront ignores the 2,3 state changes and only acts when
>>> 4 (Connected) has been reached]
>>>
>>> The problem is that git commit 3d925320e9e2de162bd138bf97816bda8c3f71be
>>> ("xen/pcifront: Use Xen-SWIOTLB when initting if required") introduced
>>> a mechanism to initialize the SWIOTLB when the Xen PCI front moves to
>>> Connected state. It also had some aggressive seatbelt code check that
>>> would warn the user if one tried to change to Connected state without
>>> hitting first the Closing state:
>>>
>>>   pcifront pci-0: PCI frontend already installed!
>>>
>>> However, that code can be relaxed and we can continue on working
>>> even if the frontend is instructed to be the 'Connected' state with
>>> no devices and then gets tickled to be in 'Connected' state again.
>>>
>>> In other words, this 4(Connected)->5(Closing)->4(Connected) state
>>> was expected, while 4(Connected)->.... anything but 
>>> 5(Closing)->4(Connected)
>>> was not. This patch removes that aggressive check and allows
>>> Xen pcifront to work with the 'xl' toolstack.
>>
>> I actually think this shouldn't be worked around here, but fixed in
>> xl. Any device removed from a guest should be driven towards
>> the "Closed" state.

There is also the per-device state. Those are moved to the 5 (Closing), 
while the
whole connection is still in the 4(Connected) state. In essence all of 
the per-device states
are closed, it is just that the global state is still Connected.


>
> Yeah, that seems pretty obvious to me.  The weird thing is that this 
> wasn't noticed before -- does this work in 4.2?  Have you been doing 
> this test all along, or has it only broken recently?

I just reproduced this in Xen 4.2. I believe that the reason I did not 
see this before was b/c I was using 'xm'
primarily.
>
> I've reproduced it on one of my test boxes; let me see if I can sort 
> it out.

OK.
>
>  -George
>

--
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
George Dunlap - June 11, 2013, 3:36 p.m.
On 06/10/2013 10:06 PM, Konrad Rzeszutek Wilk wrote:
> There are two tool-stack that can instruct the Xen PCI frontend
> and backend to change states: 'xm' (Python code with a daemon),
> and 'xl' (C library - does not keep state changes).
>
> With the 'xm', the path to disconnect a PCI device (xm pci-detach
> <guest> <BDF>)is:
>
> 4(Connected)->7(Reconfiguring*)-> 8(Reconfigured)-> 4(Connected)->5(Closing*).
>
> The * is for states that the tool-stack sets. For 'xl', it is similar:
>
> 4(Connected)->7(Reconfiguring*)-> 8(Reconfigured)-> 4(Connected)
>
> Both of them also tear down the XenBus structure, so the backend
> state ends up going in the 3(Initialised) and calls pcifront_xenbus_remove.

So I looked a little bit into this; there are actually two different 
states that happen as part of this handshake.  In order to disonnect a 
*device*, xl signals using the *bus* state, like this:
* Wait for the *bus* to be in state 4(Connected)
* Set the *device* state to 5(Closing)
* Set the *bus* state to 7(Reconfiguring)
* Wait for the *bus* state to return to 4(Connected)

So are all of these states you see the *bus* state?  And why would you 
disconnect the whole pci bus if you're only removing one device?

  -George
--
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
Konrad Rzeszutek Wilk - June 11, 2013, 4:08 p.m.
On 6/11/2013 11:36 AM, George Dunlap wrote:
> On 06/10/2013 10:06 PM, Konrad Rzeszutek Wilk wrote:
>> There are two tool-stack that can instruct the Xen PCI frontend
>> and backend to change states: 'xm' (Python code with a daemon),
>> and 'xl' (C library - does not keep state changes).
>>
>> With the 'xm', the path to disconnect a PCI device (xm pci-detach
>> <guest> <BDF>)is:
>>
>> 4(Connected)->7(Reconfiguring*)-> 8(Reconfigured)-> 
>> 4(Connected)->5(Closing*).
>>
>> The * is for states that the tool-stack sets. For 'xl', it is similar:
>>
>> 4(Connected)->7(Reconfiguring*)-> 8(Reconfigured)-> 4(Connected)
>>
>> Both of them also tear down the XenBus structure, so the backend
>> state ends up going in the 3(Initialised) and calls 
>> pcifront_xenbus_remove.
>
> So I looked a little bit into this; there are actually two different 
> states that happen as part of this handshake.  In order to disonnect a 
> *device*, xl signals using the *bus* state, like this:
> * Wait for the *bus* to be in state 4(Connected)
> * Set the *device* state to 5(Closing)
> * Set the *bus* state to 7(Reconfiguring)
> * Wait for the *bus* state to return to 4(Connected)
>
> So are all of these states you see the *bus* state?  And why would you 
> disconnect the whole pci bus if you're only removing one device?

Correct. The stats I enumerated are *bus* states. Not per-device states.
I presume (and I hadn't checked xm) that Xend has some logic to only 
disconnect the bus if all of the PCI devices have been disconnected. In 
'xl' it does not do that.

The testing I did was just with one PCI device.
>
>  -George

--
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
George Dunlap - June 11, 2013, 4:17 p.m.
On 06/11/2013 05:08 PM, konrad wilk wrote:
>
> On 6/11/2013 11:36 AM, George Dunlap wrote:
>> On 06/10/2013 10:06 PM, Konrad Rzeszutek Wilk wrote:
>>> There are two tool-stack that can instruct the Xen PCI frontend
>>> and backend to change states: 'xm' (Python code with a daemon),
>>> and 'xl' (C library - does not keep state changes).
>>>
>>> With the 'xm', the path to disconnect a PCI device (xm pci-detach
>>> <guest> <BDF>)is:
>>>
>>> 4(Connected)->7(Reconfiguring*)-> 8(Reconfigured)->
>>> 4(Connected)->5(Closing*).
>>>
>>> The * is for states that the tool-stack sets. For 'xl', it is similar:
>>>
>>> 4(Connected)->7(Reconfiguring*)-> 8(Reconfigured)-> 4(Connected)
>>>
>>> Both of them also tear down the XenBus structure, so the backend
>>> state ends up going in the 3(Initialised) and calls
>>> pcifront_xenbus_remove.
>>
>> So I looked a little bit into this; there are actually two different
>> states that happen as part of this handshake.  In order to disonnect a
>> *device*, xl signals using the *bus* state, like this:
>> * Wait for the *bus* to be in state 4(Connected)
>> * Set the *device* state to 5(Closing)
>> * Set the *bus* state to 7(Reconfiguring)
>> * Wait for the *bus* state to return to 4(Connected)
>>
>> So are all of these states you see the *bus* state?  And why would you
>> disconnect the whole pci bus if you're only removing one device?
>
> Correct. The stats I enumerated are *bus* states. Not per-device states.
> I presume (and I hadn't checked xm) that Xend has some logic to only
> disconnect the bus if all of the PCI devices have been disconnected. In
> 'xl' it does not do that.
>
> The testing I did was just with one PCI device.

Ah, OK -- I see now.  The problem is that the code in the Linux side 
didn't know about the whole "4->7->8->4" thing to unplug a device.  In 
all likelihood, if you had used xm with two devices (so that the bus 
didn't get disconnected), then you would have run across the same error.

So at least part of the problem *is* a bug in Linux.

That doesn't explain why I have problems doing this on Debian's version 
of 3.2 -- unless the "fix" you mentoned above was backported to the 
stable kernel, perhaps?

  -George
--
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
Konrad Rzeszutek Wilk - June 11, 2013, 4:24 p.m.
On 6/11/2013 12:17 PM, George Dunlap wrote:
> On 06/11/2013 05:08 PM, konrad wilk wrote:
>>
>> On 6/11/2013 11:36 AM, George Dunlap wrote:
>>> On 06/10/2013 10:06 PM, Konrad Rzeszutek Wilk wrote:
>>>> There are two tool-stack that can instruct the Xen PCI frontend
>>>> and backend to change states: 'xm' (Python code with a daemon),
>>>> and 'xl' (C library - does not keep state changes).
>>>>
>>>> With the 'xm', the path to disconnect a PCI device (xm pci-detach
>>>> <guest> <BDF>)is:
>>>>
>>>> 4(Connected)->7(Reconfiguring*)-> 8(Reconfigured)->
>>>> 4(Connected)->5(Closing*).
>>>>
>>>> The * is for states that the tool-stack sets. For 'xl', it is similar:
>>>>
>>>> 4(Connected)->7(Reconfiguring*)-> 8(Reconfigured)-> 4(Connected)
>>>>
>>>> Both of them also tear down the XenBus structure, so the backend
>>>> state ends up going in the 3(Initialised) and calls
>>>> pcifront_xenbus_remove.
>>>
>>> So I looked a little bit into this; there are actually two different
>>> states that happen as part of this handshake.  In order to disonnect a
>>> *device*, xl signals using the *bus* state, like this:
>>> * Wait for the *bus* to be in state 4(Connected)
>>> * Set the *device* state to 5(Closing)
>>> * Set the *bus* state to 7(Reconfiguring)
>>> * Wait for the *bus* state to return to 4(Connected)
>>>
>>> So are all of these states you see the *bus* state?  And why would you
>>> disconnect the whole pci bus if you're only removing one device?
>>
>> Correct. The stats I enumerated are *bus* states. Not per-device states.
>> I presume (and I hadn't checked xm) that Xend has some logic to only
>> disconnect the bus if all of the PCI devices have been disconnected. In
>> 'xl' it does not do that.
>>
>> The testing I did was just with one PCI device.
>
> Ah, OK -- I see now.  The problem is that the code in the Linux side 
> didn't know about the whole "4->7->8->4" thing to unplug a device.  In 
> all likelihood, if you had used xm with two devices (so that the bus 
> didn't get disconnected), then you would have run across the same error.
>
> So at least part of the problem *is* a bug in Linux.

Right.
>
> That doesn't explain why I have problems doing this on Debian's 
> version of 3.2 -- unless the "fix" you mentoned above was backported 
> to the stable kernel, perhaps?
No. It was a feature.
>
>  -George

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

Patch

diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
index ac99515..cc46e253 100644
--- a/drivers/pci/xen-pcifront.c
+++ b/drivers/pci/xen-pcifront.c
@@ -675,10 +675,9 @@  static int pcifront_connect_and_init_dma(struct pcifront_device *pdev)
 	if (!pcifront_dev) {
 		dev_info(&pdev->xdev->dev, "Installing PCI frontend\n");
 		pcifront_dev = pdev;
-	} else {
-		dev_err(&pdev->xdev->dev, "PCI frontend already installed!\n");
+	} else
 		err = -EEXIST;
-	}
+
 	spin_unlock(&pcifront_dev_lock);
 
 	if (!err && !swiotlb_nr_tbl()) {
@@ -846,7 +845,7 @@  static int pcifront_try_connect(struct pcifront_device *pdev)
 		goto out;
 
 	err = pcifront_connect_and_init_dma(pdev);
-	if (err) {
+	if (err && err != -EEXIST) {
 		xenbus_dev_fatal(pdev->xdev, err,
 				 "Error setting up PCI Frontend");
 		goto out;