[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] libxl: Fix stubdom PCI passthrough

  • To: Jason Andryuk <jandryuk@xxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Thu, 5 Aug 2021 17:38:49 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=u9WK7Uny8iP6bvLYMQuGW8UNEeilR7najs7REoLXaWk=; b=cc5mKoCFepW1j7TPhWqFuXOXy8TTAHdx2UYdgyHiRvIuwzoToWuV9JoO1iouQDDY5gyfGIqrMm0i5FAKvNpe0hnneesmxgUFW0fnflp4F/VaLPZ2SRdp2dCw9UiD8NEfy5lrzACLxjmsmmY0nDwkdh0Zoi/10aIvQm50tIAbtaA1vjpAV8L6nhAFSCblZL4bKktSpC33GCl+l7ihM7BP2b2aFZwl884c4BhWXkrx59fMicbQByz1kRvckk7nTGdnYpsIGvULuwyfuBrKYRy+wZ6FCKMPL7EwFL+a6pjlWgMv2GihIcVAx7ylY7aDRJLBxXKsrrGjcnMWfmVIHKaubQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=McJMRMhzq3ehLyYJscO6HPhDcefRuo6VSegKgOQJ3siQeATw3rK9nbfo40H09kTo8e08SEdngO2qyYLjw6u3O7OBOJxcEb/OUVlBxr38JZ5OBtqtQrsLrI2/SBxw3Ger+ScRj5A/4p8s/KLRVceOii/3KQk+n++Znawq+Vmpjq2OGeJBQQzO4pjend85buB+nI1ffNqoSIPji1br3qWFyqNFHYCPc5IJTuFI99Uzqk89O1gxXo/GX7Md0+t7xCCEgiZt9jNwZwrmVD6dplSnOQnvyaUWlLCnbG/3gtJ7J87OBsxJCMAU+QRIe/vRPlVkOSBvSZWXBSC9LqS1XCA8Pw==
  • Authentication-results: lists.xenproject.org; dkim=none (message not signed) header.d=none;lists.xenproject.org; dmarc=none action=none header.from=suse.com;
  • Cc: Paul Durrant <pdurrant@xxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Anthony PERARD <anthony.perard@xxxxxxxxxx>, Juergen Gross <jgross@xxxxxxxx>, xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Thu, 05 Aug 2021 15:39:03 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 05.08.2021 16:57, Jason Andryuk wrote:
> 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.

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.




Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.