Re: [PATCH v4] kvm: Use a bitmap for tracking used GSIs

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, May 18, 2009 at 02:36:41PM +0300, Avi Kivity wrote:
> Michael S. Tsirkin wrote:
>> On Sun, May 17, 2009 at 11:47:53PM +0300, Avi Kivity wrote:
>>   
>>> Alex Williamson wrote:
>>>     
>>>> On Wed, 2009-05-13 at 08:33 -0600, Alex Williamson wrote:
>>>>         
>>>>> On Wed, 2009-05-13 at 08:15 -0600, Alex Williamson wrote:
>>>>>             
>>>>>> On Wed, 2009-05-13 at 16:55 +0300, Michael S. Tsirkin wrote:
>>>>>>                 
>>>>>>> Very surprising: I haven't seen any driver disable MSI expect on device
>>>>>>> destructor path. Is this a linux guest?
>>>>>>>                     
>>>>>> Yes, Debian 2.6.26 kernel.  I'll check it it behaves the same on newer
>>>>>> upstream kernels and try to figure out why it's doing it.
>>>>>>                 
>>>>> Updating the guest to 2.6.29 seems to fix the interrupt toggling.  So
>>>>> it's either something in older kernels or something debian introduced,
>>>>> but that seems unlikely.
>>>>>             
>>>> For the curious, this was fixed prior to 2.6.27-rc1 by this:
>>>>
>>>> commit ce6fce4295ba727b36fdc73040e444bd1aae64cd
>>>> Author: Matthew Wilcox
>>>> Date:   Fri Jul 25 15:42:58 2008 -0600
>>>>
>>>>     PCI MSI: Don't disable MSIs if the mask bit isn't supported
>>>>         David Vrabel has a device which generates an interrupt 
>>>> storm on the INTx
>>>>     pin if we disable MSI interrupts altogether.  Masking interrupts is only
>>>>     a performance optimisation, so we can ignore the request to mask the
>>>>     interrupt.
>>>>
>>>> It looks like without the maskbit attribute on MSI, the default way to
>>>> mask an MSI interrupt was to toggle the MSI enable bit.  This was
>>>> introduced in 58e0543e8f355b32f0778a18858b255adb7402ae, so it's lifespan
>>>> was probably 2.6.21 - 2.6.26.
>>>>
>>>>         
>>> On the other hand, if the host device supports this maskbit 
>>> attribute,  we might want to support it.  I'm not sure exactly how 
>>> though.
>>>
>>> If we trap msi entry writes, we're inviting the guest to exit every 
>>> time  it wants to disable interrupts.  If we don't, we're inviting 
>>> spurious  interrupts, which will cause unwanted exits and injections.
>>>     
>>
>> Avi, I think you are mixing up MSI and MSI-X. MSI does not have any tables:
>> all changes go through configuration writes, which AFAIK we always trap.
>> Isn't that right?
>>   
>
> Right.
>
>> On the other hand, in MSI-X mask bit is mandatory, not optional
>> so we'll have to support it for assigned devices at some point.
>>
>> If we are worried about speed of masking/unmasking MSI-X interrupts for
>> assigned devices (older kernels used to mask them, recent kernels leave
>> this to drivers) we will probably need to have MSI-X support in the
>> kernel, and have kernel examine the mask bit before injecting the
>> interrupt, just like real devices do.
>>   
>
> Yes.

Actually, if we do that, we'll need to address a race where a driver
has updated the mask bit in the window after we tested it
and before we inject the interrupt. Not sure how to do this.

> Let's actually quantify this though.  Several times per second  
> doesn't qualify.

Oh, I didn't actually imply that we need to optimize this path.
You seemed worried about the speed of masking the interrupt,
and I commented that to optimize it we'll have to move it
to kernel.

>>> Maybe we ought to let the guest write to the msi tables without   
>>> trapping, and in the injection logic do something like
>>>
>>>    msi_entry = *msi_entry_ptr;
>>>    mb();
>>>    if (msi_entry != msi->last_msi_entry)
>>>         msi_reconfigure();
>>>    if (msi_enabled(msi_entry))
>>>         insert_the_needle();
>>>     
>>
>> I don't really understand why do you need the reconfigure
>> and tracking last msi entry here.
>>   
>
> The msi entry can change the vector and delivery mode, no?  This way we  
> can cache some stuff instead of decoding it each time.  For example, if  
> the entry points at a specific vcpu, we can cache the vcpu pointer,  
> instead of searching for the apic ID.

-- 
MST