* [PATCH v2 0/2] arm64/PPTT ACPI 6.3 thread flag support
@ 2019-06-14 22:31 Jeremy Linton
2019-06-14 22:31 ` [PATCH v2 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag Jeremy Linton
2019-06-14 22:31 ` [PATCH v2 2/2] arm64: topology: Use PPTT to determine if PE is a thread Jeremy Linton
0 siblings, 2 replies; 11+ messages in thread
From: Jeremy Linton @ 2019-06-14 22:31 UTC (permalink / raw)
To: linux-arm-kernel
Cc: linux-acpi, linux-kernel, catalin.marinas, will.deacon, rjw,
sudeep.holla, lenb, Jeremy Linton
ACPI 6.3 adds a flag to the CPU node to indicate whether
the given CPU is a thread. Add a function to return that
information for a given linux logical CPU and then utilize
it while building the arm64 topology.
v1->v2:
Return ENOENT instead on ENONET.
Jeremy Linton (2):
ACPI/PPTT: Add support for ACPI 6.3 thread flag
arm64: topology: Use PPTT to determine if PE is a thread
arch/arm64/kernel/topology.c | 8 ++++--
drivers/acpi/pptt.c | 53 +++++++++++++++++++++++++++++++++++-
include/linux/acpi.h | 5 ++++
3 files changed, 62 insertions(+), 4 deletions(-)
--
2.21.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag
2019-06-14 22:31 [PATCH v2 0/2] arm64/PPTT ACPI 6.3 thread flag support Jeremy Linton
@ 2019-06-14 22:31 ` Jeremy Linton
2019-06-17 12:34 ` Valentin Schneider
2019-06-14 22:31 ` [PATCH v2 2/2] arm64: topology: Use PPTT to determine if PE is a thread Jeremy Linton
1 sibling, 1 reply; 11+ messages in thread
From: Jeremy Linton @ 2019-06-14 22:31 UTC (permalink / raw)
To: linux-arm-kernel
Cc: linux-acpi, linux-kernel, catalin.marinas, will.deacon, rjw,
sudeep.holla, lenb, Jeremy Linton
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(-)
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/2] arm64: topology: Use PPTT to determine if PE is a thread
2019-06-14 22:31 [PATCH v2 0/2] arm64/PPTT ACPI 6.3 thread flag support Jeremy Linton
2019-06-14 22:31 ` [PATCH v2 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag Jeremy Linton
@ 2019-06-14 22:31 ` Jeremy Linton
1 sibling, 0 replies; 11+ messages in thread
From: Jeremy Linton @ 2019-06-14 22:31 UTC (permalink / raw)
To: linux-arm-kernel
Cc: linux-acpi, linux-kernel, catalin.marinas, will.deacon, rjw,
sudeep.holla, lenb, Jeremy Linton
ACPI 6.3 adds a thread flag to represent if a CPU/PE is
actually a thread. Given that the MPIDR_MT bit may not
represent this information consistently on homogeneous machines
we should prefer the PPTT flag if its available.
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
arch/arm64/kernel/topology.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 0825c4a856e3..cbbedb53cf06 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -346,11 +346,9 @@ void remove_cpu_topology(unsigned int cpu)
*/
static int __init parse_acpi_topology(void)
{
- bool is_threaded;
+ int is_threaded;
int cpu, topology_id;
- is_threaded = read_cpuid_mpidr() & MPIDR_MT_BITMASK;
-
for_each_possible_cpu(cpu) {
int i, cache_id;
@@ -358,6 +356,10 @@ static int __init parse_acpi_topology(void)
if (topology_id < 0)
return topology_id;
+ is_threaded = acpi_pptt_cpu_is_thread(cpu);
+ if (is_threaded < 0)
+ is_threaded = read_cpuid_mpidr() & MPIDR_MT_BITMASK;
+
if (is_threaded) {
cpu_topology[cpu].thread_id = topology_id;
topology_id = find_acpi_cpu_topology(cpu, 1);
--
2.21.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag
2019-06-14 22:31 ` [PATCH v2 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag Jeremy Linton
@ 2019-06-17 12:34 ` Valentin Schneider
2019-06-18 14:21 ` Jeremy Linton
2019-06-25 15:20 ` Sudeep Holla
0 siblings, 2 replies; 11+ messages in thread
From: Valentin Schneider @ 2019-06-17 12:34 UTC (permalink / raw)
To: Jeremy Linton, linux-arm-kernel
Cc: linux-acpi, linux-kernel, catalin.marinas, will.deacon, rjw,
sudeep.holla, lenb
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)
[...]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag
2019-06-17 12:34 ` Valentin Schneider
@ 2019-06-18 14:21 ` Jeremy Linton
2019-06-18 14:40 ` Valentin Schneider
2019-06-25 15:20 ` Sudeep Holla
1 sibling, 1 reply; 11+ messages in thread
From: Jeremy Linton @ 2019-06-18 14:21 UTC (permalink / raw)
To: Valentin Schneider, linux-arm-kernel
Cc: linux-acpi, linux-kernel, catalin.marinas, will.deacon, rjw,
sudeep.holla, lenb
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)
> [...]
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag
2019-06-18 14:21 ` Jeremy Linton
@ 2019-06-18 14:40 ` Valentin Schneider
2019-06-18 17:23 ` John Garry
0 siblings, 1 reply; 11+ messages in thread
From: Valentin Schneider @ 2019-06-18 14:40 UTC (permalink / raw)
To: Jeremy Linton, linux-arm-kernel
Cc: linux-acpi, linux-kernel, catalin.marinas, will.deacon, rjw,
sudeep.holla, lenb
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.
[...]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag
2019-06-18 14:40 ` Valentin Schneider
@ 2019-06-18 17:23 ` John Garry
2019-06-18 21:28 ` Jeremy Linton
0 siblings, 1 reply; 11+ messages in thread
From: John Garry @ 2019-06-18 17:23 UTC (permalink / raw)
To: Valentin Schneider, Jeremy Linton, linux-arm-kernel
Cc: linux-acpi, linux-kernel, catalin.marinas, will.deacon, rjw,
sudeep.holla, lenb
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
> [...]
>
> .
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag
2019-06-18 17:23 ` John Garry
@ 2019-06-18 21:28 ` Jeremy Linton
2019-06-19 9:15 ` John Garry
0 siblings, 1 reply; 11+ messages in thread
From: Jeremy Linton @ 2019-06-18 21:28 UTC (permalink / raw)
To: John Garry, Valentin Schneider, linux-arm-kernel
Cc: linux-acpi, linux-kernel, catalin.marinas, will.deacon, rjw,
sudeep.holla, lenb
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
>
>> [...]
>>
>> .
>>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag
2019-06-18 21:28 ` Jeremy Linton
@ 2019-06-19 9:15 ` John Garry
2019-06-28 15:21 ` Jeremy Linton
0 siblings, 1 reply; 11+ messages in thread
From: John Garry @ 2019-06-19 9:15 UTC (permalink / raw)
To: Jeremy Linton, Valentin Schneider, linux-arm-kernel
Cc: linux-acpi, linux-kernel, catalin.marinas, will.deacon, rjw,
sudeep.holla, lenb
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
>>
>>> [...]
>>>
>>> .
>>>
>>
>>
>
>
> .
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag
2019-06-17 12:34 ` Valentin Schneider
2019-06-18 14:21 ` Jeremy Linton
@ 2019-06-25 15:20 ` Sudeep Holla
1 sibling, 0 replies; 11+ messages in thread
From: Sudeep Holla @ 2019-06-25 15:20 UTC (permalink / raw)
To: Valentin Schneider
Cc: Jeremy Linton, linux-arm-kernel, linux-acpi, linux-kernel,
catalin.marinas, will.deacon, rjw, lenb, Sudeep Holla
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag
2019-06-19 9:15 ` John Garry
@ 2019-06-28 15:21 ` Jeremy Linton
0 siblings, 0 replies; 11+ messages in thread
From: Jeremy Linton @ 2019-06-28 15:21 UTC (permalink / raw)
To: John Garry, Valentin Schneider, linux-arm-kernel
Cc: linux-acpi, linux-kernel, catalin.marinas, will.deacon, rjw,
sudeep.holla, lenb
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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-06-28 15:21 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-14 22:31 [PATCH v2 0/2] arm64/PPTT ACPI 6.3 thread flag support Jeremy Linton
2019-06-14 22:31 ` [PATCH v2 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag Jeremy Linton
2019-06-17 12:34 ` Valentin Schneider
2019-06-18 14:21 ` Jeremy Linton
2019-06-18 14:40 ` Valentin Schneider
2019-06-18 17:23 ` John Garry
2019-06-18 21:28 ` Jeremy Linton
2019-06-19 9:15 ` John Garry
2019-06-28 15:21 ` Jeremy Linton
2019-06-25 15:20 ` Sudeep Holla
2019-06-14 22:31 ` [PATCH v2 2/2] arm64: topology: Use PPTT to determine if PE is a thread Jeremy Linton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).