[Qemu-devel] [PATCH 0/3] ich9: clean up handling of output interrupt pins

[Qemu-devel] [PATCH 0/3] ich9: clean up handling of output interrupt pins

[Paolo Bonzini]

Currently the southbridge in the q35 machine types gets 40 different
qemu_irqs.  The 16 lower GSIs (connected to both 8259 and IOAPIC) and
the 24 input pins of the IOAPIC (the higher 8 of which correspond to the
8 higher GSIs).  However, there's no reason to use the 16 first input
pins of the IOAPIC except through the 16 lower GSIs.

This series cleans this up by passing just the 24 GSIs to lpc_ich9.c.
The first patch is a bug fix.

Paolo

Paolo Bonzini (3):
  ich9: call ich9_lpc_update_pic for disabled pirqs
  ich9: clean up ich9_lpc_update_pic/ich9_lpc_update_apic and callers
  ich9: unify pic and ioapic IRQ vectors

 hw/i386/pc_q35.c       |  3 +--
 hw/isa/lpc_ich9.c      | 44 +++++++++++++++++---------------------------
 include/hw/i386/ich9.h |  3 +--
 3 files changed, 19 insertions(+), 31 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH 1/3] ich9: call ich9_lpc_update_pic for disabled pirqs

[Paolo Bonzini]

An asserted pirq can be disabled and the corresponding GSIs
should then go down to 0.  However, because of the conditional in
ich9_lpc_update_by_pirq, the legacy 8259 pin could remain stuck to 1.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/isa/lpc_ich9.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 213741b..b116e1b 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -234,9 +234,6 @@ static void ich9_lpc_update_by_pirq(ICH9LPCState *lpc, int pirq)
 
     ich9_lpc_pic_irq(lpc, pirq, &pic_irq, &pic_dis);
     assert(pic_irq < ICH9_LPC_PIC_NUM_PINS);
-    if (pic_dis) {
-        return;
-    }
 
     ich9_lpc_update_pic(lpc, pic_irq);
 }
-- 
2.5.5

Re: [Qemu-devel] [PATCH 1/3] ich9: call ich9_lpc_update_pic for disabled pirqs

[Ефимов Василий]

20.06.2016 17:39, Paolo Bonzini wrote:
> An asserted pirq can be disabled and the corresponding GSIs
> should then go down to 0.  However, because of the conditional in
> ich9_lpc_update_by_pirq, the legacy 8259 pin could remain stuck to 1.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/isa/lpc_ich9.c | 3 ---
>   1 file changed, 3 deletions(-)
>
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 213741b..b116e1b 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -234,9 +234,6 @@ static void ich9_lpc_update_by_pirq(ICH9LPCState *lpc, int pirq)
>
>       ich9_lpc_pic_irq(lpc, pirq, &pic_irq, &pic_dis);
>       assert(pic_irq < ICH9_LPC_PIC_NUM_PINS);
> -    if (pic_dis) {
> -        return;
> -    }
>
>       ich9_lpc_update_pic(lpc, pic_irq);
>   }
>
Reviewed-by: Efimov Vasily <real@ispras.ru>

[Qemu-devel] [PATCH 2/3] ich9: clean up ich9_lpc_update_pic/ich9_lpc_update_apic and callers

[Paolo Bonzini]

Make ich9_lpc_update_pic take care only of GSIs 0-15, and
ich9_lpc_update_apic take care only of GSIs 16-23.  Assert
that they are called with the correct GSI indices.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/isa/lpc_ich9.c | 39 ++++++++++++++++-----------------------
 1 file changed, 16 insertions(+), 23 deletions(-)

diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index b116e1b..446797b 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -204,38 +204,28 @@ static void ich9_lpc_pic_irq(ICH9LPCState *lpc, int pirq_num,
     abort();
 }
 
-/* pic_irq: i8254 irq 0-15 */
-static void ich9_lpc_update_pic(ICH9LPCState *lpc, int pic_irq)
+/* gsi: i8259 irq 0-15, otherwise ignored */
+static void ich9_lpc_update_pic(ICH9LPCState *lpc, int gsi)
 {
     int i, pic_level;
 
+    assert(gsi < ICH9_LPC_PIC_NUM_PINS);
+
     /* The pic level is the logical OR of all the PCI irqs mapped to it */
     pic_level = 0;
     for (i = 0; i < ICH9_LPC_NB_PIRQS; i++) {
         int tmp_irq;
         int tmp_dis;
         ich9_lpc_pic_irq(lpc, i, &tmp_irq, &tmp_dis);
-        if (!tmp_dis && pic_irq == tmp_irq) {
+        if (!tmp_dis && tmp_irq == gsi) {
             pic_level |= pci_bus_get_irq_level(lpc->d.bus, i);
         }
     }
-    if (pic_irq == ich9_lpc_sci_irq(lpc)) {
+    if (gsi == ich9_lpc_sci_irq(lpc)) {
         pic_level |= lpc->sci_level;
     }
 
-    qemu_set_irq(lpc->pic[pic_irq], pic_level);
-}
-
-/* pirq: pirq[A-H] 0-7*/
-static void ich9_lpc_update_by_pirq(ICH9LPCState *lpc, int pirq)
-{
-    int pic_irq;
-    int pic_dis;
-
-    ich9_lpc_pic_irq(lpc, pirq, &pic_irq, &pic_dis);
-    assert(pic_irq < ICH9_LPC_PIC_NUM_PINS);
-
-    ich9_lpc_update_pic(lpc, pic_irq);
+    qemu_set_irq(lpc->pic[gsi], pic_level);
 }
 
 /* APIC mode: GSIx: PIRQ[A-H] -> GSI 16, ... no pirq shares same APIC pins. */
@@ -253,9 +243,9 @@ static void ich9_lpc_update_apic(ICH9LPCState *lpc, int gsi)
 {
     int level = 0;
 
-    if (gsi >= ICH9_LPC_PIC_NUM_PINS) {
-        level |= pci_bus_get_irq_level(lpc->d.bus, ich9_gsi_to_pirq(gsi));
-    }
+    assert(gsi >= ICH9_LPC_PIC_NUM_PINS);
+
+    level |= pci_bus_get_irq_level(lpc->d.bus, ich9_gsi_to_pirq(gsi));
     if (gsi == ich9_lpc_sci_irq(lpc)) {
         level |= lpc->sci_level;
     }
@@ -266,12 +256,14 @@ static void ich9_lpc_update_apic(ICH9LPCState *lpc, int gsi)
 void ich9_lpc_set_irq(void *opaque, int pirq, int level)
 {
     ICH9LPCState *lpc = opaque;
+    int pic_irq, pic_dis;
 
     assert(0 <= pirq);
     assert(pirq < ICH9_LPC_NB_PIRQS);
 
     ich9_lpc_update_apic(lpc, ich9_pirq_to_gsi(pirq));
-    ich9_lpc_update_by_pirq(lpc, pirq);
+    ich9_lpc_pic_irq(lpc, pirq, &pic_irq, &pic_dis);
+    ich9_lpc_update_pic(lpc, pic_irq);
 }
 
 /* return the pirq number (PIRQ[A-H]:0-7) corresponding to
@@ -362,8 +354,9 @@ static void ich9_set_sci(void *opaque, int irq_num, int level)
         return;
     }
 
-    ich9_lpc_update_apic(lpc, irq);
-    if (irq < ICH9_LPC_PIC_NUM_PINS) {
+    if (irq >= ICH9_LPC_PIC_NUM_PINS) {
+        ich9_lpc_update_apic(lpc, irq);
+    } else {
         ich9_lpc_update_pic(lpc, irq);
     }
 }
-- 
2.5.5

Re: [Qemu-devel] [PATCH 2/3] ich9: clean up ich9_lpc_update_pic/ich9_lpc_update_apic and callers

[Ефимов Василий]

20.06.2016 17:39, Paolo Bonzini wrote:
> Make ich9_lpc_update_pic take care only of GSIs 0-15, and
> ich9_lpc_update_apic take care only of GSIs 16-23.  Assert
> that they are called with the correct GSI indices.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/isa/lpc_ich9.c | 39 ++++++++++++++++-----------------------
>   1 file changed, 16 insertions(+), 23 deletions(-)
>
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index b116e1b..446797b 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -204,38 +204,28 @@ static void ich9_lpc_pic_irq(ICH9LPCState *lpc, int pirq_num,
>       abort();
>   }
>
> -/* pic_irq: i8254 irq 0-15 */
> -static void ich9_lpc_update_pic(ICH9LPCState *lpc, int pic_irq)
> +/* gsi: i8259 irq 0-15, otherwise ignored */
> +static void ich9_lpc_update_pic(ICH9LPCState *lpc, int gsi)
>   {
>       int i, pic_level;
>
> +    assert(gsi < ICH9_LPC_PIC_NUM_PINS);
> +
>       /* The pic level is the logical OR of all the PCI irqs mapped to it */
>       pic_level = 0;
>       for (i = 0; i < ICH9_LPC_NB_PIRQS; i++) {
>           int tmp_irq;
>           int tmp_dis;
>           ich9_lpc_pic_irq(lpc, i, &tmp_irq, &tmp_dis);
> -        if (!tmp_dis && pic_irq == tmp_irq) {
> +        if (!tmp_dis && tmp_irq == gsi) {
>               pic_level |= pci_bus_get_irq_level(lpc->d.bus, i);
>           }
>       }
> -    if (pic_irq == ich9_lpc_sci_irq(lpc)) {
> +    if (gsi == ich9_lpc_sci_irq(lpc)) {
>           pic_level |= lpc->sci_level;
>       }
>
> -    qemu_set_irq(lpc->pic[pic_irq], pic_level);
> -}
> -
> -/* pirq: pirq[A-H] 0-7*/
> -static void ich9_lpc_update_by_pirq(ICH9LPCState *lpc, int pirq)
> -{
> -    int pic_irq;
> -    int pic_dis;
> -
> -    ich9_lpc_pic_irq(lpc, pirq, &pic_irq, &pic_dis);
> -    assert(pic_irq < ICH9_LPC_PIC_NUM_PINS);
> -
> -    ich9_lpc_update_pic(lpc, pic_irq);
> +    qemu_set_irq(lpc->pic[gsi], pic_level);
>   }
>
>   /* APIC mode: GSIx: PIRQ[A-H] -> GSI 16, ... no pirq shares same APIC pins. */
> @@ -253,9 +243,9 @@ static void ich9_lpc_update_apic(ICH9LPCState *lpc, int gsi)
>   {
>       int level = 0;
>
> -    if (gsi >= ICH9_LPC_PIC_NUM_PINS) {
> -        level |= pci_bus_get_irq_level(lpc->d.bus, ich9_gsi_to_pirq(gsi));
> -    }
> +    assert(gsi >= ICH9_LPC_PIC_NUM_PINS);
> +
> +    level |= pci_bus_get_irq_level(lpc->d.bus, ich9_gsi_to_pirq(gsi));
>       if (gsi == ich9_lpc_sci_irq(lpc)) {
>           level |= lpc->sci_level;
>       }
> @@ -266,12 +256,14 @@ static void ich9_lpc_update_apic(ICH9LPCState *lpc, int gsi)
>   void ich9_lpc_set_irq(void *opaque, int pirq, int level)
>   {
>       ICH9LPCState *lpc = opaque;
> +    int pic_irq, pic_dis;
>
>       assert(0 <= pirq);
>       assert(pirq < ICH9_LPC_NB_PIRQS);
>
>       ich9_lpc_update_apic(lpc, ich9_pirq_to_gsi(pirq));
> -    ich9_lpc_update_by_pirq(lpc, pirq);
> +    ich9_lpc_pic_irq(lpc, pirq, &pic_irq, &pic_dis);
> +    ich9_lpc_update_pic(lpc, pic_irq);
>   }
>
>   /* return the pirq number (PIRQ[A-H]:0-7) corresponding to
> @@ -362,8 +354,9 @@ static void ich9_set_sci(void *opaque, int irq_num, int level)
>           return;
>       }
>
> -    ich9_lpc_update_apic(lpc, irq);
> -    if (irq < ICH9_LPC_PIC_NUM_PINS) {
> +    if (irq >= ICH9_LPC_PIC_NUM_PINS) {
> +        ich9_lpc_update_apic(lpc, irq);
> +    } else {
>           ich9_lpc_update_pic(lpc, irq);
>       }
>   }
>
Reviewed-by: Efimov Vasily <real@ispras.ru>

[Qemu-devel] [PATCH 3/3] ich9: unify pic and ioapic IRQ vectors

[Paolo Bonzini]

ich9->pic and ich9->ioapic differ for the first 16 GSIs (because
ich9->pic is wired to 8259+IOAPIC but ich9->ioapic is wired to
IOAPIC only).  However, ich9->ioapic is never used for the first
16 GSIs, so the two vectors can be merged.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/i386/pc_q35.c       | 3 +--
 hw/isa/lpc_ich9.c      | 4 ++--
 include/hw/i386/ich9.h | 3 +--
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index e4b541f..78afd4f 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -184,8 +184,7 @@ static void pc_q35_init(MachineState *machine)
                              PC_MACHINE_ACPI_DEVICE_PROP, &error_abort);
 
     ich9_lpc = ICH9_LPC_DEVICE(lpc);
-    ich9_lpc->pic = gsi;
-    ich9_lpc->ioapic = gsi_state->ioapic_irq;
+    ich9_lpc->gsi = gsi;
     pci_bus_irqs(host_bus, ich9_lpc_set_irq, ich9_lpc_map_irq, ich9_lpc,
                  ICH9_LPC_NB_PIRQS);
     pci_bus_set_route_irq_fn(host_bus, ich9_route_intx_pin_to_irq);
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 446797b..7703357 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -225,7 +225,7 @@ static void ich9_lpc_update_pic(ICH9LPCState *lpc, int gsi)
         pic_level |= lpc->sci_level;
     }
 
-    qemu_set_irq(lpc->pic[gsi], pic_level);
+    qemu_set_irq(lpc->gsi[gsi], pic_level);
 }
 
 /* APIC mode: GSIx: PIRQ[A-H] -> GSI 16, ... no pirq shares same APIC pins. */
@@ -250,7 +250,7 @@ static void ich9_lpc_update_apic(ICH9LPCState *lpc, int gsi)
         level |= lpc->sci_level;
     }
 
-    qemu_set_irq(lpc->ioapic[gsi], level);
+    qemu_set_irq(lpc->gsi[gsi], level);
 }
 
 void ich9_lpc_set_irq(void *opaque, int pirq, int level)
diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
index 88233c3..11243e5 100644
--- a/include/hw/i386/ich9.h
+++ b/include/hw/i386/ich9.h
@@ -68,8 +68,7 @@ typedef struct ICH9LPCState {
     MemoryRegion rcrb_mem; /* root complex register block */
     Notifier machine_ready;
 
-    qemu_irq *pic;
-    qemu_irq *ioapic;
+    qemu_irq *gsi;
 } ICH9LPCState;
 
 Object *ich9_lpc_find(void);
-- 
2.5.5

Re: [Qemu-devel] [PATCH 3/3] ich9: unify pic and ioapic IRQ vectors

[Ефимов Василий]

20.06.2016 17:39, Paolo Bonzini wrote:
> ich9->pic and ich9->ioapic differ for the first 16 GSIs (because
> ich9->pic is wired to 8259+IOAPIC but ich9->ioapic is wired to
> IOAPIC only).  However, ich9->ioapic is never used for the first
> 16 GSIs, so the two vectors can be merged.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/i386/pc_q35.c       | 3 +--
>   hw/isa/lpc_ich9.c      | 4 ++--
>   include/hw/i386/ich9.h | 3 +--
>   3 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index e4b541f..78afd4f 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -184,8 +184,7 @@ static void pc_q35_init(MachineState *machine)
>                                PC_MACHINE_ACPI_DEVICE_PROP, &error_abort);
>
>       ich9_lpc = ICH9_LPC_DEVICE(lpc);
> -    ich9_lpc->pic = gsi;
> -    ich9_lpc->ioapic = gsi_state->ioapic_irq;
> +    ich9_lpc->gsi = gsi;
>       pci_bus_irqs(host_bus, ich9_lpc_set_irq, ich9_lpc_map_irq, ich9_lpc,
>                    ICH9_LPC_NB_PIRQS);
>       pci_bus_set_route_irq_fn(host_bus, ich9_route_intx_pin_to_irq);
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 446797b..7703357 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -225,7 +225,7 @@ static void ich9_lpc_update_pic(ICH9LPCState *lpc, int gsi)
>           pic_level |= lpc->sci_level;
>       }
>
> -    qemu_set_irq(lpc->pic[gsi], pic_level);
> +    qemu_set_irq(lpc->gsi[gsi], pic_level);
>   }
>
>   /* APIC mode: GSIx: PIRQ[A-H] -> GSI 16, ... no pirq shares same APIC pins. */
> @@ -250,7 +250,7 @@ static void ich9_lpc_update_apic(ICH9LPCState *lpc, int gsi)
>           level |= lpc->sci_level;
>       }
>
> -    qemu_set_irq(lpc->ioapic[gsi], level);
> +    qemu_set_irq(lpc->gsi[gsi], level);
>   }
>
>   void ich9_lpc_set_irq(void *opaque, int pirq, int level)
> diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
> index 88233c3..11243e5 100644
> --- a/include/hw/i386/ich9.h
> +++ b/include/hw/i386/ich9.h
> @@ -68,8 +68,7 @@ typedef struct ICH9LPCState {
>       MemoryRegion rcrb_mem; /* root complex register block */
>       Notifier machine_ready;
>
> -    qemu_irq *pic;
> -    qemu_irq *ioapic;
> +    qemu_irq *gsi;
>   } ICH9LPCState;
>
>   Object *ich9_lpc_find(void);
>
Reviewed-by: Efimov Vasily <real@ispras.ru>