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

Bertrand Marquis posted 3 patches 2 years, 5 months ago
Failed in applying to current master (apply log)
There is a newer version of this series
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(-)
[PATCH 0/3] Fixes: PCI devices passthrough on Arm
Posted by Bertrand Marquis 2 years, 5 months ago
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


Re: [PATCH 0/3] Fixes: PCI devices passthrough on Arm
Posted by Ian Jackson 2 years, 5 months ago
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
Posted by Jan Beulich 2 years, 5 months ago
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
Posted by Bertrand Marquis 2 years, 5 months ago
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
Posted by Jan Beulich 2 years, 5 months ago
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
Posted by Bertrand Marquis 2 years, 5 months ago

> 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