| Message ID | 1639072655-19271-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 Thu, Dec 9, 2021 at 11:28 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. This patch starts in the middle of pci device code addition, between erst_reg_ops and erst_post_load. I do not like this :( > > Signed-off-by: Eric DeVolder <eric.devolder@oracle.com> > --- > hw/acpi/erst.c | 241 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 241 insertions(+) > > diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c > index 81f5435..753425a 100644 > --- a/hw/acpi/erst.c > +++ b/hw/acpi/erst.c > @@ -711,6 +711,247 @@ static const MemoryRegionOps erst_reg_ops = { > .endianness = DEVICE_NATIVE_ENDIAN, > }; > > + > +/*******************************************************************/ > +/*******************************************************************/ > + > +/* 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 I prefer these definitions to come at the top of the file along with other definitions. > + > +/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */ > +static void build_serialization_instruction_entry(GArray *table_data, This function and buiild_erst() can come at the end of erst.c. They go together and are doing a common but different operation from the operations of the pci device - building the erst table. Hence, ther code should be separate from pci device code. A new file would be an overkill at this state IMHO but in the future if erst table generation code gains more weight, it can be split into two files. > + uint8_t serialization_action, > + uint8_t instruction, > + uint8_t flags, > + uint8_t register_bit_width, > + uint64_t register_address, > + uint64_t value, > + uint64_t mask) > +{ > + /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */ > + struct AcpiGenericAddress gas; > + > + /* 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; > + switch (register_bit_width) { > + case 8: > + gas.access_width = 1; > + break; > + case 16: > + gas.access_width = 2; > + break; > + case 32: > + gas.access_width = 3; > + break; > + case 64: > + gas.access_width = 4; > + break; > + default: > + gas.access_width = 0; > + break; > + } > + gas.address = register_address; > + build_append_gas_from_struct(table_data, &gas); > + /* Value */ > + build_append_int_noprefix(table_data, value , 8); > + /* Mask */ > + 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, bar1; > + AcpiTable table = { .sig = "ERST", .rev = 1, .oem_id = oem_id, > + .oem_table_id = oem_table_id }; > + > + bar0 = (pcibus_t)pci_get_bar_addr(PCI_DEVICE(erst_dev), 0); > + trace_acpi_erst_pci_bar_0(bar0); > + bar1 = (pcibus_t)pci_get_bar_addr(PCI_DEVICE(erst_dev), 1); > + trace_acpi_erst_pci_bar_1(bar1); > + > +#define MASK8 0x00000000000000FFUL > +#define MASK16 0x000000000000FFFFUL > +#define MASK32 0x00000000FFFFFFFFUL > +#define MASK64 0xFFFFFFFFFFFFFFFFUL > + > + /* > + * 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)); > + > + /* Serialization Instruction Entries */ > + action = ACTION_BEGIN_WRITE_OPERATION; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + > + action = ACTION_BEGIN_READ_OPERATION; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + > + action = ACTION_BEGIN_CLEAR_OPERATION; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + > + action = ACTION_END_OPERATION; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + > + action = ACTION_SET_RECORD_OFFSET; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER , 0, 32, > + bar0 + ERST_VALUE_OFFSET , 0, MASK32); > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + > + action = ACTION_EXECUTE_OPERATION; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_VALUE_OFFSET , ERST_EXECUTE_OPERATION_MAGIC, MASK8); > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + > + action = ACTION_CHECK_BUSY_STATUS; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_READ_REGISTER_VALUE , 0, 32, > + bar0 + ERST_VALUE_OFFSET, 0x01, MASK8); > + > + action = ACTION_GET_COMMAND_STATUS; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_READ_REGISTER , 0, 32, > + bar0 + ERST_VALUE_OFFSET, 0, MASK8); > + > + action = ACTION_GET_RECORD_IDENTIFIER; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_READ_REGISTER , 0, 64, > + bar0 + ERST_VALUE_OFFSET, 0, MASK64); > + > + action = ACTION_SET_RECORD_IDENTIFIER; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER , 0, 64, > + bar0 + ERST_VALUE_OFFSET , 0, MASK64); > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + > + action = ACTION_GET_RECORD_COUNT; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_READ_REGISTER , 0, 32, > + bar0 + ERST_VALUE_OFFSET, 0, MASK32); > + > + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_READ_REGISTER , 0, 64, > + bar0 + ERST_VALUE_OFFSET, 0, MASK64); > + > + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_READ_REGISTER , 0, 64, > + bar0 + ERST_VALUE_OFFSET, 0, MASK32); > + > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_READ_REGISTER , 0, 32, > + bar0 + ERST_VALUE_OFFSET, 0, MASK32); > + > + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_READ_REGISTER , 0, 64, > + bar0 + ERST_VALUE_OFFSET, 0, MASK64); > + > + /* 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 > + */ > + 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 int erst_post_load(void *opaque, int version_id) > -- > 1.8.3.1 >
On Thu, Dec 09, 2021 at 12:57:31PM -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 | 241 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 241 insertions(+) > > diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c > index 81f5435..753425a 100644 > --- a/hw/acpi/erst.c > +++ b/hw/acpi/erst.c > @@ -711,6 +711,247 @@ static const MemoryRegionOps erst_reg_ops = { > .endianness = DEVICE_NATIVE_ENDIAN, > }; > > + > +/*******************************************************************/ > +/*******************************************************************/ > + > +/* 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 > + I would create wrappers for the specific uses that we do have. Leave the rest alone. You just use 4 of these right? And a bunch of parameters are always the same. E.g. flags always 0, address always same. > +/* 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, maybe make it width in bytes, then you do not need a switch. > + uint64_t register_address, > + uint64_t value, > + uint64_t mask) > +{ > + /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */ > + struct AcpiGenericAddress gas; > + > + /* 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; > + switch (register_bit_width) { > + case 8: > + gas.access_width = 1; > + break; > + case 16: > + gas.access_width = 2; > + break; > + case 32: > + gas.access_width = 3; > + break; > + case 64: > + gas.access_width = 4; > + break; > + default: > + gas.access_width = 0; > + break; does this default actually work? > + } > + gas.address = register_address; > + build_append_gas_from_struct(table_data, &gas); > + /* Value */ > + build_append_int_noprefix(table_data, value , 8); > + /* Mask */ > + 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, bar1; > + AcpiTable table = { .sig = "ERST", .rev = 1, .oem_id = oem_id, > + .oem_table_id = oem_table_id }; > + > + bar0 = (pcibus_t)pci_get_bar_addr(PCI_DEVICE(erst_dev), 0); > + trace_acpi_erst_pci_bar_0(bar0); > + bar1 = (pcibus_t)pci_get_bar_addr(PCI_DEVICE(erst_dev), 1); why do we need the cast here? Also assignment at declaration point will be cleaner, won't it? > + trace_acpi_erst_pci_bar_1(bar1); bar1 seems unused ... why do we bother with it just for trace? > + > +#define MASK8 0x00000000000000FFUL > +#define MASK16 0x000000000000FFFFUL > +#define MASK32 0x00000000FFFFFFFFUL > +#define MASK64 0xFFFFFFFFFFFFFFFFUL can't we just pass # of bytes? > + > + /* > + * 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)); > + > + /* Serialization Instruction Entries */ > + action = ACTION_BEGIN_WRITE_OPERATION; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + > + action = ACTION_BEGIN_READ_OPERATION; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + > + action = ACTION_BEGIN_CLEAR_OPERATION; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + > + action = ACTION_END_OPERATION; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + > + action = ACTION_SET_RECORD_OFFSET; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER , 0, 32, > + bar0 + ERST_VALUE_OFFSET , 0, MASK32); > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + > + action = ACTION_EXECUTE_OPERATION; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_VALUE_OFFSET , ERST_EXECUTE_OPERATION_MAGIC, MASK8); > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + > + action = ACTION_CHECK_BUSY_STATUS; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_READ_REGISTER_VALUE , 0, 32, > + bar0 + ERST_VALUE_OFFSET, 0x01, MASK8); > + > + action = ACTION_GET_COMMAND_STATUS; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_READ_REGISTER , 0, 32, > + bar0 + ERST_VALUE_OFFSET, 0, MASK8); > + > + action = ACTION_GET_RECORD_IDENTIFIER; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_READ_REGISTER , 0, 64, > + bar0 + ERST_VALUE_OFFSET, 0, MASK64); > + > + action = ACTION_SET_RECORD_IDENTIFIER; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER , 0, 64, > + bar0 + ERST_VALUE_OFFSET , 0, MASK64); > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + > + action = ACTION_GET_RECORD_COUNT; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_READ_REGISTER , 0, 32, > + bar0 + ERST_VALUE_OFFSET, 0, MASK32); > + > + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_READ_REGISTER , 0, 64, > + bar0 + ERST_VALUE_OFFSET, 0, MASK64); > + > + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_READ_REGISTER , 0, 64, > + bar0 + ERST_VALUE_OFFSET, 0, MASK32); > + > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_READ_REGISTER , 0, 32, > + bar0 + ERST_VALUE_OFFSET, 0, MASK32); > + > + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS; > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > + build_serialization_instruction_entry(table_instruction_data, > + action, INST_READ_REGISTER , 0, 64, > + bar0 + ERST_VALUE_OFFSET, 0, MASK64); > + > + /* 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 > + */ assert that it's a multiple of 32 maybe > + 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 int erst_post_load(void *opaque, int version_id) > -- > 1.8.3.1
Hi Ani, inline response below. Eric On 12/12/21 07:43, Ani Sinha wrote: > . > > On Thu, Dec 9, 2021 at 11:28 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. > > This patch starts in the middle of pci device code addition, between > erst_reg_ops and erst_post_load. I do not like this :( Below you suggest moving the contents of this patch to the bottom of erst.c. Before I do that, consider moving the contents to the top of the file instead, I believe that would address the concerns cited here, and it would allow for the last line of the file to be the type_init(), like other files. I'll move it, just let me know if top or bottom. Thanks! eric > >> >> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com> >> --- >> hw/acpi/erst.c | 241 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 241 insertions(+) >> >> diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c >> index 81f5435..753425a 100644 >> --- a/hw/acpi/erst.c >> +++ b/hw/acpi/erst.c >> @@ -711,6 +711,247 @@ static const MemoryRegionOps erst_reg_ops = { >> .endianness = DEVICE_NATIVE_ENDIAN, >> }; >> >> + >> +/*******************************************************************/ >> +/*******************************************************************/ >> + >> +/* 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 > > I prefer these definitions to come at the top of the file along with > other definitions. > >> + >> +/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */ >> +static void build_serialization_instruction_entry(GArray *table_data, > > This function and buiild_erst() can come at the end of erst.c. They go > together and are doing a common but different operation from the > operations of the pci device - building the erst table. Hence, ther > code should be separate from pci device code. A new file would be an > overkill at this state IMHO but in the future if erst table generation > code gains more weight, it can be split into two files. > >> + uint8_t serialization_action, >> + uint8_t instruction, >> + uint8_t flags, >> + uint8_t register_bit_width, >> + uint64_t register_address, >> + uint64_t value, >> + uint64_t mask) >> +{ >> + /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */ >> + struct AcpiGenericAddress gas; >> + >> + /* 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; >> + switch (register_bit_width) { >> + case 8: >> + gas.access_width = 1; >> + break; >> + case 16: >> + gas.access_width = 2; >> + break; >> + case 32: >> + gas.access_width = 3; >> + break; >> + case 64: >> + gas.access_width = 4; >> + break; >> + default: >> + gas.access_width = 0; >> + break; >> + } >> + gas.address = register_address; >> + build_append_gas_from_struct(table_data, &gas); >> + /* Value */ >> + build_append_int_noprefix(table_data, value , 8); >> + /* Mask */ >> + 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, bar1; >> + AcpiTable table = { .sig = "ERST", .rev = 1, .oem_id = oem_id, >> + .oem_table_id = oem_table_id }; >> + >> + bar0 = (pcibus_t)pci_get_bar_addr(PCI_DEVICE(erst_dev), 0); >> + trace_acpi_erst_pci_bar_0(bar0); >> + bar1 = (pcibus_t)pci_get_bar_addr(PCI_DEVICE(erst_dev), 1); >> + trace_acpi_erst_pci_bar_1(bar1); >> + >> +#define MASK8 0x00000000000000FFUL >> +#define MASK16 0x000000000000FFFFUL >> +#define MASK32 0x00000000FFFFFFFFUL >> +#define MASK64 0xFFFFFFFFFFFFFFFFUL >> + >> + /* >> + * 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)); >> + >> + /* Serialization Instruction Entries */ >> + action = ACTION_BEGIN_WRITE_OPERATION; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + >> + action = ACTION_BEGIN_READ_OPERATION; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + >> + action = ACTION_BEGIN_CLEAR_OPERATION; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + >> + action = ACTION_END_OPERATION; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + >> + action = ACTION_SET_RECORD_OFFSET; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER , 0, 32, >> + bar0 + ERST_VALUE_OFFSET , 0, MASK32); >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + >> + action = ACTION_EXECUTE_OPERATION; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_VALUE_OFFSET , ERST_EXECUTE_OPERATION_MAGIC, MASK8); >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + >> + action = ACTION_CHECK_BUSY_STATUS; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_READ_REGISTER_VALUE , 0, 32, >> + bar0 + ERST_VALUE_OFFSET, 0x01, MASK8); >> + >> + action = ACTION_GET_COMMAND_STATUS; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_READ_REGISTER , 0, 32, >> + bar0 + ERST_VALUE_OFFSET, 0, MASK8); >> + >> + action = ACTION_GET_RECORD_IDENTIFIER; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_READ_REGISTER , 0, 64, >> + bar0 + ERST_VALUE_OFFSET, 0, MASK64); >> + >> + action = ACTION_SET_RECORD_IDENTIFIER; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER , 0, 64, >> + bar0 + ERST_VALUE_OFFSET , 0, MASK64); >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + >> + action = ACTION_GET_RECORD_COUNT; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_READ_REGISTER , 0, 32, >> + bar0 + ERST_VALUE_OFFSET, 0, MASK32); >> + >> + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + >> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_READ_REGISTER , 0, 64, >> + bar0 + ERST_VALUE_OFFSET, 0, MASK64); >> + >> + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_READ_REGISTER , 0, 64, >> + bar0 + ERST_VALUE_OFFSET, 0, MASK32); >> + >> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_READ_REGISTER , 0, 32, >> + bar0 + ERST_VALUE_OFFSET, 0, MASK32); >> + >> + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_READ_REGISTER , 0, 64, >> + bar0 + ERST_VALUE_OFFSET, 0, MASK64); >> + >> + /* 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 >> + */ >> + 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 int erst_post_load(void *opaque, int version_id) >> -- >> 1.8.3.1 >>
Michael, Thanks for reviewing! Inline responses below. eric On 12/12/21 16:56, Michael S. Tsirkin wrote: > On Thu, Dec 09, 2021 at 12:57:31PM -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 | 241 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 241 insertions(+) >> >> diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c >> index 81f5435..753425a 100644 >> --- a/hw/acpi/erst.c >> +++ b/hw/acpi/erst.c >> @@ -711,6 +711,247 @@ static const MemoryRegionOps erst_reg_ops = { >> .endianness = DEVICE_NATIVE_ENDIAN, >> }; >> >> + >> +/*******************************************************************/ >> +/*******************************************************************/ >> + >> +/* 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 >> + > > I would create wrappers for the specific uses that we do have. Leave the > rest alone. > You just use 4 of these right? And a bunch of parameters are > always the same. E.g. flags always 0, address always same. If I understand correctly, I think you are suggesting making wrappers for the 4 in use (ie WRITE, WRITE_VALUE, READ, READ_VALUE). in an effort to simplify/hide the underlying call to build_serialization_instruction_entry(). OK, I'll do that. > >> +/* 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, > > maybe make it width in bytes, then you do not need a switch. I can make it bytes, but the switch is still needed as the corresponding field is an encoding (ie 1,2,3,4 vs 1, 2, 4, 8). > >> + uint64_t register_address, >> + uint64_t value, >> + uint64_t mask) >> +{ >> + /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */ >> + struct AcpiGenericAddress gas; >> + >> + /* 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; >> + switch (register_bit_width) { >> + case 8: >> + gas.access_width = 1; >> + break; >> + case 16: >> + gas.access_width = 2; >> + break; >> + case 32: >> + gas.access_width = 3; >> + break; >> + case 64: >> + gas.access_width = 4; >> + break; >> + default: >> + gas.access_width = 0; >> + break; > > does this default actually work? I actually have no idea. But given that this is driven by code in this file, I'll set an assert there instead; this should never happen. > >> + } >> + gas.address = register_address; >> + build_append_gas_from_struct(table_data, &gas); >> + /* Value */ >> + build_append_int_noprefix(table_data, value , 8); >> + /* Mask */ >> + 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, bar1; >> + AcpiTable table = { .sig = "ERST", .rev = 1, .oem_id = oem_id, >> + .oem_table_id = oem_table_id }; >> + >> + bar0 = (pcibus_t)pci_get_bar_addr(PCI_DEVICE(erst_dev), 0); >> + trace_acpi_erst_pci_bar_0(bar0); >> + bar1 = (pcibus_t)pci_get_bar_addr(PCI_DEVICE(erst_dev), 1); > > why do we need the cast here? > Also assignment at declaration point will be cleaner, won't it? Corrected; cast removed and assigned at declaration. > >> + trace_acpi_erst_pci_bar_1(bar1); > > bar1 seems unused ... why do we bother with it just for trace? Corrected; bar1 not needed. > >> + >> +#define MASK8 0x00000000000000FFUL >> +#define MASK16 0x000000000000FFFFUL >> +#define MASK32 0x00000000FFFFFFFFUL >> +#define MASK64 0xFFFFFFFFFFFFFFFFUL > > > can't we just pass # of bytes? I could, but then I'd need a switch statement to convert to one of these masks. The full mask is embedded in the generic address structure. Perhaps in the wrapper I can use width in bytes and the switch statement can producing the encoding and the mask... > >> + >> + /* >> + * 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)); >> + >> + /* Serialization Instruction Entries */ >> + action = ACTION_BEGIN_WRITE_OPERATION; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + >> + action = ACTION_BEGIN_READ_OPERATION; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + >> + action = ACTION_BEGIN_CLEAR_OPERATION; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + >> + action = ACTION_END_OPERATION; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + >> + action = ACTION_SET_RECORD_OFFSET; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER , 0, 32, >> + bar0 + ERST_VALUE_OFFSET , 0, MASK32); >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + >> + action = ACTION_EXECUTE_OPERATION; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_VALUE_OFFSET , ERST_EXECUTE_OPERATION_MAGIC, MASK8); >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + >> + action = ACTION_CHECK_BUSY_STATUS; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_READ_REGISTER_VALUE , 0, 32, >> + bar0 + ERST_VALUE_OFFSET, 0x01, MASK8); >> + >> + action = ACTION_GET_COMMAND_STATUS; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_READ_REGISTER , 0, 32, >> + bar0 + ERST_VALUE_OFFSET, 0, MASK8); >> + >> + action = ACTION_GET_RECORD_IDENTIFIER; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_READ_REGISTER , 0, 64, >> + bar0 + ERST_VALUE_OFFSET, 0, MASK64); >> + >> + action = ACTION_SET_RECORD_IDENTIFIER; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER , 0, 64, >> + bar0 + ERST_VALUE_OFFSET , 0, MASK64); >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + >> + action = ACTION_GET_RECORD_COUNT; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_READ_REGISTER , 0, 32, >> + bar0 + ERST_VALUE_OFFSET, 0, MASK32); >> + >> + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + >> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_READ_REGISTER , 0, 64, >> + bar0 + ERST_VALUE_OFFSET, 0, MASK64); >> + >> + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_READ_REGISTER , 0, 64, >> + bar0 + ERST_VALUE_OFFSET, 0, MASK32); >> + >> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_READ_REGISTER , 0, 32, >> + bar0 + ERST_VALUE_OFFSET, 0, MASK32); >> + >> + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS; >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >> + build_serialization_instruction_entry(table_instruction_data, >> + action, INST_READ_REGISTER , 0, 64, >> + bar0 + ERST_VALUE_OFFSET, 0, MASK64); >> + >> + /* 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 >> + */ > > assert that it's a multiple of 32 maybe done! > >> + 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 int erst_post_load(void *opaque, int version_id) >> -- >> 1.8.3.1 >
On Mon, Dec 13, 2021 at 04:02:26PM -0600, Eric DeVolder wrote: > Michael, > Thanks for reviewing! Inline responses below. > eric > > On 12/12/21 16:56, Michael S. Tsirkin wrote: > > On Thu, Dec 09, 2021 at 12:57:31PM -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 | 241 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 241 insertions(+) > > > > > > diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c > > > index 81f5435..753425a 100644 > > > --- a/hw/acpi/erst.c > > > +++ b/hw/acpi/erst.c > > > @@ -711,6 +711,247 @@ static const MemoryRegionOps erst_reg_ops = { > > > .endianness = DEVICE_NATIVE_ENDIAN, > > > }; > > > + > > > +/*******************************************************************/ > > > +/*******************************************************************/ > > > + > > > +/* 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 > > > + > > > > I would create wrappers for the specific uses that we do have. Leave the > > rest alone. > > You just use 4 of these right? And a bunch of parameters are > > always the same. E.g. flags always 0, address always same. > > If I understand correctly, I think you are suggesting making wrappers for > the 4 in use (ie WRITE, WRITE_VALUE, READ, READ_VALUE). in an effort to > simplify/hide the underlying call to > build_serialization_instruction_entry(). OK, I'll do that. > > > > > > +/* 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, > > > > maybe make it width in bytes, then you do not need a switch. > > I can make it bytes, but the switch is still needed as the corresponding > field is an encoding (ie 1,2,3,4 vs 1, 2, 4, 8). So it's ffs basically? > > > > > + uint64_t register_address, > > > + uint64_t value, > > > + uint64_t mask) > > > +{ > > > + /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */ > > > + struct AcpiGenericAddress gas; > > > + > > > + /* 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; > > > + switch (register_bit_width) { > > > + case 8: > > > + gas.access_width = 1; > > > + break; > > > + case 16: > > > + gas.access_width = 2; > > > + break; > > > + case 32: > > > + gas.access_width = 3; > > > + break; > > > + case 64: > > > + gas.access_width = 4; > > > + break; > > > + default: > > > + gas.access_width = 0; > > > + break; > > > > does this default actually work? > I actually have no idea. But given that this is driven by code in this file, > I'll set an assert there instead; this should never happen. > > > > > > + } > > > + gas.address = register_address; > > > + build_append_gas_from_struct(table_data, &gas); > > > + /* Value */ > > > + build_append_int_noprefix(table_data, value , 8); > > > + /* Mask */ > > > + 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, bar1; > > > + AcpiTable table = { .sig = "ERST", .rev = 1, .oem_id = oem_id, > > > + .oem_table_id = oem_table_id }; > > > + > > > + bar0 = (pcibus_t)pci_get_bar_addr(PCI_DEVICE(erst_dev), 0); > > > + trace_acpi_erst_pci_bar_0(bar0); > > > + bar1 = (pcibus_t)pci_get_bar_addr(PCI_DEVICE(erst_dev), 1); > > > > why do we need the cast here? > > Also assignment at declaration point will be cleaner, won't it? > Corrected; cast removed and assigned at declaration. > > > > > + trace_acpi_erst_pci_bar_1(bar1); > > > > bar1 seems unused ... why do we bother with it just for trace? > Corrected; bar1 not needed. > > > > > > + > > > +#define MASK8 0x00000000000000FFUL > > > +#define MASK16 0x000000000000FFFFUL > > > +#define MASK32 0x00000000FFFFFFFFUL > > > +#define MASK64 0xFFFFFFFFFFFFFFFFUL > > > > > > can't we just pass # of bytes? > I could, but then I'd need a switch statement to convert to one of these > masks. The full mask is embedded in the generic address structure. > > Perhaps in the wrapper I can use width in bytes and the switch statement can > producing the encoding and the mask... we don't need a switch, mask is just (1ULL << (bytes * BITS_PER_BYTE - 1) << 1) - 1 encoding is just ffs(bytes) > > > > > + > > > + /* > > > + * 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)); > > > + > > > + /* Serialization Instruction Entries */ > > > + action = ACTION_BEGIN_WRITE_OPERATION; > > > + build_serialization_instruction_entry(table_instruction_data, > > > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > > > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > > > + > > > + action = ACTION_BEGIN_READ_OPERATION; > > > + build_serialization_instruction_entry(table_instruction_data, > > > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > > > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > > > + > > > + action = ACTION_BEGIN_CLEAR_OPERATION; > > > + build_serialization_instruction_entry(table_instruction_data, > > > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > > > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > > > + > > > + action = ACTION_END_OPERATION; > > > + build_serialization_instruction_entry(table_instruction_data, > > > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > > > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > > > + > > > + action = ACTION_SET_RECORD_OFFSET; > > > + build_serialization_instruction_entry(table_instruction_data, > > > + action, INST_WRITE_REGISTER , 0, 32, > > > + bar0 + ERST_VALUE_OFFSET , 0, MASK32); > > > + build_serialization_instruction_entry(table_instruction_data, > > > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > > > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > > > + > > > + action = ACTION_EXECUTE_OPERATION; > > > + build_serialization_instruction_entry(table_instruction_data, > > > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > > > + bar0 + ERST_VALUE_OFFSET , ERST_EXECUTE_OPERATION_MAGIC, MASK8); > > > + build_serialization_instruction_entry(table_instruction_data, > > > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > > > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > > > + > > > + action = ACTION_CHECK_BUSY_STATUS; > > > + build_serialization_instruction_entry(table_instruction_data, > > > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > > > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > > > + build_serialization_instruction_entry(table_instruction_data, > > > + action, INST_READ_REGISTER_VALUE , 0, 32, > > > + bar0 + ERST_VALUE_OFFSET, 0x01, MASK8); > > > + > > > + action = ACTION_GET_COMMAND_STATUS; > > > + build_serialization_instruction_entry(table_instruction_data, > > > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > > > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > > > + build_serialization_instruction_entry(table_instruction_data, > > > + action, INST_READ_REGISTER , 0, 32, > > > + bar0 + ERST_VALUE_OFFSET, 0, MASK8); > > > + > > > + action = ACTION_GET_RECORD_IDENTIFIER; > > > + build_serialization_instruction_entry(table_instruction_data, > > > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > > > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > > > + build_serialization_instruction_entry(table_instruction_data, > > > + action, INST_READ_REGISTER , 0, 64, > > > + bar0 + ERST_VALUE_OFFSET, 0, MASK64); > > > + > > > + action = ACTION_SET_RECORD_IDENTIFIER; > > > + build_serialization_instruction_entry(table_instruction_data, > > > + action, INST_WRITE_REGISTER , 0, 64, > > > + bar0 + ERST_VALUE_OFFSET , 0, MASK64); > > > + build_serialization_instruction_entry(table_instruction_data, > > > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > > > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > > > + > > > + action = ACTION_GET_RECORD_COUNT; > > > + build_serialization_instruction_entry(table_instruction_data, > > > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > > > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > > > + build_serialization_instruction_entry(table_instruction_data, > > > + action, INST_READ_REGISTER , 0, 32, > > > + bar0 + ERST_VALUE_OFFSET, 0, MASK32); > > > + > > > + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION; > > > + build_serialization_instruction_entry(table_instruction_data, > > > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > > > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > > > + > > > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE; > > > + build_serialization_instruction_entry(table_instruction_data, > > > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > > > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > > > + build_serialization_instruction_entry(table_instruction_data, > > > + action, INST_READ_REGISTER , 0, 64, > > > + bar0 + ERST_VALUE_OFFSET, 0, MASK64); > > > + > > > + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH; > > > + build_serialization_instruction_entry(table_instruction_data, > > > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > > > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > > > + build_serialization_instruction_entry(table_instruction_data, > > > + action, INST_READ_REGISTER , 0, 64, > > > + bar0 + ERST_VALUE_OFFSET, 0, MASK32); > > > + > > > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES; > > > + build_serialization_instruction_entry(table_instruction_data, > > > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > > > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > > > + build_serialization_instruction_entry(table_instruction_data, > > > + action, INST_READ_REGISTER , 0, 32, > > > + bar0 + ERST_VALUE_OFFSET, 0, MASK32); > > > + > > > + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS; > > > + build_serialization_instruction_entry(table_instruction_data, > > > + action, INST_WRITE_REGISTER_VALUE, 0, 32, > > > + bar0 + ERST_ACTION_OFFSET, action, MASK8); > > > + build_serialization_instruction_entry(table_instruction_data, > > > + action, INST_READ_REGISTER , 0, 64, > > > + bar0 + ERST_VALUE_OFFSET, 0, MASK64); > > > + > > > + /* 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 > > > + */ > > > > assert that it's a multiple of 32 maybe > done! > > > > > > + 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 int erst_post_load(void *opaque, int version_id) > > > -- > > > 1.8.3.1 > >
On Tue, Dec 14, 2021 at 2:57 AM Eric DeVolder <eric.devolder@oracle.com> wrote: > > Hi Ani, > inline response below. > Eric > > On 12/12/21 07:43, Ani Sinha wrote: > > . > > > > On Thu, Dec 9, 2021 at 11:28 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. > > > > This patch starts in the middle of pci device code addition, between > > erst_reg_ops and erst_post_load. I do not like this :( > > Below you suggest moving the contents of this patch to the bottom of erst.c. > Before I do that, consider moving the contents to the top of the file instead, I believe that would > address the concerns cited here, and it would allow for the last line of the file to be the > type_init(), like other files. > > I'll move it, just let me know if top or bottom. Moving to the top is fine. > Thanks! > eric > > > > > >> > >> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com> > >> --- > >> hw/acpi/erst.c | 241 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 241 insertions(+) > >> > >> diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c > >> index 81f5435..753425a 100644 > >> --- a/hw/acpi/erst.c > >> +++ b/hw/acpi/erst.c > >> @@ -711,6 +711,247 @@ static const MemoryRegionOps erst_reg_ops = { > >> .endianness = DEVICE_NATIVE_ENDIAN, > >> }; > >> > >> + > >> +/*******************************************************************/ > >> +/*******************************************************************/ > >> + > >> +/* 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 > > > > I prefer these definitions to come at the top of the file along with > > other definitions. > > > >> + > >> +/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */ > >> +static void build_serialization_instruction_entry(GArray *table_data, > > > > This function and buiild_erst() can come at the end of erst.c. They go > > together and are doing a common but different operation from the > > operations of the pci device - building the erst table. Hence, ther > > code should be separate from pci device code. A new file would be an > > overkill at this state IMHO but in the future if erst table generation > > code gains more weight, it can be split into two files. > > > >> + uint8_t serialization_action, > >> + uint8_t instruction, > >> + uint8_t flags, > >> + uint8_t register_bit_width, > >> + uint64_t register_address, > >> + uint64_t value, > >> + uint64_t mask) > >> +{ > >> + /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */ > >> + struct AcpiGenericAddress gas; > >> + > >> + /* 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; > >> + switch (register_bit_width) { > >> + case 8: > >> + gas.access_width = 1; > >> + break; > >> + case 16: > >> + gas.access_width = 2; > >> + break; > >> + case 32: > >> + gas.access_width = 3; > >> + break; > >> + case 64: > >> + gas.access_width = 4; > >> + break; > >> + default: > >> + gas.access_width = 0; > >> + break; > >> + } > >> + gas.address = register_address; > >> + build_append_gas_from_struct(table_data, &gas); > >> + /* Value */ > >> + build_append_int_noprefix(table_data, value , 8); > >> + /* Mask */ > >> + 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, bar1; > >> + AcpiTable table = { .sig = "ERST", .rev = 1, .oem_id = oem_id, > >> + .oem_table_id = oem_table_id }; > >> + > >> + bar0 = (pcibus_t)pci_get_bar_addr(PCI_DEVICE(erst_dev), 0); > >> + trace_acpi_erst_pci_bar_0(bar0); > >> + bar1 = (pcibus_t)pci_get_bar_addr(PCI_DEVICE(erst_dev), 1); > >> + trace_acpi_erst_pci_bar_1(bar1); > >> + > >> +#define MASK8 0x00000000000000FFUL > >> +#define MASK16 0x000000000000FFFFUL > >> +#define MASK32 0x00000000FFFFFFFFUL > >> +#define MASK64 0xFFFFFFFFFFFFFFFFUL > >> + > >> + /* > >> + * 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)); > >> + > >> + /* Serialization Instruction Entries */ > >> + action = ACTION_BEGIN_WRITE_OPERATION; > >> + build_serialization_instruction_entry(table_instruction_data, > >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >> + > >> + action = ACTION_BEGIN_READ_OPERATION; > >> + build_serialization_instruction_entry(table_instruction_data, > >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >> + > >> + action = ACTION_BEGIN_CLEAR_OPERATION; > >> + build_serialization_instruction_entry(table_instruction_data, > >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >> + > >> + action = ACTION_END_OPERATION; > >> + build_serialization_instruction_entry(table_instruction_data, > >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >> + > >> + action = ACTION_SET_RECORD_OFFSET; > >> + build_serialization_instruction_entry(table_instruction_data, > >> + action, INST_WRITE_REGISTER , 0, 32, > >> + bar0 + ERST_VALUE_OFFSET , 0, MASK32); > >> + build_serialization_instruction_entry(table_instruction_data, > >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >> + > >> + action = ACTION_EXECUTE_OPERATION; > >> + build_serialization_instruction_entry(table_instruction_data, > >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >> + bar0 + ERST_VALUE_OFFSET , ERST_EXECUTE_OPERATION_MAGIC, MASK8); > >> + build_serialization_instruction_entry(table_instruction_data, > >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >> + > >> + action = ACTION_CHECK_BUSY_STATUS; > >> + build_serialization_instruction_entry(table_instruction_data, > >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >> + build_serialization_instruction_entry(table_instruction_data, > >> + action, INST_READ_REGISTER_VALUE , 0, 32, > >> + bar0 + ERST_VALUE_OFFSET, 0x01, MASK8); > >> + > >> + action = ACTION_GET_COMMAND_STATUS; > >> + build_serialization_instruction_entry(table_instruction_data, > >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >> + build_serialization_instruction_entry(table_instruction_data, > >> + action, INST_READ_REGISTER , 0, 32, > >> + bar0 + ERST_VALUE_OFFSET, 0, MASK8); > >> + > >> + action = ACTION_GET_RECORD_IDENTIFIER; > >> + build_serialization_instruction_entry(table_instruction_data, > >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >> + build_serialization_instruction_entry(table_instruction_data, > >> + action, INST_READ_REGISTER , 0, 64, > >> + bar0 + ERST_VALUE_OFFSET, 0, MASK64); > >> + > >> + action = ACTION_SET_RECORD_IDENTIFIER; > >> + build_serialization_instruction_entry(table_instruction_data, > >> + action, INST_WRITE_REGISTER , 0, 64, > >> + bar0 + ERST_VALUE_OFFSET , 0, MASK64); > >> + build_serialization_instruction_entry(table_instruction_data, > >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >> + > >> + action = ACTION_GET_RECORD_COUNT; > >> + build_serialization_instruction_entry(table_instruction_data, > >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >> + build_serialization_instruction_entry(table_instruction_data, > >> + action, INST_READ_REGISTER , 0, 32, > >> + bar0 + ERST_VALUE_OFFSET, 0, MASK32); > >> + > >> + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION; > >> + build_serialization_instruction_entry(table_instruction_data, > >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >> + > >> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE; > >> + build_serialization_instruction_entry(table_instruction_data, > >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >> + build_serialization_instruction_entry(table_instruction_data, > >> + action, INST_READ_REGISTER , 0, 64, > >> + bar0 + ERST_VALUE_OFFSET, 0, MASK64); > >> + > >> + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH; > >> + build_serialization_instruction_entry(table_instruction_data, > >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >> + build_serialization_instruction_entry(table_instruction_data, > >> + action, INST_READ_REGISTER , 0, 64, > >> + bar0 + ERST_VALUE_OFFSET, 0, MASK32); > >> + > >> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES; > >> + build_serialization_instruction_entry(table_instruction_data, > >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >> + build_serialization_instruction_entry(table_instruction_data, > >> + action, INST_READ_REGISTER , 0, 32, > >> + bar0 + ERST_VALUE_OFFSET, 0, MASK32); > >> + > >> + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS; > >> + build_serialization_instruction_entry(table_instruction_data, > >> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >> + build_serialization_instruction_entry(table_instruction_data, > >> + action, INST_READ_REGISTER , 0, 64, > >> + bar0 + ERST_VALUE_OFFSET, 0, MASK64); > >> + > >> + /* 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 > >> + */ > >> + 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 int erst_post_load(void *opaque, int version_id) > >> -- > >> 1.8.3.1 > >>
Ani, one quick question below. eric On 12/13/21 20:58, Ani Sinha wrote: > On Tue, Dec 14, 2021 at 2:57 AM Eric DeVolder <eric.devolder@oracle.com> wrote: >> >> Hi Ani, >> inline response below. >> Eric >> >> On 12/12/21 07:43, Ani Sinha wrote: >>> . >>> >>> On Thu, Dec 9, 2021 at 11:28 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. >>> >>> This patch starts in the middle of pci device code addition, between >>> erst_reg_ops and erst_post_load. I do not like this :( >> >> Below you suggest moving the contents of this patch to the bottom of erst.c. >> Before I do that, consider moving the contents to the top of the file instead, I believe that would >> address the concerns cited here, and it would allow for the last line of the file to be the >> type_init(), like other files. >> >> I'll move it, just let me know if top or bottom. > > Moving to the top is fine. I've moved this to the top. The question is if you prefer this be integrated into the main erst.c patch, or still separated out? thanks! eric > >> Thanks! >> eric >> >> >>> >>>> >>>> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com> >>>> --- >>>> hw/acpi/erst.c | 241 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 241 insertions(+) >>>> >>>> diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c >>>> index 81f5435..753425a 100644 >>>> --- a/hw/acpi/erst.c >>>> +++ b/hw/acpi/erst.c >>>> @@ -711,6 +711,247 @@ static const MemoryRegionOps erst_reg_ops = { >>>> .endianness = DEVICE_NATIVE_ENDIAN, >>>> }; >>>> >>>> + >>>> +/*******************************************************************/ >>>> +/*******************************************************************/ >>>> + >>>> +/* 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 >>> >>> I prefer these definitions to come at the top of the file along with >>> other definitions. >>> >>>> + >>>> +/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */ >>>> +static void build_serialization_instruction_entry(GArray *table_data, >>> >>> This function and buiild_erst() can come at the end of erst.c. They go >>> together and are doing a common but different operation from the >>> operations of the pci device - building the erst table. Hence, ther >>> code should be separate from pci device code. A new file would be an >>> overkill at this state IMHO but in the future if erst table generation >>> code gains more weight, it can be split into two files. >>> >>>> + uint8_t serialization_action, >>>> + uint8_t instruction, >>>> + uint8_t flags, >>>> + uint8_t register_bit_width, >>>> + uint64_t register_address, >>>> + uint64_t value, >>>> + uint64_t mask) >>>> +{ >>>> + /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */ >>>> + struct AcpiGenericAddress gas; >>>> + >>>> + /* 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; >>>> + switch (register_bit_width) { >>>> + case 8: >>>> + gas.access_width = 1; >>>> + break; >>>> + case 16: >>>> + gas.access_width = 2; >>>> + break; >>>> + case 32: >>>> + gas.access_width = 3; >>>> + break; >>>> + case 64: >>>> + gas.access_width = 4; >>>> + break; >>>> + default: >>>> + gas.access_width = 0; >>>> + break; >>>> + } >>>> + gas.address = register_address; >>>> + build_append_gas_from_struct(table_data, &gas); >>>> + /* Value */ >>>> + build_append_int_noprefix(table_data, value , 8); >>>> + /* Mask */ >>>> + 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, bar1; >>>> + AcpiTable table = { .sig = "ERST", .rev = 1, .oem_id = oem_id, >>>> + .oem_table_id = oem_table_id }; >>>> + >>>> + bar0 = (pcibus_t)pci_get_bar_addr(PCI_DEVICE(erst_dev), 0); >>>> + trace_acpi_erst_pci_bar_0(bar0); >>>> + bar1 = (pcibus_t)pci_get_bar_addr(PCI_DEVICE(erst_dev), 1); >>>> + trace_acpi_erst_pci_bar_1(bar1); >>>> + >>>> +#define MASK8 0x00000000000000FFUL >>>> +#define MASK16 0x000000000000FFFFUL >>>> +#define MASK32 0x00000000FFFFFFFFUL >>>> +#define MASK64 0xFFFFFFFFFFFFFFFFUL >>>> + >>>> + /* >>>> + * 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)); >>>> + >>>> + /* Serialization Instruction Entries */ >>>> + action = ACTION_BEGIN_WRITE_OPERATION; >>>> + build_serialization_instruction_entry(table_instruction_data, >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >>>> + >>>> + action = ACTION_BEGIN_READ_OPERATION; >>>> + build_serialization_instruction_entry(table_instruction_data, >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >>>> + >>>> + action = ACTION_BEGIN_CLEAR_OPERATION; >>>> + build_serialization_instruction_entry(table_instruction_data, >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >>>> + >>>> + action = ACTION_END_OPERATION; >>>> + build_serialization_instruction_entry(table_instruction_data, >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >>>> + >>>> + action = ACTION_SET_RECORD_OFFSET; >>>> + build_serialization_instruction_entry(table_instruction_data, >>>> + action, INST_WRITE_REGISTER , 0, 32, >>>> + bar0 + ERST_VALUE_OFFSET , 0, MASK32); >>>> + build_serialization_instruction_entry(table_instruction_data, >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >>>> + >>>> + action = ACTION_EXECUTE_OPERATION; >>>> + build_serialization_instruction_entry(table_instruction_data, >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + bar0 + ERST_VALUE_OFFSET , ERST_EXECUTE_OPERATION_MAGIC, MASK8); >>>> + build_serialization_instruction_entry(table_instruction_data, >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >>>> + >>>> + action = ACTION_CHECK_BUSY_STATUS; >>>> + build_serialization_instruction_entry(table_instruction_data, >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >>>> + build_serialization_instruction_entry(table_instruction_data, >>>> + action, INST_READ_REGISTER_VALUE , 0, 32, >>>> + bar0 + ERST_VALUE_OFFSET, 0x01, MASK8); >>>> + >>>> + action = ACTION_GET_COMMAND_STATUS; >>>> + build_serialization_instruction_entry(table_instruction_data, >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >>>> + build_serialization_instruction_entry(table_instruction_data, >>>> + action, INST_READ_REGISTER , 0, 32, >>>> + bar0 + ERST_VALUE_OFFSET, 0, MASK8); >>>> + >>>> + action = ACTION_GET_RECORD_IDENTIFIER; >>>> + build_serialization_instruction_entry(table_instruction_data, >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >>>> + build_serialization_instruction_entry(table_instruction_data, >>>> + action, INST_READ_REGISTER , 0, 64, >>>> + bar0 + ERST_VALUE_OFFSET, 0, MASK64); >>>> + >>>> + action = ACTION_SET_RECORD_IDENTIFIER; >>>> + build_serialization_instruction_entry(table_instruction_data, >>>> + action, INST_WRITE_REGISTER , 0, 64, >>>> + bar0 + ERST_VALUE_OFFSET , 0, MASK64); >>>> + build_serialization_instruction_entry(table_instruction_data, >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >>>> + >>>> + action = ACTION_GET_RECORD_COUNT; >>>> + build_serialization_instruction_entry(table_instruction_data, >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >>>> + build_serialization_instruction_entry(table_instruction_data, >>>> + action, INST_READ_REGISTER , 0, 32, >>>> + bar0 + ERST_VALUE_OFFSET, 0, MASK32); >>>> + >>>> + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION; >>>> + build_serialization_instruction_entry(table_instruction_data, >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >>>> + >>>> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE; >>>> + build_serialization_instruction_entry(table_instruction_data, >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >>>> + build_serialization_instruction_entry(table_instruction_data, >>>> + action, INST_READ_REGISTER , 0, 64, >>>> + bar0 + ERST_VALUE_OFFSET, 0, MASK64); >>>> + >>>> + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH; >>>> + build_serialization_instruction_entry(table_instruction_data, >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >>>> + build_serialization_instruction_entry(table_instruction_data, >>>> + action, INST_READ_REGISTER , 0, 64, >>>> + bar0 + ERST_VALUE_OFFSET, 0, MASK32); >>>> + >>>> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES; >>>> + build_serialization_instruction_entry(table_instruction_data, >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >>>> + build_serialization_instruction_entry(table_instruction_data, >>>> + action, INST_READ_REGISTER , 0, 32, >>>> + bar0 + ERST_VALUE_OFFSET, 0, MASK32); >>>> + >>>> + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS; >>>> + build_serialization_instruction_entry(table_instruction_data, >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); >>>> + build_serialization_instruction_entry(table_instruction_data, >>>> + action, INST_READ_REGISTER , 0, 64, >>>> + bar0 + ERST_VALUE_OFFSET, 0, MASK64); >>>> + >>>> + /* 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 >>>> + */ >>>> + 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 int erst_post_load(void *opaque, int version_id) >>>> -- >>>> 1.8.3.1 >>>>
On Tue, Dec 14, 2021 at 11:42 PM Eric DeVolder <eric.devolder@oracle.com> wrote: > > Ani, one quick question below. > eric > > On 12/13/21 20:58, Ani Sinha wrote: > > On Tue, Dec 14, 2021 at 2:57 AM Eric DeVolder <eric.devolder@oracle.com> wrote: > >> > >> Hi Ani, > >> inline response below. > >> Eric > >> > >> On 12/12/21 07:43, Ani Sinha wrote: > >>> . > >>> > >>> On Thu, Dec 9, 2021 at 11:28 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. > >>> > >>> This patch starts in the middle of pci device code addition, between > >>> erst_reg_ops and erst_post_load. I do not like this :( > >> > >> Below you suggest moving the contents of this patch to the bottom of erst.c. > >> Before I do that, consider moving the contents to the top of the file instead, I believe that would > >> address the concerns cited here, and it would allow for the last line of the file to be the > >> type_init(), like other files. > >> > >> I'll move it, just let me know if top or bottom. > > > > Moving to the top is fine. > I've moved this to the top. The question is if you prefer this be integrated into the main erst.c > patch, or still separated out? you can keep this separate no problem. Then you can incorporate mine and michael's suggestions into it. > thanks! > eric > > > > >> Thanks! > >> eric > >> > >> > >>> > >>>> > >>>> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com> > >>>> --- > >>>> hw/acpi/erst.c | 241 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > >>>> 1 file changed, 241 insertions(+) > >>>> > >>>> diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c > >>>> index 81f5435..753425a 100644 > >>>> --- a/hw/acpi/erst.c > >>>> +++ b/hw/acpi/erst.c > >>>> @@ -711,6 +711,247 @@ static const MemoryRegionOps erst_reg_ops = { > >>>> .endianness = DEVICE_NATIVE_ENDIAN, > >>>> }; > >>>> > >>>> + > >>>> +/*******************************************************************/ > >>>> +/*******************************************************************/ > >>>> + > >>>> +/* 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 > >>> > >>> I prefer these definitions to come at the top of the file along with > >>> other definitions. > >>> > >>>> + > >>>> +/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */ > >>>> +static void build_serialization_instruction_entry(GArray *table_data, > >>> > >>> This function and buiild_erst() can come at the end of erst.c. They go > >>> together and are doing a common but different operation from the > >>> operations of the pci device - building the erst table. Hence, ther > >>> code should be separate from pci device code. A new file would be an > >>> overkill at this state IMHO but in the future if erst table generation > >>> code gains more weight, it can be split into two files. > >>> > >>>> + uint8_t serialization_action, > >>>> + uint8_t instruction, > >>>> + uint8_t flags, > >>>> + uint8_t register_bit_width, > >>>> + uint64_t register_address, > >>>> + uint64_t value, > >>>> + uint64_t mask) > >>>> +{ > >>>> + /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */ > >>>> + struct AcpiGenericAddress gas; > >>>> + > >>>> + /* 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; > >>>> + switch (register_bit_width) { > >>>> + case 8: > >>>> + gas.access_width = 1; > >>>> + break; > >>>> + case 16: > >>>> + gas.access_width = 2; > >>>> + break; > >>>> + case 32: > >>>> + gas.access_width = 3; > >>>> + break; > >>>> + case 64: > >>>> + gas.access_width = 4; > >>>> + break; > >>>> + default: > >>>> + gas.access_width = 0; > >>>> + break; > >>>> + } > >>>> + gas.address = register_address; > >>>> + build_append_gas_from_struct(table_data, &gas); > >>>> + /* Value */ > >>>> + build_append_int_noprefix(table_data, value , 8); > >>>> + /* Mask */ > >>>> + 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, bar1; > >>>> + AcpiTable table = { .sig = "ERST", .rev = 1, .oem_id = oem_id, > >>>> + .oem_table_id = oem_table_id }; > >>>> + > >>>> + bar0 = (pcibus_t)pci_get_bar_addr(PCI_DEVICE(erst_dev), 0); > >>>> + trace_acpi_erst_pci_bar_0(bar0); > >>>> + bar1 = (pcibus_t)pci_get_bar_addr(PCI_DEVICE(erst_dev), 1); > >>>> + trace_acpi_erst_pci_bar_1(bar1); > >>>> + > >>>> +#define MASK8 0x00000000000000FFUL > >>>> +#define MASK16 0x000000000000FFFFUL > >>>> +#define MASK32 0x00000000FFFFFFFFUL > >>>> +#define MASK64 0xFFFFFFFFFFFFFFFFUL > >>>> + > >>>> + /* > >>>> + * 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)); > >>>> + > >>>> + /* Serialization Instruction Entries */ > >>>> + action = ACTION_BEGIN_WRITE_OPERATION; > >>>> + build_serialization_instruction_entry(table_instruction_data, > >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >>>> + > >>>> + action = ACTION_BEGIN_READ_OPERATION; > >>>> + build_serialization_instruction_entry(table_instruction_data, > >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >>>> + > >>>> + action = ACTION_BEGIN_CLEAR_OPERATION; > >>>> + build_serialization_instruction_entry(table_instruction_data, > >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >>>> + > >>>> + action = ACTION_END_OPERATION; > >>>> + build_serialization_instruction_entry(table_instruction_data, > >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >>>> + > >>>> + action = ACTION_SET_RECORD_OFFSET; > >>>> + build_serialization_instruction_entry(table_instruction_data, > >>>> + action, INST_WRITE_REGISTER , 0, 32, > >>>> + bar0 + ERST_VALUE_OFFSET , 0, MASK32); > >>>> + build_serialization_instruction_entry(table_instruction_data, > >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >>>> + > >>>> + action = ACTION_EXECUTE_OPERATION; > >>>> + build_serialization_instruction_entry(table_instruction_data, > >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + bar0 + ERST_VALUE_OFFSET , ERST_EXECUTE_OPERATION_MAGIC, MASK8); > >>>> + build_serialization_instruction_entry(table_instruction_data, > >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >>>> + > >>>> + action = ACTION_CHECK_BUSY_STATUS; > >>>> + build_serialization_instruction_entry(table_instruction_data, > >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >>>> + build_serialization_instruction_entry(table_instruction_data, > >>>> + action, INST_READ_REGISTER_VALUE , 0, 32, > >>>> + bar0 + ERST_VALUE_OFFSET, 0x01, MASK8); > >>>> + > >>>> + action = ACTION_GET_COMMAND_STATUS; > >>>> + build_serialization_instruction_entry(table_instruction_data, > >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >>>> + build_serialization_instruction_entry(table_instruction_data, > >>>> + action, INST_READ_REGISTER , 0, 32, > >>>> + bar0 + ERST_VALUE_OFFSET, 0, MASK8); > >>>> + > >>>> + action = ACTION_GET_RECORD_IDENTIFIER; > >>>> + build_serialization_instruction_entry(table_instruction_data, > >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >>>> + build_serialization_instruction_entry(table_instruction_data, > >>>> + action, INST_READ_REGISTER , 0, 64, > >>>> + bar0 + ERST_VALUE_OFFSET, 0, MASK64); > >>>> + > >>>> + action = ACTION_SET_RECORD_IDENTIFIER; > >>>> + build_serialization_instruction_entry(table_instruction_data, > >>>> + action, INST_WRITE_REGISTER , 0, 64, > >>>> + bar0 + ERST_VALUE_OFFSET , 0, MASK64); > >>>> + build_serialization_instruction_entry(table_instruction_data, > >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >>>> + > >>>> + action = ACTION_GET_RECORD_COUNT; > >>>> + build_serialization_instruction_entry(table_instruction_data, > >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >>>> + build_serialization_instruction_entry(table_instruction_data, > >>>> + action, INST_READ_REGISTER , 0, 32, > >>>> + bar0 + ERST_VALUE_OFFSET, 0, MASK32); > >>>> + > >>>> + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION; > >>>> + build_serialization_instruction_entry(table_instruction_data, > >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >>>> + > >>>> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE; > >>>> + build_serialization_instruction_entry(table_instruction_data, > >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >>>> + build_serialization_instruction_entry(table_instruction_data, > >>>> + action, INST_READ_REGISTER , 0, 64, > >>>> + bar0 + ERST_VALUE_OFFSET, 0, MASK64); > >>>> + > >>>> + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH; > >>>> + build_serialization_instruction_entry(table_instruction_data, > >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >>>> + build_serialization_instruction_entry(table_instruction_data, > >>>> + action, INST_READ_REGISTER , 0, 64, > >>>> + bar0 + ERST_VALUE_OFFSET, 0, MASK32); > >>>> + > >>>> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES; > >>>> + build_serialization_instruction_entry(table_instruction_data, > >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >>>> + build_serialization_instruction_entry(table_instruction_data, > >>>> + action, INST_READ_REGISTER , 0, 32, > >>>> + bar0 + ERST_VALUE_OFFSET, 0, MASK32); > >>>> + > >>>> + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS; > >>>> + build_serialization_instruction_entry(table_instruction_data, > >>>> + action, INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + bar0 + ERST_ACTION_OFFSET, action, MASK8); > >>>> + build_serialization_instruction_entry(table_instruction_data, > >>>> + action, INST_READ_REGISTER , 0, 64, > >>>> + bar0 + ERST_VALUE_OFFSET, 0, MASK64); > >>>> + > >>>> + /* 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 > >>>> + */ > >>>> + 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 int erst_post_load(void *opaque, int version_id) > >>>> -- > >>>> 1.8.3.1 > >>>>
diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c index 81f5435..753425a 100644 --- a/hw/acpi/erst.c +++ b/hw/acpi/erst.c @@ -711,6 +711,247 @@ static const MemoryRegionOps erst_reg_ops = { .endianness = DEVICE_NATIVE_ENDIAN, }; + +/*******************************************************************/ +/*******************************************************************/ + +/* 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 + +/* 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, + uint64_t mask) +{ + /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */ + struct AcpiGenericAddress gas; + + /* 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; + switch (register_bit_width) { + case 8: + gas.access_width = 1; + break; + case 16: + gas.access_width = 2; + break; + case 32: + gas.access_width = 3; + break; + case 64: + gas.access_width = 4; + break; + default: + gas.access_width = 0; + break; + } + gas.address = register_address; + build_append_gas_from_struct(table_data, &gas); + /* Value */ + build_append_int_noprefix(table_data, value , 8); + /* Mask */ + 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, bar1; + AcpiTable table = { .sig = "ERST", .rev = 1, .oem_id = oem_id, + .oem_table_id = oem_table_id }; + + bar0 = (pcibus_t)pci_get_bar_addr(PCI_DEVICE(erst_dev), 0); + trace_acpi_erst_pci_bar_0(bar0); + bar1 = (pcibus_t)pci_get_bar_addr(PCI_DEVICE(erst_dev), 1); + trace_acpi_erst_pci_bar_1(bar1); + +#define MASK8 0x00000000000000FFUL +#define MASK16 0x000000000000FFFFUL +#define MASK32 0x00000000FFFFFFFFUL +#define MASK64 0xFFFFFFFFFFFFFFFFUL + + /* + * 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)); + + /* Serialization Instruction Entries */ + action = ACTION_BEGIN_WRITE_OPERATION; + build_serialization_instruction_entry(table_instruction_data, + action, INST_WRITE_REGISTER_VALUE, 0, 32, + bar0 + ERST_ACTION_OFFSET, action, MASK8); + + action = ACTION_BEGIN_READ_OPERATION; + build_serialization_instruction_entry(table_instruction_data, + action, INST_WRITE_REGISTER_VALUE, 0, 32, + bar0 + ERST_ACTION_OFFSET, action, MASK8); + + action = ACTION_BEGIN_CLEAR_OPERATION; + build_serialization_instruction_entry(table_instruction_data, + action, INST_WRITE_REGISTER_VALUE, 0, 32, + bar0 + ERST_ACTION_OFFSET, action, MASK8); + + action = ACTION_END_OPERATION; + build_serialization_instruction_entry(table_instruction_data, + action, INST_WRITE_REGISTER_VALUE, 0, 32, + bar0 + ERST_ACTION_OFFSET, action, MASK8); + + action = ACTION_SET_RECORD_OFFSET; + build_serialization_instruction_entry(table_instruction_data, + action, INST_WRITE_REGISTER , 0, 32, + bar0 + ERST_VALUE_OFFSET , 0, MASK32); + build_serialization_instruction_entry(table_instruction_data, + action, INST_WRITE_REGISTER_VALUE, 0, 32, + bar0 + ERST_ACTION_OFFSET, action, MASK8); + + action = ACTION_EXECUTE_OPERATION; + build_serialization_instruction_entry(table_instruction_data, + action, INST_WRITE_REGISTER_VALUE, 0, 32, + bar0 + ERST_VALUE_OFFSET , ERST_EXECUTE_OPERATION_MAGIC, MASK8); + build_serialization_instruction_entry(table_instruction_data, + action, INST_WRITE_REGISTER_VALUE, 0, 32, + bar0 + ERST_ACTION_OFFSET, action, MASK8); + + action = ACTION_CHECK_BUSY_STATUS; + build_serialization_instruction_entry(table_instruction_data, + action, INST_WRITE_REGISTER_VALUE, 0, 32, + bar0 + ERST_ACTION_OFFSET, action, MASK8); + build_serialization_instruction_entry(table_instruction_data, + action, INST_READ_REGISTER_VALUE , 0, 32, + bar0 + ERST_VALUE_OFFSET, 0x01, MASK8); + + action = ACTION_GET_COMMAND_STATUS; + build_serialization_instruction_entry(table_instruction_data, + action, INST_WRITE_REGISTER_VALUE, 0, 32, + bar0 + ERST_ACTION_OFFSET, action, MASK8); + build_serialization_instruction_entry(table_instruction_data, + action, INST_READ_REGISTER , 0, 32, + bar0 + ERST_VALUE_OFFSET, 0, MASK8); + + action = ACTION_GET_RECORD_IDENTIFIER; + build_serialization_instruction_entry(table_instruction_data, + action, INST_WRITE_REGISTER_VALUE, 0, 32, + bar0 + ERST_ACTION_OFFSET, action, MASK8); + build_serialization_instruction_entry(table_instruction_data, + action, INST_READ_REGISTER , 0, 64, + bar0 + ERST_VALUE_OFFSET, 0, MASK64); + + action = ACTION_SET_RECORD_IDENTIFIER; + build_serialization_instruction_entry(table_instruction_data, + action, INST_WRITE_REGISTER , 0, 64, + bar0 + ERST_VALUE_OFFSET , 0, MASK64); + build_serialization_instruction_entry(table_instruction_data, + action, INST_WRITE_REGISTER_VALUE, 0, 32, + bar0 + ERST_ACTION_OFFSET, action, MASK8); + + action = ACTION_GET_RECORD_COUNT; + build_serialization_instruction_entry(table_instruction_data, + action, INST_WRITE_REGISTER_VALUE, 0, 32, + bar0 + ERST_ACTION_OFFSET, action, MASK8); + build_serialization_instruction_entry(table_instruction_data, + action, INST_READ_REGISTER , 0, 32, + bar0 + ERST_VALUE_OFFSET, 0, MASK32); + + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION; + build_serialization_instruction_entry(table_instruction_data, + action, INST_WRITE_REGISTER_VALUE, 0, 32, + bar0 + ERST_ACTION_OFFSET, action, MASK8); + + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE; + build_serialization_instruction_entry(table_instruction_data, + action, INST_WRITE_REGISTER_VALUE, 0, 32, + bar0 + ERST_ACTION_OFFSET, action, MASK8); + build_serialization_instruction_entry(table_instruction_data, + action, INST_READ_REGISTER , 0, 64, + bar0 + ERST_VALUE_OFFSET, 0, MASK64); + + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH; + build_serialization_instruction_entry(table_instruction_data, + action, INST_WRITE_REGISTER_VALUE, 0, 32, + bar0 + ERST_ACTION_OFFSET, action, MASK8); + build_serialization_instruction_entry(table_instruction_data, + action, INST_READ_REGISTER , 0, 64, + bar0 + ERST_VALUE_OFFSET, 0, MASK32); + + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES; + build_serialization_instruction_entry(table_instruction_data, + action, INST_WRITE_REGISTER_VALUE, 0, 32, + bar0 + ERST_ACTION_OFFSET, action, MASK8); + build_serialization_instruction_entry(table_instruction_data, + action, INST_READ_REGISTER , 0, 32, + bar0 + ERST_VALUE_OFFSET, 0, MASK32); + + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS; + build_serialization_instruction_entry(table_instruction_data, + action, INST_WRITE_REGISTER_VALUE, 0, 32, + bar0 + ERST_ACTION_OFFSET, action, MASK8); + build_serialization_instruction_entry(table_instruction_data, + action, INST_READ_REGISTER , 0, 64, + bar0 + ERST_VALUE_OFFSET, 0, MASK64); + + /* 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 + */ + 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 int erst_post_load(void *opaque, int version_id)
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 | 241 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 241 insertions(+)