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

Re: [PATCH v2 1/1] xen/pci: Install vpci handlers on x86 and fix exit path


  • To: Bertrand Marquis <bertrand.marquis@xxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Wed, 20 Oct 2021 09:49:32 +0200
  • 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=/pKlgnD6MkN3KSMd8ywkU3dD8ruRKSqhywXbmEvrglE=; b=BStuLmdJpym9fRey15iDdMBrPJ8SY+53K9C8OJpTv/6EqOnT2RbtxiAAbHmM5xr3vTGXb1uSl1ZsnmycTN4tbGR7QPqxWfWdtooEnihtslt7jLhhgpVHRLmP7craoQyeJty5yakmsyudxfsP10dM9F4+BPuJOMwqiCzTWxt48IxLuPjxbeH2l9kHzQLBDKtqWAfU2EQ34JYVXuCdUnAGHFnTdEN8nJfBZWcoJmrHtehm8rY9lCy9zsI6PfvnNmdN1Wq4/igYBji/KjjUfNfJZiUVw7B0WWMkceNKLZOsKp8ti0N5SpytAf5e1JZjRUUZcDBX7PdtU67wwoamzs1CQA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=KaZMeU7k3/LBY7OtnfHj0YeYKyBo4ooU1BNzY1oz6G9uDADZUOghOvl7CU8G4A3qtt7/S3D25kZJryH+2lVN3zHv69VgrnWZ6jEQBxjKVKu5VE8ngEM22yLWH6VjHOtNaoScHIoFo8dHExCfJUaYOPism7cdvOUW0Ys0fs5MHWlL8WIkA8iWIFzDiVBTPFICVwQTo1IKUaVZdHiMSKcjJHNsF9VW1MXA9dU9bw+b5zZN6TMWbX/xa0GWoVSNacttLwgIWosyVf4NEkGUQ252uakuMYDXWdHLdqBL/V0nSvqFNEJdbpBY9O8m645LoTBielEXHk6sn8yqIEnCiwJ5/w==
  • Authentication-results: esa2.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: <xen-devel@xxxxxxxxxxxxxxxxxxxx>, <iwj@xxxxxxxxxxxxxx>, <Oleksandr_Andrushchenko@xxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Paul Durrant <paul@xxxxxxx>
  • Delivery-date: Wed, 20 Oct 2021 07:50:16 +0000
  • Ironport-data: A9a23:dqsk36Mp6SGbV0fvrR2gkcFynXyQoLVcMsEvi/4bfWQNrUp31mAEz mpOWj2PbKuNamD0fIx0YYm3oEkP6J7TztQwTgto+SlhQUwRpJueD7x1DKtR0wB+jCHnZBg6h ynLQoCYdKjYdpJYz/uUGuCJQUNUjMlkfZKhTr6bUsxNbVU8En540Ug7w7dRbrNA2rBVPSvc4 bsenOWHULOV82Yc3rU8sv/rRLtH5ZweiRtA1rAMTakjUGz2zhH5OKk3N6CpR0YUd6EPdgKMq 0Qv+5nilo/R109F5tpICd8XeGVSKlLZFVDmZna7x8FOK/WNz8A/+v9TCRYSVatYoxillsgtk sdij7utaBoObryRubolCwYNRkmSPYUekFPGCX22sMjVxEzaaXr8hf5pCSnaP6VBpLwxWzsXs 6VFdnZdNXhvhMrvqF6/YvNrick5atHiIasUu216zCGfBvEjKXzGa/iXu4YGjW5v7ixINcv/S JoaaWV/VkzrZF5lO1UrUaAArej90xETdBUH8QnI9MLb+VP78gt117T8NcvPTfaDT85Vg0Wwq 3rP+iLyBRRyHN6CzTuI9Fq8i+mJmjn0MKoQHrCl8v9hgHWI23ceThYRUDOTufCkjmauVtQZL FYbkgIxqYAi+UrtScPyNzWorXjBshMCVt54F+wh9BrL2qfS+xyeBGUPUnhGctNOnMU7XyAw3 1mF2dbgHyVysaa9QGiYsLyTqFuaODcXBX8PYzceSgkI6MWlp5s85jrUVcpqGqOxitzzGBnzz iqMoSx4gK8c5eYs06i45lnBjyibj57FVBMu5g7XU2Sm6St0fIegIYev7DDmAe1oddjDCAPb5 T5dxpbYvLtm4YyxeDKlbckEBO3z9dm/FyD9nHk/DsQAqQz15Cv2FWxP2w1WKEBsO8cCXDbmZ k7PpA9cjKNu0GuWgbxfON3pVZx7pUT0PZG8DKqMN4sRCnRkXFbfpHkGWKKG44z6fKHAe4kEM pCHbd3kM38eDal2pNZdb7ZAieF1rszSKGW6eHwa8/hF+ebBDJJ2Ye1cWLdrUgzfxPnYyOky2 40HX/ZmMz0FDIXDjtD/qOb/12wiI3khHozRoMdKbOOFKQcOMDh/UKOPn+J5IdA9xv09egL0E peVABMwJL3X3iWvFOl3Qio7NOOHsWhX/BrXwhDAzX73giN+MO5DHY8UdoctfKlPyQCQ5aUcc hXxQO3ZWq4nYm2eo1w1NMChxKQ/JEXDrV/fZEKNPWlgF6OMsiSUo7cIiCO0r3JQZsd23ONjy 4CdOvTzG8RTHFo8UZ6KMppCDTqZ5BAgpQ67ZGORSvF7c0Tw6ol6bSv3i/48OcYXLhvfgDCd0 m6r7d0w/IEheqc5r4vEg76qtYCsH7csF0ZWBTCDv723KTPb7iyoxooZCLSEejXUVWXV/qS+Z LoKk6GgYaNfxFsa4ZBhF7tLzL4l44e9rbFt0Qk5Tm7AaE6mC+08LyDej9VPrKBE2pRQpRCyB hCU4tBfNLjQYJHlHVcdKRALdOOG0f1IyDDe4e5seBfx5TNt/arBWkJXZkHehCtYJbpzEYUk3 eZ+55JGt13h0kIna4/UgDpV+mKAKm07f58m7pxKUpX2jgcLy01ZZcCOACHB/5zSOc5HNVMnI 2HIifOa1ahc3EfLb1E6CWPJgbhGnZ0LtR1HkA0CKlCOloaXj/M7xkQMozE+TwATxRRbyeNjf GNsMhQtd6mJ+j5pgulFXnytRF4dVEHIpBSpxgtbjnDdQmmpSnfJfT80Nuu69UwE935RI2pA9 7aCxWe5CTvncakdBMfptZKJfxA7ceFMyw==
  • Ironport-hdrordr: A9a23:yk0qpKDSBUOfUxTlHeka55DYdb4zR+YMi2TDj3oBLyC8cqSj+f xG785rrCMc6QxhLE3I9urwWpVoLUmxyXcx2/h3AV7AZniQhILLFvAG0WKK+VSJcEeSygce79 YFT0EUMrzN5C1B/KXHCX6Dc+rIruPpzEniv5an854kd3AQV0lHhz0JQzpz1ncGMjVuFN4yT4 OG4MBKvSCtdHINYK2Adww4tsb41qf2fMiKW296O/YFhTP+8Q9BqNPBYmalNllyaUI//V9tm1 K14TAQVMiYwo6G4w6Zymva9ZgTg9f61t5fbfb8+vQ9O3Hwjg6zaMB/V6aZvDYzydvfm2oXrA ==
  • Ironport-sdr: +Dc4ICcqZs3UK+MTic9jib1wnV1RaQdzdxeiY2M8N5J1GnKylTjl3XDwm9/ylbtLxScZLqzgHX 3Um0iuJdYYdHtjaKb7zPorTtVvKLXUt6wUwL3Oj+XzQJma0kvWJWHPYTU9Y87nOAda5wTPOC0R WdvW/vv1++5IIeUzaAVhKTmg/Ckl+aZWj4EPJ32R0iVEfFtl7G4s3XUbuGAS/Q7ytbRpx1R/hQ mcthtV0NmDVfOH2XVWu/CqFJySg63Sk8SsjLqcf94A1DMSn9z8Kg8IL6yBxfpkhk6YBC/1MNVX TThfXi8Eo8hwErRITQQKva72
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Tue, Oct 19, 2021 at 05:08:28PM +0100, Bertrand Marquis wrote:
> Xen might not be able to discover at boot time all devices or some devices
> might appear after specific actions from dom0.
> In this case dom0 can use the PHYSDEVOP_pci_device_add to signal some
> PCI devices to Xen.
> As those devices where not known from Xen before, the vpci handlers must
> be properly installed during pci_device_add for x86 PVH Dom0, in the
> same way as what is done currently on arm (where Xen does not detect PCI
> devices but relies on Dom0 to declare them all the time).
> 
> So this patch is removing the ifdef protecting the call to
> vpci_add_handlers and the comment which was arm specific.
> 
> vpci_add_handlers is called on during pci_device_add which can be called
> at runtime through hypercall physdev_op.
> Remove __hwdom_init as the call is not limited anymore to hardware
> domain init and fix linker script to only keep vpci_array in rodata
> section.
> 
> Add missing vpci handlers cleanup during pci_device_remove and in case
> of error with iommu during pci_device_add.
> 
> Add empty static inline for vpci_remove_device when CONFIG_VPCI is not
> defined.
> 
> Fixes: d59168dc05 ("xen/arm: Enable the existing x86 virtual PCI support
> for ARM")
> Suggested-by: Jan Beulich <jbeulich@xxxxxxxx>
> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> ---
> Changes in v2
> - add comment suggested by Jan on top of vpci_add_handlers call
> - merge the 3 patches of the serie in one patch and renamed it
> - fix x86 and arm linker script to only keep vpci_array in rodata and
> only when CONFIG_VPCI is set.
> ---
>  xen/arch/arm/xen.lds.S        | 9 +--------
>  xen/arch/x86/xen.lds.S        | 9 +--------
>  xen/drivers/passthrough/pci.c | 8 ++++----
>  xen/drivers/vpci/vpci.c       | 2 +-
>  xen/include/xen/vpci.h        | 2 ++
>  5 files changed, 9 insertions(+), 21 deletions(-)
> 
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index b773f91f1c..08016948ab 100644
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -60,7 +60,7 @@ SECTIONS
>         *(.proc.info)
>         __proc_info_end = .;
>  
> -#if defined(CONFIG_HAS_VPCI) && defined(CONFIG_LATE_HWDOM)
> +#ifdef CONFIG_HAS_VPCI
>         . = ALIGN(POINTER_ALIGN);
>         __start_vpci_array = .;
>         *(SORT(.data.vpci.*))
> @@ -189,13 +189,6 @@ SECTIONS
>         *(.init_array)
>         *(SORT(.init_array.*))
>         __ctors_end = .;
> -
> -#if defined(CONFIG_HAS_VPCI) && !defined(CONFIG_LATE_HWDOM)
> -       . = ALIGN(POINTER_ALIGN);
> -       __start_vpci_array = .;
> -       *(SORT(.data.vpci.*))
> -       __end_vpci_array = .;
> -#endif
>    } :text
>    __init_end_efi = .;
>    . = ALIGN(STACK_SIZE);
> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> index 11b1da2154..87e344d4dd 100644
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -134,7 +134,7 @@ SECTIONS
>         *(.ex_table.pre)
>         __stop___pre_ex_table = .;
>  
> -#if defined(CONFIG_HAS_VPCI) && defined(CONFIG_LATE_HWDOM)
> +#ifdef CONFIG_HAS_VPCI
>         . = ALIGN(POINTER_ALIGN);
>         __start_vpci_array = .;
>         *(SORT(.data.vpci.*))
> @@ -247,13 +247,6 @@ SECTIONS
>         *(.init_array)
>         *(SORT(.init_array.*))
>         __ctors_end = .;
> -
> -#if defined(CONFIG_HAS_VPCI) && !defined(CONFIG_LATE_HWDOM)
> -       . = ALIGN(POINTER_ALIGN);
> -       __start_vpci_array = .;
> -       *(SORT(.data.vpci.*))
> -       __end_vpci_array = .;
> -#endif
>    } PHDR(text)
>  
>    . = ALIGN(SECTION_ALIGN);
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 35e0190796..8928a1c07d 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -756,10 +756,9 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>      if ( !pdev->domain )
>      {
>          pdev->domain = hardware_domain;
> -#ifdef CONFIG_ARM
>          /*
> -         * On ARM PCI devices discovery will be done by Dom0. Add vpci 
> handler
> -         * when Dom0 inform XEN to add the PCI devices in XEN.
> +         * For devices not discovered by Xen during boot, add vPCI handlers
> +         * when Dom0 first informs Xen about such devices.
>           */
>          ret = vpci_add_handlers(pdev);
>          if ( ret )
> @@ -768,10 +767,10 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>              pdev->domain = NULL;
>              goto out;
>          }
> -#endif
>          ret = iommu_add_device(pdev);
>          if ( ret )
>          {
> +            vpci_remove_device(pdev);
>              pdev->domain = NULL;
>              goto out;
>          }
> @@ -819,6 +818,7 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
>      list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
>          if ( pdev->bus == bus && pdev->devfn == devfn )
>          {
> +            vpci_remove_device(pdev);

vpci_remove_device is missing a check for has_vpci(pdev->domain), as
it will unconditionally access pdev->vpci otherwise, and that would be
wrong for domains not using vpci.

It might also be good to add an ASSERT(!pdev->vpci) to
vpci_add_handlers in order to make sure there are no duplicated calls
to vpci_add_handlers, but that can be done in a separate patch.

Thanks, Roger.



 


Rackspace

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