Re: [RFC PATCH 4/5] hw/acpi/aml-build: add processor hierarchy node structure

[Ying Fang <fangying1@huawei.com>]

On 3/1/2021 11:50 PM, Michael S. Tsirkin wrote:
> On Mon, Mar 01, 2021 at 10:39:19AM +0100, Andrew Jones wrote:
>> On Fri, Feb 26, 2021 at 10:23:03AM +0800, Ying Fang wrote:
>>>
>>>
>>> On 2/25/2021 7:47 PM, Andrew Jones wrote:
>>>> On Thu, Feb 25, 2021 at 04:56:26PM +0800, Ying Fang wrote:
>>>>> Add the processor hierarchy node structures to build ACPI information
>>>>> for CPU topology. Since the private resources may be used to describe
>>>>> cache hierarchy and it is variable among different topology level,
>>>>> three helpers are introduced to describe the hierarchy.
>>>>>
>>>>> (1) build_socket_hierarchy for socket description
>>>>> (2) build_processor_hierarchy for processor description
>>>>> (3) build_smt_hierarchy for thread (logic processor) description
>>>>>
>>>>> Signed-off-by: Ying Fang <fangying1@huawei.com>
>>>>> Signed-off-by: Henglong Fan <fanhenglong@huawei.com>
>>>>> ---
>>>>>    hw/acpi/aml-build.c         | 40 +++++++++++++++++++++++++++++++++++++
>>>>>    include/hw/acpi/acpi-defs.h | 13 ++++++++++++
>>>>>    include/hw/acpi/aml-build.h |  7 +++++++
>>>>>    3 files changed, 60 insertions(+)
>>>>>
>>>>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>>>>> index a2cd7a5830..a0af3e9d73 100644
>>>>> --- a/hw/acpi/aml-build.c
>>>>> +++ b/hw/acpi/aml-build.c
>>>>> @@ -1888,6 +1888,46 @@ void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
>>>>>                     table_data->len - slit_start, 1, oem_id, oem_table_id);
>>>>>    }
>>>>> +/*
>>>>> + * ACPI 6.3: 5.2.29.1 Processor hierarchy node structure (Type 0)
>>>>> + */
>>>>> +void build_socket_hierarchy(GArray *tbl, uint32_t parent, uint32_t id)
>>>>> +{
>>>>> +    build_append_byte(tbl, ACPI_PPTT_TYPE_PROCESSOR); /* Type 0 - processor */
>>>>> +    build_append_byte(tbl, 20);         /* Length, no private resources */
>>>>> +    build_append_int_noprefix(tbl, 0, 2);  /* Reserved */
>>>>> +    build_append_int_noprefix(tbl, ACPI_PPTT_PHYSICAL_PACKAGE, 4);
>>>>
>>>> Missing '/* Flags */'
>>>
>>> Will fix.
>>>
>>>>
>>>>> +    build_append_int_noprefix(tbl, parent, 4); /* Parent */
>>>>> +    build_append_int_noprefix(tbl, id, 4);     /* ACPI processor ID */
>>>>> +    build_append_int_noprefix(tbl, 0, 4);  /* Number of private resources */
>>>>> +}
>>>>> +
>>>>> +void build_processor_hierarchy(GArray *tbl, uint32_t flags,
>>>>> +                               uint32_t parent, uint32_t id)
>>>>> +{
>>>>> +    build_append_byte(tbl, ACPI_PPTT_TYPE_PROCESSOR);  /* Type 0 - processor */
>>>>> +    build_append_byte(tbl, 20);         /* Length, no private resources */
>>>>> +    build_append_int_noprefix(tbl, 0, 2);      /* Reserved */
>>>>> +    build_append_int_noprefix(tbl, flags, 4);  /* Flags */
>>>>> +    build_append_int_noprefix(tbl, parent, 4); /* Parent */
>>>>> +    build_append_int_noprefix(tbl, id, 4);     /* ACPI processor ID */
>>>>> +    build_append_int_noprefix(tbl, 0, 4);  /* Number of private resources */
>>>>> +}
>>>>> +
>>>>> +void build_thread_hierarchy(GArray *tbl, uint32_t parent, uint32_t id)
>>>>> +{
>>>>> +    build_append_byte(tbl, ACPI_PPTT_TYPE_PROCESSOR); /* Type 0 - processor */
>>>>> +    build_append_byte(tbl, 20);           /* Length, no private resources */
>>>>> +    build_append_int_noprefix(tbl, 0, 2); /* Reserved */
>>>>> +    build_append_int_noprefix(tbl,
>>>>> +                              ACPI_PPTT_ACPI_PROCESSOR_ID_VALID |
>>>>> +                              ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD |
>>>>> +                              ACPI_PPTT_ACPI_LEAF_NODE, 4);  /* Flags */
>>>>> +    build_append_int_noprefix(tbl, parent , 4); /* parent */
>>>>
>>>> 'parent' not capitalized. We want these comments to exactly match the text
>>>> in the spec.
>>>
>>> Will fix.
>>>
>>>>
>>>>> +    build_append_int_noprefix(tbl, id, 4);      /* ACPI processor ID */
>>>>> +    build_append_int_noprefix(tbl, 0, 4);       /* Num of private resources */
>>>>> +}
>>>>> +
>>>>>    /* build rev1/rev3/rev5.1 FADT */
>>>>>    void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
>>>>>                    const char *oem_id, const char *oem_table_id)
>>>>> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
>>>>> index cf9f44299c..45e10d886f 100644
>>>>> --- a/include/hw/acpi/acpi-defs.h
>>>>> +++ b/include/hw/acpi/acpi-defs.h
>>>>> @@ -618,4 +618,17 @@ struct AcpiIortRC {
>>>>>    } QEMU_PACKED;
>>>>>    typedef struct AcpiIortRC AcpiIortRC;
>>>>> +enum {
>>>>> +    ACPI_PPTT_TYPE_PROCESSOR = 0,
>>>>> +    ACPI_PPTT_TYPE_CACHE,
>>>>> +    ACPI_PPTT_TYPE_ID,
>>>>> +    ACPI_PPTT_TYPE_RESERVED
>>>>> +};
>>>>> +
>>>>> +#define ACPI_PPTT_PHYSICAL_PACKAGE          (1)
>>>>> +#define ACPI_PPTT_ACPI_PROCESSOR_ID_VALID   (1 << 1)
>>>>> +#define ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD  (1 << 2)      /* ACPI 6.3 */
>>>>> +#define ACPI_PPTT_ACPI_LEAF_NODE            (1 << 3)      /* ACPI 6.3 */
>>>>> +#define ACPI_PPTT_ACPI_IDENTICAL            (1 << 4)      /* ACPI 6.3 */
> 
> You need to quote specific place in spec where this appeared, not
> just version. and what about previous ones?

Thanks, Will fix.

> 
> 
>>>>> +
>>>>>    #endif
>>>>> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
>>>>> index 380d3e3924..7f0ca1a198 100644
>>>>> --- a/include/hw/acpi/aml-build.h
>>>>> +++ b/include/hw/acpi/aml-build.h
>>>>> @@ -462,6 +462,13 @@ void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
>>>>>    void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
>>>>>                    const char *oem_id, const char *oem_table_id);
>>>>> +void build_socket_hierarchy(GArray *tbl, uint32_t parent, uint32_t id);
>>>>> +
>>>>> +void build_processor_hierarchy(GArray *tbl, uint32_t flags,
>>>>> +                               uint32_t parent, uint32_t id);
>>>>> +
>>>>> +void build_thread_hierarchy(GArray *tbl, uint32_t parent, uint32_t id);
>>>>
>>>> Why does build_processor_hierarchy() take a flags argument, but the
>>>> others don't? Why not just have a single 'flags' taking function,
>>>> like [*] that works for all of them? I think that answer to that is
>>>
>>> Yes, you are right.
>>>
>>>> that when cache topology support is added it's better to break these
>>>> into separate functions, but should we do that now? It seems odd to
>>>> be introducing unused defines and this API before it's necessary.
>>> So it is better for us to keep just one common build_processor_hierarchy
>>> API here in your opinion.
>>
>> Well, a consistent API without unused defines. Whether or not that's
>> a single common function or not isn't that important.
>>
>> Thanks,
>> drew
> 
> Yes, the preferred way is code comments:
> E.g.
> 
>      build_append_byte(tbl, ACPI_PPTT_TYPE_PROCESSOR);  /* Type 0 - processor */
> 
> should be
> 
>      build_append_byte(tbl, 0);  /* Type 0 - processor */
> 
> 
> similar:
> 
>>>>> +    build_append_int_noprefix(tbl,
>>>>> +                              ACPI_PPTT_ACPI_PROCESSOR_ID_VALID |
>>>>> +                              ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD |
>>>>> +                              ACPI_PPTT_ACPI_LEAF_NODE, 4);  /* Flags */
> 
> should be
> 
>   +    build_append_int_noprefix(tbl, /* Processor Structure Flags */
>   +                              (1 << 1)  /* ACPI Processor ID valid */|
>   +                              (1 << 2) /* Processor is a Thread */) |
>   +                              (1 << 3) /* Node is a Leaf */, 4);
> 
> where you would make sure the text matches the spec verbatim.
> 
> also note how for multi-line code comments precede the code.
> For single-line they can come after the code.

Thanks, will fix it as your suggestions.

> 
>>>
>>>>
>>>> [*] https://github.com/rhdrjones/qemu/commit/439b38d67ca1f2cbfa5b9892a822b651ebd05c11
>>>>
>>>> Thanks,
>>>> drew
>>>>
>>>>> +
>>>>>    void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
>>>>>                    const char *oem_id, const char *oem_table_id);
>>>>> -- 
>>>>> 2.23.0
>>>>>
>>>>>
>>>>
>>>> .
>>>>
>>>
>>> Thanks,
>>> Ying.
>>>
> 
> .
>