Re: [Qemu-devel] [PATCH 3/4] pc: Add 2.9 machine-types

[Laszlo Ersek <lersek@redhat.com>]

On 12/27/16 20:21, Eduardo Habkost wrote:
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  include/hw/i386/pc.h |  1 +
>  hw/i386/pc_piix.c    | 15 ++++++++++++---
>  hw/i386/pc_q35.c     | 13 +++++++++++--
>  3 files changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index b22e699..ceeacca 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -375,6 +375,7 @@ int e820_get_num_entries(void);
>  bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>  
>  #define PC_COMPAT_2_8 \
> +    HW_COMPAT_2_8
>  
>  #define PC_COMPAT_2_7 \
>      HW_COMPAT_2_7 \

Here I would recommend two things:
- introduce an empty PC_COMPAT_2_9 macro
- in the PC_COMPAT_2_8 macro being modified, add a trailing backslash
  after HW_COMPAT_2_8

Both of these aim at keeping up the current pattern; namely, for the
most recent machine type, a PC_COMPAT macro should exist (even if empty
/ unused), plus each PC_COMPAT macro should be extensible by plain
appending (hence the final trailing backslash).

Functionally the above hunk is good of course, so I'll leave it to you
whether you actually want to address my comments.

The rest looks fine; I should be able to rebase my broadcast SMI (v5)
stuff to this.

Changed or unchanged:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 5e1adbe..9f102aa 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -437,13 +437,24 @@ static void pc_i440fx_machine_options(MachineClass *m)
>      m->default_display = "std";
>  }
>  
> -static void pc_i440fx_2_8_machine_options(MachineClass *m)
> +static void pc_i440fx_2_9_machine_options(MachineClass *m)
>  {
>      pc_i440fx_machine_options(m);
>      m->alias = "pc";
>      m->is_default = 1;
>  }
>  
> +DEFINE_I440FX_MACHINE(v2_9, "pc-i440fx-2.9", NULL,
> +                      pc_i440fx_2_9_machine_options);
> +
> +static void pc_i440fx_2_8_machine_options(MachineClass *m)
> +{
> +    pc_i440fx_2_9_machine_options(m);
> +    m->is_default = 0;
> +    m->alias = NULL;
> +    SET_MACHINE_COMPAT(m, PC_COMPAT_2_8);
> +}
> +
>  DEFINE_I440FX_MACHINE(v2_8, "pc-i440fx-2.8", NULL,
>                        pc_i440fx_2_8_machine_options);
>  
> @@ -451,8 +462,6 @@ DEFINE_I440FX_MACHINE(v2_8, "pc-i440fx-2.8", NULL,
>  static void pc_i440fx_2_7_machine_options(MachineClass *m)
>  {
>      pc_i440fx_2_8_machine_options(m);
> -    m->is_default = 0;
> -    m->alias = NULL;
>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_7);
>  }
>  
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index d042fe0..dd792a8 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -301,19 +301,28 @@ static void pc_q35_machine_options(MachineClass *m)
>      m->max_cpus = 288;
>  }
>  
> -static void pc_q35_2_8_machine_options(MachineClass *m)
> +static void pc_q35_2_9_machine_options(MachineClass *m)
>  {
>      pc_q35_machine_options(m);
>      m->alias = "q35";
>  }
>  
> +DEFINE_Q35_MACHINE(v2_9, "pc-q35-2.9", NULL,
> +                   pc_q35_2_9_machine_options);
> +
> +static void pc_q35_2_8_machine_options(MachineClass *m)
> +{
> +    pc_q35_2_9_machine_options(m);
> +    m->alias = NULL;
> +    SET_MACHINE_COMPAT(m, PC_COMPAT_2_8);
> +}
> +
>  DEFINE_Q35_MACHINE(v2_8, "pc-q35-2.8", NULL,
>                     pc_q35_2_8_machine_options);
>  
>  static void pc_q35_2_7_machine_options(MachineClass *m)
>  {
>      pc_q35_2_8_machine_options(m);
> -    m->alias = NULL;
>      m->max_cpus = 255;
>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_7);
>  }
>