* [PATCH v4 00/20] Implement GENERIC_CMDLINE
From: Christophe Leroy @ 2021-04-02 15:18 UTC (permalink / raw)
To: will, danielwa, robh, daniel, arnd, akpm
Cc: linux-arch, devicetree, microblaze, linux-xtensa, linux-sh,
linux-hexagon, x86, linux-kernel, nios2, linux-mips, linux-mm,
openrisc, sparclinux, linux-riscv, linuxppc-dev, linux-arm-kernel
The purpose of this series is to improve and enhance the
handling of kernel boot arguments.
Current situation is that most if not all architectures are using
similar options to do some manupulation on command line arguments:
- Prepend built-in arguments in front of bootloader provided arguments
- Append built-in arguments after bootloader provided arguments
- Replace bootloader provided arguments by built-in arguments
- Use built-in arguments when none is provided by bootloader.
On some architectures, all the options are possible. On other ones,
only a subset are available.
The purpose of this series is to refactor and enhance the
handling of kernel boot arguments so that every architecture can
benefit from all possibilities.
It is first focussed on powerpc but also extends the capability
for other arches.
The work has been focussed on minimising the churn in architectures
by keeping the most commonly used namings.
Main changes in V4:
- Included patch from Daniel to replace powerpc's strcpy() by strlcpy()
- Using strlcpy() instead of zeroing first char + strlcat() (idea taken frm Daniel's series)
- Reworked the convertion of EFI which was wrong in V3
- Added "too long" command line handling
- Changed cmdline macro into a function
- Done a few fixes in arch (NIOS2, SH, ARM)
- Taken comments into account (see individual responses for details)
- Tested on powerpc, build tested on ARM64, X86_64.
Main changes in V3:
- Also accept destination equal to source in cmdline_build() by setting a tmp buffer in __initdata. Powerpc provides different source and destination and call __cmdline_build() directly.
- Taken comments received from Will and Rob
- Converted all architectures (Only tested on powerpc)
Christophe Leroy (19):
cmdline: Add generic function to build command line.
drivers: of: use cmdline building function
x86/efi: Replace CONFIG_CMDLINE_OVERRIDE by CONFIG_CMDLINE_FORCE
drivers: firmware: efi: use cmdline building function
cmdline: Gives architectures opportunity to use generically defined
boot cmdline manipulation
powerpc: Convert to GENERIC_CMDLINE
arm: Convert to GENERIC_CMDLINE
arm64: Convert to GENERIC_CMDLINE
hexagon: Convert to GENERIC_CMDLINE
microblaze: Convert to GENERIC_CMDLINE
nios2: Convert to GENERIC_CMDLINE
openrisc: Convert to GENERIC_CMDLINE
riscv: Convert to GENERIC_CMDLINE
sh: Convert to GENERIC_CMDLINE
sparc: Convert to GENERIC_CMDLINE
xtensa: Convert to GENERIC_CMDLINE
x86: Convert to GENERIC_CMDLINE
mips: Convert to GENERIC_CMDLINE
cmdline: Remove CONFIG_CMDLINE_EXTEND
Daniel Walker (1):
powerpc: convert strcpy to strlcpy in prom_init
arch/arm/Kconfig | 38 +--------
arch/arm/kernel/atags_parse.c | 13 +--
arch/arm64/Kconfig | 33 +-------
arch/arm64/kernel/idreg-override.c | 9 +--
arch/hexagon/Kconfig | 11 +--
arch/hexagon/kernel/setup.c | 10 +--
arch/microblaze/Kconfig | 24 +-----
arch/microblaze/configs/mmu_defconfig | 2 +-
arch/microblaze/kernel/head.S | 4 +-
arch/mips/Kconfig | 1 +
arch/mips/Kconfig.debug | 44 -----------
arch/mips/configs/ar7_defconfig | 1 -
arch/mips/configs/bcm47xx_defconfig | 1 -
arch/mips/configs/bcm63xx_defconfig | 1 -
arch/mips/configs/bmips_be_defconfig | 1 -
arch/mips/configs/bmips_stb_defconfig | 1 -
arch/mips/configs/capcella_defconfig | 1 -
arch/mips/configs/ci20_defconfig | 1 -
arch/mips/configs/cu1000-neo_defconfig | 1 -
arch/mips/configs/cu1830-neo_defconfig | 1 -
arch/mips/configs/e55_defconfig | 1 -
arch/mips/configs/generic_defconfig | 1 -
arch/mips/configs/gpr_defconfig | 1 -
arch/mips/configs/loongson3_defconfig | 1 -
arch/mips/configs/mpc30x_defconfig | 1 -
arch/mips/configs/rt305x_defconfig | 1 -
arch/mips/configs/tb0219_defconfig | 1 -
arch/mips/configs/tb0226_defconfig | 1 -
arch/mips/configs/tb0287_defconfig | 1 -
arch/mips/configs/workpad_defconfig | 1 -
arch/mips/configs/xway_defconfig | 1 -
arch/mips/kernel/relocate.c | 4 +-
arch/mips/kernel/setup.c | 40 +---------
arch/mips/pic32/pic32mzda/early_console.c | 2 +-
arch/mips/pic32/pic32mzda/init.c | 2 -
arch/nios2/Kconfig | 25 +-----
arch/nios2/kernel/setup.c | 13 +--
arch/openrisc/Kconfig | 10 +--
arch/powerpc/Kconfig | 37 +--------
arch/powerpc/kernel/prom_init.c | 46 ++++++-----
arch/riscv/Kconfig | 44 +----------
arch/riscv/kernel/setup.c | 7 +-
arch/sh/Kconfig | 28 +------
arch/sh/configs/ap325rxa_defconfig | 2 +-
arch/sh/configs/dreamcast_defconfig | 2 +-
arch/sh/configs/ecovec24-romimage_defconfig | 2 +-
arch/sh/configs/ecovec24_defconfig | 2 +-
arch/sh/configs/edosk7760_defconfig | 2 +-
arch/sh/configs/espt_defconfig | 2 +-
arch/sh/configs/j2_defconfig | 2 +-
arch/sh/configs/kfr2r09-romimage_defconfig | 2 +-
arch/sh/configs/kfr2r09_defconfig | 2 +-
arch/sh/configs/lboxre2_defconfig | 2 +-
arch/sh/configs/microdev_defconfig | 2 +-
arch/sh/configs/migor_defconfig | 2 +-
arch/sh/configs/polaris_defconfig | 2 +-
arch/sh/configs/r7780mp_defconfig | 2 +-
arch/sh/configs/r7785rp_defconfig | 2 +-
arch/sh/configs/rsk7201_defconfig | 2 +-
arch/sh/configs/rsk7203_defconfig | 2 +-
arch/sh/configs/rts7751r2d1_defconfig | 2 +-
arch/sh/configs/rts7751r2dplus_defconfig | 2 +-
arch/sh/configs/sdk7780_defconfig | 2 +-
arch/sh/configs/sdk7786_defconfig | 2 +-
arch/sh/configs/se7206_defconfig | 2 +-
arch/sh/configs/se7343_defconfig | 2 +-
arch/sh/configs/se7712_defconfig | 2 +-
arch/sh/configs/se7721_defconfig | 2 +-
arch/sh/configs/se7724_defconfig | 2 +-
arch/sh/configs/se7751_defconfig | 2 +-
arch/sh/configs/se7780_defconfig | 2 +-
arch/sh/configs/sh03_defconfig | 2 +-
arch/sh/configs/sh2007_defconfig | 2 +-
arch/sh/configs/sh7757lcr_defconfig | 2 +-
arch/sh/configs/sh7763rdp_defconfig | 2 +-
arch/sh/configs/shmin_defconfig | 2 +-
arch/sh/configs/shx3_defconfig | 2 +-
arch/sh/configs/titan_defconfig | 2 +-
arch/sh/configs/ul2_defconfig | 2 +-
arch/sh/kernel/setup.c | 11 +--
arch/sparc/Kconfig | 18 +----
arch/sparc/prom/bootstr_64.c | 2 +-
arch/x86/Kconfig | 45 +----------
arch/x86/kernel/setup.c | 17 +---
arch/xtensa/Kconfig | 15 +---
arch/xtensa/configs/audio_kc705_defconfig | 1 -
arch/xtensa/configs/common_defconfig | 1 -
arch/xtensa/configs/generic_kc705_defconfig | 1 -
arch/xtensa/configs/iss_defconfig | 1 -
arch/xtensa/configs/nommu_kc705_defconfig | 1 -
arch/xtensa/configs/smp_lx200_defconfig | 1 -
arch/xtensa/configs/virt_defconfig | 1 -
arch/xtensa/configs/xip_kc705_defconfig | 1 -
arch/xtensa/kernel/setup.c | 10 +--
.../firmware/efi/libstub/efi-stub-helper.c | 35 ++++----
drivers/firmware/efi/libstub/efi-stub.c | 23 ++----
drivers/firmware/efi/libstub/efistub.h | 2 +-
drivers/firmware/efi/libstub/x86-stub.c | 18 +----
drivers/of/fdt.c | 23 +-----
include/linux/cmdline.h | 79 +++++++++++++++++++
init/Kconfig | 46 +++++++++++
usr/Kconfig | 2 +-
102 files changed, 265 insertions(+), 628 deletions(-)
create mode 100644 include/linux/cmdline.h
--
2.25.0
^ permalink raw reply
* [PATCH v4 02/20] drivers: of: use cmdline building function
From: Christophe Leroy @ 2021-04-02 15:18 UTC (permalink / raw)
To: will, danielwa, robh, daniel, arnd, akpm
Cc: linux-arch, devicetree, microblaze, linux-xtensa, linux-sh,
linux-hexagon, x86, linux-kernel, nios2, linux-mips, linux-mm,
openrisc, sparclinux, linux-riscv, linuxppc-dev, linux-arm-kernel
In-Reply-To: <cover.1617375802.git.christophe.leroy@csgroup.eu>
This patch uses the new cmdline building function to
concatenate the of provided cmdline with built-in parts
based on compile-time options.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
drivers/of/fdt.c | 23 ++++-------------------
1 file changed, 4 insertions(+), 19 deletions(-)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index dcc1dd96911a..7c5e9fb6039b 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -25,6 +25,7 @@
#include <linux/serial_core.h>
#include <linux/sysfs.h>
#include <linux/random.h>
+#include <linux/cmdline.h>
#include <asm/setup.h> /* for COMMAND_LINE_SIZE */
#include <asm/page.h>
@@ -1050,26 +1051,10 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
/* Retrieve command line */
p = of_get_flat_dt_prop(node, "bootargs", &l);
- if (p != NULL && l > 0)
- strlcpy(data, p, min(l, COMMAND_LINE_SIZE));
+ if (l <= 0)
+ p = NULL;
- /*
- * CONFIG_CMDLINE is meant to be a default in case nothing else
- * managed to set the command line, unless CONFIG_CMDLINE_FORCE
- * is set in which case we override whatever was found earlier.
- */
-#ifdef CONFIG_CMDLINE
-#if defined(CONFIG_CMDLINE_EXTEND)
- strlcat(data, " ", COMMAND_LINE_SIZE);
- strlcat(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
-#elif defined(CONFIG_CMDLINE_FORCE)
- strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
-#else
- /* No arguments from boot loader, use kernel's cmdl*/
- if (!((char *)data)[0])
- strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
-#endif
-#endif /* CONFIG_CMDLINE */
+ cmdline_build(data, p);
pr_debug("Command line is: %s\n", (char *)data);
--
2.25.0
^ permalink raw reply related
* Re: [PATCH v2] pseries: prevent free CPU ids to be reused on another node
From: Laurent Dufour @ 2021-04-02 14:42 UTC (permalink / raw)
To: Nathan Lynch
Cc: Srikar Dronamraju, cheloha, linux-kernel, paulus, linuxppc-dev
In-Reply-To: <87a6qgbyk6.fsf@linux.ibm.com>
Thanks Nathan for reviewing this.
Le 02/04/2021 à 15:34, Nathan Lynch a écrit :
> Hi Laurent,
>
> Laurent Dufour <ldufour@linux.ibm.com> writes:
>> When a CPU is hot added, the CPU ids are taken from the available mask from
>> the lower possible set. If that set of values was previously used for CPU
>> attached to a different node, this seems to application like if these CPUs
>> have migrated from a node to another one which is not expected in real
>> life.
>
> This seems like a problem that could affect other architectures or
> platforms? I guess as long as arch code is responsible for placing new
> CPUs in cpu_present_mask, that code will have the responsibility of
> ensuring CPU IDs' NUMA assignments remain stable.
Actually, x86 is already handling this issue in the arch code specific code, see
8f54969dc8d6 ("x86/acpi: Introduce persistent storage for cpuid <-> apicid
mapping"). I didn't check for other architectures but as CPU id allocation is in
the arch part, I believe this is up to each arch to deal with this issue.
Making the CPU id allocation common to all arch is outside the scope of this patch.
> [...]
>
>> The effect of this patch can be seen by removing and adding CPUs using the
>> Qemu monitor. In the following case, the first CPU from the node 2 is
>> removed, then the first one from the node 1 is removed too. Later, the
>> first CPU of the node 2 is added back. Without that patch, the kernel will
>> numbered these CPUs using the first CPU ids available which are the ones
>> freed when removing the second CPU of the node 0. This leads to the CPU ids
>> 16-23 to move from the node 1 to the node 2. With the patch applied, the
>> CPU ids 32-39 are used since they are the lowest free ones which have not
>> been used on another node.
>>
>> At boot time:
>> [root@vm40 ~]# numactl -H | grep cpus
>> node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
>> node 1 cpus: 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31
>> node 2 cpus: 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47
>>
>> Vanilla kernel, after the CPU hot unplug/plug operations:
>> [root@vm40 ~]# numactl -H | grep cpus
>> node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
>> node 1 cpus: 24 25 26 27 28 29 30 31
>> node 2 cpus: 16 17 18 19 20 21 22 23 40 41 42 43 44 45 46 47
>>
>> Patched kernel, after the CPU hot unplug/plug operations:
>> [root@vm40 ~]# numactl -H | grep cpus
>> node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
>> node 1 cpus: 24 25 26 27 28 29 30 31
>> node 2 cpus: 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47
>
> Good demonstration of the problem. CPUs 16-23 "move" from node 1 to
> node 2.
Thanks
>
>
>> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> index 12cbffd3c2e3..48c7943b25b0 100644
>> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> @@ -39,6 +39,8 @@
>> /* This version can't take the spinlock, because it never returns */
>> static int rtas_stop_self_token = RTAS_UNKNOWN_SERVICE;
>>
>> +static cpumask_var_t node_recorded_ids_map[MAX_NUMNODES];
>
> I guess this should have documentation that it must be
> accessed/manipulated with cpu_add_remove_lock held?
I'll add a comment before the declaration to make this clear.
>
>> +
>> static void rtas_stop_self(void)
>> {
>> static struct rtas_args args;
>> @@ -151,29 +153,61 @@ static void pseries_cpu_die(unsigned int cpu)
>> */
>> static int pseries_add_processor(struct device_node *np)
>> {
>> - unsigned int cpu;
>> + unsigned int cpu, node;
>> cpumask_var_t candidate_mask, tmp;
>> - int err = -ENOSPC, len, nthreads, i;
>> + int err = -ENOSPC, len, nthreads, i, nid;
>
> From eight local vars to ten, and the two new variables' names are
> "node" and "nid". More distinctive names would help readers.
I agree that's confusing, I'll do some cleanup.
>
>
>> const __be32 *intserv;
>> + bool force_reusing = false;
>>
>> intserv = of_get_property(np, "ibm,ppc-interrupt-server#s", &len);
>> if (!intserv)
>> return 0;
>>
>> - zalloc_cpumask_var(&candidate_mask, GFP_KERNEL);
>> - zalloc_cpumask_var(&tmp, GFP_KERNEL);
>> + alloc_cpumask_var(&candidate_mask, GFP_KERNEL);
>> + alloc_cpumask_var(&tmp, GFP_KERNEL);
>> +
>> + /*
>> + * Fetch from the DT nodes read by dlpar_configure_connector() the NUMA
>> + * node id the added CPU belongs to.
>> + */
>> + nid = of_node_to_nid(np);
>> + if (nid < 0 || !node_possible(nid))
>> + nid = first_online_node;
>>
>> nthreads = len / sizeof(u32);
>> - for (i = 0; i < nthreads; i++)
>> - cpumask_set_cpu(i, tmp);
>>
>> cpu_maps_update_begin();
>>
>> BUG_ON(!cpumask_subset(cpu_present_mask, cpu_possible_mask));
>>
>> +again:
>> + cpumask_clear(candidate_mask);
>> + cpumask_clear(tmp);
>> + for (i = 0; i < nthreads; i++)
>> + cpumask_set_cpu(i, tmp);
>> +
>> /* Get a bitmap of unoccupied slots. */
>> cpumask_xor(candidate_mask, cpu_possible_mask, cpu_present_mask);
>> +
>> + /*
>> + * Remove free ids previously assigned on the other nodes. We can walk
>> + * only online nodes because once a node became online it is not turned
>> + * offlined back.
>> + */
>> + if (!force_reusing)
>> + for_each_online_node(node) {
>> + if (node == nid) /* Keep our node's recorded ids */
>> + continue;
>> + cpumask_andnot(candidate_mask, candidate_mask,
>> + node_recorded_ids_map[node]);
>> + }
>> +
>> if (cpumask_empty(candidate_mask)) {
>> + if (!force_reusing) {
>> + force_reusing = true;
>> + goto again;
>> + }
>> +
>
> Hmm I'd encourage you to work toward a solution that doesn't involve
> adding backwards jumps and a bool flag to this code.
>
> The function already mixes concerns and this change makes it a bit more
> difficult to follow. I'd suggest that you first factor out into a
> separate function the parts that allocate a suitable range from
> cpu_possible_mask, and only then introduce the behavior change
> constraining the results.
Fair enough, I'll try to factor some part of the code and avoid a backward jumps.
>
>> /* If we get here, it most likely means that NR_CPUS is
>> * less than the partition's max processors setting.
>> */
>> @@ -191,12 +225,36 @@ static int pseries_add_processor(struct device_node *np)
>> cpumask_shift_left(tmp, tmp, nthreads);
>>
>> if (cpumask_empty(tmp)) {
>> + if (!force_reusing) {
>> + force_reusing = true;
>> + goto again;
>> + }
>> printk(KERN_ERR "Unable to find space in cpu_present_mask for"
>> " processor %pOFn with %d thread(s)\n", np,
>> nthreads);
>> goto out_unlock;
>> }
>>
>> + /* Record the newly used CPU ids for the associate node. */
>> + cpumask_or(node_recorded_ids_map[nid], node_recorded_ids_map[nid], tmp);
>> +
>> + /*
>> + * If we force reusing the id, remove these ids from any node which was
>> + * previously using it.
>> + */
>> + if (force_reusing) {
>> + cpu = cpumask_first(tmp);
>> + pr_warn("Reusing free CPU ids %d-%d from another node\n",
>> + cpu, cpu + nthreads - 1);
>> +
>> + for_each_online_node(node) {
>> + if (node == nid)
>> + continue;
>> + cpumask_andnot(node_recorded_ids_map[node],
>> + node_recorded_ids_map[node], tmp);
>> + }
>> + }
>> +
>
> I don't know, should we not fail the request instead of doing the
> ABI-breaking thing the code in this change is trying to prevent? I don't
> think a warning in the kernel log is going to help any application that
> would be affected by this.
That's a really good question. One should argue that the most important is to
satisfy the CPU add operation, assuming that only few are interested in the CPU
numbering, while others would prefer the CPU adding to fail (which may prevent
adding CPUs on another nodes if the whole operation is aborted as soon as a CPU
add is failing).
I was conservative here, but if failing the operation is the best option, then
this will make that code simpler, removing the backward jump.
Who is deciding?
>
>
>> for_each_cpu(cpu, tmp) {
>> BUG_ON(cpu_present(cpu));
>> set_cpu_present(cpu, true);
>> @@ -889,6 +947,7 @@ static struct notifier_block pseries_smp_nb = {
>> static int __init pseries_cpu_hotplug_init(void)
>> {
>> int qcss_tok;
>> + unsigned int node;
>>
>> #ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
>> ppc_md.cpu_probe = dlpar_cpu_probe;
>> @@ -910,8 +969,18 @@ static int __init pseries_cpu_hotplug_init(void)
>> smp_ops->cpu_die = pseries_cpu_die;
>>
>> /* Processors can be added/removed only on LPAR */
>> - if (firmware_has_feature(FW_FEATURE_LPAR))
>> + if (firmware_has_feature(FW_FEATURE_LPAR)) {
>> + for_each_node(node) {
>> + alloc_bootmem_cpumask_var(&node_recorded_ids_map[node]);
>> +
>> + /* Record ids of CPU added at boot time */
>> + cpumask_or(node_recorded_ids_map[node],
>> + node_recorded_ids_map[node],
>> + node_to_cpumask_map[node]);
>> + }
>> +
>> of_reconfig_notifier_register(&pseries_smp_nb);
>> + }
>
> This looks OK.
>
^ permalink raw reply
* Re: [PATCH v2] pseries: prevent free CPU ids to be reused on another node
From: Nathan Lynch @ 2021-04-02 13:34 UTC (permalink / raw)
To: Laurent Dufour
Cc: Srikar Dronamraju, cheloha, linux-kernel, paulus, linuxppc-dev
In-Reply-To: <20210325093512.57856-1-ldufour@linux.ibm.com>
Hi Laurent,
Laurent Dufour <ldufour@linux.ibm.com> writes:
> When a CPU is hot added, the CPU ids are taken from the available mask from
> the lower possible set. If that set of values was previously used for CPU
> attached to a different node, this seems to application like if these CPUs
> have migrated from a node to another one which is not expected in real
> life.
This seems like a problem that could affect other architectures or
platforms? I guess as long as arch code is responsible for placing new
CPUs in cpu_present_mask, that code will have the responsibility of
ensuring CPU IDs' NUMA assignments remain stable.
[...]
> The effect of this patch can be seen by removing and adding CPUs using the
> Qemu monitor. In the following case, the first CPU from the node 2 is
> removed, then the first one from the node 1 is removed too. Later, the
> first CPU of the node 2 is added back. Without that patch, the kernel will
> numbered these CPUs using the first CPU ids available which are the ones
> freed when removing the second CPU of the node 0. This leads to the CPU ids
> 16-23 to move from the node 1 to the node 2. With the patch applied, the
> CPU ids 32-39 are used since they are the lowest free ones which have not
> been used on another node.
>
> At boot time:
> [root@vm40 ~]# numactl -H | grep cpus
> node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
> node 1 cpus: 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31
> node 2 cpus: 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47
>
> Vanilla kernel, after the CPU hot unplug/plug operations:
> [root@vm40 ~]# numactl -H | grep cpus
> node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
> node 1 cpus: 24 25 26 27 28 29 30 31
> node 2 cpus: 16 17 18 19 20 21 22 23 40 41 42 43 44 45 46 47
>
> Patched kernel, after the CPU hot unplug/plug operations:
> [root@vm40 ~]# numactl -H | grep cpus
> node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
> node 1 cpus: 24 25 26 27 28 29 30 31
> node 2 cpus: 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47
Good demonstration of the problem. CPUs 16-23 "move" from node 1 to
node 2.
> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> index 12cbffd3c2e3..48c7943b25b0 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> @@ -39,6 +39,8 @@
> /* This version can't take the spinlock, because it never returns */
> static int rtas_stop_self_token = RTAS_UNKNOWN_SERVICE;
>
> +static cpumask_var_t node_recorded_ids_map[MAX_NUMNODES];
I guess this should have documentation that it must be
accessed/manipulated with cpu_add_remove_lock held?
> +
> static void rtas_stop_self(void)
> {
> static struct rtas_args args;
> @@ -151,29 +153,61 @@ static void pseries_cpu_die(unsigned int cpu)
> */
> static int pseries_add_processor(struct device_node *np)
> {
> - unsigned int cpu;
> + unsigned int cpu, node;
> cpumask_var_t candidate_mask, tmp;
> - int err = -ENOSPC, len, nthreads, i;
> + int err = -ENOSPC, len, nthreads, i, nid;
From eight local vars to ten, and the two new variables' names are
"node" and "nid". More distinctive names would help readers.
> const __be32 *intserv;
> + bool force_reusing = false;
>
> intserv = of_get_property(np, "ibm,ppc-interrupt-server#s", &len);
> if (!intserv)
> return 0;
>
> - zalloc_cpumask_var(&candidate_mask, GFP_KERNEL);
> - zalloc_cpumask_var(&tmp, GFP_KERNEL);
> + alloc_cpumask_var(&candidate_mask, GFP_KERNEL);
> + alloc_cpumask_var(&tmp, GFP_KERNEL);
> +
> + /*
> + * Fetch from the DT nodes read by dlpar_configure_connector() the NUMA
> + * node id the added CPU belongs to.
> + */
> + nid = of_node_to_nid(np);
> + if (nid < 0 || !node_possible(nid))
> + nid = first_online_node;
>
> nthreads = len / sizeof(u32);
> - for (i = 0; i < nthreads; i++)
> - cpumask_set_cpu(i, tmp);
>
> cpu_maps_update_begin();
>
> BUG_ON(!cpumask_subset(cpu_present_mask, cpu_possible_mask));
>
> +again:
> + cpumask_clear(candidate_mask);
> + cpumask_clear(tmp);
> + for (i = 0; i < nthreads; i++)
> + cpumask_set_cpu(i, tmp);
> +
> /* Get a bitmap of unoccupied slots. */
> cpumask_xor(candidate_mask, cpu_possible_mask, cpu_present_mask);
> +
> + /*
> + * Remove free ids previously assigned on the other nodes. We can walk
> + * only online nodes because once a node became online it is not turned
> + * offlined back.
> + */
> + if (!force_reusing)
> + for_each_online_node(node) {
> + if (node == nid) /* Keep our node's recorded ids */
> + continue;
> + cpumask_andnot(candidate_mask, candidate_mask,
> + node_recorded_ids_map[node]);
> + }
> +
> if (cpumask_empty(candidate_mask)) {
> + if (!force_reusing) {
> + force_reusing = true;
> + goto again;
> + }
> +
Hmm I'd encourage you to work toward a solution that doesn't involve
adding backwards jumps and a bool flag to this code.
The function already mixes concerns and this change makes it a bit more
difficult to follow. I'd suggest that you first factor out into a
separate function the parts that allocate a suitable range from
cpu_possible_mask, and only then introduce the behavior change
constraining the results.
> /* If we get here, it most likely means that NR_CPUS is
> * less than the partition's max processors setting.
> */
> @@ -191,12 +225,36 @@ static int pseries_add_processor(struct device_node *np)
> cpumask_shift_left(tmp, tmp, nthreads);
>
> if (cpumask_empty(tmp)) {
> + if (!force_reusing) {
> + force_reusing = true;
> + goto again;
> + }
> printk(KERN_ERR "Unable to find space in cpu_present_mask for"
> " processor %pOFn with %d thread(s)\n", np,
> nthreads);
> goto out_unlock;
> }
>
> + /* Record the newly used CPU ids for the associate node. */
> + cpumask_or(node_recorded_ids_map[nid], node_recorded_ids_map[nid], tmp);
> +
> + /*
> + * If we force reusing the id, remove these ids from any node which was
> + * previously using it.
> + */
> + if (force_reusing) {
> + cpu = cpumask_first(tmp);
> + pr_warn("Reusing free CPU ids %d-%d from another node\n",
> + cpu, cpu + nthreads - 1);
> +
> + for_each_online_node(node) {
> + if (node == nid)
> + continue;
> + cpumask_andnot(node_recorded_ids_map[node],
> + node_recorded_ids_map[node], tmp);
> + }
> + }
> +
I don't know, should we not fail the request instead of doing the
ABI-breaking thing the code in this change is trying to prevent? I don't
think a warning in the kernel log is going to help any application that
would be affected by this.
> for_each_cpu(cpu, tmp) {
> BUG_ON(cpu_present(cpu));
> set_cpu_present(cpu, true);
> @@ -889,6 +947,7 @@ static struct notifier_block pseries_smp_nb = {
> static int __init pseries_cpu_hotplug_init(void)
> {
> int qcss_tok;
> + unsigned int node;
>
> #ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
> ppc_md.cpu_probe = dlpar_cpu_probe;
> @@ -910,8 +969,18 @@ static int __init pseries_cpu_hotplug_init(void)
> smp_ops->cpu_die = pseries_cpu_die;
>
> /* Processors can be added/removed only on LPAR */
> - if (firmware_has_feature(FW_FEATURE_LPAR))
> + if (firmware_has_feature(FW_FEATURE_LPAR)) {
> + for_each_node(node) {
> + alloc_bootmem_cpumask_var(&node_recorded_ids_map[node]);
> +
> + /* Record ids of CPU added at boot time */
> + cpumask_or(node_recorded_ids_map[node],
> + node_recorded_ids_map[node],
> + node_to_cpumask_map[node]);
> + }
> +
> of_reconfig_notifier_register(&pseries_smp_nb);
> + }
This looks OK.
^ permalink raw reply
* Re: [PATCH v3 8/9] powerpc/xive: Map one IPI interrupt per node
From: Cédric Le Goater @ 2021-04-02 12:19 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev; +Cc: Thomas Gleixner, Greg Kurz
In-Reply-To: <ae512e1b-91ee-1cc8-c6a8-36e014ed8788@kaod.org>
> I gave the patch below a try and we are reaching the same results,
> even better. The simplest solution is always the best. Nick, you
> should send that single patch.
FYI, here are results in a KVM guests with pinned vCPUs.
* P9 DD2.2 - 2s * 64 threads - KVM guest :
IPI/sys IPI/chip
----------- -------------------------------- --------------------------------
unhandled unhandled
chips cpus noirqdebug detection noirqdebug detection
----------- -------------------------------- --------------------------------
1 0-15 4.152813 4.084240 4.061028 4.097700 4.042539 4.008314
0-31 8.186328 8.157970 7.937127 8.277942 8.019539 7.831189
0-47 11.391635 11.232960 11.017530 11.278048 10.994501 10.889347
0-63 13.907476 14.022307 11.460280 13.933946 13.506828 11.369188
2 0-79 18.105276 18.084463 8.376047 18.069176 17.587916 15.477006
0-95 22.100683 22.265763 7.338229 22.084006 21.588463 19.502192
0-111 25.305948 25.473068 6.716235 25.429261 24.607570 22.733253
0-127 27.814449 28.029029 6.222736 27.960119 27.253432 23.884916
The three columns "IPI/chip" are results with this series. "IPI/sys" are
without. The "unhandled detection" columns are with Nick's patch.
C.
^ permalink raw reply
* Re: [PATCH v3 8/9] powerpc/xive: Map one IPI interrupt per node
From: Cédric Le Goater @ 2021-04-02 11:31 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev; +Cc: Thomas Gleixner, Greg Kurz
In-Reply-To: <1617279896.hl1bn8g3eg.astroid@bobo.none>
On 4/1/21 2:50 PM, Nicholas Piggin wrote:
> Excerpts from Cédric Le Goater's message of April 1, 2021 12:45 am:
>> ipistorm [*] can be used to benchmark the raw interrupt rate of an
>> interrupt controller by measuring the number of IPIs a system can
>> sustain. When applied to the XIVE interrupt controller of POWER9 and
>> POWER10 systems, a significant drop of the interrupt rate can be
>> observed when crossing the second node boundary.
>>
>> This is due to the fact that a single IPI interrupt is used for all
>> CPUs of the system. The structure is shared and the cache line updates
>> impact greatly the traffic between nodes and the overall IPI
>> performance.
>>
>> As a workaround, the impact can be reduced by deactivating the IRQ
>> lockup detector ("noirqdebug") which does a lot of accounting in the
>> Linux IRQ descriptor structure and is responsible for most of the
>> performance penalty.
>>
>> As a fix, this proposal allocates an IPI interrupt per node, to be
>> shared by all CPUs of that node. It solves the scaling issue, the IRQ
>> lockup detector still has an impact but the XIVE interrupt rate scales
>> linearly. It also improves the "noirqdebug" case as showed in the
>> tables below.
>>
>> * P9 DD2.2 - 2s * 64 threads
>>
>> "noirqdebug"
>> Mint/s Mint/s
>> chips cpus IPI/sys IPI/chip IPI/chip IPI/sys
>> --------------------------------------------------------------
>> 1 0-15 4.984023 4.875405 4.996536 5.048892
>> 0-31 10.879164 10.544040 10.757632 11.037859
>> 0-47 15.345301 14.688764 14.926520 15.310053
>> 0-63 17.064907 17.066812 17.613416 17.874511
>> 2 0-79 11.768764 21.650749 22.689120 22.566508
>> 0-95 10.616812 26.878789 28.434703 28.320324
>> 0-111 10.151693 31.397803 31.771773 32.388122
>> 0-127 9.948502 33.139336 34.875716 35.224548
>>
>> * P10 DD1 - 4s (not homogeneous) 352 threads
>>
>> "noirqdebug"
>> Mint/s Mint/s
>> chips cpus IPI/sys IPI/chip IPI/chip IPI/sys
>> --------------------------------------------------------------
>> 1 0-15 2.409402 2.364108 2.383303 2.395091
>> 0-31 6.028325 6.046075 6.089999 6.073750
>> 0-47 8.655178 8.644531 8.712830 8.724702
>> 0-63 11.629652 11.735953 12.088203 12.055979
>> 0-79 14.392321 14.729959 14.986701 14.973073
>> 0-95 12.604158 13.004034 17.528748 17.568095
>> 2 0-111 9.767753 13.719831 19.968606 20.024218
>> 0-127 6.744566 16.418854 22.898066 22.995110
>> 0-143 6.005699 19.174421 25.425622 25.417541
>> 0-159 5.649719 21.938836 27.952662 28.059603
>> 0-175 5.441410 24.109484 31.133915 31.127996
>> 3 0-191 5.318341 24.405322 33.999221 33.775354
>> 0-207 5.191382 26.449769 36.050161 35.867307
>> 0-223 5.102790 29.356943 39.544135 39.508169
>> 0-239 5.035295 31.933051 42.135075 42.071975
>> 0-255 4.969209 34.477367 44.655395 44.757074
>> 4 0-271 4.907652 35.887016 47.080545 47.318537
>> 0-287 4.839581 38.076137 50.464307 50.636219
>> 0-303 4.786031 40.881319 53.478684 53.310759
>> 0-319 4.743750 43.448424 56.388102 55.973969
>> 0-335 4.709936 45.623532 59.400930 58.926857
>> 0-351 4.681413 45.646151 62.035804 61.830057
>>
>> [*] https://github.com/antonblanchard/ipistorm
>>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>
> Very nice result but the default-on irqdebug code is quite a slowdown
> even with your improvements.
>
> Is the main cacheline bouncing in the fast path coming from
> desc->irq_count++ of the percpu handler? Can we do something quick and
> dirty like the attached patch?
>
> All this stuff seems totally racy with percpu handler but maybe that
> doesn't matter too much (and anyway it would be a much bigger change)
I gave the patch below a try and we are reaching the same results,
even better. The simplest solution is always the best. Nick, you
should send that single patch.
Thanks,
C.
> Thanks,
> Nick
>
> ---
> kernel/irq/spurious.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/irq/spurious.c b/kernel/irq/spurious.c
> index f865e5f4d382..6b17b737ee6c 100644
> --- a/kernel/irq/spurious.c
> +++ b/kernel/irq/spurious.c
> @@ -378,7 +378,8 @@ void note_interrupt(struct irq_desc *desc, irqreturn_t action_ret)
> * then we merily delay the spurious detection
> * by one hard interrupt. Not a real problem.
> */
> - desc->threads_handled_last &= ~SPURIOUS_DEFERRED;
> + if (desc->threads_handled_last & SPURIOUS_DEFERRED)
> + desc->threads_handled_last &= ~SPURIOUS_DEFERRED;
> }
> }
>
> @@ -403,6 +404,10 @@ void note_interrupt(struct irq_desc *desc, irqreturn_t action_ret)
> desc->irqs_unhandled -= ok;
> }
>
> + if (likely(!desc->irqs_unhandled))
> + return;
> +
> + /* Now getting into unhandled irq detection */
> desc->irq_count++;
> if (likely(desc->irq_count < 100000))
> return;
>
^ permalink raw reply
* Re: [PATCH v5 29/48] powerpc: add set_dec_or_work API for safely updating decrementer
From: Nicholas Piggin @ 2021-04-02 11:04 UTC (permalink / raw)
To: kvm-ppc; +Cc: Alexey Kardashevskiy, linuxppc-dev
In-Reply-To: <20210401150325.442125-30-npiggin@gmail.com>
Excerpts from Nicholas Piggin's message of April 2, 2021 1:03 am:
> Decrementer updates must always check for new irq work to avoid an
> irq work decrementer interrupt being lost.
>
> Add an API for this in the timer code so callers don't have to care
> about details.
Oh I forgot to update the changelog for this, it's significantly
changed, so I better withdraw the Reviewed-by as well. I think this
re-arm API is the better one, there's no reason it can't all be in
the host time.c code.
This implementation also avoids what used to be inevitable double
interrupt to take a host timer from guest (first hdec to get into
the host and set dec to some -ve value, then taking that dec as
soon as we enable interrupts) by just marking the dec pending in
that case, so it gets replayed when we enable irqs.
Thanks,
Nick
>
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/include/asm/time.h | 4 ++++
> arch/powerpc/kernel/time.c | 41 +++++++++++++++++++++++++--------
> arch/powerpc/kvm/book3s_hv.c | 6 +----
> 3 files changed, 37 insertions(+), 14 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/time.h b/arch/powerpc/include/asm/time.h
> index 0128cd9769bc..924b2157882f 100644
> --- a/arch/powerpc/include/asm/time.h
> +++ b/arch/powerpc/include/asm/time.h
> @@ -106,6 +106,10 @@ static inline u64 timer_get_next_tb(void)
> return __this_cpu_read(decrementers_next_tb);
> }
>
> +#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> +void timer_rearm_host_dec(u64 now);
> +#endif
> +
> /* Convert timebase ticks to nanoseconds */
> unsigned long long tb_to_ns(unsigned long long tb_ticks);
>
> diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
> index 8b9b38a8ce57..8bbcc6be40c0 100644
> --- a/arch/powerpc/kernel/time.c
> +++ b/arch/powerpc/kernel/time.c
> @@ -563,13 +563,43 @@ void arch_irq_work_raise(void)
> preempt_enable();
> }
>
> +static void set_dec_or_work(u64 val)
> +{
> + set_dec(val);
> + /* We may have raced with new irq work */
> + if (unlikely(test_irq_work_pending()))
> + set_dec(1);
> +}
> +
> #else /* CONFIG_IRQ_WORK */
>
> #define test_irq_work_pending() 0
> #define clear_irq_work_pending()
>
> +static void set_dec_or_work(u64 val)
> +{
> + set_dec(val);
> +}
> #endif /* CONFIG_IRQ_WORK */
>
> +#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> +void timer_rearm_host_dec(u64 now)
> +{
> + u64 *next_tb = this_cpu_ptr(&decrementers_next_tb);
> +
> + WARN_ON_ONCE(!arch_irqs_disabled());
> + WARN_ON_ONCE(mfmsr() & MSR_EE);
> +
> + if (now >= *next_tb) {
> + now = *next_tb - now;
> + set_dec_or_work(now);
> + } else {
> + local_paca->irq_happened |= PACA_IRQ_DEC;
> + }
> +}
> +EXPORT_SYMBOL_GPL(timer_rearm_host_dec);
> +#endif
> +
> /*
> * timer_interrupt - gets called when the decrementer overflows,
> * with interrupts disabled.
> @@ -630,10 +660,7 @@ DEFINE_INTERRUPT_HANDLER_ASYNC(timer_interrupt)
> } else {
> now = *next_tb - now;
> if (now <= decrementer_max)
> - set_dec(now);
> - /* We may have raced with new irq work */
> - if (test_irq_work_pending())
> - set_dec(1);
> + set_dec_or_work(now);
> __this_cpu_inc(irq_stat.timer_irqs_others);
> }
>
> @@ -875,11 +902,7 @@ static int decrementer_set_next_event(unsigned long evt,
> struct clock_event_device *dev)
> {
> __this_cpu_write(decrementers_next_tb, get_tb() + evt);
> - set_dec(evt);
> -
> - /* We may have raced with new irq work */
> - if (test_irq_work_pending())
> - set_dec(1);
> + set_dec_or_work(evt);
>
> return 0;
> }
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 8c8df88eec8c..287042b4afb5 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -3901,11 +3901,7 @@ static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
> vc->entry_exit_map = 0x101;
> vc->in_guest = 0;
>
> - next_timer = timer_get_next_tb();
> - set_dec(next_timer - tb);
> - /* We may have raced with new irq work */
> - if (test_irq_work_pending())
> - set_dec(1);
> + timer_rearm_host_dec(tb);
> mtspr(SPRN_SPRG_VDSO_WRITE, local_paca->sprg_vdso);
>
> kvmhv_load_host_pmu();
> --
> 2.23.0
>
>
^ permalink raw reply
* Re: [PATCH] powerpc/powernv: Enable HAIL (HV AIL) for ISA v3.1 processors
From: Nicholas Piggin @ 2021-04-02 8:00 UTC (permalink / raw)
To: Cédric Le Goater, linuxppc-dev
In-Reply-To: <0f41c067-7b9f-5b98-0993-c81cbaeea265@kaod.org>
Excerpts from Cédric Le Goater's message of April 2, 2021 4:10 pm:
> On 4/2/21 4:41 AM, Nicholas Piggin wrote:
>> Starting with ISA v3.1, LPCR[AIL] no longer controls the interrupt
>> mode for HV=1 interrupts. Instead, a new LPCR[HAIL] bit is defined
>> which behaves like AIL=3 for HV interrupts when set.
>
> Will QEMU need an update ?
Yes you're right it will, we need to do that.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v4 29/46] KVM: PPC: Book3S HV P9: Implement the rest of the P9 path in C
From: Nicholas Piggin @ 2021-04-02 7:58 UTC (permalink / raw)
To: Alexey Kardashevskiy, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <e2fc29aa-f38c-4650-06e2-d918c59547bf@ozlabs.ru>
Excerpts from Alexey Kardashevskiy's message of April 2, 2021 2:36 pm:
>
>
> On 01/04/2021 21:35, Nicholas Piggin wrote:
>> Excerpts from Alexey Kardashevskiy's message of April 1, 2021 3:30 pm:
>>>
>>>
>>> On 3/23/21 12:02 PM, Nicholas Piggin wrote:
>>>> Almost all logic is moved to C, by introducing a new in_guest mode that
>>>> selects and branches very early in the interrupt handler to the P9 exit
>>>> code.
>>
>> [...]
>>
>>>> +/*
>>>> + * kvmppc_p9_exit_hcall and kvmppc_p9_exit_interrupt are branched to from
>>>> + * above if the interrupt was taken for a guest that was entered via
>>>> + * kvmppc_p9_enter_guest().
>>>> + *
>>>> + * This code recovers the host stack and vcpu pointer, saves all GPRs and
>>>> + * CR, LR, CTR, XER as well as guest MSR and NIA into the VCPU, then re-
>>>> + * establishes the host stack and registers to return from the
>>>> + * kvmppc_p9_enter_guest() function.
>>>
>>> What does "this code" refer to? If it is the asm below, then it does not
>>> save CTR, it is in the c code. Otherwise it is confusing (to me) :)
>>
>> Yes you're right, CTR is saved in C.
>>
>>>> + */
>>>> +.balign IFETCH_ALIGN_BYTES
>>>> +kvmppc_p9_exit_hcall:
>>>> + mfspr r11,SPRN_SRR0
>>>> + mfspr r12,SPRN_SRR1
>>>> + li r10,0xc00
>>>> + std r10,HSTATE_SCRATCH0(r13)
>>>> +
>>>> +.balign IFETCH_ALIGN_BYTES
>>>> +kvmppc_p9_exit_interrupt:
>>
>> [...]
>>
>>>> +static inline void slb_invalidate(unsigned int ih)
>>>> +{
>>>> + asm volatile("slbia %0" :: "i"(ih));
>>>> +}
>>>
>>> This one is not used.
>>
>> It gets used in a later patch, I guess I should move it there.
>>
>> [...]
>>
>>>> +int __kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu)
>>>> +{
>>>> + u64 *exsave;
>>>> + unsigned long msr = mfmsr();
>>>> + int trap;
>>>> +
>>>> + start_timing(vcpu, &vcpu->arch.rm_entry);
>>>> +
>>>> + vcpu->arch.ceded = 0;
>>>> +
>>>> + WARN_ON_ONCE(vcpu->arch.shregs.msr & MSR_HV);
>>>> + WARN_ON_ONCE(!(vcpu->arch.shregs.msr & MSR_ME));
>>>> +
>>>> + mtspr(SPRN_HSRR0, vcpu->arch.regs.nip);
>>>> + mtspr(SPRN_HSRR1, (vcpu->arch.shregs.msr & ~MSR_HV) | MSR_ME);
>>>> +
>>>> + /*
>>>> + * On POWER9 DD2.1 and below, sometimes on a Hypervisor Data Storage
>>>> + * Interrupt (HDSI) the HDSISR is not be updated at all.
>>>> + *
>>>> + * To work around this we put a canary value into the HDSISR before
>>>> + * returning to a guest and then check for this canary when we take a
>>>> + * HDSI. If we find the canary on a HDSI, we know the hardware didn't
>>>> + * update the HDSISR. In this case we return to the guest to retake the
>>>> + * HDSI which should correctly update the HDSISR the second time HDSI
>>>> + * entry.
>>>> + *
>>>> + * Just do this on all p9 processors for now.
>>>> + */
>>>> + mtspr(SPRN_HDSISR, HDSISR_CANARY);
>>>> +
>>>> + accumulate_time(vcpu, &vcpu->arch.guest_time);
>>>> +
>>>> + local_paca->kvm_hstate.in_guest = KVM_GUEST_MODE_GUEST_HV_FAST;
>>>> + kvmppc_p9_enter_guest(vcpu);
>>>> + // Radix host and guest means host never runs with guest MMU state
>>>> + local_paca->kvm_hstate.in_guest = KVM_GUEST_MODE_NONE;
>>>> +
>>>> + accumulate_time(vcpu, &vcpu->arch.rm_intr);
>>>> +
>>>> + /* Get these from r11/12 and paca exsave */
>>>> + vcpu->arch.shregs.srr0 = mfspr(SPRN_SRR0);
>>>> + vcpu->arch.shregs.srr1 = mfspr(SPRN_SRR1);
>>>> + vcpu->arch.shregs.dar = mfspr(SPRN_DAR);
>>>> + vcpu->arch.shregs.dsisr = mfspr(SPRN_DSISR);
>>>> +
>>>> + /* 0x2 bit for HSRR is only used by PR and P7/8 HV paths, clear it */
>>>> + trap = local_paca->kvm_hstate.scratch0 & ~0x2;
>>>> + if (likely(trap > BOOK3S_INTERRUPT_MACHINE_CHECK)) {
>>>> + exsave = local_paca->exgen;
>>>> + } else if (trap == BOOK3S_INTERRUPT_SYSTEM_RESET) {
>>>> + exsave = local_paca->exnmi;
>>>> + } else { /* trap == 0x200 */
>>>> + exsave = local_paca->exmc;
>>>> + }
>>>> +
>>>> + vcpu->arch.regs.gpr[1] = local_paca->kvm_hstate.scratch1;
>>>> + vcpu->arch.regs.gpr[3] = local_paca->kvm_hstate.scratch2;
>>>> + vcpu->arch.regs.gpr[9] = exsave[EX_R9/sizeof(u64)];
>>>> + vcpu->arch.regs.gpr[10] = exsave[EX_R10/sizeof(u64)];
>>>> + vcpu->arch.regs.gpr[11] = exsave[EX_R11/sizeof(u64)];
>>>> + vcpu->arch.regs.gpr[12] = exsave[EX_R12/sizeof(u64)];
>>>> + vcpu->arch.regs.gpr[13] = exsave[EX_R13/sizeof(u64)];
>>>> + vcpu->arch.ppr = exsave[EX_PPR/sizeof(u64)];
>>>> + vcpu->arch.cfar = exsave[EX_CFAR/sizeof(u64)];
>>>> + vcpu->arch.regs.ctr = exsave[EX_CTR/sizeof(u64)];
>>>> +
>>>> + vcpu->arch.last_inst = KVM_INST_FETCH_FAILED;
>>>> +
>>>> + if (unlikely(trap == BOOK3S_INTERRUPT_MACHINE_CHECK)) {
>>>> + vcpu->arch.fault_dar = exsave[EX_DAR/sizeof(u64)];
>>>> + vcpu->arch.fault_dsisr = exsave[EX_DSISR/sizeof(u64)];
>>>> + kvmppc_realmode_machine_check(vcpu);
>>>> +
>>>> + } else if (unlikely(trap == BOOK3S_INTERRUPT_HMI)) {
>>>> + kvmppc_realmode_hmi_handler();
>>>> +
>>>> + } else if (trap == BOOK3S_INTERRUPT_H_EMUL_ASSIST) {
>>>> + vcpu->arch.emul_inst = mfspr(SPRN_HEIR);
>>>> +
>>>> + } else if (trap == BOOK3S_INTERRUPT_H_DATA_STORAGE) {
>>>> + vcpu->arch.fault_dar = exsave[EX_DAR/sizeof(u64)];
>>>> + vcpu->arch.fault_dsisr = exsave[EX_DSISR/sizeof(u64)];
>>>> + vcpu->arch.fault_gpa = mfspr(SPRN_ASDR);
>>>> +
>>>> + } else if (trap == BOOK3S_INTERRUPT_H_INST_STORAGE) {
>>>> + vcpu->arch.fault_gpa = mfspr(SPRN_ASDR);
>>>> +
>>>> + } else if (trap == BOOK3S_INTERRUPT_H_FAC_UNAVAIL) {
>>>> + vcpu->arch.hfscr = mfspr(SPRN_HFSCR);
>>>> +
>>>> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>>>> + /*
>>>> + * Softpatch interrupt for transactional memory emulation cases
>>>> + * on POWER9 DD2.2. This is early in the guest exit path - we
>>>> + * haven't saved registers or done a treclaim yet.
>>>> + */
>>>> + } else if (trap == BOOK3S_INTERRUPT_HV_SOFTPATCH) {
>>>> + vcpu->arch.emul_inst = mfspr(SPRN_HEIR);
>>>> +
>>>> + /*
>>>> + * The cases we want to handle here are those where the guest
>>>> + * is in real suspend mode and is trying to transition to
>>>> + * transactional mode.
>>>> + */
>>>> + if (local_paca->kvm_hstate.fake_suspend &&
>>>> + (vcpu->arch.shregs.msr & MSR_TS_S)) {
>>>> + if (kvmhv_p9_tm_emulation_early(vcpu)) {
>>>> + /* Prevent it being handled again. */
>>>> + trap = 0;
>>>> + }
>>>> + }
>>>> +#endif
>>>> + }
>>>> +
>>>> + radix_clear_slb();
>>>> +
>>>> + __mtmsrd(msr, 0);
>>>
>>>
>>> The asm code only sets RI but this potentially sets more bits including
>>> MSR_EE, is it expected to be 0 when __kvmhv_vcpu_entry_p9() is called?
>>
>> Yes.
>>
>>>> + mtspr(SPRN_CTRLT, 1);
>>>
>>> What is this for? ISA does not shed much light:
>>> ===
>>> 63 RUN This bit controls an external I/O pin.
>>> ===
>>
>> I don't think it even does that these days. It interacts with the PMU.
>> I was looking whether it's feasible to move it into PMU code entirely,
>> but apparently some tool or something might sample it. I'm a bit
>> suspicious about that because an untrusted guest could be running and
>> claim not to so I don't know what said tool really achieves, but I'll
>> go through that fight another day.
>>
>> But KVM has to set it to 1 at exit because Linux host has it set to 1
>> except in CPU idle.
>
>
> It this CTRLT setting a new thing or the asm does it too? I could not
> spot it.
It's quite old actually. Earlier processors (maybe POWER6) you had to
even read-modify-write but new ones you can just store 1:
Guest exit:
/* Save guest CTRL register, set runlatch to 1 */
mfspr r6,SPRN_CTRLF
stw r6,VCPU_CTRL(r9)
andi. r0,r6,1
bne 4f
ori r6,r6,1
mtspr SPRN_CTRLT,r6
4:
entry:
/* Restore state of CTRL run bit; assume 1 on entry */
lwz r5,VCPU_CTRL(r4)
andi. r5,r5,1
bne 4f
mfspr r6,SPRN_CTRLF
clrrdi r6,r6,1
mtspr SPRN_CTRLT,r6
It used to light an indicator on the front of the system once upon a
time and I think on some processors (Cell maybe?) it actually controlled
SMT threads in some way. But certainly in P9 it does almost nothing and
we'll probably try to phase it out.
>>> The asm does "For hash guest, read the guest SLB and save it away", this
>>> code does not. Is this new fast-path-in-c only for radix-on-radix or
>>> hash VMs are supported too?
>>
>> That asm code does not run for "guest_exit_short_path" case (aka the
>> p9 path aka the fast path).
>>
>> Upstream code only supports radix host and radix guest in this path.
>> The old path supports hash and radix. That's unchanged with this patch.
>>
>> After the series, the new path supports all P9 modes (hash/hash,
>> radix/radix, and radix/hash), and the old path supports P7 and P8 only.
>
>
> Thanks for clarification. Besides that CTRLT, I checked if the new c
> code matches the old asm code (which made diving into ISA incredible fun
> :) ) so fwiw
>
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Thanks for reviewing.
> I'd really like to see longer commit logs clarifying all intended
> changes but it is probably just me.
I'm not sure what the best balance is, at some point code is a more
precise description. For this particular patch I probably do need to go
over the changelog again and try to make sure it makes sense and covers
things. If you have specifics that are missing or changes you'd like
I would definitely consider them.
Thanks,
Nick
^ permalink raw reply
* Re: [RFC/PATCH] powerpc/smp: Add SD_SHARE_PKG_RESOURCES flag to MC sched-domain
From: Gautham R Shenoy @ 2021-04-02 7:36 UTC (permalink / raw)
To: Gautham R. Shenoy
Cc: Parth Shah, Michael Neuling, Vaidyanathan Srinivasan,
Vincent Guittot, Srikar Dronamraju, Rik van Riel, Peter Zijlstra,
linuxppc-dev, LKML, Nicholas Piggin, Dietmar Eggemann, Mel Gorman,
Valentin Schneider
In-Reply-To: <1617341874-1205-1-git-send-email-ego@linux.vnet.ibm.com>
(Missed cc'ing Cc Peter in the original posting)
On Fri, Apr 02, 2021 at 11:07:54AM +0530, Gautham R. Shenoy wrote:
> From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
>
> On POWER10 systems, the L2 cache is at the SMT4 small core level. The
> following commits ensure that L2 cache gets correctly discovered and
> the Last-Level-Cache domain (LLC) is set to the SMT sched-domain.
>
> 790a166 powerpc/smp: Parse ibm,thread-groups with multiple properties
> 1fdc1d6 powerpc/smp: Rename cpu_l1_cache_map as thread_group_l1_cache_map
> fbd2b67 powerpc/smp: Rename init_thread_group_l1_cache_map() to make
> it generic
> 538abe powerpc/smp: Add support detecting thread-groups sharing L2 cache
> 0be4763 powerpc/cacheinfo: Print correct cache-sibling map/list for L2 cache
>
> However, with the LLC now on the SMT sched-domain, we are seeing some
> regressions in the performance of applications that requires
> single-threaded performance. The reason for this is as follows:
>
> Prior to the change (we call this P9-sched below), the sched-domain
> hierarchy was:
>
> SMT (SMT4) --> CACHE (SMT8)[LLC] --> MC (Hemisphere) --> DIE
>
> where the CACHE sched-domain is defined to be the Last Level Cache (LLC).
>
> On the upstream kernel, with the aforementioned commmits (P10-sched),
> the sched-domain hierarchy is:
>
> SMT (SMT4)[LLC] --> MC (Hemisphere) --> DIE
>
> with the SMT sched-domain as the LLC.
>
> When the scheduler tries to wakeup a task, it chooses between the
> waker-CPU and the wakee's previous-CPU. Suppose this choice is called
> the "target", then in the target's LLC domain, the scheduler
>
> a) tries to find an idle core in the LLC. This helps exploit the
> SMT folding that the wakee task can benefit from. If an idle
> core is found, the wakee is woken up on it.
>
> b) Failing to find an idle core, the scheduler tries to find an idle
> CPU in the LLC. This helps minimise the wakeup latency for the
> wakee since it gets to run on the CPU immediately.
>
> c) Failing this, it will wake it up on target CPU.
>
> Thus, with P9-sched topology, since the CACHE domain comprises of two
> SMT4 cores, there is a decent chance that we get an idle core, failing
> which there is a relatively higher probability of finding an idle CPU
> among the 8 threads in the domain.
>
> However, in P10-sched topology, since the SMT domain is the LLC and it
> contains only a single SMT4 core, the probability that we find that
> core to be idle is less. Furthermore, since there are only 4 CPUs to
> search for an idle CPU, there is lower probability that we can get an
> idle CPU to wake up the task on.
>
> Thus applications which require single threaded performance will end
> up getting woken up on potentially busy core, even though there are
> idle cores in the system.
>
> To remedy this, this patch proposes that the LLC be moved to the MC
> level which is a group of cores in one half of the chip.
>
> SMT (SMT4) --> MC (Hemisphere)[LLC] --> DIE
>
> While there is no cache being shared at this level, this is still the
> level where some amount of cache-snooping takes place and it is
> relatively faster to access the data from the caches of the cores
> within this domain. With this change, we no longer see regressions on
> P10 for applications which require single threaded performance.
>
> The patch also improves the tail latencies on schbench and the
> usecs/op on "perf bench sched pipe"
>
> On a 10 core P10 system with 80 CPUs,
>
> schbench
> ============
> (https://git.kernel.org/pub/scm/linux/kernel/git/mason/schbench.git/)
>
> Values : Lower the better.
> 99th percentile is the tail latency.
>
>
> 99th percentile
> ~~~~~~~~~~~~~~~~~~
> No. messenger
> threads 5.12-rc4 5.12-rc4
> P10-sched MC-LLC
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 1 70 us 85 us
> 2 81 us 101 us
> 3 92 us 107 us
> 4 96 us 110 us
> 5 103 us 123 us
> 6 3412 us ----> 122 us
> 7 1490 us 136 us
> 8 6200 us 3572 us
>
>
> Hackbench
> ============
> (perf bench sched pipe)
> values: lower the better
>
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> No. of
> parallel
> instances 5.12-rc4 5.12-rc4
> P10-sched MC-LLC
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 1 24.04 us/op 18.72 us/op
> 2 24.04 us/op 18.65 us/op
> 4 24.01 us/op 18.76 us/op
> 8 24.10 us/op 19.11 us/op
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> ---
> arch/powerpc/kernel/smp.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index 5a4d59a..c75dbd4 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -976,6 +976,13 @@ static bool has_coregroup_support(void)
> return coregroup_enabled;
> }
>
> +static int powerpc_mc_flags(void)
> +{
> + if(has_coregroup_support())
> + return SD_SHARE_PKG_RESOURCES;
> + return 0;
> +}
> +
> static const struct cpumask *cpu_mc_mask(int cpu)
> {
> return cpu_coregroup_mask(cpu);
> @@ -986,7 +993,7 @@ static const struct cpumask *cpu_mc_mask(int cpu)
> { cpu_smt_mask, powerpc_smt_flags, SD_INIT_NAME(SMT) },
> #endif
> { shared_cache_mask, powerpc_shared_cache_flags, SD_INIT_NAME(CACHE) },
> - { cpu_mc_mask, SD_INIT_NAME(MC) },
> + { cpu_mc_mask, powerpc_mc_flags, SD_INIT_NAME(MC) },
> { cpu_cpu_mask, SD_INIT_NAME(DIE) },
> { NULL, },
> };
> --
> 1.9.4
>
^ permalink raw reply
* [RFC/PATCH] powerpc/smp: Add SD_SHARE_PKG_RESOURCES flag to MC sched-domain
From: Gautham R. Shenoy @ 2021-04-02 5:37 UTC (permalink / raw)
To: Michael Ellerman, Michael Neuling, Mel Gorman, Rik van Riel,
Srikar Dronamraju, Valentin Schneider, Vincent Guittot,
Dietmar Eggemann, Nicholas Piggin, Anton Blanchard, Parth Shah,
Vaidyanathan Srinivasan
Cc: Gautham R. Shenoy, linuxppc-dev, LKML
From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
On POWER10 systems, the L2 cache is at the SMT4 small core level. The
following commits ensure that L2 cache gets correctly discovered and
the Last-Level-Cache domain (LLC) is set to the SMT sched-domain.
790a166 powerpc/smp: Parse ibm,thread-groups with multiple properties
1fdc1d6 powerpc/smp: Rename cpu_l1_cache_map as thread_group_l1_cache_map
fbd2b67 powerpc/smp: Rename init_thread_group_l1_cache_map() to make
it generic
538abe powerpc/smp: Add support detecting thread-groups sharing L2 cache
0be4763 powerpc/cacheinfo: Print correct cache-sibling map/list for L2 cache
However, with the LLC now on the SMT sched-domain, we are seeing some
regressions in the performance of applications that requires
single-threaded performance. The reason for this is as follows:
Prior to the change (we call this P9-sched below), the sched-domain
hierarchy was:
SMT (SMT4) --> CACHE (SMT8)[LLC] --> MC (Hemisphere) --> DIE
where the CACHE sched-domain is defined to be the Last Level Cache (LLC).
On the upstream kernel, with the aforementioned commmits (P10-sched),
the sched-domain hierarchy is:
SMT (SMT4)[LLC] --> MC (Hemisphere) --> DIE
with the SMT sched-domain as the LLC.
When the scheduler tries to wakeup a task, it chooses between the
waker-CPU and the wakee's previous-CPU. Suppose this choice is called
the "target", then in the target's LLC domain, the scheduler
a) tries to find an idle core in the LLC. This helps exploit the
SMT folding that the wakee task can benefit from. If an idle
core is found, the wakee is woken up on it.
b) Failing to find an idle core, the scheduler tries to find an idle
CPU in the LLC. This helps minimise the wakeup latency for the
wakee since it gets to run on the CPU immediately.
c) Failing this, it will wake it up on target CPU.
Thus, with P9-sched topology, since the CACHE domain comprises of two
SMT4 cores, there is a decent chance that we get an idle core, failing
which there is a relatively higher probability of finding an idle CPU
among the 8 threads in the domain.
However, in P10-sched topology, since the SMT domain is the LLC and it
contains only a single SMT4 core, the probability that we find that
core to be idle is less. Furthermore, since there are only 4 CPUs to
search for an idle CPU, there is lower probability that we can get an
idle CPU to wake up the task on.
Thus applications which require single threaded performance will end
up getting woken up on potentially busy core, even though there are
idle cores in the system.
To remedy this, this patch proposes that the LLC be moved to the MC
level which is a group of cores in one half of the chip.
SMT (SMT4) --> MC (Hemisphere)[LLC] --> DIE
While there is no cache being shared at this level, this is still the
level where some amount of cache-snooping takes place and it is
relatively faster to access the data from the caches of the cores
within this domain. With this change, we no longer see regressions on
P10 for applications which require single threaded performance.
The patch also improves the tail latencies on schbench and the
usecs/op on "perf bench sched pipe"
On a 10 core P10 system with 80 CPUs,
schbench
============
(https://git.kernel.org/pub/scm/linux/kernel/git/mason/schbench.git/)
Values : Lower the better.
99th percentile is the tail latency.
99th percentile
~~~~~~~~~~~~~~~~~~
No. messenger
threads 5.12-rc4 5.12-rc4
P10-sched MC-LLC
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 70 us 85 us
2 81 us 101 us
3 92 us 107 us
4 96 us 110 us
5 103 us 123 us
6 3412 us ----> 122 us
7 1490 us 136 us
8 6200 us 3572 us
Hackbench
============
(perf bench sched pipe)
values: lower the better
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
No. of
parallel
instances 5.12-rc4 5.12-rc4
P10-sched MC-LLC
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 24.04 us/op 18.72 us/op
2 24.04 us/op 18.65 us/op
4 24.01 us/op 18.76 us/op
8 24.10 us/op 19.11 us/op
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
---
arch/powerpc/kernel/smp.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 5a4d59a..c75dbd4 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -976,6 +976,13 @@ static bool has_coregroup_support(void)
return coregroup_enabled;
}
+static int powerpc_mc_flags(void)
+{
+ if(has_coregroup_support())
+ return SD_SHARE_PKG_RESOURCES;
+ return 0;
+}
+
static const struct cpumask *cpu_mc_mask(int cpu)
{
return cpu_coregroup_mask(cpu);
@@ -986,7 +993,7 @@ static const struct cpumask *cpu_mc_mask(int cpu)
{ cpu_smt_mask, powerpc_smt_flags, SD_INIT_NAME(SMT) },
#endif
{ shared_cache_mask, powerpc_shared_cache_flags, SD_INIT_NAME(CACHE) },
- { cpu_mc_mask, SD_INIT_NAME(MC) },
+ { cpu_mc_mask, powerpc_mc_flags, SD_INIT_NAME(MC) },
{ cpu_cpu_mask, SD_INIT_NAME(DIE) },
{ NULL, },
};
--
1.9.4
^ permalink raw reply related
* Re: [PATCH] powerpc/powernv: Enable HAIL (HV AIL) for ISA v3.1 processors
From: Cédric Le Goater @ 2021-04-02 6:10 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev
In-Reply-To: <20210402024124.545826-1-npiggin@gmail.com>
On 4/2/21 4:41 AM, Nicholas Piggin wrote:
> Starting with ISA v3.1, LPCR[AIL] no longer controls the interrupt
> mode for HV=1 interrupts. Instead, a new LPCR[HAIL] bit is defined
> which behaves like AIL=3 for HV interrupts when set.
Will QEMU need an update ?
Thanks,
C.
> Set HAIL on bare metal to give us mmu-on interrupts and improve
> performance.
>
> This also fixes an scv bug: we don't implement scv real mode (AIL=0)
> vectors because they are at an inconvenient location, so we just
> disable scv support when AIL can not be set. However powernv assumes
> that LPCR[AIL] will enable AIL mode so it enables scv support despite
> HV interrupts being AIL=0, which causes scv interrupts to go off into
> the weeds.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/include/asm/reg.h | 1 +
> arch/powerpc/kernel/setup_64.c | 19 ++++++++++++++++---
> 2 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> index 1be20bc8dce2..9086a2644c89 100644
> --- a/arch/powerpc/include/asm/reg.h
> +++ b/arch/powerpc/include/asm/reg.h
> @@ -441,6 +441,7 @@
> #define LPCR_VRMA_LP1 ASM_CONST(0x0000800000000000)
> #define LPCR_RMLS 0x1C000000 /* Implementation dependent RMO limit sel */
> #define LPCR_RMLS_SH 26
> +#define LPCR_HAIL ASM_CONST(0x0000000004000000) /* HV AIL (ISAv3.1) */
> #define LPCR_ILE ASM_CONST(0x0000000002000000) /* !HV irqs set MSR:LE */
> #define LPCR_AIL ASM_CONST(0x0000000001800000) /* Alternate interrupt location */
> #define LPCR_AIL_0 ASM_CONST(0x0000000000000000) /* MMU off exception offset 0x0 */
> diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
> index 04a31586f760..671192afcdfd 100644
> --- a/arch/powerpc/kernel/setup_64.c
> +++ b/arch/powerpc/kernel/setup_64.c
> @@ -233,10 +233,23 @@ static void cpu_ready_for_interrupts(void)
> * If we are not in hypervisor mode the job is done once for
> * the whole partition in configure_exceptions().
> */
> - if (cpu_has_feature(CPU_FTR_HVMODE) &&
> - cpu_has_feature(CPU_FTR_ARCH_207S)) {
> + if (cpu_has_feature(CPU_FTR_HVMODE)) {
> unsigned long lpcr = mfspr(SPRN_LPCR);
> - mtspr(SPRN_LPCR, lpcr | LPCR_AIL_3);
> + unsigned long new_lpcr = lpcr;
> +
> + if (cpu_has_feature(CPU_FTR_ARCH_31)) {
> + /* P10 DD1 does not have HAIL */
> + if (pvr_version_is(PVR_POWER10) &&
> + (mfspr(SPRN_PVR) & 0xf00) == 0x100)
> + new_lpcr |= LPCR_AIL_3;
> + else
> + new_lpcr |= LPCR_HAIL;
> + } else if (cpu_has_feature(CPU_FTR_ARCH_207S)) {
> + new_lpcr |= LPCR_AIL_3;
> + }
> +
> + if (new_lpcr != lpcr)
> + mtspr(SPRN_LPCR, new_lpcr);
> }
>
> /*
>
^ permalink raw reply
* Re: [PATCH v4 29/46] KVM: PPC: Book3S HV P9: Implement the rest of the P9 path in C
From: Alexey Kardashevskiy @ 2021-04-02 4:36 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <1617272101.bcglven6fh.astroid@bobo.none>
On 01/04/2021 21:35, Nicholas Piggin wrote:
> Excerpts from Alexey Kardashevskiy's message of April 1, 2021 3:30 pm:
>>
>>
>> On 3/23/21 12:02 PM, Nicholas Piggin wrote:
>>> Almost all logic is moved to C, by introducing a new in_guest mode that
>>> selects and branches very early in the interrupt handler to the P9 exit
>>> code.
>
> [...]
>
>>> +/*
>>> + * kvmppc_p9_exit_hcall and kvmppc_p9_exit_interrupt are branched to from
>>> + * above if the interrupt was taken for a guest that was entered via
>>> + * kvmppc_p9_enter_guest().
>>> + *
>>> + * This code recovers the host stack and vcpu pointer, saves all GPRs and
>>> + * CR, LR, CTR, XER as well as guest MSR and NIA into the VCPU, then re-
>>> + * establishes the host stack and registers to return from the
>>> + * kvmppc_p9_enter_guest() function.
>>
>> What does "this code" refer to? If it is the asm below, then it does not
>> save CTR, it is in the c code. Otherwise it is confusing (to me) :)
>
> Yes you're right, CTR is saved in C.
>
>>> + */
>>> +.balign IFETCH_ALIGN_BYTES
>>> +kvmppc_p9_exit_hcall:
>>> + mfspr r11,SPRN_SRR0
>>> + mfspr r12,SPRN_SRR1
>>> + li r10,0xc00
>>> + std r10,HSTATE_SCRATCH0(r13)
>>> +
>>> +.balign IFETCH_ALIGN_BYTES
>>> +kvmppc_p9_exit_interrupt:
>
> [...]
>
>>> +static inline void slb_invalidate(unsigned int ih)
>>> +{
>>> + asm volatile("slbia %0" :: "i"(ih));
>>> +}
>>
>> This one is not used.
>
> It gets used in a later patch, I guess I should move it there.
>
> [...]
>
>>> +int __kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu)
>>> +{
>>> + u64 *exsave;
>>> + unsigned long msr = mfmsr();
>>> + int trap;
>>> +
>>> + start_timing(vcpu, &vcpu->arch.rm_entry);
>>> +
>>> + vcpu->arch.ceded = 0;
>>> +
>>> + WARN_ON_ONCE(vcpu->arch.shregs.msr & MSR_HV);
>>> + WARN_ON_ONCE(!(vcpu->arch.shregs.msr & MSR_ME));
>>> +
>>> + mtspr(SPRN_HSRR0, vcpu->arch.regs.nip);
>>> + mtspr(SPRN_HSRR1, (vcpu->arch.shregs.msr & ~MSR_HV) | MSR_ME);
>>> +
>>> + /*
>>> + * On POWER9 DD2.1 and below, sometimes on a Hypervisor Data Storage
>>> + * Interrupt (HDSI) the HDSISR is not be updated at all.
>>> + *
>>> + * To work around this we put a canary value into the HDSISR before
>>> + * returning to a guest and then check for this canary when we take a
>>> + * HDSI. If we find the canary on a HDSI, we know the hardware didn't
>>> + * update the HDSISR. In this case we return to the guest to retake the
>>> + * HDSI which should correctly update the HDSISR the second time HDSI
>>> + * entry.
>>> + *
>>> + * Just do this on all p9 processors for now.
>>> + */
>>> + mtspr(SPRN_HDSISR, HDSISR_CANARY);
>>> +
>>> + accumulate_time(vcpu, &vcpu->arch.guest_time);
>>> +
>>> + local_paca->kvm_hstate.in_guest = KVM_GUEST_MODE_GUEST_HV_FAST;
>>> + kvmppc_p9_enter_guest(vcpu);
>>> + // Radix host and guest means host never runs with guest MMU state
>>> + local_paca->kvm_hstate.in_guest = KVM_GUEST_MODE_NONE;
>>> +
>>> + accumulate_time(vcpu, &vcpu->arch.rm_intr);
>>> +
>>> + /* Get these from r11/12 and paca exsave */
>>> + vcpu->arch.shregs.srr0 = mfspr(SPRN_SRR0);
>>> + vcpu->arch.shregs.srr1 = mfspr(SPRN_SRR1);
>>> + vcpu->arch.shregs.dar = mfspr(SPRN_DAR);
>>> + vcpu->arch.shregs.dsisr = mfspr(SPRN_DSISR);
>>> +
>>> + /* 0x2 bit for HSRR is only used by PR and P7/8 HV paths, clear it */
>>> + trap = local_paca->kvm_hstate.scratch0 & ~0x2;
>>> + if (likely(trap > BOOK3S_INTERRUPT_MACHINE_CHECK)) {
>>> + exsave = local_paca->exgen;
>>> + } else if (trap == BOOK3S_INTERRUPT_SYSTEM_RESET) {
>>> + exsave = local_paca->exnmi;
>>> + } else { /* trap == 0x200 */
>>> + exsave = local_paca->exmc;
>>> + }
>>> +
>>> + vcpu->arch.regs.gpr[1] = local_paca->kvm_hstate.scratch1;
>>> + vcpu->arch.regs.gpr[3] = local_paca->kvm_hstate.scratch2;
>>> + vcpu->arch.regs.gpr[9] = exsave[EX_R9/sizeof(u64)];
>>> + vcpu->arch.regs.gpr[10] = exsave[EX_R10/sizeof(u64)];
>>> + vcpu->arch.regs.gpr[11] = exsave[EX_R11/sizeof(u64)];
>>> + vcpu->arch.regs.gpr[12] = exsave[EX_R12/sizeof(u64)];
>>> + vcpu->arch.regs.gpr[13] = exsave[EX_R13/sizeof(u64)];
>>> + vcpu->arch.ppr = exsave[EX_PPR/sizeof(u64)];
>>> + vcpu->arch.cfar = exsave[EX_CFAR/sizeof(u64)];
>>> + vcpu->arch.regs.ctr = exsave[EX_CTR/sizeof(u64)];
>>> +
>>> + vcpu->arch.last_inst = KVM_INST_FETCH_FAILED;
>>> +
>>> + if (unlikely(trap == BOOK3S_INTERRUPT_MACHINE_CHECK)) {
>>> + vcpu->arch.fault_dar = exsave[EX_DAR/sizeof(u64)];
>>> + vcpu->arch.fault_dsisr = exsave[EX_DSISR/sizeof(u64)];
>>> + kvmppc_realmode_machine_check(vcpu);
>>> +
>>> + } else if (unlikely(trap == BOOK3S_INTERRUPT_HMI)) {
>>> + kvmppc_realmode_hmi_handler();
>>> +
>>> + } else if (trap == BOOK3S_INTERRUPT_H_EMUL_ASSIST) {
>>> + vcpu->arch.emul_inst = mfspr(SPRN_HEIR);
>>> +
>>> + } else if (trap == BOOK3S_INTERRUPT_H_DATA_STORAGE) {
>>> + vcpu->arch.fault_dar = exsave[EX_DAR/sizeof(u64)];
>>> + vcpu->arch.fault_dsisr = exsave[EX_DSISR/sizeof(u64)];
>>> + vcpu->arch.fault_gpa = mfspr(SPRN_ASDR);
>>> +
>>> + } else if (trap == BOOK3S_INTERRUPT_H_INST_STORAGE) {
>>> + vcpu->arch.fault_gpa = mfspr(SPRN_ASDR);
>>> +
>>> + } else if (trap == BOOK3S_INTERRUPT_H_FAC_UNAVAIL) {
>>> + vcpu->arch.hfscr = mfspr(SPRN_HFSCR);
>>> +
>>> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>>> + /*
>>> + * Softpatch interrupt for transactional memory emulation cases
>>> + * on POWER9 DD2.2. This is early in the guest exit path - we
>>> + * haven't saved registers or done a treclaim yet.
>>> + */
>>> + } else if (trap == BOOK3S_INTERRUPT_HV_SOFTPATCH) {
>>> + vcpu->arch.emul_inst = mfspr(SPRN_HEIR);
>>> +
>>> + /*
>>> + * The cases we want to handle here are those where the guest
>>> + * is in real suspend mode and is trying to transition to
>>> + * transactional mode.
>>> + */
>>> + if (local_paca->kvm_hstate.fake_suspend &&
>>> + (vcpu->arch.shregs.msr & MSR_TS_S)) {
>>> + if (kvmhv_p9_tm_emulation_early(vcpu)) {
>>> + /* Prevent it being handled again. */
>>> + trap = 0;
>>> + }
>>> + }
>>> +#endif
>>> + }
>>> +
>>> + radix_clear_slb();
>>> +
>>> + __mtmsrd(msr, 0);
>>
>>
>> The asm code only sets RI but this potentially sets more bits including
>> MSR_EE, is it expected to be 0 when __kvmhv_vcpu_entry_p9() is called?
>
> Yes.
>
>>> + mtspr(SPRN_CTRLT, 1);
>>
>> What is this for? ISA does not shed much light:
>> ===
>> 63 RUN This bit controls an external I/O pin.
>> ===
>
> I don't think it even does that these days. It interacts with the PMU.
> I was looking whether it's feasible to move it into PMU code entirely,
> but apparently some tool or something might sample it. I'm a bit
> suspicious about that because an untrusted guest could be running and
> claim not to so I don't know what said tool really achieves, but I'll
> go through that fight another day.
>
> But KVM has to set it to 1 at exit because Linux host has it set to 1
> except in CPU idle.
It this CTRLT setting a new thing or the asm does it too? I could not
spot it.
>>
>>
>>> +
>>> + accumulate_time(vcpu, &vcpu->arch.rm_exit);
>>
>> This should not compile without CONFIG_KVM_BOOK3S_HV_EXIT_TIMING.
>
> It has an ifdef wrapper so it should work (it does on my local tree
> which is slightly newer than what you have but I don't think I fixed
> anything around this recently).
You are absolutely right, my bad.
>
>>> +
>>> + end_timing(vcpu);
>>> +
>>> + return trap;
>>
>>
>> The asm does "For hash guest, read the guest SLB and save it away", this
>> code does not. Is this new fast-path-in-c only for radix-on-radix or
>> hash VMs are supported too?
>
> That asm code does not run for "guest_exit_short_path" case (aka the
> p9 path aka the fast path).
>
> Upstream code only supports radix host and radix guest in this path.
> The old path supports hash and radix. That's unchanged with this patch.
>
> After the series, the new path supports all P9 modes (hash/hash,
> radix/radix, and radix/hash), and the old path supports P7 and P8 only.
Thanks for clarification. Besides that CTRLT, I checked if the new c
code matches the old asm code (which made diving into ISA incredible fun
:) ) so fwiw
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
I'd really like to see longer commit logs clarifying all intended
changes but it is probably just me.
>
> Thanks,
> Nick
>
--
Alexey
^ permalink raw reply
* Re: [PATCH v5 25/48] KVM: PPC: Book3S HV P9: Use host timer accounting to avoid decrementer read
From: kernel test robot @ 2021-04-02 4:19 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev, kbuild-all, Nicholas Piggin
In-Reply-To: <20210401150325.442125-26-npiggin@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 5557 bytes --]
Hi Nicholas,
I love your patch! Perhaps something to improve:
[auto build test WARNING on powerpc/next]
[also build test WARNING on v5.12-rc5 next-20210401]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Nicholas-Piggin/KVM-PPC-Book3S-C-ify-the-P9-entry-exit-code/20210401-232743
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-randconfig-s031-20210401 (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.3-279-g6d5d9b42-dirty
# https://github.com/0day-ci/linux/commit/4c3e837b20a4ebe406d7b8fddbec2f6fa86755a2
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Nicholas-Piggin/KVM-PPC-Book3S-C-ify-the-P9-entry-exit-code/20210401-232743
git checkout 4c3e837b20a4ebe406d7b8fddbec2f6fa86755a2
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=powerpc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> arch/powerpc/kernel/time.c:877:9: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected void const [noderef] __percpu *__vpp_verify @@ got unsigned long long * @@
arch/powerpc/kernel/time.c:877:9: sparse: expected void const [noderef] __percpu *__vpp_verify
arch/powerpc/kernel/time.c:877:9: sparse: got unsigned long long *
>> arch/powerpc/kernel/time.c:877:9: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected void const [noderef] __percpu *__vpp_verify @@ got unsigned long long * @@
arch/powerpc/kernel/time.c:877:9: sparse: expected void const [noderef] __percpu *__vpp_verify
arch/powerpc/kernel/time.c:877:9: sparse: got unsigned long long *
>> arch/powerpc/kernel/time.c:877:9: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected void const [noderef] __percpu *__vpp_verify @@ got unsigned long long * @@
arch/powerpc/kernel/time.c:877:9: sparse: expected void const [noderef] __percpu *__vpp_verify
arch/powerpc/kernel/time.c:877:9: sparse: got unsigned long long *
>> arch/powerpc/kernel/time.c:877:9: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected void const [noderef] __percpu *__vpp_verify @@ got unsigned long long * @@
arch/powerpc/kernel/time.c:877:9: sparse: expected void const [noderef] __percpu *__vpp_verify
arch/powerpc/kernel/time.c:877:9: sparse: got unsigned long long *
>> arch/powerpc/kernel/time.c:877:9: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected void const [noderef] __percpu *__vpp_verify @@ got unsigned long long * @@
arch/powerpc/kernel/time.c:877:9: sparse: expected void const [noderef] __percpu *__vpp_verify
arch/powerpc/kernel/time.c:877:9: sparse: got unsigned long long *
arch/powerpc/kernel/time.c:130:1: sparse: sparse: symbol 'rtc_lock' was not declared. Should it be static?
arch/powerpc/kernel/time.c:649:24: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected void const [noderef] __percpu *__vpp_verify @@ got unsigned long long * @@
arch/powerpc/kernel/time.c:649:24: sparse: expected void const [noderef] __percpu *__vpp_verify
arch/powerpc/kernel/time.c:649:24: sparse: got unsigned long long *
arch/powerpc/kernel/time.c:580:24: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected void const [noderef] __percpu *__vpp_verify @@ got unsigned long long * @@
arch/powerpc/kernel/time.c:580:24: sparse: expected void const [noderef] __percpu *__vpp_verify
arch/powerpc/kernel/time.c:580:24: sparse: got unsigned long long *
vim +877 arch/powerpc/kernel/time.c
4a4cfe3836916e Tony Breeds 2007-09-22 873
d831d0b83f2058 Tony Breeds 2007-09-21 874 static int decrementer_set_next_event(unsigned long evt,
d831d0b83f2058 Tony Breeds 2007-09-21 875 struct clock_event_device *dev)
d831d0b83f2058 Tony Breeds 2007-09-21 876 {
6601ec1c2ba929 Christophe Leroy 2020-09-29 @877 __this_cpu_write(decrementers_next_tb, get_tb() + evt);
d831d0b83f2058 Tony Breeds 2007-09-21 878 set_dec(evt);
0215f7d8c53fb1 Benjamin Herrenschmidt 2014-01-14 879
0215f7d8c53fb1 Benjamin Herrenschmidt 2014-01-14 880 /* We may have raced with new irq work */
0215f7d8c53fb1 Benjamin Herrenschmidt 2014-01-14 881 if (test_irq_work_pending())
0215f7d8c53fb1 Benjamin Herrenschmidt 2014-01-14 882 set_dec(1);
0215f7d8c53fb1 Benjamin Herrenschmidt 2014-01-14 883
d831d0b83f2058 Tony Breeds 2007-09-21 884 return 0;
d831d0b83f2058 Tony Breeds 2007-09-21 885 }
d831d0b83f2058 Tony Breeds 2007-09-21 886
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34768 bytes --]
^ permalink raw reply
* Re: [PATCH 1/1] powerpc/smp: Set numa node before updating mask
From: Srikar Dronamraju @ 2021-04-02 3:18 UTC (permalink / raw)
To: Nathan Lynch
Cc: Gautham R Shenoy, Peter Zijlstra, Scott Cheloha,
Geetika Moolchandani, Valentin Schneider, Laurent Dufour,
linuxppc-dev, Ingo Molnar
In-Reply-To: <87czvdbova.fsf@linux.ibm.com>
* Nathan Lynch <nathanl@linux.ibm.com> [2021-04-01 17:51:05]:
Thanks Nathan for reviewing.
> > - set_numa_node(numa_cpu_lookup_table[cpu]);
> > - set_numa_mem(local_memory_node(numa_cpu_lookup_table[cpu]));
> > -
>
> Regardless of your change: at boot time, this set of calls to
> set_numa_node() and set_numa_mem() is redundant, right? Because
> smp_prepare_cpus() has:
>
> for_each_possible_cpu(cpu) {
> ...
> if (cpu_present(cpu)) {
> set_cpu_numa_node(cpu, numa_cpu_lookup_table[cpu]);
> set_cpu_numa_mem(cpu,
> local_memory_node(numa_cpu_lookup_table[cpu]));
> }
>
> I would rather that, when onlining a CPU that happens to have been
> dynamically added after boot, we enter start_secondary() with conditions
> equivalent to those at boot time. Or as close to that as is practical.
>
> So I'd suggest that pseries_add_processor() be made to update
> these things when the CPUs are marked present, before onlining them.
In pseries_add_processor, we are only marking the cpu as present. i.e
I believe numa_setup_cpu() would not have been called. So we may not have a
way to associate the CPU to the node. Otherwise we will have to call
numa_setup_cpu() or the hcall_vphn.
We could try calling numa_setup_cpu() immediately after we set the
CPU to be present, but that would be one more extra hcall + I dont know if
there are any more steps needed before CPU being made present and
associating the CPU to the node. Are we sure the node is already online? For
the numa_mem, we are better of if the zonelists for the node are built.
or the other solution would be to call this in map_cpu_to_node().
Here also we have to be sure the zonelists for the node are already built.
--
Thanks and Regards
Srikar Dronamraju
^ permalink raw reply
* [PATCH] powerpc/powernv: Enable HAIL (HV AIL) for ISA v3.1 processors
From: Nicholas Piggin @ 2021-04-02 2:41 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin
Starting with ISA v3.1, LPCR[AIL] no longer controls the interrupt
mode for HV=1 interrupts. Instead, a new LPCR[HAIL] bit is defined
which behaves like AIL=3 for HV interrupts when set.
Set HAIL on bare metal to give us mmu-on interrupts and improve
performance.
This also fixes an scv bug: we don't implement scv real mode (AIL=0)
vectors because they are at an inconvenient location, so we just
disable scv support when AIL can not be set. However powernv assumes
that LPCR[AIL] will enable AIL mode so it enables scv support despite
HV interrupts being AIL=0, which causes scv interrupts to go off into
the weeds.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/reg.h | 1 +
arch/powerpc/kernel/setup_64.c | 19 ++++++++++++++++---
2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index 1be20bc8dce2..9086a2644c89 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -441,6 +441,7 @@
#define LPCR_VRMA_LP1 ASM_CONST(0x0000800000000000)
#define LPCR_RMLS 0x1C000000 /* Implementation dependent RMO limit sel */
#define LPCR_RMLS_SH 26
+#define LPCR_HAIL ASM_CONST(0x0000000004000000) /* HV AIL (ISAv3.1) */
#define LPCR_ILE ASM_CONST(0x0000000002000000) /* !HV irqs set MSR:LE */
#define LPCR_AIL ASM_CONST(0x0000000001800000) /* Alternate interrupt location */
#define LPCR_AIL_0 ASM_CONST(0x0000000000000000) /* MMU off exception offset 0x0 */
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index 04a31586f760..671192afcdfd 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -233,10 +233,23 @@ static void cpu_ready_for_interrupts(void)
* If we are not in hypervisor mode the job is done once for
* the whole partition in configure_exceptions().
*/
- if (cpu_has_feature(CPU_FTR_HVMODE) &&
- cpu_has_feature(CPU_FTR_ARCH_207S)) {
+ if (cpu_has_feature(CPU_FTR_HVMODE)) {
unsigned long lpcr = mfspr(SPRN_LPCR);
- mtspr(SPRN_LPCR, lpcr | LPCR_AIL_3);
+ unsigned long new_lpcr = lpcr;
+
+ if (cpu_has_feature(CPU_FTR_ARCH_31)) {
+ /* P10 DD1 does not have HAIL */
+ if (pvr_version_is(PVR_POWER10) &&
+ (mfspr(SPRN_PVR) & 0xf00) == 0x100)
+ new_lpcr |= LPCR_AIL_3;
+ else
+ new_lpcr |= LPCR_HAIL;
+ } else if (cpu_has_feature(CPU_FTR_ARCH_207S)) {
+ new_lpcr |= LPCR_AIL_3;
+ }
+
+ if (new_lpcr != lpcr)
+ mtspr(SPRN_LPCR, new_lpcr);
}
/*
--
2.23.0
^ permalink raw reply related
* Re: [PATCH printk v2 2/5] printk: remove safe buffers
From: Sergey Senozhatsky @ 2021-04-02 2:14 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Peter Zijlstra, Paul Mackerras, Tiezhu Yang,
Rafael Aquini, Paul E. McKenney, Aneesh Kumar K.V,
Alexey Kardashevskiy, Yue Hu, Jordan Niethe, Kees Cook,
John Ogness, Alistair Popple, Guilherme G. Piccoli,
Nicholas Piggin, Steven Rostedt, Thomas Gleixner, kexec,
linux-kernel, Sergey Senozhatsky, Eric Biederman, Andrew Morton,
linuxppc-dev
In-Reply-To: <YGXV8LJarjUJDhvy@alley>
On (21/04/01 16:17), Petr Mladek wrote:
> > For the long term, we should introduce a printk-context API that allows
> > callers to perfectly pack their multi-line output into a single
> > entry. We discussed [0][1] this back in August 2020.
>
> We need a "short" term solution. There are currently 3 solutions:
>
> 1. Keep nmi_safe() and all the hacks around.
>
> 2. Serialize nmi_cpu_backtrace() by a spin lock and later by
> the special lock used also by atomic consoles.
>
> 3. Tell complaining people how to sort the messed logs.
Are we talking about nmi_cpu_backtrace()->dump_stack() or some
other path?
dump_stack() seems to be already serialized by `dump_lock`. Hmm,
show_regs() is not serialized, seems like it should be under the
same `dump_lock` as dump_stack().
^ permalink raw reply
* Re: [PATCH v2] powerpc/traps: Enhance readability for trap types
From: Nicholas Piggin @ 2021-04-02 0:36 UTC (permalink / raw)
To: Segher Boessenkool, Xiongwei Song
Cc: ravi.bangoria, aik, peterx, oleg, paulus, kan.liang, leobras.c,
mikey, maddy, haren, peterz, kjain, msuchanek, pmladek,
john.ogness, alistair, kvm-ppc, jniethe5, atrajeev, Xiongwei Song,
linux-kernel, aneesh.kumar, akpm, linuxppc-dev, rppt
In-Reply-To: <20210401161131.GE13863@gate.crashing.org>
Excerpts from Segher Boessenkool's message of April 2, 2021 2:11 am:
> On Thu, Apr 01, 2021 at 10:55:58AM +0800, Xiongwei Song wrote:
>> Segher Boessenkool <segher@kernel.crashing.org> 于2021年4月1日周四 上午6:15写道：
>>
>> > On Wed, Mar 31, 2021 at 08:58:17PM +1100, Michael Ellerman wrote:
>> > > So perhaps:
>> > >
>> > > EXC_SYSTEM_RESET
>> > > EXC_MACHINE_CHECK
>> > > EXC_DATA_STORAGE
>> > > EXC_DATA_SEGMENT
>> > > EXC_INST_STORAGE
>> > > EXC_INST_SEGMENT
>> > > EXC_EXTERNAL_INTERRUPT
>> > > EXC_ALIGNMENT
>> > > EXC_PROGRAM_CHECK
>> > > EXC_FP_UNAVAILABLE
>> > > EXC_DECREMENTER
>> > > EXC_HV_DECREMENTER
>> > > EXC_SYSTEM_CALL
>> > > EXC_HV_DATA_STORAGE
>> > > EXC_PERF_MONITOR
>> >
>> > These are interrupt (vectors), not exceptions. It doesn't matter all
>> > that much, but confusing things more isn't useful either! There can be
>> > multiple exceptions that all can trigger the same interrupt.
>> >
>> > When looking at the reference manual of e500 and e600 from NXP
>> official, they call them as interrupts.While looking at the "The
>> Programming Environments"
>> that is also from NXP, they call them exceptions. Looks like there is
>> no explicit distinction between interrupts and exceptions.
>
> The architecture documents have always called it interrupts. The PEM
> says it calls them exceptions instead, but they are called interrupts in
> the architecture (and the PEM says that, too).
>
>> Here is the "The Programming Environments" link:
>> https://www.nxp.com.cn/docs/en/user-guide/MPCFPE_AD_R1.pdf
>
> That document is 24 years old. The architecture is still published,
> new versions regularly.
>
>> As far as I know, the values of interrupts or exceptions above are defined
>> explicitly in reference manual or the programming environments.
>
> They are defined in the architecture.
>
>> Could
>> you please provide more details about multiple exceptions with the same
>> interrupts?
>
> The simplest example is 700, program interrupt. There are many causes
> for it, including all the exceptions in FPSCR: VX, ZX, OX, UX, XX, and
> VX is actually divided into nine separate cases itself. There also are
> the various causes of privileged instruction type program interrupts,
> and the trap type program interrupt, but the FEX ones are most obvious
> here.
Also:
* Some interrupts have no corresponding exception (system call and
system call vectored). This is not just semantics or a bug in the ISA
because it is different from other synchronous interrupts: instructions
which cause exceptions (e.g., a page fault) do not complete before
taking the interrupt whereas sc does.
* It's quite usual for an exception to not cause an interrupt
immediately (MSR[EE]=0, HMEER) or never cause one and be cleared by
other means (msgclr, mtDEC, mtHMER, etc).
* It's possible for an exception to cause different interrupts!
A decrementer exception usually causes a decrementer interrupt, but it
can cause a system reset interrupt if the processor was in a power
saving mode. A data storage exception can cause a DSI or HDSI interrupt
depending on LPCR settings, and many other examples.
So I agree with Segher on this. We should use interrupt for interrupts,
reduce exception except where we really mean it, and move away from vec
and trap (I've got this wrong in the past too I admit). We don't have to
do it all immediately, but new code should go in this direction.
Thanks,
Nick
^ permalink raw reply
* [PATCH] powerpc/pseries: Add shutdown() to vio_driver and vio_bus
From: Tyrel Datwyler @ 2021-04-02 0:13 UTC (permalink / raw)
To: mpe; +Cc: linuxppc-dev, linux-kernel, Tyrel Datwyler
Currently, neither the vio_bus or vio_driver structures provide support
for a shutdown() routine.
Add support for shutdown() by allowing drivers to provide a
implementation via function pointer in their vio_driver struct and
provide a proper implementation in the driver template for the vio_bus
that calls a vio drivers shutdown() if defined.
In the case that no shutdown() is defined by a vio driver and a kexec is
in progress we implement a big hammer that calls remove() to ensure no
further DMA for the devices is possible.
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
arch/powerpc/include/asm/vio.h | 1 +
arch/powerpc/platforms/pseries/vio.c | 16 ++++++++++++++++
2 files changed, 17 insertions(+)
diff --git a/arch/powerpc/include/asm/vio.h b/arch/powerpc/include/asm/vio.h
index 721c0d6715ac..e7479a4abf96 100644
--- a/arch/powerpc/include/asm/vio.h
+++ b/arch/powerpc/include/asm/vio.h
@@ -114,6 +114,7 @@ struct vio_driver {
const struct vio_device_id *id_table;
int (*probe)(struct vio_dev *dev, const struct vio_device_id *id);
void (*remove)(struct vio_dev *dev);
+ void (*shutdown)(struct vio_dev *dev);
/* A driver must have a get_desired_dma() function to
* be loaded in a CMO environment if it uses DMA.
*/
diff --git a/arch/powerpc/platforms/pseries/vio.c b/arch/powerpc/platforms/pseries/vio.c
index 9cb4fc839fd5..d122b8644319 100644
--- a/arch/powerpc/platforms/pseries/vio.c
+++ b/arch/powerpc/platforms/pseries/vio.c
@@ -22,6 +22,7 @@
#include <linux/mm.h>
#include <linux/dma-map-ops.h>
#include <linux/kobject.h>
+#include <linux/kexec.h>
#include <asm/iommu.h>
#include <asm/dma.h>
@@ -1278,6 +1279,20 @@ static int vio_bus_remove(struct device *dev)
return 0;
}
+static void vio_bus_shutdown(struct device *dev)
+{
+ struct vio_dev *viodev = to_vio_dev(dev);
+ struct vio_driver *viodrv;
+
+ if (dev->driver) {
+ viodrv = to_vio_driver(dev->driver);
+ if (viodrv->shutdown)
+ viodrv->shutdown(viodev);
+ else if (kexec_in_progress)
+ vio_bus_remove(dev);
+ }
+}
+
/**
* vio_register_driver: - Register a new vio driver
* @viodrv: The vio_driver structure to be registered.
@@ -1613,6 +1628,7 @@ struct bus_type vio_bus_type = {
.match = vio_bus_match,
.probe = vio_bus_probe,
.remove = vio_bus_remove,
+ .shutdown = vio_bus_shutdown,
};
/**
--
2.27.0
^ permalink raw reply related
* Re: [PATCH v5 48/48] KVM: PPC: Book3S HV: remove ISA v3.0 and v3.1 support from P7/8 path
From: Nicholas Piggin @ 2021-04-02 0:11 UTC (permalink / raw)
To: kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <20210401150325.442125-49-npiggin@gmail.com>
Excerpts from Nicholas Piggin's message of April 2, 2021 1:03 am:
> POWER9 and later processors always go via the P9 guest entry path now.
> Remove the remaining support from the P7/8 path.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
[...]
> @@ -2527,28 +2259,14 @@ BEGIN_FTR_SECTION
> END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
>
> kvm_nap_sequence: /* desired LPCR value in r5 */
> -BEGIN_FTR_SECTION
> - /*
> - * PSSCR bits: exit criterion = 1 (wakeup based on LPCR at sreset)
> - * enable state loss = 1 (allow SMT mode switch)
> - * requested level = 0 (just stop dispatching)
> - */
> - lis r3, (PSSCR_EC | PSSCR_ESL)@h
> /* Set LPCR_PECE_HVEE bit to enable wakeup by HV interrupts */
> li r4, LPCR_PECE_HVEE@higher
> sldi r4, r4, 32
> or r5, r5, r4
> -FTR_SECTION_ELSE
> - li r3, PNV_THREAD_NAP
^^^^^^^
> -ALT_FTR_SECTION_END_IFSET(CPU_FTR_ARCH_300)
> mtspr SPRN_LPCR,r5
> isync
>
> -BEGIN_FTR_SECTION
> - bl isa300_idle_stop_mayloss
> -FTR_SECTION_ELSE
> bl isa206_idle_insn_mayloss
> -ALT_FTR_SECTION_END_IFSET(CPU_FTR_ARCH_300)
Got a bug or two in the old path because I didn't test SMT configs.
I'll work through those so for now don't spend too much time trying to
run the old path or going through the rmhandlers.S asm changes in the
series until I post the next round.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v5 42/48] KVM: PPC: Book3S HV: Radix guests should not have userspace hcalls reflected to them
From: Nicholas Piggin @ 2021-04-02 0:07 UTC (permalink / raw)
To: kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <20210401150325.442125-43-npiggin@gmail.com>
Excerpts from Nicholas Piggin's message of April 2, 2021 1:03 am:
> The reflection of sc 1 hcalls from PR=1 userspace is required to support
> PR KVM. Radix guests don't support PR KVM nor do they support nested
> hash guests, so this sc 1 reflection can be removed from radix guests.
> Cause a privileged program check instead, which is less surprising.
I'm thinking twice about where to put this patch. This is kind of
backwards (but also kind of not), so I decided instead to make
the change to not reflect on radix in the patch that removes real
mode hcall handlers from the P9 path.
And the patch around this part of the series will introduce reflection
for hash guest support in the P9 path.
End result is the same but I think that works better.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH 1/1] powerpc/smp: Set numa node before updating mask
From: Nathan Lynch @ 2021-04-01 22:51 UTC (permalink / raw)
To: Srikar Dronamraju
Cc: Gautham R Shenoy, Srikar Dronamraju, Peter Zijlstra,
Scott Cheloha, Geetika Moolchandani, Ingo Molnar, Laurent Dufour,
linuxppc-dev, Valentin Schneider
In-Reply-To: <20210401154200.150077-1-srikar@linux.vnet.ibm.com>
Hi Srikar,
Thanks for figuring this out.
Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
>
> Some of the per-CPU masks use cpu_cpu_mask as a filter to limit the search
> for related CPUs. On a dlpar add of a CPU, update cpu_cpu_mask before
> updating the per-CPU masks. This will ensure the cpu_cpu_mask is updated
> correctly before its used in setting the masks. Setting the numa_node will
> ensure that when cpu_cpu_mask() gets called, the correct node number is
> used. This code movement helped fix the above call trace.
>
>
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index 5a4d59a1070d..1a99d75679a8 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -1521,6 +1521,9 @@ void start_secondary(void *unused)
>
> vdso_getcpu_init();
> #endif
> + set_numa_node(numa_cpu_lookup_table[cpu]);
> + set_numa_mem(local_memory_node(numa_cpu_lookup_table[cpu]));
> +
> /* Update topology CPU masks */
> add_cpu_to_masks(cpu);
>
> @@ -1539,9 +1542,6 @@ void start_secondary(void *unused)
> shared_caches = true;
> }
>
> - set_numa_node(numa_cpu_lookup_table[cpu]);
> - set_numa_mem(local_memory_node(numa_cpu_lookup_table[cpu]));
> -
Regardless of your change: at boot time, this set of calls to
set_numa_node() and set_numa_mem() is redundant, right? Because
smp_prepare_cpus() has:
for_each_possible_cpu(cpu) {
...
if (cpu_present(cpu)) {
set_cpu_numa_node(cpu, numa_cpu_lookup_table[cpu]);
set_cpu_numa_mem(cpu,
local_memory_node(numa_cpu_lookup_table[cpu]));
}
I would rather that, when onlining a CPU that happens to have been
dynamically added after boot, we enter start_secondary() with conditions
equivalent to those at boot time. Or as close to that as is practical.
So I'd suggest that pseries_add_processor() be made to update
these things when the CPUs are marked present, before onlining them.
^ permalink raw reply
* Re: [PATCH v2 3/7] powerpc: convert config files to generic cmdline
From: Rob Herring @ 2021-04-01 20:08 UTC (permalink / raw)
To: Daniel Walker
Cc: Daniel Gimpelevich, linuxppc-dev, X86 ML, open list:MIPS,
linux-kernel@vger.kernel.org, Paul Mackerras, xe-linux-external,
Andrew Morton, Will Deacon
In-Reply-To: <20210330233137.GB2469518@zorba>
On Tue, Mar 30, 2021 at 6:31 PM Daniel Walker <danielwa@cisco.com> wrote:
>
> On Tue, Mar 30, 2021 at 03:13:04PM -0500, Rob Herring wrote:
> > On Tue, Mar 30, 2021 at 12:33 PM Daniel Walker <danielwa@cisco.com> wrote:
> > >
> > > On Thu, Mar 25, 2021 at 05:29:44PM -0600, Rob Herring wrote:
> > > > On Thu, Mar 25, 2021 at 2:00 PM Daniel Walker <danielwa@cisco.com> wrote:
> > > > >
> > > > > On Thu, Mar 25, 2021 at 01:03:55PM +0100, Christophe Leroy wrote:
> > > > > >
> > > > > > Ok, so you agree we don't need to provide two CMDLINE, one to be appended and one to be prepended.
> > > > > >
> > > > > > Let's only provide once CMDLINE as of today, and ask the user to select
> > > > > > whether he wants it appended or prepended or replacee. Then no need to
> > > > > > change all existing config to rename CONFIG_CMDLINE into either of the new
> > > > > > ones.
> > > > > >
> > > > > > That's the main difference between my series and Daniel's series. So I'll
> > > > > > finish taking Will's comment into account and we'll send out a v3 soon.
> > > > >
> > > > > It doesn't solve the needs of Cisco, I've stated many times your changes have
> > > > > little value. Please stop submitting them.
> > > >
> > > > Can you please outline what those needs are which aren't met?
> > >
> > > append AND prepend at the same time on all architectures. Christophe doesn't
> > > understand the need, and hence tries to minimize the feature set which is
> > > incompatible with Cisco needs and all the other out of tree users.
> >
> > Okay, but that's never been a feature in upstream. For upstream, we
> > refactor first and add features 2nd. In this case, the difference is
> > largely the kconfig and it would be better to not change the options
> > twice, but that's not a blocker for taking the refactoring. You won't
> > find a maintainer that's going to take adding a feature over cleanups
> > and unification.
>
> It kind of is a feature in upstream, it's a matter of opinion. Some platform
> used append and some use prepend, and it's likely because the maintainers needed
> one or the other for development.
Which arch/platform upstream does both prepend and append at the same time?
> I'm not sure why you think I can't add the features in one go. It would be
> horrid to take Christophe's changes, then have to do basically all the same work
> a second time which is what Christophe's changes would force me to do.
I didn't say it couldn't be done. In fact, I said it would be better
all at once: "it would be better to not change the options twice"
But both of you ignoring comments and continuing to post competing
series is not going to get us there. TBC, I think Christophe's series
is much closer to being in shape to merge upstream.
> Say for example I implement this change only on one architecture. In that case
> the maintainer would be accepting a feature enhancement , but there would be no
> stopping it. I shouldn't have to go two strokes on one architecture, but each
> change I'm making is essentially a single architecture. They can go in all
> together or one at a time.
Features do get implemented all the time on one arch. And then maybe a
2nd and 3rd. At some point we decide no more copying, it needs to be
common and refactored. We're at that point for cmdline handling IMO.
Rob
^ permalink raw reply
* Re: [PATCH v3 0/9] powerpc/xive: Map one IPI interrupt per node
From: Cédric Le Goater @ 2021-04-01 17:14 UTC (permalink / raw)
To: Greg Kurz; +Cc: linuxppc-dev
In-Reply-To: <20210401144501.2ee70421@bahia.lan>
On 4/1/21 2:45 PM, Greg Kurz wrote:
> On Thu, 1 Apr 2021 11:18:10 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
>
>> Hello,
>>
>> On 4/1/21 10:04 AM, Greg Kurz wrote:
>>> On Wed, 31 Mar 2021 16:45:05 +0200
>>> Cédric Le Goater <clg@kaod.org> wrote:
>>>
>>>>
>>>> Hello,
>>>>
>>>> ipistorm [*] can be used to benchmark the raw interrupt rate of an
>>>> interrupt controller by measuring the number of IPIs a system can
>>>> sustain. When applied to the XIVE interrupt controller of POWER9 and
>>>> POWER10 systems, a significant drop of the interrupt rate can be
>>>> observed when crossing the second node boundary.
>>>>
>>>> This is due to the fact that a single IPI interrupt is used for all
>>>> CPUs of the system. The structure is shared and the cache line updates
>>>> impact greatly the traffic between nodes and the overall IPI
>>>> performance.
>>>>
>>>> As a workaround, the impact can be reduced by deactivating the IRQ
>>>> lockup detector ("noirqdebug") which does a lot of accounting in the
>>>> Linux IRQ descriptor structure and is responsible for most of the
>>>> performance penalty.
>>>>
>>>> As a fix, this proposal allocates an IPI interrupt per node, to be
>>>> shared by all CPUs of that node. It solves the scaling issue, the IRQ
>>>> lockup detector still has an impact but the XIVE interrupt rate scales
>>>> linearly. It also improves the "noirqdebug" case as showed in the
>>>> tables below.
>>>>
>>>
>>> As explained by David and others, NUMA nodes happen to match sockets
>>> with current POWER CPUs but these are really different concepts. NUMA
>>> is about CPU memory accesses latency,
>>
>> This is exactly our problem. we have cache issues because hw threads
>> on different chips are trying to access the same structure in memory.
>> It happens on virtual platforms and baremetal platforms. This is not
>> restricted to pseries.
>>
>
> Ok, I get it... the XIVE HW accesses structures in RAM, just like HW threads
> do, so the closer, the better.
No. That's another problem related to the XIVE internal tables which
should be allocated on the chip where it is "mostly" used.
The problem is much simpler. As the commit log says :
This is due to the fact that a single IPI interrupt is used for all
CPUs of the system. The structure is shared and the cache line updates
impact greatly the traffic between nodes and the overall IPI
performance.
So, we have multiple threads competing for the same IRQ descriptor and
overloading the PowerBUS with cache update synchronization.
> This definitely looks NUMA related indeed. So
> yes, the idea of having the XIVE HW to only access local in-RAM data when
> handling IPIs between vCPUs in the same NUMA node makes sense.
yes. That's the goal.
> What is less clear is the exact role of ibm,chip-id actually. This is
> currently used on PowerNV only to pick up a default target on the same
> "chip" as the source if possible. What is the detailed motivation behind
> this ?
The "ibm,chip-id" issue is extra noise and not a requirement for this
patchset.
>>> while in the case of XIVE you
>>> really need to identify a XIVE chip localized in a given socket.
>>>
>>> PAPR doesn't know about sockets, only cores. In other words, a PAPR
>>> compliant guest sees all vCPUs like they all sit in a single socket.
>>
>> There are also NUMA nodes on PAPR.
>>
>
> Yes but nothing prevents a NUMA node to span over multiple sockets
> or having several NUMA nodes within the same socket, even if this
> isn't the case in practice with current POWER hardware.
yes. A NUMA node could even be a PCI adapter attached to storage.
I don't know what to say. We are missing a concept maybe.
>>> Same for the XIVE. Trying to introduce a concept of socket, either
>>> by hijacking OPAL's ibm,chip-id or NUMA node ids, is a kind of
>>> spec violation in this context. If the user cares for locality of
>>> the vCPUs and XIVE on the same socket, then it should bind vCPU
>>> threads to host CPUs from the same socket in the first place.
>>
>> Yes. that's a must have of course. You need to reflect the real HW
>> topology in the guest or LPAR if you are after performance, or
>> restrict the virtual machine to be on a single socket/chip/node.
>>
>> And this is not only a XIVE problem. XICS has the same problem with
>> a shared single IPI interrupt descriptor but XICS doesn't scale well
>> by design, so it doesn't show.
>>
>>
>>> Isn't this enough to solve the performance issues this series
>>> want to fix, without the need for virtual socket ids ?
>> what are virtual socket ids ? A new concept ?
>>
>
> For now, we have virtual CPUs identified by a virtual CPU id.
> It thus seems natural to speak of a virtual socket id, but
> anyway, the wording isn't really important here and you
> don't answer the question ;-)
if, on the hypervisor, you restrict the virtual machine vCPUs to be
on a single POWER processor/chip, there is no problem. But large
KVM guests or PowerVM LPARs do exist on 16s systems.
C.
^ permalink raw reply
* Re: [PATCH v2] powerpc/traps: Enhance readability for trap types
From: Segher Boessenkool @ 2021-04-01 16:16 UTC (permalink / raw)
To: Nicholas Piggin
Cc: ravi.bangoria, aik, peterx, oleg, paulus, kan.liang, leobras.c,
mikey, maddy, haren, peterz, kjain, msuchanek, pmladek,
john.ogness, alistair, kvm-ppc, jniethe5, atrajeev, Xiongwei Song,
Xiongwei Song, linux-kernel, aneesh.kumar, akpm, linuxppc-dev,
rppt
In-Reply-To: <1617262858.ls37f2d81f.astroid@bobo.none>
On Thu, Apr 01, 2021 at 06:01:29PM +1000, Nicholas Piggin wrote:
> Excerpts from Michael Ellerman's message of April 1, 2021 12:39 pm:
> > Segher Boessenkool <segher@kernel.crashing.org> writes:
> >> On Wed, Mar 31, 2021 at 08:58:17PM +1100, Michael Ellerman wrote:
> >>> So perhaps:
> >>>
> >>> EXC_SYSTEM_RESET
> >>> EXC_MACHINE_CHECK
> >>> EXC_DATA_STORAGE
> >>> EXC_DATA_SEGMENT
> >>> EXC_INST_STORAGE
> >>> EXC_INST_SEGMENT
> >>> EXC_EXTERNAL_INTERRUPT
> >>> EXC_ALIGNMENT
> >>> EXC_PROGRAM_CHECK
> >>> EXC_FP_UNAVAILABLE
> >>> EXC_DECREMENTER
> >>> EXC_HV_DECREMENTER
> >>> EXC_SYSTEM_CALL
> >>> EXC_HV_DATA_STORAGE
> >>> EXC_PERF_MONITOR
> >>
> >> These are interrupt (vectors), not exceptions. It doesn't matter all
> >> that much, but confusing things more isn't useful either! There can be
> >> multiple exceptions that all can trigger the same interrupt.
> >
> > Yeah I know, but I think that ship has already sailed as far as the
> > naming we have in the kernel.
>
> It has, but there are also several other ships also sailing in different
> directions. It could be worse though, at least they are not sideways in
> the Suez.
:-)
> > We have over 250 uses of "exc", and several files called "exception"
> > something.
> >
> > Using "interrupt" can also be confusing because Linux uses that to mean
> > "external interrupt".
> >
> > But I dunno, maybe INT or VEC is clearer? .. or TRAP :)
>
> We actually already have defines that follow Segher's suggestion, it's
> just that they're hidden away in a KVM header.
>
> #define BOOK3S_INTERRUPT_SYSTEM_RESET 0x100
> #define BOOK3S_INTERRUPT_MACHINE_CHECK 0x200
> #define BOOK3S_INTERRUPT_DATA_STORAGE 0x300
> #define BOOK3S_INTERRUPT_DATA_SEGMENT 0x380
> #define BOOK3S_INTERRUPT_INST_STORAGE 0x400
> #define BOOK3S_INTERRUPT_INST_SEGMENT 0x480
> #define BOOK3S_INTERRUPT_EXTERNAL 0x500
> #define BOOK3S_INTERRUPT_EXTERNAL_HV 0x502
> #define BOOK3S_INTERRUPT_ALIGNMENT 0x600
>
> It would take just a small amount of work to move these to general
> powerpc header, add #ifdefs for Book E/S where the numbers differ,
> and remove the BOOK3S_ prefix.
>
> I don't mind INTERRUPT_ but INT_ would be okay too. VEC_ actually
> doesn't match what Book E does (which is some weirdness to map some
> of them to match Book S but not all, arguably we should clean that
> up too and just use vector numbers consistently, but the INTERRUPT_
> prefix would still be valid if we did that).
VEC also is pretty incorrect: there is code at those addresses, not
vectors pointing to code (as similar things on some other architectures
have). Everyone understands what it means of course, except it is
confusing with a thing we *do* have on Power called VEC (the MSR bit) :-P
(And TRAP is just one cause of 700...)
Segher
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox