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

[Zheng, Lv]

Hi, Igor

Thanks for the review.

> From: Igor Mammedov [mailto:address@hidden
> Subject: Re: [PATCH v5 2/2] ACPI: Add -acpitable fadt= to allow FADT
> revision changes
> 
> 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.
[Lv Zheng] 
This patch is used by us to probe Windows FADT behavior.
Current known behavior includes:
1. On x86, Windows requires BIOS to have 2 FADTs, 1 is in RSDT, and probably is 
a v1 table, the other is in XSDT, and probably is a v3+ table.
2. v2 FADT never exists in the spec, it is used in very early years, by IA64 
prototype machines.
3. If the FADT in RSDT takes effective, then Windows actually ignores 64-bit 
GAS fields, but uses 32-bit register fields.
But there are still many uncertainties:
1. What if the FADT in XSDT takes effective?
2. What's the behavior on IA64/ARM platforms?
If we could have an option here, we could continue the probing work, trying 
every combination. 

Thus the original purpose of this commit is - probing de-facto standard 
behavior.

> 
> 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.
> 
[Lv Zheng] 
But it seems the code in this commit contains useful stuffs for qemu.

As I said previously, it seems Windows requires 2 FADTs.
So having a revision parameter for build_fadt() seems to be necessary.
Because if qemu wants to increase FADT revision, qemu may do this in this way:
1. Invoke build_fadt() with revision 1, and put it into RSDT;
2. Invoke build_fadt() with latest revision, and put it into XSDT.
Also we know latest revision FADT is useful, it contains reduced hardware 
support.
And some of new ACPI features rely this model.

So I'm sure that this commit contains useful stuffs for qemu to do future 
extension.

But I don't know if you want to keep the new -acpitable fadt= option.
Let me ask:
1. Are there any issues in PATCH 01? Maybe my understanding of qemu option code 
is not correct.
2. IMO, this option should be useful for a long period because we need it to 
help probing Windows behavior, so can we have it upstreamed?

Best regards
Lv

> >
> > 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,oem_id=@
> var{str}][,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,