Re: [PATCH v2] kvm: Disable MSI/MSI-X in assigned device reset path

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 2894 bytes --]

On 2012-04-08 18:08, Avi Kivity wrote:
> On 04/08/2012 07:04 PM, Michael S. Tsirkin wrote:
>> On Sun, Apr 08, 2012 at 06:50:27PM +0300, Avi Kivity wrote:
>>> On 04/08/2012 06:46 PM, Michael S. Tsirkin wrote:
>>>>>>>
>>>>>>> I'm thinking about this flow:
>>>>>>>
>>>>>>>   FLR the device
>>>>>>>   for each emulated register
>>>>>>>      read it from the hardware
>>>>>>>      if different from emulated register:
>>>>>>>         update the internal model (for example, disabling MSI in kvm if
>>>>>>> needed)
>>>>>>
>>>>>> If we do it this way we get back the problem this patch
>>>>>> is trying to solve: MSIX assigned while device
>>>>>> memory is disabled would cause unsupported request errors.
>>>>>
>>>>> Why is that?  FLR would presumably disable MSI in the device, and this
>>>>> line would disable it in kvm as well.
>>>>
>>>> The bug is that device memory is disabled (FLR would do that)
>>>> while MSI is enabled in kvm. The fix is to
>>>> disable MSI in kvm first.
>>>
>>> Yes, no need to repeat.  My question is whether my pseudo-code does the
>>> same
>>
>> It doesn't seem to: FLR (disabling memory) is followed
>> by MSI disable in kvm instead of the reverse.
> 
> Ah, so the problem is the ordering?  I see.
> 
>>> and whether or not if it is better (when applied to all emulated
>>> config space).
>>
>> I'm not sure.
>> I would like to see an example of a register that you have
>> in mind.
> 
> I went over the PCI registers and saw none that would be affected.
> 
>>>>
>>>> Yes. I'm talking about things like enabling memory, setting up irq register,
>>>> etc though. Most of this setup is done by bios.
>>>
>>> I see.  So should we have a pci_reset_function() variant that limits
>>> itself to restoring just those bits?
>>
>> We only need kernel to restore whatever qemu emulates, but
>> kernel doesn't know what that is.
>> What kind of interface do you have in mind?
>>
> 
> The same as pci_reset_function(), but leaves MSI clear.
> 
> I guess it's not worth it if the ordering problem is there.

The core problem is not the ordering. The problem is that the kernel is
susceptible to ordering mistakes of userspace. And that is because the
kernel panics on PCI errors of devices that are in user hands - a
critical kernel bug IMHO. Proper reset of MSI or even the whole PCI
config space is another issue, but one the kernel should not worry about
- still, it should be fixed (therefore this patch).

But even if we disallowed userland to disable MMIO and PIO access to the
device, we would be be able to exclude that there are secrete channels
in the device's interface having the same effect. So we likely need to
enhance PCI error handling to catch and handle faults for certain
devices differently - those we cannot trust to behave properly while
they are under userland/guest control.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]