[Qemu-devel] [PATCH 0/4] tests for -m option

[Qemu-devel] [PATCH 0/4] tests for -m option

[Igor Mammedov]

Series adds properties to machine class for its ram_size,
ram_slots, maxram_size fields making them available via QMP
interface so that management tools and unit tests could
query values.

The last patch provides rudimentary -m option testing and
basis for memory hotplug tests that will be added later.

git repo for testing:
  https://github.com/imammedo/qemu/commits/memory-hotplug-tests-v1


Igor Mammedov (4):
  switch RAM_ADDR_FMT format specifier to print decimal
  vl.c: use single local_err throughout main()
  machine: convert ram_size, maxram_size, ram_slots to properties
  test -m option parameters

 arch_init.c               |   14 ++--
 exec.c                    |    4 +-
 hw/core/machine.c         |  181 +++++++++++++++++++++++++++++++++++++++++++++
 hw/i386/pc.c              |    4 +-
 hw/ppc/ppc405_boards.c    |    2 +-
 include/exec/cpu-common.h |    6 +-
 include/hw/boards.h       |    5 +
 numa.c                    |    2 +-
 tests/Makefile            |    2 +
 tests/memhp-test.c        |  145 ++++++++++++++++++++++++++++++++++++
 vl.c                      |  127 ++++++++++++++------------------
 xen-hvm.c                 |   11 +--
 12 files changed, 410 insertions(+), 93 deletions(-)
 create mode 100644 tests/memhp-test.c

[Qemu-devel] [PATCH 1/4] switch RAM_ADDR_FMT format specifier to print decimal

[Igor Mammedov]

... and add RAM_ADDR_FMTX for hex format. In addition fix
places to print sizes/lengths as decimal and addresses as hex.

Also drop "%" from RAM_ADDR_FMT macro and use it like other
PRIfoo format placeholders specifying "%" explicitly.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 arch_init.c               |   14 +++++++-------
 exec.c                    |    4 ++--
 hw/i386/pc.c              |    4 ++--
 hw/ppc/ppc405_boards.c    |    2 +-
 include/exec/cpu-common.h |    6 ++++--
 numa.c                    |    2 +-
 vl.c                      |    6 +++---
 xen-hvm.c                 |   11 +++++------
 8 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 8ddaf35..7562325 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -1072,8 +1072,8 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
                 QTAILQ_FOREACH(block, &ram_list.blocks, next) {
                     if (!strncmp(id, block->idstr, sizeof(id))) {
                         if (block->length != length) {
-                            error_report("Length mismatch: %s: " RAM_ADDR_FMT
-                                         " in != " RAM_ADDR_FMT, id, length,
+                            error_report("Length mismatch: %s: %" RAM_ADDR_FMT
+                                         " in != %" RAM_ADDR_FMT, id, length,
                                          block->length);
                             ret =  -EINVAL;
                         }
@@ -1098,7 +1098,7 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
 
             host = host_from_stream_offset(f, addr, flags);
             if (!host) {
-                error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
+                error_report("Illegal RAM offset %" RAM_ADDR_FMTX, addr);
                 ret = -EINVAL;
                 break;
             }
@@ -1110,7 +1110,7 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
 
             host = host_from_stream_offset(f, addr, flags);
             if (!host) {
-                error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
+                error_report("Illegal RAM offset %" RAM_ADDR_FMTX, addr);
                 ret = -EINVAL;
                 break;
             }
@@ -1119,14 +1119,14 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
         } else if (flags & RAM_SAVE_FLAG_XBZRLE) {
             void *host = host_from_stream_offset(f, addr, flags);
             if (!host) {
-                error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
+                error_report("Illegal RAM offset %" RAM_ADDR_FMTX, addr);
                 ret = -EINVAL;
                 break;
             }
 
             if (load_xbzrle(f, addr, host) < 0) {
-                error_report("Failed to decompress XBZRLE page at "
-                             RAM_ADDR_FMT, addr);
+                error_report("Failed to decompress XBZRLE page at %"
+                             RAM_ADDR_FMTX, addr);
                 ret = -EINVAL;
                 break;
             }
diff --git a/exec.c b/exec.c
index c849405..f9ae8a4 100644
--- a/exec.c
+++ b/exec.c
@@ -1435,8 +1435,8 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length)
                                 flags, -1, 0);
                 }
                 if (area != vaddr) {
-                    fprintf(stderr, "Could not remap addr: "
-                            RAM_ADDR_FMT "@" RAM_ADDR_FMT "\n",
+                    fprintf(stderr, "Could not remap addr: %"
+                            RAM_ADDR_FMTX "@%" RAM_ADDR_FMTX "\n",
                             length, addr);
                     exit(1);
                 }
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 2cf22b1..a6e7ef3 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1257,8 +1257,8 @@ FWCfgState *pc_memory_init(MachineState *machine,
 
         if ((pcms->hotplug_memory_base + hotplug_mem_size) <
             hotplug_mem_size) {
-            error_report("unsupported amount of maximum memory: " RAM_ADDR_FMT,
-                         machine->maxram_size);
+            error_report("unsupported amount of maximum memory: %"
+                         RAM_ADDR_FMT, machine->maxram_size);
             exit(EXIT_FAILURE);
         }
 
diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
index 98ad2d7..2b1b4fe 100644
--- a/hw/ppc/ppc405_boards.c
+++ b/hw/ppc/ppc405_boards.c
@@ -358,7 +358,7 @@ static void ref405ep_init(MachineState *machine)
         bdloc = 0;
     }
 #ifdef DEBUG_BOARD_INIT
-    printf("bdloc " RAM_ADDR_FMT "\n", bdloc);
+    printf("bdloc %" RAM_ADDR_FMTX "\n", bdloc);
     printf("%s: Done\n", __func__);
 #endif
 }
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index e3ec4c8..6899d0c 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -38,11 +38,13 @@ enum device_endian {
 #if defined(CONFIG_XEN_BACKEND)
 typedef uint64_t ram_addr_t;
 #  define RAM_ADDR_MAX UINT64_MAX
-#  define RAM_ADDR_FMT "%" PRIx64
+#  define RAM_ADDR_FMTX PRIx64
+#  define RAM_ADDR_FMT PRIu64
 #else
 typedef uintptr_t ram_addr_t;
 #  define RAM_ADDR_MAX UINTPTR_MAX
-#  define RAM_ADDR_FMT "%" PRIxPTR
+#  define RAM_ADDR_FMTX PRIxPTR
+#  define RAM_ADDR_FMT PRIuPTR
 #endif
 
 extern ram_addr_t ram_size;
diff --git a/numa.c b/numa.c
index e471afe..d9f2655 100644
--- a/numa.c
+++ b/numa.c
@@ -189,7 +189,7 @@ void set_numa_nodes(void)
         }
         if (numa_total != ram_size) {
             error_report("total memory for NUMA nodes (%" PRIu64 ")"
-                         " should equal RAM size (" RAM_ADDR_FMT ")",
+                         " should equal RAM size (%" RAM_ADDR_FMT ")",
                          numa_total, ram_size);
             exit(1);
         }
diff --git a/vl.c b/vl.c
index a1686ef..6b09220 100644
--- a/vl.c
+++ b/vl.c
@@ -3319,7 +3319,7 @@ int main(int argc, char **argv, char **envp)
                     sz = qemu_opt_get_size(opts, "maxmem", 0);
                     if (sz < ram_size) {
                         fprintf(stderr, "qemu: invalid -m option value: maxmem "
-                                "(%" PRIu64 ") <= initial memory ("
+                                "(%" PRIu64 ") <= initial memory (%"
                                 RAM_ADDR_FMT ")\n", sz, ram_size);
                         exit(EXIT_FAILURE);
                     }
@@ -3327,7 +3327,7 @@ int main(int argc, char **argv, char **envp)
                     slots = qemu_opt_get_number(opts, "slots", 0);
                     if ((sz > ram_size) && !slots) {
                         fprintf(stderr, "qemu: invalid -m option value: maxmem "
-                                "(%" PRIu64 ") more than initial memory ("
+                                "(%" PRIu64 ") more than initial memory (%"
                                 RAM_ADDR_FMT ") but no hotplug slots where "
                                 "specified\n", sz, ram_size);
                         exit(EXIT_FAILURE);
@@ -3336,7 +3336,7 @@ int main(int argc, char **argv, char **envp)
                     if ((sz <= ram_size) && slots) {
                         fprintf(stderr, "qemu: invalid -m option value:  %"
                                 PRIu64 " hotplug slots where specified but "
-                                "maxmem (%" PRIu64 ") <= initial memory ("
+                                "maxmem (%" PRIu64 ") <= initial memory (%"
                                 RAM_ADDR_FMT ")\n", slots, sz, ram_size);
                         exit(EXIT_FAILURE);
                     }
diff --git a/xen-hvm.c b/xen-hvm.c
index bafdf12..e7b4b76 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -221,8 +221,8 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr)
 
     if (runstate_check(RUN_STATE_INMIGRATE)) {
         /* RAM already populated in Xen */
-        fprintf(stderr, "%s: do not alloc "RAM_ADDR_FMT
-                " bytes of ram at "RAM_ADDR_FMT" when runstate is INMIGRATE\n",
+        fprintf(stderr, "%s: do not alloc %"RAM_ADDR_FMT" bytes"
+                " of ram at %"RAM_ADDR_FMTX" when runstate is INMIGRATE\n",
                 __func__, size, ram_addr); 
         return;
     }
@@ -241,7 +241,7 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr)
     }
 
     if (xc_domain_populate_physmap_exact(xen_xc, xen_domid, nr_pfn, 0, 0, pfn_list)) {
-        hw_error("xen: failed to populate ram at " RAM_ADDR_FMT, ram_addr);
+        hw_error("xen: failed to populate ram at %" RAM_ADDR_FMTX, ram_addr);
     }
 
     g_free(pfn_list);
@@ -1127,9 +1127,8 @@ void xen_modified_memory(ram_addr_t start, ram_addr_t length)
             - start_pfn;
         rc = xc_hvm_modified_memory(xen_xc, xen_domid, start_pfn, nb_pages);
         if (rc) {
-            fprintf(stderr,
-                    "%s failed for "RAM_ADDR_FMT" ("RAM_ADDR_FMT"): %i, %s\n",
-                    __func__, start, nb_pages, rc, strerror(-rc));
+            fprintf(stderr, "%s failed for %"RAM_ADDR_FMTX" (%"RAM_ADDR_FMT"):"
+                    " %i, %s\n", __func__, start, nb_pages, rc, strerror(-rc));
         }
     }
 }
-- 
1.7.1

Re: [Qemu-devel] [PATCH 1/4] switch RAM_ADDR_FMT format specifier to print decimal

[Michael S. Tsirkin]

On Wed, Jun 25, 2014 at 01:42:20PM +0200, Igor Mammedov wrote:
> ... and add RAM_ADDR_FMTX for hex format. In addition fix
> places to print sizes/lengths as decimal and addresses as hex.

ugh.
maybe both - though I really think most people will find
it easier to parse he hex variant.

> Also drop "%" from RAM_ADDR_FMT macro and use it like other
> PRIfoo format placeholders specifying "%" explicitly.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  arch_init.c               |   14 +++++++-------
>  exec.c                    |    4 ++--
>  hw/i386/pc.c              |    4 ++--
>  hw/ppc/ppc405_boards.c    |    2 +-
>  include/exec/cpu-common.h |    6 ++++--
>  numa.c                    |    2 +-
>  vl.c                      |    6 +++---
>  xen-hvm.c                 |   11 +++++------
>  8 files changed, 25 insertions(+), 24 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 8ddaf35..7562325 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -1072,8 +1072,8 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>                  QTAILQ_FOREACH(block, &ram_list.blocks, next) {
>                      if (!strncmp(id, block->idstr, sizeof(id))) {
>                          if (block->length != length) {
> -                            error_report("Length mismatch: %s: " RAM_ADDR_FMT
> -                                         " in != " RAM_ADDR_FMT, id, length,
> +                            error_report("Length mismatch: %s: %" RAM_ADDR_FMT
> +                                         " in != %" RAM_ADDR_FMT, id, length,
>                                           block->length);

Not sure I like this.
length values are often powers of two.
Parsing them in decimal is hard.

>                              ret =  -EINVAL;
>                          }
> @@ -1098,7 +1098,7 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>  
>              host = host_from_stream_offset(f, addr, flags);
>              if (!host) {
> -                error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
> +                error_report("Illegal RAM offset %" RAM_ADDR_FMTX, addr);
>                  ret = -EINVAL;
>                  break;
>              }
> @@ -1110,7 +1110,7 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>  
>              host = host_from_stream_offset(f, addr, flags);
>              if (!host) {
> -                error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
> +                error_report("Illegal RAM offset %" RAM_ADDR_FMTX, addr);
>                  ret = -EINVAL;
>                  break;
>              }
> @@ -1119,14 +1119,14 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>          } else if (flags & RAM_SAVE_FLAG_XBZRLE) {
>              void *host = host_from_stream_offset(f, addr, flags);
>              if (!host) {
> -                error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
> +                error_report("Illegal RAM offset %" RAM_ADDR_FMTX, addr);
>                  ret = -EINVAL;
>                  break;
>              }
>  
>              if (load_xbzrle(f, addr, host) < 0) {
> -                error_report("Failed to decompress XBZRLE page at "
> -                             RAM_ADDR_FMT, addr);
> +                error_report("Failed to decompress XBZRLE page at %"
> +                             RAM_ADDR_FMTX, addr);
>                  ret = -EINVAL;
>                  break;
>              }
> diff --git a/exec.c b/exec.c
> index c849405..f9ae8a4 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1435,8 +1435,8 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length)
>                                  flags, -1, 0);
>                  }
>                  if (area != vaddr) {
> -                    fprintf(stderr, "Could not remap addr: "
> -                            RAM_ADDR_FMT "@" RAM_ADDR_FMT "\n",
> +                    fprintf(stderr, "Could not remap addr: %"
> +                            RAM_ADDR_FMTX "@%" RAM_ADDR_FMTX "\n",
>                              length, addr);
>                      exit(1);
>                  }
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 2cf22b1..a6e7ef3 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1257,8 +1257,8 @@ FWCfgState *pc_memory_init(MachineState *machine,
>  
>          if ((pcms->hotplug_memory_base + hotplug_mem_size) <
>              hotplug_mem_size) {
> -            error_report("unsupported amount of maximum memory: " RAM_ADDR_FMT,
> -                         machine->maxram_size);
> +            error_report("unsupported amount of maximum memory: %"
> +                         RAM_ADDR_FMT, machine->maxram_size);
>              exit(EXIT_FAILURE);
>          }
>  
> diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
> index 98ad2d7..2b1b4fe 100644
> --- a/hw/ppc/ppc405_boards.c
> +++ b/hw/ppc/ppc405_boards.c
> @@ -358,7 +358,7 @@ static void ref405ep_init(MachineState *machine)
>          bdloc = 0;
>      }
>  #ifdef DEBUG_BOARD_INIT
> -    printf("bdloc " RAM_ADDR_FMT "\n", bdloc);
> +    printf("bdloc %" RAM_ADDR_FMTX "\n", bdloc);
>      printf("%s: Done\n", __func__);
>  #endif
>  }
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index e3ec4c8..6899d0c 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -38,11 +38,13 @@ enum device_endian {
>  #if defined(CONFIG_XEN_BACKEND)
>  typedef uint64_t ram_addr_t;
>  #  define RAM_ADDR_MAX UINT64_MAX
> -#  define RAM_ADDR_FMT "%" PRIx64
> +#  define RAM_ADDR_FMTX PRIx64
> +#  define RAM_ADDR_FMT PRIu64
>  #else
>  typedef uintptr_t ram_addr_t;
>  #  define RAM_ADDR_MAX UINTPTR_MAX
> -#  define RAM_ADDR_FMT "%" PRIxPTR
> +#  define RAM_ADDR_FMTX PRIxPTR
> +#  define RAM_ADDR_FMT PRIuPTR
>  #endif
>  
>  extern ram_addr_t ram_size;
> diff --git a/numa.c b/numa.c
> index e471afe..d9f2655 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -189,7 +189,7 @@ void set_numa_nodes(void)
>          }
>          if (numa_total != ram_size) {
>              error_report("total memory for NUMA nodes (%" PRIu64 ")"
> -                         " should equal RAM size (" RAM_ADDR_FMT ")",
> +                         " should equal RAM size (%" RAM_ADDR_FMT ")",
>                           numa_total, ram_size);
>              exit(1);
>          }
> diff --git a/vl.c b/vl.c
> index a1686ef..6b09220 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3319,7 +3319,7 @@ int main(int argc, char **argv, char **envp)
>                      sz = qemu_opt_get_size(opts, "maxmem", 0);
>                      if (sz < ram_size) {
>                          fprintf(stderr, "qemu: invalid -m option value: maxmem "
> -                                "(%" PRIu64 ") <= initial memory ("
> +                                "(%" PRIu64 ") <= initial memory (%"
>                                  RAM_ADDR_FMT ")\n", sz, ram_size);
>                          exit(EXIT_FAILURE);
>                      }
> @@ -3327,7 +3327,7 @@ int main(int argc, char **argv, char **envp)
>                      slots = qemu_opt_get_number(opts, "slots", 0);
>                      if ((sz > ram_size) && !slots) {
>                          fprintf(stderr, "qemu: invalid -m option value: maxmem "
> -                                "(%" PRIu64 ") more than initial memory ("
> +                                "(%" PRIu64 ") more than initial memory (%"
>                                  RAM_ADDR_FMT ") but no hotplug slots where "
>                                  "specified\n", sz, ram_size);
>                          exit(EXIT_FAILURE);
> @@ -3336,7 +3336,7 @@ int main(int argc, char **argv, char **envp)
>                      if ((sz <= ram_size) && slots) {
>                          fprintf(stderr, "qemu: invalid -m option value:  %"
>                                  PRIu64 " hotplug slots where specified but "
> -                                "maxmem (%" PRIu64 ") <= initial memory ("
> +                                "maxmem (%" PRIu64 ") <= initial memory (%"
>                                  RAM_ADDR_FMT ")\n", slots, sz, ram_size);
>                          exit(EXIT_FAILURE);
>                      }
> diff --git a/xen-hvm.c b/xen-hvm.c
> index bafdf12..e7b4b76 100644
> --- a/xen-hvm.c
> +++ b/xen-hvm.c
> @@ -221,8 +221,8 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr)
>  
>      if (runstate_check(RUN_STATE_INMIGRATE)) {
>          /* RAM already populated in Xen */
> -        fprintf(stderr, "%s: do not alloc "RAM_ADDR_FMT
> -                " bytes of ram at "RAM_ADDR_FMT" when runstate is INMIGRATE\n",
> +        fprintf(stderr, "%s: do not alloc %"RAM_ADDR_FMT" bytes"
> +                " of ram at %"RAM_ADDR_FMTX" when runstate is INMIGRATE\n",
>                  __func__, size, ram_addr); 
>          return;
>      }
> @@ -241,7 +241,7 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr)
>      }
>  
>      if (xc_domain_populate_physmap_exact(xen_xc, xen_domid, nr_pfn, 0, 0, pfn_list)) {
> -        hw_error("xen: failed to populate ram at " RAM_ADDR_FMT, ram_addr);
> +        hw_error("xen: failed to populate ram at %" RAM_ADDR_FMTX, ram_addr);
>      }
>  
>      g_free(pfn_list);
> @@ -1127,9 +1127,8 @@ void xen_modified_memory(ram_addr_t start, ram_addr_t length)
>              - start_pfn;
>          rc = xc_hvm_modified_memory(xen_xc, xen_domid, start_pfn, nb_pages);
>          if (rc) {
> -            fprintf(stderr,
> -                    "%s failed for "RAM_ADDR_FMT" ("RAM_ADDR_FMT"): %i, %s\n",
> -                    __func__, start, nb_pages, rc, strerror(-rc));
> +            fprintf(stderr, "%s failed for %"RAM_ADDR_FMTX" (%"RAM_ADDR_FMT"):"
> +                    " %i, %s\n", __func__, start, nb_pages, rc, strerror(-rc));
>          }
>      }
>  }
> -- 
> 1.7.1

Re: [Qemu-devel] [PATCH 1/4] switch RAM_ADDR_FMT format specifier to print decimal

[Igor Mammedov]

On Wed, 25 Jun 2014 14:51:15 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jun 25, 2014 at 01:42:20PM +0200, Igor Mammedov wrote:
> > ... and add RAM_ADDR_FMTX for hex format. In addition fix
> > places to print sizes/lengths as decimal and addresses as hex.
> 
> ugh.
> maybe both - though I really think most people will find
> it easier to parse he hex variant.
> 
> > Also drop "%" from RAM_ADDR_FMT macro and use it like other
> > PRIfoo format placeholders specifying "%" explicitly.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  arch_init.c               |   14 +++++++-------
> >  exec.c                    |    4 ++--
> >  hw/i386/pc.c              |    4 ++--
> >  hw/ppc/ppc405_boards.c    |    2 +-
> >  include/exec/cpu-common.h |    6 ++++--
> >  numa.c                    |    2 +-
> >  vl.c                      |    6 +++---
> >  xen-hvm.c                 |   11 +++++------
> >  8 files changed, 25 insertions(+), 24 deletions(-)
> > 
> > diff --git a/arch_init.c b/arch_init.c
> > index 8ddaf35..7562325 100644
> > --- a/arch_init.c
> > +++ b/arch_init.c
> > @@ -1072,8 +1072,8 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
> >                  QTAILQ_FOREACH(block, &ram_list.blocks, next) {
> >                      if (!strncmp(id, block->idstr, sizeof(id))) {
> >                          if (block->length != length) {
> > -                            error_report("Length mismatch: %s: " RAM_ADDR_FMT
> > -                                         " in != " RAM_ADDR_FMT, id, length,
> > +                            error_report("Length mismatch: %s: %" RAM_ADDR_FMT
> > +                                         " in != %" RAM_ADDR_FMT, id, length,
> >                                           block->length);
> 
> Not sure I like this.
> length values are often powers of two.
> Parsing them in decimal is hard.
Problem is that when user specifies on CLI size in decimal and then sees error
message where it's hex or mix of decimal and/or hex values.

But I don't care much about it, we can drop this patch

> 
> >                              ret =  -EINVAL;
> >                          }
> > @@ -1098,7 +1098,7 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
> >  
> >              host = host_from_stream_offset(f, addr, flags);
> >              if (!host) {
> > -                error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
> > +                error_report("Illegal RAM offset %" RAM_ADDR_FMTX, addr);
> >                  ret = -EINVAL;
> >                  break;
> >              }
> > @@ -1110,7 +1110,7 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
> >  
> >              host = host_from_stream_offset(f, addr, flags);
> >              if (!host) {
> > -                error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
> > +                error_report("Illegal RAM offset %" RAM_ADDR_FMTX, addr);
> >                  ret = -EINVAL;
> >                  break;
> >              }
> > @@ -1119,14 +1119,14 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
> >          } else if (flags & RAM_SAVE_FLAG_XBZRLE) {
> >              void *host = host_from_stream_offset(f, addr, flags);
> >              if (!host) {
> > -                error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
> > +                error_report("Illegal RAM offset %" RAM_ADDR_FMTX, addr);
> >                  ret = -EINVAL;
> >                  break;
> >              }
> >  
> >              if (load_xbzrle(f, addr, host) < 0) {
> > -                error_report("Failed to decompress XBZRLE page at "
> > -                             RAM_ADDR_FMT, addr);
> > +                error_report("Failed to decompress XBZRLE page at %"
> > +                             RAM_ADDR_FMTX, addr);
> >                  ret = -EINVAL;
> >                  break;
> >              }
> > diff --git a/exec.c b/exec.c
> > index c849405..f9ae8a4 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -1435,8 +1435,8 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length)
> >                                  flags, -1, 0);
> >                  }
> >                  if (area != vaddr) {
> > -                    fprintf(stderr, "Could not remap addr: "
> > -                            RAM_ADDR_FMT "@" RAM_ADDR_FMT "\n",
> > +                    fprintf(stderr, "Could not remap addr: %"
> > +                            RAM_ADDR_FMTX "@%" RAM_ADDR_FMTX "\n",
> >                              length, addr);
> >                      exit(1);
> >                  }
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 2cf22b1..a6e7ef3 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1257,8 +1257,8 @@ FWCfgState *pc_memory_init(MachineState *machine,
> >  
> >          if ((pcms->hotplug_memory_base + hotplug_mem_size) <
> >              hotplug_mem_size) {
> > -            error_report("unsupported amount of maximum memory: " RAM_ADDR_FMT,
> > -                         machine->maxram_size);
> > +            error_report("unsupported amount of maximum memory: %"
> > +                         RAM_ADDR_FMT, machine->maxram_size);
> >              exit(EXIT_FAILURE);
> >          }
> >  
> > diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
> > index 98ad2d7..2b1b4fe 100644
> > --- a/hw/ppc/ppc405_boards.c
> > +++ b/hw/ppc/ppc405_boards.c
> > @@ -358,7 +358,7 @@ static void ref405ep_init(MachineState *machine)
> >          bdloc = 0;
> >      }
> >  #ifdef DEBUG_BOARD_INIT
> > -    printf("bdloc " RAM_ADDR_FMT "\n", bdloc);
> > +    printf("bdloc %" RAM_ADDR_FMTX "\n", bdloc);
> >      printf("%s: Done\n", __func__);
> >  #endif
> >  }
> > diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> > index e3ec4c8..6899d0c 100644
> > --- a/include/exec/cpu-common.h
> > +++ b/include/exec/cpu-common.h
> > @@ -38,11 +38,13 @@ enum device_endian {
> >  #if defined(CONFIG_XEN_BACKEND)
> >  typedef uint64_t ram_addr_t;
> >  #  define RAM_ADDR_MAX UINT64_MAX
> > -#  define RAM_ADDR_FMT "%" PRIx64
> > +#  define RAM_ADDR_FMTX PRIx64
> > +#  define RAM_ADDR_FMT PRIu64
> >  #else
> >  typedef uintptr_t ram_addr_t;
> >  #  define RAM_ADDR_MAX UINTPTR_MAX
> > -#  define RAM_ADDR_FMT "%" PRIxPTR
> > +#  define RAM_ADDR_FMTX PRIxPTR
> > +#  define RAM_ADDR_FMT PRIuPTR
> >  #endif
> >  
> >  extern ram_addr_t ram_size;
> > diff --git a/numa.c b/numa.c
> > index e471afe..d9f2655 100644
> > --- a/numa.c
> > +++ b/numa.c
> > @@ -189,7 +189,7 @@ void set_numa_nodes(void)
> >          }
> >          if (numa_total != ram_size) {
> >              error_report("total memory for NUMA nodes (%" PRIu64 ")"
> > -                         " should equal RAM size (" RAM_ADDR_FMT ")",
> > +                         " should equal RAM size (%" RAM_ADDR_FMT ")",
> >                           numa_total, ram_size);
> >              exit(1);
> >          }
> > diff --git a/vl.c b/vl.c
> > index a1686ef..6b09220 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -3319,7 +3319,7 @@ int main(int argc, char **argv, char **envp)
> >                      sz = qemu_opt_get_size(opts, "maxmem", 0);
> >                      if (sz < ram_size) {
> >                          fprintf(stderr, "qemu: invalid -m option value: maxmem "
> > -                                "(%" PRIu64 ") <= initial memory ("
> > +                                "(%" PRIu64 ") <= initial memory (%"
> >                                  RAM_ADDR_FMT ")\n", sz, ram_size);
> >                          exit(EXIT_FAILURE);
> >                      }
> > @@ -3327,7 +3327,7 @@ int main(int argc, char **argv, char **envp)
> >                      slots = qemu_opt_get_number(opts, "slots", 0);
> >                      if ((sz > ram_size) && !slots) {
> >                          fprintf(stderr, "qemu: invalid -m option value: maxmem "
> > -                                "(%" PRIu64 ") more than initial memory ("
> > +                                "(%" PRIu64 ") more than initial memory (%"
> >                                  RAM_ADDR_FMT ") but no hotplug slots where "
> >                                  "specified\n", sz, ram_size);
> >                          exit(EXIT_FAILURE);
> > @@ -3336,7 +3336,7 @@ int main(int argc, char **argv, char **envp)
> >                      if ((sz <= ram_size) && slots) {
> >                          fprintf(stderr, "qemu: invalid -m option value:  %"
> >                                  PRIu64 " hotplug slots where specified but "
> > -                                "maxmem (%" PRIu64 ") <= initial memory ("
> > +                                "maxmem (%" PRIu64 ") <= initial memory (%"
> >                                  RAM_ADDR_FMT ")\n", slots, sz, ram_size);
> >                          exit(EXIT_FAILURE);
> >                      }
> > diff --git a/xen-hvm.c b/xen-hvm.c
> > index bafdf12..e7b4b76 100644
> > --- a/xen-hvm.c
> > +++ b/xen-hvm.c
> > @@ -221,8 +221,8 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr)
> >  
> >      if (runstate_check(RUN_STATE_INMIGRATE)) {
> >          /* RAM already populated in Xen */
> > -        fprintf(stderr, "%s: do not alloc "RAM_ADDR_FMT
> > -                " bytes of ram at "RAM_ADDR_FMT" when runstate is INMIGRATE\n",
> > +        fprintf(stderr, "%s: do not alloc %"RAM_ADDR_FMT" bytes"
> > +                " of ram at %"RAM_ADDR_FMTX" when runstate is INMIGRATE\n",
> >                  __func__, size, ram_addr); 
> >          return;
> >      }
> > @@ -241,7 +241,7 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr)
> >      }
> >  
> >      if (xc_domain_populate_physmap_exact(xen_xc, xen_domid, nr_pfn, 0, 0, pfn_list)) {
> > -        hw_error("xen: failed to populate ram at " RAM_ADDR_FMT, ram_addr);
> > +        hw_error("xen: failed to populate ram at %" RAM_ADDR_FMTX, ram_addr);
> >      }
> >  
> >      g_free(pfn_list);
> > @@ -1127,9 +1127,8 @@ void xen_modified_memory(ram_addr_t start, ram_addr_t length)
> >              - start_pfn;
> >          rc = xc_hvm_modified_memory(xen_xc, xen_domid, start_pfn, nb_pages);
> >          if (rc) {
> > -            fprintf(stderr,
> > -                    "%s failed for "RAM_ADDR_FMT" ("RAM_ADDR_FMT"): %i, %s\n",
> > -                    __func__, start, nb_pages, rc, strerror(-rc));
> > +            fprintf(stderr, "%s failed for %"RAM_ADDR_FMTX" (%"RAM_ADDR_FMT"):"
> > +                    " %i, %s\n", __func__, start, nb_pages, rc, strerror(-rc));
> >          }
> >      }
> >  }
> > -- 
> > 1.7.1

Re: [Qemu-devel] [PATCH 1/4] switch RAM_ADDR_FMT format specifier to print decimal

[Michael S. Tsirkin]

On Wed, Jun 25, 2014 at 02:14:51PM +0200, Igor Mammedov wrote:
> On Wed, 25 Jun 2014 14:51:15 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Jun 25, 2014 at 01:42:20PM +0200, Igor Mammedov wrote:
> > > ... and add RAM_ADDR_FMTX for hex format. In addition fix
> > > places to print sizes/lengths as decimal and addresses as hex.
> > 
> > ugh.
> > maybe both - though I really think most people will find
> > it easier to parse he hex variant.
> > 
> > > Also drop "%" from RAM_ADDR_FMT macro and use it like other
> > > PRIfoo format placeholders specifying "%" explicitly.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  arch_init.c               |   14 +++++++-------
> > >  exec.c                    |    4 ++--
> > >  hw/i386/pc.c              |    4 ++--
> > >  hw/ppc/ppc405_boards.c    |    2 +-
> > >  include/exec/cpu-common.h |    6 ++++--
> > >  numa.c                    |    2 +-
> > >  vl.c                      |    6 +++---
> > >  xen-hvm.c                 |   11 +++++------
> > >  8 files changed, 25 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/arch_init.c b/arch_init.c
> > > index 8ddaf35..7562325 100644
> > > --- a/arch_init.c
> > > +++ b/arch_init.c
> > > @@ -1072,8 +1072,8 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
> > >                  QTAILQ_FOREACH(block, &ram_list.blocks, next) {
> > >                      if (!strncmp(id, block->idstr, sizeof(id))) {
> > >                          if (block->length != length) {
> > > -                            error_report("Length mismatch: %s: " RAM_ADDR_FMT
> > > -                                         " in != " RAM_ADDR_FMT, id, length,
> > > +                            error_report("Length mismatch: %s: %" RAM_ADDR_FMT
> > > +                                         " in != %" RAM_ADDR_FMT, id, length,
> > >                                           block->length);
> > 
> > Not sure I like this.
> > length values are often powers of two.
> > Parsing them in decimal is hard.
> Problem is that when user specifies on CLI size in decimal and then sees error
> message where it's hex or mix of decimal and/or hex values.
> 
> But I don't care much about it, we can drop this patch

User likely does something like 5G.
Seeing size in decimal bytes is unlikely to be helpful.

> > 
> > >                              ret =  -EINVAL;
> > >                          }
> > > @@ -1098,7 +1098,7 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
> > >  
> > >              host = host_from_stream_offset(f, addr, flags);
> > >              if (!host) {
> > > -                error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
> > > +                error_report("Illegal RAM offset %" RAM_ADDR_FMTX, addr);
> > >                  ret = -EINVAL;
> > >                  break;
> > >              }
> > > @@ -1110,7 +1110,7 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
> > >  
> > >              host = host_from_stream_offset(f, addr, flags);
> > >              if (!host) {
> > > -                error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
> > > +                error_report("Illegal RAM offset %" RAM_ADDR_FMTX, addr);
> > >                  ret = -EINVAL;
> > >                  break;
> > >              }
> > > @@ -1119,14 +1119,14 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
> > >          } else if (flags & RAM_SAVE_FLAG_XBZRLE) {
> > >              void *host = host_from_stream_offset(f, addr, flags);
> > >              if (!host) {
> > > -                error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
> > > +                error_report("Illegal RAM offset %" RAM_ADDR_FMTX, addr);
> > >                  ret = -EINVAL;
> > >                  break;
> > >              }
> > >  
> > >              if (load_xbzrle(f, addr, host) < 0) {
> > > -                error_report("Failed to decompress XBZRLE page at "
> > > -                             RAM_ADDR_FMT, addr);
> > > +                error_report("Failed to decompress XBZRLE page at %"
> > > +                             RAM_ADDR_FMTX, addr);
> > >                  ret = -EINVAL;
> > >                  break;
> > >              }
> > > diff --git a/exec.c b/exec.c
> > > index c849405..f9ae8a4 100644
> > > --- a/exec.c
> > > +++ b/exec.c
> > > @@ -1435,8 +1435,8 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length)
> > >                                  flags, -1, 0);
> > >                  }
> > >                  if (area != vaddr) {
> > > -                    fprintf(stderr, "Could not remap addr: "
> > > -                            RAM_ADDR_FMT "@" RAM_ADDR_FMT "\n",
> > > +                    fprintf(stderr, "Could not remap addr: %"
> > > +                            RAM_ADDR_FMTX "@%" RAM_ADDR_FMTX "\n",
> > >                              length, addr);
> > >                      exit(1);
> > >                  }
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index 2cf22b1..a6e7ef3 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -1257,8 +1257,8 @@ FWCfgState *pc_memory_init(MachineState *machine,
> > >  
> > >          if ((pcms->hotplug_memory_base + hotplug_mem_size) <
> > >              hotplug_mem_size) {
> > > -            error_report("unsupported amount of maximum memory: " RAM_ADDR_FMT,
> > > -                         machine->maxram_size);
> > > +            error_report("unsupported amount of maximum memory: %"
> > > +                         RAM_ADDR_FMT, machine->maxram_size);
> > >              exit(EXIT_FAILURE);
> > >          }
> > >  
> > > diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
> > > index 98ad2d7..2b1b4fe 100644
> > > --- a/hw/ppc/ppc405_boards.c
> > > +++ b/hw/ppc/ppc405_boards.c
> > > @@ -358,7 +358,7 @@ static void ref405ep_init(MachineState *machine)
> > >          bdloc = 0;
> > >      }
> > >  #ifdef DEBUG_BOARD_INIT
> > > -    printf("bdloc " RAM_ADDR_FMT "\n", bdloc);
> > > +    printf("bdloc %" RAM_ADDR_FMTX "\n", bdloc);
> > >      printf("%s: Done\n", __func__);
> > >  #endif
> > >  }
> > > diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> > > index e3ec4c8..6899d0c 100644
> > > --- a/include/exec/cpu-common.h
> > > +++ b/include/exec/cpu-common.h
> > > @@ -38,11 +38,13 @@ enum device_endian {
> > >  #if defined(CONFIG_XEN_BACKEND)
> > >  typedef uint64_t ram_addr_t;
> > >  #  define RAM_ADDR_MAX UINT64_MAX
> > > -#  define RAM_ADDR_FMT "%" PRIx64
> > > +#  define RAM_ADDR_FMTX PRIx64
> > > +#  define RAM_ADDR_FMT PRIu64
> > >  #else
> > >  typedef uintptr_t ram_addr_t;
> > >  #  define RAM_ADDR_MAX UINTPTR_MAX
> > > -#  define RAM_ADDR_FMT "%" PRIxPTR
> > > +#  define RAM_ADDR_FMTX PRIxPTR
> > > +#  define RAM_ADDR_FMT PRIuPTR
> > >  #endif
> > >  
> > >  extern ram_addr_t ram_size;
> > > diff --git a/numa.c b/numa.c
> > > index e471afe..d9f2655 100644
> > > --- a/numa.c
> > > +++ b/numa.c
> > > @@ -189,7 +189,7 @@ void set_numa_nodes(void)
> > >          }
> > >          if (numa_total != ram_size) {
> > >              error_report("total memory for NUMA nodes (%" PRIu64 ")"
> > > -                         " should equal RAM size (" RAM_ADDR_FMT ")",
> > > +                         " should equal RAM size (%" RAM_ADDR_FMT ")",
> > >                           numa_total, ram_size);
> > >              exit(1);
> > >          }
> > > diff --git a/vl.c b/vl.c
> > > index a1686ef..6b09220 100644
> > > --- a/vl.c
> > > +++ b/vl.c
> > > @@ -3319,7 +3319,7 @@ int main(int argc, char **argv, char **envp)
> > >                      sz = qemu_opt_get_size(opts, "maxmem", 0);
> > >                      if (sz < ram_size) {
> > >                          fprintf(stderr, "qemu: invalid -m option value: maxmem "
> > > -                                "(%" PRIu64 ") <= initial memory ("
> > > +                                "(%" PRIu64 ") <= initial memory (%"
> > >                                  RAM_ADDR_FMT ")\n", sz, ram_size);
> > >                          exit(EXIT_FAILURE);
> > >                      }
> > > @@ -3327,7 +3327,7 @@ int main(int argc, char **argv, char **envp)
> > >                      slots = qemu_opt_get_number(opts, "slots", 0);
> > >                      if ((sz > ram_size) && !slots) {
> > >                          fprintf(stderr, "qemu: invalid -m option value: maxmem "
> > > -                                "(%" PRIu64 ") more than initial memory ("
> > > +                                "(%" PRIu64 ") more than initial memory (%"
> > >                                  RAM_ADDR_FMT ") but no hotplug slots where "
> > >                                  "specified\n", sz, ram_size);
> > >                          exit(EXIT_FAILURE);
> > > @@ -3336,7 +3336,7 @@ int main(int argc, char **argv, char **envp)
> > >                      if ((sz <= ram_size) && slots) {
> > >                          fprintf(stderr, "qemu: invalid -m option value:  %"
> > >                                  PRIu64 " hotplug slots where specified but "
> > > -                                "maxmem (%" PRIu64 ") <= initial memory ("
> > > +                                "maxmem (%" PRIu64 ") <= initial memory (%"
> > >                                  RAM_ADDR_FMT ")\n", slots, sz, ram_size);
> > >                          exit(EXIT_FAILURE);
> > >                      }
> > > diff --git a/xen-hvm.c b/xen-hvm.c
> > > index bafdf12..e7b4b76 100644
> > > --- a/xen-hvm.c
> > > +++ b/xen-hvm.c
> > > @@ -221,8 +221,8 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr)
> > >  
> > >      if (runstate_check(RUN_STATE_INMIGRATE)) {
> > >          /* RAM already populated in Xen */
> > > -        fprintf(stderr, "%s: do not alloc "RAM_ADDR_FMT
> > > -                " bytes of ram at "RAM_ADDR_FMT" when runstate is INMIGRATE\n",
> > > +        fprintf(stderr, "%s: do not alloc %"RAM_ADDR_FMT" bytes"
> > > +                " of ram at %"RAM_ADDR_FMTX" when runstate is INMIGRATE\n",
> > >                  __func__, size, ram_addr); 
> > >          return;
> > >      }
> > > @@ -241,7 +241,7 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr)
> > >      }
> > >  
> > >      if (xc_domain_populate_physmap_exact(xen_xc, xen_domid, nr_pfn, 0, 0, pfn_list)) {
> > > -        hw_error("xen: failed to populate ram at " RAM_ADDR_FMT, ram_addr);
> > > +        hw_error("xen: failed to populate ram at %" RAM_ADDR_FMTX, ram_addr);
> > >      }
> > >  
> > >      g_free(pfn_list);
> > > @@ -1127,9 +1127,8 @@ void xen_modified_memory(ram_addr_t start, ram_addr_t length)
> > >              - start_pfn;
> > >          rc = xc_hvm_modified_memory(xen_xc, xen_domid, start_pfn, nb_pages);
> > >          if (rc) {
> > > -            fprintf(stderr,
> > > -                    "%s failed for "RAM_ADDR_FMT" ("RAM_ADDR_FMT"): %i, %s\n",
> > > -                    __func__, start, nb_pages, rc, strerror(-rc));
> > > +            fprintf(stderr, "%s failed for %"RAM_ADDR_FMTX" (%"RAM_ADDR_FMT"):"
> > > +                    " %i, %s\n", __func__, start, nb_pages, rc, strerror(-rc));
> > >          }
> > >      }
> > >  }
> > > -- 
> > > 1.7.1

Re: [Qemu-devel] [PATCH 1/4] switch RAM_ADDR_FMT format specifier to print decimal

[Peter Maydell]

On 25 June 2014 12:42, Igor Mammedov <imammedo@redhat.com> wrote
> ... and add RAM_ADDR_FMTX for hex format. In addition fix
> places to print sizes/lengths as decimal and addresses as hex.

Please can you make the 'body' part of your commit messages
stand on its own? It's kind of hard to understand when half of the
sentence is off wherever the mail client displays its Subject line
and only half is in the email body.

> Also drop "%" from RAM_ADDR_FMT macro and use it like other
> PRIfoo format placeholders specifying "%" explicitly.

[If you find yourself writing "And... Also..." in a commit message
it's often a sign that you probably want to split it into more than
one patch.]

If you're going to do this you should just create a set of
RAMADDR_PRI[diouxX] like we have for HWADDR, rather
than leaving the non-standard RAM_ADDR_FMT name.

Please don't change the default meaning of RAM_ADDR_FMT
in the same commit as making a lot of boilerplate no-effect
changes to move % characters around; if there are cases that
would be better done as decimal then those behavioural
changes should be done in their own patches so it's clear
which things have been changed.

thanks
-- PMM

[Qemu-devel] [PATCH 2/4] vl.c: use single local_err throughout main()

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 vl.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/vl.c b/vl.c
index 6b09220..0a39c93 100644
--- a/vl.c
+++ b/vl.c
@@ -2927,6 +2927,7 @@ int main(int argc, char **argv, char **envp)
     };
     const char *trace_events = NULL;
     const char *trace_file = NULL;
+    Error *local_err = NULL;
     const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
                                         1024 * 1024;
     ram_addr_t maxram_size = default_ram_size;
@@ -4220,7 +4221,6 @@ int main(int argc, char **argv, char **envp)
     configure_accelerator(machine_class);
 
     if (qtest_chrdev) {
-        Error *local_err = NULL;
         qtest_init(qtest_chrdev, qtest_log, &local_err);
         if (local_err) {
             error_report("%s", error_get_pretty(local_err));
@@ -4448,7 +4448,6 @@ int main(int argc, char **argv, char **envp)
 #ifdef CONFIG_VNC
     /* init remote displays */
     if (vnc_display) {
-        Error *local_err = NULL;
         vnc_display_init(ds);
         vnc_display_open(ds, vnc_display, &local_err);
         if (local_err != NULL) {
@@ -4503,7 +4502,6 @@ int main(int argc, char **argv, char **envp)
     }
 
     if (incoming) {
-        Error *local_err = NULL;
         qemu_start_incoming_migration(incoming, &local_err);
         if (local_err) {
             error_report("-incoming %s: %s", incoming,
-- 
1.7.1

Re: [Qemu-devel] [PATCH 2/4] vl.c: use single local_err throughout main()

[Michael S. Tsirkin]

On Wed, Jun 25, 2014 at 01:42:21PM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Fixes any bugs?

> ---
>  vl.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index 6b09220..0a39c93 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2927,6 +2927,7 @@ int main(int argc, char **argv, char **envp)
>      };
>      const char *trace_events = NULL;
>      const char *trace_file = NULL;
> +    Error *local_err = NULL;
>      const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
>                                          1024 * 1024;
>      ram_addr_t maxram_size = default_ram_size;
> @@ -4220,7 +4221,6 @@ int main(int argc, char **argv, char **envp)
>      configure_accelerator(machine_class);
>  
>      if (qtest_chrdev) {
> -        Error *local_err = NULL;
>          qtest_init(qtest_chrdev, qtest_log, &local_err);
>          if (local_err) {
>              error_report("%s", error_get_pretty(local_err));
> @@ -4448,7 +4448,6 @@ int main(int argc, char **argv, char **envp)
>  #ifdef CONFIG_VNC
>      /* init remote displays */
>      if (vnc_display) {
> -        Error *local_err = NULL;
>          vnc_display_init(ds);
>          vnc_display_open(ds, vnc_display, &local_err);
>          if (local_err != NULL) {
> @@ -4503,7 +4502,6 @@ int main(int argc, char **argv, char **envp)
>      }
>  
>      if (incoming) {
> -        Error *local_err = NULL;
>          qemu_start_incoming_migration(incoming, &local_err);
>          if (local_err) {
>              error_report("-incoming %s: %s", incoming,
> -- 
> 1.7.1

Re: [Qemu-devel] [PATCH 2/4] vl.c: use single local_err throughout main()

[Igor Mammedov]

On Wed, 25 Jun 2014 14:51:52 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jun 25, 2014 at 01:42:21PM +0200, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> Fixes any bugs?
Nope, it's to reduce code duplication, which would be increased
in following patch.

> 
> > ---
> >  vl.c |    4 +---
> >  1 files changed, 1 insertions(+), 3 deletions(-)
> > 
> > diff --git a/vl.c b/vl.c
> > index 6b09220..0a39c93 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -2927,6 +2927,7 @@ int main(int argc, char **argv, char **envp)
> >      };
> >      const char *trace_events = NULL;
> >      const char *trace_file = NULL;
> > +    Error *local_err = NULL;
> >      const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
> >                                          1024 * 1024;
> >      ram_addr_t maxram_size = default_ram_size;
> > @@ -4220,7 +4221,6 @@ int main(int argc, char **argv, char **envp)
> >      configure_accelerator(machine_class);
> >  
> >      if (qtest_chrdev) {
> > -        Error *local_err = NULL;
> >          qtest_init(qtest_chrdev, qtest_log, &local_err);
> >          if (local_err) {
> >              error_report("%s", error_get_pretty(local_err));
> > @@ -4448,7 +4448,6 @@ int main(int argc, char **argv, char **envp)
> >  #ifdef CONFIG_VNC
> >      /* init remote displays */
> >      if (vnc_display) {
> > -        Error *local_err = NULL;
> >          vnc_display_init(ds);
> >          vnc_display_open(ds, vnc_display, &local_err);
> >          if (local_err != NULL) {
> > @@ -4503,7 +4502,6 @@ int main(int argc, char **argv, char **envp)
> >      }
> >  
> >      if (incoming) {
> > -        Error *local_err = NULL;
> >          qemu_start_incoming_migration(incoming, &local_err);
> >          if (local_err) {
> >              error_report("-incoming %s: %s", incoming,
> > -- 
> > 1.7.1
> 

Re: [Qemu-devel] [PATCH 2/4] vl.c: use single local_err throughout main()

[Michael S. Tsirkin]

On Wed, Jun 25, 2014 at 02:16:20PM +0200, Igor Mammedov wrote:
> On Wed, 25 Jun 2014 14:51:52 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Jun 25, 2014 at 01:42:21PM +0200, Igor Mammedov wrote:
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > 
> > Fixes any bugs?
> Nope, it's to reduce code duplication, which would be increased
> in following patch.

Guys we need to get 2.1 out of the door.
So hold off on new non bugfix patches until after 2.2, or if you feel
you want early feedback, tag them as RFC.

This is just adding to a pile of mail I have to
wade through and discard, and you will have to repost
or ping me again after the release anyway.

> > 
> > > ---
> > >  vl.c |    4 +---
> > >  1 files changed, 1 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/vl.c b/vl.c
> > > index 6b09220..0a39c93 100644
> > > --- a/vl.c
> > > +++ b/vl.c
> > > @@ -2927,6 +2927,7 @@ int main(int argc, char **argv, char **envp)
> > >      };
> > >      const char *trace_events = NULL;
> > >      const char *trace_file = NULL;
> > > +    Error *local_err = NULL;
> > >      const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
> > >                                          1024 * 1024;
> > >      ram_addr_t maxram_size = default_ram_size;
> > > @@ -4220,7 +4221,6 @@ int main(int argc, char **argv, char **envp)
> > >      configure_accelerator(machine_class);
> > >  
> > >      if (qtest_chrdev) {
> > > -        Error *local_err = NULL;
> > >          qtest_init(qtest_chrdev, qtest_log, &local_err);
> > >          if (local_err) {
> > >              error_report("%s", error_get_pretty(local_err));
> > > @@ -4448,7 +4448,6 @@ int main(int argc, char **argv, char **envp)
> > >  #ifdef CONFIG_VNC
> > >      /* init remote displays */
> > >      if (vnc_display) {
> > > -        Error *local_err = NULL;
> > >          vnc_display_init(ds);
> > >          vnc_display_open(ds, vnc_display, &local_err);
> > >          if (local_err != NULL) {
> > > @@ -4503,7 +4502,6 @@ int main(int argc, char **argv, char **envp)
> > >      }
> > >  
> > >      if (incoming) {
> > > -        Error *local_err = NULL;
> > >          qemu_start_incoming_migration(incoming, &local_err);
> > >          if (local_err) {
> > >              error_report("-incoming %s: %s", incoming,
> > > -- 
> > > 1.7.1
> > 

[Qemu-devel] [PATCH 3/4] machine: convert ram_size, maxram_size, ram_slots to properties

[Igor Mammedov]

... short term it will help writing unit tests accessing
these values via QMP. And down the road it will allow to drop
global variable ram_size.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/core/machine.c   |  181 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/boards.h |    5 ++
 vl.c                |  123 +++++++++++++++-------------------
 3 files changed, 240 insertions(+), 69 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index cbba679..f73b810 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -12,6 +12,10 @@
 
 #include "hw/boards.h"
 #include "qapi/visitor.h"
+#include "qemu/error-report.h"
+#include "hw/xen/xen.h"
+
+static const ram_addr_t default_ram_size  = 128 * 1024 * 1024;
 
 static char *machine_get_accel(Object *obj, Error **errp)
 {
@@ -235,8 +239,118 @@ static void machine_set_firmware(Object *obj, const char *value, Error **errp)
     ms->firmware = g_strdup(value);
 }
 
+static void machine_get_ram_size(Object *obj, Visitor *v,
+                                 void *opaque, const char *name,
+                                 Error **errp)
+{
+    MachineState *ms = MACHINE(obj);
+    int64_t value = ms->ram_size;
+
+    visit_type_int(v, &value, name, errp);
+}
+
+static void machine_set_ram_size(Object *obj, Visitor *v,
+                                 void *opaque, const char *name,
+                                 Error **errp)
+{
+    MachineState *ms = MACHINE(obj);
+    Error *error = NULL;
+    int64_t value;
+
+    visit_type_int(v, &value, name, &error);
+    if (error) {
+        goto out;
+    }
+
+    value = QEMU_ALIGN_UP(value, 8192);
+    ms->ram_size = value;
+    if (ms->ram_size != value) {
+        error_setg(&error, "ram size too large");
+        goto out;
+    }
+
+    if (!ms->ram_size) {
+        error_setg(&error, "ram size can't be 0");
+    }
+out:
+    if (error) {
+        error_propagate(errp, error);
+    }
+}
+
+static void machine_get_maxram_size(Object *obj, Visitor *v,
+                                    void *opaque, const char *name,
+                                    Error **errp)
+{
+    MachineState *ms = MACHINE(obj);
+    int64_t value = ms->maxram_size;
+
+    visit_type_int(v, &value, name, errp);
+}
+
+static void machine_set_maxram_size(Object *obj, Visitor *v,
+                                    void *opaque, const char *name,
+                                    Error **errp)
+{
+    MachineState *ms = MACHINE(obj);
+    Error *error = NULL;
+    int64_t value;
+
+    visit_type_int(v, &value, name, &error);
+    if (error) {
+        goto out;
+    }
+
+    ms->maxram_size = value;
+    if (ms->maxram_size != value) {
+        error_setg(&error, "maxmem is too large");
+        goto out;
+    }
+
+    if (!ms->maxram_size) {
+        error_setg(&error, "maxmem can't be 0");
+    }
+out:
+    if (error) {
+        error_propagate(errp, error);
+    }
+}
+
+static void machine_get_ram_slots(Object *obj, Visitor *v,
+                                  void *opaque, const char *name,
+                                  Error **errp)
+{
+    MachineState *ms = MACHINE(obj);
+    int64_t value = ms->ram_slots;
+
+    visit_type_int(v, &value, name, errp);
+}
+
+static void machine_set_ram_slots(Object *obj, Visitor *v,
+                                  void *opaque, const char *name,
+                                  Error **errp)
+{
+    MachineState *ms = MACHINE(obj);
+    Error *error = NULL;
+    int64_t value;
+
+    visit_type_int(v, &value, name, &error);
+    if (error) {
+        goto out;
+    }
+
+    ms->ram_slots = value;
+
+out:
+    if (error) {
+        error_propagate(errp, error);
+    }
+}
+
 static void machine_initfn(Object *obj)
 {
+    MachineState *ms = MACHINE(obj);
+
     object_property_add_str(obj, "accel",
                             machine_get_accel, machine_set_accel, NULL);
     object_property_add_bool(obj, "kernel_irqchip",
@@ -274,6 +388,20 @@ static void machine_initfn(Object *obj)
     object_property_add_bool(obj, "usb", machine_get_usb, machine_set_usb, NULL);
     object_property_add_str(obj, "firmware",
                             machine_get_firmware, machine_set_firmware, NULL);
+
+    ms->ram_size = default_ram_size;
+    object_property_add(obj, MACHINE_MEMORY_SIZE_OPT, "int",
+                        machine_get_ram_size,
+                        machine_set_ram_size,
+                        NULL, NULL, NULL);
+    object_property_add(obj, MACHINE_MAXMEMORY_SIZE_OPT, "int",
+                        machine_get_maxram_size,
+                        machine_set_maxram_size,
+                        NULL, NULL, NULL);
+    object_property_add(obj, MACHINE_MEMORY_SLOTS_OPT, "int",
+                        machine_get_ram_slots,
+                        machine_set_ram_slots,
+                        NULL, NULL, NULL);
 }
 
 static void machine_finalize(Object *obj)
@@ -290,11 +418,64 @@ static void machine_finalize(Object *obj)
     g_free(ms->firmware);
 }
 
+static void machine_pre_init(MachineState *ms, Error **errp)
+{
+    Error *error = NULL;
+
+    if ((ms->ram_size > ms->maxram_size) && ms->maxram_size) {
+        error_setg(&error, "invalid \"maxmem\" value (%" RAM_ADDR_FMT
+                   ") < initial memory (%" RAM_ADDR_FMT ")",
+                   ms->maxram_size, ms->ram_size);
+        goto out;
+    }
+
+    if ((ms->ram_size < ms->maxram_size) && !ms->ram_slots) {
+        error_setg(&error, "invalid \"maxmem\" value (%" RAM_ADDR_FMT ") "
+                   "more than initial memory (%" PRIu64 ") but "
+                   "no hotplug slots where specified",
+                   ms->maxram_size, ms->ram_size);
+        goto out;
+    }
+
+    if ((ms->ram_size == ms->maxram_size) && ms->ram_slots) {
+        error_setg(&error, "invalid \"maxmem\" value (%" RAM_ADDR_FMT ") "
+                   "it must be more than initial memory if hotplug slots > 0",
+                   ms->maxram_size);
+        goto out;
+    }
+
+    if (!ms->maxram_size && !ms->ram_slots) {
+        ms->maxram_size = ms->ram_size;
+    }
+
+    if (!xen_enabled()) {
+        /* On 32-bit hosts, QEMU is limited by virtual address space */
+        if (HOST_LONG_BITS == 32 && ((ms->ram_size > (2047 << 20)) ||
+                                     (ms->maxram_size > (2047 << 20)))) {
+            error_setg(&error, "at most 2047 MB RAM can be simulated\n");
+            goto out;
+        }
+    }
+
+out:
+    if (error) {
+        error_propagate(errp, error);
+    }
+}
+
+static void machine_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+
+    mc->instance_pre_init = machine_pre_init;
+}
+
 static const TypeInfo machine_info = {
     .name = TYPE_MACHINE,
     .parent = TYPE_OBJECT,
     .abstract = true,
     .class_size = sizeof(MachineClass),
+    .class_init = machine_class_init,
     .instance_size = sizeof(MachineState),
     .instance_init = machine_initfn,
     .instance_finalize = machine_finalize,
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 605a970..1b980c5 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -81,6 +81,7 @@ struct MachineClass {
     const char *desc;
 
     void (*init)(MachineState *state);
+    void (*instance_pre_init)(MachineState *state, Error **errp);
     void (*reset)(void);
     void (*hot_add_cpu)(const int64_t id, Error **errp);
     int (*kvm_type)(const char *arg);
@@ -134,4 +135,8 @@ struct MachineState {
     const char *cpu_model;
 };
 
+#define MACHINE_MEMORY_SIZE_OPT    "memory-size"
+#define MACHINE_MEMORY_SLOTS_OPT   "memory-slots"
+#define MACHINE_MAXMEMORY_SIZE_OPT "maxmem"
+
 #endif
diff --git a/vl.c b/vl.c
index 0a39c93..3ed3582 100644
--- a/vl.c
+++ b/vl.c
@@ -119,8 +119,6 @@ int main(int argc, char **argv)
 #include "qom/object_interfaces.h"
 #include "qapi-event.h"
 
-#define DEFAULT_RAM_SIZE 128
-
 #define MAX_VIRTIO_CONSOLES 1
 #define MAX_SCLP_CONSOLES 1
 
@@ -2928,10 +2926,8 @@ int main(int argc, char **argv, char **envp)
     const char *trace_events = NULL;
     const char *trace_file = NULL;
     Error *local_err = NULL;
-    const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
-                                        1024 * 1024;
-    ram_addr_t maxram_size = default_ram_size;
-    uint64_t ram_slots = 0;
+    const char *maxram_size_str = NULL;
+    const char *ram_slots_str = NULL;
     FILE *vmstate_dump_file = NULL;
 
     atexit(qemu_run_exit_notifiers);
@@ -2978,7 +2974,7 @@ int main(int argc, char **argv, char **envp)
     module_call_init(MODULE_INIT_MACHINE);
     machine_class = find_default_machine();
     cpu_model = NULL;
-    ram_size = default_ram_size;
+    ram_size = 0;
     snapshot = 0;
     cyls = heads = secs = 0;
     translation = BIOS_ATA_TRANSLATION_AUTO;
@@ -3269,7 +3265,6 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_m: {
                 uint64_t sz;
                 const char *mem_str;
-                const char *maxmem_str, *slots_str;
 
                 opts = qemu_opts_parse(qemu_find_opts("memory"),
                                        optarg, 1);
@@ -3287,7 +3282,7 @@ int main(int argc, char **argv, char **envp)
                     exit(EXIT_FAILURE);
                 }
 
-                sz = qemu_opt_get_size(opts, "size", ram_size);
+                sz = qemu_opt_get_size(opts, "size", 0);
 
                 /* Fix up legacy suffix-less format */
                 if (g_ascii_isdigit(mem_str[strlen(mem_str) - 1])) {
@@ -3301,54 +3296,12 @@ int main(int argc, char **argv, char **envp)
                 }
 
                 /* backward compatibility behaviour for case "-m 0" */
-                if (sz == 0) {
-                    sz = default_ram_size;
-                }
-
-                sz = QEMU_ALIGN_UP(sz, 8192);
-                ram_size = sz;
-                if (ram_size != sz) {
-                    error_report("ram size too large");
-                    exit(EXIT_FAILURE);
+                if (sz != 0) {
+                    ram_size = sz;
                 }
 
-                maxmem_str = qemu_opt_get(opts, "maxmem");
-                slots_str = qemu_opt_get(opts, "slots");
-                if (maxmem_str && slots_str) {
-                    uint64_t slots;
-
-                    sz = qemu_opt_get_size(opts, "maxmem", 0);
-                    if (sz < ram_size) {
-                        fprintf(stderr, "qemu: invalid -m option value: maxmem "
-                                "(%" PRIu64 ") <= initial memory (%"
-                                RAM_ADDR_FMT ")\n", sz, ram_size);
-                        exit(EXIT_FAILURE);
-                    }
-
-                    slots = qemu_opt_get_number(opts, "slots", 0);
-                    if ((sz > ram_size) && !slots) {
-                        fprintf(stderr, "qemu: invalid -m option value: maxmem "
-                                "(%" PRIu64 ") more than initial memory (%"
-                                RAM_ADDR_FMT ") but no hotplug slots where "
-                                "specified\n", sz, ram_size);
-                        exit(EXIT_FAILURE);
-                    }
-
-                    if ((sz <= ram_size) && slots) {
-                        fprintf(stderr, "qemu: invalid -m option value:  %"
-                                PRIu64 " hotplug slots where specified but "
-                                "maxmem (%" PRIu64 ") <= initial memory (%"
-                                RAM_ADDR_FMT ")\n", slots, sz, ram_size);
-                        exit(EXIT_FAILURE);
-                    }
-                    maxram_size = sz;
-                    ram_slots = slots;
-                } else if ((!maxmem_str && slots_str) ||
-                           (maxmem_str && !slots_str)) {
-                    fprintf(stderr, "qemu: invalid -m option value: missing "
-                            "'%s' option\n", slots_str ? "maxmem" : "slots");
-                    exit(EXIT_FAILURE);
-                }
+                maxram_size_str = qemu_opt_get(opts, "maxmem");
+                ram_slots_str = qemu_opt_get(opts, "slots");
                 break;
             }
 #ifdef CONFIG_TPM
@@ -4203,14 +4156,48 @@ int main(int argc, char **argv, char **envp)
         exit(1);
     }
 
-    /* store value for the future use */
-    qemu_opt_set_number(qemu_find_opts_singleton("memory"), "size", ram_size);
-
     if (qemu_opts_foreach(qemu_find_opts("device"), device_help_func, NULL, 0)
         != 0) {
         exit(0);
     }
 
+    if (ram_size) {
+        object_property_set_int(OBJECT(current_machine), ram_size,
+                                MACHINE_MEMORY_SIZE_OPT, &local_err);
+        if (local_err) {
+            error_report("%s", error_get_pretty(local_err));
+            error_free(local_err);
+            exit(EXIT_FAILURE);
+        }
+    }
+
+    if (maxram_size_str) {
+        uint64_t sz = qemu_opt_get_size(qemu_find_opts_singleton("memory"),
+                                        "maxmem", 0);
+
+        parse_option_size("maxmem", maxram_size_str, &sz, &local_err);
+        if (local_err) {
+            error_report("%s", error_get_pretty(local_err));
+            error_free(local_err);
+            exit(EXIT_FAILURE);
+        }
+        object_property_set_int(OBJECT(current_machine), sz,
+                                MACHINE_MAXMEMORY_SIZE_OPT, &local_err);
+        if (local_err) {
+            error_report("%s", error_get_pretty(local_err));
+            error_free(local_err);
+            exit(EXIT_FAILURE);
+        }
+    }
+
+    if (ram_slots_str) {
+        if (object_set_property(MACHINE_MEMORY_SLOTS_OPT, ram_slots_str,
+                                current_machine)) {
+            exit(EXIT_FAILURE);
+        }
+    }
+
+
     machine_opts = qemu_get_machine_opts();
     if (qemu_opt_foreach(machine_opts, object_set_property, current_machine,
                          1) < 0) {
@@ -4229,6 +4216,9 @@ int main(int argc, char **argv, char **envp)
         }
     }
 
+    ram_size = object_property_get_int(OBJECT(current_machine),
+                                       MACHINE_MEMORY_SIZE_OPT,
+                                       &error_abort);
     machine_opts = qemu_get_machine_opts();
     kernel_filename = qemu_opt_get(machine_opts, "kernel");
     initrd_filename = qemu_opt_get(machine_opts, "initrd");
@@ -4314,14 +4304,6 @@ int main(int argc, char **argv, char **envp)
     if (foreach_device_config(DEV_BT, bt_parse))
         exit(1);
 
-    if (!xen_enabled()) {
-        /* On 32-bit hosts, QEMU is limited by virtual address space */
-        if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) {
-            fprintf(stderr, "qemu: at most 2047 MB RAM can be simulated\n");
-            exit(1);
-        }
-    }
-
     blk_mig_init();
     ram_mig_init();
 
@@ -4386,12 +4368,15 @@ int main(int argc, char **argv, char **envp)
 
     qdev_machine_init();
 
-    current_machine->ram_size = ram_size;
-    current_machine->maxram_size = maxram_size;
-    current_machine->ram_slots = ram_slots;
     current_machine->boot_order = boot_order;
     current_machine->cpu_model = cpu_model;
 
+    machine_class->instance_pre_init(current_machine, &local_err);
+    if (local_err != NULL) {
+        error_report("%s", error_get_pretty(local_err));
+        error_free(local_err);
+        exit(EXIT_FAILURE);
+    }
     machine_class->init(current_machine);
 
     audio_init();
-- 
1.7.1

Re: [Qemu-devel] [PATCH 3/4] machine: convert ram_size, maxram_size, ram_slots to properties

[Michael S. Tsirkin]

On Wed, Jun 25, 2014 at 01:42:22PM +0200, Igor Mammedov wrote:
> ... short term it will help writing unit tests accessing
> these values via QMP. And down the road it will allow to drop
> global variable ram_size.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/core/machine.c   |  181 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/boards.h |    5 ++
>  vl.c                |  123 +++++++++++++++-------------------
>  3 files changed, 240 insertions(+), 69 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index cbba679..f73b810 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -12,6 +12,10 @@
>  
>  #include "hw/boards.h"
>  #include "qapi/visitor.h"
> +#include "qemu/error-report.h"
> +#include "hw/xen/xen.h"
> +
> +static const ram_addr_t default_ram_size  = 128 * 1024 * 1024;
>  
>  static char *machine_get_accel(Object *obj, Error **errp)
>  {
> @@ -235,8 +239,118 @@ static void machine_set_firmware(Object *obj, const char *value, Error **errp)
>      ms->firmware = g_strdup(value);
>  }
>  
> +static void machine_get_ram_size(Object *obj, Visitor *v,
> +                                 void *opaque, const char *name,
> +                                 Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +    int64_t value = ms->ram_size;
> +
> +    visit_type_int(v, &value, name, errp);
> +}
> +
> +static void machine_set_ram_size(Object *obj, Visitor *v,
> +                                 void *opaque, const char *name,
> +                                 Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +    Error *error = NULL;
> +    int64_t value;
> +
> +    visit_type_int(v, &value, name, &error);
> +    if (error) {
> +        goto out;
> +    }
> +
> +    value = QEMU_ALIGN_UP(value, 8192);
> +    ms->ram_size = value;
> +    if (ms->ram_size != value) {
> +        error_setg(&error, "ram size too large");
> +        goto out;
> +    }
> +
> +    if (!ms->ram_size) {
> +        error_setg(&error, "ram size can't be 0");
> +    }
> +out:
> +    if (error) {
> +        error_propagate(errp, error);
> +    }
> +}
> +
> +static void machine_get_maxram_size(Object *obj, Visitor *v,
> +                                    void *opaque, const char *name,
> +                                    Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +    int64_t value = ms->maxram_size;
> +
> +    visit_type_int(v, &value, name, errp);
> +}
> +
> +static void machine_set_maxram_size(Object *obj, Visitor *v,
> +                                    void *opaque, const char *name,
> +                                    Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +    Error *error = NULL;
> +    int64_t value;
> +
> +    visit_type_int(v, &value, name, &error);
> +    if (error) {
> +        goto out;
> +    }
> +
> +    ms->maxram_size = value;
> +    if (ms->maxram_size != value) {
> +        error_setg(&error, "maxmem is too large");
> +        goto out;
> +    }
> +
> +    if (!ms->maxram_size) {
> +        error_setg(&error, "maxmem can't be 0");
> +    }
> +out:
> +    if (error) {
> +        error_propagate(errp, error);
> +    }
> +}
> +
> +static void machine_get_ram_slots(Object *obj, Visitor *v,
> +                                  void *opaque, const char *name,
> +                                  Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +    int64_t value = ms->ram_slots;
> +
> +    visit_type_int(v, &value, name, errp);
> +}
> +
> +static void machine_set_ram_slots(Object *obj, Visitor *v,
> +                                  void *opaque, const char *name,
> +                                  Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +    Error *error = NULL;
> +    int64_t value;
> +
> +    visit_type_int(v, &value, name, &error);
> +    if (error) {
> +        goto out;
> +    }
> +
> +    ms->ram_slots = value;
> +
> +out:
> +    if (error) {
> +        error_propagate(errp, error);
> +    }
> +}
>

Really that's too much boiler plate code for
each value.  Can't we support a "validate" callback from
standard integer values, to be called after parsing?


>  static void machine_initfn(Object *obj)
>  {
> +    MachineState *ms = MACHINE(obj);
> +
>      object_property_add_str(obj, "accel",
>                              machine_get_accel, machine_set_accel, NULL);
>      object_property_add_bool(obj, "kernel_irqchip",
> @@ -274,6 +388,20 @@ static void machine_initfn(Object *obj)
>      object_property_add_bool(obj, "usb", machine_get_usb, machine_set_usb, NULL);
>      object_property_add_str(obj, "firmware",
>                              machine_get_firmware, machine_set_firmware, NULL);
> +
> +    ms->ram_size = default_ram_size;
> +    object_property_add(obj, MACHINE_MEMORY_SIZE_OPT, "int",
> +                        machine_get_ram_size,
> +                        machine_set_ram_size,
> +                        NULL, NULL, NULL);
> +    object_property_add(obj, MACHINE_MAXMEMORY_SIZE_OPT, "int",
> +                        machine_get_maxram_size,
> +                        machine_set_maxram_size,
> +                        NULL, NULL, NULL);
> +    object_property_add(obj, MACHINE_MEMORY_SLOTS_OPT, "int",
> +                        machine_get_ram_slots,
> +                        machine_set_ram_slots,
> +                        NULL, NULL, NULL);
>  }
>  
>  static void machine_finalize(Object *obj)
> @@ -290,11 +418,64 @@ static void machine_finalize(Object *obj)
>      g_free(ms->firmware);
>  }
>  
> +static void machine_pre_init(MachineState *ms, Error **errp)
> +{
> +    Error *error = NULL;
> +
> +    if ((ms->ram_size > ms->maxram_size) && ms->maxram_size) {
> +        error_setg(&error, "invalid \"maxmem\" value (%" RAM_ADDR_FMT
> +                   ") < initial memory (%" RAM_ADDR_FMT ")",
> +                   ms->maxram_size, ms->ram_size);
> +        goto out;
> +    }
> +
> +    if ((ms->ram_size < ms->maxram_size) && !ms->ram_slots) {
> +        error_setg(&error, "invalid \"maxmem\" value (%" RAM_ADDR_FMT ") "
> +                   "more than initial memory (%" PRIu64 ") but "
> +                   "no hotplug slots where specified",
> +                   ms->maxram_size, ms->ram_size);
> +        goto out;
> +    }
> +
> +    if ((ms->ram_size == ms->maxram_size) && ms->ram_slots) {

ugly () within conditions, not really needed.

> +        error_setg(&error, "invalid \"maxmem\" value (%" RAM_ADDR_FMT ") "
> +                   "it must be more than initial memory if hotplug slots > 0",
> +                   ms->maxram_size);
> +        goto out;
> +    }
> +
> +    if (!ms->maxram_size && !ms->ram_slots) {
> +        ms->maxram_size = ms->ram_size;
> +    }
> +
> +    if (!xen_enabled()) {
> +        /* On 32-bit hosts, QEMU is limited by virtual address space */
> +        if (HOST_LONG_BITS == 32 && ((ms->ram_size > (2047 << 20)) ||
> +                                     (ms->maxram_size > (2047 << 20)))) {
> +            error_setg(&error, "at most 2047 MB RAM can be simulated\n");
> +            goto out;
> +        }
> +    }
> +
> +out:
> +    if (error) {
> +        error_propagate(errp, error);
> +    }
> +}
> +
> +static void machine_class_init(ObjectClass *oc, void *data)
> +{
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +
> +    mc->instance_pre_init = machine_pre_init;
> +}
> +
>  static const TypeInfo machine_info = {
>      .name = TYPE_MACHINE,
>      .parent = TYPE_OBJECT,
>      .abstract = true,
>      .class_size = sizeof(MachineClass),
> +    .class_init = machine_class_init,
>      .instance_size = sizeof(MachineState),
>      .instance_init = machine_initfn,
>      .instance_finalize = machine_finalize,
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 605a970..1b980c5 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -81,6 +81,7 @@ struct MachineClass {
>      const char *desc;
>  
>      void (*init)(MachineState *state);
> +    void (*instance_pre_init)(MachineState *state, Error **errp);
>      void (*reset)(void);
>      void (*hot_add_cpu)(const int64_t id, Error **errp);
>      int (*kvm_type)(const char *arg);
> @@ -134,4 +135,8 @@ struct MachineState {
>      const char *cpu_model;
>  };
>  
> +#define MACHINE_MEMORY_SIZE_OPT    "memory-size"
> +#define MACHINE_MEMORY_SLOTS_OPT   "memory-slots"
> +#define MACHINE_MAXMEMORY_SIZE_OPT "maxmem"
> +
>  #endif
> diff --git a/vl.c b/vl.c
> index 0a39c93..3ed3582 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -119,8 +119,6 @@ int main(int argc, char **argv)
>  #include "qom/object_interfaces.h"
>  #include "qapi-event.h"
>  
> -#define DEFAULT_RAM_SIZE 128
> -
>  #define MAX_VIRTIO_CONSOLES 1
>  #define MAX_SCLP_CONSOLES 1
>  
> @@ -2928,10 +2926,8 @@ int main(int argc, char **argv, char **envp)
>      const char *trace_events = NULL;
>      const char *trace_file = NULL;
>      Error *local_err = NULL;
> -    const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
> -                                        1024 * 1024;
> -    ram_addr_t maxram_size = default_ram_size;
> -    uint64_t ram_slots = 0;
> +    const char *maxram_size_str = NULL;
> +    const char *ram_slots_str = NULL;
>      FILE *vmstate_dump_file = NULL;
>  
>      atexit(qemu_run_exit_notifiers);
> @@ -2978,7 +2974,7 @@ int main(int argc, char **argv, char **envp)
>      module_call_init(MODULE_INIT_MACHINE);
>      machine_class = find_default_machine();
>      cpu_model = NULL;
> -    ram_size = default_ram_size;
> +    ram_size = 0;
>      snapshot = 0;
>      cyls = heads = secs = 0;
>      translation = BIOS_ATA_TRANSLATION_AUTO;
> @@ -3269,7 +3265,6 @@ int main(int argc, char **argv, char **envp)
>              case QEMU_OPTION_m: {
>                  uint64_t sz;
>                  const char *mem_str;
> -                const char *maxmem_str, *slots_str;
>  
>                  opts = qemu_opts_parse(qemu_find_opts("memory"),
>                                         optarg, 1);
> @@ -3287,7 +3282,7 @@ int main(int argc, char **argv, char **envp)
>                      exit(EXIT_FAILURE);
>                  }
>  
> -                sz = qemu_opt_get_size(opts, "size", ram_size);
> +                sz = qemu_opt_get_size(opts, "size", 0);
>  
>                  /* Fix up legacy suffix-less format */
>                  if (g_ascii_isdigit(mem_str[strlen(mem_str) - 1])) {
> @@ -3301,54 +3296,12 @@ int main(int argc, char **argv, char **envp)
>                  }
>  
>                  /* backward compatibility behaviour for case "-m 0" */
> -                if (sz == 0) {
> -                    sz = default_ram_size;
> -                }
> -
> -                sz = QEMU_ALIGN_UP(sz, 8192);
> -                ram_size = sz;
> -                if (ram_size != sz) {
> -                    error_report("ram size too large");
> -                    exit(EXIT_FAILURE);
> +                if (sz != 0) {
> +                    ram_size = sz;
>                  }
>  
> -                maxmem_str = qemu_opt_get(opts, "maxmem");
> -                slots_str = qemu_opt_get(opts, "slots");
> -                if (maxmem_str && slots_str) {
> -                    uint64_t slots;
> -
> -                    sz = qemu_opt_get_size(opts, "maxmem", 0);
> -                    if (sz < ram_size) {
> -                        fprintf(stderr, "qemu: invalid -m option value: maxmem "
> -                                "(%" PRIu64 ") <= initial memory (%"
> -                                RAM_ADDR_FMT ")\n", sz, ram_size);
> -                        exit(EXIT_FAILURE);
> -                    }
> -
> -                    slots = qemu_opt_get_number(opts, "slots", 0);
> -                    if ((sz > ram_size) && !slots) {
> -                        fprintf(stderr, "qemu: invalid -m option value: maxmem "
> -                                "(%" PRIu64 ") more than initial memory (%"
> -                                RAM_ADDR_FMT ") but no hotplug slots where "
> -                                "specified\n", sz, ram_size);
> -                        exit(EXIT_FAILURE);
> -                    }
> -
> -                    if ((sz <= ram_size) && slots) {
> -                        fprintf(stderr, "qemu: invalid -m option value:  %"
> -                                PRIu64 " hotplug slots where specified but "
> -                                "maxmem (%" PRIu64 ") <= initial memory (%"
> -                                RAM_ADDR_FMT ")\n", slots, sz, ram_size);
> -                        exit(EXIT_FAILURE);
> -                    }
> -                    maxram_size = sz;
> -                    ram_slots = slots;
> -                } else if ((!maxmem_str && slots_str) ||
> -                           (maxmem_str && !slots_str)) {
> -                    fprintf(stderr, "qemu: invalid -m option value: missing "
> -                            "'%s' option\n", slots_str ? "maxmem" : "slots");
> -                    exit(EXIT_FAILURE);
> -                }
> +                maxram_size_str = qemu_opt_get(opts, "maxmem");
> +                ram_slots_str = qemu_opt_get(opts, "slots");
>                  break;
>              }
>  #ifdef CONFIG_TPM
> @@ -4203,14 +4156,48 @@ int main(int argc, char **argv, char **envp)
>          exit(1);
>      }
>  
> -    /* store value for the future use */
> -    qemu_opt_set_number(qemu_find_opts_singleton("memory"), "size", ram_size);
> -
>      if (qemu_opts_foreach(qemu_find_opts("device"), device_help_func, NULL, 0)
>          != 0) {
>          exit(0);
>      }
>  
> +    if (ram_size) {
> +        object_property_set_int(OBJECT(current_machine), ram_size,
> +                                MACHINE_MEMORY_SIZE_OPT, &local_err);
> +        if (local_err) {
> +            error_report("%s", error_get_pretty(local_err));
> +            error_free(local_err);
> +            exit(EXIT_FAILURE);
> +        }
> +    }
> +
> +    if (maxram_size_str) {
> +        uint64_t sz = qemu_opt_get_size(qemu_find_opts_singleton("memory"),
> +                                        "maxmem", 0);
> +
> +        parse_option_size("maxmem", maxram_size_str, &sz, &local_err);
> +        if (local_err) {
> +            error_report("%s", error_get_pretty(local_err));
> +            error_free(local_err);
> +            exit(EXIT_FAILURE);
> +        }
> +        object_property_set_int(OBJECT(current_machine), sz,
> +                                MACHINE_MAXMEMORY_SIZE_OPT, &local_err);
> +        if (local_err) {
> +            error_report("%s", error_get_pretty(local_err));
> +            error_free(local_err);
> +            exit(EXIT_FAILURE);
> +        }
> +    }
> +
> +    if (ram_slots_str) {
> +        if (object_set_property(MACHINE_MEMORY_SLOTS_OPT, ram_slots_str,
> +                                current_machine)) {
> +            exit(EXIT_FAILURE);
> +        }
> +    }
> +
> +
>      machine_opts = qemu_get_machine_opts();
>      if (qemu_opt_foreach(machine_opts, object_set_property, current_machine,
>                           1) < 0) {
> @@ -4229,6 +4216,9 @@ int main(int argc, char **argv, char **envp)
>          }
>      }
>  
> +    ram_size = object_property_get_int(OBJECT(current_machine),
> +                                       MACHINE_MEMORY_SIZE_OPT,
> +                                       &error_abort);
>      machine_opts = qemu_get_machine_opts();
>      kernel_filename = qemu_opt_get(machine_opts, "kernel");
>      initrd_filename = qemu_opt_get(machine_opts, "initrd");
> @@ -4314,14 +4304,6 @@ int main(int argc, char **argv, char **envp)
>      if (foreach_device_config(DEV_BT, bt_parse))
>          exit(1);
>  
> -    if (!xen_enabled()) {
> -        /* On 32-bit hosts, QEMU is limited by virtual address space */
> -        if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) {
> -            fprintf(stderr, "qemu: at most 2047 MB RAM can be simulated\n");
> -            exit(1);
> -        }
> -    }
> -
>      blk_mig_init();
>      ram_mig_init();
>  
> @@ -4386,12 +4368,15 @@ int main(int argc, char **argv, char **envp)
>  
>      qdev_machine_init();
>  
> -    current_machine->ram_size = ram_size;
> -    current_machine->maxram_size = maxram_size;
> -    current_machine->ram_slots = ram_slots;
>      current_machine->boot_order = boot_order;
>      current_machine->cpu_model = cpu_model;
>  
> +    machine_class->instance_pre_init(current_machine, &local_err);
> +    if (local_err != NULL) {
> +        error_report("%s", error_get_pretty(local_err));
> +        error_free(local_err);
> +        exit(EXIT_FAILURE);
> +    }
>      machine_class->init(current_machine);
>  
>      audio_init();
> -- 
> 1.7.1

Re: [Qemu-devel] [PATCH 3/4] machine: convert ram_size, maxram_size, ram_slots to properties

[Igor Mammedov]

On Wed, 25 Jun 2014 14:54:47 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jun 25, 2014 at 01:42:22PM +0200, Igor Mammedov wrote:
> > ... short term it will help writing unit tests accessing
> > these values via QMP. And down the road it will allow to drop
> > global variable ram_size.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/core/machine.c   |  181 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/boards.h |    5 ++
> >  vl.c                |  123 +++++++++++++++-------------------
> >  3 files changed, 240 insertions(+), 69 deletions(-)
> > 
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index cbba679..f73b810 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -12,6 +12,10 @@
> >  
> >  #include "hw/boards.h"
> >  #include "qapi/visitor.h"
> > +#include "qemu/error-report.h"
> > +#include "hw/xen/xen.h"
> > +
> > +static const ram_addr_t default_ram_size  = 128 * 1024 * 1024;
> >  
> >  static char *machine_get_accel(Object *obj, Error **errp)
> >  {
> > @@ -235,8 +239,118 @@ static void machine_set_firmware(Object *obj, const char *value, Error **errp)
> >      ms->firmware = g_strdup(value);
> >  }
> >  
> > +static void machine_get_ram_size(Object *obj, Visitor *v,
> > +                                 void *opaque, const char *name,
> > +                                 Error **errp)
> > +{
> > +    MachineState *ms = MACHINE(obj);
> > +    int64_t value = ms->ram_size;
> > +
> > +    visit_type_int(v, &value, name, errp);
> > +}
> > +
> > +static void machine_set_ram_size(Object *obj, Visitor *v,
> > +                                 void *opaque, const char *name,
> > +                                 Error **errp)
> > +{
> > +    MachineState *ms = MACHINE(obj);
> > +    Error *error = NULL;
> > +    int64_t value;
> > +
> > +    visit_type_int(v, &value, name, &error);
> > +    if (error) {
> > +        goto out;
> > +    }
> > +
> > +    value = QEMU_ALIGN_UP(value, 8192);
> > +    ms->ram_size = value;
> > +    if (ms->ram_size != value) {
> > +        error_setg(&error, "ram size too large");
> > +        goto out;
> > +    }
> > +
> > +    if (!ms->ram_size) {
> > +        error_setg(&error, "ram size can't be 0");
> > +    }
> > +out:
> > +    if (error) {
> > +        error_propagate(errp, error);
> > +    }
> > +}
> > +
> > +static void machine_get_maxram_size(Object *obj, Visitor *v,
> > +                                    void *opaque, const char *name,
> > +                                    Error **errp)
> > +{
> > +    MachineState *ms = MACHINE(obj);
> > +    int64_t value = ms->maxram_size;
> > +
> > +    visit_type_int(v, &value, name, errp);
> > +}
> > +
> > +static void machine_set_maxram_size(Object *obj, Visitor *v,
> > +                                    void *opaque, const char *name,
> > +                                    Error **errp)
> > +{
> > +    MachineState *ms = MACHINE(obj);
> > +    Error *error = NULL;
> > +    int64_t value;
> > +
> > +    visit_type_int(v, &value, name, &error);
> > +    if (error) {
> > +        goto out;
> > +    }
> > +
> > +    ms->maxram_size = value;
> > +    if (ms->maxram_size != value) {
> > +        error_setg(&error, "maxmem is too large");
> > +        goto out;
> > +    }
> > +
> > +    if (!ms->maxram_size) {
> > +        error_setg(&error, "maxmem can't be 0");
> > +    }
> > +out:
> > +    if (error) {
> > +        error_propagate(errp, error);
> > +    }
> > +}
> > +
> > +static void machine_get_ram_slots(Object *obj, Visitor *v,
> > +                                  void *opaque, const char *name,
> > +                                  Error **errp)
> > +{
> > +    MachineState *ms = MACHINE(obj);
> > +    int64_t value = ms->ram_slots;
> > +
> > +    visit_type_int(v, &value, name, errp);
> > +}
> > +
> > +static void machine_set_ram_slots(Object *obj, Visitor *v,
> > +                                  void *opaque, const char *name,
> > +                                  Error **errp)
> > +{
> > +    MachineState *ms = MACHINE(obj);
> > +    Error *error = NULL;
> > +    int64_t value;
> > +
> > +    visit_type_int(v, &value, name, &error);
> > +    if (error) {
> > +        goto out;
> > +    }
> > +
> > +    ms->ram_slots = value;
> > +
> > +out:
> > +    if (error) {
> > +        error_propagate(errp, error);
> > +    }
> > +}
> >
> 
> Really that's too much boiler plate code for
> each value.  Can't we support a "validate" callback from
> standard integer values, to be called after parsing?
machine_set_ram_slots() could be reduced to:

+static void machine_set_ram_slots(Object *obj, Visitor *v,
+                                  void *opaque, const char *name,
+                                  Error **errp)
+{
+    MachineState *ms = MACHINE(obj);
+
+    visit_type_int(v, &value, name, errp);
+}

maxram_size/ram_size setters could be reduced ~11 LOC moving common
parts to a helper.

But generic property validator is probably out of scope for this
series and it won't help much in this case.

> 
> 
> >  static void machine_initfn(Object *obj)
> >  {
> > +    MachineState *ms = MACHINE(obj);
> > +
> >      object_property_add_str(obj, "accel",
> >                              machine_get_accel, machine_set_accel, NULL);
> >      object_property_add_bool(obj, "kernel_irqchip",
> > @@ -274,6 +388,20 @@ static void machine_initfn(Object *obj)
> >      object_property_add_bool(obj, "usb", machine_get_usb, machine_set_usb, NULL);
> >      object_property_add_str(obj, "firmware",
> >                              machine_get_firmware, machine_set_firmware, NULL);
> > +
> > +    ms->ram_size = default_ram_size;
> > +    object_property_add(obj, MACHINE_MEMORY_SIZE_OPT, "int",
> > +                        machine_get_ram_size,
> > +                        machine_set_ram_size,
> > +                        NULL, NULL, NULL);
> > +    object_property_add(obj, MACHINE_MAXMEMORY_SIZE_OPT, "int",
> > +                        machine_get_maxram_size,
> > +                        machine_set_maxram_size,
> > +                        NULL, NULL, NULL);
> > +    object_property_add(obj, MACHINE_MEMORY_SLOTS_OPT, "int",
> > +                        machine_get_ram_slots,
> > +                        machine_set_ram_slots,
> > +                        NULL, NULL, NULL);
> >  }
> >  
> >  static void machine_finalize(Object *obj)
> > @@ -290,11 +418,64 @@ static void machine_finalize(Object *obj)
> >      g_free(ms->firmware);
> >  }
> >  
> > +static void machine_pre_init(MachineState *ms, Error **errp)
> > +{
> > +    Error *error = NULL;
> > +
> > +    if ((ms->ram_size > ms->maxram_size) && ms->maxram_size) {
> > +        error_setg(&error, "invalid \"maxmem\" value (%" RAM_ADDR_FMT
> > +                   ") < initial memory (%" RAM_ADDR_FMT ")",
> > +                   ms->maxram_size, ms->ram_size);
> > +        goto out;
> > +    }
> > +
> > +    if ((ms->ram_size < ms->maxram_size) && !ms->ram_slots) {
> > +        error_setg(&error, "invalid \"maxmem\" value (%" RAM_ADDR_FMT ") "
> > +                   "more than initial memory (%" PRIu64 ") but "
> > +                   "no hotplug slots where specified",
> > +                   ms->maxram_size, ms->ram_size);
> > +        goto out;
> > +    }
> > +
> > +    if ((ms->ram_size == ms->maxram_size) && ms->ram_slots) {
> 
> ugly () within conditions, not really needed.
> 
> > +        error_setg(&error, "invalid \"maxmem\" value (%" RAM_ADDR_FMT ") "
> > +                   "it must be more than initial memory if hotplug slots > 0",
> > +                   ms->maxram_size);
> > +        goto out;
> > +    }
> > +
> > +    if (!ms->maxram_size && !ms->ram_slots) {
> > +        ms->maxram_size = ms->ram_size;
> > +    }
> > +
> > +    if (!xen_enabled()) {
> > +        /* On 32-bit hosts, QEMU is limited by virtual address space */
> > +        if (HOST_LONG_BITS == 32 && ((ms->ram_size > (2047 << 20)) ||
> > +                                     (ms->maxram_size > (2047 << 20)))) {
> > +            error_setg(&error, "at most 2047 MB RAM can be simulated\n");
> > +            goto out;
> > +        }
> > +    }
> > +
> > +out:
> > +    if (error) {
> > +        error_propagate(errp, error);
> > +    }
> > +}
> > +
> > +static void machine_class_init(ObjectClass *oc, void *data)
> > +{
> > +    MachineClass *mc = MACHINE_CLASS(oc);
> > +
> > +    mc->instance_pre_init = machine_pre_init;
> > +}
> > +
> >  static const TypeInfo machine_info = {
> >      .name = TYPE_MACHINE,
> >      .parent = TYPE_OBJECT,
> >      .abstract = true,
> >      .class_size = sizeof(MachineClass),
> > +    .class_init = machine_class_init,
> >      .instance_size = sizeof(MachineState),
> >      .instance_init = machine_initfn,
> >      .instance_finalize = machine_finalize,
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 605a970..1b980c5 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -81,6 +81,7 @@ struct MachineClass {
> >      const char *desc;
> >  
> >      void (*init)(MachineState *state);
> > +    void (*instance_pre_init)(MachineState *state, Error **errp);
> >      void (*reset)(void);
> >      void (*hot_add_cpu)(const int64_t id, Error **errp);
> >      int (*kvm_type)(const char *arg);
> > @@ -134,4 +135,8 @@ struct MachineState {
> >      const char *cpu_model;
> >  };
> >  
> > +#define MACHINE_MEMORY_SIZE_OPT    "memory-size"
> > +#define MACHINE_MEMORY_SLOTS_OPT   "memory-slots"
> > +#define MACHINE_MAXMEMORY_SIZE_OPT "maxmem"
> > +
> >  #endif
> > diff --git a/vl.c b/vl.c
> > index 0a39c93..3ed3582 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -119,8 +119,6 @@ int main(int argc, char **argv)
> >  #include "qom/object_interfaces.h"
> >  #include "qapi-event.h"
> >  
> > -#define DEFAULT_RAM_SIZE 128
> > -
> >  #define MAX_VIRTIO_CONSOLES 1
> >  #define MAX_SCLP_CONSOLES 1
> >  
> > @@ -2928,10 +2926,8 @@ int main(int argc, char **argv, char **envp)
> >      const char *trace_events = NULL;
> >      const char *trace_file = NULL;
> >      Error *local_err = NULL;
> > -    const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
> > -                                        1024 * 1024;
> > -    ram_addr_t maxram_size = default_ram_size;
> > -    uint64_t ram_slots = 0;
> > +    const char *maxram_size_str = NULL;
> > +    const char *ram_slots_str = NULL;
> >      FILE *vmstate_dump_file = NULL;
> >  
> >      atexit(qemu_run_exit_notifiers);
> > @@ -2978,7 +2974,7 @@ int main(int argc, char **argv, char **envp)
> >      module_call_init(MODULE_INIT_MACHINE);
> >      machine_class = find_default_machine();
> >      cpu_model = NULL;
> > -    ram_size = default_ram_size;
> > +    ram_size = 0;
> >      snapshot = 0;
> >      cyls = heads = secs = 0;
> >      translation = BIOS_ATA_TRANSLATION_AUTO;
> > @@ -3269,7 +3265,6 @@ int main(int argc, char **argv, char **envp)
> >              case QEMU_OPTION_m: {
> >                  uint64_t sz;
> >                  const char *mem_str;
> > -                const char *maxmem_str, *slots_str;
> >  
> >                  opts = qemu_opts_parse(qemu_find_opts("memory"),
> >                                         optarg, 1);
> > @@ -3287,7 +3282,7 @@ int main(int argc, char **argv, char **envp)
> >                      exit(EXIT_FAILURE);
> >                  }
> >  
> > -                sz = qemu_opt_get_size(opts, "size", ram_size);
> > +                sz = qemu_opt_get_size(opts, "size", 0);
> >  
> >                  /* Fix up legacy suffix-less format */
> >                  if (g_ascii_isdigit(mem_str[strlen(mem_str) - 1])) {
> > @@ -3301,54 +3296,12 @@ int main(int argc, char **argv, char **envp)
> >                  }
> >  
> >                  /* backward compatibility behaviour for case "-m 0" */
> > -                if (sz == 0) {
> > -                    sz = default_ram_size;
> > -                }
> > -
> > -                sz = QEMU_ALIGN_UP(sz, 8192);
> > -                ram_size = sz;
> > -                if (ram_size != sz) {
> > -                    error_report("ram size too large");
> > -                    exit(EXIT_FAILURE);
> > +                if (sz != 0) {
> > +                    ram_size = sz;
> >                  }
> >  
> > -                maxmem_str = qemu_opt_get(opts, "maxmem");
> > -                slots_str = qemu_opt_get(opts, "slots");
> > -                if (maxmem_str && slots_str) {
> > -                    uint64_t slots;
> > -
> > -                    sz = qemu_opt_get_size(opts, "maxmem", 0);
> > -                    if (sz < ram_size) {
> > -                        fprintf(stderr, "qemu: invalid -m option value: maxmem "
> > -                                "(%" PRIu64 ") <= initial memory (%"
> > -                                RAM_ADDR_FMT ")\n", sz, ram_size);
> > -                        exit(EXIT_FAILURE);
> > -                    }
> > -
> > -                    slots = qemu_opt_get_number(opts, "slots", 0);
> > -                    if ((sz > ram_size) && !slots) {
> > -                        fprintf(stderr, "qemu: invalid -m option value: maxmem "
> > -                                "(%" PRIu64 ") more than initial memory (%"
> > -                                RAM_ADDR_FMT ") but no hotplug slots where "
> > -                                "specified\n", sz, ram_size);
> > -                        exit(EXIT_FAILURE);
> > -                    }
> > -
> > -                    if ((sz <= ram_size) && slots) {
> > -                        fprintf(stderr, "qemu: invalid -m option value:  %"
> > -                                PRIu64 " hotplug slots where specified but "
> > -                                "maxmem (%" PRIu64 ") <= initial memory (%"
> > -                                RAM_ADDR_FMT ")\n", slots, sz, ram_size);
> > -                        exit(EXIT_FAILURE);
> > -                    }
> > -                    maxram_size = sz;
> > -                    ram_slots = slots;
> > -                } else if ((!maxmem_str && slots_str) ||
> > -                           (maxmem_str && !slots_str)) {
> > -                    fprintf(stderr, "qemu: invalid -m option value: missing "
> > -                            "'%s' option\n", slots_str ? "maxmem" : "slots");
> > -                    exit(EXIT_FAILURE);
> > -                }
> > +                maxram_size_str = qemu_opt_get(opts, "maxmem");
> > +                ram_slots_str = qemu_opt_get(opts, "slots");
> >                  break;
> >              }
> >  #ifdef CONFIG_TPM
> > @@ -4203,14 +4156,48 @@ int main(int argc, char **argv, char **envp)
> >          exit(1);
> >      }
> >  
> > -    /* store value for the future use */
> > -    qemu_opt_set_number(qemu_find_opts_singleton("memory"), "size", ram_size);
> > -
> >      if (qemu_opts_foreach(qemu_find_opts("device"), device_help_func, NULL, 0)
> >          != 0) {
> >          exit(0);
> >      }
> >  
> > +    if (ram_size) {
> > +        object_property_set_int(OBJECT(current_machine), ram_size,
> > +                                MACHINE_MEMORY_SIZE_OPT, &local_err);
> > +        if (local_err) {
> > +            error_report("%s", error_get_pretty(local_err));
> > +            error_free(local_err);
> > +            exit(EXIT_FAILURE);
> > +        }
> > +    }
> > +
> > +    if (maxram_size_str) {
> > +        uint64_t sz = qemu_opt_get_size(qemu_find_opts_singleton("memory"),
> > +                                        "maxmem", 0);
> > +
> > +        parse_option_size("maxmem", maxram_size_str, &sz, &local_err);
> > +        if (local_err) {
> > +            error_report("%s", error_get_pretty(local_err));
> > +            error_free(local_err);
> > +            exit(EXIT_FAILURE);
> > +        }
> > +        object_property_set_int(OBJECT(current_machine), sz,
> > +                                MACHINE_MAXMEMORY_SIZE_OPT, &local_err);
> > +        if (local_err) {
> > +            error_report("%s", error_get_pretty(local_err));
> > +            error_free(local_err);
> > +            exit(EXIT_FAILURE);
> > +        }
> > +    }
> > +
> > +    if (ram_slots_str) {
> > +        if (object_set_property(MACHINE_MEMORY_SLOTS_OPT, ram_slots_str,
> > +                                current_machine)) {
> > +            exit(EXIT_FAILURE);
> > +        }
> > +    }
> > +
> > +
> >      machine_opts = qemu_get_machine_opts();
> >      if (qemu_opt_foreach(machine_opts, object_set_property, current_machine,
> >                           1) < 0) {
> > @@ -4229,6 +4216,9 @@ int main(int argc, char **argv, char **envp)
> >          }
> >      }
> >  
> > +    ram_size = object_property_get_int(OBJECT(current_machine),
> > +                                       MACHINE_MEMORY_SIZE_OPT,
> > +                                       &error_abort);
> >      machine_opts = qemu_get_machine_opts();
> >      kernel_filename = qemu_opt_get(machine_opts, "kernel");
> >      initrd_filename = qemu_opt_get(machine_opts, "initrd");
> > @@ -4314,14 +4304,6 @@ int main(int argc, char **argv, char **envp)
> >      if (foreach_device_config(DEV_BT, bt_parse))
> >          exit(1);
> >  
> > -    if (!xen_enabled()) {
> > -        /* On 32-bit hosts, QEMU is limited by virtual address space */
> > -        if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) {
> > -            fprintf(stderr, "qemu: at most 2047 MB RAM can be simulated\n");
> > -            exit(1);
> > -        }
> > -    }
> > -
> >      blk_mig_init();
> >      ram_mig_init();
> >  
> > @@ -4386,12 +4368,15 @@ int main(int argc, char **argv, char **envp)
> >  
> >      qdev_machine_init();
> >  
> > -    current_machine->ram_size = ram_size;
> > -    current_machine->maxram_size = maxram_size;
> > -    current_machine->ram_slots = ram_slots;
> >      current_machine->boot_order = boot_order;
> >      current_machine->cpu_model = cpu_model;
> >  
> > +    machine_class->instance_pre_init(current_machine, &local_err);
> > +    if (local_err != NULL) {
> > +        error_report("%s", error_get_pretty(local_err));
> > +        error_free(local_err);
> > +        exit(EXIT_FAILURE);
> > +    }
> >      machine_class->init(current_machine);
> >  
> >      audio_init();
> > -- 
> > 1.7.1
> 

Re: [Qemu-devel] [PATCH 3/4] machine: convert ram_size, maxram_size, ram_slots to properties

[Michael S. Tsirkin]

On Wed, Jun 25, 2014 at 03:08:03PM +0200, Igor Mammedov wrote:
> On Wed, 25 Jun 2014 14:54:47 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Jun 25, 2014 at 01:42:22PM +0200, Igor Mammedov wrote:
> > > ... short term it will help writing unit tests accessing
> > > these values via QMP. And down the road it will allow to drop
> > > global variable ram_size.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  hw/core/machine.c   |  181 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  include/hw/boards.h |    5 ++
> > >  vl.c                |  123 +++++++++++++++-------------------
> > >  3 files changed, 240 insertions(+), 69 deletions(-)
> > > 
> > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > index cbba679..f73b810 100644
> > > --- a/hw/core/machine.c
> > > +++ b/hw/core/machine.c
> > > @@ -12,6 +12,10 @@
> > >  
> > >  #include "hw/boards.h"
> > >  #include "qapi/visitor.h"
> > > +#include "qemu/error-report.h"
> > > +#include "hw/xen/xen.h"
> > > +
> > > +static const ram_addr_t default_ram_size  = 128 * 1024 * 1024;
> > >  
> > >  static char *machine_get_accel(Object *obj, Error **errp)
> > >  {
> > > @@ -235,8 +239,118 @@ static void machine_set_firmware(Object *obj, const char *value, Error **errp)
> > >      ms->firmware = g_strdup(value);
> > >  }
> > >  
> > > +static void machine_get_ram_size(Object *obj, Visitor *v,
> > > +                                 void *opaque, const char *name,
> > > +                                 Error **errp)
> > > +{
> > > +    MachineState *ms = MACHINE(obj);
> > > +    int64_t value = ms->ram_size;
> > > +
> > > +    visit_type_int(v, &value, name, errp);
> > > +}
> > > +
> > > +static void machine_set_ram_size(Object *obj, Visitor *v,
> > > +                                 void *opaque, const char *name,
> > > +                                 Error **errp)
> > > +{
> > > +    MachineState *ms = MACHINE(obj);
> > > +    Error *error = NULL;
> > > +    int64_t value;
> > > +
> > > +    visit_type_int(v, &value, name, &error);
> > > +    if (error) {
> > > +        goto out;
> > > +    }
> > > +
> > > +    value = QEMU_ALIGN_UP(value, 8192);
> > > +    ms->ram_size = value;
> > > +    if (ms->ram_size != value) {
> > > +        error_setg(&error, "ram size too large");
> > > +        goto out;
> > > +    }
> > > +
> > > +    if (!ms->ram_size) {
> > > +        error_setg(&error, "ram size can't be 0");
> > > +    }
> > > +out:
> > > +    if (error) {
> > > +        error_propagate(errp, error);
> > > +    }
> > > +}
> > > +
> > > +static void machine_get_maxram_size(Object *obj, Visitor *v,
> > > +                                    void *opaque, const char *name,
> > > +                                    Error **errp)
> > > +{
> > > +    MachineState *ms = MACHINE(obj);
> > > +    int64_t value = ms->maxram_size;
> > > +
> > > +    visit_type_int(v, &value, name, errp);
> > > +}
> > > +
> > > +static void machine_set_maxram_size(Object *obj, Visitor *v,
> > > +                                    void *opaque, const char *name,
> > > +                                    Error **errp)
> > > +{
> > > +    MachineState *ms = MACHINE(obj);
> > > +    Error *error = NULL;
> > > +    int64_t value;
> > > +
> > > +    visit_type_int(v, &value, name, &error);
> > > +    if (error) {
> > > +        goto out;
> > > +    }
> > > +
> > > +    ms->maxram_size = value;
> > > +    if (ms->maxram_size != value) {
> > > +        error_setg(&error, "maxmem is too large");
> > > +        goto out;
> > > +    }
> > > +
> > > +    if (!ms->maxram_size) {
> > > +        error_setg(&error, "maxmem can't be 0");
> > > +    }
> > > +out:
> > > +    if (error) {
> > > +        error_propagate(errp, error);
> > > +    }
> > > +}
> > > +
> > > +static void machine_get_ram_slots(Object *obj, Visitor *v,
> > > +                                  void *opaque, const char *name,
> > > +                                  Error **errp)
> > > +{
> > > +    MachineState *ms = MACHINE(obj);
> > > +    int64_t value = ms->ram_slots;
> > > +
> > > +    visit_type_int(v, &value, name, errp);
> > > +}
> > > +
> > > +static void machine_set_ram_slots(Object *obj, Visitor *v,
> > > +                                  void *opaque, const char *name,
> > > +                                  Error **errp)
> > > +{
> > > +    MachineState *ms = MACHINE(obj);
> > > +    Error *error = NULL;
> > > +    int64_t value;
> > > +
> > > +    visit_type_int(v, &value, name, &error);
> > > +    if (error) {
> > > +        goto out;
> > > +    }
> > > +
> > > +    ms->ram_slots = value;
> > > +
> > > +out:
> > > +    if (error) {
> > > +        error_propagate(errp, error);
> > > +    }
> > > +}
> > >
> > 
> > Really that's too much boiler plate code for
> > each value.  Can't we support a "validate" callback from
> > standard integer values, to be called after parsing?
> machine_set_ram_slots() could be reduced to:
> 
> +static void machine_set_ram_slots(Object *obj, Visitor *v,
> +                                  void *opaque, const char *name,
> +                                  Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +
> +    visit_type_int(v, &value, name, errp);
> +}
> 

Still too much for a single property, sorry.

> maxram_size/ram_size setters could be reduced ~11 LOC moving common
> parts to a helper.
> 
> But generic property validator is probably out of scope for this
> series


We have time to do it right, we are past development freeze so this is
2.2 material.

In fact, FYI I'm not queueing 2.2 patches anyway, it would help me
if you tagged them as RFC explicitly.

> and it won't help much in this case.

Done right, it should help.

Basically we have a ton of integer properties
where the only tricky thing is that
value is validated before it is saved.
So what I want is a variant of DEFINE_PROP
which invokes a callback on the value
before saving it, and checks the return code.

Along the lines of:

    DEFINE_PROP_UINT32_VALIDATE("foo", type, field, default, validate),

> > 
> > 
> > >  static void machine_initfn(Object *obj)
> > >  {
> > > +    MachineState *ms = MACHINE(obj);
> > > +
> > >      object_property_add_str(obj, "accel",
> > >                              machine_get_accel, machine_set_accel, NULL);
> > >      object_property_add_bool(obj, "kernel_irqchip",
> > > @@ -274,6 +388,20 @@ static void machine_initfn(Object *obj)
> > >      object_property_add_bool(obj, "usb", machine_get_usb, machine_set_usb, NULL);
> > >      object_property_add_str(obj, "firmware",
> > >                              machine_get_firmware, machine_set_firmware, NULL);
> > > +
> > > +    ms->ram_size = default_ram_size;
> > > +    object_property_add(obj, MACHINE_MEMORY_SIZE_OPT, "int",
> > > +                        machine_get_ram_size,
> > > +                        machine_set_ram_size,
> > > +                        NULL, NULL, NULL);
> > > +    object_property_add(obj, MACHINE_MAXMEMORY_SIZE_OPT, "int",
> > > +                        machine_get_maxram_size,
> > > +                        machine_set_maxram_size,
> > > +                        NULL, NULL, NULL);
> > > +    object_property_add(obj, MACHINE_MEMORY_SLOTS_OPT, "int",
> > > +                        machine_get_ram_slots,
> > > +                        machine_set_ram_slots,
> > > +                        NULL, NULL, NULL);
> > >  }
> > >  
> > >  static void machine_finalize(Object *obj)
> > > @@ -290,11 +418,64 @@ static void machine_finalize(Object *obj)
> > >      g_free(ms->firmware);
> > >  }
> > >  
> > > +static void machine_pre_init(MachineState *ms, Error **errp)
> > > +{
> > > +    Error *error = NULL;
> > > +
> > > +    if ((ms->ram_size > ms->maxram_size) && ms->maxram_size) {
> > > +        error_setg(&error, "invalid \"maxmem\" value (%" RAM_ADDR_FMT
> > > +                   ") < initial memory (%" RAM_ADDR_FMT ")",
> > > +                   ms->maxram_size, ms->ram_size);
> > > +        goto out;
> > > +    }
> > > +
> > > +    if ((ms->ram_size < ms->maxram_size) && !ms->ram_slots) {
> > > +        error_setg(&error, "invalid \"maxmem\" value (%" RAM_ADDR_FMT ") "
> > > +                   "more than initial memory (%" PRIu64 ") but "
> > > +                   "no hotplug slots where specified",
> > > +                   ms->maxram_size, ms->ram_size);
> > > +        goto out;
> > > +    }
> > > +
> > > +    if ((ms->ram_size == ms->maxram_size) && ms->ram_slots) {
> > 
> > ugly () within conditions, not really needed.
> > 
> > > +        error_setg(&error, "invalid \"maxmem\" value (%" RAM_ADDR_FMT ") "
> > > +                   "it must be more than initial memory if hotplug slots > 0",
> > > +                   ms->maxram_size);
> > > +        goto out;
> > > +    }
> > > +
> > > +    if (!ms->maxram_size && !ms->ram_slots) {
> > > +        ms->maxram_size = ms->ram_size;
> > > +    }
> > > +
> > > +    if (!xen_enabled()) {
> > > +        /* On 32-bit hosts, QEMU is limited by virtual address space */
> > > +        if (HOST_LONG_BITS == 32 && ((ms->ram_size > (2047 << 20)) ||
> > > +                                     (ms->maxram_size > (2047 << 20)))) {
> > > +            error_setg(&error, "at most 2047 MB RAM can be simulated\n");
> > > +            goto out;
> > > +        }
> > > +    }
> > > +
> > > +out:
> > > +    if (error) {
> > > +        error_propagate(errp, error);
> > > +    }
> > > +}
> > > +
> > > +static void machine_class_init(ObjectClass *oc, void *data)
> > > +{
> > > +    MachineClass *mc = MACHINE_CLASS(oc);
> > > +
> > > +    mc->instance_pre_init = machine_pre_init;
> > > +}
> > > +
> > >  static const TypeInfo machine_info = {
> > >      .name = TYPE_MACHINE,
> > >      .parent = TYPE_OBJECT,
> > >      .abstract = true,
> > >      .class_size = sizeof(MachineClass),
> > > +    .class_init = machine_class_init,
> > >      .instance_size = sizeof(MachineState),
> > >      .instance_init = machine_initfn,
> > >      .instance_finalize = machine_finalize,
> > > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > > index 605a970..1b980c5 100644
> > > --- a/include/hw/boards.h
> > > +++ b/include/hw/boards.h
> > > @@ -81,6 +81,7 @@ struct MachineClass {
> > >      const char *desc;
> > >  
> > >      void (*init)(MachineState *state);
> > > +    void (*instance_pre_init)(MachineState *state, Error **errp);
> > >      void (*reset)(void);
> > >      void (*hot_add_cpu)(const int64_t id, Error **errp);
> > >      int (*kvm_type)(const char *arg);
> > > @@ -134,4 +135,8 @@ struct MachineState {
> > >      const char *cpu_model;
> > >  };
> > >  
> > > +#define MACHINE_MEMORY_SIZE_OPT    "memory-size"
> > > +#define MACHINE_MEMORY_SLOTS_OPT   "memory-slots"
> > > +#define MACHINE_MAXMEMORY_SIZE_OPT "maxmem"
> > > +
> > >  #endif
> > > diff --git a/vl.c b/vl.c
> > > index 0a39c93..3ed3582 100644
> > > --- a/vl.c
> > > +++ b/vl.c
> > > @@ -119,8 +119,6 @@ int main(int argc, char **argv)
> > >  #include "qom/object_interfaces.h"
> > >  #include "qapi-event.h"
> > >  
> > > -#define DEFAULT_RAM_SIZE 128
> > > -
> > >  #define MAX_VIRTIO_CONSOLES 1
> > >  #define MAX_SCLP_CONSOLES 1
> > >  
> > > @@ -2928,10 +2926,8 @@ int main(int argc, char **argv, char **envp)
> > >      const char *trace_events = NULL;
> > >      const char *trace_file = NULL;
> > >      Error *local_err = NULL;
> > > -    const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
> > > -                                        1024 * 1024;
> > > -    ram_addr_t maxram_size = default_ram_size;
> > > -    uint64_t ram_slots = 0;
> > > +    const char *maxram_size_str = NULL;
> > > +    const char *ram_slots_str = NULL;
> > >      FILE *vmstate_dump_file = NULL;
> > >  
> > >      atexit(qemu_run_exit_notifiers);
> > > @@ -2978,7 +2974,7 @@ int main(int argc, char **argv, char **envp)
> > >      module_call_init(MODULE_INIT_MACHINE);
> > >      machine_class = find_default_machine();
> > >      cpu_model = NULL;
> > > -    ram_size = default_ram_size;
> > > +    ram_size = 0;
> > >      snapshot = 0;
> > >      cyls = heads = secs = 0;
> > >      translation = BIOS_ATA_TRANSLATION_AUTO;
> > > @@ -3269,7 +3265,6 @@ int main(int argc, char **argv, char **envp)
> > >              case QEMU_OPTION_m: {
> > >                  uint64_t sz;
> > >                  const char *mem_str;
> > > -                const char *maxmem_str, *slots_str;
> > >  
> > >                  opts = qemu_opts_parse(qemu_find_opts("memory"),
> > >                                         optarg, 1);
> > > @@ -3287,7 +3282,7 @@ int main(int argc, char **argv, char **envp)
> > >                      exit(EXIT_FAILURE);
> > >                  }
> > >  
> > > -                sz = qemu_opt_get_size(opts, "size", ram_size);
> > > +                sz = qemu_opt_get_size(opts, "size", 0);
> > >  
> > >                  /* Fix up legacy suffix-less format */
> > >                  if (g_ascii_isdigit(mem_str[strlen(mem_str) - 1])) {
> > > @@ -3301,54 +3296,12 @@ int main(int argc, char **argv, char **envp)
> > >                  }
> > >  
> > >                  /* backward compatibility behaviour for case "-m 0" */
> > > -                if (sz == 0) {
> > > -                    sz = default_ram_size;
> > > -                }
> > > -
> > > -                sz = QEMU_ALIGN_UP(sz, 8192);
> > > -                ram_size = sz;
> > > -                if (ram_size != sz) {
> > > -                    error_report("ram size too large");
> > > -                    exit(EXIT_FAILURE);
> > > +                if (sz != 0) {
> > > +                    ram_size = sz;
> > >                  }
> > >  
> > > -                maxmem_str = qemu_opt_get(opts, "maxmem");
> > > -                slots_str = qemu_opt_get(opts, "slots");
> > > -                if (maxmem_str && slots_str) {
> > > -                    uint64_t slots;
> > > -
> > > -                    sz = qemu_opt_get_size(opts, "maxmem", 0);
> > > -                    if (sz < ram_size) {
> > > -                        fprintf(stderr, "qemu: invalid -m option value: maxmem "
> > > -                                "(%" PRIu64 ") <= initial memory (%"
> > > -                                RAM_ADDR_FMT ")\n", sz, ram_size);
> > > -                        exit(EXIT_FAILURE);
> > > -                    }
> > > -
> > > -                    slots = qemu_opt_get_number(opts, "slots", 0);
> > > -                    if ((sz > ram_size) && !slots) {
> > > -                        fprintf(stderr, "qemu: invalid -m option value: maxmem "
> > > -                                "(%" PRIu64 ") more than initial memory (%"
> > > -                                RAM_ADDR_FMT ") but no hotplug slots where "
> > > -                                "specified\n", sz, ram_size);
> > > -                        exit(EXIT_FAILURE);
> > > -                    }
> > > -
> > > -                    if ((sz <= ram_size) && slots) {
> > > -                        fprintf(stderr, "qemu: invalid -m option value:  %"
> > > -                                PRIu64 " hotplug slots where specified but "
> > > -                                "maxmem (%" PRIu64 ") <= initial memory (%"
> > > -                                RAM_ADDR_FMT ")\n", slots, sz, ram_size);
> > > -                        exit(EXIT_FAILURE);
> > > -                    }
> > > -                    maxram_size = sz;
> > > -                    ram_slots = slots;
> > > -                } else if ((!maxmem_str && slots_str) ||
> > > -                           (maxmem_str && !slots_str)) {
> > > -                    fprintf(stderr, "qemu: invalid -m option value: missing "
> > > -                            "'%s' option\n", slots_str ? "maxmem" : "slots");
> > > -                    exit(EXIT_FAILURE);
> > > -                }
> > > +                maxram_size_str = qemu_opt_get(opts, "maxmem");
> > > +                ram_slots_str = qemu_opt_get(opts, "slots");
> > >                  break;
> > >              }
> > >  #ifdef CONFIG_TPM
> > > @@ -4203,14 +4156,48 @@ int main(int argc, char **argv, char **envp)
> > >          exit(1);
> > >      }
> > >  
> > > -    /* store value for the future use */
> > > -    qemu_opt_set_number(qemu_find_opts_singleton("memory"), "size", ram_size);
> > > -
> > >      if (qemu_opts_foreach(qemu_find_opts("device"), device_help_func, NULL, 0)
> > >          != 0) {
> > >          exit(0);
> > >      }
> > >  
> > > +    if (ram_size) {
> > > +        object_property_set_int(OBJECT(current_machine), ram_size,
> > > +                                MACHINE_MEMORY_SIZE_OPT, &local_err);
> > > +        if (local_err) {
> > > +            error_report("%s", error_get_pretty(local_err));
> > > +            error_free(local_err);
> > > +            exit(EXIT_FAILURE);
> > > +        }
> > > +    }
> > > +
> > > +    if (maxram_size_str) {
> > > +        uint64_t sz = qemu_opt_get_size(qemu_find_opts_singleton("memory"),
> > > +                                        "maxmem", 0);
> > > +
> > > +        parse_option_size("maxmem", maxram_size_str, &sz, &local_err);
> > > +        if (local_err) {
> > > +            error_report("%s", error_get_pretty(local_err));
> > > +            error_free(local_err);
> > > +            exit(EXIT_FAILURE);
> > > +        }
> > > +        object_property_set_int(OBJECT(current_machine), sz,
> > > +                                MACHINE_MAXMEMORY_SIZE_OPT, &local_err);
> > > +        if (local_err) {
> > > +            error_report("%s", error_get_pretty(local_err));
> > > +            error_free(local_err);
> > > +            exit(EXIT_FAILURE);
> > > +        }
> > > +    }
> > > +
> > > +    if (ram_slots_str) {
> > > +        if (object_set_property(MACHINE_MEMORY_SLOTS_OPT, ram_slots_str,
> > > +                                current_machine)) {
> > > +            exit(EXIT_FAILURE);
> > > +        }
> > > +    }
> > > +
> > > +
> > >      machine_opts = qemu_get_machine_opts();
> > >      if (qemu_opt_foreach(machine_opts, object_set_property, current_machine,
> > >                           1) < 0) {
> > > @@ -4229,6 +4216,9 @@ int main(int argc, char **argv, char **envp)
> > >          }
> > >      }
> > >  
> > > +    ram_size = object_property_get_int(OBJECT(current_machine),
> > > +                                       MACHINE_MEMORY_SIZE_OPT,
> > > +                                       &error_abort);
> > >      machine_opts = qemu_get_machine_opts();
> > >      kernel_filename = qemu_opt_get(machine_opts, "kernel");
> > >      initrd_filename = qemu_opt_get(machine_opts, "initrd");
> > > @@ -4314,14 +4304,6 @@ int main(int argc, char **argv, char **envp)
> > >      if (foreach_device_config(DEV_BT, bt_parse))
> > >          exit(1);
> > >  
> > > -    if (!xen_enabled()) {
> > > -        /* On 32-bit hosts, QEMU is limited by virtual address space */
> > > -        if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) {
> > > -            fprintf(stderr, "qemu: at most 2047 MB RAM can be simulated\n");
> > > -            exit(1);
> > > -        }
> > > -    }
> > > -
> > >      blk_mig_init();
> > >      ram_mig_init();
> > >  
> > > @@ -4386,12 +4368,15 @@ int main(int argc, char **argv, char **envp)
> > >  
> > >      qdev_machine_init();
> > >  
> > > -    current_machine->ram_size = ram_size;
> > > -    current_machine->maxram_size = maxram_size;
> > > -    current_machine->ram_slots = ram_slots;
> > >      current_machine->boot_order = boot_order;
> > >      current_machine->cpu_model = cpu_model;
> > >  
> > > +    machine_class->instance_pre_init(current_machine, &local_err);
> > > +    if (local_err != NULL) {
> > > +        error_report("%s", error_get_pretty(local_err));
> > > +        error_free(local_err);
> > > +        exit(EXIT_FAILURE);
> > > +    }
> > >      machine_class->init(current_machine);
> > >  
> > >      audio_init();
> > > -- 
> > > 1.7.1
> > 

Re: [Qemu-devel] [PATCH 3/4] machine: convert ram_size, maxram_size, ram_slots to properties

[Kirill Batuzov]

On Wed, 25 Jun 2014, Igor Mammedov wrote:

>  
> +    if (ram_size) {
> +        object_property_set_int(OBJECT(current_machine), ram_size,
> +                                MACHINE_MEMORY_SIZE_OPT, &local_err);
> +        if (local_err) {
> +            error_report("%s", error_get_pretty(local_err));
> +            error_free(local_err);
> +            exit(EXIT_FAILURE);
> +        }
> +    }

You can use &error_abort (global variable) instead of &local_err and let
error_set handle the rest. Like this:

   if (ram_size) {
       object_property_set_int(OBJECT(current_machine), ram_size,
                               MACHINE_MEMORY_SIZE_OPT, &error_abort);
   }

-- 
Kirill

Re: [Qemu-devel] [PATCH 3/4] machine: convert ram_size, maxram_size, ram_slots to properties

[Igor Mammedov]

On Wed, 25 Jun 2014 18:09:52 +0400 (MSK)
Kirill Batuzov <batuzovk@ispras.ru> wrote:

> On Wed, 25 Jun 2014, Igor Mammedov wrote:
> 
> >  
> > +    if (ram_size) {
> > +        object_property_set_int(OBJECT(current_machine), ram_size,
> > +                                MACHINE_MEMORY_SIZE_OPT, &local_err);
> > +        if (local_err) {
> > +            error_report("%s", error_get_pretty(local_err));
> > +            error_free(local_err);
> > +            exit(EXIT_FAILURE);
> > +        }
> > +    }
> 
> You can use &error_abort (global variable) instead of &local_err and let
> error_set handle the rest. Like this:
> 
>    if (ram_size) {
>        object_property_set_int(OBJECT(current_machine), ram_size,
>                                MACHINE_MEMORY_SIZE_OPT, &error_abort);
>    }
> 

thanks, I'll use error_abort.

[Qemu-devel] [PATCH 4/4] test -m option parameters

[Igor Mammedov]

adds a base for memory hotplug tests, starting with
checking that -m option accepts expected options.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/Makefile     |    2 +
 tests/memhp-test.c |  145 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 147 insertions(+), 0 deletions(-)
 create mode 100644 tests/memhp-test.c

diff --git a/tests/Makefile b/tests/Makefile
index 7e53d0d..24c7e2b 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -154,6 +154,7 @@ gcov-files-i386-y += hw/pci-bridge/i82801b11.c
 check-qtest-i386-y += tests/ioh3420-test$(EXESUF)
 gcov-files-i386-y += hw/pci-bridge/ioh3420.c
 check-qtest-i386-y += tests/usb-hcd-ehci-test$(EXESUF)
+check-qtest-i386-y += tests/memhp-test$(EXESUF)
 gcov-files-i386-y += hw/usb/hcd-ehci.c
 gcov-files-i386-y += hw/usb/hcd-uhci.c
 gcov-files-i386-y += hw/usb/dev-hid.c
@@ -293,6 +294,7 @@ libqos-pc-obj-y = $(libqos-obj-y) tests/libqos/pci-pc.o
 libqos-pc-obj-y += tests/libqos/malloc-pc.o
 libqos-omap-obj-y = $(libqos-obj-y) tests/libqos/i2c-omap.o
 
+tests/memhp-test$(EXESUF): tests/memhp-test.o $(libqos-obj-y)
 tests/rtc-test$(EXESUF): tests/rtc-test.o
 tests/m48t59-test$(EXESUF): tests/m48t59-test.o
 tests/endianness-test$(EXESUF): tests/endianness-test.o
diff --git a/tests/memhp-test.c b/tests/memhp-test.c
new file mode 100644
index 0000000..4698854
--- /dev/null
+++ b/tests/memhp-test.c
@@ -0,0 +1,145 @@
+/*
+ * memory hotplug test cases.
+ *
+ * Copyright (c) 2014 Red Hat Inc.
+ *
+ * Authors:
+ *  Igor Mammedov <imammedo@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include <stdio.h>
+#include <string.h>
+#include <glib.h>
+#include "libqtest.h"
+#include "include/qemu/option.h"
+
+typedef struct MOption {
+    const char *size;
+    const char *slots;
+    const char *maxmem;
+} MOption;
+
+typedef struct TestData TestData;
+struct TestData {
+    const char *args;
+    MOption m_option;
+    void (*test)(const TestData *data);
+};
+
+static void test_machine(gconstpointer opaque)
+{
+    TestData *s = (TestData *)opaque;
+    char *args = g_strdup_printf("%s", s->args);
+
+    if (s->m_option.size) {
+        char *old = args;
+        args = g_strdup_printf("%s -m %s", args, s->m_option.size);
+        g_free(old);
+    }
+
+    if (s->m_option.slots) {
+        char *old = args;
+        args = g_strdup_printf("%s -m slots=%s", args, s->m_option.slots);
+        g_free(old);
+    }
+
+    if (s->m_option.maxmem) {
+        char *old = args;
+        args = g_strdup_printf("%s -m maxmem=%s", args, s->m_option.maxmem);
+        g_free(old);
+    }
+
+    qtest_start(args);
+    s->test(s);
+    qtest_end();
+    g_free(args);
+}
+
+#define DEFARGS "-net none -display none -machine accel=qtest "
+
+#define ADD_COMMON(name, cmdline, sz, slots_nr, max_mem, func)               \
+    {                                                                        \
+        static const TestData d = {                                          \
+        .args = DEFARGS cmdline,                                             \
+        .m_option.size = sz,                                                 \
+        .m_option.slots = slots_nr,                                          \
+        .m_option.maxmem = max_mem,                                          \
+        .test = func};                                                       \
+        char *path;                                                          \
+        path = g_strdup_printf("/memhp/%s/[%s %s%s%s%s%s%s]", name, cmdline, \
+            d.m_option.size ? " -m size=" : "",                              \
+            d.m_option.size ? d.m_option.size : "",                          \
+            d.m_option.slots ? " -m slots=" : "",                            \
+            d.m_option.slots ? d.m_option.slots : "",                        \
+            d.m_option.maxmem ? " -m maxmem=" : "",                          \
+            d.m_option.maxmem ? d.m_option.maxmem : "");                     \
+        g_test_add_data_func(path, &d, test_machine);                        \
+        g_free(path);                                                        \
+    }
+
+#define ADD_X86_COMMON(name, cmdline, sz, slots_nr, max_mem, func)           \
+    if (strcmp(qtest_get_arch(), "i386") == 0 ||                             \
+        strcmp(qtest_get_arch(), "x86_64") == 0) {                           \
+        ADD_COMMON(name, cmdline, sz, slots_nr, max_mem, func)               \
+    }
+#define ADD_440FX_TEST(name, cmdline, sz, slots_nr, max_mem, func)           \
+    ADD_X86_COMMON(name "/pc", cmdline "-M pc", sz, slots_nr, max_mem, func)
+
+#define ADD_Q35_TEST(name, cmdline, sz, slots_nr, max_mem, func)             \
+    ADD_X86_COMMON(name "/q35" , cmdline "-M q35", sz, slots_nr, max_mem, func)
+
+#define ADD_TESTS(name, cmdline, sz, slots_nr, max_mem, func)                \
+    {                                                                        \
+        ADD_440FX_TEST(name, cmdline, sz, slots_nr, max_mem, func);          \
+        ADD_Q35_TEST(name, cmdline, sz, slots_nr, max_mem, func);            \
+    }
+
+static void test_num_prop_value(const char *path, const char *prop,
+                                const char *value)
+{
+    QDict *response;
+    uint64_t ret, num_value;
+
+    response = qmp("{ 'execute': 'qom-get',"
+                   "  'arguments': { 'path': '%s',"
+                   "                 'property': '%s' } }",
+                   path, prop);
+    /* qom-get may fail but should not, e.g., segfault. */
+    g_assert(qdict_haskey(response, "return"));
+    ret = qdict_get_int(response, "return");
+    QDECREF(response);
+
+    parse_option_size(prop, value, &num_value, &error_abort);
+    g_assert(ret == num_value);
+}
+
+static void test_args(const TestData *data)
+{
+    if (data->m_option.size) {
+        test_num_prop_value("/machine", "memory-size", data->m_option.size);
+    }
+
+    if (data->m_option.slots) {
+        test_num_prop_value("/machine", "memory-slots", data->m_option.slots);
+    }
+
+    if (data->m_option.maxmem) {
+        test_num_prop_value("/machine", "maxmem", data->m_option.maxmem);
+    }
+}
+
+int main(int argc, char *argv[])
+{
+    g_test_init(&argc, &argv, NULL);
+
+    ADD_TESTS("args", "", "8M", NULL, NULL, test_args);
+    ADD_TESTS("args", "", "8M", "1", "16M", test_args);
+    ADD_TESTS("args", "", "8M", "256", "16M", test_args);
+    ADD_TESTS("args", "", "8M", "1", "1G", test_args);
+    ADD_TESTS("args", "", "8M", "1", "1T", test_args);
+
+    return g_test_run();
+}
-- 
1.7.1