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

[Peter Maydell]

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.

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