Re: [Qemu-devel] [RFC PATCH] qemu spapr-pci: added IRQ list to PCIBus

[Alexey Kardashevskiy]

On 17/05/12 06:39, Alex Williamson wrote:
> On Mon, 2012-05-14 at 14:21 +1000, Alexey Kardashevskiy wrote:
>> On 14/05/12 11:58, David Gibson wrote:
>>> On Sat, May 12, 2012 at 05:29:53PM +1000, Alexey Kardashevskiy wrote:
>>>> There is a need for a mechanism to obtain an IRQ line number to
>>>> initialize End-Of-Interrupt handler.
>>>>
>>>> There is another proposed solution (commit
>>>> b7790763828b732059ad24ba0e64ce327563fe1a "pci: Add callbacks
>>>> to support retrieving and updating interrupts") which adds pci_get_irq
>>>> callback to every PCI bus to allow an external caller to calculate
>>>> IRQ number from IRQ line (ABDD).
>>>>
>>>> However it seems to be too complicated as it affects all PCI buses
>>>> while the only user of it is VFIO-PCI so this could be done simpler
>>>> by an array of 4 IRQs (lines A, B, C, D) in struct PCIBus which
>>>> every platform would initialize in its own way.
>>>
>>> I think you need to pin down the definition of what's going on here a
>>> bit better.  Not all platforms have a concept of global IRQ number,
>>> and the usual qemu_irq model supports that.  So for this function who
>>> is it that is defining the number space in which pci_get_irq() is
>>> returning values.
>>
>>
>> The idea is that legacy interrupt (INTx) is caught by a host driver (for 
>> example, vfio-pci). A
>> driver disables it and transfers to a guest. In order to enable it back, a 
>> host driver has to make
>> sure that the interrupt has been handled by a guest. It is done by an 
>> End-Of-Interrupt (EOI) message
>> sent by a guest. Then QEMU forwards the message to a host driver which 
>> enables INTx back.
> 
> Right, the gap is that we can signal the interrupt in qemu, but don't
> know which EOI to look for to re-enable the physical device.  Later
> we'll want to know the interrupt when we inject it so we can do so via
> an irqfd directly into kvm.
> 
>> At the moment this mechanism - EOI callbacks - operates with global IRQ 
>> numbers, both on x86 (acpi)
>> and power (xics). So pci_get_irq returns only global numbers which PCIBus 
>> receives from the calling
>> code somehow (platform specific code knows how to initialize them, a bus 
>> cannot resolve it itself
>> anyway). And this is not dynamically changing information as PCI _domain_ 
>> hotplug does not exist (am
>> I right? :) ).
> 
> It actually can change dynamically on x86 due to acpi interrupt links
> which allow the guest a generic way to select from a set of possible
> interrupt routing schemes.  And of course a chipset driver could twiddle
> bits if it wanted as well.  So, we really do need the update notifiers
> from my tree that this patch drops.


You mean notifiers like these: ioapic_add_gsi_eoi_notifier?
I did not drop them, we need them so I implemented them for XICS interrupt 
controller.

> pci_get_irq is one of the few
> things a qemu chipset needs to implement to get device assignment with
> vfio.  Doing it this way with a common array in PCIBus makes the patch
> smaller, but I don't know that it really simplifies anything.  The
> chipset function is trivial on x86, it's just knowing what to do that's
> the problem.

It does not move a global IRQ calculation into PCIBus as it is not PCI bus 
business.

static int piix3_get_irq(void *opaque, int irq_num)
{
    PIIX3State *piix3 = opaque;
    return piix3->dev.config[PIIX_PIRQC + irq_num];
}

So it stores global IRQs in the config space but it really unclear who writes 
these _global_ numbers
there. Is it the guest who allocates IRQs and writes the numbers into the 
config space so QEMU knows
what pin is what IRQ? If so, I am wrong, you are right :)


>> If we do not want pci_get_irq to return global IRQ numbers than it makes 
>> more sense to keep using
>> pins (A,B,C,D) plus some bus ID and not introduce any new numbers at all.
> How does that solve the initial problem of getting the EOI?

>> How can PCIBus::pci_get_irq() _callbacks_ be useful?
> It indicates support?  Thanks,

Flag is enough :)
Honestly when I see a "get", I am looking for a "put" in any form. And for 
powerpc it makes some
sense as IRQ is set in QEMU, not from the guest.


-- 
Alexey