About PPTT find_acpi_cpu_topology_package()

About PPTT find_acpi_cpu_topology_package()

[John Garry]

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

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

So I may get something like this:

john@ubuntu:~$ more 
/sys/devices/system/cpu/cpu80/topology/physical_package_id
5418

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

"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?

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

See tools/perf/util/cpumap.c:

int cpu_map__get_die(struct perf_cpu_map *map, int idx, void *data)
{
...

	s = cpu_map__get_socket(map, idx, data);
	if (s == -1)
		return -1;

	/*
	 * Encode socket in bit range 15:8
	 * die_id is relative to socket, and
	 * we need a global id. So we combine
	 * socket + die id
	 */
	if (WARN_ONCE(die_id >> 8, "The die id number is too big.\n"))
		return -1;

...

	return (s << 8) | (die_id & 0xff);
}

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

Thanks,
John

Re: About PPTT find_acpi_cpu_topology_package()

[Sudeep Holla]

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

Re: About PPTT find_acpi_cpu_topology_package()

[John Garry]

On 12/02/2020 11:59, Sudeep Holla wrote:
> On Wed, Feb 12, 2020 at 11:20:12AM +0000, John Garry wrote:
>> Hi Jeremy,
>>

Hi Sudeep,


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

That's really what I meant.

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

I don't think it's reasonable to just label this as lazy. The table may 
just not have the flag set unintentionally. FW and software guys make 
mistakes, like the mistakes in PPTT, itself.

> Linux also will be lazy on such platform and provide weird unique numbers
> like in the above case you have mentioned.

Personally I think that the kernel can be do better than provide 
meaningless values like this, since it knows the processor IDs and which 
physical package they belong to.

If not, at least make the user know of potential deficiencies in the table.

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

Thanks,
John

Re: About PPTT find_acpi_cpu_topology_package()

[Sudeep Holla]

On Wed, Feb 12, 2020 at 12:48:33PM +0000, John Garry wrote:
> On 12/02/2020 11:59, Sudeep Holla wrote:

[...]

> > Yes, as mentioned above. We are not going to do extra work for lazy firmware.
>
> I don't think it's reasonable to just label this as lazy. The table may just
> not have the flag set unintentionally. FW and software guys make mistakes,
> like the mistakes in PPTT, itself.
>

We are not talking about flags, it's UID and it is pretty important if
there are more than one objects of same time.

> > Linux also will be lazy on such platform and provide weird unique numbers
> > like in the above case you have mentioned.
>
> Personally I think that the kernel can be do better than provide meaningless
> values like this, since it knows the processor IDs and which physical
> package they belong to.
>

This was discussed quite a lot, I can dig and point you to it. That's the
reason for choosing offset. We are *not going back* to this again. Fix the
firmware before it gets copied for all future platforms and Linux has to
deal with that *forever*.

> If not, at least make the user know of potential deficiencies in the table.
>

How ? What are your suggestions ? Does adding a warning or note that UID
is missing and offset is chosen help ? I am kind of fine with that.

--
Regards,
Sudeep

Re: About PPTT find_acpi_cpu_topology_package()

[Jeremy Linton]

Hi,

On 2/12/20 7:55 AM, Sudeep Holla wrote:
> On Wed, Feb 12, 2020 at 12:48:33PM +0000, John Garry wrote:
>> On 12/02/2020 11:59, Sudeep Holla wrote:
> 
> [...]
> 
>>> Yes, as mentioned above. We are not going to do extra work for lazy firmware.
>>
>> I don't think it's reasonable to just label this as lazy. The table may just
>> not have the flag set unintentionally. FW and software guys make mistakes,
>> like the mistakes in PPTT, itself.
>>
> 
> We are not talking about flags, it's UID and it is pretty important if
> there are more than one objects of same time.

But, this hints at my reservations with this approach. If you wanted to 
have your processors numbered 0...x and your sockets numbered 0...y, 
there could be overlap in the processor container objects, which should 
also be avoided.

> 
>>> Linux also will be lazy on such platform and provide weird unique numbers
>>> like in the above case you have mentioned.
>>
>> Personally I think that the kernel can be do better than provide meaningless
>> values like this, since it knows the processor IDs and which physical
>> package they belong to.
>>
> 
> This was discussed quite a lot, I can dig and point you to it. That's the
> reason for choosing offset. We are *not going back* to this again. Fix the
> firmware before it gets copied for all future platforms and Linux has to
> deal with that *forever*.
> 
>> If not, at least make the user know of potential deficiencies in the table.
>>
> 
> How ? What are your suggestions ? Does adding a warning or note that UID
> is missing and offset is chosen help ? I am kind of fine with that.
> 
> --
> Regards,
> Sudeep
> 

Re: About PPTT find_acpi_cpu_topology_package()

[Sudeep Holla]

On Tue, Feb 11, 2020 at 12:49:17PM -0600, Jeremy Linton wrote:
> Hi,
> 
> On 2/12/20 7:55 AM, Sudeep Holla wrote:
> > On Wed, Feb 12, 2020 at 12:48:33PM +0000, John Garry wrote:
> > > On 12/02/2020 11:59, Sudeep Holla wrote:
> > 
> > [...]
> > 
> > > > Yes, as mentioned above. We are not going to do extra work for lazy firmware.
> > > 
> > > I don't think it's reasonable to just label this as lazy. The table may just
> > > not have the flag set unintentionally. FW and software guys make mistakes,
> > > like the mistakes in PPTT, itself.
> > > 
> > 
> > We are not talking about flags, it's UID and it is pretty important if
> > there are more than one objects of same time.
> 
> But, this hints at my reservations with this approach. If you wanted to have
> your processors numbered 0...x and your sockets numbered 0...y, there could
> be overlap in the processor container objects, which should also be avoided.
> 

Of course yes, UID needs to be unique at a given processor/container level.
Yes, it's more restricted in that way compared to objects of similar type.
Here they are all same processor containers, but need to enumerate from 0
at each level.

-- 
Regards,
Sudeep

Re: About PPTT find_acpi_cpu_topology_package()

[John Garry]

On 12/02/2020 13:55, Sudeep Holla wrote:
> On Wed, Feb 12, 2020 at 12:48:33PM +0000, John Garry wrote:
>> On 12/02/2020 11:59, Sudeep Holla wrote:
> 
> [...]
> 

Hi Sudeep,

>>> Yes, as mentioned above. We are not going to do extra work for lazy firmware.
>>
>> I don't think it's reasonable to just label this as lazy. The table may just
>> not have the flag set unintentionally. FW and software guys make mistakes,
>> like the mistakes in PPTT, itself.
>>
> 
> We are not talking about flags, it's UID and it is pretty important if
> there are more than one objects of same time.
> 

I am talking about the Processor ID valid flag, which is specifically 
related.

>>> Linux also will be lazy on such platform and provide weird unique numbers
>>> like in the above case you have mentioned.
>>
>> Personally I think that the kernel can be do better than provide meaningless
>> values like this, since it knows the processor IDs and which physical
>> package they belong to.
>>
> 
> This was discussed quite a lot, I can dig and point you to it. That's the
> reason for choosing offset. We are *not going back* to this again. Fix the
> firmware before it gets copied for all future platforms and Linux has to
> deal with that *forever*.

I would liked to have been made aware earlier of the oversight. Quite 
often we only find problems when someone or something complains.

It is a strange API to provide offsets like this, and I did not realize 
that they were actually being exposed to userspace.

> 
>> If not, at least make the user know of potential deficiencies in the table.
>>
> 
> How ? What are your suggestions ? Does adding a warning or note that UID
> is missing and offset is chosen help ? 

I'd say so. I know now, but let's save others the potential hassle. And 
having this debate again.

I am kind of fine with that.

How about something like this:

--- a/drivers/acpi/pptt.c
+++ b/drivers/acpi/pptt.c
@@ -515,6 +515,8 @@ static int topology_get_acpi_cpu_tag(struct 
acpi_table_header *table,
   if (level == 0 || cpu_node->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID)
	return cpu_node->acpi_processor_id;
+   if (level == PPTT_ABORT_PACKAGE)
+	pr_warn_once("ACPI Processor ID valid not set for physical package 
node, will use node table offset as substitute for UID\n");
                 return ACPI_PTR_DIFF(cpu_node, table);
         }
         pr_warn_once("PPTT table found, but unable to locate core %d 
(%d)\n",

Thanks,
John

Re: About PPTT find_acpi_cpu_topology_package()

[Jeremy Linton]

Hi,

On 2/12/20 8:41 AM, John Garry wrote:
> On 12/02/2020 13:55, Sudeep Holla wrote:
>> On Wed, Feb 12, 2020 at 12:48:33PM +0000, John Garry wrote:
>>> On 12/02/2020 11:59, Sudeep Holla wrote:
>>
>> [...]
>>
> 
> Hi Sudeep,
> 
>>>> Yes, as mentioned above. We are not going to do extra work for lazy 
>>>> firmware.
>>>
>>> I don't think it's reasonable to just label this as lazy. The table 
>>> may just
>>> not have the flag set unintentionally. FW and software guys make 
>>> mistakes,
>>> like the mistakes in PPTT, itself.
>>>
>>
>> We are not talking about flags, it's UID and it is pretty important if
>> there are more than one objects of same time.
>>
> 
> I am talking about the Processor ID valid flag, which is specifically 
> related.
> 
>>>> Linux also will be lazy on such platform and provide weird unique 
>>>> numbers
>>>> like in the above case you have mentioned.
>>>
>>> Personally I think that the kernel can be do better than provide 
>>> meaningless
>>> values like this, since it knows the processor IDs and which physical
>>> package they belong to.
>>>
>>
>> This was discussed quite a lot, I can dig and point you to it. That's the
>> reason for choosing offset. We are *not going back* to this again. Fix 
>> the
>> firmware before it gets copied for all future platforms and Linux has to
>> deal with that *forever*.
> 
> I would liked to have been made aware earlier of the oversight. Quite 
> often we only find problems when someone or something complains.
> 
> It is a strange API to provide offsets like this, and I did not realize 
> that they were actually being exposed to userspace.
> 
>>
>>> If not, at least make the user know of potential deficiencies in the 
>>> table.
>>>
>>
>> How ? What are your suggestions ? Does adding a warning or note that UID
>> is missing and offset is chosen help ? 
> 
> I'd say so. I know now, but let's save others the potential hassle. And 
> having this debate again.
> 
> I am kind of fine with that.
> 
> How about something like this:
> 
> --- a/drivers/acpi/pptt.c
> +++ b/drivers/acpi/pptt.c
> @@ -515,6 +515,8 @@ static int topology_get_acpi_cpu_tag(struct 
> acpi_table_header *table,
>    if (level == 0 || cpu_node->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID)
>      return cpu_node->acpi_processor_id;
> +   if (level == PPTT_ABORT_PACKAGE)
> +    pr_warn_once("ACPI Processor ID valid not set for physical package 
> node, will use node table offset as substitute for UID\n");

Level will probably never be PPTT_ABORT_PACKAGE, so.. you probably have 
to have the warning higher up when it terminates the package search.

OTOH, just because this is set doesn't mean you get "nice" ids. I've had 
complaints, because there is a firmware floating around which uses the 
MPIDR value as the processor_id, so the cores come out with really weird 
numbers too.



>                  return ACPI_PTR_DIFF(cpu_node, table);
>          }
>          pr_warn_once("PPTT table found, but unable to locate core %d 
> (%d)\n",
> 
> Thanks,
> John

Re: About PPTT find_acpi_cpu_topology_package()

[John Garry]

>>>
>>>> If not, at least make the user know of potential deficiencies in the 
>>>> table.
>>>>
>>>
>>> How ? What are your suggestions ? Does adding a warning or note that UID
>>> is missing and offset is chosen help ? 
>>
>> I'd say so. I know now, but let's save others the potential hassle. 
>> And having this debate again.
>>
>> I am kind of fine with that.
>>
>> How about something like this:
>>
>> --- a/drivers/acpi/pptt.c
>> +++ b/drivers/acpi/pptt.c
>> @@ -515,6 +515,8 @@ static int topology_get_acpi_cpu_tag(struct 
>> acpi_table_header *table,
>>    if (level == 0 || cpu_node->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID)
>>      return cpu_node->acpi_processor_id;
>> +   if (level == PPTT_ABORT_PACKAGE)
>> +    pr_warn_once("ACPI Processor ID valid not set for physical 
>> package node, will use node table offset as substitute for UID\n");
> 
> Level will probably never be PPTT_ABORT_PACKAGE, so.. you probably have 
> to have the warning higher up when it terminates the package search.
> 
> OTOH, just because this is set doesn't mean you get "nice" ids. I've had 
> complaints, because there is a firmware floating around which uses the 
> MPIDR value as the processor_id, so the cores come out with really weird 
> numbers too.
> 

JFYI, we got this working now (PPTT has proper physical package ID):

root@(none)$ pwd
/sys/devices/system/cpu/cpu0/topology
root@(none)$ more package_cpus_list
0-63
root@(none)$ more physical_package_id
0
root@(none)$ cd ../../cpu64/topology
root@(none)$ more package_cpus_list
64-127
root@(none)$ more physical_package_id
1
root@(none)$

We'll share the FW tables when ready, so you can check what we have done.

BTW, I still think that we can look to add the warning message when 
physical processor package id valid is not set

Cheers,
John

> 
> 
>>                  return ACPI_PTR_DIFF(cpu_node, table);
>>          }
>>          pr_warn_once("PPTT table found, but unable to locate core %d 
>> (%d)\n",
>>
>> Thanks,
>> John
> 
> .

Re: About PPTT find_acpi_cpu_topology_package()

[Jeremy Linton]

Hi,

On 2/12/20 8:41 AM, John Garry wrote:
> On 12/02/2020 13:55, Sudeep Holla wrote:
>> On Wed, Feb 12, 2020 at 12:48:33PM +0000, John Garry wrote:
>>> On 12/02/2020 11:59, Sudeep Holla wrote:
>>
>> [...]
>>
> 
> Hi Sudeep,
> 
>>>> Yes, as mentioned above. We are not going to do extra work for lazy 
>>>> firmware.
>>>
>>> I don't think it's reasonable to just label this as lazy. The table 
>>> may just
>>> not have the flag set unintentionally. FW and software guys make 
>>> mistakes,
>>> like the mistakes in PPTT, itself.
>>>
>>
>> We are not talking about flags, it's UID and it is pretty important if
>> there are more than one objects of same time.
>>
> 
> I am talking about the Processor ID valid flag, which is specifically 
> related.
> 
>>>> Linux also will be lazy on such platform and provide weird unique 
>>>> numbers
>>>> like in the above case you have mentioned.
>>>
>>> Personally I think that the kernel can be do better than provide 
>>> meaningless
>>> values like this, since it knows the processor IDs and which physical
>>> package they belong to.
>>>
>>
>> This was discussed quite a lot, I can dig and point you to it. That's the
>> reason for choosing offset. We are *not going back* to this again. Fix 
>> the
>> firmware before it gets copied for all future platforms and Linux has to
>> deal with that *forever*.
> 
> I would liked to have been made aware earlier of the oversight. Quite 
> often we only find problems when someone or something complains.
> 
> It is a strange API to provide offsets like this, and I did not realize 
> that they were actually being exposed to userspace.
> 
>>
>>> If not, at least make the user know of potential deficiencies in the 
>>> table.
>>>
>>
>> How ? What are your suggestions ? Does adding a warning or note that UID
>> is missing and offset is chosen help ? 
> 
> I'd say so. I know now, but let's save others the potential hassle. And 
> having this debate again.
> 
> I am kind of fine with that.
> 
> How about something like this:
> 
> --- a/drivers/acpi/pptt.c
> +++ b/drivers/acpi/pptt.c
> @@ -515,6 +515,8 @@ static int topology_get_acpi_cpu_tag(struct 
> acpi_table_header *table,
>    if (level == 0 || cpu_node->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID)
>      return cpu_node->acpi_processor_id;
> +   if (level == PPTT_ABORT_PACKAGE)
> +    pr_warn_once("ACPI Processor ID valid not set for physical package 
> node, will use node table offset as substitute for UID\n");

To clarify my other email there, since I can't seem to type clearly..

Just note that find_acpi_cpu_topology_hetero_id() is also using a 
PPTT_ABORT_PACKAGE termination.


>                  return ACPI_PTR_DIFF(cpu_node, table);
>          }
>          pr_warn_once("PPTT table found, but unable to locate core %d 
> (%d)\n",
> 
> Thanks,
> John

Re: About PPTT find_acpi_cpu_topology_package()

[John Garry]

>>
>> How about something like this:
>>
>> --- a/drivers/acpi/pptt.c
>> +++ b/drivers/acpi/pptt.c
>> @@ -515,6 +515,8 @@ static int topology_get_acpi_cpu_tag(struct 
>> acpi_table_header *table,
>>    if (level == 0 || cpu_node->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID)
>>      return cpu_node->acpi_processor_id;
>> +   if (level == PPTT_ABORT_PACKAGE)
>> +    pr_warn_once("ACPI Processor ID valid not set for physical 
>> package node, will use node table offset as substitute for UID\n");
> 

Hi Jeremy,

> To clarify my other email there, since I can't seem to type clearly..
> 
> Just note that find_acpi_cpu_topology_hetero_id() is also using a 
> PPTT_ABORT_PACKAGE termination.

OK, so I may need to check the flag == ACPI_PPTT_PHYSICAL_PACKAGE also.

BTW, Is the value returned by find_acpi_cpu_topology_hetero_id() also 
exposed to userspace some way? Or any other PPTT offsets?

> 
> 
>>                  return ACPI_PTR_DIFF(cpu_node, table);
>>          }
>>          pr_warn_once("PPTT table found, but unable to locate core %d 
>> (%d)\n",
>>

I'll validate Sudeep's suggestion to set the Processor ID valid flag and 
appropriate processor id for the physical package cpu node with an 
experimental firmware before sending any patch. There seems to be a bit 
of doubt on your part regarding that.

Thanks,
John

Re: About PPTT find_acpi_cpu_topology_package()

[Jeremy Linton]

Hi,

On 2/12/20 10:41 AM, John Garry wrote:
>>>
>>> How about something like this:
>>>
>>> --- a/drivers/acpi/pptt.c
>>> +++ b/drivers/acpi/pptt.c
>>> @@ -515,6 +515,8 @@ static int topology_get_acpi_cpu_tag(struct 
>>> acpi_table_header *table,
>>>    if (level == 0 || cpu_node->flags & 
>>> ACPI_PPTT_ACPI_PROCESSOR_ID_VALID)
>>>      return cpu_node->acpi_processor_id;
>>> +   if (level == PPTT_ABORT_PACKAGE)
>>> +    pr_warn_once("ACPI Processor ID valid not set for physical 
>>> package node, will use node table offset as substitute for UID\n");
>>
> 
> Hi Jeremy,
> 
>> To clarify my other email there, since I can't seem to type clearly..
>>
>> Just note that find_acpi_cpu_topology_hetero_id() is also using a 
>> PPTT_ABORT_PACKAGE termination.
> 
> OK, so I may need to check the flag == ACPI_PPTT_PHYSICAL_PACKAGE also.

Without a lot of thought, it probably sufficient to only check the flag. 
The level is mostly noise, the ==0 check in there was to work around the 
verbiage in the first PPTT revision.

> 
> BTW, Is the value returned by find_acpi_cpu_topology_hetero_id() also  > exposed to userspace some way? Or any other PPTT offsets?

Not yet :)

> 
>>
>>
>>>                  return ACPI_PTR_DIFF(cpu_node, table);
>>>          }
>>>          pr_warn_once("PPTT table found, but unable to locate core %d 
>>> (%d)\n",
>>>
> 
> I'll validate Sudeep's suggestion to set the Processor ID valid flag and 
> appropriate processor id for the physical package cpu node with an 
> experimental firmware before sending any patch. There seems to be a bit 
> of doubt on your part regarding that.

Just pay attention to the definition of _UID/Acpi Processor UID, etc. 
The MADT says that ACPI processor UID is matched with a processor 
container with a matching numeric _UID. The processor container 
definition says that the _UID must be unique in the processor container 
hierarchy.

To me, this says that processor containers/ACPI processors UIDs must all 
be unique. AKA, you can't have both a processor with _UID=1 and a socket 
with _UID=1. Given that linux isn't matching the socket _UID, you can 
create a PPTT+DSDT that does what you want but likely violates the spec.

Re: About PPTT find_acpi_cpu_topology_package()

[John Garry]

On 11/02/2020 21:12, Jeremy Linton wrote:
> Hi,
> 
> On 2/12/20 10:41 AM, John Garry wrote:
>>>>
>>>> How about something like this:
>>>>
>>>> --- a/drivers/acpi/pptt.c
>>>> +++ b/drivers/acpi/pptt.c
>>>> @@ -515,6 +515,8 @@ static int topology_get_acpi_cpu_tag(struct 
>>>> acpi_table_header *table,
>>>>    if (level == 0 || cpu_node->flags & 
>>>> ACPI_PPTT_ACPI_PROCESSOR_ID_VALID)
>>>>      return cpu_node->acpi_processor_id;
>>>> +   if (level == PPTT_ABORT_PACKAGE)
>>>> +    pr_warn_once("ACPI Processor ID valid not set for physical 
>>>> package node, will use node table offset as substitute for UID\n");
>>>
>>
>> Hi Jeremy,
>>
>>> To clarify my other email there, since I can't seem to type clearly..
>>>
>>> Just note that find_acpi_cpu_topology_hetero_id() is also using a 
>>> PPTT_ABORT_PACKAGE termination.
>>
>> OK, so I may need to check the flag == ACPI_PPTT_PHYSICAL_PACKAGE also.
> 
> Without a lot of thought, it probably sufficient to only check the flag. 
> The level is mostly noise, the ==0 check in there was to work around the 
> verbiage in the first PPTT revision.
> 
>>
>> BTW, Is the value returned by find_acpi_cpu_topology_hetero_id() also  
>> > exposed to userspace some way? Or any other PPTT offsets?
> 
> Not yet :)
> 
>>
>>>
>>>
>>>>                  return ACPI_PTR_DIFF(cpu_node, table);
>>>>          }
>>>>          pr_warn_once("PPTT table found, but unable to locate core 
>>>> %d (%d)\n",
>>>>
>>
>> I'll validate Sudeep's suggestion to set the Processor ID valid flag 
>> and appropriate processor id for the physical package cpu node with an 
>> experimental firmware before sending any patch. There seems to be a 
>> bit of doubt on your part regarding that.
> 
> Just pay attention to the definition of _UID/Acpi Processor UID, etc. 
> The MADT says that ACPI processor UID is matched with a processor 
> container with a matching numeric _UID. The processor container 
> definition says that the _UID must be unique in the processor container 
> hierarchy.
> 
> To me, this says that processor containers/ACPI processors UIDs must all 
> be unique. AKA, you can't have both a processor with _UID=1 and a socket 
> with _UID=1. Given that linux isn't matching the socket _UID, you can 
> create a PPTT+DSDT that does what you want but likely violates the spec.

I've spoken to our FW guys, and they say that they do not set the ACPI 
Processor ID valid flag for non-leaf nodes as we have "no Socket, DIE, 
class information". I think that this means that there is no associated 
processor container.

So the spec reads "ACPI Processor ID entry can relate to a Processor 
container in the namespace" and "The processor container will have a 
matching ID value returned through the _UID method"; followed by "As not 
every processor hierarchy node structure in PPTT may have a matching 
processor container, this flag indicates whether the ACPI processor ID 
points to valid entry".

So I take this to mean that if the valid flag is set for a non-leaf 
cell, then the ACPI processor ID will match the UID of the associated 
processor container.

As for when it's not set, it's unclear. My understanding is that the 
ACPI processor id should still be used as the non-leaf node identifier, 
but it would not match a UID for a processor container (as it may not 
exist).

The kernel does have behave according to this.

So how I am misinterpreting this?

Thanks,
John

Re: About PPTT find_acpi_cpu_topology_package()

[Sudeep Holla]

On Thu, Feb 13, 2020 at 11:52:09AM +0000, John Garry wrote:

[...]

>
> As for when it's not set, it's unclear. My understanding is that the ACPI
> processor id should still be used as the non-leaf node identifier, but it
> would not match a UID for a processor container (as it may not exist).
>

I can't infer anything that matches your understanding from the spec in
this regard. If it's not set, then it is left to OSPM.

> The kernel does have behave according to this.
>

According to what in the specification ?

> So how I am misinterpreting this?
>

May be you are not, it is just ambiguous in the spec, worth checking and
fixing if it is an issue. I reiterate we don't want to generate anything
in the OS for this purpose.

--
Regards,
Sudeep

Re: About PPTT find_acpi_cpu_topology_package()

[John Garry]

On 13/02/2020 14:00, Sudeep Holla wrote:
> On Thu, Feb 13, 2020 at 11:52:09AM +0000, John Garry wrote:
> 
> [...]
> 

Hi Sudeep,

>>
>> As for when it's not set, it's unclear. My understanding is that the ACPI
>> processor id should still be used as the non-leaf node identifier, but it
>> would not match a UID for a processor container (as it may not exist).
>>
> 
> I can't infer anything that matches your understanding from the spec in
> this regard. If it's not set, then it is left to OSPM.
> 

ACPI Processor ID valid just means that there is an associated processor 
container entry which has a UID which matches the ACPI Processor ID for 
this node.

I can't see anything to say that if the ACPI Processor ID valid flag is 
unset then the ACPI processor ID itself is not still a valid identifier. 
As such, it's implied that it is still valid. But the spec should be 
clarified here.

>> The kernel does have behave according to this.
>>
> 
> According to what in the specification ?

That's just based on my interpretation, above. That being, the ACPI 
Processor ID is always valid, independent of whether ACPI Processor ID 
valid flag is set.

> 
>> So how I am misinterpreting this?
>>
> 
> May be you are not, it is just ambiguous in the spec, worth checking and
> fixing if it is an issue.

Absolutely agree on all of this.

> I reiterate we don't want to generate anything
> in the OS for this purpose.

I'm ok with that.

Cheers,
John

Re: About PPTT find_acpi_cpu_topology_package()

[Jeremy Linton]

Hi,


On 2/13/20 8:33 AM, John Garry wrote:
> On 13/02/2020 14:00, Sudeep Holla wrote:
>> On Thu, Feb 13, 2020 at 11:52:09AM +0000, John Garry wrote:
>>
>> [...]
>>
> 
> Hi Sudeep,
> 
>>>
>>> As for when it's not set, it's unclear. My understanding is that the 
>>> ACPI
>>> processor id should still be used as the non-leaf node identifier, 
>>> but it
>>> would not match a UID for a processor container (as it may not exist).
>>>
>>
>> I can't infer anything that matches your understanding from the spec in
>> this regard. If it's not set, then it is left to OSPM.
>>
> 
> ACPI Processor ID valid just means that there is an associated processor 
> container entry which has a UID which matches the ACPI Processor ID for 
> this node.
> 
> I can't see anything to say that if the ACPI Processor ID valid flag is 
> unset then the ACPI processor ID itself is not still a valid identifier. 
> As such, it's implied that it is still valid. But the spec should be 
> clarified here.

I see what your saying here, but I think the implication is that no 
useful information is contained in the field when its not marked valid.
"The flags field (...) includes a bit to describe whether (this field) 
is valid"

Make sure your looking at ACPI 6.3+ because its a lot cleaner than the 
earlier revisions, particularly around the leaf node case.

Re: About PPTT find_acpi_cpu_topology_package()

[John Garry]

>>>
>>
>> ACPI Processor ID valid just means that there is an associated 
>> processor container entry which has a UID which matches the ACPI 
>> Processor ID for this node.
>>
>> I can't see anything to say that if the ACPI Processor ID valid flag 
>> is unset then the ACPI processor ID itself is not still a valid 
>> identifier. As such, it's implied that it is still valid. But the spec 
>> should be clarified here.
> 
> I see what your saying here, but I think the implication is that no 
> useful information is contained in the field when its not marked valid.
> "The flags field (...) includes a bit to describe whether (this field) 
> is valid"

OK, right. So I think that the wording can be improved in the spec, 
specifically around the meaning in ACPI Processor ID valid.

> 
> Make sure your looking at ACPI 6.3+ because its a lot cleaner than the 
> earlier revisions, particularly around the leaf node case.

So you have something newer than 
https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf?

> .

So my FW colleague tells me that they tried adding processor containers 
for hierarchy components, but the kernel complained. I don't know the 
specifics. I need to follow up on that.

Do I see this, which we could refer to:

https://github.com/tianocore/edk2-platforms/blob/master/Platform/ARM/JunoPkg/AcpiTables/Dsdt.asl#L36

Any more pointers as references would be appreciated.

Thanks,
John

Re: About PPTT find_acpi_cpu_topology_package()

[Sudeep Holla]

On Fri, Feb 14, 2020 at 10:35:03AM +0000, John Garry wrote:

[...]

>
> So you have something newer than
> https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf?
>

No, I too refer the same.

> > .
>
> So my FW colleague tells me that they tried adding processor containers for
> hierarchy components, but the kernel complained. I don't know the specifics.
> I need to follow up on that.
>
> Do I see this, which we could refer to:
>
> https://github.com/tianocore/edk2-platforms/blob/master/Platform/ARM/JunoPkg/AcpiTables/Dsdt.asl#L36
>

OK, if anything is wrong there, I am the person to blame. I must say that
this was added pre-PPTT dates for LPI. So I expect things may not be up
to date TBH.

> Any more pointers as references would be appreciated.
>

Nothing more than above sole reference.

--
Regards,
Sudeep

Re: About PPTT find_acpi_cpu_topology_package()

[Sudeep Holla]

On Wed, Feb 12, 2020 at 02:41:04PM +0000, John Garry wrote:
> On 12/02/2020 13:55, Sudeep Holla wrote:
> > On Wed, Feb 12, 2020 at 12:48:33PM +0000, John Garry wrote:
> > > On 12/02/2020 11:59, Sudeep Holla wrote:
> >
> > [...]
> >
>
> Hi Sudeep,
>
> > > > Yes, as mentioned above. We are not going to do extra work for lazy firmware.
> > >
> > > I don't think it's reasonable to just label this as lazy. The table may just
> > > not have the flag set unintentionally. FW and software guys make mistakes,
> > > like the mistakes in PPTT, itself.
> > >
> >
> > We are not talking about flags, it's UID and it is pretty important if
> > there are more than one objects of same time.
> >
>
> I am talking about the Processor ID valid flag, which is specifically
> related.
>

Ah OK, sorry I had forgotten the specific. I recall it now.

> > > > Linux also will be lazy on such platform and provide weird unique numbers
> > > > like in the above case you have mentioned.
> > >
> > > Personally I think that the kernel can be do better than provide meaningless
> > > values like this, since it knows the processor IDs and which physical
> > > package they belong to.
> > >
> >
> > This was discussed quite a lot, I can dig and point you to it. That's the
> > reason for choosing offset. We are *not going back* to this again. Fix the
> > firmware before it gets copied for all future platforms and Linux has to
> > deal with that *forever*.
>
> I would liked to have been made aware earlier of the oversight. Quite often
> we only find problems when someone or something complains.
>

Agreed.

> It is a strange API to provide offsets like this, and I did not realize that
> they were actually being exposed to userspace.
>

We couldn't come up with something that produces same result always and
obtained from firmware data. Yes that being in the user-space was the main
concern for not generating it in the Linux as we can't guarantee to generate
same ID for a given physical socket. Depends on the order in which we boot
them or something similar.

> >
> > > If not, at least make the user know of potential deficiencies in the table.
> > >
> >
> > How ? What are your suggestions ? Does adding a warning or note that UID
> > is missing and offset is chosen help ?
>
> I'd say so. I know now, but let's save others the potential hassle. And
> having this debate again.
>

No argument there. I agree completely.

> I am kind of fine with that.
>
> How about something like this:
>

Looks good to me. Please post the patch. I am not sure on Rafael's
preference on such lengthy warnings(does it need to be split ?)

--
Regards,
Sudeep