| Message ID | 20230720003205.1828537-12-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI devices passthrough on Arm, part 3 | expand |
On 20.07.2023 02:32, Volodymyr Babchuk wrote: > --- a/xen/drivers/vpci/vpci.c > +++ b/xen/drivers/vpci/vpci.c > @@ -46,6 +46,16 @@ void vpci_remove_device(struct pci_dev *pdev) > return; > > spin_lock(&pdev->vpci->lock); > + > +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT > + if ( pdev->vpci->guest_sbdf.sbdf != ~0 ) > + { > + __clear_bit(pdev->vpci->guest_sbdf.dev, > + &pdev->domain->vpci_dev_assigned_map); > + pdev->vpci->guest_sbdf.sbdf = ~0; > + } > +#endif The lock acquired above is not ... > @@ -115,6 +129,54 @@ int vpci_add_handlers(struct pci_dev *pdev) > } > > #ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT > +static int add_virtual_device(struct pci_dev *pdev) > +{ > + struct domain *d = pdev->domain; > + pci_sbdf_t sbdf = { 0 }; > + unsigned long new_dev_number; > + > + if ( is_hardware_domain(d) ) > + return 0; > + > + ASSERT(pcidevs_locked()); > + > + /* > + * Each PCI bus supports 32 devices/slots at max or up to 256 when > + * there are multi-function ones which are not yet supported. > + */ > + if ( pdev->info.is_extfn ) > + { > + gdprintk(XENLOG_ERR, "%pp: only function 0 passthrough supported\n", > + &pdev->sbdf); > + return -EOPNOTSUPP; > + } > + > + write_lock(&pdev->domain->pci_lock); > + new_dev_number = find_first_zero_bit(d->vpci_dev_assigned_map, > + VPCI_MAX_VIRT_DEV); > + if ( new_dev_number >= VPCI_MAX_VIRT_DEV ) > + { > + write_unlock(&pdev->domain->pci_lock); > + return -ENOSPC; > + } > + > + __set_bit(new_dev_number, &d->vpci_dev_assigned_map); ... the same as the one held here, so the bitmap still isn't properly protected afaics, unless the intention is to continue to rely on the global PCI lock (assuming that one's held in both cases, which I didn't check it is). Conversely it looks like the vPCI lock isn't held here. Both aspects may be intentional, but the locks being acquired differing requires suitable code comments imo. I've also briefly looked at patch 1, and I'm afraid that still lacks commentary about intended lock nesting. That might be relevant here in case locking visible from patch / patch context isn't providing the full picture. > + /* > + * Both segment and bus number are 0: > + * - we emulate a single host bridge for the guest, e.g. segment 0 > + * - with bus 0 the virtual devices are seen as embedded > + * endpoints behind the root complex > + * > + * TODO: add support for multi-function devices. > + */ > + sbdf.devfn = PCI_DEVFN(new_dev_number, 0); > + pdev->vpci->guest_sbdf = sbdf; > + write_unlock(&pdev->domain->pci_lock); With the above I also wonder whether this lock can't (and hence should) be dropped a little earlier (right after fiddling with the bitmap). Jan
Hi Jan, Jan Beulich <jbeulich@suse.com> writes: > On 20.07.2023 02:32, Volodymyr Babchuk wrote: >> --- a/xen/drivers/vpci/vpci.c >> +++ b/xen/drivers/vpci/vpci.c >> @@ -46,6 +46,16 @@ void vpci_remove_device(struct pci_dev *pdev) >> return; >> >> spin_lock(&pdev->vpci->lock); >> + >> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT >> + if ( pdev->vpci->guest_sbdf.sbdf != ~0 ) >> + { >> + __clear_bit(pdev->vpci->guest_sbdf.dev, >> + &pdev->domain->vpci_dev_assigned_map); >> + pdev->vpci->guest_sbdf.sbdf = ~0; >> + } >> +#endif > > The lock acquired above is not ... vpci_remove_device() is called when d->pci_lock is already held. But, I'll move this hunk before spin_lock(&pdev->vpci->lock); we don't need to hold it while cleaning vpci_dev_assigned_map >> @@ -115,6 +129,54 @@ int vpci_add_handlers(struct pci_dev *pdev) >> } >> >> #ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT >> +static int add_virtual_device(struct pci_dev *pdev) >> +{ >> + struct domain *d = pdev->domain; >> + pci_sbdf_t sbdf = { 0 }; >> + unsigned long new_dev_number; >> + >> + if ( is_hardware_domain(d) ) >> + return 0; >> + >> + ASSERT(pcidevs_locked()); >> + >> + /* >> + * Each PCI bus supports 32 devices/slots at max or up to 256 when >> + * there are multi-function ones which are not yet supported. >> + */ >> + if ( pdev->info.is_extfn ) >> + { >> + gdprintk(XENLOG_ERR, "%pp: only function 0 passthrough supported\n", >> + &pdev->sbdf); >> + return -EOPNOTSUPP; >> + } >> + >> + write_lock(&pdev->domain->pci_lock); >> + new_dev_number = find_first_zero_bit(d->vpci_dev_assigned_map, >> + VPCI_MAX_VIRT_DEV); >> + if ( new_dev_number >= VPCI_MAX_VIRT_DEV ) >> + { >> + write_unlock(&pdev->domain->pci_lock); >> + return -ENOSPC; >> + } >> + >> + __set_bit(new_dev_number, &d->vpci_dev_assigned_map); > > ... the same as the one held here, so the bitmap still isn't properly > protected afaics, unless the intention is to continue to rely on > the global PCI lock (assuming that one's held in both cases, which I > didn't check it is). Conversely it looks like the vPCI lock isn't > held here. Both aspects may be intentional, but the locks being > acquired differing requires suitable code comments imo. As I stated above, vpci_remove_device() is called when d->pci_lock is already held. > I've also briefly looked at patch 1, and I'm afraid that still lacks > commentary about intended lock nesting. That might be relevant here > in case locking visible from patch / patch context isn't providing > the full picture. > There is ASSERT(rw_is_write_locked(&pdev->domain->pci_lock)); at the beginning of vpci_remove_device(), which is added by "vpci: use per-domain PCI lock to protect vpci structure". I believe, it will be more beneficial to review series from the beginning. >> + /* >> + * Both segment and bus number are 0: >> + * - we emulate a single host bridge for the guest, e.g. segment 0 >> + * - with bus 0 the virtual devices are seen as embedded >> + * endpoints behind the root complex >> + * >> + * TODO: add support for multi-function devices. >> + */ >> + sbdf.devfn = PCI_DEVFN(new_dev_number, 0); >> + pdev->vpci->guest_sbdf = sbdf; >> + write_unlock(&pdev->domain->pci_lock); > > With the above I also wonder whether this lock can't (and hence > should) be dropped a little earlier (right after fiddling with the > bitmap). This is the good observation, thanks.
On Thu, Jul 20, 2023 at 12:32:33AM +0000, Volodymyr Babchuk wrote: > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h > index 80dd150bbf..478bd21f3e 100644 > --- a/xen/include/xen/sched.h > +++ b/xen/include/xen/sched.h > @@ -461,6 +461,14 @@ struct domain > #ifdef CONFIG_HAS_PCI > struct list_head pdev_list; > rwlock_t pci_lock; > +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT > + /* > + * The bitmap which shows which device numbers are already used by the > + * virtual PCI bus topology and is used to assign a unique SBDF to the > + * next passed through virtual PCI device. > + */ > + DECLARE_BITMAP(vpci_dev_assigned_map, VPCI_MAX_VIRT_DEV); > +#endif I think it would be helpful to state that vpci_dev_assigned_map is protected by pci_lock (as I understand it's the intention). Thanks, Roger.
On Thu, Jul 20, 2023 at 12:32:33AM +0000, Volodymyr Babchuk wrote: > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > Assign SBDF to the PCI devices being passed through with bus 0. > The resulting topology is where PCIe devices reside on the bus 0 of the > root complex itself (embedded endpoints). > This implementation is limited to 32 devices which are allowed on > a single PCI bus. I do wonder how this will work with ioreqs, iow: shouldn't it be the toolstack that selects the virtual slot of the PCI device (in the guest bus). Otherwise I see a hard time reconciling how ioreqs and vPCI can work together if vPCI has it's own (private) view of the bus, and thinks it has exclusive ownership of it. It might be something to deal afterwards, but would likely need a TODO tag in order to realize it needs to be improved. Thanks, Roger.
On 7/19/23 20:32, Volodymyr Babchuk wrote: > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > Assign SBDF to the PCI devices being passed through with bus 0. > The resulting topology is where PCIe devices reside on the bus 0 of the > root complex itself (embedded endpoints). > This implementation is limited to 32 devices which are allowed on > a single PCI bus. > > Please note, that at the moment only function 0 of a multifunction > device can be passed through. > > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > --- > Since v8: > - Added write lock in add_virtual_device > Since v6: > - re-work wrt new locking scheme > - OT: add ASSERT(pcidevs_write_locked()); to add_virtual_device() > Since v5: > - s/vpci_add_virtual_device/add_virtual_device and make it static > - call add_virtual_device from vpci_assign_device and do not use > REGISTER_VPCI_INIT machinery > - add pcidevs_locked ASSERT > - use DECLARE_BITMAP for vpci_dev_assigned_map > Since v4: > - moved and re-worked guest sbdf initializers > - s/set_bit/__set_bit > - s/clear_bit/__clear_bit > - minor comment fix s/Virtual/Guest/ > - added VPCI_MAX_VIRT_DEV constant (PCI_SLOT(~0) + 1) which will be used > later for counting the number of MMIO handlers required for a guest > (Julien) > Since v3: > - make use of VPCI_INIT > - moved all new code to vpci.c which belongs to it > - changed open-coded 31 to PCI_SLOT(~0) > - added comments and code to reject multifunction devices with > functions other than 0 > - updated comment about vpci_dev_next and made it unsigned int > - implement roll back in case of error while assigning/deassigning devices > - s/dom%pd/%pd > Since v2: > - remove casts that are (a) malformed and (b) unnecessary > - add new line for better readability > - remove CONFIG_HAS_VPCI_GUEST_SUPPORT ifdef's as the relevant vPCI > functions are now completely gated with this config > - gate common code with CONFIG_HAS_VPCI_GUEST_SUPPORT > New in v2 > --- > xen/drivers/vpci/vpci.c | 72 ++++++++++++++++++++++++++++++++++++++++- > xen/include/xen/sched.h | 8 +++++ > xen/include/xen/vpci.h | 11 +++++++ > 3 files changed, 90 insertions(+), 1 deletion(-) > > diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c > index ca3505ecb7..baaafe4a2a 100644 > --- a/xen/drivers/vpci/vpci.c > +++ b/xen/drivers/vpci/vpci.c > @@ -46,6 +46,16 @@ void vpci_remove_device(struct pci_dev *pdev) > return; > > spin_lock(&pdev->vpci->lock); > + > +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT > + if ( pdev->vpci->guest_sbdf.sbdf != ~0 ) > + { > + __clear_bit(pdev->vpci->guest_sbdf.dev, > + &pdev->domain->vpci_dev_assigned_map); > + pdev->vpci->guest_sbdf.sbdf = ~0; > + } > +#endif > + > while ( !list_empty(&pdev->vpci->handlers) ) > { > struct vpci_register *r = list_first_entry(&pdev->vpci->handlers, > @@ -101,6 +111,10 @@ int vpci_add_handlers(struct pci_dev *pdev) > INIT_LIST_HEAD(&pdev->vpci->handlers); > spin_lock_init(&pdev->vpci->lock); > > +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT > + pdev->vpci->guest_sbdf.sbdf = ~0; > +#endif > + > for ( i = 0; i < NUM_VPCI_INIT; i++ ) > { > rc = __start_vpci_array[i](pdev); > @@ -115,6 +129,54 @@ int vpci_add_handlers(struct pci_dev *pdev) > } > > #ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT > +static int add_virtual_device(struct pci_dev *pdev) > +{ > + struct domain *d = pdev->domain; > + pci_sbdf_t sbdf = { 0 }; > + unsigned long new_dev_number; > + > + if ( is_hardware_domain(d) ) > + return 0; > + > + ASSERT(pcidevs_locked()); > + > + /* > + * Each PCI bus supports 32 devices/slots at max or up to 256 when > + * there are multi-function ones which are not yet supported. > + */ > + if ( pdev->info.is_extfn ) > + { > + gdprintk(XENLOG_ERR, "%pp: only function 0 passthrough supported\n", > + &pdev->sbdf); > + return -EOPNOTSUPP; > + } > + > + write_lock(&pdev->domain->pci_lock); This should be replaced with an ASSERT, same as the one in vpci_add_handlers() above. The lock is already acquired a few patches before this in the caller in drivers/passthrough/pci.c:assign_device() 1524 write_lock(&pdev->domain->pci_lock); 1525 rc = vpci_assign_device(pdev); 1526 write_unlock(&pdev->domain->pci_lock); > + new_dev_number = find_first_zero_bit(d->vpci_dev_assigned_map, > + VPCI_MAX_VIRT_DEV); > + if ( new_dev_number >= VPCI_MAX_VIRT_DEV ) > + { > + write_unlock(&pdev->domain->pci_lock); > + return -ENOSPC; > + } > + > + __set_bit(new_dev_number, &d->vpci_dev_assigned_map); > + > + /* > + * Both segment and bus number are 0: > + * - we emulate a single host bridge for the guest, e.g. segment 0 > + * - with bus 0 the virtual devices are seen as embedded > + * endpoints behind the root complex > + * > + * TODO: add support for multi-function devices. > + */ > + sbdf.devfn = PCI_DEVFN(new_dev_number, 0); > + pdev->vpci->guest_sbdf = sbdf; > + write_unlock(&pdev->domain->pci_lock); > + > + return 0; > +} > + > /* Notify vPCI that device is assigned to guest. */ > int vpci_assign_device(struct pci_dev *pdev) > { > @@ -125,8 +187,16 @@ int vpci_assign_device(struct pci_dev *pdev) > > rc = vpci_add_handlers(pdev); > if ( rc ) > - vpci_deassign_device(pdev); > + goto fail; > + > + rc = add_virtual_device(pdev); > + if ( rc ) > + goto fail; > + > + return 0; > > + fail: > + vpci_deassign_device(pdev); > return rc; > } > #endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */ > diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h > index 80dd150bbf..478bd21f3e 100644 > --- a/xen/include/xen/sched.h > +++ b/xen/include/xen/sched.h > @@ -461,6 +461,14 @@ struct domain > #ifdef CONFIG_HAS_PCI > struct list_head pdev_list; > rwlock_t pci_lock; > +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT > + /* > + * The bitmap which shows which device numbers are already used by the > + * virtual PCI bus topology and is used to assign a unique SBDF to the > + * next passed through virtual PCI device. > + */ > + DECLARE_BITMAP(vpci_dev_assigned_map, VPCI_MAX_VIRT_DEV); > +#endif > #endif > > #ifdef CONFIG_HAS_PASSTHROUGH > diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h > index 6099d2141d..c55c45f7a1 100644 > --- a/xen/include/xen/vpci.h > +++ b/xen/include/xen/vpci.h > @@ -21,6 +21,13 @@ typedef int vpci_register_init_t(struct pci_dev *dev); > > #define VPCI_ECAM_BDF(addr) (((addr) & 0x0ffff000) >> 12) > > +/* > + * Maximum number of devices supported by the virtual bus topology: > + * each PCI bus supports 32 devices/slots at max or up to 256 when > + * there are multi-function ones which are not yet supported. > + */ > +#define VPCI_MAX_VIRT_DEV (PCI_SLOT(~0) + 1) > + > #define REGISTER_VPCI_INIT(x, p) \ > static vpci_register_init_t *const x##_entry \ > __used_section(".data.vpci." p) = x > @@ -155,6 +162,10 @@ struct vpci { > struct vpci_arch_msix_entry arch; > } entries[]; > } *msix; > +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT > + /* Guest SBDF of the device. */ > + pci_sbdf_t guest_sbdf; > +#endif > #endif > }; > > -- > 2.41.0 >
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c index ca3505ecb7..baaafe4a2a 100644 --- a/xen/drivers/vpci/vpci.c +++ b/xen/drivers/vpci/vpci.c @@ -46,6 +46,16 @@ void vpci_remove_device(struct pci_dev *pdev) return; spin_lock(&pdev->vpci->lock); + +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT + if ( pdev->vpci->guest_sbdf.sbdf != ~0 ) + { + __clear_bit(pdev->vpci->guest_sbdf.dev, + &pdev->domain->vpci_dev_assigned_map); + pdev->vpci->guest_sbdf.sbdf = ~0; + } +#endif + while ( !list_empty(&pdev->vpci->handlers) ) { struct vpci_register *r = list_first_entry(&pdev->vpci->handlers, @@ -101,6 +111,10 @@ int vpci_add_handlers(struct pci_dev *pdev) INIT_LIST_HEAD(&pdev->vpci->handlers); spin_lock_init(&pdev->vpci->lock); +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT + pdev->vpci->guest_sbdf.sbdf = ~0; +#endif + for ( i = 0; i < NUM_VPCI_INIT; i++ ) { rc = __start_vpci_array[i](pdev); @@ -115,6 +129,54 @@ int vpci_add_handlers(struct pci_dev *pdev) } #ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT +static int add_virtual_device(struct pci_dev *pdev) +{ + struct domain *d = pdev->domain; + pci_sbdf_t sbdf = { 0 }; + unsigned long new_dev_number; + + if ( is_hardware_domain(d) ) + return 0; + + ASSERT(pcidevs_locked()); + + /* + * Each PCI bus supports 32 devices/slots at max or up to 256 when + * there are multi-function ones which are not yet supported. + */ + if ( pdev->info.is_extfn ) + { + gdprintk(XENLOG_ERR, "%pp: only function 0 passthrough supported\n", + &pdev->sbdf); + return -EOPNOTSUPP; + } + + write_lock(&pdev->domain->pci_lock); + new_dev_number = find_first_zero_bit(d->vpci_dev_assigned_map, + VPCI_MAX_VIRT_DEV); + if ( new_dev_number >= VPCI_MAX_VIRT_DEV ) + { + write_unlock(&pdev->domain->pci_lock); + return -ENOSPC; + } + + __set_bit(new_dev_number, &d->vpci_dev_assigned_map); + + /* + * Both segment and bus number are 0: + * - we emulate a single host bridge for the guest, e.g. segment 0 + * - with bus 0 the virtual devices are seen as embedded + * endpoints behind the root complex + * + * TODO: add support for multi-function devices. + */ + sbdf.devfn = PCI_DEVFN(new_dev_number, 0); + pdev->vpci->guest_sbdf = sbdf; + write_unlock(&pdev->domain->pci_lock); + + return 0; +} + /* Notify vPCI that device is assigned to guest. */ int vpci_assign_device(struct pci_dev *pdev) { @@ -125,8 +187,16 @@ int vpci_assign_device(struct pci_dev *pdev) rc = vpci_add_handlers(pdev); if ( rc ) - vpci_deassign_device(pdev); + goto fail; + + rc = add_virtual_device(pdev); + if ( rc ) + goto fail; + + return 0; + fail: + vpci_deassign_device(pdev); return rc; } #endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */ diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index 80dd150bbf..478bd21f3e 100644 --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -461,6 +461,14 @@ struct domain #ifdef CONFIG_HAS_PCI struct list_head pdev_list; rwlock_t pci_lock; +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT + /* + * The bitmap which shows which device numbers are already used by the + * virtual PCI bus topology and is used to assign a unique SBDF to the + * next passed through virtual PCI device. + */ + DECLARE_BITMAP(vpci_dev_assigned_map, VPCI_MAX_VIRT_DEV); +#endif #endif #ifdef CONFIG_HAS_PASSTHROUGH diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h index 6099d2141d..c55c45f7a1 100644 --- a/xen/include/xen/vpci.h +++ b/xen/include/xen/vpci.h @@ -21,6 +21,13 @@ typedef int vpci_register_init_t(struct pci_dev *dev); #define VPCI_ECAM_BDF(addr) (((addr) & 0x0ffff000) >> 12) +/* + * Maximum number of devices supported by the virtual bus topology: + * each PCI bus supports 32 devices/slots at max or up to 256 when + * there are multi-function ones which are not yet supported. + */ +#define VPCI_MAX_VIRT_DEV (PCI_SLOT(~0) + 1) + #define REGISTER_VPCI_INIT(x, p) \ static vpci_register_init_t *const x##_entry \ __used_section(".data.vpci." p) = x @@ -155,6 +162,10 @@ struct vpci { struct vpci_arch_msix_entry arch; } entries[]; } *msix; +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT + /* Guest SBDF of the device. */ + pci_sbdf_t guest_sbdf; +#endif #endif };