| Message ID | 20210804221749.56320-1-jandryuk@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libxl: Fix stubdom PCI passthrough | expand |
On 05.08.2021 00:17, Jason Andryuk wrote: > commit 0fdb48ffe7a1 "libxl: Make sure devices added by pci-attach are > reflected in the config" This should be in a Fixes: tag; whether you fully spell out the reference here or instead refer to that tag would by up to you. > broken stubdom PCI passthrough when it moved > libxl__create_pci_backend later in the function. xl pci-attach for a > running PV domain may also have been broken, but that was not verified. > > The stubdomain is running (!starting) and PV, so it calls > libxl__wait_for_backend. With the new placement of > libxl__create_pci_backend, the path does not exist and the call > immediately fails. > libxl: error: libxl_device.c:1388:libxl__wait_for_backend: Backend /local/domain/0/backend/pci/43/0 does not exist > libxl: error: libxl_pci.c:1764:device_pci_add_done: Domain 42:libxl__device_pci_add failed for PCI device 0:2:0.0 (rc -3) > libxl: error: libxl_create.c:1857:domcreate_attach_devices: Domain 42:unable to add pci devices > > The wait is only relevant when the backend is already present. Use the > read on num_devs to decide whether the backend exists and therefore the > wait is needed. But the presence of the node is not an indication of the backend existing, is it? libxl__device_pci_add_xenstore() itself writes the node a few lines down from your change, so upon processing a 2nd device the function would still behave as it does now. Jan > This restores starting an HVM w/ linux stubdom and PCI > passthrough. > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > --- > Looks like this should be backported to 4.15, but I have not tested. > --- > tools/libs/light/libxl_pci.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c > index 1a1c263080..19daf1d4ee 100644 > --- a/tools/libs/light/libxl_pci.c > +++ b/tools/libs/light/libxl_pci.c > @@ -157,8 +157,10 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, > if (domtype == LIBXL_DOMAIN_TYPE_INVALID) > return ERROR_FAIL; > > - if (!starting && domtype == LIBXL_DOMAIN_TYPE_PV) { > - if (libxl__wait_for_backend(gc, be_path, GCSPRINTF("%d", XenbusStateConnected)) < 0) > + /* wait is only needed if the backend already exists (num_devs != NULL) */ > + if (num_devs && !starting && domtype == LIBXL_DOMAIN_TYPE_PV) { > + if (libxl__wait_for_backend(gc, be_path, > + GCSPRINTF("%d", XenbusStateConnected)) < 0) > return ERROR_FAIL; > } > >
On Thu, Aug 5, 2021 at 2:20 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 05.08.2021 00:17, Jason Andryuk wrote: > > commit 0fdb48ffe7a1 "libxl: Make sure devices added by pci-attach are > > reflected in the config" > > This should be in a Fixes: tag; whether you fully spell out the > reference here or instead refer to that tag would by up to you. Yes, you are correct. Thanks. > > broken stubdom PCI passthrough when it moved > > libxl__create_pci_backend later in the function. xl pci-attach for a > > running PV domain may also have been broken, but that was not verified. > > > > The stubdomain is running (!starting) and PV, so it calls > > libxl__wait_for_backend. With the new placement of > > libxl__create_pci_backend, the path does not exist and the call > > immediately fails. > > libxl: error: libxl_device.c:1388:libxl__wait_for_backend: Backend /local/domain/0/backend/pci/43/0 does not exist > > libxl: error: libxl_pci.c:1764:device_pci_add_done: Domain 42:libxl__device_pci_add failed for PCI device 0:2:0.0 (rc -3) > > libxl: error: libxl_create.c:1857:domcreate_attach_devices: Domain 42:unable to add pci devices > > > > The wait is only relevant when the backend is already present. Use the > > read on num_devs to decide whether the backend exists and therefore the > > wait is needed. > > But the presence of the node is not an indication of the backend existing, > is it? libxl__device_pci_add_xenstore() itself writes the node a few lines > down from your change, so upon processing a 2nd device the function would > still behave as it does now. The way this code is written, num_devs is an indicator. As you say, it's used to control if the overall backend is created. When the backend is created without num_devs, Linux xen-pciback closes the front/back with "Error reading number of devices". I also tried adding a new libxl__create_pci_backend() call which wrote num_devs "0", but that failed with some error I did not write down. Although I may have messed that up by not executing the transaction. When libxl__device_pci_add_xenstore() runs a second time, the wait is fine because the backend exists. I just tested to confirm. Testing `xl create` for HVM w/ Linux stubdom and 2 PCI devices shows the wait's watch trigger for Reconfiguring and Reconfigured before it settles back to Connected. The point of the wait is to let the front/back finish any Reconfiguring for a running domain before a new attachment is initiated. If we have to create the backend, then the wait is unnecessary - a non-existant backend cannot be Reconfiguring. The function already changes behavior depending on the num_devs node. When num_devs doesn't exist, it creates the backend. That is why I went with an additional parameter and comment. Would you like an expansion of the commit message with something like: """ The wait is only relevant when the backend is already present. num_devs is already used to determine if the backend needs to be created. Re-use num_devs to determine if the backend wait is necessary. The wait is necessary to avoid racing with another PCI attachment reconfiguring the front/back. If we are creating the backend, then we don't have to worry about a racing reconfigure. """ And having written that, two "1st" `xl pci-attach` could maybe race for a stubdom since there is no backend? They would both try to write the same nodes, so only 1 would take effect. I guess that is okay. Non-stubdom takes libxl__lock_domain_userdata(), so it should be okay. Regards, Jason > Jan > > > This restores starting an HVM w/ linux stubdom and PCI > > passthrough. > > > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > > --- > > Looks like this should be backported to 4.15, but I have not tested. > > --- > > tools/libs/light/libxl_pci.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c > > index 1a1c263080..19daf1d4ee 100644 > > --- a/tools/libs/light/libxl_pci.c > > +++ b/tools/libs/light/libxl_pci.c > > @@ -157,8 +157,10 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, > > if (domtype == LIBXL_DOMAIN_TYPE_INVALID) > > return ERROR_FAIL; > > > > - if (!starting && domtype == LIBXL_DOMAIN_TYPE_PV) { > > - if (libxl__wait_for_backend(gc, be_path, GCSPRINTF("%d", XenbusStateConnected)) < 0) > > + /* wait is only needed if the backend already exists (num_devs != NULL) */ > > + if (num_devs && !starting && domtype == LIBXL_DOMAIN_TYPE_PV) { > > + if (libxl__wait_for_backend(gc, be_path, > > + GCSPRINTF("%d", XenbusStateConnected)) < 0) > > return ERROR_FAIL; > > } > > > > >
On 05.08.2021 16:57, Jason Andryuk wrote: > On Thu, Aug 5, 2021 at 2:20 AM Jan Beulich <jbeulich@suse.com> wrote: >> >> On 05.08.2021 00:17, Jason Andryuk wrote: >>> commit 0fdb48ffe7a1 "libxl: Make sure devices added by pci-attach are >>> reflected in the config" >> >> This should be in a Fixes: tag; whether you fully spell out the >> reference here or instead refer to that tag would by up to you. > > Yes, you are correct. Thanks. > >>> broken stubdom PCI passthrough when it moved >>> libxl__create_pci_backend later in the function. xl pci-attach for a >>> running PV domain may also have been broken, but that was not verified. >>> >>> The stubdomain is running (!starting) and PV, so it calls >>> libxl__wait_for_backend. With the new placement of >>> libxl__create_pci_backend, the path does not exist and the call >>> immediately fails. >>> libxl: error: libxl_device.c:1388:libxl__wait_for_backend: Backend /local/domain/0/backend/pci/43/0 does not exist >>> libxl: error: libxl_pci.c:1764:device_pci_add_done: Domain 42:libxl__device_pci_add failed for PCI device 0:2:0.0 (rc -3) >>> libxl: error: libxl_create.c:1857:domcreate_attach_devices: Domain 42:unable to add pci devices >>> >>> The wait is only relevant when the backend is already present. Use the >>> read on num_devs to decide whether the backend exists and therefore the >>> wait is needed. >> >> But the presence of the node is not an indication of the backend existing, >> is it? libxl__device_pci_add_xenstore() itself writes the node a few lines >> down from your change, so upon processing a 2nd device the function would >> still behave as it does now. > > The way this code is written, num_devs is an indicator. As you say, > it's used to control if the overall backend is created. When the > backend is created without num_devs, Linux xen-pciback closes the > front/back with "Error reading number of devices". I also tried > adding a new libxl__create_pci_backend() call which wrote num_devs > "0", but that failed with some error I did not write down. Although I > may have messed that up by not executing the transaction. > > When libxl__device_pci_add_xenstore() runs a second time, the wait is > fine because the backend exists. Ah yes, now I recall, because I stumbled over this unhelpful behavior a couple of months ago. As mentioned back then I think it is wrong to create the backend after adding just the first device; it would better be created once all devices have got populated to xenstore. At the time I did submit a kernel side patch to deal with the odd behavior. This has gone in upstream in the meantime, but I would have preferred if the issue would (also) have been addressed in libxl. > I just tested to confirm. Testing > `xl create` for HVM w/ Linux stubdom and 2 PCI devices shows the > wait's watch trigger for Reconfiguring and Reconfigured before it > settles back to Connected. > > The point of the wait is to let the front/back finish any > Reconfiguring for a running domain before a new attachment is > initiated. If we have to create the backend, then the wait is > unnecessary - a non-existant backend cannot be Reconfiguring. The > function already changes behavior depending on the num_devs node. > When num_devs doesn't exist, it creates the backend. That is why I > went with an additional parameter and comment. > > Would you like an expansion of the commit message with something like: > """ > The wait is only relevant when the backend is already present. > num_devs is already used to determine if the backend needs to be > created. Re-use num_devs to determine if the backend wait is > necessary. The wait is necessary to avoid racing with another PCI > attachment reconfiguring the front/back. If we are creating the > backend, then we don't have to worry about a racing reconfigure. > """ Might help, yes. Jan
diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c index 1a1c263080..19daf1d4ee 100644 --- a/tools/libs/light/libxl_pci.c +++ b/tools/libs/light/libxl_pci.c @@ -157,8 +157,10 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, if (domtype == LIBXL_DOMAIN_TYPE_INVALID) return ERROR_FAIL; - if (!starting && domtype == LIBXL_DOMAIN_TYPE_PV) { - if (libxl__wait_for_backend(gc, be_path, GCSPRINTF("%d", XenbusStateConnected)) < 0) + /* wait is only needed if the backend already exists (num_devs != NULL) */ + if (num_devs && !starting && domtype == LIBXL_DOMAIN_TYPE_PV) { + if (libxl__wait_for_backend(gc, be_path, + GCSPRINTF("%d", XenbusStateConnected)) < 0) return ERROR_FAIL; }
commit 0fdb48ffe7a1 "libxl: Make sure devices added by pci-attach are reflected in the config" broken stubdom PCI passthrough when it moved libxl__create_pci_backend later in the function. xl pci-attach for a running PV domain may also have been broken, but that was not verified. The stubdomain is running (!starting) and PV, so it calls libxl__wait_for_backend. With the new placement of libxl__create_pci_backend, the path does not exist and the call immediately fails. libxl: error: libxl_device.c:1388:libxl__wait_for_backend: Backend /local/domain/0/backend/pci/43/0 does not exist libxl: error: libxl_pci.c:1764:device_pci_add_done: Domain 42:libxl__device_pci_add failed for PCI device 0:2:0.0 (rc -3) libxl: error: libxl_create.c:1857:domcreate_attach_devices: Domain 42:unable to add pci devices The wait is only relevant when the backend is already present. Use the read on num_devs to decide whether the backend exists and therefore the wait is needed. This restores starting an HVM w/ linux stubdom and PCI passthrough. Signed-off-by: Jason Andryuk <jandryuk@gmail.com> --- Looks like this should be backported to 4.15, but I have not tested. --- tools/libs/light/libxl_pci.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)