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

[Daniel P. Berrange]

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.

> 
> 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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|