Re: [PATCH v8 5/8] hw/acpi/aml-build: Add PPTT table

["wangyanan (Y)" <wangyanan55@huawei.com>]

Hi Eric,

On 2021/10/20 19:11, Eric Auger wrote:
> Hi Yanan,
> On 10/20/21 11:51 AM, wangyanan (Y) wrote:
>> Hi Eric,
>>
>> On 2021/10/20 16:02, Eric Auger wrote:
>>> Hi,
>>>
>>> On 10/14/21 3:22 PM, Yanan Wang wrote:
>>>> From: Andrew Jones <drjones@redhat.com>
>>>>
>>>> Add the Processor Properties Topology Table (PPTT) used to
>>>> describe CPU topology information to ACPI guests.
>>>>
>>>> Note, a DT-boot Linux guest with a non-flat CPU topology will
>>>> see socket and core IDs being sequential integers starting
>>>> from zero, which is different from ACPI-boot Linux guest,
>>>> e.g. with -smp 4,sockets=2,cores=2,threads=1
>>>>
>>>> a DT boot produces:
>>>>
>>>>    cpu:  0 package_id:  0 core_id:  0
>>>>    cpu:  1 package_id:  0 core_id:  1
>>>>    cpu:  2 package_id:  1 core_id:  0
>>>>    cpu:  3 package_id:  1 core_id:  1
>>>>
>>>> an ACPI boot produces:
>>>>
>>>>    cpu:  0 package_id: 36 core_id:  0
>>>>    cpu:  1 package_id: 36 core_id:  1
>>>>    cpu:  2 package_id: 96 core_id:  2
>>>>    cpu:  3 package_id: 96 core_id:  3
>>>>
>>>> This is due to several reasons:
>>>>
>>>>    1) DT cpu nodes do not have an equivalent field to what the PPTT
>>>>       ACPI Processor ID must be, i.e. something equal to the MADT CPU
>>>>       UID or equal to the UID of an ACPI processor container. In both
>>>>       ACPI cases those are platform dependant IDs assigned by the
>>>>       vendor.
>>>>
>>>>    2) While QEMU is the vendor for a guest, if the topology specifies
>>>>       SMT (> 1 thread), then, with ACPI, it is impossible to assign a
>>>>       core-id the same value as a package-id, thus it is not possible
>>>>       to have package-id=0 and core-id=0. This is because package and
>>>>       core containers must be in the same ACPI namespace and therefore
>>>>       must have unique UIDs.
>>>>
>>>>    3) ACPI processor containers are not mandatorily required for PPTT
>>>>       tables to be used and, due to the limitations of which IDs are
>>>>       selected described above in (2), they are not helpful for QEMU,
>>>>       so we don't build them with this patch. In the absence of them,
>>>>       Linux assigns its own unique IDs. The maintainers have chosen not
>>>>       to use counters from zero, but rather ACPI table offsets, which
>>>>       explains why the numbers are so much larger than with DT.
>>>>
>>>>    4) When there is no SMT (threads=1) the core IDs for ACPI boot guests
>>>>       match the logical CPU IDs, because these IDs must be equal to the
>>>>       MADT CPU UID (as no processor containers are present), and QEMU
>>>>       uses the logical CPU ID for these MADT IDs.
>>>>
>>>> So in summary, with QEMU as the vendor for the guests, we simply
>>>> use sequential integers starting from zero for the non-leaf nodes
>>>> but with ID-valid flag unset, so that guest will ignore them and
>>>> use table offsets as unique container IDs. And we use logical CPU
>>>> IDs for the leaf nodes with the ID-valid flag set, which will be
>>>> consistent with MADT.
>>>>
>>>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>>>> Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
>>>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>>> ---
>>>>    hw/acpi/aml-build.c         | 60 +++++++++++++++++++++++++++++++++++++
>>>>    include/hw/acpi/aml-build.h |  3 ++
>>>>    2 files changed, 63 insertions(+)
>>>>
>>>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>>>> index b7b9db6888..0d50e88e9d 100644
>>>> --- a/hw/acpi/aml-build.c
>>>> +++ b/hw/acpi/aml-build.c
>>>> @@ -1990,6 +1990,66 @@ void build_processor_hierarchy_node(GArray
>>>> *tbl, uint32_t flags,
>>>>        }
>>>>    }
>>>>    +/* ACPI 6.2: 5.2.29 Processor Properties Topology Table (PPTT) */
>>>> +void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState
>>>> *ms,
>>>> +                const char *oem_id, const char *oem_table_id)
>>>> +{
>>>> +    int pptt_start = table_data->len;
>>>> +    int uid = 0;
>>>> +    int socket;
>>>> +    AcpiTable table = { .sig = "PPTT", .rev = 2,
>>>> +                        .oem_id = oem_id, .oem_table_id =
>>>> oem_table_id };
>>> Table 5-149 of 6.2 spec (6.2 May 2017) tells the rev shall be 1. Or is
>>> it an erratum somewhere I did miss?
>> Yes, the revision in 6.2 spec is 1. And it's 2 in spec 6.3.
>> So just to be sure, should I use the oldest revision ?
> If you need (and use) features (such as flags) introduced in 6.3 then
> you should say the code complies with 6.3 and update the above comment.
The comment /* ACPI 6.2: 5.2.29 Processor Properties Topology Table 
(PPTT) */
tried to explain the first spec version where PPTT is introduced is 6.2. 
But it may
adds some confusion. I think it's better to replace it with:
/*
  * ACPI spec 5.2.29 Processor Properties Topology Table (PPTT)
  * (Revision 6.2 or later)
  */

just like our build_slit().
>>> I would also add the spec version in the commit msg.
>>>> +
>>>> +    acpi_table_begin(&table, table_data);
>>>> +
>>>> +    for (socket = 0; socket < ms->smp.sockets; socket++) {
>>>> +        uint32_t socket_offset = table_data->len - pptt_start;
>>>> +        int core;
>>>> +
>>>> +        build_processor_hierarchy_node(
>>>> +            table_data,
>>>> +            /*
>>>> +             * ACPI 6.2 - Physical package
>>>> +             * represents the boundary of a physical package
>>>> +             */
>>>> +            (1 << 0),
>>>> +            0, socket, NULL, 0);
>>> I see we set an ACPI process ID but in the meantime the ACPI processor
>>> ID valid flag is not set. I am not sure I fully catch the meaning of
>>> this latter but just to double check if this is done on purpose.
>> Yes, it's on purpose.
>>> Maybe
>>> wort a general comment as this also happens below.
>> The ID of the container node is invalid and ID of the leaf node is valid.
>> The commit message by Andrew has explained why (reason 3). I think
>> it may be clear enough to explain there why we don't need a valid ID
>> for the container node.
>>>> +
>>>> +        for (core = 0; core < ms->smp.cores; core++) {
>>>> +            uint32_t core_offset = table_data->len - pptt_start;
>>>> +            int thread;
>>>> +
>>>> +            if (ms->smp.threads > 1) {
>>>> +                build_processor_hierarchy_node(
>>>> +                    table_data,
>>>> +                    /*
>>>> +                     * ACPI 6.2 - Physical package
>>>> +                     * doesn't represent the boundary of a physical
>>>> package
>>>> +                     */
>>>> +                    (0 << 0),
>>> would rather say (0 << 0) /* not a physical package */ and same elsewhere
>> Ok, thanks.
>>>> +                    socket_offset, core, NULL, 0);
>>>> +
>>>> +                for (thread = 0; thread < ms->smp.threads; thread++) {
>>>> +                    build_processor_hierarchy_node(
>>>> +                        table_data,
>>>> +                        (1 << 1) | /* ACPI 6.2 - ACPI Processor ID
>>>> valid */
>>>> +                        (1 << 2) | /* ACPI 6.3 - Processor is a
>>>> Thread */
>>> So the references look globaly confusing to me. Either it complies to
>>> 6.2 or to 6.3. Looks ir rather complies with 6.3. To me, this needs to
>>> be clarified.
>> ACPI 6.2 in the comment means the flag is introduced in the spec since 6.2.
>> The same, ACPI 6.3 means the flag is introduced since 6.3. Maybe I should
>> just drop all the version-prefix in the comment ?
> Yes I think you can drop those comments and just upgrade the global
> compliance with 6.3
>
I will drop the prefix and keep the rest. And add a generic comment on
top of build_pptt() as I replied above.

Thanks,
Yanan
>>> I would also add the reference it complies to in the
>>> commit msg.
>> Ok, sure.
>>
>> Thanks,
>> Yanan
>> .
>>>> +                        (1 << 3),  /* ACPI 6.3 - Node is a Leaf */
>>>> +                        core_offset, uid++, NULL, 0);
>>>> +                }
>>>> +            } else {
>>>> +                build_processor_hierarchy_node(
>>>> +                    table_data,
>>>> +                    (1 << 1) | /* ACPI 6.2 - ACPI Processor ID valid */
>>>> +                    (1 << 3),  /* ACPI 6.3 - Node is a Leaf */
>>>> +                    socket_offset, uid++, NULL, 0);
>>>> +            }
>>>> +        }
>>>> +    }
>>>> +
>>>> +    acpi_table_end(linker, &table);
>>>> +}
>>>> +
>>>>    /* 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/aml-build.h b/include/hw/acpi/aml-build.h
>>>> index 2c457c8f17..b92706388c 100644
>>>> --- a/include/hw/acpi/aml-build.h
>>>> +++ b/include/hw/acpi/aml-build.h
>>>> @@ -493,6 +493,9 @@ void build_processor_hierarchy_node(GArray *tbl,
>>>> uint32_t flags,
>>>>                                        uint32_t parent, uint32_t id,
>>>>                                        uint32_t *priv_rsrc, uint32_t
>>>> priv_num);
>>>>    +void build_pptt(GArray *table_data, BIOSLinker *linker,
>>>> MachineState *ms,
>>>> +                const char *oem_id, const char *oem_table_id);
>>>> +
>>>>    void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData
>>>> *f,
>>>>                    const char *oem_id, const char *oem_table_id);
>>>>   
>>> Thanks
>>>
>>> Eric
>>>
>>> .
> .