| Message ID | 1582631466-13880-12-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Tue, 25 Feb 2020 at 11:59, Paolo Bonzini <pbonzini@redhat.com> wrote: > > From: Igor Mammedov <imammedo@redhat.com> > > memory_region_allocate_system_memory() API is going away, so > replace it with memdev allocated MemoryRegion. The later is > initialized by generic code, so board only needs to opt in > to memdev scheme by providing > MachineClass::default_ram_id > and using MachineState::ram instead of manually initializing > RAM memory region. > > PS: > While at it, get rid of no longer needed CubieBoardState wrapper. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > Reviewed-by: Andrew Jones <drjones@redhat.com> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > Message-Id: <20200219160953.13771-13-imammedo@redhat.com> > --- > hw/arm/cubieboard.c | 25 ++++++++----------------- > 1 file changed, 8 insertions(+), 17 deletions(-) > > diff --git a/hw/arm/cubieboard.c b/hw/arm/cubieboard.c > index 6dc2f1d..089f9a3 100644 > --- a/hw/arm/cubieboard.c > +++ b/hw/arm/cubieboard.c > @@ -28,52 +28,42 @@ static struct arm_boot_info cubieboard_binfo = { > .board_id = 0x1008, > }; > > -typedef struct CubieBoardState { > - AwA10State *a10; > - MemoryRegion sdram; > -} CubieBoardState; > - > static void cubieboard_init(MachineState *machine) > { > - CubieBoardState *s = g_new(CubieBoardState, 1); > + AwA10State *a10 = AW_A10(object_new(TYPE_AW_A10)); > Error *err = NULL; > > - s->a10 = AW_A10(object_new(TYPE_AW_A10)); Hi Igor, I just noticed this, and I don't think it's the right thing. The board model should have its own state structure which contains any objects it creates. Just because there happens currently to be only a single object in this case doesn't mean we want to lose the structure. In particular, we now just leak the pointer to the TYPE_AW_A10 object, rather than having it be tracked by being pointed to from the MachineState. Being able to avoid just leaking pointers to objects like that is one of the reasons I like having a MachineState now. Could you send a patch that reverts this bit, please (and any of the other boards where you did this in the course of this refactoring)? thanks -- PMM
On Mon, 2 Mar 2020 15:41:13 +0000 Peter Maydell <peter.maydell@linaro.org> wrote: > On Tue, 25 Feb 2020 at 11:59, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > From: Igor Mammedov <imammedo@redhat.com> > > > > memory_region_allocate_system_memory() API is going away, so > > replace it with memdev allocated MemoryRegion. The later is > > initialized by generic code, so board only needs to opt in > > to memdev scheme by providing > > MachineClass::default_ram_id > > and using MachineState::ram instead of manually initializing > > RAM memory region. > > > > PS: > > While at it, get rid of no longer needed CubieBoardState wrapper. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > Reviewed-by: Andrew Jones <drjones@redhat.com> > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > > Message-Id: <20200219160953.13771-13-imammedo@redhat.com> > > --- > > hw/arm/cubieboard.c | 25 ++++++++----------------- > > 1 file changed, 8 insertions(+), 17 deletions(-) > > > > diff --git a/hw/arm/cubieboard.c b/hw/arm/cubieboard.c > > index 6dc2f1d..089f9a3 100644 > > --- a/hw/arm/cubieboard.c > > +++ b/hw/arm/cubieboard.c > > @@ -28,52 +28,42 @@ static struct arm_boot_info cubieboard_binfo = { > > .board_id = 0x1008, > > }; > > > > -typedef struct CubieBoardState { > > - AwA10State *a10; > > - MemoryRegion sdram; > > -} CubieBoardState; > > - > > static void cubieboard_init(MachineState *machine) > > { > > - CubieBoardState *s = g_new(CubieBoardState, 1); > > + AwA10State *a10 = AW_A10(object_new(TYPE_AW_A10)); > > Error *err = NULL; > > > > - s->a10 = AW_A10(object_new(TYPE_AW_A10)); > > Hi Igor, I just noticed this, and I don't think it's the > right thing. The board model should have its own state > structure which contains any objects it creates. Just > because there happens currently to be only a single > object in this case doesn't mean we want to lose the > structure. In particular, we now just leak the > pointer to the TYPE_AW_A10 object, rather than having > it be tracked by being pointed to from the MachineState. > Being able to avoid just leaking pointers to objects like > that is one of the reasons I like having a MachineState now. The reason why this structure was removed was that it wasn't MachineState object but a random structure which was a common pattern in pre-QOM qemu. Code was allocating 's', assigning pointer to s->a10 member and then happily loosing both pointers in the end. Since rewriting such "states" to objects derived from MachineState was out of scope of memory-backend refactoring I've opted to in favor of simplified cleanup, which removes at least 1 lost pointer. This way however touches that code again to store some additional board state or actually fix pre-existing a10 object_new leak, could be asked to use MachineState derived object for that. So patch isn't changing anything in terms of lost pointers or proper board modeling. > Could you send a patch that reverts this bit, please > (and any of the other boards where you did this in > the course of this refactoring)? I can convert it to MachineState derived board as an example to follow. But it would be best if target/board maintainers would take care of other boards to MachineState objects across respective targets, to get rid of legacy examples so it won't be copied later on. In some cases it's trivial (like with this board) but in other cases it's a bit more complicated (Like Philippe did with RPi boards). > thanks > -- PMM >
On Mon, 2 Mar 2020 at 16:55, Igor Mammedov <imammedo@redhat.com> wrote: > > On Mon, 2 Mar 2020 15:41:13 +0000 > Peter Maydell <peter.maydell@linaro.org> wrote: > > Hi Igor, I just noticed this, and I don't think it's the > > right thing. The board model should have its own state > > structure which contains any objects it creates. Just > > because there happens currently to be only a single > > object in this case doesn't mean we want to lose the > > structure. In particular, we now just leak the > > pointer to the TYPE_AW_A10 object, rather than having > > it be tracked by being pointed to from the MachineState. > > Being able to avoid just leaking pointers to objects like > > that is one of the reasons I like having a MachineState now. > The reason why this structure was removed was that it wasn't > MachineState object but a random structure which was a common > pattern in pre-QOM qemu. > > Code was allocating 's', assigning pointer to s->a10 member and > then happily loosing both pointers in the end. Oh, so it was -- I misread the code. > I can convert it to MachineState derived board as > an example to follow. No, you don't need to do that, I hadn't realized it wasn't already MachineState derived, because it looked superficially like it was. Apologies for the false alarm. thanks -- PMM
diff --git a/hw/arm/cubieboard.c b/hw/arm/cubieboard.c index 6dc2f1d..089f9a3 100644 --- a/hw/arm/cubieboard.c +++ b/hw/arm/cubieboard.c @@ -28,52 +28,42 @@ static struct arm_boot_info cubieboard_binfo = { .board_id = 0x1008, }; -typedef struct CubieBoardState { - AwA10State *a10; - MemoryRegion sdram; -} CubieBoardState; - static void cubieboard_init(MachineState *machine) { - CubieBoardState *s = g_new(CubieBoardState, 1); + AwA10State *a10 = AW_A10(object_new(TYPE_AW_A10)); Error *err = NULL; - s->a10 = AW_A10(object_new(TYPE_AW_A10)); - - object_property_set_int(OBJECT(&s->a10->emac), 1, "phy-addr", &err); + object_property_set_int(OBJECT(&a10->emac), 1, "phy-addr", &err); if (err != NULL) { error_reportf_err(err, "Couldn't set phy address: "); exit(1); } - object_property_set_int(OBJECT(&s->a10->timer), 32768, "clk0-freq", &err); + object_property_set_int(OBJECT(&a10->timer), 32768, "clk0-freq", &err); if (err != NULL) { error_reportf_err(err, "Couldn't set clk0 frequency: "); exit(1); } - object_property_set_int(OBJECT(&s->a10->timer), 24000000, "clk1-freq", - &err); + object_property_set_int(OBJECT(&a10->timer), 24000000, "clk1-freq", &err); if (err != NULL) { error_reportf_err(err, "Couldn't set clk1 frequency: "); exit(1); } - object_property_set_bool(OBJECT(s->a10), true, "realized", &err); + object_property_set_bool(OBJECT(a10), true, "realized", &err); if (err != NULL) { error_reportf_err(err, "Couldn't realize Allwinner A10: "); exit(1); } - memory_region_allocate_system_memory(&s->sdram, NULL, "cubieboard.ram", - machine->ram_size); memory_region_add_subregion(get_system_memory(), AW_A10_SDRAM_BASE, - &s->sdram); + machine->ram); /* TODO create and connect IDE devices for ide_drive_get() */ cubieboard_binfo.ram_size = machine->ram_size; - arm_load_kernel(&s->a10->cpu, machine, &cubieboard_binfo); + arm_load_kernel(&a10->cpu, machine, &cubieboard_binfo); } static void cubieboard_machine_init(MachineClass *mc) @@ -84,6 +74,7 @@ static void cubieboard_machine_init(MachineClass *mc) mc->block_default_type = IF_IDE; mc->units_per_default_bus = 1; mc->ignore_memory_transaction_failures = true; + mc->default_ram_id = "cubieboard.ram"; } DEFINE_MACHINE("cubieboard", cubieboard_machine_init)