[Qemu-devel] [PATCH 00/22] Rework i8259 and PC interrupt models

[Qemu-devel] [PATCH 00/22] Rework i8259 and PC interrupt models

[Jan Kiszka]

Highlights of this series:
 - generic i8259, now part of hwlib
 - qdev conversion of i8259
 - fix for i8259 poll mode (and removal of PREP hack)

The refactoring will also be important to instantiate i8259-kvm devices
for in-kernel irqchip acceleration one day.

Note: depends on "mips_fulong2e: Reorder ISA bus and i8259 creation"

CC: Alexander Graf <agraf@suse.de>

Jan Kiszka (22):
  pc: Drop useless test from isa_irq_handler
  pc: Generalize ISA IRQs to GSIs
  pc: Convert GSIState::i8259_irq into array
  pc: Fix and clean up PIC-to-APIC IRQ path
  i8259: Remove premature inline function attributes
  i8259: Drop obsolete prototypes
  i8259: Move pic_set_irq1 after pic_update_irq
  i8239: Introduce per-PIC output interrupt
  i8259: Do not update IRQ output after spurious pic_poll_read
  i8259: Reorder intack in pic_read_irq
  i8259: Update IRQ state after reset
  i8259: Switch to per-PIC IRQ update
  i8259: Fix poll command
  i8259: Clean up pic_ioport_read
  i8259: PREP: Replace pic_intack_read with pic_read_irq
  i8259: Replace PicState::pics_state with master flag
  i8259: Eliminate PicState2
  qdev: Add HEX8 property
  i8259: Convert to qdev
  i8259: Fix coding style
  monitor: Restrict pic/irq_info to supporting targets
  i8259: Move to hw library

 Makefile.objs           |    2 +-
 Makefile.target         |    8 +-
 hw/an5206.c             |   10 --
 hw/apic.c               |    4 +
 hw/arm_pic.c            |   11 --
 hw/cris_pic_cpu.c       |    6 -
 hw/etraxfs.h            |    1 +
 hw/i8259.c              |  387 ++++++++++++++++++++++++-----------------------
 hw/ioapic.h             |    7 +
 hw/isa.h                |    2 +
 hw/lm32_pic.c           |    4 +-
 hw/lm32_pic.h           |    3 +
 hw/microblaze_pic_cpu.c |    6 -
 hw/pc.c                 |   24 ++--
 hw/pc.h                 |   29 ++--
 hw/pc_piix.c            |   30 ++--
 hw/ppc_prep.c           |    2 +-
 hw/qdev-properties.c    |   29 ++++
 hw/qdev.h               |    3 +
 hw/s390-virtio.c        |   11 --
 hw/shix.c               |   11 --
 hw/slavio_intctl.c      |   14 ++-
 hw/sun4m.c              |   16 +--
 hw/sun4m.h              |    6 +-
 hw/sun4u.c              |    8 -
 hw/xtensa_pic.c         |   10 --
 monitor.c               |   21 +++
 27 files changed, 331 insertions(+), 334 deletions(-)

-- 
1.7.3.4

[Qemu-devel] [PATCH 01/22] pc: Drop useless test from isa_irq_handler

[Jan Kiszka]

IsaIrqState::ioapic is always non-NULL. Probably, the concrete
qemu_irq was supposed to be tested, but that's already done by
qemu_set_irq.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/pc.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 203627d..a15d165 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -96,9 +96,8 @@ void isa_irq_handler(void *opaque, int n, int level)
     if (n < 16) {
         qemu_set_irq(isa->i8259[n], level);
     }
-    if (isa->ioapic)
-        qemu_set_irq(isa->ioapic[n], level);
-};
+    qemu_set_irq(isa->ioapic[n], level);
+}
 
 static void ioport80_write(void *opaque, uint32_t addr, uint32_t data)
 {
-- 
1.7.3.4

[Qemu-devel] [PATCH 02/22] pc: Generalize ISA IRQs to GSIs

[Jan Kiszka]

The ISA bus IRQ range is 0..15. What isa_irq_handler and IsaIrqState are
actually dealing with are the Global System Interrupts. Refactor the
code to clarify this.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/ioapic.h  |    7 +++++++
 hw/isa.h     |    2 ++
 hw/pc.c      |   18 +++++++++---------
 hw/pc.h      |   18 ++++++++++--------
 hw/pc_piix.c |   28 ++++++++++++++--------------
 5 files changed, 42 insertions(+), 31 deletions(-)

diff --git a/hw/ioapic.h b/hw/ioapic.h
index cb2642a..86e63da 100644
--- a/hw/ioapic.h
+++ b/hw/ioapic.h
@@ -17,4 +17,11 @@
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
 
+#ifndef HW_IOAPIC_H
+#define HW_IOAPIC_H
+
+#define IOAPIC_NUM_PINS 24
+
 void ioapic_eoi_broadcast(int vector);
+
+#endif /* !HW_IOAPIC_H */
diff --git a/hw/isa.h b/hw/isa.h
index 432d17a..820c390 100644
--- a/hw/isa.h
+++ b/hw/isa.h
@@ -7,6 +7,8 @@
 #include "memory.h"
 #include "qdev.h"
 
+#define ISA_NUM_IRQS 16
+
 typedef struct ISABus ISABus;
 typedef struct ISADevice ISADevice;
 typedef struct ISADeviceInfo ISADeviceInfo;
diff --git a/hw/pc.c b/hw/pc.c
index a15d165..c979d4b 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -88,15 +88,15 @@ struct e820_table {
 static struct e820_table e820_table;
 struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
 
-void isa_irq_handler(void *opaque, int n, int level)
+void gsi_handler(void *opaque, int n, int level)
 {
-    IsaIrqState *isa = (IsaIrqState *)opaque;
+    GSIState *s = opaque;
 
-    DPRINTF("isa_irqs: %s irq %d\n", level? "raise" : "lower", n);
-    if (n < 16) {
-        qemu_set_irq(isa->i8259[n], level);
+    DPRINTF("pc: %s GSI %d\n", level ? "raising" : "lowering", n);
+    if (n < ISA_NUM_IRQS) {
+        qemu_set_irq(s->i8259_irq[n], level);
     }
-    qemu_set_irq(isa->ioapic[n], level);
+    qemu_set_irq(s->ioapic_irq[n], level);
 }
 
 static void ioport80_write(void *opaque, uint32_t addr, uint32_t data)
@@ -1115,7 +1115,7 @@ static void cpu_request_exit(void *opaque, int irq, int level)
     }
 }
 
-void pc_basic_device_init(qemu_irq *isa_irq,
+void pc_basic_device_init(qemu_irq *gsi,
                           ISADevice **rtc_state,
                           bool no_vmport)
 {
@@ -1134,8 +1134,8 @@ void pc_basic_device_init(qemu_irq *isa_irq,
         DeviceState *hpet = sysbus_try_create_simple("hpet", HPET_BASE, NULL);
 
         if (hpet) {
-            for (i = 0; i < 24; i++) {
-                sysbus_connect_irq(sysbus_from_qdev(hpet), i, isa_irq[i]);
+            for (i = 0; i < GSI_NUM_PINS; i++) {
+                sysbus_connect_irq(sysbus_from_qdev(hpet), i, gsi[i]);
             }
             rtc_irq = qdev_get_gpio_in(hpet, 0);
         }
diff --git a/hw/pc.h b/hw/pc.h
index 7e6ddba..4333898 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -8,6 +8,7 @@
 #include "fdc.h"
 #include "net.h"
 #include "memory.h"
+#include "ioapic.h"
 
 /* PC-style peripherals (also used by other machines).  */
 
@@ -70,15 +71,16 @@ uint32_t pic_intack_read(PicState2 *s);
 void pic_info(Monitor *mon);
 void irq_info(Monitor *mon);
 
-/* ISA */
-#define IOAPIC_NUM_PINS 0x18
+/* Global System Interrupts */
 
-typedef struct isa_irq_state {
-    qemu_irq *i8259;
-    qemu_irq ioapic[IOAPIC_NUM_PINS];
-} IsaIrqState;
+#define GSI_NUM_PINS IOAPIC_NUM_PINS
 
-void isa_irq_handler(void *opaque, int n, int level);
+typedef struct GSIState {
+    qemu_irq *i8259_irq;
+    qemu_irq ioapic_irq[IOAPIC_NUM_PINS];
+} GSIState;
+
+void gsi_handler(void *opaque, int n, int level);
 
 /* i8254.c */
 
@@ -141,7 +143,7 @@ void pc_memory_init(MemoryRegion *system_memory,
                     MemoryRegion **ram_memory);
 qemu_irq *pc_allocate_cpu_irq(void);
 void pc_vga_init(PCIBus *pci_bus);
-void pc_basic_device_init(qemu_irq *isa_irq,
+void pc_basic_device_init(qemu_irq *gsi,
                           ISADevice **rtc_state,
                           bool no_vmport);
 void pc_init_ne2k_isa(NICInfo *nd);
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index ce1c87f..e6e280c 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -53,7 +53,7 @@ static const int ide_iobase[MAX_IDE_BUS] = { 0x1f0, 0x170 };
 static const int ide_iobase2[MAX_IDE_BUS] = { 0x3f6, 0x376 };
 static const int ide_irq[MAX_IDE_BUS] = { 14, 15 };
 
-static void ioapic_init(IsaIrqState *isa_irq_state)
+static void ioapic_init(GSIState *gsi_state)
 {
     DeviceState *dev;
     SysBusDevice *d;
@@ -65,7 +65,7 @@ static void ioapic_init(IsaIrqState *isa_irq_state)
     sysbus_mmio_map(d, 0, 0xfec00000);
 
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
-        isa_irq_state->ioapic[i] = qdev_get_gpio_in(dev, i);
+        gsi_state->ioapic_irq[i] = qdev_get_gpio_in(dev, i);
     }
 }
 
@@ -87,11 +87,11 @@ static void pc_init1(MemoryRegion *system_memory,
     PCII440FXState *i440fx_state;
     int piix3_devfn = -1;
     qemu_irq *cpu_irq;
-    qemu_irq *isa_irq;
+    qemu_irq *gsi;
     qemu_irq *i8259;
     qemu_irq *cmos_s3;
     qemu_irq *smi_irq;
-    IsaIrqState *isa_irq_state;
+    GSIState *gsi_state;
     DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
     BusState *idebus[MAX_IDE_BUS];
     ISADevice *rtc_state;
@@ -130,11 +130,11 @@ static void pc_init1(MemoryRegion *system_memory,
                        pci_enabled ? rom_memory : system_memory, &ram_memory);
     }
 
-    isa_irq_state = g_malloc0(sizeof(*isa_irq_state));
-    isa_irq = qemu_allocate_irqs(isa_irq_handler, isa_irq_state, 24);
+    gsi_state = g_malloc0(sizeof(*gsi_state));
+    gsi = qemu_allocate_irqs(gsi_handler, gsi_state, GSI_NUM_PINS);
 
     if (pci_enabled) {
-        pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, isa_irq,
+        pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, gsi,
                               system_memory, system_io, ram_size,
                               below_4g_mem_size,
                               0x100000000ULL - below_4g_mem_size,
@@ -149,7 +149,7 @@ static void pc_init1(MemoryRegion *system_memory,
         isa_bus_new(NULL, system_io);
         no_hpet = 1;
     }
-    isa_bus_irqs(isa_irq);
+    isa_bus_irqs(gsi);
 
     if (!xen_enabled()) {
         cpu_irq = pc_allocate_cpu_irq();
@@ -158,12 +158,12 @@ static void pc_init1(MemoryRegion *system_memory,
         i8259 = xen_interrupt_controller_init();
     }
 
-    isa_irq_state->i8259 = i8259;
+    gsi_state->i8259_irq = i8259;
     if (pci_enabled) {
-        ioapic_init(isa_irq_state);
+        ioapic_init(gsi_state);
     }
 
-    pc_register_ferr_irq(isa_get_irq(13));
+    pc_register_ferr_irq(gsi[13]);
 
     pc_vga_init(pci_enabled? pci_bus: NULL);
 
@@ -172,7 +172,7 @@ static void pc_init1(MemoryRegion *system_memory,
     }
 
     /* init basic PC hardware */
-    pc_basic_device_init(isa_irq, &rtc_state, xen_enabled());
+    pc_basic_device_init(gsi, &rtc_state, xen_enabled());
 
     for(i = 0; i < nb_nics; i++) {
         NICInfo *nd = &nd_table[i];
@@ -202,7 +202,7 @@ static void pc_init1(MemoryRegion *system_memory,
         }
     }
 
-    audio_init(isa_irq, pci_enabled ? pci_bus : NULL);
+    audio_init(gsi, pci_enabled ? pci_bus : NULL);
 
     pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device,
                  idebus[0], idebus[1], rtc_state);
@@ -222,7 +222,7 @@ static void pc_init1(MemoryRegion *system_memory,
         smi_irq = qemu_allocate_irqs(pc_acpi_smi_interrupt, first_cpu, 1);
         /* TODO: Populate SPD eeprom data.  */
         smbus = piix4_pm_init(pci_bus, piix3_devfn + 3, 0xb100,
-                              isa_get_irq(9), *cmos_s3, *smi_irq,
+                              gsi[9], *cmos_s3, *smi_irq,
                               kvm_enabled());
         smbus_eeprom_init(smbus, 8, NULL, 0);
     }
-- 
1.7.3.4

[Qemu-devel] [PATCH 03/22] pc: Convert GSIState::i8259_irq into array

[Jan Kiszka]

Will be required when we no longer let i8259_init allocate the PIC IRQs
but convert that chips to qdev.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/pc.h      |    2 +-
 hw/pc_piix.c |    4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/pc.h b/hw/pc.h
index 4333898..2870be4 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -76,7 +76,7 @@ void irq_info(Monitor *mon);
 #define GSI_NUM_PINS IOAPIC_NUM_PINS
 
 typedef struct GSIState {
-    qemu_irq *i8259_irq;
+    qemu_irq i8259_irq[ISA_NUM_IRQS];
     qemu_irq ioapic_irq[IOAPIC_NUM_PINS];
 } GSIState;
 
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index e6e280c..c89042f 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -158,7 +158,9 @@ static void pc_init1(MemoryRegion *system_memory,
         i8259 = xen_interrupt_controller_init();
     }
 
-    gsi_state->i8259_irq = i8259;
+    for (i = 0; i < ISA_NUM_IRQS; i++) {
+        gsi_state->i8259_irq[i] = i8259[i];
+    }
     if (pci_enabled) {
         ioapic_init(gsi_state);
     }
-- 
1.7.3.4

[Qemu-devel] [PATCH 04/22] pc: Fix and clean up PIC-to-APIC IRQ path

[Jan Kiszka]

The master PIC is connected to the LINTIN0 of the APICs. As the APIC
currently does not track the state of that line, we have to ask the PIC
to reinject its IRQ after the CPU picked up an event from the APIC.

This introduces pic_get_output to read the master PIC IRQ line state
without changing it. The APIC uses this function to decide if a PIC IRQ
should be reinjected on apic_update_irq. This reflects better how the
real hardware works.

The patch fixes some failures of the kvm unit tests apic and eventinj by
allowing to enable the proper CPU IRQ deassertion when the guest masks
some pending IRQs at PIC level.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/apic.c  |    4 ++++
 hw/i8259.c |   15 +++++++--------
 hw/pc.c    |    3 ---
 hw/pc.h    |    2 +-
 4 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/hw/apic.c b/hw/apic.c
index d8f56c8..8289eef 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -23,6 +23,7 @@
 #include "host-utils.h"
 #include "sysbus.h"
 #include "trace.h"
+#include "pc.h"
 
 /* APIC Local Vector Table */
 #define APIC_LVT_TIMER   0
@@ -399,6 +400,9 @@ static void apic_update_irq(APICState *s)
     }
     if (apic_irq_pending(s) > 0) {
         cpu_interrupt(s->cpu_env, CPU_INTERRUPT_HARD);
+    } else if (apic_accept_pic_intr(&s->busdev.qdev) &&
+               pic_get_output(isa_pic)) {
+        apic_deliver_pic_intr(&s->busdev.qdev, 1);
     }
 }
 
diff --git a/hw/i8259.c b/hw/i8259.c
index e5323ff..6006123 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -146,8 +146,7 @@ static int pic_get_irq(PicState *s)
 
 /* raise irq to CPU if necessary. must be called every time the active
    irq may change */
-/* XXX: should not export it, but it is needed for an APIC kludge */
-void pic_update_irq(PicState2 *s)
+static void pic_update_irq(PicState2 *s)
 {
     int irq2, irq;
 
@@ -174,14 +173,9 @@ void pic_update_irq(PicState2 *s)
         printf("pic: cpu_interrupt\n");
 #endif
         qemu_irq_raise(s->parent_irq);
-    }
-
-/* all targets should do this rather than acking the IRQ in the cpu */
-#if defined(TARGET_MIPS) || defined(TARGET_PPC) || defined(TARGET_ALPHA)
-    else {
+    } else {
         qemu_irq_lower(s->parent_irq);
     }
-#endif
 }
 
 #ifdef DEBUG_IRQ_LATENCY
@@ -441,6 +435,11 @@ uint32_t pic_intack_read(PicState2 *s)
     return ret;
 }
 
+int pic_get_output(PicState2 *s)
+{
+    return (pic_get_irq(&s->pics[0]) >= 0);
+}
+
 static void elcr_ioport_write(void *opaque, target_phys_addr_t addr,
                               uint64_t val, unsigned size)
 {
diff --git a/hw/pc.c b/hw/pc.c
index c979d4b..ff2111c 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -155,9 +155,6 @@ int cpu_get_pic_interrupt(CPUState *env)
 
     intno = apic_get_interrupt(env->apic_state);
     if (intno >= 0) {
-        /* set irq request if a PIC irq is still pending */
-        /* XXX: improve that */
-        pic_update_irq(isa_pic);
         return intno;
     }
     /* read the irq from the PIC */
diff --git a/hw/pc.h b/hw/pc.h
index 2870be4..60da282 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -66,7 +66,7 @@ void pic_set_irq(int irq, int level);
 void pic_set_irq_new(void *opaque, int irq, int level);
 qemu_irq *i8259_init(qemu_irq parent_irq);
 int pic_read_irq(PicState2 *s);
-void pic_update_irq(PicState2 *s);
+int pic_get_output(PicState2 *s);
 uint32_t pic_intack_read(PicState2 *s);
 void pic_info(Monitor *mon);
 void irq_info(Monitor *mon);
-- 
1.7.3.4

[Qemu-devel] [PATCH 05/22] i8259: Remove premature inline function attributes

[Jan Kiszka]

The compiler is smarter in choosing the right optimization.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/i8259.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/i8259.c b/hw/i8259.c
index 6006123..f1d58ba 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -80,7 +80,7 @@ static uint64_t irq_count[16];
 PicState2 *isa_pic;
 
 /* set irq level. If an edge is detected, then the IRR is set to 1 */
-static inline void pic_set_irq1(PicState *s, int irq, int level)
+static void pic_set_irq1(PicState *s, int irq, int level)
 {
     int mask;
     mask = 1 << irq;
@@ -107,7 +107,7 @@ static inline void pic_set_irq1(PicState *s, int irq, int level)
 
 /* return the highest priority found in mask (highest = smallest
    number). Return 8 if no irq */
-static inline int get_priority(PicState *s, int mask)
+static int get_priority(PicState *s, int mask)
 {
     int priority;
     if (mask == 0)
@@ -206,7 +206,7 @@ static void i8259_set_irq(void *opaque, int irq, int level)
 }
 
 /* acknowledge interrupt 'irq' */
-static inline void pic_intack(PicState *s, int irq)
+static void pic_intack(PicState *s, int irq)
 {
     if (s->auto_eoi) {
         if (s->rotate_on_auto_eoi)
-- 
1.7.3.4

[Qemu-devel] [PATCH 06/22] i8259: Drop obsolete prototypes

[Jan Kiszka]

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/pc.h |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/hw/pc.h b/hw/pc.h
index 60da282..fd5f9b2 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -62,8 +62,6 @@ bool parallel_mm_init(target_phys_addr_t base, int it_shift, qemu_irq irq,
 
 typedef struct PicState2 PicState2;
 extern PicState2 *isa_pic;
-void pic_set_irq(int irq, int level);
-void pic_set_irq_new(void *opaque, int irq, int level);
 qemu_irq *i8259_init(qemu_irq parent_irq);
 int pic_read_irq(PicState2 *s);
 int pic_get_output(PicState2 *s);
-- 
1.7.3.4

[Qemu-devel] [PATCH 07/22] i8259: Move pic_set_irq1 after pic_update_irq

[Jan Kiszka]

We are about to call the latter from the former. No functional changes.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/i8259.c |   55 +++++++++++++++++++++++++++++--------------------------
 1 files changed, 29 insertions(+), 26 deletions(-)

diff --git a/hw/i8259.c b/hw/i8259.c
index f1d58ba..de2d5ca 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -79,32 +79,6 @@ static uint64_t irq_count[16];
 #endif
 PicState2 *isa_pic;
 
-/* set irq level. If an edge is detected, then the IRR is set to 1 */
-static void pic_set_irq1(PicState *s, int irq, int level)
-{
-    int mask;
-    mask = 1 << irq;
-    if (s->elcr & mask) {
-        /* level triggered */
-        if (level) {
-            s->irr |= mask;
-            s->last_irr |= mask;
-        } else {
-            s->irr &= ~mask;
-            s->last_irr &= ~mask;
-        }
-    } else {
-        /* edge triggered */
-        if (level) {
-            if ((s->last_irr & mask) == 0)
-                s->irr |= mask;
-            s->last_irr |= mask;
-        } else {
-            s->last_irr &= ~mask;
-        }
-    }
-}
-
 /* return the highest priority found in mask (highest = smallest
    number). Return 8 if no irq */
 static int get_priority(PicState *s, int mask)
@@ -144,6 +118,8 @@ static int pic_get_irq(PicState *s)
     }
 }
 
+static void pic_set_irq1(PicState *s, int irq, int level);
+
 /* raise irq to CPU if necessary. must be called every time the active
    irq may change */
 static void pic_update_irq(PicState2 *s)
@@ -178,6 +154,33 @@ static void pic_update_irq(PicState2 *s)
     }
 }
 
+/* set irq level. If an edge is detected, then the IRR is set to 1 */
+static void pic_set_irq1(PicState *s, int irq, int level)
+{
+    int mask;
+    mask = 1 << irq;
+    if (s->elcr & mask) {
+        /* level triggered */
+        if (level) {
+            s->irr |= mask;
+            s->last_irr |= mask;
+        } else {
+            s->irr &= ~mask;
+            s->last_irr &= ~mask;
+        }
+    } else {
+        /* edge triggered */
+        if (level) {
+            if ((s->last_irr & mask) == 0) {
+                s->irr |= mask;
+            }
+            s->last_irr |= mask;
+        } else {
+            s->last_irr &= ~mask;
+        }
+    }
+}
+
 #ifdef DEBUG_IRQ_LATENCY
 int64_t irq_time[16];
 #endif
-- 
1.7.3.4

[Qemu-devel] [PATCH 08/22] i8239: Introduce per-PIC output interrupt

[Jan Kiszka]

As a first step towards more generic master-slave support, remove
parent_irq in favor of a per-PIC output interrupt line. The slave's
line is attached to IRQ2 of the master, but it remains unused for now.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/i8259.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/hw/i8259.c b/hw/i8259.c
index de2d5ca..65123bd 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -58,6 +58,7 @@ typedef struct PicState {
     uint8_t single_mode; /* true if slave pic is not initialized */
     uint8_t elcr; /* PIIX edge/trigger selection*/
     uint8_t elcr_mask;
+    qemu_irq int_out;
     PicState2 *pics_state;
     MemoryRegion base_io;
     MemoryRegion elcr_io;
@@ -67,7 +68,6 @@ struct PicState2 {
     /* 0 is master pic, 1 is slave pic */
     /* XXX: better separation between the two pics */
     PicState pics[2];
-    qemu_irq parent_irq;
     void *irq_request_opaque;
 };
 
@@ -148,9 +148,9 @@ static void pic_update_irq(PicState2 *s)
         }
         printf("pic: cpu_interrupt\n");
 #endif
-        qemu_irq_raise(s->parent_irq);
+        qemu_irq_raise(s->pics[0].int_out);
     } else {
-        qemu_irq_lower(s->parent_irq);
+        qemu_irq_lower(s->pics[0].int_out);
     }
 }
 
@@ -297,7 +297,7 @@ static void pic_ioport_write(void *opaque, target_phys_addr_t addr64,
             /* init */
             pic_reset(s);
             /* deassert a pending interrupt */
-            qemu_irq_lower(s->pics_state->parent_irq);
+            qemu_irq_lower(s->pics_state->pics[0].int_out);
             s->init_state = 1;
             s->init4 = val & 1;
             s->single_mode = val & 2;
@@ -502,8 +502,10 @@ static const MemoryRegionOps pic_elcr_ioport_ops = {
 };
 
 /* XXX: add generic master/slave system */
-static void pic_init1(int io_addr, int elcr_addr, PicState *s)
+static void pic_init(int io_addr, int elcr_addr, PicState *s, qemu_irq int_out)
 {
+    s->int_out = int_out;
+
     memory_region_init_io(&s->base_io, &pic_base_ioport_ops, s, "pic", 2);
     memory_region_init_io(&s->elcr_io, &pic_elcr_ioport_ops, s, "elcr", 1);
 
@@ -553,16 +555,17 @@ void irq_info(Monitor *mon)
 
 qemu_irq *i8259_init(qemu_irq parent_irq)
 {
+    qemu_irq *irqs;
     PicState2 *s;
 
     s = g_malloc0(sizeof(PicState2));
-    pic_init1(0x20, 0x4d0, &s->pics[0]);
-    pic_init1(0xa0, 0x4d1, &s->pics[1]);
+    irqs = qemu_allocate_irqs(i8259_set_irq, s, 16);
+    pic_init(0x20, 0x4d0, &s->pics[0], parent_irq);
+    pic_init(0xa0, 0x4d1, &s->pics[1], irqs[2]);
     s->pics[0].elcr_mask = 0xf8;
     s->pics[1].elcr_mask = 0xde;
-    s->parent_irq = parent_irq;
     s->pics[0].pics_state = s;
     s->pics[1].pics_state = s;
     isa_pic = s;
-    return qemu_allocate_irqs(i8259_set_irq, s, 16);
+    return irqs;
 }
-- 
1.7.3.4

[Qemu-devel] [PATCH 09/22] i8259: Do not update IRQ output after spurious pic_poll_read

[Jan Kiszka]

If pic_poll_read finds no pending IRQ and return a spurious one instead,
no PIC state is changed, thus we do not need to call pic_update_irq.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/i8259.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/hw/i8259.c b/hw/i8259.c
index 65123bd..cddd3c7 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -393,7 +393,6 @@ static uint32_t pic_poll_read(PicState *s)
             pic_update_irq(s->pics_state);
     } else {
         ret = 0x07;
-        pic_update_irq(s->pics_state);
     }
 
     return ret;
-- 
1.7.3.4

[Qemu-devel] [PATCH 10/22] i8259: Reorder intack in pic_read_irq

[Jan Kiszka]

As we want to move the IRQ update to pic_intack, ordering matters: the
slave ack must be executed before the master ack to avoid missing
further pending slave IRQs.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/i8259.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/i8259.c b/hw/i8259.c
index cddd3c7..b7a011f 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -228,7 +228,6 @@ int pic_read_irq(PicState2 *s)
 
     irq = pic_get_irq(&s->pics[0]);
     if (irq >= 0) {
-        pic_intack(&s->pics[0], irq);
         if (irq == 2) {
             irq2 = pic_get_irq(&s->pics[1]);
             if (irq2 >= 0) {
@@ -238,12 +237,10 @@ int pic_read_irq(PicState2 *s)
                 irq2 = 7;
             }
             intno = s->pics[1].irq_base + irq2;
-#if defined(DEBUG_PIC) || defined(DEBUG_IRQ_LATENCY)
-            irq = irq2 + 8;
-#endif
         } else {
             intno = s->pics[0].irq_base + irq;
         }
+        pic_intack(&s->pics[0], irq);
     } else {
         /* spurious IRQ on host controller */
         irq = 7;
@@ -251,6 +248,11 @@ int pic_read_irq(PicState2 *s)
     }
     pic_update_irq(s);
 
+#if defined(DEBUG_PIC) || defined(DEBUG_IRQ_LATENCY)
+    if (irq == 2) {
+        irq = irq2 + 8;
+    }
+#endif
 #ifdef DEBUG_IRQ_LATENCY
     printf("IRQ%d latency=%0.3fus\n",
            irq,
-- 
1.7.3.4

[Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Jan Kiszka]

As we clearly modify the PIC state on pic_reset, we also have to update
the IRQ output. This only happened on init so far. Apply this
consistently.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/i8259.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/hw/i8259.c b/hw/i8259.c
index b7a011f..3498c6b 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -283,6 +283,7 @@ static void pic_reset(void *opaque)
     s->init4 = 0;
     s->single_mode = 0;
     /* Note: ELCR is not reset */
+    pic_update_irq(s->pics_state);
 }
 
 static void pic_ioport_write(void *opaque, target_phys_addr_t addr64,
@@ -298,8 +299,6 @@ static void pic_ioport_write(void *opaque, target_phys_addr_t addr64,
         if (val & 0x10) {
             /* init */
             pic_reset(s);
-            /* deassert a pending interrupt */
-            qemu_irq_lower(s->pics_state->pics[0].int_out);
             s->init_state = 1;
             s->init4 = val & 1;
             s->single_mode = val & 2;
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Blue Swirl]

On Wed, Sep 28, 2011 at 11:00 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> As we clearly modify the PIC state on pic_reset, we also have to update
> the IRQ output. This only happened on init so far. Apply this
> consistently.

Nack, IRQ lines shouldn't be touched on reset. The other side may not
be ready for receiving the interrupt change and qemu_irqs are
stateless anyway.

> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  hw/i8259.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/hw/i8259.c b/hw/i8259.c
> index b7a011f..3498c6b 100644
> --- a/hw/i8259.c
> +++ b/hw/i8259.c
> @@ -283,6 +283,7 @@ static void pic_reset(void *opaque)
>     s->init4 = 0;
>     s->single_mode = 0;
>     /* Note: ELCR is not reset */
> +    pic_update_irq(s->pics_state);
>  }
>
>  static void pic_ioport_write(void *opaque, target_phys_addr_t addr64,
> @@ -298,8 +299,6 @@ static void pic_ioport_write(void *opaque, target_phys_addr_t addr64,
>         if (val & 0x10) {
>             /* init */
>             pic_reset(s);
> -            /* deassert a pending interrupt */
> -            qemu_irq_lower(s->pics_state->pics[0].int_out);
>             s->init_state = 1;
>             s->init4 = val & 1;
>             s->single_mode = val & 2;
> --
> 1.7.3.4
>
>

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Peter Maydell]

On 28 September 2011 19:01, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Wed, Sep 28, 2011 at 11:00 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> As we clearly modify the PIC state on pic_reset, we also have to update
>> the IRQ output. This only happened on init so far. Apply this
>> consistently.
>
> Nack, IRQ lines shouldn't be touched on reset. The other side may not
> be ready for receiving the interrupt change and qemu_irqs are
> stateless anyway.

So how does this work for a device whose reset state is "this
irq/gpio line is asserted" ?

-- PMM

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Blue Swirl]

On Wed, Sep 28, 2011 at 6:09 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 28 September 2011 19:01, Blue Swirl <blauwirbel@gmail.com> wrote:
>> On Wed, Sep 28, 2011 at 11:00 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> As we clearly modify the PIC state on pic_reset, we also have to update
>>> the IRQ output. This only happened on init so far. Apply this
>>> consistently.
>>
>> Nack, IRQ lines shouldn't be touched on reset. The other side may not
>> be ready for receiving the interrupt change and qemu_irqs are
>> stateless anyway.
>
> So how does this work for a device whose reset state is "this
> irq/gpio line is asserted" ?

The polarity could be reversed but that wouldn't work if the signal is
used in several places.

But the duration of reset in QEMU is infinitely small, nothing is
expected to happen during that. This doesn't match real HW though, the
reset could take several hundred milliseconds but I doubt modeling
that would be interesting.

We also assume that the reset states of devices and their outputs are
coherent i.e. the output of one device during reset would not change
the state of another device from its reset state.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Peter Maydell]

On 28 September 2011 19:42, Blue Swirl <blauwirbel@gmail.com> wrote:
> We also assume that the reset states of devices and their outputs are
> coherent i.e. the output of one device during reset would not change
> the state of another device from its reset state.

This seems a very dubious assumption to make. Consider the
realview board: the 'card present' line from the pl181
MMC card controller is wired up to (1) a GPIO input on the
'sysctl' system register module [which causes it to appear
as a status bit in a register] and (2) via an inverter to
an input on a pl061 GPIO module. On coming out of reset
something has to be done to make the reset value of the
output from the pl181 be reflected in the internal status
of the sysctl and pl061 devices.

(On real hardware this happens when the pl061/sysctl
devices come out of reset and sample their input pins.
If we wanted to model it that way we'd need internal state
on all gpio lines and a two phase "enter reset"+"leave reset"
or something.)

-- PMM

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Blue Swirl]

On Wed, Sep 28, 2011 at 9:38 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 28 September 2011 19:42, Blue Swirl <blauwirbel@gmail.com> wrote:
>> We also assume that the reset states of devices and their outputs are
>> coherent i.e. the output of one device during reset would not change
>> the state of another device from its reset state.
>
> This seems a very dubious assumption to make. Consider the
> realview board: the 'card present' line from the pl181
> MMC card controller is wired up to (1) a GPIO input on the
> 'sysctl' system register module [which causes it to appear
> as a status bit in a register] and (2) via an inverter to
> an input on a pl061 GPIO module. On coming out of reset
> something has to be done to make the reset value of the
> output from the pl181 be reflected in the internal status
> of the sysctl and pl061 devices.
>
> (On real hardware this happens when the pl061/sysctl
> devices come out of reset and sample their input pins.
> If we wanted to model it that way we'd need internal state
> on all gpio lines and a two phase "enter reset"+"leave reset"
> or something.)

Maybe the rule should be that during reset, all inputs should be in
high impedance mode. We'd need a two-phase reset for QEMU for that
too, or to disconnect all signals before reset and then reconnect them
after that.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Jan Kiszka]

On 2011-09-28 20:01, Blue Swirl wrote:
> On Wed, Sep 28, 2011 at 11:00 AM, Jan Kiszka<jan.kiszka@siemens.com>  wrote:
>> As we clearly modify the PIC state on pic_reset, we also have to update
>> the IRQ output. This only happened on init so far. Apply this
>> consistently.
>
> Nack, IRQ lines shouldn't be touched on reset. The other side may not
> be ready for receiving the interrupt change and qemu_irqs are
> stateless anyway.

Sorry, but failing to clear the line (this is what pic_update_irq will 
effectively do) is a clear bug in the current code. This patch is 100% 
analogue to what, e.g. the PCI layer does on reset. Please re-read.

Jan

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Blue Swirl]

On Wed, Sep 28, 2011 at 9:18 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2011-09-28 20:01, Blue Swirl wrote:
>>
>> On Wed, Sep 28, 2011 at 11:00 AM, Jan Kiszka<jan.kiszka@siemens.com>
>>  wrote:
>>>
>>> As we clearly modify the PIC state on pic_reset, we also have to update
>>> the IRQ output. This only happened on init so far. Apply this
>>> consistently.
>>
>> Nack, IRQ lines shouldn't be touched on reset. The other side may not
>> be ready for receiving the interrupt change and qemu_irqs are
>> stateless anyway.
>
> Sorry, but failing to clear the line (this is what pic_update_irq will
> effectively do) is a clear bug in the current code. This patch is 100%
> analogue to what, e.g. the PCI layer does on reset. Please re-read.

Reset will happen also when the devices are created. At that time,
qemu_irq callback triggered by changing of the state may produce
undesired effects on the other side. There have been bugs earlier, see
bc26e55a6615dc594be425d293db40d5cdcdb84b and
42f1ced228c9b616cfa2b69846025271618e4ef5 and discussion in
http://lists.nongnu.org/archive/html/qemu-devel/2009-06/msg01024.html.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Jan Kiszka]

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

On 2011-09-29 21:45, Blue Swirl wrote:
> On Wed, Sep 28, 2011 at 9:18 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2011-09-28 20:01, Blue Swirl wrote:
>>>
>>> On Wed, Sep 28, 2011 at 11:00 AM, Jan Kiszka<jan.kiszka@siemens.com>
>>>  wrote:
>>>>
>>>> As we clearly modify the PIC state on pic_reset, we also have to update
>>>> the IRQ output. This only happened on init so far. Apply this
>>>> consistently.
>>>
>>> Nack, IRQ lines shouldn't be touched on reset. The other side may not
>>> be ready for receiving the interrupt change and qemu_irqs are
>>> stateless anyway.
>>
>> Sorry, but failing to clear the line (this is what pic_update_irq will
>> effectively do) is a clear bug in the current code. This patch is 100%
>> analogue to what, e.g. the PCI layer does on reset. Please re-read.
> 
> Reset will happen also when the devices are created. At that time,
> qemu_irq callback triggered by changing of the state may produce
> undesired effects on the other side.

All those potential effects will be cleared again when the receiver is
reset as well. If not, that would be a bug which requires fixing.

> There have been bugs earlier, see
> bc26e55a6615dc594be425d293db40d5cdcdb84b and
> 42f1ced228c9b616cfa2b69846025271618e4ef5 and discussion in
> http://lists.nongnu.org/archive/html/qemu-devel/2009-06/msg01024.html.

Deasserting IRQs at PCI device level is indeed useless as we already do
this in the PCI core.

There is no difference between system reset after power-up or later on.
And there should be no difference between per device, per group of
devices or system-wide reset. A device model cannot tell these apart.
That the devices reset handler is only called on system reset is an odd
and fragile assumption - not that fragile for platform devices like the
i8259, but definitely bogus for pluggable ones.

My series does not depend on this cleanup/fix, but the reason not to
apply remains wrong IMHO.

Jan


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

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Peter Maydell]

On 30 September 2011 07:47, Jan Kiszka <jan.kiszka@web.de> wrote:
> There is no difference between system reset after power-up or later on.
> And there should be no difference between per device, per group of
> devices or system-wide reset. A device model cannot tell these apart.

I think it's also useful to distinguish "system reset" meaning 'simulated
reset button was pressed, causing the reset line to be asserted on various
devices' from "simulation reset" meaning 'qemu effectively tore down the
whole simulation and put it into the same state it would be when qemu
initially starts'. I don't think you can usefully reset a single device
except by asserting its simulated reset signal, except possibly for
hotplug type situations where you're simulating applying power to it.

> My series does not depend on this cleanup/fix, but the reason not to
> apply remains wrong IMHO.

(just to be clear) I'm not arguing against your patch, just more
generally that I'm not sure our current model of reset is entirely
coherent.

-- PMM

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Blue Swirl]

On Fri, Sep 30, 2011 at 9:14 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 30 September 2011 07:47, Jan Kiszka <jan.kiszka@web.de> wrote:
>> There is no difference between system reset after power-up or later on.
>> And there should be no difference between per device, per group of
>> devices or system-wide reset. A device model cannot tell these apart.
>
> I think it's also useful to distinguish "system reset" meaning 'simulated
> reset button was pressed, causing the reset line to be asserted on various
> devices' from "simulation reset" meaning 'qemu effectively tore down the
> whole simulation and put it into the same state it would be when qemu
> initially starts'. I don't think you can usefully reset a single device
> except by asserting its simulated reset signal, except possibly for
> hotplug type situations where you're simulating applying power to it.

In the 2009 thread, i286 triple fault was mentioned. It's also
possible to reset a single device, which can then reset a slave device
and so forth, like Sparc32 DMA controller can reset ESP device and
that should perform a SCSI bus reset.

>> My series does not depend on this cleanup/fix, but the reason not to
>> apply remains wrong IMHO.
>
> (just to be clear) I'm not arguing against your patch, just more
> generally that I'm not sure our current model of reset is entirely
> coherent.
>
> -- PMM
>

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Blue Swirl]

On Fri, Sep 30, 2011 at 6:47 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2011-09-29 21:45, Blue Swirl wrote:
>> On Wed, Sep 28, 2011 at 9:18 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>> On 2011-09-28 20:01, Blue Swirl wrote:
>>>>
>>>> On Wed, Sep 28, 2011 at 11:00 AM, Jan Kiszka<jan.kiszka@siemens.com>
>>>>  wrote:
>>>>>
>>>>> As we clearly modify the PIC state on pic_reset, we also have to update
>>>>> the IRQ output. This only happened on init so far. Apply this
>>>>> consistently.
>>>>
>>>> Nack, IRQ lines shouldn't be touched on reset. The other side may not
>>>> be ready for receiving the interrupt change and qemu_irqs are
>>>> stateless anyway.
>>>
>>> Sorry, but failing to clear the line (this is what pic_update_irq will
>>> effectively do) is a clear bug in the current code. This patch is 100%
>>> analogue to what, e.g. the PCI layer does on reset. Please re-read.
>>
>> Reset will happen also when the devices are created. At that time,
>> qemu_irq callback triggered by changing of the state may produce
>> undesired effects on the other side.
>
> All those potential effects will be cleared again when the receiver is
> reset as well. If not, that would be a bug which requires fixing.

No, the order of resetting devices is not known so the IRQ may or may
not have the effect. If the other device still has pre-reset
configuration, it may spread incorrect effects to devices which
already were reset.

If reliable post reset effects are needed, reset should be divided
into two separate phases. So far no interesting cases have been
demonstrated.

>> There have been bugs earlier, see
>> bc26e55a6615dc594be425d293db40d5cdcdb84b and
>> 42f1ced228c9b616cfa2b69846025271618e4ef5 and discussion in
>> http://lists.nongnu.org/archive/html/qemu-devel/2009-06/msg01024.html.
>
> Deasserting IRQs at PCI device level is indeed useless as we already do
> this in the PCI core.
>
> There is no difference between system reset after power-up or later on.
> And there should be no difference between per device, per group of
> devices or system-wide reset. A device model cannot tell these apart.
> That the devices reset handler is only called on system reset is an odd
> and fragile assumption - not that fragile for platform devices like the
> i8259, but definitely bogus for pluggable ones.

That part of the discussion is obsolete (or at least uninteresting
here). For example this message has a relevant example:
http://lists.nongnu.org/archive/html/qemu-devel/2009-06/msg01081.html

It's about VM restore, but the situation is similar during reset.

> My series does not depend on this cleanup/fix, but the reason not to
> apply remains wrong IMHO.

Sorry, but if a patch is incorrect, it is not a cleanup nor a fix.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Jan Kiszka]

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

On 2011-09-30 22:47, Blue Swirl wrote:
> That part of the discussion is obsolete (or at least uninteresting
> here). For example this message has a relevant example:
> http://lists.nongnu.org/archive/html/qemu-devel/2009-06/msg01081.html
> 
> It's about VM restore, but the situation is similar during reset.

Actually, that is not comparable as we are entering the device's
quiescent state.

> 
>> My series does not depend on this cleanup/fix, but the reason not to
>> apply remains wrong IMHO.
> 
> Sorry, but if a patch is incorrect, it is not a cleanup nor a fix.

Well, both current MIPS and PREP look like they would benefit from it.

Jan


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

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Blue Swirl]

On Sat, Oct 1, 2011 at 6:47 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2011-09-30 22:47, Blue Swirl wrote:
>> That part of the discussion is obsolete (or at least uninteresting
>> here). For example this message has a relevant example:
>> http://lists.nongnu.org/archive/html/qemu-devel/2009-06/msg01081.html
>>
>> It's about VM restore, but the situation is similar during reset.
>
> Actually, that is not comparable as we are entering the device's
> quiescent state.

It is. Here's an example for the reset case based on the Paul's original one.

Because devices are reset in unpredictable order that they should not
be communicating with other devices (e.g. by modifying IRQ lines).

Consider a system with a device (DEV) and a level triggered interrupt
controller (PIC1) with the ability to toggle the level where
triggering happens, chained to a rising edge triggered interrupt
controller (PIC2).

(DEV) ->  (PIC1) -> (PIC2)

Before reset, DEV output is high, PIC1 has the interrupt unmasked (but
high) and the trigger level is configured as active low, PIC2 has no
pending interrupts.

We now reset, so the state should be that DEV output is low, PIC1 has
masked all interrupts and its input set to active high, and PIC2 has
no pending interrupts. Devices are reset in the order PIC2, DEV, PIC1.

If devices toggle their interrupts on reset then we get incorrect
state after the reset:

PIC2 is reset to the desired no-interrupts-pending state.

DEV is reset. This lowers the IRQ, which is passed to PIC1. PIC1 still
has the old interrupt mask and level set to active low, so it passes
the IRQ through to PIC2, which detects the edge event and marks the
interrupt as pending.

PIC1 is reset, updates the new mask, sets the input level to active
high and lowers its output. However this event does not clear the
internal PIC2 pending interrupt flag, so machine state will be wrong
after reset.

Therefore it is incorrect to perform any qemu_irq activities during
reset (also VM restore like the original example), don't you agree?

If we continued to reset all the devices (call the reset handlers
multiple times), eventually machine state should stabilize (equivalent
of real HW with nice long reset pulses), but on QEMU the reset event
is infinitely short so we have to be more careful.

Actually I don't think that even a two-phase reset with qemu_irq or
Pin activity on the second phase would produce correct results in
every obscure case. Though this may be detectable since the start
state would be known.

>>> My series does not depend on this cleanup/fix, but the reason not to
>>> apply remains wrong IMHO.
>>
>> Sorry, but if a patch is incorrect, it is not a cleanup nor a fix.
>
> Well, both current MIPS and PREP look like they would benefit from it.

How?

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Jan Kiszka]

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

On 2011-10-01 09:31, Blue Swirl wrote:
> On Sat, Oct 1, 2011 at 6:47 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2011-09-30 22:47, Blue Swirl wrote:
>>> That part of the discussion is obsolete (or at least uninteresting
>>> here). For example this message has a relevant example:
>>> http://lists.nongnu.org/archive/html/qemu-devel/2009-06/msg01081.html
>>>
>>> It's about VM restore, but the situation is similar during reset.
>>
>> Actually, that is not comparable as we are entering the device's
>> quiescent state.
> 
> It is. Here's an example for the reset case based on the Paul's original one.
> 
> Because devices are reset in unpredictable order that they should not
> be communicating with other devices (e.g. by modifying IRQ lines).
> 
> Consider a system with a device (DEV) and a level triggered interrupt
> controller (PIC1) with the ability to toggle the level where
> triggering happens, chained to a rising edge triggered interrupt
> controller (PIC2).
> 
> (DEV) ->  (PIC1) -> (PIC2)
> 
> Before reset, DEV output is high, PIC1 has the interrupt unmasked (but
> high) and the trigger level is configured as active low, PIC2 has no
> pending interrupts.
> 
> We now reset, so the state should be that DEV output is low, PIC1 has
> masked all interrupts and its input set to active high, and PIC2 has
> no pending interrupts. Devices are reset in the order PIC2, DEV, PIC1.
> 
> If devices toggle their interrupts on reset then we get incorrect
> state after the reset:
> 
> PIC2 is reset to the desired no-interrupts-pending state.
> 
> DEV is reset. This lowers the IRQ, which is passed to PIC1. PIC1 still
> has the old interrupt mask and level set to active low, so it passes
> the IRQ through to PIC2, which detects the edge event and marks the
> interrupt as pending.
> 
> PIC1 is reset, updates the new mask, sets the input level to active
> high and lowers its output. However this event does not clear the
> internal PIC2 pending interrupt flag, so machine state will be wrong
> after reset.
> 
> Therefore it is incorrect to perform any qemu_irq activities during
> reset (also VM restore like the original example), don't you agree?

A rather odd but valid counterexample. Have you seen such a setup already?

But I'll provide a real example where the model "no IRQ change
propagated on reset, devices handle this internally" fails as well:

PIC -> CPU

We have a level-triggered active-high line in this case. When the CPU is
reset, it "somehow" knows that it is attached to the PIC and assumes
that this device is reset as well. Therefore, the CPU clears its cached
input state on reset. That works if both devices are actually reset. But
it fails if only the CPU is reset while the PIC output is active.

That's likely the reason why MIPS and PPC/PREP do no touch the cached
interrupt line state on reset but expect that the source will inform
them whenever the line goes down - e.g. due to reset.

The conflict we are in with the current reset model is hard-coding the
board wiring and source knowledge into sink device models vs.
propagating reset states. I agree that both have their corner cases.

But in order to continue with properly disentangling board knowledge
from generic device models, we should head for the latter variant where
already possible (like in the i8259 case). On the long term, this should
be resolved using a two-stage model where every root of an interrupt
line signals its state down the chain at the end of a reset phase.

Jan


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

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Blue Swirl]

On Sun, Oct 2, 2011 at 4:27 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2011-10-01 09:31, Blue Swirl wrote:
>> On Sat, Oct 1, 2011 at 6:47 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>> On 2011-09-30 22:47, Blue Swirl wrote:
>>>> That part of the discussion is obsolete (or at least uninteresting
>>>> here). For example this message has a relevant example:
>>>> http://lists.nongnu.org/archive/html/qemu-devel/2009-06/msg01081.html
>>>>
>>>> It's about VM restore, but the situation is similar during reset.
>>>
>>> Actually, that is not comparable as we are entering the device's
>>> quiescent state.
>>
>> It is. Here's an example for the reset case based on the Paul's original one.
>>
>> Because devices are reset in unpredictable order that they should not
>> be communicating with other devices (e.g. by modifying IRQ lines).
>>
>> Consider a system with a device (DEV) and a level triggered interrupt
>> controller (PIC1) with the ability to toggle the level where
>> triggering happens, chained to a rising edge triggered interrupt
>> controller (PIC2).
>>
>> (DEV) ->  (PIC1) -> (PIC2)
>>
>> Before reset, DEV output is high, PIC1 has the interrupt unmasked (but
>> high) and the trigger level is configured as active low, PIC2 has no
>> pending interrupts.
>>
>> We now reset, so the state should be that DEV output is low, PIC1 has
>> masked all interrupts and its input set to active high, and PIC2 has
>> no pending interrupts. Devices are reset in the order PIC2, DEV, PIC1.
>>
>> If devices toggle their interrupts on reset then we get incorrect
>> state after the reset:
>>
>> PIC2 is reset to the desired no-interrupts-pending state.
>>
>> DEV is reset. This lowers the IRQ, which is passed to PIC1. PIC1 still
>> has the old interrupt mask and level set to active low, so it passes
>> the IRQ through to PIC2, which detects the edge event and marks the
>> interrupt as pending.
>>
>> PIC1 is reset, updates the new mask, sets the input level to active
>> high and lowers its output. However this event does not clear the
>> internal PIC2 pending interrupt flag, so machine state will be wrong
>> after reset.
>>
>> Therefore it is incorrect to perform any qemu_irq activities during
>> reset (also VM restore like the original example), don't you agree?
>
> A rather odd but valid counterexample. Have you seen such a setup already?
>
> But I'll provide a real example where the model "no IRQ change
> propagated on reset, devices handle this internally" fails as well:
>
> PIC -> CPU
>
> We have a level-triggered active-high line in this case. When the CPU is
> reset, it "somehow" knows that it is attached to the PIC and assumes
> that this device is reset as well. Therefore, the CPU clears its cached
> input state on reset. That works if both devices are actually reset. But
> it fails if only the CPU is reset while the PIC output is active.

OK, we have a positive incorrect case and negative one. This means
that the current model is broken.

> That's likely the reason why MIPS and PPC/PREP do no touch the cached
> interrupt line state on reset but expect that the source will inform
> them whenever the line goes down - e.g. due to reset.
>
> The conflict we are in with the current reset model is hard-coding the
> board wiring and source knowledge into sink device models vs.
> propagating reset states. I agree that both have their corner cases.
>
> But in order to continue with properly disentangling board knowledge
> from generic device models, we should head for the latter variant where
> already possible (like in the i8259 case). On the long term, this should
> be resolved using a two-stage model where every root of an interrupt
> line signals its state down the chain at the end of a reset phase.

I don't think that even a two phase reset can solve the instability in
all possible cases. If the qemu_irq lines form a complex network,
several cycles could be needed until the effects have propagated and
the network has stabilized. In a defective network (loop with NOT in
the middle), the network could oscillate forever and never stabilize
(or until qemu_irq callbacks fill the stack and QEMU crashes), just
like real HW.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Avi Kivity]

On 10/01/2011 10:31 AM, Blue Swirl wrote:
> Therefore it is incorrect to perform any qemu_irq activities during
> reset (also VM restore like the original example), don't you agree?

It is not incorrect.  Real hardware updates outputs on RESET assertion, 
and real hardware deals with devices entering reset at different times 
(due to signal propagation delay or slow devices).

> If we continued to reset all the devices (call the reset handlers
> multiple times), eventually machine state should stabilize (equivalent
> of real HW with nice long reset pulses), but on QEMU the reset event
> is infinitely short so we have to be more careful.

calling qemu_irq_pulse(reset) simulates a reset signal of any length 
(since nothing happens between the two edges).

> Actually I don't think that even a two-phase reset with qemu_irq or
> Pin activity on the second phase would produce correct results in
> every obscure case. Though this may be detectable since the start
> state would be known.

The output signals have to stabilize before the second edge of the reset 
signal.

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

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Blue Swirl]

On Sun, Oct 2, 2011 at 4:56 PM, Avi Kivity <avi@redhat.com> wrote:
> On 10/01/2011 10:31 AM, Blue Swirl wrote:
>>
>> Therefore it is incorrect to perform any qemu_irq activities during
>> reset (also VM restore like the original example), don't you agree?
>
> It is not incorrect.  Real hardware updates outputs on RESET assertion, and
> real hardware deals with devices entering reset at different times (due to
> signal propagation delay or slow devices).

Yes, but on real hardware, during the propagation of any effects, the
reset line is held asserted for millions of clock cycles in order to
stabilize the machine.

>> If we continued to reset all the devices (call the reset handlers
>> multiple times), eventually machine state should stabilize (equivalent
>> of real HW with nice long reset pulses), but on QEMU the reset event
>> is infinitely short so we have to be more careful.
>
> calling qemu_irq_pulse(reset) simulates a reset signal of any length (since
> nothing happens between the two edges).

Not really. The first edge could trigger the reset (but don't sample
inputs) phase, this would be equal to forcefully stabilizing any
device internal state. The second would release the device inputs, but
that's also a source of problems.

>> Actually I don't think that even a two-phase reset with qemu_irq or
>> Pin activity on the second phase would produce correct results in
>> every obscure case. Though this may be detectable since the start
>> state would be known.
>
> The output signals have to stabilize before the second edge of the reset
> signal.

They can't since the devices' inputs are ignored at that phase.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Avi Kivity]

----- Original Message -----
> On Sun, Oct 2, 2011 at 4:56 PM, Avi Kivity <avi@redhat.com> wrote:
> > On 10/01/2011 10:31 AM, Blue Swirl wrote:
> >>
> >> Therefore it is incorrect to perform any qemu_irq activities
> >> during
> >> reset (also VM restore like the original example), don't you
> >> agree?
> >
> > It is not incorrect.  Real hardware updates outputs on RESET
> > assertion, and
> > real hardware deals with devices entering reset at different times
> > (due to
> > signal propagation delay or slow devices).
>
> Yes, but on real hardware, during the propagation of any effects, the
> reset line is held asserted for millions of clock cycles in order to
> stabilize the machine.

But nothing can happen in these cycles, since qemu emulates everything that happens in the on the edge, and any timers will be cancelled.


>
> >> If we continued to reset all the devices (call the reset handlers
> >> multiple times), eventually machine state should stabilize
> >> (equivalent
> >> of real HW with nice long reset pulses), but on QEMU the reset
> >> event
> >> is infinitely short so we have to be more careful.
> >
> > calling qemu_irq_pulse(reset) simulates a reset signal of any
> > length (since
> > nothing happens between the two edges).
>
> Not really. The first edge could trigger the reset (but don't sample
> inputs) phase, this would be equal to forcefully stabilizing any
> device internal state. The second would release the device inputs,
> but
> that's also a source of problems.


Can you elaborate on the problems?

>
> >> Actually I don't think that even a two-phase reset with qemu_irq
> >> or
> >> Pin activity on the second phase would produce correct results in
> >> every obscure case. Though this may be detectable since the start
> >> state would be known.
> >
> > The output signals have to stabilize before the second edge of the
> > reset
> > signal.
>
> They can't since the devices' inputs are ignored at that phase.
>

A real device also ignores inputs during reset (or if it doesn't, we can just emulate that).


I would really like to see a concrete example we can discuss.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Blue Swirl]

On Sun, Oct 2, 2011 at 7:20 PM, Avi Kivity <avi@redhat.com> wrote:
>
>
> ----- Original Message -----
>> On Sun, Oct 2, 2011 at 4:56 PM, Avi Kivity <avi@redhat.com> wrote:
>> > On 10/01/2011 10:31 AM, Blue Swirl wrote:
>> >>
>> >> Therefore it is incorrect to perform any qemu_irq activities
>> >> during
>> >> reset (also VM restore like the original example), don't you
>> >> agree?
>> >
>> > It is not incorrect.  Real hardware updates outputs on RESET
>> > assertion, and
>> > real hardware deals with devices entering reset at different times
>> > (due to
>> > signal propagation delay or slow devices).
>>
>> Yes, but on real hardware, during the propagation of any effects, the
>> reset line is held asserted for millions of clock cycles in order to
>> stabilize the machine.
>
> But nothing can happen in these cycles, since qemu emulates everything that happens in the on the edge, and any timers will be cancelled.

There are some wild activities in the beginning of the reset, but
after that nothing will happen.

>>
>> >> If we continued to reset all the devices (call the reset handlers
>> >> multiple times), eventually machine state should stabilize
>> >> (equivalent
>> >> of real HW with nice long reset pulses), but on QEMU the reset
>> >> event
>> >> is infinitely short so we have to be more careful.
>> >
>> > calling qemu_irq_pulse(reset) simulates a reset signal of any
>> > length (since
>> > nothing happens between the two edges).
>>
>> Not really. The first edge could trigger the reset (but don't sample
>> inputs) phase, this would be equal to forcefully stabilizing any
>> device internal state. The second would release the device inputs,
>> but
>> that's also a source of problems.
>
>
> Can you elaborate on the problems?
>
>>
>> >> Actually I don't think that even a two-phase reset with qemu_irq
>> >> or
>> >> Pin activity on the second phase would produce correct results in
>> >> every obscure case. Though this may be detectable since the start
>> >> state would be known.
>> >
>> > The output signals have to stabilize before the second edge of the
>> > reset
>> > signal.
>>
>> They can't since the devices' inputs are ignored at that phase.
>>
>
> A real device also ignores inputs during reset (or if it doesn't, we can just emulate that).

Maybe this could work:
1 - issue start of reset cycle (raise qemu_irq, unrealize): internal
states reset, no I/O.
2 - issue start of I/O, reset held (new phase): evaluate inputs and
post outputs, but consider also that reset is still active, so
transition is not complete for all devices
3 - release reset line (lower qemu_irq, realize): gentlemen, start your engines

During phase 2 some of the network effects could be contained.

> I would really like to see a concrete example we can discuss.

See my previous message.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Avi Kivity]

> >
> > A real device also ignores inputs during reset (or if it doesn't,
> > we can just emulate that).
> 
> Maybe this could work:
> 1 - issue start of reset cycle (raise qemu_irq, unrealize): internal
> states reset, no I/O.
> 2 - issue start of I/O, reset held (new phase): evaluate inputs and
> post outputs, but consider also that reset is still active, so
> transition is not complete for all devices

This doesn't correspond to real hardware.  Each device detects the reset edge independently.  There is no signal that says "all devices have seen the edge".

We should either have just 1, or merge 1 and 2.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Blue Swirl]

On Sun, Oct 2, 2011 at 7:44 PM, Avi Kivity <avi@redhat.com> wrote:
>> >
>> > A real device also ignores inputs during reset (or if it doesn't,
>> > we can just emulate that).
>>
>> Maybe this could work:
>> 1 - issue start of reset cycle (raise qemu_irq, unrealize): internal
>> states reset, no I/O.
>> 2 - issue start of I/O, reset held (new phase): evaluate inputs and
>> post outputs, but consider also that reset is still active, so
>> transition is not complete for all devices
>
> This doesn't correspond to real hardware.   Each device detects the reset edge independently.  There is no signal that says "all devices have seen the edge".

Reset is not edge triggered but level signal.

> We should either have just 1, or merge 1 and 2.
>

Then we'd be back to where the problems started.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Avi Kivity]

----- Original Message -----
> On Sun, Oct 2, 2011 at 7:44 PM, Avi Kivity <avi@redhat.com> wrote:
> >> >
> >> > A real device also ignores inputs during reset (or if it
> >> > doesn't,
> >> > we can just emulate that).
> >>
> >> Maybe this could work:
> >> 1 - issue start of reset cycle (raise qemu_irq, unrealize):
> >> internal
> >> states reset, no I/O.
> >> 2 - issue start of I/O, reset held (new phase): evaluate inputs
> >> and
> >> post outputs, but consider also that reset is still active, so
> >> transition is not complete for all devices
> >
> > This doesn't correspond to real hardware.   Each device detects the
> > reset edge independently.  There is no signal that says "all
> > devices have seen the edge".
>
> Reset is not edge triggered but level signal.

It still has propagation delay.  If your XNOR gate connects to the NORAD master launch controller, your design may have side effects.

>
> > We should either have just 1, or merge 1 and 2.
> >
>
> Then we'd be back to where the problems started.
>

Sorry, I don't see the problem yet.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Blue Swirl]

On Sun, Oct 2, 2011 at 7:52 PM, Avi Kivity <avi@redhat.com> wrote:
>
>
> ----- Original Message -----
>> On Sun, Oct 2, 2011 at 7:44 PM, Avi Kivity <avi@redhat.com> wrote:
>> >> >
>> >> > A real device also ignores inputs during reset (or if it
>> >> > doesn't,
>> >> > we can just emulate that).
>> >>
>> >> Maybe this could work:
>> >> 1 - issue start of reset cycle (raise qemu_irq, unrealize):
>> >> internal
>> >> states reset, no I/O.
>> >> 2 - issue start of I/O, reset held (new phase): evaluate inputs
>> >> and
>> >> post outputs, but consider also that reset is still active, so
>> >> transition is not complete for all devices
>> >
>> > This doesn't correspond to real hardware.   Each device detects the
>> > reset edge independently.  There is no signal that says "all
>> > devices have seen the edge".
>>
>> Reset is not edge triggered but level signal.
>
> It still has propagation delay.  If your XNOR gate connects to the NORAD master launch controller, your design may have side effects.

Hopefully the reset signal would control also the edge triggered
launch controller.

The delays are one reason why real reset pulses are so long. The other
reasons are the stabilization effects, internal and external.

>> > We should either have just 1, or merge 1 and 2.
>> >
>>
>> Then we'd be back to where the problems started.
>>
>
> Sorry, I don't see the problem yet.

I'm not sure we have a complete solution either. Phase 1 would solve
most problems with old state which is the most important issue here.
If the machine refuses to wake up from reset, this should be noticed
quite soon, so we could move to dual phase system while still
searching for the best possible system in all possible worlds.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Avi Kivity]

> >
> > It still has propagation delay.  If your XNOR gate connects to the
> > NORAD master launch controller, your design may have side effects.
>
> Hopefully the reset signal would control also the edge triggered
> launch controller.

It doesn't help.  There is propagation delay there as well.  If the input wins the race against reset, the launch sequence is started.

>
> The delays are one reason why real reset pulses are so long. The
> other
> reasons are the stabilization effects, internal and external.
>
> >> > We should either have just 1, or merge 1 and 2.
> >> >
> >>
> >> Then we'd be back to where the problems started.
> >>
> >
> > Sorry, I don't see the problem yet.
>
> I'm not sure we have a complete solution either. Phase 1 would solve
> most problems with old state which is the most important issue here.
> If the machine refuses to wake up from reset, this should be noticed
> quite soon, so we could move to dual phase system while still
> searching for the best possible system in all possible worlds.
>


Okay.  I still don't think there is a problem.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Blue Swirl]

On Sun, Oct 2, 2011 at 8:03 PM, Avi Kivity <avi@redhat.com> wrote:
>> >
>> > It still has propagation delay.  If your XNOR gate connects to the
>> > NORAD master launch controller, your design may have side effects.
>>
>> Hopefully the reset signal would control also the edge triggered
>> launch controller.
>
> It doesn't help.  There is propagation delay there as well.  If the input wins the race against reset, the launch sequence is started.

To bring the example back to QEMU, a disk write could be issued or
network packet could be sent. But still, this only confirms that
during the initial phase of reset, no output activities can be allowed
to avoid such problems.

>> The delays are one reason why real reset pulses are so long. The
>> other
>> reasons are the stabilization effects, internal and external.
>>
>> >> > We should either have just 1, or merge 1 and 2.
>> >> >
>> >>
>> >> Then we'd be back to where the problems started.
>> >>
>> >
>> > Sorry, I don't see the problem yet.
>>
>> I'm not sure we have a complete solution either. Phase 1 would solve
>> most problems with old state which is the most important issue here.
>> If the machine refuses to wake up from reset, this should be noticed
>> quite soon, so we could move to dual phase system while still
>> searching for the best possible system in all possible worlds.
>>
>
>
> Okay.  I still don't think there is a problem.

There is, Jan's real example and my theoretical cases show that.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Avi Kivity]

> >
> > It doesn't help.  There is propagation delay there as well.  If the
> > input wins the race against reset, the launch sequence is started.
>
> To bring the example back to QEMU, a disk write could be issued or
> network packet could be sent. But still, this only confirms that
> during the initial phase of reset, no output activities can be
> allowed
> to avoid such problems.

In fact these aren't problems.  The packet may be sent or data written, as long as they aren't corrupted.  A device is allowed to "delay" a reset (but not indefinitely).


> > Okay.  I still don't think there is a problem.
>
> There is, Jan's real example and my theoretical cases show that.
>

I believe your example has the same problem on real hardware.  Jan, is your problem not solved by two-phase reset?

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Blue Swirl]

On Sun, Oct 2, 2011 at 8:17 PM, Avi Kivity <avi@redhat.com> wrote:
>> >
>> > It doesn't help.  There is propagation delay there as well.  If the
>> > input wins the race against reset, the launch sequence is started.
>>
>> To bring the example back to QEMU, a disk write could be issued or
>> network packet could be sent. But still, this only confirms that
>> during the initial phase of reset, no output activities can be
>> allowed
>> to avoid such problems.
>
> In fact these aren't problems.  The packet may be sent or data written, as long as they aren't corrupted.  A device is allowed to "delay" a reset (but not indefinitely).

Oh, but corruption could easily happen. Consider for example a disk
controller waiting for DMA ready signal a device separate from the DMA
controller. Due to reset glitches in the device or signal chain, the
DMA ready signal arrives but the DMA controller still contains old
information, writing the data to disk from wrong memory location.

>> > Okay.  I still don't think there is a problem.
>>
>> There is, Jan's real example and my theoretical cases show that.
>>
>
> I believe your example has the same problem on real hardware.  Jan, is your problem not solved by two-phase reset?

On real hardware the problem is masked by the long reset pulse. We
have an infinitely short pulse.

Jan's case should be OK, a CPU doesn't have any outputs.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Avi Kivity]

> >
> > In fact these aren't problems.  The packet may be sent or data
> > written, as long as they aren't corrupted.  A device is allowed to
> > "delay" a reset (but not indefinitely).
>
> Oh, but corruption could easily happen. Consider for example a disk
> controller waiting for DMA ready signal a device separate from the
> DMA
> controller. Due to reset glitches in the device or signal chain, the
> DMA ready signal arrives but the DMA controller still contains old
> information, writing the data to disk from wrong memory location.


Would not this corruption also happen on real hardware?  If reset to the disk controller is delayed by a slow gate or extra capacitance on a line?

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Blue Swirl]

On Sun, Oct 2, 2011 at 8:31 PM, Avi Kivity <avi@redhat.com> wrote:
>
>> >
>> > In fact these aren't problems.  The packet may be sent or data
>> > written, as long as they aren't corrupted.  A device is allowed to
>> > "delay" a reset (but not indefinitely).
>>
>> Oh, but corruption could easily happen. Consider for example a disk
>> controller waiting for DMA ready signal a device separate from the
>> DMA
>> controller. Due to reset glitches in the device or signal chain, the
>> DMA ready signal arrives but the DMA controller still contains old
>> information, writing the data to disk from wrong memory location.
>
>
> Would not this corruption also happen on real hardware?  If reset to the disk controller is delayed by a slow gate or extra capacitance on a line?

Maybe, but the delays are probably too short on real HW before any
packets are sent or disk gets written. On QEMU, I/O can be
instantaneous.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Avi Kivity]

> >
> >
> > Would not this corruption also happen on real hardware?  If reset
> > to the disk controller is delayed by a slow gate or extra
> > capacitance on a line?
>
> Maybe, but the delays are probably too short on real HW before any
> packets are sent or disk gets written. On QEMU, I/O can be
> instantaneous.
>

Right, this is a real difference.  If any hardware actually depends on this, we can model it by launching a timer instead of issuing the I/O.  When the reset arrives to the disk controller, it will cancel the timer.

This is expensive both to code and in run-time performance, but we can afford the expense since we don't have such a case, yes?

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Blue Swirl]

On Sun, Oct 2, 2011 at 8:41 PM, Avi Kivity <avi@redhat.com> wrote:
>> >
>> >
>> > Would not this corruption also happen on real hardware?  If reset
>> > to the disk controller is delayed by a slow gate or extra
>> > capacitance on a line?
>>
>> Maybe, but the delays are probably too short on real HW before any
>> packets are sent or disk gets written. On QEMU, I/O can be
>> instantaneous.
>>
>
> Right, this is a real difference.  If any hardware actually depends on this, we can model it by launching a timer instead of issuing the I/O.  When the reset arrives to the disk controller, it will cancel the timer.
>
> This is expensive both to code and in run-time performance, but we can afford the expense since we don't have such a case, yes?

It's already in, see -win2k-hack and floppy DMA hack.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Paolo Bonzini]

On 10/02/2011 10:55 PM, Blue Swirl wrote:
> It's already in, see -win2k-hack and floppy DMA hack.

Is the floppy DMA hack just a consequence of the weird idle BHs, or is 
it to actually please a real BIOS?

Paolo

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Avi Kivity]

On 10/02/2011 10:36 PM, Blue Swirl wrote:
> On Sun, Oct 2, 2011 at 8:31 PM, Avi Kivity<avi@redhat.com>  wrote:
> >
> >>  >
> >>  >  In fact these aren't problems.  The packet may be sent or data
> >>  >  written, as long as they aren't corrupted.  A device is allowed to
> >>  >  "delay" a reset (but not indefinitely).
> >>
> >>  Oh, but corruption could easily happen. Consider for example a disk
> >>  controller waiting for DMA ready signal a device separate from the
> >>  DMA
> >>  controller. Due to reset glitches in the device or signal chain, the
> >>  DMA ready signal arrives but the DMA controller still contains old
> >>  information, writing the data to disk from wrong memory location.
> >
> >
> >  Would not this corruption also happen on real hardware?  If reset to the disk controller is delayed by a slow gate or extra capacitance on a line?
>
> Maybe, but the delays are probably too short on real HW before any
> packets are sent or disk gets written. On QEMU, I/O can be
> instantaneous.

As an anecdote, while reading a chip errata I came upon this:



15. CPU May Record Signal Glitches When MCH is Being Reset

Problem: When the MCH is reset via RSTIN# the CPU may record any one or 
more of the
following errors: address strobe glitch (MSR IA32_MCi_STATUS bit 23), 
data strobe
glitch (MSR IA32_MCi_STATUS bit 22), P/N data strobes out of sync (MSR
IA32_MCi_STATUS bit 21), PIC & FSB data parity (MSR IA32_MCi_STATUS bit 
19), RSP
parity (MSR IA32_MCi_STATUS bit 18), or FSB address parity (MSR 
IA32_MCi_STATUS
bit 16).  This can happen when the MCH asserts CPURST# just after the 
MCH drives an
FSB transaction.  This may happen because RSTIN# and CPURST# maintain an
asynchronous relationship with each other.

Workaround: None.

Status: No Fix

(of course the situation there is different, there is no global reset 
signal)

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

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Peter Maydell]

On 30 September 2011 21:47, Blue Swirl <blauwirbel@gmail.com> wrote:
> If reliable post reset effects are needed, reset should be divided
> into two separate phases. So far no interesting cases have been
> demonstrated.

I've already cited one in this thread [realview mmc card present]
and that's just the one I happen to know about because I made some
patches in that area. I bet there are more in the codebase that
we're not handling properly; they're not easy to find because they
depend on an interaction between how two devices happen to be wired
up rather than being a bug in a particular device.

Another random example: lan9118_reset() calls phy_reset() calls
phy_update_link() calls phy_update_irq() calls lan9118_update()
calls qemu_set_irq(). That's a bug at the moment (ie I agree
that with qdev as it is currently trying to assert outgoing
gpio lines in the reset callback is broken), but we do somehow
need to drive the irq line high on reset (both of the
"write-to-register" triggered kind and the plain old power-on
kind) because the signal defaults to active-low on reset.
(we can't just say our model of the irq line tracks the logical
state of the line, because there's a device config register
that lets you flip it from active-low to active-high, so
our model of the irq line has to have the same sense that the
interrupt controller expects.)

-- PMM

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Avi Kivity]

On 09/28/2011 09:01 PM, Blue Swirl wrote:
> On Wed, Sep 28, 2011 at 11:00 AM, Jan Kiszka<jan.kiszka@siemens.com>  wrote:
> >  As we clearly modify the PIC state on pic_reset, we also have to update
> >  the IRQ output. This only happened on init so far. Apply this
> >  consistently.
>
> Nack, IRQ lines shouldn't be touched on reset. The other side may not
> be ready for receiving the interrupt change and qemu_irqs are
> stateless anyway.
>

The way to fix it is two-phase reset:

phase 1: reset internal state (-> move all outputs to reset values), 
don't sample inputs yet
phase 2: allow sampling inputs



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

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Jan Kiszka]

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

On 2011-10-02 18:39, Avi Kivity wrote:
> On 09/28/2011 09:01 PM, Blue Swirl wrote:
>> On Wed, Sep 28, 2011 at 11:00 AM, Jan Kiszka<jan.kiszka@siemens.com> 
>> wrote:
>> >  As we clearly modify the PIC state on pic_reset, we also have to
>> update
>> >  the IRQ output. This only happened on init so far. Apply this
>> >  consistently.
>>
>> Nack, IRQ lines shouldn't be touched on reset. The other side may not
>> be ready for receiving the interrupt change and qemu_irqs are
>> stateless anyway.
>>
> 
> The way to fix it is two-phase reset:
> 
> phase 1: reset internal state (-> move all outputs to reset values),
> don't sample inputs yet
> phase 2: allow sampling inputs

As far as I understood Anthony's QOM plans, phase 1 will correspond to
"unrealize", phase 2 to "realize".

However, we do not depend on two phases in this particular case (i8259)
and can live with a coalescing both for now.

Jan


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

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Avi Kivity]

> > 
> > The way to fix it is two-phase reset:
> > 
> > phase 1: reset internal state (-> move all outputs to reset
> > values),
> > don't sample inputs yet
> > phase 2: allow sampling inputs
> 
> As far as I understood Anthony's QOM plans, phase 1 will correspond
> to
> "unrealize", phase 2 to "realize".

That smells of abusing mechanism used for construction for reset purposes.

Why not use an ordinary qemu_irq?  It reresents a pin; 0->1 edge (assert) enters phase 1, 1->0 edge (deassert) enters phase 2.  Exactly like real hardware.

> 
> However, we do not depend on two phases in this particular case
> (i8259)
> and can live with a coalescing both for now.
> 

Agree.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Blue Swirl]

On Sun, Oct 2, 2011 at 7:07 PM, Avi Kivity <avi@redhat.com> wrote:
>> >
>> > The way to fix it is two-phase reset:
>> >
>> > phase 1: reset internal state (-> move all outputs to reset
>> > values),
>> > don't sample inputs yet
>> > phase 2: allow sampling inputs
>>
>> As far as I understood Anthony's QOM plans, phase 1 will correspond
>> to
>> "unrealize", phase 2 to "realize".
>
> That smells of abusing mechanism used for construction for reset purposes.
>
> Why not use an ordinary qemu_irq?  It reresents a pin; 0->1 edge (assert) enters phase 1, 1->0 edge (deassert) enters phase 2.  Exactly like real hardware.

Fully agree. I also proposed using qemu_irq for reset (but without
phases) a long time ago.

>>
>> However, we do not depend on two phases in this particular case
>> (i8259)
>> and can live with a coalescing both for now.
>>
>
> Agree.
>

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Jan Kiszka]

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

On 2011-10-02 21:07, Avi Kivity wrote:
>>>
>>> The way to fix it is two-phase reset:
>>>
>>> phase 1: reset internal state (-> move all outputs to reset
>>> values),
>>> don't sample inputs yet
>>> phase 2: allow sampling inputs
>>
>> As far as I understood Anthony's QOM plans, phase 1 will correspond
>> to
>> "unrealize", phase 2 to "realize".
> 
> That smells of abusing mechanism used for construction for reset purposes.
> 
> Why not use an ordinary qemu_irq?  It reresents a pin; 0->1 edge (assert) enters phase 1, 1->0 edge (deassert) enters phase 2.  Exactly like real hardware.

QEMU makes no difference between power-on reset and system reset right
now. At least I am not aware of any device model that has problems with
this. Thus there is apparently no need to treat reset and create
differently. As QOM requires a two-phase device creation anyway (to let
properties to "settle"), it looks like there is some reuse potential.

Jan


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

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Avi Kivity]

> > 
> > Why not use an ordinary qemu_irq?  It reresents a pin; 0->1 edge
> > (assert) enters phase 1, 1->0 edge (deassert) enters phase 2.
> >  Exactly like real hardware.
> 
> QEMU makes no difference between power-on reset and system reset
> right
> now. At least I am not aware of any device model that has problems
> with
> this. Thus there is apparently no need to treat reset and create
> differently. As QOM requires a two-phase device creation anyway (to
> let
> properties to "settle"), it looks like there is some reuse potential.
> 

I'm all for sharing this.  But let's model it as a qemu_irq or Pin.

btw, we could have qemu_irqs that take an extra qemu_irq reset input, so we can do this without much coding.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Blue Swirl]

On Sun, Oct 2, 2011 at 4:39 PM, Avi Kivity <avi@redhat.com> wrote:
> On 09/28/2011 09:01 PM, Blue Swirl wrote:
>>
>> On Wed, Sep 28, 2011 at 11:00 AM, Jan Kiszka<jan.kiszka@siemens.com>
>>  wrote:
>> >  As we clearly modify the PIC state on pic_reset, we also have to update
>> >  the IRQ output. This only happened on init so far. Apply this
>> >  consistently.
>>
>> Nack, IRQ lines shouldn't be touched on reset. The other side may not
>> be ready for receiving the interrupt change and qemu_irqs are
>> stateless anyway.
>>
>
> The way to fix it is two-phase reset:
>
> phase 1: reset internal state (-> move all outputs to reset values), don't
> sample inputs yet

This solves the problem of old state accidentally interfering with reset state.

> phase 2: allow sampling inputs

This could lead to incorrect state for complex networks. It would
still be better than what we have now.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Avi Kivity]

> > phase 1: reset internal state (-> move all outputs to reset
> > values), don't
> > sample inputs yet
> 
> This solves the problem of old state accidentally interfering with
> reset state.
> 
> > phase 2: allow sampling inputs
> 
> This could lead to incorrect state for complex networks. It would
> still be better than what we have now.
> 

Can you give an example?  Can be theoretical, doesn't have to refer to real hardware.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Blue Swirl]

On Sun, Oct 2, 2011 at 7:08 PM, Avi Kivity <avi@redhat.com> wrote:
>> > phase 1: reset internal state (-> move all outputs to reset
>> > values), don't
>> > sample inputs yet
>>
>> This solves the problem of old state accidentally interfering with
>> reset state.
>>
>> > phase 2: allow sampling inputs
>>
>> This could lead to incorrect state for complex networks. It would
>> still be better than what we have now.
>>
>
> Can you give an example?  Can be theoretical, doesn't have to refer to real hardware.

For example, outputs A and B should both be driven high by reset. They
are connected to a XNOR gate, whose output is fed to edge triggered
device. The device should not see any edges outside of the reset
cycle, during reset cycle they are ignored.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Avi Kivity]

> >
> > Can you give an example?  Can be theoretical, doesn't have to refer
> > to real hardware.
>
> For example, outputs A and B should both be driven high by reset.
> They
> are connected to a XNOR gate, whose output is fed to edge triggered
> device. The device should not see any edges outside of the reset
> cycle, during reset cycle they are ignored.
>

I don't see the issue?  After phase 1 the two outputs will be high, after phase two they will be whatever the device logic computes.

During phase 1 you may see an edge, but that also happens with real hardware.  The target device my see A and B driven high before it sees the reset pulse, and A and B (and the inputs of the XNOR gate) may have different timings.

The device may see an edge immediately before reset, but then it will be reset itself.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Blue Swirl]

On Sun, Oct 2, 2011 at 7:35 PM, Avi Kivity <avi@redhat.com> wrote:
>> >
>> > Can you give an example?  Can be theoretical, doesn't have to refer
>> > to real hardware.
>>
>> For example, outputs A and B should both be driven high by reset.
>> They
>> are connected to a XNOR gate, whose output is fed to edge triggered
>> device. The device should not see any edges outside of the reset
>> cycle, during reset cycle they are ignored.
>>
>
> I don't see the issue?  After phase 1 the two outputs will be high, after phase two they will be whatever the device logic computes.
>
> During phase 1 you may see an edge, but that also happens with real hardware.  The target device my see A and B driven high before it sees the reset pulse, and A and B (and the inputs of the XNOR gate) may have different timings.
>
> The device may see an edge immediately before reset, but then it will be reset itself.

The difference is that on real HW the edge will happen when the reset
line is still active, so it will be ignored.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Avi Kivity]

> >> For example, outputs A and B should both be driven high by reset.
> >> They
> >> are connected to a XNOR gate, whose output is fed to edge
> >> triggered
> >> device. The device should not see any edges outside of the reset
> >> cycle, during reset cycle they are ignored.
> >>
> >
> > I don't see the issue?  After phase 1 the two outputs will be high,
> > after phase two they will be whatever the device logic computes.
> >
> > During phase 1 you may see an edge, but that also happens with real
> > hardware.  The target device my see A and B driven high before it
> > sees the reset pulse, and A and B (and the inputs of the XNOR
> > gate) may have different timings.
> >
> > The device may see an edge immediately before reset, but then it
> > will be reset itself.
>
> The difference is that on real HW the edge will happen when the reset
> line is still active, so it will be ignored.
>

There is no way to guarantee this.  If A is driven high before the target device detects RESET, it will see the edge.

Of course real hardware has timing specs, but these are maximum latencies.  If the XNOR gate is especially fast today it can overtake the target device's reset edge detector.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Blue Swirl]

On Sun, Oct 2, 2011 at 7:47 PM, Avi Kivity <avi@redhat.com> wrote:
>> >> For example, outputs A and B should both be driven high by reset.
>> >> They
>> >> are connected to a XNOR gate, whose output is fed to edge
>> >> triggered
>> >> device. The device should not see any edges outside of the reset
>> >> cycle, during reset cycle they are ignored.
>> >>
>> >
>> > I don't see the issue?  After phase 1 the two outputs will be high,
>> > after phase two they will be whatever the device logic computes.
>> >
>> > During phase 1 you may see an edge, but that also happens with real
>> > hardware.  The target device my see A and B driven high before it
>> > sees the reset pulse, and A and B (and the inputs of the XNOR
>> > gate) may have different timings.
>> >
>> > The device may see an edge immediately before reset, but then it
>> > will be reset itself.
>>
>> The difference is that on real HW the edge will happen when the reset
>> line is still active, so it will be ignored.
>>
>
> There is no way to guarantee this.  If A is driven high before the target device detects RESET, it will see the edge.

That case is not what we have here, it would be equivalent of pulsing
qemu_irq reset lines for each device in order. This would be even
worse than what we have now.

> Of course real hardware has timing specs, but these are maximum latencies.  If the XNOR gate is especially fast today it can overtake the target device's reset edge detector.

Devices don't have reset edge detectors.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Avi Kivity]

> >
> > There is no way to guarantee this.  If A is driven high before the
> > target device detects RESET, it will see the edge.
>
> That case is not what we have here, it would be equivalent of pulsing
> qemu_irq reset lines for each device in order. This would be even
> worse than what we have now.


That's  not what I'm saying.


>
> > Of course real hardware has timing specs, but these are maximum
> > latencies.  If the XNOR gate is especially fast today it can
> > overtake the target device's reset edge detector.
>
> Devices don't have reset edge detectors.
>

Say the target device's output has an AND connecting #RESET and an input, to the output.  When #RESET is asserted, the input is driven low.  The output is connected to a counter.

When #RESET is asserted, the source device's A and B are raised high, with delay Da and Db.  If they are different, the XNOR gate generates a pulse with delay Dx.  If Dx is smaller than the AND gate's delay Drm, then the counter will count.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Blue Swirl]

On Sun, Oct 2, 2011 at 7:58 PM, Avi Kivity <avi@redhat.com> wrote:
>> >
>> > There is no way to guarantee this.  If A is driven high before the
>> > target device detects RESET, it will see the edge.
>>
>> That case is not what we have here, it would be equivalent of pulsing
>> qemu_irq reset lines for each device in order. This would be even
>> worse than what we have now.
>
>
> That's  not what I'm saying.
>
>
>>
>> > Of course real hardware has timing specs, but these are maximum
>> > latencies.  If the XNOR gate is especially fast today it can
>> > overtake the target device's reset edge detector.
>>
>> Devices don't have reset edge detectors.
>>
>
> Say the target device's output has an AND connecting #RESET and an input, to the output.  When #RESET is asserted, the input is driven low.  The output is connected to a counter.
>
> When #RESET is asserted, the source device's A and B are raised high, with delay Da and Db.  If they are different, the XNOR gate generates a pulse with delay Dx.  If Dx is smaller than the AND gate's delay Drm, then the counter will count.

On a real device, the reset and clocks are the few global signals
available, distributed to most places. The counter would be
constructed of JK flip flops and the reset line would be connected to
the clear line of the flip flops. Then the counters don't count while
reset is active.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Avi Kivity]

> >
> > Say the target device's output has an AND connecting #RESET and an
> > input, to the output.  When #RESET is asserted, the input is
> > driven low.  The output is connected to a counter.
> >
> > When #RESET is asserted, the source device's A and B are raised
> > high, with delay Da and Db.  If they are different, the XNOR gate
> > generates a pulse with delay Dx.  If Dx is smaller than the AND
> > gate's delay Drm, then the counter will count.
>
> On a real device, the reset and clocks are the few global signals
> available, distributed to most places. The counter would be
> constructed of JK flip flops and the reset line would be connected to
> the clear line of the flip flops. Then the counters don't count while
> reset is active.
>
>

You could have a momentary count if the RESET input's gate delay to the counter is slower than the other inputs.  It would be cleared immediately afterwards (when phase 1 sweeps into the counter as well).

What I'm saying is that RESET order isn't defined on real hardware either, due to signal propagation effects.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Blue Swirl]

On Sun, Oct 2, 2011 at 8:14 PM, Avi Kivity <avi@redhat.com> wrote:
>> >
>> > Say the target device's output has an AND connecting #RESET and an
>> > input, to the output.  When #RESET is asserted, the input is
>> > driven low.  The output is connected to a counter.
>> >
>> > When #RESET is asserted, the source device's A and B are raised
>> > high, with delay Da and Db.  If they are different, the XNOR gate
>> > generates a pulse with delay Dx.  If Dx is smaller than the AND
>> > gate's delay Drm, then the counter will count.
>>
>> On a real device, the reset and clocks are the few global signals
>> available, distributed to most places. The counter would be
>> constructed of JK flip flops and the reset line would be connected to
>> the clear line of the flip flops. Then the counters don't count while
>> reset is active.
>>
>>
>
> You could have a momentary count if the RESET input's gate delay to the counter is slower than the other inputs.  It would be cleared immediately afterwards (when phase 1 sweeps into the counter as well).
>
> What I'm saying is that RESET order isn't defined on real hardware either, due to signal propagation effects.

Yes, but there the millions of reset cycles help immensely to suppress
the effects.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Avi Kivity]

> >
> > What I'm saying is that RESET order isn't defined on real hardware
> > either, due to signal propagation effects.
> 
> Yes, but there the millions of reset cycles help immensely to
> suppress
> the effects.
> 

That's modeled correctly.  After the end of phase 1, everything is settled.  During phase 1, you can see some spikes, but you can see them on real hardware as well.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Blue Swirl]

On Sun, Oct 2, 2011 at 8:21 PM, Avi Kivity <avi@redhat.com> wrote:
>> >
>> > What I'm saying is that RESET order isn't defined on real hardware
>> > either, due to signal propagation effects.
>>
>> Yes, but there the millions of reset cycles help immensely to
>> suppress
>> the effects.
>>
>
> That's modeled correctly.  After the end of phase 1, everything is settled.  During phase 1, you can see some spikes, but you can see them on real hardware as well.

No. With two phase reset (like I understood it), at the beginning of
phase, everything internal is settled (registers reset), no I/O. After
the phase 1, starting the external I/O activities cause spikes but
these are not suppressed.

In my 3-phase version, the reset would be held during phase 2 just to
handle the spikes (registers continue to be reset as needed).

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Avi Kivity]

----- Original Message -----
> On Sun, Oct 2, 2011 at 8:21 PM, Avi Kivity <avi@redhat.com> wrote:
> >> >
> >> > What I'm saying is that RESET order isn't defined on real
> >> > hardware
> >> > either, due to signal propagation effects.
> >>
> >> Yes, but there the millions of reset cycles help immensely to
> >> suppress
> >> the effects.
> >>
> >
> > That's modeled correctly.  After the end of phase 1, everything is
> > settled.  During phase 1, you can see some spikes, but you can see
> > them on real hardware as well.
>
> No. With two phase reset (like I understood it), at the beginning of
> phase, everything internal is settled (registers reset), no I/O.
> After
> the phase 1, starting the external I/O activities cause spikes but
> these are not suppressed.

Phase 1 is qemu_irq_raise() on all RESET inputs.  During this period, outputs of devices that have seen the edge move to their RESET values, and they ignore inputs.  Because it doesn't happen simultaneously, some devices see those outputs moving and may act on them.  This corresponds to different devices having different propagation delays.

At the end of phase 1, everything is settled.  This corresponds to T = max(reset latency of all devices).

Phase 2 is qemu_irq_lower() on all RESET inputs.  At this time, inputs begin to be considered.

Between phase 1 and phase 2 are those millions of cycles (minus T above).


So yes, you see spikes, but you also see them on real hardware.

Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Blue Swirl]

On Sun, Oct 2, 2011 at 8:39 PM, Avi Kivity <avi@redhat.com> wrote:
>
>
> ----- Original Message -----
>> On Sun, Oct 2, 2011 at 8:21 PM, Avi Kivity <avi@redhat.com> wrote:
>> >> >
>> >> > What I'm saying is that RESET order isn't defined on real
>> >> > hardware
>> >> > either, due to signal propagation effects.
>> >>
>> >> Yes, but there the millions of reset cycles help immensely to
>> >> suppress
>> >> the effects.
>> >>
>> >
>> > That's modeled correctly.  After the end of phase 1, everything is
>> > settled.  During phase 1, you can see some spikes, but you can see
>> > them on real hardware as well.
>>
>> No. With two phase reset (like I understood it), at the beginning of
>> phase, everything internal is settled (registers reset), no I/O.
>> After
>> the phase 1, starting the external I/O activities cause spikes but
>> these are not suppressed.
>
> Phase 1 is qemu_irq_raise() on all RESET inputs.  During this period, outputs of devices that have seen the edge move to their RESET values, and they ignore inputs.  Because it doesn't happen simultaneously, some devices see those outputs moving and may act on them.  This corresponds to different devices having different propagation delays.
> At the end of phase 1, everything is settled.  This corresponds to T = max(reset latency of all devices).
>
> Phase 2 is qemu_irq_lower() on all RESET inputs.  At this time, inputs begin to be considered.
>
> Between phase 1 and phase 2 are those millions of cycles (minus T above).
>
>
> So yes, you see spikes, but you also see them on real hardware.

With this definition of phases, at least the DEV/PIC1/PIC2 example I
presented earlier isn't possible. PIC2 would ignore the new input, so
it would retain reset state.

[Qemu-devel] [PATCH 12/22] i8259: Switch to per-PIC IRQ update

[Jan Kiszka]

This converts pic_update_irq to work against a single PIC instead of the
complete cascade. Along this change, the required update after
pic_set_irq1 is now moved into that function.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/i8259.c |   59 ++++++++++++++++++++---------------------------------------
 1 files changed, 20 insertions(+), 39 deletions(-)

diff --git a/hw/i8259.c b/hw/i8259.c
index 3498c6b..53b86dd 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -118,39 +118,19 @@ static int pic_get_irq(PicState *s)
     }
 }
 
-static void pic_set_irq1(PicState *s, int irq, int level);
-
-/* raise irq to CPU if necessary. must be called every time the active
-   irq may change */
-static void pic_update_irq(PicState2 *s)
+/* Update INT output. Must be called every time the output may have changed. */
+static void pic_update_irq(PicState *s)
 {
-    int irq2, irq;
-
-    /* first look at slave pic */
-    irq2 = pic_get_irq(&s->pics[1]);
-    if (irq2 >= 0) {
-        /* if irq request by slave pic, signal master PIC */
-        pic_set_irq1(&s->pics[0], 2, 1);
-        pic_set_irq1(&s->pics[0], 2, 0);
-    }
-    /* look at requested irq */
-    irq = pic_get_irq(&s->pics[0]);
-    if (irq >= 0) {
-#if defined(DEBUG_PIC)
-        {
-            int i;
-            for(i = 0; i < 2; i++) {
-                printf("pic%d: imr=%x irr=%x padd=%d\n",
-                       i, s->pics[i].imr, s->pics[i].irr,
-                       s->pics[i].priority_add);
+    int irq;
 
-            }
-        }
-        printf("pic: cpu_interrupt\n");
-#endif
-        qemu_irq_raise(s->pics[0].int_out);
+    irq = pic_get_irq(s);
+    if (irq >= 0) {
+        DPRINTF("pic%d: imr=%x irr=%x padd=%d\n",
+                s == &s->pics_state->pics[0] ? 0 : 1, s->imr, s->irr,
+                s->priority_add);
+        qemu_irq_raise(s->int_out);
     } else {
-        qemu_irq_lower(s->pics[0].int_out);
+        qemu_irq_lower(s->int_out);
     }
 }
 
@@ -179,6 +159,7 @@ static void pic_set_irq1(PicState *s, int irq, int level)
             s->last_irr &= ~mask;
         }
     }
+    pic_update_irq(s);
 }
 
 #ifdef DEBUG_IRQ_LATENCY
@@ -205,7 +186,6 @@ static void i8259_set_irq(void *opaque, int irq, int level)
     }
 #endif
     pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);
-    pic_update_irq(s);
 }
 
 /* acknowledge interrupt 'irq' */
@@ -220,6 +200,7 @@ static void pic_intack(PicState *s, int irq)
     /* We don't clear a level sensitive interrupt here */
     if (!(s->elcr & (1 << irq)))
         s->irr &= ~(1 << irq);
+    pic_update_irq(s);
 }
 
 int pic_read_irq(PicState2 *s)
@@ -246,7 +227,6 @@ int pic_read_irq(PicState2 *s)
         irq = 7;
         intno = s->pics[0].irq_base + irq;
     }
-    pic_update_irq(s);
 
 #if defined(DEBUG_PIC) || defined(DEBUG_IRQ_LATENCY)
     if (irq == 2) {
@@ -283,7 +263,7 @@ static void pic_reset(void *opaque)
     s->init4 = 0;
     s->single_mode = 0;
     /* Note: ELCR is not reset */
-    pic_update_irq(s->pics_state);
+    pic_update_irq(s);
 }
 
 static void pic_ioport_write(void *opaque, target_phys_addr_t addr64,
@@ -326,23 +306,23 @@ static void pic_ioport_write(void *opaque, target_phys_addr_t addr64,
                     s->isr &= ~(1 << irq);
                     if (cmd == 5)
                         s->priority_add = (irq + 1) & 7;
-                    pic_update_irq(s->pics_state);
+                    pic_update_irq(s);
                 }
                 break;
             case 3:
                 irq = val & 7;
                 s->isr &= ~(1 << irq);
-                pic_update_irq(s->pics_state);
+                pic_update_irq(s);
                 break;
             case 6:
                 s->priority_add = (val + 1) & 7;
-                pic_update_irq(s->pics_state);
+                pic_update_irq(s);
                 break;
             case 7:
                 irq = val & 7;
                 s->isr &= ~(1 << irq);
                 s->priority_add = (irq + 1) & 7;
-                pic_update_irq(s->pics_state);
+                pic_update_irq(s);
                 break;
             default:
                 /* no operation */
@@ -354,7 +334,7 @@ static void pic_ioport_write(void *opaque, target_phys_addr_t addr64,
         case 0:
             /* normal mode */
             s->imr = val;
-            pic_update_irq(s->pics_state);
+            pic_update_irq(s);
             break;
         case 1:
             s->irq_base = val & 0xf8;
@@ -390,8 +370,9 @@ static uint32_t pic_poll_read(PicState *s)
         }
         s->irr &= ~(1 << ret);
         s->isr &= ~(1 << ret);
-        if (slave || ret != 2)
-            pic_update_irq(s->pics_state);
+        if (slave || ret != 2) {
+            pic_update_irq(s);
+        }
     } else {
         ret = 0x07;
     }
-- 
1.7.3.4

[Qemu-devel] [PATCH 13/22] i8259: Fix poll command

[Jan Kiszka]

This was probably never used so far: According to the spec, polling
means ack'ing the pending IRQ and setting its corresponding bit in isr.
Moreover, we have to signal a pending IRQ via bit 7 of the returned
value, and we must not return a spurious IRQ if none is pending.

This implements the poll command without the help of pic_poll_read which
is left untouched as pic_intack_read is still using it.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/i8259.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/hw/i8259.c b/hw/i8259.c
index 53b86dd..849c82e 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -388,7 +388,13 @@ static uint64_t pic_ioport_read(void *opaque, target_phys_addr_t addr1,
     int ret;
 
     if (s->poll) {
-        ret = pic_poll_read(s);
+        ret = pic_get_irq(s);
+        if (ret >= 0) {
+            pic_intack(s, ret);
+            ret |= 0x80;
+        } else {
+            ret = 0;
+        }
         s->poll = 0;
     } else {
         if (addr == 0) {
-- 
1.7.3.4

[Qemu-devel] [PATCH 14/22] i8259: Clean up pic_ioport_read

[Jan Kiszka]

Drop redundant local address variable.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/i8259.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/hw/i8259.c b/hw/i8259.c
index 849c82e..9e32ceb 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -380,11 +380,10 @@ static uint32_t pic_poll_read(PicState *s)
     return ret;
 }
 
-static uint64_t pic_ioport_read(void *opaque, target_phys_addr_t addr1,
+static uint64_t pic_ioport_read(void *opaque, target_phys_addr_t addr,
                                 unsigned size)
 {
     PicState *s = opaque;
-    unsigned int addr = addr1;
     int ret;
 
     if (s->poll) {
-- 
1.7.3.4

[Qemu-devel] [PATCH 15/22] i8259: PREP: Replace pic_intack_read with pic_read_irq

[Jan Kiszka]

There is nothing in the i8259 spec that justifies the special
pic_intack_read. At least the Linux PREP kernels configure the PICs
properly so that pic_read_irq returns identical values, and setting
read_reg_select in PIC0 cannot be derived from any special i8259 mode.

So switch ppc_prep to pic_read_irq and drop the now unused PIC code.

CC: Alexander Graf <agraf@suse.de>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/i8259.c    |   39 ---------------------------------------
 hw/pc.h       |    1 -
 hw/ppc_prep.c |    2 +-
 3 files changed, 1 insertions(+), 41 deletions(-)

diff --git a/hw/i8259.c b/hw/i8259.c
index 9e32ceb..bbb12cf 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -356,30 +356,6 @@ static void pic_ioport_write(void *opaque, target_phys_addr_t addr64,
     }
 }
 
-static uint32_t pic_poll_read(PicState *s)
-{
-    int ret;
-
-    ret = pic_get_irq(s);
-    if (ret >= 0) {
-        bool slave = (s == &isa_pic->pics[1]);
-
-        if (slave) {
-            s->pics_state->pics[0].isr &= ~(1 << 2);
-            s->pics_state->pics[0].irr &= ~(1 << 2);
-        }
-        s->irr &= ~(1 << ret);
-        s->isr &= ~(1 << ret);
-        if (slave || ret != 2) {
-            pic_update_irq(s);
-        }
-    } else {
-        ret = 0x07;
-    }
-
-    return ret;
-}
-
 static uint64_t pic_ioport_read(void *opaque, target_phys_addr_t addr,
                                 unsigned size)
 {
@@ -409,21 +385,6 @@ static uint64_t pic_ioport_read(void *opaque, target_phys_addr_t addr,
     return ret;
 }
 
-/* memory mapped interrupt status */
-/* XXX: may be the same than pic_read_irq() */
-uint32_t pic_intack_read(PicState2 *s)
-{
-    int ret;
-
-    ret = pic_poll_read(&s->pics[0]);
-    if (ret == 2)
-        ret = pic_poll_read(&s->pics[1]) + 8;
-    /* Prepare for ISR read */
-    s->pics[0].read_reg_select = 1;
-
-    return ret;
-}
-
 int pic_get_output(PicState2 *s)
 {
     return (pic_get_irq(&s->pics[0]) >= 0);
diff --git a/hw/pc.h b/hw/pc.h
index fd5f9b2..14c61a2 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -65,7 +65,6 @@ extern PicState2 *isa_pic;
 qemu_irq *i8259_init(qemu_irq parent_irq);
 int pic_read_irq(PicState2 *s);
 int pic_get_output(PicState2 *s);
-uint32_t pic_intack_read(PicState2 *s);
 void pic_info(Monitor *mon);
 void irq_info(Monitor *mon);
 
diff --git a/hw/ppc_prep.c b/hw/ppc_prep.c
index d26049b..6427baa 100644
--- a/hw/ppc_prep.c
+++ b/hw/ppc_prep.c
@@ -130,7 +130,7 @@ static inline uint32_t _PPC_intack_read(target_phys_addr_t addr)
     uint32_t retval = 0;
 
     if ((addr & 0xf) == 0)
-        retval = pic_intack_read(isa_pic);
+        retval = pic_read_irq(isa_pic);
 #if 0
     printf("%s: 0x" TARGET_FMT_plx " <= %08" PRIx32 "\n", __func__, addr,
            retval);
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH 15/22] i8259: PREP: Replace pic_intack_read with pic_read_irq

[Alexander Graf]

On 28.09.2011, at 13:01, Jan Kiszka wrote:

> There is nothing in the i8259 spec that justifies the special
> pic_intack_read. At least the Linux PREP kernels configure the PICs
> properly so that pic_read_irq returns identical values, and setting
> read_reg_select in PIC0 cannot be derived from any special i8259 mode.
> 
> So switch ppc_prep to pic_read_irq and drop the now unused PIC code.

Andreas is maintaining PREP :)


Alex

[Qemu-devel] [PATCH 16/22] i8259: Replace PicState::pics_state with master flag

[Jan Kiszka]

This reflects how real PICs indentify their role (in non-buffered mode):
Pass the state of the /SP input on pic_init and use it instead of
pics_state to differentiate between master and slave mode.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/i8259.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/hw/i8259.c b/hw/i8259.c
index bbb12cf..9a4f112 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -59,7 +59,7 @@ typedef struct PicState {
     uint8_t elcr; /* PIIX edge/trigger selection*/
     uint8_t elcr_mask;
     qemu_irq int_out;
-    PicState2 *pics_state;
+    bool master; /* reflects /SP input pin */
     MemoryRegion base_io;
     MemoryRegion elcr_io;
 } PicState;
@@ -107,8 +107,9 @@ static int pic_get_irq(PicState *s)
     mask = s->isr;
     if (s->special_mask)
         mask &= ~s->imr;
-    if (s->special_fully_nested_mode && s == &s->pics_state->pics[0])
+    if (s->special_fully_nested_mode && s->master) {
         mask &= ~(1 << 2);
+    }
     cur_priority = get_priority(s, mask);
     if (priority < cur_priority) {
         /* higher priority found: an irq should be generated */
@@ -126,8 +127,7 @@ static void pic_update_irq(PicState *s)
     irq = pic_get_irq(s);
     if (irq >= 0) {
         DPRINTF("pic%d: imr=%x irr=%x padd=%d\n",
-                s == &s->pics_state->pics[0] ? 0 : 1, s->imr, s->irr,
-                s->priority_add);
+                s->master ? 0 : 1, s->imr, s->irr, s->priority_add);
         qemu_irq_raise(s->int_out);
     } else {
         qemu_irq_lower(s->int_out);
@@ -449,9 +449,11 @@ static const MemoryRegionOps pic_elcr_ioport_ops = {
 };
 
 /* XXX: add generic master/slave system */
-static void pic_init(int io_addr, int elcr_addr, PicState *s, qemu_irq int_out)
+static void pic_init(int io_addr, int elcr_addr, PicState *s, qemu_irq int_out,
+                     bool master)
 {
     s->int_out = int_out;
+    s->master = master;
 
     memory_region_init_io(&s->base_io, &pic_base_ioport_ops, s, "pic", 2);
     memory_region_init_io(&s->elcr_io, &pic_elcr_ioport_ops, s, "elcr", 1);
@@ -507,12 +509,10 @@ qemu_irq *i8259_init(qemu_irq parent_irq)
 
     s = g_malloc0(sizeof(PicState2));
     irqs = qemu_allocate_irqs(i8259_set_irq, s, 16);
-    pic_init(0x20, 0x4d0, &s->pics[0], parent_irq);
-    pic_init(0xa0, 0x4d1, &s->pics[1], irqs[2]);
+    pic_init(0x20, 0x4d0, &s->pics[0], parent_irq, true);
+    pic_init(0xa0, 0x4d1, &s->pics[1], irqs[2], false);
     s->pics[0].elcr_mask = 0xf8;
     s->pics[1].elcr_mask = 0xde;
-    s->pics[0].pics_state = s;
-    s->pics[1].pics_state = s;
     isa_pic = s;
     return irqs;
 }
-- 
1.7.3.4

[Qemu-devel] [PATCH 17/22] i8259: Eliminate PicState2

[Jan Kiszka]

Introduce a reference to the slave PIC for the few cases we need to
access it without a proper pointer at hand and drop PicState2. We could
even live without slave_pic if we had a better way of modeling the
cascade bus the PICs are attached to (in addition to the ISA bus).

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/i8259.c |   61 +++++++++++++++++++++++++++++------------------------------
 hw/pc.h    |    8 +++---
 2 files changed, 34 insertions(+), 35 deletions(-)

diff --git a/hw/i8259.c b/hw/i8259.c
index 9a4f112..cf0c6ed 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -40,7 +40,7 @@
 //#define DEBUG_IRQ_LATENCY
 //#define DEBUG_IRQ_COUNT
 
-typedef struct PicState {
+struct PicState {
     uint8_t last_irr; /* edge detection */
     uint8_t irr; /* interrupt request register */
     uint8_t imr; /* interrupt mask register */
@@ -62,13 +62,6 @@ typedef struct PicState {
     bool master; /* reflects /SP input pin */
     MemoryRegion base_io;
     MemoryRegion elcr_io;
-} PicState;
-
-struct PicState2 {
-    /* 0 is master pic, 1 is slave pic */
-    /* XXX: better separation between the two pics */
-    PicState pics[2];
-    void *irq_request_opaque;
 };
 
 #if defined(DEBUG_PIC) || defined (DEBUG_IRQ_COUNT)
@@ -77,7 +70,8 @@ static int irq_level[16];
 #ifdef DEBUG_IRQ_COUNT
 static uint64_t irq_count[16];
 #endif
-PicState2 *isa_pic;
+PicState *isa_pic;
+static PicState *slave_pic;
 
 /* return the highest priority found in mask (highest = smallest
    number). Return 8 if no irq */
@@ -168,7 +162,7 @@ int64_t irq_time[16];
 
 static void i8259_set_irq(void *opaque, int irq, int level)
 {
-    PicState2 *s = opaque;
+    PicState *s = irq <= 7 ? isa_pic : slave_pic;
 
 #if defined(DEBUG_PIC) || defined(DEBUG_IRQ_COUNT)
     if (level != irq_level[irq]) {
@@ -185,7 +179,7 @@ static void i8259_set_irq(void *opaque, int irq, int level)
         irq_time[irq] = qemu_get_clock_ns(vm_clock);
     }
 #endif
-    pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);
+    pic_set_irq1(s, irq & 7, level);
 }
 
 /* acknowledge interrupt 'irq' */
@@ -203,29 +197,29 @@ static void pic_intack(PicState *s, int irq)
     pic_update_irq(s);
 }
 
-int pic_read_irq(PicState2 *s)
+int pic_read_irq(PicState *s)
 {
     int irq, irq2, intno;
 
-    irq = pic_get_irq(&s->pics[0]);
+    irq = pic_get_irq(s);
     if (irq >= 0) {
         if (irq == 2) {
-            irq2 = pic_get_irq(&s->pics[1]);
+            irq2 = pic_get_irq(slave_pic);
             if (irq2 >= 0) {
-                pic_intack(&s->pics[1], irq2);
+                pic_intack(slave_pic, irq2);
             } else {
                 /* spurious IRQ on slave controller */
                 irq2 = 7;
             }
-            intno = s->pics[1].irq_base + irq2;
+            intno = slave_pic->irq_base + irq2;
         } else {
-            intno = s->pics[0].irq_base + irq;
+            intno = s->irq_base + irq;
         }
-        pic_intack(&s->pics[0], irq);
+        pic_intack(s, irq);
     } else {
         /* spurious IRQ on host controller */
         irq = 7;
-        intno = s->pics[0].irq_base + irq;
+        intno = s->irq_base + irq;
     }
 
 #if defined(DEBUG_PIC) || defined(DEBUG_IRQ_LATENCY)
@@ -385,9 +379,9 @@ static uint64_t pic_ioport_read(void *opaque, target_phys_addr_t addr,
     return ret;
 }
 
-int pic_get_output(PicState2 *s)
+int pic_get_output(PicState *s)
 {
-    return (pic_get_irq(&s->pics[0]) >= 0);
+    return (pic_get_irq(s) >= 0);
 }
 
 static void elcr_ioport_write(void *opaque, target_phys_addr_t addr,
@@ -475,8 +469,8 @@ void pic_info(Monitor *mon)
     if (!isa_pic)
         return;
 
-    for(i=0;i<2;i++) {
-        s = &isa_pic->pics[i];
+    for (i = 0; i < 2; i++) {
+        s = i == 0 ? isa_pic : slave_pic;
         monitor_printf(mon, "pic%d: irr=%02x imr=%02x isr=%02x hprio=%d "
                        "irq_base=%02x rr_sel=%d elcr=%02x fnm=%d\n",
                        i, s->irr, s->imr, s->isr, s->priority_add,
@@ -505,14 +499,19 @@ void irq_info(Monitor *mon)
 qemu_irq *i8259_init(qemu_irq parent_irq)
 {
     qemu_irq *irqs;
-    PicState2 *s;
-
-    s = g_malloc0(sizeof(PicState2));
-    irqs = qemu_allocate_irqs(i8259_set_irq, s, 16);
-    pic_init(0x20, 0x4d0, &s->pics[0], parent_irq, true);
-    pic_init(0xa0, 0x4d1, &s->pics[1], irqs[2], false);
-    s->pics[0].elcr_mask = 0xf8;
-    s->pics[1].elcr_mask = 0xde;
+    PicState *s;
+
+    irqs = qemu_allocate_irqs(i8259_set_irq, NULL, 16);
+
+    s = g_malloc0(sizeof(PicState));
+    pic_init(0x20, 0x4d0, s, parent_irq, true);
+    s->elcr_mask = 0xf8;
     isa_pic = s;
+
+    s = g_malloc0(sizeof(PicState));
+    pic_init(0xa0, 0x4d1, s, irqs[2], false);
+    s->elcr_mask = 0xde;
+    slave_pic = s;
+
     return irqs;
 }
diff --git a/hw/pc.h b/hw/pc.h
index 14c61a2..bfe3dd1 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -60,11 +60,11 @@ bool parallel_mm_init(target_phys_addr_t base, int it_shift, qemu_irq irq,
 
 /* i8259.c */
 
-typedef struct PicState2 PicState2;
-extern PicState2 *isa_pic;
+typedef struct PicState PicState;
+extern PicState *isa_pic;
 qemu_irq *i8259_init(qemu_irq parent_irq);
-int pic_read_irq(PicState2 *s);
-int pic_get_output(PicState2 *s);
+int pic_read_irq(PicState *s);
+int pic_get_output(PicState *s);
 void pic_info(Monitor *mon);
 void irq_info(Monitor *mon);
 
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH 17/22] i8259: Eliminate PicState2

[Richard Henderson]

On 09/28/2011 04:01 AM, Jan Kiszka wrote:
> +    s = g_malloc0(sizeof(PicState));

Probably better to convert to g_new0 at this time.


r~

Re: [Qemu-devel] [PATCH 17/22] i8259: Eliminate PicState2

[Richard Henderson]

On 09/28/2011 09:23 AM, Richard Henderson wrote:
> On 09/28/2011 04:01 AM, Jan Kiszka wrote:
>> +    s = g_malloc0(sizeof(PicState));
> 
> Probably better to convert to g_new0 at this time.

Nevermind.  I now see this code goes away again in a few patches.


r~

[Qemu-devel] [PATCH 18/22] qdev: Add HEX8 property

[Jan Kiszka]

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/qdev-properties.c |   29 +++++++++++++++++++++++++++++
 hw/qdev.h            |    3 +++
 2 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index e0e54aa..f0b811c 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -93,6 +93,35 @@ PropertyInfo qdev_prop_uint8 = {
     .print = print_uint8,
 };
 
+/* --- 8bit hex value --- */
+
+static int parse_hex8(DeviceState *dev, Property *prop, const char *str)
+{
+    uint8_t *ptr = qdev_get_prop_ptr(dev, prop);
+    char *end;
+
+    *ptr = strtoul(str, &end, 16);
+    if ((*end != '\0') || (end == str)) {
+        return -EINVAL;
+    }
+
+    return 0;
+}
+
+static int print_hex8(DeviceState *dev, Property *prop, char *dest, size_t len)
+{
+    uint8_t *ptr = qdev_get_prop_ptr(dev, prop);
+    return snprintf(dest, len, "0x%" PRIx8, *ptr);
+}
+
+PropertyInfo qdev_prop_hex8 = {
+    .name  = "hex8",
+    .type  = PROP_TYPE_UINT8,
+    .size  = sizeof(uint8_t),
+    .parse = parse_hex8,
+    .print = print_hex8,
+};
+
 /* --- 16bit integer --- */
 
 static int parse_uint16(DeviceState *dev, Property *prop, const char *str)
diff --git a/hw/qdev.h b/hw/qdev.h
index 8a13ec9..aa7ae36 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -224,6 +224,7 @@ extern PropertyInfo qdev_prop_uint16;
 extern PropertyInfo qdev_prop_uint32;
 extern PropertyInfo qdev_prop_int32;
 extern PropertyInfo qdev_prop_uint64;
+extern PropertyInfo qdev_prop_hex8;
 extern PropertyInfo qdev_prop_hex32;
 extern PropertyInfo qdev_prop_hex64;
 extern PropertyInfo qdev_prop_string;
@@ -267,6 +268,8 @@ extern PropertyInfo qdev_prop_pci_devfn;
     DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_int32, int32_t)
 #define DEFINE_PROP_UINT64(_n, _s, _f, _d)                      \
     DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_uint64, uint64_t)
+#define DEFINE_PROP_HEX8(_n, _s, _f, _d)                       \
+    DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_hex8, uint8_t)
 #define DEFINE_PROP_HEX32(_n, _s, _f, _d)                       \
     DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_hex32, uint32_t)
 #define DEFINE_PROP_HEX64(_n, _s, _f, _d)                       \
-- 
1.7.3.4

[Qemu-devel] [PATCH 19/22] i8259: Convert to qdev

[Jan Kiszka]

This key cleanup step requires to move the IRQ debugging bit from
i8259_set_irq directly to the per-PIC pic_set_irq, to pass the PIC
parameters (I/O base, ELCR address and mask, master/slave mode) as
qdev properties, and to interconnect the PICs with their environment via
GPIO pins.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/i8259.c |  158 ++++++++++++++++++++++++++++++++++++++----------------------
 1 files changed, 100 insertions(+), 58 deletions(-)

diff --git a/hw/i8259.c b/hw/i8259.c
index cf0c6ed..ae6f784 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -41,6 +41,7 @@
 //#define DEBUG_IRQ_COUNT
 
 struct PicState {
+    ISADevice dev;
     uint8_t last_irr; /* edge detection */
     uint8_t irr; /* interrupt request register */
     uint8_t imr; /* interrupt mask register */
@@ -58,8 +59,10 @@ struct PicState {
     uint8_t single_mode; /* true if slave pic is not initialized */
     uint8_t elcr; /* PIIX edge/trigger selection*/
     uint8_t elcr_mask;
-    qemu_irq int_out;
-    bool master; /* reflects /SP input pin */
+    qemu_irq int_out[1];
+    uint32_t master; /* reflects /SP input pin */
+    uint32_t iobase;
+    uint32_t elcr_addr;
     MemoryRegion base_io;
     MemoryRegion elcr_io;
 };
@@ -70,6 +73,9 @@ static int irq_level[16];
 #ifdef DEBUG_IRQ_COUNT
 static uint64_t irq_count[16];
 #endif
+#ifdef DEBUG_IRQ_LATENCY
+static int64_t irq_time[16];
+#endif
 PicState *isa_pic;
 static PicState *slave_pic;
 
@@ -122,17 +128,39 @@ static void pic_update_irq(PicState *s)
     if (irq >= 0) {
         DPRINTF("pic%d: imr=%x irr=%x padd=%d\n",
                 s->master ? 0 : 1, s->imr, s->irr, s->priority_add);
-        qemu_irq_raise(s->int_out);
+        qemu_irq_raise(s->int_out[0]);
     } else {
-        qemu_irq_lower(s->int_out);
+        qemu_irq_lower(s->int_out[0]);
     }
 }
 
 /* set irq level. If an edge is detected, then the IRR is set to 1 */
-static void pic_set_irq1(PicState *s, int irq, int level)
+static void pic_set_irq(void *opaque, int irq, int level)
 {
-    int mask;
-    mask = 1 << irq;
+    PicState *s = opaque;
+    int mask = 1 << irq;
+
+#if defined(DEBUG_PIC) || defined(DEBUG_IRQ_COUNT) || \
+    defined(DEBUG_IRQ_LATENCY)
+    int irq_index = s->master ? irq : irq + 8;
+#endif
+#if defined(DEBUG_PIC) || defined(DEBUG_IRQ_COUNT)
+    if (level != irq_level[irq_index]) {
+        DPRINTF("pic_set_irq: irq=%d level=%d\n", irq_index, level);
+        irq_level[irq_index] = level;
+#ifdef DEBUG_IRQ_COUNT
+        if (level == 1) {
+            irq_count[irq_index]++;
+        }
+#endif
+    }
+#endif
+#ifdef DEBUG_IRQ_LATENCY
+    if (level) {
+        irq_time[irq_index] = qemu_get_clock_ns(vm_clock);
+    }
+#endif
+
     if (s->elcr & mask) {
         /* level triggered */
         if (level) {
@@ -156,32 +184,6 @@ static void pic_set_irq1(PicState *s, int irq, int level)
     pic_update_irq(s);
 }
 
-#ifdef DEBUG_IRQ_LATENCY
-int64_t irq_time[16];
-#endif
-
-static void i8259_set_irq(void *opaque, int irq, int level)
-{
-    PicState *s = irq <= 7 ? isa_pic : slave_pic;
-
-#if defined(DEBUG_PIC) || defined(DEBUG_IRQ_COUNT)
-    if (level != irq_level[irq]) {
-        DPRINTF("i8259_set_irq: irq=%d level=%d\n", irq, level);
-        irq_level[irq] = level;
-#ifdef DEBUG_IRQ_COUNT
-	if (level == 1)
-	    irq_count[irq]++;
-#endif
-    }
-#endif
-#ifdef DEBUG_IRQ_LATENCY
-    if (level) {
-        irq_time[irq] = qemu_get_clock_ns(vm_clock);
-    }
-#endif
-    pic_set_irq1(s, irq & 7, level);
-}
-
 /* acknowledge interrupt 'irq' */
 static void pic_intack(PicState *s, int irq)
 {
@@ -237,9 +239,9 @@ int pic_read_irq(PicState *s)
     return intno;
 }
 
-static void pic_reset(void *opaque)
+static void pic_reset(DeviceState *dev)
 {
-    PicState *s = opaque;
+    PicState *s = container_of(dev, PicState, dev.qdev);
 
     s->last_irr = 0;
     s->irr = 0;
@@ -272,7 +274,7 @@ static void pic_ioport_write(void *opaque, target_phys_addr_t addr64,
     if (addr == 0) {
         if (val & 0x10) {
             /* init */
-            pic_reset(s);
+            pic_reset(&s->dev.qdev);
             s->init_state = 1;
             s->init4 = val & 1;
             s->single_mode = val & 2;
@@ -443,22 +445,22 @@ static const MemoryRegionOps pic_elcr_ioport_ops = {
 };
 
 /* XXX: add generic master/slave system */
-static void pic_init(int io_addr, int elcr_addr, PicState *s, qemu_irq int_out,
-                     bool master)
+static int pic_initfn(ISADevice *dev)
 {
-    s->int_out = int_out;
-    s->master = master;
+    PicState *s = DO_UPCAST(PicState, dev, dev);
 
     memory_region_init_io(&s->base_io, &pic_base_ioport_ops, s, "pic", 2);
     memory_region_init_io(&s->elcr_io, &pic_elcr_ioport_ops, s, "elcr", 1);
 
-    isa_register_ioport(NULL, &s->base_io, io_addr);
-    if (elcr_addr >= 0) {
-        isa_register_ioport(NULL, &s->elcr_io, elcr_addr);
-    }
+    isa_register_ioport(NULL, &s->base_io, s->iobase);
+    isa_register_ioport(NULL, &s->elcr_io, s->elcr_addr);
+
+    qdev_init_gpio_out(&dev->qdev, s->int_out, ARRAY_SIZE(s->int_out));
+    qdev_init_gpio_in(&dev->qdev, pic_set_irq, 8);
 
-    vmstate_register(NULL, io_addr, &vmstate_pic, s);
-    qemu_register_reset(pic_reset, s);
+    qdev_set_legacy_instance_id(&dev->qdev, s->iobase, 1);
+
+    return 0;
 }
 
 void pic_info(Monitor *mon)
@@ -498,20 +500,60 @@ void irq_info(Monitor *mon)
 
 qemu_irq *i8259_init(qemu_irq parent_irq)
 {
-    qemu_irq *irqs;
-    PicState *s;
+    qemu_irq *irq_set;
+    ISADevice *dev;
+    int i;
 
-    irqs = qemu_allocate_irqs(i8259_set_irq, NULL, 16);
+    irq_set = g_malloc(ISA_NUM_IRQS * sizeof(qemu_irq));
 
-    s = g_malloc0(sizeof(PicState));
-    pic_init(0x20, 0x4d0, s, parent_irq, true);
-    s->elcr_mask = 0xf8;
-    isa_pic = s;
+    dev = isa_create("isa-i8259");
+    qdev_prop_set_uint32(&dev->qdev, "iobase", 0x20);
+    qdev_prop_set_uint32(&dev->qdev, "elcr_addr", 0x4d0);
+    qdev_prop_set_uint8(&dev->qdev, "elcr_mask", 0xf8);
+    qdev_prop_set_bit(&dev->qdev, "master", true);
+    qdev_init_nofail(&dev->qdev);
 
-    s = g_malloc0(sizeof(PicState));
-    pic_init(0xa0, 0x4d1, s, irqs[2], false);
-    s->elcr_mask = 0xde;
-    slave_pic = s;
+    qdev_connect_gpio_out(&dev->qdev, 0, parent_irq);
+    for (i = 0 ; i < 8; i++) {
+        irq_set[i] = qdev_get_gpio_in(&dev->qdev, i);
+    }
+
+    isa_pic = DO_UPCAST(PicState, dev, dev);
+
+    dev = isa_create("isa-i8259");
+    qdev_prop_set_uint32(&dev->qdev, "iobase", 0xa0);
+    qdev_prop_set_uint32(&dev->qdev, "elcr_addr", 0x4d1);
+    qdev_prop_set_uint8(&dev->qdev, "elcr_mask", 0xde);
+    qdev_init_nofail(&dev->qdev);
+
+    qdev_connect_gpio_out(&dev->qdev, 0, irq_set[2]);
+    for (i = 0 ; i < 8; i++) {
+        irq_set[i + 8] = qdev_get_gpio_in(&dev->qdev, i);
+    }
+
+    slave_pic = DO_UPCAST(PicState, dev, dev);
 
-    return irqs;
+    return irq_set;
+}
+
+static ISADeviceInfo i8259_info = {
+    .qdev.name     = "isa-i8259",
+    .qdev.size     = sizeof(PicState),
+    .qdev.vmsd     = &vmstate_pic,
+    .qdev.reset    = pic_reset,
+    .qdev.no_user  = 1,
+    .init          = pic_initfn,
+    .qdev.props = (Property[]) {
+        DEFINE_PROP_HEX32("iobase", PicState, iobase,  -1),
+        DEFINE_PROP_HEX32("elcr_addr", PicState, elcr_addr,  -1),
+        DEFINE_PROP_HEX8("elcr_mask", PicState, elcr_mask,  -1),
+        DEFINE_PROP_BIT("master", PicState, master,  0, false),
+        DEFINE_PROP_END_OF_LIST(),
+    },
+};
+
+static void pic_register(void)
+{
+    isa_qdev_register(&i8259_info);
 }
+device_init(pic_register)
-- 
1.7.3.4

[Qemu-devel] [PATCH 20/22] i8259: Fix coding style

[Jan Kiszka]

No functional changes.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/i8259.c |   54 ++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 34 insertions(+), 20 deletions(-)

diff --git a/hw/i8259.c b/hw/i8259.c
index ae6f784..04ef0ca 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -67,7 +67,7 @@ struct PicState {
     MemoryRegion elcr_io;
 };
 
-#if defined(DEBUG_PIC) || defined (DEBUG_IRQ_COUNT)
+#if defined(DEBUG_PIC) || defined(DEBUG_IRQ_COUNT)
 static int irq_level[16];
 #endif
 #ifdef DEBUG_IRQ_COUNT
@@ -84,11 +84,14 @@ static PicState *slave_pic;
 static int get_priority(PicState *s, int mask)
 {
     int priority;
-    if (mask == 0)
+
+    if (mask == 0) {
         return 8;
+    }
     priority = 0;
-    while ((mask & (1 << ((priority + s->priority_add) & 7))) == 0)
+    while ((mask & (1 << ((priority + s->priority_add) & 7))) == 0) {
         priority++;
+    }
     return priority;
 }
 
@@ -99,14 +102,16 @@ static int pic_get_irq(PicState *s)
 
     mask = s->irr & ~s->imr;
     priority = get_priority(s, mask);
-    if (priority == 8)
+    if (priority == 8) {
         return -1;
+    }
     /* compute current priority. If special fully nested mode on the
        master, the IRQ coming from the slave is not taken into account
        for the priority computation. */
     mask = s->isr;
-    if (s->special_mask)
+    if (s->special_mask) {
         mask &= ~s->imr;
+    }
     if (s->special_fully_nested_mode && s->master) {
         mask &= ~(1 << 2);
     }
@@ -188,14 +193,16 @@ static void pic_set_irq(void *opaque, int irq, int level)
 static void pic_intack(PicState *s, int irq)
 {
     if (s->auto_eoi) {
-        if (s->rotate_on_auto_eoi)
+        if (s->rotate_on_auto_eoi) {
             s->priority_add = (irq + 1) & 7;
+        }
     } else {
         s->isr |= (1 << irq);
     }
     /* We don't clear a level sensitive interrupt here */
-    if (!(s->elcr & (1 << irq)))
+    if (!(s->elcr & (1 << irq))) {
         s->irr &= ~(1 << irq);
+    }
     pic_update_irq(s);
 }
 
@@ -278,18 +285,22 @@ static void pic_ioport_write(void *opaque, target_phys_addr_t addr64,
             s->init_state = 1;
             s->init4 = val & 1;
             s->single_mode = val & 2;
-            if (val & 0x08)
+            if (val & 0x08) {
                 hw_error("level sensitive irq not supported");
+            }
         } else if (val & 0x08) {
-            if (val & 0x04)
+            if (val & 0x04) {
                 s->poll = 1;
-            if (val & 0x02)
+            }
+            if (val & 0x02) {
                 s->read_reg_select = val & 1;
-            if (val & 0x40)
+            }
+            if (val & 0x40) {
                 s->special_mask = (val >> 5) & 1;
+            }
         } else {
             cmd = val >> 5;
-            switch(cmd) {
+            switch (cmd) {
             case 0:
             case 4:
                 s->rotate_on_auto_eoi = cmd >> 2;
@@ -300,8 +311,9 @@ static void pic_ioport_write(void *opaque, target_phys_addr_t addr64,
                 if (priority != 8) {
                     irq = (priority + s->priority_add) & 7;
                     s->isr &= ~(1 << irq);
-                    if (cmd == 5)
+                    if (cmd == 5) {
                         s->priority_add = (irq + 1) & 7;
+                    }
                     pic_update_irq(s);
                 }
                 break;
@@ -326,7 +338,7 @@ static void pic_ioport_write(void *opaque, target_phys_addr_t addr64,
             }
         }
     } else {
-        switch(s->init_state) {
+        switch (s->init_state) {
         case 0:
             /* normal mode */
             s->imr = val;
@@ -369,10 +381,11 @@ static uint64_t pic_ioport_read(void *opaque, target_phys_addr_t addr,
         s->poll = 0;
     } else {
         if (addr == 0) {
-            if (s->read_reg_select)
+            if (s->read_reg_select) {
                 ret = s->isr;
-            else
+            } else {
                 ret = s->irr;
+            }
         } else {
             ret = s->imr;
         }
@@ -405,7 +418,7 @@ static const VMStateDescription vmstate_pic = {
     .version_id = 1,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
-    .fields      = (VMStateField []) {
+    .fields = (VMStateField[]) {
         VMSTATE_UINT8(last_irr, PicState),
         VMSTATE_UINT8(irr, PicState),
         VMSTATE_UINT8(imr, PicState),
@@ -468,9 +481,9 @@ void pic_info(Monitor *mon)
     int i;
     PicState *s;
 
-    if (!isa_pic)
+    if (!isa_pic) {
         return;
-
+    }
     for (i = 0; i < 2; i++) {
         s = i == 0 ? isa_pic : slave_pic;
         monitor_printf(mon, "pic%d: irr=%02x imr=%02x isr=%02x hprio=%d "
@@ -492,8 +505,9 @@ void irq_info(Monitor *mon)
     monitor_printf(mon, "IRQ statistics:\n");
     for (i = 0; i < 16; i++) {
         count = irq_count[i];
-        if (count > 0)
+        if (count > 0) {
             monitor_printf(mon, "%2d: %" PRId64 "\n", i, count);
+        }
     }
 #endif
 }
-- 
1.7.3.4

[Qemu-devel] [PATCH 21/22] monitor: Restrict pic/irq_info to supporting targets

[Jan Kiszka]

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/an5206.c             |   10 ----------
 hw/arm_pic.c            |   11 -----------
 hw/cris_pic_cpu.c       |    6 ------
 hw/etraxfs.h            |    1 +
 hw/lm32_pic.c           |    4 ++--
 hw/lm32_pic.h           |    3 +++
 hw/microblaze_pic_cpu.c |    6 ------
 hw/s390-virtio.c        |   11 -----------
 hw/shix.c               |   11 -----------
 hw/slavio_intctl.c      |   14 ++++++++++----
 hw/sun4m.c              |   16 ++--------------
 hw/sun4m.h              |    6 ++++--
 hw/sun4u.c              |    8 --------
 hw/xtensa_pic.c         |   10 ----------
 monitor.c               |   21 +++++++++++++++++++++
 15 files changed, 43 insertions(+), 95 deletions(-)

diff --git a/hw/an5206.c b/hw/an5206.c
index 481ae60..3fe1f00 100644
--- a/hw/an5206.c
+++ b/hw/an5206.c
@@ -7,7 +7,6 @@
  */
 
 #include "hw.h"
-#include "pc.h"
 #include "mcf.h"
 #include "boards.h"
 #include "loader.h"
@@ -18,15 +17,6 @@
 #define AN5206_MBAR_ADDR 0x10000000
 #define AN5206_RAMBAR_ADDR 0x20000000
 
-/* Stub functions for hardware that doesn't exist.  */
-void pic_info(Monitor *mon)
-{
-}
-
-void irq_info(Monitor *mon)
-{
-}
-
 /* Board init.  */
 
 static void an5206_init(ram_addr_t ram_size,
diff --git a/hw/arm_pic.c b/hw/arm_pic.c
index 985148a..4e63845 100644
--- a/hw/arm_pic.c
+++ b/hw/arm_pic.c
@@ -8,19 +8,8 @@
  */
 
 #include "hw.h"
-#include "pc.h"
 #include "arm-misc.h"
 
-/* Stub functions for hardware that doesn't exist.  */
-void pic_info(Monitor *mon)
-{
-}
-
-void irq_info(Monitor *mon)
-{
-}
-
-
 /* Input 0 is IRQ and input 1 is FIQ.  */
 static void arm_pic_cpu_handler(void *opaque, int irq, int level)
 {
diff --git a/hw/cris_pic_cpu.c b/hw/cris_pic_cpu.c
index 7f1e4ab..06ae484 100644
--- a/hw/cris_pic_cpu.c
+++ b/hw/cris_pic_cpu.c
@@ -24,16 +24,10 @@
 
 #include "sysbus.h"
 #include "hw.h"
-#include "pc.h"
 #include "etraxfs.h"
 
 #define D(x)
 
-void pic_info(Monitor *mon)
-{}
-void irq_info(Monitor *mon)
-{}
-
 static void cris_pic_cpu_handler(void *opaque, int irq, int level)
 {
     CPUState *env = (CPUState *)opaque;
diff --git a/hw/etraxfs.h b/hw/etraxfs.h
index 1554b0b..24e8fd8 100644
--- a/hw/etraxfs.h
+++ b/hw/etraxfs.h
@@ -22,6 +22,7 @@
  * THE SOFTWARE.
  */
 
+#include "net.h"
 #include "etraxfs_dma.h"
 
 qemu_irq *cris_pic_init_cpu(CPUState *env);
diff --git a/hw/lm32_pic.c b/hw/lm32_pic.c
index 02941a7..8dd0050 100644
--- a/hw/lm32_pic.c
+++ b/hw/lm32_pic.c
@@ -39,7 +39,7 @@ struct LM32PicState {
 typedef struct LM32PicState LM32PicState;
 
 static LM32PicState *pic;
-void pic_info(Monitor *mon)
+void lm32_do_pic_info(Monitor *mon)
 {
     if (pic == NULL) {
         return;
@@ -49,7 +49,7 @@ void pic_info(Monitor *mon)
             pic->im, pic->ip, pic->irq_state);
 }
 
-void irq_info(Monitor *mon)
+void lm32_irq_info(Monitor *mon)
 {
     int i;
     uint32_t count;
diff --git a/hw/lm32_pic.h b/hw/lm32_pic.h
index e6479b8..14456f3 100644
--- a/hw/lm32_pic.h
+++ b/hw/lm32_pic.h
@@ -8,4 +8,7 @@ uint32_t lm32_pic_get_im(DeviceState *d);
 void lm32_pic_set_ip(DeviceState *d, uint32_t ip);
 void lm32_pic_set_im(DeviceState *d, uint32_t im);
 
+void lm32_do_pic_info(Monitor *mon);
+void lm32_irq_info(Monitor *mon);
+
 #endif /* QEMU_HW_LM32_PIC_H */
diff --git a/hw/microblaze_pic_cpu.c b/hw/microblaze_pic_cpu.c
index 9ad48b4..8b5623c 100644
--- a/hw/microblaze_pic_cpu.c
+++ b/hw/microblaze_pic_cpu.c
@@ -23,16 +23,10 @@
  */
 
 #include "hw.h"
-#include "pc.h"
 #include "microblaze_pic_cpu.h"
 
 #define D(x)
 
-void pic_info(Monitor *mon)
-{}
-void irq_info(Monitor *mon)
-{}
-
 static void microblaze_pic_cpu_handler(void *opaque, int irq, int level)
 {
     CPUState *env = (CPUState *)opaque;
diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
index acbf026..778cffe 100644
--- a/hw/s390-virtio.c
+++ b/hw/s390-virtio.c
@@ -62,17 +62,6 @@
 static VirtIOS390Bus *s390_bus;
 static CPUState **ipi_states;
 
-void irq_info(Monitor *mon);
-void pic_info(Monitor *mon);
-
-void irq_info(Monitor *mon)
-{
-}
-
-void pic_info(Monitor *mon)
-{
-}
-
 CPUState *s390_cpu_addr2state(uint16_t cpu_addr)
 {
     if (cpu_addr >= smp_cpus) {
diff --git a/hw/shix.c b/hw/shix.c
index 638bf16..dbf4764 100644
--- a/hw/shix.c
+++ b/hw/shix.c
@@ -28,7 +28,6 @@
    More information in target-sh4/README.sh4
 */
 #include "hw.h"
-#include "pc.h"
 #include "sh.h"
 #include "sysemu.h"
 #include "boards.h"
@@ -37,16 +36,6 @@
 #define BIOS_FILENAME "shix_bios.bin"
 #define BIOS_ADDRESS 0xA0000000
 
-void irq_info(Monitor *mon)
-{
-    /* XXXXX */
-}
-
-void pic_info(Monitor *mon)
-{
-    /* XXXXX */
-}
-
 static void shix_init(ram_addr_t ram_size,
                const char *boot_device,
 	       const char *kernel_filename, const char *kernel_cmdline,
diff --git a/hw/slavio_intctl.c b/hw/slavio_intctl.c
index 329c251..2d1dc12 100644
--- a/hw/slavio_intctl.c
+++ b/hw/slavio_intctl.c
@@ -204,13 +204,16 @@ static CPUWriteMemoryFunc * const slavio_intctlm_mem_write[3] = {
     slavio_intctlm_mem_writel,
 };
 
-void slavio_pic_info(Monitor *mon, DeviceState *dev)
+void slavio_pic_info(Monitor *mon)
 {
     SysBusDevice *sd;
     SLAVIO_INTCTLState *s;
     int i;
 
-    sd = sysbus_from_qdev(dev);
+    if (!slavio_intctl) {
+        return;
+    }
+    sd = sysbus_from_qdev(slavio_intctl);
     s = FROM_SYSBUS(SLAVIO_INTCTLState, sd);
     for (i = 0; i < MAX_CPUS; i++) {
         monitor_printf(mon, "per-cpu %d: pending 0x%08x\n", i,
@@ -220,7 +223,7 @@ void slavio_pic_info(Monitor *mon, DeviceState *dev)
                    s->intregm_pending, s->intregm_disabled);
 }
 
-void slavio_irq_info(Monitor *mon, DeviceState *dev)
+void slavio_irq_info(Monitor *mon)
 {
 #ifndef DEBUG_IRQ_COUNT
     monitor_printf(mon, "irq statistic code not compiled.\n");
@@ -230,7 +233,10 @@ void slavio_irq_info(Monitor *mon, DeviceState *dev)
     int i;
     int64_t count;
 
-    sd = sysbus_from_qdev(dev);
+    if (!slavio_intctl) {
+        return;
+    }
+    sd = sysbus_from_qdev(slavio_intctl);
     s = FROM_SYSBUS(SLAVIO_INTCTLState, sd);
     monitor_printf(mon, "IRQ statistics:\n");
     for (i = 0; i < 32; i++) {
diff --git a/hw/sun4m.c b/hw/sun4m.c
index dcaed38..589b505 100644
--- a/hw/sun4m.c
+++ b/hw/sun4m.c
@@ -135,6 +135,8 @@ struct sun4c_hwdef {
     uint8_t nvram_machine_id;
 };
 
+DeviceState *slavio_intctl;
+
 int DMA_get_channel_mode (int nchan)
 {
     return 0;
@@ -214,20 +216,6 @@ static void nvram_init(M48t59State *nvram, uint8_t *macaddr,
         m48t59_write(nvram, i, image[i]);
 }
 
-static DeviceState *slavio_intctl;
-
-void pic_info(Monitor *mon)
-{
-    if (slavio_intctl)
-        slavio_pic_info(mon, slavio_intctl);
-}
-
-void irq_info(Monitor *mon)
-{
-    if (slavio_intctl)
-        slavio_irq_info(mon, slavio_intctl);
-}
-
 void cpu_check_irqs(CPUState *env)
 {
     if (env->pil_in && (env->interrupt_index == 0 ||
diff --git a/hw/sun4m.h b/hw/sun4m.h
index ce97ee5..39a73aa 100644
--- a/hw/sun4m.h
+++ b/hw/sun4m.h
@@ -3,6 +3,8 @@
 
 #include "qemu-common.h"
 
+extern DeviceState *slavio_intctl;
+
 /* Devices used by sparc32 system.  */
 
 /* iommu.c */
@@ -23,8 +25,8 @@ static inline void sparc_iommu_memory_write(void *opaque,
 }
 
 /* slavio_intctl.c */
-void slavio_pic_info(Monitor *mon, DeviceState *dev);
-void slavio_irq_info(Monitor *mon, DeviceState *dev);
+void slavio_pic_info(Monitor *mon);
+void slavio_irq_info(Monitor *mon);
 
 /* sun4c_intctl.c */
 void sun4c_pic_info(Monitor *mon, void *opaque);
diff --git a/hw/sun4u.c b/hw/sun4u.c
index fbef350..568819d 100644
--- a/hw/sun4u.c
+++ b/hw/sun4u.c
@@ -242,14 +242,6 @@ static unsigned long sun4u_load_kernel(const char *kernel_filename,
     return kernel_size;
 }
 
-void pic_info(Monitor *mon)
-{
-}
-
-void irq_info(Monitor *mon)
-{
-}
-
 void cpu_check_irqs(CPUState *env)
 {
     uint32_t pil = env->pil_in |
diff --git a/hw/xtensa_pic.c b/hw/xtensa_pic.c
index 3033ae2..9357684 100644
--- a/hw/xtensa_pic.c
+++ b/hw/xtensa_pic.c
@@ -26,19 +26,9 @@
  */
 
 #include "hw.h"
-#include "pc.h"
 #include "qemu-log.h"
 #include "qemu-timer.h"
 
-/* Stub functions for hardware that doesn't exist.  */
-void pic_info(Monitor *mon)
-{
-}
-
-void irq_info(Monitor *mon)
-{
-}
-
 void xtensa_advance_ccount(CPUState *env, uint32_t d)
 {
     uint32_t old_ccount = env->sregs[CCOUNT];
diff --git a/monitor.c b/monitor.c
index 8ec2c5e..418161c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -64,6 +64,12 @@
 #include "trace/control.h"
 #include "ui/qemu-spice.h"
 
+/* for pic/irq_info */
+#if defined(TARGET_SPARC)
+#include "hw/sun4m.h"
+#endif
+#include "hw/lm32_pic.h"
+
 //#define DEBUG
 //#define DEBUG_COMPLETION
 
@@ -2937,20 +2943,35 @@ static const mon_cmd_t info_cmds[] = {
         .help       = "show the command line history",
         .mhandler.info = do_info_history,
     },
+#if defined(TARGET_I386) || defined(TARGET_PPC) || defined(TARGET_MIPS) || \
+    defined(TARGET_LM32) || (defined(TARGET_SPARC) && !defined(TARGET_SPARC64))
     {
         .name       = "irq",
         .args_type  = "",
         .params     = "",
         .help       = "show the interrupts statistics (if available)",
+#ifdef TARGET_SPARC
+        .mhandler.info = slavio_irq_info,
+#elif defined(TARGET_LM32)
+        .mhandler.info = lm32_irq_info,
+#else
         .mhandler.info = irq_info,
+#endif
     },
     {
         .name       = "pic",
         .args_type  = "",
         .params     = "",
         .help       = "show i8259 (PIC) state",
+#ifdef TARGET_SPARC
+        .mhandler.info = slavio_pic_info,
+#elif defined(TARGET_LM32)
+        .mhandler.info = lm32_do_pic_info,
+#else
         .mhandler.info = pic_info,
+#endif
     },
+#endif
     {
         .name       = "pci",
         .args_type  = "",
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH 21/22] monitor: Restrict pic/irq_info to supporting targets

[Blue Swirl]

On Wed, Sep 28, 2011 at 11:01 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  hw/an5206.c             |   10 ----------
>  hw/arm_pic.c            |   11 -----------
>  hw/cris_pic_cpu.c       |    6 ------
>  hw/etraxfs.h            |    1 +
>  hw/lm32_pic.c           |    4 ++--
>  hw/lm32_pic.h           |    3 +++
>  hw/microblaze_pic_cpu.c |    6 ------
>  hw/s390-virtio.c        |   11 -----------
>  hw/shix.c               |   11 -----------
>  hw/slavio_intctl.c      |   14 ++++++++++----
>  hw/sun4m.c              |   16 ++--------------
>  hw/sun4m.h              |    6 ++++--
>  hw/sun4u.c              |    8 --------
>  hw/xtensa_pic.c         |   10 ----------
>  monitor.c               |   21 +++++++++++++++++++++
>  15 files changed, 43 insertions(+), 95 deletions(-)
>
> diff --git a/hw/an5206.c b/hw/an5206.c
> index 481ae60..3fe1f00 100644
> --- a/hw/an5206.c
> +++ b/hw/an5206.c
> @@ -7,7 +7,6 @@
>  */
>
>  #include "hw.h"
> -#include "pc.h"
>  #include "mcf.h"
>  #include "boards.h"
>  #include "loader.h"
> @@ -18,15 +17,6 @@
>  #define AN5206_MBAR_ADDR 0x10000000
>  #define AN5206_RAMBAR_ADDR 0x20000000
>
> -/* Stub functions for hardware that doesn't exist.  */
> -void pic_info(Monitor *mon)
> -{
> -}
> -
> -void irq_info(Monitor *mon)
> -{
> -}
> -
>  /* Board init.  */
>
>  static void an5206_init(ram_addr_t ram_size,
> diff --git a/hw/arm_pic.c b/hw/arm_pic.c
> index 985148a..4e63845 100644
> --- a/hw/arm_pic.c
> +++ b/hw/arm_pic.c
> @@ -8,19 +8,8 @@
>  */
>
>  #include "hw.h"
> -#include "pc.h"
>  #include "arm-misc.h"
>
> -/* Stub functions for hardware that doesn't exist.  */
> -void pic_info(Monitor *mon)
> -{
> -}
> -
> -void irq_info(Monitor *mon)
> -{
> -}
> -
> -
>  /* Input 0 is IRQ and input 1 is FIQ.  */
>  static void arm_pic_cpu_handler(void *opaque, int irq, int level)
>  {
> diff --git a/hw/cris_pic_cpu.c b/hw/cris_pic_cpu.c
> index 7f1e4ab..06ae484 100644
> --- a/hw/cris_pic_cpu.c
> +++ b/hw/cris_pic_cpu.c
> @@ -24,16 +24,10 @@
>
>  #include "sysbus.h"
>  #include "hw.h"
> -#include "pc.h"
>  #include "etraxfs.h"
>
>  #define D(x)
>
> -void pic_info(Monitor *mon)
> -{}
> -void irq_info(Monitor *mon)
> -{}
> -
>  static void cris_pic_cpu_handler(void *opaque, int irq, int level)
>  {
>     CPUState *env = (CPUState *)opaque;
> diff --git a/hw/etraxfs.h b/hw/etraxfs.h
> index 1554b0b..24e8fd8 100644
> --- a/hw/etraxfs.h
> +++ b/hw/etraxfs.h
> @@ -22,6 +22,7 @@
>  * THE SOFTWARE.
>  */
>
> +#include "net.h"
>  #include "etraxfs_dma.h"
>
>  qemu_irq *cris_pic_init_cpu(CPUState *env);
> diff --git a/hw/lm32_pic.c b/hw/lm32_pic.c
> index 02941a7..8dd0050 100644
> --- a/hw/lm32_pic.c
> +++ b/hw/lm32_pic.c
> @@ -39,7 +39,7 @@ struct LM32PicState {
>  typedef struct LM32PicState LM32PicState;
>
>  static LM32PicState *pic;
> -void pic_info(Monitor *mon)
> +void lm32_do_pic_info(Monitor *mon)
>  {
>     if (pic == NULL) {
>         return;
> @@ -49,7 +49,7 @@ void pic_info(Monitor *mon)
>             pic->im, pic->ip, pic->irq_state);
>  }
>
> -void irq_info(Monitor *mon)
> +void lm32_irq_info(Monitor *mon)
>  {
>     int i;
>     uint32_t count;
> diff --git a/hw/lm32_pic.h b/hw/lm32_pic.h
> index e6479b8..14456f3 100644
> --- a/hw/lm32_pic.h
> +++ b/hw/lm32_pic.h
> @@ -8,4 +8,7 @@ uint32_t lm32_pic_get_im(DeviceState *d);
>  void lm32_pic_set_ip(DeviceState *d, uint32_t ip);
>  void lm32_pic_set_im(DeviceState *d, uint32_t im);
>
> +void lm32_do_pic_info(Monitor *mon);
> +void lm32_irq_info(Monitor *mon);
> +
>  #endif /* QEMU_HW_LM32_PIC_H */
> diff --git a/hw/microblaze_pic_cpu.c b/hw/microblaze_pic_cpu.c
> index 9ad48b4..8b5623c 100644
> --- a/hw/microblaze_pic_cpu.c
> +++ b/hw/microblaze_pic_cpu.c
> @@ -23,16 +23,10 @@
>  */
>
>  #include "hw.h"
> -#include "pc.h"
>  #include "microblaze_pic_cpu.h"
>
>  #define D(x)
>
> -void pic_info(Monitor *mon)
> -{}
> -void irq_info(Monitor *mon)
> -{}
> -
>  static void microblaze_pic_cpu_handler(void *opaque, int irq, int level)
>  {
>     CPUState *env = (CPUState *)opaque;
> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
> index acbf026..778cffe 100644
> --- a/hw/s390-virtio.c
> +++ b/hw/s390-virtio.c
> @@ -62,17 +62,6 @@
>  static VirtIOS390Bus *s390_bus;
>  static CPUState **ipi_states;
>
> -void irq_info(Monitor *mon);
> -void pic_info(Monitor *mon);
> -
> -void irq_info(Monitor *mon)
> -{
> -}
> -
> -void pic_info(Monitor *mon)
> -{
> -}
> -
>  CPUState *s390_cpu_addr2state(uint16_t cpu_addr)
>  {
>     if (cpu_addr >= smp_cpus) {
> diff --git a/hw/shix.c b/hw/shix.c
> index 638bf16..dbf4764 100644
> --- a/hw/shix.c
> +++ b/hw/shix.c
> @@ -28,7 +28,6 @@
>    More information in target-sh4/README.sh4
>  */
>  #include "hw.h"
> -#include "pc.h"
>  #include "sh.h"
>  #include "sysemu.h"
>  #include "boards.h"
> @@ -37,16 +36,6 @@
>  #define BIOS_FILENAME "shix_bios.bin"
>  #define BIOS_ADDRESS 0xA0000000
>
> -void irq_info(Monitor *mon)
> -{
> -    /* XXXXX */
> -}
> -
> -void pic_info(Monitor *mon)
> -{
> -    /* XXXXX */
> -}
> -
>  static void shix_init(ram_addr_t ram_size,
>                const char *boot_device,
>               const char *kernel_filename, const char *kernel_cmdline,
> diff --git a/hw/slavio_intctl.c b/hw/slavio_intctl.c
> index 329c251..2d1dc12 100644
> --- a/hw/slavio_intctl.c
> +++ b/hw/slavio_intctl.c
> @@ -204,13 +204,16 @@ static CPUWriteMemoryFunc * const slavio_intctlm_mem_write[3] = {
>     slavio_intctlm_mem_writel,
>  };
>
> -void slavio_pic_info(Monitor *mon, DeviceState *dev)
> +void slavio_pic_info(Monitor *mon)
>  {
>     SysBusDevice *sd;
>     SLAVIO_INTCTLState *s;
>     int i;
>
> -    sd = sysbus_from_qdev(dev);
> +    if (!slavio_intctl) {
> +        return;
> +    }
> +    sd = sysbus_from_qdev(slavio_intctl);
>     s = FROM_SYSBUS(SLAVIO_INTCTLState, sd);
>     for (i = 0; i < MAX_CPUS; i++) {
>         monitor_printf(mon, "per-cpu %d: pending 0x%08x\n", i,
> @@ -220,7 +223,7 @@ void slavio_pic_info(Monitor *mon, DeviceState *dev)
>                    s->intregm_pending, s->intregm_disabled);
>  }
>
> -void slavio_irq_info(Monitor *mon, DeviceState *dev)
> +void slavio_irq_info(Monitor *mon)
>  {
>  #ifndef DEBUG_IRQ_COUNT
>     monitor_printf(mon, "irq statistic code not compiled.\n");
> @@ -230,7 +233,10 @@ void slavio_irq_info(Monitor *mon, DeviceState *dev)
>     int i;
>     int64_t count;
>
> -    sd = sysbus_from_qdev(dev);
> +    if (!slavio_intctl) {
> +        return;
> +    }
> +    sd = sysbus_from_qdev(slavio_intctl);
>     s = FROM_SYSBUS(SLAVIO_INTCTLState, sd);
>     monitor_printf(mon, "IRQ statistics:\n");
>     for (i = 0; i < 32; i++) {
> diff --git a/hw/sun4m.c b/hw/sun4m.c
> index dcaed38..589b505 100644
> --- a/hw/sun4m.c
> +++ b/hw/sun4m.c
> @@ -135,6 +135,8 @@ struct sun4c_hwdef {
>     uint8_t nvram_machine_id;
>  };
>
> +DeviceState *slavio_intctl;

Instead of adding new global variables, could you add a registration
mechanism instead to pass the DeviceState used by the handler?

Or maybe I should revive this patch set:
http://lists.nongnu.org/archive/html/qemu-devel/2009-09/msg00459.html

> +
>  int DMA_get_channel_mode (int nchan)
>  {
>     return 0;
> @@ -214,20 +216,6 @@ static void nvram_init(M48t59State *nvram, uint8_t *macaddr,
>         m48t59_write(nvram, i, image[i]);
>  }
>
> -static DeviceState *slavio_intctl;
> -
> -void pic_info(Monitor *mon)
> -{
> -    if (slavio_intctl)
> -        slavio_pic_info(mon, slavio_intctl);
> -}
> -
> -void irq_info(Monitor *mon)
> -{
> -    if (slavio_intctl)
> -        slavio_irq_info(mon, slavio_intctl);
> -}
> -
>  void cpu_check_irqs(CPUState *env)
>  {
>     if (env->pil_in && (env->interrupt_index == 0 ||
> diff --git a/hw/sun4m.h b/hw/sun4m.h
> index ce97ee5..39a73aa 100644
> --- a/hw/sun4m.h
> +++ b/hw/sun4m.h
> @@ -3,6 +3,8 @@
>
>  #include "qemu-common.h"
>
> +extern DeviceState *slavio_intctl;
> +
>  /* Devices used by sparc32 system.  */
>
>  /* iommu.c */
> @@ -23,8 +25,8 @@ static inline void sparc_iommu_memory_write(void *opaque,
>  }
>
>  /* slavio_intctl.c */
> -void slavio_pic_info(Monitor *mon, DeviceState *dev);
> -void slavio_irq_info(Monitor *mon, DeviceState *dev);
> +void slavio_pic_info(Monitor *mon);
> +void slavio_irq_info(Monitor *mon);
>
>  /* sun4c_intctl.c */
>  void sun4c_pic_info(Monitor *mon, void *opaque);
> diff --git a/hw/sun4u.c b/hw/sun4u.c
> index fbef350..568819d 100644
> --- a/hw/sun4u.c
> +++ b/hw/sun4u.c
> @@ -242,14 +242,6 @@ static unsigned long sun4u_load_kernel(const char *kernel_filename,
>     return kernel_size;
>  }
>
> -void pic_info(Monitor *mon)
> -{
> -}
> -
> -void irq_info(Monitor *mon)
> -{
> -}
> -
>  void cpu_check_irqs(CPUState *env)
>  {
>     uint32_t pil = env->pil_in |
> diff --git a/hw/xtensa_pic.c b/hw/xtensa_pic.c
> index 3033ae2..9357684 100644
> --- a/hw/xtensa_pic.c
> +++ b/hw/xtensa_pic.c
> @@ -26,19 +26,9 @@
>  */
>
>  #include "hw.h"
> -#include "pc.h"
>  #include "qemu-log.h"
>  #include "qemu-timer.h"
>
> -/* Stub functions for hardware that doesn't exist.  */
> -void pic_info(Monitor *mon)
> -{
> -}
> -
> -void irq_info(Monitor *mon)
> -{
> -}
> -
>  void xtensa_advance_ccount(CPUState *env, uint32_t d)
>  {
>     uint32_t old_ccount = env->sregs[CCOUNT];
> diff --git a/monitor.c b/monitor.c
> index 8ec2c5e..418161c 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -64,6 +64,12 @@
>  #include "trace/control.h"
>  #include "ui/qemu-spice.h"
>
> +/* for pic/irq_info */
> +#if defined(TARGET_SPARC)
> +#include "hw/sun4m.h"
> +#endif
> +#include "hw/lm32_pic.h"
> +
>  //#define DEBUG
>  //#define DEBUG_COMPLETION
>
> @@ -2937,20 +2943,35 @@ static const mon_cmd_t info_cmds[] = {
>         .help       = "show the command line history",
>         .mhandler.info = do_info_history,
>     },
> +#if defined(TARGET_I386) || defined(TARGET_PPC) || defined(TARGET_MIPS) || \
> +    defined(TARGET_LM32) || (defined(TARGET_SPARC) && !defined(TARGET_SPARC64))
>     {
>         .name       = "irq",
>         .args_type  = "",
>         .params     = "",
>         .help       = "show the interrupts statistics (if available)",
> +#ifdef TARGET_SPARC
> +        .mhandler.info = slavio_irq_info,
> +#elif defined(TARGET_LM32)
> +        .mhandler.info = lm32_irq_info,
> +#else
>         .mhandler.info = irq_info,
> +#endif
>     },
>     {
>         .name       = "pic",
>         .args_type  = "",
>         .params     = "",
>         .help       = "show i8259 (PIC) state",
> +#ifdef TARGET_SPARC
> +        .mhandler.info = slavio_pic_info,
> +#elif defined(TARGET_LM32)
> +        .mhandler.info = lm32_do_pic_info,
> +#else
>         .mhandler.info = pic_info,
> +#endif
>     },
> +#endif
>     {
>         .name       = "pci",
>         .args_type  = "",
> --
> 1.7.3.4
>
>

Re: [Qemu-devel] [PATCH 21/22] monitor: Restrict pic/irq_info to supporting targets

[Jan Kiszka]

On 2011-09-28 20:19, Blue Swirl wrote:
> On Wed, Sep 28, 2011 at 11:01 AM, Jan Kiszka<jan.kiszka@siemens.com>  wrote:
>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>> ---
>>   hw/an5206.c             |   10 ----------
>>   hw/arm_pic.c            |   11 -----------
>>   hw/cris_pic_cpu.c       |    6 ------
>>   hw/etraxfs.h            |    1 +
>>   hw/lm32_pic.c           |    4 ++--
>>   hw/lm32_pic.h           |    3 +++
>>   hw/microblaze_pic_cpu.c |    6 ------
>>   hw/s390-virtio.c        |   11 -----------
>>   hw/shix.c               |   11 -----------
>>   hw/slavio_intctl.c      |   14 ++++++++++----
>>   hw/sun4m.c              |   16 ++--------------
>>   hw/sun4m.h              |    6 ++++--
>>   hw/sun4u.c              |    8 --------
>>   hw/xtensa_pic.c         |   10 ----------
>>   monitor.c               |   21 +++++++++++++++++++++
>>   15 files changed, 43 insertions(+), 95 deletions(-)
>>
>> diff --git a/hw/an5206.c b/hw/an5206.c
>> index 481ae60..3fe1f00 100644
>> --- a/hw/an5206.c
>> +++ b/hw/an5206.c
>> @@ -7,7 +7,6 @@
>>   */
>>
>>   #include "hw.h"
>> -#include "pc.h"
>>   #include "mcf.h"
>>   #include "boards.h"
>>   #include "loader.h"
>> @@ -18,15 +17,6 @@
>>   #define AN5206_MBAR_ADDR 0x10000000
>>   #define AN5206_RAMBAR_ADDR 0x20000000
>>
>> -/* Stub functions for hardware that doesn't exist.  */
>> -void pic_info(Monitor *mon)
>> -{
>> -}
>> -
>> -void irq_info(Monitor *mon)
>> -{
>> -}
>> -
>>   /* Board init.  */
>>
>>   static void an5206_init(ram_addr_t ram_size,
>> diff --git a/hw/arm_pic.c b/hw/arm_pic.c
>> index 985148a..4e63845 100644
>> --- a/hw/arm_pic.c
>> +++ b/hw/arm_pic.c
>> @@ -8,19 +8,8 @@
>>   */
>>
>>   #include "hw.h"
>> -#include "pc.h"
>>   #include "arm-misc.h"
>>
>> -/* Stub functions for hardware that doesn't exist.  */
>> -void pic_info(Monitor *mon)
>> -{
>> -}
>> -
>> -void irq_info(Monitor *mon)
>> -{
>> -}
>> -
>> -
>>   /* Input 0 is IRQ and input 1 is FIQ.  */
>>   static void arm_pic_cpu_handler(void *opaque, int irq, int level)
>>   {
>> diff --git a/hw/cris_pic_cpu.c b/hw/cris_pic_cpu.c
>> index 7f1e4ab..06ae484 100644
>> --- a/hw/cris_pic_cpu.c
>> +++ b/hw/cris_pic_cpu.c
>> @@ -24,16 +24,10 @@
>>
>>   #include "sysbus.h"
>>   #include "hw.h"
>> -#include "pc.h"
>>   #include "etraxfs.h"
>>
>>   #define D(x)
>>
>> -void pic_info(Monitor *mon)
>> -{}
>> -void irq_info(Monitor *mon)
>> -{}
>> -
>>   static void cris_pic_cpu_handler(void *opaque, int irq, int level)
>>   {
>>      CPUState *env = (CPUState *)opaque;
>> diff --git a/hw/etraxfs.h b/hw/etraxfs.h
>> index 1554b0b..24e8fd8 100644
>> --- a/hw/etraxfs.h
>> +++ b/hw/etraxfs.h
>> @@ -22,6 +22,7 @@
>>   * THE SOFTWARE.
>>   */
>>
>> +#include "net.h"
>>   #include "etraxfs_dma.h"
>>
>>   qemu_irq *cris_pic_init_cpu(CPUState *env);
>> diff --git a/hw/lm32_pic.c b/hw/lm32_pic.c
>> index 02941a7..8dd0050 100644
>> --- a/hw/lm32_pic.c
>> +++ b/hw/lm32_pic.c
>> @@ -39,7 +39,7 @@ struct LM32PicState {
>>   typedef struct LM32PicState LM32PicState;
>>
>>   static LM32PicState *pic;
>> -void pic_info(Monitor *mon)
>> +void lm32_do_pic_info(Monitor *mon)
>>   {
>>      if (pic == NULL) {
>>          return;
>> @@ -49,7 +49,7 @@ void pic_info(Monitor *mon)
>>              pic->im, pic->ip, pic->irq_state);
>>   }
>>
>> -void irq_info(Monitor *mon)
>> +void lm32_irq_info(Monitor *mon)
>>   {
>>      int i;
>>      uint32_t count;
>> diff --git a/hw/lm32_pic.h b/hw/lm32_pic.h
>> index e6479b8..14456f3 100644
>> --- a/hw/lm32_pic.h
>> +++ b/hw/lm32_pic.h
>> @@ -8,4 +8,7 @@ uint32_t lm32_pic_get_im(DeviceState *d);
>>   void lm32_pic_set_ip(DeviceState *d, uint32_t ip);
>>   void lm32_pic_set_im(DeviceState *d, uint32_t im);
>>
>> +void lm32_do_pic_info(Monitor *mon);
>> +void lm32_irq_info(Monitor *mon);
>> +
>>   #endif /* QEMU_HW_LM32_PIC_H */
>> diff --git a/hw/microblaze_pic_cpu.c b/hw/microblaze_pic_cpu.c
>> index 9ad48b4..8b5623c 100644
>> --- a/hw/microblaze_pic_cpu.c
>> +++ b/hw/microblaze_pic_cpu.c
>> @@ -23,16 +23,10 @@
>>   */
>>
>>   #include "hw.h"
>> -#include "pc.h"
>>   #include "microblaze_pic_cpu.h"
>>
>>   #define D(x)
>>
>> -void pic_info(Monitor *mon)
>> -{}
>> -void irq_info(Monitor *mon)
>> -{}
>> -
>>   static void microblaze_pic_cpu_handler(void *opaque, int irq, int level)
>>   {
>>      CPUState *env = (CPUState *)opaque;
>> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
>> index acbf026..778cffe 100644
>> --- a/hw/s390-virtio.c
>> +++ b/hw/s390-virtio.c
>> @@ -62,17 +62,6 @@
>>   static VirtIOS390Bus *s390_bus;
>>   static CPUState **ipi_states;
>>
>> -void irq_info(Monitor *mon);
>> -void pic_info(Monitor *mon);
>> -
>> -void irq_info(Monitor *mon)
>> -{
>> -}
>> -
>> -void pic_info(Monitor *mon)
>> -{
>> -}
>> -
>>   CPUState *s390_cpu_addr2state(uint16_t cpu_addr)
>>   {
>>      if (cpu_addr>= smp_cpus) {
>> diff --git a/hw/shix.c b/hw/shix.c
>> index 638bf16..dbf4764 100644
>> --- a/hw/shix.c
>> +++ b/hw/shix.c
>> @@ -28,7 +28,6 @@
>>     More information in target-sh4/README.sh4
>>   */
>>   #include "hw.h"
>> -#include "pc.h"
>>   #include "sh.h"
>>   #include "sysemu.h"
>>   #include "boards.h"
>> @@ -37,16 +36,6 @@
>>   #define BIOS_FILENAME "shix_bios.bin"
>>   #define BIOS_ADDRESS 0xA0000000
>>
>> -void irq_info(Monitor *mon)
>> -{
>> -    /* XXXXX */
>> -}
>> -
>> -void pic_info(Monitor *mon)
>> -{
>> -    /* XXXXX */
>> -}
>> -
>>   static void shix_init(ram_addr_t ram_size,
>>                 const char *boot_device,
>>                const char *kernel_filename, const char *kernel_cmdline,
>> diff --git a/hw/slavio_intctl.c b/hw/slavio_intctl.c
>> index 329c251..2d1dc12 100644
>> --- a/hw/slavio_intctl.c
>> +++ b/hw/slavio_intctl.c
>> @@ -204,13 +204,16 @@ static CPUWriteMemoryFunc * const slavio_intctlm_mem_write[3] = {
>>      slavio_intctlm_mem_writel,
>>   };
>>
>> -void slavio_pic_info(Monitor *mon, DeviceState *dev)
>> +void slavio_pic_info(Monitor *mon)
>>   {
>>      SysBusDevice *sd;
>>      SLAVIO_INTCTLState *s;
>>      int i;
>>
>> -    sd = sysbus_from_qdev(dev);
>> +    if (!slavio_intctl) {
>> +        return;
>> +    }
>> +    sd = sysbus_from_qdev(slavio_intctl);
>>      s = FROM_SYSBUS(SLAVIO_INTCTLState, sd);
>>      for (i = 0; i<  MAX_CPUS; i++) {
>>          monitor_printf(mon, "per-cpu %d: pending 0x%08x\n", i,
>> @@ -220,7 +223,7 @@ void slavio_pic_info(Monitor *mon, DeviceState *dev)
>>                     s->intregm_pending, s->intregm_disabled);
>>   }
>>
>> -void slavio_irq_info(Monitor *mon, DeviceState *dev)
>> +void slavio_irq_info(Monitor *mon)
>>   {
>>   #ifndef DEBUG_IRQ_COUNT
>>      monitor_printf(mon, "irq statistic code not compiled.\n");
>> @@ -230,7 +233,10 @@ void slavio_irq_info(Monitor *mon, DeviceState *dev)
>>      int i;
>>      int64_t count;
>>
>> -    sd = sysbus_from_qdev(dev);
>> +    if (!slavio_intctl) {
>> +        return;
>> +    }
>> +    sd = sysbus_from_qdev(slavio_intctl);
>>      s = FROM_SYSBUS(SLAVIO_INTCTLState, sd);
>>      monitor_printf(mon, "IRQ statistics:\n");
>>      for (i = 0; i<  32; i++) {
>> diff --git a/hw/sun4m.c b/hw/sun4m.c
>> index dcaed38..589b505 100644
>> --- a/hw/sun4m.c
>> +++ b/hw/sun4m.c
>> @@ -135,6 +135,8 @@ struct sun4c_hwdef {
>>      uint8_t nvram_machine_id;
>>   };
>>
>> +DeviceState *slavio_intctl;
>
> Instead of adding new global variables, could you add a registration
> mechanism instead to pass the DeviceState used by the handler?

Actually, I don't want to over-design this rather useless legacy monitor 
command, rather completely remove it once device_show is merged. This 
patch is just an intermediate step to enable moving i8259 to hwlib.

Jan

Re: [Qemu-devel] [PATCH 21/22] monitor: Restrict pic/irq_info to supporting targets

[Blue Swirl]

On Wed, Sep 28, 2011 at 9:26 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2011-09-28 20:19, Blue Swirl wrote:
>>
>> On Wed, Sep 28, 2011 at 11:01 AM, Jan Kiszka<jan.kiszka@siemens.com>
>>  wrote:
>>>
>>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>> ---
>>>  hw/an5206.c             |   10 ----------
>>>  hw/arm_pic.c            |   11 -----------
>>>  hw/cris_pic_cpu.c       |    6 ------
>>>  hw/etraxfs.h            |    1 +
>>>  hw/lm32_pic.c           |    4 ++--
>>>  hw/lm32_pic.h           |    3 +++
>>>  hw/microblaze_pic_cpu.c |    6 ------
>>>  hw/s390-virtio.c        |   11 -----------
>>>  hw/shix.c               |   11 -----------
>>>  hw/slavio_intctl.c      |   14 ++++++++++----
>>>  hw/sun4m.c              |   16 ++--------------
>>>  hw/sun4m.h              |    6 ++++--
>>>  hw/sun4u.c              |    8 --------
>>>  hw/xtensa_pic.c         |   10 ----------
>>>  monitor.c               |   21 +++++++++++++++++++++
>>>  15 files changed, 43 insertions(+), 95 deletions(-)
>>>
>>> diff --git a/hw/an5206.c b/hw/an5206.c
>>> index 481ae60..3fe1f00 100644
>>> --- a/hw/an5206.c
>>> +++ b/hw/an5206.c
>>> @@ -7,7 +7,6 @@
>>>  */
>>>
>>>  #include "hw.h"
>>> -#include "pc.h"
>>>  #include "mcf.h"
>>>  #include "boards.h"
>>>  #include "loader.h"
>>> @@ -18,15 +17,6 @@
>>>  #define AN5206_MBAR_ADDR 0x10000000
>>>  #define AN5206_RAMBAR_ADDR 0x20000000
>>>
>>> -/* Stub functions for hardware that doesn't exist.  */
>>> -void pic_info(Monitor *mon)
>>> -{
>>> -}
>>> -
>>> -void irq_info(Monitor *mon)
>>> -{
>>> -}
>>> -
>>>  /* Board init.  */
>>>
>>>  static void an5206_init(ram_addr_t ram_size,
>>> diff --git a/hw/arm_pic.c b/hw/arm_pic.c
>>> index 985148a..4e63845 100644
>>> --- a/hw/arm_pic.c
>>> +++ b/hw/arm_pic.c
>>> @@ -8,19 +8,8 @@
>>>  */
>>>
>>>  #include "hw.h"
>>> -#include "pc.h"
>>>  #include "arm-misc.h"
>>>
>>> -/* Stub functions for hardware that doesn't exist.  */
>>> -void pic_info(Monitor *mon)
>>> -{
>>> -}
>>> -
>>> -void irq_info(Monitor *mon)
>>> -{
>>> -}
>>> -
>>> -
>>>  /* Input 0 is IRQ and input 1 is FIQ.  */
>>>  static void arm_pic_cpu_handler(void *opaque, int irq, int level)
>>>  {
>>> diff --git a/hw/cris_pic_cpu.c b/hw/cris_pic_cpu.c
>>> index 7f1e4ab..06ae484 100644
>>> --- a/hw/cris_pic_cpu.c
>>> +++ b/hw/cris_pic_cpu.c
>>> @@ -24,16 +24,10 @@
>>>
>>>  #include "sysbus.h"
>>>  #include "hw.h"
>>> -#include "pc.h"
>>>  #include "etraxfs.h"
>>>
>>>  #define D(x)
>>>
>>> -void pic_info(Monitor *mon)
>>> -{}
>>> -void irq_info(Monitor *mon)
>>> -{}
>>> -
>>>  static void cris_pic_cpu_handler(void *opaque, int irq, int level)
>>>  {
>>>     CPUState *env = (CPUState *)opaque;
>>> diff --git a/hw/etraxfs.h b/hw/etraxfs.h
>>> index 1554b0b..24e8fd8 100644
>>> --- a/hw/etraxfs.h
>>> +++ b/hw/etraxfs.h
>>> @@ -22,6 +22,7 @@
>>>  * THE SOFTWARE.
>>>  */
>>>
>>> +#include "net.h"
>>>  #include "etraxfs_dma.h"
>>>
>>>  qemu_irq *cris_pic_init_cpu(CPUState *env);
>>> diff --git a/hw/lm32_pic.c b/hw/lm32_pic.c
>>> index 02941a7..8dd0050 100644
>>> --- a/hw/lm32_pic.c
>>> +++ b/hw/lm32_pic.c
>>> @@ -39,7 +39,7 @@ struct LM32PicState {
>>>  typedef struct LM32PicState LM32PicState;
>>>
>>>  static LM32PicState *pic;
>>> -void pic_info(Monitor *mon)
>>> +void lm32_do_pic_info(Monitor *mon)
>>>  {
>>>     if (pic == NULL) {
>>>         return;
>>> @@ -49,7 +49,7 @@ void pic_info(Monitor *mon)
>>>             pic->im, pic->ip, pic->irq_state);
>>>  }
>>>
>>> -void irq_info(Monitor *mon)
>>> +void lm32_irq_info(Monitor *mon)
>>>  {
>>>     int i;
>>>     uint32_t count;
>>> diff --git a/hw/lm32_pic.h b/hw/lm32_pic.h
>>> index e6479b8..14456f3 100644
>>> --- a/hw/lm32_pic.h
>>> +++ b/hw/lm32_pic.h
>>> @@ -8,4 +8,7 @@ uint32_t lm32_pic_get_im(DeviceState *d);
>>>  void lm32_pic_set_ip(DeviceState *d, uint32_t ip);
>>>  void lm32_pic_set_im(DeviceState *d, uint32_t im);
>>>
>>> +void lm32_do_pic_info(Monitor *mon);
>>> +void lm32_irq_info(Monitor *mon);
>>> +
>>>  #endif /* QEMU_HW_LM32_PIC_H */
>>> diff --git a/hw/microblaze_pic_cpu.c b/hw/microblaze_pic_cpu.c
>>> index 9ad48b4..8b5623c 100644
>>> --- a/hw/microblaze_pic_cpu.c
>>> +++ b/hw/microblaze_pic_cpu.c
>>> @@ -23,16 +23,10 @@
>>>  */
>>>
>>>  #include "hw.h"
>>> -#include "pc.h"
>>>  #include "microblaze_pic_cpu.h"
>>>
>>>  #define D(x)
>>>
>>> -void pic_info(Monitor *mon)
>>> -{}
>>> -void irq_info(Monitor *mon)
>>> -{}
>>> -
>>>  static void microblaze_pic_cpu_handler(void *opaque, int irq, int level)
>>>  {
>>>     CPUState *env = (CPUState *)opaque;
>>> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
>>> index acbf026..778cffe 100644
>>> --- a/hw/s390-virtio.c
>>> +++ b/hw/s390-virtio.c
>>> @@ -62,17 +62,6 @@
>>>  static VirtIOS390Bus *s390_bus;
>>>  static CPUState **ipi_states;
>>>
>>> -void irq_info(Monitor *mon);
>>> -void pic_info(Monitor *mon);
>>> -
>>> -void irq_info(Monitor *mon)
>>> -{
>>> -}
>>> -
>>> -void pic_info(Monitor *mon)
>>> -{
>>> -}
>>> -
>>>  CPUState *s390_cpu_addr2state(uint16_t cpu_addr)
>>>  {
>>>     if (cpu_addr>= smp_cpus) {
>>> diff --git a/hw/shix.c b/hw/shix.c
>>> index 638bf16..dbf4764 100644
>>> --- a/hw/shix.c
>>> +++ b/hw/shix.c
>>> @@ -28,7 +28,6 @@
>>>    More information in target-sh4/README.sh4
>>>  */
>>>  #include "hw.h"
>>> -#include "pc.h"
>>>  #include "sh.h"
>>>  #include "sysemu.h"
>>>  #include "boards.h"
>>> @@ -37,16 +36,6 @@
>>>  #define BIOS_FILENAME "shix_bios.bin"
>>>  #define BIOS_ADDRESS 0xA0000000
>>>
>>> -void irq_info(Monitor *mon)
>>> -{
>>> -    /* XXXXX */
>>> -}
>>> -
>>> -void pic_info(Monitor *mon)
>>> -{
>>> -    /* XXXXX */
>>> -}
>>> -
>>>  static void shix_init(ram_addr_t ram_size,
>>>                const char *boot_device,
>>>               const char *kernel_filename, const char *kernel_cmdline,
>>> diff --git a/hw/slavio_intctl.c b/hw/slavio_intctl.c
>>> index 329c251..2d1dc12 100644
>>> --- a/hw/slavio_intctl.c
>>> +++ b/hw/slavio_intctl.c
>>> @@ -204,13 +204,16 @@ static CPUWriteMemoryFunc * const
>>> slavio_intctlm_mem_write[3] = {
>>>     slavio_intctlm_mem_writel,
>>>  };
>>>
>>> -void slavio_pic_info(Monitor *mon, DeviceState *dev)
>>> +void slavio_pic_info(Monitor *mon)
>>>  {
>>>     SysBusDevice *sd;
>>>     SLAVIO_INTCTLState *s;
>>>     int i;
>>>
>>> -    sd = sysbus_from_qdev(dev);
>>> +    if (!slavio_intctl) {
>>> +        return;
>>> +    }
>>> +    sd = sysbus_from_qdev(slavio_intctl);
>>>     s = FROM_SYSBUS(SLAVIO_INTCTLState, sd);
>>>     for (i = 0; i<  MAX_CPUS; i++) {
>>>         monitor_printf(mon, "per-cpu %d: pending 0x%08x\n", i,
>>> @@ -220,7 +223,7 @@ void slavio_pic_info(Monitor *mon, DeviceState *dev)
>>>                    s->intregm_pending, s->intregm_disabled);
>>>  }
>>>
>>> -void slavio_irq_info(Monitor *mon, DeviceState *dev)
>>> +void slavio_irq_info(Monitor *mon)
>>>  {
>>>  #ifndef DEBUG_IRQ_COUNT
>>>     monitor_printf(mon, "irq statistic code not compiled.\n");
>>> @@ -230,7 +233,10 @@ void slavio_irq_info(Monitor *mon, DeviceState *dev)
>>>     int i;
>>>     int64_t count;
>>>
>>> -    sd = sysbus_from_qdev(dev);
>>> +    if (!slavio_intctl) {
>>> +        return;
>>> +    }
>>> +    sd = sysbus_from_qdev(slavio_intctl);
>>>     s = FROM_SYSBUS(SLAVIO_INTCTLState, sd);
>>>     monitor_printf(mon, "IRQ statistics:\n");
>>>     for (i = 0; i<  32; i++) {
>>> diff --git a/hw/sun4m.c b/hw/sun4m.c
>>> index dcaed38..589b505 100644
>>> --- a/hw/sun4m.c
>>> +++ b/hw/sun4m.c
>>> @@ -135,6 +135,8 @@ struct sun4c_hwdef {
>>>     uint8_t nvram_machine_id;
>>>  };
>>>
>>> +DeviceState *slavio_intctl;
>>
>> Instead of adding new global variables, could you add a registration
>> mechanism instead to pass the DeviceState used by the handler?
>
> Actually, I don't want to over-design this rather useless legacy monitor
> command, rather completely remove it once device_show is merged. This patch
> is just an intermediate step to enable moving i8259 to hwlib.

Fine, but then don't introduce the global variable but leave it static
at sun4m.c.

[Qemu-devel] [PATCH v2 21/22] monitor: Restrict pic/irq_info to supporting targets

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Changes in v2:
 - keep slavio_intctl local, introduce sun4m_pic/irq_info instead

 hw/an5206.c             |   10 ----------
 hw/arm_pic.c            |   11 -----------
 hw/cris_pic_cpu.c       |    6 ------
 hw/etraxfs.h            |    1 +
 hw/lm32_pic.c           |    4 ++--
 hw/lm32_pic.h           |    3 +++
 hw/microblaze_pic_cpu.c |    6 ------
 hw/s390-virtio.c        |   11 -----------
 hw/shix.c               |   11 -----------
 hw/sun4m.c              |    4 ++--
 hw/sun4m.h              |    4 ++++
 hw/sun4u.c              |    8 --------
 hw/xtensa_pic.c         |   10 ----------
 monitor.c               |   21 +++++++++++++++++++++
 14 files changed, 33 insertions(+), 77 deletions(-)

diff --git a/hw/an5206.c b/hw/an5206.c
index 481ae60..3fe1f00 100644
--- a/hw/an5206.c
+++ b/hw/an5206.c
@@ -7,7 +7,6 @@
  */
 
 #include "hw.h"
-#include "pc.h"
 #include "mcf.h"
 #include "boards.h"
 #include "loader.h"
@@ -18,15 +17,6 @@
 #define AN5206_MBAR_ADDR 0x10000000
 #define AN5206_RAMBAR_ADDR 0x20000000
 
-/* Stub functions for hardware that doesn't exist.  */
-void pic_info(Monitor *mon)
-{
-}
-
-void irq_info(Monitor *mon)
-{
-}
-
 /* Board init.  */
 
 static void an5206_init(ram_addr_t ram_size,
diff --git a/hw/arm_pic.c b/hw/arm_pic.c
index 985148a..4e63845 100644
--- a/hw/arm_pic.c
+++ b/hw/arm_pic.c
@@ -8,19 +8,8 @@
  */
 
 #include "hw.h"
-#include "pc.h"
 #include "arm-misc.h"
 
-/* Stub functions for hardware that doesn't exist.  */
-void pic_info(Monitor *mon)
-{
-}
-
-void irq_info(Monitor *mon)
-{
-}
-
-
 /* Input 0 is IRQ and input 1 is FIQ.  */
 static void arm_pic_cpu_handler(void *opaque, int irq, int level)
 {
diff --git a/hw/cris_pic_cpu.c b/hw/cris_pic_cpu.c
index 7f1e4ab..06ae484 100644
--- a/hw/cris_pic_cpu.c
+++ b/hw/cris_pic_cpu.c
@@ -24,16 +24,10 @@
 
 #include "sysbus.h"
 #include "hw.h"
-#include "pc.h"
 #include "etraxfs.h"
 
 #define D(x)
 
-void pic_info(Monitor *mon)
-{}
-void irq_info(Monitor *mon)
-{}
-
 static void cris_pic_cpu_handler(void *opaque, int irq, int level)
 {
     CPUState *env = (CPUState *)opaque;
diff --git a/hw/etraxfs.h b/hw/etraxfs.h
index 1554b0b..24e8fd8 100644
--- a/hw/etraxfs.h
+++ b/hw/etraxfs.h
@@ -22,6 +22,7 @@
  * THE SOFTWARE.
  */
 
+#include "net.h"
 #include "etraxfs_dma.h"
 
 qemu_irq *cris_pic_init_cpu(CPUState *env);
diff --git a/hw/lm32_pic.c b/hw/lm32_pic.c
index 02941a7..8dd0050 100644
--- a/hw/lm32_pic.c
+++ b/hw/lm32_pic.c
@@ -39,7 +39,7 @@ struct LM32PicState {
 typedef struct LM32PicState LM32PicState;
 
 static LM32PicState *pic;
-void pic_info(Monitor *mon)
+void lm32_do_pic_info(Monitor *mon)
 {
     if (pic == NULL) {
         return;
@@ -49,7 +49,7 @@ void pic_info(Monitor *mon)
             pic->im, pic->ip, pic->irq_state);
 }
 
-void irq_info(Monitor *mon)
+void lm32_irq_info(Monitor *mon)
 {
     int i;
     uint32_t count;
diff --git a/hw/lm32_pic.h b/hw/lm32_pic.h
index e6479b8..14456f3 100644
--- a/hw/lm32_pic.h
+++ b/hw/lm32_pic.h
@@ -8,4 +8,7 @@ uint32_t lm32_pic_get_im(DeviceState *d);
 void lm32_pic_set_ip(DeviceState *d, uint32_t ip);
 void lm32_pic_set_im(DeviceState *d, uint32_t im);
 
+void lm32_do_pic_info(Monitor *mon);
+void lm32_irq_info(Monitor *mon);
+
 #endif /* QEMU_HW_LM32_PIC_H */
diff --git a/hw/microblaze_pic_cpu.c b/hw/microblaze_pic_cpu.c
index 9ad48b4..8b5623c 100644
--- a/hw/microblaze_pic_cpu.c
+++ b/hw/microblaze_pic_cpu.c
@@ -23,16 +23,10 @@
  */
 
 #include "hw.h"
-#include "pc.h"
 #include "microblaze_pic_cpu.h"
 
 #define D(x)
 
-void pic_info(Monitor *mon)
-{}
-void irq_info(Monitor *mon)
-{}
-
 static void microblaze_pic_cpu_handler(void *opaque, int irq, int level)
 {
     CPUState *env = (CPUState *)opaque;
diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
index acbf026..778cffe 100644
--- a/hw/s390-virtio.c
+++ b/hw/s390-virtio.c
@@ -62,17 +62,6 @@
 static VirtIOS390Bus *s390_bus;
 static CPUState **ipi_states;
 
-void irq_info(Monitor *mon);
-void pic_info(Monitor *mon);
-
-void irq_info(Monitor *mon)
-{
-}
-
-void pic_info(Monitor *mon)
-{
-}
-
 CPUState *s390_cpu_addr2state(uint16_t cpu_addr)
 {
     if (cpu_addr >= smp_cpus) {
diff --git a/hw/shix.c b/hw/shix.c
index 638bf16..dbf4764 100644
--- a/hw/shix.c
+++ b/hw/shix.c
@@ -28,7 +28,6 @@
    More information in target-sh4/README.sh4
 */
 #include "hw.h"
-#include "pc.h"
 #include "sh.h"
 #include "sysemu.h"
 #include "boards.h"
@@ -37,16 +36,6 @@
 #define BIOS_FILENAME "shix_bios.bin"
 #define BIOS_ADDRESS 0xA0000000
 
-void irq_info(Monitor *mon)
-{
-    /* XXXXX */
-}
-
-void pic_info(Monitor *mon)
-{
-    /* XXXXX */
-}
-
 static void shix_init(ram_addr_t ram_size,
                const char *boot_device,
 	       const char *kernel_filename, const char *kernel_cmdline,
diff --git a/hw/sun4m.c b/hw/sun4m.c
index dcaed38..71bf648 100644
--- a/hw/sun4m.c
+++ b/hw/sun4m.c
@@ -216,13 +216,13 @@ static void nvram_init(M48t59State *nvram, uint8_t *macaddr,
 
 static DeviceState *slavio_intctl;
 
-void pic_info(Monitor *mon)
+void sun4m_pic_info(Monitor *mon)
 {
     if (slavio_intctl)
         slavio_pic_info(mon, slavio_intctl);
 }
 
-void irq_info(Monitor *mon)
+void sun4m_irq_info(Monitor *mon)
 {
     if (slavio_intctl)
         slavio_irq_info(mon, slavio_intctl);
diff --git a/hw/sun4m.h b/hw/sun4m.h
index ce97ee5..504c3af 100644
--- a/hw/sun4m.h
+++ b/hw/sun4m.h
@@ -30,6 +30,10 @@ void slavio_irq_info(Monitor *mon, DeviceState *dev);
 void sun4c_pic_info(Monitor *mon, void *opaque);
 void sun4c_irq_info(Monitor *mon, void *opaque);
 
+/* sun4m.c */
+void sun4m_pic_info(Monitor *mon);
+void sun4m_irq_info(Monitor *mon);
+
 /* sparc32_dma.c */
 #include "sparc32_dma.h"
 
diff --git a/hw/sun4u.c b/hw/sun4u.c
index fbef350..568819d 100644
--- a/hw/sun4u.c
+++ b/hw/sun4u.c
@@ -242,14 +242,6 @@ static unsigned long sun4u_load_kernel(const char *kernel_filename,
     return kernel_size;
 }
 
-void pic_info(Monitor *mon)
-{
-}
-
-void irq_info(Monitor *mon)
-{
-}
-
 void cpu_check_irqs(CPUState *env)
 {
     uint32_t pil = env->pil_in |
diff --git a/hw/xtensa_pic.c b/hw/xtensa_pic.c
index 3033ae2..9357684 100644
--- a/hw/xtensa_pic.c
+++ b/hw/xtensa_pic.c
@@ -26,19 +26,9 @@
  */
 
 #include "hw.h"
-#include "pc.h"
 #include "qemu-log.h"
 #include "qemu-timer.h"
 
-/* Stub functions for hardware that doesn't exist.  */
-void pic_info(Monitor *mon)
-{
-}
-
-void irq_info(Monitor *mon)
-{
-}
-
 void xtensa_advance_ccount(CPUState *env, uint32_t d)
 {
     uint32_t old_ccount = env->sregs[CCOUNT];
diff --git a/monitor.c b/monitor.c
index 8ec2c5e..04a3912 100644
--- a/monitor.c
+++ b/monitor.c
@@ -64,6 +64,12 @@
 #include "trace/control.h"
 #include "ui/qemu-spice.h"
 
+/* for pic/irq_info */
+#if defined(TARGET_SPARC)
+#include "hw/sun4m.h"
+#endif
+#include "hw/lm32_pic.h"
+
 //#define DEBUG
 //#define DEBUG_COMPLETION
 
@@ -2937,20 +2943,35 @@ static const mon_cmd_t info_cmds[] = {
         .help       = "show the command line history",
         .mhandler.info = do_info_history,
     },
+#if defined(TARGET_I386) || defined(TARGET_PPC) || defined(TARGET_MIPS) || \
+    defined(TARGET_LM32) || (defined(TARGET_SPARC) && !defined(TARGET_SPARC64))
     {
         .name       = "irq",
         .args_type  = "",
         .params     = "",
         .help       = "show the interrupts statistics (if available)",
+#ifdef TARGET_SPARC
+        .mhandler.info = sun4m_irq_info,
+#elif defined(TARGET_LM32)
+        .mhandler.info = lm32_irq_info,
+#else
         .mhandler.info = irq_info,
+#endif
     },
     {
         .name       = "pic",
         .args_type  = "",
         .params     = "",
         .help       = "show i8259 (PIC) state",
+#ifdef TARGET_SPARC
+        .mhandler.info = sun4m_pic_info,
+#elif defined(TARGET_LM32)
+        .mhandler.info = lm32_do_pic_info,
+#else
         .mhandler.info = pic_info,
+#endif
     },
+#endif
     {
         .name       = "pci",
         .args_type  = "",
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH v2 21/22] monitor: Restrict pic/irq_info to supporting targets

[Blue Swirl]

On Fri, Sep 30, 2011 at 6:50 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Acked-by: Blue Swirl <blauwirbel@gmail.com>

> ---
>
> Changes in v2:
>  - keep slavio_intctl local, introduce sun4m_pic/irq_info instead
>
>  hw/an5206.c             |   10 ----------
>  hw/arm_pic.c            |   11 -----------
>  hw/cris_pic_cpu.c       |    6 ------
>  hw/etraxfs.h            |    1 +
>  hw/lm32_pic.c           |    4 ++--
>  hw/lm32_pic.h           |    3 +++
>  hw/microblaze_pic_cpu.c |    6 ------
>  hw/s390-virtio.c        |   11 -----------
>  hw/shix.c               |   11 -----------
>  hw/sun4m.c              |    4 ++--
>  hw/sun4m.h              |    4 ++++
>  hw/sun4u.c              |    8 --------
>  hw/xtensa_pic.c         |   10 ----------
>  monitor.c               |   21 +++++++++++++++++++++
>  14 files changed, 33 insertions(+), 77 deletions(-)
>
> diff --git a/hw/an5206.c b/hw/an5206.c
> index 481ae60..3fe1f00 100644
> --- a/hw/an5206.c
> +++ b/hw/an5206.c
> @@ -7,7 +7,6 @@
>  */
>
>  #include "hw.h"
> -#include "pc.h"
>  #include "mcf.h"
>  #include "boards.h"
>  #include "loader.h"
> @@ -18,15 +17,6 @@
>  #define AN5206_MBAR_ADDR 0x10000000
>  #define AN5206_RAMBAR_ADDR 0x20000000
>
> -/* Stub functions for hardware that doesn't exist.  */
> -void pic_info(Monitor *mon)
> -{
> -}
> -
> -void irq_info(Monitor *mon)
> -{
> -}
> -
>  /* Board init.  */
>
>  static void an5206_init(ram_addr_t ram_size,
> diff --git a/hw/arm_pic.c b/hw/arm_pic.c
> index 985148a..4e63845 100644
> --- a/hw/arm_pic.c
> +++ b/hw/arm_pic.c
> @@ -8,19 +8,8 @@
>  */
>
>  #include "hw.h"
> -#include "pc.h"
>  #include "arm-misc.h"
>
> -/* Stub functions for hardware that doesn't exist.  */
> -void pic_info(Monitor *mon)
> -{
> -}
> -
> -void irq_info(Monitor *mon)
> -{
> -}
> -
> -
>  /* Input 0 is IRQ and input 1 is FIQ.  */
>  static void arm_pic_cpu_handler(void *opaque, int irq, int level)
>  {
> diff --git a/hw/cris_pic_cpu.c b/hw/cris_pic_cpu.c
> index 7f1e4ab..06ae484 100644
> --- a/hw/cris_pic_cpu.c
> +++ b/hw/cris_pic_cpu.c
> @@ -24,16 +24,10 @@
>
>  #include "sysbus.h"
>  #include "hw.h"
> -#include "pc.h"
>  #include "etraxfs.h"
>
>  #define D(x)
>
> -void pic_info(Monitor *mon)
> -{}
> -void irq_info(Monitor *mon)
> -{}
> -
>  static void cris_pic_cpu_handler(void *opaque, int irq, int level)
>  {
>     CPUState *env = (CPUState *)opaque;
> diff --git a/hw/etraxfs.h b/hw/etraxfs.h
> index 1554b0b..24e8fd8 100644
> --- a/hw/etraxfs.h
> +++ b/hw/etraxfs.h
> @@ -22,6 +22,7 @@
>  * THE SOFTWARE.
>  */
>
> +#include "net.h"
>  #include "etraxfs_dma.h"
>
>  qemu_irq *cris_pic_init_cpu(CPUState *env);
> diff --git a/hw/lm32_pic.c b/hw/lm32_pic.c
> index 02941a7..8dd0050 100644
> --- a/hw/lm32_pic.c
> +++ b/hw/lm32_pic.c
> @@ -39,7 +39,7 @@ struct LM32PicState {
>  typedef struct LM32PicState LM32PicState;
>
>  static LM32PicState *pic;
> -void pic_info(Monitor *mon)
> +void lm32_do_pic_info(Monitor *mon)
>  {
>     if (pic == NULL) {
>         return;
> @@ -49,7 +49,7 @@ void pic_info(Monitor *mon)
>             pic->im, pic->ip, pic->irq_state);
>  }
>
> -void irq_info(Monitor *mon)
> +void lm32_irq_info(Monitor *mon)
>  {
>     int i;
>     uint32_t count;
> diff --git a/hw/lm32_pic.h b/hw/lm32_pic.h
> index e6479b8..14456f3 100644
> --- a/hw/lm32_pic.h
> +++ b/hw/lm32_pic.h
> @@ -8,4 +8,7 @@ uint32_t lm32_pic_get_im(DeviceState *d);
>  void lm32_pic_set_ip(DeviceState *d, uint32_t ip);
>  void lm32_pic_set_im(DeviceState *d, uint32_t im);
>
> +void lm32_do_pic_info(Monitor *mon);
> +void lm32_irq_info(Monitor *mon);
> +
>  #endif /* QEMU_HW_LM32_PIC_H */
> diff --git a/hw/microblaze_pic_cpu.c b/hw/microblaze_pic_cpu.c
> index 9ad48b4..8b5623c 100644
> --- a/hw/microblaze_pic_cpu.c
> +++ b/hw/microblaze_pic_cpu.c
> @@ -23,16 +23,10 @@
>  */
>
>  #include "hw.h"
> -#include "pc.h"
>  #include "microblaze_pic_cpu.h"
>
>  #define D(x)
>
> -void pic_info(Monitor *mon)
> -{}
> -void irq_info(Monitor *mon)
> -{}
> -
>  static void microblaze_pic_cpu_handler(void *opaque, int irq, int level)
>  {
>     CPUState *env = (CPUState *)opaque;
> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
> index acbf026..778cffe 100644
> --- a/hw/s390-virtio.c
> +++ b/hw/s390-virtio.c
> @@ -62,17 +62,6 @@
>  static VirtIOS390Bus *s390_bus;
>  static CPUState **ipi_states;
>
> -void irq_info(Monitor *mon);
> -void pic_info(Monitor *mon);
> -
> -void irq_info(Monitor *mon)
> -{
> -}
> -
> -void pic_info(Monitor *mon)
> -{
> -}
> -
>  CPUState *s390_cpu_addr2state(uint16_t cpu_addr)
>  {
>     if (cpu_addr >= smp_cpus) {
> diff --git a/hw/shix.c b/hw/shix.c
> index 638bf16..dbf4764 100644
> --- a/hw/shix.c
> +++ b/hw/shix.c
> @@ -28,7 +28,6 @@
>    More information in target-sh4/README.sh4
>  */
>  #include "hw.h"
> -#include "pc.h"
>  #include "sh.h"
>  #include "sysemu.h"
>  #include "boards.h"
> @@ -37,16 +36,6 @@
>  #define BIOS_FILENAME "shix_bios.bin"
>  #define BIOS_ADDRESS 0xA0000000
>
> -void irq_info(Monitor *mon)
> -{
> -    /* XXXXX */
> -}
> -
> -void pic_info(Monitor *mon)
> -{
> -    /* XXXXX */
> -}
> -
>  static void shix_init(ram_addr_t ram_size,
>                const char *boot_device,
>               const char *kernel_filename, const char *kernel_cmdline,
> diff --git a/hw/sun4m.c b/hw/sun4m.c
> index dcaed38..71bf648 100644
> --- a/hw/sun4m.c
> +++ b/hw/sun4m.c
> @@ -216,13 +216,13 @@ static void nvram_init(M48t59State *nvram, uint8_t *macaddr,
>
>  static DeviceState *slavio_intctl;
>
> -void pic_info(Monitor *mon)
> +void sun4m_pic_info(Monitor *mon)
>  {
>     if (slavio_intctl)
>         slavio_pic_info(mon, slavio_intctl);
>  }
>
> -void irq_info(Monitor *mon)
> +void sun4m_irq_info(Monitor *mon)
>  {
>     if (slavio_intctl)
>         slavio_irq_info(mon, slavio_intctl);
> diff --git a/hw/sun4m.h b/hw/sun4m.h
> index ce97ee5..504c3af 100644
> --- a/hw/sun4m.h
> +++ b/hw/sun4m.h
> @@ -30,6 +30,10 @@ void slavio_irq_info(Monitor *mon, DeviceState *dev);
>  void sun4c_pic_info(Monitor *mon, void *opaque);
>  void sun4c_irq_info(Monitor *mon, void *opaque);
>
> +/* sun4m.c */
> +void sun4m_pic_info(Monitor *mon);
> +void sun4m_irq_info(Monitor *mon);
> +
>  /* sparc32_dma.c */
>  #include "sparc32_dma.h"
>
> diff --git a/hw/sun4u.c b/hw/sun4u.c
> index fbef350..568819d 100644
> --- a/hw/sun4u.c
> +++ b/hw/sun4u.c
> @@ -242,14 +242,6 @@ static unsigned long sun4u_load_kernel(const char *kernel_filename,
>     return kernel_size;
>  }
>
> -void pic_info(Monitor *mon)
> -{
> -}
> -
> -void irq_info(Monitor *mon)
> -{
> -}
> -
>  void cpu_check_irqs(CPUState *env)
>  {
>     uint32_t pil = env->pil_in |
> diff --git a/hw/xtensa_pic.c b/hw/xtensa_pic.c
> index 3033ae2..9357684 100644
> --- a/hw/xtensa_pic.c
> +++ b/hw/xtensa_pic.c
> @@ -26,19 +26,9 @@
>  */
>
>  #include "hw.h"
> -#include "pc.h"
>  #include "qemu-log.h"
>  #include "qemu-timer.h"
>
> -/* Stub functions for hardware that doesn't exist.  */
> -void pic_info(Monitor *mon)
> -{
> -}
> -
> -void irq_info(Monitor *mon)
> -{
> -}
> -
>  void xtensa_advance_ccount(CPUState *env, uint32_t d)
>  {
>     uint32_t old_ccount = env->sregs[CCOUNT];
> diff --git a/monitor.c b/monitor.c
> index 8ec2c5e..04a3912 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -64,6 +64,12 @@
>  #include "trace/control.h"
>  #include "ui/qemu-spice.h"
>
> +/* for pic/irq_info */
> +#if defined(TARGET_SPARC)
> +#include "hw/sun4m.h"
> +#endif
> +#include "hw/lm32_pic.h"
> +
>  //#define DEBUG
>  //#define DEBUG_COMPLETION
>
> @@ -2937,20 +2943,35 @@ static const mon_cmd_t info_cmds[] = {
>         .help       = "show the command line history",
>         .mhandler.info = do_info_history,
>     },
> +#if defined(TARGET_I386) || defined(TARGET_PPC) || defined(TARGET_MIPS) || \
> +    defined(TARGET_LM32) || (defined(TARGET_SPARC) && !defined(TARGET_SPARC64))
>     {
>         .name       = "irq",
>         .args_type  = "",
>         .params     = "",
>         .help       = "show the interrupts statistics (if available)",
> +#ifdef TARGET_SPARC
> +        .mhandler.info = sun4m_irq_info,
> +#elif defined(TARGET_LM32)
> +        .mhandler.info = lm32_irq_info,
> +#else
>         .mhandler.info = irq_info,
> +#endif
>     },
>     {
>         .name       = "pic",
>         .args_type  = "",
>         .params     = "",
>         .help       = "show i8259 (PIC) state",
> +#ifdef TARGET_SPARC
> +        .mhandler.info = sun4m_pic_info,
> +#elif defined(TARGET_LM32)
> +        .mhandler.info = lm32_do_pic_info,
> +#else
>         .mhandler.info = pic_info,
> +#endif
>     },
> +#endif
>     {
>         .name       = "pci",
>         .args_type  = "",
> --
> 1.7.3.4
>

[Qemu-devel] [PATCH 22/22] i8259: Move to hw library

[Jan Kiszka]

No target-specific bits remaining, let's move it over.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 Makefile.objs   |    2 +-
 Makefile.target |    8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/Makefile.objs b/Makefile.objs
index 1c65087..e56c18a 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -276,7 +276,7 @@ hw-obj-$(CONFIG_AHCI) += ide/ich.o
 hw-obj-$(CONFIG_LSI_SCSI_PCI) += lsi53c895a.o
 hw-obj-$(CONFIG_ESP) += esp.o
 
-hw-obj-y += dma-helpers.o sysbus.o isa-bus.o
+hw-obj-y += dma-helpers.o sysbus.o isa-bus.o i8259.o
 hw-obj-y += qdev-addr.o
 
 # VGA
diff --git a/Makefile.target b/Makefile.target
index 88d2f1f..393f58d 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -219,7 +219,7 @@ obj-$(CONFIG_IVSHMEM) += ivshmem.o
 
 # Hardware support
 obj-i386-y += vga.o
-obj-i386-y += mc146818rtc.o i8259.o pc.o
+obj-i386-y += mc146818rtc.o pc.o
 obj-i386-y += cirrus_vga.o sga.o apic.o ioapic.o piix_pci.o
 obj-i386-y += vmport.o
 obj-i386-y += device-hotplug.o pci-hotplug.o smbios.o wdt_ib700.o
@@ -232,7 +232,7 @@ obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
 obj-ppc-y = ppc.o
 obj-ppc-y += vga.o
 # PREP target
-obj-ppc-y += i8259.o mc146818rtc.o
+obj-ppc-y += mc146818rtc.o
 obj-ppc-y += ppc_prep.o
 # OldWorld PowerMac
 obj-ppc-y += ppc_oldworld.o
@@ -283,7 +283,7 @@ obj-lm32-y += framebuffer.o
 
 obj-mips-y = mips_r4k.o mips_jazz.o mips_malta.o mips_mipssim.o
 obj-mips-y += mips_addr.o mips_timer.o mips_int.o
-obj-mips-y += vga.o i8259.o
+obj-mips-y += vga.o
 obj-mips-y += jazz_led.o
 obj-mips-y += gt64xxx.o mc146818rtc.o
 obj-mips-y += cirrus_vga.o
@@ -365,7 +365,7 @@ obj-m68k-y += m68k-semi.o dummy_m68k.o
 
 obj-s390x-y = s390-virtio-bus.o s390-virtio.o
 
-obj-alpha-y = i8259.o mc146818rtc.o
+obj-alpha-y = mc146818rtc.o
 obj-alpha-y += vga.o cirrus_vga.o
 
 obj-xtensa-y += xtensa_pic.o
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH 22/22] i8259: Move to hw library

[Blue Swirl]

On Wed, Sep 28, 2011 at 11:01 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> No target-specific bits remaining, let's move it over.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  Makefile.objs   |    2 +-
>  Makefile.target |    8 ++++----
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/Makefile.objs b/Makefile.objs
> index 1c65087..e56c18a 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -276,7 +276,7 @@ hw-obj-$(CONFIG_AHCI) += ide/ich.o
>  hw-obj-$(CONFIG_LSI_SCSI_PCI) += lsi53c895a.o
>  hw-obj-$(CONFIG_ESP) += esp.o
>
> -hw-obj-y += dma-helpers.o sysbus.o isa-bus.o
> +hw-obj-y += dma-helpers.o sysbus.o isa-bus.o i8259.o

Please make this
hw-obj-$(CONFIG_I8259) += i8259.o
and adjust default-configs/*.

>  hw-obj-y += qdev-addr.o
>
>  # VGA
> diff --git a/Makefile.target b/Makefile.target
> index 88d2f1f..393f58d 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -219,7 +219,7 @@ obj-$(CONFIG_IVSHMEM) += ivshmem.o
>
>  # Hardware support
>  obj-i386-y += vga.o
> -obj-i386-y += mc146818rtc.o i8259.o pc.o
> +obj-i386-y += mc146818rtc.o pc.o
>  obj-i386-y += cirrus_vga.o sga.o apic.o ioapic.o piix_pci.o
>  obj-i386-y += vmport.o
>  obj-i386-y += device-hotplug.o pci-hotplug.o smbios.o wdt_ib700.o
> @@ -232,7 +232,7 @@ obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
>  obj-ppc-y = ppc.o
>  obj-ppc-y += vga.o
>  # PREP target
> -obj-ppc-y += i8259.o mc146818rtc.o
> +obj-ppc-y += mc146818rtc.o
>  obj-ppc-y += ppc_prep.o
>  # OldWorld PowerMac
>  obj-ppc-y += ppc_oldworld.o
> @@ -283,7 +283,7 @@ obj-lm32-y += framebuffer.o
>
>  obj-mips-y = mips_r4k.o mips_jazz.o mips_malta.o mips_mipssim.o
>  obj-mips-y += mips_addr.o mips_timer.o mips_int.o
> -obj-mips-y += vga.o i8259.o
> +obj-mips-y += vga.o
>  obj-mips-y += jazz_led.o
>  obj-mips-y += gt64xxx.o mc146818rtc.o
>  obj-mips-y += cirrus_vga.o
> @@ -365,7 +365,7 @@ obj-m68k-y += m68k-semi.o dummy_m68k.o
>
>  obj-s390x-y = s390-virtio-bus.o s390-virtio.o
>
> -obj-alpha-y = i8259.o mc146818rtc.o
> +obj-alpha-y = mc146818rtc.o
>  obj-alpha-y += vga.o cirrus_vga.o
>
>  obj-xtensa-y += xtensa_pic.o
> --
> 1.7.3.4
>
>
>

[Qemu-devel] [PATCH v2 22/22] i8259: Move to hw library

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

No target-specific bits remaining, let's move it over.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Changes in v2:
 - Use defconfigs to control the existence in hwlib

 Makefile.objs                        |    1 +
 Makefile.target                      |    8 ++++----
 default-configs/alpha-softmmu.mak    |    1 +
 default-configs/i386-softmmu.mak     |    1 +
 default-configs/mips-softmmu.mak     |    1 +
 default-configs/mips64-softmmu.mak   |    1 +
 default-configs/mips64el-softmmu.mak |    1 +
 default-configs/mipsel-softmmu.mak   |    1 +
 default-configs/ppc-softmmu.mak      |    1 +
 default-configs/ppc64-softmmu.mak    |    1 +
 default-configs/ppcemb-softmmu.mak   |    1 +
 default-configs/x86_64-softmmu.mak   |    1 +
 12 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/Makefile.objs b/Makefile.objs
index 1c65087..d457997 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -220,6 +220,7 @@ hw-obj-$(CONFIG_APPLESMC) += applesmc.o
 hw-obj-$(CONFIG_SMARTCARD) += usb-ccid.o ccid-card-passthru.o
 hw-obj-$(CONFIG_SMARTCARD_NSS) += ccid-card-emulated.o
 hw-obj-$(CONFIG_USB_REDIR) += usb-redir.o
+hw-obj-$(CONFIG_I8259) += i8259.o
 
 # PPC devices
 hw-obj-$(CONFIG_OPENPIC) += openpic.o
diff --git a/Makefile.target b/Makefile.target
index 88d2f1f..393f58d 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -219,7 +219,7 @@ obj-$(CONFIG_IVSHMEM) += ivshmem.o
 
 # Hardware support
 obj-i386-y += vga.o
-obj-i386-y += mc146818rtc.o i8259.o pc.o
+obj-i386-y += mc146818rtc.o pc.o
 obj-i386-y += cirrus_vga.o sga.o apic.o ioapic.o piix_pci.o
 obj-i386-y += vmport.o
 obj-i386-y += device-hotplug.o pci-hotplug.o smbios.o wdt_ib700.o
@@ -232,7 +232,7 @@ obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
 obj-ppc-y = ppc.o
 obj-ppc-y += vga.o
 # PREP target
-obj-ppc-y += i8259.o mc146818rtc.o
+obj-ppc-y += mc146818rtc.o
 obj-ppc-y += ppc_prep.o
 # OldWorld PowerMac
 obj-ppc-y += ppc_oldworld.o
@@ -283,7 +283,7 @@ obj-lm32-y += framebuffer.o
 
 obj-mips-y = mips_r4k.o mips_jazz.o mips_malta.o mips_mipssim.o
 obj-mips-y += mips_addr.o mips_timer.o mips_int.o
-obj-mips-y += vga.o i8259.o
+obj-mips-y += vga.o
 obj-mips-y += jazz_led.o
 obj-mips-y += gt64xxx.o mc146818rtc.o
 obj-mips-y += cirrus_vga.o
@@ -365,7 +365,7 @@ obj-m68k-y += m68k-semi.o dummy_m68k.o
 
 obj-s390x-y = s390-virtio-bus.o s390-virtio.o
 
-obj-alpha-y = i8259.o mc146818rtc.o
+obj-alpha-y = mc146818rtc.o
 obj-alpha-y += vga.o cirrus_vga.o
 
 obj-xtensa-y += xtensa_pic.o
diff --git a/default-configs/alpha-softmmu.mak b/default-configs/alpha-softmmu.mak
index abadcff..3889213 100644
--- a/default-configs/alpha-softmmu.mak
+++ b/default-configs/alpha-softmmu.mak
@@ -7,3 +7,4 @@ CONFIG_VGA_PCI=y
 CONFIG_IDE_CORE=y
 CONFIG_IDE_QDEV=y
 CONFIG_VMWARE_VGA=y
+CONFIG_I8259=y
diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
index 55589fa..e67ebb3 100644
--- a/default-configs/i386-softmmu.mak
+++ b/default-configs/i386-softmmu.mak
@@ -21,3 +21,4 @@ CONFIG_PIIX_PCI=y
 CONFIG_SOUND=y
 CONFIG_HPET=y
 CONFIG_APPLESMC=y
+CONFIG_I8259=y
diff --git a/default-configs/mips-softmmu.mak b/default-configs/mips-softmmu.mak
index 45bdefb..94a3486 100644
--- a/default-configs/mips-softmmu.mak
+++ b/default-configs/mips-softmmu.mak
@@ -27,3 +27,4 @@ CONFIG_DS1225Y=y
 CONFIG_MIPSNET=y
 CONFIG_PFLASH_CFI01=y
 CONFIG_G364FB=y
+CONFIG_I8259=y
diff --git a/default-configs/mips64-softmmu.mak b/default-configs/mips64-softmmu.mak
index d43e33c..b5d3108 100644
--- a/default-configs/mips64-softmmu.mak
+++ b/default-configs/mips64-softmmu.mak
@@ -27,3 +27,4 @@ CONFIG_DS1225Y=y
 CONFIG_MIPSNET=y
 CONFIG_PFLASH_CFI01=y
 CONFIG_G364FB=y
+CONFIG_I8259=y
diff --git a/default-configs/mips64el-softmmu.mak b/default-configs/mips64el-softmmu.mak
index f307e8d..2831f44 100644
--- a/default-configs/mips64el-softmmu.mak
+++ b/default-configs/mips64el-softmmu.mak
@@ -29,3 +29,4 @@ CONFIG_MIPSNET=y
 CONFIG_PFLASH_CFI01=y
 CONFIG_FULONG=y
 CONFIG_G364FB=y
+CONFIG_I8259=y
diff --git a/default-configs/mipsel-softmmu.mak b/default-configs/mipsel-softmmu.mak
index 1a66bc3..14c949d 100644
--- a/default-configs/mipsel-softmmu.mak
+++ b/default-configs/mipsel-softmmu.mak
@@ -27,3 +27,4 @@ CONFIG_DS1225Y=y
 CONFIG_MIPSNET=y
 CONFIG_PFLASH_CFI01=y
 CONFIG_G364FB=y
+CONFIG_I8259=y
diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
index 4563742..c85cdce 100644
--- a/default-configs/ppc-softmmu.mak
+++ b/default-configs/ppc-softmmu.mak
@@ -31,3 +31,4 @@ CONFIG_SOUND=y
 CONFIG_PFLASH_CFI01=y
 CONFIG_PFLASH_CFI02=y
 CONFIG_PTIMER=y
+CONFIG_I8259=y
diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
index d5073b3..8874115 100644
--- a/default-configs/ppc64-softmmu.mak
+++ b/default-configs/ppc64-softmmu.mak
@@ -31,3 +31,4 @@ CONFIG_SOUND=y
 CONFIG_PFLASH_CFI01=y
 CONFIG_PFLASH_CFI02=y
 CONFIG_PTIMER=y
+CONFIG_I8259=y
diff --git a/default-configs/ppcemb-softmmu.mak b/default-configs/ppcemb-softmmu.mak
index 9f0730c..5db7205 100644
--- a/default-configs/ppcemb-softmmu.mak
+++ b/default-configs/ppcemb-softmmu.mak
@@ -31,3 +31,4 @@ CONFIG_SOUND=y
 CONFIG_PFLASH_CFI01=y
 CONFIG_PFLASH_CFI02=y
 CONFIG_PTIMER=y
+CONFIG_I8259=y
diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
index 8895028..b75757e 100644
--- a/default-configs/x86_64-softmmu.mak
+++ b/default-configs/x86_64-softmmu.mak
@@ -21,3 +21,4 @@ CONFIG_PIIX_PCI=y
 CONFIG_SOUND=y
 CONFIG_HPET=y
 CONFIG_APPLESMC=y
+CONFIG_I8259=y
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH 00/22] Rework i8259 and PC interrupt models

[Richard Henderson]

On 09/28/2011 04:00 AM, Jan Kiszka wrote:
> Highlights of this series:
>  - generic i8259, now part of hwlib
>  - qdev conversion of i8259
>  - fix for i8259 poll mode (and removal of PREP hack)
> 
> The refactoring will also be important to instantiate i8259-kvm devices
> for in-kernel irqchip acceleration one day.
> 
> Note: depends on "mips_fulong2e: Reorder ISA bus and i8259 creation"
> 
> CC: Alexander Graf <agraf@suse.de>

All of the patches look reasonable.  I'll try and give it a try with
my alpha port sometime soon.

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

Re: [Qemu-devel] [PATCH 00/22] Rework i8259 and PC interrupt models

[Jan Kiszka]

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

On 2011-09-28 18:39, Richard Henderson wrote:
> On 09/28/2011 04:00 AM, Jan Kiszka wrote:
>> Highlights of this series:
>>  - generic i8259, now part of hwlib
>>  - qdev conversion of i8259
>>  - fix for i8259 poll mode (and removal of PREP hack)
>>
>> The refactoring will also be important to instantiate i8259-kvm devices
>> for in-kernel irqchip acceleration one day.
>>
>> Note: depends on "mips_fulong2e: Reorder ISA bus and i8259 creation"
>>
>> CC: Alexander Graf <agraf@suse.de>
> 
> All of the patches look reasonable.  I'll try and give it a try with
> my alpha port sometime soon.
> 
> Reviewed-by: Richard Henderson <rth@twiddle.net>

Thanks! Having another non-x86 check would be welcome of course.

Jan



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