Re: [PATCH] libxl: Fix stubdom PCI passthrough

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Thu, Aug 5, 2021 at 2:20 AM Jan Beulich <jbeulich@xxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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;
> >      }
> >
> >
>