[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3] libxl/PCI: Fix PV hotplug & stubdom coldplug
On Sun, Jan 09, 2022 at 01:04:36PM -0500, Jason Andryuk wrote: > commit 0fdb48ffe7a1 "libxl: Make sure devices added by pci-attach are > reflected in the config" broken PCI hotplug (xl pci-attach) for PV > domains when it moved libxl__create_pci_backend() later in the function. > > This also broke HVM + stubdom PCI passthrough coldplug. For that, the > PCI devices are hotplugged to a running PV stubdom, and then the QEMU > QMP device_add commands are made to QEMU inside the stubdom. > > Are running PV domain calls libxl__wait_for_backend(). With the current > 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. 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 or changing to some other state like closing. If we are > creating the backend, then we don't have to worry about the state since > it is being created. > > Fixes: 0fdb48ffe7a1 ("libxl: Make sure devices added by pci-attach are > reflected in the config") > > Signed-off-by: Jason Andryuk <jandryuk@xxxxxxxxx> > --- > Alternative to Jan's patch: > https://lore.kernel.org/xen-devel/5114ae87-bc0e-3d58-e16e-6d9d2fee0801@xxxxxxxx/ > > v3: > Change title & commit message > > v2: > https://lore.kernel.org/xen-devel/20210812005700.3159-1-jandryuk@xxxxxxxxx/ > Add Fixes > Expand num_devs use in commit message > --- > 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 4c2d7aeefb..e8fd3bd937 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) { It's kind of weird to check whether the "num_devs" key is present or not, but I guess that kind of work. > + if (libxl__wait_for_backend(gc, be_path, > + GCSPRINTF("%d", XenbusStateConnected)) < > 0) So there is something in the coding style document, in "error handling", about how to write this condition. Could you store the return value of libxl__wait_for_backend() into "rc" (because it's a libxl function), then check it and return it? if (rc) return rc; Thanks, -- Anthony PERARD
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |