[Qemu-devel] [RFC 0/8] bus-less device hotplug

[Qemu-devel] [RFC 0/8] bus-less device hotplug

[Igor Mammedov]

This series is a excerpt from memory hotplug series, posted
for getting an opinion on hotplug implementation for bus-less
devices using link<>s as a means to provide connection
between hotplugged device and hotplug controller that performs
board specific actions that hot-added device ishould not be aware of.

Patches implementing link<> based hotplug are:
  qdev: link based hotplug
  pc: preallocate hotplug links for DIMMDevices
  ...
  pc: make PC_MACHINE memory hotplug controller
the rest is just an environment to make above work.

compile-able WIP tree is available at:
https://github.com/imammedo/qemu/commits/memory-hotplug-v8

way to test:
 - run:
x86_64-softmmu/qemu-system-x86_64 -monitor stdio -object memory-ram,id=foo,size=1G -m 1G,slots=1,maxmem=2G
 - in monitor execute:
(qemu) device_add dimm,id=dimm1,memdev=foo
(qemu) info mtree

there should be "foo" memory region under "hotplug-memory" container.


Igor Mammedov (7):
  vl.c: extend -m option to support options for memory hotplug
  make machine_class_init() accessible outside of vl.c
  pc: prepare PC for custom machine state
  qdev: link based hotplug
  pc: preallocate hotplug links for DIMMDevices
  pc: initialize memory hotplug address space
  pc: make PC_MACHINE memory hotplug controller

Vasilis Liaskovitis (1):
  dimm: implement dimm device abstraction

 default-configs/i386-softmmu.mak   |    1 +
 default-configs/x86_64-softmmu.mak |    1 +
 hw/Makefile.objs                   |    1 +
 hw/core/hotplug.c                  |   30 +++++++++++
 hw/core/machine.c                  |    7 +++
 hw/core/qdev.c                     |   15 +++++
 hw/i386/pc.c                       |   95 +++++++++++++++++++++++++++++++++-
 hw/i386/pc_piix.c                  |   34 ++++++------
 hw/i386/pc_q35.c                   |   10 ++--
 hw/mem/Makefile.objs               |    1 +
 hw/mem/dimm.c                      |  100 ++++++++++++++++++++++++++++++++++++
 include/hw/boards.h                |    4 ++
 include/hw/hotplug.h               |    2 +
 include/hw/i386/pc.h               |   23 ++++++++
 include/hw/mem/dimm.h              |   58 +++++++++++++++++++++
 include/hw/qdev-core.h             |    6 ++
 qemu-options.hx                    |    9 ++-
 vl.c                               |   58 ++++++++++++++++++---
 18 files changed, 421 insertions(+), 34 deletions(-)
 create mode 100644 hw/mem/Makefile.objs
 create mode 100644 hw/mem/dimm.c
 create mode 100644 include/hw/mem/dimm.h

[Qemu-devel] [RFC 1/8] vl.c: extend -m option to support options for memory hotplug

[Igor Mammedov]

Add following parameters:
  "slots" - total number of hotplug memory slots
  "maxmem" - maximum possible memory

"slots" and "maxmem" should go in pair and "maxmem" should be greater
than "mem" for memory hotplug to be enabled.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v3:
 - store maxmem & slots valus in QEMUMachineInitArgs
v2:
 - rebased on top of the latest "vl: convert -m to QemuOpts"
---
 include/hw/boards.h |    2 ++
 qemu-options.hx     |    9 ++++++---
 vl.c                |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+), 3 deletions(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 7bd2ea7..22d9496 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -10,6 +10,8 @@
 typedef struct QEMUMachineInitArgs {
     const QEMUMachine *machine;
     ram_addr_t ram_size;
+    ram_addr_t maxram_size;
+    uint64_t   ram_slots;
     const char *boot_order;
     const char *kernel_filename;
     const char *kernel_cmdline;
diff --git a/qemu-options.hx b/qemu-options.hx
index 7482750..add34c4 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -210,17 +210,20 @@ use is discouraged as it may be removed from future versions.
 ETEXI
 
 DEF("m", HAS_ARG, QEMU_OPTION_m,
-    "-m [size=]megs\n"
+    "-m[emory] [size=]megs[,slots=n,maxmem=size]\n"
     "                configure guest RAM\n"
     "                size: initial amount of guest memory (default: "
-    stringify(DEFAULT_RAM_SIZE) "MiB)\n",
+    stringify(DEFAULT_RAM_SIZE) "MiB)\n"
+    "                slots: number of hotplug slots (default: none)\n"
+    "                maxmem: maximum amount of guest memory (default: none)\n",
     QEMU_ARCH_ALL)
 STEXI
 @item -m [size=]@var{megs}
 @findex -m
 Set virtual RAM size to @var{megs} megabytes. Default is 128 MiB.  Optionally,
 a suffix of ``M'' or ``G'' can be used to signify a value in megabytes or
-gigabytes respectively.
+gigabytes respectively. Optional pair @var{slots}, @var{maxmem} could be used
+to set amount of hotluggable memory slots and possible maximum amount of memory.
 ETEXI
 
 DEF("mem-path", HAS_ARG, QEMU_OPTION_mempath,
diff --git a/vl.c b/vl.c
index 4b36bde..7bae6fe 100644
--- a/vl.c
+++ b/vl.c
@@ -517,6 +517,14 @@ static QemuOptsList qemu_mem_opts = {
             .name = "size",
             .type = QEMU_OPT_SIZE,
         },
+        {
+            .name = "slots",
+            .type = QEMU_OPT_NUMBER,
+        },
+        {
+            .name = "maxmem",
+            .type = QEMU_OPT_SIZE,
+        },
         { /* end of list */ }
     },
 };
@@ -2946,6 +2954,8 @@ int main(int argc, char **argv, char **envp)
     const char *trace_file = 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;
 
     atexit(qemu_run_exit_notifiers);
     error_set_progname(argv[0]);
@@ -3291,6 +3301,7 @@ 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);
@@ -3332,6 +3343,44 @@ int main(int argc, char **argv, char **envp)
                     error_report("ram size too large");
                     exit(EXIT_FAILURE);
                 }
+
+                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 (%"
+                                PRIu64 ")\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 (%"
+                                PRIu64 ") 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 (%"
+                                PRIu64 ")\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);
+                }
                 break;
             }
 #ifdef CONFIG_TPM
@@ -4389,6 +4438,8 @@ int main(int argc, char **argv, char **envp)
 
     QEMUMachineInitArgs args = { .machine = machine,
                                  .ram_size = ram_size,
+                                 .maxram_size = maxram_size,
+                                 .ram_slots = ram_slots,
                                  .boot_order = boot_order,
                                  .kernel_filename = kernel_filename,
                                  .kernel_cmdline = kernel_cmdline,
-- 
1.7.1

[Qemu-devel] [RFC 2/8] make machine_class_init() accessible outside of vl.c

[Igor Mammedov]

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

diff --git a/hw/core/machine.c b/hw/core/machine.c
index d3ffef7..ae308f4 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -12,6 +12,13 @@
 
 #include "hw/boards.h"
 
+void machine_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+
+    mc->qemu_machine = data;
+}
+
 static const TypeInfo machine_info = {
     .name = TYPE_MACHINE,
     .parent = TYPE_OBJECT,
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 22d9496..e1f1938 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -81,6 +81,8 @@ struct MachineClass {
     QEMUMachine *qemu_machine;
 };
 
+void machine_class_init(ObjectClass *oc, void *data);
+
 /**
  * MachineState:
  */
diff --git a/vl.c b/vl.c
index 7bae6fe..05b1158 100644
--- a/vl.c
+++ b/vl.c
@@ -1588,13 +1588,6 @@ void pcmcia_info(Monitor *mon, const QDict *qdict)
 
 MachineState *current_machine;
 
-static void machine_class_init(ObjectClass *oc, void *data)
-{
-    MachineClass *mc = MACHINE_CLASS(oc);
-
-    mc->qemu_machine = data;
-}
-
 int qemu_register_machine(QEMUMachine *m)
 {
     TypeInfo ti = {
-- 
1.7.1

Re: [Qemu-devel] [RFC 2/8] make machine_class_init() accessible outside of vl.c

[Marcel Apfelbaum]

On Thu, 2014-03-20 at 16:01 +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/core/machine.c   |    7 +++++++
>  include/hw/boards.h |    2 ++
>  vl.c                |    7 -------
>  3 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index d3ffef7..ae308f4 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -12,6 +12,13 @@
>  
>  #include "hw/boards.h"
>  
> +void machine_class_init(ObjectClass *oc, void *data)
As I said in 3/8 review, if we *need* to move the above method,
I would hide it in hw/core/machine.c.

Please have a look on 3/8 review for details.
Thanks,
Marcel

> +{
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +
> +    mc->qemu_machine = data;
> +}
> +
>  static const TypeInfo machine_info = {
>      .name = TYPE_MACHINE,
>      .parent = TYPE_OBJECT,
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 22d9496..e1f1938 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -81,6 +81,8 @@ struct MachineClass {
>      QEMUMachine *qemu_machine;
>  };
>  
> +void machine_class_init(ObjectClass *oc, void *data);
> +
>  /**
>   * MachineState:
>   */
> diff --git a/vl.c b/vl.c
> index 7bae6fe..05b1158 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1588,13 +1588,6 @@ void pcmcia_info(Monitor *mon, const QDict *qdict)
>  
>  MachineState *current_machine;
>  
> -static void machine_class_init(ObjectClass *oc, void *data)
> -{
> -    MachineClass *mc = MACHINE_CLASS(oc);
> -
> -    mc->qemu_machine = data;
> -}
> -
>  int qemu_register_machine(QEMUMachine *m)
>  {
>      TypeInfo ti = {

Re: [Qemu-devel] [RFC 2/8] make machine_class_init() accessible outside of vl.c

[Marcel Apfelbaum]

On Sun, 2014-03-23 at 17:27 +0200, Marcel Apfelbaum wrote:
> On Thu, 2014-03-20 at 16:01 +0100, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/core/machine.c   |    7 +++++++
> >  include/hw/boards.h |    2 ++
> >  vl.c                |    7 -------
> >  3 files changed, 9 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index d3ffef7..ae308f4 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -12,6 +12,13 @@
> >  
> >  #include "hw/boards.h"
> >  
> > +void machine_class_init(ObjectClass *oc, void *data)
> As I said in 3/8 review, if we *need* to move the above method,
> I would hide it in hw/core/machine.c.
Just to make it more clear, I was talking about not making
it visible in "hw/boards.h", because you *did* move it to
hw/core/machine.c :)

Thanks,
Marcel

> 
> Please have a look on 3/8 review for details.
> Thanks,
> Marcel
> 
> > +{
> > +    MachineClass *mc = MACHINE_CLASS(oc);
> > +
> > +    mc->qemu_machine = data;
> > +}
> > +
> >  static const TypeInfo machine_info = {
> >      .name = TYPE_MACHINE,
> >      .parent = TYPE_OBJECT,
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 22d9496..e1f1938 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -81,6 +81,8 @@ struct MachineClass {
> >      QEMUMachine *qemu_machine;
> >  };
> >  
> > +void machine_class_init(ObjectClass *oc, void *data);
> > +
> >  /**
> >   * MachineState:
> >   */
> > diff --git a/vl.c b/vl.c
> > index 7bae6fe..05b1158 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -1588,13 +1588,6 @@ void pcmcia_info(Monitor *mon, const QDict *qdict)
> >  
> >  MachineState *current_machine;
> >  
> > -static void machine_class_init(ObjectClass *oc, void *data)
> > -{
> > -    MachineClass *mc = MACHINE_CLASS(oc);
> > -
> > -    mc->qemu_machine = data;
> > -}
> > -
> >  int qemu_register_machine(QEMUMachine *m)
> >  {
> >      TypeInfo ti = {
> 
> 
> 
> 

[Qemu-devel] [RFC 3/8] pc: prepare PC for custom machine state

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc.c         |   26 ++++++++++++++++++++++++++
 hw/i386/pc_piix.c    |   34 +++++++++++++++++-----------------
 hw/i386/pc_q35.c     |   10 +++++-----
 include/hw/i386/pc.h |   14 ++++++++++++++
 4 files changed, 62 insertions(+), 22 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index e715a33..e0bc3a2 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1413,3 +1413,29 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
         gsi_state->ioapic_irq[i] = qdev_get_gpio_in(dev, i);
     }
 }
+
+void qemu_register_pc_machine(QEMUMachine *m)
+{
+    TypeInfo ti = {
+        .name       = g_strconcat(m->name, TYPE_MACHINE_SUFFIX, NULL),
+        .parent     = TYPE_PC_MACHINE,
+        .class_init = machine_class_init,
+        .class_data = (void *)m,
+    };
+
+    type_register(&ti);
+}
+
+static const TypeInfo pc_machine_info = {
+    .name = TYPE_PC_MACHINE,
+    .parent = TYPE_MACHINE,
+    .abstract = true,
+    .instance_size = sizeof(PCMachineState),
+};
+
+static void pc_machine_register_types(void)
+{
+    type_register_static(&pc_machine_info);
+}
+
+type_init(pc_machine_register_types)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 7930a26..97df43e 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -817,24 +817,24 @@ static QEMUMachine xenfv_machine = {
 
 static void pc_machine_init(void)
 {
-    qemu_register_machine(&pc_i440fx_machine_v2_0);
-    qemu_register_machine(&pc_i440fx_machine_v1_7);
-    qemu_register_machine(&pc_i440fx_machine_v1_6);
-    qemu_register_machine(&pc_i440fx_machine_v1_5);
-    qemu_register_machine(&pc_i440fx_machine_v1_4);
-    qemu_register_machine(&pc_machine_v1_3);
-    qemu_register_machine(&pc_machine_v1_2);
-    qemu_register_machine(&pc_machine_v1_1);
-    qemu_register_machine(&pc_machine_v1_0);
-    qemu_register_machine(&pc_machine_v0_15);
-    qemu_register_machine(&pc_machine_v0_14);
-    qemu_register_machine(&pc_machine_v0_13);
-    qemu_register_machine(&pc_machine_v0_12);
-    qemu_register_machine(&pc_machine_v0_11);
-    qemu_register_machine(&pc_machine_v0_10);
-    qemu_register_machine(&isapc_machine);
+    qemu_register_pc_machine(&pc_i440fx_machine_v2_0);
+    qemu_register_pc_machine(&pc_i440fx_machine_v1_7);
+    qemu_register_pc_machine(&pc_i440fx_machine_v1_6);
+    qemu_register_pc_machine(&pc_i440fx_machine_v1_5);
+    qemu_register_pc_machine(&pc_i440fx_machine_v1_4);
+    qemu_register_pc_machine(&pc_machine_v1_3);
+    qemu_register_pc_machine(&pc_machine_v1_2);
+    qemu_register_pc_machine(&pc_machine_v1_1);
+    qemu_register_pc_machine(&pc_machine_v1_0);
+    qemu_register_pc_machine(&pc_machine_v0_15);
+    qemu_register_pc_machine(&pc_machine_v0_14);
+    qemu_register_pc_machine(&pc_machine_v0_13);
+    qemu_register_pc_machine(&pc_machine_v0_12);
+    qemu_register_pc_machine(&pc_machine_v0_11);
+    qemu_register_pc_machine(&pc_machine_v0_10);
+    qemu_register_pc_machine(&isapc_machine);
 #ifdef CONFIG_XEN
-    qemu_register_machine(&xenfv_machine);
+    qemu_register_pc_machine(&xenfv_machine);
 #endif
 }
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index c844dc2..16b4daa 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -358,11 +358,11 @@ static QEMUMachine pc_q35_machine_v1_4 = {
 
 static void pc_q35_machine_init(void)
 {
-    qemu_register_machine(&pc_q35_machine_v2_0);
-    qemu_register_machine(&pc_q35_machine_v1_7);
-    qemu_register_machine(&pc_q35_machine_v1_6);
-    qemu_register_machine(&pc_q35_machine_v1_5);
-    qemu_register_machine(&pc_q35_machine_v1_4);
+    qemu_register_pc_machine(&pc_q35_machine_v2_0);
+    qemu_register_pc_machine(&pc_q35_machine_v1_7);
+    qemu_register_pc_machine(&pc_q35_machine_v1_6);
+    qemu_register_pc_machine(&pc_q35_machine_v1_5);
+    qemu_register_pc_machine(&pc_q35_machine_v1_4);
 }
 
 machine_init(pc_q35_machine_init);
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 9010246..a01a220 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -12,9 +12,23 @@
 #include "qemu/bitmap.h"
 #include "sysemu/sysemu.h"
 #include "hw/pci/pci.h"
+#include "hw/boards.h"
 
 #define HPET_INTCAP "hpet-intcap"
 
+struct PCMachineState {
+    /*< private >*/
+    MachineState parent_obj;
+};
+
+typedef struct PCMachineState PCMachineState;
+
+#define TYPE_PC_MACHINE "abstract-pc-machine"
+#define PC_MACHINE(obj) \
+     OBJECT_CHECK(PCMachineState, (obj), TYPE_PC_MACHINE)
+
+void qemu_register_pc_machine(QEMUMachine *m);
+
 /* PC-style peripherals (also used by other machines).  */
 
 typedef struct PcPciInfo {
-- 
1.7.1

Re: [Qemu-devel] [RFC 3/8] pc: prepare PC for custom machine state

[Marcel Apfelbaum]

On Thu, 2014-03-20 at 16:01 +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/i386/pc.c         |   26 ++++++++++++++++++++++++++
>  hw/i386/pc_piix.c    |   34 +++++++++++++++++-----------------
>  hw/i386/pc_q35.c     |   10 +++++-----
>  include/hw/i386/pc.h |   14 ++++++++++++++
>  4 files changed, 62 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index e715a33..e0bc3a2 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1413,3 +1413,29 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
>          gsi_state->ioapic_irq[i] = qdev_get_gpio_in(dev, i);
>      }
>  }
> +
> +void qemu_register_pc_machine(QEMUMachine *m)
I am not so comfortable with this approach because
every subsystem (e.g pc) will have to duplicate the
"register machine" code until the conversion from
QEMUMachine to MachineClass is over. (which I hope
it will not take too much time)

I propose a patch already in the list which does that
automatically by moving this logic into hw/core/machine.c .
In this way it will be much less code "touched" during conversion. 

Andreas, did you have anything against the usage of 'class_base_init' ?

The patch is:
[PATCH 1/3] hw/machine: move QEMUMachine assignment into the core machine
http://lists.gnu.org/archive/html/qemu-devel/2014-03/msg02151.html

> +{
> +    TypeInfo ti = {
> +        .name       = g_strconcat(m->name, TYPE_MACHINE_SUFFIX, NULL),
This leads to a small memory leak, I missed that myself :(
Here is a fixed version:
[PATCHv2] vl.c: Fix memory leak in qemu_register_machine
https://www.mail-archive.com/qemu-devel@nongnu.org/msg222800.html

Thanks,
Marcel

> +        .parent     = TYPE_PC_MACHINE,
> +        .class_init = machine_class_init,
> +        .class_data = (void *)m,
> +    };
> +
> +    type_register(&ti);
> +}
> +
> +static const TypeInfo pc_machine_info = {
> +    .name = TYPE_PC_MACHINE,
> +    .parent = TYPE_MACHINE,
> +    .abstract = true,
> +    .instance_size = sizeof(PCMachineState),
> +};
> +
> +static void pc_machine_register_types(void)
> +{
> +    type_register_static(&pc_machine_info);
> +}
> +
> +type_init(pc_machine_register_types)
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 7930a26..97df43e 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -817,24 +817,24 @@ static QEMUMachine xenfv_machine = {
>  
>  static void pc_machine_init(void)
>  {
> -    qemu_register_machine(&pc_i440fx_machine_v2_0);
> -    qemu_register_machine(&pc_i440fx_machine_v1_7);
> -    qemu_register_machine(&pc_i440fx_machine_v1_6);
> -    qemu_register_machine(&pc_i440fx_machine_v1_5);
> -    qemu_register_machine(&pc_i440fx_machine_v1_4);
> -    qemu_register_machine(&pc_machine_v1_3);
> -    qemu_register_machine(&pc_machine_v1_2);
> -    qemu_register_machine(&pc_machine_v1_1);
> -    qemu_register_machine(&pc_machine_v1_0);
> -    qemu_register_machine(&pc_machine_v0_15);
> -    qemu_register_machine(&pc_machine_v0_14);
> -    qemu_register_machine(&pc_machine_v0_13);
> -    qemu_register_machine(&pc_machine_v0_12);
> -    qemu_register_machine(&pc_machine_v0_11);
> -    qemu_register_machine(&pc_machine_v0_10);
> -    qemu_register_machine(&isapc_machine);
> +    qemu_register_pc_machine(&pc_i440fx_machine_v2_0);
> +    qemu_register_pc_machine(&pc_i440fx_machine_v1_7);
> +    qemu_register_pc_machine(&pc_i440fx_machine_v1_6);
> +    qemu_register_pc_machine(&pc_i440fx_machine_v1_5);
> +    qemu_register_pc_machine(&pc_i440fx_machine_v1_4);
> +    qemu_register_pc_machine(&pc_machine_v1_3);
> +    qemu_register_pc_machine(&pc_machine_v1_2);
> +    qemu_register_pc_machine(&pc_machine_v1_1);
> +    qemu_register_pc_machine(&pc_machine_v1_0);
> +    qemu_register_pc_machine(&pc_machine_v0_15);
> +    qemu_register_pc_machine(&pc_machine_v0_14);
> +    qemu_register_pc_machine(&pc_machine_v0_13);
> +    qemu_register_pc_machine(&pc_machine_v0_12);
> +    qemu_register_pc_machine(&pc_machine_v0_11);
> +    qemu_register_pc_machine(&pc_machine_v0_10);
> +    qemu_register_pc_machine(&isapc_machine);
>  #ifdef CONFIG_XEN
> -    qemu_register_machine(&xenfv_machine);
> +    qemu_register_pc_machine(&xenfv_machine);
>  #endif
>  }
>  
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index c844dc2..16b4daa 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -358,11 +358,11 @@ static QEMUMachine pc_q35_machine_v1_4 = {
>  
>  static void pc_q35_machine_init(void)
>  {
> -    qemu_register_machine(&pc_q35_machine_v2_0);
> -    qemu_register_machine(&pc_q35_machine_v1_7);
> -    qemu_register_machine(&pc_q35_machine_v1_6);
> -    qemu_register_machine(&pc_q35_machine_v1_5);
> -    qemu_register_machine(&pc_q35_machine_v1_4);
> +    qemu_register_pc_machine(&pc_q35_machine_v2_0);
> +    qemu_register_pc_machine(&pc_q35_machine_v1_7);
> +    qemu_register_pc_machine(&pc_q35_machine_v1_6);
> +    qemu_register_pc_machine(&pc_q35_machine_v1_5);
> +    qemu_register_pc_machine(&pc_q35_machine_v1_4);
>  }
>  
>  machine_init(pc_q35_machine_init);
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 9010246..a01a220 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -12,9 +12,23 @@
>  #include "qemu/bitmap.h"
>  #include "sysemu/sysemu.h"
>  #include "hw/pci/pci.h"
> +#include "hw/boards.h"
>  
>  #define HPET_INTCAP "hpet-intcap"
>  
> +struct PCMachineState {
> +    /*< private >*/
> +    MachineState parent_obj;
> +};
> +
> +typedef struct PCMachineState PCMachineState;
> +
> +#define TYPE_PC_MACHINE "abstract-pc-machine"
> +#define PC_MACHINE(obj) \
> +     OBJECT_CHECK(PCMachineState, (obj), TYPE_PC_MACHINE)
> +
> +void qemu_register_pc_machine(QEMUMachine *m);
> +
>  /* PC-style peripherals (also used by other machines).  */
>  
>  typedef struct PcPciInfo {

Re: [Qemu-devel] [RFC 3/8] pc: prepare PC for custom machine state

[Igor Mammedov]

On Sun, 23 Mar 2014 17:13:37 +0200
Marcel Apfelbaum <marcel.a@redhat.com> wrote:

> On Thu, 2014-03-20 at 16:01 +0100, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/i386/pc.c         |   26 ++++++++++++++++++++++++++
> >  hw/i386/pc_piix.c    |   34 +++++++++++++++++-----------------
> >  hw/i386/pc_q35.c     |   10 +++++-----
> >  include/hw/i386/pc.h |   14 ++++++++++++++
> >  4 files changed, 62 insertions(+), 22 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index e715a33..e0bc3a2 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1413,3 +1413,29 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
> >          gsi_state->ioapic_irq[i] = qdev_get_gpio_in(dev, i);
> >      }
> >  }
> > +
> > +void qemu_register_pc_machine(QEMUMachine *m)
> I am not so comfortable with this approach because
> every subsystem (e.g pc) will have to duplicate the
> "register machine" code until the conversion from
> QEMUMachine to MachineClass is over. (which I hope
> it will not take too much time)
There is no much one can do once leaf type is not directly
inherited from TYPE_MACHINE. This function would eventually
go away leaf machine types are converted to static type
registration.

> 
> I propose a patch already in the list which does that
> automatically by moving this logic into hw/core/machine.c .
> In this way it will be much less code "touched" during conversion. 
> 
> Andreas, did you have anything against the usage of 'class_base_init' ?
> 
> The patch is:
> [PATCH 1/3] hw/machine: move QEMUMachine assignment into the core machine
> http://lists.gnu.org/archive/html/qemu-devel/2014-03/msg02151.html
Yep, this allows to drop 2/8 and works as expected.

> 
> > +{
> > +    TypeInfo ti = {
> > +        .name       = g_strconcat(m->name, TYPE_MACHINE_SUFFIX, NULL),
> This leads to a small memory leak, I missed that myself :(
> Here is a fixed version:
> [PATCHv2] vl.c: Fix memory leak in qemu_register_machine
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg222800.html
Thanks

> 
> Thanks,
> Marcel
> 
> > +        .parent     = TYPE_PC_MACHINE,
> > +        .class_init = machine_class_init,
> > +        .class_data = (void *)m,
> > +    };
> > +
> > +    type_register(&ti);
> > +}
> > +
> > +static const TypeInfo pc_machine_info = {
> > +    .name = TYPE_PC_MACHINE,
> > +    .parent = TYPE_MACHINE,
> > +    .abstract = true,
> > +    .instance_size = sizeof(PCMachineState),
> > +};
> > +
> > +static void pc_machine_register_types(void)
> > +{
> > +    type_register_static(&pc_machine_info);
> > +}
> > +
> > +type_init(pc_machine_register_types)
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index 7930a26..97df43e 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -817,24 +817,24 @@ static QEMUMachine xenfv_machine = {
> >  
> >  static void pc_machine_init(void)
> >  {
> > -    qemu_register_machine(&pc_i440fx_machine_v2_0);
> > -    qemu_register_machine(&pc_i440fx_machine_v1_7);
> > -    qemu_register_machine(&pc_i440fx_machine_v1_6);
> > -    qemu_register_machine(&pc_i440fx_machine_v1_5);
> > -    qemu_register_machine(&pc_i440fx_machine_v1_4);
> > -    qemu_register_machine(&pc_machine_v1_3);
> > -    qemu_register_machine(&pc_machine_v1_2);
> > -    qemu_register_machine(&pc_machine_v1_1);
> > -    qemu_register_machine(&pc_machine_v1_0);
> > -    qemu_register_machine(&pc_machine_v0_15);
> > -    qemu_register_machine(&pc_machine_v0_14);
> > -    qemu_register_machine(&pc_machine_v0_13);
> > -    qemu_register_machine(&pc_machine_v0_12);
> > -    qemu_register_machine(&pc_machine_v0_11);
> > -    qemu_register_machine(&pc_machine_v0_10);
> > -    qemu_register_machine(&isapc_machine);
> > +    qemu_register_pc_machine(&pc_i440fx_machine_v2_0);
> > +    qemu_register_pc_machine(&pc_i440fx_machine_v1_7);
> > +    qemu_register_pc_machine(&pc_i440fx_machine_v1_6);
> > +    qemu_register_pc_machine(&pc_i440fx_machine_v1_5);
> > +    qemu_register_pc_machine(&pc_i440fx_machine_v1_4);
> > +    qemu_register_pc_machine(&pc_machine_v1_3);
> > +    qemu_register_pc_machine(&pc_machine_v1_2);
> > +    qemu_register_pc_machine(&pc_machine_v1_1);
> > +    qemu_register_pc_machine(&pc_machine_v1_0);
> > +    qemu_register_pc_machine(&pc_machine_v0_15);
> > +    qemu_register_pc_machine(&pc_machine_v0_14);
> > +    qemu_register_pc_machine(&pc_machine_v0_13);
> > +    qemu_register_pc_machine(&pc_machine_v0_12);
> > +    qemu_register_pc_machine(&pc_machine_v0_11);
> > +    qemu_register_pc_machine(&pc_machine_v0_10);
> > +    qemu_register_pc_machine(&isapc_machine);
> >  #ifdef CONFIG_XEN
> > -    qemu_register_machine(&xenfv_machine);
> > +    qemu_register_pc_machine(&xenfv_machine);
> >  #endif
> >  }
> >  
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index c844dc2..16b4daa 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -358,11 +358,11 @@ static QEMUMachine pc_q35_machine_v1_4 = {
> >  
> >  static void pc_q35_machine_init(void)
> >  {
> > -    qemu_register_machine(&pc_q35_machine_v2_0);
> > -    qemu_register_machine(&pc_q35_machine_v1_7);
> > -    qemu_register_machine(&pc_q35_machine_v1_6);
> > -    qemu_register_machine(&pc_q35_machine_v1_5);
> > -    qemu_register_machine(&pc_q35_machine_v1_4);
> > +    qemu_register_pc_machine(&pc_q35_machine_v2_0);
> > +    qemu_register_pc_machine(&pc_q35_machine_v1_7);
> > +    qemu_register_pc_machine(&pc_q35_machine_v1_6);
> > +    qemu_register_pc_machine(&pc_q35_machine_v1_5);
> > +    qemu_register_pc_machine(&pc_q35_machine_v1_4);
> >  }
> >  
> >  machine_init(pc_q35_machine_init);
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index 9010246..a01a220 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -12,9 +12,23 @@
> >  #include "qemu/bitmap.h"
> >  #include "sysemu/sysemu.h"
> >  #include "hw/pci/pci.h"
> > +#include "hw/boards.h"
> >  
> >  #define HPET_INTCAP "hpet-intcap"
> >  
> > +struct PCMachineState {
> > +    /*< private >*/
> > +    MachineState parent_obj;
> > +};
> > +
> > +typedef struct PCMachineState PCMachineState;
> > +
> > +#define TYPE_PC_MACHINE "abstract-pc-machine"
> > +#define PC_MACHINE(obj) \
> > +     OBJECT_CHECK(PCMachineState, (obj), TYPE_PC_MACHINE)
> > +
> > +void qemu_register_pc_machine(QEMUMachine *m);
> > +
> >  /* PC-style peripherals (also used by other machines).  */
> >  
> >  typedef struct PcPciInfo {
> 
> 
> 

Re: [Qemu-devel] [RFC 3/8] pc: prepare PC for custom machine state

[Marcel Apfelbaum]

On Mon, 2014-03-24 at 11:34 +0100, Igor Mammedov wrote:
> On Sun, 23 Mar 2014 17:13:37 +0200
> Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> 
> > On Thu, 2014-03-20 at 16:01 +0100, Igor Mammedov wrote:
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  hw/i386/pc.c         |   26 ++++++++++++++++++++++++++
> > >  hw/i386/pc_piix.c    |   34 +++++++++++++++++-----------------
> > >  hw/i386/pc_q35.c     |   10 +++++-----
> > >  include/hw/i386/pc.h |   14 ++++++++++++++
> > >  4 files changed, 62 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index e715a33..e0bc3a2 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -1413,3 +1413,29 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
> > >          gsi_state->ioapic_irq[i] = qdev_get_gpio_in(dev, i);
> > >      }
> > >  }
> > > +
> > > +void qemu_register_pc_machine(QEMUMachine *m)
> > I am not so comfortable with this approach because
> > every subsystem (e.g pc) will have to duplicate the
> > "register machine" code until the conversion from
> > QEMUMachine to MachineClass is over. (which I hope
> > it will not take too much time)
> There is no much one can do once leaf type is not directly
> inherited from TYPE_MACHINE. This function would eventually
> go away leaf machine types are converted to static type
> registration.
> 
> > 
> > I propose a patch already in the list which does that
> > automatically by moving this logic into hw/core/machine.c .
> > In this way it will be much less code "touched" during conversion. 
> > 
> > Andreas, did you have anything against the usage of 'class_base_init' ?
> > 
> > The patch is:
> > [PATCH 1/3] hw/machine: move QEMUMachine assignment into the core machine
> > http://lists.gnu.org/archive/html/qemu-devel/2014-03/msg02151.html
> Yep, this allows to drop 2/8 and works as expected.
However Andreas does not like it, because the base class init depends
on the derived class data. Anyway, I hope I'll have a version soon without QemuMachine.

Thanks,
Marcel

> 
> > 
> > > +{
> > > +    TypeInfo ti = {
> > > +        .name       = g_strconcat(m->name, TYPE_MACHINE_SUFFIX, NULL),
> > This leads to a small memory leak, I missed that myself :(
> > Here is a fixed version:
> > [PATCHv2] vl.c: Fix memory leak in qemu_register_machine
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg222800.html
> Thanks
> 
> > 
> > Thanks,
> > Marcel
> > 
> > > +        .parent     = TYPE_PC_MACHINE,
> > > +        .class_init = machine_class_init,
> > > +        .class_data = (void *)m,
> > > +    };
> > > +
> > > +    type_register(&ti);
> > > +}
> > > +
> > > +static const TypeInfo pc_machine_info = {
> > > +    .name = TYPE_PC_MACHINE,
> > > +    .parent = TYPE_MACHINE,
> > > +    .abstract = true,
> > > +    .instance_size = sizeof(PCMachineState),
> > > +};
> > > +
> > > +static void pc_machine_register_types(void)
> > > +{
> > > +    type_register_static(&pc_machine_info);
> > > +}
> > > +
> > > +type_init(pc_machine_register_types)
> > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > > index 7930a26..97df43e 100644
> > > --- a/hw/i386/pc_piix.c
> > > +++ b/hw/i386/pc_piix.c
> > > @@ -817,24 +817,24 @@ static QEMUMachine xenfv_machine = {
> > >  
> > >  static void pc_machine_init(void)
> > >  {
> > > -    qemu_register_machine(&pc_i440fx_machine_v2_0);
> > > -    qemu_register_machine(&pc_i440fx_machine_v1_7);
> > > -    qemu_register_machine(&pc_i440fx_machine_v1_6);
> > > -    qemu_register_machine(&pc_i440fx_machine_v1_5);
> > > -    qemu_register_machine(&pc_i440fx_machine_v1_4);
> > > -    qemu_register_machine(&pc_machine_v1_3);
> > > -    qemu_register_machine(&pc_machine_v1_2);
> > > -    qemu_register_machine(&pc_machine_v1_1);
> > > -    qemu_register_machine(&pc_machine_v1_0);
> > > -    qemu_register_machine(&pc_machine_v0_15);
> > > -    qemu_register_machine(&pc_machine_v0_14);
> > > -    qemu_register_machine(&pc_machine_v0_13);
> > > -    qemu_register_machine(&pc_machine_v0_12);
> > > -    qemu_register_machine(&pc_machine_v0_11);
> > > -    qemu_register_machine(&pc_machine_v0_10);
> > > -    qemu_register_machine(&isapc_machine);
> > > +    qemu_register_pc_machine(&pc_i440fx_machine_v2_0);
> > > +    qemu_register_pc_machine(&pc_i440fx_machine_v1_7);
> > > +    qemu_register_pc_machine(&pc_i440fx_machine_v1_6);
> > > +    qemu_register_pc_machine(&pc_i440fx_machine_v1_5);
> > > +    qemu_register_pc_machine(&pc_i440fx_machine_v1_4);
> > > +    qemu_register_pc_machine(&pc_machine_v1_3);
> > > +    qemu_register_pc_machine(&pc_machine_v1_2);
> > > +    qemu_register_pc_machine(&pc_machine_v1_1);
> > > +    qemu_register_pc_machine(&pc_machine_v1_0);
> > > +    qemu_register_pc_machine(&pc_machine_v0_15);
> > > +    qemu_register_pc_machine(&pc_machine_v0_14);
> > > +    qemu_register_pc_machine(&pc_machine_v0_13);
> > > +    qemu_register_pc_machine(&pc_machine_v0_12);
> > > +    qemu_register_pc_machine(&pc_machine_v0_11);
> > > +    qemu_register_pc_machine(&pc_machine_v0_10);
> > > +    qemu_register_pc_machine(&isapc_machine);
> > >  #ifdef CONFIG_XEN
> > > -    qemu_register_machine(&xenfv_machine);
> > > +    qemu_register_pc_machine(&xenfv_machine);
> > >  #endif
> > >  }
> > >  
> > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > > index c844dc2..16b4daa 100644
> > > --- a/hw/i386/pc_q35.c
> > > +++ b/hw/i386/pc_q35.c
> > > @@ -358,11 +358,11 @@ static QEMUMachine pc_q35_machine_v1_4 = {
> > >  
> > >  static void pc_q35_machine_init(void)
> > >  {
> > > -    qemu_register_machine(&pc_q35_machine_v2_0);
> > > -    qemu_register_machine(&pc_q35_machine_v1_7);
> > > -    qemu_register_machine(&pc_q35_machine_v1_6);
> > > -    qemu_register_machine(&pc_q35_machine_v1_5);
> > > -    qemu_register_machine(&pc_q35_machine_v1_4);
> > > +    qemu_register_pc_machine(&pc_q35_machine_v2_0);
> > > +    qemu_register_pc_machine(&pc_q35_machine_v1_7);
> > > +    qemu_register_pc_machine(&pc_q35_machine_v1_6);
> > > +    qemu_register_pc_machine(&pc_q35_machine_v1_5);
> > > +    qemu_register_pc_machine(&pc_q35_machine_v1_4);
> > >  }
> > >  
> > >  machine_init(pc_q35_machine_init);
> > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > index 9010246..a01a220 100644
> > > --- a/include/hw/i386/pc.h
> > > +++ b/include/hw/i386/pc.h
> > > @@ -12,9 +12,23 @@
> > >  #include "qemu/bitmap.h"
> > >  #include "sysemu/sysemu.h"
> > >  #include "hw/pci/pci.h"
> > > +#include "hw/boards.h"
> > >  
> > >  #define HPET_INTCAP "hpet-intcap"
> > >  
> > > +struct PCMachineState {
> > > +    /*< private >*/
> > > +    MachineState parent_obj;
> > > +};
> > > +
> > > +typedef struct PCMachineState PCMachineState;
> > > +
> > > +#define TYPE_PC_MACHINE "abstract-pc-machine"
> > > +#define PC_MACHINE(obj) \
> > > +     OBJECT_CHECK(PCMachineState, (obj), TYPE_PC_MACHINE)
> > > +
> > > +void qemu_register_pc_machine(QEMUMachine *m);
> > > +
> > >  /* PC-style peripherals (also used by other machines).  */
> > >  
> > >  typedef struct PcPciInfo {
> > 
> > 
> > 
> 

Re: [Qemu-devel] [RFC 3/8] pc: prepare PC for custom machine state

[Andreas Färber]

Am 23.03.2014 16:13, schrieb Marcel Apfelbaum:
> On Thu, 2014-03-20 at 16:01 +0100, Igor Mammedov wrote:
>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>> ---
>>  hw/i386/pc.c         |   26 ++++++++++++++++++++++++++
>>  hw/i386/pc_piix.c    |   34 +++++++++++++++++-----------------
>>  hw/i386/pc_q35.c     |   10 +++++-----
>>  include/hw/i386/pc.h |   14 ++++++++++++++
>>  4 files changed, 62 insertions(+), 22 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index e715a33..e0bc3a2 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1413,3 +1413,29 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
>>          gsi_state->ioapic_irq[i] = qdev_get_gpio_in(dev, i);
>>      }
>>  }
>> +
>> +void qemu_register_pc_machine(QEMUMachine *m)
> I am not so comfortable with this approach because
> every subsystem (e.g pc) will have to duplicate the
> "register machine" code until the conversion from
> QEMUMachine to MachineClass is over. (which I hope
> it will not take too much time)
> 
> I propose a patch already in the list which does that
> automatically by moving this logic into hw/core/machine.c .
> In this way it will be much less code "touched" during conversion. 
> 
> Andreas, did you have anything against the usage of 'class_base_init' ?

Yes, I do, .class_base_init is wrong for this, it would be .class_init;
but please avoid making a base class' .class_init (or .class_base_init)
depend on a subtype specifying .class_data.

I believe I asked you to post patches that finish your conversion so
that MachineClass no longer needs this pointer to QEMUMachine and
actually uses the fields you already prepared. In my mind the next
logical step of QOM'ification is to have each machine specify what is
now in a QEMUMachine struct in its own type's class_init, then there is
no duplication of such a general assignment any more.

Since this patch is for one machine only, I would much prefer to have
the PC duplicate the class_init like we did for sPAPR machine
(hw/ppc/spapr.c) over exposing the class_init across files - if this
series cannot wait to be ordered after the machine series.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [RFC 3/8] pc: prepare PC for custom machine state

[Michael S. Tsirkin]

On Mon, Mar 24, 2014 at 11:52:45AM +0100, Andreas Färber wrote:
> Am 23.03.2014 16:13, schrieb Marcel Apfelbaum:
> > On Thu, 2014-03-20 at 16:01 +0100, Igor Mammedov wrote:
> >> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >> ---
> >>  hw/i386/pc.c         |   26 ++++++++++++++++++++++++++
> >>  hw/i386/pc_piix.c    |   34 +++++++++++++++++-----------------
> >>  hw/i386/pc_q35.c     |   10 +++++-----
> >>  include/hw/i386/pc.h |   14 ++++++++++++++
> >>  4 files changed, 62 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >> index e715a33..e0bc3a2 100644
> >> --- a/hw/i386/pc.c
> >> +++ b/hw/i386/pc.c
> >> @@ -1413,3 +1413,29 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
> >>          gsi_state->ioapic_irq[i] = qdev_get_gpio_in(dev, i);
> >>      }
> >>  }
> >> +
> >> +void qemu_register_pc_machine(QEMUMachine *m)
> > I am not so comfortable with this approach because
> > every subsystem (e.g pc) will have to duplicate the
> > "register machine" code until the conversion from
> > QEMUMachine to MachineClass is over. (which I hope
> > it will not take too much time)
> > 
> > I propose a patch already in the list which does that
> > automatically by moving this logic into hw/core/machine.c .
> > In this way it will be much less code "touched" during conversion. 
> > 
> > Andreas, did you have anything against the usage of 'class_base_init' ?
> 
> Yes, I do, .class_base_init is wrong for this, it would be .class_init;
> but please avoid making a base class' .class_init (or .class_base_init)
> depend on a subtype specifying .class_data.
> 
> I believe I asked you to post patches that finish your conversion so
> that MachineClass no longer needs this pointer to QEMUMachine and
> actually uses the fields you already prepared. In my mind the next
> logical step of QOM'ification is to have each machine specify what is
> now in a QEMUMachine struct in its own type's class_init, then there is
> no duplication of such a general assignment any more.
> 
> Since this patch is for one machine only, I would much prefer to have
> the PC duplicate the class_init like we did for sPAPR machine
> (hw/ppc/spapr.c) over exposing the class_init across files - if this
> series cannot wait to be ordered after the machine series.
> 
> Regards,
> Andreas

Ok IIUC this is exactly what Igor did so you ack the original patch?

> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [RFC 3/8] pc: prepare PC for custom machine state

[Andreas Färber]

Am 24.03.2014 12:20, schrieb Michael S. Tsirkin:
> On Mon, Mar 24, 2014 at 11:52:45AM +0100, Andreas Färber wrote:
>> Am 23.03.2014 16:13, schrieb Marcel Apfelbaum:
>>> On Thu, 2014-03-20 at 16:01 +0100, Igor Mammedov wrote:
>>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>>> ---
>>>>  hw/i386/pc.c         |   26 ++++++++++++++++++++++++++
>>>>  hw/i386/pc_piix.c    |   34 +++++++++++++++++-----------------
>>>>  hw/i386/pc_q35.c     |   10 +++++-----
>>>>  include/hw/i386/pc.h |   14 ++++++++++++++
>>>>  4 files changed, 62 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>>> index e715a33..e0bc3a2 100644
>>>> --- a/hw/i386/pc.c
>>>> +++ b/hw/i386/pc.c
>>>> @@ -1413,3 +1413,29 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
>>>>          gsi_state->ioapic_irq[i] = qdev_get_gpio_in(dev, i);
>>>>      }
>>>>  }
>>>> +
>>>> +void qemu_register_pc_machine(QEMUMachine *m)
>>> I am not so comfortable with this approach because
>>> every subsystem (e.g pc) will have to duplicate the
>>> "register machine" code until the conversion from
>>> QEMUMachine to MachineClass is over. (which I hope
>>> it will not take too much time)
>>>
>>> I propose a patch already in the list which does that
>>> automatically by moving this logic into hw/core/machine.c .
>>> In this way it will be much less code "touched" during conversion. 
>>>
>>> Andreas, did you have anything against the usage of 'class_base_init' ?
>>
>> Yes, I do, .class_base_init is wrong for this, it would be .class_init;
>> but please avoid making a base class' .class_init (or .class_base_init)
>> depend on a subtype specifying .class_data.
>>
>> I believe I asked you to post patches that finish your conversion so
>> that MachineClass no longer needs this pointer to QEMUMachine and
>> actually uses the fields you already prepared. In my mind the next
>> logical step of QOM'ification is to have each machine specify what is
>> now in a QEMUMachine struct in its own type's class_init, then there is
>> no duplication of such a general assignment any more.
>>
>> Since this patch is for one machine only, I would much prefer to have
>> the PC duplicate the class_init like we did for sPAPR machine
>> (hw/ppc/spapr.c) over exposing the class_init across files - if this
>> series cannot wait to be ordered after the machine series.
>>
>> Regards,
>> Andreas
> 
> Ok IIUC this is exactly what Igor did so you ack the original patch?

No, it doesn't? It is a nack for 2/8. Nothing wrong with 3/8 in general
though, it can be rebased, given that this seems clearly 2.1 material.
Idea is to minimize changes to code outside of PC to not collide with
the refactoring. If Marcel (re)sends those changes soon, I can queue
them on qom-next and Igor can rebase on that for convenience.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [RFC 3/8] pc: prepare PC for custom machine state

[Marcel Apfelbaum]

On Mon, 2014-03-24 at 11:52 +0100, Andreas Färber wrote:
> Am 23.03.2014 16:13, schrieb Marcel Apfelbaum:
> > On Thu, 2014-03-20 at 16:01 +0100, Igor Mammedov wrote:
> >> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >> ---
> >>  hw/i386/pc.c         |   26 ++++++++++++++++++++++++++
> >>  hw/i386/pc_piix.c    |   34 +++++++++++++++++-----------------
> >>  hw/i386/pc_q35.c     |   10 +++++-----
> >>  include/hw/i386/pc.h |   14 ++++++++++++++
> >>  4 files changed, 62 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >> index e715a33..e0bc3a2 100644
> >> --- a/hw/i386/pc.c
> >> +++ b/hw/i386/pc.c
> >> @@ -1413,3 +1413,29 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
> >>          gsi_state->ioapic_irq[i] = qdev_get_gpio_in(dev, i);
> >>      }
> >>  }
> >> +
> >> +void qemu_register_pc_machine(QEMUMachine *m)
> > I am not so comfortable with this approach because
> > every subsystem (e.g pc) will have to duplicate the
> > "register machine" code until the conversion from
> > QEMUMachine to MachineClass is over. (which I hope
> > it will not take too much time)
> > 
> > I propose a patch already in the list which does that
> > automatically by moving this logic into hw/core/machine.c .
> > In this way it will be much less code "touched" during conversion. 
> > 
> > Andreas, did you have anything against the usage of 'class_base_init' ?
> 
> Yes, I do, .class_base_init is wrong for this, it would be .class_init;
> but please avoid making a base class' .class_init (or .class_base_init)
> depend on a subtype specifying .class_data.
Got it, thanks

> 
> I believe I asked you to post patches that finish your conversion so
> that MachineClass no longer needs this pointer to QEMUMachine and
> actually uses the fields you already prepared. In my mind the next
> logical step of QOM'ification is to have each machine specify what is
> now in a QEMUMachine struct in its own type's class_init, then there is
> no duplication of such a general assignment any more.
Yes, this is exactly what I am working on right now, I am going to
work on both 'QemuOpts -> MachineState properties'  and QEMUMachine
elimination.

Sorry, I've been hold back by other issues until now :)
Thanks,
Marcel

 
> 
> Since this patch is for one machine only, I would much prefer to have
> the PC duplicate the class_init like we did for sPAPR machine
> (hw/ppc/spapr.c) over exposing the class_init across files - if this
> series cannot wait to be ordered after the machine series.
> 
> Regards,
> Andreas
> 

[Qemu-devel] [RFC 4/8] qdev: link based hotplug

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/core/hotplug.c      |   30 ++++++++++++++++++++++++++++++
 hw/core/qdev.c         |   15 +++++++++++++++
 include/hw/hotplug.h   |    2 ++
 include/hw/qdev-core.h |    6 ++++++
 4 files changed, 53 insertions(+), 0 deletions(-)

diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
index 5573d9d..1a13ca3 100644
--- a/hw/core/hotplug.c
+++ b/hw/core/hotplug.c
@@ -11,6 +11,36 @@
  */
 #include "hw/hotplug.h"
 #include "qemu/module.h"
+#include <string.h>
+
+HotplugHandler *hotplug_handler_get_from_path(const char *path)
+{
+    Object *obj;
+    const char *sep;
+    bool ambiguous = false;
+    HotplugHandler *hh = NULL;
+    char *current_path = g_strdup(path);
+
+    while ((sep = strrchr(current_path, '/'))) {
+        char *old_path = current_path;
+        if (!sep || sep == current_path) {
+            break;
+        }
+
+        current_path = g_strndup(current_path, sep - current_path);
+        g_free(old_path);
+
+        obj = object_resolve_path(current_path, &ambiguous);
+        g_assert(!ambiguous);
+        hh = (HotplugHandler *)object_dynamic_cast(obj, TYPE_HOTPLUG_HANDLER);
+        if (hh != NULL) {
+            break;
+        }
+    }
+
+    g_free(current_path);
+    return hh;
+}
 
 void hotplug_handler_plug(HotplugHandler *plug_handler,
                           DeviceState *plugged_dev,
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 9f0a522..a8d6eea 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -743,6 +743,21 @@ static void device_set_realized(Object *obj, bool value, Error **err)
             local_err == NULL) {
             hotplug_handler_plug(dev->parent_bus->hotplug_handler,
                                  dev, &local_err);
+        } else if (dc->hotplug_path != NULL && local_err == NULL) {
+            HotplugHandler *hotplug_ctrl;
+            char *target_name;
+            char *path = dc->hotplug_path(dev);
+
+            hotplug_ctrl = hotplug_handler_get_from_path(path);
+            g_assert(hotplug_ctrl);
+
+            target_name = g_strdup(strrchr(path, '/') + 1);
+            object_property_set_link(OBJECT(hotplug_ctrl), OBJECT(dev),
+                                     target_name, &local_err);
+            g_free(target_name);
+            g_free(path);
+
+            hotplug_handler_plug(hotplug_ctrl, dev, &local_err);
         }
 
         if (qdev_get_vmsd(dev) && local_err == NULL) {
diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
index a6533cb..7a3815e 100644
--- a/include/hw/hotplug.h
+++ b/include/hw/hotplug.h
@@ -75,4 +75,6 @@ void hotplug_handler_plug(HotplugHandler *plug_handler,
 void hotplug_handler_unplug(HotplugHandler *plug_handler,
                             DeviceState *plugged_dev,
                             Error **errp);
+
+HotplugHandler *hotplug_handler_get_from_path(const char *path);
 #endif
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index dbe473c..6cd936d 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -116,6 +116,12 @@ typedef struct DeviceClass {
     bool cannot_instantiate_with_device_add_yet;
     bool hotpluggable;
 
+    /*
+     * Returns path to link<> that should be set/unset on dev hotplug.
+     * Used for link based bussless devices hotplug.
+     */
+    char* (*hotplug_path)(DeviceState *dev);
+
     /* callbacks */
     void (*reset)(DeviceState *dev);
     DeviceRealize realize;
-- 
1.7.1

Re: [Qemu-devel] [RFC 4/8] qdev: link based hotplug

[Paolo Bonzini]

Il 20/03/2014 16:01, Igor Mammedov ha scritto:
> +    /*
> +     * Returns path to link<> that should be set/unset on dev hotplug.
> +     * Used for link based bussless devices hotplug.
> +     */
> +    char* (*hotplug_path)(DeviceState *dev);
> +

What about just looking up on the QOM tree until you find a 
HotplugHandler, if the device doesn't have a bus or the bus doesn't have 
a hotplug handler link itself?  This is similar to how FWPathProvider works.

Paolo

Re: [Qemu-devel] [RFC 4/8] qdev: link based hotplug

[Igor Mammedov]

On Thu, 20 Mar 2014 17:12:17 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 20/03/2014 16:01, Igor Mammedov ha scritto:
> > +    /*
> > +     * Returns path to link<> that should be set/unset on dev hotplug.
> > +     * Used for link based bussless devices hotplug.
> > +     */
> > +    char* (*hotplug_path)(DeviceState *dev);
> > +
> 
> What about just looking up on the QOM tree until you find a 
> HotplugHandler, if the device doesn't have a bus or the bus doesn't have 
> a hotplug handler link itself?  This is similar to how FWPathProvider works.

it does so "hotplug_handler_get_from_path()",
above just provides option to specify lookup path. See 6/8 where PC board
allocates links and sets its own board specific path for generic DimmDevice.

> 
> Paolo

Re: [Qemu-devel] [RFC 4/8] qdev: link based hotplug

[Paolo Bonzini]

Il 20/03/2014 17:20, Igor Mammedov ha scritto:
>> >
>> > What about just looking up on the QOM tree until you find a
>> > HotplugHandler, if the device doesn't have a bus or the bus doesn't have
>> > a hotplug handler link itself?  This is similar to how FWPathProvider works.
> it does so "hotplug_handler_get_from_path()",
> above just provides option to specify lookup path. See 6/8 where PC board
> allocates links and sets its own board specific path for generic DimmDevice.

Yeah, but I'm not sure why you need the links.  Why can't you just start 
from the canonical path, such as /machine/peripheral/dimm-0 for -device 
dimm,id=dimm-0, and walk up from there?

Paolo

Re: [Qemu-devel] [RFC 4/8] qdev: link based hotplug

[Igor Mammedov]

On Thu, 20 Mar 2014 19:03:57 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 20/03/2014 17:20, Igor Mammedov ha scritto:
> >> >
> >> > What about just looking up on the QOM tree until you find a
> >> > HotplugHandler, if the device doesn't have a bus or the bus doesn't have
> >> > a hotplug handler link itself?  This is similar to how FWPathProvider works.
> > it does so "hotplug_handler_get_from_path()",
> > above just provides option to specify lookup path. See 6/8 where PC board
> > allocates links and sets its own board specific path for generic DimmDevice.
> 
> Yeah, but I'm not sure why you need the links.  Why can't you just start 
> from the canonical path, such as /machine/peripheral/dimm-0 for -device 
> dimm,id=dimm-0, and walk up from there?
That would work in this particular case since /machine is hotplug handler,
but in generic case hotplug handler can be located somewhere else in QOM tree
i.e. it isn't a parent of hotplugged device.

Allowing to customize lookup path via DeviceClass.hotplug_path() makes
wiring hotplug handler flexible. For example:
 * A target that decides to use DimmDevice cloud have hotplug handler
   located at /machine/peripheral/ec_foo, walking down from
   /machine/peripheral/dimm-0 or /machine/unassigned/dimm-0 (in hotplug case)
   won't allow to find  hotplug handler. But overriding DeviceClass.hotplug_path()
   the target machine could provide means to locate necessary hotplug handler

Maybe setting link<> is over-engineered and should be dropped for now,
while still keeping DeviceClass.hotplug_path() as a means to provide custom
hotplug hander path.


BTW not related to hotplug but why I used link<>s:

I've added link<>s as an attempt to visualize Andreas' idea to use them for
hotplug and mgmt. It has it's own benefits if we try to provide more or
less uniform QOM interface view for management. What I have in mind is that
we could have tree like this:
 /machine/node[...]/dimm[...]
                   /cpu[...]/core[...]/thread[...]

where leaves are link<>s which are prebuilt at startup and set when device
is added. It provides an easy to enumerate interface for mgmt and also
gives us a quite informative path that encodes topology and later
we could use it instead of custom properties. For example:

  device_add x86cpu,path=/machine/node[1]/cpu[0]/core[3]/thread[2]
vs
  device_add x86cpu,apic-id=[who knows how it's calculated]

or
  device_add dimm,path=/machine/node[0]/dimm[5]
vs
  device_add dimm,node=0,slot=5

i.e. being added device could decode all needed information from above
provided path instead of creating a bunch of custom properties.

> 
> Paolo

Re: [Qemu-devel] [RFC 4/8] qdev: link based hotplug

[Paolo Bonzini]

Il 21/03/2014 11:35, Igor Mammedov ha scritto:
> On Thu, 20 Mar 2014 19:03:57 +0100
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> Il 20/03/2014 17:20, Igor Mammedov ha scritto:
>>>>>
>>>>> What about just looking up on the QOM tree until you find a
>>>>> HotplugHandler, if the device doesn't have a bus or the bus doesn't have
>>>>> a hotplug handler link itself?  This is similar to how FWPathProvider works.
>>> it does so "hotplug_handler_get_from_path()",
>>> above just provides option to specify lookup path. See 6/8 where PC board
>>> allocates links and sets its own board specific path for generic DimmDevice.
>>
>> Yeah, but I'm not sure why you need the links.  Why can't you just start 
>> from the canonical path, such as /machine/peripheral/dimm-0 for -device 
>> dimm,id=dimm-0, and walk up from there?
> That would work in this particular case since /machine is hotplug handler,
> but in generic case hotplug handler can be located somewhere else in QOM tree
> i.e. it isn't a parent of hotplugged device.

Yes, it removes more flexibility than I thought.  Hotplugged devices 
are by definition at /machine/peripheral, so there's not much "walking 
up" that we can do.

However...

> Allowing to customize lookup path via DeviceClass.hotplug_path() makes
> wiring hotplug handler flexible. For example:
>  * A target that decides to use DimmDevice cloud have hotplug handler
>    located at /machine/peripheral/ec_foo, walking down from
>    /machine/peripheral/dimm-0 or /machine/unassigned/dimm-0 (in hotplug case)
>    won't allow to find  hotplug handler. But overriding DeviceClass.hotplug_path()
>    the target machine could provide means to locate necessary hotplug handler

... since this is meant for "monkeypatching" in the target machine 
(which we don't really do elsewhere, do we?), perhaps /machine itself 
could grow a get_hotplug_handler() or get_hotplug_path() method?

The machine object then can return itself if you want to implement 
HotplugHandler in /machine, or it could return the PM device, or some 
other controller.

Or even simpler (perhaps too simple) you could just check if /machine 
implements HotplugHandler if the hotplug device is busless.

> I've added link<>s as an attempt to visualize Andreas' idea to use them for
> hotplug and mgmt.

Yes, links for hotplug/management are a nice idea.  I think however
we're still in an early phase of that, and we've already made memory 
hotplug depend on a lot of infrastructure!

> leaves are link<>s which are prebuilt at startup and set when device
> is added. It provides an easy to enumerate interface for mgmt and also
> gives us a quite informative path that encodes topology and later
> we could use it instead of custom properties. For example:
> 
>   device_add x86cpu,path=/machine/node[1]/cpu[0]/core[3]/thread[2]
> vs
>   device_add x86cpu,apic-id=[who knows how it's calculated]
> 
> or
>   device_add dimm,path=/machine/node[0]/dimm[5]
> vs
>   device_add dimm,node=0,slot=5
> 
> i.e. being added device could decode all needed information from above
> provided path instead of creating a bunch of custom properties.

So "path" would mean "look for a link there and set it to myself".
In turn the link setter would take care of setting the device's
address based on the property name, as well as (if applicable)
adding the device on the bus.

It's a nice alternative to the bus+addr, and one we could consider to 
move device creation to -object.  Anthony in the past had mentioned 
something like

    -object dimm,id=foo
    -set machine/node[0].dimm[5]=foo
    -set dimm.realize=true

I think I like your proposal better.

We now have moved towards UserCreatable instead of setting magic 
properties manually, and I think "path" fits in the "UserCreatable" 
scheme better than "-set".

It could be extended to PCI, as basically a version of bus+addr
with QOM paths:

    -object e1000,id=e1000,path=/machine/pci[0]/slot[03.0]

Here is an example of configuring a PCIe switch with relative paths:

    -object ioh3420,id=port4,path=pci[0]/slot[1c.0]
    -object x3130-upstream,id=up,path=port4/pci[0]/slot[00.0],chassis=1
    -object xio3130-downstream,id=down0,multifunction=on,path=up/pci[0]/slot[00.0],chassis=2
    -object xio3130-downstream,id=down1,multifunction=on,path=up/pci[0]/slot[00.1],chassis=3
    -object e1000,id=e1000,path=down0/pci[0]/slot[00.0]

It's a bit verbose.  It doesn't allow for automatically attributing
slots within a bus, which is the main drawback compared to bus+addr.
Quite frankly I'm not sure I would like it as a user, even though it's
likely superior for management and for complicated configurations
such as the above PCIe example.

In the past we stalled on how to create the properties, since there is 
the problem of requiring pre-creation of links.  On SCSI you would have 
thousands of links.

Since interfaces are fancy now, perhaps we could use them here too 
(DynamicPropertyCreator?).  object_property_find would pass missing 
property names to the interface if implemented, and the object would 
use it to validate dynamic property names and create them dynamically.

Thanks for throwing up these ideas.  Even if we end up with a vastly 
simplified mechanism for memory hotplug, it's good to  get a fresh
view on old concepts!

Paolo

Re: [Qemu-devel] [RFC 4/8] qdev: link based hotplug

[Igor Mammedov]

On Fri, 21 Mar 2014 12:45:01 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 21/03/2014 11:35, Igor Mammedov ha scritto:
> > On Thu, 20 Mar 2014 19:03:57 +0100
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > 
> >> Il 20/03/2014 17:20, Igor Mammedov ha scritto:
> >>>>>
> >>>>> What about just looking up on the QOM tree until you find a
> >>>>> HotplugHandler, if the device doesn't have a bus or the bus doesn't have
> >>>>> a hotplug handler link itself?  This is similar to how FWPathProvider works.
> >>> it does so "hotplug_handler_get_from_path()",
> >>> above just provides option to specify lookup path. See 6/8 where PC board
> >>> allocates links and sets its own board specific path for generic DimmDevice.
> >>
> >> Yeah, but I'm not sure why you need the links.  Why can't you just start 
> >> from the canonical path, such as /machine/peripheral/dimm-0 for -device 
> >> dimm,id=dimm-0, and walk up from there?
> > That would work in this particular case since /machine is hotplug handler,
> > but in generic case hotplug handler can be located somewhere else in QOM tree
> > i.e. it isn't a parent of hotplugged device.
> 
> Yes, it removes more flexibility than I thought.  Hotplugged devices 
> are by definition at /machine/peripheral, so there's not much "walking 
> up" that we can do.
> 
> However...
> 
> > Allowing to customize lookup path via DeviceClass.hotplug_path() makes
> > wiring hotplug handler flexible. For example:
> >  * A target that decides to use DimmDevice cloud have hotplug handler
> >    located at /machine/peripheral/ec_foo, walking down from
> >    /machine/peripheral/dimm-0 or /machine/unassigned/dimm-0 (in hotplug case)
> >    won't allow to find  hotplug handler. But overriding DeviceClass.hotplug_path()
> >    the target machine could provide means to locate necessary hotplug handler
> 
> ... since this is meant for "monkeypatching" in the target machine 
> (which we don't really do elsewhere, do we?), perhaps /machine itself 
> could grow a get_hotplug_handler() or get_hotplug_path() method?
Ok for memory hotplug, I'll try to add and use get_hotplug_handler() method
to /machine as a minimal solution to the problem.

> 
> The machine object then can return itself if you want to implement 
> HotplugHandler in /machine, or it could return the PM device, or some 
> other controller.
> 
> Or even simpler (perhaps too simple) you could just check if /machine 
> implements HotplugHandler if the hotplug device is busless.
> 
> > I've added link<>s as an attempt to visualize Andreas' idea to use them for
> > hotplug and mgmt.
> 
> Yes, links for hotplug/management are a nice idea.  I think however
> we're still in an early phase of that, and we've already made memory 
> hotplug depend on a lot of infrastructure!
> 
> > leaves are link<>s which are prebuilt at startup and set when device
> > is added. It provides an easy to enumerate interface for mgmt and also
> > gives us a quite informative path that encodes topology and later
> > we could use it instead of custom properties. For example:
> > 
> >   device_add x86cpu,path=/machine/node[1]/cpu[0]/core[3]/thread[2]
> > vs
> >   device_add x86cpu,apic-id=[who knows how it's calculated]
> > 
> > or
> >   device_add dimm,path=/machine/node[0]/dimm[5]
> > vs
> >   device_add dimm,node=0,slot=5
> > 
> > i.e. being added device could decode all needed information from above
> > provided path instead of creating a bunch of custom properties.
> 
> So "path" would mean "look for a link there and set it to myself".
> In turn the link setter would take care of setting the device's
> address based on the property name, as well as (if applicable)
> adding the device on the bus.
> 
> It's a nice alternative to the bus+addr, and one we could consider to 
> move device creation to -object.  Anthony in the past had mentioned 
> something like
> 
>     -object dimm,id=foo
>     -set machine/node[0].dimm[5]=foo
>     -set dimm.realize=true
> 
> I think I like your proposal better.
> 
> We now have moved towards UserCreatable instead of setting magic 
> properties manually, and I think "path" fits in the "UserCreatable" 
> scheme better than "-set".
> 
> It could be extended to PCI, as basically a version of bus+addr
> with QOM paths:
> 
>     -object e1000,id=e1000,path=/machine/pci[0]/slot[03.0]
> 
> Here is an example of configuring a PCIe switch with relative paths:
> 
>     -object ioh3420,id=port4,path=pci[0]/slot[1c.0]
>     -object x3130-upstream,id=up,path=port4/pci[0]/slot[00.0],chassis=1
>     -object xio3130-downstream,id=down0,multifunction=on,path=up/pci[0]/slot[00.0],chassis=2
>     -object xio3130-downstream,id=down1,multifunction=on,path=up/pci[0]/slot[00.1],chassis=3
>     -object e1000,id=e1000,path=down0/pci[0]/slot[00.0]
> 
> It's a bit verbose.  It doesn't allow for automatically attributing
> slots within a bus, which is the main drawback compared to bus+addr.
> Quite frankly I'm not sure I would like it as a user, even though it's
> likely superior for management and for complicated configurations
> such as the above PCIe example.
> 
> In the past we stalled on how to create the properties, since there is 
> the problem of requiring pre-creation of links.  On SCSI you would have 
> thousands of links.
> 
> Since interfaces are fancy now, perhaps we could use them here too 
> (DynamicPropertyCreator?).  object_property_find would pass missing 
> property names to the interface if implemented, and the object would 
> use it to validate dynamic property names and create them dynamically.
> 
> Thanks for throwing up these ideas.  Even if we end up with a vastly 
> simplified mechanism for memory hotplug, it's good to  get a fresh
> view on old concepts!
> 
> Paolo

Re: [Qemu-devel] [RFC 4/8] qdev: link based hotplug

[Andreas Färber]

Am 21.03.2014 11:35, schrieb Igor Mammedov:
> BTW not related to hotplug but why I used link<>s:
> 
> I've added link<>s as an attempt to visualize Andreas' idea to use them for

Anthony's :)

> hotplug and mgmt. It has it's own benefits if we try to provide more or
> less uniform QOM interface view for management. What I have in mind is that
> we could have tree like this:
>  /machine/node[...]/dimm[...]
>                    /cpu[...]/core[...]/thread[...]
> 
> where leaves are link<>s which are prebuilt at startup and set when device
> is added. It provides an easy to enumerate interface for mgmt and also
> gives us a quite informative path that encodes topology and later
> we could use it instead of custom properties. For example:
> 
>   device_add x86cpu,path=/machine/node[1]/cpu[0]/core[3]/thread[2]
> vs
>   device_add x86cpu,apic-id=[who knows how it's calculated]

This still collides with what Anthony and me have been saying about CPU
hot-add: It should not happen on thread level. cpu-add covers the
current approach, but device_add should add a full socket and definitely
not in that /machine/node[n]/... path. You or someone replied on Paolo's
NUMA thread that this was only as an internal convenience for lookups,
now you're pushing this as user ABI. NACK to the latter.

I was hoping that we could let device_add auto-discover free link<>
slots like it does for buses today; having two places to link the same
object would complicate that, so we need to be careful in designing our
link<>s: Having /machine/node[0]/cpu[0] be a link<> would mean no second
link<> directly on /machine/cpu[0], i.e. the NUMA node acting as a
sub-container of the board; on the other hand having a link<> to the
thread CPUState from some NUMA-specific path outside of the composition
model would still be possible due to the different link<> type but
having cpu[0] be a link to the actual socket object rules out using the
same name for a NUMA-only container object as proposed.

> or
>   device_add dimm,path=/machine/node[0]/dimm[5]
> vs
>   device_add dimm,node=0,slot=5
> 
> i.e. being added device could decode all needed information from above
> provided path instead of creating a bunch of custom properties.

Hm, the advantage of having properties there would be better error
checking in terms of restricting paths. I'd be open to having both.

I do wonder in both cases if we should use paths relative to /machine to
avoid them cluttering the command line?

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [RFC 4/8] qdev: link based hotplug

[Michael S. Tsirkin]

On Mon, Mar 24, 2014 at 01:20:46PM +0100, Andreas Färber wrote:
> Am 21.03.2014 11:35, schrieb Igor Mammedov:
> > BTW not related to hotplug but why I used link<>s:
> > 
> > I've added link<>s as an attempt to visualize Andreas' idea to use them for
> 
> Anthony's :)
> 
> > hotplug and mgmt. It has it's own benefits if we try to provide more or
> > less uniform QOM interface view for management. What I have in mind is that
> > we could have tree like this:
> >  /machine/node[...]/dimm[...]
> >                    /cpu[...]/core[...]/thread[...]
> > 
> > where leaves are link<>s which are prebuilt at startup and set when device
> > is added. It provides an easy to enumerate interface for mgmt and also
> > gives us a quite informative path that encodes topology and later
> > we could use it instead of custom properties. For example:
> > 
> >   device_add x86cpu,path=/machine/node[1]/cpu[0]/core[3]/thread[2]
> > vs
> >   device_add x86cpu,apic-id=[who knows how it's calculated]
> 
> This still collides with what Anthony and me have been saying about CPU
> hot-add: It should not happen on thread level. cpu-add covers the
> current approach, but device_add should add a full socket and definitely
> not in that /machine/node[n]/... path. You or someone replied on Paolo's
> NUMA thread that this was only as an internal convenience for lookups,
> now you're pushing this as user ABI. NACK to the latter.
> 
> I was hoping that we could let device_add auto-discover free link<>
> slots like it does for buses today;

That's friendly for HMP but a huge maintainance pain.
In the end hardly anyone uses HMP for hotplug so I
don't think there's a lot of value in auto-discovery,
make management as explicit as possible.

> having two places to link the same
> object would complicate that, so we need to be careful in designing our
> link<>s: Having /machine/node[0]/cpu[0] be a link<> would mean no second
> link<> directly on /machine/cpu[0], i.e. the NUMA node acting as a
> sub-container of the board; on the other hand having a link<> to the
> thread CPUState from some NUMA-specific path outside of the composition
> model would still be possible due to the different link<> type but
> having cpu[0] be a link to the actual socket object rules out using the
> same name for a NUMA-only container object as proposed.
> 
> > or
> >   device_add dimm,path=/machine/node[0]/dimm[5]
> > vs
> >   device_add dimm,node=0,slot=5
> > 
> > i.e. being added device could decode all needed information from above
> > provided path instead of creating a bunch of custom properties.
> 
> Hm, the advantage of having properties there would be better error
> checking in terms of restricting paths. I'd be open to having both.
> 
> I do wonder in both cases if we should use paths relative to /machine to
> avoid them cluttering the command line?
> 
> Regards,
> Andreas
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [RFC 5/8] dimm: implement dimm device abstraction

[Igor Mammedov]

From: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>

Each hotplug-able memory slot is a DimmDevice. All DimmDevices are
attached to a new bus called DimmBus.

A hot-add operation for a DIMM:
- creates a new DimmDevice and attaches it to the DimmBus

Hotplug operations are done through normal device_add commands.
For migration case, all hotplugged DIMMs on source should be specified
on target's command line using '-device' option with properties set to
the same values as on target.

To simplify review, patch introduces only DimmDevice and DimmBus basic
QOM skeleton that will be extended by following patches to implement
actual memory hotplug and related functions.

Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v3:
  pc: compile memhotplug on i386 target too
v2:
  fix typo s/DimmBus/DimmDevice/ in doc comment
  s/klass/oc/;s/*/parent_obj/;a/gtk-doc markup/

dimm impl
---
 default-configs/i386-softmmu.mak   |    1 +
 default-configs/x86_64-softmmu.mak |    1 +
 hw/Makefile.objs                   |    1 +
 hw/mem/Makefile.objs               |    1 +
 hw/mem/dimm.c                      |  100 ++++++++++++++++++++++++++++++++++++
 include/hw/mem/dimm.h              |   58 +++++++++++++++++++++
 6 files changed, 162 insertions(+), 0 deletions(-)
 create mode 100644 hw/mem/Makefile.objs
 create mode 100644 hw/mem/dimm.c
 create mode 100644 include/hw/mem/dimm.h

diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
index 37ef90f..8e08841 100644
--- a/default-configs/i386-softmmu.mak
+++ b/default-configs/i386-softmmu.mak
@@ -44,3 +44,4 @@ CONFIG_APIC=y
 CONFIG_IOAPIC=y
 CONFIG_ICC_BUS=y
 CONFIG_PVPANIC=y
+CONFIG_MEM_HOTPLUG=y
diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
index 31bddce..66557ac 100644
--- a/default-configs/x86_64-softmmu.mak
+++ b/default-configs/x86_64-softmmu.mak
@@ -44,3 +44,4 @@ CONFIG_APIC=y
 CONFIG_IOAPIC=y
 CONFIG_ICC_BUS=y
 CONFIG_PVPANIC=y
+CONFIG_MEM_HOTPLUG=y
diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index d178b65..52a1464 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -29,6 +29,7 @@ devices-dirs-$(CONFIG_SOFTMMU) += usb/
 devices-dirs-$(CONFIG_VIRTIO) += virtio/
 devices-dirs-$(CONFIG_SOFTMMU) += watchdog/
 devices-dirs-$(CONFIG_SOFTMMU) += xen/
+devices-dirs-$(CONFIG_MEM_HOTPLUG) += mem/
 devices-dirs-y += core/
 common-obj-y += $(devices-dirs-y)
 obj-y += $(devices-dirs-y)
diff --git a/hw/mem/Makefile.objs b/hw/mem/Makefile.objs
new file mode 100644
index 0000000..7563ef5
--- /dev/null
+++ b/hw/mem/Makefile.objs
@@ -0,0 +1 @@
+common-obj-$(CONFIG_MEM_HOTPLUG) += dimm.o
diff --git a/hw/mem/dimm.c b/hw/mem/dimm.c
new file mode 100644
index 0000000..06e406c
--- /dev/null
+++ b/hw/mem/dimm.c
@@ -0,0 +1,100 @@
+/*
+ * Dimm device for Memory Hotplug
+ *
+ * Copyright ProfitBricks GmbH 2012
+ * Copyright (C) 2013 Red Hat Inc
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+
+#include "hw/mem/dimm.h"
+#include "qemu/config-file.h"
+#include "qapi/visitor.h"
+
+static Property dimm_properties[] = {
+    DEFINE_PROP_UINT64("start", DimmDevice, start, 0),
+    DEFINE_PROP_UINT32("node", DimmDevice, node, 0),
+    DEFINE_PROP_INT32("slot", DimmDevice, slot, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void dimm_get_size(Object *obj, Visitor *v, void *opaque,
+                          const char *name, Error **errp)
+{
+    int64_t value;
+    MemoryRegion *mr;
+    DimmDevice *dimm = DIMM(obj);
+
+    mr = host_memory_backend_get_memory(dimm->hostmem, errp);
+    value = memory_region_size(mr);
+
+    visit_type_int(v, &value, name, errp);
+}
+
+static void dimm_initfn(Object *obj)
+{
+    DimmDevice *dimm = DIMM(obj);
+
+    object_property_add(obj, "size", "int", dimm_get_size,
+                        NULL, NULL, NULL, NULL);
+    object_property_add_link(obj, "memdev", TYPE_MEMORY_BACKEND,
+                            (Object **)&dimm->hostmem, &error_abort);
+}
+
+static void dimm_realize(DeviceState *dev, Error **errp)
+{
+    DimmDevice *dimm = DIMM(dev);
+
+    if (!dimm->hostmem) {
+        error_setg(errp, "'memdev' property is not set");
+        return;
+    }
+
+    if (!dev->id) {
+        error_setg(errp, "'id' property is not set");
+        return;
+    }
+}
+
+static MemoryRegion *dimm_get_memory_region(DimmDevice *dimm)
+{
+    return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
+}
+
+static void dimm_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+    DimmDeviceClass *ddc = DIMM_CLASS(oc);
+
+    dc->realize = dimm_realize;
+    dc->props = dimm_properties;
+
+    ddc->get_memory_region = dimm_get_memory_region;
+}
+
+static TypeInfo dimm_info = {
+    .name          = TYPE_DIMM,
+    .parent        = TYPE_DEVICE,
+    .instance_size = sizeof(DimmDevice),
+    .instance_init = dimm_initfn,
+    .class_init    = dimm_class_init,
+    .class_size    = sizeof(DimmDeviceClass),
+};
+
+static void dimm_register_types(void)
+{
+    type_register_static(&dimm_info);
+}
+
+type_init(dimm_register_types)
diff --git a/include/hw/mem/dimm.h b/include/hw/mem/dimm.h
new file mode 100644
index 0000000..e748e3e
--- /dev/null
+++ b/include/hw/mem/dimm.h
@@ -0,0 +1,58 @@
+/*
+ * DIMM device
+ *
+ * Copyright ProfitBricks GmbH 2012
+ * Copyright (C) 2013 Red Hat Inc
+ *
+ * Authors:
+ *  Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
+ *  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.
+ *
+ */
+
+#ifndef QEMU_DIMM_H
+#define QEMU_DIMM_H
+
+#include "exec/memory.h"
+#include "sysemu/hostmem.h"
+#include "hw/qdev.h"
+
+#define DEFAULT_DIMMSIZE (1024*1024*1024)
+
+#define TYPE_DIMM "dimm"
+#define DIMM(obj) \
+    OBJECT_CHECK(DimmDevice, (obj), TYPE_DIMM)
+#define DIMM_CLASS(oc) \
+    OBJECT_CLASS_CHECK(DimmDeviceClass, (oc), TYPE_DIMM)
+#define DIMM_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(DimmDeviceClass, (obj), TYPE_DIMM)
+
+/**
+ * DimmDevice:
+ * @parent_obj: opaque parent object container
+ * @start: starting guest physical address, where @DimmDevice is mapped.
+ *         Default value: 0, means that address is auto-allocated.
+ * @node: numa node to which @DimmDevice is attached.
+ * @slot: slot number into which @DimmDevice is plugged in.
+ *        Default value: -1, means that slot is auto-allocated.
+ * @mr: memory region provided by host memory backend
+ */
+typedef struct DimmDevice {
+    /* private */
+    DeviceState parent_obj;
+    ram_addr_t start;
+    uint32_t node;
+    int32_t slot;
+    HostMemoryBackend *hostmem;
+} DimmDevice;
+
+typedef struct DimmDeviceClass {
+    DeviceClass parent_class;
+
+    MemoryRegion *(*get_memory_region)(DimmDevice *dimm);
+} DimmDeviceClass;
+
+#endif
-- 
1.7.1

[Qemu-devel] [RFC 6/8] pc: preallocate hotplug links for DIMMDevices

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc.c         |   33 +++++++++++++++++++++++++++++++++
 include/hw/i386/pc.h |    6 ++++++
 2 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index e0bc3a2..7bfd2c9 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1155,6 +1155,9 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory,
     MemoryRegion *ram, *option_rom_mr;
     MemoryRegion *ram_below_4g, *ram_above_4g;
     FWCfgState *fw_cfg;
+    uint64_t ram_slot;
+    MachineState *machine = MACHINE(qdev_get_machine());
+    PCMachineState *pcms = PC_MACHINE(machine);
 
     linux_boot = (kernel_filename != NULL);
 
@@ -1181,6 +1184,20 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory,
         e820_add_entry(0x100000000ULL, above_4g_mem_size, E820_RAM);
     }
 
+    for (ram_slot = 0; ram_slot < machine->init_args.ram_slots; ram_slot++) {
+        char *dimm_name;
+
+        if (ram_slot >= PC_MAX_DIMM_SLOTS) {
+            error_report("supported memory slots limit: PC_MAX_DIMM_SLOTS");
+            exit(EXIT_FAILURE);
+        }
+
+        dimm_name = g_strdup_printf("dimm[%lu]", ram_slot);
+        object_property_add_link(OBJECT(pcms), dimm_name, TYPE_DIMM,
+                             (Object **)&pcms->memory_slots[ram_slot],
+                             &error_abort);
+        g_free(dimm_name);
+    }
 
     /* Initialize PC system firmware */
     pc_system_firmware_init(rom_memory, guest_info->isapc_ram_fw);
@@ -1426,11 +1443,27 @@ void qemu_register_pc_machine(QEMUMachine *m)
     type_register(&ti);
 }
 
+static char *pc_dimm_hotplug_path(DeviceState *dev)
+{
+    DimmDevice *dimm = DIMM(dev);
+
+    return g_strdup_printf("/machine/dimm[%d]", dimm->slot);
+}
+
+static void pc_machine_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dimm_dc = DEVICE_CLASS(object_class_by_name(TYPE_DIMM));
+
+    /* set board specific link path provider for DIMM device */
+    dimm_dc->hotplug_path = pc_dimm_hotplug_path;
+}
+
 static const TypeInfo pc_machine_info = {
     .name = TYPE_PC_MACHINE,
     .parent = TYPE_MACHINE,
     .abstract = true,
     .instance_size = sizeof(PCMachineState),
+    .class_init = pc_machine_class_init,
 };
 
 static void pc_machine_register_types(void)
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index a01a220..277bbb9 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -13,12 +13,18 @@
 #include "sysemu/sysemu.h"
 #include "hw/pci/pci.h"
 #include "hw/boards.h"
+#include "hw/mem/dimm.h"
 
 #define HPET_INTCAP "hpet-intcap"
 
+#define PC_MAX_DIMM_SLOTS 256
+
 struct PCMachineState {
     /*< private >*/
     MachineState parent_obj;
+
+    /* <public> */
+    DimmDevice *memory_slots[PC_MAX_DIMM_SLOTS];
 };
 
 typedef struct PCMachineState PCMachineState;
-- 
1.7.1

[Qemu-devel] [RFC 7/8] pc: initialize memory hotplug address space

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc.c         |   18 ++++++++++++++++--
 include/hw/i386/pc.h |    3 +++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 7bfd2c9..1824c3d 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1155,6 +1155,7 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory,
     MemoryRegion *ram, *option_rom_mr;
     MemoryRegion *ram_below_4g, *ram_above_4g;
     FWCfgState *fw_cfg;
+    ram_addr_t ram_size = below_4g_mem_size + above_4g_mem_size;
     uint64_t ram_slot;
     MachineState *machine = MACHINE(qdev_get_machine());
     PCMachineState *pcms = PC_MACHINE(machine);
@@ -1166,8 +1167,7 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory,
      * with older qemus that used qemu_ram_alloc().
      */
     ram = g_malloc(sizeof(*ram));
-    memory_region_init_ram(ram, NULL, "pc.ram",
-                           below_4g_mem_size + above_4g_mem_size);
+    memory_region_init_ram(ram, NULL, "pc.ram", ram_size);
     vmstate_register_ram_global(ram);
     *ram_memory = ram;
     ram_below_4g = g_malloc(sizeof(*ram_below_4g));
@@ -1184,6 +1184,20 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory,
         e820_add_entry(0x100000000ULL, above_4g_mem_size, E820_RAM);
     }
 
+    /* initialize memory hotplug address space */
+    if (ram_size < machine->init_args.maxram_size) {
+        ram_addr_t hotplug_mem_size =
+            machine->init_args.maxram_size - ram_size;
+
+        pcms->hotplug_memory_base =
+            ROUND_UP(0x100000000ULL + above_4g_mem_size, 1ULL << 30);
+
+        memory_region_init(&pcms->hotplug_memory, OBJECT(pcms),
+                           "hotplug-memory", hotplug_mem_size);
+        memory_region_add_subregion(system_memory, pcms->hotplug_memory_base,
+                                    &pcms->hotplug_memory);
+    }
+
     for (ram_slot = 0; ram_slot < machine->init_args.ram_slots; ram_slot++) {
         char *dimm_name;
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 277bbb9..a25957d 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -25,6 +25,9 @@ struct PCMachineState {
 
     /* <public> */
     DimmDevice *memory_slots[PC_MAX_DIMM_SLOTS];
+
+    ram_addr_t hotplug_memory_base;
+    MemoryRegion hotplug_memory;
 };
 
 typedef struct PCMachineState PCMachineState;
-- 
1.7.1

[Qemu-devel] [RFC 8/8] pc: make PC_MACHINE memory hotplug controller

[Igor Mammedov]

that performs mapping of DIMM into guest's address space

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 1824c3d..1cef30d 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1445,6 +1445,17 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
     }
 }
 
+static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
+                                      DeviceState *dev, Error **errp)
+{
+   DimmDevice *dimm = DIMM(dev);
+   DimmDeviceClass *ddc = DIMM_GET_CLASS(dimm);
+   PCMachineState *pcms = PC_MACHINE(hotplug_dev);
+   MemoryRegion *mr = ddc->get_memory_region(dimm);
+
+   memory_region_add_subregion(&pcms->hotplug_memory, dimm->start, mr);
+}
+
 void qemu_register_pc_machine(QEMUMachine *m)
 {
     TypeInfo ti = {
@@ -1467,9 +1478,12 @@ static char *pc_dimm_hotplug_path(DeviceState *dev)
 static void pc_machine_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dimm_dc = DEVICE_CLASS(object_class_by_name(TYPE_DIMM));
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
 
     /* set board specific link path provider for DIMM device */
     dimm_dc->hotplug_path = pc_dimm_hotplug_path;
+
+    hc->plug = pc_machine_device_plug_cb;
 }
 
 static const TypeInfo pc_machine_info = {
@@ -1478,6 +1492,10 @@ static const TypeInfo pc_machine_info = {
     .abstract = true,
     .instance_size = sizeof(PCMachineState),
     .class_init = pc_machine_class_init,
+    .interfaces = (InterfaceInfo[]) {
+         { TYPE_HOTPLUG_HANDLER },
+         { }
+    },
 };
 
 static void pc_machine_register_types(void)
-- 
1.7.1