On 07.09.2021 10:33, Oleksandr Andrushchenko wrote: > On 06.09.21 16:23, Jan Beulich wrote: >> 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. > > 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 "ok" to have the code structured in the > assign_device as it is, e.g. roll back will be handled on deassign_device. > So, it is up to the toolstack to re-assign the device to Dom0 or DomIO(?) > in case of error and we do rely on the toolstack in Xen. > >> >> Also in the latter case don't you need to additionally call >> vpci_deassign_device() for the prior owner of the device? > > Even if we wanted to help the toolstack with the roll-back in case of an error > this would IMO make things even worth, e.g. we will de-assign for vPCI, but > will > leave IOMMU path untouched which would result in some mess. > So, my only guess here is that we should rely on the toolstack completely as > it was before PCI passthrough on Arm.
Well, okay, but please make this explicit in the description then. Jan
