On Wed, Dec 15, 2021 at 23:27 Eric DeVolder 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 > wrote: > >> > >> This builds the ACPI ERST table to inform OSPM how to communicate > >> with the acpi-erst device. > >> > >> Signed-off-by: Eric DeVolder > >> --- > >> 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 > >> >