[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 10:08:51 +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=WUeLAbaOP4QSjoUbJ+Xvmssa/tlRf+CiEwTL8sgh5TM=; b=M3vb7QWdiT1CJmAeCn602xqCgOJLs/Y9JUF1TcZx1dv/h0nAqHUumyz1m6+MdsR9Apyv0OJTv0a7EeIegzbXlTMSKTGJQTor/vdhDIDg6Js95FZoOkc7/1kkbY8WFiJ2pONe7xt4e0XQqVhKFSrYACLhhB8Qvln8N2CsTSzKqAlp0WlUu0IpKrVsVg4gABvuD5lU2E8JLZF9hUMQG2ibMQ9CvqfFRsK/I3cHvpCu711aJyXuzRiyYz/nYppDGW9njXhtZ4xKi7yd4YyagVvhDtnBZh7kDewcOKtdJ/Ev8HWr5QfnKg5SUyKgqmiF2ueGwH1Km8Oi5jUleeK7Au59GA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=VzNLjj48hRNf7hhSyN7JUh6cuL3L9YelDo84r4X1zXAxb7T/M3rKPNkrkz+mtHhs2ekEq7iRw85E8E8WZuxd5pkPFgpQXUMd9I6e+Hf+3nCkLIILHnCdXIkj8Nk/RmsBYDXL6SvOzyr7ttr34kWSipteYEYgSLPCUPP1Mw7ut58ignrjxBUbfTebA4EInJORGkLF78xn+eTZhjQ+op+4vorSYD3bbB8luMLDmDeTBieA/2qa5PW+EH8dLCl8JfhGmyi4dUl2IJs4y/rS/LGg2jzkAjkY8fsTAJm6eVTv6w736hN6xk1xc0F7roA3yEzjK6WTFA+gMr1hM06Fr8fyzA==
  • Authentication-results: esa5.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, "iwj@xxxxxxxxxxxxxx" <iwj@xxxxxxxxxxxxxx>, "Oleksandr_Andrushchenko@xxxxxxxx" <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 08:09:21 +0000
  • Ironport-data: A9a23:F2zjuKKyzKKu2+CBFE+R1JMlxSXFcZb7ZxGr2PjKsXjdYENS0mRUz GEXC2qDa/mMMGT2KYgnaN619k5XuJTdx4JqTwplqX01Q3x08seUXt7xwmUcns+xwm8vaGo9s q3yv/GZdJhcokcxIn5BC5C5xZVG/fjgqoHUVaiUZUideSc+EH140Eo5w7Zg6mJVqYPR7z2l6 IuaT/L3YDdJ6xYsWo7Dw/vewP/HlK2aVAIw5jTSV9gS1LPtvyB94KYkDbOwNxPFrrx8RYZWc QphIIaRpQs19z91Yj+sfy2SnkciGtY+NiDW4pZatjTLbrGvaUXe345iXMfwZ3u7hB2XnNBcy YkUpaefRFc2P6L+wbwyc1phRnQW0a1uoNcrIFC6uM2XiUbHb2Ht07NlC0Re0Y8wo7gtRzsUr LpBdW5LPkvra+GemdpXTsF2gcsuNo/zNZ43sXB81zDJS/0hRPgvRo2XuIYHhGhu3aiiG96BY ZobbAR9US3hahd1EXInL5MApMCR0yyXnzpw9wvO+PtfD3Lo5DJ21L/hId/EYOugTM9enlubj m/e9mG/CRYfXPSd1D6E/3SEluLJ2yThV+o6Fre16/pri1273XEIBVsdUl7TieGwl0qWS99Zb UsO9UIGsqwa5EGtCN7nUHWQsHOC+xIRRddUO+k78x2WjLrZ5R6DAWoJRSIHb8Yp3OccSCY2z FaPk5XMDCZ2rbyOYXuH8/GfqjbaESkKL0cSaCkcVwwH7tL/5oYpgXrnVc1/GaS4itn0HzDYw D2QqiU6wbIJgqYj8KG2/kvOhT62kbHPQhQo/QXcXm+j7QRRaZasYsqj7l2z0BpbBN/HFB/b5 iFCwpXAqrBVZX2QqMCTaPsOHJyou6+VCxLR0UFuEKt4rxuNuEf2KOi8/wpCDEtuN88Ffxrgb 0nSpR5d6fdvAZe6UUNkS9nuU5pynMAMAfygD6qONoMfPfCdYSfepHk2DXN8yVwBh6TFfUsXA pycbdq3RUgTDaBq3VJarM9MjOd1mEjSKY7VLK0XLihLM5LCOxZ5qp9faTNii9zVCove/G05F P4Eb6O3J+13CrGWX8Uu2dd7wao2BXY6H4vqjMdca/SOJAFrcEl4VaSNn+1wI9Y5x/0M/gstw p1bchUEoLYYrSafQThml1g5MO+/NXqBhSNT0dMQ0aaAhCF4PNfHAFY3fJorZ7g3nNGPPtYvJ 8Tpj/6oW6wVIhyeomx1RcCk8ORKKUT67SrTbnHNSGVuIPZdq/nhp4aMkv3Hr3JVUEJadKIW/ tWd6+8sacBdF188UpuONKjHIpHYlSF1pd+elnDge7F7UE7t7JJrO2r2iPo2KNsLMhLN2n2R0 AP+PPvSjbKlT1Yd/IabiKaagZ2uFuciTENWE3OCteS9NDXA/3rlyohFCb7acTfYXWLy2aOje eQKkK2sbKxZxA5H49hmDrJm7aMi/N+z9bVU+RtpQSfQZFOxB7I+fnTfhZtTtrdAz6NysBetX h7d4cFTPLiEYZu3EFMYKAc/QP6E0PUYxmvb4fgveR2o7y5r5ruXF05VOkDU2iBaKbJ0NqIjw Psg55FKu1Du1EJyP4/f3C5O9mmKIngRaIkdt8kXUN3xlw4m6lBeepiAWCX4146CNodXOU4wL z7K2Keb3+ZAxlDPemYYHGTW2bYPnowHvR1HwQNQJ1mNndaZ1PY70AcIrGYyRwVRiB5Gz/hyK i5gMEgsffeC+DJhhc5iWWGwGl4eWE3FqxKpk1ZZxnfES0SIV3DWKDxvMOmAy0kV7mZAc2UJ5 7qf0mvkDW7nccyZMvHegqK5RygPleBMyzA=
  • Ironport-hdrordr: A9a23:iDtA66GCa5iJcydLpLqFe5HXdLJyesId70hD6qkvc3Jom52j+P xGws526faVslYssHFJo6HnBEClewKgyXcT2/hsAV7CZnidhILMFuBfBOTZsljd8kHFh4pgPO JbAtdD4b7LfChHZKTBkXGF+r8bqbHtms3Y5pa9vgRQpENRGtpdBm9Ce3em+yZNNXB77PQCZf 2hDp0tnUvfRZ1bVLX2OlA1G8z44/HbnpPvZhALQzYh9Qm1lDutrJr3CQKR0BsyWy5Ghe5Kyx mKryXJooGY992rwB7V0GHeq7xQhdva09NGQOiBkNIcJDnAghuhIK5hR7qBljYop/zH0idkrP D85zMbe+hj4XLYeW+45TPrxgnbyT4rr0TvzFeJ6EGT6vDRdXYfMY5slIhZehzW5w4Lp9dnyp 9G2Gqfqt5+EQ7AtD6V3amKazha0m6P5VYym+8aiHJSFaEEbqVKkIAZ9ERJVL8dASPB7pw9Gu UGNrCd2B9vSyLeU5nlhBgq/DT1NU5DWStuA3Jy+/B96gIm3kyQlCAjtYoidnRpzuN0d3AL3Z WcDkzE/Is+OfP+VpgNcdvpd/HHQ1AlcSi8Ql56Hm6XYJ3vG0i94aIfs49Fqd1DRvQzve0Pcd L6IQtliVI=
  • Ironport-sdr: 42WFowZyT0ZW7REpZp4ggEO6cV7Fzu9GGTJGDfwLPM6tZz7vQmSIDPDmh5SFznL4zCGZnq8V2J 0Yo0i25D/wgqDzEmZVgxyLLYuqNQEuUKrfCSywjfIbtVPgcZQsS6SegQLdwkDrzhAlTPLQ9QZQ N7gMhe/q/+jvOVNEwlB3lWJYnKRznb4abDvLNEJZHA3TPAvOJQ3dQ+rQvJzeRAi3CivUN8Gn03 auVww7j+1I0YOkJy9p/7vegX66iEK9r/q3LE5vTQTu4n9B6zg0kMrv4gj9k/ppZoQkSNxHUQjd hAnOp02+jh2XrE88HkLdmC4e
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Wed, Oct 20, 2021 at 07:57:15AM +0000, Bertrand Marquis wrote:
> Hi Roger,
> 
> > On 20 Oct 2021, at 08:49, Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> > 
> > 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.
> 
> I can do both in v3 (together with the change of in the patch name).
> 
> Unless you want the ASSERT in a different patch, in this case I do not think
> I can make a new patch for that.
> 
> Can you confirm if I should or not add the ASSERT directly in the patch ?

I'm fine with having the assert added here: in wasn't necessary
previously as there was a single caller of vpci_add_handlers. Now that
there are multiple ones we should make sure no duplicated calls can
happen.

On a different note (and not something that should be solved here,
just wanted to raise attention to it) there's an existing TODO about
vpci_remove_device because it doesn't clean the 2nd stage mappings for
BARs. At some point we need to solve this, or else the removal of the
device is not complete.

Thanks, Roger.



 


Rackspace

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