| Message ID | 1625080041-29010-7-git-send-email-eric.devolder@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | acpi: Error Record Serialization Table, ERST, support for QEMU | expand |
On Wed, 30 Jun 2021 15:07:17 -0400 Eric DeVolder <eric.devolder@oracle.com> wrote: > This code is called from the machine code (if ACPI supported) > to generate the ACPI ERST table. should be along lines: This builds ACPI ERST table /spec ref/ to inform OSMP how to communicate with ... device. > > Signed-off-by: Eric DeVolder <eric.devolder@oracle.com> > --- > hw/acpi/erst.c | 214 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 214 insertions(+) > > diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c > index 6e9bd2e..1f1dbbc 100644 > --- a/hw/acpi/erst.c > +++ b/hw/acpi/erst.c > @@ -555,6 +555,220 @@ static const MemoryRegionOps erst_mem_ops = { > /*******************************************************************/ > /*******************************************************************/ > > +/* 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) like I mentioned in previous patch, It could be simplified a lot if it's possible to use fixed 64-bit access with every action and the same width 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) > +{ > + ERSTDeviceState *s = ACPIERST(erst_dev); globals are not welcomed in new code, pass erst_dev as argument here (ex: find_vmgenid_dev) > + unsigned action; > + unsigned erst_start = table_data->len; > + > + s->bar0 = (hwaddr)pci_get_bar_addr(PCI_DEVICE(erst_dev), 0); > + trace_acpi_erst_pci_bar_0(s->bar0); > + s->bar1 = (hwaddr)pci_get_bar_addr(PCI_DEVICE(erst_dev), 1); just store pci_get_bar_addr(PCI_DEVICE(erst_dev), 0) in local variable, Bar 1 is not used in this function so you don't need it here. > + trace_acpi_erst_pci_bar_1(s->bar1); > + > + acpi_data_push(table_data, sizeof(AcpiTableHeader)); > + /* serialization_header_length */ comments documenting table entries should be verbatim copy from spec, see build_amd_iommu() as example of preferred style. > + build_append_int_noprefix(table_data, 48, 4); > + /* reserved */ > + build_append_int_noprefix(table_data, 0, 4); > + /* > + * instruction_entry_count - changes to the number of serialization > + * instructions in the ACTIONs below must be reflected in this > + * pre-computed value. > + */ > + build_append_int_noprefix(table_data, 29, 4); a bit fragile as it can easily diverge from actual number later on. maybe instead of building instruction entries in place, build it in separate array and when done, use actual count to fill instruction_entry_count. pseudo code could look like: /* prepare instructions in advance because ... */ GArray table_instruction_data; build_serialization_instruction_entry(table_instruction_data,...);; ... build_serialization_instruction_entry(table_instruction_data,...); /* instructions count */ build_append_int_noprefix(table_data, table_instruction_data.len/entry_size, 4); /* copy prepared in advance instructions */ g_array_append_vals(table_data, table_instruction_data.data, table_instruction_data.len); > + > +#define MASK8 0x00000000000000FFUL > +#define MASK16 0x000000000000FFFFUL > +#define MASK32 0x00000000FFFFFFFFUL > +#define MASK64 0xFFFFFFFFFFFFFFFFUL > + > + for (action = 0; action < ACPI_ERST_MAX_ACTIONS; ++action) { I'd unroll this loop and just directly code entries in required order. also drop reserved and nop actions/instructions or explain why they are necessary. > + switch (action) { > + case ACPI_ERST_ACTION_BEGIN_WRITE_OPERATION: given these names will/should never be exposed outside of hw/acpi/erst.c I'd drop ACPI_ERST_ACTION_/ACPI_ERST_INST_ prefixes (i.e. use names as defined in spec) if it doesn't cause build issues. > + build_serialization_instruction_entry(table_data, action, > + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > + s->bar0 + ERST_CSR_ACTION, action, MASK8); > + break; > + case ACPI_ERST_ACTION_BEGIN_READ_OPERATION: > + build_serialization_instruction_entry(table_data, action, > + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > + s->bar0 + ERST_CSR_ACTION, action, MASK8); > + break; > + case ACPI_ERST_ACTION_BEGIN_CLEAR_OPERATION: > + build_serialization_instruction_entry(table_data, action, > + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > + s->bar0 + ERST_CSR_ACTION, action, MASK8); > + break; > + case ACPI_ERST_ACTION_END_OPERATION: > + build_serialization_instruction_entry(table_data, action, > + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > + s->bar0 + ERST_CSR_ACTION, action, MASK8); > + break; > + case ACPI_ERST_ACTION_SET_RECORD_OFFSET: > + build_serialization_instruction_entry(table_data, action, > + ACPI_ERST_INST_WRITE_REGISTER , 0, 32, > + s->bar0 + ERST_CSR_VALUE , 0, MASK32); > + build_serialization_instruction_entry(table_data, action, > + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > + s->bar0 + ERST_CSR_ACTION, action, MASK8); > + break; > + case ACPI_ERST_ACTION_EXECUTE_OPERATION: > + build_serialization_instruction_entry(table_data, action, > + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > + s->bar0 + ERST_CSR_VALUE , ERST_EXECUTE_OPERATION_MAGIC, MASK8); > + build_serialization_instruction_entry(table_data, action, > + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > + s->bar0 + ERST_CSR_ACTION, action, MASK8); > + break; > + case ACPI_ERST_ACTION_CHECK_BUSY_STATUS: > + build_serialization_instruction_entry(table_data, action, > + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > + s->bar0 + ERST_CSR_ACTION, action, MASK8); > + build_serialization_instruction_entry(table_data, action, > + ACPI_ERST_INST_READ_REGISTER_VALUE , 0, 32, > + s->bar0 + ERST_CSR_VALUE, 0x01, MASK8); > + break; > + case ACPI_ERST_ACTION_GET_COMMAND_STATUS: > + build_serialization_instruction_entry(table_data, action, > + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > + s->bar0 + ERST_CSR_ACTION, action, MASK8); > + build_serialization_instruction_entry(table_data, action, > + ACPI_ERST_INST_READ_REGISTER , 0, 32, > + s->bar0 + ERST_CSR_VALUE, 0, MASK8); > + break; > + case ACPI_ERST_ACTION_GET_RECORD_IDENTIFIER: > + build_serialization_instruction_entry(table_data, action, > + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > + s->bar0 + ERST_CSR_ACTION, action, MASK8); > + build_serialization_instruction_entry(table_data, action, > + ACPI_ERST_INST_READ_REGISTER , 0, 64, > + s->bar0 + ERST_CSR_VALUE, 0, MASK64); > + break; > + case ACPI_ERST_ACTION_SET_RECORD_IDENTIFIER: > + build_serialization_instruction_entry(table_data, action, > + ACPI_ERST_INST_WRITE_REGISTER , 0, 64, > + s->bar0 + ERST_CSR_VALUE , 0, MASK64); > + build_serialization_instruction_entry(table_data, action, > + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > + s->bar0 + ERST_CSR_ACTION, action, MASK8); > + break; > + case ACPI_ERST_ACTION_GET_RECORD_COUNT: > + build_serialization_instruction_entry(table_data, action, > + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > + s->bar0 + ERST_CSR_ACTION, action, MASK8); > + build_serialization_instruction_entry(table_data, action, > + ACPI_ERST_INST_READ_REGISTER , 0, 32, > + s->bar0 + ERST_CSR_VALUE, 0, MASK32); > + break; > + case ACPI_ERST_ACTION_BEGIN_DUMMY_WRITE_OPERATION: > + build_serialization_instruction_entry(table_data, action, > + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > + s->bar0 + ERST_CSR_ACTION, action, MASK8); > + break; > + case ACPI_ERST_ACTION_RESERVED: > + build_serialization_instruction_entry(table_data, action, > + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > + s->bar0 + ERST_CSR_ACTION, action, MASK8); > + break; > + case ACPI_ERST_ACTION_GET_ERROR_LOG_ADDRESS_RANGE: > + build_serialization_instruction_entry(table_data, action, > + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > + s->bar0 + ERST_CSR_ACTION, action, MASK8); > + build_serialization_instruction_entry(table_data, action, > + ACPI_ERST_INST_READ_REGISTER , 0, 64, > + s->bar0 + ERST_CSR_VALUE, 0, MASK64); > + break; > + case ACPI_ERST_ACTION_GET_ERROR_LOG_ADDRESS_LENGTH: > + build_serialization_instruction_entry(table_data, action, > + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > + s->bar0 + ERST_CSR_ACTION, action, MASK8); > + build_serialization_instruction_entry(table_data, action, > + ACPI_ERST_INST_READ_REGISTER , 0, 64, > + s->bar0 + ERST_CSR_VALUE, 0, MASK32); > + break; > + case ACPI_ERST_ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES: > + build_serialization_instruction_entry(table_data, action, > + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > + s->bar0 + ERST_CSR_ACTION, action, MASK8); > + build_serialization_instruction_entry(table_data, action, > + ACPI_ERST_INST_READ_REGISTER , 0, 32, > + s->bar0 + ERST_CSR_VALUE, 0, MASK32); > + break; > + case ACPI_ERST_ACTION_GET_EXECUTE_OPERATION_TIMINGS: > + build_serialization_instruction_entry(table_data, action, > + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > + s->bar0 + ERST_CSR_ACTION, action, MASK8); > + build_serialization_instruction_entry(table_data, action, > + ACPI_ERST_INST_READ_REGISTER , 0, 64, > + s->bar0 + ERST_CSR_VALUE, 0, MASK64); > + default: > + build_serialization_instruction_entry(table_data, action, > + ACPI_ERST_INST_NOOP, 0, 0, 0, action, 0); > + break; > + } > + } > + build_header(linker, table_data, > + (void *)(table_data->data + erst_start), > + "ERST", table_data->len - erst_start, > + 1, oem_id, oem_table_id); > +} > + > +/*******************************************************************/ > +/*******************************************************************/ > + > static const VMStateDescription erst_vmstate = { > .name = "acpi-erst", > .version_id = 1,
On Tue, 20 Jul 2021 15:16:40 +0200 Igor Mammedov <imammedo@redhat.com> wrote: > On Wed, 30 Jun 2021 15:07:17 -0400 > Eric DeVolder <eric.devolder@oracle.com> wrote: [...] > > +/* 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) > > +{ > > + ERSTDeviceState *s = ACPIERST(erst_dev); > > globals are not welcomed in new code, > pass erst_dev as argument here (ex: find_vmgenid_dev) ignore this, I didn't notice that it's passed as argument.
On 7/20/21 8:16 AM, Igor Mammedov wrote: > On Wed, 30 Jun 2021 15:07:17 -0400 > Eric DeVolder <eric.devolder@oracle.com> wrote: > >> This code is called from the machine code (if ACPI supported) >> to generate the ACPI ERST table. > should be along lines: > This builds ACPI ERST table /spec ref/ to inform OSMP > how to communicate with ... device. Like this? This builds the ACPI ERST table to inform OSMP how to communicate with the acpi-erst device. > >> >> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com> >> --- >> hw/acpi/erst.c | 214 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 214 insertions(+) >> >> diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c >> index 6e9bd2e..1f1dbbc 100644 >> --- a/hw/acpi/erst.c >> +++ b/hw/acpi/erst.c >> @@ -555,6 +555,220 @@ static const MemoryRegionOps erst_mem_ops = { >> /*******************************************************************/ >> /*******************************************************************/ >> >> +/* 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) > like I mentioned in previous patch, It could be simplified > a lot if it's possible to use fixed 64-bit access with every > action and the same width mask. See previous response. > >> +{ >> + /* 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) >> +{ >> + ERSTDeviceState *s = ACPIERST(erst_dev); > > globals are not welcomed in new code, > pass erst_dev as argument here (ex: find_vmgenid_dev) > >> + unsigned action; >> + unsigned erst_start = table_data->len; >> + > >> + s->bar0 = (hwaddr)pci_get_bar_addr(PCI_DEVICE(erst_dev), 0); >> + trace_acpi_erst_pci_bar_0(s->bar0); >> + s->bar1 = (hwaddr)pci_get_bar_addr(PCI_DEVICE(erst_dev), 1); > > just store pci_get_bar_addr(PCI_DEVICE(erst_dev), 0) in local variable, > Bar 1 is not used in this function so you don't need it here. Corrected > > >> + trace_acpi_erst_pci_bar_1(s->bar1); >> + >> + acpi_data_push(table_data, sizeof(AcpiTableHeader)); >> + /* serialization_header_length */ > comments documenting table entries should be verbatim copy from spec, > see build_amd_iommu() as example of preferred style. Corrected > >> + build_append_int_noprefix(table_data, 48, 4); >> + /* reserved */ >> + build_append_int_noprefix(table_data, 0, 4); >> + /* >> + * instruction_entry_count - changes to the number of serialization >> + * instructions in the ACTIONs below must be reflected in this >> + * pre-computed value. >> + */ >> + build_append_int_noprefix(table_data, 29, 4); > a bit fragile as it can easily diverge from actual number later on. > maybe instead of building instruction entries in place, build it > in separate array and when done, use actual count to fill instruction_entry_count. > pseudo code could look like: > > /* prepare instructions in advance because ... */ > GArray table_instruction_data; > build_serialization_instruction_entry(table_instruction_data,...);; > ... > build_serialization_instruction_entry(table_instruction_data,...); > /* instructions count */ > build_append_int_noprefix(table_data, table_instruction_data.len/entry_size, 4); > /* copy prepared in advance instructions */ > g_array_append_vals(table_data, table_instruction_data.data, table_instruction_data.len); Corrected > > >> + >> +#define MASK8 0x00000000000000FFUL >> +#define MASK16 0x000000000000FFFFUL >> +#define MASK32 0x00000000FFFFFFFFUL >> +#define MASK64 0xFFFFFFFFFFFFFFFFUL >> + >> + for (action = 0; action < ACPI_ERST_MAX_ACTIONS; ++action) { > I'd unroll this loop and just directly code entries in required order. > also drop reserved and nop actions/instructions or explain why they are necessary. Unrolled. Dropped the NOP. > >> + switch (action) { >> + case ACPI_ERST_ACTION_BEGIN_WRITE_OPERATION: > given these names will/should never be exposed outside of hw/acpi/erst.c > I'd drop ACPI_ERST_ACTION_/ACPI_ERST_INST_ prefixes (i.e. use names as defined in spec) > if it doesn't cause build issues. These are in include/hw/acpi/erst.h which is included by hw/i386/acpi-build.c, which includes many other hardware files. Removing the prefix leaves a rather generic name. I'd prefer to leave them as it uniquely differentiates. > >> + build_serialization_instruction_entry(table_data, action, >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >> + break; >> + case ACPI_ERST_ACTION_BEGIN_READ_OPERATION: >> + build_serialization_instruction_entry(table_data, action, >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >> + break; >> + case ACPI_ERST_ACTION_BEGIN_CLEAR_OPERATION: >> + build_serialization_instruction_entry(table_data, action, >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >> + break; >> + case ACPI_ERST_ACTION_END_OPERATION: >> + build_serialization_instruction_entry(table_data, action, >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >> + break; >> + case ACPI_ERST_ACTION_SET_RECORD_OFFSET: >> + build_serialization_instruction_entry(table_data, action, >> + ACPI_ERST_INST_WRITE_REGISTER , 0, 32, >> + s->bar0 + ERST_CSR_VALUE , 0, MASK32); >> + build_serialization_instruction_entry(table_data, action, >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >> + break; >> + case ACPI_ERST_ACTION_EXECUTE_OPERATION: >> + build_serialization_instruction_entry(table_data, action, >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >> + s->bar0 + ERST_CSR_VALUE , ERST_EXECUTE_OPERATION_MAGIC, MASK8); >> + build_serialization_instruction_entry(table_data, action, >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >> + break; >> + case ACPI_ERST_ACTION_CHECK_BUSY_STATUS: >> + build_serialization_instruction_entry(table_data, action, >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >> + build_serialization_instruction_entry(table_data, action, >> + ACPI_ERST_INST_READ_REGISTER_VALUE , 0, 32, >> + s->bar0 + ERST_CSR_VALUE, 0x01, MASK8); >> + break; >> + case ACPI_ERST_ACTION_GET_COMMAND_STATUS: >> + build_serialization_instruction_entry(table_data, action, >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >> + build_serialization_instruction_entry(table_data, action, >> + ACPI_ERST_INST_READ_REGISTER , 0, 32, >> + s->bar0 + ERST_CSR_VALUE, 0, MASK8); >> + break; >> + case ACPI_ERST_ACTION_GET_RECORD_IDENTIFIER: >> + build_serialization_instruction_entry(table_data, action, >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >> + build_serialization_instruction_entry(table_data, action, >> + ACPI_ERST_INST_READ_REGISTER , 0, 64, >> + s->bar0 + ERST_CSR_VALUE, 0, MASK64); >> + break; >> + case ACPI_ERST_ACTION_SET_RECORD_IDENTIFIER: >> + build_serialization_instruction_entry(table_data, action, >> + ACPI_ERST_INST_WRITE_REGISTER , 0, 64, >> + s->bar0 + ERST_CSR_VALUE , 0, MASK64); >> + build_serialization_instruction_entry(table_data, action, >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >> + break; >> + case ACPI_ERST_ACTION_GET_RECORD_COUNT: >> + build_serialization_instruction_entry(table_data, action, >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >> + build_serialization_instruction_entry(table_data, action, >> + ACPI_ERST_INST_READ_REGISTER , 0, 32, >> + s->bar0 + ERST_CSR_VALUE, 0, MASK32); >> + break; >> + case ACPI_ERST_ACTION_BEGIN_DUMMY_WRITE_OPERATION: >> + build_serialization_instruction_entry(table_data, action, >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >> + break; >> + case ACPI_ERST_ACTION_RESERVED: >> + build_serialization_instruction_entry(table_data, action, >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >> + break; >> + case ACPI_ERST_ACTION_GET_ERROR_LOG_ADDRESS_RANGE: >> + build_serialization_instruction_entry(table_data, action, >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >> + build_serialization_instruction_entry(table_data, action, >> + ACPI_ERST_INST_READ_REGISTER , 0, 64, >> + s->bar0 + ERST_CSR_VALUE, 0, MASK64); >> + break; >> + case ACPI_ERST_ACTION_GET_ERROR_LOG_ADDRESS_LENGTH: >> + build_serialization_instruction_entry(table_data, action, >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >> + build_serialization_instruction_entry(table_data, action, >> + ACPI_ERST_INST_READ_REGISTER , 0, 64, >> + s->bar0 + ERST_CSR_VALUE, 0, MASK32); >> + break; >> + case ACPI_ERST_ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES: >> + build_serialization_instruction_entry(table_data, action, >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >> + build_serialization_instruction_entry(table_data, action, >> + ACPI_ERST_INST_READ_REGISTER , 0, 32, >> + s->bar0 + ERST_CSR_VALUE, 0, MASK32); >> + break; >> + case ACPI_ERST_ACTION_GET_EXECUTE_OPERATION_TIMINGS: >> + build_serialization_instruction_entry(table_data, action, >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >> + build_serialization_instruction_entry(table_data, action, >> + ACPI_ERST_INST_READ_REGISTER , 0, 64, >> + s->bar0 + ERST_CSR_VALUE, 0, MASK64); >> + default: >> + build_serialization_instruction_entry(table_data, action, >> + ACPI_ERST_INST_NOOP, 0, 0, 0, action, 0); >> + break; >> + } >> + } >> + build_header(linker, table_data, >> + (void *)(table_data->data + erst_start), >> + "ERST", table_data->len - erst_start, >> + 1, oem_id, oem_table_id); >> +} >> + >> +/*******************************************************************/ >> +/*******************************************************************/ >> + >> static const VMStateDescription erst_vmstate = { >> .name = "acpi-erst", >> .version_id = 1, >
On Wed, 21 Jul 2021 11:12:41 -0500 Eric DeVolder <eric.devolder@oracle.com> wrote: > On 7/20/21 8:16 AM, Igor Mammedov wrote: > > On Wed, 30 Jun 2021 15:07:17 -0400 > > Eric DeVolder <eric.devolder@oracle.com> wrote: > > > >> This code is called from the machine code (if ACPI supported) > >> to generate the ACPI ERST table. > > should be along lines: > > This builds ACPI ERST table /spec ref/ to inform OSMP > > how to communicate with ... device. > > Like this? > This builds the ACPI ERST table to inform OSMP how to communicate ^ [1] > with the acpi-erst device. > 1) ACPI spec vX.Y, chapter foo > > > > >> > >> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com> > >> --- > >> hw/acpi/erst.c | 214 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 214 insertions(+) > >> > >> diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c > >> index 6e9bd2e..1f1dbbc 100644 > >> --- a/hw/acpi/erst.c > >> +++ b/hw/acpi/erst.c > >> @@ -555,6 +555,220 @@ static const MemoryRegionOps erst_mem_ops = { > >> /*******************************************************************/ > >> /*******************************************************************/ > >> > >> +/* 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) > > like I mentioned in previous patch, It could be simplified > > a lot if it's possible to use fixed 64-bit access with every > > action and the same width mask. > See previous response. lets see if it's a guest OS issue first, and then decide what to do with it. > > > > >> +{ > >> + /* 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) > >> +{ > >> + ERSTDeviceState *s = ACPIERST(erst_dev); > > > > globals are not welcomed in new code, > > pass erst_dev as argument here (ex: find_vmgenid_dev) > > > >> + unsigned action; > >> + unsigned erst_start = table_data->len; > >> + > > > >> + s->bar0 = (hwaddr)pci_get_bar_addr(PCI_DEVICE(erst_dev), 0); > >> + trace_acpi_erst_pci_bar_0(s->bar0); > >> + s->bar1 = (hwaddr)pci_get_bar_addr(PCI_DEVICE(erst_dev), 1); > > > > just store pci_get_bar_addr(PCI_DEVICE(erst_dev), 0) in local variable, > > Bar 1 is not used in this function so you don't need it here. > Corrected > > > > > > >> + trace_acpi_erst_pci_bar_1(s->bar1); > >> + > >> + acpi_data_push(table_data, sizeof(AcpiTableHeader)); > >> + /* serialization_header_length */ > > comments documenting table entries should be verbatim copy from spec, > > see build_amd_iommu() as example of preferred style. > Corrected > > > > >> + build_append_int_noprefix(table_data, 48, 4); > >> + /* reserved */ > >> + build_append_int_noprefix(table_data, 0, 4); > >> + /* > >> + * instruction_entry_count - changes to the number of serialization > >> + * instructions in the ACTIONs below must be reflected in this > >> + * pre-computed value. > >> + */ > >> + build_append_int_noprefix(table_data, 29, 4); > > a bit fragile as it can easily diverge from actual number later on. > > maybe instead of building instruction entries in place, build it > > in separate array and when done, use actual count to fill instruction_entry_count. > > pseudo code could look like: > > > > /* prepare instructions in advance because ... */ > > GArray table_instruction_data; > > build_serialization_instruction_entry(table_instruction_data,...);; > > ... > > build_serialization_instruction_entry(table_instruction_data,...); > > /* instructions count */ > > build_append_int_noprefix(table_data, table_instruction_data.len/entry_size, 4); > > /* copy prepared in advance instructions */ > > g_array_append_vals(table_data, table_instruction_data.data, table_instruction_data.len); > Corrected > > > > > > >> + > >> +#define MASK8 0x00000000000000FFUL > >> +#define MASK16 0x000000000000FFFFUL > >> +#define MASK32 0x00000000FFFFFFFFUL > >> +#define MASK64 0xFFFFFFFFFFFFFFFFUL > >> + > >> + for (action = 0; action < ACPI_ERST_MAX_ACTIONS; ++action) { > > I'd unroll this loop and just directly code entries in required order. > > also drop reserved and nop actions/instructions or explain why they are necessary. > Unrolled. Dropped the NOP. What about 'reserved"? > > > > >> + switch (action) { > >> + case ACPI_ERST_ACTION_BEGIN_WRITE_OPERATION: > > given these names will/should never be exposed outside of hw/acpi/erst.c > > I'd drop ACPI_ERST_ACTION_/ACPI_ERST_INST_ prefixes (i.e. use names as defined in spec) > > if it doesn't cause build issues. > These are in include/hw/acpi/erst.h which is included by hw/i386/acpi-build.c, > which includes many other hardware files. > Removing the prefix leaves a rather generic name. > I'd prefer to leave them as it uniquely differentiates. is there a reason to put them into erst.h and expose to outside world? If not then it might be better to move them into erst.c > > > > > >> + build_serialization_instruction_entry(table_data, action, > >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >> + break; > >> + case ACPI_ERST_ACTION_BEGIN_READ_OPERATION: > >> + build_serialization_instruction_entry(table_data, action, > >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >> + break; > >> + case ACPI_ERST_ACTION_BEGIN_CLEAR_OPERATION: > >> + build_serialization_instruction_entry(table_data, action, > >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >> + break; > >> + case ACPI_ERST_ACTION_END_OPERATION: > >> + build_serialization_instruction_entry(table_data, action, > >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >> + break; > >> + case ACPI_ERST_ACTION_SET_RECORD_OFFSET: > >> + build_serialization_instruction_entry(table_data, action, > >> + ACPI_ERST_INST_WRITE_REGISTER , 0, 32, > >> + s->bar0 + ERST_CSR_VALUE , 0, MASK32); > >> + build_serialization_instruction_entry(table_data, action, > >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >> + break; > >> + case ACPI_ERST_ACTION_EXECUTE_OPERATION: > >> + build_serialization_instruction_entry(table_data, action, > >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >> + s->bar0 + ERST_CSR_VALUE , ERST_EXECUTE_OPERATION_MAGIC, MASK8); > >> + build_serialization_instruction_entry(table_data, action, > >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >> + break; > >> + case ACPI_ERST_ACTION_CHECK_BUSY_STATUS: > >> + build_serialization_instruction_entry(table_data, action, > >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >> + build_serialization_instruction_entry(table_data, action, > >> + ACPI_ERST_INST_READ_REGISTER_VALUE , 0, 32, > >> + s->bar0 + ERST_CSR_VALUE, 0x01, MASK8); > >> + break; > >> + case ACPI_ERST_ACTION_GET_COMMAND_STATUS: > >> + build_serialization_instruction_entry(table_data, action, > >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >> + build_serialization_instruction_entry(table_data, action, > >> + ACPI_ERST_INST_READ_REGISTER , 0, 32, > >> + s->bar0 + ERST_CSR_VALUE, 0, MASK8); > >> + break; > >> + case ACPI_ERST_ACTION_GET_RECORD_IDENTIFIER: > >> + build_serialization_instruction_entry(table_data, action, > >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >> + build_serialization_instruction_entry(table_data, action, > >> + ACPI_ERST_INST_READ_REGISTER , 0, 64, > >> + s->bar0 + ERST_CSR_VALUE, 0, MASK64); > >> + break; > >> + case ACPI_ERST_ACTION_SET_RECORD_IDENTIFIER: > >> + build_serialization_instruction_entry(table_data, action, > >> + ACPI_ERST_INST_WRITE_REGISTER , 0, 64, > >> + s->bar0 + ERST_CSR_VALUE , 0, MASK64); > >> + build_serialization_instruction_entry(table_data, action, > >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >> + break; > >> + case ACPI_ERST_ACTION_GET_RECORD_COUNT: > >> + build_serialization_instruction_entry(table_data, action, > >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >> + build_serialization_instruction_entry(table_data, action, > >> + ACPI_ERST_INST_READ_REGISTER , 0, 32, > >> + s->bar0 + ERST_CSR_VALUE, 0, MASK32); > >> + break; > >> + case ACPI_ERST_ACTION_BEGIN_DUMMY_WRITE_OPERATION: > >> + build_serialization_instruction_entry(table_data, action, > >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >> + break; > >> + case ACPI_ERST_ACTION_RESERVED: > >> + build_serialization_instruction_entry(table_data, action, > >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >> + break; > >> + case ACPI_ERST_ACTION_GET_ERROR_LOG_ADDRESS_RANGE: > >> + build_serialization_instruction_entry(table_data, action, > >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >> + build_serialization_instruction_entry(table_data, action, > >> + ACPI_ERST_INST_READ_REGISTER , 0, 64, > >> + s->bar0 + ERST_CSR_VALUE, 0, MASK64); > >> + break; > >> + case ACPI_ERST_ACTION_GET_ERROR_LOG_ADDRESS_LENGTH: > >> + build_serialization_instruction_entry(table_data, action, > >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >> + build_serialization_instruction_entry(table_data, action, > >> + ACPI_ERST_INST_READ_REGISTER , 0, 64, > >> + s->bar0 + ERST_CSR_VALUE, 0, MASK32); > >> + break; > >> + case ACPI_ERST_ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES: > >> + build_serialization_instruction_entry(table_data, action, > >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >> + build_serialization_instruction_entry(table_data, action, > >> + ACPI_ERST_INST_READ_REGISTER , 0, 32, > >> + s->bar0 + ERST_CSR_VALUE, 0, MASK32); > >> + break; > >> + case ACPI_ERST_ACTION_GET_EXECUTE_OPERATION_TIMINGS: > >> + build_serialization_instruction_entry(table_data, action, > >> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >> + build_serialization_instruction_entry(table_data, action, > >> + ACPI_ERST_INST_READ_REGISTER , 0, 64, > >> + s->bar0 + ERST_CSR_VALUE, 0, MASK64); > >> + default: > >> + build_serialization_instruction_entry(table_data, action, > >> + ACPI_ERST_INST_NOOP, 0, 0, 0, action, 0); > >> + break; > >> + } > >> + } > >> + build_header(linker, table_data, > >> + (void *)(table_data->data + erst_start), > >> + "ERST", table_data->len - erst_start, > >> + 1, oem_id, oem_table_id); > >> +} > >> + > >> +/*******************************************************************/ > >> +/*******************************************************************/ > >> + > >> static const VMStateDescription erst_vmstate = { > >> .name = "acpi-erst", > >> .version_id = 1, > > >
On 7/26/21 6:00 AM, Igor Mammedov wrote: > On Wed, 21 Jul 2021 11:12:41 -0500 > Eric DeVolder <eric.devolder@oracle.com> wrote: > >> On 7/20/21 8:16 AM, Igor Mammedov wrote: >>> On Wed, 30 Jun 2021 15:07:17 -0400 >>> Eric DeVolder <eric.devolder@oracle.com> wrote: >>> >>>> This code is called from the machine code (if ACPI supported) >>>> to generate the ACPI ERST table. >>> should be along lines: >>> This builds ACPI ERST table /spec ref/ to inform OSMP >>> how to communicate with ... device. >> >> Like this? >> This builds the ACPI ERST table to inform OSMP how to communicate > ^ [1] >> with the acpi-erst device. >> > > 1) ACPI spec vX.Y, chapter foo done > >> >>> >>>> >>>> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com> >>>> --- >>>> hw/acpi/erst.c | 214 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 214 insertions(+) >>>> >>>> diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c >>>> index 6e9bd2e..1f1dbbc 100644 >>>> --- a/hw/acpi/erst.c >>>> +++ b/hw/acpi/erst.c >>>> @@ -555,6 +555,220 @@ static const MemoryRegionOps erst_mem_ops = { >>>> /*******************************************************************/ >>>> /*******************************************************************/ >>>> >>>> +/* 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) >>> like I mentioned in previous patch, It could be simplified >>> a lot if it's possible to use fixed 64-bit access with every >>> action and the same width mask. >> See previous response. > lets see if it's a guest OS issue first, and then decide what to do with it. > >> >>> >>>> +{ >>>> + /* 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) >>>> +{ >>>> + ERSTDeviceState *s = ACPIERST(erst_dev); >>> >>> globals are not welcomed in new code, >>> pass erst_dev as argument here (ex: find_vmgenid_dev) >>> >>>> + unsigned action; >>>> + unsigned erst_start = table_data->len; >>>> + >>> >>>> + s->bar0 = (hwaddr)pci_get_bar_addr(PCI_DEVICE(erst_dev), 0); >>>> + trace_acpi_erst_pci_bar_0(s->bar0); >>>> + s->bar1 = (hwaddr)pci_get_bar_addr(PCI_DEVICE(erst_dev), 1); >>> >>> just store pci_get_bar_addr(PCI_DEVICE(erst_dev), 0) in local variable, >>> Bar 1 is not used in this function so you don't need it here. >> Corrected >> >>> >>> >>>> + trace_acpi_erst_pci_bar_1(s->bar1); >>>> + >>>> + acpi_data_push(table_data, sizeof(AcpiTableHeader)); >>>> + /* serialization_header_length */ >>> comments documenting table entries should be verbatim copy from spec, >>> see build_amd_iommu() as example of preferred style. >> Corrected >> >>> >>>> + build_append_int_noprefix(table_data, 48, 4); >>>> + /* reserved */ >>>> + build_append_int_noprefix(table_data, 0, 4); >>>> + /* >>>> + * instruction_entry_count - changes to the number of serialization >>>> + * instructions in the ACTIONs below must be reflected in this >>>> + * pre-computed value. >>>> + */ >>>> + build_append_int_noprefix(table_data, 29, 4); >>> a bit fragile as it can easily diverge from actual number later on. >>> maybe instead of building instruction entries in place, build it >>> in separate array and when done, use actual count to fill instruction_entry_count. >>> pseudo code could look like: >>> >>> /* prepare instructions in advance because ... */ >>> GArray table_instruction_data; >>> build_serialization_instruction_entry(table_instruction_data,...);; >>> ... >>> build_serialization_instruction_entry(table_instruction_data,...); >>> /* instructions count */ >>> build_append_int_noprefix(table_data, table_instruction_data.len/entry_size, 4); >>> /* copy prepared in advance instructions */ >>> g_array_append_vals(table_data, table_instruction_data.data, table_instruction_data.len); >> Corrected >> >>> >>> >>>> + >>>> +#define MASK8 0x00000000000000FFUL >>>> +#define MASK16 0x000000000000FFFFUL >>>> +#define MASK32 0x00000000FFFFFFFFUL >>>> +#define MASK64 0xFFFFFFFFFFFFFFFFUL >>>> + >>>> + for (action = 0; action < ACPI_ERST_MAX_ACTIONS; ++action) { >>> I'd unroll this loop and just directly code entries in required order. >>> also drop reserved and nop actions/instructions or explain why they are necessary. >> Unrolled. Dropped the NOP. > > What about 'reserved"? I dropped Reserved as it was composed of a NOP, and isn't needed either. > >> >>> >>>> + switch (action) { >>>> + case ACPI_ERST_ACTION_BEGIN_WRITE_OPERATION: >>> given these names will/should never be exposed outside of hw/acpi/erst.c >>> I'd drop ACPI_ERST_ACTION_/ACPI_ERST_INST_ prefixes (i.e. use names as defined in spec) >>> if it doesn't cause build issues. >> These are in include/hw/acpi/erst.h which is included by hw/i386/acpi-build.c, >> which includes many other hardware files. >> Removing the prefix leaves a rather generic name. >> I'd prefer to leave them as it uniquely differentiates. > is there a reason to put them into erst.h and expose to outside world? > If not then it might be better to move them into erst.c I've moved them into erst.c > >> >> >>> >>>> + build_serialization_instruction_entry(table_data, action, >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>> + break; >>>> + case ACPI_ERST_ACTION_BEGIN_READ_OPERATION: >>>> + build_serialization_instruction_entry(table_data, action, >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>> + break; >>>> + case ACPI_ERST_ACTION_BEGIN_CLEAR_OPERATION: >>>> + build_serialization_instruction_entry(table_data, action, >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>> + break; >>>> + case ACPI_ERST_ACTION_END_OPERATION: >>>> + build_serialization_instruction_entry(table_data, action, >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>> + break; >>>> + case ACPI_ERST_ACTION_SET_RECORD_OFFSET: >>>> + build_serialization_instruction_entry(table_data, action, >>>> + ACPI_ERST_INST_WRITE_REGISTER , 0, 32, >>>> + s->bar0 + ERST_CSR_VALUE , 0, MASK32); >>>> + build_serialization_instruction_entry(table_data, action, >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>> + break; >>>> + case ACPI_ERST_ACTION_EXECUTE_OPERATION: >>>> + build_serialization_instruction_entry(table_data, action, >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + s->bar0 + ERST_CSR_VALUE , ERST_EXECUTE_OPERATION_MAGIC, MASK8); >>>> + build_serialization_instruction_entry(table_data, action, >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>> + break; >>>> + case ACPI_ERST_ACTION_CHECK_BUSY_STATUS: >>>> + build_serialization_instruction_entry(table_data, action, >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>> + build_serialization_instruction_entry(table_data, action, >>>> + ACPI_ERST_INST_READ_REGISTER_VALUE , 0, 32, >>>> + s->bar0 + ERST_CSR_VALUE, 0x01, MASK8); >>>> + break; >>>> + case ACPI_ERST_ACTION_GET_COMMAND_STATUS: >>>> + build_serialization_instruction_entry(table_data, action, >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>> + build_serialization_instruction_entry(table_data, action, >>>> + ACPI_ERST_INST_READ_REGISTER , 0, 32, >>>> + s->bar0 + ERST_CSR_VALUE, 0, MASK8); >>>> + break; >>>> + case ACPI_ERST_ACTION_GET_RECORD_IDENTIFIER: >>>> + build_serialization_instruction_entry(table_data, action, >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>> + build_serialization_instruction_entry(table_data, action, >>>> + ACPI_ERST_INST_READ_REGISTER , 0, 64, >>>> + s->bar0 + ERST_CSR_VALUE, 0, MASK64); >>>> + break; >>>> + case ACPI_ERST_ACTION_SET_RECORD_IDENTIFIER: >>>> + build_serialization_instruction_entry(table_data, action, >>>> + ACPI_ERST_INST_WRITE_REGISTER , 0, 64, >>>> + s->bar0 + ERST_CSR_VALUE , 0, MASK64); >>>> + build_serialization_instruction_entry(table_data, action, >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>> + break; >>>> + case ACPI_ERST_ACTION_GET_RECORD_COUNT: >>>> + build_serialization_instruction_entry(table_data, action, >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>> + build_serialization_instruction_entry(table_data, action, >>>> + ACPI_ERST_INST_READ_REGISTER , 0, 32, >>>> + s->bar0 + ERST_CSR_VALUE, 0, MASK32); >>>> + break; >>>> + case ACPI_ERST_ACTION_BEGIN_DUMMY_WRITE_OPERATION: >>>> + build_serialization_instruction_entry(table_data, action, >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>> + break; >>>> + case ACPI_ERST_ACTION_RESERVED: >>>> + build_serialization_instruction_entry(table_data, action, >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>> + break; >>>> + case ACPI_ERST_ACTION_GET_ERROR_LOG_ADDRESS_RANGE: >>>> + build_serialization_instruction_entry(table_data, action, >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>> + build_serialization_instruction_entry(table_data, action, >>>> + ACPI_ERST_INST_READ_REGISTER , 0, 64, >>>> + s->bar0 + ERST_CSR_VALUE, 0, MASK64); >>>> + break; >>>> + case ACPI_ERST_ACTION_GET_ERROR_LOG_ADDRESS_LENGTH: >>>> + build_serialization_instruction_entry(table_data, action, >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>> + build_serialization_instruction_entry(table_data, action, >>>> + ACPI_ERST_INST_READ_REGISTER , 0, 64, >>>> + s->bar0 + ERST_CSR_VALUE, 0, MASK32); >>>> + break; >>>> + case ACPI_ERST_ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES: >>>> + build_serialization_instruction_entry(table_data, action, >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>> + build_serialization_instruction_entry(table_data, action, >>>> + ACPI_ERST_INST_READ_REGISTER , 0, 32, >>>> + s->bar0 + ERST_CSR_VALUE, 0, MASK32); >>>> + break; >>>> + case ACPI_ERST_ACTION_GET_EXECUTE_OPERATION_TIMINGS: >>>> + build_serialization_instruction_entry(table_data, action, >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>> + build_serialization_instruction_entry(table_data, action, >>>> + ACPI_ERST_INST_READ_REGISTER , 0, 64, >>>> + s->bar0 + ERST_CSR_VALUE, 0, MASK64); >>>> + default: >>>> + build_serialization_instruction_entry(table_data, action, >>>> + ACPI_ERST_INST_NOOP, 0, 0, 0, action, 0); >>>> + break; >>>> + } >>>> + } >>>> + build_header(linker, table_data, >>>> + (void *)(table_data->data + erst_start), >>>> + "ERST", table_data->len - erst_start, >>>> + 1, oem_id, oem_table_id); >>>> +} >>>> + >>>> +/*******************************************************************/ >>>> +/*******************************************************************/ >>>> + >>>> static const VMStateDescription erst_vmstate = { >>>> .name = "acpi-erst", >>>> .version_id = 1, >>> >> >
On Mon, 26 Jul 2021 15:02:55 -0500 Eric DeVolder <eric.devolder@oracle.com> wrote: [...] > >>>> + build_serialization_instruction_entry(table_data, action, > >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >>>> + break; > >>>> + case ACPI_ERST_ACTION_BEGIN_READ_OPERATION: > >>>> + build_serialization_instruction_entry(table_data, action, > >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >>>> + break; > >>>> + case ACPI_ERST_ACTION_BEGIN_CLEAR_OPERATION: > >>>> + build_serialization_instruction_entry(table_data, action, > >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >>>> + break; > >>>> + case ACPI_ERST_ACTION_END_OPERATION: > >>>> + build_serialization_instruction_entry(table_data, action, > >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >>>> + break; > >>>> + case ACPI_ERST_ACTION_SET_RECORD_OFFSET: > >>>> + build_serialization_instruction_entry(table_data, action, > >>>> + ACPI_ERST_INST_WRITE_REGISTER , 0, 32, > >>>> + s->bar0 + ERST_CSR_VALUE , 0, MASK32); > >>>> + build_serialization_instruction_entry(table_data, action, > >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >>>> + break; > >>>> + case ACPI_ERST_ACTION_EXECUTE_OPERATION: > >>>> + build_serialization_instruction_entry(table_data, action, > >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + s->bar0 + ERST_CSR_VALUE , ERST_EXECUTE_OPERATION_MAGIC, MASK8); > >>>> + build_serialization_instruction_entry(table_data, action, > >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >>>> + break; > >>>> + case ACPI_ERST_ACTION_CHECK_BUSY_STATUS: > >>>> + build_serialization_instruction_entry(table_data, action, > >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >>>> + build_serialization_instruction_entry(table_data, action, > >>>> + ACPI_ERST_INST_READ_REGISTER_VALUE , 0, 32, > >>>> + s->bar0 + ERST_CSR_VALUE, 0x01, MASK8); > >>>> + break; > >>>> + case ACPI_ERST_ACTION_GET_COMMAND_STATUS: > >>>> + build_serialization_instruction_entry(table_data, action, > >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >>>> + build_serialization_instruction_entry(table_data, action, > >>>> + ACPI_ERST_INST_READ_REGISTER , 0, 32, > >>>> + s->bar0 + ERST_CSR_VALUE, 0, MASK8); > >>>> + break; > >>>> + case ACPI_ERST_ACTION_GET_RECORD_IDENTIFIER: > >>>> + build_serialization_instruction_entry(table_data, action, > >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >>>> + build_serialization_instruction_entry(table_data, action, > >>>> + ACPI_ERST_INST_READ_REGISTER , 0, 64, > >>>> + s->bar0 + ERST_CSR_VALUE, 0, MASK64); > >>>> + break; > >>>> + case ACPI_ERST_ACTION_SET_RECORD_IDENTIFIER: > >>>> + build_serialization_instruction_entry(table_data, action, > >>>> + ACPI_ERST_INST_WRITE_REGISTER , 0, 64, > >>>> + s->bar0 + ERST_CSR_VALUE , 0, MASK64); > >>>> + build_serialization_instruction_entry(table_data, action, > >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >>>> + break; > >>>> + case ACPI_ERST_ACTION_GET_RECORD_COUNT: > >>>> + build_serialization_instruction_entry(table_data, action, > >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >>>> + build_serialization_instruction_entry(table_data, action, > >>>> + ACPI_ERST_INST_READ_REGISTER , 0, 32, > >>>> + s->bar0 + ERST_CSR_VALUE, 0, MASK32); > >>>> + break; > >>>> + case ACPI_ERST_ACTION_BEGIN_DUMMY_WRITE_OPERATION: > >>>> + build_serialization_instruction_entry(table_data, action, > >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >>>> + break; > >>>> + case ACPI_ERST_ACTION_RESERVED: > >>>> + build_serialization_instruction_entry(table_data, action, > >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >>>> + break; > >>>> + case ACPI_ERST_ACTION_GET_ERROR_LOG_ADDRESS_RANGE: > >>>> + build_serialization_instruction_entry(table_data, action, > >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >>>> + build_serialization_instruction_entry(table_data, action, > >>>> + ACPI_ERST_INST_READ_REGISTER , 0, 64, > >>>> + s->bar0 + ERST_CSR_VALUE, 0, MASK64); > >>>> + break; > >>>> + case ACPI_ERST_ACTION_GET_ERROR_LOG_ADDRESS_LENGTH: > >>>> + build_serialization_instruction_entry(table_data, action, > >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >>>> + build_serialization_instruction_entry(table_data, action, > >>>> + ACPI_ERST_INST_READ_REGISTER , 0, 64, > >>>> + s->bar0 + ERST_CSR_VALUE, 0, MASK32); > >>>> + break; > >>>> + case ACPI_ERST_ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES: > >>>> + build_serialization_instruction_entry(table_data, action, > >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >>>> + build_serialization_instruction_entry(table_data, action, > >>>> + ACPI_ERST_INST_READ_REGISTER , 0, 32, > >>>> + s->bar0 + ERST_CSR_VALUE, 0, MASK32); > >>>> + break; > >>>> + case ACPI_ERST_ACTION_GET_EXECUTE_OPERATION_TIMINGS: > >>>> + build_serialization_instruction_entry(table_data, action, > >>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, > >>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); > >>>> + build_serialization_instruction_entry(table_data, action, > >>>> + ACPI_ERST_INST_READ_REGISTER , 0, 64, > >>>> + s->bar0 + ERST_CSR_VALUE, 0, MASK64); > >>>> + default: > >>>> + build_serialization_instruction_entry(table_data, action, > >>>> + ACPI_ERST_INST_NOOP, 0, 0, 0, action, 0); > >>>> + break; > >>>> + } ../../builds/imammedo/qemu/hw/acpi/erst.c: In function ‘build_erst’: ../../builds/imammedo/qemu/hw/acpi/erst.c:754:13: error: this statement may fall through [-Werror=implicit-fallthrough=] build_serialization_instruction_entry(table_data, action, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ACPI_ERST_INST_READ_REGISTER , 0, 64, ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ s->bar0 + ERST_CSR_VALUE, 0, MASK64); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../builds/imammedo/qemu/hw/acpi/erst.c:757:9: note: here default: ^~~~~~~ cc1: all warnings being treated as errors > >>>> + } > >>>> + build_header(linker, table_data, > >>>> + (void *)(table_data->data + erst_start), > >>>> + "ERST", table_data->len - erst_start, > >>>> + 1, oem_id, oem_table_id); > >>>> +} > >>>> + > >>>> +/*******************************************************************/ > >>>> +/*******************************************************************/ > >>>> + > >>>> static const VMStateDescription erst_vmstate = { > >>>> .name = "acpi-erst", > >>>> .version_id = 1, > >>> > >> > > >
On 7/27/21 7:01 AM, Igor Mammedov wrote: > On Mon, 26 Jul 2021 15:02:55 -0500 > Eric DeVolder <eric.devolder@oracle.com> wrote: > > [...] >>>>>> + build_serialization_instruction_entry(table_data, action, >>>>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>>>> + break; >>>>>> + case ACPI_ERST_ACTION_BEGIN_READ_OPERATION: >>>>>> + build_serialization_instruction_entry(table_data, action, >>>>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>>>> + break; >>>>>> + case ACPI_ERST_ACTION_BEGIN_CLEAR_OPERATION: >>>>>> + build_serialization_instruction_entry(table_data, action, >>>>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>>>> + break; >>>>>> + case ACPI_ERST_ACTION_END_OPERATION: >>>>>> + build_serialization_instruction_entry(table_data, action, >>>>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>>>> + break; >>>>>> + case ACPI_ERST_ACTION_SET_RECORD_OFFSET: >>>>>> + build_serialization_instruction_entry(table_data, action, >>>>>> + ACPI_ERST_INST_WRITE_REGISTER , 0, 32, >>>>>> + s->bar0 + ERST_CSR_VALUE , 0, MASK32); >>>>>> + build_serialization_instruction_entry(table_data, action, >>>>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>>>> + break; >>>>>> + case ACPI_ERST_ACTION_EXECUTE_OPERATION: >>>>>> + build_serialization_instruction_entry(table_data, action, >>>>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>>>> + s->bar0 + ERST_CSR_VALUE , ERST_EXECUTE_OPERATION_MAGIC, MASK8); >>>>>> + build_serialization_instruction_entry(table_data, action, >>>>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>>>> + break; >>>>>> + case ACPI_ERST_ACTION_CHECK_BUSY_STATUS: >>>>>> + build_serialization_instruction_entry(table_data, action, >>>>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>>>> + build_serialization_instruction_entry(table_data, action, >>>>>> + ACPI_ERST_INST_READ_REGISTER_VALUE , 0, 32, >>>>>> + s->bar0 + ERST_CSR_VALUE, 0x01, MASK8); >>>>>> + break; >>>>>> + case ACPI_ERST_ACTION_GET_COMMAND_STATUS: >>>>>> + build_serialization_instruction_entry(table_data, action, >>>>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>>>> + build_serialization_instruction_entry(table_data, action, >>>>>> + ACPI_ERST_INST_READ_REGISTER , 0, 32, >>>>>> + s->bar0 + ERST_CSR_VALUE, 0, MASK8); >>>>>> + break; >>>>>> + case ACPI_ERST_ACTION_GET_RECORD_IDENTIFIER: >>>>>> + build_serialization_instruction_entry(table_data, action, >>>>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>>>> + build_serialization_instruction_entry(table_data, action, >>>>>> + ACPI_ERST_INST_READ_REGISTER , 0, 64, >>>>>> + s->bar0 + ERST_CSR_VALUE, 0, MASK64); >>>>>> + break; >>>>>> + case ACPI_ERST_ACTION_SET_RECORD_IDENTIFIER: >>>>>> + build_serialization_instruction_entry(table_data, action, >>>>>> + ACPI_ERST_INST_WRITE_REGISTER , 0, 64, >>>>>> + s->bar0 + ERST_CSR_VALUE , 0, MASK64); >>>>>> + build_serialization_instruction_entry(table_data, action, >>>>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>>>> + break; >>>>>> + case ACPI_ERST_ACTION_GET_RECORD_COUNT: >>>>>> + build_serialization_instruction_entry(table_data, action, >>>>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>>>> + build_serialization_instruction_entry(table_data, action, >>>>>> + ACPI_ERST_INST_READ_REGISTER , 0, 32, >>>>>> + s->bar0 + ERST_CSR_VALUE, 0, MASK32); >>>>>> + break; >>>>>> + case ACPI_ERST_ACTION_BEGIN_DUMMY_WRITE_OPERATION: >>>>>> + build_serialization_instruction_entry(table_data, action, >>>>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>>>> + break; >>>>>> + case ACPI_ERST_ACTION_RESERVED: >>>>>> + build_serialization_instruction_entry(table_data, action, >>>>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>>>> + break; >>>>>> + case ACPI_ERST_ACTION_GET_ERROR_LOG_ADDRESS_RANGE: >>>>>> + build_serialization_instruction_entry(table_data, action, >>>>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>>>> + build_serialization_instruction_entry(table_data, action, >>>>>> + ACPI_ERST_INST_READ_REGISTER , 0, 64, >>>>>> + s->bar0 + ERST_CSR_VALUE, 0, MASK64); >>>>>> + break; >>>>>> + case ACPI_ERST_ACTION_GET_ERROR_LOG_ADDRESS_LENGTH: >>>>>> + build_serialization_instruction_entry(table_data, action, >>>>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>>>> + build_serialization_instruction_entry(table_data, action, >>>>>> + ACPI_ERST_INST_READ_REGISTER , 0, 64, >>>>>> + s->bar0 + ERST_CSR_VALUE, 0, MASK32); >>>>>> + break; >>>>>> + case ACPI_ERST_ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES: >>>>>> + build_serialization_instruction_entry(table_data, action, >>>>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>>>> + build_serialization_instruction_entry(table_data, action, >>>>>> + ACPI_ERST_INST_READ_REGISTER , 0, 32, >>>>>> + s->bar0 + ERST_CSR_VALUE, 0, MASK32); >>>>>> + break; >>>>>> + case ACPI_ERST_ACTION_GET_EXECUTE_OPERATION_TIMINGS: >>>>>> + build_serialization_instruction_entry(table_data, action, >>>>>> + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, >>>>>> + s->bar0 + ERST_CSR_ACTION, action, MASK8); >>>>>> + build_serialization_instruction_entry(table_data, action, >>>>>> + ACPI_ERST_INST_READ_REGISTER , 0, 64, >>>>>> + s->bar0 + ERST_CSR_VALUE, 0, MASK64); >>>>>> + default: >>>>>> + build_serialization_instruction_entry(table_data, action, >>>>>> + ACPI_ERST_INST_NOOP, 0, 0, 0, action, 0); >>>>>> + break; >>>>>> + } > > ../../builds/imammedo/qemu/hw/acpi/erst.c: In function ‘build_erst’: > ../../builds/imammedo/qemu/hw/acpi/erst.c:754:13: error: this statement may fall through [-Werror=implicit-fallthrough=] > build_serialization_instruction_entry(table_data, action, > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ACPI_ERST_INST_READ_REGISTER , 0, 64, > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > s->bar0 + ERST_CSR_VALUE, 0, MASK64); > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ../../builds/imammedo/qemu/hw/acpi/erst.c:757:9: note: here > default: > ^~~~~~~ > cc1: all warnings being treated as errors Michael pointed this one out last week, I've since corrected it. > > >>>>>> + } >>>>>> + build_header(linker, table_data, >>>>>> + (void *)(table_data->data + erst_start), >>>>>> + "ERST", table_data->len - erst_start, >>>>>> + 1, oem_id, oem_table_id); >>>>>> +} >>>>>> + >>>>>> +/*******************************************************************/ >>>>>> +/*******************************************************************/ >>>>>> + >>>>>> static const VMStateDescription erst_vmstate = { >>>>>> .name = "acpi-erst", >>>>>> .version_id = 1, >>>>> >>>> >>> >> >
diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c index 6e9bd2e..1f1dbbc 100644 --- a/hw/acpi/erst.c +++ b/hw/acpi/erst.c @@ -555,6 +555,220 @@ static const MemoryRegionOps erst_mem_ops = { /*******************************************************************/ /*******************************************************************/ +/* 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) +{ + ERSTDeviceState *s = ACPIERST(erst_dev); + unsigned action; + unsigned erst_start = table_data->len; + + s->bar0 = (hwaddr)pci_get_bar_addr(PCI_DEVICE(erst_dev), 0); + trace_acpi_erst_pci_bar_0(s->bar0); + s->bar1 = (hwaddr)pci_get_bar_addr(PCI_DEVICE(erst_dev), 1); + trace_acpi_erst_pci_bar_1(s->bar1); + + acpi_data_push(table_data, sizeof(AcpiTableHeader)); + /* serialization_header_length */ + build_append_int_noprefix(table_data, 48, 4); + /* reserved */ + build_append_int_noprefix(table_data, 0, 4); + /* + * instruction_entry_count - changes to the number of serialization + * instructions in the ACTIONs below must be reflected in this + * pre-computed value. + */ + build_append_int_noprefix(table_data, 29, 4); + +#define MASK8 0x00000000000000FFUL +#define MASK16 0x000000000000FFFFUL +#define MASK32 0x00000000FFFFFFFFUL +#define MASK64 0xFFFFFFFFFFFFFFFFUL + + for (action = 0; action < ACPI_ERST_MAX_ACTIONS; ++action) { + switch (action) { + case ACPI_ERST_ACTION_BEGIN_WRITE_OPERATION: + build_serialization_instruction_entry(table_data, action, + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, + s->bar0 + ERST_CSR_ACTION, action, MASK8); + break; + case ACPI_ERST_ACTION_BEGIN_READ_OPERATION: + build_serialization_instruction_entry(table_data, action, + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, + s->bar0 + ERST_CSR_ACTION, action, MASK8); + break; + case ACPI_ERST_ACTION_BEGIN_CLEAR_OPERATION: + build_serialization_instruction_entry(table_data, action, + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, + s->bar0 + ERST_CSR_ACTION, action, MASK8); + break; + case ACPI_ERST_ACTION_END_OPERATION: + build_serialization_instruction_entry(table_data, action, + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, + s->bar0 + ERST_CSR_ACTION, action, MASK8); + break; + case ACPI_ERST_ACTION_SET_RECORD_OFFSET: + build_serialization_instruction_entry(table_data, action, + ACPI_ERST_INST_WRITE_REGISTER , 0, 32, + s->bar0 + ERST_CSR_VALUE , 0, MASK32); + build_serialization_instruction_entry(table_data, action, + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, + s->bar0 + ERST_CSR_ACTION, action, MASK8); + break; + case ACPI_ERST_ACTION_EXECUTE_OPERATION: + build_serialization_instruction_entry(table_data, action, + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, + s->bar0 + ERST_CSR_VALUE , ERST_EXECUTE_OPERATION_MAGIC, MASK8); + build_serialization_instruction_entry(table_data, action, + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, + s->bar0 + ERST_CSR_ACTION, action, MASK8); + break; + case ACPI_ERST_ACTION_CHECK_BUSY_STATUS: + build_serialization_instruction_entry(table_data, action, + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, + s->bar0 + ERST_CSR_ACTION, action, MASK8); + build_serialization_instruction_entry(table_data, action, + ACPI_ERST_INST_READ_REGISTER_VALUE , 0, 32, + s->bar0 + ERST_CSR_VALUE, 0x01, MASK8); + break; + case ACPI_ERST_ACTION_GET_COMMAND_STATUS: + build_serialization_instruction_entry(table_data, action, + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, + s->bar0 + ERST_CSR_ACTION, action, MASK8); + build_serialization_instruction_entry(table_data, action, + ACPI_ERST_INST_READ_REGISTER , 0, 32, + s->bar0 + ERST_CSR_VALUE, 0, MASK8); + break; + case ACPI_ERST_ACTION_GET_RECORD_IDENTIFIER: + build_serialization_instruction_entry(table_data, action, + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, + s->bar0 + ERST_CSR_ACTION, action, MASK8); + build_serialization_instruction_entry(table_data, action, + ACPI_ERST_INST_READ_REGISTER , 0, 64, + s->bar0 + ERST_CSR_VALUE, 0, MASK64); + break; + case ACPI_ERST_ACTION_SET_RECORD_IDENTIFIER: + build_serialization_instruction_entry(table_data, action, + ACPI_ERST_INST_WRITE_REGISTER , 0, 64, + s->bar0 + ERST_CSR_VALUE , 0, MASK64); + build_serialization_instruction_entry(table_data, action, + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, + s->bar0 + ERST_CSR_ACTION, action, MASK8); + break; + case ACPI_ERST_ACTION_GET_RECORD_COUNT: + build_serialization_instruction_entry(table_data, action, + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, + s->bar0 + ERST_CSR_ACTION, action, MASK8); + build_serialization_instruction_entry(table_data, action, + ACPI_ERST_INST_READ_REGISTER , 0, 32, + s->bar0 + ERST_CSR_VALUE, 0, MASK32); + break; + case ACPI_ERST_ACTION_BEGIN_DUMMY_WRITE_OPERATION: + build_serialization_instruction_entry(table_data, action, + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, + s->bar0 + ERST_CSR_ACTION, action, MASK8); + break; + case ACPI_ERST_ACTION_RESERVED: + build_serialization_instruction_entry(table_data, action, + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, + s->bar0 + ERST_CSR_ACTION, action, MASK8); + break; + case ACPI_ERST_ACTION_GET_ERROR_LOG_ADDRESS_RANGE: + build_serialization_instruction_entry(table_data, action, + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, + s->bar0 + ERST_CSR_ACTION, action, MASK8); + build_serialization_instruction_entry(table_data, action, + ACPI_ERST_INST_READ_REGISTER , 0, 64, + s->bar0 + ERST_CSR_VALUE, 0, MASK64); + break; + case ACPI_ERST_ACTION_GET_ERROR_LOG_ADDRESS_LENGTH: + build_serialization_instruction_entry(table_data, action, + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, + s->bar0 + ERST_CSR_ACTION, action, MASK8); + build_serialization_instruction_entry(table_data, action, + ACPI_ERST_INST_READ_REGISTER , 0, 64, + s->bar0 + ERST_CSR_VALUE, 0, MASK32); + break; + case ACPI_ERST_ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES: + build_serialization_instruction_entry(table_data, action, + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, + s->bar0 + ERST_CSR_ACTION, action, MASK8); + build_serialization_instruction_entry(table_data, action, + ACPI_ERST_INST_READ_REGISTER , 0, 32, + s->bar0 + ERST_CSR_VALUE, 0, MASK32); + break; + case ACPI_ERST_ACTION_GET_EXECUTE_OPERATION_TIMINGS: + build_serialization_instruction_entry(table_data, action, + ACPI_ERST_INST_WRITE_REGISTER_VALUE, 0, 32, + s->bar0 + ERST_CSR_ACTION, action, MASK8); + build_serialization_instruction_entry(table_data, action, + ACPI_ERST_INST_READ_REGISTER , 0, 64, + s->bar0 + ERST_CSR_VALUE, 0, MASK64); + default: + build_serialization_instruction_entry(table_data, action, + ACPI_ERST_INST_NOOP, 0, 0, 0, action, 0); + break; + } + } + build_header(linker, table_data, + (void *)(table_data->data + erst_start), + "ERST", table_data->len - erst_start, + 1, oem_id, oem_table_id); +} + +/*******************************************************************/ +/*******************************************************************/ + static const VMStateDescription erst_vmstate = { .name = "acpi-erst", .version_id = 1,
This code is called from the machine code (if ACPI supported) to generate the ACPI ERST table. Signed-off-by: Eric DeVolder <eric.devolder@oracle.com> --- hw/acpi/erst.c | 214 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 214 insertions(+)