[v2,2/2] machine: Introduce QEMU_COMPAT_* macros

The QEMU_COMPAT_* macros will contain compat properties that are not
specific to PC, and may be reused by other machine-types.

The compat properties for PC-specific devices were moved to
QEMU_COMPAT_* too, because they are simply not going to be applied to
any device if they are not instantiated.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 include/hw/compat.h  | 210 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/i386/pc.h | 206 +++-----------------------------------------------
 2 files changed, 221 insertions(+), 195 deletions(-)
 create mode 100644 include/hw/compat.h

On Tue, Jun 24, 2014 at 11:04:45PM -0300, Eduardo Habkost wrote:
> The QEMU_COMPAT_* macros will contain compat properties that are not
> specific to PC, and may be reused by other machine-types.
> 
> The compat properties for PC-specific devices were moved to
> QEMU_COMPAT_* too, because they are simply not going to be applied to
> any device if they are not instantiated.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

I don't see value in this.
If some target wants to start versioning their machine types,
let them start from 2.0 (or even 2.1 - does any non-PC target
plan to support cross-version compatibility for 2.0?).
Why carry around useless code for < 2.0 for non PC platforms?

> ---
>  include/hw/compat.h  | 210 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/i386/pc.h | 206 +++-----------------------------------------------
>  2 files changed, 221 insertions(+), 195 deletions(-)
>  create mode 100644 include/hw/compat.h

And we have lost all we have gained in 1/2 :(

> 
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> new file mode 100644
> index 0000000..dff55c8
> --- /dev/null
> +++ b/include/hw/compat.h
> @@ -0,0 +1,210 @@
> +/* Common compat properties that can be used by machine-types to keep guest ABI
> + * stable between QEMU versions.
> + */
> +
> +#ifndef HW_COMPAT_H
> +#define HW_COMPAT_H
> +
> +#define QEMU_COMPAT_2_0 \
> +    {\
> +        .driver   = "virtio-scsi-pci",\
> +        .property = "any_layout",\
> +        .value    = "off",\
> +    },{\
> +        .driver   = "PIIX4_PM",\
> +        .property = "memory-hotplug-support",\
> +        .value    = "off",\
> +    },\
> +    {\
> +        .driver   = "apic",\
> +        .property = "version",\
> +        .value    = stringify(0x11),\
> +    },\
> +    {\
> +        .driver   = "nec-usb-xhci",\
> +        .property = "superspeed-ports-first",\
> +        .value    = "off",\
> +    },\
> +    {\
> +        .driver   = "pci-serial",\
> +        .property = "prog_if",\
> +        .value    = stringify(0),\
> +    },\
> +    {\
> +        .driver   = "pci-serial-2x",\
> +        .property = "prog_if",\
> +        .value    = stringify(0),\
> +    },\
> +    {\
> +        .driver   = "pci-serial-4x",\
> +        .property = "prog_if",\
> +        .value    = stringify(0),\
> +    },\
> +    {\
> +        .driver   = "virtio-net-pci",\
> +        .property = "guest_announce",\
> +        .value    = "off",\
> +    },\
> +    {\
> +        .driver   = "ICH9-LPC",\
> +        .property = "memory-hotplug-support",\
> +        .value    = "off",\
> +    },{\
> +        .driver   = "xio3130-downstream",\
> +        .property = COMPAT_PROP_PCP,\
> +        .value    = "off",\
> +    },{\
> +        .driver   = "ioh3420",\
> +        .property = COMPAT_PROP_PCP,\
> +        .value    = "off",\
> +    }
> +
> +#define QEMU_COMPAT_1_7 \
> +    {\
> +        .driver   = TYPE_USB_DEVICE,\
> +        .property = "msos-desc",\
> +        .value    = "no",\
> +    },\
> +    {\
> +        .driver   = "PIIX4_PM",\
> +        .property = "acpi-pci-hotplug-with-bridge-support",\
> +        .value    = "off",\
> +    },\
> +    {\
> +        .driver   = "hpet",\
> +        .property = HPET_INTCAP,\
> +        .value    = stringify(4),\
> +    }
> +
> +#define QEMU_COMPAT_1_6 \
> +    {\
> +        .driver   = "e1000",\
> +        .property = "mitigation",\
> +        .value    = "off",\
> +    },{\
> +        .driver   = "qemu64-" TYPE_X86_CPU,\
> +        .property = "model",\
> +        .value    = stringify(2),\
> +    },{\
> +        .driver   = "qemu32-" TYPE_X86_CPU,\
> +        .property = "model",\
> +        .value    = stringify(3),\
> +    },{\
> +        .driver   = "i440FX-pcihost",\
> +        .property = "short_root_bus",\
> +        .value    = stringify(1),\
> +    },{\
> +        .driver   = "q35-pcihost",\
> +        .property = "short_root_bus",\
> +        .value    = stringify(1),\
> +    }
> +
> +#define QEMU_COMPAT_1_5 \
> +    {\
> +        .driver   = "Conroe-" TYPE_X86_CPU,\
> +        .property = "model",\
> +        .value    = stringify(2),\
> +    },{\
> +        .driver   = "Conroe-" TYPE_X86_CPU,\
> +        .property = "level",\
> +        .value    = stringify(2),\
> +    },{\
> +        .driver   = "Penryn-" TYPE_X86_CPU,\
> +        .property = "model",\
> +        .value    = stringify(2),\
> +    },{\
> +        .driver   = "Penryn-" TYPE_X86_CPU,\
> +        .property = "level",\
> +        .value    = stringify(2),\
> +    },{\
> +        .driver   = "Nehalem-" TYPE_X86_CPU,\
> +        .property = "model",\
> +        .value    = stringify(2),\
> +    },{\
> +        .driver   = "Nehalem-" TYPE_X86_CPU,\
> +        .property = "level",\
> +        .value    = stringify(2),\
> +    },{\
> +        .driver   = "virtio-net-pci",\
> +        .property = "any_layout",\
> +        .value    = "off",\
> +    },{\
> +        .driver = TYPE_X86_CPU,\
> +        .property = "pmu",\
> +        .value = "on",\
> +    },{\
> +        .driver   = "i440FX-pcihost",\
> +        .property = "short_root_bus",\
> +        .value    = stringify(0),\
> +    },{\
> +        .driver   = "q35-pcihost",\
> +        .property = "short_root_bus",\
> +        .value    = stringify(0),\
> +    }
> +
> +#define QEMU_COMPAT_1_4 \
> +    {\
> +        .driver   = "scsi-hd",\
> +        .property = "discard_granularity",\
> +        .value    = stringify(0),\
> +    },{\
> +        .driver   = "scsi-cd",\
> +        .property = "discard_granularity",\
> +        .value    = stringify(0),\
> +    },{\
> +        .driver   = "scsi-disk",\
> +        .property = "discard_granularity",\
> +        .value    = stringify(0),\
> +    },{\
> +        .driver   = "ide-hd",\
> +        .property = "discard_granularity",\
> +        .value    = stringify(0),\
> +    },{\
> +        .driver   = "ide-cd",\
> +        .property = "discard_granularity",\
> +        .value    = stringify(0),\
> +    },{\
> +        .driver   = "ide-drive",\
> +        .property = "discard_granularity",\
> +        .value    = stringify(0),\
> +    },{\
> +        .driver   = "virtio-blk-pci",\
> +        .property = "discard_granularity",\
> +        .value    = stringify(0),\
> +    },{\
> +        .driver   = "virtio-serial-pci",\
> +        .property = "vectors",\
> +        /* DEV_NVECTORS_UNSPECIFIED as a uint32_t string */\
> +        .value    = stringify(0xFFFFFFFF),\
> +    },{ \
> +        .driver   = "virtio-net-pci", \
> +        .property = "ctrl_guest_offloads", \
> +        .value    = "off", \
> +    },{\
> +        .driver   = "e1000",\
> +        .property = "romfile",\
> +        .value    = "pxe-e1000.rom",\
> +    },{\
> +        .driver   = "ne2k_pci",\
> +        .property = "romfile",\
> +        .value    = "pxe-ne2k_pci.rom",\
> +    },{\
> +        .driver   = "pcnet",\
> +        .property = "romfile",\
> +        .value    = "pxe-pcnet.rom",\
> +    },{\
> +        .driver   = "rtl8139",\
> +        .property = "romfile",\
> +        .value    = "pxe-rtl8139.rom",\
> +    },{\
> +        .driver   = "virtio-net-pci",\
> +        .property = "romfile",\
> +        .value    = "pxe-virtio.rom",\
> +    },{\
> +        .driver   = "486-" TYPE_X86_CPU,\
> +        .property = "model",\
> +        .value    = stringify(0),\
> +    }
> +
> +

Two empty lines.

> +#endif
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 1c0c382..5907e99 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -4,6 +4,7 @@
>  #include "qemu-common.h"
>  #include "exec/memory.h"
>  #include "hw/boards.h"
> +#include "hw/compat.h"
>  #include "hw/isa/isa.h"
>  #include "hw/block/fdc.h"
>  #include "net/net.h"
> @@ -295,209 +296,24 @@ int e820_get_num_entries(void);
>  bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>  
>  #define PC_COMPAT_2_0 \
> -        {\
> -            .driver   = "virtio-scsi-pci",\
> -            .property = "any_layout",\
> -            .value    = "off",\
> -        },{\
> -            .driver   = "PIIX4_PM",\
> -            .property = "memory-hotplug-support",\
> -            .value    = "off",\
> -        },\
> -        {\
> -            .driver   = "apic",\
> -            .property = "version",\
> -            .value    = stringify(0x11),\
> -        },\
> -        {\
> -            .driver   = "nec-usb-xhci",\
> -            .property = "superspeed-ports-first",\
> -            .value    = "off",\
> -        },\
> -        {\
> -            .driver   = "pci-serial",\
> -            .property = "prog_if",\
> -            .value    = stringify(0),\
> -        },\
> -        {\
> -            .driver   = "pci-serial-2x",\
> -            .property = "prog_if",\
> -            .value    = stringify(0),\
> -        },\
> -        {\
> -            .driver   = "pci-serial-4x",\
> -            .property = "prog_if",\
> -            .value    = stringify(0),\
> -        },\
> -        {\
> -            .driver   = "virtio-net-pci",\
> -            .property = "guest_announce",\
> -            .value    = "off",\
> -        },\
> -        {\
> -            .driver   = "ICH9-LPC",\
> -            .property = "memory-hotplug-support",\
> -            .value    = "off",\
> -        },{\
> -            .driver   = "xio3130-downstream",\
> -            .property = COMPAT_PROP_PCP,\
> -            .value    = "off",\
> -        },{\
> -            .driver   = "ioh3420",\
> -            .property = COMPAT_PROP_PCP,\
> -            .value    = "off",\
> -        }
> +    QEMU_COMPAT_2_0
>  
>  #define PC_COMPAT_1_7 \
> -        PC_COMPAT_2_0, \
> -        {\
> -            .driver   = TYPE_USB_DEVICE,\
> -            .property = "msos-desc",\
> -            .value    = "no",\
> -        },\
> -        {\
> -            .driver   = "PIIX4_PM",\
> -            .property = "acpi-pci-hotplug-with-bridge-support",\
> -            .value    = "off",\
> -        },\
> -        {\
> -            .driver   = "hpet",\
> -            .property = HPET_INTCAP,\
> -            .value    = stringify(4),\
> -        }
> +    PC_COMPAT_2_0,\
> +    QEMU_COMPAT_1_7
>  
>  #define PC_COMPAT_1_6 \
> -        PC_COMPAT_1_7, \
> -        {\
> -            .driver   = "e1000",\
> -            .property = "mitigation",\
> -            .value    = "off",\
> -        },{\
> -            .driver   = "qemu64-" TYPE_X86_CPU,\
> -            .property = "model",\
> -            .value    = stringify(2),\
> -        },{\
> -            .driver   = "qemu32-" TYPE_X86_CPU,\
> -            .property = "model",\
> -            .value    = stringify(3),\
> -        },{\
> -            .driver   = "i440FX-pcihost",\
> -            .property = "short_root_bus",\
> -            .value    = stringify(1),\
> -        },{\
> -            .driver   = "q35-pcihost",\
> -            .property = "short_root_bus",\
> -            .value    = stringify(1),\
> -        }
> +    PC_COMPAT_1_7,\
> +    QEMU_COMPAT_1_6
>  
>  #define PC_COMPAT_1_5 \
> -        PC_COMPAT_1_6, \
> -        {\
> -            .driver   = "Conroe-" TYPE_X86_CPU,\
> -            .property = "model",\
> -            .value    = stringify(2),\
> -        },{\
> -            .driver   = "Conroe-" TYPE_X86_CPU,\
> -            .property = "level",\
> -            .value    = stringify(2),\
> -        },{\
> -            .driver   = "Penryn-" TYPE_X86_CPU,\
> -            .property = "model",\
> -            .value    = stringify(2),\
> -        },{\
> -            .driver   = "Penryn-" TYPE_X86_CPU,\
> -            .property = "level",\
> -            .value    = stringify(2),\
> -        },{\
> -            .driver   = "Nehalem-" TYPE_X86_CPU,\
> -            .property = "model",\
> -            .value    = stringify(2),\
> -        },{\
> -            .driver   = "Nehalem-" TYPE_X86_CPU,\
> -            .property = "level",\
> -            .value    = stringify(2),\
> -        },{\
> -            .driver   = "virtio-net-pci",\
> -            .property = "any_layout",\
> -            .value    = "off",\
> -        },{\
> -            .driver = TYPE_X86_CPU,\
> -            .property = "pmu",\
> -            .value = "on",\
> -        },{\
> -            .driver   = "i440FX-pcihost",\
> -            .property = "short_root_bus",\
> -            .value    = stringify(0),\
> -        },{\
> -            .driver   = "q35-pcihost",\
> -            .property = "short_root_bus",\
> -            .value    = stringify(0),\
> -        }
> +    PC_COMPAT_1_6,\
> +    QEMU_COMPAT_1_5
>  
>  #define PC_COMPAT_1_4 \
> -        PC_COMPAT_1_5, \
> -        {\
> -            .driver   = "scsi-hd",\
> -            .property = "discard_granularity",\
> -            .value    = stringify(0),\
> -	},{\
> -            .driver   = "scsi-cd",\
> -            .property = "discard_granularity",\
> -            .value    = stringify(0),\
> -	},{\
> -            .driver   = "scsi-disk",\
> -            .property = "discard_granularity",\
> -            .value    = stringify(0),\
> -	},{\
> -            .driver   = "ide-hd",\
> -            .property = "discard_granularity",\
> -            .value    = stringify(0),\
> -	},{\
> -            .driver   = "ide-cd",\
> -            .property = "discard_granularity",\
> -            .value    = stringify(0),\
> -	},{\
> -            .driver   = "ide-drive",\
> -            .property = "discard_granularity",\
> -            .value    = stringify(0),\
> -        },{\
> -            .driver   = "virtio-blk-pci",\
> -            .property = "discard_granularity",\
> -            .value    = stringify(0),\
> -	},{\
> -            .driver   = "virtio-serial-pci",\
> -            .property = "vectors",\
> -            /* DEV_NVECTORS_UNSPECIFIED as a uint32_t string */\
> -            .value    = stringify(0xFFFFFFFF),\
> -        },{ \
> -            .driver   = "virtio-net-pci", \
> -            .property = "ctrl_guest_offloads", \
> -            .value    = "off", \
> -        },{\
> -            .driver   = "e1000",\
> -            .property = "romfile",\
> -            .value    = "pxe-e1000.rom",\
> -        },{\
> -            .driver   = "ne2k_pci",\
> -            .property = "romfile",\
> -            .value    = "pxe-ne2k_pci.rom",\
> -        },{\
> -            .driver   = "pcnet",\
> -            .property = "romfile",\
> -            .value    = "pxe-pcnet.rom",\
> -        },{\
> -            .driver   = "rtl8139",\
> -            .property = "romfile",\
> -            .value    = "pxe-rtl8139.rom",\
> -        },{\
> -            .driver   = "virtio-net-pci",\
> -            .property = "romfile",\
> -            .value    = "pxe-virtio.rom",\
> -        },{\
> -            .driver   = "486-" TYPE_X86_CPU,\
> -            .property = "model",\
> -            .value    = stringify(0),\
> -        }
> +    PC_COMPAT_1_5,\
> +    QEMU_COMPAT_1_4
> +
>  

Two emoty lines.

>  #define PC_COMMON_MACHINE_OPTIONS \
>      .default_boot_order = "cad"
> -- 
> 1.9.3

On Wed, Jun 25, 2014 at 08:56:43AM +0300, Michael S. Tsirkin wrote:
> On Tue, Jun 24, 2014 at 11:04:45PM -0300, Eduardo Habkost wrote:
> > The QEMU_COMPAT_* macros will contain compat properties that are not
> > specific to PC, and may be reused by other machine-types.
> > 
> > The compat properties for PC-specific devices were moved to
> > QEMU_COMPAT_* too, because they are simply not going to be applied to
> > any device if they are not instantiated.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> I don't see value in this.
> If some target wants to start versioning their machine types,
> let them start from 2.0 (or even 2.1 - does any non-PC target
> plan to support cross-version compatibility for 2.0?).
> Why carry around useless code for < 2.0 for non PC platforms?
> 
> > ---
> >  include/hw/compat.h  | 210 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/i386/pc.h | 206 +++-----------------------------------------------
> >  2 files changed, 221 insertions(+), 195 deletions(-)
> >  create mode 100644 include/hw/compat.h
> 
> And we have lost all we have gained in 1/2 :(
> 

As we will be required to follow the QEMU_COMPAT/PC_COMPAT pattern
starting from 2.1, I like to keep exactly the same pattern for all older
macros, even if QEMU_COMPAT_1_* are used only by PC. Makes it easier to
read and understand, and information easier to find (instead of having
the old compat lists scattered all over the place, most of them are now
in a single header file).

Also, note that the PC_COMPAT_* macros will be removed once we convert
PC to pure QOM code after 2.1.0.

Anyway, if the maintainers see no value in having QEMU_COMPAT_* macros
used only by PC, we can do this only after 2.1. Otherwise, if there's
value in a QEMU_COMPAT_2_0 macro used only by PC, there's value in
QEMU_COMPAT_1_* too.

In either case, this is simply a demonstration of how we can proceed to
introduce the versioned non-PC machine-types later. We don't strictly
need this patch today.

> > 
[...]
> > +    }
> > +
> > +
> 
> Two empty lines.

I will fix it in case I submit it again. Thanks.

> 
[...]
> > -        }
> > +    PC_COMPAT_1_5,\
> > +    QEMU_COMPAT_1_4
> > +
> >  
> 
> Two emoty lines.

Ditto.

On Wed, Jun 25, 2014 at 10:23:06AM -0300, Eduardo Habkost wrote:
> On Wed, Jun 25, 2014 at 08:56:43AM +0300, Michael S. Tsirkin wrote:
> > On Tue, Jun 24, 2014 at 11:04:45PM -0300, Eduardo Habkost wrote:
> > > The QEMU_COMPAT_* macros will contain compat properties that are not
> > > specific to PC, and may be reused by other machine-types.
> > > 
> > > The compat properties for PC-specific devices were moved to
> > > QEMU_COMPAT_* too, because they are simply not going to be applied to
> > > any device if they are not instantiated.
> > > 
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > 
> > I don't see value in this.
> > If some target wants to start versioning their machine types,
> > let them start from 2.0 (or even 2.1 - does any non-PC target
> > plan to support cross-version compatibility for 2.0?).
> > Why carry around useless code for < 2.0 for non PC platforms?
> > 
> > > ---
> > >  include/hw/compat.h  | 210 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  include/hw/i386/pc.h | 206 +++-----------------------------------------------
> > >  2 files changed, 221 insertions(+), 195 deletions(-)
> > >  create mode 100644 include/hw/compat.h
> > 
> > And we have lost all we have gained in 1/2 :(
> > 
> 
> As we will be required to follow the QEMU_COMPAT/PC_COMPAT pattern
> starting from 2.1, I like to keep exactly the same pattern for all older
> macros, even if QEMU_COMPAT_1_* are used only by PC. Makes it easier to
> read and understand, and information easier to find (instead of having
> the old compat lists scattered all over the place, most of them are now
> in a single header file).
> 
> Also, note that the PC_COMPAT_* macros will be removed once we convert
> PC to pure QOM code after 2.1.0.
> 
> Anyway, if the maintainers see no value in having QEMU_COMPAT_* macros
> used only by PC, we can do this only after 2.1. Otherwise, if there's
> value in a QEMU_COMPAT_2_0 macro used only by PC, there's value in
> QEMU_COMPAT_1_* too.
> 
> In either case, this is simply a demonstration of how we can proceed to
> introduce the versioned non-PC machine-types later. We don't strictly
> need this patch today.

I think it's best for you guys to hammer on it on a private
branch. It's past devel freeze and this is new code,
so I'm not applying anything that's not trivial.
Patch 1/2 seems to hit the sweet spot.


> > > 
> [...]
> > > +    }
> > > +
> > > +
> > 
> > Two empty lines.
> 
> I will fix it in case I submit it again. Thanks.
> 
> > 
> [...]
> > > -        }
> > > +    PC_COMPAT_1_5,\
> > > +    QEMU_COMPAT_1_4
> > > +
> > >  
> > 
> > Two emoty lines.
> 
> Ditto.
> 
> -- 
> Eduardo