[Qemu-devel] [PATCH 0/4] cg3/tcx cleanups

[Qemu-devel] [PATCH 0/4] cg3/tcx cleanups

[Mark Cave-Ayland]

This patchset primarily addresses Andreas' comments with regard to cg3 init/realize. The
first patch addresses this, followed by a second patch to address a previous comment made
by Paolo.

The last two patches then alter tcx in a similar fashion to bring it in line with the 
cg3 changes.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


Mark Cave-Ayland (4):
  cg3: move initialisation from realizefn to initfn
  cg3: add extra check to prevent CG3 register array overflow
  tcx: move initialisation from SysBusDevice class to TCX class
    realizefn
  tcx: move initialisation from realizefn to initfn

 hw/display/cg3.c |   27 ++++++++++++++---------
 hw/display/tcx.c |   64 ++++++++++++++++++++++++++++++------------------------
 hw/sparc/sun4m.c |   10 ++++-----
 3 files changed, 58 insertions(+), 43 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/4] cg3: move initialisation from realizefn to initfn

[Mark Cave-Ayland]

Initialisation cleanup as suggested by Andreas.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
CC: Andreas Färber <afaerber@suse.de>
---
 hw/display/cg3.c |   23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/hw/display/cg3.c b/hw/display/cg3.c
index f5a8299..cd9297d 100644
--- a/hw/display/cg3.c
+++ b/hw/display/cg3.c
@@ -274,6 +274,20 @@ static const GraphicHwOps cg3_ops = {
     .gfx_update = cg3_update_display,
 };
 
+static void cg3_initfn(Object *obj)
+{
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+    CG3State *s = CG3(obj);
+
+    memory_region_init_ram(&s->rom, NULL, "cg3.prom", FCODE_MAX_ROM_SIZE);
+    memory_region_set_readonly(&s->rom, true);
+    sysbus_init_mmio(sbd, &s->rom);
+
+    memory_region_init_io(&s->reg, NULL, &cg3_reg_ops, s, "cg3.reg",
+                          CG3_REG_SIZE);
+    sysbus_init_mmio(sbd, &s->reg);
+}
+
 static void cg3_realizefn(DeviceState *dev, Error **errp)
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
@@ -282,11 +296,7 @@ static void cg3_realizefn(DeviceState *dev, Error **errp)
     char *fcode_filename;
 
     /* FCode ROM */
-    memory_region_init_ram(&s->rom, NULL, "cg3.prom", FCODE_MAX_ROM_SIZE);
     vmstate_register_ram_global(&s->rom);
-    memory_region_set_readonly(&s->rom, true);
-    sysbus_init_mmio(sbd, &s->rom);
-
     fcode_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, CG3_ROM_FILE);
     if (fcode_filename) {
         ret = load_image_targphys(fcode_filename, s->prom_addr,
@@ -296,10 +306,6 @@ static void cg3_realizefn(DeviceState *dev, Error **errp)
         }
     }
 
-    memory_region_init_io(&s->reg, NULL, &cg3_reg_ops, s, "cg3.reg",
-                          CG3_REG_SIZE);
-    sysbus_init_mmio(sbd, &s->reg);
-
     memory_region_init_ram(&s->vram_mem, NULL, "cg3.vram", s->vram_size);
     vmstate_register_ram_global(&s->vram_mem);
     sysbus_init_mmio(sbd, &s->vram_mem);
@@ -374,6 +380,7 @@ static const TypeInfo cg3_info = {
     .name          = TYPE_CG3,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(CG3State),
+    .instance_init = cg3_initfn,
     .class_init    = cg3_class_init,
 };
 
-- 
1.7.10.4

[Qemu-devel] [PATCH 2/4] cg3: add extra check to prevent CG3 register array overflow

[Mark Cave-Ayland]

The case statements in the CG3 read and write register routines have a maximum
value of CG3_REG_SIZE, so if a value were written to this offset then it
would overflow the register array.

Currently this cannot be exploited since the MemoryRegion restricts accesses
to the range 0 ... CG3_REG_SIZE - 1, but it seems worth clarifying this for
future review and/or static analysis.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
CC: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/display/cg3.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/display/cg3.c b/hw/display/cg3.c
index cd9297d..65ef7a7 100644
--- a/hw/display/cg3.c
+++ b/hw/display/cg3.c
@@ -177,7 +177,7 @@ static uint64_t cg3_reg_read(void *opaque, hwaddr addr, unsigned size)
         /* monitor ID 6, board type = 1 (color) */
         val = s->regs[1] | CG3_SR_1152_900_76_B | CG3_SR_ID_COLOR;
         break;
-    case CG3_REG_FBC_CURSTART ... CG3_REG_SIZE:
+    case CG3_REG_FBC_CURSTART ... CG3_REG_SIZE - 1:
         val = s->regs[addr - 0x10];
         break;
     default:
@@ -247,7 +247,7 @@ static void cg3_reg_write(void *opaque, hwaddr addr, uint64_t val,
             qemu_irq_lower(s->irq);
         }
         break;
-    case CG3_REG_FBC_CURSTART ... CG3_REG_SIZE:
+    case CG3_REG_FBC_CURSTART ... CG3_REG_SIZE - 1:
         s->regs[addr - 0x10] = val;
         break;
     default:
-- 
1.7.10.4

[Qemu-devel] [PATCH 3/4] tcx: move initialisation from SysBusDevice class to TCX class realizefn

[Mark Cave-Ayland]

This is an intermediate step to bring TCX in line with CG3.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
CC: Andreas Färber <afaerber@suse.de>
---
 hw/display/tcx.c |   26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index 2551b67..8fc4e38 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -530,8 +530,9 @@ static const GraphicHwOps tcx24_ops = {
     .gfx_update = tcx24_update_display,
 };
 
-static int tcx_init1(SysBusDevice *dev)
+static void tcx_realizefn(DeviceState *dev, Error **errp)
 {
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     TCXState *s = TCX(dev);
     ram_addr_t vram_offset = 0;
     int size, ret;
@@ -547,15 +548,14 @@ static int tcx_init1(SysBusDevice *dev)
     memory_region_init_ram(&s->rom, NULL, "tcx.prom", FCODE_MAX_ROM_SIZE);
     vmstate_register_ram_global(&s->rom);
     memory_region_set_readonly(&s->rom, true);
-    sysbus_init_mmio(dev, &s->rom);
+    sysbus_init_mmio(sbd, &s->rom);
 
     fcode_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, TCX_ROM_FILE);
     if (fcode_filename) {
         ret = load_image_targphys(fcode_filename, s->prom_addr,
                                   FCODE_MAX_ROM_SIZE);
         if (ret < 0 || ret > FCODE_MAX_ROM_SIZE) {
-            fprintf(stderr, "tcx: could not load prom '%s'\n", TCX_ROM_FILE);
-            return -1;
+            error_report("tcx: could not load prom '%s'", TCX_ROM_FILE);
         }
     }
 
@@ -564,23 +564,23 @@ static int tcx_init1(SysBusDevice *dev)
     size = s->vram_size;
     memory_region_init_alias(&s->vram_8bit, OBJECT(s), "tcx.vram.8bit",
                              &s->vram_mem, vram_offset, size);
-    sysbus_init_mmio(dev, &s->vram_8bit);
+    sysbus_init_mmio(sbd, &s->vram_8bit);
     vram_offset += size;
     vram_base += size;
 
     /* DAC */
     memory_region_init_io(&s->dac, OBJECT(s), &tcx_dac_ops, s,
                           "tcx.dac", TCX_DAC_NREGS);
-    sysbus_init_mmio(dev, &s->dac);
+    sysbus_init_mmio(sbd, &s->dac);
 
     /* TEC (dummy) */
     memory_region_init_io(&s->tec, OBJECT(s), &dummy_ops, s,
                           "tcx.tec", TCX_TEC_NREGS);
-    sysbus_init_mmio(dev, &s->tec);
+    sysbus_init_mmio(sbd, &s->tec);
     /* THC: NetBSD writes here even with 8-bit display: dummy */
     memory_region_init_io(&s->thc24, OBJECT(s), &dummy_ops, s, "tcx.thc24",
                           TCX_THC_NREGS_24);
-    sysbus_init_mmio(dev, &s->thc24);
+    sysbus_init_mmio(sbd, &s->thc24);
 
     if (s->depth == 24) {
         /* 24-bit plane */
@@ -589,7 +589,7 @@ static int tcx_init1(SysBusDevice *dev)
         s->vram24_offset = vram_offset;
         memory_region_init_alias(&s->vram_24bit, OBJECT(s), "tcx.vram.24bit",
                                  &s->vram_mem, vram_offset, size);
-        sysbus_init_mmio(dev, &s->vram_24bit);
+        sysbus_init_mmio(sbd, &s->vram_24bit);
         vram_offset += size;
         vram_base += size;
 
@@ -599,20 +599,19 @@ static int tcx_init1(SysBusDevice *dev)
         s->cplane_offset = vram_offset;
         memory_region_init_alias(&s->vram_cplane, OBJECT(s), "tcx.vram.cplane",
                                  &s->vram_mem, vram_offset, size);
-        sysbus_init_mmio(dev, &s->vram_cplane);
+        sysbus_init_mmio(sbd, &s->vram_cplane);
 
         s->con = graphic_console_init(DEVICE(dev), 0, &tcx24_ops, s);
     } else {
         /* THC 8 bit (dummy) */
         memory_region_init_io(&s->thc8, OBJECT(s), &dummy_ops, s, "tcx.thc8",
                               TCX_THC_NREGS_8);
-        sysbus_init_mmio(dev, &s->thc8);
+        sysbus_init_mmio(sbd, &s->thc8);
 
         s->con = graphic_console_init(DEVICE(dev), 0, &tcx_ops, s);
     }
 
     qemu_console_resize(s->con, s->width, s->height);
-    return 0;
 }
 
 static Property tcx_properties[] = {
@@ -627,9 +626,8 @@ static Property tcx_properties[] = {
 static void tcx_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
-    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
 
-    k->init = tcx_init1;
+    dc->realize = tcx_realizefn;
     dc->reset = tcx_reset;
     dc->vmsd = &vmstate_tcx;
     dc->props = tcx_properties;
-- 
1.7.10.4

[Qemu-devel] [PATCH 4/4] tcx: move initialisation from realizefn to initfn

[Mark Cave-Ayland]

Initialisation cleanup as suggested by Andreas.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
CC: Andreas Färber <afaerber@suse.de>
---
 hw/display/tcx.c |   46 ++++++++++++++++++++++++++++------------------
 hw/sparc/sun4m.c |   10 +++++-----
 2 files changed, 33 insertions(+), 23 deletions(-)

diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index 8fc4e38..28c742c 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -530,6 +530,33 @@ static const GraphicHwOps tcx24_ops = {
     .gfx_update = tcx24_update_display,
 };
 
+static void tcx_initfn(Object *obj)
+{
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+    TCXState *s = TCX(obj);
+
+    memory_region_init_ram(&s->rom, NULL, "tcx.prom", FCODE_MAX_ROM_SIZE);
+    memory_region_set_readonly(&s->rom, true);
+    sysbus_init_mmio(sbd, &s->rom);
+
+    /* DAC */
+    memory_region_init_io(&s->dac, OBJECT(s), &tcx_dac_ops, s,
+                          "tcx.dac", TCX_DAC_NREGS);
+    sysbus_init_mmio(sbd, &s->dac);
+
+    /* TEC (dummy) */
+    memory_region_init_io(&s->tec, OBJECT(s), &dummy_ops, s,
+                          "tcx.tec", TCX_TEC_NREGS);
+    sysbus_init_mmio(sbd, &s->tec);
+
+    /* THC: NetBSD writes here even with 8-bit display: dummy */
+    memory_region_init_io(&s->thc24, OBJECT(s), &dummy_ops, s, "tcx.thc24",
+                          TCX_THC_NREGS_24);
+    sysbus_init_mmio(sbd, &s->thc24);
+
+    return;
+}
+
 static void tcx_realizefn(DeviceState *dev, Error **errp)
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
@@ -545,11 +572,7 @@ static void tcx_realizefn(DeviceState *dev, Error **errp)
     vram_base = memory_region_get_ram_ptr(&s->vram_mem);
 
     /* FCode ROM */
-    memory_region_init_ram(&s->rom, NULL, "tcx.prom", FCODE_MAX_ROM_SIZE);
     vmstate_register_ram_global(&s->rom);
-    memory_region_set_readonly(&s->rom, true);
-    sysbus_init_mmio(sbd, &s->rom);
-
     fcode_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, TCX_ROM_FILE);
     if (fcode_filename) {
         ret = load_image_targphys(fcode_filename, s->prom_addr,
@@ -568,20 +591,6 @@ static void tcx_realizefn(DeviceState *dev, Error **errp)
     vram_offset += size;
     vram_base += size;
 
-    /* DAC */
-    memory_region_init_io(&s->dac, OBJECT(s), &tcx_dac_ops, s,
-                          "tcx.dac", TCX_DAC_NREGS);
-    sysbus_init_mmio(sbd, &s->dac);
-
-    /* TEC (dummy) */
-    memory_region_init_io(&s->tec, OBJECT(s), &dummy_ops, s,
-                          "tcx.tec", TCX_TEC_NREGS);
-    sysbus_init_mmio(sbd, &s->tec);
-    /* THC: NetBSD writes here even with 8-bit display: dummy */
-    memory_region_init_io(&s->thc24, OBJECT(s), &dummy_ops, s, "tcx.thc24",
-                          TCX_THC_NREGS_24);
-    sysbus_init_mmio(sbd, &s->thc24);
-
     if (s->depth == 24) {
         /* 24-bit plane */
         size = s->vram_size * 4;
@@ -637,6 +646,7 @@ static const TypeInfo tcx_info = {
     .name          = TYPE_TCX,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(TCXState),
+    .instance_init = tcx_initfn,
     .class_init    = tcx_class_init,
 };
 
diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index 75adb68..93803e1 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -543,14 +543,14 @@ static void tcx_init(hwaddr addr, int vram_size, int width,
     s = SYS_BUS_DEVICE(dev);
     /* FCode ROM */
     sysbus_mmio_map(s, 0, addr);
-    /* 8-bit plane */
-    sysbus_mmio_map(s, 1, addr + 0x00800000ULL);
     /* DAC */
-    sysbus_mmio_map(s, 2, addr + 0x00200000ULL);
+    sysbus_mmio_map(s, 1, addr + 0x00200000ULL);
     /* TEC (dummy) */
-    sysbus_mmio_map(s, 3, addr + 0x00700000ULL);
+    sysbus_mmio_map(s, 2, addr + 0x00700000ULL);
     /* THC 24 bit: NetBSD writes here even with 8-bit display: dummy */
-    sysbus_mmio_map(s, 4, addr + 0x00301000ULL);
+    sysbus_mmio_map(s, 3, addr + 0x00301000ULL);
+    /* 8-bit plane */
+    sysbus_mmio_map(s, 4, addr + 0x00800000ULL);
     if (depth == 24) {
         /* 24-bit plane */
         sysbus_mmio_map(s, 5, addr + 0x02000000ULL);
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 0/4] cg3/tcx cleanups

[Mark Cave-Ayland]

On 24/05/14 13:44, Mark Cave-Ayland wrote:

> This patchset primarily addresses Andreas' comments with regard to cg3 init/realize. The
> first patch addresses this, followed by a second patch to address a previous comment made
> by Paolo.
>
> The last two patches then alter tcx in a similar fashion to bring it in line with the
> cg3 changes.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

As this patchset resolves the remaining issues pointed out by Andreas' 
last review (and I've received no further feedback since), I've applied 
this to my qemu-sparc branch. Any remaining snags can be picked up 
during warm freeze.


ATB,

Mark.