[PATCH 0/6] hw/southbridge: QOM'ify vt82c686 as VT82C686B_SOUTHBRIDGE

[PATCH 0/6] hw/southbridge: QOM'ify vt82c686 as VT82C686B_SOUTHBRIDGE

[Philippe Mathieu-Daudé]

The motivation behind this series is to remove the
isa_get_irq(NULL) call to simplify the ISA generic model.

Philippe Mathieu-Daudé (6):
  hw/isa/vt82c686: Name output IRQ as 'intr'
  hw/isa/vt82c686: Simplify removing unuseful qemu_allocate_irqs() call
  hw/isa/vt82c686: Let ISA function expose ISA IRQs
  hw/ide/via: Replace magic 2 value by ARRAY_SIZE / MAX_IDE_DEVS
  hw/ide/via: Connect IDE function output IRQs to the ISA function input
  hw/southbridge/vt82c686: Introduce VT82C686B_SOUTHBRIDGE

 hw/ide/via.c               |  31 ++++++++---
 hw/isa/vt82c686.c          |  23 ++++----
 hw/mips/fuloong2e.c        |  35 +++---------
 hw/southbridge/vt82c686.c  | 107 +++++++++++++++++++++++++++++++++++++
 MAINTAINERS                |   1 +
 hw/Kconfig                 |   1 +
 hw/isa/Kconfig             |   8 ---
 hw/meson.build             |   1 +
 hw/southbridge/Kconfig     |   7 +++
 hw/southbridge/meson.build |   1 +
 10 files changed, 162 insertions(+), 53 deletions(-)
 create mode 100644 hw/southbridge/vt82c686.c
 create mode 100644 hw/southbridge/Kconfig
 create mode 100644 hw/southbridge/meson.build

-- 
2.26.2

[PATCH 1/6] hw/isa/vt82c686: Name output IRQ as 'intr'

[Philippe Mathieu-Daudé]

Named IRQs are easier to understand in the monitor.
Name the single output interrupt as 'intr'.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/isa/vt82c686.c   | 2 +-
 hw/mips/fuloong2e.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 05d084f6982..87473ec121f 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -387,7 +387,7 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
     qemu_irq *isa_irq;
     int i;
 
-    qdev_init_gpio_out(dev, &s->cpu_intr, 1);
+    qdev_init_gpio_out_named(dev, &s->cpu_intr, "intr", 1);
     isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
     isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d),
                           &error_fatal);
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 4f61f2c873b..931385c760f 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -206,7 +206,7 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
 
     dev = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(slot, 0), true,
                                           TYPE_VT82C686B_ISA);
-    qdev_connect_gpio_out(DEVICE(dev), 0, intc);
+    qdev_connect_gpio_out_named(DEVICE(dev), "intr", 0, intc);
 
     dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide");
     pci_ide_create_devs(dev);
-- 
2.26.2

Re: [PATCH 1/6] hw/isa/vt82c686: Name output IRQ as 'intr'

[Richard Henderson]

On 3/24/21 11:54 AM, Philippe Mathieu-Daudé wrote:
> Named IRQs are easier to understand in the monitor.
> Name the single output interrupt as 'intr'.
> 
> Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org>
> ---
>   hw/isa/vt82c686.c   | 2 +-
>   hw/mips/fuloong2e.c | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 2/6] hw/isa/vt82c686: Simplify removing unuseful qemu_allocate_irqs() call

[Philippe Mathieu-Daudé]

Instead of creating an input IRQ with qemu_allocate_irqs()
to pass it as output IRQ of the PIC, with its handler simply
dispatching into the "intr" output IRQ, simplify by directly
connecting the PIC to the "intr" named output.

Fixes: 3dc31cb8490 ("vt82c686: Move creation of ISA devices to the ISA bridge")
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/isa/vt82c686.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 87473ec121f..3dc3454858e 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -323,12 +323,6 @@ struct VT82C686BISAState {
     SuperIOConfig superio_cfg;
 };
 
-static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
-{
-    VT82C686BISAState *s = opaque;
-    qemu_set_irq(s->cpu_intr, level);
-}
-
 static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
                                    uint32_t val, int len)
 {
@@ -384,14 +378,12 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
     VT82C686BISAState *s = VT82C686B_ISA(d);
     DeviceState *dev = DEVICE(d);
     ISABus *isa_bus;
-    qemu_irq *isa_irq;
     int i;
 
     qdev_init_gpio_out_named(dev, &s->cpu_intr, "intr", 1);
-    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
     isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d),
                           &error_fatal);
-    isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq));
+    isa_bus_irqs(isa_bus, i8259_init(isa_bus, s->cpu_intr));
     i8254_pit_init(isa_bus, 0x40, 0, NULL);
     i8257_dma_init(isa_bus, 0);
     isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
-- 
2.26.2

Re: [PATCH 2/6] hw/isa/vt82c686: Simplify removing unuseful qemu_allocate_irqs() call

[Richard Henderson]

On 3/24/21 11:54 AM, Philippe Mathieu-Daudé wrote:
> Instead of creating an input IRQ with qemu_allocate_irqs()
> to pass it as output IRQ of the PIC, with its handler simply
> dispatching into the "intr" output IRQ, simplify by directly
> connecting the PIC to the "intr" named output.
> 
> Fixes: 3dc31cb8490 ("vt82c686: Move creation of ISA devices to the ISA bridge")
> Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org>
> ---
>   hw/isa/vt82c686.c | 10 +---------
>   1 file changed, 1 insertion(+), 9 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 3/6] hw/isa/vt82c686: Let ISA function expose ISA IRQs

[Philippe Mathieu-Daudé]

The 2 cascaded 8259 PIC are managed by the PCI function #0
(ISA bridge). Expose the 16 IRQs on this function, so other
functions from the same chipset can access them.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/isa/vt82c686.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 3dc3454858e..4359c87e2ee 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -320,6 +320,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BISAState, VT82C686B_ISA)
 struct VT82C686BISAState {
     PCIDevice dev;
     qemu_irq cpu_intr;
+    qemu_irq *pic_irq;
     SuperIOConfig superio_cfg;
 };
 
@@ -373,6 +374,13 @@ static void vt82c686b_isa_reset(DeviceState *dev)
     s->superio_cfg.regs[0xe8] = 0xbe; /* Serial port 2 base addr */
 }
 
+static void vt82c686b_isa_irq(void *opaque, int irq, int level)
+{
+    VT82C686BISAState *s = opaque;
+
+    qemu_set_irq(s->pic_irq[irq], level);
+}
+
 static void vt82c686b_realize(PCIDevice *d, Error **errp)
 {
     VT82C686BISAState *s = VT82C686B_ISA(d);
@@ -383,7 +391,10 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
     qdev_init_gpio_out_named(dev, &s->cpu_intr, "intr", 1);
     isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d),
                           &error_fatal);
-    isa_bus_irqs(isa_bus, i8259_init(isa_bus, s->cpu_intr));
+    s->pic_irq = i8259_init(isa_bus, s->cpu_intr);
+    isa_bus_irqs(isa_bus, s->pic_irq);
+    qdev_init_gpio_in_named(dev, vt82c686b_isa_irq, "isa-irq", ISA_NUM_IRQS);
+
     i8254_pit_init(isa_bus, 0x40, 0, NULL);
     i8257_dma_init(isa_bus, 0);
     isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
-- 
2.26.2

Re: [PATCH 3/6] hw/isa/vt82c686: Let ISA function expose ISA IRQs

[Richard Henderson]

On 3/24/21 11:54 AM, Philippe Mathieu-Daudé wrote:
> The 2 cascaded 8259 PIC are managed by the PCI function #0
> (ISA bridge). Expose the 16 IRQs on this function, so other
> functions from the same chipset can access them.
> 
> Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org>
> ---
>   hw/isa/vt82c686.c | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 4/6] hw/ide/via: Replace magic 2 value by ARRAY_SIZE / MAX_IDE_DEVS

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/ide/via.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index be09912b334..6c667a92130 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -90,7 +90,7 @@ static void bmdma_setup_bar(PCIIDEState *d)
     int i;
 
     memory_region_init(&d->bmdma_bar, OBJECT(d), "via-bmdma-container", 16);
-    for(i = 0;i < 2; i++) {
+    for (i = 0; i < ARRAY_SIZE(d->bmdma); i++) {
         BMDMAState *bm = &d->bmdma[i];
 
         memory_region_init_io(&bm->extra_io, OBJECT(d), &via_bmdma_ops, bm,
@@ -122,7 +122,7 @@ static void via_ide_reset(DeviceState *dev)
     uint8_t *pci_conf = pd->config;
     int i;
 
-    for (i = 0; i < 2; i++) {
+    for (i = 0; i < ARRAY_SIZE(d->bus); i++) {
         ide_bus_reset(&d->bus[i]);
     }
 
@@ -188,9 +188,9 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
     bmdma_setup_bar(d);
     pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
 
-    qdev_init_gpio_in(ds, via_ide_set_irq, 2);
-    for (i = 0; i < 2; i++) {
-        ide_bus_new(&d->bus[i], sizeof(d->bus[i]), ds, i, 2);
+    qdev_init_gpio_in(ds, via_ide_set_irq, ARRAY_SIZE(d->bus));
+    for (i = 0; i < ARRAY_SIZE(d->bus); i++) {
+        ide_bus_new(&d->bus[i], sizeof(d->bus[i]), ds, i, MAX_IDE_DEVS);
         ide_init2(&d->bus[i], qdev_get_gpio_in(ds, i));
 
         bmdma_init(&d->bus[i], &d->bmdma[i], d);
@@ -204,7 +204,7 @@ static void via_ide_exitfn(PCIDevice *dev)
     PCIIDEState *d = PCI_IDE(dev);
     unsigned i;
 
-    for (i = 0; i < 2; ++i) {
+    for (i = 0; i < ARRAY_SIZE(d->bmdma); ++i) {
         memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].extra_io);
         memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].addr_ioport);
     }
-- 
2.26.2

Re: [PATCH 4/6] hw/ide/via: Replace magic 2 value by ARRAY_SIZE / MAX_IDE_DEVS

[Richard Henderson]

On 3/24/21 11:54 AM, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org>
> ---
>   hw/ide/via.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 4/6] hw/ide/via: Replace magic 2 value by ARRAY_SIZE / MAX_IDE_DEVS

[John Snow]

On 3/24/21 1:54 PM, Philippe Mathieu-Daudé wrote:
> +    for (i = 0; i < ARRAY_SIZE(d->bus); i++) {
> +        ide_bus_new(&d->bus[i], sizeof(d->bus[i]), ds, i, MAX_IDE_DEVS);

I bet nothing good happens if this value is ever not 2, but I bet that's 
no worse than the current reality. :)

ACK

[PATCH 5/6] hw/ide/via: Connect IDE function output IRQs to the ISA function input

[Philippe Mathieu-Daudé]

To avoid abusing isa_get_irq(NULL) using a hidden ISA bridge
under the hood, let the IDE function expose 2 output IRQs,
and connect them to the ISA function inputs when creating
the south bridge chipset model in vt82c686b_southbridge_init.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/ide/via.c        | 19 +++++++++++++++++--
 hw/mips/fuloong2e.c |  9 ++++++++-
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 6c667a92130..7887bf181e6 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -33,6 +33,17 @@
 #include "hw/ide/pci.h"
 #include "trace.h"
 
+#define TYPE_VIA_IDE "via-ide"
+OBJECT_DECLARE_SIMPLE_TYPE(VIAIDEState, VIA_IDE)
+
+struct VIAIDEState {
+    /* <private> */
+    PCIIDEState parent_obj;
+    /* <public> */
+
+    qemu_irq irq[2];
+};
+
 static uint64_t bmdma_read(void *opaque, hwaddr addr,
                            unsigned size)
 {
@@ -105,6 +116,7 @@ static void bmdma_setup_bar(PCIIDEState *d)
 static void via_ide_set_irq(void *opaque, int n, int level)
 {
     PCIDevice *d = PCI_DEVICE(opaque);
+    VIAIDEState *s = VIA_IDE(d);
 
     if (level) {
         d->config[0x70 + n * 8] |= 0x80;
@@ -112,7 +124,7 @@ static void via_ide_set_irq(void *opaque, int n, int level)
         d->config[0x70 + n * 8] &= ~0x80;
     }
 
-    qemu_set_irq(isa_get_irq(NULL, 14 + n), level);
+    qemu_set_irq(s->irq[n], level);
 }
 
 static void via_ide_reset(DeviceState *dev)
@@ -159,6 +171,7 @@ static void via_ide_reset(DeviceState *dev)
 
 static void via_ide_realize(PCIDevice *dev, Error **errp)
 {
+    VIAIDEState *s = VIA_IDE(dev);
     PCIIDEState *d = PCI_IDE(dev);
     DeviceState *ds = DEVICE(dev);
     uint8_t *pci_conf = dev->config;
@@ -188,6 +201,7 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
     bmdma_setup_bar(d);
     pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
 
+    qdev_init_gpio_out_named(ds, s->irq, "ide-irq", ARRAY_SIZE(s->irq));
     qdev_init_gpio_in(ds, via_ide_set_irq, ARRAY_SIZE(d->bus));
     for (i = 0; i < ARRAY_SIZE(d->bus); i++) {
         ide_bus_new(&d->bus[i], sizeof(d->bus[i]), ds, i, MAX_IDE_DEVS);
@@ -227,8 +241,9 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo via_ide_info = {
-    .name          = "via-ide",
+    .name          = TYPE_VIA_IDE,
     .parent        = TYPE_PCI_IDE,
+    .instance_size = sizeof(VIAIDEState),
     .class_init    = via_ide_class_init,
 };
 
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 931385c760f..f1c5db13b78 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -203,12 +203,19 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
                                        I2CBus **i2c_bus)
 {
     PCIDevice *dev;
+    DeviceState *isa;
 
     dev = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(slot, 0), true,
                                           TYPE_VT82C686B_ISA);
-    qdev_connect_gpio_out_named(DEVICE(dev), "intr", 0, intc);
+    isa = DEVICE(dev);
+    qdev_connect_gpio_out_named(isa, "intr", 0, intc);
 
     dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide");
+    for (unsigned i = 0; i < 2; i++) {
+        qdev_connect_gpio_out_named(DEVICE(dev), "ide-irq", i,
+                                    qdev_get_gpio_in_named(isa,
+                                                           "isa-irq", 14 + i));
+    }
     pci_ide_create_devs(dev);
 
     pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");
-- 
2.26.2

Re: [PATCH 5/6] hw/ide/via: Connect IDE function output IRQs to the ISA function input

[Richard Henderson]

On 3/24/21 11:54 AM, Philippe Mathieu-Daudé wrote:
> To avoid abusing isa_get_irq(NULL) using a hidden ISA bridge
> under the hood, let the IDE function expose 2 output IRQs,
> and connect them to the ISA function inputs when creating
> the south bridge chipset model in vt82c686b_southbridge_init.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/ide/via.c        | 19 +++++++++++++++++--
>   hw/mips/fuloong2e.c |  9 ++++++++-
>   2 files changed, 25 insertions(+), 3 deletions(-)
> 


> @@ -112,7 +124,7 @@ static void via_ide_set_irq(void *opaque, int n, int level)
>           d->config[0x70 + n * 8] &= ~0x80;
>       }
>   
> -    qemu_set_irq(isa_get_irq(NULL, 14 + n), level);
> +    qemu_set_irq(s->irq[n], level);
>   }
>   
>   static void via_ide_reset(DeviceState *dev)
...
> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
> index 931385c760f..f1c5db13b78 100644
> --- a/hw/mips/fuloong2e.c
> +++ b/hw/mips/fuloong2e.c
> @@ -203,12 +203,19 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
>                                          I2CBus **i2c_bus)
>   {
>       PCIDevice *dev;
> +    DeviceState *isa;
>   
>       dev = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(slot, 0), true,
>                                             TYPE_VT82C686B_ISA);
> -    qdev_connect_gpio_out_named(DEVICE(dev), "intr", 0, intc);
> +    isa = DEVICE(dev);
> +    qdev_connect_gpio_out_named(isa, "intr", 0, intc);
>   
>       dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide");
> +    for (unsigned i = 0; i < 2; i++) {
> +        qdev_connect_gpio_out_named(DEVICE(dev), "ide-irq", i,

                                        ^^^^^^^^^^^ isa?

> +                                    qdev_get_gpio_in_named(isa,
> +                                                           "isa-irq", 14 + i));
> +    }

It all looks a little funny, but I think I follow it, and see that it can't be 
split further, because of the movement of the +14.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 5/6] hw/ide/via: Connect IDE function output IRQs to the ISA function input

[Philippe Mathieu-Daudé]

On 3/25/21 1:29 PM, Richard Henderson wrote:
> On 3/24/21 11:54 AM, Philippe Mathieu-Daudé wrote:
>> To avoid abusing isa_get_irq(NULL) using a hidden ISA bridge
>> under the hood, let the IDE function expose 2 output IRQs,
>> and connect them to the ISA function inputs when creating
>> the south bridge chipset model in vt82c686b_southbridge_init.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   hw/ide/via.c        | 19 +++++++++++++++++--
>>   hw/mips/fuloong2e.c |  9 ++++++++-
>>   2 files changed, 25 insertions(+), 3 deletions(-)

>> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
>> index 931385c760f..f1c5db13b78 100644
>> --- a/hw/mips/fuloong2e.c
>> +++ b/hw/mips/fuloong2e.c
>> @@ -203,12 +203,19 @@ static void vt82c686b_southbridge_init(PCIBus
>> *pci_bus, int slot, qemu_irq intc,
>>                                          I2CBus **i2c_bus)
>>   {
>>       PCIDevice *dev;
>> +    DeviceState *isa;
>>         dev = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(slot,
>> 0), true,
>>                                             TYPE_VT82C686B_ISA);
>> -    qdev_connect_gpio_out_named(DEVICE(dev), "intr", 0, intc);
>> +    isa = DEVICE(dev);
>> +    qdev_connect_gpio_out_named(isa, "intr", 0, intc);
>>         dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide");
>> +    for (unsigned i = 0; i < 2; i++) {
>> +        qdev_connect_gpio_out_named(DEVICE(dev), "ide-irq", i,
> 
>                                        ^^^^^^^^^^^ isa?

OK.

>> +                                    qdev_get_gpio_in_named(isa,
>> +                                                           "isa-irq",
>> 14 + i));
>> +    }
> 
> It all looks a little funny, but I think I follow it, and see that it
> can't be split further, because of the movement of the +14.

I can break the indent to shift left. Anyway this disappear in the next
commit.

> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 

Thanks!

Phil.

Re: [PATCH 5/6] hw/ide/via: Connect IDE function output IRQs to the ISA function input

[John Snow]

On 3/24/21 1:54 PM, Philippe Mathieu-Daudé wrote:
> To avoid abusing isa_get_irq(NULL) using a hidden ISA bridge
> under the hood, let the IDE function expose 2 output IRQs,
> and connect them to the ISA function inputs when creating
> the south bridge chipset model in vt82c686b_southbridge_init.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

No real opinion, how was it tested? Can probably ack in good faith after 
review comments from Richard Henderson and BALATON Zoltan.

--js

> ---
>   hw/ide/via.c        | 19 +++++++++++++++++--
>   hw/mips/fuloong2e.c |  9 ++++++++-
>   2 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index 6c667a92130..7887bf181e6 100644
> --- a/hw/ide/via.c
> +++ b/hw/ide/via.c
> @@ -33,6 +33,17 @@
>   #include "hw/ide/pci.h"
>   #include "trace.h"
>   
> +#define TYPE_VIA_IDE "via-ide"
> +OBJECT_DECLARE_SIMPLE_TYPE(VIAIDEState, VIA_IDE)
> +
> +struct VIAIDEState {
> +    /* <private> */
> +    PCIIDEState parent_obj;
> +    /* <public> */
> +
> +    qemu_irq irq[2];
> +};
> +
>   static uint64_t bmdma_read(void *opaque, hwaddr addr,
>                              unsigned size)
>   {
> @@ -105,6 +116,7 @@ static void bmdma_setup_bar(PCIIDEState *d)
>   static void via_ide_set_irq(void *opaque, int n, int level)
>   {
>       PCIDevice *d = PCI_DEVICE(opaque);
> +    VIAIDEState *s = VIA_IDE(d);
>   
>       if (level) {
>           d->config[0x70 + n * 8] |= 0x80;
> @@ -112,7 +124,7 @@ static void via_ide_set_irq(void *opaque, int n, int level)
>           d->config[0x70 + n * 8] &= ~0x80;
>       }
>   
> -    qemu_set_irq(isa_get_irq(NULL, 14 + n), level);
> +    qemu_set_irq(s->irq[n], level);
>   }
>   
>   static void via_ide_reset(DeviceState *dev)
> @@ -159,6 +171,7 @@ static void via_ide_reset(DeviceState *dev)
>   
>   static void via_ide_realize(PCIDevice *dev, Error **errp)
>   {
> +    VIAIDEState *s = VIA_IDE(dev);
>       PCIIDEState *d = PCI_IDE(dev);
>       DeviceState *ds = DEVICE(dev);
>       uint8_t *pci_conf = dev->config;
> @@ -188,6 +201,7 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
>       bmdma_setup_bar(d);
>       pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
>   
> +    qdev_init_gpio_out_named(ds, s->irq, "ide-irq", ARRAY_SIZE(s->irq));
>       qdev_init_gpio_in(ds, via_ide_set_irq, ARRAY_SIZE(d->bus));
>       for (i = 0; i < ARRAY_SIZE(d->bus); i++) {
>           ide_bus_new(&d->bus[i], sizeof(d->bus[i]), ds, i, MAX_IDE_DEVS);
> @@ -227,8 +241,9 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
>   }
>   
>   static const TypeInfo via_ide_info = {
> -    .name          = "via-ide",
> +    .name          = TYPE_VIA_IDE,
>       .parent        = TYPE_PCI_IDE,
> +    .instance_size = sizeof(VIAIDEState),
>       .class_init    = via_ide_class_init,
>   };
>   
> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
> index 931385c760f..f1c5db13b78 100644
> --- a/hw/mips/fuloong2e.c
> +++ b/hw/mips/fuloong2e.c
> @@ -203,12 +203,19 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
>                                          I2CBus **i2c_bus)
>   {
>       PCIDevice *dev;
> +    DeviceState *isa;
>   
>       dev = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(slot, 0), true,
>                                             TYPE_VT82C686B_ISA);
> -    qdev_connect_gpio_out_named(DEVICE(dev), "intr", 0, intc);
> +    isa = DEVICE(dev);
> +    qdev_connect_gpio_out_named(isa, "intr", 0, intc);
>   
>       dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide");
> +    for (unsigned i = 0; i < 2; i++) {
> +        qdev_connect_gpio_out_named(DEVICE(dev), "ide-irq", i,
> +                                    qdev_get_gpio_in_named(isa,
> +                                                           "isa-irq", 14 + i));
> +    }
>       pci_ide_create_devs(dev);
>   
>       pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");
> 

[PATCH 6/6] hw/southbridge/vt82c686: Introduce VT82C686B_SOUTHBRIDGE

[Philippe Mathieu-Daudé]

The VT82C686B south bridge is a single chipset. Model
it as a single sysbus device.
Move the vt82c686b_southbridge_init as via_southbridge_realize,
add the QOM state: ViaSouthBridgeState. This device needs 2
properties to be realized: the PCI bus and its slot number.
2 aliases are exposed: the ISA PIC output IRQ and the I2C bus.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/mips/fuloong2e.c        |  42 +++------------
 hw/southbridge/vt82c686.c  | 107 +++++++++++++++++++++++++++++++++++++
 MAINTAINERS                |   1 +
 hw/Kconfig                 |   1 +
 hw/isa/Kconfig             |   8 ---
 hw/meson.build             |   1 +
 hw/southbridge/Kconfig     |   7 +++
 hw/southbridge/meson.build |   1 +
 8 files changed, 126 insertions(+), 42 deletions(-)
 create mode 100644 hw/southbridge/vt82c686.c
 create mode 100644 hw/southbridge/Kconfig
 create mode 100644 hw/southbridge/meson.build

diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index f1c5db13b78..01f5ef89339 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -35,10 +35,8 @@
 #include "hw/pci/pci.h"
 #include "qemu/log.h"
 #include "hw/loader.h"
-#include "hw/ide/pci.h"
 #include "hw/qdev-properties.h"
 #include "elf.h"
-#include "hw/isa/vt82c686.h"
 #include "exec/address-spaces.h"
 #include "sysemu/qtest.h"
 #include "sysemu/reset.h"
@@ -199,36 +197,6 @@ static void main_cpu_reset(void *opaque)
     }
 }
 
-static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
-                                       I2CBus **i2c_bus)
-{
-    PCIDevice *dev;
-    DeviceState *isa;
-
-    dev = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(slot, 0), true,
-                                          TYPE_VT82C686B_ISA);
-    isa = DEVICE(dev);
-    qdev_connect_gpio_out_named(isa, "intr", 0, intc);
-
-    dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide");
-    for (unsigned i = 0; i < 2; i++) {
-        qdev_connect_gpio_out_named(DEVICE(dev), "ide-irq", i,
-                                    qdev_get_gpio_in_named(isa,
-                                                           "isa-irq", 14 + i));
-    }
-    pci_ide_create_devs(dev);
-
-    pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");
-    pci_create_simple(pci_bus, PCI_DEVFN(slot, 3), "vt82c686b-usb-uhci");
-
-    dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 4), TYPE_VT82C686B_PM);
-    *i2c_bus = I2C_BUS(qdev_get_child_bus(DEVICE(dev), "i2c"));
-
-    /* Audio support */
-    pci_create_simple(pci_bus, PCI_DEVFN(slot, 5), TYPE_VIA_AC97);
-    pci_create_simple(pci_bus, PCI_DEVFN(slot, 6), TYPE_VIA_MC97);
-}
-
 /* Network support */
 static void network_init(PCIBus *pci_bus)
 {
@@ -325,8 +293,14 @@ static void mips_fuloong2e_init(MachineState *machine)
     pci_bus = bonito_init((qemu_irq *)&(env->irq[2]));
 
     /* South bridge -> IP5 */
-    vt82c686b_southbridge_init(pci_bus, FULOONG2E_VIA_SLOT, env->irq[5],
-                               &smbus);
+    dev = qdev_new("vt82c686b-southbridge");
+    object_property_set_uint(OBJECT(dev), "pci-slot",
+                             FULOONG2E_VIA_SLOT, &error_fatal);
+    object_property_set_link(OBJECT(dev), "pci-bus",
+                             OBJECT(pci_bus), &error_fatal);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    qdev_connect_gpio_out_named(dev, "intr", 0, env->irq[5]);
+    smbus = I2C_BUS(qdev_get_child_bus(dev, "i2c"));
 
     /* GPU */
     if (vga_interface_type != VGA_NONE) {
diff --git a/hw/southbridge/vt82c686.c b/hw/southbridge/vt82c686.c
new file mode 100644
index 00000000000..61c3e6ae306
--- /dev/null
+++ b/hw/southbridge/vt82c686.c
@@ -0,0 +1,107 @@
+/*
+ * VT82C686B south bridge emulation
+ *
+ * Copyright (c) 2008 yajin (yajin@vm-kernel.org)
+ * Copyright (c) 2009 chenming (chenming@rdc.faw.com.cn)
+ * Copyright (c) 2010 Huacai Chen (zltjiangshi@gmail.com)
+ * Copyright (c) 2021 Philippe Mathieu-Daudé <f4bug@amsat.org>
+ * This code is licensed under the GNU GPL v2.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/qdev-properties.h"
+#include "hw/sysbus.h"
+#include "hw/pci/pci.h"
+#include "hw/ide/pci.h"
+#include "hw/isa/vt82c686.h"
+
+#define TYPE_VT82C686B_SOUTHBRIDGE "vt82c686b-southbridge"
+OBJECT_DECLARE_SIMPLE_TYPE(ViaSouthBridgeState, VT82C686B_SOUTHBRIDGE)
+
+struct ViaSouthBridgeState {
+    /* <private> */
+    SysBusDevice parent_obj;
+    /* <public> */
+
+    uint8_t pci_slot;
+    PCIBus *pci_bus;
+    PCIDevice *isa;
+    PCIDevice *ide;
+    PCIDevice *usb[2];
+    PCIDevice *apm;
+    PCIDevice *audio;
+    PCIDevice *modem;
+};
+
+static void via_southbridge_realize(DeviceState *dev, Error **errp)
+{
+    ViaSouthBridgeState *s = VT82C686B_SOUTHBRIDGE(dev);
+
+    if (!s->pci_bus) {
+        error_setg(errp, "SMMU is not attached to any PCI bus!");
+        return;
+    }
+
+    s->isa = pci_create_simple_multifunction(s->pci_bus,
+                                             PCI_DEVFN(s->pci_slot, 0),
+                                             true, TYPE_VT82C686B_ISA);
+    qdev_pass_gpios(DEVICE(s->isa), dev, "intr");
+
+    s->ide = pci_create_simple(s->pci_bus,
+                               PCI_DEVFN(s->pci_slot, 1), "via-ide");
+    for (unsigned i = 0; i < 2; i++) {
+        qdev_connect_gpio_out_named(DEVICE(s->ide), "ide-irq", i,
+                            qdev_get_gpio_in_named(DEVICE(s->isa),
+                                                   "isa-irq", 14 + i));
+    }
+    pci_ide_create_devs(s->ide);
+
+    s->usb[0] = pci_create_simple(s->pci_bus,
+                                  PCI_DEVFN(s->pci_slot, 2),
+                                  "vt82c686b-usb-uhci");
+    s->usb[1] = pci_create_simple(s->pci_bus,
+                                  PCI_DEVFN(s->pci_slot, 3),
+                                  "vt82c686b-usb-uhci");
+
+    s->apm = pci_create_simple(s->pci_bus,
+                               PCI_DEVFN(s->pci_slot, 4),
+                               TYPE_VT82C686B_PM);
+    object_property_add_alias(OBJECT(s), "i2c",
+                              OBJECT(s->apm), "i2c");
+
+    s->audio = pci_create_simple(s->pci_bus,
+                                 PCI_DEVFN(s->pci_slot, 5),
+                                 TYPE_VIA_AC97);
+    s->modem = pci_create_simple(s->pci_bus,
+                                 PCI_DEVFN(s->pci_slot, 6),
+                                 TYPE_VIA_MC97);
+}
+
+static Property via_southbridge_properties[] = {
+    DEFINE_PROP_UINT8("pci-slot", ViaSouthBridgeState, pci_slot, 0),
+    DEFINE_PROP_LINK("pci-bus", ViaSouthBridgeState, pci_bus, "PCI", PCIBus *),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void via_southbridge_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = via_southbridge_realize;
+    device_class_set_props(dc, via_southbridge_properties);
+}
+
+static const TypeInfo via_southbridge_info = {
+    .name          = TYPE_VT82C686B_SOUTHBRIDGE,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(ViaSouthBridgeState),
+    .class_init    = via_southbridge_class_init,
+};
+
+static void via_southbridge_register_types(void)
+{
+    type_register_static(&via_southbridge_info);
+}
+
+type_init(via_southbridge_register_types)
diff --git a/MAINTAINERS b/MAINTAINERS
index 10ed6d76240..65a0ec11c2a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1195,6 +1195,7 @@ S: Odd Fixes
 F: hw/mips/fuloong2e.c
 F: hw/isa/vt82c686.c
 F: hw/pci-host/bonito.c
+F: hw/southbridge/vt82c686.c
 F: hw/usb/vt82c686-uhci-pci.c
 F: include/hw/isa/vt82c686.h
 
diff --git a/hw/Kconfig b/hw/Kconfig
index ff40bd3f7bb..76e35ad189f 100644
--- a/hw/Kconfig
+++ b/hw/Kconfig
@@ -32,6 +32,7 @@ source rtc/Kconfig
 source scsi/Kconfig
 source sd/Kconfig
 source smbios/Kconfig
+source southbridge/Kconfig
 source ssi/Kconfig
 source timer/Kconfig
 source tpm/Kconfig
diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
index 2691eae2f0c..34adc411fa6 100644
--- a/hw/isa/Kconfig
+++ b/hw/isa/Kconfig
@@ -41,14 +41,6 @@ config PIIX4
     select ISA_BUS
     select USB_UHCI
 
-config VT82C686
-    bool
-    select ISA_SUPERIO
-    select ACPI_SMBUS
-    select SERIAL_ISA
-    select FDC
-    select USB_UHCI
-
 config SMC37C669
     bool
     select ISA_SUPERIO
diff --git a/hw/meson.build b/hw/meson.build
index 8ba79b1a528..4bdd254e041 100644
--- a/hw/meson.build
+++ b/hw/meson.build
@@ -31,6 +31,7 @@
 subdir('scsi')
 subdir('sd')
 subdir('smbios')
+subdir('southbridge')
 subdir('ssi')
 subdir('timer')
 subdir('tpm')
diff --git a/hw/southbridge/Kconfig b/hw/southbridge/Kconfig
new file mode 100644
index 00000000000..356434f4e44
--- /dev/null
+++ b/hw/southbridge/Kconfig
@@ -0,0 +1,7 @@
+config VT82C686
+    bool
+    select ISA_SUPERIO
+    select ACPI_SMBUS
+    select SERIAL_ISA
+    select FDC
+    select USB_UHCI
diff --git a/hw/southbridge/meson.build b/hw/southbridge/meson.build
new file mode 100644
index 00000000000..53b02e9563c
--- /dev/null
+++ b/hw/southbridge/meson.build
@@ -0,0 +1 @@
+softmmu_ss.add(when: 'CONFIG_VT82C686', if_true: files('vt82c686.c'))
-- 
2.26.2

Re: [PATCH 0/6] hw/southbridge: QOM'ify vt82c686 as VT82C686B_SOUTHBRIDGE

[BALATON Zoltan]

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

On Wed, 24 Mar 2021, Philippe Mathieu-Daudé wrote:
> The motivation behind this series is to remove the
> isa_get_irq(NULL) call to simplify the ISA generic model.

Could you please wait with this until after my pegasos2 series is merged? 
Otherwise I'll have to rewrite that again for which I don't have much time 
and don't want to miss 6.1 too now that it's almost reviewed and accepted. 
This series is a cleanup that could be more fully done after the other 
machine using vt82c686.c is also there.

Regards,
BALATON Zoltan