diff mbox series

[v2,1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag

Message ID 20190614223158.49575-2-jeremy.linton@arm.com (mailing list archive)
State New, archived
Headers show
Series arm64/PPTT ACPI 6.3 thread flag support | expand

Commit Message

Jeremy Linton June 14, 2019, 10:31 p.m. UTC
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 <jeremy.linton@arm.com>
---
 drivers/acpi/pptt.c  | 53 +++++++++++++++++++++++++++++++++++++++++++-
 include/linux/acpi.h |  5 +++++
 2 files changed, 57 insertions(+), 1 deletion(-)

Comments

Valentin Schneider June 17, 2019, 12:34 p.m. UTC | #1
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)
[...]
Jeremy Linton June 18, 2019, 2:21 p.m. UTC | #2
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)
> [...]
>
Valentin Schneider June 18, 2019, 2:40 p.m. UTC | #3
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.

[...]
John Garry June 18, 2019, 5:23 p.m. UTC | #4
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

> [...]
>
> .
>
Jeremy Linton June 18, 2019, 9:28 p.m. UTC | #5
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
> 
>> [...]
>>
>> .
>>
> 
>
John Garry June 19, 2019, 9:15 a.m. UTC | #6
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

>>
>>> [...]
>>>
>>> .
>>>
>>
>>
>
>
> .
>
Sudeep Holla June 25, 2019, 3:20 p.m. UTC | #7
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
Jeremy Linton June 28, 2019, 3:21 p.m. UTC | #8
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
diff mbox series

Patch

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;