diff mbox series

[v4,01/35] acpi: add helper routines to initialize ACPI tables

Message ID 20210924122802.1455362-2-imammedo@redhat.com (mailing list archive)
State New, archived
Headers show
Series acpi: refactor error prone build_header() and packed structures usage in ACPI tables | expand

Commit Message

Igor Mammedov Sept. 24, 2021, 12:27 p.m. UTC
Patch introduces acpi_table_begin()/ acpi_table_end() API
that hides pointer/offset arithmetic from user as opposed
to build_header(), to prevent errors caused by it [1].

 acpi_table_begin():
     initializes table header and keeps track of
     table data/offsets
 acpi_table_end():
     sets actual table length and tells bios loader
     where table is for the later initialization on
     guest side.

1) commits
   bb9feea43179 x86: acpi: use offset instead of pointer when using build_header()
   4d027afeb3a9 Virt: ACPI: fix qemu assert due to re-assigned table data address

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
CC: eric.auger@redhat.com
---
 include/hw/acpi/aml-build.h | 31 +++++++++++++++++++
 hw/acpi/aml-build.c         | 62 +++++++++++++++++++++++++++++++++++++
 2 files changed, 93 insertions(+)

Comments

Stefan Berger Sept. 24, 2021, 8:52 p.m. UTC | #1
On 9/24/21 8:27 AM, Igor Mammedov wrote:
> Patch introduces acpi_table_begin()/ acpi_table_end() API
> that hides pointer/offset arithmetic from user as opposed
> to build_header(), to prevent errors caused by it [1].
>
>   acpi_table_begin():
>       initializes table header and keeps track of
>       table data/offsets
>   acpi_table_end():
>       sets actual table length and tells bios loader
>       where table is for the later initialization on
>       guest side.
>
> 1) commits
>     bb9feea43179 x86: acpi: use offset instead of pointer when using build_header()
>     4d027afeb3a9 Virt: ACPI: fix qemu assert due to re-assigned table data address
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>

Tested-by: Stefan Berger <stefanb@linux.ibm.com>


> ---
> CC: eric.auger@redhat.com
> ---
>   include/hw/acpi/aml-build.h | 31 +++++++++++++++++++
>   hw/acpi/aml-build.c         | 62 +++++++++++++++++++++++++++++++++++++
>   2 files changed, 93 insertions(+)
>
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 471266d739..4242382399 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -413,6 +413,37 @@ Aml *aml_concatenate(Aml *source1, Aml *source2, Aml *target);
>   Aml *aml_object_type(Aml *object);
>
>   void build_append_int_noprefix(GArray *table, uint64_t value, int size);
> +
> +typedef struct AcpiTable {
> +    const char *sig;
> +    const uint8_t rev;
> +    const char *oem_id;
> +    const char *oem_table_id;
> +    /* private vars tracking table state */
> +    GArray *array;
> +    unsigned table_offset;
> +} AcpiTable;
> +
> +/**
> + * acpi_table_begin:
> + * initializes table header and keeps track of
> + * table data/offsets
> + * @desc: ACPI table descriptor
> + * @array: blob where the ACPI table will be composed/stored.
> + */
> +void acpi_table_begin(AcpiTable *desc, GArray *array);
> +
> +/**
> + * acpi_table_end:
> + * sets actual table length and tells bios loader
> + * where table is for the later initialization on
> + * guest side.
> + * @linker: reference to BIOSLinker object to use for the table
> + * @table: ACPI table descriptor that was used with @acpi_table_begin
> + * counterpart
> + */
> +void acpi_table_end(BIOSLinker *linker, AcpiTable *table);
> +
>   void
>   build_header(BIOSLinker *linker, GArray *table_data,
>                AcpiTableHeader *h, const char *sig, int len, uint8_t rev,
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index d5103e6d7b..229a3eb654 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -52,6 +52,19 @@ static void build_append_byte(GArray *array, uint8_t val)
>       g_array_append_val(array, val);
>   }
>
> +static void build_append_padded_str(GArray *array, const char *str,
> +                                    size_t maxlen, char pad)
> +{
> +    size_t i;
> +    size_t len = strlen(str);
> +
> +    g_assert(len <= maxlen);
> +    g_array_append_vals(array, str, len);
> +    for (i = maxlen - len; i > 0; i--) {
> +        g_array_append_val(array, pad);
> +    }
> +}
> +
>   static void build_append_array(GArray *array, GArray *val)
>   {
>       g_array_append_vals(array, val->data, val->len);
> @@ -1692,6 +1705,55 @@ Aml *aml_object_type(Aml *object)
>       return var;
>   }
>
> +void acpi_table_begin(AcpiTable *desc, GArray *array)
> +{
> +
> +    desc->array = array;
> +    desc->table_offset = array->len;
> +
> +    /*
> +     * ACPI spec 1.0b
> +     * 5.2.3 System Description Table Header
> +     */
> +    g_assert(strlen(desc->sig) == 4);
> +    g_array_append_vals(array, desc->sig, 4); /* Signature */
> +    /*
> +     * reserve space for Length field, which will be patched by
> +     * acpi_table_end() when the table creation is finished.
> +     */
> +    build_append_int_noprefix(array, 0, 4); /* Length */
> +    build_append_int_noprefix(array, desc->rev, 1); /* Revision */
> +    build_append_int_noprefix(array, 0, 1); /* Checksum */
> +    build_append_padded_str(array, desc->oem_id, 6, ' '); /* OEMID */
> +    /* OEM Table ID */
> +    build_append_padded_str(array, desc->oem_table_id, 8, ' ');
> +    build_append_int_noprefix(array, 1, 4); /* OEM Revision */
> +    g_array_append_vals(array, ACPI_BUILD_APPNAME8, 4); /* Creator ID */
> +    build_append_int_noprefix(array, 1, 4); /* Creator Revision */
> +}
> +
> +void acpi_table_end(BIOSLinker *linker, AcpiTable *desc)
> +{
> +    /*
> +     * ACPI spec 1.0b
> +     * 5.2.3 System Description Table Header
> +     * Table 5-2 DESCRIPTION_HEADER Fields
> +     */
> +    const unsigned checksum_offset = 9;
> +    uint32_t table_len = desc->array->len - desc->table_offset;
> +    uint32_t table_len_le = cpu_to_le32(table_len);
> +    gchar *len_ptr = &desc->array->data[desc->table_offset + 4];
> +
> +    /* patch "Length" field that has been reserved by acpi_table_begin()
> +     * to the actual length, i.e. accumulated table length from
> +     * acpi_table_begin() till acpi_table_end()
> +     */
> +    memcpy(len_ptr, &table_len_le, sizeof table_len_le);
> +
> +    bios_linker_loader_add_checksum(linker, ACPI_BUILD_TABLE_FILE,
> +        desc->table_offset, table_len, desc->table_offset + checksum_offset);
> +}
> +
>   void
>   build_header(BIOSLinker *linker, GArray *table_data,
>                AcpiTableHeader *h, const char *sig, int len, uint8_t rev,
Yanan Wang Sept. 26, 2021, 9:16 a.m. UTC | #2
Hi,

On 2021/9/24 20:27, Igor Mammedov wrote:
> Patch introduces acpi_table_begin()/ acpi_table_end() API
> that hides pointer/offset arithmetic from user as opposed
> to build_header(), to prevent errors caused by it [1].
>
>   acpi_table_begin():
>       initializes table header and keeps track of
>       table data/offsets
>   acpi_table_end():
>       sets actual table length and tells bios loader
>       where table is for the later initialization on
>       guest side.
>
> 1) commits
>     bb9feea43179 x86: acpi: use offset instead of pointer when using build_header()
>     4d027afeb3a9 Virt: ACPI: fix qemu assert due to re-assigned table data address
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> ---
> CC: eric.auger@redhat.com
> ---
>   include/hw/acpi/aml-build.h | 31 +++++++++++++++++++
>   hw/acpi/aml-build.c         | 62 +++++++++++++++++++++++++++++++++++++
>   2 files changed, 93 insertions(+)
>
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 471266d739..4242382399 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -413,6 +413,37 @@ Aml *aml_concatenate(Aml *source1, Aml *source2, Aml *target);
>   Aml *aml_object_type(Aml *object);
>   
>   void build_append_int_noprefix(GArray *table, uint64_t value, int size);
> +
> +typedef struct AcpiTable {
> +    const char *sig;
> +    const uint8_t rev;
> +    const char *oem_id;
> +    const char *oem_table_id;
> +    /* private vars tracking table state */
> +    GArray *array;
> +    unsigned table_offset;
> +} AcpiTable;
> +
> +/**
> + * acpi_table_begin:
> + * initializes table header and keeps track of
> + * table data/offsets
> + * @desc: ACPI table descriptor
> + * @array: blob where the ACPI table will be composed/stored.
> + */
> +void acpi_table_begin(AcpiTable *desc, GArray *array);
> +
> +/**
> + * acpi_table_end:
> + * sets actual table length and tells bios loader
> + * where table is for the later initialization on
> + * guest side.
> + * @linker: reference to BIOSLinker object to use for the table
> + * @table: ACPI table descriptor that was used with @acpi_table_begin
> + * counterpart
> + */
> +void acpi_table_end(BIOSLinker *linker, AcpiTable *table);
> +
>   void
>   build_header(BIOSLinker *linker, GArray *table_data,
>                AcpiTableHeader *h, const char *sig, int len, uint8_t rev,
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index d5103e6d7b..229a3eb654 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -52,6 +52,19 @@ static void build_append_byte(GArray *array, uint8_t val)
>       g_array_append_val(array, val);
>   }
>   
> +static void build_append_padded_str(GArray *array, const char *str,
> +                                    size_t maxlen, char pad)
> +{
> +    size_t i;
> +    size_t len = strlen(str);
> +
> +    g_assert(len <= maxlen);
> +    g_array_append_vals(array, str, len);
> +    for (i = maxlen - len; i > 0; i--) {
> +        g_array_append_val(array, pad);
> +    }
> +}
> +
>   static void build_append_array(GArray *array, GArray *val)
>   {
>       g_array_append_vals(array, val->data, val->len);
> @@ -1692,6 +1705,55 @@ Aml *aml_object_type(Aml *object)
>       return var;
>   }
>   
> +void acpi_table_begin(AcpiTable *desc, GArray *array)
> +{
> +
> +    desc->array = array;
> +    desc->table_offset = array->len;
> +
> +    /*
> +     * ACPI spec 1.0b
> +     * 5.2.3 System Description Table Header
> +     */
> +    g_assert(strlen(desc->sig) == 4);
> +    g_array_append_vals(array, desc->sig, 4); /* Signature */
> +    /*
> +     * reserve space for Length field, which will be patched by
> +     * acpi_table_end() when the table creation is finished.
> +     */
> +    build_append_int_noprefix(array, 0, 4); /* Length */
> +    build_append_int_noprefix(array, desc->rev, 1); /* Revision */
> +    build_append_int_noprefix(array, 0, 1); /* Checksum */
> +    build_append_padded_str(array, desc->oem_id, 6, ' '); /* OEMID */
> +    /* OEM Table ID */
> +    build_append_padded_str(array, desc->oem_table_id, 8, ' ');
> +    build_append_int_noprefix(array, 1, 4); /* OEM Revision */
> +    g_array_append_vals(array, ACPI_BUILD_APPNAME8, 4); /* Creator ID */
> +    build_append_int_noprefix(array, 1, 4); /* Creator Revision */
> +}
> +
> +void acpi_table_end(BIOSLinker *linker, AcpiTable *desc)
> +{
> +    /*
> +     * ACPI spec 1.0b
> +     * 5.2.3 System Description Table Header
> +     * Table 5-2 DESCRIPTION_HEADER Fields
> +     */
> +    const unsigned checksum_offset = 9;
> +    uint32_t table_len = desc->array->len - desc->table_offset;
> +    uint32_t table_len_le = cpu_to_le32(table_len);
> +    gchar *len_ptr = &desc->array->data[desc->table_offset + 4];
> +
> +    /* patch "Length" field that has been reserved by acpi_table_begin()
> +     * to the actual length, i.e. accumulated table length from
> +     * acpi_table_begin() till acpi_table_end()
> +     */
> +    memcpy(len_ptr, &table_len_le, sizeof table_len_le);
> +
> +    bios_linker_loader_add_checksum(linker, ACPI_BUILD_TABLE_FILE,
> +        desc->table_offset, table_len, desc->table_offset + checksum_offset);
> +}
> +
>   void
>   build_header(BIOSLinker *linker, GArray *table_data,
>                AcpiTableHeader *h, const char *sig, int len, uint8_t rev,
I have rewrote the ACPI PPTT table generation patches based on this 
series, and
the testing output of PPTT was as expected. So for the functionality of 
the APIs:

Tested-by: Yanan Wang <wangyanan55@huawei.com>

Thanks,
Yanan
.
diff mbox series

Patch

diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 471266d739..4242382399 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -413,6 +413,37 @@  Aml *aml_concatenate(Aml *source1, Aml *source2, Aml *target);
 Aml *aml_object_type(Aml *object);
 
 void build_append_int_noprefix(GArray *table, uint64_t value, int size);
+
+typedef struct AcpiTable {
+    const char *sig;
+    const uint8_t rev;
+    const char *oem_id;
+    const char *oem_table_id;
+    /* private vars tracking table state */
+    GArray *array;
+    unsigned table_offset;
+} AcpiTable;
+
+/**
+ * acpi_table_begin:
+ * initializes table header and keeps track of
+ * table data/offsets
+ * @desc: ACPI table descriptor
+ * @array: blob where the ACPI table will be composed/stored.
+ */
+void acpi_table_begin(AcpiTable *desc, GArray *array);
+
+/**
+ * acpi_table_end:
+ * sets actual table length and tells bios loader
+ * where table is for the later initialization on
+ * guest side.
+ * @linker: reference to BIOSLinker object to use for the table
+ * @table: ACPI table descriptor that was used with @acpi_table_begin
+ * counterpart
+ */
+void acpi_table_end(BIOSLinker *linker, AcpiTable *table);
+
 void
 build_header(BIOSLinker *linker, GArray *table_data,
              AcpiTableHeader *h, const char *sig, int len, uint8_t rev,
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index d5103e6d7b..229a3eb654 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -52,6 +52,19 @@  static void build_append_byte(GArray *array, uint8_t val)
     g_array_append_val(array, val);
 }
 
+static void build_append_padded_str(GArray *array, const char *str,
+                                    size_t maxlen, char pad)
+{
+    size_t i;
+    size_t len = strlen(str);
+
+    g_assert(len <= maxlen);
+    g_array_append_vals(array, str, len);
+    for (i = maxlen - len; i > 0; i--) {
+        g_array_append_val(array, pad);
+    }
+}
+
 static void build_append_array(GArray *array, GArray *val)
 {
     g_array_append_vals(array, val->data, val->len);
@@ -1692,6 +1705,55 @@  Aml *aml_object_type(Aml *object)
     return var;
 }
 
+void acpi_table_begin(AcpiTable *desc, GArray *array)
+{
+
+    desc->array = array;
+    desc->table_offset = array->len;
+
+    /*
+     * ACPI spec 1.0b
+     * 5.2.3 System Description Table Header
+     */
+    g_assert(strlen(desc->sig) == 4);
+    g_array_append_vals(array, desc->sig, 4); /* Signature */
+    /*
+     * reserve space for Length field, which will be patched by
+     * acpi_table_end() when the table creation is finished.
+     */
+    build_append_int_noprefix(array, 0, 4); /* Length */
+    build_append_int_noprefix(array, desc->rev, 1); /* Revision */
+    build_append_int_noprefix(array, 0, 1); /* Checksum */
+    build_append_padded_str(array, desc->oem_id, 6, ' '); /* OEMID */
+    /* OEM Table ID */
+    build_append_padded_str(array, desc->oem_table_id, 8, ' ');
+    build_append_int_noprefix(array, 1, 4); /* OEM Revision */
+    g_array_append_vals(array, ACPI_BUILD_APPNAME8, 4); /* Creator ID */
+    build_append_int_noprefix(array, 1, 4); /* Creator Revision */
+}
+
+void acpi_table_end(BIOSLinker *linker, AcpiTable *desc)
+{
+    /*
+     * ACPI spec 1.0b
+     * 5.2.3 System Description Table Header
+     * Table 5-2 DESCRIPTION_HEADER Fields
+     */
+    const unsigned checksum_offset = 9;
+    uint32_t table_len = desc->array->len - desc->table_offset;
+    uint32_t table_len_le = cpu_to_le32(table_len);
+    gchar *len_ptr = &desc->array->data[desc->table_offset + 4];
+
+    /* patch "Length" field that has been reserved by acpi_table_begin()
+     * to the actual length, i.e. accumulated table length from
+     * acpi_table_begin() till acpi_table_end()
+     */
+    memcpy(len_ptr, &table_len_le, sizeof table_len_le);
+
+    bios_linker_loader_add_checksum(linker, ACPI_BUILD_TABLE_FILE,
+        desc->table_offset, table_len, desc->table_offset + checksum_offset);
+}
+
 void
 build_header(BIOSLinker *linker, GArray *table_data,
              AcpiTableHeader *h, const char *sig, int len, uint8_t rev,