[PATCH v2 0/2] xen-pciback: a fix and a workaround

[PATCH v2 0/2] xen-pciback: a fix and a workaround

[Jan Beulich]

The first change completes a several years old but still incomplete
change. As mentioned there, reverting the original change may also
be an option. The second change works around some odd libxl behavior,
as described in [1]. As per a response to that mail addressing the
issue in libxl may also be possible, but it's not clear to me who
would get to doing so at which point in time. Hence the kernel side
alternative is being proposed here.

As to Konrad being on the Cc list: I find it puzzling that he's
listed under "XEN PCI SUBSYSTEM", but pciback isn't considered part
of this.

1: redo VF placement in the virtual topology
2: reconfigure also from backend watch handler

Jan

[1] https://lists.xen.org/archives/html/xen-devel/2021-03/msg00956.html

[PATCH v2 1/2] xen-pciback: redo VF placement in the virtual topology

[Jan Beulich]

The commit referenced below was incomplete: It merely affected what
would get written to the vdev-<N> xenstore node. The guest would still
find the function at the original function number as long as 
__xen_pcibk_get_pci_dev() wouldn't be in sync. The same goes for AER wrt
__xen_pcibk_get_pcifront_dev().

Undo overriding the function to zero and instead make sure that VFs at
function zero remain alone in their slot. This has the added benefit of
improving overall capacity, considering that there's only a total of 32
slots available right now (PCI segment and bus can both only ever be
zero at present).

Fixes: 8a5248fe10b1 ("xen PV passthru: assign SR-IOV virtual functions to separate virtual slots")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Cc: stable@vger.kernel.org
---
Like the original change this has the effect of changing where devices
would appear in the guest, when there are multiple of them. I don't see
an immediate problem with this, but if there is we may need to reduce
the effect of the change.
Taking into account, besides the described breakage, how xen-pcifront's
pcifront_scan_bus() works, I also wonder what problem it was in the
first place that needed fixing. It may therefore also be worth to
consider simply reverting the original change.

--- a/drivers/xen/xen-pciback/vpci.c
+++ b/drivers/xen/xen-pciback/vpci.c
@@ -70,7 +70,7 @@ static int __xen_pcibk_add_pci_dev(struc
 				   struct pci_dev *dev, int devid,
 				   publish_pci_dev_cb publish_cb)
 {
-	int err = 0, slot, func = -1;
+	int err = 0, slot, func = PCI_FUNC(dev->devfn);
 	struct pci_dev_entry *t, *dev_entry;
 	struct vpci_dev_data *vpci_dev = pdev->pci_dev_data;
 
@@ -95,22 +95,25 @@ static int __xen_pcibk_add_pci_dev(struc
 
 	/*
 	 * Keep multi-function devices together on the virtual PCI bus, except
-	 * virtual functions.
+	 * that we want to keep virtual functions at func 0 on their own. They
+	 * aren't multi-function devices and hence their presence at func 0
+	 * may cause guests to not scan the other functions.
 	 */
-	if (!dev->is_virtfn) {
+	if (!dev->is_virtfn || func) {
 		for (slot = 0; slot < PCI_SLOT_MAX; slot++) {
 			if (list_empty(&vpci_dev->dev_list[slot]))
 				continue;
 
 			t = list_entry(list_first(&vpci_dev->dev_list[slot]),
 				       struct pci_dev_entry, list);
+			if (t->dev->is_virtfn && !PCI_FUNC(t->dev->devfn))
+				continue;
 
 			if (match_slot(dev, t->dev)) {
 				dev_info(&dev->dev, "vpci: assign to virtual slot %d func %d\n",
-					 slot, PCI_FUNC(dev->devfn));
+					 slot, func);
 				list_add_tail(&dev_entry->list,
 					      &vpci_dev->dev_list[slot]);
-				func = PCI_FUNC(dev->devfn);
 				goto unlock;
 			}
 		}
@@ -123,7 +126,6 @@ static int __xen_pcibk_add_pci_dev(struc
 				 slot);
 			list_add_tail(&dev_entry->list,
 				      &vpci_dev->dev_list[slot]);
-			func = dev->is_virtfn ? 0 : PCI_FUNC(dev->devfn);
 			goto unlock;
 		}
 	}

Re: [PATCH v2 1/2] xen-pciback: redo VF placement in the virtual topology

[Boris Ostrovsky]

On 5/18/21 12:13 PM, Jan Beulich wrote:
>  
> @@ -95,22 +95,25 @@ static int __xen_pcibk_add_pci_dev(struc
>  
>  	/*
>  	 * Keep multi-function devices together on the virtual PCI bus, except
> -	 * virtual functions.
> +	 * that we want to keep virtual functions at func 0 on their own. They
> +	 * aren't multi-function devices and hence their presence at func 0
> +	 * may cause guests to not scan the other functions.


So your reading of the original commit is that whatever the issue it was, only function zero was causing the problem? In other words, you are not concerned that pci_scan_slot() may now look at function 1 and skip all higher-numbered function (assuming the problem is still there)?


-boris

Re: [PATCH v2 1/2] xen-pciback: redo VF placement in the virtual topology

[Jan Beulich]

On 20.05.2021 02:36, Boris Ostrovsky wrote:
> 
> On 5/18/21 12:13 PM, Jan Beulich wrote:
>>  
>> @@ -95,22 +95,25 @@ static int __xen_pcibk_add_pci_dev(struc
>>  
>>  	/*
>>  	 * Keep multi-function devices together on the virtual PCI bus, except
>> -	 * virtual functions.
>> +	 * that we want to keep virtual functions at func 0 on their own. They
>> +	 * aren't multi-function devices and hence their presence at func 0
>> +	 * may cause guests to not scan the other functions.
> 
> 
> So your reading of the original commit is that whatever the issue it was, only function zero was causing the problem? In other words, you are not concerned that pci_scan_slot() may now look at function 1 and skip all higher-numbered function (assuming the problem is still there)?

I'm not sure I understand the question: Whether to look at higher numbered
slots is a function of slot 0's multi-function bit alone, aiui. IOW if
slot 1 is being looked at in the first place, slots 2-7 should also be
looked at.

Jan

Re: [PATCH v2 1/2] xen-pciback: redo VF placement in the virtual topology

[Boris Ostrovsky]

On 5/20/21 3:43 AM, Jan Beulich wrote:
> On 20.05.2021 02:36, Boris Ostrovsky wrote:
>> On 5/18/21 12:13 PM, Jan Beulich wrote:
>>>  
>>> @@ -95,22 +95,25 @@ static int __xen_pcibk_add_pci_dev(struc
>>>  
>>>  	/*
>>>  	 * Keep multi-function devices together on the virtual PCI bus, except
>>> -	 * virtual functions.
>>> +	 * that we want to keep virtual functions at func 0 on their own. They
>>> +	 * aren't multi-function devices and hence their presence at func 0
>>> +	 * may cause guests to not scan the other functions.
>>
>> So your reading of the original commit is that whatever the issue it was, only function zero was causing the problem? In other words, you are not concerned that pci_scan_slot() may now look at function 1 and skip all higher-numbered function (assuming the problem is still there)?
> I'm not sure I understand the question: Whether to look at higher numbered
> slots is a function of slot 0's multi-function bit alone, aiui. IOW if
> slot 1 is being looked at in the first place, slots 2-7 should also be
> looked at.


Wasn't the original patch describing a problem strictly as one for single-function devices, so the multi-function bit is not set? I.e. if all VFs (which are single-function devices) are placed in the same slot then pci_scan_slot() would only look at function 0 and ignore anything higher-numbered.


My question is whether it would "only look at function 0 and ignore anything higher-numbered" or "only look at the lowest-numbered function and ignore anything higher-numbered".


-boris

Re: [PATCH v2 1/2] xen-pciback: redo VF placement in the virtual topology

[Jan Beulich]

On 20.05.2021 16:38, Boris Ostrovsky wrote:
> 
> On 5/20/21 3:43 AM, Jan Beulich wrote:
>> On 20.05.2021 02:36, Boris Ostrovsky wrote:
>>> On 5/18/21 12:13 PM, Jan Beulich wrote:
>>>>  
>>>> @@ -95,22 +95,25 @@ static int __xen_pcibk_add_pci_dev(struc
>>>>  
>>>>  	/*
>>>>  	 * Keep multi-function devices together on the virtual PCI bus, except
>>>> -	 * virtual functions.
>>>> +	 * that we want to keep virtual functions at func 0 on their own. They
>>>> +	 * aren't multi-function devices and hence their presence at func 0
>>>> +	 * may cause guests to not scan the other functions.
>>>
>>> So your reading of the original commit is that whatever the issue it was, only function zero was causing the problem? In other words, you are not concerned that pci_scan_slot() may now look at function 1 and skip all higher-numbered function (assuming the problem is still there)?
>> I'm not sure I understand the question: Whether to look at higher numbered
>> slots is a function of slot 0's multi-function bit alone, aiui. IOW if
>> slot 1 is being looked at in the first place, slots 2-7 should also be
>> looked at.
> 
> 
> Wasn't the original patch describing a problem strictly as one for single-function devices, so the multi-function bit is not set? I.e. if all VFs (which are single-function devices) are placed in the same slot then pci_scan_slot() would only look at function 0 and ignore anything higher-numbered.
> 
> 
> My question is whether it would "only look at function 0 and ignore anything higher-numbered" or "only look at the lowest-numbered function and ignore anything higher-numbered".

The common scanning logic is to look at slot 0 first. If that's populated,
other slots get looked at only if slot 0 has the multi-function bit set.
If slot 0 is not populated, nothing is known about the other slots, and
hence they need to be scanned.

Jan

Re: [PATCH v2 1/2] xen-pciback: redo VF placement in the virtual topology

[Jan Beulich]

On 20.05.2021 16:44, Jan Beulich wrote:
> On 20.05.2021 16:38, Boris Ostrovsky wrote:
>>
>> On 5/20/21 3:43 AM, Jan Beulich wrote:
>>> On 20.05.2021 02:36, Boris Ostrovsky wrote:
>>>> On 5/18/21 12:13 PM, Jan Beulich wrote:
>>>>>  
>>>>> @@ -95,22 +95,25 @@ static int __xen_pcibk_add_pci_dev(struc
>>>>>  
>>>>>  	/*
>>>>>  	 * Keep multi-function devices together on the virtual PCI bus, except
>>>>> -	 * virtual functions.
>>>>> +	 * that we want to keep virtual functions at func 0 on their own. They
>>>>> +	 * aren't multi-function devices and hence their presence at func 0
>>>>> +	 * may cause guests to not scan the other functions.
>>>>
>>>> So your reading of the original commit is that whatever the issue it was, only function zero was causing the problem? In other words, you are not concerned that pci_scan_slot() may now look at function 1 and skip all higher-numbered function (assuming the problem is still there)?
>>> I'm not sure I understand the question: Whether to look at higher numbered
>>> slots is a function of slot 0's multi-function bit alone, aiui. IOW if
>>> slot 1 is being looked at in the first place, slots 2-7 should also be
>>> looked at.
>>
>>
>> Wasn't the original patch describing a problem strictly as one for single-function devices, so the multi-function bit is not set? I.e. if all VFs (which are single-function devices) are placed in the same slot then pci_scan_slot() would only look at function 0 and ignore anything higher-numbered.
>>
>>
>> My question is whether it would "only look at function 0 and ignore anything higher-numbered" or "only look at the lowest-numbered function and ignore anything higher-numbered".
> 
> The common scanning logic is to look at slot 0 first. If that's populated,
> other slots get looked at only if slot 0 has the multi-function bit set.
> If slot 0 is not populated, nothing is known about the other slots, and
> hence they need to be scanned.

In particular Linux'es next_fn() ends with

	/* dev may be NULL for non-contiguous multifunction devices */
	if (!dev || dev->multifunction)
		return (fn + 1) % 8;

	return 0;

Jan

Re: [PATCH v2 1/2] xen-pciback: redo VF placement in the virtual topology

[Boris Ostrovsky]

On 5/20/21 10:46 AM, Jan Beulich wrote:
> On 20.05.2021 16:44, Jan Beulich wrote:
>> On 20.05.2021 16:38, Boris Ostrovsky wrote:
>>> On 5/20/21 3:43 AM, Jan Beulich wrote:
>>>> On 20.05.2021 02:36, Boris Ostrovsky wrote:
>>>>> On 5/18/21 12:13 PM, Jan Beulich wrote:
>>>>>>  
>>>>>> @@ -95,22 +95,25 @@ static int __xen_pcibk_add_pci_dev(struc
>>>>>>  
>>>>>>  	/*
>>>>>>  	 * Keep multi-function devices together on the virtual PCI bus, except
>>>>>> -	 * virtual functions.
>>>>>> +	 * that we want to keep virtual functions at func 0 on their own. They
>>>>>> +	 * aren't multi-function devices and hence their presence at func 0
>>>>>> +	 * may cause guests to not scan the other functions.
>>>>> So your reading of the original commit is that whatever the issue it was, only function zero was causing the problem? In other words, you are not concerned that pci_scan_slot() may now look at function 1 and skip all higher-numbered function (assuming the problem is still there)?
>>>> I'm not sure I understand the question: Whether to look at higher numbered
>>>> slots is a function of slot 0's multi-function bit alone, aiui. IOW if
>>>> slot 1 is being looked at in the first place, slots 2-7 should also be
>>>> looked at.
>>>
>>> Wasn't the original patch describing a problem strictly as one for single-function devices, so the multi-function bit is not set? I.e. if all VFs (which are single-function devices) are placed in the same slot then pci_scan_slot() would only look at function 0 and ignore anything higher-numbered.
>>>
>>>
>>> My question is whether it would "only look at function 0 and ignore anything higher-numbered" or "only look at the lowest-numbered function and ignore anything higher-numbered".
>> The common scanning logic is to look at slot 0 first. If that's populated,
>> other slots get looked at only if slot 0 has the multi-function bit set.
>> If slot 0 is not populated, nothing is known about the other slots, and
>> hence they need to be scanned.
> In particular Linux'es next_fn() ends with
>
> 	/* dev may be NULL for non-contiguous multifunction devices */
> 	if (!dev || dev->multifunction)
> 		return (fn + 1) % 8;
>
> 	return 0;



Ah yes.


Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

[PATCH v2 2/2] xen-pciback: reconfigure also from backend watch handler

[Jan Beulich]

When multiple PCI devices get assigned to a guest right at boot, libxl
incrementally populates the backend tree. The writes for the first of
the devices trigger the backend watch. In turn xen_pcibk_setup_backend()
will set the XenBus state to Initialised, at which point no further
reconfigures would happen unless a device got hotplugged. Arrange for
reconfigure to also get triggered from the backend watch handler.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Cc: stable@vger.kernel.org
---
v2: Also move comment. Add a comment.
---
I will admit that this isn't entirely race-free (with the guest actually
attaching in parallel), but from the looks of it such a race ought to be
benign (not the least thanks to the mutex). Ideally the tool stack
wouldn't write num_devs until all devices had their information
populated. I tried doing so in libxl, but xen_pcibk_setup_backend()
calling xenbus_dev_fatal() when not being able to read that node
prohibits such an approach (or else libxl and driver changes would need
to go into use in lock-step).

I wonder why what is being watched isn't just the num_devs node. Right
now the watch triggers quite frequently without anything relevant
actually having changed (I suppose in at least some cases in response
to writes by the backend itself).

--- a/drivers/xen/xen-pciback/xenbus.c
+++ b/drivers/xen/xen-pciback/xenbus.c
@@ -359,7 +359,8 @@ out:
 	return err;
 }
 
-static int xen_pcibk_reconfigure(struct xen_pcibk_device *pdev)
+static int xen_pcibk_reconfigure(struct xen_pcibk_device *pdev,
+				 enum xenbus_state state)
 {
 	int err = 0;
 	int num_devs;
@@ -373,9 +374,7 @@ static int xen_pcibk_reconfigure(struct
 	dev_dbg(&pdev->xdev->dev, "Reconfiguring device ...\n");
 
 	mutex_lock(&pdev->dev_lock);
-	/* Make sure we only reconfigure once */
-	if (xenbus_read_driver_state(pdev->xdev->nodename) !=
-	    XenbusStateReconfiguring)
+	if (xenbus_read_driver_state(pdev->xdev->nodename) != state)
 		goto out;
 
 	err = xenbus_scanf(XBT_NIL, pdev->xdev->nodename, "num_devs", "%d",
@@ -500,6 +499,10 @@ static int xen_pcibk_reconfigure(struct
 		}
 	}
 
+	if (state != XenbusStateReconfiguring)
+		/* Make sure we only reconfigure once. */
+		goto out;
+
 	err = xenbus_switch_state(pdev->xdev, XenbusStateReconfigured);
 	if (err) {
 		xenbus_dev_fatal(pdev->xdev, err,
@@ -525,7 +528,7 @@ static void xen_pcibk_frontend_changed(s
 		break;
 
 	case XenbusStateReconfiguring:
-		xen_pcibk_reconfigure(pdev);
+		xen_pcibk_reconfigure(pdev, XenbusStateReconfiguring);
 		break;
 
 	case XenbusStateConnected:
@@ -664,6 +667,15 @@ static void xen_pcibk_be_watch(struct xe
 		xen_pcibk_setup_backend(pdev);
 		break;
 
+	case XenbusStateInitialised:
+		/*
+		 * We typically move to Initialised when the first device was
+		 * added. Hence subsequent devices getting added may need
+		 * reconfiguring.
+		 */
+		xen_pcibk_reconfigure(pdev, XenbusStateInitialised);
+		break;
+
 	default:
 		break;
 	}

Re: [PATCH v2 2/2] xen-pciback: reconfigure also from backend watch handler

[Boris Ostrovsky]

On 5/18/21 12:14 PM, Jan Beulich wrote:
> When multiple PCI devices get assigned to a guest right at boot, libxl
> incrementally populates the backend tree. The writes for the first of
> the devices trigger the backend watch. In turn xen_pcibk_setup_backend()
> will set the XenBus state to Initialised, at which point no further
> reconfigures would happen unless a device got hotplugged. Arrange for
> reconfigure to also get triggered from the backend watch handler.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Cc: stable@vger.kernel.org


Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Re: [PATCH v2 0/2] xen-pciback: a fix and a workaround

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 819 bytes --]

On 18.05.21 18:12, Jan Beulich wrote:
> The first change completes a several years old but still incomplete
> change. As mentioned there, reverting the original change may also
> be an option. The second change works around some odd libxl behavior,
> as described in [1]. As per a response to that mail addressing the
> issue in libxl may also be possible, but it's not clear to me who
> would get to doing so at which point in time. Hence the kernel side
> alternative is being proposed here.
> 
> As to Konrad being on the Cc list: I find it puzzling that he's
> listed under "XEN PCI SUBSYSTEM", but pciback isn't considered part
> of this.
> 
> 1: redo VF placement in the virtual topology
> 2: reconfigure also from backend watch handler

Series pushed to xen/tip.git for-linus-5.13b


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]