1. Patchew
  2. QEMU
  3. [PATCH v14 00/10] acpi: Error Record Serialization Table, ERST, support for QEMU
  4. View patch

[PATCH v14 06/10] ACPI ERST: build the ACPI ERST table

Eric DeVolder posted 10 patches 4 years ago
Maintainers: Thomas Huth <thuth@redhat.com>, Laurent Vivier <lvivier@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Igor Mammedov <imammedo@redhat.com>, Ani Sinha <ani@anisinha.ca>, Richard Henderson <richard.henderson@linaro.org>, "Michael S. Tsirkin" <mst@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, Paolo Bonzini <pbonzini@redhat.com>
There is a newer version of this series
[PATCH v14 06/10] ACPI ERST: build the ACPI ERST table
Posted by Eric DeVolder 4 years ago 
This builds the ACPI ERST table to inform OSPM how to communicate
with the acpi-erst device.

Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
---
 hw/acpi/erst.c | 225 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 225 insertions(+)

diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
index fe9ba51..5d5a639 100644
--- a/hw/acpi/erst.c
+++ b/hw/acpi/erst.c
@@ -59,6 +59,27 @@
 #define STATUS_RECORD_STORE_EMPTY     0x04
 #define STATUS_RECORD_NOT_FOUND       0x05
 
+/* ACPI 4.0: Table 17-19 Serialization Instructions */
+#define INST_READ_REGISTER                 0x00
+#define INST_READ_REGISTER_VALUE           0x01
+#define INST_WRITE_REGISTER                0x02
+#define INST_WRITE_REGISTER_VALUE          0x03
+#define INST_NOOP                          0x04
+#define INST_LOAD_VAR1                     0x05
+#define INST_LOAD_VAR2                     0x06
+#define INST_STORE_VAR1                    0x07
+#define INST_ADD                           0x08
+#define INST_SUBTRACT                      0x09
+#define INST_ADD_VALUE                     0x0A
+#define INST_SUBTRACT_VALUE                0x0B
+#define INST_STALL                         0x0C
+#define INST_STALL_WHILE_TRUE              0x0D
+#define INST_SKIP_NEXT_INSTRUCTION_IF_TRUE 0x0E
+#define INST_GOTO                          0x0F
+#define INST_SET_SRC_ADDRESS_BASE          0x10
+#define INST_SET_DST_ADDRESS_BASE          0x11
+#define INST_MOVE_DATA                     0x12
+
 /* UEFI 2.1: Appendix N Common Platform Error Record */
 #define UEFI_CPER_RECORD_MIN_SIZE 128U
 #define UEFI_CPER_RECORD_LENGTH_OFFSET 20U
@@ -172,6 +193,210 @@ typedef struct {
 
 /*******************************************************************/
 /*******************************************************************/
+typedef struct {
+    GArray *table_data;
+    pcibus_t bar;
+    uint8_t instruction;
+    uint8_t flags;
+    uint8_t register_bit_width;
+    pcibus_t register_offset;
+} BuildSerializationInstructionEntry;
+
+/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */
+static void build_serialization_instruction(
+    BuildSerializationInstructionEntry *e,
+    uint8_t serialization_action,
+    uint64_t value)
+{
+    /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */
+    struct AcpiGenericAddress gas;
+    uint64_t mask;
+
+    /* Serialization Action */
+    build_append_int_noprefix(e->table_data, serialization_action, 1);
+    /* Instruction */
+    build_append_int_noprefix(e->table_data, e->instruction, 1);
+    /* Flags */
+    build_append_int_noprefix(e->table_data, e->flags, 1);
+    /* Reserved */
+    build_append_int_noprefix(e->table_data, 0, 1);
+    /* Register Region */
+    gas.space_id = AML_SYSTEM_MEMORY;
+    gas.bit_width = e->register_bit_width;
+    gas.bit_offset = 0;
+    gas.access_width = ctz32(e->register_bit_width) - 2;
+    gas.address = (uint64_t)(e->bar + e->register_offset);
+    build_append_gas_from_struct(e->table_data, &gas);
+    /* Value */
+    build_append_int_noprefix(e->table_data, value, 8);
+    /* Mask */
+    mask = (1ULL << (e->register_bit_width - 1) << 1) - 1;
+    build_append_int_noprefix(e->table_data, mask, 8);
+}
+
+/* ACPI 4.0: 17.4.1 Serialization Action Table */
+void build_erst(GArray *table_data, BIOSLinker *linker, Object *erst_dev,
+    const char *oem_id, const char *oem_table_id)
+{
+    /*
+     * Serialization Action Table
+     * The serialization action table must be generated first
+     * so that its size can be known in order to populate the
+     * Instruction Entry Count field.
+     */
+    GArray *table_instruction_data = g_array_new(FALSE, FALSE, sizeof(char));
+    pcibus_t bar0 = pci_get_bar_addr(PCI_DEVICE(erst_dev), 0);
+    AcpiTable table = { .sig = "ERST", .rev = 1, .oem_id = oem_id,
+                        .oem_table_id = oem_table_id };
+    /* Contexts for the different ways ACTION and VALUE are accessed */
+    BuildSerializationInstructionEntry rd_value_32_val = {
+        .table_data = table_instruction_data,
+        .bar = bar0,
+        .instruction = INST_READ_REGISTER_VALUE,
+        .flags = 0,
+        .register_bit_width = 32,
+        .register_offset = ERST_VALUE_OFFSET,
+    };
+    BuildSerializationInstructionEntry rd_value_32 = {
+        .table_data = table_instruction_data,
+        .bar = bar0,
+        .instruction = INST_READ_REGISTER,
+        .flags = 0,
+        .register_bit_width = 32,
+        .register_offset = ERST_VALUE_OFFSET,
+    };
+    BuildSerializationInstructionEntry rd_value_64 = {
+        .table_data = table_instruction_data,
+        .bar = bar0,
+        .instruction = INST_READ_REGISTER,
+        .flags = 0,
+        .register_bit_width = 64,
+        .register_offset = ERST_VALUE_OFFSET,
+    };
+    BuildSerializationInstructionEntry wr_value_32_val = {
+        .table_data = table_instruction_data,
+        .bar = bar0,
+        .instruction = INST_WRITE_REGISTER_VALUE,
+        .flags = 0,
+        .register_bit_width = 32,
+        .register_offset = ERST_VALUE_OFFSET,
+    };
+    BuildSerializationInstructionEntry wr_value_32 = {
+        .table_data = table_instruction_data,
+        .bar = bar0,
+        .instruction = INST_WRITE_REGISTER,
+        .flags = 0,
+        .register_bit_width = 32,
+        .register_offset = ERST_VALUE_OFFSET,
+    };
+    BuildSerializationInstructionEntry wr_value_64 = {
+        .table_data = table_instruction_data,
+        .bar = bar0,
+        .instruction = INST_WRITE_REGISTER,
+        .flags = 0,
+        .register_bit_width = 64,
+        .register_offset = ERST_VALUE_OFFSET,
+    };
+    BuildSerializationInstructionEntry wr_action = {
+        .table_data = table_instruction_data,
+        .bar = bar0,
+        .instruction = INST_WRITE_REGISTER_VALUE,
+        .flags = 0,
+        .register_bit_width = 32,
+        .register_offset = ERST_ACTION_OFFSET,
+    };
+    unsigned action;
+
+    trace_acpi_erst_pci_bar_0(bar0);
+
+    /* Serialization Instruction Entries */
+    action = ACTION_BEGIN_WRITE_OPERATION;
+    build_serialization_instruction(&wr_action, action, action);
+
+    action = ACTION_BEGIN_READ_OPERATION;
+    build_serialization_instruction(&wr_action, action, action);
+
+    action = ACTION_BEGIN_CLEAR_OPERATION;
+    build_serialization_instruction(&wr_action, action, action);
+
+    action = ACTION_END_OPERATION;
+    build_serialization_instruction(&wr_action, action, action);
+
+    action = ACTION_SET_RECORD_OFFSET;
+    build_serialization_instruction(&wr_value_32, action, 0);
+    build_serialization_instruction(&wr_action, action, action);
+
+    action = ACTION_EXECUTE_OPERATION;
+    build_serialization_instruction(&wr_value_32_val, action,
+        ERST_EXECUTE_OPERATION_MAGIC);
+    build_serialization_instruction(&wr_action, action, action);
+
+    action = ACTION_CHECK_BUSY_STATUS;
+    build_serialization_instruction(&wr_action, action, action);
+    build_serialization_instruction(&rd_value_32_val, action, 0x01);
+
+    action = ACTION_GET_COMMAND_STATUS;
+    build_serialization_instruction(&wr_action, action, action);
+    build_serialization_instruction(&rd_value_32, action, 0);
+
+    action = ACTION_GET_RECORD_IDENTIFIER;
+    build_serialization_instruction(&wr_action, action, action);
+    build_serialization_instruction(&rd_value_64, action, 0);
+
+    action = ACTION_SET_RECORD_IDENTIFIER;
+    build_serialization_instruction(&wr_value_64, action, 0);
+    build_serialization_instruction(&wr_action, action, action);
+
+    action = ACTION_GET_RECORD_COUNT;
+    build_serialization_instruction(&wr_action, action, action);
+    build_serialization_instruction(&rd_value_32, action, 0);
+
+    action = ACTION_BEGIN_DUMMY_WRITE_OPERATION;
+    build_serialization_instruction(&wr_action, action, action);
+
+    action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE;
+    build_serialization_instruction(&wr_action, action, action);
+    build_serialization_instruction(&rd_value_64, action, 0);
+
+    action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH;
+    build_serialization_instruction(&wr_action, action, action);
+    build_serialization_instruction(&rd_value_64, action, 0);
+
+    action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES;
+    build_serialization_instruction(&wr_action, action, action);
+    build_serialization_instruction(&rd_value_32, action, 0);
+
+    action = ACTION_GET_EXECUTE_OPERATION_TIMINGS;
+    build_serialization_instruction(&wr_action, action, action);
+    build_serialization_instruction(&rd_value_64, action, 0);
+
+    /* Serialization Header */
+    acpi_table_begin(&table, table_data);
+
+    /* Serialization Header Size */
+    build_append_int_noprefix(table_data, 48, 4);
+
+    /* Reserved */
+    build_append_int_noprefix(table_data,  0, 4);
+
+    /*
+     * Instruction Entry Count
+     * Each instruction entry is 32 bytes
+     */
+    g_assert((table_instruction_data->len) % 32 == 0);
+    build_append_int_noprefix(table_data,
+        (table_instruction_data->len / 32), 4);
+
+    /* Serialization Instruction Entries */
+    g_array_append_vals(table_data, table_instruction_data->data,
+        table_instruction_data->len);
+    g_array_free(table_instruction_data, TRUE);
+
+    acpi_table_end(linker, &table);
+}
+
+/*******************************************************************/
+/*******************************************************************/
 static uint8_t *get_nvram_ptr_by_index(ERSTDeviceState *s, unsigned index)
 {
     uint8_t *rc = NULL;
-- 
1.8.3.1
Re: [PATCH v14 06/10] ACPI ERST: build the ACPI ERST table
Posted by Ani Sinha 4 years ago 

On Wed, 26 Jan 2022, Eric DeVolder wrote:

> This builds the ACPI ERST table to inform OSPM how to communicate
> with the acpi-erst device.

There might be more optimizations possible but I think we have messaged
this code enough. We can further rework the code if needed in subsequent
patches once this is pushed.

>
> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>

with some minor comments,

Reviewed-by: Ani Sinha <ani@anisinha.ca>

>  hw/acpi/erst.c | 225 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 225 insertions(+)
>
> diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
> index fe9ba51..5d5a639 100644
> --- a/hw/acpi/erst.c
> +++ b/hw/acpi/erst.c
> @@ -59,6 +59,27 @@
>  #define STATUS_RECORD_STORE_EMPTY     0x04
>  #define STATUS_RECORD_NOT_FOUND       0x05
>
> +/* ACPI 4.0: Table 17-19 Serialization Instructions */
> +#define INST_READ_REGISTER                 0x00
> +#define INST_READ_REGISTER_VALUE           0x01
> +#define INST_WRITE_REGISTER                0x02
> +#define INST_WRITE_REGISTER_VALUE          0x03
> +#define INST_NOOP                          0x04
> +#define INST_LOAD_VAR1                     0x05
> +#define INST_LOAD_VAR2                     0x06
> +#define INST_STORE_VAR1                    0x07
> +#define INST_ADD                           0x08
> +#define INST_SUBTRACT                      0x09
> +#define INST_ADD_VALUE                     0x0A
> +#define INST_SUBTRACT_VALUE                0x0B
> +#define INST_STALL                         0x0C
> +#define INST_STALL_WHILE_TRUE              0x0D
> +#define INST_SKIP_NEXT_INSTRUCTION_IF_TRUE 0x0E
> +#define INST_GOTO                          0x0F
> +#define INST_SET_SRC_ADDRESS_BASE          0x10
> +#define INST_SET_DST_ADDRESS_BASE          0x11
> +#define INST_MOVE_DATA                     0x12
> +
>  /* UEFI 2.1: Appendix N Common Platform Error Record */
>  #define UEFI_CPER_RECORD_MIN_SIZE 128U
>  #define UEFI_CPER_RECORD_LENGTH_OFFSET 20U
> @@ -172,6 +193,210 @@ typedef struct {
>
>  /*******************************************************************/
>  /*******************************************************************/
> +typedef struct {
> +    GArray *table_data;
> +    pcibus_t bar;
> +    uint8_t instruction;
> +    uint8_t flags;
> +    uint8_t register_bit_width;
> +    pcibus_t register_offset;
> +} BuildSerializationInstructionEntry;
> +
> +/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */
> +static void build_serialization_instruction(
> +    BuildSerializationInstructionEntry *e,
> +    uint8_t serialization_action,
> +    uint64_t value)
> +{
> +    /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */
> +    struct AcpiGenericAddress gas;
> +    uint64_t mask;
> +
> +    /* Serialization Action */
> +    build_append_int_noprefix(e->table_data, serialization_action, 1);
> +    /* Instruction */
> +    build_append_int_noprefix(e->table_data, e->instruction, 1);
> +    /* Flags */
> +    build_append_int_noprefix(e->table_data, e->flags, 1);
> +    /* Reserved */
> +    build_append_int_noprefix(e->table_data, 0, 1);
> +    /* Register Region */
> +    gas.space_id = AML_SYSTEM_MEMORY;
> +    gas.bit_width = e->register_bit_width;
> +    gas.bit_offset = 0;
> +    gas.access_width = ctz32(e->register_bit_width) - 2;

Should this be casted as unit8_t?

> +    gas.address = (uint64_t)(e->bar + e->register_offset);
> +    build_append_gas_from_struct(e->table_data, &gas);
> +    /* Value */
> +    build_append_int_noprefix(e->table_data, value, 8);
> +    /* Mask */
> +    mask = (1ULL << (e->register_bit_width - 1) << 1) - 1;
> +    build_append_int_noprefix(e->table_data, mask, 8);
> +}
> +
> +/* ACPI 4.0: 17.4.1 Serialization Action Table */
> +void build_erst(GArray *table_data, BIOSLinker *linker, Object *erst_dev,
> +    const char *oem_id, const char *oem_table_id)
> +{
> +    /*
> +     * Serialization Action Table
> +     * The serialization action table must be generated first
> +     * so that its size can be known in order to populate the
> +     * Instruction Entry Count field.
> +     */
> +    GArray *table_instruction_data = g_array_new(FALSE, FALSE, sizeof(char));
> +    pcibus_t bar0 = pci_get_bar_addr(PCI_DEVICE(erst_dev), 0);
> +    AcpiTable table = { .sig = "ERST", .rev = 1, .oem_id = oem_id,
> +                        .oem_table_id = oem_table_id };
> +    /* Contexts for the different ways ACTION and VALUE are accessed */
> +    BuildSerializationInstructionEntry rd_value_32_val = {
> +        .table_data = table_instruction_data,
> +        .bar = bar0,
> +        .instruction = INST_READ_REGISTER_VALUE,
> +        .flags = 0,
> +        .register_bit_width = 32,
> +        .register_offset = ERST_VALUE_OFFSET,
> +    };
> +    BuildSerializationInstructionEntry rd_value_32 = {
> +        .table_data = table_instruction_data,
> +        .bar = bar0,
> +        .instruction = INST_READ_REGISTER,
> +        .flags = 0,
> +        .register_bit_width = 32,
> +        .register_offset = ERST_VALUE_OFFSET,
> +    };
> +    BuildSerializationInstructionEntry rd_value_64 = {
> +        .table_data = table_instruction_data,
> +        .bar = bar0,
> +        .instruction = INST_READ_REGISTER,
> +        .flags = 0,
> +        .register_bit_width = 64,
> +        .register_offset = ERST_VALUE_OFFSET,
> +    };
> +    BuildSerializationInstructionEntry wr_value_32_val = {
> +        .table_data = table_instruction_data,
> +        .bar = bar0,
> +        .instruction = INST_WRITE_REGISTER_VALUE,
> +        .flags = 0,
> +        .register_bit_width = 32,
> +        .register_offset = ERST_VALUE_OFFSET,
> +    };
> +    BuildSerializationInstructionEntry wr_value_32 = {
> +        .table_data = table_instruction_data,
> +        .bar = bar0,
> +        .instruction = INST_WRITE_REGISTER,
> +        .flags = 0,
> +        .register_bit_width = 32,
> +        .register_offset = ERST_VALUE_OFFSET,
> +    };
> +    BuildSerializationInstructionEntry wr_value_64 = {
> +        .table_data = table_instruction_data,
> +        .bar = bar0,
> +        .instruction = INST_WRITE_REGISTER,
> +        .flags = 0,
> +        .register_bit_width = 64,
> +        .register_offset = ERST_VALUE_OFFSET,
> +    };
> +    BuildSerializationInstructionEntry wr_action = {
> +        .table_data = table_instruction_data,
> +        .bar = bar0,
> +        .instruction = INST_WRITE_REGISTER_VALUE,
> +        .flags = 0,
> +        .register_bit_width = 32,
> +        .register_offset = ERST_ACTION_OFFSET,
> +    };

We can probably write a macro to simply generating these structs. I see
.bar and .flags are the same in all structs. The .bit_width can be a param
into the macro etc.

> +    unsigned action;
> +
> +    trace_acpi_erst_pci_bar_0(bar0);
> +
> +    /* Serialization Instruction Entries */
> +    action = ACTION_BEGIN_WRITE_OPERATION;
> +    build_serialization_instruction(&wr_action, action, action);
> +
> +    action = ACTION_BEGIN_READ_OPERATION;
> +    build_serialization_instruction(&wr_action, action, action);
> +
> +    action = ACTION_BEGIN_CLEAR_OPERATION;
> +    build_serialization_instruction(&wr_action, action, action);
> +
> +    action = ACTION_END_OPERATION;
> +    build_serialization_instruction(&wr_action, action, action);
> +
> +    action = ACTION_SET_RECORD_OFFSET;
> +    build_serialization_instruction(&wr_value_32, action, 0);
> +    build_serialization_instruction(&wr_action, action, action);
> +
> +    action = ACTION_EXECUTE_OPERATION;
> +    build_serialization_instruction(&wr_value_32_val, action,
> +        ERST_EXECUTE_OPERATION_MAGIC);
> +    build_serialization_instruction(&wr_action, action, action);
> +
> +    action = ACTION_CHECK_BUSY_STATUS;
> +    build_serialization_instruction(&wr_action, action, action);
> +    build_serialization_instruction(&rd_value_32_val, action, 0x01);
> +
> +    action = ACTION_GET_COMMAND_STATUS;
> +    build_serialization_instruction(&wr_action, action, action);
> +    build_serialization_instruction(&rd_value_32, action, 0);
> +
> +    action = ACTION_GET_RECORD_IDENTIFIER;
> +    build_serialization_instruction(&wr_action, action, action);
> +    build_serialization_instruction(&rd_value_64, action, 0);
> +
> +    action = ACTION_SET_RECORD_IDENTIFIER;
> +    build_serialization_instruction(&wr_value_64, action, 0);
> +    build_serialization_instruction(&wr_action, action, action);
> +
> +    action = ACTION_GET_RECORD_COUNT;
> +    build_serialization_instruction(&wr_action, action, action);
> +    build_serialization_instruction(&rd_value_32, action, 0);
> +
> +    action = ACTION_BEGIN_DUMMY_WRITE_OPERATION;
> +    build_serialization_instruction(&wr_action, action, action);
> +
> +    action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE;
> +    build_serialization_instruction(&wr_action, action, action);
> +    build_serialization_instruction(&rd_value_64, action, 0);
> +
> +    action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH;
> +    build_serialization_instruction(&wr_action, action, action);
> +    build_serialization_instruction(&rd_value_64, action, 0);
> +
> +    action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES;
> +    build_serialization_instruction(&wr_action, action, action);
> +    build_serialization_instruction(&rd_value_32, action, 0);
> +
> +    action = ACTION_GET_EXECUTE_OPERATION_TIMINGS;
> +    build_serialization_instruction(&wr_action, action, action);
> +    build_serialization_instruction(&rd_value_64, action, 0);
> +
> +    /* Serialization Header */
> +    acpi_table_begin(&table, table_data);
> +
> +    /* Serialization Header Size */
> +    build_append_int_noprefix(table_data, 48, 4);
> +
> +    /* Reserved */
> +    build_append_int_noprefix(table_data,  0, 4);
> +
> +    /*
> +     * Instruction Entry Count
> +     * Each instruction entry is 32 bytes
> +     */
> +    g_assert((table_instruction_data->len) % 32 == 0);
> +    build_append_int_noprefix(table_data,
> +        (table_instruction_data->len / 32), 4);
> +
> +    /* Serialization Instruction Entries */
> +    g_array_append_vals(table_data, table_instruction_data->data,
> +        table_instruction_data->len);
> +    g_array_free(table_instruction_data, TRUE);
> +
> +    acpi_table_end(linker, &table);
> +}
> +
> +/*******************************************************************/
> +/*******************************************************************/
>  static uint8_t *get_nvram_ptr_by_index(ERSTDeviceState *s, unsigned index)
>  {
>      uint8_t *rc = NULL;
> --
> 1.8.3.1
>
>
Re: [PATCH v14 06/10] ACPI ERST: build the ACPI ERST table
Posted by Eric DeVolder 4 years ago 
Ani,
Thanks for the RB! Inline responses below.
eric

On 1/27/22 02:36, Ani Sinha wrote:
> 
> 
> On Wed, 26 Jan 2022, Eric DeVolder wrote:
> 
>> This builds the ACPI ERST table to inform OSPM how to communicate
>> with the acpi-erst device.
> 
> There might be more optimizations possible but I think we have messaged
> this code enough. We can further rework the code if needed in subsequent
> patches once this is pushed.
> 
>>
>> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> 
> with some minor comments,
> 
> Reviewed-by: Ani Sinha <ani@anisinha.ca>
> 
>>   hw/acpi/erst.c | 225 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 225 insertions(+)
>>
>> diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
>> index fe9ba51..5d5a639 100644
>> --- a/hw/acpi/erst.c
>> +++ b/hw/acpi/erst.c
>> @@ -59,6 +59,27 @@
>>   #define STATUS_RECORD_STORE_EMPTY     0x04
>>   #define STATUS_RECORD_NOT_FOUND       0x05
>>
>> +/* ACPI 4.0: Table 17-19 Serialization Instructions */
>> +#define INST_READ_REGISTER                 0x00
>> +#define INST_READ_REGISTER_VALUE           0x01
>> +#define INST_WRITE_REGISTER                0x02
>> +#define INST_WRITE_REGISTER_VALUE          0x03
>> +#define INST_NOOP                          0x04
>> +#define INST_LOAD_VAR1                     0x05
>> +#define INST_LOAD_VAR2                     0x06
>> +#define INST_STORE_VAR1                    0x07
>> +#define INST_ADD                           0x08
>> +#define INST_SUBTRACT                      0x09
>> +#define INST_ADD_VALUE                     0x0A
>> +#define INST_SUBTRACT_VALUE                0x0B
>> +#define INST_STALL                         0x0C
>> +#define INST_STALL_WHILE_TRUE              0x0D
>> +#define INST_SKIP_NEXT_INSTRUCTION_IF_TRUE 0x0E
>> +#define INST_GOTO                          0x0F
>> +#define INST_SET_SRC_ADDRESS_BASE          0x10
>> +#define INST_SET_DST_ADDRESS_BASE          0x11
>> +#define INST_MOVE_DATA                     0x12
>> +
>>   /* UEFI 2.1: Appendix N Common Platform Error Record */
>>   #define UEFI_CPER_RECORD_MIN_SIZE 128U
>>   #define UEFI_CPER_RECORD_LENGTH_OFFSET 20U
>> @@ -172,6 +193,210 @@ typedef struct {
>>
>>   /*******************************************************************/
>>   /*******************************************************************/
>> +typedef struct {
>> +    GArray *table_data;
>> +    pcibus_t bar;
>> +    uint8_t instruction;
>> +    uint8_t flags;
>> +    uint8_t register_bit_width;
>> +    pcibus_t register_offset;
>> +} BuildSerializationInstructionEntry;
>> +
>> +/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */
>> +static void build_serialization_instruction(
>> +    BuildSerializationInstructionEntry *e,
>> +    uint8_t serialization_action,
>> +    uint64_t value)
>> +{
>> +    /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */
>> +    struct AcpiGenericAddress gas;
>> +    uint64_t mask;
>> +
>> +    /* Serialization Action */
>> +    build_append_int_noprefix(e->table_data, serialization_action, 1);
>> +    /* Instruction */
>> +    build_append_int_noprefix(e->table_data, e->instruction, 1);
>> +    /* Flags */
>> +    build_append_int_noprefix(e->table_data, e->flags, 1);
>> +    /* Reserved */
>> +    build_append_int_noprefix(e->table_data, 0, 1);
>> +    /* Register Region */
>> +    gas.space_id = AML_SYSTEM_MEMORY;
>> +    gas.bit_width = e->register_bit_width;
>> +    gas.bit_offset = 0;
>> +    gas.access_width = ctz32(e->register_bit_width) - 2;
> 
> Should this be casted as unit8_t?
OK, done.

> 
>> +    gas.address = (uint64_t)(e->bar + e->register_offset);
>> +    build_append_gas_from_struct(e->table_data, &gas);
>> +    /* Value */
>> +    build_append_int_noprefix(e->table_data, value, 8);
>> +    /* Mask */
>> +    mask = (1ULL << (e->register_bit_width - 1) << 1) - 1;
>> +    build_append_int_noprefix(e->table_data, mask, 8);
>> +}
>> +
>> +/* ACPI 4.0: 17.4.1 Serialization Action Table */
>> +void build_erst(GArray *table_data, BIOSLinker *linker, Object *erst_dev,
>> +    const char *oem_id, const char *oem_table_id)
>> +{
>> +    /*
>> +     * Serialization Action Table
>> +     * The serialization action table must be generated first
>> +     * so that its size can be known in order to populate the
>> +     * Instruction Entry Count field.
>> +     */
>> +    GArray *table_instruction_data = g_array_new(FALSE, FALSE, sizeof(char));
>> +    pcibus_t bar0 = pci_get_bar_addr(PCI_DEVICE(erst_dev), 0);
>> +    AcpiTable table = { .sig = "ERST", .rev = 1, .oem_id = oem_id,
>> +                        .oem_table_id = oem_table_id };
>> +    /* Contexts for the different ways ACTION and VALUE are accessed */
>> +    BuildSerializationInstructionEntry rd_value_32_val = {
>> +        .table_data = table_instruction_data,
>> +        .bar = bar0,
>> +        .instruction = INST_READ_REGISTER_VALUE,
>> +        .flags = 0,
>> +        .register_bit_width = 32,
>> +        .register_offset = ERST_VALUE_OFFSET,
>> +    };
>> +    BuildSerializationInstructionEntry rd_value_32 = {
>> +        .table_data = table_instruction_data,
>> +        .bar = bar0,
>> +        .instruction = INST_READ_REGISTER,
>> +        .flags = 0,
>> +        .register_bit_width = 32,
>> +        .register_offset = ERST_VALUE_OFFSET,
>> +    };
>> +    BuildSerializationInstructionEntry rd_value_64 = {
>> +        .table_data = table_instruction_data,
>> +        .bar = bar0,
>> +        .instruction = INST_READ_REGISTER,
>> +        .flags = 0,
>> +        .register_bit_width = 64,
>> +        .register_offset = ERST_VALUE_OFFSET,
>> +    };
>> +    BuildSerializationInstructionEntry wr_value_32_val = {
>> +        .table_data = table_instruction_data,
>> +        .bar = bar0,
>> +        .instruction = INST_WRITE_REGISTER_VALUE,
>> +        .flags = 0,
>> +        .register_bit_width = 32,
>> +        .register_offset = ERST_VALUE_OFFSET,
>> +    };
>> +    BuildSerializationInstructionEntry wr_value_32 = {
>> +        .table_data = table_instruction_data,
>> +        .bar = bar0,
>> +        .instruction = INST_WRITE_REGISTER,
>> +        .flags = 0,
>> +        .register_bit_width = 32,
>> +        .register_offset = ERST_VALUE_OFFSET,
>> +    };
>> +    BuildSerializationInstructionEntry wr_value_64 = {
>> +        .table_data = table_instruction_data,
>> +        .bar = bar0,
>> +        .instruction = INST_WRITE_REGISTER,
>> +        .flags = 0,
>> +        .register_bit_width = 64,
>> +        .register_offset = ERST_VALUE_OFFSET,
>> +    };
>> +    BuildSerializationInstructionEntry wr_action = {
>> +        .table_data = table_instruction_data,
>> +        .bar = bar0,
>> +        .instruction = INST_WRITE_REGISTER_VALUE,
>> +        .flags = 0,
>> +        .register_bit_width = 32,
>> +        .register_offset = ERST_ACTION_OFFSET,
>> +    };
> 
> We can probably write a macro to simply generating these structs. I see
> .bar and .flags are the same in all structs. The .bit_width can be a param
> into the macro etc.
Agree, however the request was to NOT hide the use of local variables in
macros, so while .flags is always 0, .instruction, .register_bit_width and .register_offset
would be parameters, that leaves .table_data and .bar which just need the local
variables. Is the following acceptable (which hides the use of the local variables)?

#define SERIALIZATIONINSTRUCTIONCTX(name, \
     inst, bit_width, offset) \
     BuildSerializationInstructionEntry name = { \
         .table_data = table_instruction_data, \
         .bar = bar0, \
         .instruction = inst, \
         .flags = 0, \
         .register_bit_width = bit_width, \
         .register_offset = offset, \
     }
     SERIALIZATIONINSTRUCTIONCTX(rd_value_32_val,
         INST_READ_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
     SERIALIZATIONINSTRUCTIONCTX(rd_value_32,
         INST_READ_REGISTER, 32, ERST_VALUE_OFFSET);
     SERIALIZATIONINSTRUCTIONCTX(rd_value_64,
         INST_READ_REGISTER, 64, ERST_VALUE_OFFSET);
     SERIALIZATIONINSTRUCTIONCTX(wr_value_32_val,
         INST_WRITE_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
     SERIALIZATIONINSTRUCTIONCTX(wr_value_32,
         INST_WRITE_REGISTER, 32, ERST_VALUE_OFFSET);
     SERIALIZATIONINSTRUCTIONCTX(wr_value_64,
         INST_WRITE_REGISTER, 64, ERST_VALUE_OFFSET);
     SERIALIZATIONINSTRUCTIONCTX(wr_action,
         INST_WRITE_REGISTER_VALUE, 32, ERST_ACTION_OFFSET);

These are the 7 accessors needed.

> 
>> +    unsigned action;
>> +
>> +    trace_acpi_erst_pci_bar_0(bar0);
>> +
>> +    /* Serialization Instruction Entries */
>> +    action = ACTION_BEGIN_WRITE_OPERATION;
>> +    build_serialization_instruction(&wr_action, action, action);
>> +
>> +    action = ACTION_BEGIN_READ_OPERATION;
>> +    build_serialization_instruction(&wr_action, action, action);
>> +
>> +    action = ACTION_BEGIN_CLEAR_OPERATION;
>> +    build_serialization_instruction(&wr_action, action, action);
>> +
>> +    action = ACTION_END_OPERATION;
>> +    build_serialization_instruction(&wr_action, action, action);
>> +
>> +    action = ACTION_SET_RECORD_OFFSET;
>> +    build_serialization_instruction(&wr_value_32, action, 0);
>> +    build_serialization_instruction(&wr_action, action, action);
>> +
>> +    action = ACTION_EXECUTE_OPERATION;
>> +    build_serialization_instruction(&wr_value_32_val, action,
>> +        ERST_EXECUTE_OPERATION_MAGIC);
>> +    build_serialization_instruction(&wr_action, action, action);
>> +
>> +    action = ACTION_CHECK_BUSY_STATUS;
>> +    build_serialization_instruction(&wr_action, action, action);
>> +    build_serialization_instruction(&rd_value_32_val, action, 0x01);
>> +
>> +    action = ACTION_GET_COMMAND_STATUS;
>> +    build_serialization_instruction(&wr_action, action, action);
>> +    build_serialization_instruction(&rd_value_32, action, 0);
>> +
>> +    action = ACTION_GET_RECORD_IDENTIFIER;
>> +    build_serialization_instruction(&wr_action, action, action);
>> +    build_serialization_instruction(&rd_value_64, action, 0);
>> +
>> +    action = ACTION_SET_RECORD_IDENTIFIER;
>> +    build_serialization_instruction(&wr_value_64, action, 0);
>> +    build_serialization_instruction(&wr_action, action, action);
>> +
>> +    action = ACTION_GET_RECORD_COUNT;
>> +    build_serialization_instruction(&wr_action, action, action);
>> +    build_serialization_instruction(&rd_value_32, action, 0);
>> +
>> +    action = ACTION_BEGIN_DUMMY_WRITE_OPERATION;
>> +    build_serialization_instruction(&wr_action, action, action);
>> +
>> +    action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE;
>> +    build_serialization_instruction(&wr_action, action, action);
>> +    build_serialization_instruction(&rd_value_64, action, 0);
>> +
>> +    action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH;
>> +    build_serialization_instruction(&wr_action, action, action);
>> +    build_serialization_instruction(&rd_value_64, action, 0);
>> +
>> +    action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES;
>> +    build_serialization_instruction(&wr_action, action, action);
>> +    build_serialization_instruction(&rd_value_32, action, 0);
>> +
>> +    action = ACTION_GET_EXECUTE_OPERATION_TIMINGS;
>> +    build_serialization_instruction(&wr_action, action, action);
>> +    build_serialization_instruction(&rd_value_64, action, 0);
>> +
>> +    /* Serialization Header */
>> +    acpi_table_begin(&table, table_data);
>> +
>> +    /* Serialization Header Size */
>> +    build_append_int_noprefix(table_data, 48, 4);
>> +
>> +    /* Reserved */
>> +    build_append_int_noprefix(table_data,  0, 4);
>> +
>> +    /*
>> +     * Instruction Entry Count
>> +     * Each instruction entry is 32 bytes
>> +     */
>> +    g_assert((table_instruction_data->len) % 32 == 0);
>> +    build_append_int_noprefix(table_data,
>> +        (table_instruction_data->len / 32), 4);
>> +
>> +    /* Serialization Instruction Entries */
>> +    g_array_append_vals(table_data, table_instruction_data->data,
>> +        table_instruction_data->len);
>> +    g_array_free(table_instruction_data, TRUE);
>> +
>> +    acpi_table_end(linker, &table);
>> +}
>> +
>> +/*******************************************************************/
>> +/*******************************************************************/
>>   static uint8_t *get_nvram_ptr_by_index(ERSTDeviceState *s, unsigned index)
>>   {
>>       uint8_t *rc = NULL;
>> --
>> 1.8.3.1
>>
>>
Re: [PATCH v14 06/10] ACPI ERST: build the ACPI ERST table
Posted by Michael S. Tsirkin 4 years ago 
On Thu, Jan 27, 2022 at 04:02:07PM -0600, Eric DeVolder wrote:
> Ani,
> Thanks for the RB! Inline responses below.
> eric
> 
> On 1/27/22 02:36, Ani Sinha wrote:
> > 
> > 
> > On Wed, 26 Jan 2022, Eric DeVolder wrote:
> > 
> > > This builds the ACPI ERST table to inform OSPM how to communicate
> > > with the acpi-erst device.
> > 
> > There might be more optimizations possible but I think we have messaged
> > this code enough. We can further rework the code if needed in subsequent
> > patches once this is pushed.
> > 
> > > 
> > > Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> > 
> > with some minor comments,
> > 
> > Reviewed-by: Ani Sinha <ani@anisinha.ca>
> > 
> > >   hw/acpi/erst.c | 225 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >   1 file changed, 225 insertions(+)
> > > 
> > > diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
> > > index fe9ba51..5d5a639 100644
> > > --- a/hw/acpi/erst.c
> > > +++ b/hw/acpi/erst.c
> > > @@ -59,6 +59,27 @@
> > >   #define STATUS_RECORD_STORE_EMPTY     0x04
> > >   #define STATUS_RECORD_NOT_FOUND       0x05
> > > 
> > > +/* ACPI 4.0: Table 17-19 Serialization Instructions */
> > > +#define INST_READ_REGISTER                 0x00
> > > +#define INST_READ_REGISTER_VALUE           0x01
> > > +#define INST_WRITE_REGISTER                0x02
> > > +#define INST_WRITE_REGISTER_VALUE          0x03
> > > +#define INST_NOOP                          0x04
> > > +#define INST_LOAD_VAR1                     0x05
> > > +#define INST_LOAD_VAR2                     0x06
> > > +#define INST_STORE_VAR1                    0x07
> > > +#define INST_ADD                           0x08
> > > +#define INST_SUBTRACT                      0x09
> > > +#define INST_ADD_VALUE                     0x0A
> > > +#define INST_SUBTRACT_VALUE                0x0B
> > > +#define INST_STALL                         0x0C
> > > +#define INST_STALL_WHILE_TRUE              0x0D
> > > +#define INST_SKIP_NEXT_INSTRUCTION_IF_TRUE 0x0E
> > > +#define INST_GOTO                          0x0F
> > > +#define INST_SET_SRC_ADDRESS_BASE          0x10
> > > +#define INST_SET_DST_ADDRESS_BASE          0x11
> > > +#define INST_MOVE_DATA                     0x12
> > > +
> > >   /* UEFI 2.1: Appendix N Common Platform Error Record */
> > >   #define UEFI_CPER_RECORD_MIN_SIZE 128U
> > >   #define UEFI_CPER_RECORD_LENGTH_OFFSET 20U
> > > @@ -172,6 +193,210 @@ typedef struct {
> > > 
> > >   /*******************************************************************/
> > >   /*******************************************************************/
> > > +typedef struct {
> > > +    GArray *table_data;
> > > +    pcibus_t bar;
> > > +    uint8_t instruction;
> > > +    uint8_t flags;
> > > +    uint8_t register_bit_width;
> > > +    pcibus_t register_offset;
> > > +} BuildSerializationInstructionEntry;
> > > +
> > > +/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */
> > > +static void build_serialization_instruction(
> > > +    BuildSerializationInstructionEntry *e,
> > > +    uint8_t serialization_action,
> > > +    uint64_t value)
> > > +{
> > > +    /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */
> > > +    struct AcpiGenericAddress gas;
> > > +    uint64_t mask;
> > > +
> > > +    /* Serialization Action */
> > > +    build_append_int_noprefix(e->table_data, serialization_action, 1);
> > > +    /* Instruction */
> > > +    build_append_int_noprefix(e->table_data, e->instruction, 1);
> > > +    /* Flags */
> > > +    build_append_int_noprefix(e->table_data, e->flags, 1);
> > > +    /* Reserved */
> > > +    build_append_int_noprefix(e->table_data, 0, 1);
> > > +    /* Register Region */
> > > +    gas.space_id = AML_SYSTEM_MEMORY;
> > > +    gas.bit_width = e->register_bit_width;
> > > +    gas.bit_offset = 0;
> > > +    gas.access_width = ctz32(e->register_bit_width) - 2;
> > 
> > Should this be casted as unit8_t?
> OK, done.
> 
> > 
> > > +    gas.address = (uint64_t)(e->bar + e->register_offset);
> > > +    build_append_gas_from_struct(e->table_data, &gas);
> > > +    /* Value */
> > > +    build_append_int_noprefix(e->table_data, value, 8);
> > > +    /* Mask */
> > > +    mask = (1ULL << (e->register_bit_width - 1) << 1) - 1;
> > > +    build_append_int_noprefix(e->table_data, mask, 8);
> > > +}
> > > +
> > > +/* ACPI 4.0: 17.4.1 Serialization Action Table */
> > > +void build_erst(GArray *table_data, BIOSLinker *linker, Object *erst_dev,
> > > +    const char *oem_id, const char *oem_table_id)
> > > +{
> > > +    /*
> > > +     * Serialization Action Table
> > > +     * The serialization action table must be generated first
> > > +     * so that its size can be known in order to populate the
> > > +     * Instruction Entry Count field.
> > > +     */
> > > +    GArray *table_instruction_data = g_array_new(FALSE, FALSE, sizeof(char));
> > > +    pcibus_t bar0 = pci_get_bar_addr(PCI_DEVICE(erst_dev), 0);
> > > +    AcpiTable table = { .sig = "ERST", .rev = 1, .oem_id = oem_id,
> > > +                        .oem_table_id = oem_table_id };
> > > +    /* Contexts for the different ways ACTION and VALUE are accessed */
> > > +    BuildSerializationInstructionEntry rd_value_32_val = {
> > > +        .table_data = table_instruction_data,
> > > +        .bar = bar0,
> > > +        .instruction = INST_READ_REGISTER_VALUE,
> > > +        .flags = 0,
> > > +        .register_bit_width = 32,
> > > +        .register_offset = ERST_VALUE_OFFSET,
> > > +    };
> > > +    BuildSerializationInstructionEntry rd_value_32 = {
> > > +        .table_data = table_instruction_data,
> > > +        .bar = bar0,
> > > +        .instruction = INST_READ_REGISTER,
> > > +        .flags = 0,
> > > +        .register_bit_width = 32,
> > > +        .register_offset = ERST_VALUE_OFFSET,
> > > +    };
> > > +    BuildSerializationInstructionEntry rd_value_64 = {
> > > +        .table_data = table_instruction_data,
> > > +        .bar = bar0,
> > > +        .instruction = INST_READ_REGISTER,
> > > +        .flags = 0,
> > > +        .register_bit_width = 64,
> > > +        .register_offset = ERST_VALUE_OFFSET,
> > > +    };
> > > +    BuildSerializationInstructionEntry wr_value_32_val = {
> > > +        .table_data = table_instruction_data,
> > > +        .bar = bar0,
> > > +        .instruction = INST_WRITE_REGISTER_VALUE,
> > > +        .flags = 0,
> > > +        .register_bit_width = 32,
> > > +        .register_offset = ERST_VALUE_OFFSET,
> > > +    };
> > > +    BuildSerializationInstructionEntry wr_value_32 = {
> > > +        .table_data = table_instruction_data,
> > > +        .bar = bar0,
> > > +        .instruction = INST_WRITE_REGISTER,
> > > +        .flags = 0,
> > > +        .register_bit_width = 32,
> > > +        .register_offset = ERST_VALUE_OFFSET,
> > > +    };
> > > +    BuildSerializationInstructionEntry wr_value_64 = {
> > > +        .table_data = table_instruction_data,
> > > +        .bar = bar0,
> > > +        .instruction = INST_WRITE_REGISTER,
> > > +        .flags = 0,
> > > +        .register_bit_width = 64,
> > > +        .register_offset = ERST_VALUE_OFFSET,
> > > +    };
> > > +    BuildSerializationInstructionEntry wr_action = {
> > > +        .table_data = table_instruction_data,
> > > +        .bar = bar0,
> > > +        .instruction = INST_WRITE_REGISTER_VALUE,
> > > +        .flags = 0,
> > > +        .register_bit_width = 32,
> > > +        .register_offset = ERST_ACTION_OFFSET,
> > > +    };
> > 
> > We can probably write a macro to simply generating these structs. I see
> > .bar and .flags are the same in all structs. The .bit_width can be a param
> > into the macro etc.
> Agree, however the request was to NOT hide the use of local variables in
> macros, so while .flags is always 0, .instruction, .register_bit_width and .register_offset
> would be parameters, that leaves .table_data and .bar which just need the local
> variables. Is the following acceptable (which hides the use of the local variables)?

You can just use assignment:

   BuildSerializationInstructionEntry entry = {
       .table_data = table_instruction_data,
       .bar = bar0,
       .flags = 0,
       .register_bit_width = 32,
   };
   BuildSerializationInstructionEntry rd_value_32_val = entry;
   rd_value_32_val.action = INST_READ_REGISTER_VALUE;
   rd_value_32_val.register_offset = ERST_ACTION_OFFSET;

and so on.
not sure it's worth it, it's just a couple of lines.



> #define SERIALIZATIONINSTRUCTIONCTX(name, \
>     inst, bit_width, offset) \
>     BuildSerializationInstructionEntry name = { \
>         .table_data = table_instruction_data, \
>         .bar = bar0, \
>         .instruction = inst, \
>         .flags = 0, \
>         .register_bit_width = bit_width, \
>         .register_offset = offset, \
>     }
>     SERIALIZATIONINSTRUCTIONCTX(rd_value_32_val,
>         INST_READ_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
>     SERIALIZATIONINSTRUCTIONCTX(rd_value_32,
>         INST_READ_REGISTER, 32, ERST_VALUE_OFFSET);
>     SERIALIZATIONINSTRUCTIONCTX(rd_value_64,
>         INST_READ_REGISTER, 64, ERST_VALUE_OFFSET);
>     SERIALIZATIONINSTRUCTIONCTX(wr_value_32_val,
>         INST_WRITE_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
>     SERIALIZATIONINSTRUCTIONCTX(wr_value_32,
>         INST_WRITE_REGISTER, 32, ERST_VALUE_OFFSET);
>     SERIALIZATIONINSTRUCTIONCTX(wr_value_64,
>         INST_WRITE_REGISTER, 64, ERST_VALUE_OFFSET);
>     SERIALIZATIONINSTRUCTIONCTX(wr_action,
>         INST_WRITE_REGISTER_VALUE, 32, ERST_ACTION_OFFSET);
> 
> These are the 7 accessors needed.

not at all sure this one is worth the macro mess.

> > 
> > > +    unsigned action;
> > > +
> > > +    trace_acpi_erst_pci_bar_0(bar0);
> > > +
> > > +    /* Serialization Instruction Entries */
> > > +    action = ACTION_BEGIN_WRITE_OPERATION;
> > > +    build_serialization_instruction(&wr_action, action, action);
> > > +
> > > +    action = ACTION_BEGIN_READ_OPERATION;
> > > +    build_serialization_instruction(&wr_action, action, action);
> > > +
> > > +    action = ACTION_BEGIN_CLEAR_OPERATION;
> > > +    build_serialization_instruction(&wr_action, action, action);
> > > +
> > > +    action = ACTION_END_OPERATION;
> > > +    build_serialization_instruction(&wr_action, action, action);
> > > +
> > > +    action = ACTION_SET_RECORD_OFFSET;
> > > +    build_serialization_instruction(&wr_value_32, action, 0);
> > > +    build_serialization_instruction(&wr_action, action, action);
> > > +
> > > +    action = ACTION_EXECUTE_OPERATION;
> > > +    build_serialization_instruction(&wr_value_32_val, action,
> > > +        ERST_EXECUTE_OPERATION_MAGIC);
> > > +    build_serialization_instruction(&wr_action, action, action);
> > > +
> > > +    action = ACTION_CHECK_BUSY_STATUS;
> > > +    build_serialization_instruction(&wr_action, action, action);
> > > +    build_serialization_instruction(&rd_value_32_val, action, 0x01);
> > > +
> > > +    action = ACTION_GET_COMMAND_STATUS;
> > > +    build_serialization_instruction(&wr_action, action, action);
> > > +    build_serialization_instruction(&rd_value_32, action, 0);
> > > +
> > > +    action = ACTION_GET_RECORD_IDENTIFIER;
> > > +    build_serialization_instruction(&wr_action, action, action);
> > > +    build_serialization_instruction(&rd_value_64, action, 0);
> > > +
> > > +    action = ACTION_SET_RECORD_IDENTIFIER;
> > > +    build_serialization_instruction(&wr_value_64, action, 0);
> > > +    build_serialization_instruction(&wr_action, action, action);
> > > +
> > > +    action = ACTION_GET_RECORD_COUNT;
> > > +    build_serialization_instruction(&wr_action, action, action);
> > > +    build_serialization_instruction(&rd_value_32, action, 0);
> > > +
> > > +    action = ACTION_BEGIN_DUMMY_WRITE_OPERATION;
> > > +    build_serialization_instruction(&wr_action, action, action);
> > > +
> > > +    action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE;
> > > +    build_serialization_instruction(&wr_action, action, action);
> > > +    build_serialization_instruction(&rd_value_64, action, 0);
> > > +
> > > +    action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH;
> > > +    build_serialization_instruction(&wr_action, action, action);
> > > +    build_serialization_instruction(&rd_value_64, action, 0);
> > > +
> > > +    action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES;
> > > +    build_serialization_instruction(&wr_action, action, action);
> > > +    build_serialization_instruction(&rd_value_32, action, 0);
> > > +
> > > +    action = ACTION_GET_EXECUTE_OPERATION_TIMINGS;
> > > +    build_serialization_instruction(&wr_action, action, action);
> > > +    build_serialization_instruction(&rd_value_64, action, 0);
> > > +
> > > +    /* Serialization Header */
> > > +    acpi_table_begin(&table, table_data);
> > > +
> > > +    /* Serialization Header Size */
> > > +    build_append_int_noprefix(table_data, 48, 4);
> > > +
> > > +    /* Reserved */
> > > +    build_append_int_noprefix(table_data,  0, 4);
> > > +
> > > +    /*
> > > +     * Instruction Entry Count
> > > +     * Each instruction entry is 32 bytes
> > > +     */
> > > +    g_assert((table_instruction_data->len) % 32 == 0);
> > > +    build_append_int_noprefix(table_data,
> > > +        (table_instruction_data->len / 32), 4);
> > > +
> > > +    /* Serialization Instruction Entries */
> > > +    g_array_append_vals(table_data, table_instruction_data->data,
> > > +        table_instruction_data->len);
> > > +    g_array_free(table_instruction_data, TRUE);
> > > +
> > > +    acpi_table_end(linker, &table);
> > > +}
> > > +
> > > +/*******************************************************************/
> > > +/*******************************************************************/
> > >   static uint8_t *get_nvram_ptr_by_index(ERSTDeviceState *s, unsigned index)
> > >   {
> > >       uint8_t *rc = NULL;
> > > --
> > > 1.8.3.1
> > > 
> > >
Re: [PATCH v14 06/10] ACPI ERST: build the ACPI ERST table
Posted by Ani Sinha 4 years ago 
> > #define SERIALIZATIONINSTRUCTIONCTX(name, \
> >     inst, bit_width, offset) \
> >     BuildSerializationInstructionEntry name = { \
> >         .table_data = table_instruction_data, \
> >         .bar = bar0, \
> >         .instruction = inst, \
> >         .flags = 0, \
> >         .register_bit_width = bit_width, \
> >         .register_offset = offset, \
> >     }
> >     SERIALIZATIONINSTRUCTIONCTX(rd_value_32_val,
> >         INST_READ_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
> >     SERIALIZATIONINSTRUCTIONCTX(rd_value_32,
> >         INST_READ_REGISTER, 32, ERST_VALUE_OFFSET);
> >     SERIALIZATIONINSTRUCTIONCTX(rd_value_64,
> >         INST_READ_REGISTER, 64, ERST_VALUE_OFFSET);
> >     SERIALIZATIONINSTRUCTIONCTX(wr_value_32_val,
> >         INST_WRITE_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
> >     SERIALIZATIONINSTRUCTIONCTX(wr_value_32,
> >         INST_WRITE_REGISTER, 32, ERST_VALUE_OFFSET);
> >     SERIALIZATIONINSTRUCTIONCTX(wr_value_64,
> >         INST_WRITE_REGISTER, 64, ERST_VALUE_OFFSET);
> >     SERIALIZATIONINSTRUCTIONCTX(wr_action,
> >         INST_WRITE_REGISTER_VALUE, 32, ERST_ACTION_OFFSET);
> >
> > These are the 7 accessors needed.
>
> not at all sure this one is worth the macro mess.
>

I did not quite have this in my mind when I said macro but it's fine.
We can improve the code later.
Re: [PATCH v14 06/10] ACPI ERST: build the ACPI ERST table
Posted by Eric DeVolder 4 years ago 
Michael,
Thanks for examining this. Inline response below.
eric

On 1/27/22 18:37, Michael S. Tsirkin wrote:
> On Thu, Jan 27, 2022 at 04:02:07PM -0600, Eric DeVolder wrote:
>> Ani,
>> Thanks for the RB! Inline responses below.
>> eric
>>
>> On 1/27/22 02:36, Ani Sinha wrote:
>>>
>>>
>>> On Wed, 26 Jan 2022, Eric DeVolder wrote:
>>>
>>>> This builds the ACPI ERST table to inform OSPM how to communicate
>>>> with the acpi-erst device.
>>>
>>> There might be more optimizations possible but I think we have messaged
>>> this code enough. We can further rework the code if needed in subsequent
>>> patches once this is pushed.
>>>
>>>>
>>>> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
>>>
>>> with some minor comments,
>>>
>>> Reviewed-by: Ani Sinha <ani@anisinha.ca>
>>>
>>>>    hw/acpi/erst.c | 225 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 225 insertions(+)
>>>>
>>>> diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
>>>> index fe9ba51..5d5a639 100644
>>>> --- a/hw/acpi/erst.c
>>>> +++ b/hw/acpi/erst.c
>>>> @@ -59,6 +59,27 @@
>>>>    #define STATUS_RECORD_STORE_EMPTY     0x04
>>>>    #define STATUS_RECORD_NOT_FOUND       0x05
>>>>
>>>> +/* ACPI 4.0: Table 17-19 Serialization Instructions */
>>>> +#define INST_READ_REGISTER                 0x00
>>>> +#define INST_READ_REGISTER_VALUE           0x01
>>>> +#define INST_WRITE_REGISTER                0x02
>>>> +#define INST_WRITE_REGISTER_VALUE          0x03
>>>> +#define INST_NOOP                          0x04
>>>> +#define INST_LOAD_VAR1                     0x05
>>>> +#define INST_LOAD_VAR2                     0x06
>>>> +#define INST_STORE_VAR1                    0x07
>>>> +#define INST_ADD                           0x08
>>>> +#define INST_SUBTRACT                      0x09
>>>> +#define INST_ADD_VALUE                     0x0A
>>>> +#define INST_SUBTRACT_VALUE                0x0B
>>>> +#define INST_STALL                         0x0C
>>>> +#define INST_STALL_WHILE_TRUE              0x0D
>>>> +#define INST_SKIP_NEXT_INSTRUCTION_IF_TRUE 0x0E
>>>> +#define INST_GOTO                          0x0F
>>>> +#define INST_SET_SRC_ADDRESS_BASE          0x10
>>>> +#define INST_SET_DST_ADDRESS_BASE          0x11
>>>> +#define INST_MOVE_DATA                     0x12
>>>> +
>>>>    /* UEFI 2.1: Appendix N Common Platform Error Record */
>>>>    #define UEFI_CPER_RECORD_MIN_SIZE 128U
>>>>    #define UEFI_CPER_RECORD_LENGTH_OFFSET 20U
>>>> @@ -172,6 +193,210 @@ typedef struct {
>>>>
>>>>    /*******************************************************************/
>>>>    /*******************************************************************/
>>>> +typedef struct {
>>>> +    GArray *table_data;
>>>> +    pcibus_t bar;
>>>> +    uint8_t instruction;
>>>> +    uint8_t flags;
>>>> +    uint8_t register_bit_width;
>>>> +    pcibus_t register_offset;
>>>> +} BuildSerializationInstructionEntry;
>>>> +
>>>> +/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */
>>>> +static void build_serialization_instruction(
>>>> +    BuildSerializationInstructionEntry *e,
>>>> +    uint8_t serialization_action,
>>>> +    uint64_t value)
>>>> +{
>>>> +    /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */
>>>> +    struct AcpiGenericAddress gas;
>>>> +    uint64_t mask;
>>>> +
>>>> +    /* Serialization Action */
>>>> +    build_append_int_noprefix(e->table_data, serialization_action, 1);
>>>> +    /* Instruction */
>>>> +    build_append_int_noprefix(e->table_data, e->instruction, 1);
>>>> +    /* Flags */
>>>> +    build_append_int_noprefix(e->table_data, e->flags, 1);
>>>> +    /* Reserved */
>>>> +    build_append_int_noprefix(e->table_data, 0, 1);
>>>> +    /* Register Region */
>>>> +    gas.space_id = AML_SYSTEM_MEMORY;
>>>> +    gas.bit_width = e->register_bit_width;
>>>> +    gas.bit_offset = 0;
>>>> +    gas.access_width = ctz32(e->register_bit_width) - 2;
>>>
>>> Should this be casted as unit8_t?
>> OK, done.
>>
>>>
>>>> +    gas.address = (uint64_t)(e->bar + e->register_offset);
>>>> +    build_append_gas_from_struct(e->table_data, &gas);
>>>> +    /* Value */
>>>> +    build_append_int_noprefix(e->table_data, value, 8);
>>>> +    /* Mask */
>>>> +    mask = (1ULL << (e->register_bit_width - 1) << 1) - 1;
>>>> +    build_append_int_noprefix(e->table_data, mask, 8);
>>>> +}
>>>> +
>>>> +/* ACPI 4.0: 17.4.1 Serialization Action Table */
>>>> +void build_erst(GArray *table_data, BIOSLinker *linker, Object *erst_dev,
>>>> +    const char *oem_id, const char *oem_table_id)
>>>> +{
>>>> +    /*
>>>> +     * Serialization Action Table
>>>> +     * The serialization action table must be generated first
>>>> +     * so that its size can be known in order to populate the
>>>> +     * Instruction Entry Count field.
>>>> +     */
>>>> +    GArray *table_instruction_data = g_array_new(FALSE, FALSE, sizeof(char));
>>>> +    pcibus_t bar0 = pci_get_bar_addr(PCI_DEVICE(erst_dev), 0);
>>>> +    AcpiTable table = { .sig = "ERST", .rev = 1, .oem_id = oem_id,
>>>> +                        .oem_table_id = oem_table_id };
>>>> +    /* Contexts for the different ways ACTION and VALUE are accessed */
>>>> +    BuildSerializationInstructionEntry rd_value_32_val = {
>>>> +        .table_data = table_instruction_data,
>>>> +        .bar = bar0,
>>>> +        .instruction = INST_READ_REGISTER_VALUE,
>>>> +        .flags = 0,
>>>> +        .register_bit_width = 32,
>>>> +        .register_offset = ERST_VALUE_OFFSET,
>>>> +    };
>>>> +    BuildSerializationInstructionEntry rd_value_32 = {
>>>> +        .table_data = table_instruction_data,
>>>> +        .bar = bar0,
>>>> +        .instruction = INST_READ_REGISTER,
>>>> +        .flags = 0,
>>>> +        .register_bit_width = 32,
>>>> +        .register_offset = ERST_VALUE_OFFSET,
>>>> +    };
>>>> +    BuildSerializationInstructionEntry rd_value_64 = {
>>>> +        .table_data = table_instruction_data,
>>>> +        .bar = bar0,
>>>> +        .instruction = INST_READ_REGISTER,
>>>> +        .flags = 0,
>>>> +        .register_bit_width = 64,
>>>> +        .register_offset = ERST_VALUE_OFFSET,
>>>> +    };
>>>> +    BuildSerializationInstructionEntry wr_value_32_val = {
>>>> +        .table_data = table_instruction_data,
>>>> +        .bar = bar0,
>>>> +        .instruction = INST_WRITE_REGISTER_VALUE,
>>>> +        .flags = 0,
>>>> +        .register_bit_width = 32,
>>>> +        .register_offset = ERST_VALUE_OFFSET,
>>>> +    };
>>>> +    BuildSerializationInstructionEntry wr_value_32 = {
>>>> +        .table_data = table_instruction_data,
>>>> +        .bar = bar0,
>>>> +        .instruction = INST_WRITE_REGISTER,
>>>> +        .flags = 0,
>>>> +        .register_bit_width = 32,
>>>> +        .register_offset = ERST_VALUE_OFFSET,
>>>> +    };
>>>> +    BuildSerializationInstructionEntry wr_value_64 = {
>>>> +        .table_data = table_instruction_data,
>>>> +        .bar = bar0,
>>>> +        .instruction = INST_WRITE_REGISTER,
>>>> +        .flags = 0,
>>>> +        .register_bit_width = 64,
>>>> +        .register_offset = ERST_VALUE_OFFSET,
>>>> +    };
>>>> +    BuildSerializationInstructionEntry wr_action = {
>>>> +        .table_data = table_instruction_data,
>>>> +        .bar = bar0,
>>>> +        .instruction = INST_WRITE_REGISTER_VALUE,
>>>> +        .flags = 0,
>>>> +        .register_bit_width = 32,
>>>> +        .register_offset = ERST_ACTION_OFFSET,
>>>> +    };
>>>
>>> We can probably write a macro to simply generating these structs. I see
>>> .bar and .flags are the same in all structs. The .bit_width can be a param
>>> into the macro etc.
>> Agree, however the request was to NOT hide the use of local variables in
>> macros, so while .flags is always 0, .instruction, .register_bit_width and .register_offset
>> would be parameters, that leaves .table_data and .bar which just need the local
>> variables. Is the following acceptable (which hides the use of the local variables)?
> 
> You can just use assignment:
> 
>     BuildSerializationInstructionEntry entry = {
>         .table_data = table_instruction_data,
>         .bar = bar0,
>         .flags = 0,
>         .register_bit_width = 32,
>     };
>     BuildSerializationInstructionEntry rd_value_32_val = entry;
>     rd_value_32_val.action = INST_READ_REGISTER_VALUE;
>     rd_value_32_val.register_offset = ERST_ACTION_OFFSET;
> 
> and so on.
> not sure it's worth it, it's just a couple of lines.
> 

OK, here is the equivalent using struct assignment, is this what you were after?

     BuildSerializationInstructionEntry base = {
         .table_data = table_instruction_data,
         .bar = bar0,
         .instruction = INST_WRITE_REGISTER,
         .flags = 0,
         .register_bit_width = 32,
         .register_offset = ERST_VALUE_OFFSET,
     };
     BuildSerializationInstructionEntry rd_value_32_val = base;
     rd_value_32_val.instruction = INST_READ_REGISTER_VALUE;
     BuildSerializationInstructionEntry rd_value_32 = base;
     rd_value_32.instruction = INST_READ_REGISTER;
     BuildSerializationInstructionEntry rd_value_64 = base;
     rd_value_64.instruction = INST_READ_REGISTER;
     rd_value_64.register_bit_width = 64;
     BuildSerializationInstructionEntry wr_value_32_val = base;
     wr_value_32_val.instruction = INST_WRITE_REGISTER_VALUE;
     BuildSerializationInstructionEntry wr_value_32 = base;
     BuildSerializationInstructionEntry wr_value_64 = base;
     wr_value_64.register_bit_width = 64;
     BuildSerializationInstructionEntry wr_action = base;
     wr_action.instruction = INST_WRITE_REGISTER_VALUE;
     wr_action.register_offset = ERST_ACTION_OFFSET;



> 
> 
>> #define SERIALIZATIONINSTRUCTIONCTX(name, \
>>      inst, bit_width, offset) \
>>      BuildSerializationInstructionEntry name = { \
>>          .table_data = table_instruction_data, \
>>          .bar = bar0, \
>>          .instruction = inst, \
>>          .flags = 0, \
>>          .register_bit_width = bit_width, \
>>          .register_offset = offset, \
>>      }
>>      SERIALIZATIONINSTRUCTIONCTX(rd_value_32_val,
>>          INST_READ_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
>>      SERIALIZATIONINSTRUCTIONCTX(rd_value_32,
>>          INST_READ_REGISTER, 32, ERST_VALUE_OFFSET);
>>      SERIALIZATIONINSTRUCTIONCTX(rd_value_64,
>>          INST_READ_REGISTER, 64, ERST_VALUE_OFFSET);
>>      SERIALIZATIONINSTRUCTIONCTX(wr_value_32_val,
>>          INST_WRITE_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
>>      SERIALIZATIONINSTRUCTIONCTX(wr_value_32,
>>          INST_WRITE_REGISTER, 32, ERST_VALUE_OFFSET);
>>      SERIALIZATIONINSTRUCTIONCTX(wr_value_64,
>>          INST_WRITE_REGISTER, 64, ERST_VALUE_OFFSET);
>>      SERIALIZATIONINSTRUCTIONCTX(wr_action,
>>          INST_WRITE_REGISTER_VALUE, 32, ERST_ACTION_OFFSET);
>>
>> These are the 7 accessors needed.
> 
> not at all sure this one is worth the macro mess.

I'm hoping to produce a v15 with the style you want.
eric

> 
>>>
>>>> +    unsigned action;
>>>> +
>>>> +    trace_acpi_erst_pci_bar_0(bar0);
>>>> +
>>>> +    /* Serialization Instruction Entries */
>>>> +    action = ACTION_BEGIN_WRITE_OPERATION;
>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>> +
>>>> +    action = ACTION_BEGIN_READ_OPERATION;
>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>> +
>>>> +    action = ACTION_BEGIN_CLEAR_OPERATION;
>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>> +
>>>> +    action = ACTION_END_OPERATION;
>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>> +
>>>> +    action = ACTION_SET_RECORD_OFFSET;
>>>> +    build_serialization_instruction(&wr_value_32, action, 0);
>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>> +
>>>> +    action = ACTION_EXECUTE_OPERATION;
>>>> +    build_serialization_instruction(&wr_value_32_val, action,
>>>> +        ERST_EXECUTE_OPERATION_MAGIC);
>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>> +
>>>> +    action = ACTION_CHECK_BUSY_STATUS;
>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>> +    build_serialization_instruction(&rd_value_32_val, action, 0x01);
>>>> +
>>>> +    action = ACTION_GET_COMMAND_STATUS;
>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>> +    build_serialization_instruction(&rd_value_32, action, 0);
>>>> +
>>>> +    action = ACTION_GET_RECORD_IDENTIFIER;
>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>> +    build_serialization_instruction(&rd_value_64, action, 0);
>>>> +
>>>> +    action = ACTION_SET_RECORD_IDENTIFIER;
>>>> +    build_serialization_instruction(&wr_value_64, action, 0);
>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>> +
>>>> +    action = ACTION_GET_RECORD_COUNT;
>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>> +    build_serialization_instruction(&rd_value_32, action, 0);
>>>> +
>>>> +    action = ACTION_BEGIN_DUMMY_WRITE_OPERATION;
>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>> +
>>>> +    action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE;
>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>> +    build_serialization_instruction(&rd_value_64, action, 0);
>>>> +
>>>> +    action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH;
>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>> +    build_serialization_instruction(&rd_value_64, action, 0);
>>>> +
>>>> +    action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES;
>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>> +    build_serialization_instruction(&rd_value_32, action, 0);
>>>> +
>>>> +    action = ACTION_GET_EXECUTE_OPERATION_TIMINGS;
>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>> +    build_serialization_instruction(&rd_value_64, action, 0);
>>>> +
>>>> +    /* Serialization Header */
>>>> +    acpi_table_begin(&table, table_data);
>>>> +
>>>> +    /* Serialization Header Size */
>>>> +    build_append_int_noprefix(table_data, 48, 4);
>>>> +
>>>> +    /* Reserved */
>>>> +    build_append_int_noprefix(table_data,  0, 4);
>>>> +
>>>> +    /*
>>>> +     * Instruction Entry Count
>>>> +     * Each instruction entry is 32 bytes
>>>> +     */
>>>> +    g_assert((table_instruction_data->len) % 32 == 0);
>>>> +    build_append_int_noprefix(table_data,
>>>> +        (table_instruction_data->len / 32), 4);
>>>> +
>>>> +    /* Serialization Instruction Entries */
>>>> +    g_array_append_vals(table_data, table_instruction_data->data,
>>>> +        table_instruction_data->len);
>>>> +    g_array_free(table_instruction_data, TRUE);
>>>> +
>>>> +    acpi_table_end(linker, &table);
>>>> +}
>>>> +
>>>> +/*******************************************************************/
>>>> +/*******************************************************************/
>>>>    static uint8_t *get_nvram_ptr_by_index(ERSTDeviceState *s, unsigned index)
>>>>    {
>>>>        uint8_t *rc = NULL;
>>>> --
>>>> 1.8.3.1
>>>>
>>>>
>
Re: [PATCH v14 06/10] ACPI ERST: build the ACPI ERST table
Posted by Michael S. Tsirkin 4 years ago 
On Fri, Jan 28, 2022 at 09:11:41AM -0600, Eric DeVolder wrote:
> Michael,
> Thanks for examining this. Inline response below.
> eric
> 
> On 1/27/22 18:37, Michael S. Tsirkin wrote:
> > On Thu, Jan 27, 2022 at 04:02:07PM -0600, Eric DeVolder wrote:
> > > Ani,
> > > Thanks for the RB! Inline responses below.
> > > eric
> > > 
> > > On 1/27/22 02:36, Ani Sinha wrote:
> > > > 
> > > > 
> > > > On Wed, 26 Jan 2022, Eric DeVolder wrote:
> > > > 
> > > > > This builds the ACPI ERST table to inform OSPM how to communicate
> > > > > with the acpi-erst device.
> > > > 
> > > > There might be more optimizations possible but I think we have messaged
> > > > this code enough. We can further rework the code if needed in subsequent
> > > > patches once this is pushed.
> > > > 
> > > > > 
> > > > > Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> > > > 
> > > > with some minor comments,
> > > > 
> > > > Reviewed-by: Ani Sinha <ani@anisinha.ca>
> > > > 
> > > > >    hw/acpi/erst.c | 225 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >    1 file changed, 225 insertions(+)
> > > > > 
> > > > > diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
> > > > > index fe9ba51..5d5a639 100644
> > > > > --- a/hw/acpi/erst.c
> > > > > +++ b/hw/acpi/erst.c
> > > > > @@ -59,6 +59,27 @@
> > > > >    #define STATUS_RECORD_STORE_EMPTY     0x04
> > > > >    #define STATUS_RECORD_NOT_FOUND       0x05
> > > > > 
> > > > > +/* ACPI 4.0: Table 17-19 Serialization Instructions */
> > > > > +#define INST_READ_REGISTER                 0x00
> > > > > +#define INST_READ_REGISTER_VALUE           0x01
> > > > > +#define INST_WRITE_REGISTER                0x02
> > > > > +#define INST_WRITE_REGISTER_VALUE          0x03
> > > > > +#define INST_NOOP                          0x04
> > > > > +#define INST_LOAD_VAR1                     0x05
> > > > > +#define INST_LOAD_VAR2                     0x06
> > > > > +#define INST_STORE_VAR1                    0x07
> > > > > +#define INST_ADD                           0x08
> > > > > +#define INST_SUBTRACT                      0x09
> > > > > +#define INST_ADD_VALUE                     0x0A
> > > > > +#define INST_SUBTRACT_VALUE                0x0B
> > > > > +#define INST_STALL                         0x0C
> > > > > +#define INST_STALL_WHILE_TRUE              0x0D
> > > > > +#define INST_SKIP_NEXT_INSTRUCTION_IF_TRUE 0x0E
> > > > > +#define INST_GOTO                          0x0F
> > > > > +#define INST_SET_SRC_ADDRESS_BASE          0x10
> > > > > +#define INST_SET_DST_ADDRESS_BASE          0x11
> > > > > +#define INST_MOVE_DATA                     0x12
> > > > > +
> > > > >    /* UEFI 2.1: Appendix N Common Platform Error Record */
> > > > >    #define UEFI_CPER_RECORD_MIN_SIZE 128U
> > > > >    #define UEFI_CPER_RECORD_LENGTH_OFFSET 20U
> > > > > @@ -172,6 +193,210 @@ typedef struct {
> > > > > 
> > > > >    /*******************************************************************/
> > > > >    /*******************************************************************/
> > > > > +typedef struct {
> > > > > +    GArray *table_data;
> > > > > +    pcibus_t bar;
> > > > > +    uint8_t instruction;
> > > > > +    uint8_t flags;
> > > > > +    uint8_t register_bit_width;
> > > > > +    pcibus_t register_offset;
> > > > > +} BuildSerializationInstructionEntry;
> > > > > +
> > > > > +/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */
> > > > > +static void build_serialization_instruction(
> > > > > +    BuildSerializationInstructionEntry *e,
> > > > > +    uint8_t serialization_action,
> > > > > +    uint64_t value)
> > > > > +{
> > > > > +    /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */
> > > > > +    struct AcpiGenericAddress gas;
> > > > > +    uint64_t mask;
> > > > > +
> > > > > +    /* Serialization Action */
> > > > > +    build_append_int_noprefix(e->table_data, serialization_action, 1);
> > > > > +    /* Instruction */
> > > > > +    build_append_int_noprefix(e->table_data, e->instruction, 1);
> > > > > +    /* Flags */
> > > > > +    build_append_int_noprefix(e->table_data, e->flags, 1);
> > > > > +    /* Reserved */
> > > > > +    build_append_int_noprefix(e->table_data, 0, 1);
> > > > > +    /* Register Region */
> > > > > +    gas.space_id = AML_SYSTEM_MEMORY;
> > > > > +    gas.bit_width = e->register_bit_width;
> > > > > +    gas.bit_offset = 0;
> > > > > +    gas.access_width = ctz32(e->register_bit_width) - 2;
> > > > 
> > > > Should this be casted as unit8_t?
> > > OK, done.
> > > 
> > > > 
> > > > > +    gas.address = (uint64_t)(e->bar + e->register_offset);
> > > > > +    build_append_gas_from_struct(e->table_data, &gas);
> > > > > +    /* Value */
> > > > > +    build_append_int_noprefix(e->table_data, value, 8);
> > > > > +    /* Mask */
> > > > > +    mask = (1ULL << (e->register_bit_width - 1) << 1) - 1;
> > > > > +    build_append_int_noprefix(e->table_data, mask, 8);
> > > > > +}
> > > > > +
> > > > > +/* ACPI 4.0: 17.4.1 Serialization Action Table */
> > > > > +void build_erst(GArray *table_data, BIOSLinker *linker, Object *erst_dev,
> > > > > +    const char *oem_id, const char *oem_table_id)
> > > > > +{
> > > > > +    /*
> > > > > +     * Serialization Action Table
> > > > > +     * The serialization action table must be generated first
> > > > > +     * so that its size can be known in order to populate the
> > > > > +     * Instruction Entry Count field.
> > > > > +     */
> > > > > +    GArray *table_instruction_data = g_array_new(FALSE, FALSE, sizeof(char));
> > > > > +    pcibus_t bar0 = pci_get_bar_addr(PCI_DEVICE(erst_dev), 0);
> > > > > +    AcpiTable table = { .sig = "ERST", .rev = 1, .oem_id = oem_id,
> > > > > +                        .oem_table_id = oem_table_id };
> > > > > +    /* Contexts for the different ways ACTION and VALUE are accessed */
> > > > > +    BuildSerializationInstructionEntry rd_value_32_val = {
> > > > > +        .table_data = table_instruction_data,
> > > > > +        .bar = bar0,
> > > > > +        .instruction = INST_READ_REGISTER_VALUE,
> > > > > +        .flags = 0,
> > > > > +        .register_bit_width = 32,
> > > > > +        .register_offset = ERST_VALUE_OFFSET,
> > > > > +    };
> > > > > +    BuildSerializationInstructionEntry rd_value_32 = {
> > > > > +        .table_data = table_instruction_data,
> > > > > +        .bar = bar0,
> > > > > +        .instruction = INST_READ_REGISTER,
> > > > > +        .flags = 0,
> > > > > +        .register_bit_width = 32,
> > > > > +        .register_offset = ERST_VALUE_OFFSET,
> > > > > +    };
> > > > > +    BuildSerializationInstructionEntry rd_value_64 = {
> > > > > +        .table_data = table_instruction_data,
> > > > > +        .bar = bar0,
> > > > > +        .instruction = INST_READ_REGISTER,
> > > > > +        .flags = 0,
> > > > > +        .register_bit_width = 64,
> > > > > +        .register_offset = ERST_VALUE_OFFSET,
> > > > > +    };
> > > > > +    BuildSerializationInstructionEntry wr_value_32_val = {
> > > > > +        .table_data = table_instruction_data,
> > > > > +        .bar = bar0,
> > > > > +        .instruction = INST_WRITE_REGISTER_VALUE,
> > > > > +        .flags = 0,
> > > > > +        .register_bit_width = 32,
> > > > > +        .register_offset = ERST_VALUE_OFFSET,
> > > > > +    };
> > > > > +    BuildSerializationInstructionEntry wr_value_32 = {
> > > > > +        .table_data = table_instruction_data,
> > > > > +        .bar = bar0,
> > > > > +        .instruction = INST_WRITE_REGISTER,
> > > > > +        .flags = 0,
> > > > > +        .register_bit_width = 32,
> > > > > +        .register_offset = ERST_VALUE_OFFSET,
> > > > > +    };
> > > > > +    BuildSerializationInstructionEntry wr_value_64 = {
> > > > > +        .table_data = table_instruction_data,
> > > > > +        .bar = bar0,
> > > > > +        .instruction = INST_WRITE_REGISTER,
> > > > > +        .flags = 0,
> > > > > +        .register_bit_width = 64,
> > > > > +        .register_offset = ERST_VALUE_OFFSET,
> > > > > +    };
> > > > > +    BuildSerializationInstructionEntry wr_action = {
> > > > > +        .table_data = table_instruction_data,
> > > > > +        .bar = bar0,
> > > > > +        .instruction = INST_WRITE_REGISTER_VALUE,
> > > > > +        .flags = 0,
> > > > > +        .register_bit_width = 32,
> > > > > +        .register_offset = ERST_ACTION_OFFSET,
> > > > > +    };
> > > > 
> > > > We can probably write a macro to simply generating these structs. I see
> > > > .bar and .flags are the same in all structs. The .bit_width can be a param
> > > > into the macro etc.
> > > Agree, however the request was to NOT hide the use of local variables in
> > > macros, so while .flags is always 0, .instruction, .register_bit_width and .register_offset
> > > would be parameters, that leaves .table_data and .bar which just need the local
> > > variables. Is the following acceptable (which hides the use of the local variables)?
> > 
> > You can just use assignment:
> > 
> >     BuildSerializationInstructionEntry entry = {
> >         .table_data = table_instruction_data,
> >         .bar = bar0,
> >         .flags = 0,
> >         .register_bit_width = 32,
> >     };
> >     BuildSerializationInstructionEntry rd_value_32_val = entry;
> >     rd_value_32_val.action = INST_READ_REGISTER_VALUE;
> >     rd_value_32_val.register_offset = ERST_ACTION_OFFSET;
> > 
> > and so on.
> > not sure it's worth it, it's just a couple of lines.
> > 
> 
> OK, here is the equivalent using struct assignment, is this what you were after?
> 
>     BuildSerializationInstructionEntry base = {
>         .table_data = table_instruction_data,
>         .bar = bar0,
>         .instruction = INST_WRITE_REGISTER,
>         .flags = 0,
>         .register_bit_width = 32,
>         .register_offset = ERST_VALUE_OFFSET,
>     };
>     BuildSerializationInstructionEntry rd_value_32_val = base;
>     rd_value_32_val.instruction = INST_READ_REGISTER_VALUE;
>     BuildSerializationInstructionEntry rd_value_32 = base;
>     rd_value_32.instruction = INST_READ_REGISTER;
>     BuildSerializationInstructionEntry rd_value_64 = base;
>     rd_value_64.instruction = INST_READ_REGISTER;
>     rd_value_64.register_bit_width = 64;
>     BuildSerializationInstructionEntry wr_value_32_val = base;
>     wr_value_32_val.instruction = INST_WRITE_REGISTER_VALUE;
>     BuildSerializationInstructionEntry wr_value_32 = base;
>     BuildSerializationInstructionEntry wr_value_64 = base;
>     wr_value_64.register_bit_width = 64;
>     BuildSerializationInstructionEntry wr_action = base;
>     wr_action.instruction = INST_WRITE_REGISTER_VALUE;
>     wr_action.register_offset = ERST_ACTION_OFFSET;
> 

That's what I described, yes. We should have some empty lines here I
guess. I'm ok with the original one too, there's not too much
duplication.


> 
> > 
> > 
> > > #define SERIALIZATIONINSTRUCTIONCTX(name, \
> > >      inst, bit_width, offset) \
> > >      BuildSerializationInstructionEntry name = { \
> > >          .table_data = table_instruction_data, \
> > >          .bar = bar0, \
> > >          .instruction = inst, \
> > >          .flags = 0, \
> > >          .register_bit_width = bit_width, \
> > >          .register_offset = offset, \
> > >      }
> > >      SERIALIZATIONINSTRUCTIONCTX(rd_value_32_val,
> > >          INST_READ_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
> > >      SERIALIZATIONINSTRUCTIONCTX(rd_value_32,
> > >          INST_READ_REGISTER, 32, ERST_VALUE_OFFSET);
> > >      SERIALIZATIONINSTRUCTIONCTX(rd_value_64,
> > >          INST_READ_REGISTER, 64, ERST_VALUE_OFFSET);
> > >      SERIALIZATIONINSTRUCTIONCTX(wr_value_32_val,
> > >          INST_WRITE_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
> > >      SERIALIZATIONINSTRUCTIONCTX(wr_value_32,
> > >          INST_WRITE_REGISTER, 32, ERST_VALUE_OFFSET);
> > >      SERIALIZATIONINSTRUCTIONCTX(wr_value_64,
> > >          INST_WRITE_REGISTER, 64, ERST_VALUE_OFFSET);
> > >      SERIALIZATIONINSTRUCTIONCTX(wr_action,
> > >          INST_WRITE_REGISTER_VALUE, 32, ERST_ACTION_OFFSET);
> > > 
> > > These are the 7 accessors needed.
> > 
> > not at all sure this one is worth the macro mess.
> 
> I'm hoping to produce a v15 with the style you want.
> eric
> 
> > 
> > > > 
> > > > > +    unsigned action;
> > > > > +
> > > > > +    trace_acpi_erst_pci_bar_0(bar0);
> > > > > +
> > > > > +    /* Serialization Instruction Entries */
> > > > > +    action = ACTION_BEGIN_WRITE_OPERATION;
> > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > +
> > > > > +    action = ACTION_BEGIN_READ_OPERATION;
> > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > +
> > > > > +    action = ACTION_BEGIN_CLEAR_OPERATION;
> > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > +
> > > > > +    action = ACTION_END_OPERATION;
> > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > +
> > > > > +    action = ACTION_SET_RECORD_OFFSET;
> > > > > +    build_serialization_instruction(&wr_value_32, action, 0);
> > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > +
> > > > > +    action = ACTION_EXECUTE_OPERATION;
> > > > > +    build_serialization_instruction(&wr_value_32_val, action,
> > > > > +        ERST_EXECUTE_OPERATION_MAGIC);
> > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > +
> > > > > +    action = ACTION_CHECK_BUSY_STATUS;
> > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > +    build_serialization_instruction(&rd_value_32_val, action, 0x01);
> > > > > +
> > > > > +    action = ACTION_GET_COMMAND_STATUS;
> > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > +    build_serialization_instruction(&rd_value_32, action, 0);
> > > > > +
> > > > > +    action = ACTION_GET_RECORD_IDENTIFIER;
> > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > +    build_serialization_instruction(&rd_value_64, action, 0);
> > > > > +
> > > > > +    action = ACTION_SET_RECORD_IDENTIFIER;
> > > > > +    build_serialization_instruction(&wr_value_64, action, 0);
> > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > +
> > > > > +    action = ACTION_GET_RECORD_COUNT;
> > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > +    build_serialization_instruction(&rd_value_32, action, 0);
> > > > > +
> > > > > +    action = ACTION_BEGIN_DUMMY_WRITE_OPERATION;
> > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > +
> > > > > +    action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE;
> > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > +    build_serialization_instruction(&rd_value_64, action, 0);
> > > > > +
> > > > > +    action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH;
> > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > +    build_serialization_instruction(&rd_value_64, action, 0);
> > > > > +
> > > > > +    action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES;
> > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > +    build_serialization_instruction(&rd_value_32, action, 0);
> > > > > +
> > > > > +    action = ACTION_GET_EXECUTE_OPERATION_TIMINGS;
> > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > +    build_serialization_instruction(&rd_value_64, action, 0);
> > > > > +
> > > > > +    /* Serialization Header */
> > > > > +    acpi_table_begin(&table, table_data);
> > > > > +
> > > > > +    /* Serialization Header Size */
> > > > > +    build_append_int_noprefix(table_data, 48, 4);
> > > > > +
> > > > > +    /* Reserved */
> > > > > +    build_append_int_noprefix(table_data,  0, 4);
> > > > > +
> > > > > +    /*
> > > > > +     * Instruction Entry Count
> > > > > +     * Each instruction entry is 32 bytes
> > > > > +     */
> > > > > +    g_assert((table_instruction_data->len) % 32 == 0);
> > > > > +    build_append_int_noprefix(table_data,
> > > > > +        (table_instruction_data->len / 32), 4);
> > > > > +
> > > > > +    /* Serialization Instruction Entries */
> > > > > +    g_array_append_vals(table_data, table_instruction_data->data,
> > > > > +        table_instruction_data->len);
> > > > > +    g_array_free(table_instruction_data, TRUE);
> > > > > +
> > > > > +    acpi_table_end(linker, &table);
> > > > > +}
> > > > > +
> > > > > +/*******************************************************************/
> > > > > +/*******************************************************************/
> > > > >    static uint8_t *get_nvram_ptr_by_index(ERSTDeviceState *s, unsigned index)
> > > > >    {
> > > > >        uint8_t *rc = NULL;
> > > > > --
> > > > > 1.8.3.1
> > > > > 
> > > > > 
> >
Re: [PATCH v14 06/10] ACPI ERST: build the ACPI ERST table
Posted by Eric DeVolder 4 years ago 
Michael, thanks! See inline response below, please.
eric

On 1/28/22 09:54, Michael S. Tsirkin wrote:
> On Fri, Jan 28, 2022 at 09:11:41AM -0600, Eric DeVolder wrote:
>> Michael,
>> Thanks for examining this. Inline response below.
>> eric
>>
>> On 1/27/22 18:37, Michael S. Tsirkin wrote:
>>> On Thu, Jan 27, 2022 at 04:02:07PM -0600, Eric DeVolder wrote:
>>>> Ani,
>>>> Thanks for the RB! Inline responses below.
>>>> eric
>>>>
>>>> On 1/27/22 02:36, Ani Sinha wrote:
>>>>>
>>>>>
>>>>> On Wed, 26 Jan 2022, Eric DeVolder wrote:
>>>>>
>>>>>> This builds the ACPI ERST table to inform OSPM how to communicate
>>>>>> with the acpi-erst device.
>>>>>
>>>>> There might be more optimizations possible but I think we have messaged
>>>>> this code enough. We can further rework the code if needed in subsequent
>>>>> patches once this is pushed.
>>>>>
>>>>>>
>>>>>> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
>>>>>
>>>>> with some minor comments,
>>>>>
>>>>> Reviewed-by: Ani Sinha <ani@anisinha.ca>
>>>>>
>>>>>>     hw/acpi/erst.c | 225 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>     1 file changed, 225 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
>>>>>> index fe9ba51..5d5a639 100644
>>>>>> --- a/hw/acpi/erst.c
>>>>>> +++ b/hw/acpi/erst.c
>>>>>> @@ -59,6 +59,27 @@
>>>>>>     #define STATUS_RECORD_STORE_EMPTY     0x04
>>>>>>     #define STATUS_RECORD_NOT_FOUND       0x05
>>>>>>
>>>>>> +/* ACPI 4.0: Table 17-19 Serialization Instructions */
>>>>>> +#define INST_READ_REGISTER                 0x00
>>>>>> +#define INST_READ_REGISTER_VALUE           0x01
>>>>>> +#define INST_WRITE_REGISTER                0x02
>>>>>> +#define INST_WRITE_REGISTER_VALUE          0x03
>>>>>> +#define INST_NOOP                          0x04
>>>>>> +#define INST_LOAD_VAR1                     0x05
>>>>>> +#define INST_LOAD_VAR2                     0x06
>>>>>> +#define INST_STORE_VAR1                    0x07
>>>>>> +#define INST_ADD                           0x08
>>>>>> +#define INST_SUBTRACT                      0x09
>>>>>> +#define INST_ADD_VALUE                     0x0A
>>>>>> +#define INST_SUBTRACT_VALUE                0x0B
>>>>>> +#define INST_STALL                         0x0C
>>>>>> +#define INST_STALL_WHILE_TRUE              0x0D
>>>>>> +#define INST_SKIP_NEXT_INSTRUCTION_IF_TRUE 0x0E
>>>>>> +#define INST_GOTO                          0x0F
>>>>>> +#define INST_SET_SRC_ADDRESS_BASE          0x10
>>>>>> +#define INST_SET_DST_ADDRESS_BASE          0x11
>>>>>> +#define INST_MOVE_DATA                     0x12
>>>>>> +
>>>>>>     /* UEFI 2.1: Appendix N Common Platform Error Record */
>>>>>>     #define UEFI_CPER_RECORD_MIN_SIZE 128U
>>>>>>     #define UEFI_CPER_RECORD_LENGTH_OFFSET 20U
>>>>>> @@ -172,6 +193,210 @@ typedef struct {
>>>>>>
>>>>>>     /*******************************************************************/
>>>>>>     /*******************************************************************/
>>>>>> +typedef struct {
>>>>>> +    GArray *table_data;
>>>>>> +    pcibus_t bar;
>>>>>> +    uint8_t instruction;
>>>>>> +    uint8_t flags;
>>>>>> +    uint8_t register_bit_width;
>>>>>> +    pcibus_t register_offset;
>>>>>> +} BuildSerializationInstructionEntry;
>>>>>> +
>>>>>> +/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */
>>>>>> +static void build_serialization_instruction(
>>>>>> +    BuildSerializationInstructionEntry *e,
>>>>>> +    uint8_t serialization_action,
>>>>>> +    uint64_t value)
>>>>>> +{
>>>>>> +    /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */
>>>>>> +    struct AcpiGenericAddress gas;
>>>>>> +    uint64_t mask;
>>>>>> +
>>>>>> +    /* Serialization Action */
>>>>>> +    build_append_int_noprefix(e->table_data, serialization_action, 1);
>>>>>> +    /* Instruction */
>>>>>> +    build_append_int_noprefix(e->table_data, e->instruction, 1);
>>>>>> +    /* Flags */
>>>>>> +    build_append_int_noprefix(e->table_data, e->flags, 1);
>>>>>> +    /* Reserved */
>>>>>> +    build_append_int_noprefix(e->table_data, 0, 1);
>>>>>> +    /* Register Region */
>>>>>> +    gas.space_id = AML_SYSTEM_MEMORY;
>>>>>> +    gas.bit_width = e->register_bit_width;
>>>>>> +    gas.bit_offset = 0;
>>>>>> +    gas.access_width = ctz32(e->register_bit_width) - 2;
>>>>>
>>>>> Should this be casted as unit8_t?
>>>> OK, done.
>>>>
>>>>>
>>>>>> +    gas.address = (uint64_t)(e->bar + e->register_offset);
>>>>>> +    build_append_gas_from_struct(e->table_data, &gas);
>>>>>> +    /* Value */
>>>>>> +    build_append_int_noprefix(e->table_data, value, 8);
>>>>>> +    /* Mask */
>>>>>> +    mask = (1ULL << (e->register_bit_width - 1) << 1) - 1;
>>>>>> +    build_append_int_noprefix(e->table_data, mask, 8);
>>>>>> +}
>>>>>> +
>>>>>> +/* ACPI 4.0: 17.4.1 Serialization Action Table */
>>>>>> +void build_erst(GArray *table_data, BIOSLinker *linker, Object *erst_dev,
>>>>>> +    const char *oem_id, const char *oem_table_id)
>>>>>> +{
>>>>>> +    /*
>>>>>> +     * Serialization Action Table
>>>>>> +     * The serialization action table must be generated first
>>>>>> +     * so that its size can be known in order to populate the
>>>>>> +     * Instruction Entry Count field.
>>>>>> +     */
>>>>>> +    GArray *table_instruction_data = g_array_new(FALSE, FALSE, sizeof(char));
>>>>>> +    pcibus_t bar0 = pci_get_bar_addr(PCI_DEVICE(erst_dev), 0);
>>>>>> +    AcpiTable table = { .sig = "ERST", .rev = 1, .oem_id = oem_id,
>>>>>> +                        .oem_table_id = oem_table_id };
>>>>>> +    /* Contexts for the different ways ACTION and VALUE are accessed */
>>>>>> +    BuildSerializationInstructionEntry rd_value_32_val = {
>>>>>> +        .table_data = table_instruction_data,
>>>>>> +        .bar = bar0,
>>>>>> +        .instruction = INST_READ_REGISTER_VALUE,
>>>>>> +        .flags = 0,
>>>>>> +        .register_bit_width = 32,
>>>>>> +        .register_offset = ERST_VALUE_OFFSET,
>>>>>> +    };
>>>>>> +    BuildSerializationInstructionEntry rd_value_32 = {
>>>>>> +        .table_data = table_instruction_data,
>>>>>> +        .bar = bar0,
>>>>>> +        .instruction = INST_READ_REGISTER,
>>>>>> +        .flags = 0,
>>>>>> +        .register_bit_width = 32,
>>>>>> +        .register_offset = ERST_VALUE_OFFSET,
>>>>>> +    };
>>>>>> +    BuildSerializationInstructionEntry rd_value_64 = {
>>>>>> +        .table_data = table_instruction_data,
>>>>>> +        .bar = bar0,
>>>>>> +        .instruction = INST_READ_REGISTER,
>>>>>> +        .flags = 0,
>>>>>> +        .register_bit_width = 64,
>>>>>> +        .register_offset = ERST_VALUE_OFFSET,
>>>>>> +    };
>>>>>> +    BuildSerializationInstructionEntry wr_value_32_val = {
>>>>>> +        .table_data = table_instruction_data,
>>>>>> +        .bar = bar0,
>>>>>> +        .instruction = INST_WRITE_REGISTER_VALUE,
>>>>>> +        .flags = 0,
>>>>>> +        .register_bit_width = 32,
>>>>>> +        .register_offset = ERST_VALUE_OFFSET,
>>>>>> +    };
>>>>>> +    BuildSerializationInstructionEntry wr_value_32 = {
>>>>>> +        .table_data = table_instruction_data,
>>>>>> +        .bar = bar0,
>>>>>> +        .instruction = INST_WRITE_REGISTER,
>>>>>> +        .flags = 0,
>>>>>> +        .register_bit_width = 32,
>>>>>> +        .register_offset = ERST_VALUE_OFFSET,
>>>>>> +    };
>>>>>> +    BuildSerializationInstructionEntry wr_value_64 = {
>>>>>> +        .table_data = table_instruction_data,
>>>>>> +        .bar = bar0,
>>>>>> +        .instruction = INST_WRITE_REGISTER,
>>>>>> +        .flags = 0,
>>>>>> +        .register_bit_width = 64,
>>>>>> +        .register_offset = ERST_VALUE_OFFSET,
>>>>>> +    };
>>>>>> +    BuildSerializationInstructionEntry wr_action = {
>>>>>> +        .table_data = table_instruction_data,
>>>>>> +        .bar = bar0,
>>>>>> +        .instruction = INST_WRITE_REGISTER_VALUE,
>>>>>> +        .flags = 0,
>>>>>> +        .register_bit_width = 32,
>>>>>> +        .register_offset = ERST_ACTION_OFFSET,
>>>>>> +    };
>>>>>
>>>>> We can probably write a macro to simply generating these structs. I see
>>>>> .bar and .flags are the same in all structs. The .bit_width can be a param
>>>>> into the macro etc.
>>>> Agree, however the request was to NOT hide the use of local variables in
>>>> macros, so while .flags is always 0, .instruction, .register_bit_width and .register_offset
>>>> would be parameters, that leaves .table_data and .bar which just need the local
>>>> variables. Is the following acceptable (which hides the use of the local variables)?
>>>
>>> You can just use assignment:
>>>
>>>      BuildSerializationInstructionEntry entry = {
>>>          .table_data = table_instruction_data,
>>>          .bar = bar0,
>>>          .flags = 0,
>>>          .register_bit_width = 32,
>>>      };
>>>      BuildSerializationInstructionEntry rd_value_32_val = entry;
>>>      rd_value_32_val.action = INST_READ_REGISTER_VALUE;
>>>      rd_value_32_val.register_offset = ERST_ACTION_OFFSET;
>>>
>>> and so on.
>>> not sure it's worth it, it's just a couple of lines.
>>>
>>
>> OK, here is the equivalent using struct assignment, is this what you were after?
>>
>>      BuildSerializationInstructionEntry base = {
>>          .table_data = table_instruction_data,
>>          .bar = bar0,
>>          .instruction = INST_WRITE_REGISTER,
>>          .flags = 0,
>>          .register_bit_width = 32,
>>          .register_offset = ERST_VALUE_OFFSET,
>>      };
>>      BuildSerializationInstructionEntry rd_value_32_val = base;
>>      rd_value_32_val.instruction = INST_READ_REGISTER_VALUE;
>>      BuildSerializationInstructionEntry rd_value_32 = base;
>>      rd_value_32.instruction = INST_READ_REGISTER;
>>      BuildSerializationInstructionEntry rd_value_64 = base;
>>      rd_value_64.instruction = INST_READ_REGISTER;
>>      rd_value_64.register_bit_width = 64;
>>      BuildSerializationInstructionEntry wr_value_32_val = base;
>>      wr_value_32_val.instruction = INST_WRITE_REGISTER_VALUE;
>>      BuildSerializationInstructionEntry wr_value_32 = base;
>>      BuildSerializationInstructionEntry wr_value_64 = base;
>>      wr_value_64.register_bit_width = 64;
>>      BuildSerializationInstructionEntry wr_action = base;
>>      wr_action.instruction = INST_WRITE_REGISTER_VALUE;
>>      wr_action.register_offset = ERST_ACTION_OFFSET;
>>
> 
> That's what I described, yes. We should have some empty lines here I
> guess. I'm ok with the original one too, there's not too much
> duplication.

Are the blank lines referring to spacing out the setup of each of the 7 accesors?
If so, I could put a one line comment between each setup? Or is a blank line also
needed?

Is it OK to post v15 with the struct assignment approach? Or would you prefer the
explicit structs (which is what I think you mean by 'the original one')?

Thanks!
eric
> 
> 
>>
>>>
>>>
>>>> #define SERIALIZATIONINSTRUCTIONCTX(name, \
>>>>       inst, bit_width, offset) \
>>>>       BuildSerializationInstructionEntry name = { \
>>>>           .table_data = table_instruction_data, \
>>>>           .bar = bar0, \
>>>>           .instruction = inst, \
>>>>           .flags = 0, \
>>>>           .register_bit_width = bit_width, \
>>>>           .register_offset = offset, \
>>>>       }
>>>>       SERIALIZATIONINSTRUCTIONCTX(rd_value_32_val,
>>>>           INST_READ_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
>>>>       SERIALIZATIONINSTRUCTIONCTX(rd_value_32,
>>>>           INST_READ_REGISTER, 32, ERST_VALUE_OFFSET);
>>>>       SERIALIZATIONINSTRUCTIONCTX(rd_value_64,
>>>>           INST_READ_REGISTER, 64, ERST_VALUE_OFFSET);
>>>>       SERIALIZATIONINSTRUCTIONCTX(wr_value_32_val,
>>>>           INST_WRITE_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
>>>>       SERIALIZATIONINSTRUCTIONCTX(wr_value_32,
>>>>           INST_WRITE_REGISTER, 32, ERST_VALUE_OFFSET);
>>>>       SERIALIZATIONINSTRUCTIONCTX(wr_value_64,
>>>>           INST_WRITE_REGISTER, 64, ERST_VALUE_OFFSET);
>>>>       SERIALIZATIONINSTRUCTIONCTX(wr_action,
>>>>           INST_WRITE_REGISTER_VALUE, 32, ERST_ACTION_OFFSET);
>>>>
>>>> These are the 7 accessors needed.
>>>
>>> not at all sure this one is worth the macro mess.
>>
>> I'm hoping to produce a v15 with the style you want.
>> eric
>>
>>>
>>>>>
>>>>>> +    unsigned action;
>>>>>> +
>>>>>> +    trace_acpi_erst_pci_bar_0(bar0);
>>>>>> +
>>>>>> +    /* Serialization Instruction Entries */
>>>>>> +    action = ACTION_BEGIN_WRITE_OPERATION;
>>>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>>>> +
>>>>>> +    action = ACTION_BEGIN_READ_OPERATION;
>>>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>>>> +
>>>>>> +    action = ACTION_BEGIN_CLEAR_OPERATION;
>>>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>>>> +
>>>>>> +    action = ACTION_END_OPERATION;
>>>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>>>> +
>>>>>> +    action = ACTION_SET_RECORD_OFFSET;
>>>>>> +    build_serialization_instruction(&wr_value_32, action, 0);
>>>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>>>> +
>>>>>> +    action = ACTION_EXECUTE_OPERATION;
>>>>>> +    build_serialization_instruction(&wr_value_32_val, action,
>>>>>> +        ERST_EXECUTE_OPERATION_MAGIC);
>>>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>>>> +
>>>>>> +    action = ACTION_CHECK_BUSY_STATUS;
>>>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>>>> +    build_serialization_instruction(&rd_value_32_val, action, 0x01);
>>>>>> +
>>>>>> +    action = ACTION_GET_COMMAND_STATUS;
>>>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>>>> +    build_serialization_instruction(&rd_value_32, action, 0);
>>>>>> +
>>>>>> +    action = ACTION_GET_RECORD_IDENTIFIER;
>>>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>>>> +    build_serialization_instruction(&rd_value_64, action, 0);
>>>>>> +
>>>>>> +    action = ACTION_SET_RECORD_IDENTIFIER;
>>>>>> +    build_serialization_instruction(&wr_value_64, action, 0);
>>>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>>>> +
>>>>>> +    action = ACTION_GET_RECORD_COUNT;
>>>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>>>> +    build_serialization_instruction(&rd_value_32, action, 0);
>>>>>> +
>>>>>> +    action = ACTION_BEGIN_DUMMY_WRITE_OPERATION;
>>>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>>>> +
>>>>>> +    action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE;
>>>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>>>> +    build_serialization_instruction(&rd_value_64, action, 0);
>>>>>> +
>>>>>> +    action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH;
>>>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>>>> +    build_serialization_instruction(&rd_value_64, action, 0);
>>>>>> +
>>>>>> +    action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES;
>>>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>>>> +    build_serialization_instruction(&rd_value_32, action, 0);
>>>>>> +
>>>>>> +    action = ACTION_GET_EXECUTE_OPERATION_TIMINGS;
>>>>>> +    build_serialization_instruction(&wr_action, action, action);
>>>>>> +    build_serialization_instruction(&rd_value_64, action, 0);
>>>>>> +
>>>>>> +    /* Serialization Header */
>>>>>> +    acpi_table_begin(&table, table_data);
>>>>>> +
>>>>>> +    /* Serialization Header Size */
>>>>>> +    build_append_int_noprefix(table_data, 48, 4);
>>>>>> +
>>>>>> +    /* Reserved */
>>>>>> +    build_append_int_noprefix(table_data,  0, 4);
>>>>>> +
>>>>>> +    /*
>>>>>> +     * Instruction Entry Count
>>>>>> +     * Each instruction entry is 32 bytes
>>>>>> +     */
>>>>>> +    g_assert((table_instruction_data->len) % 32 == 0);
>>>>>> +    build_append_int_noprefix(table_data,
>>>>>> +        (table_instruction_data->len / 32), 4);
>>>>>> +
>>>>>> +    /* Serialization Instruction Entries */
>>>>>> +    g_array_append_vals(table_data, table_instruction_data->data,
>>>>>> +        table_instruction_data->len);
>>>>>> +    g_array_free(table_instruction_data, TRUE);
>>>>>> +
>>>>>> +    acpi_table_end(linker, &table);
>>>>>> +}
>>>>>> +
>>>>>> +/*******************************************************************/
>>>>>> +/*******************************************************************/
>>>>>>     static uint8_t *get_nvram_ptr_by_index(ERSTDeviceState *s, unsigned index)
>>>>>>     {
>>>>>>         uint8_t *rc = NULL;
>>>>>> --
>>>>>> 1.8.3.1
>>>>>>
>>>>>>
>>>
>
Re: [PATCH v14 06/10] ACPI ERST: build the ACPI ERST table
Posted by Michael S. Tsirkin 4 years ago 
On Fri, Jan 28, 2022 at 10:01:18AM -0600, Eric DeVolder wrote:
> Michael, thanks! See inline response below, please.
> eric
> 
> On 1/28/22 09:54, Michael S. Tsirkin wrote:
> > On Fri, Jan 28, 2022 at 09:11:41AM -0600, Eric DeVolder wrote:
> > > Michael,
> > > Thanks for examining this. Inline response below.
> > > eric
> > > 
> > > On 1/27/22 18:37, Michael S. Tsirkin wrote:
> > > > On Thu, Jan 27, 2022 at 04:02:07PM -0600, Eric DeVolder wrote:
> > > > > Ani,
> > > > > Thanks for the RB! Inline responses below.
> > > > > eric
> > > > > 
> > > > > On 1/27/22 02:36, Ani Sinha wrote:
> > > > > > 
> > > > > > 
> > > > > > On Wed, 26 Jan 2022, Eric DeVolder wrote:
> > > > > > 
> > > > > > > This builds the ACPI ERST table to inform OSPM how to communicate
> > > > > > > with the acpi-erst device.
> > > > > > 
> > > > > > There might be more optimizations possible but I think we have messaged
> > > > > > this code enough. We can further rework the code if needed in subsequent
> > > > > > patches once this is pushed.
> > > > > > 
> > > > > > > 
> > > > > > > Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> > > > > > 
> > > > > > with some minor comments,
> > > > > > 
> > > > > > Reviewed-by: Ani Sinha <ani@anisinha.ca>
> > > > > > 
> > > > > > >     hw/acpi/erst.c | 225 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > >     1 file changed, 225 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
> > > > > > > index fe9ba51..5d5a639 100644
> > > > > > > --- a/hw/acpi/erst.c
> > > > > > > +++ b/hw/acpi/erst.c
> > > > > > > @@ -59,6 +59,27 @@
> > > > > > >     #define STATUS_RECORD_STORE_EMPTY     0x04
> > > > > > >     #define STATUS_RECORD_NOT_FOUND       0x05
> > > > > > > 
> > > > > > > +/* ACPI 4.0: Table 17-19 Serialization Instructions */
> > > > > > > +#define INST_READ_REGISTER                 0x00
> > > > > > > +#define INST_READ_REGISTER_VALUE           0x01
> > > > > > > +#define INST_WRITE_REGISTER                0x02
> > > > > > > +#define INST_WRITE_REGISTER_VALUE          0x03
> > > > > > > +#define INST_NOOP                          0x04
> > > > > > > +#define INST_LOAD_VAR1                     0x05
> > > > > > > +#define INST_LOAD_VAR2                     0x06
> > > > > > > +#define INST_STORE_VAR1                    0x07
> > > > > > > +#define INST_ADD                           0x08
> > > > > > > +#define INST_SUBTRACT                      0x09
> > > > > > > +#define INST_ADD_VALUE                     0x0A
> > > > > > > +#define INST_SUBTRACT_VALUE                0x0B
> > > > > > > +#define INST_STALL                         0x0C
> > > > > > > +#define INST_STALL_WHILE_TRUE              0x0D
> > > > > > > +#define INST_SKIP_NEXT_INSTRUCTION_IF_TRUE 0x0E
> > > > > > > +#define INST_GOTO                          0x0F
> > > > > > > +#define INST_SET_SRC_ADDRESS_BASE          0x10
> > > > > > > +#define INST_SET_DST_ADDRESS_BASE          0x11
> > > > > > > +#define INST_MOVE_DATA                     0x12
> > > > > > > +
> > > > > > >     /* UEFI 2.1: Appendix N Common Platform Error Record */
> > > > > > >     #define UEFI_CPER_RECORD_MIN_SIZE 128U
> > > > > > >     #define UEFI_CPER_RECORD_LENGTH_OFFSET 20U
> > > > > > > @@ -172,6 +193,210 @@ typedef struct {
> > > > > > > 
> > > > > > >     /*******************************************************************/
> > > > > > >     /*******************************************************************/
> > > > > > > +typedef struct {
> > > > > > > +    GArray *table_data;
> > > > > > > +    pcibus_t bar;
> > > > > > > +    uint8_t instruction;
> > > > > > > +    uint8_t flags;
> > > > > > > +    uint8_t register_bit_width;
> > > > > > > +    pcibus_t register_offset;
> > > > > > > +} BuildSerializationInstructionEntry;
> > > > > > > +
> > > > > > > +/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */
> > > > > > > +static void build_serialization_instruction(
> > > > > > > +    BuildSerializationInstructionEntry *e,
> > > > > > > +    uint8_t serialization_action,
> > > > > > > +    uint64_t value)
> > > > > > > +{
> > > > > > > +    /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */
> > > > > > > +    struct AcpiGenericAddress gas;
> > > > > > > +    uint64_t mask;
> > > > > > > +
> > > > > > > +    /* Serialization Action */
> > > > > > > +    build_append_int_noprefix(e->table_data, serialization_action, 1);
> > > > > > > +    /* Instruction */
> > > > > > > +    build_append_int_noprefix(e->table_data, e->instruction, 1);
> > > > > > > +    /* Flags */
> > > > > > > +    build_append_int_noprefix(e->table_data, e->flags, 1);
> > > > > > > +    /* Reserved */
> > > > > > > +    build_append_int_noprefix(e->table_data, 0, 1);
> > > > > > > +    /* Register Region */
> > > > > > > +    gas.space_id = AML_SYSTEM_MEMORY;
> > > > > > > +    gas.bit_width = e->register_bit_width;
> > > > > > > +    gas.bit_offset = 0;
> > > > > > > +    gas.access_width = ctz32(e->register_bit_width) - 2;
> > > > > > 
> > > > > > Should this be casted as unit8_t?
> > > > > OK, done.
> > > > > 
> > > > > > 
> > > > > > > +    gas.address = (uint64_t)(e->bar + e->register_offset);
> > > > > > > +    build_append_gas_from_struct(e->table_data, &gas);
> > > > > > > +    /* Value */
> > > > > > > +    build_append_int_noprefix(e->table_data, value, 8);
> > > > > > > +    /* Mask */
> > > > > > > +    mask = (1ULL << (e->register_bit_width - 1) << 1) - 1;
> > > > > > > +    build_append_int_noprefix(e->table_data, mask, 8);
> > > > > > > +}
> > > > > > > +
> > > > > > > +/* ACPI 4.0: 17.4.1 Serialization Action Table */
> > > > > > > +void build_erst(GArray *table_data, BIOSLinker *linker, Object *erst_dev,
> > > > > > > +    const char *oem_id, const char *oem_table_id)
> > > > > > > +{
> > > > > > > +    /*
> > > > > > > +     * Serialization Action Table
> > > > > > > +     * The serialization action table must be generated first
> > > > > > > +     * so that its size can be known in order to populate the
> > > > > > > +     * Instruction Entry Count field.
> > > > > > > +     */
> > > > > > > +    GArray *table_instruction_data = g_array_new(FALSE, FALSE, sizeof(char));
> > > > > > > +    pcibus_t bar0 = pci_get_bar_addr(PCI_DEVICE(erst_dev), 0);
> > > > > > > +    AcpiTable table = { .sig = "ERST", .rev = 1, .oem_id = oem_id,
> > > > > > > +                        .oem_table_id = oem_table_id };
> > > > > > > +    /* Contexts for the different ways ACTION and VALUE are accessed */
> > > > > > > +    BuildSerializationInstructionEntry rd_value_32_val = {
> > > > > > > +        .table_data = table_instruction_data,
> > > > > > > +        .bar = bar0,
> > > > > > > +        .instruction = INST_READ_REGISTER_VALUE,
> > > > > > > +        .flags = 0,
> > > > > > > +        .register_bit_width = 32,
> > > > > > > +        .register_offset = ERST_VALUE_OFFSET,
> > > > > > > +    };
> > > > > > > +    BuildSerializationInstructionEntry rd_value_32 = {
> > > > > > > +        .table_data = table_instruction_data,
> > > > > > > +        .bar = bar0,
> > > > > > > +        .instruction = INST_READ_REGISTER,
> > > > > > > +        .flags = 0,
> > > > > > > +        .register_bit_width = 32,
> > > > > > > +        .register_offset = ERST_VALUE_OFFSET,
> > > > > > > +    };
> > > > > > > +    BuildSerializationInstructionEntry rd_value_64 = {
> > > > > > > +        .table_data = table_instruction_data,
> > > > > > > +        .bar = bar0,
> > > > > > > +        .instruction = INST_READ_REGISTER,
> > > > > > > +        .flags = 0,
> > > > > > > +        .register_bit_width = 64,
> > > > > > > +        .register_offset = ERST_VALUE_OFFSET,
> > > > > > > +    };
> > > > > > > +    BuildSerializationInstructionEntry wr_value_32_val = {
> > > > > > > +        .table_data = table_instruction_data,
> > > > > > > +        .bar = bar0,
> > > > > > > +        .instruction = INST_WRITE_REGISTER_VALUE,
> > > > > > > +        .flags = 0,
> > > > > > > +        .register_bit_width = 32,
> > > > > > > +        .register_offset = ERST_VALUE_OFFSET,
> > > > > > > +    };
> > > > > > > +    BuildSerializationInstructionEntry wr_value_32 = {
> > > > > > > +        .table_data = table_instruction_data,
> > > > > > > +        .bar = bar0,
> > > > > > > +        .instruction = INST_WRITE_REGISTER,
> > > > > > > +        .flags = 0,
> > > > > > > +        .register_bit_width = 32,
> > > > > > > +        .register_offset = ERST_VALUE_OFFSET,
> > > > > > > +    };
> > > > > > > +    BuildSerializationInstructionEntry wr_value_64 = {
> > > > > > > +        .table_data = table_instruction_data,
> > > > > > > +        .bar = bar0,
> > > > > > > +        .instruction = INST_WRITE_REGISTER,
> > > > > > > +        .flags = 0,
> > > > > > > +        .register_bit_width = 64,
> > > > > > > +        .register_offset = ERST_VALUE_OFFSET,
> > > > > > > +    };
> > > > > > > +    BuildSerializationInstructionEntry wr_action = {
> > > > > > > +        .table_data = table_instruction_data,
> > > > > > > +        .bar = bar0,
> > > > > > > +        .instruction = INST_WRITE_REGISTER_VALUE,
> > > > > > > +        .flags = 0,
> > > > > > > +        .register_bit_width = 32,
> > > > > > > +        .register_offset = ERST_ACTION_OFFSET,
> > > > > > > +    };
> > > > > > 
> > > > > > We can probably write a macro to simply generating these structs. I see
> > > > > > .bar and .flags are the same in all structs. The .bit_width can be a param
> > > > > > into the macro etc.
> > > > > Agree, however the request was to NOT hide the use of local variables in
> > > > > macros, so while .flags is always 0, .instruction, .register_bit_width and .register_offset
> > > > > would be parameters, that leaves .table_data and .bar which just need the local
> > > > > variables. Is the following acceptable (which hides the use of the local variables)?
> > > > 
> > > > You can just use assignment:
> > > > 
> > > >      BuildSerializationInstructionEntry entry = {
> > > >          .table_data = table_instruction_data,
> > > >          .bar = bar0,
> > > >          .flags = 0,
> > > >          .register_bit_width = 32,
> > > >      };
> > > >      BuildSerializationInstructionEntry rd_value_32_val = entry;
> > > >      rd_value_32_val.action = INST_READ_REGISTER_VALUE;
> > > >      rd_value_32_val.register_offset = ERST_ACTION_OFFSET;
> > > > 
> > > > and so on.
> > > > not sure it's worth it, it's just a couple of lines.
> > > > 
> > > 
> > > OK, here is the equivalent using struct assignment, is this what you were after?
> > > 
> > >      BuildSerializationInstructionEntry base = {
> > >          .table_data = table_instruction_data,
> > >          .bar = bar0,
> > >          .instruction = INST_WRITE_REGISTER,
> > >          .flags = 0,
> > >          .register_bit_width = 32,
> > >          .register_offset = ERST_VALUE_OFFSET,
> > >      };
> > >      BuildSerializationInstructionEntry rd_value_32_val = base;
> > >      rd_value_32_val.instruction = INST_READ_REGISTER_VALUE;
> > >      BuildSerializationInstructionEntry rd_value_32 = base;
> > >      rd_value_32.instruction = INST_READ_REGISTER;
> > >      BuildSerializationInstructionEntry rd_value_64 = base;
> > >      rd_value_64.instruction = INST_READ_REGISTER;
> > >      rd_value_64.register_bit_width = 64;
> > >      BuildSerializationInstructionEntry wr_value_32_val = base;
> > >      wr_value_32_val.instruction = INST_WRITE_REGISTER_VALUE;
> > >      BuildSerializationInstructionEntry wr_value_32 = base;
> > >      BuildSerializationInstructionEntry wr_value_64 = base;
> > >      wr_value_64.register_bit_width = 64;
> > >      BuildSerializationInstructionEntry wr_action = base;
> > >      wr_action.instruction = INST_WRITE_REGISTER_VALUE;
> > >      wr_action.register_offset = ERST_ACTION_OFFSET;
> > > 
> > 
> > That's what I described, yes. We should have some empty lines here I
> > guess. I'm ok with the original one too, there's not too much
> > duplication.
> 
> Are the blank lines referring to spacing out the setup of each of the 7 accesors?
> If so, I could put a one line comment between each setup? Or is a blank line also
> needed?

A blank line between declarations and code is usually a good idea.


> Is it OK to post v15 with the struct assignment approach? Or would you prefer the
> explicit structs (which is what I think you mean by 'the original one')?
> 
> Thanks!
> eric

I don't care either way.

> > 
> > 
> > > 
> > > > 
> > > > 
> > > > > #define SERIALIZATIONINSTRUCTIONCTX(name, \
> > > > >       inst, bit_width, offset) \
> > > > >       BuildSerializationInstructionEntry name = { \
> > > > >           .table_data = table_instruction_data, \
> > > > >           .bar = bar0, \
> > > > >           .instruction = inst, \
> > > > >           .flags = 0, \
> > > > >           .register_bit_width = bit_width, \
> > > > >           .register_offset = offset, \
> > > > >       }
> > > > >       SERIALIZATIONINSTRUCTIONCTX(rd_value_32_val,
> > > > >           INST_READ_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
> > > > >       SERIALIZATIONINSTRUCTIONCTX(rd_value_32,
> > > > >           INST_READ_REGISTER, 32, ERST_VALUE_OFFSET);
> > > > >       SERIALIZATIONINSTRUCTIONCTX(rd_value_64,
> > > > >           INST_READ_REGISTER, 64, ERST_VALUE_OFFSET);
> > > > >       SERIALIZATIONINSTRUCTIONCTX(wr_value_32_val,
> > > > >           INST_WRITE_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
> > > > >       SERIALIZATIONINSTRUCTIONCTX(wr_value_32,
> > > > >           INST_WRITE_REGISTER, 32, ERST_VALUE_OFFSET);
> > > > >       SERIALIZATIONINSTRUCTIONCTX(wr_value_64,
> > > > >           INST_WRITE_REGISTER, 64, ERST_VALUE_OFFSET);
> > > > >       SERIALIZATIONINSTRUCTIONCTX(wr_action,
> > > > >           INST_WRITE_REGISTER_VALUE, 32, ERST_ACTION_OFFSET);
> > > > > 
> > > > > These are the 7 accessors needed.
> > > > 
> > > > not at all sure this one is worth the macro mess.
> > > 
> > > I'm hoping to produce a v15 with the style you want.
> > > eric
> > > 
> > > > 
> > > > > > 
> > > > > > > +    unsigned action;
> > > > > > > +
> > > > > > > +    trace_acpi_erst_pci_bar_0(bar0);
> > > > > > > +
> > > > > > > +    /* Serialization Instruction Entries */
> > > > > > > +    action = ACTION_BEGIN_WRITE_OPERATION;
> > > > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > > > +
> > > > > > > +    action = ACTION_BEGIN_READ_OPERATION;
> > > > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > > > +
> > > > > > > +    action = ACTION_BEGIN_CLEAR_OPERATION;
> > > > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > > > +
> > > > > > > +    action = ACTION_END_OPERATION;
> > > > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > > > +
> > > > > > > +    action = ACTION_SET_RECORD_OFFSET;
> > > > > > > +    build_serialization_instruction(&wr_value_32, action, 0);
> > > > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > > > +
> > > > > > > +    action = ACTION_EXECUTE_OPERATION;
> > > > > > > +    build_serialization_instruction(&wr_value_32_val, action,
> > > > > > > +        ERST_EXECUTE_OPERATION_MAGIC);
> > > > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > > > +
> > > > > > > +    action = ACTION_CHECK_BUSY_STATUS;
> > > > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > > > +    build_serialization_instruction(&rd_value_32_val, action, 0x01);
> > > > > > > +
> > > > > > > +    action = ACTION_GET_COMMAND_STATUS;
> > > > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > > > +    build_serialization_instruction(&rd_value_32, action, 0);
> > > > > > > +
> > > > > > > +    action = ACTION_GET_RECORD_IDENTIFIER;
> > > > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > > > +    build_serialization_instruction(&rd_value_64, action, 0);
> > > > > > > +
> > > > > > > +    action = ACTION_SET_RECORD_IDENTIFIER;
> > > > > > > +    build_serialization_instruction(&wr_value_64, action, 0);
> > > > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > > > +
> > > > > > > +    action = ACTION_GET_RECORD_COUNT;
> > > > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > > > +    build_serialization_instruction(&rd_value_32, action, 0);
> > > > > > > +
> > > > > > > +    action = ACTION_BEGIN_DUMMY_WRITE_OPERATION;
> > > > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > > > +
> > > > > > > +    action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE;
> > > > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > > > +    build_serialization_instruction(&rd_value_64, action, 0);
> > > > > > > +
> > > > > > > +    action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH;
> > > > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > > > +    build_serialization_instruction(&rd_value_64, action, 0);
> > > > > > > +
> > > > > > > +    action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES;
> > > > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > > > +    build_serialization_instruction(&rd_value_32, action, 0);
> > > > > > > +
> > > > > > > +    action = ACTION_GET_EXECUTE_OPERATION_TIMINGS;
> > > > > > > +    build_serialization_instruction(&wr_action, action, action);
> > > > > > > +    build_serialization_instruction(&rd_value_64, action, 0);
> > > > > > > +
> > > > > > > +    /* Serialization Header */
> > > > > > > +    acpi_table_begin(&table, table_data);
> > > > > > > +
> > > > > > > +    /* Serialization Header Size */
> > > > > > > +    build_append_int_noprefix(table_data, 48, 4);
> > > > > > > +
> > > > > > > +    /* Reserved */
> > > > > > > +    build_append_int_noprefix(table_data,  0, 4);
> > > > > > > +
> > > > > > > +    /*
> > > > > > > +     * Instruction Entry Count
> > > > > > > +     * Each instruction entry is 32 bytes
> > > > > > > +     */
> > > > > > > +    g_assert((table_instruction_data->len) % 32 == 0);
> > > > > > > +    build_append_int_noprefix(table_data,
> > > > > > > +        (table_instruction_data->len / 32), 4);
> > > > > > > +
> > > > > > > +    /* Serialization Instruction Entries */
> > > > > > > +    g_array_append_vals(table_data, table_instruction_data->data,
> > > > > > > +        table_instruction_data->len);
> > > > > > > +    g_array_free(table_instruction_data, TRUE);
> > > > > > > +
> > > > > > > +    acpi_table_end(linker, &table);
> > > > > > > +}
> > > > > > > +
> > > > > > > +/*******************************************************************/
> > > > > > > +/*******************************************************************/
> > > > > > >     static uint8_t *get_nvram_ptr_by_index(ERSTDeviceState *s, unsigned index)
> > > > > > >     {
> > > > > > >         uint8_t *rc = NULL;
> > > > > > > --
> > > > > > > 1.8.3.1
> > > > > > > 
> > > > > > > 
> > > > 
> >
Re: [PATCH v14 06/10] ACPI ERST: build the ACPI ERST table
Posted by Ani Sinha 4 years ago 
On Fri, Jan 28, 2022 at 9:44 PM Michael S. Tsirkin <mst@redhat.com> wrote:

> On Fri, Jan 28, 2022 at 10:01:18AM -0600, Eric DeVolder wrote:
> > Michael, thanks! See inline response below, please.
> > eric
> >
> > On 1/28/22 09:54, Michael S. Tsirkin wrote:
> > > On Fri, Jan 28, 2022 at 09:11:41AM -0600, Eric DeVolder wrote:
> > > > Michael,
> > > > Thanks for examining this. Inline response below.
> > > > eric
> > > >
> > > > On 1/27/22 18:37, Michael S. Tsirkin wrote:
> > > > > On Thu, Jan 27, 2022 at 04:02:07PM -0600, Eric DeVolder wrote:
> > > > > > Ani,
> > > > > > Thanks for the RB! Inline responses below.
> > > > > > eric
> > > > > >
> > > > > > On 1/27/22 02:36, Ani Sinha wrote:
> > > > > > >
> > > > > > >
> > > > > > > On Wed, 26 Jan 2022, Eric DeVolder wrote:
> > > > > > >
> > > > > > > > This builds the ACPI ERST table to inform OSPM how to
> communicate
> > > > > > > > with the acpi-erst device.
> > > > > > >
> > > > > > > There might be more optimizations possible but I think we have
> messaged
> > > > > > > this code enough. We can further rework the code if needed in
> subsequent
> > > > > > > patches once this is pushed.
> > > > > > >
> > > > > > > >
> > > > > > > > Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> > > > > > >
> > > > > > > with some minor comments,
> > > > > > >
> > > > > > > Reviewed-by: Ani Sinha <ani@anisinha.ca>
> > > > > > >
> > > > > > > >     hw/acpi/erst.c | 225
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > >     1 file changed, 225 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
> > > > > > > > index fe9ba51..5d5a639 100644
> > > > > > > > --- a/hw/acpi/erst.c
> > > > > > > > +++ b/hw/acpi/erst.c
> > > > > > > > @@ -59,6 +59,27 @@
> > > > > > > >     #define STATUS_RECORD_STORE_EMPTY     0x04
> > > > > > > >     #define STATUS_RECORD_NOT_FOUND       0x05
> > > > > > > >
> > > > > > > > +/* ACPI 4.0: Table 17-19 Serialization Instructions */
> > > > > > > > +#define INST_READ_REGISTER                 0x00
> > > > > > > > +#define INST_READ_REGISTER_VALUE           0x01
> > > > > > > > +#define INST_WRITE_REGISTER                0x02
> > > > > > > > +#define INST_WRITE_REGISTER_VALUE          0x03
> > > > > > > > +#define INST_NOOP                          0x04
> > > > > > > > +#define INST_LOAD_VAR1                     0x05
> > > > > > > > +#define INST_LOAD_VAR2                     0x06
> > > > > > > > +#define INST_STORE_VAR1                    0x07
> > > > > > > > +#define INST_ADD                           0x08
> > > > > > > > +#define INST_SUBTRACT                      0x09
> > > > > > > > +#define INST_ADD_VALUE                     0x0A
> > > > > > > > +#define INST_SUBTRACT_VALUE                0x0B
> > > > > > > > +#define INST_STALL                         0x0C
> > > > > > > > +#define INST_STALL_WHILE_TRUE              0x0D
> > > > > > > > +#define INST_SKIP_NEXT_INSTRUCTION_IF_TRUE 0x0E
> > > > > > > > +#define INST_GOTO                          0x0F
> > > > > > > > +#define INST_SET_SRC_ADDRESS_BASE          0x10
> > > > > > > > +#define INST_SET_DST_ADDRESS_BASE          0x11
> > > > > > > > +#define INST_MOVE_DATA                     0x12
> > > > > > > > +
> > > > > > > >     /* UEFI 2.1: Appendix N Common Platform Error Record */
> > > > > > > >     #define UEFI_CPER_RECORD_MIN_SIZE 128U
> > > > > > > >     #define UEFI_CPER_RECORD_LENGTH_OFFSET 20U
> > > > > > > > @@ -172,6 +193,210 @@ typedef struct {
> > > > > > > >
> > > > > > > >
>  /*******************************************************************/
> > > > > > > >
>  /*******************************************************************/
> > > > > > > > +typedef struct {
> > > > > > > > +    GArray *table_data;
> > > > > > > > +    pcibus_t bar;
> > > > > > > > +    uint8_t instruction;
> > > > > > > > +    uint8_t flags;
> > > > > > > > +    uint8_t register_bit_width;
> > > > > > > > +    pcibus_t register_offset;
> > > > > > > > +} BuildSerializationInstructionEntry;
> > > > > > > > +
> > > > > > > > +/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */
> > > > > > > > +static void build_serialization_instruction(
> > > > > > > > +    BuildSerializationInstructionEntry *e,
> > > > > > > > +    uint8_t serialization_action,
> > > > > > > > +    uint64_t value)
> > > > > > > > +{
> > > > > > > > +    /* ACPI 4.0: Table 17-18 Serialization Instruction
> Entry */
> > > > > > > > +    struct AcpiGenericAddress gas;
> > > > > > > > +    uint64_t mask;
> > > > > > > > +
> > > > > > > > +    /* Serialization Action */
> > > > > > > > +    build_append_int_noprefix(e->table_data,
> serialization_action, 1);
> > > > > > > > +    /* Instruction */
> > > > > > > > +    build_append_int_noprefix(e->table_data,
> e->instruction, 1);
> > > > > > > > +    /* Flags */
> > > > > > > > +    build_append_int_noprefix(e->table_data, e->flags, 1);
> > > > > > > > +    /* Reserved */
> > > > > > > > +    build_append_int_noprefix(e->table_data, 0, 1);
> > > > > > > > +    /* Register Region */
> > > > > > > > +    gas.space_id = AML_SYSTEM_MEMORY;
> > > > > > > > +    gas.bit_width = e->register_bit_width;
> > > > > > > > +    gas.bit_offset = 0;
> > > > > > > > +    gas.access_width = ctz32(e->register_bit_width) - 2;
> > > > > > >
> > > > > > > Should this be casted as unit8_t?
> > > > > > OK, done.
> > > > > >
> > > > > > >
> > > > > > > > +    gas.address = (uint64_t)(e->bar + e->register_offset);
> > > > > > > > +    build_append_gas_from_struct(e->table_data, &gas);
> > > > > > > > +    /* Value */
> > > > > > > > +    build_append_int_noprefix(e->table_data, value, 8);
> > > > > > > > +    /* Mask */
> > > > > > > > +    mask = (1ULL << (e->register_bit_width - 1) << 1) - 1;
> > > > > > > > +    build_append_int_noprefix(e->table_data, mask, 8);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +/* ACPI 4.0: 17.4.1 Serialization Action Table */
> > > > > > > > +void build_erst(GArray *table_data, BIOSLinker *linker,
> Object *erst_dev,
> > > > > > > > +    const char *oem_id, const char *oem_table_id)
> > > > > > > > +{
> > > > > > > > +    /*
> > > > > > > > +     * Serialization Action Table
> > > > > > > > +     * The serialization action table must be generated
> first
> > > > > > > > +     * so that its size can be known in order to populate
> the
> > > > > > > > +     * Instruction Entry Count field.
> > > > > > > > +     */
> > > > > > > > +    GArray *table_instruction_data = g_array_new(FALSE,
> FALSE, sizeof(char));
> > > > > > > > +    pcibus_t bar0 = pci_get_bar_addr(PCI_DEVICE(erst_dev),
> 0);
> > > > > > > > +    AcpiTable table = { .sig = "ERST", .rev = 1, .oem_id =
> oem_id,
> > > > > > > > +                        .oem_table_id = oem_table_id };
> > > > > > > > +    /* Contexts for the different ways ACTION and VALUE are
> accessed */
> > > > > > > > +    BuildSerializationInstructionEntry rd_value_32_val = {
> > > > > > > > +        .table_data = table_instruction_data,
> > > > > > > > +        .bar = bar0,
> > > > > > > > +        .instruction = INST_READ_REGISTER_VALUE,
> > > > > > > > +        .flags = 0,
> > > > > > > > +        .register_bit_width = 32,
> > > > > > > > +        .register_offset = ERST_VALUE_OFFSET,
> > > > > > > > +    };
> > > > > > > > +    BuildSerializationInstructionEntry rd_value_32 = {
> > > > > > > > +        .table_data = table_instruction_data,
> > > > > > > > +        .bar = bar0,
> > > > > > > > +        .instruction = INST_READ_REGISTER,
> > > > > > > > +        .flags = 0,
> > > > > > > > +        .register_bit_width = 32,
> > > > > > > > +        .register_offset = ERST_VALUE_OFFSET,
> > > > > > > > +    };
> > > > > > > > +    BuildSerializationInstructionEntry rd_value_64 = {
> > > > > > > > +        .table_data = table_instruction_data,
> > > > > > > > +        .bar = bar0,
> > > > > > > > +        .instruction = INST_READ_REGISTER,
> > > > > > > > +        .flags = 0,
> > > > > > > > +        .register_bit_width = 64,
> > > > > > > > +        .register_offset = ERST_VALUE_OFFSET,
> > > > > > > > +    };
> > > > > > > > +    BuildSerializationInstructionEntry wr_value_32_val = {
> > > > > > > > +        .table_data = table_instruction_data,
> > > > > > > > +        .bar = bar0,
> > > > > > > > +        .instruction = INST_WRITE_REGISTER_VALUE,
> > > > > > > > +        .flags = 0,
> > > > > > > > +        .register_bit_width = 32,
> > > > > > > > +        .register_offset = ERST_VALUE_OFFSET,
> > > > > > > > +    };
> > > > > > > > +    BuildSerializationInstructionEntry wr_value_32 = {
> > > > > > > > +        .table_data = table_instruction_data,
> > > > > > > > +        .bar = bar0,
> > > > > > > > +        .instruction = INST_WRITE_REGISTER,
> > > > > > > > +        .flags = 0,
> > > > > > > > +        .register_bit_width = 32,
> > > > > > > > +        .register_offset = ERST_VALUE_OFFSET,
> > > > > > > > +    };
> > > > > > > > +    BuildSerializationInstructionEntry wr_value_64 = {
> > > > > > > > +        .table_data = table_instruction_data,
> > > > > > > > +        .bar = bar0,
> > > > > > > > +        .instruction = INST_WRITE_REGISTER,
> > > > > > > > +        .flags = 0,
> > > > > > > > +        .register_bit_width = 64,
> > > > > > > > +        .register_offset = ERST_VALUE_OFFSET,
> > > > > > > > +    };
> > > > > > > > +    BuildSerializationInstructionEntry wr_action = {
> > > > > > > > +        .table_data = table_instruction_data,
> > > > > > > > +        .bar = bar0,
> > > > > > > > +        .instruction = INST_WRITE_REGISTER_VALUE,
> > > > > > > > +        .flags = 0,
> > > > > > > > +        .register_bit_width = 32,
> > > > > > > > +        .register_offset = ERST_ACTION_OFFSET,
> > > > > > > > +    };
> > > > > > >
> > > > > > > We can probably write a macro to simply generating these
> structs. I see
> > > > > > > .bar and .flags are the same in all structs. The .bit_width
> can be a param
> > > > > > > into the macro etc.
> > > > > > Agree, however the request was to NOT hide the use of local
> variables in
> > > > > > macros, so while .flags is always 0, .instruction,
> .register_bit_width and .register_offset
> > > > > > would be parameters, that leaves .table_data and .bar which just
> need the local
> > > > > > variables. Is the following acceptable (which hides the use of
> the local variables)?
> > > > >
> > > > > You can just use assignment:
> > > > >
> > > > >      BuildSerializationInstructionEntry entry = {
> > > > >          .table_data = table_instruction_data,
> > > > >          .bar = bar0,
> > > > >          .flags = 0,
> > > > >          .register_bit_width = 32,
> > > > >      };
> > > > >      BuildSerializationInstructionEntry rd_value_32_val = entry;
> > > > >      rd_value_32_val.action = INST_READ_REGISTER_VALUE;
> > > > >      rd_value_32_val.register_offset = ERST_ACTION_OFFSET;
> > > > >
> > > > > and so on.
> > > > > not sure it's worth it, it's just a couple of lines.
> > > > >
> > > >
> > > > OK, here is the equivalent using struct assignment, is this what you
> were after?
> > > >
> > > >      BuildSerializationInstructionEntry base = {
> > > >          .table_data = table_instruction_data,
> > > >          .bar = bar0,
> > > >          .instruction = INST_WRITE_REGISTER,
> > > >          .flags = 0,
> > > >          .register_bit_width = 32,
> > > >          .register_offset = ERST_VALUE_OFFSET,
> > > >      };
> > > >      BuildSerializationInstructionEntry rd_value_32_val = base;
> > > >      rd_value_32_val.instruction = INST_READ_REGISTER_VALUE;
> > > >      BuildSerializationInstructionEntry rd_value_32 = base;
> > > >      rd_value_32.instruction = INST_READ_REGISTER;
> > > >      BuildSerializationInstructionEntry rd_value_64 = base;
> > > >      rd_value_64.instruction = INST_READ_REGISTER;
> > > >      rd_value_64.register_bit_width = 64;
> > > >      BuildSerializationInstructionEntry wr_value_32_val = base;
> > > >      wr_value_32_val.instruction = INST_WRITE_REGISTER_VALUE;
> > > >      BuildSerializationInstructionEntry wr_value_32 = base;
> > > >      BuildSerializationInstructionEntry wr_value_64 = base;
> > > >      wr_value_64.register_bit_width = 64;
> > > >      BuildSerializationInstructionEntry wr_action = base;
> > > >      wr_action.instruction = INST_WRITE_REGISTER_VALUE;
> > > >      wr_action.register_offset = ERST_ACTION_OFFSET;
> > > >
> > >
> > > That's what I described, yes. We should have some empty lines here I
> > > guess. I'm ok with the original one too, there's not too much
> > > duplication.
> >
> > Are the blank lines referring to spacing out the setup of each of the 7
> accesors?
> > If so, I could put a one line comment between each setup? Or is a blank
> line also
> > needed?
>
> A blank line between declarations and code is usually a good idea.
>
>
> > Is it OK to post v15 with the struct assignment approach? Or would you
> prefer the
> > explicit structs (which is what I think you mean by 'the original one')?


I prefer the explicit structs as you had posted before.


> >
> > Thanks!
> > eric
>
> I don't care either way.
>
> > >
> > >
> > > >
> > > > >
> > > > >
> > > > > > #define SERIALIZATIONINSTRUCTIONCTX(name, \
> > > > > >       inst, bit_width, offset) \
> > > > > >       BuildSerializationInstructionEntry name = { \
> > > > > >           .table_data = table_instruction_data, \
> > > > > >           .bar = bar0, \
> > > > > >           .instruction = inst, \
> > > > > >           .flags = 0, \
> > > > > >           .register_bit_width = bit_width, \
> > > > > >           .register_offset = offset, \
> > > > > >       }
> > > > > >       SERIALIZATIONINSTRUCTIONCTX(rd_value_32_val,
> > > > > >           INST_READ_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
> > > > > >       SERIALIZATIONINSTRUCTIONCTX(rd_value_32,
> > > > > >           INST_READ_REGISTER, 32, ERST_VALUE_OFFSET);
> > > > > >       SERIALIZATIONINSTRUCTIONCTX(rd_value_64,
> > > > > >           INST_READ_REGISTER, 64, ERST_VALUE_OFFSET);
> > > > > >       SERIALIZATIONINSTRUCTIONCTX(wr_value_32_val,
> > > > > >           INST_WRITE_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
> > > > > >       SERIALIZATIONINSTRUCTIONCTX(wr_value_32,
> > > > > >           INST_WRITE_REGISTER, 32, ERST_VALUE_OFFSET);
> > > > > >       SERIALIZATIONINSTRUCTIONCTX(wr_value_64,
> > > > > >           INST_WRITE_REGISTER, 64, ERST_VALUE_OFFSET);
> > > > > >       SERIALIZATIONINSTRUCTIONCTX(wr_action,
> > > > > >           INST_WRITE_REGISTER_VALUE, 32, ERST_ACTION_OFFSET);
> > > > > >
> > > > > > These are the 7 accessors needed.
> > > > >
> > > > > not at all sure this one is worth the macro mess.
> > > >
> > > > I'm hoping to produce a v15 with the style you want.
> > > > eric
> > > >
> > > > >
> > > > > > >
> > > > > > > > +    unsigned action;
> > > > > > > > +
> > > > > > > > +    trace_acpi_erst_pci_bar_0(bar0);
> > > > > > > > +
> > > > > > > > +    /* Serialization Instruction Entries */
> > > > > > > > +    action = ACTION_BEGIN_WRITE_OPERATION;
> > > > > > > > +    build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > +
> > > > > > > > +    action = ACTION_BEGIN_READ_OPERATION;
> > > > > > > > +    build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > +
> > > > > > > > +    action = ACTION_BEGIN_CLEAR_OPERATION;
> > > > > > > > +    build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > +
> > > > > > > > +    action = ACTION_END_OPERATION;
> > > > > > > > +    build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > +
> > > > > > > > +    action = ACTION_SET_RECORD_OFFSET;
> > > > > > > > +    build_serialization_instruction(&wr_value_32, action,
> 0);
> > > > > > > > +    build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > +
> > > > > > > > +    action = ACTION_EXECUTE_OPERATION;
> > > > > > > > +    build_serialization_instruction(&wr_value_32_val,
> action,
> > > > > > > > +        ERST_EXECUTE_OPERATION_MAGIC);
> > > > > > > > +    build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > +
> > > > > > > > +    action = ACTION_CHECK_BUSY_STATUS;
> > > > > > > > +    build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > +    build_serialization_instruction(&rd_value_32_val,
> action, 0x01);
> > > > > > > > +
> > > > > > > > +    action = ACTION_GET_COMMAND_STATUS;
> > > > > > > > +    build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > +    build_serialization_instruction(&rd_value_32, action,
> 0);
> > > > > > > > +
> > > > > > > > +    action = ACTION_GET_RECORD_IDENTIFIER;
> > > > > > > > +    build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > +    build_serialization_instruction(&rd_value_64, action,
> 0);
> > > > > > > > +
> > > > > > > > +    action = ACTION_SET_RECORD_IDENTIFIER;
> > > > > > > > +    build_serialization_instruction(&wr_value_64, action,
> 0);
> > > > > > > > +    build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > +
> > > > > > > > +    action = ACTION_GET_RECORD_COUNT;
> > > > > > > > +    build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > +    build_serialization_instruction(&rd_value_32, action,
> 0);
> > > > > > > > +
> > > > > > > > +    action = ACTION_BEGIN_DUMMY_WRITE_OPERATION;
> > > > > > > > +    build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > +
> > > > > > > > +    action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE;
> > > > > > > > +    build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > +    build_serialization_instruction(&rd_value_64, action,
> 0);
> > > > > > > > +
> > > > > > > > +    action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH;
> > > > > > > > +    build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > +    build_serialization_instruction(&rd_value_64, action,
> 0);
> > > > > > > > +
> > > > > > > > +    action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES;
> > > > > > > > +    build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > +    build_serialization_instruction(&rd_value_32, action,
> 0);
> > > > > > > > +
> > > > > > > > +    action = ACTION_GET_EXECUTE_OPERATION_TIMINGS;
> > > > > > > > +    build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > +    build_serialization_instruction(&rd_value_64, action,
> 0);
> > > > > > > > +
> > > > > > > > +    /* Serialization Header */
> > > > > > > > +    acpi_table_begin(&table, table_data);
> > > > > > > > +
> > > > > > > > +    /* Serialization Header Size */
> > > > > > > > +    build_append_int_noprefix(table_data, 48, 4);
> > > > > > > > +
> > > > > > > > +    /* Reserved */
> > > > > > > > +    build_append_int_noprefix(table_data,  0, 4);
> > > > > > > > +
> > > > > > > > +    /*
> > > > > > > > +     * Instruction Entry Count
> > > > > > > > +     * Each instruction entry is 32 bytes
> > > > > > > > +     */
> > > > > > > > +    g_assert((table_instruction_data->len) % 32 == 0);
> > > > > > > > +    build_append_int_noprefix(table_data,
> > > > > > > > +        (table_instruction_data->len / 32), 4);
> > > > > > > > +
> > > > > > > > +    /* Serialization Instruction Entries */
> > > > > > > > +    g_array_append_vals(table_data,
> table_instruction_data->data,
> > > > > > > > +        table_instruction_data->len);
> > > > > > > > +    g_array_free(table_instruction_data, TRUE);
> > > > > > > > +
> > > > > > > > +    acpi_table_end(linker, &table);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >
> +/*******************************************************************/
> > > > > > > >
> +/*******************************************************************/
> > > > > > > >     static uint8_t *get_nvram_ptr_by_index(ERSTDeviceState
> *s, unsigned index)
> > > > > > > >     {
> > > > > > > >         uint8_t *rc = NULL;
> > > > > > > > --
> > > > > > > > 1.8.3.1
> > > > > > > >
> > > > > > > >
> > > > >
> > >
>
>
Re: [PATCH v14 06/10] ACPI ERST: build the ACPI ERST table
Posted by Eric DeVolder 4 years ago 

On 1/28/22 11:25, Ani Sinha wrote:
> 
> [snip]
> On Fri, Jan 28, 2022 at 9:44 PM Michael S. Tsirkin <mst@redhat.com <mailto:mst@redhat.com>> wrote:
> 

>      > > > OK, here is the equivalent using struct assignment, is this what you were after?
>      > > >
>      > > >      BuildSerializationInstructionEntry base = {
>      > > >          .table_data = table_instruction_data,
>      > > >          .bar = bar0,
>      > > >          .instruction = INST_WRITE_REGISTER,
>      > > >          .flags = 0,
>      > > >          .register_bit_width = 32,
>      > > >          .register_offset = ERST_VALUE_OFFSET,
>      > > >      };
>      > > >      BuildSerializationInstructionEntry rd_value_32_val = base;
>      > > >      rd_value_32_val.instruction = INST_READ_REGISTER_VALUE;
>      > > >      BuildSerializationInstructionEntry rd_value_32 = base;
>      > > >      rd_value_32.instruction = INST_READ_REGISTER;
>      > > >      BuildSerializationInstructionEntry rd_value_64 = base;
>      > > >      rd_value_64.instruction = INST_READ_REGISTER;
>      > > >      rd_value_64.register_bit_width = 64;
>      > > >      BuildSerializationInstructionEntry wr_value_32_val = base;
>      > > >      wr_value_32_val.instruction = INST_WRITE_REGISTER_VALUE;
>      > > >      BuildSerializationInstructionEntry wr_value_32 = base;
>      > > >      BuildSerializationInstructionEntry wr_value_64 = base;
>      > > >      wr_value_64.register_bit_width = 64;
>      > > >      BuildSerializationInstructionEntry wr_action = base;
>      > > >      wr_action.instruction = INST_WRITE_REGISTER_VALUE;
>      > > >      wr_action.register_offset = ERST_ACTION_OFFSET;
>      > > >
>      > >
>      > > That's what I described, yes. We should have some empty lines here I
>      > > guess. I'm ok with the original one too, there's not too much
>      > > duplication.
>      >
>      > Are the blank lines referring to spacing out the setup of each of the 7 accesors?
>      > If so, I could put a one line comment between each setup? Or is a blank line also
>      > needed?
> 
>     A blank line between declarations and code is usually a good idea.
> 
> 
>      > Is it OK to post v15 with the struct assignment approach? Or would you prefer the
>      > explicit structs (which is what I think you mean by 'the original one')?
> 
> 
> I prefer the explicit structs as you had posted before.

Ok, as Michael does not have a preference, so let's go with your preference of the
explicit structs!
Thank you!
eric

> 
> 
>      >
>      > Thanks!
>      > eric
> 
>     I don't care either way.
> 
>      > >
>      > >
>      > > >
>      > > > >
>      > > > >
>      > > > > > #define SERIALIZATIONINSTRUCTIONCTX(name, \
>      > > > > >       inst, bit_width, offset) \
>      > > > > >       BuildSerializationInstructionEntry name = { \
>      > > > > >           .table_data = table_instruction_data, \
>      > > > > >           .bar = bar0, \
>      > > > > >           .instruction = inst, \
>      > > > > >           .flags = 0, \
>      > > > > >           .register_bit_width = bit_width, \
>      > > > > >           .register_offset = offset, \
>      > > > > >       }
>      > > > > >       SERIALIZATIONINSTRUCTIONCTX(rd_value_32_val,
>      > > > > >           INST_READ_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
>      > > > > >       SERIALIZATIONINSTRUCTIONCTX(rd_value_32,
>      > > > > >           INST_READ_REGISTER, 32, ERST_VALUE_OFFSET);
>      > > > > >       SERIALIZATIONINSTRUCTIONCTX(rd_value_64,
>      > > > > >           INST_READ_REGISTER, 64, ERST_VALUE_OFFSET);
>      > > > > >       SERIALIZATIONINSTRUCTIONCTX(wr_value_32_val,
>      > > > > >           INST_WRITE_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
>      > > > > >       SERIALIZATIONINSTRUCTIONCTX(wr_value_32,
>      > > > > >           INST_WRITE_REGISTER, 32, ERST_VALUE_OFFSET);
>      > > > > >       SERIALIZATIONINSTRUCTIONCTX(wr_value_64,
>      > > > > >           INST_WRITE_REGISTER, 64, ERST_VALUE_OFFSET);
>      > > > > >       SERIALIZATIONINSTRUCTIONCTX(wr_action,
>      > > > > >           INST_WRITE_REGISTER_VALUE, 32, ERST_ACTION_OFFSET);
>      > > > > >
>      > > > > > These are the 7 accessors needed.
>      > > > >
>      > > > > not at all sure this one is worth the macro mess.
>      > > >
>      > > > I'm hoping to produce a v15 with the style you want.
>      > > > eric
>      > > >
>      > > > >
>      > > > > > >
>      > > > > > > > +    unsigned action;
>      > > > > > > > +
>      > > > > > > > +    trace_acpi_erst_pci_bar_0(bar0);
>      > > > > > > > +
>      > > > > > > > +    /* Serialization Instruction Entries */
>      > > > > > > > +    action = ACTION_BEGIN_WRITE_OPERATION;
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_BEGIN_READ_OPERATION;
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_BEGIN_CLEAR_OPERATION;
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_END_OPERATION;
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_SET_RECORD_OFFSET;
>      > > > > > > > +    build_serialization_instruction(&wr_value_32, action, 0);
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_EXECUTE_OPERATION;
>      > > > > > > > +    build_serialization_instruction(&wr_value_32_val, action,
>      > > > > > > > +        ERST_EXECUTE_OPERATION_MAGIC);
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_CHECK_BUSY_STATUS;
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +    build_serialization_instruction(&rd_value_32_val, action, 0x01);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_GET_COMMAND_STATUS;
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +    build_serialization_instruction(&rd_value_32, action, 0);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_GET_RECORD_IDENTIFIER;
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +    build_serialization_instruction(&rd_value_64, action, 0);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_SET_RECORD_IDENTIFIER;
>      > > > > > > > +    build_serialization_instruction(&wr_value_64, action, 0);
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_GET_RECORD_COUNT;
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +    build_serialization_instruction(&rd_value_32, action, 0);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_BEGIN_DUMMY_WRITE_OPERATION;
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE;
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +    build_serialization_instruction(&rd_value_64, action, 0);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH;
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +    build_serialization_instruction(&rd_value_64, action, 0);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES;
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +    build_serialization_instruction(&rd_value_32, action, 0);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_GET_EXECUTE_OPERATION_TIMINGS;
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +    build_serialization_instruction(&rd_value_64, action, 0);
>      > > > > > > > +
>      > > > > > > > +    /* Serialization Header */
>      > > > > > > > +    acpi_table_begin(&table, table_data);
>      > > > > > > > +
>      > > > > > > > +    /* Serialization Header Size */
>      > > > > > > > +    build_append_int_noprefix(table_data, 48, 4);
>      > > > > > > > +
>      > > > > > > > +    /* Reserved */
>      > > > > > > > +    build_append_int_noprefix(table_data,  0, 4);
>      > > > > > > > +
>      > > > > > > > +    /*
>      > > > > > > > +     * Instruction Entry Count
>      > > > > > > > +     * Each instruction entry is 32 bytes
>      > > > > > > > +     */
>      > > > > > > > +    g_assert((table_instruction_data->len) % 32 == 0);
>      > > > > > > > +    build_append_int_noprefix(table_data,
>      > > > > > > > +        (table_instruction_data->len / 32), 4);
>      > > > > > > > +
>      > > > > > > > +    /* Serialization Instruction Entries */
>      > > > > > > > +    g_array_append_vals(table_data, table_instruction_data->data,
>      > > > > > > > +        table_instruction_data->len);
>      > > > > > > > +    g_array_free(table_instruction_data, TRUE);
>      > > > > > > > +
>      > > > > > > > +    acpi_table_end(linker, &table);
>      > > > > > > > +}
>      > > > > > > > +
>      > > > > > > > +/*******************************************************************/
>      > > > > > > > +/*******************************************************************/
>      > > > > > > >     static uint8_t *get_nvram_ptr_by_index(ERSTDeviceState *s, unsigned index)
>      > > > > > > >     {
>      > > > > > > >         uint8_t *rc = NULL;
>      > > > > > > > --
>      > > > > > > > 1.8.3.1
>      > > > > > > >
>      > > > > > > >
>      > > > >
>      > >
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.