| Message ID | 1639582695-7328-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, Dec 15, 2021 at 9:08 PM Eric DeVolder <eric.devolder@oracle.com> 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 bb6cad4..05177b3 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; This variable can be eliminated (see below). > + 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(action, 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(action, 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(action, 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(action, 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(action, 32, ERST_ACTION_OFFSET, action); > + > + action = ACTION_BEGIN_READ_OPERATION; > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + > + action = ACTION_BEGIN_CLEAR_OPERATION; > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + > + action = ACTION_END_OPERATION; > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + > + action = ACTION_SET_RECORD_OFFSET; > + build_write_register(action, 32, ERST_VALUE_OFFSET, 0); > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + > + action = ACTION_EXECUTE_OPERATION; > + build_write_register_value(action, 32, ERST_VALUE_OFFSET, > + ERST_EXECUTE_OPERATION_MAGIC); > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + > + action = ACTION_CHECK_BUSY_STATUS; > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + build_read_register_value(action, 32, ERST_VALUE_OFFSET, 0x01); > + > + action = ACTION_GET_COMMAND_STATUS; > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + build_read_register(action, 32, ERST_VALUE_OFFSET); > + > + action = ACTION_GET_RECORD_IDENTIFIER; > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + build_read_register(action, 64, ERST_VALUE_OFFSET); > + > + action = ACTION_SET_RECORD_IDENTIFIER; > + build_write_register(action, 64, ERST_VALUE_OFFSET, 0); > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + > + action = ACTION_GET_RECORD_COUNT; > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + build_read_register(action, 32, ERST_VALUE_OFFSET); > + > + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION; > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE; > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + build_read_register(action, 64, ERST_VALUE_OFFSET); > + > + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH; > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + build_read_register(action, 64, ERST_VALUE_OFFSET); > + > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES; > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + build_read_register(action, 32, ERST_VALUE_OFFSET); > + > + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS; > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + build_read_register(action, 64, ERST_VALUE_OFFSET); if I am reading this right, build_write_register_value() is called with the same parameters, except that action is changing. We can optimize repetative calls with the same parameters. build_read_register can be split into build_read_register_32() and build_read_register_64(). So we can have : build_write_register_value(ACTION_GET_EXECUTE_OPERATION_TIMINGS); build_read_register_64(ACTION_GET_EXECUTE_OPERATION_TIMINGS); > + > + /* 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 >
Hi Ani, Thanks for such quick feedback! One inline response below. eric On 12/15/21 10:33, Ani Sinha wrote: > On Wed, Dec 15, 2021 at 9:08 PM Eric DeVolder <eric.devolder@oracle.com> 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 bb6cad4..05177b3 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; > > This variable can be eliminated (see below). > >> + 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(action, 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(action, 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(action, 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(action, 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(action, 32, ERST_ACTION_OFFSET, action); >> + >> + action = ACTION_BEGIN_READ_OPERATION; >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + >> + action = ACTION_BEGIN_CLEAR_OPERATION; >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + >> + action = ACTION_END_OPERATION; >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + >> + action = ACTION_SET_RECORD_OFFSET; >> + build_write_register(action, 32, ERST_VALUE_OFFSET, 0); >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + >> + action = ACTION_EXECUTE_OPERATION; >> + build_write_register_value(action, 32, ERST_VALUE_OFFSET, >> + ERST_EXECUTE_OPERATION_MAGIC); >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > > > >> + >> + action = ACTION_CHECK_BUSY_STATUS; >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + build_read_register_value(action, 32, ERST_VALUE_OFFSET, 0x01); >> + >> + action = ACTION_GET_COMMAND_STATUS; >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + build_read_register(action, 32, ERST_VALUE_OFFSET); >> + >> + action = ACTION_GET_RECORD_IDENTIFIER; >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + build_read_register(action, 64, ERST_VALUE_OFFSET); >> + >> + action = ACTION_SET_RECORD_IDENTIFIER; >> + build_write_register(action, 64, ERST_VALUE_OFFSET, 0); >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + >> + action = ACTION_GET_RECORD_COUNT; >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + build_read_register(action, 32, ERST_VALUE_OFFSET); >> + >> + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION; >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + >> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE; >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + build_read_register(action, 64, ERST_VALUE_OFFSET); >> + >> + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH; >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + build_read_register(action, 64, ERST_VALUE_OFFSET); >> + >> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES; >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + build_read_register(action, 32, ERST_VALUE_OFFSET); >> + >> + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS; >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + build_read_register(action, 64, ERST_VALUE_OFFSET); > > if I am reading this right, build_write_register_value() is called > with the same parameters, except that action is changing. We can > optimize repetative calls with the same parameters. > > build_read_register can be split into build_read_register_32() and > build_read_register_64(). > So we can have : > build_write_register_value(ACTION_GET_EXECUTE_OPERATION_TIMINGS); > build_read_register_64(ACTION_GET_EXECUTE_OPERATION_TIMINGS); > If I understand correctly, you are essentially asking for appropriate accessor functions. I did an inventory and the following would be the list of unique accessors needed: To ACTION register: write_value_32 To VALUE register: write_value_32 write_32 write_64 read_value_32 read_32 read_64 So that is 7 accessors, which must spell out the access type and register name in the macro. With respect to the comment on eliminating the action variable. Given the current code I did miss an optimization to avoid passing 'action' as the first parameter to the macros; I should have just used it directly within the macro. Plus the 'action' assignment acts as documentation to inform you as to which action is being constructed. But in other places, 'action' is being used as a true parameter to the macros as the value to write. If you think that going to more accessors is simpler/more clear, then I'll do so. Thanks! eric >> + >> + /* 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 Wed, Dec 15, 2021 at 23:27 Eric DeVolder <eric.devolder@oracle.com> wrote: > Hi Ani, > Thanks for such quick feedback! One inline response below. > eric > > On 12/15/21 10:33, Ani Sinha wrote: > > On Wed, Dec 15, 2021 at 9:08 PM Eric DeVolder <eric.devolder@oracle.com> > 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 bb6cad4..05177b3 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; > > > > This variable can be eliminated (see below). > > > >> + 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(action, 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(action, 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(action, 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(action, 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(action, 32, ERST_ACTION_OFFSET, action); > >> + > >> + action = ACTION_BEGIN_READ_OPERATION; > >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > >> + > >> + action = ACTION_BEGIN_CLEAR_OPERATION; > >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > >> + > >> + action = ACTION_END_OPERATION; > >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > >> + > >> + action = ACTION_SET_RECORD_OFFSET; > >> + build_write_register(action, 32, ERST_VALUE_OFFSET, 0); > >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > >> + > >> + action = ACTION_EXECUTE_OPERATION; > >> + build_write_register_value(action, 32, ERST_VALUE_OFFSET, > >> + ERST_EXECUTE_OPERATION_MAGIC); > >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > > > > > > > >> + > >> + action = ACTION_CHECK_BUSY_STATUS; > >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > >> + build_read_register_value(action, 32, ERST_VALUE_OFFSET, 0x01); > >> + > >> + action = ACTION_GET_COMMAND_STATUS; > >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > >> + build_read_register(action, 32, ERST_VALUE_OFFSET); > >> + > >> + action = ACTION_GET_RECORD_IDENTIFIER; > >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > >> + build_read_register(action, 64, ERST_VALUE_OFFSET); > >> + > >> + action = ACTION_SET_RECORD_IDENTIFIER; > >> + build_write_register(action, 64, ERST_VALUE_OFFSET, 0); > >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > >> + > >> + action = ACTION_GET_RECORD_COUNT; > >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > >> + build_read_register(action, 32, ERST_VALUE_OFFSET); > >> + > >> + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION; > >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > >> + > >> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE; > >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > >> + build_read_register(action, 64, ERST_VALUE_OFFSET); > >> + > >> + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH; > >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > >> + build_read_register(action, 64, ERST_VALUE_OFFSET); > >> + > >> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES; > >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > >> + build_read_register(action, 32, ERST_VALUE_OFFSET); > >> + > >> + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS; > >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > >> + build_read_register(action, 64, ERST_VALUE_OFFSET); > > > > if I am reading this right, build_write_register_value() is called > > with the same parameters, except that action is changing. We can > > optimize repetative calls with the same parameters. > > > > build_read_register can be split into build_read_register_32() and > > build_read_register_64(). > > So we can have : > > build_write_register_value(ACTION_GET_EXECUTE_OPERATION_TIMINGS); > > build_read_register_64(ACTION_GET_EXECUTE_OPERATION_TIMINGS); > > > > If I understand correctly, you are essentially asking for appropriate > accessor functions. > I did an inventory and the following would be the list of unique accessors > needed: > > To ACTION register: > write_value_32 > > To VALUE register: > write_value_32 > write_32 > write_64 > read_value_32 > read_32 > read_64 > > So that is 7 accessors, which must spell out the access type and register > name in the macro. > > With respect to the comment on eliminating the action variable. Given the > current code I did miss an > optimization to avoid passing 'action' as the first parameter to the > macros; I should have just used > it directly within the macro. Plus the 'action' assignment acts as > documentation to inform you as to > which action is being constructed. But in other places, 'action' is being > used as a true parameter > to the macros as the value to write. > > If you think that going to more accessors is simpler/more clear, then I'll > do so. Let’s wait and see what Michael thinks about this. > > Thanks! > eric > > > > > >> + > >> + /* 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 Wed, Dec 15, 2021 at 10:38:11AM -0500, 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 bb6cad4..05177b3 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(action, 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(action, 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(action, 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(action, 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'm not sure why these are macros not functions. But assuming it's preferable this way, pls make them use ALL_CAPS as per QEMU coding style. > + > + /* Serialization Instruction Entries */ > + action = ACTION_BEGIN_WRITE_OPERATION; > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + > + action = ACTION_BEGIN_READ_OPERATION; > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + > + action = ACTION_BEGIN_CLEAR_OPERATION; > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + > + action = ACTION_END_OPERATION; > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + > + action = ACTION_SET_RECORD_OFFSET; > + build_write_register(action, 32, ERST_VALUE_OFFSET, 0); > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + > + action = ACTION_EXECUTE_OPERATION; > + build_write_register_value(action, 32, ERST_VALUE_OFFSET, > + ERST_EXECUTE_OPERATION_MAGIC); > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + > + action = ACTION_CHECK_BUSY_STATUS; > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + build_read_register_value(action, 32, ERST_VALUE_OFFSET, 0x01); > + > + action = ACTION_GET_COMMAND_STATUS; > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + build_read_register(action, 32, ERST_VALUE_OFFSET); > + > + action = ACTION_GET_RECORD_IDENTIFIER; > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + build_read_register(action, 64, ERST_VALUE_OFFSET); > + > + action = ACTION_SET_RECORD_IDENTIFIER; > + build_write_register(action, 64, ERST_VALUE_OFFSET, 0); > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + > + action = ACTION_GET_RECORD_COUNT; > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + build_read_register(action, 32, ERST_VALUE_OFFSET); > + > + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION; > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE; > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + build_read_register(action, 64, ERST_VALUE_OFFSET); > + > + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH; > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + build_read_register(action, 64, ERST_VALUE_OFFSET); > + > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES; > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + build_read_register(action, 32, ERST_VALUE_OFFSET); > + > + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS; > + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); > + build_read_register(action, 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; > -- > 1.8.3.1
Thanks for looking at this Michael, I've an inline response below. eric On 1/6/22 04:45, Michael S. Tsirkin wrote: > On Wed, Dec 15, 2021 at 10:38:11AM -0500, 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 bb6cad4..05177b3 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(action, 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(action, 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(action, 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(action, 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'm not sure why these are macros not functions. > But assuming it's preferable this way, pls make them > use ALL_CAPS as per QEMU coding style. > For brevity I chose to have these 4 macros of 4 lines each, rather than replicate 4 functions of 25 lines each. I have uppercased the macros to follow style.rst. I've posted v12 with these changes. Thanks! eric > >> + >> + /* Serialization Instruction Entries */ >> + action = ACTION_BEGIN_WRITE_OPERATION; >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + >> + action = ACTION_BEGIN_READ_OPERATION; >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + >> + action = ACTION_BEGIN_CLEAR_OPERATION; >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + >> + action = ACTION_END_OPERATION; >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + >> + action = ACTION_SET_RECORD_OFFSET; >> + build_write_register(action, 32, ERST_VALUE_OFFSET, 0); >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + >> + action = ACTION_EXECUTE_OPERATION; >> + build_write_register_value(action, 32, ERST_VALUE_OFFSET, >> + ERST_EXECUTE_OPERATION_MAGIC); >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + >> + action = ACTION_CHECK_BUSY_STATUS; >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + build_read_register_value(action, 32, ERST_VALUE_OFFSET, 0x01); >> + >> + action = ACTION_GET_COMMAND_STATUS; >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + build_read_register(action, 32, ERST_VALUE_OFFSET); >> + >> + action = ACTION_GET_RECORD_IDENTIFIER; >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + build_read_register(action, 64, ERST_VALUE_OFFSET); >> + >> + action = ACTION_SET_RECORD_IDENTIFIER; >> + build_write_register(action, 64, ERST_VALUE_OFFSET, 0); >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + >> + action = ACTION_GET_RECORD_COUNT; >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + build_read_register(action, 32, ERST_VALUE_OFFSET); >> + >> + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION; >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + >> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE; >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + build_read_register(action, 64, ERST_VALUE_OFFSET); >> + >> + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH; >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + build_read_register(action, 64, ERST_VALUE_OFFSET); >> + >> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES; >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + build_read_register(action, 32, ERST_VALUE_OFFSET); >> + >> + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS; >> + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); >> + build_read_register(action, 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; >> -- >> 1.8.3.1 >
diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c index bb6cad4..05177b3 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(action, 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(action, 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(action, 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(action, 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(action, 32, ERST_ACTION_OFFSET, action); + + action = ACTION_BEGIN_READ_OPERATION; + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); + + action = ACTION_BEGIN_CLEAR_OPERATION; + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); + + action = ACTION_END_OPERATION; + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); + + action = ACTION_SET_RECORD_OFFSET; + build_write_register(action, 32, ERST_VALUE_OFFSET, 0); + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); + + action = ACTION_EXECUTE_OPERATION; + build_write_register_value(action, 32, ERST_VALUE_OFFSET, + ERST_EXECUTE_OPERATION_MAGIC); + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); + + action = ACTION_CHECK_BUSY_STATUS; + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); + build_read_register_value(action, 32, ERST_VALUE_OFFSET, 0x01); + + action = ACTION_GET_COMMAND_STATUS; + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); + build_read_register(action, 32, ERST_VALUE_OFFSET); + + action = ACTION_GET_RECORD_IDENTIFIER; + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); + build_read_register(action, 64, ERST_VALUE_OFFSET); + + action = ACTION_SET_RECORD_IDENTIFIER; + build_write_register(action, 64, ERST_VALUE_OFFSET, 0); + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); + + action = ACTION_GET_RECORD_COUNT; + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); + build_read_register(action, 32, ERST_VALUE_OFFSET); + + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION; + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); + + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE; + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); + build_read_register(action, 64, ERST_VALUE_OFFSET); + + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH; + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); + build_read_register(action, 64, ERST_VALUE_OFFSET); + + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES; + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); + build_read_register(action, 32, ERST_VALUE_OFFSET); + + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS; + build_write_register_value(action, 32, ERST_ACTION_OFFSET, action); + build_read_register(action, 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(+)