diff mbox

[07/10] qtest: IRQ interception infrastructure (v2)

Message ID 1330198969-27364-8-git-send-email-aliguori@us.ibm.com
State New
Headers show

Commit Message

Anthony Liguori Feb. 25, 2012, 7:42 p.m. UTC
From: Paolo Bonzini <pbonzini@redhat.com>

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
v1 -> v2
 - rebased to latest (aliguori)
---
 hw/irq.c     |   17 ++++++++++
 hw/irq.h     |    5 +++
 hw/pc_piix.c |    9 +++--
 qtest.c      |   97 ++++++++++++++++++++++++++++++++++++++++-----------------
 qtest.h      |    2 -
 5 files changed, 95 insertions(+), 35 deletions(-)

Comments

Paolo Bonzini Feb. 25, 2012, 8:20 p.m. UTC | #1
On 02/25/2012 08:42 PM, Anthony Liguori wrote:
> @@ -224,7 +223,9 @@ static void pc_init1(MemoryRegion *system_memory,
>          gsi_state->i8259_irq[i] = i8259[i];
>      }
>      if (pci_enabled) {
> -        ioapic_init(gsi_state);
> +        dev = ioapic_init(gsi_state);
> +        object_property_add_child(object_resolve_path("/i440fx/piix3", NULL),
> +                                  "ioapic", OBJECT(dev), NULL);
>      }
>  
>      pc_register_ferr_irq(gsi[13]);

Jan objected to putting this under /i440fx/piix3.

Paolo
Anthony Liguori Feb. 25, 2012, 9:16 p.m. UTC | #2
On 02/25/2012 02:20 PM, Paolo Bonzini wrote:
> On 02/25/2012 08:42 PM, Anthony Liguori wrote:
>> @@ -224,7 +223,9 @@ static void pc_init1(MemoryRegion *system_memory,
>>           gsi_state->i8259_irq[i] = i8259[i];
>>       }
>>       if (pci_enabled) {
>> -        ioapic_init(gsi_state);
>> +        dev = ioapic_init(gsi_state);
>> +        object_property_add_child(object_resolve_path("/i440fx/piix3", NULL),
>> +                                  "ioapic", OBJECT(dev), NULL);
>>       }
>>
>>       pc_register_ferr_irq(gsi[13]);
>
> Jan objected to putting this under /i440fx/piix3.

It's not technically part of the PIIX3 packaging, but the I/O APIC is attached 
to the PIIX3 and it's I/O requests propagate through the I/O APIC.  See section 
8.12 of the PIIX4 manual.

I think for our model, having it as a child property makes quite a lot of sense.

Regards,

Anthony Liguori

> Paolo
>
>
Andreas Färber Feb. 26, 2012, 1:31 a.m. UTC | #3
Am 25.02.2012 22:16, schrieb Anthony Liguori:
> On 02/25/2012 02:20 PM, Paolo Bonzini wrote:
>> On 02/25/2012 08:42 PM, Anthony Liguori wrote:
>>> +       
>>> object_property_add_child(object_resolve_path("/i440fx/piix3", NULL),
>>> +                                  "ioapic", OBJECT(dev), NULL);
>>
>> Jan objected to putting this under /i440fx/piix3.
> 
> I think for our model, having it as a child property makes quite a lot
> of sense.

Weren't you discussing rearranging the tree so that devices are under a
separate node from block devices etc. (e.g., /devices)? Would be a good
idea to introduce a (dummy) object_resolve_device_path() to avoid having
to change hardcoded absolute string paths like the above later.
Or do that change first so that new paths like this one at least don't
need to be touched again.

Andreas
Anthony Liguori Feb. 26, 2012, 1:34 a.m. UTC | #4
On 02/25/2012 07:31 PM, Andreas Färber wrote:
> Am 25.02.2012 22:16, schrieb Anthony Liguori:
>> On 02/25/2012 02:20 PM, Paolo Bonzini wrote:
>>> On 02/25/2012 08:42 PM, Anthony Liguori wrote:
>>>> +
>>>> object_property_add_child(object_resolve_path("/i440fx/piix3", NULL),
>>>> +                                  "ioapic", OBJECT(dev), NULL);
>>>
>>> Jan objected to putting this under /i440fx/piix3.
>>
>> I think for our model, having it as a child property makes quite a lot
>> of sense.
>
> Weren't you discussing rearranging the tree so that devices are under a
> separate node from block devices etc. (e.g., /devices)? Would be a good
> idea to introduce a (dummy) object_resolve_device_path() to avoid having
> to change hardcoded absolute string paths like the above later.

No need.  object_resolve_path("i440fx/piix3", NULL) would have the same effect.

But I also don't think it's necessary.  Direct calls to object_resolve_path 
almost always indicates the need to refactor code.

Regards,

Anthony Liguori

> Or do that change first so that new paths like this one at least don't
> need to be touched again.
>
> Andreas
>
diff mbox

Patch

diff --git a/hw/irq.c b/hw/irq.c
index 62f766e..d413a0b 100644
--- a/hw/irq.c
+++ b/hw/irq.c
@@ -104,3 +104,20 @@  qemu_irq *qemu_irq_proxy(qemu_irq **target, int n)
 {
     return qemu_allocate_irqs(proxy_irq_handler, target, n);
 }
+
+void qemu_irq_intercept_in(qemu_irq *gpio_in, qemu_irq_handler handler, int n)
+{
+    int i;
+    qemu_irq *old_irqs = qemu_allocate_irqs(NULL, NULL, n);
+    for (i = 0; i < n; i++) {
+        *old_irqs[i] = *gpio_in[i];
+        gpio_in[i]->handler = handler;
+        gpio_in[i]->opaque = old_irqs;
+    }
+}
+
+void qemu_irq_intercept_out(qemu_irq **gpio_out, qemu_irq_handler handler, int n)
+{
+    qemu_irq *old_irqs = *gpio_out;
+    *gpio_out = qemu_allocate_irqs(handler, old_irqs, n);
+}
diff --git a/hw/irq.h b/hw/irq.h
index 64da2fd..56c55f0 100644
--- a/hw/irq.h
+++ b/hw/irq.h
@@ -38,4 +38,9 @@  qemu_irq qemu_irq_split(qemu_irq irq1, qemu_irq irq2);
  */
 qemu_irq *qemu_irq_proxy(qemu_irq **target, int n);
 
+/* For internal use in qtest.  Similar to qemu_irq_split, but operating
+   on an existing vector of qemu_irq.  */
+void qemu_irq_intercept_in(qemu_irq *gpio_in, qemu_irq_handler handler, int n);
+void qemu_irq_intercept_out(qemu_irq **gpio_out, qemu_irq_handler handler, int n);
+
 #endif
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 2c0881e..36c06d5 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -99,7 +99,7 @@  static void kvm_piix3_gsi_handler(void *opaque, int n, int level)
     }
 }
 
-static void ioapic_init(GSIState *gsi_state)
+static DeviceState *ioapic_init(GSIState *gsi_state)
 {
     DeviceState *dev;
     SysBusDevice *d;
@@ -117,6 +117,7 @@  static void ioapic_init(GSIState *gsi_state)
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
         gsi_state->ioapic_irq[i] = qdev_get_gpio_in(dev, i);
     }
+    return dev;
 }
 
 /* PC hardware initialisation */
@@ -213,8 +214,6 @@  static void pc_init1(MemoryRegion *system_memory,
         i8259 = kvm_i8259_init(isa_bus);
     } else if (xen_enabled()) {
         i8259 = xen_interrupt_controller_init();
-    } else if (qtest_enabled()) {
-        i8259 = qtest_interrupt_controller_init();
     } else {
         cpu_irq = pc_allocate_cpu_irq();
         i8259 = i8259_init(isa_bus, cpu_irq[0]);
@@ -224,7 +223,9 @@  static void pc_init1(MemoryRegion *system_memory,
         gsi_state->i8259_irq[i] = i8259[i];
     }
     if (pci_enabled) {
-        ioapic_init(gsi_state);
+        dev = ioapic_init(gsi_state);
+        object_property_add_child(object_resolve_path("/i440fx/piix3", NULL),
+                                  "ioapic", OBJECT(dev), NULL);
     }
 
     pc_register_ferr_irq(gsi[13]);
diff --git a/qtest.c b/qtest.c
index c2fbf50..a1eca49 100644
--- a/qtest.c
+++ b/qtest.c
@@ -12,6 +12,7 @@ 
  */
 
 #include "qtest.h"
+#include "hw/qdev.h"
 #include "qemu-char.h"
 #include "ioport.h"
 #include "memory.h"
@@ -24,6 +25,7 @@  const char *qtest_chrdev;
 const char *qtest_log;
 int qtest_allowed = 0;
 
+static DeviceState *irq_intercept_dev;
 static FILE *qtest_log_fp;
 static CharDriverState *qtest_chr;
 static GString *inbuf;
@@ -66,18 +68,30 @@  static bool qtest_opened;
  *  > write ADDR SIZE DATA
  *  < OK
  *
- * Valid async messages:
- *
- *  IRQ raise NUM
- *  IRQ lower NUM
- *
  * ADDR, SIZE, VALUE are all integers parsed with strtoul() with a base of 0.
  *
  * DATA is an arbitrarily long hex number prefixed with '0x'.  If it's smaller
  * than the expected size, the value will be zero filled at the end of the data
  * sequence.
  *
- * NUM is an IRQ number.
+ * IRQ management:
+ *
+ *  > irq_intercept_in QOM-PATH
+ *  < OK
+ *
+ *  > irq_intercept_out QOM-PATH
+ *  < OK
+ *
+ * Attach to the gpio-in (resp. gpio-out) pins exported by the device at
+ * QOM-PATH.  When the pin is triggered, one of the following async messages
+ * will be printed to the qtest stream:
+ *
+ *  IRQ raise NUM
+ *  IRQ lower NUM
+ *
+ * where NUM is an IRQ number.  For the PC, interrupts can be intercepted
+ * simply with "irq_intercept_in ioapic" (note that IRQ0 comes out with
+ * NUM=0 even though it is remapped to GSI 2).
  */
 
 static int hex2nib(char ch)
@@ -133,6 +147,20 @@  static void qtest_send(CharDriverState *chr, const char *fmt, ...)
     }
 }
 
+static void qtest_irq_handler(void *opaque, int n, int level)
+{
+    qemu_irq *old_irqs = opaque;
+    qemu_set_irq(old_irqs[n], level);
+
+    if (irq_levels[n] != level) {
+        CharDriverState *chr = qtest_chr;
+        irq_levels[n] = level;
+        qtest_send_prefix(chr);
+        qtest_send(chr, "IRQ %s %d\n",
+                   level ? "raise" : "lower", n);
+    }
+}
+
 static void qtest_process_command(CharDriverState *chr, gchar **words)
 {
     const gchar *command;
@@ -155,9 +183,40 @@  static void qtest_process_command(CharDriverState *chr, gchar **words)
     }
 
     g_assert(command);
-    if (strcmp(words[0], "outb") == 0 ||
-        strcmp(words[0], "outw") == 0 ||
-        strcmp(words[0], "outl") == 0) {
+    if (strcmp(words[0], "irq_intercept_out") == 0
+        || strcmp(words[0], "irq_intercept_in") == 0) {
+	DeviceState *dev;
+
+        g_assert(words[1]);
+        dev = DEVICE(object_resolve_path(words[1], NULL));
+        if (!dev) {
+            qtest_send_prefix(chr);
+            qtest_send(chr, "FAIL Unknown device\n");
+	    return;
+        }
+
+        if (irq_intercept_dev) {
+            qtest_send_prefix(chr);
+            if (irq_intercept_dev != dev) {
+                qtest_send(chr, "FAIL IRQ intercept already enabled\n");
+            } else {
+                qtest_send(chr, "OK\n");
+            }
+	    return;
+        }
+
+        if (words[0][14] == 'o') {
+            qemu_irq_intercept_out(&dev->gpio_out, qtest_irq_handler, dev->num_gpio_out);
+        } else {
+            qemu_irq_intercept_in(dev->gpio_in, qtest_irq_handler, dev->num_gpio_in);
+        }
+        irq_intercept_dev = dev;
+        qtest_send_prefix(chr);
+        qtest_send(chr, "OK\n");
+
+    } else if (strcmp(words[0], "outb") == 0 ||
+               strcmp(words[0], "outw") == 0 ||
+               strcmp(words[0], "outl") == 0) {
         uint16_t addr;
         uint32_t value;
 
@@ -312,26 +371,6 @@  static void qtest_event(void *opaque, int event)
     }
 }
 
-static void qtest_set_irq(void *opaque, int irq, int level)
-{
-    CharDriverState *chr = qtest_chr;
-    bool changed;
-
-    changed = (irq_levels[irq] != level);
-    irq_levels[irq] = level;
-
-    if (changed) {
-        qtest_send_prefix(chr);
-        qtest_send(chr, "IRQ %s %d\n",
-                   level ? "raise" : "lower", irq);
-    }
-}
-
-qemu_irq *qtest_interrupt_controller_init(void)
-{
-    return qemu_allocate_irqs(qtest_set_irq, NULL, MAX_IRQ);
-}
-
 int qtest_init(void)
 {
     CharDriverState *chr;
diff --git a/qtest.h b/qtest.h
index f0e1377..1478343 100644
--- a/qtest.h
+++ b/qtest.h
@@ -32,6 +32,4 @@  static inline int qtest_available(void)
 
 int qtest_init(void);
 
-qemu_irq *qtest_interrupt_controller_init(void);
-
 #endif