Re: [RFC PATCH linux-2.6.18-xen] pciback: clean up (MSI-X vec, entrynr) list when resetting PCI device

[Laszlo Ersek <lersek@redhat.com>]

On 06/01/11 16:59, Jan Beulich wrote:
>>>> On 01.06.11 at 16:12, Laszlo Ersek<lersek@redhat.com>  wrote:
>> On 06/01/11 14:56, Jan Beulich wrote:
>>>>>> On 01.06.11 at 12:05, Laszlo Ersek<lersek@redhat.com>   wrote:
>>
>>>> domU: igbvf_shutdown()
>>>>           igbvf_suspend()
>>>>             pci_disable_device()
>>>>               pcibios_disable_device()
>>>>                 pcibios_disable_resources()
>>>>                   pci_write_config_word(
>>>>                       dev, PCI_COMMAND,
>>>>                       orig&   ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY)
>>>>                   )
>>>>                     ...
>>>>                       pcifront_bus_write()
>>>>                         do_pci_op(XEN_PCI_OP_conf_write)
>>>> dom0:                   pciback_do_op()
>>>>                             pciback_config_write()
>>>>                               conf_space_write()
>>>>                                 command_write() [via PCI_COMMAND funcptr]
>>>>                                   pci_disable_device()
>>>>                                     disable_msi_mode()
>>>>                                       dev->msix_enabled = 0;
>>>>

>>> So I wonder whether you shouldn't fix
>>> pci_disable_device() instead, or alternatively move the vector
>>> de-allocation (and then for MSI and MSI-X) into disable_msi_mode().
>>
>> Yes, I did think of that, however IMO that would introduce exactly the
>> problem that you describe below. If either pci_disable_device() or
>> disable_msi_mode() frees the vector, then re-enabling the device can
>> face yet another stumbling block.
>
> Why - pci_enable_msi{,x}() call msi{,x}_capability_init(), which
> obtains the vectors.

Which can fail. I imagined there was a conscious deliberation in the 
background: hold on to the vectors we already have, just enable/disable 
generation of the MSIs in the device.


> The problem is that calling disable_msi_mode() alone is always a
> mistake, this should have been a static function just like its
> counterpart, enable_msi_mode().

Well this would unify the "disposition" and the "generation", and 
dev->msix_enabled would have a clear meaning.


>>> While the approach you take covers the guest shutdown/restart
>>> case, what if the guest called its pci_disable_device() at runtime,
>>> expecting to be able to call pci_enable_{device,msi,msix}() on it
>>> again later on? Your newly added function would also be called here,
>>
>> May I ask how? The function is only called from pciback_reset_device(),
>
> I was just looking at the call tree you provided (and which is still
> quoted at the top). Oh - you're referring to Dom0 calling the
> function, whereas I wrote about what happens when guest calls
> its version of it.

> Yeah, when the domain is gone you have this problem. But if you
> properly removed the vectors while the domain is still there, you
> shouldn't.

> For that situation this was the natural place, but you're dealing
> with an alive domain not being cleaned up after properly during
> certain operations, and deferring the cleanup until the domain is
> gone is only going to call for further problems (as outlined earlier,
> and who knows what other cases we didn't consider).

The original idea (probably the most straightforward one) was to modify 
the igbvf driver (ie. the domU) so that it would call igbvf_remove() 
from igbvf_shutdown(), instead of the current igbvf_suspend(); 
restricted to the case when the code runs in a PV domU. igbvf_remove() 
calls igbvf_reset_interrupt_capability(), which in turn calls 
pci_disable_msix(). The latter percolates through the entire stack and 
does the right thing.

Or, as you say above, the guest's generic MSI-X massaging PCI code could 
be modified.

We didn't want to leave this to the guest, however. It's not that I 
intentionally try to postpone the cleanup until after the domain has 
vanished; it's rather the domU can't be expected / trusted to do the 
cleanup itself in all cases.

(I realize we can't talk about "security" in this case at all, since 
passthrough-to-PV doesn't cross the IOMMU and "has the potential to 
access the DMA address space of dom0, which is a potential security 
concern" anyway. Or some such.)


> Hence while I realize that from a pragmatic point of view your
> approach may seem acceptable, it only fixing a particular aspect
> of a more general problem is not really something I like.

Thank you for the review :)
lacos