Re: [PATCH] ACPI/PPTT: use ACPI ID whenever ACPI_PPTT_ACPI_PROCESSOR_ID_VALID is set

[Sudeep Holla]

On 29/06/18 19:18, Jeremy Linton wrote:
> Hi,
> 
> On 06/29/2018 11:17 AM, Sudeep Holla wrote:
>> Currently we use the ACPI processor ID only for the leaf/processor nodes
>> as the specification states it must match the value of ACPI processor ID
>> field in the processorâs entry in the MADT.
>>
>> However, if a PPTT structure represents processors group, it match a
>> processor container UID in the namespace and
>> ACPI_PPTT_ACPI_PROCESSOR_ID_VALID
>> flag describe whether the ACPI processor ID is valid.
>>
>> Lets use UID whenever ACPI_PPTT_ACPI_PROCESSOR_ID_VALID is set to be
>> consistent instead of using table offset as it's currently done for non
>> leaf nodes.
>>
>> Cc: "Rafael J. Wysocki" <rjw@xxxxxxxxxxxxx>
>> Signed-off-by: Sudeep Holla <sudeep.holla@xxxxxxx>
>> ---
>> Â drivers/acpi/pptt.c | 10 ++++++++--
>> Â 1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> Hi,
>>
>> There's ongoing discussion on assigning ID based in OS using simple
>> counters. It can never be consistent with firmware's view. So if the
>> firmware provides valid UID for non-processors node, we must use it.
>>
>> Regards,
>> Sudeep
>>
>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
>> index e5ea1974d1e3..d1e26cb599bf 100644
>> --- a/drivers/acpi/pptt.c
>> +++ b/drivers/acpi/pptt.c
>> @@ -481,8 +481,14 @@ static int topology_get_acpi_cpu_tag(struct
>> acpi_table_header *table,
>> ÂÂÂÂÂ if (cpu_node) {
>> ÂÂÂÂÂÂÂÂÂ cpu_node = acpi_find_processor_package_id(table, cpu_node,
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ level, flag);
>> -ÂÂÂÂÂÂÂ /* Only the first level has a guaranteed id */
>> -ÂÂÂÂÂÂÂ if (level == 0)
>> +ÂÂÂÂÂÂÂ /*
>> +ÂÂÂÂÂÂÂÂ * As per specification if the processor structure represents
>> +ÂÂÂÂÂÂÂÂ * an actual processor, then ACPI processor ID must be valid.
>> +ÂÂÂÂÂÂÂÂ * For processor containers ACPI_PPTT_ACPI_PROCESSOR_ID_VALID
>> +ÂÂÂÂÂÂÂÂ * should be set if the UID is valid
>> +ÂÂÂÂÂÂÂÂ */
>> +ÂÂÂÂÂÂÂ if (level == 0 ||
>> +ÂÂÂÂÂÂÂÂÂÂÂ cpu_node->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID)
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ return cpu_node->acpi_processor_id;
> 
> While, for some machines this likely helps create more human readable
> ID's... What happens when the ID namespaces conflict with each other?
> 

That's entirely left to the platform firmware. It should help userspace
to identify the topology in a way firmware is describing and no more
than that. If users use them for anything more, it's at their own risk.

> AKA, I'm a little shy of this change because your going from something
> we can guarantee is unique to depending on an portion of the PPTT
> definition that has a couple different ways that it can be interpreted.
> 

No, I am not guaranteeing anything here. I am just passing valid UID if
present to the caller. Interpretation is left to the caller and in ARM64
we should just use(at least my preference) the value as is for sysfs
topology.

> OTOH the change is probably safe at the moment because i don't think
> anyone has partially marked nodes at a given PPTT "level" valid, or put
> structures that aren't part of the PE/cache's in the tree (outside of my
> juno test tree with the GPU's/etc).
> 

Even if they are present, I don't see issue. If that's how firmware
presents the CPU topology, that should be exactly the way we too need to.

-- 
Regards,
Sudeep