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

[Michael S. Tsirkin]

On Mon, Aug 15, 2016 at 02:18:55AM +0000, Zheng, Lv wrote:
> Hi, 
> 
> > From: Michael S. Tsirkin [mailto:address@hidden
> > 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:address@hidden
> > > > 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: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.
> > > >
> > > > 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.

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.

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