Re: [PATCH v5 08/11] xen/arm: Enable the existing x86 virtual PCI support for ARM.

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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 <jbeulich@xxxxxxxx> 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.