[RFC PATCH 0/2] ACPI / PPTT: ids for caches

[RFC PATCH 0/2] ACPI / PPTT: ids for caches

[James Morse]

Hi guys,

To get resctrl working on arm64, we need to generate 'id's for caches.
This is this value that shows up in, e.g.:
| /sys/devices/system/cpu/cpu0/cache/index3/id

This value needs to be unique for each level of cache, but doesn't
need to be contiguous. (there may be gaps, it may not start at 0).
Details in Documentation/x86/intel_rdt_ui.txt::Cache IDs

resctrl receives these values back via its schemata file. e.g.:
| echo "L3:0=fff;1=fff" > /sys/fs/resctrl/p1/schemata
Where 0 and 1 are the ids of two caches in the system.

These values become ABI, and are likely to be baked into shell scripts.
We want a value that is the same over reboots, and should be the same
on identical hardware, even if the PPTT is generated in a different
order. The hardware doesn't give us any indication of which caches are
shared, so this information must come from firmware tables.

This series generates an id from the PPTT topology, based on the lowest
MPIDR of the cpus that share a cache.

The remaining problems with this approach are:
 * the 32bit ID field is full of MPIDR.Aff{0-3}. We don't have space to
   hide 'i/d/unified', so can only generate ids for unified caches. If we
   ever get an Aff4 (plenty of RES0 space in there) we can no longer generate
   an id. Having all these bits accounted for in the initial version doesn't
   feel like a good ABI choice.

* Existing software is going to assume caches are numbered 0,1,2. This was
  documented as not guaranteed, and its likely never going to be the case
  if we generate ids like this.

* The table walk is recursive.


Fixes for the first two require extra-code to compact the ID range, which would
require us generating all the IDs up front, not from hotplug callbacks as has
to happen today.

Alternatively, we could try and change the abi to provide a u64 as the
cache id. The size isn't documented, and for resctrl userspace can treat
it as a string.

Better ideas welcome!

Thanks,

James Morse (2):
  ACPI / processor: Add helper to convert acpi_id to a phys_cpuid
  ACPI / PPTT: cacheinfo: Label caches based on fw_token

 arch/arm64/include/asm/acpi.h |  6 +++
 drivers/acpi/pptt.c           | 81 +++++++++++++++++++++++++++++++++++
 drivers/acpi/processor_core.c | 16 +++++++
 include/acpi/processor.h      |  1 +
 4 files changed, 104 insertions(+)

-- 
2.18.0

[RFC PATCH 1/2] ACPI / processor: Add helper to convert acpi_id to a phys_cpuid

[James Morse]

The PPTT parsing code only has access to an acpi_id, we need a hardware
property, preferably the corresponding phys_cpuid_t.

acpi_get_cpuid() requires us to have the acpi_handle, which would imply
we already have the acpi_device or acpi_processor structure. This call
is useful when the CPU may not have been mapped, e.g. when walking the
namespace.

The PPTT is parsed after CPUs have been discovered and mapped, add a
helper to walk the possible CPUs and test whether the acpi_processor
matches our acpi_id.

Signed-off-by: James Morse <james.morse@arm.com>

---
I'm not entirely sure what 'mapping processors' is about, so may have the
wrong end of the stick here.
---
 drivers/acpi/processor_core.c | 16 ++++++++++++++++
 include/acpi/processor.h      |  1 +
 2 files changed, 17 insertions(+)

diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
index 8c0a54d50d0e..333547bf7845 100644
--- a/drivers/acpi/processor_core.c
+++ b/drivers/acpi/processor_core.c
@@ -9,7 +9,9 @@
  *	Yinghai Lu <yinghai@kernel.org>
  *	Jiang Liu <jiang.liu@intel.com>
  */
+#include <linux/percpu.h>
 #include <linux/export.h>
+#include <linux/cpumask.h>
 #include <linux/acpi.h>
 #include <acpi/processor.h>
 
@@ -263,6 +265,20 @@ int acpi_get_cpuid(acpi_handle handle, int type, u32 acpi_id)
 }
 EXPORT_SYMBOL_GPL(acpi_get_cpuid);
 
+phys_cpuid_t acpi_id_to_phys_cpuid(u32 acpi_id)
+{
+	int cpu;
+	struct acpi_processor *pr;
+
+	for_each_possible_cpu(cpu) {
+		pr = per_cpu(processors, cpu);
+		if (pr && pr->acpi_id == acpi_id)
+			return pr->phys_id;
+	}
+
+	return PHYS_CPUID_INVALID;
+}
+
 #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC
 static int get_ioapic_id(struct acpi_subtable_header *entry, u32 gsi_base,
 			 u64 *phys_addr, int *ioapic_id)
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 1194a4c78d55..9235b41a9d52 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -333,6 +333,7 @@ phys_cpuid_t acpi_get_phys_id(acpi_handle, int type, u32 acpi_id);
 phys_cpuid_t acpi_map_madt_entry(u32 acpi_id);
 int acpi_map_cpuid(phys_cpuid_t phys_id, u32 acpi_id);
 int acpi_get_cpuid(acpi_handle, int type, u32 acpi_id);
+phys_cpuid_t acpi_id_to_phys_cpuid(u32 acpi_id);
 
 #ifdef CONFIG_ACPI_CPPC_LIB
 extern int acpi_cppc_processor_probe(struct acpi_processor *pr);
-- 
2.18.0

[RFC PATCH 2/2] ACPI / PPTT: cacheinfo: Label caches based on fw_token

[James Morse]

The resctrl ABI requires caches to have a unique id. This number must
be unique across all caches at this level, but doesn't need to be
contiguous. (there may be gaps, it may not start at 0).
See Documentation/x86/intel_rdt_ui.txt::Cache IDs

We want a value that is the same over reboots, and should be the same
on identical hardware, even if the PPTT is generated in a different
order. The hardware doesn't give us any indication of which caches are
shared, so this information must come from firmware tables.

Starting with a cacheinfo's fw_token, we walk the table to find all
CPUs that share this cpu_node (and thus cache), and take the lowest
physical id to use as the id for the cache. On arm64 this value
corresponds to the MPIDR.

This is only done for unified caches, as instruction/data caches would
generate the same id using this scheme.

Signed-off-by: James Morse <james.morse@arm.com>
---
 arch/arm64/include/asm/acpi.h |  6 +++
 drivers/acpi/pptt.c           | 81 +++++++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index 709208dfdc8b..16b9b3d771a8 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -53,6 +53,12 @@ static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
 typedef u64 phys_cpuid_t;
 #define PHYS_CPUID_INVALID INVALID_HWID
 
+/* Shift the relevant bits out of u64 phys_cpuid_t into a u32 */
+#define ARCH_PHYSID_TO_U32(x) (u32)(MPIDR_AFFINITY_LEVEL(x, 0)		|\
+			MPIDR_AFFINITY_LEVEL(x, 1) << MPIDR_LEVEL_BITS  |\
+			MPIDR_AFFINITY_LEVEL(x, 2) << 2*MPIDR_LEVEL_BITS|\
+			MPIDR_AFFINITY_LEVEL(x, 3) << 3*MPIDR_LEVEL_BITS)
+
 #define acpi_strict 1	/* No out-of-spec workarounds on ARM64 */
 extern int acpi_disabled;
 extern int acpi_noirq;
diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
index d1e26cb599bf..9478f8c28158 100644
--- a/drivers/acpi/pptt.c
+++ b/drivers/acpi/pptt.c
@@ -341,6 +341,84 @@ static struct acpi_pptt_cache *acpi_find_cache_node(struct acpi_table_header *ta
 /* total number of attributes checked by the properties code */
 #define PPTT_CHECKED_ATTRIBUTES 4
 
+/**
+ * acpi_pptt_min_physid_from_cpu_node() - Recursivly find @min_physid for all
+ * leaf CPUs below @cpu_node.
+ * @table_hdr:	Pointer to the head of the PPTT table
+ * @cpu_node:	The point in the toplogy to start the walk
+ * @min_physid:	The min_physid to update with leaf CPUs.
+ */
+void acpi_pptt_min_physid_from_cpu_node(struct acpi_table_header *table_hdr,
+					struct acpi_pptt_processor *cpu_node,
+					phys_cpuid_t *min_physid)
+{
+	bool leaf = true;
+	u32 acpi_processor_id;
+	phys_cpuid_t cpu_node_phys_id;
+	struct acpi_subtable_header *iter;
+	struct acpi_pptt_processor *iter_node;
+	u32 target_node = ACPI_PTR_DIFF(cpu_node, table_hdr);
+	u32 proc_sz = sizeof(struct acpi_pptt_processor *);
+	unsigned long table_end = (unsigned long)table_hdr + table_hdr->length;
+
+	/*
+	 * Walk the PPTT, looking for nodes that reference cpu_node
+	 * as parent.
+	 */
+	iter = ACPI_ADD_PTR(struct acpi_subtable_header, table_hdr,
+			     sizeof(struct acpi_table_pptt));
+
+	while ((unsigned long)iter + proc_sz < table_end) {
+		iter_node = (struct acpi_pptt_processor *)iter;
+
+		if (iter->type == ACPI_PPTT_TYPE_PROCESSOR &&
+		    iter_node->parent == target_node) {
+			leaf = false;
+			acpi_pptt_min_physid_from_cpu_node(table_hdr, iter_node,
+							   min_physid);
+		}
+
+		if (iter->length == 0)
+			return;
+		iter = ACPI_ADD_PTR(struct acpi_subtable_header, iter,
+				    iter->length);
+	}
+
+	if (leaf && cpu_node->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID) {
+		acpi_processor_id = cpu_node->acpi_processor_id;
+		cpu_node_phys_id = acpi_id_to_phys_cpuid(acpi_processor_id);
+		*min_physid = min(*min_physid, cpu_node_phys_id);
+	}
+}
+
+static void acpi_pptt_label_cache(struct cacheinfo *this_leaf)
+{
+	acpi_status status;
+	struct acpi_table_header *table;
+	struct acpi_pptt_processor *cpu_node;
+	phys_cpuid_t min_physid = PHYS_CPUID_INVALID;
+
+	/* Affinity based IDs for non-unified caches would not be unique */
+	if (this_leaf->type != CACHE_TYPE_UNIFIED)
+		return;
+
+	if (!this_leaf->fw_token)
+		return;
+	cpu_node = this_leaf->fw_token;
+
+	status = acpi_get_table(ACPI_SIG_PPTT, 0, &table);
+	if (ACPI_FAILURE(status))
+		return;
+
+	acpi_pptt_min_physid_from_cpu_node(table, cpu_node, &min_physid);
+	acpi_put_table(table);
+
+	WARN_ON_ONCE(min_physid == PHYS_CPUID_INVALID);
+
+	this_leaf->id = ARCH_PHYSID_TO_U32(min_physid);
+	this_leaf->attributes |= CACHE_ID;
+}
+
 /**
  * update_cache_properties() - Update cacheinfo for the given processor
  * @this_leaf: Kernel cache info structure being updated
@@ -408,6 +486,9 @@ static void update_cache_properties(struct cacheinfo *this_leaf,
 	if (this_leaf->type == CACHE_TYPE_NOCACHE &&
 	    valid_flags == PPTT_CHECKED_ATTRIBUTES)
 		this_leaf->type = CACHE_TYPE_UNIFIED;
+
+	/* Now that the type is known, try and generate an id. */
+	acpi_pptt_label_cache(this_leaf);
 }
 
 static void cache_setup_acpi_cpu(struct acpi_table_header *table,
-- 
2.18.0

Re: [RFC PATCH 2/2] ACPI / PPTT: cacheinfo: Label caches based on fw_token

[Jeremy Linton]

Hi,

On 10/05/2018 10:02 AM, James Morse wrote:
> The resctrl ABI requires caches to have a unique id. This number must
> be unique across all caches at this level, but doesn't need to be
> contiguous. (there may be gaps, it may not start at 0).
> See Documentation/x86/intel_rdt_ui.txt::Cache IDs
> 
> We want a value that is the same over reboots, and should be the same
> on identical hardware, even if the PPTT is generated in a different
> order. The hardware doesn't give us any indication of which caches are
> shared, so this information must come from firmware tables.
> 
> Starting with a cacheinfo's fw_token, we walk the table to find all
> CPUs that share this cpu_node (and thus cache), and take the lowest
> physical id to use as the id for the cache. On arm64 this value
> corresponds to the MPIDR.
> 
> This is only done for unified caches, as instruction/data caches would
> generate the same id using this scheme.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>   arch/arm64/include/asm/acpi.h |  6 +++
>   drivers/acpi/pptt.c           | 81 +++++++++++++++++++++++++++++++++++
>   2 files changed, 87 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index 709208dfdc8b..16b9b3d771a8 100644
> --- a/arch/arm64/include/asm/acpi.h
> +++ b/arch/arm64/include/asm/acpi.h
> @@ -53,6 +53,12 @@ static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
>   typedef u64 phys_cpuid_t;
>   #define PHYS_CPUID_INVALID INVALID_HWID
>   
> +/* Shift the relevant bits out of u64 phys_cpuid_t into a u32 */
> +#define ARCH_PHYSID_TO_U32(x) (u32)(MPIDR_AFFINITY_LEVEL(x, 0)		|\
> +			MPIDR_AFFINITY_LEVEL(x, 1) << MPIDR_LEVEL_BITS  |\
> +			MPIDR_AFFINITY_LEVEL(x, 2) << 2*MPIDR_LEVEL_BITS|\
> +			MPIDR_AFFINITY_LEVEL(x, 3) << 3*MPIDR_LEVEL_BITS)
> +
>   #define acpi_strict 1	/* No out-of-spec workarounds on ARM64 */
>   extern int acpi_disabled;
>   extern int acpi_noirq;
> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
> index d1e26cb599bf..9478f8c28158 100644
> --- a/drivers/acpi/pptt.c
> +++ b/drivers/acpi/pptt.c
> @@ -341,6 +341,84 @@ static struct acpi_pptt_cache *acpi_find_cache_node(struct acpi_table_header *ta
>   /* total number of attributes checked by the properties code */
>   #define PPTT_CHECKED_ATTRIBUTES 4
>   
> +/**
> + * acpi_pptt_min_physid_from_cpu_node() - Recursivly find @min_physid for all
> + * leaf CPUs below @cpu_node.
> + * @table_hdr:	Pointer to the head of the PPTT table
> + * @cpu_node:	The point in the toplogy to start the walk
> + * @min_physid:	The min_physid to update with leaf CPUs.
> + */
> +void acpi_pptt_min_physid_from_cpu_node(struct acpi_table_header *table_hdr,
> +					struct acpi_pptt_processor *cpu_node,
> +					phys_cpuid_t *min_physid)
> +{
> +	bool leaf = true;
> +	u32 acpi_processor_id;
> +	phys_cpuid_t cpu_node_phys_id;
> +	struct acpi_subtable_header *iter;
> +	struct acpi_pptt_processor *iter_node;
> +	u32 target_node = ACPI_PTR_DIFF(cpu_node, table_hdr);
> +	u32 proc_sz = sizeof(struct acpi_pptt_processor *);
> +	unsigned long table_end = (unsigned long)table_hdr + table_hdr->length;
> +
> +	/*
> +	 * Walk the PPTT, looking for nodes that reference cpu_node
> +	 * as parent.
> +	 */
> +	iter = ACPI_ADD_PTR(struct acpi_subtable_header, table_hdr,
> +			     sizeof(struct acpi_table_pptt));
> +
> +	while ((unsigned long)iter + proc_sz < table_end) {
> +		iter_node = (struct acpi_pptt_processor *)iter;
> +
> +		if (iter->type == ACPI_PPTT_TYPE_PROCESSOR &&
> +		    iter_node->parent == target_node) {
> +			leaf = false;
> +			acpi_pptt_min_physid_from_cpu_node(table_hdr, iter_node,
> +							   min_physid);
> +		}
> +
> +		if (iter->length == 0)
> +			return;
> +		iter = ACPI_ADD_PTR(struct acpi_subtable_header, iter,
> +				    iter->length);
> +	}
> +
> +	if (leaf && cpu_node->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID) {
> +		acpi_processor_id = cpu_node->acpi_processor_id;
> +		cpu_node_phys_id = acpi_id_to_phys_cpuid(acpi_processor_id);
> +		*min_physid = min(*min_physid, cpu_node_phys_id);
> +	}
> +}

Tho me, is seems a reliable way to acquire a stable id.

My only hangup here is with the recursion (which was avoided elsewhere 
in this code despite considerable simplification in a couple places). In 
a reasonable table the tree depth should be quite limited (and not 
contain any branch loops) but it seems a needless risk. How much worse 
is the non-recursive version?

Also, the first version of the PPTT spec can be read that 
ACPI_PPTT_ACPI_PROCESSOR_ID_VALID should _not_ be set on leaf nodes. So 
IMHO a better check is just whether the leaf's processor_id is valid in 
the MADT. Hopefully this flag becomes more reliable in time...

> +
> +static void acpi_pptt_label_cache(struct cacheinfo *this_leaf)
> +{
> +	acpi_status status;
> +	struct acpi_table_header *table;
> +	struct acpi_pptt_processor *cpu_node;
> +	phys_cpuid_t min_physid = PHYS_CPUID_INVALID;
> +
> +	/* Affinity based IDs for non-unified caches would not be unique */
> +	if (this_leaf->type != CACHE_TYPE_UNIFIED)
> +		return;
> +
> +	if (!this_leaf->fw_token)
> +		return;
> +	cpu_node = this_leaf->fw_token;
> +
> +	status = acpi_get_table(ACPI_SIG_PPTT, 0, &table);
> +	if (ACPI_FAILURE(status))
> +		return;
> +
> +	acpi_pptt_min_physid_from_cpu_node(table, cpu_node, &min_physid);
> +	acpi_put_table(table);
> +
> +	WARN_ON_ONCE(min_physid == PHYS_CPUID_INVALID);
> +
> +	this_leaf->id = ARCH_PHYSID_TO_U32(min_physid);
> +	this_leaf->attributes |= CACHE_ID;
> +}

To me its seems a little odd to be acpi_get_table()ing inside the PPTT 
parse routines because we lost the reference via the call to 
update_cache_properties(). Rather if this routine were called from 
cache_setup_acpi_cpu() the table could be passed in.

> +
>   /**
>    * update_cache_properties() - Update cacheinfo for the given processor
>    * @this_leaf: Kernel cache info structure being updated
> @@ -408,6 +486,9 @@ static void update_cache_properties(struct cacheinfo *this_leaf,
>   	if (this_leaf->type == CACHE_TYPE_NOCACHE &&
>   	    valid_flags == PPTT_CHECKED_ATTRIBUTES)
>   		this_leaf->type = CACHE_TYPE_UNIFIED;
> +
> +	/* Now that the type is known, try and generate an id. */
> +	acpi_pptt_label_cache(this_leaf);
>   }
>   
>   static void cache_setup_acpi_cpu(struct acpi_table_header *table,
> 

Re: [RFC PATCH 2/2] ACPI / PPTT: cacheinfo: Label caches based on fw_token

[James Morse]

Hi Jeremy,

On 09/10/2018 17:45, Jeremy Linton wrote:
> On 10/05/2018 10:02 AM, James Morse wrote:
>> The resctrl ABI requires caches to have a unique id. This number must
>> be unique across all caches at this level, but doesn't need to be
>> contiguous. (there may be gaps, it may not start at 0).
>> See Documentation/x86/intel_rdt_ui.txt::Cache IDs
>>
>> We want a value that is the same over reboots, and should be the same
>> on identical hardware, even if the PPTT is generated in a different
>> order. The hardware doesn't give us any indication of which caches are
>> shared, so this information must come from firmware tables.
>>
>> Starting with a cacheinfo's fw_token, we walk the table to find all
>> CPUs that share this cpu_node (and thus cache), and take the lowest
>> physical id to use as the id for the cache. On arm64 this value
>> corresponds to the MPIDR.
>>
>> This is only done for unified caches, as instruction/data caches would
>> generate the same id using this scheme.

>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
>> index d1e26cb599bf..9478f8c28158 100644
>> --- a/drivers/acpi/pptt.c
>> +++ b/drivers/acpi/pptt.c
>> @@ -341,6 +341,84 @@ static struct acpi_pptt_cache
>> *acpi_find_cache_node(struct acpi_table_header *ta
>>   /* total number of attributes checked by the properties code */
>>   #define PPTT_CHECKED_ATTRIBUTES 4
>>   +/**
>> + * acpi_pptt_min_physid_from_cpu_node() - Recursivly find @min_physid for all
>> + * leaf CPUs below @cpu_node.
>> + * @table_hdr:    Pointer to the head of the PPTT table
>> + * @cpu_node:    The point in the toplogy to start the walk
>> + * @min_physid:    The min_physid to update with leaf CPUs.
>> + */
>> +void acpi_pptt_min_physid_from_cpu_node(struct acpi_table_header *table_hdr,
>> +                    struct acpi_pptt_processor *cpu_node,
>> +                    phys_cpuid_t *min_physid)
>> +{
>> +    bool leaf = true;
>> +    u32 acpi_processor_id;
>> +    phys_cpuid_t cpu_node_phys_id;
>> +    struct acpi_subtable_header *iter;
>> +    struct acpi_pptt_processor *iter_node;
>> +    u32 target_node = ACPI_PTR_DIFF(cpu_node, table_hdr);
>> +    u32 proc_sz = sizeof(struct acpi_pptt_processor *);
>> +    unsigned long table_end = (unsigned long)table_hdr + table_hdr->length;
>> +
>> +    /*
>> +     * Walk the PPTT, looking for nodes that reference cpu_node
>> +     * as parent.
>> +     */
>> +    iter = ACPI_ADD_PTR(struct acpi_subtable_header, table_hdr,
>> +                 sizeof(struct acpi_table_pptt));
>> +
>> +    while ((unsigned long)iter + proc_sz < table_end) {
>> +        iter_node = (struct acpi_pptt_processor *)iter;
>> +
>> +        if (iter->type == ACPI_PPTT_TYPE_PROCESSOR &&
>> +            iter_node->parent == target_node) {
>> +            leaf = false;
>> +            acpi_pptt_min_physid_from_cpu_node(table_hdr, iter_node,
>> +                               min_physid);
>> +        }
>> +
>> +        if (iter->length == 0)
>> +            return;
>> +        iter = ACPI_ADD_PTR(struct acpi_subtable_header, iter,
>> +                    iter->length);
>> +    }
>> +
>> +    if (leaf && cpu_node->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID) {
>> +        acpi_processor_id = cpu_node->acpi_processor_id;
>> +        cpu_node_phys_id = acpi_id_to_phys_cpuid(acpi_processor_id);
>> +        *min_physid = min(*min_physid, cpu_node_phys_id);
>> +    }
>> +}
> 
> Tho me, is seems a reliable way to acquire a stable id.
> 
> My only hangup here is with the recursion (which was avoided elsewhere in this
> code despite considerable simplification in a couple places). In a reasonable
> table the tree depth should be quite limited (and not contain any branch loops)
> but it seems a needless risk. How much worse is the non-recursive version?

I haven't tried, this was just to get the discussion about the cache ids going.

The neatest way I can think of would be to find each cpu, then walk up the
parent pointers to see if this node is on the path. This avoids allocating
memory to hold the stuff we haven't done yet when walking down/around.


> Also, the first version of the PPTT spec can be read that
> ACPI_PPTT_ACPI_PROCESSOR_ID_VALID should _not_ be set on leaf nodes. So IMHO a
> better check is just whether the leaf's processor_id is valid in the MADT.
> Hopefully this flag becomes more reliable in time...

It can be set for a non-leaf entry, I assumed it would always be set for a leaf.
Is anyone doing this with a PPTT table?


>> +static void acpi_pptt_label_cache(struct cacheinfo *this_leaf)
>> +{
>> +    acpi_status status;
>> +    struct acpi_table_header *table;
>> +    struct acpi_pptt_processor *cpu_node;
>> +    phys_cpuid_t min_physid = PHYS_CPUID_INVALID;
>> +
>> +    /* Affinity based IDs for non-unified caches would not be unique */
>> +    if (this_leaf->type != CACHE_TYPE_UNIFIED)
>> +        return;
>> +
>> +    if (!this_leaf->fw_token)
>> +        return;
>> +    cpu_node = this_leaf->fw_token;
>> +
>> +    status = acpi_get_table(ACPI_SIG_PPTT, 0, &table);
>> +    if (ACPI_FAILURE(status))
>> +        return;
>> +
>> +    acpi_pptt_min_physid_from_cpu_node(table, cpu_node, &min_physid);
>> +    acpi_put_table(table);
>> +
>> +    WARN_ON_ONCE(min_physid == PHYS_CPUID_INVALID);
>> +
>> +    this_leaf->id = ARCH_PHYSID_TO_U32(min_physid);
>> +    this_leaf->attributes |= CACHE_ID;
>> +}
> 
> To me its seems a little odd to be acpi_get_table()ing inside the PPTT parse
> routines because we lost the reference via the call to
> update_cache_properties(). Rather if this routine were called from
> cache_setup_acpi_cpu() the table could be passed in.

Makes sense. This was just the last point the type could be set.


Thanks,

James

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 2/2] ACPI / PPTT: cacheinfo: Label caches based on fw_token

[Jeffrey Hugo]

On 10/9/2018 11:58 AM, James Morse wrote:
> Hi Jeremy,
> 
> On 09/10/2018 17:45, Jeremy Linton wrote:
>> On 10/05/2018 10:02 AM, James Morse wrote:
>>> The resctrl ABI requires caches to have a unique id. This number must
>>> be unique across all caches at this level, but doesn't need to be
>>> contiguous. (there may be gaps, it may not start at 0).
>>> See Documentation/x86/intel_rdt_ui.txt::Cache IDs
>>>
>>> We want a value that is the same over reboots, and should be the same
>>> on identical hardware, even if the PPTT is generated in a different
>>> order. The hardware doesn't give us any indication of which caches are
>>> shared, so this information must come from firmware tables.
>>>
>>> Starting with a cacheinfo's fw_token, we walk the table to find all
>>> CPUs that share this cpu_node (and thus cache), and take the lowest
>>> physical id to use as the id for the cache. On arm64 this value
>>> corresponds to the MPIDR.
>>>
>>> This is only done for unified caches, as instruction/data caches would
>>> generate the same id using this scheme.
> 
>>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
>>> index d1e26cb599bf..9478f8c28158 100644
>>> --- a/drivers/acpi/pptt.c
>>> +++ b/drivers/acpi/pptt.c
>>> @@ -341,6 +341,84 @@ static struct acpi_pptt_cache
>>> *acpi_find_cache_node(struct acpi_table_header *ta
>>>    /* total number of attributes checked by the properties code */
>>>    #define PPTT_CHECKED_ATTRIBUTES 4
>>>    +/**
>>> + * acpi_pptt_min_physid_from_cpu_node() - Recursivly find @min_physid for all
>>> + * leaf CPUs below @cpu_node.
>>> + * @table_hdr:    Pointer to the head of the PPTT table
>>> + * @cpu_node:    The point in the toplogy to start the walk
>>> + * @min_physid:    The min_physid to update with leaf CPUs.
>>> + */
>>> +void acpi_pptt_min_physid_from_cpu_node(struct acpi_table_header *table_hdr,
>>> +                    struct acpi_pptt_processor *cpu_node,
>>> +                    phys_cpuid_t *min_physid)
>>> +{
>>> +    bool leaf = true;
>>> +    u32 acpi_processor_id;
>>> +    phys_cpuid_t cpu_node_phys_id;
>>> +    struct acpi_subtable_header *iter;
>>> +    struct acpi_pptt_processor *iter_node;
>>> +    u32 target_node = ACPI_PTR_DIFF(cpu_node, table_hdr);
>>> +    u32 proc_sz = sizeof(struct acpi_pptt_processor *);
>>> +    unsigned long table_end = (unsigned long)table_hdr + table_hdr->length;
>>> +
>>> +    /*
>>> +     * Walk the PPTT, looking for nodes that reference cpu_node
>>> +     * as parent.
>>> +     */
>>> +    iter = ACPI_ADD_PTR(struct acpi_subtable_header, table_hdr,
>>> +                 sizeof(struct acpi_table_pptt));
>>> +
>>> +    while ((unsigned long)iter + proc_sz < table_end) {
>>> +        iter_node = (struct acpi_pptt_processor *)iter;
>>> +
>>> +        if (iter->type == ACPI_PPTT_TYPE_PROCESSOR &&
>>> +            iter_node->parent == target_node) {
>>> +            leaf = false;
>>> +            acpi_pptt_min_physid_from_cpu_node(table_hdr, iter_node,
>>> +                               min_physid);
>>> +        }
>>> +
>>> +        if (iter->length == 0)
>>> +            return;
>>> +        iter = ACPI_ADD_PTR(struct acpi_subtable_header, iter,
>>> +                    iter->length);
>>> +    }
>>> +
>>> +    if (leaf && cpu_node->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID) {
>>> +        acpi_processor_id = cpu_node->acpi_processor_id;
>>> +        cpu_node_phys_id = acpi_id_to_phys_cpuid(acpi_processor_id);
>>> +        *min_physid = min(*min_physid, cpu_node_phys_id);
>>> +    }
>>> +}
>>
>> Tho me, is seems a reliable way to acquire a stable id.
>>
>> My only hangup here is with the recursion (which was avoided elsewhere in this
>> code despite considerable simplification in a couple places). In a reasonable
>> table the tree depth should be quite limited (and not contain any branch loops)
>> but it seems a needless risk. How much worse is the non-recursive version?
> 
> I haven't tried, this was just to get the discussion about the cache ids going.
> 
> The neatest way I can think of would be to find each cpu, then walk up the
> parent pointers to see if this node is on the path. This avoids allocating
> memory to hold the stuff we haven't done yet when walking down/around.
> 
> 
>> Also, the first version of the PPTT spec can be read that
>> ACPI_PPTT_ACPI_PROCESSOR_ID_VALID should _not_ be set on leaf nodes. So IMHO a
>> better check is just whether the leaf's processor_id is valid in the MADT.
>> Hopefully this flag becomes more reliable in time...
> 
> It can be set for a non-leaf entry, I assumed it would always be set for a leaf.
> Is anyone doing this with a PPTT table?

QDF2400 takes a strict interpretation of the spec, and does not set the 
flag for leaf nodes.  I believe there are other implementations which do 
set the flag for leaf nodes.

> 
> 
>>> +static void acpi_pptt_label_cache(struct cacheinfo *this_leaf)
>>> +{
>>> +    acpi_status status;
>>> +    struct acpi_table_header *table;
>>> +    struct acpi_pptt_processor *cpu_node;
>>> +    phys_cpuid_t min_physid = PHYS_CPUID_INVALID;
>>> +
>>> +    /* Affinity based IDs for non-unified caches would not be unique */
>>> +    if (this_leaf->type != CACHE_TYPE_UNIFIED)
>>> +        return;
>>> +
>>> +    if (!this_leaf->fw_token)
>>> +        return;
>>> +    cpu_node = this_leaf->fw_token;
>>> +
>>> +    status = acpi_get_table(ACPI_SIG_PPTT, 0, &table);
>>> +    if (ACPI_FAILURE(status))
>>> +        return;
>>> +
>>> +    acpi_pptt_min_physid_from_cpu_node(table, cpu_node, &min_physid);
>>> +    acpi_put_table(table);
>>> +
>>> +    WARN_ON_ONCE(min_physid == PHYS_CPUID_INVALID);
>>> +
>>> +    this_leaf->id = ARCH_PHYSID_TO_U32(min_physid);
>>> +    this_leaf->attributes |= CACHE_ID;
>>> +}
>>
>> To me its seems a little odd to be acpi_get_table()ing inside the PPTT parse
>> routines because we lost the reference via the call to
>> update_cache_properties(). Rather if this routine were called from
>> cache_setup_acpi_cpu() the table could be passed in.
> 
> Makes sense. This was just the last point the type could be set.
> 
> 
> Thanks,
> 
> James
> 


-- 
Jeffrey Hugo
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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 2/2] ACPI / PPTT: cacheinfo: Label caches based on fw_token

[Sudeep Holla]

On Tue, Oct 09, 2018 at 12:34:51PM -0600, Jeffrey Hugo wrote:
> On 10/9/2018 11:58 AM, James Morse wrote:
> >
> > It can be set for a non-leaf entry, I assumed it would always be set for a leaf.
> > Is anyone doing this with a PPTT table?
>
> QDF2400 takes a strict interpretation of the spec, and does not set the flag
> for leaf nodes.  I believe there are other implementations which do set the
> flag for leaf nodes.
>

IIRC, based on the discussions when this was added, the ACPI Processor
ID  *must be* valid for the lead nodes.T he flag bit corresponding
to that should be considered as don't care as it's always guaranteed
to be valid.

--
Regards,
Sudeep

Re: [RFC PATCH 2/2] ACPI / PPTT: cacheinfo: Label caches based on fw_token

[Jeffrey Hugo]

On 10/10/2018 3:46 AM, Sudeep Holla wrote:
> On Tue, Oct 09, 2018 at 12:34:51PM -0600, Jeffrey Hugo wrote:
>> On 10/9/2018 11:58 AM, James Morse wrote:
>>>
>>> It can be set for a non-leaf entry, I assumed it would always be set for a leaf.
>>> Is anyone doing this with a PPTT table?
>>
>> QDF2400 takes a strict interpretation of the spec, and does not set the flag
>> for leaf nodes.  I believe there are other implementations which do set the
>> flag for leaf nodes.
>>
> 
> IIRC, based on the discussions when this was added, the ACPI Processor
> ID  *must be* valid for the lead nodes.T he flag bit corresponding
> to that should be considered as don't care as it's always guaranteed
> to be valid.

Correct.

-- 
Jeffrey Hugo
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: [RFC PATCH 2/2] ACPI / PPTT: cacheinfo: Label caches based on fw_token

[Shameerali Kolothum Thodi]

Hi James,

> -----Original Message-----
> From: linux-arm-kernel [mailto:linux-arm-kernel-bounces@lists.infradead.org]
> On Behalf Of James Morse
> Sent: 05 October 2018 16:03
> To: linux-acpi@vger.kernel.org
> Cc: Vijaya Kumar K <vkilari@codeaurora.org>; Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com>; Jeffrey Hugo <jhugo@codeaurora.org>; Sudeep
> Holla <sudeep.holla@arm.com>; Jeremy Linton <jeremy.linton@arm.com>;
> Tomasz Nowicki <Tomasz.Nowicki@cavium.com>; James Morse
> <james.morse@arm.com>; Richard Ruigrok <rruigrok@qti.qualcomm.com>;
> Guohanjun (Hanjun Guo) <guohanjun@huawei.com>; wangxiongfeng (C)
> <wangxiongfeng2@huawei.com>; linux-arm-kernel@lists.infradead.org
> Subject: [RFC PATCH 2/2] ACPI / PPTT: cacheinfo: Label caches based on
> fw_token
> 
> The resctrl ABI requires caches to have a unique id. This number must be
> unique across all caches at this level, but doesn't need to be contiguous. (there
> may be gaps, it may not start at 0).
> See Documentation/x86/intel_rdt_ui.txt::Cache IDs
> 
> We want a value that is the same over reboots, and should be the same on
> identical hardware, even if the PPTT is generated in a different order. The
> hardware doesn't give us any indication of which caches are shared, so this
> information must come from firmware tables.
> 
> Starting with a cacheinfo's fw_token, we walk the table to find all CPUs that
> share this cpu_node (and thus cache), and take the lowest physical id to use as
> the id for the cache. On arm64 this value corresponds to the MPIDR.
> 
> This is only done for unified caches, as instruction/data caches would generate
> the same id using this scheme.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>  arch/arm64/include/asm/acpi.h |  6 +++
>  drivers/acpi/pptt.c           | 81
> +++++++++++++++++++++++++++++++++++
>  2 files changed, 87 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index 709208dfdc8b..16b9b3d771a8 100644
> --- a/arch/arm64/include/asm/acpi.h
> +++ b/arch/arm64/include/asm/acpi.h
> @@ -53,6 +53,12 @@ static inline void __iomem
> *acpi_os_ioremap(acpi_physical_address phys,  typedef u64 phys_cpuid_t;
> #define PHYS_CPUID_INVALID INVALID_HWID
> 
> +/* Shift the relevant bits out of u64 phys_cpuid_t into a u32 */
> +#define ARCH_PHYSID_TO_U32(x) (u32)(MPIDR_AFFINITY_LEVEL(x, 0)
> 	|\
> +			MPIDR_AFFINITY_LEVEL(x, 1) << MPIDR_LEVEL_BITS  |\
> +			MPIDR_AFFINITY_LEVEL(x, 2) << 2*MPIDR_LEVEL_BITS|\
> +			MPIDR_AFFINITY_LEVEL(x, 3) << 3*MPIDR_LEVEL_BITS)
> +
>  #define acpi_strict 1	/* No out-of-spec workarounds on ARM64 */
>  extern int acpi_disabled;
>  extern int acpi_noirq;
> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c index
> d1e26cb599bf..9478f8c28158 100644
> --- a/drivers/acpi/pptt.c
> +++ b/drivers/acpi/pptt.c
> @@ -341,6 +341,84 @@ static struct acpi_pptt_cache
> *acpi_find_cache_node(struct acpi_table_header *ta
>  /* total number of attributes checked by the properties code */  #define
> PPTT_CHECKED_ATTRIBUTES 4
> 
> +/**
> + * acpi_pptt_min_physid_from_cpu_node() - Recursivly find @min_physid
> +for all
> + * leaf CPUs below @cpu_node.
> + * @table_hdr:	Pointer to the head of the PPTT table
> + * @cpu_node:	The point in the toplogy to start the walk
> + * @min_physid:	The min_physid to update with leaf CPUs.
> + */
> +void acpi_pptt_min_physid_from_cpu_node(struct acpi_table_header
> *table_hdr,
> +					struct acpi_pptt_processor *cpu_node,
> +					phys_cpuid_t *min_physid)
> +{
> +	bool leaf = true;
> +	u32 acpi_processor_id;
> +	phys_cpuid_t cpu_node_phys_id;
> +	struct acpi_subtable_header *iter;
> +	struct acpi_pptt_processor *iter_node;
> +	u32 target_node = ACPI_PTR_DIFF(cpu_node, table_hdr);
> +	u32 proc_sz = sizeof(struct acpi_pptt_processor *);
> +	unsigned long table_end = (unsigned long)table_hdr +
> +table_hdr->length;
> +
> +	/*
> +	 * Walk the PPTT, looking for nodes that reference cpu_node
> +	 * as parent.
> +	 */
> +	iter = ACPI_ADD_PTR(struct acpi_subtable_header, table_hdr,
> +			     sizeof(struct acpi_table_pptt));
> +
> +	while ((unsigned long)iter + proc_sz < table_end) {
> +		iter_node = (struct acpi_pptt_processor *)iter;
> +
> +		if (iter->type == ACPI_PPTT_TYPE_PROCESSOR &&
> +		    iter_node->parent == target_node) {
> +			leaf = false;
> +			acpi_pptt_min_physid_from_cpu_node(table_hdr, iter_node,
> +							   min_physid);
> +		}
> +
> +		if (iter->length == 0)
> +			return;
> +		iter = ACPI_ADD_PTR(struct acpi_subtable_header, iter,
> +				    iter->length);
> +	}
> +
> +	if (leaf && cpu_node->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID) {
> +		acpi_processor_id = cpu_node->acpi_processor_id;
> +		cpu_node_phys_id = acpi_id_to_phys_cpuid(acpi_processor_id);
> +		*min_physid = min(*min_physid, cpu_node_phys_id);
> +	}
> +}

I was just trying out the latest public MPAM branch available here[1] and noted that
on our HiSilicon platform all the L3 cache were labeled with the same Id. Debugging
revealed that the above leaf node check was removed in this branch[2] which makes
the min_physid calculation going wrong. Just wondering is there any particular reason
for removing the check or the branch is not carrying the latest patch?

Please let me know.

Thanks,
Shameer

1. http://www.linux-arm.org/git?p=linux-jm.git;a=shortlog;h=refs/heads/mpam/snapshot/may
2. http://www.linux-arm.org/git?p=linux-jm.git;a=commitdiff;h=413eb4281b072e1ee60f88b814f2a418358f2155;hp=44f0d62a6e00f8da7f04ab9e0673340e6c67ae65


> +static void acpi_pptt_label_cache(struct cacheinfo *this_leaf) {
> +	acpi_status status;
> +	struct acpi_table_header *table;
> +	struct acpi_pptt_processor *cpu_node;
> +	phys_cpuid_t min_physid = PHYS_CPUID_INVALID;
> +
> +	/* Affinity based IDs for non-unified caches would not be unique */
> +	if (this_leaf->type != CACHE_TYPE_UNIFIED)
> +		return;
> +
> +	if (!this_leaf->fw_token)
> +		return;
> +	cpu_node = this_leaf->fw_token;
> +
> +	status = acpi_get_table(ACPI_SIG_PPTT, 0, &table);
> +	if (ACPI_FAILURE(status))
> +		return;
> +
> +	acpi_pptt_min_physid_from_cpu_node(table, cpu_node, &min_physid);
> +	acpi_put_table(table);
> +
> +	WARN_ON_ONCE(min_physid == PHYS_CPUID_INVALID);
> +
> +	this_leaf->id = ARCH_PHYSID_TO_U32(min_physid);
> +	this_leaf->attributes |= CACHE_ID;
> +}
> +
>  /**
>   * update_cache_properties() - Update cacheinfo for the given processor
>   * @this_leaf: Kernel cache info structure being updated @@ -408,6 +486,9
> @@ static void update_cache_properties(struct cacheinfo *this_leaf,
>  	if (this_leaf->type == CACHE_TYPE_NOCACHE &&
>  	    valid_flags == PPTT_CHECKED_ATTRIBUTES)
>  		this_leaf->type = CACHE_TYPE_UNIFIED;
> +
> +	/* Now that the type is known, try and generate an id. */
> +	acpi_pptt_label_cache(this_leaf);
>  }
> 
>  static void cache_setup_acpi_cpu(struct acpi_table_header *table,
> --
> 2.18.0
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 2/2] ACPI / PPTT: cacheinfo: Label caches based on fw_token

[James Morse]

Hi Shameer,

On 17/06/2019 09:28, Shameerali Kolothum Thodi wrote:
>> -----Original Message-----

>> The resctrl ABI requires caches to have a unique id. This number must be
>> unique across all caches at this level, but doesn't need to be contiguous. (there
>> may be gaps, it may not start at 0).
>> See Documentation/x86/intel_rdt_ui.txt::Cache IDs
>>
>> We want a value that is the same over reboots, and should be the same on
>> identical hardware, even if the PPTT is generated in a different order. The
>> hardware doesn't give us any indication of which caches are shared, so this
>> information must come from firmware tables.
>>
>> Starting with a cacheinfo's fw_token, we walk the table to find all CPUs that
>> share this cpu_node (and thus cache), and take the lowest physical id to use as
>> the id for the cache. On arm64 this value corresponds to the MPIDR.
>>
>> This is only done for unified caches, as instruction/data caches would generate
>> the same id using this scheme.


>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c index
>> d1e26cb599bf..9478f8c28158 100644
>> --- a/drivers/acpi/pptt.c
>> +++ b/drivers/acpi/pptt.c
>> @@ -341,6 +341,84 @@ static struct acpi_pptt_cache
>> +/**
>> + * acpi_pptt_min_physid_from_cpu_node() - Recursivly find @min_physid
>> +for all
>> + * leaf CPUs below @cpu_node.
>> + * @table_hdr:	Pointer to the head of the PPTT table
>> + * @cpu_node:	The point in the toplogy to start the walk
>> + * @min_physid:	The min_physid to update with leaf CPUs.
>> + */
>> +void acpi_pptt_min_physid_from_cpu_node(struct acpi_table_header
>> *table_hdr,
>> +					struct acpi_pptt_processor *cpu_node,
>> +					phys_cpuid_t *min_physid)
>> +{
>> +	bool leaf = true;
>> +	u32 acpi_processor_id;
>> +	phys_cpuid_t cpu_node_phys_id;
>> +	struct acpi_subtable_header *iter;
>> +	struct acpi_pptt_processor *iter_node;
>> +	u32 target_node = ACPI_PTR_DIFF(cpu_node, table_hdr);
>> +	u32 proc_sz = sizeof(struct acpi_pptt_processor *);
>> +	unsigned long table_end = (unsigned long)table_hdr +
>> +table_hdr->length;
>> +
>> +	/*
>> +	 * Walk the PPTT, looking for nodes that reference cpu_node
>> +	 * as parent.
>> +	 */
>> +	iter = ACPI_ADD_PTR(struct acpi_subtable_header, table_hdr,
>> +			     sizeof(struct acpi_table_pptt));
>> +
>> +	while ((unsigned long)iter + proc_sz < table_end) {
>> +		iter_node = (struct acpi_pptt_processor *)iter;
>> +
>> +		if (iter->type == ACPI_PPTT_TYPE_PROCESSOR &&
>> +		    iter_node->parent == target_node) {
>> +			leaf = false;
>> +			acpi_pptt_min_physid_from_cpu_node(table_hdr, iter_node,
>> +							   min_physid);
>> +		}
>> +
>> +		if (iter->length == 0)
>> +			return;
>> +		iter = ACPI_ADD_PTR(struct acpi_subtable_header, iter,
>> +				    iter->length);
>> +	}
>> +
>> +	if (leaf && cpu_node->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID) {
>> +		acpi_processor_id = cpu_node->acpi_processor_id;
>> +		cpu_node_phys_id = acpi_id_to_phys_cpuid(acpi_processor_id);
>> +		*min_physid = min(*min_physid, cpu_node_phys_id);
>> +	}
>> +}

> I was just trying out the latest public MPAM branch available here[1]

Great!


> and noted that
> on our HiSilicon platform all the L3 cache were labeled with the same Id. Debugging> revealed that the above leaf node check was removed in this branch[2] which makes
> the min_physid calculation going wrong.

Thanks for debugging this,

> Just wondering is there any particular reason
> for removing the check or the branch is not carrying the latest patch?

Nope, that's a bug.

Jeremy Linton's review feedback[0] was that that PROCESSOR_ID_VALID flag can't be relied
on. It looks like I over-zealously removed the whole if(), and this doesn't cause a
problem with my pptt so I didn't notice.

I've fixed it locally, I've also pushed a fix to those branches, but it will get folded in
next time I push a branch.


Thanks!

James

[0] lore.kernel.org/r/a68abfd2-1e28-d9e7-919a-8b3133db4d20@arm.com

Re: [RFC PATCH 0/2] ACPI / PPTT: ids for caches

[Jeffrey Hugo]

On 10/5/2018 9:02 AM, James Morse wrote:
> Hi guys,
> 
> To get resctrl working on arm64, we need to generate 'id's for caches.
> This is this value that shows up in, e.g.:
> | /sys/devices/system/cpu/cpu0/cache/index3/id
> 
> This value needs to be unique for each level of cache, but doesn't
> need to be contiguous. (there may be gaps, it may not start at 0).
> Details in Documentation/x86/intel_rdt_ui.txt::Cache IDs
> 
> resctrl receives these values back via its schemata file. e.g.:
> | echo "L3:0=fff;1=fff" > /sys/fs/resctrl/p1/schemata
> Where 0 and 1 are the ids of two caches in the system.
> 
> These values become ABI, and are likely to be baked into shell scripts.
> We want a value that is the same over reboots, and should be the same
> on identical hardware, even if the PPTT is generated in a different
> order. The hardware doesn't give us any indication of which caches are
> shared, so this information must come from firmware tables.
> 
> This series generates an id from the PPTT topology, based on the lowest
> MPIDR of the cpus that share a cache.
> 
> The remaining problems with this approach are:
>   * the 32bit ID field is full of MPIDR.Aff{0-3}. We don't have space to
>     hide 'i/d/unified', so can only generate ids for unified caches. If we
>     ever get an Aff4 (plenty of RES0 space in there) we can no longer generate
>     an id. Having all these bits accounted for in the initial version doesn't
>     feel like a good ABI choice.
> 
> * Existing software is going to assume caches are numbered 0,1,2. This was
>    documented as not guaranteed, and its likely never going to be the case
>    if we generate ids like this.
> 
> * The table walk is recursive.
> 
> 
> Fixes for the first two require extra-code to compact the ID range, which would
> require us generating all the IDs up front, not from hotplug callbacks as has
> to happen today.
> 
> Alternatively, we could try and change the abi to provide a u64 as the
> cache id. The size isn't documented, and for resctrl userspace can treat
> it as a string.
> 
> Better ideas welcome!

I'm sorry, I'm not familiar with this resctrl, and therefore I don't 
quite feel like I have a handle on what we need out of the ids file (and 
the Documentation you pointed to doesn't seem to clarify it for me).

Lets assume we have a trivial 4 core system.  Each core has a private 
L1i and L1d cache.  Cores 0/1 and 2/3 share a L2.  Cores 0-3 share L3.

If we are assigning ids in the range 1-N, what might we expect the id of 
each cache to be?

Is this sane (each unique cache instance has a unique id), or have I 
misunderstood?
CPU0 L1i - 1
CPU0 L1d - 2
CPU1 L1i - 3
CPU1 L1d - 4
CPU2 L1i - 5
CPU2 L1d - 6
CPU3 L1i - 7
CPU3 L1d - 8
CPU0/1 L2 - 9
CPU2/3 L2 - 10
        L3 - 11

-- 
Jeffrey Hugo
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: [RFC PATCH 0/2] ACPI / PPTT: ids for caches

[James Morse]

Hi Jeffrey,

On 05/10/18 16:20, Jeffrey Hugo wrote:
> On 10/5/2018 9:02 AM, James Morse wrote:
>> To get resctrl working on arm64, we need to generate 'id's for caches.
>> This is this value that shows up in, e.g.:
>> | /sys/devices/system/cpu/cpu0/cache/index3/id
>>
>> This value needs to be unique for each level of cache, but doesn't
>> need to be contiguous. (there may be gaps, it may not start at 0).
>> Details in Documentation/x86/intel_rdt_ui.txt::Cache IDs
>>
>> resctrl receives these values back via its schemata file. e.g.:
>> | echo "L3:0=fff;1=fff" > /sys/fs/resctrl/p1/schemata
>> Where 0 and 1 are the ids of two caches in the system.
>>
>> These values become ABI, and are likely to be baked into shell scripts.
>> We want a value that is the same over reboots, and should be the same
>> on identical hardware, even if the PPTT is generated in a different
>> order. The hardware doesn't give us any indication of which caches are
>> shared, so this information must come from firmware tables.
>>
>> This series generates an id from the PPTT topology, based on the lowest
>> MPIDR of the cpus that share a cache.
>>
>> The remaining problems with this approach are:
>>   * the 32bit ID field is full of MPIDR.Aff{0-3}. We don't have space to
>>     hide 'i/d/unified', so can only generate ids for unified caches. If we
>>     ever get an Aff4 (plenty of RES0 space in there) we can no longer generate
>>     an id. Having all these bits accounted for in the initial version doesn't
>>     feel like a good ABI choice.
>>
>> * Existing software is going to assume caches are numbered 0,1,2. This was
>>    documented as not guaranteed, and its likely never going to be the case
>>    if we generate ids like this.
>>
>> * The table walk is recursive.
>>
>>
>> Fixes for the first two require extra-code to compact the ID range, which would
>> require us generating all the IDs up front, not from hotplug callbacks as has
>> to happen today.
>>
>> Alternatively, we could try and change the abi to provide a u64 as the
>> cache id. The size isn't documented, and for resctrl userspace can treat
>> it as a string.
>>
>> Better ideas welcome!
> 
> I'm sorry, I'm not familiar with this resctrl, and therefore I don't quite feel
> like I have a handle on what we need out of the ids file (and the Documentation
> you pointed to doesn't seem to clarify it for me).

> Lets assume we have a trivial 4 core system.  Each core has a private L1i and
> L1d cache.  Cores 0/1 and 2/3 share a L2.  Cores 0-3 share L3.

The i/d caches wouldn't get an ID, because we can't easily generate unique
values for these. (with this scheme, all the id bits are in use for shared
unified caches).

Cores 0 and 1 should show the same ID for their L2, 2 and 3 should show a
different ID. Cores 0-3 should all show the same id for L3.


> If we are assigning ids in the range 1-N, what might we expect the id of each
> cache to be?
> 
> Is this sane (each unique cache instance has a unique id), or have I misunderstood?
> CPU0 L1i - 1
> CPU0 L1d - 2
> CPU1 L1i - 3
> CPU1 L1d - 4
> CPU2 L1i - 5
> CPU2 L1d - 6
> CPU3 L1i - 7
> CPU3 L1d - 8

> CPU0/1 L2 - 9
> CPU2/3 L2 - 10

>        L3 - 11

This would be sane. We don't need to continue the numbering between L1/L2/L3.
The id only needs to be unique at that level.


The problem is generating these numbers if only some of the CPUs are online, or
if the acpi tables are generated by firmware at power-on and have a different
layout every time.
We don't even want to rely on linux's cpu numbering.

The suggestion here is to use the smallest MPIDR, as that's as hardware property
that won't change even if the tables are generated differently every boot.

Assuming two clusters in your example above, it would look like:

| CPU0/1 (cluster 0) L2 - 0x0
| CPU2/3 (cluster 1) L2 - 0x100
|                    L3 - 0x0



Thanks,

James

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 0/2] ACPI / PPTT: ids for caches

[Jeffrey Hugo]

On 10/5/2018 9:54 AM, James Morse wrote:
> Hi Jeffrey,
> 
> On 05/10/18 16:20, Jeffrey Hugo wrote:
>> On 10/5/2018 9:02 AM, James Morse wrote:
>>> To get resctrl working on arm64, we need to generate 'id's for caches.
>>> This is this value that shows up in, e.g.:
>>> | /sys/devices/system/cpu/cpu0/cache/index3/id
>>>
>>> This value needs to be unique for each level of cache, but doesn't
>>> need to be contiguous. (there may be gaps, it may not start at 0).
>>> Details in Documentation/x86/intel_rdt_ui.txt::Cache IDs
>>>
>>> resctrl receives these values back via its schemata file. e.g.:
>>> | echo "L3:0=fff;1=fff" > /sys/fs/resctrl/p1/schemata
>>> Where 0 and 1 are the ids of two caches in the system.
>>>
>>> These values become ABI, and are likely to be baked into shell scripts.
>>> We want a value that is the same over reboots, and should be the same
>>> on identical hardware, even if the PPTT is generated in a different
>>> order. The hardware doesn't give us any indication of which caches are
>>> shared, so this information must come from firmware tables.
>>>
>>> This series generates an id from the PPTT topology, based on the lowest
>>> MPIDR of the cpus that share a cache.
>>>
>>> The remaining problems with this approach are:
>>>    * the 32bit ID field is full of MPIDR.Aff{0-3}. We don't have space to
>>>      hide 'i/d/unified', so can only generate ids for unified caches. If we
>>>      ever get an Aff4 (plenty of RES0 space in there) we can no longer generate
>>>      an id. Having all these bits accounted for in the initial version doesn't
>>>      feel like a good ABI choice.
>>>
>>> * Existing software is going to assume caches are numbered 0,1,2. This was
>>>     documented as not guaranteed, and its likely never going to be the case
>>>     if we generate ids like this.
>>>
>>> * The table walk is recursive.
>>>
>>>
>>> Fixes for the first two require extra-code to compact the ID range, which would
>>> require us generating all the IDs up front, not from hotplug callbacks as has
>>> to happen today.
>>>
>>> Alternatively, we could try and change the abi to provide a u64 as the
>>> cache id. The size isn't documented, and for resctrl userspace can treat
>>> it as a string.
>>>
>>> Better ideas welcome!
>>
>> I'm sorry, I'm not familiar with this resctrl, and therefore I don't quite feel
>> like I have a handle on what we need out of the ids file (and the Documentation
>> you pointed to doesn't seem to clarify it for me).
> 
>> Lets assume we have a trivial 4 core system.  Each core has a private L1i and
>> L1d cache.  Cores 0/1 and 2/3 share a L2.  Cores 0-3 share L3.
> 
> The i/d caches wouldn't get an ID, because we can't easily generate unique
> values for these. (with this scheme, all the id bits are in use for shared
> unified caches).
> 
> Cores 0 and 1 should show the same ID for their L2, 2 and 3 should show a
> different ID. Cores 0-3 should all show the same id for L3.
> 
> 
>> If we are assigning ids in the range 1-N, what might we expect the id of each
>> cache to be?
>>
>> Is this sane (each unique cache instance has a unique id), or have I misunderstood?
>> CPU0 L1i - 1
>> CPU0 L1d - 2
>> CPU1 L1i - 3
>> CPU1 L1d - 4
>> CPU2 L1i - 5
>> CPU2 L1d - 6
>> CPU3 L1i - 7
>> CPU3 L1d - 8
> 
>> CPU0/1 L2 - 9
>> CPU2/3 L2 - 10
> 
>>         L3 - 11
> 
> This would be sane. We don't need to continue the numbering between L1/L2/L3.
> The id only needs to be unique at that level.
> 
> 
> The problem is generating these numbers if only some of the CPUs are online, or
> if the acpi tables are generated by firmware at power-on and have a different
> layout every time.
> We don't even want to rely on linux's cpu numbering.
> 
> The suggestion here is to use the smallest MPIDR, as that's as hardware property
> that won't change even if the tables are generated differently every boot.

I can't think of a reason why affinity level 0 would ever change for a 
particular thread or core (SMT vs non-SMT), however none of the other 
affinity levels have a well defined meaning (implementation dependent), 
and could very well change boot to boot.

I would strongly avoid using MPIDR, particularly for the usecase you've 
described.

> 
> Assuming two clusters in your example above, it would look like:
> 
> | CPU0/1 (cluster 0) L2 - 0x0
> | CPU2/3 (cluster 1) L2 - 0x100
> |                    L3 - 0x0

Thanks for the clarification.  I think I've got enough to wrap my head 
around this.  Let me think on it a bit to see if I can come up with a 
suggestion (we can debate how good it is).

-- 
Jeffrey Hugo
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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 0/2] ACPI / PPTT: ids for caches

[James Morse]

Hi Jeffrey,

On 05/10/2018 17:39, Jeffrey Hugo wrote:
> On 10/5/2018 9:54 AM, James Morse wrote:
>> The problem is generating these numbers if only some of the CPUs are online, or
>> if the acpi tables are generated by firmware at power-on and have a different
>> layout every time.
>> We don't even want to rely on linux's cpu numbering.
>>
>> The suggestion here is to use the smallest MPIDR, as that's as hardware property
>> that won't change even if the tables are generated differently every boot.
> 
> I can't think of a reason why affinity level 0 would ever change for a

affinity level of what? The caches? Sure, that should be impossible to change.


> particular thread or core (SMT vs non-SMT), however none of the other affinity
> levels have a well defined meaning (implementation dependent), and could very
> well change boot to boot.

Ah, you mean the level numbers. Yes, these are (quasi) discovered from the
table, so can't be relied on.

If you insert a new level then this would shuffle the user-visible indexes and
levels. I would argue this is no longer the same hardware.

Doing this may already break resctrl as the 'L2' and 'L3' numbers are part of
the ABI.

The ids generated would still be unique for a level.


> I would strongly avoid using MPIDR, particularly for the usecase you've described.

Is there an alternative? It needs to be a hardware property to insulate us from
the tables being re-generated.

I agree the MPIDR numbers are likely to be ugly, (I don't really care...).
The u32 id being full from day 1 is more of a problem.


>> Assuming two clusters in your example above, it would look like:
>>
>> | CPU0/1 (cluster 0) L2 - 0x0
>> | CPU2/3 (cluster 1) L2 - 0x100
>> |                    L3 - 0x0
> 
> Thanks for the clarification.  I think I've got enough to wrap my head around
> this.  Let me think on it a bit to see if I can come up with a suggestion (we
> can debate how good it is).

Sounds good.


Thanks!

James

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 0/2] ACPI / PPTT: ids for caches

[Jeffrey Hugo]

On 10/8/2018 3:26 AM, James Morse wrote:
> Hi Jeffrey,
> 
> On 05/10/2018 17:39, Jeffrey Hugo wrote:
>> On 10/5/2018 9:54 AM, James Morse wrote:
>>> The problem is generating these numbers if only some of the CPUs are online, or
>>> if the acpi tables are generated by firmware at power-on and have a different
>>> layout every time.
>>> We don't even want to rely on linux's cpu numbering.
>>>
>>> The suggestion here is to use the smallest MPIDR, as that's as hardware property
>>> that won't change even if the tables are generated differently every boot.
>>
>> I can't think of a reason why affinity level 0 would ever change for a
> 
> affinity level of what? The caches? Sure, that should be impossible to change.
> 
> 
>> particular thread or core (SMT vs non-SMT), however none of the other affinity
>> levels have a well defined meaning (implementation dependent), and could very
>> well change boot to boot.
> 
> Ah, you mean the level numbers. Yes, these are (quasi) discovered from the
> table, so can't be relied on.
> 
> If you insert a new level then this would shuffle the user-visible indexes and
> levels. I would argue this is no longer the same hardware.

Well, so its common in my experience for x86 server hardware to allow 
the user to enable/disable SMT.  This seems to allow the user to 
configure the hardware to better suit their workloads.

Applying this to ARM, You might have a system where it starts in non-SMT 
mode:

Aff3: 0
Aff2: 0
Aff1: 0
Aff0: X

however, after changing the setting to enable SMT and a reboot:

Aff3: 0
Aff2: 0
Aff1: Y
Aff0: X

If you base the cache ids on MPIDR, then they would likely change SMT vs 
non-SMT for example.  However, you seem to consider that as different 
hardware, and therefore if the cache ids change, so be it.

> 
> Doing this may already break resctrl as the 'L2' and 'L3' numbers are part of
> the ABI.
> 
> The ids generated would still be unique for a level.
> 
> 
>> I would strongly avoid using MPIDR, particularly for the usecase you've described.
> 
> Is there an alternative? It needs to be a hardware property to insulate us from
> the tables being re-generated.
> 
> I agree the MPIDR numbers are likely to be ugly, (I don't really care...).
> The u32 id being full from day 1 is more of a problem.

I agree, it needs to be a hardware property, and for the purposes of 
uniquely identifying a core, I'm not seeing an alternative to MPIDR.  I 
think that MPIDR should be ok so long as we don't attempt to derive any 
additional information from it, other than its an opaque value which 
uniquely identifies a core (or thread).

Since you have a u32 cache id, and MPIDR is a u64, I'm not really 
thrilled with trying to jam MPIDR into the cache id.  Also, as you point 
out, even though its not guaranteed, SW is going to expect the cache ids 
are numbered 0, 1, 2, etc.  I've seen issues with virtio where MPIDR 
values were exposed via sysfs as unique CPU identifiers, and it ended up 
preventing VMs from initializing on the system.

I suspect identifying the list of MPIDRs in the system, sorting them, 
and then assigning cache ids based on the sorted list position would be 
the best solution, ignoring that implementing it in PPTT parsing is 
probably going to be painful.

> 
> 
>>> Assuming two clusters in your example above, it would look like:
>>>
>>> | CPU0/1 (cluster 0) L2 - 0x0
>>> | CPU2/3 (cluster 1) L2 - 0x100
>>> |                    L3 - 0x0
>>
>> Thanks for the clarification.  I think I've got enough to wrap my head around
>> this.  Let me think on it a bit to see if I can come up with a suggestion (we
>> can debate how good it is).
> 
> Sounds good.
> 
> 
> Thanks!


-- 
Jeffrey Hugo
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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel