[PATCH v2 07/86] arm:aspeed: convert valid RAM sizes to data

[PATCH v2 07/86] arm:aspeed: convert valid RAM sizes to data

[Igor Mammedov]

various foo_rambits() hardcode mapping of RAM sizes to RAM feature bits,
which is hard to reuse and repeats over and over.

Convert maps into GLib's hash tables and perform mapping using
common mapping function.

Follow up patch will reuse tables for actually checking ram-size
property.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: clg@kaod.org
CC: peter.maydell@linaro.org
CC: andrew@aj.id.au
CC: joel@jms.id.au
CC: qemu-arm@nongnu.org
---
 include/hw/misc/aspeed_sdmc.h |   2 +
 hw/misc/aspeed_sdmc.c         | 116 ++++++++++++++++--------------------------
 2 files changed, 47 insertions(+), 71 deletions(-)

diff --git a/include/hw/misc/aspeed_sdmc.h b/include/hw/misc/aspeed_sdmc.h
index 5dbde59..de1501f 100644
--- a/include/hw/misc/aspeed_sdmc.h
+++ b/include/hw/misc/aspeed_sdmc.h
@@ -39,6 +39,8 @@ typedef struct AspeedSDMCState {
 typedef struct AspeedSDMCClass {
     SysBusDeviceClass parent_class;
 
+    GHashTable *ram2feat;
+    int fallback_ram_size;
     uint64_t max_ram_size;
     uint32_t (*compute_conf)(AspeedSDMCState *s, uint32_t data);
     void (*write)(AspeedSDMCState *s, uint32_t reg, uint32_t data);
diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
index 2df3244..3fc80f0 100644
--- a/hw/misc/aspeed_sdmc.c
+++ b/hw/misc/aspeed_sdmc.c
@@ -148,72 +148,6 @@ static const MemoryRegionOps aspeed_sdmc_ops = {
     .valid.max_access_size = 4,
 };
 
-static int ast2400_rambits(AspeedSDMCState *s)
-{
-    switch (s->ram_size >> 20) {
-    case 64:
-        return ASPEED_SDMC_DRAM_64MB;
-    case 128:
-        return ASPEED_SDMC_DRAM_128MB;
-    case 256:
-        return ASPEED_SDMC_DRAM_256MB;
-    case 512:
-        return ASPEED_SDMC_DRAM_512MB;
-    default:
-        break;
-    }
-
-    /* use a common default */
-    warn_report("Invalid RAM size 0x%" PRIx64 ". Using default 256M",
-                s->ram_size);
-    s->ram_size = 256 << 20;
-    return ASPEED_SDMC_DRAM_256MB;
-}
-
-static int ast2500_rambits(AspeedSDMCState *s)
-{
-    switch (s->ram_size >> 20) {
-    case 128:
-        return ASPEED_SDMC_AST2500_128MB;
-    case 256:
-        return ASPEED_SDMC_AST2500_256MB;
-    case 512:
-        return ASPEED_SDMC_AST2500_512MB;
-    case 1024:
-        return ASPEED_SDMC_AST2500_1024MB;
-    default:
-        break;
-    }
-
-    /* use a common default */
-    warn_report("Invalid RAM size 0x%" PRIx64 ". Using default 512M",
-                s->ram_size);
-    s->ram_size = 512 << 20;
-    return ASPEED_SDMC_AST2500_512MB;
-}
-
-static int ast2600_rambits(AspeedSDMCState *s)
-{
-    switch (s->ram_size >> 20) {
-    case 256:
-        return ASPEED_SDMC_AST2600_256MB;
-    case 512:
-        return ASPEED_SDMC_AST2600_512MB;
-    case 1024:
-        return ASPEED_SDMC_AST2600_1024MB;
-    case 2048:
-        return ASPEED_SDMC_AST2600_2048MB;
-    default:
-        break;
-    }
-
-    /* use a common default */
-    warn_report("Invalid RAM size 0x%" PRIx64 ". Using default 1024M",
-                s->ram_size);
-    s->ram_size = 1024 << 20;
-    return ASPEED_SDMC_AST2600_1024MB;
-}
-
 static void aspeed_sdmc_reset(DeviceState *dev)
 {
     AspeedSDMCState *s = ASPEED_SDMC(dev);
@@ -257,11 +191,14 @@ static Property aspeed_sdmc_properties[] = {
 static void aspeed_sdmc_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    AspeedSDMCClass *asc = ASPEED_SDMC_CLASS(klass);
+
     dc->realize = aspeed_sdmc_realize;
     dc->reset = aspeed_sdmc_reset;
     dc->desc = "ASPEED SDRAM Memory Controller";
     dc->vmsd = &vmstate_aspeed_sdmc;
     dc->props = aspeed_sdmc_properties;
+    asc->ram2feat = g_hash_table_new(g_direct_hash, g_direct_equal);
 }
 
 static const TypeInfo aspeed_sdmc_info = {
@@ -273,10 +210,28 @@ static const TypeInfo aspeed_sdmc_info = {
     .abstract   = true,
 };
 
+static int aspeed_get_ram_feat(AspeedSDMCState *s)
+{
+    AspeedSDMCClass *asc = ASPEED_SDMC_GET_CLASS(s);
+    int ram_mb = s->ram_size >> 20;
+    gpointer val;
+
+    if (g_hash_table_contains(asc->ram2feat, GINT_TO_POINTER(ram_mb))) {
+        val = g_hash_table_lookup(asc->ram2feat, GINT_TO_POINTER(ram_mb));
+        return GPOINTER_TO_INT(val);
+    }
+
+    warn_report("Invalid RAM size 0x%" PRIx64 ". Using default %dM",
+                 s->ram_size, asc->fallback_ram_size);
+    s->ram_size = asc->fallback_ram_size << 20;
+    val = g_hash_table_lookup(asc->ram2feat, &asc->fallback_ram_size);
+    return GPOINTER_TO_INT(val);
+}
+
 static uint32_t aspeed_2400_sdmc_compute_conf(AspeedSDMCState *s, uint32_t data)
 {
-    uint32_t fixed_conf = ASPEED_SDMC_VGA_COMPAT |
-        ASPEED_SDMC_DRAM_SIZE(ast2400_rambits(s));
+    int ram_f = aspeed_get_ram_feat(s);
+    uint32_t fixed_conf = ASPEED_SDMC_VGA_COMPAT | ASPEED_SDMC_DRAM_SIZE(ram_f);
 
     /* Make sure readonly bits are kept */
     data &= ~ASPEED_SDMC_READONLY_MASK;
@@ -298,6 +253,9 @@ static void aspeed_2400_sdmc_write(AspeedSDMCState *s, uint32_t reg,
     s->regs[reg] = data;
 }
 
+#define REGISTER_RAM_SIZE(h, k, v) \
+    g_hash_table_insert(h->ram2feat, GINT_TO_POINTER(k), GINT_TO_POINTER(v))
+
 static void aspeed_2400_sdmc_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -307,6 +265,11 @@ static void aspeed_2400_sdmc_class_init(ObjectClass *klass, void *data)
     asc->max_ram_size = 512 << 20;
     asc->compute_conf = aspeed_2400_sdmc_compute_conf;
     asc->write = aspeed_2400_sdmc_write;
+    asc->fallback_ram_size = 256;
+    REGISTER_RAM_SIZE(asc, 64, ASPEED_SDMC_DRAM_64MB);
+    REGISTER_RAM_SIZE(asc, 128, ASPEED_SDMC_DRAM_128MB);
+    REGISTER_RAM_SIZE(asc, 256, ASPEED_SDMC_DRAM_256MB);
+    REGISTER_RAM_SIZE(asc, 512, ASPEED_SDMC_DRAM_512MB);
 }
 
 static const TypeInfo aspeed_2400_sdmc_info = {
@@ -317,10 +280,10 @@ static const TypeInfo aspeed_2400_sdmc_info = {
 
 static uint32_t aspeed_2500_sdmc_compute_conf(AspeedSDMCState *s, uint32_t data)
 {
+    int ram_f = aspeed_get_ram_feat(s);
     uint32_t fixed_conf = ASPEED_SDMC_HW_VERSION(1) |
         ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
-        ASPEED_SDMC_CACHE_INITIAL_DONE |
-        ASPEED_SDMC_DRAM_SIZE(ast2500_rambits(s));
+        ASPEED_SDMC_CACHE_INITIAL_DONE | ASPEED_SDMC_DRAM_SIZE(ram_f);
 
     /* Make sure readonly bits are kept */
     data &= ~ASPEED_SDMC_AST2500_READONLY_MASK;
@@ -360,6 +323,11 @@ static void aspeed_2500_sdmc_class_init(ObjectClass *klass, void *data)
     asc->max_ram_size = 1024 << 20;
     asc->compute_conf = aspeed_2500_sdmc_compute_conf;
     asc->write = aspeed_2500_sdmc_write;
+    asc->fallback_ram_size = 512;
+    REGISTER_RAM_SIZE(asc, 128, ASPEED_SDMC_AST2500_128MB);
+    REGISTER_RAM_SIZE(asc, 256, ASPEED_SDMC_AST2500_256MB);
+    REGISTER_RAM_SIZE(asc, 512, ASPEED_SDMC_AST2500_512MB);
+    REGISTER_RAM_SIZE(asc, 1024, ASPEED_SDMC_AST2500_1024MB);
 }
 
 static const TypeInfo aspeed_2500_sdmc_info = {
@@ -370,9 +338,10 @@ static const TypeInfo aspeed_2500_sdmc_info = {
 
 static uint32_t aspeed_2600_sdmc_compute_conf(AspeedSDMCState *s, uint32_t data)
 {
+    int ram_f = aspeed_get_ram_feat(s);
     uint32_t fixed_conf = ASPEED_SDMC_HW_VERSION(3) |
         ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
-        ASPEED_SDMC_DRAM_SIZE(ast2600_rambits(s));
+        ASPEED_SDMC_DRAM_SIZE(ram_f);
 
     /* Make sure readonly bits are kept (use ast2500 mask) */
     data &= ~ASPEED_SDMC_AST2500_READONLY_MASK;
@@ -413,6 +382,11 @@ static void aspeed_2600_sdmc_class_init(ObjectClass *klass, void *data)
     asc->max_ram_size = 2048 << 20;
     asc->compute_conf = aspeed_2600_sdmc_compute_conf;
     asc->write = aspeed_2600_sdmc_write;
+    asc->fallback_ram_size = 512;
+    REGISTER_RAM_SIZE(asc, 256, ASPEED_SDMC_AST2600_256MB);
+    REGISTER_RAM_SIZE(asc, 512, ASPEED_SDMC_AST2600_512MB);
+    REGISTER_RAM_SIZE(asc, 1024, ASPEED_SDMC_AST2600_1024MB);
+    REGISTER_RAM_SIZE(asc, 2048, ASPEED_SDMC_AST2600_2048MB);
 }
 
 static const TypeInfo aspeed_2600_sdmc_info = {
-- 
2.7.4

Re: [PATCH v2 07/86] arm:aspeed: convert valid RAM sizes to data

[Joel Stanley]

On Wed, 15 Jan 2020 at 15:10, Igor Mammedov <imammedo@redhat.com> wrote:
>
> various foo_rambits() hardcode mapping of RAM sizes to RAM feature bits,
> which is hard to reuse and repeats over and over.
>
> Convert maps into GLib's hash tables and perform mapping using
> common mapping function.

Thanks for the patch.

I find the existing code straight forward to understand, and for this
reason I would prefer to leave it as it is. Would you mind dropping
this patch from your series?

>
> Follow up patch will reuse tables for actually checking ram-size
> property.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> CC: clg@kaod.org
> CC: peter.maydell@linaro.org
> CC: andrew@aj.id.au
> CC: joel@jms.id.au
> CC: qemu-arm@nongnu.org
> ---
>  include/hw/misc/aspeed_sdmc.h |   2 +
>  hw/misc/aspeed_sdmc.c         | 116 ++++++++++++++++--------------------------
>  2 files changed, 47 insertions(+), 71 deletions(-)
>
> diff --git a/include/hw/misc/aspeed_sdmc.h b/include/hw/misc/aspeed_sdmc.h
> index 5dbde59..de1501f 100644
> --- a/include/hw/misc/aspeed_sdmc.h
> +++ b/include/hw/misc/aspeed_sdmc.h
> @@ -39,6 +39,8 @@ typedef struct AspeedSDMCState {
>  typedef struct AspeedSDMCClass {
>      SysBusDeviceClass parent_class;
>
> +    GHashTable *ram2feat;
> +    int fallback_ram_size;
>      uint64_t max_ram_size;
>      uint32_t (*compute_conf)(AspeedSDMCState *s, uint32_t data);
>      void (*write)(AspeedSDMCState *s, uint32_t reg, uint32_t data);
> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
> index 2df3244..3fc80f0 100644
> --- a/hw/misc/aspeed_sdmc.c
> +++ b/hw/misc/aspeed_sdmc.c
> @@ -148,72 +148,6 @@ static const MemoryRegionOps aspeed_sdmc_ops = {
>      .valid.max_access_size = 4,
>  };
>
> -static int ast2400_rambits(AspeedSDMCState *s)
> -{
> -    switch (s->ram_size >> 20) {
> -    case 64:
> -        return ASPEED_SDMC_DRAM_64MB;
> -    case 128:
> -        return ASPEED_SDMC_DRAM_128MB;
> -    case 256:
> -        return ASPEED_SDMC_DRAM_256MB;
> -    case 512:
> -        return ASPEED_SDMC_DRAM_512MB;
> -    default:
> -        break;
> -    }
> -
> -    /* use a common default */
> -    warn_report("Invalid RAM size 0x%" PRIx64 ". Using default 256M",
> -                s->ram_size);
> -    s->ram_size = 256 << 20;
> -    return ASPEED_SDMC_DRAM_256MB;
> -}
> -
> -static int ast2500_rambits(AspeedSDMCState *s)
> -{
> -    switch (s->ram_size >> 20) {
> -    case 128:
> -        return ASPEED_SDMC_AST2500_128MB;
> -    case 256:
> -        return ASPEED_SDMC_AST2500_256MB;
> -    case 512:
> -        return ASPEED_SDMC_AST2500_512MB;
> -    case 1024:
> -        return ASPEED_SDMC_AST2500_1024MB;
> -    default:
> -        break;
> -    }
> -
> -    /* use a common default */
> -    warn_report("Invalid RAM size 0x%" PRIx64 ". Using default 512M",
> -                s->ram_size);
> -    s->ram_size = 512 << 20;
> -    return ASPEED_SDMC_AST2500_512MB;
> -}
> -
> -static int ast2600_rambits(AspeedSDMCState *s)
> -{
> -    switch (s->ram_size >> 20) {
> -    case 256:
> -        return ASPEED_SDMC_AST2600_256MB;
> -    case 512:
> -        return ASPEED_SDMC_AST2600_512MB;
> -    case 1024:
> -        return ASPEED_SDMC_AST2600_1024MB;
> -    case 2048:
> -        return ASPEED_SDMC_AST2600_2048MB;
> -    default:
> -        break;
> -    }
> -
> -    /* use a common default */
> -    warn_report("Invalid RAM size 0x%" PRIx64 ". Using default 1024M",
> -                s->ram_size);
> -    s->ram_size = 1024 << 20;
> -    return ASPEED_SDMC_AST2600_1024MB;
> -}
> -
>  static void aspeed_sdmc_reset(DeviceState *dev)
>  {
>      AspeedSDMCState *s = ASPEED_SDMC(dev);
> @@ -257,11 +191,14 @@ static Property aspeed_sdmc_properties[] = {
>  static void aspeed_sdmc_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> +    AspeedSDMCClass *asc = ASPEED_SDMC_CLASS(klass);
> +
>      dc->realize = aspeed_sdmc_realize;
>      dc->reset = aspeed_sdmc_reset;
>      dc->desc = "ASPEED SDRAM Memory Controller";
>      dc->vmsd = &vmstate_aspeed_sdmc;
>      dc->props = aspeed_sdmc_properties;
> +    asc->ram2feat = g_hash_table_new(g_direct_hash, g_direct_equal);
>  }
>
>  static const TypeInfo aspeed_sdmc_info = {
> @@ -273,10 +210,28 @@ static const TypeInfo aspeed_sdmc_info = {
>      .abstract   = true,
>  };
>
> +static int aspeed_get_ram_feat(AspeedSDMCState *s)
> +{
> +    AspeedSDMCClass *asc = ASPEED_SDMC_GET_CLASS(s);
> +    int ram_mb = s->ram_size >> 20;
> +    gpointer val;
> +
> +    if (g_hash_table_contains(asc->ram2feat, GINT_TO_POINTER(ram_mb))) {
> +        val = g_hash_table_lookup(asc->ram2feat, GINT_TO_POINTER(ram_mb));
> +        return GPOINTER_TO_INT(val);
> +    }
> +
> +    warn_report("Invalid RAM size 0x%" PRIx64 ". Using default %dM",
> +                 s->ram_size, asc->fallback_ram_size);
> +    s->ram_size = asc->fallback_ram_size << 20;
> +    val = g_hash_table_lookup(asc->ram2feat, &asc->fallback_ram_size);
> +    return GPOINTER_TO_INT(val);
> +}
> +
>  static uint32_t aspeed_2400_sdmc_compute_conf(AspeedSDMCState *s, uint32_t data)
>  {
> -    uint32_t fixed_conf = ASPEED_SDMC_VGA_COMPAT |
> -        ASPEED_SDMC_DRAM_SIZE(ast2400_rambits(s));
> +    int ram_f = aspeed_get_ram_feat(s);
> +    uint32_t fixed_conf = ASPEED_SDMC_VGA_COMPAT | ASPEED_SDMC_DRAM_SIZE(ram_f);
>
>      /* Make sure readonly bits are kept */
>      data &= ~ASPEED_SDMC_READONLY_MASK;
> @@ -298,6 +253,9 @@ static void aspeed_2400_sdmc_write(AspeedSDMCState *s, uint32_t reg,
>      s->regs[reg] = data;
>  }
>
> +#define REGISTER_RAM_SIZE(h, k, v) \
> +    g_hash_table_insert(h->ram2feat, GINT_TO_POINTER(k), GINT_TO_POINTER(v))
> +
>  static void aspeed_2400_sdmc_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -307,6 +265,11 @@ static void aspeed_2400_sdmc_class_init(ObjectClass *klass, void *data)
>      asc->max_ram_size = 512 << 20;
>      asc->compute_conf = aspeed_2400_sdmc_compute_conf;
>      asc->write = aspeed_2400_sdmc_write;
> +    asc->fallback_ram_size = 256;
> +    REGISTER_RAM_SIZE(asc, 64, ASPEED_SDMC_DRAM_64MB);
> +    REGISTER_RAM_SIZE(asc, 128, ASPEED_SDMC_DRAM_128MB);
> +    REGISTER_RAM_SIZE(asc, 256, ASPEED_SDMC_DRAM_256MB);
> +    REGISTER_RAM_SIZE(asc, 512, ASPEED_SDMC_DRAM_512MB);
>  }
>
>  static const TypeInfo aspeed_2400_sdmc_info = {
> @@ -317,10 +280,10 @@ static const TypeInfo aspeed_2400_sdmc_info = {
>
>  static uint32_t aspeed_2500_sdmc_compute_conf(AspeedSDMCState *s, uint32_t data)
>  {
> +    int ram_f = aspeed_get_ram_feat(s);
>      uint32_t fixed_conf = ASPEED_SDMC_HW_VERSION(1) |
>          ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
> -        ASPEED_SDMC_CACHE_INITIAL_DONE |
> -        ASPEED_SDMC_DRAM_SIZE(ast2500_rambits(s));
> +        ASPEED_SDMC_CACHE_INITIAL_DONE | ASPEED_SDMC_DRAM_SIZE(ram_f);
>
>      /* Make sure readonly bits are kept */
>      data &= ~ASPEED_SDMC_AST2500_READONLY_MASK;
> @@ -360,6 +323,11 @@ static void aspeed_2500_sdmc_class_init(ObjectClass *klass, void *data)
>      asc->max_ram_size = 1024 << 20;
>      asc->compute_conf = aspeed_2500_sdmc_compute_conf;
>      asc->write = aspeed_2500_sdmc_write;
> +    asc->fallback_ram_size = 512;
> +    REGISTER_RAM_SIZE(asc, 128, ASPEED_SDMC_AST2500_128MB);
> +    REGISTER_RAM_SIZE(asc, 256, ASPEED_SDMC_AST2500_256MB);
> +    REGISTER_RAM_SIZE(asc, 512, ASPEED_SDMC_AST2500_512MB);
> +    REGISTER_RAM_SIZE(asc, 1024, ASPEED_SDMC_AST2500_1024MB);
>  }
>
>  static const TypeInfo aspeed_2500_sdmc_info = {
> @@ -370,9 +338,10 @@ static const TypeInfo aspeed_2500_sdmc_info = {
>
>  static uint32_t aspeed_2600_sdmc_compute_conf(AspeedSDMCState *s, uint32_t data)
>  {
> +    int ram_f = aspeed_get_ram_feat(s);
>      uint32_t fixed_conf = ASPEED_SDMC_HW_VERSION(3) |
>          ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
> -        ASPEED_SDMC_DRAM_SIZE(ast2600_rambits(s));
> +        ASPEED_SDMC_DRAM_SIZE(ram_f);
>
>      /* Make sure readonly bits are kept (use ast2500 mask) */
>      data &= ~ASPEED_SDMC_AST2500_READONLY_MASK;
> @@ -413,6 +382,11 @@ static void aspeed_2600_sdmc_class_init(ObjectClass *klass, void *data)
>      asc->max_ram_size = 2048 << 20;
>      asc->compute_conf = aspeed_2600_sdmc_compute_conf;
>      asc->write = aspeed_2600_sdmc_write;
> +    asc->fallback_ram_size = 512;
> +    REGISTER_RAM_SIZE(asc, 256, ASPEED_SDMC_AST2600_256MB);
> +    REGISTER_RAM_SIZE(asc, 512, ASPEED_SDMC_AST2600_512MB);
> +    REGISTER_RAM_SIZE(asc, 1024, ASPEED_SDMC_AST2600_1024MB);
> +    REGISTER_RAM_SIZE(asc, 2048, ASPEED_SDMC_AST2600_2048MB);
>  }
>
>  static const TypeInfo aspeed_2600_sdmc_info = {
> --
> 2.7.4
>

[PATCH v2 08/86] arm:aspeed: actually check RAM size

[Igor Mammedov]

It's supposed that SOC will check if "-m" provided
RAM size is valid by setting "ram-size" property and
then board would read back valid (possibly corrected
value) to map RAM MemoryReging with valid size.
Well it isn't doing so, since check is called only
indirectly from
  aspeed_sdmc_reset()->asc->compute_conf()
or much later when guest writes to configuration
register.

So depending on "-m" value QEMU end-ups with a warning
and an invalid MemoryRegion size allocated and mapped.
(examples:
 -M ast2500-evb -m 1M
    0000000080000000-000000017ffffffe (prio 0, i/o): aspeed-ram-container
      0000000080000000-00000000800fffff (prio 0, ram): ram
      0000000080100000-00000000bfffffff (prio 0, i/o): max_ram
 -M ast2500-evb -m 3G
    0000000080000000-000000017ffffffe (prio 0, i/o): aspeed-ram-container
      0000000080000000-000000013fffffff (prio 0, ram): ram
      [DETECTED OVERFLOW!] 0000000140000000-00000000bfffffff (prio 0, i/o): max_ram
)
On top of that sdmc falls back and reports to guest
"default" size, it thinks machine should have.

I don't know how hardware is supposed to work so
I've kept it as is.
But as for CLI side machine should honor whatever
user configured or error out to make user fix CLI.

This patch makes ram-size check actually work and
changes behavior from a warning later on during
machine reset to error_fatal at the moment SOC is
realized so user will have to fix RAM size on CLI
to start machine.

It also gets out of the way mutable ram-size logic,
so we could consolidate RAM allocation logic around
pre-allocated hostmem backend (supplied by user or
auto created by generic machine code depending on
supplied -m/mem-path/mem-prealloc options.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: clg@kaod.org
CC: peter.maydell@linaro.org
CC: andrew@aj.id.au
CC: joel@jms.id.au
CC: qemu-arm@nongnu.org
---
 hw/arm/aspeed.c       | 9 +--------
 hw/misc/aspeed_sdmc.c | 5 +++++
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index cc06af4..525c547 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -213,14 +213,7 @@ static void aspeed_machine_init(MachineState *machine)
                                 "hw-prot-key", &error_abort);
     }
     object_property_set_bool(OBJECT(&bmc->soc), true, "realized",
-                             &error_abort);
-
-    /*
-     * Allocate RAM after the memory controller has checked the size
-     * was valid. If not, a default value is used.
-     */
-    ram_size = object_property_get_uint(OBJECT(&bmc->soc), "ram-size",
-                                        &error_abort);
+                             &error_fatal);
 
     memory_region_allocate_system_memory(&bmc->ram, NULL, "ram", ram_size);
     memory_region_add_subregion(&bmc->ram_container, 0, &bmc->ram);
diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
index 3fc80f0..b398e36 100644
--- a/hw/misc/aspeed_sdmc.c
+++ b/hw/misc/aspeed_sdmc.c
@@ -165,6 +165,11 @@ static void aspeed_sdmc_realize(DeviceState *dev, Error **errp)
     AspeedSDMCState *s = ASPEED_SDMC(dev);
     AspeedSDMCClass *asc = ASPEED_SDMC_GET_CLASS(s);
 
+    if (!g_hash_table_contains(asc->ram2feat,
+                               GINT_TO_POINTER(s->ram_size >> 20))) {
+        error_setg(errp, "Invalid RAM size 0x%" PRIx64, s->ram_size);
+        return;
+    }
     s->max_ram_size = asc->max_ram_size;
 
     memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_sdmc_ops, s,
-- 
2.7.4

Re: [PATCH v2 08/86] arm:aspeed: actually check RAM size

[Cédric Le Goater]

On 1/15/20 4:06 PM, Igor Mammedov wrote:
> It's supposed that SOC will check if "-m" provided
> RAM size is valid by setting "ram-size" property and
> then board would read back valid (possibly corrected
> value) to map RAM MemoryReging with valid size.
> Well it isn't doing so, since check is called only
> indirectly from
>   aspeed_sdmc_reset()->asc->compute_conf()
> or much later when guest writes to configuration
> register.
> 
> So depending on "-m" value QEMU end-ups with a warning
> and an invalid MemoryRegion size allocated and mapped.
> (examples:
>  -M ast2500-evb -m 1M
>     0000000080000000-000000017ffffffe (prio 0, i/o): aspeed-ram-container
>       0000000080000000-00000000800fffff (prio 0, ram): ram
>       0000000080100000-00000000bfffffff (prio 0, i/o): max_ram
>  -M ast2500-evb -m 3G
>     0000000080000000-000000017ffffffe (prio 0, i/o): aspeed-ram-container
>       0000000080000000-000000013fffffff (prio 0, ram): ram
>       [DETECTED OVERFLOW!] 0000000140000000-00000000bfffffff (prio 0, i/o): max_ram
> )
> On top of that sdmc falls back and reports to guest
> "default" size, it thinks machine should have.>
> I don't know how hardware is supposed to work so
> I've kept it as is.

The HW is hardwired and the modeling is trying to accommodate with
the '-m' option, the machine definition and the SDRAM controller
limits and register definitions for a given SoC. The result is not 
that good it seems :/ 

> But as for CLI side machine should honor whatever
> user configured or error out to make user fix CLI.
> 
> This patch makes ram-size check actually work and
> changes behavior from a warning later on during
> machine reset to error_fatal at the moment SOC is
> realized so user will have to fix RAM size on CLI
> to start machine.

commit 8e00d1a97d1d ("aspeed/sdmc: Introduce an object class per SoC") 
moved some calls from the realize handler to reset handler and it
broke the checks on the RAM size.

I think we should introduce get/set handlers on the "ram-size" property
that would look for a matching size in an AspeedSDMCClass array of valid
RAM sizes. The default size of the machine would be a valid default and
bogus user defined sizes would be fatal to QEMU.  

We could get rid of the code :

    /* use a common default */
    warn_report("Invalid RAM size 0x%" PRIx64 ". Using default 512M",
                s->ram_size);
    s->ram_size = 512 << 20;
    return ASPEED_SDMC_AST2500_512MB;


'max_ram_size' would be the last entry of the AspeedSDMCClass array
and, anyhow, we need to check bmc->max_ram is really needed. I am not 
sure anymore. 

Thanks,

C. 

> It also gets out of the way mutable ram-size logic,
> so we could consolidate RAM allocation logic around
> pre-allocated hostmem backend (supplied by user or
> auto created by generic machine code depending on
> supplied -m/mem-path/mem-prealloc options.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> CC: clg@kaod.org
> CC: peter.maydell@linaro.org
> CC: andrew@aj.id.au
> CC: joel@jms.id.au
> CC: qemu-arm@nongnu.org
> ---
>  hw/arm/aspeed.c       | 9 +--------
>  hw/misc/aspeed_sdmc.c | 5 +++++
>  2 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index cc06af4..525c547 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -213,14 +213,7 @@ static void aspeed_machine_init(MachineState *machine)
>                                  "hw-prot-key", &error_abort);
>      }
>      object_property_set_bool(OBJECT(&bmc->soc), true, "realized",
> -                             &error_abort);
> -
> -    /*
> -     * Allocate RAM after the memory controller has checked the size
> -     * was valid. If not, a default value is used.
> -     */
> -    ram_size = object_property_get_uint(OBJECT(&bmc->soc), "ram-size",
> -                                        &error_abort);
> +                             &error_fatal);
>  
>      memory_region_allocate_system_memory(&bmc->ram, NULL, "ram", ram_size);
>      memory_region_add_subregion(&bmc->ram_container, 0, &bmc->ram);
> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
> index 3fc80f0..b398e36 100644
> --- a/hw/misc/aspeed_sdmc.c
> +++ b/hw/misc/aspeed_sdmc.c
> @@ -165,6 +165,11 @@ static void aspeed_sdmc_realize(DeviceState *dev, Error **errp)
>      AspeedSDMCState *s = ASPEED_SDMC(dev);
>      AspeedSDMCClass *asc = ASPEED_SDMC_GET_CLASS(s);
>  
> +    if (!g_hash_table_contains(asc->ram2feat,
> +                               GINT_TO_POINTER(s->ram_size >> 20))) {
> +        error_setg(errp, "Invalid RAM size 0x%" PRIx64, s->ram_size);
> +        return;
> +    }
>      s->max_ram_size = asc->max_ram_size;
>  
>      memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_sdmc_ops, s,
> 

Re: [PATCH v2 08/86] arm:aspeed: actually check RAM size

[Igor Mammedov]

On Thu, 16 Jan 2020 09:41:03 +0100
Cédric Le Goater <clg@kaod.org> wrote:

> On 1/15/20 4:06 PM, Igor Mammedov wrote:
> > It's supposed that SOC will check if "-m" provided
> > RAM size is valid by setting "ram-size" property and
> > then board would read back valid (possibly corrected
> > value) to map RAM MemoryReging with valid size.
> > Well it isn't doing so, since check is called only
> > indirectly from
> >   aspeed_sdmc_reset()->asc->compute_conf()
> > or much later when guest writes to configuration
> > register.
> > 
> > So depending on "-m" value QEMU end-ups with a warning
> > and an invalid MemoryRegion size allocated and mapped.
> > (examples:
> >  -M ast2500-evb -m 1M
> >     0000000080000000-000000017ffffffe (prio 0, i/o): aspeed-ram-container
> >       0000000080000000-00000000800fffff (prio 0, ram): ram
> >       0000000080100000-00000000bfffffff (prio 0, i/o): max_ram
> >  -M ast2500-evb -m 3G
> >     0000000080000000-000000017ffffffe (prio 0, i/o): aspeed-ram-container
> >       0000000080000000-000000013fffffff (prio 0, ram): ram
> >       [DETECTED OVERFLOW!] 0000000140000000-00000000bfffffff (prio 0, i/o): max_ram
> > )
> > On top of that sdmc falls back and reports to guest
> > "default" size, it thinks machine should have.>
> > I don't know how hardware is supposed to work so
> > I've kept it as is.  
> 
> The HW is hardwired and the modeling is trying to accommodate with
> the '-m' option, the machine definition and the SDRAM controller
> limits and register definitions for a given SoC. The result is not 
> that good it seems :/ 
> 
> > But as for CLI side machine should honor whatever
> > user configured or error out to make user fix CLI.
> > 
> > This patch makes ram-size check actually work and
> > changes behavior from a warning later on during
> > machine reset to error_fatal at the moment SOC is
> > realized so user will have to fix RAM size on CLI
> > to start machine.  
> 
> commit 8e00d1a97d1d ("aspeed/sdmc: Introduce an object class per SoC") 
> moved some calls from the realize handler to reset handler and it
> broke the checks on the RAM size.
> 
> I think we should introduce get/set handlers on the "ram-size" property
> that would look for a matching size in an AspeedSDMCClass array of valid
> RAM sizes. The default size of the machine would be a valid default and
> bogus user defined sizes would be fatal to QEMU.  

That's what I'm after, i.e. board either accepts user asked size or exits
with error. So as far as there aren't any fix-ups it should work for
the purpose of this series

> 
> We could get rid of the code :
> 
>     /* use a common default */
>     warn_report("Invalid RAM size 0x%" PRIx64 ". Using default 512M",
>                 s->ram_size);
>     s->ram_size = 512 << 20;
>     return ASPEED_SDMC_AST2500_512MB;
> 
> 
> 'max_ram_size' would be the last entry of the AspeedSDMCClass array
> and, anyhow, we need to check bmc->max_ram is really needed. I am not 
> sure anymore. 

I'll rework aspeed parts taking in account feedback.

> 
> Thanks,
> 
> C. 
> 
> > It also gets out of the way mutable ram-size logic,
> > so we could consolidate RAM allocation logic around
> > pre-allocated hostmem backend (supplied by user or
> > auto created by generic machine code depending on
> > supplied -m/mem-path/mem-prealloc options.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > CC: clg@kaod.org
> > CC: peter.maydell@linaro.org
> > CC: andrew@aj.id.au
> > CC: joel@jms.id.au
> > CC: qemu-arm@nongnu.org
> > ---
> >  hw/arm/aspeed.c       | 9 +--------
> >  hw/misc/aspeed_sdmc.c | 5 +++++
> >  2 files changed, 6 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> > index cc06af4..525c547 100644
> > --- a/hw/arm/aspeed.c
> > +++ b/hw/arm/aspeed.c
> > @@ -213,14 +213,7 @@ static void aspeed_machine_init(MachineState *machine)
> >                                  "hw-prot-key", &error_abort);
> >      }
> >      object_property_set_bool(OBJECT(&bmc->soc), true, "realized",
> > -                             &error_abort);
> > -
> > -    /*
> > -     * Allocate RAM after the memory controller has checked the size
> > -     * was valid. If not, a default value is used.
> > -     */
> > -    ram_size = object_property_get_uint(OBJECT(&bmc->soc), "ram-size",
> > -                                        &error_abort);
> > +                             &error_fatal);
> >  
> >      memory_region_allocate_system_memory(&bmc->ram, NULL, "ram", ram_size);
> >      memory_region_add_subregion(&bmc->ram_container, 0, &bmc->ram);
> > diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
> > index 3fc80f0..b398e36 100644
> > --- a/hw/misc/aspeed_sdmc.c
> > +++ b/hw/misc/aspeed_sdmc.c
> > @@ -165,6 +165,11 @@ static void aspeed_sdmc_realize(DeviceState *dev, Error **errp)
> >      AspeedSDMCState *s = ASPEED_SDMC(dev);
> >      AspeedSDMCClass *asc = ASPEED_SDMC_GET_CLASS(s);
> >  
> > +    if (!g_hash_table_contains(asc->ram2feat,
> > +                               GINT_TO_POINTER(s->ram_size >> 20))) {
> > +        error_setg(errp, "Invalid RAM size 0x%" PRIx64, s->ram_size);
> > +        return;
> > +    }
> >      s->max_ram_size = asc->max_ram_size;
> >  
> >      memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_sdmc_ops, s,
> >   
> 
> 

Re: [PATCH v2 08/86] arm:aspeed: actually check RAM size

[Cédric Le Goater]

On 1/16/20 6:35 PM, Igor Mammedov wrote:
> On Thu, 16 Jan 2020 09:41:03 +0100
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> On 1/15/20 4:06 PM, Igor Mammedov wrote:
>>> It's supposed that SOC will check if "-m" provided
>>> RAM size is valid by setting "ram-size" property and
>>> then board would read back valid (possibly corrected
>>> value) to map RAM MemoryReging with valid size.
>>> Well it isn't doing so, since check is called only
>>> indirectly from
>>>   aspeed_sdmc_reset()->asc->compute_conf()
>>> or much later when guest writes to configuration
>>> register.
>>>
>>> So depending on "-m" value QEMU end-ups with a warning
>>> and an invalid MemoryRegion size allocated and mapped.
>>> (examples:
>>>  -M ast2500-evb -m 1M
>>>     0000000080000000-000000017ffffffe (prio 0, i/o): aspeed-ram-container
>>>       0000000080000000-00000000800fffff (prio 0, ram): ram
>>>       0000000080100000-00000000bfffffff (prio 0, i/o): max_ram
>>>  -M ast2500-evb -m 3G
>>>     0000000080000000-000000017ffffffe (prio 0, i/o): aspeed-ram-container
>>>       0000000080000000-000000013fffffff (prio 0, ram): ram
>>>       [DETECTED OVERFLOW!] 0000000140000000-00000000bfffffff (prio 0, i/o): max_ram
>>> )
>>> On top of that sdmc falls back and reports to guest
>>> "default" size, it thinks machine should have.>
>>> I don't know how hardware is supposed to work so
>>> I've kept it as is.  
>>
>> The HW is hardwired and the modeling is trying to accommodate with
>> the '-m' option, the machine definition and the SDRAM controller
>> limits and register definitions for a given SoC. The result is not 
>> that good it seems :/ 
>>
>>> But as for CLI side machine should honor whatever
>>> user configured or error out to make user fix CLI.
>>>
>>> This patch makes ram-size check actually work and
>>> changes behavior from a warning later on during
>>> machine reset to error_fatal at the moment SOC is
>>> realized so user will have to fix RAM size on CLI
>>> to start machine.  
>>
>> commit 8e00d1a97d1d ("aspeed/sdmc: Introduce an object class per SoC") 
>> moved some calls from the realize handler to reset handler and it
>> broke the checks on the RAM size.
>>
>> I think we should introduce get/set handlers on the "ram-size" property
>> that would look for a matching size in an AspeedSDMCClass array of valid
>> RAM sizes. The default size of the machine would be a valid default and
>> bogus user defined sizes would be fatal to QEMU.  
> 
> That's what I'm after, i.e. board either accepts user asked size or exits
> with error. 

We should be able to catch bogus values with :

    object_property_set_uint(OBJECT(&bmc->soc), ram_size, "ram-size",
                             &error_abort);

in aspeed_machine_init() and let QEMU exit.

> So as far as there aren't any fix-ups it should work for
> the purpose of this series
>
>>
>> We could get rid of the code :
>>
>>     /* use a common default */
>>     warn_report("Invalid RAM size 0x%" PRIx64 ". Using default 512M",
>>                 s->ram_size);
>>     s->ram_size = 512 << 20;
>>     return ASPEED_SDMC_AST2500_512MB;
>>
>>
>> 'max_ram_size' would be the last entry of the AspeedSDMCClass array
>> and, anyhow, we need to check bmc->max_ram is really needed. I am not 
>> sure anymore. 
> 
> I'll rework aspeed parts taking in account feedback.

OK. We will give them a try. I don't think you have to bother with 
bmc->max_ram for the moment. It doesn't seem to be in your way. 

Thanks,

C. 

>>> It also gets out of the way mutable ram-size logic,
>>> so we could consolidate RAM allocation logic around
>>> pre-allocated hostmem backend (supplied by user or
>>> auto created by generic machine code depending on
>>> supplied -m/mem-path/mem-prealloc options.
>>>
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
>>> CC: clg@kaod.org
>>> CC: peter.maydell@linaro.org
>>> CC: andrew@aj.id.au
>>> CC: joel@jms.id.au
>>> CC: qemu-arm@nongnu.org
>>> ---
>>>  hw/arm/aspeed.c       | 9 +--------
>>>  hw/misc/aspeed_sdmc.c | 5 +++++
>>>  2 files changed, 6 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>>> index cc06af4..525c547 100644
>>> --- a/hw/arm/aspeed.c
>>> +++ b/hw/arm/aspeed.c
>>> @@ -213,14 +213,7 @@ static void aspeed_machine_init(MachineState *machine)
>>>                                  "hw-prot-key", &error_abort);
>>>      }
>>>      object_property_set_bool(OBJECT(&bmc->soc), true, "realized",
>>> -                             &error_abort);
>>> -
>>> -    /*
>>> -     * Allocate RAM after the memory controller has checked the size
>>> -     * was valid. If not, a default value is used.
>>> -     */
>>> -    ram_size = object_property_get_uint(OBJECT(&bmc->soc), "ram-size",
>>> -                                        &error_abort);
>>> +                             &error_fatal);
>>>  
>>>      memory_region_allocate_system_memory(&bmc->ram, NULL, "ram", ram_size);
>>>      memory_region_add_subregion(&bmc->ram_container, 0, &bmc->ram);
>>> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
>>> index 3fc80f0..b398e36 100644
>>> --- a/hw/misc/aspeed_sdmc.c
>>> +++ b/hw/misc/aspeed_sdmc.c
>>> @@ -165,6 +165,11 @@ static void aspeed_sdmc_realize(DeviceState *dev, Error **errp)
>>>      AspeedSDMCState *s = ASPEED_SDMC(dev);
>>>      AspeedSDMCClass *asc = ASPEED_SDMC_GET_CLASS(s);
>>>  
>>> +    if (!g_hash_table_contains(asc->ram2feat,
>>> +                               GINT_TO_POINTER(s->ram_size >> 20))) {
>>> +        error_setg(errp, "Invalid RAM size 0x%" PRIx64, s->ram_size);
>>> +        return;
>>> +    }
>>>      s->max_ram_size = asc->max_ram_size;
>>>  
>>>      memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_sdmc_ops, s,
>>>   
>>
>>
> 

[PATCH v3 07/84] hw/arm/aspeed: actually check RAM size

[Igor Mammedov]

It's supposed that SOC will check if "-m" provided
RAM size is valid by setting "ram-size" property and
then board would read back valid (possibly corrected
value) to map RAM MemoryReging with valid size.
It isn't doing so, since check is called only
indirectly from
  aspeed_sdmc_reset()->asc->compute_conf()
or much later when guest writes to configuration
register.

So depending on "-m" value QEMU end-ups with a warning
and an invalid MemoryRegion size allocated and mapped.
(examples:
 -M ast2500-evb -m 1M
    0000000080000000-000000017ffffffe (prio 0, i/o): aspeed-ram-container
      0000000080000000-00000000800fffff (prio 0, ram): ram
      0000000080100000-00000000bfffffff (prio 0, i/o): max_ram
 -M ast2500-evb -m 3G
    0000000080000000-000000017ffffffe (prio 0, i/o): aspeed-ram-container
      0000000080000000-000000013fffffff (prio 0, ram): ram
      [DETECTED OVERFLOW!] 0000000140000000-00000000bfffffff (prio 0, i/o): max_ram
)
On top of that sdmc falls back and reports to guest
"default" size, it thinks machine should have.

This patch makes ram-size check actually work and
changes behavior from a warning later on during
machine reset to error_fatal at the moment SOC.ram-size
is set so user will have to fix RAM size on CLI
to start machine.

It also gets out of the way mutable ram-size logic,
so we could consolidate RAM allocation logic around
pre-allocated hostmem backend (supplied by user or
auto created by generic machine code depending on
supplied -m/mem-path/mem-prealloc options.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v3:
  * replace
     [PATCH v2 07/86] arm:aspeed: convert valid RAM sizes to data
     [PATCH v2 08/86] arm:aspeed: actually check RAM size
    with a simplified variant that adds ram_size check to sdmc.ram-size
    property

CC: clg@kaod.org
CC: peter.maydell@linaro.org
CC: andrew@aj.id.au
CC: joel@jms.id.au
CC: qemu-arm@nongnu.org

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/misc/aspeed_sdmc.h |  1 +
 hw/arm/aspeed.c               | 13 +++-----
 hw/misc/aspeed_sdmc.c         | 77 +++++++++++++++++++++++++++++++++++--------
 3 files changed, 70 insertions(+), 21 deletions(-)

diff --git a/include/hw/misc/aspeed_sdmc.h b/include/hw/misc/aspeed_sdmc.h
index 5dbde59..cea1e67 100644
--- a/include/hw/misc/aspeed_sdmc.h
+++ b/include/hw/misc/aspeed_sdmc.h
@@ -40,6 +40,7 @@ typedef struct AspeedSDMCClass {
     SysBusDeviceClass parent_class;
 
     uint64_t max_ram_size;
+    const uint64_t *valid_ram_sizes;
     uint32_t (*compute_conf)(AspeedSDMCState *s, uint32_t data);
     void (*write)(AspeedSDMCState *s, uint32_t reg, uint32_t data);
 } AspeedSDMCClass;
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index cc06af4..c8573e5 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -191,8 +191,12 @@ static void aspeed_machine_init(MachineState *machine)
 
     sc = ASPEED_SOC_GET_CLASS(&bmc->soc);
 
+    /*
+     * This will error out if isize is not supported by memory controller.
+     */
     object_property_set_uint(OBJECT(&bmc->soc), ram_size, "ram-size",
-                             &error_abort);
+                             &error_fatal);
+
     object_property_set_int(OBJECT(&bmc->soc), amc->hw_strap1, "hw-strap1",
                             &error_abort);
     object_property_set_int(OBJECT(&bmc->soc), amc->hw_strap2, "hw-strap2",
@@ -215,13 +219,6 @@ static void aspeed_machine_init(MachineState *machine)
     object_property_set_bool(OBJECT(&bmc->soc), true, "realized",
                              &error_abort);
 
-    /*
-     * Allocate RAM after the memory controller has checked the size
-     * was valid. If not, a default value is used.
-     */
-    ram_size = object_property_get_uint(OBJECT(&bmc->soc), "ram-size",
-                                        &error_abort);
-
     memory_region_allocate_system_memory(&bmc->ram, NULL, "ram", ram_size);
     memory_region_add_subregion(&bmc->ram_container, 0, &bmc->ram);
     memory_region_add_subregion(get_system_memory(),
diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
index 2df3244..b36b362 100644
--- a/hw/misc/aspeed_sdmc.c
+++ b/hw/misc/aspeed_sdmc.c
@@ -17,6 +17,9 @@
 #include "migration/vmstate.h"
 #include "qapi/error.h"
 #include "trace.h"
+#include "qemu/units.h"
+#include "qemu/cutils.h"
+#include "qapi/visitor.h"
 
 /* Protection Key Register */
 #define R_PROT            (0x00 / 4)
@@ -163,10 +166,7 @@ static int ast2400_rambits(AspeedSDMCState *s)
         break;
     }
 
-    /* use a common default */
-    warn_report("Invalid RAM size 0x%" PRIx64 ". Using default 256M",
-                s->ram_size);
-    s->ram_size = 256 << 20;
+    assert(0);
     return ASPEED_SDMC_DRAM_256MB;
 }
 
@@ -185,10 +185,7 @@ static int ast2500_rambits(AspeedSDMCState *s)
         break;
     }
 
-    /* use a common default */
-    warn_report("Invalid RAM size 0x%" PRIx64 ". Using default 512M",
-                s->ram_size);
-    s->ram_size = 512 << 20;
+    assert(0);
     return ASPEED_SDMC_AST2500_512MB;
 }
 
@@ -207,10 +204,7 @@ static int ast2600_rambits(AspeedSDMCState *s)
         break;
     }
 
-    /* use a common default */
-    warn_report("Invalid RAM size 0x%" PRIx64 ". Using default 1024M",
-                s->ram_size);
-    s->ram_size = 1024 << 20;
+    assert(0);
     return ASPEED_SDMC_AST2600_1024MB;
 }
 
@@ -225,6 +219,51 @@ static void aspeed_sdmc_reset(DeviceState *dev)
     s->regs[R_CONF] = asc->compute_conf(s, 0);
 }
 
+static void aspeed_sdmc_get_ram_size(Object *obj, Visitor *v, const char *name,
+                                     void *opaque, Error **errp)
+{
+    AspeedSDMCState *s = ASPEED_SDMC(obj);
+    int64_t value = s->ram_size;
+
+    visit_type_int(v, name, &value, errp);
+}
+
+static void aspeed_sdmc_set_ram_size(Object *obj, Visitor *v, const char *name,
+                                     void *opaque, Error **errp)
+{
+    int i;
+    char *sz;
+    int64_t value;
+    Error *local_err = NULL;
+    AspeedSDMCState *s = ASPEED_SDMC(obj);
+    AspeedSDMCClass *asc = ASPEED_SDMC_GET_CLASS(s);
+
+    visit_type_int(v, name, &value, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    for (i = 0; asc->valid_ram_sizes[i]; i++) {
+        if (value == asc->valid_ram_sizes[i]) {
+            s->ram_size = value;
+            return;
+        }
+    }
+
+    sz = size_to_str(value);
+    error_setg(&local_err, "Invalid RAM size %s", sz);
+    g_free(sz);
+    error_propagate(errp, local_err);
+}
+
+static void aspeed_sdmc_initfn(Object *obj)
+{
+    object_property_add(obj, "ram-size", "int",
+                        aspeed_sdmc_get_ram_size, aspeed_sdmc_set_ram_size,
+                        NULL, NULL, NULL);
+}
+
 static void aspeed_sdmc_realize(DeviceState *dev, Error **errp)
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
@@ -249,7 +288,6 @@ static const VMStateDescription vmstate_aspeed_sdmc = {
 };
 
 static Property aspeed_sdmc_properties[] = {
-    DEFINE_PROP_UINT64("ram-size", AspeedSDMCState, ram_size, 0),
     DEFINE_PROP_UINT64("max-ram-size", AspeedSDMCState, max_ram_size, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
@@ -268,6 +306,7 @@ static const TypeInfo aspeed_sdmc_info = {
     .name = TYPE_ASPEED_SDMC,
     .parent = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(AspeedSDMCState),
+    .instance_init = aspeed_sdmc_initfn,
     .class_init = aspeed_sdmc_class_init,
     .class_size = sizeof(AspeedSDMCClass),
     .abstract   = true,
@@ -298,6 +337,9 @@ static void aspeed_2400_sdmc_write(AspeedSDMCState *s, uint32_t reg,
     s->regs[reg] = data;
 }
 
+static const uint64_t
+aspeed_2400_ram_sizes[] = { 64 * MiB, 128 * MiB, 256 * MiB, 512 * MiB, 0};
+
 static void aspeed_2400_sdmc_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -307,6 +349,7 @@ static void aspeed_2400_sdmc_class_init(ObjectClass *klass, void *data)
     asc->max_ram_size = 512 << 20;
     asc->compute_conf = aspeed_2400_sdmc_compute_conf;
     asc->write = aspeed_2400_sdmc_write;
+    asc->valid_ram_sizes = aspeed_2400_ram_sizes;
 }
 
 static const TypeInfo aspeed_2400_sdmc_info = {
@@ -351,6 +394,9 @@ static void aspeed_2500_sdmc_write(AspeedSDMCState *s, uint32_t reg,
     s->regs[reg] = data;
 }
 
+static const uint64_t
+aspeed_2500_ram_sizes[] = { 128 * MiB, 256 * MiB, 512 * MiB, 1024 * MiB, 0};
+
 static void aspeed_2500_sdmc_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -360,6 +406,7 @@ static void aspeed_2500_sdmc_class_init(ObjectClass *klass, void *data)
     asc->max_ram_size = 1024 << 20;
     asc->compute_conf = aspeed_2500_sdmc_compute_conf;
     asc->write = aspeed_2500_sdmc_write;
+    asc->valid_ram_sizes = aspeed_2500_ram_sizes;
 }
 
 static const TypeInfo aspeed_2500_sdmc_info = {
@@ -404,6 +451,9 @@ static void aspeed_2600_sdmc_write(AspeedSDMCState *s, uint32_t reg,
     s->regs[reg] = data;
 }
 
+static const uint64_t
+aspeed_2600_ram_sizes[] = { 256 * MiB, 512 * MiB, 1024 * MiB, 2048 * MiB, 0};
+
 static void aspeed_2600_sdmc_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -413,6 +463,7 @@ static void aspeed_2600_sdmc_class_init(ObjectClass *klass, void *data)
     asc->max_ram_size = 2048 << 20;
     asc->compute_conf = aspeed_2600_sdmc_compute_conf;
     asc->write = aspeed_2600_sdmc_write;
+    asc->valid_ram_sizes = aspeed_2600_ram_sizes;
 }
 
 static const TypeInfo aspeed_2600_sdmc_info = {
-- 
2.7.4

Re: [PATCH v3 07/84] hw/arm/aspeed: actually check RAM size

[Cédric Le Goater]

On 1/20/20 3:21 PM, Igor Mammedov wrote:
> It's supposed that SOC will check if "-m" provided
> RAM size is valid by setting "ram-size" property and
> then board would read back valid (possibly corrected
> value) to map RAM MemoryReging with valid size.
> It isn't doing so, since check is called only
> indirectly from
>   aspeed_sdmc_reset()->asc->compute_conf()
> or much later when guest writes to configuration
> register.
> 
> So depending on "-m" value QEMU end-ups with a warning
> and an invalid MemoryRegion size allocated and mapped.
> (examples:
>  -M ast2500-evb -m 1M
>     0000000080000000-000000017ffffffe (prio 0, i/o): aspeed-ram-container
>       0000000080000000-00000000800fffff (prio 0, ram): ram
>       0000000080100000-00000000bfffffff (prio 0, i/o): max_ram
>  -M ast2500-evb -m 3G
>     0000000080000000-000000017ffffffe (prio 0, i/o): aspeed-ram-container
>       0000000080000000-000000013fffffff (prio 0, ram): ram
>       [DETECTED OVERFLOW!] 0000000140000000-00000000bfffffff (prio 0, i/o): max_ram
> )
> On top of that sdmc falls back and reports to guest
> "default" size, it thinks machine should have.
> 
> This patch makes ram-size check actually work and
> changes behavior from a warning later on during
> machine reset to error_fatal at the moment SOC.ram-size
> is set so user will have to fix RAM size on CLI
> to start machine.
> 
> It also gets out of the way mutable ram-size logic,
> so we could consolidate RAM allocation logic around
> pre-allocated hostmem backend (supplied by user or
> auto created by generic machine code depending on
> supplied -m/mem-path/mem-prealloc options.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

LGTM, some comments below.

> ---
> v3:
>   * replace
>      [PATCH v2 07/86] arm:aspeed: convert valid RAM sizes to data
>      [PATCH v2 08/86] arm:aspeed: actually check RAM size
>     with a simplified variant that adds ram_size check to sdmc.ram-size
>     property
> 
> CC: clg@kaod.org
> CC: peter.maydell@linaro.org
> CC: andrew@aj.id.au
> CC: joel@jms.id.au
> CC: qemu-arm@nongnu.org
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  include/hw/misc/aspeed_sdmc.h |  1 +
>  hw/arm/aspeed.c               | 13 +++-----
>  hw/misc/aspeed_sdmc.c         | 77 +++++++++++++++++++++++++++++++++++--------
>  3 files changed, 70 insertions(+), 21 deletions(-)
> 
> diff --git a/include/hw/misc/aspeed_sdmc.h b/include/hw/misc/aspeed_sdmc.h
> index 5dbde59..cea1e67 100644
> --- a/include/hw/misc/aspeed_sdmc.h
> +++ b/include/hw/misc/aspeed_sdmc.h
> @@ -40,6 +40,7 @@ typedef struct AspeedSDMCClass {
>      SysBusDeviceClass parent_class;
>  
>      uint64_t max_ram_size;
> +    const uint64_t *valid_ram_sizes;
>      uint32_t (*compute_conf)(AspeedSDMCState *s, uint32_t data);
>      void (*write)(AspeedSDMCState *s, uint32_t reg, uint32_t data);
>  } AspeedSDMCClass;
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index cc06af4..c8573e5 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -191,8 +191,12 @@ static void aspeed_machine_init(MachineState *machine)
>  
>      sc = ASPEED_SOC_GET_CLASS(&bmc->soc);
>  
> +    /*
> +     * This will error out if isize is not supported by memory controller.
> +     */
>      object_property_set_uint(OBJECT(&bmc->soc), ram_size, "ram-size",
> -                             &error_abort);
> +                             &error_fatal);
> +
>      object_property_set_int(OBJECT(&bmc->soc), amc->hw_strap1, "hw-strap1",
>                              &error_abort);
>      object_property_set_int(OBJECT(&bmc->soc), amc->hw_strap2, "hw-strap2",
> @@ -215,13 +219,6 @@ static void aspeed_machine_init(MachineState *machine)
>      object_property_set_bool(OBJECT(&bmc->soc), true, "realized",
>                               &error_abort);
>  
> -    /*
> -     * Allocate RAM after the memory controller has checked the size
> -     * was valid. If not, a default value is used.
> -     */
> -    ram_size = object_property_get_uint(OBJECT(&bmc->soc), "ram-size",
> -                                        &error_abort);
> -
>      memory_region_allocate_system_memory(&bmc->ram, NULL, "ram", ram_size);
>      memory_region_add_subregion(&bmc->ram_container, 0, &bmc->ram);
>      memory_region_add_subregion(get_system_memory(),
> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
> index 2df3244..b36b362 100644
> --- a/hw/misc/aspeed_sdmc.c
> +++ b/hw/misc/aspeed_sdmc.c
> @@ -17,6 +17,9 @@
>  #include "migration/vmstate.h"
>  #include "qapi/error.h"
>  #include "trace.h"
> +#include "qemu/units.h"
> +#include "qemu/cutils.h"
> +#include "qapi/visitor.h"
>  
>  /* Protection Key Register */
>  #define R_PROT            (0x00 / 4)
> @@ -163,10 +166,7 @@ static int ast2400_rambits(AspeedSDMCState *s)
>          break;
>      }

Now that the ramsizes are being checked, I think we can add in the default
statement :

	g_assert_not_reached();


>  
> -    /* use a common default */
> -    warn_report("Invalid RAM size 0x%" PRIx64 ". Using default 256M",
> -                s->ram_size);
> -    s->ram_size = 256 << 20;
> +    assert(0);
>      return ASPEED_SDMC_DRAM_256MB;


and skip the default return value.

A part from that,

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

>  }
>  
> @@ -185,10 +185,7 @@ static int ast2500_rambits(AspeedSDMCState *s)
>          break;
>      }
>  
> -    /* use a common default */
> -    warn_report("Invalid RAM size 0x%" PRIx64 ". Using default 512M",
> -                s->ram_size);
> -    s->ram_size = 512 << 20;
> +    assert(0);
>      return ASPEED_SDMC_AST2500_512MB;
>  }
>  
> @@ -207,10 +204,7 @@ static int ast2600_rambits(AspeedSDMCState *s)
>          break;
>      }
>  
> -    /* use a common default */
> -    warn_report("Invalid RAM size 0x%" PRIx64 ". Using default 1024M",
> -                s->ram_size);
> -    s->ram_size = 1024 << 20;
> +    assert(0);
>      return ASPEED_SDMC_AST2600_1024MB;
>  }
>  
> @@ -225,6 +219,51 @@ static void aspeed_sdmc_reset(DeviceState *dev)
>      s->regs[R_CONF] = asc->compute_conf(s, 0);
>  }
>  
> +static void aspeed_sdmc_get_ram_size(Object *obj, Visitor *v, const char *name,
> +                                     void *opaque, Error **errp)
> +{
> +    AspeedSDMCState *s = ASPEED_SDMC(obj);
> +    int64_t value = s->ram_size;
> +
> +    visit_type_int(v, name, &value, errp);
> +}
> +
> +static void aspeed_sdmc_set_ram_size(Object *obj, Visitor *v, const char *name,
> +                                     void *opaque, Error **errp)
> +{
> +    int i;
> +    char *sz;
> +    int64_t value;
> +    Error *local_err = NULL;
> +    AspeedSDMCState *s = ASPEED_SDMC(obj);
> +    AspeedSDMCClass *asc = ASPEED_SDMC_GET_CLASS(s);
> +
> +    visit_type_int(v, name, &value, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    for (i = 0; asc->valid_ram_sizes[i]; i++) {
> +        if (value == asc->valid_ram_sizes[i]) {
> +            s->ram_size = value;
> +            return;
> +        }
> +    }
> +
> +    sz = size_to_str(value);
> +    error_setg(&local_err, "Invalid RAM size %s", sz);
> +    g_free(sz);
> +    error_propagate(errp, local_err);
> +}
> +
> +static void aspeed_sdmc_initfn(Object *obj)
> +{
> +    object_property_add(obj, "ram-size", "int",
> +                        aspeed_sdmc_get_ram_size, aspeed_sdmc_set_ram_size,
> +                        NULL, NULL, NULL);
> +}
> +
>  static void aspeed_sdmc_realize(DeviceState *dev, Error **errp)
>  {
>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> @@ -249,7 +288,6 @@ static const VMStateDescription vmstate_aspeed_sdmc = {
>  };
>  
>  static Property aspeed_sdmc_properties[] = {
> -    DEFINE_PROP_UINT64("ram-size", AspeedSDMCState, ram_size, 0),
>      DEFINE_PROP_UINT64("max-ram-size", AspeedSDMCState, max_ram_size, 0),
>      DEFINE_PROP_END_OF_LIST(),
>  };
> @@ -268,6 +306,7 @@ static const TypeInfo aspeed_sdmc_info = {
>      .name = TYPE_ASPEED_SDMC,
>      .parent = TYPE_SYS_BUS_DEVICE,
>      .instance_size = sizeof(AspeedSDMCState),
> +    .instance_init = aspeed_sdmc_initfn,
>      .class_init = aspeed_sdmc_class_init,
>      .class_size = sizeof(AspeedSDMCClass),
>      .abstract   = true,
> @@ -298,6 +337,9 @@ static void aspeed_2400_sdmc_write(AspeedSDMCState *s, uint32_t reg,
>      s->regs[reg] = data;
>  }
>  
> +static const uint64_t
> +aspeed_2400_ram_sizes[] = { 64 * MiB, 128 * MiB, 256 * MiB, 512 * MiB, 0};
> +
>  static void aspeed_2400_sdmc_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -307,6 +349,7 @@ static void aspeed_2400_sdmc_class_init(ObjectClass *klass, void *data)
>      asc->max_ram_size = 512 << 20;
>      asc->compute_conf = aspeed_2400_sdmc_compute_conf;
>      asc->write = aspeed_2400_sdmc_write;
> +    asc->valid_ram_sizes = aspeed_2400_ram_sizes;
>  }
>  
>  static const TypeInfo aspeed_2400_sdmc_info = {
> @@ -351,6 +394,9 @@ static void aspeed_2500_sdmc_write(AspeedSDMCState *s, uint32_t reg,
>      s->regs[reg] = data;
>  }
>  
> +static const uint64_t
> +aspeed_2500_ram_sizes[] = { 128 * MiB, 256 * MiB, 512 * MiB, 1024 * MiB, 0};
> +
>  static void aspeed_2500_sdmc_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -360,6 +406,7 @@ static void aspeed_2500_sdmc_class_init(ObjectClass *klass, void *data)
>      asc->max_ram_size = 1024 << 20;
>      asc->compute_conf = aspeed_2500_sdmc_compute_conf;
>      asc->write = aspeed_2500_sdmc_write;
> +    asc->valid_ram_sizes = aspeed_2500_ram_sizes;
>  }
>  
>  static const TypeInfo aspeed_2500_sdmc_info = {
> @@ -404,6 +451,9 @@ static void aspeed_2600_sdmc_write(AspeedSDMCState *s, uint32_t reg,
>      s->regs[reg] = data;
>  }
>  
> +static const uint64_t
> +aspeed_2600_ram_sizes[] = { 256 * MiB, 512 * MiB, 1024 * MiB, 2048 * MiB, 0};
> +
>  static void aspeed_2600_sdmc_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -413,6 +463,7 @@ static void aspeed_2600_sdmc_class_init(ObjectClass *klass, void *data)
>      asc->max_ram_size = 2048 << 20;
>      asc->compute_conf = aspeed_2600_sdmc_compute_conf;
>      asc->write = aspeed_2600_sdmc_write;
> +    asc->valid_ram_sizes = aspeed_2600_ram_sizes;
>  }
>  
>  static const TypeInfo aspeed_2600_sdmc_info = {
> 

[PATCH v2 09/86] hw:aspeed: drop warning and bogus ram_size fixup

[Igor Mammedov]

It was useless to try fixup ram_size and print warning
on guest access to config register to begin with.

Now previous patch made sure that SDMC can not be realized
with invalid RAM size, so there is no case where warning
and not used ram_size fixup could be triggered.

So remove now dead code.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: clg@kaod.org
CC: peter.maydell@linaro.org
CC: andrew@aj.id.au
CC: joel@jms.id.au
CC: qemu-arm@nongnu.org
---
 hw/misc/aspeed_sdmc.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
index b398e36..942b27a 100644
--- a/hw/misc/aspeed_sdmc.c
+++ b/hw/misc/aspeed_sdmc.c
@@ -219,17 +219,8 @@ static int aspeed_get_ram_feat(AspeedSDMCState *s)
 {
     AspeedSDMCClass *asc = ASPEED_SDMC_GET_CLASS(s);
     int ram_mb = s->ram_size >> 20;
-    gpointer val;
+    gpointer val = g_hash_table_lookup(asc->ram2feat, GINT_TO_POINTER(ram_mb));
 
-    if (g_hash_table_contains(asc->ram2feat, GINT_TO_POINTER(ram_mb))) {
-        val = g_hash_table_lookup(asc->ram2feat, GINT_TO_POINTER(ram_mb));
-        return GPOINTER_TO_INT(val);
-    }
-
-    warn_report("Invalid RAM size 0x%" PRIx64 ". Using default %dM",
-                 s->ram_size, asc->fallback_ram_size);
-    s->ram_size = asc->fallback_ram_size << 20;
-    val = g_hash_table_lookup(asc->ram2feat, &asc->fallback_ram_size);
     return GPOINTER_TO_INT(val);
 }
 
-- 
2.7.4

[PATCH v2 10/86] arm:aspeed: use memdev for RAM

[Igor Mammedov]

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.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: clg@kaod.org
CC: peter.maydell@linaro.org
CC: andrew@aj.id.au
CC: joel@jms.id.au
CC: qemu-arm@nongnu.org
---
 hw/arm/aspeed.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 525c547..330254b 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -35,7 +35,6 @@ static struct arm_boot_info aspeed_board_binfo = {
 struct AspeedBoardState {
     AspeedSoCState soc;
     MemoryRegion ram_container;
-    MemoryRegion ram;
     MemoryRegion max_ram;
 };
 
@@ -184,6 +183,7 @@ static void aspeed_machine_init(MachineState *machine)
 
     memory_region_init(&bmc->ram_container, NULL, "aspeed-ram-container",
                        UINT32_MAX);
+    memory_region_add_subregion(&bmc->ram_container, 0, machine->ram);
 
     object_initialize_child(OBJECT(machine), "soc", &bmc->soc,
                             (sizeof(bmc->soc)), amc->soc_name, &error_abort,
@@ -215,8 +215,6 @@ static void aspeed_machine_init(MachineState *machine)
     object_property_set_bool(OBJECT(&bmc->soc), true, "realized",
                              &error_fatal);
 
-    memory_region_allocate_system_memory(&bmc->ram, NULL, "ram", ram_size);
-    memory_region_add_subregion(&bmc->ram_container, 0, &bmc->ram);
     memory_region_add_subregion(get_system_memory(),
                                 sc->memmap[ASPEED_SDRAM],
                                 &bmc->ram_container);
@@ -393,6 +391,7 @@ static void aspeed_machine_class_init(ObjectClass *oc, void *data)
     mc->no_floppy = 1;
     mc->no_cdrom = 1;
     mc->no_parallel = 1;
+    mc->default_ram_id = "ram";
 }
 
 static void aspeed_machine_palmetto_class_init(ObjectClass *oc, void *data)
-- 
2.7.4

Re: [PATCH v2 10/86] arm:aspeed: use memdev for RAM

[Philippe Mathieu-Daudé]

On 1/15/20 4:06 PM, Igor Mammedov wrote:
> 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.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> CC: clg@kaod.org
> CC: peter.maydell@linaro.org
> CC: andrew@aj.id.au
> CC: joel@jms.id.au
> CC: qemu-arm@nongnu.org
> ---
>   hw/arm/aspeed.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 525c547..330254b 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -35,7 +35,6 @@ static struct arm_boot_info aspeed_board_binfo = {
>   struct AspeedBoardState {
>       AspeedSoCState soc;
>       MemoryRegion ram_container;
> -    MemoryRegion ram;
>       MemoryRegion max_ram;
>   };
>   
> @@ -184,6 +183,7 @@ static void aspeed_machine_init(MachineState *machine)
>   
>       memory_region_init(&bmc->ram_container, NULL, "aspeed-ram-container",
>                          UINT32_MAX);
> +    memory_region_add_subregion(&bmc->ram_container, 0, machine->ram);
>   
>       object_initialize_child(OBJECT(machine), "soc", &bmc->soc,
>                               (sizeof(bmc->soc)), amc->soc_name, &error_abort,
> @@ -215,8 +215,6 @@ static void aspeed_machine_init(MachineState *machine)
>       object_property_set_bool(OBJECT(&bmc->soc), true, "realized",
>                                &error_fatal);
>   
> -    memory_region_allocate_system_memory(&bmc->ram, NULL, "ram", ram_size);
> -    memory_region_add_subregion(&bmc->ram_container, 0, &bmc->ram);
>       memory_region_add_subregion(get_system_memory(),
>                                   sc->memmap[ASPEED_SDRAM],
>                                   &bmc->ram_container);
> @@ -393,6 +391,7 @@ static void aspeed_machine_class_init(ObjectClass *oc, void *data)
>       mc->no_floppy = 1;
>       mc->no_cdrom = 1;
>       mc->no_parallel = 1;
> +    mc->default_ram_id = "ram";
>   }
>   
>   static void aspeed_machine_palmetto_class_init(ObjectClass *oc, void *data)
> 

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

Re: [PATCH v2 10/86] arm:aspeed: use memdev for RAM

[Cédric Le Goater]

On 1/15/20 4:06 PM, Igor Mammedov wrote:
> 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.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Cédric Le Goater <clg@kaod.org>


We need to check 'max_ram' is still needed. I remember that old firmwares
were testing the RAM size by doing write/read checks at the top of the RAM.
This was breaking QEMU sometime ago.

C.

> ---
> CC: clg@kaod.org
> CC: peter.maydell@linaro.org
> CC: andrew@aj.id.au
> CC: joel@jms.id.au
> CC: qemu-arm@nongnu.org
> ---
>  hw/arm/aspeed.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 525c547..330254b 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -35,7 +35,6 @@ static struct arm_boot_info aspeed_board_binfo = {
>  struct AspeedBoardState {
>      AspeedSoCState soc;
>      MemoryRegion ram_container;
> -    MemoryRegion ram;
>      MemoryRegion max_ram;
>  };
>  
> @@ -184,6 +183,7 @@ static void aspeed_machine_init(MachineState *machine)
>  
>      memory_region_init(&bmc->ram_container, NULL, "aspeed-ram-container",
>                         UINT32_MAX);
> +    memory_region_add_subregion(&bmc->ram_container, 0, machine->ram);
>  
>      object_initialize_child(OBJECT(machine), "soc", &bmc->soc,
>                              (sizeof(bmc->soc)), amc->soc_name, &error_abort,
> @@ -215,8 +215,6 @@ static void aspeed_machine_init(MachineState *machine)
>      object_property_set_bool(OBJECT(&bmc->soc), true, "realized",
>                               &error_fatal);
>  
> -    memory_region_allocate_system_memory(&bmc->ram, NULL, "ram", ram_size);
> -    memory_region_add_subregion(&bmc->ram_container, 0, &bmc->ram);
>      memory_region_add_subregion(get_system_memory(),
>                                  sc->memmap[ASPEED_SDRAM],
>                                  &bmc->ram_container);
> @@ -393,6 +391,7 @@ static void aspeed_machine_class_init(ObjectClass *oc, void *data)
>      mc->no_floppy = 1;
>      mc->no_cdrom = 1;
>      mc->no_parallel = 1;
> +    mc->default_ram_id = "ram";
>  }
>  
>  static void aspeed_machine_palmetto_class_init(ObjectClass *oc, void *data)
> 

Re: [PATCH v2 10/86] arm:aspeed: use memdev for RAM

[Philippe Mathieu-Daudé]

On 1/16/20 10:24 AM, Cédric Le Goater wrote:
> On 1/15/20 4:06 PM, Igor Mammedov wrote:
>> 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.
>>
>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> 
> We need to check 'max_ram' is still needed. I remember that old firmwares
> were testing the RAM size by doing write/read checks at the top of the RAM.
> This was breaking QEMU sometime ago.

Do you remember any version we can use to test this? Also add to our 
acceptance tests.

> C.
> 
>> ---
>> CC: clg@kaod.org
>> CC: peter.maydell@linaro.org
>> CC: andrew@aj.id.au
>> CC: joel@jms.id.au
>> CC: qemu-arm@nongnu.org
>> ---
>>   hw/arm/aspeed.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index 525c547..330254b 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -35,7 +35,6 @@ static struct arm_boot_info aspeed_board_binfo = {
>>   struct AspeedBoardState {
>>       AspeedSoCState soc;
>>       MemoryRegion ram_container;
>> -    MemoryRegion ram;
>>       MemoryRegion max_ram;
>>   };
>>   
>> @@ -184,6 +183,7 @@ static void aspeed_machine_init(MachineState *machine)
>>   
>>       memory_region_init(&bmc->ram_container, NULL, "aspeed-ram-container",
>>                          UINT32_MAX);
>> +    memory_region_add_subregion(&bmc->ram_container, 0, machine->ram);
>>   
>>       object_initialize_child(OBJECT(machine), "soc", &bmc->soc,
>>                               (sizeof(bmc->soc)), amc->soc_name, &error_abort,
>> @@ -215,8 +215,6 @@ static void aspeed_machine_init(MachineState *machine)
>>       object_property_set_bool(OBJECT(&bmc->soc), true, "realized",
>>                                &error_fatal);
>>   
>> -    memory_region_allocate_system_memory(&bmc->ram, NULL, "ram", ram_size);
>> -    memory_region_add_subregion(&bmc->ram_container, 0, &bmc->ram);
>>       memory_region_add_subregion(get_system_memory(),
>>                                   sc->memmap[ASPEED_SDRAM],
>>                                   &bmc->ram_container);
>> @@ -393,6 +391,7 @@ static void aspeed_machine_class_init(ObjectClass *oc, void *data)
>>       mc->no_floppy = 1;
>>       mc->no_cdrom = 1;
>>       mc->no_parallel = 1;
>> +    mc->default_ram_id = "ram";
>>   }
>>   
>>   static void aspeed_machine_palmetto_class_init(ObjectClass *oc, void *data)
>>
> 
> 

[PATCH v2 11/86] arm:collie: use memdev for RAM

[Igor Mammedov]

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 add check for user supplied RAM size and error
   out if it mismatches board expected value.
 - introduce RAM_ADDR_UFMT to avoid build errors on 32-bit hosts
   when specifying format string for ram_addr_t type

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  * fix format string causing build failure on 32-bit host
    (Philippe Mathieu-Daudé <philmd@redhat.com>)

CC: peter.maydell@linaro.org
CC: qemu-arm@nongnu.org
---
 include/exec/cpu-common.h |  2 ++
 hw/arm/collie.c           | 15 ++++++++++-----
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index 81753bb..0ea4886 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -33,10 +33,12 @@ enum device_endian {
 typedef uint64_t ram_addr_t;
 #  define RAM_ADDR_MAX UINT64_MAX
 #  define RAM_ADDR_FMT "%" PRIx64
+#  define RAM_ADDR_UFMT "%" PRIu64
 #else
 typedef uintptr_t ram_addr_t;
 #  define RAM_ADDR_MAX UINTPTR_MAX
 #  define RAM_ADDR_FMT "%" PRIxPTR
+#  define RAM_ADDR_UFMT "%" PRIuPTR
 #endif
 
 extern ram_addr_t ram_size;
diff --git a/hw/arm/collie.c b/hw/arm/collie.c
index 970a440..176cf09 100644
--- a/hw/arm/collie.c
+++ b/hw/arm/collie.c
@@ -20,20 +20,23 @@
 
 static struct arm_boot_info collie_binfo = {
     .loader_start = SA_SDCS0,
-    .ram_size = 0x20000000,
 };
 
 static void collie_init(MachineState *machine)
 {
     StrongARMState *s;
     DriveInfo *dinfo;
-    MemoryRegion *sdram = g_new(MemoryRegion, 1);
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
+
+    if (machine->ram_size != mc->default_ram_size) {
+        error_report("Invalid RAM size, should be " RAM_ADDR_UFMT " Bytes",
+                     mc->default_ram_size);
+        exit(EXIT_FAILURE);
+    }
 
     s = sa1110_init(machine->cpu_type);
 
-    memory_region_allocate_system_memory(sdram, NULL, "strongarm.sdram",
-                                         collie_binfo.ram_size);
-    memory_region_add_subregion(get_system_memory(), SA_SDCS0, sdram);
+    memory_region_add_subregion(get_system_memory(), SA_SDCS0, machine->ram);
 
     dinfo = drive_get(IF_PFLASH, 0, 0);
     pflash_cfi01_register(SA_CS0, "collie.fl1", 0x02000000,
@@ -57,6 +60,8 @@ static void collie_machine_init(MachineClass *mc)
     mc->init = collie_init;
     mc->ignore_memory_transaction_failures = true;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("sa1110");
+    mc->default_ram_size = 0x20000000;
+    mc->default_ram_id = "strongarm.sdram";
 }
 
 DEFINE_MACHINE("collie", collie_machine_init)
-- 
2.7.4

[PATCH v2 12/86] arm:cubieboard: use memdev for RAM

[Igor Mammedov]

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 <imammedo@redhat.com>
---
CC: b.galvani@gmail.com
CC: peter.maydell@linaro.org
CC: qemu-arm@nongnu.org
---
 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));
-
-    object_property_set_int(OBJECT(&s->a10->emac), 1, "phy-addr", &err);
+    object_property_set_int(OBJECT(&a10->emac), 1, "phy-addr", &err);
     if (err != NULL) {
         error_reportf_err(err, "Couldn't set phy address: ");
         exit(1);
     }
 
-    object_property_set_int(OBJECT(&s->a10->timer), 32768, "clk0-freq", &err);
+    object_property_set_int(OBJECT(&a10->timer), 32768, "clk0-freq", &err);
     if (err != NULL) {
         error_reportf_err(err, "Couldn't set clk0 frequency: ");
         exit(1);
     }
 
-    object_property_set_int(OBJECT(&s->a10->timer), 24000000, "clk1-freq",
-                            &err);
+    object_property_set_int(OBJECT(&a10->timer), 24000000, "clk1-freq", &err);
     if (err != NULL) {
         error_reportf_err(err, "Couldn't set clk1 frequency: ");
         exit(1);
     }
 
-    object_property_set_bool(OBJECT(s->a10), true, "realized", &err);
+    object_property_set_bool(OBJECT(a10), true, "realized", &err);
     if (err != NULL) {
         error_reportf_err(err, "Couldn't realize Allwinner A10: ");
         exit(1);
     }
 
-    memory_region_allocate_system_memory(&s->sdram, NULL, "cubieboard.ram",
-                                         machine->ram_size);
     memory_region_add_subregion(get_system_memory(), AW_A10_SDRAM_BASE,
-                                &s->sdram);
+                                machine->ram);
 
     /* TODO create and connect IDE devices for ide_drive_get() */
 
     cubieboard_binfo.ram_size = machine->ram_size;
-    arm_load_kernel(&s->a10->cpu, machine, &cubieboard_binfo);
+    arm_load_kernel(&a10->cpu, machine, &cubieboard_binfo);
 }
 
 static void cubieboard_machine_init(MachineClass *mc)
@@ -84,6 +74,7 @@ static void cubieboard_machine_init(MachineClass *mc)
     mc->block_default_type = IF_IDE;
     mc->units_per_default_bus = 1;
     mc->ignore_memory_transaction_failures = true;
+    mc->default_ram_id = "cubieboard.ram";
 }
 
 DEFINE_MACHINE("cubieboard", cubieboard_machine_init)
-- 
2.7.4

[PATCH v2 13/86] arm:digic_boards: use memdev for RAM

[Igor Mammedov]

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:
 remove no longer needed DigicBoardState

PS2:
 while at it add check for user supplied RAM size and error
 out if it mismatches board expected value.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  * fix format string causing build failure on 32-bit host
    (Philippe Mathieu-Daudé <philmd@redhat.com>)

CC: antonynpavlov@gmail.com
CC: peter.maydell@linaro.org
CC: qemu-arm@nongnu.org
---
 hw/arm/digic_boards.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/hw/arm/digic_boards.c b/hw/arm/digic_boards.c
index ef3fc2b..9f094d4 100644
--- a/hw/arm/digic_boards.c
+++ b/hw/arm/digic_boards.c
@@ -35,39 +35,38 @@
 #include "hw/loader.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/qtest.h"
+#include "qemu/units.h"
 
 #define DIGIC4_ROM0_BASE      0xf0000000
 #define DIGIC4_ROM1_BASE      0xf8000000
 #define DIGIC4_ROM_MAX_SIZE   0x08000000
 
-typedef struct DigicBoardState {
-    DigicState *digic;
-    MemoryRegion ram;
-} DigicBoardState;
-
 typedef struct DigicBoard {
-    hwaddr ram_size;
-    void (*add_rom0)(DigicBoardState *, hwaddr, const char *);
+    void (*add_rom0)(DigicState *, hwaddr, const char *);
     const char *rom0_def_filename;
-    void (*add_rom1)(DigicBoardState *, hwaddr, const char *);
+    void (*add_rom1)(DigicState *, hwaddr, const char *);
     const char *rom1_def_filename;
 } DigicBoard;
 
-static void digic4_board_init(DigicBoard *board)
+static void digic4_board_init(MachineState *machine, DigicBoard *board)
 {
     Error *err = NULL;
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
 
-    DigicBoardState *s = g_new(DigicBoardState, 1);
+    if (machine->ram_size != mc->default_ram_size) {
+        error_report("Invalid RAM size, should be " RAM_ADDR_UFMT " Bytes",
+                      mc->default_ram_size);
+        exit(EXIT_FAILURE);
+    }
 
-    s->digic = DIGIC(object_new(TYPE_DIGIC));
-    object_property_set_bool(OBJECT(s->digic), true, "realized", &err);
+    DigicState *s = DIGIC(object_new(TYPE_DIGIC));
+    object_property_set_bool(OBJECT(s), true, "realized", &err);
     if (err != NULL) {
         error_reportf_err(err, "Couldn't realize DIGIC SoC: ");
         exit(1);
     }
 
-    memory_region_allocate_system_memory(&s->ram, NULL, "ram", board->ram_size);
-    memory_region_add_subregion(get_system_memory(), 0, &s->ram);
+    memory_region_add_subregion(get_system_memory(), 0, machine->ram);
 
     if (board->add_rom0) {
         board->add_rom0(s, DIGIC4_ROM0_BASE, board->rom0_def_filename);
@@ -78,7 +77,7 @@ static void digic4_board_init(DigicBoard *board)
     }
 }
 
-static void digic_load_rom(DigicBoardState *s, hwaddr addr,
+static void digic_load_rom(DigicState *s, hwaddr addr,
                            hwaddr max_size, const char *def_filename)
 {
     target_long rom_size;
@@ -118,7 +117,7 @@ static void digic_load_rom(DigicBoardState *s, hwaddr addr,
  * Samsung K8P3215UQB
  * 64M Bit (4Mx16) Page Mode / Multi-Bank NOR Flash Memory
  */
-static void digic4_add_k8p3215uqb_rom(DigicBoardState *s, hwaddr addr,
+static void digic4_add_k8p3215uqb_rom(DigicState *s, hwaddr addr,
                                       const char *def_filename)
 {
 #define FLASH_K8P3215UQB_SIZE (4 * 1024 * 1024)
@@ -135,14 +134,13 @@ static void digic4_add_k8p3215uqb_rom(DigicBoardState *s, hwaddr addr,
 }
 
 static DigicBoard digic4_board_canon_a1100 = {
-    .ram_size = 64 * 1024 * 1024,
     .add_rom1 = digic4_add_k8p3215uqb_rom,
     .rom1_def_filename = "canon-a1100-rom1.bin",
 };
 
 static void canon_a1100_init(MachineState *machine)
 {
-    digic4_board_init(&digic4_board_canon_a1100);
+    digic4_board_init(machine, &digic4_board_canon_a1100);
 }
 
 static void canon_a1100_machine_init(MachineClass *mc)
@@ -150,6 +148,8 @@ static void canon_a1100_machine_init(MachineClass *mc)
     mc->desc = "Canon PowerShot A1100 IS";
     mc->init = &canon_a1100_init;
     mc->ignore_memory_transaction_failures = true;
+    mc->default_ram_size = 64 * MiB;
+    mc->default_ram_id = "ram";
 }
 
 DEFINE_MACHINE("canon-a1100", canon_a1100_machine_init)
-- 
2.7.4

[PATCH v2 14/86] arm:highbank: use memdev for RAM

[Igor Mammedov]

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.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: robh@kernel.org
CC: peter.maydell@linaro.org
CC: qemu-arm@nongnu.org
---
 hw/arm/highbank.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
index 518d935..ac9de94 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -236,7 +236,6 @@ enum cxmachines {
  */
 static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
 {
-    ram_addr_t ram_size = machine->ram_size;
     DeviceState *dev = NULL;
     SysBusDevice *busdev;
     qemu_irq pic[128];
@@ -247,7 +246,6 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
     qemu_irq cpu_virq[4];
     qemu_irq cpu_vfiq[4];
     MemoryRegion *sysram;
-    MemoryRegion *dram;
     MemoryRegion *sysmem;
     char *sysboot_filename;
 
@@ -290,10 +288,8 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
     }
 
     sysmem = get_system_memory();
-    dram = g_new(MemoryRegion, 1);
-    memory_region_allocate_system_memory(dram, NULL, "highbank.dram", ram_size);
     /* SDRAM at address zero.  */
-    memory_region_add_subregion(sysmem, 0, dram);
+    memory_region_add_subregion(sysmem, 0, machine->ram);
 
     sysram = g_new(MemoryRegion, 1);
     memory_region_init_ram(sysram, NULL, "highbank.sysram", 0x8000,
@@ -387,7 +383,7 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
 
     /* TODO create and connect IDE devices for ide_drive_get() */
 
-    highbank_binfo.ram_size = ram_size;
+    highbank_binfo.ram_size = machine->ram_size;
     /* highbank requires a dtb in order to boot, and the dtb will override
      * the board ID. The following value is ignored, so set it to -1 to be
      * clear that the value is meaningless.
@@ -430,6 +426,7 @@ static void highbank_class_init(ObjectClass *oc, void *data)
     mc->units_per_default_bus = 1;
     mc->max_cpus = 4;
     mc->ignore_memory_transaction_failures = true;
+    mc->default_ram_id = "highbank.dram";
 }
 
 static const TypeInfo highbank_type = {
@@ -448,6 +445,7 @@ static void midway_class_init(ObjectClass *oc, void *data)
     mc->units_per_default_bus = 1;
     mc->max_cpus = 4;
     mc->ignore_memory_transaction_failures = true;
+    mc->default_ram_id = "highbank.dram";
 }
 
 static const TypeInfo midway_type = {
-- 
2.7.4

Re: [PATCH v2 14/86] arm:highbank: use memdev for RAM

[Philippe Mathieu-Daudé]

On 1/15/20 4:06 PM, Igor Mammedov wrote:
> 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.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> CC: robh@kernel.org
> CC: peter.maydell@linaro.org
> CC: qemu-arm@nongnu.org
> ---
>   hw/arm/highbank.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
> index 518d935..ac9de94 100644
> --- a/hw/arm/highbank.c
> +++ b/hw/arm/highbank.c
> @@ -236,7 +236,6 @@ enum cxmachines {
>    */
>   static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
>   {
> -    ram_addr_t ram_size = machine->ram_size;
>       DeviceState *dev = NULL;
>       SysBusDevice *busdev;
>       qemu_irq pic[128];
> @@ -247,7 +246,6 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
>       qemu_irq cpu_virq[4];
>       qemu_irq cpu_vfiq[4];
>       MemoryRegion *sysram;
> -    MemoryRegion *dram;
>       MemoryRegion *sysmem;
>       char *sysboot_filename;
>   
> @@ -290,10 +288,8 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
>       }
>   
>       sysmem = get_system_memory();
> -    dram = g_new(MemoryRegion, 1);
> -    memory_region_allocate_system_memory(dram, NULL, "highbank.dram", ram_size);
>       /* SDRAM at address zero.  */
> -    memory_region_add_subregion(sysmem, 0, dram);
> +    memory_region_add_subregion(sysmem, 0, machine->ram);
>   
>       sysram = g_new(MemoryRegion, 1);
>       memory_region_init_ram(sysram, NULL, "highbank.sysram", 0x8000,
> @@ -387,7 +383,7 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
>   
>       /* TODO create and connect IDE devices for ide_drive_get() */
>   
> -    highbank_binfo.ram_size = ram_size;
> +    highbank_binfo.ram_size = machine->ram_size;
>       /* highbank requires a dtb in order to boot, and the dtb will override
>        * the board ID. The following value is ignored, so set it to -1 to be
>        * clear that the value is meaningless.
> @@ -430,6 +426,7 @@ static void highbank_class_init(ObjectClass *oc, void *data)
>       mc->units_per_default_bus = 1;
>       mc->max_cpus = 4;
>       mc->ignore_memory_transaction_failures = true;
> +    mc->default_ram_id = "highbank.dram";
>   }
>   
>   static const TypeInfo highbank_type = {
> @@ -448,6 +445,7 @@ static void midway_class_init(ObjectClass *oc, void *data)
>       mc->units_per_default_bus = 1;
>       mc->max_cpus = 4;
>       mc->ignore_memory_transaction_failures = true;
> +    mc->default_ram_id = "highbank.dram";
>   }
>   
>   static const TypeInfo midway_type = {
> 

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

[PATCH v2 15/86] arm:imx25_pdk: drop RAM size fixup

[Igor Mammedov]

If user provided non-sense RAM size, board will complain and
continue running with max RAM size supported.
Also RAM is going to be allocated by generic code, so it won't be
possible for board to fix things up for user.

Make it error message and exit to force user fix CLI,
instead of accepting non-sense CLI values.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: jcd@tribudubois.net
CC: peter.maydell@linaro.org
CC: qemu-arm@nongnu.org
---
 hw/arm/imx25_pdk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm/imx25_pdk.c b/hw/arm/imx25_pdk.c
index c76fc2b..a2b7b35 100644
--- a/hw/arm/imx25_pdk.c
+++ b/hw/arm/imx25_pdk.c
@@ -78,10 +78,10 @@ static void imx25_pdk_init(MachineState *machine)
 
     /* We need to initialize our memory */
     if (machine->ram_size > (FSL_IMX25_SDRAM0_SIZE + FSL_IMX25_SDRAM1_SIZE)) {
-        warn_report("RAM size " RAM_ADDR_FMT " above max supported, "
+        error_report("RAM size " RAM_ADDR_FMT " above max supported, "
                     "reduced to %x", machine->ram_size,
                     FSL_IMX25_SDRAM0_SIZE + FSL_IMX25_SDRAM1_SIZE);
-        machine->ram_size = FSL_IMX25_SDRAM0_SIZE + FSL_IMX25_SDRAM1_SIZE;
+        exit(EXIT_FAILURE);
     }
 
     memory_region_allocate_system_memory(&s->ram, NULL, "imx25.ram",
-- 
2.7.4

[PATCH v2 16/86] arm:imx25_pdk: use memdev for RAM

[Igor Mammedov]

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.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: jcd@tribudubois.net
CC: peter.maydell@linaro.org
CC: qemu-arm@nongnu.org
---
 hw/arm/imx25_pdk.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/arm/imx25_pdk.c b/hw/arm/imx25_pdk.c
index a2b7b35..9087fcb 100644
--- a/hw/arm/imx25_pdk.c
+++ b/hw/arm/imx25_pdk.c
@@ -58,7 +58,6 @@
 
 typedef struct IMX25PDK {
     FslIMX25State soc;
-    MemoryRegion ram;
     MemoryRegion ram_alias;
 } IMX25PDK;
 
@@ -84,10 +83,8 @@ static void imx25_pdk_init(MachineState *machine)
         exit(EXIT_FAILURE);
     }
 
-    memory_region_allocate_system_memory(&s->ram, NULL, "imx25.ram",
-                                         machine->ram_size);
     memory_region_add_subregion(get_system_memory(), FSL_IMX25_SDRAM0_ADDR,
-                                &s->ram);
+                                machine->ram);
 
     /* initialize the alias memory if any */
     for (i = 0, ram_size = machine->ram_size, alias_offset = 0;
@@ -107,7 +104,8 @@ static void imx25_pdk_init(MachineState *machine)
 
         if (size < ram[i].size) {
             memory_region_init_alias(&s->ram_alias, NULL, "ram.alias",
-                                     &s->ram, alias_offset, ram[i].size - size);
+                                     machine->ram,
+                                     alias_offset, ram[i].size - size);
             memory_region_add_subregion(get_system_memory(),
                                         ram[i].addr + size, &s->ram_alias);
         }
@@ -135,6 +133,7 @@ static void imx25_pdk_machine_init(MachineClass *mc)
     mc->desc = "ARM i.MX25 PDK board (ARM926)";
     mc->init = imx25_pdk_init;
     mc->ignore_memory_transaction_failures = true;
+    mc->default_ram_id = "imx25.ram";
 }
 
 DEFINE_MACHINE("imx25-pdk", imx25_pdk_machine_init)
-- 
2.7.4

Re: [PATCH v2 16/86] arm:imx25_pdk: use memdev for RAM

[Philippe Mathieu-Daudé]

On 1/15/20 4:06 PM, Igor Mammedov wrote:
> 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.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> CC: jcd@tribudubois.net
> CC: peter.maydell@linaro.org
> CC: qemu-arm@nongnu.org
> ---
>   hw/arm/imx25_pdk.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/arm/imx25_pdk.c b/hw/arm/imx25_pdk.c
> index a2b7b35..9087fcb 100644
> --- a/hw/arm/imx25_pdk.c
> +++ b/hw/arm/imx25_pdk.c
> @@ -58,7 +58,6 @@
>   
>   typedef struct IMX25PDK {
>       FslIMX25State soc;
> -    MemoryRegion ram;
>       MemoryRegion ram_alias;
>   } IMX25PDK;
>   
> @@ -84,10 +83,8 @@ static void imx25_pdk_init(MachineState *machine)
>           exit(EXIT_FAILURE);
>       }
>   
> -    memory_region_allocate_system_memory(&s->ram, NULL, "imx25.ram",
> -                                         machine->ram_size);
>       memory_region_add_subregion(get_system_memory(), FSL_IMX25_SDRAM0_ADDR,
> -                                &s->ram);
> +                                machine->ram);
>   
>       /* initialize the alias memory if any */
>       for (i = 0, ram_size = machine->ram_size, alias_offset = 0;
> @@ -107,7 +104,8 @@ static void imx25_pdk_init(MachineState *machine)
>   
>           if (size < ram[i].size) {
>               memory_region_init_alias(&s->ram_alias, NULL, "ram.alias",
> -                                     &s->ram, alias_offset, ram[i].size - size);
> +                                     machine->ram,
> +                                     alias_offset, ram[i].size - size);
>               memory_region_add_subregion(get_system_memory(),
>                                           ram[i].addr + size, &s->ram_alias);
>           }
> @@ -135,6 +133,7 @@ static void imx25_pdk_machine_init(MachineClass *mc)
>       mc->desc = "ARM i.MX25 PDK board (ARM926)";
>       mc->init = imx25_pdk_init;
>       mc->ignore_memory_transaction_failures = true;
> +    mc->default_ram_id = "imx25.ram";
>   }
>   
>   DEFINE_MACHINE("imx25-pdk", imx25_pdk_machine_init)
> 

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

[PATCH v2 17/86] arm:integratorcp: use memdev for RAM

[Igor Mammedov]

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.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: peter.chubb@nicta.com.au
CC: peter.maydell@linaro.org
CC: qemu-arm@nongnu.org
---
 hw/arm/integratorcp.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
index 5249708..89c223b 100644
--- a/hw/arm/integratorcp.c
+++ b/hw/arm/integratorcp.c
@@ -585,7 +585,6 @@ static void integratorcp_init(MachineState *machine)
     Object *cpuobj;
     ARMCPU *cpu;
     MemoryRegion *address_space_mem = get_system_memory();
-    MemoryRegion *ram = g_new(MemoryRegion, 1);
     MemoryRegion *ram_alias = g_new(MemoryRegion, 1);
     qemu_irq pic[32];
     DeviceState *dev, *sic, *icp;
@@ -605,14 +604,13 @@ static void integratorcp_init(MachineState *machine)
 
     cpu = ARM_CPU(cpuobj);
 
-    memory_region_allocate_system_memory(ram, NULL, "integrator.ram",
-                                         ram_size);
     /* ??? On a real system the first 1Mb is mapped as SSRAM or boot flash.  */
     /* ??? RAM should repeat to fill physical memory space.  */
     /* SDRAM at address zero*/
-    memory_region_add_subregion(address_space_mem, 0, ram);
+    memory_region_add_subregion(address_space_mem, 0, machine->ram);
     /* And again at address 0x80000000 */
-    memory_region_init_alias(ram_alias, NULL, "ram.alias", ram, 0, ram_size);
+    memory_region_init_alias(ram_alias, NULL, "ram.alias", machine->ram,
+                             0, ram_size);
     memory_region_add_subregion(address_space_mem, 0x80000000, ram_alias);
 
     dev = qdev_create(NULL, TYPE_INTEGRATOR_CM);
@@ -660,6 +658,7 @@ static void integratorcp_machine_init(MachineClass *mc)
     mc->init = integratorcp_init;
     mc->ignore_memory_transaction_failures = true;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("arm926");
+    mc->default_ram_id = "integrator.ram";
 }
 
 DEFINE_MACHINE("integratorcp", integratorcp_machine_init)
-- 
2.7.4

[PATCH v2 18/86] arm:kzm: drop RAM size fixup

[Igor Mammedov]

If user provided non-sense RAM size, board will complain and
continue running with max RAM size supported.
Also RAM is going to be allocated by generic code, so it won't be
possible for board to fix things up for user.

Make it error message and exit to force user fix CLI,
instead of accepting non-sense CLI values.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: peter.chubb@nicta.com.au
CC: peter.maydell@linaro.org
CC: qemu-arm@nongnu.org
---
 hw/arm/kzm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm/kzm.c b/hw/arm/kzm.c
index 1d5ef28..27800c5 100644
--- a/hw/arm/kzm.c
+++ b/hw/arm/kzm.c
@@ -78,10 +78,10 @@ static void kzm_init(MachineState *machine)
 
     /* Check the amount of memory is compatible with the SOC */
     if (machine->ram_size > (FSL_IMX31_SDRAM0_SIZE + FSL_IMX31_SDRAM1_SIZE)) {
-        warn_report("RAM size " RAM_ADDR_FMT " above max supported, "
+        error_report("RAM size " RAM_ADDR_FMT " above max supported, "
                     "reduced to %x", machine->ram_size,
                     FSL_IMX31_SDRAM0_SIZE + FSL_IMX31_SDRAM1_SIZE);
-        machine->ram_size = FSL_IMX31_SDRAM0_SIZE + FSL_IMX31_SDRAM1_SIZE;
+        exit(EXIT_FAILURE);
     }
 
     memory_region_allocate_system_memory(&s->ram, NULL, "kzm.ram",
-- 
2.7.4

Re: [PATCH v2 18/86] arm:kzm: drop RAM size fixup

[Chubb, Peter (Data61, Kensington NSW)]

>>>>> "Igor" == Igor Mammedov <imammedo@redhat.com> writes:

Igor> If user provided non-sense RAM size, board will complain and
Igor> continue running with max RAM size supported.  Also RAM is going
Igor> to be allocated by generic code, so it won't be possible for
Igor> board to fix things up for user.

Igor> Make it error message and exit to force user fix CLI, instead of
Igor> accepting non-sense CLI values.
 I think this comment needs rewording a little.  Maybe:
   If the user provided too large a RAM size, the code used to
   complain and trim it to the max size.  Now tht RAM is allocated by
   generic code, that's no longer possible, so generate an error and
   exit instead.


Igor>    /* Check the amount of memory is compatible with the SOC */
Igor>   if (machine->ram_size > (FSL_IMX31_SDRAM0_SIZE +
Igor>   FSL_IMX31_SDRAM1_SIZE)) {
Igor> - warn_report("RAM size " RAM_ADDR_FMT " above max supported, "
Igor> + error_report("RAM size " RAM_ADDR_FMT " above max supported, "
Igor>                "reduced to %x", machine->ram_size,
Igor>                FSL_IMX31_SDRAM0_SIZE + FSL_IMX31_SDRAM1_SIZE);

        This needs to be changed so it doesn't say that the RAM size
	is reduced, just what the maximum is.  Maybe:
	error_report("RAM size " RAM_ADDR_FMT " above max (%x) supported.",
	machine->ram_size, FSL_IMX31_SDRAM0_SIZE +
	FSL_IMX31_SDRAM1_SIZE);


Peter C

[PATCH v3 18/86] arm:kzm: drop RAM size fixup

[Igor Mammedov]

If the user provided too large a RAM size, the code used to
complain and trim it to the max size.  Now tht RAM is allocated by
generic code, that's no longer possible, so generate an error and
exit instead.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v3:
 * rephrase commit message in nicer way
   ("Chubb, Peter (Data61, Kensington NSW)" <Peter.Chubb@data61.csiro.au>)
 * reword error message and use size_to_str() to pretty print suggested size
   ("Chubb, Peter (Data61, Kensington NSW)" <Peter.Chubb@data61.csiro.au>)

CC: peter.chubb@nicta.com.au
CC: peter.maydell@linaro.org
CC: qemu-arm@nongnu.org
---
 hw/arm/kzm.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/arm/kzm.c b/hw/arm/kzm.c
index 1d5ef28..94cbac1 100644
--- a/hw/arm/kzm.c
+++ b/hw/arm/kzm.c
@@ -25,6 +25,7 @@
 #include "hw/char/serial.h"
 #include "sysemu/qtest.h"
 #include "sysemu/sysemu.h"
+#include "qemu/cutils.h"
 
 /* Memory map for Kzm Emulation Baseboard:
  * 0x00000000-0x7fffffff See i.MX31 SOC for support
@@ -78,10 +79,10 @@ static void kzm_init(MachineState *machine)
 
     /* Check the amount of memory is compatible with the SOC */
     if (machine->ram_size > (FSL_IMX31_SDRAM0_SIZE + FSL_IMX31_SDRAM1_SIZE)) {
-        warn_report("RAM size " RAM_ADDR_FMT " above max supported, "
-                    "reduced to %x", machine->ram_size,
-                    FSL_IMX31_SDRAM0_SIZE + FSL_IMX31_SDRAM1_SIZE);
-        machine->ram_size = FSL_IMX31_SDRAM0_SIZE + FSL_IMX31_SDRAM1_SIZE;
+        char *sz = size_to_str(FSL_IMX31_SDRAM0_SIZE + FSL_IMX31_SDRAM1_SIZE);
+        error_report("RAM size more than %s is not supported", sz);
+        g_free(sz);
+        exit(EXIT_FAILURE);
     }
 
     memory_region_allocate_system_memory(&s->ram, NULL, "kzm.ram",
-- 
2.7.4

Re: [PATCH v3 18/86] arm:kzm: drop RAM size fixup

[Philippe Mathieu-Daudé]

On 1/16/20 6:26 PM, Igor Mammedov wrote:
> If the user provided too large a RAM size, the code used to
> complain and trim it to the max size.  Now tht RAM is allocated by
> generic code, that's no longer possible, so generate an error and
> exit instead.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v3:
>   * rephrase commit message in nicer way
>     ("Chubb, Peter (Data61, Kensington NSW)" <Peter.Chubb@data61.csiro.au>)
>   * reword error message and use size_to_str() to pretty print suggested size
>     ("Chubb, Peter (Data61, Kensington NSW)" <Peter.Chubb@data61.csiro.au>)
> 
> CC: peter.chubb@nicta.com.au
> CC: peter.maydell@linaro.org
> CC: qemu-arm@nongnu.org
> ---
>   hw/arm/kzm.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/kzm.c b/hw/arm/kzm.c
> index 1d5ef28..94cbac1 100644
> --- a/hw/arm/kzm.c
> +++ b/hw/arm/kzm.c
> @@ -25,6 +25,7 @@
>   #include "hw/char/serial.h"
>   #include "sysemu/qtest.h"
>   #include "sysemu/sysemu.h"
> +#include "qemu/cutils.h"
>   
>   /* Memory map for Kzm Emulation Baseboard:
>    * 0x00000000-0x7fffffff See i.MX31 SOC for support
> @@ -78,10 +79,10 @@ static void kzm_init(MachineState *machine)
>   
>       /* Check the amount of memory is compatible with the SOC */
>       if (machine->ram_size > (FSL_IMX31_SDRAM0_SIZE + FSL_IMX31_SDRAM1_SIZE)) {
> -        warn_report("RAM size " RAM_ADDR_FMT " above max supported, "
> -                    "reduced to %x", machine->ram_size,
> -                    FSL_IMX31_SDRAM0_SIZE + FSL_IMX31_SDRAM1_SIZE);
> -        machine->ram_size = FSL_IMX31_SDRAM0_SIZE + FSL_IMX31_SDRAM1_SIZE;
> +        char *sz = size_to_str(FSL_IMX31_SDRAM0_SIZE + FSL_IMX31_SDRAM1_SIZE);
> +        error_report("RAM size more than %s is not supported", sz);

Yay! Can you use this pattern the other patches too?

You might want to add:

#define FSL_IMX31_SDRAM_SIZE_MAX \
   (FSL_IMX31_SDRAM0_SIZE + FSL_IMX31_SDRAM1_SIZE)

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

> +        g_free(sz);
> +        exit(EXIT_FAILURE);
>       }
>   
>       memory_region_allocate_system_memory(&s->ram, NULL, "kzm.ram",
> 

Re: [PATCH v3 18/86] arm:kzm: drop RAM size fixup

[Igor Mammedov]

On Thu, 16 Jan 2020 19:22:08 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 1/16/20 6:26 PM, Igor Mammedov wrote:
> > If the user provided too large a RAM size, the code used to
> > complain and trim it to the max size.  Now tht RAM is allocated by
> > generic code, that's no longer possible, so generate an error and
> > exit instead.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v3:
> >   * rephrase commit message in nicer way
> >     ("Chubb, Peter (Data61, Kensington NSW)" <Peter.Chubb@data61.csiro.au>)
> >   * reword error message and use size_to_str() to pretty print suggested size
> >     ("Chubb, Peter (Data61, Kensington NSW)" <Peter.Chubb@data61.csiro.au>)
> > 
> > CC: peter.chubb@nicta.com.au
> > CC: peter.maydell@linaro.org
> > CC: qemu-arm@nongnu.org
> > ---
> >   hw/arm/kzm.c | 9 +++++----
> >   1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/arm/kzm.c b/hw/arm/kzm.c
> > index 1d5ef28..94cbac1 100644
> > --- a/hw/arm/kzm.c
> > +++ b/hw/arm/kzm.c
> > @@ -25,6 +25,7 @@
> >   #include "hw/char/serial.h"
> >   #include "sysemu/qtest.h"
> >   #include "sysemu/sysemu.h"
> > +#include "qemu/cutils.h"
> >   
> >   /* Memory map for Kzm Emulation Baseboard:
> >    * 0x00000000-0x7fffffff See i.MX31 SOC for support
> > @@ -78,10 +79,10 @@ static void kzm_init(MachineState *machine)
> >   
> >       /* Check the amount of memory is compatible with the SOC */
> >       if (machine->ram_size > (FSL_IMX31_SDRAM0_SIZE + FSL_IMX31_SDRAM1_SIZE)) {
> > -        warn_report("RAM size " RAM_ADDR_FMT " above max supported, "
> > -                    "reduced to %x", machine->ram_size,
> > -                    FSL_IMX31_SDRAM0_SIZE + FSL_IMX31_SDRAM1_SIZE);
> > -        machine->ram_size = FSL_IMX31_SDRAM0_SIZE + FSL_IMX31_SDRAM1_SIZE;
> > +        char *sz = size_to_str(FSL_IMX31_SDRAM0_SIZE + FSL_IMX31_SDRAM1_SIZE);
> > +        error_report("RAM size more than %s is not supported", sz);  
> 
> Yay! Can you use this pattern the other patches too?

I plan to, as it's much neater and I can avoid adding RAM_ADDR_FMT

Would your acks still stand or should I drop your Reviewed-bys
on changed in such way patches?
 
> You might want to add:
> 
> #define FSL_IMX31_SDRAM_SIZE_MAX \
>    (FSL_IMX31_SDRAM0_SIZE + FSL_IMX31_SDRAM1_SIZE)
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> > +        g_free(sz);
> > +        exit(EXIT_FAILURE);
> >       }
> >   
> >       memory_region_allocate_system_memory(&s->ram, NULL, "kzm.ram",
> >   
> 

Re: [PATCH v3 18/86] arm:kzm: drop RAM size fixup

[Philippe Mathieu-Daudé]

On 1/17/20 10:50 AM, Igor Mammedov wrote:
> On Thu, 16 Jan 2020 19:22:08 +0100
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
>> On 1/16/20 6:26 PM, Igor Mammedov wrote:
>>> If the user provided too large a RAM size, the code used to
>>> complain and trim it to the max size.  Now tht RAM is allocated by
>>> generic code, that's no longer possible, so generate an error and
>>> exit instead.
>>>
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
>>> v3:
>>>    * rephrase commit message in nicer way
>>>      ("Chubb, Peter (Data61, Kensington NSW)" <Peter.Chubb@data61.csiro.au>)
>>>    * reword error message and use size_to_str() to pretty print suggested size
>>>      ("Chubb, Peter (Data61, Kensington NSW)" <Peter.Chubb@data61.csiro.au>)
>>>
>>> CC: peter.chubb@nicta.com.au
>>> CC: peter.maydell@linaro.org
>>> CC: qemu-arm@nongnu.org
>>> ---
>>>    hw/arm/kzm.c | 9 +++++----
>>>    1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/arm/kzm.c b/hw/arm/kzm.c
>>> index 1d5ef28..94cbac1 100644
>>> --- a/hw/arm/kzm.c
>>> +++ b/hw/arm/kzm.c
>>> @@ -25,6 +25,7 @@
>>>    #include "hw/char/serial.h"
>>>    #include "sysemu/qtest.h"
>>>    #include "sysemu/sysemu.h"
>>> +#include "qemu/cutils.h"
>>>    
>>>    /* Memory map for Kzm Emulation Baseboard:
>>>     * 0x00000000-0x7fffffff See i.MX31 SOC for support
>>> @@ -78,10 +79,10 @@ static void kzm_init(MachineState *machine)
>>>    
>>>        /* Check the amount of memory is compatible with the SOC */
>>>        if (machine->ram_size > (FSL_IMX31_SDRAM0_SIZE + FSL_IMX31_SDRAM1_SIZE)) {
>>> -        warn_report("RAM size " RAM_ADDR_FMT " above max supported, "
>>> -                    "reduced to %x", machine->ram_size,
>>> -                    FSL_IMX31_SDRAM0_SIZE + FSL_IMX31_SDRAM1_SIZE);
>>> -        machine->ram_size = FSL_IMX31_SDRAM0_SIZE + FSL_IMX31_SDRAM1_SIZE;
>>> +        char *sz = size_to_str(FSL_IMX31_SDRAM0_SIZE + FSL_IMX31_SDRAM1_SIZE);
>>> +        error_report("RAM size more than %s is not supported", sz);
>>
>> Yay! Can you use this pattern the other patches too?
> 
> I plan to, as it's much neater and I can avoid adding RAM_ADDR_FMT
> 
> Would your acks still stand or should I drop your Reviewed-bys
> on changed in such way patches?

Yes please keep my Reviewed-by tag in the other patches too.

>> You might want to add:
>>
>> #define FSL_IMX31_SDRAM_SIZE_MAX \
>>     (FSL_IMX31_SDRAM0_SIZE + FSL_IMX31_SDRAM1_SIZE)
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>
>>> +        g_free(sz);
>>> +        exit(EXIT_FAILURE);
>>>        }
>>>    
>>>        memory_region_allocate_system_memory(&s->ram, NULL, "kzm.ram",
>>>    
>>
> 

[PATCH v2 19/86] arm:kzm: use memdev for RAM

[Igor Mammedov]

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.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
CC: peter.chubb@nicta.com.au
CC: peter.maydell@linaro.org
CC: qemu-arm@nongnu.org
---
 hw/arm/kzm.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/arm/kzm.c b/hw/arm/kzm.c
index 27800c5..f63902e 100644
--- a/hw/arm/kzm.c
+++ b/hw/arm/kzm.c
@@ -51,7 +51,6 @@
 
 typedef struct IMX31KZM {
     FslIMX31State soc;
-    MemoryRegion ram;
     MemoryRegion ram_alias;
 } IMX31KZM;
 
@@ -84,10 +83,8 @@ static void kzm_init(MachineState *machine)
         exit(EXIT_FAILURE);
     }
 
-    memory_region_allocate_system_memory(&s->ram, NULL, "kzm.ram",
-                                         machine->ram_size);
     memory_region_add_subregion(get_system_memory(), FSL_IMX31_SDRAM0_ADDR,
-                                &s->ram);
+                                machine->ram);
 
     /* initialize the alias memory if any */
     for (i = 0, ram_size = machine->ram_size, alias_offset = 0;
@@ -107,7 +104,8 @@ static void kzm_init(MachineState *machine)
 
         if (size < ram[i].size) {
             memory_region_init_alias(&s->ram_alias, NULL, "ram.alias",
-                                     &s->ram, alias_offset, ram[i].size - size);
+                                     machine->ram,
+                                     alias_offset, ram[i].size - size);
             memory_region_add_subregion(get_system_memory(),
                                         ram[i].addr + size, &s->ram_alias);
         }
@@ -139,6 +137,7 @@ static void kzm_machine_init(MachineClass *mc)
     mc->desc = "ARM KZM Emulation Baseboard (ARM1136)";
     mc->init = kzm_init;
     mc->ignore_memory_transaction_failures = true;
+    mc->default_ram_id = "kzm.ram";
 }
 
 DEFINE_MACHINE("kzm", kzm_machine_init)
-- 
2.7.4

Re: [PATCH v2 19/86] arm:kzm: use memdev for RAM

[Chubb, Peter (Data61, Kensington NSW)]

>>>>> "Igor" == Igor Mammedov <imammedo@redhat.com> writes:

Igor> memory region.

Igor> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Igor> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Igor> ---
Igor> CC: peter.chubb@nicta.com.au

You can add:
Reviewed-by: Peter Chubb <peter.chubb@data61.csiro.au>

Igor> CC: peter.maydell@linaro.org
Igor> CC: qemu-arm@nongnu.org

Peter C

[PATCH v2 20/86] arm:mcimx6ul-evk: use memdev for RAM

[Igor Mammedov]

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:
  remove no longer needed MCIMX6ULEVK

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: jcd@tribudubois.net
CC: peter.maydell@linaro.org
CC: qemu-arm@nongnu.org
---
 hw/arm/mcimx6ul-evk.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/hw/arm/mcimx6ul-evk.c b/hw/arm/mcimx6ul-evk.c
index e90b393..23a71ed 100644
--- a/hw/arm/mcimx6ul-evk.c
+++ b/hw/arm/mcimx6ul-evk.c
@@ -19,15 +19,10 @@
 #include "qemu/error-report.h"
 #include "sysemu/qtest.h"
 
-typedef struct {
-    FslIMX6ULState soc;
-    MemoryRegion ram;
-} MCIMX6ULEVK;
-
 static void mcimx6ul_evk_init(MachineState *machine)
 {
     static struct arm_boot_info boot_info;
-    MCIMX6ULEVK *s = g_new0(MCIMX6ULEVK, 1);
+    FslIMX6ULState *s;
     int i;
 
     if (machine->ram_size > FSL_IMX6UL_MMDC_SIZE) {
@@ -43,15 +38,12 @@ static void mcimx6ul_evk_init(MachineState *machine)
         .nb_cpus = machine->smp.cpus,
     };
 
-    object_initialize_child(OBJECT(machine), "soc", &s->soc,  sizeof(s->soc),
-                            TYPE_FSL_IMX6UL, &error_fatal, NULL);
-
-    object_property_set_bool(OBJECT(&s->soc), true, "realized", &error_fatal);
+    s = FSL_IMX6UL(object_new(TYPE_FSL_IMX6UL));
+    object_property_add_child(OBJECT(machine), "soc", OBJECT(s), &error_fatal);
+    object_property_set_bool(OBJECT(s), true, "realized", &error_fatal);
 
-    memory_region_allocate_system_memory(&s->ram, NULL, "mcimx6ul-evk.ram",
-                                         machine->ram_size);
-    memory_region_add_subregion(get_system_memory(),
-                                FSL_IMX6UL_MMDC_ADDR, &s->ram);
+    memory_region_add_subregion(get_system_memory(), FSL_IMX6UL_MMDC_ADDR,
+                                machine->ram);
 
     for (i = 0; i < FSL_IMX6UL_NUM_USDHCS; i++) {
         BusState *bus;
@@ -61,7 +53,7 @@ static void mcimx6ul_evk_init(MachineState *machine)
 
         di = drive_get_next(IF_SD);
         blk = di ? blk_by_legacy_dinfo(di) : NULL;
-        bus = qdev_get_child_bus(DEVICE(&s->soc.usdhc[i]), "sd-bus");
+        bus = qdev_get_child_bus(DEVICE(&s->usdhc[i]), "sd-bus");
         carddev = qdev_create(bus, TYPE_SD_CARD);
         qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
         object_property_set_bool(OBJECT(carddev), true,
@@ -69,7 +61,7 @@ static void mcimx6ul_evk_init(MachineState *machine)
     }
 
     if (!qtest_enabled()) {
-        arm_load_kernel(&s->soc.cpu, machine, &boot_info);
+        arm_load_kernel(&s->cpu, machine, &boot_info);
     }
 }
 
@@ -78,5 +70,6 @@ static void mcimx6ul_evk_machine_init(MachineClass *mc)
     mc->desc = "Freescale i.MX6UL Evaluation Kit (Cortex A7)";
     mc->init = mcimx6ul_evk_init;
     mc->max_cpus = FSL_IMX6UL_NUM_CPUS;
+    mc->default_ram_id = "mcimx6ul-evk.ram";
 }
 DEFINE_MACHINE("mcimx6ul-evk", mcimx6ul_evk_machine_init)
-- 
2.7.4

[PATCH v2 21/86] arm:mcimx7d-sabre: use memdev for RAM

[Igor Mammedov]

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:
 remove no longer needed MCIMX7Sabre

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: andrew.smirnov@gmail.com
CC: peter.maydell@linaro.org
CC: qemu-arm@nongnu.org
---
 hw/arm/mcimx7d-sabre.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/hw/arm/mcimx7d-sabre.c b/hw/arm/mcimx7d-sabre.c
index 0d1f62d..de1e264 100644
--- a/hw/arm/mcimx7d-sabre.c
+++ b/hw/arm/mcimx7d-sabre.c
@@ -21,15 +21,10 @@
 #include "qemu/error-report.h"
 #include "sysemu/qtest.h"
 
-typedef struct {
-    FslIMX7State soc;
-    MemoryRegion ram;
-} MCIMX7Sabre;
-
 static void mcimx7d_sabre_init(MachineState *machine)
 {
     static struct arm_boot_info boot_info;
-    MCIMX7Sabre *s = g_new0(MCIMX7Sabre, 1);
+    FslIMX7State *s;
     int i;
 
     if (machine->ram_size > FSL_IMX7_MMDC_SIZE) {
@@ -45,15 +40,12 @@ static void mcimx7d_sabre_init(MachineState *machine)
         .nb_cpus = machine->smp.cpus,
     };
 
-    object_initialize_child(OBJECT(machine), "soc",
-                            &s->soc, sizeof(s->soc),
-                            TYPE_FSL_IMX7, &error_fatal, NULL);
-    object_property_set_bool(OBJECT(&s->soc), true, "realized", &error_fatal);
+    s = FSL_IMX7(object_new(TYPE_FSL_IMX7));
+    object_property_add_child(OBJECT(machine), "soc", OBJECT(s), &error_fatal);
+    object_property_set_bool(OBJECT(s), true, "realized", &error_fatal);
 
-    memory_region_allocate_system_memory(&s->ram, NULL, "mcimx7d-sabre.ram",
-                                         machine->ram_size);
-    memory_region_add_subregion(get_system_memory(),
-                                FSL_IMX7_MMDC_ADDR, &s->ram);
+    memory_region_add_subregion(get_system_memory(), FSL_IMX7_MMDC_ADDR,
+                                machine->ram);
 
     for (i = 0; i < FSL_IMX7_NUM_USDHCS; i++) {
         BusState *bus;
@@ -63,7 +55,7 @@ static void mcimx7d_sabre_init(MachineState *machine)
 
         di = drive_get_next(IF_SD);
         blk = di ? blk_by_legacy_dinfo(di) : NULL;
-        bus = qdev_get_child_bus(DEVICE(&s->soc.usdhc[i]), "sd-bus");
+        bus = qdev_get_child_bus(DEVICE(&s->usdhc[i]), "sd-bus");
         carddev = qdev_create(bus, TYPE_SD_CARD);
         qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
         object_property_set_bool(OBJECT(carddev), true,
@@ -71,7 +63,7 @@ static void mcimx7d_sabre_init(MachineState *machine)
     }
 
     if (!qtest_enabled()) {
-        arm_load_kernel(&s->soc.cpu[0], machine, &boot_info);
+        arm_load_kernel(&s->cpu[0], machine, &boot_info);
     }
 }
 
@@ -80,5 +72,6 @@ static void mcimx7d_sabre_machine_init(MachineClass *mc)
     mc->desc = "Freescale i.MX7 DUAL SABRE (Cortex A7)";
     mc->init = mcimx7d_sabre_init;
     mc->max_cpus = FSL_IMX7_NUM_CPUS;
+    mc->default_ram_id = "mcimx7d-sabre.ram";
 }
 DEFINE_MACHINE("mcimx7d-sabre", mcimx7d_sabre_machine_init)
-- 
2.7.4

[PATCH v2 22/86] arm:mps2-tz: use memdev for RAM

[Igor Mammedov]

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 add check for user supplied RAM size and error
 out if it mismatches board expected value.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  * fix format string causing build failure on 32-bit host
    (Philippe Mathieu-Daudé <philmd@redhat.com>)

CC: peter.maydell@linaro.org
CC: qemu-arm@nongnu.org
---
 hw/arm/mps2-tz.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c
index f8b620b..ac03142 100644
--- a/hw/arm/mps2-tz.c
+++ b/hw/arm/mps2-tz.c
@@ -79,7 +79,6 @@ typedef struct {
     MachineState parent;
 
     ARMSSE iotkit;
-    MemoryRegion psram;
     MemoryRegion ssram[3];
     MemoryRegion ssram1_m;
     MPS2SCC scc;
@@ -388,6 +387,12 @@ static void mps2tz_common_init(MachineState *machine)
         exit(1);
     }
 
+    if (machine->ram_size != mc->default_ram_size) {
+        error_report("Invalid RAM size, should be " RAM_ADDR_UFMT " Bytes",
+                     mc->default_ram_size);
+        exit(EXIT_FAILURE);
+    }
+
     sysbus_init_child_obj(OBJECT(machine), "iotkit", &mms->iotkit,
                           sizeof(mms->iotkit), mmc->armsse_type);
     iotkitdev = DEVICE(&mms->iotkit);
@@ -458,9 +463,7 @@ static void mps2tz_common_init(MachineState *machine)
      * tradeoffs. For QEMU they're all just RAM, though. We arbitrarily
      * call the 16MB our "system memory", as it's the largest lump.
      */
-    memory_region_allocate_system_memory(&mms->psram,
-                                         NULL, "mps.ram", 16 * MiB);
-    memory_region_add_subregion(system_memory, 0x80000000, &mms->psram);
+    memory_region_add_subregion(system_memory, 0x80000000, machine->ram);
 
     /* The overflow IRQs for all UARTs are ORed together.
      * Tx, Rx and "combined" IRQs are sent to the NVIC separately.
@@ -642,6 +645,7 @@ static void mps2tz_class_init(ObjectClass *oc, void *data)
 
     mc->init = mps2tz_common_init;
     iic->check = mps2_tz_idau_check;
+    mc->default_ram_id = "mps.ram";
 }
 
 static void mps2tz_an505_class_init(ObjectClass *oc, void *data)
@@ -657,6 +661,7 @@ static void mps2tz_an505_class_init(ObjectClass *oc, void *data)
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-m33");
     mmc->scc_id = 0x41045050;
     mmc->armsse_type = TYPE_IOTKIT;
+    mc->default_ram_size = 16 * MiB;
 }
 
 static void mps2tz_an521_class_init(ObjectClass *oc, void *data)
-- 
2.7.4

[PATCH v2 23/86] arm:mps2: use memdev for RAM

[Igor Mammedov]

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 add check for user supplied RAM size and error
 out if it mismatches board expected value.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  * fix format string causing build failure on 32-bit host
    (Philippe Mathieu-Daudé <philmd@redhat.com>)

CC: peter.maydell@linaro.org
CC: qemu-arm@nongnu.org
---
 hw/arm/mps2.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/arm/mps2.c b/hw/arm/mps2.c
index d002b12..5867ff0 100644
--- a/hw/arm/mps2.c
+++ b/hw/arm/mps2.c
@@ -55,7 +55,6 @@ typedef struct {
     MachineState parent;
 
     ARMv7MState armv7m;
-    MemoryRegion psram;
     MemoryRegion ssram1;
     MemoryRegion ssram1_m;
     MemoryRegion ssram23;
@@ -118,6 +117,12 @@ static void mps2_common_init(MachineState *machine)
         exit(1);
     }
 
+    if (machine->ram_size != mc->default_ram_size) {
+        error_report("Invalid RAM size, should be " RAM_ADDR_UFMT " Bytes",
+                     mc->default_ram_size);
+        exit(EXIT_FAILURE);
+    }
+
     /* The FPGA images have an odd combination of different RAMs,
      * because in hardware they are different implementations and
      * connected to different buses, giving varying performance/size
@@ -146,9 +151,7 @@ static void mps2_common_init(MachineState *machine)
      * This is of no use for QEMU so we don't implement it (as if
      * zbt_boot_ctrl is always zero).
      */
-    memory_region_allocate_system_memory(&mms->psram,
-                                         NULL, "mps.ram", 16 * MiB);
-    memory_region_add_subregion(system_memory, 0x21000000, &mms->psram);
+    memory_region_add_subregion(system_memory, 0x21000000, machine->ram);
 
     switch (mmc->fpga_type) {
     case FPGA_AN385:
@@ -338,6 +341,8 @@ static void mps2_class_init(ObjectClass *oc, void *data)
 
     mc->init = mps2_common_init;
     mc->max_cpus = 1;
+    mc->default_ram_size = 16 * MiB;
+    mc->default_ram_id = "mps.ram";
 }
 
 static void mps2_an385_class_init(ObjectClass *oc, void *data)
-- 
2.7.4

[PATCH v2 24/86] arm:musicpal: use memdev for RAM

[Igor Mammedov]

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 add check for user supplied RAM size and error
 out if it mismatches board expected value.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  * fix format string causing build failure on 32-bit host
    (Philippe Mathieu-Daudé <philmd@redhat.com>)

CC: jan.kiszka@web.de
CC: peter.maydell@linaro.org
CC: qemu-arm@nongnu.org
---
 hw/arm/musicpal.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
index f68a399..4242c4a 100644
--- a/hw/arm/musicpal.c
+++ b/hw/arm/musicpal.c
@@ -1589,16 +1589,20 @@ static void musicpal_init(MachineState *machine)
     int i;
     unsigned long flash_size;
     DriveInfo *dinfo;
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
     MemoryRegion *address_space_mem = get_system_memory();
-    MemoryRegion *ram = g_new(MemoryRegion, 1);
     MemoryRegion *sram = g_new(MemoryRegion, 1);
 
+    /* For now we use a fixed - the original - RAM size */
+    if (machine->ram_size != mc->default_ram_size) {
+        error_report("Invalid RAM size, should be " RAM_ADDR_UFMT " Bytes",
+                     mc->default_ram_size);
+        exit(EXIT_FAILURE);
+    }
+
     cpu = ARM_CPU(cpu_create(machine->cpu_type));
 
-    /* For now we use a fixed - the original - RAM size */
-    memory_region_allocate_system_memory(ram, NULL, "musicpal.ram",
-                                         MP_RAM_DEFAULT_SIZE);
-    memory_region_add_subregion(address_space_mem, 0, ram);
+    memory_region_add_subregion(address_space_mem, 0, machine->ram);
 
     memory_region_init_ram(sram, NULL, "musicpal.sram", MP_SRAM_SIZE,
                            &error_fatal);
@@ -1714,6 +1718,8 @@ static void musicpal_machine_init(MachineClass *mc)
     mc->init = musicpal_init;
     mc->ignore_memory_transaction_failures = true;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("arm926");
+    mc->default_ram_size = MP_RAM_DEFAULT_SIZE;
+    mc->default_ram_id = "musicpal.ram";
 }
 
 DEFINE_MACHINE("musicpal", musicpal_machine_init)
-- 
2.7.4

[PATCH v2 25/86] arm:nseries: use memdev for RAM

[Igor Mammedov]

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 add check for user supplied RAM size and error
 out if it mismatches board expected value.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  * fix format string causing build failure on 32-bit host
    (Philippe Mathieu-Daudé <philmd@redhat.com>)

CC: balrogg@gmail.com
CC: peter.maydell@linaro.org
CC: qemu-arm@nongnu.org
---
 hw/arm/nseries.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/hw/arm/nseries.c b/hw/arm/nseries.c
index 3fd196f..f797e69 100644
--- a/hw/arm/nseries.c
+++ b/hw/arm/nseries.c
@@ -47,7 +47,6 @@
 
 /* Nokia N8x0 support */
 struct n800_s {
-    MemoryRegion sdram;
     struct omap_mpu_state_s *mpu;
 
     struct rfbi_chip_s blizzard;
@@ -1311,13 +1310,18 @@ static void n8x0_init(MachineState *machine,
                       struct arm_boot_info *binfo, int model)
 {
     struct n800_s *s = (struct n800_s *) g_malloc0(sizeof(*s));
-    uint64_t sdram_size = binfo->ram_size;
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
 
-    memory_region_allocate_system_memory(&s->sdram, NULL, "omap2.dram",
-                                         sdram_size);
-    memory_region_add_subregion(get_system_memory(), OMAP2_Q2_BASE, &s->sdram);
+    if (machine->ram_size != mc->default_ram_size) {
+        error_report("Invalid RAM size, should be " RAM_ADDR_UFMT " Bytes",
+                     mc->default_ram_size);
+        exit(EXIT_FAILURE);
+    }
+
+    memory_region_add_subregion(get_system_memory(), OMAP2_Q2_BASE,
+                                machine->ram);
 
-    s->mpu = omap2420_mpu_init(&s->sdram, machine->cpu_type);
+    s->mpu = omap2420_mpu_init(machine->ram, machine->cpu_type);
 
     /* Setup peripherals
      *
@@ -1383,9 +1387,8 @@ static void n8x0_init(MachineState *machine,
          *
          * The code above is for loading the `zImage' file from Nokia
          * images.  */
-        load_image_targphys(option_rom[0].name,
-                            OMAP2_Q2_BASE + 0x400000,
-                            sdram_size - 0x400000);
+        load_image_targphys(option_rom[0].name, OMAP2_Q2_BASE + 0x400000,
+                            machine->ram_size - 0x400000);
 
         n800_setup_nolo_tags(nolo_tags);
         cpu_physical_memory_write(OMAP2_SRAM_BASE, nolo_tags, 0x10000);
@@ -1395,16 +1398,12 @@ static void n8x0_init(MachineState *machine,
 
 static struct arm_boot_info n800_binfo = {
     .loader_start = OMAP2_Q2_BASE,
-    /* Actually two chips of 0x4000000 bytes each */
-    .ram_size = 0x08000000,
     .board_id = 0x4f7,
     .atag_board = n800_atag_setup,
 };
 
 static struct arm_boot_info n810_binfo = {
     .loader_start = OMAP2_Q2_BASE,
-    /* Actually two chips of 0x4000000 bytes each */
-    .ram_size = 0x08000000,
     /* 0x60c and 0x6bf (WiMAX Edition) have been assigned but are not
      * used by some older versions of the bootloader and 5555 is used
      * instead (including versions that shipped with many devices).  */
@@ -1431,6 +1430,9 @@ static void n800_class_init(ObjectClass *oc, void *data)
     mc->default_boot_order = "";
     mc->ignore_memory_transaction_failures = true;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("arm1136-r2");
+    /* Actually two chips of 0x4000000 bytes each */
+    mc->default_ram_size = 0x08000000;
+    mc->default_ram_id = "omap2.dram";
 }
 
 static const TypeInfo n800_type = {
@@ -1448,6 +1450,9 @@ static void n810_class_init(ObjectClass *oc, void *data)
     mc->default_boot_order = "";
     mc->ignore_memory_transaction_failures = true;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("arm1136-r2");
+    /* Actually two chips of 0x4000000 bytes each */
+    mc->default_ram_size = 0x08000000;
+    mc->default_ram_id = "omap2.dram";
 }
 
 static const TypeInfo n810_type = {
-- 
2.7.4

[PATCH v2 26/86] arm:omap_sx1: use memdev for RAM

[Igor Mammedov]

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 add check for user supplied RAM size and error
 out if it mismatches board expected value.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  * fix format string causing build failure on 32-bit host
    (Philippe Mathieu-Daudé <philmd@redhat.com>)

CC: peter.maydell@linaro.org
CC: qemu-arm@nongnu.org
---
 hw/arm/omap_sx1.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/hw/arm/omap_sx1.c b/hw/arm/omap_sx1.c
index be24571..aa1831b 100644
--- a/hw/arm/omap_sx1.c
+++ b/hw/arm/omap_sx1.c
@@ -102,8 +102,8 @@ static struct arm_boot_info sx1_binfo = {
 static void sx1_init(MachineState *machine, const int version)
 {
     struct omap_mpu_state_s *mpu;
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
     MemoryRegion *address_space = get_system_memory();
-    MemoryRegion *dram = g_new(MemoryRegion, 1);
     MemoryRegion *flash = g_new(MemoryRegion, 1);
     MemoryRegion *cs = g_new(MemoryRegion, 4);
     static uint32_t cs0val = 0x00213090;
@@ -115,15 +115,19 @@ static void sx1_init(MachineState *machine, const int version)
     uint32_t flash_size = flash0_size;
     int be;
 
+    if (machine->ram_size != mc->default_ram_size) {
+        error_report("Invalid RAM size, should be " RAM_ADDR_UFMT " Bytes",
+                     mc->default_ram_size);
+        exit(EXIT_FAILURE);
+    }
+
     if (version == 2) {
         flash_size = flash2_size;
     }
 
-    memory_region_allocate_system_memory(dram, NULL, "omap1.dram",
-                                         sx1_binfo.ram_size);
-    memory_region_add_subregion(address_space, OMAP_EMIFF_BASE, dram);
+    memory_region_add_subregion(address_space, OMAP_EMIFF_BASE, machine->ram);
 
-    mpu = omap310_mpu_init(dram, machine->cpu_type);
+    mpu = omap310_mpu_init(machine->ram, machine->cpu_type);
 
     /* External Flash (EMIFS) */
     memory_region_init_ram(flash, NULL, "omap_sx1.flash0-0", flash_size,
@@ -223,6 +227,8 @@ static void sx1_machine_v2_class_init(ObjectClass *oc, void *data)
     mc->init = sx1_init_v2;
     mc->ignore_memory_transaction_failures = true;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("ti925t");
+    mc->default_ram_size = sdram_size;
+    mc->default_ram_id = "omap1.dram";
 }
 
 static const TypeInfo sx1_machine_v2_type = {
@@ -239,6 +245,8 @@ static void sx1_machine_v1_class_init(ObjectClass *oc, void *data)
     mc->init = sx1_init_v1;
     mc->ignore_memory_transaction_failures = true;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("ti925t");
+    mc->default_ram_size = sdram_size;
+    mc->default_ram_id = "omap1.dram";
 }
 
 static const TypeInfo sx1_machine_v1_type = {
-- 
2.7.4

[PATCH v2 27/86] arm:palm: use memdev for RAM

[Igor Mammedov]

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 add check for user supplied RAM size and error
 out if it mismatches board expected value.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  * fix format string causing build failure on 32-bit host
    (Philippe Mathieu-Daudé <philmd@redhat.com>)

CC: balrogg@gmail.com
CC: peter.maydell@linaro.org
CC: qemu-arm@nongnu.org
---
 hw/arm/palm.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/hw/arm/palm.c b/hw/arm/palm.c
index 72eca8c..336ac4b 100644
--- a/hw/arm/palm.c
+++ b/hw/arm/palm.c
@@ -181,7 +181,6 @@ static void palmte_gpio_setup(struct omap_mpu_state_s *cpu)
 
 static struct arm_boot_info palmte_binfo = {
     .loader_start = OMAP_EMIFF_BASE,
-    .ram_size = 0x02000000,
     .board_id = 0x331,
 };
 
@@ -195,15 +194,20 @@ static void palmte_init(MachineState *machine)
     static uint32_t cs2val = 0x0000e1a0;
     static uint32_t cs3val = 0xe1a0e1a0;
     int rom_size, rom_loaded = 0;
-    MemoryRegion *dram = g_new(MemoryRegion, 1);
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
     MemoryRegion *flash = g_new(MemoryRegion, 1);
     MemoryRegion *cs = g_new(MemoryRegion, 4);
 
-    memory_region_allocate_system_memory(dram, NULL, "omap1.dram",
-                                         palmte_binfo.ram_size);
-    memory_region_add_subregion(address_space_mem, OMAP_EMIFF_BASE, dram);
+    if (machine->ram_size != mc->default_ram_size) {
+        error_report("Invalid RAM size, should be " RAM_ADDR_UFMT " Bytes",
+                     mc->default_ram_size);
+        exit(EXIT_FAILURE);
+    }
+
+    memory_region_add_subregion(address_space_mem, OMAP_EMIFF_BASE,
+                                machine->ram);
 
-    mpu = omap310_mpu_init(dram, machine->cpu_type);
+    mpu = omap310_mpu_init(machine->ram, machine->cpu_type);
 
     /* External Flash (EMIFS) */
     memory_region_init_ram(flash, NULL, "palmte.flash", flash_size,
@@ -265,6 +269,8 @@ static void palmte_machine_init(MachineClass *mc)
     mc->init = palmte_init;
     mc->ignore_memory_transaction_failures = true;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("ti925t");
+    mc->default_ram_size = 0x02000000;
+    mc->default_ram_id = "omap1.dram";
 }
 
 DEFINE_MACHINE("cheetah", palmte_machine_init)
-- 
2.7.4

[PATCH v2 28/86] arm:raspi: use memdev for RAM

[Igor Mammedov]

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:
 remove no longer needed RasPiState

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: Andrew.Baumann@microsoft.com
CC: philmd@redhat.com
CC: peter.maydell@linaro.org
CC: qemu-arm@nongnu.org
---
 hw/arm/raspi.c | 34 +++++++++++++---------------------
 1 file changed, 13 insertions(+), 21 deletions(-)

diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index 6a510aa..33ace66 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -32,11 +32,6 @@
 /* Table of Linux board IDs for different Pi versions */
 static const int raspi_boardid[] = {[1] = 0xc42, [2] = 0xc43, [3] = 0xc44};
 
-typedef struct RasPiState {
-    BCM283XState soc;
-    MemoryRegion ram;
-} RasPiState;
-
 static void write_smpboot(ARMCPU *cpu, const struct arm_boot_info *info)
 {
     static const uint32_t smpboot[] = {
@@ -166,7 +161,7 @@ static void setup_boot(MachineState *machine, int version, size_t ram_size)
 
 static void raspi_init(MachineState *machine, int version)
 {
-    RasPiState *s = g_new0(RasPiState, 1);
+    Object *soc;
     uint32_t vcram_size;
     DriveInfo *di;
     BlockBackend *blk;
@@ -179,30 +174,26 @@ static void raspi_init(MachineState *machine, int version)
         exit(1);
     }
 
-    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
-                            version == 3 ? TYPE_BCM2837 : TYPE_BCM2836,
-                            &error_abort, NULL);
+    soc = object_new(version == 3 ? TYPE_BCM2837 : TYPE_BCM2836);
+    object_property_add_child(OBJECT(machine), "soc", soc, &error_fatal);
 
-    /* Allocate and map RAM */
-    memory_region_allocate_system_memory(&s->ram, OBJECT(machine), "ram",
-                                         machine->ram_size);
     /* FIXME: Remove when we have custom CPU address space support */
-    memory_region_add_subregion_overlap(get_system_memory(), 0, &s->ram, 0);
+    memory_region_add_subregion_overlap(get_system_memory(), 0,
+                                        machine->ram, 0);
 
     /* Setup the SOC */
-    object_property_add_const_link(OBJECT(&s->soc), "ram", OBJECT(&s->ram),
+    object_property_add_const_link(soc, "ram", OBJECT(machine->ram),
                                    &error_abort);
-    object_property_set_int(OBJECT(&s->soc), machine->smp.cpus, "enabled-cpus",
+    object_property_set_int(soc, machine->smp.cpus, "enabled-cpus",
                             &error_abort);
     int board_rev = version == 3 ? 0xa02082 : 0xa21041;
-    object_property_set_int(OBJECT(&s->soc), board_rev, "board-rev",
-                            &error_abort);
-    object_property_set_bool(OBJECT(&s->soc), true, "realized", &error_abort);
+    object_property_set_int(soc, board_rev, "board-rev", &error_abort);
+    object_property_set_bool(soc, true, "realized", &error_abort);
 
     /* Create and plug in the SD cards */
     di = drive_get_next(IF_SD);
     blk = di ? blk_by_legacy_dinfo(di) : NULL;
-    bus = qdev_get_child_bus(DEVICE(&s->soc), "sd-bus");
+    bus = qdev_get_child_bus(DEVICE(soc), "sd-bus");
     if (bus == NULL) {
         error_report("No SD bus found in SOC object");
         exit(1);
@@ -211,8 +202,7 @@ static void raspi_init(MachineState *machine, int version)
     qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
     object_property_set_bool(OBJECT(carddev), true, "realized", &error_fatal);
 
-    vcram_size = object_property_get_uint(OBJECT(&s->soc), "vcram-size",
-                                          &error_abort);
+    vcram_size = object_property_get_uint(soc, "vcram-size", &error_abort);
     setup_boot(machine, version, machine->ram_size - vcram_size);
 }
 
@@ -233,6 +223,7 @@ static void raspi2_machine_init(MachineClass *mc)
     mc->min_cpus = BCM283X_NCPUS;
     mc->default_cpus = BCM283X_NCPUS;
     mc->default_ram_size = 1 * GiB;
+    mc->default_ram_id = "ram";
     mc->ignore_memory_transaction_failures = true;
 };
 DEFINE_MACHINE("raspi2", raspi2_machine_init)
@@ -255,6 +246,7 @@ static void raspi3_machine_init(MachineClass *mc)
     mc->min_cpus = BCM283X_NCPUS;
     mc->default_cpus = BCM283X_NCPUS;
     mc->default_ram_size = 1 * GiB;
+    mc->default_ram_id = "ram";
 }
 DEFINE_MACHINE("raspi3", raspi3_machine_init)
 #endif
-- 
2.7.4

Re: [PATCH v2 28/86] arm:raspi: use memdev for RAM

[Philippe Mathieu-Daudé]

On 1/15/20 4:06 PM, Igor Mammedov wrote:
> 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:
>   remove no longer needed RasPiState
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> CC: Andrew.Baumann@microsoft.com
> CC: philmd@redhat.com
> CC: peter.maydell@linaro.org
> CC: qemu-arm@nongnu.org
> ---
>   hw/arm/raspi.c | 34 +++++++++++++---------------------
>   1 file changed, 13 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index 6a510aa..33ace66 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -32,11 +32,6 @@
>   /* Table of Linux board IDs for different Pi versions */
>   static const int raspi_boardid[] = {[1] = 0xc42, [2] = 0xc43, [3] = 0xc44};
>   
> -typedef struct RasPiState {
> -    BCM283XState soc;
> -    MemoryRegion ram;
> -} RasPiState;
> -
>   static void write_smpboot(ARMCPU *cpu, const struct arm_boot_info *info)
>   {
>       static const uint32_t smpboot[] = {
> @@ -166,7 +161,7 @@ static void setup_boot(MachineState *machine, int version, size_t ram_size)
>   
>   static void raspi_init(MachineState *machine, int version)
>   {
> -    RasPiState *s = g_new0(RasPiState, 1);
> +    Object *soc;
>       uint32_t vcram_size;
>       DriveInfo *di;
>       BlockBackend *blk;
> @@ -179,30 +174,26 @@ static void raspi_init(MachineState *machine, int version)
>           exit(1);
>       }
>   
> -    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> -                            version == 3 ? TYPE_BCM2837 : TYPE_BCM2836,
> -                            &error_abort, NULL);
> +    soc = object_new(version == 3 ? TYPE_BCM2837 : TYPE_BCM2836);
> +    object_property_add_child(OBJECT(machine), "soc", soc, &error_fatal);
>   
> -    /* Allocate and map RAM */
> -    memory_region_allocate_system_memory(&s->ram, OBJECT(machine), "ram",
> -                                         machine->ram_size);
>       /* FIXME: Remove when we have custom CPU address space support */
> -    memory_region_add_subregion_overlap(get_system_memory(), 0, &s->ram, 0);
> +    memory_region_add_subregion_overlap(get_system_memory(), 0,
> +                                        machine->ram, 0);
>   
>       /* Setup the SOC */
> -    object_property_add_const_link(OBJECT(&s->soc), "ram", OBJECT(&s->ram),
> +    object_property_add_const_link(soc, "ram", OBJECT(machine->ram),
>                                      &error_abort);
> -    object_property_set_int(OBJECT(&s->soc), machine->smp.cpus, "enabled-cpus",
> +    object_property_set_int(soc, machine->smp.cpus, "enabled-cpus",
>                               &error_abort);
>       int board_rev = version == 3 ? 0xa02082 : 0xa21041;
> -    object_property_set_int(OBJECT(&s->soc), board_rev, "board-rev",
> -                            &error_abort);
> -    object_property_set_bool(OBJECT(&s->soc), true, "realized", &error_abort);
> +    object_property_set_int(soc, board_rev, "board-rev", &error_abort);
> +    object_property_set_bool(soc, true, "realized", &error_abort);
>   
>       /* Create and plug in the SD cards */
>       di = drive_get_next(IF_SD);
>       blk = di ? blk_by_legacy_dinfo(di) : NULL;
> -    bus = qdev_get_child_bus(DEVICE(&s->soc), "sd-bus");
> +    bus = qdev_get_child_bus(DEVICE(soc), "sd-bus");
>       if (bus == NULL) {
>           error_report("No SD bus found in SOC object");
>           exit(1);
> @@ -211,8 +202,7 @@ static void raspi_init(MachineState *machine, int version)
>       qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
>       object_property_set_bool(OBJECT(carddev), true, "realized", &error_fatal);
>   
> -    vcram_size = object_property_get_uint(OBJECT(&s->soc), "vcram-size",
> -                                          &error_abort);
> +    vcram_size = object_property_get_uint(soc, "vcram-size", &error_abort);
>       setup_boot(machine, version, machine->ram_size - vcram_size);
>   }
>   
> @@ -233,6 +223,7 @@ static void raspi2_machine_init(MachineClass *mc)
>       mc->min_cpus = BCM283X_NCPUS;
>       mc->default_cpus = BCM283X_NCPUS;
>       mc->default_ram_size = 1 * GiB;
> +    mc->default_ram_id = "ram";
>       mc->ignore_memory_transaction_failures = true;
>   };
>   DEFINE_MACHINE("raspi2", raspi2_machine_init)
> @@ -255,6 +246,7 @@ static void raspi3_machine_init(MachineClass *mc)
>       mc->min_cpus = BCM283X_NCPUS;
>       mc->default_cpus = BCM283X_NCPUS;
>       mc->default_ram_size = 1 * GiB;
> +    mc->default_ram_id = "ram";
>   }
>   DEFINE_MACHINE("raspi3", raspi3_machine_init)
>   #endif
> 

This patch diverges a lot from my current work:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg653818.html
So I'm not very happy about it. Maybe my bad I should ping more 
aggressively my patches. I can respin mine preparing for your series on top.

Meanwhile if you are in a hurry I tested yours, so:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v2 28/86] arm:raspi: use memdev for RAM

[Igor Mammedov]

On Wed, 15 Jan 2020 20:07:34 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 1/15/20 4:06 PM, Igor Mammedov wrote:
> > 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:
> >   remove no longer needed RasPiState
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > CC: Andrew.Baumann@microsoft.com
> > CC: philmd@redhat.com
> > CC: peter.maydell@linaro.org
> > CC: qemu-arm@nongnu.org
> > ---
> >   hw/arm/raspi.c | 34 +++++++++++++---------------------
> >   1 file changed, 13 insertions(+), 21 deletions(-)
> > 
> > diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> > index 6a510aa..33ace66 100644
> > --- a/hw/arm/raspi.c
> > +++ b/hw/arm/raspi.c
> > @@ -32,11 +32,6 @@
> >   /* Table of Linux board IDs for different Pi versions */
> >   static const int raspi_boardid[] = {[1] = 0xc42, [2] = 0xc43, [3] = 0xc44};
> >   
> > -typedef struct RasPiState {
> > -    BCM283XState soc;
> > -    MemoryRegion ram;
> > -} RasPiState;
> > -
> >   static void write_smpboot(ARMCPU *cpu, const struct arm_boot_info *info)
> >   {
> >       static const uint32_t smpboot[] = {
> > @@ -166,7 +161,7 @@ static void setup_boot(MachineState *machine, int version, size_t ram_size)
> >   
> >   static void raspi_init(MachineState *machine, int version)
> >   {
> > -    RasPiState *s = g_new0(RasPiState, 1);
> > +    Object *soc;
> >       uint32_t vcram_size;
> >       DriveInfo *di;
> >       BlockBackend *blk;
> > @@ -179,30 +174,26 @@ static void raspi_init(MachineState *machine, int version)
> >           exit(1);
> >       }
> >   
> > -    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> > -                            version == 3 ? TYPE_BCM2837 : TYPE_BCM2836,
> > -                            &error_abort, NULL);
> > +    soc = object_new(version == 3 ? TYPE_BCM2837 : TYPE_BCM2836);
> > +    object_property_add_child(OBJECT(machine), "soc", soc, &error_fatal);
> >   
> > -    /* Allocate and map RAM */
> > -    memory_region_allocate_system_memory(&s->ram, OBJECT(machine), "ram",
> > -                                         machine->ram_size);
> >       /* FIXME: Remove when we have custom CPU address space support */
> > -    memory_region_add_subregion_overlap(get_system_memory(), 0, &s->ram, 0);
> > +    memory_region_add_subregion_overlap(get_system_memory(), 0,
> > +                                        machine->ram, 0);
> >   
> >       /* Setup the SOC */
> > -    object_property_add_const_link(OBJECT(&s->soc), "ram", OBJECT(&s->ram),
> > +    object_property_add_const_link(soc, "ram", OBJECT(machine->ram),
> >                                      &error_abort);
> > -    object_property_set_int(OBJECT(&s->soc), machine->smp.cpus, "enabled-cpus",
> > +    object_property_set_int(soc, machine->smp.cpus, "enabled-cpus",
> >                               &error_abort);
> >       int board_rev = version == 3 ? 0xa02082 : 0xa21041;
> > -    object_property_set_int(OBJECT(&s->soc), board_rev, "board-rev",
> > -                            &error_abort);
> > -    object_property_set_bool(OBJECT(&s->soc), true, "realized", &error_abort);
> > +    object_property_set_int(soc, board_rev, "board-rev", &error_abort);
> > +    object_property_set_bool(soc, true, "realized", &error_abort);
> >   
> >       /* Create and plug in the SD cards */
> >       di = drive_get_next(IF_SD);
> >       blk = di ? blk_by_legacy_dinfo(di) : NULL;
> > -    bus = qdev_get_child_bus(DEVICE(&s->soc), "sd-bus");
> > +    bus = qdev_get_child_bus(DEVICE(soc), "sd-bus");
> >       if (bus == NULL) {
> >           error_report("No SD bus found in SOC object");
> >           exit(1);
> > @@ -211,8 +202,7 @@ static void raspi_init(MachineState *machine, int version)
> >       qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
> >       object_property_set_bool(OBJECT(carddev), true, "realized", &error_fatal);
> >   
> > -    vcram_size = object_property_get_uint(OBJECT(&s->soc), "vcram-size",
> > -                                          &error_abort);
> > +    vcram_size = object_property_get_uint(soc, "vcram-size", &error_abort);
> >       setup_boot(machine, version, machine->ram_size - vcram_size);
> >   }
> >   
> > @@ -233,6 +223,7 @@ static void raspi2_machine_init(MachineClass *mc)
> >       mc->min_cpus = BCM283X_NCPUS;
> >       mc->default_cpus = BCM283X_NCPUS;
> >       mc->default_ram_size = 1 * GiB;
> > +    mc->default_ram_id = "ram";
> >       mc->ignore_memory_transaction_failures = true;
> >   };
> >   DEFINE_MACHINE("raspi2", raspi2_machine_init)
> > @@ -255,6 +246,7 @@ static void raspi3_machine_init(MachineClass *mc)
> >       mc->min_cpus = BCM283X_NCPUS;
> >       mc->default_cpus = BCM283X_NCPUS;
> >       mc->default_ram_size = 1 * GiB;
> > +    mc->default_ram_id = "ram";
> >   }
> >   DEFINE_MACHINE("raspi3", raspi3_machine_init)
> >   #endif
> >   
> 
> This patch diverges a lot from my current work:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg653818.html
perhaps we could generalize size checks on top of that.
there were some ideas in that direction in
  "[PATCH v2 66/86] ppc/{ppc440_bamboo,sam460x}: drop RAM size fixup"
thread


> So I'm not very happy about it. Maybe my bad I should ping more 
> aggressively my patches. I can respin mine preparing for your series on top.

this patch is trivial,
if your patches merged before this, than I'll just rebase.


> 
> Meanwhile if you are in a hurry I tested yours, so:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> 

[PATCH v2 29/86] arm:sabrelite: use memdev for RAM

[Igor Mammedov]

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:
 remove no longer needed IMX6Sabrelite

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: peter.maydell@linaro.org
CC: qemu-arm@nongnu.org
CC: jcd@tribudubois.net
---
 hw/arm/sabrelite.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/hw/arm/sabrelite.c b/hw/arm/sabrelite.c
index 96cc455..e31694b 100644
--- a/hw/arm/sabrelite.c
+++ b/hw/arm/sabrelite.c
@@ -19,11 +19,6 @@
 #include "qemu/error-report.h"
 #include "sysemu/qtest.h"
 
-typedef struct IMX6Sabrelite {
-    FslIMX6State soc;
-    MemoryRegion ram;
-} IMX6Sabrelite;
-
 static struct arm_boot_info sabrelite_binfo = {
     /* DDR memory start */
     .loader_start = FSL_IMX6_MMDC_ADDR,
@@ -45,7 +40,7 @@ static void sabrelite_reset_secondary(ARMCPU *cpu,
 
 static void sabrelite_init(MachineState *machine)
 {
-    IMX6Sabrelite *s = g_new0(IMX6Sabrelite, 1);
+    FslIMX6State *s;
     Error *err = NULL;
 
     /* Check the amount of memory is compatible with the SOC */
@@ -55,19 +50,16 @@ static void sabrelite_init(MachineState *machine)
         exit(1);
     }
 
-    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
-                            TYPE_FSL_IMX6, &error_abort, NULL);
-
-    object_property_set_bool(OBJECT(&s->soc), true, "realized", &err);
+    s = FSL_IMX6(object_new(TYPE_FSL_IMX6));
+    object_property_add_child(OBJECT(machine), "soc", OBJECT(s), &error_fatal);
+    object_property_set_bool(OBJECT(s), true, "realized", &err);
     if (err != NULL) {
         error_report("%s", error_get_pretty(err));
         exit(1);
     }
 
-    memory_region_allocate_system_memory(&s->ram, NULL, "sabrelite.ram",
-                                         machine->ram_size);
     memory_region_add_subregion(get_system_memory(), FSL_IMX6_MMDC_ADDR,
-                                &s->ram);
+                                machine->ram);
 
     {
         /*
@@ -78,7 +70,7 @@ static void sabrelite_init(MachineState *machine)
         /* Add the sst25vf016b NOR FLASH memory to first SPI */
         Object *spi_dev;
 
-        spi_dev = object_resolve_path_component(OBJECT(&s->soc), "spi1");
+        spi_dev = object_resolve_path_component(OBJECT(s), "spi1");
         if (spi_dev) {
             SSIBus *spi_bus;
 
@@ -109,7 +101,7 @@ static void sabrelite_init(MachineState *machine)
     sabrelite_binfo.secondary_cpu_reset_hook = sabrelite_reset_secondary;
 
     if (!qtest_enabled()) {
-        arm_load_kernel(&s->soc.cpu[0], machine, &sabrelite_binfo);
+        arm_load_kernel(&s->cpu[0], machine, &sabrelite_binfo);
     }
 }
 
@@ -119,6 +111,7 @@ static void sabrelite_machine_init(MachineClass *mc)
     mc->init = sabrelite_init;
     mc->max_cpus = FSL_IMX6_NUM_CPUS;
     mc->ignore_memory_transaction_failures = true;
+    mc->default_ram_id = "sabrelite.ram";
 }
 
 DEFINE_MACHINE("sabrelite", sabrelite_machine_init)
-- 
2.7.4

[PATCH v2 30/86] arm:sbsa-ref: use memdev for RAM

[Igor Mammedov]

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.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: peter.maydell@linaro.org
CC: qemu-arm@nongnu.org
CC: radoslaw.biernacki@linaro.org
CC: leif.lindholm@linaro.org
---
 hw/arm/sbsa-ref.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index 9b5bcb5..1cba9fc 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -593,7 +593,6 @@ static void sbsa_ref_init(MachineState *machine)
     MachineClass *mc = MACHINE_GET_CLASS(machine);
     MemoryRegion *sysmem = get_system_memory();
     MemoryRegion *secure_sysmem = g_new(MemoryRegion, 1);
-    MemoryRegion *ram = g_new(MemoryRegion, 1);
     bool firmware_loaded;
     const CPUArchIdList *possible_cpus;
     int n, sbsa_max_cpus;
@@ -685,9 +684,8 @@ static void sbsa_ref_init(MachineState *machine)
         object_unref(cpuobj);
     }
 
-    memory_region_allocate_system_memory(ram, NULL, "sbsa-ref.ram",
-                                         machine->ram_size);
-    memory_region_add_subregion(sysmem, sbsa_ref_memmap[SBSA_MEM].base, ram);
+    memory_region_add_subregion(sysmem, sbsa_ref_memmap[SBSA_MEM].base,
+                                machine->ram);
 
     create_fdt(sms);
 
@@ -785,6 +783,7 @@ static void sbsa_ref_class_init(ObjectClass *oc, void *data)
     mc->block_default_type = IF_IDE;
     mc->no_cdrom = 1;
     mc->default_ram_size = 1 * GiB;
+    mc->default_ram_id = "sbsa-ref.ram";
     mc->default_cpus = 4;
     mc->possible_cpu_arch_ids = sbsa_ref_possible_cpu_arch_ids;
     mc->cpu_index_to_instance_props = sbsa_ref_cpu_index_to_props;
-- 
2.7.4

Re: [PATCH v2 30/86] arm:sbsa-ref: use memdev for RAM

[Philippe Mathieu-Daudé]

On 1/15/20 4:06 PM, Igor Mammedov wrote:
> 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.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> CC: peter.maydell@linaro.org
> CC: qemu-arm@nongnu.org
> CC: radoslaw.biernacki@linaro.org
> CC: leif.lindholm@linaro.org
> ---
>   hw/arm/sbsa-ref.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index 9b5bcb5..1cba9fc 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -593,7 +593,6 @@ static void sbsa_ref_init(MachineState *machine)
>       MachineClass *mc = MACHINE_GET_CLASS(machine);
>       MemoryRegion *sysmem = get_system_memory();
>       MemoryRegion *secure_sysmem = g_new(MemoryRegion, 1);
> -    MemoryRegion *ram = g_new(MemoryRegion, 1);
>       bool firmware_loaded;
>       const CPUArchIdList *possible_cpus;
>       int n, sbsa_max_cpus;
> @@ -685,9 +684,8 @@ static void sbsa_ref_init(MachineState *machine)
>           object_unref(cpuobj);
>       }
>   
> -    memory_region_allocate_system_memory(ram, NULL, "sbsa-ref.ram",
> -                                         machine->ram_size);
> -    memory_region_add_subregion(sysmem, sbsa_ref_memmap[SBSA_MEM].base, ram);
> +    memory_region_add_subregion(sysmem, sbsa_ref_memmap[SBSA_MEM].base,
> +                                machine->ram);
>   
>       create_fdt(sms);
>   
> @@ -785,6 +783,7 @@ static void sbsa_ref_class_init(ObjectClass *oc, void *data)
>       mc->block_default_type = IF_IDE;
>       mc->no_cdrom = 1;
>       mc->default_ram_size = 1 * GiB;
> +    mc->default_ram_id = "sbsa-ref.ram";
>       mc->default_cpus = 4;
>       mc->possible_cpu_arch_ids = sbsa_ref_possible_cpu_arch_ids;
>       mc->cpu_index_to_instance_props = sbsa_ref_cpu_index_to_props;
> 

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

[PATCH v2 31/86] arm:versatilepb: use memdev for RAM

[Igor Mammedov]

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.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: peter.maydell@linaro.org
CC: qemu-arm@nongnu.org
---
 hw/arm/versatilepb.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
index e86af01..f3c4a50 100644
--- a/hw/arm/versatilepb.c
+++ b/hw/arm/versatilepb.c
@@ -184,7 +184,6 @@ static void versatile_init(MachineState *machine, int board_id)
     Object *cpuobj;
     ARMCPU *cpu;
     MemoryRegion *sysmem = get_system_memory();
-    MemoryRegion *ram = g_new(MemoryRegion, 1);
     qemu_irq pic[32];
     qemu_irq sic[32];
     DeviceState *dev, *sysctl;
@@ -220,11 +219,9 @@ static void versatile_init(MachineState *machine, int board_id)
 
     cpu = ARM_CPU(cpuobj);
 
-    memory_region_allocate_system_memory(ram, NULL, "versatile.ram",
-                                         machine->ram_size);
     /* ??? RAM should repeat to fill physical memory space.  */
     /* SDRAM at address zero.  */
-    memory_region_add_subregion(sysmem, 0, ram);
+    memory_region_add_subregion(sysmem, 0, machine->ram);
 
     sysctl = qdev_create(NULL, "realview_sysctl");
     qdev_prop_set_uint32(sysctl, "sys_id", 0x41007004);
@@ -398,6 +395,7 @@ static void versatilepb_class_init(ObjectClass *oc, void *data)
     mc->block_default_type = IF_SCSI;
     mc->ignore_memory_transaction_failures = true;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("arm926");
+    mc->default_ram_id = "versatile.ram";
 }
 
 static const TypeInfo versatilepb_type = {
@@ -415,6 +413,7 @@ static void versatileab_class_init(ObjectClass *oc, void *data)
     mc->block_default_type = IF_SCSI;
     mc->ignore_memory_transaction_failures = true;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("arm926");
+    mc->default_ram_id = "versatile.ram";
 }
 
 static const TypeInfo versatileab_type = {
-- 
2.7.4

Re: [PATCH v2 31/86] arm:versatilepb: use memdev for RAM

[Philippe Mathieu-Daudé]

On 1/15/20 4:06 PM, Igor Mammedov wrote:
> 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.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> CC: peter.maydell@linaro.org
> CC: qemu-arm@nongnu.org
> ---
>   hw/arm/versatilepb.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
> index e86af01..f3c4a50 100644
> --- a/hw/arm/versatilepb.c
> +++ b/hw/arm/versatilepb.c
> @@ -184,7 +184,6 @@ static void versatile_init(MachineState *machine, int board_id)
>       Object *cpuobj;
>       ARMCPU *cpu;
>       MemoryRegion *sysmem = get_system_memory();
> -    MemoryRegion *ram = g_new(MemoryRegion, 1);
>       qemu_irq pic[32];
>       qemu_irq sic[32];
>       DeviceState *dev, *sysctl;
> @@ -220,11 +219,9 @@ static void versatile_init(MachineState *machine, int board_id)
>   
>       cpu = ARM_CPU(cpuobj);
>   
> -    memory_region_allocate_system_memory(ram, NULL, "versatile.ram",
> -                                         machine->ram_size);
>       /* ??? RAM should repeat to fill physical memory space.  */
>       /* SDRAM at address zero.  */
> -    memory_region_add_subregion(sysmem, 0, ram);
> +    memory_region_add_subregion(sysmem, 0, machine->ram);
>   
>       sysctl = qdev_create(NULL, "realview_sysctl");
>       qdev_prop_set_uint32(sysctl, "sys_id", 0x41007004);
> @@ -398,6 +395,7 @@ static void versatilepb_class_init(ObjectClass *oc, void *data)
>       mc->block_default_type = IF_SCSI;
>       mc->ignore_memory_transaction_failures = true;
>       mc->default_cpu_type = ARM_CPU_TYPE_NAME("arm926");
> +    mc->default_ram_id = "versatile.ram";
>   }
>   
>   static const TypeInfo versatilepb_type = {
> @@ -415,6 +413,7 @@ static void versatileab_class_init(ObjectClass *oc, void *data)
>       mc->block_default_type = IF_SCSI;
>       mc->ignore_memory_transaction_failures = true;
>       mc->default_cpu_type = ARM_CPU_TYPE_NAME("arm926");
> +    mc->default_ram_id = "versatile.ram";
>   }
>   
>   static const TypeInfo versatileab_type = {
> 

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

[PATCH v2 32/86] arm:vexpress: use memdev for RAM

[Igor Mammedov]

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.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: peter.maydell@linaro.org
CC: qemu-arm@nongnu.org
---
 hw/arm/vexpress.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index 4673a88..ed683ee 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -273,7 +273,6 @@ static void a9_daughterboard_init(const VexpressMachineState *vms,
 {
     MachineState *machine = MACHINE(vms);
     MemoryRegion *sysmem = get_system_memory();
-    MemoryRegion *ram = g_new(MemoryRegion, 1);
     MemoryRegion *lowram = g_new(MemoryRegion, 1);
     ram_addr_t low_ram_size;
 
@@ -283,8 +282,6 @@ static void a9_daughterboard_init(const VexpressMachineState *vms,
         exit(1);
     }
 
-    memory_region_allocate_system_memory(ram, NULL, "vexpress.highmem",
-                                         ram_size);
     low_ram_size = ram_size;
     if (low_ram_size > 0x4000000) {
         low_ram_size = 0x4000000;
@@ -293,9 +290,10 @@ static void a9_daughterboard_init(const VexpressMachineState *vms,
      * address space should in theory be remappable to various
      * things including ROM or RAM; we always map the RAM there.
      */
-    memory_region_init_alias(lowram, NULL, "vexpress.lowmem", ram, 0, low_ram_size);
+    memory_region_init_alias(lowram, NULL, "vexpress.lowmem", machine->ram,
+                             0, low_ram_size);
     memory_region_add_subregion(sysmem, 0x0, lowram);
-    memory_region_add_subregion(sysmem, 0x60000000, ram);
+    memory_region_add_subregion(sysmem, 0x60000000, machine->ram);
 
     /* 0x1e000000 A9MPCore (SCU) private memory region */
     init_cpus(machine, cpu_type, TYPE_A9MPCORE_PRIV, 0x1e000000, pic,
@@ -360,7 +358,6 @@ static void a15_daughterboard_init(const VexpressMachineState *vms,
 {
     MachineState *machine = MACHINE(vms);
     MemoryRegion *sysmem = get_system_memory();
-    MemoryRegion *ram = g_new(MemoryRegion, 1);
     MemoryRegion *sram = g_new(MemoryRegion, 1);
 
     {
@@ -375,10 +372,8 @@ static void a15_daughterboard_init(const VexpressMachineState *vms,
         }
     }
 
-    memory_region_allocate_system_memory(ram, NULL, "vexpress.highmem",
-                                         ram_size);
     /* RAM is from 0x80000000 upwards; there is no low-memory alias for it. */
-    memory_region_add_subregion(sysmem, 0x80000000, ram);
+    memory_region_add_subregion(sysmem, 0x80000000, machine->ram);
 
     /* 0x2c000000 A15MPCore private memory region (GIC) */
     init_cpus(machine, cpu_type, TYPE_A15MPCORE_PRIV,
@@ -795,6 +790,7 @@ static void vexpress_class_init(ObjectClass *oc, void *data)
     mc->init = vexpress_common_init;
     mc->max_cpus = 4;
     mc->ignore_memory_transaction_failures = true;
+    mc->default_ram_id = "vexpress.highmem";
 }
 
 static void vexpress_a9_class_init(ObjectClass *oc, void *data)
-- 
2.7.4

Re: [PATCH v2 32/86] arm:vexpress: use memdev for RAM

[Philippe Mathieu-Daudé]

On 1/15/20 4:06 PM, Igor Mammedov wrote:
> 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.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> CC: peter.maydell@linaro.org
> CC: qemu-arm@nongnu.org
> ---
>   hw/arm/vexpress.c | 14 +++++---------
>   1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
> index 4673a88..ed683ee 100644
> --- a/hw/arm/vexpress.c
> +++ b/hw/arm/vexpress.c
> @@ -273,7 +273,6 @@ static void a9_daughterboard_init(const VexpressMachineState *vms,
>   {
>       MachineState *machine = MACHINE(vms);
>       MemoryRegion *sysmem = get_system_memory();
> -    MemoryRegion *ram = g_new(MemoryRegion, 1);
>       MemoryRegion *lowram = g_new(MemoryRegion, 1);
>       ram_addr_t low_ram_size;
>   
> @@ -283,8 +282,6 @@ static void a9_daughterboard_init(const VexpressMachineState *vms,
>           exit(1);
>       }
>   
> -    memory_region_allocate_system_memory(ram, NULL, "vexpress.highmem",
> -                                         ram_size);
>       low_ram_size = ram_size;
>       if (low_ram_size > 0x4000000) {
>           low_ram_size = 0x4000000;
> @@ -293,9 +290,10 @@ static void a9_daughterboard_init(const VexpressMachineState *vms,
>        * address space should in theory be remappable to various
>        * things including ROM or RAM; we always map the RAM there.
>        */
> -    memory_region_init_alias(lowram, NULL, "vexpress.lowmem", ram, 0, low_ram_size);
> +    memory_region_init_alias(lowram, NULL, "vexpress.lowmem", machine->ram,
> +                             0, low_ram_size);
>       memory_region_add_subregion(sysmem, 0x0, lowram);
> -    memory_region_add_subregion(sysmem, 0x60000000, ram);
> +    memory_region_add_subregion(sysmem, 0x60000000, machine->ram);
>   
>       /* 0x1e000000 A9MPCore (SCU) private memory region */
>       init_cpus(machine, cpu_type, TYPE_A9MPCORE_PRIV, 0x1e000000, pic,
> @@ -360,7 +358,6 @@ static void a15_daughterboard_init(const VexpressMachineState *vms,
>   {
>       MachineState *machine = MACHINE(vms);
>       MemoryRegion *sysmem = get_system_memory();
> -    MemoryRegion *ram = g_new(MemoryRegion, 1);
>       MemoryRegion *sram = g_new(MemoryRegion, 1);
>   
>       {
> @@ -375,10 +372,8 @@ static void a15_daughterboard_init(const VexpressMachineState *vms,
>           }
>       }
>   
> -    memory_region_allocate_system_memory(ram, NULL, "vexpress.highmem",
> -                                         ram_size);
>       /* RAM is from 0x80000000 upwards; there is no low-memory alias for it. */
> -    memory_region_add_subregion(sysmem, 0x80000000, ram);
> +    memory_region_add_subregion(sysmem, 0x80000000, machine->ram);
>   
>       /* 0x2c000000 A15MPCore private memory region (GIC) */
>       init_cpus(machine, cpu_type, TYPE_A15MPCORE_PRIV,
> @@ -795,6 +790,7 @@ static void vexpress_class_init(ObjectClass *oc, void *data)
>       mc->init = vexpress_common_init;
>       mc->max_cpus = 4;
>       mc->ignore_memory_transaction_failures = true;
> +    mc->default_ram_id = "vexpress.highmem";
>   }
>   
>   static void vexpress_a9_class_init(ObjectClass *oc, void *data)
> 

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

[PATCH v2 33/86] arm:virt: use memdev for RAM

[Igor Mammedov]

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 then map memory region provided by
  MachineState::ram_memdev

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: peter.maydell@linaro.org
CC: qemu-arm@nongnu.org
CC: drjones@redhat.com
---
 hw/arm/virt.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 39ab5f4..e2fbca3 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1512,7 +1512,6 @@ static void machvirt_init(MachineState *machine)
     MemoryRegion *sysmem = get_system_memory();
     MemoryRegion *secure_sysmem = NULL;
     int n, virt_max_cpus;
-    MemoryRegion *ram = g_new(MemoryRegion, 1);
     bool firmware_loaded;
     bool aarch64 = true;
     bool has_ged = !vmc->no_ged;
@@ -1701,9 +1700,8 @@ static void machvirt_init(MachineState *machine)
         }
     }
 
-    memory_region_allocate_system_memory(ram, NULL, "mach-virt.ram",
-                                         machine->ram_size);
-    memory_region_add_subregion(sysmem, vms->memmap[VIRT_MEM].base, ram);
+    memory_region_add_subregion(sysmem, vms->memmap[VIRT_MEM].base,
+                                machine->ram);
     if (machine->device_memory) {
         memory_region_add_subregion(sysmem, machine->device_memory->base,
                                     &machine->device_memory->mr);
@@ -2053,6 +2051,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
     hc->unplug_request = virt_machine_device_unplug_request_cb;
     mc->numa_mem_supported = true;
     mc->auto_enable_numa_with_memhp = true;
+    mc->default_ram_id = "mach-virt.ram";
 }
 
 static void virt_instance_init(Object *obj)
-- 
2.7.4

Re: [PATCH v2 33/86] arm:virt: use memdev for RAM

[Philippe Mathieu-Daudé]

On 1/15/20 4:06 PM, Igor Mammedov wrote:
> 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 then map memory region provided by
>    MachineState::ram_memdev
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> CC: peter.maydell@linaro.org
> CC: qemu-arm@nongnu.org
> CC: drjones@redhat.com
> ---
>   hw/arm/virt.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 39ab5f4..e2fbca3 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1512,7 +1512,6 @@ static void machvirt_init(MachineState *machine)
>       MemoryRegion *sysmem = get_system_memory();
>       MemoryRegion *secure_sysmem = NULL;
>       int n, virt_max_cpus;
> -    MemoryRegion *ram = g_new(MemoryRegion, 1);
>       bool firmware_loaded;
>       bool aarch64 = true;
>       bool has_ged = !vmc->no_ged;
> @@ -1701,9 +1700,8 @@ static void machvirt_init(MachineState *machine)
>           }
>       }
>   
> -    memory_region_allocate_system_memory(ram, NULL, "mach-virt.ram",
> -                                         machine->ram_size);
> -    memory_region_add_subregion(sysmem, vms->memmap[VIRT_MEM].base, ram);
> +    memory_region_add_subregion(sysmem, vms->memmap[VIRT_MEM].base,
> +                                machine->ram);
>       if (machine->device_memory) {
>           memory_region_add_subregion(sysmem, machine->device_memory->base,
>                                       &machine->device_memory->mr);
> @@ -2053,6 +2051,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>       hc->unplug_request = virt_machine_device_unplug_request_cb;
>       mc->numa_mem_supported = true;
>       mc->auto_enable_numa_with_memhp = true;
> +    mc->default_ram_id = "mach-virt.ram";
>   }
>   
>   static void virt_instance_init(Object *obj)
> 

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

[PATCH v2 34/86] arm:xilinx_zynq: drop RAM size fixup

[Igor Mammedov]

If user provided non-sense RAM size, board will complain and
continue running with max RAM size supported.
Also RAM is going to be allocated by generic code, so it won't be
possible for board to fix things up for user.

Make it error message and exit to force user fix CLI,
instead of accepting non-sense CLI values.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: peter.maydell@linaro.org
CC: qemu-arm@nongnu.org
CC: edgar.iglesias@gmail.com
CC: alistair@alistair23.me
---
 hw/arm/xilinx_zynq.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 3a0fa5b..df950fc 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -158,7 +158,6 @@ static inline void zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
 
 static void zynq_init(MachineState *machine)
 {
-    ram_addr_t ram_size = machine->ram_size;
     ARMCPU *cpu;
     MemoryRegion *address_space_mem = get_system_memory();
     MemoryRegion *ext_ram = g_new(MemoryRegion, 1);
@@ -168,6 +167,12 @@ static void zynq_init(MachineState *machine)
     qemu_irq pic[64];
     int n;
 
+    /* max 2GB ram */
+    if (machine->ram_size > 0x80000000) {
+        error_report("RAM size more than %d is not supported", 0x80000000);
+        exit(EXIT_FAILURE);
+    }
+
     cpu = ARM_CPU(object_new(machine->cpu_type));
 
     /* By default A9 CPUs have EL3 enabled.  This board does not
@@ -184,14 +189,9 @@ static void zynq_init(MachineState *machine)
                             &error_fatal);
     object_property_set_bool(OBJECT(cpu), true, "realized", &error_fatal);
 
-    /* max 2GB ram */
-    if (ram_size > 0x80000000) {
-        ram_size = 0x80000000;
-    }
-
     /* DDR remapped to address zero.  */
     memory_region_allocate_system_memory(ext_ram, NULL, "zynq.ext_ram",
-                                         ram_size);
+                                         machine->ram_size);
     memory_region_add_subregion(address_space_mem, 0, ext_ram);
 
     /* 256K of on-chip memory */
@@ -300,7 +300,7 @@ static void zynq_init(MachineState *machine)
     sysbus_connect_irq(busdev, 0, pic[40 - IRQ_OFFSET]);
     sysbus_mmio_map(busdev, 0, 0xF8007000);
 
-    zynq_binfo.ram_size = ram_size;
+    zynq_binfo.ram_size = machine->ram_size;
     zynq_binfo.nb_cpus = 1;
     zynq_binfo.board_id = 0xd32;
     zynq_binfo.loader_start = 0;
-- 
2.7.4

Re: [PATCH v2 34/86] arm:xilinx_zynq: drop RAM size fixup

[Alistair Francis]

On Thu, Jan 16, 2020 at 1:25 AM Igor Mammedov <imammedo@redhat.com> wrote:
>
> If user provided non-sense RAM size, board will complain and
> continue running with max RAM size supported.
> Also RAM is going to be allocated by generic code, so it won't be
> possible for board to fix things up for user.
>
> Make it error message and exit to force user fix CLI,
> instead of accepting non-sense CLI values.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> CC: peter.maydell@linaro.org
> CC: qemu-arm@nongnu.org
> CC: edgar.iglesias@gmail.com
> CC: alistair@alistair23.me
> ---
>  hw/arm/xilinx_zynq.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index 3a0fa5b..df950fc 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -158,7 +158,6 @@ static inline void zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
>
>  static void zynq_init(MachineState *machine)
>  {
> -    ram_addr_t ram_size = machine->ram_size;
>      ARMCPU *cpu;
>      MemoryRegion *address_space_mem = get_system_memory();
>      MemoryRegion *ext_ram = g_new(MemoryRegion, 1);
> @@ -168,6 +167,12 @@ static void zynq_init(MachineState *machine)
>      qemu_irq pic[64];
>      int n;
>
> +    /* max 2GB ram */
> +    if (machine->ram_size > 0x80000000) {
> +        error_report("RAM size more than %d is not supported", 0x80000000);
> +        exit(EXIT_FAILURE);
> +    }
> +
>      cpu = ARM_CPU(object_new(machine->cpu_type));
>
>      /* By default A9 CPUs have EL3 enabled.  This board does not
> @@ -184,14 +189,9 @@ static void zynq_init(MachineState *machine)
>                              &error_fatal);
>      object_property_set_bool(OBJECT(cpu), true, "realized", &error_fatal);
>
> -    /* max 2GB ram */
> -    if (ram_size > 0x80000000) {
> -        ram_size = 0x80000000;
> -    }
> -
>      /* DDR remapped to address zero.  */
>      memory_region_allocate_system_memory(ext_ram, NULL, "zynq.ext_ram",
> -                                         ram_size);
> +                                         machine->ram_size);
>      memory_region_add_subregion(address_space_mem, 0, ext_ram);
>
>      /* 256K of on-chip memory */
> @@ -300,7 +300,7 @@ static void zynq_init(MachineState *machine)
>      sysbus_connect_irq(busdev, 0, pic[40 - IRQ_OFFSET]);
>      sysbus_mmio_map(busdev, 0, 0xF8007000);
>
> -    zynq_binfo.ram_size = ram_size;
> +    zynq_binfo.ram_size = machine->ram_size;
>      zynq_binfo.nb_cpus = 1;
>      zynq_binfo.board_id = 0xd32;
>      zynq_binfo.loader_start = 0;
> --
> 2.7.4
>
>

[PATCH v2 35/86] arm:xilinx_zynq: use memdev for RAM

[Igor Mammedov]

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.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: peter.maydell@linaro.org
CC: qemu-arm@nongnu.org
CC: edgar.iglesias@gmail.com
CC: alistair@alistair23.me
---
 hw/arm/xilinx_zynq.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index df950fc..0ef9688 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -160,7 +160,6 @@ static void zynq_init(MachineState *machine)
 {
     ARMCPU *cpu;
     MemoryRegion *address_space_mem = get_system_memory();
-    MemoryRegion *ext_ram = g_new(MemoryRegion, 1);
     MemoryRegion *ocm_ram = g_new(MemoryRegion, 1);
     DeviceState *dev;
     SysBusDevice *busdev;
@@ -190,9 +189,7 @@ static void zynq_init(MachineState *machine)
     object_property_set_bool(OBJECT(cpu), true, "realized", &error_fatal);
 
     /* DDR remapped to address zero.  */
-    memory_region_allocate_system_memory(ext_ram, NULL, "zynq.ext_ram",
-                                         machine->ram_size);
-    memory_region_add_subregion(address_space_mem, 0, ext_ram);
+    memory_region_add_subregion(address_space_mem, 0, machine->ram);
 
     /* 256K of on-chip memory */
     memory_region_init_ram(ocm_ram, NULL, "zynq.ocm_ram", 256 * KiB,
@@ -318,6 +315,7 @@ static void zynq_machine_init(MachineClass *mc)
     mc->no_sdcard = 1;
     mc->ignore_memory_transaction_failures = true;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a9");
+    mc->default_ram_id = "zynq.ext_ram";
 }
 
 DEFINE_MACHINE("xilinx-zynq-a9", zynq_machine_init)
-- 
2.7.4

Re: [PATCH v2 35/86] arm:xilinx_zynq: use memdev for RAM

[Philippe Mathieu-Daudé]

On 1/15/20 4:06 PM, Igor Mammedov wrote:
> 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.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> CC: peter.maydell@linaro.org
> CC: qemu-arm@nongnu.org
> CC: edgar.iglesias@gmail.com
> CC: alistair@alistair23.me
> ---
>   hw/arm/xilinx_zynq.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index df950fc..0ef9688 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -160,7 +160,6 @@ static void zynq_init(MachineState *machine)
>   {
>       ARMCPU *cpu;
>       MemoryRegion *address_space_mem = get_system_memory();
> -    MemoryRegion *ext_ram = g_new(MemoryRegion, 1);
>       MemoryRegion *ocm_ram = g_new(MemoryRegion, 1);
>       DeviceState *dev;
>       SysBusDevice *busdev;
> @@ -190,9 +189,7 @@ static void zynq_init(MachineState *machine)
>       object_property_set_bool(OBJECT(cpu), true, "realized", &error_fatal);
>   
>       /* DDR remapped to address zero.  */
> -    memory_region_allocate_system_memory(ext_ram, NULL, "zynq.ext_ram",
> -                                         machine->ram_size);
> -    memory_region_add_subregion(address_space_mem, 0, ext_ram);
> +    memory_region_add_subregion(address_space_mem, 0, machine->ram);
>   
>       /* 256K of on-chip memory */
>       memory_region_init_ram(ocm_ram, NULL, "zynq.ocm_ram", 256 * KiB,
> @@ -318,6 +315,7 @@ static void zynq_machine_init(MachineClass *mc)
>       mc->no_sdcard = 1;
>       mc->ignore_memory_transaction_failures = true;
>       mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a9");
> +    mc->default_ram_id = "zynq.ext_ram";
>   }
>   
>   DEFINE_MACHINE("xilinx-zynq-a9", zynq_machine_init)
> 

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

Re: [PATCH v2 35/86] arm:xilinx_zynq: use memdev for RAM

[Alistair Francis]

On Thu, Jan 16, 2020 at 1:29 AM Igor Mammedov <imammedo@redhat.com> wrote:
>
> 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.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> CC: peter.maydell@linaro.org
> CC: qemu-arm@nongnu.org
> CC: edgar.iglesias@gmail.com
> CC: alistair@alistair23.me
> ---
>  hw/arm/xilinx_zynq.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index df950fc..0ef9688 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -160,7 +160,6 @@ static void zynq_init(MachineState *machine)
>  {
>      ARMCPU *cpu;
>      MemoryRegion *address_space_mem = get_system_memory();
> -    MemoryRegion *ext_ram = g_new(MemoryRegion, 1);
>      MemoryRegion *ocm_ram = g_new(MemoryRegion, 1);
>      DeviceState *dev;
>      SysBusDevice *busdev;
> @@ -190,9 +189,7 @@ static void zynq_init(MachineState *machine)
>      object_property_set_bool(OBJECT(cpu), true, "realized", &error_fatal);
>
>      /* DDR remapped to address zero.  */
> -    memory_region_allocate_system_memory(ext_ram, NULL, "zynq.ext_ram",
> -                                         machine->ram_size);
> -    memory_region_add_subregion(address_space_mem, 0, ext_ram);
> +    memory_region_add_subregion(address_space_mem, 0, machine->ram);
>
>      /* 256K of on-chip memory */
>      memory_region_init_ram(ocm_ram, NULL, "zynq.ocm_ram", 256 * KiB,
> @@ -318,6 +315,7 @@ static void zynq_machine_init(MachineClass *mc)
>      mc->no_sdcard = 1;
>      mc->ignore_memory_transaction_failures = true;
>      mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a9");
> +    mc->default_ram_id = "zynq.ext_ram";
>  }
>
>  DEFINE_MACHINE("xilinx-zynq-a9", zynq_machine_init)
> --
> 2.7.4
>
>

[PATCH v2 37/86] arm:xlnx-zcu102: use memdev for RAM

[Igor Mammedov]

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.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: alistair@alistair23.me
CC: edgar.iglesias@gmail.com
CC: peter.maydell@linaro.org
CC: qemu-arm@nongnu.org
---
 hw/arm/xlnx-zcu102.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
index 53cfe7c..bd645ad 100644
--- a/hw/arm/xlnx-zcu102.c
+++ b/hw/arm/xlnx-zcu102.c
@@ -28,7 +28,6 @@ typedef struct XlnxZCU102 {
     MachineState parent_obj;
 
     XlnxZynqMPState soc;
-    MemoryRegion ddr_ram;
 
     bool secure;
     bool virt;
@@ -87,13 +86,10 @@ static void xlnx_zcu102_init(MachineState *machine)
                  ram_size);
     }
 
-    memory_region_allocate_system_memory(&s->ddr_ram, NULL, "ddr-ram",
-                                         ram_size);
-
     object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
                             TYPE_XLNX_ZYNQMP, &error_abort, NULL);
 
-    object_property_set_link(OBJECT(&s->soc), OBJECT(&s->ddr_ram),
+    object_property_set_link(OBJECT(&s->soc), OBJECT(machine->ram),
                          "ddr-ram", &error_abort);
     object_property_set_bool(OBJECT(&s->soc), s->secure, "secure",
                              &error_fatal);
@@ -211,6 +207,7 @@ static void xlnx_zcu102_machine_class_init(ObjectClass *oc, void *data)
     mc->ignore_memory_transaction_failures = true;
     mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS + XLNX_ZYNQMP_NUM_RPU_CPUS;
     mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS;
+    mc->default_ram_id = "ddr-ram";
 }
 
 static const TypeInfo xlnx_zcu102_machine_init_typeinfo = {
-- 
2.7.4

Re: [PATCH v2 37/86] arm:xlnx-zcu102: use memdev for RAM

[Philippe Mathieu-Daudé]

On 1/15/20 4:06 PM, Igor Mammedov wrote:
> 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.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> CC: alistair@alistair23.me
> CC: edgar.iglesias@gmail.com
> CC: peter.maydell@linaro.org
> CC: qemu-arm@nongnu.org
> ---
>   hw/arm/xlnx-zcu102.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
> index 53cfe7c..bd645ad 100644
> --- a/hw/arm/xlnx-zcu102.c
> +++ b/hw/arm/xlnx-zcu102.c
> @@ -28,7 +28,6 @@ typedef struct XlnxZCU102 {
>       MachineState parent_obj;
>   
>       XlnxZynqMPState soc;
> -    MemoryRegion ddr_ram;
>   
>       bool secure;
>       bool virt;
> @@ -87,13 +86,10 @@ static void xlnx_zcu102_init(MachineState *machine)
>                    ram_size);
>       }
>   
> -    memory_region_allocate_system_memory(&s->ddr_ram, NULL, "ddr-ram",
> -                                         ram_size);
> -
>       object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
>                               TYPE_XLNX_ZYNQMP, &error_abort, NULL);
>   
> -    object_property_set_link(OBJECT(&s->soc), OBJECT(&s->ddr_ram),
> +    object_property_set_link(OBJECT(&s->soc), OBJECT(machine->ram),
>                            "ddr-ram", &error_abort);
>       object_property_set_bool(OBJECT(&s->soc), s->secure, "secure",
>                                &error_fatal);
> @@ -211,6 +207,7 @@ static void xlnx_zcu102_machine_class_init(ObjectClass *oc, void *data)
>       mc->ignore_memory_transaction_failures = true;
>       mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS + XLNX_ZYNQMP_NUM_RPU_CPUS;
>       mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS;
> +    mc->default_ram_id = "ddr-ram";
>   }
>   
>   static const TypeInfo xlnx_zcu102_machine_init_typeinfo = {
> 

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

Re: [PATCH v2 37/86] arm:xlnx-zcu102: use memdev for RAM

[Alistair Francis]

On Thu, Jan 16, 2020 at 1:39 AM Igor Mammedov <imammedo@redhat.com> wrote:
>
> 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.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> CC: alistair@alistair23.me
> CC: edgar.iglesias@gmail.com
> CC: peter.maydell@linaro.org
> CC: qemu-arm@nongnu.org
> ---
>  hw/arm/xlnx-zcu102.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
> index 53cfe7c..bd645ad 100644
> --- a/hw/arm/xlnx-zcu102.c
> +++ b/hw/arm/xlnx-zcu102.c
> @@ -28,7 +28,6 @@ typedef struct XlnxZCU102 {
>      MachineState parent_obj;
>
>      XlnxZynqMPState soc;
> -    MemoryRegion ddr_ram;
>
>      bool secure;
>      bool virt;
> @@ -87,13 +86,10 @@ static void xlnx_zcu102_init(MachineState *machine)
>                   ram_size);
>      }
>
> -    memory_region_allocate_system_memory(&s->ddr_ram, NULL, "ddr-ram",
> -                                         ram_size);
> -
>      object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
>                              TYPE_XLNX_ZYNQMP, &error_abort, NULL);
>
> -    object_property_set_link(OBJECT(&s->soc), OBJECT(&s->ddr_ram),
> +    object_property_set_link(OBJECT(&s->soc), OBJECT(machine->ram),
>                           "ddr-ram", &error_abort);
>      object_property_set_bool(OBJECT(&s->soc), s->secure, "secure",
>                               &error_fatal);
> @@ -211,6 +207,7 @@ static void xlnx_zcu102_machine_class_init(ObjectClass *oc, void *data)
>      mc->ignore_memory_transaction_failures = true;
>      mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS + XLNX_ZYNQMP_NUM_RPU_CPUS;
>      mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS;
> +    mc->default_ram_id = "ddr-ram";
>  }
>
>  static const TypeInfo xlnx_zcu102_machine_init_typeinfo = {
> --
> 2.7.4
>
>

[PATCH v2 82/86] numa: forbid '-numa node, mem' for 5.0 and newer machine types

[Igor Mammedov]

Deprecation period is ran out and it's a time to flip the switch
introduced by cd5ff8333a.
Disable legacy option for new machine types and amend documentation.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: peter.maydell@linaro.org
CC: ehabkost@redhat.com
CC: marcel.apfelbaum@gmail.com
CC: mst@redhat.com
CC: pbonzini@redhat.com
CC: rth@twiddle.net
CC: david@gibson.dropbear.id.au
CC: libvir-list@redhat.com
CC: qemu-arm@nongnu.org
CC: qemu-ppc@nongnu.org
---
 hw/arm/virt.c        |  2 +-
 hw/core/numa.c       |  6 ++++++
 hw/i386/pc.c         |  1 -
 hw/i386/pc_piix.c    |  1 +
 hw/i386/pc_q35.c     |  1 +
 hw/ppc/spapr.c       |  2 +-
 qemu-deprecated.texi | 16 ----------------
 qemu-options.hx      |  8 ++++----
 8 files changed, 14 insertions(+), 23 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index e2fbca3..49de0d8 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2049,7 +2049,6 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
     hc->pre_plug = virt_machine_device_pre_plug_cb;
     hc->plug = virt_machine_device_plug_cb;
     hc->unplug_request = virt_machine_device_unplug_request_cb;
-    mc->numa_mem_supported = true;
     mc->auto_enable_numa_with_memhp = true;
     mc->default_ram_id = "mach-virt.ram";
 }
@@ -2153,6 +2152,7 @@ DEFINE_VIRT_MACHINE_AS_LATEST(5, 0)
 static void virt_machine_4_2_options(MachineClass *mc)
 {
     compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
+    mc->numa_mem_supported = true;
 }
 DEFINE_VIRT_MACHINE(4, 2)
 
diff --git a/hw/core/numa.c b/hw/core/numa.c
index 0970a30..3177066 100644
--- a/hw/core/numa.c
+++ b/hw/core/numa.c
@@ -117,6 +117,12 @@ static void parse_numa_node(MachineState *ms, NumaNodeOptions *node,
     }
 
     if (node->has_mem) {
+        if (!mc->numa_mem_supported) {
+            error_setg(errp, "Parameter -numa node,mem is not supported by this"
+                      " machine type. Use -numa node,memdev instead");
+            return;
+        }
+
         numa_info[nodenr].node_mem = node->mem;
         if (!qtest_enabled()) {
             warn_report("Parameter -numa node,mem is deprecated,"
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 21b8290..fa8d024 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1947,7 +1947,6 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     hc->unplug = pc_machine_device_unplug_cb;
     mc->default_cpu_type = TARGET_DEFAULT_CPU_TYPE;
     mc->nvdimm_supported = true;
-    mc->numa_mem_supported = true;
     mc->default_ram_id = "pc.ram";
 
     object_class_property_add(oc, PC_MACHINE_DEVMEM_REGION_SIZE, "int",
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index fa12203..0a9b9e0 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -435,6 +435,7 @@ static void pc_i440fx_4_2_machine_options(MachineClass *m)
     pc_i440fx_5_0_machine_options(m);
     m->alias = NULL;
     m->is_default = 0;
+    m->numa_mem_supported = true;
     compat_props_add(m->compat_props, hw_compat_4_2, hw_compat_4_2_len);
     compat_props_add(m->compat_props, pc_compat_4_2, pc_compat_4_2_len);
 }
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 84cf925..4d6e2be 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -363,6 +363,7 @@ static void pc_q35_4_2_machine_options(MachineClass *m)
 {
     pc_q35_5_0_machine_options(m);
     m->alias = NULL;
+    m->numa_mem_supported = true;
     compat_props_add(m->compat_props, hw_compat_4_2, hw_compat_4_2_len);
     compat_props_add(m->compat_props, pc_compat_4_2, pc_compat_4_2_len);
 }
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index bcbe1f1..2686b73 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4383,7 +4383,6 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
      * in which LMBs are represented and hot-added
      */
     mc->numa_mem_align_shift = 28;
-    mc->numa_mem_supported = true;
     mc->auto_enable_numa = true;
 
     smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
@@ -4465,6 +4464,7 @@ static void spapr_machine_4_2_class_options(MachineClass *mc)
 {
     spapr_machine_5_0_class_options(mc);
     compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
+    mc->numa_mem_supported = true;
 }
 
 DEFINE_SPAPR_MACHINE(4_2, "4.2", false);
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 982af95..17a0e1d 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -89,22 +89,6 @@ error in the future.
 The @code{-realtime mlock=on|off} argument has been replaced by the
 @code{-overcommit mem-lock=on|off} argument.
 
-@subsection -numa node,mem=@var{size} (since 4.1)
-
-The parameter @option{mem} of @option{-numa node} is used to assign a part of
-guest RAM to a NUMA node. But when using it, it's impossible to manage specified
-RAM chunk on the host side (like bind it to a host node, setting bind policy, ...),
-so guest end-ups with the fake NUMA configuration with suboptiomal performance.
-However since 2014 there is an alternative way to assign RAM to a NUMA node
-using parameter @option{memdev}, which does the same as @option{mem} and adds
-means to actualy manage node RAM on the host side. Use parameter @option{memdev}
-with @var{memory-backend-ram} backend as an replacement for parameter @option{mem}
-to achieve the same fake NUMA effect or a properly configured
-@var{memory-backend-file} backend to actually benefit from NUMA configuration.
-In future new machine versions will not accept the option but it will still
-work with old machine types. User can check QAPI schema to see if the legacy
-option is supported by looking at MachineInfo::numa-mem-supported property.
-
 @subsection -numa node (without memory specified) (since 4.1)
 
 Splitting RAM by default between NUMA nodes has the same issues as @option{mem}
diff --git a/qemu-options.hx b/qemu-options.hx
index 709162c..55500bd 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -223,10 +223,10 @@ For example:
 -numa cpu,node-id=0,socket-id=0 -numa cpu,node-id=1,socket-id=1
 @end example
 
-@samp{mem} assigns a given RAM amount to a node. @samp{memdev}
-assigns RAM from a given memory backend device to a node. If
-@samp{mem} and @samp{memdev} are omitted in all nodes, RAM is
-split equally between them.
+Legacy @samp{mem} assigns a given RAM amount to a node (not supported for 5.0
+and newer machine types). @samp{memdev} assigns RAM from a given memory backend
+device to a node. If @samp{mem} and @samp{memdev} are omitted in all nodes, RAM
+is split equally between them.
 
 @samp{mem} and @samp{memdev} are mutually exclusive. Furthermore,
 if one node uses @samp{memdev}, all of them have to use it.
-- 
2.7.4

Re: [libvirt] [PATCH v2 82/86] numa: forbid '-numa node, mem' for 5.0 and newer machine types

[Peter Krempa]

On Wed, Jan 15, 2020 at 16:07:37 +0100, Igor Mammedov wrote:
> Deprecation period is ran out and it's a time to flip the switch
> introduced by cd5ff8333a.
> Disable legacy option for new machine types and amend documentation.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> CC: peter.maydell@linaro.org
> CC: ehabkost@redhat.com
> CC: marcel.apfelbaum@gmail.com
> CC: mst@redhat.com
> CC: pbonzini@redhat.com
> CC: rth@twiddle.net
> CC: david@gibson.dropbear.id.au
> CC: libvir-list@redhat.com
> CC: qemu-arm@nongnu.org
> CC: qemu-ppc@nongnu.org
> ---
>  hw/arm/virt.c        |  2 +-
>  hw/core/numa.c       |  6 ++++++
>  hw/i386/pc.c         |  1 -
>  hw/i386/pc_piix.c    |  1 +
>  hw/i386/pc_q35.c     |  1 +
>  hw/ppc/spapr.c       |  2 +-
>  qemu-deprecated.texi | 16 ----------------
>  qemu-options.hx      |  8 ++++----
>  8 files changed, 14 insertions(+), 23 deletions(-)

I'm afraid nobody bothered to fix it yet:

https://bugzilla.redhat.com/show_bug.cgi?id=1783355

Re: [libvirt] [PATCH v2 82/86] numa: forbid '-numa node, mem' for 5.0 and newer machine types

[Igor Mammedov]

On Wed, 15 Jan 2020 16:34:53 +0100
Peter Krempa <pkrempa@redhat.com> wrote:

> On Wed, Jan 15, 2020 at 16:07:37 +0100, Igor Mammedov wrote:
> > Deprecation period is ran out and it's a time to flip the switch
> > introduced by cd5ff8333a.
> > Disable legacy option for new machine types and amend documentation.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > CC: peter.maydell@linaro.org
> > CC: ehabkost@redhat.com
> > CC: marcel.apfelbaum@gmail.com
> > CC: mst@redhat.com
> > CC: pbonzini@redhat.com
> > CC: rth@twiddle.net
> > CC: david@gibson.dropbear.id.au
> > CC: libvir-list@redhat.com
> > CC: qemu-arm@nongnu.org
> > CC: qemu-ppc@nongnu.org
> > ---
> >  hw/arm/virt.c        |  2 +-
> >  hw/core/numa.c       |  6 ++++++
> >  hw/i386/pc.c         |  1 -
> >  hw/i386/pc_piix.c    |  1 +
> >  hw/i386/pc_q35.c     |  1 +
> >  hw/ppc/spapr.c       |  2 +-
> >  qemu-deprecated.texi | 16 ----------------
> >  qemu-options.hx      |  8 ++++----
> >  8 files changed, 14 insertions(+), 23 deletions(-)  
> 
> I'm afraid nobody bothered to fix it yet:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1783355

It's time to start working on it :)
(looks like just deprecating stuff isn't sufficient motivation,
maybe actual switch flipping would work out better)

Re: [libvirt] [PATCH v2 82/86] numa: forbid '-numa node, mem' for 5.0 and newer machine types

[Michal Privoznik]

On 1/15/20 5:52 PM, Igor Mammedov wrote:
> On Wed, 15 Jan 2020 16:34:53 +0100
> Peter Krempa <pkrempa@redhat.com> wrote:
> 
>> On Wed, Jan 15, 2020 at 16:07:37 +0100, Igor Mammedov wrote:
>>> Deprecation period is ran out and it's a time to flip the switch
>>> introduced by cd5ff8333a.
>>> Disable legacy option for new machine types and amend documentation.
>>>
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
>>> CC: peter.maydell@linaro.org
>>> CC: ehabkost@redhat.com
>>> CC: marcel.apfelbaum@gmail.com
>>> CC: mst@redhat.com
>>> CC: pbonzini@redhat.com
>>> CC: rth@twiddle.net
>>> CC: david@gibson.dropbear.id.au
>>> CC: libvir-list@redhat.com
>>> CC: qemu-arm@nongnu.org
>>> CC: qemu-ppc@nongnu.org
>>> ---
>>>   hw/arm/virt.c        |  2 +-
>>>   hw/core/numa.c       |  6 ++++++
>>>   hw/i386/pc.c         |  1 -
>>>   hw/i386/pc_piix.c    |  1 +
>>>   hw/i386/pc_q35.c     |  1 +
>>>   hw/ppc/spapr.c       |  2 +-
>>>   qemu-deprecated.texi | 16 ----------------
>>>   qemu-options.hx      |  8 ++++----
>>>   8 files changed, 14 insertions(+), 23 deletions(-)
>>
>> I'm afraid nobody bothered to fix it yet:
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1783355
> 
> It's time to start working on it :)
> (looks like just deprecating stuff isn't sufficient motivation,
> maybe actual switch flipping would work out better)
> 

So how was the upgrade from older to newer version resolved? I mean, if 
the old qemu used -numa node,mem=XXX and it is migrated to a host with 
newer qemu, the cmd line can't be switched to -numa node,memdev=node0, 
can it? I'm asking because I've just started working on this.

Michal

Re: [libvirt] [PATCH v2 82/86] numa: forbid '-numa node, mem' for 5.0 and newer machine types

[Igor Mammedov]

On Thu, 16 Jan 2020 11:42:09 +0100
Michal Privoznik <mprivozn@redhat.com> wrote:

> On 1/15/20 5:52 PM, Igor Mammedov wrote:
> > On Wed, 15 Jan 2020 16:34:53 +0100
> > Peter Krempa <pkrempa@redhat.com> wrote:
> >   
> >> On Wed, Jan 15, 2020 at 16:07:37 +0100, Igor Mammedov wrote:  
> >>> Deprecation period is ran out and it's a time to flip the switch
> >>> introduced by cd5ff8333a.
> >>> Disable legacy option for new machine types and amend documentation.
> >>>
> >>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >>> ---
> >>> CC: peter.maydell@linaro.org
> >>> CC: ehabkost@redhat.com
> >>> CC: marcel.apfelbaum@gmail.com
> >>> CC: mst@redhat.com
> >>> CC: pbonzini@redhat.com
> >>> CC: rth@twiddle.net
> >>> CC: david@gibson.dropbear.id.au
> >>> CC: libvir-list@redhat.com
> >>> CC: qemu-arm@nongnu.org
> >>> CC: qemu-ppc@nongnu.org
> >>> ---
> >>>   hw/arm/virt.c        |  2 +-
> >>>   hw/core/numa.c       |  6 ++++++
> >>>   hw/i386/pc.c         |  1 -
> >>>   hw/i386/pc_piix.c    |  1 +
> >>>   hw/i386/pc_q35.c     |  1 +
> >>>   hw/ppc/spapr.c       |  2 +-
> >>>   qemu-deprecated.texi | 16 ----------------
> >>>   qemu-options.hx      |  8 ++++----
> >>>   8 files changed, 14 insertions(+), 23 deletions(-)  
> >>
> >> I'm afraid nobody bothered to fix it yet:
> >>
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1783355  
> > 
> > It's time to start working on it :)
> > (looks like just deprecating stuff isn't sufficient motivation,
> > maybe actual switch flipping would work out better)
> >   
> 
> So how was the upgrade from older to newer version resolved? I mean, if 
> the old qemu used -numa node,mem=XXX and it is migrated to a host with 
> newer qemu, the cmd line can't be switched to -numa node,memdev=node0, 
> can it? I'm asking because I've just started working on this.

see commit cd5ff8333a3c87 for detailed info.
Short answer is it's not really resolved [*],
-numa node,mem will keep working on newer QEMU but only for old machine types
new machine types will accept only -numa node,memdev.

One can check if "mem=' is supported by using QAPI query-machines
and checking numa-mem-supported field. That field is flipped to false
for 5.0 and later machine types in this patch.


*) I might give another try to removing 'mem' completely in migration
compatible manner but that's well beyond the scope of this series
So far I hasn't been able to convince myself that previous attempts
to do it were absolutely correct for all corner cases that are there.

> Michal

Re: [libvirt] [PATCH v2 82/86] numa: forbid '-numa node, mem' for 5.0 and newer machine types

[Michal Privoznik]

On 1/16/20 1:37 PM, Igor Mammedov wrote:
> On Thu, 16 Jan 2020 11:42:09 +0100
> Michal Privoznik <mprivozn@redhat.com> wrote:
> 
>> On 1/15/20 5:52 PM, Igor Mammedov wrote:
>>> On Wed, 15 Jan 2020 16:34:53 +0100
>>> Peter Krempa <pkrempa@redhat.com> wrote:
>>>    
>>>> On Wed, Jan 15, 2020 at 16:07:37 +0100, Igor Mammedov wrote:
>>>>> Deprecation period is ran out and it's a time to flip the switch
>>>>> introduced by cd5ff8333a.
>>>>> Disable legacy option for new machine types and amend documentation.
>>>>>
>>>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>>>> ---
>>>>> CC: peter.maydell@linaro.org
>>>>> CC: ehabkost@redhat.com
>>>>> CC: marcel.apfelbaum@gmail.com
>>>>> CC: mst@redhat.com
>>>>> CC: pbonzini@redhat.com
>>>>> CC: rth@twiddle.net
>>>>> CC: david@gibson.dropbear.id.au
>>>>> CC: libvir-list@redhat.com
>>>>> CC: qemu-arm@nongnu.org
>>>>> CC: qemu-ppc@nongnu.org
>>>>> ---
>>>>>    hw/arm/virt.c        |  2 +-
>>>>>    hw/core/numa.c       |  6 ++++++
>>>>>    hw/i386/pc.c         |  1 -
>>>>>    hw/i386/pc_piix.c    |  1 +
>>>>>    hw/i386/pc_q35.c     |  1 +
>>>>>    hw/ppc/spapr.c       |  2 +-
>>>>>    qemu-deprecated.texi | 16 ----------------
>>>>>    qemu-options.hx      |  8 ++++----
>>>>>    8 files changed, 14 insertions(+), 23 deletions(-)
>>>>
>>>> I'm afraid nobody bothered to fix it yet:
>>>>
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1783355
>>>
>>> It's time to start working on it :)
>>> (looks like just deprecating stuff isn't sufficient motivation,
>>> maybe actual switch flipping would work out better)
>>>    
>>
>> So how was the upgrade from older to newer version resolved? I mean, if
>> the old qemu used -numa node,mem=XXX and it is migrated to a host with
>> newer qemu, the cmd line can't be switched to -numa node,memdev=node0,
>> can it? I'm asking because I've just started working on this.
> 
> see commit cd5ff8333a3c87 for detailed info.
> Short answer is it's not really resolved [*],
> -numa node,mem will keep working on newer QEMU but only for old machine types
> new machine types will accept only -numa node,memdev.
> 
> One can check if "mem=' is supported by using QAPI query-machines
> and checking numa-mem-supported field. That field is flipped to false
> for 5.0 and later machine types in this patch.

Alright, so what we can do is the following:

1) For new machine types (pc-5.0/q35-5.0 and newer) use memdev= always.

2) For older machine types, we are stuck with mem= until qemu is capable 
of migrating from mem= to memdev=

I think this is a safe thing to do since migrating from one version of a 
machine type to another is not supported (since it can change guest 
ABI). And we will see how much 2) bothers us. Does this sound reasonable?

Michal

Re: [libvirt] [PATCH v2 82/86] numa: forbid '-numa node, mem' for 5.0 and newer machine types

[Igor Mammedov]

On Thu, 16 Jan 2020 14:03:12 +0100
Michal Privoznik <mprivozn@redhat.com> wrote:

> On 1/16/20 1:37 PM, Igor Mammedov wrote:
> > On Thu, 16 Jan 2020 11:42:09 +0100
> > Michal Privoznik <mprivozn@redhat.com> wrote:
> >   
> >> On 1/15/20 5:52 PM, Igor Mammedov wrote:  
> >>> On Wed, 15 Jan 2020 16:34:53 +0100
> >>> Peter Krempa <pkrempa@redhat.com> wrote:
> >>>      
> >>>> On Wed, Jan 15, 2020 at 16:07:37 +0100, Igor Mammedov wrote:  
> >>>>> Deprecation period is ran out and it's a time to flip the switch
> >>>>> introduced by cd5ff8333a.
> >>>>> Disable legacy option for new machine types and amend documentation.
> >>>>>
> >>>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >>>>> ---
> >>>>> CC: peter.maydell@linaro.org
> >>>>> CC: ehabkost@redhat.com
> >>>>> CC: marcel.apfelbaum@gmail.com
> >>>>> CC: mst@redhat.com
> >>>>> CC: pbonzini@redhat.com
> >>>>> CC: rth@twiddle.net
> >>>>> CC: david@gibson.dropbear.id.au
> >>>>> CC: libvir-list@redhat.com
> >>>>> CC: qemu-arm@nongnu.org
> >>>>> CC: qemu-ppc@nongnu.org
> >>>>> ---
> >>>>>    hw/arm/virt.c        |  2 +-
> >>>>>    hw/core/numa.c       |  6 ++++++
> >>>>>    hw/i386/pc.c         |  1 -
> >>>>>    hw/i386/pc_piix.c    |  1 +
> >>>>>    hw/i386/pc_q35.c     |  1 +
> >>>>>    hw/ppc/spapr.c       |  2 +-
> >>>>>    qemu-deprecated.texi | 16 ----------------
> >>>>>    qemu-options.hx      |  8 ++++----
> >>>>>    8 files changed, 14 insertions(+), 23 deletions(-)  
> >>>>
> >>>> I'm afraid nobody bothered to fix it yet:
> >>>>
> >>>> https://bugzilla.redhat.com/show_bug.cgi?id=1783355  
> >>>
> >>> It's time to start working on it :)
> >>> (looks like just deprecating stuff isn't sufficient motivation,
> >>> maybe actual switch flipping would work out better)
> >>>      
> >>
> >> So how was the upgrade from older to newer version resolved? I mean, if
> >> the old qemu used -numa node,mem=XXX and it is migrated to a host with
> >> newer qemu, the cmd line can't be switched to -numa node,memdev=node0,
> >> can it? I'm asking because I've just started working on this.  
> > 
> > see commit cd5ff8333a3c87 for detailed info.
> > Short answer is it's not really resolved [*],
> > -numa node,mem will keep working on newer QEMU but only for old machine types
> > new machine types will accept only -numa node,memdev.
> > 
> > One can check if "mem=' is supported by using QAPI query-machines
> > and checking numa-mem-supported field. That field is flipped to false
> > for 5.0 and later machine types in this patch.  
> 
> Alright, so what we can do is the following:
> 
> 1) For new machine types (pc-5.0/q35-5.0 and newer) use memdev= always.
it's not only x86, it's for all machines that support numa
hence numa-mem-supported was introduced to make it easier for libvirt
to figure out when to use which syntax.

The plan was to release libvirt with support for numa-mem-supported and
then when newer QEMU forbids 'mem=' it change will be transparent for
relatively fresh livirt.

Whether it still does make sense though.

We could go with your suggestion in which case libvirt unilaterally
switches to using only 'memdev' for 5.0 machine types and then later
(5.1..) we release QEMU that enforces it.
In this case we can axe numa-mem-supported (I'd volunteer) to avoid
supporting yet another ABI/smart logic where your way could be sufficient.

Daniel,
what's your take on Michal's approach?

> 2) For older machine types, we are stuck with mem= until qemu is capable 
> of migrating from mem= to memdev=
> 
> I think this is a safe thing to do since migrating from one version of a 
> machine type to another is not supported (since it can change guest 
> ABI). And we will see how much 2) bothers us. Does this sound reasonable?\


> 
> Michal
> 
> 

Re: [libvirt] [PATCH v2 82/86] numa: forbid '-numa node, mem' for 5.0 and newer machine types

[Daniel P. Berrangé]

On Thu, Jan 16, 2020 at 01:37:03PM +0100, Igor Mammedov wrote:
> On Thu, 16 Jan 2020 11:42:09 +0100
> Michal Privoznik <mprivozn@redhat.com> wrote:
> 
> > On 1/15/20 5:52 PM, Igor Mammedov wrote:
> > > On Wed, 15 Jan 2020 16:34:53 +0100
> > > Peter Krempa <pkrempa@redhat.com> wrote:
> > >   
> > >> On Wed, Jan 15, 2020 at 16:07:37 +0100, Igor Mammedov wrote:  
> > >>> Deprecation period is ran out and it's a time to flip the switch
> > >>> introduced by cd5ff8333a.
> > >>> Disable legacy option for new machine types and amend documentation.
> > >>>
> > >>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > >>> ---
> > >>> CC: peter.maydell@linaro.org
> > >>> CC: ehabkost@redhat.com
> > >>> CC: marcel.apfelbaum@gmail.com
> > >>> CC: mst@redhat.com
> > >>> CC: pbonzini@redhat.com
> > >>> CC: rth@twiddle.net
> > >>> CC: david@gibson.dropbear.id.au
> > >>> CC: libvir-list@redhat.com
> > >>> CC: qemu-arm@nongnu.org
> > >>> CC: qemu-ppc@nongnu.org
> > >>> ---
> > >>>   hw/arm/virt.c        |  2 +-
> > >>>   hw/core/numa.c       |  6 ++++++
> > >>>   hw/i386/pc.c         |  1 -
> > >>>   hw/i386/pc_piix.c    |  1 +
> > >>>   hw/i386/pc_q35.c     |  1 +
> > >>>   hw/ppc/spapr.c       |  2 +-
> > >>>   qemu-deprecated.texi | 16 ----------------
> > >>>   qemu-options.hx      |  8 ++++----
> > >>>   8 files changed, 14 insertions(+), 23 deletions(-)  
> > >>
> > >> I'm afraid nobody bothered to fix it yet:
> > >>
> > >> https://bugzilla.redhat.com/show_bug.cgi?id=1783355  
> > > 
> > > It's time to start working on it :)
> > > (looks like just deprecating stuff isn't sufficient motivation,
> > > maybe actual switch flipping would work out better)
> > >   
> > 
> > So how was the upgrade from older to newer version resolved? I mean, if 
> > the old qemu used -numa node,mem=XXX and it is migrated to a host with 
> > newer qemu, the cmd line can't be switched to -numa node,memdev=node0, 
> > can it? I'm asking because I've just started working on this.
> 
> see commit cd5ff8333a3c87 for detailed info.
> Short answer is it's not really resolved [*],
> -numa node,mem will keep working on newer QEMU but only for old machine types
> new machine types will accept only -numa node,memdev.
> 
> One can check if "mem=' is supported by using QAPI query-machines
> and checking numa-mem-supported field. That field is flipped to false
> for 5.0 and later machine types in this patch.

Since libvirt droppped the ball here, can we postpone this change
to machine types until a later release. 


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [libvirt] [PATCH v2 82/86] numa: forbid '-numa node, mem' for 5.0 and newer machine types

[Igor Mammedov]

On Thu, 16 Jan 2020 13:06:28 +0000
Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Thu, Jan 16, 2020 at 01:37:03PM +0100, Igor Mammedov wrote:
> > On Thu, 16 Jan 2020 11:42:09 +0100
> > Michal Privoznik <mprivozn@redhat.com> wrote:
> >   
> > > On 1/15/20 5:52 PM, Igor Mammedov wrote:  
> > > > On Wed, 15 Jan 2020 16:34:53 +0100
> > > > Peter Krempa <pkrempa@redhat.com> wrote:
> > > >     
> > > >> On Wed, Jan 15, 2020 at 16:07:37 +0100, Igor Mammedov wrote:    
> > > >>> Deprecation period is ran out and it's a time to flip the switch
> > > >>> introduced by cd5ff8333a.
> > > >>> Disable legacy option for new machine types and amend documentation.
> > > >>>
> > > >>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > >>> ---
> > > >>> CC: peter.maydell@linaro.org
> > > >>> CC: ehabkost@redhat.com
> > > >>> CC: marcel.apfelbaum@gmail.com
> > > >>> CC: mst@redhat.com
> > > >>> CC: pbonzini@redhat.com
> > > >>> CC: rth@twiddle.net
> > > >>> CC: david@gibson.dropbear.id.au
> > > >>> CC: libvir-list@redhat.com
> > > >>> CC: qemu-arm@nongnu.org
> > > >>> CC: qemu-ppc@nongnu.org
> > > >>> ---
> > > >>>   hw/arm/virt.c        |  2 +-
> > > >>>   hw/core/numa.c       |  6 ++++++
> > > >>>   hw/i386/pc.c         |  1 -
> > > >>>   hw/i386/pc_piix.c    |  1 +
> > > >>>   hw/i386/pc_q35.c     |  1 +
> > > >>>   hw/ppc/spapr.c       |  2 +-
> > > >>>   qemu-deprecated.texi | 16 ----------------
> > > >>>   qemu-options.hx      |  8 ++++----
> > > >>>   8 files changed, 14 insertions(+), 23 deletions(-)    
> > > >>
> > > >> I'm afraid nobody bothered to fix it yet:
> > > >>
> > > >> https://bugzilla.redhat.com/show_bug.cgi?id=1783355    
> > > > 
> > > > It's time to start working on it :)
> > > > (looks like just deprecating stuff isn't sufficient motivation,
> > > > maybe actual switch flipping would work out better)
> > > >     
> > > 
> > > So how was the upgrade from older to newer version resolved? I mean, if 
> > > the old qemu used -numa node,mem=XXX and it is migrated to a host with 
> > > newer qemu, the cmd line can't be switched to -numa node,memdev=node0, 
> > > can it? I'm asking because I've just started working on this.  
> > 
> > see commit cd5ff8333a3c87 for detailed info.
> > Short answer is it's not really resolved [*],
> > -numa node,mem will keep working on newer QEMU but only for old machine types
> > new machine types will accept only -numa node,memdev.
> > 
> > One can check if "mem=' is supported by using QAPI query-machines
> > and checking numa-mem-supported field. That field is flipped to false
> > for 5.0 and later machine types in this patch.  
> 
> Since libvirt droppped the ball here, can we postpone this change
> to machine types until a later release. 

Looks like we have to at this point.
We can do this for [82-86/86] patches which are mostly numa
related changes.

The rest could go in this release as it is in-depended of
numa, it mainly introduces memdev backend for main RAM
and consolidates twisted main RAM allocation logic.

> 
> Regards,
> Daniel

Re: [PATCH v2 82/86] numa: forbid '-numa node, mem' for 5.0 and newer machine types

[David Gibson]

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

On Wed, Jan 15, 2020 at 04:07:37PM +0100, Igor Mammedov wrote:
> Deprecation period is ran out and it's a time to flip the switch
> introduced by cd5ff8333a.
> Disable legacy option for new machine types and amend documentation.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

> ---
> CC: peter.maydell@linaro.org
> CC: ehabkost@redhat.com
> CC: marcel.apfelbaum@gmail.com
> CC: mst@redhat.com
> CC: pbonzini@redhat.com
> CC: rth@twiddle.net
> CC: david@gibson.dropbear.id.au
> CC: libvir-list@redhat.com
> CC: qemu-arm@nongnu.org
> CC: qemu-ppc@nongnu.org
> ---
>  hw/arm/virt.c        |  2 +-
>  hw/core/numa.c       |  6 ++++++
>  hw/i386/pc.c         |  1 -
>  hw/i386/pc_piix.c    |  1 +
>  hw/i386/pc_q35.c     |  1 +
>  hw/ppc/spapr.c       |  2 +-
>  qemu-deprecated.texi | 16 ----------------
>  qemu-options.hx      |  8 ++++----
>  8 files changed, 14 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index e2fbca3..49de0d8 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2049,7 +2049,6 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>      hc->pre_plug = virt_machine_device_pre_plug_cb;
>      hc->plug = virt_machine_device_plug_cb;
>      hc->unplug_request = virt_machine_device_unplug_request_cb;
> -    mc->numa_mem_supported = true;
>      mc->auto_enable_numa_with_memhp = true;
>      mc->default_ram_id = "mach-virt.ram";
>  }
> @@ -2153,6 +2152,7 @@ DEFINE_VIRT_MACHINE_AS_LATEST(5, 0)
>  static void virt_machine_4_2_options(MachineClass *mc)
>  {
>      compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
> +    mc->numa_mem_supported = true;
>  }
>  DEFINE_VIRT_MACHINE(4, 2)
>  
> diff --git a/hw/core/numa.c b/hw/core/numa.c
> index 0970a30..3177066 100644
> --- a/hw/core/numa.c
> +++ b/hw/core/numa.c
> @@ -117,6 +117,12 @@ static void parse_numa_node(MachineState *ms, NumaNodeOptions *node,
>      }
>  
>      if (node->has_mem) {
> +        if (!mc->numa_mem_supported) {
> +            error_setg(errp, "Parameter -numa node,mem is not supported by this"
> +                      " machine type. Use -numa node,memdev instead");
> +            return;
> +        }
> +
>          numa_info[nodenr].node_mem = node->mem;
>          if (!qtest_enabled()) {
>              warn_report("Parameter -numa node,mem is deprecated,"
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 21b8290..fa8d024 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1947,7 +1947,6 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      hc->unplug = pc_machine_device_unplug_cb;
>      mc->default_cpu_type = TARGET_DEFAULT_CPU_TYPE;
>      mc->nvdimm_supported = true;
> -    mc->numa_mem_supported = true;
>      mc->default_ram_id = "pc.ram";
>  
>      object_class_property_add(oc, PC_MACHINE_DEVMEM_REGION_SIZE, "int",
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index fa12203..0a9b9e0 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -435,6 +435,7 @@ static void pc_i440fx_4_2_machine_options(MachineClass *m)
>      pc_i440fx_5_0_machine_options(m);
>      m->alias = NULL;
>      m->is_default = 0;
> +    m->numa_mem_supported = true;
>      compat_props_add(m->compat_props, hw_compat_4_2, hw_compat_4_2_len);
>      compat_props_add(m->compat_props, pc_compat_4_2, pc_compat_4_2_len);
>  }
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 84cf925..4d6e2be 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -363,6 +363,7 @@ static void pc_q35_4_2_machine_options(MachineClass *m)
>  {
>      pc_q35_5_0_machine_options(m);
>      m->alias = NULL;
> +    m->numa_mem_supported = true;
>      compat_props_add(m->compat_props, hw_compat_4_2, hw_compat_4_2_len);
>      compat_props_add(m->compat_props, pc_compat_4_2, pc_compat_4_2_len);
>  }
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index bcbe1f1..2686b73 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4383,7 +4383,6 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>       * in which LMBs are represented and hot-added
>       */
>      mc->numa_mem_align_shift = 28;
> -    mc->numa_mem_supported = true;
>      mc->auto_enable_numa = true;
>  
>      smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
> @@ -4465,6 +4464,7 @@ static void spapr_machine_4_2_class_options(MachineClass *mc)
>  {
>      spapr_machine_5_0_class_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
> +    mc->numa_mem_supported = true;
>  }
>  
>  DEFINE_SPAPR_MACHINE(4_2, "4.2", false);
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 982af95..17a0e1d 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -89,22 +89,6 @@ error in the future.
>  The @code{-realtime mlock=on|off} argument has been replaced by the
>  @code{-overcommit mem-lock=on|off} argument.
>  
> -@subsection -numa node,mem=@var{size} (since 4.1)
> -
> -The parameter @option{mem} of @option{-numa node} is used to assign a part of
> -guest RAM to a NUMA node. But when using it, it's impossible to manage specified
> -RAM chunk on the host side (like bind it to a host node, setting bind policy, ...),
> -so guest end-ups with the fake NUMA configuration with suboptiomal performance.
> -However since 2014 there is an alternative way to assign RAM to a NUMA node
> -using parameter @option{memdev}, which does the same as @option{mem} and adds
> -means to actualy manage node RAM on the host side. Use parameter @option{memdev}
> -with @var{memory-backend-ram} backend as an replacement for parameter @option{mem}
> -to achieve the same fake NUMA effect or a properly configured
> -@var{memory-backend-file} backend to actually benefit from NUMA configuration.
> -In future new machine versions will not accept the option but it will still
> -work with old machine types. User can check QAPI schema to see if the legacy
> -option is supported by looking at MachineInfo::numa-mem-supported property.
> -
>  @subsection -numa node (without memory specified) (since 4.1)
>  
>  Splitting RAM by default between NUMA nodes has the same issues as @option{mem}
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 709162c..55500bd 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -223,10 +223,10 @@ For example:
>  -numa cpu,node-id=0,socket-id=0 -numa cpu,node-id=1,socket-id=1
>  @end example
>  
> -@samp{mem} assigns a given RAM amount to a node. @samp{memdev}
> -assigns RAM from a given memory backend device to a node. If
> -@samp{mem} and @samp{memdev} are omitted in all nodes, RAM is
> -split equally between them.
> +Legacy @samp{mem} assigns a given RAM amount to a node (not supported for 5.0
> +and newer machine types). @samp{memdev} assigns RAM from a given memory backend
> +device to a node. If @samp{mem} and @samp{memdev} are omitted in all nodes, RAM
> +is split equally between them.
>  
>  @samp{mem} and @samp{memdev} are mutually exclusive. Furthermore,
>  if one node uses @samp{memdev}, all of them have to use it.

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