Hi, Igor
> From: Igor Mammedov [mailto:imamm...@redhat.com]
> Subject: Re: [PATCH v5 2/2] ACPI: Add -acpitable fadt= to allow FADT
> revision changes
>
> On Fri, 12 Aug 2016 00:47:04 +0000
> "Zheng, Lv" <lv.zh...@intel.com> wrote:
>
> > Hi, Igor
> >
> > Thanks for the review.
> >
> > > From: Igor Mammedov [mailto:imamm...@redhat.com]
> > > 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 <lv.zh...@intel.com> 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.
I tried v3 FADT using RSDT.
Windows can boot with a v3 table where both v1/v3 fields are filled.
But if I remove v1 compatible fields but leave v3 compatible fields in this
FADT, Windows cannot boot.
So it looks there can be possible restrictions related to FADT.
>
> > 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.
I'm trying to enable XSDT in qemu to investigate this.
>
> > 2. What's the behavior on IA64/ARM platforms?
> For ARM virt machine, QEMU currently provides ACPI 5.1 variant of FADT.
OK.
>
> > 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).
OK.
>
> 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.
OK.
>
> >
> > >
> > > 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?
No, it's my bad wording.
Windows requires 2 FADTs when XSDT is provided.
My guess is XSDT has been supported since Windows Vista.
>
> > 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.
OK.
>
> > 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.
OK.
Sounds good.
>
> 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"
Good to know, I may also need this to do my FADT investigations.
Thanks and best regards
Lv
>
> > 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 <lv.zh...@intel.com>
> > > > ---
> > > > 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
> > >
> data=@var{file1}[:@var{file2}]...[,sig=@var{str}][,rev=@var{n}][,oem_id=@
> > > var{str}][,oem_table_id=@var{str}][,oem_rev=@var{n}]
> > > [,asl_compiler_id=@var{str}][,asl_compiler_rev=@var{n}]
> > > > @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.
> > > > +
> > > > +@item -acpitable fadt=@var{n}
> > > > +Configure FADT revision to 1, 3, 5, default 1.
> > > > ETEXI
> > > >
> > > > DEF("smbios", HAS_ARG, QEMU_OPTION_smbios,
> >
