diff mbox series

[v6,28/33] hw/isa/piix3: Merge hw/isa/piix4.c

Message ID 20230109172347.1830-29-shentey@gmail.com (mailing list archive)
State New, archived
Headers show
Series Consolidate PIIX south bridges | expand

Commit Message

Bernhard Beschow Jan. 9, 2023, 5:23 p.m. UTC
Now that the PIIX3 and PIIX4 device models are sufficiently consolidated,
their implementations can be merged into one file for further
consolidation.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20221022150508.26830-37-shentey@gmail.com>
---
 hw/isa/{piix3.c => piix.c} | 158 ++++++++++++++++++++
 hw/isa/piix4.c             | 285 -------------------------------------
 MAINTAINERS                |   6 +-
 hw/i386/Kconfig            |   2 +-
 hw/isa/Kconfig             |  12 +-
 hw/isa/meson.build         |   3 +-
 hw/mips/Kconfig            |   2 +-
 7 files changed, 165 insertions(+), 303 deletions(-)
 rename hw/isa/{piix3.c => piix.c} (75%)
 delete mode 100644 hw/isa/piix4.c

Comments

Philippe Mathieu-Daudé Jan. 12, 2023, 12:50 p.m. UTC | #1
On 9/1/23 18:23, Bernhard Beschow wrote:
> Now that the PIIX3 and PIIX4 device models are sufficiently consolidated,
> their implementations can be merged into one file for further
> consolidation.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Message-Id: <20221022150508.26830-37-shentey@gmail.com>
> ---
>   hw/isa/{piix3.c => piix.c} | 158 ++++++++++++++++++++
>   hw/isa/piix4.c             | 285 -------------------------------------
>   MAINTAINERS                |   6 +-
>   hw/i386/Kconfig            |   2 +-
>   hw/isa/Kconfig             |  12 +-
>   hw/isa/meson.build         |   3 +-
>   hw/mips/Kconfig            |   2 +-
>   7 files changed, 165 insertions(+), 303 deletions(-)
>   rename hw/isa/{piix3.c => piix.c} (75%)
>   delete mode 100644 hw/isa/piix4.c

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Philippe Mathieu-Daudé Jan. 12, 2023, 1:32 p.m. UTC | #2
On 12/1/23 13:50, Philippe Mathieu-Daudé wrote:
> On 9/1/23 18:23, Bernhard Beschow wrote:
>> Now that the PIIX3 and PIIX4 device models are sufficiently consolidated,
>> their implementations can be merged into one file for further
>> consolidation.
>>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>> Message-Id: <20221022150508.26830-37-shentey@gmail.com>
>> ---
>>   hw/isa/{piix3.c => piix.c} | 158 ++++++++++++++++++++
>>   hw/isa/piix4.c             | 285 -------------------------------------
>>   MAINTAINERS                |   6 +-
>>   hw/i386/Kconfig            |   2 +-
>>   hw/isa/Kconfig             |  12 +-
>>   hw/isa/meson.build         |   3 +-
>>   hw/mips/Kconfig            |   2 +-
>>   7 files changed, 165 insertions(+), 303 deletions(-)
>>   rename hw/isa/{piix3.c => piix.c} (75%)
>>   delete mode 100644 hw/isa/piix4.c
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

BTW I wonder why PIIX4 isn't calling pci_bus_set_route_irq_fn()...
Any clue?
Philippe Mathieu-Daudé Jan. 12, 2023, 3:04 p.m. UTC | #3
On 9/1/23 18:23, Bernhard Beschow wrote:
> Now that the PIIX3 and PIIX4 device models are sufficiently consolidated,
> their implementations can be merged into one file for further
> consolidation.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Message-Id: <20221022150508.26830-37-shentey@gmail.com>
> ---
>   hw/isa/{piix3.c => piix.c} | 158 ++++++++++++++++++++
>   hw/isa/piix4.c             | 285 -------------------------------------
>   MAINTAINERS                |   6 +-
>   hw/i386/Kconfig            |   2 +-
>   hw/isa/Kconfig             |  12 +-
>   hw/isa/meson.build         |   3 +-
>   hw/mips/Kconfig            |   2 +-
>   7 files changed, 165 insertions(+), 303 deletions(-)
>   rename hw/isa/{piix3.c => piix.c} (75%)
>   delete mode 100644 hw/isa/piix4.c

> +static void piix4_realize(PCIDevice *dev, Error **errp)
> +{
> +    PIIXState *s = PIIX_PCI_DEVICE(dev);
> +    PCIBus *pci_bus = pci_get_bus(dev);
> +    ISABus *isa_bus;
> +
> +    isa_bus = isa_bus_new(DEVICE(dev), pci_address_space(dev),
> +                          pci_address_space_io(dev), errp);
> +    if (!isa_bus) {
> +        return;
> +    }
> +
> +    memory_region_init_io(&s->rcr_mem, OBJECT(dev), &rcr_ops, s,
> +                          "reset-control", 1);
> +    memory_region_add_subregion_overlap(pci_address_space_io(dev),
> +                                        PIIX_RCR_IOPORT, &s->rcr_mem, 1);
> +
> +    /* initialize i8259 pic */
> +    if (!qdev_realize(DEVICE(&s->pic), NULL, errp)) {
> +        return;
> +    }
> +
> +    /* initialize ISA irqs */
> +    isa_bus_irqs(isa_bus, s->pic.in_irqs);
> +
> +    /* initialize pit */
> +    i8254_pit_init(isa_bus, 0x40, 0, NULL);
> +
> +    /* DMA */
> +    i8257_dma_init(isa_bus, 0);
> +
> +    /* RTC */
> +    qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
> +    if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
> +        return;
> +    }
> +    s->rtc.irq = qdev_get_gpio_in(DEVICE(&s->pic), s->rtc.isairq);

Pre-existing; it seems this difference with PIIX3 can be removed
because already taken care by calling isa_connect_gpio_out() in 
mc146818_rtc_init()?

ISADevice *mc146818_rtc_init(ISABus *bus, int base_year, qemu_irq 
intercept_irq)
{
     DeviceState *dev;
     ISADevice *isadev;
     RTCState *s;

     isadev = isa_new(TYPE_MC146818_RTC);
     dev = DEVICE(isadev);
     s = MC146818_RTC(isadev);
     qdev_prop_set_int32(dev, "base_year", base_year);
     isa_realize_and_unref(isadev, bus, &error_fatal);
     if (intercept_irq) {
         qdev_connect_gpio_out(dev, 0, intercept_irq);
     } else {
         isa_connect_gpio_out(isadev, 0, s->isairq);

Having:

void isa_connect_gpio_out(ISADevice *isadev, int gpioirq, unsigned isairq)
{
     qemu_irq irq = isa_get_irq(isadev, isairq);
     qdev_connect_gpio_out(DEVICE(isadev), gpioirq, irq);
}

What do you think?
Philippe Mathieu-Daudé Jan. 12, 2023, 4:31 p.m. UTC | #4
On 12/1/23 16:04, Philippe Mathieu-Daudé wrote:
> On 9/1/23 18:23, Bernhard Beschow wrote:
>> Now that the PIIX3 and PIIX4 device models are sufficiently consolidated,
>> their implementations can be merged into one file for further
>> consolidation.
>>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>> Message-Id: <20221022150508.26830-37-shentey@gmail.com>
>> ---
>>   hw/isa/{piix3.c => piix.c} | 158 ++++++++++++++++++++
>>   hw/isa/piix4.c             | 285 -------------------------------------
>>   MAINTAINERS                |   6 +-
>>   hw/i386/Kconfig            |   2 +-
>>   hw/isa/Kconfig             |  12 +-
>>   hw/isa/meson.build         |   3 +-
>>   hw/mips/Kconfig            |   2 +-
>>   7 files changed, 165 insertions(+), 303 deletions(-)
>>   rename hw/isa/{piix3.c => piix.c} (75%)
>>   delete mode 100644 hw/isa/piix4.c
> 
>> +static void piix4_realize(PCIDevice *dev, Error **errp)
>> +{
>> +    PIIXState *s = PIIX_PCI_DEVICE(dev);
>> +    PCIBus *pci_bus = pci_get_bus(dev);
>> +    ISABus *isa_bus;
>> +
>> +    isa_bus = isa_bus_new(DEVICE(dev), pci_address_space(dev),
>> +                          pci_address_space_io(dev), errp);
>> +    if (!isa_bus) {
>> +        return;
>> +    }
>> +
>> +    memory_region_init_io(&s->rcr_mem, OBJECT(dev), &rcr_ops, s,
>> +                          "reset-control", 1);
>> +    memory_region_add_subregion_overlap(pci_address_space_io(dev),
>> +                                        PIIX_RCR_IOPORT, &s->rcr_mem, 
>> 1);
>> +
>> +    /* initialize i8259 pic */
>> +    if (!qdev_realize(DEVICE(&s->pic), NULL, errp)) {
>> +        return;
>> +    }
>> +
>> +    /* initialize ISA irqs */
>> +    isa_bus_irqs(isa_bus, s->pic.in_irqs);
>> +
>> +    /* initialize pit */
>> +    i8254_pit_init(isa_bus, 0x40, 0, NULL);
>> +
>> +    /* DMA */
>> +    i8257_dma_init(isa_bus, 0);
>> +
>> +    /* RTC */
>> +    qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
>> +    if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
>> +        return;
>> +    }
>> +    s->rtc.irq = qdev_get_gpio_in(DEVICE(&s->pic), s->rtc.isairq);
> 
> Pre-existing; it seems this difference with PIIX3 can be removed
> because already taken care by calling isa_connect_gpio_out() in 
> mc146818_rtc_init()?
> 
> ISADevice *mc146818_rtc_init(ISABus *bus, int base_year, qemu_irq 
> intercept_irq)
> {
>      DeviceState *dev;
>      ISADevice *isadev;
>      RTCState *s;
> 
>      isadev = isa_new(TYPE_MC146818_RTC);
>      dev = DEVICE(isadev);
>      s = MC146818_RTC(isadev);
>      qdev_prop_set_int32(dev, "base_year", base_year);
>      isa_realize_and_unref(isadev, bus, &error_fatal);
>      if (intercept_irq) {
>          qdev_connect_gpio_out(dev, 0, intercept_irq);
>      } else {
>          isa_connect_gpio_out(isadev, 0, s->isairq);
> 

I meant to paste:

static void rtc_realizefn(DeviceState *dev, Error **errp)
{
     ...
     qdev_init_gpio_out(dev, &s->irq, 1);


> Having:
> 
> void isa_connect_gpio_out(ISADevice *isadev, int gpioirq, unsigned isairq)
> {
>      qemu_irq irq = isa_get_irq(isadev, isairq);
>      qdev_connect_gpio_out(DEVICE(isadev), gpioirq, irq);
> }
> 
> What do you think?
Philippe Mathieu-Daudé Jan. 12, 2023, 4:36 p.m. UTC | #5
On 9/1/23 18:23, Bernhard Beschow wrote:
> Now that the PIIX3 and PIIX4 device models are sufficiently consolidated,
> their implementations can be merged into one file for further
> consolidation.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Message-Id: <20221022150508.26830-37-shentey@gmail.com>
> ---
>   hw/isa/{piix3.c => piix.c} | 158 ++++++++++++++++++++
>   hw/isa/piix4.c             | 285 -------------------------------------
>   MAINTAINERS                |   6 +-
>   hw/i386/Kconfig            |   2 +-
>   hw/isa/Kconfig             |  12 +-
>   hw/isa/meson.build         |   3 +-
>   hw/mips/Kconfig            |   2 +-
>   7 files changed, 165 insertions(+), 303 deletions(-)
>   rename hw/isa/{piix3.c => piix.c} (75%)
>   delete mode 100644 hw/isa/piix4.c


> @@ -489,11 +534,124 @@ static const TypeInfo piix3_xen_info = {
>       .class_init    = piix3_xen_class_init,
>   };
>   
> +static void piix4_realize(PCIDevice *dev, Error **errp)
> +{

> +    /* initialize pit */
> +    i8254_pit_init(isa_bus, 0x40, 0, NULL);
Pre-existing, why there is no equivalent PIT creation in the
PIIX3 variant? Due to in-kernel PIT in KVM?
Bernhard Beschow Jan. 12, 2023, 6:03 p.m. UTC | #6
Am 12. Januar 2023 16:31:23 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 12/1/23 16:04, Philippe Mathieu-Daudé wrote:
>> On 9/1/23 18:23, Bernhard Beschow wrote:
>>> Now that the PIIX3 and PIIX4 device models are sufficiently consolidated,
>>> their implementations can be merged into one file for further
>>> consolidation.
>>> 
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>> Message-Id: <20221022150508.26830-37-shentey@gmail.com>
>>> ---
>>>   hw/isa/{piix3.c => piix.c} | 158 ++++++++++++++++++++
>>>   hw/isa/piix4.c             | 285 -------------------------------------
>>>   MAINTAINERS                |   6 +-
>>>   hw/i386/Kconfig            |   2 +-
>>>   hw/isa/Kconfig             |  12 +-
>>>   hw/isa/meson.build         |   3 +-
>>>   hw/mips/Kconfig            |   2 +-
>>>   7 files changed, 165 insertions(+), 303 deletions(-)
>>>   rename hw/isa/{piix3.c => piix.c} (75%)
>>>   delete mode 100644 hw/isa/piix4.c
>> 
>>> +static void piix4_realize(PCIDevice *dev, Error **errp)
>>> +{
>>> +    PIIXState *s = PIIX_PCI_DEVICE(dev);
>>> +    PCIBus *pci_bus = pci_get_bus(dev);
>>> +    ISABus *isa_bus;
>>> +
>>> +    isa_bus = isa_bus_new(DEVICE(dev), pci_address_space(dev),
>>> +                          pci_address_space_io(dev), errp);
>>> +    if (!isa_bus) {
>>> +        return;
>>> +    }
>>> +
>>> +    memory_region_init_io(&s->rcr_mem, OBJECT(dev), &rcr_ops, s,
>>> +                          "reset-control", 1);
>>> +    memory_region_add_subregion_overlap(pci_address_space_io(dev),
>>> +                                        PIIX_RCR_IOPORT, &s->rcr_mem, 1);
>>> +
>>> +    /* initialize i8259 pic */
>>> +    if (!qdev_realize(DEVICE(&s->pic), NULL, errp)) {
>>> +        return;
>>> +    }
>>> +
>>> +    /* initialize ISA irqs */
>>> +    isa_bus_irqs(isa_bus, s->pic.in_irqs);
>>> +
>>> +    /* initialize pit */
>>> +    i8254_pit_init(isa_bus, 0x40, 0, NULL);
>>> +
>>> +    /* DMA */
>>> +    i8257_dma_init(isa_bus, 0);
>>> +
>>> +    /* RTC */
>>> +    qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
>>> +    if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
>>> +        return;
>>> +    }
>>> +    s->rtc.irq = qdev_get_gpio_in(DEVICE(&s->pic), s->rtc.isairq);
>> 
>> Pre-existing; it seems this difference with PIIX3 can be removed
>> because already taken care by calling isa_connect_gpio_out() in mc146818_rtc_init()?
>> 
>> ISADevice *mc146818_rtc_init(ISABus *bus, int base_year, qemu_irq intercept_irq)
>> {
>>      DeviceState *dev;
>>      ISADevice *isadev;
>>      RTCState *s;
>> 
>>      isadev = isa_new(TYPE_MC146818_RTC);
>>      dev = DEVICE(isadev);
>>      s = MC146818_RTC(isadev);
>>      qdev_prop_set_int32(dev, "base_year", base_year);
>>      isa_realize_and_unref(isadev, bus, &error_fatal);
>>      if (intercept_irq) {
>>          qdev_connect_gpio_out(dev, 0, intercept_irq);
>>      } else {
>>          isa_connect_gpio_out(isadev, 0, s->isairq);
>> 
>
>I meant to paste:
>
>static void rtc_realizefn(DeviceState *dev, Error **errp)
>{
>    ...
>    qdev_init_gpio_out(dev, &s->irq, 1);
>
>
>> Having:
>> 
>> void isa_connect_gpio_out(ISADevice *isadev, int gpioirq, unsigned isairq)
>> {
>>      qemu_irq irq = isa_get_irq(isadev, isairq);
>>      qdev_connect_gpio_out(DEVICE(isadev), gpioirq, irq);
>> }
>> 
>> What do you think?
>

In "[PATCH v6 11/33] hw/i386/pc: Create RTC controllers in south bridges" mc146818_rtc_init() got inlined, taking into account the intercept_irq, which required the rtc interrupt to be wired up in board code. Since we don't have to deal with intercept_irq in the Malta code, wiring up of the rtc interrupt could be moved into PIIX4.

I would prefer to wire up the rtc interrupt in PIIX3 as well, and to re-wire it in board code in case of intercept_irq != NULL. That's still an open question which needs to be solved for PIIX4 to become a drop-in replacement for PIIX3. Any ideas?

Best regards,
Bernhard
Bernhard Beschow Jan. 12, 2023, 6:21 p.m. UTC | #7
Am 12. Januar 2023 16:36:30 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 9/1/23 18:23, Bernhard Beschow wrote:
>> Now that the PIIX3 and PIIX4 device models are sufficiently consolidated,
>> their implementations can be merged into one file for further
>> consolidation.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>> Message-Id: <20221022150508.26830-37-shentey@gmail.com>
>> ---
>>   hw/isa/{piix3.c => piix.c} | 158 ++++++++++++++++++++
>>   hw/isa/piix4.c             | 285 -------------------------------------
>>   MAINTAINERS                |   6 +-
>>   hw/i386/Kconfig            |   2 +-
>>   hw/isa/Kconfig             |  12 +-
>>   hw/isa/meson.build         |   3 +-
>>   hw/mips/Kconfig            |   2 +-
>>   7 files changed, 165 insertions(+), 303 deletions(-)
>>   rename hw/isa/{piix3.c => piix.c} (75%)
>>   delete mode 100644 hw/isa/piix4.c
>
>
>> @@ -489,11 +534,124 @@ static const TypeInfo piix3_xen_info = {
>>       .class_init    = piix3_xen_class_init,
>>   };
>>   +static void piix4_realize(PCIDevice *dev, Error **errp)
>> +{
>
>> +    /* initialize pit */
>> +    i8254_pit_init(isa_bus, 0x40, 0, NULL);
>Pre-existing, why there is no equivalent PIT creation in the
>PIIX3 variant? Due to in-kernel PIT in KVM?

Correct, that's one reason. The other reason is for interrupt wiring in case an HPET is present (see our discussion about the intercept_irq for the rtc).

I would like to create and wire up the PIT in PIIX3 as well. Since we'd have to take KVM into account here we may need a similar solution as for I8259. This is another open question for PIIX4 to become a drop-in replacement for PIIX3. Any ideas?

Best regards,
Bernhard
Bernhard Beschow Jan. 12, 2023, 6:24 p.m. UTC | #8
Am 12. Januar 2023 13:32:02 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 12/1/23 13:50, Philippe Mathieu-Daudé wrote:
>> On 9/1/23 18:23, Bernhard Beschow wrote:
>>> Now that the PIIX3 and PIIX4 device models are sufficiently consolidated,
>>> their implementations can be merged into one file for further
>>> consolidation.
>>> 
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>> Message-Id: <20221022150508.26830-37-shentey@gmail.com>
>>> ---
>>>   hw/isa/{piix3.c => piix.c} | 158 ++++++++++++++++++++
>>>   hw/isa/piix4.c             | 285 -------------------------------------
>>>   MAINTAINERS                |   6 +-
>>>   hw/i386/Kconfig            |   2 +-
>>>   hw/isa/Kconfig             |  12 +-
>>>   hw/isa/meson.build         |   3 +-
>>>   hw/mips/Kconfig            |   2 +-
>>>   7 files changed, 165 insertions(+), 303 deletions(-)
>>>   rename hw/isa/{piix3.c => piix.c} (75%)
>>>   delete mode 100644 hw/isa/piix4.c
>> 
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
>BTW I wonder why PIIX4 isn't calling pci_bus_set_route_irq_fn()...
>Any clue?

Looks like it gets used for proxying: https://elixir.bootlin.com/qemu/v7.2.0/C/ident/pci_device_route_intx_to_irq
diff mbox series

Patch

diff --git a/hw/isa/piix3.c b/hw/isa/piix.c
similarity index 75%
rename from hw/isa/piix3.c
rename to hw/isa/piix.c
index 4ce1653406..81cb4e64ab 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix.c
@@ -2,6 +2,7 @@ 
  * QEMU PIIX PCI ISA Bridge Emulation
  *
  * Copyright (c) 2006 Fabrice Bellard
+ * Copyright (c) 2018 Hervé Poussineau
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to deal
@@ -27,6 +28,7 @@ 
 #include "qapi/error.h"
 #include "hw/dma/i8257.h"
 #include "hw/southbridge/piix.h"
+#include "hw/timer/i8254.h"
 #include "hw/irq.h"
 #include "hw/qdev-properties.h"
 #include "hw/ide/piix.h"
@@ -81,6 +83,27 @@  static void piix3_set_irq(void *opaque, int pirq, int level)
     piix3_set_irq_level(piix3, pirq, level);
 }
 
+static void piix4_set_irq(void *opaque, int irq_num, int level)
+{
+    int i, pic_irq, pic_level;
+    PIIXState *s = opaque;
+    PCIBus *bus = pci_get_bus(&s->dev);
+
+    /* now we change the pic irq level according to the piix irq mappings */
+    /* XXX: optimize */
+    pic_irq = s->dev.config[PIIX_PIRQCA + irq_num];
+    if (pic_irq < ISA_NUM_IRQS) {
+        /* The pic level is the logical OR of all the PCI irqs mapped to it. */
+        pic_level = 0;
+        for (i = 0; i < PIIX_NUM_PIRQS; i++) {
+            if (pic_irq == s->dev.config[PIIX_PIRQCA + i]) {
+                pic_level |= pci_bus_get_irq_level(bus, i);
+            }
+        }
+        qemu_set_irq(s->pic.in_irqs[pic_irq], pic_level);
+    }
+}
+
 static PCIINTxRoute piix3_route_intx_pin_to_irq(void *opaque, int pin)
 {
     PIIXState *piix3 = opaque;
@@ -208,6 +231,17 @@  static int piix3_post_load(void *opaque, int version_id)
     return 0;
 }
 
+static int piix4_post_load(void *opaque, int version_id)
+{
+    PIIXState *s = opaque;
+
+    if (version_id == 2) {
+        s->rcr = 0;
+    }
+
+    return 0;
+}
+
 static int piix3_pre_save(void *opaque)
 {
     int i;
@@ -257,6 +291,17 @@  static const VMStateDescription vmstate_piix3 = {
     }
 };
 
+static const VMStateDescription vmstate_piix4 = {
+    .name = "PIIX4",
+    .version_id = 3,
+    .minimum_version_id = 2,
+    .post_load = piix4_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_PCI_DEVICE(dev, PIIXState),
+        VMSTATE_UINT8_V(rcr, PIIXState, 3),
+        VMSTATE_END_OF_LIST()
+    }
+};
 
 static void rcr_write(void *opaque, hwaddr addr, uint64_t val, unsigned len)
 {
@@ -489,11 +534,124 @@  static const TypeInfo piix3_xen_info = {
     .class_init    = piix3_xen_class_init,
 };
 
+static void piix4_realize(PCIDevice *dev, Error **errp)
+{
+    PIIXState *s = PIIX_PCI_DEVICE(dev);
+    PCIBus *pci_bus = pci_get_bus(dev);
+    ISABus *isa_bus;
+
+    isa_bus = isa_bus_new(DEVICE(dev), pci_address_space(dev),
+                          pci_address_space_io(dev), errp);
+    if (!isa_bus) {
+        return;
+    }
+
+    memory_region_init_io(&s->rcr_mem, OBJECT(dev), &rcr_ops, s,
+                          "reset-control", 1);
+    memory_region_add_subregion_overlap(pci_address_space_io(dev),
+                                        PIIX_RCR_IOPORT, &s->rcr_mem, 1);
+
+    /* initialize i8259 pic */
+    if (!qdev_realize(DEVICE(&s->pic), NULL, errp)) {
+        return;
+    }
+
+    /* initialize ISA irqs */
+    isa_bus_irqs(isa_bus, s->pic.in_irqs);
+
+    /* initialize pit */
+    i8254_pit_init(isa_bus, 0x40, 0, NULL);
+
+    /* DMA */
+    i8257_dma_init(isa_bus, 0);
+
+    /* RTC */
+    qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
+    if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
+        return;
+    }
+    s->rtc.irq = qdev_get_gpio_in(DEVICE(&s->pic), s->rtc.isairq);
+
+    /* IDE */
+    qdev_prop_set_int32(DEVICE(&s->ide), "addr", dev->devfn + 1);
+    if (!qdev_realize(DEVICE(&s->ide), BUS(pci_bus), errp)) {
+        return;
+    }
+
+    /* USB */
+    if (s->has_usb) {
+        object_initialize_child(OBJECT(dev), "uhci", &s->uhci,
+                                TYPE_PIIX4_USB_UHCI);
+        qdev_prop_set_int32(DEVICE(&s->uhci), "addr", dev->devfn + 2);
+        if (!qdev_realize(DEVICE(&s->uhci), BUS(pci_bus), errp)) {
+            return;
+        }
+    }
+
+    /* ACPI controller */
+    if (s->has_acpi) {
+        object_initialize_child(OBJECT(s), "pm", &s->pm, TYPE_PIIX4_PM);
+        qdev_prop_set_int32(DEVICE(&s->pm), "addr", dev->devfn + 3);
+        qdev_prop_set_uint32(DEVICE(&s->pm), "smb_io_base", s->smb_io_base);
+        qdev_prop_set_bit(DEVICE(&s->pm), "smm-enabled", s->smm_enabled);
+        if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) {
+            return;
+        }
+        qdev_connect_gpio_out(DEVICE(&s->pm), 0,
+                              qdev_get_gpio_in(DEVICE(&s->pic), 9));
+    }
+
+    pci_bus_irqs(pci_bus, piix4_set_irq, s, PIIX_NUM_PIRQS);
+}
+
+static void piix4_init(Object *obj)
+{
+    PIIXState *s = PIIX_PCI_DEVICE(obj);
+
+    object_initialize_child(obj, "pic", &s->pic, TYPE_ISA_PIC);
+    object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
+    object_initialize_child(obj, "ide", &s->ide, TYPE_PIIX4_IDE);
+}
+
+static void piix4_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    k->realize = piix4_realize;
+    k->vendor_id = PCI_VENDOR_ID_INTEL;
+    k->device_id = PCI_DEVICE_ID_INTEL_82371AB_0;
+    k->class_id = PCI_CLASS_BRIDGE_ISA;
+    dc->reset = piix_reset;
+    dc->desc = "ISA bridge";
+    dc->vmsd = &vmstate_piix4;
+    /*
+     * Reason: part of PIIX4 southbridge, needs to be wired up,
+     * e.g. by mips_malta_init()
+     */
+    dc->user_creatable = false;
+    dc->hotpluggable = false;
+    device_class_set_props(dc, pci_piix_props);
+}
+
+static const TypeInfo piix4_info = {
+    .name          = TYPE_PIIX4_PCI_DEVICE,
+    .parent        = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(PIIXState),
+    .instance_init = piix4_init,
+    .class_init    = piix4_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { INTERFACE_CONVENTIONAL_PCI_DEVICE },
+        { },
+    },
+};
+
 static void piix3_register_types(void)
 {
     type_register_static(&piix_pci_type_info);
     type_register_static(&piix3_info);
     type_register_static(&piix3_xen_info);
+    type_register_static(&piix4_info);
 }
 
 type_init(piix3_register_types)
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
deleted file mode 100644
index de7cde192f..0000000000
--- a/hw/isa/piix4.c
+++ /dev/null
@@ -1,285 +0,0 @@ 
-/*
- * QEMU PIIX4 PCI Bridge Emulation
- *
- * Copyright (c) 2006 Fabrice Bellard
- * Copyright (c) 2018 Hervé Poussineau
- *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to deal
- * in the Software without restriction, including without limitation the rights
- * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
- * copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
- * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
- * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
- * THE SOFTWARE.
- */
-
-#include "qemu/osdep.h"
-#include "qapi/error.h"
-#include "hw/irq.h"
-#include "hw/southbridge/piix.h"
-#include "hw/pci/pci.h"
-#include "hw/ide/piix.h"
-#include "hw/isa/isa.h"
-#include "hw/intc/i8259.h"
-#include "hw/dma/i8257.h"
-#include "hw/timer/i8254.h"
-#include "hw/rtc/mc146818rtc.h"
-#include "hw/ide/pci.h"
-#include "hw/acpi/piix4.h"
-#include "hw/usb/hcd-uhci.h"
-#include "migration/vmstate.h"
-#include "sysemu/reset.h"
-#include "sysemu/runstate.h"
-#include "qom/object.h"
-
-static void piix4_set_irq(void *opaque, int irq_num, int level)
-{
-    int i, pic_irq, pic_level;
-    PIIXState *s = opaque;
-    PCIBus *bus = pci_get_bus(&s->dev);
-
-    /* now we change the pic irq level according to the piix irq mappings */
-    /* XXX: optimize */
-    pic_irq = s->dev.config[PIIX_PIRQCA + irq_num];
-    if (pic_irq < ISA_NUM_IRQS) {
-        /* The pic level is the logical OR of all the PCI irqs mapped to it. */
-        pic_level = 0;
-        for (i = 0; i < PIIX_NUM_PIRQS; i++) {
-            if (pic_irq == s->dev.config[PIIX_PIRQCA + i]) {
-                pic_level |= pci_bus_get_irq_level(bus, i);
-            }
-        }
-        qemu_set_irq(s->pic.in_irqs[pic_irq], pic_level);
-    }
-}
-
-static void piix4_isa_reset(DeviceState *dev)
-{
-    PIIXState *d = PIIX_PCI_DEVICE(dev);
-    uint8_t *pci_conf = d->dev.config;
-
-    pci_conf[0x04] = 0x07; // master, memory and I/O
-    pci_conf[0x05] = 0x00;
-    pci_conf[0x06] = 0x00;
-    pci_conf[0x07] = 0x02; // PCI_status_devsel_medium
-    pci_conf[0x4c] = 0x4d;
-    pci_conf[0x4e] = 0x03;
-    pci_conf[0x4f] = 0x00;
-    pci_conf[0x60] = 0x80;
-    pci_conf[0x61] = 0x80;
-    pci_conf[0x62] = 0x80;
-    pci_conf[0x63] = 0x80;
-    pci_conf[0x69] = 0x02;
-    pci_conf[0x70] = 0x80;
-    pci_conf[0x76] = 0x0c;
-    pci_conf[0x77] = 0x0c;
-    pci_conf[0x78] = 0x02;
-    pci_conf[0x79] = 0x00;
-    pci_conf[0x80] = 0x00;
-    pci_conf[0x82] = 0x00;
-    pci_conf[0xa0] = 0x08;
-    pci_conf[0xa2] = 0x00;
-    pci_conf[0xa3] = 0x00;
-    pci_conf[0xa4] = 0x00;
-    pci_conf[0xa5] = 0x00;
-    pci_conf[0xa6] = 0x00;
-    pci_conf[0xa7] = 0x00;
-    pci_conf[0xa8] = 0x0f;
-    pci_conf[0xaa] = 0x00;
-    pci_conf[0xab] = 0x00;
-    pci_conf[0xac] = 0x00;
-    pci_conf[0xae] = 0x00;
-
-    d->pic_levels = 0; /* not used in PIIX4 */
-    d->rcr = 0;
-}
-
-static int piix4_post_load(void *opaque, int version_id)
-{
-    PIIXState *s = opaque;
-
-    if (version_id == 2) {
-        s->rcr = 0;
-    }
-
-    return 0;
-}
-
-static const VMStateDescription vmstate_piix4 = {
-    .name = "PIIX4",
-    .version_id = 3,
-    .minimum_version_id = 2,
-    .post_load = piix4_post_load,
-    .fields = (VMStateField[]) {
-        VMSTATE_PCI_DEVICE(dev, PIIXState),
-        VMSTATE_UINT8_V(rcr, PIIXState, 3),
-        VMSTATE_END_OF_LIST()
-    }
-};
-
-static void rcr_write(void *opaque, hwaddr addr, uint64_t val,
-                            unsigned int len)
-{
-    PIIXState *s = opaque;
-
-    if (val & 4) {
-        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
-        return;
-    }
-
-    s->rcr = val & 2; /* keep System Reset type only */
-}
-
-static uint64_t rcr_read(void *opaque, hwaddr addr, unsigned int len)
-{
-    PIIXState *s = opaque;
-
-    return s->rcr;
-}
-
-static const MemoryRegionOps rcr_ops = {
-    .read = rcr_read,
-    .write = rcr_write,
-    .endianness = DEVICE_LITTLE_ENDIAN,
-    .impl = {
-        .min_access_size = 1,
-        .max_access_size = 1,
-    },
-};
-
-static void piix4_realize(PCIDevice *dev, Error **errp)
-{
-    PIIXState *s = PIIX_PCI_DEVICE(dev);
-    PCIBus *pci_bus = pci_get_bus(dev);
-    ISABus *isa_bus;
-
-    isa_bus = isa_bus_new(DEVICE(dev), pci_address_space(dev),
-                          pci_address_space_io(dev), errp);
-    if (!isa_bus) {
-        return;
-    }
-
-    memory_region_init_io(&s->rcr_mem, OBJECT(dev), &rcr_ops, s,
-                          "reset-control", 1);
-    memory_region_add_subregion_overlap(pci_address_space_io(dev),
-                                        PIIX_RCR_IOPORT, &s->rcr_mem, 1);
-
-    /* initialize i8259 pic */
-    if (!qdev_realize(DEVICE(&s->pic), NULL, errp)) {
-        return;
-    }
-
-    /* initialize ISA irqs */
-    isa_bus_irqs(isa_bus, s->pic.in_irqs);
-
-    /* initialize pit */
-    i8254_pit_init(isa_bus, 0x40, 0, NULL);
-
-    /* DMA */
-    i8257_dma_init(isa_bus, 0);
-
-    /* RTC */
-    qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
-    if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
-        return;
-    }
-    s->rtc.irq = qdev_get_gpio_in(DEVICE(&s->pic), s->rtc.isairq);
-
-    /* IDE */
-    qdev_prop_set_int32(DEVICE(&s->ide), "addr", dev->devfn + 1);
-    if (!qdev_realize(DEVICE(&s->ide), BUS(pci_bus), errp)) {
-        return;
-    }
-
-    /* USB */
-    if (s->has_usb) {
-        object_initialize_child(OBJECT(dev), "uhci", &s->uhci,
-                                TYPE_PIIX4_USB_UHCI);
-        qdev_prop_set_int32(DEVICE(&s->uhci), "addr", dev->devfn + 2);
-        if (!qdev_realize(DEVICE(&s->uhci), BUS(pci_bus), errp)) {
-            return;
-        }
-    }
-
-    /* ACPI controller */
-    if (s->has_acpi) {
-        object_initialize_child(OBJECT(s), "pm", &s->pm, TYPE_PIIX4_PM);
-        qdev_prop_set_int32(DEVICE(&s->pm), "addr", dev->devfn + 3);
-        qdev_prop_set_uint32(DEVICE(&s->pm), "smb_io_base", s->smb_io_base);
-        qdev_prop_set_bit(DEVICE(&s->pm), "smm-enabled", s->smm_enabled);
-        if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) {
-            return;
-        }
-        qdev_connect_gpio_out(DEVICE(&s->pm), 0,
-                              qdev_get_gpio_in(DEVICE(&s->pic), 9));
-    }
-
-    pci_bus_irqs(pci_bus, piix4_set_irq, s, PIIX_NUM_PIRQS);
-}
-
-static void piix4_init(Object *obj)
-{
-    PIIXState *s = PIIX_PCI_DEVICE(obj);
-
-    object_initialize_child(obj, "pic", &s->pic, TYPE_ISA_PIC);
-    object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
-    object_initialize_child(obj, "ide", &s->ide, TYPE_PIIX4_IDE);
-}
-
-static Property piix4_props[] = {
-    DEFINE_PROP_UINT32("smb_io_base", PIIXState, smb_io_base, 0),
-    DEFINE_PROP_BOOL("has-acpi", PIIXState, has_acpi, true),
-    DEFINE_PROP_BOOL("has-usb", PIIXState, has_usb, true),
-    DEFINE_PROP_BOOL("smm-enabled", PIIXState, smm_enabled, false),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
-static void piix4_class_init(ObjectClass *klass, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(klass);
-    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
-
-    k->realize = piix4_realize;
-    k->vendor_id = PCI_VENDOR_ID_INTEL;
-    k->device_id = PCI_DEVICE_ID_INTEL_82371AB_0;
-    k->class_id = PCI_CLASS_BRIDGE_ISA;
-    dc->reset = piix4_isa_reset;
-    dc->desc = "ISA bridge";
-    dc->vmsd = &vmstate_piix4;
-    /*
-     * Reason: part of PIIX4 southbridge, needs to be wired up,
-     * e.g. by mips_malta_init()
-     */
-    dc->user_creatable = false;
-    dc->hotpluggable = false;
-    device_class_set_props(dc, piix4_props);
-}
-
-static const TypeInfo piix4_info = {
-    .name          = TYPE_PIIX4_PCI_DEVICE,
-    .parent        = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(PIIXState),
-    .instance_init = piix4_init,
-    .class_init    = piix4_class_init,
-    .interfaces = (InterfaceInfo[]) {
-        { INTERFACE_CONVENTIONAL_PCI_DEVICE },
-        { },
-    },
-};
-
-static void piix4_register_types(void)
-{
-    type_register_static(&piix4_info);
-}
-
-type_init(piix4_register_types)
diff --git a/MAINTAINERS b/MAINTAINERS
index 7a40d4d865..569042c3a1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1236,7 +1236,7 @@  Malta
 M: Philippe Mathieu-Daudé <philmd@linaro.org>
 R: Aurelien Jarno <aurelien@aurel32.net>
 S: Odd Fixes
-F: hw/isa/piix4.c
+F: hw/isa/piix.c
 F: hw/acpi/piix4.c
 F: hw/mips/malta.c
 F: hw/mips/gt64xxx_pci.c
@@ -1654,7 +1654,7 @@  F: hw/pci-host/pam.c
 F: include/hw/pci-host/i440fx.h
 F: include/hw/pci-host/q35.h
 F: include/hw/pci-host/pam.h
-F: hw/isa/piix3.c
+F: hw/isa/piix.c
 F: hw/isa/lpc_ich9.c
 F: hw/i2c/smbus_ich9.c
 F: hw/acpi/piix4.c
@@ -2344,7 +2344,7 @@  PIIX4 South Bridge (i82371AB)
 M: Hervé Poussineau <hpoussin@reactos.org>
 M: Philippe Mathieu-Daudé <philmd@linaro.org>
 S: Maintained
-F: hw/isa/piix4.c
+F: hw/isa/piix.c
 F: include/hw/southbridge/piix.h
 
 Firmware configuration (fw_cfg)
diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
index 7e53dc8f82..5e04e07758 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -73,7 +73,7 @@  config I440FX
     select PC_ACPI
     select I8259
     select PCI_I440FX
-    select PIIX3
+    select PIIX
     select DIMM
     select SMBIOS
     select FW_CFG_DMA
diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
index b911306909..040a18c070 100644
--- a/hw/isa/Kconfig
+++ b/hw/isa/Kconfig
@@ -31,17 +31,7 @@  config PC87312
     select FDC_ISA
     select IDE_ISA
 
-config PIIX3
-    bool
-    select ACPI_PIIX4
-    select I8257
-    select I8259
-    select IDE_PIIX
-    select ISA_BUS
-    select MC146818RTC
-    select USB_UHCI
-
-config PIIX4
+config PIIX
     bool
     # For historical reasons, SuperIO devices are created in the board
     # for PIIX4.
diff --git a/hw/isa/meson.build b/hw/isa/meson.build
index 8bf678ca0a..314bbd0860 100644
--- a/hw/isa/meson.build
+++ b/hw/isa/meson.build
@@ -3,8 +3,7 @@  softmmu_ss.add(when: 'CONFIG_I82378', if_true: files('i82378.c'))
 softmmu_ss.add(when: 'CONFIG_ISA_BUS', if_true: files('isa-bus.c'))
 softmmu_ss.add(when: 'CONFIG_ISA_SUPERIO', if_true: files('isa-superio.c'))
 softmmu_ss.add(when: 'CONFIG_PC87312', if_true: files('pc87312.c'))
-softmmu_ss.add(when: 'CONFIG_PIIX3', if_true: files('piix3.c'))
-softmmu_ss.add(when: 'CONFIG_PIIX4', if_true: files('piix4.c'))
+softmmu_ss.add(when: 'CONFIG_PIIX', if_true: files('piix.c'))
 softmmu_ss.add(when: 'CONFIG_SMC37C669', if_true: files('smc37c669-superio.c'))
 softmmu_ss.add(when: 'CONFIG_VT82C686', if_true: files('vt82c686.c'))
 
diff --git a/hw/mips/Kconfig b/hw/mips/Kconfig
index d156de812c..5b16ff4ed2 100644
--- a/hw/mips/Kconfig
+++ b/hw/mips/Kconfig
@@ -2,7 +2,7 @@  config MALTA
     bool
     select I8259
     select ISA_SUPERIO
-    select PIIX4
+    select PIIX
 
 config MIPSSIM
     bool