* [Qemu-devel] [PATCH 0/3] acpi: more infrastructure cleanups @ 2013-04-15 9:37 Michael S. Tsirkin 2013-04-15 9:37 ` [Qemu-devel] [PATCH 1/3] acpi: move declarations from pc.h to acpi.h Michael S. Tsirkin ` (4 more replies) 0 siblings, 5 replies; 13+ messages in thread From: Michael S. Tsirkin @ 2013-04-15 9:37 UTC (permalink / raw) To: qemu-devel I wanted to help Laszlo completely move ACPI tables out of seabios in time for 1.5, but had to put out some fires so didn't manage to do this in time, and going offline for now. Meanwhile, here is some refactoring that I did complete. This does not do much by itself, but works fine for me and will be helpful down the road. This includes a couple of patches I posted previously and another one on top. Pls consider for 1.5. Michael S. Tsirkin (3): acpi: move declarations from pc.h to acpi.h acpi: add acpi_table_add_hdr acpi.h: make it self contained arch_init.c | 1 + hw/acpi/core.c | 97 +++++++++++++++++++++++++++++++++++--------------- hw/i386/pc.c | 1 + hw/i386/pc_piix.c | 1 + include/hw/acpi/acpi.h | 20 +++++++++++ include/hw/i386/pc.h | 8 ----- 6 files changed, 91 insertions(+), 37 deletions(-) -- MST ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH 1/3] acpi: move declarations from pc.h to acpi.h 2013-04-15 9:37 [Qemu-devel] [PATCH 0/3] acpi: more infrastructure cleanups Michael S. Tsirkin @ 2013-04-15 9:37 ` Michael S. Tsirkin 2013-04-15 10:42 ` Paolo Bonzini 2013-04-15 9:37 ` [Qemu-devel] [PATCH 2/3] acpi: add acpi_table_add_hdr Michael S. Tsirkin ` (3 subsequent siblings) 4 siblings, 1 reply; 13+ messages in thread From: Michael S. Tsirkin @ 2013-04-15 9:37 UTC (permalink / raw) To: qemu-devel Functions defined in acpi/ should be declared in acpi.h Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- arch_init.c | 1 + hw/i386/pc.c | 1 + hw/i386/pc_piix.c | 1 + include/hw/acpi/acpi.h | 10 ++++++++++ include/hw/i386/pc.h | 8 -------- 5 files changed, 13 insertions(+), 8 deletions(-) diff --git a/arch_init.c b/arch_init.c index 769ce77..fba0889 100644 --- a/arch_init.c +++ b/arch_init.c @@ -49,6 +49,7 @@ #include "qmp-commands.h" #include "trace.h" #include "exec/cpu-all.h" +#include "hw/acpi/acpi.h" #ifdef DEBUG_ARCH_INIT #define DPRINTF(fmt, ...) \ diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 8d75b34..0d6e72b 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -52,6 +52,7 @@ #include "sysemu/arch_init.h" #include "qemu/bitmap.h" #include "qemu/config-file.h" +#include "hw/acpi/acpi.h" /* debug PC/ISA interrupts */ //#define DEBUG_IRQ diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index cff8013..943758a 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -43,6 +43,7 @@ #include "hw/xen/xen.h" #include "exec/memory.h" #include "exec/address-spaces.h" +#include "hw/acpi/acpi.h" #include "cpu.h" #ifdef CONFIG_XEN # include <xen/hvm/hvm_info_table.h> diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h index e18ef28..80e955d 100644 --- a/include/hw/acpi/acpi.h +++ b/include/hw/acpi/acpi.h @@ -154,4 +154,14 @@ void acpi_gpe_reset(ACPIREGS *ar); void acpi_gpe_ioport_writeb(ACPIREGS *ar, uint32_t addr, uint32_t val); uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, uint32_t addr); +/* acpi.c */ +extern int acpi_enabled; +extern char unsigned *acpi_tables; +extern size_t acpi_tables_len; + +void acpi_table_install(const char unsigned *blob, size_t bloblen, + bool has_header, const struct AcpiTableOptions *hdrs, + Error **errp); +void acpi_table_add(const QemuOpts *opts, Error **errp); + #endif /* !QEMU_HW_ACPI_H */ diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index 5d40914..9bcc819 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -107,14 +107,6 @@ void cpu_smm_register(cpu_set_smm_t callback, void *arg); void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name); -/* acpi.c */ -extern int acpi_enabled; -extern char unsigned *acpi_tables; -extern size_t acpi_tables_len; - -void acpi_bios_init(void); -void acpi_table_add(const QemuOpts *opts, Error **errp); - /* acpi_piix.c */ i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base, -- MST ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] acpi: move declarations from pc.h to acpi.h 2013-04-15 9:37 ` [Qemu-devel] [PATCH 1/3] acpi: move declarations from pc.h to acpi.h Michael S. Tsirkin @ 2013-04-15 10:42 ` Paolo Bonzini 2013-04-15 11:00 ` Laszlo Ersek 0 siblings, 1 reply; 13+ messages in thread From: Paolo Bonzini @ 2013-04-15 10:42 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel Il 15/04/2013 11:37, Michael S. Tsirkin ha scritto: > +/* acpi.c */ > +extern int acpi_enabled; > +extern char unsigned *acpi_tables; > +extern size_t acpi_tables_len; > + > +void acpi_table_install(const char unsigned *blob, size_t bloblen, > + bool has_header, const struct AcpiTableOptions *hdrs, > + Error **errp); This function is static, I've fixed it up and will include it in the hw/ fixups pull request. I picked up patch 3 as well, I'll leave 2 to Laszlo. Paolo > +void acpi_table_add(const QemuOpts *opts, Error **errp); ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] acpi: move declarations from pc.h to acpi.h 2013-04-15 10:42 ` Paolo Bonzini @ 2013-04-15 11:00 ` Laszlo Ersek 2013-04-15 11:00 ` Paolo Bonzini 0 siblings, 1 reply; 13+ messages in thread From: Laszlo Ersek @ 2013-04-15 11:00 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: Paolo Bonzini, qemu-devel On 04/15/13 12:42, Paolo Bonzini wrote: > Il 15/04/2013 11:37, Michael S. Tsirkin ha scritto: >> +/* acpi.c */ >> +extern int acpi_enabled; >> +extern char unsigned *acpi_tables; >> +extern size_t acpi_tables_len; >> + >> +void acpi_table_install(const char unsigned *blob, size_t bloblen, >> + bool has_header, const struct AcpiTableOptions *hdrs, >> + Error **errp); > > This function is static, I've fixed it up and will include it in the hw/ > fixups pull request. > > I picked up patch 3 as well, I'll leave 2 to Laszlo. > > Paolo > >> +void acpi_table_add(const QemuOpts *opts, Error **errp); Doesn't this series conflict with my pending <http://lists.nongnu.org/archive/html/qemu-devel/2013-04/msg01427.html>? Thanks Laszlo ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] acpi: move declarations from pc.h to acpi.h 2013-04-15 11:00 ` Laszlo Ersek @ 2013-04-15 11:00 ` Paolo Bonzini 0 siblings, 0 replies; 13+ messages in thread From: Paolo Bonzini @ 2013-04-15 11:00 UTC (permalink / raw) To: Laszlo Ersek; +Cc: qemu-devel, Michael S. Tsirkin Il 15/04/2013 13:00, Laszlo Ersek ha scritto: >> > >>> >> +void acpi_table_add(const QemuOpts *opts, Error **errp); > Doesn't this series conflict with my pending > <http://lists.nongnu.org/archive/html/qemu-devel/2013-04/msg01427.html>? Yeah, but the conflicts are trivial. Paolo ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH 2/3] acpi: add acpi_table_add_hdr 2013-04-15 9:37 [Qemu-devel] [PATCH 0/3] acpi: more infrastructure cleanups Michael S. Tsirkin 2013-04-15 9:37 ` [Qemu-devel] [PATCH 1/3] acpi: move declarations from pc.h to acpi.h Michael S. Tsirkin @ 2013-04-15 9:37 ` Michael S. Tsirkin 2013-04-15 9:37 ` [Qemu-devel] [PATCH 3/3] acpi.h: make it self contained Michael S. Tsirkin ` (2 subsequent siblings) 4 siblings, 0 replies; 13+ messages in thread From: Michael S. Tsirkin @ 2013-04-15 9:37 UTC (permalink / raw) To: qemu-devel Export a function for magling headers, without updating the global acpitables blob. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- hw/acpi/core.c | 97 +++++++++++++++++++++++++++++++++++--------------- include/hw/acpi/acpi.h | 4 +++ 2 files changed, 72 insertions(+), 29 deletions(-) diff --git a/hw/acpi/core.c b/hw/acpi/core.c index 64b8718..b76d58c 100644 --- a/hw/acpi/core.c +++ b/hw/acpi/core.c @@ -78,27 +78,27 @@ static int acpi_checksum(const uint8_t *data, int len) return (-sum) & 0xff; } - -/* Install a copy of the ACPI table specified in @blob. +/* + * Fill an ACPI header in a copy of the ACPI table specified in @blob, + * of length @bloblen. + * An updated blob is allocated and returned in @out_blob, + * updated length in @out_bloblen. * * If @has_header is set, @blob starts with the System Description Table Header * structure. Otherwise, "dfl_hdr" is prepended. In any case, each header field * is optionally overwritten from @hdrs. * * It is valid to call this function with - * (@blob == NULL && bloblen == 0 && !has_header). + * (@blob == NULL && bloblen == 0 &&!@has_header). * * @hdrs->file and @hdrs->data are ignored. * * SIZE_MAX is considered "infinity" in this function. - * - * The number of tables that can be installed is not limited, but the 16-bit - * counter at the beginning of "acpi_tables" wraps around after UINT16_MAX. */ -static void acpi_table_install(const char unsigned *blob, size_t bloblen, - bool has_header, - const struct AcpiTableOptions *hdrs, - Error **errp) +int acpi_table_add_hdr(const char unsigned *blob, size_t bloblen, + bool has_header, const struct AcpiTableOptions *hdrs, + char unsigned **out_blob, size_t *out_bloblen, + Error **errp) { size_t body_start; const char unsigned *hdr_src; @@ -124,7 +124,7 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen, error_setg(errp, "ACPI table claiming to have header is too " "short, available: %zu, expected: %zu", bloblen, body_start); - return; + return -1; } hdr_src = blob; } else { @@ -145,33 +145,29 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen, if (acpi_payload_size > UINT16_MAX) { error_setg(errp, "ACPI table too big, requested: %zu, max: %u", acpi_payload_size, (unsigned)UINT16_MAX); - return; - } - - /* We won't fail from here on. Initialize / extend the globals. */ - if (acpi_tables == NULL) { - acpi_tables_len = sizeof(uint16_t); - acpi_tables = g_malloc0(acpi_tables_len); + return -1; } - acpi_tables = g_realloc(acpi_tables, acpi_tables_len + - ACPI_TABLE_PFX_SIZE + - sizeof dfl_hdr + body_size); + /* We won't fail from here on. Allocate memory. */ + *out_bloblen = sizeof(uint16_t); + *out_blob = g_malloc0(*out_bloblen + + ACPI_TABLE_PFX_SIZE + + sizeof dfl_hdr + body_size); - ext_hdr = (struct acpi_table_header *)(acpi_tables + acpi_tables_len); - acpi_tables_len += ACPI_TABLE_PFX_SIZE; + ext_hdr = (struct acpi_table_header *)(*out_blob + *out_bloblen); + *out_bloblen += ACPI_TABLE_PFX_SIZE; - memcpy(acpi_tables + acpi_tables_len, hdr_src, sizeof dfl_hdr); - acpi_tables_len += sizeof dfl_hdr; + memcpy(*out_blob + *out_bloblen, hdr_src, sizeof dfl_hdr); + *out_bloblen += sizeof dfl_hdr; if (blob != NULL) { - memcpy(acpi_tables + acpi_tables_len, blob + body_start, body_size); - acpi_tables_len += body_size; + memcpy(*out_blob + *out_bloblen, blob + body_start, body_size); + *out_bloblen += body_size; } /* increase number of tables */ - cpu_to_le16wu((uint16_t *)acpi_tables, - le16_to_cpupu((uint16_t *)acpi_tables) + 1u); + cpu_to_le16wu((uint16_t *)*out_blob, + le16_to_cpupu((uint16_t *)*out_blob) + 1u); /* Update the header fields. The strings need not be NUL-terminated. */ changed_fields = 0; @@ -227,6 +223,49 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen, /* recalculate checksum */ ext_hdr->checksum = acpi_checksum((const char unsigned *)ext_hdr + ACPI_TABLE_PFX_SIZE, acpi_payload_size); + return 0; +} + +/* Install a copy of the ACPI table specified in @blob. + * + * If @has_header is set, @blob starts with the System Description Table Header + * structure. Otherwise, "dfl_hdr" is prepended. In any case, each header field + * is optionally overwritten from @hdrs. + * + * It is valid to call this function with + * (@blob == NULL && bloblen == 0 && !has_header). + * + * @hdrs->file and @hdrs->data are ignored. + * + * SIZE_MAX is considered "infinity" in this function. + * + * The number of tables that can be installed is not limited, but the 16-bit + * counter at the beginning of "acpi_tables" wraps around after UINT16_MAX. + */ +void acpi_table_install(const char unsigned *blob, size_t bloblen, + bool has_header, const struct AcpiTableOptions *hdrs, + Error **errp) +{ + char unsigned *out_blob; + size_t out_bloblen; + int r; + + r = acpi_table_add_hdr(blob, bloblen, has_header, hdrs, + &out_blob, &out_bloblen, errp); + if (r < 0) { + return; + } + + /* We won't fail from here on. Initialize / extend the globals. */ + if (acpi_tables == NULL) { + acpi_tables_len = sizeof(uint16_t); + acpi_tables = g_malloc0(acpi_tables_len); + } + + acpi_tables = g_realloc(acpi_tables, acpi_tables_len + out_bloblen); + memcpy(acpi_tables + acpi_tables_len, out_blob, out_bloblen); + acpi_tables_len += out_bloblen; + g_free(out_blob); } void acpi_table_add(const QemuOpts *opts, Error **errp) diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h index 80e955d..35f7e09 100644 --- a/include/hw/acpi/acpi.h +++ b/include/hw/acpi/acpi.h @@ -159,6 +159,10 @@ extern int acpi_enabled; extern char unsigned *acpi_tables; extern size_t acpi_tables_len; +int acpi_table_add_hdr(const char unsigned *blob, size_t bloblen, + bool has_header, const struct AcpiTableOptions *hdrs, + char unsigned **out_blob, size_t *out_bloblen, + Error **errp); void acpi_table_install(const char unsigned *blob, size_t bloblen, bool has_header, const struct AcpiTableOptions *hdrs, Error **errp); -- MST ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH 3/3] acpi.h: make it self contained 2013-04-15 9:37 [Qemu-devel] [PATCH 0/3] acpi: more infrastructure cleanups Michael S. Tsirkin 2013-04-15 9:37 ` [Qemu-devel] [PATCH 1/3] acpi: move declarations from pc.h to acpi.h Michael S. Tsirkin 2013-04-15 9:37 ` [Qemu-devel] [PATCH 2/3] acpi: add acpi_table_add_hdr Michael S. Tsirkin @ 2013-04-15 9:37 ` Michael S. Tsirkin 2013-04-15 11:25 ` [Qemu-devel] [PATCH 0/3] acpi: more infrastructure cleanups Michael S. Tsirkin 2013-04-15 21:10 ` Anthony Liguori 4 siblings, 0 replies; 13+ messages in thread From: Michael S. Tsirkin @ 2013-04-15 9:37 UTC (permalink / raw) To: qemu-devel Headers shouldn't assume another header is included, pull in everything necessary. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- include/hw/acpi/acpi.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h index 35f7e09..88f7378 100644 --- a/include/hw/acpi/acpi.h +++ b/include/hw/acpi/acpi.h @@ -19,6 +19,12 @@ * <http://www.gnu.org/licenses/>. */ +#include "qapi/error.h" +#include "qemu/typedefs.h" +#include "qemu/notify.h" +#include "qemu/option.h" +#include "exec/memory.h" + /* from linux include/acpi/actype.h */ /* Default ACPI register widths */ -- MST ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] acpi: more infrastructure cleanups 2013-04-15 9:37 [Qemu-devel] [PATCH 0/3] acpi: more infrastructure cleanups Michael S. Tsirkin ` (2 preceding siblings ...) 2013-04-15 9:37 ` [Qemu-devel] [PATCH 3/3] acpi.h: make it self contained Michael S. Tsirkin @ 2013-04-15 11:25 ` Michael S. Tsirkin 2013-04-15 13:24 ` Laszlo Ersek 2013-04-15 21:10 ` Anthony Liguori 4 siblings, 1 reply; 13+ messages in thread From: Michael S. Tsirkin @ 2013-04-15 11:25 UTC (permalink / raw) To: qemu-devel On Mon, Apr 15, 2013 at 12:37:37PM +0300, Michael S. Tsirkin wrote: > I wanted to help Laszlo completely move ACPI tables out of seabios in > time for 1.5, but had to put out some fires so didn't manage to do this > in time, and going offline for now. > > Meanwhile, here is some refactoring that I did complete. This does not > do much by itself, but works fine for me and will be helpful down the > road. > > This includes a couple of patches I posted previously > and another one on top. > > Pls consider for 1.5. Just two clarifications 1. This conflicts with Laszlo's MADT change. As such if Laszlo's patch is applied, this will need to be rebased on top. 2. This was lightly tested but there's a holiday here so I run out of time for testing. Will report in a couple of days. > > Michael S. Tsirkin (3): > acpi: move declarations from pc.h to acpi.h > acpi: add acpi_table_add_hdr > acpi.h: make it self contained > > arch_init.c | 1 + > hw/acpi/core.c | 97 +++++++++++++++++++++++++++++++++++--------------- > hw/i386/pc.c | 1 + > hw/i386/pc_piix.c | 1 + > include/hw/acpi/acpi.h | 20 +++++++++++ > include/hw/i386/pc.h | 8 ----- > 6 files changed, 91 insertions(+), 37 deletions(-) > > -- > MST ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] acpi: more infrastructure cleanups 2013-04-15 11:25 ` [Qemu-devel] [PATCH 0/3] acpi: more infrastructure cleanups Michael S. Tsirkin @ 2013-04-15 13:24 ` Laszlo Ersek 2013-04-15 13:31 ` Laszlo Ersek 2013-04-15 14:05 ` Michael S. Tsirkin 0 siblings, 2 replies; 13+ messages in thread From: Laszlo Ersek @ 2013-04-15 13:24 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: Paolo Bonzini, Anthony Liguori, qemu-devel On 04/15/13 13:25, Michael S. Tsirkin wrote: > On Mon, Apr 15, 2013 at 12:37:37PM +0300, Michael S. Tsirkin wrote: >> I wanted to help Laszlo completely move ACPI tables out of seabios in >> time for 1.5, but had to put out some fires so didn't manage to do this >> in time, and going offline for now. >> >> Meanwhile, here is some refactoring that I did complete. This does not >> do much by itself, but works fine for me and will be helpful down the >> road. >> >> This includes a couple of patches I posted previously >> and another one on top. >> >> Pls consider for 1.5. > > Just two clarifications > 1. This conflicts with Laszlo's MADT change. As such if Laszlo's patch is applied, > this will need to be rebased on top. > 2. This was lightly tested but there's a holiday here so I run out of > time for testing. Will report in a couple of days. First, thank you very much for the help! Second, 2/3 breaks the binary format exported under the FW_CFG_ACPI_TABLES key. The format is as follows: * number of tables in entire fw_cfg blob : uint16_t for each table: * ACPI payload size belonging to this table : uint16_t * standard ACPI header * ACPI table contents whereas 2/3 changes the format to: * zero-filled uint16_t (i) for each table: * uint16_t storing value 1 in LE order (ii) * number of bytes in blob segment belonging to this table : uint16_t * standard ACPI header * ACPI table contents (ii) shouldn't exist in per-table storage, the increments should target (i) actually. Another note: 2/3 introduces two ways to spell "has_header" in the leading comments. The first use is "&&!@has_header" (correct with @, but whitespace missing). The second use is "&& !has_header" (from my last series, also see "bloblen"), which has the whitespace OK, but misses the at-sign @ customary in comments when referring to function paramteres. Finally, regarding the purpose of this patch. It seems to be to prepend a standard ACPI header to a "headerles" table. I think we can do without the reallocation. When preparing any ACPI table, we know in advance the size of the ACPI header we're going to need. So we can take it into account when allocating the buffer: fill in the table body, and then call a function that puts the header at front, into existing storage. See pc_acpi_install() in "[Qemu-devel] [qemu PATCH 5/5] i386/pc: build ACPI MADT (APIC) for fw_cfg clients". I propose the following: (1) I'll resend my "[qemu PATCH 0/5] publish etc/acpi/APIC in fw_cfg" series from last Monday, rebased to current (or then) master, with the following changes suggested by Michael in private: (1a) separating some stuff out into different files, (1b) adding licensing bits related to SeaBIOS, (1c) I'll try to consider 1/3 and 3/3 in this series that Paolo has already picked up (may not match or be useful any longer), (2) Then, please review / apply that updated series of mine, (3) then please rebase 2/3 from here on top of (2). Again, I suggest that acpi_table_add_hdr(), if any, fill in preallocated space; this is consistent with updating the checksum too. (4) I *do* anticipate that my code in (2) is not / will not be flexible enough for all tables. However, please split up functions / extract bits / reorganize them only in a series that puts those new extracted functions to use *at once* (probably near the end of said series). I can't reason about "librarization" without actual use. I'll wait for Michael's answer before doing anything. Thanks Laszlo ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] acpi: more infrastructure cleanups 2013-04-15 13:24 ` Laszlo Ersek @ 2013-04-15 13:31 ` Laszlo Ersek 2013-04-15 14:05 ` Michael S. Tsirkin 1 sibling, 0 replies; 13+ messages in thread From: Laszlo Ersek @ 2013-04-15 13:31 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: Paolo Bonzini, Anthony Liguori, qemu-devel On 04/15/13 15:24, Laszlo Ersek wrote: > Second, 2/3 breaks the binary format exported under the > FW_CFG_ACPI_TABLES key. The format is as follows: > > * number of tables in entire fw_cfg blob : uint16_t > for each table: > * ACPI payload size belonging to this table : uint16_t > * standard ACPI header > * ACPI table contents > > whereas 2/3 changes the format to: > > * zero-filled uint16_t (i) > for each table: > * uint16_t storing value 1 in LE order (ii) > * number of bytes in blob segment belonging to this table : uint16_t Sorry, this was mis-editing the email on my part: the _length field is computed alright, it's only the (i)<->(ii) thing that goes wrong. > * standard ACPI header > * ACPI table contents > > (ii) shouldn't exist in per-table storage, the increments should target > (i) actually. Laszlo ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] acpi: more infrastructure cleanups 2013-04-15 13:24 ` Laszlo Ersek 2013-04-15 13:31 ` Laszlo Ersek @ 2013-04-15 14:05 ` Michael S. Tsirkin 1 sibling, 0 replies; 13+ messages in thread From: Michael S. Tsirkin @ 2013-04-15 14:05 UTC (permalink / raw) To: Laszlo Ersek; +Cc: Paolo Bonzini, Anthony Liguori, qemu-devel On Mon, Apr 15, 2013 at 03:24:00PM +0200, Laszlo Ersek wrote: > On 04/15/13 13:25, Michael S. Tsirkin wrote: > > On Mon, Apr 15, 2013 at 12:37:37PM +0300, Michael S. Tsirkin wrote: > >> I wanted to help Laszlo completely move ACPI tables out of seabios in > >> time for 1.5, but had to put out some fires so didn't manage to do this > >> in time, and going offline for now. > >> > >> Meanwhile, here is some refactoring that I did complete. This does not > >> do much by itself, but works fine for me and will be helpful down the > >> road. > >> > >> This includes a couple of patches I posted previously > >> and another one on top. > >> > >> Pls consider for 1.5. > > > > Just two clarifications > > 1. This conflicts with Laszlo's MADT change. As such if Laszlo's patch is applied, > > this will need to be rebased on top. > > 2. This was lightly tested but there's a holiday here so I run out of > > time for testing. Will report in a couple of days. > > First, thank you very much for the help! > > Second, 2/3 breaks the binary format exported under the > FW_CFG_ACPI_TABLES key. The format is as follows: > > * number of tables in entire fw_cfg blob : uint16_t > for each table: > * ACPI payload size belonging to this table : uint16_t > * standard ACPI header > * ACPI table contents > > whereas 2/3 changes the format to: > > * zero-filled uint16_t (i) > for each table: > * uint16_t storing value 1 in LE order (ii) > * number of bytes in blob segment belonging to this table : uint16_t > * standard ACPI header > * ACPI table contents > > (ii) shouldn't exist in per-table storage, the increments should target > (i) actually. > > > Another note: 2/3 introduces two ways to spell "has_header" in the > leading comments. The first use is "&&!@has_header" (correct with @, but > whitespace missing). The second use is "&& !has_header" (from my last > series, also see "bloblen"), which has the whitespace OK, but misses the > at-sign @ customary in comments when referring to function paramteres. > > > Finally, regarding the purpose of this patch. It seems to be to prepend > a standard ACPI header to a "headerles" table. > > I think we can do without the reallocation. When preparing any ACPI > table, we know in advance the size of the ACPI header we're going to > need. So we can take it into account when allocating the buffer: fill in > the table body, and then call a function that puts the header at front, > into existing storage. See pc_acpi_install() in "[Qemu-devel] [qemu > PATCH 5/5] i386/pc: build ACPI MADT (APIC) for fw_cfg clients". > > I propose the following: > > (1) I'll resend my "[qemu PATCH 0/5] publish etc/acpi/APIC in fw_cfg" > series from last Monday, rebased to current (or then) master, with the > following changes suggested by Michael in private: > (1a) separating some stuff out into different files, > (1b) adding licensing bits related to SeaBIOS, > (1c) I'll try to consider 1/3 and 3/3 in this series that Paolo has > already picked up (may not match or be useful any longer), > > (2) Then, please review / apply that updated series of mine, > > (3) then please rebase 2/3 from here on top of (2). Again, I suggest > that acpi_table_add_hdr(), if any, fill in preallocated space; this is > consistent with updating the checksum too. > > (4) I *do* anticipate that my code in (2) is not / will not be flexible > enough for all tables. However, please split up functions / extract bits > / reorganize them only in a series that puts those new extracted > functions to use *at once* (probably near the end of said series). I > can't reason about "librarization" without actual use. > > I'll wait for Michael's answer before doing anything. > > Thanks > Laszlo I'm fine with this. FYI, I'm offline for the rest of today and tomorrow. -- MST ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] acpi: more infrastructure cleanups 2013-04-15 9:37 [Qemu-devel] [PATCH 0/3] acpi: more infrastructure cleanups Michael S. Tsirkin ` (3 preceding siblings ...) 2013-04-15 11:25 ` [Qemu-devel] [PATCH 0/3] acpi: more infrastructure cleanups Michael S. Tsirkin @ 2013-04-15 21:10 ` Anthony Liguori 2013-04-15 21:26 ` Laszlo Ersek 4 siblings, 1 reply; 13+ messages in thread From: Anthony Liguori @ 2013-04-15 21:10 UTC (permalink / raw) To: Michael S. Tsirkin, qemu-devel; +Cc: Paolo Bonzini, Anthony Liguori Applied. Thanks. Regards, Anthony Liguori ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] acpi: more infrastructure cleanups 2013-04-15 21:10 ` Anthony Liguori @ 2013-04-15 21:26 ` Laszlo Ersek 0 siblings, 0 replies; 13+ messages in thread From: Laszlo Ersek @ 2013-04-15 21:26 UTC (permalink / raw) To: Anthony Liguori; +Cc: Paolo Bonzini, qemu-devel, Michael S. Tsirkin On 04/15/13 23:10, Anthony Liguori wrote: > Applied. Thanks. > > Regards, > > Anthony Liguori > Please revert. As I wrote in my review, 2/3 breaks stuff. 1/3 and 3/3 have been picked up by Paolo. Thanks, Laszlo ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2013-04-15 21:24 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-04-15 9:37 [Qemu-devel] [PATCH 0/3] acpi: more infrastructure cleanups Michael S. Tsirkin 2013-04-15 9:37 ` [Qemu-devel] [PATCH 1/3] acpi: move declarations from pc.h to acpi.h Michael S. Tsirkin 2013-04-15 10:42 ` Paolo Bonzini 2013-04-15 11:00 ` Laszlo Ersek 2013-04-15 11:00 ` Paolo Bonzini 2013-04-15 9:37 ` [Qemu-devel] [PATCH 2/3] acpi: add acpi_table_add_hdr Michael S. Tsirkin 2013-04-15 9:37 ` [Qemu-devel] [PATCH 3/3] acpi.h: make it self contained Michael S. Tsirkin 2013-04-15 11:25 ` [Qemu-devel] [PATCH 0/3] acpi: more infrastructure cleanups Michael S. Tsirkin 2013-04-15 13:24 ` Laszlo Ersek 2013-04-15 13:31 ` Laszlo Ersek 2013-04-15 14:05 ` Michael S. Tsirkin 2013-04-15 21:10 ` Anthony Liguori 2013-04-15 21:26 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).