Deterministic cache parameters can be learned from CPUID leaf 04H.
Executing CPUID with a particular index in EAX would return the cache
parameters associated with that index in the EAX, EBX, ECX, and EDX
registers.
At this time, when discovering cache parameters for a particular cache
index, only the parameters returned in EAX, EBX, and ECX are parsed.
Parameters returned in EDX are ignored. One of the parameters in EDX,
whether the cache is inclusive of lower level caches, is valuable to
know when determining if a system can support L3 cache pseudo-locking.
If the L3 cache is not inclusive then pseudo-locked data within the L3
cache would be evicted when migrated to L2.
Add support for parsing the cache parameters obtained from EDX and make
the inclusive cache parameter available via the cacheinfo that can be
queried from the cache pseudo-locking code.
Do not expose this information to user space at this time. At this time
this information is required within the kernel only. Also, it is
not obvious what the best formatting of this information should be in
support of the variety of ways users may use this information.
Signed-off-by: Reinette Chatre <[email protected]>
---
arch/x86/kernel/cpu/cacheinfo.c | 42 +++++++++++++++++++++++++++++----
include/linux/cacheinfo.h | 4 ++++
2 files changed, 42 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
index c7503be92f35..3b678f46be53 100644
--- a/arch/x86/kernel/cpu/cacheinfo.c
+++ b/arch/x86/kernel/cpu/cacheinfo.c
@@ -154,10 +154,33 @@ union _cpuid4_leaf_ecx {
u32 full;
};
+/*
+ * According to details about CPUID instruction documented in Intel SDM
+ * the third bit of the EDX register is used to indicate if complex
+ * cache indexing is in use.
+ * According to AMD specification (Open Source Register Reference For AMD
+ * Family 17h processors Models 00h-2Fh 56255 Rev 3.03 - July, 2018), only
+ * the first two bits are in use. Since HYGON is based on AMD the
+ * assumption is that it supports the same.
+ *
+ * There is no consumer for the complex indexing information so this bit is
+ * not added to the declaration of what processor can provide in EDX
+ * register. The declaration thus only considers bits supported by all
+ * architectures.
+ */
+union _cpuid4_leaf_edx {
+ struct {
+ unsigned int wbinvd_no_guarantee:1;
+ unsigned int inclusive:1;
+ } split;
+ u32 full;
+};
+
struct _cpuid4_info_regs {
union _cpuid4_leaf_eax eax;
union _cpuid4_leaf_ebx ebx;
union _cpuid4_leaf_ecx ecx;
+ union _cpuid4_leaf_edx edx;
unsigned int id;
unsigned long size;
struct amd_northbridge *nb;
@@ -595,21 +618,24 @@ cpuid4_cache_lookup_regs(int index, struct _cpuid4_info_regs *this_leaf)
union _cpuid4_leaf_eax eax;
union _cpuid4_leaf_ebx ebx;
union _cpuid4_leaf_ecx ecx;
- unsigned edx;
+ union _cpuid4_leaf_edx edx;
+
+ edx.full = 0;
if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
if (boot_cpu_has(X86_FEATURE_TOPOEXT))
cpuid_count(0x8000001d, index, &eax.full,
- &ebx.full, &ecx.full, &edx);
+ &ebx.full, &ecx.full, &edx.full);
else
amd_cpuid4(index, &eax, &ebx, &ecx);
amd_init_l3_cache(this_leaf, index);
} else if (boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
cpuid_count(0x8000001d, index, &eax.full,
- &ebx.full, &ecx.full, &edx);
+ &ebx.full, &ecx.full, &edx.full);
amd_init_l3_cache(this_leaf, index);
} else {
- cpuid_count(4, index, &eax.full, &ebx.full, &ecx.full, &edx);
+ cpuid_count(4, index, &eax.full, &ebx.full, &ecx.full,
+ &edx.full);
}
if (eax.split.type == CTYPE_NULL)
@@ -618,6 +644,7 @@ cpuid4_cache_lookup_regs(int index, struct _cpuid4_info_regs *this_leaf)
this_leaf->eax = eax;
this_leaf->ebx = ebx;
this_leaf->ecx = ecx;
+ this_leaf->edx = edx;
this_leaf->size = (ecx.split.number_of_sets + 1) *
(ebx.split.coherency_line_size + 1) *
(ebx.split.physical_line_partition + 1) *
@@ -982,6 +1009,13 @@ static void ci_leaf_init(struct cacheinfo *this_leaf,
this_leaf->number_of_sets = base->ecx.split.number_of_sets + 1;
this_leaf->physical_line_partition =
base->ebx.split.physical_line_partition + 1;
+ if ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
+ boot_cpu_has(X86_FEATURE_TOPOEXT)) ||
+ boot_cpu_data.x86_vendor == X86_VENDOR_HYGON ||
+ boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) {
+ this_leaf->attributes |= CACHE_INCLUSIVE_SET;
+ this_leaf->inclusive = base->edx.split.inclusive;
+ }
this_leaf->priv = base->nb;
}
diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
index 46b92cd61d0c..cdc7a9d6923f 100644
--- a/include/linux/cacheinfo.h
+++ b/include/linux/cacheinfo.h
@@ -33,6 +33,8 @@ extern unsigned int coherency_max_size;
* @physical_line_partition: number of physical cache lines sharing the
* same cachetag
* @size: Total size of the cache
+ * @inclusive: Cache is inclusive of lower level caches. Only valid if
+ * CACHE_INCLUSIVE_SET attribute is set.
* @shared_cpu_map: logical cpumask representing all the cpus sharing
* this cache node
* @attributes: bitfield representing various cache attributes
@@ -55,6 +57,7 @@ struct cacheinfo {
unsigned int ways_of_associativity;
unsigned int physical_line_partition;
unsigned int size;
+ unsigned int inclusive;
cpumask_t shared_cpu_map;
unsigned int attributes;
#define CACHE_WRITE_THROUGH BIT(0)
@@ -66,6 +69,7 @@ struct cacheinfo {
#define CACHE_ALLOCATE_POLICY_MASK \
(CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE)
#define CACHE_ID BIT(4)
+#define CACHE_INCLUSIVE_SET BIT(5)
void *fw_token;
bool disable_sysfs;
void *priv;
--
2.17.2
On Tue, Jul 30, 2019 at 10:29:35AM -0700, Reinette Chatre wrote:
> Deterministic cache parameters can be learned from CPUID leaf 04H.
> Executing CPUID with a particular index in EAX would return the cache
> parameters associated with that index in the EAX, EBX, ECX, and EDX
> registers.
>
> At this time, when discovering cache parameters for a particular cache
> index, only the parameters returned in EAX, EBX, and ECX are parsed.
> Parameters returned in EDX are ignored. One of the parameters in EDX,
> whether the cache is inclusive of lower level caches, is valuable to
> know when determining if a system can support L3 cache pseudo-locking.
> If the L3 cache is not inclusive then pseudo-locked data within the L3
> cache would be evicted when migrated to L2.
>
> Add support for parsing the cache parameters obtained from EDX and make
> the inclusive cache parameter available via the cacheinfo that can be
> queried from the cache pseudo-locking code.
>
> Do not expose this information to user space at this time. At this time
> this information is required within the kernel only. Also, it is
> not obvious what the best formatting of this information should be in
> support of the variety of ways users may use this information.
>
> Signed-off-by: Reinette Chatre <[email protected]>
> ---
> arch/x86/kernel/cpu/cacheinfo.c | 42 +++++++++++++++++++++++++++++----
> include/linux/cacheinfo.h | 4 ++++
> 2 files changed, 42 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
> index c7503be92f35..3b678f46be53 100644
> --- a/arch/x86/kernel/cpu/cacheinfo.c
> +++ b/arch/x86/kernel/cpu/cacheinfo.c
> @@ -154,10 +154,33 @@ union _cpuid4_leaf_ecx {
> u32 full;
> };
>
> +/*
> + * According to details about CPUID instruction documented in Intel SDM
> + * the third bit of the EDX register is used to indicate if complex
> + * cache indexing is in use.
> + * According to AMD specification (Open Source Register Reference For AMD
> + * Family 17h processors Models 00h-2Fh 56255 Rev 3.03 - July, 2018), only
> + * the first two bits are in use. Since HYGON is based on AMD the
> + * assumption is that it supports the same.
> + *
> + * There is no consumer for the complex indexing information so this bit is
> + * not added to the declaration of what processor can provide in EDX
> + * register. The declaration thus only considers bits supported by all
> + * architectures.
> + */
I don't think you need all that commenting in here since it'll become
stale as soon as the other vendors change their respective 0x8000001D
leafs. It is sufficient to say that the union below is going to contain
only the bits shared by all vendors.
> +union _cpuid4_leaf_edx {
> + struct {
> + unsigned int wbinvd_no_guarantee:1;
^^^^^^^^^^^^^^^^^^^^^
That's unused so no need to name the bit. You only need "inclusive".
> + unsigned int inclusive:1;
> + } split;
> + u32 full;
> +};
> +
> struct _cpuid4_info_regs {
> union _cpuid4_leaf_eax eax;
> union _cpuid4_leaf_ebx ebx;
> union _cpuid4_leaf_ecx ecx;
> + union _cpuid4_leaf_edx edx;
> unsigned int id;
> unsigned long size;
> struct amd_northbridge *nb;
> @@ -595,21 +618,24 @@ cpuid4_cache_lookup_regs(int index, struct _cpuid4_info_regs *this_leaf)
> union _cpuid4_leaf_eax eax;
> union _cpuid4_leaf_ebx ebx;
> union _cpuid4_leaf_ecx ecx;
> - unsigned edx;
> + union _cpuid4_leaf_edx edx;
> +
> + edx.full = 0;
Yeah, the proper way to shut up gcc is to do this (diff ontop):
---
diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
index 3b678f46be53..4ff4e95e22b4 100644
--- a/arch/x86/kernel/cpu/cacheinfo.c
+++ b/arch/x86/kernel/cpu/cacheinfo.c
@@ -252,7 +252,8 @@ static const enum cache_type cache_type_map[] = {
static void
amd_cpuid4(int leaf, union _cpuid4_leaf_eax *eax,
union _cpuid4_leaf_ebx *ebx,
- union _cpuid4_leaf_ecx *ecx)
+ union _cpuid4_leaf_ecx *ecx,
+ union _cpuid4_leaf_edx *edx)
{
unsigned dummy;
unsigned line_size, lines_per_tag, assoc, size_in_kb;
@@ -264,6 +265,7 @@ amd_cpuid4(int leaf, union _cpuid4_leaf_eax *eax,
eax->full = 0;
ebx->full = 0;
ecx->full = 0;
+ edx->full = 0;
cpuid(0x80000005, &dummy, &dummy, &l1d.val, &l1i.val);
cpuid(0x80000006, &dummy, &dummy, &l2.val, &l3.val);
@@ -620,14 +622,12 @@ cpuid4_cache_lookup_regs(int index, struct _cpuid4_info_regs *this_leaf)
union _cpuid4_leaf_ecx ecx;
union _cpuid4_leaf_edx edx;
- edx.full = 0;
-
if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
if (boot_cpu_has(X86_FEATURE_TOPOEXT))
cpuid_count(0x8000001d, index, &eax.full,
&ebx.full, &ecx.full, &edx.full);
else
- amd_cpuid4(index, &eax, &ebx, &ecx);
+ amd_cpuid4(index, &eax, &ebx, &ecx, &edx);
amd_init_l3_cache(this_leaf, index);
} else if (boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
cpuid_count(0x8000001d, index, &eax.full,
>
> if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
> if (boot_cpu_has(X86_FEATURE_TOPOEXT))
> cpuid_count(0x8000001d, index, &eax.full,
> - &ebx.full, &ecx.full, &edx);
> + &ebx.full, &ecx.full, &edx.full);
> else
> amd_cpuid4(index, &eax, &ebx, &ecx);
> amd_init_l3_cache(this_leaf, index);
> } else if (boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
> cpuid_count(0x8000001d, index, &eax.full,
> - &ebx.full, &ecx.full, &edx);
> + &ebx.full, &ecx.full, &edx.full);
> amd_init_l3_cache(this_leaf, index);
> } else {
> - cpuid_count(4, index, &eax.full, &ebx.full, &ecx.full, &edx);
> + cpuid_count(4, index, &eax.full, &ebx.full, &ecx.full,
> + &edx.full);
Let that line stick out and rename "index" to "idx" so that it fits the
80 cols rule.
> }
>
> if (eax.split.type == CTYPE_NULL)
> @@ -618,6 +644,7 @@ cpuid4_cache_lookup_regs(int index, struct _cpuid4_info_regs *this_leaf)
> this_leaf->eax = eax;
> this_leaf->ebx = ebx;
> this_leaf->ecx = ecx;
> + this_leaf->edx = edx;
> this_leaf->size = (ecx.split.number_of_sets + 1) *
> (ebx.split.coherency_line_size + 1) *
> (ebx.split.physical_line_partition + 1) *
> @@ -982,6 +1009,13 @@ static void ci_leaf_init(struct cacheinfo *this_leaf,
> this_leaf->number_of_sets = base->ecx.split.number_of_sets + 1;
> this_leaf->physical_line_partition =
> base->ebx.split.physical_line_partition + 1;
<---- newline here.
> + if ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
> + boot_cpu_has(X86_FEATURE_TOPOEXT)) ||
> + boot_cpu_data.x86_vendor == X86_VENDOR_HYGON ||
> + boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) {
> + this_leaf->attributes |= CACHE_INCLUSIVE_SET;
> + this_leaf->inclusive = base->edx.split.inclusive;
A whole int for a single bit?
Why don't you define the CACHE_INCLUSIVE_SET thing as CACHE_INCLUSIVE to
mean if set, the cache is inclusive, if not, it isn't or unknown?
> + }
> this_leaf->priv = base->nb;
> }
>
> diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
> index 46b92cd61d0c..cdc7a9d6923f 100644
> --- a/include/linux/cacheinfo.h
> +++ b/include/linux/cacheinfo.h
> @@ -33,6 +33,8 @@ extern unsigned int coherency_max_size;
> * @physical_line_partition: number of physical cache lines sharing the
> * same cachetag
> * @size: Total size of the cache
> + * @inclusive: Cache is inclusive of lower level caches. Only valid if
> + * CACHE_INCLUSIVE_SET attribute is set.
> * @shared_cpu_map: logical cpumask representing all the cpus sharing
> * this cache node
> * @attributes: bitfield representing various cache attributes
> @@ -55,6 +57,7 @@ struct cacheinfo {
> unsigned int ways_of_associativity;
> unsigned int physical_line_partition;
> unsigned int size;
> + unsigned int inclusive;
> cpumask_t shared_cpu_map;
> unsigned int attributes;
> #define CACHE_WRITE_THROUGH BIT(0)
> @@ -66,6 +69,7 @@ struct cacheinfo {
> #define CACHE_ALLOCATE_POLICY_MASK \
> (CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE)
> #define CACHE_ID BIT(4)
> +#define CACHE_INCLUSIVE_SET BIT(5)
> void *fw_token;
> bool disable_sysfs;
> void *priv;
> --
> 2.17.2
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
Hi Borislav,
On 8/2/2019 11:03 AM, Borislav Petkov wrote:
> On Tue, Jul 30, 2019 at 10:29:35AM -0700, Reinette Chatre wrote:
>> +/*
>> + * According to details about CPUID instruction documented in Intel SDM
>> + * the third bit of the EDX register is used to indicate if complex
>> + * cache indexing is in use.
>> + * According to AMD specification (Open Source Register Reference For AMD
>> + * Family 17h processors Models 00h-2Fh 56255 Rev 3.03 - July, 2018), only
>> + * the first two bits are in use. Since HYGON is based on AMD the
>> + * assumption is that it supports the same.
>> + *
>> + * There is no consumer for the complex indexing information so this bit is
>> + * not added to the declaration of what processor can provide in EDX
>> + * register. The declaration thus only considers bits supported by all
>> + * architectures.
>> + */
>
> I don't think you need all that commenting in here since it'll become
> stale as soon as the other vendors change their respective 0x8000001D
> leafs. It is sufficient to say that the union below is going to contain
> only the bits shared by all vendors.
Will do.
>
>> +union _cpuid4_leaf_edx {
>> + struct {
>> + unsigned int wbinvd_no_guarantee:1;
> ^^^^^^^^^^^^^^^^^^^^^
>
> That's unused so no need to name the bit. You only need "inclusive".
Will do.
>
>> + unsigned int inclusive:1;
>> + } split;
>> + u32 full;
>> +};
>> +
>> struct _cpuid4_info_regs {
>> union _cpuid4_leaf_eax eax;
>> union _cpuid4_leaf_ebx ebx;
>> union _cpuid4_leaf_ecx ecx;
>> + union _cpuid4_leaf_edx edx;
>> unsigned int id;
>> unsigned long size;
>> struct amd_northbridge *nb;
>> @@ -595,21 +618,24 @@ cpuid4_cache_lookup_regs(int index, struct _cpuid4_info_regs *this_leaf)
>> union _cpuid4_leaf_eax eax;
>> union _cpuid4_leaf_ebx ebx;
>> union _cpuid4_leaf_ecx ecx;
>> - unsigned edx;
>> + union _cpuid4_leaf_edx edx;
>> +
>> + edx.full = 0;
>
> Yeah, the proper way to shut up gcc is to do this (diff ontop):
>
> ---
> diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
> index 3b678f46be53..4ff4e95e22b4 100644
> --- a/arch/x86/kernel/cpu/cacheinfo.c
> +++ b/arch/x86/kernel/cpu/cacheinfo.c
> @@ -252,7 +252,8 @@ static const enum cache_type cache_type_map[] = {
> static void
> amd_cpuid4(int leaf, union _cpuid4_leaf_eax *eax,
> union _cpuid4_leaf_ebx *ebx,
> - union _cpuid4_leaf_ecx *ecx)
> + union _cpuid4_leaf_ecx *ecx,
> + union _cpuid4_leaf_edx *edx)
> {
> unsigned dummy;
> unsigned line_size, lines_per_tag, assoc, size_in_kb;
> @@ -264,6 +265,7 @@ amd_cpuid4(int leaf, union _cpuid4_leaf_eax *eax,
> eax->full = 0;
> ebx->full = 0;
> ecx->full = 0;
> + edx->full = 0;
>
> cpuid(0x80000005, &dummy, &dummy, &l1d.val, &l1i.val);
> cpuid(0x80000006, &dummy, &dummy, &l2.val, &l3.val);
> @@ -620,14 +622,12 @@ cpuid4_cache_lookup_regs(int index, struct _cpuid4_info_regs *this_leaf)
> union _cpuid4_leaf_ecx ecx;
> union _cpuid4_leaf_edx edx;
>
> - edx.full = 0;
> -
> if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
> if (boot_cpu_has(X86_FEATURE_TOPOEXT))
> cpuid_count(0x8000001d, index, &eax.full,
> &ebx.full, &ecx.full, &edx.full);
> else
> - amd_cpuid4(index, &eax, &ebx, &ecx);
> + amd_cpuid4(index, &eax, &ebx, &ecx, &edx);
> amd_init_l3_cache(this_leaf, index);
> } else if (boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
> cpuid_count(0x8000001d, index, &eax.full,
>
Thank you very much. I'll fix this.
>>
>> if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
>> if (boot_cpu_has(X86_FEATURE_TOPOEXT))
>> cpuid_count(0x8000001d, index, &eax.full,
>> - &ebx.full, &ecx.full, &edx);
>> + &ebx.full, &ecx.full, &edx.full);
>> else
>> amd_cpuid4(index, &eax, &ebx, &ecx);
>> amd_init_l3_cache(this_leaf, index);
>> } else if (boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
>> cpuid_count(0x8000001d, index, &eax.full,
>> - &ebx.full, &ecx.full, &edx);
>> + &ebx.full, &ecx.full, &edx.full);
>> amd_init_l3_cache(this_leaf, index);
>> } else {
>> - cpuid_count(4, index, &eax.full, &ebx.full, &ecx.full, &edx);
>> + cpuid_count(4, index, &eax.full, &ebx.full, &ecx.full,
>> + &edx.full);
>
> Let that line stick out and rename "index" to "idx" so that it fits the
> 80 cols rule.
Will do. Do you prefer a new prepare patch that does the renaming before
this patch or will you be ok with the renaming done within this patch?
>
>> }
>>
>> if (eax.split.type == CTYPE_NULL)
>> @@ -618,6 +644,7 @@ cpuid4_cache_lookup_regs(int index, struct _cpuid4_info_regs *this_leaf)
>> this_leaf->eax = eax;
>> this_leaf->ebx = ebx;
>> this_leaf->ecx = ecx;
>> + this_leaf->edx = edx;
>> this_leaf->size = (ecx.split.number_of_sets + 1) *
>> (ebx.split.coherency_line_size + 1) *
>> (ebx.split.physical_line_partition + 1) *
>> @@ -982,6 +1009,13 @@ static void ci_leaf_init(struct cacheinfo *this_leaf,
>> this_leaf->number_of_sets = base->ecx.split.number_of_sets + 1;
>> this_leaf->physical_line_partition =
>> base->ebx.split.physical_line_partition + 1;
>
> <---- newline here.
>
Will do.
>> + if ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
>> + boot_cpu_has(X86_FEATURE_TOPOEXT)) ||
>> + boot_cpu_data.x86_vendor == X86_VENDOR_HYGON ||
>> + boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) {
>> + this_leaf->attributes |= CACHE_INCLUSIVE_SET;
>> + this_leaf->inclusive = base->edx.split.inclusive;
>
> A whole int for a single bit?
>
> Why don't you define the CACHE_INCLUSIVE_SET thing as CACHE_INCLUSIVE to
> mean if set, the cache is inclusive, if not, it isn't or unknown?
>
This patch only makes it possible to determine whether cache is
inclusive for some x86 platforms while all platforms of all
architectures are given visibility into this new "inclusive" cache
information field within the global "struct cacheinfo". I did the above
because I wanted it to be possible to distinguish between the "not
inclusive" vs "unknown" case. With the above the "inclusive" field would
only be considered valid if "CACHE_INCLUSIVE_SET" is set.
You do seem to acknowledge this exact motivation though ... since you
explicitly state "isn't or unknown". Do I understand correctly that you
are ok with it not being possible to distinguish between "not inclusive"
and "unknown"?
Thank you very much for your valuable feedback.
Reinette
On Fri, Aug 02, 2019 at 01:11:13PM -0700, Reinette Chatre wrote:
> Will do. Do you prefer a new prepare patch that does the renaming before
> this patch or will you be ok with the renaming done within this patch?
Nah, within this patch is ok.
> This patch only makes it possible to determine whether cache is
> inclusive for some x86 platforms while all platforms of all
> architectures are given visibility into this new "inclusive" cache
> information field within the global "struct cacheinfo".
And this begs the question: do the other architectures even need that
thing exposed? As it is now, this is x86-only so I'm wondering whether
adding all that machinery to the generic struct cacheinfo is even needed
at this point.
TBH, I'd do it differently: read CPUID at init time and cache the
information whether the cache is inclusive locally and be done with it.
It is going to be a single system-wide bit anyway as I'd strongly assume
cache settings like inclusivity should be the same across the whole
system.
When the other arches do need it, we can extract that info "up" into the
generic layer.
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
Hi Borislav,
On 8/3/2019 2:44 AM, Borislav Petkov wrote:
> On Fri, Aug 02, 2019 at 01:11:13PM -0700, Reinette Chatre wrote:
>> This patch only makes it possible to determine whether cache is
>> inclusive for some x86 platforms while all platforms of all
>> architectures are given visibility into this new "inclusive" cache
>> information field within the global "struct cacheinfo".
>
> And this begs the question: do the other architectures even need that
> thing exposed? As it is now, this is x86-only so I'm wondering whether
> adding all that machinery to the generic struct cacheinfo is even needed
> at this point.
I do not know if more users would appear in the future but the goal of
this patch is to make this information available to resctrl. Since there
are a few ways to do so I really appreciate your guidance on how to do
this right. I'd be happy to make needed changes.
> TBH, I'd do it differently: read CPUID at init time and cache the
> information whether the cache is inclusive locally and be done with it.
> It is going to be a single system-wide bit anyway as I'd strongly assume
> cache settings like inclusivity should be the same across the whole
> system.
I've been digesting your comment and tried a few options but I have been
unable to come up with something that fulfill all your suggestions -
specifically the "single system-wide bit" one. These areas of code are
unfamiliar to me so I am not confident what I came up with for other
suggestions are the right way either.
So far it seems I can do the following:
Introduce a new bitfield into struct cpuinfo_x86. There is one existing
bitfield in this structure ("initialized") so we could add a new one
("x86_cache_l3_inclusive"?) just before it. With this information
included in this struct it becomes available via the global
boot_cpu_data, this seems to address the "system-wide bit" but this
struct is also maintained for all other CPUs on the system so may not be
what you had in mind (not a "single system-wide bit")?
If proceeding with inclusion into struct cpuinfo_x86 this new bitfield
can be initialized within init_intel_cacheinfo(). There are a few cache
properties within cpuinfo_x86 and they are initialized in a variety of
paths. init_intel_cacheinfo() is initialized via init_intel() and it
already has the needed CPUID information available making initialization
here straight forward. Alternatively there is also identify_cpu() that
also initializes many cache properties ... but would need some more code
to obtain needed values.
Finally, if the above is done, the resctrl code could just refer to this
new property directly as obtained from boot_cpu_data.x86_cache_l3_inclusive
What do you think?
>
> When the other arches do need it, we can extract that info "up" into the
> generic layer.
Reinette
Hi Borislav,
On 8/6/2019 8:57 AM, Borislav Petkov wrote:
> On Mon, Aug 05, 2019 at 10:57:04AM -0700, Reinette Chatre wrote:
>> What do you think?
>
> Actually, I was thinking about something a lot simpler: something
> along the lines of adding the CPUID check in a helper function which
> rdt_pseudo_lock_init() calls. If the cache is not inclusive - and my
> guess is it would suffice to check any cache but I'd prefer you correct
> me on that - you simply return error and rdt_pseudo_lock_init() returns
> early without doing any futher init.
>
> How does that sound?
I am a bit cautious about this. When I started this work I initially
added a helper function to resctrl that calls CPUID to determine if the
cache is inclusive. At that time I became aware of a discussion
motivating against scattered CPUID calls and motivating for one instance
of CPUID information:
http://lkml.kernel.org/r/[email protected]
My interpretation of the above resulted in a move away from calling
CPUID in resctrl to the patch you are reviewing now.
I do indeed prefer a simple approach and would change to what you
suggest if you find it to be best.
To answer your question about checking any cache: this seems to be
different between L2 and L3. On the Atom systems where L2 pseudo-locking
works well the L2 cache is not inclusive. We are also working on
supporting cache pseudo-locking on L3 cache that is not inclusive.
I could add the single CPUID check during rdt_pseudo_lock_init(),
checking on any CPU should work. I think it would be simpler (reasons
below) to initialize that single system-wide setting in
rdt_pseudo_lock_init() and keep the result locally in the pseudo-locking
code, that can be referred to when the user requests the pseudo-locked
region.
Simpler for two reasons:
* Prevent future platform specific code within rdt_pseudo_lock_init()
trying to pick when L3 is allowed to be inclusive or not.
* rdt_pseudo_lock_init() does not currently make decision whether
platform supports pseudo-locking - this is currently done when user
requests a pseudo-lock region. rdt_pseudo_lock_init() does
initialization of things that should not fail and for which resctrl's
mount should not proceed if it fails. A platform not supporting
pseudo-locking should not prevent resctrl from being mounted and used
for cache allocation.
Thank you very much
Reinette
On Tue, Aug 06, 2019 at 09:55:56AM -0700, Reinette Chatre wrote:
> I am a bit cautious about this. When I started this work I initially
> added a helper function to resctrl that calls CPUID to determine if the
> cache is inclusive. At that time I became aware of a discussion
> motivating against scattered CPUID calls and motivating for one instance
> of CPUID information:
> http://lkml.kernel.org/r/[email protected]
Ah, there's that. That's still somewhat a work/discussion in progress
thing. Let me discuss it with tglx.
> To answer your question about checking any cache: this seems to be
I meant the CPUID on any CPU and thus any cache - i.e., all L3s on the
system should be inclusive and identical in that respect. Can't work
otherwise, I'd strongly presume.
> different between L2 and L3. On the Atom systems where L2 pseudo-locking
> works well the L2 cache is not inclusive. We are also working on
> supporting cache pseudo-locking on L3 cache that is not inclusive.
Hmm, so why are you enforcing the inclusivity now:
+ if (p->r->cache_level == 3 &&
+ !get_cache_inclusive(plr->cpu, p->r->cache_level)) {
+ rdt_last_cmd_puts("L3 cache not inclusive\n");
but then will remove this requirement in the future? Why are we even
looking at cache inclusivity then and not make pseudo-locking work
regardless of that cache property?
Because if we're going to go and model this cache inclusivity property
properly in struct cpuinfo_x86 or struct cacheinfo or wherever, and do
that for all cache levels because apparently we're going to need that;
but then later it turns out we won't need it after all, why are we even
bothering?
Or am I missing some aspect?
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On Mon, Aug 05, 2019 at 10:57:04AM -0700, Reinette Chatre wrote:
> What do you think?
Actually, I was thinking about something a lot simpler: something
along the lines of adding the CPUID check in a helper function which
rdt_pseudo_lock_init() calls. If the cache is not inclusive - and my
guess is it would suffice to check any cache but I'd prefer you correct
me on that - you simply return error and rdt_pseudo_lock_init() returns
early without doing any futher init.
How does that sound?
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
Hi Borislav,
On 8/6/2019 10:33 AM, Borislav Petkov wrote:
> On Tue, Aug 06, 2019 at 09:55:56AM -0700, Reinette Chatre wrote:
>> I am a bit cautious about this. When I started this work I initially
>> added a helper function to resctrl that calls CPUID to determine if the
>> cache is inclusive. At that time I became aware of a discussion
>> motivating against scattered CPUID calls and motivating for one instance
>> of CPUID information:
>> http://lkml.kernel.org/r/[email protected]
>
> Ah, there's that. That's still somewhat a work/discussion in progress
> thing. Let me discuss it with tglx.
>
>> To answer your question about checking any cache: this seems to be
>
> I meant the CPUID on any CPU and thus any cache - i.e., all L3s on the
> system should be inclusive and identical in that respect. Can't work
> otherwise, I'd strongly presume.
This is my understanding, yes. While this patch supports knowing whether
each L3 is inclusive or not, I expect this information to be the same
for all L3 instances as will be supported by a single query in
rdt_pseudo_lock_init(). This definitely is the case on the platforms we
are enabling in this round.
>> different between L2 and L3. On the Atom systems where L2 pseudo-locking
>> works well the L2 cache is not inclusive. We are also working on
>> supporting cache pseudo-locking on L3 cache that is not inclusive.
>
> Hmm, so why are you enforcing the inclusivity now:
>
> + if (p->r->cache_level == 3 &&
> + !get_cache_inclusive(plr->cpu, p->r->cache_level)) {
> + rdt_last_cmd_puts("L3 cache not inclusive\n");
>
> but then will remove this requirement in the future? Why are we even
> looking at cache inclusivity then and not make pseudo-locking work
> regardless of that cache property?
Some platforms being enabled in this round have SKUs with inclusive
cache and also SKUs with non-inclusive cache. The non-inclusive cache
SKUs do not support cache pseudo-locking and cannot be made to support
cache pseudo-locking with software changes. Needing to know if cache is
inclusive or not will thus remain a requirement to distinguish between
these different SKUs. Supporting cache pseudo-locking on platforms with
non inclusive cache will require new hardware features.
> Because if we're going to go and model this cache inclusivity property
> properly in struct cpuinfo_x86 or struct cacheinfo or wherever, and do
> that for all cache levels because apparently we're going to need that;
> but then later it turns out we won't need it after all, why are we even
> bothering?
>
> Or am I missing some aspect?
Reinette
On Tue, Aug 06, 2019 at 11:13:22AM -0700, Reinette Chatre wrote:
> Some platforms being enabled in this round have SKUs with inclusive
> cache and also SKUs with non-inclusive cache. The non-inclusive cache
> SKUs do not support cache pseudo-locking and cannot be made to support
> cache pseudo-locking with software changes. Needing to know if cache is
> inclusive or not will thus remain a requirement to distinguish between
> these different SKUs. Supporting cache pseudo-locking on platforms with
> non inclusive cache will require new hardware features.
Is there another way/CPUID bit or whatever to tell us whether the
platform supports cache pseudo-locking or is the cache inclusivity the
only one?
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
Hi Borislav,
On 8/6/2019 11:33 AM, Borislav Petkov wrote:
> On Tue, Aug 06, 2019 at 11:13:22AM -0700, Reinette Chatre wrote:
>> Some platforms being enabled in this round have SKUs with inclusive
>> cache and also SKUs with non-inclusive cache. The non-inclusive cache
>> SKUs do not support cache pseudo-locking and cannot be made to support
>> cache pseudo-locking with software changes. Needing to know if cache is
>> inclusive or not will thus remain a requirement to distinguish between
>> these different SKUs. Supporting cache pseudo-locking on platforms with
>> non inclusive cache will require new hardware features.
>
> Is there another way/CPUID bit or whatever to tell us whether the
> platform supports cache pseudo-locking or is the cache inclusivity the
> only one?
Unfortunately there are no hardware bits that software can use to
determine if cache pseudo-locking is supported. The way software
currently determines if cache pseudo-locking is supported is through
initialization of the hardware prefetch disable bits. Cache
pseudo-locking requires the hardware prefetch bits to be disabled
(during the locking flow only), those cannot be discovered either and
thus software hardcodes the hardware prefetch disable bits only for
those platforms that support cache pseudo-locking.
What you will see in the code is this:
int rdtgroup_locksetup_enter(struct rdtgroup *rdtgrp)
{
...
prefetch_disable_bits = get_prefetch_disable_bits();
if (prefetch_disable_bits == 0) {
rdt_last_cmd_puts("Pseudo-locking not supported\n");
return -EINVAL;
}
...
}
In get_prefetch_disable_bits() the platforms that support cache
pseudo-locking are hardcoded as part of configuring the hardware
prefetch disable bits to use.
The current problem is that an upcoming platform has this difference
between SKUs so a single platform-wide decision is not sufficient.
Reinette
On Tue, Aug 06, 2019 at 11:53:40AM -0700, Reinette Chatre wrote:
> In get_prefetch_disable_bits() the platforms that support cache
> pseudo-locking are hardcoded as part of configuring the hardware
> prefetch disable bits to use.
Ok, so there is already a way to check pseudo-locking support. Now, why
do we have to look at cache inclusivity too?
Your 0/10 mail says:
"Only systems with L3 inclusive cache is supported at this time because
if the L3 cache is not inclusive then pseudo-locked memory within the L3
cache would be evicted when migrated to L2."
but then a couple of mails earlier you said:
"... this seems to be different between L2 and L3. On the Atom systems
where L2 pseudo-locking works well the L2 cache is not inclusive. We are
also working on supporting cache pseudo-locking on L3 cache that is not
inclusive."
which leads me to still think that we don't really need L3 cache
inclusivity and theoretically, you could do without it.
Or are you saying that cache pseudo-locking on non-inclusive L3 is not
supported yet so no need to enable it yet?
Hmmm?
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
Hi Borislav,
On 8/6/2019 12:16 PM, Borislav Petkov wrote:
> On Tue, Aug 06, 2019 at 11:53:40AM -0700, Reinette Chatre wrote:
>> In get_prefetch_disable_bits() the platforms that support cache
>> pseudo-locking are hardcoded as part of configuring the hardware
>> prefetch disable bits to use.
>
> Ok, so there is already a way to check pseudo-locking support.
Correct. This check is per platform though.
> Now, why
> do we have to look at cache inclusivity too?
... because some platforms differ in which SKUs support cache
pseudo-locking. On these platforms only the SKUs with inclusive cache
support cache pseudo-locking, thus the additional check.
>
> Your 0/10 mail says:
>
> "Only systems with L3 inclusive cache is supported at this time because
> if the L3 cache is not inclusive then pseudo-locked memory within the L3
> cache would be evicted when migrated to L2."
>
> but then a couple of mails earlier you said:
>
> "... this seems to be different between L2 and L3. On the Atom systems
> where L2 pseudo-locking works well the L2 cache is not inclusive. We are
> also working on supporting cache pseudo-locking on L3 cache that is not
> inclusive."
>
> which leads me to still think that we don't really need L3 cache
> inclusivity and theoretically, you could do without it.
>
> Or are you saying that cache pseudo-locking on non-inclusive L3 is not
> supported yet so no need to enable it yet?
Correct. Hardware and software changes will be needed to support this.
Reinette
On Tue, Aug 06, 2019 at 01:22:22PM -0700, Reinette Chatre wrote:
> ... because some platforms differ in which SKUs support cache
> pseudo-locking. On these platforms only the SKUs with inclusive cache
> support cache pseudo-locking, thus the additional check.
Ok, so it sounds to me like that check in get_prefetch_disable_bits()
should be extended (and maybe renamed) to check for cache inclusivity
too, in order to know which platforms support cache pseudo-locking.
I'd leave it to tglx to say how we should mirror cache inclusivity in
cpuinfo_x86: whether a synthetic X86_FEATURE bit or cache the respective
CPUID words which state whether L2/L3 is inclusive...
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
Hi Borislav,
On 8/6/2019 1:40 PM, Borislav Petkov wrote:
> On Tue, Aug 06, 2019 at 01:22:22PM -0700, Reinette Chatre wrote:
>> ... because some platforms differ in which SKUs support cache
>> pseudo-locking. On these platforms only the SKUs with inclusive cache
>> support cache pseudo-locking, thus the additional check.
>
> Ok, so it sounds to me like that check in get_prefetch_disable_bits()
> should be extended (and maybe renamed) to check for cache inclusivity
> too, in order to know which platforms support cache pseudo-locking.
Indeed. As you pointed out this would be same system-wide and the check
thus need not be delayed until it is known which cache is being
pseudo-locked.
> I'd leave it to tglx to say how we should mirror cache inclusivity in
> cpuinfo_x86: whether a synthetic X86_FEATURE bit or cache the respective
> CPUID words which state whether L2/L3 is inclusive...
Thank you very much. I appreciate your guidance here.
Reinette
On Tue, Aug 06, 2019 at 02:16:10PM -0700, Reinette Chatre wrote:
> > I'd leave it to tglx to say how we should mirror cache inclusivity in
> > cpuinfo_x86: whether a synthetic X86_FEATURE bit or cache the respective
> > CPUID words which state whether L2/L3 is inclusive...
>
> Thank you very much. I appreciate your guidance here.
Ok, tglx and I talked it over a bit on IRC: so your 1/10 patch is pretty
close - just leave out the generic struct cacheinfo bits and put the
cache inclusivity property in a static variable there. It will be a
single bit of information only anyway, as this is system-wide and we can
always move it to generic code when some other arch wants it too.
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On Thu, Aug 08, 2019 at 10:08:41AM +0200, Borislav Petkov wrote:
> Ok, tglx and I talked it over a bit on IRC: so your 1/10 patch is pretty
> close - just leave out the generic struct cacheinfo bits and put the
> cache inclusivity property in a static variable there.
... and by "there" I mean arch/x86/kernel/cpu/cacheinfo.c which contains
all cache properties etc on x86 and is the proper place to put stuff
like that.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
Hi Borislav,
On 8/8/2019 1:13 AM, Borislav Petkov wrote:
> On Thu, Aug 08, 2019 at 10:08:41AM +0200, Borislav Petkov wrote:
>> Ok, tglx and I talked it over a bit on IRC: so your 1/10 patch is pretty
>> close - just leave out the generic struct cacheinfo bits and put the
>> cache inclusivity property in a static variable there.
>
> ... and by "there" I mean arch/x86/kernel/cpu/cacheinfo.c which contains
> all cache properties etc on x86 and is the proper place to put stuff
> like that.
With the goal of following these guidelines exactly I came up with the
below that is an incremental diff on top of what this review started out as.
Some changes to highlight that may be of concern:
* In your previous email you do mention that this will be a "single bit
of information". Please note that I did not specifically use an actual
bit to capture this information but an unsigned int (I am very aware
that you also commented on this initially). If you do mean that this
should be stored as an actual bit, could you please help me by
elaborating how you would like to see this implemented?
* Please note that I moved the initialization to init_intel_cacheinfo()
to be specific to Intel. I did so because from what I understand there
are some AMD platforms for which this information cannot be determined
and I thought it simpler to make it specific to Intel with the new
single static variable.
* Please note that while this is a single global static variable it will
be set over and over for each CPU on the system.
diff --git a/arch/x86/include/asm/cacheinfo.h
b/arch/x86/include/asm/cacheinfo.h
index 86b63c7feab7..97be5141bb4b 100644
--- a/arch/x86/include/asm/cacheinfo.h
+++ b/arch/x86/include/asm/cacheinfo.h
@@ -5,4 +5,6 @@
void cacheinfo_amd_init_llc_id(struct cpuinfo_x86 *c, int cpu, u8 node_id);
void cacheinfo_hygon_init_llc_id(struct cpuinfo_x86 *c, int cpu, u8
node_id);
+unsigned int cacheinfo_intel_l3_inclusive(void);
+
#endif /* _ASM_X86_CACHEINFO_H */
diff --git a/arch/x86/kernel/cpu/cacheinfo.c
b/arch/x86/kernel/cpu/cacheinfo.c
index 733874f84f41..247b6a9b5c88 100644
--- a/arch/x86/kernel/cpu/cacheinfo.c
+++ b/arch/x86/kernel/cpu/cacheinfo.c
@@ -187,6 +187,7 @@ struct _cpuid4_info_regs {
};
static unsigned short num_cache_leaves;
+static unsigned l3_inclusive;
/* AMD doesn't have CPUID4. Emulate it here to report the same
information to the user. This makes some assumptions about the machine:
@@ -745,6 +746,11 @@ void init_hygon_cacheinfo(struct cpuinfo_x86 *c)
num_cache_leaves = find_num_cache_leaves(c);
}
+unsigned int cacheinfo_intel_l3_inclusive(void)
+{
+ return l3_inclusive;
+}
+
void init_intel_cacheinfo(struct cpuinfo_x86 *c)
{
/* Cache sizes */
@@ -795,6 +801,7 @@ void init_intel_cacheinfo(struct cpuinfo_x86 *c)
num_threads_sharing = 1 +
this_leaf.eax.split.num_threads_sharing;
index_msb =
get_count_order(num_threads_sharing);
l3_id = c->apicid & ~((1 << index_msb) - 1);
+ l3_inclusive =
this_leaf.edx.split.inclusive;
break;
default:
break;
@@ -1010,13 +1017,6 @@ static void ci_leaf_init(struct cacheinfo *this_leaf,
this_leaf->physical_line_partition =
base->ebx.split.physical_line_partition + 1;
- if ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
- boot_cpu_has(X86_FEATURE_TOPOEXT)) ||
- boot_cpu_data.x86_vendor == X86_VENDOR_HYGON ||
- boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) {
- this_leaf->attributes |= CACHE_INCLUSIVE_SET;
- this_leaf->inclusive = base->edx.split.inclusive;
- }
this_leaf->priv = base->nb;
}
What do you think?
Reinette
On Thu, Aug 08, 2019 at 01:08:59PM -0700, Reinette Chatre wrote:
> With the goal of following these guidelines exactly I came up with the
> below that is an incremental diff on top of what this review started out as.
Thanks but pls in the future, do not use windoze to send a diff - it
mangles it to inapplicability.
> Some changes to highlight that may be of concern:
> * In your previous email you do mention that this will be a "single bit
> of information". Please note that I did not specifically use an actual
> bit to capture this information but an unsigned int (I am very aware
> that you also commented on this initially). If you do mean that this
> should be stored as an actual bit, could you please help me by
> elaborating how you would like to see this implemented?
See below for a possible way to do it.
> * Please note that I moved the initialization to init_intel_cacheinfo()
> to be specific to Intel. I did so because from what I understand there
> are some AMD platforms for which this information cannot be determined
> and I thought it simpler to make it specific to Intel with the new
> single static variable.
Yeah, I renamed your function to cacheinfo_l3_inclusive() in case the
other vendors would want to use it someday.
> * Please note that while this is a single global static variable it will
> be set over and over for each CPU on the system.
That's fine.
Also, the bits in include/linux/cacheinfo.h need to go too. Here's a diff ontop
of your patchset:
---
diff --git a/arch/x86/include/asm/cacheinfo.h b/arch/x86/include/asm/cacheinfo.h
index 86b63c7feab7..87eca716e03d 100644
--- a/arch/x86/include/asm/cacheinfo.h
+++ b/arch/x86/include/asm/cacheinfo.h
@@ -5,4 +5,6 @@
void cacheinfo_amd_init_llc_id(struct cpuinfo_x86 *c, int cpu, u8 node_id);
void cacheinfo_hygon_init_llc_id(struct cpuinfo_x86 *c, int cpu, u8 node_id);
+bool cacheinfo_l3_inclusive(void);
+
#endif /* _ASM_X86_CACHEINFO_H */
diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
index 3b678f46be53..418a6f7392d0 100644
--- a/arch/x86/kernel/cpu/cacheinfo.c
+++ b/arch/x86/kernel/cpu/cacheinfo.c
@@ -188,6 +188,13 @@ struct _cpuid4_info_regs {
static unsigned short num_cache_leaves;
+struct cache_attributes {
+ u64 l3_inclusive : 1,
+ __resv : 63;
+};
+
+static struct cache_attributes cache_attrs;
+
/* AMD doesn't have CPUID4. Emulate it here to report the same
information to the user. This makes some assumptions about the machine:
L2 not shared, no SMT etc. that is currently true on AMD CPUs.
@@ -745,6 +752,14 @@ void init_hygon_cacheinfo(struct cpuinfo_x86 *c)
num_cache_leaves = find_num_cache_leaves(c);
}
+bool cacheinfo_l3_inclusive(void)
+{
+ if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
+ return false;
+
+ return cache_attrs.l3_inclusive;
+}
+
void init_intel_cacheinfo(struct cpuinfo_x86 *c)
{
/* Cache sizes */
@@ -795,6 +810,7 @@ void init_intel_cacheinfo(struct cpuinfo_x86 *c)
num_threads_sharing = 1 + this_leaf.eax.split.num_threads_sharing;
index_msb = get_count_order(num_threads_sharing);
l3_id = c->apicid & ~((1 << index_msb) - 1);
+ cache_attrs.l3_inclusive = this_leaf.edx.split.inclusive;
break;
default:
break;
@@ -1009,13 +1025,6 @@ static void ci_leaf_init(struct cacheinfo *this_leaf,
this_leaf->number_of_sets = base->ecx.split.number_of_sets + 1;
this_leaf->physical_line_partition =
base->ebx.split.physical_line_partition + 1;
- if ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
- boot_cpu_has(X86_FEATURE_TOPOEXT)) ||
- boot_cpu_data.x86_vendor == X86_VENDOR_HYGON ||
- boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) {
- this_leaf->attributes |= CACHE_INCLUSIVE_SET;
- this_leaf->inclusive = base->edx.split.inclusive;
- }
this_leaf->priv = base->nb;
}
diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
index b4fff88572bd..644d1780671e 100644
--- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
+++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
@@ -26,6 +26,7 @@
#include <asm/intel-family.h>
#include <asm/resctrl_sched.h>
#include <asm/perf_event.h>
+#include <asm/cacheinfo.h>
#include "../../events/perf_event.h" /* For X86_CONFIG() */
#include "internal.h"
@@ -125,30 +126,6 @@ static unsigned int get_cache_line_size(unsigned int cpu, int level)
return 0;
}
-/**
- * get_cache_inclusive - Determine if cache is inclusive of lower levels
- * @cpu: CPU with which cache is associated
- * @level: Cache level
- *
- * Context: @cpu has to be online.
- * Return: 1 if cache is inclusive of lower cache levels, 0 if cache is not
- * inclusive of lower cache levels or on failure.
- */
-static unsigned int get_cache_inclusive(unsigned int cpu, int level)
-{
- struct cpu_cacheinfo *ci;
- int i;
-
- ci = get_cpu_cacheinfo(cpu);
-
- for (i = 0; i < ci->num_leaves; i++) {
- if (ci->info_list[i].level == level)
- return ci->info_list[i].inclusive;
- }
-
- return 0;
-}
-
/**
* pseudo_lock_minor_get - Obtain available minor number
* @minor: Pointer to where new minor number will be stored
@@ -341,8 +318,7 @@ static int pseudo_lock_single_portion_valid(struct pseudo_lock_region *plr,
goto err_cpu;
}
- if (p->r->cache_level == 3 &&
- !get_cache_inclusive(plr->cpu, p->r->cache_level)) {
+ if (p->r->cache_level == 3 && !cacheinfo_l3_inclusive()) {
rdt_last_cmd_puts("L3 cache not inclusive\n");
goto err_cpu;
}
@@ -448,7 +424,7 @@ static int pseudo_lock_l2_l3_portions_valid(struct pseudo_lock_region *plr,
goto err_cpu;
}
- if (!get_cache_inclusive(plr->cpu, l3_p->r->cache_level)) {
+ if (!cacheinfo_l3_inclusive()) {
rdt_last_cmd_puts("L3 cache not inclusive\n");
goto err_cpu;
}
diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
index cdc7a9d6923f..46b92cd61d0c 100644
--- a/include/linux/cacheinfo.h
+++ b/include/linux/cacheinfo.h
@@ -33,8 +33,6 @@ extern unsigned int coherency_max_size;
* @physical_line_partition: number of physical cache lines sharing the
* same cachetag
* @size: Total size of the cache
- * @inclusive: Cache is inclusive of lower level caches. Only valid if
- * CACHE_INCLUSIVE_SET attribute is set.
* @shared_cpu_map: logical cpumask representing all the cpus sharing
* this cache node
* @attributes: bitfield representing various cache attributes
@@ -57,7 +55,6 @@ struct cacheinfo {
unsigned int ways_of_associativity;
unsigned int physical_line_partition;
unsigned int size;
- unsigned int inclusive;
cpumask_t shared_cpu_map;
unsigned int attributes;
#define CACHE_WRITE_THROUGH BIT(0)
@@ -69,7 +66,6 @@ struct cacheinfo {
#define CACHE_ALLOCATE_POLICY_MASK \
(CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE)
#define CACHE_ID BIT(4)
-#define CACHE_INCLUSIVE_SET BIT(5)
void *fw_token;
bool disable_sysfs;
void *priv;
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
Hi Borislav,
On 8/9/2019 12:33 AM, Borislav Petkov wrote:
> On Thu, Aug 08, 2019 at 01:08:59PM -0700, Reinette Chatre wrote:
>> With the goal of following these guidelines exactly I came up with the
>> below that is an incremental diff on top of what this review started out as.
>
> Thanks but pls in the future, do not use windoze to send a diff - it
> mangles it to inapplicability.
I am sorry about that.
>
>> Some changes to highlight that may be of concern:
>> * In your previous email you do mention that this will be a "single bit
>> of information". Please note that I did not specifically use an actual
>> bit to capture this information but an unsigned int (I am very aware
>> that you also commented on this initially). If you do mean that this
>> should be stored as an actual bit, could you please help me by
>> elaborating how you would like to see this implemented?
>
> See below for a possible way to do it.
>
>> * Please note that I moved the initialization to init_intel_cacheinfo()
>> to be specific to Intel. I did so because from what I understand there
>> are some AMD platforms for which this information cannot be determined
>> and I thought it simpler to make it specific to Intel with the new
>> single static variable.
>
> Yeah, I renamed your function to cacheinfo_l3_inclusive() in case the
> other vendors would want to use it someday.
>
>> * Please note that while this is a single global static variable it will
>> be set over and over for each CPU on the system.
>
> That's fine.
>
> Also, the bits in include/linux/cacheinfo.h need to go too. Here's a diff ontop
> of your patchset:
>
> ---
> diff --git a/arch/x86/include/asm/cacheinfo.h b/arch/x86/include/asm/cacheinfo.h
> index 86b63c7feab7..87eca716e03d 100644
> --- a/arch/x86/include/asm/cacheinfo.h
> +++ b/arch/x86/include/asm/cacheinfo.h
> @@ -5,4 +5,6 @@
> void cacheinfo_amd_init_llc_id(struct cpuinfo_x86 *c, int cpu, u8 node_id);
> void cacheinfo_hygon_init_llc_id(struct cpuinfo_x86 *c, int cpu, u8 node_id);
>
> +bool cacheinfo_l3_inclusive(void);
> +
> #endif /* _ASM_X86_CACHEINFO_H */
> diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
> index 3b678f46be53..418a6f7392d0 100644
> --- a/arch/x86/kernel/cpu/cacheinfo.c
> +++ b/arch/x86/kernel/cpu/cacheinfo.c
> @@ -188,6 +188,13 @@ struct _cpuid4_info_regs {
>
> static unsigned short num_cache_leaves;
>
> +struct cache_attributes {
> + u64 l3_inclusive : 1,
> + __resv : 63;
> +};
> +
> +static struct cache_attributes cache_attrs;
> +
> /* AMD doesn't have CPUID4. Emulate it here to report the same
> information to the user. This makes some assumptions about the machine:
> L2 not shared, no SMT etc. that is currently true on AMD CPUs.
> @@ -745,6 +752,14 @@ void init_hygon_cacheinfo(struct cpuinfo_x86 *c)
> num_cache_leaves = find_num_cache_leaves(c);
> }
>
> +bool cacheinfo_l3_inclusive(void)
> +{
> + if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
> + return false;
> +
> + return cache_attrs.l3_inclusive;
> +}
> +
> void init_intel_cacheinfo(struct cpuinfo_x86 *c)
> {
> /* Cache sizes */
> @@ -795,6 +810,7 @@ void init_intel_cacheinfo(struct cpuinfo_x86 *c)
> num_threads_sharing = 1 + this_leaf.eax.split.num_threads_sharing;
> index_msb = get_count_order(num_threads_sharing);
> l3_id = c->apicid & ~((1 << index_msb) - 1);
> + cache_attrs.l3_inclusive = this_leaf.edx.split.inclusive;
> break;
> default:
> break;
> @@ -1009,13 +1025,6 @@ static void ci_leaf_init(struct cacheinfo *this_leaf,
> this_leaf->number_of_sets = base->ecx.split.number_of_sets + 1;
> this_leaf->physical_line_partition =
> base->ebx.split.physical_line_partition + 1;
> - if ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
> - boot_cpu_has(X86_FEATURE_TOPOEXT)) ||
> - boot_cpu_data.x86_vendor == X86_VENDOR_HYGON ||
> - boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) {
> - this_leaf->attributes |= CACHE_INCLUSIVE_SET;
> - this_leaf->inclusive = base->edx.split.inclusive;
> - }
> this_leaf->priv = base->nb;
> }
>
> diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> index b4fff88572bd..644d1780671e 100644
> --- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> +++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> @@ -26,6 +26,7 @@
> #include <asm/intel-family.h>
> #include <asm/resctrl_sched.h>
> #include <asm/perf_event.h>
> +#include <asm/cacheinfo.h>
>
> #include "../../events/perf_event.h" /* For X86_CONFIG() */
> #include "internal.h"
> @@ -125,30 +126,6 @@ static unsigned int get_cache_line_size(unsigned int cpu, int level)
> return 0;
> }
>
> -/**
> - * get_cache_inclusive - Determine if cache is inclusive of lower levels
> - * @cpu: CPU with which cache is associated
> - * @level: Cache level
> - *
> - * Context: @cpu has to be online.
> - * Return: 1 if cache is inclusive of lower cache levels, 0 if cache is not
> - * inclusive of lower cache levels or on failure.
> - */
> -static unsigned int get_cache_inclusive(unsigned int cpu, int level)
> -{
> - struct cpu_cacheinfo *ci;
> - int i;
> -
> - ci = get_cpu_cacheinfo(cpu);
> -
> - for (i = 0; i < ci->num_leaves; i++) {
> - if (ci->info_list[i].level == level)
> - return ci->info_list[i].inclusive;
> - }
> -
> - return 0;
> -}
> -
> /**
> * pseudo_lock_minor_get - Obtain available minor number
> * @minor: Pointer to where new minor number will be stored
> @@ -341,8 +318,7 @@ static int pseudo_lock_single_portion_valid(struct pseudo_lock_region *plr,
> goto err_cpu;
> }
>
> - if (p->r->cache_level == 3 &&
> - !get_cache_inclusive(plr->cpu, p->r->cache_level)) {
> + if (p->r->cache_level == 3 && !cacheinfo_l3_inclusive()) {
> rdt_last_cmd_puts("L3 cache not inclusive\n");
> goto err_cpu;
> }
> @@ -448,7 +424,7 @@ static int pseudo_lock_l2_l3_portions_valid(struct pseudo_lock_region *plr,
> goto err_cpu;
> }
>
> - if (!get_cache_inclusive(plr->cpu, l3_p->r->cache_level)) {
> + if (!cacheinfo_l3_inclusive()) {
> rdt_last_cmd_puts("L3 cache not inclusive\n");
> goto err_cpu;
> }
> diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
> index cdc7a9d6923f..46b92cd61d0c 100644
> --- a/include/linux/cacheinfo.h
> +++ b/include/linux/cacheinfo.h
> @@ -33,8 +33,6 @@ extern unsigned int coherency_max_size;
> * @physical_line_partition: number of physical cache lines sharing the
> * same cachetag
> * @size: Total size of the cache
> - * @inclusive: Cache is inclusive of lower level caches. Only valid if
> - * CACHE_INCLUSIVE_SET attribute is set.
> * @shared_cpu_map: logical cpumask representing all the cpus sharing
> * this cache node
> * @attributes: bitfield representing various cache attributes
> @@ -57,7 +55,6 @@ struct cacheinfo {
> unsigned int ways_of_associativity;
> unsigned int physical_line_partition;
> unsigned int size;
> - unsigned int inclusive;
> cpumask_t shared_cpu_map;
> unsigned int attributes;
> #define CACHE_WRITE_THROUGH BIT(0)
> @@ -69,7 +66,6 @@ struct cacheinfo {
> #define CACHE_ALLOCATE_POLICY_MASK \
> (CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE)
> #define CACHE_ID BIT(4)
> -#define CACHE_INCLUSIVE_SET BIT(5)
> void *fw_token;
> bool disable_sysfs;
> void *priv;
>
>
I really appreciate that you took the time to put this together. I now
understand how all your suggestions come together.
Thank you for your patience in reviewing this change.
Reinette