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 <a...@ozlabs.ru> >>>>>>>> --- >>>>>>>> >>>>>>>> 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
