On 05.08.2021 16:57, Jason Andryuk wrote: > On Thu, Aug 5, 2021 at 2:20 AM Jan Beulich <jbeul...@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
