[PATCH v2 0/4] hw/display: QOM'ify vga_mmio_init() as TYPE_VGA_MMIO

[PATCH v2 0/4] hw/display: QOM'ify vga_mmio_init() as TYPE_VGA_MMIO

[Philippe Mathieu-Daudé]

Convert the open-coded vga_mmio_init() to a sysbus device.

Since v1:
- Addressed Zolton and Thomas comments, added their R-b tags

Philippe Mathieu-Daudé (4):
  hw/display: Rename VGA_ISA_MM -> VGA_MMIO
  hw/display/vga-mmio: Inline vga_mm_init()
  hw/display/vga-mmio: QOM'ify vga_mmio_init() as TYPE_VGA_MMIO
  hw/mips/jazz: Inline vga_mmio_init() and remove it

 configs/devices/mips-softmmu/common.mak |   2 +-
 include/hw/display/vga.h                |   6 +-
 hw/display/vga-isa-mm.c                 | 114 -------------------
 hw/display/vga-mmio.c                   | 139 ++++++++++++++++++++++++
 hw/mips/jazz.c                          |   9 +-
 hw/display/Kconfig                      |   2 +-
 hw/display/meson.build                  |   2 +-
 hw/mips/Kconfig                         |   2 +-
 8 files changed, 152 insertions(+), 124 deletions(-)
 delete mode 100644 hw/display/vga-isa-mm.c
 create mode 100644 hw/display/vga-mmio.c

-- 
2.33.1

[PATCH v2 1/4] hw/display: Rename VGA_ISA_MM -> VGA_MMIO

[Philippe Mathieu-Daudé]

There is no ISA bus part in the MMIO VGA device, so rename:

 *  hw/display/vga-isa-mm.c -> hw/display/vga-mmio.c
 *  CONFIG_VGA_ISA_MM -> CONFIG_VGA_MMIO
 *  ISAVGAMMState -> VGAMmioState
 *  isa_vga_mm_init() -> vga_mmio_init()

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 configs/devices/mips-softmmu/common.mak |  2 +-
 include/hw/display/vga.h                |  5 ++---
 hw/display/{vga-isa-mm.c => vga-mmio.c} | 19 +++++++++----------
 hw/mips/jazz.c                          |  2 +-
 hw/display/Kconfig                      |  2 +-
 hw/display/meson.build                  |  2 +-
 hw/mips/Kconfig                         |  2 +-
 7 files changed, 16 insertions(+), 18 deletions(-)
 rename hw/display/{vga-isa-mm.c => vga-mmio.c} (90%)

diff --git a/configs/devices/mips-softmmu/common.mak b/configs/devices/mips-softmmu/common.mak
index 752b62b1e63..d2202c839e0 100644
--- a/configs/devices/mips-softmmu/common.mak
+++ b/configs/devices/mips-softmmu/common.mak
@@ -7,7 +7,7 @@ CONFIG_ISA_BUS=y
 CONFIG_PCI=y
 CONFIG_PCI_DEVICES=y
 CONFIG_VGA_ISA=y
-CONFIG_VGA_ISA_MM=y
+CONFIG_VGA_MMIO=y
 CONFIG_VGA_CIRRUS=y
 CONFIG_VMWARE_VGA=y
 CONFIG_SERIAL=y
diff --git a/include/hw/display/vga.h b/include/hw/display/vga.h
index 5f7825e0e36..c16a5c26dae 100644
--- a/include/hw/display/vga.h
+++ b/include/hw/display/vga.h
@@ -24,8 +24,7 @@ enum vga_retrace_method {
 
 extern enum vga_retrace_method vga_retrace_method;
 
-int isa_vga_mm_init(hwaddr vram_base,
-                    hwaddr ctrl_base, int it_shift,
-                    MemoryRegion *address_space);
+int vga_mmio_init(hwaddr vram_base, hwaddr ctrl_base,
+                  int it_shift, MemoryRegion *address_space);
 
 #endif
diff --git a/hw/display/vga-isa-mm.c b/hw/display/vga-mmio.c
similarity index 90%
rename from hw/display/vga-isa-mm.c
rename to hw/display/vga-mmio.c
index 7321b7a06d5..4ffe3afe32d 100644
--- a/hw/display/vga-isa-mm.c
+++ b/hw/display/vga-mmio.c
@@ -1,5 +1,5 @@
 /*
- * QEMU ISA MM VGA Emulator.
+ * QEMU MMIO VGA Emulator.
  *
  * Copyright (c) 2003 Fabrice Bellard
  *
@@ -32,15 +32,15 @@
 
 #define VGA_RAM_SIZE (8 * MiB)
 
-typedef struct ISAVGAMMState {
+typedef struct VGAMmioState {
     VGACommonState vga;
     int it_shift;
-} ISAVGAMMState;
+} VGAMmioState;
 
 /* Memory mapped interface */
 static uint64_t vga_mm_read(void *opaque, hwaddr addr, unsigned size)
 {
-    ISAVGAMMState *s = opaque;
+    VGAMmioState *s = opaque;
 
     return vga_ioport_read(&s->vga, addr >> s->it_shift) &
         MAKE_64BIT_MASK(0, size * 8);
@@ -49,7 +49,7 @@ static uint64_t vga_mm_read(void *opaque, hwaddr addr, unsigned size)
 static void vga_mm_write(void *opaque, hwaddr addr, uint64_t value,
                          unsigned size)
 {
-    ISAVGAMMState *s = opaque;
+    VGAMmioState *s = opaque;
 
     vga_ioport_write(&s->vga, addr >> s->it_shift,
                      value & MAKE_64BIT_MASK(0, size * 8));
@@ -65,7 +65,7 @@ static const MemoryRegionOps vga_mm_ctrl_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static void vga_mm_init(ISAVGAMMState *s, hwaddr vram_base,
+static void vga_mm_init(VGAMmioState *s, hwaddr vram_base,
                         hwaddr ctrl_base, int it_shift,
                         MemoryRegion *address_space)
 {
@@ -91,11 +91,10 @@ static void vga_mm_init(ISAVGAMMState *s, hwaddr vram_base,
     memory_region_set_coalescing(vga_io_memory);
 }
 
-int isa_vga_mm_init(hwaddr vram_base,
-                    hwaddr ctrl_base, int it_shift,
-                    MemoryRegion *address_space)
+int vga_mmio_init(hwaddr vram_base, hwaddr ctrl_base,
+                  int it_shift, MemoryRegion *address_space)
 {
-    ISAVGAMMState *s;
+    VGAMmioState *s;
 
     s = g_malloc0(sizeof(*s));
 
diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
index f5a26e174d5..8f345afd137 100644
--- a/hw/mips/jazz.c
+++ b/hw/mips/jazz.c
@@ -274,7 +274,7 @@ static void mips_jazz_init(MachineState *machine,
         }
         break;
     case JAZZ_PICA61:
-        isa_vga_mm_init(0x40000000, 0x60000000, 0, get_system_memory());
+        vga_mmio_init(0x40000000, 0x60000000, 0, get_system_memory());
         break;
     default:
         break;
diff --git a/hw/display/Kconfig b/hw/display/Kconfig
index a2306b67d87..a1b159becd7 100644
--- a/hw/display/Kconfig
+++ b/hw/display/Kconfig
@@ -49,7 +49,7 @@ config VGA_ISA
     depends on ISA_BUS
     select VGA
 
-config VGA_ISA_MM
+config VGA_MMIO
     bool
     select VGA
 
diff --git a/hw/display/meson.build b/hw/display/meson.build
index 861c43ff984..adc53dd8b6c 100644
--- a/hw/display/meson.build
+++ b/hw/display/meson.build
@@ -18,7 +18,7 @@
 
 softmmu_ss.add(when: 'CONFIG_VGA_PCI', if_true: files('vga-pci.c'))
 softmmu_ss.add(when: 'CONFIG_VGA_ISA', if_true: files('vga-isa.c'))
-softmmu_ss.add(when: 'CONFIG_VGA_ISA_MM', if_true: files('vga-isa-mm.c'))
+softmmu_ss.add(when: 'CONFIG_VGA_MMIO', if_true: files('vga-mmio.c'))
 softmmu_ss.add(when: 'CONFIG_VMWARE_VGA', if_true: files('vmware_vga.c'))
 softmmu_ss.add(when: 'CONFIG_BOCHS_DISPLAY', if_true: files('bochs-display.c'))
 
diff --git a/hw/mips/Kconfig b/hw/mips/Kconfig
index b4c5549ce84..725525358d9 100644
--- a/hw/mips/Kconfig
+++ b/hw/mips/Kconfig
@@ -16,7 +16,7 @@ config JAZZ
     select I8254
     select I8257
     select PCSPK
-    select VGA_ISA_MM
+    select VGA_MMIO
     select G364FB
     select DP8393X
     select ESP
-- 
2.33.1

[PATCH v2 2/4] hw/display/vga-mmio: Inline vga_mm_init()

[Philippe Mathieu-Daudé]

Inline vga_mm_init() in vga_mmio_init() to simplify the
next patch review. Kind of.

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/display/vga-mmio.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/hw/display/vga-mmio.c b/hw/display/vga-mmio.c
index 4ffe3afe32d..5671fdb920f 100644
--- a/hw/display/vga-mmio.c
+++ b/hw/display/vga-mmio.c
@@ -65,12 +65,18 @@ static const MemoryRegionOps vga_mm_ctrl_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static void vga_mm_init(VGAMmioState *s, hwaddr vram_base,
-                        hwaddr ctrl_base, int it_shift,
-                        MemoryRegion *address_space)
+int vga_mmio_init(hwaddr vram_base, hwaddr ctrl_base,
+                  int it_shift, MemoryRegion *address_space)
 {
+    VGAMmioState *s;
     MemoryRegion *s_ioport_ctrl, *vga_io_memory;
 
+    s = g_malloc0(sizeof(*s));
+
+    s->vga.vram_size_mb = VGA_RAM_SIZE / MiB;
+    s->vga.global_vmstate = true;
+    vga_common_init(&s->vga, NULL);
+
     s->it_shift = it_shift;
     s_ioport_ctrl = g_malloc(sizeof(*s_ioport_ctrl));
     memory_region_init_io(s_ioport_ctrl, NULL, &vga_mm_ctrl_ops, s,
@@ -89,19 +95,6 @@ static void vga_mm_init(VGAMmioState *s, hwaddr vram_base,
     memory_region_add_subregion(address_space,
                                 vram_base + 0x000a0000, vga_io_memory);
     memory_region_set_coalescing(vga_io_memory);
-}
-
-int vga_mmio_init(hwaddr vram_base, hwaddr ctrl_base,
-                  int it_shift, MemoryRegion *address_space)
-{
-    VGAMmioState *s;
-
-    s = g_malloc0(sizeof(*s));
-
-    s->vga.vram_size_mb = VGA_RAM_SIZE / MiB;
-    s->vga.global_vmstate = true;
-    vga_common_init(&s->vga, NULL);
-    vga_mm_init(s, vram_base, ctrl_base, it_shift, address_space);
 
     s->vga.con = graphic_console_init(NULL, 0, s->vga.hw_ops, s);
 
-- 
2.33.1

[PATCH v2 3/4] hw/display/vga-mmio: QOM'ify vga_mmio_init() as TYPE_VGA_MMIO

[Philippe Mathieu-Daudé]

Introduce TYPE_VGA_MMIO, a sysbus device.

While there is no change in the vga_mmio_init()
interface, this is a migration compatibility break
of the MIPS Acer Pica 61 Jazz machine (pica61).

Suggested-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/display/vga.h |   2 +
 hw/display/vga-mmio.c    | 132 +++++++++++++++++++++++++++------------
 2 files changed, 94 insertions(+), 40 deletions(-)

diff --git a/include/hw/display/vga.h b/include/hw/display/vga.h
index c16a5c26dae..98b2e560f9b 100644
--- a/include/hw/display/vga.h
+++ b/include/hw/display/vga.h
@@ -24,6 +24,8 @@ enum vga_retrace_method {
 
 extern enum vga_retrace_method vga_retrace_method;
 
+#define TYPE_VGA_MMIO "vga-mmio"
+
 int vga_mmio_init(hwaddr vram_base, hwaddr ctrl_base,
                   int it_shift, MemoryRegion *address_space);
 
diff --git a/hw/display/vga-mmio.c b/hw/display/vga-mmio.c
index 5671fdb920f..10bde32af5c 100644
--- a/hw/display/vga-mmio.c
+++ b/hw/display/vga-mmio.c
@@ -23,21 +23,34 @@
  */
 
 #include "qemu/osdep.h"
-#include "qemu/bitops.h"
-#include "qemu/units.h"
-#include "migration/vmstate.h"
+#include "qapi/error.h"
 #include "hw/display/vga.h"
+#include "hw/sysbus.h"
+#include "hw/display/vga.h"
+#include "hw/qdev-properties.h"
 #include "vga_int.h"
-#include "ui/pixel_ops.h"
 
-#define VGA_RAM_SIZE (8 * MiB)
+/*
+ * QEMU interface:
+ *  + sysbus MMIO region 0: VGA I/O registers
+ *  + sysbus MMIO region 1: VGA MMIO registers
+ *  + sysbus MMIO region 2: VGA memory
+ */
 
-typedef struct VGAMmioState {
+OBJECT_DECLARE_SIMPLE_TYPE(VGAMmioState, VGA_MMIO)
+
+struct VGAMmioState {
+    /*< private >*/
+    SysBusDevice parent_obj;
+
+    /*< public >*/
     VGACommonState vga;
-    int it_shift;
-} VGAMmioState;
+    MemoryRegion iomem;
+    MemoryRegion lowmem;
+
+    uint8_t it_shift;
+};
 
-/* Memory mapped interface */
 static uint64_t vga_mm_read(void *opaque, hwaddr addr, unsigned size)
 {
     VGAMmioState *s = opaque;
@@ -65,42 +78,81 @@ static const MemoryRegionOps vga_mm_ctrl_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
+static void vga_mmio_reset(DeviceState *dev)
+{
+    VGAMmioState *s = VGA_MMIO(dev);
+
+    vga_common_reset(&s->vga);
+}
+
 int vga_mmio_init(hwaddr vram_base, hwaddr ctrl_base,
                   int it_shift, MemoryRegion *address_space)
 {
-    VGAMmioState *s;
-    MemoryRegion *s_ioport_ctrl, *vga_io_memory;
+    DeviceState *dev;
+    SysBusDevice *s;
 
-    s = g_malloc0(sizeof(*s));
+    dev = qdev_new(TYPE_VGA_MMIO);
+    qdev_prop_set_uint8(dev, "it_shift", it_shift);
+    s = SYS_BUS_DEVICE(dev);
+    sysbus_realize_and_unref(s, &error_fatal);
 
-    s->vga.vram_size_mb = VGA_RAM_SIZE / MiB;
-    s->vga.global_vmstate = true;
-    vga_common_init(&s->vga, NULL);
-
-    s->it_shift = it_shift;
-    s_ioport_ctrl = g_malloc(sizeof(*s_ioport_ctrl));
-    memory_region_init_io(s_ioport_ctrl, NULL, &vga_mm_ctrl_ops, s,
-                          "vga-mm-ctrl", 0x100000);
-    memory_region_set_flush_coalesced(s_ioport_ctrl);
-
-    vga_io_memory = g_malloc(sizeof(*vga_io_memory));
-    /* XXX: endianness? */
-    memory_region_init_io(vga_io_memory, NULL, &vga_mem_ops, &s->vga,
-                          "vga-mem", 0x20000);
-
-    vmstate_register(NULL, 0, &vmstate_vga_common, s);
-
-    memory_region_add_subregion(address_space, ctrl_base, s_ioport_ctrl);
-    s->vga.bank_offset = 0;
-    memory_region_add_subregion(address_space,
-                                vram_base + 0x000a0000, vga_io_memory);
-    memory_region_set_coalescing(vga_io_memory);
-
-    s->vga.con = graphic_console_init(NULL, 0, s->vga.hw_ops, s);
-
-    memory_region_add_subregion(address_space,
-                                VBE_DISPI_LFB_PHYSICAL_ADDRESS,
-                                &s->vga.vram);
+    sysbus_mmio_map(s, 0, ctrl_base);
+    sysbus_mmio_map(s, 1, vram_base + 0x000a0000);
+    sysbus_mmio_map(s, 2, VBE_DISPI_LFB_PHYSICAL_ADDRESS);
 
     return 0;
 }
+
+static void vga_mmio_realizefn(DeviceState *dev, Error **errp)
+{
+    VGAMmioState *s = VGA_MMIO(dev);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+
+    memory_region_init_io(&s->iomem, OBJECT(dev), &vga_mm_ctrl_ops, s,
+                          "vga-mmio", 0x100000);
+    memory_region_set_flush_coalesced(&s->iomem);
+    sysbus_init_mmio(sbd, &s->iomem);
+
+    /* XXX: endianness? */
+    memory_region_init_io(&s->lowmem, OBJECT(dev), &vga_mem_ops, &s->vga,
+                          "vga-lowmem", 0x20000);
+    memory_region_set_coalescing(&s->lowmem);
+    sysbus_init_mmio(sbd, &s->lowmem);
+
+    s->vga.bank_offset = 0;
+    s->vga.global_vmstate = true;
+    vga_common_init(&s->vga, OBJECT(dev));
+    sysbus_init_mmio(sbd, &s->vga.vram);
+    s->vga.con = graphic_console_init(dev, 0, s->vga.hw_ops, &s->vga);
+}
+
+static Property vga_mmio_properties[] = {
+    DEFINE_PROP_UINT8("it_shift", VGAMmioState, it_shift, 0),
+    DEFINE_PROP_UINT32("vgamem_mb", VGAMmioState, vga.vram_size_mb, 8),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void vga_mmio_class_initfn(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = vga_mmio_realizefn;
+    dc->reset = vga_mmio_reset;
+    dc->vmsd = &vmstate_vga_common;
+    device_class_set_props(dc, vga_mmio_properties);
+    set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
+}
+
+static const TypeInfo vga_mmio_info = {
+    .name          = TYPE_VGA_MMIO,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(VGAMmioState),
+    .class_init    = vga_mmio_class_initfn,
+};
+
+static void vga_mmio_register_types(void)
+{
+    type_register_static(&vga_mmio_info);
+}
+
+type_init(vga_mmio_register_types)
-- 
2.33.1

[PATCH v2 4/4] hw/mips/jazz: Inline vga_mmio_init() and remove it

[Philippe Mathieu-Daudé]

vga_mmio_init() is used only one time and not very helpful,
inline and remove it.

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/display/vga.h |  5 -----
 hw/display/vga-mmio.c    | 19 -------------------
 hw/mips/jazz.c           |  9 ++++++++-
 3 files changed, 8 insertions(+), 25 deletions(-)

diff --git a/include/hw/display/vga.h b/include/hw/display/vga.h
index 98b2e560f9b..a79aa2909b2 100644
--- a/include/hw/display/vga.h
+++ b/include/hw/display/vga.h
@@ -9,8 +9,6 @@
 #ifndef QEMU_HW_DISPLAY_VGA_H
 #define QEMU_HW_DISPLAY_VGA_H
 
-#include "exec/hwaddr.h"
-
 /*
  * modules can reference this symbol to avoid being loaded
  * into system emulators without vga support
@@ -26,7 +24,4 @@ extern enum vga_retrace_method vga_retrace_method;
 
 #define TYPE_VGA_MMIO "vga-mmio"
 
-int vga_mmio_init(hwaddr vram_base, hwaddr ctrl_base,
-                  int it_shift, MemoryRegion *address_space);
-
 #endif
diff --git a/hw/display/vga-mmio.c b/hw/display/vga-mmio.c
index 10bde32af5c..49693680813 100644
--- a/hw/display/vga-mmio.c
+++ b/hw/display/vga-mmio.c
@@ -24,7 +24,6 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
-#include "hw/display/vga.h"
 #include "hw/sysbus.h"
 #include "hw/display/vga.h"
 #include "hw/qdev-properties.h"
@@ -85,24 +84,6 @@ static void vga_mmio_reset(DeviceState *dev)
     vga_common_reset(&s->vga);
 }
 
-int vga_mmio_init(hwaddr vram_base, hwaddr ctrl_base,
-                  int it_shift, MemoryRegion *address_space)
-{
-    DeviceState *dev;
-    SysBusDevice *s;
-
-    dev = qdev_new(TYPE_VGA_MMIO);
-    qdev_prop_set_uint8(dev, "it_shift", it_shift);
-    s = SYS_BUS_DEVICE(dev);
-    sysbus_realize_and_unref(s, &error_fatal);
-
-    sysbus_mmio_map(s, 0, ctrl_base);
-    sysbus_mmio_map(s, 1, vram_base + 0x000a0000);
-    sysbus_mmio_map(s, 2, VBE_DISPI_LFB_PHYSICAL_ADDRESS);
-
-    return 0;
-}
-
 static void vga_mmio_realizefn(DeviceState *dev, Error **errp)
 {
     VGAMmioState *s = VGA_MMIO(dev);
diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
index 8f345afd137..44f0d48bfd7 100644
--- a/hw/mips/jazz.c
+++ b/hw/mips/jazz.c
@@ -43,6 +43,7 @@
 #include "hw/rtc/mc146818rtc.h"
 #include "hw/timer/i8254.h"
 #include "hw/display/vga.h"
+#include "hw/display/bochs-vbe.h"
 #include "hw/audio/pcspk.h"
 #include "hw/input/i8042.h"
 #include "hw/sysbus.h"
@@ -274,7 +275,13 @@ static void mips_jazz_init(MachineState *machine,
         }
         break;
     case JAZZ_PICA61:
-        vga_mmio_init(0x40000000, 0x60000000, 0, get_system_memory());
+        dev = qdev_new(TYPE_VGA_MMIO);
+        qdev_prop_set_uint8(dev, "it_shift", 0);
+        sysbus = SYS_BUS_DEVICE(dev);
+        sysbus_realize_and_unref(sysbus, &error_fatal);
+        sysbus_mmio_map(sysbus, 0, 0x60000000);
+        sysbus_mmio_map(sysbus, 1, 0x400a0000);
+        sysbus_mmio_map(sysbus, 2, VBE_DISPI_LFB_PHYSICAL_ADDRESS);
         break;
     default:
         break;
-- 
2.33.1

Re: [PATCH v2 0/4] hw/display: QOM'ify vga_mmio_init() as TYPE_VGA_MMIO

[Philippe Mathieu-Daudé]

On 12/6/21 23:45, Philippe Mathieu-Daudé wrote:
> Convert the open-coded vga_mmio_init() to a sysbus device.

> Philippe Mathieu-Daudé (4):
>   hw/display: Rename VGA_ISA_MM -> VGA_MMIO
>   hw/display/vga-mmio: Inline vga_mm_init()
>   hw/display/vga-mmio: QOM'ify vga_mmio_init() as TYPE_VGA_MMIO
>   hw/mips/jazz: Inline vga_mmio_init() and remove it

Queued to mips-next.

Re: [PATCH v2 4/4] hw/mips/jazz: Inline vga_mmio_init() and remove it

[Thomas Huth]

On 06/12/2021 23.45, Philippe Mathieu-Daudé wrote:
> vga_mmio_init() is used only one time and not very helpful,
> inline and remove it.
> 
> Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   include/hw/display/vga.h |  5 -----
>   hw/display/vga-mmio.c    | 19 -------------------
>   hw/mips/jazz.c           |  9 ++++++++-
>   3 files changed, 8 insertions(+), 25 deletions(-)

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

Re: [PATCH v2 3/4] hw/display/vga-mmio: QOM'ify vga_mmio_init() as TYPE_VGA_MMIO

[Thomas Huth]

On 06/12/2021 23.45, Philippe Mathieu-Daudé wrote:
> Introduce TYPE_VGA_MMIO, a sysbus device.
> 
> While there is no change in the vga_mmio_init()
> interface, this is a migration compatibility break
> of the MIPS Acer Pica 61 Jazz machine (pica61).

It's unfortunate, but as far as I know, it would be pretty difficult or even 
impossible to get this done without versioned machine types? So IMHO it's ok 
to break this in this case here.

> Suggested-by: Thomas Huth <thuth@redhat.com>
> Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   include/hw/display/vga.h |   2 +
>   hw/display/vga-mmio.c    | 132 +++++++++++++++++++++++++++------------
>   2 files changed, 94 insertions(+), 40 deletions(-)
> 
> diff --git a/include/hw/display/vga.h b/include/hw/display/vga.h
> index c16a5c26dae..98b2e560f9b 100644
> --- a/include/hw/display/vga.h
> +++ b/include/hw/display/vga.h
> @@ -24,6 +24,8 @@ enum vga_retrace_method {
>   
>   extern enum vga_retrace_method vga_retrace_method;
>   
> +#define TYPE_VGA_MMIO "vga-mmio"
> +
>   int vga_mmio_init(hwaddr vram_base, hwaddr ctrl_base,
>                     int it_shift, MemoryRegion *address_space);
>   
> diff --git a/hw/display/vga-mmio.c b/hw/display/vga-mmio.c
> index 5671fdb920f..10bde32af5c 100644
> --- a/hw/display/vga-mmio.c
> +++ b/hw/display/vga-mmio.c
> @@ -23,21 +23,34 @@
>    */
>   
>   #include "qemu/osdep.h"
> -#include "qemu/bitops.h"
> -#include "qemu/units.h"
> -#include "migration/vmstate.h"
> +#include "qapi/error.h"
>   #include "hw/display/vga.h"
> +#include "hw/sysbus.h"
> +#include "hw/display/vga.h"
> +#include "hw/qdev-properties.h"
>   #include "vga_int.h"
> -#include "ui/pixel_ops.h"
>   
> -#define VGA_RAM_SIZE (8 * MiB)
> +/*
> + * QEMU interface:
> + *  + sysbus MMIO region 0: VGA I/O registers
> + *  + sysbus MMIO region 1: VGA MMIO registers
> + *  + sysbus MMIO region 2: VGA memory
> + */
>   
> -typedef struct VGAMmioState {
> +OBJECT_DECLARE_SIMPLE_TYPE(VGAMmioState, VGA_MMIO)
> +
> +struct VGAMmioState {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +
> +    /*< public >*/
>       VGACommonState vga;
> -    int it_shift;
> -} VGAMmioState;
> +    MemoryRegion iomem;
> +    MemoryRegion lowmem;
> +
> +    uint8_t it_shift;
> +};
>   
> -/* Memory mapped interface */
>   static uint64_t vga_mm_read(void *opaque, hwaddr addr, unsigned size)
>   {
>       VGAMmioState *s = opaque;
> @@ -65,42 +78,81 @@ static const MemoryRegionOps vga_mm_ctrl_ops = {
>       .endianness = DEVICE_NATIVE_ENDIAN,
>   };
>   
> +static void vga_mmio_reset(DeviceState *dev)
> +{
> +    VGAMmioState *s = VGA_MMIO(dev);
> +
> +    vga_common_reset(&s->vga);
> +}
> +
>   int vga_mmio_init(hwaddr vram_base, hwaddr ctrl_base,
>                     int it_shift, MemoryRegion *address_space)
>   {
> -    VGAMmioState *s;
> -    MemoryRegion *s_ioport_ctrl, *vga_io_memory;
> +    DeviceState *dev;
> +    SysBusDevice *s;
>   
> -    s = g_malloc0(sizeof(*s));
> +    dev = qdev_new(TYPE_VGA_MMIO);
> +    qdev_prop_set_uint8(dev, "it_shift", it_shift);
> +    s = SYS_BUS_DEVICE(dev);
> +    sysbus_realize_and_unref(s, &error_fatal);
>   
> -    s->vga.vram_size_mb = VGA_RAM_SIZE / MiB;
> -    s->vga.global_vmstate = true;
> -    vga_common_init(&s->vga, NULL);
> -
> -    s->it_shift = it_shift;
> -    s_ioport_ctrl = g_malloc(sizeof(*s_ioport_ctrl));
> -    memory_region_init_io(s_ioport_ctrl, NULL, &vga_mm_ctrl_ops, s,
> -                          "vga-mm-ctrl", 0x100000);
> -    memory_region_set_flush_coalesced(s_ioport_ctrl);
> -
> -    vga_io_memory = g_malloc(sizeof(*vga_io_memory));
> -    /* XXX: endianness? */
> -    memory_region_init_io(vga_io_memory, NULL, &vga_mem_ops, &s->vga,
> -                          "vga-mem", 0x20000);
> -
> -    vmstate_register(NULL, 0, &vmstate_vga_common, s);
> -
> -    memory_region_add_subregion(address_space, ctrl_base, s_ioport_ctrl);
> -    s->vga.bank_offset = 0;
> -    memory_region_add_subregion(address_space,
> -                                vram_base + 0x000a0000, vga_io_memory);
> -    memory_region_set_coalescing(vga_io_memory);
> -
> -    s->vga.con = graphic_console_init(NULL, 0, s->vga.hw_ops, s);
> -
> -    memory_region_add_subregion(address_space,
> -                                VBE_DISPI_LFB_PHYSICAL_ADDRESS,
> -                                &s->vga.vram);
> +    sysbus_mmio_map(s, 0, ctrl_base);
> +    sysbus_mmio_map(s, 1, vram_base + 0x000a0000);
> +    sysbus_mmio_map(s, 2, VBE_DISPI_LFB_PHYSICAL_ADDRESS);
>   
>       return 0;
>   }
> +
> +static void vga_mmio_realizefn(DeviceState *dev, Error **errp)
> +{
> +    VGAMmioState *s = VGA_MMIO(dev);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +
> +    memory_region_init_io(&s->iomem, OBJECT(dev), &vga_mm_ctrl_ops, s,
> +                          "vga-mmio", 0x100000);
> +    memory_region_set_flush_coalesced(&s->iomem);
> +    sysbus_init_mmio(sbd, &s->iomem);
> +
> +    /* XXX: endianness? */
> +    memory_region_init_io(&s->lowmem, OBJECT(dev), &vga_mem_ops, &s->vga,
> +                          "vga-lowmem", 0x20000);
> +    memory_region_set_coalescing(&s->lowmem);
> +    sysbus_init_mmio(sbd, &s->lowmem);
> +
> +    s->vga.bank_offset = 0;
> +    s->vga.global_vmstate = true;
> +    vga_common_init(&s->vga, OBJECT(dev));
> +    sysbus_init_mmio(sbd, &s->vga.vram);
> +    s->vga.con = graphic_console_init(dev, 0, s->vga.hw_ops, &s->vga);
> +}
> +
> +static Property vga_mmio_properties[] = {
> +    DEFINE_PROP_UINT8("it_shift", VGAMmioState, it_shift, 0),
> +    DEFINE_PROP_UINT32("vgamem_mb", VGAMmioState, vga.vram_size_mb, 8),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void vga_mmio_class_initfn(ObjectClass *klass, void *data)

Just a matter of taste, but I'd prefer "oc" instead of "klass".

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

Re: [PATCH v2 3/4] hw/display/vga-mmio: QOM'ify vga_mmio_init() as TYPE_VGA_MMIO

[Philippe Mathieu-Daudé]

On 12/13/21 11:48, Thomas Huth wrote:
> On 06/12/2021 23.45, Philippe Mathieu-Daudé wrote:
>> Introduce TYPE_VGA_MMIO, a sysbus device.
>>
>> While there is no change in the vga_mmio_init()
>> interface, this is a migration compatibility break
>> of the MIPS Acer Pica 61 Jazz machine (pica61).
> 
> It's unfortunate, but as far as I know, it would be pretty difficult or
> even impossible to get this done without versioned machine types? So
> IMHO it's ok to break this in this case here.

Hervé and Zoltan were already Cc'ed, adding Mark and Finn;
I am not sure who else would be annoyed by that change.

>> Suggested-by: Thomas Huth <thuth@redhat.com>
>> Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   include/hw/display/vga.h |   2 +
>>   hw/display/vga-mmio.c    | 132 +++++++++++++++++++++++++++------------
>>   2 files changed, 94 insertions(+), 40 deletions(-)
>>
>> diff --git a/include/hw/display/vga.h b/include/hw/display/vga.h
>> index c16a5c26dae..98b2e560f9b 100644
>> --- a/include/hw/display/vga.h
>> +++ b/include/hw/display/vga.h
>> @@ -24,6 +24,8 @@ enum vga_retrace_method {
>>     extern enum vga_retrace_method vga_retrace_method;
>>   +#define TYPE_VGA_MMIO "vga-mmio"
>> +
>>   int vga_mmio_init(hwaddr vram_base, hwaddr ctrl_base,
>>                     int it_shift, MemoryRegion *address_space);
>>   diff --git a/hw/display/vga-mmio.c b/hw/display/vga-mmio.c
>> index 5671fdb920f..10bde32af5c 100644
>> --- a/hw/display/vga-mmio.c
>> +++ b/hw/display/vga-mmio.c
>> @@ -23,21 +23,34 @@
>>    */
>>     #include "qemu/osdep.h"
>> -#include "qemu/bitops.h"
>> -#include "qemu/units.h"
>> -#include "migration/vmstate.h"
>> +#include "qapi/error.h"
>>   #include "hw/display/vga.h"
>> +#include "hw/sysbus.h"
>> +#include "hw/display/vga.h"
>> +#include "hw/qdev-properties.h"
>>   #include "vga_int.h"
>> -#include "ui/pixel_ops.h"
>>   -#define VGA_RAM_SIZE (8 * MiB)
>> +/*
>> + * QEMU interface:
>> + *  + sysbus MMIO region 0: VGA I/O registers
>> + *  + sysbus MMIO region 1: VGA MMIO registers
>> + *  + sysbus MMIO region 2: VGA memory
>> + */
>>   -typedef struct VGAMmioState {
>> +OBJECT_DECLARE_SIMPLE_TYPE(VGAMmioState, VGA_MMIO)
>> +
>> +struct VGAMmioState {
>> +    /*< private >*/
>> +    SysBusDevice parent_obj;
>> +
>> +    /*< public >*/
>>       VGACommonState vga;
>> -    int it_shift;
>> -} VGAMmioState;
>> +    MemoryRegion iomem;
>> +    MemoryRegion lowmem;
>> +
>> +    uint8_t it_shift;
>> +};
>>   -/* Memory mapped interface */
>>   static uint64_t vga_mm_read(void *opaque, hwaddr addr, unsigned size)
>>   {
>>       VGAMmioState *s = opaque;
>> @@ -65,42 +78,81 @@ static const MemoryRegionOps vga_mm_ctrl_ops = {
>>       .endianness = DEVICE_NATIVE_ENDIAN,
>>   };
>>   +static void vga_mmio_reset(DeviceState *dev)
>> +{
>> +    VGAMmioState *s = VGA_MMIO(dev);
>> +
>> +    vga_common_reset(&s->vga);
>> +}
>> +
>>   int vga_mmio_init(hwaddr vram_base, hwaddr ctrl_base,
>>                     int it_shift, MemoryRegion *address_space)
>>   {
>> -    VGAMmioState *s;
>> -    MemoryRegion *s_ioport_ctrl, *vga_io_memory;
>> +    DeviceState *dev;
>> +    SysBusDevice *s;
>>   -    s = g_malloc0(sizeof(*s));
>> +    dev = qdev_new(TYPE_VGA_MMIO);
>> +    qdev_prop_set_uint8(dev, "it_shift", it_shift);
>> +    s = SYS_BUS_DEVICE(dev);
>> +    sysbus_realize_and_unref(s, &error_fatal);
>>   -    s->vga.vram_size_mb = VGA_RAM_SIZE / MiB;
>> -    s->vga.global_vmstate = true;
>> -    vga_common_init(&s->vga, NULL);
>> -
>> -    s->it_shift = it_shift;
>> -    s_ioport_ctrl = g_malloc(sizeof(*s_ioport_ctrl));
>> -    memory_region_init_io(s_ioport_ctrl, NULL, &vga_mm_ctrl_ops, s,
>> -                          "vga-mm-ctrl", 0x100000);
>> -    memory_region_set_flush_coalesced(s_ioport_ctrl);
>> -
>> -    vga_io_memory = g_malloc(sizeof(*vga_io_memory));
>> -    /* XXX: endianness? */
>> -    memory_region_init_io(vga_io_memory, NULL, &vga_mem_ops, &s->vga,
>> -                          "vga-mem", 0x20000);
>> -
>> -    vmstate_register(NULL, 0, &vmstate_vga_common, s);
>> -
>> -    memory_region_add_subregion(address_space, ctrl_base,
>> s_ioport_ctrl);
>> -    s->vga.bank_offset = 0;
>> -    memory_region_add_subregion(address_space,
>> -                                vram_base + 0x000a0000, vga_io_memory);
>> -    memory_region_set_coalescing(vga_io_memory);
>> -
>> -    s->vga.con = graphic_console_init(NULL, 0, s->vga.hw_ops, s);
>> -
>> -    memory_region_add_subregion(address_space,
>> -                                VBE_DISPI_LFB_PHYSICAL_ADDRESS,
>> -                                &s->vga.vram);
>> +    sysbus_mmio_map(s, 0, ctrl_base);
>> +    sysbus_mmio_map(s, 1, vram_base + 0x000a0000);
>> +    sysbus_mmio_map(s, 2, VBE_DISPI_LFB_PHYSICAL_ADDRESS);
>>         return 0;
>>   }
>> +
>> +static void vga_mmio_realizefn(DeviceState *dev, Error **errp)
>> +{
>> +    VGAMmioState *s = VGA_MMIO(dev);
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>> +
>> +    memory_region_init_io(&s->iomem, OBJECT(dev), &vga_mm_ctrl_ops, s,
>> +                          "vga-mmio", 0x100000);
>> +    memory_region_set_flush_coalesced(&s->iomem);
>> +    sysbus_init_mmio(sbd, &s->iomem);
>> +
>> +    /* XXX: endianness? */
>> +    memory_region_init_io(&s->lowmem, OBJECT(dev), &vga_mem_ops,
>> &s->vga,
>> +                          "vga-lowmem", 0x20000);
>> +    memory_region_set_coalescing(&s->lowmem);
>> +    sysbus_init_mmio(sbd, &s->lowmem);
>> +
>> +    s->vga.bank_offset = 0;
>> +    s->vga.global_vmstate = true;
>> +    vga_common_init(&s->vga, OBJECT(dev));
>> +    sysbus_init_mmio(sbd, &s->vga.vram);
>> +    s->vga.con = graphic_console_init(dev, 0, s->vga.hw_ops, &s->vga);
>> +}
>> +
>> +static Property vga_mmio_properties[] = {
>> +    DEFINE_PROP_UINT8("it_shift", VGAMmioState, it_shift, 0),
>> +    DEFINE_PROP_UINT32("vgamem_mb", VGAMmioState, vga.vram_size_mb, 8),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void vga_mmio_class_initfn(ObjectClass *klass, void *data)
> 
> Just a matter of taste, but I'd prefer "oc" instead of "klass".
> 
> Anyway:
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> 

Re: [PATCH v2 3/4] hw/display/vga-mmio: QOM'ify vga_mmio_init() as TYPE_VGA_MMIO

[Mark Cave-Ayland]

On 13/12/2021 11:05, Philippe Mathieu-Daudé wrote:

> On 12/13/21 11:48, Thomas Huth wrote:
>> On 06/12/2021 23.45, Philippe Mathieu-Daudé wrote:
>>> Introduce TYPE_VGA_MMIO, a sysbus device.
>>>
>>> While there is no change in the vga_mmio_init()
>>> interface, this is a migration compatibility break
>>> of the MIPS Acer Pica 61 Jazz machine (pica61).
>>
>> It's unfortunate, but as far as I know, it would be pretty difficult or
>> even impossible to get this done without versioned machine types? So
>> IMHO it's ok to break this in this case here.
> 
> Hervé and Zoltan were already Cc'ed, adding Mark and Finn;
> I am not sure who else would be annoyed by that change.

No issues from this end. I've done a few bits and pieces on the 40p machine when 
testing OpenBIOS, but that won't be affected by this change.


ATB,

Mark.

Re: [PATCH v2 3/4] hw/display/vga-mmio: QOM'ify vga_mmio_init() as TYPE_VGA_MMIO

[Juan Quintela]

Thomas Huth <thuth@redhat.com> wrote:
> On 06/12/2021 23.45, Philippe Mathieu-Daudé wrote:
>> Introduce TYPE_VGA_MMIO, a sysbus device.
>> While there is no change in the vga_mmio_init()
>> interface, this is a migration compatibility break
>> of the MIPS Acer Pica 61 Jazz machine (pica61).
>
> It's unfortunate, but as far as I know, it would be pretty difficult
> or even impossible to get this done without versioned machine types?
> So IMHO it's ok to break this in this case here.

Hi

My understanding is that outside of x86*, and now ppc, arm and s390,
no one else really cares about migration compatibility.  I am not even
sure if they really care about migration at all O:-)

So, if the code is better for other reasons, I will not wonder about
migration compatibility.

Later, Juan.

Re: [PATCH v2 3/4] hw/display/vga-mmio: QOM'ify vga_mmio_init() as TYPE_VGA_MMIO

[Daniel P. Berrangé]

On Tue, Dec 14, 2021 at 01:05:29PM +0100, Juan Quintela wrote:
> Thomas Huth <thuth@redhat.com> wrote:
> > On 06/12/2021 23.45, Philippe Mathieu-Daudé wrote:
> >> Introduce TYPE_VGA_MMIO, a sysbus device.
> >> While there is no change in the vga_mmio_init()
> >> interface, this is a migration compatibility break
> >> of the MIPS Acer Pica 61 Jazz machine (pica61).
> >
> > It's unfortunate, but as far as I know, it would be pretty difficult
> > or even impossible to get this done without versioned machine types?
> > So IMHO it's ok to break this in this case here.
> 
> Hi
> 
> My understanding is that outside of x86*, and now ppc, arm and s390,
> no one else really cares about migration compatibility.  I am not even
> sure if they really care about migration at all O:-)
>
> So, if the code is better for other reasons, I will not wonder about
> migration compatibility.

Essentially if it has versioned machine types, then migration ABI
compat is mandatory. If it doesn't have versioned machine types
then migration API compat explicitly doesn't exist.

There are no versioned machine types for MIPS, so migration compat
is a non-issue.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 3/4] hw/display/vga-mmio: QOM'ify vga_mmio_init() as TYPE_VGA_MMIO

[Philippe Mathieu-Daudé]

On 12/14/21 13:52, Daniel P. Berrangé wrote:
> On Tue, Dec 14, 2021 at 01:05:29PM +0100, Juan Quintela wrote:
>> Thomas Huth <thuth@redhat.com> wrote:
>>> On 06/12/2021 23.45, Philippe Mathieu-Daudé wrote:
>>>> Introduce TYPE_VGA_MMIO, a sysbus device.
>>>> While there is no change in the vga_mmio_init()
>>>> interface, this is a migration compatibility break
>>>> of the MIPS Acer Pica 61 Jazz machine (pica61).
>>>
>>> It's unfortunate, but as far as I know, it would be pretty difficult
>>> or even impossible to get this done without versioned machine types?
>>> So IMHO it's ok to break this in this case here.
>>
>> Hi
>>
>> My understanding is that outside of x86*, and now ppc, arm and s390,
>> no one else really cares about migration compatibility.  I am not even
>> sure if they really care about migration at all O:-)
>>
>> So, if the code is better for other reasons, I will not wonder about
>> migration compatibility.
> 
> Essentially if it has versioned machine types, then migration ABI
> compat is mandatory. If it doesn't have versioned machine types
> then migration API compat explicitly doesn't exist.
> 
> There are no versioned machine types for MIPS, so migration compat
> is a non-issue.

Thank you, this is a clear explanation worth being added on
docs/devel/migration.rst FAQ ("General advice for device developers"
section).