pseries: Move memory hotplug to the kernel
cover pseries: Move memory hotplug to the kernel
In order to better support device hotplug (cpu, memory, and pci) in the PowerVM and PowerKVM environments, the handling of device hotplug could be updated so that the act of hotplugging a device occurs entirely in the kernel. This patch set begins to address this by moving memory hotplug to the kernel. Patches to follow will do the same for cpu and pci devices.
To provide background, the current handling of memory hotplug is handled by the drmgr command. This command is invoked when memory add/remove requests are made at the HMC and conveyed to a partition through the RSCT framework. The drmgr command then performs parts of the hotplug in user-space and makes requests to the kernel to perform other pieces. This is not really ideal, we can do everything in the kernel and do it faster.
In this patchset, hotplug events will now be communicated to the kernel in the form of rtas hotplug events. For PowerKVM systems this is done by qemu using the ras epow interrupt. For PowerVM systems the drmgr command will be updated to create a rtas hotplug event and send it to the kernel via a new /proc/powerpc/dlpar interface. Both of these entry points for hotplug rtas events then call a common routine for handling rtas hotplug events.
-Nathan
Patch 1/5
- Add definition of hotplug rtas event sections.
Patch 2/5
- export the dlpar_[acquire|release]drc() routines.
Patch 3/5
- Create the new /proc/powerpc/dlpar interface
Patch 4/5
- Implement memory hotplug add in the kernel.
Patch 5/5
- Implement memory hotplug remove in the kernel.
include/asm/rtas.h | 26 ++ platforms/pseries/dlpar.c | 63 ++++++- platforms/pseries/hotplug-memory.c | 332 ++++++++++++++++++++++++++++++++++++- platforms/pseries/pseries.h | 12 + 4 files changed, 431 insertions(+), 2 deletions(-)
PATCH 1/5 pseries: Define rtas hotplug event sections
In order to handle device hotplug in the kernel on pseries the hotplug notification will be communicated to the kernel in the form of a rtas hotplug event. This patch adds the definition of rtas hotplug event sections.
--- arch/powerpc/include/asm/rtas.h | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)
arch/powerpc/include/asm/rtas.h +26 -0
@@ -273,6 +273,7 @@ inline uint32_t rtas_ext_event_company_id(struct rtas_ext_event_log_v6 *ext_log)#define PSERIES_ELOG_SECT_ID_MANUFACT_INFO (('M' << 8) | 'I')#define PSERIES_ELOG_SECT_ID_CALL_HOME (('C' << 8) | 'H')#define PSERIES_ELOG_SECT_ID_USER_DEF (('U' << 8) | 'D') +#define PSERIES_ELOG_SECT_ID_HOTPLUG (('H' << 8) | 'P')/* Vendor specific Platform Event Log Format, Version 6, section header */ struct pseries_errorlog {
@@ -296,6 +297,31 @@ inline uint16_t pseries_errorlog_length(struct pseries_errorlog *sect)return be16_to_cpu (sect->length); } +/* RTAS pseries hotplug errorlog section */ +struct pseries_hp_errorlog { +uint8_t resource; +uint8_t action; +uint8_t id_type; +uint8_t reserved; +union { +__be32 drc_index; +__be32 drc_count; +char drc_name[1 ]; + } _drc_u; +}; + +#define PSERIES_HP_ELOG_RESOURCE_CPU 1 +#define PSERIES_HP_ELOG_RESOURCE_MEM 2 +#define PSERIES_HP_ELOG_RESOURCE_SLOT 3 +#define PSERIES_HP_ELOG_RESOURCE_PHB 4 + +#define PSERIES_HP_ELOG_ACTION_ADD 1 +#define PSERIES_HP_ELOG_ACTION_REMOVE 2 + +#define PSERIES_HP_ELOG_ID_DRC_NAME 1 +#define PSERIES_HP_ELOG_ID_DRC_INDEX 2 +#define PSERIES_HP_ELOG_ID_DRC_COUNT 3 +struct pseries_errorlog *get_pseries_errorlog (struct rtas_error_log *log,uint16_t section_id);
Michael Ellerman
reviewerRe: [1/5] pseries: Define rtas hotplug event sections
Quoted 13 lines · Nathan Fontenot
arch/powerpc/include/asm/rtas.h +1
-0@@ -273,6 +273,7 @@ inline uint32_t rtas_ext_event_company_id(struct rtas_ext_event_log_v6 *ext_log)#define PSERIES_ELOG_SECT_ID_MANUFACT_INFO (('M' << 8) | 'I')#define PSERIES_ELOG_SECT_ID_CALL_HOME (('C' << 8) | 'H')#define PSERIES_ELOG_SECT_ID_USER_DEF (('U' << 8) | 'D') +#define PSERIES_ELOG_SECT_ID_HOTPLUG (('H' << 8) | 'P')/* Vendor specific Platform Event Log Format, Version 6, section header */ struct pseries_errorlog {
arch/powerpc/include/asm/rtas.h inline uint16_t pseries_errorlog_length(struct pseries_errorlog *sect) · line 305
Quoted 9 lines
arch/powerpc/include/asm/rtas.h +6
-0@@ -296,6 +297,31 @@ inline uint16_t pseries_errorlog_length(struct pseries_errorlog *sect)return be16_to_cpu (sect->length); } +/* RTAS pseries hotplug errorlog section */ +struct pseries_hp_errorlog { +uint8_t resource; +uint8_t action; +uint8_t id_type; +uint8_t reserved;
These should be u8.
arch/powerpc/include/asm/rtas.h inline uint16_t pseries_errorlog_length(struct pseries_errorlog *sect) · line 309
Quoted 4 lines
arch/powerpc/include/asm/rtas.h +4
-0+union { +__be32 drc_index; +__be32 drc_count; +char drc_name[1 ];
I don't see drc_name used?
arch/powerpc/include/asm/rtas.h inline uint16_t pseries_errorlog_length(struct pseries_errorlog *sect) · line 311
Quoted 2 lines
arch/powerpc/include/asm/rtas.h +2
-0+ } _drc_u; +};
cheers
Re: [1/5] pseries: Define rtas hotplug event sections
Quoted 18 lines · Michael Ellerman
Quoted 16 lines · Nathan Fontenot
arch/powerpc/include/asm/rtas.h +1
-0@@ -273,6 +273,7 @@ inline uint32_t rtas_ext_event_company_id(struct rtas_ext_event_log_v6 *ext_log)#define PSERIES_ELOG_SECT_ID_MANUFACT_INFO (('M' << 8) | 'I')#define PSERIES_ELOG_SECT_ID_CALL_HOME (('C' << 8) | 'H')#define PSERIES_ELOG_SECT_ID_USER_DEF (('U' << 8) | 'D') +#define PSERIES_ELOG_SECT_ID_HOTPLUG (('H' << 8) | 'P')/* Vendor specific Platform Event Log Format, Version 6, section header */ struct pseries_errorlog {@@ -296,6 +297,31 @@ inline uint16_t pseries_errorlog_length(struct pseries_errorlog *sect)return be16_to_cpu (sect->length); }
Quoted 8 lines
arch/powerpc/include/asm/rtas.h inline uint16_t pseries_errorlog_length(struct pseries_errorlog *sect) · line 305
Quoted 6 lines
arch/powerpc/include/asm/rtas.h +6
-0+/* RTAS pseries hotplug errorlog section */ +struct pseries_hp_errorlog { +uint8_t resource; +uint8_t action; +uint8_t id_type; +uint8_t reserved;These should be u8.
ok.
Quoted 7 lines
arch/powerpc/include/asm/rtas.h inline uint16_t pseries_errorlog_length(struct pseries_errorlog *sect) · line 309
Quoted 5 lines
arch/powerpc/include/asm/rtas.h +4
-0+union { +__be32 drc_index; +__be32 drc_count; +char drc_name[1 ];I don't see
drc_nameused?
I don't use drc_name in this patch set but the drc_name piece is part of the rtas hotplug section definition and I wanted to provide a complete definition of the section.
-Nathan
Quoted 14 lines
arch/powerpc/include/asm/rtas.h inline uint16_t pseries_errorlog_length(struct pseries_errorlog *sect) · line 311
Quoted 3 lines
arch/powerpc/include/asm/rtas.h +2
-0+ } _drc_u; +};cheers
_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://lists.ozlabs.org/listinfo/linuxppc-dev
PATCH 2/5 pseries: Export drc_[acquire|release]_drc() routines
In reply to Nathan FontenotExport the routines to acquire and release a drc index.
--- arch/powerpc/platforms/pseries/pseries.h | 2 ++ 1 file changed, 2 insertions(+)
arch/powerpc/platforms/pseries/pseries.h +2 -0
@@ -59,6 +59,8 @@ extern void dlpar_free_cc_property(struct property *);extern struct device_node *dlpar_configure_connector (u32 ,struct device_node *);extern int dlpar_attach_node (struct device_node *);extern int dlpar_detach_node (struct device_node *); +extern int dlpar_acquire_drc (u32 ); +extern int dlpar_release_drc (u32 );/* PCI root bridge prepare function override for pseries */ struct pci_host_bridge ;
Michael Ellerman
reviewerRe: [2/5] pseries: Export drc_[acquire|release]_drc() routines
arch/powerpc/platforms/pseries/pseries.h extern void dlpar_free_cc_property(struct property *); · line 63
Quoted 10 lines · Nathan Fontenot
arch/powerpc/platforms/pseries/pseries.h +2
-0@@ -59,6 +59,8 @@ extern void dlpar_free_cc_property(struct property *);extern struct device_node *dlpar_configure_connector (u32 ,struct device_node *);extern int dlpar_attach_node (struct device_node *);extern int dlpar_detach_node (struct device_node *); +extern int dlpar_acquire_drc (u32 ); +extern int dlpar_release_drc (u32 );
Please name the parameters.
And don't bother with extern.
cheers
Re: [2/5] pseries: Export drc_[acquire|release]_drc() routines
Quoted 14 lines · Michael Ellerman
arch/powerpc/platforms/pseries/pseries.h extern void dlpar_free_cc_property(struct property *); · line 63
Quoted 10 lines · Nathan Fontenot
arch/powerpc/platforms/pseries/pseries.h +2
-0@@ -59,6 +59,8 @@ extern void dlpar_free_cc_property(struct property *);extern struct device_node *dlpar_configure_connector (u32 ,struct device_node *);extern int dlpar_attach_node (struct device_node *);extern int dlpar_detach_node (struct device_node *); +extern int dlpar_acquire_drc (u32 ); +extern int dlpar_release_drc (u32 );Please name the parameters.
Will do.
Quoted 2 lines
And don't bother with extern.
I was following the convention used in the file, droppping the extern is fine with me though.
-Nathan
PATCH 3/5 pseries: Create device hotplug entry point
In reply to Nathan FontenotFor pseries system the kernel will be notified of hotplug requests in the form of rtas hotplug events. This patch creates a common routine that can handle these requests in both the PowerVM anbd PowerKVM environments, handle_dlpar_errorlog(). This also creates the initial memory hotplug request handling stub.
For PowerVM this patch also creates a new /proc file that the drmgr command will use to write rtas hotplug events to.
For future PowerKVM handling the rtas check-exception code can pass any rtas hotplug events received to handle_dlpar_errorlog().
--- arch/powerpc/platforms/pseries/dlpar.c | 63 +++++++++++++++++++++++ arch/powerpc/platforms/pseries/hotplug-memory.c | 22 ++++++++ arch/powerpc/platforms/pseries/pseries.h | 10 ++++ 3 files changed, 94 insertions(+), 1 deletion(-)
arch/powerpc/platforms/pseries/dlpar.c +62 -1
@@ -16,7 +16,9 @@#include <linux/cpu.h> #include <linux/slab.h> #include <linux/of.h> +#include <linux/proc_fs.h> #include "offline_states.h" +#include "pseries.h" #include <asm/prom.h> #include <asm/machdep.h>
@@ -530,13 +532,72 @@ static ssize_t dlpar_cpu_release(const char *buf, size_t count)return count; } +#endif /* CONFIG_ARCH_CPU_PROBE_RELEASE */ + +static int handle_dlpar_errorlog (struct rtas_error_log *error_log) +{ +struct pseries_errorlog *pseries_log; +struct pseries_hp_errorlog *hp_elog; +int rc = -EINVAL ; + + pseries_log =get_pseries_errorlog (error_log, +PSERIES_ELOG_SECT_ID_HOTPLUG ); +if (!pseries_log) +return rc; + + hp_elog = (struct pseries_hp_errorlog *)pseries_log->data; +if (!hp_elog) +return rc; + +switch (hp_elog->resource) { +case PSERIES_HP_ELOG_RESOURCE_MEM : + rc =dlpar_memory (hp_elog); +break ; + } + +return rc; +} + +static ssize_t dlpar_write (struct file *file,const char __user *buf, +size_t count,loff_t *offset) +{ +char *event_buf; +int rc; + + event_buf =kmalloc (count +1 ,GFP_KERNEL ); +if (!event_buf) +return -ENOMEM ; + + rc =copy_from_user (event_buf, buf, count); +if (rc) { +kfree (event_buf); +return rc; + } + + rc =handle_dlpar_errorlog ((struct rtas_error_log *)event_buf); +kfree (event_buf); +return rc ? rc : count; +} + +static const struct file_operations dlpar_fops = { + .write = dlpar_write, + .llseek = noop_llseek, +}; +static int __init pseries_dlpar_init (void ) { +struct proc_dir_entry *proc_ent; + + proc_ent =proc_create ("powerpc/dlpar" ,S_IWUSR ,NULL , &dlpar_fops); +if (proc_ent) +proc_set_size (proc_ent,0 ); + +#ifdef CONFIG_ARCH_CPU_PROBE_RELEASE ppc_md.cpu_probe = dlpar_cpu_probe; ppc_md.cpu_release = dlpar_cpu_release; +#endif /* CONFIG_ARCH_CPU_PROBE_RELEASE */ return 0 ; }machine_device_initcall (pseries, pseries_dlpar_init); -#endif /* CONFIG_ARCH_CPU_PROBE_RELEASE */
arch/powerpc/platforms/pseries/hotplug-memory.c +22 -0
@@ -9,6 +9,8 @@* 2 of the License, or (at your option) any later version. */ +#define pr_fmt (fmt) "pseries-hotplug: " fmt +#include <linux/of.h> #include <linux/of_address.h> #include <linux/memblock.h>
@@ -20,6 +22,9 @@#include <asm/machdep.h> #include <asm/prom.h> #include <asm/sparsemem.h> +#include <asm/rtas.h> + +DEFINE_MUTEX (dlpar_mem_mutex);unsigned long pseries_memory_block_size (void ) {
@@ -150,6 +155,23 @@ static inline int pseries_remove_mem_node(struct device_node *np) }#endif /* CONFIG_MEMORY_HOTREMOVE */ +int dlpar_memory (struct pseries_hp_errorlog *hp_elog) +{ +int rc =0 ; + +mutex_lock (&dlpar_mem_mutex); + +switch (hp_elog->action) { +default : +pr_err ("Invalid action (%d) specified\n" , hp_elog->action); + rc = -EINVAL ; +break ; + } + +mutex_unlock (&dlpar_mem_mutex); +return rc; +} +static int pseries_add_mem_node (struct device_node *np) {const char *type;
arch/powerpc/platforms/pseries/pseries.h +10 -0
@@ -11,6 +11,7 @@#define _PSERIES_PSERIES_H#include <linux/interrupt.h> +#include <asm/rtas.h> struct device_node ;
@@ -62,6 +63,15 @@ extern int dlpar_detach_node(struct device_node *);extern int dlpar_acquire_drc (u32 );extern int dlpar_release_drc (u32 ); +#ifdef CONFIG_MEMORY_HOTPLUG +extern int dlpar_memory (struct pseries_hp_errorlog *); +#else +static inline int dlpar_memory (struct pseries_hp_errorlog *hp_elog) +{ +return -ENOTSUPP ; +} +#endif +/* PCI root bridge prepare function override for pseries */ struct pci_host_bridge ;int pseries_root_bridge_prepare (struct pci_host_bridge *bridge);
Michael Ellerman
reviewerRe: [3/5] pseries: Create device hotplug entry point
Quoted 2 lines · Nathan Fontenot
For pseries system the kernel will be notified of hotplug requests in the form of rtas hotplug events.
Can you flesh that design out a bit for me, I don't entirely get how it's going to work.
The kernel gets the rtas hotplug events (in rtasd.c) and spits them out to userspace, which then writes them back in ?
Quoted 2 lines
This patch creates a common routine that can handle these requests in both the PowerVM anbd PowerKVM environments,
handle_dlpar_errorlog(). This also
^
Quoted 4 lines
creates the initial memory hotplug request handling stub.
For PowerVM this patch also creates a new /proc file that the drmgr command will use to write rtas hotplug events to.
Why is this different between phyp and KVM?
Quoted 2 lines
For future PowerKVM handling the rtas check-exception code can pass any rtas hotplug events received to
handle_dlpar_errorlog().
Internally to the kernel you mean?
Quoted 15 lines
arch/powerpc/platforms/pseries/dlpar.c +2
-0@@ -16,7 +16,9 @@#include <linux/cpu.h> #include <linux/slab.h> #include <linux/of.h> +#include <linux/proc_fs.h> #include "offline_states.h" +#include "pseries.h" #include <asm/prom.h> #include <asm/machdep.h>
arch/powerpc/platforms/pseries/dlpar.c static ssize_t dlpar_cpu_release(const char *buf, size_t count) · line 535
Quoted 4 lines
arch/powerpc/platforms/pseries/dlpar.c +1
-0@@ -530,13 +532,72 @@ static ssize_t dlpar_cpu_release(const char *buf, size_t count) return count; } +#endif /* CONFIG_ARCH_CPU_PROBE_RELEASE */
That is really confusing, but I think it's just a diff artifact?
Quoted 4 lines
arch/powerpc/platforms/pseries/dlpar.c +4
-0+static int handle_dlpar_errorlog (struct rtas_error_log *error_log) +{ +struct pseries_errorlog *pseries_log; +struct pseries_hp_errorlog *hp_elog;
arch/powerpc/platforms/pseries/dlpar.c static ssize_t dlpar_cpu_release(const char *buf, size_t count) · line 550
Quoted 10 lines
arch/powerpc/platforms/pseries/dlpar.c +10
-0+int rc = -EINVAL ; + + pseries_log =get_pseries_errorlog (error_log, +PSERIES_ELOG_SECT_ID_HOTPLUG ); +if (!pseries_log) +return rc; + + hp_elog = (struct pseries_hp_errorlog *)pseries_log->data; +if (!hp_elog) +return rc;
I don't see how that can happen?
struct pseries_errorlog {
__be16 id; /* 0x00 2-byte ASCII section ID */
__be16 length; /* 0x02 Section length in bytes */
uint8_t version; /* 0x04 Section version */
uint8_t subtype; /* 0x05 Section subtype */
__be16 creator_component; /* 0x06 Creator component ID */
uint8_t data[]; /* 0x08 Start of section data */
};Should you be checking for length == 0 instead ?
Also I think the code will probably end up cleaner if you do the endian conversions immediately when you read the hp_elog, rather than passing it around in BE and having to remember to convert at all the usages.
arch/powerpc/platforms/pseries/dlpar.c static ssize_t dlpar_cpu_release(const char *buf, size_t count) · line 555
Quoted 4 lines
arch/powerpc/platforms/pseries/dlpar.c +4
-0+switch (hp_elog->resource) { +case PSERIES_HP_ELOG_RESOURCE_MEM : + rc =dlpar_memory (hp_elog); +break ;
Please add:
default:
pr_warn_ratelimited("Unknown resource ..")
Or something.
arch/powerpc/platforms/pseries/dlpar.c static ssize_t dlpar_cpu_release(const char *buf, size_t count) · line 567
Quoted 12 lines
arch/powerpc/platforms/pseries/dlpar.c +12
-0+ } + +return rc; +} + +static ssize_t dlpar_write (struct file *file,const char __user *buf, +size_t count,loff_t *offset) +{ +char *event_buf; +int rc; + + event_buf =kmalloc (count +1 ,GFP_KERNEL );
Why + 1 ? It's not null-terminated AFAICS.
arch/powerpc/platforms/pseries/dlpar.c static ssize_t dlpar_cpu_release(const char *buf, size_t count) · line 577
Quoted 10 lines
arch/powerpc/platforms/pseries/dlpar.c +10
-0+if (!event_buf) +return -ENOMEM ; + + rc =copy_from_user (event_buf, buf, count); +if (rc) { +kfree (event_buf); +return rc; + } + + rc =handle_dlpar_errorlog ((struct rtas_error_log *)event_buf);
If you start with a struct rtas_error_log * you shouldn't need any casts.
Quoted 6 lines
arch/powerpc/platforms/pseries/dlpar.c +6
-0+kfree (event_buf); +return rc ? rc : count; +} + +static const struct file_operations dlpar_fops = { + .write = dlpar_write,
arch/powerpc/platforms/pseries/dlpar.c static ssize_t dlpar_cpu_release(const char *buf, size_t count) · line 593
Quoted 10 lines
arch/powerpc/platforms/pseries/dlpar.c +8
-0+ .llseek = noop_llseek, +}; +static int __init pseries_dlpar_init (void ) { +struct proc_dir_entry *proc_ent; + + proc_ent =proc_create ("powerpc/dlpar" ,S_IWUSR ,NULL , &dlpar_fops); +if (proc_ent) +proc_set_size (proc_ent,0 );
else
error message at least please
Why are we putting it in /proc, can't it go in /sys/kernel like the mobility stuff?
arch/powerpc/platforms/pseries/hotplug-memory.c line 27
Quoted 11 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +3
-0@@ -20,6 +22,9 @@#include <asm/machdep.h> #include <asm/prom.h> #include <asm/sparsemem.h> +#include <asm/rtas.h> + +DEFINE_MUTEX (dlpar_mem_mutex);
static ?
Quoted 5 lines
arch/powerpc/platforms/pseries/pseries.h +0
-0
arch/powerpc/platforms/pseries/pseries.h extern int dlpar_detach_node(struct device_node *); · line 71
Quoted 9 lines
arch/powerpc/platforms/pseries/pseries.h +6
-0@@ -62,6 +63,15 @@ extern int dlpar_detach_node(struct device_node *);extern int dlpar_acquire_drc (u32 );extern int dlpar_release_drc (u32 ); +#ifdef CONFIG_MEMORY_HOTPLUG +extern int dlpar_memory (struct pseries_hp_errorlog *); +#else +static inline int dlpar_memory (struct pseries_hp_errorlog *hp_elog) +{ +return -ENOTSUPP ;
EOPNOTSUPP is a bit more standard.
cheers
Re: [3/5] pseries: Create device hotplug entry point
Quoted 19 lines · Michael Ellerman
Quoted 2 lines · Nathan Fontenot
For pseries system the kernel will be notified of hotplug requests in the form of rtas hotplug events.
Can you flesh that design out a bit for me, I don't entirely get how it's going to work.
The kernel gets the rtas hotplug events (in
rtasd.c) and spits them out to userspace, which then writes them back in ?Quoted 2 lines
This patch creates a common routine that can handle these requests in both the PowerVM anbd PowerKVM environments,
handle_dlpar_errorlog(). This also^Quoted 4 lines
creates the initial memory hotplug request handling stub.
For PowerVM this patch also creates a new /proc file that the drmgr command will use to write rtas hotplug events to.
Quoted 7 lines
Why is this different between phyp and KVM?
Quoted 2 lines
For future PowerKVM handling the rtas check-exception code can pass any rtas hotplug events received to
handle_dlpar_errorlog().Internally to the kernel you mean?
Perhaps a better explanation of how things work today and where I see them going is needed. I was trying to avoid a long explanation and I don't think my shortened explanation worked. I'll include this in v2 of the patchset too.
The current hotplug (or dlpar) of devices (the process is generally the same for memory, cpu, and pci) on PowerVM systems is initiated from the HMC, which communicates the request to the partitions through the RSCT framework. The RSCT framework then invokes the drmgr command. The drmgr command performs the hotplug operation by doing some pieces, such as most of the rtas calls and device tree parsing, in userspace and make requests to the kernel to online/offline the device, update the device tree and add/remove the device.
For PowerKVM the approach is to follow what is currently being done for pci hotplug. A hotplug request is initiated from the host. QEMU then sends an EPOW interrupt to the guest which causes the guest to make the rtas,check-exception call. In QEMU, the rtas,check-exception call returns a rtas hotplug event to the guest. I was using this same framework to also enable memory (and next cpu) hotplug.
You are correct that the current pci hotplug path for PowerKVM involves the kernel receiving the rtas event, passing it to rtas_errd in userspace, and having rtas_errd invoke drmgr. The drmgr command then handles the request as described above for PowerVM systems.
There is no need for this circuitous route, we should just handle the entire hotplug of devices in the kernel. What I am hoping to do is to enable this by moving the code to handle hotplug from drmgr into the kernel and provide a single path for handling hotplug for PowerVM and PowerKVM. To make this work for PowerKVM we will update the kernel rtas code to recognize rtas hotplug events returned from rtas,check-exception calls and call handle_dlpar_errorlog(). The hotplug rtas event is never sent out to userspace.
For PowerVM systems, I created the /proc/powerpc/dlpar file that a rtas hotplug event can be written to and passed to handle_dlpar_errorlog(). There is no chance of updating how we receive hotplug requests on PowerVM systems.
Hopefully that explains the design better.
Quoted 18 lines
Quoted 18 lines
arch/powerpc/platforms/pseries/dlpar.c +2
-0@@ -16,7 +16,9 @@#include <linux/cpu.h> #include <linux/slab.h> #include <linux/of.h> +#include <linux/proc_fs.h> #include "offline_states.h" +#include "pseries.h" #include <asm/prom.h> #include <asm/machdep.h> @@ -530,13 +532,72 @@ static ssize_t dlpar_cpu_release(const char *buf, size_t count)return count; }
Quoted 3 lines
arch/powerpc/platforms/pseries/dlpar.c static ssize_t dlpar_cpu_release(const char *buf, size_t count) · line 535
Quoted 1 line
arch/powerpc/platforms/pseries/dlpar.c +1
-0+#endif /* CONFIG_ARCH_CPU_PROBE_RELEASE */ That is really confusing, but I think it's just a diff artifact?
Yes, diff artifact.
Quoted 27 lines
Quoted 5 lines
arch/powerpc/platforms/pseries/dlpar.c +4
-0+static int handle_dlpar_errorlog (struct rtas_error_log *error_log) +{ +struct pseries_errorlog *pseries_log; +struct pseries_hp_errorlog *hp_elog;arch/powerpc/platforms/pseries/dlpar.c static ssize_t dlpar_cpu_release(const char *buf, size_t count) · line 550
Quoted 10 lines
arch/powerpc/platforms/pseries/dlpar.c +10
-0+int rc = -EINVAL ; + + pseries_log =get_pseries_errorlog (error_log, +PSERIES_ELOG_SECT_ID_HOTPLUG ); +if (!pseries_log) +return rc; + + hp_elog = (struct pseries_hp_errorlog *)pseries_log->data; +if (!hp_elog) +return rc;I don't see how that can happen?
struct pseries_errorlog { __be16 id; /* 0x00 2-byte ASCII section ID */ __be16 length; /* 0x02 Section length in bytes */ uint8_t version; /* 0x04 Section version */ uint8_t subtype; /* 0x05 Section subtype */ __be16 creator_component; /* 0x06 Creator component ID */ uint8_t data[]; /* 0x08 Start of section data */ };
Quoted 2 lines
Should you be checking for length == 0 instead ?
You are correct.
Quoted 4 lines
Also I think the code will probably end up cleaner if you do the endian conversions immediately when you read the
hp_elog, rather than passing it around in BE and having to remember to convert at all the usages.
Agreed. I'll clean that up.
Quoted 11 lines
arch/powerpc/platforms/pseries/dlpar.c static ssize_t dlpar_cpu_release(const char *buf, size_t count) · line 555
Quoted 4 lines
arch/powerpc/platforms/pseries/dlpar.c +4
-0+switch (hp_elog->resource) { +case PSERIES_HP_ELOG_RESOURCE_MEM : + rc =dlpar_memory (hp_elog); +break ;Please add:
default:
pr_warn_ratelimited("Unknown resource ..")Or something.
can do.
Quoted 16 lines
Quoted 4 lines
arch/powerpc/platforms/pseries/dlpar.c +2
-0+ } +arch/powerpc/platforms/pseries/dlpar.c static ssize_t dlpar_cpu_release(const char *buf, size_t count) · line 567
Quoted 10 lines
arch/powerpc/platforms/pseries/dlpar.c +10
-0+return rc; +} + +static ssize_t dlpar_write (struct file *file,const char __user *buf, +size_t count,loff_t *offset) +{ +char *event_buf; +int rc; + + event_buf =kmalloc (count +1 ,GFP_KERNEL );Why + 1 ? It's not null-terminated AFAICS.
I think that's just habit. You're correct, it's not needed.
Quoted 13 lines
arch/powerpc/platforms/pseries/dlpar.c static ssize_t dlpar_cpu_release(const char *buf, size_t count) · line 577
Quoted 11 lines
arch/powerpc/platforms/pseries/dlpar.c +10
-0+if (!event_buf) +return -ENOMEM ; + + rc =copy_from_user (event_buf, buf, count); +if (rc) { +kfree (event_buf); +return rc; + } + + rc =handle_dlpar_errorlog ((struct rtas_error_log *)event_buf);If you start with a struct
rtas_error_log* you shouldn't need any casts.
good point.
Quoted 21 lines
Quoted 7 lines
arch/powerpc/platforms/pseries/dlpar.c +6
-0+kfree (event_buf); +return rc ? rc : count; +} + +static const struct file_operations dlpar_fops = { + .write = dlpar_write,arch/powerpc/platforms/pseries/dlpar.c static ssize_t dlpar_cpu_release(const char *buf, size_t count) · line 593
Quoted 10 lines
arch/powerpc/platforms/pseries/dlpar.c +8
-0+ .llseek = noop_llseek, +}; +static int __init pseries_dlpar_init (void ) { +struct proc_dir_entry *proc_ent; + + proc_ent =proc_create ("powerpc/dlpar" ,S_IWUSR ,NULL , &dlpar_fops); +if (proc_ent) +proc_set_size (proc_ent,0 );else
error message at least please
Quoted 2 lines
Why are we putting it in /proc, can't it go in
/sys/kernellike the mobility stuff?
I have no personal preference, I'll move it to /sys/kernel and update the creation handling.
Quoted 14 lines
arch/powerpc/platforms/pseries/hotplug-memory.c line 27
Quoted 12 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +3
-0@@ -20,6 +22,9 @@#include <asm/machdep.h> #include <asm/prom.h> #include <asm/sparsemem.h> +#include <asm/rtas.h> + +DEFINE_MUTEX (dlpar_mem_mutex);static ?
yes, that should have been static.
Quoted 9 lines
Quoted 9 lines
arch/powerpc/platforms/pseries/pseries.h +0
-0@@ -62,6 +63,15 @@ extern int dlpar_detach_node(struct device_node *);extern int dlpar_acquire_drc (u32 );extern int dlpar_release_drc (u32 );
Quoted 8 lines
arch/powerpc/platforms/pseries/pseries.h extern int dlpar_detach_node(struct device_node *); · line 71
Quoted 6 lines
arch/powerpc/platforms/pseries/pseries.h +6
-0+#ifdef CONFIG_MEMORY_HOTPLUG +extern int dlpar_memory (struct pseries_hp_errorlog *); +#else +static inline int dlpar_memory (struct pseries_hp_errorlog *hp_elog) +{ +return -ENOTSUPP ;EOPNOTSUPP is a bit more standard.
ok.
Thanks for all the feedback.
-Nathan
Re: [3/5] pseries: Create device hotplug entry point
Quoted 54 lines · Nathan Fontenot
Quoted 19 lines · Michael Ellerman
Quoted 2 lines · Nathan Fontenot
For pseries system the kernel will be notified of hotplug requests in the form of rtas hotplug events.
Can you flesh that design out a bit for me, I don't entirely get how it's going to work.
The kernel gets the rtas hotplug events (in
rtasd.c) and spits them out to userspace, which then writes them back in ?Quoted 2 lines
This patch creates a common routine that can handle these requests in both the PowerVM anbd PowerKVM environments,
handle_dlpar_errorlog(). This also^Quoted 4 lines
creates the initial memory hotplug request handling stub.
For PowerVM this patch also creates a new /proc file that the drmgr command will use to write rtas hotplug events to.
Quoted 7 lines
Why is this different between phyp and KVM?
Quoted 2 lines
For future PowerKVM handling the rtas check-exception code can pass any rtas hotplug events received to
handle_dlpar_errorlog().Internally to the kernel you mean?
Perhaps a better explanation of how things work today and where I see them going is needed. I was trying to avoid a long explanation and I don't think my shortened explanation worked. I'll include this in v2 of the patchset too.
The current hotplug (or dlpar) of devices (the process is generally the same for memory, cpu, and pci) on PowerVM systems is initiated from the HMC, which communicates the request to the partitions through the RSCT framework. The RSCT framework then invokes the drmgr command. The drmgr command performs the hotplug operation by doing some pieces, such as most of the rtas calls and device tree parsing, in userspace and make requests to the kernel to online/offline the device, update the device tree and add/remove the device.
For PowerKVM the approach is to follow what is currently being done for pci hotplug. A hotplug request is initiated from the host. QEMU then sends an EPOW interrupt to the guest which causes the guest to make the rtas,check-exception call. In QEMU, the rtas,check-exception call returns a rtas hotplug event to the guest. I was using this same framework to also enable memory (and next cpu) hotplug.
You are correct that the current pci hotplug path for PowerKVM involves the kernel receiving the rtas event, passing it to
rtas_errdin userspace, and havingrtas_errdinvoke drmgr. The drmgr command then handles the request as described above for PowerVM systems.
Quoted 8 lines
There is no need for this circuitous route, we should just handle the entire hotplug of devices in the kernel. What I am hoping to do is to enable this by moving the code to handle hotplug from drmgr into the kernel and provide a single path for handling hotplug for PowerVM and PowerKVM. To make this work for PowerKVM we will update the kernel rtas code to recognize rtas hotplug events returned from rtas,check-exception calls and call
handle_dlpar_errorlog(). The hotplug rtas event is never sent out to userspace.
Wouldn't we still want the event surfaced to userspace so that it can at least be logged?
-Tyrel
Quoted 193 lines
For PowerVM systems, I created the
/proc/powerpc/dlparfile that a rtas hotplug event can be written to and passed tohandle_dlpar_errorlog(). There is no chance of updating how we receive hotplug requests on PowerVM systems.Hopefully that explains the design better.
Quoted 18 lines
Quoted 18 lines
arch/powerpc/platforms/pseries/dlpar.c +2
-0@@ -16,7 +16,9 @@#include <linux/cpu.h> #include <linux/slab.h> #include <linux/of.h> +#include <linux/proc_fs.h> #include "offline_states.h" +#include "pseries.h" #include <asm/prom.h> #include <asm/machdep.h> @@ -530,13 +532,72 @@ static ssize_t dlpar_cpu_release(const char *buf, size_t count)return count; }Quoted 3 lines
arch/powerpc/platforms/pseries/dlpar.c static ssize_t dlpar_cpu_release(const char *buf, size_t count) · line 535
Quoted 1 line
arch/powerpc/platforms/pseries/dlpar.c +1
-0+#endif /* CONFIG_ARCH_CPU_PROBE_RELEASE */ That is really confusing, but I think it's just a diff artifact?
Yes, diff artifact.
Quoted 27 lines
Quoted 5 lines
arch/powerpc/platforms/pseries/dlpar.c +4
-0+static int handle_dlpar_errorlog (struct rtas_error_log *error_log) +{ +struct pseries_errorlog *pseries_log; +struct pseries_hp_errorlog *hp_elog;arch/powerpc/platforms/pseries/dlpar.c static ssize_t dlpar_cpu_release(const char *buf, size_t count) · line 550
Quoted 10 lines
arch/powerpc/platforms/pseries/dlpar.c +10
-0+int rc = -EINVAL ; + + pseries_log =get_pseries_errorlog (error_log, +PSERIES_ELOG_SECT_ID_HOTPLUG ); +if (!pseries_log) +return rc; + + hp_elog = (struct pseries_hp_errorlog *)pseries_log->data; +if (!hp_elog) +return rc;I don't see how that can happen?
struct pseries_errorlog { __be16 id; /* 0x00 2-byte ASCII section ID */ __be16 length; /* 0x02 Section length in bytes */ uint8_t version; /* 0x04 Section version */ uint8_t subtype; /* 0x05 Section subtype */ __be16 creator_component; /* 0x06 Creator component ID */ uint8_t data[]; /* 0x08 Start of section data */ };Quoted 2 lines
Should you be checking for length == 0 instead ?
You are correct.
Quoted 4 lines
Also I think the code will probably end up cleaner if you do the endian conversions immediately when you read the
hp_elog, rather than passing it around in BE and having to remember to convert at all the usages.Agreed. I'll clean that up.
Quoted 11 lines
arch/powerpc/platforms/pseries/dlpar.c static ssize_t dlpar_cpu_release(const char *buf, size_t count) · line 555
Quoted 4 lines
arch/powerpc/platforms/pseries/dlpar.c +4
-0+switch (hp_elog->resource) { +case PSERIES_HP_ELOG_RESOURCE_MEM : + rc =dlpar_memory (hp_elog); +break ;Please add:
default:
pr_warn_ratelimited("Unknown resource ..")Or something.
can do.
Quoted 16 lines
Quoted 4 lines
arch/powerpc/platforms/pseries/dlpar.c +2
-0+ } +arch/powerpc/platforms/pseries/dlpar.c static ssize_t dlpar_cpu_release(const char *buf, size_t count) · line 567
Quoted 10 lines
arch/powerpc/platforms/pseries/dlpar.c +10
-0+return rc; +} + +static ssize_t dlpar_write (struct file *file,const char __user *buf, +size_t count,loff_t *offset) +{ +char *event_buf; +int rc; + + event_buf =kmalloc (count +1 ,GFP_KERNEL );Why + 1 ? It's not null-terminated AFAICS.
I think that's just habit. You're correct, it's not needed.
Quoted 13 lines
arch/powerpc/platforms/pseries/dlpar.c static ssize_t dlpar_cpu_release(const char *buf, size_t count) · line 577
Quoted 11 lines
arch/powerpc/platforms/pseries/dlpar.c +10
-0+if (!event_buf) +return -ENOMEM ; + + rc =copy_from_user (event_buf, buf, count); +if (rc) { +kfree (event_buf); +return rc; + } + + rc =handle_dlpar_errorlog ((struct rtas_error_log *)event_buf);If you start with a struct
rtas_error_log* you shouldn't need any casts.good point.
Quoted 21 lines
Quoted 7 lines
arch/powerpc/platforms/pseries/dlpar.c +6
-0+kfree (event_buf); +return rc ? rc : count; +} + +static const struct file_operations dlpar_fops = { + .write = dlpar_write,arch/powerpc/platforms/pseries/dlpar.c static ssize_t dlpar_cpu_release(const char *buf, size_t count) · line 593
Quoted 10 lines
arch/powerpc/platforms/pseries/dlpar.c +8
-0+ .llseek = noop_llseek, +}; +static int __init pseries_dlpar_init (void ) { +struct proc_dir_entry *proc_ent; + + proc_ent =proc_create ("powerpc/dlpar" ,S_IWUSR ,NULL , &dlpar_fops); +if (proc_ent) +proc_set_size (proc_ent,0 );else
error message at least pleaseQuoted 2 lines
Why are we putting it in /proc, can't it go in
/sys/kernellike the mobility stuff?I have no personal preference, I'll move it to
/sys/kerneland update the creation handling.Quoted 14 lines
arch/powerpc/platforms/pseries/hotplug-memory.c line 27
Quoted 12 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +3
-0@@ -20,6 +22,9 @@#include <asm/machdep.h> #include <asm/prom.h> #include <asm/sparsemem.h> +#include <asm/rtas.h> + +DEFINE_MUTEX (dlpar_mem_mutex);static ?
yes, that should have been static.
Quoted 9 lines
Quoted 9 lines
arch/powerpc/platforms/pseries/pseries.h +0
-0@@ -62,6 +63,15 @@ extern int dlpar_detach_node(struct device_node *);extern int dlpar_acquire_drc (u32 );extern int dlpar_release_drc (u32 );Quoted 8 lines
arch/powerpc/platforms/pseries/pseries.h extern int dlpar_detach_node(struct device_node *); · line 71
Quoted 6 lines
arch/powerpc/platforms/pseries/pseries.h +6
-0+#ifdef CONFIG_MEMORY_HOTPLUG +extern int dlpar_memory (struct pseries_hp_errorlog *); +#else +static inline int dlpar_memory (struct pseries_hp_errorlog *hp_elog) +{ +return -ENOTSUPP ;EOPNOTSUPP is a bit more standard.
ok.
Thanks for all the feedback.
-Nathan
_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [3/5] pseries: Create device hotplug entry point
Quoted 64 lines · Tyrel Datwyler
Quoted 54 lines · Nathan Fontenot
Quoted 19 lines · Michael Ellerman
Quoted 2 lines · Nathan Fontenot
For pseries system the kernel will be notified of hotplug requests in the form of rtas hotplug events.
Can you flesh that design out a bit for me, I don't entirely get how it's going to work.
The kernel gets the rtas hotplug events (in
rtasd.c) and spits them out to userspace, which then writes them back in ?Quoted 2 lines
This patch creates a common routine that can handle these requests in both the PowerVM anbd PowerKVM environments,
handle_dlpar_errorlog(). This also^Quoted 4 lines
creates the initial memory hotplug request handling stub.
For PowerVM this patch also creates a new /proc file that the drmgr command will use to write rtas hotplug events to.
Quoted 7 lines
Why is this different between phyp and KVM?
Quoted 2 lines
For future PowerKVM handling the rtas check-exception code can pass any rtas hotplug events received to
handle_dlpar_errorlog().Internally to the kernel you mean?
Perhaps a better explanation of how things work today and where I see them going is needed. I was trying to avoid a long explanation and I don't think my shortened explanation worked. I'll include this in v2 of the patchset too.
The current hotplug (or dlpar) of devices (the process is generally the same for memory, cpu, and pci) on PowerVM systems is initiated from the HMC, which communicates the request to the partitions through the RSCT framework. The RSCT framework then invokes the drmgr command. The drmgr command performs the hotplug operation by doing some pieces, such as most of the rtas calls and device tree parsing, in userspace and make requests to the kernel to online/offline the device, update the device tree and add/remove the device.
For PowerKVM the approach is to follow what is currently being done for pci hotplug. A hotplug request is initiated from the host. QEMU then sends an EPOW interrupt to the guest which causes the guest to make the rtas,check-exception call. In QEMU, the rtas,check-exception call returns a rtas hotplug event to the guest. I was using this same framework to also enable memory (and next cpu) hotplug.
You are correct that the current pci hotplug path for PowerKVM involves the kernel receiving the rtas event, passing it to
rtas_errdin userspace, and havingrtas_errdinvoke drmgr. The drmgr command then handles the request as described above for PowerVM systems.Quoted 8 lines
There is no need for this circuitous route, we should just handle the entire hotplug of devices in the kernel. What I am hoping to do is to enable this by moving the code to handle hotplug from drmgr into the kernel and provide a single path for handling hotplug for PowerVM and PowerKVM. To make this work for PowerKVM we will update the kernel rtas code to recognize rtas hotplug events returned from rtas,check-exception calls and call
handle_dlpar_errorlog(). The hotplug rtas event is never sent out to userspace.
Quoted 3 lines
Wouldn't we still want the event surfaced to userspace so that it can at least be logged?
The only logging of hotplug/dlpar events we do is putting a notification iv /var/log/messages. This is done today by the drmgr command.
I can add a pr_info message to log the hotplug/dlpar request and it's success/failure.
Also, I believe one of the longer term goals is to not require the rtas_errd daemon for PowerKVM.
-Nathan
PATCH 4/5 pseries: Implement memory hotplug add in the kernel
In reply to Nathan FontenotThis patch adds the ability to do memory hotplug adding in the kernel.
Currently the hotplug add/remove of memory is handled by the drmgr command. The drmgr command performs the add/remove by performing some work in user-space and making requests to the kernel to handle other pieces. By moving all of the work to the kernel we can do the add and remove faster, and provide a common place to do memory hotplug for both the PowerVM and PowerKVM environments.
--- arch/powerpc/platforms/pseries/hotplug-memory.c | 170 +++++++++++++++++++++++ 1 file changed, 170 insertions(+)
arch/powerpc/platforms/pseries/hotplug-memory.c +170 -0
@@ -17,6 +17,7 @@#include <linux/vmalloc.h> #include <linux/memory.h> #include <linux/memory_hotplug.h> +#include <linux/slab.h> #include <asm/firmware.h> #include <asm/machdep.h>
@@ -24,6 +25,8 @@#include <asm/sparsemem.h> #include <asm/rtas.h> +#include "pseries.h" +DEFINE_MUTEX (dlpar_mem_mutex);unsigned long pseries_memory_block_size (void )
@@ -69,6 +72,53 @@ unsigned long pseries_memory_block_size(void)return memblock_size; } +static void dlpar_free_drconf_property (struct property *prop) +{ +kfree (prop->name); +kfree (prop->value); +kfree (prop); +} + +static struct property *dlpar_clone_drconf_property (struct device_node *dn) +{ +struct property *prop, *new_prop; + + prop =of_find_property (dn,"ibm,dynamic-memory" ,NULL ); +if (!prop) +return NULL ; + + new_prop =kzalloc (sizeof (*new_prop),GFP_KERNEL ); +if (!new_prop) +return NULL ; + + new_prop->name =kstrdup (prop->name,GFP_KERNEL ); + new_prop->value =kmalloc (prop->length +1 ,GFP_KERNEL ); +if (!new_prop->name || !new_prop->value) { +dlpar_free_drconf_property (new_prop); +return NULL ; + } + +memcpy (new_prop->value, prop->value, prop->length); + new_prop->length = prop->length; + *(((char *)new_prop->value) + new_prop->length) =0 ; + +return new_prop; +} + +static struct memory_block *lmb_to_memblock (struct of_drconf_cell *lmb) +{ +unsigned long section_nr; +struct mem_section *mem_sect; +struct memory_block *mem_block; +u64 phys_addr =be64_to_cpu (lmb->base_addr); + + section_nr =pfn_to_section_nr (PFN_DOWN (phys_addr)); + mem_sect =__nr_to_section (section_nr); + + mem_block =find_memory_block (mem_sect); +return mem_block; +} +#ifdef CONFIG_MEMORY_HOTREMOVE static int pseries_remove_memory (u64 start,u64 size) {
@@ -155,13 +205,133 @@ static inline int pseries_remove_mem_node(struct device_node *np) }#endif /* CONFIG_MEMORY_HOTREMOVE */ +static int dlpar_add_one_lmb (struct of_drconf_cell *lmb) +{ +struct memory_block *mem_block; +u64 phys_addr; +unsigned long pages_per_block; +unsigned long block_sz; +int nid, sections_per_block; +int rc; + + phys_addr =be64_to_cpu (lmb->base_addr); + block_sz =memory_block_size_bytes (); + sections_per_block = block_sz /MIN_MEMORY_BLOCK_SIZE ; + pages_per_block =PAGES_PER_SECTION * sections_per_block; + +if (phys_addr & ((pages_per_block <<PAGE_SHIFT ) -1 )) +return -EINVAL ; + + nid =memory_add_physaddr_to_nid (phys_addr); + rc =add_memory (nid, phys_addr, block_sz); +if (rc) +return rc; + + rc =memblock_add (phys_addr, block_sz); +if (rc) { +remove_memory (nid, phys_addr, block_sz); +return rc; + } + + mem_block =lmb_to_memblock (lmb); +if (!mem_block) { +remove_memory (nid, phys_addr, block_sz); +return -EINVAL ; + } + + rc =device_online (&mem_block->dev); +put_device (&mem_block->dev); +if (rc) +remove_memory (nid, phys_addr, block_sz); + +return rc; +} + +static int dlpar_memory_add (struct pseries_hp_errorlog *hp_elog) +{ +struct of_drconf_cell *lmb; +struct device_node *dn; +struct property *prop; +uint32_t entries, *p; +int i, lmbs_to_add; +int lmbs_added =0 ; +int rc = -EINVAL ; + +if (hp_elog->id_type ==PSERIES_HP_ELOG_ID_DRC_COUNT ) { + lmbs_to_add =be32_to_cpu (hp_elog->_drc_u.drc_count); +pr_info ("Attempting to hot-add %d LMB(s)\n" , lmbs_to_add); + }else { + lmbs_to_add =1 ; +pr_info ("Attempting to hot-add LMB, drc index %x\n" , +be32_to_cpu (hp_elog->_drc_u.drc_index)); + } + + dn =of_find_node_by_path ("/ibm,dynamic-reconfiguration-memory" ); +if (!dn) +return -EINVAL ; + + prop =dlpar_clone_drconf_property (dn); +if (!prop) { +of_node_put (dn); +return -EINVAL ; + } + + p = prop->value; + entries =be32_to_cpu (*p++); + lmb = (struct of_drconf_cell *)p; + +for (i =0 ; i < entries; i++, lmb++) { +u32 drc_index =be32_to_cpu (lmb->drc_index); + +if (lmbs_to_add == lmbs_added) +break ; + +if (be32_to_cpu (lmb->flags) &DRCONF_MEM_ASSIGNED ) +continue ; + +if (hp_elog->id_type ==PSERIES_HP_ELOG_ID_DRC_INDEX + && lmb->drc_index != hp_elog->_drc_u.drc_index) +continue ; + + rc =dlpar_acquire_drc (drc_index); +if (rc) +continue ; + + rc =dlpar_add_one_lmb (lmb); +if (rc) { +dlpar_release_drc (drc_index); +continue ; + } + + lmb->flags |=cpu_to_be32 (DRCONF_MEM_ASSIGNED ); + lmbs_added++; +pr_info ("Memory at %llx (drc index %x) has been hot-added\n" , +be64_to_cpu (lmb->base_addr), drc_index); + } + +if (lmbs_added) + rc =of_update_property (dn, prop); +else +dlpar_free_drconf_property (prop); + +of_node_put (dn); +return rc ? rc : lmbs_added; +} +int dlpar_memory (struct pseries_hp_errorlog *hp_elog) {int rc =0 ; +if (hp_elog->id_type !=PSERIES_HP_ELOG_ID_DRC_COUNT + && hp_elog->id_type !=PSERIES_HP_ELOG_ID_DRC_INDEX ) +return -EINVAL ; +mutex_lock (&dlpar_mem_mutex);switch (hp_elog->action) { +case PSERIES_HP_ELOG_ACTION_ADD : + rc =dlpar_memory_add (hp_elog); +break ;default :pr_err ("Invalid action (%d) specified\n" , hp_elog->action); rc = -EINVAL ;
Michael Ellerman
reviewerRe: [4/5] pseries: Implement memory hotplug add in the kernel
Quoted 58 lines · Nathan Fontenot
This patch adds the ability to do memory hotplug adding in the kernel.
Currently the hotplug add/remove of memory is handled by the drmgr command. The drmgr command performs the add/remove by performing some work in user-space and making requests to the kernel to handle other pieces. By moving all of the work to the kernel we can do the add and remove faster, and provide a common place to do memory hotplug for both the PowerVM and PowerKVM environments.
Signed-off-by Nathan Fontenot <[email protected]>--- arch/powerpc/platforms/pseries/hotplug-memory.c | 170 +++++++++++++++++++++++ 1 file changed, 170 insertions(+)arch/powerpc/platforms/pseries/hotplug-memory.c +22
-0@@ -17,6 +17,7 @@#include <linux/vmalloc.h> #include <linux/memory.h> #include <linux/memory_hotplug.h> +#include <linux/slab.h> #include <asm/firmware.h> #include <asm/machdep.h> @@ -24,6 +25,8 @@#include <asm/sparsemem.h> #include <asm/rtas.h> +#include "pseries.h" +DEFINE_MUTEX (dlpar_mem_mutex);unsigned long pseries_memory_block_size (void )@@ -69,6 +72,53 @@ unsigned long pseries_memory_block_size(void)return memblock_size; } +static void dlpar_free_drconf_property (struct property *prop) +{ +kfree (prop->name); +kfree (prop->value); +kfree (prop); +} + +static struct property *dlpar_clone_drconf_property (struct device_node *dn) +{ +struct property *prop, *new_prop; + + prop =of_find_property (dn,"ibm,dynamic-memory" ,NULL ); +if (!prop) +return NULL ; + + new_prop =kzalloc (sizeof (*new_prop),GFP_KERNEL ); +if (!new_prop) +return NULL ; +
arch/powerpc/platforms/pseries/hotplug-memory.c unsigned long pseries_memory_block_size(void) · line 103
Quoted 10 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +10
-0+ new_prop->name =kstrdup (prop->name,GFP_KERNEL ); + new_prop->value =kmalloc (prop->length +1 ,GFP_KERNEL ); +if (!new_prop->name || !new_prop->value) { +dlpar_free_drconf_property (new_prop); +return NULL ; + } + +memcpy (new_prop->value, prop->value, prop->length); + new_prop->length = prop->length; + *(((char *)new_prop->value) + new_prop->length) =0 ;
It's not a string, is it?
Quoted 24 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +17
-0+return new_prop; +} + +static struct memory_block *lmb_to_memblock (struct of_drconf_cell *lmb) +{ +unsigned long section_nr; +struct mem_section *mem_sect; +struct memory_block *mem_block; +u64 phys_addr =be64_to_cpu (lmb->base_addr); + + section_nr =pfn_to_section_nr (PFN_DOWN (phys_addr)); + mem_sect =__nr_to_section (section_nr); + + mem_block =find_memory_block (mem_sect); +return mem_block; +} +#ifdef CONFIG_MEMORY_HOTREMOVE static int pseries_remove_memory (u64 start,u64 size) {@@ -155,13 +205,133 @@ static inline int pseries_remove_mem_node(struct device_node *np) }#endif /* CONFIG_MEMORY_HOTREMOVE */
arch/powerpc/platforms/pseries/hotplug-memory.c static inline int pseries_remove_mem_node(struct device_node *np) · line 217
Quoted 10 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +10
-0+static int dlpar_add_one_lmb (struct of_drconf_cell *lmb) +{ +struct memory_block *mem_block; +u64 phys_addr; +unsigned long pages_per_block; +unsigned long block_sz; +int nid, sections_per_block; +int rc; + + phys_addr =be64_to_cpu (lmb->base_addr);
of_drconf_cell needs endian annotations.
Quoted 13 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +13
-0+ block_sz =memory_block_size_bytes (); + sections_per_block = block_sz /MIN_MEMORY_BLOCK_SIZE ; + pages_per_block =PAGES_PER_SECTION * sections_per_block; + +if (phys_addr & ((pages_per_block <<PAGE_SHIFT ) -1 )) +return -EINVAL ; + + nid =memory_add_physaddr_to_nid (phys_addr); + rc =add_memory (nid, phys_addr, block_sz); +if (rc) +return rc; + + rc =memblock_add (phys_addr, block_sz);
arch/powerpc/platforms/pseries/hotplug-memory.c static inline int pseries_remove_mem_node(struct device_node *np) · line 240
Quoted 10 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +10
-0+if (rc) { +remove_memory (nid, phys_addr, block_sz); +return rc; + } + + mem_block =lmb_to_memblock (lmb); +if (!mem_block) { +remove_memory (nid, phys_addr, block_sz); +return -EINVAL ; + }
That could all use a lot of comments. ie. why do we have to add it twice?
Quoted 4 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +4
-0+ rc =device_online (&mem_block->dev); +put_device (&mem_block->dev); +if (rc) +remove_memory (nid, phys_addr, block_sz);
arch/powerpc/platforms/pseries/hotplug-memory.c static inline int pseries_remove_mem_node(struct device_node *np) · line 255
Quoted 10 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +10
-0+ +return rc; +} + +static int dlpar_memory_add (struct pseries_hp_errorlog *hp_elog) +{ +struct of_drconf_cell *lmb; +struct device_node *dn; +struct property *prop; +uint32_t entries, *p;
*p should be __be32.
arch/powerpc/platforms/pseries/hotplug-memory.c static inline int pseries_remove_mem_node(struct device_node *np) · line 258
Quoted 3 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +3
-0+int i, lmbs_to_add; +int lmbs_added =0 ; +int rc = -EINVAL ;
Don't pre-initialise your rc variables.
Quoted 12 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +12
-0+if (hp_elog->id_type ==PSERIES_HP_ELOG_ID_DRC_COUNT ) { + lmbs_to_add =be32_to_cpu (hp_elog->_drc_u.drc_count); +pr_info ("Attempting to hot-add %d LMB(s)\n" , lmbs_to_add); + }else { + lmbs_to_add =1 ; +pr_info ("Attempting to hot-add LMB, drc index %x\n" , +be32_to_cpu (hp_elog->_drc_u.drc_index)); + } + + dn =of_find_node_by_path ("/ibm,dynamic-reconfiguration-memory" ); +if (!dn) +return -EINVAL ;
arch/powerpc/platforms/pseries/hotplug-memory.c static inline int pseries_remove_mem_node(struct device_node *np) · line 281
Quoted 10 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +10
-0+ + prop =dlpar_clone_drconf_property (dn); +if (!prop) { +of_node_put (dn); +return -EINVAL ; + } + + p = prop->value; + entries =be32_to_cpu (*p++); + lmb = (struct of_drconf_cell *)p;
So if I'm reading this right the hp_elog either contains an index or a count of LMBs to add. But it doesn't contain anything about which address ranges to add or any of those details. That is all in the ibm,dynamic-memory property - but how did it get in there?
Quoted 13 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +13
-0+ +for (i =0 ; i < entries; i++, lmb++) { +u32 drc_index =be32_to_cpu (lmb->drc_index); + +if (lmbs_to_add == lmbs_added) +break ; + +if (be32_to_cpu (lmb->flags) &DRCONF_MEM_ASSIGNED ) +continue ; + +if (hp_elog->id_type ==PSERIES_HP_ELOG_ID_DRC_INDEX + && lmb->drc_index != hp_elog->_drc_u.drc_index) +continue ;
arch/powerpc/platforms/pseries/hotplug-memory.c static inline int pseries_remove_mem_node(struct device_node *np) · line 304
Quoted 10 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +10
-0+ + rc =dlpar_acquire_drc (drc_index); +if (rc) +continue ; + + rc =dlpar_add_one_lmb (lmb); +if (rc) { +dlpar_release_drc (drc_index); +continue ; + }
In both the above error cases you just move along. That means we potentially hotplugged some memory but not everything that we were asked to. That seems like a bad idea, we should either do everything or nothing.
arch/powerpc/platforms/pseries/hotplug-memory.c static inline int pseries_remove_mem_node(struct device_node *np) · line 315
Quoted 11 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +11
-0+ + lmb->flags |=cpu_to_be32 (DRCONF_MEM_ASSIGNED ); + lmbs_added++; +pr_info ("Memory at %llx (drc index %x) has been hot-added\n" , +be64_to_cpu (lmb->base_addr), drc_index); + } + +if (lmbs_added) + rc =of_update_property (dn, prop); +else +dlpar_free_drconf_property (prop);
The value of rc here is not clear. It could be EINVAL or it could be the result of the last dlpar_add_one_lmb(lmb). gcc would have told you that if you hadn't initialised it.
arch/powerpc/platforms/pseries/hotplug-memory.c static inline int pseries_remove_mem_node(struct device_node *np) · line 318
Quoted 3 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +3
-0+ +of_node_put (dn); +return rc ? rc : lmbs_added;
This looks wrong.
Doesn't the rc eventually go back to dlpar_write(), which expects 0 for success?
That should show up as the write failing in userspace.
arch/powerpc/platforms/pseries/hotplug-memory.c static inline int pseries_remove_mem_node(struct device_node *np) · line 323
Quoted 3 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +0
-0int dlpar_memory (struct pseries_hp_errorlog *hp_elog) {int rc =0 ;
Don't initialise to zero, that way gcc can tell you if there's a path where you forget to initialise it. It also means you can't accidentally return success.
arch/powerpc/platforms/pseries/hotplug-memory.c static inline int pseries_remove_mem_node(struct device_node *np) · line 327
Quoted 3 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +3
-0+if (hp_elog->id_type !=PSERIES_HP_ELOG_ID_DRC_COUNT + && hp_elog->id_type !=PSERIES_HP_ELOG_ID_DRC_INDEX ) +return -EINVAL ;
This would look nicer as a switch I think.
arch/powerpc/platforms/pseries/hotplug-memory.c static inline int pseries_remove_mem_node(struct device_node *np)
Quoted 10 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +3
-0mutex_lock (&dlpar_mem_mutex);switch (hp_elog->action) { +case PSERIES_HP_ELOG_ACTION_ADD : + rc =dlpar_memory_add (hp_elog); +break ;default :pr_err ("Invalid action (%d) specified\n" , hp_elog->action); rc = -EINVAL ;
cheers
Re: [4/5] pseries: Implement memory hotplug add in the kernel
Quoted 64 lines · Michael Ellerman
Quoted 62 lines · Nathan Fontenot
This patch adds the ability to do memory hotplug adding in the kernel.
Currently the hotplug add/remove of memory is handled by the drmgr command. The drmgr command performs the add/remove by performing some work in user-space and making requests to the kernel to handle other pieces. By moving all of the work to the kernel we can do the add and remove faster, and provide a common place to do memory hotplug for both the PowerVM and PowerKVM environments.
Signed-off-by Nathan Fontenot <[email protected]>--- arch/powerpc/platforms/pseries/hotplug-memory.c | 170 +++++++++++++++++++++++ 1 file changed, 170 insertions(+)arch/powerpc/platforms/pseries/hotplug-memory.c +26
-0@@ -17,6 +17,7 @@#include <linux/vmalloc.h> #include <linux/memory.h> #include <linux/memory_hotplug.h> +#include <linux/slab.h> #include <asm/firmware.h> #include <asm/machdep.h> @@ -24,6 +25,8 @@#include <asm/sparsemem.h> #include <asm/rtas.h> +#include "pseries.h" +DEFINE_MUTEX (dlpar_mem_mutex);unsigned long pseries_memory_block_size (void )@@ -69,6 +72,53 @@ unsigned long pseries_memory_block_size(void)return memblock_size; } +static void dlpar_free_drconf_property (struct property *prop) +{ +kfree (prop->name); +kfree (prop->value); +kfree (prop); +} + +static struct property *dlpar_clone_drconf_property (struct device_node *dn) +{ +struct property *prop, *new_prop; + + prop =of_find_property (dn,"ibm,dynamic-memory" ,NULL ); +if (!prop) +return NULL ; + + new_prop =kzalloc (sizeof (*new_prop),GFP_KERNEL ); +if (!new_prop) +return NULL ; + + new_prop->name =kstrdup (prop->name,GFP_KERNEL ); + new_prop->value =kmalloc (prop->length +1 ,GFP_KERNEL ); +if (!new_prop->name || !new_prop->value) { +dlpar_free_drconf_property (new_prop);
Quoted 8 lines
arch/powerpc/platforms/pseries/hotplug-memory.c unsigned long pseries_memory_block_size(void) · line 103
Quoted 6 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +6
-0+return NULL ; + } + +memcpy (new_prop->value, prop->value, prop->length); + new_prop->length = prop->length; + *(((char *)new_prop->value) + new_prop->length) =0 ;It's not a string, is it?
No, property->value is a void*. I'll drop that line of code.
Quoted 25 lines
Quoted 25 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +17
-0+return new_prop; +} + +static struct memory_block *lmb_to_memblock (struct of_drconf_cell *lmb) +{ +unsigned long section_nr; +struct mem_section *mem_sect; +struct memory_block *mem_block; +u64 phys_addr =be64_to_cpu (lmb->base_addr); + + section_nr =pfn_to_section_nr (PFN_DOWN (phys_addr)); + mem_sect =__nr_to_section (section_nr); + + mem_block =find_memory_block (mem_sect); +return mem_block; +} +#ifdef CONFIG_MEMORY_HOTREMOVE static int pseries_remove_memory (u64 start,u64 size) {@@ -155,13 +205,133 @@ static inline int pseries_remove_mem_node(struct device_node *np) }#endif /* CONFIG_MEMORY_HOTREMOVE */
Quoted 12 lines
arch/powerpc/platforms/pseries/hotplug-memory.c static inline int pseries_remove_mem_node(struct device_node *np) · line 217
Quoted 10 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +10
-0+static int dlpar_add_one_lmb (struct of_drconf_cell *lmb) +{ +struct memory_block *mem_block; +u64 phys_addr; +unsigned long pages_per_block; +unsigned long block_sz; +int nid, sections_per_block; +int rc; + + phys_addr =be64_to_cpu (lmb->base_addr);
of_drconf_cellneeds endian annotations.
Yes it does. I can include a patch to update the struct.
Quoted 18 lines
Quoted 18 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +17
-0+ block_sz =memory_block_size_bytes (); + sections_per_block = block_sz /MIN_MEMORY_BLOCK_SIZE ; + pages_per_block =PAGES_PER_SECTION * sections_per_block; + +if (phys_addr & ((pages_per_block <<PAGE_SHIFT ) -1 )) +return -EINVAL ; + + nid =memory_add_physaddr_to_nid (phys_addr); + rc =add_memory (nid, phys_addr, block_sz); +if (rc) +return rc; + + rc =memblock_add (phys_addr, block_sz); +if (rc) { +remove_memory (nid, phys_addr, block_sz); +return rc; + }
Quoted 8 lines
arch/powerpc/platforms/pseries/hotplug-memory.c static inline int pseries_remove_mem_node(struct device_node *np) · line 240
Quoted 6 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +6
-0+ + mem_block =lmb_to_memblock (lmb); +if (!mem_block) { +remove_memory (nid, phys_addr, block_sz); +return -EINVAL ; + }That could all use a lot of comments. ie. why do we have to add it twice?
We don't actually add it twice, though I can see how one could think that based on the names of the routines called. I'll add comments to clarify this in v2 of the patch.
memory_add_physaddr_to_nid(), this doesn't add anything despite its naming. The routine finds the node id for the specified physical address.
add_memory(), this actually adds the memory.
memblock_add(), this informs the memory block information tracking about the newly added memory. Why this is not done as part of add_memory I don't know.
Quoted 17 lines
Quoted 5 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +4
-0+ rc =device_online (&mem_block->dev); +put_device (&mem_block->dev); +if (rc) +remove_memory (nid, phys_addr, block_sz);arch/powerpc/platforms/pseries/hotplug-memory.c static inline int pseries_remove_mem_node(struct device_node *np) · line 255
Quoted 10 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +10
-0+ +return rc; +} + +static int dlpar_memory_add (struct pseries_hp_errorlog *hp_elog) +{ +struct of_drconf_cell *lmb; +struct device_node *dn; +struct property *prop; +uint32_t entries, *p;*p should be
__be32.
Yes.
Quoted 6 lines
arch/powerpc/platforms/pseries/hotplug-memory.c static inline int pseries_remove_mem_node(struct device_node *np) · line 258
Quoted 4 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +3
-0+int i, lmbs_to_add; +int lmbs_added =0 ; +int rc = -EINVAL ;Don't pre-initialise your rc variables.
I did this here for a reason. When asking to add memory by drc_index it is possible to fall out of the for() loop traversing the lmb entries and not find the requested drc_index.
Adding a check for this situation after the loop would do the same thing and probably make this situation more clear.
Quoted 21 lines
Quoted 21 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +20
-0+if (hp_elog->id_type ==PSERIES_HP_ELOG_ID_DRC_COUNT ) { + lmbs_to_add =be32_to_cpu (hp_elog->_drc_u.drc_count); +pr_info ("Attempting to hot-add %d LMB(s)\n" , lmbs_to_add); + }else { + lmbs_to_add =1 ; +pr_info ("Attempting to hot-add LMB, drc index %x\n" , +be32_to_cpu (hp_elog->_drc_u.drc_index)); + } + + dn =of_find_node_by_path ("/ibm,dynamic-reconfiguration-memory" ); +if (!dn) +return -EINVAL ; + + prop =dlpar_clone_drconf_property (dn); +if (!prop) { +of_node_put (dn); +return -EINVAL ; + } + + p = prop->value;
Quoted 8 lines
arch/powerpc/platforms/pseries/hotplug-memory.c static inline int pseries_remove_mem_node(struct device_node *np) · line 281
Quoted 2 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +2
-0+ entries =be32_to_cpu (*p++); + lmb = (struct of_drconf_cell *)p;So if I'm reading this right the
hp_elogeither contains an index or a count of LMBs to add. But it doesn't contain anything about which address ranges to add or any of those details. That is all in the ibm,dynamic-memory property - but how did it get in there?
The ibm,dynamic-memory property of the device tree is passed to the kernel by phyp/qemu. The property is an array that associates each LMB with a starting physical address and associativity.
Quoted 20 lines
Quoted 20 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +19
-0+ +for (i =0 ; i < entries; i++, lmb++) { +u32 drc_index =be32_to_cpu (lmb->drc_index); + +if (lmbs_to_add == lmbs_added) +break ; + +if (be32_to_cpu (lmb->flags) &DRCONF_MEM_ASSIGNED ) +continue ; + +if (hp_elog->id_type ==PSERIES_HP_ELOG_ID_DRC_INDEX + && lmb->drc_index != hp_elog->_drc_u.drc_index) +continue ; + + rc =dlpar_acquire_drc (drc_index); +if (rc) +continue ; + + rc =dlpar_add_one_lmb (lmb);
Quoted 10 lines
arch/powerpc/platforms/pseries/hotplug-memory.c static inline int pseries_remove_mem_node(struct device_node *np) · line 304
Quoted 4 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +4
-0+if (rc) { +dlpar_release_drc (drc_index); +continue ; + }In both the above error cases you just move along. That means we potentially hotplugged some memory but not everything that we were asked to. That seems like a bad idea, we should either do everything or nothing.
That can be done, though will require some additional tracking.
The current hotplug handling for PowerVM make a best effort and tries to add/remove as much of the requested memory as possible. I was going with that same approach here, but have no problem moving to an all or nothing approach.
We will need to keep track of the LMBs added/removed during a request so we can return to the original state if the request cannot be satisfied.
Quoted 22 lines
arch/powerpc/platforms/pseries/hotplug-memory.c static inline int pseries_remove_mem_node(struct device_node *np) · line 315
Quoted 11 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +11
-0+ + lmb->flags |=cpu_to_be32 (DRCONF_MEM_ASSIGNED ); + lmbs_added++; +pr_info ("Memory at %llx (drc index %x) has been hot-added\n" , +be64_to_cpu (lmb->base_addr), drc_index); + } + +if (lmbs_added) + rc =of_update_property (dn, prop); +else +dlpar_free_drconf_property (prop);The value of rc here is not clear. It could be EINVAL or it could be the result of the last
dlpar_add_one_lmb(lmb). gcc would have told you that if you hadn't initialised it.arch/powerpc/platforms/pseries/hotplug-memory.c static inline int pseries_remove_mem_node(struct device_node *np) · line 318
Quoted 3 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +3
-0+ +of_node_put (dn); +return rc ? rc : lmbs_added;This looks wrong.
Quoted 5 lines
Doesn't the rc eventually go back to
dlpar_write(), which expects 0 for success?That should show up as the write failing in userspace.
Based on previous comments I think the handling of rc will be updated so we either return success or failure.
Quoted 4 lines
arch/powerpc/platforms/pseries/hotplug-memory.c static inline int pseries_remove_mem_node(struct device_node *np) · line 323
Quoted 3 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +0
-0int dlpar_memory (struct pseries_hp_errorlog *hp_elog) {int rc =0 ;
Quoted 8 lines
Don't initialise to zero, that way gcc can tell you if there's a path where you forget to initialise it. It also means you can't accidentally return success.
arch/powerpc/platforms/pseries/hotplug-memory.c static inline int pseries_remove_mem_node(struct device_node *np) · line 327
Quoted 3 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +3
-0+if (hp_elog->id_type !=PSERIES_HP_ELOG_ID_DRC_COUNT + && hp_elog->id_type !=PSERIES_HP_ELOG_ID_DRC_INDEX ) +return -EINVAL ;This would look nicer as a switch I think.
can do.
-Nathan
Re: [4/5] pseries: Implement memory hotplug add in the kernel
In reply to Michael EllermanQuoted 32 lines · Michael Ellerman
Quoted 30 lines · Nathan Fontenot
This patch adds the ability to do memory hotplug adding in the kernel.
Currently the hotplug add/remove of memory is handled by the drmgr command. The drmgr command performs the add/remove by performing some work in user-space and making requests to the kernel to handle other pieces. By moving all of the work to the kernel we can do the add and remove faster, and provide a common place to do memory hotplug for both the PowerVM and PowerKVM environments.
Signed-off-by Nathan Fontenot <[email protected]>---
arch/powerpc/platforms/pseries/hotplug-memory.c +18
-0+for (i =0 ; i < entries; i++, lmb++) { +u32 drc_index =be32_to_cpu (lmb->drc_index); + +if (lmbs_to_add == lmbs_added) +break ; + +if (be32_to_cpu (lmb->flags) &DRCONF_MEM_ASSIGNED ) +continue ; + +if (hp_elog->id_type ==PSERIES_HP_ELOG_ID_DRC_INDEX + && lmb->drc_index != hp_elog->_drc_u.drc_index) +continue ; + + rc =dlpar_acquire_drc (drc_index); +if (rc) +continue ; + + rc =dlpar_add_one_lmb (lmb);
Quoted 9 lines
arch/powerpc/platforms/pseries/hotplug-memory.c static inline int pseries_remove_mem_node(struct device_node *np) · line 304
Quoted 4 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +4
-0+if (rc) { +dlpar_release_drc (drc_index); +continue ; + }In both the above error cases you just move along. That means we potentially hotplugged some memory but not everything that we were asked to. That seems like a bad idea, we should either do everything or nothing.
Michael, how set are you on the all or nothing approach?
Note that I think the all or nothing approach is best but I think it will present some problems. We do memory add (and remove) on a LMB basis, so it is possible to hit a scenario in which we cannot revert back to the original state. For example, a request to add 5 LMBs only succeeds in adding 4 LMBs. There is no guarantee that we then remove the 4 MLBs that were added. That memory could be in use somewhere that it cannot be moved.
I would suggest we continue with the current approach in that we try to satisfy the request but not try to roll-back the changes if the entire request cannot be satisfied.
-Nathan
PATCH 5/5 pseries: Implement memory hotplug remove in the kernel
In reply to Nathan FontenotThis patch adds the ability to do memory hotplug remove in the kernel.
Currently the hotplug add/remove of memory is handled by the drmgr command. The drmgr command performs the add/remove by performing some work in user-space and making requests to the kernel to handle other pieces. By moving all of the work to the kernel we can do the add and remove faster, and provide a common place to do memory hotplug for both the PowerVM and PowerKVM environments.
--- arch/powerpc/platforms/pseries/hotplug-memory.c | 140 +++++++++++++++++++++++ 1 file changed, 139 insertions(+), 1 deletion(-)
arch/powerpc/platforms/pseries/hotplug-memory.c +139 -1
@@ -193,7 +193,137 @@ static int pseries_remove_mem_node(struct device_node *np)pseries_remove_memblock (base, lmb_size);return 0 ; } + +static int lmb_is_removable (struct of_drconf_cell *lmb) +{ +int i, scns_per_block; +int rc =1 ; +unsigned long pfn, block_sz; +u64 phys_addr; + + phys_addr =be64_to_cpu (lmb->base_addr); + block_sz =memory_block_size_bytes (); + scns_per_block = block_sz /MIN_MEMORY_BLOCK_SIZE ; + +for (i =0 ; i < scns_per_block; i++) { + pfn =PFN_DOWN (phys_addr); +if (!pfn_present (pfn)) +continue ; + + rc &=is_mem_section_removable (pfn,PAGES_PER_SECTION ); + phys_addr +=MIN_MEMORY_BLOCK_SIZE ; + } + +return rc; +} + +static int dlpar_add_one_lmb (struct of_drconf_cell *); + +static int dlpar_remove_one_lmb (struct of_drconf_cell *lmb) +{ +struct memory_block *mem_block; +unsigned long block_sz; +u64 phys_addr; +int nid, rc; + + block_sz =memory_block_size_bytes (); + phys_addr =be64_to_cpu (lmb->base_addr); + nid =memory_add_physaddr_to_nid (phys_addr); + +if (!pfn_valid (phys_addr >>PAGE_SHIFT )) { +memblock_remove (phys_addr, block_sz); +return 0 ; + } + + mem_block =lmb_to_memblock (lmb); +if (!mem_block) +return -EINVAL ; + + rc =device_offline (&mem_block->dev); +put_device (&mem_block->dev); +if (rc) +return rc; + +remove_memory (nid, phys_addr, block_sz); +memblock_remove (phys_addr, block_sz); + +return 0 ; +} + +static int dlpar_memory_remove (struct pseries_hp_errorlog *hp_elog) +{ +struct of_drconf_cell *lmb; +struct device_node *dn; +struct property *prop; +int lmbs_to_remove, lmbs_removed =0 ; +int i, entries; +int rc = -EINVAL ; +uint32_t *p; + +if (hp_elog->id_type ==PSERIES_HP_ELOG_ID_DRC_COUNT ) { + lmbs_to_remove =be32_to_cpu (hp_elog->_drc_u.drc_count); +pr_info ("Attempting to hot-remove %d LMB(s)\n" , lmbs_to_remove); + }else { + lmbs_to_remove =1 ; +pr_info ("Attempting to hot-remove LMB, drc index %x\n" , +be32_to_cpu (hp_elog->_drc_u.drc_index)); + } + + dn =of_find_node_by_path ("/ibm,dynamic-reconfiguration-memory" ); +if (!dn) +return -EINVAL ; + + prop =dlpar_clone_drconf_property (dn); +if (!prop) { +of_node_put (dn); +return -EINVAL ; + } + + p = prop->value; + entries =be32_to_cpu (*p++); + lmb = (struct of_drconf_cell *)p; + +for (i =0 ; i < entries; i++, lmb++) { +u32 drc_index =be32_to_cpu (lmb->drc_index); + +if (lmbs_to_remove == lmbs_removed) +break ; + +if (hp_elog->id_type ==PSERIES_HP_ELOG_ID_DRC_INDEX + && lmb->drc_index != hp_elog->_drc_u.drc_index) +continue ; + +if (!(be32_to_cpu (lmb->flags) &DRCONF_MEM_ASSIGNED ) + || !lmb_is_removable (lmb)) +continue ; + + rc =dlpar_remove_one_lmb (lmb); +if (rc) +continue ; + + rc =dlpar_release_drc (drc_index); +if (rc) { +dlpar_add_one_lmb (lmb); +continue ; + } + + lmb->flags &=cpu_to_be32 (~DRCONF_MEM_ASSIGNED ); + lmbs_removed++; +pr_info ("Memory at %llx (drc index %x) has been hot-removed\n" , +be64_to_cpu (lmb->base_addr), drc_index); + } + +if (lmbs_removed) + rc =of_update_property (dn, prop); +else +dlpar_free_drconf_property (prop); + +of_node_put (dn); +return rc ? rc : lmbs_removed; +} +#else +static inline int pseries_remove_memblock (unsigned long base,unsigned int memblock_size) {
@@ -203,6 +333,11 @@ static inline int pseries_remove_mem_node(struct device_node *np) {return 0 ; } +static inline int dlpar_memory_remove (struct pseries_hp_errorlog *hp_elog) +{ +return -EOPNOTSUPP ; +} +#endif /* CONFIG_MEMORY_HOTREMOVE */ static int dlpar_add_one_lmb (struct of_drconf_cell *lmb)
@@ -320,7 +455,7 @@ static int dlpar_memory_add(struct pseries_hp_errorlog *hp_elog)int dlpar_memory (struct pseries_hp_errorlog *hp_elog) { -int rc =0 ; +int rc;if (hp_elog->id_type !=PSERIES_HP_ELOG_ID_DRC_COUNT && hp_elog->id_type !=PSERIES_HP_ELOG_ID_DRC_INDEX )
@@ -332,6 +467,9 @@ int dlpar_memory(struct pseries_hp_errorlog *hp_elog)case PSERIES_HP_ELOG_ACTION_ADD : rc =dlpar_memory_add (hp_elog);break ; +case PSERIES_HP_ELOG_ACTION_REMOVE : + rc =dlpar_memory_remove (hp_elog); +break ;default :pr_err ("Invalid action (%d) specified\n" , hp_elog->action); rc = -EINVAL ;
Michael Ellerman
reviewerRe: [5/5] pseries: Implement memory hotplug remove in the kernel
Quoted 19 lines · Nathan Fontenot
This patch adds the ability to do memory hotplug remove in the kernel.
Currently the hotplug add/remove of memory is handled by the drmgr command. The drmgr command performs the add/remove by performing some work in user-space and making requests to the kernel to handle other pieces. By moving all of the work to the kernel we can do the add and remove faster, and provide a common place to do memory hotplug for both the PowerVM and PowerKVM environments.
Signed-off-by Nathan Fontenot <[email protected]>--- arch/powerpc/platforms/pseries/hotplug-memory.c | 140 +++++++++++++++++++++++ 1 file changed, 139 insertions(+), 1 deletion(-)arch/powerpc/platforms/pseries/hotplug-memory.c +0
-0
arch/powerpc/platforms/pseries/hotplug-memory.c static int pseries_remove_mem_node(struct device_node *np) · line 198
Quoted 6 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +3
-0@@ -193,7 +193,137 @@ static int pseries_remove_mem_node(struct device_node *np)pseries_remove_memblock (base, lmb_size);return 0 ; } + +static int lmb_is_removable (struct of_drconf_cell *lmb) +{
Do we not already have something like this?
arch/powerpc/platforms/pseries/hotplug-memory.c static int pseries_remove_mem_node(struct device_node *np) · line 200
Quoted 2 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +2
-0+int i, scns_per_block; +int rc =1 ;
I can see this makes the &= work below.
But what if block_sz / MIN_MEMORY_BLOCK_SIZE = 0 ?
Quoted 21 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +20
-0+unsigned long pfn, block_sz; +u64 phys_addr; + + phys_addr =be64_to_cpu (lmb->base_addr); + block_sz =memory_block_size_bytes (); + scns_per_block = block_sz /MIN_MEMORY_BLOCK_SIZE ; + +for (i =0 ; i < scns_per_block; i++) { + pfn =PFN_DOWN (phys_addr); +if (!pfn_present (pfn)) +continue ; + + rc &=is_mem_section_removable (pfn,PAGES_PER_SECTION ); + phys_addr +=MIN_MEMORY_BLOCK_SIZE ; + } + +return rc; +} +static int dlpar_memory_remove (struct pseries_hp_errorlog *hp_elog) +{
...
Quoted 1 line
+}
Most of the same comments as for add.
cheers
Re: [5/5] pseries: Implement memory hotplug remove in the kernel
Quoted 16 lines · Michael Ellerman
Quoted 14 lines · Nathan Fontenot
This patch adds the ability to do memory hotplug remove in the kernel.
Currently the hotplug add/remove of memory is handled by the drmgr command. The drmgr command performs the add/remove by performing some work in user-space and making requests to the kernel to handle other pieces. By moving all of the work to the kernel we can do the add and remove faster, and provide a common place to do memory hotplug for both the PowerVM and PowerKVM environments.
Signed-off-by Nathan Fontenot <[email protected]>--- arch/powerpc/platforms/pseries/hotplug-memory.c | 140 +++++++++++++++++++++++ 1 file changed, 139 insertions(+), 1 deletion(-)
Quoted 13 lines
arch/powerpc/platforms/pseries/hotplug-memory.c static int pseries_remove_mem_node(struct device_node *np) · line 198
Quoted 11 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +3
-0@@ -193,7 +193,137 @@ static int pseries_remove_mem_node(struct device_node *np)pseries_remove_memblock (base, lmb_size);return 0 ; } + +static int lmb_is_removable (struct of_drconf_cell *lmb) +{Do we not already have something like this?
No. Perhaps your thinking of the code in drivers/base/memory.c that handles the sysfs removable file. That code just calls the same is_mem_section_removable() routine.
Quoted 7 lines
arch/powerpc/platforms/pseries/hotplug-memory.c static int pseries_remove_mem_node(struct device_node *np) · line 200
Quoted 3 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +2
-0+int i, scns_per_block; +int rc =1 ;I can see this makes the &= work below.
But what if
block_sz/MIN_MEMORY_BLOCK_SIZE= 0 ?
If that happens, something else is really wrong. Most likely a malformed device tree.
For pseries MIN_MEMORY_BLOCK_SIZE is defined to be the smallest LMB size we suppport, 16MB.
I can add a pr_warn() statement here and bail if that happens.
Quoted 20 lines
Quoted 19 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +18
-0+unsigned long pfn, block_sz; +u64 phys_addr; + + phys_addr =be64_to_cpu (lmb->base_addr); + block_sz =memory_block_size_bytes (); + scns_per_block = block_sz /MIN_MEMORY_BLOCK_SIZE ; + +for (i =0 ; i < scns_per_block; i++) { + pfn =PFN_DOWN (phys_addr); +if (!pfn_present (pfn)) +continue ; + + rc &=is_mem_section_removable (pfn,PAGES_PER_SECTION ); + phys_addr +=MIN_MEMORY_BLOCK_SIZE ; + } + +return rc; +}
Quoted 9 lines
Quoted 2 lines
arch/powerpc/platforms/pseries/hotplug-memory.c +2
-0+static int dlpar_memory_remove (struct pseries_hp_errorlog *hp_elog) +{...
Quoted 1 line
+}
Most of the same comments as for add.
ok, I'll go through them and apply them to the remove code.
Thanks for the review.
-Nathan