| Message ID | 20230829231912.4091958-5-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI devices passthrough on Arm, part 3 | expand |
On 30.08.2023 01:19, Volodymyr Babchuk wrote: > @@ -1481,6 +1488,13 @@ 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; > > + if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) ) > + { > + write_lock(&pdev->domain->pci_lock); > + vpci_deassign_device(pdev); > + write_unlock(&pdev->domain->pci_lock); > + } Why is the DomIO special case ... > @@ -1506,6 +1520,15 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag) > rc = iommu_call(hd->platform_ops, assign_device, d, devfn, > pci_to_dev(pdev), flag); > } > + if ( rc ) > + goto done; > + > + if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) && d != dom_io) > + { > + write_lock(&d->pci_lock); > + rc = vpci_assign_device(pdev); > + write_unlock(&d->pci_lock); > + } ... relevant only here? Jan
Hi Jan, Jan Beulich <jbeulich@suse.com> writes: > On 30.08.2023 01:19, Volodymyr Babchuk wrote: >> @@ -1481,6 +1488,13 @@ 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; >> >> + if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) ) >> + { >> + write_lock(&pdev->domain->pci_lock); >> + vpci_deassign_device(pdev); >> + write_unlock(&pdev->domain->pci_lock); >> + } > > Why is the DomIO special case ... vpci_deassign_device() does nothing if vPCI was initialized for a domain. So it not wrong to call this function even if pdev belongs to dom_io. >> @@ -1506,6 +1520,15 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag) >> rc = iommu_call(hd->platform_ops, assign_device, d, devfn, >> pci_to_dev(pdev), flag); >> } >> + if ( rc ) >> + goto done; >> + >> + if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) && d != dom_io) >> + { >> + write_lock(&d->pci_lock); >> + rc = vpci_assign_device(pdev); >> + write_unlock(&d->pci_lock); >> + } > > ... relevant only here? > There is no sense to initialize vPCI for dom_io.
On 13.09.2023 01:41, Volodymyr Babchuk wrote: > Jan Beulich <jbeulich@suse.com> writes: >> On 30.08.2023 01:19, Volodymyr Babchuk wrote: >>> @@ -1481,6 +1488,13 @@ 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; >>> >>> + if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) ) >>> + { >>> + write_lock(&pdev->domain->pci_lock); >>> + vpci_deassign_device(pdev); >>> + write_unlock(&pdev->domain->pci_lock); >>> + } >> >> Why is the DomIO special case ... > > vpci_deassign_device() does nothing if vPCI was initialized for a > domain. So it not wrong to call this function even if pdev belongs to dom_io. Well, okay, but then you acquire a lock just to do nothing (apart from the apparent asymmetry). >>> @@ -1506,6 +1520,15 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag) >>> rc = iommu_call(hd->platform_ops, assign_device, d, devfn, >>> pci_to_dev(pdev), flag); >>> } >>> + if ( rc ) >>> + goto done; >>> + >>> + if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) && d != dom_io) >>> + { >>> + write_lock(&d->pci_lock); >>> + rc = vpci_assign_device(pdev); >>> + write_unlock(&d->pci_lock); >>> + } >> >> ... relevant only here? >> > > There is no sense to initialize vPCI for dom_io. Of course. Jan
Hi, Jan Beulich <jbeulich@suse.com> writes: > On 13.09.2023 01:41, Volodymyr Babchuk wrote: >> Jan Beulich <jbeulich@suse.com> writes: >>> On 30.08.2023 01:19, Volodymyr Babchuk wrote: >>>> @@ -1481,6 +1488,13 @@ 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; >>>> >>>> + if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) ) >>>> + { >>>> + write_lock(&pdev->domain->pci_lock); >>>> + vpci_deassign_device(pdev); >>>> + write_unlock(&pdev->domain->pci_lock); >>>> + } >>> >>> Why is the DomIO special case ... >> >> vpci_deassign_device() does nothing if vPCI was initialized for a >> domain. So it not wrong to call this function even if pdev belongs to dom_io. > > Well, okay, but then you acquire a lock just to do nothing (apart > from the apparent asymmetry). Yes, I agree. I'll add the same check as below. Thanks for the review.
On Tue, Aug 29, 2023 at 11:19:43PM +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> > --- > Since 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 > Since v8: > - removed vpci_deassign_device > Since 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 > Since 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 > Since 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 > Since 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 > Since v2: > - define CONFIG_HAS_VPCI_GUEST_SUPPORT so dead code is not compiled > for x86 > Since v1: > - constify struct pci_dev where possible > - do not open code is_system_domain() > - extended the commit message > --- > xen/drivers/Kconfig | 4 ++++ > xen/drivers/passthrough/pci.c | 31 +++++++++++++++++++++++++++---- > xen/drivers/vpci/header.c | 2 +- > xen/drivers/vpci/vpci.c | 6 +++--- > xen/include/xen/vpci.h | 10 +++++----- > 5 files changed, 40 insertions(+), 13 deletions(-) > > diff --git a/xen/drivers/Kconfig b/xen/drivers/Kconfig > index db94393f47..780490cf8e 100644 > --- a/xen/drivers/Kconfig > +++ b/xen/drivers/Kconfig > @@ -15,4 +15,8 @@ source "drivers/video/Kconfig" > config HAS_VPCI > bool > > +config HAS_VPCI_GUEST_SUPPORT > + bool > + depends on HAS_VPCI > + > endmenu > diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c > index 4f18293900..64281f2d5e 100644 > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -757,7 +757,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 ) > { > printk(XENLOG_ERR "Setup of vPCI failed: %d\n", ret); > @@ -771,7 +771,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; > @@ -819,7 +819,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 ) > @@ -877,6 +877,13 @@ static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus, > goto out; > } > > + if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) ) > + { > + write_lock(&d->pci_lock); > + vpci_deassign_device(pdev); > + write_unlock(&d->pci_lock); > + } I'm confused by this one, shouldn't the code rely on has_vpci() instead? (which is already checked for in vpci_deassign_device()). If you have a system without CONFIG_HAS_VPCI_GUEST_SUPPORT but vPCI is used by dom0 you likely still need the hooks in {,de}assign_device() so that vPCI status is properly handled for dom0 as the devices get deassigned to dom0 and assigned to a guest? (and maybe moved back to dom0 at a later point). > + > devfn = pdev->devfn; > ret = iommu_call(hd->platform_ops, reassign_device, d, target, devfn, > pci_to_dev(pdev)); > @@ -1147,7 +1154,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", > @@ -1481,6 +1488,13 @@ 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; > > + if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) ) > + { > + 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; > @@ -1506,6 +1520,15 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag) > rc = iommu_call(hd->platform_ops, assign_device, d, devfn, > pci_to_dev(pdev), flag); > } > + if ( rc ) > + goto done; rc can't be != 0 here, as the increment statement in the for loop above will zero rc at each iteration. > + > + if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) && d != dom_io) > + { > + write_lock(&d->pci_lock); > + rc = vpci_assign_device(pdev); > + write_unlock(&d->pci_lock); > + } Why do you need the extra d != dom_io check here? has_vpci() will fail for dom_io, no need to special case it here. Thanks, Roger.
On Wed, Sep 13, 2023 at 11:53:56PM +0000, Volodymyr Babchuk wrote: > > Hi, > > Jan Beulich <jbeulich@suse.com> writes: > > > On 13.09.2023 01:41, Volodymyr Babchuk wrote: > >> Jan Beulich <jbeulich@suse.com> writes: > >>> On 30.08.2023 01:19, Volodymyr Babchuk wrote: > >>>> @@ -1481,6 +1488,13 @@ 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; > >>>> > >>>> + if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) ) > >>>> + { > >>>> + write_lock(&pdev->domain->pci_lock); > >>>> + vpci_deassign_device(pdev); > >>>> + write_unlock(&pdev->domain->pci_lock); > >>>> + } > >>> > >>> Why is the DomIO special case ... > >> > >> vpci_deassign_device() does nothing if vPCI was initialized for a > >> domain. So it not wrong to call this function even if pdev belongs to dom_io. > > > > Well, okay, but then you acquire a lock just to do nothing (apart > > from the apparent asymmetry). > > Yes, I agree. I'll add the same check as below. Thanks for the review. Hm, no, I would rather rely on the has_vpci check inside of vpci_{,de}assign_device() than open coding dom_io checks elsewhere. This is not a hot path, the extra lock taking is likely negligible compared to the cost of assign operations. Thanks, Roger.
diff --git a/xen/drivers/Kconfig b/xen/drivers/Kconfig index db94393f47..780490cf8e 100644 --- a/xen/drivers/Kconfig +++ b/xen/drivers/Kconfig @@ -15,4 +15,8 @@ source "drivers/video/Kconfig" config HAS_VPCI bool +config HAS_VPCI_GUEST_SUPPORT + bool + depends on HAS_VPCI + endmenu diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index 4f18293900..64281f2d5e 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -757,7 +757,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 ) { printk(XENLOG_ERR "Setup of vPCI failed: %d\n", ret); @@ -771,7 +771,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; @@ -819,7 +819,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 ) @@ -877,6 +877,13 @@ static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus, goto out; } + if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) ) + { + 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)); @@ -1147,7 +1154,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", @@ -1481,6 +1488,13 @@ 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; + if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) ) + { + 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; @@ -1506,6 +1520,15 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag) rc = iommu_call(hd->platform_ops, assign_device, d, devfn, pci_to_dev(pdev), flag); } + if ( rc ) + goto done; + + if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) && d != dom_io) + { + write_lock(&d->pci_lock); + rc = vpci_assign_device(pdev); + write_unlock(&d->pci_lock); + } done: if ( rc ) diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c index 177a6b57a5..3b797df82f 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 cb45904114..135d390218 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 0b8a2a3c74..2a0ae34500 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 *dev); +/* Assign vPCI to device by adding handlers to device. */ +int __must_check vpci_assign_device(struct pci_dev *dev); /* 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) { }