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

[Igor Mammedov]

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.

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.


> > 
> > 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);
> >  }
> >  
> > -- 
> > 2.7.4
> > 
> >   
> 
> Regards,
> Daniel