Re: [Qemu-devel] [PATCH 1/2] acpi: Allow ACPI default OEM ID and OEM table ID fields to be set.

[Michael S. Tsirkin]

On Wed, Sep 02, 2015 at 08:03:37PM +0100, Richard W.M. Jones wrote:
> When using qemu's internal ACPI table generation, qemu sets the fields
> OEM ID and OEM table ID to arbitrary default values.  OEM ID is set to
> "BOCHS " and OEM table ID is set to "BXPCxxxx" where "xxxx" is
> replaced by the ACPI table name (eg. "BXPCRSDT" for the RSDT table).
> 
> This patch allows you to override these default values.
> 
> Use one of these alternatives:
> 
>   qemu -acpidefault oem_id=ABCD,oem_table_id=EFGH
>   qemu -acpidefault oem_id=ABCD,oem_table_id=EFGHIJKL
> 
> In the first case, the last four types of the OEM table name field are
> set to the ACPI table name.
> 
> This does not affect the -acpitable option (for user-defined ACPI
> tables), which has its own method for setting these fields.
> 
> Signed-off-by: Richard W.M. Jones <address@hidden>

I don't think the 1st option makes sense - why force
4 bytes of the ID?

And the issue with the 2nd one is that it won't work if
there are two tables with same ID - which might be the
case for SSID and UEFI tables at least.

Maybe specify this per signature?

qemu -acpi-header signature=FADT,oem_id=ABCD,oem_table_id=EFGHIJKL,oem_rev=XXXX

Maybe allow oem_id=file as well? Might be handy.

> BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1248758

BTW I won't mind using debian's approach too - except
they have a bug in that they don't update the field in FADT.

> ---
>  hw/acpi/aml-build.c         | 44 +++++++++++++++++++++++++++++++++---
>  hw/arm/virt-acpi-build.c    |  5 ++++-
>  hw/i386/acpi-build.c        |  5 ++++-
>  include/hw/acpi/aml-build.h |  3 +--
>  qemu-options.hx             | 19 ++++++++++++++++
>  vl.c                        | 55 
> +++++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 124 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 0d4b324..7a2a2fb 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -28,8 +28,13 @@
>  #include "hw/acpi/aml-build.h"
>  #include "qemu/bswap.h"
>  #include "qemu/bitops.h"
> +#include "qemu/config-file.h"
> +#include "qemu/option.h"
>  #include "hw/acpi/bios-linker-loader.h"
>  
> +#define ACPI_BUILD_APPNAME6 "BOCHS "
> +#define ACPI_BUILD_APPNAME4 "BXPC"
> +
>  static GArray *build_alloc_array(void)
>  {
>      return g_array_new(false, true /* clear */, 1);
> @@ -1135,16 +1140,49 @@ Aml *aml_unicode(const char *str)
>      return var;
>  }
>  
> +/* The caller should pass in 6 and 8 byte char arrays resp. */
> +void
> +acpi_get_oem_defaults(const char *sig, char *oem_id, char *oem_table_id)
> +{
> +    QemuOpts *opts;
> +    const char *opt_oem_id;
> +    const char *opt_oem_table_id;
> +
> +    opts = qemu_opts_find(qemu_find_opts("acpidefault"), NULL);
> +    if (opts == NULL) {
> +        memcpy(oem_id, ACPI_BUILD_APPNAME6, 6);
> +        memcpy(oem_table_id, ACPI_BUILD_APPNAME4, 4);
> +        memcpy(oem_table_id + 4, sig, 4);
> +    }
> +
> +    opt_oem_id = qemu_opt_get(opts, "oem_id");
> +    if (opt_oem_id) {
> +        memcpy(oem_id, opt_oem_id, 6);
> +    }
> +    opt_oem_table_id = qemu_opt_get(opts, "oem_table_id");
> +    if (opt_oem_table_id) {
> +        if (strlen(opt_oem_table_id) == 8) {
> +            memcpy(oem_table_id, opt_oem_table_id, 8);
> +        } else {
> +            memcpy(oem_table_id, opt_oem_table_id, 4);
> +            memcpy(oem_table_id + 4, sig, 4);
> +        }
> +    }
> +}
> +
>  void
>  build_header(GArray *linker, GArray *table_data,
>               AcpiTableHeader *h, const char *sig, int len, uint8_t rev)
>  {
> +    char oem_id[6], oem_table_id[8];
> +
> +    acpi_get_oem_defaults(sig, oem_id, oem_table_id);
> +
>      memcpy(&h->signature, sig, 4);
>      h->length = cpu_to_le32(len);
>      h->revision = rev;
> -    memcpy(h->oem_id, ACPI_BUILD_APPNAME6, 6);
> -    memcpy(h->oem_table_id, ACPI_BUILD_APPNAME4, 4);
> -    memcpy(h->oem_table_id + 4, sig, 4);
> +    memcpy(h->oem_id, oem_id, 6);
> +    memcpy(h->oem_table_id, oem_table_id, 8);
>      h->oem_revision = cpu_to_le32(1);
>      memcpy(h->asl_compiler_id, ACPI_BUILD_APPNAME4, 4);
>      h->asl_compiler_revision = cpu_to_le32(1);
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index f365140..85677f2 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -314,13 +314,16 @@ static void acpi_dsdt_add_pci(Aml *scope, const 
> MemMapEntry *memmap, int irq)
>  static GArray *
>  build_rsdp(GArray *rsdp_table, GArray *linker, unsigned rsdt)
>  {
> +    char oem_id[6], oem_table_id[8];
>      AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
>  
> +    acpi_get_oem_defaults("RSDP", oem_id, oem_table_id);
> +
>      bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, 16,
>                               true /* fseg memory */);
>  
>      memcpy(&rsdp->signature, "RSD PTR ", sizeof(rsdp->signature));
> -    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, sizeof(rsdp->oem_id));
> +    memcpy(rsdp->oem_id, oem_id, sizeof(oem_id));
>      rsdp->length = cpu_to_le32(sizeof(*rsdp));
>      rsdp->revision = 0x02;
>  
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 95e0c65..8cbf90d 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1593,13 +1593,16 @@ build_dsdt(GArray *table_data, GArray *linker, 
> AcpiMiscInfo *misc)
>  static GArray *
>  build_rsdp(GArray *rsdp_table, GArray *linker, unsigned rsdt)
>  {
> +    char oem_id[6], oem_table_id[8];
>      AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
>  
> +    acpi_get_oem_defaults("RSDP", oem_id, oem_table_id);
> +
>      bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, 16,
>                               true /* fseg memory */);
>  
>      memcpy(&rsdp->signature, "RSD PTR ", 8);
> -    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, 6);
> +    memcpy(rsdp->oem_id, oem_id, sizeof(oem_id));
>      rsdp->rsdt_physical_address = cpu_to_le32(rsdt);
>      /* Address to be filled by Guest linker */
>      bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE,
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index e3afa13..85a3c2c 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -10,8 +10,6 @@
>  #define ACPI_BUILD_TABLE_MAX_SIZE         0x200000
>  
>  #define ACPI_BUILD_APPNAME  "Bochs"
> -#define ACPI_BUILD_APPNAME6 "BOCHS "
> -#define ACPI_BUILD_APPNAME4 "BXPC"
>  
>  #define ACPI_BUILD_TABLE_FILE "etc/acpi/tables"
>  #define ACPI_BUILD_RSDP_FILE "etc/acpi/rsdp"
> @@ -276,6 +274,7 @@ Aml *aml_varpackage(uint32_t num_elements);
>  Aml *aml_touuid(const char *uuid);
>  Aml *aml_unicode(const char *str);
>  
> +void acpi_get_oem_defaults(const char *sig, char *oem_id, char 
> *oem_table_id);
>  void
>  build_header(GArray *linker, GArray *table_data,
>               AcpiTableHeader *h, const char *sig, int len, uint8_t rev);
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 77f5853..73c8e97 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2705,6 +2705,25 @@ Add named fw_cfg entry from file. @var{name} 
> determines the name of
>  the entry in the fw_cfg file directory exposed to the guest.
>  ETEXI
>  
> +DEF("acpidefault", HAS_ARG, QEMU_OPTION_acpidefault,
> +    "-acpidefault oem_id=<OEMID>,oem_table_id=<OEMTABLEID>\n"
> +    "                set default OEM ID (6 bytes) and OEM table ID (4 or 8 
> bytes)\n",
> +    QEMU_ARCH_ALL)
> +STEXI
> address@hidden -acpidefault address@hidden,address@hidden
> address@hidden -acpidefault
> +Set the default OEM ID and OEM table ID used in ACPI tables.  The
> +OEM ID should be 6 bytes (pad with spaces if needed), and the OEM
> +table ID should be 4 or 8 bytes.
> +
> +If not set, qemu uses @code{"BOCHS "} and @code{"BXPCxxxx"} where
> address@hidden is the table name (eg. @code{"BXPCRSDT"} in the RSDT table).
> +
> +If you are adding user-defined ACPI tables on the qemu command line,
> +use @code{-acpitable} instead.  The defaults here will not be used
> +in this case.
> +ETEXI
> +
>  DEF("serial", HAS_ARG, QEMU_OPTION_serial, \
>      "-serial dev     redirect the serial port to char device 'dev'\n",
>      QEMU_ARCH_ALL)
> diff --git a/vl.c b/vl.c
> index 584ca88..614c66f 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -517,6 +517,24 @@ static QemuOptsList qemu_fw_cfg_opts = {
>      },
>  };
>  
> +static QemuOptsList qemu_acpidefault_opts = {
> +    .name = "acpidefault",
> +    .merge_lists = true,
> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_acpidefault_opts.head),
> +    .desc = {
> +        {
> +            .name = "oem_id",
> +            .type = QEMU_OPT_STRING,
> +            .help = "Set default OEM ID (6 bytes)",
> +        }, {
> +            .name = "oem_table_id",
> +            .type = QEMU_OPT_STRING,
> +            .help = "Set default OEM table ID (4 or 8 bytes)",
> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
>  /**
>   * Get machine options
>   *
> @@ -2287,6 +2305,30 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, 
> Error **errp)
>      return 0;
>  }
>  
> +static int check_acpi_option(QemuOpts *opts)
> +{
> +    const char *oem_id;
> +    const char *oem_table_id;
> +
> +    oem_id = qemu_opt_get(opts, "oem_id");
> +    if (oem_id) {
> +        if (strlen(oem_id) != 6) {
> +            error_report("-acpi oem_id parameter must be 6 bytes long");
> +            return -1;
> +        }

It's legal for it to be shorter I think. Spec says it's 0-terminated
then.

> +    }
> +    oem_table_id = qemu_opt_get(opts, "oem_table_id");
> +    if (oem_table_id) {
> +        size_t len = strlen(oem_table_id);
> +        if (len != 4 && len != 8) {
> +            error_report("-acpi oem_table_id parameter "
> +                         "must be 4 or 8 bytes long");
> +            return -1;
> +        }
> +    }
> +    return 0;
> +}
> +
>  static int device_help_func(void *opaque, QemuOpts *opts, Error **errp)
>  {
>      return qdev_device_help(opts);
> @@ -3018,6 +3060,7 @@ int main(int argc, char **argv, char **envp)
>      qemu_add_opts(&qemu_icount_opts);
>      qemu_add_opts(&qemu_semihosting_config_opts);
>      qemu_add_opts(&qemu_fw_cfg_opts);
> +    qemu_add_opts(&qemu_acpidefault_opts);
>  
>      runstate_init();
>  
> @@ -3653,6 +3696,13 @@ int main(int argc, char **argv, char **envp)
>                      exit(1);
>                  }
>                  break;
> +            case QEMU_OPTION_acpidefault:
> +                opts = qemu_opts_parse_noisily(qemu_find_opts("acpidefault"),
> +                                               optarg, false);
> +                if (opts == NULL) {
> +                    exit(1);
> +                }
> +                break;
>              case QEMU_OPTION_enable_kvm:
>                  olist = qemu_find_opts("machine");
>                  qemu_opts_parse_noisily(olist, "accel=kvm", false);
> @@ -4522,6 +4572,11 @@ int main(int argc, char **argv, char **envp)
>          exit(1);
>      }
>  
> +    if (check_acpi_option(qemu_opts_find(qemu_find_opts("acpidefault"),
> +                                         NULL)) < 0) {
> +        exit(1);
> +    }
> +
>      /* init USB devices */
>      if (usb_enabled()) {
>          if (foreach_device_config(DEV_USB, usb_parse) < 0)
> -- 
> 2.5.0