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

Re: [PATCH v5 11/14] vpci: add initial support for virtual PCI bus topology


  • To: Oleksandr Andrushchenko <andr2000@xxxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Thu, 13 Jan 2022 12:35:34 +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=4KMvU8XdiVKuVXBKPWPYetNs0Q+6GRrLYoAXAy5/bqA=; b=OMvCEI2PltDVyvtcKYFz/siwjPLneuQyN6o9v10BSPSqLy+p/r0OOUyK7qmEZ6sUBFEocfy5OwbIxQoxC8t0kB5xArcgfcV18hE+Qf3Fmfv5RDC+kdUnJuvD04ieVHnVm53LYNyxKhcbOwoEjTLUwmMf+9obetLxqtntXxpIhzNudTIRy+folveRXnkIkZTQuEBRNWjv22fIt+ulavfEf/kRsGZYfy9nlndJliVyfGktXiBpn5AuILX+MRct5eQ87nApSK0fYOmBCRAgfGsPSanKIMP7yamdPO4aKlpiE6hRz+h+8ML1SX3uyFkYfupR7t7ccAUxWd+vKFgFWfMUlw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=aTGzW8+IRhA7Xa1sTA0hqkNiJTJjvFSnLArWuf4gxngcOKM+BNJi9JUvtiEgpNwfHLQNnYqq+x5F7f16RfRuM9aMZVIovpl9cD1u1cWGUkp8EBv1huy6lvqHOvU5mtgGb7mo3G3OYCIEW/o6rNsquwSGGA65t8J6cRHi08GUrj0o9fHXqR2W7175X3MK2J22UMEGybQBjwtWpPgEBu4DMQaaDLi320XvgzWg59Ba1FL32n7dk6aEs4pp7nppToafeoaLZHTytl2M9qAhi9v7TZ76nL/X6KMrINRs3f+udg9QZyY133hV/2iRUOiwRT03ay7cyrpXgVmfeJHRqPZl4A==
  • Authentication-results: esa1.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: Thu, 13 Jan 2022 11:36:05 +0000
  • Ironport-data: A9a23:Unz7I6wIwmyPuj0+Wa96t+f3wCrEfRIJ4+MujC+fZmUNrF6WrkUFy GBJD2+PPfqJN2D1c9h+bo6//RgAuZ7Xy9NhHlRk+SAxQypGp/SeCIXCJC8cHc8zwu4v7q5Dx 59DAjUVBJlsFhcwnvopW1TYhSEUOZugH9IQM8aZfHAhLeNYYH1500g7wrdi2tcAbeWRWGthh /uj+6UzB3f9s9JEGjp8B3Wr8U4HUFza4Vv0j3RmDRx5lAa2e0o9VfrzEZqZPXrgKrS4K8bhL wr1IBNVyUuCl/slIovNfr8W6STmSJaKVeSFoiI+t6RPHnGuD8H9u0o2HKN0VKtZt9mGt9Zo8 M4Oh7utdRYkJKjziuUAcklgCAgraMWq+JefSZS+mcmazkmAeHrw2fR+SkoxOOX0+M4uXzsIr 6ZBbmlQMFbT3Ipaw5riIgVort4kI8TxepsWp1lrzC3DDOZgSpfGK0nPzYECh25t35ARdRrYT 9oSZSBCMhSaWT9wNW9KGJ1nht6mq1CqJlW0r3rK/PFqsgA/1jdZyLHwNPLFd9rMQt9a9m6ar G/b+2XyAjkBKceSjzGC9xqEr/XTkCbMfZMdHby16NZnmFSWgGcUDXU+Ul+2ouKwjEKkbNtZJ 1YJ4SolraU090uDQ8H0Wluzp3vslgQVW8dUVfY77g6N4qPO5kCSAW1sZjRMcsA8vck6Azkjz EaUnsjBDCZq9raSTBq19KqQrD60ETgYKykFfyBsZRsI5ZzvrZ8+ijrLT81/C+ilg9vtAzbyz juW6i8kiN07hMgHzf/jpQjvjDelp5yPRQkwji3JWWai4hJ8dZSSbYWi4ljG7t5NNI+cCFKGu RAsnMyT7/sHC52XozCcW+UGHLyv5PGtPSXVhBhkGJxJ3y+253epcIRU4Td/DERkKMAJfXnue kC7hO9KzMYNZj3wN/YxOt/vTZRxpUT9KTj7fvbNVsENUL9sSB6K5iRRd0+N/jDVi2F5xMnTJ qynWcqrCH8bD4Fuwzy3W/oR3NcX+8wu+Y/Abcullkr6iNJycFbQEO5YawXWMojV+YvZ+F29z jpJCyedJ/yzusXaazKfz4McJEtiwZMTVcGv8Jw/mgJuz2Nb9IAd5x35neJJl29Nxf09egL0E peVAB8wJL3X3yyvFOlyQio/AI4DpL4mxZ7BAQQiPEyzx18oapu14aEUevMfJOd7rrQ6lqYqE 6leIa1s58ijrBydq1zxirGn/eRfmOmD31rSb0JJnhBiF3Kfe+A50oC9JVa+nMX/JiG2qdE/s 9WdOvDzGvI+q/BZJJ+OMpqHlgrp1VBEwb4adxaWfrF7JRu9mKA3e32ZpqJmeKkkdEScrgZ2I i7LW3/0U8GX/d9smDQI7IjZx7qU/xxWRRsFTzKFvOfvZUE3PAOLmOd9bQpBRhiEPEvc86S+f +RFifb6NfwMhlFRtIRgVb1syMoDCxHH+Ne2FyxoQyfGaUqFELRlLiXU1MVDrPQVlLRYpRG3S gSE/dwDYeeFP8bsEVgwIgs5b7vciaFIy2eKtfllcl/n4CJX/aacVRkANRe7lyEAfqB+N5kow Ll9tZdOuRC/kBcjLv2PkjtQqzaXNnUFXqh+7sMaDYbnhxAF0FZHZZCAWCb67IvWM4dHM1UwI y/Sj63H3uwOyk3Hens1NH7MwesC2she5EEUlAcPfg3blMDEi/k72Axq3Q42FgkFnA9a1+9TO 3RwMxEnL6u54Do11tNIWHqhGl8dCUTBqFDx0VYAiEbQU1KsCj7WNGQ4NOuAoBIZ/mZbcmQJ9 b2U0j+4AzPjfcW31SouQ0917ffkSIUppAHFncmmGeWDHoU7PmW50vP/OzJQpku1G941iW3Gu fJurbR5ZqDMPCINp7E2VtuB3rMKRRHYfGFPTJmNJk/S8b0wrN1q5QWzFg==
  • Ironport-hdrordr: A9a23:8cAeva+BZeWzkYfHIbduk+E8db1zdoMgy1knxilNoENuHPBwxv rAoB1E73PJYVYqOE3Jmbi7Sc+9qFfnhONICOgqTM2ftWzd2VdAQ7sSiLcKrweQfxEWs9QtqZ uIEJIOeeEYb2IK9foSiTPQe71LrajlgcKVbKXlvgxQpGlRGt9dBmxCe3+m+yNNNW577c1TLu vi2iMLnUvqRV0nKuCAQlUVVenKoNPG0LrgfB49HhYirC2Dlymh5rLWGwWRmk52aUID/Z4StU z+1yDp7KSqtP+2jjfaym/o9pxT3P/s0MFKCsCggtUcbh/slgGrToJ8XKDqhkF+nMifrHIR1P XcqRYpOMp+r1vXY2GOuBPonzLt1T4/gkWSv2OwsD/Gm4jUVTg6A81OicZyaR3C8Xctu9l6ze Ziw3+Zn4A/N2KPoA3No/zzEz16nEu9pnQv1cQJiWZEbIcYYLhN6aQC4UJuFosaFi6S0vFpLA BXNrCd2B9qSyLYU5iA1VMfguBEH05DUitue3Jy+/B8iFNt7TVEJ0hx/r1pop5PzuN4d3B+3Z W2Dk1frsA7ciYnV9MMOA4/e7rENoXse2OEDIvAGyWuKEk4U0i93qIfpo9Fo92XRA==
  • Ironport-sdr: Hq+BAdRuB6fkMmxGZGnQU0JzUEACrv04UeANnMCNeGB2MYALu+mZ/DzRYMXdHA77dGCJHWuN2d keng7Bd14bC00c30epL6UXLrhWCBw2IsVMT3fbvX8gKUO1mGm17K9wspq2a7a1hSL+DZkEtbsL XxwTd5nSZ4mhMQKq0DvX5EIWx+5Q+cNSCUQ1p8B1MOcB7X5z+Aot40ZMnmFH+15cM4LQKcbvtK WVLnf6lOqnPKM3lznYBwrg9LUkSN3PB6o27CkY/g9QtRcGU7rmFETr3ErCeqS4tASTNpPeX/YB 66p3HiOaqk/oF9/qkWwtliBu
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Thu, Nov 25, 2021 at 01:02:48PM +0200, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> 
> Assign SBDF to the PCI devices being passed through with bus 0.
> The resulting topology is where PCIe devices reside on the bus 0 of the
> root complex itself (embedded endpoints).
> This implementation is limited to 32 devices which are allowed on
> a single PCI bus.
> 
> Please note, that at the moment only function 0 of a multifunction
> device can be passed through.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> ---
> Since v4:
> - moved and re-worked guest sbdf initializers
> - s/set_bit/__set_bit
> - s/clear_bit/__clear_bit
> - minor comment fix s/Virtual/Guest/
> - added VPCI_MAX_VIRT_DEV constant (PCI_SLOT(~0) + 1) which will be used
>   later for counting the number of MMIO handlers required for a guest
>   (Julien)
> Since v3:
>  - make use of VPCI_INIT
>  - moved all new code to vpci.c which belongs to it
>  - changed open-coded 31 to PCI_SLOT(~0)
>  - added comments and code to reject multifunction devices with
>    functions other than 0
>  - updated comment about vpci_dev_next and made it unsigned int
>  - implement roll back in case of error while assigning/deassigning devices
>  - s/dom%pd/%pd
> Since v2:
>  - remove casts that are (a) malformed and (b) unnecessary
>  - add new line for better readability
>  - remove CONFIG_HAS_VPCI_GUEST_SUPPORT ifdef's as the relevant vPCI
>     functions are now completely gated with this config
>  - gate common code with CONFIG_HAS_VPCI_GUEST_SUPPORT
> New in v2
> ---
>  xen/drivers/vpci/vpci.c | 51 +++++++++++++++++++++++++++++++++++++++++
>  xen/include/xen/sched.h |  8 +++++++
>  xen/include/xen/vpci.h  | 11 +++++++++
>  3 files changed, 70 insertions(+)
> 
> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> index 98b12a61be6f..c2fb4d4db233 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -114,6 +114,9 @@ int vpci_add_handlers(struct pci_dev *pdev)
>      spin_lock(&pdev->vpci_lock);
>      pdev->vpci = vpci;
>      INIT_LIST_HEAD(&pdev->vpci->handlers);
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> +    pdev->vpci->guest_sbdf.sbdf = ~0;
> +#endif
>  
>      header = &pdev->vpci->header;
>      for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
> @@ -145,6 +148,53 @@ int vpci_add_handlers(struct pci_dev *pdev)
>  }
>  
>  #ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> +int vpci_add_virtual_device(struct pci_dev *pdev)
> +{
> +    struct domain *d = pdev->domain;
> +    pci_sbdf_t sbdf = { 0 };
> +    unsigned long new_dev_number;

I think this needs to be limited to non-hardware domains?

Or else you will report failures for the hardware domain even if it's
not using the virtual topology at all.

> +    /*
> +     * Each PCI bus supports 32 devices/slots at max or up to 256 when
> +     * there are multi-function ones which are not yet supported.
> +     */
> +    if ( pdev->info.is_extfn )
> +    {
> +        gdprintk(XENLOG_ERR, "%pp: only function 0 passthrough supported\n",
> +                 &pdev->sbdf);
> +        return -EOPNOTSUPP;
> +    }
> +
> +    new_dev_number = find_first_zero_bit(&d->vpci_dev_assigned_map,
> +                                         VPCI_MAX_VIRT_DEV);
> +    if ( new_dev_number >= VPCI_MAX_VIRT_DEV )
> +        return -ENOSPC;
> +
> +    __set_bit(new_dev_number, &d->vpci_dev_assigned_map);

How is vpci_dev_assigned_map protected from concurrent accesses? Does
it rely on the pcidevs lock being held while accessing it?

If so it needs spelling out (and likely an assert added).

> +    /*
> +     * Both segment and bus number are 0:
> +     *  - we emulate a single host bridge for the guest, e.g. segment 0
> +     *  - with bus 0 the virtual devices are seen as embedded
> +     *    endpoints behind the root complex
> +     *
> +     * TODO: add support for multi-function devices.
> +     */
> +    sbdf.devfn = PCI_DEVFN(new_dev_number, 0);
> +    pdev->vpci->guest_sbdf = sbdf;
> +
> +    return 0;
> +
> +}
> +REGISTER_VPCI_INIT(vpci_add_virtual_device, VPCI_PRIORITY_MIDDLE);

I'm unsure this is the right place to do virtual SBDF assignment, my
plan was to use REGISTER_VPCI_INIT exclusively with PCI capabilities.

I think it would be better to do the virtual SBDF assignment from
vpci_assign_device.

> +
> +static void vpci_remove_virtual_device(struct domain *d,
> +                                       const struct pci_dev *pdev)
> +{
> +    __clear_bit(pdev->vpci->guest_sbdf.dev, &d->vpci_dev_assigned_map);
> +    pdev->vpci->guest_sbdf.sbdf = ~0;
> +}
> +
>  /* Notify vPCI that device is assigned to guest. */
>  int vpci_assign_device(struct domain *d, struct pci_dev *pdev)
>  {
> @@ -171,6 +221,7 @@ int vpci_deassign_device(struct domain *d, struct pci_dev 
> *pdev)
>          return 0;
>  
>      spin_lock(&pdev->vpci_lock);
> +    vpci_remove_virtual_device(d, pdev);
>      vpci_remove_device_handlers_locked(pdev);
>      spin_unlock(&pdev->vpci_lock);
>  
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 28146ee404e6..10bff103317c 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -444,6 +444,14 @@ struct domain
>  
>  #ifdef CONFIG_HAS_PCI
>      struct list_head pdev_list;
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> +    /*
> +     * The bitmap which shows which device numbers are already used by the
> +     * virtual PCI bus topology and is used to assign a unique SBDF to the
> +     * next passed through virtual PCI device.
> +     */
> +    unsigned long vpci_dev_assigned_map;

Please use DECLARE_BITMAP with the maximum number of supported
devices as parameter.

> +#endif
>  #endif
>  
>  #ifdef CONFIG_HAS_PASSTHROUGH
> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
> index 18319fc329f9..e5258bd7ce90 100644
> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -21,6 +21,13 @@ typedef int vpci_register_init_t(struct pci_dev *dev);
>  
>  #define VPCI_ECAM_BDF(addr)     (((addr) & 0x0ffff000) >> 12)
>  
> +/*
> + * Maximum number of devices supported by the virtual bus topology:
> + * each PCI bus supports 32 devices/slots at max or up to 256 when
> + * there are multi-function ones which are not yet supported.
> + */
> +#define VPCI_MAX_VIRT_DEV       (PCI_SLOT(~0) + 1)
> +
>  #define REGISTER_VPCI_INIT(x, p)                \
>    static vpci_register_init_t *const x##_entry  \
>                 __used_section(".data.vpci." p) = x
> @@ -143,6 +150,10 @@ struct vpci {
>              struct vpci_arch_msix_entry arch;
>          } entries[];
>      } *msix;
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> +    /* Guest SBDF of the device. */
> +    pci_sbdf_t guest_sbdf;
> +#endif
>  #endif
>  };
>  
> -- 
> 2.25.1
> 



 


Rackspace

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