| Message ID | 1441220618-4750-2-git-send-email-rjones@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 I should have copied the actual command lines I was using. The oem_id should be 6 bytes, so correct examples are: qemu -acpidefault oem_id=ABCDEF,oem_table_id=GHIJ qemu -acpidefault oem_id=ABCDEF,oem_table_id=GHIJKLMN > In the first case, the last four types of the OEM table name field are s/types/bytes/ > 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 <rjones@redhat.com> > BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1248758 > --- > 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 > +@item -acpidefault oem_id=@var{OEMID},file=@var{OEMTABLEID} > +@findex -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 > +@code{xxxx} 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; > + } > + } > + 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 >
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 <rjones@redhat.com> 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 > +@item -acpidefault oem_id=@var{OEMID},file=@var{OEMTABLEID} > +@findex -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 > +@code{xxxx} 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
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 <rjones@redhat.com> > > 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 >> +@item -acpidefault oem_id=@var{OEMID},file=@var{OEMTABLEID} >> +@findex -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 >> +@code{xxxx} 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 >
03.09.2015 18:09, Laszlo Ersek wrote: > 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 <rjones@redhat.com> >> >> 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. Actually win7 do care about oem table id. And my patch shows it. This is the minimal portion of RSDT (and also FACP in UEFI mode) which should match SLIC -- oem_id (6 bytes) and oem_table_id (8 bytes). My patch copies 10 bytes starting with oem_id. Without that, win7 offline activation doesn't work. FWIW :) Thanks, /mjt
On 09/03/15 17:17, Michael Tokarev wrote: > 03.09.2015 18:09, Laszlo Ersek wrote: >> 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 <rjones@redhat.com> >>> >>> 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. > > Actually win7 do care about oem table id. And my patch shows it. This > is the minimal portion of RSDT (and also FACP in UEFI mode) which should > match SLIC -- oem_id (6 bytes) and oem_table_id (8 bytes). My patch copies > 10 bytes starting with oem_id. Actually, 6 + 4 + 4 = 14, so OemID and OemTableID. But, you are right, I was working off the commit message & comments only, not the actual amount of bytes copied. This ties down both OemID and OemTableId, between all of RSDT, SLIC, and FADT. Since - I argue against exposing a generic oem_table_id on the command line (unlike oem_id), - and I also find that an oem_table_id "map" would be overkill, I think it follows that I can only ask for the special SLIC-handling logic already visible in your patch. "User passed in SLIC --> adapt RSDT and FADT." That is, it is already user-controlled. (The FADT change will ensure that OVMF will update the RSDT that *it* installs.) This is just my preference, of course... But at least it doesn't seem to conflict with Michael's! :) Thanks Laszlo > Without that, win7 offline activation > doesn't work. > > FWIW :) > > Thanks, > > /mjt >
03.09.2015 18:37, Laszlo Ersek wrote: [] > Actually, 6 + 4 + 4 = 14, so OemID and OemTableID. But, you are right, I > was working off the commit message & comments only, not the actual > amount of bytes copied. > > This ties down both OemID and OemTableId, between all of RSDT, SLIC, and > FADT. Since > - I argue against exposing a generic oem_table_id on the command > line (unlike oem_id), > - and I also find that an oem_table_id "map" would be overkill, > > I think it follows that I can only ask for the special SLIC-handling > logic already visible in your patch. "User passed in SLIC --> adapt RSDT > and FADT." That is, it is already user-controlled. It might be useful to have it controllable by user in other cases too. But I don't have any usage case for that. > (The FADT change will ensure that OVMF will update the RSDT that *it* > installs.) > > This is just my preference, of course... But at least it doesn't seem to > conflict with Michael's! :) Yes, it might just work. Especially since in case when SLIC is specified, the oem_table&Co should come from SLIC, not forcing user to specify them on command line. Command line can be used anyway, with the default value coming from slic if it is provided. BTW, I updated the patch for 2.4 a few days ago, it is hackish as I wanted to touch as few files as possible. And BTW2, the code in acpi/core.c uses its own local definition of ACPI table data structures, instead of using common code from acpi.h... ;) Thanks, /mjt
On 09/03/15 18:33, Michael Tokarev wrote: > 03.09.2015 18:37, Laszlo Ersek wrote: > [] >> Actually, 6 + 4 + 4 = 14, so OemID and OemTableID. But, you are right, I >> was working off the commit message & comments only, not the actual >> amount of bytes copied. >> >> This ties down both OemID and OemTableId, between all of RSDT, SLIC, and >> FADT. Since >> - I argue against exposing a generic oem_table_id on the command >> line (unlike oem_id), >> - and I also find that an oem_table_id "map" would be overkill, >> >> I think it follows that I can only ask for the special SLIC-handling >> logic already visible in your patch. "User passed in SLIC --> adapt RSDT >> and FADT." That is, it is already user-controlled. > > It might be useful to have it controllable by user in other cases too. > But I don't have any usage case for that. > >> (The FADT change will ensure that OVMF will update the RSDT that *it* >> installs.) >> >> This is just my preference, of course... But at least it doesn't seem to >> conflict with Michael's! :) > > Yes, it might just work. Especially since in case when SLIC is specified, > the oem_table&Co should come from SLIC, not forcing user to specify them > on command line. Command line can be used anyway, with the default value > coming from slic if it is provided. > > BTW, I updated the patch for 2.4 a few days ago, it is hackish as I wanted > to touch as few files as possible. > > And BTW2, the code in acpi/core.c uses its own local definition of ACPI > table data structures, instead of using common code from acpi.h... ;) True, but the SDT header struct in "hw/acpi/core.c" (from 2009-2013) seems to predate the same in "include/hw/acpi/acpi-defs.h" (from Summer 2013) by quite a few years! :) > Thanks, > > /mjt >
On Thu, Sep 03, 2015 at 05:37:15PM +0200, Laszlo Ersek wrote: > On 09/03/15 17:17, Michael Tokarev wrote: > > 03.09.2015 18:09, Laszlo Ersek wrote: > >> 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 <rjones@redhat.com> > >>> > >>> 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. > > > > Actually win7 do care about oem table id. And my patch shows it. This > > is the minimal portion of RSDT (and also FACP in UEFI mode) which should > > match SLIC -- oem_id (6 bytes) and oem_table_id (8 bytes). My patch copies > > 10 bytes starting with oem_id. > > Actually, 6 + 4 + 4 = 14, so OemID and OemTableID. But, you are right, I > was working off the commit message & comments only, not the actual > amount of bytes copied. > > This ties down both OemID and OemTableId, between all of RSDT, SLIC, and > FADT. Since > - I argue against exposing a generic oem_table_id on the command > line (unlike oem_id), > - and I also find that an oem_table_id "map" would be overkill, > > I think it follows that I can only ask for the special SLIC-handling > logic already visible in your patch. "User passed in SLIC --> adapt RSDT > and FADT." That is, it is already user-controlled. > > (The FADT change will ensure that OVMF will update the RSDT that *it* > installs.) > > This is just my preference, of course... But at least it doesn't seem to > conflict with Michael's! :) > > Thanks > Laszlo If done this way, I think a machine option to turn this off might not be a bad idea, just in case. > > Without that, win7 offline activation > > doesn't work. > > > > FWIW :) > > > > Thanks, > > > > /mjt > >
On Thu, Sep 03, 2015 at 07:33:17PM +0300, Michael Tokarev wrote: > 03.09.2015 18:37, Laszlo Ersek wrote: > [] > > Actually, 6 + 4 + 4 = 14, so OemID and OemTableID. But, you are right, I > > was working off the commit message & comments only, not the actual > > amount of bytes copied. > > > > This ties down both OemID and OemTableId, between all of RSDT, SLIC, and > > FADT. Since > > - I argue against exposing a generic oem_table_id on the command > > line (unlike oem_id), > > - and I also find that an oem_table_id "map" would be overkill, > > > > I think it follows that I can only ask for the special SLIC-handling > > logic already visible in your patch. "User passed in SLIC --> adapt RSDT > > and FADT." That is, it is already user-controlled. > > It might be useful to have it controllable by user in other cases too. > But I don't have any usage case for that. > > > (The FADT change will ensure that OVMF will update the RSDT that *it* > > installs.) > > > > This is just my preference, of course... But at least it doesn't seem to > > conflict with Michael's! :) > > Yes, it might just work. Especially since in case when SLIC is specified, > the oem_table&Co should come from SLIC, not forcing user to specify them > on command line. Command line can be used anyway, with the default value > coming from slic if it is provided. > > BTW, I updated the patch for 2.4 a few days ago, it is hackish as I wanted > to touch as few files as possible. > > And BTW2, the code in acpi/core.c uses its own local definition of ACPI > table data structures, instead of using common code from acpi.h... ;) > > Thanks, > > /mjt Yes, we want that cleaned up.
Long comment from the bug reporter in case anyone isn't following that bug: https://bugzilla.redhat.com/show_bug.cgi?id=1248758#c15 Rich.
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 +@item -acpidefault oem_id=@var{OEMID},file=@var{OEMTABLEID} +@findex -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 +@code{xxxx} 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; + } + } + 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)
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 <rjones@redhat.com> BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1248758 --- 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(-)