Re: [PULL 012/136] arm/cubieboard: use memdev for RAM

[Igor Mammedov]

On Mon, 2 Mar 2020 15:41:13 +0000
Peter Maydell <address@hidden> wrote:

> On Tue, 25 Feb 2020 at 11:59, Paolo Bonzini <address@hidden> wrote:
> >
> > From: Igor Mammedov <address@hidden>
> >
> > 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 <address@hidden>
> > Reviewed-by: Andrew Jones <address@hidden>
> > Reviewed-by: Richard Henderson <address@hidden>
> > Message-Id: <address@hidden>
> > ---
> >  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
>