On Tue, Dec 14, 2021 at 11:42 PM Eric DeVolder <eric.devol...@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.devol...@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.devol...@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.devol...@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 > >>>>