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

[Jan Beulich <jbeulich@suse.com>]

On 04.02.2022 07:34, Oleksandr Andrushchenko wrote:
> @@ -1507,6 +1511,8 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
>                          pci_to_dev(pdev), flag);
>      }
>  
> +    rc = vpci_assign_device(d, pdev);
> +
>   done:
>      if ( rc )
>          printk(XENLOG_G_WARNING "%pd: assign (%pp) failed (%d)\n",

There's no attempt to undo anything in the case of getting back an
error. ISTR this being deemed okay on the basis that the tool stack
would then take whatever action, but whatever it is that is supposed
to deal with errors here wants spelling out in the description.
What's important is that no caller up the call tree may be left with
the impression that the device is still owned by the original
domain. With how you have it, the device is going to be owned by the
new domain, but not really usable.

> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -99,6 +99,33 @@ 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 domain *d, struct pci_dev *pdev)
> +{
> +    int rc;
> +
> +    if ( !has_vpci(d) )
> +        return 0;
> +
> +    rc = vpci_add_handlers(pdev);
> +    if ( rc )
> +        vpci_deassign_device(d, pdev);
> +
> +    return rc;
> +}
> +
> +/* Notify vPCI that device is de-assigned from guest. */
> +void vpci_deassign_device(struct domain *d, struct pci_dev *pdev)
> +{
> +    if ( !has_vpci(d) )
> +        return;
> +
> +    vpci_remove_device(pdev);
> +}
> +#endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */

While for the latter function you look to need two parameters, do you
really need them also in the former one?

Symmetry considerations make me wonder though whether the de-assign
hook shouldn't be called earlier, when pdev->domain still has the
original owner. At which point the 2nd parameter could disappear there
as well.

Jan