Re: [PATCH v5 06/10] ACPI ERST: build the ACPI ERST table

[Igor Mammedov <imammedo@redhat.com>]

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,