On Wed, Jul 26, 2023 at 01:38:30AM +0000, Volodymyr Babchuk wrote: > > Hi Roger, > > Roger Pau Monné <[email protected]> writes: > > > On Thu, Jul 20, 2023 at 12:32:31AM +0000, Volodymyr Babchuk wrote: > >> From: Oleksandr Andrushchenko <[email protected]> > >> @@ -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. > > Yes, thanks. > > >> + 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? > > I believe, intention was to not leave device in an unknown state: we > failed both assign_device() and deassign_device() call, so what to do > now? But yes, I think you are right and it is better to let caller to > decide what to do next.
I don't think it would be a security risk to leave the device in that state. For domUs the guest won't get access to the device registers anyway as we use an allow list approach. Also deassign_device() is not called when we fail to assign one of the phantom functions. We don't seem to undo any of the work in assign_device() on error so it should be fine to not do the call to deassign_device() on error to initialize vPCI. > > > > 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, ...); > > But deassign_device() has the loop for all phantom devices that already > does all the work. Unless I miss something, of course. No, you are right, a single call is fine. Thanks, Roger.
