| Message ID | 20231012220854.2736994-6-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI devices passthrough on Arm, part 3 | expand |
On Thu, Oct 12, 2023 at 10:09:15PM +0000, Volodymyr Babchuk wrote: > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > When a PCI device gets assigned/de-assigned we need to > initialize/de-initialize vPCI state for the device. > > Also, rename vpci_add_handlers() to vpci_assign_device() and > vpci_remove_device() to vpci_deassign_device() to better reflect role > of the functions. > > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> > --- > > In v10: > - removed HAS_VPCI_GUEST_SUPPORT checks > - HAS_VPCI_GUEST_SUPPORT config option (in Kconfig) as it is not used > anywhere > In v9: > - removed previous vpci_[de]assign_device function and renamed > existing handlers > - dropped attempts to handle errors in assign_device() function > - do not call vpci_assign_device for dom_io > - use d instead of pdev->domain > - use IS_ENABLED macro > In v8: > - removed vpci_deassign_device > In v6: > - do not pass struct domain to vpci_{assign|deassign}_device as > pdev->domain can be used > - do not leave the device assigned (pdev->domain == new domain) in case > vpci_assign_device fails: try to de-assign and if this also fails, then > crash the domain > In v5: > - do not split code into run_vpci_init > - do not check for is_system_domain in vpci_{de}assign_device > - do not use vpci_remove_device_handlers_locked and re-allocate > pdev->vpci completely > - make vpci_deassign_device void > In v4: > - de-assign vPCI from the previous domain on device assignment > - do not remove handlers in vpci_assign_device as those must not > exist at that point > In v3: > - remove toolstack roll-back description from the commit message > as error are to be handled with proper cleanup in Xen itself > - remove __must_check > - remove redundant rc check while assigning devices > - fix redundant CONFIG_HAS_VPCI check for CONFIG_HAS_VPCI_GUEST_SUPPORT > - use REGISTER_VPCI_INIT machinery to run required steps on device > init/assign: add run_vpci_init helper > In v2: > - define CONFIG_HAS_VPCI_GUEST_SUPPORT so dead code is not compiled > for x86 > In v1: > - constify struct pci_dev where possible > - do not open code is_system_domain() > - extended the commit message > --- > xen/drivers/passthrough/pci.c | 20 ++++++++++++++++---- > xen/drivers/vpci/header.c | 2 +- > xen/drivers/vpci/vpci.c | 6 +++--- > xen/include/xen/vpci.h | 10 +++++----- > 4 files changed, 25 insertions(+), 13 deletions(-) > > diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c > index 182da45acb..b7926a291c 100644 > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -755,7 +755,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, > * For devices not discovered by Xen during boot, add vPCI handlers > * when Dom0 first informs Xen about such devices. > */ > - ret = vpci_add_handlers(pdev); > + ret = vpci_assign_device(pdev); > if ( ret ) > { > list_del(&pdev->domain_list); > @@ -769,7 +769,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, > if ( ret ) > { > write_lock(&hardware_domain->pci_lock); > - vpci_remove_device(pdev); > + vpci_deassign_device(pdev); > list_del(&pdev->domain_list); > write_unlock(&hardware_domain->pci_lock); > pdev->domain = NULL; > @@ -817,7 +817,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_deassign_device(pdev); > pci_cleanup_msi(pdev); > ret = iommu_remove_device(pdev); > if ( pdev->domain ) > @@ -875,6 +875,10 @@ static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus, > goto out; > } > > + write_lock(&d->pci_lock); > + vpci_deassign_device(pdev); > + write_unlock(&d->pci_lock); > + > devfn = pdev->devfn; > ret = iommu_call(hd->platform_ops, reassign_device, d, target, devfn, > pci_to_dev(pdev)); In deassign_device() you are missing a call to vpci_assign_device() in order to setup the vPCI handlers for the target domain (not for dom_io, but possibly for hardware_domain if it's PVH-like). If the call to reassign_device is successful you need to call vpci_assign_device(). The rest LGTM. Thanks, Roger.
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index 182da45acb..b7926a291c 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -755,7 +755,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, * For devices not discovered by Xen during boot, add vPCI handlers * when Dom0 first informs Xen about such devices. */ - ret = vpci_add_handlers(pdev); + ret = vpci_assign_device(pdev); if ( ret ) { list_del(&pdev->domain_list); @@ -769,7 +769,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, if ( ret ) { write_lock(&hardware_domain->pci_lock); - vpci_remove_device(pdev); + vpci_deassign_device(pdev); list_del(&pdev->domain_list); write_unlock(&hardware_domain->pci_lock); pdev->domain = NULL; @@ -817,7 +817,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_deassign_device(pdev); pci_cleanup_msi(pdev); ret = iommu_remove_device(pdev); if ( pdev->domain ) @@ -875,6 +875,10 @@ static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus, goto out; } + write_lock(&d->pci_lock); + vpci_deassign_device(pdev); + write_unlock(&d->pci_lock); + devfn = pdev->devfn; ret = iommu_call(hd->platform_ops, reassign_device, d, target, devfn, pci_to_dev(pdev)); @@ -1146,7 +1150,7 @@ static void __hwdom_init setup_one_hwdom_device(const struct setup_hwdom *ctxt, PCI_SLOT(devfn) == PCI_SLOT(pdev->devfn) ); write_lock(&ctxt->d->pci_lock); - err = vpci_add_handlers(pdev); + err = vpci_assign_device(pdev); write_unlock(&ctxt->d->pci_lock); if ( err ) printk(XENLOG_ERR "setup of vPCI for d%d failed: %d\n", @@ -1476,6 +1480,10 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag) if ( pdev->broken && d != hardware_domain && d != dom_io ) goto done; + write_lock(&pdev->domain->pci_lock); + vpci_deassign_device(pdev); + write_unlock(&pdev->domain->pci_lock); + rc = pdev_msix_assign(d, pdev); if ( rc ) goto done; @@ -1502,6 +1510,10 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag) pci_to_dev(pdev), flag); } + write_lock(&d->pci_lock); + rc = vpci_assign_device(pdev); + write_unlock(&d->pci_lock); + done: if ( rc ) printk(XENLOG_G_WARNING "%pd: assign (%pp) failed (%d)\n", diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c index a52e52db96..176fe16b9f 100644 --- a/xen/drivers/vpci/header.c +++ b/xen/drivers/vpci/header.c @@ -190,7 +190,7 @@ bool vpci_process_pending(struct vcpu *v) * killed in order to avoid leaking stale p2m mappings on * failure. */ - vpci_remove_device(v->vpci.pdev); + vpci_deassign_device(v->vpci.pdev); write_unlock(&v->domain->pci_lock); } diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c index 724987e981..b20bee2b0b 100644 --- a/xen/drivers/vpci/vpci.c +++ b/xen/drivers/vpci/vpci.c @@ -36,7 +36,7 @@ extern vpci_register_init_t *const __start_vpci_array[]; extern vpci_register_init_t *const __end_vpci_array[]; #define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array) -void vpci_remove_device(struct pci_dev *pdev) +void vpci_deassign_device(struct pci_dev *pdev) { ASSERT(rw_is_write_locked(&pdev->domain->pci_lock)); @@ -69,7 +69,7 @@ void vpci_remove_device(struct pci_dev *pdev) pdev->vpci = NULL; } -int vpci_add_handlers(struct pci_dev *pdev) +int vpci_assign_device(struct pci_dev *pdev) { unsigned int i; const unsigned long *ro_map; @@ -103,7 +103,7 @@ int vpci_add_handlers(struct pci_dev *pdev) } if ( rc ) - vpci_remove_device(pdev); + vpci_deassign_device(pdev); return rc; } diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h index d743d96a10..75cfb532ee 100644 --- a/xen/include/xen/vpci.h +++ b/xen/include/xen/vpci.h @@ -25,11 +25,11 @@ typedef int vpci_register_init_t(struct pci_dev *dev); static vpci_register_init_t *const x##_entry \ __used_section(".data.vpci." p) = x -/* Add vPCI handlers to device. */ -int __must_check vpci_add_handlers(struct pci_dev *pdev); +/* Assign vPCI to device by adding handlers to device. */ +int __must_check vpci_assign_device(struct pci_dev *pdev); /* Remove all handlers and free vpci related structures. */ -void vpci_remove_device(struct pci_dev *pdev); +void vpci_deassign_device(struct pci_dev *pdev); /* Add/remove a register handler. */ int __must_check vpci_add_register(struct vpci *vpci, @@ -235,12 +235,12 @@ bool vpci_ecam_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int len, #else /* !CONFIG_HAS_VPCI */ struct vpci_vcpu {}; -static inline int vpci_add_handlers(struct pci_dev *pdev) +static inline int vpci_assign_device(struct pci_dev *pdev) { return 0; } -static inline void vpci_remove_device(struct pci_dev *pdev) { } +static inline void vpci_deassign_device(struct pci_dev *pdev) { } static inline void vpci_dump_msi(void) { }