| Message ID | 20230613103159.524763-11-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI devices passthrough on Arm, part 3 | expand |
On 13.06.2023 12:32, Volodymyr Babchuk wrote: > @@ -121,6 +124,62 @@ 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; > + } > + > + new_dev_number = find_first_zero_bit(d->vpci_dev_assigned_map, > + VPCI_MAX_VIRT_DEV); > + if ( new_dev_number >= VPCI_MAX_VIRT_DEV ) > + return -ENOSPC; > + > + __set_bit(new_dev_number, &d->vpci_dev_assigned_map); Since the find-and-set can't easily be atomic, the lock used here ( asserted to be held above) needs to be the same as ... > + /* > + * 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; > + > + return 0; > + > +} > + > +static void vpci_remove_virtual_device(const struct pci_dev *pdev) > +{ > + write_lock(&pdev->domain->vpci_rwlock); > + if ( pdev->vpci ) > + { > + __clear_bit(pdev->vpci->guest_sbdf.dev, > + &pdev->domain->vpci_dev_assigned_map); > + pdev->vpci->guest_sbdf.sbdf = ~0; > + } > + write_unlock(&pdev->domain->vpci_rwlock); ... the one used here. Jan
On 21.06.23 15:06, Jan Beulich wrote: Hello all > On 13.06.2023 12:32, Volodymyr Babchuk wrote: >> @@ -121,6 +124,62 @@ 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; >> + } >> + >> + new_dev_number = find_first_zero_bit(d->vpci_dev_assigned_map, >> + VPCI_MAX_VIRT_DEV); >> + if ( new_dev_number >= VPCI_MAX_VIRT_DEV ) >> + return -ENOSPC; >> + >> + __set_bit(new_dev_number, &d->vpci_dev_assigned_map); > > Since the find-and-set can't easily be atomic, the lock used here ( > asserted to be held above) needs to be the same as ... > >> + /* >> + * 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; >> + >> + return 0; >> + >> +} >> + >> +static void vpci_remove_virtual_device(const struct pci_dev *pdev) >> +{ >> + write_lock(&pdev->domain->vpci_rwlock); >> + if ( pdev->vpci ) >> + { >> + __clear_bit(pdev->vpci->guest_sbdf.dev, >> + &pdev->domain->vpci_dev_assigned_map); >> + pdev->vpci->guest_sbdf.sbdf = ~0; >> + } >> + write_unlock(&pdev->domain->vpci_rwlock); > > ... the one used here. I think, it makes sense, yes. *** There is one more thing. As far as I remember, there were some requests provided for the previous version (also v7) [1]. At least one of them, I assume, is still applicable here. I am speaking about a request to consider moving "cleaning up guest_sbdf / vpci_dev_assigned_map" into vpci_remove_device() here and aliasing of vpci_deassign_device() to vpci_remove_device() in commit #03/12. The diff below (to be applied on top of current patch) is my understanding (not even build tested): diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c index a61282cc5b..c3e6c153bc 100644 --- a/xen/drivers/vpci/vpci.c +++ b/xen/drivers/vpci/vpci.c @@ -51,6 +51,15 @@ void vpci_remove_device(struct pci_dev *pdev) return; } +#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 + vpci = pdev->vpci; pdev->vpci = NULL; write_unlock(&pdev->domain->vpci_rwlock); @@ -152,10 +161,14 @@ static int add_virtual_device(struct pci_dev *pdev) return -EOPNOTSUPP; } + write_lock(&pdev->domain->vpci_rwlock); 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->vpci_rwlock); return -ENOSPC; + } __set_bit(new_dev_number, &d->vpci_dev_assigned_map); @@ -169,23 +182,12 @@ static int add_virtual_device(struct pci_dev *pdev) */ sbdf.devfn = PCI_DEVFN(new_dev_number, 0); pdev->vpci->guest_sbdf = sbdf; + write_unlock(&pdev->domain->vpci_rwlock); return 0; } -static void vpci_remove_virtual_device(const struct pci_dev *pdev) -{ - write_lock(&pdev->domain->vpci_rwlock); - if ( pdev->vpci ) - { - __clear_bit(pdev->vpci->guest_sbdf.dev, - &pdev->domain->vpci_dev_assigned_map); - pdev->vpci->guest_sbdf.sbdf = ~0; - } - write_unlock(&pdev->domain->vpci_rwlock); -} - /* Notify vPCI that device is assigned to guest. */ int vpci_assign_device(struct pci_dev *pdev) { @@ -215,7 +217,6 @@ void vpci_deassign_device(struct pci_dev *pdev) if ( !has_vpci(pdev->domain) ) return; - vpci_remove_virtual_device(pdev); vpci_remove_device(pdev); } #endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */ (END) [1] https://lore.kernel.org/xen-devel/20220719174253.541965-10-olekstysh@gmail.com/ https://lore.kernel.org/xen-devel/20220719174253.541965-3-olekstysh@gmail.com/ > > Jan >
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c index b542ddaf7b..9dacb70bf3 100644 --- a/xen/drivers/vpci/vpci.c +++ b/xen/drivers/vpci/vpci.c @@ -98,6 +98,9 @@ int vpci_add_handlers(struct pci_dev *pdev) INIT_LIST_HEAD(&vpci->handlers); spin_lock_init(&vpci->lock); +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT + vpci->guest_sbdf.sbdf = ~0; +#endif write_lock(&pdev->domain->vpci_rwlock); @@ -121,6 +124,62 @@ 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; + } + + new_dev_number = find_first_zero_bit(d->vpci_dev_assigned_map, + VPCI_MAX_VIRT_DEV); + if ( new_dev_number >= VPCI_MAX_VIRT_DEV ) + 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; + + return 0; + +} + +static void vpci_remove_virtual_device(const struct pci_dev *pdev) +{ + write_lock(&pdev->domain->vpci_rwlock); + if ( pdev->vpci ) + { + __clear_bit(pdev->vpci->guest_sbdf.dev, + &pdev->domain->vpci_dev_assigned_map); + pdev->vpci->guest_sbdf.sbdf = ~0; + } + write_unlock(&pdev->domain->vpci_rwlock); +} + /* Notify vPCI that device is assigned to guest. */ int vpci_assign_device(struct pci_dev *pdev) { @@ -131,8 +190,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; } @@ -142,6 +209,7 @@ void vpci_deassign_device(struct pci_dev *pdev) if ( !has_vpci(pdev->domain) ) return; + vpci_remove_virtual_device(pdev); vpci_remove_device(pdev); } #endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */ diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index 78227b7a1d..312a692b31 100644 --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -463,6 +463,14 @@ struct domain #ifdef CONFIG_HAS_VPCI rwlock_t vpci_rwlock; #endif +#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 fe100a8cb7..3cb5d63e84 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 @@ -160,6 +167,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 };