Re: [Qemu-devel] [PATCH] pc: acpi: force FADT rev1 for old i440fx machine types

[Michael S. Tsirkin]

On Mon, Jul 24, 2017 at 08:48:48AM +0200, Igor Mammedov wrote:
> On Sat, 22 Jul 2017 02:40:46 +0300
> "Michael S. Tsirkin" <address@hidden> wrote:
> 
> > On Fri, Jul 21, 2017 at 12:10:48PM +0200, Igor Mammedov wrote:
> > > On Fri, 21 Jul 2017 10:49:55 +0100
> > > "Daniel P. Berrange" <address@hidden> wrote:
> > >   
> > > > On Fri, Jul 21, 2017 at 11:32:11AM +0200, Igor Mammedov wrote:  
> > > > > w2k used to boot on QEMU until we bumped revision of FADT to rev3
> > > > > (commit 77af8a2b hw/i386: Use Rev3 FADT (ACPI 2.0) instead of Rev1 to 
> > > > > improve guest OS support.)
> > > > > 
> > > > > Considering that w2k is ancient and long time EOLed, leave default
> > > > > rev3 but make pc-i440fx-2.9 and older machine types to force rev1
> > > > > so old setups won't break (w2k could boot).    
> > > > 
> > > > There needs to be a machine type property added to control this
> > > > feature. When provisioning new VMs, management apps need to be
> > > > able to set the property explicitly - having them rely on picking
> > > > particular machine type name+versions is not viable, because
> > > > downstream vendors replace the machine types with their own
> > > > names + versions.  
> > > having property doesn't really help here and we don't do it for every
> > > compat tweak /ex: save_tsc_khz, linuxboot_dma_enabled/.
> > > 
> > > Management would not benefit much from having property vs machine version
> > > as it would have to encode somewhere that for w2k it should set
> > > some machine property or pick a particular machine type.  
> > 
> > I think I'd disagree with that. If
> > users might need this for compatibility with some guests,
> > then it should be a property not just a machine type.
> > 
> > But see below - I think we rushed it for the PC anyway.
> > 
> > > Probably no one would worry about fixing virt-install or something
> > > else for the sake of w2k and if they are going to fix it
> > > it doesn't matter if they map machine type vs property.
> > > 
> > > Also with new machine type deprecation policy we would be able
> > > easily to phase out rev1 support along with 2.9 machine,
> > > but if you expose property then removing it would break
> > > CLI not only for 2.9 but possible later machines if it's set there.
> > > 
> > > So I'm against adding properties/CLI options for unless we have to in 
> > > this case,
> > > and I'm not convinced that w2k deserves it.  
> > 
> > If I have to choose, I'd say Mac OSX is way less interesting than old
> > windows versions. Lots of people have software that will only run on old
> > windows and there's probably good money to be had running it on new
> > hardware in VMs. And PC machine is all about compatibility - we have Q35
> > for new stuff.  Besides OSX uses q35 anyway I think.
> Question is for how long we are going to maintain legacy stuff,
> ACPI spec periodically adds new stuff, which someday is going
> to break legacy OSes. And maintaining 2 branches of or worse
> a mix will cost us in time and future regressions, we need to
> have some policy to cut off legacy features that hold us back,
> like we are starting to do with machine types.
> w2k has been EOLed in 2010 even if we drop its support now,
> users still can use it as they have an option to use old QEMU
> for that.

Users can't normally get old QEMU, even if they could it is not
a good idea because of security.

> (Ladi said that w2k fails to install since 2.7).

Bisect will likely find what's wrong.


> > 
> > So maybe the right thing to do is to
> > - switch default for PC back to rev 1
> > - keep default for Q35 at rev 3
> > 
> > No machinetype hacks.
> it's still machine hack pc vs q35 and an extra branch to look
> after but it's better than an option, I'll respin patch.
> 
> > 
> > > > > 
> > > > > Signed-off-by: Igor Mammedov <address@hidden>
> > > > > ---
> > > > > CC: Programmingkid <address@hidden>
> > > > > CC: Phil Dennis-Jordan <address@hidden>
> > > > > CC: "Michael S. Tsirkin" <address@hidden>
> > > > > 
> > > > > Only compile test since I don't have w2k to test with
> > > > > 
> > > > > ---
> > > > >  include/hw/i386/pc.h |  1 +
> > > > >  hw/i386/acpi-build.c | 26 +++++++++++++++++++-------
> > > > >  hw/i386/pc_piix.c    |  2 ++
> > > > >  3 files changed, 22 insertions(+), 7 deletions(-)
> > > > > 
> > > > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > > > index d80859b..d6f65dd 100644
> > > > > --- a/include/hw/i386/pc.h
> > > > > +++ b/include/hw/i386/pc.h
> > > > > @@ -122,6 +122,7 @@ struct PCMachineClass {
> > > > >      bool rsdp_in_ram;
> > > > >      int legacy_acpi_table_size;
> > > > >      unsigned acpi_data_size;
> > > > > +    bool force_rev1_fadt;
> > > > >  
> > > > >      /* SMBIOS compat: */
> > > > >      bool smbios_defaults;
> > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > > index 6b7bade..227f9ad 100644
> > > > > --- a/hw/i386/acpi-build.c
> > > > > +++ b/hw/i386/acpi-build.c
> > > > > @@ -272,7 +272,7 @@ build_facs(GArray *table_data, BIOSLinker *linker)
> > > > >  }
> > > > >  
> > > > >  /* Load chipset information in FADT */
> > > > > -static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiPmInfo *pm)
> > > > > +static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiPmInfo *pm, 
> > > > > bool rev1)
> > > > >  {
> > > > >      fadt->model = 1;
> > > > >      fadt->reserved1 = 0;
> > > > > @@ -304,6 +304,9 @@ static void fadt_setup(AcpiFadtDescriptorRev3 
> > > > > *fadt, AcpiPmInfo *pm)
> > > > >          fadt->flags |= cpu_to_le32(1 << 
> > > > > ACPI_FADT_F_FORCE_APIC_CLUSTER_MODEL);
> > > > >      }
> > > > >      fadt->century = RTC_CENTURY;
> > > > > +    if (rev1) {
> > > > > +        return;
> > > > > +    }
> > > > >  
> > > > >      fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_RESET_REG_SUP);
> > > > >      fadt->reset_value = 0xf;
> > > > > @@ -335,6 +338,7 @@ static void fadt_setup(AcpiFadtDescriptorRev3 
> > > > > *fadt, AcpiPmInfo *pm)
> > > > >  /* FADT */
> > > > >  static void
> > > > >  build_fadt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm,
> > > > > +           MachineState *machine,
> > > > >             unsigned facs_tbl_offset, unsigned dsdt_tbl_offset,
> > > > >             const char *oem_id, const char *oem_table_id)
> > > > >  {
> > > > > @@ -342,6 +346,9 @@ build_fadt(GArray *table_data, BIOSLinker 
> > > > > *linker, AcpiPmInfo *pm,
> > > > >      unsigned fw_ctrl_offset = (char *)&fadt->firmware_ctrl - 
> > > > > table_data->data;
> > > > >      unsigned dsdt_entry_offset = (char *)&fadt->dsdt - 
> > > > > table_data->data;
> > > > >      unsigned xdsdt_entry_offset = (char *)&fadt->x_dsdt - 
> > > > > table_data->data;
> > > > > +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
> > > > > +    int fadt_size = sizeof(*fadt);
> > > > > +    int rev = 3;
> > > > >  
> > > > >      /* FACS address to be filled by Guest linker */
> > > > >      bios_linker_loader_add_pointer(linker,
> > > > > @@ -349,16 +356,21 @@ build_fadt(GArray *table_data, BIOSLinker 
> > > > > *linker, AcpiPmInfo *pm,
> > > > >          ACPI_BUILD_TABLE_FILE, facs_tbl_offset);
> > > > >  
> > > > >      /* DSDT address to be filled by Guest linker */
> > > > > -    fadt_setup(fadt, pm);
> > > > > +    fadt_setup(fadt, pm, pcmc->force_rev1_fadt);
> > > > >      bios_linker_loader_add_pointer(linker,
> > > > >          ACPI_BUILD_TABLE_FILE, dsdt_entry_offset, sizeof(fadt->dsdt),
> > > > >          ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
> > > > > -    bios_linker_loader_add_pointer(linker,
> > > > > -        ACPI_BUILD_TABLE_FILE, xdsdt_entry_offset, 
> > > > > sizeof(fadt->x_dsdt),
> > > > > -        ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
> > > > > +    if (pcmc->force_rev1_fadt) {
> > > > > +        rev = 1;
> > > > > +        fadt_size = offsetof(typeof(*fadt), reset_register);
> > > > > +    } else {
> > > > > +        bios_linker_loader_add_pointer(linker,
> > > > > +            ACPI_BUILD_TABLE_FILE, xdsdt_entry_offset, 
> > > > > sizeof(fadt->x_dsdt),
> > > > > +            ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
> > > > > +    }
> > > > >  
> > > > >      build_header(linker, table_data,
> > > > > -                 (void *)fadt, "FACP", sizeof(*fadt), 3, oem_id, 
> > > > > oem_table_id);
> > > > > +                 (void *)fadt, "FACP", fadt_size, rev, oem_id, 
> > > > > oem_table_id);
> > > > >  }
> > > > >  
> > > > >  void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
> > > > > @@ -2667,7 +2679,7 @@ void acpi_build(AcpiBuildTables *tables, 
> > > > > MachineState *machine)
> > > > >      /* ACPI tables pointed to by RSDT */
> > > > >      fadt = tables_blob->len;
> > > > >      acpi_add_table(table_offsets, tables_blob);
> > > > > -    build_fadt(tables_blob, tables->linker, &pm, facs, dsdt,
> > > > > +    build_fadt(tables_blob, tables->linker, &pm, machine, facs, dsdt,
> > > > >                 slic_oem.id, slic_oem.table_id);
> > > > >      aml_len += tables_blob->len - fadt;
> > > > >  
> > > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > > > > index 11b4336..bc61332 100644
> > > > > --- a/hw/i386/pc_piix.c
> > > > > +++ b/hw/i386/pc_piix.c
> > > > > @@ -449,9 +449,11 @@ DEFINE_I440FX_MACHINE(v2_10, "pc-i440fx-2.10", 
> > > > > NULL,
> > > > >  
> > > > >  static void pc_i440fx_2_9_machine_options(MachineClass *m)
> > > > >  {
> > > > > +    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> > > > >      pc_i440fx_2_10_machine_options(m);
> > > > >      m->is_default = 0;
> > > > >      m->alias = NULL;
> > > > > +    pcmc->force_rev1_fadt = true;
> > > > >      SET_MACHINE_COMPAT(m, PC_COMPAT_2_9);  
> > 
> > whatever switch we use for this option, I think it makes
> > sense to add comments to document why we keep this around.
> ok
> 
> > 
> > 
> > > > >  }
> > > > >  
> > > > > -- 
> > > > > 2.7.4
> > > > > 
> > > > >     
> > > > 
> > > > Regards,
> > > > Daniel  
> >