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

["Håkon Alstadheim" <hakon@alstadheim.priv.no>]

[-- Attachment #1: Type: text/plain, Size: 5063 bytes --]


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 <wl@xen.org> 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 <pasik@iki.fi> 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 <pasik@iki.fi> 
>>>>>>>>>>> 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 ).


[-- Attachment #2: pci_brute_reset-home-hack.patch --]
[-- Type: text/x-patch, Size: 3558 bytes --]

--- 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(&pcistub_devices_lock, flags);
+
+	list_for_each_entry(psdev, &pcistub_devices, dev_list) {
+		if (psdev->dev == dev) {
+			found_dev = true;
+			arg->dcount++;
+			break;
+		}
+	}
+
+	spin_unlock_irqrestore(&pcistub_devices_lock, flags);
+
+	/* Device not owned by pcistub, someone owns it. Abort the walk */
+	if (!found_dev)
+		arg->dev = dev;
+
+	return found_dev ? 0 : 1;
+}
+
+static int pcistub_reset_dev(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->dev, "[%s]\n", __func__);
+
+	if (!pci_probe_reset_slot(dev->slot))
+		slot = true;
+	else if ((!pci_probe_reset_bus(dev->bus)) &&
+		 (!pci_is_root_bus(dev->bus)))
+		bus = true;
+
+	if (!bus && !slot)
+		return -EOPNOTSUPP;
+
+	/*
+	 * Make sure all devices on this bus are owned by the
+	 * PCI backend so that we can safely reset the whole bus.
+	 */
+	pci_walk_bus(dev->bus, pcistub_search_dev, &arg);
+
+	/* All devices under the bus should be part of pcistub! */
+	if (arg.dev) {
+		dev_err(&dev->dev, "%s device on bus 0x%x is not owned by pcistub\n",
+			pci_name(arg.dev), dev->bus->number);
+
+		return -EBUSY;
+	}
+
+	dev_dbg(&dev->dev, "pcistub owns %d devices on bus 0x%x\n",
+		arg.dcount, dev->bus->number);
+
+	dev_data = pci_get_drvdata(dev);
+	if (!pci_load_saved_state(dev, dev_data->pci_saved_state))
+		pci_restore_state(dev);
+
+	/* This disables the device. */
+	xen_pcibk_reset_device(dev);
+
+	/* Cleanup up any emulated fields */
+	xen_pcibk_config_reset_dev(dev);
+
+	dev_dbg(&dev->dev, "resetting %s device using %s reset\n",
+		pci_name(dev), slot ? "slot" : "bus");
+
+	return pci_reset_bus(dev);
+}
+
 /*
  * Called when:
  *  - XenBus state has been reconfigure (pci unplug). See xen_pcibk_remove_device
@@ -1492,6 +1576,33 @@
 }
 static DRIVER_ATTR_RW(allow_interrupt_control);
 
+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, &domain, &bus, &slot, &func);
+	if (err)
+		return err;
+
+	psdev = pcistub_device_find(domain, bus, slot, func);
+	if (psdev) {
+		err = pcistub_reset_dev(psdev->dev);
+		pcistub_device_put(psdev);
+	} else {
+		err = -ENODEV;
+	}
+
+	if (!err)
+		err = count;
+
+	return err;
+}
+
+static DRIVER_ATTR_WO(reset);
+
 static void pcistub_exit(void)
 {
 	driver_remove_file(&xen_pcibk_pci_driver.driver, &driver_attr_new_slot);
@@ -1507,6 +1618,8 @@
 			   &driver_attr_irq_handlers);
 	driver_remove_file(&xen_pcibk_pci_driver.driver,
 			   &driver_attr_irq_handler_state);
+	driver_remove_file(&xen_pcibk_pci_driver.driver,
+			   &driver_attr_reset);
 	pci_unregister_driver(&xen_pcibk_pci_driver);
 }
 
@@ -1603,6 +1716,11 @@
 	if (!err)
 		err = driver_create_file(&xen_pcibk_pci_driver.driver,
 					&driver_attr_irq_handler_state);
+
+	if (!err)
+		err = driver_create_file(&xen_pcibk_pci_driver.driver,
+					 &driver_attr_reset);
+
 	if (err)
 		pcistub_exit();
 

[-- Attachment #3: pci_brute_reset-home-hack-doc.patch --]
[-- Type: text/x-patch, Size: 931 bytes --]

--- a/Documentation/ABI/testing/sysfs-driver-pciback	2020-03-30 00:25:41.000000000 +0200
+++ b/Documentation/ABI/testing/sysfs-driver-pciback	2020-03-30 21:01:58.909583316 +0200
@@ -12,6 +12,19 @@
                 will allow the guest to read and write to the configuration
                 register 0x0E.
 
+
+What:           /sys/bus/pci/drivers/pciback/reset
+Date:           Nov 2017
+KernelVersion:  none
+Contact:        xen-devel@lists.xenproject.org
+Description:
+                An option to perform a slot or bus reset when a PCI device
+		is owned by Xen PCI backend. Writing a string of DDDD:BB:DD.F
+		will cause the pciback driver to perform a slot or bus reset
+		if the device supports it. It also checks to make sure that
+		all of the devices under the bridge are owned by Xen PCI
+		backend.
+
 What:           /sys/bus/pci/drivers/pciback/allow_interrupt_control
 Date:           Jan 2020
 KernelVersion:  5.6

[-- Attachment #4: pci-reset-min-egen.patch --]
[-- Type: text/x-patch, Size: 426 bytes --]

--- a/tools/libxl/libxl_pci.c	2020-04-09 09:26:54.000000000 +0200
+++ b/tools/libxl/libxl_pci.c	2020-04-14 23:39:12.830752851 +0200
@@ -1452,7 +1452,7 @@
     char *reset;
     int fd, rc;
 
-    reset = GCSPRINTF("%s/do_flr", SYSFS_PCIBACK_DRIVER);
+    reset = GCSPRINTF("%s/reset", SYSFS_PCIBACK_DRIVER);
     fd = open(reset, O_WRONLY);
     if (fd >= 0) {
         char *buf = GCSPRINTF(PCI_BDF, domain, bus, dev, func);