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

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

[Alexey Kardashevskiy]

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.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/pci.c           |   18 ++++++++++++++++++
 hw/pci.h           |    1 +
 hw/pci_internals.h |    2 ++
 3 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 1f7c924..8c2e193 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1067,6 +1067,24 @@ static void pci_set_irq(void *opaque, int irq_num, int level)
     pci_change_irq_level(pci_dev, irq_num, change);
 }

+int pci_get_irq(PCIDevice *pci_dev, int pin)
+{
+    PCIBus *bus;
+    for (;;) {
+        if (!pci_dev)
+            return -ENOSYS;
+        bus = pci_dev->bus;
+        if (!bus)
+            return -ENOSYS;
+        pin = bus->map_irq(pci_dev, pin);
+        if (pin) {
+            return bus->irqs[pin];
+        }
+        pci_dev = bus->parent_dev;
+    }
+    return 0;
+}
+
 /***********************************************************/
 /* monitor info on PCI */

diff --git a/hw/pci.h b/hw/pci.h
index 1273dc3..25b414a 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -257,6 +257,7 @@ uint32_t pci_default_read_config(PCIDevice *d,
                                  uint32_t address, int len);
 void pci_default_write_config(PCIDevice *d,
                               uint32_t address, uint32_t val, int len);
+int pci_get_irq(PCIDevice *pci_dev, int pin);
 void pci_device_save(PCIDevice *s, QEMUFile *f);
 int pci_device_load(PCIDevice *s, QEMUFile *f);
 MemoryRegion *pci_address_space(PCIDevice *dev);
diff --git a/hw/pci_internals.h b/hw/pci_internals.h
index b6b7a0e..2f53039 100644
--- a/hw/pci_internals.h
+++ b/hw/pci_internals.h
@@ -38,6 +38,8 @@ struct PCIBus {
        Keep a count of the number of devices with raised IRQs.  */
     int nirq;
     int *irq_count;
+
+    uint32_t irqs[PCI_NUM_PINS]; /* A, B, C, D */
 };

 struct PCIBridge {

-- 
Alexey

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

[David Gibson]

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.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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

[Alexey Kardashevskiy]

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.

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? :) ).

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 can PCIBus::pci_get_irq() _callbacks_ be useful?



-- 
Alexey

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

[Alex Williamson]

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.  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.

> 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,

Alex

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

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

[Benjamin Herrenschmidt]

On Thu, 2012-05-17 at 12:16 +1000, Alexey Kardashevskiy wrote:

> > 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.

So I haven't completely understood the problem, however:

 .../...

> 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 :)

So you can certainly not write our global irq numbers in the config
space, since the config space IRQ_LINE register is only 8 bits long
which means it's not long enough.

In general we don't use that register on ppc anyways, it's just a
storage, it should have no actual effect on HW.

The IRQ_PIN register however is important and needs to indicate
which of the 4 interrupt lines associated with the bus the
device will trigger, so we need to get that right especially
vs. swizzling performed by intermediary bridges.

Here, we can either show all the bridges to the guest (emulate
them is fine) and keep the original PIN numbers, or we need
to filter config space to change the PIN numbers seen by the
guest to match the lack of appropriate swizzling.

> >> 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.

Cheers,
Ben.

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

[Alex Williamson]

On Thu, 2012-05-17 at 13:00 +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2012-05-17 at 12:16 +1000, Alexey Kardashevskiy wrote:
> 
> > > 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.
> 
> So I haven't completely understood the problem, however:
> 
>  .../...
> 
> > 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 :)

Yes, from the piix4 spec:

        4.1.10.
        PIRQRC[A:D]—PIRQX ROUTE CONTROL REGISTERS (FUNCTION 0)
        Address Offset: 60h (PIRQRCA#)–63h (PIRQRCD#)
        Default Value: 80h
        Attribute: R/W
        
        These registers control the routing of the PIRQ[A:D]# signals to
        the IRQ inputs of the interrupt controller. Each PIRQx# can be
        independently routed to any one of 11 interrupts. All four
        PIRQx# lines can be routed to the same IRQx input. Note that the
        IRQ that is selected through bits [3:0] must be set to level
        sensitive mode in the corresponding ELCR Register. When a PIRQ
        signal is routed to an interrupt controller IRQ, PIIX4 masks the
        corresponding IRQ signal.

Touching this register directly by the guest might be discouraged, but
we provide PCI interrupt link devices in ACPI namespace that allows the
guest to mess with this in a more indirect way.

> So you can certainly not write our global irq numbers in the config
> space, since the config space IRQ_LINE register is only 8 bits long
> which means it's not long enough.
> 
> In general we don't use that register on ppc anyways, it's just a
> storage, it should have no actual effect on HW.
> 
> The IRQ_PIN register however is important and needs to indicate
> which of the 4 interrupt lines associated with the bus the
> device will trigger, so we need to get that right especially
> vs. swizzling performed by intermediary bridges.
> 
> Here, we can either show all the bridges to the guest (emulate
> them is fine) and keep the original PIN numbers, or we need
> to filter config space to change the PIN numbers seen by the
> guest to match the lack of appropriate swizzling.

That's a different problem.  For this it doesn't matter which IRQ_PIN
the device claims to pull when signaling an interrupt so long as qemu
and the config space exposed to the guest are in agreement.  We should
be able to easily virtualize IRQ_PIN if we want to avoid swizzling
problems.  The problem here is that we've told qemu to fire the
interrupt to the guest for the device and have disabled the interrupt
for the physical device; how do we know when to re-enable it?  An
emulated device sets the interrupt level back when the guest has
interacted with the device and there's nothing left to do.  This happens
in the emulated driver code.  For an assigned device we have no idea
what guest access indicates that the interrupt is done, so we try to
take advantage of PCI using level interrupts and figure out which EOI
corresponds to the interrupt we triggered.  Thanks,

Alex

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

[Alexey Kardashevskiy]

On 17/05/12 13:00, Benjamin Herrenschmidt wrote:
> On Thu, 2012-05-17 at 12:16 +1000, Alexey Kardashevskiy wrote:
> 
>>> 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.
> 
> So I haven't completely understood the problem, however:
> 
>  .../...
> 
>> 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 :)
> 
> So you can certainly not write our global irq numbers in the config
> space, since the config space IRQ_LINE register is only 8 bits long
> which means it's not long enough.

[had a char]
No, it is all about piix3 extended capability.



-- 
Alexey