qemu spapr-pci: added IRQ list to PCIBus
RFC PATCH 1/1 qemu spapr-pci: added IRQ list to PCIBus
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.
--- hw/pci.c | 18 ++++++++++++++++++ hw/pci.h | 1 + hw/pci_internals.h | 2 ++ 3 files changed, 21 insertions(+), 0 deletions(-)
hw/pci.c +18 -0
@@ -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 */
hw/pci.h +1 -0
@@ -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);
hw/pci_internals.h +2 -0
@@ -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 {
Signature · 1 line
Alexey
Quoted 9 lines · 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 addspci_get_irqcallback to every PCI bus to allow an external caller to calculate IRQ number from IRQ line (ABDD).
Quoted 4 lines
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.
Signature · 4 lines
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
Quoted 15 lines · David Gibson
Quoted 9 lines · 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 addspci_get_irqcallback to every PCI bus to allow an external caller to calculate IRQ number from IRQ line (ABDD).Quoted 4 lines
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.
Quoted 5 lines
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_irqmodel supports that. So for this function who is it that is defining the number space in whichpci_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?
Signature · 1 line
Alexey
Quoted 23 lines · Alexey Kardashevskiy
Quoted 15 lines · David Gibson
Quoted 9 lines · 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 addspci_get_irqcallback to every PCI bus to allow an external caller to calculate IRQ number from IRQ line (ABDD).Quoted 4 lines
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.
Quoted 5 lines
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_irqmodel supports that. So for this function who is it that is defining the number space in whichpci_get_irq()is returning values.
Quoted 4 lines
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.
Quoted 5 lines
At the moment this mechanism - EOI callbacks - operates with global IRQ numbers, both on x86 (acpi) and power (xics). So
pci_get_irqreturns 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.
Quoted 2 lines
If we do not want
pci_get_irqto 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?
Quoted 1 line
How can
PCIBus::pci_get_irq()_callbacks_ be useful?
It indicates support? Thanks,
Alex
Quoted 40 lines · Alex Williamson
Quoted 23 lines · Alexey Kardashevskiy
Quoted 15 lines · David Gibson
Quoted 9 lines · 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 addspci_get_irqcallback to every PCI bus to allow an external caller to calculate IRQ number from IRQ line (ABDD).Quoted 4 lines
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.
Quoted 5 lines
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_irqmodel supports that. So for this function who is it that is defining the number space in whichpci_get_irq()is returning values.Quoted 4 lines
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.
Quoted 5 lines
At the moment this mechanism - EOI callbacks - operates with global IRQ numbers, both on x86 (acpi) and power (xics). So
pci_get_irqreturns 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? :) ).
Quoted 5 lines
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.
Quoted 6 lines
pci_get_irqis 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 :)
Quoted 6 lines
Quoted 2 lines
If we do not want
pci_get_irqto 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?
Quoted 1 line
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.
Signature · 1 line
Alexey
Quoted 9 lines · Alexey Kardashevskiy
Quoted 5 lines
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:
.../...
Quoted 3 lines
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.
Quoted 10 lines
Quoted 6 lines
Quoted 2 lines
If we do not want
pci_get_irqto 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?
Quoted 1 line
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.
Quoted 9 lines · Benjamin Herrenschmidt
Quoted 7 lines · Alexey Kardashevskiy
Quoted 5 lines
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.
Quoted 10 lines
Quoted 2 lines
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:
.../...
Quoted 3 lines
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.
Quoted 12 lines
So you can certainly not write our global irq numbers in the config space, since the config space
IRQ_LINEregister 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_PINregister 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.
Quoted 4 lines
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
Quoted 16 lines · Benjamin Herrenschmidt
Quoted 9 lines · Alexey Kardashevskiy
Quoted 5 lines
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:
.../...
Quoted 7 lines
Quoted 3 lines
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_LINEregister is only 8 bits long which means it's not long enough.
[had a char]
No, it is all about piix3 extended capability.
Signature · 1 line
Alexey