Re: [Qemu-devel] [PATCH v17 05/10] acpi: add build_append_ghes_generic_status() helper for Generic Error Status Block

[gengdongjiu <gengdongjiu@huawei.com>]

On 2019/6/20 20:42, Igor Mammedov wrote:
> On Tue, 14 May 2019 04:18:18 -0700
> Dongjiu Geng <gengdongjiu@huawei.com> wrote:
> 
>> It will help to add Generic Error Status Block to ACPI tables
>> without using packed C structures and avoid endianness
>> issues as API doesn't need explicit conversion.
>>
>> Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
>> ---
>>  hw/acpi/aml-build.c         | 14 ++++++++++++++
>>  include/hw/acpi/aml-build.h |  6 ++++++
>>  2 files changed, 20 insertions(+)
>>
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index 102a288..ce90970 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -296,6 +296,20 @@ void build_append_ghes_notify(GArray *table, const uint8_t type,
>>          build_append_int_noprefix(table, error_threshold_window, 4);
>>  }
>>  
>> +/* Generic Error Status Block
>> + * ACPI 4.0: 17.3.2.6.1 Generic Error Data
>> + */
>> +void build_append_ghes_generic_status(GArray *table, uint32_t block_status,
> maybe ..._generic_error_status???
good point, the build_append_ghes_generic_error_status() is better than build_append_ghes_generic_status()

> 
>> +                      uint32_t raw_data_offset, uint32_t raw_data_length,
>> +                      uint32_t data_length, uint32_t error_severity)
> see CODING_STYLE, 1.1 Multiline Indent
> 
>> +{
> when describing filds from spec try to add 'verbatim' copy of the name from spec
> so it would be esy to grep for it. Like:
>        /* Block Status */
>> +    build_append_int_noprefix(table, block_status, 4);
>        /* Raw Data Offset */
> 
> note applies all other places where you compose ACPI tables
ok

> 
>> +    build_append_int_noprefix(table, raw_data_offset, 4);
>> +    build_append_int_noprefix(table, raw_data_length, 4);
>> +    build_append_int_noprefix(table, data_length, 4);
>> +    build_append_int_noprefix(table, error_severity, 4);
>> +}
>> +
>>  /* Generic Error Data Entry
>>   * ACPI 4.0: 17.3.2.6.1 Generic Error Data
>>   */
>> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
>> index a71db2f..1ec7e1b 100644
>> --- a/include/hw/acpi/aml-build.h
>> +++ b/include/hw/acpi/aml-build.h
>> @@ -425,6 +425,12 @@ void build_append_ghes_generic_data(GArray *table, const char *section_type,
>>                                      uint32_t error_data_length, uint8_t *fru_id,
>>                                      uint8_t *fru_text, uint64_t time_stamp);
>>  
>> +void
>> +build_append_ghes_generic_status(GArray *table, uint32_t block_status,
>> +                                 uint32_t raw_data_offset,
>> +                                 uint32_t raw_data_length,
>> +                                 uint32_t data_length, uint32_t error_severity);
> this and previous patch, it might be better to to move declaration
> to its own header, for example to include/hw/acpi/acpi_ghes.h
> that you are adding later in the series.
> And maybe move helpers to hw/acpi/acpi_ghes.c
> They are not really independent ACPI primitives that are shared
> with other tables, aren't they?
Some ACPI primitives are shared with other table, such as Notification Structure.
we have 10 types of error sources, some error source will share the  Notification Structure primitives.
Now I only implement Generic Hardware Error Source version 2 (GHESv2 - Type 10)

> .
>> +
>>  void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
>>                         uint64_t len, int node, MemoryAffinityFlags flags);
>>  
> 
> .
>