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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxx>
>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>> ---
>> Changes in v2
>> - add comment suggested by Jan on top of vpci_add_handlers call
>> - merge the 3 patches of the serie in one patch and renamed it
>> - fix x86 and arm linker script to only keep vpci_array in rodata and
>> only when CONFIG_VPCI is set.
>> ---
>>  xen/arch/arm/xen.lds.S        | 9 +--------
>>  xen/arch/x86/xen.lds.S        | 9 +--------
>>  xen/drivers/passthrough/pci.c | 8 ++++----
>>  xen/drivers/vpci/vpci.c       | 2 +-
>>  xen/include/xen/vpci.h        | 2 ++
>>  5 files changed, 9 insertions(+), 21 deletions(-)
>>
>> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
>> index b773f91f1c..08016948ab 100644
>> --- a/xen/arch/arm/xen.lds.S
>> +++ b/xen/arch/arm/xen.lds.S
>> @@ -60,7 +60,7 @@ SECTIONS
>>         *(.proc.info)
>>         __proc_info_end = .;
>>  
>> -#if defined(CONFIG_HAS_VPCI) && defined(CONFIG_LATE_HWDOM)
>> +#ifdef CONFIG_HAS_VPCI
>>         . = ALIGN(POINTER_ALIGN);
>>         __start_vpci_array = .;
>>         *(SORT(.data.vpci.*))
>> @@ -189,13 +189,6 @@ SECTIONS
>>         *(.init_array)
>>         *(SORT(.init_array.*))
>>         __ctors_end = .;
>> -
>> -#if defined(CONFIG_HAS_VPCI) && !defined(CONFIG_LATE_HWDOM)
>> -       . = ALIGN(POINTER_ALIGN);
>> -       __start_vpci_array = .;
>> -       *(SORT(.data.vpci.*))
>> -       __end_vpci_array = .;
>> -#endif
>>    } :text
>>    __init_end_efi = .;
>>    . = ALIGN(STACK_SIZE);
>> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
>> index 11b1da2154..87e344d4dd 100644
>> --- a/xen/arch/x86/xen.lds.S
>> +++ b/xen/arch/x86/xen.lds.S
>> @@ -134,7 +134,7 @@ SECTIONS
>>         *(.ex_table.pre)
>>         __stop___pre_ex_table = .;
>>  
>> -#if defined(CONFIG_HAS_VPCI) && defined(CONFIG_LATE_HWDOM)
>> +#ifdef CONFIG_HAS_VPCI
>>         . = ALIGN(POINTER_ALIGN);
>>         __start_vpci_array = .;
>>         *(SORT(.data.vpci.*))
>> @@ -247,13 +247,6 @@ SECTIONS
>>         *(.init_array)
>>         *(SORT(.init_array.*))
>>         __ctors_end = .;
>> -
>> -#if defined(CONFIG_HAS_VPCI) && !defined(CONFIG_LATE_HWDOM)
>> -       . = ALIGN(POINTER_ALIGN);
>> -       __start_vpci_array = .;
>> -       *(SORT(.data.vpci.*))
>> -       __end_vpci_array = .;
>> -#endif
>>    } PHDR(text)
>>  
>>    . = ALIGN(SECTION_ALIGN);
>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>> index 35e0190796..8928a1c07d 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -756,10 +756,9 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>      if ( !pdev->domain )
>>      {
>>          pdev->domain = hardware_domain;
>> -#ifdef CONFIG_ARM
>>          /*
>> -         * On ARM PCI devices discovery will be done by Dom0. Add vpci 
>> handler
>> -         * when Dom0 inform XEN to add the PCI devices in XEN.
>> +         * For devices not discovered by Xen during boot, add vPCI handlers
>> +         * when Dom0 first informs Xen about such devices.
>>           */
>>          ret = vpci_add_handlers(pdev);
>>          if ( ret )
> 
> 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