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 <drjo...@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 <drjo...@redhat.com>
>>> Co-developed-by: Yanan Wang <wangyana...@huawei.com>
>>> Signed-off-by: Yanan Wang <wangyana...@huawei.com>
>>> Reviewed-by: Michael S. Tsirkin <m...@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
>>
>> .
>
