On Fri, Aug 12, 2016 at 12:47:04AM +0000, Zheng, Lv 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.
What does "requires" mean here? It seems to work fine without.
> 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.
Nice, but why do we need PATCH 2 upstream? Is it hard for you to maintain
out of tree?
> >
> > 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.
Didn't review yet, I'll let Igor review first.
> 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?
ATM PATCH 2 does not seem generally useful.
>
> 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,
