Re: About PPTT find_acpi_cpu_topology_package()

[Sudeep Holla <sudeep.holla@arm.com>]

On Wed, Feb 12, 2020 at 11:20:12AM +0000, John Garry wrote:
> Hi Jeremy,
>
> I have a question about $subject for you, since you wrote the code.
>
> This function returns a unique identifier for the package, but would not be
> the logically indexed package id we would expect, like 0, 1, 2, ...
>

Firstly, it must be physical socket number and not logical id.

> It returns of the offset in the PPTT of the topology physical CPU node.
>

Yes, intentionally. We don't want to generate a logical index for this.
Simply not going to happen as we can't guarantee unique number always.
We need to get that uniqueness from the firmware and hence the choice of
offset. Remember that the offset is used only if firmware conveniently
ignored all the optional properties including UID in the processor
container representing the physical socket.

> So I may get something like this:
>
> john@ubuntu:~$ more
> /sys/devices/system/cpu/cpu80/topology/physical_package_id
> 5418
>

Good, now the platform have a reason to fix it in the firmware if it is
very hard to see and understand the above value.

> For sure, this does not violate the ABI in
> Documentation/ABI/testing/sysfs-devices-system-cpu:
>

Very good to see you are not disagreeing with that :)

> "physical_package_id: physical package id of cpu#. Typically	 corresponds to
> a physical socket number, but the actual value is architecture and platform
> dependent."
>
> Question: Is there any reason for which we cannot assign an indexed package
> id to each package node?
>

Yes, as mentioned above. We are not going to do extra work for lazy firmware.
Linux also will be lazy on such platform and provide weird unique numbers
like in the above case you have mentioned.

> Some userspace tools rely on a sane meaningful package id, like perf:
>

Good that you mention now. Time to update the firmware then.


[...]

>
> This can only deal with a socket id which fits in a byte. I'd rather not
> change this code if possible.
>

Agreed, add UID to the processor container, job done.

--
Regards,
Sudeep