Re: [PATCH v8 04/13] vpci: add hooks for PCI device assign/de-assign

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Jul 20, 2023 at 12:32:31AM +0000, Volodymyr Babchuk 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 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 | 21 +++++++++++++++++++++
>  xen/drivers/vpci/vpci.c       | 18 ++++++++++++++++++
>  xen/include/xen/vpci.h        | 15 +++++++++++++++
>  4 files changed, 58 insertions(+)
> 
> 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 6f8692cd9c..265d359704 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -885,6 +885,10 @@ static int deassign_device(struct domain *d, uint16_t 
> seg, uint8_t bus,
>      if ( ret )
>          goto out;
>  
> +    write_lock(&pdev->domain->pci_lock);
> +    vpci_deassign_device(pdev);
> +    write_unlock(&pdev->domain->pci_lock);
> +
>      if ( pdev->domain == hardware_domain  )
>          pdev->quarantine = false;
>  
> @@ -1484,6 +1488,10 @@ 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;
>  
> +    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;
> @@ -1509,6 +1517,19 @@ 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;
> +
> +    devfn = pdev->devfn;
> +    write_lock(&pdev->domain->pci_lock);
> +    rc = vpci_assign_device(pdev);
> +    write_unlock(&pdev->domain->pci_lock);
> +    if ( rc && deassign_device(d, seg, bus, devfn) )
> +    {
> +        printk(XENLOG_ERR "%pd: %pp was left partially assigned\n",
> +               d, &PCI_SBDF(seg, bus, devfn));

&pdev->sbdf?  Then you can get of the devfn usage above.

> +        domain_crash(d);

This seems like a bit different from the other error paths in the
function, isn't it fine to return an error and let the caller handle
the deassign?

Also, if we really need to call deassign_device() we must do so for
all possible phantom devices, see the above loop around
iommu_call(..., assing_device, ...);

> +    }
>  
>   done:
>      if ( rc )
> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> index a6d2cf8660..a97710a806 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -107,6 +107,24 @@ int vpci_add_handlers(struct pci_dev *pdev)
>  
>      return rc;
>  }
> +
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> +/* Notify vPCI that device is assigned to guest. */
> +int vpci_assign_device(struct pci_dev *pdev)
> +{
> +    int rc;
> +
> +    if ( !has_vpci(pdev->domain) )
> +        return 0;
> +
> +    rc = vpci_add_handlers(pdev);
> +    if ( rc )
> +        vpci_deassign_device(pdev);

Why do you need this handler, vpci_add_handlers() when failing will
already call vpci_remove_device(), which is what
vpci_deassign_device() does.

> +
> +    return rc;
> +}
> +#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 0b8a2a3c74..44296623e1 100644
> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -264,6 +264,21 @@ static inline bool __must_check 
> vpci_process_pending(struct vcpu *v)
>  }
>  #endif
>  
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> +/* Notify vPCI that device is assigned/de-assigned to/from guest. */
> +int vpci_assign_device(struct pci_dev *pdev);
> +#define vpci_deassign_device vpci_remove_device
> +#else
> +static inline int vpci_assign_device(struct pci_dev *pdev)
> +{
> +    return 0;
> +};
> +
> +static inline void vpci_deassign_device(struct pci_dev *pdev)
> +{
> +};
> +#endif

I don't think there's much point in introducing new functions, see
above.  I'm fine if the current ones want to be renamed to
vpci_{,de}assign_device(), but adding defines like the above just
makes the code harder to follow.

Thanks, Roger.