On Thu, Nov 25, 2021 at 01:02:42PM +0200, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <[email protected]>
>
> 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 <[email protected]>
> ---
> 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.
