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

[Igor Mammedov]

On Fri, 12 Aug 2016 00:47:04 +0000
"Zheng, Lv" <address@hidden> wrote:

> 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.
RDST is not limited to rev 1 only
it can as easily reference v2/3/4/5/6 tables
differences between RSDT and XSDT is that the former is able to point
to tables located below 4G only.

> 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?
ACPI 6.0 spec says:
"An ACPI-compatible OS must use the XSDT if present."
hence XSDT referenced FADT must be used, but that's Windows
so I won't bet that it actually does what spec mandates.

> 2. What's the behavior on IA64/ARM platforms?
For ARM virt machine, QEMU currently provides ACPI 5.1 variant of FADT.

> 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.
What I'd suggest is try to bump up revision unconditionally to 5.1 (same as ARM)
so it would provide 64-bit fields although not really used by QEMU
and upstream it (provided it won't break anything).

As for probing part with 64-bit fields actually used,
I'd keep it out of tree as it doesn't make sense for upstream
QEMU to have it since all addresses are below 4Gb and it's not
going to change to keep compatibility with legacy OSes.

That would keep out of tree bits minimal and at the same time
reduce maintenance headache on QEMU side since it won't have
to maintain extra -acpitable option forever.

> 
> > 
> > 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.
Windows boots just fine with 1 FADT so far.

I wonder, what Windows version does require 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.
providing revision number is not enough, each revision might have
its own FADT structure, and it would be a bunch of code to maintain
only to play and see if Windows is happy about it.
I'd keep that out of tree.

> 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.
I'd not touch -acpitable as you did even for out of tree code
but instead add property to piix4_pm/ich9-lpc that would enable
testing bits you need, for example "enable_64bit_gpe" then
you could easily switch it on for tests by doing following:

 $QEMU -global piix4_pm.enable_64bit_gpe=on ...

grep for "disable_s3" option and see how it's used/implemented.

That way you won't risk breaking -acpitable CLI option behavior
and keep experiment bits relatively isolated (easier to rebase)
and it would be easier to upstream some if they make sense for
general usage.

You also might want to look at the work Michael have done wrt XSDT[1]
when we played with initial implementation of ACPI bits for ARM
but in the end we've picked RSDT there as well.

1) just google for "qemu-devel XSDT"

> 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,  
>