* Re: About PPTT find_acpi_cpu_topology_package()
2020-02-12 14:41 ` John Garry
@ 2020-02-11 19:01 ` Jeremy Linton
2020-03-25 11:43 ` John Garry
2020-02-11 19:31 ` Jeremy Linton
2020-02-12 15:32 ` Sudeep Holla
2 siblings, 1 reply; 19+ messages in thread
From: Jeremy Linton @ 2020-02-11 19:01 UTC (permalink / raw)
To: John Garry, Sudeep Holla
Cc: Guohanjun (Hanjun Guo), ACPI Devel Maling List, liuqi (BA), wanghuiqiang
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: About PPTT find_acpi_cpu_topology_package()
2020-02-11 19:01 ` Jeremy Linton
@ 2020-03-25 11:43 ` John Garry
0 siblings, 0 replies; 19+ messages in thread
From: John Garry @ 2020-03-25 11:43 UTC (permalink / raw)
To: Jeremy Linton, Sudeep Holla
Cc: Guohanjun (Hanjun Guo), ACPI Devel Maling List, liuqi (BA),
wanghuiqiang, wangxiongfeng (C)
>>>
>>>> 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
>
> .
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: About PPTT find_acpi_cpu_topology_package()
2020-02-12 14:41 ` John Garry
2020-02-11 19:01 ` Jeremy Linton
@ 2020-02-11 19:31 ` Jeremy Linton
2020-02-12 16:41 ` John Garry
2020-02-12 15:32 ` Sudeep Holla
2 siblings, 1 reply; 19+ messages in thread
From: Jeremy Linton @ 2020-02-11 19:31 UTC (permalink / raw)
To: John Garry, Sudeep Holla
Cc: Guohanjun (Hanjun Guo), ACPI Devel Maling List, liuqi (BA), wanghuiqiang
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: About PPTT find_acpi_cpu_topology_package()
2020-02-11 19:31 ` Jeremy Linton
@ 2020-02-12 16:41 ` John Garry
2020-02-11 21:12 ` Jeremy Linton
0 siblings, 1 reply; 19+ messages in thread
From: John Garry @ 2020-02-12 16:41 UTC (permalink / raw)
To: Jeremy Linton, Sudeep Holla
Cc: Guohanjun (Hanjun Guo), ACPI Devel Maling List, liuqi (BA), wanghuiqiang
>>
>> 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: About PPTT find_acpi_cpu_topology_package()
2020-02-12 16:41 ` John Garry
@ 2020-02-11 21:12 ` Jeremy Linton
2020-02-13 11:52 ` John Garry
0 siblings, 1 reply; 19+ messages in thread
From: Jeremy Linton @ 2020-02-11 21:12 UTC (permalink / raw)
To: John Garry, Sudeep Holla
Cc: Guohanjun (Hanjun Guo), ACPI Devel Maling List, liuqi (BA), wanghuiqiang
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: About PPTT find_acpi_cpu_topology_package()
2020-02-11 21:12 ` Jeremy Linton
@ 2020-02-13 11:52 ` John Garry
2020-02-13 14:00 ` Sudeep Holla
0 siblings, 1 reply; 19+ messages in thread
From: John Garry @ 2020-02-13 11:52 UTC (permalink / raw)
To: Jeremy Linton, Sudeep Holla
Cc: Guohanjun (Hanjun Guo), ACPI Devel Maling List, liuqi (BA),
wanghuiqiang, wangxiongfeng (C)
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: About PPTT find_acpi_cpu_topology_package()
2020-02-13 11:52 ` John Garry
@ 2020-02-13 14:00 ` Sudeep Holla
2020-02-13 14:33 ` John Garry
0 siblings, 1 reply; 19+ messages in thread
From: Sudeep Holla @ 2020-02-13 14:00 UTC (permalink / raw)
To: John Garry
Cc: Jeremy Linton, Guohanjun (Hanjun Guo),
ACPI Devel Maling List, liuqi (BA),
wanghuiqiang, wangxiongfeng (C),
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: About PPTT find_acpi_cpu_topology_package()
2020-02-13 14:00 ` Sudeep Holla
@ 2020-02-13 14:33 ` John Garry
2020-02-13 16:52 ` Jeremy Linton
0 siblings, 1 reply; 19+ messages in thread
From: John Garry @ 2020-02-13 14:33 UTC (permalink / raw)
To: Sudeep Holla
Cc: Jeremy Linton, Guohanjun (Hanjun Guo),
ACPI Devel Maling List, liuqi (BA),
wanghuiqiang, wangxiongfeng (C)
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: About PPTT find_acpi_cpu_topology_package()
2020-02-13 14:33 ` John Garry
@ 2020-02-13 16:52 ` Jeremy Linton
2020-02-14 10:35 ` John Garry
0 siblings, 1 reply; 19+ messages in thread
From: Jeremy Linton @ 2020-02-13 16:52 UTC (permalink / raw)
To: John Garry, Sudeep Holla
Cc: Guohanjun (Hanjun Guo), ACPI Devel Maling List, liuqi (BA),
wanghuiqiang, wangxiongfeng (C)
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: About PPTT find_acpi_cpu_topology_package()
2020-02-13 16:52 ` Jeremy Linton
@ 2020-02-14 10:35 ` John Garry
2020-02-14 11:22 ` Sudeep Holla
0 siblings, 1 reply; 19+ messages in thread
From: John Garry @ 2020-02-14 10:35 UTC (permalink / raw)
To: Jeremy Linton, Sudeep Holla
Cc: Guohanjun (Hanjun Guo), ACPI Devel Maling List, liuqi (BA),
wanghuiqiang, wangxiongfeng (C)
>>>
>>
>> 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: About PPTT find_acpi_cpu_topology_package()
2020-02-14 10:35 ` John Garry
@ 2020-02-14 11:22 ` Sudeep Holla
0 siblings, 0 replies; 19+ messages in thread
From: Sudeep Holla @ 2020-02-14 11:22 UTC (permalink / raw)
To: John Garry
Cc: Jeremy Linton, Guohanjun (Hanjun Guo),
ACPI Devel Maling List, liuqi (BA),
wanghuiqiang, wangxiongfeng (C),
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: About PPTT find_acpi_cpu_topology_package()
2020-02-12 14:41 ` John Garry
2020-02-11 19:01 ` Jeremy Linton
2020-02-11 19:31 ` Jeremy Linton
@ 2020-02-12 15:32 ` Sudeep Holla
2 siblings, 0 replies; 19+ messages in thread
From: Sudeep Holla @ 2020-02-12 15:32 UTC (permalink / raw)
To: John Garry
Cc: Jeremy Linton, Guohanjun (Hanjun Guo),
ACPI Devel Maling List, liuqi (BA),
wanghuiqiang, 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
^ permalink raw reply [flat|nested] 19+ messages in thread