| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ecec8891-1228-4f03-841c-60fb4484c4d6@amd.com>
Date: Thu, 27 Feb 2025 14:24:16 -0600
From: "Moger, Babu" <babu.moger@....com>
To: James Morse <james.morse@....com>, x86@...nel.org,
linux-kernel@...r.kernel.org
Cc: Reinette Chatre <reinette.chatre@...el.com>,
Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
Borislav Petkov <bp@...en8.de>, H Peter Anvin <hpa@...or.com>,
shameerali.kolothum.thodi@...wei.com,
D Scott Phillips OS <scott@...amperecomputing.com>,
carl@...amperecomputing.com, lcherian@...vell.com,
bobo.shaobowang@...wei.com, tan.shaopeng@...itsu.com,
baolin.wang@...ux.alibaba.com, Jamie Iles <quic_jiles@...cinc.com>,
Xin Hao <xhao@...ux.alibaba.com>, peternewman@...gle.com,
dfustini@...libre.com, amitsinght@...vell.com,
David Hildenbrand <david@...hat.com>, Rex Nie <rex.nie@...uarmicro.com>,
Dave Martin <dave.martin@....com>, Koba Ko <kobak@...dia.com>,
Shanker Donthineni <sdonthineni@...dia.com>,
Shaopeng Tan <tan.shaopeng@...fujitsu.com>, Tony Luck <tony.luck@...el.com>
Subject: Re: [PATCH v6 02/42] x86/resctrl: Add a helper to avoid reaching into
the arch code resource list
Hi James,
You probably missed few cases here.
static int logical_rmid_to_physical_rmid(int cpu, int lrmid)
{
struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
--
}
Any reason not use the new call resctrl_arch_get_resource() here?
Thanks
Babu
On 2/7/25 12:17, James Morse wrote:
> Resctrl occasionally wants to know something about a specific resource,
> in these cases it reaches into the arch code's rdt_resources_all[]
> array.
>
> Once the filesystem parts of resctrl are moved to /fs/, this means it
> will need visibility of the architecture specific struct
> rdt_hw_resource definition, and the array of all resources. All
> architectures would also need a r_resctrl member in this struct.
>
> Instead, abstract this via a helper to allow architectures to do
> different things here. Move the level enum to the resctrl header and
> add a helper to retrieve the struct rdt_resource by 'rid'.
>
> resctrl_arch_get_resource() should not return NULL for any value in
> the enum, it may instead return a dummy resource that is
> !alloc_enabled && !mon_enabled.
>
> Co-developed-by: Dave Martin <Dave.Martin@....com>
> Signed-off-by: Dave Martin <Dave.Martin@....com>
> Signed-off-by: James Morse <james.morse@....com>
> Tested-by: Peter Newman <peternewman@...gle.com>
> Tested-by: Carl Worth <carl@...amperecomputing.com> # arm64
> Tested-by: Shaopeng Tan <tan.shaopeng@...fujitsu.com>
> Reviewed-by: Shaopeng Tan <tan.shaopeng@...fujitsu.com>
> Reviewed-by: Reinette Chatre <reinette.chatre@...el.com>
> Reviewed-by: Tony Luck <tony.luck@...el.com>
> ---
> Change since v5:
> * Ensure rdt_resources_all[] is always padded to RDT_NUM_RESOURCES
>
> Changes since v1:
> * Backed out non-functional renaming of "r" to "l3" in rdt_get_tree(),
> and unhoisted the assignment of r (as now is) back into the if ()
> where it started out. There seem to be no uses of this variable
> outside this if().
> * [Commit message only] Typo fix:
> s/resctrl_hw_resource/rdt_hw_resource/g
> ---
> arch/x86/kernel/cpu/resctrl/core.c | 12 ++++++++++--
> arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 2 +-
> arch/x86/kernel/cpu/resctrl/internal.h | 10 ----------
> arch/x86/kernel/cpu/resctrl/monitor.c | 8 ++++----
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 14 +++++++-------
> include/linux/resctrl.h | 17 +++++++++++++++++
> 6 files changed, 39 insertions(+), 24 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 3d1735ed8d1f..12b41316d8f7 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -62,7 +62,7 @@ static void mba_wrmsr_amd(struct msr_param *m);
> #define ctrl_domain_init(id) LIST_HEAD_INIT(rdt_resources_all[id].r_resctrl.ctrl_domains)
> #define mon_domain_init(id) LIST_HEAD_INIT(rdt_resources_all[id].r_resctrl.mon_domains)
>
> -struct rdt_hw_resource rdt_resources_all[] = {
> +struct rdt_hw_resource rdt_resources_all[RDT_NUM_RESOURCES] = {
> [RDT_RESOURCE_L3] =
> {
> .r_resctrl = {
> @@ -127,6 +127,14 @@ u32 resctrl_arch_system_num_rmid_idx(void)
> return r->num_rmid;
> }
>
> +struct rdt_resource *resctrl_arch_get_resource(enum resctrl_res_level l)
> +{
> + if (l >= RDT_NUM_RESOURCES)
> + return NULL;
> +
> + return &rdt_resources_all[l].r_resctrl;
> +}
> +
> /*
> * cache_alloc_hsw_probe() - Have to probe for Intel haswell server CPUs
> * as they do not have CPUID enumeration support for Cache allocation.
> @@ -174,7 +182,7 @@ static inline void cache_alloc_hsw_probe(void)
> bool is_mba_sc(struct rdt_resource *r)
> {
> if (!r)
> - return rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl.membw.mba_sc;
> + r = resctrl_arch_get_resource(RDT_RESOURCE_MBA);
>
> /*
> * The software controller support is only applicable to MBA resource.
> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> index 536351159cc2..4af27ef5a8a1 100644
> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> @@ -649,7 +649,7 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
> resid = md.u.rid;
> domid = md.u.domid;
> evtid = md.u.evtid;
> - r = &rdt_resources_all[resid].r_resctrl;
> + r = resctrl_arch_get_resource(resid);
>
> if (md.u.sum) {
> /*
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 20c898f09b7e..75252a7e1ebc 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -512,16 +512,6 @@ extern struct rdtgroup rdtgroup_default;
> extern struct dentry *debugfs_resctrl;
> extern enum resctrl_event_id mba_mbps_default_event;
>
> -enum resctrl_res_level {
> - RDT_RESOURCE_L3,
> - RDT_RESOURCE_L2,
> - RDT_RESOURCE_MBA,
> - RDT_RESOURCE_SMBA,
> -
> - /* Must be the last */
> - RDT_NUM_RESOURCES,
> -};
> -
> static inline struct rdt_resource *resctrl_inc(struct rdt_resource *res)
> {
> struct rdt_hw_resource *hw_res = resctrl_to_arch_res(res);
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 94a1d9780461..58b5b21349a8 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -365,7 +365,7 @@ static void limbo_release_entry(struct rmid_entry *entry)
> */
> void __check_limbo(struct rdt_mon_domain *d, bool force_free)
> {
> - struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> + struct rdt_resource *r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
> u32 idx_limit = resctrl_arch_system_num_rmid_idx();
> struct rmid_entry *entry;
> u32 idx, cur_idx = 1;
> @@ -521,7 +521,7 @@ int alloc_rmid(u32 closid)
>
> static void add_rmid_to_limbo(struct rmid_entry *entry)
> {
> - struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> + struct rdt_resource *r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
> struct rdt_mon_domain *d;
> u32 idx;
>
> @@ -761,7 +761,7 @@ static void update_mba_bw(struct rdtgroup *rgrp, struct rdt_mon_domain *dom_mbm)
> struct rdtgroup *entry;
> u32 cur_bw, user_bw;
>
> - r_mba = &rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl;
> + r_mba = resctrl_arch_get_resource(RDT_RESOURCE_MBA);
> evt_id = rgrp->mba_mbps_event;
>
> closid = rgrp->closid;
> @@ -925,7 +925,7 @@ void mbm_handle_overflow(struct work_struct *work)
> if (!resctrl_mounted || !resctrl_arch_mon_capable())
> goto out_unlock;
>
> - r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> + r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
> d = container_of(work, struct rdt_mon_domain, mbm_over.work);
>
> list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) {
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 04b653d613e8..45093b9e8e63 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2256,7 +2256,7 @@ static void l2_qos_cfg_update(void *arg)
>
> static inline bool is_mba_linear(void)
> {
> - return rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl.membw.delay_linear;
> + return resctrl_arch_get_resource(RDT_RESOURCE_MBA)->membw.delay_linear;
> }
>
> static int set_cache_qos_cfg(int level, bool enable)
> @@ -2346,8 +2346,8 @@ static void mba_sc_domain_destroy(struct rdt_resource *r,
> */
> static bool supports_mba_mbps(void)
> {
> - struct rdt_resource *rmbm = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> - struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl;
> + struct rdt_resource *rmbm = resctrl_arch_get_resource(RDT_RESOURCE_L3);
> + struct rdt_resource *r = resctrl_arch_get_resource(RDT_RESOURCE_MBA);
>
> return (is_mbm_enabled() &&
> r->alloc_capable && is_mba_linear() &&
> @@ -2360,7 +2360,7 @@ static bool supports_mba_mbps(void)
> */
> static int set_mba_sc(bool mba_sc)
> {
> - struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl;
> + struct rdt_resource *r = resctrl_arch_get_resource(RDT_RESOURCE_MBA);
> u32 num_closid = resctrl_arch_get_num_closid(r);
> struct rdt_ctrl_domain *d;
> unsigned long fflags;
> @@ -2714,7 +2714,7 @@ static int rdt_get_tree(struct fs_context *fc)
> resctrl_mounted = true;
>
> if (is_mbm_enabled()) {
> - r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> + r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
> list_for_each_entry(dom, &r->mon_domains, hdr.list)
> mbm_setup_overflow_handler(dom, MBM_OVERFLOW_INTERVAL,
> RESCTRL_PICK_ANY_CPU);
> @@ -3951,7 +3951,7 @@ static int rdtgroup_show_options(struct seq_file *seq, struct kernfs_root *kf)
> if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L2))
> seq_puts(seq, ",cdpl2");
>
> - if (is_mba_sc(&rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl))
> + if (is_mba_sc(resctrl_arch_get_resource(RDT_RESOURCE_MBA)))
> seq_puts(seq, ",mba_MBps");
>
> if (resctrl_debug)
> @@ -4151,7 +4151,7 @@ static void clear_childcpus(struct rdtgroup *r, unsigned int cpu)
>
> void resctrl_offline_cpu(unsigned int cpu)
> {
> - struct rdt_resource *l3 = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> + struct rdt_resource *l3 = resctrl_arch_get_resource(RDT_RESOURCE_L3);
> struct rdt_mon_domain *d;
> struct rdtgroup *rdtgrp;
>
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index d94abba1c716..37279e2a89da 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -37,6 +37,16 @@ enum resctrl_conf_type {
> CDP_DATA,
> };
>
> +enum resctrl_res_level {
> + RDT_RESOURCE_L3,
> + RDT_RESOURCE_L2,
> + RDT_RESOURCE_MBA,
> + RDT_RESOURCE_SMBA,
> +
> + /* Must be the last */
> + RDT_NUM_RESOURCES,
> +};
> +
> #define CDP_NUM_TYPES (CDP_DATA + 1)
>
> /*
> @@ -226,6 +236,13 @@ struct rdt_resource {
> bool cdp_capable;
> };
>
> +/*
> + * Get the resource that exists at this level. If the level is not supported
> + * a dummy/not-capable resource can be returned. Levels >= RDT_NUM_RESOURCES
> + * will return NULL.
> + */
> +struct rdt_resource *resctrl_arch_get_resource(enum resctrl_res_level l);
> +
> /**
> * struct resctrl_schema - configuration abilities of a resource presented to
> * user-space
--
Thanks
Babu Moger
Powered by blists - more mailing lists