piix_pci cleanup

PATCH Superseded Juan Quintela 2 participants 8 messages Aug 21, 2009 – Aug 24, 2009 Kbuild

Share & tools

Juan Quintela

author

2009-08-21 17:38 UTC #0 lore raw

cover piix_pci cleanup

This series:
- split piix4 from piix_pci. The only shared code where piix_save/load, i.e. almost nothing. Once there, compile piix4.o only for mips (it was not used anywhere else).
- Move global variables to PCII440FXState. Nice and clean until....
- pci_irq_levels: This is great:
* it is saved/loaded from i440fx
* it is cleaned during reset in piix3
* it is used in piix3_set_irq, that don't receive neither i440fx nor piix state, it needs to be a global variable. (created a global link until a better solution appear).

I looked where to "hide" pci_irq_levels and piix3_dev (both needed in piix3_set_irq), and didn't found where to pass them, out of:
- hack i8259 to hide it into PicState2 (that one got passed through the pic) ugly for words, but will work
- try to add <something> at setup_irq time, but at that point we have:
* opaque -> pci_dev of device that we are working with
* from there we can get to the bus that the device is attached to, but no way to go from there to the Host bridge (that is what we wanted in the 1st place)

Notice that this is needed for both pc and mips_malta.c

Ideas on where to hide a pci_dev inside a bus?

Later, Juan.

Juan Quintela (5):
piix4 don't use pci_irq_levels at all
Split piix4 support from piix_pci.c
Use PCII440FXState instead of generic PCIDevice Move smm_enabled and isa_memory_mappings to PCII440FXState Cleanup of pci_irq_levels belong to i440fx

 Makefile.target |    2 +-
 hw/pc.c         |    2 +-
 hw/pc.h         |   10 +++-
 hw/piix4.c      |  127 +++++++++++++++++++++++++++++++++++++++++++
 hw/piix_pci.c   |  161 +++++++++++++++++-------------------------------------
 5 files changed, 187 insertions(+), 115 deletions(-)
 create mode 100644 hw/piix4.c

Juan Quintela

author

2009-08-21 17:38 UTC #1 lore raw

PATCH 1/5 piix4 don't use pci_irq_levels at all

Signed-off-by Juan Quintela <[email protected]>

---
 hw/piix_pci.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

hw/piix_pci.c +0 -2

index e2ddf4b..7cf1d99 100644
@@ -321,8 +321,6 @@ static void piix4_reset(void *opaque)
     pci_conf[0xab] = 0x00;
     pci_conf[0xac] = 0x00;
     pci_conf[0xae] = 0x00;
-
-    memset(pci_irq_levels, 0, sizeof(pci_irq_levels));
 }
 
 static void piix_save(QEMUFile* f, void *opaque)

Signature · 1 line

1.6.2.5

Juan Quintela

author

2009-08-21 17:38 UTC #2 lore raw

PATCH 2/5 Split piix4 support from piix_pci.c

In reply to Juan Quintela

Now mips_malta uses piix4 and pc's use piix_pci definitions

Signed-off-by Juan Quintela <[email protected]>

---
 Makefile.target |    2 +-
 hw/pc.h         |    1 +
 hw/piix4.c      |  127 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/piix_pci.c   |   71 -------------------------------
 4 files changed, 129 insertions(+), 72 deletions(-)
 create mode 100644 hw/piix4.c

Makefile.target +1 -1

index 066af8d..4a5d917 100644
@@ -217,7 +217,7 @@ obj-mips-y = mips_r4k.o mips_jazz.o mips_malta.o mips_mipssim.o
 obj-mips-y += mips_timer.o mips_int.o dma.o vga.o serial.o i8254.o i8259.o rc4030.o
 obj-mips-y += g364fb.o jazz_led.o dp8393x.o
 obj-mips-y += ide.o gt64xxx.o pckbd.o fdc.o mc146818rtc.o usb-uhci.o acpi.o ds1225y.o
-obj-mips-y += piix_pci.o parallel.o cirrus_vga.o isa-bus.o pcspk.o $(sound-obj-y)
+obj-mips-y += piix4.o parallel.o cirrus_vga.o isa-bus.o pcspk.o $(sound-obj-y)
 obj-mips-y += mipsnet.o
 obj-mips-y += pflash_cfi01.o
 obj-mips-y += vmware_vga.o

hw/pc.h +1 -0

index 58d569b..bfa52d6 100644
@@ -124,6 +124,7 @@ void i440fx_set_smm(PCIDevice *d, int val);
 int piix3_init(PCIBus *bus, int devfn);
 void i440fx_init_memory_mappings(PCIDevice *d);
 
+/* piix4.c */
 extern PCIDevice *piix4_dev;
 int piix4_init(PCIBus *bus, int devfn);

hw/piix4.c new file +127 -0

127 lines not on this page · raw message

hw/piix_pci.c +0 ~~-71~~

101 lines not on this page · raw message

Signature · 1 line

1.6.2.5

Juan Quintela

author

2009-08-21 17:38 UTC #3 lore raw

PATCH 3/5 Use PCII440FXState instead of generic PCIDevice

In reply to Juan Quintela

Signed-off-by Juan Quintela <[email protected]>

---
 hw/pc.c       |    2 +-
 hw/pc.h       |    9 ++++++---
 hw/piix_pci.c |   54 +++++++++++++++++++++++++++++++-----------------------
 3 files changed, 38 insertions(+), 27 deletions(-)

hw/pc.c +1 -1

index cc6e7e8..a908f8b 100644
@@ -61,7 +61,7 @@ static fdctrl_t *floppy_controller;
 static RTCState *rtc_state;
 static PITState *pit;
 static IOAPICState *ioapic;
-static PCIDevice *i440fx_state;
+static PCII440FXState *i440fx_state;
 
 typedef struct rom_reset_data {
     uint8_t *data;

hw/pc.h +6 -3

index bfa52d6..7733d1a 100644
@@ -119,10 +119,13 @@ void pcspk_init(PITState *);
 int pcspk_audio_init(qemu_irq *pic);
 
 /* piix_pci.c */
-PCIBus *i440fx_init(PCIDevice **pi440fx_state, qemu_irq *pic);
-void i440fx_set_smm(PCIDevice *d, int val);
+struct PCII440FXState;
+typedef struct PCII440FXState PCII440FXState;
+
+PCIBus *i440fx_init(PCII440FXState **pi440fx_state, qemu_irq *pic);
+void i440fx_set_smm(PCII440FXState *d, int val);
 int piix3_init(PCIBus *bus, int devfn);
-void i440fx_init_memory_mappings(PCIDevice *d);
+void i440fx_init_memory_mappings(PCII440FXState *d);
 
 /* piix4.c */
 extern PCIDevice *piix4_dev;

hw/piix_pci.c +31 ~~-23~~

142 lines not on this page · raw message

Signature · 1 line

1.6.2.5

Juan Quintela

author

2009-08-21 17:38 UTC #4 lore raw

PATCH 4/5 Move smm_enabled and isa_memory_mappings to PCII440FXState

In reply to Juan Quintela

Signed-off-by Juan Quintela <[email protected]>

---
 hw/piix_pci.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

hw/piix_pci.c +10 ~~-10~~

index 2450c31..d8aeeec 100644
@@ -35,6 +35,8 @@ typedef PCIHostState I440FXState;
 
 struct PCII440FXState {
     PCIDevice dev;
+    target_phys_addr_t isa_page_descs[384 / 4];
+    uint8_t smm_enabled;
 };
 
 static void i440fx_addr_writel(void* opaque, uint32_t addr, uint32_t val)

@@ -61,8 +63,6 @@ static int pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
     return (irq_num + slot_addend) & 3;
 }
 
-static target_phys_addr_t isa_page_descs[384 / 4];
-static uint8_t smm_enabled;
 static int pci_irq_levels[4];
 
 static void update_pam(PCII440FXState *d, uint32_t start, uint32_t end, int r)

@@ -86,7 +86,7 @@ static void update_pam(PCII440FXState *d, uint32_t start, uint32_t end, int r)
         /* XXX: should distinguish read/write cases */
         for(addr = start; addr < end; addr += 4096) {
             cpu_register_physical_memory(addr, 4096,
-                                         isa_page_descs[(addr - 0xa0000) >> 12]);
+                                         d->isa_page_descs[(addr - 0xa0000) >> 12]);
         }
         break;
     }

@@ -103,12 +103,12 @@ static void i440fx_update_memory_mappings(PCII440FXState *d)
         update_pam(d, 0xc0000 + 0x4000 * i, 0xc0000 + 0x4000 * (i + 1), r);
     }
     smram = d->dev.config[0x72];
-    if ((smm_enabled && (smram & 0x08)) || (smram & 0x40)) {
+    if ((d->smm_enabled && (smram & 0x08)) || (smram & 0x40)) {
         cpu_register_physical_memory(0xa0000, 0x20000, 0xa0000);
     } else {
         for(addr = 0xa0000; addr < 0xc0000; addr += 4096) {
             cpu_register_physical_memory(addr, 4096,
-                                         isa_page_descs[(addr - 0xa0000) >> 12]);
+                                         d->isa_page_descs[(addr - 0xa0000) >> 12]);
         }
     }
 }

@@ -116,8 +116,8 @@ static void i440fx_update_memory_mappings(PCII440FXState *d)
 void i440fx_set_smm(PCII440FXState *d, int val)
 {
     val = (val != 0);
-    if (smm_enabled != val) {
-        smm_enabled = val;
+    if (d->smm_enabled != val) {
+        d->smm_enabled = val;
         i440fx_update_memory_mappings(d);
     }
 }

@@ -130,7 +130,7 @@ void i440fx_init_memory_mappings(PCII440FXState *d)
 {
     int i;
     for(i = 0; i < 96; i++) {
-        isa_page_descs[i] = cpu_get_physical_page_desc(0xa0000 + i * 0x1000);
+        d->isa_page_descs[i] = cpu_get_physical_page_desc(0xa0000 + i * 0x1000);
     }
 }

@@ -151,7 +151,7 @@ static void i440fx_save(QEMUFile* f, void *opaque)
     int i;
 
     pci_device_save(&d->dev, f);
-    qemu_put_8s(f, &smm_enabled);
+    qemu_put_8s(f, &d->smm_enabled);
 
     for (i = 0; i < 4; i++)
         qemu_put_be32(f, pci_irq_levels[i]);

@@ -168,7 +168,7 @@ static int i440fx_load(QEMUFile* f, void *opaque, int version_id)
     if (ret < 0)
         return ret;
     i440fx_update_memory_mappings(d);
-    qemu_get_8s(f, &smm_enabled);
+    qemu_get_8s(f, &d->smm_enabled);
 
     if (version_id >= 2)
         for (i = 0; i < 4; i++)

Signature · 1 line

1.6.2.5

Juan Quintela

author

2009-08-21 17:38 UTC #5 lore raw

PATCH 5/5 Cleanup of pci_irq_levels belong to i440fx

In reply to Juan Quintela

Do there in a new reset function.

Signed-off-by Juan Quintela <[email protected]>

---
 hw/piix_pci.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

hw/piix_pci.c +10 -4

index d8aeeec..bd6ab9b 100644
@@ -63,8 +63,6 @@ static int pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
     return (irq_num + slot_addend) & 3;
 }
 
-static int pci_irq_levels[4];
-
 static void update_pam(PCII440FXState *d, uint32_t start, uint32_t end, int r)
 {
     uint32_t addr;

@@ -177,6 +175,13 @@ static int i440fx_load(QEMUFile* f, void *opaque, int version_id)
     return 0;
 }
 
+static void i440fx_reset(void *opaque)
+{
+    PCII440FXState *d = opaque;
+
+    memset(d->pci_irq_levels, 0, sizeof(d->pci_irq_levels));
+}
+
 static void i440fx_pcihost_initfn(SysBusDevice *dev)
 {
     I440FXState *s = FROM_SYSBUS(I440FXState, dev);

@@ -205,6 +210,9 @@ static void i440fx_initfn(PCIDevice *dev)
     d->dev.config[0x72] = 0x02; /* SMRAM */
 
     register_savevm("I440FX", 0, 2, i440fx_save, i440fx_load, d);
+
+    i440fx_reset(d);
+    qemu_register_reset(i440fx_reset, d);
 }
 
 PCIBus *i440fx_init(PCII440FXState **pi440fx_state, qemu_irq *pic)

@@ -288,8 +296,6 @@ static void piix3_reset(void *opaque)
     pci_conf[0xab] = 0x00;
     pci_conf[0xac] = 0x00;
     pci_conf[0xae] = 0x00;
-
-    memset(pci_irq_levels, 0, sizeof(pci_irq_levels));
 }
 
 static void piix_save(QEMUFile* f, void *opaque)

Signature · 1 line

1.6.2.5

Blue Swirl

2009-08-21 19:59 UTC #6 lore raw

In reply to Juan Quintela

Quoted 24 lines · Juan Quintela

Hi
This series:
- split piix4 from piix_pci. The only shared code where piix_save/load, i.e. almost nothing. Once there, compile piix4.o only for mips (it was not used anywhere else).
- Move global variables to PCII440FXState. Nice and clean until....
- pci_irq_levels: This is great:
* it is saved/loaded from i440fx
* it is cleaned during reset in piix3
* it is used in piix3_set_irq, that don't receive neither i440fx nor piix state, it needs to be a global variable. (created a global link until a better solution appear).
I looked where to "hide" pci_irq_levels and piix3_dev (both needed in piix3_set_irq), and didn't found where to pass them, out of:
- hack i8259 to hide it into PicState2 (that one got passed through the pic) ugly for words, but will work
- try to add <something> at setup_irq time, but at that point we have:
* opaque -> pci_dev of device that we are working with
* from there we can get to the bus that the device is attached to, but no way to go from there to the Host bridge (that is what we wanted in the 1st place)

Quoted 3 lines

Notice that this is needed for both pc and mips_malta.c
Ideas on where to hide a pci_dev inside a bus?

pci_set_irq_fn should take an opaque state pointer (given at pci_register_bus time):

typedef void (*pci_set_irq_fn)(void *opaque, qemu_irq *pic, int irq_num, int level);

PCIBus *pci_register_bus(DeviceState *parent, const char *name,
pci_set_irq_fn set_irq, pci_map_irq_fn map_irq, qemu_irq *pic, void *irq_opaque, int
devfn_min, int nirq);

Juan Quintela

author

2009-08-24 15:23 UTC #7 lore raw

Quoted 32 lines · Blue Swirl

Quoted 24 lines · Juan Quintela
Hi
This series:
- split piix4 from piix_pci. The only shared code where piix_save/load, i.e. almost nothing. Once there, compile piix4.o only for mips (it was not used anywhere else).
- Move global variables to PCII440FXState. Nice and clean until....
- pci_irq_levels: This is great:
* it is saved/loaded from i440fx
* it is cleaned during reset in piix3
* it is used in piix3_set_irq, that don't receive neither i440fx nor piix state, it needs to be a global variable. (created a global link until a better solution appear).
I looked where to "hide" pci_irq_levels and piix3_dev (both needed in piix3_set_irq), and didn't found where to pass them, out of:
- hack i8259 to hide it into PicState2 (that one got passed through the pic) ugly for words, but will work
- try to add <something> at setup_irq time, but at that point we have:
* opaque -> pci_dev of device that we are working with
* from there we can get to the bus that the device is attached to, but no way to go from there to the Host bridge (that is what we wanted in the 1st place)
Quoted 3 lines
Notice that this is needed for both pc and mips_malta.c
Ideas on where to hide a pci_dev inside a bus?
pci_set_irq_fn should take an opaque state pointer (given at pci_register_bus time):

Quoted 7 lines

typedef void (*pci_set_irq_fn)(void *opaque, qemu_irq *pic, int irq_num, int level);
PCIBus *pci_register_bus(DeviceState *parent, const char *name,
pci_set_irq_fn set_irq, pci_map_irq_fn map_irq, qemu_irq *pic, void *irq_opaque, int
devfn_min, int nirq);

I fixed it changing the type of qemu_irq *pic to void *irq_state. See the v2 of the series.

Thanks very much for the suggestion.

later, Juan.