qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Eric DeVolder
Subject: Re: [PATCH v14 06/10] ACPI ERST: build the ACPI ERST table
Date: Fri, 28 Jan 2022 12:18:19 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 


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
     > > > > > > >
     > > > > > > >
     > > > >
     > >
reply via email to
[Prev in Thread] Current Thread [Next in Thread]