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

[Jan Beulich <jbeulich@xxxxxxxx>]

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 <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.

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().

Jan