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

Re: [PATCH v5 05/14] vpci: add hooks for PCI device assign/de-assign


  • To: Oleksandr Andrushchenko <andr2000@xxxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Wed, 12 Jan 2022 13:12:12 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.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-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=eDYg8WEqjw17gvw6eD819gI8IPywABDXoVDPovaRbPo=; b=GAPJt3y378kww2nn0vT9uE1eZe+ONP22bjblUxNtJVuZdSg+XYJVgI7AAEXQ4gMq4Q93I5y9LyFh3V3rfVC6f0yLKPxcyMoZn2DASv2iANjYbIloVMf7pmvA9dOqTTIXNFo0Jwoq6hdv/eQjQo2kugR52Lib+bMWmPp7R78Z9ZUwE8B5gNTRnE3yV5yUX1YeC+Diwrr4FdPDNK9dFKyZsoPlIwVK4APEevvK/SS4XGaysv/wM9l+APvJsOBWlIhUXyElUiHcxZbxE4lWsN7XwWubPJGzpSCgM8jFV4wqGbeMOcYv9bBTaHIWs0dovWakmsjI9JUcHcSHv5N/riVjZQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=gRj/jj6u0RW1fKPxa0hTfygYTnkSQyh+vuMO7IZcxehGPOlIfA3PCgh0t+o8bviPW+P/ZVhjcH1zcWiQFljmaOBI2TMsQNSPlBJymuP6GO8OFd8QKCqSZ9/HEqxoPftfJefTOssqBCUabJJXeFXzvUxVMczR2s+l1hTUyjVx9ye0aq16HWG+kBXWwqXiKyqZfGnQH0v9HdwabT5LOXkh3FrHNSvV5hJRCBWdfeEQxjm+ck4y4TDpWPsx2ybNjH+izw0gRmebx6JKbp4039v2+/nswpgsKkjVF555CHxhe+redMi5Ezn6SZ1ZGtdiJSNB3kpNUIJmsYOo1KapHCtUIA==
  • Authentication-results: esa3.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: <xen-devel@xxxxxxxxxxxxxxxxxxxx>, <julien@xxxxxxx>, <sstabellini@xxxxxxxxxx>, <oleksandr_tyshchenko@xxxxxxxx>, <volodymyr_babchuk@xxxxxxxx>, <Artem_Mygaiev@xxxxxxxx>, <jbeulich@xxxxxxxx>, <andrew.cooper3@xxxxxxxxxx>, <george.dunlap@xxxxxxxxxx>, <paul@xxxxxxx>, <bertrand.marquis@xxxxxxx>, <rahul.singh@xxxxxxx>, Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
  • Delivery-date: Wed, 12 Jan 2022 12:12:39 +0000
  • Ironport-data: A9a23:hkArd6POITJE0TDvrR1lkcFynXyQoLVcMsEvi/4bfWQNrUorhWEOz TZJD2GGaf6DMWr2LohyO4jg9EtU7MTQzYBgTAto+SlhQUwRpJueD7x1DKtR0wB+jCHnZBg6h ynLQoCYdKjYdpJYz/uUGuCJQUNUjMlkfZKhTr6UUsxNbVU8En150Es5w7RRbrNA2rBVPSvc4 bsenOWHULOV82Yc3rU8sv/rRLtH5ZweiRtA1rAMTakjUGz2zhH5OKk3N6CpR0YUd6EPdgKMq 0Qv+5nilo/R109F5tpICd8XeGVSKlLZFVDmZna7x8FOK/WNz8A/+v9TCRYSVatYoxirretjy ZJEiaGLbw0jDPyPm+E2DSANRkmSPYUekFPGCX22sMjVxEzaaXr8hf5pCSnaP6VBpLwxWzsXs 6VFdnZdNXhvhMrvqF6/YvNrick5atHiIasUu216zCGfBvEjKXzGa/uTtYUFh25r7ixINbGDd 9REU2doVg3NSCxLY0coKJ1hkt790xETdBUH8QnI9MLb+VP71AVs1JD9PdyTfcaFLe1XlEuFo mPN/0ziHwoXcteYzFKt22iwi+r4uDL0UYMfCpW17vdvxlaUwwQ7DxkbVkCyp/WjvUe4V8hCM Ewf+icorq8a+VSiS5/2WBjQiGSNvgMYHcFRFeI6wAiXz+zf5APxLmIJVCJbYdoq8so/XyU31 0ShlsnsQzdotdW9S2+Z97qShSO/P24SN2BqTT8JS04J7sfupKk3jwnTVZBzHaitlNr3FDrsh TeQo0AWjrMfl5RTj/2T8lXOgjbqrZ/MJiYr4QHQUnOg/xlOboevbIy16nDW9f9Fao2eSzGpu 3wJmNOX6uwUOo2cjyyGQOgLH7aB6u6MNXvXhlsHN4I66z2n9nqnfIZRyDJzPkFkNoADYzCBX aPIkVoPvtkJZiLsNPIpJdLqYyg38UT+PfnHZMvGb51HXqVKUgWNznAtZku7gVm4xSDAjpoDE ZucdM+tC1MTBqJm0Ce6So8h7FM7+swt7TiNHM6mlnxLxZLbPSfIEuldbDNie8hktPvsnenDz zpI2yJmIT17Wfa2XCTY+JV7wbsifSliXsCeRyC6m4e+zuta9IMJV665LVAJIdUNc0FpegHgp CDVtqhwkguXuJE/AV/WAk2PkZu2NXqFkVo1PDY3IXGj0GU5bICk4c83LsVrJOh9qrIzlaEvH pHpnvls5NwVG1wrHBxHPPHAQHFKLkz31WpiwQL4CNTAQ3KQb1OQoYK1Fuce3CIPEjC2paMDT 06IjWvmrW44b106Vq7+Mav3p3vo5CR1sL8sAyPgf4cCEG2xoNkCA3Gg1ZcffpBTQSgvMxPHj W569z9C+7mUy2L0mfGU7Z25Q3CBSLohThEETjiCvd5b90DypwKe/GOJa87RFRj1X2Lo4qSyI +JTyvD3Kvocm1hW9YF7Ft5WIWgWvbMDfpdWkVZpGmvldVOuBu8yK3WKx5AX5KZM2qVYqU29X UfWootWPrCAOcXEFl8NJVV6MrTfhK9MwjSCv+4oJEja5TNs+ObVW0tlIBTR2jdWK6F4Md15z L556tIW8QG2ljEjLs2C0nJP722JI3FZC/cnu5gWDZXFkA0uzl0eM5XQBjWvuMOEaslWM1lsK TiR3fKQi7NZz0vEUnwyCXmSgrYN2cVQ4EhHlQZQKU6Il9zJgu4M8CdQqTlnHB5Iyhhn0v5oP jQ5PUNCOqjTrSxjg9JOXj7wFlgZVgGZ4EH413AAiHbdExuzTmXIIWAwZbSN8UQe/z4OdzRX5 ujFmmPsUDKsd8DtxCoiH0VirqW7H9B28wTDnuGhHtiEQMZmMWa03Pf2aDpasQbjDOMwmFbD9 Ltj8+tHYKHmMTId/v8gAI6A2LVMEB2JKQSumx26EH/lyY0ERAyP5A==
  • Ironport-hdrordr: A9a23:d//KBq4aVwFNPJZZ1APXwSyBI+orL9Y04lQ7vn2ZFiY6TiXIra +TdaoguSMc6AxwZJkh8erwXpVoZUmsiKKdgLNhR4tKOTOGhILGFvAG0WKP+UyFJ8S6zJ8g6U 4CSdkONDSTNykDsS+S2mDReLxMsbr3kpxAx92utEuFJTsaFZ2IhD0JczpzfHcGIzWvUvECZe WhD4d81nGdUEVSSv7+KmgOXuDFqdGOvJX6YSQeDxpizAWVlzun5JPzDhDdh34lIn5y6IZn1V KAvx3y562lvf3+4hjA11XL55ATvNf60NNMCOGFl8BQADTxjQSDYphnRtS5zXoIidDqzGxvvM jHoh8mMcg2w3TNflutqR+o4AXk2CZG0Q6V9XaoxV/Y5eDpTjMzDMRMwahDdAHC1kYmtNZglI pWwmOwrfNsfF39tRW4w+KNewBhl0Kyr3Znu/UUlWZjXYwXb6IUhZAD/XlSDIwLEEvBmc8a+d FVfYHhDcttABCnhyizhBgs/DXsZAV+Iv6+eDlChiTPuAIm2UyQzCMjtbsidzk7hdYAoqJ/lp f525JT5cVzp/8tHNJA7dg6MLmK40z2MFvx2TGpUBza/J9uAQO5l3ew2sRz2N2X
  • Ironport-sdr: KKhmlUQe3zZxv8iehL/2OSRWsostTEeeGpDfSsWPYzWo85CPITNnzdgAbW5VLPYb+oc5nupYse 6KeSlIlmXzi2RcI8ulKophRrSt66Yfvybz27m1oyeR3ZInhUE1rWhpFCVN6hlKWBOlnicXicOU Od4s8RhspnD6o9prcHbqMi3jOS85JQZMCvkS+12D/txTsv1cGbKbHbpFvFeiN4kFSB0TTyO9NI Uy9mCC7iQPFvcilDWCDOs6Bd2AkwSU0OvnB1QRLi3MIS2ZK6OblPHE3qIBLUPkrb6EzjdToT5n cHiUX14UETGduj/CKDxbDZxV
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Thu, Nov 25, 2021 at 01:02:42PM +0200, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> 
> When a PCI device gets assigned/de-assigned some work on vPCI side needs
> to be done for that device. Introduce a pair of hooks so vPCI can handle
> that.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> ---
> Since v4:
>  - de-assign vPCI from the previous domain on device assignment
>  - do not remove handlers in vpci_assign_device as those must not
>    exist at that point
> Since v3:
>  - remove toolstack roll-back description from the commit message
>    as error are to be handled with proper cleanup in Xen itself
>  - remove __must_check
>  - remove redundant rc check while assigning devices
>  - fix redundant CONFIG_HAS_VPCI check for CONFIG_HAS_VPCI_GUEST_SUPPORT
>  - use REGISTER_VPCI_INIT machinery to run required steps on device
>    init/assign: add run_vpci_init helper
> Since v2:
> - define CONFIG_HAS_VPCI_GUEST_SUPPORT so dead code is not compiled
>   for x86
> Since v1:
>  - constify struct pci_dev where possible
>  - do not open code is_system_domain()
>  - extended the commit message
> ---
>  xen/drivers/Kconfig           |  4 +++
>  xen/drivers/passthrough/pci.c | 10 ++++++
>  xen/drivers/vpci/vpci.c       | 61 +++++++++++++++++++++++++++++------
>  xen/include/xen/vpci.h        | 16 +++++++++
>  4 files changed, 82 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/drivers/Kconfig b/xen/drivers/Kconfig
> index db94393f47a6..780490cf8e39 100644
> --- a/xen/drivers/Kconfig
> +++ b/xen/drivers/Kconfig
> @@ -15,4 +15,8 @@ source "drivers/video/Kconfig"
>  config HAS_VPCI
>       bool
>  
> +config HAS_VPCI_GUEST_SUPPORT
> +     bool
> +     depends on HAS_VPCI
> +
>  endmenu
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 286808b25e65..d9ef91571adf 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -874,6 +874,10 @@ static int deassign_device(struct domain *d, uint16_t 
> seg, uint8_t bus,
>      if ( ret )
>          goto out;
>  
> +    ret = vpci_deassign_device(d, pdev);
> +    if ( ret )
> +        goto out;

Following my comment below, this won't be allowed to fail.

> +
>      if ( pdev->domain == hardware_domain  )
>          pdev->quarantine = false;
>  
> @@ -1429,6 +1433,10 @@ static int assign_device(struct domain *d, u16 seg, u8 
> bus, u8 devfn, u32 flag)
>      ASSERT(pdev && (pdev->domain == hardware_domain ||
>                      pdev->domain == dom_io));
>  
> +    rc = vpci_deassign_device(pdev->domain, pdev);
> +    if ( rc )
> +        goto done;
> +
>      rc = pdev_msix_assign(d, pdev);
>      if ( rc )
>          goto done;
> @@ -1446,6 +1454,8 @@ static int assign_device(struct domain *d, u16 seg, u8 
> bus, u8 devfn, u32 flag)
>          rc = hd->platform_ops->assign_device(d, devfn, pci_to_dev(pdev), 
> flag);
>      }
>  
> +    rc = vpci_assign_device(d, pdev);
> +
>   done:
>      if ( rc )
>          printk(XENLOG_G_WARNING "%pd: assign (%pp) failed (%d)\n",
> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> index 37103e207635..a9e9e8ec438c 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -74,12 +74,26 @@ void vpci_remove_device(struct pci_dev *pdev)
>      spin_unlock(&pdev->vpci_lock);
>  }
>  
> -int vpci_add_handlers(struct pci_dev *pdev)
> +static int run_vpci_init(struct pci_dev *pdev)

Just using add_handlers as function name would be clearer IMO.

>  {
> -    struct vpci *vpci;
>      unsigned int i;
>      int rc = 0;
>  
> +    for ( i = 0; i < NUM_VPCI_INIT; i++ )
> +    {
> +        rc = __start_vpci_array[i](pdev);
> +        if ( rc )
> +            break;
> +    }
> +
> +    return rc;
> +}
> +
> +int vpci_add_handlers(struct pci_dev *pdev)
> +{
> +    struct vpci *vpci;
> +    int rc;
> +
>      if ( !has_vpci(pdev->domain) )
>          return 0;
>  
> @@ -94,19 +108,48 @@ int vpci_add_handlers(struct pci_dev *pdev)
>      pdev->vpci = vpci;
>      INIT_LIST_HEAD(&pdev->vpci->handlers);
>  
> -    for ( i = 0; i < NUM_VPCI_INIT; i++ )
> -    {
> -        rc = __start_vpci_array[i](pdev);
> -        if ( rc )
> -            break;
> -    }
> -
> +    rc = run_vpci_init(pdev);
>      if ( rc )
>          vpci_remove_device_locked(pdev);
>      spin_unlock(&pdev->vpci_lock);
>  
>      return rc;
>  }
> +
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> +/* Notify vPCI that device is assigned to guest. */
> +int vpci_assign_device(struct domain *d, struct pci_dev *pdev)
> +{
> +    int rc;
> +
> +    /* It only makes sense to assign for hwdom or guest domain. */
> +    if ( is_system_domain(d) || !has_vpci(d) )

Do you really need the is_system_domain check? System domains
shouldn't have the VPCI flag set anyway, so should fail the has_vpci
test.

> +        return 0;
> +
> +    spin_lock(&pdev->vpci_lock);
> +    rc = run_vpci_init(pdev);
> +    spin_unlock(&pdev->vpci_lock);
> +    if ( rc )
> +        vpci_deassign_device(d, pdev);
> +
> +    return rc;
> +}
> +
> +/* Notify vPCI that device is de-assigned from guest. */
> +int vpci_deassign_device(struct domain *d, struct pci_dev *pdev)

There's no need to return any value from this function AFAICT. It
should have void return type.

Thanks, Roger.



 


Rackspace

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