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

From: Peter Maydell <peter.maydell@linaro.org>
To: "Philippe Mathieu-Daudé" <philmd@redhat.com>
Cc: "Laurent Vivier" <laurent@vivier.eu>,
	"open list:RISC-V" <qemu-riscv@nongnu.org>,
	"Edgar E. Iglesias" <edgar.iglesias@gmail.com>,
	"Sagar Karandikar" <sagark@eecs.berkeley.edu>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	"Igor Mitsyanko" <i.mitsyanko@gmail.com>,
	"Bastian Koppelmann" <kbastian@mail.uni-paderborn.de>,
	"Alistair Francis" <alistair@alistair23.me>,
	"Mark Cave-Ayland" <mark.cave-ayland@ilande.co.uk>,
	"QEMU Developers" <qemu-devel@nongnu.org>,
	"Subbaraya Sundeep" <sundeep.lkml@gmail.com>,
	"Peter Chubb" <peter.chubb@nicta.com.au>,
	qemu-arm <qemu-arm@nongnu.org>,
	"Alistair Francis" <Alistair.Francis@wdc.com>,
	"Joel Stanley" <joel@jms.id.au>,
	"Marc-André Lureau" <marcandre.lureau@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Aleksandar Rikalo" <aleksandar.rikalo@rt-rk.com>,
	"Jean-Christophe Dubois" <jcd@tribudubois.net>,
	"Palmer Dabbelt" <palmer@dabbelt.com>,
	"Hervé Poussineau" <hpoussin@reactos.org>
Subject: Re: [PATCH 2/7] hw/arm: Let devices own the MemoryRegion they create
Date: Fri, 21 Feb 2020 17:36:03 +0000	[thread overview]
Message-ID: <CAFEAcA8+LWYhNjaXUu-Y2oat1hsKGbt6Hcir_yeNpmY+c6+8KA@mail.gmail.com> (raw)
In-Reply-To: <20200221173049.18134-3-philmd@redhat.com>

On Fri, 21 Feb 2020 at 17:31, Philippe Mathieu-Daudé <philmd@redhat.com> 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é <philmd@redhat.com>
> ---
>  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