[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v5 05/14] vpci: add hooks for PCI device assign/de-assign
On Thu, Nov 25, 2021 at 01:02:42PM +0200, Oleksandr Andrushchenko wrote: > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx> > > 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@xxxxxxxx> > --- > 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 | 10 ++++++ > xen/drivers/vpci/vpci.c | 61 +++++++++++++++++++++++++++++------ > xen/include/xen/vpci.h | 16 +++++++++ > 4 files changed, 82 insertions(+), 9 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 286808b25e65..d9ef91571adf 100644 > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -874,6 +874,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; Following my comment below, this won't be allowed to fail. > + > if ( pdev->domain == hardware_domain ) > pdev->quarantine = false; > > @@ -1429,6 +1433,10 @@ static int assign_device(struct domain *d, u16 seg, u8 > bus, u8 devfn, u32 flag) > ASSERT(pdev && (pdev->domain == hardware_domain || > pdev->domain == dom_io)); > > + rc = vpci_deassign_device(pdev->domain, pdev); > + if ( rc ) > + goto done; > + > rc = pdev_msix_assign(d, pdev); > if ( rc ) > goto done; > @@ -1446,6 +1454,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 37103e207635..a9e9e8ec438c 100644 > --- a/xen/drivers/vpci/vpci.c > +++ b/xen/drivers/vpci/vpci.c > @@ -74,12 +74,26 @@ void vpci_remove_device(struct pci_dev *pdev) > spin_unlock(&pdev->vpci_lock); > } > > -int vpci_add_handlers(struct pci_dev *pdev) > +static int run_vpci_init(struct pci_dev *pdev) Just using add_handlers as function name would be clearer IMO. > { > - struct vpci *vpci; > 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) > +{ > + struct vpci *vpci; > + int rc; > + > if ( !has_vpci(pdev->domain) ) > return 0; > > @@ -94,19 +108,48 @@ int vpci_add_handlers(struct pci_dev *pdev) > pdev->vpci = vpci; > INIT_LIST_HEAD(&pdev->vpci->handlers); > > - 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_locked(pdev); > spin_unlock(&pdev->vpci_lock); > > 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) ) Do you really need the is_system_domain check? System domains shouldn't have the VPCI flag set anyway, so should fail the has_vpci test. > + return 0; > + > + spin_lock(&pdev->vpci_lock); > + rc = run_vpci_init(pdev); > + spin_unlock(&pdev->vpci_lock); > + 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) There's no need to return any value from this function AFAICT. It should have void return type. Thanks, Roger.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |