| Message ID | 1643214514-2839-7-git-send-email-eric.devolder@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | acpi: Error Record Serialization Table, ERST, support for QEMU | expand |
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 > >
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 >> >>
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 > > > > > >
> > #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.
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 >>>> >>>> >
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 > > > > > > > > > > > >
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 >>>>>> >>>>>> >>> >
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 > > > > > > > > > > > > > > > > > > > >
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 > > > > > > > > > > > > > > > > > > > > > > > > > >
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 > > > > > > > > > > > > > > > > > > > > > > > > >
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;
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(+)