Hi Jan, > On 13 Oct 2021, at 07:10, Jan Beulich <jbeul...@suse.com> wrote: > > On 12.10.2021 23:37, Stefano Stabellini wrote: >> On Tue, 12 Oct 2021, Jan Beulich wrote: >>> On 11.10.2021 20:18, Stefano Stabellini wrote: >>>> On Mon, 11 Oct 2021, Jan Beulich wrote: >>>>> On 11.10.2021 15:34, Bertrand Marquis wrote: >>>>>>> On 11 Oct 2021, at 14:09, Jan Beulich <jbeul...@suse.com> wrote: >>>>>>> On 11.10.2021 14:41, Bertrand Marquis wrote: >>>>>>>> But digging deeper into this, I would have 2 questions: >>>>>>>> >>>>>>>> - msi_cleanup was done there after a request from Stefano, but is not >>>>>>>> done in case or iommu error, is there an issue to solve here ? >>>>>>> >>>>>>> Maybe, but I'm not sure. This very much depends on what a domain >>>>>>> could in principle do with a partly set-up device. Plus let's >>>>>>> not forget that we're talking of only Dom0 here (for now at least, >>>>>>> i.e. not considering the dom0less case). >>>>>>> >>>>>>> But I'd also like to further defer to Stefano. >>>>>> >>>>>> Ok, I must admit I do not really see at that stage why doing an MSI >>>>>> cleanup >>>>>> could be needed so I will wait for Stefano to know if I need to keep >>>>>> this when >>>>>> moving the block up (at the end it is theoretical right now as this is >>>>>> empty). >>>> >>>> I know that MSIs are not supported yet on ARM (pci_cleanup_msi does >>>> nothing). But I wanted to make sure that the pci_cleanup_msi() calls are >>>> present anywhere necessary, especially on the error paths. So that once >>>> we add MSI support, we don't need to search through the code to find all >>>> the error paths missing a pci_cleanup_msi() call. >>>> >>>> To answer your first question: you are right, we are also missing a >>>> pci_cleanup_msi() call in the case of IOMMU error. So it might be better >>>> to move the call to pci_cleanup_msi() under the "out" label so that we >>>> can do it once for both cases. >>>> >>>> To answer your second point about whether it is necessary at all: if >>>> MSIs and MSI-Xs cannot be already setup at this point at all (not even >>>> the enable bit), then we don't need any call to pci_cleanup_msi() in >>>> pci_add_device. >>> >>> Well, at the very least MSI can't be set up ahead of the traps getting >>> put in place. Whether partial success of putting traps in place may >>> allow a cunning guest to set up MSI may depend on further aspects. >> >> Good point about MSIs not being setup before the traps. We should remove >> the call to pci_cleanup_msi() in the error path then. > > Your reply makes me fear you didn't pay enough attention to the "partial" > in my earlier reply. The traps for the various registers can't all be set > up atomically, so there may be a transient period where enough traps are > already in place for a cunning guest to arrange for setup. Unless, as > said, there are further setup steps needed before a guest could succeed > in doing so. > > But even if partial trap setup alone was sufficient, I think the cleaning > up of MSI then might still better go on the error path there than on that > of pci_add_device().
I think I should put the msi_cleanup in the exit path if pdev is not null but we got a non null ret (in an else if ( pdev ) ). This would cover all exit paths, especially as I will move the add_handler before the iommu init. Would that be ok for everyone ? Cheers Bertrand
