diff mbox

pc: reduce duplication in compat machine types

Message ID 20120326094020.GA14437@redhat.com
State New
Headers show

Commit Message

Michael S. Tsirkin March 26, 2012, 9:40 a.m. UTC 
Make it easier to add compat properties, by
adding macros for properties duplicated across
machine types.

Note: there could be bugs in compat properties,
this patch does not attempt to address them,
the code is bug for bug identical to the original.

Tested by: generated a preprocessed file, sorted and
compared to sorted original.
Lightly tested on x86_64.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

I've put the above on my branch as it's needed for
fixing balloon pci class. Let me know if there are
any objections.

 hw/pc_piix.c |  288 +++++++++++++++-------------------------------------------
 1 files changed, 73 insertions(+), 215 deletions(-)

Comments

Avi Kivity March 26, 2012, 12:51 p.m. UTC | #1 
On 03/26/2012 11:40 AM, Michael S. Tsirkin wrote:
> Make it easier to add compat properties, by
> adding macros for properties duplicated across
> machine types.
>
> Note: there could be bugs in compat properties,
> this patch does not attempt to address them,
> the code is bug for bug identical to the original.
>
> Tested by: generated a preprocessed file, sorted and
> compared to sorted original.
> Lightly tested on x86_64.
>
>  
> +#define PC_COMPAT_1_0 \
> +        {\
> +            .driver   = "pc-sysfw",\
> +            .property = "rom_only",\
> +            .value    = stringify(1),\
> +        }, {\
> +            .driver   = "isa-fdc",\
> +            .property = "check_media_rate",\
> +            .value    = "off",\
> +        }
> +

Hmm.  how about

>  static QEMUMachine pc_machine_v1_0 = {
>      .name = "pc-1.0",
>      .desc = "Standard PC",
>      .init = pc_init_pci,
>      .max_cpus = 255,
>      .compat_props = (GlobalProperty[]) {
> -        {
> -            .driver   = "pc-sysfw",
> -            .property = "rom_only",
> -            .value    = stringify(1),
> -        }, {
> -            .driver   = "isa-fdc",
> -            .property = "check_media_rate",
> -            .value    = "off",
> -        },
> +        PC_COMPAT_1_0,

+    .base_machine =  &pc_machine_v1_1;

Then it would be easier to define machines differentially.


>          { /* end of list */ }
>      },
Andreas Färber March 26, 2012, 1:12 p.m. UTC | #2 
Am 26.03.2012 11:40, schrieb Michael S. Tsirkin:
> Make it easier to add compat properties, by
> adding macros for properties duplicated across
> machine types.
> 
> Note: there could be bugs in compat properties,
> this patch does not attempt to address them,
> the code is bug for bug identical to the original.
> 
> Tested by: generated a preprocessed file, sorted and
> compared to sorted original.
> Lightly tested on x86_64.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 
> I've put the above on my branch as it's needed for
> fixing balloon pci class. Let me know if there are
> any objections.

Better reusing code is certainly a good idea. I wonder though if we
might ever get into a situation where things change back and forth. I'd
hope that properties are processed top to bottom so that, e.g.,
PC_COMPAT_1_0, { ... } can overwrite 0_15 compat properties inherited
through 1_0.

Andreas

> 
>  hw/pc_piix.c |  288 +++++++++++++++-------------------------------------------
>  1 files changed, 73 insertions(+), 215 deletions(-)
> 
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 3f99f9a..e39ef69 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -372,50 +372,69 @@ static QEMUMachine pc_machine_v1_1 = {
>      .is_default = 1,
>  };
>  
> +#define PC_COMPAT_1_0 \
> +        {\
> +            .driver   = "pc-sysfw",\
> +            .property = "rom_only",\
> +            .value    = stringify(1),\
> +        }, {\
> +            .driver   = "isa-fdc",\
> +            .property = "check_media_rate",\
> +            .value    = "off",\
> +        }
> +
>  static QEMUMachine pc_machine_v1_0 = {
>      .name = "pc-1.0",
>      .desc = "Standard PC",
>      .init = pc_init_pci,
>      .max_cpus = 255,
>      .compat_props = (GlobalProperty[]) {
> -        {
> -            .driver   = "pc-sysfw",
> -            .property = "rom_only",
> -            .value    = stringify(1),
> -        }, {
> -            .driver   = "isa-fdc",
> -            .property = "check_media_rate",
> -            .value    = "off",
> -        },
> +        PC_COMPAT_1_0,
>          { /* end of list */ }
>      },
>  };
[snip]
Michael S. Tsirkin March 26, 2012, 1:23 p.m. UTC | #3 
On Mon, Mar 26, 2012 at 03:12:37PM +0200, Andreas Färber wrote:
> Am 26.03.2012 11:40, schrieb Michael S. Tsirkin:
> > Make it easier to add compat properties, by
> > adding macros for properties duplicated across
> > machine types.
> > 
> > Note: there could be bugs in compat properties,
> > this patch does not attempt to address them,
> > the code is bug for bug identical to the original.
> > 
> > Tested by: generated a preprocessed file, sorted and
> > compared to sorted original.
> > Lightly tested on x86_64.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > 
> > I've put the above on my branch as it's needed for
> > fixing balloon pci class. Let me know if there are
> > any objections.
> 
> Better reusing code is certainly a good idea. I wonder though if we
> might ever get into a situation where things change back and forth. I'd
> hope that properties are processed top to bottom so that, e.g.,
> PC_COMPAT_1_0, { ... } can overwrite 0_15 compat properties inherited
> through 1_0.
> 
> Andreas

There are some properties like that currently.
For example, see ide-drive "ver" property or
VGA rombar property.  I'm not sure that's intentional but I kept
it as is.

In that case we can simply keep it out of
the common macros, that's what I did. There's no rule that says
we must have everything in there.

> > 
> >  hw/pc_piix.c |  288 +++++++++++++++-------------------------------------------
> >  1 files changed, 73 insertions(+), 215 deletions(-)
> > 
> > diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> > index 3f99f9a..e39ef69 100644
> > --- a/hw/pc_piix.c
> > +++ b/hw/pc_piix.c
> > @@ -372,50 +372,69 @@ static QEMUMachine pc_machine_v1_1 = {
> >      .is_default = 1,
> >  };
> >  
> > +#define PC_COMPAT_1_0 \
> > +        {\
> > +            .driver   = "pc-sysfw",\
> > +            .property = "rom_only",\
> > +            .value    = stringify(1),\
> > +        }, {\
> > +            .driver   = "isa-fdc",\
> > +            .property = "check_media_rate",\
> > +            .value    = "off",\
> > +        }
> > +
> >  static QEMUMachine pc_machine_v1_0 = {
> >      .name = "pc-1.0",
> >      .desc = "Standard PC",
> >      .init = pc_init_pci,
> >      .max_cpus = 255,
> >      .compat_props = (GlobalProperty[]) {
> > -        {
> > -            .driver   = "pc-sysfw",
> > -            .property = "rom_only",
> > -            .value    = stringify(1),
> > -        }, {
> > -            .driver   = "isa-fdc",
> > -            .property = "check_media_rate",
> > -            .value    = "off",
> > -        },
> > +        PC_COMPAT_1_0,
> >          { /* end of list */ }
> >      },
> >  };
> [snip]
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Michael S. Tsirkin March 26, 2012, 1:32 p.m. UTC | #4 
On Mon, Mar 26, 2012 at 02:51:38PM +0200, Avi Kivity wrote:
> On 03/26/2012 11:40 AM, Michael S. Tsirkin wrote:
> > Make it easier to add compat properties, by
> > adding macros for properties duplicated across
> > machine types.
> >
> > Note: there could be bugs in compat properties,
> > this patch does not attempt to address them,
> > the code is bug for bug identical to the original.
> >
> > Tested by: generated a preprocessed file, sorted and
> > compared to sorted original.
> > Lightly tested on x86_64.
> >
> >  
> > +#define PC_COMPAT_1_0 \
> > +        {\
> > +            .driver   = "pc-sysfw",\
> > +            .property = "rom_only",\
> > +            .value    = stringify(1),\
> > +        }, {\
> > +            .driver   = "isa-fdc",\
> > +            .property = "check_media_rate",\
> > +            .value    = "off",\
> > +        }
> > +
> 
> Hmm.  how about
> 
> >  static QEMUMachine pc_machine_v1_0 = {
> >      .name = "pc-1.0",
> >      .desc = "Standard PC",
> >      .init = pc_init_pci,
> >      .max_cpus = 255,
> >      .compat_props = (GlobalProperty[]) {
> > -        {
> > -            .driver   = "pc-sysfw",
> > -            .property = "rom_only",
> > -            .value    = stringify(1),
> > -        }, {
> > -            .driver   = "isa-fdc",
> > -            .property = "check_media_rate",
> > -            .value    = "off",
> > -        },
> > +        PC_COMPAT_1_0,
> 
> +    .base_machine =  &pc_machine_v1_1;
> 

It's a lot of work, and the result won't be easier to
modify or debug than it already is, IMO.

> Then it would be easier to define machines differentially.

We add new 1 machine each release, so I'm not worried about
adding them easily, adding properies easily happens
between releases so I want to make is simpler.

> >          { /* end of list */ }
> >      },

> 
> -- 
> error compiling committee.c: too many arguments to function
diff mbox

Patch

diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 3f99f9a..e39ef69 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -372,50 +372,69 @@  static QEMUMachine pc_machine_v1_1 = {
     .is_default = 1,
 };
 
+#define PC_COMPAT_1_0 \
+        {\
+            .driver   = "pc-sysfw",\
+            .property = "rom_only",\
+            .value    = stringify(1),\
+        }, {\
+            .driver   = "isa-fdc",\
+            .property = "check_media_rate",\
+            .value    = "off",\
+        }
+
 static QEMUMachine pc_machine_v1_0 = {
     .name = "pc-1.0",
     .desc = "Standard PC",
     .init = pc_init_pci,
     .max_cpus = 255,
     .compat_props = (GlobalProperty[]) {
-        {
-            .driver   = "pc-sysfw",
-            .property = "rom_only",
-            .value    = stringify(1),
-        }, {
-            .driver   = "isa-fdc",
-            .property = "check_media_rate",
-            .value    = "off",
-        },
+        PC_COMPAT_1_0,
         { /* end of list */ }
     },
 };
 
+#define PC_COMPAT_0_15 \
+        PC_COMPAT_1_0
+
 static QEMUMachine pc_machine_v0_15 = {
     .name = "pc-0.15",
     .desc = "Standard PC",
     .init = pc_init_pci,
     .max_cpus = 255,
     .compat_props = (GlobalProperty[]) {
-        {
-            .driver   = "pc-sysfw",
-            .property = "rom_only",
-            .value    = stringify(1),
-        }, {
-            .driver   = "isa-fdc",
-            .property = "check_media_rate",
-            .value    = "off",
-        },
+        PC_COMPAT_0_15,
         { /* end of list */ }
     },
 };
 
+#define PC_COMPAT_0_14 \
+        PC_COMPAT_0_15,\
+        {\
+            .driver   = "virtio-blk-pci",\
+            .property = "event_idx",\
+            .value    = "off",\
+        },{\
+            .driver   = "virtio-serial-pci",\
+            .property = "event_idx",\
+            .value    = "off",\
+        },{\
+            .driver   = "virtio-net-pci",\
+            .property = "event_idx",\
+            .value    = "off",\
+        },{\
+            .driver   = "virtio-balloon-pci",\
+            .property = "event_idx",\
+            .value    = "off",\
+        }
+
 static QEMUMachine pc_machine_v0_14 = {
     .name = "pc-0.14",
     .desc = "Standard PC",
     .init = pc_init_pci,
     .max_cpus = 255,
     .compat_props = (GlobalProperty[]) {
+        PC_COMPAT_0_14, 
         {
             .driver   = "qxl",
             .property = "revision",
@@ -424,42 +443,30 @@  static QEMUMachine pc_machine_v0_14 = {
             .driver   = "qxl-vga",
             .property = "revision",
             .value    = stringify(2),
-        },{
-            .driver   = "virtio-blk-pci",
-            .property = "event_idx",
-            .value    = "off",
-        },{
-            .driver   = "virtio-serial-pci",
-            .property = "event_idx",
-            .value    = "off",
-        },{
-            .driver   = "virtio-net-pci",
-            .property = "event_idx",
-            .value    = "off",
-        },{
-            .driver   = "virtio-balloon-pci",
-            .property = "event_idx",
-            .value    = "off",
-        },{
-            .driver   = "isa-fdc",
-            .property = "check_media_rate",
-            .value    = "off",
-        },
-        {
-            .driver   = "pc-sysfw",
-            .property = "rom_only",
-            .value    = stringify(1),
         },
         { /* end of list */ }
     },
 };
 
+#define PC_COMPAT_0_13 \
+        PC_COMPAT_0_14,\
+        {\
+            .driver   = "PCI",\
+            .property = "command_serr_enable",\
+            .value    = "off",\
+        },{\
+            .driver   = "AC97",\
+            .property = "use_broken_id",\
+            .value    = stringify(1),\
+        }
+
 static QEMUMachine pc_machine_v0_13 = {
     .name = "pc-0.13",
     .desc = "Standard PC",
     .init = pc_init_pci_no_kvmclock,
     .max_cpus = 255,
     .compat_props = (GlobalProperty[]) {
+        PC_COMPAT_0_13,
         {
             .driver   = "virtio-9p-pci",
             .property = "vectors",
@@ -472,59 +479,31 @@  static QEMUMachine pc_machine_v0_13 = {
             .driver   = "vmware-svga",
             .property = "rombar",
             .value    = stringify(0),
-        },{
-            .driver   = "PCI",
-            .property = "command_serr_enable",
-            .value    = "off",
-        },{
-            .driver   = "virtio-blk-pci",
-            .property = "event_idx",
-            .value    = "off",
-        },{
-            .driver   = "virtio-serial-pci",
-            .property = "event_idx",
-            .value    = "off",
-        },{
-            .driver   = "virtio-net-pci",
-            .property = "event_idx",
-            .value    = "off",
-        },{
-            .driver   = "virtio-balloon-pci",
-            .property = "event_idx",
-            .value    = "off",
-        },{
-            .driver   = "AC97",
-            .property = "use_broken_id",
-            .value    = stringify(1),
-        },{
-            .driver   = "isa-fdc",
-            .property = "check_media_rate",
-            .value    = "off",
-        },
-        {
-            .driver   = "pc-sysfw",
-            .property = "rom_only",
-            .value    = stringify(1),
         },
         { /* end of list */ }
     },
 };
 
+#define PC_COMPAT_0_12 \
+        PC_COMPAT_0_13,\
+        {\
+            .driver   = "virtio-serial-pci",\
+            .property = "max_ports",\
+            .value    = stringify(1),\
+        },{\
+            .driver   = "virtio-serial-pci",\
+            .property = "vectors",\
+            .value    = stringify(0),\
+        }
+
 static QEMUMachine pc_machine_v0_12 = {
     .name = "pc-0.12",
     .desc = "Standard PC",
     .init = pc_init_pci_no_kvmclock,
     .max_cpus = 255,
     .compat_props = (GlobalProperty[]) {
+        PC_COMPAT_0_12,
         {
-            .driver   = "virtio-serial-pci",
-            .property = "max_ports",
-            .value    = stringify(1),
-        },{
-            .driver   = "virtio-serial-pci",
-            .property = "vectors",
-            .value    = stringify(0),
-        },{
             .driver   = "VGA",
             .property = "rombar",
             .value    = stringify(0),
@@ -532,63 +511,27 @@  static QEMUMachine pc_machine_v0_12 = {
             .driver   = "vmware-svga",
             .property = "rombar",
             .value    = stringify(0),
-        },{
-            .driver   = "PCI",
-            .property = "command_serr_enable",
-            .value    = "off",
-        },{
-            .driver   = "virtio-blk-pci",
-            .property = "event_idx",
-            .value    = "off",
-        },{
-            .driver   = "virtio-serial-pci",
-            .property = "event_idx",
-            .value    = "off",
-        },{
-            .driver   = "virtio-net-pci",
-            .property = "event_idx",
-            .value    = "off",
-        },{
-            .driver   = "virtio-balloon-pci",
-            .property = "event_idx",
-            .value    = "off",
-        },{
-            .driver   = "AC97",
-            .property = "use_broken_id",
-            .value    = stringify(1),
-        },{
-            .driver   = "isa-fdc",
-            .property = "check_media_rate",
-            .value    = "off",
-        },
-        {
-            .driver   = "pc-sysfw",
-            .property = "rom_only",
-            .value    = stringify(1),
         },
         { /* end of list */ }
     }
 };
 
+#define PC_COMPAT_0_11 \
+        PC_COMPAT_0_12,\
+        {\
+            .driver   = "virtio-blk-pci",\
+            .property = "vectors",\
+            .value    = stringify(0),\
+        }
+
 static QEMUMachine pc_machine_v0_11 = {
     .name = "pc-0.11",
     .desc = "Standard PC, qemu 0.11",
     .init = pc_init_pci_no_kvmclock,
     .max_cpus = 255,
     .compat_props = (GlobalProperty[]) {
+        PC_COMPAT_0_11,
         {
-            .driver   = "virtio-blk-pci",
-            .property = "vectors",
-            .value    = stringify(0),
-        },{
-            .driver   = "virtio-serial-pci",
-            .property = "max_ports",
-            .value    = stringify(1),
-        },{
-            .driver   = "virtio-serial-pci",
-            .property = "vectors",
-            .value    = stringify(0),
-        },{
             .driver   = "ide-drive",
             .property = "ver",
             .value    = "0.11",
@@ -596,43 +539,6 @@  static QEMUMachine pc_machine_v0_11 = {
             .driver   = "scsi-disk",
             .property = "ver",
             .value    = "0.11",
-        },{
-            .driver   = "PCI",
-            .property = "rombar",
-            .value    = stringify(0),
-        },{
-            .driver   = "PCI",
-            .property = "command_serr_enable",
-            .value    = "off",
-        },{
-            .driver   = "virtio-blk-pci",
-            .property = "event_idx",
-            .value    = "off",
-        },{
-            .driver   = "virtio-serial-pci",
-            .property = "event_idx",
-            .value    = "off",
-        },{
-            .driver   = "virtio-net-pci",
-            .property = "event_idx",
-            .value    = "off",
-        },{
-            .driver   = "virtio-balloon-pci",
-            .property = "event_idx",
-            .value    = "off",
-        },{
-            .driver   = "AC97",
-            .property = "use_broken_id",
-            .value    = stringify(1),
-        },{
-            .driver   = "isa-fdc",
-            .property = "check_media_rate",
-            .value    = "off",
-        },
-        {
-            .driver   = "pc-sysfw",
-            .property = "rom_only",
-            .value    = stringify(1),
         },
         { /* end of list */ }
     }
@@ -644,6 +550,7 @@  static QEMUMachine pc_machine_v0_10 = {
     .init = pc_init_pci_no_kvmclock,
     .max_cpus = 255,
     .compat_props = (GlobalProperty[]) {
+        PC_COMPAT_0_11,
         {
             .driver   = "virtio-blk-pci",
             .property = "class",
@@ -653,22 +560,10 @@  static QEMUMachine pc_machine_v0_10 = {
             .property = "class",
             .value    = stringify(PCI_CLASS_DISPLAY_OTHER),
         },{
-            .driver   = "virtio-serial-pci",
-            .property = "max_ports",
-            .value    = stringify(1),
-        },{
-            .driver   = "virtio-serial-pci",
-            .property = "vectors",
-            .value    = stringify(0),
-        },{
             .driver   = "virtio-net-pci",
             .property = "vectors",
             .value    = stringify(0),
         },{
-            .driver   = "virtio-blk-pci",
-            .property = "vectors",
-            .value    = stringify(0),
-        },{
             .driver   = "ide-drive",
             .property = "ver",
             .value    = "0.10",
@@ -676,43 +571,6 @@  static QEMUMachine pc_machine_v0_10 = {
             .driver   = "scsi-disk",
             .property = "ver",
             .value    = "0.10",
-        },{
-            .driver   = "PCI",
-            .property = "rombar",
-            .value    = stringify(0),
-        },{
-            .driver   = "PCI",
-            .property = "command_serr_enable",
-            .value    = "off",
-        },{
-            .driver   = "virtio-blk-pci",
-            .property = "event_idx",
-            .value    = "off",
-        },{
-            .driver   = "virtio-serial-pci",
-            .property = "event_idx",
-            .value    = "off",
-        },{
-            .driver   = "virtio-net-pci",
-            .property = "event_idx",
-            .value    = "off",
-        },{
-            .driver   = "virtio-balloon-pci",
-            .property = "event_idx",
-            .value    = "off",
-        },{
-            .driver   = "AC97",
-            .property = "use_broken_id",
-            .value    = stringify(1),
-        },{
-            .driver   = "isa-fdc",
-            .property = "check_media_rate",
-            .value    = "off",
-        },
-        {
-            .driver   = "pc-sysfw",
-            .property = "rom_only",
-            .value    = stringify(1),
         },
         { /* end of list */ }
     },