From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56393) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YO3BS-0004SX-4M for qemu-devel@nongnu.org; Wed, 18 Feb 2015 06:52:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YO3BP-00012V-Cx for qemu-devel@nongnu.org; Wed, 18 Feb 2015 06:52:50 -0500 Received: from mx1.redhat.com ([209.132.183.28]:47945) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YO2wW-0001lF-Lo for qemu-devel@nongnu.org; Wed, 18 Feb 2015 06:37:24 -0500 Date: Wed, 18 Feb 2015 12:36:54 +0100 From: "Michael S. Tsirkin" Message-ID: <20150218113654.GB1932@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> <20150217190352.GB28982@redhat.com> <20150218114708.16f3590d@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150218114708.16f3590d@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 11:47:08AM +0100, Igor Mammedov wrote: > On Tue, 17 Feb 2015 20:03:52 +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 > > > > Sorry I'm not merging this in this form, we should use structures > > where possible, this is where C shines, just use the language > > properly. > Although I don't see benefits of using C structures here as > I've wrote in previous reply, I can easily switch internal API functions > to structures, if it helps with merging. I don't think we need to change too much. Let's defer aml_def_block and its users for now, the rest of the series is fine I think. > > > > > > > > > > > > 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