xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] xen-pciback: a fix, a workaround, and some simplification
@ 2021-04-07 14:35 Jan Beulich
  2021-04-07 14:37 ` [PATCH 1/3] xen-pciback: redo VF placement in the virtual topology Jan Beulich
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Jan Beulich @ 2021-04-07 14:35 UTC (permalink / raw)
  To: xen-devel; +Cc: Juergen Gross, Boris Ostrovsky, Konrad Wilk

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. The third change is some
simplification that I noticed would be possible while doing the
other work 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
3: simplify vpci's find hook

Jan

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/3] xen-pciback: redo VF placement in the virtual topology
  2021-04-07 14:35 [PATCH 0/3] xen-pciback: a fix, a workaround, and some simplification Jan Beulich
@ 2021-04-07 14:37 ` Jan Beulich
  2021-04-08 22:28   ` Boris Ostrovsky
  2021-04-07 14:37 ` [PATCH 2/3] xen-pciback: reconfigure also from backend watch handler Jan Beulich
  2021-04-07 14:37 ` [PATCH 3/3] xen-pciback: simplify vpci's find hook Jan Beulich
  2 siblings, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2021-04-07 14:37 UTC (permalink / raw)
  To: xen-devel; +Cc: Juergen Gross, Boris Ostrovsky, Konrad Wilk

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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 2/3] xen-pciback: reconfigure also from backend watch handler
  2021-04-07 14:35 [PATCH 0/3] xen-pciback: a fix, a workaround, and some simplification Jan Beulich
  2021-04-07 14:37 ` [PATCH 1/3] xen-pciback: redo VF placement in the virtual topology Jan Beulich
@ 2021-04-07 14:37 ` Jan Beulich
  2021-04-09 21:43   ` Boris Ostrovsky
  2021-04-07 14:37 ` [PATCH 3/3] xen-pciback: simplify vpci's find hook Jan Beulich
  2 siblings, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2021-04-07 14:37 UTC (permalink / raw)
  To: xen-devel; +Cc: Juergen Gross, Boris Ostrovsky, Konrad Wilk

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
---
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;
@@ -374,8 +375,7 @@ static int xen_pcibk_reconfigure(struct
 
 	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 +500,9 @@ static int xen_pcibk_reconfigure(struct
 		}
 	}
 
+	if (state != XenbusStateReconfiguring)
+		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,10 @@ static void xen_pcibk_be_watch(struct xe
 		xen_pcibk_setup_backend(pdev);
 		break;
 
+	case XenbusStateInitialised:
+		xen_pcibk_reconfigure(pdev, XenbusStateInitialised);
+		break;
+
 	default:
 		break;
 	}



^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 3/3] xen-pciback: simplify vpci's find hook
  2021-04-07 14:35 [PATCH 0/3] xen-pciback: a fix, a workaround, and some simplification Jan Beulich
  2021-04-07 14:37 ` [PATCH 1/3] xen-pciback: redo VF placement in the virtual topology Jan Beulich
  2021-04-07 14:37 ` [PATCH 2/3] xen-pciback: reconfigure also from backend watch handler Jan Beulich
@ 2021-04-07 14:37 ` Jan Beulich
  2021-04-09 21:45   ` Boris Ostrovsky
  2021-04-23  8:05   ` Juergen Gross
  2 siblings, 2 replies; 15+ messages in thread
From: Jan Beulich @ 2021-04-07 14:37 UTC (permalink / raw)
  To: xen-devel; +Cc: Juergen Gross, Boris Ostrovsky, Konrad Wilk

There's no point in comparing SBDF - we can simply compare the struct
pci_dev pointers. If they weren't the same for a given device, we'd have
bigger problems from having stored a stale pointer.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/drivers/xen/xen-pciback/vpci.c
+++ b/drivers/xen/xen-pciback/vpci.c
@@ -235,7 +235,6 @@ static int __xen_pcibk_get_pcifront_dev(
 					unsigned int *devfn)
 {
 	struct pci_dev_entry *entry;
-	struct pci_dev *dev = NULL;
 	struct vpci_dev_data *vpci_dev = pdev->pci_dev_data;
 	int found = 0, slot;
 
@@ -244,11 +243,7 @@ static int __xen_pcibk_get_pcifront_dev(
 		list_for_each_entry(entry,
 			    &vpci_dev->dev_list[slot],
 			    list) {
-			dev = entry->dev;
-			if (dev && dev->bus->number == pcidev->bus->number
-				&& pci_domain_nr(dev->bus) ==
-					pci_domain_nr(pcidev->bus)
-				&& dev->devfn == pcidev->devfn) {
+			if (entry->dev == pcidev) {
 				found = 1;
 				*domain = 0;
 				*bus = 0;



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] xen-pciback: redo VF placement in the virtual topology
  2021-04-07 14:37 ` [PATCH 1/3] xen-pciback: redo VF placement in the virtual topology Jan Beulich
@ 2021-04-08 22:28   ` Boris Ostrovsky
  2021-04-09  8:16     ` Jan Beulich
  0 siblings, 1 reply; 15+ messages in thread
From: Boris Ostrovsky @ 2021-04-08 22:28 UTC (permalink / raw)
  To: Jan Beulich, xen-devel; +Cc: Juergen Gross, Konrad Wilk


On 4/7/21 10:37 AM, Jan Beulich wrote:
> 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.


Perhaps this is no longer a problem, it's been 9 years since that patch. Have you tried reverting to 8a5248fe10b101104d92d01438f918e899414fd1~1 and testing that?


-boris



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] xen-pciback: redo VF placement in the virtual topology
  2021-04-08 22:28   ` Boris Ostrovsky
@ 2021-04-09  8:16     ` Jan Beulich
  0 siblings, 0 replies; 15+ messages in thread
From: Jan Beulich @ 2021-04-09  8:16 UTC (permalink / raw)
  To: Boris Ostrovsky; +Cc: Juergen Gross, Konrad Wilk, xen-devel

On 09.04.2021 00:28, Boris Ostrovsky wrote:
> 
> On 4/7/21 10:37 AM, Jan Beulich wrote:
>> 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.
> 
> 
> Perhaps this is no longer a problem, it's been 9 years since that patch. Have you tried reverting to 8a5248fe10b101104d92d01438f918e899414fd1~1 and testing that?

Well, no, for the simple reason that I don't really understand how that
change was meant to make a difference. Hence while simply reverting may
be an option, it's not something I would want to suggest myself (simply
because I couldn't fully justify doing so).

Jan


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/3] xen-pciback: reconfigure also from backend watch handler
  2021-04-07 14:37 ` [PATCH 2/3] xen-pciback: reconfigure also from backend watch handler Jan Beulich
@ 2021-04-09 21:43   ` Boris Ostrovsky
  2021-04-12  9:44     ` Jan Beulich
  0 siblings, 1 reply; 15+ messages in thread
From: Boris Ostrovsky @ 2021-04-09 21:43 UTC (permalink / raw)
  To: Jan Beulich, xen-devel; +Cc: Juergen Gross, Konrad Wilk


On 4/7/21 10:37 AM, 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
> ---
> 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;
> @@ -374,8 +375,7 @@ static int xen_pcibk_reconfigure(struct
>  
>  	mutex_lock(&pdev->dev_lock);
>  	/* Make sure we only reconfigure once */


Is this comment still valid or should it be moved ...


> -	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 +500,9 @@ static int xen_pcibk_reconfigure(struct
>  		}
>  	}
>  
> +	if (state != XenbusStateReconfiguring)
> +		goto out;
> +


... here?


>  	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,10 @@ static void xen_pcibk_be_watch(struct xe
>  		xen_pcibk_setup_backend(pdev);
>  		break;
>  
> +	case XenbusStateInitialised:
> +		xen_pcibk_reconfigure(pdev, XenbusStateInitialised);


Could you add a comment here that this is needed when multiple devices are added?


It also looks a bit odd that adding a device is now viewed as a reconfiguration. It seems to me that xen_pcibk_setup_backend() and xen_pcibk_reconfigure() really should be merged --- initialization code for both looks pretty much the same.


-boris



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/3] xen-pciback: simplify vpci's find hook
  2021-04-07 14:37 ` [PATCH 3/3] xen-pciback: simplify vpci's find hook Jan Beulich
@ 2021-04-09 21:45   ` Boris Ostrovsky
  2021-04-12  9:50     ` Jan Beulich
  2021-04-23  8:05   ` Juergen Gross
  1 sibling, 1 reply; 15+ messages in thread
From: Boris Ostrovsky @ 2021-04-09 21:45 UTC (permalink / raw)
  To: Jan Beulich, xen-devel; +Cc: Juergen Gross, Konrad Wilk


On 4/7/21 10:37 AM, Jan Beulich wrote:
> There's no point in comparing SBDF - we can simply compare the struct
> pci_dev pointers. If they weren't the same for a given device, we'd have
> bigger problems from having stored a stale pointer.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>


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




^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/3] xen-pciback: reconfigure also from backend watch handler
  2021-04-09 21:43   ` Boris Ostrovsky
@ 2021-04-12  9:44     ` Jan Beulich
  2021-04-12 15:55       ` Boris Ostrovsky
  0 siblings, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2021-04-12  9:44 UTC (permalink / raw)
  To: Boris Ostrovsky; +Cc: Juergen Gross, Konrad Wilk, xen-devel

On 09.04.2021 23:43, Boris Ostrovsky wrote:
> 
> On 4/7/21 10:37 AM, 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
>> ---
>> 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;
>> @@ -374,8 +375,7 @@ static int xen_pcibk_reconfigure(struct
>>  
>>  	mutex_lock(&pdev->dev_lock);
>>  	/* Make sure we only reconfigure once */
> 
> 
> Is this comment still valid or should it be moved ...
> 
> 
>> -	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 +500,9 @@ static int xen_pcibk_reconfigure(struct
>>  		}
>>  	}
>>  
>> +	if (state != XenbusStateReconfiguring)
>> +		goto out;
>> +
> 
> 
> ... here?

Yeah, probably better to move it.

>>  	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,10 @@ static void xen_pcibk_be_watch(struct xe
>>  		xen_pcibk_setup_backend(pdev);
>>  		break;
>>  
>> +	case XenbusStateInitialised:
>> +		xen_pcibk_reconfigure(pdev, XenbusStateInitialised);
> 
> 
> Could you add a comment here that this is needed when multiple devices are added?

Sure.

> It also looks a bit odd that adding a device is now viewed as a reconfiguration. It seems to me that xen_pcibk_setup_backend() and xen_pcibk_reconfigure() really should be merged --- initialization code for both looks pretty much the same.

I thought the same, but didn't want to make more of a change than is
needed to address the problem at hand. Plus of course there's still
the possibility that someone may fix libxl, at which point the change
here (and hence the merging) would become unnecessary.

Jan


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/3] xen-pciback: simplify vpci's find hook
  2021-04-09 21:45   ` Boris Ostrovsky
@ 2021-04-12  9:50     ` Jan Beulich
  2021-04-12 16:05       ` Boris Ostrovsky
  0 siblings, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2021-04-12  9:50 UTC (permalink / raw)
  To: Boris Ostrovsky; +Cc: Juergen Gross, Konrad Wilk, xen-devel

On 09.04.2021 23:45, Boris Ostrovsky wrote:
> 
> On 4/7/21 10:37 AM, Jan Beulich wrote:
>> There's no point in comparing SBDF - we can simply compare the struct
>> pci_dev pointers. If they weren't the same for a given device, we'd have
>> bigger problems from having stored a stale pointer.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> 
> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Thanks. As the 1st patch of this series still looks to have an unclear
disposition (unless not getting back a reply on my responses to your
comments means silent agreement), I can't predict yet when I'd be able
to submit v2. Hence I'd like to point out that this patch is
independent of the former two, and hence would need to wait further if
you wanted to apply it. After all this one (unlike the other two) is
merely cleanup, and hence would rather want to go in during a merge
window.

Jan


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/3] xen-pciback: reconfigure also from backend watch handler
  2021-04-12  9:44     ` Jan Beulich
@ 2021-04-12 15:55       ` Boris Ostrovsky
  0 siblings, 0 replies; 15+ messages in thread
From: Boris Ostrovsky @ 2021-04-12 15:55 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Juergen Gross, Konrad Wilk, xen-devel


On 4/12/21 5:44 AM, Jan Beulich wrote:
>
>> It also looks a bit odd that adding a device is now viewed as a reconfiguration. It seems to me that xen_pcibk_setup_backend() and xen_pcibk_reconfigure() really should be merged --- initialization code for both looks pretty much the same.
> I thought the same, but didn't want to make more of a change than is
> needed to address the problem at hand. Plus of course there's still
> the possibility that someone may fix libxl, at which point the change
> here (and hence the merging) would become unnecessary.


Merging them would be a good thing regardless of fixing libxl. But I agree that it is separate from fixing the issue at hand.


Maybe rename the function? (I myself can't think of a good name).


-boris



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/3] xen-pciback: simplify vpci's find hook
  2021-04-12  9:50     ` Jan Beulich
@ 2021-04-12 16:05       ` Boris Ostrovsky
  2021-04-13  8:09         ` Jan Beulich
  0 siblings, 1 reply; 15+ messages in thread
From: Boris Ostrovsky @ 2021-04-12 16:05 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Juergen Gross, Konrad Wilk, xen-devel


On 4/12/21 5:50 AM, Jan Beulich wrote:
> On 09.04.2021 23:45, Boris Ostrovsky wrote:
>> On 4/7/21 10:37 AM, Jan Beulich wrote:
>>> There's no point in comparing SBDF - we can simply compare the struct
>>> pci_dev pointers. If they weren't the same for a given device, we'd have
>>> bigger problems from having stored a stale pointer.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Thanks. As the 1st patch of this series still looks to have an unclear
> disposition (unless not getting back a reply on my responses to your
> comments means silent agreement),


TBH I have been struggling with understanding both the original problem (just like you have) and the solution you are proposing (apart from making dev_list[] more compact).


>  I can't predict yet when I'd be able
> to submit v2. Hence I'd like to point out that this patch is
> independent of the former two, and hence would need to wait further if
> you wanted to apply it. After all this one (unlike the other two) is
> merely cleanup, and hence would rather want to go in during a merge
> window.


Given that next Sunday may be when 5.12 is released I think everything but stoppers will have to wait for the merge window.


-boris



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/3] xen-pciback: simplify vpci's find hook
  2021-04-12 16:05       ` Boris Ostrovsky
@ 2021-04-13  8:09         ` Jan Beulich
  2021-04-13 12:54           ` Boris Ostrovsky
  0 siblings, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2021-04-13  8:09 UTC (permalink / raw)
  To: Boris Ostrovsky; +Cc: Juergen Gross, Konrad Wilk, xen-devel

On 12.04.2021 18:05, Boris Ostrovsky wrote:
> 
> On 4/12/21 5:50 AM, Jan Beulich wrote:
>> On 09.04.2021 23:45, Boris Ostrovsky wrote:
>>> On 4/7/21 10:37 AM, Jan Beulich wrote:
>>>> There's no point in comparing SBDF - we can simply compare the struct
>>>> pci_dev pointers. If they weren't the same for a given device, we'd have
>>>> bigger problems from having stored a stale pointer.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Thanks. As the 1st patch of this series still looks to have an unclear
>> disposition (unless not getting back a reply on my responses to your
>> comments means silent agreement),
> 
> 
> TBH I have been struggling with understanding both the original problem (just like you have) and the solution you are proposing (apart from making dev_list[] more compact).
> 
> 
>>  I can't predict yet when I'd be able
>> to submit v2. Hence I'd like to point out that this patch is
>> independent of the former two, and hence would need to wait further if
>> you wanted to apply it. After all this one (unlike the other two) is
>> merely cleanup, and hence would rather want to go in during a merge
>> window.
> 
> 
> Given that next Sunday may be when 5.12 is released I think everything but stoppers will have to wait for the merge window.

Oh, I didn't mean it this way. Instead I thought the 3rd patch here could
be pushed to Linus during the merge window, while the other two may be
fine to go his way also during early RCs of 5.13 (giving us some time to
sort what exactly we want to do).

Jan


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/3] xen-pciback: simplify vpci's find hook
  2021-04-13  8:09         ` Jan Beulich
@ 2021-04-13 12:54           ` Boris Ostrovsky
  0 siblings, 0 replies; 15+ messages in thread
From: Boris Ostrovsky @ 2021-04-13 12:54 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Juergen Gross, Konrad Wilk, xen-devel


On 4/13/21 4:09 AM, Jan Beulich wrote:
> On 12.04.2021 18:05, Boris Ostrovsky wrote:
>>
>> Given that next Sunday may be when 5.12 is released I think everything but stoppers will have to wait for the merge window.
> Oh, I didn't mean it this way. Instead I thought the 3rd patch here could
> be pushed to Linus during the merge window, while the other two may be
> fine to go his way also during early RCs of 5.13 (giving us some time to
> sort what exactly we want to do).
>

Ah, yes, that works.


-boris



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/3] xen-pciback: simplify vpci's find hook
  2021-04-07 14:37 ` [PATCH 3/3] xen-pciback: simplify vpci's find hook Jan Beulich
  2021-04-09 21:45   ` Boris Ostrovsky
@ 2021-04-23  8:05   ` Juergen Gross
  1 sibling, 0 replies; 15+ messages in thread
From: Juergen Gross @ 2021-04-23  8:05 UTC (permalink / raw)
  To: Jan Beulich, xen-devel; +Cc: Boris Ostrovsky, Konrad Wilk


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

On 07.04.21 16:37, Jan Beulich wrote:
> There's no point in comparing SBDF - we can simply compare the struct
> pci_dev pointers. If they weren't the same for a given device, we'd have
> bigger problems from having stored a stale pointer.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Pushed to xen/tip.git for-linus-5.13


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2021-04-23  8:05 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-07 14:35 [PATCH 0/3] xen-pciback: a fix, a workaround, and some simplification Jan Beulich
2021-04-07 14:37 ` [PATCH 1/3] xen-pciback: redo VF placement in the virtual topology Jan Beulich
2021-04-08 22:28   ` Boris Ostrovsky
2021-04-09  8:16     ` Jan Beulich
2021-04-07 14:37 ` [PATCH 2/3] xen-pciback: reconfigure also from backend watch handler Jan Beulich
2021-04-09 21:43   ` Boris Ostrovsky
2021-04-12  9:44     ` Jan Beulich
2021-04-12 15:55       ` Boris Ostrovsky
2021-04-07 14:37 ` [PATCH 3/3] xen-pciback: simplify vpci's find hook Jan Beulich
2021-04-09 21:45   ` Boris Ostrovsky
2021-04-12  9:50     ` Jan Beulich
2021-04-12 16:05       ` Boris Ostrovsky
2021-04-13  8:09         ` Jan Beulich
2021-04-13 12:54           ` Boris Ostrovsky
2021-04-23  8:05   ` Juergen Gross

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).