[PATCH v1 20/31] x86/resctrl: Allow an architecture to disable pseudo lock

James Morse posted 31 patches 1 year, 10 months ago
There is a newer version of this series
[PATCH v1 20/31] x86/resctrl: Allow an architecture to disable pseudo lock
Posted by James Morse 1 year, 10 months ago
Pseudo-lock relies on knowledge of the micro-architecture to disable
prefetchers etc.

On arm64 these controls are typically secure only, meaning linux can't
access them. Arm's cache-lockdown feature works in a very different
way. Resctrl's pseudo-lock isn't going to be used on arm64 platforms.

Add a Kconfig symbol that can be selected by the architecture. This
enables or disables building of the psuedo_lock.c file, and replaces
the functions with stubs. An additional IS_ENABLED() check is needed
in rdtgroup_mode_write() so that attempting to enable pseudo-lock
reports an "Unknown or unsupported mode" to user-space.

Signed-off-by: James Morse <james.morse@arm.com>
---
 arch/x86/Kconfig                       |  7 ++++
 arch/x86/kernel/cpu/resctrl/Makefile   |  5 +--
 arch/x86/kernel/cpu/resctrl/internal.h | 48 +++++++++++++++++++++-----
 arch/x86/kernel/cpu/resctrl/rdtgroup.c |  3 +-
 4 files changed, 52 insertions(+), 11 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 7aed87cbf386..e071e564452e 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -481,6 +481,7 @@ config X86_CPU_RESCTRL
 	depends on X86 && (CPU_SUP_INTEL || CPU_SUP_AMD)
 	select KERNFS
 	select PROC_CPU_RESCTRL		if PROC_FS
+	select RESCTRL_FS_PSEUDO_LOCK
 	help
 	  Enable x86 CPU resource control support.
 
@@ -506,6 +507,12 @@ config X86_FRED
 	  ring transitions and exception/interrupt handling if the
 	  system supports.
 
+config RESCTRL_FS_PSEUDO_LOCK
+	bool
+	help
+	  Software mechanism to pin data in a cache portion using
+	  micro-architecture specific knowledge.
+
 if X86_32
 config X86_BIGSMP
 	bool "Support for big SMP systems with more than 8 CPUs"
diff --git a/arch/x86/kernel/cpu/resctrl/Makefile b/arch/x86/kernel/cpu/resctrl/Makefile
index 4a06c37b9cf1..0c13b0befd8a 100644
--- a/arch/x86/kernel/cpu/resctrl/Makefile
+++ b/arch/x86/kernel/cpu/resctrl/Makefile
@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
-obj-$(CONFIG_X86_CPU_RESCTRL)	+= core.o rdtgroup.o monitor.o
-obj-$(CONFIG_X86_CPU_RESCTRL)	+= ctrlmondata.o pseudo_lock.o
+obj-$(CONFIG_X86_CPU_RESCTRL)		+= core.o rdtgroup.o monitor.o
+obj-$(CONFIG_X86_CPU_RESCTRL)		+= ctrlmondata.o
+obj-$(CONFIG_RESCTRL_FS_PSEUDO_LOCK)	+= pseudo_lock.o
 CFLAGS_pseudo_lock.o = -I$(src)
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 238b81d3f64a..d6db15839dc4 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -489,14 +489,6 @@ unsigned int rdtgroup_cbm_to_size(struct rdt_resource *r, struct rdt_domain *d,
 				  unsigned long cbm);
 enum rdtgrp_mode rdtgroup_mode_by_closid(int closid);
 int rdtgroup_tasks_assigned(struct rdtgroup *r);
-int rdtgroup_locksetup_enter(struct rdtgroup *rdtgrp);
-int rdtgroup_locksetup_exit(struct rdtgroup *rdtgrp);
-bool rdtgroup_cbm_overlaps_pseudo_locked(struct rdt_domain *d, unsigned long cbm);
-bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d);
-int rdt_pseudo_lock_init(void);
-void rdt_pseudo_lock_release(void);
-int rdtgroup_pseudo_lock_create(struct rdtgroup *rdtgrp);
-void rdtgroup_pseudo_lock_remove(struct rdtgroup *rdtgrp);
 struct rdt_domain *get_domain_from_cpu(int cpu, struct rdt_resource *r);
 int closids_supported(void);
 void closid_free(int closid);
@@ -529,4 +521,44 @@ void rdt_staged_configs_clear(void);
 bool closid_allocated(unsigned int closid);
 int resctrl_find_cleanest_closid(void);
 
+#ifdef CONFIG_RESCTRL_FS_PSEUDO_LOCK
+int rdtgroup_locksetup_enter(struct rdtgroup *rdtgrp);
+int rdtgroup_locksetup_exit(struct rdtgroup *rdtgrp);
+bool rdtgroup_cbm_overlaps_pseudo_locked(struct rdt_domain *d, unsigned long cbm);
+bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d);
+int rdt_pseudo_lock_init(void);
+void rdt_pseudo_lock_release(void);
+int rdtgroup_pseudo_lock_create(struct rdtgroup *rdtgrp);
+void rdtgroup_pseudo_lock_remove(struct rdtgroup *rdtgrp);
+#else
+static inline int rdtgroup_locksetup_enter(struct rdtgroup *rdtgrp)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline int rdtgroup_locksetup_exit(struct rdtgroup *rdtgrp)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline bool rdtgroup_cbm_overlaps_pseudo_locked(struct rdt_domain *d, unsigned long cbm)
+{
+	return false;
+}
+
+static inline bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d)
+{
+	return false;
+}
+
+static inline int rdt_pseudo_lock_init(void) { return 0; }
+static inline void rdt_pseudo_lock_release(void) { }
+static inline int rdtgroup_pseudo_lock_create(struct rdtgroup *rdtgrp)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline void rdtgroup_pseudo_lock_remove(struct rdtgroup *rdtgrp) { }
+#endif /* CONFIG_RESCTRL_FS_PSEUDO_LOCK */
+
 #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 56a0bfdc11f7..9275d6f8a74e 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1452,7 +1452,8 @@ static ssize_t rdtgroup_mode_write(struct kernfs_open_file *of,
 				goto out;
 		}
 		rdtgrp->mode = RDT_MODE_EXCLUSIVE;
-	} else if (!strcmp(buf, "pseudo-locksetup")) {
+	} else if (IS_ENABLED(CONFIG_RESCTRL_FS_PSEUDO_LOCK) &&
+		   !strcmp(buf, "pseudo-locksetup")) {
 		ret = rdtgroup_locksetup_enter(rdtgrp);
 		if (ret)
 			goto out;
-- 
2.39.2
Re: [PATCH v1 20/31] x86/resctrl: Allow an architecture to disable pseudo lock
Posted by Reinette Chatre 1 year, 10 months ago
Hi James,

On 3/21/2024 9:50 AM, James Morse wrote:
> Pseudo-lock relies on knowledge of the micro-architecture to disable
> prefetchers etc.
> 
> On arm64 these controls are typically secure only, meaning linux can't
> access them. Arm's cache-lockdown feature works in a very different
> way. Resctrl's pseudo-lock isn't going to be used on arm64 platforms.
> 
> Add a Kconfig symbol that can be selected by the architecture. This
> enables or disables building of the psuedo_lock.c file, and replaces

pseudo_lock.c

> the functions with stubs. An additional IS_ENABLED() check is needed
> in rdtgroup_mode_write() so that attempting to enable pseudo-lock
> reports an "Unknown or unsupported mode" to user-space.
> 

I am missing something here. It is not obvious to me why the IS_ENABLED()
check is needed. Wouldn't rdtgroup_locksetup_enter()
return -EOPNOTSUPP if CONFIG_RESCTRL_FS_PSEUDO_LOCK is not enabled?

Reinette
Re: [PATCH v1 20/31] x86/resctrl: Allow an architecture to disable pseudo lock
Posted by Dave Martin 1 year, 10 months ago
On Mon, Apr 08, 2024 at 08:24:12PM -0700, Reinette Chatre wrote:
> Hi James,
> 
> On 3/21/2024 9:50 AM, James Morse wrote:
> > Pseudo-lock relies on knowledge of the micro-architecture to disable
> > prefetchers etc.
> > 
> > On arm64 these controls are typically secure only, meaning linux can't
> > access them. Arm's cache-lockdown feature works in a very different
> > way. Resctrl's pseudo-lock isn't going to be used on arm64 platforms.
> > 
> > Add a Kconfig symbol that can be selected by the architecture. This
> > enables or disables building of the psuedo_lock.c file, and replaces
> 
> pseudo_lock.c

Noted.

> > the functions with stubs. An additional IS_ENABLED() check is needed
> > in rdtgroup_mode_write() so that attempting to enable pseudo-lock
> > reports an "Unknown or unsupported mode" to user-space.
> > 
> 
> I am missing something here. It is not obvious to me why the IS_ENABLED()
> check is needed. Wouldn't rdtgroup_locksetup_enter()
> return -EOPNOTSUPP if CONFIG_RESCTRL_FS_PSEUDO_LOCK is not enabled?
> 
> Reinette
> 

Hmm, if I've understood all this correctly, then it looks like the
existing code in rdtgroup_mode_write() relies on the dispatched
function (rdtgroup_locksetup_enter() etc.) to do an appropriate
rdt_last_cmd_puts() on failure.  If no function is called at all and
the requested mode change is not a no-op or otherwise trivially
successful, then it looks like we're supposed to fall into the else
clause.

I'd guess James' intent here was to use the fallback else {} to write
a suitable status string, while keeping the stub functions as trivial
as possible.

Just taking the IS_ENABLED() away would result in error return from the
write(), but no suitable last_cmd_status string.

For consistency with the existing x86 implementation, I wonder whether
we should put a suitable rdt_last_cmd_puts() in the stub for
rdtgroup_locksetup_enter().

There might be other ways to refactor or simplify this, though.

Thoughts?

Cheers
---Dave
Re: [PATCH v1 20/31] x86/resctrl: Allow an architecture to disable pseudo lock
Posted by Reinette Chatre 1 year, 10 months ago
Hi Dave,

On 4/11/2024 7:17 AM, Dave Martin wrote:
> On Mon, Apr 08, 2024 at 08:24:12PM -0700, Reinette Chatre wrote:
>> Hi James,
>>
>> On 3/21/2024 9:50 AM, James Morse wrote:
>>> Pseudo-lock relies on knowledge of the micro-architecture to disable
>>> prefetchers etc.
>>>
>>> On arm64 these controls are typically secure only, meaning linux can't
>>> access them. Arm's cache-lockdown feature works in a very different
>>> way. Resctrl's pseudo-lock isn't going to be used on arm64 platforms.
>>>
>>> Add a Kconfig symbol that can be selected by the architecture. This
>>> enables or disables building of the psuedo_lock.c file, and replaces
>>
>> pseudo_lock.c
> 
> Noted.
> 
>>> the functions with stubs. An additional IS_ENABLED() check is needed
>>> in rdtgroup_mode_write() so that attempting to enable pseudo-lock
>>> reports an "Unknown or unsupported mode" to user-space.
>>>
>>
>> I am missing something here. It is not obvious to me why the IS_ENABLED()
>> check is needed. Wouldn't rdtgroup_locksetup_enter()
>> return -EOPNOTSUPP if CONFIG_RESCTRL_FS_PSEUDO_LOCK is not enabled?
>>
>> Reinette
>>
> 
> Hmm, if I've understood all this correctly, then it looks like the
> existing code in rdtgroup_mode_write() relies on the dispatched
> function (rdtgroup_locksetup_enter() etc.) to do an appropriate
> rdt_last_cmd_puts() on failure.  If no function is called at all and
> the requested mode change is not a no-op or otherwise trivially
> successful, then it looks like we're supposed to fall into the else
> clause.
> 
> I'd guess James' intent here was to use the fallback else {} to write
> a suitable status string, while keeping the stub functions as trivial
> as possible.
> 
> Just taking the IS_ENABLED() away would result in error return from the
> write(), but no suitable last_cmd_status string.
> 
> For consistency with the existing x86 implementation, I wonder whether
> we should put a suitable rdt_last_cmd_puts() in the stub for
> rdtgroup_locksetup_enter().
> 
> There might be other ways to refactor or simplify this, though.
> 
> Thoughts?

Thank you for digging into this. It was not obvious to me that
the changelog referred to the last_cmd_status string. I do
not think this warrants making the stubs more complicated.

Reinette
Re: [PATCH v1 20/31] x86/resctrl: Allow an architecture to disable pseudo lock
Posted by Dave Martin 1 year, 9 months ago
Hi Reinette,

On Thu, Apr 11, 2024 at 10:40:03AM -0700, Reinette Chatre wrote:
> Hi Dave,
> 
> On 4/11/2024 7:17 AM, Dave Martin wrote:
> > On Mon, Apr 08, 2024 at 08:24:12PM -0700, Reinette Chatre wrote:
> >> Hi James,
> >>
> >> On 3/21/2024 9:50 AM, James Morse wrote:
> >>> Pseudo-lock relies on knowledge of the micro-architecture to disable
> >>> prefetchers etc.
> >>>
> >>> On arm64 these controls are typically secure only, meaning linux can't
> >>> access them. Arm's cache-lockdown feature works in a very different
> >>> way. Resctrl's pseudo-lock isn't going to be used on arm64 platforms.
> >>>
> >>> Add a Kconfig symbol that can be selected by the architecture. This
> >>> enables or disables building of the psuedo_lock.c file, and replaces
> >>
> >> pseudo_lock.c
> > 
> > Noted.
> > 
> >>> the functions with stubs. An additional IS_ENABLED() check is needed
> >>> in rdtgroup_mode_write() so that attempting to enable pseudo-lock
> >>> reports an "Unknown or unsupported mode" to user-space.
> >>>
> >>
> >> I am missing something here. It is not obvious to me why the IS_ENABLED()
> >> check is needed. Wouldn't rdtgroup_locksetup_enter()
> >> return -EOPNOTSUPP if CONFIG_RESCTRL_FS_PSEUDO_LOCK is not enabled?
> >>
> >> Reinette
> >>
> > 
> > Hmm, if I've understood all this correctly, then it looks like the
> > existing code in rdtgroup_mode_write() relies on the dispatched
> > function (rdtgroup_locksetup_enter() etc.) to do an appropriate
> > rdt_last_cmd_puts() on failure.  If no function is called at all and
> > the requested mode change is not a no-op or otherwise trivially
> > successful, then it looks like we're supposed to fall into the else
> > clause.
> > 
> > I'd guess James' intent here was to use the fallback else {} to write
> > a suitable status string, while keeping the stub functions as trivial
> > as possible.
> > 
> > Just taking the IS_ENABLED() away would result in error return from the
> > write(), but no suitable last_cmd_status string.
> > 
> > For consistency with the existing x86 implementation, I wonder whether
> > we should put a suitable rdt_last_cmd_puts() in the stub for
> > rdtgroup_locksetup_enter().
> > 
> > There might be other ways to refactor or simplify this, though.
> > 
> > Thoughts?
> 
> Thank you for digging into this. It was not obvious to me that
> the changelog referred to the last_cmd_status string. I do
> not think this warrants making the stubs more complicated.
> 
> Reinette
> 

OK, I'll leave this as-is for now.

Cheers
---Dave