* [PATCH v6 1/3] powerpc/pseries: Correct possible read beyond dlpar sysfs buffer
2016-10-18 17:20 [PATCH v6 0/3] powerpc/pseries: Implement indexed-count memory hotplug Nathan Fontenot
@ 2016-10-18 17:20 ` Nathan Fontenot
2016-10-27 14:20 ` John Allen
2016-10-18 17:20 ` [PATCH v6 2/3] powerpc/pseries: Implement indexed-count hotplug memory add Nathan Fontenot
2016-10-18 17:21 ` [PATCH v6 3/3] powerpc/pseries: Implement indexed-count hotplug memory remove Nathan Fontenot
2 siblings, 1 reply; 9+ messages in thread
From: Nathan Fontenot @ 2016-10-18 17:20 UTC (permalink / raw)
To: linuxppc-dev; +Cc: sahilmehta17
The pasrsing of data written to the dlpar file in sysfs does not correctly
account for the possibility of reading past the end of the buffer. Correct
this by updating the buffer parsing code to make a local copy and use the
strsep() and sysfs_streq() routines to parse the buffer. This also
separates the parsing code into subroutines to make cleaner.
Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
---
arch/powerpc/platforms/pseries/dlpar.c | 141 ++++++++++++++++++++++----------
1 file changed, 96 insertions(+), 45 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index 423e450..ec5d677 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -418,84 +418,135 @@ void queue_hotplug_event(struct pseries_hp_errorlog *hp_errlog,
}
}
-static ssize_t dlpar_store(struct class *class, struct class_attribute *attr,
- const char *buf, size_t count)
+static int dlpar_parse_resource(char **cmd, struct pseries_hp_errorlog *hp_elog)
{
- struct pseries_hp_errorlog *hp_elog;
- struct completion hotplug_done;
- const char *arg;
- int rc;
+ char *arg;
- hp_elog = kzalloc(sizeof(*hp_elog), GFP_KERNEL);
- if (!hp_elog) {
- rc = -ENOMEM;
- goto dlpar_store_out;
- }
+ arg = strsep(cmd, " ");
+ if (!arg)
+ return -EINVAL;
- /* Parse out the request from the user, this will be in the form
- * <resource> <action> <id_type> <id>
- */
- arg = buf;
- if (!strncmp(arg, "memory", 6)) {
+ if (sysfs_streq(arg, "memory")) {
hp_elog->resource = PSERIES_HP_ELOG_RESOURCE_MEM;
- arg += strlen("memory ");
- } else if (!strncmp(arg, "cpu", 3)) {
+ } else if (sysfs_streq(arg, "cpu")) {
hp_elog->resource = PSERIES_HP_ELOG_RESOURCE_CPU;
- arg += strlen("cpu ");
} else {
- pr_err("Invalid resource specified: \"%s\"\n", buf);
- rc = -EINVAL;
- goto dlpar_store_out;
+ pr_err("Invalid resource specified.\n");
+ return -EINVAL;
}
- if (!strncmp(arg, "add", 3)) {
+ return 0;
+}
+
+static int dlpar_parse_action(char **cmd, struct pseries_hp_errorlog *hp_elog)
+{
+ char *arg;
+
+ arg = strsep(cmd, " ");
+ if (!arg)
+ return -EINVAL;
+
+ if (sysfs_streq(arg, "add")) {
hp_elog->action = PSERIES_HP_ELOG_ACTION_ADD;
- arg += strlen("add ");
- } else if (!strncmp(arg, "remove", 6)) {
+ } else if (sysfs_streq(arg, "remove")) {
hp_elog->action = PSERIES_HP_ELOG_ACTION_REMOVE;
- arg += strlen("remove ");
} else {
- pr_err("Invalid action specified: \"%s\"\n", buf);
- rc = -EINVAL;
- goto dlpar_store_out;
+ pr_err("Invalid action specified.\n");
+ return -EINVAL;
}
- if (!strncmp(arg, "index", 5)) {
- u32 index;
+ return 0;
+}
+static int dlpar_parse_id_type(char **cmd, struct pseries_hp_errorlog *hp_elog)
+{
+ char *arg;
+ u32 count, index;
+
+ arg = strsep(cmd, " ");
+ if (!arg)
+ return -EINVAL;
+
+ if (sysfs_streq(arg, "index")) {
hp_elog->id_type = PSERIES_HP_ELOG_ID_DRC_INDEX;
- arg += strlen("index ");
+ arg = strsep(cmd, " ");
+ if (!arg) {
+ pr_err("No DRC Index specified.\n");
+ return -EINVAL;
+ }
+
if (kstrtou32(arg, 0, &index)) {
- rc = -EINVAL;
- pr_err("Invalid drc_index specified: \"%s\"\n", buf);
- goto dlpar_store_out;
+ pr_err("Invalid DRC Index specified.\n");
+ return -EINVAL;
}
hp_elog->_drc_u.drc_index = cpu_to_be32(index);
- } else if (!strncmp(arg, "count", 5)) {
- u32 count;
-
+ } else if (sysfs_streq(arg, "count")) {
hp_elog->id_type = PSERIES_HP_ELOG_ID_DRC_COUNT;
- arg += strlen("count ");
+ arg = strsep(cmd, " ");
+ if (!arg) {
+ pr_err("No DRC count specified.\n");
+ return -EINVAL;
+ }
+
if (kstrtou32(arg, 0, &count)) {
- rc = -EINVAL;
- pr_err("Invalid count specified: \"%s\"\n", buf);
- goto dlpar_store_out;
+ pr_err("Invalid DRC count specified.\n");
+ return -EINVAL;
}
hp_elog->_drc_u.drc_count = cpu_to_be32(count);
} else {
- pr_err("Invalid id_type specified: \"%s\"\n", buf);
- rc = -EINVAL;
- goto dlpar_store_out;
+ pr_err("Invalid id_type specified.\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static ssize_t dlpar_store(struct class *class, struct class_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct pseries_hp_errorlog *hp_elog;
+ struct completion hotplug_done;
+ char *argbuf;
+ char *args;
+ int rc;
+
+ args = argbuf = kstrdup(buf, GFP_KERNEL);
+ hp_elog = kzalloc(sizeof(*hp_elog), GFP_KERNEL);
+ if (!hp_elog || !argbuf) {
+ pr_info("Could not allocate resources for DLPAR operation\n");
+ kfree(argbuf);
+ kfree(hp_elog);
+ return -ENOMEM;
}
+ /* Parse out the request from the user, this will be in the form
+ * <resource> <action> <id_type> <id>
+ */
+ rc = dlpar_parse_resource(&args, hp_elog);
+ if (rc)
+ goto dlpar_store_out;
+
+ rc = dlpar_parse_action(&args, hp_elog);
+ if (rc)
+ goto dlpar_store_out;
+
+ rc = dlpar_parse_id_type(&args, hp_elog);
+ if (rc)
+ goto dlpar_store_out;
+
init_completion(&hotplug_done);
queue_hotplug_event(hp_elog, &hotplug_done, &rc);
wait_for_completion(&hotplug_done);
dlpar_store_out:
+ kfree(argbuf);
kfree(hp_elog);
+
+ if (rc)
+ pr_err("Could not handle DLPAR request \"%s\"\n", buf);
+
return rc ? rc : count;
}
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v6 1/3] powerpc/pseries: Correct possible read beyond dlpar sysfs buffer
2016-10-18 17:20 ` [PATCH v6 1/3] powerpc/pseries: Correct possible read beyond dlpar sysfs buffer Nathan Fontenot
@ 2016-10-27 14:20 ` John Allen
0 siblings, 0 replies; 9+ messages in thread
From: John Allen @ 2016-10-27 14:20 UTC (permalink / raw)
To: Nathan Fontenot, linuxppc-dev; +Cc: sahilmehta17
On 10/18/2016 12:20 PM, Nathan Fontenot wrote:
> The pasrsing of data written to the dlpar file in sysfs does not correctly
> account for the possibility of reading past the end of the buffer. Correct
> this by updating the buffer parsing code to make a local copy and use the
> strsep() and sysfs_streq() routines to parse the buffer. This also
> separates the parsing code into subroutines to make cleaner.
>
> Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
> ---
Reviewed-by: John Allen <jallen@linux.vnet.ibm.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v6 2/3] powerpc/pseries: Implement indexed-count hotplug memory add
2016-10-18 17:20 [PATCH v6 0/3] powerpc/pseries: Implement indexed-count memory hotplug Nathan Fontenot
2016-10-18 17:20 ` [PATCH v6 1/3] powerpc/pseries: Correct possible read beyond dlpar sysfs buffer Nathan Fontenot
@ 2016-10-18 17:20 ` Nathan Fontenot
2016-10-27 14:21 ` John Allen
2016-10-18 17:21 ` [PATCH v6 3/3] powerpc/pseries: Implement indexed-count hotplug memory remove Nathan Fontenot
2 siblings, 1 reply; 9+ messages in thread
From: Nathan Fontenot @ 2016-10-18 17:20 UTC (permalink / raw)
To: linuxppc-dev; +Cc: sahilmehta17
From: Sahil Mehta <sahilmehta17@gmail.com>
Indexed-count add for memory hotplug guarantees that a contiguous block
of <count> lmbs beginning at a specified <index> will be assigned (NOT
that <count> lmbs will be added). Because of Qemu's per-DIMM memory
management, the addition of a contiguous block of memory currently
requires a series of individual calls. Indexed-count add reduces
this series into a single call.
Signed-off-by: Sahil Mehta <sahilmehta17@gmail.com>
Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
---
v2: -remove potential memory leak when parsing command
-use u32s drc_index and count instead of u32 ic[]
in dlpar_memory
v3: -add logic to handle invalid drc_index input
-update indexed-count trigger to follow naming convention
-update dlpar_memory to follow kernel if-else style
v4: -add logic to handle kstrdup failure
-add logic to handle invalid command format
v5: -none
v6: -Corrected the sysfs buffer parsing to align with the updated
parsing code in the new patch 1 of the series.
arch/powerpc/include/asm/rtas.h | 2
arch/powerpc/platforms/pseries/dlpar.c | 38 +++++++-
arch/powerpc/platforms/pseries/hotplug-memory.c | 110 +++++++++++++++++++++--
3 files changed, 138 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
index 9c23baa..5d65de7 100644
--- a/arch/powerpc/include/asm/rtas.h
+++ b/arch/powerpc/include/asm/rtas.h
@@ -307,6 +307,7 @@ struct pseries_hp_errorlog {
union {
__be32 drc_index;
__be32 drc_count;
+ __be32 indexed_count[2];
char drc_name[1];
} _drc_u;
};
@@ -322,6 +323,7 @@ struct pseries_hp_errorlog {
#define PSERIES_HP_ELOG_ID_DRC_NAME 1
#define PSERIES_HP_ELOG_ID_DRC_INDEX 2
#define PSERIES_HP_ELOG_ID_DRC_COUNT 3
+#define PSERIES_HP_ELOG_ID_DRC_IC 4
struct pseries_errorlog *get_pseries_errorlog(struct rtas_error_log *log,
uint16_t section_id);
diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index ec5d677..2e01b1d 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -354,11 +354,17 @@ static int handle_dlpar_errorlog(struct pseries_hp_errorlog *hp_elog)
switch (hp_elog->id_type) {
case PSERIES_HP_ELOG_ID_DRC_COUNT:
hp_elog->_drc_u.drc_count =
- be32_to_cpu(hp_elog->_drc_u.drc_count);
+ be32_to_cpu(hp_elog->_drc_u.drc_count);
break;
case PSERIES_HP_ELOG_ID_DRC_INDEX:
hp_elog->_drc_u.drc_index =
- be32_to_cpu(hp_elog->_drc_u.drc_index);
+ be32_to_cpu(hp_elog->_drc_u.drc_index);
+ break;
+ case PSERIES_HP_ELOG_ID_DRC_IC:
+ hp_elog->_drc_u.indexed_count[0] =
+ be32_to_cpu(hp_elog->_drc_u.indexed_count[0]);
+ hp_elog->_drc_u.indexed_count[1] =
+ be32_to_cpu(hp_elog->_drc_u.indexed_count[1]);
}
switch (hp_elog->resource) {
@@ -467,7 +473,33 @@ static int dlpar_parse_id_type(char **cmd, struct pseries_hp_errorlog *hp_elog)
if (!arg)
return -EINVAL;
- if (sysfs_streq(arg, "index")) {
+ if (sysfs_streq(arg, "indexed-count")) {
+ hp_elog->id_type = PSERIES_HP_ELOG_ID_DRC_IC;
+ arg = strsep(cmd, " ");
+ if (!arg) {
+ pr_err("No DRC count specified.\n");
+ return -EINVAL;
+ }
+
+ if (kstrtou32(arg, 0, &count)) {
+ pr_err("Invalid DRC count specified.\n");
+ return -EINVAL;
+ }
+
+ arg = strsep(cmd, " ");
+ if (!arg) {
+ pr_err("No DRC Index specified.\n");
+ return -EINVAL;
+ }
+
+ if (kstrtou32(arg, 0, &index)) {
+ pr_err("Invalid DRC Index specified.\n");
+ return -EINVAL;
+ }
+
+ hp_elog->_drc_u.indexed_count[0] = cpu_to_be32(count);
+ hp_elog->_drc_u.indexed_count[1] = cpu_to_be32(index);
+ } else if (sysfs_streq(arg, "index")) {
hp_elog->id_type = PSERIES_HP_ELOG_ID_DRC_INDEX;
arg = strsep(cmd, " ");
if (!arg) {
diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index 76ec104..badc66d 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -725,6 +725,89 @@ static int dlpar_memory_add_by_index(u32 drc_index, struct property *prop)
return rc;
}
+static int dlpar_memory_add_by_ic(u32 lmbs_to_add, u32 drc_index,
+ struct property *prop)
+{
+ struct of_drconf_cell *lmbs;
+ u32 num_lmbs, *p;
+ int i, rc, start_lmb_found;
+ int lmbs_available = 0, start_index = 0, end_index;
+
+ pr_info("Attempting to hot-add %u LMB(s) at index %x\n",
+ lmbs_to_add, drc_index);
+
+ if (lmbs_to_add == 0)
+ return -EINVAL;
+
+ p = prop->value;
+ num_lmbs = *p++;
+ lmbs = (struct of_drconf_cell *)p;
+ start_lmb_found = 0;
+
+ /* Navigate to drc_index */
+ while (start_index < num_lmbs) {
+ if (lmbs[start_index].drc_index == drc_index) {
+ start_lmb_found = 1;
+ break;
+ }
+
+ start_index++;
+ }
+
+ if (!start_lmb_found)
+ return -EINVAL;
+
+ end_index = start_index + lmbs_to_add;
+
+ /* Validate that there are enough LMBs to satisfy the request */
+ for (i = start_index; i < end_index; i++) {
+ if (lmbs[i].flags & DRCONF_MEM_RESERVED)
+ break;
+
+ lmbs_available++;
+ }
+
+ if (lmbs_available < lmbs_to_add)
+ return -EINVAL;
+
+ for (i = start_index; i < end_index; i++) {
+ if (lmbs[i].flags & DRCONF_MEM_ASSIGNED)
+ continue;
+
+ rc = dlpar_add_lmb(&lmbs[i]);
+ if (rc)
+ break;
+
+ lmbs[i].reserved = 1;
+ }
+
+ if (rc) {
+ pr_err("Memory indexed-count-add failed, removing any added LMBs\n");
+
+ for (i = start_index; i < end_index; i++) {
+ if (!lmbs[i].reserved)
+ continue;
+
+ rc = dlpar_remove_lmb(&lmbs[i]);
+ if (rc)
+ pr_err("Failed to remove LMB, drc index %x\n",
+ be32_to_cpu(lmbs[i].drc_index));
+ }
+ rc = -EINVAL;
+ } else {
+ for (i = start_index; i < end_index; i++) {
+ if (!lmbs[i].reserved)
+ continue;
+
+ pr_info("Memory at %llx (drc index %x) was hot-added\n",
+ lmbs[i].base_addr, lmbs[i].drc_index);
+ lmbs[i].reserved = 0;
+ }
+ }
+
+ return rc;
+}
+
int dlpar_memory(struct pseries_hp_errorlog *hp_elog)
{
struct device_node *dn;
@@ -732,9 +815,6 @@ int dlpar_memory(struct pseries_hp_errorlog *hp_elog)
u32 count, drc_index;
int rc;
- count = hp_elog->_drc_u.drc_count;
- drc_index = hp_elog->_drc_u.drc_index;
-
lock_device_hotplug();
dn = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory");
@@ -751,20 +831,32 @@ int dlpar_memory(struct pseries_hp_errorlog *hp_elog)
switch (hp_elog->action) {
case PSERIES_HP_ELOG_ACTION_ADD:
- if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_COUNT)
+ if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_COUNT) {
+ count = hp_elog->_drc_u.drc_count;
rc = dlpar_memory_add_by_count(count, prop);
- else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX)
+ } else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX) {
+ drc_index = hp_elog->_drc_u.drc_index;
rc = dlpar_memory_add_by_index(drc_index, prop);
- else
+ } else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_IC) {
+ count = hp_elog->_drc_u.indexed_count[0];
+ drc_index = hp_elog->_drc_u.indexed_count[1];
+ rc = dlpar_memory_add_by_ic(count, drc_index, prop);
+ } else {
rc = -EINVAL;
+ }
+
break;
case PSERIES_HP_ELOG_ACTION_REMOVE:
- if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_COUNT)
+ if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_COUNT) {
+ count = hp_elog->_drc_u.drc_count;
rc = dlpar_memory_remove_by_count(count, prop);
- else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX)
+ } else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX) {
+ drc_index = hp_elog->_drc_u.drc_index;
rc = dlpar_memory_remove_by_index(drc_index, prop);
- else
+ } else {
rc = -EINVAL;
+ }
+
break;
default:
pr_err("Invalid action (%d) specified\n", hp_elog->action);
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v6 2/3] powerpc/pseries: Implement indexed-count hotplug memory add
2016-10-18 17:20 ` [PATCH v6 2/3] powerpc/pseries: Implement indexed-count hotplug memory add Nathan Fontenot
@ 2016-10-27 14:21 ` John Allen
0 siblings, 0 replies; 9+ messages in thread
From: John Allen @ 2016-10-27 14:21 UTC (permalink / raw)
To: Nathan Fontenot, linuxppc-dev; +Cc: sahilmehta17
On 10/18/2016 12:20 PM, Nathan Fontenot wrote:
> From: Sahil Mehta <sahilmehta17@gmail.com>
>
> Indexed-count add for memory hotplug guarantees that a contiguous block
> of <count> lmbs beginning at a specified <index> will be assigned (NOT
> that <count> lmbs will be added). Because of Qemu's per-DIMM memory
> management, the addition of a contiguous block of memory currently
> requires a series of individual calls. Indexed-count add reduces
> this series into a single call.
>
> Signed-off-by: Sahil Mehta <sahilmehta17@gmail.com>
> Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
> ---
Reviewed-by: John Allen <jallen@linux.vnet.ibm.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v6 3/3] powerpc/pseries: Implement indexed-count hotplug memory remove
2016-10-18 17:20 [PATCH v6 0/3] powerpc/pseries: Implement indexed-count memory hotplug Nathan Fontenot
2016-10-18 17:20 ` [PATCH v6 1/3] powerpc/pseries: Correct possible read beyond dlpar sysfs buffer Nathan Fontenot
2016-10-18 17:20 ` [PATCH v6 2/3] powerpc/pseries: Implement indexed-count hotplug memory add Nathan Fontenot
@ 2016-10-18 17:21 ` Nathan Fontenot
2016-10-27 14:22 ` John Allen
2016-11-15 0:58 ` Michael Roth
2 siblings, 2 replies; 9+ messages in thread
From: Nathan Fontenot @ 2016-10-18 17:21 UTC (permalink / raw)
To: linuxppc-dev; +Cc: sahilmehta17
From: Sahil Mehta <sahilmehta17@gmail.com>
Indexed-count remove for memory hotplug guarantees that a contiguous block
of <count> lmbs beginning at a specified <index> will be unassigned (NOT
that <count> lmbs will be removed). Because of Qemu's per-DIMM memory
management, the removal of a contiguous block of memory currently
requires a series of individual calls. Indexed-count remove reduces
this series into a single call.
Signed-off-by: Sahil Mehta <sahilmehta17@gmail.com>
Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
---
v2: -use u32s drc_index and count instead of u32 ic[]
in dlpar_memory
v3: -add logic to handle invalid drc_index input
v4: -none
v5: -Update for() loop to start at start_index
v6: -none
arch/powerpc/platforms/pseries/hotplug-memory.c | 90 +++++++++++++++++++++++
1 file changed, 90 insertions(+)
diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index badc66d..19ad081 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -558,6 +558,92 @@ static int dlpar_memory_remove_by_index(u32 drc_index, struct property *prop)
return rc;
}
+static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index,
+ struct property *prop)
+{
+ struct of_drconf_cell *lmbs;
+ u32 num_lmbs, *p;
+ int i, rc, start_lmb_found;
+ int lmbs_available = 0, start_index = 0, end_index;
+
+ pr_info("Attempting to hot-remove %u LMB(s) at %x\n",
+ lmbs_to_remove, drc_index);
+
+ if (lmbs_to_remove == 0)
+ return -EINVAL;
+
+ p = prop->value;
+ num_lmbs = *p++;
+ lmbs = (struct of_drconf_cell *)p;
+ start_lmb_found = 0;
+
+ /* Navigate to drc_index */
+ while (start_index < num_lmbs) {
+ if (lmbs[start_index].drc_index == drc_index) {
+ start_lmb_found = 1;
+ break;
+ }
+
+ start_index++;
+ }
+
+ if (!start_lmb_found)
+ return -EINVAL;
+
+ end_index = start_index + lmbs_to_remove;
+
+ /* Validate that there are enough LMBs to satisfy the request */
+ for (i = start_index; i < end_index; i++) {
+ if (lmbs[i].flags & DRCONF_MEM_RESERVED)
+ break;
+
+ lmbs_available++;
+ }
+
+ if (lmbs_available < lmbs_to_remove)
+ return -EINVAL;
+
+ for (i = start_index; i < end_index; i++) {
+ if (!(lmbs[i].flags & DRCONF_MEM_ASSIGNED))
+ continue;
+
+ rc = dlpar_remove_lmb(&lmbs[i]);
+ if (rc)
+ break;
+
+ lmbs[i].reserved = 1;
+ }
+
+ if (rc) {
+ pr_err("Memory indexed-count-remove failed, adding any removed LMBs\n");
+
+ for (i = start_index; i < end_index; i++) {
+ if (!lmbs[i].reserved)
+ continue;
+
+ rc = dlpar_add_lmb(&lmbs[i]);
+ if (rc)
+ pr_err("Failed to add LMB, drc index %x\n",
+ be32_to_cpu(lmbs[i].drc_index));
+
+ lmbs[i].reserved = 0;
+ }
+ rc = -EINVAL;
+ } else {
+ for (i = start_index; i < end_index; i++) {
+ if (!lmbs[i].reserved)
+ continue;
+
+ pr_info("Memory at %llx (drc index %x) was hot-removed\n",
+ lmbs[i].base_addr, lmbs[i].drc_index);
+
+ lmbs[i].reserved = 0;
+ }
+ }
+
+ return rc;
+}
+
#else
static inline int pseries_remove_memblock(unsigned long base,
unsigned int memblock_size)
@@ -853,6 +939,10 @@ int dlpar_memory(struct pseries_hp_errorlog *hp_elog)
} else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX) {
drc_index = hp_elog->_drc_u.drc_index;
rc = dlpar_memory_remove_by_index(drc_index, prop);
+ } else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_IC) {
+ count = hp_elog->_drc_u.indexed_count[0];
+ drc_index = hp_elog->_drc_u.indexed_count[1];
+ rc = dlpar_memory_remove_by_ic(count, drc_index, prop);
} else {
rc = -EINVAL;
}
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v6 3/3] powerpc/pseries: Implement indexed-count hotplug memory remove
2016-10-18 17:21 ` [PATCH v6 3/3] powerpc/pseries: Implement indexed-count hotplug memory remove Nathan Fontenot
@ 2016-10-27 14:22 ` John Allen
2016-11-15 0:58 ` Michael Roth
1 sibling, 0 replies; 9+ messages in thread
From: John Allen @ 2016-10-27 14:22 UTC (permalink / raw)
To: Nathan Fontenot, linuxppc-dev; +Cc: sahilmehta17
On 10/18/2016 12:21 PM, Nathan Fontenot wrote:
> From: Sahil Mehta <sahilmehta17@gmail.com>
>
> Indexed-count remove for memory hotplug guarantees that a contiguous block
> of <count> lmbs beginning at a specified <index> will be unassigned (NOT
> that <count> lmbs will be removed). Because of Qemu's per-DIMM memory
> management, the removal of a contiguous block of memory currently
> requires a series of individual calls. Indexed-count remove reduces
> this series into a single call.
>
> Signed-off-by: Sahil Mehta <sahilmehta17@gmail.com>
> Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
> ---
Reviewed-by: John Allen <jallen@linux.vnet.ibm.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v6 3/3] powerpc/pseries: Implement indexed-count hotplug memory remove
2016-10-18 17:21 ` [PATCH v6 3/3] powerpc/pseries: Implement indexed-count hotplug memory remove Nathan Fontenot
2016-10-27 14:22 ` John Allen
@ 2016-11-15 0:58 ` Michael Roth
2016-11-15 20:09 ` Nathan Fontenot
1 sibling, 1 reply; 9+ messages in thread
From: Michael Roth @ 2016-11-15 0:58 UTC (permalink / raw)
To: Nathan Fontenot, linuxppc-dev; +Cc: sahilmehta17
Quoting Nathan Fontenot (2016-10-18 12:21:06)
> From: Sahil Mehta <sahilmehta17@gmail.com>
> =
> Indexed-count remove for memory hotplug guarantees that a contiguous block
> of <count> lmbs beginning at a specified <index> will be unassigned (NOT
> that <count> lmbs will be removed). Because of Qemu's per-DIMM memory
> management, the removal of a contiguous block of memory currently
> requires a series of individual calls. Indexed-count remove reduces
> this series into a single call.
> =
> Signed-off-by: Sahil Mehta <sahilmehta17@gmail.com>
> Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
> ---
> v2: -use u32s drc_index and count instead of u32 ic[]
> in dlpar_memory
> v3: -add logic to handle invalid drc_index input
> v4: -none
> v5: -Update for() loop to start at start_index
> v6: -none
> =
> arch/powerpc/platforms/pseries/hotplug-memory.c | 90 +++++++++++++++++=
++++++
> 1 file changed, 90 insertions(+)
> =
> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/power=
pc/platforms/pseries/hotplug-memory.c
> index badc66d..19ad081 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -558,6 +558,92 @@ static int dlpar_memory_remove_by_index(u32 drc_inde=
x, struct property *prop)
> return rc;
> }
> =
> +static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index,
> + struct property *prop)
> +{
> + struct of_drconf_cell *lmbs;
> + u32 num_lmbs, *p;
> + int i, rc, start_lmb_found;
> + int lmbs_available =3D 0, start_index =3D 0, end_index;
> +
> + pr_info("Attempting to hot-remove %u LMB(s) at %x\n",
> + lmbs_to_remove, drc_index);
> +
> + if (lmbs_to_remove =3D=3D 0)
> + return -EINVAL;
> +
> + p =3D prop->value;
> + num_lmbs =3D *p++;
> + lmbs =3D (struct of_drconf_cell *)p;
> + start_lmb_found =3D 0;
> +
> + /* Navigate to drc_index */
> + while (start_index < num_lmbs) {
> + if (lmbs[start_index].drc_index =3D=3D drc_index) {
> + start_lmb_found =3D 1;
> + break;
> + }
> +
> + start_index++;
> + }
> +
> + if (!start_lmb_found)
> + return -EINVAL;
> +
> + end_index =3D start_index + lmbs_to_remove;
> +
> + /* Validate that there are enough LMBs to satisfy the request */
> + for (i =3D start_index; i < end_index; i++) {
> + if (lmbs[i].flags & DRCONF_MEM_RESERVED)
> + break;
> +
> + lmbs_available++;
> + }
> +
> + if (lmbs_available < lmbs_to_remove)
> + return -EINVAL;
> +
> + for (i =3D start_index; i < end_index; i++) {
> + if (!(lmbs[i].flags & DRCONF_MEM_ASSIGNED))
> + continue;
> +
> + rc =3D dlpar_remove_lmb(&lmbs[i]);
dlpar_remove_lmb() currently does both offlining of the memory as well
as releasing the LMB back to the platform, but the specification for
hotplug notifications has the following verbage regarding
indexed-count/count identifiers:
'When using =E2=80=9Cdrc count=E2=80=9D or =E2=80=9Cdrc count indexed=E2=80=
=9D as the Hotplug Identifier,
the OS should take steps to verify the entirety of the request can be
satisfied before proceeding with the hotplug / unplug operations. If
only a partial count can be satisfied, the OS should ignore the entirety
of the request. If the OS cannot determine this beforehand, it should
satisfy the hotplug / unplug request for as many of the requested
resources as possible, and attempt to revert to the original OS / DRC
state.'
So doing the dlpar_remove->dlapr_add in case of failure is in line with
the spec, but it should only be done as a last-resort. To me that
suggests that we should be attempting to offline all the LMBs
beforehand, and only after that's successful should we begin attempting
to release LMBs back to the platform. Should we consider introducing
that logic in the patchset? Or maybe as a follow-up?
> + if (rc)
> + break;
> +
> + lmbs[i].reserved =3D 1;
> + }
> +
> + if (rc) {
> + pr_err("Memory indexed-count-remove failed, adding any re=
moved LMBs\n");
> +
> + for (i =3D start_index; i < end_index; i++) {
> + if (!lmbs[i].reserved)
> + continue;
> +
> + rc =3D dlpar_add_lmb(&lmbs[i]);
> + if (rc)
> + pr_err("Failed to add LMB, drc index %x\n=
",
> + be32_to_cpu(lmbs[i].drc_index));
> +
> + lmbs[i].reserved =3D 0;
> + }
> + rc =3D -EINVAL;
> + } else {
> + for (i =3D start_index; i < end_index; i++) {
> + if (!lmbs[i].reserved)
> + continue;
> +
> + pr_info("Memory at %llx (drc index %x) was hot-re=
moved\n",
> + lmbs[i].base_addr, lmbs[i].drc_index);
> +
> + lmbs[i].reserved =3D 0;
> + }
> + }
> +
> + return rc;
> +}
> +
> #else
> static inline int pseries_remove_memblock(unsigned long base,
> unsigned int memblock_size)
> @@ -853,6 +939,10 @@ int dlpar_memory(struct pseries_hp_errorlog *hp_elog)
> } else if (hp_elog->id_type =3D=3D PSERIES_HP_ELOG_ID_DRC=
_INDEX) {
> drc_index =3D hp_elog->_drc_u.drc_index;
> rc =3D dlpar_memory_remove_by_index(drc_index, pr=
op);
> + } else if (hp_elog->id_type =3D=3D PSERIES_HP_ELOG_ID_DRC=
_IC) {
> + count =3D hp_elog->_drc_u.indexed_count[0];
> + drc_index =3D hp_elog->_drc_u.indexed_count[1];
> + rc =3D dlpar_memory_remove_by_ic(count, drc_index=
, prop);
> } else {
> rc =3D -EINVAL;
> }
>=20
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v6 3/3] powerpc/pseries: Implement indexed-count hotplug memory remove
2016-11-15 0:58 ` Michael Roth
@ 2016-11-15 20:09 ` Nathan Fontenot
0 siblings, 0 replies; 9+ messages in thread
From: Nathan Fontenot @ 2016-11-15 20:09 UTC (permalink / raw)
To: Michael Roth, linuxppc-dev; +Cc: sahilmehta17
On 11/14/2016 06:58 PM, Michael Roth wrote:
> Quoting Nathan Fontenot (2016-10-18 12:21:06)
>> From: Sahil Mehta <sahilmehta17@gmail.com>
>>
>> Indexed-count remove for memory hotplug guarantees that a contiguous block
>> of <count> lmbs beginning at a specified <index> will be unassigned (NOT
>> that <count> lmbs will be removed). Because of Qemu's per-DIMM memory
>> management, the removal of a contiguous block of memory currently
>> requires a series of individual calls. Indexed-count remove reduces
>> this series into a single call.
>>
>> Signed-off-by: Sahil Mehta <sahilmehta17@gmail.com>
>> Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
>> ---
>> v2: -use u32s drc_index and count instead of u32 ic[]
>> in dlpar_memory
>> v3: -add logic to handle invalid drc_index input
>> v4: -none
>> v5: -Update for() loop to start at start_index
>> v6: -none
>>
>> arch/powerpc/platforms/pseries/hotplug-memory.c | 90 +++++++++++++++++++++++
>> 1 file changed, 90 insertions(+)
>>
>> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
>> index badc66d..19ad081 100644
>> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
>> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
>> @@ -558,6 +558,92 @@ static int dlpar_memory_remove_by_index(u32 drc_index, struct property *prop)
>> return rc;
>> }
>>
>> +static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index,
>> + struct property *prop)
>> +{
>> + struct of_drconf_cell *lmbs;
>> + u32 num_lmbs, *p;
>> + int i, rc, start_lmb_found;
>> + int lmbs_available = 0, start_index = 0, end_index;
>> +
>> + pr_info("Attempting to hot-remove %u LMB(s) at %x\n",
>> + lmbs_to_remove, drc_index);
>> +
>> + if (lmbs_to_remove == 0)
>> + return -EINVAL;
>> +
>> + p = prop->value;
>> + num_lmbs = *p++;
>> + lmbs = (struct of_drconf_cell *)p;
>> + start_lmb_found = 0;
>> +
>> + /* Navigate to drc_index */
>> + while (start_index < num_lmbs) {
>> + if (lmbs[start_index].drc_index == drc_index) {
>> + start_lmb_found = 1;
>> + break;
>> + }
>> +
>> + start_index++;
>> + }
>> +
>> + if (!start_lmb_found)
>> + return -EINVAL;
>> +
>> + end_index = start_index + lmbs_to_remove;
>> +
>> + /* Validate that there are enough LMBs to satisfy the request */
>> + for (i = start_index; i < end_index; i++) {
>> + if (lmbs[i].flags & DRCONF_MEM_RESERVED)
>> + break;
>> +
>> + lmbs_available++;
>> + }
>> +
>> + if (lmbs_available < lmbs_to_remove)
>> + return -EINVAL;
>> +
>> + for (i = start_index; i < end_index; i++) {
>> + if (!(lmbs[i].flags & DRCONF_MEM_ASSIGNED))
>> + continue;
>> +
>> + rc = dlpar_remove_lmb(&lmbs[i]);
>
> dlpar_remove_lmb() currently does both offlining of the memory as well
> as releasing the LMB back to the platform, but the specification for
> hotplug notifications has the following verbage regarding
> indexed-count/count identifiers:
>
> 'When using “drc count” or “drc count indexed” as the Hotplug Identifier,
> the OS should take steps to verify the entirety of the request can be
> satisfied before proceeding with the hotplug / unplug operations. If
> only a partial count can be satisfied, the OS should ignore the entirety
> of the request. If the OS cannot determine this beforehand, it should
> satisfy the hotplug / unplug request for as many of the requested
> resources as possible, and attempt to revert to the original OS / DRC
> state.'
>
> So doing the dlpar_remove->dlapr_add in case of failure is in line with
> the spec, but it should only be done as a last-resort. To me that
> suggests that we should be attempting to offline all the LMBs
> beforehand, and only after that's successful should we begin attempting
> to release LMBs back to the platform. Should we consider introducing
> that logic in the patchset? Or maybe as a follow-up?
Introducing it as a pre-cursor to this patch set may be best.
I'll send patches that split the dlpar_remove_lmb into a remove routine
that does the hotplug remove of the memory from the kernel and a new
dlpare_release_lmb routine that releases the drc-index and updates the
device tree. Plus a patch to make dlpar_add_lmb symmetrical.
-Nathan
>
>> + if (rc)
>> + break;
>> +
>> + lmbs[i].reserved = 1;
>> + }
>> +
>> + if (rc) {
>> + pr_err("Memory indexed-count-remove failed, adding any removed LMBs\n");
>> +
>> + for (i = start_index; i < end_index; i++) {
>> + if (!lmbs[i].reserved)
>> + continue;
>> +
>> + rc = dlpar_add_lmb(&lmbs[i]);
>> + if (rc)
>> + pr_err("Failed to add LMB, drc index %x\n",
>> + be32_to_cpu(lmbs[i].drc_index));
>> +
>> + lmbs[i].reserved = 0;
>> + }
>> + rc = -EINVAL;
>> + } else {
>> + for (i = start_index; i < end_index; i++) {
>> + if (!lmbs[i].reserved)
>> + continue;
>> +
>> + pr_info("Memory at %llx (drc index %x) was hot-removed\n",
>> + lmbs[i].base_addr, lmbs[i].drc_index);
>> +
>> + lmbs[i].reserved = 0;
>> + }
>> + }
>> +
>> + return rc;
>> +}
>> +
>> #else
>> static inline int pseries_remove_memblock(unsigned long base,
>> unsigned int memblock_size)
>> @@ -853,6 +939,10 @@ int dlpar_memory(struct pseries_hp_errorlog *hp_elog)
>> } else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX) {
>> drc_index = hp_elog->_drc_u.drc_index;
>> rc = dlpar_memory_remove_by_index(drc_index, prop);
>> + } else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_IC) {
>> + count = hp_elog->_drc_u.indexed_count[0];
>> + drc_index = hp_elog->_drc_u.indexed_count[1];
>> + rc = dlpar_memory_remove_by_ic(count, drc_index, prop);
>> } else {
>> rc = -EINVAL;
>> }
>>
^ permalink raw reply [flat|nested] 9+ messages in thread