On Tue, Aug 29, 2023 at 11:19:43PM +0000, Volodymyr Babchuk wrote:
> From: Oleksandr Andrushchenko <[email protected]>
>
> When a PCI device gets assigned/de-assigned we need to
> initialize/de-initialize vPCI state for the device.
>
> Also, rename vpci_add_handlers() to vpci_assign_device() and
> vpci_remove_device() to vpci_deassign_device() to better reflect role
> of the functions.
>
> Signed-off-by: Oleksandr Andrushchenko <[email protected]>
> Signed-off-by: Volodymyr Babchuk <[email protected]>
> ---
> Since v9:
> - removed previous vpci_[de]assign_device function and renamed
> existing handlers
> - dropped attempts to handle errors in assign_device() function
> - do not call vpci_assign_device for dom_io
> - use d instead of pdev->domain
> - use IS_ENABLED macro
> Since v8:
> - removed vpci_deassign_device
> Since v6:
> - do not pass struct domain to vpci_{assign|deassign}_device as
> pdev->domain can be used
> - do not leave the device assigned (pdev->domain == new domain) in case
> vpci_assign_device fails: try to de-assign and if this also fails, then
> crash the domain
> Since v5:
> - do not split code into run_vpci_init
> - do not check for is_system_domain in vpci_{de}assign_device
> - do not use vpci_remove_device_handlers_locked and re-allocate
> pdev->vpci completely
> - make vpci_deassign_device void
> 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 | 31 +++++++++++++++++++++++++++----
> xen/drivers/vpci/header.c | 2 +-
> xen/drivers/vpci/vpci.c | 6 +++---
> xen/include/xen/vpci.h | 10 +++++-----
> 5 files changed, 40 insertions(+), 13 deletions(-)
>
> diff --git a/xen/drivers/Kconfig b/xen/drivers/Kconfig
> index db94393f47..780490cf8e 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 4f18293900..64281f2d5e 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -757,7 +757,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
> * For devices not discovered by Xen during boot, add vPCI handlers
> * when Dom0 first informs Xen about such devices.
> */
> - ret = vpci_add_handlers(pdev);
> + ret = vpci_assign_device(pdev);
> if ( ret )
> {
> printk(XENLOG_ERR "Setup of vPCI failed: %d\n", ret);
> @@ -771,7 +771,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
> if ( ret )
> {
> write_lock(&hardware_domain->pci_lock);
> - vpci_remove_device(pdev);
> + vpci_deassign_device(pdev);
> list_del(&pdev->domain_list);
> write_unlock(&hardware_domain->pci_lock);
> pdev->domain = NULL;
> @@ -819,7 +819,7 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
> list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
> if ( pdev->bus == bus && pdev->devfn == devfn )
> {
> - vpci_remove_device(pdev);
> + vpci_deassign_device(pdev);
> pci_cleanup_msi(pdev);
> ret = iommu_remove_device(pdev);
> if ( pdev->domain )
> @@ -877,6 +877,13 @@ static int deassign_device(struct domain *d, uint16_t
> seg, uint8_t bus,
> goto out;
> }
>
> + if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) )
> + {
> + write_lock(&d->pci_lock);
> + vpci_deassign_device(pdev);
> + write_unlock(&d->pci_lock);
> + }
I'm confused by this one, shouldn't the code rely on has_vpci()
instead? (which is already checked for in vpci_deassign_device()).
If you have a system without CONFIG_HAS_VPCI_GUEST_SUPPORT but vPCI is
used by dom0 you likely still need the hooks in {,de}assign_device()
so that vPCI status is properly handled for dom0 as the devices get
deassigned to dom0 and assigned to a guest? (and maybe moved back to
dom0 at a later point).
> +
> devfn = pdev->devfn;
> ret = iommu_call(hd->platform_ops, reassign_device, d, target, devfn,
> pci_to_dev(pdev));
> @@ -1147,7 +1154,7 @@ static void __hwdom_init setup_one_hwdom_device(const
> struct setup_hwdom *ctxt,
> PCI_SLOT(devfn) == PCI_SLOT(pdev->devfn) );
>
> write_lock(&ctxt->d->pci_lock);
> - err = vpci_add_handlers(pdev);
> + err = vpci_assign_device(pdev);
> write_unlock(&ctxt->d->pci_lock);
> if ( err )
> printk(XENLOG_ERR "setup of vPCI for d%d failed: %d\n",
> @@ -1481,6 +1488,13 @@ static int assign_device(struct domain *d, u16 seg, u8
> bus, u8 devfn, u32 flag)
> if ( pdev->broken && d != hardware_domain && d != dom_io )
> goto done;
>
> + if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) )
> + {
> + write_lock(&pdev->domain->pci_lock);
> + vpci_deassign_device(pdev);
> + write_unlock(&pdev->domain->pci_lock);
> + }
> +
> rc = pdev_msix_assign(d, pdev);
> if ( rc )
> goto done;
> @@ -1506,6 +1520,15 @@ static int assign_device(struct domain *d, u16 seg, u8
> bus, u8 devfn, u32 flag)
> rc = iommu_call(hd->platform_ops, assign_device, d, devfn,
> pci_to_dev(pdev), flag);
> }
> + if ( rc )
> + goto done;
rc can't be != 0 here, as the increment statement in the for loop
above will zero rc at each iteration.
> +
> + if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) && d != dom_io)
> + {
> + write_lock(&d->pci_lock);
> + rc = vpci_assign_device(pdev);
> + write_unlock(&d->pci_lock);
> + }
Why do you need the extra d != dom_io check here? has_vpci() will
fail for dom_io, no need to special case it here.
Thanks, Roger.
