| Message ID | 20211105065629.940943-5-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI devices passthrough on Arm, part 3 | expand |
On 05.11.2021 07:56, Oleksandr Andrushchenko wrote: > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > When a PCI device gets assigned/de-assigned some work on vPCI side needs > to be done for that device. Introduce a pair of hooks so vPCI can handle > that. > > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > --- > 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 | 6 ++++ > xen/drivers/vpci/vpci.c | 57 ++++++++++++++++++++++++++++++----- > xen/include/xen/vpci.h | 16 ++++++++++ > 4 files changed, 75 insertions(+), 8 deletions(-) > > diff --git a/xen/drivers/Kconfig b/xen/drivers/Kconfig > index db94393f47a6..780490cf8e39 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 a9d31293ac09..529a4f50aa80 100644 > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -873,6 +873,10 @@ static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus, > if ( ret ) > goto out; > > + ret = vpci_deassign_device(d, pdev); > + if ( ret ) > + goto out; > + > if ( pdev->domain == hardware_domain ) > pdev->quarantine = false; > > @@ -1445,6 +1449,8 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag) > rc = hd->platform_ops->assign_device(d, devfn, pci_to_dev(pdev), flag); > } > > + rc = vpci_assign_device(d, pdev); > + > done: > if ( rc ) > printk(XENLOG_G_WARNING "%pd: assign (%pp) failed (%d)\n", Don't you need to call vpci_deassign_device() higher up in this function for the prior owner of the device? > +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT > +/* Notify vPCI that device is assigned to guest. */ > +int vpci_assign_device(struct domain *d, struct pci_dev *pdev) > +{ > + int rc; > + > + /* It only makes sense to assign for hwdom or guest domain. */ > + if ( is_system_domain(d) || !has_vpci(d) ) > + return 0; > + > + vpci_remove_device_handlers(pdev); This removes handlers in d, not in the prior owner domain. Is this really intended? And if it really is meant to remove the new domain's handlers (of which there ought to be none) - why is this necessary? Jan
On 15.11.21 19:06, Jan Beulich wrote: > On 05.11.2021 07:56, Oleksandr Andrushchenko wrote: >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> >> When a PCI device gets assigned/de-assigned some work on vPCI side needs >> to be done for that device. Introduce a pair of hooks so vPCI can handle >> that. >> >> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> --- >> 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 | 6 ++++ >> xen/drivers/vpci/vpci.c | 57 ++++++++++++++++++++++++++++++----- >> xen/include/xen/vpci.h | 16 ++++++++++ >> 4 files changed, 75 insertions(+), 8 deletions(-) >> >> diff --git a/xen/drivers/Kconfig b/xen/drivers/Kconfig >> index db94393f47a6..780490cf8e39 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 a9d31293ac09..529a4f50aa80 100644 >> --- a/xen/drivers/passthrough/pci.c >> +++ b/xen/drivers/passthrough/pci.c >> @@ -873,6 +873,10 @@ static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus, >> if ( ret ) >> goto out; >> >> + ret = vpci_deassign_device(d, pdev); >> + if ( ret ) >> + goto out; >> + >> if ( pdev->domain == hardware_domain ) >> pdev->quarantine = false; >> >> @@ -1445,6 +1449,8 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag) >> rc = hd->platform_ops->assign_device(d, devfn, pci_to_dev(pdev), flag); >> } >> >> + rc = vpci_assign_device(d, pdev); >> + >> done: >> if ( rc ) >> printk(XENLOG_G_WARNING "%pd: assign (%pp) failed (%d)\n", > Don't you need to call vpci_deassign_device() higher up in this > function for the prior owner of the device? Yes, this does make more sense, e.g. vpci_deassign_device(pdev->domain, pdev) before assigning pdev to an IOMMU which updates pdev->domain and then... > >> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT >> +/* Notify vPCI that device is assigned to guest. */ >> +int vpci_assign_device(struct domain *d, struct pci_dev *pdev) >> +{ >> + int rc; >> + >> + /* It only makes sense to assign for hwdom or guest domain. */ >> + if ( is_system_domain(d) || !has_vpci(d) ) >> + return 0; >> + >> + vpci_remove_device_handlers(pdev); > This removes handlers in d, not in the prior owner domain. Is this > really intended? And if it really is meant to remove the new domain's > handlers (of which there ought to be none) - why is this necessary? the above won't be needed > > Jan > Thank you, Oleksandr
diff --git a/xen/drivers/Kconfig b/xen/drivers/Kconfig index db94393f47a6..780490cf8e39 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 a9d31293ac09..529a4f50aa80 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -873,6 +873,10 @@ static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus, if ( ret ) goto out; + ret = vpci_deassign_device(d, pdev); + if ( ret ) + goto out; + if ( pdev->domain == hardware_domain ) pdev->quarantine = false; @@ -1445,6 +1449,8 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag) rc = hd->platform_ops->assign_device(d, devfn, pci_to_dev(pdev), flag); } + rc = vpci_assign_device(d, pdev); + done: if ( rc ) printk(XENLOG_G_WARNING "%pd: assign (%pp) failed (%d)\n", diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c index d7f033a0811f..5f086398a98c 100644 --- a/xen/drivers/vpci/vpci.c +++ b/xen/drivers/vpci/vpci.c @@ -63,11 +63,25 @@ void vpci_remove_device(struct pci_dev *pdev) pdev->vpci = NULL; } -int vpci_add_handlers(struct pci_dev *pdev) +static int run_vpci_init(struct pci_dev *pdev) { unsigned int i; int rc = 0; + for ( i = 0; i < NUM_VPCI_INIT; i++ ) + { + rc = __start_vpci_array[i](pdev); + if ( rc ) + break; + } + + return rc; +} + +int vpci_add_handlers(struct pci_dev *pdev) +{ + int rc; + if ( !has_vpci(pdev->domain) ) return 0; @@ -81,18 +95,45 @@ int vpci_add_handlers(struct pci_dev *pdev) INIT_LIST_HEAD(&pdev->vpci->handlers); spin_lock_init(&pdev->vpci->lock); - for ( i = 0; i < NUM_VPCI_INIT; i++ ) - { - rc = __start_vpci_array[i](pdev); - if ( rc ) - break; - } - + rc = run_vpci_init(pdev); if ( rc ) vpci_remove_device(pdev); return rc; } + +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT +/* Notify vPCI that device is assigned to guest. */ +int vpci_assign_device(struct domain *d, struct pci_dev *pdev) +{ + int rc; + + /* It only makes sense to assign for hwdom or guest domain. */ + if ( is_system_domain(d) || !has_vpci(d) ) + return 0; + + vpci_remove_device_handlers(pdev); + + rc = run_vpci_init(pdev); + if ( rc ) + vpci_deassign_device(d, pdev); + + return rc; +} + +/* Notify vPCI that device is de-assigned from guest. */ +int vpci_deassign_device(struct domain *d, struct pci_dev *pdev) +{ + /* It only makes sense to de-assign from hwdom or guest domain. */ + if ( is_system_domain(d) || !has_vpci(d) ) + return 0; + + vpci_remove_device_handlers(pdev); + + return 0; +} +#endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */ + #endif /* __XEN__ */ static int vpci_register_cmp(const struct vpci_register *r1, diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h index 1883b9d08a70..a016b4197801 100644 --- a/xen/include/xen/vpci.h +++ b/xen/include/xen/vpci.h @@ -262,6 +262,22 @@ static inline void vpci_cancel_pending(const struct pci_dev *pdev) } #endif +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT +/* Notify vPCI that device is assigned/de-assigned to/from guest. */ +int vpci_assign_device(struct domain *d, struct pci_dev *pdev); +int vpci_deassign_device(struct domain *d, struct pci_dev *pdev); +#else +static inline int vpci_assign_device(struct domain *d, struct pci_dev *pdev) +{ + return 0; +}; + +static inline int vpci_deassign_device(struct domain *d, struct pci_dev *pdev) +{ + return 0; +}; +#endif + #endif /*