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

[Eric DeVolder <eric.devolder@oracle.com>]

On 1/28/22 11:25, Ani Sinha wrote:
> 
> [snip]
> On Fri, Jan 28, 2022 at 9:44 PM Michael S. Tsirkin <mst@redhat.com <mailto:mst@redhat.com>> wrote:
> 

>      > > > OK, here is the equivalent using struct assignment, is this what you were after?
>      > > >
>      > > >      BuildSerializationInstructionEntry base = {
>      > > >          .table_data = table_instruction_data,
>      > > >          .bar = bar0,
>      > > >          .instruction = INST_WRITE_REGISTER,
>      > > >          .flags = 0,
>      > > >          .register_bit_width = 32,
>      > > >          .register_offset = ERST_VALUE_OFFSET,
>      > > >      };
>      > > >      BuildSerializationInstructionEntry rd_value_32_val = base;
>      > > >      rd_value_32_val.instruction = INST_READ_REGISTER_VALUE;
>      > > >      BuildSerializationInstructionEntry rd_value_32 = base;
>      > > >      rd_value_32.instruction = INST_READ_REGISTER;
>      > > >      BuildSerializationInstructionEntry rd_value_64 = base;
>      > > >      rd_value_64.instruction = INST_READ_REGISTER;
>      > > >      rd_value_64.register_bit_width = 64;
>      > > >      BuildSerializationInstructionEntry wr_value_32_val = base;
>      > > >      wr_value_32_val.instruction = INST_WRITE_REGISTER_VALUE;
>      > > >      BuildSerializationInstructionEntry wr_value_32 = base;
>      > > >      BuildSerializationInstructionEntry wr_value_64 = base;
>      > > >      wr_value_64.register_bit_width = 64;
>      > > >      BuildSerializationInstructionEntry wr_action = base;
>      > > >      wr_action.instruction = INST_WRITE_REGISTER_VALUE;
>      > > >      wr_action.register_offset = ERST_ACTION_OFFSET;
>      > > >
>      > >
>      > > That's what I described, yes. We should have some empty lines here I
>      > > guess. I'm ok with the original one too, there's not too much
>      > > duplication.
>      >
>      > Are the blank lines referring to spacing out the setup of each of the 7 accesors?
>      > If so, I could put a one line comment between each setup? Or is a blank line also
>      > needed?
> 
>     A blank line between declarations and code is usually a good idea.
> 
> 
>      > Is it OK to post v15 with the struct assignment approach? Or would you prefer the
>      > explicit structs (which is what I think you mean by 'the original one')?
> 
> 
> I prefer the explicit structs as you had posted before.

Ok, as Michael does not have a preference, so let's go with your preference of the
explicit structs!
Thank you!
eric

> 
> 
>      >
>      > Thanks!
>      > eric
> 
>     I don't care either way.
> 
>      > >
>      > >
>      > > >
>      > > > >
>      > > > >
>      > > > > > #define SERIALIZATIONINSTRUCTIONCTX(name, \
>      > > > > >       inst, bit_width, offset) \
>      > > > > >       BuildSerializationInstructionEntry name = { \
>      > > > > >           .table_data = table_instruction_data, \
>      > > > > >           .bar = bar0, \
>      > > > > >           .instruction = inst, \
>      > > > > >           .flags = 0, \
>      > > > > >           .register_bit_width = bit_width, \
>      > > > > >           .register_offset = offset, \
>      > > > > >       }
>      > > > > >       SERIALIZATIONINSTRUCTIONCTX(rd_value_32_val,
>      > > > > >           INST_READ_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
>      > > > > >       SERIALIZATIONINSTRUCTIONCTX(rd_value_32,
>      > > > > >           INST_READ_REGISTER, 32, ERST_VALUE_OFFSET);
>      > > > > >       SERIALIZATIONINSTRUCTIONCTX(rd_value_64,
>      > > > > >           INST_READ_REGISTER, 64, ERST_VALUE_OFFSET);
>      > > > > >       SERIALIZATIONINSTRUCTIONCTX(wr_value_32_val,
>      > > > > >           INST_WRITE_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
>      > > > > >       SERIALIZATIONINSTRUCTIONCTX(wr_value_32,
>      > > > > >           INST_WRITE_REGISTER, 32, ERST_VALUE_OFFSET);
>      > > > > >       SERIALIZATIONINSTRUCTIONCTX(wr_value_64,
>      > > > > >           INST_WRITE_REGISTER, 64, ERST_VALUE_OFFSET);
>      > > > > >       SERIALIZATIONINSTRUCTIONCTX(wr_action,
>      > > > > >           INST_WRITE_REGISTER_VALUE, 32, ERST_ACTION_OFFSET);
>      > > > > >
>      > > > > > These are the 7 accessors needed.
>      > > > >
>      > > > > not at all sure this one is worth the macro mess.
>      > > >
>      > > > I'm hoping to produce a v15 with the style you want.
>      > > > eric
>      > > >
>      > > > >
>      > > > > > >
>      > > > > > > > +    unsigned action;
>      > > > > > > > +
>      > > > > > > > +    trace_acpi_erst_pci_bar_0(bar0);
>      > > > > > > > +
>      > > > > > > > +    /* Serialization Instruction Entries */
>      > > > > > > > +    action = ACTION_BEGIN_WRITE_OPERATION;
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_BEGIN_READ_OPERATION;
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_BEGIN_CLEAR_OPERATION;
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_END_OPERATION;
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_SET_RECORD_OFFSET;
>      > > > > > > > +    build_serialization_instruction(&wr_value_32, action, 0);
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_EXECUTE_OPERATION;
>      > > > > > > > +    build_serialization_instruction(&wr_value_32_val, action,
>      > > > > > > > +        ERST_EXECUTE_OPERATION_MAGIC);
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_CHECK_BUSY_STATUS;
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +    build_serialization_instruction(&rd_value_32_val, action, 0x01);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_GET_COMMAND_STATUS;
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +    build_serialization_instruction(&rd_value_32, action, 0);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_GET_RECORD_IDENTIFIER;
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +    build_serialization_instruction(&rd_value_64, action, 0);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_SET_RECORD_IDENTIFIER;
>      > > > > > > > +    build_serialization_instruction(&wr_value_64, action, 0);
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_GET_RECORD_COUNT;
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +    build_serialization_instruction(&rd_value_32, action, 0);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_BEGIN_DUMMY_WRITE_OPERATION;
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE;
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +    build_serialization_instruction(&rd_value_64, action, 0);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH;
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +    build_serialization_instruction(&rd_value_64, action, 0);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES;
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +    build_serialization_instruction(&rd_value_32, action, 0);
>      > > > > > > > +
>      > > > > > > > +    action = ACTION_GET_EXECUTE_OPERATION_TIMINGS;
>      > > > > > > > +    build_serialization_instruction(&wr_action, action, action);
>      > > > > > > > +    build_serialization_instruction(&rd_value_64, action, 0);
>      > > > > > > > +
>      > > > > > > > +    /* Serialization Header */
>      > > > > > > > +    acpi_table_begin(&table, table_data);
>      > > > > > > > +
>      > > > > > > > +    /* Serialization Header Size */
>      > > > > > > > +    build_append_int_noprefix(table_data, 48, 4);
>      > > > > > > > +
>      > > > > > > > +    /* Reserved */
>      > > > > > > > +    build_append_int_noprefix(table_data,  0, 4);
>      > > > > > > > +
>      > > > > > > > +    /*
>      > > > > > > > +     * Instruction Entry Count
>      > > > > > > > +     * Each instruction entry is 32 bytes
>      > > > > > > > +     */
>      > > > > > > > +    g_assert((table_instruction_data->len) % 32 == 0);
>      > > > > > > > +    build_append_int_noprefix(table_data,
>      > > > > > > > +        (table_instruction_data->len / 32), 4);
>      > > > > > > > +
>      > > > > > > > +    /* Serialization Instruction Entries */
>      > > > > > > > +    g_array_append_vals(table_data, table_instruction_data->data,
>      > > > > > > > +        table_instruction_data->len);
>      > > > > > > > +    g_array_free(table_instruction_data, TRUE);
>      > > > > > > > +
>      > > > > > > > +    acpi_table_end(linker, &table);
>      > > > > > > > +}
>      > > > > > > > +
>      > > > > > > > +/*******************************************************************/
>      > > > > > > > +/*******************************************************************/
>      > > > > > > >     static uint8_t *get_nvram_ptr_by_index(ERSTDeviceState *s, unsigned index)
>      > > > > > > >     {
>      > > > > > > >         uint8_t *rc = NULL;
>      > > > > > > > --
>      > > > > > > > 1.8.3.1
>      > > > > > > >
>      > > > > > > >
>      > > > >
>      > >
>