[PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory()

[PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory()

[Igor Mammedov]

Series cleans up remaining boards that call memory_region_allocate_system_memory()
multiple times, violating interface contract (the function should be called only
once).

With that cleaned up, it should be possible to switch from adhoc RAM allocation
in memory_region_allocate_system_memory()->allocate_system_memory_nonnuma() to
memory-backend based allocation, remaining roadblock for doing it is deprecated
-mem-path fallback to RAM allocation, which is scheduled for removal at 4.3
merge window. So remaining patches to consolidate system RAM allocation around
memory-backends and aliasing -mem-path/mem-prealloc to it are postponed till
then.


Igor Mammedov (3):
  sparc64: use memory_region_allocate_system_memory() only for '-m'
    specified RAM
  ppc: rs6000_mc: drop usage of memory_region_allocate_system_memory()
  hppa: drop usage of memory_region_allocate_system_memory() for ROM

 hw/hppa/machine.c    |  5 ++---
 hw/ppc/rs6000_mc.c   | 15 ++++++++++-----
 hw/sparc64/niagara.c | 25 +++++++++++++------------
 3 files changed, 25 insertions(+), 20 deletions(-)

-- 
2.18.1

[PATCH 1/3] sparc64: use memory_region_allocate_system_memory() only for '-m' specified RAM

[Igor Mammedov]

memory_region_allocate_system_memory() was designed to be called for
allocating inital RAM. Using it mutiple times within one board is not
supported and could fail if -mem-path with non hugepage path is used.

Keep using memory_region_allocate_system_memory() only for initial
RAM and use memory_region_init_ram() for the rest fixed size regions.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/sparc64/niagara.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/hw/sparc64/niagara.c b/hw/sparc64/niagara.c
index 167143bffe..5987693659 100644
--- a/hw/sparc64/niagara.c
+++ b/hw/sparc64/niagara.c
@@ -36,6 +36,7 @@
 #include "qemu/error-report.h"
 #include "sysemu/qtest.h"
 #include "sysemu/sysemu.h"
+#include "qapi/error.h"
 
 typedef struct NiagaraBoardState {
     MemoryRegion hv_ram;
@@ -106,8 +107,8 @@ static void niagara_init(MachineState *machine)
     /* init CPUs */
     sparc64_cpu_devinit(machine->cpu_type, NIAGARA_PROM_BASE);
     /* set up devices */
-    memory_region_allocate_system_memory(&s->hv_ram, NULL, "sun4v-hv.ram",
-                                         NIAGARA_HV_RAM_SIZE);
+    memory_region_init_ram(&s->hv_ram, NULL, "sun4v-hv.ram",
+                           NIAGARA_HV_RAM_SIZE, &error_fatal);
     memory_region_add_subregion(sysmem, NIAGARA_HV_RAM_BASE, &s->hv_ram);
 
     memory_region_allocate_system_memory(&s->partition_ram, NULL,
@@ -116,17 +117,17 @@ static void niagara_init(MachineState *machine)
     memory_region_add_subregion(sysmem, NIAGARA_PARTITION_RAM_BASE,
                                 &s->partition_ram);
 
-    memory_region_allocate_system_memory(&s->nvram, NULL,
-                                         "sun4v.nvram", NIAGARA_NVRAM_SIZE);
+    memory_region_init_ram(&s->nvram, NULL, "sun4v.nvram", NIAGARA_NVRAM_SIZE,
+                           &error_fatal);
     memory_region_add_subregion(sysmem, NIAGARA_NVRAM_BASE, &s->nvram);
-    memory_region_allocate_system_memory(&s->md_rom, NULL,
-                                         "sun4v-md.rom", NIAGARA_MD_ROM_SIZE);
+    memory_region_init_ram(&s->md_rom, NULL, "sun4v-md.rom",
+                           NIAGARA_MD_ROM_SIZE, &error_fatal);
     memory_region_add_subregion(sysmem, NIAGARA_MD_ROM_BASE, &s->md_rom);
-    memory_region_allocate_system_memory(&s->hv_rom, NULL,
-                                         "sun4v-hv.rom", NIAGARA_HV_ROM_SIZE);
+    memory_region_init_ram(&s->hv_rom, NULL, "sun4v-hv.rom",
+                           NIAGARA_HV_ROM_SIZE, &error_fatal);
     memory_region_add_subregion(sysmem, NIAGARA_HV_ROM_BASE, &s->hv_rom);
-    memory_region_allocate_system_memory(&s->prom, NULL,
-                                         "sun4v.prom", PROM_SIZE_MAX);
+    memory_region_init_ram(&s->prom, NULL, "sun4v.prom", PROM_SIZE_MAX,
+                           &error_fatal);
     memory_region_add_subregion(sysmem, NIAGARA_PROM_BASE, &s->prom);
 
     add_rom_or_fail("nvram1", NIAGARA_NVRAM_BASE);
@@ -143,8 +144,8 @@ static void niagara_init(MachineState *machine)
         BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
         int size = blk_getlength(blk);
         if (size > 0) {
-            memory_region_allocate_system_memory(&s->vdisk_ram, NULL,
-                                                 "sun4v_vdisk.ram", size);
+            memory_region_init_ram(&s->vdisk_ram, NULL, "sun4v_vdisk.ram", size,
+                                   &error_fatal);
             memory_region_add_subregion(get_system_memory(),
                                         NIAGARA_VDISK_BASE, &s->vdisk_ram);
             dinfo->is_default = 1;
-- 
2.18.1

Re: [PATCH 1/3] sparc64: use memory_region_allocate_system_memory() only for '-m' specified RAM

[Philippe Mathieu-Daudé]

On 10/8/19 1:33 PM, Igor Mammedov wrote:
> memory_region_allocate_system_memory() was designed to be called for
> allocating inital RAM. Using it mutiple times within one board is not
> supported and could fail if -mem-path with non hugepage path is used.
> 
> Keep using memory_region_allocate_system_memory() only for initial
> RAM and use memory_region_init_ram() for the rest fixed size regions.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>   hw/sparc64/niagara.c | 25 +++++++++++++------------
>   1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/sparc64/niagara.c b/hw/sparc64/niagara.c
> index 167143bffe..5987693659 100644
> --- a/hw/sparc64/niagara.c
> +++ b/hw/sparc64/niagara.c
> @@ -36,6 +36,7 @@
>   #include "qemu/error-report.h"
>   #include "sysemu/qtest.h"
>   #include "sysemu/sysemu.h"
> +#include "qapi/error.h"
>   
>   typedef struct NiagaraBoardState {
>       MemoryRegion hv_ram;
> @@ -106,8 +107,8 @@ static void niagara_init(MachineState *machine)
>       /* init CPUs */
>       sparc64_cpu_devinit(machine->cpu_type, NIAGARA_PROM_BASE);
>       /* set up devices */
> -    memory_region_allocate_system_memory(&s->hv_ram, NULL, "sun4v-hv.ram",
> -                                         NIAGARA_HV_RAM_SIZE);
> +    memory_region_init_ram(&s->hv_ram, NULL, "sun4v-hv.ram",
> +                           NIAGARA_HV_RAM_SIZE, &error_fatal);
>       memory_region_add_subregion(sysmem, NIAGARA_HV_RAM_BASE, &s->hv_ram);
>   
>       memory_region_allocate_system_memory(&s->partition_ram, NULL,

Did we have an issue calling memory_region_allocate_system_memory() 
*after* memory_region_init_ram() by the past?
Maybe not, or it is fixed, and your patch is fine:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> @@ -116,17 +117,17 @@ static void niagara_init(MachineState *machine)
>       memory_region_add_subregion(sysmem, NIAGARA_PARTITION_RAM_BASE,
>                                   &s->partition_ram);
>   
> -    memory_region_allocate_system_memory(&s->nvram, NULL,
> -                                         "sun4v.nvram", NIAGARA_NVRAM_SIZE);
> +    memory_region_init_ram(&s->nvram, NULL, "sun4v.nvram", NIAGARA_NVRAM_SIZE,
> +                           &error_fatal);
>       memory_region_add_subregion(sysmem, NIAGARA_NVRAM_BASE, &s->nvram);
> -    memory_region_allocate_system_memory(&s->md_rom, NULL,
> -                                         "sun4v-md.rom", NIAGARA_MD_ROM_SIZE);
> +    memory_region_init_ram(&s->md_rom, NULL, "sun4v-md.rom",
> +                           NIAGARA_MD_ROM_SIZE, &error_fatal);
>       memory_region_add_subregion(sysmem, NIAGARA_MD_ROM_BASE, &s->md_rom);
> -    memory_region_allocate_system_memory(&s->hv_rom, NULL,
> -                                         "sun4v-hv.rom", NIAGARA_HV_ROM_SIZE);
> +    memory_region_init_ram(&s->hv_rom, NULL, "sun4v-hv.rom",
> +                           NIAGARA_HV_ROM_SIZE, &error_fatal);
>       memory_region_add_subregion(sysmem, NIAGARA_HV_ROM_BASE, &s->hv_rom);
> -    memory_region_allocate_system_memory(&s->prom, NULL,
> -                                         "sun4v.prom", PROM_SIZE_MAX);
> +    memory_region_init_ram(&s->prom, NULL, "sun4v.prom", PROM_SIZE_MAX,
> +                           &error_fatal);
>       memory_region_add_subregion(sysmem, NIAGARA_PROM_BASE, &s->prom);
>   
>       add_rom_or_fail("nvram1", NIAGARA_NVRAM_BASE);
> @@ -143,8 +144,8 @@ static void niagara_init(MachineState *machine)
>           BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
>           int size = blk_getlength(blk);
>           if (size > 0) {
> -            memory_region_allocate_system_memory(&s->vdisk_ram, NULL,
> -                                                 "sun4v_vdisk.ram", size);
> +            memory_region_init_ram(&s->vdisk_ram, NULL, "sun4v_vdisk.ram", size,
> +                                   &error_fatal);
>               memory_region_add_subregion(get_system_memory(),
>                                           NIAGARA_VDISK_BASE, &s->vdisk_ram);
>               dinfo->is_default = 1;
> 

Re: [PATCH 1/3] sparc64: use memory_region_allocate_system_memory() only for '-m' specified RAM

[Igor Mammedov]

On Tue, 8 Oct 2019 14:09:28 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 10/8/19 1:33 PM, Igor Mammedov wrote:
> > memory_region_allocate_system_memory() was designed to be called for
> > allocating inital RAM. Using it mutiple times within one board is not
> > supported and could fail if -mem-path with non hugepage path is used.
> > 
> > Keep using memory_region_allocate_system_memory() only for initial
> > RAM and use memory_region_init_ram() for the rest fixed size regions.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >   hw/sparc64/niagara.c | 25 +++++++++++++------------
> >   1 file changed, 13 insertions(+), 12 deletions(-)
> > 
> > diff --git a/hw/sparc64/niagara.c b/hw/sparc64/niagara.c
> > index 167143bffe..5987693659 100644
> > --- a/hw/sparc64/niagara.c
> > +++ b/hw/sparc64/niagara.c
> > @@ -36,6 +36,7 @@
> >   #include "qemu/error-report.h"
> >   #include "sysemu/qtest.h"
> >   #include "sysemu/sysemu.h"
> > +#include "qapi/error.h"
> >   
> >   typedef struct NiagaraBoardState {
> >       MemoryRegion hv_ram;
> > @@ -106,8 +107,8 @@ static void niagara_init(MachineState *machine)
> >       /* init CPUs */
> >       sparc64_cpu_devinit(machine->cpu_type, NIAGARA_PROM_BASE);
> >       /* set up devices */
> > -    memory_region_allocate_system_memory(&s->hv_ram, NULL, "sun4v-hv.ram",
> > -                                         NIAGARA_HV_RAM_SIZE);
> > +    memory_region_init_ram(&s->hv_ram, NULL, "sun4v-hv.ram",
> > +                           NIAGARA_HV_RAM_SIZE, &error_fatal);
> >       memory_region_add_subregion(sysmem, NIAGARA_HV_RAM_BASE, &s->hv_ram);
> >   
> >       memory_region_allocate_system_memory(&s->partition_ram, NULL,  
> 
> Did we have an issue calling memory_region_allocate_system_memory() 
> *after* memory_region_init_ram() by the past?

I don't recall any, memory_region_init_ram() is essentially the same
as memory_region_allocate_system_memory() for most of emulated machines
(i.e. non-numa, no mem-path cases)

> Maybe not, or it is fixed, and your patch is fine:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Thanks!

> 
> > @@ -116,17 +117,17 @@ static void niagara_init(MachineState *machine)
> >       memory_region_add_subregion(sysmem, NIAGARA_PARTITION_RAM_BASE,
> >                                   &s->partition_ram);
> >   
> > -    memory_region_allocate_system_memory(&s->nvram, NULL,
> > -                                         "sun4v.nvram", NIAGARA_NVRAM_SIZE);
> > +    memory_region_init_ram(&s->nvram, NULL, "sun4v.nvram", NIAGARA_NVRAM_SIZE,
> > +                           &error_fatal);
> >       memory_region_add_subregion(sysmem, NIAGARA_NVRAM_BASE, &s->nvram);
> > -    memory_region_allocate_system_memory(&s->md_rom, NULL,
> > -                                         "sun4v-md.rom", NIAGARA_MD_ROM_SIZE);
> > +    memory_region_init_ram(&s->md_rom, NULL, "sun4v-md.rom",
> > +                           NIAGARA_MD_ROM_SIZE, &error_fatal);
> >       memory_region_add_subregion(sysmem, NIAGARA_MD_ROM_BASE, &s->md_rom);
> > -    memory_region_allocate_system_memory(&s->hv_rom, NULL,
> > -                                         "sun4v-hv.rom", NIAGARA_HV_ROM_SIZE);
> > +    memory_region_init_ram(&s->hv_rom, NULL, "sun4v-hv.rom",
> > +                           NIAGARA_HV_ROM_SIZE, &error_fatal);
> >       memory_region_add_subregion(sysmem, NIAGARA_HV_ROM_BASE, &s->hv_rom);
> > -    memory_region_allocate_system_memory(&s->prom, NULL,
> > -                                         "sun4v.prom", PROM_SIZE_MAX);
> > +    memory_region_init_ram(&s->prom, NULL, "sun4v.prom", PROM_SIZE_MAX,
> > +                           &error_fatal);
> >       memory_region_add_subregion(sysmem, NIAGARA_PROM_BASE, &s->prom);
> >   
> >       add_rom_or_fail("nvram1", NIAGARA_NVRAM_BASE);
> > @@ -143,8 +144,8 @@ static void niagara_init(MachineState *machine)
> >           BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
> >           int size = blk_getlength(blk);
> >           if (size > 0) {
> > -            memory_region_allocate_system_memory(&s->vdisk_ram, NULL,
> > -                                                 "sun4v_vdisk.ram", size);
> > +            memory_region_init_ram(&s->vdisk_ram, NULL, "sun4v_vdisk.ram", size,
> > +                                   &error_fatal);
> >               memory_region_add_subregion(get_system_memory(),
> >                                           NIAGARA_VDISK_BASE, &s->vdisk_ram);
> >               dinfo->is_default = 1;
> >   
> 

[PATCH 2/3] ppc: rs6000_mc: drop usage of memory_region_allocate_system_memory()

[Igor Mammedov]

rs6000mc_realize() violates memory_region_allocate_system_memory() contract
by calling it multiple times which could break -mem-path. Replace it with
plain memory_region_init_ram() instead.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/ppc/rs6000_mc.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/rs6000_mc.c b/hw/ppc/rs6000_mc.c
index df7c0006fc..66b14db5fa 100644
--- a/hw/ppc/rs6000_mc.c
+++ b/hw/ppc/rs6000_mc.c
@@ -144,6 +144,7 @@ static void rs6000mc_realize(DeviceState *dev, Error **errp)
     RS6000MCState *s = RS6000MC_DEVICE(dev);
     int socket = 0;
     unsigned int ram_size = s->ram_size / MiB;
+    Error *local_err = NULL;
 
     while (socket < 6) {
         if (ram_size >= 64) {
@@ -165,19 +166,21 @@ static void rs6000mc_realize(DeviceState *dev, Error **errp)
         if (s->simm_size[socket]) {
             char name[] = "simm.?";
             name[5] = socket + '0';
-            memory_region_allocate_system_memory(&s->simm[socket], OBJECT(dev),
-                                                 name,
-                                                 s->simm_size[socket] * MiB);
+            memory_region_init_ram(&s->simm[socket], OBJECT(dev), name,
+                                   s->simm_size[socket] * MiB, &local_err);
+            if (local_err) {
+                goto out;
+            }
             memory_region_add_subregion_overlap(get_system_memory(), 0,
                                                 &s->simm[socket], socket);
         }
     }
     if (ram_size) {
         /* unable to push all requested RAM in SIMMs */
-        error_setg(errp, "RAM size incompatible with this board. "
+        error_setg(&local_err, "RAM size incompatible with this board. "
                    "Try again with something else, like %" PRId64 " MB",
                    s->ram_size / MiB - ram_size);
-        return;
+        goto out;
     }
 
     if (s->autoconfigure) {
@@ -193,6 +196,8 @@ static void rs6000mc_realize(DeviceState *dev, Error **errp)
 
     isa_register_portio_list(ISA_DEVICE(dev), &s->portio, 0x0,
                              rs6000mc_port_list, s, "rs6000mc");
+out:
+    error_propagate(errp, local_err);
 }
 
 static const VMStateDescription vmstate_rs6000mc = {
-- 
2.18.1

Re: [PATCH 2/3] ppc: rs6000_mc: drop usage of memory_region_allocate_system_memory()

[Philippe Mathieu-Daudé]

On 10/8/19 1:33 PM, Igor Mammedov wrote:
> rs6000mc_realize() violates memory_region_allocate_system_memory() contract
> by calling it multiple times which could break -mem-path. Replace it with
> plain memory_region_init_ram() instead.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   hw/ppc/rs6000_mc.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc/rs6000_mc.c b/hw/ppc/rs6000_mc.c
> index df7c0006fc..66b14db5fa 100644
> --- a/hw/ppc/rs6000_mc.c
> +++ b/hw/ppc/rs6000_mc.c
> @@ -144,6 +144,7 @@ static void rs6000mc_realize(DeviceState *dev, Error **errp)
>       RS6000MCState *s = RS6000MC_DEVICE(dev);
>       int socket = 0;
>       unsigned int ram_size = s->ram_size / MiB;
> +    Error *local_err = NULL;
>   
>       while (socket < 6) {
>           if (ram_size >= 64) {
> @@ -165,19 +166,21 @@ static void rs6000mc_realize(DeviceState *dev, Error **errp)
>           if (s->simm_size[socket]) {
>               char name[] = "simm.?";
>               name[5] = socket + '0';
> -            memory_region_allocate_system_memory(&s->simm[socket], OBJECT(dev),
> -                                                 name,
> -                                                 s->simm_size[socket] * MiB);
> +            memory_region_init_ram(&s->simm[socket], OBJECT(dev), name,
> +                                   s->simm_size[socket] * MiB, &local_err);
> +            if (local_err) {
> +                goto out;
> +            }
>               memory_region_add_subregion_overlap(get_system_memory(), 0,
>                                                   &s->simm[socket], socket);
>           }
>       }
>       if (ram_size) {
>           /* unable to push all requested RAM in SIMMs */
> -        error_setg(errp, "RAM size incompatible with this board. "
> +        error_setg(&local_err, "RAM size incompatible with this board. "
>                      "Try again with something else, like %" PRId64 " MB",
>                      s->ram_size / MiB - ram_size);
> -        return;
> +        goto out;
>       }
>   
>       if (s->autoconfigure) {
> @@ -193,6 +196,8 @@ static void rs6000mc_realize(DeviceState *dev, Error **errp)
>   
>       isa_register_portio_list(ISA_DEVICE(dev), &s->portio, 0x0,
>                                rs6000mc_port_list, s, "rs6000mc");
> +out:
> +    error_propagate(errp, local_err);
>   }
>   
>   static const VMStateDescription vmstate_rs6000mc = {
> 

Re: [PATCH 2/3] ppc: rs6000_mc: drop usage of memory_region_allocate_system_memory()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 628 bytes --]

On Tue, Oct 08, 2019 at 07:33:17AM -0400, Igor Mammedov wrote:
> rs6000mc_realize() violates memory_region_allocate_system_memory() contract
> by calling it multiple times which could break -mem-path. Replace it with
> plain memory_region_init_ram() instead.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

Or would you like me to merge this through my tree?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/3] ppc: rs6000_mc: drop usage of memory_region_allocate_system_memory()

[Igor Mammedov]

On Wed, 9 Oct 2019 12:21:06 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Oct 08, 2019 at 07:33:17AM -0400, Igor Mammedov wrote:
> > rs6000mc_realize() violates memory_region_allocate_system_memory() contract
> > by calling it multiple times which could break -mem-path. Replace it with
> > plain memory_region_init_ram() instead.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> 
> Acked-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Or would you like me to merge this through my tree?


(since it doesn't belong to a particular tree, I'd need find
some to take in in)

Since you volunteered :),
please merge this series via your tree.

Thanks!

Re: [PATCH 2/3] ppc: rs6000_mc: drop usage of memory_region_allocate_system_memory()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1205 bytes --]

On Wed, Oct 09, 2019 at 01:19:21PM +0200, Igor Mammedov wrote:
> On Wed, 9 Oct 2019 12:21:06 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Tue, Oct 08, 2019 at 07:33:17AM -0400, Igor Mammedov wrote:
> > > rs6000mc_realize() violates memory_region_allocate_system_memory() contract
> > > by calling it multiple times which could break -mem-path. Replace it with
> > > plain memory_region_init_ram() instead.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> > 
> > Acked-by: David Gibson <david@gibson.dropbear.id.au>
> > 
> > Or would you like me to merge this through my tree?
> 
> 
> (since it doesn't belong to a particular tree, I'd need find
> some to take in in)
> 
> Since you volunteered :),
> please merge this series via your tree.

Uhhh.. I was offering to take this patch, not the whole series.  I'm
not real comfortable taking the others, since they don't involve ppc
code, nor are they prereqs for or motivated by any ppc change.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 3/3] hppa: drop usage of memory_region_allocate_system_memory() for ROM

[Igor Mammedov]

machine_hppa_init() violates memory_region_allocate_system_memory() contract
by calling it multiple times which could break with -mem-path. Replace
the second usage (for 'rom') with memory_region_init_ram() instead.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/hppa/machine.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index 7e23675429..953d454f48 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -161,9 +161,8 @@ static void machine_hppa_init(MachineState *machine)
     g_free(firmware_filename);
 
     rom_region = g_new(MemoryRegion, 1);
-    memory_region_allocate_system_memory(rom_region, OBJECT(machine),
-                                         "firmware",
-                                         (FIRMWARE_END - FIRMWARE_START));
+    memory_region_init_ram(rom_region, NULL, "firmware",
+                           (FIRMWARE_END - FIRMWARE_START), &error_fatal);
     memory_region_add_subregion(addr_space, FIRMWARE_START, rom_region);
 
     /* Load kernel */
-- 
2.18.1

Re: [PATCH 3/3] hppa: drop usage of memory_region_allocate_system_memory() for ROM

[Philippe Mathieu-Daudé]

On 10/8/19 1:33 PM, Igor Mammedov wrote:
> machine_hppa_init() violates memory_region_allocate_system_memory() contract
> by calling it multiple times which could break with -mem-path. Replace
> the second usage (for 'rom') with memory_region_init_ram() instead.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>   hw/hppa/machine.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
> index 7e23675429..953d454f48 100644
> --- a/hw/hppa/machine.c
> +++ b/hw/hppa/machine.c
> @@ -161,9 +161,8 @@ static void machine_hppa_init(MachineState *machine)
>       g_free(firmware_filename);
>   
>       rom_region = g_new(MemoryRegion, 1);
> -    memory_region_allocate_system_memory(rom_region, OBJECT(machine),
> -                                         "firmware",
> -                                         (FIRMWARE_END - FIRMWARE_START));
> +    memory_region_init_ram(rom_region, NULL, "firmware",
> +                           (FIRMWARE_END - FIRMWARE_START), &error_fatal);
>       memory_region_add_subregion(addr_space, FIRMWARE_START, rom_region);
>   
>       /* Load kernel */
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 3/3] hppa: drop usage of memory_region_allocate_system_memory() for ROM

[Philippe Mathieu-Daudé]

On 10/8/19 1:33 PM, Igor Mammedov wrote:
> machine_hppa_init() violates memory_region_allocate_system_memory() contract
> by calling it multiple times which could break with -mem-path. Replace
> the second usage (for 'rom') with memory_region_init_ram() instead.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>   hw/hppa/machine.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
> index 7e23675429..953d454f48 100644
> --- a/hw/hppa/machine.c
> +++ b/hw/hppa/machine.c
> @@ -161,9 +161,8 @@ static void machine_hppa_init(MachineState *machine)
>       g_free(firmware_filename);
>   
>       rom_region = g_new(MemoryRegion, 1);
> -    memory_region_allocate_system_memory(rom_region, OBJECT(machine),
> -                                         "firmware",
> -                                         (FIRMWARE_END - FIRMWARE_START));
> +    memory_region_init_ram(rom_region, NULL, "firmware",
> +                           (FIRMWARE_END - FIRMWARE_START), &error_fatal);
>       memory_region_add_subregion(addr_space, FIRMWARE_START, rom_region);
>   
>       /* Load kernel */
> 

Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory()

[Philippe Mathieu-Daudé]

Hi Igor,

On 10/8/19 1:33 PM, Igor Mammedov wrote:
> Series cleans up remaining boards that call memory_region_allocate_system_memory()
> multiple times, violating interface contract (the function should be called only
> once).
> 
> With that cleaned up, it should be possible to switch from adhoc RAM allocation
> in memory_region_allocate_system_memory()->allocate_system_memory_nonnuma() to
> memory-backend based allocation, remaining roadblock for doing it is deprecated
> -mem-path fallback to RAM allocation, which is scheduled for removal at 4.3
> merge window. So remaining patches to consolidate system RAM allocation around
> memory-backends and aliasing -mem-path/mem-prealloc to it are postponed till
> then.

How do we protect the codebase for new boards to not make the same mistake?

What about some code like this snippet (or nicer, but since this is a 
developer error, and assert is enough IMO):

-- >8 --

diff --git a/hw/core/numa.c b/hw/core/numa.c
index 4dfec5c95b..a487677672 100644
--- a/hw/core/numa.c
+++ b/hw/core/numa.c
@@ -484,6 +484,11 @@ static void 
allocate_system_memory_nonnuma(MemoryRegion *mr, Object *owner,
                                             const char *name,
                                             uint64_t ram_size)
  {
+    static bool nonnuma_system_memory_allocated;
+
+    g_assert(!nonnuma_system_memory_allocated);
+    nonnuma_system_memory_allocated = true;
+
      if (mem_path) {
  #ifdef __linux__
          Error *err = NULL;
---

$ hppa-softmmu/qemu-system-hppa
**
ERROR:/home/phil/source/qemu/hw/core/numa.c:489:allocate_system_memory_nonnuma: 
assertion failed: (!nonnuma_system_memory_allocated)
Aborted (core dumped)

Re: [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory()

[Igor Mammedov]

On Tue, 8 Oct 2019 14:41:25 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> Hi Igor,
> 
> On 10/8/19 1:33 PM, Igor Mammedov wrote:
> > Series cleans up remaining boards that call memory_region_allocate_system_memory()
> > multiple times, violating interface contract (the function should be called only
> > once).
> > 
> > With that cleaned up, it should be possible to switch from adhoc RAM allocation
> > in memory_region_allocate_system_memory()->allocate_system_memory_nonnuma() to
> > memory-backend based allocation, remaining roadblock for doing it is deprecated
> > -mem-path fallback to RAM allocation, which is scheduled for removal at 4.3
> > merge window. So remaining patches to consolidate system RAM allocation around
> > memory-backends and aliasing -mem-path/mem-prealloc to it are postponed till
> > then.  
> 
> How do we protect the codebase for new boards to not make the same mistake?
> 
> What about some code like this snippet (or nicer, but since this is a 
> developer error, and assert is enough IMO):
probably it's not worth effort (it's not too long till 4.2 softfreeze).

Like cover letter say, I'm planing to finish refactoring of
memory_region_allocate_system_memory() and I hope this function
will be gone during 4.3 and most boards will only need to map
pre-created (by common code) memory-backend wherever they used to
map RAM memory region.

> -- >8 --  
> 
> diff --git a/hw/core/numa.c b/hw/core/numa.c
> index 4dfec5c95b..a487677672 100644
> --- a/hw/core/numa.c
> +++ b/hw/core/numa.c
> @@ -484,6 +484,11 @@ static void 
> allocate_system_memory_nonnuma(MemoryRegion *mr, Object *owner,
>                                              const char *name,
>                                              uint64_t ram_size)
>   {
> +    static bool nonnuma_system_memory_allocated;
> +
> +    g_assert(!nonnuma_system_memory_allocated);
> +    nonnuma_system_memory_allocated = true;
> +
>       if (mem_path) {
>   #ifdef __linux__
>           Error *err = NULL;
> ---
> 
> $ hppa-softmmu/qemu-system-hppa
> **
> ERROR:/home/phil/source/qemu/hw/core/numa.c:489:allocate_system_memory_nonnuma: 
> assertion failed: (!nonnuma_system_memory_allocated)
> Aborted (core dumped)

Re: [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory()

[Igor Mammedov]

On Tue,  8 Oct 2019 07:33:15 -0400
Igor Mammedov <imammedo@redhat.com> wrote:

> 
> Series cleans up remaining boards that call memory_region_allocate_system_memory()
> multiple times, violating interface contract (the function should be called only
> once).
> 
> With that cleaned up, it should be possible to switch from adhoc RAM allocation
> in memory_region_allocate_system_memory()->allocate_system_memory_nonnuma() to
> memory-backend based allocation, remaining roadblock for doing it is deprecated
> -mem-path fallback to RAM allocation, which is scheduled for removal at 4.3
> merge window. So remaining patches to consolidate system RAM allocation around
> memory-backends and aliasing -mem-path/mem-prealloc to it are postponed till
> then.

Eduardo,

This patches are fixing various machines across tree, so series does not belong
to any particular arch specific tree, can you merge it via generic machine tree?


> 
> 
> Igor Mammedov (3):
>   sparc64: use memory_region_allocate_system_memory() only for '-m'
>     specified RAM
>   ppc: rs6000_mc: drop usage of memory_region_allocate_system_memory()
>   hppa: drop usage of memory_region_allocate_system_memory() for ROM
> 
>  hw/hppa/machine.c    |  5 ++---
>  hw/ppc/rs6000_mc.c   | 15 ++++++++++-----
>  hw/sparc64/niagara.c | 25 +++++++++++++------------
>  3 files changed, 25 insertions(+), 20 deletions(-)
> 

Re: [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory()

[Igor Mammedov]

On Thu, 10 Oct 2019 19:35:03 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

Forgot to actually CC Eduardo,

> On Tue,  8 Oct 2019 07:33:15 -0400
> Igor Mammedov <imammedo@redhat.com> wrote:
...
> Eduardo,
> 
> This patches are fixing various machines across tree, so series does not belong
> to any particular arch specific tree, can you merge it via generic machine tree?


> > 
> > 
> > Igor Mammedov (3):
> >   sparc64: use memory_region_allocate_system_memory() only for '-m'
> >     specified RAM
> >   ppc: rs6000_mc: drop usage of memory_region_allocate_system_memory()
> >   hppa: drop usage of memory_region_allocate_system_memory() for ROM
> > 
> >  hw/hppa/machine.c    |  5 ++---
> >  hw/ppc/rs6000_mc.c   | 15 ++++++++++-----
> >  hw/sparc64/niagara.c | 25 +++++++++++++------------
> >  3 files changed, 25 insertions(+), 20 deletions(-)
> >   
> 
> 

Re: [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory()

[Philippe Mathieu-Daudé]

Ping?

On Fri, Oct 11, 2019 at 5:23 PM Igor Mammedov <imammedo@redhat.com> wrote:
> On Thu, 10 Oct 2019 19:35:03 +0200
> Igor Mammedov <imammedo@redhat.com> wrote:
>
> Forgot to actually CC Eduardo,
>
> > On Tue,  8 Oct 2019 07:33:15 -0400
> > Igor Mammedov <imammedo@redhat.com> wrote:
> ...
> > Eduardo,
> >
> > This patches are fixing various machines across tree, so series does not belong
> > to any particular arch specific tree, can you merge it via generic machine tree?
>
>
> > >
> > >
> > > Igor Mammedov (3):
> > >   sparc64: use memory_region_allocate_system_memory() only for '-m'
> > >     specified RAM
> > >   ppc: rs6000_mc: drop usage of memory_region_allocate_system_memory()
> > >   hppa: drop usage of memory_region_allocate_system_memory() for ROM
> > >
> > >  hw/hppa/machine.c    |  5 ++---
> > >  hw/ppc/rs6000_mc.c   | 15 ++++++++++-----
> > >  hw/sparc64/niagara.c | 25 +++++++++++++------------
> > >  3 files changed, 25 insertions(+), 20 deletions(-)
> > >

Re: [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory()

[Eduardo Habkost]

On Sun, Oct 20, 2019 at 04:38:06PM +0200, Philippe Mathieu-Daudé wrote:
> Ping?

Sorry, missed this.  queued on machine-next.  Pull request will
be submitted today or tomorrow.

> 
> On Fri, Oct 11, 2019 at 5:23 PM Igor Mammedov <imammedo@redhat.com> wrote:
> > On Thu, 10 Oct 2019 19:35:03 +0200
> > Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > Forgot to actually CC Eduardo,
> >
> > > On Tue,  8 Oct 2019 07:33:15 -0400
> > > Igor Mammedov <imammedo@redhat.com> wrote:
> > ...
> > > Eduardo,
> > >
> > > This patches are fixing various machines across tree, so series does not belong
> > > to any particular arch specific tree, can you merge it via generic machine tree?
> >
> >
> > > >
> > > >
> > > > Igor Mammedov (3):
> > > >   sparc64: use memory_region_allocate_system_memory() only for '-m'
> > > >     specified RAM
> > > >   ppc: rs6000_mc: drop usage of memory_region_allocate_system_memory()
> > > >   hppa: drop usage of memory_region_allocate_system_memory() for ROM
> > > >
> > > >  hw/hppa/machine.c    |  5 ++---
> > > >  hw/ppc/rs6000_mc.c   | 15 ++++++++++-----
> > > >  hw/sparc64/niagara.c | 25 +++++++++++++------------
> > > >  3 files changed, 25 insertions(+), 20 deletions(-)
> > > >

-- 
Eduardo

Re: [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory()

[Philippe Mathieu-Daudé]

Hi Igor,

On 10/8/19 1:33 PM, Igor Mammedov wrote:
> Series cleans up remaining boards that call memory_region_allocate_system_memory()
> multiple times, violating interface contract (the function should be called only
> once).
> 
> With that cleaned up, it should be possible to switch from adhoc RAM allocation
> in memory_region_allocate_system_memory()->allocate_system_memory_nonnuma() to
> memory-backend based allocation, remaining roadblock for doing it is deprecated
> -mem-path fallback to RAM allocation, which is scheduled for removal at 4.3
> merge window. So remaining patches to consolidate system RAM allocation around
> memory-backends and aliasing -mem-path/mem-prealloc to it are postponed till
> then.
> 
> 
> Igor Mammedov (3):
>    sparc64: use memory_region_allocate_system_memory() only for '-m'
>      specified RAM
>    ppc: rs6000_mc: drop usage of memory_region_allocate_system_memory()
>    hppa: drop usage of memory_region_allocate_system_memory() for ROM
> 
>   hw/hppa/machine.c    |  5 ++---
>   hw/ppc/rs6000_mc.c   | 15 ++++++++++-----
>   hw/sparc64/niagara.c | 25 +++++++++++++------------

What about the TYPE_SUN4M_MEMORY device in hw/sparc/sun4m.c?

Re: [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory()

[Igor Mammedov]

On Mon, 21 Oct 2019 10:59:56 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> Hi Igor,
> 
> On 10/8/19 1:33 PM, Igor Mammedov wrote:
> > Series cleans up remaining boards that call memory_region_allocate_system_memory()
> > multiple times, violating interface contract (the function should be called only
> > once).
> > 
> > With that cleaned up, it should be possible to switch from adhoc RAM allocation
> > in memory_region_allocate_system_memory()->allocate_system_memory_nonnuma() to
> > memory-backend based allocation, remaining roadblock for doing it is deprecated
> > -mem-path fallback to RAM allocation, which is scheduled for removal at 4.3
> > merge window. So remaining patches to consolidate system RAM allocation around
> > memory-backends and aliasing -mem-path/mem-prealloc to it are postponed till
> > then.
> > 
> > 
> > Igor Mammedov (3):
> >    sparc64: use memory_region_allocate_system_memory() only for '-m'
> >      specified RAM
> >    ppc: rs6000_mc: drop usage of memory_region_allocate_system_memory()
> >    hppa: drop usage of memory_region_allocate_system_memory() for ROM
> > 
> >   hw/hppa/machine.c    |  5 ++---
> >   hw/ppc/rs6000_mc.c   | 15 ++++++++++-----
> >   hw/sparc64/niagara.c | 25 +++++++++++++------------  
> 
> What about the TYPE_SUN4M_MEMORY device in hw/sparc/sun4m.c?

it has only one call site so it's not issue this series targets.
I'll take care of it in follow up series, which will deal with
converting memory_region_allocate_system_memory() to memdev only
(probably I'll drop TYPE_SUN4M_MEMORY altogether)


>