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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Roger,

> On 20 Oct 2021, at 09:08, Roger Pau Monné <roger.pau@xxxxxxxxxx> 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@xxxxxxxxxx> 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 )
>>>> @@ -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.