QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory() 
@ 2019-10-08 11:33 Igor Mammedov
  2019-10-08 11:33 ` [PATCH 1/3] sparc64: use memory_region_allocate_system_memory() only for '-m' specified RAM Igor Mammedov
                   ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Igor Mammedov @ 2019-10-08 11:33 UTC (permalink / raw)
  To: qemu-devel
  Cc: deller, mark.cave-ayland, hpoussin, qemu-ppc, rth, atar4qemu, david


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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/3] sparc64: use memory_region_allocate_system_memory() only for '-m' specified RAM
  2019-10-08 11:33 [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory() Igor Mammedov
@ 2019-10-08 11:33 ` Igor Mammedov
  2019-10-08 12:09   ` Philippe Mathieu-Daudé
  2019-10-08 11:33 ` [PATCH 2/3] ppc: rs6000_mc: drop usage of memory_region_allocate_system_memory() Igor Mammedov
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 15+ messages in thread
From: Igor Mammedov @ 2019-10-08 11:33 UTC (permalink / raw)
  To: qemu-devel
  Cc: deller, mark.cave-ayland, hpoussin, qemu-ppc, rth, atar4qemu, david

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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 2/3] ppc: rs6000_mc: drop usage of memory_region_allocate_system_memory()
  2019-10-08 11:33 [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory() Igor Mammedov
  2019-10-08 11:33 ` [PATCH 1/3] sparc64: use memory_region_allocate_system_memory() only for '-m' specified RAM Igor Mammedov
@ 2019-10-08 11:33 ` Igor Mammedov
  2019-10-08 12:24   ` Philippe Mathieu-Daudé
  2019-10-09  1:21   ` David Gibson
  2019-10-08 11:33 ` [PATCH 3/3] hppa: drop usage of memory_region_allocate_system_memory() for ROM Igor Mammedov
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 15+ messages in thread
From: Igor Mammedov @ 2019-10-08 11:33 UTC (permalink / raw)
  To: qemu-devel
  Cc: deller, mark.cave-ayland, hpoussin, qemu-ppc, rth, atar4qemu, david

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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 3/3] hppa: drop usage of memory_region_allocate_system_memory() for ROM
  2019-10-08 11:33 [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory() Igor Mammedov
  2019-10-08 11:33 ` [PATCH 1/3] sparc64: use memory_region_allocate_system_memory() only for '-m' specified RAM Igor Mammedov
  2019-10-08 11:33 ` [PATCH 2/3] ppc: rs6000_mc: drop usage of memory_region_allocate_system_memory() Igor Mammedov
@ 2019-10-08 11:33 ` Igor Mammedov
  2019-10-08 12:30   ` Philippe Mathieu-Daudé
  2019-10-08 12:41 ` [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory() Philippe Mathieu-Daudé
  2019-10-10 17:35 ` Igor Mammedov
  4 siblings, 1 reply; 15+ messages in thread
From: Igor Mammedov @ 2019-10-08 11:33 UTC (permalink / raw)
  To: qemu-devel
  Cc: deller, mark.cave-ayland, hpoussin, qemu-ppc, rth, atar4qemu, david

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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] sparc64: use memory_region_allocate_system_memory() only for '-m' specified RAM
  2019-10-08 11:33 ` [PATCH 1/3] sparc64: use memory_region_allocate_system_memory() only for '-m' specified RAM Igor Mammedov
@ 2019-10-08 12:09   ` Philippe Mathieu-Daudé
  2019-10-08 13:55     ` Igor Mammedov
  0 siblings, 1 reply; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-10-08 12:09 UTC (permalink / raw)
  To: Igor Mammedov, qemu-devel
  Cc: Peter Maydell, deller, mark.cave-ayland, qemu-ppc, hpoussin,
	david, atar4qemu, rth

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;
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/3] ppc: rs6000_mc: drop usage of memory_region_allocate_system_memory()
  2019-10-08 11:33 ` [PATCH 2/3] ppc: rs6000_mc: drop usage of memory_region_allocate_system_memory() Igor Mammedov
@ 2019-10-08 12:24   ` Philippe Mathieu-Daudé
  2019-10-09  1:21   ` David Gibson
  1 sibling, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-10-08 12:24 UTC (permalink / raw)
  To: Igor Mammedov, qemu-devel
  Cc: deller, mark.cave-ayland, qemu-ppc, hpoussin, david, atar4qemu, rth

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 = {
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/3] hppa: drop usage of memory_region_allocate_system_memory() for ROM
  2019-10-08 11:33 ` [PATCH 3/3] hppa: drop usage of memory_region_allocate_system_memory() for ROM Igor Mammedov
@ 2019-10-08 12:30   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-10-08 12:30 UTC (permalink / raw)
  To: Igor Mammedov, qemu-devel
  Cc: deller, mark.cave-ayland, qemu-ppc, hpoussin, david, atar4qemu, rth

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>


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory()
  2019-10-08 11:33 [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory() Igor Mammedov
                   ` (2 preceding siblings ...)
  2019-10-08 11:33 ` [PATCH 3/3] hppa: drop usage of memory_region_allocate_system_memory() for ROM Igor Mammedov
@ 2019-10-08 12:41 ` Philippe Mathieu-Daudé
  2019-10-08 13:52   ` Igor Mammedov
  2019-10-10 17:35 ` Igor Mammedov
  4 siblings, 1 reply; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-10-08 12:41 UTC (permalink / raw)
  To: Igor Mammedov, qemu-devel
  Cc: deller, mark.cave-ayland, qemu-ppc, hpoussin, david, atar4qemu, rth

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)


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory()
  2019-10-08 12:41 ` [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory() Philippe Mathieu-Daudé
@ 2019-10-08 13:52   ` Igor Mammedov
  0 siblings, 0 replies; 15+ messages in thread
From: Igor Mammedov @ 2019-10-08 13:52 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: deller, mark.cave-ayland, qemu-devel, qemu-ppc, hpoussin, david,
	atar4qemu, rth

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)



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] sparc64: use memory_region_allocate_system_memory() only for '-m' specified RAM
  2019-10-08 12:09   ` Philippe Mathieu-Daudé
@ 2019-10-08 13:55     ` Igor Mammedov
  0 siblings, 0 replies; 15+ messages in thread
From: Igor Mammedov @ 2019-10-08 13:55 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Peter Maydell, deller, mark.cave-ayland, qemu-devel, qemu-ppc,
	hpoussin, rth, atar4qemu, david

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;
> >   
> 



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/3] ppc: rs6000_mc: drop usage of memory_region_allocate_system_memory()
  2019-10-08 11:33 ` [PATCH 2/3] ppc: rs6000_mc: drop usage of memory_region_allocate_system_memory() Igor Mammedov
  2019-10-08 12:24   ` Philippe Mathieu-Daudé
@ 2019-10-09  1:21   ` David Gibson
  2019-10-09 11:19     ` Igor Mammedov
  1 sibling, 1 reply; 15+ messages in thread
From: David Gibson @ 2019-10-09  1:21 UTC (permalink / raw)
  To: Igor Mammedov
  Cc: deller, mark.cave-ayland, qemu-devel, hpoussin, qemu-ppc, atar4qemu, rth

[-- 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 --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/3] ppc: rs6000_mc: drop usage of memory_region_allocate_system_memory()
  2019-10-09  1:21   ` David Gibson
@ 2019-10-09 11:19     ` Igor Mammedov
  2019-10-09 12:02       ` David Gibson
  0 siblings, 1 reply; 15+ messages in thread
From: Igor Mammedov @ 2019-10-09 11:19 UTC (permalink / raw)
  To: David Gibson
  Cc: deller, mark.cave-ayland, qemu-devel, qemu-ppc, hpoussin, atar4qemu, rth

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!


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/3] ppc: rs6000_mc: drop usage of memory_region_allocate_system_memory()
  2019-10-09 11:19     ` Igor Mammedov
@ 2019-10-09 12:02       ` David Gibson
  0 siblings, 0 replies; 15+ messages in thread
From: David Gibson @ 2019-10-09 12:02 UTC (permalink / raw)
  To: Igor Mammedov
  Cc: deller, mark.cave-ayland, qemu-devel, qemu-ppc, hpoussin, atar4qemu, rth

[-- 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 --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory()
  2019-10-08 11:33 [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory() Igor Mammedov
                   ` (3 preceding siblings ...)
  2019-10-08 12:41 ` [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory() Philippe Mathieu-Daudé
@ 2019-10-10 17:35 ` Igor Mammedov
  2019-10-11 15:23   ` Igor Mammedov
  4 siblings, 1 reply; 15+ messages in thread
From: Igor Mammedov @ 2019-10-10 17:35 UTC (permalink / raw)
  To: qemu-devel
  Cc: deller, mark.cave-ayland, qemu-ppc, hpoussin, david, atar4qemu, rth

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(-)
> 



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory()
  2019-10-10 17:35 ` Igor Mammedov
@ 2019-10-11 15:23   ` Igor Mammedov
  0 siblings, 0 replies; 15+ messages in thread
From: Igor Mammedov @ 2019-10-11 15:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eduardo Habkost, deller, mark.cave-ayland, qemu-ppc, hpoussin,
	rth, atar4qemu, david

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(-)
> >   
> 
> 



^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, back to index

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-08 11:33 [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory() Igor Mammedov
2019-10-08 11:33 ` [PATCH 1/3] sparc64: use memory_region_allocate_system_memory() only for '-m' specified RAM Igor Mammedov
2019-10-08 12:09   ` Philippe Mathieu-Daudé
2019-10-08 13:55     ` Igor Mammedov
2019-10-08 11:33 ` [PATCH 2/3] ppc: rs6000_mc: drop usage of memory_region_allocate_system_memory() Igor Mammedov
2019-10-08 12:24   ` Philippe Mathieu-Daudé
2019-10-09  1:21   ` David Gibson
2019-10-09 11:19     ` Igor Mammedov
2019-10-09 12:02       ` David Gibson
2019-10-08 11:33 ` [PATCH 3/3] hppa: drop usage of memory_region_allocate_system_memory() for ROM Igor Mammedov
2019-10-08 12:30   ` Philippe Mathieu-Daudé
2019-10-08 12:41 ` [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory() Philippe Mathieu-Daudé
2019-10-08 13:52   ` Igor Mammedov
2019-10-10 17:35 ` Igor Mammedov
2019-10-11 15:23   ` Igor Mammedov

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org qemu-devel@archiver.kernel.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox