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

[Eric Auger]

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.
>> 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

Thanks

Eric
>> 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
>>
>> .
>