[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
Hi, Roger! On 12.01.22 14:12, Roger Pau Monné wrote: > 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. Ok, will change > >> { >> - 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. No, it seems we do not need this check: will remove > >> + 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. Makes sense, I will s/int/void > Thanks, Roger. Thank you, Oleksandr
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |