| Message ID | 1643044621-15892-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 Mon, 24 Jan 2022, Eric DeVolder wrote: > 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 | 188 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 188 insertions(+) > > diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c > index fe9ba51..b0c7539 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,173 @@ typedef struct { > > /*******************************************************************/ > /*******************************************************************/ > + > +/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */ > +static void build_serialization_instruction_entry(GArray *table_data, > + uint8_t serialization_action, > + uint8_t instruction, > + uint8_t flags, > + uint8_t register_bit_width, > + uint64_t register_address, > + uint64_t value) > +{ > + /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */ > + struct AcpiGenericAddress gas; > + uint64_t mask; > + > + /* Serialization Action */ > + build_append_int_noprefix(table_data, serialization_action, 1); > + /* Instruction */ > + build_append_int_noprefix(table_data, instruction , 1); > + /* Flags */ > + build_append_int_noprefix(table_data, flags , 1); > + /* Reserved */ > + build_append_int_noprefix(table_data, 0 , 1); > + /* Register Region */ > + gas.space_id = AML_SYSTEM_MEMORY; > + gas.bit_width = register_bit_width; > + gas.bit_offset = 0; > + gas.access_width = ctz32(register_bit_width) - 2; > + gas.address = register_address; > + build_append_gas_from_struct(table_data, &gas); > + /* Value */ > + build_append_int_noprefix(table_data, value , 8); > + /* Mask */ > + mask = (1ULL << (register_bit_width - 1) << 1) - 1; > + build_append_int_noprefix(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) > +{ > + GArray *table_instruction_data; > + unsigned action; > + 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 }; > + > + trace_acpi_erst_pci_bar_0(bar0); > + > + /* > + * 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. > + */ > + table_instruction_data = g_array_new(FALSE, FALSE, sizeof(char)); > + > + /* > + * Macros for use with construction of the action instructions > + */ > +#define BUILD_READ_REGISTER(width_in_bits, reg) \ > + build_serialization_instruction_entry(table_instruction_data, \ > + action, INST_READ_REGISTER, 0, width_in_bits, \ > + bar0 + reg, 0) > + > +#define BUILD_READ_REGISTER_VALUE(width_in_bits, reg, value) \ > + build_serialization_instruction_entry(table_instruction_data, \ > + action, INST_READ_REGISTER_VALUE, 0, width_in_bits, \ > + bar0 + reg, value) > + > +#define BUILD_WRITE_REGISTER(width_in_bits, reg, value) \ > + build_serialization_instruction_entry(table_instruction_data, \ > + action, INST_WRITE_REGISTER, 0, width_in_bits, \ > + bar0 + reg, value) > + > +#define BUILD_WRITE_REGISTER_VALUE(width_in_bits, reg, value) \ > + build_serialization_instruction_entry(table_instruction_data, \ > + action, INST_WRITE_REGISTER_VALUE, 0, width_in_bits, \ > + bar0 + reg, value) > + > + /* Serialization Instruction Entries */ > + action = ACTION_BEGIN_WRITE_OPERATION; > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > + > + action = ACTION_BEGIN_READ_OPERATION; > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > + > + action = ACTION_BEGIN_CLEAR_OPERATION; > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > + > + action = ACTION_END_OPERATION; > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > + > + action = ACTION_SET_RECORD_OFFSET; > + BUILD_WRITE_REGISTER(32, ERST_VALUE_OFFSET, 0); > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > + > + action = ACTION_EXECUTE_OPERATION; > + BUILD_WRITE_REGISTER_VALUE(32, ERST_VALUE_OFFSET, > + ERST_EXECUTE_OPERATION_MAGIC); except here, on all cases we have BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); We should treat the above as special case and simplify the rest of the calls (eliminate repeated common arguments). > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > + > + action = ACTION_CHECK_BUSY_STATUS; > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > + BUILD_READ_REGISTER_VALUE(32, ERST_VALUE_OFFSET, 0x01); > + > + action = ACTION_GET_COMMAND_STATUS; > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET); > + > + action = ACTION_GET_RECORD_IDENTIFIER; > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); > + > + action = ACTION_SET_RECORD_IDENTIFIER; > + BUILD_WRITE_REGISTER(64, ERST_VALUE_OFFSET, 0); > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); This one seems reverted. Should this be BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); BUILD_WRITE_REGISTER(64, ERST_VALUE_OFFSET, 0); like others? > + > + action = ACTION_GET_RECORD_COUNT; > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET); > + > + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION; > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > + > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE; > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); > + > + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH; > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); > + > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES; > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET); > + > + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS; > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); > + BUILD_READ_REGISTER() is always called with ERST_VALUE_OFFSET as second argument. WE should eliminate this repeated passing of same argument. > + /* 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 Tue, Jan 25, 2022 at 04:23:49PM +0530, Ani Sinha wrote: > > > On Mon, 24 Jan 2022, Eric DeVolder wrote: > > > 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 | 188 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 188 insertions(+) > > > > diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c > > index fe9ba51..b0c7539 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,173 @@ typedef struct { > > > > /*******************************************************************/ > > /*******************************************************************/ > > + > > +/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */ > > +static void build_serialization_instruction_entry(GArray *table_data, > > + uint8_t serialization_action, > > + uint8_t instruction, > > + uint8_t flags, > > + uint8_t register_bit_width, > > + uint64_t register_address, > > + uint64_t value) > > +{ > > + /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */ > > + struct AcpiGenericAddress gas; > > + uint64_t mask; > > + > > + /* Serialization Action */ > > + build_append_int_noprefix(table_data, serialization_action, 1); > > + /* Instruction */ > > + build_append_int_noprefix(table_data, instruction , 1); > > + /* Flags */ > > + build_append_int_noprefix(table_data, flags , 1); > > + /* Reserved */ > > + build_append_int_noprefix(table_data, 0 , 1); > > + /* Register Region */ > > + gas.space_id = AML_SYSTEM_MEMORY; > > + gas.bit_width = register_bit_width; > > + gas.bit_offset = 0; > > + gas.access_width = ctz32(register_bit_width) - 2; > > + gas.address = register_address; > > + build_append_gas_from_struct(table_data, &gas); > > + /* Value */ > > + build_append_int_noprefix(table_data, value , 8); > > + /* Mask */ > > + mask = (1ULL << (register_bit_width - 1) << 1) - 1; > > + build_append_int_noprefix(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) > > +{ > > + GArray *table_instruction_data; > > + unsigned action; > > + 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 }; > > + > > + trace_acpi_erst_pci_bar_0(bar0); > > + > > + /* > > + * 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. > > + */ > > + table_instruction_data = g_array_new(FALSE, FALSE, sizeof(char)); > > + > > + /* > > + * Macros for use with construction of the action instructions > > + */ > > +#define BUILD_READ_REGISTER(width_in_bits, reg) \ > > + build_serialization_instruction_entry(table_instruction_data, \ > > + action, INST_READ_REGISTER, 0, width_in_bits, \ > > + bar0 + reg, 0) > > + > > +#define BUILD_READ_REGISTER_VALUE(width_in_bits, reg, value) \ > > + build_serialization_instruction_entry(table_instruction_data, \ > > + action, INST_READ_REGISTER_VALUE, 0, width_in_bits, \ > > + bar0 + reg, value) > > + > > +#define BUILD_WRITE_REGISTER(width_in_bits, reg, value) \ > > + build_serialization_instruction_entry(table_instruction_data, \ > > + action, INST_WRITE_REGISTER, 0, width_in_bits, \ > > + bar0 + reg, value) > > + > > +#define BUILD_WRITE_REGISTER_VALUE(width_in_bits, reg, value) \ > > + build_serialization_instruction_entry(table_instruction_data, \ > > + action, INST_WRITE_REGISTER_VALUE, 0, width_in_bits, \ > > + bar0 + reg, value) I think these macros which in a hidden way use the bar0 variable really should be replaced with inline functions, improving type safety. > > + > > + /* Serialization Instruction Entries */ > > + action = ACTION_BEGIN_WRITE_OPERATION; > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > + > > + action = ACTION_BEGIN_READ_OPERATION; > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > + > > + action = ACTION_BEGIN_CLEAR_OPERATION; > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > + > > + action = ACTION_END_OPERATION; > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > + > > + action = ACTION_SET_RECORD_OFFSET; > > + BUILD_WRITE_REGISTER(32, ERST_VALUE_OFFSET, 0); > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > + > > + action = ACTION_EXECUTE_OPERATION; > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_VALUE_OFFSET, > > + ERST_EXECUTE_OPERATION_MAGIC); > > except here, on all cases we have > BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > We should treat the above as special case and simplify the rest of the > calls (eliminate repeated common arguments). > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > + > > + action = ACTION_CHECK_BUSY_STATUS; > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > + BUILD_READ_REGISTER_VALUE(32, ERST_VALUE_OFFSET, 0x01); > > + > > + action = ACTION_GET_COMMAND_STATUS; > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET); > > + > > + action = ACTION_GET_RECORD_IDENTIFIER; > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); > > + > > + action = ACTION_SET_RECORD_IDENTIFIER; > > + BUILD_WRITE_REGISTER(64, ERST_VALUE_OFFSET, 0); > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > This one seems reverted. Should this be > BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > BUILD_WRITE_REGISTER(64, ERST_VALUE_OFFSET, 0); > > like others? > > > + > > + action = ACTION_GET_RECORD_COUNT; > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET); > > + > > + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION; > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > + > > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE; > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); > > + > > + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH; > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); > > + > > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES; > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET); > > + > > + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS; > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); > > + > > BUILD_READ_REGISTER() is always called with ERST_VALUE_OFFSET as second > argument. WE should eliminate this repeated passing of same argument. > > > > + /* 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 > > > >
> > I think these macros which in a hidden way use the bar0 variable really > should be replaced with inline functions, improving type safety. > I second that.
Ani, Thanks for the feedback! Inline responses below. eric On 1/25/22 04:53, Ani Sinha wrote: > > > On Mon, 24 Jan 2022, Eric DeVolder wrote: > >> 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 | 188 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 188 insertions(+) >> >> diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c >> index fe9ba51..b0c7539 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,173 @@ typedef struct { >> >> /*******************************************************************/ >> /*******************************************************************/ >> + >> +/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */ >> +static void build_serialization_instruction_entry(GArray *table_data, >> + uint8_t serialization_action, >> + uint8_t instruction, >> + uint8_t flags, >> + uint8_t register_bit_width, >> + uint64_t register_address, >> + uint64_t value) >> +{ >> + /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */ >> + struct AcpiGenericAddress gas; >> + uint64_t mask; >> + >> + /* Serialization Action */ >> + build_append_int_noprefix(table_data, serialization_action, 1); >> + /* Instruction */ >> + build_append_int_noprefix(table_data, instruction , 1); >> + /* Flags */ >> + build_append_int_noprefix(table_data, flags , 1); >> + /* Reserved */ >> + build_append_int_noprefix(table_data, 0 , 1); >> + /* Register Region */ >> + gas.space_id = AML_SYSTEM_MEMORY; >> + gas.bit_width = register_bit_width; >> + gas.bit_offset = 0; >> + gas.access_width = ctz32(register_bit_width) - 2; >> + gas.address = register_address; >> + build_append_gas_from_struct(table_data, &gas); >> + /* Value */ >> + build_append_int_noprefix(table_data, value , 8); >> + /* Mask */ >> + mask = (1ULL << (register_bit_width - 1) << 1) - 1; >> + build_append_int_noprefix(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) >> +{ >> + GArray *table_instruction_data; >> + unsigned action; >> + 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 }; >> + >> + trace_acpi_erst_pci_bar_0(bar0); >> + >> + /* >> + * 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. >> + */ >> + table_instruction_data = g_array_new(FALSE, FALSE, sizeof(char)); >> + >> + /* >> + * Macros for use with construction of the action instructions >> + */ >> +#define BUILD_READ_REGISTER(width_in_bits, reg) \ >> + build_serialization_instruction_entry(table_instruction_data, \ >> + action, INST_READ_REGISTER, 0, width_in_bits, \ >> + bar0 + reg, 0) >> + >> +#define BUILD_READ_REGISTER_VALUE(width_in_bits, reg, value) \ >> + build_serialization_instruction_entry(table_instruction_data, \ >> + action, INST_READ_REGISTER_VALUE, 0, width_in_bits, \ >> + bar0 + reg, value) >> + >> +#define BUILD_WRITE_REGISTER(width_in_bits, reg, value) \ >> + build_serialization_instruction_entry(table_instruction_data, \ >> + action, INST_WRITE_REGISTER, 0, width_in_bits, \ >> + bar0 + reg, value) >> + >> +#define BUILD_WRITE_REGISTER_VALUE(width_in_bits, reg, value) \ >> + build_serialization_instruction_entry(table_instruction_data, \ >> + action, INST_WRITE_REGISTER_VALUE, 0, width_in_bits, \ >> + bar0 + reg, value) >> + >> + /* Serialization Instruction Entries */ >> + action = ACTION_BEGIN_WRITE_OPERATION; >> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >> + >> + action = ACTION_BEGIN_READ_OPERATION; >> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >> + >> + action = ACTION_BEGIN_CLEAR_OPERATION; >> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >> + >> + action = ACTION_END_OPERATION; >> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >> + >> + action = ACTION_SET_RECORD_OFFSET; >> + BUILD_WRITE_REGISTER(32, ERST_VALUE_OFFSET, 0); >> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >> + >> + action = ACTION_EXECUTE_OPERATION; >> + BUILD_WRITE_REGISTER_VALUE(32, ERST_VALUE_OFFSET, >> + ERST_EXECUTE_OPERATION_MAGIC); > > except here, on all cases we have > BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > We should treat the above as special case and simplify the rest of the > calls (eliminate repeated common arguments). OK, I created BUILD_WRITE_ACTION() to replace this occurrence. I've provided what this section of code looks like with this and the other below change at the end. I have seen the comment from Michael and you about using inline functions, I will respond to that in the other message. > >> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >> + >> + action = ACTION_CHECK_BUSY_STATUS; >> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >> + BUILD_READ_REGISTER_VALUE(32, ERST_VALUE_OFFSET, 0x01); >> + >> + action = ACTION_GET_COMMAND_STATUS; >> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >> + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET); >> + >> + action = ACTION_GET_RECORD_IDENTIFIER; >> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >> + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); >> + >> + action = ACTION_SET_RECORD_IDENTIFIER; >> + BUILD_WRITE_REGISTER(64, ERST_VALUE_OFFSET, 0); >> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > This one seems reverted. Should this be > BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > BUILD_WRITE_REGISTER(64, ERST_VALUE_OFFSET, 0); > > like others? This is a SET operation, so the data is provided in VALUE register, then the ACTION is written to perform the command, ie record the value. > >> + >> + action = ACTION_GET_RECORD_COUNT; >> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >> + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET); >> + >> + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION; >> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >> + >> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE; >> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >> + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); >> + >> + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH; >> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >> + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); >> + >> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES; >> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >> + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET); >> + >> + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS; >> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >> + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); >> + > > BUILD_READ_REGISTER() is always called with ERST_VALUE_OFFSET as second > argument. WE should eliminate this repeated passing of same argument. The BUILD_READ_REGISTER is always against the VALUE register, as you point out, so I've s/BUILD_READ_REGISTER/BUILD_READ_VALUE/ and embedded the offset in the macro now. You can see this below. > > >> + /* 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 >> >> And here is what the main snippet looks like with the above changes (a diff is quite messy): /* * Macros for use with construction of the action instructions */ #define BUILD_READ_VALUE(width_in_bits) \ build_serialization_instruction_entry(table_instruction_data, \ action, INST_READ_REGISTER, 0, width_in_bits, \ bar0 + ERST_VALUE_OFFSET, 0) #define BUILD_READ_VALUE_VALUE(width_in_bits, value) \ build_serialization_instruction_entry(table_instruction_data, \ action, INST_READ_REGISTER_VALUE, 0, width_in_bits, \ bar0 + ERST_VALUE_OFFSET, value) #define BUILD_WRITE_REGISTER(width_in_bits, reg, value) \ build_serialization_instruction_entry(table_instruction_data, \ action, INST_WRITE_REGISTER, 0, width_in_bits, \ bar0 + reg, value) #define BUILD_WRITE_REGISTER_VALUE(width_in_bits, reg, value) \ build_serialization_instruction_entry(table_instruction_data, \ action, INST_WRITE_REGISTER_VALUE, 0, width_in_bits, \ bar0 + reg, value) #define BUILD_WRITE_ACTION() \ BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action) /* Serialization Instruction Entries */ action = ACTION_BEGIN_WRITE_OPERATION; BUILD_WRITE_ACTION(); action = ACTION_BEGIN_READ_OPERATION; BUILD_WRITE_ACTION(); action = ACTION_BEGIN_CLEAR_OPERATION; BUILD_WRITE_ACTION(); action = ACTION_END_OPERATION; BUILD_WRITE_ACTION(); action = ACTION_SET_RECORD_OFFSET; BUILD_WRITE_REGISTER(32, ERST_VALUE_OFFSET, 0); BUILD_WRITE_ACTION(); action = ACTION_EXECUTE_OPERATION; BUILD_WRITE_REGISTER_VALUE(32, ERST_VALUE_OFFSET, ERST_EXECUTE_OPERATION_MAGIC); BUILD_WRITE_ACTION(); action = ACTION_CHECK_BUSY_STATUS; BUILD_WRITE_ACTION(); BUILD_READ_VALUE_VALUE(32, 0x01); action = ACTION_GET_COMMAND_STATUS; BUILD_WRITE_ACTION(); BUILD_READ_VALUE(32); action = ACTION_GET_RECORD_IDENTIFIER; BUILD_WRITE_ACTION(); BUILD_READ_VALUE(64); action = ACTION_SET_RECORD_IDENTIFIER; BUILD_WRITE_REGISTER(64, ERST_VALUE_OFFSET, 0); BUILD_WRITE_ACTION(); action = ACTION_GET_RECORD_COUNT; BUILD_WRITE_ACTION(); BUILD_READ_VALUE(32); action = ACTION_BEGIN_DUMMY_WRITE_OPERATION; BUILD_WRITE_ACTION(); BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE; BUILD_WRITE_ACTION(); BUILD_READ_VALUE(64); action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH; BUILD_WRITE_ACTION(); BUILD_READ_VALUE(64); action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES; BUILD_WRITE_ACTION(); BUILD_READ_VALUE(32); action = ACTION_GET_EXECUTE_OPERATION_TIMINGS; BUILD_WRITE_ACTION(); BUILD_READ_VALUE(64); /* Serialization Header */
Hi Michael, Thanks for examining this! Inline response below. eric On 1/25/22 06:05, Michael S. Tsirkin wrote: > On Tue, Jan 25, 2022 at 04:23:49PM +0530, Ani Sinha wrote: >> >> >> On Mon, 24 Jan 2022, Eric DeVolder wrote: >> >>> 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 | 188 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 188 insertions(+) >>> >>> diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c >>> index fe9ba51..b0c7539 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,173 @@ typedef struct { >>> >>> /*******************************************************************/ >>> /*******************************************************************/ >>> + >>> +/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */ >>> +static void build_serialization_instruction_entry(GArray *table_data, >>> + uint8_t serialization_action, >>> + uint8_t instruction, >>> + uint8_t flags, >>> + uint8_t register_bit_width, >>> + uint64_t register_address, >>> + uint64_t value) >>> +{ >>> + /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */ >>> + struct AcpiGenericAddress gas; >>> + uint64_t mask; >>> + >>> + /* Serialization Action */ >>> + build_append_int_noprefix(table_data, serialization_action, 1); >>> + /* Instruction */ >>> + build_append_int_noprefix(table_data, instruction , 1); >>> + /* Flags */ >>> + build_append_int_noprefix(table_data, flags , 1); >>> + /* Reserved */ >>> + build_append_int_noprefix(table_data, 0 , 1); >>> + /* Register Region */ >>> + gas.space_id = AML_SYSTEM_MEMORY; >>> + gas.bit_width = register_bit_width; >>> + gas.bit_offset = 0; >>> + gas.access_width = ctz32(register_bit_width) - 2; >>> + gas.address = register_address; >>> + build_append_gas_from_struct(table_data, &gas); >>> + /* Value */ >>> + build_append_int_noprefix(table_data, value , 8); >>> + /* Mask */ >>> + mask = (1ULL << (register_bit_width - 1) << 1) - 1; >>> + build_append_int_noprefix(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) >>> +{ >>> + GArray *table_instruction_data; >>> + unsigned action; >>> + 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 }; >>> + >>> + trace_acpi_erst_pci_bar_0(bar0); >>> + >>> + /* >>> + * 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. >>> + */ >>> + table_instruction_data = g_array_new(FALSE, FALSE, sizeof(char)); >>> + >>> + /* >>> + * Macros for use with construction of the action instructions >>> + */ >>> +#define BUILD_READ_REGISTER(width_in_bits, reg) \ >>> + build_serialization_instruction_entry(table_instruction_data, \ >>> + action, INST_READ_REGISTER, 0, width_in_bits, \ >>> + bar0 + reg, 0) >>> + >>> +#define BUILD_READ_REGISTER_VALUE(width_in_bits, reg, value) \ >>> + build_serialization_instruction_entry(table_instruction_data, \ >>> + action, INST_READ_REGISTER_VALUE, 0, width_in_bits, \ >>> + bar0 + reg, value) >>> + >>> +#define BUILD_WRITE_REGISTER(width_in_bits, reg, value) \ >>> + build_serialization_instruction_entry(table_instruction_data, \ >>> + action, INST_WRITE_REGISTER, 0, width_in_bits, \ >>> + bar0 + reg, value) >>> + >>> +#define BUILD_WRITE_REGISTER_VALUE(width_in_bits, reg, value) \ >>> + build_serialization_instruction_entry(table_instruction_data, \ >>> + action, INST_WRITE_REGISTER_VALUE, 0, width_in_bits, \ >>> + bar0 + reg, value) > > > I think these macros which in a hidden way use the bar0 variable really > should be replaced with inline functions, improving type safety. I had not stated this previously, but my choice for using macros over functions was the use three local variables: table_instruction_data, bar0, and action. Any function would then automatically require these three as parameters, or I'm stuffing these temporary items into local globals (to avoid passing as parameters). As for the type safety of bar0, I don't quite understand what I should do differently (regardless of macro vs function). Ultimately these call build_serialization_instruction_entry() with the 'uint64_t register address' accepting the bar0+offset value. Bar0 is pcibar_t and the compiler happily implicitly typecasts to uint64_t. What would an acceptable function prototype look like? > > > > >>> + >>> + /* Serialization Instruction Entries */ >>> + action = ACTION_BEGIN_WRITE_OPERATION; >>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>> + >>> + action = ACTION_BEGIN_READ_OPERATION; >>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>> + >>> + action = ACTION_BEGIN_CLEAR_OPERATION; >>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>> + >>> + action = ACTION_END_OPERATION; >>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>> + >>> + action = ACTION_SET_RECORD_OFFSET; >>> + BUILD_WRITE_REGISTER(32, ERST_VALUE_OFFSET, 0); >>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>> + >>> + action = ACTION_EXECUTE_OPERATION; >>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_VALUE_OFFSET, >>> + ERST_EXECUTE_OPERATION_MAGIC); >> >> except here, on all cases we have >> BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >> >> We should treat the above as special case and simplify the rest of the >> calls (eliminate repeated common arguments). >> >>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>> + >>> + action = ACTION_CHECK_BUSY_STATUS; >>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>> + BUILD_READ_REGISTER_VALUE(32, ERST_VALUE_OFFSET, 0x01); >>> + >>> + action = ACTION_GET_COMMAND_STATUS; >>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>> + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET); >>> + >>> + action = ACTION_GET_RECORD_IDENTIFIER; >>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>> + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); >>> + >>> + action = ACTION_SET_RECORD_IDENTIFIER; >>> + BUILD_WRITE_REGISTER(64, ERST_VALUE_OFFSET, 0); >>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >> >> This one seems reverted. Should this be >> BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >> BUILD_WRITE_REGISTER(64, ERST_VALUE_OFFSET, 0); >> >> like others? >> >>> + >>> + action = ACTION_GET_RECORD_COUNT; >>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>> + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET); >>> + >>> + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION; >>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>> + >>> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE; >>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>> + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); >>> + >>> + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH; >>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>> + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); >>> + >>> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES; >>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>> + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET); >>> + >>> + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS; >>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>> + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); >>> + >> >> BUILD_READ_REGISTER() is always called with ERST_VALUE_OFFSET as second >> argument. WE should eliminate this repeated passing of same argument. >> >> >>> + /* 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 Tue, Jan 25, 2022 at 10:32:45AM -0600, Eric DeVolder wrote: > Hi Michael, > Thanks for examining this! Inline response below. > eric > > On 1/25/22 06:05, Michael S. Tsirkin wrote: > > On Tue, Jan 25, 2022 at 04:23:49PM +0530, Ani Sinha wrote: > > > > > > > > > On Mon, 24 Jan 2022, Eric DeVolder wrote: > > > > > > > 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 | 188 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 188 insertions(+) > > > > > > > > diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c > > > > index fe9ba51..b0c7539 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,173 @@ typedef struct { > > > > > > > > /*******************************************************************/ > > > > /*******************************************************************/ > > > > + > > > > +/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */ > > > > +static void build_serialization_instruction_entry(GArray *table_data, > > > > + uint8_t serialization_action, > > > > + uint8_t instruction, > > > > + uint8_t flags, > > > > + uint8_t register_bit_width, > > > > + uint64_t register_address, > > > > + uint64_t value) > > > > +{ > > > > + /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */ > > > > + struct AcpiGenericAddress gas; > > > > + uint64_t mask; > > > > + > > > > + /* Serialization Action */ > > > > + build_append_int_noprefix(table_data, serialization_action, 1); > > > > + /* Instruction */ > > > > + build_append_int_noprefix(table_data, instruction , 1); > > > > + /* Flags */ > > > > + build_append_int_noprefix(table_data, flags , 1); > > > > + /* Reserved */ > > > > + build_append_int_noprefix(table_data, 0 , 1); > > > > + /* Register Region */ > > > > + gas.space_id = AML_SYSTEM_MEMORY; > > > > + gas.bit_width = register_bit_width; > > > > + gas.bit_offset = 0; > > > > + gas.access_width = ctz32(register_bit_width) - 2; > > > > + gas.address = register_address; > > > > + build_append_gas_from_struct(table_data, &gas); > > > > + /* Value */ > > > > + build_append_int_noprefix(table_data, value , 8); > > > > + /* Mask */ > > > > + mask = (1ULL << (register_bit_width - 1) << 1) - 1; > > > > + build_append_int_noprefix(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) > > > > +{ > > > > + GArray *table_instruction_data; > > > > + unsigned action; > > > > + 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 }; > > > > + > > > > + trace_acpi_erst_pci_bar_0(bar0); > > > > + > > > > + /* > > > > + * 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. > > > > + */ > > > > + table_instruction_data = g_array_new(FALSE, FALSE, sizeof(char)); > > > > + > > > > + /* > > > > + * Macros for use with construction of the action instructions > > > > + */ > > > > +#define BUILD_READ_REGISTER(width_in_bits, reg) \ > > > > + build_serialization_instruction_entry(table_instruction_data, \ > > > > + action, INST_READ_REGISTER, 0, width_in_bits, \ > > > > + bar0 + reg, 0) > > > > + > > > > +#define BUILD_READ_REGISTER_VALUE(width_in_bits, reg, value) \ > > > > + build_serialization_instruction_entry(table_instruction_data, \ > > > > + action, INST_READ_REGISTER_VALUE, 0, width_in_bits, \ > > > > + bar0 + reg, value) > > > > + > > > > +#define BUILD_WRITE_REGISTER(width_in_bits, reg, value) \ > > > > + build_serialization_instruction_entry(table_instruction_data, \ > > > > + action, INST_WRITE_REGISTER, 0, width_in_bits, \ > > > > + bar0 + reg, value) > > > > + > > > > +#define BUILD_WRITE_REGISTER_VALUE(width_in_bits, reg, value) \ > > > > + build_serialization_instruction_entry(table_instruction_data, \ > > > > + action, INST_WRITE_REGISTER_VALUE, 0, width_in_bits, \ > > > > + bar0 + reg, value) > > > > > > I think these macros which in a hidden way use the bar0 variable really > > should be replaced with inline functions, improving type safety. > > I had not stated this previously, but my choice for using macros over functions > was the use three local variables: table_instruction_data, bar0, and action. Oh, didn't notice the others too. > Any function would then automatically require these three as parameters, or I'm > stuffing these temporary items into local globals (to avoid passing as parameters). Please, no globals. > As for the type safety of bar0, I don't quite understand what I should do differently > (regardless of macro vs function). Ultimately these call build_serialization_instruction_entry() > with the 'uint64_t register address' accepting the bar0+offset value. Bar0 is pcibar_t > and the compiler happily implicitly typecasts to uint64_t. The other way could warn though - maybe we should build with -Wconversion. That's going to require quite a bit of work though. > What would an acceptable function prototype look like? Well, that's a limitation of C. We can pass a structure if you really need to save some code lines but that's really all we can do. > > > > > > > > > > > > > + > > > > + /* Serialization Instruction Entries */ > > > > + action = ACTION_BEGIN_WRITE_OPERATION; > > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > > + > > > > + action = ACTION_BEGIN_READ_OPERATION; > > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > > + > > > > + action = ACTION_BEGIN_CLEAR_OPERATION; > > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > > + > > > > + action = ACTION_END_OPERATION; > > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > > + > > > > + action = ACTION_SET_RECORD_OFFSET; > > > > + BUILD_WRITE_REGISTER(32, ERST_VALUE_OFFSET, 0); > > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > > + > > > > + action = ACTION_EXECUTE_OPERATION; > > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_VALUE_OFFSET, > > > > + ERST_EXECUTE_OPERATION_MAGIC); > > > > > > except here, on all cases we have > > > BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > > > > We should treat the above as special case and simplify the rest of the > > > calls (eliminate repeated common arguments). > > > > > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > > + > > > > + action = ACTION_CHECK_BUSY_STATUS; > > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > > + BUILD_READ_REGISTER_VALUE(32, ERST_VALUE_OFFSET, 0x01); > > > > + > > > > + action = ACTION_GET_COMMAND_STATUS; > > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > > + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET); > > > > + > > > > + action = ACTION_GET_RECORD_IDENTIFIER; > > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > > + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); > > > > + > > > > + action = ACTION_SET_RECORD_IDENTIFIER; > > > > + BUILD_WRITE_REGISTER(64, ERST_VALUE_OFFSET, 0); > > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > > > > This one seems reverted. Should this be > > > BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > BUILD_WRITE_REGISTER(64, ERST_VALUE_OFFSET, 0); > > > > > > like others? > > > > > > > + > > > > + action = ACTION_GET_RECORD_COUNT; > > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > > + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET); > > > > + > > > > + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION; > > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > > + > > > > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE; > > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > > + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); > > > > + > > > > + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH; > > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > > + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); > > > > + > > > > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES; > > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > > + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET); > > > > + > > > > + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS; > > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > > + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); > > > > + > > > > > > BUILD_READ_REGISTER() is always called with ERST_VALUE_OFFSET as second > > > argument. WE should eliminate this repeated passing of same argument. > > > > > > > > > > + /* 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 Tue, Jan 25, 2022 at 10:24:49AM -0600, Eric DeVolder wrote: > Ani, > Thanks for the feedback! Inline responses below. > eric > > On 1/25/22 04:53, Ani Sinha wrote: > > > > > > On Mon, 24 Jan 2022, Eric DeVolder wrote: > > > > > 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 | 188 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 188 insertions(+) > > > > > > diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c > > > index fe9ba51..b0c7539 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,173 @@ typedef struct { > > > > > > /*******************************************************************/ > > > /*******************************************************************/ > > > + > > > +/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */ > > > +static void build_serialization_instruction_entry(GArray *table_data, > > > + uint8_t serialization_action, > > > + uint8_t instruction, > > > + uint8_t flags, > > > + uint8_t register_bit_width, > > > + uint64_t register_address, > > > + uint64_t value) > > > +{ > > > + /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */ > > > + struct AcpiGenericAddress gas; > > > + uint64_t mask; > > > + > > > + /* Serialization Action */ > > > + build_append_int_noprefix(table_data, serialization_action, 1); > > > + /* Instruction */ > > > + build_append_int_noprefix(table_data, instruction , 1); > > > + /* Flags */ > > > + build_append_int_noprefix(table_data, flags , 1); > > > + /* Reserved */ > > > + build_append_int_noprefix(table_data, 0 , 1); > > > + /* Register Region */ > > > + gas.space_id = AML_SYSTEM_MEMORY; > > > + gas.bit_width = register_bit_width; > > > + gas.bit_offset = 0; > > > + gas.access_width = ctz32(register_bit_width) - 2; > > > + gas.address = register_address; > > > + build_append_gas_from_struct(table_data, &gas); > > > + /* Value */ > > > + build_append_int_noprefix(table_data, value , 8); > > > + /* Mask */ > > > + mask = (1ULL << (register_bit_width - 1) << 1) - 1; > > > + build_append_int_noprefix(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) > > > +{ > > > + GArray *table_instruction_data; > > > + unsigned action; > > > + 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 }; > > > + > > > + trace_acpi_erst_pci_bar_0(bar0); > > > + > > > + /* > > > + * 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. > > > + */ > > > + table_instruction_data = g_array_new(FALSE, FALSE, sizeof(char)); > > > + > > > + /* > > > + * Macros for use with construction of the action instructions > > > + */ > > > +#define BUILD_READ_REGISTER(width_in_bits, reg) \ > > > + build_serialization_instruction_entry(table_instruction_data, \ > > > + action, INST_READ_REGISTER, 0, width_in_bits, \ > > > + bar0 + reg, 0) > > > + > > > +#define BUILD_READ_REGISTER_VALUE(width_in_bits, reg, value) \ > > > + build_serialization_instruction_entry(table_instruction_data, \ > > > + action, INST_READ_REGISTER_VALUE, 0, width_in_bits, \ > > > + bar0 + reg, value) > > > + > > > +#define BUILD_WRITE_REGISTER(width_in_bits, reg, value) \ > > > + build_serialization_instruction_entry(table_instruction_data, \ > > > + action, INST_WRITE_REGISTER, 0, width_in_bits, \ > > > + bar0 + reg, value) > > > + > > > +#define BUILD_WRITE_REGISTER_VALUE(width_in_bits, reg, value) \ > > > + build_serialization_instruction_entry(table_instruction_data, \ > > > + action, INST_WRITE_REGISTER_VALUE, 0, width_in_bits, \ > > > + bar0 + reg, value) > > > + > > > + /* Serialization Instruction Entries */ > > > + action = ACTION_BEGIN_WRITE_OPERATION; > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > + > > > + action = ACTION_BEGIN_READ_OPERATION; > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > + > > > + action = ACTION_BEGIN_CLEAR_OPERATION; > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > + > > > + action = ACTION_END_OPERATION; > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > + > > > + action = ACTION_SET_RECORD_OFFSET; > > > + BUILD_WRITE_REGISTER(32, ERST_VALUE_OFFSET, 0); > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > + > > > + action = ACTION_EXECUTE_OPERATION; > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_VALUE_OFFSET, > > > + ERST_EXECUTE_OPERATION_MAGIC); So e.g. typedef struct { GArray *table_data; uint8_t register_bit_width; pcibus_t bar; unsigned register; } BuildSerializationInstructionEntry; void build_serialization_instruction_entry(BuildSerializationInstructionEntry *) and now BuildSerializationInstructionEntry entry = { .table_data = table_instruction_data, bar = bar0, }; BuildSerializationInstructionEntry write32 = { .table_data = entry.table_data, .bar = entry.bar, .register_bit_width = 32, .register = ERST_ACTION_OFFSET }; etc. you can move common fields into structure, uncommon ones out of it. > > > > except here, on all cases we have > > BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > > We should treat the above as special case and simplify the rest of the > > calls (eliminate repeated common arguments). > > OK, I created BUILD_WRITE_ACTION() to replace this occurrence. I've provided > what this section of code looks like with this and the other below change at > the end. > > I have seen the comment from Michael and you about using inline functions, I > will respond to that in the other message. > > > > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > + > > > + action = ACTION_CHECK_BUSY_STATUS; > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > + BUILD_READ_REGISTER_VALUE(32, ERST_VALUE_OFFSET, 0x01); > > > + > > > + action = ACTION_GET_COMMAND_STATUS; > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET); > > > + > > > + action = ACTION_GET_RECORD_IDENTIFIER; > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); > > > + > > > + action = ACTION_SET_RECORD_IDENTIFIER; > > > + BUILD_WRITE_REGISTER(64, ERST_VALUE_OFFSET, 0); > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > > This one seems reverted. Should this be > > BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > BUILD_WRITE_REGISTER(64, ERST_VALUE_OFFSET, 0); > > > > like others? > > This is a SET operation, so the data is provided in VALUE register, then > the ACTION is written to perform the command, ie record the value. > > > > > > + > > > + action = ACTION_GET_RECORD_COUNT; > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET); > > > + > > > + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION; > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > + > > > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE; > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); > > > + > > > + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH; > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); > > > + > > > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES; > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET); > > > + > > > + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS; > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); > > > + > > > > BUILD_READ_REGISTER() is always called with ERST_VALUE_OFFSET as second > > argument. WE should eliminate this repeated passing of same argument. > > The BUILD_READ_REGISTER is always against the VALUE register, as you point out, > so I've s/BUILD_READ_REGISTER/BUILD_READ_VALUE/ and embedded the offset in the > macro now. You can see this below. > > > > > > > > + /* 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 > > > > > > > > And here is what the main snippet looks like with the above changes (a diff > is quite messy): > > /* > * Macros for use with construction of the action instructions > */ > #define BUILD_READ_VALUE(width_in_bits) \ > build_serialization_instruction_entry(table_instruction_data, \ > action, INST_READ_REGISTER, 0, width_in_bits, \ > bar0 + ERST_VALUE_OFFSET, 0) > > #define BUILD_READ_VALUE_VALUE(width_in_bits, value) \ > build_serialization_instruction_entry(table_instruction_data, \ > action, INST_READ_REGISTER_VALUE, 0, width_in_bits, \ > bar0 + ERST_VALUE_OFFSET, value) > > #define BUILD_WRITE_REGISTER(width_in_bits, reg, value) \ > build_serialization_instruction_entry(table_instruction_data, \ > action, INST_WRITE_REGISTER, 0, width_in_bits, \ > bar0 + reg, value) > > #define BUILD_WRITE_REGISTER_VALUE(width_in_bits, reg, value) \ > build_serialization_instruction_entry(table_instruction_data, \ > action, INST_WRITE_REGISTER_VALUE, 0, width_in_bits, \ > bar0 + reg, value) > > #define BUILD_WRITE_ACTION() \ > BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action) > > /* Serialization Instruction Entries */ > action = ACTION_BEGIN_WRITE_OPERATION; > BUILD_WRITE_ACTION(); > > action = ACTION_BEGIN_READ_OPERATION; > BUILD_WRITE_ACTION(); > > action = ACTION_BEGIN_CLEAR_OPERATION; > BUILD_WRITE_ACTION(); > > action = ACTION_END_OPERATION; > BUILD_WRITE_ACTION(); > > action = ACTION_SET_RECORD_OFFSET; > BUILD_WRITE_REGISTER(32, ERST_VALUE_OFFSET, 0); > BUILD_WRITE_ACTION(); > > action = ACTION_EXECUTE_OPERATION; > BUILD_WRITE_REGISTER_VALUE(32, ERST_VALUE_OFFSET, > ERST_EXECUTE_OPERATION_MAGIC); > BUILD_WRITE_ACTION(); > > action = ACTION_CHECK_BUSY_STATUS; > BUILD_WRITE_ACTION(); > BUILD_READ_VALUE_VALUE(32, 0x01); > > action = ACTION_GET_COMMAND_STATUS; > BUILD_WRITE_ACTION(); > BUILD_READ_VALUE(32); > > action = ACTION_GET_RECORD_IDENTIFIER; > BUILD_WRITE_ACTION(); > BUILD_READ_VALUE(64); > > action = ACTION_SET_RECORD_IDENTIFIER; > BUILD_WRITE_REGISTER(64, ERST_VALUE_OFFSET, 0); > BUILD_WRITE_ACTION(); > > action = ACTION_GET_RECORD_COUNT; > BUILD_WRITE_ACTION(); > BUILD_READ_VALUE(32); > > action = ACTION_BEGIN_DUMMY_WRITE_OPERATION; > BUILD_WRITE_ACTION(); > BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE; > BUILD_WRITE_ACTION(); > BUILD_READ_VALUE(64); > > action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH; > BUILD_WRITE_ACTION(); > BUILD_READ_VALUE(64); > > action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES; > BUILD_WRITE_ACTION(); > BUILD_READ_VALUE(32); > > action = ACTION_GET_EXECUTE_OPERATION_TIMINGS; > BUILD_WRITE_ACTION(); > BUILD_READ_VALUE(64); > > /* Serialization Header */
On Tue, 25 Jan 2022, Eric DeVolder wrote: > Ani, > Thanks for the feedback! Inline responses below. > eric > > On 1/25/22 04:53, Ani Sinha wrote: > > > > > > > + > > > + action = ACTION_BEGIN_CLEAR_OPERATION; > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > + > > > + action = ACTION_END_OPERATION; > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > + > > > + action = ACTION_SET_RECORD_OFFSET; > > > + BUILD_WRITE_REGISTER(32, ERST_VALUE_OFFSET, 0); > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > + > > > + action = ACTION_EXECUTE_OPERATION; > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_VALUE_OFFSET, > > > + ERST_EXECUTE_OPERATION_MAGIC); > > > > except here, on all cases we have > > BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > > We should treat the above as special case and simplify the rest of the > > calls (eliminate repeated common arguments). > > OK, I created BUILD_WRITE_ACTION() to replace this occurrence. I've provided > what this section of code looks like with this and the other below change at > the end. > > I have seen the comment from Michael and you about using inline functions, I > will respond to that in the other message. > > > > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > + > > > + action = ACTION_CHECK_BUSY_STATUS; > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > + BUILD_READ_REGISTER_VALUE(32, ERST_VALUE_OFFSET, 0x01); > > > + > > > + action = ACTION_GET_COMMAND_STATUS; > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET); > > > + > > > + action = ACTION_GET_RECORD_IDENTIFIER; > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); > > > + > > > + action = ACTION_SET_RECORD_IDENTIFIER; > > > + BUILD_WRITE_REGISTER(64, ERST_VALUE_OFFSET, 0); > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > > This one seems reverted. Should this be > > BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > BUILD_WRITE_REGISTER(64, ERST_VALUE_OFFSET, 0); > > > > like others? > > This is a SET operation, so the data is provided in VALUE register, then > the ACTION is written to perform the command, ie record the value. > Ok I see. makes sense. > > > > > + > > > + action = ACTION_GET_RECORD_COUNT; > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET); > > > + > > > + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION; > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > + > > > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE; > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); > > > + > > > + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH; > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); > > > + > > > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES; > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET); > > > + > > > + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS; > > > + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > > + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); > > > + > > > > BUILD_READ_REGISTER() is always called with ERST_VALUE_OFFSET as second > > argument. WE should eliminate this repeated passing of same argument. > > The BUILD_READ_REGISTER is always against the VALUE register, as you point > out, > so I've s/BUILD_READ_REGISTER/BUILD_READ_VALUE/ and embedded the offset in the > macro now. You can see this below. > > And here is what the main snippet looks like with the above changes (a diff > is quite messy): > > /* > * Macros for use with construction of the action instructions > */ > #define BUILD_READ_VALUE(width_in_bits) \ > build_serialization_instruction_entry(table_instruction_data, \ > action, INST_READ_REGISTER, 0, width_in_bits, \ > bar0 + ERST_VALUE_OFFSET, 0) > > #define BUILD_READ_VALUE_VALUE(width_in_bits, value) \ > build_serialization_instruction_entry(table_instruction_data, \ > action, INST_READ_REGISTER_VALUE, 0, width_in_bits, \ > bar0 + ERST_VALUE_OFFSET, value) > > #define BUILD_WRITE_REGISTER(width_in_bits, reg, value) \ > build_serialization_instruction_entry(table_instruction_data, \ > action, INST_WRITE_REGISTER, 0, width_in_bits, \ > bar0 + reg, value) > > #define BUILD_WRITE_REGISTER_VALUE(width_in_bits, reg, value) \ > build_serialization_instruction_entry(table_instruction_data, \ > action, INST_WRITE_REGISTER_VALUE, 0, width_in_bits, \ > bar0 + reg, value) > > #define BUILD_WRITE_ACTION() \ > BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action) > > /* Serialization Instruction Entries */ > action = ACTION_BEGIN_WRITE_OPERATION; > BUILD_WRITE_ACTION(); > > action = ACTION_BEGIN_READ_OPERATION; > BUILD_WRITE_ACTION(); > > action = ACTION_BEGIN_CLEAR_OPERATION; > BUILD_WRITE_ACTION(); > > action = ACTION_END_OPERATION; > BUILD_WRITE_ACTION(); > > action = ACTION_SET_RECORD_OFFSET; > BUILD_WRITE_REGISTER(32, ERST_VALUE_OFFSET, 0); > BUILD_WRITE_ACTION(); > > action = ACTION_EXECUTE_OPERATION; > BUILD_WRITE_REGISTER_VALUE(32, ERST_VALUE_OFFSET, > ERST_EXECUTE_OPERATION_MAGIC); > BUILD_WRITE_ACTION(); > > action = ACTION_CHECK_BUSY_STATUS; > BUILD_WRITE_ACTION(); > BUILD_READ_VALUE_VALUE(32, 0x01); > > action = ACTION_GET_COMMAND_STATUS; > BUILD_WRITE_ACTION(); > BUILD_READ_VALUE(32); > > action = ACTION_GET_RECORD_IDENTIFIER; > BUILD_WRITE_ACTION(); > BUILD_READ_VALUE(64); > > action = ACTION_SET_RECORD_IDENTIFIER; > BUILD_WRITE_REGISTER(64, ERST_VALUE_OFFSET, 0); > BUILD_WRITE_ACTION(); > > action = ACTION_GET_RECORD_COUNT; > BUILD_WRITE_ACTION(); > BUILD_READ_VALUE(32); > > action = ACTION_BEGIN_DUMMY_WRITE_OPERATION; > BUILD_WRITE_ACTION(); > BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); > > action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE; > BUILD_WRITE_ACTION(); > BUILD_READ_VALUE(64); > > action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH; > BUILD_WRITE_ACTION(); > BUILD_READ_VALUE(64); > > action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES; > BUILD_WRITE_ACTION(); > BUILD_READ_VALUE(32); > > action = ACTION_GET_EXECUTE_OPERATION_TIMINGS; > BUILD_WRITE_ACTION(); > BUILD_READ_VALUE(64); > > /* Serialization Header */ Yes this looks a lot cleaner. Now as Michael suggested, we can convert them to inline functions and pass a struct with the common params. Maybe we can use a macro also to make things even more cleaner. Like calling the inline function from the macro with the common struct. I am trying to avoid repeated copy-paste code.
On Tue, Jan 25, 2022 at 10:24:49AM -0600, Eric DeVolder wrote: > And here is what the main snippet looks like with the above changes (a diff > is quite messy): > > /* > * Macros for use with construction of the action instructions > */ > #define BUILD_READ_VALUE(width_in_bits) \ > build_serialization_instruction_entry(table_instruction_data, \ > action, INST_READ_REGISTER, 0, width_in_bits, \ > bar0 + ERST_VALUE_OFFSET, 0) > > #define BUILD_READ_VALUE_VALUE(width_in_bits, value) \ > build_serialization_instruction_entry(table_instruction_data, \ > action, INST_READ_REGISTER_VALUE, 0, width_in_bits, \ > bar0 + ERST_VALUE_OFFSET, value) > > #define BUILD_WRITE_REGISTER(width_in_bits, reg, value) \ > build_serialization_instruction_entry(table_instruction_data, \ > action, INST_WRITE_REGISTER, 0, width_in_bits, \ > bar0 + reg, value) > > #define BUILD_WRITE_REGISTER_VALUE(width_in_bits, reg, value) \ > build_serialization_instruction_entry(table_instruction_data, \ > action, INST_WRITE_REGISTER_VALUE, 0, width_in_bits, \ > bar0 + reg, value) Please, don't hide variable accesses in macros like that. whatever is accessed should be part of a parameter.
Ani, Michael, An inline response at the bottom. Thanks! eric On 1/26/22 01:05, Ani Sinha wrote: > > > On Tue, 25 Jan 2022, Eric DeVolder wrote: > >> Ani, >> Thanks for the feedback! Inline responses below. >> eric >> >> On 1/25/22 04:53, Ani Sinha wrote: >>> >>> >>>> + >>>> + action = ACTION_BEGIN_CLEAR_OPERATION; >>>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>>> + >>>> + action = ACTION_END_OPERATION; >>>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>>> + >>>> + action = ACTION_SET_RECORD_OFFSET; >>>> + BUILD_WRITE_REGISTER(32, ERST_VALUE_OFFSET, 0); >>>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>>> + >>>> + action = ACTION_EXECUTE_OPERATION; >>>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_VALUE_OFFSET, >>>> + ERST_EXECUTE_OPERATION_MAGIC); >>> >>> except here, on all cases we have >>> BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>> >>> We should treat the above as special case and simplify the rest of the >>> calls (eliminate repeated common arguments). >> >> OK, I created BUILD_WRITE_ACTION() to replace this occurrence. I've provided >> what this section of code looks like with this and the other below change at >> the end. >> >> I have seen the comment from Michael and you about using inline functions, I >> will respond to that in the other message. >> >>> >>>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>>> + >>>> + action = ACTION_CHECK_BUSY_STATUS; >>>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>>> + BUILD_READ_REGISTER_VALUE(32, ERST_VALUE_OFFSET, 0x01); >>>> + >>>> + action = ACTION_GET_COMMAND_STATUS; >>>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>>> + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET); >>>> + >>>> + action = ACTION_GET_RECORD_IDENTIFIER; >>>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>>> + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); >>>> + >>>> + action = ACTION_SET_RECORD_IDENTIFIER; >>>> + BUILD_WRITE_REGISTER(64, ERST_VALUE_OFFSET, 0); >>>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>> >>> This one seems reverted. Should this be >>> BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>> BUILD_WRITE_REGISTER(64, ERST_VALUE_OFFSET, 0); >>> >>> like others? >> >> This is a SET operation, so the data is provided in VALUE register, then >> the ACTION is written to perform the command, ie record the value. >> > > Ok I see. makes sense. > >>> >>>> + >>>> + action = ACTION_GET_RECORD_COUNT; >>>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>>> + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET); >>>> + >>>> + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION; >>>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>>> + >>>> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE; >>>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>>> + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); >>>> + >>>> + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH; >>>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>>> + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); >>>> + >>>> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES; >>>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>>> + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET); >>>> + >>>> + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS; >>>> + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >>>> + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); >>>> + >>> >>> BUILD_READ_REGISTER() is always called with ERST_VALUE_OFFSET as second >>> argument. WE should eliminate this repeated passing of same argument. >> >> The BUILD_READ_REGISTER is always against the VALUE register, as you point >> out, >> so I've s/BUILD_READ_REGISTER/BUILD_READ_VALUE/ and embedded the offset in the >> macro now. You can see this below. >> > >> And here is what the main snippet looks like with the above changes (a diff >> is quite messy): >> >> /* >> * Macros for use with construction of the action instructions >> */ >> #define BUILD_READ_VALUE(width_in_bits) \ >> build_serialization_instruction_entry(table_instruction_data, \ >> action, INST_READ_REGISTER, 0, width_in_bits, \ >> bar0 + ERST_VALUE_OFFSET, 0) >> >> #define BUILD_READ_VALUE_VALUE(width_in_bits, value) \ >> build_serialization_instruction_entry(table_instruction_data, \ >> action, INST_READ_REGISTER_VALUE, 0, width_in_bits, \ >> bar0 + ERST_VALUE_OFFSET, value) >> >> #define BUILD_WRITE_REGISTER(width_in_bits, reg, value) \ >> build_serialization_instruction_entry(table_instruction_data, \ >> action, INST_WRITE_REGISTER, 0, width_in_bits, \ >> bar0 + reg, value) >> >> #define BUILD_WRITE_REGISTER_VALUE(width_in_bits, reg, value) \ >> build_serialization_instruction_entry(table_instruction_data, \ >> action, INST_WRITE_REGISTER_VALUE, 0, width_in_bits, \ >> bar0 + reg, value) >> >> #define BUILD_WRITE_ACTION() \ >> BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action) >> >> /* Serialization Instruction Entries */ >> action = ACTION_BEGIN_WRITE_OPERATION; >> BUILD_WRITE_ACTION(); >> >> action = ACTION_BEGIN_READ_OPERATION; >> BUILD_WRITE_ACTION(); >> >> action = ACTION_BEGIN_CLEAR_OPERATION; >> BUILD_WRITE_ACTION(); >> >> action = ACTION_END_OPERATION; >> BUILD_WRITE_ACTION(); >> >> action = ACTION_SET_RECORD_OFFSET; >> BUILD_WRITE_REGISTER(32, ERST_VALUE_OFFSET, 0); >> BUILD_WRITE_ACTION(); >> >> action = ACTION_EXECUTE_OPERATION; >> BUILD_WRITE_REGISTER_VALUE(32, ERST_VALUE_OFFSET, >> ERST_EXECUTE_OPERATION_MAGIC); >> BUILD_WRITE_ACTION(); >> >> action = ACTION_CHECK_BUSY_STATUS; >> BUILD_WRITE_ACTION(); >> BUILD_READ_VALUE_VALUE(32, 0x01); >> >> action = ACTION_GET_COMMAND_STATUS; >> BUILD_WRITE_ACTION(); >> BUILD_READ_VALUE(32); >> >> action = ACTION_GET_RECORD_IDENTIFIER; >> BUILD_WRITE_ACTION(); >> BUILD_READ_VALUE(64); >> >> action = ACTION_SET_RECORD_IDENTIFIER; >> BUILD_WRITE_REGISTER(64, ERST_VALUE_OFFSET, 0); >> BUILD_WRITE_ACTION(); >> >> action = ACTION_GET_RECORD_COUNT; >> BUILD_WRITE_ACTION(); >> BUILD_READ_VALUE(32); >> >> action = ACTION_BEGIN_DUMMY_WRITE_OPERATION; >> BUILD_WRITE_ACTION(); >> BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); >> >> action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE; >> BUILD_WRITE_ACTION(); >> BUILD_READ_VALUE(64); >> >> action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH; >> BUILD_WRITE_ACTION(); >> BUILD_READ_VALUE(64); >> >> action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES; >> BUILD_WRITE_ACTION(); >> BUILD_READ_VALUE(32); >> >> action = ACTION_GET_EXECUTE_OPERATION_TIMINGS; >> BUILD_WRITE_ACTION(); >> BUILD_READ_VALUE(64); >> >> /* Serialization Header */ > > Yes this looks a lot cleaner. Now as Michael suggested, we can convert > them to inline functions and pass a struct with the common params. Maybe > we can use a macro also to make things even more cleaner. Like calling > the inline function from the macro with the common struct. I am trying to > avoid repeated copy-paste code. > OK, I've converted the above to utilize a context structure that Michael outlined, and a function call, rather than macro (as above), to generate the table content. Without using macros, I think this code is about as simplified as can be. As this has significant changes, I'll post as v14. Note that the code grew by about 35 lines, not too bad. eric
diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c index fe9ba51..b0c7539 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,173 @@ typedef struct { /*******************************************************************/ /*******************************************************************/ + +/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */ +static void build_serialization_instruction_entry(GArray *table_data, + uint8_t serialization_action, + uint8_t instruction, + uint8_t flags, + uint8_t register_bit_width, + uint64_t register_address, + uint64_t value) +{ + /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */ + struct AcpiGenericAddress gas; + uint64_t mask; + + /* Serialization Action */ + build_append_int_noprefix(table_data, serialization_action, 1); + /* Instruction */ + build_append_int_noprefix(table_data, instruction , 1); + /* Flags */ + build_append_int_noprefix(table_data, flags , 1); + /* Reserved */ + build_append_int_noprefix(table_data, 0 , 1); + /* Register Region */ + gas.space_id = AML_SYSTEM_MEMORY; + gas.bit_width = register_bit_width; + gas.bit_offset = 0; + gas.access_width = ctz32(register_bit_width) - 2; + gas.address = register_address; + build_append_gas_from_struct(table_data, &gas); + /* Value */ + build_append_int_noprefix(table_data, value , 8); + /* Mask */ + mask = (1ULL << (register_bit_width - 1) << 1) - 1; + build_append_int_noprefix(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) +{ + GArray *table_instruction_data; + unsigned action; + 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 }; + + trace_acpi_erst_pci_bar_0(bar0); + + /* + * 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. + */ + table_instruction_data = g_array_new(FALSE, FALSE, sizeof(char)); + + /* + * Macros for use with construction of the action instructions + */ +#define BUILD_READ_REGISTER(width_in_bits, reg) \ + build_serialization_instruction_entry(table_instruction_data, \ + action, INST_READ_REGISTER, 0, width_in_bits, \ + bar0 + reg, 0) + +#define BUILD_READ_REGISTER_VALUE(width_in_bits, reg, value) \ + build_serialization_instruction_entry(table_instruction_data, \ + action, INST_READ_REGISTER_VALUE, 0, width_in_bits, \ + bar0 + reg, value) + +#define BUILD_WRITE_REGISTER(width_in_bits, reg, value) \ + build_serialization_instruction_entry(table_instruction_data, \ + action, INST_WRITE_REGISTER, 0, width_in_bits, \ + bar0 + reg, value) + +#define BUILD_WRITE_REGISTER_VALUE(width_in_bits, reg, value) \ + build_serialization_instruction_entry(table_instruction_data, \ + action, INST_WRITE_REGISTER_VALUE, 0, width_in_bits, \ + bar0 + reg, value) + + /* Serialization Instruction Entries */ + action = ACTION_BEGIN_WRITE_OPERATION; + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); + + action = ACTION_BEGIN_READ_OPERATION; + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); + + action = ACTION_BEGIN_CLEAR_OPERATION; + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); + + action = ACTION_END_OPERATION; + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); + + action = ACTION_SET_RECORD_OFFSET; + BUILD_WRITE_REGISTER(32, ERST_VALUE_OFFSET, 0); + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); + + action = ACTION_EXECUTE_OPERATION; + BUILD_WRITE_REGISTER_VALUE(32, ERST_VALUE_OFFSET, + ERST_EXECUTE_OPERATION_MAGIC); + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); + + action = ACTION_CHECK_BUSY_STATUS; + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); + BUILD_READ_REGISTER_VALUE(32, ERST_VALUE_OFFSET, 0x01); + + action = ACTION_GET_COMMAND_STATUS; + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET); + + action = ACTION_GET_RECORD_IDENTIFIER; + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); + + action = ACTION_SET_RECORD_IDENTIFIER; + BUILD_WRITE_REGISTER(64, ERST_VALUE_OFFSET, 0); + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); + + action = ACTION_GET_RECORD_COUNT; + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET); + + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION; + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); + + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE; + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); + + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH; + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); + + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES; + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); + BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET); + + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS; + BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action); + BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET); + + /* 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 | 188 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 188 insertions(+)