Re: [PATCH v3 02/11] vpci: Add hooks for PCI device assign/de-assign

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

On Thu, Sep 30, 2021 at 10:52:14AM +0300, 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.
> 
> Please note, that in the current design the error path is handled by
> the toolstack via XEN_DOMCTL_assign_device/XEN_DOMCTL_deassign_device,
> so this is why it is acceptable not to de-assign devices if vPCI's
> assign fails, e.g. the roll back will be handled on deassign_device when
> it is called by the toolstack.

It's kind of hard to see what would need to be rolled back, as the
functions are just dummies right now that don't perform any actions.

I don't think the toolstack should be the one to deal with the
fallout, as it could leave Xen in a broken state. The current commit
message doesn't provide any information about why it has been designed
this way.

> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> ---
> 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 |  9 +++++++++
>  xen/drivers/vpci/vpci.c       | 23 +++++++++++++++++++++++
>  xen/include/xen/vpci.h        | 20 ++++++++++++++++++++
>  4 files changed, 56 insertions(+)
> 
> 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

I would assume this is to go away once the work is finished? I don't
think it makes sense to split vPCI code between domU/dom0 on a build
time basis.

> +
>  endmenu
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 9f804a50e780..805ab86ed555 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -870,6 +870,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;
> +
>      if ( pdev->domain == hardware_domain  )
>          pdev->quarantine = false;
>  
> @@ -1429,6 +1433,11 @@ 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);
>      }
>  
> +    if ( rc )
> +        goto done;
> +
> +    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 1666402d55b8..0fe86cb30d23 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -86,6 +86,29 @@ int __hwdom_init 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 domain *d, const struct pci_dev *dev)
> +{
> +    /* It only makes sense to assign for hwdom or guest domain. */
> +    if ( is_system_domain(d) || !has_vpci(d) )
> +        return 0;
> +
> +    return 0;
> +}
> +
> +/* Notify vPCI that device is de-assigned from guest. */
> +int vpci_deassign_device(struct domain *d, const struct pci_dev *dev)
> +{
> +    /* It only makes sense to de-assign from hwdom or guest domain. */
> +    if ( is_system_domain(d) || !has_vpci(d) )
> +        return 0;
> +
> +    return 0;
> +}
> +#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 2e910d0b1f90..ecc08f2c0f65 100644
> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -242,6 +242,26 @@ static inline bool vpci_process_pending(struct vcpu *v)
>  }
>  #endif
>  
> +#if defined(CONFIG_HAS_VPCI) && defined(CONFIG_HAS_VPCI_GUEST_SUPPORT)

You don't need to check for CONFIG_HAS_VPCI, as
CONFIG_HAS_VPCI_GUEST_SUPPORT already depends on CONFIG_HAS_VPCI being
set.

> +/* Notify vPCI that device is assigned/de-assigned to/from guest. */
> +int __must_check vpci_assign_device(struct domain *d,
> +                                    const struct pci_dev *dev);
> +int __must_check vpci_deassign_device(struct domain *d,
> +                                      const struct pci_dev *dev);
> +#else
> +static inline int vpci_assign_device(struct domain *d,
> +                                     const struct pci_dev *dev)
> +{
> +    return 0;
> +};
> +
> +static inline int vpci_deassign_device(struct domain *d,
> +                                       const struct pci_dev *dev)
> +{
> +    return 0;
> +};

You need the __must_check attributes here also to match the prototypes
above.

Thanks, Roger.