ACPI 6.3 adds a flag to the CPU node to indicate whether
the given PE is a thread. Add a function to return that
information for a given linux logical CPU.
Signed-off-by: Jeremy Linton <[email protected]>
---
drivers/acpi/pptt.c | 53 +++++++++++++++++++++++++++++++++++++++++++-
include/linux/acpi.h | 5 +++++
2 files changed, 57 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
index b72e6afaa8fb..6f45f8c07b46 100644
--- a/drivers/acpi/pptt.c
+++ b/drivers/acpi/pptt.c
@@ -517,6 +517,43 @@ static int find_acpi_cpu_topology_tag(unsigned int cpu, int level, int flag)
return retval;
}
+/**
+ * check_acpi_cpu_flag() - Determine if CPU node has a flag set
+ * @cpu: Kernel logical CPU number
+ * @rev: The PPTT revision defining the flag
+ * @flag: The flag itself
+ *
+ * Check the node representing a CPU for a given flag.
+ *
+ * Return: -ENOENT if the PPTT doesn't exist, the CPU cannot be found or
+ * the table revision isn't new enough.
+ * Otherwise returns flag value
+ */
+static int check_acpi_cpu_flag(unsigned int cpu, int rev, u32 flag)
+{
+ struct acpi_table_header *table;
+ acpi_status status;
+ u32 acpi_cpu_id = get_acpi_id_for_cpu(cpu);
+ struct acpi_pptt_processor *cpu_node = NULL;
+ int ret = -ENOENT;
+
+ status = acpi_get_table(ACPI_SIG_PPTT, 0, &table);
+ if (ACPI_FAILURE(status)) {
+ acpi_pptt_warn_missing();
+ return ret;
+ }
+
+ if (table->revision >= rev)
+ cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
+
+ if (cpu_node)
+ ret = cpu_node->flags & flag;
+
+ acpi_put_table(table);
+
+ return ret;
+}
+
/**
* acpi_find_last_cache_level() - Determines the number of cache levels for a PE
* @cpu: Kernel logical CPU number
@@ -581,6 +618,21 @@ int cache_setup_acpi(unsigned int cpu)
return status;
}
+/**
+ * acpi_pptt_cpu_is_thread() - Determine if CPU is a thread
+ * @cpu: Kernel logical CPU number
+ *
+ *
+ * Return: 1, a thread
+ * 0, not a thread
+ * -ENOENT ,if the PPTT doesn't exist, the CPU cannot be found or
+ * the table revision isn't new enough.
+ */
+int acpi_pptt_cpu_is_thread(unsigned int cpu)
+{
+ return check_acpi_cpu_flag(cpu, 2, ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD);
+}
+
/**
* find_acpi_cpu_topology() - Determine a unique topology value for a given CPU
* @cpu: Kernel logical CPU number
@@ -641,7 +693,6 @@ int find_acpi_cpu_cache_topology(unsigned int cpu, int level)
return ret;
}
-
/**
* find_acpi_cpu_topology_package() - Determine a unique CPU package value
* @cpu: Kernel logical CPU number
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index d315d86844e4..3e339375e213 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1301,10 +1301,15 @@ static inline int lpit_read_residency_count_address(u64 *address)
#endif
#ifdef CONFIG_ACPI_PPTT
+int acpi_pptt_cpu_is_thread(unsigned int cpu);
int find_acpi_cpu_topology(unsigned int cpu, int level);
int find_acpi_cpu_topology_package(unsigned int cpu);
int find_acpi_cpu_cache_topology(unsigned int cpu, int level);
#else
+static inline int acpi_pptt_cpu_is_thread(unsigned int cpu)
+{
+ return -EINVAL;
+}
static inline int find_acpi_cpu_topology(unsigned int cpu, int level)
{
return -EINVAL;
--
2.21.0
Hi Jeremy,
Few nits below.
Also, I had a look at the other PPTT processor flags that were introduced
in 6.3, and the only other one being used is ACPI_LEAF_NODE in
acpi_pptt_leaf_node(). However that one already has a handle on the table
header, so the check_acpi_cpu_flag() isn't of much help there.
I don't believe the other existing flags will benefit from the helper since
they are more about describing the PPTT tree, but I think it doesn't hurt
to keep it around for potential future flags.
On 14/06/2019 23:31, Jeremy Linton wrote:
[...]
> @@ -517,6 +517,43 @@ static int find_acpi_cpu_topology_tag(unsigned int cpu, int level, int flag)
> return retval;
> }
>
> +/**
> + * check_acpi_cpu_flag() - Determine if CPU node has a flag set
> + * @cpu: Kernel logical CPU number
> + * @rev: The PPTT revision defining the flag
> + * @flag: The flag itself
> + *
> + * Check the node representing a CPU for a given flag.
> + *
> + * Return: -ENOENT if the PPTT doesn't exist, the CPU cannot be found or
> + * the table revision isn't new enough.
> + * Otherwise returns flag value
> + */
Nit: strictly speaking we're not returning the flag value but its mask
applied to the flags field. I don't think anyone will care about getting
the actual flag value, but it should be made obvious in the doc:
-ENOENT if ...
0 if the flag isn't set
> 0 if it is set.
[...]
> @@ -581,6 +618,21 @@ int cache_setup_acpi(unsigned int cpu)
> return status;
> }
>
> +/**
> + * acpi_pptt_cpu_is_thread() - Determine if CPU is a thread
> + * @cpu: Kernel logical CPU number
> + *
> + *
Nit: extra newline
> + * Return: 1, a thread
> + * 0, not a thread
> + * -ENOENT ,if the PPTT doesn't exist, the CPU cannot be found or
> + * the table revision isn't new enough.
> + */
> +int acpi_pptt_cpu_is_thread(unsigned int cpu)
> +{
> + return check_acpi_cpu_flag(cpu, 2, ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD);
> +}
> +
> /**
> * find_acpi_cpu_topology() - Determine a unique topology value for a given CPU
> * @cpu: Kernel logical CPU number
> @@ -641,7 +693,6 @@ int find_acpi_cpu_cache_topology(unsigned int cpu, int level)
[...]
On 6/17/19 7:34 AM, Valentin Schneider wrote:
> Hi Jeremy,
>
> Few nits below.
>
> Also, I had a look at the other PPTT processor flags that were introduced
> in 6.3, and the only other one being used is ACPI_LEAF_NODE in
> acpi_pptt_leaf_node(). However that one already has a handle on the table
> header, so the check_acpi_cpu_flag() isn't of much help there.
>
> I don't believe the other existing flags will benefit from the helper since
> they are more about describing the PPTT tree, but I think it doesn't hurt
> to keep it around for potential future flags.
That was the thought process.
>
> On 14/06/2019 23:31, Jeremy Linton wrote:
> [...]
>> @@ -517,6 +517,43 @@ static int find_acpi_cpu_topology_tag(unsigned int cpu, int level, int flag)
>> return retval;
>> }
>>
>> +/**
>> + * check_acpi_cpu_flag() - Determine if CPU node has a flag set
>> + * @cpu: Kernel logical CPU number
>> + * @rev: The PPTT revision defining the flag
>> + * @flag: The flag itself
>> + *
>> + * Check the node representing a CPU for a given flag.
>> + *
>> + * Return: -ENOENT if the PPTT doesn't exist, the CPU cannot be found or
>> + * the table revision isn't new enough.
>> + * Otherwise returns flag value
>> + */
>
> Nit: strictly speaking we're not returning the flag value but its mask
> applied to the flags field. I don't think anyone will care about getting
> the actual flag value, but it should be made obvious in the doc:
Or I clarify the code to actually do what the comments says. Maybe that
is what John G was also pointing out too?
>
> -ENOENT if ...
> 0 if the flag isn't set
>> 0 if it is set.
>
> [...]
>> @@ -581,6 +618,21 @@ int cache_setup_acpi(unsigned int cpu)
>> return status;
>> }
>>
>> +/**
>> + * acpi_pptt_cpu_is_thread() - Determine if CPU is a thread
>> + * @cpu: Kernel logical CPU number
>> + *
>> + *
>
> Nit: extra newline
>
>> + * Return: 1, a thread
>> + * 0, not a thread
>> + * -ENOENT ,if the PPTT doesn't exist, the CPU cannot be found or
>> + * the table revision isn't new enough.
>> + */
>> +int acpi_pptt_cpu_is_thread(unsigned int cpu)
>> +{
>> + return check_acpi_cpu_flag(cpu, 2, ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD);
>> +}
>> +
>> /**
>> * find_acpi_cpu_topology() - Determine a unique topology value for a given CPU
>> * @cpu: Kernel logical CPU number
>> @@ -641,7 +693,6 @@ int find_acpi_cpu_cache_topology(unsigned int cpu, int level)
> [...]
>
On 18/06/2019 15:21, Jeremy Linton wrote:
[...]
>>> + * Return: -ENOENT if the PPTT doesn't exist, the CPU cannot be found or
>>> + * the table revision isn't new enough.
>>> + * Otherwise returns flag value
>>> + */
>>
>> Nit: strictly speaking we're not returning the flag value but its mask
>> applied to the flags field. I don't think anyone will care about getting
>> the actual flag value, but it should be made obvious in the doc:
>
> Or I clarify the code to actually do what the comments says. Maybe that is what John G was also pointing out too?
>
Mmm I didn't find any reply from John regarding this in v1, but I wouldn't
mind either way, as long as the doc & code are aligned.
[...]
On 18/06/2019 15:40, Valentin Schneider wrote:
> On 18/06/2019 15:21, Jeremy Linton wrote:
> [...]
>>>> + * Return: -ENOENT if the PPTT doesn't exist, the CPU cannot be found or
>>>> + * the table revision isn't new enough.
>>>> + * Otherwise returns flag value
>>>> + */
>>>
>>> Nit: strictly speaking we're not returning the flag value but its mask
>>> applied to the flags field. I don't think anyone will care about getting
>>> the actual flag value, but it should be made obvious in the doc:
>>
>> Or I clarify the code to actually do what the comments says. Maybe that is what John G was also pointing out too?
>>
No, I was just saying that the kernel topology can be broken without
this series.
>
> Mmm I didn't find any reply from John regarding this in v1, but I wouldn't
> mind either way, as long as the doc & code are aligned.
>
BTW, to me, function acpi_pptt_cpu_is_thread() seems to try to do too
much, i.e. check if the PPTT is new enough to support the thread flag
and also check if it is set for a specific cpu. I'd consider separate
functions here.
thanks,
John
> [...]
>
> .
>
Hi,
On 6/18/19 12:23 PM, John Garry wrote:
> On 18/06/2019 15:40, Valentin Schneider wrote:
>> On 18/06/2019 15:21, Jeremy Linton wrote:
>> [...]
>>>>> + * Return: -ENOENT if the PPTT doesn't exist, the CPU cannot be
>>>>> found or
>>>>> + * the table revision isn't new enough.
>>>>> + * Otherwise returns flag value
>>>>> + */
>>>>
>>>> Nit: strictly speaking we're not returning the flag value but its mask
>>>> applied to the flags field. I don't think anyone will care about
>>>> getting
>>>> the actual flag value, but it should be made obvious in the doc:
>>>
>>> Or I clarify the code to actually do what the comments says. Maybe
>>> that is what John G was also pointing out too?
>>>
>
> No, I was just saying that the kernel topology can be broken without
> this series.
>
>>
>> Mmm I didn't find any reply from John regarding this in v1, but I
>> wouldn't
>> mind either way, as long as the doc & code are aligned.
>>
>
> BTW, to me, function acpi_pptt_cpu_is_thread() seems to try to do too
> much, i.e. check if the PPTT is new enough to support the thread flag
> and also check if it is set for a specific cpu. I'd consider separate
> functions here.
? Your suggesting replacing the
if (table->revision >= rev)
cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
check with
if (revision_check(table, rev))
cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
and a function like
static int revision_check(acpixxxx *table, int rev)
{
return (table->revision >= rev);
}
Although, frankly if one were to do this, it should probably be a macro
with the table type, and used in the dozen or so other places I found
doing similar checks (spcr, iort, etc).
Or something else?
>
> thanks,
> John
>
>> [...]
>>
>> .
>>
>
>
On 18/06/2019 22:28, Jeremy Linton wrote:
> Hi,
>
> On 6/18/19 12:23 PM, John Garry wrote:
>> On 18/06/2019 15:40, Valentin Schneider wrote:
>>> On 18/06/2019 15:21, Jeremy Linton wrote:
>>> [...]
>>>>>> + * Return: -ENOENT if the PPTT doesn't exist, the CPU cannot be
>>>>>> found or
>>>>>> + * the table revision isn't new enough.
>>>>>> + * Otherwise returns flag value
>>>>>> + */
>>>>>
>>>>> Nit: strictly speaking we're not returning the flag value but its mask
>>>>> applied to the flags field. I don't think anyone will care about
>>>>> getting
>>>>> the actual flag value, but it should be made obvious in the doc:
>>>>
>>>> Or I clarify the code to actually do what the comments says. Maybe
>>>> that is what John G was also pointing out too?
>>>>
>>
>> No, I was just saying that the kernel topology can be broken without
>> this series.
>>
>>>
>>> Mmm I didn't find any reply from John regarding this in v1, but I
>>> wouldn't
>>> mind either way, as long as the doc & code are aligned.
>>>
>>
>> BTW, to me, function acpi_pptt_cpu_is_thread() seems to try to do too
>> much, i.e. check if the PPTT is new enough to support the thread flag
>> and also check if it is set for a specific cpu. I'd consider separate
>> functions here.
>
Hi,
> ? Your suggesting replacing the
>
I am not saying definitely that this should be changed, it's just that
acpi_pptt_cpu_is_thread() returning false, true, or "no entry" is not a
typical API format.
How about acpi_pptt_support_thread_info(cpu) and
acpi_pptt_cpu_is_threaded(cpu), both returning false/true only?
None of this is ideal.
BTW, Have you audited which arm64 systems have MT bit set legitimately?
>
> if (table->revision >= rev)
I know that checking the table revision is not on the fast path, but it
seems unnecessarily inefficient to always read it this way, I mean
calling acpi_table_get().
Can you have a static value for the table revision? Or is this just how
other table info is accessed in ACPI code?
> cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
>
> check with
>
> if (revision_check(table, rev))
> cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
>
>
> and a function like
>
> static int revision_check(acpixxxx *table, int rev)
> {
> return (table->revision >= rev);
> }
>
> Although, frankly if one were to do this, it should probably be a macro
> with the table type, and used in the dozen or so other places I found
> doing similar checks (spcr, iort, etc).
>
> Or something else?
>
>
>
>
thanks,
John
>>
>>> [...]
>>>
>>> .
>>>
>>
>>
>
>
> .
>
On Mon, Jun 17, 2019 at 01:34:51PM +0100, Valentin Schneider wrote:
> Hi Jeremy,
>
> Few nits below.
>
> Also, I had a look at the other PPTT processor flags that were introduced
> in 6.3, and the only other one being used is ACPI_LEAF_NODE in
> acpi_pptt_leaf_node(). However that one already has a handle on the table
> header, so the check_acpi_cpu_flag() isn't of much help there.
>
> I don't believe the other existing flags will benefit from the helper since
> they are more about describing the PPTT tree, but I think it doesn't hurt
> to keep it around for potential future flags.
>
> On 14/06/2019 23:31, Jeremy Linton wrote:
> [...]
> > @@ -517,6 +517,43 @@ static int find_acpi_cpu_topology_tag(unsigned int cpu, int level, int flag)
> > return retval;
> > }
> >
> > +/**
> > + * check_acpi_cpu_flag() - Determine if CPU node has a flag set
> > + * @cpu: Kernel logical CPU number
> > + * @rev: The PPTT revision defining the flag
> > + * @flag: The flag itself
How about the "the processor structure flag being examined" ?
> > + *
> > + * Check the node representing a CPU for a given flag.
> > + *
> > + * Return: -ENOENT if the PPTT doesn't exist, the CPU cannot be found or
> > + * the table revision isn't new enough.
> > + * Otherwise returns flag value
> > + */
>
> Nit: strictly speaking we're not returning the flag value but its mask
> applied to the flags field. I don't think anyone will care about getting
> the actual flag value, but it should be made obvious in the doc:
>
I agree with that. I am also fine if you want to change the code to
return 0 or 1 based on the flag value. It then aligns well with comment
under acpi_pptt_cpu_is_thread. Either way, we just need consistency.
--
Regards,
Sudeep
Hi,
On 6/19/19 4:15 AM, John Garry wrote:
> On 18/06/2019 22:28, Jeremy Linton wrote:
>> Hi,
>>
>> On 6/18/19 12:23 PM, John Garry wrote:
>>> On 18/06/2019 15:40, Valentin Schneider wrote:
>>>> On 18/06/2019 15:21, Jeremy Linton wrote:
>>>> [...]
>>>>>>> + * Return: -ENOENT if the PPTT doesn't exist, the CPU cannot be
>>>>>>> found or
>>>>>>> + * the table revision isn't new enough.
>>>>>>> + * Otherwise returns flag value
>>>>>>> + */
>>>>>>
>>>>>> Nit: strictly speaking we're not returning the flag value but its
>>>>>> mask
>>>>>> applied to the flags field. I don't think anyone will care about
>>>>>> getting
>>>>>> the actual flag value, but it should be made obvious in the doc:
>>>>>
>>>>> Or I clarify the code to actually do what the comments says. Maybe
>>>>> that is what John G was also pointing out too?
>>>>>
>>>
>>> No, I was just saying that the kernel topology can be broken without
>>> this series.
>>>
>>>>
>>>> Mmm I didn't find any reply from John regarding this in v1, but I
>>>> wouldn't
>>>> mind either way, as long as the doc & code are aligned.
>>>>
>>>
>>> BTW, to me, function acpi_pptt_cpu_is_thread() seems to try to do too
>>> much, i.e. check if the PPTT is new enough to support the thread flag
>>> and also check if it is set for a specific cpu. I'd consider separate
>>> functions here.
>>
>
> Hi,
>
>> ? Your suggesting replacing the
>>
>
> I am not saying definitely that this should be changed, it's just that
> acpi_pptt_cpu_is_thread() returning false, true, or "no entry" is not a
> typical API format.
>
> How about acpi_pptt_support_thread_info(cpu) and
> acpi_pptt_cpu_is_threaded(cpu), both returning false/true only?
I'm not sure we want to be exporting what is effectively a version check
into the rest of the code. Plus, AFAIK it doesn't really simplify
anything except the case of ACPI machines with revision 1 PPTTs, because
those would only be doing a single check and assuming the state of the
MT bit. That MT check is suspect anyway, although AFAIK it gets the
right answer on all machines that predate ACPI 6.3..
>
> None of this is ideal.
>
> BTW, Have you audited which arm64 systems have MT bit set legitimately?
Not formally, given I don't have access to everything available.
>
>>
>> if (table->revision >= rev)
>
> I know that checking the table revision is not on the fast path, but it
> seems unnecessarily inefficient to always read it this way, I mean
> calling acpi_table_get().
>
> Can you have a static value for the table revision? Or is this just how
> other table info is accessed in ACPI code?
Yes caching the revision internally would save a get/put per core, for
older machines. I don't think its a big deal in normal operation but its
a couple extra lines so...
I will post it with an internally cached saved_pptt_rev. That will save
CPU count get/puts in the case where the revision isn't new enough.
>
>> cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
>>
>> check with
>>
>> if (revision_check(table, rev))
>> cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
>>
>>
>> and a function like
>>
>> static int revision_check(acpixxxx *table, int rev)
>> {
>> return (table->revision >= rev);
>> }
>>
>> Although, frankly if one were to do this, it should probably be a macro
>> with the table type, and used in the dozen or so other places I found
>> doing similar checks (spcr, iort, etc).
>>
>> Or something else?
>>
>>
>>
>>
>
> thanks,
> John