From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34744) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YNnf5-0006CR-AP for qemu-devel@nongnu.org; Tue, 17 Feb 2015 14:18:24 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YNnf1-0002hB-6N for qemu-devel@nongnu.org; Tue, 17 Feb 2015 14:18:23 -0500 Received: from mx1.redhat.com ([209.132.183.28]:34546) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YNnf0-0002h3-Ut for qemu-devel@nongnu.org; Tue, 17 Feb 2015 14:18:19 -0500 Date: Tue, 17 Feb 2015 20:15:42 +0100 From: "Michael S. Tsirkin" Message-ID: <20150217191542.GC29473@redhat.com> References: <1423479254-15342-1-git-send-email-imammedo@redhat.com> <1423479254-15342-6-git-send-email-imammedo@redhat.com> <20150217163555.GA25875@redhat.com> <20150217174726.29d69713@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150217174726.29d69713@nial.brq.redhat.com> Subject: Re: [Qemu-devel] [PATCH v3 05/52] acpi: add aml_def_block() term List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: drjones@redhat.com, marcel.a@redhat.com, claudio.fontana@huawei.com, qemu-devel@nongnu.org, zhaoshenglong@huawei.com On Tue, Feb 17, 2015 at 05:47:26PM +0100, Igor Mammedov wrote: > On Tue, 17 Feb 2015 17:35:55 +0100 > "Michael S. Tsirkin" wrote: > > > On Mon, Feb 09, 2015 at 10:53:27AM +0000, Igor Mammedov wrote: > > > Signed-off-by: Igor Mammedov > > > --- > > > hw/acpi/aml-build.c | 38 ++++++++++++++++++++++++++++++++++++++ > > > hw/i386/acpi-build.c | 4 ---- > > > include/hw/acpi/aml-build.h | 10 ++++++++++ > > > 3 files changed, 48 insertions(+), 4 deletions(-) > > > > > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c > > > index 67d1371..cb1a1bd 100644 > > > --- a/hw/acpi/aml-build.c > > > +++ b/hw/acpi/aml-build.c > > > @@ -335,3 +335,41 @@ void aml_append(Aml *parent_ctx, Aml *child) > > > } > > > build_append_array(parent_ctx->buf, child->buf); > > > } > > > + > > > +/* > > > + * ACPI 1.0b: 16.2.1 Top Level AML > > > + * 5.2.3 System Description Table Header > > > + * > > > + * ACPI 5.0: 20.2.1 Table and Table Header Encoding > > > + */ > > > +Aml *aml_def_block(const char *signature, uint8_t revision, > > > + const char *oem_id, const char *oem_table_id, > > > + uint32_t oem_revision, uint32_t creator_id, > > > + uint32_t creator_revision) > > > +{ > > > + int len; > > > + Aml *var = aml_alloc(); > > > + var->block_flags = AML_DEF_BLOCK; > > > + > > > + assert(strlen(signature) == 4); > > > + g_array_append_vals(var->buf, signature, 4); > > > + build_append_value(var->buf, 0, 4); /* Length place holder */ > > > + build_append_byte(var->buf, revision); > > > + build_append_byte(var->buf, 0); /* place holder for Checksum */ > > > + > > > + len = strlen(oem_id); > > > + assert(len <= 6); > > > + g_array_append_vals(var->buf, oem_id, len); > > > + g_array_append_vals(var->buf, "\0\0\0\0\0\0\0\0", 6 - len); > > > + > > > + len = strlen(oem_table_id); > > > + assert(len <= 8); > > > + g_array_append_vals(var->buf, oem_table_id, len); > > > + g_array_append_vals(var->buf, "\0\0\0\0\0\0\0\0", 8 - len); > > > + > > > + build_append_value(var->buf, oem_revision, 4); > > > + build_append_value(var->buf, creator_id, 4); > > > + build_append_value(var->buf, creator_revision, 4); > > > + > > > + return var; > > > > > > Please don't do this. > > First, this is not a definition block encoding, so name > > is wrong. > > > > Second, we already have functions to fill in headers. > > So just call them. > > > > Filling structures byte by byte is unreadable - > > I know ACPI spec often makes us do this but > > when it doesn't, we should not do it. > it does, in ACPI 5.1, > > 20.2.1 Table and Table Header Encoding > AMLCode := DefBlockHeader TermList > DefBlockHeader := TableSignature TableLength SpecCompliance CheckSum OemID > OemTableID OemRevision CreatorID CreatorRevision So spec explains it very badly, but these all are fixed width fields. Filling them in using add_byte just makes for hard to read code. > > > > Pls keep using build_header for now. > > > > > > > +} > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > > index a1bf450..553c86b 100644 > > > --- a/hw/i386/acpi-build.c > > > +++ b/hw/i386/acpi-build.c > > > @@ -254,10 +254,6 @@ static void acpi_get_pci_info(PcPciInfo *info) > > > NULL); > > > } > > > > > > -#define ACPI_BUILD_APPNAME "Bochs" > > > -#define ACPI_BUILD_APPNAME6 "BOCHS " > > > -#define ACPI_BUILD_APPNAME4 "BXPC" > > > - > > > #define ACPI_BUILD_RSDP_FILE "etc/acpi/rsdp" > > > #define ACPI_BUILD_TPMLOG_FILE "etc/tpm/log" > > > > > > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h > > > index 4033796..2610336 100644 > > > --- a/include/hw/acpi/aml-build.h > > > +++ b/include/hw/acpi/aml-build.h > > > @@ -6,6 +6,10 @@ > > > #include "qemu/compiler.h" > > > > > > #define ACPI_BUILD_TABLE_FILE "etc/acpi/tables" > > > +#define ACPI_BUILD_APPNAME "Bochs" > > > +#define ACPI_BUILD_APPNAME6 "BOCHS " > > > +#define ACPI_BUILD_APPNAME4 "BXPC" > > > +#define ACPI_BUILD_APPNAME4_HEX 0x43505842 > > > > > > typedef enum { > > > AML_HELPER = 0, > > > @@ -65,6 +69,12 @@ void free_aml_allocator(void); > > > */ > > > void aml_append(Aml *parent_ctx, Aml *child); > > > > > > +/* Block AML object primitives */ > > > +Aml *aml_def_block(const char *signature, uint8_t revision, > > > + const char *oem_id, const char *oem_table_id, > > > + uint32_t oem_revision, uint32_t creator_id, > > > + uint32_t creator_revision); > > > + > > > /* other helpers */ > > > GArray *build_alloc_array(void); > > > void build_free_array(GArray *array); > > > -- > > > 1.8.3.1