[PATCH 0/3] Fixes: PCI devices passthrough on Arm

[PATCH 0/3] Fixes: PCI devices passthrough on Arm

[Bertrand Marquis]

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

Bertrand Marquis (3):
  xen/arm: call vpci_add_handlers on x86
  xen/vpci: Remove __hwdom_init for vpci_add_handlers
  xen/pci: Add missing vpci handler cleanup

 xen/drivers/passthrough/pci.c | 8 ++------
 xen/drivers/vpci/vpci.c       | 2 +-
 xen/include/xen/vpci.h        | 2 ++
 3 files changed, 5 insertions(+), 7 deletions(-)

-- 
2.25.1

[PATCH 1/3] xen/arm: call vpci_add_handlers on x86

[Bertrand Marquis]

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.

Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
 xen/drivers/passthrough/pci.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 35e0190796..d7e09448d1 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -756,11 +756,6 @@ 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.
-         */
         ret = vpci_add_handlers(pdev);
         if ( ret )
         {
@@ -768,7 +763,6 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
             pdev->domain = NULL;
             goto out;
         }
-#endif
         ret = iommu_add_device(pdev);
         if ( ret )
         {
-- 
2.25.1

Re: [PATCH 1/3] xen/arm: call vpci_add_handlers on x86

[Jan Beulich]

On 19.10.2021 12:40, Bertrand Marquis wrote:
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -756,11 +756,6 @@ 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.
> -         */
>          ret = vpci_add_handlers(pdev);

I think it wouldn't hurt to retain a (re-worded) comment here, maybe
along the lines of "For devices not discovered by Xen during boot,
add vPCI handlers when Dom0 first informs Xen about such devices."

Jan

Re: [PATCH 1/3] xen/arm: call vpci_add_handlers on x86

[Bertrand Marquis]

Hi Jan,

> On 19 Oct 2021, at 13:29, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 19.10.2021 12:40, Bertrand Marquis wrote:
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -756,11 +756,6 @@ 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.
>> -         */
>>         ret = vpci_add_handlers(pdev);
> 
> I think it wouldn't hurt to retain a (re-worded) comment here, maybe
> along the lines of "For devices not discovered by Xen during boot,
> add vPCI handlers when Dom0 first informs Xen about such devices."

Ok, I will add that in v2.

Cheers
Bertrand

> 
> Jan
> 

[PATCH 2/3] xen/vpci: Remove __hwdom_init for vpci_add_handlers

[Bertrand Marquis]

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.

Fixes: d59168dc05 ("xen/arm: Enable the existing x86 virtual PCI support
for ARM")
Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
 xen/drivers/vpci/vpci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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;
-- 
2.25.1

Re: [PATCH 2/3] xen/vpci: Remove __hwdom_init for vpci_add_handlers

[Jan Beulich]

On 19.10.2021 12:40, Bertrand Marquis wrote:
> --- 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)

A fundamental requirement when altering section attributes is to
also check that all referenced entities are appropriately placed.
Afaict this is not the case for __start_vpci_array[], and you'll
need to also adjust linker scripts to deal with that. Further
you'd have to check that all functions referenced by that array
aren't __hwdom_init. In taking an example (init_msi()) I'm
actually surprised to find it's not marked __hwdom_init. So
maybe all is fine as far as these are concerned.

Jan

Re: [PATCH 2/3] xen/vpci: Remove __hwdom_init for vpci_add_handlers

[Roger Pau Monné]

On Tue, Oct 19, 2021 at 02:39:17PM +0200, Jan Beulich wrote:
> On 19.10.2021 12:40, Bertrand Marquis wrote:
> > --- 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)
> 
> A fundamental requirement when altering section attributes is to
> also check that all referenced entities are appropriately placed.
> Afaict this is not the case for __start_vpci_array[], and you'll
> need to also adjust linker scripts to deal with that.

Indeed, we need to just keep the CONFIG_LATE_HWDOM placement in
.rodata.

> Further
> you'd have to check that all functions referenced by that array
> aren't __hwdom_init. In taking an example (init_msi()) I'm
> actually surprised to find it's not marked __hwdom_init. So
> maybe all is fine as far as these are concerned.

My bad, I've forgot to mark the initializers used by
REGISTER_VPCI_INIT as __hwdom_init. I think there's no need for a
change there.

Thanks, Roger.

Re: [PATCH 2/3] xen/vpci: Remove __hwdom_init for vpci_add_handlers

[Jan Beulich]

On 19.10.2021 15:17, Roger Pau Monné wrote:
> On Tue, Oct 19, 2021 at 02:39:17PM +0200, Jan Beulich wrote:
>> On 19.10.2021 12:40, Bertrand Marquis wrote:
>>> --- 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)
>>
>> A fundamental requirement when altering section attributes is to
>> also check that all referenced entities are appropriately placed.
>> Afaict this is not the case for __start_vpci_array[], and you'll
>> need to also adjust linker scripts to deal with that.
> 
> Indeed, we need to just keep the CONFIG_LATE_HWDOM placement in
> .rodata.
> 
>> Further
>> you'd have to check that all functions referenced by that array
>> aren't __hwdom_init. In taking an example (init_msi()) I'm
>> actually surprised to find it's not marked __hwdom_init. So
>> maybe all is fine as far as these are concerned.
> 
> My bad, I've forgot to mark the initializers used by
> REGISTER_VPCI_INIT as __hwdom_init. I think there's no need for a
> change there.

Not anymore, indeed. And less code churn now as a result.

Jan

Re: [PATCH 2/3] xen/vpci: Remove __hwdom_init for vpci_add_handlers

[Bertrand Marquis]

Hi,

> On 19 Oct 2021, at 14:17, Roger Pau Monné <roger.pau@citrix.com> wrote:
> 
> On Tue, Oct 19, 2021 at 02:39:17PM +0200, Jan Beulich wrote:
>> On 19.10.2021 12:40, Bertrand Marquis wrote:
>>> --- 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)
>> 
>> A fundamental requirement when altering section attributes is to
>> also check that all referenced entities are appropriately placed.
>> Afaict this is not the case for __start_vpci_array[], and you'll
>> need to also adjust linker scripts to deal with that.
> 
> Indeed, we need to just keep the CONFIG_LATE_HWDOM placement in
> .rodata.

I will also need to remove the test for CONFIG_LATE_HWDOM in there
and only test for CONFIG_HAS_VPCI.
This will be applied to both arm and x86 linker script.

> 
>> Further
>> you'd have to check that all functions referenced by that array
>> aren't __hwdom_init. In taking an example (init_msi()) I'm
>> actually surprised to find it's not marked __hwdom_init. So
>> maybe all is fine as far as these are concerned.
> 
> My bad, I've forgot to mark the initializers used by
> REGISTER_VPCI_INIT as __hwdom_init. I think there's no need for a
> change there.

Thanks for the confirmation here. I checked in the code and did not find
anything and was looking again. But I definitely missed the linker script.

I will send a v2 squashing the 3 patches together and modifying the linker
scripts.

Thanks
Bertrand

> 
> Thanks, Roger.

[PATCH 3/3] xen/pci: Add missing vpci handler cleanup

[Bertrand Marquis]

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")
Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
 xen/drivers/passthrough/pci.c | 2 ++
 xen/include/xen/vpci.h        | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index d7e09448d1..ddf08a3fae 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -766,6 +766,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
         ret = iommu_add_device(pdev);
         if ( ret )
         {
+            vpci_remove_device(pdev);
             pdev->domain = NULL;
             goto out;
         }
@@ -813,6 +814,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/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

Re: [PATCH 0/3] Fixes: PCI devices passthrough on Arm

[Ian Jackson]

Bertrand Marquis writes ("[PATCH 0/3] Fixes: PCI devices passthrough on Arm"):
> 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

Thanks.  Roger, Jan, what do you think of this ?

I have no qualms from my RM POV other than that I want a fix
resolves the concenrs previously expressed by maintainers.

Thanks,
Ian.

Re: [PATCH 0/3] Fixes: PCI devices passthrough on Arm

[Jan Beulich]

On 19.10.2021 13:12, Ian Jackson wrote:
> Bertrand Marquis writes ("[PATCH 0/3] Fixes: PCI devices passthrough on Arm"):
>> 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
> 
> Thanks.  Roger, Jan, what do you think of this ?

I'll get to this, but at the first glance I'd say that the change in
patch 1 coming before what patch 2 does is already a problem.

Jan

Re: [PATCH 0/3] Fixes: PCI devices passthrough on Arm

[Bertrand Marquis]

Hi Jan,

> On 19 Oct 2021, at 12:25, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 19.10.2021 13:12, Ian Jackson wrote:
>> Bertrand Marquis writes ("[PATCH 0/3] Fixes: PCI devices passthrough on Arm"):
>>> 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
>> 
>> Thanks.  Roger, Jan, what do you think of this ?
> 
> I'll get to this, but at the first glance I'd say that the change in
> patch 1 coming before what patch 2 does is already a problem.

Because path 1 is actually introducing a bug solved by patch 2, I should have thought of that.

I can either invert those 2 (or actually put patch 1 at the end) or merge patch 1 and 2 together.

Bertrand

> 
> Jan
> 

Re: [PATCH 0/3] Fixes: PCI devices passthrough on Arm

[Jan Beulich]

On 19.10.2021 12:40, Bertrand Marquis wrote:
> 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
> 
> Bertrand Marquis (3):
>   xen/arm: call vpci_add_handlers on x86
>   xen/vpci: Remove __hwdom_init for vpci_add_handlers
>   xen/pci: Add missing vpci handler cleanup

Imo all three changes need to be in a single patch. An option might be
to have first what is now patch 3, with CONFIG_ARM conditionals, which
the subsequent patch would then delete. But what is now patch 1 cannot
come before patch 2, and patch 2 alone would unduly impact x86 by
moving code from .init.text to .text for no reason. (Still it could
technically be done that way.)

But let me also comment patches 1 and 2 individually (patch 3 looks
okay, apart from the ordering issue).

Jan

Re: [PATCH 0/3] Fixes: PCI devices passthrough on Arm

[Bertrand Marquis]

> On 19 Oct 2021, at 13:26, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 19.10.2021 12:40, Bertrand Marquis wrote:
>> 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
>> 
>> Bertrand Marquis (3):
>>  xen/arm: call vpci_add_handlers on x86
>>  xen/vpci: Remove __hwdom_init for vpci_add_handlers
>>  xen/pci: Add missing vpci handler cleanup
> 
> Imo all three changes need to be in a single patch.

I will merge all changes in one patch.

> An option might be
> to have first what is now patch 3, with CONFIG_ARM conditionals, which
> the subsequent patch would then delete. But what is now patch 1 cannot
> come before patch 2, and patch 2 alone would unduly impact x86 by
> moving code from .init.text to .text for no reason. (Still it could
> technically be done that way.)
> 
> But let me also comment patches 1 and 2 individually (patch 3 looks
> okay, apart from the ordering issue).

Thanks
Bertrand

> 
> Jan