Re: [Qemu-arm] [PATCH v2] ACPI: Add -acpifadt to allow FADT revision changes

[Zheng, Lv]

Hi,

> From: Paolo Bonzini [mailto:address@hidden On Behalf Of Paolo
> Bonzini
> Subject: Re: [PATCH v2] ACPI: Add -acpifadt to allow FADT revision changes
> 
> 
> 
> On 08/08/2016 10:16, Lv Zheng 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 -acpifadt 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.
> >
> > Signed-off-by: Lv Zheng <address@hidden>
> 
> Hi, please make this a suboption of -acpitable instead.  We try not to
> add new command-line options without supporting them in QemuOpts
> too.
[Lv Zheng] 
OK. I'll prepare a v3 patch to address this.
Thanks for the review.

Cheers
-Lv

> 
> Paolo
> 
> > ---
> > History:
> >
> > v2: Fix coding style issues.
> > ---
> >  hw/i386/acpi-build.c   |   62
> ++++++++++++++++++++++++++++++++++++++++++------
> >  include/hw/acpi/acpi.h |    1 +
> >  qemu-options.hx        |    9 +++++++
> >  vl.c                   |   23 ++++++++++++++++++
> >  4 files changed, 88 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index ce7cbc5..4479695 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,9 +349,9 @@ 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));
> > +    AcpiFadtDescriptorRev5_1 *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;
> >
> > @@ -328,13 +361,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", sizeof(*fadt),
> > +                 revision, oem_id, oem_table_id);
> >  }
> >
> >  void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
> > @@ -2681,7 +2729,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 a71aaf8..cff34f6 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -1492,6 +1492,15 @@ STEXI
> >  Disable HPET support.
> >  ETEXI
> >
> > +DEF("acpifadt", HAS_ARG, QEMU_OPTION_acpifadt, \
> > +    "-acpifadt  n    configure FADT revision number (1, 3, 5)\n",
> > +    QEMU_ARCH_ALL)
> > +STEXI
> > address@hidden -acpifadt @var{n}
> > address@hidden -acpifadt
> > +Configure FADT revision number, default is 1.
> > +ETEXI
> > +
> >  DEF("acpitable", HAS_ARG, QEMU_OPTION_acpitable,
> >      "-acpitable
> [sig=str][,rev=n][,oem_id=str][,oem_table_id=str][,oem_rev=n][,asl_compi
> ler_id=str][,asl_compiler_rev=n][,{data|file}=file1[:file2]...]\n"
> >      "                ACPI table description\n", QEMU_ARCH_I386)
> > diff --git a/vl.c b/vl.c
> > index e7c2c62..220e36d 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -64,6 +64,7 @@ int main(int argc, char **argv)
> >  #include "hw/bt.h"
> >  #include "sysemu/watchdog.h"
> >  #include "hw/smbios/smbios.h"
> > +#include "hw/acpi/acpi.h"
> >  #include "hw/xen/xen.h"
> >  #include "hw/qdev.h"
> >  #include "hw/loader.h"
> > @@ -158,6 +159,7 @@ int max_cpus = 1;
> >  int smp_cores = 1;
> >  int smp_threads = 1;
> >  int acpi_enabled = 1;
> > +uint8_t acpi_fadt_rev = 1;
> >  int no_hpet = 0;
> >  int fd_bootchk = 1;
> >  static int no_reboot;
> > @@ -2301,6 +2303,24 @@ static int parse_fw_cfg(void *opaque,
> QemuOpts *opts, Error **errp)
> >      return 0;
> >  }
> >
> > +static void fadt_parse(const char *arg)
> > +{
> > +    unsigned long rev;
> > +    int err;
> > +
> > +    err = qemu_strtoul(arg, NULL, 10, &rev);
> > +    if (err) {
> > +        error_printf("Invalid FADT revision.\n");
> > +        exit(1);
> > +    }
> > +    if (rev != 1 && rev != 3 && rev != 5) {
> > +        error_printf("Unsupported FADT revision %lu, "
> > +                     "please specify 1,3,5.\n", rev);
> > +        exit(1);
> > +    }
> > +    acpi_fadt_rev = rev;
> > +}
> > +
> >  static int device_help_func(void *opaque, QemuOpts *opts, Error
> **errp)
> >  {
> >      return qdev_device_help(opts);
> > @@ -3622,6 +3642,9 @@ int main(int argc, char **argv, char **envp)
> >                  qdev_prop_register_global(&slew_lost_ticks);
> >                  break;
> >              }
> > +            case QEMU_OPTION_acpifadt:
> > +                fadt_parse(optarg);
> > +                break;
> >              case QEMU_OPTION_acpitable:
> >                  opts = qemu_opts_parse_noisily(qemu_find_opts("acpi"),
> >                                                 optarg, true);
> >