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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Oct 20, 2021 at 08:39:48AM +0000, Bertrand Marquis wrote:
> 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.

LGTM. I've been looking but I don't think there's a need to do
iommu_add_device before the call to vpci_add_handlers, so the proposed
solution is fine.

Thanks, Roger.