Re: [Qemu-devel] [PATCH v2 8/8] spapr_pci: Use XICS interrupt allocator and do not cache interrupts in PHB

[Alexey Kardashevskiy]

On 05/21/2014 07:38 PM, Michael S. Tsirkin wrote:
> On Wed, May 21, 2014 at 07:33:36PM +1000, Alexey Kardashevskiy wrote:
>> On 05/21/2014 07:13 PM, Alexander Graf wrote:
>>>
>>> On 21.05.14 11:11, Michael S. Tsirkin wrote:
>>>> On Wed, May 21, 2014 at 11:06:09AM +0200, Alexander Graf wrote:
>>>>> On 21.05.14 10:52, Alexey Kardashevskiy wrote:
>>>>>> On 05/21/2014 06:40 PM, Alexander Graf wrote:
>>>>>>> On 15.05.14 11:59, Alexey Kardashevskiy wrote:
>>>>>>>> Currently SPAPR PHB keeps track of all allocated MSI/MISX interrupt as
>>>>>>>> XICS used to be unable to reuse interrupts which becomes a problem for
>>>>>>>> dynamic MSI reconfiguration which is happening on guest driver reload 
>>>>>>>> or
>>>>>>>> PCI hot (un)plug. Another problem is that PHB has a limit of devices
>>>>>>>> supporting MSI/MSIX (SPAPR_MSIX_MAX_DEVS=32) and there is no good 
>>>>>>>> reason
>>>>>>>> for that.
>>>>>>>>
>>>>>>>> This makes use of new XICS ability to reuse interrupts.
>>>>>>>>
>>>>>>>> This removes cached MSI configuration from SPAPR PHB so the first IRQ
>>>>>>>> number
>>>>>>>> of a device is stored in MSI/MSIX config space so there is no need to
>>>>>>>> store
>>>>>>>> this anywhere else. From now on, SPAPR PHB only keeps flags telling 
>>>>>>>> what
>>>>>>>> type
>>>>>>>> of interrupt for which device it has configured in order to return
>>>>>>>> error if
>>>>>>>> (for example) MSIX was enabled and the guest is trying to disable MSI
>>>>>>>> which
>>>>>>>> it has not enabled.
>>>>>>>>
>>>>>>>> This removes a limit for the maximum number of MSIX-enabled devices
>>>>>>>> per PHB,
>>>>>>>> now XICS and PCI bus capacity are the only limitation.
>>>>>>>>
>>>>>>>> This changes migration stream as it fixes vmstate_spapr_pci_msi::name
>>>>>>>> which was
>>>>>>>> wrong since the beginning.
>>>>>>>>
>>>>>>>> This fixed traces to be more informative.
>>>>>>>>
>>>>>>>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> In reality either MSIX or MSI is enabled, never both. So I could remove
>>>>>>>> msi/msix
>>>>>>>> bitmaps from this patch, would it make sense?
>>>>>>> Is this a hard requirement? Does a device have to choose between MSIX 
>>>>>>> and
>>>>>>> MSI or could it theoretically have both enabled? Is this a PCI
>>>>>>> limitation,
>>>>>>> a PAPR/XICS limitation or just a limitation of your implementation?
>>>>>> My implementation does not have this limitation, I asked if I can 
>>>>>> simplify
>>>>>> code by introducing one :)
>>>>>>
>>>>>> I cannot see any reason why PCI cannot have both MSI and MSIX enabled but
>>>>>> it does not seem to be used by anyone => cannot debug and confirm.
>>>>>>
>>>>>> PAPR spec assumes that if the guest tries enabling MSIX when MSI is
>>>>>> already
>>>>>> enabled, this is a "change", not enabling both types. But it also says 
>>>>>> MSI
>>>>>> and MSIX vector numbers are not shared. Hm.
>>>>> Yeah, I'm not aware of any limitation on hardware here and I'd
>>>>> rather not impose one.
>>>>>
>>>>> Michael, do you know of any hardware that uses MSI *and* MSI-X at
>>>>> the same time?
>>>>>
>>>>>
>>>>> Alex
>>>> No, and the PCI spec says:
>>>>     A function is permitted to implement both MSI and MSI-X, but system
>>>>     software is
>>>>     prohibited from enabling both at the same time. If system software
>>>>     enables both at the same time, the result is undefined.
>>>
>>> Ah, cool. So yes Alexey, feel free to impose it :).
>>
>> Heh. This solves just half of the problem - I still have to keep track of
>> what device got MSI/MSIX configured via that ibm,change-msi interface. I
>> was hoping I can store such flag somewhere in a device PCI config space but
>> MSI/MSIX enable bit is not good as it is not set when those calls are made.
> 
> Hmm could you pls remind me why is it desirable to store this
> in device?

I need this flag to know if I can process "disable" or return an error. So
I need to save it somewhere. And there can be up to 256 buses * 32 dev * 8
functions = 65536 flags which is 8KB. And only a small portion of it will
ever be used for obvious reasons. Having 1 bit anywhere in config space or
QEMU's PCIDevice would help here...

At the moment I keep an array in SPAPR's PHB, it is 32 entries long so
SPAPR PHB can have only 32 MSI-enabled devices and our testers think this
is not enough :)


> Device is not yet sending MSI interrupts after all
> otherwise enable would be set.

That is correct.


> 
>> And I cannot rely on address/data fields much as the guest can change them
>> (I already use them to store IRQ numbers and btw it is missing checks when
>> I read them back for disposal, I'll fix in next round).
>>
>> Or on "enable" event I could put IRQ numbers to .data of MSI config space
>> and on "disable" check if it is not zero, then configuration took place,
>> then I can remove my msi[]/msix[] flag arrays. If the guest did any change
>> to MSI/MSIX config space (it does not on SPAPR except weird selftest
>> cases), I compare .data with what ICS can possibly have and either reject
>> "disable" or handle it and if it breaks XICS - that's too bad for the
>> stupid guest. Would that be acceptable?
> 
> 
>>
>> -- 
>> Alexey


-- 
Alexey