Re: [Qemu-arm] [PATCH v5 2/2] ACPI: Add -acpitable fadt= to allow FADT revision changes

[Igor Mammedov]

On Thu, 11 Aug 2016 17:36:45 +0800
Lv Zheng <address@hidden> wrote:

> This patch allows FADT to be built with different revisions. When the revision
> is greater than 1.0, 64-bit address fields may also be filled.
> 
> Note that FADT revision 2 has never been defined by the ACPI specification. So
> this patch only adds an option -acpitable fadt= to allow revision 1,3,5 to be
> configured by the users.
> 
> 1. Tested by booting a linux image, the 64-bit addresses are correctly filled
>    in the dumped FADT.
> 2. Tested by booting a Windows image, no boot failure can be seen.

series still doesn't say why do you need 64-bit addresses in FADT,
current Seabios/OVMF allocates tables below 4Gb and gpe/pm
register blocks are below 4gb as well so there is no point in
accepting theses patches and adding extra CLI options as these
patches do.

If we'd ever need to bump FADT revision, I'd first try to increase it
unconditionally and test/see if it would not upset legacy guests
Windows starting from XP, RHEL4/5/6
If it works then these patches are not necessary,
(this approach worked  just fine for bumping up DSDT revision to 2
it would be possible to use 64-bit math in ASL/AML).

if it doesn't, I still won't do it this way but rather:
 - make new revision used by default
 - add compat property to piix4_pm/ich9-lpc to force legacy revision
 - turn on legacy revision for old machine types using added above property

That way we won't regress existing setups as old machines will stay
the same and only new machine types will be affected. And users that
actually want to play with legacy revision on new machine types could
force it by using above property.

> 
> Signed-off-by: Lv Zheng <address@hidden>
> ---
>  hw/acpi/core.c         |   20 ++++++++++++-
>  hw/i386/acpi-build.c   |   76 
> ++++++++++++++++++++++++++++++++++++++++++------
>  include/hw/acpi/acpi.h |    1 +
>  qemu-options.hx        |    8 ++++-
>  4 files changed, 94 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index 85e0e94..832c86b 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -19,6 +19,7 @@
>   * GNU GPL, version 2 or (at your option) any later version.
>   */
>  #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
>  #include "sysemu/sysemu.h"
>  #include "hw/hw.h"
>  #include "hw/i386/pc.h"
> @@ -54,6 +55,7 @@ static const char unsigned dfl_hdr[ACPI_TABLE_HDR_SIZE - 
> ACPI_TABLE_PFX_SIZE] =
>  
>  char unsigned *acpi_tables;
>  size_t acpi_tables_len;
> +uint8_t acpi_fadt_rev = 1;
>  
>  static QemuOptsList qemu_acpi_opts = {
>      .name = "acpi",
> @@ -312,7 +314,23 @@ void acpi_table_add(const QemuOpts *opts, Error **errp)
>          return;
>      }
>  
> -    error_setg(errp, "'-acpitable' requires one of 'data' or 'file'");
> +    val = qemu_opt_get((QemuOpts *)opts, "fadt");
> +    if (val) {
> +        unsigned long rev;
> +        int err;
> +
> +        err = qemu_strtoul(val, NULL, 10, &rev);
> +        if (err ||
> +            (rev != 1 && rev != 3 && rev != 5)) {
> +            error_setg(errp, "Unsupported FADT revision %s, "
> +                       "please specify 1,3,5", val);
> +            return;
> +        }
> +        acpi_fadt_rev = rev;
> +        return;
> +    }
> +
> +    error_setg(errp, "'-acpitable' requires one of 'data','file' or 'fadt'");
>  }
>  
>  static bool acpi_table_builtin = false;
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index ce7cbc5..8be6578 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -276,8 +276,22 @@ build_facs(GArray *table_data, BIOSLinker *linker)
>      facs->length = cpu_to_le32(sizeof(*facs));
>  }
>  
> +/* GAS */
> +static void
> +build_gas(struct AcpiGenericAddress *gas, uint8_t space_id,
> +          uint8_t bit_width, uint8_t bit_offset,
> +          uint8_t access_width, uint64_t address)
> +{
> +    gas->space_id = space_id;
> +    gas->bit_width = bit_width;
> +    gas->bit_offset = bit_offset;
> +    gas->access_width = access_width;
> +    gas->address = address;
> +}
> +
>  /* Load chipset information in FADT */
> -static void fadt_setup(AcpiFadtDescriptorRev1 *fadt, AcpiPmInfo *pm)
> +static void fadt_setup(AcpiFadtDescriptorRev5_1 *fadt, AcpiPmInfo *pm,
> +                       uint8_t revision)
>  {
>      fadt->model = 1;
>      fadt->reserved1 = 0;
> @@ -309,6 +323,25 @@ static void fadt_setup(AcpiFadtDescriptorRev1 *fadt, 
> AcpiPmInfo *pm)
>          fadt->flags |= cpu_to_le32(1 << 
> ACPI_FADT_F_FORCE_APIC_CLUSTER_MODEL);
>      }
>      fadt->century = RTC_CENTURY;
> +
> +    /* Build ACPI 2.0 (FADT version 3+) GAS fields */
> +    if (revision >= 3) {
> +        /* EVT, CNT, TMR register matches hw/acpi/core.c */
> +        build_gas(&fadt->xpm1a_event_block, AML_SYSTEM_IO,
> +                  32, 0, 0, cpu_to_le64(pm->io_base));
> +        build_gas(&fadt->xpm1a_control_block, AML_SYSTEM_IO,
> +                  16, 0, 0, cpu_to_le64(pm->io_base + 0x04));
> +        build_gas(&fadt->xpm_timer_block, AML_SYSTEM_IO,
> +                  32, 0, 0, cpu_to_le64(pm->io_base + 0x08));
> +        build_gas(&fadt->xgpe0_block, AML_SYSTEM_IO,
> +                  pm->gpe0_blk_len << 3, 0, 0, cpu_to_le64(pm->gpe0_blk));
> +    }
> +
> +    /* Build dummy ACPI 5.0 fields */
> +    if (revision >= 5) {
> +        build_gas(&fadt->sleep_control, AML_SYSTEM_MEMORY, 0, 0, 0, 0);
> +        build_gas(&fadt->sleep_status, AML_SYSTEM_MEMORY, 0, 0, 0, 0);
> +    }
>  }
>  
>  
> @@ -316,11 +349,21 @@ static void fadt_setup(AcpiFadtDescriptorRev1 *fadt, 
> AcpiPmInfo *pm)
>  static void
>  build_fadt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm,
>             unsigned facs_tbl_offset, unsigned dsdt_tbl_offset,
> -           const char *oem_id, const char *oem_table_id)
> +           const char *oem_id, const char *oem_table_id, uint8_t revision)
>  {
> -    AcpiFadtDescriptorRev1 *fadt = acpi_data_push(table_data, sizeof(*fadt));
> -    unsigned fw_ctrl_offset = (char *)&fadt->firmware_ctrl - 
> table_data->data;
> -    unsigned dsdt_entry_offset = (char *)&fadt->dsdt - table_data->data;
> +    AcpiFadtDescriptorRev5_1 *fadt;
> +    unsigned fw_ctrl_offset;
> +    unsigned dsdt_entry_offset;
> +    unsigned fadt_len;
> +
> +    if (revision == 1) {
> +        fadt_len = sizeof(AcpiFadtDescriptorRev1);
> +    } else {
> +        fadt_len = sizeof(AcpiFadtDescriptorRev5_1);
> +    }
> +    fadt = acpi_data_push(table_data, fadt_len);
> +    fw_ctrl_offset = (char *)&fadt->firmware_ctrl - table_data->data;
> +    dsdt_entry_offset = (char *)&fadt->dsdt - table_data->data;
>  
>      /* FACS address to be filled by Guest linker */
>      bios_linker_loader_add_pointer(linker,
> @@ -328,13 +371,28 @@ 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);
>      bios_linker_loader_add_pointer(linker,
>          ACPI_BUILD_TABLE_FILE, dsdt_entry_offset, sizeof(fadt->dsdt),
>          ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
>  
> -    build_header(linker, table_data,
> -                 (void *)fadt, "FACP", sizeof(*fadt), 1, oem_id, 
> oem_table_id);
> +    if (revision > 2) {
> +        fw_ctrl_offset = (char *)&fadt->Xfacs - table_data->data;
> +        dsdt_entry_offset = (char *)&fadt->Xdsdt - table_data->data;
> +
> +        /* FACS address to be filled by Guest linker */
> +        bios_linker_loader_add_pointer(linker,
> +            ACPI_BUILD_TABLE_FILE, fw_ctrl_offset, sizeof(fadt->Xfacs),
> +            ACPI_BUILD_TABLE_FILE, facs_tbl_offset);
> +
> +        /* DSDT address to be filled by Guest linker */
> +        bios_linker_loader_add_pointer(linker,
> +            ACPI_BUILD_TABLE_FILE, dsdt_entry_offset, sizeof(fadt->Xdsdt),
> +            ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
> +    }
> +
> +    fadt_setup(fadt, pm, revision);
> +    build_header(linker, table_data, (void *)fadt, "FACP", fadt_len,
> +                 revision, oem_id, oem_table_id);
>  }
>  
>  void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
> @@ -2681,7 +2739,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState 
> *machine)
>      fadt = tables_blob->len;
>      acpi_add_table(table_offsets, tables_blob);
>      build_fadt(tables_blob, tables->linker, &pm, facs, dsdt,
> -               slic_oem.id, slic_oem.table_id);
> +               slic_oem.id, slic_oem.table_id, acpi_fadt_rev);
>      aml_len += tables_blob->len - fadt;
>  
>      acpi_add_table(table_offsets, tables_blob);
> diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
> index 7b3d93c..63df38d 100644
> --- a/include/hw/acpi/acpi.h
> +++ b/include/hw/acpi/acpi.h
> @@ -173,6 +173,7 @@ void acpi_update_sci(ACPIREGS *acpi_regs, qemu_irq irq);
>  
>  /* acpi.c */
>  extern int acpi_enabled;
> +extern uint8_t acpi_fadt_rev;
>  extern char unsigned *acpi_tables;
>  extern size_t acpi_tables_len;
>  
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 5fe7f87..d61dd92 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1497,7 +1497,10 @@ DEF("acpitable", HAS_ARG, QEMU_OPTION_acpitable,
>      "             [,sig=str][,rev=n]\n"
>      "             [,oem_id=str][,oem_table_id=str][,oem_rev=n]\n"
>      "             [,asl_compiler_id=str][,asl_compiler_rev=n]\n"
> -    "                ACPI table description\n", QEMU_ARCH_I386)
> +    "                ACPI table description\n"
> +    "-acpitable fadt=n\n"
> +    "                Configure FADT revision\n",
> +    QEMU_ARCH_I386)
>  STEXI
>  @item -acpitable 
> address@hidden:@var{file2}]...[,address@hidden,address@hidden,address@hidden,address@hidden,address@hidden
>  [,address@hidden,address@hidden
>  @findex -acpitable
> @@ -1511,6 +1514,9 @@ If a SLIC table is supplied to QEMU, then the SLIC's 
> oem_id and oem_table_id
>  fields will override the same in the RSDT and the FADT (a.k.a. FACP), in 
> order
>  to ensure the field matches required by the Microsoft SLIC spec and the ACPI
>  spec.
> +
> address@hidden -acpitable address@hidden
> +Configure FADT revision to 1, 3, 5, default 1.
>  ETEXI
>  
>  DEF("smbios", HAS_ARG, QEMU_OPTION_smbios,