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

[BALATON Zoltan]

On Fri, 19 Nov 2021, 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).
>
> Suggested-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/display/vga-mmio.c | 134 +++++++++++++++++++++++++++++-------------
> 1 file changed, 94 insertions(+), 40 deletions(-)
>
> diff --git a/hw/display/vga-mmio.c b/hw/display/vga-mmio.c
> index 0aefbcf53a0..d1c5f31c134 100644
> --- a/hw/display/vga-mmio.c
> +++ b/hw/display/vga-mmio.c
> @@ -2,6 +2,7 @@
>  * QEMU MMIO VGA Emulator.
>  *
>  * Copyright (c) 2003 Fabrice Bellard
> + * Copyright (c) 2021 Philippe Mathieu-Daudé

I think it's OK to add copyright line to record who's to blame if you 
change or rewrite significant parts of a file. In this case just 
QOM-ifying it may not be large enough change for me to add my copyright if 
I were doing this. I don't mind, just thought git blame in this case might 
be enough unless of course you want to also take maintainership of the 
file. :-)

>  *
>  * Permission is hereby granted, free of charge, to any person obtaining a copy
>  * of this software and associated documentation files (the "Software"), to deal
> @@ -23,21 +24,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/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 {
> +#define TYPE_VGA_MMIO "vga-mmio"
> +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,43 +79,83 @@ static const MemoryRegionOps vga_mm_ctrl_ops = {
>     .endianness = DEVICE_NATIVE_ENDIAN,
> };
>
> +static void vga_mmio_reset(DeviceState *dev)
> +{
> +    VGAMmioState *d = VGA_MMIO(dev);
> +    VGACommonState *s = &d->vga;
> +
> +    vga_common_reset(s);

Maybe there's no need for separate variable s here as it's only used once, 
you could just as well write vga_common_reset(&d->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,

But maybe you can store OBJECT(dev) in a local as it's needed more than 
once here. Otherwise

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

> +                          "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)