Re: [PATCH 2/7] hw/arm: Let devices own the MemoryRegion they create

[Peter Maydell]

On Fri, 21 Feb 2020 at 17:31, Philippe Mathieu-Daudé <address@hidden> wrote:
>
> To avoid orphean memory regions being added in the /unattached

("orphan")

> QOM container, use the memory_region_owner_nonnull.cocci script
> to set the correct ownership.
>
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> ---
>  hw/arm/exynos4210.c    | 14 +++++++-------
>  hw/arm/fsl-imx25.c     | 14 +++++++-------
>  hw/arm/fsl-imx31.c     | 10 +++++-----
>  hw/arm/fsl-imx6.c      | 10 +++++-----
>  hw/arm/fsl-imx6ul.c    | 14 +++++++-------
>  hw/arm/msf2-soc.c      |  8 ++++----
>  hw/arm/nrf51_soc.c     |  2 +-
>  hw/arm/stm32f205_soc.c | 10 +++++-----
>  hw/arm/stm32f405_soc.c | 13 +++++++------
>  hw/arm/xlnx-zynqmp.c   | 13 ++++++-------
>  10 files changed, 54 insertions(+), 54 deletions(-)
>
> diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
> index 59a27bdd68..d4b05336ee 100644
> --- a/hw/arm/exynos4210.c
> +++ b/hw/arm/exynos4210.c
> @@ -305,28 +305,28 @@ static void exynos4210_realize(DeviceState *socdev, 
> Error **errp)
>      /*** Memory ***/
>
>      /* Chip-ID and OMR */
> -    memory_region_init_io(&s->chipid_mem, NULL, 
> &exynos4210_chipid_and_omr_ops,
> -        NULL, "exynos4210.chipid", sizeof(chipid_and_omr));
> +    memory_region_init_io(&s->chipid_mem, OBJECT(socdev),
> +                          &exynos4210_chipid_and_omr_ops, NULL,
> +                          "exynos4210.chipid", sizeof(chipid_and_omr));
>      memory_region_add_subregion(system_mem, EXYNOS4210_CHIPID_ADDR,
>                                  &s->chipid_mem);
>
>      /* Internal ROM */
> -    memory_region_init_ram(&s->irom_mem, NULL, "exynos4210.irom",
> +    memory_region_init_ram(&s->irom_mem, OBJECT(socdev), "exynos4210.irom",
>                             EXYNOS4210_IROM_SIZE, &error_fatal);

I have a feeling that the owner of a RAM memory region affects
the name we use for the underlying ramblock, which in turn
is used in the on-the-wire data stream in migration, which means
that changing ownership breaks migration compatibility.
That's probably OK in most cases as we don't care too much
about migration-compat on most boards, but I think it does
mean that you want to keep those changes separated out from
the ones for IO and alias regions, which I think shouldn't
have visible effects.

thanks
-- PMM