Re: [PATCH v3 1/1] xen/pci: Install vpci handlers on x86 and fix error paths

[Julien Grall <julien@xxxxxxx>]

Hi Bertrand,

On 20/10/2021 11:05, 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.
>
> Move code adding the domain to the pdev domain_list as vpci_add_handlers
> needs this to be set and remove it from the list in the error path.
>
> Exit early of vpci_remove_device if the domain has no vpci support.
>
> Add empty static inline for vpci_remove_device when CONFIG_VPCI is not
> defined.
>
> Add an ASSERT in vpci_add_handlers to check that the function is not
> called twice for the same device.
>
> 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>
For the arm bits:

Acked-by: Julien Grall <jgrall@xxxxxxxxxx>

Cheers,

> ---
> Changes in v3
> - change title (s/exit/error/ and s/path/paths)
> - add early exit in vpci_remove_device if the domain has no vpci support
> - add ASSERT in vpci_add_handlers to check that the call is only made
> once per device
> - move the call adding the domain in the pdev domain list and remove it
> in the error path
> 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 | 14 ++++++++------
>   xen/drivers/vpci/vpci.c       |  8 +++++++-
>   xen/include/xen/vpci.h        |  2 ++
>   5 files changed, 19 insertions(+), 23 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..0d8ab2e716 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -756,27 +756,28 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>       if ( !pdev->domain )
>       {
>           pdev->domain = hardware_domain;
> -#ifdef CONFIG_ARM
> +        list_add(&pdev->domain_list, &hardware_domain->pdev_list);
> +
>           /*
> -         * 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 )
>           {
>               printk(XENLOG_ERR "Setup of vPCI failed: %d\n", ret);
> +            list_del(&pdev->domain_list);
>               pdev->domain = NULL;
>               goto out;
>           }
> -#endif
>           ret = iommu_add_device(pdev);
>           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
>           iommu_enable_device(pdev);
> @@ -819,6 +820,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);
>               pci_cleanup_msi(pdev);
>               ret = iommu_remove_device(pdev);
>               if ( pdev->domain )
> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> index decf7d87a1..657697fe34 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -37,6 +37,9 @@ extern vpci_register_init_t *const __end_vpci_array[];
>   
>   void vpci_remove_device(struct pci_dev *pdev)
>   {
> +    if ( !has_vpci(pdev->domain) )
> +        return;
> +
>       spin_lock(&pdev->vpci->lock);
>       while ( !list_empty(&pdev->vpci->handlers) )
>       {
> @@ -54,7 +57,7 @@ void vpci_remove_device(struct pci_dev *pdev)
>       pdev->vpci = NULL;
>   }
>   
> -int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
> +int vpci_add_handlers(struct pci_dev *pdev)
>   {
>       unsigned int i;
>       int rc = 0;
> @@ -62,6 +65,9 @@ int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
>       if ( !has_vpci(pdev->domain) )
>           return 0;
>   
> +    /* We should not get here twice for the same device. */
> +    ASSERT(!pdev->vpci);
> +
>       pdev->vpci = xzalloc(struct vpci);
>       if ( !pdev->vpci )
>           return -ENOMEM;
> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
> index 6746c2589a..9ea66e033f 100644
> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -230,6 +230,8 @@ static inline int vpci_add_handlers(struct pci_dev *pdev)
>       return 0;
>   }
>   
> +static inline void vpci_remove_device(struct pci_dev *pdev) { }
> +
>   static inline void vpci_dump_msi(void) { }
>   
>   static inline uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg,
--
Julien Grall