Hi,
On 10/20/21 2:27 PM, wangyanan (Y) wrote:
> 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 <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.
> 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().
That's not my understanding. I think we usually point to the latest spec
revision you comply with, ie. in that case 6.3 chapter ...
Thanks
Eric
>>>> 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
>>>>
>>>> .
>> .
>
