From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59875) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YO3Fj-00086B-0c for qemu-devel@nongnu.org; Wed, 18 Feb 2015 06:57:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YO3Fe-0003Ef-F3 for qemu-devel@nongnu.org; Wed, 18 Feb 2015 06:57:14 -0500 Received: from mx1.redhat.com ([209.132.183.28]:51632) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YO2us-00012K-3O for qemu-devel@nongnu.org; Wed, 18 Feb 2015 06:35:42 -0500 Date: Wed, 18 Feb 2015 12:35:34 +0100 From: "Michael S. Tsirkin" Message-ID: <20150218113534.GA1932@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> <20150217191542.GC29473@redhat.com> <20150218105054.11478768@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150218105054.11478768@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 Wed, Feb 18, 2015 at 10:50:54AM +0100, Igor Mammedov wrote: > On Tue, 17 Feb 2015 20:15:42 +0100 > "Michael S. Tsirkin" wrote: > > > 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. > If it were only add_byte, I'd agree but it basically composes > by whole fields and in this particular case is easy to > verify just by looking to spec, just like the rest of AML > terms does. > And one doesn't have to care about endianness because > build_append_value() packs integer fields as expected > by spec as opposed to using C struct-s. things like 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); are kind of ugly, are they not? 8 is repeated 3 times here, and you need to go look at spec to figure out what 8 stands for. Besides, we aren't yet getting rid of build_header, are we? How about this, we get rest of this code in, then work on rewriting build_header everywhere? > > > > > > > > > > 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