qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 1/2] acpi: Allow ACPI default OEM ID and OEM tab


From: Laszlo Ersek
Subject: Re: [Qemu-devel] [PATCH 1/2] acpi: Allow ACPI default OEM ID and OEM table ID fields to be set.
Date: Thu, 3 Sep 2015 17:09:23 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0

On 09/02/15 23:32, Michael S. Tsirkin wrote:
> 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?

I agree with these remarks, but I'd propose a different idea for solving
the OEM Table ID problem.

As can be seen from the previous discussion that Rich linked in the
blurb, and from Michael Tokarev's patch, and from Chris Evich's earlier
testing of Win8.1 (linked in the previous discussion), the *only* guest
OS where this matters is Windows 7. And Windows 7 doesn't care about the
OEM Table Id, only the OEM Id.

So, I'd simplify this by dropping support for "oem_table_id" completely.

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

Or let's do that, yes. I don't think it's a problem to acknowledge that
this is being done exactly for one guest OS (which won't hurt elsewhere
either).

--*--

Going forward (you mention the UEFI tables above, thanks for that!),
I'll have to update / extend the build_header() function to expose more
internals. For the vmgenid stuff / UEFI table(s) the current rule

  OEM Table ID := "BXPC" + Signature

won't be good enough. The OEM Table ID will have to be settable
independently from the Signature (as an exception -- most of the code
can remain unchanged). Justification:

Some Signatures (like "UEFI") are allowed to have multiple instances.
The DataTableRegion() operator uses the

  (Signature, OemID, OemTableID)

triplet for locating a table uniquely. Given that Signature is
non-unique in general (see above), and that OemID is usually the same
across all tables (either hard-wired by QEMU, or set by Richard's
patches), the OemTableID is the *only* key component that can ensure
unicity.

However, the current rule derives OemTableID directly from Signature.
Which means that at the moment we have no unique

  (Signature, OemID, OemTableID)

keys; for the purpose of selectivity, they are now equivalent to

  (Signature, OemID)

which is not unique.

We're just getting away with it because noone tries to use
DataTableRegion(). (Not in a way that would expose this problem anyway.)

... So my point is, whatever method is chosen here, ie. full awareness
of SLIC, *or* exposing oem_id on the command line (but *not*
oem_table_id), please do it in a way that will later allow me to build
ACPI table headers with *any* (otherwise valid) OemTableID values.

> - except
> they have a bug in that they don't update the field in FADT.

Indeed.

Thanks!
Laszlo

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]