On 03.09.2021 12:08, Oleksandr Andrushchenko wrote:
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -864,6 +864,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;
>
> @@ -1425,6 +1429,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",
I have to admit that I'm worried by the further lack of unwinding in
case of error: We're not really good at this, I agree, but it would
be quite nice if the problem didn't get worse. At the very least if
the device was de-assigned from Dom0 and assignment to a DomU failed,
imo you will want to restore Dom0's settings.
Also in the latter case don't you need to additionally call
vpci_deassign_device() for the prior owner of the device?
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -86,6 +86,27 @@ int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
>
> return rc;
> }
> +
> +/* Notify vPCI that device is assigned to guest. */
> +int vpci_assign_device(struct domain *d, struct pci_dev *dev)
> +{
> + /* It only makes sense to assign for hwdom or guest domain. */
> + if ( !has_vpci(d) || (d->domain_id >= DOMID_FIRST_RESERVED) )
Please don't open-code is_system_domain(). I also think you want to
flip the two sides of the ||, to avoid evaluating whatever has_vcpi()
expands to for system domains. (Both again below.)
> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -26,6 +26,12 @@ typedef int vpci_register_init_t(struct pci_dev *dev);
> /* Add vPCI handlers to device. */
> int __must_check vpci_add_handlers(struct pci_dev *dev);
>
> +/* Notify vPCI that device is assigned to guest. */
> +int __must_check vpci_assign_device(struct domain *d, struct pci_dev *dev);
> +
> +/* Notify vPCI that device is de-assigned from guest. */
> +int __must_check vpci_deassign_device(struct domain *d, struct pci_dev *dev);
Is the expectation that "dev" may get altered? If not, it may want to
become pointer-to-const. (For "d" there might be the need to acquire
locks, so I guess it might not be a god idea to constify that one.)
I also think that one comment ought to be enough for the two functions.
Jan
