All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/1] Fixes: PCI devices passthrough on Arm
@ 2021-10-19 16:08 Bertrand Marquis
  2021-10-19 16:08 ` [PATCH v2 1/1] xen/pci: Install vpci handlers on x86 and fix exit path Bertrand Marquis
  0 siblings, 1 reply; 16+ messages in thread
From: Bertrand Marquis @ 2021-10-19 16:08 UTC (permalink / raw)
  To: xen-devel
  Cc: iwj, Oleksandr_Andrushchenko, Stefano Stabellini, Julien Grall,
	Volodymyr Babchuk, Jan Beulich, Andrew Cooper,
	Roger Pau Monné,
	Wei Liu, Paul Durrant

This patch serie is a follow-up after various findings on d59168dc05
("xen/arm: Enable the existing x86 virtual PCI support for ARM") as of
agreed in [1].

It does the following:
- enable vpci_add_handlers on x86 and not only on arm
- remove __hwdom_init flag for vpci_add_handlers
- add missing vpci handler cleanup in error path during pci_device_add
  and pci_device_remove

[1] https://marc.info/?l=xen-devel&m=163455502020100&w=2

In the second version of the serie, the 3 patches have been merged into
one single patch.

Bertrand Marquis (1):
  xen/pci: Install vpci handlers on x86 and fix exit path

 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(-)

-- 
2.25.1



^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2 1/1] xen/pci: Install vpci handlers on x86 and fix exit path
  2021-10-19 16:08 [PATCH v2 0/1] Fixes: PCI devices passthrough on Arm Bertrand Marquis
@ 2021-10-19 16:08 ` Bertrand Marquis
  2021-10-19 23:14   ` Stefano Stabellini
                     ` (3 more replies)
  0 siblings, 4 replies; 16+ messages in thread
From: Bertrand Marquis @ 2021-10-19 16:08 UTC (permalink / raw)
  To: xen-devel
  Cc: iwj, Oleksandr_Andrushchenko, Stefano Stabellini, Julien Grall,
	Volodymyr Babchuk, Jan Beulich, Andrew Cooper,
	Roger Pau Monné,
	Wei Liu, Paul Durrant

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@suse.com>
Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
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);
             pci_cleanup_msi(pdev);
             ret = iommu_remove_device(pdev);
             if ( pdev->domain )
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index decf7d87a1..74894bcbac 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -54,7 +54,7 @@ void vpci_remove_device(struct pci_dev *pdev)
     pdev->vpci = NULL;
 }
 
-int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
+int vpci_add_handlers(struct pci_dev *pdev)
 {
     unsigned int i;
     int rc = 0;
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 6746c2589a..9ea66e033f 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -230,6 +230,8 @@ static inline int vpci_add_handlers(struct pci_dev *pdev)
     return 0;
 }
 
+static inline void vpci_remove_device(struct pci_dev *pdev) { }
+
 static inline void vpci_dump_msi(void) { }
 
 static inline uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg,
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/1] xen/pci: Install vpci handlers on x86 and fix exit path
  2021-10-19 16:08 ` [PATCH v2 1/1] xen/pci: Install vpci handlers on x86 and fix exit path Bertrand Marquis
@ 2021-10-19 23:14   ` Stefano Stabellini
  2021-10-20  7:16   ` Jan Beulich
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 16+ messages in thread
From: Stefano Stabellini @ 2021-10-19 23:14 UTC (permalink / raw)
  To: Bertrand Marquis
  Cc: xen-devel, iwj, Oleksandr_Andrushchenko, Stefano Stabellini,
	Julien Grall, Volodymyr Babchuk, Jan Beulich, Andrew Cooper,
	Roger Pau Monné,
	Wei Liu, Paul Durrant

On Tue, 19 Oct 2021, 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@suse.com>
> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> 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);
>              pci_cleanup_msi(pdev);
>              ret = iommu_remove_device(pdev);
>              if ( pdev->domain )
> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> index decf7d87a1..74894bcbac 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -54,7 +54,7 @@ void vpci_remove_device(struct pci_dev *pdev)
>      pdev->vpci = NULL;
>  }
>  
> -int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
> +int vpci_add_handlers(struct pci_dev *pdev)
>  {
>      unsigned int i;
>      int rc = 0;
> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
> index 6746c2589a..9ea66e033f 100644
> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -230,6 +230,8 @@ static inline int vpci_add_handlers(struct pci_dev *pdev)
>      return 0;
>  }
>  
> +static inline void vpci_remove_device(struct pci_dev *pdev) { }
> +
>  static inline void vpci_dump_msi(void) { }
>  
>  static inline uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg,
> -- 
> 2.25.1
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/1] xen/pci: Install vpci handlers on x86 and fix exit path
  2021-10-19 16:08 ` [PATCH v2 1/1] xen/pci: Install vpci handlers on x86 and fix exit path Bertrand Marquis
  2021-10-19 23:14   ` Stefano Stabellini
@ 2021-10-20  7:16   ` Jan Beulich
  2021-10-20  7:20     ` Bertrand Marquis
  2021-10-20  7:49   ` Roger Pau Monné
  2021-10-20  8:27   ` Roger Pau Monné
  3 siblings, 1 reply; 16+ messages in thread
From: Jan Beulich @ 2021-10-20  7:16 UTC (permalink / raw)
  To: Bertrand Marquis
  Cc: iwj, Oleksandr_Andrushchenko, Stefano Stabellini, Julien Grall,
	Volodymyr Babchuk, Andrew Cooper, Roger Pau Monné,
	Wei Liu, Paul Durrant, xen-devel

On 19.10.2021 18:08, 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@suse.com>
> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

I'm inclined to suggest s/exit/error/ in the title though (and maybe
also s/path/paths/), which would be easy enough to do while committing.
But first we need Roger's ack here anyway ...

Jan



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/1] xen/pci: Install vpci handlers on x86 and fix exit path
  2021-10-20  7:16   ` Jan Beulich
@ 2021-10-20  7:20     ` Bertrand Marquis
  2021-10-20 10:51       ` Ian Jackson
  0 siblings, 1 reply; 16+ messages in thread
From: Bertrand Marquis @ 2021-10-20  7:20 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Ian Jackson, Oleksandr_Andrushchenko, Stefano Stabellini,
	Julien Grall, Volodymyr Babchuk, Andrew Cooper,
	Roger Pau Monné,
	Wei Liu, Paul Durrant, xen-devel

Hi,

> On 20 Oct 2021, at 08:16, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 19.10.2021 18:08, 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@suse.com>
>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thank.

> 
> I'm inclined to suggest s/exit/error/ in the title though (and maybe
> also s/path/paths/), which would be easy enough to do while committing.

@Ian: Please tell me if this is ok to be fixed during commit.

> But first we need Roger's ack here anyway ...

Yes.

Cheers
Bertrand

> 
> Jan



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/1] xen/pci: Install vpci handlers on x86 and fix exit path
  2021-10-19 16:08 ` [PATCH v2 1/1] xen/pci: Install vpci handlers on x86 and fix exit path Bertrand Marquis
  2021-10-19 23:14   ` Stefano Stabellini
  2021-10-20  7:16   ` Jan Beulich
@ 2021-10-20  7:49   ` Roger Pau Monné
  2021-10-20  7:57     ` Bertrand Marquis
  2021-10-20  8:27   ` Roger Pau Monné
  3 siblings, 1 reply; 16+ messages in thread
From: Roger Pau Monné @ 2021-10-20  7:49 UTC (permalink / raw)
  To: Bertrand Marquis
  Cc: xen-devel, iwj, Oleksandr_Andrushchenko, Stefano Stabellini,
	Julien Grall, Volodymyr Babchuk, Jan Beulich, Andrew Cooper,
	Wei Liu, Paul Durrant

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@suse.com>
> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
> ---
> 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.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/1] xen/pci: Install vpci handlers on x86 and fix exit path
  2021-10-20  7:49   ` Roger Pau Monné
@ 2021-10-20  7:57     ` Bertrand Marquis
  2021-10-20  8:08       ` Roger Pau Monné
  0 siblings, 1 reply; 16+ messages in thread
From: Bertrand Marquis @ 2021-10-20  7:57 UTC (permalink / raw)
  To: Roger Pau Monné
  Cc: Xen-devel, iwj, Oleksandr_Andrushchenko, Stefano Stabellini,
	Julien Grall, Volodymyr Babchuk, Jan Beulich, Andrew Cooper,
	Wei Liu, Paul Durrant

Hi Roger,

> On 20 Oct 2021, at 08:49, Roger Pau Monné <roger.pau@citrix.com> 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@suse.com>
>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
>> ---
>> 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 ?


Regards
Bertrand


> 
> Thanks, Roger.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/1] xen/pci: Install vpci handlers on x86 and fix exit path
  2021-10-20  7:57     ` Bertrand Marquis
@ 2021-10-20  8:08       ` Roger Pau Monné
  2021-10-20  8:24         ` Bertrand Marquis
  0 siblings, 1 reply; 16+ messages in thread
From: Roger Pau Monné @ 2021-10-20  8:08 UTC (permalink / raw)
  To: Bertrand Marquis
  Cc: Xen-devel, iwj, Oleksandr_Andrushchenko, Stefano Stabellini,
	Julien Grall, Volodymyr Babchuk, Jan Beulich, Andrew Cooper,
	Wei Liu, Paul Durrant

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@citrix.com> 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@suse.com>
> >> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
> >> ---
> >> 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.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/1] xen/pci: Install vpci handlers on x86 and fix exit path
  2021-10-20  8:08       ` Roger Pau Monné
@ 2021-10-20  8:24         ` Bertrand Marquis
  0 siblings, 0 replies; 16+ messages in thread
From: Bertrand Marquis @ 2021-10-20  8:24 UTC (permalink / raw)
  To: Roger Pau Monné
  Cc: Xen-devel, iwj, Oleksandr_Andrushchenko, Stefano Stabellini,
	Julien Grall, Volodymyr Babchuk, Jan Beulich, Andrew Cooper,
	Wei Liu, Paul Durrant

Hi Roger,

> On 20 Oct 2021, at 09:08, Roger Pau Monné <roger.pau@citrix.com> wrote:
> 
> 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@citrix.com> 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@suse.com>
>>>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
>>>> ---
>>>> 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.

Ok I will add it then and send v3 this morning.

> 
> 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.

I will try to keep that in mind.
Maybe adding a TODO in the code would be a good idea.

Thanks
Bertrand

> 
> Thanks, Roger.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/1] xen/pci: Install vpci handlers on x86 and fix exit path
  2021-10-19 16:08 ` [PATCH v2 1/1] xen/pci: Install vpci handlers on x86 and fix exit path Bertrand Marquis
                     ` (2 preceding siblings ...)
  2021-10-20  7:49   ` Roger Pau Monné
@ 2021-10-20  8:27   ` Roger Pau Monné
  2021-10-20  8:34     ` Jan Beulich
  3 siblings, 1 reply; 16+ messages in thread
From: Roger Pau Monné @ 2021-10-20  8:27 UTC (permalink / raw)
  To: Bertrand Marquis
  Cc: xen-devel, iwj, Oleksandr_Andrushchenko, Stefano Stabellini,
	Julien Grall, Volodymyr Babchuk, Jan Beulich, Andrew Cooper,
	Wei Liu, Paul Durrant

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@suse.com>
> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
> ---
> 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 )

Sorry to be a pain, but I think this placement of the call to
vpci_add_handlers is bogus and should instead be done strictly after
the device has been added to the hardware_domain->pdev_list list.

Otherwise the loop over domain->pdev_list (for_each_pdev) in
modify_bars won't be able to find the device and hit the assert below
it. That can happen in vpci_add_handlers as it will call init_bars
which in turn will call into modify_bars if memory decoding is enabled
for the device.

Regards, Roger.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/1] xen/pci: Install vpci handlers on x86 and fix exit path
  2021-10-20  8:27   ` Roger Pau Monné
@ 2021-10-20  8:34     ` Jan Beulich
  2021-10-20  8:39       ` Bertrand Marquis
  0 siblings, 1 reply; 16+ messages in thread
From: Jan Beulich @ 2021-10-20  8:34 UTC (permalink / raw)
  To: Roger Pau Monné, Bertrand Marquis
  Cc: xen-devel, iwj, Oleksandr_Andrushchenko, Stefano Stabellini,
	Julien Grall, Volodymyr Babchuk, Andrew Cooper, Wei Liu,
	Paul Durrant

On 20.10.2021 10:27, Roger Pau Monné 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@suse.com>
>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
>> ---
>> 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 )
> 
> Sorry to be a pain, but I think this placement of the call to
> vpci_add_handlers is bogus and should instead be done strictly after
> the device has been added to the hardware_domain->pdev_list list.
> 
> Otherwise the loop over domain->pdev_list (for_each_pdev) in
> modify_bars won't be able to find the device and hit the assert below
> it. That can happen in vpci_add_handlers as it will call init_bars
> which in turn will call into modify_bars if memory decoding is enabled
> for the device.

Oh, good point. And I should have thought of this myself, given that
I did hit that ASSERT() recently with a hidden device. FTAOD I think
this means that the list addition will need to move up (and then
would need undoing on the error path(s)).

Jan



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/1] xen/pci: Install vpci handlers on x86 and fix exit path
  2021-10-20  8:34     ` Jan Beulich
@ 2021-10-20  8:39       ` Bertrand Marquis
  2021-10-20  9:08         ` Roger Pau Monné
  2021-10-20  9:21         ` Jan Beulich
  0 siblings, 2 replies; 16+ messages in thread
From: Bertrand Marquis @ 2021-10-20  8:39 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Roger Pau Monné,
	xen-devel, iwj, Oleksandr_Andrushchenko, Stefano Stabellini,
	Julien Grall, Volodymyr Babchuk, Andrew Cooper, Wei Liu,
	Paul Durrant

Hi,

> On 20 Oct 2021, at 09:34, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 20.10.2021 10:27, Roger Pau Monné 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@suse.com>
>>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
>>> ---
>>> 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 )
>> 
>> Sorry to be a pain, but I think this placement of the call to
>> vpci_add_handlers is bogus and should instead be done strictly after
>> the device has been added to the hardware_domain->pdev_list list.
>> 
>> Otherwise the loop over domain->pdev_list (for_each_pdev) in
>> modify_bars won't be able to find the device and hit the assert below
>> it. That can happen in vpci_add_handlers as it will call init_bars
>> which in turn will call into modify_bars if memory decoding is enabled
>> for the device.
> 
> Oh, good point. And I should have thought of this myself, given that
> I did hit that ASSERT() recently with a hidden device. FTAOD I think
> this means that the list addition will need to move up (and then
> would need undoing on the error path(s)).

Agree, I just need to make sure that calling iommu_add_device is not
impacted by this. It is probably not but a confirmation would be good.

Just to confirm, this specific change would look like that:
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 8928a1c07d..0d8ab2e716 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -756,6 +756,8 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
     if ( !pdev->domain )
     {
         pdev->domain = hardware_domain;
+        list_add(&pdev->domain_list, &hardware_domain->pdev_list);
+
         /*
          * For devices not discovered by Xen during boot, add vPCI handlers
          * when Dom0 first informs Xen about such devices.
@@ -764,6 +766,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
         if ( ret )
         {
             printk(XENLOG_ERR "Setup of vPCI failed: %d\n", ret);
+            list_del(&pdev->domain_list);
             pdev->domain = NULL;
             goto out;
         }
@@ -771,11 +774,10 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
         if ( ret )
         {
             vpci_remove_device(pdev);
+            list_del(&pdev->domain_list);
             pdev->domain = NULL;
             goto out;
         }
-
-        list_add(&pdev->domain_list, &hardware_domain->pdev_list);
     }
     else


Cheers
Bertrand

> 
> Jan
> 
> 


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/1] xen/pci: Install vpci handlers on x86 and fix exit path
  2021-10-20  8:39       ` Bertrand Marquis
@ 2021-10-20  9:08         ` Roger Pau Monné
  2021-10-20  9:21         ` Jan Beulich
  1 sibling, 0 replies; 16+ messages in thread
From: Roger Pau Monné @ 2021-10-20  9:08 UTC (permalink / raw)
  To: Bertrand Marquis
  Cc: Jan Beulich, xen-devel, iwj, Oleksandr_Andrushchenko,
	Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
	Andrew Cooper, Wei Liu, Paul Durrant

On Wed, Oct 20, 2021 at 08:39:48AM +0000, Bertrand Marquis wrote:
> Hi,
> 
> > On 20 Oct 2021, at 09:34, Jan Beulich <jbeulich@suse.com> wrote:
> > 
> > On 20.10.2021 10:27, Roger Pau Monné 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@suse.com>
> >>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
> >>> ---
> >>> 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 )
> >> 
> >> Sorry to be a pain, but I think this placement of the call to
> >> vpci_add_handlers is bogus and should instead be done strictly after
> >> the device has been added to the hardware_domain->pdev_list list.
> >> 
> >> Otherwise the loop over domain->pdev_list (for_each_pdev) in
> >> modify_bars won't be able to find the device and hit the assert below
> >> it. That can happen in vpci_add_handlers as it will call init_bars
> >> which in turn will call into modify_bars if memory decoding is enabled
> >> for the device.
> > 
> > Oh, good point. And I should have thought of this myself, given that
> > I did hit that ASSERT() recently with a hidden device. FTAOD I think
> > this means that the list addition will need to move up (and then
> > would need undoing on the error path(s)).
> 
> Agree, I just need to make sure that calling iommu_add_device is not
> impacted by this. It is probably not but a confirmation would be good.

LGTM. I've been looking but I don't think there's a need to do
iommu_add_device before the call to vpci_add_handlers, so the proposed
solution is fine.

Thanks, Roger.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/1] xen/pci: Install vpci handlers on x86 and fix exit path
  2021-10-20  8:39       ` Bertrand Marquis
  2021-10-20  9:08         ` Roger Pau Monné
@ 2021-10-20  9:21         ` Jan Beulich
  1 sibling, 0 replies; 16+ messages in thread
From: Jan Beulich @ 2021-10-20  9:21 UTC (permalink / raw)
  To: Bertrand Marquis
  Cc: Roger Pau Monné,
	xen-devel, iwj, Oleksandr_Andrushchenko, Stefano Stabellini,
	Julien Grall, Volodymyr Babchuk, Andrew Cooper, Wei Liu,
	Paul Durrant

On 20.10.2021 10:39, Bertrand Marquis wrote:
>> On 20 Oct 2021, at 09:34, Jan Beulich <jbeulich@suse.com> wrote:
>> On 20.10.2021 10:27, Roger Pau Monné wrote:
>>> On Tue, Oct 19, 2021 at 05:08:28PM +0100, Bertrand Marquis wrote:
>>>> --- 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 )
>>>
>>> Sorry to be a pain, but I think this placement of the call to
>>> vpci_add_handlers is bogus and should instead be done strictly after
>>> the device has been added to the hardware_domain->pdev_list list.
>>>
>>> Otherwise the loop over domain->pdev_list (for_each_pdev) in
>>> modify_bars won't be able to find the device and hit the assert below
>>> it. That can happen in vpci_add_handlers as it will call init_bars
>>> which in turn will call into modify_bars if memory decoding is enabled
>>> for the device.
>>
>> Oh, good point. And I should have thought of this myself, given that
>> I did hit that ASSERT() recently with a hidden device. FTAOD I think
>> this means that the list addition will need to move up (and then
>> would need undoing on the error path(s)).
> 
> Agree, I just need to make sure that calling iommu_add_device is not
> impacted by this. It is probably not but a confirmation would be good.

Like Roger, I'm unaware of any issue there. It would be odd anyway for
that (or about anything) to have a "is [not] on list" check. And the
set of list iterations is pretty limited iirc.

Jan



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/1] xen/pci: Install vpci handlers on x86 and fix exit path
  2021-10-20  7:20     ` Bertrand Marquis
@ 2021-10-20 10:51       ` Ian Jackson
  2021-10-20 19:56         ` Stefano Stabellini
  0 siblings, 1 reply; 16+ messages in thread
From: Ian Jackson @ 2021-10-20 10:51 UTC (permalink / raw)
  To: Bertrand Marquis
  Cc: Jan Beulich, Oleksandr_Andrushchenko, Stefano Stabellini,
	Julien Grall, Volodymyr Babchuk, Andrew Cooper,
	Roger Pau Monné,
	Wei Liu, Paul Durrant, xen-devel

Bertrand Marquis writes ("Re: [PATCH v2 1/1] xen/pci: Install vpci handlers on x86 and fix exit path"):
> > On 20 Oct 2021, at 08:16, Jan Beulich <jbeulich@suse.com> wrote:
> > I'm inclined to suggest s/exit/error/ in the title though (and maybe
> > also s/path/paths/), which would be easy enough to do while committing.
> 
> @Ian: Please tell me if this is ok to be fixed during commit.

It's academic now, since you need a respin anyway, but I would in any
case have preferred a new version.  I find that a much better
workflow.  Expecting to make changes on commit pushes work onto
committers from submitters, which is less scaleable.

And it also ends up with committer making changes without any further
human review of any kind.  The committer must do so during an activity
which is otherwise often shallow and administrative.  This is not the
best way to get good results.

I believe I have made these points before, but perhaps not so clearly
and explicitly.  They are IMO very general.  (FTAOD I'm not saying
that making changes on commit is never appropriate, but it ought to be
exceptional.)

Thanks,
Ian.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/1] xen/pci: Install vpci handlers on x86 and fix exit path
  2021-10-20 10:51       ` Ian Jackson
@ 2021-10-20 19:56         ` Stefano Stabellini
  0 siblings, 0 replies; 16+ messages in thread
From: Stefano Stabellini @ 2021-10-20 19:56 UTC (permalink / raw)
  To: Ian Jackson
  Cc: Bertrand Marquis, Jan Beulich, Oleksandr_Andrushchenko,
	Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
	Andrew Cooper, Roger Pau Monné,
	Wei Liu, Paul Durrant, xen-devel

On Wed, 20 Oct 2021, Ian Jackson wrote:
> Bertrand Marquis writes ("Re: [PATCH v2 1/1] xen/pci: Install vpci handlers on x86 and fix exit path"):
> > > On 20 Oct 2021, at 08:16, Jan Beulich <jbeulich@suse.com> wrote:
> > > I'm inclined to suggest s/exit/error/ in the title though (and maybe
> > > also s/path/paths/), which would be easy enough to do while committing.
> > 
> > @Ian: Please tell me if this is ok to be fixed during commit.
> 
> It's academic now, since you need a respin anyway, but I would in any
> case have preferred a new version.  I find that a much better
> workflow.  Expecting to make changes on commit pushes work onto
> committers from submitters, which is less scaleable.
> 
> And it also ends up with committer making changes without any further
> human review of any kind.  The committer must do so during an activity
> which is otherwise often shallow and administrative.  This is not the
> best way to get good results.
> 
> I believe I have made these points before, but perhaps not so clearly
> and explicitly.  They are IMO very general.  (FTAOD I'm not saying
> that making changes on commit is never appropriate, but it ought to be
> exceptional.)

Everything you wrote makes sense and your position is perfectly valid.
At the same time let me write a different perspective on the issue. All
the committers have slightly different workflows so it is natural that
things that are better for one person might not be best for another.

When I commit a patch series often I don't simply take the patches (with
all the necessary acks) and commit them. I typically do a number of
other things:
1) check that the series has all the requested changes compared to the
   previous version
2) check that nothing else was changed unexpectedly
3) run a local build test in a variety of configurations
4) run a full gitlab-ci set of tests with the patches applied

All of the above take time. So if only a small change is required, it is
much faster for me to make the change myself on commit rather than
having to go through steps 1-4 one more time again. Of course this is
only good for small changes with no impact, e.g. code style or in-code
comments, I wouldn't want to do this for anything meaningful.


^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2021-10-20 19:57 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-19 16:08 [PATCH v2 0/1] Fixes: PCI devices passthrough on Arm Bertrand Marquis
2021-10-19 16:08 ` [PATCH v2 1/1] xen/pci: Install vpci handlers on x86 and fix exit path Bertrand Marquis
2021-10-19 23:14   ` Stefano Stabellini
2021-10-20  7:16   ` Jan Beulich
2021-10-20  7:20     ` Bertrand Marquis
2021-10-20 10:51       ` Ian Jackson
2021-10-20 19:56         ` Stefano Stabellini
2021-10-20  7:49   ` Roger Pau Monné
2021-10-20  7:57     ` Bertrand Marquis
2021-10-20  8:08       ` Roger Pau Monné
2021-10-20  8:24         ` Bertrand Marquis
2021-10-20  8:27   ` Roger Pau Monné
2021-10-20  8:34     ` Jan Beulich
2021-10-20  8:39       ` Bertrand Marquis
2021-10-20  9:08         ` Roger Pau Monné
2021-10-20  9:21         ` Jan Beulich

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.