| Message ID | c82cc9c933e09806c9d043c61d92bd793060f9ab.1634723903.git.bertrand.marquis@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes: PCI devices passthrough on Arm | expand |
On Wed, Oct 20, 2021 at 11:05:37AM +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. > > 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@suse.com> > Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks!
On 20.10.2021 12: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@suse.com> > Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> This looks to address all review comments, so Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks, Jan
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@suse.com> > Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> For the arm bits: Acked-by: Julien Grall <jgrall@amazon.com> 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, >
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,
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@suse.com> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> --- 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(-)