Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Tomasz Nowicki]

Hi John,

On 19.10.2017 12:25, John Garry wrote:

On 19/10/2017 06:18, Tomasz Nowicki wrote:


Summary:

I'm not at all happy with this specification's attempt to leave out
pieces of information which make parsing things more deterministic. In
this case I'm happy to demote the message level, but not remove it
entirely but I do think the obvious case you list shouldn't be the
default one.

Lastly:

I'm assuming the final result is that the table is actually being
parsed correctly despite the ugly message?


Indeed, the ThunderX2 PPTT table is being parsed so that topology shown
in lstopo and lscpu is correct.


Hi Tomasz,

Can you share the lscpu output? Does it have cluster info? I did not 
think that lscpu has a concept of clustering.


I would say that the per-cpu cluster index sysfs entry needs be added to 
drivers/base/arch_topology.c (and other appropiate code under 
GENERIC_ARCH_TOPOLOGY) to support this.


Here is what I get:

tn@val2-11 [~]$ lscpu -ap
# The following is the parsable format, which can be fed to other
# programs. Each different item in every column has an unique ID
# starting from zero.
# CPU,Core,Socket,Node,,L1d,L1i,L2,L3
[...]
1,0,0,0,,0,0,0,0
[...]

so yes, no cluster info.

Thanks,
Tomasz

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Tomasz Nowicki]

Hi John,

On 19.10.2017 12:25, John Garry wrote:

On 19/10/2017 06:18, Tomasz Nowicki wrote:


Summary:

I'm not at all happy with this specification's attempt to leave out
pieces of information which make parsing things more deterministic. In
this case I'm happy to demote the message level, but not remove it
entirely but I do think the obvious case you list shouldn't be the
default one.

Lastly:

I'm assuming the final result is that the table is actually being
parsed correctly despite the ugly message?


Indeed, the ThunderX2 PPTT table is being parsed so that topology shown
in lstopo and lscpu is correct.


Hi Tomasz,

Can you share the lscpu output? Does it have cluster info? I did not 
think that lscpu has a concept of clustering.


I would say that the per-cpu cluster index sysfs entry needs be added to 
drivers/base/arch_topology.c (and other appropiate code under 
GENERIC_ARCH_TOPOLOGY) to support this.


Here is what I get:

tn@val2-11 [~]$ lscpu -ap
# The following is the parsable format, which can be fed to other
# programs. Each different item in every column has an unique ID
# starting from zero.
# CPU,Core,Socket,Node,,L1d,L1i,L2,L3
[...]
1,0,0,0,,0,0,0,0
[...]

so yes, no cluster info.

Thanks,
Tomasz

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Jeremy Linton]

Hi,

On 10/20/2017 02:53 PM, Christ, Austin wrote:

Hey Jeremy,

Quick comment below.

On 10/12/2017 1:48 PM, Jeremy Linton wrote:

+static int topology_setup_acpi_cpu(struct acpi_table_header *table,
+    unsigned int cpu, int level)
+{
+    struct acpi_pptt_processor *cpu_node;
+    u32 acpi_cpu_id = acpi_cpu_get_madt_gicc(cpu)->uid;


This lookup for the acpi id is architecture dependent. Can you use a 
function that would work for any user of PPTT and MADT? It may require 
writing and exporting the inverse lookup of the function 
acpi_get_cpuid() which is exported from processor_core.c


Sure, I was actually thinking about just passing it into the function, 
so it becomes the responsibility of the caller to do the platform 
specific reverse lookup.





+
+    cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
+    if (cpu_node) {
+    cpu_node = acpi_find_processor_package_id(table, cpu_node, 
level);

+    /* Only the first level has a guaranteed id */
+    if (level == 0)
+    return cpu_node->acpi_processor_id;
+    return (int)((u8 *)cpu_node - (u8 *)table);
+    }
+    pr_err_once("PPTT table found, but unable to locate core for %d\n",
+    cpu);
+    return -ENOENT;
+}

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Jeremy Linton]

Hi,

On 10/20/2017 02:53 PM, Christ, Austin wrote:

Hey Jeremy,

Quick comment below.

On 10/12/2017 1:48 PM, Jeremy Linton wrote:

+static int topology_setup_acpi_cpu(struct acpi_table_header *table,
+    unsigned int cpu, int level)
+{
+    struct acpi_pptt_processor *cpu_node;
+    u32 acpi_cpu_id = acpi_cpu_get_madt_gicc(cpu)->uid;


This lookup for the acpi id is architecture dependent. Can you use a 
function that would work for any user of PPTT and MADT? It may require 
writing and exporting the inverse lookup of the function 
acpi_get_cpuid() which is exported from processor_core.c


Sure, I was actually thinking about just passing it into the function, 
so it becomes the responsibility of the caller to do the platform 
specific reverse lookup.





+
+    cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
+    if (cpu_node) {
+    cpu_node = acpi_find_processor_package_id(table, cpu_node, 
level);

+    /* Only the first level has a guaranteed id */
+    if (level == 0)
+    return cpu_node->acpi_processor_id;
+    return (int)((u8 *)cpu_node - (u8 *)table);
+    }
+    pr_err_once("PPTT table found, but unable to locate core for %d\n",
+    cpu);
+    return -ENOENT;
+}

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Christ, Austin]

Hey Jeremy,

Quick comment below.

On 10/12/2017 1:48 PM, Jeremy Linton wrote:

+static int topology_setup_acpi_cpu(struct acpi_table_header *table,
+   unsigned int cpu, int level)
+{
+   struct acpi_pptt_processor *cpu_node;
+   u32 acpi_cpu_id = acpi_cpu_get_madt_gicc(cpu)->uid;


This lookup for the acpi id is architecture dependent. Can you use a 
function that would work for any user of PPTT and MADT? It may require 
writing and exporting the inverse lookup of the function 
acpi_get_cpuid() which is exported from processor_core.c



+
+   cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
+   if (cpu_node) {
+   cpu_node = acpi_find_processor_package_id(table, cpu_node, 
level);
+   /* Only the first level has a guaranteed id */
+   if (level == 0)
+   return cpu_node->acpi_processor_id;
+   return (int)((u8 *)cpu_node - (u8 *)table);
+   }
+   pr_err_once("PPTT table found, but unable to locate core for %d\n",
+   cpu);
+   return -ENOENT;
+}


--
Austin Christ
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.

Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Christ, Austin]

Hey Jeremy,

Quick comment below.

On 10/12/2017 1:48 PM, Jeremy Linton wrote:

+static int topology_setup_acpi_cpu(struct acpi_table_header *table,
+   unsigned int cpu, int level)
+{
+   struct acpi_pptt_processor *cpu_node;
+   u32 acpi_cpu_id = acpi_cpu_get_madt_gicc(cpu)->uid;


This lookup for the acpi id is architecture dependent. Can you use a 
function that would work for any user of PPTT and MADT? It may require 
writing and exporting the inverse lookup of the function 
acpi_get_cpuid() which is exported from processor_core.c



+
+   cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
+   if (cpu_node) {
+   cpu_node = acpi_find_processor_package_id(table, cpu_node, 
level);
+   /* Only the first level has a guaranteed id */
+   if (level == 0)
+   return cpu_node->acpi_processor_id;
+   return (int)((u8 *)cpu_node - (u8 *)table);
+   }
+   pr_err_once("PPTT table found, but unable to locate core for %d\n",
+   cpu);
+   return -ENOENT;
+}


--
Austin Christ
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.

Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Lorenzo Pieralisi]

On Thu, Oct 19, 2017 at 10:43:46AM -0500, Jeremy Linton wrote:
> On 10/19/2017 05:22 AM, Lorenzo Pieralisi wrote:
> >On Thu, Oct 12, 2017 at 02:48:50PM -0500, Jeremy Linton wrote:
> >>ACPI 6.2 adds a new table, which describes how processing units
> >>are related to each other in tree like fashion. Caches are
> >>also sprinkled throughout the tree and describe the properties
> >>of the caches in relation to other caches and processing units.
> >>
> >>Add the code to parse the cache hierarchy and report the total
> >>number of levels of cache for a given core using
> >>acpi_find_last_cache_level() as well as fill out the individual
> >>cores cache information with cache_setup_acpi() once the
> >>cpu_cacheinfo structure has been populated by the arch specific
> >>code.
> >>
> >>Further, report peers in the topology using setup_acpi_cpu_topology()
> >>to report a unique ID for each processing unit at a given level
> >>in the tree. These unique id's can then be used to match related
> >>processing units which exist as threads, COD (clusters
> >>on die), within a given package, etc.
> >
> >I think this patch should be split ((1) topology (2) cache), it is doing
> >too much which makes it hard to review.
> 
> If you look at the RFC, it only did cache parsing, the topology
> changes were added for v1. The cache bits are the ugly parts because
> they are walking up/down both the node tree, as well as the cache
> tree's attached to the nodes during the walk. Once that was in the
> place the addition of the cpu topology was trivial. But, trying to
> understand the cpu topology without first understanding the weird
> stuff done for the cache topology might not be the right way to
> approach this code.

Topology and cache bindings parsing seem decoupled to me:

cache_setup_acpi(cpu)
setup_acpi_cpu_topology(cpu, level)

I mentioned that because it can simplify review (and merging)
of this series.

> >
> >[...]
> >
> >>+/* determine if the given node is a leaf node */
> >>+static int acpi_pptt_leaf_node(struct acpi_table_header *table_hdr,
> >>+  struct acpi_pptt_processor *node)
> >>+{
> >>+   struct acpi_subtable_header *entry;
> >>+   unsigned long table_end;
> >>+   u32 node_entry;
> >>+   struct acpi_pptt_processor *cpu_node;
> >>+
> >>+   table_end = (unsigned long)table_hdr + table_hdr->length;
> >>+   node_entry = (u32)((u8 *)node - (u8 *)table_hdr);
> >>+   entry = (struct acpi_subtable_header *)((u8 *)table_hdr +
> >>+   sizeof(struct acpi_table_pptt));
> >>+
> >>+   while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < 
> >>table_end) {
> >>+   cpu_node = (struct acpi_pptt_processor *)entry;
> >>+   if ((entry->type == ACPI_PPTT_TYPE_PROCESSOR) &&
> >>+   (cpu_node->parent == node_entry))
> >>+   return 0;
> >>+   entry = (struct acpi_subtable_header *)((u8 *)entry + 
> >>entry->length);
> >>+   }
> >
> >A leaf node is a node with a valid acpi_id corresponding to an MADT
> >entry, right ? By the way, is this function really needed ?
> 
> Yes, because the only way to determine if it is a leaf node is to
> see if there are any references to it elsewhere in the table because
> the nodes point towards the root of the tree (rather than the other
> way).

The question is whether we need to know a node is a leaf, see below.

> This piece was the primary change for v1->v2.
> 
> >
> >>+   return 1;
> >>+}
> >>+
> >>+/*
> >>+ * Find the subtable entry describing the provided processor
> >>+ */
> >>+static struct acpi_pptt_processor *acpi_find_processor_node(
> >>+   struct acpi_table_header *table_hdr,
> >>+   u32 acpi_cpu_id)
> >>+{
> >>+   struct acpi_subtable_header *entry;
> >>+   unsigned long table_end;
> >>+   struct acpi_pptt_processor *cpu_node;
> >>+
> >>+   table_end = (unsigned long)table_hdr + table_hdr->length;
> >>+   entry = (struct acpi_subtable_header *)((u8 *)table_hdr +
> >>+   sizeof(struct acpi_table_pptt));
> >>+
> >>+   /* find the processor structure associated with this cpuid */
> >>+   while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < 
> >>table_end) {
> >>+   cpu_node = (struct acpi_pptt_processor *)entry;
> >>+
> >>+   if ((entry->type == ACPI_PPTT_TYPE_PROCESSOR) &&
> >>+   acpi_pptt_leaf_node(table_hdr, cpu_node)) {
> >
> >Is the leaf node check necessary ? Or you just need to check the
> >ACPI Processor ID valid flag (as discussed offline) ?
> 
> The valid flag doesn't mean anything for the leaf nodes, so its the
> only correct way of determining if the node _might_ have a valid
> madt/acpi ID. This actually should have the acpi_cpu_id checked as
> part of the if statement and the leaf node check below because doing
> it this way makes this parse n^2 instead of 2n. Of course in my
> mind, checking the id before we know it might be valid is backwards
> of the "logical" 

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Lorenzo Pieralisi]

On Thu, Oct 19, 2017 at 10:43:46AM -0500, Jeremy Linton wrote:
> On 10/19/2017 05:22 AM, Lorenzo Pieralisi wrote:
> >On Thu, Oct 12, 2017 at 02:48:50PM -0500, Jeremy Linton wrote:
> >>ACPI 6.2 adds a new table, which describes how processing units
> >>are related to each other in tree like fashion. Caches are
> >>also sprinkled throughout the tree and describe the properties
> >>of the caches in relation to other caches and processing units.
> >>
> >>Add the code to parse the cache hierarchy and report the total
> >>number of levels of cache for a given core using
> >>acpi_find_last_cache_level() as well as fill out the individual
> >>cores cache information with cache_setup_acpi() once the
> >>cpu_cacheinfo structure has been populated by the arch specific
> >>code.
> >>
> >>Further, report peers in the topology using setup_acpi_cpu_topology()
> >>to report a unique ID for each processing unit at a given level
> >>in the tree. These unique id's can then be used to match related
> >>processing units which exist as threads, COD (clusters
> >>on die), within a given package, etc.
> >
> >I think this patch should be split ((1) topology (2) cache), it is doing
> >too much which makes it hard to review.
> 
> If you look at the RFC, it only did cache parsing, the topology
> changes were added for v1. The cache bits are the ugly parts because
> they are walking up/down both the node tree, as well as the cache
> tree's attached to the nodes during the walk. Once that was in the
> place the addition of the cpu topology was trivial. But, trying to
> understand the cpu topology without first understanding the weird
> stuff done for the cache topology might not be the right way to
> approach this code.

Topology and cache bindings parsing seem decoupled to me:

cache_setup_acpi(cpu)
setup_acpi_cpu_topology(cpu, level)

I mentioned that because it can simplify review (and merging)
of this series.

> >
> >[...]
> >
> >>+/* determine if the given node is a leaf node */
> >>+static int acpi_pptt_leaf_node(struct acpi_table_header *table_hdr,
> >>+  struct acpi_pptt_processor *node)
> >>+{
> >>+   struct acpi_subtable_header *entry;
> >>+   unsigned long table_end;
> >>+   u32 node_entry;
> >>+   struct acpi_pptt_processor *cpu_node;
> >>+
> >>+   table_end = (unsigned long)table_hdr + table_hdr->length;
> >>+   node_entry = (u32)((u8 *)node - (u8 *)table_hdr);
> >>+   entry = (struct acpi_subtable_header *)((u8 *)table_hdr +
> >>+   sizeof(struct acpi_table_pptt));
> >>+
> >>+   while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < 
> >>table_end) {
> >>+   cpu_node = (struct acpi_pptt_processor *)entry;
> >>+   if ((entry->type == ACPI_PPTT_TYPE_PROCESSOR) &&
> >>+   (cpu_node->parent == node_entry))
> >>+   return 0;
> >>+   entry = (struct acpi_subtable_header *)((u8 *)entry + 
> >>entry->length);
> >>+   }
> >
> >A leaf node is a node with a valid acpi_id corresponding to an MADT
> >entry, right ? By the way, is this function really needed ?
> 
> Yes, because the only way to determine if it is a leaf node is to
> see if there are any references to it elsewhere in the table because
> the nodes point towards the root of the tree (rather than the other
> way).

The question is whether we need to know a node is a leaf, see below.

> This piece was the primary change for v1->v2.
> 
> >
> >>+   return 1;
> >>+}
> >>+
> >>+/*
> >>+ * Find the subtable entry describing the provided processor
> >>+ */
> >>+static struct acpi_pptt_processor *acpi_find_processor_node(
> >>+   struct acpi_table_header *table_hdr,
> >>+   u32 acpi_cpu_id)
> >>+{
> >>+   struct acpi_subtable_header *entry;
> >>+   unsigned long table_end;
> >>+   struct acpi_pptt_processor *cpu_node;
> >>+
> >>+   table_end = (unsigned long)table_hdr + table_hdr->length;
> >>+   entry = (struct acpi_subtable_header *)((u8 *)table_hdr +
> >>+   sizeof(struct acpi_table_pptt));
> >>+
> >>+   /* find the processor structure associated with this cpuid */
> >>+   while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < 
> >>table_end) {
> >>+   cpu_node = (struct acpi_pptt_processor *)entry;
> >>+
> >>+   if ((entry->type == ACPI_PPTT_TYPE_PROCESSOR) &&
> >>+   acpi_pptt_leaf_node(table_hdr, cpu_node)) {
> >
> >Is the leaf node check necessary ? Or you just need to check the
> >ACPI Processor ID valid flag (as discussed offline) ?
> 
> The valid flag doesn't mean anything for the leaf nodes, so its the
> only correct way of determining if the node _might_ have a valid
> madt/acpi ID. This actually should have the acpi_cpu_id checked as
> part of the if statement and the leaf node check below because doing
> it this way makes this parse n^2 instead of 2n. Of course in my
> mind, checking the id before we know it might be valid is backwards
> of the "logical" 

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Jeremy Linton]

On 10/19/2017 05:22 AM, Lorenzo Pieralisi wrote:

On Thu, Oct 12, 2017 at 02:48:50PM -0500, Jeremy Linton wrote:

ACPI 6.2 adds a new table, which describes how processing units
are related to each other in tree like fashion. Caches are
also sprinkled throughout the tree and describe the properties
of the caches in relation to other caches and processing units.

Add the code to parse the cache hierarchy and report the total
number of levels of cache for a given core using
acpi_find_last_cache_level() as well as fill out the individual
cores cache information with cache_setup_acpi() once the
cpu_cacheinfo structure has been populated by the arch specific
code.

Further, report peers in the topology using setup_acpi_cpu_topology()
to report a unique ID for each processing unit at a given level
in the tree. These unique id's can then be used to match related
processing units which exist as threads, COD (clusters
on die), within a given package, etc.


I think this patch should be split ((1) topology (2) cache), it is doing
too much which makes it hard to review.


If you look at the RFC, it only did cache parsing, the topology changes 
were added for v1. The cache bits are the ugly parts because they are 
walking up/down both the node tree, as well as the cache tree's attached 
to the nodes during the walk. Once that was in the place the addition of 
the cpu topology was trivial. But, trying to understand the cpu topology 
without first understanding the weird stuff done for the cache topology 
might not be the right way to approach this code.




[...]


+/* determine if the given node is a leaf node */
+static int acpi_pptt_leaf_node(struct acpi_table_header *table_hdr,
+  struct acpi_pptt_processor *node)
+{
+   struct acpi_subtable_header *entry;
+   unsigned long table_end;
+   u32 node_entry;
+   struct acpi_pptt_processor *cpu_node;
+
+   table_end = (unsigned long)table_hdr + table_hdr->length;
+   node_entry = (u32)((u8 *)node - (u8 *)table_hdr);
+   entry = (struct acpi_subtable_header *)((u8 *)table_hdr +
+   sizeof(struct acpi_table_pptt));
+
+   while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < 
table_end) {
+   cpu_node = (struct acpi_pptt_processor *)entry;
+   if ((entry->type == ACPI_PPTT_TYPE_PROCESSOR) &&
+   (cpu_node->parent == node_entry))
+   return 0;
+   entry = (struct acpi_subtable_header *)((u8 *)entry + 
entry->length);
+   }


A leaf node is a node with a valid acpi_id corresponding to an MADT
entry, right ? By the way, is this function really needed ?


Yes, because the only way to determine if it is a leaf node is to see if 
there are any references to it elsewhere in the table because the nodes 
point towards the root of the tree (rather than the other way).


This piece was the primary change for v1->v2.




+   return 1;
+}
+
+/*
+ * Find the subtable entry describing the provided processor
+ */
+static struct acpi_pptt_processor *acpi_find_processor_node(
+   struct acpi_table_header *table_hdr,
+   u32 acpi_cpu_id)
+{
+   struct acpi_subtable_header *entry;
+   unsigned long table_end;
+   struct acpi_pptt_processor *cpu_node;
+
+   table_end = (unsigned long)table_hdr + table_hdr->length;
+   entry = (struct acpi_subtable_header *)((u8 *)table_hdr +
+   sizeof(struct acpi_table_pptt));
+
+   /* find the processor structure associated with this cpuid */
+   while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < 
table_end) {
+   cpu_node = (struct acpi_pptt_processor *)entry;
+
+   if ((entry->type == ACPI_PPTT_TYPE_PROCESSOR) &&
+   acpi_pptt_leaf_node(table_hdr, cpu_node)) {


Is the leaf node check necessary ? Or you just need to check the
ACPI Processor ID valid flag (as discussed offline) ?


The valid flag doesn't mean anything for the leaf nodes, so its the only 
correct way of determining if the node _might_ have a valid madt/acpi 
ID. This actually should have the acpi_cpu_id checked as part of the if 
statement and the leaf node check below because doing it this way makes 
this parse n^2 instead of 2n. Of course in my mind, checking the id 
before we know it might be valid is backwards of the "logical" way to do it.





+   pr_debug("checking phy_cpu_id %d against acpi id %d\n",
+acpi_cpu_id, cpu_node->acpi_processor_id);


Side note: I'd question (some of) these pr_debug() messages


+   if (acpi_cpu_id == cpu_node->acpi_processor_id) {
+   /* found the correct entry */
+   pr_debug("match found!\n");


Like this one for instance.


This one is a bit redundant, but I come from the school that I want 

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Jeremy Linton]

On 10/19/2017 05:22 AM, Lorenzo Pieralisi wrote:

On Thu, Oct 12, 2017 at 02:48:50PM -0500, Jeremy Linton wrote:

ACPI 6.2 adds a new table, which describes how processing units
are related to each other in tree like fashion. Caches are
also sprinkled throughout the tree and describe the properties
of the caches in relation to other caches and processing units.

Add the code to parse the cache hierarchy and report the total
number of levels of cache for a given core using
acpi_find_last_cache_level() as well as fill out the individual
cores cache information with cache_setup_acpi() once the
cpu_cacheinfo structure has been populated by the arch specific
code.

Further, report peers in the topology using setup_acpi_cpu_topology()
to report a unique ID for each processing unit at a given level
in the tree. These unique id's can then be used to match related
processing units which exist as threads, COD (clusters
on die), within a given package, etc.


I think this patch should be split ((1) topology (2) cache), it is doing
too much which makes it hard to review.


If you look at the RFC, it only did cache parsing, the topology changes 
were added for v1. The cache bits are the ugly parts because they are 
walking up/down both the node tree, as well as the cache tree's attached 
to the nodes during the walk. Once that was in the place the addition of 
the cpu topology was trivial. But, trying to understand the cpu topology 
without first understanding the weird stuff done for the cache topology 
might not be the right way to approach this code.




[...]


+/* determine if the given node is a leaf node */
+static int acpi_pptt_leaf_node(struct acpi_table_header *table_hdr,
+  struct acpi_pptt_processor *node)
+{
+   struct acpi_subtable_header *entry;
+   unsigned long table_end;
+   u32 node_entry;
+   struct acpi_pptt_processor *cpu_node;
+
+   table_end = (unsigned long)table_hdr + table_hdr->length;
+   node_entry = (u32)((u8 *)node - (u8 *)table_hdr);
+   entry = (struct acpi_subtable_header *)((u8 *)table_hdr +
+   sizeof(struct acpi_table_pptt));
+
+   while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < 
table_end) {
+   cpu_node = (struct acpi_pptt_processor *)entry;
+   if ((entry->type == ACPI_PPTT_TYPE_PROCESSOR) &&
+   (cpu_node->parent == node_entry))
+   return 0;
+   entry = (struct acpi_subtable_header *)((u8 *)entry + 
entry->length);
+   }


A leaf node is a node with a valid acpi_id corresponding to an MADT
entry, right ? By the way, is this function really needed ?


Yes, because the only way to determine if it is a leaf node is to see if 
there are any references to it elsewhere in the table because the nodes 
point towards the root of the tree (rather than the other way).


This piece was the primary change for v1->v2.




+   return 1;
+}
+
+/*
+ * Find the subtable entry describing the provided processor
+ */
+static struct acpi_pptt_processor *acpi_find_processor_node(
+   struct acpi_table_header *table_hdr,
+   u32 acpi_cpu_id)
+{
+   struct acpi_subtable_header *entry;
+   unsigned long table_end;
+   struct acpi_pptt_processor *cpu_node;
+
+   table_end = (unsigned long)table_hdr + table_hdr->length;
+   entry = (struct acpi_subtable_header *)((u8 *)table_hdr +
+   sizeof(struct acpi_table_pptt));
+
+   /* find the processor structure associated with this cpuid */
+   while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < 
table_end) {
+   cpu_node = (struct acpi_pptt_processor *)entry;
+
+   if ((entry->type == ACPI_PPTT_TYPE_PROCESSOR) &&
+   acpi_pptt_leaf_node(table_hdr, cpu_node)) {


Is the leaf node check necessary ? Or you just need to check the
ACPI Processor ID valid flag (as discussed offline) ?


The valid flag doesn't mean anything for the leaf nodes, so its the only 
correct way of determining if the node _might_ have a valid madt/acpi 
ID. This actually should have the acpi_cpu_id checked as part of the if 
statement and the leaf node check below because doing it this way makes 
this parse n^2 instead of 2n. Of course in my mind, checking the id 
before we know it might be valid is backwards of the "logical" way to do it.





+   pr_debug("checking phy_cpu_id %d against acpi id %d\n",
+acpi_cpu_id, cpu_node->acpi_processor_id);


Side note: I'd question (some of) these pr_debug() messages


+   if (acpi_cpu_id == cpu_node->acpi_processor_id) {
+   /* found the correct entry */
+   pr_debug("match found!\n");


Like this one for instance.


This one is a bit redundant, but I come from the school that I want 

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Jeremy Linton]

Hi,


On 10/19/2017 12:18 AM, Tomasz Nowicki wrote:

On 18.10.2017 19:30, Jeremy Linton wrote:

On 10/18/2017 05:24 AM, Tomasz Nowicki wrote:

On 18.10.2017 07:39, Tomasz Nowicki wrote:

On 17.10.2017 17:22, Jeremy Linton wrote:

On 10/17/2017 08:25 AM, Tomasz Nowicki wrote:

On 12.10.2017 21:48, Jeremy Linton wrote:

(trimming)

+    if (*found != NULL)
+    pr_err("Found duplicate cache level/type unable 
to determine uniqueness\n");


Actually I still see this error messages in my dmesg. It is because 
the following ThunderX2 per-core L1 and L2 cache hierarchy:


Core
  --
|  |
| L1i -    |
| |    |
|  L2  |
| |    |
| L1d -    |
|  |
  --

In this case we have two paths which lead to L2 cache and hit above 
case. Is it really error case?


No, but its not deterministic unless we mark the node, which doesn't 
solve the problem of a table constructed like


L1i->L2 (unified)
L1d->L2 (unified)

or various other structures which aren't disallowed by the spec and 
have non-deterministic real world meanings, anymore than constructing 
the table like:


L1i
Lid->L2(unified)

which I tend to prefer because with a structuring like that it can be 
deterministic (and in a way actually represents the non-coherent 
behavior of (most?) ARM64 core's i-caches, as could be argued the 
first example if the allocation policies are varied between the L2 
nodes).


The really ugly bits here happen if you add another layer:

L1i->L2i-L3
L1d--^

which is why I made that an error message, not including the fact that 
since the levels aren't tagged the numbering and meaning isn't clear.


(the L1i in the above example might be better called an L0i to avoid 
throwing off the reset of the hierarchy numbering, also so it could be 
ignored).


Summary:

I'm not at all happy with this specification's attempt to leave out 
pieces of information which make parsing things more deterministic. In 
this case I'm happy to demote the message level, but not remove it 
entirely but I do think the obvious case you list shouldn't be the 
default one.


Lastly:

I'm assuming the final result is that the table is actually being 
parsed correctly despite the ugly message?


Indeed, the ThunderX2 PPTT table is being parsed so that topology shown 
in lstopo and lscpu is correct.


Great.

Also, I think this is a better change:

 if ((*found != NULL) && (*found != cache))
 pr_err("Found duplicate cache level/type unable to determine 
uniqueness\n");


Which if its a duplicate node/type at the given level the message is 
just suppressed. It will of course still trigger in cases like:


L1d->L2
l1i->L2

or other odd cases.

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Jeremy Linton]

Hi,


On 10/19/2017 12:18 AM, Tomasz Nowicki wrote:

On 18.10.2017 19:30, Jeremy Linton wrote:

On 10/18/2017 05:24 AM, Tomasz Nowicki wrote:

On 18.10.2017 07:39, Tomasz Nowicki wrote:

On 17.10.2017 17:22, Jeremy Linton wrote:

On 10/17/2017 08:25 AM, Tomasz Nowicki wrote:

On 12.10.2017 21:48, Jeremy Linton wrote:

(trimming)

+    if (*found != NULL)
+    pr_err("Found duplicate cache level/type unable 
to determine uniqueness\n");


Actually I still see this error messages in my dmesg. It is because 
the following ThunderX2 per-core L1 and L2 cache hierarchy:


Core
  --
|  |
| L1i -    |
| |    |
|  L2  |
| |    |
| L1d -    |
|  |
  --

In this case we have two paths which lead to L2 cache and hit above 
case. Is it really error case?


No, but its not deterministic unless we mark the node, which doesn't 
solve the problem of a table constructed like


L1i->L2 (unified)
L1d->L2 (unified)

or various other structures which aren't disallowed by the spec and 
have non-deterministic real world meanings, anymore than constructing 
the table like:


L1i
Lid->L2(unified)

which I tend to prefer because with a structuring like that it can be 
deterministic (and in a way actually represents the non-coherent 
behavior of (most?) ARM64 core's i-caches, as could be argued the 
first example if the allocation policies are varied between the L2 
nodes).


The really ugly bits here happen if you add another layer:

L1i->L2i-L3
L1d--^

which is why I made that an error message, not including the fact that 
since the levels aren't tagged the numbering and meaning isn't clear.


(the L1i in the above example might be better called an L0i to avoid 
throwing off the reset of the hierarchy numbering, also so it could be 
ignored).


Summary:

I'm not at all happy with this specification's attempt to leave out 
pieces of information which make parsing things more deterministic. In 
this case I'm happy to demote the message level, but not remove it 
entirely but I do think the obvious case you list shouldn't be the 
default one.


Lastly:

I'm assuming the final result is that the table is actually being 
parsed correctly despite the ugly message?


Indeed, the ThunderX2 PPTT table is being parsed so that topology shown 
in lstopo and lscpu is correct.


Great.

Also, I think this is a better change:

 if ((*found != NULL) && (*found != cache))
 pr_err("Found duplicate cache level/type unable to determine 
uniqueness\n");


Which if its a duplicate node/type at the given level the message is 
just suppressed. It will of course still trigger in cases like:


L1d->L2
l1i->L2

or other odd cases.

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[John Garry]

On 19/10/2017 06:18, Tomasz Nowicki wrote:


Summary:

I'm not at all happy with this specification's attempt to leave out
pieces of information which make parsing things more deterministic. In
this case I'm happy to demote the message level, but not remove it
entirely but I do think the obvious case you list shouldn't be the
default one.

Lastly:

I'm assuming the final result is that the table is actually being
parsed correctly despite the ugly message?


Indeed, the ThunderX2 PPTT table is being parsed so that topology shown
in lstopo and lscpu is correct.


Hi Tomasz,

Can you share the lscpu output? Does it have cluster info? I did not 
think that lscpu has a concept of clustering.


I would say that the per-cpu cluster index sysfs entry needs be added to 
drivers/base/arch_topology.c (and other appropiate code under 
GENERIC_ARCH_TOPOLOGY) to support this.


Thanks,
John




Thanks,
Tomasz

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[John Garry]

On 19/10/2017 06:18, Tomasz Nowicki wrote:


Summary:

I'm not at all happy with this specification's attempt to leave out
pieces of information which make parsing things more deterministic. In
this case I'm happy to demote the message level, but not remove it
entirely but I do think the obvious case you list shouldn't be the
default one.

Lastly:

I'm assuming the final result is that the table is actually being
parsed correctly despite the ugly message?


Indeed, the ThunderX2 PPTT table is being parsed so that topology shown
in lstopo and lscpu is correct.


Hi Tomasz,

Can you share the lscpu output? Does it have cluster info? I did not 
think that lscpu has a concept of clustering.


I would say that the per-cpu cluster index sysfs entry needs be added to 
drivers/base/arch_topology.c (and other appropiate code under 
GENERIC_ARCH_TOPOLOGY) to support this.


Thanks,
John




Thanks,
Tomasz

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Lorenzo Pieralisi]

On Thu, Oct 12, 2017 at 02:48:50PM -0500, Jeremy Linton wrote:
> ACPI 6.2 adds a new table, which describes how processing units
> are related to each other in tree like fashion. Caches are
> also sprinkled throughout the tree and describe the properties
> of the caches in relation to other caches and processing units.
> 
> Add the code to parse the cache hierarchy and report the total
> number of levels of cache for a given core using
> acpi_find_last_cache_level() as well as fill out the individual
> cores cache information with cache_setup_acpi() once the
> cpu_cacheinfo structure has been populated by the arch specific
> code.
> 
> Further, report peers in the topology using setup_acpi_cpu_topology()
> to report a unique ID for each processing unit at a given level
> in the tree. These unique id's can then be used to match related
> processing units which exist as threads, COD (clusters
> on die), within a given package, etc.

I think this patch should be split ((1) topology (2) cache), it is doing
too much which makes it hard to review.

[...]

> +/* determine if the given node is a leaf node */
> +static int acpi_pptt_leaf_node(struct acpi_table_header *table_hdr,
> +struct acpi_pptt_processor *node)
> +{
> + struct acpi_subtable_header *entry;
> + unsigned long table_end;
> + u32 node_entry;
> + struct acpi_pptt_processor *cpu_node;
> +
> + table_end = (unsigned long)table_hdr + table_hdr->length;
> + node_entry = (u32)((u8 *)node - (u8 *)table_hdr);
> + entry = (struct acpi_subtable_header *)((u8 *)table_hdr +
> + sizeof(struct acpi_table_pptt));
> +
> + while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < 
> table_end) {
> + cpu_node = (struct acpi_pptt_processor *)entry;
> + if ((entry->type == ACPI_PPTT_TYPE_PROCESSOR) &&
> + (cpu_node->parent == node_entry))
> + return 0;
> + entry = (struct acpi_subtable_header *)((u8 *)entry + 
> entry->length);
> + }

A leaf node is a node with a valid acpi_id corresponding to an MADT
entry, right ? By the way, is this function really needed ?

> + return 1;
> +}
> +
> +/*
> + * Find the subtable entry describing the provided processor
> + */
> +static struct acpi_pptt_processor *acpi_find_processor_node(
> + struct acpi_table_header *table_hdr,
> + u32 acpi_cpu_id)
> +{
> + struct acpi_subtable_header *entry;
> + unsigned long table_end;
> + struct acpi_pptt_processor *cpu_node;
> +
> + table_end = (unsigned long)table_hdr + table_hdr->length;
> + entry = (struct acpi_subtable_header *)((u8 *)table_hdr +
> + sizeof(struct acpi_table_pptt));
> +
> + /* find the processor structure associated with this cpuid */
> + while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < 
> table_end) {
> + cpu_node = (struct acpi_pptt_processor *)entry;
> +
> + if ((entry->type == ACPI_PPTT_TYPE_PROCESSOR) &&
> + acpi_pptt_leaf_node(table_hdr, cpu_node)) {

Is the leaf node check necessary ? Or you just need to check the
ACPI Processor ID valid flag (as discussed offline) ?

> + pr_debug("checking phy_cpu_id %d against acpi id %d\n",
> +  acpi_cpu_id, cpu_node->acpi_processor_id);

Side note: I'd question (some of) these pr_debug() messages.

> + if (acpi_cpu_id == cpu_node->acpi_processor_id) {
> + /* found the correct entry */
> + pr_debug("match found!\n");

Like this one for instance.

> + return (struct acpi_pptt_processor *)entry;
> + }
> + }
> +
> + if (entry->length == 0) {
> + pr_err("Invalid zero length subtable\n");
> + break;
> + }

This should be moved at the beginning of the loop.

> + entry = (struct acpi_subtable_header *)
> + ((u8 *)entry + entry->length);
> + }
> +
> + return NULL;
> +}
> +
> +/*
> + * Given a acpi_pptt_processor node, walk up until we identify the
> + * package that the node is associated with or we run out of levels
> + * to request.
> + */
> +static struct acpi_pptt_processor *acpi_find_processor_package_id(
> + struct acpi_table_header *table_hdr,
> + struct acpi_pptt_processor *cpu,
> + int level)
> +{
> + struct acpi_pptt_processor *prev_node;
> +
> + while (cpu && level && !(cpu->flags & ACPI_PPTT_PHYSICAL_PACKAGE)) {

I really do not understand what ACPI_PPTT_PHYSICAL_PACKAGE means and
more importantly, how it is actually used in this code.

This function is used to get a topology id (that is just a number for
a given topology level) for a given level starting from a given leaf
node.

Why do we care at 

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Lorenzo Pieralisi]

On Thu, Oct 12, 2017 at 02:48:50PM -0500, Jeremy Linton wrote:
> ACPI 6.2 adds a new table, which describes how processing units
> are related to each other in tree like fashion. Caches are
> also sprinkled throughout the tree and describe the properties
> of the caches in relation to other caches and processing units.
> 
> Add the code to parse the cache hierarchy and report the total
> number of levels of cache for a given core using
> acpi_find_last_cache_level() as well as fill out the individual
> cores cache information with cache_setup_acpi() once the
> cpu_cacheinfo structure has been populated by the arch specific
> code.
> 
> Further, report peers in the topology using setup_acpi_cpu_topology()
> to report a unique ID for each processing unit at a given level
> in the tree. These unique id's can then be used to match related
> processing units which exist as threads, COD (clusters
> on die), within a given package, etc.

I think this patch should be split ((1) topology (2) cache), it is doing
too much which makes it hard to review.

[...]

> +/* determine if the given node is a leaf node */
> +static int acpi_pptt_leaf_node(struct acpi_table_header *table_hdr,
> +struct acpi_pptt_processor *node)
> +{
> + struct acpi_subtable_header *entry;
> + unsigned long table_end;
> + u32 node_entry;
> + struct acpi_pptt_processor *cpu_node;
> +
> + table_end = (unsigned long)table_hdr + table_hdr->length;
> + node_entry = (u32)((u8 *)node - (u8 *)table_hdr);
> + entry = (struct acpi_subtable_header *)((u8 *)table_hdr +
> + sizeof(struct acpi_table_pptt));
> +
> + while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < 
> table_end) {
> + cpu_node = (struct acpi_pptt_processor *)entry;
> + if ((entry->type == ACPI_PPTT_TYPE_PROCESSOR) &&
> + (cpu_node->parent == node_entry))
> + return 0;
> + entry = (struct acpi_subtable_header *)((u8 *)entry + 
> entry->length);
> + }

A leaf node is a node with a valid acpi_id corresponding to an MADT
entry, right ? By the way, is this function really needed ?

> + return 1;
> +}
> +
> +/*
> + * Find the subtable entry describing the provided processor
> + */
> +static struct acpi_pptt_processor *acpi_find_processor_node(
> + struct acpi_table_header *table_hdr,
> + u32 acpi_cpu_id)
> +{
> + struct acpi_subtable_header *entry;
> + unsigned long table_end;
> + struct acpi_pptt_processor *cpu_node;
> +
> + table_end = (unsigned long)table_hdr + table_hdr->length;
> + entry = (struct acpi_subtable_header *)((u8 *)table_hdr +
> + sizeof(struct acpi_table_pptt));
> +
> + /* find the processor structure associated with this cpuid */
> + while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < 
> table_end) {
> + cpu_node = (struct acpi_pptt_processor *)entry;
> +
> + if ((entry->type == ACPI_PPTT_TYPE_PROCESSOR) &&
> + acpi_pptt_leaf_node(table_hdr, cpu_node)) {

Is the leaf node check necessary ? Or you just need to check the
ACPI Processor ID valid flag (as discussed offline) ?

> + pr_debug("checking phy_cpu_id %d against acpi id %d\n",
> +  acpi_cpu_id, cpu_node->acpi_processor_id);

Side note: I'd question (some of) these pr_debug() messages.

> + if (acpi_cpu_id == cpu_node->acpi_processor_id) {
> + /* found the correct entry */
> + pr_debug("match found!\n");

Like this one for instance.

> + return (struct acpi_pptt_processor *)entry;
> + }
> + }
> +
> + if (entry->length == 0) {
> + pr_err("Invalid zero length subtable\n");
> + break;
> + }

This should be moved at the beginning of the loop.

> + entry = (struct acpi_subtable_header *)
> + ((u8 *)entry + entry->length);
> + }
> +
> + return NULL;
> +}
> +
> +/*
> + * Given a acpi_pptt_processor node, walk up until we identify the
> + * package that the node is associated with or we run out of levels
> + * to request.
> + */
> +static struct acpi_pptt_processor *acpi_find_processor_package_id(
> + struct acpi_table_header *table_hdr,
> + struct acpi_pptt_processor *cpu,
> + int level)
> +{
> + struct acpi_pptt_processor *prev_node;
> +
> + while (cpu && level && !(cpu->flags & ACPI_PPTT_PHYSICAL_PACKAGE)) {

I really do not understand what ACPI_PPTT_PHYSICAL_PACKAGE means and
more importantly, how it is actually used in this code.

This function is used to get a topology id (that is just a number for
a given topology level) for a given level starting from a given leaf
node.

Why do we care at 

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Tomasz Nowicki]

On 18.10.2017 19:30, Jeremy Linton wrote:

On 10/18/2017 05:24 AM, Tomasz Nowicki wrote:

On 18.10.2017 07:39, Tomasz Nowicki wrote:

Hi,

On 17.10.2017 17:22, Jeremy Linton wrote:

Hi,

On 10/17/2017 08:25 AM, Tomasz Nowicki wrote:

Hi Jeremy,

I did second round of review and have some more comments, please 
see below:


On 12.10.2017 21:48, Jeremy Linton wrote:

ACPI 6.2 adds a new table, which describes how processing units
are related to each other in tree like fashion. Caches are
also sprinkled throughout the tree and describe the properties
of the caches in relation to other caches and processing units.

Add the code to parse the cache hierarchy and report the total
number of levels of cache for a given core using
acpi_find_last_cache_level() as well as fill out the individual
cores cache information with cache_setup_acpi() once the
cpu_cacheinfo structure has been populated by the arch specific
code.

Further, report peers in the topology using setup_acpi_cpu_topology()
to report a unique ID for each processing unit at a given level
in the tree. These unique id's can then be used to match related
processing units which exist as threads, COD (clusters
on die), within a given package, etc.

Signed-off-by: Jeremy Linton 
---
  drivers/acpi/pptt.c | 485 


  1 file changed, 485 insertions(+)
  create mode 100644 drivers/acpi/pptt.c

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
new file mode 100644
index ..c86715fed4a7
--- /dev/null
+++ b/drivers/acpi/pptt.c
@@ -0,1 +1,485 @@
+/*
+ * Copyright (C) 2017, ARM
+ *
+ * This program is free software; you can redistribute it and/or 
modify it

+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but 
WITHOUT
+ * ANY WARRANTY; without even the implied warranty of 
MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public 
License for

+ * more details.
+ *
+ * This file implements parsing of Processor Properties Topology 
Table (PPTT)
+ * which is optionally used to describe the processor and cache 
topology.
+ * Due to the relative pointers used throughout the table, this 
doesn't

+ * leverage the existing subtable parsing in the kernel.
+ */
+#define pr_fmt(fmt) "ACPI PPTT: " fmt
+
+#include 
+#include 
+#include 
+
+/*
+ * Given the PPTT table, find and verify that the subtable entry
+ * is located within the table
+ */
+static struct acpi_subtable_header *fetch_pptt_subtable(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    struct acpi_subtable_header *entry;
+
+    /* there isn't a subtable at reference 0 */
+    if (!pptt_ref)
+    return NULL;
+
+    if (pptt_ref + sizeof(struct acpi_subtable_header) > 
table_hdr->length)

+    return NULL;
+
+    entry = (struct acpi_subtable_header *)((u8 *)table_hdr + 
pptt_ref);

+
+    if (pptt_ref + entry->length > table_hdr->length)
+    return NULL;
+
+    return entry;
+}
+
+static struct acpi_pptt_processor *fetch_pptt_node(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    return (struct acpi_pptt_processor 
*)fetch_pptt_subtable(table_hdr, pptt_ref);

+}
+
+static struct acpi_pptt_cache *fetch_pptt_cache(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    return (struct acpi_pptt_cache 
*)fetch_pptt_subtable(table_hdr, pptt_ref);

+}
+
+static struct acpi_subtable_header *acpi_get_pptt_resource(
+    struct acpi_table_header *table_hdr,
+    struct acpi_pptt_processor *node, int resource)
+{
+    u32 ref;
+
+    if (resource >= node->number_of_priv_resources)
+    return NULL;
+
+    ref = *(u32 *)((u8 *)node + sizeof(struct acpi_pptt_processor) +
+  sizeof(u32) * resource);
+
+    return fetch_pptt_subtable(table_hdr, ref);
+}
+
+/*
+ * given a pptt resource, verify that it is a cache node, then walk
+ * down each level of caches, counting how many levels are found
+ * as well as checking the cache type (icache, dcache, unified). 
If a

+ * level & type match, then we set found, and continue the search.
+ * Once the entire cache branch has been walked return its max
+ * depth.
+ */
+static int acpi_pptt_walk_cache(struct acpi_table_header *table_hdr,
+    int local_level,
+    struct acpi_subtable_header *res,
+    struct acpi_pptt_cache **found,
+    int level, int type)
+{
+    struct acpi_pptt_cache *cache;
+
+    if (res->type != ACPI_PPTT_TYPE_CACHE)
+    return 0;
+
+    cache = (struct acpi_pptt_cache *) res;
+    while (cache) {
+    local_level++;
+
+    if ((local_level == level) &&
+    (cache->flags & ACPI_PPTT_CACHE_TYPE_VALID) &&
+    ((cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) == 
type)) {


Attributes have to be shifted:

(cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) >> 2

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Tomasz Nowicki]

On 18.10.2017 19:30, Jeremy Linton wrote:

On 10/18/2017 05:24 AM, Tomasz Nowicki wrote:

On 18.10.2017 07:39, Tomasz Nowicki wrote:

Hi,

On 17.10.2017 17:22, Jeremy Linton wrote:

Hi,

On 10/17/2017 08:25 AM, Tomasz Nowicki wrote:

Hi Jeremy,

I did second round of review and have some more comments, please 
see below:


On 12.10.2017 21:48, Jeremy Linton wrote:

ACPI 6.2 adds a new table, which describes how processing units
are related to each other in tree like fashion. Caches are
also sprinkled throughout the tree and describe the properties
of the caches in relation to other caches and processing units.

Add the code to parse the cache hierarchy and report the total
number of levels of cache for a given core using
acpi_find_last_cache_level() as well as fill out the individual
cores cache information with cache_setup_acpi() once the
cpu_cacheinfo structure has been populated by the arch specific
code.

Further, report peers in the topology using setup_acpi_cpu_topology()
to report a unique ID for each processing unit at a given level
in the tree. These unique id's can then be used to match related
processing units which exist as threads, COD (clusters
on die), within a given package, etc.

Signed-off-by: Jeremy Linton 
---
  drivers/acpi/pptt.c | 485 


  1 file changed, 485 insertions(+)
  create mode 100644 drivers/acpi/pptt.c

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
new file mode 100644
index ..c86715fed4a7
--- /dev/null
+++ b/drivers/acpi/pptt.c
@@ -0,1 +1,485 @@
+/*
+ * Copyright (C) 2017, ARM
+ *
+ * This program is free software; you can redistribute it and/or 
modify it

+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but 
WITHOUT
+ * ANY WARRANTY; without even the implied warranty of 
MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public 
License for

+ * more details.
+ *
+ * This file implements parsing of Processor Properties Topology 
Table (PPTT)
+ * which is optionally used to describe the processor and cache 
topology.
+ * Due to the relative pointers used throughout the table, this 
doesn't

+ * leverage the existing subtable parsing in the kernel.
+ */
+#define pr_fmt(fmt) "ACPI PPTT: " fmt
+
+#include 
+#include 
+#include 
+
+/*
+ * Given the PPTT table, find and verify that the subtable entry
+ * is located within the table
+ */
+static struct acpi_subtable_header *fetch_pptt_subtable(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    struct acpi_subtable_header *entry;
+
+    /* there isn't a subtable at reference 0 */
+    if (!pptt_ref)
+    return NULL;
+
+    if (pptt_ref + sizeof(struct acpi_subtable_header) > 
table_hdr->length)

+    return NULL;
+
+    entry = (struct acpi_subtable_header *)((u8 *)table_hdr + 
pptt_ref);

+
+    if (pptt_ref + entry->length > table_hdr->length)
+    return NULL;
+
+    return entry;
+}
+
+static struct acpi_pptt_processor *fetch_pptt_node(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    return (struct acpi_pptt_processor 
*)fetch_pptt_subtable(table_hdr, pptt_ref);

+}
+
+static struct acpi_pptt_cache *fetch_pptt_cache(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    return (struct acpi_pptt_cache 
*)fetch_pptt_subtable(table_hdr, pptt_ref);

+}
+
+static struct acpi_subtable_header *acpi_get_pptt_resource(
+    struct acpi_table_header *table_hdr,
+    struct acpi_pptt_processor *node, int resource)
+{
+    u32 ref;
+
+    if (resource >= node->number_of_priv_resources)
+    return NULL;
+
+    ref = *(u32 *)((u8 *)node + sizeof(struct acpi_pptt_processor) +
+  sizeof(u32) * resource);
+
+    return fetch_pptt_subtable(table_hdr, ref);
+}
+
+/*
+ * given a pptt resource, verify that it is a cache node, then walk
+ * down each level of caches, counting how many levels are found
+ * as well as checking the cache type (icache, dcache, unified). 
If a

+ * level & type match, then we set found, and continue the search.
+ * Once the entire cache branch has been walked return its max
+ * depth.
+ */
+static int acpi_pptt_walk_cache(struct acpi_table_header *table_hdr,
+    int local_level,
+    struct acpi_subtable_header *res,
+    struct acpi_pptt_cache **found,
+    int level, int type)
+{
+    struct acpi_pptt_cache *cache;
+
+    if (res->type != ACPI_PPTT_TYPE_CACHE)
+    return 0;
+
+    cache = (struct acpi_pptt_cache *) res;
+    while (cache) {
+    local_level++;
+
+    if ((local_level == level) &&
+    (cache->flags & ACPI_PPTT_CACHE_TYPE_VALID) &&
+    ((cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) == 
type)) {


Attributes have to be shifted:

(cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) >> 2


Hmmm, I'm not sure that 

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Jeremy Linton]

On 10/18/2017 05:24 AM, Tomasz Nowicki wrote:

On 18.10.2017 07:39, Tomasz Nowicki wrote:

Hi,

On 17.10.2017 17:22, Jeremy Linton wrote:

Hi,

On 10/17/2017 08:25 AM, Tomasz Nowicki wrote:

Hi Jeremy,

I did second round of review and have some more comments, please see 
below:


On 12.10.2017 21:48, Jeremy Linton wrote:

ACPI 6.2 adds a new table, which describes how processing units
are related to each other in tree like fashion. Caches are
also sprinkled throughout the tree and describe the properties
of the caches in relation to other caches and processing units.

Add the code to parse the cache hierarchy and report the total
number of levels of cache for a given core using
acpi_find_last_cache_level() as well as fill out the individual
cores cache information with cache_setup_acpi() once the
cpu_cacheinfo structure has been populated by the arch specific
code.

Further, report peers in the topology using setup_acpi_cpu_topology()
to report a unique ID for each processing unit at a given level
in the tree. These unique id's can then be used to match related
processing units which exist as threads, COD (clusters
on die), within a given package, etc.

Signed-off-by: Jeremy Linton 
---
  drivers/acpi/pptt.c | 485 


  1 file changed, 485 insertions(+)
  create mode 100644 drivers/acpi/pptt.c

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
new file mode 100644
index ..c86715fed4a7
--- /dev/null
+++ b/drivers/acpi/pptt.c
@@ -0,1 +1,485 @@
+/*
+ * Copyright (C) 2017, ARM
+ *
+ * This program is free software; you can redistribute it and/or 
modify it

+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but 
WITHOUT
+ * ANY WARRANTY; without even the implied warranty of 
MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public 
License for

+ * more details.
+ *
+ * This file implements parsing of Processor Properties Topology 
Table (PPTT)
+ * which is optionally used to describe the processor and cache 
topology.
+ * Due to the relative pointers used throughout the table, this 
doesn't

+ * leverage the existing subtable parsing in the kernel.
+ */
+#define pr_fmt(fmt) "ACPI PPTT: " fmt
+
+#include 
+#include 
+#include 
+
+/*
+ * Given the PPTT table, find and verify that the subtable entry
+ * is located within the table
+ */
+static struct acpi_subtable_header *fetch_pptt_subtable(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    struct acpi_subtable_header *entry;
+
+    /* there isn't a subtable at reference 0 */
+    if (!pptt_ref)
+    return NULL;
+
+    if (pptt_ref + sizeof(struct acpi_subtable_header) > 
table_hdr->length)

+    return NULL;
+
+    entry = (struct acpi_subtable_header *)((u8 *)table_hdr + 
pptt_ref);

+
+    if (pptt_ref + entry->length > table_hdr->length)
+    return NULL;
+
+    return entry;
+}
+
+static struct acpi_pptt_processor *fetch_pptt_node(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    return (struct acpi_pptt_processor 
*)fetch_pptt_subtable(table_hdr, pptt_ref);

+}
+
+static struct acpi_pptt_cache *fetch_pptt_cache(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    return (struct acpi_pptt_cache 
*)fetch_pptt_subtable(table_hdr, pptt_ref);

+}
+
+static struct acpi_subtable_header *acpi_get_pptt_resource(
+    struct acpi_table_header *table_hdr,
+    struct acpi_pptt_processor *node, int resource)
+{
+    u32 ref;
+
+    if (resource >= node->number_of_priv_resources)
+    return NULL;
+
+    ref = *(u32 *)((u8 *)node + sizeof(struct acpi_pptt_processor) +
+  sizeof(u32) * resource);
+
+    return fetch_pptt_subtable(table_hdr, ref);
+}
+
+/*
+ * given a pptt resource, verify that it is a cache node, then walk
+ * down each level of caches, counting how many levels are found
+ * as well as checking the cache type (icache, dcache, unified). If a
+ * level & type match, then we set found, and continue the search.
+ * Once the entire cache branch has been walked return its max
+ * depth.
+ */
+static int acpi_pptt_walk_cache(struct acpi_table_header *table_hdr,
+    int local_level,
+    struct acpi_subtable_header *res,
+    struct acpi_pptt_cache **found,
+    int level, int type)
+{
+    struct acpi_pptt_cache *cache;
+
+    if (res->type != ACPI_PPTT_TYPE_CACHE)
+    return 0;
+
+    cache = (struct acpi_pptt_cache *) res;
+    while (cache) {
+    local_level++;
+
+    if ((local_level == level) &&
+    (cache->flags & ACPI_PPTT_CACHE_TYPE_VALID) &&
+    ((cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) == 
type)) {


Attributes have to be shifted:

(cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) >> 2


Hmmm, I'm not sure that is true, the top 

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Jeremy Linton]

On 10/18/2017 05:24 AM, Tomasz Nowicki wrote:

On 18.10.2017 07:39, Tomasz Nowicki wrote:

Hi,

On 17.10.2017 17:22, Jeremy Linton wrote:

Hi,

On 10/17/2017 08:25 AM, Tomasz Nowicki wrote:

Hi Jeremy,

I did second round of review and have some more comments, please see 
below:


On 12.10.2017 21:48, Jeremy Linton wrote:

ACPI 6.2 adds a new table, which describes how processing units
are related to each other in tree like fashion. Caches are
also sprinkled throughout the tree and describe the properties
of the caches in relation to other caches and processing units.

Add the code to parse the cache hierarchy and report the total
number of levels of cache for a given core using
acpi_find_last_cache_level() as well as fill out the individual
cores cache information with cache_setup_acpi() once the
cpu_cacheinfo structure has been populated by the arch specific
code.

Further, report peers in the topology using setup_acpi_cpu_topology()
to report a unique ID for each processing unit at a given level
in the tree. These unique id's can then be used to match related
processing units which exist as threads, COD (clusters
on die), within a given package, etc.

Signed-off-by: Jeremy Linton 
---
  drivers/acpi/pptt.c | 485 


  1 file changed, 485 insertions(+)
  create mode 100644 drivers/acpi/pptt.c

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
new file mode 100644
index ..c86715fed4a7
--- /dev/null
+++ b/drivers/acpi/pptt.c
@@ -0,1 +1,485 @@
+/*
+ * Copyright (C) 2017, ARM
+ *
+ * This program is free software; you can redistribute it and/or 
modify it

+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but 
WITHOUT
+ * ANY WARRANTY; without even the implied warranty of 
MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public 
License for

+ * more details.
+ *
+ * This file implements parsing of Processor Properties Topology 
Table (PPTT)
+ * which is optionally used to describe the processor and cache 
topology.
+ * Due to the relative pointers used throughout the table, this 
doesn't

+ * leverage the existing subtable parsing in the kernel.
+ */
+#define pr_fmt(fmt) "ACPI PPTT: " fmt
+
+#include 
+#include 
+#include 
+
+/*
+ * Given the PPTT table, find and verify that the subtable entry
+ * is located within the table
+ */
+static struct acpi_subtable_header *fetch_pptt_subtable(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    struct acpi_subtable_header *entry;
+
+    /* there isn't a subtable at reference 0 */
+    if (!pptt_ref)
+    return NULL;
+
+    if (pptt_ref + sizeof(struct acpi_subtable_header) > 
table_hdr->length)

+    return NULL;
+
+    entry = (struct acpi_subtable_header *)((u8 *)table_hdr + 
pptt_ref);

+
+    if (pptt_ref + entry->length > table_hdr->length)
+    return NULL;
+
+    return entry;
+}
+
+static struct acpi_pptt_processor *fetch_pptt_node(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    return (struct acpi_pptt_processor 
*)fetch_pptt_subtable(table_hdr, pptt_ref);

+}
+
+static struct acpi_pptt_cache *fetch_pptt_cache(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    return (struct acpi_pptt_cache 
*)fetch_pptt_subtable(table_hdr, pptt_ref);

+}
+
+static struct acpi_subtable_header *acpi_get_pptt_resource(
+    struct acpi_table_header *table_hdr,
+    struct acpi_pptt_processor *node, int resource)
+{
+    u32 ref;
+
+    if (resource >= node->number_of_priv_resources)
+    return NULL;
+
+    ref = *(u32 *)((u8 *)node + sizeof(struct acpi_pptt_processor) +
+  sizeof(u32) * resource);
+
+    return fetch_pptt_subtable(table_hdr, ref);
+}
+
+/*
+ * given a pptt resource, verify that it is a cache node, then walk
+ * down each level of caches, counting how many levels are found
+ * as well as checking the cache type (icache, dcache, unified). If a
+ * level & type match, then we set found, and continue the search.
+ * Once the entire cache branch has been walked return its max
+ * depth.
+ */
+static int acpi_pptt_walk_cache(struct acpi_table_header *table_hdr,
+    int local_level,
+    struct acpi_subtable_header *res,
+    struct acpi_pptt_cache **found,
+    int level, int type)
+{
+    struct acpi_pptt_cache *cache;
+
+    if (res->type != ACPI_PPTT_TYPE_CACHE)
+    return 0;
+
+    cache = (struct acpi_pptt_cache *) res;
+    while (cache) {
+    local_level++;
+
+    if ((local_level == level) &&
+    (cache->flags & ACPI_PPTT_CACHE_TYPE_VALID) &&
+    ((cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) == 
type)) {


Attributes have to be shifted:

(cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) >> 2


Hmmm, I'm not sure that is true, the top level function in this 

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Tomasz Nowicki]

On 18.10.2017 07:39, Tomasz Nowicki wrote:

Hi,

On 17.10.2017 17:22, Jeremy Linton wrote:

Hi,

On 10/17/2017 08:25 AM, Tomasz Nowicki wrote:

Hi Jeremy,

I did second round of review and have some more comments, please see 
below:


On 12.10.2017 21:48, Jeremy Linton wrote:

ACPI 6.2 adds a new table, which describes how processing units
are related to each other in tree like fashion. Caches are
also sprinkled throughout the tree and describe the properties
of the caches in relation to other caches and processing units.

Add the code to parse the cache hierarchy and report the total
number of levels of cache for a given core using
acpi_find_last_cache_level() as well as fill out the individual
cores cache information with cache_setup_acpi() once the
cpu_cacheinfo structure has been populated by the arch specific
code.

Further, report peers in the topology using setup_acpi_cpu_topology()
to report a unique ID for each processing unit at a given level
in the tree. These unique id's can then be used to match related
processing units which exist as threads, COD (clusters
on die), within a given package, etc.

Signed-off-by: Jeremy Linton 
---
  drivers/acpi/pptt.c | 485 


  1 file changed, 485 insertions(+)
  create mode 100644 drivers/acpi/pptt.c

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
new file mode 100644
index ..c86715fed4a7
--- /dev/null
+++ b/drivers/acpi/pptt.c
@@ -0,1 +1,485 @@
+/*
+ * Copyright (C) 2017, ARM
+ *
+ * This program is free software; you can redistribute it and/or 
modify it

+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but 
WITHOUT
+ * ANY WARRANTY; without even the implied warranty of 
MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public 
License for

+ * more details.
+ *
+ * This file implements parsing of Processor Properties Topology 
Table (PPTT)
+ * which is optionally used to describe the processor and cache 
topology.
+ * Due to the relative pointers used throughout the table, this 
doesn't

+ * leverage the existing subtable parsing in the kernel.
+ */
+#define pr_fmt(fmt) "ACPI PPTT: " fmt
+
+#include 
+#include 
+#include 
+
+/*
+ * Given the PPTT table, find and verify that the subtable entry
+ * is located within the table
+ */
+static struct acpi_subtable_header *fetch_pptt_subtable(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    struct acpi_subtable_header *entry;
+
+    /* there isn't a subtable at reference 0 */
+    if (!pptt_ref)
+    return NULL;
+
+    if (pptt_ref + sizeof(struct acpi_subtable_header) > 
table_hdr->length)

+    return NULL;
+
+    entry = (struct acpi_subtable_header *)((u8 *)table_hdr + 
pptt_ref);

+
+    if (pptt_ref + entry->length > table_hdr->length)
+    return NULL;
+
+    return entry;
+}
+
+static struct acpi_pptt_processor *fetch_pptt_node(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    return (struct acpi_pptt_processor 
*)fetch_pptt_subtable(table_hdr, pptt_ref);

+}
+
+static struct acpi_pptt_cache *fetch_pptt_cache(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    return (struct acpi_pptt_cache *)fetch_pptt_subtable(table_hdr, 
pptt_ref);

+}
+
+static struct acpi_subtable_header *acpi_get_pptt_resource(
+    struct acpi_table_header *table_hdr,
+    struct acpi_pptt_processor *node, int resource)
+{
+    u32 ref;
+
+    if (resource >= node->number_of_priv_resources)
+    return NULL;
+
+    ref = *(u32 *)((u8 *)node + sizeof(struct acpi_pptt_processor) +
+  sizeof(u32) * resource);
+
+    return fetch_pptt_subtable(table_hdr, ref);
+}
+
+/*
+ * given a pptt resource, verify that it is a cache node, then walk
+ * down each level of caches, counting how many levels are found
+ * as well as checking the cache type (icache, dcache, unified). If a
+ * level & type match, then we set found, and continue the search.
+ * Once the entire cache branch has been walked return its max
+ * depth.
+ */
+static int acpi_pptt_walk_cache(struct acpi_table_header *table_hdr,
+    int local_level,
+    struct acpi_subtable_header *res,
+    struct acpi_pptt_cache **found,
+    int level, int type)
+{
+    struct acpi_pptt_cache *cache;
+
+    if (res->type != ACPI_PPTT_TYPE_CACHE)
+    return 0;
+
+    cache = (struct acpi_pptt_cache *) res;
+    while (cache) {
+    local_level++;
+
+    if ((local_level == level) &&
+    (cache->flags & ACPI_PPTT_CACHE_TYPE_VALID) &&
+    ((cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) == 
type)) {


Attributes have to be shifted:

(cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) >> 2


Hmmm, I'm not sure that is true, the top level function in this 
routine convert the "linux" 

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Tomasz Nowicki]

On 18.10.2017 07:39, Tomasz Nowicki wrote:

Hi,

On 17.10.2017 17:22, Jeremy Linton wrote:

Hi,

On 10/17/2017 08:25 AM, Tomasz Nowicki wrote:

Hi Jeremy,

I did second round of review and have some more comments, please see 
below:


On 12.10.2017 21:48, Jeremy Linton wrote:

ACPI 6.2 adds a new table, which describes how processing units
are related to each other in tree like fashion. Caches are
also sprinkled throughout the tree and describe the properties
of the caches in relation to other caches and processing units.

Add the code to parse the cache hierarchy and report the total
number of levels of cache for a given core using
acpi_find_last_cache_level() as well as fill out the individual
cores cache information with cache_setup_acpi() once the
cpu_cacheinfo structure has been populated by the arch specific
code.

Further, report peers in the topology using setup_acpi_cpu_topology()
to report a unique ID for each processing unit at a given level
in the tree. These unique id's can then be used to match related
processing units which exist as threads, COD (clusters
on die), within a given package, etc.

Signed-off-by: Jeremy Linton 
---
  drivers/acpi/pptt.c | 485 


  1 file changed, 485 insertions(+)
  create mode 100644 drivers/acpi/pptt.c

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
new file mode 100644
index ..c86715fed4a7
--- /dev/null
+++ b/drivers/acpi/pptt.c
@@ -0,1 +1,485 @@
+/*
+ * Copyright (C) 2017, ARM
+ *
+ * This program is free software; you can redistribute it and/or 
modify it

+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but 
WITHOUT
+ * ANY WARRANTY; without even the implied warranty of 
MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public 
License for

+ * more details.
+ *
+ * This file implements parsing of Processor Properties Topology 
Table (PPTT)
+ * which is optionally used to describe the processor and cache 
topology.
+ * Due to the relative pointers used throughout the table, this 
doesn't

+ * leverage the existing subtable parsing in the kernel.
+ */
+#define pr_fmt(fmt) "ACPI PPTT: " fmt
+
+#include 
+#include 
+#include 
+
+/*
+ * Given the PPTT table, find and verify that the subtable entry
+ * is located within the table
+ */
+static struct acpi_subtable_header *fetch_pptt_subtable(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    struct acpi_subtable_header *entry;
+
+    /* there isn't a subtable at reference 0 */
+    if (!pptt_ref)
+    return NULL;
+
+    if (pptt_ref + sizeof(struct acpi_subtable_header) > 
table_hdr->length)

+    return NULL;
+
+    entry = (struct acpi_subtable_header *)((u8 *)table_hdr + 
pptt_ref);

+
+    if (pptt_ref + entry->length > table_hdr->length)
+    return NULL;
+
+    return entry;
+}
+
+static struct acpi_pptt_processor *fetch_pptt_node(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    return (struct acpi_pptt_processor 
*)fetch_pptt_subtable(table_hdr, pptt_ref);

+}
+
+static struct acpi_pptt_cache *fetch_pptt_cache(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    return (struct acpi_pptt_cache *)fetch_pptt_subtable(table_hdr, 
pptt_ref);

+}
+
+static struct acpi_subtable_header *acpi_get_pptt_resource(
+    struct acpi_table_header *table_hdr,
+    struct acpi_pptt_processor *node, int resource)
+{
+    u32 ref;
+
+    if (resource >= node->number_of_priv_resources)
+    return NULL;
+
+    ref = *(u32 *)((u8 *)node + sizeof(struct acpi_pptt_processor) +
+  sizeof(u32) * resource);
+
+    return fetch_pptt_subtable(table_hdr, ref);
+}
+
+/*
+ * given a pptt resource, verify that it is a cache node, then walk
+ * down each level of caches, counting how many levels are found
+ * as well as checking the cache type (icache, dcache, unified). If a
+ * level & type match, then we set found, and continue the search.
+ * Once the entire cache branch has been walked return its max
+ * depth.
+ */
+static int acpi_pptt_walk_cache(struct acpi_table_header *table_hdr,
+    int local_level,
+    struct acpi_subtable_header *res,
+    struct acpi_pptt_cache **found,
+    int level, int type)
+{
+    struct acpi_pptt_cache *cache;
+
+    if (res->type != ACPI_PPTT_TYPE_CACHE)
+    return 0;
+
+    cache = (struct acpi_pptt_cache *) res;
+    while (cache) {
+    local_level++;
+
+    if ((local_level == level) &&
+    (cache->flags & ACPI_PPTT_CACHE_TYPE_VALID) &&
+    ((cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) == 
type)) {


Attributes have to be shifted:

(cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) >> 2


Hmmm, I'm not sure that is true, the top level function in this 
routine convert the "linux" constant to the ACPI 

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Tomasz Nowicki]

Hi,

On 17.10.2017 17:22, Jeremy Linton wrote:

Hi,

On 10/17/2017 08:25 AM, Tomasz Nowicki wrote:

Hi Jeremy,

I did second round of review and have some more comments, please see 
below:


On 12.10.2017 21:48, Jeremy Linton wrote:

ACPI 6.2 adds a new table, which describes how processing units
are related to each other in tree like fashion. Caches are
also sprinkled throughout the tree and describe the properties
of the caches in relation to other caches and processing units.

Add the code to parse the cache hierarchy and report the total
number of levels of cache for a given core using
acpi_find_last_cache_level() as well as fill out the individual
cores cache information with cache_setup_acpi() once the
cpu_cacheinfo structure has been populated by the arch specific
code.

Further, report peers in the topology using setup_acpi_cpu_topology()
to report a unique ID for each processing unit at a given level
in the tree. These unique id's can then be used to match related
processing units which exist as threads, COD (clusters
on die), within a given package, etc.

Signed-off-by: Jeremy Linton 
---
  drivers/acpi/pptt.c | 485 


  1 file changed, 485 insertions(+)
  create mode 100644 drivers/acpi/pptt.c

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
new file mode 100644
index ..c86715fed4a7
--- /dev/null
+++ b/drivers/acpi/pptt.c
@@ -0,1 +1,485 @@
+/*
+ * Copyright (C) 2017, ARM
+ *
+ * This program is free software; you can redistribute it and/or 
modify it

+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but 
WITHOUT
+ * ANY WARRANTY; without even the implied warranty of 
MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public 
License for

+ * more details.
+ *
+ * This file implements parsing of Processor Properties Topology 
Table (PPTT)
+ * which is optionally used to describe the processor and cache 
topology.

+ * Due to the relative pointers used throughout the table, this doesn't
+ * leverage the existing subtable parsing in the kernel.
+ */
+#define pr_fmt(fmt) "ACPI PPTT: " fmt
+
+#include 
+#include 
+#include 
+
+/*
+ * Given the PPTT table, find and verify that the subtable entry
+ * is located within the table
+ */
+static struct acpi_subtable_header *fetch_pptt_subtable(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    struct acpi_subtable_header *entry;
+
+    /* there isn't a subtable at reference 0 */
+    if (!pptt_ref)
+    return NULL;
+
+    if (pptt_ref + sizeof(struct acpi_subtable_header) > 
table_hdr->length)

+    return NULL;
+
+    entry = (struct acpi_subtable_header *)((u8 *)table_hdr + 
pptt_ref);

+
+    if (pptt_ref + entry->length > table_hdr->length)
+    return NULL;
+
+    return entry;
+}
+
+static struct acpi_pptt_processor *fetch_pptt_node(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    return (struct acpi_pptt_processor 
*)fetch_pptt_subtable(table_hdr, pptt_ref);

+}
+
+static struct acpi_pptt_cache *fetch_pptt_cache(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    return (struct acpi_pptt_cache *)fetch_pptt_subtable(table_hdr, 
pptt_ref);

+}
+
+static struct acpi_subtable_header *acpi_get_pptt_resource(
+    struct acpi_table_header *table_hdr,
+    struct acpi_pptt_processor *node, int resource)
+{
+    u32 ref;
+
+    if (resource >= node->number_of_priv_resources)
+    return NULL;
+
+    ref = *(u32 *)((u8 *)node + sizeof(struct acpi_pptt_processor) +
+  sizeof(u32) * resource);
+
+    return fetch_pptt_subtable(table_hdr, ref);
+}
+
+/*
+ * given a pptt resource, verify that it is a cache node, then walk
+ * down each level of caches, counting how many levels are found
+ * as well as checking the cache type (icache, dcache, unified). If a
+ * level & type match, then we set found, and continue the search.
+ * Once the entire cache branch has been walked return its max
+ * depth.
+ */
+static int acpi_pptt_walk_cache(struct acpi_table_header *table_hdr,
+    int local_level,
+    struct acpi_subtable_header *res,
+    struct acpi_pptt_cache **found,
+    int level, int type)
+{
+    struct acpi_pptt_cache *cache;
+
+    if (res->type != ACPI_PPTT_TYPE_CACHE)
+    return 0;
+
+    cache = (struct acpi_pptt_cache *) res;
+    while (cache) {
+    local_level++;
+
+    if ((local_level == level) &&
+    (cache->flags & ACPI_PPTT_CACHE_TYPE_VALID) &&
+    ((cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) == 
type)) {


Attributes have to be shifted:

(cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) >> 2


Hmmm, I'm not sure that is true, the top level function in this routine 
convert the "linux" constant to the ACPI version of that 

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Tomasz Nowicki]

Hi,

On 17.10.2017 17:22, Jeremy Linton wrote:

Hi,

On 10/17/2017 08:25 AM, Tomasz Nowicki wrote:

Hi Jeremy,

I did second round of review and have some more comments, please see 
below:


On 12.10.2017 21:48, Jeremy Linton wrote:

ACPI 6.2 adds a new table, which describes how processing units
are related to each other in tree like fashion. Caches are
also sprinkled throughout the tree and describe the properties
of the caches in relation to other caches and processing units.

Add the code to parse the cache hierarchy and report the total
number of levels of cache for a given core using
acpi_find_last_cache_level() as well as fill out the individual
cores cache information with cache_setup_acpi() once the
cpu_cacheinfo structure has been populated by the arch specific
code.

Further, report peers in the topology using setup_acpi_cpu_topology()
to report a unique ID for each processing unit at a given level
in the tree. These unique id's can then be used to match related
processing units which exist as threads, COD (clusters
on die), within a given package, etc.

Signed-off-by: Jeremy Linton 
---
  drivers/acpi/pptt.c | 485 


  1 file changed, 485 insertions(+)
  create mode 100644 drivers/acpi/pptt.c

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
new file mode 100644
index ..c86715fed4a7
--- /dev/null
+++ b/drivers/acpi/pptt.c
@@ -0,1 +1,485 @@
+/*
+ * Copyright (C) 2017, ARM
+ *
+ * This program is free software; you can redistribute it and/or 
modify it

+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but 
WITHOUT
+ * ANY WARRANTY; without even the implied warranty of 
MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public 
License for

+ * more details.
+ *
+ * This file implements parsing of Processor Properties Topology 
Table (PPTT)
+ * which is optionally used to describe the processor and cache 
topology.

+ * Due to the relative pointers used throughout the table, this doesn't
+ * leverage the existing subtable parsing in the kernel.
+ */
+#define pr_fmt(fmt) "ACPI PPTT: " fmt
+
+#include 
+#include 
+#include 
+
+/*
+ * Given the PPTT table, find and verify that the subtable entry
+ * is located within the table
+ */
+static struct acpi_subtable_header *fetch_pptt_subtable(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    struct acpi_subtable_header *entry;
+
+    /* there isn't a subtable at reference 0 */
+    if (!pptt_ref)
+    return NULL;
+
+    if (pptt_ref + sizeof(struct acpi_subtable_header) > 
table_hdr->length)

+    return NULL;
+
+    entry = (struct acpi_subtable_header *)((u8 *)table_hdr + 
pptt_ref);

+
+    if (pptt_ref + entry->length > table_hdr->length)
+    return NULL;
+
+    return entry;
+}
+
+static struct acpi_pptt_processor *fetch_pptt_node(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    return (struct acpi_pptt_processor 
*)fetch_pptt_subtable(table_hdr, pptt_ref);

+}
+
+static struct acpi_pptt_cache *fetch_pptt_cache(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    return (struct acpi_pptt_cache *)fetch_pptt_subtable(table_hdr, 
pptt_ref);

+}
+
+static struct acpi_subtable_header *acpi_get_pptt_resource(
+    struct acpi_table_header *table_hdr,
+    struct acpi_pptt_processor *node, int resource)
+{
+    u32 ref;
+
+    if (resource >= node->number_of_priv_resources)
+    return NULL;
+
+    ref = *(u32 *)((u8 *)node + sizeof(struct acpi_pptt_processor) +
+  sizeof(u32) * resource);
+
+    return fetch_pptt_subtable(table_hdr, ref);
+}
+
+/*
+ * given a pptt resource, verify that it is a cache node, then walk
+ * down each level of caches, counting how many levels are found
+ * as well as checking the cache type (icache, dcache, unified). If a
+ * level & type match, then we set found, and continue the search.
+ * Once the entire cache branch has been walked return its max
+ * depth.
+ */
+static int acpi_pptt_walk_cache(struct acpi_table_header *table_hdr,
+    int local_level,
+    struct acpi_subtable_header *res,
+    struct acpi_pptt_cache **found,
+    int level, int type)
+{
+    struct acpi_pptt_cache *cache;
+
+    if (res->type != ACPI_PPTT_TYPE_CACHE)
+    return 0;
+
+    cache = (struct acpi_pptt_cache *) res;
+    while (cache) {
+    local_level++;
+
+    if ((local_level == level) &&
+    (cache->flags & ACPI_PPTT_CACHE_TYPE_VALID) &&
+    ((cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) == 
type)) {


Attributes have to be shifted:

(cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) >> 2


Hmmm, I'm not sure that is true, the top level function in this routine 
convert the "linux" constant to the ACPI version of that constant. In 
that case the 

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Xiongfeng Wang]

Hi Jeremy,


On 2017/10/17 23:22, Jeremy Linton wrote:
> Hi,
>
> On 10/17/2017 08:25 AM, Tomasz Nowicki wrote:
>> Hi Jeremy,
>>
>> I did second round of review and have some more comments, please see below:
>>
>> On 12.10.2017 21:48, Jeremy Linton wrote:
> I disagree, that routine is shared by the two code paths because its 
> functionality is 99% duplicated between the two. The difference being whether 
> it terminates the search at a given level, or continues searching until it 
> runs out of nodes. The latter case is simply a degenerate version of the 
> first.
>
>
>>
>> Also, seems like we can merge acpi_parse_pptt() & acpi_process_node().
>
> That is true, but I fail to see how any of this is actually fixes anything. 
> There are a million ways to do this, including as pointed out by building 
> another data-structure to simplify the parsing what is a table that is less 
> than ideal for runtime parsing (starting with the direction of the relative 
> pointers, and ending with having to "infer" information that isn't directly 
> flagged). I actually built a couple other versions of this, including a nice 
> cute version which is about 1/8 this size of this and really easy to 
> understand but of course is recursive...
>
>
Maybe you can see my version below. It is similar to what you said above. It 
may give some help.
https://github.com/fenghusthu/acpi_pptt

Thanks
Xiongfeng Wang
>>
>> Here are my suggestions:
>>
>>
>> static struct acpi_pptt_cache *acpi_pptt_cache_type_level(
>>  struct acpi_table_header *table_hdr,
>>  struct acpi_subtable_header *res,
>>  int *local_level,
>>  int level, int type)
>> {
>>  struct acpi_pptt_cache *cache = (struct acpi_pptt_cache *) res;
>>
>>  if (res->type != ACPI_PPTT_TYPE_CACHE)
>>  return NULL;
>>
>>  while (cache) {
>>  if ((*local_level == level) &&
>>  (cache->flags & ACPI_PPTT_CACHE_TYPE_VALID) &&
>>  ((cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) >> 2 == type)) 
>> {
>>
>>  pr_debug("Found cache @ level %d\n", level);
>>  return cache;
>>  }
>>  cache = fetch_pptt_cache(table_hdr, cache->next_level_of_cache);
>>  (*local_level)++;
>>  }
>>  return NULL;
>> }
>>
>> static struct acpi_pptt_cache *_acpi_find_cache_node(
>>  struct acpi_table_header *table_hdr,
>>  struct acpi_pptt_processor *cpu_node,
>>  int *local_level, int level, int type)
>> {
>>  struct acpi_subtable_header *res;
>>  struct acpi_pptt_cache *cache_tmp, *cache = NULL;
>>  int resource = 0;
>>
>>  /* walk down from the processor node */
>>  while ((res = acpi_get_pptt_resource(table_hdr, cpu_node, resource))) {
>>
>>  cache_tmp = acpi_pptt_cache_type_level(table_hdr, res,
>> local_level, level, type);
>>  if (cache_tmp) {
>>  if (cache)
>>  pr_err("Found duplicate cache level/type unable to 
>> determine uniqueness\n");
>>
>>  cache = cache_tmp;
>>  }
>>  resource++;
>>  }
>>  return cache;
>> }
>>
>> /* find the ACPI node describing the cache type/level for the given CPU */
>> static struct acpi_pptt_cache *acpi_find_cache_node(
>>  struct acpi_table_header *table_hdr, u32 acpi_cpu_id,
>>  enum cache_type type, unsigned int level,
>>  struct acpi_pptt_processor **node)
>> {
>>  int total_levels = 0;
>>  struct acpi_pptt_cache *found = NULL;
>>  struct acpi_pptt_processor *cpu_node;
>>  u8 acpi_type = acpi_cache_type(type);
>>
>>  pr_debug("Looking for CPU %d's level %d cache type %d\n",
>>   acpi_cpu_id, level, acpi_type);
>>
>>  cpu_node = acpi_find_processor_node(table_hdr, acpi_cpu_id);
>>  if (!cpu_node)
>>  return NULL;
>>
>>  do {
>>  found = _acpi_find_cache_node(table_hdr, cpu_node,
>>_levels, level, acpi_type);
>>  *node = cpu_node;
>>  cpu_node = fetch_pptt_node(table_hdr, cpu_node->parent);
>>  } while ((cpu_node) && (!found));
>>
>>  return found;
>> }
>>
>> static int acpi_pptt_cache_level(struct acpi_table_header *table_hdr,
>>  struct acpi_subtable_header *res)
>> {
>>  struct acpi_pptt_cache *cache = (struct acpi_pptt_cache *) res;
>>  int local_level = 1;
>>
>>  if (res->type != ACPI_PPTT_TYPE_CACHE)
>>  return 0;
>>
>>  while ((cache = fetch_pptt_cache(table_hdr, 
>> cache->next_level_of_cache)))
>>  local_level++;
>>  return local_level;
>> }
>>
>> static int _acpi_count_cache_level(
>>  struct acpi_table_header *table_hdr,
>>  struct acpi_pptt_processor *cpu_node)
>> {
>>  struct acpi_subtable_header *res;
>>  int levels = 0, resource = 0, number_of_levels = 0;
>>
>>  /* walk down from the processor node */
>>  while ((res = acpi_get_pptt_resource(table_hdr, cpu_node, resource))) {
>>  levels = 

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Xiongfeng Wang]

Hi Jeremy,


On 2017/10/17 23:22, Jeremy Linton wrote:
> Hi,
>
> On 10/17/2017 08:25 AM, Tomasz Nowicki wrote:
>> Hi Jeremy,
>>
>> I did second round of review and have some more comments, please see below:
>>
>> On 12.10.2017 21:48, Jeremy Linton wrote:
> I disagree, that routine is shared by the two code paths because its 
> functionality is 99% duplicated between the two. The difference being whether 
> it terminates the search at a given level, or continues searching until it 
> runs out of nodes. The latter case is simply a degenerate version of the 
> first.
>
>
>>
>> Also, seems like we can merge acpi_parse_pptt() & acpi_process_node().
>
> That is true, but I fail to see how any of this is actually fixes anything. 
> There are a million ways to do this, including as pointed out by building 
> another data-structure to simplify the parsing what is a table that is less 
> than ideal for runtime parsing (starting with the direction of the relative 
> pointers, and ending with having to "infer" information that isn't directly 
> flagged). I actually built a couple other versions of this, including a nice 
> cute version which is about 1/8 this size of this and really easy to 
> understand but of course is recursive...
>
>
Maybe you can see my version below. It is similar to what you said above. It 
may give some help.
https://github.com/fenghusthu/acpi_pptt

Thanks
Xiongfeng Wang
>>
>> Here are my suggestions:
>>
>>
>> static struct acpi_pptt_cache *acpi_pptt_cache_type_level(
>>  struct acpi_table_header *table_hdr,
>>  struct acpi_subtable_header *res,
>>  int *local_level,
>>  int level, int type)
>> {
>>  struct acpi_pptt_cache *cache = (struct acpi_pptt_cache *) res;
>>
>>  if (res->type != ACPI_PPTT_TYPE_CACHE)
>>  return NULL;
>>
>>  while (cache) {
>>  if ((*local_level == level) &&
>>  (cache->flags & ACPI_PPTT_CACHE_TYPE_VALID) &&
>>  ((cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) >> 2 == type)) 
>> {
>>
>>  pr_debug("Found cache @ level %d\n", level);
>>  return cache;
>>  }
>>  cache = fetch_pptt_cache(table_hdr, cache->next_level_of_cache);
>>  (*local_level)++;
>>  }
>>  return NULL;
>> }
>>
>> static struct acpi_pptt_cache *_acpi_find_cache_node(
>>  struct acpi_table_header *table_hdr,
>>  struct acpi_pptt_processor *cpu_node,
>>  int *local_level, int level, int type)
>> {
>>  struct acpi_subtable_header *res;
>>  struct acpi_pptt_cache *cache_tmp, *cache = NULL;
>>  int resource = 0;
>>
>>  /* walk down from the processor node */
>>  while ((res = acpi_get_pptt_resource(table_hdr, cpu_node, resource))) {
>>
>>  cache_tmp = acpi_pptt_cache_type_level(table_hdr, res,
>> local_level, level, type);
>>  if (cache_tmp) {
>>  if (cache)
>>  pr_err("Found duplicate cache level/type unable to 
>> determine uniqueness\n");
>>
>>  cache = cache_tmp;
>>  }
>>  resource++;
>>  }
>>  return cache;
>> }
>>
>> /* find the ACPI node describing the cache type/level for the given CPU */
>> static struct acpi_pptt_cache *acpi_find_cache_node(
>>  struct acpi_table_header *table_hdr, u32 acpi_cpu_id,
>>  enum cache_type type, unsigned int level,
>>  struct acpi_pptt_processor **node)
>> {
>>  int total_levels = 0;
>>  struct acpi_pptt_cache *found = NULL;
>>  struct acpi_pptt_processor *cpu_node;
>>  u8 acpi_type = acpi_cache_type(type);
>>
>>  pr_debug("Looking for CPU %d's level %d cache type %d\n",
>>   acpi_cpu_id, level, acpi_type);
>>
>>  cpu_node = acpi_find_processor_node(table_hdr, acpi_cpu_id);
>>  if (!cpu_node)
>>  return NULL;
>>
>>  do {
>>  found = _acpi_find_cache_node(table_hdr, cpu_node,
>>_levels, level, acpi_type);
>>  *node = cpu_node;
>>  cpu_node = fetch_pptt_node(table_hdr, cpu_node->parent);
>>  } while ((cpu_node) && (!found));
>>
>>  return found;
>> }
>>
>> static int acpi_pptt_cache_level(struct acpi_table_header *table_hdr,
>>  struct acpi_subtable_header *res)
>> {
>>  struct acpi_pptt_cache *cache = (struct acpi_pptt_cache *) res;
>>  int local_level = 1;
>>
>>  if (res->type != ACPI_PPTT_TYPE_CACHE)
>>  return 0;
>>
>>  while ((cache = fetch_pptt_cache(table_hdr, 
>> cache->next_level_of_cache)))
>>  local_level++;
>>  return local_level;
>> }
>>
>> static int _acpi_count_cache_level(
>>  struct acpi_table_header *table_hdr,
>>  struct acpi_pptt_processor *cpu_node)
>> {
>>  struct acpi_subtable_header *res;
>>  int levels = 0, resource = 0, number_of_levels = 0;
>>
>>  /* walk down from the processor node */
>>  while ((res = acpi_get_pptt_resource(table_hdr, cpu_node, resource))) {
>>  levels = 

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Jeremy Linton]

Hi,

On 10/17/2017 08:25 AM, Tomasz Nowicki wrote:

Hi Jeremy,

I did second round of review and have some more comments, please see below:

On 12.10.2017 21:48, Jeremy Linton wrote:

ACPI 6.2 adds a new table, which describes how processing units
are related to each other in tree like fashion. Caches are
also sprinkled throughout the tree and describe the properties
of the caches in relation to other caches and processing units.

Add the code to parse the cache hierarchy and report the total
number of levels of cache for a given core using
acpi_find_last_cache_level() as well as fill out the individual
cores cache information with cache_setup_acpi() once the
cpu_cacheinfo structure has been populated by the arch specific
code.

Further, report peers in the topology using setup_acpi_cpu_topology()
to report a unique ID for each processing unit at a given level
in the tree. These unique id's can then be used to match related
processing units which exist as threads, COD (clusters
on die), within a given package, etc.

Signed-off-by: Jeremy Linton 
---
  drivers/acpi/pptt.c | 485 


  1 file changed, 485 insertions(+)
  create mode 100644 drivers/acpi/pptt.c

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
new file mode 100644
index ..c86715fed4a7
--- /dev/null
+++ b/drivers/acpi/pptt.c
@@ -0,1 +1,485 @@
+/*
+ * Copyright (C) 2017, ARM
+ *
+ * This program is free software; you can redistribute it and/or 
modify it

+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but 
WITHOUT

+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public 
License for

+ * more details.
+ *
+ * This file implements parsing of Processor Properties Topology 
Table (PPTT)
+ * which is optionally used to describe the processor and cache 
topology.

+ * Due to the relative pointers used throughout the table, this doesn't
+ * leverage the existing subtable parsing in the kernel.
+ */
+#define pr_fmt(fmt) "ACPI PPTT: " fmt
+
+#include 
+#include 
+#include 
+
+/*
+ * Given the PPTT table, find and verify that the subtable entry
+ * is located within the table
+ */
+static struct acpi_subtable_header *fetch_pptt_subtable(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    struct acpi_subtable_header *entry;
+
+    /* there isn't a subtable at reference 0 */
+    if (!pptt_ref)
+    return NULL;
+
+    if (pptt_ref + sizeof(struct acpi_subtable_header) > 
table_hdr->length)

+    return NULL;
+
+    entry = (struct acpi_subtable_header *)((u8 *)table_hdr + pptt_ref);
+
+    if (pptt_ref + entry->length > table_hdr->length)
+    return NULL;
+
+    return entry;
+}
+
+static struct acpi_pptt_processor *fetch_pptt_node(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    return (struct acpi_pptt_processor 
*)fetch_pptt_subtable(table_hdr, pptt_ref);

+}
+
+static struct acpi_pptt_cache *fetch_pptt_cache(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    return (struct acpi_pptt_cache *)fetch_pptt_subtable(table_hdr, 
pptt_ref);

+}
+
+static struct acpi_subtable_header *acpi_get_pptt_resource(
+    struct acpi_table_header *table_hdr,
+    struct acpi_pptt_processor *node, int resource)
+{
+    u32 ref;
+
+    if (resource >= node->number_of_priv_resources)
+    return NULL;
+
+    ref = *(u32 *)((u8 *)node + sizeof(struct acpi_pptt_processor) +
+  sizeof(u32) * resource);
+
+    return fetch_pptt_subtable(table_hdr, ref);
+}
+
+/*
+ * given a pptt resource, verify that it is a cache node, then walk
+ * down each level of caches, counting how many levels are found
+ * as well as checking the cache type (icache, dcache, unified). If a
+ * level & type match, then we set found, and continue the search.
+ * Once the entire cache branch has been walked return its max
+ * depth.
+ */
+static int acpi_pptt_walk_cache(struct acpi_table_header *table_hdr,
+    int local_level,
+    struct acpi_subtable_header *res,
+    struct acpi_pptt_cache **found,
+    int level, int type)
+{
+    struct acpi_pptt_cache *cache;
+
+    if (res->type != ACPI_PPTT_TYPE_CACHE)
+    return 0;
+
+    cache = (struct acpi_pptt_cache *) res;
+    while (cache) {
+    local_level++;
+
+    if ((local_level == level) &&
+    (cache->flags & ACPI_PPTT_CACHE_TYPE_VALID) &&
+    ((cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) == type)) {


Attributes have to be shifted:

(cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) >> 2


Hmmm, I'm not sure that is true, the top level function in this routine 
convert the "linux" constant to the ACPI version of that constant. In 
that case the "type" field is pre-shifted, so 

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Jeremy Linton]

Hi,

On 10/17/2017 08:25 AM, Tomasz Nowicki wrote:

Hi Jeremy,

I did second round of review and have some more comments, please see below:

On 12.10.2017 21:48, Jeremy Linton wrote:

ACPI 6.2 adds a new table, which describes how processing units
are related to each other in tree like fashion. Caches are
also sprinkled throughout the tree and describe the properties
of the caches in relation to other caches and processing units.

Add the code to parse the cache hierarchy and report the total
number of levels of cache for a given core using
acpi_find_last_cache_level() as well as fill out the individual
cores cache information with cache_setup_acpi() once the
cpu_cacheinfo structure has been populated by the arch specific
code.

Further, report peers in the topology using setup_acpi_cpu_topology()
to report a unique ID for each processing unit at a given level
in the tree. These unique id's can then be used to match related
processing units which exist as threads, COD (clusters
on die), within a given package, etc.

Signed-off-by: Jeremy Linton 
---
  drivers/acpi/pptt.c | 485 


  1 file changed, 485 insertions(+)
  create mode 100644 drivers/acpi/pptt.c

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
new file mode 100644
index ..c86715fed4a7
--- /dev/null
+++ b/drivers/acpi/pptt.c
@@ -0,1 +1,485 @@
+/*
+ * Copyright (C) 2017, ARM
+ *
+ * This program is free software; you can redistribute it and/or 
modify it

+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but 
WITHOUT

+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public 
License for

+ * more details.
+ *
+ * This file implements parsing of Processor Properties Topology 
Table (PPTT)
+ * which is optionally used to describe the processor and cache 
topology.

+ * Due to the relative pointers used throughout the table, this doesn't
+ * leverage the existing subtable parsing in the kernel.
+ */
+#define pr_fmt(fmt) "ACPI PPTT: " fmt
+
+#include 
+#include 
+#include 
+
+/*
+ * Given the PPTT table, find and verify that the subtable entry
+ * is located within the table
+ */
+static struct acpi_subtable_header *fetch_pptt_subtable(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    struct acpi_subtable_header *entry;
+
+    /* there isn't a subtable at reference 0 */
+    if (!pptt_ref)
+    return NULL;
+
+    if (pptt_ref + sizeof(struct acpi_subtable_header) > 
table_hdr->length)

+    return NULL;
+
+    entry = (struct acpi_subtable_header *)((u8 *)table_hdr + pptt_ref);
+
+    if (pptt_ref + entry->length > table_hdr->length)
+    return NULL;
+
+    return entry;
+}
+
+static struct acpi_pptt_processor *fetch_pptt_node(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    return (struct acpi_pptt_processor 
*)fetch_pptt_subtable(table_hdr, pptt_ref);

+}
+
+static struct acpi_pptt_cache *fetch_pptt_cache(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    return (struct acpi_pptt_cache *)fetch_pptt_subtable(table_hdr, 
pptt_ref);

+}
+
+static struct acpi_subtable_header *acpi_get_pptt_resource(
+    struct acpi_table_header *table_hdr,
+    struct acpi_pptt_processor *node, int resource)
+{
+    u32 ref;
+
+    if (resource >= node->number_of_priv_resources)
+    return NULL;
+
+    ref = *(u32 *)((u8 *)node + sizeof(struct acpi_pptt_processor) +
+  sizeof(u32) * resource);
+
+    return fetch_pptt_subtable(table_hdr, ref);
+}
+
+/*
+ * given a pptt resource, verify that it is a cache node, then walk
+ * down each level of caches, counting how many levels are found
+ * as well as checking the cache type (icache, dcache, unified). If a
+ * level & type match, then we set found, and continue the search.
+ * Once the entire cache branch has been walked return its max
+ * depth.
+ */
+static int acpi_pptt_walk_cache(struct acpi_table_header *table_hdr,
+    int local_level,
+    struct acpi_subtable_header *res,
+    struct acpi_pptt_cache **found,
+    int level, int type)
+{
+    struct acpi_pptt_cache *cache;
+
+    if (res->type != ACPI_PPTT_TYPE_CACHE)
+    return 0;
+
+    cache = (struct acpi_pptt_cache *) res;
+    while (cache) {
+    local_level++;
+
+    if ((local_level == level) &&
+    (cache->flags & ACPI_PPTT_CACHE_TYPE_VALID) &&
+    ((cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) == type)) {


Attributes have to be shifted:

(cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) >> 2


Hmmm, I'm not sure that is true, the top level function in this routine 
convert the "linux" constant to the ACPI version of that constant. In 
that case the "type" field is pre-shifted, so that it matches the 

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Tomasz Nowicki]

Hi Jeremy,

I did second round of review and have some more comments, please see below:

On 12.10.2017 21:48, Jeremy Linton wrote:

ACPI 6.2 adds a new table, which describes how processing units
are related to each other in tree like fashion. Caches are
also sprinkled throughout the tree and describe the properties
of the caches in relation to other caches and processing units.

Add the code to parse the cache hierarchy and report the total
number of levels of cache for a given core using
acpi_find_last_cache_level() as well as fill out the individual
cores cache information with cache_setup_acpi() once the
cpu_cacheinfo structure has been populated by the arch specific
code.

Further, report peers in the topology using setup_acpi_cpu_topology()
to report a unique ID for each processing unit at a given level
in the tree. These unique id's can then be used to match related
processing units which exist as threads, COD (clusters
on die), within a given package, etc.

Signed-off-by: Jeremy Linton 
---
  drivers/acpi/pptt.c | 485 
  1 file changed, 485 insertions(+)
  create mode 100644 drivers/acpi/pptt.c

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
new file mode 100644
index ..c86715fed4a7
--- /dev/null
+++ b/drivers/acpi/pptt.c
@@ -0,1 +1,485 @@
+/*
+ * Copyright (C) 2017, ARM
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * This file implements parsing of Processor Properties Topology Table (PPTT)
+ * which is optionally used to describe the processor and cache topology.
+ * Due to the relative pointers used throughout the table, this doesn't
+ * leverage the existing subtable parsing in the kernel.
+ */
+#define pr_fmt(fmt) "ACPI PPTT: " fmt
+
+#include 
+#include 
+#include 
+
+/*
+ * Given the PPTT table, find and verify that the subtable entry
+ * is located within the table
+ */
+static struct acpi_subtable_header *fetch_pptt_subtable(
+   struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+   struct acpi_subtable_header *entry;
+
+   /* there isn't a subtable at reference 0 */
+   if (!pptt_ref)
+   return NULL;
+
+   if (pptt_ref + sizeof(struct acpi_subtable_header) > table_hdr->length)
+   return NULL;
+
+   entry = (struct acpi_subtable_header *)((u8 *)table_hdr + pptt_ref);
+
+   if (pptt_ref + entry->length > table_hdr->length)
+   return NULL;
+
+   return entry;
+}
+
+static struct acpi_pptt_processor *fetch_pptt_node(
+   struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+   return (struct acpi_pptt_processor *)fetch_pptt_subtable(table_hdr, 
pptt_ref);
+}
+
+static struct acpi_pptt_cache *fetch_pptt_cache(
+   struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+   return (struct acpi_pptt_cache *)fetch_pptt_subtable(table_hdr, 
pptt_ref);
+}
+
+static struct acpi_subtable_header *acpi_get_pptt_resource(
+   struct acpi_table_header *table_hdr,
+   struct acpi_pptt_processor *node, int resource)
+{
+   u32 ref;
+
+   if (resource >= node->number_of_priv_resources)
+   return NULL;
+
+   ref = *(u32 *)((u8 *)node + sizeof(struct acpi_pptt_processor) +
+ sizeof(u32) * resource);
+
+   return fetch_pptt_subtable(table_hdr, ref);
+}
+
+/*
+ * given a pptt resource, verify that it is a cache node, then walk
+ * down each level of caches, counting how many levels are found
+ * as well as checking the cache type (icache, dcache, unified). If a
+ * level & type match, then we set found, and continue the search.
+ * Once the entire cache branch has been walked return its max
+ * depth.
+ */
+static int acpi_pptt_walk_cache(struct acpi_table_header *table_hdr,
+   int local_level,
+   struct acpi_subtable_header *res,
+   struct acpi_pptt_cache **found,
+   int level, int type)
+{
+   struct acpi_pptt_cache *cache;
+
+   if (res->type != ACPI_PPTT_TYPE_CACHE)
+   return 0;
+
+   cache = (struct acpi_pptt_cache *) res;
+   while (cache) {
+   local_level++;
+
+   if ((local_level == level) &&
+   (cache->flags & ACPI_PPTT_CACHE_TYPE_VALID) &&
+   ((cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) == type)) {


Attributes have to be shifted:

(cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) >> 2


+   if (*found != NULL)
+ 

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Tomasz Nowicki]

Hi Jeremy,

I did second round of review and have some more comments, please see below:

On 12.10.2017 21:48, Jeremy Linton wrote:

ACPI 6.2 adds a new table, which describes how processing units
are related to each other in tree like fashion. Caches are
also sprinkled throughout the tree and describe the properties
of the caches in relation to other caches and processing units.

Add the code to parse the cache hierarchy and report the total
number of levels of cache for a given core using
acpi_find_last_cache_level() as well as fill out the individual
cores cache information with cache_setup_acpi() once the
cpu_cacheinfo structure has been populated by the arch specific
code.

Further, report peers in the topology using setup_acpi_cpu_topology()
to report a unique ID for each processing unit at a given level
in the tree. These unique id's can then be used to match related
processing units which exist as threads, COD (clusters
on die), within a given package, etc.

Signed-off-by: Jeremy Linton 
---
  drivers/acpi/pptt.c | 485 
  1 file changed, 485 insertions(+)
  create mode 100644 drivers/acpi/pptt.c

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
new file mode 100644
index ..c86715fed4a7
--- /dev/null
+++ b/drivers/acpi/pptt.c
@@ -0,1 +1,485 @@
+/*
+ * Copyright (C) 2017, ARM
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * This file implements parsing of Processor Properties Topology Table (PPTT)
+ * which is optionally used to describe the processor and cache topology.
+ * Due to the relative pointers used throughout the table, this doesn't
+ * leverage the existing subtable parsing in the kernel.
+ */
+#define pr_fmt(fmt) "ACPI PPTT: " fmt
+
+#include 
+#include 
+#include 
+
+/*
+ * Given the PPTT table, find and verify that the subtable entry
+ * is located within the table
+ */
+static struct acpi_subtable_header *fetch_pptt_subtable(
+   struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+   struct acpi_subtable_header *entry;
+
+   /* there isn't a subtable at reference 0 */
+   if (!pptt_ref)
+   return NULL;
+
+   if (pptt_ref + sizeof(struct acpi_subtable_header) > table_hdr->length)
+   return NULL;
+
+   entry = (struct acpi_subtable_header *)((u8 *)table_hdr + pptt_ref);
+
+   if (pptt_ref + entry->length > table_hdr->length)
+   return NULL;
+
+   return entry;
+}
+
+static struct acpi_pptt_processor *fetch_pptt_node(
+   struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+   return (struct acpi_pptt_processor *)fetch_pptt_subtable(table_hdr, 
pptt_ref);
+}
+
+static struct acpi_pptt_cache *fetch_pptt_cache(
+   struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+   return (struct acpi_pptt_cache *)fetch_pptt_subtable(table_hdr, 
pptt_ref);
+}
+
+static struct acpi_subtable_header *acpi_get_pptt_resource(
+   struct acpi_table_header *table_hdr,
+   struct acpi_pptt_processor *node, int resource)
+{
+   u32 ref;
+
+   if (resource >= node->number_of_priv_resources)
+   return NULL;
+
+   ref = *(u32 *)((u8 *)node + sizeof(struct acpi_pptt_processor) +
+ sizeof(u32) * resource);
+
+   return fetch_pptt_subtable(table_hdr, ref);
+}
+
+/*
+ * given a pptt resource, verify that it is a cache node, then walk
+ * down each level of caches, counting how many levels are found
+ * as well as checking the cache type (icache, dcache, unified). If a
+ * level & type match, then we set found, and continue the search.
+ * Once the entire cache branch has been walked return its max
+ * depth.
+ */
+static int acpi_pptt_walk_cache(struct acpi_table_header *table_hdr,
+   int local_level,
+   struct acpi_subtable_header *res,
+   struct acpi_pptt_cache **found,
+   int level, int type)
+{
+   struct acpi_pptt_cache *cache;
+
+   if (res->type != ACPI_PPTT_TYPE_CACHE)
+   return 0;
+
+   cache = (struct acpi_pptt_cache *) res;
+   while (cache) {
+   local_level++;
+
+   if ((local_level == level) &&
+   (cache->flags & ACPI_PPTT_CACHE_TYPE_VALID) &&
+   ((cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) == type)) {


Attributes have to be shifted:

(cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) >> 2


+   if (*found != NULL)
+   

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[John Garry]

On 12/10/2017 20:48, Jeremy Linton wrote:

ACPI 6.2 adds a new table, which describes how processing units
are related to each other in tree like fashion. Caches are
also sprinkled throughout the tree and describe the properties
of the caches in relation to other caches and processing units.

Add the code to parse the cache hierarchy and report the total
number of levels of cache for a given core using
acpi_find_last_cache_level() as well as fill out the individual
cores cache information with cache_setup_acpi() once the
cpu_cacheinfo structure has been populated by the arch specific
code.

Further, report peers in the topology using setup_acpi_cpu_topology()
to report a unique ID for each processing unit at a given level
in the tree. These unique id's can then be used to match related
processing units which exist as threads, COD (clusters
on die), within a given package, etc.



As already commented, there are many lines over 80 characters.

And so far I only really looked at cpu topology part.


Signed-off-by: Jeremy Linton 
---
 drivers/acpi/pptt.c | 485 
 1 file changed, 485 insertions(+)
 create mode 100644 drivers/acpi/pptt.c

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
new file mode 100644
index ..c86715fed4a7
--- /dev/null
+++ b/drivers/acpi/pptt.c
@@ -0,1 +1,485 @@
+/*
+ * Copyright (C) 2017, ARM
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * This file implements parsing of Processor Properties Topology Table (PPTT)
+ * which is optionally used to describe the processor and cache topology.
+ * Due to the relative pointers used throughout the table, this doesn't
+ * leverage the existing subtable parsing in the kernel.
+ */
+#define pr_fmt(fmt) "ACPI PPTT: " fmt
+
+#include 
+#include 
+#include 
+
+/*
+ * Given the PPTT table, find and verify that the subtable entry
+ * is located within the table
+ */
+static struct acpi_subtable_header *fetch_pptt_subtable(
+   struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+   struct acpi_subtable_header *entry;
+
+   /* there isn't a subtable at reference 0 */
+   if (!pptt_ref)
+   return NULL;
+
+   if (pptt_ref + sizeof(struct acpi_subtable_header) > table_hdr->length)
+   return NULL;
+
+   entry = (struct acpi_subtable_header *)((u8 *)table_hdr + pptt_ref);
+
+   if (pptt_ref + entry->length > table_hdr->length)
+   return NULL;
+
+   return entry;
+}
+
+static struct acpi_pptt_processor *fetch_pptt_node(
+   struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+   return (struct acpi_pptt_processor *)fetch_pptt_subtable(table_hdr, 
pptt_ref);
+}
+
+static struct acpi_pptt_cache *fetch_pptt_cache(
+   struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+   return (struct acpi_pptt_cache *)fetch_pptt_subtable(table_hdr, 
pptt_ref);
+}
+
+static struct acpi_subtable_header *acpi_get_pptt_resource(
+   struct acpi_table_header *table_hdr,
+   struct acpi_pptt_processor *node, int resource)
+{
+   u32 ref;
+
+   if (resource >= node->number_of_priv_resources)
+   return NULL;
+
+   ref = *(u32 *)((u8 *)node + sizeof(struct acpi_pptt_processor) +
+ sizeof(u32) * resource);
+
+   return fetch_pptt_subtable(table_hdr, ref);
+}
+
+/*
+ * given a pptt resource, verify that it is a cache node, then walk


/s/given/Given/


+ * down each level of caches, counting how many levels are found
+ * as well as checking the cache type (icache, dcache, unified). If a
+ * level & type match, then we set found, and continue the search.
+ * Once the entire cache branch has been walked return its max
+ * depth.
+ */
+static int acpi_pptt_walk_cache(struct acpi_table_header *table_hdr,
+   int local_level,
+   struct acpi_subtable_header *res,
+   struct acpi_pptt_cache **found,
+   int level, int type)
+{
+   struct acpi_pptt_cache *cache;
+
+   if (res->type != ACPI_PPTT_TYPE_CACHE)
+   return 0;
+
+   cache = (struct acpi_pptt_cache *) res;


please remove whitespace before res


+   while (cache) {
+   local_level++;
+
+   if ((local_level == level) &&
+   (cache->flags & ACPI_PPTT_CACHE_TYPE_VALID) &&
+   ((cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) == type)) {
+   if (*found != NULL)
+

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[John Garry]

On 12/10/2017 20:48, Jeremy Linton wrote:

ACPI 6.2 adds a new table, which describes how processing units
are related to each other in tree like fashion. Caches are
also sprinkled throughout the tree and describe the properties
of the caches in relation to other caches and processing units.

Add the code to parse the cache hierarchy and report the total
number of levels of cache for a given core using
acpi_find_last_cache_level() as well as fill out the individual
cores cache information with cache_setup_acpi() once the
cpu_cacheinfo structure has been populated by the arch specific
code.

Further, report peers in the topology using setup_acpi_cpu_topology()
to report a unique ID for each processing unit at a given level
in the tree. These unique id's can then be used to match related
processing units which exist as threads, COD (clusters
on die), within a given package, etc.



As already commented, there are many lines over 80 characters.

And so far I only really looked at cpu topology part.


Signed-off-by: Jeremy Linton 
---
 drivers/acpi/pptt.c | 485 
 1 file changed, 485 insertions(+)
 create mode 100644 drivers/acpi/pptt.c

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
new file mode 100644
index ..c86715fed4a7
--- /dev/null
+++ b/drivers/acpi/pptt.c
@@ -0,1 +1,485 @@
+/*
+ * Copyright (C) 2017, ARM
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * This file implements parsing of Processor Properties Topology Table (PPTT)
+ * which is optionally used to describe the processor and cache topology.
+ * Due to the relative pointers used throughout the table, this doesn't
+ * leverage the existing subtable parsing in the kernel.
+ */
+#define pr_fmt(fmt) "ACPI PPTT: " fmt
+
+#include 
+#include 
+#include 
+
+/*
+ * Given the PPTT table, find and verify that the subtable entry
+ * is located within the table
+ */
+static struct acpi_subtable_header *fetch_pptt_subtable(
+   struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+   struct acpi_subtable_header *entry;
+
+   /* there isn't a subtable at reference 0 */
+   if (!pptt_ref)
+   return NULL;
+
+   if (pptt_ref + sizeof(struct acpi_subtable_header) > table_hdr->length)
+   return NULL;
+
+   entry = (struct acpi_subtable_header *)((u8 *)table_hdr + pptt_ref);
+
+   if (pptt_ref + entry->length > table_hdr->length)
+   return NULL;
+
+   return entry;
+}
+
+static struct acpi_pptt_processor *fetch_pptt_node(
+   struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+   return (struct acpi_pptt_processor *)fetch_pptt_subtable(table_hdr, 
pptt_ref);
+}
+
+static struct acpi_pptt_cache *fetch_pptt_cache(
+   struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+   return (struct acpi_pptt_cache *)fetch_pptt_subtable(table_hdr, 
pptt_ref);
+}
+
+static struct acpi_subtable_header *acpi_get_pptt_resource(
+   struct acpi_table_header *table_hdr,
+   struct acpi_pptt_processor *node, int resource)
+{
+   u32 ref;
+
+   if (resource >= node->number_of_priv_resources)
+   return NULL;
+
+   ref = *(u32 *)((u8 *)node + sizeof(struct acpi_pptt_processor) +
+ sizeof(u32) * resource);
+
+   return fetch_pptt_subtable(table_hdr, ref);
+}
+
+/*
+ * given a pptt resource, verify that it is a cache node, then walk


/s/given/Given/


+ * down each level of caches, counting how many levels are found
+ * as well as checking the cache type (icache, dcache, unified). If a
+ * level & type match, then we set found, and continue the search.
+ * Once the entire cache branch has been walked return its max
+ * depth.
+ */
+static int acpi_pptt_walk_cache(struct acpi_table_header *table_hdr,
+   int local_level,
+   struct acpi_subtable_header *res,
+   struct acpi_pptt_cache **found,
+   int level, int type)
+{
+   struct acpi_pptt_cache *cache;
+
+   if (res->type != ACPI_PPTT_TYPE_CACHE)
+   return 0;
+
+   cache = (struct acpi_pptt_cache *) res;


please remove whitespace before res


+   while (cache) {
+   local_level++;
+
+   if ((local_level == level) &&
+   (cache->flags & ACPI_PPTT_CACHE_TYPE_VALID) &&
+   ((cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) == type)) {
+   if (*found != NULL)
+   

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Jeremy Linton]

Hi,


Thanks for spending the time to take a look at this.


On 10/13/2017 04:56 AM, Julien Thierry wrote:

Hi Jeremy,

Please see below some suggestions.

On 12/10/17 20:48, Jeremy Linton wrote:

ACPI 6.2 adds a new table, which describes how processing units
are related to each other in tree like fashion. Caches are
also sprinkled throughout the tree and describe the properties
of the caches in relation to other caches and processing units.

Add the code to parse the cache hierarchy and report the total
number of levels of cache for a given core using
acpi_find_last_cache_level() as well as fill out the individual
cores cache information with cache_setup_acpi() once the
cpu_cacheinfo structure has been populated by the arch specific
code.

Further, report peers in the topology using setup_acpi_cpu_topology()
to report a unique ID for each processing unit at a given level
in the tree. These unique id's can then be used to match related
processing units which exist as threads, COD (clusters
on die), within a given package, etc.

Signed-off-by: Jeremy Linton 
---
  drivers/acpi/pptt.c | 485 


  1 file changed, 485 insertions(+)
  create mode 100644 drivers/acpi/pptt.c

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
new file mode 100644
index ..c86715fed4a7
--- /dev/null
+++ b/drivers/acpi/pptt.c
@@ -0,1 +1,485 @@
+/*
+ * Copyright (C) 2017, ARM
+ *
+ * This program is free software; you can redistribute it and/or 
modify it

+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but 
WITHOUT

+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public 
License for

+ * more details.
+ *
+ * This file implements parsing of Processor Properties Topology 
Table (PPTT)
+ * which is optionally used to describe the processor and cache 
topology.

+ * Due to the relative pointers used throughout the table, this doesn't
+ * leverage the existing subtable parsing in the kernel.
+ */
+#define pr_fmt(fmt) "ACPI PPTT: " fmt
+
+#include 
+#include 
+#include 
+
+/*
+ * Given the PPTT table, find and verify that the subtable entry
+ * is located within the table
+ */
+static struct acpi_subtable_header *fetch_pptt_subtable(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    struct acpi_subtable_header *entry;
+
+    /* there isn't a subtable at reference 0 */
+    if (!pptt_ref)
+    return NULL;


Seeing the usage of pptt_ref to retrieve the subtable, would the 
following be a more accurate check?


 if (pptt_ref < sizeof(struct acpi_table_header))
     return NULL;


Yes, that makes it better match the comment, and I guess tightens up the 
sanity checking. The original intention was just to catch null 
references that were encoded as parent/etc fields.





+
+    if (pptt_ref + sizeof(struct acpi_subtable_header) > 
table_hdr->length)

+    return NULL;
+
+    entry = (struct acpi_subtable_header *)((u8 *)table_hdr + pptt_ref);
+
+    if (pptt_ref + entry->length > table_hdr->length)
+    return NULL;
+
+    return entry;
+}
+
+static struct acpi_pptt_processor *fetch_pptt_node(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    return (struct acpi_pptt_processor 
*)fetch_pptt_subtable(table_hdr, pptt_ref);

+}
+
+static struct acpi_pptt_cache *fetch_pptt_cache(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    return (struct acpi_pptt_cache *)fetch_pptt_subtable(table_hdr, 
pptt_ref);

+}
+
+static struct acpi_subtable_header *acpi_get_pptt_resource(
+    struct acpi_table_header *table_hdr,
+    struct acpi_pptt_processor *node, int resource)
+{
+    u32 ref;
+
+    if (resource >= node->number_of_priv_resources)
+    return NULL;
+
+    ref = *(u32 *)((u8 *)node + sizeof(struct acpi_pptt_processor) +
+  sizeof(u32) * resource);
+


I think this can be simplified as:

 ref = *((u32 *)(node + 1) + resource);


I think Thomasz had a better suggestion with regard to ACPI_ADD_PTR() 
for avoiding the explicit pointer math, although it may not be that 
clean either because it doesn't fit 1:1 with the macro at the moment, 
maybe i'm doing it wrong...





+    return fetch_pptt_subtable(table_hdr, ref);
+}
+
+/*
+ * given a pptt resource, verify that it is a cache node, then walk
+ * down each level of caches, counting how many levels are found
+ * as well as checking the cache type (icache, dcache, unified). If a
+ * level & type match, then we set found, and continue the search.
+ * Once the entire cache branch has been walked return its max
+ * depth.
+ */
+static int acpi_pptt_walk_cache(struct acpi_table_header *table_hdr,
+    int local_level,
+    struct acpi_subtable_header *res,
+    

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Jeremy Linton]

Hi,


Thanks for spending the time to take a look at this.


On 10/13/2017 04:56 AM, Julien Thierry wrote:

Hi Jeremy,

Please see below some suggestions.

On 12/10/17 20:48, Jeremy Linton wrote:

ACPI 6.2 adds a new table, which describes how processing units
are related to each other in tree like fashion. Caches are
also sprinkled throughout the tree and describe the properties
of the caches in relation to other caches and processing units.

Add the code to parse the cache hierarchy and report the total
number of levels of cache for a given core using
acpi_find_last_cache_level() as well as fill out the individual
cores cache information with cache_setup_acpi() once the
cpu_cacheinfo structure has been populated by the arch specific
code.

Further, report peers in the topology using setup_acpi_cpu_topology()
to report a unique ID for each processing unit at a given level
in the tree. These unique id's can then be used to match related
processing units which exist as threads, COD (clusters
on die), within a given package, etc.

Signed-off-by: Jeremy Linton 
---
  drivers/acpi/pptt.c | 485 


  1 file changed, 485 insertions(+)
  create mode 100644 drivers/acpi/pptt.c

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
new file mode 100644
index ..c86715fed4a7
--- /dev/null
+++ b/drivers/acpi/pptt.c
@@ -0,1 +1,485 @@
+/*
+ * Copyright (C) 2017, ARM
+ *
+ * This program is free software; you can redistribute it and/or 
modify it

+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but 
WITHOUT

+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public 
License for

+ * more details.
+ *
+ * This file implements parsing of Processor Properties Topology 
Table (PPTT)
+ * which is optionally used to describe the processor and cache 
topology.

+ * Due to the relative pointers used throughout the table, this doesn't
+ * leverage the existing subtable parsing in the kernel.
+ */
+#define pr_fmt(fmt) "ACPI PPTT: " fmt
+
+#include 
+#include 
+#include 
+
+/*
+ * Given the PPTT table, find and verify that the subtable entry
+ * is located within the table
+ */
+static struct acpi_subtable_header *fetch_pptt_subtable(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    struct acpi_subtable_header *entry;
+
+    /* there isn't a subtable at reference 0 */
+    if (!pptt_ref)
+    return NULL;


Seeing the usage of pptt_ref to retrieve the subtable, would the 
following be a more accurate check?


 if (pptt_ref < sizeof(struct acpi_table_header))
     return NULL;


Yes, that makes it better match the comment, and I guess tightens up the 
sanity checking. The original intention was just to catch null 
references that were encoded as parent/etc fields.





+
+    if (pptt_ref + sizeof(struct acpi_subtable_header) > 
table_hdr->length)

+    return NULL;
+
+    entry = (struct acpi_subtable_header *)((u8 *)table_hdr + pptt_ref);
+
+    if (pptt_ref + entry->length > table_hdr->length)
+    return NULL;
+
+    return entry;
+}
+
+static struct acpi_pptt_processor *fetch_pptt_node(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    return (struct acpi_pptt_processor 
*)fetch_pptt_subtable(table_hdr, pptt_ref);

+}
+
+static struct acpi_pptt_cache *fetch_pptt_cache(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    return (struct acpi_pptt_cache *)fetch_pptt_subtable(table_hdr, 
pptt_ref);

+}
+
+static struct acpi_subtable_header *acpi_get_pptt_resource(
+    struct acpi_table_header *table_hdr,
+    struct acpi_pptt_processor *node, int resource)
+{
+    u32 ref;
+
+    if (resource >= node->number_of_priv_resources)
+    return NULL;
+
+    ref = *(u32 *)((u8 *)node + sizeof(struct acpi_pptt_processor) +
+  sizeof(u32) * resource);
+


I think this can be simplified as:

 ref = *((u32 *)(node + 1) + resource);


I think Thomasz had a better suggestion with regard to ACPI_ADD_PTR() 
for avoiding the explicit pointer math, although it may not be that 
clean either because it doesn't fit 1:1 with the macro at the moment, 
maybe i'm doing it wrong...





+    return fetch_pptt_subtable(table_hdr, ref);
+}
+
+/*
+ * given a pptt resource, verify that it is a cache node, then walk
+ * down each level of caches, counting how many levels are found
+ * as well as checking the cache type (icache, dcache, unified). If a
+ * level & type match, then we set found, and continue the search.
+ * Once the entire cache branch has been walked return its max
+ * depth.
+ */
+static int acpi_pptt_walk_cache(struct acpi_table_header *table_hdr,
+    int local_level,
+    struct acpi_subtable_header *res,
+    struct acpi_pptt_cache 

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Jeremy Linton]

Hi,

On 10/13/2017 09:23 AM, tn wrote:

Hi Jeremy,

On 12.10.2017 21:48, Jeremy Linton wrote:

ACPI 6.2 adds a new table, which describes how processing units
are related to each other in tree like fashion. Caches are
also sprinkled throughout the tree and describe the properties
of the caches in relation to other caches and processing units.

Add the code to parse the cache hierarchy and report the total
number of levels of cache for a given core using
acpi_find_last_cache_level() as well as fill out the individual
cores cache information with cache_setup_acpi() once the
cpu_cacheinfo structure has been populated by the arch specific
code.

Further, report peers in the topology using setup_acpi_cpu_topology()
to report a unique ID for each processing unit at a given level
in the tree. These unique id's can then be used to match related
processing units which exist as threads, COD (clusters
on die), within a given package, etc.

Signed-off-by: Jeremy Linton 
---
  drivers/acpi/pptt.c | 485 


  1 file changed, 485 insertions(+)
  create mode 100644 drivers/acpi/pptt.c

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
new file mode 100644
index ..c86715fed4a7
--- /dev/null
+++ b/drivers/acpi/pptt.c
@@ -0,1 +1,485 @@
+/*
+ * Copyright (C) 2017, ARM
+ *
+ * This program is free software; you can redistribute it and/or 
modify it

+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but 
WITHOUT

+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public 
License for

+ * more details.
+ *
+ * This file implements parsing of Processor Properties Topology 
Table (PPTT)
+ * which is optionally used to describe the processor and cache 
topology.

+ * Due to the relative pointers used throughout the table, this doesn't
+ * leverage the existing subtable parsing in the kernel.
+ */
+#define pr_fmt(fmt) "ACPI PPTT: " fmt
+
+#include 
+#include 
+#include 
+
+/*
+ * Given the PPTT table, find and verify that the subtable entry
+ * is located within the table
+ */
+static struct acpi_subtable_header *fetch_pptt_subtable(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    struct acpi_subtable_header *entry;
+
+    /* there isn't a subtable at reference 0 */
+    if (!pptt_ref)
+    return NULL;
+
+    if (pptt_ref + sizeof(struct acpi_subtable_header) > 
table_hdr->length)

+    return NULL;
+
+    entry = (struct acpi_subtable_header *)((u8 *)table_hdr + pptt_ref);


You can use ACPI_ADD_PTR() here.


Hmmm, that is a useful macro.





+
+    if (pptt_ref + entry->length > table_hdr->length)
+    return NULL;
+
+    return entry;
+}
+
+static struct acpi_pptt_processor *fetch_pptt_node(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    return (struct acpi_pptt_processor 
*)fetch_pptt_subtable(table_hdr, pptt_ref);

+}
+
+static struct acpi_pptt_cache *fetch_pptt_cache(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    return (struct acpi_pptt_cache *)fetch_pptt_subtable(table_hdr, 
pptt_ref);

+}
+
+static struct acpi_subtable_header *acpi_get_pptt_resource(
+    struct acpi_table_header *table_hdr,
+    struct acpi_pptt_processor *node, int resource)
+{
+    u32 ref;
+
+    if (resource >= node->number_of_priv_resources)
+    return NULL;
+
+    ref = *(u32 *)((u8 *)node + sizeof(struct acpi_pptt_processor) +
+  sizeof(u32) * resource);


ACPI_ADD_PTR()


+
+    return fetch_pptt_subtable(table_hdr, ref);
+}
+
+/*
+ * given a pptt resource, verify that it is a cache node, then walk
+ * down each level of caches, counting how many levels are found
+ * as well as checking the cache type (icache, dcache, unified). If a
+ * level & type match, then we set found, and continue the search.
+ * Once the entire cache branch has been walked return its max
+ * depth.
+ */
+static int acpi_pptt_walk_cache(struct acpi_table_header *table_hdr,
+    int local_level,
+    struct acpi_subtable_header *res,
+    struct acpi_pptt_cache **found,
+    int level, int type)
+{
+    struct acpi_pptt_cache *cache;
+
+    if (res->type != ACPI_PPTT_TYPE_CACHE)
+    return 0;
+
+    cache = (struct acpi_pptt_cache *) res;
+    while (cache) {
+    local_level++;
+
+    if ((local_level == level) &&
+    (cache->flags & ACPI_PPTT_CACHE_TYPE_VALID) &&
+    ((cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) == type)) {
+    if (*found != NULL)
+    pr_err("Found duplicate cache level/type unable to 
determine uniqueness\n");

+
+    pr_debug("Found cache @ level %d\n", level);
+    *found = cache;
+    /*
+ * continue looking at this 

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Jeremy Linton]

Hi,

On 10/13/2017 09:23 AM, tn wrote:

Hi Jeremy,

On 12.10.2017 21:48, Jeremy Linton wrote:

ACPI 6.2 adds a new table, which describes how processing units
are related to each other in tree like fashion. Caches are
also sprinkled throughout the tree and describe the properties
of the caches in relation to other caches and processing units.

Add the code to parse the cache hierarchy and report the total
number of levels of cache for a given core using
acpi_find_last_cache_level() as well as fill out the individual
cores cache information with cache_setup_acpi() once the
cpu_cacheinfo structure has been populated by the arch specific
code.

Further, report peers in the topology using setup_acpi_cpu_topology()
to report a unique ID for each processing unit at a given level
in the tree. These unique id's can then be used to match related
processing units which exist as threads, COD (clusters
on die), within a given package, etc.

Signed-off-by: Jeremy Linton 
---
  drivers/acpi/pptt.c | 485 


  1 file changed, 485 insertions(+)
  create mode 100644 drivers/acpi/pptt.c

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
new file mode 100644
index ..c86715fed4a7
--- /dev/null
+++ b/drivers/acpi/pptt.c
@@ -0,1 +1,485 @@
+/*
+ * Copyright (C) 2017, ARM
+ *
+ * This program is free software; you can redistribute it and/or 
modify it

+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but 
WITHOUT

+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public 
License for

+ * more details.
+ *
+ * This file implements parsing of Processor Properties Topology 
Table (PPTT)
+ * which is optionally used to describe the processor and cache 
topology.

+ * Due to the relative pointers used throughout the table, this doesn't
+ * leverage the existing subtable parsing in the kernel.
+ */
+#define pr_fmt(fmt) "ACPI PPTT: " fmt
+
+#include 
+#include 
+#include 
+
+/*
+ * Given the PPTT table, find and verify that the subtable entry
+ * is located within the table
+ */
+static struct acpi_subtable_header *fetch_pptt_subtable(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    struct acpi_subtable_header *entry;
+
+    /* there isn't a subtable at reference 0 */
+    if (!pptt_ref)
+    return NULL;
+
+    if (pptt_ref + sizeof(struct acpi_subtable_header) > 
table_hdr->length)

+    return NULL;
+
+    entry = (struct acpi_subtable_header *)((u8 *)table_hdr + pptt_ref);


You can use ACPI_ADD_PTR() here.


Hmmm, that is a useful macro.





+
+    if (pptt_ref + entry->length > table_hdr->length)
+    return NULL;
+
+    return entry;
+}
+
+static struct acpi_pptt_processor *fetch_pptt_node(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    return (struct acpi_pptt_processor 
*)fetch_pptt_subtable(table_hdr, pptt_ref);

+}
+
+static struct acpi_pptt_cache *fetch_pptt_cache(
+    struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+    return (struct acpi_pptt_cache *)fetch_pptt_subtable(table_hdr, 
pptt_ref);

+}
+
+static struct acpi_subtable_header *acpi_get_pptt_resource(
+    struct acpi_table_header *table_hdr,
+    struct acpi_pptt_processor *node, int resource)
+{
+    u32 ref;
+
+    if (resource >= node->number_of_priv_resources)
+    return NULL;
+
+    ref = *(u32 *)((u8 *)node + sizeof(struct acpi_pptt_processor) +
+  sizeof(u32) * resource);


ACPI_ADD_PTR()


+
+    return fetch_pptt_subtable(table_hdr, ref);
+}
+
+/*
+ * given a pptt resource, verify that it is a cache node, then walk
+ * down each level of caches, counting how many levels are found
+ * as well as checking the cache type (icache, dcache, unified). If a
+ * level & type match, then we set found, and continue the search.
+ * Once the entire cache branch has been walked return its max
+ * depth.
+ */
+static int acpi_pptt_walk_cache(struct acpi_table_header *table_hdr,
+    int local_level,
+    struct acpi_subtable_header *res,
+    struct acpi_pptt_cache **found,
+    int level, int type)
+{
+    struct acpi_pptt_cache *cache;
+
+    if (res->type != ACPI_PPTT_TYPE_CACHE)
+    return 0;
+
+    cache = (struct acpi_pptt_cache *) res;
+    while (cache) {
+    local_level++;
+
+    if ((local_level == level) &&
+    (cache->flags & ACPI_PPTT_CACHE_TYPE_VALID) &&
+    ((cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) == type)) {
+    if (*found != NULL)
+    pr_err("Found duplicate cache level/type unable to 
determine uniqueness\n");

+
+    pr_debug("Found cache @ level %d\n", level);
+    *found = cache;
+    /*
+ * continue looking at this node's resource list
+

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[tn]

Hi Jeremy,

On 12.10.2017 21:48, Jeremy Linton wrote:

ACPI 6.2 adds a new table, which describes how processing units
are related to each other in tree like fashion. Caches are
also sprinkled throughout the tree and describe the properties
of the caches in relation to other caches and processing units.

Add the code to parse the cache hierarchy and report the total
number of levels of cache for a given core using
acpi_find_last_cache_level() as well as fill out the individual
cores cache information with cache_setup_acpi() once the
cpu_cacheinfo structure has been populated by the arch specific
code.

Further, report peers in the topology using setup_acpi_cpu_topology()
to report a unique ID for each processing unit at a given level
in the tree. These unique id's can then be used to match related
processing units which exist as threads, COD (clusters
on die), within a given package, etc.

Signed-off-by: Jeremy Linton 
---
  drivers/acpi/pptt.c | 485 
  1 file changed, 485 insertions(+)
  create mode 100644 drivers/acpi/pptt.c

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
new file mode 100644
index ..c86715fed4a7
--- /dev/null
+++ b/drivers/acpi/pptt.c
@@ -0,1 +1,485 @@
+/*
+ * Copyright (C) 2017, ARM
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * This file implements parsing of Processor Properties Topology Table (PPTT)
+ * which is optionally used to describe the processor and cache topology.
+ * Due to the relative pointers used throughout the table, this doesn't
+ * leverage the existing subtable parsing in the kernel.
+ */
+#define pr_fmt(fmt) "ACPI PPTT: " fmt
+
+#include 
+#include 
+#include 
+
+/*
+ * Given the PPTT table, find and verify that the subtable entry
+ * is located within the table
+ */
+static struct acpi_subtable_header *fetch_pptt_subtable(
+   struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+   struct acpi_subtable_header *entry;
+
+   /* there isn't a subtable at reference 0 */
+   if (!pptt_ref)
+   return NULL;
+
+   if (pptt_ref + sizeof(struct acpi_subtable_header) > table_hdr->length)
+   return NULL;
+
+   entry = (struct acpi_subtable_header *)((u8 *)table_hdr + pptt_ref);


You can use ACPI_ADD_PTR() here.


+
+   if (pptt_ref + entry->length > table_hdr->length)
+   return NULL;
+
+   return entry;
+}
+
+static struct acpi_pptt_processor *fetch_pptt_node(
+   struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+   return (struct acpi_pptt_processor *)fetch_pptt_subtable(table_hdr, 
pptt_ref);
+}
+
+static struct acpi_pptt_cache *fetch_pptt_cache(
+   struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+   return (struct acpi_pptt_cache *)fetch_pptt_subtable(table_hdr, 
pptt_ref);
+}
+
+static struct acpi_subtable_header *acpi_get_pptt_resource(
+   struct acpi_table_header *table_hdr,
+   struct acpi_pptt_processor *node, int resource)
+{
+   u32 ref;
+
+   if (resource >= node->number_of_priv_resources)
+   return NULL;
+
+   ref = *(u32 *)((u8 *)node + sizeof(struct acpi_pptt_processor) +
+ sizeof(u32) * resource);


ACPI_ADD_PTR()


+
+   return fetch_pptt_subtable(table_hdr, ref);
+}
+
+/*
+ * given a pptt resource, verify that it is a cache node, then walk
+ * down each level of caches, counting how many levels are found
+ * as well as checking the cache type (icache, dcache, unified). If a
+ * level & type match, then we set found, and continue the search.
+ * Once the entire cache branch has been walked return its max
+ * depth.
+ */
+static int acpi_pptt_walk_cache(struct acpi_table_header *table_hdr,
+   int local_level,
+   struct acpi_subtable_header *res,
+   struct acpi_pptt_cache **found,
+   int level, int type)
+{
+   struct acpi_pptt_cache *cache;
+
+   if (res->type != ACPI_PPTT_TYPE_CACHE)
+   return 0;
+
+   cache = (struct acpi_pptt_cache *) res;
+   while (cache) {
+   local_level++;
+
+   if ((local_level == level) &&
+   (cache->flags & ACPI_PPTT_CACHE_TYPE_VALID) &&
+   ((cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) == type)) {
+   if (*found != NULL)
+   pr_err("Found duplicate cache level/type unable to 
determine uniqueness\n");
+
+   

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[tn]

Hi Jeremy,

On 12.10.2017 21:48, Jeremy Linton wrote:

ACPI 6.2 adds a new table, which describes how processing units
are related to each other in tree like fashion. Caches are
also sprinkled throughout the tree and describe the properties
of the caches in relation to other caches and processing units.

Add the code to parse the cache hierarchy and report the total
number of levels of cache for a given core using
acpi_find_last_cache_level() as well as fill out the individual
cores cache information with cache_setup_acpi() once the
cpu_cacheinfo structure has been populated by the arch specific
code.

Further, report peers in the topology using setup_acpi_cpu_topology()
to report a unique ID for each processing unit at a given level
in the tree. These unique id's can then be used to match related
processing units which exist as threads, COD (clusters
on die), within a given package, etc.

Signed-off-by: Jeremy Linton 
---
  drivers/acpi/pptt.c | 485 
  1 file changed, 485 insertions(+)
  create mode 100644 drivers/acpi/pptt.c

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
new file mode 100644
index ..c86715fed4a7
--- /dev/null
+++ b/drivers/acpi/pptt.c
@@ -0,1 +1,485 @@
+/*
+ * Copyright (C) 2017, ARM
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * This file implements parsing of Processor Properties Topology Table (PPTT)
+ * which is optionally used to describe the processor and cache topology.
+ * Due to the relative pointers used throughout the table, this doesn't
+ * leverage the existing subtable parsing in the kernel.
+ */
+#define pr_fmt(fmt) "ACPI PPTT: " fmt
+
+#include 
+#include 
+#include 
+
+/*
+ * Given the PPTT table, find and verify that the subtable entry
+ * is located within the table
+ */
+static struct acpi_subtable_header *fetch_pptt_subtable(
+   struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+   struct acpi_subtable_header *entry;
+
+   /* there isn't a subtable at reference 0 */
+   if (!pptt_ref)
+   return NULL;
+
+   if (pptt_ref + sizeof(struct acpi_subtable_header) > table_hdr->length)
+   return NULL;
+
+   entry = (struct acpi_subtable_header *)((u8 *)table_hdr + pptt_ref);


You can use ACPI_ADD_PTR() here.


+
+   if (pptt_ref + entry->length > table_hdr->length)
+   return NULL;
+
+   return entry;
+}
+
+static struct acpi_pptt_processor *fetch_pptt_node(
+   struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+   return (struct acpi_pptt_processor *)fetch_pptt_subtable(table_hdr, 
pptt_ref);
+}
+
+static struct acpi_pptt_cache *fetch_pptt_cache(
+   struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+   return (struct acpi_pptt_cache *)fetch_pptt_subtable(table_hdr, 
pptt_ref);
+}
+
+static struct acpi_subtable_header *acpi_get_pptt_resource(
+   struct acpi_table_header *table_hdr,
+   struct acpi_pptt_processor *node, int resource)
+{
+   u32 ref;
+
+   if (resource >= node->number_of_priv_resources)
+   return NULL;
+
+   ref = *(u32 *)((u8 *)node + sizeof(struct acpi_pptt_processor) +
+ sizeof(u32) * resource);


ACPI_ADD_PTR()


+
+   return fetch_pptt_subtable(table_hdr, ref);
+}
+
+/*
+ * given a pptt resource, verify that it is a cache node, then walk
+ * down each level of caches, counting how many levels are found
+ * as well as checking the cache type (icache, dcache, unified). If a
+ * level & type match, then we set found, and continue the search.
+ * Once the entire cache branch has been walked return its max
+ * depth.
+ */
+static int acpi_pptt_walk_cache(struct acpi_table_header *table_hdr,
+   int local_level,
+   struct acpi_subtable_header *res,
+   struct acpi_pptt_cache **found,
+   int level, int type)
+{
+   struct acpi_pptt_cache *cache;
+
+   if (res->type != ACPI_PPTT_TYPE_CACHE)
+   return 0;
+
+   cache = (struct acpi_pptt_cache *) res;
+   while (cache) {
+   local_level++;
+
+   if ((local_level == level) &&
+   (cache->flags & ACPI_PPTT_CACHE_TYPE_VALID) &&
+   ((cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) == type)) {
+   if (*found != NULL)
+   pr_err("Found duplicate cache level/type unable to 
determine uniqueness\n");
+
+   

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Julien Thierry]

Hi Jeremy,

Please see below some suggestions.

On 12/10/17 20:48, Jeremy Linton wrote:

ACPI 6.2 adds a new table, which describes how processing units
are related to each other in tree like fashion. Caches are
also sprinkled throughout the tree and describe the properties
of the caches in relation to other caches and processing units.

Add the code to parse the cache hierarchy and report the total
number of levels of cache for a given core using
acpi_find_last_cache_level() as well as fill out the individual
cores cache information with cache_setup_acpi() once the
cpu_cacheinfo structure has been populated by the arch specific
code.

Further, report peers in the topology using setup_acpi_cpu_topology()
to report a unique ID for each processing unit at a given level
in the tree. These unique id's can then be used to match related
processing units which exist as threads, COD (clusters
on die), within a given package, etc.

Signed-off-by: Jeremy Linton 
---
  drivers/acpi/pptt.c | 485 
  1 file changed, 485 insertions(+)
  create mode 100644 drivers/acpi/pptt.c

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
new file mode 100644
index ..c86715fed4a7
--- /dev/null
+++ b/drivers/acpi/pptt.c
@@ -0,1 +1,485 @@
+/*
+ * Copyright (C) 2017, ARM
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * This file implements parsing of Processor Properties Topology Table (PPTT)
+ * which is optionally used to describe the processor and cache topology.
+ * Due to the relative pointers used throughout the table, this doesn't
+ * leverage the existing subtable parsing in the kernel.
+ */
+#define pr_fmt(fmt) "ACPI PPTT: " fmt
+
+#include 
+#include 
+#include 
+
+/*
+ * Given the PPTT table, find and verify that the subtable entry
+ * is located within the table
+ */
+static struct acpi_subtable_header *fetch_pptt_subtable(
+   struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+   struct acpi_subtable_header *entry;
+
+   /* there isn't a subtable at reference 0 */
+   if (!pptt_ref)
+   return NULL;


Seeing the usage of pptt_ref to retrieve the subtable, would the 
following be a more accurate check?


if (pptt_ref < sizeof(struct acpi_table_header))
return NULL;


+
+   if (pptt_ref + sizeof(struct acpi_subtable_header) > table_hdr->length)
+   return NULL;
+
+   entry = (struct acpi_subtable_header *)((u8 *)table_hdr + pptt_ref);
+
+   if (pptt_ref + entry->length > table_hdr->length)
+   return NULL;
+
+   return entry;
+}
+
+static struct acpi_pptt_processor *fetch_pptt_node(
+   struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+   return (struct acpi_pptt_processor *)fetch_pptt_subtable(table_hdr, 
pptt_ref);
+}
+
+static struct acpi_pptt_cache *fetch_pptt_cache(
+   struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+   return (struct acpi_pptt_cache *)fetch_pptt_subtable(table_hdr, 
pptt_ref);
+}
+
+static struct acpi_subtable_header *acpi_get_pptt_resource(
+   struct acpi_table_header *table_hdr,
+   struct acpi_pptt_processor *node, int resource)
+{
+   u32 ref;
+
+   if (resource >= node->number_of_priv_resources)
+   return NULL;
+
+   ref = *(u32 *)((u8 *)node + sizeof(struct acpi_pptt_processor) +
+ sizeof(u32) * resource);
+


I think this can be simplified as:

ref = *((u32 *)(node + 1) + resource);


+   return fetch_pptt_subtable(table_hdr, ref);
+}
+
+/*
+ * given a pptt resource, verify that it is a cache node, then walk
+ * down each level of caches, counting how many levels are found
+ * as well as checking the cache type (icache, dcache, unified). If a
+ * level & type match, then we set found, and continue the search.
+ * Once the entire cache branch has been walked return its max
+ * depth.
+ */
+static int acpi_pptt_walk_cache(struct acpi_table_header *table_hdr,
+   int local_level,
+   struct acpi_subtable_header *res,
+   struct acpi_pptt_cache **found,
+   int level, int type)
+{
+   struct acpi_pptt_cache *cache;
+
+   if (res->type != ACPI_PPTT_TYPE_CACHE)
+   return 0;
+
+   cache = (struct acpi_pptt_cache *) res;
+   while (cache) {
+   local_level++;
+
+   if ((local_level == level) &&
+   (cache->flags & 

Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Julien Thierry]

Hi Jeremy,

Please see below some suggestions.

On 12/10/17 20:48, Jeremy Linton wrote:

ACPI 6.2 adds a new table, which describes how processing units
are related to each other in tree like fashion. Caches are
also sprinkled throughout the tree and describe the properties
of the caches in relation to other caches and processing units.

Add the code to parse the cache hierarchy and report the total
number of levels of cache for a given core using
acpi_find_last_cache_level() as well as fill out the individual
cores cache information with cache_setup_acpi() once the
cpu_cacheinfo structure has been populated by the arch specific
code.

Further, report peers in the topology using setup_acpi_cpu_topology()
to report a unique ID for each processing unit at a given level
in the tree. These unique id's can then be used to match related
processing units which exist as threads, COD (clusters
on die), within a given package, etc.

Signed-off-by: Jeremy Linton 
---
  drivers/acpi/pptt.c | 485 
  1 file changed, 485 insertions(+)
  create mode 100644 drivers/acpi/pptt.c

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
new file mode 100644
index ..c86715fed4a7
--- /dev/null
+++ b/drivers/acpi/pptt.c
@@ -0,1 +1,485 @@
+/*
+ * Copyright (C) 2017, ARM
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * This file implements parsing of Processor Properties Topology Table (PPTT)
+ * which is optionally used to describe the processor and cache topology.
+ * Due to the relative pointers used throughout the table, this doesn't
+ * leverage the existing subtable parsing in the kernel.
+ */
+#define pr_fmt(fmt) "ACPI PPTT: " fmt
+
+#include 
+#include 
+#include 
+
+/*
+ * Given the PPTT table, find and verify that the subtable entry
+ * is located within the table
+ */
+static struct acpi_subtable_header *fetch_pptt_subtable(
+   struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+   struct acpi_subtable_header *entry;
+
+   /* there isn't a subtable at reference 0 */
+   if (!pptt_ref)
+   return NULL;


Seeing the usage of pptt_ref to retrieve the subtable, would the 
following be a more accurate check?


if (pptt_ref < sizeof(struct acpi_table_header))
return NULL;


+
+   if (pptt_ref + sizeof(struct acpi_subtable_header) > table_hdr->length)
+   return NULL;
+
+   entry = (struct acpi_subtable_header *)((u8 *)table_hdr + pptt_ref);
+
+   if (pptt_ref + entry->length > table_hdr->length)
+   return NULL;
+
+   return entry;
+}
+
+static struct acpi_pptt_processor *fetch_pptt_node(
+   struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+   return (struct acpi_pptt_processor *)fetch_pptt_subtable(table_hdr, 
pptt_ref);
+}
+
+static struct acpi_pptt_cache *fetch_pptt_cache(
+   struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+   return (struct acpi_pptt_cache *)fetch_pptt_subtable(table_hdr, 
pptt_ref);
+}
+
+static struct acpi_subtable_header *acpi_get_pptt_resource(
+   struct acpi_table_header *table_hdr,
+   struct acpi_pptt_processor *node, int resource)
+{
+   u32 ref;
+
+   if (resource >= node->number_of_priv_resources)
+   return NULL;
+
+   ref = *(u32 *)((u8 *)node + sizeof(struct acpi_pptt_processor) +
+ sizeof(u32) * resource);
+


I think this can be simplified as:

ref = *((u32 *)(node + 1) + resource);


+   return fetch_pptt_subtable(table_hdr, ref);
+}
+
+/*
+ * given a pptt resource, verify that it is a cache node, then walk
+ * down each level of caches, counting how many levels are found
+ * as well as checking the cache type (icache, dcache, unified). If a
+ * level & type match, then we set found, and continue the search.
+ * Once the entire cache branch has been walked return its max
+ * depth.
+ */
+static int acpi_pptt_walk_cache(struct acpi_table_header *table_hdr,
+   int local_level,
+   struct acpi_subtable_header *res,
+   struct acpi_pptt_cache **found,
+   int level, int type)
+{
+   struct acpi_pptt_cache *cache;
+
+   if (res->type != ACPI_PPTT_TYPE_CACHE)
+   return 0;
+
+   cache = (struct acpi_pptt_cache *) res;
+   while (cache) {
+   local_level++;
+
+   if ((local_level == level) &&
+   (cache->flags & ACPI_PPTT_CACHE_TYPE_VALID) &&
+