Re: [Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

2020-10-19 Thread boris . ostrovsky 


On 10/19/20 6:43 PM, Rich Persaud wrote:
> On Oct 19, 2020, at 11:52, Håkon Alstadheim  wrote:
>> 
>> Den 19.10.2020 17:16, skrev Håkon Alstadheim:
>>> Den 19.10.2020 13:00, skrev George Dunlap:
> On Jan 31, 2020, at 3:33 PM, Wei Liu  wrote:
>
> On Fri, Jan 17, 2020 at 02:13:04PM -0500, Rich Persaud wrote:
>> On Aug 26, 2019, at 17:08, Pasi Kärkkäinen  wrote:
>>> Hi,
>>>
 On Mon, Oct 08, 2018 at 10:32:45AM -0400, Boris Ostrovsky wrote:
> On 10/3/18 11:51 AM, Pasi Kärkkäinen wrote:
> On Wed, Sep 19, 2018 at 11:05:26AM +0200, Roger Pau Monné wrote:
>> On Tue, Sep 18, 2018 at 02:09:53PM -0400, Boris Ostrovsky wrote:
>>> On 9/18/18 5:32 AM, George Dunlap wrote:
> On Sep 18, 2018, at 8:15 AM, Pasi Kärkkäinen  wrote:
> Hi,
>> On Mon, Sep 17, 2018 at 02:06:02PM -0400, Boris Ostrovsky wrote:
>>> What about the toolstack changes? Have they been accepted? I 
>>> vaguely
>>> recall there was a discussion about those changes but don't 
>>> remember how
>>> it ended.
>> I don't think toolstack/libxl patch has been applied yet either.
>> "[PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS 
>> attribute":
>> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html
>>  
>> "[PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' SysFS 
>> attribute":
>> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html
>>  
 Will this patch work for *BSD? Roger?
>>> At least FreeBSD don't support pci-passthrough, so none of this 
>>> works
>>> ATM. There's no sysfs on BSD, so much of what's in libxl_pci.c will
>>> have to be moved to libxl_linux.c when BSD support is added.
>> Ok. That sounds like it's OK for the initial pci 'reset' 
>> implementation in xl/libxl to be linux-only..
> Are these two patches still needed? ISTR they were written originally
> to deal with guest trying to use device that was previously assigned 
> to
> another guest. But pcistub_put_pci_dev() calls
> __pci_reset_function_locked() which first tries FLR, and it looks like
> it was added relatively recently.
 Replying to an old thread.. I only now realized I forgot to reply to 
 this message earlier.

 afaik these patches are still needed. Håkon (CC'd) wrote to me in 
 private that
 he gets a (dom0) Linux kernel crash if he doesn't have these patches 
 applied.


If this is still crashing I'd be curious to see the splat.




 Here are the links to both the linux kernel and libxl patches:


 "[Xen-devel] [PATCH V3 0/2] Xen/PCIback: PCI reset using 'reset' SysFS 
 attribute":
 https://lists.xen.org/archives/html/xen-devel/2017-12/msg00659.html

 [Note that PATCH V3 1/2 "Drivers/PCI: Export pcie_has_flr() interface" 
 is already applied in upstream linux kernel, so it's not needed 
 anymore]

 "[Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus 
 reset with 'reset' SysFS attribute":
 https://lists.xen.org/archives/html/xen-devel/2017-12/msg00661.html


 "[Xen-devel] [PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS 
 attribute":
 https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html

 "[Xen-devel] [PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' 
 SysFS attribute":
 https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html
>>> [dropping Linux mailing lists]
>>>
>>> What is required to get the Xen patches merged?  Rebasing against Xen
>>> master?  OpenXT has been carrying a similar patch for many years and
>>> we would like to move to an upstream implementation.  Xen users of PCI
>>> passthrough would benefit from more reliable device reset.
>> Rebase and resend?


If you are is going to resend then please address Jan's comments from 
https://lists.xen.org/archives/html/xen-devel/2017-12/msg00695.html (although I 
am not sure in usefulness of the last one)


>>
>> Skimming that thread I think the major concern was backward
>> compatibility. That seemed to have been addressed.
>>
>> Unfortunately I don't have the time to dig into Linux to see if the
>> claim there is true or not.
>>
>> It would be helpful to write a concise paragraph to say why backward
>> compatibility is not required.
 Just going through my old “make sure something happens with this” mails.  
 Did anything ever happen with this?  Who has the ball here / who is this 
 stuck on?
>>> We're waiting for "somebody" to

Re: [Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

2020-10-19 Thread Rich Persaud 
On Oct 19, 2020, at 11:52, Håkon Alstadheim  wrote:
> 
> 
> Den 19.10.2020 17:16, skrev Håkon Alstadheim:
>> Den 19.10.2020 13:00, skrev George Dunlap:
>>> 
 On Jan 31, 2020, at 3:33 PM, Wei Liu  wrote:
 
 On Fri, Jan 17, 2020 at 02:13:04PM -0500, Rich Persaud wrote:
> On Aug 26, 2019, at 17:08, Pasi Kärkkäinen  wrote:
>> Hi,
>> 
>>> On Mon, Oct 08, 2018 at 10:32:45AM -0400, Boris Ostrovsky wrote:
 On 10/3/18 11:51 AM, Pasi Kärkkäinen wrote:
 On Wed, Sep 19, 2018 at 11:05:26AM +0200, Roger Pau Monné wrote:
> On Tue, Sep 18, 2018 at 02:09:53PM -0400, Boris Ostrovsky wrote:
>> On 9/18/18 5:32 AM, George Dunlap wrote:
 On Sep 18, 2018, at 8:15 AM, Pasi Kärkkäinen  wrote:
 Hi,
> On Mon, Sep 17, 2018 at 02:06:02PM -0400, Boris Ostrovsky wrote:
>> What about the toolstack changes? Have they been accepted? I 
>> vaguely
>> recall there was a discussion about those changes but don't 
>> remember how
>> it ended.
> I don't think toolstack/libxl patch has been applied yet either.
> "[PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS 
> attribute":
> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html
>  
> "[PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' SysFS 
> attribute":
> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html
>  
>>> Will this patch work for *BSD? Roger?
>> At least FreeBSD don't support pci-passthrough, so none of this works
>> ATM. There's no sysfs on BSD, so much of what's in libxl_pci.c will
>> have to be moved to libxl_linux.c when BSD support is added.
> Ok. That sounds like it's OK for the initial pci 'reset' 
> implementation in xl/libxl to be linux-only..
 Are these two patches still needed? ISTR they were written originally
 to deal with guest trying to use device that was previously assigned to
 another guest. But pcistub_put_pci_dev() calls
 __pci_reset_function_locked() which first tries FLR, and it looks like
 it was added relatively recently.
>>> Replying to an old thread.. I only now realized I forgot to reply to 
>>> this message earlier.
>>> 
>>> afaik these patches are still needed. Håkon (CC'd) wrote to me in 
>>> private that
>>> he gets a (dom0) Linux kernel crash if he doesn't have these patches 
>>> applied.
>>> 
>>> 
>>> Here are the links to both the linux kernel and libxl patches:
>>> 
>>> 
>>> "[Xen-devel] [PATCH V3 0/2] Xen/PCIback: PCI reset using 'reset' SysFS 
>>> attribute":
>>> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00659.html
>>> 
>>> [Note that PATCH V3 1/2 "Drivers/PCI: Export pcie_has_flr() interface" 
>>> is already applied in upstream linux kernel, so it's not needed anymore]
>>> 
>>> "[Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus 
>>> reset with 'reset' SysFS attribute":
>>> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00661.html
>>> 
>>> 
>>> "[Xen-devel] [PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS 
>>> attribute":
>>> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html
>>> 
>>> "[Xen-devel] [PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' 
>>> SysFS attribute":
>>> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html
>> [dropping Linux mailing lists]
>> 
>> What is required to get the Xen patches merged?  Rebasing against Xen
>> master?  OpenXT has been carrying a similar patch for many years and
>> we would like to move to an upstream implementation.  Xen users of PCI
>> passthrough would benefit from more reliable device reset.
> Rebase and resend?
> 
> Skimming that thread I think the major concern was backward
> compatibility. That seemed to have been addressed.
> 
> Unfortunately I don't have the time to dig into Linux to see if the
> claim there is true or not.
> 
> It would be helpful to write a concise paragraph to say why backward
> compatibility is not required.
>>> Just going through my old “make sure something happens with this” mails.  
>>> Did anything ever happen with this?  Who has the ball here / who is this 
>>> stuck on?
>> 
>> We're waiting for "somebody" to testify that fixing this will not adversely 
>> affect anyone. I'm not qualified, but my strong belief is that since "reset" 
>> or "do_flr"  in the linux kernel is not currently implemented/used in any 
>> official distribution, it should be OK.
>> 
>> Patches still work in current staging-4.14 btw.
>> 
> Just for the record, attached are the patches I am running on top of linux 
>

Re: [Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

2020-10-19 Thread Håkon Alstadheim 


Den 19.10.2020 17:16, skrev Håkon Alstadheim:

Den 19.10.2020 13:00, skrev George Dunlap:



On Jan 31, 2020, at 3:33 PM, Wei Liu  wrote:

On Fri, Jan 17, 2020 at 02:13:04PM -0500, Rich Persaud wrote:

On Aug 26, 2019, at 17:08, Pasi Kärkkäinen  wrote:

Hi,


On Mon, Oct 08, 2018 at 10:32:45AM -0400, Boris Ostrovsky wrote:

On 10/3/18 11:51 AM, Pasi Kärkkäinen wrote:
On Wed, Sep 19, 2018 at 11:05:26AM +0200, Roger Pau Monné wrote:

On Tue, Sep 18, 2018 at 02:09:53PM -0400, Boris Ostrovsky wrote:

On 9/18/18 5:32 AM, George Dunlap wrote:
On Sep 18, 2018, at 8:15 AM, Pasi Kärkkäinen  
wrote:

Hi,
On Mon, Sep 17, 2018 at 02:06:02PM -0400, Boris Ostrovsky 
wrote:
What about the toolstack changes? Have they been accepted? 
I vaguely
recall there was a discussion about those changes but don't 
remember how

it ended.
I don't think toolstack/libxl patch has been applied yet 
either.
"[PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS 
attribute":
https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html 

"[PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' 
SysFS attribute":
https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html 


Will this patch work for *BSD? Roger?
At least FreeBSD don't support pci-passthrough, so none of this 
works
ATM. There's no sysfs on BSD, so much of what's in libxl_pci.c 
will

have to be moved to libxl_linux.c when BSD support is added.
Ok. That sounds like it's OK for the initial pci 'reset' 
implementation in xl/libxl to be linux-only..
Are these two patches still needed? ISTR they were written 
originally
to deal with guest trying to use device that was previously 
assigned to

another guest. But pcistub_put_pci_dev() calls
__pci_reset_function_locked() which first tries FLR, and it looks 
like

it was added relatively recently.
Replying to an old thread.. I only now realized I forgot to reply 
to this message earlier.


afaik these patches are still needed. Håkon (CC'd) wrote to me in 
private that
he gets a (dom0) Linux kernel crash if he doesn't have these 
patches applied.



Here are the links to both the linux kernel and libxl patches:


"[Xen-devel] [PATCH V3 0/2] Xen/PCIback: PCI reset using 'reset' 
SysFS attribute":

https://lists.xen.org/archives/html/xen-devel/2017-12/msg00659.html

[Note that PATCH V3 1/2 "Drivers/PCI: Export pcie_has_flr() 
interface" is already applied in upstream linux kernel, so it's 
not needed anymore]


"[Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI 
flr/slot/bus reset with 'reset' SysFS attribute":

https://lists.xen.org/archives/html/xen-devel/2017-12/msg00661.html


"[Xen-devel] [PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' 
SysFS attribute":

https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html

"[Xen-devel] [PATCH V1 1/1] Xen/libxl: Perform PCI reset using 
'reset' SysFS attribute":

https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html

[dropping Linux mailing lists]

What is required to get the Xen patches merged?  Rebasing against Xen
master?  OpenXT has been carrying a similar patch for many years and
we would like to move to an upstream implementation.  Xen users of PCI
passthrough would benefit from more reliable device reset.

Rebase and resend?

Skimming that thread I think the major concern was backward
compatibility. That seemed to have been addressed.

Unfortunately I don't have the time to dig into Linux to see if the
claim there is true or not.

It would be helpful to write a concise paragraph to say why backward
compatibility is not required.
Just going through my old “make sure something happens with this” 
mails.  Did anything ever happen with this?  Who has the ball here / 
who is this stuck on?


We're waiting for "somebody" to testify that fixing this will not 
adversely affect anyone. I'm not qualified, but my strong belief is 
that since "reset" or "do_flr"  in the linux kernel is not currently 
implemented/used in any official distribution, it should be OK.


Patches still work in current staging-4.14 btw.

Just for the record, attached are the patches I am running on top of 
linux gentoo-sources-5.9.1  and xen-staging-4.14 respectively. (I am 
also running with the patch to mark populated reserved memory that 
contains ACPI tables as "ACPI NVS", not attached here ).


--- a/drivers/xen/xen-pciback/pci_stub.c	2020-03-30 21:08:39.406994339 +0200
+++ b/drivers/xen/xen-pciback/pci_stub.c	2020-03-30 20:56:18.225810279 +0200
@@ -245,6 +245,90 @@
 	return found_dev;
 }
 
+struct pcistub_args {
+	struct pci_dev *dev;
+	unsigned int dcount;
+};
+
+static int pcistub_search_dev(struct pci_dev *dev, void *data)
+{
+	struct pcistub_device *psdev;
+	struct pcistub_args *arg = data;
+	bool found_dev = false;
+	unsigned long flags;
+
+	spin_lock_irqsave(_devices_lock, flags);
+
+	list_for_each_entry(psdev, _devices, dev_list) {
+		if (psdev->dev == dev) {
+			found_dev = true;
+			arg->dcount++;
+			break;
+		}
+	}
+
+

Re: [Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

2020-10-19 Thread Håkon Alstadheim 

Den 19.10.2020 13:00, skrev George Dunlap:



On Jan 31, 2020, at 3:33 PM, Wei Liu  wrote:

On Fri, Jan 17, 2020 at 02:13:04PM -0500, Rich Persaud wrote:

On Aug 26, 2019, at 17:08, Pasi Kärkkäinen  wrote:

Hi,


On Mon, Oct 08, 2018 at 10:32:45AM -0400, Boris Ostrovsky wrote:

On 10/3/18 11:51 AM, Pasi Kärkkäinen wrote:
On Wed, Sep 19, 2018 at 11:05:26AM +0200, Roger Pau Monné wrote:

On Tue, Sep 18, 2018 at 02:09:53PM -0400, Boris Ostrovsky wrote:

On 9/18/18 5:32 AM, George Dunlap wrote:

On Sep 18, 2018, at 8:15 AM, Pasi Kärkkäinen  wrote:
Hi,
On Mon, Sep 17, 2018 at 02:06:02PM -0400, Boris Ostrovsky wrote:

What about the toolstack changes? Have they been accepted? I vaguely
recall there was a discussion about those changes but don't remember how
it ended.

I don't think toolstack/libxl patch has been applied yet either.
"[PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS attribute":
https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html
"[PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' SysFS attribute":
https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html

Will this patch work for *BSD? Roger?

At least FreeBSD don't support pci-passthrough, so none of this works
ATM. There's no sysfs on BSD, so much of what's in libxl_pci.c will
have to be moved to libxl_linux.c when BSD support is added.

Ok. That sounds like it's OK for the initial pci 'reset' implementation in 
xl/libxl to be linux-only..

Are these two patches still needed? ISTR they were  written originally
to deal with guest trying to use device that was previously assigned to
another guest. But pcistub_put_pci_dev() calls
__pci_reset_function_locked() which first tries FLR, and it looks like
it was added relatively recently.

Replying to an old thread.. I only now realized I forgot to reply to this 
message earlier.

afaik these patches are still needed. Håkon (CC'd) wrote to me in private that
he gets a (dom0) Linux kernel crash if he doesn't have these patches applied.


Here are the links to both the linux kernel and libxl patches:


"[Xen-devel] [PATCH V3 0/2] Xen/PCIback: PCI reset using 'reset' SysFS 
attribute":
https://lists.xen.org/archives/html/xen-devel/2017-12/msg00659.html

[Note that PATCH V3 1/2 "Drivers/PCI: Export pcie_has_flr() interface" is 
already applied in upstream linux kernel, so it's not needed anymore]

"[Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 
'reset' SysFS attribute":
https://lists.xen.org/archives/html/xen-devel/2017-12/msg00661.html


"[Xen-devel] [PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS attribute":
https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html

"[Xen-devel] [PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' SysFS 
attribute":
https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html

[dropping Linux mailing lists]

What is required to get the Xen patches merged?  Rebasing against Xen
master?  OpenXT has been carrying a similar patch for many years and
we would like to move to an upstream implementation.  Xen users of PCI
passthrough would benefit from more reliable device reset.

Rebase and resend?

Skimming that thread I think the major concern was backward
compatibility. That seemed to have been addressed.

Unfortunately I don't have the time to dig into Linux to see if the
claim there is true or not.

It would be helpful to write a concise paragraph to say why backward
compatibility is not required.

Just going through my old “make sure something happens with this” mails.  Did 
anything ever happen with this?  Who has the ball here / who is this stuck on?


We're waiting for "somebody" to testify that fixing this will not 
adversely affect anyone. I'm not qualified, but my strong belief is that 
since "reset" or "do_flr"  in the linux kernel is not currently 
implemented/used in any official distribution, it should be OK.


Patches still work in current staging-4.14 btw.

Re: [Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

2020-10-19 Thread George Dunlap 


> On Jan 31, 2020, at 3:33 PM, Wei Liu  wrote:
> 
> On Fri, Jan 17, 2020 at 02:13:04PM -0500, Rich Persaud wrote:
>> On Aug 26, 2019, at 17:08, Pasi Kärkkäinen  wrote:
>>> Hi,
>>> 
 On Mon, Oct 08, 2018 at 10:32:45AM -0400, Boris Ostrovsky wrote:
> On 10/3/18 11:51 AM, Pasi Kärkkäinen wrote:
> On Wed, Sep 19, 2018 at 11:05:26AM +0200, Roger Pau Monné wrote:
>> On Tue, Sep 18, 2018 at 02:09:53PM -0400, Boris Ostrovsky wrote:
>>> On 9/18/18 5:32 AM, George Dunlap wrote:
> On Sep 18, 2018, at 8:15 AM, Pasi Kärkkäinen  wrote:
> Hi,
> On Mon, Sep 17, 2018 at 02:06:02PM -0400, Boris Ostrovsky wrote:
>> What about the toolstack changes? Have they been accepted? I vaguely
>> recall there was a discussion about those changes but don't remember 
>> how
>> it ended.
> I don't think toolstack/libxl patch has been applied yet either.
> "[PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS attribute":
> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html
> "[PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' SysFS 
> attribute":
> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html
>>> Will this patch work for *BSD? Roger?
>> At least FreeBSD don't support pci-passthrough, so none of this works
>> ATM. There's no sysfs on BSD, so much of what's in libxl_pci.c will
>> have to be moved to libxl_linux.c when BSD support is added.
> Ok. That sounds like it's OK for the initial pci 'reset' implementation 
> in xl/libxl to be linux-only..
 
 Are these two patches still needed? ISTR they were  written originally
 to deal with guest trying to use device that was previously assigned to
 another guest. But pcistub_put_pci_dev() calls
 __pci_reset_function_locked() which first tries FLR, and it looks like
 it was added relatively recently.
>>> 
>>> Replying to an old thread.. I only now realized I forgot to reply to this 
>>> message earlier.
>>> 
>>> afaik these patches are still needed. Håkon (CC'd) wrote to me in private 
>>> that
>>> he gets a (dom0) Linux kernel crash if he doesn't have these patches 
>>> applied.
>>> 
>>> 
>>> Here are the links to both the linux kernel and libxl patches:
>>> 
>>> 
>>> "[Xen-devel] [PATCH V3 0/2] Xen/PCIback: PCI reset using 'reset' SysFS 
>>> attribute":
>>> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00659.html
>>> 
>>> [Note that PATCH V3 1/2 "Drivers/PCI: Export pcie_has_flr() interface" is 
>>> already applied in upstream linux kernel, so it's not needed anymore]
>>> 
>>> "[Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset 
>>> with 'reset' SysFS attribute":
>>> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00661.html
>>> 
>>> 
>>> "[Xen-devel] [PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS 
>>> attribute":
>>> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html
>>> 
>>> "[Xen-devel] [PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' 
>>> SysFS attribute":
>>> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html
>> 
>> [dropping Linux mailing lists]
>> 
>> What is required to get the Xen patches merged?  Rebasing against Xen
>> master?  OpenXT has been carrying a similar patch for many years and
>> we would like to move to an upstream implementation.  Xen users of PCI
>> passthrough would benefit from more reliable device reset.
> 
> Rebase and resend?
> 
> Skimming that thread I think the major concern was backward
> compatibility. That seemed to have been addressed.
> 
> Unfortunately I don't have the time to dig into Linux to see if the
> claim there is true or not.
> 
> It would be helpful to write a concise paragraph to say why backward
> compatibility is not required.

Just going through my old “make sure something happens with this” mails.  Did 
anything ever happen with this?  Who has the ball here / who is this stuck on?

 -George

Re: [Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

2020-01-31 Thread Wei Liu 
On Fri, Jan 17, 2020 at 02:13:04PM -0500, Rich Persaud wrote:
> On Aug 26, 2019, at 17:08, Pasi Kärkkäinen  wrote:
> > Hi,
> > 
> >> On Mon, Oct 08, 2018 at 10:32:45AM -0400, Boris Ostrovsky wrote:
> >>> On 10/3/18 11:51 AM, Pasi Kärkkäinen wrote:
> >>> On Wed, Sep 19, 2018 at 11:05:26AM +0200, Roger Pau Monné wrote:
>  On Tue, Sep 18, 2018 at 02:09:53PM -0400, Boris Ostrovsky wrote:
> > On 9/18/18 5:32 AM, George Dunlap wrote:
> >>> On Sep 18, 2018, at 8:15 AM, Pasi Kärkkäinen  wrote:
> >>> Hi,
> >>> On Mon, Sep 17, 2018 at 02:06:02PM -0400, Boris Ostrovsky wrote:
>  What about the toolstack changes? Have they been accepted? I vaguely
>  recall there was a discussion about those changes but don't remember 
>  how
>  it ended.
> >>> I don't think toolstack/libxl patch has been applied yet either.
> >>> "[PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS attribute":
> >>> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html
> >>> "[PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' SysFS 
> >>> attribute":
> >>> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html
> > Will this patch work for *BSD? Roger?
>  At least FreeBSD don't support pci-passthrough, so none of this works
>  ATM. There's no sysfs on BSD, so much of what's in libxl_pci.c will
>  have to be moved to libxl_linux.c when BSD support is added.
> >>> Ok. That sounds like it's OK for the initial pci 'reset' implementation 
> >>> in xl/libxl to be linux-only..
> >> 
> >> Are these two patches still needed? ISTR they were  written originally
> >> to deal with guest trying to use device that was previously assigned to
> >> another guest. But pcistub_put_pci_dev() calls
> >> __pci_reset_function_locked() which first tries FLR, and it looks like
> >> it was added relatively recently.
> > 
> > Replying to an old thread.. I only now realized I forgot to reply to this 
> > message earlier.
> > 
> > afaik these patches are still needed. Håkon (CC'd) wrote to me in private 
> > that
> > he gets a (dom0) Linux kernel crash if he doesn't have these patches 
> > applied.
> > 
> > 
> > Here are the links to both the linux kernel and libxl patches:
> > 
> > 
> > "[Xen-devel] [PATCH V3 0/2] Xen/PCIback: PCI reset using 'reset' SysFS 
> > attribute":
> > https://lists.xen.org/archives/html/xen-devel/2017-12/msg00659.html
> > 
> > [Note that PATCH V3 1/2 "Drivers/PCI: Export pcie_has_flr() interface" is 
> > already applied in upstream linux kernel, so it's not needed anymore]
> > 
> > "[Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset 
> > with 'reset' SysFS attribute":
> > https://lists.xen.org/archives/html/xen-devel/2017-12/msg00661.html
> > 
> > 
> > "[Xen-devel] [PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS 
> > attribute":
> > https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html
> > 
> > "[Xen-devel] [PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' 
> > SysFS attribute":
> > https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html
> 
> [dropping Linux mailing lists]
> 
> What is required to get the Xen patches merged?  Rebasing against Xen
> master?  OpenXT has been carrying a similar patch for many years and
> we would like to move to an upstream implementation.  Xen users of PCI
> passthrough would benefit from more reliable device reset.

Rebase and resend?

Skimming that thread I think the major concern was backward
compatibility. That seemed to have been addressed.

Unfortunately I don't have the time to dig into Linux to see if the
claim there is true or not.

It would be helpful to write a concise paragraph to say why backward
compatibility is not required.

Wei.

> 
>   2017 thread, including OpenXT patch: https://lists.gt.net/xen/devel/492945
>   2017-2019 thread: https://lists.gt.net/xen/devel/532648
> 
> Rich

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

2020-01-23 Thread Pasi Kärkkäinen 
Hi,

On Fri, Jan 17, 2020 at 02:13:04PM -0500, Rich Persaud wrote:
>On Aug 26, 2019, at 17:08, Pasi Kärkkäinen  wrote:
> 
>  Hi,
>  On Mon, Oct 08, 2018 at 10:32:45AM -0400, Boris Ostrovsky wrote:
> 
>On 10/3/18 11:51 AM, Pasi Kärkkäinen wrote:
> 
>  On Wed, Sep 19, 2018 at 11:05:26AM +0200, Roger Pau Monné wrote:
> 
>On Tue, Sep 18, 2018 at 02:09:53PM -0400, Boris Ostrovsky wrote:
> 
>  On 9/18/18 5:32 AM, George Dunlap wrote:
> 
>  On Sep 18, 2018, at 8:15 AM, Pasi Kärkkäinen 
>  wrote:
> 
>  Hi,
> 
>  On Mon, Sep 17, 2018 at 02:06:02PM -0400, Boris Ostrovsky
>  wrote:
> 
>What about the toolstack changes? Have they been accepted?
>I vaguely
> 
>recall there was a discussion about those changes but
>don't remember how
> 
>it ended.
> 
>  I don't think toolstack/libxl patch has been applied yet
>  either.
> 
>  "[PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS
>  attribute":
> 
>  
> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html
> 
>  "[PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset'
>  SysFS attribute":
> 
>  
> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html
> 
>  Will this patch work for *BSD? Roger?
> 
>At least FreeBSD don't support pci-passthrough, so none of this
>works
> 
>ATM. There's no sysfs on BSD, so much of what's in libxl_pci.c
>will
> 
>have to be moved to libxl_linux.c when BSD support is added.
> 
>  Ok. That sounds like it's OK for the initial pci 'reset'
>  implementation in xl/libxl to be linux-only..
> 
>Are these two patches still needed? ISTR they were  written originally
> 
>to deal with guest trying to use device that was previously assigned
>to
> 
>another guest. But pcistub_put_pci_dev() calls
> 
>__pci_reset_function_locked() which first tries FLR, and it looks like
> 
>it was added relatively recently.
> 
>  Replying to an old thread.. I only now realized I forgot to reply to
>  this message earlier.
>  afaik these patches are still needed. Håkon (CC'd) wrote to me in
>  private that
>  he gets a (dom0) Linux kernel crash if he doesn't have these patches
>  applied.
>  Here are the links to both the linux kernel and libxl patches:
>  "[Xen-devel] [PATCH V3 0/2] Xen/PCIback: PCI reset using 'reset' SysFS
>  attribute":
>  https://lists.xen.org/archives/html/xen-devel/2017-12/msg00659.html
>  [Note that PATCH V3 1/2 "Drivers/PCI: Export pcie_has_flr() interface"
>  is already applied in upstream linux kernel, so it's not needed anymore]
>  "[Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus
>  reset with 'reset' SysFS attribute":
>  https://lists.xen.org/archives/html/xen-devel/2017-12/msg00661.html
>  "[Xen-devel] [PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS
>  attribute":
>  https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html
>  "[Xen-devel] [PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset'
>  SysFS attribute":
>  https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html
> 
>[dropping Linux mailing lists]
>What is required to get the Xen patches merged?  Rebasing against Xen
>master?  OpenXT has been carrying a similar patch for many years and we
>would like to move to an upstream implementation.  Xen users of PCI
>passthrough would benefit from more reliable device reset.
>  2017 thread, including OpenXT
>patch: [1]https://lists.gt.net/xen/devel/492945
>  2017-2019 thread: [2]https://lists.gt.net/xen/devel/532648
>

Yes, rebasing the kernel patch against the current Linux kernel, and also 
rebasing the libxl bits against current master/staging.
That should be a good start!

I'd like to see the reset functionality merged aswell.


>Rich
> 


Thanks,

-- Pasi


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

2020-01-17 Thread Rich Persaud 
On Aug 26, 2019, at 17:08, Pasi Kärkkäinen  wrote:
> Hi,
> 
>> On Mon, Oct 08, 2018 at 10:32:45AM -0400, Boris Ostrovsky wrote:
>>> On 10/3/18 11:51 AM, Pasi Kärkkäinen wrote:
>>> On Wed, Sep 19, 2018 at 11:05:26AM +0200, Roger Pau Monné wrote:
 On Tue, Sep 18, 2018 at 02:09:53PM -0400, Boris Ostrovsky wrote:
> On 9/18/18 5:32 AM, George Dunlap wrote:
>>> On Sep 18, 2018, at 8:15 AM, Pasi Kärkkäinen  wrote:
>>> Hi,
>>> On Mon, Sep 17, 2018 at 02:06:02PM -0400, Boris Ostrovsky wrote:
 What about the toolstack changes? Have they been accepted? I vaguely
 recall there was a discussion about those changes but don't remember 
 how
 it ended.
>>> I don't think toolstack/libxl patch has been applied yet either.
>>> "[PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS attribute":
>>> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html
>>> "[PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' SysFS 
>>> attribute":
>>> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html
> Will this patch work for *BSD? Roger?
 At least FreeBSD don't support pci-passthrough, so none of this works
 ATM. There's no sysfs on BSD, so much of what's in libxl_pci.c will
 have to be moved to libxl_linux.c when BSD support is added.
>>> Ok. That sounds like it's OK for the initial pci 'reset' implementation in 
>>> xl/libxl to be linux-only..
>> 
>> Are these two patches still needed? ISTR they were  written originally
>> to deal with guest trying to use device that was previously assigned to
>> another guest. But pcistub_put_pci_dev() calls
>> __pci_reset_function_locked() which first tries FLR, and it looks like
>> it was added relatively recently.
> 
> Replying to an old thread.. I only now realized I forgot to reply to this 
> message earlier.
> 
> afaik these patches are still needed. Håkon (CC'd) wrote to me in private that
> he gets a (dom0) Linux kernel crash if he doesn't have these patches applied.
> 
> 
> Here are the links to both the linux kernel and libxl patches:
> 
> 
> "[Xen-devel] [PATCH V3 0/2] Xen/PCIback: PCI reset using 'reset' SysFS 
> attribute":
> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00659.html
> 
> [Note that PATCH V3 1/2 "Drivers/PCI: Export pcie_has_flr() interface" is 
> already applied in upstream linux kernel, so it's not needed anymore]
> 
> "[Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset 
> with 'reset' SysFS attribute":
> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00661.html
> 
> 
> "[Xen-devel] [PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS 
> attribute":
> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html
> 
> "[Xen-devel] [PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' SysFS 
> attribute":
> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html

[dropping Linux mailing lists]

What is required to get the Xen patches merged?  Rebasing against Xen master?  
OpenXT has been carrying a similar patch for many years and we would like to 
move to an upstream implementation.  Xen users of PCI passthrough would benefit 
from more reliable device reset.

  2017 thread, including OpenXT patch: https://lists.gt.net/xen/devel/492945
  2017-2019 thread: https://lists.gt.net/xen/devel/532648

Rich___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

2019-08-26 Thread Pasi Kärkkäinen 
Hi,

On Mon, Oct 08, 2018 at 10:32:45AM -0400, Boris Ostrovsky wrote:
> On 10/3/18 11:51 AM, Pasi Kärkkäinen wrote:
> > On Wed, Sep 19, 2018 at 11:05:26AM +0200, Roger Pau Monné wrote:
> >> On Tue, Sep 18, 2018 at 02:09:53PM -0400, Boris Ostrovsky wrote:
> >>> On 9/18/18 5:32 AM, George Dunlap wrote:
> > On Sep 18, 2018, at 8:15 AM, Pasi Kärkkäinen  wrote:
> >
> > Hi,
> >
> > On Mon, Sep 17, 2018 at 02:06:02PM -0400, Boris Ostrovsky wrote:
> >> What about the toolstack changes? Have they been accepted? I vaguely
> >> recall there was a discussion about those changes but don't remember 
> >> how
> >> it ended.
> >>
> > I don't think toolstack/libxl patch has been applied yet either.
> >
> >
> > "[PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS attribute":
> > https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html
> >
> > "[PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' SysFS 
> > attribute":
> > https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html
> >>>
> >>> Will this patch work for *BSD? Roger?
> >> At least FreeBSD don't support pci-passthrough, so none of this works
> >> ATM. There's no sysfs on BSD, so much of what's in libxl_pci.c will
> >> have to be moved to libxl_linux.c when BSD support is added.
> >>
> > Ok. That sounds like it's OK for the initial pci 'reset' implementation in 
> > xl/libxl to be linux-only.. 
> >
> 
> Are these two patches still needed? ISTR they were  written originally
> to deal with guest trying to use device that was previously assigned to
> another guest. But pcistub_put_pci_dev() calls
> __pci_reset_function_locked() which first tries FLR, and it looks like
> it was added relatively recently.
> 

Replying to an old thread.. I only now realized I forgot to reply to this 
message earlier.

afaik these patches are still needed. Håkon (CC'd) wrote to me in private that
he gets a (dom0) Linux kernel crash if he doesn't have these patches applied.


Here are the links to both the linux kernel and libxl patches:


"[Xen-devel] [PATCH V3 0/2] Xen/PCIback: PCI reset using 'reset' SysFS 
attribute":
https://lists.xen.org/archives/html/xen-devel/2017-12/msg00659.html

[Note that PATCH V3 1/2 "Drivers/PCI: Export pcie_has_flr() interface" is 
already applied in upstream linux kernel, so it's not needed anymore]

"[Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 
'reset' SysFS attribute":
https://lists.xen.org/archives/html/xen-devel/2017-12/msg00661.html


"[Xen-devel] [PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS attribute":
https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html

"[Xen-devel] [PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' SysFS 
attribute":
https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html


> 
> -boris


Thanks,

-- Pasi


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

2018-10-29 Thread Pasi Kärkkäinen 
Hi,

On Tue, Oct 23, 2018 at 08:40:29PM +0200, Håkon Alstadheim wrote:
> 
> 
> Den 08. okt. 2018 16:32, skrev Boris Ostrovsky:
> >
> > Are these two patches still needed? ISTR they were  written originally
> > to deal with guest trying to use device that was previously assigned to
> > another guest. But pcistub_put_pci_dev() calls
> > __pci_reset_function_locked() which first tries FLR, and it looks like
> > it was added relatively recently.
> >
> >
> Sorry for the late reply, but I just now booted xen staging-4.11
> (94fba9f438a2c36ad9bf3a481a6013ddc7cf8cd9), with gentoo-sources-4.19.0
> as dom0. Shut down and started again a VM that has a secondary GPU
> passed through, and the whole  machine hung. I haven't had time to look
> more closely into this, other than that my old "do_flr" patch no longer
> applies to gentoo-sources (i.e. the linux kernel sources) . "do_flr"
> worked for me on linux-4.18.? , with appropriate patch to the linux kernel.
> 
> So, something is definitely needed. No "reset" , or "do_flr" is present
> in linux-4.19.0, viz:
> 
> $ cd /usr/src/linux/drivers/xen/xen-pciback
> $ grep DRIVER *
> pci_stub.c:#define PCISTUB_DRIVER_NAME "pciback"
> pci_stub.c:  !strcmp(dev->driver_override, PCISTUB_DRIVER_NAME)) ||
> pci_stub.c: .name = PCISTUB_DRIVER_NAME,
> pci_stub.c:static DRIVER_ATTR_WO(new_slot);
> pci_stub.c:static DRIVER_ATTR_WO(remove_slot);
> pci_stub.c:static DRIVER_ATTR_RO(slots);
> pci_stub.c:static DRIVER_ATTR_RO(irq_handlers);
> pci_stub.c:static DRIVER_ATTR_WO(irq_handler_state);
> pci_stub.c:static DRIVER_ATTR_RW(quirks);
> pci_stub.c:static DRIVER_ATTR_RW(permissive);
> pci_stub.c: if (action != BUS_NOTIFY_UNBIND_DRIVER)
> $
> 
> 
> I'd be happy to test patches. Seems I only got one corrupt file from my
> test this morning :-D .
> 

Håkon: Please do test the patches and report how it works for you!
Here are the links:

Linux kernel xen-pciback 'reset' patches:

"[PATCH V3 0/2] Xen/PCIback: PCI reset using 'reset' SysFS attribute":
https://lists.xen.org/archives/html/xen-devel/2017-12/msg00659.html

(Patch 1/2 is not needed anymore in upstream Linux kernel, as 
pcie_has_flr() is already exported meanwhile)

"[PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 
'reset' SysFS attribute":
https://lists.xen.org/archives/html/xen-devel/2017-12/msg00661.html

Xen libxl 'reset' patches:

"[PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS attribute":
https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html

"[PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' SysFS 
attribute":
https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html



Thanks,

-- Pasi


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

2018-10-08 Thread Boris Ostrovsky 
On 10/3/18 11:51 AM, Pasi Kärkkäinen wrote:
> On Wed, Sep 19, 2018 at 11:05:26AM +0200, Roger Pau Monné wrote:
>> On Tue, Sep 18, 2018 at 02:09:53PM -0400, Boris Ostrovsky wrote:
>>> On 9/18/18 5:32 AM, George Dunlap wrote:
> On Sep 18, 2018, at 8:15 AM, Pasi Kärkkäinen  wrote:
>
> Hi,
>
> On Mon, Sep 17, 2018 at 02:06:02PM -0400, Boris Ostrovsky wrote:
>> What about the toolstack changes? Have they been accepted? I vaguely
>> recall there was a discussion about those changes but don't remember how
>> it ended.
>>
> I don't think toolstack/libxl patch has been applied yet either.
>
>
> "[PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS attribute":
> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html
>
> "[PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' SysFS 
> attribute":
> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html
>>>
>>> Will this patch work for *BSD? Roger?
>> At least FreeBSD don't support pci-passthrough, so none of this works
>> ATM. There's no sysfs on BSD, so much of what's in libxl_pci.c will
>> have to be moved to libxl_linux.c when BSD support is added.
>>
> Ok. That sounds like it's OK for the initial pci 'reset' implementation in 
> xl/libxl to be linux-only.. 
>

Are these two patches still needed? ISTR they were  written originally
to deal with guest trying to use device that was previously assigned to
another guest. But pcistub_put_pci_dev() calls
__pci_reset_function_locked() which first tries FLR, and it looks like
it was added relatively recently.


-boris

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

2018-10-03 Thread Pasi Kärkkäinen 
On Wed, Sep 19, 2018 at 11:05:26AM +0200, Roger Pau Monné wrote:
> On Tue, Sep 18, 2018 at 02:09:53PM -0400, Boris Ostrovsky wrote:
> > On 9/18/18 5:32 AM, George Dunlap wrote:
> > >
> > >> On Sep 18, 2018, at 8:15 AM, Pasi Kärkkäinen  wrote:
> > >>
> > >> Hi,
> > >>
> > >> On Mon, Sep 17, 2018 at 02:06:02PM -0400, Boris Ostrovsky wrote:
> > >>> What about the toolstack changes? Have they been accepted? I vaguely
> > >>> recall there was a discussion about those changes but don't remember how
> > >>> it ended.
> > >>>
> > >> I don't think toolstack/libxl patch has been applied yet either.
> > >>
> > >>
> > >> "[PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS attribute":
> > >> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html
> > >>
> > >> "[PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' SysFS 
> > >> attribute":
> > >> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html
> > 
> > 
> > Will this patch work for *BSD? Roger?
> 
> At least FreeBSD don't support pci-passthrough, so none of this works
> ATM. There's no sysfs on BSD, so much of what's in libxl_pci.c will
> have to be moved to libxl_linux.c when BSD support is added.
> 

Ok. That sounds like it's OK for the initial pci 'reset' implementation in 
xl/libxl to be linux-only.. 


Thanks,

-- Pasi


> Thanks, Roger.


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

2018-09-19 Thread Roger Pau Monné 
On Tue, Sep 18, 2018 at 02:09:53PM -0400, Boris Ostrovsky wrote:
> On 9/18/18 5:32 AM, George Dunlap wrote:
> >
> >> On Sep 18, 2018, at 8:15 AM, Pasi Kärkkäinen  wrote:
> >>
> >> Hi,
> >>
> >> On Mon, Sep 17, 2018 at 02:06:02PM -0400, Boris Ostrovsky wrote:
> >>> What about the toolstack changes? Have they been accepted? I vaguely
> >>> recall there was a discussion about those changes but don't remember how
> >>> it ended.
> >>>
> >> I don't think toolstack/libxl patch has been applied yet either.
> >>
> >>
> >> "[PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS attribute":
> >> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html
> >>
> >> "[PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' SysFS 
> >> attribute":
> >> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html
> 
> 
> Will this patch work for *BSD? Roger?

At least FreeBSD don't support pci-passthrough, so none of this works
ATM. There's no sysfs on BSD, so much of what's in libxl_pci.c will
have to be moved to libxl_linux.c when BSD support is added.

Thanks, Roger.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

2018-09-18 Thread Boris Ostrovsky 
On 9/18/18 5:32 AM, George Dunlap wrote:
>
>> On Sep 18, 2018, at 8:15 AM, Pasi Kärkkäinen  wrote:
>>
>> Hi,
>>
>> On Mon, Sep 17, 2018 at 02:06:02PM -0400, Boris Ostrovsky wrote:
>>> What about the toolstack changes? Have they been accepted? I vaguely
>>> recall there was a discussion about those changes but don't remember how
>>> it ended.
>>>
>> I don't think toolstack/libxl patch has been applied yet either.
>>
>>
>> "[PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS attribute":
>> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html
>>
>> "[PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' SysFS attribute":
>> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html


Will this patch work for *BSD? Roger?


>>
>> George asked for some clarifications:
>> https://lists.xen.org/archives/html/xen-devel/2017-12/msg01044.html
>> https://lists.xen.org/archives/html/xen-devel/2017-12/msg01116.html
> Right, the description of the patch didn’t actually tell you what was going 
> on.  It should have said something like, “xl currently attempts to reset a 
> device using X; but that’s never been implemented in Linux.  Instead, use Y, 
> which [is better for whatever reason]”.

Yes, the description can be tightened a bit ;-)

-boris


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

2018-09-18 Thread George Dunlap 


> On Sep 18, 2018, at 8:15 AM, Pasi Kärkkäinen  wrote:
> 
> Hi,
> 
> On Mon, Sep 17, 2018 at 02:06:02PM -0400, Boris Ostrovsky wrote:
>> On 9/16/18 7:43 AM, Pasi Kärkkäinen wrote:
>>> Hi,
>>> 
>>> On Mon, Dec 18, 2017 at 12:32:11PM -0500, Boris Ostrovsky wrote:
 On 12/18/2017 02:36 AM, Jan Beulich wrote:
 On 15.12.17 at 20:52,  wrote:
> +static int pcistub_device_reset(struct pci_dev *dev)
> +{
> + struct xen_pcibk_dev_data *dev_data;
> + bool slot = false, bus = false;
> + struct pcistub_args arg = {};
> +
> + if (!dev)
> + return -EINVAL;
> +
> + dev_dbg(>dev, "[%s]\n", __func__);
> +
> + /* First check and try FLR */
> + if (pcie_has_flr(dev)) {
> + dev_dbg(>dev, "resetting %s device using FLR\n",
> + pci_name(dev));
> + pcie_flr(dev);
 The lack of error check here puzzled me, but I see the function
 indeed returns void right now. I think the prereq patch should
 change this along with exporting the function - you really don't
 want the device to be handed to a guest when the FLR timed
 out.
>>> We will change pcie_flr() to return error code. I will make this change
>>> in the next version of this patch.
>> I exchanged some emails with Bjorn/Christoph and it looks like Christoph
>> as some planto restructure pcie flr specific functions but I don't know
>> the exact time-frame. For now,I am planning to use existing pcie_flr()
>> after checking FLR capability. We will switchto revised pcie_flr() once
>> it is available.
>> 
>> I hope you are fine with this approach. Please let me know. Thanks.
> I've seen that other discussion. I don't think the change here
> should be done prior to the error reporting being put in place,
> for security reasons. But in the end it'll be Konrad as the
> maintainer to judge.
> 
> Or wait, looks like there's some confusion in ./MAINTAINERS:
> Konrad is listed as maintainer for "XEN PCI SUBSYSTEM", but the
> list of files doesn't include pciback. So it would instead be Boris
> or Jürgen to give you a final word.
 
 This is now 4.16 material so we can at least wait until closer to
 opening of the merge window when we may have the PCI updates. (And I
 just noticed that you responded to Christoph.)
 
 Besides, we don't want to make kernel changes until the interface is
 settled (i.e the toolstack changes are accepted).
 
>>> It seems Govinda's email address is giving an error, so I assume someone 
>>> else needs to pick up this pciback 'reset' feature.
>>> Is it likely someone else from Oracle can/will pick up and refresh this 
>>> patch, with the review comments addressed?
>> 
>> 
>> Govinda is no longer at Oracle.
>> 
> 
> Yep, thought so. Removed from CC list.
> 
> 
>> What about the toolstack changes? Have they been accepted? I vaguely
>> recall there was a discussion about those changes but don't remember how
>> it ended.
>> 
> 
> I don't think toolstack/libxl patch has been applied yet either.
> 
> 
> "[PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS attribute":
> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html
> 
> "[PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' SysFS attribute":
> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html
> 
> George asked for some clarifications:
> https://lists.xen.org/archives/html/xen-devel/2017-12/msg01044.html
> https://lists.xen.org/archives/html/xen-devel/2017-12/msg01116.html

Right, the description of the patch didn’t actually tell you what was going on. 
 It should have said something like, “xl currently attempts to reset a device 
using X; but that’s never been implemented in Linux.  Instead, use Y, which [is 
better for whatever reason]”.

 -George

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

2018-09-18 Thread Pasi Kärkkäinen 
Hi,

On Mon, Sep 17, 2018 at 02:06:02PM -0400, Boris Ostrovsky wrote:
> On 9/16/18 7:43 AM, Pasi Kärkkäinen wrote:
> > Hi,
> >
> > On Mon, Dec 18, 2017 at 12:32:11PM -0500, Boris Ostrovsky wrote:
> >> On 12/18/2017 02:36 AM, Jan Beulich wrote:
> >> On 15.12.17 at 20:52,  wrote:
> >>> +static int pcistub_device_reset(struct pci_dev *dev)
> >>> +{
> >>> + struct xen_pcibk_dev_data *dev_data;
> >>> + bool slot = false, bus = false;
> >>> + struct pcistub_args arg = {};
> >>> +
> >>> + if (!dev)
> >>> + return -EINVAL;
> >>> +
> >>> + dev_dbg(>dev, "[%s]\n", __func__);
> >>> +
> >>> + /* First check and try FLR */
> >>> + if (pcie_has_flr(dev)) {
> >>> + dev_dbg(>dev, "resetting %s device using FLR\n",
> >>> + pci_name(dev));
> >>> + pcie_flr(dev);
> >> The lack of error check here puzzled me, but I see the function
> >> indeed returns void right now. I think the prereq patch should
> >> change this along with exporting the function - you really don't
> >> want the device to be handed to a guest when the FLR timed
> >> out.
> > We will change pcie_flr() to return error code. I will make this change
> > in the next version of this patch.
>  I exchanged some emails with Bjorn/Christoph and it looks like Christoph
>  as some planto restructure pcie flr specific functions but I don't know
>  the exact time-frame. For now,I am planning to use existing pcie_flr()
>  after checking FLR capability. We will switchto revised pcie_flr() once
>  it is available.
> 
>  I hope you are fine with this approach. Please let me know. Thanks.
> >>> I've seen that other discussion. I don't think the change here
> >>> should be done prior to the error reporting being put in place,
> >>> for security reasons. But in the end it'll be Konrad as the
> >>> maintainer to judge.
> >>>
> >>> Or wait, looks like there's some confusion in ./MAINTAINERS:
> >>> Konrad is listed as maintainer for "XEN PCI SUBSYSTEM", but the
> >>> list of files doesn't include pciback. So it would instead be Boris
> >>> or Jürgen to give you a final word.
> >>
> >> This is now 4.16 material so we can at least wait until closer to
> >> opening of the merge window when we may have the PCI updates. (And I
> >> just noticed that you responded to Christoph.)
> >>
> >> Besides, we don't want to make kernel changes until the interface is
> >> settled (i.e the toolstack changes are accepted).
> >>
> > It seems Govinda's email address is giving an error, so I assume someone 
> > else needs to pick up this pciback 'reset' feature.
> > Is it likely someone else from Oracle can/will pick up and refresh this 
> > patch, with the review comments addressed?
> 
> 
> Govinda is no longer at Oracle.
> 

Yep, thought so. Removed from CC list.


> What about the toolstack changes? Have they been accepted? I vaguely
> recall there was a discussion about those changes but don't remember how
> it ended.
> 

I don't think toolstack/libxl patch has been applied yet either.


"[PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS attribute":
https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html

"[PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' SysFS attribute":
https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html

George asked for some clarifications:
https://lists.xen.org/archives/html/xen-devel/2017-12/msg01044.html
https://lists.xen.org/archives/html/xen-devel/2017-12/msg01116.html

> 
> -boris
> 


Thanks,

-- Pasi

> 
> >
> >
> > Meanwhile the pcie_has_flr() has been exported in upstream Linux kernel, so 
> > that's already available for use now.
> >
> > "PCI: Export pcie_has_flr()":
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2d2917f7747805a1f4188672f308d82a8ba01700
> >
> >
> >
> 

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

2018-09-17 Thread Boris Ostrovsky 
On 9/16/18 7:43 AM, Pasi Kärkkäinen wrote:
> Hi,
>
> On Mon, Dec 18, 2017 at 12:32:11PM -0500, Boris Ostrovsky wrote:
>> On 12/18/2017 02:36 AM, Jan Beulich wrote:
>> On 15.12.17 at 20:52,  wrote:
>>> +static int pcistub_device_reset(struct pci_dev *dev)
>>> +{
>>> +   struct xen_pcibk_dev_data *dev_data;
>>> +   bool slot = false, bus = false;
>>> +   struct pcistub_args arg = {};
>>> +
>>> +   if (!dev)
>>> +   return -EINVAL;
>>> +
>>> +   dev_dbg(>dev, "[%s]\n", __func__);
>>> +
>>> +   /* First check and try FLR */
>>> +   if (pcie_has_flr(dev)) {
>>> +   dev_dbg(>dev, "resetting %s device using FLR\n",
>>> +   pci_name(dev));
>>> +   pcie_flr(dev);
>> The lack of error check here puzzled me, but I see the function
>> indeed returns void right now. I think the prereq patch should
>> change this along with exporting the function - you really don't
>> want the device to be handed to a guest when the FLR timed
>> out.
> We will change pcie_flr() to return error code. I will make this change
> in the next version of this patch.
 I exchanged some emails with Bjorn/Christoph and it looks like Christoph
 as some planto restructure pcie flr specific functions but I don't know
 the exact time-frame. For now,I am planning to use existing pcie_flr()
 after checking FLR capability. We will switchto revised pcie_flr() once
 it is available.

 I hope you are fine with this approach. Please let me know. Thanks.
>>> I've seen that other discussion. I don't think the change here
>>> should be done prior to the error reporting being put in place,
>>> for security reasons. But in the end it'll be Konrad as the
>>> maintainer to judge.
>>>
>>> Or wait, looks like there's some confusion in ./MAINTAINERS:
>>> Konrad is listed as maintainer for "XEN PCI SUBSYSTEM", but the
>>> list of files doesn't include pciback. So it would instead be Boris
>>> or Jürgen to give you a final word.
>>
>> This is now 4.16 material so we can at least wait until closer to
>> opening of the merge window when we may have the PCI updates. (And I
>> just noticed that you responded to Christoph.)
>>
>> Besides, we don't want to make kernel changes until the interface is
>> settled (i.e the toolstack changes are accepted).
>>
> It seems Govinda's email address is giving an error, so I assume someone else 
> needs to pick up this pciback 'reset' feature.
> Is it likely someone else from Oracle can/will pick up and refresh this 
> patch, with the review comments addressed?


Govinda is no longer at Oracle.

What about the toolstack changes? Have they been accepted? I vaguely
recall there was a discussion about those changes but don't remember how
it ended.


-boris


>
>
> Meanwhile the pcie_has_flr() has been exported in upstream Linux kernel, so 
> that's already available for use now.
>
> "PCI: Export pcie_has_flr()":
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2d2917f7747805a1f4188672f308d82a8ba01700
>
>
>


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

2018-09-16 Thread Pasi Kärkkäinen 
Hi,

On Mon, Dec 18, 2017 at 12:32:11PM -0500, Boris Ostrovsky wrote:
> On 12/18/2017 02:36 AM, Jan Beulich wrote:
>  On 15.12.17 at 20:52,  wrote:
> > +static int pcistub_device_reset(struct pci_dev *dev)
> > +{
> > +   struct xen_pcibk_dev_data *dev_data;
> > +   bool slot = false, bus = false;
> > +   struct pcistub_args arg = {};
> > +
> > +   if (!dev)
> > +   return -EINVAL;
> > +
> > +   dev_dbg(>dev, "[%s]\n", __func__);
> > +
> > +   /* First check and try FLR */
> > +   if (pcie_has_flr(dev)) {
> > +   dev_dbg(>dev, "resetting %s device using FLR\n",
> > +   pci_name(dev));
> > +   pcie_flr(dev);
>  The lack of error check here puzzled me, but I see the function
>  indeed returns void right now. I think the prereq patch should
>  change this along with exporting the function - you really don't
>  want the device to be handed to a guest when the FLR timed
>  out.
> >>> We will change pcie_flr() to return error code. I will make this change
> >>> in the next version of this patch.
> >> I exchanged some emails with Bjorn/Christoph and it looks like Christoph
> >> as some planto restructure pcie flr specific functions but I don't know
> >> the exact time-frame. For now,I am planning to use existing pcie_flr()
> >> after checking FLR capability. We will switchto revised pcie_flr() once
> >> it is available.
> >>
> >> I hope you are fine with this approach. Please let me know. Thanks.
> > I've seen that other discussion. I don't think the change here
> > should be done prior to the error reporting being put in place,
> > for security reasons. But in the end it'll be Konrad as the
> > maintainer to judge.
> >
> > Or wait, looks like there's some confusion in ./MAINTAINERS:
> > Konrad is listed as maintainer for "XEN PCI SUBSYSTEM", but the
> > list of files doesn't include pciback. So it would instead be Boris
> > or Jürgen to give you a final word.
> 
> 
> This is now 4.16 material so we can at least wait until closer to
> opening of the merge window when we may have the PCI updates. (And I
> just noticed that you responded to Christoph.)
> 
> Besides, we don't want to make kernel changes until the interface is
> settled (i.e the toolstack changes are accepted).
> 

It seems Govinda's email address is giving an error, so I assume someone else 
needs to pick up this pciback 'reset' feature.
Is it likely someone else from Oracle can/will pick up and refresh this patch, 
with the review comments addressed?


Meanwhile the pcie_has_flr() has been exported in upstream Linux kernel, so 
that's already available for use now.

"PCI: Export pcie_has_flr()":
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2d2917f7747805a1f4188672f308d82a8ba01700


> -boris
>


Thanks,

-- Pasi


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

2017-12-18 Thread Boris Ostrovsky 
On 12/18/2017 02:36 AM, Jan Beulich wrote:
 On 15.12.17 at 20:52,  wrote:
> +static int pcistub_device_reset(struct pci_dev *dev)
> +{
> + struct xen_pcibk_dev_data *dev_data;
> + bool slot = false, bus = false;
> + struct pcistub_args arg = {};
> +
> + if (!dev)
> + return -EINVAL;
> +
> + dev_dbg(>dev, "[%s]\n", __func__);
> +
> + /* First check and try FLR */
> + if (pcie_has_flr(dev)) {
> + dev_dbg(>dev, "resetting %s device using FLR\n",
> + pci_name(dev));
> + pcie_flr(dev);
 The lack of error check here puzzled me, but I see the function
 indeed returns void right now. I think the prereq patch should
 change this along with exporting the function - you really don't
 want the device to be handed to a guest when the FLR timed
 out.
>>> We will change pcie_flr() to return error code. I will make this change
>>> in the next version of this patch.
>> I exchanged some emails with Bjorn/Christoph and it looks like Christoph
>> as some planto restructure pcie flr specific functions but I don't know
>> the exact time-frame. For now,I am planning to use existing pcie_flr()
>> after checking FLR capability. We will switchto revised pcie_flr() once
>> it is available.
>>
>> I hope you are fine with this approach. Please let me know. Thanks.
> I've seen that other discussion. I don't think the change here
> should be done prior to the error reporting being put in place,
> for security reasons. But in the end it'll be Konrad as the
> maintainer to judge.
>
> Or wait, looks like there's some confusion in ./MAINTAINERS:
> Konrad is listed as maintainer for "XEN PCI SUBSYSTEM", but the
> list of files doesn't include pciback. So it would instead be Boris
> or Jürgen to give you a final word.


This is now 4.16 material so we can at least wait until closer to
opening of the merge window when we may have the PCI updates. (And I
just noticed that you responded to Christoph.)

Besides, we don't want to make kernel changes until the interface is
settled (i.e the toolstack changes are accepted).

-boris


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

2017-12-17 Thread Jan Beulich 
>>> On 15.12.17 at 20:52,  wrote:
 +static int pcistub_device_reset(struct pci_dev *dev)
 +{
 +  struct xen_pcibk_dev_data *dev_data;
 +  bool slot = false, bus = false;
 +  struct pcistub_args arg = {};
 +
 +  if (!dev)
 +  return -EINVAL;
 +
 +  dev_dbg(>dev, "[%s]\n", __func__);
 +
 +  /* First check and try FLR */
 +  if (pcie_has_flr(dev)) {
 +  dev_dbg(>dev, "resetting %s device using FLR\n",
 +  pci_name(dev));
 +  pcie_flr(dev);
>>> The lack of error check here puzzled me, but I see the function
>>> indeed returns void right now. I think the prereq patch should
>>> change this along with exporting the function - you really don't
>>> want the device to be handed to a guest when the FLR timed
>>> out.
>> We will change pcie_flr() to return error code. I will make this change
>> in the next version of this patch.
> I exchanged some emails with Bjorn/Christoph and it looks like Christoph
> as some planto restructure pcie flr specific functions but I don't know
> the exact time-frame. For now,I am planning to use existing pcie_flr()
> after checking FLR capability. We will switchto revised pcie_flr() once
> it is available.
> 
> I hope you are fine with this approach. Please let me know. Thanks.

I've seen that other discussion. I don't think the change here
should be done prior to the error reporting being put in place,
for security reasons. But in the end it'll be Konrad as the
maintainer to judge.

Or wait, looks like there's some confusion in ./MAINTAINERS:
Konrad is listed as maintainer for "XEN PCI SUBSYSTEM", but the
list of files doesn't include pciback. So it would instead be Boris
or Jürgen to give you a final word.

Jan

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

2017-12-15 Thread Govinda Tatti 

Jan,

One quick update on pcie_flr() specific implementation. Please see below.

+static int pcistub_device_reset(struct pci_dev *dev)
+{
+   struct xen_pcibk_dev_data *dev_data;
+   bool slot = false, bus = false;
+   struct pcistub_args arg = {};
+
+   if (!dev)
+   return -EINVAL;
+
+   dev_dbg(>dev, "[%s]\n", __func__);
+
+   /* First check and try FLR */
+   if (pcie_has_flr(dev)) {
+   dev_dbg(>dev, "resetting %s device using FLR\n",
+   pci_name(dev));
+   pcie_flr(dev);

The lack of error check here puzzled me, but I see the function
indeed returns void right now. I think the prereq patch should
change this along with exporting the function - you really don't
want the device to be handed to a guest when the FLR timed
out.

We will change pcie_flr() to return error code. I will make this change
in the next version of this patch.

I exchanged some emails with Bjorn/Christoph and it looks like Christoph
as some planto restructure pcie flr specific functions but I don't know
the exact time-frame. For now,I am planning to use existing pcie_flr()
after checking FLR capability. We will switchto revised pcie_flr() once
it is available.

I hope you are fine with this approach. Please let me know. Thanks.

Cheers
GOVINDA

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

2017-12-12 Thread Govinda Tatti 



On 12/12/2017 9:01 AM, Jan Beulich wrote:

On 12.12.17 at 15:48,  wrote:

Thanks Jan for your review comments. Please see below for my comments.

First of all - can you please do something about your reply style?
HTML mail should be avoided. You'll see that the (plain text) reply
as a result is rather hard to follow, too.

Sorry about it. I had an issue with my Thunderbird setting.



--- a/Documentation/ABI/testing/sysfs-driver-pciback
+++ b/Documentation/ABI/testing/sysfs-driver-pciback
@@ -11,3 +11,18 @@ Description:
  #echo 00:19.0-E0:2:FF > /sys/bus/pci/drivers/pciback/quirks
  will allow the guest to read and write to the configuration
  register 0x0E.
+
+What:   /sys/bus/pci/drivers/pciback/reset
+Date:   Dec 2017
+KernelVersion:  4.15
+Contact:xen-devel@lists.xenproject.org
+Description:
+An option to perform a flr/slot/bus reset when a PCI device
+ is owned by Xen PCI backend. Writing a string of :BB:DD.F
:BB:DD.F (or else the D-s are ambiguous, the more that "domain"
in Xen code is ambiguous anyway - I continue to be mislead by struct
pcistub_device_id's domain field)  Thanks for catching this issue. I will
fix it.


Also I assume the  part is optional (default zero), which
probably can and should be expressed in some way.   can be 0 or
non-zero, subject to system configuration.

The question isn't system configuration, but whether the field can
be omitted on input, with zero being assumed in such a case. That's
a common shorthand, considering that the vast majority of x86
(and maybe other) systems aren't using segments other than zero

Yes, it can be omitted if  is zero.I will add this information
to above documentation file.

Cheers
GOVINDA

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

2017-12-12 Thread Jan Beulich 
>>> On 12.12.17 at 15:48,  wrote:
> Thanks Jan for your review comments. Please see below for my comments.

First of all - can you please do something about your reply style?
HTML mail should be avoided. You'll see that the (plain text) reply
as a result is rather hard to follow, too.

> --- a/Documentation/ABI/testing/sysfs-driver-pciback
> +++ b/Documentation/ABI/testing/sysfs-driver-pciback
> @@ -11,3 +11,18 @@ Description:
>  #echo 00:19.0-E0:2:FF > /sys/bus/pci/drivers/pciback/quirks
>  will allow the guest to read and write to the configuration
>  register 0x0E.
> +
> +What:   /sys/bus/pci/drivers/pciback/reset
> +Date:   Dec 2017
> +KernelVersion:  4.15
> +Contact:xen-devel@lists.xenproject.org 
> +Description:
> +An option to perform a flr/slot/bus reset when a PCI device
> + is owned by Xen PCI backend. Writing a string of :BB:DD.F  
> :BB:DD.F (or else the D-s are ambiguous, the more that "domain"
> in Xen code is ambiguous anyway - I continue to be mislead by struct
> pcistub_device_id's domain field)  Thanks for catching this issue. I will 
> fix it.
> 
> 
> Also I assume the  part is optional (default zero), which
> probably can and should be expressed in some way.   can be 0 or 
> non-zero, subject to system configuration.

The question isn't system configuration, but whether the field can
be omitted on input, with zero being assumed in such a case. That's
a common shorthand, considering that the vast majority of x86
(and maybe other) systems aren't using segments other than zero.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

2017-12-12 Thread Govinda Tatti 

  
  
Thanks Jan for your review comments. Please see below for my
  comments.


On 12/8/2017 3:34 AM, Jan Beulich
wrote:
  

  

  
On 07.12.17 at 23:21,  wrote:

  

Due to the complexity with the PCI lock we cannot do the reset when a
device is bound ('echo $BDF > bind') or when unbound ('echo $BDF > unbind')
as the pci_[slot|bus]_reset also takes the same lock resulting in a
dead-lock.

  
  
It took me a moment to figure that here you're referring to the
process of (un)binding, not the state. To avoid that ambiguity in
wording, how about "... we cannot do the reset while a device is
being bound (...) or while it is being unbound ..."?

Sure, I will fix it.


  


  
--- a/Documentation/ABI/testing/sysfs-driver-pciback
+++ b/Documentation/ABI/testing/sysfs-driver-pciback
@@ -11,3 +11,18 @@ Description:
 #echo 00:19.0-E0:2:FF > /sys/bus/pci/drivers/pciback/quirks
 will allow the guest to read and write to the configuration
 register 0x0E.
+
+What:   /sys/bus/pci/drivers/pciback/reset
+Date:   Dec 2017
+KernelVersion:  4.15
+Contact:xen-devel@lists.xenproject.org 
+Description:
+An option to perform a flr/slot/bus reset when a PCI device
+		is owned by Xen PCI backend. Writing a string of :BB:DD.F

  
  
:BB:DD.F (or else the D-s are ambiguous, the more that "domain"
in Xen code is ambiguous anyway - I continue to be mislead by struct
pcistub_device_id's domain field)

Thanks for catching this issue. I will fix it.


  

Also I assume the  part is optional (default zero), which
probably can and should be expressed in some way.

 can be 0 or non-zero, subject to system configuration.


  


  
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -313,6 +313,102 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
 	up_write(_sem);
 }
 
+struct pcistub_args {
+	const struct pci_dev *dev;
+	unsigned int dcount;

  
  
The sole use of this field is for a debug message. Why not drop it
and make "dev" the "data" argument without further indirection?

I prefer to keep this data structure since it will be helpful to
  debug any issues
  or for future enhancements.


  


  
+static int pcistub_device_search(struct pci_dev *dev, void *data)
+{
+	struct pcistub_device *psdev;
+	struct pcistub_args *arg = data;
+	bool found = false;
+	unsigned long flags;
+
+	spin_lock_irqsave(_devices_lock, flags);
+
+	list_for_each_entry(psdev, _devices, dev_list) {
+		if (psdev->dev == dev) {
+			found = true;
+			arg->dcount++;
+			break;

  
  
Neither here nor in the caller I can see a check whether the device
is currently assigned to a guest. Ownership by pciback alone imo is
not sufficient to allow a reset to be performed.

I can add the following check

if ((psdev->dev == dev) &&
  (pci_is_dev_assigned(dev)))


  


  
+static int pcistub_device_reset(struct pci_dev *dev)
+{
+	struct xen_pcibk_dev_data *dev_data;
+	bool slot = false, bus = false;
+	struct pcistub_args arg = {};
+
+	if (!dev)
+		return -EINVAL;
+
+	dev_dbg(>dev, "[%s]\n", __func__);
+
+	/* First check and try FLR */
+	if (pcie_has_flr(dev)) {
+		dev_dbg(>dev, "resetting %s device using FLR\n",
+			pci_name(dev));
+		pcie_flr(dev);

  
  
The lack of error check here puzzled me, but I see the function
indeed returns void right now. I think the prereq patch should
change this along with exporting the function - you really don't
want the device to be handed to a guest when the FLR timed
out.

We will change pcie_flr() to return error code. I will make this
  change
in the next version of this patch.


  


  
+		return 0;
+	}
+
+	if (!pci_probe_reset_slot(dev->slot))
+		slot = true;
+	else if ((!pci_probe_reset_bus(dev->bus)) &&
+		 (!pci_is_root_bus(dev->bus)))

  
  
Too many parentheses for my taste.

I will fix it.


  


  
+static ssize_t reset_store(struct device_driver *drv, const char *buf,
+			   size_t count)
+{
+	struct pcistub_device *psdev;
+	int domain, bus, slot, func;
+	int err;
+
+	err = str_to_slot(buf, , , , );
+	if (err)
+		return err;
+
+	psdev = pcistub_device_find(domain, bus, slot, func);
+	if (psdev) {
+		err = pcistub_device_reset(psdev->dev);
+		pcistub_device_put(psdev);
+	} else {
+		err = -ENODEV;
+	}
+
+	if (!err)
+		err = count;
+
+	return err;
+}
+static DRIVER_ATTR_WO(reset);

  
  
Would it be worth for reads of the file to return whether the device
can be reset this way (i.e. the result of the checks you do before
actually doing the reset)?

I don't think so. Plus, it makes this

Re: [Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

2017-12-08 Thread Jan Beulich 
>>> On 07.12.17 at 23:21,  wrote:
> Due to the complexity with the PCI lock we cannot do the reset when a
> device is bound ('echo $BDF > bind') or when unbound ('echo $BDF > unbind')
> as the pci_[slot|bus]_reset also takes the same lock resulting in a
> dead-lock.

It took me a moment to figure that here you're referring to the
process of (un)binding, not the state. To avoid that ambiguity in
wording, how about "... we cannot do the reset while a device is
being bound (...) or while it is being unbound ..."?

> --- a/Documentation/ABI/testing/sysfs-driver-pciback
> +++ b/Documentation/ABI/testing/sysfs-driver-pciback
> @@ -11,3 +11,18 @@ Description:
>  #echo 00:19.0-E0:2:FF > /sys/bus/pci/drivers/pciback/quirks
>  will allow the guest to read and write to the configuration
>  register 0x0E.
> +
> +What:   /sys/bus/pci/drivers/pciback/reset
> +Date:   Dec 2017
> +KernelVersion:  4.15
> +Contact:xen-devel@lists.xenproject.org 
> +Description:
> +An option to perform a flr/slot/bus reset when a PCI device
> + is owned by Xen PCI backend. Writing a string of :BB:DD.F

:BB:DD.F (or else the D-s are ambiguous, the more that "domain"
in Xen code is ambiguous anyway - I continue to be mislead by struct
pcistub_device_id's domain field)

Also I assume the  part is optional (default zero), which
probably can and should be expressed in some way.

> --- a/drivers/xen/xen-pciback/pci_stub.c
> +++ b/drivers/xen/xen-pciback/pci_stub.c
> @@ -313,6 +313,102 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
>   up_write(_sem);
>  }
>  
> +struct pcistub_args {
> + const struct pci_dev *dev;
> + unsigned int dcount;

The sole use of this field is for a debug message. Why not drop it
and make "dev" the "data" argument without further indirection?

> +static int pcistub_device_search(struct pci_dev *dev, void *data)
> +{
> + struct pcistub_device *psdev;
> + struct pcistub_args *arg = data;
> + bool found = false;
> + unsigned long flags;
> +
> + spin_lock_irqsave(_devices_lock, flags);
> +
> + list_for_each_entry(psdev, _devices, dev_list) {
> + if (psdev->dev == dev) {
> + found = true;
> + arg->dcount++;
> + break;

Neither here nor in the caller I can see a check whether the device
is currently assigned to a guest. Ownership by pciback alone imo is
not sufficient to allow a reset to be performed.

> +static int pcistub_device_reset(struct pci_dev *dev)
> +{
> + struct xen_pcibk_dev_data *dev_data;
> + bool slot = false, bus = false;
> + struct pcistub_args arg = {};
> +
> + if (!dev)
> + return -EINVAL;
> +
> + dev_dbg(>dev, "[%s]\n", __func__);
> +
> + /* First check and try FLR */
> + if (pcie_has_flr(dev)) {
> + dev_dbg(>dev, "resetting %s device using FLR\n",
> + pci_name(dev));
> + pcie_flr(dev);

The lack of error check here puzzled me, but I see the function
indeed returns void right now. I think the prereq patch should
change this along with exporting the function - you really don't
want the device to be handed to a guest when the FLR timed
out.

> + return 0;
> + }
> +
> + if (!pci_probe_reset_slot(dev->slot))
> + slot = true;
> + else if ((!pci_probe_reset_bus(dev->bus)) &&
> +  (!pci_is_root_bus(dev->bus)))

Too many parentheses for my taste.

> +static ssize_t reset_store(struct device_driver *drv, const char *buf,
> +size_t count)
> +{
> + struct pcistub_device *psdev;
> + int domain, bus, slot, func;
> + int err;
> +
> + err = str_to_slot(buf, , , , );
> + if (err)
> + return err;
> +
> + psdev = pcistub_device_find(domain, bus, slot, func);
> + if (psdev) {
> + err = pcistub_device_reset(psdev->dev);
> + pcistub_device_put(psdev);
> + } else {
> + err = -ENODEV;
> + }
> +
> + if (!err)
> + err = count;
> +
> + return err;
> +}
> +static DRIVER_ATTR_WO(reset);

Would it be worth for reads of the file to return whether the device
can be reset this way (i.e. the result of the checks you do before
actually doing the reset)?

Jan

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel