Re: [Qemu-devel] [PATCH v19 7/9] machine: add properties to compat_props incrementaly

[Cornelia Huck]

On Thu, 28 Jan 2016 11:58:08 +0100
Igor Mammedov  wrote:

> Switch to adding compat properties incrementaly instead of
> completly overwriting compat_props per machine type.
> That removes data duplication which we have due to nested
> [PC|SPAPR]_COMPAT_* macros.

We'll try to switch to something similar to spapr for ccw so we can get
rid of the nesting as well (once one of us has time to look into that).

> 
> It also allows to set default device properties from
> default foo_machine_options() hook, which will be used
> in following patch for putting VMGENID device as
> a function if ISA bridge on pc/q35 machines.
> 
> Suggested-by: Eduardo Habkost 
> Signed-off-by: Igor Mammedov 

master + this patch (+ <20160115120143.GB2432@work-vm>) survives some
playing around with virsh managedsave and the 2.4/2.5/2.6 ccw machines,
so

Acked-by: Cornelia Huck 

Re: [Qemu-devel] [PATCH v19 7/9] machine: add properties to compat_props incrementaly

[Igor Mammedov]

On Thu, 28 Jan 2016 12:02:12 -0200
Eduardo Habkost  wrote:

> On Thu, Jan 28, 2016 at 11:58:08AM +0100, Igor Mammedov wrote:
> > Switch to adding compat properties incrementaly instead of
> > completly overwriting compat_props per machine type.
> > That removes data duplication which we have due to nested
> > [PC|SPAPR]_COMPAT_* macros.
> > 
> > It also allows to set default device properties from
> > default foo_machine_options() hook, which will be used
> > in following patch for putting VMGENID device as
> > a function if ISA bridge on pc/q35 machines.
> > 
> > Suggested-by: Eduardo Habkost 
> > Signed-off-by: Igor Mammedov 
> >   
> 
> Very nice. The only suggestion I have is to use the simpler GList
> type, instead of GArray.
It's fine with me to use GList here as well,
fill free to pick this patch in case you'd like to do it.
it should be trivial to swap from one type to another.

It looks like this series might go nowhere but this patch
is not tied to it and useful to us in general
so perhaps you could pick it up after ACKs from
S390/SPAPR maintainers.

> 
> Reviewed-by: Eduardo Habkost 
> 
> > compat_props GArray  
> 
> I assume this line was left here by mistake?
yep, it's leftover from squashing fixup.

> 
> > ---
> >  hw/core/machine.c  | 10 ++
> >  hw/ppc/spapr.c |  3 ---
> >  hw/s390x/s390-virtio-ccw.c | 12 ++--
> >  include/hw/boards.h| 11 +--
> >  include/hw/i386/pc.h   |  9 -
> >  vl.c   |  6 +-
> >  6 files changed, 26 insertions(+), 25 deletions(-)  
> [...]
> 

[Qemu-devel] [PATCH v19 7/9] machine: add properties to compat_props incrementaly

[Igor Mammedov]

Switch to adding compat properties incrementaly instead of
completly overwriting compat_props per machine type.
That removes data duplication which we have due to nested
[PC|SPAPR]_COMPAT_* macros.

It also allows to set default device properties from
default foo_machine_options() hook, which will be used
in following patch for putting VMGENID device as
a function if ISA bridge on pc/q35 machines.

Suggested-by: Eduardo Habkost 
Signed-off-by: Igor Mammedov 

compat_props GArray
---
 hw/core/machine.c  | 10 ++
 hw/ppc/spapr.c |  3 ---
 hw/s390x/s390-virtio-ccw.c | 12 ++--
 include/hw/boards.h| 11 +--
 include/hw/i386/pc.h   |  9 -
 vl.c   |  6 +-
 6 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index c46ddc7..2b96f47 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -526,6 +526,15 @@ bool machine_mem_merge(MachineState *machine)
 return machine->mem_merge;
 }
 
+static void machine_class_finalize(ObjectClass *klass, void *data)
+{
+MachineClass *mc = MACHINE_CLASS(klass);
+
+if (mc->compat_props) {
+g_array_free(mc->compat_props, true);
+}
+}
+
 static const TypeInfo machine_info = {
 .name = TYPE_MACHINE,
 .parent = TYPE_OBJECT,
@@ -533,6 +542,7 @@ static const TypeInfo machine_info = {
 .class_size = sizeof(MachineClass),
 .class_init= machine_class_init,
 .class_base_init = machine_class_base_init,
+.class_finalize = machine_class_finalize,
 .instance_size = sizeof(MachineState),
 .instance_init = machine_initfn,
 .instance_finalize = machine_finalize,
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 50e5a26..4ec1156 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2412,7 +2412,6 @@ DEFINE_SPAPR_MACHINE(2_4, "2.4", false);
  * pseries-2.3
  */
 #define SPAPR_COMPAT_2_3 \
-SPAPR_COMPAT_2_4 \
 HW_COMPAT_2_3 \
 {\
 .driver   = "spapr-pci-host-bridge",\
@@ -2439,7 +2438,6 @@ DEFINE_SPAPR_MACHINE(2_3, "2.3", false);
  */
 
 #define SPAPR_COMPAT_2_2 \
-SPAPR_COMPAT_2_3 \
 HW_COMPAT_2_2 \
 {\
 .driver   = TYPE_SPAPR_PCI_HOST_BRIDGE,\
@@ -2463,7 +2461,6 @@ DEFINE_SPAPR_MACHINE(2_2, "2.2", false);
  * pseries-2.1
  */
 #define SPAPR_COMPAT_2_1 \
-SPAPR_COMPAT_2_2 \
 HW_COMPAT_2_1
 
 static void spapr_machine_2_1_instance_options(MachineState *machine)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 586ddbb..0d3c3f8 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -282,13 +282,9 @@ static const TypeInfo ccw_machine_info = {
 static void ccw_machine_2_4_class_init(ObjectClass *oc, void *data)
 {
 MachineClass *mc = MACHINE_CLASS(oc);
-static GlobalProperty compat_props[] = {
-CCW_COMPAT_2_4
-{ /* end of list */ }
-};
 
 mc->desc = "VirtIO-ccw based S390 machine v2.4";
-mc->compat_props = compat_props;
+SET_MACHINE_COMPAT(mc, CCW_COMPAT_2_4);
 }
 
 static const TypeInfo ccw_machine_2_4_info = {
@@ -300,13 +296,9 @@ static const TypeInfo ccw_machine_2_4_info = {
 static void ccw_machine_2_5_class_init(ObjectClass *oc, void *data)
 {
 MachineClass *mc = MACHINE_CLASS(oc);
-static GlobalProperty compat_props[] = {
-CCW_COMPAT_2_5
-{ /* end of list */ }
-};
 
 mc->desc = "VirtIO-ccw based S390 machine v2.5";
-mc->compat_props = compat_props;
+SET_MACHINE_COMPAT(mc, CCW_COMPAT_2_5);
 }
 
 static const TypeInfo ccw_machine_2_5_info = {
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 0f30959..cdb4a98 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -90,7 +90,7 @@ struct MachineClass {
 const char *default_machine_opts;
 const char *default_boot_order;
 const char *default_display;
-GlobalProperty *compat_props;
+GArray *compat_props;
 const char *hw_version;
 ram_addr_t default_ram_size;
 bool option_rom_has_mr;
@@ -159,11 +159,18 @@ struct MachineState {
 
 #define SET_MACHINE_COMPAT(m, COMPAT) \
 do {  \
+int i;\
 static GlobalProperty props[] = {   \
 COMPAT  \
 { /* end of list */ }   \
 };  \
-(m)->compat_props = props;  \
+if (!m->compat_props) { \
+m->compat_props = g_array_new(false, false, sizeof(void *)); \
+} \
+for (i = 0; props[i].driver != NULL; i++) {\
+GlobalProperty *prop = [i];  \
+g_array_append_val(m->compat_props, prop); \
+}  \
 } while (0)
 
 #endif
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 65e8f24..7713361 100644
--- 

Re: [Qemu-devel] [PATCH v19 7/9] machine: add properties to compat_props incrementaly

[Eduardo Habkost]

On Thu, Jan 28, 2016 at 11:58:08AM +0100, Igor Mammedov wrote:
> Switch to adding compat properties incrementaly instead of
> completly overwriting compat_props per machine type.
> That removes data duplication which we have due to nested
> [PC|SPAPR]_COMPAT_* macros.
> 
> It also allows to set default device properties from
> default foo_machine_options() hook, which will be used
> in following patch for putting VMGENID device as
> a function if ISA bridge on pc/q35 machines.
> 
> Suggested-by: Eduardo Habkost 
> Signed-off-by: Igor Mammedov 
> 

Very nice. The only suggestion I have is to use the simpler GList
type, instead of GArray.

Reviewed-by: Eduardo Habkost 

> compat_props GArray

I assume this line was left here by mistake?

> ---
>  hw/core/machine.c  | 10 ++
>  hw/ppc/spapr.c |  3 ---
>  hw/s390x/s390-virtio-ccw.c | 12 ++--
>  include/hw/boards.h| 11 +--
>  include/hw/i386/pc.h   |  9 -
>  vl.c   |  6 +-
>  6 files changed, 26 insertions(+), 25 deletions(-)
[...]

-- 
Eduardo