[Qemu-devel] [PATCH for-4.1 0/4] hw/mips/r4k: Refactor the Super I/O chipset

[Qemu-devel] [PATCH for-4.1 0/4] hw/mips/r4k: Refactor the Super I/O chipset

[Philippe Mathieu-Daudé]

Hi,

This series refactor the superio code used in the mips_r4k machine, adding the TYPE_R4K_SUPERIO device.
Doing so we improve the ISASuperIODevice, fixing an easy bug.

This series also demonstrate that no machine access a I8042 ISA device out of a Super I/O chipset.

Regards,

Phil.

Philippe Mathieu-Daudé (4):
  hw/isa/superio: Rename a variable
  hw/isa/superio: Support more than one IDE bus
  hw/isa/superio: Support chipsets with no Floppy Disk controller
  hw/mips/r4k: Refactor the Super I/O chipset

 hw/isa/isa-superio.c     | 99 +++++++++++++++++++++-------------------
 hw/mips/mips_r4k.c       | 61 ++++++++++++++++++-------
 include/hw/isa/superio.h |  2 +-
 3 files changed, 98 insertions(+), 64 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH for-4.1 1/4] hw/isa/superio: Rename a variable

[Philippe Mathieu-Daudé]

This patch is purely cosmetic. No functional change.
This will ease the next patch where we re-indent an if() statement.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/isa/isa-superio.c | 69 ++++++++++++++++++++++----------------------
 1 file changed, 34 insertions(+), 35 deletions(-)

diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c
index d54463bf035..c6845eaf578 100644
--- a/hw/isa/isa-superio.c
+++ b/hw/isa/isa-superio.c
@@ -22,8 +22,8 @@
 
 static void isa_superio_realize(DeviceState *dev, Error **errp)
 {
-    ISASuperIODevice *sio = ISA_SUPERIO(dev);
-    ISASuperIOClass *k = ISA_SUPERIO_GET_CLASS(sio);
+    ISASuperIODevice *s = ISA_SUPERIO(dev);
+    ISASuperIOClass *k = ISA_SUPERIO_GET_CLASS(s);
     ISABus *bus = isa_bus_from_device(ISA_DEVICE(dev));
     ISADevice *isa;
     DeviceState *d;
@@ -34,12 +34,12 @@ static void isa_superio_realize(DeviceState *dev, Error **errp)
 
     /* Parallel port */
     for (i = 0; i < k->parallel.count; i++) {
-        if (i >= ARRAY_SIZE(sio->parallel)) {
+        if (i >= ARRAY_SIZE(s->parallel)) {
             warn_report("superio: ignoring %td parallel controllers",
-                        k->parallel.count - ARRAY_SIZE(sio->parallel));
+                        k->parallel.count - ARRAY_SIZE(s->parallel));
             break;
         }
-        if (!k->parallel.is_enabled || k->parallel.is_enabled(sio, i)) {
+        if (!k->parallel.is_enabled || k->parallel.is_enabled(s, i)) {
             /* FIXME use a qdev chardev prop instead of parallel_hds[] */
             chr = parallel_hds[i];
             if (chr == NULL) {
@@ -53,33 +53,33 @@ static void isa_superio_realize(DeviceState *dev, Error **errp)
             qdev_prop_set_uint32(d, "index", i);
             if (k->parallel.get_iobase) {
                 qdev_prop_set_uint32(d, "iobase",
-                                     k->parallel.get_iobase(sio, i));
+                                     k->parallel.get_iobase(s, i));
             }
             if (k->parallel.get_irq) {
-                qdev_prop_set_uint32(d, "irq", k->parallel.get_irq(sio, i));
+                qdev_prop_set_uint32(d, "irq", k->parallel.get_irq(s, i));
             }
             qdev_prop_set_chr(d, "chardev", chr);
             qdev_init_nofail(d);
-            sio->parallel[i] = isa;
+            s->parallel[i] = isa;
             trace_superio_create_parallel(i,
                                           k->parallel.get_iobase ?
-                                          k->parallel.get_iobase(sio, i) : -1,
+                                          k->parallel.get_iobase(s, i) : -1,
                                           k->parallel.get_irq ?
-                                          k->parallel.get_irq(sio, i) : -1);
+                                          k->parallel.get_irq(s, i) : -1);
             object_property_add_child(OBJECT(dev), name,
-                                      OBJECT(sio->parallel[i]), NULL);
+                                      OBJECT(s->parallel[i]), NULL);
             g_free(name);
         }
     }
 
     /* Serial */
     for (i = 0; i < k->serial.count; i++) {
-        if (i >= ARRAY_SIZE(sio->serial)) {
+        if (i >= ARRAY_SIZE(s->serial)) {
             warn_report("superio: ignoring %td serial controllers",
-                        k->serial.count - ARRAY_SIZE(sio->serial));
+                        k->serial.count - ARRAY_SIZE(s->serial));
             break;
         }
-        if (!k->serial.is_enabled || k->serial.is_enabled(sio, i)) {
+        if (!k->serial.is_enabled || k->serial.is_enabled(s, i)) {
             /* FIXME use a qdev chardev prop instead of serial_hd() */
             chr = serial_hd(i);
             if (chr == NULL) {
@@ -92,35 +92,34 @@ static void isa_superio_realize(DeviceState *dev, Error **errp)
             d = DEVICE(isa);
             qdev_prop_set_uint32(d, "index", i);
             if (k->serial.get_iobase) {
-                qdev_prop_set_uint32(d, "iobase",
-                                     k->serial.get_iobase(sio, i));
+                qdev_prop_set_uint32(d, "iobase", k->serial.get_iobase(s, i));
             }
             if (k->serial.get_irq) {
-                qdev_prop_set_uint32(d, "irq", k->serial.get_irq(sio, i));
+                qdev_prop_set_uint32(d, "irq", k->serial.get_irq(s, i));
             }
             qdev_prop_set_chr(d, "chardev", chr);
             qdev_init_nofail(d);
-            sio->serial[i] = isa;
+            s->serial[i] = isa;
             trace_superio_create_serial(i,
                                         k->serial.get_iobase ?
-                                        k->serial.get_iobase(sio, i) : -1,
+                                        k->serial.get_iobase(s, i) : -1,
                                         k->serial.get_irq ?
-                                        k->serial.get_irq(sio, i) : -1);
+                                        k->serial.get_irq(s, i) : -1);
             object_property_add_child(OBJECT(dev), name,
-                                      OBJECT(sio->serial[0]), NULL);
+                                      OBJECT(s->serial[0]), NULL);
             g_free(name);
         }
     }
 
     /* Floppy disc */
-    if (!k->floppy.is_enabled || k->floppy.is_enabled(sio, 0)) {
+    if (!k->floppy.is_enabled || k->floppy.is_enabled(s, 0)) {
         isa = isa_create(bus, "isa-fdc");
         d = DEVICE(isa);
         if (k->floppy.get_iobase) {
-            qdev_prop_set_uint32(d, "iobase", k->floppy.get_iobase(sio, 0));
+            qdev_prop_set_uint32(d, "iobase", k->floppy.get_iobase(s, 0));
         }
         if (k->floppy.get_irq) {
-            qdev_prop_set_uint32(d, "irq", k->floppy.get_irq(sio, 0));
+            qdev_prop_set_uint32(d, "irq", k->floppy.get_irq(s, 0));
         }
         /* FIXME use a qdev drive property instead of drive_get() */
         drive = drive_get(IF_FLOPPY, 0, 0);
@@ -135,37 +134,37 @@ static void isa_superio_realize(DeviceState *dev, Error **errp)
                                 &error_fatal);
         }
         qdev_init_nofail(d);
-        sio->floppy = isa;
+        s->floppy = isa;
         trace_superio_create_floppy(0,
                                     k->floppy.get_iobase ?
-                                    k->floppy.get_iobase(sio, 0) : -1,
+                                    k->floppy.get_iobase(s, 0) : -1,
                                     k->floppy.get_irq ?
-                                    k->floppy.get_irq(sio, 0) : -1);
+                                    k->floppy.get_irq(s, 0) : -1);
     }
 
     /* Keyboard, mouse */
-    sio->kbc = isa_create_simple(bus, TYPE_I8042);
+    s->kbc = isa_create_simple(bus, TYPE_I8042);
 
     /* IDE */
-    if (k->ide.count && (!k->ide.is_enabled || k->ide.is_enabled(sio, 0))) {
+    if (k->ide.count && (!k->ide.is_enabled || k->ide.is_enabled(s, 0))) {
         isa = isa_create(bus, "isa-ide");
         d = DEVICE(isa);
         if (k->ide.get_iobase) {
-            qdev_prop_set_uint32(d, "iobase", k->ide.get_iobase(sio, 0));
+            qdev_prop_set_uint32(d, "iobase", k->ide.get_iobase(s, 0));
         }
         if (k->ide.get_iobase) {
-            qdev_prop_set_uint32(d, "iobase2", k->ide.get_iobase(sio, 1));
+            qdev_prop_set_uint32(d, "iobase2", k->ide.get_iobase(s, 1));
         }
         if (k->ide.get_irq) {
-            qdev_prop_set_uint32(d, "irq", k->ide.get_irq(sio, 0));
+            qdev_prop_set_uint32(d, "irq", k->ide.get_irq(s, 0));
         }
         qdev_init_nofail(d);
-        sio->ide = isa;
+        s->ide = isa;
         trace_superio_create_ide(0,
                                  k->ide.get_iobase ?
-                                 k->ide.get_iobase(sio, 0) : -1,
+                                 k->ide.get_iobase(s, 0) : -1,
                                  k->ide.get_irq ?
-                                 k->ide.get_irq(sio, 0) : -1);
+                                 k->ide.get_irq(s, 0) : -1);
     }
 }
 
-- 
2.20.1

[Qemu-devel] [PATCH for-4.1 2/4] hw/isa/superio: Support more than one IDE bus

[Philippe Mathieu-Daudé]

The current code is limited to a single IDE bus (supporting
two IDE drives). Some Super I/O chipset provide two IDE buses
(four IDE drives).
Modify the model to support more that one IDE bus.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/isa/isa-superio.c     | 43 ++++++++++++++++++++++------------------
 include/hw/isa/superio.h |  2 +-
 2 files changed, 25 insertions(+), 20 deletions(-)

diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c
index c6845eaf578..b0761ea1f96 100644
--- a/hw/isa/isa-superio.c
+++ b/hw/isa/isa-superio.c
@@ -18,6 +18,7 @@
 #include "hw/isa/superio.h"
 #include "hw/input/i8042.h"
 #include "hw/char/serial.h"
+#include "hw/ide.h"
 #include "trace.h"
 
 static void isa_superio_realize(DeviceState *dev, Error **errp)
@@ -30,7 +31,7 @@ static void isa_superio_realize(DeviceState *dev, Error **errp)
     Chardev *chr;
     DriveInfo *drive;
     char *name;
-    int i;
+    int i, j;
 
     /* Parallel port */
     for (i = 0; i < k->parallel.count; i++) {
@@ -146,25 +147,29 @@ static void isa_superio_realize(DeviceState *dev, Error **errp)
     s->kbc = isa_create_simple(bus, TYPE_I8042);
 
     /* IDE */
-    if (k->ide.count && (!k->ide.is_enabled || k->ide.is_enabled(s, 0))) {
-        isa = isa_create(bus, "isa-ide");
-        d = DEVICE(isa);
-        if (k->ide.get_iobase) {
-            qdev_prop_set_uint32(d, "iobase", k->ide.get_iobase(s, 0));
-        }
-        if (k->ide.get_iobase) {
-            qdev_prop_set_uint32(d, "iobase2", k->ide.get_iobase(s, 1));
-        }
-        if (k->ide.get_irq) {
-            qdev_prop_set_uint32(d, "irq", k->ide.get_irq(s, 0));
+    size_t bus_count = k->ide.count / MAX_IDE_DEVS;
+    s->ide = g_new(ISADevice *, bus_count);
+    for (i = 0, j = 0; i < bus_count; i++, j += MAX_IDE_DEVS) {
+        if (!k->ide.is_enabled || k->ide.is_enabled(s, j)) {
+            isa = isa_create(bus, "isa-ide");
+            d = DEVICE(isa);
+            if (k->ide.get_iobase) {
+                qdev_prop_set_uint32(d, "iobase", k->ide.get_iobase(s, j));
+            }
+            if (k->ide.get_iobase) {
+                qdev_prop_set_uint32(d, "iobase2", k->ide.get_iobase(s, j + 1));
+            }
+            if (k->ide.get_irq) {
+                qdev_prop_set_uint32(d, "irq", k->ide.get_irq(s, j));
+            }
+            qdev_init_nofail(d);
+            s->ide[i] = isa;
+            trace_superio_create_ide(i,
+                                     k->ide.get_iobase ?
+                                     k->ide.get_iobase(s, j) : -1,
+                                     k->ide.get_irq ?
+                                     k->ide.get_irq(s, j) : -1);
         }
-        qdev_init_nofail(d);
-        s->ide = isa;
-        trace_superio_create_ide(0,
-                                 k->ide.get_iobase ?
-                                 k->ide.get_iobase(s, 0) : -1,
-                                 k->ide.get_irq ?
-                                 k->ide.get_irq(s, 0) : -1);
     }
 }
 
diff --git a/include/hw/isa/superio.h b/include/hw/isa/superio.h
index 345f0060817..980a9691696 100644
--- a/include/hw/isa/superio.h
+++ b/include/hw/isa/superio.h
@@ -33,7 +33,7 @@ typedef struct ISASuperIODevice {
     ISADevice *serial[SUPERIO_MAX_SERIAL_PORTS];
     ISADevice *floppy;
     ISADevice *kbc;
-    ISADevice *ide;
+    ISADevice **ide;
 } ISASuperIODevice;
 
 typedef struct ISASuperIOFuncs {
-- 
2.20.1

[Qemu-devel] [PATCH for-4.1 3/4] hw/isa/superio: Support chipsets with no Floppy Disk controller

[Philippe Mathieu-Daudé]

Not all Super I/O chipsets provide a Floppy Disk Controller.

Without this change, using a Super I/O with no FDC would abort QEMU with:

  Initialization of device isa-fdc failed: ISA controller does not support DMA

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/isa/isa-superio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c
index b0761ea1f96..6956f06d529 100644
--- a/hw/isa/isa-superio.c
+++ b/hw/isa/isa-superio.c
@@ -113,7 +113,8 @@ static void isa_superio_realize(DeviceState *dev, Error **errp)
     }
 
     /* Floppy disc */
-    if (!k->floppy.is_enabled || k->floppy.is_enabled(s, 0)) {
+    if (k->floppy.count
+            && (!k->floppy.is_enabled || k->floppy.is_enabled(s, 0))) {
         isa = isa_create(bus, "isa-fdc");
         d = DEVICE(isa);
         if (k->floppy.get_iobase) {
-- 
2.20.1

[Qemu-devel] [PATCH for-4.1 4/4] hw/mips/r4k: Refactor the Super I/O chipset

[Philippe Mathieu-Daudé]

ISA Super I/O are already modeled by the ISASuperIODevice abstract
device.
Since this board uses a generic ISA Super I/O chipset, refactor it
as the TYPE_R4K_SUPERIO device, child of ISASuperIODevice.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/mips/mips_r4k.c | 61 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 45 insertions(+), 16 deletions(-)

diff --git a/hw/mips/mips_r4k.c b/hw/mips/mips_r4k.c
index 93dbf76bb49..b51a9523b43 100644
--- a/hw/mips/mips_r4k.c
+++ b/hw/mips/mips_r4k.c
@@ -18,6 +18,7 @@
 #include "hw/i386/pc.h"
 #include "hw/char/serial.h"
 #include "hw/isa/isa.h"
+#include "hw/isa/superio.h"
 #include "net/net.h"
 #include "hw/net/ne2000-isa.h"
 #include "sysemu/sysemu.h"
@@ -29,7 +30,6 @@
 #include "hw/loader.h"
 #include "elf.h"
 #include "hw/timer/mc146818rtc.h"
-#include "hw/input/i8042.h"
 #include "hw/timer/i8254.h"
 #include "exec/address-spaces.h"
 #include "sysemu/qtest.h"
@@ -37,10 +37,6 @@
 
 #define MAX_IDE_BUS 2
 
-static const int ide_iobase[2] = { 0x1f0, 0x170 };
-static const int ide_iobase2[2] = { 0x3f6, 0x376 };
-static const int ide_irq[2] = { 14, 15 };
-
 static ISADevice *pit; /* PIT i8254 */
 
 /* i8254 PIT is attached to the IRQ0 at PIC i8259 */
@@ -73,6 +69,49 @@ static const MemoryRegionOps mips_qemu_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
+#define TYPE_R4K_SUPERIO "r4k-superio"
+
+static uint16_t get_ide_iobase(ISASuperIODevice *sio, uint8_t index)
+{
+    static const uint16_t ide_iobase[] = { 0x1f0, 0x3f6, 0x170, 0x376 };
+
+    return ide_iobase[index];
+}
+
+static unsigned int get_ide_irq(ISASuperIODevice *sio, uint8_t index)
+{
+    return index < MAX_IDE_DEVS ? 14 : 15;
+}
+
+static void r4k_superio_class_init(ObjectClass *klass, void *data)
+{
+    ISASuperIOClass *sc = ISA_SUPERIO_CLASS(klass);
+
+    sc->serial.count = MAX_ISA_SERIAL_PORTS;
+    sc->parallel.count = 0;
+    sc->floppy.count = 0;
+    sc->ide = (ISASuperIOFuncs){
+        .count = MAX_IDE_BUS * MAX_IDE_DEVS,
+        .get_iobase = get_ide_iobase,
+        .get_irq    = get_ide_irq,
+    };
+}
+
+static const TypeInfo r4k_superio_type_info = {
+    .name          = TYPE_R4K_SUPERIO,
+    .parent        = TYPE_ISA_SUPERIO,
+    .instance_size = sizeof(ISASuperIODevice),
+    .class_size    = sizeof(ISASuperIOClass),
+    .class_init    = r4k_superio_class_init,
+};
+
+static void r4k_superio_register_types(void)
+{
+    type_register_static(&r4k_superio_type_info);
+}
+
+type_init(r4k_superio_register_types)
+
 typedef struct ResetData {
     MIPSCPU *cpu;
     uint64_t vector;
@@ -179,10 +218,8 @@ void mips_r4k_init(MachineState *machine)
     MIPSCPU *cpu;
     CPUMIPSState *env;
     ResetData *reset_info;
-    int i;
     qemu_irq *i8259;
     ISABus *isa_bus;
-    DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
     DriveInfo *dinfo;
     int be;
 
@@ -274,20 +311,12 @@ void mips_r4k_init(MachineState *machine)
 
     pit = i8254_pit_init(isa_bus, 0x40, 0, NULL);
 
-    serial_hds_isa_init(isa_bus, 0, MAX_ISA_SERIAL_PORTS);
-
     isa_vga_init(isa_bus);
 
     if (nd_table[0].used)
         isa_ne2000_init(isa_bus, 0x300, 9, &nd_table[0]);
 
-    ide_drive_get(hd, ARRAY_SIZE(hd));
-    for(i = 0; i < MAX_IDE_BUS; i++)
-        isa_ide_init(isa_bus, ide_iobase[i], ide_iobase2[i], ide_irq[i],
-                     hd[MAX_IDE_DEVS * i],
-                     hd[MAX_IDE_DEVS * i + 1]);
-
-    isa_create_simple(isa_bus, TYPE_I8042);
+    isa_create_simple(isa_bus, TYPE_R4K_SUPERIO);
 }
 
 static void mips_machine_init(MachineClass *mc)
-- 
2.20.1

Re: [Qemu-devel] [PATCH for-4.1 1/4] hw/isa/superio: Rename a variable

[Thomas Huth]

On 05/04/2019 00.12, Philippe Mathieu-Daudé wrote:
> This patch is purely cosmetic. No functional change.
> This will ease the next patch where we re-indent an if() statement.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/isa/isa-superio.c | 69 ++++++++++++++++++++++----------------------
>  1 file changed, 34 insertions(+), 35 deletions(-)
> 
> diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c
> index d54463bf035..c6845eaf578 100644
> --- a/hw/isa/isa-superio.c
> +++ b/hw/isa/isa-superio.c
> @@ -22,8 +22,8 @@
>  
>  static void isa_superio_realize(DeviceState *dev, Error **errp)
>  {
> -    ISASuperIODevice *sio = ISA_SUPERIO(dev);
> -    ISASuperIOClass *k = ISA_SUPERIO_GET_CLASS(sio);
> +    ISASuperIODevice *s = ISA_SUPERIO(dev);
> +    ISASuperIOClass *k = ISA_SUPERIO_GET_CLASS(s);

Sorry, but I have to say that I normally really don't like single-letter
variable names. They are much harder to search for, and way less
self-describing.

If you get problems with the line length in a later patch, what about
refactoring the related code into a separate function? So that you'd
only have something like this in the realize function:

    for (i = 0, j = 0; i < bus_count; i++, j += MAX_IDE_DEVS) {
        if (!k->ide.is_enabled || k->ide.is_enabled(s, j)) {
            isa_superio_init_ide(...);
        }
    }

 Thomas

Re: [Qemu-devel] [PATCH for-4.1 1/4] hw/isa/superio: Rename a variable

[Thomas Huth]

On 05/04/2019 00.12, Philippe Mathieu-Daudé wrote:
> This patch is purely cosmetic. No functional change.
> This will ease the next patch where we re-indent an if() statement.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/isa/isa-superio.c | 69 ++++++++++++++++++++++----------------------
>  1 file changed, 34 insertions(+), 35 deletions(-)
> 
> diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c
> index d54463bf035..c6845eaf578 100644
> --- a/hw/isa/isa-superio.c
> +++ b/hw/isa/isa-superio.c
> @@ -22,8 +22,8 @@
>  
>  static void isa_superio_realize(DeviceState *dev, Error **errp)
>  {
> -    ISASuperIODevice *sio = ISA_SUPERIO(dev);
> -    ISASuperIOClass *k = ISA_SUPERIO_GET_CLASS(sio);
> +    ISASuperIODevice *s = ISA_SUPERIO(dev);
> +    ISASuperIOClass *k = ISA_SUPERIO_GET_CLASS(s);

Sorry, but I have to say that I normally really don't like single-letter
variable names. They are much harder to search for, and way less
self-describing.

If you get problems with the line length in a later patch, what about
refactoring the related code into a separate function? So that you'd
only have something like this in the realize function:

    for (i = 0, j = 0; i < bus_count; i++, j += MAX_IDE_DEVS) {
        if (!k->ide.is_enabled || k->ide.is_enabled(s, j)) {
            isa_superio_init_ide(...);
        }
    }

 Thomas

Re: [Qemu-devel] [PATCH for-4.1 3/4] hw/isa/superio: Support chipsets with no Floppy Disk controller

[Thomas Huth]

On 05/04/2019 00.12, Philippe Mathieu-Daudé wrote:
> Not all Super I/O chipsets provide a Floppy Disk Controller.
> 
> Without this change, using a Super I/O with no FDC would abort QEMU with:
> 
>   Initialization of device isa-fdc failed: ISA controller does not support DMA
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/isa/isa-superio.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c
> index b0761ea1f96..6956f06d529 100644
> --- a/hw/isa/isa-superio.c
> +++ b/hw/isa/isa-superio.c
> @@ -113,7 +113,8 @@ static void isa_superio_realize(DeviceState *dev, Error **errp)
>      }
>  
>      /* Floppy disc */
> -    if (!k->floppy.is_enabled || k->floppy.is_enabled(s, 0)) {
> +    if (k->floppy.count
> +            && (!k->floppy.is_enabled || k->floppy.is_enabled(s, 0))) {
>          isa = isa_create(bus, "isa-fdc");
>          d = DEVICE(isa);
>          if (k->floppy.get_iobase) {

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH for-4.1 3/4] hw/isa/superio: Support chipsets with no Floppy Disk controller

[Thomas Huth]

On 05/04/2019 00.12, Philippe Mathieu-Daudé wrote:
> Not all Super I/O chipsets provide a Floppy Disk Controller.
> 
> Without this change, using a Super I/O with no FDC would abort QEMU with:
> 
>   Initialization of device isa-fdc failed: ISA controller does not support DMA
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/isa/isa-superio.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c
> index b0761ea1f96..6956f06d529 100644
> --- a/hw/isa/isa-superio.c
> +++ b/hw/isa/isa-superio.c
> @@ -113,7 +113,8 @@ static void isa_superio_realize(DeviceState *dev, Error **errp)
>      }
>  
>      /* Floppy disc */
> -    if (!k->floppy.is_enabled || k->floppy.is_enabled(s, 0)) {
> +    if (k->floppy.count
> +            && (!k->floppy.is_enabled || k->floppy.is_enabled(s, 0))) {
>          isa = isa_create(bus, "isa-fdc");
>          d = DEVICE(isa);
>          if (k->floppy.get_iobase) {

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH for-4.1 4/4] hw/mips/r4k: Refactor the Super I/O chipset

[Thomas Huth]

On 05/04/2019 00.12, Philippe Mathieu-Daudé wrote:
> ISA Super I/O are already modeled by the ISASuperIODevice abstract
> device.
> Since this board uses a generic ISA Super I/O chipset, refactor it
> as the TYPE_R4K_SUPERIO device, child of ISASuperIODevice.

Good idea!

> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/mips/mips_r4k.c | 61 ++++++++++++++++++++++++++++++++++------------
>  1 file changed, 45 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/mips/mips_r4k.c b/hw/mips/mips_r4k.c
> index 93dbf76bb49..b51a9523b43 100644
> --- a/hw/mips/mips_r4k.c
> +++ b/hw/mips/mips_r4k.c
> @@ -18,6 +18,7 @@
>  #include "hw/i386/pc.h"
>  #include "hw/char/serial.h"
>  #include "hw/isa/isa.h"
> +#include "hw/isa/superio.h"
>  #include "net/net.h"
>  #include "hw/net/ne2000-isa.h"
>  #include "sysemu/sysemu.h"
> @@ -29,7 +30,6 @@
>  #include "hw/loader.h"
>  #include "elf.h"
>  #include "hw/timer/mc146818rtc.h"
> -#include "hw/input/i8042.h"
>  #include "hw/timer/i8254.h"
>  #include "exec/address-spaces.h"
>  #include "sysemu/qtest.h"
> @@ -37,10 +37,6 @@
>  
>  #define MAX_IDE_BUS 2
>  
> -static const int ide_iobase[2] = { 0x1f0, 0x170 };
> -static const int ide_iobase2[2] = { 0x3f6, 0x376 };
> -static const int ide_irq[2] = { 14, 15 };
> -
>  static ISADevice *pit; /* PIT i8254 */
>  
>  /* i8254 PIT is attached to the IRQ0 at PIC i8259 */
> @@ -73,6 +69,49 @@ static const MemoryRegionOps mips_qemu_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> +#define TYPE_R4K_SUPERIO "r4k-superio"
> +
> +static uint16_t get_ide_iobase(ISASuperIODevice *sio, uint8_t index)
> +{
> +    static const uint16_t ide_iobase[] = { 0x1f0, 0x3f6, 0x170, 0x376 };
> +
> +    return ide_iobase[index];
> +}
> +
> +static unsigned int get_ide_irq(ISASuperIODevice *sio, uint8_t index)
> +{
> +    return index < MAX_IDE_DEVS ? 14 : 15;
> +}
> +
> +static void r4k_superio_class_init(ObjectClass *klass, void *data)
> +{
> +    ISASuperIOClass *sc = ISA_SUPERIO_CLASS(klass);
> +
> +    sc->serial.count = MAX_ISA_SERIAL_PORTS;
> +    sc->parallel.count = 0;
> +    sc->floppy.count = 0;
> +    sc->ide = (ISASuperIOFuncs){
> +        .count = MAX_IDE_BUS * MAX_IDE_DEVS,
> +        .get_iobase = get_ide_iobase,
> +        .get_irq    = get_ide_irq,
> +    };
> +}
> +
> +static const TypeInfo r4k_superio_type_info = {
> +    .name          = TYPE_R4K_SUPERIO,
> +    .parent        = TYPE_ISA_SUPERIO,
> +    .instance_size = sizeof(ISASuperIODevice),
> +    .class_size    = sizeof(ISASuperIOClass),
> +    .class_init    = r4k_superio_class_init,
> +};
> +
> +static void r4k_superio_register_types(void)
> +{
> +    type_register_static(&r4k_superio_type_info);
> +}
> +
> +type_init(r4k_superio_register_types)
> +
>  typedef struct ResetData {
>      MIPSCPU *cpu;
>      uint64_t vector;
> @@ -179,10 +218,8 @@ void mips_r4k_init(MachineState *machine)
>      MIPSCPU *cpu;
>      CPUMIPSState *env;
>      ResetData *reset_info;
> -    int i;
>      qemu_irq *i8259;
>      ISABus *isa_bus;
> -    DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
>      DriveInfo *dinfo;
>      int be;
>  
> @@ -274,20 +311,12 @@ void mips_r4k_init(MachineState *machine)
>  
>      pit = i8254_pit_init(isa_bus, 0x40, 0, NULL);
>  
> -    serial_hds_isa_init(isa_bus, 0, MAX_ISA_SERIAL_PORTS);
> -
>      isa_vga_init(isa_bus);
>  
>      if (nd_table[0].used)
>          isa_ne2000_init(isa_bus, 0x300, 9, &nd_table[0]);
>  
> -    ide_drive_get(hd, ARRAY_SIZE(hd));
> -    for(i = 0; i < MAX_IDE_BUS; i++)
> -        isa_ide_init(isa_bus, ide_iobase[i], ide_iobase2[i], ide_irq[i],
> -                     hd[MAX_IDE_DEVS * i],
> -                     hd[MAX_IDE_DEVS * i + 1]);
> -
> -    isa_create_simple(isa_bus, TYPE_I8042);
> +    isa_create_simple(isa_bus, TYPE_R4K_SUPERIO);

What about the ide_drive_get() and the ide_create_drive() that is done
by isa_ide_init() internally? As far as I can see, the superio code does
not do this job for you? So don't you have to do that manually here
after creating the R4K_SUPERIO device?

 Thomas

Re: [Qemu-devel] [PATCH for-4.1 4/4] hw/mips/r4k: Refactor the Super I/O chipset

[Thomas Huth]

On 05/04/2019 00.12, Philippe Mathieu-Daudé wrote:
> ISA Super I/O are already modeled by the ISASuperIODevice abstract
> device.
> Since this board uses a generic ISA Super I/O chipset, refactor it
> as the TYPE_R4K_SUPERIO device, child of ISASuperIODevice.

Good idea!

> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/mips/mips_r4k.c | 61 ++++++++++++++++++++++++++++++++++------------
>  1 file changed, 45 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/mips/mips_r4k.c b/hw/mips/mips_r4k.c
> index 93dbf76bb49..b51a9523b43 100644
> --- a/hw/mips/mips_r4k.c
> +++ b/hw/mips/mips_r4k.c
> @@ -18,6 +18,7 @@
>  #include "hw/i386/pc.h"
>  #include "hw/char/serial.h"
>  #include "hw/isa/isa.h"
> +#include "hw/isa/superio.h"
>  #include "net/net.h"
>  #include "hw/net/ne2000-isa.h"
>  #include "sysemu/sysemu.h"
> @@ -29,7 +30,6 @@
>  #include "hw/loader.h"
>  #include "elf.h"
>  #include "hw/timer/mc146818rtc.h"
> -#include "hw/input/i8042.h"
>  #include "hw/timer/i8254.h"
>  #include "exec/address-spaces.h"
>  #include "sysemu/qtest.h"
> @@ -37,10 +37,6 @@
>  
>  #define MAX_IDE_BUS 2
>  
> -static const int ide_iobase[2] = { 0x1f0, 0x170 };
> -static const int ide_iobase2[2] = { 0x3f6, 0x376 };
> -static const int ide_irq[2] = { 14, 15 };
> -
>  static ISADevice *pit; /* PIT i8254 */
>  
>  /* i8254 PIT is attached to the IRQ0 at PIC i8259 */
> @@ -73,6 +69,49 @@ static const MemoryRegionOps mips_qemu_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> +#define TYPE_R4K_SUPERIO "r4k-superio"
> +
> +static uint16_t get_ide_iobase(ISASuperIODevice *sio, uint8_t index)
> +{
> +    static const uint16_t ide_iobase[] = { 0x1f0, 0x3f6, 0x170, 0x376 };
> +
> +    return ide_iobase[index];
> +}
> +
> +static unsigned int get_ide_irq(ISASuperIODevice *sio, uint8_t index)
> +{
> +    return index < MAX_IDE_DEVS ? 14 : 15;
> +}
> +
> +static void r4k_superio_class_init(ObjectClass *klass, void *data)
> +{
> +    ISASuperIOClass *sc = ISA_SUPERIO_CLASS(klass);
> +
> +    sc->serial.count = MAX_ISA_SERIAL_PORTS;
> +    sc->parallel.count = 0;
> +    sc->floppy.count = 0;
> +    sc->ide = (ISASuperIOFuncs){
> +        .count = MAX_IDE_BUS * MAX_IDE_DEVS,
> +        .get_iobase = get_ide_iobase,
> +        .get_irq    = get_ide_irq,
> +    };
> +}
> +
> +static const TypeInfo r4k_superio_type_info = {
> +    .name          = TYPE_R4K_SUPERIO,
> +    .parent        = TYPE_ISA_SUPERIO,
> +    .instance_size = sizeof(ISASuperIODevice),
> +    .class_size    = sizeof(ISASuperIOClass),
> +    .class_init    = r4k_superio_class_init,
> +};
> +
> +static void r4k_superio_register_types(void)
> +{
> +    type_register_static(&r4k_superio_type_info);
> +}
> +
> +type_init(r4k_superio_register_types)
> +
>  typedef struct ResetData {
>      MIPSCPU *cpu;
>      uint64_t vector;
> @@ -179,10 +218,8 @@ void mips_r4k_init(MachineState *machine)
>      MIPSCPU *cpu;
>      CPUMIPSState *env;
>      ResetData *reset_info;
> -    int i;
>      qemu_irq *i8259;
>      ISABus *isa_bus;
> -    DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
>      DriveInfo *dinfo;
>      int be;
>  
> @@ -274,20 +311,12 @@ void mips_r4k_init(MachineState *machine)
>  
>      pit = i8254_pit_init(isa_bus, 0x40, 0, NULL);
>  
> -    serial_hds_isa_init(isa_bus, 0, MAX_ISA_SERIAL_PORTS);
> -
>      isa_vga_init(isa_bus);
>  
>      if (nd_table[0].used)
>          isa_ne2000_init(isa_bus, 0x300, 9, &nd_table[0]);
>  
> -    ide_drive_get(hd, ARRAY_SIZE(hd));
> -    for(i = 0; i < MAX_IDE_BUS; i++)
> -        isa_ide_init(isa_bus, ide_iobase[i], ide_iobase2[i], ide_irq[i],
> -                     hd[MAX_IDE_DEVS * i],
> -                     hd[MAX_IDE_DEVS * i + 1]);
> -
> -    isa_create_simple(isa_bus, TYPE_I8042);
> +    isa_create_simple(isa_bus, TYPE_R4K_SUPERIO);

What about the ide_drive_get() and the ide_create_drive() that is done
by isa_ide_init() internally? As far as I can see, the superio code does
not do this job for you? So don't you have to do that manually here
after creating the R4K_SUPERIO device?

 Thomas

Re: [Qemu-devel] [PATCH for-4.1 1/4] hw/isa/superio: Rename a variable

[Philippe Mathieu-Daudé]

On 4/5/19 6:14 AM, Thomas Huth wrote:
> On 05/04/2019 00.12, Philippe Mathieu-Daudé wrote:
>> This patch is purely cosmetic. No functional change.
>> This will ease the next patch where we re-indent an if() statement.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/isa/isa-superio.c | 69 ++++++++++++++++++++++----------------------
>>  1 file changed, 34 insertions(+), 35 deletions(-)
>>
>> diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c
>> index d54463bf035..c6845eaf578 100644
>> --- a/hw/isa/isa-superio.c
>> +++ b/hw/isa/isa-superio.c
>> @@ -22,8 +22,8 @@
>>  
>>  static void isa_superio_realize(DeviceState *dev, Error **errp)
>>  {
>> -    ISASuperIODevice *sio = ISA_SUPERIO(dev);
>> -    ISASuperIOClass *k = ISA_SUPERIO_GET_CLASS(sio);
>> +    ISASuperIODevice *s = ISA_SUPERIO(dev);
>> +    ISASuperIOClass *k = ISA_SUPERIO_GET_CLASS(s);
> 
> Sorry, but I have to say that I normally really don't like single-letter
> variable names. They are much harder to search for, and way less
> self-describing.

Me too ;) But sometimes I find the 80 chars/line limit generate
indentation harder to review. But here I have no excuse :)

> If you get problems with the line length in a later patch, what about
> refactoring the related code into a separate function? So that you'd
> only have something like this in the realize function:
> 
>     for (i = 0, j = 0; i < bus_count; i++, j += MAX_IDE_DEVS) {
>         if (!k->ide.is_enabled || k->ide.is_enabled(s, j)) {
>             isa_superio_init_ide(...);
>         }
>     }

Yes, fair enough. Note you used the shortened variables in your example! :P

Regards,

Phil.

Re: [Qemu-devel] [PATCH for-4.1 1/4] hw/isa/superio: Rename a variable

[Philippe Mathieu-Daudé]

On 4/5/19 6:14 AM, Thomas Huth wrote:
> On 05/04/2019 00.12, Philippe Mathieu-Daudé wrote:
>> This patch is purely cosmetic. No functional change.
>> This will ease the next patch where we re-indent an if() statement.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/isa/isa-superio.c | 69 ++++++++++++++++++++++----------------------
>>  1 file changed, 34 insertions(+), 35 deletions(-)
>>
>> diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c
>> index d54463bf035..c6845eaf578 100644
>> --- a/hw/isa/isa-superio.c
>> +++ b/hw/isa/isa-superio.c
>> @@ -22,8 +22,8 @@
>>  
>>  static void isa_superio_realize(DeviceState *dev, Error **errp)
>>  {
>> -    ISASuperIODevice *sio = ISA_SUPERIO(dev);
>> -    ISASuperIOClass *k = ISA_SUPERIO_GET_CLASS(sio);
>> +    ISASuperIODevice *s = ISA_SUPERIO(dev);
>> +    ISASuperIOClass *k = ISA_SUPERIO_GET_CLASS(s);
> 
> Sorry, but I have to say that I normally really don't like single-letter
> variable names. They are much harder to search for, and way less
> self-describing.

Me too ;) But sometimes I find the 80 chars/line limit generate
indentation harder to review. But here I have no excuse :)

> If you get problems with the line length in a later patch, what about
> refactoring the related code into a separate function? So that you'd
> only have something like this in the realize function:
> 
>     for (i = 0, j = 0; i < bus_count; i++, j += MAX_IDE_DEVS) {
>         if (!k->ide.is_enabled || k->ide.is_enabled(s, j)) {
>             isa_superio_init_ide(...);
>         }
>     }

Yes, fair enough. Note you used the shortened variables in your example! :P

Regards,

Phil.

Re: [Qemu-devel] [PATCH for-4.1 4/4] hw/mips/r4k: Refactor the Super I/O chipset

[Philippe Mathieu-Daudé]

On 4/5/19 6:51 AM, Thomas Huth wrote:
> On 05/04/2019 00.12, Philippe Mathieu-Daudé wrote:
>> ISA Super I/O are already modeled by the ISASuperIODevice abstract
>> device.
>> Since this board uses a generic ISA Super I/O chipset, refactor it
>> as the TYPE_R4K_SUPERIO device, child of ISASuperIODevice.
> 
> Good idea!
> 
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/mips/mips_r4k.c | 61 ++++++++++++++++++++++++++++++++++------------
>>  1 file changed, 45 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/mips/mips_r4k.c b/hw/mips/mips_r4k.c
>> index 93dbf76bb49..b51a9523b43 100644
>> --- a/hw/mips/mips_r4k.c
>> +++ b/hw/mips/mips_r4k.c
>> @@ -18,6 +18,7 @@
>>  #include "hw/i386/pc.h"
>>  #include "hw/char/serial.h"
>>  #include "hw/isa/isa.h"
>> +#include "hw/isa/superio.h"
>>  #include "net/net.h"
>>  #include "hw/net/ne2000-isa.h"
>>  #include "sysemu/sysemu.h"
>> @@ -29,7 +30,6 @@
>>  #include "hw/loader.h"
>>  #include "elf.h"
>>  #include "hw/timer/mc146818rtc.h"
>> -#include "hw/input/i8042.h"
>>  #include "hw/timer/i8254.h"
>>  #include "exec/address-spaces.h"
>>  #include "sysemu/qtest.h"
>> @@ -37,10 +37,6 @@
>>  
>>  #define MAX_IDE_BUS 2
>>  
>> -static const int ide_iobase[2] = { 0x1f0, 0x170 };
>> -static const int ide_iobase2[2] = { 0x3f6, 0x376 };
>> -static const int ide_irq[2] = { 14, 15 };
>> -
>>  static ISADevice *pit; /* PIT i8254 */
>>  
>>  /* i8254 PIT is attached to the IRQ0 at PIC i8259 */
>> @@ -73,6 +69,49 @@ static const MemoryRegionOps mips_qemu_ops = {
>>      .endianness = DEVICE_NATIVE_ENDIAN,
>>  };
>>  
>> +#define TYPE_R4K_SUPERIO "r4k-superio"
>> +
>> +static uint16_t get_ide_iobase(ISASuperIODevice *sio, uint8_t index)
>> +{
>> +    static const uint16_t ide_iobase[] = { 0x1f0, 0x3f6, 0x170, 0x376 };
>> +
>> +    return ide_iobase[index];
>> +}
>> +
>> +static unsigned int get_ide_irq(ISASuperIODevice *sio, uint8_t index)
>> +{
>> +    return index < MAX_IDE_DEVS ? 14 : 15;
>> +}
>> +
>> +static void r4k_superio_class_init(ObjectClass *klass, void *data)
>> +{
>> +    ISASuperIOClass *sc = ISA_SUPERIO_CLASS(klass);
>> +
>> +    sc->serial.count = MAX_ISA_SERIAL_PORTS;
>> +    sc->parallel.count = 0;
>> +    sc->floppy.count = 0;
>> +    sc->ide = (ISASuperIOFuncs){
>> +        .count = MAX_IDE_BUS * MAX_IDE_DEVS,
>> +        .get_iobase = get_ide_iobase,
>> +        .get_irq    = get_ide_irq,
>> +    };
>> +}
>> +
>> +static const TypeInfo r4k_superio_type_info = {
>> +    .name          = TYPE_R4K_SUPERIO,
>> +    .parent        = TYPE_ISA_SUPERIO,
>> +    .instance_size = sizeof(ISASuperIODevice),
>> +    .class_size    = sizeof(ISASuperIOClass),
>> +    .class_init    = r4k_superio_class_init,
>> +};
>> +
>> +static void r4k_superio_register_types(void)
>> +{
>> +    type_register_static(&r4k_superio_type_info);
>> +}
>> +
>> +type_init(r4k_superio_register_types)
>> +
>>  typedef struct ResetData {
>>      MIPSCPU *cpu;
>>      uint64_t vector;
>> @@ -179,10 +218,8 @@ void mips_r4k_init(MachineState *machine)
>>      MIPSCPU *cpu;
>>      CPUMIPSState *env;
>>      ResetData *reset_info;
>> -    int i;
>>      qemu_irq *i8259;
>>      ISABus *isa_bus;
>> -    DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
>>      DriveInfo *dinfo;
>>      int be;
>>  
>> @@ -274,20 +311,12 @@ void mips_r4k_init(MachineState *machine)
>>  
>>      pit = i8254_pit_init(isa_bus, 0x40, 0, NULL);
>>  
>> -    serial_hds_isa_init(isa_bus, 0, MAX_ISA_SERIAL_PORTS);
>> -
>>      isa_vga_init(isa_bus);
>>  
>>      if (nd_table[0].used)
>>          isa_ne2000_init(isa_bus, 0x300, 9, &nd_table[0]);
>>  
>> -    ide_drive_get(hd, ARRAY_SIZE(hd));
>> -    for(i = 0; i < MAX_IDE_BUS; i++)
>> -        isa_ide_init(isa_bus, ide_iobase[i], ide_iobase2[i], ide_irq[i],
>> -                     hd[MAX_IDE_DEVS * i],
>> -                     hd[MAX_IDE_DEVS * i + 1]);
>> -
>> -    isa_create_simple(isa_bus, TYPE_I8042);
>> +    isa_create_simple(isa_bus, TYPE_R4K_SUPERIO);
> 
> What about the ide_drive_get() and the ide_create_drive() that is done
> by isa_ide_init() internally? As far as I can see, the superio code does
> not do this job for you? So don't you have to do that manually here
> after creating the R4K_SUPERIO device?

Oops... I was scared I did the same mistake with the Floppy controller
in 7313b1f28be but hopefully not. However you made me notice I never
considered the case MAX_FD>2. I'll send a mail asking if this case is
still used.

Thanks!

Phil.

Re: [Qemu-devel] [PATCH for-4.1 4/4] hw/mips/r4k: Refactor the Super I/O chipset

[Philippe Mathieu-Daudé]

On 4/5/19 6:51 AM, Thomas Huth wrote:
> On 05/04/2019 00.12, Philippe Mathieu-Daudé wrote:
>> ISA Super I/O are already modeled by the ISASuperIODevice abstract
>> device.
>> Since this board uses a generic ISA Super I/O chipset, refactor it
>> as the TYPE_R4K_SUPERIO device, child of ISASuperIODevice.
> 
> Good idea!
> 
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/mips/mips_r4k.c | 61 ++++++++++++++++++++++++++++++++++------------
>>  1 file changed, 45 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/mips/mips_r4k.c b/hw/mips/mips_r4k.c
>> index 93dbf76bb49..b51a9523b43 100644
>> --- a/hw/mips/mips_r4k.c
>> +++ b/hw/mips/mips_r4k.c
>> @@ -18,6 +18,7 @@
>>  #include "hw/i386/pc.h"
>>  #include "hw/char/serial.h"
>>  #include "hw/isa/isa.h"
>> +#include "hw/isa/superio.h"
>>  #include "net/net.h"
>>  #include "hw/net/ne2000-isa.h"
>>  #include "sysemu/sysemu.h"
>> @@ -29,7 +30,6 @@
>>  #include "hw/loader.h"
>>  #include "elf.h"
>>  #include "hw/timer/mc146818rtc.h"
>> -#include "hw/input/i8042.h"
>>  #include "hw/timer/i8254.h"
>>  #include "exec/address-spaces.h"
>>  #include "sysemu/qtest.h"
>> @@ -37,10 +37,6 @@
>>  
>>  #define MAX_IDE_BUS 2
>>  
>> -static const int ide_iobase[2] = { 0x1f0, 0x170 };
>> -static const int ide_iobase2[2] = { 0x3f6, 0x376 };
>> -static const int ide_irq[2] = { 14, 15 };
>> -
>>  static ISADevice *pit; /* PIT i8254 */
>>  
>>  /* i8254 PIT is attached to the IRQ0 at PIC i8259 */
>> @@ -73,6 +69,49 @@ static const MemoryRegionOps mips_qemu_ops = {
>>      .endianness = DEVICE_NATIVE_ENDIAN,
>>  };
>>  
>> +#define TYPE_R4K_SUPERIO "r4k-superio"
>> +
>> +static uint16_t get_ide_iobase(ISASuperIODevice *sio, uint8_t index)
>> +{
>> +    static const uint16_t ide_iobase[] = { 0x1f0, 0x3f6, 0x170, 0x376 };
>> +
>> +    return ide_iobase[index];
>> +}
>> +
>> +static unsigned int get_ide_irq(ISASuperIODevice *sio, uint8_t index)
>> +{
>> +    return index < MAX_IDE_DEVS ? 14 : 15;
>> +}
>> +
>> +static void r4k_superio_class_init(ObjectClass *klass, void *data)
>> +{
>> +    ISASuperIOClass *sc = ISA_SUPERIO_CLASS(klass);
>> +
>> +    sc->serial.count = MAX_ISA_SERIAL_PORTS;
>> +    sc->parallel.count = 0;
>> +    sc->floppy.count = 0;
>> +    sc->ide = (ISASuperIOFuncs){
>> +        .count = MAX_IDE_BUS * MAX_IDE_DEVS,
>> +        .get_iobase = get_ide_iobase,
>> +        .get_irq    = get_ide_irq,
>> +    };
>> +}
>> +
>> +static const TypeInfo r4k_superio_type_info = {
>> +    .name          = TYPE_R4K_SUPERIO,
>> +    .parent        = TYPE_ISA_SUPERIO,
>> +    .instance_size = sizeof(ISASuperIODevice),
>> +    .class_size    = sizeof(ISASuperIOClass),
>> +    .class_init    = r4k_superio_class_init,
>> +};
>> +
>> +static void r4k_superio_register_types(void)
>> +{
>> +    type_register_static(&r4k_superio_type_info);
>> +}
>> +
>> +type_init(r4k_superio_register_types)
>> +
>>  typedef struct ResetData {
>>      MIPSCPU *cpu;
>>      uint64_t vector;
>> @@ -179,10 +218,8 @@ void mips_r4k_init(MachineState *machine)
>>      MIPSCPU *cpu;
>>      CPUMIPSState *env;
>>      ResetData *reset_info;
>> -    int i;
>>      qemu_irq *i8259;
>>      ISABus *isa_bus;
>> -    DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
>>      DriveInfo *dinfo;
>>      int be;
>>  
>> @@ -274,20 +311,12 @@ void mips_r4k_init(MachineState *machine)
>>  
>>      pit = i8254_pit_init(isa_bus, 0x40, 0, NULL);
>>  
>> -    serial_hds_isa_init(isa_bus, 0, MAX_ISA_SERIAL_PORTS);
>> -
>>      isa_vga_init(isa_bus);
>>  
>>      if (nd_table[0].used)
>>          isa_ne2000_init(isa_bus, 0x300, 9, &nd_table[0]);
>>  
>> -    ide_drive_get(hd, ARRAY_SIZE(hd));
>> -    for(i = 0; i < MAX_IDE_BUS; i++)
>> -        isa_ide_init(isa_bus, ide_iobase[i], ide_iobase2[i], ide_irq[i],
>> -                     hd[MAX_IDE_DEVS * i],
>> -                     hd[MAX_IDE_DEVS * i + 1]);
>> -
>> -    isa_create_simple(isa_bus, TYPE_I8042);
>> +    isa_create_simple(isa_bus, TYPE_R4K_SUPERIO);
> 
> What about the ide_drive_get() and the ide_create_drive() that is done
> by isa_ide_init() internally? As far as I can see, the superio code does
> not do this job for you? So don't you have to do that manually here
> after creating the R4K_SUPERIO device?

Oops... I was scared I did the same mistake with the Floppy controller
in 7313b1f28be but hopefully not. However you made me notice I never
considered the case MAX_FD>2. I'll send a mail asking if this case is
still used.

Thanks!

Phil.