Re: [PATCH 11/12] next-cube.c: replace sysmem with get_system_memory() in next_cube_init()

[BALATON Zoltan]

On Sat, 16 Dec 2023, Thomas Huth wrote:
> Am Fri, 15 Dec 2023 20:00:08 +0000
> schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>
> > Removing the intermediate variable helps simplify the code in next_cube_init().
> >
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > ---
> >  hw/m68k/next-cube.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
> > index d9a1f234ec..73deef25ca 100644
> > --- a/hw/m68k/next-cube.c
> > +++ b/hw/m68k/next-cube.c
> > @@ -974,7 +974,6 @@ static void next_cube_init(MachineState *machine)
> >      MemoryRegion *dmamem = g_new(MemoryRegion, 1);
> >      MemoryRegion *bmapm1 = g_new(MemoryRegion, 1);
> >      MemoryRegion *bmapm2 = g_new(MemoryRegion, 1);
> > -    MemoryRegion *sysmem = get_system_memory();
> >      const char *bios_name = machine->firmware ?: ROM_FILE;
> >      DeviceState *pcdev;
> >
> > @@ -996,7 +995,8 @@ static void next_cube_init(MachineState *machine)
> >      sysbus_realize_and_unref(SYS_BUS_DEVICE(pcdev), &error_fatal);
> >
> >      /* 64MB RAM starting at 0x04000000  */
> > -    memory_region_add_subregion(sysmem, 0x04000000, machine->ram);
> > +    memory_region_add_subregion(get_system_memory(), 0x04000000,
> > +                                machine->ram);
> >
> >      /* Framebuffer */
> >      sysbus_create_simple(TYPE_NEXTFB, 0x0B000000, NULL);
> > @@ -1010,19 +1010,19 @@ static void next_cube_init(MachineState *machine)
> >      /* BMAP memory */
> >      memory_region_init_ram_flags_nomigrate(bmapm1, NULL, "next.bmapmem", 64,
> >                                             RAM_SHARED, &error_fatal);
> > -    memory_region_add_subregion(sysmem, 0x020c0000, bmapm1);
> > +    memory_region_add_subregion(get_system_memory(), 0x020c0000, bmapm1);
> >      /* The Rev_2.5_v66.bin firmware accesses it at 0x820c0020, too */
> >      memory_region_init_alias(bmapm2, NULL, "next.bmapmem2", bmapm1, 0x0, 64);
> > -    memory_region_add_subregion(sysmem, 0x820c0000, bmapm2);
> > +    memory_region_add_subregion(get_system_memory(), 0x820c0000, bmapm2);
> >
> >      /* KBD */
> >      sysbus_create_simple(TYPE_NEXTKBD, 0x0200e000, NULL);
> >
> >      /* Load ROM here */
> >      memory_region_init_rom(rom, NULL, "next.rom", 0x20000, &error_fatal);
> > -    memory_region_add_subregion(sysmem, 0x01000000, rom);
> > +    memory_region_add_subregion(get_system_memory(), 0x01000000, rom);
> >      memory_region_init_alias(rom2, NULL, "next.rom2", rom, 0x0, 0x20000);
> > -    memory_region_add_subregion(sysmem, 0x0, rom2);
> > +    memory_region_add_subregion(get_system_memory(), 0x0, rom2);
> >      if (load_image_targphys(bios_name, 0x01000000, 0x20000) < 8) {
> >          if (!qtest_enabled()) {
> >              error_report("Failed to load firmware '%s'.", bios_name);
> > @@ -1051,7 +1051,7 @@ static void next_cube_init(MachineState *machine)
> >      /* DMA */
> >      memory_region_init_io(dmamem, NULL, &next_dma_ops, machine, "next.dma",
> >                            0x5000);
> > -    memory_region_add_subregion(sysmem, 0x02000000, dmamem);
> > +    memory_region_add_subregion(get_system_memory(), 0x02000000, dmamem);
> >  }
>
> Mostly a matter of taste, but I'd prefer to keep it like it was before - I
> dislike calling functions multiple times if one time is sufficient.

The get_system_memory() function will only return a pointer to a static 
variable though so it's not expensive to call it multiple times and 
introducing a local variable just adds one more name for it to look up 
when reading the code so I generally prefer using it directly as it would 
likely be inlined by the compiler anyway.

That's also matter of taste but all the memory regions the next patch 
moves to machine state aren't really needed as these are only used for 
creating a mem region and adding it as subregion to system memory so one 
MemoryRegion *mr variable would be enough (and a meybe one more for alias 
regions) that are reused for all of these without storing them in machine 
state where they aren't used any more so no need to srore them.

Also I think in memory region call backs the void *opaque parameter don't 
need a QOM cast as these are registered here with already the wanted type 
for opaque so no need to check that every time the memory region is accessed.

Regards,
BALATON Zoltan