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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

> On 20 Oct 2021, at 09:34, Jan Beulich <jbeulich@xxxxxxxx> 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@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)).

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