* [PATCH] libxl: Fix stubdom PCI passthrough
@ 2021-08-04 22:17 Jason Andryuk
2021-08-05 6:20 ` Jan Beulich
0 siblings, 1 reply; 4+ messages in thread
From: Jason Andryuk @ 2021-08-04 22:17 UTC (permalink / raw)
To: xen-devel
Cc: pdurrant, Jason Andryuk, Ian Jackson, Wei Liu, Anthony PERARD,
Juergen Gross
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(-)
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;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] libxl: Fix stubdom PCI passthrough
2021-08-04 22:17 [PATCH] libxl: Fix stubdom PCI passthrough Jason Andryuk
@ 2021-08-05 6:20 ` Jan Beulich
2021-08-05 14:57 ` Jason Andryuk
0 siblings, 1 reply; 4+ messages in thread
From: Jan Beulich @ 2021-08-05 6:20 UTC (permalink / raw)
To: Jason Andryuk
Cc: pdurrant, Ian Jackson, Wei Liu, Anthony PERARD, Juergen Gross, xen-devel
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;
> }
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] libxl: Fix stubdom PCI passthrough
2021-08-05 6:20 ` Jan Beulich
@ 2021-08-05 14:57 ` Jason Andryuk
2021-08-05 15:38 ` Jan Beulich
0 siblings, 1 reply; 4+ messages in thread
From: Jason Andryuk @ 2021-08-05 14:57 UTC (permalink / raw)
To: Jan Beulich
Cc: Paul Durrant, Ian Jackson, Wei Liu, Anthony PERARD,
Juergen Gross, xen-devel
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;
> > }
> >
> >
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] libxl: Fix stubdom PCI passthrough
2021-08-05 14:57 ` Jason Andryuk
@ 2021-08-05 15:38 ` Jan Beulich
0 siblings, 0 replies; 4+ messages in thread
From: Jan Beulich @ 2021-08-05 15:38 UTC (permalink / raw)
To: Jason Andryuk
Cc: Paul Durrant, Ian Jackson, Wei Liu, Anthony PERARD,
Juergen Gross, xen-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-08-05 15:39 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-04 22:17 [PATCH] libxl: Fix stubdom PCI passthrough Jason Andryuk
2021-08-05 6:20 ` Jan Beulich
2021-08-05 14:57 ` Jason Andryuk
2021-08-05 15:38 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.