[PATCH] testdev: adjust for ISA irq changes

[PATCH] testdev: adjust for ISA irq changes

[Avi Kivity]

Recent changes killed the ioapic_irq_hack hack, use the isa_get_irq() API
instead.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 hw/isa-bus.c |    2 +-
 hw/testdev.c |    4 +---
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/isa-bus.c b/hw/isa-bus.c
index 2765543..7e06efc 100644
--- a/hw/isa-bus.c
+++ b/hw/isa-bus.c
@@ -66,7 +66,7 @@ void isa_bus_irqs(qemu_irq *irqs)
  */
 qemu_irq isa_get_irq(int isairq)
 {
-    if (isairq < 0 || isairq > 15) {
+    if (isairq < 0 || isairq > 23) {
         hw_error("isa irq %d invalid", isairq);
     }
     return isabus->irqs[isairq];
diff --git a/hw/testdev.c b/hw/testdev.c
index f0355c8..bf1611c 100644
--- a/hw/testdev.c
+++ b/hw/testdev.c
@@ -28,11 +28,9 @@ static uint32_t test_device_memsize_read(void *opaque, uint32_t addr)
     return ram_size;
 }
 
-extern qemu_irq *ioapic_irq_hack;
-
 static void test_device_irq_line(void *opaque, uint32_t addr, uint32_t data)
 {
-    qemu_set_irq(ioapic_irq_hack[addr - 0x2000], !!data);
+    qemu_set_irq(isa_get_irq(addr - 0x2000), !!data);
 }
 
 static uint32 test_device_ioport_data;
-- 
1.7.5.2

Re: [PATCH] testdev: adjust for ISA irq changes

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 904 bytes --]

On 2011-05-29 14:57, Avi Kivity wrote:
> Recent changes killed the ioapic_irq_hack hack, use the isa_get_irq() API
> instead.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  hw/isa-bus.c |    2 +-
>  hw/testdev.c |    4 +---
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/isa-bus.c b/hw/isa-bus.c
> index 2765543..7e06efc 100644
> --- a/hw/isa-bus.c
> +++ b/hw/isa-bus.c
> @@ -66,7 +66,7 @@ void isa_bus_irqs(qemu_irq *irqs)
>   */
>  qemu_irq isa_get_irq(int isairq)
>  {
> -    if (isairq < 0 || isairq > 15) {
> +    if (isairq < 0 || isairq > 23) {

That's a fairly evil hack. It will break again when we clean up the kvm
irqchips (not to speak of side effects in non-irqchip/non-kvm mode).

Why not hook into the kvm irqchip directly? Would keep generic code out
of this business until we have real irq pin manipulation in qemu.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [PATCH] testdev: adjust for ISA irq changes

[Avi Kivity]

On 05/29/2011 06:05 PM, Jan Kiszka wrote:
> On 2011-05-29 14:57, Avi Kivity wrote:
> >  Recent changes killed the ioapic_irq_hack hack, use the isa_get_irq() API
> >  instead.
> >
> >  Signed-off-by: Avi Kivity<avi@redhat.com>
> >  ---
> >   hw/isa-bus.c |    2 +-
> >   hw/testdev.c |    4 +---
> >   2 files changed, 2 insertions(+), 4 deletions(-)
> >
> >  diff --git a/hw/isa-bus.c b/hw/isa-bus.c
> >  index 2765543..7e06efc 100644
> >  --- a/hw/isa-bus.c
> >  +++ b/hw/isa-bus.c
> >  @@ -66,7 +66,7 @@ void isa_bus_irqs(qemu_irq *irqs)
> >    */
> >   qemu_irq isa_get_irq(int isairq)
> >   {
> >  -    if (isairq<  0 || isairq>  15) {
> >  +    if (isairq<  0 || isairq>  23) {
>
> That's a fairly evil hack. It will break again when we clean up the kvm
> irqchips (not to speak of side effects in non-irqchip/non-kvm mode).
>
> Why not hook into the kvm irqchip directly? Would keep generic code out
> of this business until we have real irq pin manipulation in qemu.

Firstly, I did the minimal patch to get things working, as is my custom 
when fixing up merge/refactoring problems.  Not that I often follow up 
with the clean fix.

Second, I'm not sure it's such a hack.  Suppose our motherboard wired 
the PCI links to GSI16-19 (or GSI16-23, as we once wanted before we had 
MSI-X)?  We'd need an API to access non-ISA interrupt lines.

So what's the clean fix here?  gsi_get_irq()?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] testdev: adjust for ISA irq changes

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2228 bytes --]

On 2011-05-29 17:10, Avi Kivity wrote:
> On 05/29/2011 06:05 PM, Jan Kiszka wrote:
>> On 2011-05-29 14:57, Avi Kivity wrote:
>> >  Recent changes killed the ioapic_irq_hack hack, use the
>> isa_get_irq() API
>> >  instead.
>> >
>> >  Signed-off-by: Avi Kivity<avi@redhat.com>
>> >  ---
>> >   hw/isa-bus.c |    2 +-
>> >   hw/testdev.c |    4 +---
>> >   2 files changed, 2 insertions(+), 4 deletions(-)
>> >
>> >  diff --git a/hw/isa-bus.c b/hw/isa-bus.c
>> >  index 2765543..7e06efc 100644
>> >  --- a/hw/isa-bus.c
>> >  +++ b/hw/isa-bus.c
>> >  @@ -66,7 +66,7 @@ void isa_bus_irqs(qemu_irq *irqs)
>> >    */
>> >   qemu_irq isa_get_irq(int isairq)
>> >   {
>> >  -    if (isairq<  0 || isairq>  15) {
>> >  +    if (isairq<  0 || isairq>  23) {
>>
>> That's a fairly evil hack. It will break again when we clean up the kvm
>> irqchips (not to speak of side effects in non-irqchip/non-kvm mode).
>>
>> Why not hook into the kvm irqchip directly? Would keep generic code out
>> of this business until we have real irq pin manipulation in qemu.
> 
> Firstly, I did the minimal patch to get things working, as is my custom
> when fixing up merge/refactoring problems.  Not that I often follow up
> with the clean fix.

The minimal fix would be simply initializing ioapic_isa_hack again. That
was lost while merging with the xen bits.

> 
> Second, I'm not sure it's such a hack.  

The hack is that isa_get_irq is supposed to return ISA IRQs, not GSIs.
Name-wise, there is already a bit mixed up in QEMU, but at least not in
that function.

> Suppose our motherboard wired
> the PCI links to GSI16-19 (or GSI16-23, as we once wanted before we had
> MSI-X)?  We'd need an API to access non-ISA interrupt lines.
> 
> So what's the clean fix here?  gsi_get_irq()?

Maybe. Depends on the requirements of the testdev. If you also want to
address PIC and IOAPIC separately or simulate injection from a specific
device, we need more logic.

We also need a better interface to discover and track legacy IRQ routes
for device assignment. Markus is currently collecting requirements for
qdev enhancements, and I think generic IRQ manipulation and discovery
belongs there.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [PATCH] testdev: adjust for ISA irq changes

[Avi Kivity]

On 05/29/2011 06:21 PM, Jan Kiszka wrote:
> >  Suppose our motherboard wired
> >  the PCI links to GSI16-19 (or GSI16-23, as we once wanted before we had
> >  MSI-X)?  We'd need an API to access non-ISA interrupt lines.
> >
> >  So what's the clean fix here?  gsi_get_irq()?
>
> Maybe. Depends on the requirements of the testdev. If you also want to
> address PIC and IOAPIC separately or simulate injection from a specific
> device, we need more logic.

It's impossible to address them separately, the input lines are tied 
together (esp. with kvm irq routing).  I think using GSIs is the right 
thing here.

> We also need a better interface to discover and track legacy IRQ routes
> for device assignment. Markus is currently collecting requirements for
> qdev enhancements, and I think generic IRQ manipulation and discovery
> belongs there.

Possibly.  But note that attempting to shoehorn everything into 
bus/device model may not work.  Motherboard devices, especially, often 
bypass the bus/device relationship, just because everything is available 
to them on the motherboard, and because hardware designers didn't go to 
software engineering schools but instead do what's necessary to get 
things working.  We have to be prepared for exceptions.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] testdev: adjust for ISA irq changes

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1976 bytes --]

On 2011-05-29 17:26, Avi Kivity wrote:
> On 05/29/2011 06:21 PM, Jan Kiszka wrote:
>> >  Suppose our motherboard wired
>> >  the PCI links to GSI16-19 (or GSI16-23, as we once wanted before we
>> had
>> >  MSI-X)?  We'd need an API to access non-ISA interrupt lines.
>> >
>> >  So what's the clean fix here?  gsi_get_irq()?
>>
>> Maybe. Depends on the requirements of the testdev. If you also want to
>> address PIC and IOAPIC separately or simulate injection from a specific
>> device, we need more logic.
> 
> It's impossible to address them separately, the input lines are tied
> together (esp. with kvm irq routing).

That's just how user land implements it ATM. Needs fixing for chipsets
like Q35 that allow the guest to configure the routing. Fortunately, the
KVM kernel interface supports this already.

>  I think using GSIs is the right
> thing here.

It really depends on what you want to test with testdev. It's an
artificial thing anyway, so you are free to inject wherever and whatever
you want. But I guess GSI is what was the intention so far.

> 
>> We also need a better interface to discover and track legacy IRQ routes
>> for device assignment. Markus is currently collecting requirements for
>> qdev enhancements, and I think generic IRQ manipulation and discovery
>> belongs there.
> 
> Possibly.  But note that attempting to shoehorn everything into
> bus/device model may not work.  Motherboard devices, especially, often
> bypass the bus/device relationship, just because everything is available
> to them on the motherboard, and because hardware designers didn't go to
> software engineering schools but instead do what's necessary to get
> things working.  We have to be prepared for exceptions.

Yes, the IRQ tree should probably be independent of the qdev tree. I was
thinking of qdev as the infrastructure that allows to build and maintain
that tree and to associate IRQ pins with qdev devices.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [PATCH] testdev: adjust for ISA irq changes

[Markus Armbruster]

Jan Kiszka <jan.kiszka@web.de> writes:

> On 2011-05-29 17:26, Avi Kivity wrote:
>> On 05/29/2011 06:21 PM, Jan Kiszka wrote:
[...]
>>> We also need a better interface to discover and track legacy IRQ routes
>>> for device assignment. Markus is currently collecting requirements for
>>> qdev enhancements, and I think generic IRQ manipulation and discovery
>>> belongs there.
>> 
>> Possibly.  But note that attempting to shoehorn everything into
>> bus/device model may not work.  Motherboard devices, especially, often
>> bypass the bus/device relationship, just because everything is available
>> to them on the motherboard, and because hardware designers didn't go to
>> software engineering schools but instead do what's necessary to get
>> things working.  We have to be prepared for exceptions.
>
> Yes, the IRQ tree should probably be independent of the qdev tree. I was
> thinking of qdev as the infrastructure that allows to build and maintain
> that tree and to associate IRQ pins with qdev devices.

The bus/device model limits the connections between devices to a tree,
which can't model real world interrupt wirings.  We obviously need to
generalize there.