Hi, Michael
> From: Michael S. Tsirkin [mailto:m...@redhat.com]
> Subject: Re: [PATCH v5 2/2] ACPI: Add -acpitable fadt= to allow FADT
> revision changes
>
> On Mon, Aug 15, 2016 at 02:18:55AM +0000, Zheng, Lv wrote:
> > Hi,
> >
> > > From: Michael S. Tsirkin [mailto:m...@redhat.com]
> > > Subject: Re: [PATCH v5 2/2] ACPI: Add -acpitable fadt= to allow FADT
> > > revision changes
> > >
> > > On Mon, Aug 15, 2016 at 01:33:41AM +0000, Zheng, Lv wrote:
> > > > Hi,
> > > >
> > > > > From: Michael S. Tsirkin [mailto:m...@redhat.com]
> > > > > Subject: Re: [PATCH v5 2/2] ACPI: Add -acpitable fadt= to allow
> FADT
> > > > > revision changes
> > > > >
> > > > > 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.
> > > >
> > > > Let me reword:
> > > > Windows requires BIOS to provide 2 FADTs when both RSDT and
> XSDT
> > > are provided.
> > >
> > > That's good to know, thanks.
> > > Do you see a reason to have both at the moment?
> >
> > If qemu always uses RSDT, then all Windows clones should support such
> an old BIOS because all Windows clones are backward compatible.
> >
> > But, ACPI 2.0+/5.0+ introduced new features, (for example, reduced
> hardware model).
> > My investigation result shows if an FADT is provided via RSDT, windows
> will ignore some of its fields.
> > So if qemu starts to support new features, then qemu may need to
> provide both RSDT and XSDT.
> > As a result, qemu may need to provide 2 FADTs.
> >
> > However, there is no real ACPI 2.0+ support in qemu/x86 code.
> > So this feature (having a version parameter for FADT setup) is just a
> good-to-have feature.
> > With which, developers may start to introduce some ACPI 2.0+ features.
> >
> > Thanks
> > Lv
>
> Right. Let's include this when we actually need some of the
> features that require an XSDT.
OK.
I should:
1. remove -acpitable fadt option since it is only for the purpose of probing
Windows behavior;
2. do not increase FADT version since currently there is no XSDT in qemu/x86.
>
> Pls note that OVMF currently won't do the right thing
> if you have both an RSDT and an XSDT: it will try to
> install two copies of all tables.
> If you are interested in this, please take a look at
> the OVMF code and fix it up.
Good to know this.
I'll try when I have bandwidth.
Thanks and best regards
Lv
>
> > >
> > > > >
> > > > > > 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?
> > > >
> > > > Not hard.
> > > > I just may encounter problems if I want to put a document in Linux
> > > kernel related to FADT support and Windows behavior proofs.
> > > > I'll remove this option.
> > > >
> > > > >
> > > > > > >
> > > > > > > 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.
> > > >
> > > > OK. I see.
> > > >
> > > > Thanks
> > > > Lv
> > > >
> > > > >
> > > > > >
> > > > > > 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,
