* [PATCH kernel] powerpc/prom_init: Pass the "os-term" message to hypervisor
From: Alexey Kardashevskiy @ 2020-03-12 7:44 UTC (permalink / raw)
To: linuxppc-dev
Cc: Alexey Kardashevskiy, Ram Pai, Thiago Jung Bauermann, kvm-ppc,
David Gibson
The "os-term" RTAS calls has one argument with a message address of
OS termination cause. rtas_os_term() already passes it but the recently
added prom_init's version of that missed it; it also does not fill args
correctly.
This passes the message address and initializes the number of arguments.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
arch/powerpc/kernel/prom_init.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index 577345382b23..673f13b87db1 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c
@@ -1773,6 +1773,9 @@ static void __init prom_rtas_os_term(char *str)
if (token == 0)
prom_panic("Could not get token for ibm,os-term\n");
os_term_args.token = cpu_to_be32(token);
+ os_term_args.nargs = cpu_to_be32(1);
+ os_term_args.nret = cpu_to_be32(1);
+ os_term_args.args[0] = cpu_to_be32(__pa(str));
prom_rtas_hcall((uint64_t)&os_term_args);
}
#endif /* CONFIG_PPC_SVM */
--
2.17.1
^ permalink raw reply related
* [PATCH v4] powerpc: Replace setup_irq() by request_irq()
From: afzal mohammed @ 2020-03-12 6:42 UTC (permalink / raw)
To: linuxppc-dev, linux-kernel; +Cc: Scott Wood, Paul Mackerras, afzal mohammed
In-Reply-To: <20200304004746.4557-1-afzal.mohd.ma@gmail.com>
request_irq() is preferred over setup_irq(). Invocations of setup_irq()
occur after memory allocators are ready.
Per tglx[1], setup_irq() existed in olden days when allocators were not
ready by the time early interrupts were initialized.
Hence replace setup_irq() by request_irq().
[1] https://lkml.kernel.org/r/alpine.DEB.2.20.1710191609480.1971@nanos
Signed-off-by: afzal mohammed <afzal.mohd.ma@gmail.com>
---
v4:
* pass non-NULL dev_id while requesting shared irq in mpc85xx_cds.c, as
request_irq() can fail due to sanity check, which is not done in
setup_irq()
v3:
* Split out from tree wide series, as Thomas suggested to get it thr'
respective maintainers
* Modify pr_err displayed in case of error
* Re-arrange code & choose pr_err args as required to improve readability
* Remove irrelevant parts from commit message & improve
v2:
* Replace pr_err("request_irq() on %s failed" by
pr_err("%s: request_irq() failed"
* Commit message massage
arch/powerpc/platforms/85xx/mpc85xx_cds.c | 11 ++++-----
arch/powerpc/platforms/8xx/cpm1.c | 9 ++-----
arch/powerpc/platforms/8xx/m8xx_setup.c | 9 ++-----
arch/powerpc/platforms/chrp/setup.c | 14 ++++-------
arch/powerpc/platforms/powermac/pic.c | 29 +++++++++--------------
arch/powerpc/platforms/powermac/smp.c | 12 ++++------
6 files changed, 29 insertions(+), 55 deletions(-)
diff --git a/arch/powerpc/platforms/85xx/mpc85xx_cds.c b/arch/powerpc/platforms/85xx/mpc85xx_cds.c
index 6b1436abe9b1..915ab6710b93 100644
--- a/arch/powerpc/platforms/85xx/mpc85xx_cds.c
+++ b/arch/powerpc/platforms/85xx/mpc85xx_cds.c
@@ -218,12 +218,6 @@ static irqreturn_t mpc85xx_8259_cascade_action(int irq, void *dev_id)
{
return IRQ_HANDLED;
}
-
-static struct irqaction mpc85xxcds_8259_irqaction = {
- .handler = mpc85xx_8259_cascade_action,
- .flags = IRQF_SHARED | IRQF_NO_THREAD,
- .name = "8259 cascade",
-};
#endif /* PPC_I8259 */
#endif /* CONFIG_PCI */
@@ -271,7 +265,10 @@ static int mpc85xx_cds_8259_attach(void)
* disabled when the last user of the shared IRQ line frees their
* interrupt.
*/
- if ((ret = setup_irq(cascade_irq, &mpc85xxcds_8259_irqaction))) {
+ ret = request_irq(cascade_irq, mpc85xx_8259_cascade_action,
+ IRQF_SHARED | IRQF_NO_THREAD, "8259 cascade",
+ cascade_node);
+ if (ret) {
printk(KERN_ERR "Failed to setup cascade interrupt\n");
return ret;
}
diff --git a/arch/powerpc/platforms/8xx/cpm1.c b/arch/powerpc/platforms/8xx/cpm1.c
index a43ee7d1ff85..4db4ca2e1222 100644
--- a/arch/powerpc/platforms/8xx/cpm1.c
+++ b/arch/powerpc/platforms/8xx/cpm1.c
@@ -120,12 +120,6 @@ static irqreturn_t cpm_error_interrupt(int irq, void *dev)
return IRQ_HANDLED;
}
-static struct irqaction cpm_error_irqaction = {
- .handler = cpm_error_interrupt,
- .flags = IRQF_NO_THREAD,
- .name = "error",
-};
-
static const struct irq_domain_ops cpm_pic_host_ops = {
.map = cpm_pic_host_map,
};
@@ -187,7 +181,8 @@ unsigned int __init cpm_pic_init(void)
if (!eirq)
goto end;
- if (setup_irq(eirq, &cpm_error_irqaction))
+ if (request_irq(eirq, cpm_error_interrupt, IRQF_NO_THREAD, "error",
+ NULL))
printk(KERN_ERR "Could not allocate CPM error IRQ!");
setbits32(&cpic_reg->cpic_cicr, CICR_IEN);
diff --git a/arch/powerpc/platforms/8xx/m8xx_setup.c b/arch/powerpc/platforms/8xx/m8xx_setup.c
index f1c805c8adbc..df4d57d07f9a 100644
--- a/arch/powerpc/platforms/8xx/m8xx_setup.c
+++ b/arch/powerpc/platforms/8xx/m8xx_setup.c
@@ -39,12 +39,6 @@ static irqreturn_t timebase_interrupt(int irq, void *dev)
return IRQ_HANDLED;
}
-static struct irqaction tbint_irqaction = {
- .handler = timebase_interrupt,
- .flags = IRQF_NO_THREAD,
- .name = "tbint",
-};
-
/* per-board overridable init_internal_rtc() function. */
void __init __attribute__ ((weak))
init_internal_rtc(void)
@@ -157,7 +151,8 @@ void __init mpc8xx_calibrate_decr(void)
(TBSCR_TBF | TBSCR_TBE));
immr_unmap(sys_tmr2);
- if (setup_irq(virq, &tbint_irqaction))
+ if (request_irq(virq, timebase_interrupt, IRQF_NO_THREAD, "tbint",
+ NULL))
panic("Could not allocate timer IRQ!");
}
diff --git a/arch/powerpc/platforms/chrp/setup.c b/arch/powerpc/platforms/chrp/setup.c
index fcf6f2342ef4..8328cd5817b0 100644
--- a/arch/powerpc/platforms/chrp/setup.c
+++ b/arch/powerpc/platforms/chrp/setup.c
@@ -451,13 +451,6 @@ static void __init chrp_find_openpic(void)
of_node_put(np);
}
-#if defined(CONFIG_VT) && defined(CONFIG_INPUT_ADBHID) && defined(CONFIG_XMON)
-static struct irqaction xmon_irqaction = {
- .handler = xmon_irq,
- .name = "XMON break",
-};
-#endif
-
static void __init chrp_find_8259(void)
{
struct device_node *np, *pic = NULL;
@@ -541,8 +534,11 @@ static void __init chrp_init_IRQ(void)
if (of_node_is_type(kbd->parent, "adb"))
break;
of_node_put(kbd);
- if (kbd)
- setup_irq(HYDRA_INT_ADB_NMI, &xmon_irqaction);
+ if (kbd) {
+ if (request_irq(HYDRA_INT_ADB_NMI, xmon_irq, 0, "XMON break",
+ NULL))
+ pr_err("Failed to register XMON break interrupt\n");
+ }
#endif
}
diff --git a/arch/powerpc/platforms/powermac/pic.c b/arch/powerpc/platforms/powermac/pic.c
index 2e969073473d..4921bccf0376 100644
--- a/arch/powerpc/platforms/powermac/pic.c
+++ b/arch/powerpc/platforms/powermac/pic.c
@@ -250,20 +250,6 @@ static unsigned int pmac_pic_get_irq(void)
return irq_linear_revmap(pmac_pic_host, irq);
}
-#ifdef CONFIG_XMON
-static struct irqaction xmon_action = {
- .handler = xmon_irq,
- .flags = IRQF_NO_THREAD,
- .name = "NMI - XMON"
-};
-#endif
-
-static struct irqaction gatwick_cascade_action = {
- .handler = gatwick_action,
- .flags = IRQF_NO_THREAD,
- .name = "cascade",
-};
-
static int pmac_pic_host_match(struct irq_domain *h, struct device_node *node,
enum irq_domain_bus_token bus_token)
{
@@ -384,12 +370,17 @@ static void __init pmac_pic_probe_oldstyle(void)
out_le32(&pmac_irq_hw[i]->enable, 0);
/* Hookup cascade irq */
- if (slave && pmac_irq_cascade)
- setup_irq(pmac_irq_cascade, &gatwick_cascade_action);
+ if (slave && pmac_irq_cascade) {
+ if (request_irq(pmac_irq_cascade, gatwick_action,
+ IRQF_NO_THREAD, "cascade", NULL))
+ pr_err("Failed to register cascade interrupt\n");
+ }
printk(KERN_INFO "irq: System has %d possible interrupts\n", max_irqs);
#ifdef CONFIG_XMON
- setup_irq(irq_create_mapping(NULL, 20), &xmon_action);
+ i = irq_create_mapping(NULL, 20);
+ if (request_irq(i, xmon_irq, IRQF_NO_THREAD, "NMI - XMON", NULL))
+ pr_err("Failed to register NMI-XMON interrupt\n");
#endif
}
@@ -441,7 +432,9 @@ static void __init pmac_pic_setup_mpic_nmi(struct mpic *mpic)
nmi_irq = irq_of_parse_and_map(pswitch, 0);
if (nmi_irq) {
mpic_irq_set_priority(nmi_irq, 9);
- setup_irq(nmi_irq, &xmon_action);
+ if (request_irq(nmi_irq, xmon_irq, IRQF_NO_THREAD,
+ "NMI - XMON", NULL))
+ pr_err("Failed to register NMI-XMON interrupt\n");
}
of_node_put(pswitch);
}
diff --git a/arch/powerpc/platforms/powermac/smp.c b/arch/powerpc/platforms/powermac/smp.c
index f95fbdee6efe..c55bf474ed4e 100644
--- a/arch/powerpc/platforms/powermac/smp.c
+++ b/arch/powerpc/platforms/powermac/smp.c
@@ -399,21 +399,19 @@ static int __init smp_psurge_kick_cpu(int nr)
return 0;
}
-static struct irqaction psurge_irqaction = {
- .handler = psurge_ipi_intr,
- .flags = IRQF_PERCPU | IRQF_NO_THREAD,
- .name = "primary IPI",
-};
-
static void __init smp_psurge_setup_cpu(int cpu_nr)
{
+ unsigned long flags = IRQF_PERCPU | IRQF_NO_THREAD;
+ int irq;
+
if (cpu_nr != 0 || !psurge_start)
return;
/* reset the entry point so if we get another intr we won't
* try to startup again */
out_be32(psurge_start, 0x100);
- if (setup_irq(irq_create_mapping(NULL, 30), &psurge_irqaction))
+ irq = irq_create_mapping(NULL, 30);
+ if (request_irq(irq, psurge_ipi_intr, flags, "primary IPI", NULL))
printk(KERN_ERR "Couldn't get primary IPI interrupt");
}
--
2.18.0
^ permalink raw reply related
* Re: [PATCH v4 6/8] perf/tools: Enhance JSON/metric infrastructure to handle "?"
From: kajoljain @ 2020-03-12 6:11 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: mark.rutland, maddy, peterz, yao.jin, mingo, kan.liang, ak,
alexander.shishkin, anju, mamatha4, sukadev, ravi.bangoria,
jmario, namhyung, tglx, mpetlan, gregkh, linux-kernel,
linux-perf-users, jolsa, linuxppc-dev
In-Reply-To: <20200310183455.GB12036@kernel.org>
On 3/11/20 12:04 AM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Mar 09, 2020 at 11:55:50AM +0530, Kajol Jain escreveu:
>> Patch enhances current metric infrastructure to handle "?" in the metric
>> expression. The "?" can be use for parameters whose value not known while
>> creating metric events and which can be replace later at runtime to
>> the proper value. It also add flexibility to create multiple events out
>> of single metric event added in json file.
>>
>> Patch adds function 'arch_get_runtimeparam' which is a arch specific
>> function, returns the count of metric events need to be created.
>> By default it return 1.
>>
>> One loop is added in function 'metricgroup__add_metric', which create
>> multiple events at run time depend on return value of
>> 'arch_get_runtimeparam' and merge that event in 'group_list'.
>>
>> This infrastructure needed for hv_24x7 socket/chip level events.
>> "hv_24x7" chip level events needs specific chip-id to which the
>> data is requested. Function 'arch_get_runtimeparam' implemented
>> in header.c which extract number of sockets from sysfs file
>> "sockets" under "/sys/devices/hv_24x7/interface/".
>>
>> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
>> ---
>> tools/perf/arch/powerpc/util/header.c | 22 +++++
>> tools/perf/util/expr.h | 1 +
>> tools/perf/util/expr.l | 19 +++-
>> tools/perf/util/metricgroup.c | 124 ++++++++++++++++++++------
>> tools/perf/util/metricgroup.h | 1 +
>> tools/perf/util/stat-shadow.c | 8 ++
>> 6 files changed, 148 insertions(+), 27 deletions(-)
>>
>> diff --git a/tools/perf/arch/powerpc/util/header.c b/tools/perf/arch/powerpc/util/header.c
>> index 3b4cdfc5efd6..036f6b2ce202 100644
>> --- a/tools/perf/arch/powerpc/util/header.c
>> +++ b/tools/perf/arch/powerpc/util/header.c
>> @@ -7,6 +7,11 @@
>> #include <string.h>
>> #include <linux/stringify.h>
>> #include "header.h"
>> +#include "metricgroup.h"
>> +#include "evlist.h"
>> +#include <dirent.h>
>> +#include "pmu.h"
>> +#include <api/fs/fs.h>
>>
>> #define mfspr(rn) ({unsigned long rval; \
>> asm volatile("mfspr %0," __stringify(rn) \
>> @@ -16,6 +21,8 @@
>> #define PVR_VER(pvr) (((pvr) >> 16) & 0xFFFF) /* Version field */
>> #define PVR_REV(pvr) (((pvr) >> 0) & 0xFFFF) /* Revison field */
>>
>> +#define SOCKETS_INFO_FILE_PATH "/devices/hv_24x7/interface/"
>> +
>> int
>> get_cpuid(char *buffer, size_t sz)
>> {
>> @@ -44,3 +51,18 @@ get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
>>
>> return bufp;
>> }
>> +
>> +int arch_get_runtimeparam(void)
>> +{
>> + int count;
>> + char path[PATH_MAX];
>> + char filename[] = "sockets";
>> +
>> + snprintf(path, PATH_MAX,
>> + SOCKETS_INFO_FILE_PATH "%s", filename);
>> +
>> + if (sysfs__read_ull(path, (unsigned long long *)&count) < 0)
>> + return 1;
>> + else
>> + return count;
>
> Why this cast dance? We have sysfs__read_int(path, &count).
>
> Also this is more compact:
>
> return sysfs__read_int(path, &count) < 0 ? 1 : count;
Hi Arnaldo,
Yes it will be better to use like that.
>
>> +}
>> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
>> index 9377538f4097..d17664e628db 100644
>> --- a/tools/perf/util/expr.h
>> +++ b/tools/perf/util/expr.h
>> @@ -15,6 +15,7 @@ struct parse_ctx {
>> struct parse_id ids[MAX_PARSE_ID];
>> };
>>
>> +int expr__runtimeparam;
>> void expr__ctx_init(struct parse_ctx *ctx);
>> void expr__add_id(struct parse_ctx *ctx, const char *id, double val);
>> int expr__parse(double *final_val, struct parse_ctx *ctx, const char *expr);
>> diff --git a/tools/perf/util/expr.l b/tools/perf/util/expr.l
>> index 1928f2a3dddc..ec4b00671f67 100644
>> --- a/tools/perf/util/expr.l
>> +++ b/tools/perf/util/expr.l
>> @@ -45,6 +45,21 @@ static char *normalize(char *str)
>> *dst++ = '/';
>> else if (*str == '\\')
>> *dst++ = *++str;
>> + else if (*str == '?') {
>> +
>> + int size = snprintf(NULL, 0, "%d", expr__runtimeparam);
>
> TIL that C99 allows for a NULL str to format and return the number of
> bytes it would write if the string was large enough... wonders never
> cease :-)
>
>> + char * paramval = (char *)malloc(size);
>
> No need for the cast, malloc returns void *, or has that changed? 8-)
> and please no space before the variable name.
>
Okk, Will take care of that.
> Humm this is all complicated, why not use asprintf and have something
> like:
>
>> + int i = 0;
>> +
>> + if(!paramval)
>> + *dst++ = '0';
>> + else {
>> + sprintf(paramval, "%d", expr__runtimeparam);
>> + while(i < size)
>> + *dst++ = paramval[i++];
>> + free(paramval);
>> + }
>
> char *paramval;
> int size = asprintf(¶mval, "%d", expr__runtimeparam);
>
> if (size < 0)
> *dst++ = '0';
> else {
> while (i < size)
> *dst++ = paramval[i++];
> free(paramval);
> }
>
>
>> + }
>> else
>> *dst++ = *str;
>> str++;
>> @@ -72,8 +87,8 @@ number [0-9]+
>>
>> sch [-,=]
>> spec \\{sch}
>> -sym [0-9a-zA-Z_\.:@]+
>> -symbol {spec}*{sym}*{spec}*{sym}*
>> +sym [0-9a-zA-Z_\.:@?]+
>> +symbol {spec}*{sym}*{spec}*{sym}*{spec}*{sym}
>>
>> %%
>> {
>> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
>> index c3a8c701609a..11eeeb929b91 100644
>> --- a/tools/perf/util/metricgroup.c
>> +++ b/tools/perf/util/metricgroup.c
>> @@ -474,6 +474,98 @@ static bool metricgroup__has_constraint(struct pmu_event *pe)
>> return false;
>> }
>>
>> +int __weak arch_get_runtimeparam(void)
>> +{
>> + return 1;
>> +}
>> +
>> +static int metricgroup__add_metric_runtime_param(struct strbuf *events,
>> + struct list_head *group_list, struct pmu_event *pe)
>> +{
>> + int i, count;
>> + int ret = -EINVAL;
>> +
>> + count = arch_get_runtimeparam();
>> +
>> + /* This loop is added to create multiple
>> + * events depend on count value and add
>> + * those events to group_list.
>> + */
>> +
>> + for (i = 0; i < count; i++) {
>> + const char **ids;
>> + int idnum;
>> + struct egroup *eg;
>> + char value[PATH_MAX];
>> +
>> + expr__runtimeparam = i;
>> +
>> + if (expr__find_other(pe->metric_expr,
>> + NULL, &ids, &idnum) < 0)
>> + return ret;
>> +
>> + if (events->len > 0)
>> + strbuf_addf(events, ",");
>> +
>> + if (metricgroup__has_constraint(pe))
>> + metricgroup__add_metric_non_group(events, ids, idnum);
>> + else
>> + metricgroup__add_metric_weak_group(events, ids, idnum);
>> +
>> + eg = malloc(sizeof(struct egroup));
>
> Shorter form that works even if you change that type:
>
> + eg = malloc(sizeof(*eg));
>
>> + if (!eg) {
>> + ret = -ENOMEM;
>> + return ret;
>> + }
>> + sprintf(value, "%s%c%d", pe->metric_name, '_', i);
>> + eg->ids = ids;
>> + eg->idnum = idnum;
>> + eg->metric_name = strdup(value);
>
> Please check strdup() return just like you checked the malloc(sizeof(struct egroup)).
Right, I missed that part, will update it.
>
>> + eg->metric_expr = pe->metric_expr;
>> + eg->metric_unit = pe->unit;
>> + list_add_tail(&eg->nd, group_list);
>> + ret = 0;
>> +
>> + if (ret != 0)
>> + break;
>> + }
>> + return ret;
>> +}
>> +static int metricgroup__add_metric_param(struct strbuf *events,
>> + struct list_head *group_list, struct pmu_event *pe)
>> +{
>> +
>> + const char **ids;
>> + int idnum;
>> + struct egroup *eg;
>> + int ret = -EINVAL;
>> +
>> + if (expr__find_other(pe->metric_expr,
>> + NULL, &ids, &idnum) < 0)
>
> Why break the above in two lines?
>
>> + return ret;
>> + if (events->len > 0)
>> + strbuf_addf(events, ",");
>> +
>> + if (metricgroup__has_constraint(pe))
>> + metricgroup__add_metric_non_group(events, ids, idnum);
>> + else
>> + metricgroup__add_metric_weak_group(events, ids, idnum);
>> +
>> + eg = malloc(sizeof(struct egroup));
>
> Ditto
>
>> + if (!eg)
>> + ret = -ENOMEM;
>> +
>> + eg->ids = ids;
>> + eg->idnum = idnum;
>> + eg->metric_name = pe->metric_name;
>> + eg->metric_expr = pe->metric_expr;
>> + eg->metric_unit = pe->unit;
>> + list_add_tail(&eg->nd, group_list);
>> + ret = 0;
>> +
>> + return ret;
>> +}
>> +
>> static int metricgroup__add_metric(const char *metric, struct strbuf *events,
>> struct list_head *group_list)
>> {
>> @@ -493,35 +585,17 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
>> continue;
>> if (match_metric(pe->metric_group, metric) ||
>> match_metric(pe->metric_name, metric)) {
>> - const char **ids;
>> - int idnum;
>> - struct egroup *eg;
>>
>> pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
>>
>> - if (expr__find_other(pe->metric_expr,
>> - NULL, &ids, &idnum) < 0)
>> - continue;
>> - if (events->len > 0)
>> - strbuf_addf(events, ",");
>> -
>> - if (metricgroup__has_constraint(pe))
>> - metricgroup__add_metric_non_group(events, ids, idnum);
>> + if (strstr(pe->metric_expr, "?"))
>> + ret = metricgroup__add_metric_runtime_param(events,
>> + group_list, pe);
>> else
>> - metricgroup__add_metric_weak_group(events, ids, idnum);
>> -
>> - eg = malloc(sizeof(struct egroup));
>
> *eg
>> - if (!eg) {
>> - ret = -ENOMEM;
>> - break;
>> - }
>> - eg->ids = ids;
>> - eg->idnum = idnum;
>> - eg->metric_name = pe->metric_name;
>> - eg->metric_expr = pe->metric_expr;
>> - eg->metric_unit = pe->unit;
>> - list_add_tail(&eg->nd, group_list);
>> - ret = 0;
>> + ret = metricgroup__add_metric_param(events,
>> + group_list, pe);
>> + if (ret == -EINVAL)
>> + continue;
>> }
>> }
>> return ret;
>> diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h
>> index 475c7f912864..81224ba1270d 100644
>> --- a/tools/perf/util/metricgroup.h
>> +++ b/tools/perf/util/metricgroup.h
>> @@ -34,4 +34,5 @@ int metricgroup__parse_groups(const struct option *opt,
>> void metricgroup__print(bool metrics, bool groups, char *filter,
>> bool raw, bool details);
>> bool metricgroup__has_metric(const char *metric);
>> +int arch_get_runtimeparam(void);
>> #endif
>> diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
>> index 0fd713d3674f..92c4c9abbaa0 100644
>> --- a/tools/perf/util/stat-shadow.c
>> +++ b/tools/perf/util/stat-shadow.c
>> @@ -777,6 +777,14 @@ static void generic_metric(struct perf_stat_config *config,
>> }
>>
>> if (!metric_events[i]) {
>> +
>> + if (strstr(metric_expr, "?")) {
>> + char *tmp = strrchr(metric_name, '_');
>> +
>
> So at this point a metric name is guaranteed to have a _?
Yes, because we are appending '_' with parameter value in metric name
when we adding these events in 'group_list'. So, I added it, because
we can have multiple values of that runtime_parameter, so by appending it
in metric name, we can understand to which event the counter data belongs.
Thanks for reviewing the patch, I will send updated patch.
Thanks,
Kajol
>
>> + tmp++;
>> + expr__runtimeparam = strtol(tmp, &tmp, 10);
>> + }
>> +
>> if (expr__parse(&ratio, &pctx, metric_expr) == 0) {
>> char *unit;
>> char metric_bf[64];
>> --
>> 2.18.1
>>
>
^ permalink raw reply
* Re: [PATCH] powerpc/pseries: fix of_read_drc_info_cell() to point at next record
From: Michael Ellerman @ 2020-03-12 5:43 UTC (permalink / raw)
To: Tyrel Datwyler, Nathan Lynch; +Cc: mwb, msuchanek, linuxppc-dev
In-Reply-To: <e99f5e39-4788-a9ef-30d7-8096df7ee36e@linux.ibm.com>
Tyrel Datwyler <tyreld@linux.ibm.com> writes:
> On 3/10/20 10:25 AM, Nathan Lynch wrote:
>> Tyrel Datwyler <tyreld@linux.ibm.com> writes:
>>> The expectation is that when calling of_read_drc_info_cell()
>>> repeatedly to parse multiple drc-info records that the in/out curval
>>> parameter points at the start of the next record on return. However,
>>> the current behavior has curval still pointing at the final value of
>>> the record just parsed. The result of which is that if the
>>> ibm,drc-info property contains multiple properties the parsed value
>>> of the drc_type for any record after the first has the power_domain
>>> value of the previous record appended to the type string.
>>>
>>> Ex: observed the following 0xffffffff prepended to PHB
>>>
>>> [ 69.485037] drc-info: type: \xff\xff\xff\xffPHB, prefix: PHB , index_start: 0x20000001
>>> [ 69.485038] drc-info: suffix_start: 1, sequential_elems: 3072, sequential_inc: 1
>>> [ 69.485038] drc-info: power-domain: 0xffffffff, last_index: 0x20000c00
>>>
>>> Fix by incrementing curval past the power_domain value to point at
>>> drc_type string of next record.
>>>
>>> Fixes: a29396653b8bf ("pseries/drc-info: Search DRC properties for CPU indexes")
>>
>> I have a different commit hash for that:
>> e83636ac3334 pseries/drc-info: Search DRC properties for CPU indexes
>
> Oof, looks like I grabbed the commit hash from the SLES 15 SP1 branch. You have
> the correct upstream commit.
>
> Fixes: e83636ac3334 ("pseries/drc-info: Search DRC properties for CPU indexes")
>
> Michael, let me know if you want me to resubmit, or if you will fixup the Fixes
> tag on your end?
I can update the Fixes tag.
What's the practical effect of this bug? It seems like it should break
all the hotplug stuff, but presumably it doesn't in practice for some
reason?
It would also be *really* nice if we had some unit tests for this
parsing code, it's demonstrably very bug prone.
cheers
^ permalink raw reply
* Re: [PATCH 1/3] powerpc/numa: Set numa_node for all possible cpus
From: Srikar Dronamraju @ 2020-03-12 5:27 UTC (permalink / raw)
To: Michal Hocko
Cc: Linus Torvalds, linux-kernel, linux-mm, Mel Gorman,
Kirill A. Shutemov, Andrew Morton, linuxppc-dev,
Christopher Lameter, Vlastimil Babka
In-Reply-To: <20200311115735.GM23944@dhcp22.suse.cz>
* Michal Hocko <mhocko@kernel.org> [2020-03-11 12:57:35]:
> On Wed 11-03-20 16:32:35, Srikar Dronamraju wrote:
> > A Powerpc system with multiple possible nodes and with CONFIG_NUMA
> > enabled always used to have a node 0, even if node 0 does not any cpus
> > or memory attached to it. As per PAPR, node affinity of a cpu is only
> > available once its present / online. For all cpus that are possible but
> > not present, cpu_to_node() would point to node 0.
> >
> > To ensure a cpuless, memoryless dummy node is not online, powerpc need
> > to make sure all possible but not present cpu_to_node are set to a
> > proper node.
>
> Just curious, is this somehow related to
> http://lkml.kernel.org/r/20200227182650.GG3771@dhcp22.suse.cz?
>
The issue I am trying to fix is a known issue in Powerpc since many years.
So this surely not a problem after a75056fc1e7c (mm/memcontrol.c: allocate
shrinker_map on appropriate NUMA node").
I tried v5.6-rc4 + a75056fc1e7c but didnt face any issues booting the
kernel. Will work with Sachin/Abdul (reporters of the issue).
> > Cc: linuxppc-dev@lists.ozlabs.org
> > Cc: linux-mm@kvack.org
> > Cc: linux-kernel@vger.kernel.org
> > Cc: Michal Hocko <mhocko@suse.com>
> > Cc: Mel Gorman <mgorman@suse.de>
> > Cc: Vlastimil Babka <vbabka@suse.cz>
> > Cc: "Kirill A. Shutemov" <kirill@shutemov.name>
> > Cc: Christopher Lameter <cl@linux.com>
> > Cc: Michael Ellerman <mpe@ellerman.id.au>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> > ---
> > arch/powerpc/mm/numa.c | 16 ++++++++++++++--
> > 1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> > index 8a399db..54dcd49 100644
> > --- a/arch/powerpc/mm/numa.c
> > +++ b/arch/powerpc/mm/numa.c
> > @@ -931,8 +931,20 @@ void __init mem_topology_setup(void)
> >
> > reset_numa_cpu_lookup_table();
> >
> > - for_each_present_cpu(cpu)
> > - numa_setup_cpu(cpu);
> > + for_each_possible_cpu(cpu) {
> > + /*
> > + * Powerpc with CONFIG_NUMA always used to have a node 0,
> > + * even if it was memoryless or cpuless. For all cpus that
> > + * are possible but not present, cpu_to_node() would point
> > + * to node 0. To remove a cpuless, memoryless dummy node,
> > + * powerpc need to make sure all possible but not present
> > + * cpu_to_node are set to a proper node.
> > + */
> > + if (cpu_present(cpu))
> > + numa_setup_cpu(cpu);
> > + else
> > + set_cpu_numa_node(cpu, first_online_node);
> > + }
> > }
> >
> > void __init initmem_init(void)
> > --
> > 1.8.3.1
>
> --
> Michal Hocko
> SUSE Labs
--
Thanks and Regards
Srikar Dronamraju
^ permalink raw reply
* Re: [RFC PATCH v1] pseries/drmem: don't cache node id in drmem_lmb struct
From: Michal Suchánek @ 2020-03-12 5:02 UTC (permalink / raw)
To: Scott Cheloha
Cc: Nathan Lynch, Nathan Fontenont, David Hildenbrand, Aneesh Kumar,
Paul Mackerras, linuxppc-dev, Rick Lindsley
In-Reply-To: <20200311230815.1432367-1-cheloha@linux.ibm.com>
On Wed, Mar 11, 2020 at 06:08:15PM -0500, Scott Cheloha wrote:
> At memory hot-remove time we can retrieve an LMB's nid from its
> corresponding memory_block. There is no need to store the nid
> in multiple locations.
>
> Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com>
> ---
> The linear search in powerpc's memory_add_physaddr_to_nid() has become a
> bottleneck at boot on systems with many LMBs.
>
> As described in this patch here:
>
> https://lore.kernel.org/linuxppc-dev/20200221172901.1596249-2-cheloha@linux.ibm.com/
>
> the linear search seriously cripples drmem_init().
>
> The obvious solution (shown in that patch) is to just make the search
> in memory_add_physaddr_to_nid() faster. An XArray seems well-suited
> to the task of mapping an address range to an LMB object.
>
> The less obvious approach is to just call memory_add_physaddr_to_nid()
> in fewer places.
>
> I'm not sure which approach is correct, hence the RFC.
You basically revert the below which will likely cause the very error
that was fixed there:
commit b2d3b5ee66f2a04a918cc043cec0c9ed3de58f40
Author: Nathan Fontenot <nfont@linux.vnet.ibm.com>
Date: Tue Oct 2 10:35:59 2018 -0500
powerpc/pseries: Track LMB nid instead of using device tree
When removing memory we need to remove the memory from the node
it was added to instead of looking up the node it should be in
in the device tree.
During testing we have seen scenarios where the affinity for a
LMB changes due to a partition migration or PRRN event. In these
cases the node the LMB exists in may not match the node the device
tree indicates it belongs in. This can lead to a system crash
when trying to DLPAR remove the LMB after a migration or PRRN
event. The current code looks up the node in the device tree to
remove the LMB from, the crash occurs when we try to offline this
node and it does not have any data, i.e. node_data[nid] == NULL.
Thanks
Michal
^ permalink raw reply
* Re: [PATCH v3 23/27] powerpc/powernv/pmem: Add debug IOCTLs
From: Alastair D'Silva @ 2020-03-12 4:58 UTC (permalink / raw)
To: Andrew Donnellan
Cc: Madhavan Srinivasan, Alexey Kardashevskiy, Masahiro Yamada,
Oliver O'Halloran, Mauro Carvalho Chehab, Ira Weiny,
Thomas Gleixner, Rob Herring, Dave Jiang, linux-nvdimm,
Aneesh Kumar K . V, Krzysztof Kozlowski, Anju T Sudhakar,
Mahesh Salgaonkar, Arnd Bergmann, Greg Kurz, Nicholas Piggin,
Cédric Le Goater, Dan Williams, Hari Bathini, linux-mm,
Greg Kroah-Hartman, linux-kernel, Vishal Verma, Frederic Barrat,
Paul Mackerras, Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <5e2be4dd-bc8b-ff2c-d057-acd5f3728f4a@linux.ibm.com>
On Thu, 2020-03-05 at 14:11 +1100, Andrew Donnellan wrote:
> On 21/2/20 2:27 pm, Alastair D'Silva wrote:
> > From: Alastair D'Silva <alastair@d-silva.org>
> >
> > These IOCTLs provide low level access to the card to aid in
> > debugging
> > controller/FPGA firmware.
> >
> > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> > ---
> > arch/powerpc/platforms/powernv/pmem/Kconfig | 6 +
> > arch/powerpc/platforms/powernv/pmem/ocxl.c | 249
> > ++++++++++++++++++++
> > include/uapi/nvdimm/ocxl-pmem.h | 32 +++
> > 3 files changed, 287 insertions(+)
> >
> > diff --git a/arch/powerpc/platforms/powernv/pmem/Kconfig
> > b/arch/powerpc/platforms/powernv/pmem/Kconfig
> > index c5d927520920..3f44429d70c9 100644
> > --- a/arch/powerpc/platforms/powernv/pmem/Kconfig
> > +++ b/arch/powerpc/platforms/powernv/pmem/Kconfig
> > @@ -12,4 +12,10 @@ config OCXL_PMEM
> >
> > Select N if unsure.
> >
> > +config OCXL_PMEM_DEBUG
> > + bool "OpenCAPI Persistent Memory debugging"
> > + depends on OCXL_PMEM
> > + help
> > + Enables low level IOCTLs for OpenCAPI Persistent Memory
> > firmware development
> > +
>
> How dangerous are these ioctls and does that need to be pointed out
> in
> this description?
Good point, I'll elaborate.
>
> > endif
> > diff --git a/arch/powerpc/platforms/powernv/pmem/ocxl.c
> > b/arch/powerpc/platforms/powernv/pmem/ocxl.c
> > index e01f6f9fc180..d4ce5e9e0521 100644
> > --- a/arch/powerpc/platforms/powernv/pmem/ocxl.c
> > +++ b/arch/powerpc/platforms/powernv/pmem/ocxl.c
> > @@ -1050,6 +1050,235 @@ int req_controller_health_perf(struct
> > ocxlpmem *ocxlpmem)
> > GLOBAL_MMIO_HCI_REQ_HEALTH_PERF);
> > }
> >
> > +#ifdef CONFIG_OCXL_PMEM_DEBUG
> > +/**
> > + * enable_fwdebug() - Enable FW debug on the controller
> > + * @ocxlpmem: the device metadata
> > + * Return: 0 on success, negative on failure
> > + */
> > +static int enable_fwdebug(const struct ocxlpmem *ocxlpmem)
> > +{
> > + return ocxl_global_mmio_set64(ocxlpmem->ocxl_afu,
> > GLOBAL_MMIO_HCI,
> > + OCXL_LITTLE_ENDIAN,
> > + GLOBAL_MMIO_HCI_FW_DEBUG);
> > +}
> > +
> > +/**
> > + * disable_fwdebug() - Disable FW debug on the controller
> > + * @ocxlpmem: the device metadata
> > + * Return: 0 on success, negative on failure
> > + */
> > +static int disable_fwdebug(const struct ocxlpmem *ocxlpmem)
> > +{
> > + return ocxl_global_mmio_set64(ocxlpmem->ocxl_afu,
> > GLOBAL_MMIO_HCIC,
> > + OCXL_LITTLE_ENDIAN,
> > + GLOBAL_MMIO_HCI_FW_DEBUG);
> > +}
> > +
> > +static int ioctl_fwdebug(struct ocxlpmem *ocxlpmem,
> > + struct ioctl_ocxl_pmem_fwdebug __user
> > *uarg)
> > +{
> > + struct ioctl_ocxl_pmem_fwdebug args;
> > + u64 val;
> > + int i;
> > + int rc;
> > +
> > + if (copy_from_user(&args, uarg, sizeof(args)))
> > + return -EFAULT;
> > +
> > + // Buffer size must be a multiple of 8
> > + if ((args.buf_size & 0x07))
> > + return -EINVAL;
> > +
> > + if (args.buf_size > ocxlpmem->admin_command.data_size)
> > + return -EINVAL;
> > +
> > + mutex_lock(&ocxlpmem->admin_command.lock);
> > +
> > + rc = enable_fwdebug(ocxlpmem);
> > + if (rc)
> > + goto out;
> > +
> > + rc = admin_command_request(ocxlpmem, ADMIN_COMMAND_FW_DEBUG);
> > + if (rc)
> > + goto out;
> > +
> > + // Write DebugAction & FunctionCode
> > + val = ((u64)args.debug_action << 56) | ((u64)args.function_code
> > << 40);
> > +
> > + rc = ocxl_global_mmio_write64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.request_offset + 0x08,
> > + OCXL_LITTLE_ENDIAN, val);
> > + if (rc)
> > + goto out;
> > +
> > + rc = ocxl_global_mmio_write64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.request_offset + 0x10,
> > + OCXL_LITTLE_ENDIAN,
> > args.debug_parameter_1);
> > + if (rc)
> > + goto out;
> > +
> > + rc = ocxl_global_mmio_write64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.request_offset + 0x18,
> > + OCXL_LITTLE_ENDIAN,
> > args.debug_parameter_2);
> > + if (rc)
> > + goto out;
> > +
> > + for (i = 0x20; i < 0x38; i += 0x08)
>
> Comparison should be <=, the request block ends at 0x40.
>
> But in any case, scm_command_request() should I think already handle
> the
> clearing of the request block?
>
Correct.
> > + rc = ocxl_global_mmio_write64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.request_offset + i,
> > + OCXL_LITTLE_ENDIAN, 0);
> > + if (rc)
> > + goto out;
> > +
> > +
> > + // Populate admin command buffer
> > + if (args.buf_size) {
> > + for (i = 0; i < args.buf_size; i += sizeof(u64)) {
> > + u64 val;
> > +
> > + if (copy_from_user(&val, &args.buf[i],
> > sizeof(u64)))
> > + return -EFAULT;
> > +
> > + rc = ocxl_global_mmio_write64(ocxlpmem-
> > >ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset + i,
> > + OCXL_HOST_ENDIAN,
> > val);
> > + if (rc)
> > + goto out;
> > + }
> > + }
> > +
> > + rc = admin_command_execute(ocxlpmem);
> > + if (rc)
> > + goto out;
> > +
> > + rc = admin_command_complete_timeout(ocxlpmem,
> > + ocxlpmem-
> > >timeouts[ADMIN_COMMAND_FW_DEBUG]);
> > + if (rc < 0)
> > + goto out;
> > +
> > + rc = admin_response(ocxlpmem);
> > + if (rc < 0)
> > + goto out;
> > + if (rc != STATUS_SUCCESS) {
> > + warn_status(ocxlpmem, "Unexpected status from FW
> > Debug", rc);
> > + goto out;
> > + }
> > +
> > + if (args.buf_size) {
> > + for (i = 0; i < args.buf_size; i += sizeof(u64)) {
> > + u64 val;
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem-
> > >ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset + i,
> > + OCXL_HOST_ENDIAN,
> > &val);
>
> No check of the data identifier?
>
There isn't one documented.
> It seems to me that there's no definition in the spec whatsoever for
> the
> format of the data, so just copying as much as fits in the buffer
> seems
> correct.
>
That's right, decisions about this data will be left to userspace.
> > + if (rc)
> > + goto out;
> > +
> > + if (copy_to_user(&args.buf[i], &val,
> > sizeof(u64))) {
> > + rc = -EFAULT;
> > + goto out;
> > + }
> > + }
> > + }
> > +
> > + rc = admin_response_handled(ocxlpmem);
> > + if (rc)
> > + goto out;
> > +
> > + rc = disable_fwdebug(ocxlpmem);
> > + if (rc)
> > + goto out;
> > +
> > +out:
> > + mutex_unlock(&ocxlpmem->admin_command.lock);
> > + return rc;
> > +}
> > +
> > +static int ioctl_shutdown(struct ocxlpmem *ocxlpmem)
> > +{
> > + int rc;
> > +
> > + mutex_lock(&ocxlpmem->admin_command.lock);
> > +
> > + rc = admin_command_request(ocxlpmem, ADMIN_COMMAND_SHUTDOWN);
> > + if (rc)
> > + goto out;
> > +
> > + rc = admin_command_execute(ocxlpmem);
> > + if (rc)
> > + goto out;
> > +
> > + rc = admin_command_complete_timeout(ocxlpmem,
> > ADMIN_COMMAND_SHUTDOWN);
> > + if (rc < 0) {
> > + dev_warn(&ocxlpmem->dev, "Shutdown timed out\n");
> > + goto out;
> > + }
> > +
> > + rc = 0;
> > + goto out;
> > +
> > +out:
> > + mutex_unlock(&ocxlpmem->admin_command.lock);
> > + return rc;
> > +}
> > +
> > +static int ioctl_mmio_write(struct ocxlpmem *ocxlpmem,
> > + struct ioctl_ocxl_pmem_mmio __user
> > *uarg)
> > +{
> > + struct scm_ioctl_mmio args;
> > +
> > + if (copy_from_user(&args, uarg, sizeof(args)))
> > + return -EFAULT;
> > +
> > + return ocxl_global_mmio_write64(ocxlpmem->ocxl_afu,
> > args.address,
> > + OCXL_LITTLE_ENDIAN, args.val);
> > +}
> > +
> > +static int ioctl_mmio_read(struct ocxlpmem *ocxlpmem,
> > + struct ioctl_ocxl_pmem_mmio __user
> > *uarg)
> > +{
> > + struct ioctl_ocxl_pmem_mmio args;
> > + int rc;
> > +
> > + if (copy_from_user(&args, uarg, sizeof(args)))
> > + return -EFAULT;
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu, args.address,
> > + OCXL_LITTLE_ENDIAN, &args.val);
> > + if (rc)
> > + return rc;
> > +
> > + if (copy_to_user(uarg, &args, sizeof(args)))
> > + return -EFAULT;
> > +
> > + return 0;
> > +}
> > +#else /* CONFIG_OCXL_PMEM_DEBUG */
> > +static int ioctl_fwdebug(struct ocxlpmem *ocxlpmem,
> > + struct ioctl_ocxl_pmem_fwdebug __user
> > *uarg)
> > +{
> > + return -EPERM;
> > +}
> > +
> > +static int ioctl_shutdown(struct ocxlpmem *ocxlpmem)
> > +{
> > + return -EPERM;
> > +}
> > +
> > +static int ioctl_mmio_write(struct ocxlpmem *ocxlpmem,
> > + struct ioctl_ocxl_pmem_mmio __user
> > *uarg)
> > +{
> > + return -EPERM;
> > +}
> > +
> > +static int ioctl_mmio_read(struct ocxlpmem *ocxlpmem,
> > + struct ioctl_ocxl_pmem_mmio __user
> > *uarg)
> > +{
> > + return -EPERM;
> > +}
> > +#endif /* CONFIG_OCXL_PMEM_DEBUG */
> > +
> > static long file_ioctl(struct file *file, unsigned int cmd,
> > unsigned long args)
> > {
> > struct ocxlpmem *ocxlpmem = file->private_data;
> > @@ -1091,6 +1320,26 @@ static long file_ioctl(struct file *file,
> > unsigned int cmd, unsigned long args)
> > case IOCTL_OCXL_PMEM_REQUEST_HEALTH:
> > rc = req_controller_health_perf(ocxlpmem);
> > break;
> > +
> > + case IOCTL_OCXL_PMEM_FWDEBUG:
> > + rc = ioctl_fwdebug(ocxlpmem,
> > + (struct ioctl_ocxl_pmem_fwdebug
> > __user *)args);
> > + break;
> > +
> > + case IOCTL_OCXL_PMEM_SHUTDOWN:
> > + rc = ioctl_shutdown(ocxlpmem);
> > + break;
> > +
> > + case IOCTL_OCXL_PMEM_MMIO_WRITE:
> > + rc = ioctl_mmio_write(ocxlpmem,
> > + (struct ioctl_ocxl_pmem_mmio
> > __user *)args);
> > + break;
> > +
> > + case IOCTL_OCXL_PMEM_MMIO_READ:
> > + rc = ioctl_mmio_read(ocxlpmem,
> > + (struct ioctl_ocxl_pmem_mmio
> > __user *)args);
> > + break;
> > +
> > }
> >
> > return rc;
> > diff --git a/include/uapi/nvdimm/ocxl-pmem.h
> > b/include/uapi/nvdimm/ocxl-pmem.h
> > index 0d03abb44001..e20a4f8be82a 100644
> > --- a/include/uapi/nvdimm/ocxl-pmem.h
> > +++ b/include/uapi/nvdimm/ocxl-pmem.h
> > @@ -6,6 +6,28 @@
> > #include <linux/types.h>
> > #include <linux/ioctl.h>
> >
> > +enum ocxlpmem_fwdebug_action {
> > + OCXL_PMEM_FWDEBUG_READ_CONTROLLER_MEMORY = 0x01,
> > + OCXL_PMEM_FWDEBUG_WRITE_CONTROLLER_MEMORY = 0x02,
> > + OCXL_PMEM_FWDEBUG_ENABLE_FUNCTION = 0x03,
> > + OCXL_PMEM_FWDEBUG_DISABLE_FUNCTION = 0x04,
> > + OCXL_PMEM_FWDEBUG_GET_PEL = 0x05, // Retrieve Persistent Error
> > Log
> > +};
> > +
> > +struct ioctl_ocxl_pmem_buffer_info {
> > + __u32 admin_command_buffer_size; // out
> > + __u32 near_storage_buffer_size; // out
> > +};
>
> This struct seems unused.
>
Whoops
> > +
> > +struct ioctl_ocxl_pmem_fwdebug { // All args are inputs
> > + enum ocxlpmem_fwdebug_action debug_action;
> > + __u16 function_code;
> > + __u16 buf_size; // Size of optional data buffer
> > + __u64 debug_parameter_1;
> > + __u64 debug_parameter_2;
> > + __u8 *buf; // Pointer to optional in/out data buffer
> > +};
> > +
> > #define OCXL_PMEM_ERROR_LOG_ACTION_RESET (1 << (32-32))
> > #define OCXL_PMEM_ERROR_LOG_ACTION_CHKFW (1 << (53-32))
> > #define OCXL_PMEM_ERROR_LOG_ACTION_REPLACE (1 << (54-32))
> > @@ -66,6 +88,11 @@ struct ioctl_ocxl_pmem_controller_stats {
> > __u64 cache_write_latency; /* nanoseconds */
> > };
> >
> > +struct ioctl_ocxl_pmem_mmio {
> > + __u64 address; /* Offset in global MMIO space */
> > + __u64 val; /* value to write/was read */
> > +};
> > +
> > struct ioctl_ocxl_pmem_eventfd {
> > __s32 eventfd;
> > __u32 reserved;
> > @@ -92,4 +119,9 @@ struct ioctl_ocxl_pmem_eventfd {
> > #define IOCTL_OCXL_PMEM_EVENT_CHECK _IOR(OC
> > XL_PMEM_MAGIC, 0x07, __u64)
> > #define IOCTL_OCXL_PMEM_REQUEST_HEALTH _IO(OCX
> > L_PMEM_MAGIC, 0x08)
> >
> > +#define IOCTL_OCXL_PMEM_FWDEBUG _IOWR(OCXL_PMEM_MAGIC,
> > 0xf0, struct ioctl_ocxl_pmem_fwdebug)
> > +#define IOCTL_OCXL_PMEM_MMIO_WRITE _IOW(OCXL_PMEM_MAGIC, 0xf1,
> > struct ioctl_ocxl_pmem_mmio)
> > +#define IOCTL_OCXL_PMEM_MMIO_READ _IOWR(OCXL_PMEM_MAGIC, 0xf2,
> > struct ioctl_ocxl_pmem_mmio)
> > +#define IOCTL_OCXL_PMEM_SHUTDOWN _IO(OCXL_PMEM_MAGIC, 0xf3)
> > +
> > #endif /* _UAPI_OCXL_SCM_H */
> >
--
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819
^ permalink raw reply
* Re: [PATCH v3 19/27] powerpc/powernv/pmem: Add an IOCTL to report controller statistics
From: Alastair D'Silva @ 2020-03-12 4:47 UTC (permalink / raw)
To: Andrew Donnellan
Cc: Madhavan Srinivasan, Alexey Kardashevskiy, Masahiro Yamada,
Oliver O'Halloran, Mauro Carvalho Chehab, Ira Weiny,
Thomas Gleixner, Rob Herring, Dave Jiang, linux-nvdimm,
Aneesh Kumar K . V, Krzysztof Kozlowski, Anju T Sudhakar,
Mahesh Salgaonkar, Arnd Bergmann, Greg Kurz, Nicholas Piggin,
Cédric Le Goater, Dan Williams, Hari Bathini, linux-mm,
Greg Kroah-Hartman, linux-kernel, Vishal Verma, Frederic Barrat,
Paul Mackerras, Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <c0002b11-7f54-38d3-4ae2-9008a5cc0b61@linux.ibm.com>
On Thu, 2020-03-05 at 11:46 +1100, Andrew Donnellan wrote:
> On 21/2/20 2:27 pm, Alastair D'Silva wrote:
> > From: Alastair D'Silva <alastair@d-silva.org>
> >
> > The controller can report a number of statistics that are useful
> > in evaluating the performance and reliability of the card.
> >
> > This patch exposes this information via an IOCTL.
> >
> > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> > ---
> > arch/powerpc/platforms/powernv/pmem/ocxl.c | 185
> > +++++++++++++++++++++
> > include/uapi/nvdimm/ocxl-pmem.h | 17 ++
> > 2 files changed, 202 insertions(+)
> >
> > diff --git a/arch/powerpc/platforms/powernv/pmem/ocxl.c
> > b/arch/powerpc/platforms/powernv/pmem/ocxl.c
> > index 2cabafe1fc58..009d4fd29e7d 100644
> > --- a/arch/powerpc/platforms/powernv/pmem/ocxl.c
> > +++ b/arch/powerpc/platforms/powernv/pmem/ocxl.c
> > @@ -758,6 +758,186 @@ static int
> > ioctl_controller_dump_complete(struct ocxlpmem *ocxlpmem)
> > GLOBAL_MMIO_HCI_CONTROLLER_DUMP_COL
> > LECTED);
> > }
> >
> > +/**
> > + * controller_stats_header_parse() - Parse the first 64 bits of
> > the controller stats admin command response
> > + * @ocxlpmem: the device metadata
> > + * @length: out, returns the number of bytes in the response
> > (excluding the 64 bit header)
> > + */
> > +static int controller_stats_header_parse(struct ocxlpmem
> > *ocxlpmem,
> > + u32 *length)
> > +{
> > + int rc;
> > + u64 val;
> > +
> > + u16 data_identifier;
> > + u32 data_length;
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset,
> > + OCXL_LITTLE_ENDIAN, &val);
> > + if (rc)
> > + return rc;
> > +
> > + data_identifier = val >> 48;
> > + data_length = val & 0xFFFFFFFF;
> > +
> > + if (data_identifier != 0x4353) { // 'CS'
> > + dev_err(&ocxlpmem->dev,
> > + "Bad data identifier for controller stats,
> > expected 'CS', got '%-.*s'\n",
> > + 2, (char *)&data_identifier);
> > + return -EINVAL;
>
> Same comment as earlier patches re EINVAL
>
I don't think I've seen a comment yet on these particular blocks. Can
you suggest a better return value?
> > + }
> > +
> > + *length = data_length;
> > + return 0;
> > +}
> > +
> > +static int ioctl_controller_stats(struct ocxlpmem *ocxlpmem,
> > + struct
> > ioctl_ocxl_pmem_controller_stats __user *uarg)
> > +{
> > + struct ioctl_ocxl_pmem_controller_stats args;
> > + u32 length;
> > + int rc;
> > + u64 val;
> > +
> > + memset(&args, '\0', sizeof(args));
> > +
> > + mutex_lock(&ocxlpmem->admin_command.lock);
> > +
> > + rc = admin_command_request(ocxlpmem,
> > ADMIN_COMMAND_CONTROLLER_STATS);
> > + if (rc)
> > + goto out;
> > +
> > + rc = ocxl_global_mmio_write64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.request_offset + 0x08,
> > + OCXL_LITTLE_ENDIAN, 0);
> > + if (rc)
> > + goto out;
> > +
> > + rc = admin_command_execute(ocxlpmem);
> > + if (rc)
> > + goto out;
> > +
> > +
> > + rc = admin_command_complete_timeout(ocxlpmem,
> > + ADMIN_COMMAND_CONTROLLER_ST
> > ATS);
> > + if (rc < 0) {
> > + dev_warn(&ocxlpmem->dev, "Controller stats timed
> > out\n");
> > + goto out;
> > + }
> > +
> > + rc = admin_response(ocxlpmem);
> > + if (rc < 0)
> > + goto out;
> > + if (rc != STATUS_SUCCESS) {
> > + warn_status(ocxlpmem,
> > + "Unexpected status from controller stats",
> > rc);
> > + goto out;
> > + }
> > +
> > + rc = controller_stats_header_parse(ocxlpmem, &length);
> > + if (rc)
> > + goto out;
> > +
> > + if (length != 0x140)
> > + warn_status(ocxlpmem,
> > + "Unexpected length for controller stats
> > data, expected 0x140, got 0x%x",
> > + length);
>
> Might be worth a comment to explain where 0x140 comes from (it looks
> correct from my reading of the spec)
Ok
>
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset + 0x08 + 0x08,
> > + OCXL_LITTLE_ENDIAN, &val);
> > + if (rc)
> > + goto out;
> > +
> > + args.reset_count = val >> 32;
> > + args.reset_uptime = val & 0xFFFFFFFF;
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset + 0x08 + 0x10,
> > + OCXL_LITTLE_ENDIAN, &val);
> > + if (rc)
> > + goto out;
> > +
> > + args.power_on_uptime = val >> 32;
>
> We're not collecting life remaining?
>
It looks like my implementation is out of date. I'll bring it in line
with the spec.
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset + 0x08 + 0x40 + 0x08,
> > + OCXL_LITTLE_ENDIAN,
> > &args.host_load_count);
>
> My reading of the spec says HLC is at +0x10
>
Ditto
> > + if (rc)
> > + goto out;
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset + 0x08 + 0x40 + 0x10,
> > + OCXL_LITTLE_ENDIAN,
> > &args.host_store_count);
>
> HSC at +0x18
>
Ditto
> > + if (rc)
> > + goto out;
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset + 0x08 + 0x40 + 0x18,
> > + OCXL_LITTLE_ENDIAN,
> > &args.media_read_count);
>
> MRC is at +0x50
>
> And you're missing CRU, HLD, HSD
>
> > + if (rc)
> > + goto out;
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset + 0x08 + 0x40 + 0x20,
> > + OCXL_LITTLE_ENDIAN,
> > &args.media_write_count);
>
> MWC at +0x58
>
> > + if (rc)
> > + goto out;
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset + 0x08 + 0x40 + 0x28,
> > + OCXL_LITTLE_ENDIAN,
> > &args.cache_hit_count);
>
> CRHC at +0x90
>
> > + if (rc)
> > + goto out;
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset + 0x08 + 0x40 + 0x30,
> > + OCXL_LITTLE_ENDIAN,
> > &args.cache_miss_count);
>
> This field doesn't seem to exist at all in my copy of the spec
>
> > + if (rc)
> > + goto out;
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset + 0x08 + 0x40 + 0x38,
> > + OCXL_LITTLE_ENDIAN,
> > &args.media_read_latency);
>
> Nor this one
>
> > + if (rc)
> > + goto out;
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset + 0x08 + 0x40 + 0x40,
> > + OCXL_LITTLE_ENDIAN,
> > &args.media_write_latency);
>
> Nor this one
>
> > + if (rc)
> > + goto out;
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset + 0x08 + 0x40 + 0x48,
> > + OCXL_LITTLE_ENDIAN,
> > &args.cache_read_latency);
>
> Nor this one
>
> > + if (rc)
> > + goto out;
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset + 0x08 + 0x40 + 0x50,
> > + OCXL_LITTLE_ENDIAN,
> > &args.cache_write_latency);
>
> Nor this one
>
> > + if (rc)
> > + goto out;
> > +
> > + if (copy_to_user(uarg, &args, sizeof(args))) {
> > + rc = -EFAULT;
> > + goto out;
> > + }
> > +
> > + rc = admin_response_handled(ocxlpmem);
> > + if (rc)
> > + goto out;
> > +
> > + rc = 0;
> > + goto out;
>
> Per Fred this pattern isn't common in the kernel, but perhaps this
> is
> just personal taste
>
Ok
> > +
> > +out:
> > + mutex_unlock(&ocxlpmem->admin_command.lock);
> > + return rc;
> > +}
> > +
> > static long file_ioctl(struct file *file, unsigned int cmd,
> > unsigned long args)
> > {
> > struct ocxlpmem *ocxlpmem = file->private_data;
> > @@ -781,6 +961,11 @@ static long file_ioctl(struct file *file,
> > unsigned int cmd, unsigned long args)
> > case IOCTL_OCXL_PMEM_CONTROLLER_DUMP_COMPLETE:
> > rc = ioctl_controller_dump_complete(ocxlpmem);
> > break;
> > +
> > + case IOCTL_OCXL_PMEM_CONTROLLER_STATS:
> > + rc = ioctl_controller_stats(ocxlpmem,
> > + (struct
> > ioctl_ocxl_pmem_controller_stats __user *)args);
> > + break;
> > }
> >
> > return rc;
> > diff --git a/include/uapi/nvdimm/ocxl-pmem.h
> > b/include/uapi/nvdimm/ocxl-pmem.h
> > index d4d8512d03f7..add223aa2fdb 100644
> > --- a/include/uapi/nvdimm/ocxl-pmem.h
> > +++ b/include/uapi/nvdimm/ocxl-pmem.h
> > @@ -50,6 +50,22 @@ struct ioctl_ocxl_pmem_controller_dump_data {
> > __u64 reserved[8];
> > };
> >
> > +struct ioctl_ocxl_pmem_controller_stats {
> > + __u32 reset_count;
> > + __u32 reset_uptime; /* seconds */
> > + __u32 power_on_uptime; /* seconds */
> > + __u64 host_load_count;
> > + __u64 host_store_count;
> > + __u64 media_read_count;
> > + __u64 media_write_count;
> > + __u64 cache_hit_count;
> > + __u64 cache_miss_count;
> > + __u64 media_read_latency; /* nanoseconds */
> > + __u64 media_write_latency; /* nanoseconds */
> > + __u64 cache_read_latency; /* nanoseconds */
> > + __u64 cache_write_latency; /* nanoseconds */
> > +};
> > +
> > /* ioctl numbers */
> > #define OCXL_PMEM_MAGIC 0x5C
> > /* SCM devices */
> > @@ -57,5 +73,6 @@ struct ioctl_ocxl_pmem_controller_dump_data {
> > #define IOCTL_OCXL_PMEM_CONTROLLER_DUMP _IO(OCX
> > L_PMEM_MAGIC, 0x02)
> > #define IOCTL_OCXL_PMEM_CONTROLLER_DUMP_DATA _IOWR(O
> > CXL_PMEM_MAGIC, 0x03, struct ioctl_ocxl_pmem_controller_dump_data)
> > #define IOCTL_OCXL_PMEM_CONTROLLER_DUMP_COMPLETE _IO(OCXL_PMEM_M
> > AGIC, 0x04)
> > +#define IOCTL_OCXL_PMEM_CONTROLLER_STATS _IO(OCXL_PMEM_M
> > AGIC, 0x05)
> >
> > #endif /* _UAPI_OCXL_SCM_H */
> >
--
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819
^ permalink raw reply
* Re: [PATCH v3 23/27] powerpc/powernv/pmem: Add debug IOCTLs
From: Alastair D'Silva @ 2020-03-12 4:24 UTC (permalink / raw)
To: Frederic Barrat
Cc: Madhavan Srinivasan, Alexey Kardashevskiy, Masahiro Yamada,
Oliver O'Halloran, Mauro Carvalho Chehab, Ira Weiny,
Thomas Gleixner, Rob Herring, Dave Jiang, linux-nvdimm,
Aneesh Kumar K . V, Krzysztof Kozlowski, Anju T Sudhakar,
Mahesh Salgaonkar, Andrew Donnellan, Arnd Bergmann, Greg Kurz,
Nicholas Piggin, Cédric Le Goater, Dan Williams,
Hari Bathini, linux-mm, Greg Kroah-Hartman, linux-kernel,
Vishal Verma, Paul Mackerras, Andrew Morton, linuxppc-dev,
David S. Miller
In-Reply-To: <7e0e3b71-d70c-1dee-b630-0c33596b7223@linux.ibm.com>
On Wed, 2020-03-04 at 16:21 +0100, Frederic Barrat wrote:
>
> Le 21/02/2020 à 04:27, Alastair D'Silva a écrit :
> > From: Alastair D'Silva <alastair@d-silva.org>
> >
> > These IOCTLs provide low level access to the card to aid in
> > debugging
> > controller/FPGA firmware.
> >
> > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> > ---
> > arch/powerpc/platforms/powernv/pmem/Kconfig | 6 +
> > arch/powerpc/platforms/powernv/pmem/ocxl.c | 249
> > ++++++++++++++++++++
> > include/uapi/nvdimm/ocxl-pmem.h | 32 +++
> > 3 files changed, 287 insertions(+)
> >
> > diff --git a/arch/powerpc/platforms/powernv/pmem/Kconfig
> > b/arch/powerpc/platforms/powernv/pmem/Kconfig
> > index c5d927520920..3f44429d70c9 100644
> > --- a/arch/powerpc/platforms/powernv/pmem/Kconfig
> > +++ b/arch/powerpc/platforms/powernv/pmem/Kconfig
> > @@ -12,4 +12,10 @@ config OCXL_PMEM
> >
> > Select N if unsure.
> >
> > +config OCXL_PMEM_DEBUG
> > + bool "OpenCAPI Persistent Memory debugging"
> > + depends on OCXL_PMEM
> > + help
> > + Enables low level IOCTLs for OpenCAPI Persistent Memory
> > firmware development
> > +
> > endif
> > diff --git a/arch/powerpc/platforms/powernv/pmem/ocxl.c
> > b/arch/powerpc/platforms/powernv/pmem/ocxl.c
> > index e01f6f9fc180..d4ce5e9e0521 100644
> > --- a/arch/powerpc/platforms/powernv/pmem/ocxl.c
> > +++ b/arch/powerpc/platforms/powernv/pmem/ocxl.c
> > @@ -1050,6 +1050,235 @@ int req_controller_health_perf(struct
> > ocxlpmem *ocxlpmem)
> > GLOBAL_MMIO_HCI_REQ_HEALTH_PERF);
> > }
> >
> > +#ifdef CONFIG_OCXL_PMEM_DEBUG
> > +/**
> > + * enable_fwdebug() - Enable FW debug on the controller
> > + * @ocxlpmem: the device metadata
> > + * Return: 0 on success, negative on failure
> > + */
> > +static int enable_fwdebug(const struct ocxlpmem *ocxlpmem)
> > +{
> > + return ocxl_global_mmio_set64(ocxlpmem->ocxl_afu,
> > GLOBAL_MMIO_HCI,
> > + OCXL_LITTLE_ENDIAN,
> > + GLOBAL_MMIO_HCI_FW_DEBUG);
> > +}
> > +
> > +/**
> > + * disable_fwdebug() - Disable FW debug on the controller
> > + * @ocxlpmem: the device metadata
> > + * Return: 0 on success, negative on failure
> > + */
> > +static int disable_fwdebug(const struct ocxlpmem *ocxlpmem)
> > +{
> > + return ocxl_global_mmio_set64(ocxlpmem->ocxl_afu,
> > GLOBAL_MMIO_HCIC,
> > + OCXL_LITTLE_ENDIAN,
> > + GLOBAL_MMIO_HCI_FW_DEBUG);
> > +}
> > +
> > +static int ioctl_fwdebug(struct ocxlpmem *ocxlpmem,
> > + struct ioctl_ocxl_pmem_fwdebug __user
> > *uarg)
> > +{
> > + struct ioctl_ocxl_pmem_fwdebug args;
> > + u64 val;
> > + int i;
> > + int rc;
> > +
> > + if (copy_from_user(&args, uarg, sizeof(args)))
> > + return -EFAULT;
> > +
> > + // Buffer size must be a multiple of 8
> > + if ((args.buf_size & 0x07))
> > + return -EINVAL;
> > +
> > + if (args.buf_size > ocxlpmem->admin_command.data_size)
> > + return -EINVAL;
> > +
> > + mutex_lock(&ocxlpmem->admin_command.lock);
> > +
> > + rc = enable_fwdebug(ocxlpmem);
> > + if (rc)
> > + goto out;
> > +
> > + rc = admin_command_request(ocxlpmem, ADMIN_COMMAND_FW_DEBUG);
> > + if (rc)
> > + goto out;
> > +
> > + // Write DebugAction & FunctionCode
> > + val = ((u64)args.debug_action << 56) | ((u64)args.function_code
> > << 40);
> > +
> > + rc = ocxl_global_mmio_write64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.request_offset + 0x08,
> > + OCXL_LITTLE_ENDIAN, val);
> > + if (rc)
> > + goto out;
> > +
> > + rc = ocxl_global_mmio_write64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.request_offset + 0x10,
> > + OCXL_LITTLE_ENDIAN,
> > args.debug_parameter_1);
> > + if (rc)
> > + goto out;
> > +
> > + rc = ocxl_global_mmio_write64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.request_offset + 0x18,
> > + OCXL_LITTLE_ENDIAN,
> > args.debug_parameter_2);
> > + if (rc)
> > + goto out;
> > +
> > + for (i = 0x20; i < 0x38; i += 0x08)
> > + rc = ocxl_global_mmio_write64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.request_offset + i,
> > + OCXL_LITTLE_ENDIAN, 0);
> > + if (rc)
> > + goto out;
>
> rc is the for loop body. The rc test is not.
>
Whoops :)
>
> > +
> > +
> > + // Populate admin command buffer
> > + if (args.buf_size) {
> > + for (i = 0; i < args.buf_size; i += sizeof(u64)) {
> > + u64 val;
> > +
> > + if (copy_from_user(&val, &args.buf[i],
> > sizeof(u64)))
> > + return -EFAULT;
>
> need to get rc and goto out because of the mutex
>
Ok
>
> > +
> > + rc = ocxl_global_mmio_write64(ocxlpmem-
> > >ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset + i,
> > + OCXL_HOST_ENDIAN,
> > val);
> > + if (rc)
> > + goto out;
> > + }
> > + }
> > +
> > + rc = admin_command_execute(ocxlpmem);
> > + if (rc)
> > + goto out;
> > +
> > + rc = admin_command_complete_timeout(ocxlpmem,
> > + ocxlpmem-
> > >timeouts[ADMIN_COMMAND_FW_DEBUG]);
> > + if (rc < 0)
> > + goto out;
> > +
> > + rc = admin_response(ocxlpmem);
> > + if (rc < 0)
> > + goto out;
> > + if (rc != STATUS_SUCCESS) {
> > + warn_status(ocxlpmem, "Unexpected status from FW
> > Debug", rc);
> > + goto out;
> > + }
> > +
> > + if (args.buf_size) {
> > + for (i = 0; i < args.buf_size; i += sizeof(u64)) {
> > + u64 val;
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem-
> > >ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset + i,
> > + OCXL_HOST_ENDIAN,
> > &val);
> > + if (rc)
> > + goto out;
> > +
> > + if (copy_to_user(&args.buf[i], &val,
> > sizeof(u64))) {
> > + rc = -EFAULT;
> > + goto out;
> > + }
> > + }
> > + }
> > +
> > + rc = admin_response_handled(ocxlpmem);
> > + if (rc)
> > + goto out;
> > +
> > + rc = disable_fwdebug(ocxlpmem);
> > + if (rc)
> > + goto out;
> > +
> > +out:
> > + mutex_unlock(&ocxlpmem->admin_command.lock);
> > + return rc;
> > +}
> > +
> > +static int ioctl_shutdown(struct ocxlpmem *ocxlpmem)
> > +{
> > + int rc;
> > +
> > + mutex_lock(&ocxlpmem->admin_command.lock);
> > +
> > + rc = admin_command_request(ocxlpmem, ADMIN_COMMAND_SHUTDOWN);
> > + if (rc)
> > + goto out;
> > +
> > + rc = admin_command_execute(ocxlpmem);
> > + if (rc)
> > + goto out;
> > +
> > + rc = admin_command_complete_timeout(ocxlpmem,
> > ADMIN_COMMAND_SHUTDOWN);
> > + if (rc < 0) {
> > + dev_warn(&ocxlpmem->dev, "Shutdown timed out\n");
> > + goto out;
> > + }
> > +
> > + rc = 0;
> > + goto out;
>
> We can remove that goto.
Ok
>
> No admin_response_handled()? Is that shutting down the full adapter
> and
> we have nobody to talk to? What happens next?
>
That's an oversight, we should call admin_response_handled().
>
> > +
> > +out:
> > + mutex_unlock(&ocxlpmem->admin_command.lock);
> > + return rc;
> > +}
> > +
> > +static int ioctl_mmio_write(struct ocxlpmem *ocxlpmem,
> > + struct ioctl_ocxl_pmem_mmio __user
> > *uarg)
> > +{
> > + struct scm_ioctl_mmio args;
> > +
> > + if (copy_from_user(&args, uarg, sizeof(args)))
> > + return -EFAULT;
> > +
> > + return ocxl_global_mmio_write64(ocxlpmem->ocxl_afu,
> > args.address,
> > + OCXL_LITTLE_ENDIAN, args.val);
> > +}
> > +
> > +static int ioctl_mmio_read(struct ocxlpmem *ocxlpmem,
> > + struct ioctl_ocxl_pmem_mmio __user
> > *uarg)
> > +{
> > + struct ioctl_ocxl_pmem_mmio args;
> > + int rc;
> > +
> > + if (copy_from_user(&args, uarg, sizeof(args)))
> > + return -EFAULT;
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu, args.address,
> > + OCXL_LITTLE_ENDIAN, &args.val);
> > + if (rc)
> > + return rc;
> > +
> > + if (copy_to_user(uarg, &args, sizeof(args)))
> > + return -EFAULT;
> > +
> > + return 0;
> > +}
> > +#else /* CONFIG_OCXL_PMEM_DEBUG */
> > +static int ioctl_fwdebug(struct ocxlpmem *ocxlpmem,
> > + struct ioctl_ocxl_pmem_fwdebug __user
> > *uarg)
> > +{
> > + return -EPERM;
> > +}
> > +
> > +static int ioctl_shutdown(struct ocxlpmem *ocxlpmem)
> > +{
> > + return -EPERM;
> > +}
> > +
> > +static int ioctl_mmio_write(struct ocxlpmem *ocxlpmem,
> > + struct ioctl_ocxl_pmem_mmio __user
> > *uarg)
> > +{
> > + return -EPERM;
> > +}
> > +
> > +static int ioctl_mmio_read(struct ocxlpmem *ocxlpmem,
> > + struct ioctl_ocxl_pmem_mmio __user
> > *uarg)
> > +{
> > + return -EPERM;
> > +}
>
> The 'else' clause could be dropped, the ioctls will return EINVAL,
> which
> is fine, I think.
>
>
Ok
>
> > +#endif /* CONFIG_OCXL_PMEM_DEBUG */
> > +
> > static long file_ioctl(struct file *file, unsigned int cmd,
> > unsigned long args)
> > {
> > struct ocxlpmem *ocxlpmem = file->private_data;
> > @@ -1091,6 +1320,26 @@ static long file_ioctl(struct file *file,
> > unsigned int cmd, unsigned long args)
> > case IOCTL_OCXL_PMEM_REQUEST_HEALTH:
> > rc = req_controller_health_perf(ocxlpmem);
> > break;
> > +
> > + case IOCTL_OCXL_PMEM_FWDEBUG:
> > + rc = ioctl_fwdebug(ocxlpmem,
> > + (struct ioctl_ocxl_pmem_fwdebug
> > __user *)args);
> > + break;
> > +
> > + case IOCTL_OCXL_PMEM_SHUTDOWN:
> > + rc = ioctl_shutdown(ocxlpmem);
> > + break;
> > +
> > + case IOCTL_OCXL_PMEM_MMIO_WRITE:
> > + rc = ioctl_mmio_write(ocxlpmem,
> > + (struct ioctl_ocxl_pmem_mmio
> > __user *)args);
> > + break;
> > +
> > + case IOCTL_OCXL_PMEM_MMIO_READ:
> > + rc = ioctl_mmio_read(ocxlpmem,
> > + (struct ioctl_ocxl_pmem_mmio
> > __user *)args);
> > + break;
> > +
> > }
> >
> > return rc;
> > diff --git a/include/uapi/nvdimm/ocxl-pmem.h
> > b/include/uapi/nvdimm/ocxl-pmem.h
> > index 0d03abb44001..e20a4f8be82a 100644
> > --- a/include/uapi/nvdimm/ocxl-pmem.h
> > +++ b/include/uapi/nvdimm/ocxl-pmem.h
> > @@ -6,6 +6,28 @@
> > #include <linux/types.h>
> > #include <linux/ioctl.h>
> >
> > +enum ocxlpmem_fwdebug_action {
> > + OCXL_PMEM_FWDEBUG_READ_CONTROLLER_MEMORY = 0x01,
> > + OCXL_PMEM_FWDEBUG_WRITE_CONTROLLER_MEMORY = 0x02,
> > + OCXL_PMEM_FWDEBUG_ENABLE_FUNCTION = 0x03,
> > + OCXL_PMEM_FWDEBUG_DISABLE_FUNCTION = 0x04,
> > + OCXL_PMEM_FWDEBUG_GET_PEL = 0x05, // Retrieve Persistent Error
> > Log
> > +};
> > +
> > +struct ioctl_ocxl_pmem_buffer_info {
> > + __u32 admin_command_buffer_size; // out
> > + __u32 near_storage_buffer_size; // out
> > +};
> > +
> > +struct ioctl_ocxl_pmem_fwdebug { // All args are inputs
> > + enum ocxlpmem_fwdebug_action debug_action;
>
> More kernel ABI problems. My interpretation of the "enumeration
> specifiers" section of C99 is that we can't rely on the size of the
> enum.
>
Ok
>
> > + __u16 function_code;
> > + __u16 buf_size; // Size of optional data buffer
> > + __u64 debug_parameter_1;
> > + __u64 debug_parameter_2;
> > + __u8 *buf; // Pointer to optional in/out data buffer
> > +};
> > +
> > #define OCXL_PMEM_ERROR_LOG_ACTION_RESET (1 << (32-32))
> > #define OCXL_PMEM_ERROR_LOG_ACTION_CHKFW (1 << (53-32))
> > #define OCXL_PMEM_ERROR_LOG_ACTION_REPLACE (1 << (54-32))
> > @@ -66,6 +88,11 @@ struct ioctl_ocxl_pmem_controller_stats {
> > __u64 cache_write_latency; /* nanoseconds */
> > };
> >
> > +struct ioctl_ocxl_pmem_mmio {
> > + __u64 address; /* Offset in global MMIO space */
> > + __u64 val; /* value to write/was read */
> > +};
>
> Can we group all the debug data structures together in the header
> file,
> with a comment indicating that they may not be available in the
> kernel,
> depending on the config?
>
Ok
> Fred
>
>
> > +
> > struct ioctl_ocxl_pmem_eventfd {
> > __s32 eventfd;
> > __u32 reserved;
> > @@ -92,4 +119,9 @@ struct ioctl_ocxl_pmem_eventfd {
> > #define IOCTL_OCXL_PMEM_EVENT_CHECK _IOR(OC
> > XL_PMEM_MAGIC, 0x07, __u64)
> > #define IOCTL_OCXL_PMEM_REQUEST_HEALTH _IO(OCX
> > L_PMEM_MAGIC, 0x08)
> >
> > +#define IOCTL_OCXL_PMEM_FWDEBUG _IOWR(OCXL_PMEM_MAGIC,
> > 0xf0, struct ioctl_ocxl_pmem_fwdebug)
> > +#define IOCTL_OCXL_PMEM_MMIO_WRITE _IOW(OCXL_PMEM_MAGIC, 0xf1,
> > struct ioctl_ocxl_pmem_mmio)
> > +#define IOCTL_OCXL_PMEM_MMIO_READ _IOWR(OCXL_PMEM_MAGIC, 0xf2,
> > struct ioctl_ocxl_pmem_mmio)
> > +#define IOCTL_OCXL_PMEM_SHUTDOWN _IO(OCXL_PMEM_MAGIC, 0xf3)
> > +
> > #endif /* _UAPI_OCXL_SCM_H */
> >
--
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819
^ permalink raw reply
* Re: [PATCH v4 1/5] mm/memremap_pages: Introduce memremap_compat_align()
From: Michael Ellerman @ 2020-03-12 3:17 UTC (permalink / raw)
To: Dan Williams, linux-nvdimm
Cc: Aneesh Kumar K.V, linux-kernel, Jeff Moyer, Paul Mackerras,
vishal.l.verma, linuxppc-dev
In-Reply-To: <158328768844.2223916.3587427265166021149.stgit@dwillia2-desk3.amr.corp.intel.com>
Dan Williams <dan.j.williams@intel.com> writes:
> The "sub-section memory hotplug" facility allows memremap_pages() users
> like libnvdimm to compensate for hardware platforms like x86 that have a
> section size larger than their hardware memory mapping granularity. The
> compensation that sub-section support affords is being tolerant of
> physical memory resources shifting by units smaller (64MiB on x86) than
> the memory-hotplug section size (128 MiB). Where the platform
> physical-memory mapping granularity is limited by the number and
> capability of address-decode-registers in the memory controller.
>
> While the sub-section support allows memremap_pages() to operate on
> sub-section (2MiB) granularity, the Power architecture may still
> require 16MiB alignment on "!radix_enabled()" platforms.
>
> In order for libnvdimm to be able to detect and manage this per-arch
> limitation, introduce memremap_compat_align() as a common minimum
> alignment across all driver-facing memory-mapping interfaces, and let
> Power override it to 16MiB in the "!radix_enabled()" case.
>
> The assumption / requirement for 16MiB to be a viable
> memremap_compat_align() value is that Power does not have platforms
> where its equivalent of address-decode-registers never hardware remaps a
> persistent memory resource on smaller than 16MiB boundaries. Note that I
> tried my best to not add a new Kconfig symbol, but header include
> entanglements defeated the #ifndef memremap_compat_align design pattern
> and the need to export it defeats the __weak design pattern for arch
> overrides.
>
> Based on an initial patch by Aneesh.
>
> Link: http://lore.kernel.org/r/CAPcyv4gBGNP95APYaBcsocEa50tQj9b5h__83vgngjq3ouGX_Q@mail.gmail.com
> Reported-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Reported-by: Jeff Moyer <jmoyer@redhat.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> arch/powerpc/Kconfig | 1 +
> arch/powerpc/mm/ioremap.c | 21 +++++++++++++++++++++
> drivers/nvdimm/pfn_devs.c | 2 +-
> include/linux/memremap.h | 8 ++++++++
> include/linux/mmzone.h | 1 +
> lib/Kconfig | 3 +++
> mm/memremap.c | 23 +++++++++++++++++++++++
> 7 files changed, 58 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 497b7d0b2d7e..e6ffe905e2b9 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -122,6 +122,7 @@ config PPC
> select ARCH_HAS_GCOV_PROFILE_ALL
> select ARCH_HAS_KCOV
> select ARCH_HAS_HUGEPD if HUGETLB_PAGE
> + select ARCH_HAS_MEMREMAP_COMPAT_ALIGN
> select ARCH_HAS_MMIOWB if PPC64
> select ARCH_HAS_PHYS_TO_DMA
> select ARCH_HAS_PMEM_API
> diff --git a/arch/powerpc/mm/ioremap.c b/arch/powerpc/mm/ioremap.c
> index fc669643ce6a..b1a0aebe8c48 100644
> --- a/arch/powerpc/mm/ioremap.c
> +++ b/arch/powerpc/mm/ioremap.c
> @@ -2,6 +2,7 @@
>
> #include <linux/io.h>
> #include <linux/slab.h>
> +#include <linux/mmzone.h>
> #include <linux/vmalloc.h>
> #include <asm/io-workarounds.h>
>
> @@ -97,3 +98,23 @@ void __iomem *do_ioremap(phys_addr_t pa, phys_addr_t offset, unsigned long size,
>
> return NULL;
> }
> +
> +#ifdef CONFIG_ZONE_DEVICE
> +/*
> + * Override the generic version in mm/memremap.c.
> + *
> + * With hash translation, the direct-map range is mapped with just one
> + * page size selected by htab_init_page_sizes(). Consult
> + * mmu_psize_defs[] to determine the minimum page size alignment.
> +*/
> +unsigned long memremap_compat_align(void)
> +{
> + unsigned int shift = mmu_psize_defs[mmu_linear_psize].shift;
> +
> + if (radix_enabled())
> + return SUBSECTION_SIZE;
> + return max(SUBSECTION_SIZE, 1UL << shift);
> +
> +}
> +EXPORT_SYMBOL_GPL(memremap_compat_align);
> +#endif
LGTM.
Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
cheers
^ permalink raw reply
* Re: [PATCH v3 19/27] powerpc/powernv/pmem: Add an IOCTL to report controller statistics
From: Alastair D'Silva @ 2020-03-12 0:15 UTC (permalink / raw)
To: Frederic Barrat
Cc: Madhavan Srinivasan, Alexey Kardashevskiy, Masahiro Yamada,
Oliver O'Halloran, Mauro Carvalho Chehab, Ira Weiny,
Thomas Gleixner, Rob Herring, Dave Jiang, linux-nvdimm,
Aneesh Kumar K . V, Krzysztof Kozlowski, Anju T Sudhakar,
Mahesh Salgaonkar, Andrew Donnellan, Arnd Bergmann, Greg Kurz,
Nicholas Piggin, Cédric Le Goater, Dan Williams,
Hari Bathini, linux-mm, Greg Kroah-Hartman, linux-kernel,
Vishal Verma, Paul Mackerras, Andrew Morton, linuxppc-dev,
David S. Miller
In-Reply-To: <6ee036c7-f4ea-4e42-faad-66a1921553ce@linux.ibm.com>
On Wed, 2020-03-04 at 10:25 +0100, Frederic Barrat wrote:
>
> Le 21/02/2020 à 04:27, Alastair D'Silva a écrit :
> > From: Alastair D'Silva <alastair@d-silva.org>
> >
> > The controller can report a number of statistics that are useful
> > in evaluating the performance and reliability of the card.
> >
> > This patch exposes this information via an IOCTL.
> >
> > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> > ---
> > arch/powerpc/platforms/powernv/pmem/ocxl.c | 185
> > +++++++++++++++++++++
> > include/uapi/nvdimm/ocxl-pmem.h | 17 ++
> > 2 files changed, 202 insertions(+)
> >
> > diff --git a/arch/powerpc/platforms/powernv/pmem/ocxl.c
> > b/arch/powerpc/platforms/powernv/pmem/ocxl.c
> > index 2cabafe1fc58..009d4fd29e7d 100644
> > --- a/arch/powerpc/platforms/powernv/pmem/ocxl.c
> > +++ b/arch/powerpc/platforms/powernv/pmem/ocxl.c
> > @@ -758,6 +758,186 @@ static int
> > ioctl_controller_dump_complete(struct ocxlpmem *ocxlpmem)
> > GLOBAL_MMIO_HCI_CONTROLLER_DUMP_COL
> > LECTED);
> > }
> >
> > +/**
> > + * controller_stats_header_parse() - Parse the first 64 bits of
> > the controller stats admin command response
> > + * @ocxlpmem: the device metadata
> > + * @length: out, returns the number of bytes in the response
> > (excluding the 64 bit header)
> > + */
> > +static int controller_stats_header_parse(struct ocxlpmem
> > *ocxlpmem,
> > + u32 *length)
> > +{
> > + int rc;
> > + u64 val;
> > +
>
> unexpected empty line
>
Ok
>
> > + u16 data_identifier;
> > + u32 data_length;
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset,
> > + OCXL_LITTLE_ENDIAN, &val);
> > + if (rc)
> > + return rc;
> > +
> > + data_identifier = val >> 48;
> > + data_length = val & 0xFFFFFFFF;
> > +
> > + if (data_identifier != 0x4353) { // 'CS'
> > + dev_err(&ocxlpmem->dev,
> > + "Bad data identifier for controller stats,
> > expected 'CS', got '%-.*s'\n",
> > + 2, (char *)&data_identifier);
>
>
> Wow, I'm clueless what that string format looks like :-)
> 2 arguments? Did you check the kernel string formatter does what you
> want?
> You may consider unifying the format though, the error log patch uses
> a
> simpler (better?) format for a similar message.
>
Sorry, force of habit from my old job where we dealt with a lot of
variable length, non-NULL terminated buffers. FYI - it takes the string
length from the first argument.
I'll change it to a fixed length string like the others :)
>
>
> > + return -EINVAL;
> > + }
> > +
> > + *length = data_length;
> > + return 0;
> > +}
> > +
> > +static int ioctl_controller_stats(struct ocxlpmem *ocxlpmem,
> > + struct
> > ioctl_ocxl_pmem_controller_stats __user *uarg)
> > +{
> > + struct ioctl_ocxl_pmem_controller_stats args;
> > + u32 length;
> > + int rc;
> > + u64 val;
> > +
> > + memset(&args, '\0', sizeof(args));
> > +
> > + mutex_lock(&ocxlpmem->admin_command.lock);
> > +
> > + rc = admin_command_request(ocxlpmem,
> > ADMIN_COMMAND_CONTROLLER_STATS);
> > + if (rc)
> > + goto out;
> > +
> > + rc = ocxl_global_mmio_write64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.request_offset + 0x08,
> > + OCXL_LITTLE_ENDIAN, 0);
> > + if (rc)
> > + goto out;
> > +
> > + rc = admin_command_execute(ocxlpmem);
> > + if (rc)
> > + goto out;
> > +
> > +
> > + rc = admin_command_complete_timeout(ocxlpmem,
> > + ADMIN_COMMAND_CONTROLLER_ST
> > ATS);
> > + if (rc < 0) {
> > + dev_warn(&ocxlpmem->dev, "Controller stats timed
> > out\n");
> > + goto out;
> > + }
> > +
> > + rc = admin_response(ocxlpmem);
> > + if (rc < 0)
> > + goto out;
> > + if (rc != STATUS_SUCCESS) {
> > + warn_status(ocxlpmem,
> > + "Unexpected status from controller stats",
> > rc);
> > + goto out;
> > + }
>
> All those ioctls commands follow the same pattern:
> 1. admin_command_request()
> 2. optionnaly, set some mmio registers specific to the command
> 3. admin_command_execute()
> 4. admin_command_complete_timeout()
> 5. admin_response()
>
> By swapping 1 and 2, we could then factorize steps 1, 3, 4 and 5 in
> a
> function and simplify/shorten the code each time a command is called.
>
> Regarding step 2 (and that's true for all similar patches), a
> comment
> about what the mmio tuning does would help and avoid looking up the
> spec. Looking up the spec during the review is expected, but it will
> ease reading the code 6 months from now.
>
>
I'll rework this and add a wrapper in the Admin Commands patch.
>
> > +
> > + rc = controller_stats_header_parse(ocxlpmem, &length);
> > + if (rc)
> > + goto out;
> > +
> > + if (length != 0x140)
> > + warn_status(ocxlpmem,
> > + "Unexpected length for controller stats
> > data, expected 0x140, got 0x%x",
> > + length);
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset + 0x08 + 0x08,
> > + OCXL_LITTLE_ENDIAN, &val);
> > + if (rc)
> > + goto out;
> > +
> > + args.reset_count = val >> 32;
> > + args.reset_uptime = val & 0xFFFFFFFF;
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset + 0x08 + 0x10,
> > + OCXL_LITTLE_ENDIAN, &val);
> > + if (rc)
> > + goto out;
> > +
> > + args.power_on_uptime = val >> 32;
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset + 0x08 + 0x40 + 0x08,
> > + OCXL_LITTLE_ENDIAN,
> &args.host_load_count);
>
>
> Those offsets are hard to understand, even with the spec next to me.
> And
> it seems that we could harden things a bit:
> each block as a "statistics parameter ID" and the length of the data
> for
> that block. We should check that and make sure we're reading what we
> expect.
> For example, from the spec I'm looking (110d), I would expect the
> host
> load count to be at offset 0x10. It's entirely possible I'm
> misreading
> it though.
>
I'll rework this too.
>
>
> > + if (rc)
> > + goto out;
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset + 0x08 + 0x40 + 0x10,
> > + OCXL_LITTLE_ENDIAN,
> > &args.host_store_count);
> > + if (rc)
> > + goto out;
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset + 0x08 + 0x40 + 0x18,
> > + OCXL_LITTLE_ENDIAN,
> > &args.media_read_count);
> > + if (rc)
> > + goto out;
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset + 0x08 + 0x40 + 0x20,
> > + OCXL_LITTLE_ENDIAN,
> > &args.media_write_count);
> > + if (rc)
> > + goto out;
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset + 0x08 + 0x40 + 0x28,
> > + OCXL_LITTLE_ENDIAN,
> > &args.cache_hit_count);
> > + if (rc)
> > + goto out;
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset + 0x08 + 0x40 + 0x30,
> > + OCXL_LITTLE_ENDIAN,
> > &args.cache_miss_count);
> > + if (rc)
> > + goto out;
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset + 0x08 + 0x40 + 0x38,
> > + OCXL_LITTLE_ENDIAN,
> > &args.media_read_latency);
> > + if (rc)
> > + goto out;
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset + 0x08 + 0x40 + 0x40,
> > + OCXL_LITTLE_ENDIAN,
> > &args.media_write_latency);
> > + if (rc)
> > + goto out;
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset + 0x08 + 0x40 + 0x48,
> > + OCXL_LITTLE_ENDIAN,
> > &args.cache_read_latency);
> > + if (rc)
> > + goto out;
> > +
> > + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset + 0x08 + 0x40 + 0x50,
> > + OCXL_LITTLE_ENDIAN,
> > &args.cache_write_latency);
> > + if (rc)
> > + goto out;
> > +
> > + if (copy_to_user(uarg, &args, sizeof(args))) {
> > + rc = -EFAULT;
> > + goto out;
> > + }
> > +
> > + rc = admin_response_handled(ocxlpmem);
> > + if (rc)
> > + goto out;
> > +
> > + rc = 0;
> > + goto out;
>
> That may be more of a personal habit, but that final goto disrupts
> the
> "good case" flow. And I think it's pretty unusual within the kernel.
>
Ok
>
> > +
> > +out:
> > + mutex_unlock(&ocxlpmem->admin_command.lock);
> > + return rc;
> > +}
> > +
> > static long file_ioctl(struct file *file, unsigned int cmd,
> > unsigned long args)
> > {
> > struct ocxlpmem *ocxlpmem = file->private_data;
> > @@ -781,6 +961,11 @@ static long file_ioctl(struct file *file,
> > unsigned int cmd, unsigned long args)
> > case IOCTL_OCXL_PMEM_CONTROLLER_DUMP_COMPLETE:
> > rc = ioctl_controller_dump_complete(ocxlpmem);
> > break;
> > +
> > + case IOCTL_OCXL_PMEM_CONTROLLER_STATS:
> > + rc = ioctl_controller_stats(ocxlpmem,
> > + (struct
> > ioctl_ocxl_pmem_controller_stats __user *)args);
> > + break;
> > }
> >
> > return rc;
> > diff --git a/include/uapi/nvdimm/ocxl-pmem.h
> > b/include/uapi/nvdimm/ocxl-pmem.h
> > index d4d8512d03f7..add223aa2fdb 100644
> > --- a/include/uapi/nvdimm/ocxl-pmem.h
> > +++ b/include/uapi/nvdimm/ocxl-pmem.h
> > @@ -50,6 +50,22 @@ struct ioctl_ocxl_pmem_controller_dump_data {
> > __u64 reserved[8];
> > };
> >
> > +struct ioctl_ocxl_pmem_controller_stats {
> > + __u32 reset_count;
> > + __u32 reset_uptime; /* seconds */
> > + __u32 power_on_uptime; /* seconds */
>
> Same as before, we're going to have some padding here.
>
> Fred
>
Ok
>
> > + __u64 host_load_count;
> > + __u64 host_store_count;
> > + __u64 media_read_count;
> > + __u64 media_write_count;
> > + __u64 cache_hit_count;
> > + __u64 cache_miss_count;
> > + __u64 media_read_latency; /* nanoseconds */
> > + __u64 media_write_latency; /* nanoseconds */
> > + __u64 cache_read_latency; /* nanoseconds */
> > + __u64 cache_write_latency; /* nanoseconds */
> > +};
> > +
> > /* ioctl numbers */
> > #define OCXL_PMEM_MAGIC 0x5C
> > /* SCM devices */
> > @@ -57,5 +73,6 @@ struct ioctl_ocxl_pmem_controller_dump_data {
> > #define IOCTL_OCXL_PMEM_CONTROLLER_DUMP _IO(OCX
> > L_PMEM_MAGIC, 0x02)
> > #define IOCTL_OCXL_PMEM_CONTROLLER_DUMP_DATA _IOWR(O
> > CXL_PMEM_MAGIC, 0x03, struct ioctl_ocxl_pmem_controller_dump_data)
> > #define IOCTL_OCXL_PMEM_CONTROLLER_DUMP_COMPLETE _IO(OCXL_PMEM_M
> > AGIC, 0x04)
> > +#define IOCTL_OCXL_PMEM_CONTROLLER_STATS _IO(OCXL_PMEM_M
> > AGIC, 0x05)
> >
> > #endif /* _UAPI_OCXL_SCM_H */
> >
--
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819
^ permalink raw reply
* [RFC PATCH v1] pseries/drmem: don't cache node id in drmem_lmb struct
From: Scott Cheloha @ 2020-03-11 23:08 UTC (permalink / raw)
To: linuxppc-dev
Cc: Nathan Lynch, Nathan Fontenont, David Hildenbrand, Paul Mackerras,
Aneesh Kumar, Rick Lindsley
At memory hot-remove time we can retrieve an LMB's nid from its
corresponding memory_block. There is no need to store the nid
in multiple locations.
Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com>
---
The linear search in powerpc's memory_add_physaddr_to_nid() has become a
bottleneck at boot on systems with many LMBs.
As described in this patch here:
https://lore.kernel.org/linuxppc-dev/20200221172901.1596249-2-cheloha@linux.ibm.com/
the linear search seriously cripples drmem_init().
The obvious solution (shown in that patch) is to just make the search
in memory_add_physaddr_to_nid() faster. An XArray seems well-suited
to the task of mapping an address range to an LMB object.
The less obvious approach is to just call memory_add_physaddr_to_nid()
in fewer places.
I'm not sure which approach is correct, hence the RFC.
The attached patch is an example of how we could just eliminate the
memory_add_physaddr_to_nid() calls during drmem_init(). Unless this
is somehow an abuse of the memory_block I think retrieving the nid in
this way deduplicates memoization.
There is another spot where we don't *need* to search by address to
find the node id. In dlpar_memory_add() we're calling lmb_set_nid(),
which is just memory_add_physaddr_to_nid(). But we could easily bypass
the search and call of_drconf_to_nid_single() directly with the LMB we
already have a pointer to.
Then again, I see callers like the xen balloon driver and probe_store()
that seemingly need to do an address-to-nid lookup.
So, does memory_add_physaddr_to_nid() need to be fast? Should I bite
the bullet and make it fast so we can leverage it without slowdown?
Or is calling it in fewer places an acceptable approach?
arch/powerpc/include/asm/drmem.h | 21 -------------------
arch/powerpc/mm/drmem.c | 6 +-----
.../platforms/pseries/hotplug-memory.c | 19 ++++++++++-------
3 files changed, 13 insertions(+), 33 deletions(-)
diff --git a/arch/powerpc/include/asm/drmem.h b/arch/powerpc/include/asm/drmem.h
index 3d76e1c388c2..4e7b6b70e366 100644
--- a/arch/powerpc/include/asm/drmem.h
+++ b/arch/powerpc/include/asm/drmem.h
@@ -13,9 +13,6 @@ struct drmem_lmb {
u32 drc_index;
u32 aa_index;
u32 flags;
-#ifdef CONFIG_MEMORY_HOTPLUG
- int nid;
-#endif
};
struct drmem_lmb_info {
@@ -103,22 +100,4 @@ static inline void invalidate_lmb_associativity_index(struct drmem_lmb *lmb)
lmb->aa_index = 0xffffffff;
}
-#ifdef CONFIG_MEMORY_HOTPLUG
-static inline void lmb_set_nid(struct drmem_lmb *lmb)
-{
- lmb->nid = memory_add_physaddr_to_nid(lmb->base_addr);
-}
-static inline void lmb_clear_nid(struct drmem_lmb *lmb)
-{
- lmb->nid = -1;
-}
-#else
-static inline void lmb_set_nid(struct drmem_lmb *lmb)
-{
-}
-static inline void lmb_clear_nid(struct drmem_lmb *lmb)
-{
-}
-#endif
-
#endif /* _ASM_POWERPC_LMB_H */
diff --git a/arch/powerpc/mm/drmem.c b/arch/powerpc/mm/drmem.c
index 59327cefbc6a..873fcfc7b875 100644
--- a/arch/powerpc/mm/drmem.c
+++ b/arch/powerpc/mm/drmem.c
@@ -362,10 +362,8 @@ static void __init init_drmem_v1_lmbs(const __be32 *prop)
if (!drmem_info->lmbs)
return;
- for_each_drmem_lmb(lmb) {
+ for_each_drmem_lmb(lmb)
read_drconf_v1_cell(lmb, &prop);
- lmb_set_nid(lmb);
- }
}
static void __init init_drmem_v2_lmbs(const __be32 *prop)
@@ -410,8 +408,6 @@ static void __init init_drmem_v2_lmbs(const __be32 *prop)
lmb->aa_index = dr_cell.aa_index;
lmb->flags = dr_cell.flags;
-
- lmb_set_nid(lmb);
}
}
}
diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index a4d40a3ceea3..f6d4236286cf 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -376,25 +376,29 @@ static int dlpar_add_lmb(struct drmem_lmb *);
static int dlpar_remove_lmb(struct drmem_lmb *lmb)
{
+ struct memory_block *mem_block;
unsigned long block_sz;
int rc;
if (!lmb_is_removable(lmb))
return -EINVAL;
+ mem_block = lmb_to_memblock(lmb);
+ if (mem_block == NULL)
+ return -EINVAL;
+
rc = dlpar_offline_lmb(lmb);
if (rc)
return rc;
block_sz = pseries_memory_block_size();
- __remove_memory(lmb->nid, lmb->base_addr, block_sz);
+ __remove_memory(mem_block->nid, lmb->base_addr, block_sz);
/* Update memory regions for memory remove */
memblock_remove(lmb->base_addr, block_sz);
invalidate_lmb_associativity_index(lmb);
- lmb_clear_nid(lmb);
lmb->flags &= ~DRCONF_MEM_ASSIGNED;
return 0;
@@ -651,7 +655,7 @@ static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index)
static int dlpar_add_lmb(struct drmem_lmb *lmb)
{
unsigned long block_sz;
- int rc;
+ int nid, rc;
if (lmb->flags & DRCONF_MEM_ASSIGNED)
return -EINVAL;
@@ -662,11 +666,13 @@ static int dlpar_add_lmb(struct drmem_lmb *lmb)
return rc;
}
- lmb_set_nid(lmb);
block_sz = memory_block_size_bytes();
+ /* Find the node id for this address. */
+ nid = memory_add_physaddr_to_nid(lmb->base_addr);
+
/* Add the memory */
- rc = __add_memory(lmb->nid, lmb->base_addr, block_sz);
+ rc = __add_memory(nid, lmb->base_addr, block_sz);
if (rc) {
invalidate_lmb_associativity_index(lmb);
return rc;
@@ -674,9 +680,8 @@ static int dlpar_add_lmb(struct drmem_lmb *lmb)
rc = dlpar_online_lmb(lmb);
if (rc) {
- __remove_memory(lmb->nid, lmb->base_addr, block_sz);
+ __remove_memory(nid, lmb->base_addr, block_sz);
invalidate_lmb_associativity_index(lmb);
- lmb_clear_nid(lmb);
} else {
lmb->flags |= DRCONF_MEM_ASSIGNED;
}
--
2.24.1
^ permalink raw reply related
* Re: [PATCH v3] ima: add a new CONFIG for loading arch-specific policies
From: Philipp Rudo @ 2020-03-11 18:35 UTC (permalink / raw)
To: Nayna Jain
Cc: linux-s390, linux-efi, Nayna Jain, x86, linux-kernel, zohar,
Thomas Gleixner, linux-integrity, linuxppc-dev, Ard Biesheuvel
In-Reply-To: <1583715471-15525-1-git-send-email-nayna@linux.ibm.com>
On Sun, 8 Mar 2020 20:57:51 -0400
Nayna Jain <nayna@linux.ibm.com> wrote:
> From: Nayna Jain <nayna@linux.vnet.ibm.com>
>
> Every time a new architecture defines the IMA architecture specific
> functions - arch_ima_get_secureboot() and arch_ima_get_policy(), the IMA
> include file needs to be updated. To avoid this "noise", this patch
> defines a new IMA Kconfig IMA_SECURE_AND_OR_TRUSTED_BOOT option, allowing
> the different architectures to select it.
>
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
> Acked-by: Ard Biesheuvel <ardb@kernel.org>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Philipp Rudo <prudo@linux.ibm.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> ---
> v3:
> * Removes CONFIG_IMA dependency. Thanks Ard.
> * Updated the patch with improvements suggested by Michael. It now uses
> "imply" instead of "select". Thanks Michael.
> * Replaced the CONFIG_IMA in x86 and s390 with new config, else it was
> resulting in redefinition when the IMA_SECURE_AND_OR_TRUSTED_BOOT
> is not enabled. Thanks to Mimi for identifying the problem.
> * Removed "#ifdef EFI" check in the arch/x86/Makefile for compiling
> ima_arch.c file.
> * Ard, Thanks for your Acked-by. I have changed the arch/x86/Makefile in
> this version. Can you please review again and confirm ?
> * Rudo, Thanks for your review. I have changed arch/s390/Makefile as well.
> Can you also please review again ?
Looks good to me
for the s390 part
Acked-by: Philipp Rudo <prudo@linux.ibm.com>
Thanks
Philipp
>
> v2:
> * Fixed the issue identified by Mimi. Thanks Mimi, Ard, Heiko and Michael for
> discussing the fix.
>
> arch/powerpc/Kconfig | 1 +
> arch/s390/Kconfig | 1 +
> arch/s390/kernel/Makefile | 2 +-
> arch/x86/Kconfig | 1 +
> arch/x86/kernel/Makefile | 4 +---
> include/linux/ima.h | 3 +--
> security/integrity/ima/Kconfig | 7 +++++++
> 7 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 497b7d0b2d7e..5b9f1cba2a44 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -979,6 +979,7 @@ config PPC_SECURE_BOOT
> bool
> depends on PPC_POWERNV
> depends on IMA_ARCH_POLICY
> + imply IMA_SECURE_AND_OR_TRUSTED_BOOT
> help
> Systems with firmware secure boot enabled need to define security
> policies to extend secure boot to the OS. This config allows a user
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index 8abe77536d9d..59c216af6264 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -195,6 +195,7 @@ config S390
> select ARCH_HAS_FORCE_DMA_UNENCRYPTED
> select SWIOTLB
> select GENERIC_ALLOCATOR
> + imply IMA_SECURE_AND_OR_TRUSTED_BOOT
>
>
> config SCHED_OMIT_FRAME_POINTER
> diff --git a/arch/s390/kernel/Makefile b/arch/s390/kernel/Makefile
> index 2b1203cf7be6..578a6fa82ea4 100644
> --- a/arch/s390/kernel/Makefile
> +++ b/arch/s390/kernel/Makefile
> @@ -70,7 +70,7 @@ obj-$(CONFIG_JUMP_LABEL) += jump_label.o
> obj-$(CONFIG_KEXEC_FILE) += machine_kexec_file.o kexec_image.o
> obj-$(CONFIG_KEXEC_FILE) += kexec_elf.o
>
> -obj-$(CONFIG_IMA) += ima_arch.o
> +obj-$(CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT) += ima_arch.o
>
> obj-$(CONFIG_PERF_EVENTS) += perf_event.o perf_cpum_cf_common.o
> obj-$(CONFIG_PERF_EVENTS) += perf_cpum_cf.o perf_cpum_sf.o
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index beea77046f9b..dcf5b1729f7c 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -230,6 +230,7 @@ config X86
> select VIRT_TO_BUS
> select X86_FEATURE_NAMES if PROC_FS
> select PROC_PID_ARCH_STATUS if PROC_FS
> + imply IMA_SECURE_AND_OR_TRUSTED_BOOT if EFI
>
> config INSTRUCTION_DECODER
> def_bool y
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index 9b294c13809a..cfef37a27def 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -154,6 +154,4 @@ ifeq ($(CONFIG_X86_64),y)
> obj-y += vsmp_64.o
> endif
>
> -ifdef CONFIG_EFI
> -obj-$(CONFIG_IMA) += ima_arch.o
> -endif
> +obj-$(CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT) += ima_arch.o
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 1659217e9b60..aefe758f4466 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -30,8 +30,7 @@ extern void ima_kexec_cmdline(const void *buf, int size);
> extern void ima_add_kexec_buffer(struct kimage *image);
> #endif
>
> -#if (defined(CONFIG_X86) && defined(CONFIG_EFI)) || defined(CONFIG_S390) \
> - || defined(CONFIG_PPC_SECURE_BOOT)
> +#ifdef CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT
> extern bool arch_ima_get_secureboot(void);
> extern const char * const *arch_get_ima_policy(void);
> #else
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index 3f3ee4e2eb0d..edde88dbe576 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -327,3 +327,10 @@ config IMA_QUEUE_EARLY_BOOT_KEYS
> depends on IMA_MEASURE_ASYMMETRIC_KEYS
> depends on SYSTEM_TRUSTED_KEYRING
> default y
> +
> +config IMA_SECURE_AND_OR_TRUSTED_BOOT
> + bool
> + depends on IMA_ARCH_POLICY
> + help
> + This option is selected by architectures to enable secure and/or
> + trusted boot based on IMA runtime policies.
^ permalink raw reply
* Re: [PATCH v1 5/5] mm/memory_hotplug: allow to specify a default online_type
From: David Hildenbrand @ 2020-03-11 17:05 UTC (permalink / raw)
To: Vitaly Kuznetsov, linux-kernel
Cc: linux-hyperv, Baoquan He, Rafael J. Wysocki, Greg Kroah-Hartman,
Michal Hocko, linux-mm, Andrew Morton, Wei Yang, linuxppc-dev,
Oscar Salvador
In-Reply-To: <877dzqsuej.fsf@vitty.brq.redhat.com>
On 11.03.20 17:55, Vitaly Kuznetsov wrote:
> David Hildenbrand <david@redhat.com> writes:
>
>> For now, distributions implement advanced udev rules to essentially
>> - Don't online any hotplugged memory (s390x)
>> - Online all memory to ZONE_NORMAL (e.g., most virt environments like
>> hyperv)
>> - Online all memory to ZONE_MOVABLE in case the zone imbalance is taken
>> care of (e.g., bare metal, special virt environments)
>>
>> In summary: All memory is usually onlined the same way, however, the
>> kernel always has to ask userspace to come up with the same answer.
>> E.g., HyperV always waits for a memory block to get onlined before
>> continuing, otherwise it might end up adding memory faster than
>> hotplugging it, which can result in strange OOM situations.
>>
>> Let's allow to specify a default online_type, not just "online" and
>> "offline". This allows distributions to configure the default online_type
>> when booting up and be done with it.
>>
>> We can now specify "offline", "online", "online_movable" and
>> "online_kernel" via
>> - "memhp_default_state=" on the kernel cmdline
>> - /sys/devices/systemn/memory/auto_online_blocks
>> just like we are able to specify for a single memory block via
>> /sys/devices/systemn/memory/memoryX/state
>>
>
> Thank you for picking this up!
>
> It's been awhile since I've added CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE
> but I vaguely recall one problem: memory hotplug may happen *very* early
> (just because some memory is presented to a VM as hotplug memory, it is
> not in e820). It happens way before we launch userspace (including
> udev). The question is -- which ZONE will this memory be assigned too?
If it's added via add_memory() ("hot/cold plugged memory") like ACPI
DIMMs not part of e820, Hyper-V balloon added memory, XEN balloon added
memory, s390x standby memory etc. the memory will be onlined as
configured via memhp_default_online_type. Assume that one is set to
"offline".
*If* userspace changes memhp_default_online_type (as in my script in the
cover letter), userspace has to online all memory that has been added
before userspace was active itself (again, as done in my script).
Memory not added via add_memory() is considered "initial memory" and not
as hot/cold plugged memory.
Same handling as for now using udev rules. (once userspace is up, udev
rules for all early added memory is triggered as well)
>
> 'memhp_default_state=' resolves the issue but nobody likes additional
> kernel parameters for anything but
> debug. CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE was supposed to help, but it
> is binary and distro-wide (so *all* deployments will get the same
> default and as you validly stated we want it differently).
>
> We could've added something like your example onlining script to the
> kernel itself but this is likely going to be hard to sell: "policies
> belong to userspace!" will likely be the answer.
Exactly my thought.
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v1 5/5] mm/memory_hotplug: allow to specify a default online_type
From: Vitaly Kuznetsov @ 2020-03-11 16:55 UTC (permalink / raw)
To: David Hildenbrand, linux-kernel
Cc: linux-hyperv, Baoquan He, David Hildenbrand, Greg Kroah-Hartman,
Rafael J. Wysocki, Michal Hocko, linux-mm, Andrew Morton,
Wei Yang, linuxppc-dev, Oscar Salvador
In-Reply-To: <20200311123026.16071-6-david@redhat.com>
David Hildenbrand <david@redhat.com> writes:
> For now, distributions implement advanced udev rules to essentially
> - Don't online any hotplugged memory (s390x)
> - Online all memory to ZONE_NORMAL (e.g., most virt environments like
> hyperv)
> - Online all memory to ZONE_MOVABLE in case the zone imbalance is taken
> care of (e.g., bare metal, special virt environments)
>
> In summary: All memory is usually onlined the same way, however, the
> kernel always has to ask userspace to come up with the same answer.
> E.g., HyperV always waits for a memory block to get onlined before
> continuing, otherwise it might end up adding memory faster than
> hotplugging it, which can result in strange OOM situations.
>
> Let's allow to specify a default online_type, not just "online" and
> "offline". This allows distributions to configure the default online_type
> when booting up and be done with it.
>
> We can now specify "offline", "online", "online_movable" and
> "online_kernel" via
> - "memhp_default_state=" on the kernel cmdline
> - /sys/devices/systemn/memory/auto_online_blocks
> just like we are able to specify for a single memory block via
> /sys/devices/systemn/memory/memoryX/state
>
Thank you for picking this up!
It's been awhile since I've added CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE
but I vaguely recall one problem: memory hotplug may happen *very* early
(just because some memory is presented to a VM as hotplug memory, it is
not in e820). It happens way before we launch userspace (including
udev). The question is -- which ZONE will this memory be assigned too?
'memhp_default_state=' resolves the issue but nobody likes additional
kernel parameters for anything but
debug. CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE was supposed to help, but it
is binary and distro-wide (so *all* deployments will get the same
default and as you validly stated we want it differently).
We could've added something like your example onlining script to the
kernel itself but this is likely going to be hard to sell: "policies
belong to userspace!" will likely be the answer.
So if we don't want to start the endless discussions (again), your
proposal is 'good enough'.
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Oscar Salvador <osalvador@suse.de>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Baoquan He <bhe@redhat.com>
> Cc: Wei Yang <richard.weiyang@gmail.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> drivers/base/memory.c | 11 +++++------
> include/linux/memory_hotplug.h | 2 ++
> mm/memory_hotplug.c | 8 ++++----
> 3 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index 8d3e16dab69f..2b09b68b9f78 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -35,7 +35,7 @@ static const char *const online_type_to_str[] = {
> [MMOP_ONLINE_MOVABLE] = "online_movable",
> };
>
> -static int memhp_online_type_from_str(const char *str)
> +int memhp_online_type_from_str(const char *str)
> {
> int i;
>
> @@ -394,13 +394,12 @@ static ssize_t auto_online_blocks_store(struct device *dev,
> struct device_attribute *attr,
> const char *buf, size_t count)
> {
> - if (sysfs_streq(buf, "online"))
> - memhp_default_online_type = MMOP_ONLINE;
> - else if (sysfs_streq(buf, "offline"))
> - memhp_default_online_type = MMOP_OFFLINE;
> - else
> + const int online_type = memhp_online_type_from_str(buf);
> +
> + if (online_type < 0)
> return -EINVAL;
>
> + memhp_default_online_type = online_type;
> return count;
> }
>
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index c6e090b34c4b..ef55115320fb 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -117,6 +117,8 @@ extern int arch_add_memory(int nid, u64 start, u64 size,
> struct mhp_restrictions *restrictions);
> extern u64 max_mem_size;
>
> +extern int memhp_online_type_from_str(const char *str);
> +
> /* Default online_type (MMOP_*) when new memory blocks are added. */
> extern int memhp_default_online_type;
> /* If movable_node boot option specified */
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 01443c70aa27..4a96273eafa7 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -75,10 +75,10 @@ EXPORT_SYMBOL_GPL(memhp_default_online_type);
>
> static int __init setup_memhp_default_state(char *str)
> {
> - if (!strcmp(str, "online"))
> - memhp_default_online_type = MMOP_ONLINE;
> - else if (!strcmp(str, "offline"))
> - memhp_default_online_type = MMOP_OFFLINE;
> + const int online_type = memhp_online_type_from_str(str);
> +
> + if (online_type >= 0)
> + memhp_default_online_type = online_type;
>
> return 1;
> }
--
Vitaly
^ permalink raw reply
* Re: [RFC 00/11] perf: Enhancing perf to export processor hazard information
From: Ravi Bangoria @ 2020-03-11 16:00 UTC (permalink / raw)
To: Kim Phillips
Cc: Stephane Eranian, Peter Zijlstra, linuxppc-dev, LKML,
Michael Ellerman, Paul Mackerras, Ingo Molnar,
Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, Adrian Hunter, Andi Kleen, Liang, Kan,
Alexey Budankov, yao.jin, Robert Richter, maddy, Ravi Bangoria
In-Reply-To: <d3c82708-dd09-80e0-4e9f-1cbab118a169@amd.com>
Hi Kim,
On 3/6/20 3:36 AM, Kim Phillips wrote:
>> On 3/3/20 3:55 AM, Kim Phillips wrote:
>>> On 3/2/20 2:21 PM, Stephane Eranian wrote:
>>>> On Mon, Mar 2, 2020 at 2:13 AM Peter Zijlstra <peterz@infradead.org> wrote:
>>>>>
>>>>> On Mon, Mar 02, 2020 at 10:53:44AM +0530, Ravi Bangoria wrote:
>>>>>> Modern processors export such hazard data in Performance
>>>>>> Monitoring Unit (PMU) registers. Ex, 'Sampled Instruction Event
>>>>>> Register' on IBM PowerPC[1][2] and 'Instruction-Based Sampling' on
>>>>>> AMD[3] provides similar information.
>>>>>>
>>>>>> Implementation detail:
>>>>>>
>>>>>> A new sample_type called PERF_SAMPLE_PIPELINE_HAZ is introduced.
>>>>>> If it's set, kernel converts arch specific hazard information
>>>>>> into generic format:
>>>>>>
>>>>>> struct perf_pipeline_haz_data {
>>>>>> /* Instruction/Opcode type: Load, Store, Branch .... */
>>>>>> __u8 itype;
>>>>>> /* Instruction Cache source */
>>>>>> __u8 icache;
>>>>>> /* Instruction suffered hazard in pipeline stage */
>>>>>> __u8 hazard_stage;
>>>>>> /* Hazard reason */
>>>>>> __u8 hazard_reason;
>>>>>> /* Instruction suffered stall in pipeline stage */
>>>>>> __u8 stall_stage;
>>>>>> /* Stall reason */
>>>>>> __u8 stall_reason;
>>>>>> __u16 pad;
>>>>>> };
>>>>>
>>>>> Kim, does this format indeed work for AMD IBS?
>>>
>>> It's not really 1:1, we don't have these separations of stages
>>> and reasons, for example: we have missed in L2 cache, for example.
>>> So IBS output is flatter, with more cycle latency figures than
>>> IBM's AFAICT.
>>
>> AMD IBS captures pipeline latency data incase Fetch sampling like the
>> Fetch latency, tag to retire latency, completion to retire latency and
>> so on. Yes, Ops sampling do provide more data on load/store centric
>> information. But it also captures more detailed data for Branch instructions.
>> And we also looked at ARM SPE, which also captures more details pipeline
>> data and latency information.
>>
>>>> Personally, I don't like the term hazard. This is too IBM Power
>>>> specific. We need to find a better term, maybe stall or penalty.
>>>
>>> Right, IBS doesn't have a filter to only count stalled or otherwise
>>> bad events. IBS' PPR descriptions has one occurrence of the
>>> word stall, and no penalty. The way I read IBS is it's just
>>> reporting more sample data than just the precise IP: things like
>>> hits, misses, cycle latencies, addresses, types, etc., so words
>>> like 'extended', or the 'auxiliary' already used today even
>>> are more appropriate for IBS, although I'm the last person to
>>> bikeshed.
>>
>> We are thinking of using "pipeline" word instead of Hazard.
>
> Hm, the word 'pipeline' occurs 0 times in IBS documentation.
NP. We thought pipeline is generic hw term so we proposed "pipeline"
word. We are open to term which can be generic enough.
>
> I realize there are a couple of core pipeline-specific pieces
> of information coming out of it, but the vast majority
> are addresses, latencies of various components in the memory
> hierarchy, and various component hit/miss bits.
Yes. we should capture core pipeline specific details. For example,
IBS generates Branch unit information(IbsOpData1) and Icahce related
data(IbsFetchCtl) which is something that shouldn't be extended as
part of perf-mem, IMO.
>
> What's needed here is a vendor-specific extended
> sample information that all these technologies gather,
> of which things like e.g., 'L1 TLB cycle latency' we
> all should have in common.
Yes. We will include fields to capture the latency cycles (like Issue
latency, Instruction completion latency etc..) along with other pipeline
details in the proposed structure.
>
> I'm not sure why a new PERF_SAMPLE_PIPELINE_HAZ is needed
> either. Can we use PERF_SAMPLE_AUX instead?
We took a look at PERF_SAMPLE_AUX. IIUC, PERF_SAMPLE_AUX is intended when
large volume of data needs to be captured as part of perf.data without
frequent PMIs. But proposed type is to address the capture of pipeline
information on each sample using PMI at periodic intervals. Hence proposing
PERF_SAMPLE_PIPELINE_HAZ.
> Take a look at
> commit 98dcf14d7f9c "perf tools: Add kernel AUX area sampling
> definitions". The sample identifier can be used to determine
> which vendor's sampling IP's data is in it, and events can
> be recorded just by copying the content of the SIER, etc.
> registers, and then events get synthesized from the aux
> sample at report/inject/annotate etc. time. This allows
> for less sample recording overhead, and moves all the vendor
> specific decoding and common event conversions for userspace
> to figure out.
When AUX buffer data is structured, tool side changes added to present the
pipeline data can be re-used.
>
>>>> Also worth considering is the support of ARM SPE (Statistical
>>>> Profiling Extension) which is their version of IBS.
>>>> Whatever gets added need to cover all three with no limitations.
>>>
>>> I thought Intel's various LBR, PEBS, and PT supported providing
>>> similar sample data in perf already, like with perf mem/c2c?
>>
>> perf-mem is more of data centric in my opinion. It is more towards
>> memory profiling. So proposal here is to expose pipeline related
>> details like stalls and latencies.
>
> Like I said, I don't see it that way, I see it as "any particular
> vendor's event's extended details', and these pipeline details
> have overlap with existing infrastructure within perf, e.g., L2
> cache misses.
>
> Kim
>
^ permalink raw reply
* Re: [PATCH v3] ima: add a new CONFIG for loading arch-specific policies
From: Mimi Zohar @ 2020-03-11 15:42 UTC (permalink / raw)
To: Nayna Jain, linux-integrity, linuxppc-dev, linux-efi, linux-s390,
x86
Cc: Ard Biesheuvel, Philipp Rudo, Michael Ellerman, Thomas Gleixner,
linux-kernel, Nayna Jain
In-Reply-To: <1583715471-15525-1-git-send-email-nayna@linux.ibm.com>
On Sun, 2020-03-08 at 20:57 -0400, Nayna Jain wrote:
> From: Nayna Jain <nayna@linux.vnet.ibm.com>
>
> Every time a new architecture defines the IMA architecture specific
> functions - arch_ima_get_secureboot() and arch_ima_get_policy(), the IMA
> include file needs to be updated. To avoid this "noise", this patch
> defines a new IMA Kconfig IMA_SECURE_AND_OR_TRUSTED_BOOT option, allowing
> the different architectures to select it.
>
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
> Acked-by: Ard Biesheuvel <ardb@kernel.org>
> Cc: Philipp Rudo <prudo@linux.ibm.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
Thanks, Michael for the suggestion of using "imply". Seems to be
working nicely. Thanks, Nayna. I pushed this patch out to next-
integrity-testing. Could we get some tags on this version of the
patch?
thanks,
Mimi
^ permalink raw reply
* Re: [PATCH v1 5/5] mm/memory_hotplug: allow to specify a default online_type
From: David Hildenbrand @ 2020-03-11 15:20 UTC (permalink / raw)
To: Wei Yang
Cc: linux-hyperv, Baoquan He, Rafael J. Wysocki, Greg Kroah-Hartman,
linux-kernel, Michal Hocko, linux-mm, Andrew Morton, linuxppc-dev,
Oscar Salvador
In-Reply-To: <20200311142632.xvdwqk2lun4ookez@master>
On 11.03.20 15:26, Wei Yang wrote:
> On Wed, Mar 11, 2020 at 01:30:26PM +0100, David Hildenbrand wrote:
>> For now, distributions implement advanced udev rules to essentially
>> - Don't online any hotplugged memory (s390x)
>> - Online all memory to ZONE_NORMAL (e.g., most virt environments like
>> hyperv)
>> - Online all memory to ZONE_MOVABLE in case the zone imbalance is taken
>> care of (e.g., bare metal, special virt environments)
>>
>> In summary: All memory is usually onlined the same way, however, the
>> kernel always has to ask userspace to come up with the same answer.
>> E.g., HyperV always waits for a memory block to get onlined before
>> continuing, otherwise it might end up adding memory faster than
>> hotplugging it, which can result in strange OOM situations.
>>
>> Let's allow to specify a default online_type, not just "online" and
>> "offline". This allows distributions to configure the default online_type
>> when booting up and be done with it.
>>
>> We can now specify "offline", "online", "online_movable" and
>> "online_kernel" via
>> - "memhp_default_state=" on the kernel cmdline
>> - /sys/devices/systemn/memory/auto_online_blocks
>> just like we are able to specify for a single memory block via
>> /sys/devices/systemn/memory/memoryX/state
>>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Michal Hocko <mhocko@kernel.org>
>> Cc: Oscar Salvador <osalvador@suse.de>
>> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
>> Cc: Baoquan He <bhe@redhat.com>
>> Cc: Wei Yang <richard.weiyang@gmail.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>
> Ok, I got the reason to leave the change on string compare here.
Thanks for your *very fast* review! :)
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [PATCH] hwmon: (ibmpowernv) Use scnprintf() for avoiding potential buffer overflow
From: Guenter Roeck @ 2020-03-11 15:09 UTC (permalink / raw)
To: Takashi Iwai; +Cc: linux-hwmon, Jean Delvare, linuxppc-dev
On Wed, Mar 11, 2020 at 08:39:44AM +0100, Takashi Iwai wrote:
> Since snprintf() returns the would-be-output size instead of the
> actual output size, the succeeding calls may go beyond the given
> buffer limit. Fix it by replacing with scnprintf().
>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
Applied to hwmon-next.
Thanks,
Guenter
> ---
> drivers/hwmon/ibmpowernv.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
> index 0e525cfbdfc5..a750647e66a4 100644
> --- a/drivers/hwmon/ibmpowernv.c
> +++ b/drivers/hwmon/ibmpowernv.c
> @@ -186,7 +186,7 @@ static void make_sensor_label(struct device_node *np,
> u32 id;
> size_t n;
>
> - n = snprintf(sdata->label, sizeof(sdata->label), "%s", label);
> + n = scnprintf(sdata->label, sizeof(sdata->label), "%s", label);
>
> /*
> * Core temp pretty print
> @@ -199,11 +199,11 @@ static void make_sensor_label(struct device_node *np,
> * The digital thermal sensors are associated
> * with a core.
> */
> - n += snprintf(sdata->label + n,
> + n += scnprintf(sdata->label + n,
> sizeof(sdata->label) - n, " %d",
> cpuid);
> else
> - n += snprintf(sdata->label + n,
> + n += scnprintf(sdata->label + n,
> sizeof(sdata->label) - n, " phy%d", id);
> }
>
> @@ -211,7 +211,7 @@ static void make_sensor_label(struct device_node *np,
> * Membuffer pretty print
> */
> if (!of_property_read_u32(np, "ibm,chip-id", &id))
> - n += snprintf(sdata->label + n, sizeof(sdata->label) - n,
> + n += scnprintf(sdata->label + n, sizeof(sdata->label) - n,
> " %d", id & 0xffff);
> }
>
> --
> 2.16.4
>
^ permalink raw reply
* Re: [PATCH v1 4/5] mm/memory_hotplug: convert memhp_auto_online to store an online_type
From: Wei Yang @ 2020-03-11 14:25 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-hyperv, K. Y. Srinivasan, Stephen Hemminger, Baoquan He,
Rafael J. Wysocki, Greg Kroah-Hartman, Haiyang Zhang,
linux-kernel, Michal Hocko, linux-mm, Wei Liu, Paul Mackerras,
Andrew Morton, Wei Yang, linuxppc-dev, Thomas Gleixner,
Oscar Salvador
In-Reply-To: <20200311123026.16071-5-david@redhat.com>
On Wed, Mar 11, 2020 at 01:30:25PM +0100, David Hildenbrand wrote:
>... and rename it to memhp_default_online_type. This is a preparation
>for more detailed default online behavior.
>
>Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>Cc: Andrew Morton <akpm@linux-foundation.org>
>Cc: Michal Hocko <mhocko@kernel.org>
>Cc: Oscar Salvador <osalvador@suse.de>
>Cc: "Rafael J. Wysocki" <rafael@kernel.org>
>Cc: Baoquan He <bhe@redhat.com>
>Cc: Wei Yang <richard.weiyang@gmail.com>
>Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>Cc: Paul Mackerras <paulus@samba.org>
>Cc: Michael Ellerman <mpe@ellerman.id.au>
>Cc: "K. Y. Srinivasan" <kys@microsoft.com>
>Cc: Haiyang Zhang <haiyangz@microsoft.com>
>Cc: Stephen Hemminger <sthemmin@microsoft.com>
>Cc: Wei Liu <wei.liu@kernel.org>
>Cc: Thomas Gleixner <tglx@linutronix.de>
>Cc: linuxppc-dev@lists.ozlabs.org
>Cc: linux-hyperv@vger.kernel.org
>Signed-off-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
>---
> arch/powerpc/platforms/powernv/memtrace.c | 2 +-
> drivers/base/memory.c | 10 ++++------
> drivers/hv/hv_balloon.c | 2 +-
> include/linux/memory_hotplug.h | 3 ++-
> mm/memory_hotplug.c | 13 +++++++------
> 5 files changed, 15 insertions(+), 15 deletions(-)
>
>diff --git a/arch/powerpc/platforms/powernv/memtrace.c b/arch/powerpc/platforms/powernv/memtrace.c
>index d6d64f8718e6..e15a600cfa4d 100644
>--- a/arch/powerpc/platforms/powernv/memtrace.c
>+++ b/arch/powerpc/platforms/powernv/memtrace.c
>@@ -235,7 +235,7 @@ static int memtrace_online(void)
> * If kernel isn't compiled with the auto online option
> * we need to online the memory ourselves.
> */
>- if (!memhp_auto_online) {
>+ if (memhp_default_online_type == MMOP_OFFLINE) {
> lock_device_hotplug();
> walk_memory_blocks(ent->start, ent->size, NULL,
> online_mem_block);
>diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>index 8a7f29c0bf97..8d3e16dab69f 100644
>--- a/drivers/base/memory.c
>+++ b/drivers/base/memory.c
>@@ -386,10 +386,8 @@ static DEVICE_ATTR_RO(block_size_bytes);
> static ssize_t auto_online_blocks_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
>- if (memhp_auto_online)
>- return sprintf(buf, "online\n");
>- else
>- return sprintf(buf, "offline\n");
>+ return sprintf(buf, "%s\n",
>+ online_type_to_str[memhp_default_online_type]);
> }
>
> static ssize_t auto_online_blocks_store(struct device *dev,
>@@ -397,9 +395,9 @@ static ssize_t auto_online_blocks_store(struct device *dev,
> const char *buf, size_t count)
> {
> if (sysfs_streq(buf, "online"))
>- memhp_auto_online = true;
>+ memhp_default_online_type = MMOP_ONLINE;
> else if (sysfs_streq(buf, "offline"))
>- memhp_auto_online = false;
>+ memhp_default_online_type = MMOP_OFFLINE;
> else
> return -EINVAL;
>
>diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
>index a02ce43d778d..3b90fd12e0c5 100644
>--- a/drivers/hv/hv_balloon.c
>+++ b/drivers/hv/hv_balloon.c
>@@ -727,7 +727,7 @@ static void hv_mem_hot_add(unsigned long start, unsigned long size,
> spin_unlock_irqrestore(&dm_device.ha_lock, flags);
>
> init_completion(&dm_device.ol_waitevent);
>- dm_device.ha_waiting = !memhp_auto_online;
>+ dm_device.ha_waiting = memhp_default_online_type == MMOP_OFFLINE;
>
> nid = memory_add_physaddr_to_nid(PFN_PHYS(start_pfn));
> ret = add_memory(nid, PFN_PHYS((start_pfn)),
>diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
>index c2e06ed5e0e9..c6e090b34c4b 100644
>--- a/include/linux/memory_hotplug.h
>+++ b/include/linux/memory_hotplug.h
>@@ -117,7 +117,8 @@ extern int arch_add_memory(int nid, u64 start, u64 size,
> struct mhp_restrictions *restrictions);
> extern u64 max_mem_size;
>
>-extern bool memhp_auto_online;
>+/* Default online_type (MMOP_*) when new memory blocks are added. */
>+extern int memhp_default_online_type;
> /* If movable_node boot option specified */
> extern bool movable_node_enabled;
> static inline bool movable_node_is_enabled(void)
>diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>index 1a00b5a37ef6..01443c70aa27 100644
>--- a/mm/memory_hotplug.c
>+++ b/mm/memory_hotplug.c
>@@ -67,18 +67,18 @@ void put_online_mems(void)
> bool movable_node_enabled = false;
>
> #ifndef CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE
>-bool memhp_auto_online;
>+int memhp_default_online_type = MMOP_OFFLINE;
> #else
>-bool memhp_auto_online = true;
>+int memhp_default_online_type = MMOP_ONLINE;
> #endif
>-EXPORT_SYMBOL_GPL(memhp_auto_online);
>+EXPORT_SYMBOL_GPL(memhp_default_online_type);
>
> static int __init setup_memhp_default_state(char *str)
> {
> if (!strcmp(str, "online"))
>- memhp_auto_online = true;
>+ memhp_default_online_type = MMOP_ONLINE;
> else if (!strcmp(str, "offline"))
>- memhp_auto_online = false;
>+ memhp_default_online_type = MMOP_OFFLINE;
>
> return 1;
> }
>@@ -991,6 +991,7 @@ static int check_hotplug_memory_range(u64 start, u64 size)
>
> static int online_memory_block(struct memory_block *mem, void *arg)
> {
>+ mem->online_type = memhp_default_online_type;
> return device_online(&mem->dev);
> }
>
>@@ -1063,7 +1064,7 @@ int __ref add_memory_resource(int nid, struct resource *res)
> mem_hotplug_done();
>
> /* online pages if requested */
>- if (memhp_auto_online)
>+ if (memhp_default_online_type != MMOP_OFFLINE)
> walk_memory_blocks(start, size, NULL, online_memory_block);
>
> return ret;
>--
>2.24.1
--
Wei Yang
Help you, Help me
^ permalink raw reply
* Re: [PATCH v1 3/5] drivers/base/memory: store mapping between MMOP_* and string in an array
From: Wei Yang @ 2020-03-11 14:27 UTC (permalink / raw)
To: Wei Yang
Cc: linux-hyperv, Baoquan He, David Hildenbrand, Greg Kroah-Hartman,
Rafael J. Wysocki, linux-kernel, Michal Hocko, linux-mm,
Andrew Morton, linuxppc-dev, Oscar Salvador
In-Reply-To: <20200311142002.2htiv4llyam2svta@master>
On Wed, Mar 11, 2020 at 02:20:02PM +0000, Wei Yang wrote:
>On Wed, Mar 11, 2020 at 01:30:24PM +0100, David Hildenbrand wrote:
>>Let's use a simple array which we can reuse soon. While at it, move the
>>string->mmop conversion out of the device hotplug lock.
>>
>>Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>Cc: Andrew Morton <akpm@linux-foundation.org>
>>Cc: Michal Hocko <mhocko@kernel.org>
>>Cc: Oscar Salvador <osalvador@suse.de>
>>Cc: "Rafael J. Wysocki" <rafael@kernel.org>
>>Cc: Baoquan He <bhe@redhat.com>
>>Cc: Wei Yang <richard.weiyang@gmail.com>
>>Signed-off-by: David Hildenbrand <david@redhat.com>
Ok, I got the reason.
Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
>>---
>> drivers/base/memory.c | 38 +++++++++++++++++++++++---------------
>> 1 file changed, 23 insertions(+), 15 deletions(-)
>>
>>diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>>index e7e77cafef80..8a7f29c0bf97 100644
>>--- a/drivers/base/memory.c
>>+++ b/drivers/base/memory.c
>>@@ -28,6 +28,24 @@
>>
>> #define MEMORY_CLASS_NAME "memory"
>>
>>+static const char *const online_type_to_str[] = {
>>+ [MMOP_OFFLINE] = "offline",
>>+ [MMOP_ONLINE] = "online",
>>+ [MMOP_ONLINE_KERNEL] = "online_kernel",
>>+ [MMOP_ONLINE_MOVABLE] = "online_movable",
>>+};
>>+
>>+static int memhp_online_type_from_str(const char *str)
>>+{
>>+ int i;
>>+
>>+ for (i = 0; i < ARRAY_SIZE(online_type_to_str); i++) {
>>+ if (sysfs_streq(str, online_type_to_str[i]))
>>+ return i;
>>+ }
>>+ return -EINVAL;
>>+}
>>+
>> #define to_memory_block(dev) container_of(dev, struct memory_block, dev)
>>
>> static int sections_per_block;
>>@@ -236,26 +254,17 @@ static int memory_subsys_offline(struct device *dev)
>> static ssize_t state_store(struct device *dev, struct device_attribute *attr,
>> const char *buf, size_t count)
>> {
>>+ const int online_type = memhp_online_type_from_str(buf);
>
>In your following patch, you did the same conversion. Is it possible to merge
>them into this one?
>
>> struct memory_block *mem = to_memory_block(dev);
>>- int ret, online_type;
>>+ int ret;
>>+
>>+ if (online_type < 0)
>>+ return -EINVAL;
>>
>> ret = lock_device_hotplug_sysfs();
>> if (ret)
>> return ret;
>>
>>- if (sysfs_streq(buf, "online_kernel"))
>>- online_type = MMOP_ONLINE_KERNEL;
>>- else if (sysfs_streq(buf, "online_movable"))
>>- online_type = MMOP_ONLINE_MOVABLE;
>>- else if (sysfs_streq(buf, "online"))
>>- online_type = MMOP_ONLINE;
>>- else if (sysfs_streq(buf, "offline"))
>>- online_type = MMOP_OFFLINE;
>>- else {
>>- ret = -EINVAL;
>>- goto err;
>>- }
>>-
>> switch (online_type) {
>> case MMOP_ONLINE_KERNEL:
>> case MMOP_ONLINE_MOVABLE:
>>@@ -271,7 +280,6 @@ static ssize_t state_store(struct device *dev, struct device_attribute *attr,
>> ret = -EINVAL; /* should never happen */
>> }
>>
>>-err:
>> unlock_device_hotplug();
>>
>> if (ret < 0)
>>--
>>2.24.1
>
>--
>Wei Yang
>Help you, Help me
--
Wei Yang
Help you, Help me
^ permalink raw reply
* Re: [PATCH v1 5/5] mm/memory_hotplug: allow to specify a default online_type
From: Wei Yang @ 2020-03-11 14:26 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-hyperv, Baoquan He, Rafael J. Wysocki, Greg Kroah-Hartman,
linux-kernel, Michal Hocko, linux-mm, Andrew Morton, Wei Yang,
linuxppc-dev, Oscar Salvador
In-Reply-To: <20200311123026.16071-6-david@redhat.com>
On Wed, Mar 11, 2020 at 01:30:26PM +0100, David Hildenbrand wrote:
>For now, distributions implement advanced udev rules to essentially
>- Don't online any hotplugged memory (s390x)
>- Online all memory to ZONE_NORMAL (e.g., most virt environments like
> hyperv)
>- Online all memory to ZONE_MOVABLE in case the zone imbalance is taken
> care of (e.g., bare metal, special virt environments)
>
>In summary: All memory is usually onlined the same way, however, the
>kernel always has to ask userspace to come up with the same answer.
>E.g., HyperV always waits for a memory block to get onlined before
>continuing, otherwise it might end up adding memory faster than
>hotplugging it, which can result in strange OOM situations.
>
>Let's allow to specify a default online_type, not just "online" and
>"offline". This allows distributions to configure the default online_type
>when booting up and be done with it.
>
>We can now specify "offline", "online", "online_movable" and
>"online_kernel" via
>- "memhp_default_state=" on the kernel cmdline
>- /sys/devices/systemn/memory/auto_online_blocks
>just like we are able to specify for a single memory block via
>/sys/devices/systemn/memory/memoryX/state
>
>Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>Cc: Andrew Morton <akpm@linux-foundation.org>
>Cc: Michal Hocko <mhocko@kernel.org>
>Cc: Oscar Salvador <osalvador@suse.de>
>Cc: "Rafael J. Wysocki" <rafael@kernel.org>
>Cc: Baoquan He <bhe@redhat.com>
>Cc: Wei Yang <richard.weiyang@gmail.com>
>Signed-off-by: David Hildenbrand <david@redhat.com>
Ok, I got the reason to leave the change on string compare here.
Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
>---
> drivers/base/memory.c | 11 +++++------
> include/linux/memory_hotplug.h | 2 ++
> mm/memory_hotplug.c | 8 ++++----
> 3 files changed, 11 insertions(+), 10 deletions(-)
>
>diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>index 8d3e16dab69f..2b09b68b9f78 100644
>--- a/drivers/base/memory.c
>+++ b/drivers/base/memory.c
>@@ -35,7 +35,7 @@ static const char *const online_type_to_str[] = {
> [MMOP_ONLINE_MOVABLE] = "online_movable",
> };
>
>-static int memhp_online_type_from_str(const char *str)
>+int memhp_online_type_from_str(const char *str)
> {
> int i;
>
>@@ -394,13 +394,12 @@ static ssize_t auto_online_blocks_store(struct device *dev,
> struct device_attribute *attr,
> const char *buf, size_t count)
> {
>- if (sysfs_streq(buf, "online"))
>- memhp_default_online_type = MMOP_ONLINE;
>- else if (sysfs_streq(buf, "offline"))
>- memhp_default_online_type = MMOP_OFFLINE;
>- else
>+ const int online_type = memhp_online_type_from_str(buf);
>+
>+ if (online_type < 0)
> return -EINVAL;
>
>+ memhp_default_online_type = online_type;
> return count;
> }
>
>diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
>index c6e090b34c4b..ef55115320fb 100644
>--- a/include/linux/memory_hotplug.h
>+++ b/include/linux/memory_hotplug.h
>@@ -117,6 +117,8 @@ extern int arch_add_memory(int nid, u64 start, u64 size,
> struct mhp_restrictions *restrictions);
> extern u64 max_mem_size;
>
>+extern int memhp_online_type_from_str(const char *str);
>+
> /* Default online_type (MMOP_*) when new memory blocks are added. */
> extern int memhp_default_online_type;
> /* If movable_node boot option specified */
>diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>index 01443c70aa27..4a96273eafa7 100644
>--- a/mm/memory_hotplug.c
>+++ b/mm/memory_hotplug.c
>@@ -75,10 +75,10 @@ EXPORT_SYMBOL_GPL(memhp_default_online_type);
>
> static int __init setup_memhp_default_state(char *str)
> {
>- if (!strcmp(str, "online"))
>- memhp_default_online_type = MMOP_ONLINE;
>- else if (!strcmp(str, "offline"))
>- memhp_default_online_type = MMOP_OFFLINE;
>+ const int online_type = memhp_online_type_from_str(str);
>+
>+ if (online_type >= 0)
>+ memhp_default_online_type = online_type;
>
> return 1;
> }
>--
>2.24.1
--
Wei Yang
Help you, Help me
^ permalink raw reply
* Re: [PATCH v1 3/5] drivers/base/memory: store mapping between MMOP_* and string in an array
From: Wei Yang @ 2020-03-11 14:20 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-hyperv, Baoquan He, Rafael J. Wysocki, Greg Kroah-Hartman,
linux-kernel, Michal Hocko, linux-mm, Andrew Morton, Wei Yang,
linuxppc-dev, Oscar Salvador
In-Reply-To: <20200311123026.16071-4-david@redhat.com>
On Wed, Mar 11, 2020 at 01:30:24PM +0100, David Hildenbrand wrote:
>Let's use a simple array which we can reuse soon. While at it, move the
>string->mmop conversion out of the device hotplug lock.
>
>Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>Cc: Andrew Morton <akpm@linux-foundation.org>
>Cc: Michal Hocko <mhocko@kernel.org>
>Cc: Oscar Salvador <osalvador@suse.de>
>Cc: "Rafael J. Wysocki" <rafael@kernel.org>
>Cc: Baoquan He <bhe@redhat.com>
>Cc: Wei Yang <richard.weiyang@gmail.com>
>Signed-off-by: David Hildenbrand <david@redhat.com>
>---
> drivers/base/memory.c | 38 +++++++++++++++++++++++---------------
> 1 file changed, 23 insertions(+), 15 deletions(-)
>
>diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>index e7e77cafef80..8a7f29c0bf97 100644
>--- a/drivers/base/memory.c
>+++ b/drivers/base/memory.c
>@@ -28,6 +28,24 @@
>
> #define MEMORY_CLASS_NAME "memory"
>
>+static const char *const online_type_to_str[] = {
>+ [MMOP_OFFLINE] = "offline",
>+ [MMOP_ONLINE] = "online",
>+ [MMOP_ONLINE_KERNEL] = "online_kernel",
>+ [MMOP_ONLINE_MOVABLE] = "online_movable",
>+};
>+
>+static int memhp_online_type_from_str(const char *str)
>+{
>+ int i;
>+
>+ for (i = 0; i < ARRAY_SIZE(online_type_to_str); i++) {
>+ if (sysfs_streq(str, online_type_to_str[i]))
>+ return i;
>+ }
>+ return -EINVAL;
>+}
>+
> #define to_memory_block(dev) container_of(dev, struct memory_block, dev)
>
> static int sections_per_block;
>@@ -236,26 +254,17 @@ static int memory_subsys_offline(struct device *dev)
> static ssize_t state_store(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
> {
>+ const int online_type = memhp_online_type_from_str(buf);
In your following patch, you did the same conversion. Is it possible to merge
them into this one?
> struct memory_block *mem = to_memory_block(dev);
>- int ret, online_type;
>+ int ret;
>+
>+ if (online_type < 0)
>+ return -EINVAL;
>
> ret = lock_device_hotplug_sysfs();
> if (ret)
> return ret;
>
>- if (sysfs_streq(buf, "online_kernel"))
>- online_type = MMOP_ONLINE_KERNEL;
>- else if (sysfs_streq(buf, "online_movable"))
>- online_type = MMOP_ONLINE_MOVABLE;
>- else if (sysfs_streq(buf, "online"))
>- online_type = MMOP_ONLINE;
>- else if (sysfs_streq(buf, "offline"))
>- online_type = MMOP_OFFLINE;
>- else {
>- ret = -EINVAL;
>- goto err;
>- }
>-
> switch (online_type) {
> case MMOP_ONLINE_KERNEL:
> case MMOP_ONLINE_MOVABLE:
>@@ -271,7 +280,6 @@ static ssize_t state_store(struct device *dev, struct device_attribute *attr,
> ret = -EINVAL; /* should never happen */
> }
>
>-err:
> unlock_device_hotplug();
>
> if (ret < 0)
>--
>2.24.1
--
Wei Yang
Help you, Help me
^ permalink raw reply
* Re: [PATCH v1 2/5] drivers/base/memory: map MMOP_OFFLINE to 0
From: Wei Yang @ 2020-03-11 14:18 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-hyperv, Baoquan He, Rafael J. Wysocki, Greg Kroah-Hartman,
linux-kernel, Michal Hocko, linux-mm, Andrew Morton, Wei Yang,
linuxppc-dev, Oscar Salvador
In-Reply-To: <20200311123026.16071-3-david@redhat.com>
On Wed, Mar 11, 2020 at 01:30:23PM +0100, David Hildenbrand wrote:
>I have no idea why we have to start at -1. Just treat 0 as the special
>case. Clarify a comment (which was wrong, when we come via
>device_online() the first time, the online_type would have been 0 /
>MEM_ONLINE). The default is now always MMOP_OFFLINE.
>
>This is a preparation to use the online_type as an array index.
>
>Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>Cc: Andrew Morton <akpm@linux-foundation.org>
>Cc: Michal Hocko <mhocko@kernel.org>
>Cc: Oscar Salvador <osalvador@suse.de>
>Cc: "Rafael J. Wysocki" <rafael@kernel.org>
>Cc: Baoquan He <bhe@redhat.com>
>Cc: Wei Yang <richard.weiyang@gmail.com>
>Signed-off-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
>---
> drivers/base/memory.c | 11 ++++-------
> include/linux/memory_hotplug.h | 2 +-
> 2 files changed, 5 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>index 8c5ce42c0fc3..e7e77cafef80 100644
>--- a/drivers/base/memory.c
>+++ b/drivers/base/memory.c
>@@ -211,17 +211,14 @@ static int memory_subsys_online(struct device *dev)
> return 0;
>
> /*
>- * If we are called from state_store(), online_type will be
>- * set >= 0 Otherwise we were called from the device online
>- * attribute and need to set the online_type.
>+ * When called via device_online() without configuring the online_type,
>+ * we want to default to MMOP_ONLINE.
> */
>- if (mem->online_type < 0)
>+ if (mem->online_type == MMOP_OFFLINE)
> mem->online_type = MMOP_ONLINE;
>
> ret = memory_block_change_state(mem, MEM_ONLINE, MEM_OFFLINE);
>-
>- /* clear online_type */
>- mem->online_type = -1;
>+ mem->online_type = MMOP_OFFLINE;
>
> return ret;
> }
>diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
>index 261dbf010d5d..c2e06ed5e0e9 100644
>--- a/include/linux/memory_hotplug.h
>+++ b/include/linux/memory_hotplug.h
>@@ -48,7 +48,7 @@ enum {
> /* Types for control the zone type of onlined and offlined memory */
> enum {
> /* Offline the memory. */
>- MMOP_OFFLINE = -1,
>+ MMOP_OFFLINE = 0,
> /* Online the memory. Zone depends, see default_zone_for_pfn(). */
> MMOP_ONLINE,
> /* Online the memory to ZONE_NORMAL. */
>--
>2.24.1
--
Wei Yang
Help you, Help me
^ permalink raw reply
* Re: [PATCH v1 1/5] drivers/base/memory: rename MMOP_ONLINE_KEEP to MMOP_ONLINE
From: Wei Yang @ 2020-03-11 14:17 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-hyperv, Baoquan He, Rafael J. Wysocki, Greg Kroah-Hartman,
linux-kernel, Michal Hocko, linux-mm, Andrew Morton, Wei Yang,
linuxppc-dev, Oscar Salvador
In-Reply-To: <20200311123026.16071-2-david@redhat.com>
On Wed, Mar 11, 2020 at 01:30:22PM +0100, David Hildenbrand wrote:
>The name is misleading. Let's just name it like the online_type name we
>expose to user space ("online").
>
>Add some documentation to the types.
>
>Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>Cc: Andrew Morton <akpm@linux-foundation.org>
>Cc: Michal Hocko <mhocko@kernel.org>
>Cc: Oscar Salvador <osalvador@suse.de>
>Cc: "Rafael J. Wysocki" <rafael@kernel.org>
>Cc: Baoquan He <bhe@redhat.com>
>Cc: Wei Yang <richard.weiyang@gmail.com>
>Signed-off-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
>---
> drivers/base/memory.c | 9 +++++----
> include/linux/memory_hotplug.h | 6 +++++-
> 2 files changed, 10 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>index 6448c9ece2cb..8c5ce42c0fc3 100644
>--- a/drivers/base/memory.c
>+++ b/drivers/base/memory.c
>@@ -216,7 +216,7 @@ static int memory_subsys_online(struct device *dev)
> * attribute and need to set the online_type.
> */
> if (mem->online_type < 0)
>- mem->online_type = MMOP_ONLINE_KEEP;
>+ mem->online_type = MMOP_ONLINE;
>
> ret = memory_block_change_state(mem, MEM_ONLINE, MEM_OFFLINE);
>
>@@ -251,7 +251,7 @@ static ssize_t state_store(struct device *dev, struct device_attribute *attr,
> else if (sysfs_streq(buf, "online_movable"))
> online_type = MMOP_ONLINE_MOVABLE;
> else if (sysfs_streq(buf, "online"))
>- online_type = MMOP_ONLINE_KEEP;
>+ online_type = MMOP_ONLINE;
> else if (sysfs_streq(buf, "offline"))
> online_type = MMOP_OFFLINE;
> else {
>@@ -262,7 +262,7 @@ static ssize_t state_store(struct device *dev, struct device_attribute *attr,
> switch (online_type) {
> case MMOP_ONLINE_KERNEL:
> case MMOP_ONLINE_MOVABLE:
>- case MMOP_ONLINE_KEEP:
>+ case MMOP_ONLINE:
> /* mem->online_type is protected by device_hotplug_lock */
> mem->online_type = online_type;
> ret = device_online(&mem->dev);
>@@ -342,7 +342,8 @@ static ssize_t valid_zones_show(struct device *dev,
> }
>
> nid = mem->nid;
>- default_zone = zone_for_pfn_range(MMOP_ONLINE_KEEP, nid, start_pfn, nr_pages);
>+ default_zone = zone_for_pfn_range(MMOP_ONLINE, nid, start_pfn,
>+ nr_pages);
> strcat(buf, default_zone->name);
>
> print_allowed_zone(buf, nid, start_pfn, nr_pages, MMOP_ONLINE_KERNEL,
>diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
>index f4d59155f3d4..261dbf010d5d 100644
>--- a/include/linux/memory_hotplug.h
>+++ b/include/linux/memory_hotplug.h
>@@ -47,9 +47,13 @@ enum {
>
> /* Types for control the zone type of onlined and offlined memory */
> enum {
>+ /* Offline the memory. */
> MMOP_OFFLINE = -1,
>- MMOP_ONLINE_KEEP,
>+ /* Online the memory. Zone depends, see default_zone_for_pfn(). */
>+ MMOP_ONLINE,
>+ /* Online the memory to ZONE_NORMAL. */
> MMOP_ONLINE_KERNEL,
>+ /* Online the memory to ZONE_MOVABLE. */
> MMOP_ONLINE_MOVABLE,
> };
>
>--
>2.24.1
--
Wei Yang
Help you, Help me
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox