[RESEND] q35: Remove old machine versions

Migration with q35 was not possible before commit
04329029a8c539eb5f75dcb6d8b016f0c53a031a, because q35 unconditionally
creates an ich9-ahci device, that was marked as unmigratable. So all q35
machine classes before pc-q35-2.4 were not migratable, so there's no
point in keeping compatibility code for them.

Remove all old pc-q35 machine classes and keep only pc-q35-2.4.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Resubmitting this series, as I didn't see any convincing argument
to keep the old code around.
---
 hw/i386/pc_q35.c | 163 -------------------------------------------------------
 1 file changed, 163 deletions(-)

On 12/17/2015 12:27 PM, Eduardo Habkost wrote:
> Migration with q35 was not possible before commit
> 04329029a8c539eb5f75dcb6d8b016f0c53a031a, because q35 unconditionally
> creates an ich9-ahci device, that was marked as unmigratable. So all q35
> machine classes before pc-q35-2.4 were not migratable, so there's no
> point in keeping compatibility code for them.
> 
> Remove all old pc-q35 machine classes and keep only pc-q35-2.4.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Resubmitting this series, as I didn't see any convincing argument
> to keep the old code around.
> ---
>  hw/i386/pc_q35.c | 163 -------------------------------------------------------
>  1 file changed, 163 deletions(-)
> 
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 9a12068..4156f5e 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -277,75 +277,6 @@ static void pc_q35_init(MachineState *machine)
>      }
>  }
>  
> -/* Looking for a pc_compat_2_4() function? It doesn't exist.
> - * pc_compat_*() functions that run on machine-init time and
> - * change global QEMU state are deprecated. Please don't create
> - * one, and implement any pc-*-2.4 (and newer) compat code in
> - * HW_COMPAT_*, PC_COMPAT_*, or * pc_*_machine_options().
> - */
> -
> -static void pc_compat_2_3(MachineState *machine)
> -{
> -    PCMachineState *pcms = PC_MACHINE(machine);
> -    savevm_skip_section_footers();
> -    if (kvm_enabled()) {
> -        pcms->smm = ON_OFF_AUTO_OFF;
> -    }
> -    global_state_set_optional();
> -    savevm_skip_configuration();
> -}
> -
> -static void pc_compat_2_2(MachineState *machine)
> -{
> -    pc_compat_2_3(machine);
> -    rsdp_in_ram = false;
> -    machine->suppress_vmdesc = true;
> -}
> -
> -static void pc_compat_2_1(MachineState *machine)
> -{
> -    PCMachineState *pcms = PC_MACHINE(machine);
> -
> -    pc_compat_2_2(machine);
> -    pcms->enforce_aligned_dimm = false;
> -    smbios_uuid_encoded = false;
> -    x86_cpu_change_kvm_default("svm", NULL);
> -}
> -
> -static void pc_compat_2_0(MachineState *machine)
> -{
> -    pc_compat_2_1(machine);
> -    smbios_legacy_mode = true;
> -    has_reserved_memory = false;
> -    pc_set_legacy_acpi_data_size();
> -}
> -
> -static void pc_compat_1_7(MachineState *machine)
> -{
> -    pc_compat_2_0(machine);
> -    smbios_defaults = false;
> -    gigabyte_align = false;
> -    option_rom_has_mr = true;
> -    x86_cpu_change_kvm_default("x2apic", NULL);
> -}
> -
> -static void pc_compat_1_6(MachineState *machine)
> -{
> -    pc_compat_1_7(machine);
> -    rom_file_has_mr = false;
> -    has_acpi_build = false;
> -}
> -
> -static void pc_compat_1_5(MachineState *machine)
> -{
> -    pc_compat_1_6(machine);
> -}
> -
> -static void pc_compat_1_4(MachineState *machine)
> -{
> -    pc_compat_1_5(machine);
> -}
> -
>  #define DEFINE_Q35_MACHINE(suffix, name, compatfn, optionfn) \
>      static void pc_init_##suffix(MachineState *machine) \
>      { \
> @@ -391,97 +322,3 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
>  
>  DEFINE_Q35_MACHINE(v2_4, "pc-q35-2.4", NULL,
>                     pc_q35_2_4_machine_options);
> -
> -
> -static void pc_q35_2_3_machine_options(MachineClass *m)
> -{
> -    pc_q35_2_4_machine_options(m);
> -    m->hw_version = "2.3.0";
> -    m->no_floppy = 0;
> -    m->no_tco = 1;
> -    m->alias = NULL;
> -    SET_MACHINE_COMPAT(m, PC_COMPAT_2_3);
> -}
> -
> -DEFINE_Q35_MACHINE(v2_3, "pc-q35-2.3", pc_compat_2_3,
> -                   pc_q35_2_3_machine_options);
> -
> -
> -static void pc_q35_2_2_machine_options(MachineClass *m)
> -{
> -    pc_q35_2_3_machine_options(m);
> -    m->hw_version = "2.2.0";
> -    SET_MACHINE_COMPAT(m, PC_COMPAT_2_2);
> -}
> -
> -DEFINE_Q35_MACHINE(v2_2, "pc-q35-2.2", pc_compat_2_2,
> -                   pc_q35_2_2_machine_options);
> -
> -
> -static void pc_q35_2_1_machine_options(MachineClass *m)
> -{
> -    pc_q35_2_2_machine_options(m);
> -    m->hw_version = "2.1.0";
> -    m->default_display = NULL;
> -    SET_MACHINE_COMPAT(m, PC_COMPAT_2_1);
> -}
> -
> -DEFINE_Q35_MACHINE(v2_1, "pc-q35-2.1", pc_compat_2_1,
> -                   pc_q35_2_1_machine_options);
> -
> -
> -static void pc_q35_2_0_machine_options(MachineClass *m)
> -{
> -    pc_q35_2_1_machine_options(m);
> -    m->hw_version = "2.0.0";
> -    SET_MACHINE_COMPAT(m, PC_COMPAT_2_0);
> -}
> -
> -DEFINE_Q35_MACHINE(v2_0, "pc-q35-2.0", pc_compat_2_0,
> -                   pc_q35_2_0_machine_options);
> -
> -
> -static void pc_q35_1_7_machine_options(MachineClass *m)
> -{
> -    pc_q35_2_0_machine_options(m);
> -    m->hw_version = "1.7.0";
> -    m->default_machine_opts = NULL;
> -    SET_MACHINE_COMPAT(m, PC_COMPAT_1_7);
> -}
> -
> -DEFINE_Q35_MACHINE(v1_7, "pc-q35-1.7", pc_compat_1_7,
> -                   pc_q35_1_7_machine_options);
> -
> -
> -static void pc_q35_1_6_machine_options(MachineClass *m)
> -{
> -    pc_q35_machine_options(m);
> -    m->hw_version = "1.6.0";
> -    SET_MACHINE_COMPAT(m, PC_COMPAT_1_6);
> -}
> -
> -DEFINE_Q35_MACHINE(v1_6, "pc-q35-1.6", pc_compat_1_6,
> -                   pc_q35_1_6_machine_options);
> -
> -
> -static void pc_q35_1_5_machine_options(MachineClass *m)
> -{
> -    pc_q35_1_6_machine_options(m);
> -    m->hw_version = "1.5.0";
> -    SET_MACHINE_COMPAT(m, PC_COMPAT_1_5);
> -}
> -
> -DEFINE_Q35_MACHINE(v1_5, "pc-q35-1.5", pc_compat_1_5,
> -                   pc_q35_1_5_machine_options);
> -
> -
> -static void pc_q35_1_4_machine_options(MachineClass *m)
> -{
> -    pc_q35_1_5_machine_options(m);
> -    m->hw_version = "1.4.0";
> -    m->hot_add_cpu = NULL;
> -    SET_MACHINE_COMPAT(m, PC_COMPAT_1_4);
> -}
> -
> -DEFINE_Q35_MACHINE(v1_4, "pc-q35-1.4", pc_compat_1_4,
> -                   pc_q35_1_4_machine_options);
> 

To recap, the main objection last time was that it might upset existing
libvirt configurations because libvirt saves the machine types in the
XML, and this will cause existing configurations to be unable to boot.

There was some discussion over "fudging" it so that we silently forced
users onto 2.4+, but this was rejected as a bad idea in favor of just
simply breaking anybody relying on pre-2.4 machine types.

So the question becomes:

What do we gain by deleting this? Is cleaning up this code worth
breaking some configurations?

On Thu, Dec 17, 2015 at 12:36:11PM -0500, John Snow wrote:
> 
> 
> On 12/17/2015 12:27 PM, Eduardo Habkost wrote:
> > Migration with q35 was not possible before commit
> > 04329029a8c539eb5f75dcb6d8b016f0c53a031a, because q35 unconditionally
> > creates an ich9-ahci device, that was marked as unmigratable. So all q35
> > machine classes before pc-q35-2.4 were not migratable, so there's no
> > point in keeping compatibility code for them.
> > 
> > Remove all old pc-q35 machine classes and keep only pc-q35-2.4.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Resubmitting this series, as I didn't see any convincing argument
> > to keep the old code around.
> > ---
> >  hw/i386/pc_q35.c | 163 -------------------------------------------------------
> >  1 file changed, 163 deletions(-)
> > 
[...]
> 
> To recap, the main objection last time was that it might upset existing
> libvirt configurations because libvirt saves the machine types in the
> XML, and this will cause existing configurations to be unable to boot.
> 
> There was some discussion over "fudging" it so that we silently forced
> users onto 2.4+, but this was rejected as a bad idea in favor of just
> simply breaking anybody relying on pre-2.4 machine types.
> 
> So the question becomes:
> 
> What do we gain by deleting this? Is cleaning up this code worth
> breaking some configurations?

Developer time. We don't waste time reviewing, refactoring, and
fixing compat code that nobody should ever run. See, for example,
the patch sent by Gerd today:
  [PATCH 6/6] q35: skip q35-acpi-dsdt.aml load if not needed

On Do, 2015-12-17 at 15:27 -0200, Eduardo Habkost wrote:
> Migration with q35 was not possible before commit
> 04329029a8c539eb5f75dcb6d8b016f0c53a031a, because q35 unconditionally
> creates an ich9-ahci device, that was marked as unmigratable. So all q35
> machine classes before pc-q35-2.4 were not migratable, so there's no
> point in keeping compatibility code for them.
> 
> Remove all old pc-q35 machine classes and keep only pc-q35-2.4.

> -static void pc_compat_1_6(MachineState *machine)
> -{
> -    pc_compat_1_7(machine);
> -    rom_file_has_mr = false;
> -    has_acpi_build = false;

After applying this patch has_acpi_build is always true and can be
dropped, together with some other code elsewhere which depends on
has_acpi_build.  The same is probably true for other variables as well
(gigabyte_align?).  Guess it's best to do this as one-per-variable
incremental patches.

Reviewed-by: Gerd Hoffmann <kraxel@gmail.com>

On Fri, Dec 18, 2015 at 10:41:45AM +0100, Gerd Hoffmann wrote:
> On Do, 2015-12-17 at 15:27 -0200, Eduardo Habkost wrote:
> > Migration with q35 was not possible before commit
> > 04329029a8c539eb5f75dcb6d8b016f0c53a031a, because q35 unconditionally
> > creates an ich9-ahci device, that was marked as unmigratable. So all q35
> > machine classes before pc-q35-2.4 were not migratable, so there's no
> > point in keeping compatibility code for them.
> > 
> > Remove all old pc-q35 machine classes and keep only pc-q35-2.4.
> 
> > -static void pc_compat_1_6(MachineState *machine)
> > -{
> > -    pc_compat_1_7(machine);
> > -    rom_file_has_mr = false;
> > -    has_acpi_build = false;
> 
> After applying this patch has_acpi_build is always true and can be
> dropped, together with some other code elsewhere which depends on
> has_acpi_build.  The same is probably true for other variables as well
> (gigabyte_align?).  Guess it's best to do this as one-per-variable
> incremental patches.

The static variables will be replaced by PCMachineClass fields. See:

  Subject: [Qemu-devel] [PATCH v3 0/6] pc: Initialization and compat function cleanup
  Date: Tue,  1 Dec 2015 20:58:02 -0200
  Message-Id: <1449010688-19205-1-git-send-email-ehabkost@redhat.com>

> 
> Reviewed-by: Gerd Hoffmann <kraxel@gmail.com>

Thanks!