[PATCH v8 00/13] arch/resctrl: AMD QoS support

[PATCH v8 00/13] arch/resctrl: AMD QoS support

[Moger, Babu]

This series adds support for AMD64 architectural extensions for Platform
Quality of Service. These extensions are intended to provide for the
monitoring of the usage of certain system resources by one or more
processors and for the separate allocation and enforcement of limits on
the use of certain system resources by one or more processors.

The monitoring and enforcement are not necessarily applied across the
entire system, but in general apply to a QOS domain which corresponds to
some shared system resource.  The set of resources which are monitored and
the set for which the enforcement of limits is provided are implementation
dependent. Platform QOS features are implemented on a logical processor
basis. Therefore, multiple hardware threads of a single physical CPU core
may have independent resource monitoring and enforcement configurations.

AMD's next generation of processors support following QoS sub-features.
- L3 Cache allocation enforcement
- L3 Cache occupancy monitoring
- L3 Code-Data Prioritization support
- Memory Bandwidth Enforcement(Allocation)

The public specification for this feature is available at
https://developer.amd.com/wp-content/resources/56375.pdf

Obviously, there are multiple ways we can go about these changes. We felt
it is appropriate to rename and re-organize the code little bit before
making the functional changes. The first few patches(1-10) renames and
re-organizes the sources in preparation. Rest of the patches(7-11) adds
support for AMD QoS features.

Please review.

Changes from v7 -> v8:
https://lore.kernel.org/lkml/20181109205153.14811-1-babu.moger@amd.com/
 a. Addressed comments from Borislav Petkov.
    Changed the subject line to arch/resctrl from arch/x86.
    Few code cleanup related changes on patch 2 and 3.

Changes from v6 -> v7:
 a. Mainly addressed the comments from Mattew Wilcox. Removed both 
    CONFIG_INTEL_RDT and CONFIG_AMD_QOS config parameter. We dont need
    to use two separate config parameter to enable this feature. The
    final code will be same in both the cases. So, used the new vendor
    neutral parameter CONFIG_RESCTRL to enable resource control feature
    for both Intel and AMD 

Changes from v5 -> v6:
https://lore.kernel.org/lkml/20181106154034.15386-1-babu.moger@amd.com/
 a. Addressed comments from Fenghua Yu. Added vendor check while
    detecting MBA software controller support.
 a. Rebased again on top of
    git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/cache

Changes from v4 -> v5:
https://lore.kernel.org/lkml/20181018225124.23043-1-babu.moger@amd.com/
 a. Addressed comments from Fenghua Yu.
 b. The functions update_mba_bw and set_mba_sc is not required for AMD.
    Removed all the changes related to these functions.

Changes from v3 -> v4:
https://lore.kernel.org/lkml/20181015205514.25387-1-babu.moger@amd.com/
 a. Addressed comments from Reinette Chatre and Borislav Petkov.
 b. Removed X86 dependency for CONFIG_AMD_QOS. Implicitly is it already
    dependent on X86.
 c. Updated the MAINTAINER file for name changes.
 d. Addressed most of "checkpatch.pl --strict" issues.
 d. Updated Documentation/x86/resctrl_ui.txt(previously
    intel_rdt_ui.txt) file with AMD specific details. Changed few names
    to resctrl from intel_rdt.

Changes from v2 -> v3:
 https://lore.kernel.org/lkml/20181011203223.18157-1-babu.moger@amd.com/
 a. Rebased the patches on top of below branch as suggested by Thomas Gleixner.
    git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/cache
 b. Addressed comments from Reinette Chatre, Fenghua Yu and Borislav Petkov.
 c. Main changes are related to renaming the files and functions.
    Renamed from intel_rdt to more generic resctrl(patches 1 to 3).
 d. Config parameter changed from PLATFORM_QOS to more generic RESCTRL.
 e. Fixed minor indentation issues.

Changes from v1 -> v2:
 https://lore.kernel.org/lkml/20181005205512.29545-1-babu.moger@amd.com/
 a. Removed RFC from subject header. Based on the discussion so far, 
    plan is to go ahead with these patches and eventually re-structure
    the code to make arch and non-arch separate.
 b. Addressed comments from Reinette Chatre and Fenghua Yu.
 c. Separated quirks and MBA from rdt init code. Kept the rest of the
    code as is.
 d. Added _intel suffixes all the Intel only code just like AMD code.
 e. Added one more patch to bring the macros into header file.
 f. Few minor text changes.

v1:
 https://lore.kernel.org/lkml/20180924191841.29111-1-babu.moger@amd.com/

Babu Moger (12):
  arch/resctrl: Start renaming the rdt files to more generic names
  arch/resctrl: Rename the RDT functions and definitions
  arch/resctrl: Re-arrange RDT init code
  arch/resctrl: Bring all the macros to resctrl.h
  arch/resctrl: Rename config parameter INTEL_RDT to RESCTRL
  arch/resctrl: Initialize the resource functions that are different
  arch/resctrl: Bring cbm_validate function into the resource structure
  arch/resctrl: Add vendor check for MBA software controller
  arch/resctrl: Update the RESCTRL config parameter
  arch/resctrl: Introduce QOS feature for AMD
  Documentation: Rename and update intel_rdt_ui.txt to resctrl_ui.txt
  MAINTAINERS: Update the file and documentation names in arch/x86

Sherry Hurwitz (1):
  arch/resctrl: Add AMD feature bit X86_FEATURE_MBA in cpuid bits array

 .../x86/{intel_rdt_ui.txt => resctrl_ui.txt}  |   9 +-
 MAINTAINERS                                   |   6 +-
 arch/x86/Kconfig                              |  23 ++-
 .../{intel_rdt_sched.h => resctrl_sched.h}    |  28 +--
 arch/x86/kernel/cpu/Makefile                  |   6 +-
 .../x86/kernel/cpu/{intel_rdt.c => resctrl.c} | 165 +++++++++++++++---
 .../x86/kernel/cpu/{intel_rdt.h => resctrl.h} |  37 ++--
 ...dt_ctrlmondata.c => resctrl_ctrlmondata.c} |  80 ++++++++-
 ...{intel_rdt_monitor.c => resctrl_monitor.c} |  20 +--
 ...dt_pseudo_lock.c => resctrl_pseudo_lock.c} |   6 +-
 ...ck_event.h => resctrl_pseudo_lock_event.h} |   2 +-
 ...ntel_rdt_rdtgroup.c => resctrl_rdtgroup.c} |  19 +-
 arch/x86/kernel/cpu/scattered.c               |   7 +-
 arch/x86/kernel/process_32.c                  |   4 +-
 arch/x86/kernel/process_64.c                  |   4 +-
 include/linux/sched.h                         |   2 +-
 16 files changed, 315 insertions(+), 103 deletions(-)
 rename Documentation/x86/{intel_rdt_ui.txt => resctrl_ui.txt} (99%)
 rename arch/x86/include/asm/{intel_rdt_sched.h => resctrl_sched.h} (77%)
 rename arch/x86/kernel/cpu/{intel_rdt.c => resctrl.c} (85%)
 rename arch/x86/kernel/cpu/{intel_rdt.h => resctrl.h} (94%)
 rename arch/x86/kernel/cpu/{intel_rdt_ctrlmondata.c => resctrl_ctrlmondata.c} (86%)
 rename arch/x86/kernel/cpu/{intel_rdt_monitor.c => resctrl_monitor.c} (97%)
 rename arch/x86/kernel/cpu/{intel_rdt_pseudo_lock.c => resctrl_pseudo_lock.c} (99%)
 rename arch/x86/kernel/cpu/{intel_rdt_pseudo_lock_event.h => resctrl_pseudo_lock_event.h} (95%)
 rename arch/x86/kernel/cpu/{intel_rdt_rdtgroup.c => resctrl_rdtgroup.c} (99%)

-- 
2.17.1

[PATCH v8 01/13] arch/resctrl: Start renaming the rdt files to more generic names

[Moger, Babu]

New generation of AMD processors start supporting RDT(or QOS) features.
With more than one vendors supporting these features, it seems more
appropriate to rename these files.

Changed intel_rdt to resctrl where applicable.

Signed-off-by: Babu Moger <babu.moger@amd.com>
Reviewed-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/include/asm/{intel_rdt_sched.h => resctrl_sched.h} | 0
 arch/x86/kernel/cpu/Makefile                                | 6 +++---
 arch/x86/kernel/cpu/{intel_rdt.c => resctrl.c}              | 4 ++--
 arch/x86/kernel/cpu/{intel_rdt.h => resctrl.h}              | 6 +++---
 .../cpu/{intel_rdt_ctrlmondata.c => resctrl_ctrlmondata.c}  | 2 +-
 .../kernel/cpu/{intel_rdt_monitor.c => resctrl_monitor.c}   | 2 +-
 .../cpu/{intel_rdt_pseudo_lock.c => resctrl_pseudo_lock.c}  | 6 +++---
 ..._rdt_pseudo_lock_event.h => resctrl_pseudo_lock_event.h} | 2 +-
 .../kernel/cpu/{intel_rdt_rdtgroup.c => resctrl_rdtgroup.c} | 4 ++--
 arch/x86/kernel/process_32.c                                | 2 +-
 arch/x86/kernel/process_64.c                                | 2 +-
 11 files changed, 18 insertions(+), 18 deletions(-)
 rename arch/x86/include/asm/{intel_rdt_sched.h => resctrl_sched.h} (100%)
 rename arch/x86/kernel/cpu/{intel_rdt.c => resctrl.c} (99%)
 rename arch/x86/kernel/cpu/{intel_rdt.h => resctrl.h} (99%)
 rename arch/x86/kernel/cpu/{intel_rdt_ctrlmondata.c => resctrl_ctrlmondata.c} (99%)
 rename arch/x86/kernel/cpu/{intel_rdt_monitor.c => resctrl_monitor.c} (99%)
 rename arch/x86/kernel/cpu/{intel_rdt_pseudo_lock.c => resctrl_pseudo_lock.c} (99%)
 rename arch/x86/kernel/cpu/{intel_rdt_pseudo_lock_event.h => resctrl_pseudo_lock_event.h} (95%)
 rename arch/x86/kernel/cpu/{intel_rdt_rdtgroup.c => resctrl_rdtgroup.c} (99%)

diff --git a/arch/x86/include/asm/intel_rdt_sched.h b/arch/x86/include/asm/resctrl_sched.h
similarity index 100%
rename from arch/x86/include/asm/intel_rdt_sched.h
rename to arch/x86/include/asm/resctrl_sched.h
diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
index 347137e80bf5..222cf8cc078d 100644
--- a/arch/x86/kernel/cpu/Makefile
+++ b/arch/x86/kernel/cpu/Makefile
@@ -35,9 +35,9 @@ obj-$(CONFIG_CPU_SUP_CENTAUR)		+= centaur.o
 obj-$(CONFIG_CPU_SUP_TRANSMETA_32)	+= transmeta.o
 obj-$(CONFIG_CPU_SUP_UMC_32)		+= umc.o
 
-obj-$(CONFIG_INTEL_RDT)	+= intel_rdt.o intel_rdt_rdtgroup.o intel_rdt_monitor.o
-obj-$(CONFIG_INTEL_RDT)	+= intel_rdt_ctrlmondata.o intel_rdt_pseudo_lock.o
-CFLAGS_intel_rdt_pseudo_lock.o = -I$(src)
+obj-$(CONFIG_INTEL_RDT)	+= resctrl.o resctrl_rdtgroup.o resctrl_monitor.o
+obj-$(CONFIG_INTEL_RDT)	+= resctrl_ctrlmondata.o resctrl_pseudo_lock.o
+CFLAGS_resctrl_pseudo_lock.o = -I$(src)
 
 obj-$(CONFIG_X86_MCE)			+= mcheck/
 obj-$(CONFIG_MTRR)			+= mtrr/
diff --git a/arch/x86/kernel/cpu/intel_rdt.c b/arch/x86/kernel/cpu/resctrl.c
similarity index 99%
rename from arch/x86/kernel/cpu/intel_rdt.c
rename to arch/x86/kernel/cpu/resctrl.c
index 44272b7107ad..3f1a135a4ae0 100644
--- a/arch/x86/kernel/cpu/intel_rdt.c
+++ b/arch/x86/kernel/cpu/resctrl.c
@@ -30,8 +30,8 @@
 #include <linux/cpuhotplug.h>
 
 #include <asm/intel-family.h>
-#include <asm/intel_rdt_sched.h>
-#include "intel_rdt.h"
+#include <asm/resctrl_sched.h>
+#include "resctrl.h"
 
 #define MBA_IS_LINEAR	0x4
 #define MBA_MAX_MBPS	U32_MAX
diff --git a/arch/x86/kernel/cpu/intel_rdt.h b/arch/x86/kernel/cpu/resctrl.h
similarity index 99%
rename from arch/x86/kernel/cpu/intel_rdt.h
rename to arch/x86/kernel/cpu/resctrl.h
index 3736f6dc9545..a9d906767bb2 100644
--- a/arch/x86/kernel/cpu/intel_rdt.h
+++ b/arch/x86/kernel/cpu/resctrl.h
@@ -1,6 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0 */
-#ifndef _ASM_X86_INTEL_RDT_H
-#define _ASM_X86_INTEL_RDT_H
+#ifndef _ASM_X86_RESCTRL_H
+#define _ASM_X86_RESCTRL_H
 
 #include <linux/sched.h>
 #include <linux/kernfs.h>
@@ -568,4 +568,4 @@ void cqm_handle_limbo(struct work_struct *work);
 bool has_busy_rmid(struct rdt_resource *r, struct rdt_domain *d);
 void __check_limbo(struct rdt_domain *d, bool force_free);
 
-#endif /* _ASM_X86_INTEL_RDT_H */
+#endif /* _ASM_X86_RESCTRL_H */
diff --git a/arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c b/arch/x86/kernel/cpu/resctrl_ctrlmondata.c
similarity index 99%
rename from arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c
rename to arch/x86/kernel/cpu/resctrl_ctrlmondata.c
index 27937458c231..e8d21306d0f9 100644
--- a/arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl_ctrlmondata.c
@@ -26,7 +26,7 @@
 #include <linux/kernfs.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
-#include "intel_rdt.h"
+#include "resctrl.h"
 
 /*
  * Check whether MBA bandwidth percentage value is correct. The value is
diff --git a/arch/x86/kernel/cpu/intel_rdt_monitor.c b/arch/x86/kernel/cpu/resctrl_monitor.c
similarity index 99%
rename from arch/x86/kernel/cpu/intel_rdt_monitor.c
rename to arch/x86/kernel/cpu/resctrl_monitor.c
index b0f3aed76b75..211d97bcbde5 100644
--- a/arch/x86/kernel/cpu/intel_rdt_monitor.c
+++ b/arch/x86/kernel/cpu/resctrl_monitor.c
@@ -26,7 +26,7 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <asm/cpu_device_id.h>
-#include "intel_rdt.h"
+#include "resctrl.h"
 
 #define MSR_IA32_QM_CTR		0x0c8e
 #define MSR_IA32_QM_EVTSEL		0x0c8d
diff --git a/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c b/arch/x86/kernel/cpu/resctrl_pseudo_lock.c
similarity index 99%
rename from arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
rename to arch/x86/kernel/cpu/resctrl_pseudo_lock.c
index 966ac0c20d67..f9c15fafd467 100644
--- a/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
+++ b/arch/x86/kernel/cpu/resctrl_pseudo_lock.c
@@ -24,14 +24,14 @@
 
 #include <asm/cacheflush.h>
 #include <asm/intel-family.h>
-#include <asm/intel_rdt_sched.h>
+#include <asm/resctrl_sched.h>
 #include <asm/perf_event.h>
 
 #include "../../events/perf_event.h" /* For X86_CONFIG() */
-#include "intel_rdt.h"
+#include "resctrl.h"
 
 #define CREATE_TRACE_POINTS
-#include "intel_rdt_pseudo_lock_event.h"
+#include "resctrl_pseudo_lock_event.h"
 
 /*
  * MSR_MISC_FEATURE_CONTROL register enables the modification of hardware
diff --git a/arch/x86/kernel/cpu/intel_rdt_pseudo_lock_event.h b/arch/x86/kernel/cpu/resctrl_pseudo_lock_event.h
similarity index 95%
rename from arch/x86/kernel/cpu/intel_rdt_pseudo_lock_event.h
rename to arch/x86/kernel/cpu/resctrl_pseudo_lock_event.h
index 2c041e6d9f05..d354851ed66c 100644
--- a/arch/x86/kernel/cpu/intel_rdt_pseudo_lock_event.h
+++ b/arch/x86/kernel/cpu/resctrl_pseudo_lock_event.h
@@ -39,5 +39,5 @@ TRACE_EVENT(pseudo_lock_l3,
 
 #undef TRACE_INCLUDE_PATH
 #define TRACE_INCLUDE_PATH .
-#define TRACE_INCLUDE_FILE intel_rdt_pseudo_lock_event
+#define TRACE_INCLUDE_FILE resctrl_pseudo_lock_event
 #include <trace/define_trace.h>
diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c b/arch/x86/kernel/cpu/resctrl_rdtgroup.c
similarity index 99%
rename from arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
rename to arch/x86/kernel/cpu/resctrl_rdtgroup.c
index f27b8115ffa2..6308ba3905e5 100644
--- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl_rdtgroup.c
@@ -35,8 +35,8 @@
 
 #include <uapi/linux/magic.h>
 
-#include <asm/intel_rdt_sched.h>
-#include "intel_rdt.h"
+#include <asm/resctrl_sched.h>
+#include "resctrl.h"
 
 DEFINE_STATIC_KEY_FALSE(rdt_enable_key);
 DEFINE_STATIC_KEY_FALSE(rdt_mon_enable_key);
diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index 5046a3c9dec2..d16207e7d1e5 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -56,7 +56,7 @@
 #include <asm/debugreg.h>
 #include <asm/switch_to.h>
 #include <asm/vm86.h>
-#include <asm/intel_rdt_sched.h>
+#include <asm/resctrl_sched.h>
 #include <asm/proto.h>
 
 void __show_regs(struct pt_regs *regs, enum show_regs_mode mode)
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index ea5ea850348d..af73223ad6ba 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -52,7 +52,7 @@
 #include <asm/switch_to.h>
 #include <asm/xen/hypervisor.h>
 #include <asm/vdso.h>
-#include <asm/intel_rdt_sched.h>
+#include <asm/resctrl_sched.h>
 #include <asm/unistd.h>
 #ifdef CONFIG_IA32_EMULATION
 /* Not included via unistd.h */
-- 
2.17.1

Re: [PATCH v8 01/13] arch/resctrl: Start renaming the rdt files to more generic names

[Borislav Petkov]

Just to state it here on the thread: prefix should be "x86/resctrl" as
we said earlier. But that will be addressed in the next iteration, as we
agreed offlist.

On Fri, Nov 16, 2018 at 08:54:22PM +0000, Moger, Babu wrote:
> New generation of AMD processors start supporting RDT(or QOS) features.
> With more than one vendors supporting these features, it seems more
> appropriate to rename these files.
> 
> Changed intel_rdt to resctrl where applicable.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> Reviewed-by: Borislav Petkov <bp@suse.de>
> ---
>  arch/x86/include/asm/{intel_rdt_sched.h => resctrl_sched.h} | 0
>  arch/x86/kernel/cpu/Makefile                                | 6 +++---
>  arch/x86/kernel/cpu/{intel_rdt.c => resctrl.c}              | 4 ++--
>  arch/x86/kernel/cpu/{intel_rdt.h => resctrl.h}              | 6 +++---
>  .../cpu/{intel_rdt_ctrlmondata.c => resctrl_ctrlmondata.c}  | 2 +-
>  .../kernel/cpu/{intel_rdt_monitor.c => resctrl_monitor.c}   | 2 +-
>  .../cpu/{intel_rdt_pseudo_lock.c => resctrl_pseudo_lock.c}  | 6 +++---
>  ..._rdt_pseudo_lock_event.h => resctrl_pseudo_lock_event.h} | 2 +-
>  .../kernel/cpu/{intel_rdt_rdtgroup.c => resctrl_rdtgroup.c} | 4 ++--
>  arch/x86/kernel/process_32.c                                | 2 +-
>  arch/x86/kernel/process_64.c                                | 2 +-
>  11 files changed, 18 insertions(+), 18 deletions(-)
>  rename arch/x86/include/asm/{intel_rdt_sched.h => resctrl_sched.h} (100%)
>  rename arch/x86/kernel/cpu/{intel_rdt.c => resctrl.c} (99%)
>  rename arch/x86/kernel/cpu/{intel_rdt.h => resctrl.h} (99%)
>  rename arch/x86/kernel/cpu/{intel_rdt_ctrlmondata.c => resctrl_ctrlmondata.c} (99%)
>  rename arch/x86/kernel/cpu/{intel_rdt_monitor.c => resctrl_monitor.c} (99%)
>  rename arch/x86/kernel/cpu/{intel_rdt_pseudo_lock.c => resctrl_pseudo_lock.c} (99%)
>  rename arch/x86/kernel/cpu/{intel_rdt_pseudo_lock_event.h => resctrl_pseudo_lock_event.h} (95%)
>  rename arch/x86/kernel/cpu/{intel_rdt_rdtgroup.c => resctrl_rdtgroup.c} (99%)

Ok, looking at this more, it looks like this:

ls -l arch/x86/kernel/cpu/resctrl*
 arch/x86/kernel/cpu/resctrl.c
 arch/x86/kernel/cpu/resctrl_ctrlmondata.c
 arch/x86/kernel/cpu/resctrl.h
 arch/x86/kernel/cpu/resctrl_monitor.c
 arch/x86/kernel/cpu/resctrl_pseudo_lock.c
 arch/x86/kernel/cpu/resctrl_pseudo_lock_event.h
 arch/x86/kernel/cpu/resctrl_rdtgroup.c

Now, it looks to me like the whole resctrl thing begs for its own dir:

 arch/x86/kernel/cpu/resctrl/core.c
 arch/x86/kernel/cpu/resctrl/ctrlmondata.c
 arch/x86/kernel/cpu/resctrl/internal.h
 arch/x86/kernel/cpu/resctrl/monitor.c
 arch/x86/kernel/cpu/resctrl/pseudo_lock.c
 arch/x86/kernel/cpu/resctrl/pseudo_lock_event.h
 arch/x86/kernel/cpu/resctrl/rdtgroup.c

This way, the whole RDT machinery will be confined to its own directory.

Hmm.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v8 01/13] arch/resctrl: Start renaming the rdt files to more generic names

[Thomas Gleixner]

On Sun, 18 Nov 2018, Borislav Petkov wrote:
> 
> Now, it looks to me like the whole resctrl thing begs for its own dir:
> 
>  arch/x86/kernel/cpu/resctrl/core.c
>  arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>  arch/x86/kernel/cpu/resctrl/internal.h
>  arch/x86/kernel/cpu/resctrl/monitor.c
>  arch/x86/kernel/cpu/resctrl/pseudo_lock.c
>  arch/x86/kernel/cpu/resctrl/pseudo_lock_event.h
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c
> 
> This way, the whole RDT machinery will be confined to its own directory.

Fine with that.

Thanks,

	tglx

RE: [PATCH v8 01/13] arch/resctrl: Start renaming the rdt files to more generic names

[Moger, Babu]

Boris/Thomas,

> -----Original Message-----
> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Monday, November 19, 2018 1:14 PM
> To: Borislav Petkov <bp@alien8.de>
> Cc: Moger, Babu <Babu.Moger@amd.com>; mingo@redhat.com;
> corbet@lwn.net; fenghua.yu@intel.com; reinette.chatre@intel.com;
> peterz@infradead.org; gregkh@linuxfoundation.org;
> davem@davemloft.net; akpm@linux-foundation.org; hpa@zytor.com;
> x86@kernel.org; mchehab+samsung@kernel.org; arnd@arndb.de;
> kstewart@linuxfoundation.org; pombredanne@nexb.com;
> rafael@kernel.org; kirill.shutemov@linux.intel.com; tony.luck@intel.com;
> qianyue.zj@alibaba-inc.com; xiaochen.shen@intel.com;
> pbonzini@redhat.com; Singh, Brijesh <brijesh.singh@amd.com>; Hurwitz,
> Sherry <sherry.hurwitz@amd.com>; dwmw2@infradead.org; Lendacky,
> Thomas <Thomas.Lendacky@amd.com>; luto@kernel.org; joro@8bytes.org;
> jannh@google.com; vkuznets@redhat.com; rian@alum.mit.edu;
> jpoimboe@redhat.com; linux-kernel@vger.kernel.org; linux-
> doc@vger.kernel.org
> Subject: Re: [PATCH v8 01/13] arch/resctrl: Start renaming the rdt files to
> more generic names
> 
> On Sun, 18 Nov 2018, Borislav Petkov wrote:
> >
> > Now, it looks to me like the whole resctrl thing begs for its own dir:
> >
> >  arch/x86/kernel/cpu/resctrl/core.c
> >  arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> >  arch/x86/kernel/cpu/resctrl/internal.h
> >  arch/x86/kernel/cpu/resctrl/monitor.c
> >  arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> >  arch/x86/kernel/cpu/resctrl/pseudo_lock_event.h
> >  arch/x86/kernel/cpu/resctrl/rdtgroup.c
> >
> > This way, the whole RDT machinery will be confined to its own directory.
> 
> Fine with that.

Ok. Sure.  Will change it. How does this look like?

arch/x86/kernel/cpu/resctrl/res.c
arch/x86/kernel/cpu/resctrl/ctrlmondata.c
arch/x86/kernel/cpu/resctrl/res.h
arch/x86/kernel/cpu/resctrl/monitor.c
arch/x86/kernel/cpu/resctrl/pseudo_lock.c
arch/x86/kernel/cpu/resctrl/pseudo_lock_event.h
arch/x86/kernel/cpu/resctrl/rdtgroup.c

Changed core.c and internel.h to res.c and res.h respectively. Both of
these files are about resource definitions and initialization.

> 
> Thanks,
> 
> 	tglx

Re: [PATCH v8 01/13] arch/resctrl: Start renaming the rdt files to more generic names

[Borislav Petkov]

On Mon, Nov 19, 2018 at 08:11:36PM +0000, Moger, Babu wrote:
> Changed core.c and internel.h to res.c and res.h respectively. Both of
> these files are about resource definitions and initialization.

I guess but the core.c thing we do a lot in the kernel:

$ git ls-files | grep -E "/core\.c" | wc -l
100

which denotes where the core functionality is located. res.c
doesn't tell me that. res.h doesn't tell me either that it is an
resctrl-internal header...

IOW, calling it core.c and internal.h gives you an additional
information what those files contain. And I think that's useful.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v8 01/13] arch/resctrl: Start renaming the rdt files to more generic names

[Moger, Babu]

On 11/19/18 2:22 PM, Borislav Petkov wrote:
> On Mon, Nov 19, 2018 at 08:11:36PM +0000, Moger, Babu wrote:
>> Changed core.c and internel.h to res.c and res.h respectively. Both of
>> these files are about resource definitions and initialization.
> 
> I guess but the core.c thing we do a lot in the kernel:
> 
> $ git ls-files | grep -E "/core\.c" | wc -l
> 100
> 
> which denotes where the core functionality is located. res.c
> doesn't tell me that. res.h doesn't tell me either that it is an
> resctrl-internal header...
> 
> IOW, calling it core.c and internal.h gives you an additional
> information what those files contain. And I think that's useful.

Ok. Fine. Will keep it as core.c and internel.h.

[PATCH v8 02/13] arch/resctrl: Rename the RDT functions and definitions

[Moger, Babu]

As AMD is starting to support RDT(or QOS) features, rename
the RDT functions and definitions to more generic names.

Replace intel_rdt to resctrl where applicable.

Signed-off-by: Babu Moger <babu.moger@amd.com>
Reviewed-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/include/asm/resctrl_sched.h   | 24 ++++++++++++------------
 arch/x86/kernel/cpu/resctrl.c          | 26 +++++++++++++-------------
 arch/x86/kernel/cpu/resctrl.h          |  2 +-
 arch/x86/kernel/cpu/resctrl_monitor.c  | 11 ++++++-----
 arch/x86/kernel/cpu/resctrl_rdtgroup.c | 10 +++++-----
 arch/x86/kernel/process_32.c           |  2 +-
 arch/x86/kernel/process_64.c           |  2 +-
 7 files changed, 39 insertions(+), 38 deletions(-)

diff --git a/arch/x86/include/asm/resctrl_sched.h b/arch/x86/include/asm/resctrl_sched.h
index 9acb06b6f81e..6e082697a613 100644
--- a/arch/x86/include/asm/resctrl_sched.h
+++ b/arch/x86/include/asm/resctrl_sched.h
@@ -1,6 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0 */
-#ifndef _ASM_X86_INTEL_RDT_SCHED_H
-#define _ASM_X86_INTEL_RDT_SCHED_H
+#ifndef _ASM_X86_RESCTRL_SCHED_H
+#define _ASM_X86_RESCTRL_SCHED_H
 
 #ifdef CONFIG_INTEL_RDT
 
@@ -10,7 +10,7 @@
 #define IA32_PQR_ASSOC	0x0c8f
 
 /**
- * struct intel_pqr_state - State cache for the PQR MSR
+ * struct resctrl_pqr_state - State cache for the PQR MSR
  * @cur_rmid:		The cached Resource Monitoring ID
  * @cur_closid:	The cached Class Of Service ID
  * @default_rmid:	The user assigned Resource Monitoring ID
@@ -24,21 +24,21 @@
  * The cache also helps to avoid pointless updates if the value does
  * not change.
  */
-struct intel_pqr_state {
+struct resctrl_pqr_state {
 	u32			cur_rmid;
 	u32			cur_closid;
 	u32			default_rmid;
 	u32			default_closid;
 };
 
-DECLARE_PER_CPU(struct intel_pqr_state, pqr_state);
+DECLARE_PER_CPU(struct resctrl_pqr_state, pqr_state);
 
 DECLARE_STATIC_KEY_FALSE(rdt_enable_key);
 DECLARE_STATIC_KEY_FALSE(rdt_alloc_enable_key);
 DECLARE_STATIC_KEY_FALSE(rdt_mon_enable_key);
 
 /*
- * __intel_rdt_sched_in() - Writes the task's CLOSid/RMID to IA32_PQR_MSR
+ * __resctrl_sched_in() - Writes the task's CLOSid/RMID to IA32_PQR_MSR
  *
  * Following considerations are made so that this has minimal impact
  * on scheduler hot path:
@@ -51,9 +51,9 @@ DECLARE_STATIC_KEY_FALSE(rdt_mon_enable_key);
  *   simple as possible.
  * Must be called with preemption disabled.
  */
-static void __intel_rdt_sched_in(void)
+static void __resctrl_sched_in(void)
 {
-	struct intel_pqr_state *state = this_cpu_ptr(&pqr_state);
+	struct resctrl_pqr_state *state = this_cpu_ptr(&pqr_state);
 	u32 closid = state->default_closid;
 	u32 rmid = state->default_rmid;
 
@@ -78,16 +78,16 @@ static void __intel_rdt_sched_in(void)
 	}
 }
 
-static inline void intel_rdt_sched_in(void)
+static inline void resctrl_sched_in(void)
 {
 	if (static_branch_likely(&rdt_enable_key))
-		__intel_rdt_sched_in();
+		__resctrl_sched_in();
 }
 
 #else
 
-static inline void intel_rdt_sched_in(void) {}
+static inline void resctrl_sched_in(void) {}
 
 #endif /* CONFIG_INTEL_RDT */
 
-#endif /* _ASM_X86_INTEL_RDT_SCHED_H */
+#endif /* _ASM_X86_RESCTRL_SCHED_H */
diff --git a/arch/x86/kernel/cpu/resctrl.c b/arch/x86/kernel/cpu/resctrl.c
index 3f1a135a4ae0..5d526dc45751 100644
--- a/arch/x86/kernel/cpu/resctrl.c
+++ b/arch/x86/kernel/cpu/resctrl.c
@@ -40,12 +40,12 @@
 DEFINE_MUTEX(rdtgroup_mutex);
 
 /*
- * The cached intel_pqr_state is strictly per CPU and can never be
+ * The cached resctrl_pqr_state is strictly per CPU and can never be
  * updated from a remote CPU. Functions which modify the state
  * are called with interrupts disabled and no preemption, which
  * is sufficient for the protection.
  */
-DEFINE_PER_CPU(struct intel_pqr_state, pqr_state);
+DEFINE_PER_CPU(struct resctrl_pqr_state, pqr_state);
 
 /*
  * Used to store the max resource name width and max resource data width
@@ -639,7 +639,7 @@ static void domain_remove_cpu(int cpu, struct rdt_resource *r)
 
 static void clear_closid_rmid(int cpu)
 {
-	struct intel_pqr_state *state = this_cpu_ptr(&pqr_state);
+	struct resctrl_pqr_state *state = this_cpu_ptr(&pqr_state);
 
 	state->default_closid = 0;
 	state->default_rmid = 0;
@@ -648,7 +648,7 @@ static void clear_closid_rmid(int cpu)
 	wrmsr(IA32_PQR_ASSOC, 0, 0);
 }
 
-static int intel_rdt_online_cpu(unsigned int cpu)
+static int resctrl_online_cpu(unsigned int cpu)
 {
 	struct rdt_resource *r;
 
@@ -674,7 +674,7 @@ static void clear_childcpus(struct rdtgroup *r, unsigned int cpu)
 	}
 }
 
-static int intel_rdt_offline_cpu(unsigned int cpu)
+static int resctrl_offline_cpu(unsigned int cpu)
 {
 	struct rdtgroup *rdtgrp;
 	struct rdt_resource *r;
@@ -866,7 +866,7 @@ static __init bool get_rdt_resources(void)
 
 static enum cpuhp_state rdt_online;
 
-static int __init intel_rdt_late_init(void)
+static int __init resctrl_late_init(void)
 {
 	struct rdt_resource *r;
 	int state, ret;
@@ -877,8 +877,8 @@ static int __init intel_rdt_late_init(void)
 	rdt_init_padding();
 
 	state = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
-				  "x86/rdt/cat:online:",
-				  intel_rdt_online_cpu, intel_rdt_offline_cpu);
+				  "x86/resctrl/cat:online:",
+				  resctrl_online_cpu, resctrl_offline_cpu);
 	if (state < 0)
 		return state;
 
@@ -890,20 +890,20 @@ static int __init intel_rdt_late_init(void)
 	rdt_online = state;
 
 	for_each_alloc_capable_rdt_resource(r)
-		pr_info("Intel RDT %s allocation detected\n", r->name);
+		pr_info("%s allocation detected\n", r->name);
 
 	for_each_mon_capable_rdt_resource(r)
-		pr_info("Intel RDT %s monitoring detected\n", r->name);
+		pr_info("%s monitoring detected\n", r->name);
 
 	return 0;
 }
 
-late_initcall(intel_rdt_late_init);
+late_initcall(resctrl_late_init);
 
-static void __exit intel_rdt_exit(void)
+static void __exit resctrl_exit(void)
 {
 	cpuhp_remove_state(rdt_online);
 	rdtgroup_exit();
 }
 
-__exitcall(intel_rdt_exit);
+__exitcall(resctrl_exit);
diff --git a/arch/x86/kernel/cpu/resctrl.h b/arch/x86/kernel/cpu/resctrl.h
index a9d906767bb2..abf5c7e4c625 100644
--- a/arch/x86/kernel/cpu/resctrl.h
+++ b/arch/x86/kernel/cpu/resctrl.h
@@ -69,7 +69,7 @@ struct rmid_read {
 	u64			val;
 };
 
-extern unsigned int intel_cqm_threshold;
+extern unsigned int resctrl_cqm_threshold;
 extern bool rdt_alloc_capable;
 extern bool rdt_mon_capable;
 extern unsigned int rdt_mon_features;
diff --git a/arch/x86/kernel/cpu/resctrl_monitor.c b/arch/x86/kernel/cpu/resctrl_monitor.c
index 211d97bcbde5..6d654f7b0eba 100644
--- a/arch/x86/kernel/cpu/resctrl_monitor.c
+++ b/arch/x86/kernel/cpu/resctrl_monitor.c
@@ -73,7 +73,7 @@ unsigned int rdt_mon_features;
  * This is the threshold cache occupancy at which we will consider an
  * RMID available for re-allocation.
  */
-unsigned int intel_cqm_threshold;
+unsigned int resctrl_cqm_threshold;
 
 static inline struct rmid_entry *__rmid_entry(u32 rmid)
 {
@@ -107,7 +107,7 @@ static bool rmid_dirty(struct rmid_entry *entry)
 {
 	u64 val = __rmid_read(entry->rmid, QOS_L3_OCCUP_EVENT_ID);
 
-	return val >= intel_cqm_threshold;
+	return val >= resctrl_cqm_threshold;
 }
 
 /*
@@ -187,7 +187,7 @@ static void add_rmid_to_limbo(struct rmid_entry *entry)
 	list_for_each_entry(d, &r->domains, list) {
 		if (cpumask_test_cpu(cpu, &d->cpu_mask)) {
 			val = __rmid_read(entry->rmid, QOS_L3_OCCUP_EVENT_ID);
-			if (val <= intel_cqm_threshold)
+			if (val <= resctrl_cqm_threshold)
 				continue;
 		}
 
@@ -625,6 +625,7 @@ static void l3_mon_evt_init(struct rdt_resource *r)
 
 int rdt_get_mon_l3_config(struct rdt_resource *r)
 {
+	unsigned int cl_size = boot_cpu_data.x86_cache_size;
 	int ret;
 
 	r->mon_scale = boot_cpu_data.x86_cache_occ_scale;
@@ -637,10 +638,10 @@ int rdt_get_mon_l3_config(struct rdt_resource *r)
 	 *
 	 * For a 35MB LLC and 56 RMIDs, this is ~1.8% of the LLC.
 	 */
-	intel_cqm_threshold = boot_cpu_data.x86_cache_size * 1024 / r->num_rmid;
+	resctrl_cqm_threshold = cl_size * 1024 / r->num_rmid;
 
 	/* h/w works in units of "boot_cpu_data.x86_cache_occ_scale" */
-	intel_cqm_threshold /= r->mon_scale;
+	resctrl_cqm_threshold /= r->mon_scale;
 
 	ret = dom_data_init(r);
 	if (ret)
diff --git a/arch/x86/kernel/cpu/resctrl_rdtgroup.c b/arch/x86/kernel/cpu/resctrl_rdtgroup.c
index 6308ba3905e5..8b6b4a8bb7ca 100644
--- a/arch/x86/kernel/cpu/resctrl_rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl_rdtgroup.c
@@ -298,7 +298,7 @@ static int rdtgroup_cpus_show(struct kernfs_open_file *of,
 }
 
 /*
- * This is safe against intel_rdt_sched_in() called from __switch_to()
+ * This is safe against resctrl_sched_in() called from __switch_to()
  * because __switch_to() is executed with interrupts disabled. A local call
  * from update_closid_rmid() is proteced against __switch_to() because
  * preemption is disabled.
@@ -317,7 +317,7 @@ static void update_cpu_closid_rmid(void *info)
 	 * executing task might have its own closid selected. Just reuse
 	 * the context switch code.
 	 */
-	intel_rdt_sched_in();
+	resctrl_sched_in();
 }
 
 /*
@@ -542,7 +542,7 @@ static void move_myself(struct callback_head *head)
 
 	preempt_disable();
 	/* update PQR_ASSOC MSR to make resource group go into effect */
-	intel_rdt_sched_in();
+	resctrl_sched_in();
 	preempt_enable();
 
 	kfree(callback);
@@ -926,7 +926,7 @@ static int max_threshold_occ_show(struct kernfs_open_file *of,
 {
 	struct rdt_resource *r = of->kn->parent->priv;
 
-	seq_printf(seq, "%u\n", intel_cqm_threshold * r->mon_scale);
+	seq_printf(seq, "%u\n", resctrl_cqm_threshold * r->mon_scale);
 
 	return 0;
 }
@@ -945,7 +945,7 @@ static ssize_t max_threshold_occ_write(struct kernfs_open_file *of,
 	if (bytes > (boot_cpu_data.x86_cache_size * 1024))
 		return -EINVAL;
 
-	intel_cqm_threshold = bytes / r->mon_scale;
+	resctrl_cqm_threshold = bytes / r->mon_scale;
 
 	return nbytes;
 }
diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index d16207e7d1e5..dc4d92764d1a 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -302,7 +302,7 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
 	this_cpu_write(current_task, next_p);
 
 	/* Load the Intel cache allocation PQR MSR. */
-	intel_rdt_sched_in();
+	resctrl_sched_in();
 
 	return prev_p;
 }
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index af73223ad6ba..5ada7354b5d4 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -536,7 +536,7 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
 	}
 
 	/* Load the Intel cache allocation PQR MSR. */
-	intel_rdt_sched_in();
+	resctrl_sched_in();
 
 	return prev_p;
 }
-- 
2.17.1

[PATCH v8 03/13] arch/resctrl: Re-arrange RDT init code

[Moger, Babu]

Separate the call sequence for rdt_quirks and MBA feature.
This is in preparation to handle vendor differences in these
call sequences.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 arch/x86/kernel/cpu/resctrl.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl.c b/arch/x86/kernel/cpu/resctrl.c
index 5d526dc45751..4cea275c7c57 100644
--- a/arch/x86/kernel/cpu/resctrl.c
+++ b/arch/x86/kernel/cpu/resctrl.c
@@ -794,6 +794,14 @@ static bool __init rdt_cpu_has(int flag)
 	return ret;
 }
 
+static __init bool rdt_mba_config(void)
+{
+	if (rdt_cpu_has(X86_FEATURE_MBA))
+		return rdt_get_mem_config(&rdt_resources_all[RDT_RESOURCE_MBA]);
+
+	return false;
+}
+
 static __init bool get_rdt_alloc_resources(void)
 {
 	bool ret = false;
@@ -818,10 +826,9 @@ static __init bool get_rdt_alloc_resources(void)
 		ret = true;
 	}
 
-	if (rdt_cpu_has(X86_FEATURE_MBA)) {
-		if (rdt_get_mem_config(&rdt_resources_all[RDT_RESOURCE_MBA]))
-			ret = true;
-	}
+	if (rdt_mba_config())
+		ret = true;
+
 	return ret;
 }
 
@@ -840,7 +847,7 @@ static __init bool get_rdt_mon_resources(void)
 	return !rdt_get_mon_l3_config(&rdt_resources_all[RDT_RESOURCE_L3]);
 }
 
-static __init void rdt_quirks(void)
+static __init void rdt_quirks_intel(void)
 {
 	switch (boot_cpu_data.x86_model) {
 	case INTEL_FAM6_HASWELL_X:
@@ -855,9 +862,14 @@ static __init void rdt_quirks(void)
 	}
 }
 
+static __init void rdt_quirks(void)
+{
+	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
+		rdt_quirks_intel();
+}
+
 static __init bool get_rdt_resources(void)
 {
-	rdt_quirks();
 	rdt_alloc_capable = get_rdt_alloc_resources();
 	rdt_mon_capable = get_rdt_mon_resources();
 
@@ -871,6 +883,9 @@ static int __init resctrl_late_init(void)
 	struct rdt_resource *r;
 	int state, ret;
 
+	/* Run quirks first */
+	rdt_quirks();
+
 	if (!get_rdt_resources())
 		return -ENODEV;
 
-- 
2.17.1

Re: [PATCH v8 03/13] arch/resctrl: Re-arrange RDT init code

[Borislav Petkov]

On Fri, Nov 16, 2018 at 08:54:26PM +0000, Moger, Babu wrote:
> Separate the call sequence for rdt_quirks and MBA feature.
> This is in preparation to handle vendor differences in these
> call sequences.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  arch/x86/kernel/cpu/resctrl.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl.c b/arch/x86/kernel/cpu/resctrl.c
> index 5d526dc45751..4cea275c7c57 100644
> --- a/arch/x86/kernel/cpu/resctrl.c
> +++ b/arch/x86/kernel/cpu/resctrl.c
> @@ -794,6 +794,14 @@ static bool __init rdt_cpu_has(int flag)
>  	return ret;
>  }

Just nitpicks:

> +static __init bool rdt_mba_config(void)

That function doesn't have a verb in its name but it needs to have one
stating what it does. You could do

mv rdt_get_mem_config() -> __rdt_get_mem_config()
mv rdt_mba_config() -> rdt_get_mem_config()

to have the hierarchy of what calls what. And then the AMD variant will
be called __rdt_get_mem_config_amd().

Also, those are all static functions so you can just as well drop the
"rdt" prefix, I'd say.

> +{
> +	if (rdt_cpu_has(X86_FEATURE_MBA))
> +		return rdt_get_mem_config(&rdt_resources_all[RDT_RESOURCE_MBA]);
> +
> +	return false;
> +}
> +
>  static __init bool get_rdt_alloc_resources(void)
>  {
>  	bool ret = false;
> @@ -818,10 +826,9 @@ static __init bool get_rdt_alloc_resources(void)
>  		ret = true;
>  	}
>  
> -	if (rdt_cpu_has(X86_FEATURE_MBA)) {
> -		if (rdt_get_mem_config(&rdt_resources_all[RDT_RESOURCE_MBA]))
> -			ret = true;
> -	}
> +	if (rdt_mba_config())
> +		ret = true;
> +
>  	return ret;
>  }
>  
> @@ -840,7 +847,7 @@ static __init bool get_rdt_mon_resources(void)
>  	return !rdt_get_mon_l3_config(&rdt_resources_all[RDT_RESOURCE_L3]);
>  }
>  
> -static __init void rdt_quirks(void)
> +static __init void rdt_quirks_intel(void)
>  {
>  	switch (boot_cpu_data.x86_model) {
>  	case INTEL_FAM6_HASWELL_X:
> @@ -855,9 +862,14 @@ static __init void rdt_quirks(void)
>  	}
>  }
>  
> +static __init void rdt_quirks(void)

Those functions also need to have a verb in the name stating what they
do.

> +{
> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
> +		rdt_quirks_intel();
> +}
> +
>  static __init bool get_rdt_resources(void)
>  {
> -	rdt_quirks();
>  	rdt_alloc_capable = get_rdt_alloc_resources();
>  	rdt_mon_capable = get_rdt_mon_resources();
>  
> @@ -871,6 +883,9 @@ static int __init resctrl_late_init(void)
>  	struct rdt_resource *r;
>  	int state, ret;
>  
> +	/* Run quirks first */
> +	rdt_quirks();

If the comment wasn't there, seeing "rdt_quirks();" doesn't say much and
makes me go look at what that function does.

> +
>  	if (!get_rdt_resources())

Unlike here, where it is clear that this gets the rdt resources.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

RE: [PATCH v8 03/13] arch/resctrl: Re-arrange RDT init code

[Moger, Babu]

Boris,

> -----Original Message-----
> From: Borislav Petkov <bp@alien8.de>
> Sent: Tuesday, November 20, 2018 3:28 AM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: tglx@linutronix.de; mingo@redhat.com; corbet@lwn.net;
> fenghua.yu@intel.com; reinette.chatre@intel.com; peterz@infradead.org;
> gregkh@linuxfoundation.org; davem@davemloft.net; akpm@linux-
> foundation.org; hpa@zytor.com; x86@kernel.org;
> mchehab+samsung@kernel.org; arnd@arndb.de;
> kstewart@linuxfoundation.org; pombredanne@nexb.com;
> rafael@kernel.org; kirill.shutemov@linux.intel.com; tony.luck@intel.com;
> qianyue.zj@alibaba-inc.com; xiaochen.shen@intel.com;
> pbonzini@redhat.com; Singh, Brijesh <brijesh.singh@amd.com>; Hurwitz,
> Sherry <sherry.hurwitz@amd.com>; dwmw2@infradead.org; Lendacky,
> Thomas <Thomas.Lendacky@amd.com>; luto@kernel.org; joro@8bytes.org;
> jannh@google.com; vkuznets@redhat.com; rian@alum.mit.edu;
> jpoimboe@redhat.com; linux-kernel@vger.kernel.org; linux-
> doc@vger.kernel.org
> Subject: Re: [PATCH v8 03/13] arch/resctrl: Re-arrange RDT init code
> 
> On Fri, Nov 16, 2018 at 08:54:26PM +0000, Moger, Babu wrote:
> > Separate the call sequence for rdt_quirks and MBA feature.
> > This is in preparation to handle vendor differences in these
> > call sequences.
> >
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > ---
> >  arch/x86/kernel/cpu/resctrl.c | 27 +++++++++++++++++++++------
> >  1 file changed, 21 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/resctrl.c b/arch/x86/kernel/cpu/resctrl.c
> > index 5d526dc45751..4cea275c7c57 100644
> > --- a/arch/x86/kernel/cpu/resctrl.c
> > +++ b/arch/x86/kernel/cpu/resctrl.c
> > @@ -794,6 +794,14 @@ static bool __init rdt_cpu_has(int flag)
> >  	return ret;
> >  }
> 
> Just nitpicks:
> 
> > +static __init bool rdt_mba_config(void)
> 
> That function doesn't have a verb in its name but it needs to have one
> stating what it does. You could do
> 
> mv rdt_get_mem_config() -> __rdt_get_mem_config()
> mv rdt_mba_config() -> rdt_get_mem_config()
> 
> to have the hierarchy of what calls what. And then the AMD variant will
> be called __rdt_get_mem_config_amd().
> 
> Also, those are all static functions so you can just as well drop the
> "rdt" prefix, I'd say.

Ok. Sure. Will take care of these.

> 
> > +{
> > +	if (rdt_cpu_has(X86_FEATURE_MBA))
> > +		return
> rdt_get_mem_config(&rdt_resources_all[RDT_RESOURCE_MBA]);
> > +
> > +	return false;
> > +}
> > +
> >  static __init bool get_rdt_alloc_resources(void)
> >  {
> >  	bool ret = false;
> > @@ -818,10 +826,9 @@ static __init bool get_rdt_alloc_resources(void)
> >  		ret = true;
> >  	}
> >
> > -	if (rdt_cpu_has(X86_FEATURE_MBA)) {
> > -		if
> (rdt_get_mem_config(&rdt_resources_all[RDT_RESOURCE_MBA]))
> > -			ret = true;
> > -	}
> > +	if (rdt_mba_config())
> > +		ret = true;
> > +
> >  	return ret;
> >  }
> >
> > @@ -840,7 +847,7 @@ static __init bool get_rdt_mon_resources(void)
> >  	return
> !rdt_get_mon_l3_config(&rdt_resources_all[RDT_RESOURCE_L3]);
> >  }
> >
> > -static __init void rdt_quirks(void)
> > +static __init void rdt_quirks_intel(void)
> >  {
> >  	switch (boot_cpu_data.x86_model) {
> >  	case INTEL_FAM6_HASWELL_X:
> > @@ -855,9 +862,14 @@ static __init void rdt_quirks(void)
> >  	}
> >  }
> >
> > +static __init void rdt_quirks(void)
> 
> Those functions also need to have a verb in the name stating what they
> do.

Ok. Will change it to rdt_check_quirks.

> 
> > +{
> > +	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
> > +		rdt_quirks_intel();
> > +}
> > +
> >  static __init bool get_rdt_resources(void)
> >  {
> > -	rdt_quirks();
> >  	rdt_alloc_capable = get_rdt_alloc_resources();
> >  	rdt_mon_capable = get_rdt_mon_resources();
> >
> > @@ -871,6 +883,9 @@ static int __init resctrl_late_init(void)
> >  	struct rdt_resource *r;
> >  	int state, ret;
> >
> > +	/* Run quirks first */
> > +	rdt_quirks();
> 
> If the comment wasn't there, seeing "rdt_quirks();" doesn't say much and
> makes me go look at what that function does.

Will remove the comment and rename the function to rdt_check_quirks().

> 
> > +
> >  	if (!get_rdt_resources())
> 
> Unlike here, where it is clear that this gets the rdt resources.
> 
> Thx.
> 
> --
> Regards/Gruss,
>     Boris.
> 
> Good mailing practices for 400: avoid top-posting and trim the reply.

[PATCH v8 04/13] arch/resctrl: Bring all the macros to resctrl.h

[Moger, Babu]

Bring all the macros to resctrl.h and rename for consistency.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 arch/x86/kernel/cpu/resctrl.c         | 3 ---
 arch/x86/kernel/cpu/resctrl.h         | 5 +++++
 arch/x86/kernel/cpu/resctrl_monitor.c | 7 ++-----
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl.c b/arch/x86/kernel/cpu/resctrl.c
index 4cea275c7c57..18c8222f326c 100644
--- a/arch/x86/kernel/cpu/resctrl.c
+++ b/arch/x86/kernel/cpu/resctrl.c
@@ -33,9 +33,6 @@
 #include <asm/resctrl_sched.h>
 #include "resctrl.h"
 
-#define MBA_IS_LINEAR	0x4
-#define MBA_MAX_MBPS	U32_MAX
-
 /* Mutex to protect rdtgroup access. */
 DEFINE_MUTEX(rdtgroup_mutex);
 
diff --git a/arch/x86/kernel/cpu/resctrl.h b/arch/x86/kernel/cpu/resctrl.h
index abf5c7e4c625..e5f7bf6a8d09 100644
--- a/arch/x86/kernel/cpu/resctrl.h
+++ b/arch/x86/kernel/cpu/resctrl.h
@@ -12,6 +12,9 @@
 #define IA32_L2_CBM_BASE	0xd10
 #define IA32_MBA_THRTL_BASE	0xd50
 
+#define IA32_QM_CTR		0x0c8e
+#define IA32_QM_EVTSEL		0x0c8d
+
 #define L3_QOS_CDP_ENABLE	0x01ULL
 
 #define L2_QOS_CDP_ENABLE	0x01ULL
@@ -29,6 +32,8 @@
 #define MBM_CNTR_WIDTH			24
 #define MBM_OVERFLOW_INTERVAL		1000
 #define MAX_MBA_BW			100u
+#define MBA_IS_LINEAR			0x4
+#define MBA_MAX_MBPS			U32_MAX
 
 #define RMID_VAL_ERROR			BIT_ULL(63)
 #define RMID_VAL_UNAVAIL		BIT_ULL(62)
diff --git a/arch/x86/kernel/cpu/resctrl_monitor.c b/arch/x86/kernel/cpu/resctrl_monitor.c
index 6d654f7b0eba..9fd62263dabd 100644
--- a/arch/x86/kernel/cpu/resctrl_monitor.c
+++ b/arch/x86/kernel/cpu/resctrl_monitor.c
@@ -28,9 +28,6 @@
 #include <asm/cpu_device_id.h>
 #include "resctrl.h"
 
-#define MSR_IA32_QM_CTR		0x0c8e
-#define MSR_IA32_QM_EVTSEL		0x0c8d
-
 struct rmid_entry {
 	u32				rmid;
 	int				busy;
@@ -97,8 +94,8 @@ static u64 __rmid_read(u32 rmid, u32 eventid)
 	 * IA32_QM_CTR.Error (bit 63) and IA32_QM_CTR.Unavailable (bit 62)
 	 * are error bits.
 	 */
-	wrmsr(MSR_IA32_QM_EVTSEL, eventid, rmid);
-	rdmsrl(MSR_IA32_QM_CTR, val);
+	wrmsr(IA32_QM_EVTSEL, eventid, rmid);
+	rdmsrl(IA32_QM_CTR, val);
 
 	return val;
 }
-- 
2.17.1

Re: [PATCH v8 04/13] arch/resctrl: Bring all the macros to resctrl.h

[Borislav Petkov]

On Fri, Nov 16, 2018 at 08:54:28PM +0000, Moger, Babu wrote:
> diff --git a/arch/x86/kernel/cpu/resctrl_monitor.c b/arch/x86/kernel/cpu/resctrl_monitor.c
> index 6d654f7b0eba..9fd62263dabd 100644
> --- a/arch/x86/kernel/cpu/resctrl_monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl_monitor.c
> @@ -28,9 +28,6 @@
>  #include <asm/cpu_device_id.h>
>  #include "resctrl.h"
>  
> -#define MSR_IA32_QM_CTR		0x0c8e
> -#define MSR_IA32_QM_EVTSEL		0x0c8d
> -
>  struct rmid_entry {
>  	u32				rmid;
>  	int				busy;
> @@ -97,8 +94,8 @@ static u64 __rmid_read(u32 rmid, u32 eventid)
>  	 * IA32_QM_CTR.Error (bit 63) and IA32_QM_CTR.Unavailable (bit 62)
>  	 * are error bits.
>  	 */
> -	wrmsr(MSR_IA32_QM_EVTSEL, eventid, rmid);
> -	rdmsrl(MSR_IA32_QM_CTR, val);
> +	wrmsr(IA32_QM_EVTSEL, eventid, rmid);
> +	rdmsrl(IA32_QM_CTR, val);

Well, if you have a look at arch/x86/include/asm/msr-index.h, you'll see
that all MSR defines are prefixed with "MSR_" so dropping that prefix
here loses information and staring at the define doesn't tell me what it
is.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

RE: [PATCH v8 04/13] arch/resctrl: Bring all the macros to resctrl.h

[Moger, Babu]

Boris,

> -----Original Message-----
> From: Borislav Petkov <bp@alien8.de>
> Sent: Tuesday, November 20, 2018 3:36 AM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: tglx@linutronix.de; mingo@redhat.com; corbet@lwn.net;
> fenghua.yu@intel.com; reinette.chatre@intel.com; peterz@infradead.org;
> gregkh@linuxfoundation.org; davem@davemloft.net; akpm@linux-
> foundation.org; hpa@zytor.com; x86@kernel.org;
> mchehab+samsung@kernel.org; arnd@arndb.de;
> kstewart@linuxfoundation.org; pombredanne@nexb.com;
> rafael@kernel.org; kirill.shutemov@linux.intel.com; tony.luck@intel.com;
> qianyue.zj@alibaba-inc.com; xiaochen.shen@intel.com;
> pbonzini@redhat.com; Singh, Brijesh <brijesh.singh@amd.com>; Hurwitz,
> Sherry <sherry.hurwitz@amd.com>; dwmw2@infradead.org; Lendacky,
> Thomas <Thomas.Lendacky@amd.com>; luto@kernel.org; joro@8bytes.org;
> jannh@google.com; vkuznets@redhat.com; rian@alum.mit.edu;
> jpoimboe@redhat.com; linux-kernel@vger.kernel.org; linux-
> doc@vger.kernel.org
> Subject: Re: [PATCH v8 04/13] arch/resctrl: Bring all the macros to resctrl.h
> 
> On Fri, Nov 16, 2018 at 08:54:28PM +0000, Moger, Babu wrote:
> > diff --git a/arch/x86/kernel/cpu/resctrl_monitor.c
> b/arch/x86/kernel/cpu/resctrl_monitor.c
> > index 6d654f7b0eba..9fd62263dabd 100644
> > --- a/arch/x86/kernel/cpu/resctrl_monitor.c
> > +++ b/arch/x86/kernel/cpu/resctrl_monitor.c
> > @@ -28,9 +28,6 @@
> >  #include <asm/cpu_device_id.h>
> >  #include "resctrl.h"
> >
> > -#define MSR_IA32_QM_CTR		0x0c8e
> > -#define MSR_IA32_QM_EVTSEL		0x0c8d
> > -
> >  struct rmid_entry {
> >  	u32				rmid;
> >  	int				busy;
> > @@ -97,8 +94,8 @@ static u64 __rmid_read(u32 rmid, u32 eventid)
> >  	 * IA32_QM_CTR.Error (bit 63) and IA32_QM_CTR.Unavailable (bit 62)
> >  	 * are error bits.
> >  	 */
> > -	wrmsr(MSR_IA32_QM_EVTSEL, eventid, rmid);
> > -	rdmsrl(MSR_IA32_QM_CTR, val);
> > +	wrmsr(IA32_QM_EVTSEL, eventid, rmid);
> > +	rdmsrl(IA32_QM_CTR, val);
> 
> Well, if you have a look at arch/x86/include/asm/msr-index.h, you'll see
> that all MSR defines are prefixed with "MSR_" so dropping that prefix
> here loses information and staring at the define doesn't tell me what it
> is.

Ok. Sure. Will retain MSR_.

> 
> --
> Regards/Gruss,
>     Boris.
> 
> Good mailing practices for 400: avoid top-posting and trim the reply.

[PATCH v8 05/13] arch/resctrl: Rename config parameter INTEL_RDT to RESCTRL

[Moger, Babu]

Now Resource control feature is supported by both Intel and AMD.
Rename the INTEL_RDT to vendor neutral RESCTRL.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 arch/x86/Kconfig                     | 4 ++--
 arch/x86/include/asm/resctrl_sched.h | 4 ++--
 arch/x86/kernel/cpu/Makefile         | 4 ++--
 include/linux/sched.h                | 2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 1a0be022f91d..4aae7aba4d61 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -445,8 +445,8 @@ config RETPOLINE
 	  code are eliminated. Since this includes the syscall entry path,
 	  it is not entirely pointless.
 
-config INTEL_RDT
-	bool "Intel Resource Director Technology support"
+config RESCTRL
+	bool "Resource Control feature support"
 	default n
 	depends on X86 && CPU_SUP_INTEL
 	select KERNFS
diff --git a/arch/x86/include/asm/resctrl_sched.h b/arch/x86/include/asm/resctrl_sched.h
index 6e082697a613..54990fe2a3ae 100644
--- a/arch/x86/include/asm/resctrl_sched.h
+++ b/arch/x86/include/asm/resctrl_sched.h
@@ -2,7 +2,7 @@
 #ifndef _ASM_X86_RESCTRL_SCHED_H
 #define _ASM_X86_RESCTRL_SCHED_H
 
-#ifdef CONFIG_INTEL_RDT
+#ifdef CONFIG_RESCTRL
 
 #include <linux/sched.h>
 #include <linux/jump_label.h>
@@ -88,6 +88,6 @@ static inline void resctrl_sched_in(void)
 
 static inline void resctrl_sched_in(void) {}
 
-#endif /* CONFIG_INTEL_RDT */
+#endif /* CONFIG_RESCTRL */
 
 #endif /* _ASM_X86_RESCTRL_SCHED_H */
diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
index 222cf8cc078d..79279953c5f9 100644
--- a/arch/x86/kernel/cpu/Makefile
+++ b/arch/x86/kernel/cpu/Makefile
@@ -35,8 +35,8 @@ obj-$(CONFIG_CPU_SUP_CENTAUR)		+= centaur.o
 obj-$(CONFIG_CPU_SUP_TRANSMETA_32)	+= transmeta.o
 obj-$(CONFIG_CPU_SUP_UMC_32)		+= umc.o
 
-obj-$(CONFIG_INTEL_RDT)	+= resctrl.o resctrl_rdtgroup.o resctrl_monitor.o
-obj-$(CONFIG_INTEL_RDT)	+= resctrl_ctrlmondata.o resctrl_pseudo_lock.o
+obj-$(CONFIG_RESCTRL)	+= resctrl.o resctrl_rdtgroup.o resctrl_monitor.o
+obj-$(CONFIG_RESCTRL)	+= resctrl_ctrlmondata.o resctrl_pseudo_lock.o
 CFLAGS_resctrl_pseudo_lock.o = -I$(src)
 
 obj-$(CONFIG_X86_MCE)			+= mcheck/
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 977cb57d7bc9..c4cf94c447b2 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -985,7 +985,7 @@ struct task_struct {
 	/* cg_list protected by css_set_lock and tsk->alloc_lock: */
 	struct list_head		cg_list;
 #endif
-#ifdef CONFIG_INTEL_RDT
+#ifdef CONFIG_RESCTRL
 	u32				closid;
 	u32				rmid;
 #endif
-- 
2.17.1

Re: [PATCH v8 05/13] arch/resctrl: Rename config parameter INTEL_RDT to RESCTRL

[Borislav Petkov]

On Fri, Nov 16, 2018 at 08:54:30PM +0000, Moger, Babu wrote:
> Now Resource control feature is supported by both Intel and AMD.
> Rename the INTEL_RDT to vendor neutral RESCTRL.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  arch/x86/Kconfig                     | 4 ++--
>  arch/x86/include/asm/resctrl_sched.h | 4 ++--
>  arch/x86/kernel/cpu/Makefile         | 4 ++--
>  include/linux/sched.h                | 2 +-
>  4 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 1a0be022f91d..4aae7aba4d61 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -445,8 +445,8 @@ config RETPOLINE
>  	  code are eliminated. Since this includes the syscall entry path,
>  	  it is not entirely pointless.
>  
> -config INTEL_RDT
> -	bool "Intel Resource Director Technology support"
> +config RESCTRL
> +	bool "Resource Control feature support"

s/feature // - they're all features. :)

>  	default n
>  	depends on X86 && CPU_SUP_INTEL

Patch 9/13 is begging to be merged with this one.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

RE: [PATCH v8 05/13] arch/resctrl: Rename config parameter INTEL_RDT to RESCTRL

[Moger, Babu]

Boris,

> -----Original Message-----
> From: Borislav Petkov <bp@alien8.de>
> Sent: Tuesday, November 20, 2018 3:42 AM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: tglx@linutronix.de; mingo@redhat.com; corbet@lwn.net;
> fenghua.yu@intel.com; reinette.chatre@intel.com; peterz@infradead.org;
> gregkh@linuxfoundation.org; davem@davemloft.net; akpm@linux-
> foundation.org; hpa@zytor.com; x86@kernel.org;
> mchehab+samsung@kernel.org; arnd@arndb.de;
> kstewart@linuxfoundation.org; pombredanne@nexb.com;
> rafael@kernel.org; kirill.shutemov@linux.intel.com; tony.luck@intel.com;
> qianyue.zj@alibaba-inc.com; xiaochen.shen@intel.com;
> pbonzini@redhat.com; Singh, Brijesh <brijesh.singh@amd.com>; Hurwitz,
> Sherry <sherry.hurwitz@amd.com>; dwmw2@infradead.org; Lendacky,
> Thomas <Thomas.Lendacky@amd.com>; luto@kernel.org; joro@8bytes.org;
> jannh@google.com; vkuznets@redhat.com; rian@alum.mit.edu;
> jpoimboe@redhat.com; linux-kernel@vger.kernel.org; linux-
> doc@vger.kernel.org
> Subject: Re: [PATCH v8 05/13] arch/resctrl: Rename config parameter
> INTEL_RDT to RESCTRL
> 
> On Fri, Nov 16, 2018 at 08:54:30PM +0000, Moger, Babu wrote:
> > Now Resource control feature is supported by both Intel and AMD.
> > Rename the INTEL_RDT to vendor neutral RESCTRL.
> >
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > ---
> >  arch/x86/Kconfig                     | 4 ++--
> >  arch/x86/include/asm/resctrl_sched.h | 4 ++--
> >  arch/x86/kernel/cpu/Makefile         | 4 ++--
> >  include/linux/sched.h                | 2 +-
> >  4 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index 1a0be022f91d..4aae7aba4d61 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -445,8 +445,8 @@ config RETPOLINE
> >  	  code are eliminated. Since this includes the syscall entry path,
> >  	  it is not entirely pointless.
> >
> > -config INTEL_RDT
> > -	bool "Intel Resource Director Technology support"
> > +config RESCTRL
> > +	bool "Resource Control feature support"
> 
> s/feature // - they're all features. :)

Ok sure
> 
> >  	default n
> >  	depends on X86 && CPU_SUP_INTEL
> 
> Patch 9/13 is begging to be merged with this one.

Ok. I will merge these changes in patch 9/13.

> 
> --
> Regards/Gruss,
>     Boris.
> 
> Good mailing practices for 400: avoid top-posting and trim the reply.

[PATCH v8 06/13] arch/resctrl: Initialize the resource functions that are different

[Moger, Babu]

Initialize the resource functions that are different between the
vendors. Some features are initialized differently between the vendors.
Add _intel suffix to Intel specific functions.

For example, MBA feature varies significantly between Intel and AMD.
Separate the initialization of these resource functions. That way we
can easily add AMD's functions later.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 arch/x86/kernel/cpu/resctrl.c             | 34 +++++++++++++++++++----
 arch/x86/kernel/cpu/resctrl.h             |  8 ++++--
 arch/x86/kernel/cpu/resctrl_ctrlmondata.c |  4 +--
 3 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl.c b/arch/x86/kernel/cpu/resctrl.c
index 18c8222f326c..eeb7e0e4883e 100644
--- a/arch/x86/kernel/cpu/resctrl.c
+++ b/arch/x86/kernel/cpu/resctrl.c
@@ -57,7 +57,8 @@ int max_name_width, max_data_width;
 bool rdt_alloc_capable;
 
 static void
-mba_wrmsr(struct rdt_domain *d, struct msr_param *m, struct rdt_resource *r);
+mba_wrmsr_intel(struct rdt_domain *d, struct msr_param *m,
+		struct rdt_resource *r);
 static void
 cat_wrmsr(struct rdt_domain *d, struct msr_param *m, struct rdt_resource *r);
 
@@ -171,10 +172,7 @@ struct rdt_resource rdt_resources_all[] = {
 		.rid			= RDT_RESOURCE_MBA,
 		.name			= "MB",
 		.domains		= domain_init(RDT_RESOURCE_MBA),
-		.msr_base		= IA32_MBA_THRTL_BASE,
-		.msr_update		= mba_wrmsr,
 		.cache_level		= 3,
-		.parse_ctrlval		= parse_bw,
 		.format_str		= "%d=%*u",
 		.fflags			= RFTYPE_RES_MB,
 	},
@@ -356,7 +354,8 @@ u32 delay_bw_map(unsigned long bw, struct rdt_resource *r)
 }
 
 static void
-mba_wrmsr(struct rdt_domain *d, struct msr_param *m, struct rdt_resource *r)
+mba_wrmsr_intel(struct rdt_domain *d, struct msr_param *m,
+		struct rdt_resource *r)
 {
 	unsigned int i;
 
@@ -873,6 +872,25 @@ static __init bool get_rdt_resources(void)
 	return (rdt_mon_capable || rdt_alloc_capable);
 }
 
+static __init void rdt_init_res_defs_intel(void)
+{
+	struct rdt_resource *r;
+
+	for_each_rdt_resource(r) {
+		if (r->rid == RDT_RESOURCE_MBA) {
+			r->msr_base = IA32_MBA_THRTL_BASE;
+			r->msr_update = mba_wrmsr_intel;
+			r->parse_ctrlval = parse_bw_intel;
+		}
+	}
+}
+
+static __init void rdt_init_res_defs(void)
+{
+	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
+		rdt_init_res_defs_intel();
+}
+
 static enum cpuhp_state rdt_online;
 
 static int __init resctrl_late_init(void)
@@ -880,6 +898,12 @@ static int __init resctrl_late_init(void)
 	struct rdt_resource *r;
 	int state, ret;
 
+	/*
+	 * Initialize functions(or definitions) that are different
+	 * between vendors here.
+	 */
+	rdt_init_res_defs();
+
 	/* Run quirks first */
 	rdt_quirks();
 
diff --git a/arch/x86/kernel/cpu/resctrl.h b/arch/x86/kernel/cpu/resctrl.h
index e5f7bf6a8d09..8731b7c91c28 100644
--- a/arch/x86/kernel/cpu/resctrl.h
+++ b/arch/x86/kernel/cpu/resctrl.h
@@ -444,8 +444,8 @@ struct rdt_resource {
 
 int parse_cbm(struct rdt_parse_data *data, struct rdt_resource *r,
 	      struct rdt_domain *d);
-int parse_bw(struct rdt_parse_data *data, struct rdt_resource *r,
-	     struct rdt_domain *d);
+int parse_bw_intel(struct rdt_parse_data *data, struct rdt_resource *r,
+		   struct rdt_domain *d);
 
 extern struct mutex rdtgroup_mutex;
 
@@ -468,6 +468,10 @@ enum {
 	RDT_NUM_RESOURCES,
 };
 
+#define for_each_rdt_resource(r)					      \
+	for (r = rdt_resources_all; r < rdt_resources_all + RDT_NUM_RESOURCES;\
+	     r++)
+
 #define for_each_capable_rdt_resource(r)				      \
 	for (r = rdt_resources_all; r < rdt_resources_all + RDT_NUM_RESOURCES;\
 	     r++)							      \
diff --git a/arch/x86/kernel/cpu/resctrl_ctrlmondata.c b/arch/x86/kernel/cpu/resctrl_ctrlmondata.c
index e8d21306d0f9..78089ae8683c 100644
--- a/arch/x86/kernel/cpu/resctrl_ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl_ctrlmondata.c
@@ -64,8 +64,8 @@ static bool bw_validate(char *buf, unsigned long *data, struct rdt_resource *r)
 	return true;
 }
 
-int parse_bw(struct rdt_parse_data *data, struct rdt_resource *r,
-	     struct rdt_domain *d)
+int parse_bw_intel(struct rdt_parse_data *data, struct rdt_resource *r,
+		   struct rdt_domain *d)
 {
 	unsigned long bw_val;
 
-- 
2.17.1

Re: [PATCH v8 06/13] arch/resctrl: Initialize the resource functions that are different

[Borislav Petkov]

On Fri, Nov 16, 2018 at 08:54:32PM +0000, Moger, Babu wrote:
> Initialize the resource functions that are different between the
> vendors. Some features are initialized differently between the vendors.
> Add _intel suffix to Intel specific functions.
> 
> For example, MBA feature varies significantly between Intel and AMD.
> Separate the initialization of these resource functions. That way we
> can easily add AMD's functions later.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  arch/x86/kernel/cpu/resctrl.c             | 34 +++++++++++++++++++----
>  arch/x86/kernel/cpu/resctrl.h             |  8 ++++--
>  arch/x86/kernel/cpu/resctrl_ctrlmondata.c |  4 +--
>  3 files changed, 37 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl.c b/arch/x86/kernel/cpu/resctrl.c
> index 18c8222f326c..eeb7e0e4883e 100644
> --- a/arch/x86/kernel/cpu/resctrl.c
> +++ b/arch/x86/kernel/cpu/resctrl.c
> @@ -57,7 +57,8 @@ int max_name_width, max_data_width;
>  bool rdt_alloc_capable;
>  
>  static void
> -mba_wrmsr(struct rdt_domain *d, struct msr_param *m, struct rdt_resource *r);
> +mba_wrmsr_intel(struct rdt_domain *d, struct msr_param *m,
> +		struct rdt_resource *r);
>  static void
>  cat_wrmsr(struct rdt_domain *d, struct msr_param *m, struct rdt_resource *r);
>  
> @@ -171,10 +172,7 @@ struct rdt_resource rdt_resources_all[] = {
>  		.rid			= RDT_RESOURCE_MBA,
>  		.name			= "MB",
>  		.domains		= domain_init(RDT_RESOURCE_MBA),
> -		.msr_base		= IA32_MBA_THRTL_BASE,
> -		.msr_update		= mba_wrmsr,
>  		.cache_level		= 3,
> -		.parse_ctrlval		= parse_bw,
>  		.format_str		= "%d=%*u",
>  		.fflags			= RFTYPE_RES_MB,
>  	},
> @@ -356,7 +354,8 @@ u32 delay_bw_map(unsigned long bw, struct rdt_resource *r)
>  }
>  
>  static void
> -mba_wrmsr(struct rdt_domain *d, struct msr_param *m, struct rdt_resource *r)
> +mba_wrmsr_intel(struct rdt_domain *d, struct msr_param *m,
> +		struct rdt_resource *r)
>  {
>  	unsigned int i;
>  
> @@ -873,6 +872,25 @@ static __init bool get_rdt_resources(void)
>  	return (rdt_mon_capable || rdt_alloc_capable);
>  }
>  
> +static __init void rdt_init_res_defs_intel(void)
> +{
> +	struct rdt_resource *r;
> +
> +	for_each_rdt_resource(r) {
> +		if (r->rid == RDT_RESOURCE_MBA) {
> +			r->msr_base = IA32_MBA_THRTL_BASE;
> +			r->msr_update = mba_wrmsr_intel;
> +			r->parse_ctrlval = parse_bw_intel;
> +		}
> +	}
> +}
> +
> +static __init void rdt_init_res_defs(void)
> +{
> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
> +		rdt_init_res_defs_intel();
> +}

So I'm wondering: instead of having mba_wrmsr_intel() and
mba_wrmsr_amd() and adding those per-vendor initialization functions,
why don't you push down the vendor differentiation into mba_wrmsr()?

Then in that function you do

	if (vendor == X86_VENDOR_INTEL)
		__mba_wrmsr_intel();
	else if (vendor == X86_VENDOR_AMD)
		__mba_wrmsr_amd();

and so on and then you don't have to do any of that initialization dance
here and the struct rdt_resource assignment for the MBA will remain
nicely similar to the other ones...

Hmmm?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v8 06/13] arch/resctrl: Initialize the resource functions that are different

[Borislav Petkov]

On Tue, Nov 20, 2018 at 10:59:18AM +0100, Borislav Petkov wrote:
> So I'm wondering: instead of having mba_wrmsr_intel() and
> mba_wrmsr_amd() and adding those per-vendor initialization functions,
> why don't you push down the vendor differentiation into mba_wrmsr()?
> 
> Then in that function you do
> 
> 	if (vendor == X86_VENDOR_INTEL)
> 		__mba_wrmsr_intel();
> 	else if (vendor == X86_VENDOR_AMD)
> 		__mba_wrmsr_amd();
> 
> and so on and then you don't have to do any of that initialization dance
> here and the struct rdt_resource assignment for the MBA will remain
> nicely similar to the other ones...
> 
> Hmmm?

Yeah, after having look at the patchset further, that might not be a
good idea as you need to assign more per-vendor stuff than just an MSR
accessor function...

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

RE: [PATCH v8 06/13] arch/resctrl: Initialize the resource functions that are different

[Moger, Babu]

> -----Original Message-----
> From: Borislav Petkov <bp@alien8.de>
> Sent: Tuesday, November 20, 2018 6:05 AM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: tglx@linutronix.de; mingo@redhat.com; corbet@lwn.net;
> fenghua.yu@intel.com; reinette.chatre@intel.com; peterz@infradead.org;
> gregkh@linuxfoundation.org; davem@davemloft.net; akpm@linux-
> foundation.org; hpa@zytor.com; x86@kernel.org;
> mchehab+samsung@kernel.org; arnd@arndb.de;
> kstewart@linuxfoundation.org; pombredanne@nexb.com;
> rafael@kernel.org; kirill.shutemov@linux.intel.com; tony.luck@intel.com;
> qianyue.zj@alibaba-inc.com; xiaochen.shen@intel.com;
> pbonzini@redhat.com; Singh, Brijesh <brijesh.singh@amd.com>; Hurwitz,
> Sherry <sherry.hurwitz@amd.com>; dwmw2@infradead.org; Lendacky,
> Thomas <Thomas.Lendacky@amd.com>; luto@kernel.org; joro@8bytes.org;
> jannh@google.com; vkuznets@redhat.com; rian@alum.mit.edu;
> jpoimboe@redhat.com; linux-kernel@vger.kernel.org; linux-
> doc@vger.kernel.org
> Subject: Re: [PATCH v8 06/13] arch/resctrl: Initialize the resource functions
> that are different
> 
> On Tue, Nov 20, 2018 at 10:59:18AM +0100, Borislav Petkov wrote:
> > So I'm wondering: instead of having mba_wrmsr_intel() and
> > mba_wrmsr_amd() and adding those per-vendor initialization functions,
> > why don't you push down the vendor differentiation into mba_wrmsr()?
> >
> > Then in that function you do
> >
> > 	if (vendor == X86_VENDOR_INTEL)
> > 		__mba_wrmsr_intel();
> > 	else if (vendor == X86_VENDOR_AMD)
> > 		__mba_wrmsr_amd();
> >
> > and so on and then you don't have to do any of that initialization dance
> > here and the struct rdt_resource assignment for the MBA will remain
> > nicely similar to the other ones...
> >
> > Hmmm?
> 
> Yeah, after having look at the patchset further, that might not be a
> good idea as you need to assign more per-vendor stuff than just an MSR
> accessor function...

Ok. No changes here. Thanks

> 
> --
> Regards/Gruss,
>     Boris.
> 
> Good mailing practices for 400: avoid top-posting and trim the reply.

[PATCH v8 07/13] arch/resctrl: Bring cbm_validate function into the resource structure

[Moger, Babu]

Idea is to bring all the functions that are different between the
vendors into resource structure and initialize them dynamically.
Add _intel suffix to Intel specific functions.

Following function is implemented separately for each vendors.
cbm_validate  : Cache bitmask validate function. AMD allows
                non-contiguous masks. So, use separate functions for
                Intel and AMD.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 arch/x86/kernel/cpu/resctrl.c             |  9 ++++++++-
 arch/x86/kernel/cpu/resctrl.h             | 11 +++++++----
 arch/x86/kernel/cpu/resctrl_ctrlmondata.c |  4 ++--
 3 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl.c b/arch/x86/kernel/cpu/resctrl.c
index eeb7e0e4883e..3f26c7c114e7 100644
--- a/arch/x86/kernel/cpu/resctrl.c
+++ b/arch/x86/kernel/cpu/resctrl.c
@@ -877,7 +877,14 @@ static __init void rdt_init_res_defs_intel(void)
 	struct rdt_resource *r;
 
 	for_each_rdt_resource(r) {
-		if (r->rid == RDT_RESOURCE_MBA) {
+		if (r->rid == RDT_RESOURCE_L3 ||
+		    r->rid == RDT_RESOURCE_L3DATA ||
+		    r->rid == RDT_RESOURCE_L3CODE ||
+		    r->rid == RDT_RESOURCE_L2 ||
+		    r->rid == RDT_RESOURCE_L2DATA ||
+		    r->rid == RDT_RESOURCE_L2CODE)
+			r->cbm_validate = cbm_validate_intel;
+		else if (r->rid == RDT_RESOURCE_MBA) {
 			r->msr_base = IA32_MBA_THRTL_BASE;
 			r->msr_update = mba_wrmsr_intel;
 			r->parse_ctrlval = parse_bw_intel;
diff --git a/arch/x86/kernel/cpu/resctrl.h b/arch/x86/kernel/cpu/resctrl.h
index 8731b7c91c28..102bcffbefd7 100644
--- a/arch/x86/kernel/cpu/resctrl.h
+++ b/arch/x86/kernel/cpu/resctrl.h
@@ -410,10 +410,11 @@ struct rdt_parse_data {
  * @cache:		Cache allocation related data
  * @format_str:		Per resource format string to show domain value
  * @parse_ctrlval:	Per resource function pointer to parse control values
- * @evt_list:			List of monitoring events
- * @num_rmid:			Number of RMIDs available
- * @mon_scale:			cqm counter * mon_scale = occupancy in bytes
- * @fflags:			flags to choose base and info files
+ * @cbm_validate	Cache bitmask validate function
+ * @evt_list:		List of monitoring events
+ * @num_rmid:		Number of RMIDs available
+ * @mon_scale:		cqm counter * mon_scale = occupancy in bytes
+ * @fflags:		flags to choose base and info files
  */
 struct rdt_resource {
 	int			rid;
@@ -436,6 +437,7 @@ struct rdt_resource {
 	int (*parse_ctrlval)(struct rdt_parse_data *data,
 			     struct rdt_resource *r,
 			     struct rdt_domain *d);
+	bool (*cbm_validate)(char *buf, u32 *data, struct rdt_resource *r);
 	struct list_head	evt_list;
 	int			num_rmid;
 	unsigned int		mon_scale;
@@ -576,5 +578,6 @@ void cqm_setup_limbo_handler(struct rdt_domain *dom, unsigned long delay_ms);
 void cqm_handle_limbo(struct work_struct *work);
 bool has_busy_rmid(struct rdt_resource *r, struct rdt_domain *d);
 void __check_limbo(struct rdt_domain *d, bool force_free);
+bool cbm_validate_intel(char *buf, u32 *data, struct rdt_resource *r);
 
 #endif /* _ASM_X86_RESCTRL_H */
diff --git a/arch/x86/kernel/cpu/resctrl_ctrlmondata.c b/arch/x86/kernel/cpu/resctrl_ctrlmondata.c
index 78089ae8683c..71aa1d971430 100644
--- a/arch/x86/kernel/cpu/resctrl_ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl_ctrlmondata.c
@@ -88,7 +88,7 @@ int parse_bw_intel(struct rdt_parse_data *data, struct rdt_resource *r,
  *	are allowed (e.g. FFFFH, 0FF0H, 003CH, etc.).
  * Additionally Haswell requires at least two bits set.
  */
-static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
+bool cbm_validate_intel(char *buf, u32 *data, struct rdt_resource *r)
 {
 	unsigned long first_bit, zero_bit, val;
 	unsigned int cbm_len = r->cache.cbm_len;
@@ -148,7 +148,7 @@ int parse_cbm(struct rdt_parse_data *data, struct rdt_resource *r,
 		return -EINVAL;
 	}
 
-	if (!cbm_validate(data->buf, &cbm_val, r))
+	if (r->cbm_validate && !r->cbm_validate(data->buf, &cbm_val, r))
 		return -EINVAL;
 
 	if ((rdtgrp->mode == RDT_MODE_EXCLUSIVE ||
-- 
2.17.1

[PATCH v8 08/13] arch/resctrl: Add vendor check for MBA software controller

[Moger, Babu]

MBA software controller support is available only on Intel.

Suggested-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 arch/x86/kernel/cpu/resctrl_rdtgroup.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/resctrl_rdtgroup.c b/arch/x86/kernel/cpu/resctrl_rdtgroup.c
index 8b6b4a8bb7ca..022a56096024 100644
--- a/arch/x86/kernel/cpu/resctrl_rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl_rdtgroup.c
@@ -1878,7 +1878,10 @@ static int parse_rdtgroupfs_options(char *data)
 			if (ret)
 				goto out;
 		} else if (!strcmp(token, "mba_MBps")) {
-			ret = set_mba_sc(true);
+			if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
+				ret = set_mba_sc(true);
+			else
+				ret = -EINVAL;
 			if (ret)
 				goto out;
 		} else {
-- 
2.17.1

[PATCH v8 09/13] arch/resctrl: Update the RESCTRL config parameter

[Moger, Babu]

CONFIG_RESCTRL will be used for both Intel and AMD to enable
Resource Control feature. Update the details and condition accordingly.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 arch/x86/Kconfig | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 4aae7aba4d61..c5ef3afa89a6 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -448,13 +448,22 @@ config RETPOLINE
 config RESCTRL
 	bool "Resource Control feature support"
 	default n
-	depends on X86 && CPU_SUP_INTEL
+	depends on X86 && (CPU_SUP_INTEL || CPU_SUP_AMD)
 	select KERNFS
 	help
-	  Select to enable resource allocation and monitoring which are
-	  sub-features of Intel Resource Director Technology(RDT). More
-	  information about RDT can be found in the Intel x86
-	  Architecture Software Developer Manual.
+	  Select to enable Resource Control feature support.
+
+	  These features are intended to provide the support for the allocation
+	  and monitoring the usage of certain system resources by one or more
+	  processors.
+
+	  Intel refers this feature as Intel Resource Director Technology
+	  (Intel(R) RDT). More information about RDT can be found in the
+	  Intel x86 Architecture Software Developer Manual.
+
+	  AMD refers this feature as AMD Platform Quality of Service(AMD QoS).
+	  More information about AMD QoS can be found in AMD64 Technology
+	  Platform Quality of Service Extensions manual.
 
 	  Say N if unsure.
 
-- 
2.17.1

Re: [PATCH v8 09/13] arch/resctrl: Update the RESCTRL config parameter

[Randy Dunlap]

On 11/16/18 12:54 PM, Moger, Babu wrote:
> CONFIG_RESCTRL will be used for both Intel and AMD to enable
> Resource Control feature. Update the details and condition accordingly.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  arch/x86/Kconfig | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 4aae7aba4d61..c5ef3afa89a6 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -448,13 +448,22 @@ config RETPOLINE
>  config RESCTRL
>  	bool "Resource Control feature support"
>  	default n
> -	depends on X86 && CPU_SUP_INTEL
> +	depends on X86 && (CPU_SUP_INTEL || CPU_SUP_AMD)
>  	select KERNFS
>  	help
> -	  Select to enable resource allocation and monitoring which are
> -	  sub-features of Intel Resource Director Technology(RDT). More
> -	  information about RDT can be found in the Intel x86
> -	  Architecture Software Developer Manual.
> +	  Select to enable Resource Control feature support.
> +
> +	  These features are intended to provide the support for the allocation
> +	  and monitoring the usage of certain system resources by one or more

	  and monitoring of the usage

> +	  processors.
> +
> +	  Intel refers this feature as Intel Resource Director Technology

	        refers to this feature

> +	  (Intel(R) RDT). More information about RDT can be found in the
> +	  Intel x86 Architecture Software Developer Manual.
> +
> +	  AMD refers this feature as AMD Platform Quality of Service(AMD QoS).

	      refers to this feature

> +	  More information about AMD QoS can be found in AMD64 Technology
> +	  Platform Quality of Service Extensions manual.
>  
>  	  Say N if unsure.
>  
> 


-- 
~Randy

RE: [PATCH v8 09/13] arch/resctrl: Update the RESCTRL config parameter

[Moger, Babu]

Randy,

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org <linux-kernel-
> owner@vger.kernel.org> On Behalf Of Randy Dunlap
> Sent: Friday, November 16, 2018 3:18 PM
> To: Moger, Babu <Babu.Moger@amd.com>; tglx@linutronix.de;
> mingo@redhat.com; bp@alien8.de; corbet@lwn.net;
> fenghua.yu@intel.com; reinette.chatre@intel.com; peterz@infradead.org;
> gregkh@linuxfoundation.org; davem@davemloft.net; akpm@linux-
> foundation.org
> Cc: hpa@zytor.com; x86@kernel.org; mchehab+samsung@kernel.org;
> arnd@arndb.de; kstewart@linuxfoundation.org; pombredanne@nexb.com;
> rafael@kernel.org; kirill.shutemov@linux.intel.com; tony.luck@intel.com;
> qianyue.zj@alibaba-inc.com; xiaochen.shen@intel.com;
> pbonzini@redhat.com; Singh, Brijesh <brijesh.singh@amd.com>; Hurwitz,
> Sherry <sherry.hurwitz@amd.com>; dwmw2@infradead.org; Lendacky,
> Thomas <Thomas.Lendacky@amd.com>; luto@kernel.org; joro@8bytes.org;
> jannh@google.com; vkuznets@redhat.com; rian@alum.mit.edu;
> jpoimboe@redhat.com; linux-kernel@vger.kernel.org; linux-
> doc@vger.kernel.org
> Subject: Re: [PATCH v8 09/13] arch/resctrl: Update the RESCTRL config
> parameter
> 
> On 11/16/18 12:54 PM, Moger, Babu wrote:
> > CONFIG_RESCTRL will be used for both Intel and AMD to enable
> > Resource Control feature. Update the details and condition accordingly.
> >
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > ---
> >  arch/x86/Kconfig | 19 ++++++++++++++-----
> >  1 file changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index 4aae7aba4d61..c5ef3afa89a6 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -448,13 +448,22 @@ config RETPOLINE
> >  config RESCTRL
> >  	bool "Resource Control feature support"
> >  	default n
> > -	depends on X86 && CPU_SUP_INTEL
> > +	depends on X86 && (CPU_SUP_INTEL || CPU_SUP_AMD)
> >  	select KERNFS
> >  	help
> > -	  Select to enable resource allocation and monitoring which are
> > -	  sub-features of Intel Resource Director Technology(RDT). More
> > -	  information about RDT can be found in the Intel x86
> > -	  Architecture Software Developer Manual.
> > +	  Select to enable Resource Control feature support.
> > +
> > +	  These features are intended to provide the support for the
> allocation
> > +	  and monitoring the usage of certain system resources by one or
> more
> 
> 	  and monitoring of the usage
 
Sure. Will take care of this next revision.
> 
> > +	  processors.
> > +
> > +	  Intel refers this feature as Intel Resource Director Technology
> 
> 	        refers to this feature

Yes. Will take care.

> 
> > +	  (Intel(R) RDT). More information about RDT can be found in the
> > +	  Intel x86 Architecture Software Developer Manual.
> > +
> > +	  AMD refers this feature as AMD Platform Quality of Service(AMD
> QoS).
> 
> 	      refers to this feature

Sure. Will fix. Thanks for the comments.

> 
> > +	  More information about AMD QoS can be found in AMD64
> Technology
> > +	  Platform Quality of Service Extensions manual.
> >
> >  	  Say N if unsure.
> >
> >
> 
> 
> --
> ~Randy

[PATCH v8 10/13] arch/resctrl: Add AMD feature bit X86_FEATURE_MBA in cpuid bits array

[Moger, Babu]

From: Sherry Hurwitz <sherry.hurwitz@amd.com>

The feature bit X86_FEATURE_MBA is detected via CPUID leaf 0x80000008
EBX Bit 06. This bit indicates the support of AMD's MBA feature.

This feature is supported by both Intel and AMD. But they are detected
in different CPUID leaves.

Signed-off-by: Sherry Hurwitz <sherry.hurwitz@amd.com>
Signed-off-by: Babu Moger <babu.moger@amd.com>
Reviewed-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/kernel/cpu/scattered.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
index 772c219b6889..bd7853334b27 100644
--- a/arch/x86/kernel/cpu/scattered.c
+++ b/arch/x86/kernel/cpu/scattered.c
@@ -17,7 +17,11 @@ struct cpuid_bit {
 	u32 sub_leaf;
 };
 
-/* Please keep the leaf sorted by cpuid_bit.level for faster search. */
+/*
+ * Please keep the leaf sorted by cpuid_bit.level for faster search.
+ * X86_FEATURE_MBA supported by both Intel and AMD. But the cpuid
+ * levels are different. Add a separate enty for each.
+ */
 static const struct cpuid_bit cpuid_bits[] = {
 	{ X86_FEATURE_APERFMPERF,       CPUID_ECX,  0, 0x00000006, 0 },
 	{ X86_FEATURE_EPB,		CPUID_ECX,  3, 0x00000006, 0 },
@@ -29,6 +33,7 @@ static const struct cpuid_bit cpuid_bits[] = {
 	{ X86_FEATURE_HW_PSTATE,	CPUID_EDX,  7, 0x80000007, 0 },
 	{ X86_FEATURE_CPB,		CPUID_EDX,  9, 0x80000007, 0 },
 	{ X86_FEATURE_PROC_FEEDBACK,    CPUID_EDX, 11, 0x80000007, 0 },
+	{ X86_FEATURE_MBA,		CPUID_EBX,  6, 0x80000008, 0 },
 	{ X86_FEATURE_SME,		CPUID_EAX,  0, 0x8000001f, 0 },
 	{ X86_FEATURE_SEV,		CPUID_EAX,  1, 0x8000001f, 0 },
 	{ 0, 0, 0, 0, 0 }
-- 
2.17.1

[PATCH v8 11/13] arch/resctrl: Introduce QOS feature for AMD

[Moger, Babu]

Enables QOS feature on AMD.
Following QoS sub-features are supported in AMD if the underlying
hardware supports it.
 - L3 Cache allocation enforcement
 - L3 Cache occupancy monitoring
 - L3 Code-Data Prioritization support
 - Memory Bandwidth Enforcement(Allocation)

The specification for this feature is available at
https://developer.amd.com/wp-content/resources/56375.pdf

There are differences in the way some of the features are implemented.
Separate those functions and add those as vendor specific functions.
The major difference is in MBA feature.
 - AMD uses CPUID leaf 0x80000020 to initialize the MBA features.
 - AMD uses direct bandwidth value instead of delay based on bandwidth
   values.
 - MSR register base addresses are different for MBA.
 - Also AMD allows non-contiguous L3 cache bit masks.

Adds following functions to take care of the differences.
rdt_get_mem_config_amd : MBA initialization function
parse_bw_amd : Bandwidth parsing
mba_wrmsr_amd: Writes bandwidth value
cbm_validate_amd : L3 cache bitmask validation

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 arch/x86/kernel/cpu/resctrl.c             | 68 +++++++++++++++++++++-
 arch/x86/kernel/cpu/resctrl.h             |  5 ++
 arch/x86/kernel/cpu/resctrl_ctrlmondata.c | 70 +++++++++++++++++++++++
 3 files changed, 141 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl.c b/arch/x86/kernel/cpu/resctrl.c
index 3f26c7c114e7..0d700ab7fcf9 100644
--- a/arch/x86/kernel/cpu/resctrl.c
+++ b/arch/x86/kernel/cpu/resctrl.c
@@ -61,6 +61,9 @@ mba_wrmsr_intel(struct rdt_domain *d, struct msr_param *m,
 		struct rdt_resource *r);
 static void
 cat_wrmsr(struct rdt_domain *d, struct msr_param *m, struct rdt_resource *r);
+static void
+mba_wrmsr_amd(struct rdt_domain *d, struct msr_param *m,
+	      struct rdt_resource *r);
 
 #define domain_init(id) LIST_HEAD_INIT(rdt_resources_all[id].domains)
 
@@ -280,6 +283,31 @@ static bool rdt_get_mem_config(struct rdt_resource *r)
 	return true;
 }
 
+static bool rdt_get_mem_config_amd(struct rdt_resource *r)
+{
+	union cpuid_0x10_3_eax eax;
+	union cpuid_0x10_x_edx edx;
+	u32 ebx, ecx;
+
+	cpuid_count(0x80000020, 1, &eax.full, &ebx, &ecx, &edx.full);
+	r->num_closid = edx.split.cos_max + 1;
+	r->default_ctrl = MAX_MBA_BW_AMD;
+
+	/* AMD does not use delay. Set delay_linear to false by default */
+	r->membw.delay_linear = false;
+
+	/* FIX ME - May need to be read from MSR */
+	r->membw.min_bw = 0;
+	r->membw.bw_gran = 1;
+	/* Max value is 2048, Data width should be 4 in decimal */
+	r->data_width = 4;
+
+	r->alloc_capable = true;
+	r->alloc_enabled = true;
+
+	return true;
+}
+
 static void rdt_get_cache_alloc_cfg(int idx, struct rdt_resource *r)
 {
 	union cpuid_0x10_1_eax eax;
@@ -339,6 +367,16 @@ static int get_cache_id(int cpu, int level)
 	return -1;
 }
 
+static void
+mba_wrmsr_amd(struct rdt_domain *d, struct msr_param *m, struct rdt_resource *r)
+{
+	unsigned int i;
+
+	/*  Write the bw values for mba. */
+	for (i = m->low; i < m->high; i++)
+		wrmsrl(r->msr_base + i, d->ctrl_val[i]);
+}
+
 /*
  * Map the memory b/w percentage value to delay values
  * that can be written to QOS_MSRs.
@@ -792,8 +830,12 @@ static bool __init rdt_cpu_has(int flag)
 
 static __init bool rdt_mba_config(void)
 {
-	if (rdt_cpu_has(X86_FEATURE_MBA))
-		return rdt_get_mem_config(&rdt_resources_all[RDT_RESOURCE_MBA]);
+	if (rdt_cpu_has(X86_FEATURE_MBA)) {
+		if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
+			return rdt_get_mem_config(&rdt_resources_all[RDT_RESOURCE_MBA]);
+		else if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
+			return rdt_get_mem_config_amd(&rdt_resources_all[RDT_RESOURCE_MBA]);
+	}
 
 	return false;
 }
@@ -892,10 +934,32 @@ static __init void rdt_init_res_defs_intel(void)
 	}
 }
 
+static __init void rdt_init_res_defs_amd(void)
+{
+	struct rdt_resource *r;
+
+	for_each_rdt_resource(r) {
+		if (r->rid == RDT_RESOURCE_L3 ||
+		    r->rid == RDT_RESOURCE_L3DATA ||
+		    r->rid == RDT_RESOURCE_L3CODE ||
+		    r->rid == RDT_RESOURCE_L2 ||
+		    r->rid == RDT_RESOURCE_L2DATA ||
+		    r->rid == RDT_RESOURCE_L2CODE)
+			r->cbm_validate = cbm_validate_amd;
+		else if (r->rid == RDT_RESOURCE_MBA) {
+			r->msr_base = IA32_MBA_BW_BASE;
+			r->msr_update = mba_wrmsr_amd;
+			r->parse_ctrlval = parse_bw_amd;
+		}
+	}
+}
+
 static __init void rdt_init_res_defs(void)
 {
 	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
 		rdt_init_res_defs_intel();
+	else if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
+		rdt_init_res_defs_amd();
 }
 
 static enum cpuhp_state rdt_online;
diff --git a/arch/x86/kernel/cpu/resctrl.h b/arch/x86/kernel/cpu/resctrl.h
index 102bcffbefd7..54ba21b7de2c 100644
--- a/arch/x86/kernel/cpu/resctrl.h
+++ b/arch/x86/kernel/cpu/resctrl.h
@@ -11,6 +11,7 @@
 #define IA32_L3_CBM_BASE	0xc90
 #define IA32_L2_CBM_BASE	0xd10
 #define IA32_MBA_THRTL_BASE	0xd50
+#define IA32_MBA_BW_BASE	0xc0000200
 
 #define IA32_QM_CTR		0x0c8e
 #define IA32_QM_EVTSEL		0x0c8d
@@ -34,6 +35,7 @@
 #define MAX_MBA_BW			100u
 #define MBA_IS_LINEAR			0x4
 #define MBA_MAX_MBPS			U32_MAX
+#define MAX_MBA_BW_AMD			0x800
 
 #define RMID_VAL_ERROR			BIT_ULL(63)
 #define RMID_VAL_UNAVAIL		BIT_ULL(62)
@@ -448,6 +450,8 @@ int parse_cbm(struct rdt_parse_data *data, struct rdt_resource *r,
 	      struct rdt_domain *d);
 int parse_bw_intel(struct rdt_parse_data *data, struct rdt_resource *r,
 		   struct rdt_domain *d);
+int parse_bw_amd(struct rdt_parse_data *data, struct rdt_resource *r,
+		 struct rdt_domain *d);
 
 extern struct mutex rdtgroup_mutex;
 
@@ -579,5 +583,6 @@ void cqm_handle_limbo(struct work_struct *work);
 bool has_busy_rmid(struct rdt_resource *r, struct rdt_domain *d);
 void __check_limbo(struct rdt_domain *d, bool force_free);
 bool cbm_validate_intel(char *buf, u32 *data, struct rdt_resource *r);
+bool cbm_validate_amd(char *buf, u32 *data, struct rdt_resource *r);
 
 #endif /* _ASM_X86_RESCTRL_H */
diff --git a/arch/x86/kernel/cpu/resctrl_ctrlmondata.c b/arch/x86/kernel/cpu/resctrl_ctrlmondata.c
index 71aa1d971430..b6ceb4db9322 100644
--- a/arch/x86/kernel/cpu/resctrl_ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl_ctrlmondata.c
@@ -28,6 +28,52 @@
 #include <linux/slab.h>
 #include "resctrl.h"
 
+/*
+ * Check whether MBA bandwidth percentage value is correct. The value is
+ * checked against the minimum and max bandwidth values specified by the
+ * hardware. The allocated bandwidth percentage is rounded to the next
+ * control step available on the hardware.
+ */
+static bool bw_validate_amd(char *buf, unsigned long *data,
+			    struct rdt_resource *r)
+{
+	unsigned long bw;
+	int ret;
+
+	ret = kstrtoul(buf, 10, &bw);
+	if (ret) {
+		rdt_last_cmd_printf("Non-decimal digit in MB value %s\n", buf);
+		return false;
+	}
+
+	if (bw < r->membw.min_bw || bw > r->default_ctrl) {
+		rdt_last_cmd_printf("MB value %ld out of range [%d,%d]\n", bw,
+				    r->membw.min_bw, r->default_ctrl);
+		return false;
+	}
+
+	*data = roundup(bw, (unsigned long)r->membw.bw_gran);
+	return true;
+}
+
+int parse_bw_amd(struct rdt_parse_data *data, struct rdt_resource *r,
+		 struct rdt_domain *d)
+{
+	unsigned long bw_val;
+
+	if (d->have_new_ctrl) {
+		rdt_last_cmd_printf("duplicate domain %d\n", d->id);
+		return -EINVAL;
+	}
+
+	if (!bw_validate_amd(data->buf, &bw_val, r))
+		return -EINVAL;
+	d->new_ctrl = bw_val;
+	d->have_new_ctrl = true;
+
+	return 0;
+}
+
 /*
  * Check whether MBA bandwidth percentage value is correct. The value is
  * checked against the minimum and max bandwidth values specified by the
@@ -123,6 +169,30 @@ bool cbm_validate_intel(char *buf, u32 *data, struct rdt_resource *r)
 	return true;
 }
 
+/*
+ * Check whether a cache bit mask is valid. AMD allows non-contiguous
+ * bitmasks
+ */
+bool cbm_validate_amd(char *buf, u32 *data, struct rdt_resource *r)
+{
+	unsigned long val;
+	int ret;
+
+	ret = kstrtoul(buf, 16, &val);
+	if (ret) {
+		rdt_last_cmd_printf("non-hex character in mask %s\n", buf);
+		return false;
+	}
+
+	if (val > r->default_ctrl) {
+		rdt_last_cmd_puts("mask out of range\n");
+		return false;
+	}
+
+	*data = val;
+	return true;
+}
+
 /*
  * Read one cache bit mask (hex). Check that it is valid for the current
  * resource type.
-- 
2.17.1

Re: [PATCH v8 11/13] arch/resctrl: Introduce QOS feature for AMD

[Borislav Petkov]

On Fri, Nov 16, 2018 at 08:54:43PM +0000, Moger, Babu wrote:
> Enables QOS feature on AMD.

From Documentation/process/submitting-patches.rst:

"Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
to do frotz", as if you are giving orders to the codebase to change
its behaviour."

> Following QoS sub-features are supported in AMD if the underlying
> hardware supports it.
>  - L3 Cache allocation enforcement
>  - L3 Cache occupancy monitoring
>  - L3 Code-Data Prioritization support
>  - Memory Bandwidth Enforcement(Allocation)
> 
> The specification for this feature is available at
> https://developer.amd.com/wp-content/resources/56375.pdf

I hope that URL is stable.

> There are differences in the way some of the features are implemented.
> Separate those functions and add those as vendor specific functions.
> The major difference is in MBA feature.
>  - AMD uses CPUID leaf 0x80000020 to initialize the MBA features.
>  - AMD uses direct bandwidth value instead of delay based on bandwidth
>    values.
>  - MSR register base addresses are different for MBA.
>  - Also AMD allows non-contiguous L3 cache bit masks.
> 
> Adds following functions to take care of the differences.
> rdt_get_mem_config_amd : MBA initialization function
> parse_bw_amd : Bandwidth parsing
> mba_wrmsr_amd: Writes bandwidth value
> cbm_validate_amd : L3 cache bitmask validation

This paragraph is not needed - what you do is visible in the patch
itself.

> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  arch/x86/kernel/cpu/resctrl.c             | 68 +++++++++++++++++++++-
>  arch/x86/kernel/cpu/resctrl.h             |  5 ++
>  arch/x86/kernel/cpu/resctrl_ctrlmondata.c | 70 +++++++++++++++++++++++
>  3 files changed, 141 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl.c b/arch/x86/kernel/cpu/resctrl.c
> index 3f26c7c114e7..0d700ab7fcf9 100644
> --- a/arch/x86/kernel/cpu/resctrl.c
> +++ b/arch/x86/kernel/cpu/resctrl.c
> @@ -61,6 +61,9 @@ mba_wrmsr_intel(struct rdt_domain *d, struct msr_param *m,
>  		struct rdt_resource *r);
>  static void
>  cat_wrmsr(struct rdt_domain *d, struct msr_param *m, struct rdt_resource *r);
> +static void
> +mba_wrmsr_amd(struct rdt_domain *d, struct msr_param *m,
> +	      struct rdt_resource *r);
>  
>  #define domain_init(id) LIST_HEAD_INIT(rdt_resources_all[id].domains)
>  
> @@ -280,6 +283,31 @@ static bool rdt_get_mem_config(struct rdt_resource *r)
>  	return true;
>  }
>  
> +static bool rdt_get_mem_config_amd(struct rdt_resource *r)
> +{
> +	union cpuid_0x10_3_eax eax;
> +	union cpuid_0x10_x_edx edx;
> +	u32 ebx, ecx;
> +
> +	cpuid_count(0x80000020, 1, &eax.full, &ebx, &ecx, &edx.full);
> +	r->num_closid = edx.split.cos_max + 1;
> +	r->default_ctrl = MAX_MBA_BW_AMD;
> +
> +	/* AMD does not use delay. Set delay_linear to false by default */

You don't need to write in the comment *what* you do - that's obvious.
"AMD does not use delay" is more than enough.

> +	r->membw.delay_linear = false;
> +
> +	/* FIX ME - May need to be read from MSR */

FIX ME?

> +	r->membw.min_bw = 0;
> +	r->membw.bw_gran = 1;
> +	/* Max value is 2048, Data width should be 4 in decimal */
> +	r->data_width = 4;
> +
> +	r->alloc_capable = true;
> +	r->alloc_enabled = true;
> +
> +	return true;
> +}
> +
>  static void rdt_get_cache_alloc_cfg(int idx, struct rdt_resource *r)
>  {
>  	union cpuid_0x10_1_eax eax;
> @@ -339,6 +367,16 @@ static int get_cache_id(int cpu, int level)
>  	return -1;
>  }
>  
> +static void
> +mba_wrmsr_amd(struct rdt_domain *d, struct msr_param *m, struct rdt_resource *r)
> +{
> +	unsigned int i;
> +
> +	/*  Write the bw values for mba. */

That's an obvious comment. Drop it.

> +	for (i = m->low; i < m->high; i++)
> +		wrmsrl(r->msr_base + i, d->ctrl_val[i]);
> +}
> +
>  /*
>   * Map the memory b/w percentage value to delay values
>   * that can be written to QOS_MSRs.
> @@ -792,8 +830,12 @@ static bool __init rdt_cpu_has(int flag)
>  
>  static __init bool rdt_mba_config(void)
>  {
> -	if (rdt_cpu_has(X86_FEATURE_MBA))
> -		return rdt_get_mem_config(&rdt_resources_all[RDT_RESOURCE_MBA]);
> +	if (rdt_cpu_has(X86_FEATURE_MBA)) {

Save yourself an indentation level:

	if (!rdt_cpu_has(X86_FEATURE_MBA))
		return false;

	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
		...


> +			return rdt_get_mem_config(&rdt_resources_all[RDT_RESOURCE_MBA]);
> +		else if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
> +			return rdt_get_mem_config_amd(&rdt_resources_all[RDT_RESOURCE_MBA]);
> +	}
>  
>  	return false;
>  }

...

> diff --git a/arch/x86/kernel/cpu/resctrl.h b/arch/x86/kernel/cpu/resctrl.h
> index 102bcffbefd7..54ba21b7de2c 100644
> --- a/arch/x86/kernel/cpu/resctrl.h
> +++ b/arch/x86/kernel/cpu/resctrl.h
> @@ -11,6 +11,7 @@
>  #define IA32_L3_CBM_BASE	0xc90
>  #define IA32_L2_CBM_BASE	0xd10
>  #define IA32_MBA_THRTL_BASE	0xd50
> +#define IA32_MBA_BW_BASE	0xc0000200

	MSR_...

>  
>  #define IA32_QM_CTR		0x0c8e
>  #define IA32_QM_EVTSEL		0x0c8d
> @@ -34,6 +35,7 @@
>  #define MAX_MBA_BW			100u
>  #define MBA_IS_LINEAR			0x4
>  #define MBA_MAX_MBPS			U32_MAX
> +#define MAX_MBA_BW_AMD			0x800
>  
>  #define RMID_VAL_ERROR			BIT_ULL(63)
>  #define RMID_VAL_UNAVAIL		BIT_ULL(62)
> @@ -448,6 +450,8 @@ int parse_cbm(struct rdt_parse_data *data, struct rdt_resource *r,
>  	      struct rdt_domain *d);
>  int parse_bw_intel(struct rdt_parse_data *data, struct rdt_resource *r,
>  		   struct rdt_domain *d);
> +int parse_bw_amd(struct rdt_parse_data *data, struct rdt_resource *r,
> +		 struct rdt_domain *d);
>  
>  extern struct mutex rdtgroup_mutex;
>  
> @@ -579,5 +583,6 @@ void cqm_handle_limbo(struct work_struct *work);
>  bool has_busy_rmid(struct rdt_resource *r, struct rdt_domain *d);
>  void __check_limbo(struct rdt_domain *d, bool force_free);
>  bool cbm_validate_intel(char *buf, u32 *data, struct rdt_resource *r);
> +bool cbm_validate_amd(char *buf, u32 *data, struct rdt_resource *r);
>  
>  #endif /* _ASM_X86_RESCTRL_H */
> diff --git a/arch/x86/kernel/cpu/resctrl_ctrlmondata.c b/arch/x86/kernel/cpu/resctrl_ctrlmondata.c
> index 71aa1d971430..b6ceb4db9322 100644
> --- a/arch/x86/kernel/cpu/resctrl_ctrlmondata.c
> +++ b/arch/x86/kernel/cpu/resctrl_ctrlmondata.c
> @@ -28,6 +28,52 @@
>  #include <linux/slab.h>
>  #include "resctrl.h"
>  
> +/*
> + * Check whether MBA bandwidth percentage value is correct. The value is
> + * checked against the minimum and max bandwidth values specified by the

Either write "min" and "max" or write them both out fully.

> + * hardware. The allocated bandwidth percentage is rounded to the next
> + * control step available on the hardware.
> + */
> +static bool bw_validate_amd(char *buf, unsigned long *data,
> +			    struct rdt_resource *r)
> +{
> +	unsigned long bw;
> +	int ret;
> +
> +	ret = kstrtoul(buf, 10, &bw);
> +	if (ret) {
> +		rdt_last_cmd_printf("Non-decimal digit in MB value %s\n", buf);
> +		return false;
> +	}
> +
> +	if (bw < r->membw.min_bw || bw > r->default_ctrl) {
> +		rdt_last_cmd_printf("MB value %ld out of range [%d,%d]\n", bw,
> +				    r->membw.min_bw, r->default_ctrl);
> +		return false;
> +	}
> +
> +	*data = roundup(bw, (unsigned long)r->membw.bw_gran);
> +	return true;
> +}
> +
> +int parse_bw_amd(struct rdt_parse_data *data, struct rdt_resource *r,
> +		 struct rdt_domain *d)
> +{
> +	unsigned long bw_val;
> +
> +	if (d->have_new_ctrl) {
> +		rdt_last_cmd_printf("duplicate domain %d\n", d->id);

Start with a capital letter: "Duplicate domain..."

And looking at the rest, some of them start with a capital letter and
some of them not. Please unify that in a separate patch.

> +		return -EINVAL;
> +	}
> +
> +	if (!bw_validate_amd(data->buf, &bw_val, r))
> +		return -EINVAL;

<---- newline here.

> +	d->new_ctrl = bw_val;
> +	d->have_new_ctrl = true;
> +
> +	return 0;
> +}
> +
>  /*
>   * Check whether MBA bandwidth percentage value is correct. The value is
>   * checked against the minimum and max bandwidth values specified by the

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

RE: [PATCH v8 11/13] arch/resctrl: Introduce QOS feature for AMD

[Moger, Babu]

Boris,

> -----Original Message-----
> From: Borislav Petkov <bp@alien8.de>
> Sent: Tuesday, November 20, 2018 6:16 AM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: tglx@linutronix.de; mingo@redhat.com; corbet@lwn.net;
> fenghua.yu@intel.com; reinette.chatre@intel.com; peterz@infradead.org;
> gregkh@linuxfoundation.org; davem@davemloft.net; akpm@linux-
> foundation.org; hpa@zytor.com; x86@kernel.org;
> mchehab+samsung@kernel.org; arnd@arndb.de;
> kstewart@linuxfoundation.org; pombredanne@nexb.com;
> rafael@kernel.org; kirill.shutemov@linux.intel.com; tony.luck@intel.com;
> qianyue.zj@alibaba-inc.com; xiaochen.shen@intel.com;
> pbonzini@redhat.com; Singh, Brijesh <brijesh.singh@amd.com>; Hurwitz,
> Sherry <sherry.hurwitz@amd.com>; dwmw2@infradead.org; Lendacky,
> Thomas <Thomas.Lendacky@amd.com>; luto@kernel.org; joro@8bytes.org;
> jannh@google.com; vkuznets@redhat.com; rian@alum.mit.edu;
> jpoimboe@redhat.com; linux-kernel@vger.kernel.org; linux-
> doc@vger.kernel.org
> Subject: Re: [PATCH v8 11/13] arch/resctrl: Introduce QOS feature for AMD
> 
> On Fri, Nov 16, 2018 at 08:54:43PM +0000, Moger, Babu wrote:
> > Enables QOS feature on AMD.
> 
> From Documentation/process/submitting-patches.rst:
> 
> "Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
> instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
> to do frotz", as if you are giving orders to the codebase to change
> its behaviour."

Ok. Sure.
 
> > Following QoS sub-features are supported in AMD if the underlying
> > hardware supports it.
> >  - L3 Cache allocation enforcement
> >  - L3 Cache occupancy monitoring
> >  - L3 Code-Data Prioritization support
> >  - Memory Bandwidth Enforcement(Allocation)
> >
> > The specification for this feature is available at
> > https://developer.amd.com/wp-content/resources/56375.pdf
> 
> I hope that URL is stable.
Yes. It is.
> 
> > There are differences in the way some of the features are implemented.
> > Separate those functions and add those as vendor specific functions.
> > The major difference is in MBA feature.
> >  - AMD uses CPUID leaf 0x80000020 to initialize the MBA features.
> >  - AMD uses direct bandwidth value instead of delay based on bandwidth
> >    values.
> >  - MSR register base addresses are different for MBA.
> >  - Also AMD allows non-contiguous L3 cache bit masks.
> >
> > Adds following functions to take care of the differences.
> > rdt_get_mem_config_amd : MBA initialization function
> > parse_bw_amd : Bandwidth parsing
> > mba_wrmsr_amd: Writes bandwidth value
> > cbm_validate_amd : L3 cache bitmask validation
> 
> This paragraph is not needed - what you do is visible in the patch
> itself.
Ok. Will remove.
> 
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > ---
> >  arch/x86/kernel/cpu/resctrl.c             | 68 +++++++++++++++++++++-
> >  arch/x86/kernel/cpu/resctrl.h             |  5 ++
> >  arch/x86/kernel/cpu/resctrl_ctrlmondata.c | 70
> +++++++++++++++++++++++
> >  3 files changed, 141 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/resctrl.c b/arch/x86/kernel/cpu/resctrl.c
> > index 3f26c7c114e7..0d700ab7fcf9 100644
> > --- a/arch/x86/kernel/cpu/resctrl.c
> > +++ b/arch/x86/kernel/cpu/resctrl.c
> > @@ -61,6 +61,9 @@ mba_wrmsr_intel(struct rdt_domain *d, struct
> msr_param *m,
> >  		struct rdt_resource *r);
> >  static void
> >  cat_wrmsr(struct rdt_domain *d, struct msr_param *m, struct
> rdt_resource *r);
> > +static void
> > +mba_wrmsr_amd(struct rdt_domain *d, struct msr_param *m,
> > +	      struct rdt_resource *r);
> >
> >  #define domain_init(id) LIST_HEAD_INIT(rdt_resources_all[id].domains)
> >
> > @@ -280,6 +283,31 @@ static bool rdt_get_mem_config(struct
> rdt_resource *r)
> >  	return true;
> >  }
> >
> > +static bool rdt_get_mem_config_amd(struct rdt_resource *r)
> > +{
> > +	union cpuid_0x10_3_eax eax;
> > +	union cpuid_0x10_x_edx edx;
> > +	u32 ebx, ecx;
> > +
> > +	cpuid_count(0x80000020, 1, &eax.full, &ebx, &ecx, &edx.full);
> > +	r->num_closid = edx.split.cos_max + 1;
> > +	r->default_ctrl = MAX_MBA_BW_AMD;
> > +
> > +	/* AMD does not use delay. Set delay_linear to false by default */
> 
> You don't need to write in the comment *what* you do - that's obvious.
> "AMD does not use delay" is more than enough.

Ok.

> 
> > +	r->membw.delay_linear = false;
> > +
> > +	/* FIX ME - May need to be read from MSR */
> 
> FIX ME?
We don’t need fix me here. We are not going to read from MSR.  I will drop it.
> 
> > +	r->membw.min_bw = 0;
> > +	r->membw.bw_gran = 1;
> > +	/* Max value is 2048, Data width should be 4 in decimal */
> > +	r->data_width = 4;
> > +
> > +	r->alloc_capable = true;
> > +	r->alloc_enabled = true;
> > +
> > +	return true;
> > +}
> > +
> >  static void rdt_get_cache_alloc_cfg(int idx, struct rdt_resource *r)
> >  {
> >  	union cpuid_0x10_1_eax eax;
> > @@ -339,6 +367,16 @@ static int get_cache_id(int cpu, int level)
> >  	return -1;
> >  }
> >
> > +static void
> > +mba_wrmsr_amd(struct rdt_domain *d, struct msr_param *m, struct
> rdt_resource *r)
> > +{
> > +	unsigned int i;
> > +
> > +	/*  Write the bw values for mba. */
> 
> That's an obvious comment. Drop it.
Ok.
> 
> > +	for (i = m->low; i < m->high; i++)
> > +		wrmsrl(r->msr_base + i, d->ctrl_val[i]);
> > +}
> > +
> >  /*
> >   * Map the memory b/w percentage value to delay values
> >   * that can be written to QOS_MSRs.
> > @@ -792,8 +830,12 @@ static bool __init rdt_cpu_has(int flag)
> >
> >  static __init bool rdt_mba_config(void)
> >  {
> > -	if (rdt_cpu_has(X86_FEATURE_MBA))
> > -		return
> rdt_get_mem_config(&rdt_resources_all[RDT_RESOURCE_MBA]);
> > +	if (rdt_cpu_has(X86_FEATURE_MBA)) {
> 
> Save yourself an indentation level:
Ok.
> 
> 	if (!rdt_cpu_has(X86_FEATURE_MBA))
> 		return false;
> 
> 	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
> 		...
Ok.  Got it.
> 
> 
> > +			return
> rdt_get_mem_config(&rdt_resources_all[RDT_RESOURCE_MBA]);
> > +		else if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
> > +			return
> rdt_get_mem_config_amd(&rdt_resources_all[RDT_RESOURCE_MBA]);
> > +	}
> >
> >  	return false;
> >  }
> 
> ...
> 
> > diff --git a/arch/x86/kernel/cpu/resctrl.h b/arch/x86/kernel/cpu/resctrl.h
> > index 102bcffbefd7..54ba21b7de2c 100644
> > --- a/arch/x86/kernel/cpu/resctrl.h
> > +++ b/arch/x86/kernel/cpu/resctrl.h
> > @@ -11,6 +11,7 @@
> >  #define IA32_L3_CBM_BASE	0xc90
> >  #define IA32_L2_CBM_BASE	0xd10
> >  #define IA32_MBA_THRTL_BASE	0xd50
> > +#define IA32_MBA_BW_BASE	0xc0000200
> 
> 	MSR_...
Ok.
> 
> >
> >  #define IA32_QM_CTR		0x0c8e
> >  #define IA32_QM_EVTSEL		0x0c8d
> > @@ -34,6 +35,7 @@
> >  #define MAX_MBA_BW			100u
> >  #define MBA_IS_LINEAR			0x4
> >  #define MBA_MAX_MBPS			U32_MAX
> > +#define MAX_MBA_BW_AMD			0x800
> >
> >  #define RMID_VAL_ERROR			BIT_ULL(63)
> >  #define RMID_VAL_UNAVAIL		BIT_ULL(62)
> > @@ -448,6 +450,8 @@ int parse_cbm(struct rdt_parse_data *data, struct
> rdt_resource *r,
> >  	      struct rdt_domain *d);
> >  int parse_bw_intel(struct rdt_parse_data *data, struct rdt_resource *r,
> >  		   struct rdt_domain *d);
> > +int parse_bw_amd(struct rdt_parse_data *data, struct rdt_resource *r,
> > +		 struct rdt_domain *d);
> >
> >  extern struct mutex rdtgroup_mutex;
> >
> > @@ -579,5 +583,6 @@ void cqm_handle_limbo(struct work_struct *work);
> >  bool has_busy_rmid(struct rdt_resource *r, struct rdt_domain *d);
> >  void __check_limbo(struct rdt_domain *d, bool force_free);
> >  bool cbm_validate_intel(char *buf, u32 *data, struct rdt_resource *r);
> > +bool cbm_validate_amd(char *buf, u32 *data, struct rdt_resource *r);
> >
> >  #endif /* _ASM_X86_RESCTRL_H */
> > diff --git a/arch/x86/kernel/cpu/resctrl_ctrlmondata.c
> b/arch/x86/kernel/cpu/resctrl_ctrlmondata.c
> > index 71aa1d971430..b6ceb4db9322 100644
> > --- a/arch/x86/kernel/cpu/resctrl_ctrlmondata.c
> > +++ b/arch/x86/kernel/cpu/resctrl_ctrlmondata.c
> > @@ -28,6 +28,52 @@
> >  #include <linux/slab.h>
> >  #include "resctrl.h"
> >
> > +/*
> > + * Check whether MBA bandwidth percentage value is correct. The value
> is
> > + * checked against the minimum and max bandwidth values specified by
> the
> 
> Either write "min" and "max" or write them both out fully.

Ok
> 
> > + * hardware. The allocated bandwidth percentage is rounded to the next
> > + * control step available on the hardware.
> > + */
> > +static bool bw_validate_amd(char *buf, unsigned long *data,
> > +			    struct rdt_resource *r)
> > +{
> > +	unsigned long bw;
> > +	int ret;
> > +
> > +	ret = kstrtoul(buf, 10, &bw);
> > +	if (ret) {
> > +		rdt_last_cmd_printf("Non-decimal digit in MB value %s\n",
> buf);
> > +		return false;
> > +	}
> > +
> > +	if (bw < r->membw.min_bw || bw > r->default_ctrl) {
> > +		rdt_last_cmd_printf("MB value %ld out of range [%d,%d]\n",
> bw,
> > +				    r->membw.min_bw, r->default_ctrl);
> > +		return false;
> > +	}
> > +
> > +	*data = roundup(bw, (unsigned long)r->membw.bw_gran);
> > +	return true;
> > +}
> > +
> > +int parse_bw_amd(struct rdt_parse_data *data, struct rdt_resource *r,
> > +		 struct rdt_domain *d)
> > +{
> > +	unsigned long bw_val;
> > +
> > +	if (d->have_new_ctrl) {
> > +		rdt_last_cmd_printf("duplicate domain %d\n", d->id);
> 
> Start with a capital letter: "Duplicate domain..."
ok
> 
> And looking at the rest, some of them start with a capital letter and
> some of them not. Please unify that in a separate patch.

Yes, I see it. I will fix all the rdt_last_cmd_printf texts.

> 
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (!bw_validate_amd(data->buf, &bw_val, r))
> > +		return -EINVAL;
> 
> <---- newline here.
Ok.
> 
> > +	d->new_ctrl = bw_val;
> > +	d->have_new_ctrl = true;
> > +
> > +	return 0;
> > +}
> > +
> >  /*
> >   * Check whether MBA bandwidth percentage value is correct. The value is
> >   * checked against the minimum and max bandwidth values specified by
> the
> 
> --
> Regards/Gruss,
>     Boris.
> 
> Good mailing practices for 400: avoid top-posting and trim the reply.

[PATCH v8 12/13] Documentation: Rename and update intel_rdt_ui.txt to resctrl_ui.txt

[Moger, Babu]

Rename intel_rdt_ui.txt to generic resctrl_ui.txt and update the
documentation for AMD.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 Documentation/x86/{intel_rdt_ui.txt => resctrl_ui.txt} | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
 rename Documentation/x86/{intel_rdt_ui.txt => resctrl_ui.txt} (99%)

diff --git a/Documentation/x86/intel_rdt_ui.txt b/Documentation/x86/resctrl_ui.txt
similarity index 99%
rename from Documentation/x86/intel_rdt_ui.txt
rename to Documentation/x86/resctrl_ui.txt
index 52b10945ff75..d9aed8303984 100644
--- a/Documentation/x86/intel_rdt_ui.txt
+++ b/Documentation/x86/resctrl_ui.txt
@@ -1,4 +1,7 @@
-User Interface for Resource Allocation in Intel Resource Director Technology
+User Interface for Resource Control feature
+
+Intel refers to this feature as Intel Resource Director Technology(Intel(R) RDT).
+AMD refers to this feature as AMD Platform Quality of Service(AMD QoS).
 
 Copyright (C) 2016 Intel Corporation
 
@@ -6,8 +9,8 @@ Fenghua Yu <fenghua.yu@intel.com>
 Tony Luck <tony.luck@intel.com>
 Vikas Shivappa <vikas.shivappa@intel.com>
 
-This feature is enabled by the CONFIG_INTEL_RDT Kconfig and the
-X86 /proc/cpuinfo flag bits:
+This feature is enabled by the CONFIG_RESCTRL and the X86 /proc/cpuinfo
+flag bits:
 RDT (Resource Director Technology) Allocation - "rdt_a"
 CAT (Cache Allocation Technology) - "cat_l3", "cat_l2"
 CDP (Code and Data Prioritization ) - "cdp_l3", "cdp_l2"
-- 
2.17.1

[PATCH v8 13/13] MAINTAINERS: Update the file and documentation names in arch/x86

[Moger, Babu]

Update the MAINTAINERS to reflect the changed file(and documentation)
names in arch/x86/kernel/cpu. The file names have changed from
intel_rdt* to resctrl*.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 MAINTAINERS | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 48a65c3a4189..7643dba289c6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12267,9 +12267,9 @@ M:	Fenghua Yu <fenghua.yu@intel.com>
 M:	Reinette Chatre <reinette.chatre@intel.com>
 L:	linux-kernel@vger.kernel.org
 S:	Supported
-F:	arch/x86/kernel/cpu/intel_rdt*
-F:	arch/x86/include/asm/intel_rdt_sched.h
-F:	Documentation/x86/intel_rdt*
+F:	arch/x86/kernel/cpu/resctrl*
+F:	arch/x86/include/asm/resctrl_sched.h
+F:	Documentation/x86/resctrl*
 
 READ-COPY UPDATE (RCU)
 M:	"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
-- 
2.17.1