[PATCH 0/5] hw/i386/pc: Extract pc_gsi_create() and pc_i8259_create()

[PATCH 0/5] hw/i386/pc: Extract pc_gsi_create() and pc_i8259_create()

[Philippe Mathieu-Daudé]

These are few patches extracted from the previous too big series:
hw/i386/pc: Split PIIX3 southbridge from i440FX northbridge
https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg03685.html

Dropped "Move kvm_i8259_init() declaration to sysemu/kvm.h" (thuth),
no logical changes:

$ git backport-diff -u pc_split_i440fx_piix-v1
Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/5:[----] [-C] 'hw/i386/pc: Extract pc_gsi_create()'
002/5:[----] [--] 'hw/i386/pc: Reduce gsi_handler scope'
003/5:[----] [--] 'hw/i386/pc: Move gsi_state creation code'
004/5:[----] [--] 'hw/i386/pc: Extract pc_i8259_create()'
005/5:[----] [--] 'hw/i386/pc: Remove kvm_i386.h include'

Based-on: <20191018134754.16362-1-philmd@redhat.com>
hw/i386/pc: Split PIIX3 southbridge from i440FX northbridge [v2]
https://mid.mail-archive.com/20191018134754.16362-1-philmd@redhat.com

Philippe Mathieu-Daudé (5):
  hw/i386/pc: Extract pc_gsi_create()
  hw/i386/pc: Reduce gsi_handler scope
  hw/i386/pc: Move gsi_state creation code
  hw/i386/pc: Extract pc_i8259_create()
  hw/i386/pc: Remove kvm_i386.h include

 hw/i386/pc.c         | 36 +++++++++++++++++++++++++++++++++++-
 hw/i386/pc_piix.c    | 23 ++---------------------
 hw/i386/pc_q35.c     | 28 ++++------------------------
 include/hw/i386/pc.h |  3 ++-
 4 files changed, 43 insertions(+), 47 deletions(-)

-- 
2.21.0

[PATCH 1/5] hw/i386/pc: Extract pc_gsi_create()

[Philippe Mathieu-Daudé]

The GSI creation code is common to all PC machines, extract the
common code.

Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/pc.c         | 15 +++++++++++++++
 hw/i386/pc_piix.c    |  9 +--------
 hw/i386/pc_q35.c     |  9 +--------
 include/hw/i386/pc.h |  2 ++
 4 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 4b1904237e..e3e191a811 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -357,6 +357,21 @@ void gsi_handler(void *opaque, int n, int level)
     qemu_set_irq(s->ioapic_irq[n], level);
 }
 
+GSIState *pc_gsi_create(qemu_irq **irqs, bool pci_enabled)
+{
+    GSIState *s;
+
+    s = g_new0(GSIState, 1);
+    if (kvm_ioapic_in_kernel()) {
+        kvm_pc_setup_irq_routing(pci_enabled);
+        *irqs = qemu_allocate_irqs(kvm_pc_gsi_handler, s, GSI_NUM_PINS);
+    } else {
+        *irqs = qemu_allocate_irqs(gsi_handler, s, GSI_NUM_PINS);
+    }
+
+    return s;
+}
+
 static void ioport80_write(void *opaque, hwaddr addr, uint64_t data,
                            unsigned size)
 {
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index f6e7196a82..47bdc64f64 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -189,14 +189,7 @@ static void pc_init1(MachineState *machine,
         xen_load_linux(pcms);
     }
 
-    gsi_state = g_malloc0(sizeof(*gsi_state));
-    if (kvm_ioapic_in_kernel()) {
-        kvm_pc_setup_irq_routing(pcmc->pci_enabled);
-        pcms->gsi = qemu_allocate_irqs(kvm_pc_gsi_handler, gsi_state,
-                                       GSI_NUM_PINS);
-    } else {
-        pcms->gsi = qemu_allocate_irqs(gsi_handler, gsi_state, GSI_NUM_PINS);
-    }
+    gsi_state = pc_gsi_create(&pcms->gsi, pcmc->pci_enabled);
 
     if (pcmc->pci_enabled) {
         PIIX3State *piix3;
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 8fad20f314..52261962b8 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -210,14 +210,7 @@ static void pc_q35_init(MachineState *machine)
     }
 
     /* irq lines */
-    gsi_state = g_malloc0(sizeof(*gsi_state));
-    if (kvm_ioapic_in_kernel()) {
-        kvm_pc_setup_irq_routing(pcmc->pci_enabled);
-        pcms->gsi = qemu_allocate_irqs(kvm_pc_gsi_handler, gsi_state,
-                                       GSI_NUM_PINS);
-    } else {
-        pcms->gsi = qemu_allocate_irqs(gsi_handler, gsi_state, GSI_NUM_PINS);
-    }
+    gsi_state = pc_gsi_create(&pcms->gsi, pcmc->pci_enabled);
 
     /* create pci host bus */
     q35_host = Q35_HOST_DEVICE(qdev_create(NULL, TYPE_Q35_HOST_DEVICE));
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 833bc6737f..53b2243788 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -178,6 +178,8 @@ typedef struct GSIState {
 
 void gsi_handler(void *opaque, int n, int level);
 
+GSIState *pc_gsi_create(qemu_irq **irqs, bool pci_enabled);
+
 /* vmport.c */
 #define TYPE_VMPORT "vmport"
 typedef uint32_t (VMPortReadFunc)(void *opaque, uint32_t address);
-- 
2.21.0

[PATCH 2/5] hw/i386/pc: Reduce gsi_handler scope

[Philippe Mathieu-Daudé]

pc_gsi_create() is the single function that uses gsi_handler.
Make it a static variable.

Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/pc.c         | 2 +-
 include/hw/i386/pc.h | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index e3e191a811..21efde33a5 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -346,7 +346,7 @@ GlobalProperty pc_compat_1_4[] = {
 };
 const size_t pc_compat_1_4_len = G_N_ELEMENTS(pc_compat_1_4);
 
-void gsi_handler(void *opaque, int n, int level)
+static void gsi_handler(void *opaque, int n, int level)
 {
     GSIState *s = opaque;
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 53b2243788..9ad417cef0 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -176,8 +176,6 @@ typedef struct GSIState {
     qemu_irq ioapic_irq[IOAPIC_NUM_PINS];
 } GSIState;
 
-void gsi_handler(void *opaque, int n, int level);
-
 GSIState *pc_gsi_create(qemu_irq **irqs, bool pci_enabled);
 
 /* vmport.c */
-- 
2.21.0

[PATCH 3/5] hw/i386/pc: Move gsi_state creation code

[Philippe Mathieu-Daudé]

The code block related to IRQ starts few lines later. Move
the comment and the pc_gsi_create() invocation where we start
to use the IRQs.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v2: reword comment (Aleksandar)
---
 hw/i386/pc_q35.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 52261962b8..6d096eff28 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -209,9 +209,6 @@ static void pc_q35_init(MachineState *machine)
                        rom_memory, &ram_memory);
     }
 
-    /* irq lines */
-    gsi_state = pc_gsi_create(&pcms->gsi, pcmc->pci_enabled);
-
     /* create pci host bus */
     q35_host = Q35_HOST_DEVICE(qdev_create(NULL, TYPE_Q35_HOST_DEVICE));
 
@@ -245,6 +242,9 @@ static void pc_q35_init(MachineState *machine)
     object_property_set_link(OBJECT(machine), OBJECT(lpc),
                              PC_MACHINE_ACPI_DEVICE_PROP, &error_abort);
 
+    /* irq lines */
+    gsi_state = pc_gsi_create(&pcms->gsi, pcmc->pci_enabled);
+
     ich9_lpc = ICH9_LPC_DEVICE(lpc);
     lpc_dev = DEVICE(lpc);
     for (i = 0; i < GSI_NUM_PINS; i++) {
-- 
2.21.0

[PATCH 4/5] hw/i386/pc: Extract pc_i8259_create()

[Philippe Mathieu-Daudé]

The i8259 creation code is common to all PC machines, extract the
common code.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/pc.c         | 19 +++++++++++++++++++
 hw/i386/pc_piix.c    | 13 +------------
 hw/i386/pc_q35.c     | 14 +-------------
 include/hw/i386/pc.h |  1 +
 4 files changed, 22 insertions(+), 25 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 21efde33a5..16703c5edb 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1981,6 +1981,25 @@ void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus)
     rom_reset_order_override();
 }
 
+void pc_i8259_create(ISABus *isa_bus, qemu_irq *i8259_irqs)
+{
+    qemu_irq *i8259;
+
+    if (kvm_pic_in_kernel()) {
+        i8259 = kvm_i8259_init(isa_bus);
+    } else if (xen_enabled()) {
+        i8259 = xen_interrupt_controller_init();
+    } else {
+        i8259 = i8259_init(isa_bus, pc_allocate_cpu_irq());
+    }
+
+    for (size_t i = 0; i < ISA_NUM_IRQS; i++) {
+        i8259_irqs[i] = i8259[i];
+    }
+
+    g_free(i8259);
+}
+
 void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
 {
     DeviceState *dev;
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 47bdc64f64..1200ac6c0b 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -82,7 +82,6 @@ static void pc_init1(MachineState *machine,
     ISABus *isa_bus;
     PCII440FXState *i440fx_state;
     int piix3_devfn = -1;
-    qemu_irq *i8259;
     qemu_irq smi_irq;
     GSIState *gsi_state;
     DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
@@ -215,18 +214,8 @@ static void pc_init1(MachineState *machine,
     }
     isa_bus_irqs(isa_bus, pcms->gsi);
 
-    if (kvm_pic_in_kernel()) {
-        i8259 = kvm_i8259_init(isa_bus);
-    } else if (xen_enabled()) {
-        i8259 = xen_interrupt_controller_init();
-    } else {
-        i8259 = i8259_init(isa_bus, pc_allocate_cpu_irq());
-    }
+    pc_i8259_create(isa_bus, gsi_state->i8259_irq);
 
-    for (i = 0; i < ISA_NUM_IRQS; i++) {
-        gsi_state->i8259_irq[i] = i8259[i];
-    }
-    g_free(i8259);
     if (pcmc->pci_enabled) {
         ioapic_init_gsi(gsi_state, "i440fx");
     }
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 6d096eff28..f4fb9a02ba 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -128,7 +128,6 @@ static void pc_q35_init(MachineState *machine)
     MemoryRegion *ram_memory;
     GSIState *gsi_state;
     ISABus *isa_bus;
-    qemu_irq *i8259;
     int i;
     ICH9LPCState *ich9_lpc;
     PCIDevice *ahci;
@@ -255,18 +254,7 @@ static void pc_q35_init(MachineState *machine)
     pci_bus_set_route_irq_fn(host_bus, ich9_route_intx_pin_to_irq);
     isa_bus = ich9_lpc->isa_bus;
 
-    if (kvm_pic_in_kernel()) {
-        i8259 = kvm_i8259_init(isa_bus);
-    } else if (xen_enabled()) {
-        i8259 = xen_interrupt_controller_init();
-    } else {
-        i8259 = i8259_init(isa_bus, pc_allocate_cpu_irq());
-    }
-
-    for (i = 0; i < ISA_NUM_IRQS; i++) {
-        gsi_state->i8259_irq[i] = i8259[i];
-    }
-    g_free(i8259);
+    pc_i8259_create(isa_bus, gsi_state->i8259_irq);
 
     if (pcmc->pci_enabled) {
         ioapic_init_gsi(gsi_state, "q35");
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 9ad417cef0..06126034ae 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -239,6 +239,7 @@ void pc_pci_device_init(PCIBus *pci_bus);
 
 typedef void (*cpu_set_smm_t)(int smm, void *arg);
 
+void pc_i8259_create(ISABus *isa_bus, qemu_irq *i8259_irqs);
 void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name);
 
 ISADevice *pc_find_fdc0(void);
-- 
2.21.0

Re: [PATCH 0/5] hw/i386/pc: Extract pc_gsi_create() and pc_i8259_create()

[Paolo Bonzini]

On 18/10/19 15:59, Philippe Mathieu-Daudé wrote:
> These are few patches extracted from the previous too big series:
> hw/i386/pc: Split PIIX3 southbridge from i440FX northbridge
> https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg03685.html
> 
> Dropped "Move kvm_i8259_init() declaration to sysemu/kvm.h" (thuth),
> no logical changes:

I queued this, but neither I nor patchew got patch 5.  I just got it
from the PIIX3/i440FX series.

paolo

> $ git backport-diff -u pc_split_i440fx_piix-v1
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
> 
> 001/5:[----] [-C] 'hw/i386/pc: Extract pc_gsi_create()'
> 002/5:[----] [--] 'hw/i386/pc: Reduce gsi_handler scope'
> 003/5:[----] [--] 'hw/i386/pc: Move gsi_state creation code'
> 004/5:[----] [--] 'hw/i386/pc: Extract pc_i8259_create()'
> 005/5:[----] [--] 'hw/i386/pc: Remove kvm_i386.h include'
> 
> Based-on: <20191018134754.16362-1-philmd@redhat.com>
> hw/i386/pc: Split PIIX3 southbridge from i440FX northbridge [v2]
> https://mid.mail-archive.com/20191018134754.16362-1-philmd@redhat.com
> 
> Philippe Mathieu-Daudé (5):
>   hw/i386/pc: Extract pc_gsi_create()
>   hw/i386/pc: Reduce gsi_handler scope
>   hw/i386/pc: Move gsi_state creation code
>   hw/i386/pc: Extract pc_i8259_create()
>   hw/i386/pc: Remove kvm_i386.h include
> 
>  hw/i386/pc.c         | 36 +++++++++++++++++++++++++++++++++++-
>  hw/i386/pc_piix.c    | 23 ++---------------------
>  hw/i386/pc_q35.c     | 28 ++++------------------------
>  include/hw/i386/pc.h |  3 ++-
>  4 files changed, 43 insertions(+), 47 deletions(-)
> 

Re: [PATCH 0/5] hw/i386/pc: Extract pc_gsi_create() and pc_i8259_create()

[Philippe Mathieu-Daudé]

On 10/22/19 6:55 PM, Paolo Bonzini wrote:
> On 18/10/19 15:59, Philippe Mathieu-Daudé wrote:
>> These are few patches extracted from the previous too big series:
>> hw/i386/pc: Split PIIX3 southbridge from i440FX northbridge
>> https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg03685.html
>>
>> Dropped "Move kvm_i8259_init() declaration to sysemu/kvm.h" (thuth),
>> no logical changes:
> 
> I queued this, but neither I nor patchew got patch 5.  I just got it
> from the PIIX3/i440FX series.

Odd... Yes this is the same patch resent with no change. Thanks for
noticing this and carrying about finding it!

> 
> paolo
> 
>> $ git backport-diff -u pc_split_i440fx_piix-v1
>> Key:
>> [----] : patches are identical
>> [####] : number of functional differences between upstream/downstream patch
>> [down] : patch is downstream-only
>> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
>>
>> 001/5:[----] [-C] 'hw/i386/pc: Extract pc_gsi_create()'
>> 002/5:[----] [--] 'hw/i386/pc: Reduce gsi_handler scope'
>> 003/5:[----] [--] 'hw/i386/pc: Move gsi_state creation code'
>> 004/5:[----] [--] 'hw/i386/pc: Extract pc_i8259_create()'
>> 005/5:[----] [--] 'hw/i386/pc: Remove kvm_i386.h include'
>>
>> Based-on: <20191018134754.16362-1-philmd@redhat.com>
>> hw/i386/pc: Split PIIX3 southbridge from i440FX northbridge [v2]
>> https://mid.mail-archive.com/20191018134754.16362-1-philmd@redhat.com
>>
>> Philippe Mathieu-Daudé (5):
>>    hw/i386/pc: Extract pc_gsi_create()
>>    hw/i386/pc: Reduce gsi_handler scope
>>    hw/i386/pc: Move gsi_state creation code
>>    hw/i386/pc: Extract pc_i8259_create()
>>    hw/i386/pc: Remove kvm_i386.h include
>>
>>   hw/i386/pc.c         | 36 +++++++++++++++++++++++++++++++++++-
>>   hw/i386/pc_piix.c    | 23 ++---------------------
>>   hw/i386/pc_q35.c     | 28 ++++------------------------
>>   include/hw/i386/pc.h |  3 ++-
>>   4 files changed, 43 insertions(+), 47 deletions(-)
>>
> 

Re: [PATCH 0/5] hw/i386/pc: Extract pc_gsi_create() and pc_i8259_create()

[Philippe Mathieu-Daudé]

Hi Paolo,

On 10/22/19 7:32 PM, Philippe Mathieu-Daudé wrote:
> On 10/22/19 6:55 PM, Paolo Bonzini wrote:
>> On 18/10/19 15:59, Philippe Mathieu-Daudé wrote:
>>> These are few patches extracted from the previous too big series:
>>> hw/i386/pc: Split PIIX3 southbridge from i440FX northbridge
>>> https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg03685.html
>>>
>>> Dropped "Move kvm_i8259_init() declaration to sysemu/kvm.h" (thuth),
>>> no logical changes:
>>
>> I queued this, but neither I nor patchew got patch 5.  I just got it
>> from the PIIX3/i440FX series.

In the patch 'hw/i386: Introduce the microvm machine type'
Sergio uses gsi_handler(), so you need to drop 'hw/i386/pc:
Reduce gsi_handler scope' to keep microvm building.

> Odd... Yes this is the same patch resent with no change. Thanks for
> noticing this and carrying about finding it!
> 
>>
>> paolo
>>
>>> $ git backport-diff -u pc_split_i440fx_piix-v1
>>> Key:
>>> [----] : patches are identical
>>> [####] : number of functional differences between upstream/downstream 
>>> patch
>>> [down] : patch is downstream-only
>>> The flags [FC] indicate (F)unctional and (C)ontextual differences, 
>>> respectively
>>>
>>> 001/5:[----] [-C] 'hw/i386/pc: Extract pc_gsi_create()'
>>> 002/5:[----] [--] 'hw/i386/pc: Reduce gsi_handler scope'
>>> 003/5:[----] [--] 'hw/i386/pc: Move gsi_state creation code'
>>> 004/5:[----] [--] 'hw/i386/pc: Extract pc_i8259_create()'
>>> 005/5:[----] [--] 'hw/i386/pc: Remove kvm_i386.h include'
>>>
>>> Based-on: <20191018134754.16362-1-philmd@redhat.com>
>>> hw/i386/pc: Split PIIX3 southbridge from i440FX northbridge [v2]
>>> https://mid.mail-archive.com/20191018134754.16362-1-philmd@redhat.com
>>>
>>> Philippe Mathieu-Daudé (5):
>>>    hw/i386/pc: Extract pc_gsi_create()
>>>    hw/i386/pc: Reduce gsi_handler scope
>>>    hw/i386/pc: Move gsi_state creation code
>>>    hw/i386/pc: Extract pc_i8259_create()
>>>    hw/i386/pc: Remove kvm_i386.h include
>>>
>>>   hw/i386/pc.c         | 36 +++++++++++++++++++++++++++++++++++-
>>>   hw/i386/pc_piix.c    | 23 ++---------------------
>>>   hw/i386/pc_q35.c     | 28 ++++------------------------
>>>   include/hw/i386/pc.h |  3 ++-
>>>   4 files changed, 43 insertions(+), 47 deletions(-)
>>>
>>

Re: [PATCH 0/5] hw/i386/pc: Extract pc_gsi_create() and pc_i8259_create()

[Paolo Bonzini]

On 26/10/19 16:55, Philippe Mathieu-Daudé wrote:
> In the patch 'hw/i386: Introduce the microvm machine type'
> Sergio uses gsi_handler(), so you need to drop 'hw/i386/pc:
> Reduce gsi_handler scope' to keep microvm building.

Indeed, and I also have patches on top to move those out of pc.c
(otherwise you cannot build CONFIG_MICROVM without either i440fx or q35).

Paolo