| Message ID | 20231215200009.346212-12-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | next-cube: various tidy-ups and improvements | expand |
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. Thomas
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
On 16/12/2023 20:20, 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. No problem, I can drop this patch from the series. ATB, Mark.
On 16/12/2023 21:31, BALATON Zoltan wrote: > 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. I don't really have a preference either way (it was mainly inspired by looking at existing code), so if Thomas would prefer that as maintainer then that's fine with me. > 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. Embedding the memory regions in the machine state is simply encapsulating them in into a container, as we already do for static memory regions used by devices. Also if you don't keep track of the memory regions, presumably they will leak when QEMU terminates? > 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. I don't think that will be an issue here, and I quite like that it provides an extra layer of type safety. ATB, Mark.
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); } static void next_machine_class_init(ObjectClass *oc, void *data)
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(-)