* Re: [PATCH] powerpc/ptdump: Fix display a BAT's size unit
From: Michael Ellerman @ 2021-12-07 13:27 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Paul Mackerras,
Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <f16c30f5c9185a63335322cf1a8b22f189d335ef.1637922595.git.christophe.leroy@csgroup.eu>
On Fri, 26 Nov 2021 11:30:03 +0100, Christophe Leroy wrote:
> We have wrong units on BAT's sizes (G instead of M, M instead of ...)
>
> ---[ Instruction Block Address Translation ]---
> 0: 0xc0000000-0xc03fffff 0x00000000 4G Kernel x m
> 1: 0xc0400000-0xc05fffff 0x00400000 2G Kernel x m
> 2: 0xc0600000-0xc06fffff 0x00600000 1G Kernel x m
> 3: 0xc0700000-0xc077ffff 0x00700000 512M Kernel x m
> 4: 0xc0780000-0xc079ffff 0x00780000 128M Kernel x m
> 5: 0xc07a0000-0xc07bffff 0x007a0000 128M Kernel x m
> 6: -
> 7: -
>
> [...]
Applied to powerpc/next.
[1/1] powerpc/ptdump: Fix display a BAT's size unit
https://git.kernel.org/powerpc/c/cdc81aece8041fd5437bdabde6c543cdeb2891a8
cheers
^ permalink raw reply
* Re: [PATCH v1 0/5] Implement livepatch on PPC32
From: Michael Ellerman @ 2021-12-07 13:26 UTC (permalink / raw)
To: Steven Rostedt, Joe Lawrence, Miroslav Benes, Josh Poimboeuf,
Petr Mladek, Ingo Molnar, Naveen N . Rao, Jiri Kosina,
Christophe Leroy
Cc: live-patching, linuxppc-dev, linux-kernel
In-Reply-To: <cover.1635423081.git.christophe.leroy@csgroup.eu>
On Thu, 28 Oct 2021 14:24:00 +0200, Christophe Leroy wrote:
> This series implements livepatch on PPC32.
>
> This is largely copied from what's done on PPC64.
>
> Christophe Leroy (5):
> livepatch: Fix build failure on 32 bits processors
> powerpc/ftrace: No need to read LR from stack in _mcount()
> powerpc/ftrace: Add module_trampoline_target() for PPC32
> powerpc/ftrace: Activate HAVE_DYNAMIC_FTRACE_WITH_REGS on PPC32
> powerpc/ftrace: Add support for livepatch to PPC32
>
> [...]
Patches 2-4 applied to powerpc/next.
[2/5] powerpc/ftrace: No need to read LR from stack in _mcount()
https://git.kernel.org/powerpc/c/88670fdb26800228606c078ba4a018e9522a75a8
[3/5] powerpc/ftrace: Add module_trampoline_target() for PPC32
https://git.kernel.org/powerpc/c/c93d4f6ecf4b0699d0f2088f7bd9cd09af45d65a
[4/5] powerpc/ftrace: Activate HAVE_DYNAMIC_FTRACE_WITH_REGS on PPC32
https://git.kernel.org/powerpc/c/7dfbfb87c243cf08bc2b9cc23699ac207b726458
cheers
^ permalink raw reply
* Re: [PATCH v5 1/3] powerpc/bitops: Use immediate operand when possible
From: Michael Ellerman @ 2021-12-07 13:27 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Paul Mackerras,
Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <e6f815d9181bab09df3b350af51149437863e9f9.1632236981.git.christophe.leroy@csgroup.eu>
On Tue, 21 Sep 2021 17:09:47 +0200, Christophe Leroy wrote:
> Today we get the following code generation for bitops like
> set or clear bit:
>
> c0009fe0: 39 40 08 00 li r10,2048
> c0009fe4: 7c e0 40 28 lwarx r7,0,r8
> c0009fe8: 7c e7 53 78 or r7,r7,r10
> c0009fec: 7c e0 41 2d stwcx. r7,0,r8
>
> [...]
Applied to powerpc/next.
[1/3] powerpc/bitops: Use immediate operand when possible
https://git.kernel.org/powerpc/c/fb350784d8d17952afa93383bb47aaa6b715c459
[2/3] powerpc/atomics: Use immediate operand when possible
https://git.kernel.org/powerpc/c/41d65207de9fbff58acd8937a7c3f8940c186a87
[3/3] powerpc/atomics: Remove atomic_inc()/atomic_dec() and friends
https://git.kernel.org/powerpc/c/f05cab0034babaa9b3dfaf6003ee6493496a8180
cheers
^ permalink raw reply
* Re: [PATCH] powerpc/xive: Fix compile when !CONFIG_PPC_POWERNV.
From: Michael Ellerman @ 2021-12-07 13:27 UTC (permalink / raw)
To: Cédric Le Goater, linuxppc-dev; +Cc: kernel test robot
In-Reply-To: <20211201165418.1041842-1-clg@kaod.org>
On Wed, 1 Dec 2021 17:54:18 +0100, Cédric Le Goater wrote:
> The automatic "save & restore" of interrupt context is a POWER10/XIVE2
> feature exploited by KVM under the PowerNV platform. It is not
> available under pSeries and the associated toggle should not be
> exposed under the XIVE debugfs directory.
>
> Introduce a platform handler for debugfs initialization and move the
> 'save-restore' entry under the native (PowerNV) backend to fix compile
> when !CONFIG_PPC_POWERNV.
>
> [...]
Applied to powerpc/next.
[1/1] powerpc/xive: Fix compile when !CONFIG_PPC_POWERNV.
https://git.kernel.org/powerpc/c/2a2ac8a7018b953cd23d770ebd28f8e1ea365df4
cheers
^ permalink raw reply
* Re: [PATCH] powerpc/signal32: Use struct_group() to zero spe regs
From: Michael Ellerman @ 2021-12-07 13:27 UTC (permalink / raw)
To: Michael Ellerman, Kees Cook
Cc: kernel test robot, Peter Zijlstra, Aneesh Kumar K.V, linux-kernel,
Nicholas Piggin, Paul Mackerras, linux-hardening, Sudeep Holla,
linuxppc-dev, Eric W. Biederman
In-Reply-To: <20211118203604.1288379-1-keescook@chromium.org>
On Thu, 18 Nov 2021 12:36:04 -0800, Kees Cook wrote:
> In preparation for FORTIFY_SOURCE performing compile-time and run-time
> field bounds checking for memset(), avoid intentionally writing across
> neighboring fields.
>
> Add a struct_group() for the spe registers so that memset() can correctly reason
> about the size:
>
> [...]
Applied to powerpc/next.
[1/1] powerpc/signal32: Use struct_group() to zero spe regs
https://git.kernel.org/powerpc/c/62ea67e31981bca95ec16c37e2a1fba68f3dd8c5
cheers
^ permalink raw reply
* Re: [PATCH 1/6] powerpc/85xx: Fix no previous prototype warning for mpc85xx_setup_pmc()
From: Michael Ellerman @ 2021-12-07 13:26 UTC (permalink / raw)
To: linuxppc-dev, Michael Ellerman
In-Reply-To: <20211124093254.1054750-1-mpe@ellerman.id.au>
On Wed, 24 Nov 2021 20:32:49 +1100, Michael Ellerman wrote:
> Fixes the following W=1 warning:
> arch/powerpc/platforms/85xx/mpc85xx_pm_ops.c:89:12: warning: no previous prototype for 'mpc85xx_setup_pmc'
>
>
Applied to powerpc/next.
[1/6] powerpc/85xx: Fix no previous prototype warning for mpc85xx_setup_pmc()
https://git.kernel.org/powerpc/c/4ea9e321c27fd531a8dfe0fa1d1b2ee15fc3444e
[2/6] powerpc/85xx: Make mpc85xx_smp_kexec_cpu_down() static
https://git.kernel.org/powerpc/c/84a61fb43fdfc528a3a7ff00e0b14ba91f5eb745
[3/6] powerpc/85xx: Make c293_pcie_pic_init() static
https://git.kernel.org/powerpc/c/d9150d5bb5586dc20b6c424e59d5ea29fe1b3030
[4/6] powerpc/mm: Move tlbcam_sz() and make it static
https://git.kernel.org/powerpc/c/ff47a95d1a67477e9bc2049a840d93b68508e079
[5/6] powerpc/smp: Move setup_profiling_timer() under CONFIG_PROFILING
https://git.kernel.org/powerpc/c/a4ac0d249a5db80e79d573db9e4ad29354b643a8
[6/6] powerpc: Mark probe_machine() __init and static
https://git.kernel.org/powerpc/c/ab85a273957eadfcf7906bcd8a0adf5909d802ee
cheers
^ permalink raw reply
* Re: [PATCH 1/2] powerpc: remove cpu_online_cores_map function
From: Michael Ellerman @ 2021-12-07 13:26 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev
In-Reply-To: <20211105035042.1398309-1-npiggin@gmail.com>
On Fri, 5 Nov 2021 13:50:41 +1000, Nicholas Piggin wrote:
> This function builds the cores online map with on-stack cpumasks which
> can cause high stack usage with large NR_CPUS.
>
> It is not used in any performance sensitive paths, so instead just check
> for first thread sibling.
>
>
> [...]
Applied to powerpc/next.
[1/2] powerpc: remove cpu_online_cores_map function
https://git.kernel.org/powerpc/c/b350111bf7b3f4a780d28c44f18f7c9fcbe6d11b
[2/2] powerpc: select CPUMASK_OFFSTACK if NR_CPUS >= 8192
https://git.kernel.org/powerpc/c/2eafc4748bc08c5b9b6ee0b5b65ad20b30f7d704
cheers
^ permalink raw reply
* Re: [PATCH v2] powerpc: flexible GPR range save/restore macros
From: Michael Ellerman @ 2021-12-07 13:26 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev
In-Reply-To: <20211022061322.2671178-1-npiggin@gmail.com>
On Fri, 22 Oct 2021 16:13:22 +1000, Nicholas Piggin wrote:
> Introduce macros that operate on a (start, end) range of GPRs, which
> reduces lines of code and need to do mental arithmetic while reading the
> code.
>
>
Applied to powerpc/next.
[1/1] powerpc: flexible GPR range save/restore macros
https://git.kernel.org/powerpc/c/aebd1fb45c622e9a2b06fb70665d084d3a8d6c78
cheers
^ permalink raw reply
* Re: [PATCH v3] powerpc/64s: Get LPID bit width from device tree
From: Michael Ellerman @ 2021-12-07 13:27 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev; +Cc: Fabiano Rosas
In-Reply-To: <20211129030915.1888332-1-npiggin@gmail.com>
On Mon, 29 Nov 2021 13:09:15 +1000, Nicholas Piggin wrote:
> Allow the LPID bit width and partition table size to be set at runtime
> from the device tree.
>
> Move the PID bit width detection into the same place.
>
> KVM does not support using the extra bits yet, this is mainly required
> to get the PTCR register values correct (so KVM will run but it will
> not allocate > 4096 LPIDs).
>
> [...]
Applied to powerpc/next.
[1/1] powerpc/64s: Get LPID bit width from device tree
https://git.kernel.org/powerpc/c/5402e239d09feea482d25d60df9b908cfaf9ec3c
cheers
^ permalink raw reply
* Re: (subset) [PATCH v4 0/5] powerpc: watchdog fixes
From: Michael Ellerman @ 2021-12-07 13:26 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev; +Cc: Laurent Dufour, Daniel Axtens
In-Reply-To: <20211119113146.752759-1-npiggin@gmail.com>
On Fri, 19 Nov 2021 21:31:41 +1000, Nicholas Piggin wrote:
> These are some watchdog fixes and improvements, in particular a
> deadlock between the wd_smp_lock and console lock when the watchdog
> fires, found by Laurent.
>
> Thanks,
> Nick
>
> [...]
Patch 5 applied to powerpc/next.
[5/5] powerpc/watchdog: help remote CPUs to flush NMI printk output
https://git.kernel.org/powerpc/c/e012c499985c608c936410d8bab29d9596d62859
cheers
^ permalink raw reply
* Re: [PATCH v3] powerpc/pseries: read the lpar name from the firmware
From: Nathan Lynch @ 2021-12-07 14:32 UTC (permalink / raw)
To: Laurent Dufour; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <20211203154321.13168-1-ldufour@linux.ibm.com>
Hi Laurent,
Laurent Dufour <ldufour@linux.ibm.com> writes:
> +/*
> + * PAPR defines, in section "7.3.16 System Parameters Option", the token 55 to
> + * read the LPAR name.
> + */
> +#define SPLPAR_LPAR_NAME_TOKEN 55
> +static void read_lpar_name(struct seq_file *m)
> +{
> + int rc, len, token;
> + union {
> + char raw_buffer[RTAS_DATA_BUF_SIZE];
> + struct {
> + __be16 len;
This:
> + char name[RTAS_DATA_BUF_SIZE-2];
^^^^^^
should be 4000, not (4K - 2), according to PAPR (it's weird and I don't
know the reason).
> + };
> + } *local_buffer;
> +
> + token = rtas_token("ibm,get-system-parameter");
> + if (token == RTAS_UNKNOWN_SERVICE)
> + return;
> +
> + local_buffer = kmalloc(sizeof(*local_buffer), GFP_KERNEL);
> + if (!local_buffer)
> + return;
> +
> + do {
> + spin_lock(&rtas_data_buf_lock);
> + memset(rtas_data_buf, 0, RTAS_DATA_BUF_SIZE);
> + rc = rtas_call(token, 3, 1, NULL, SPLPAR_LPAR_NAME_TOKEN,
> + __pa(rtas_data_buf), RTAS_DATA_BUF_SIZE);
> + if (!rc)
> + memcpy(local_buffer->raw_buffer, rtas_data_buf,
> + RTAS_DATA_BUF_SIZE);
> + spin_unlock(&rtas_data_buf_lock);
> + } while (rtas_busy_delay(rc));
> +
> + if (rc != 0) {
> + pr_err_once(
> + "%s %s Error calling get-system-parameter (0x%x)\n",
> + __FILE__, __func__, rc);
The __FILE__ and __func__ in the message seem unnecessary, and rc should
be reported in decimal so the error meaning is apparent.
Is there a reasonable fallback for VMs where this parameter doesn't
exist? PowerVM partitions should always have it, but what do we want the
behavior to be on other hypervisors?
> + } else {
> + /* Force end of string */
> + len = be16_to_cpu(local_buffer->len);
> + if (len >= (RTAS_DATA_BUF_SIZE-2))
> + len = RTAS_DATA_BUF_SIZE-2;
Could use min() or clamp(), and it would be better to build the
expression using the value of sizeof(local_buffer->name).
> + local_buffer->name[len] = '\0';
If 'len' can be (RTAS_DATA_BUF_SIZE - 2), then this writes past the end
of the buffer, no?
^ permalink raw reply
* Re: [PATCH] powerpc/module_64: Fix livepatching for RO modules
From: Joe Lawrence @ 2021-12-07 14:44 UTC (permalink / raw)
To: Russell Currey, linuxppc-dev; +Cc: live-patching, jniethe5, naveen.n.rao
In-Reply-To: <20211123081520.18843-1-ruscur@russell.cc>
On 11/23/21 3:15 AM, Russell Currey wrote:
> Livepatching a loaded module involves applying relocations through
> apply_relocate_add(), which attempts to write to read-only memory when
> CONFIG_STRICT_MODULE_RWX=y. Work around this by performing these
> writes through the text poke area by using patch_instruction().
>
> R_PPC_REL24 is the only relocation type generated by the kpatch-build
> userspace tool or klp-convert kernel tree that I observed applying a
> relocation to a post-init module.
>
> A more comprehensive solution is planned, but using patch_instruction()
> for R_PPC_REL24 on should serve as a sufficient fix.
>
> This does have a performance impact, I observed ~15% overhead in
> module_load() on POWER8 bare metal with checksum verification off.
>
> Fixes: c35717c71e98 ("powerpc: Set ARCH_HAS_STRICT_MODULE_RWX")
> Cc: stable@vger.kernel.org # v5.14+
> Reported-by: Joe Lawrence <joe.lawrence@redhat.com>
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> ---
> Intended to be a minimal fix that can go to stable.
>
> arch/powerpc/kernel/module_64.c | 30 ++++++++++++++++++++++--------
> 1 file changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
> index 6baa676e7cb6..c25ef36c3ef4 100644
> --- a/arch/powerpc/kernel/module_64.c
> +++ b/arch/powerpc/kernel/module_64.c
> @@ -422,11 +422,16 @@ static inline int create_stub(const Elf64_Shdr *sechdrs,
> const char *name)
> {
> long reladdr;
> + func_desc_t desc;
> + int i;
>
> if (is_mprofile_ftrace_call(name))
> return create_ftrace_stub(entry, addr, me);
>
> - memcpy(entry->jump, ppc64_stub_insns, sizeof(ppc64_stub_insns));
> + for (i = 0; i < sizeof(ppc64_stub_insns) / sizeof(u32); i++) {
> + patch_instruction(&entry->jump[i],
> + ppc_inst(ppc64_stub_insns[i]));
> + }
>
> /* Stub uses address relative to r2. */
> reladdr = (unsigned long)entry - my_r2(sechdrs, me);
> @@ -437,10 +442,19 @@ static inline int create_stub(const Elf64_Shdr *sechdrs,
> }
> pr_debug("Stub %p get data from reladdr %li\n", entry, reladdr);
>
> - entry->jump[0] |= PPC_HA(reladdr);
> - entry->jump[1] |= PPC_LO(reladdr);
> - entry->funcdata = func_desc(addr);
> - entry->magic = STUB_MAGIC;
> + patch_instruction(&entry->jump[0],
> + ppc_inst(entry->jump[0] | PPC_HA(reladdr)));
> + patch_instruction(&entry->jump[1],
> + ppc_inst(entry->jump[1] | PPC_LO(reladdr)));
> +
> + // func_desc_t is 8 bytes if ABIv2, else 16 bytes
> + desc = func_desc(addr);
> + for (i = 0; i < sizeof(func_desc_t) / sizeof(u32); i++) {
> + patch_instruction(((u32 *)&entry->funcdata) + i,
> + ppc_inst(((u32 *)(&desc))[i]));
> + }
> +
> + patch_instruction(&entry->magic, ppc_inst(STUB_MAGIC));
>
> return 1;
> }
> @@ -496,7 +510,7 @@ static int restore_r2(const char *name, u32 *instruction, struct module *me)
> return 0;
> }
> /* ld r2,R2_STACK_OFFSET(r1) */
> - *instruction = PPC_INST_LD_TOC;
> + patch_instruction(instruction, ppc_inst(PPC_INST_LD_TOC));
> return 1;
> }
>
> @@ -636,9 +650,9 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> }
>
> /* Only replace bits 2 through 26 */
> - *(uint32_t *)location
> - = (*(uint32_t *)location & ~0x03fffffc)
> + value = (*(uint32_t *)location & ~0x03fffffc)
> | (value & 0x03fffffc);
> + patch_instruction((u32 *)location, ppc_inst(value));
> break;
>
> case R_PPC64_REL64:
>
[[ cc += livepatching list ]]
Hi Russell,
Thanks for writing a minimal fix for stable / backporting. As I
mentioned on the github issue [1], this avoided the crashes I reported
here and over on kpatch github [2]. I wasn't sure if this is the final
version for stable, but feel free to add my:
Tested-by: Joe Lawrence <joe.lawrence@redhat.com>
[1] https://github.com/linuxppc/issues/issues/375
[2] https://github.com/dynup/kpatch/issues/1228
--
Joe
^ permalink raw reply
* Re: [PATCH V2 1/2] tools/perf: Include global and local variants for p_stage_cyc sort key
From: Arnaldo Carvalho de Melo @ 2021-12-07 14:52 UTC (permalink / raw)
To: Athira Rajeev
Cc: maddy, rnsastry, linux-perf-users, jolsa, kjain, namhyung,
linuxppc-dev
In-Reply-To: <20211203022038.48240-1-atrajeev@linux.vnet.ibm.com>
Em Fri, Dec 03, 2021 at 07:50:37AM +0530, Athira Rajeev escreveu:
> Sort key p_stage_cyc is used to present the latency
> cycles spend in pipeline stages. perf tool has local
> p_stage_cyc sort key to display this info. There is no
> global variant available for this sort key. local variant
> shows latency in a sinlge sample, whereas, global value
> will be useful to present the total latency (sum of
> latencies) in the hist entry. It represents latency
> number multiplied by the number of samples.
>
> Add global (p_stage_cyc) and local variant
> (local_p_stage_cyc) for this sort key. Use the
> local_p_stage_cyc as default option for "mem" sort mode.
> Also add this to list of dynamic sort keys and made the
> "dynamic_headers" and "arch_specific_sort_keys" as static.
>
> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> Reported-by: Namhyung Kim <namhyung@kernel.org>
I got this for v1, does it stand for v2?
Tested-by: Nageswara R Sastry <rnsastry@linux.ibm.com>
> ---
> Changelog:
> v1 -> v2:
> Addressed review comments from Jiri by making the
> "dynamic_headers" and "arch_specific_sort_keys"
> as static.
>
> tools/perf/util/hist.c | 4 +++-
> tools/perf/util/hist.h | 3 ++-
> tools/perf/util/sort.c | 34 +++++++++++++++++++++++++---------
> tools/perf/util/sort.h | 3 ++-
> 4 files changed, 32 insertions(+), 12 deletions(-)
>
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index b776465e04ef..0a8033b09e28 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -211,7 +211,9 @@ void hists__calc_col_len(struct hists *hists, struct hist_entry *h)
> hists__new_col_len(hists, HISTC_MEM_BLOCKED, 10);
> hists__new_col_len(hists, HISTC_LOCAL_INS_LAT, 13);
> hists__new_col_len(hists, HISTC_GLOBAL_INS_LAT, 13);
> - hists__new_col_len(hists, HISTC_P_STAGE_CYC, 13);
> + hists__new_col_len(hists, HISTC_LOCAL_P_STAGE_CYC, 13);
> + hists__new_col_len(hists, HISTC_GLOBAL_P_STAGE_CYC, 13);
> +
> if (symbol_conf.nanosecs)
> hists__new_col_len(hists, HISTC_TIME, 16);
> else
> diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
> index 621f35ae1efa..2a15e22fb89c 100644
> --- a/tools/perf/util/hist.h
> +++ b/tools/perf/util/hist.h
> @@ -75,7 +75,8 @@ enum hist_column {
> HISTC_MEM_BLOCKED,
> HISTC_LOCAL_INS_LAT,
> HISTC_GLOBAL_INS_LAT,
> - HISTC_P_STAGE_CYC,
> + HISTC_LOCAL_P_STAGE_CYC,
> + HISTC_GLOBAL_P_STAGE_CYC,
> HISTC_NR_COLS, /* Last entry */
> };
>
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index a111065b484e..e417e47f51b9 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -37,7 +37,7 @@ const char default_parent_pattern[] = "^sys_|^do_page_fault";
> const char *parent_pattern = default_parent_pattern;
> const char *default_sort_order = "comm,dso,symbol";
> const char default_branch_sort_order[] = "comm,dso_from,symbol_from,symbol_to,cycles";
> -const char default_mem_sort_order[] = "local_weight,mem,sym,dso,symbol_daddr,dso_daddr,snoop,tlb,locked,blocked,local_ins_lat,p_stage_cyc";
> +const char default_mem_sort_order[] = "local_weight,mem,sym,dso,symbol_daddr,dso_daddr,snoop,tlb,locked,blocked,local_ins_lat,local_p_stage_cyc";
> const char default_top_sort_order[] = "dso,symbol";
> const char default_diff_sort_order[] = "dso,symbol";
> const char default_tracepoint_sort_order[] = "trace";
> @@ -46,8 +46,8 @@ const char *field_order;
> regex_t ignore_callees_regex;
> int have_ignore_callees = 0;
> enum sort_mode sort__mode = SORT_MODE__NORMAL;
> -const char *dynamic_headers[] = {"local_ins_lat", "p_stage_cyc"};
> -const char *arch_specific_sort_keys[] = {"p_stage_cyc"};
> +static const char *const dynamic_headers[] = {"local_ins_lat", "ins_lat", "local_p_stage_cyc", "p_stage_cyc"};
> +static const char *const arch_specific_sort_keys[] = {"local_p_stage_cyc", "p_stage_cyc"};
>
> /*
> * Replaces all occurrences of a char used with the:
> @@ -1392,22 +1392,37 @@ struct sort_entry sort_global_ins_lat = {
> };
>
> static int64_t
> -sort__global_p_stage_cyc_cmp(struct hist_entry *left, struct hist_entry *right)
> +sort__p_stage_cyc_cmp(struct hist_entry *left, struct hist_entry *right)
> {
> return left->p_stage_cyc - right->p_stage_cyc;
> }
>
> +static int hist_entry__global_p_stage_cyc_snprintf(struct hist_entry *he, char *bf,
> + size_t size, unsigned int width)
> +{
> + return repsep_snprintf(bf, size, "%-*u", width,
> + he->p_stage_cyc * he->stat.nr_events);
> +}
> +
> +
> static int hist_entry__p_stage_cyc_snprintf(struct hist_entry *he, char *bf,
> size_t size, unsigned int width)
> {
> return repsep_snprintf(bf, size, "%-*u", width, he->p_stage_cyc);
> }
>
> -struct sort_entry sort_p_stage_cyc = {
> - .se_header = "Pipeline Stage Cycle",
> - .se_cmp = sort__global_p_stage_cyc_cmp,
> +struct sort_entry sort_local_p_stage_cyc = {
> + .se_header = "Local Pipeline Stage Cycle",
> + .se_cmp = sort__p_stage_cyc_cmp,
> .se_snprintf = hist_entry__p_stage_cyc_snprintf,
> - .se_width_idx = HISTC_P_STAGE_CYC,
> + .se_width_idx = HISTC_LOCAL_P_STAGE_CYC,
> +};
> +
> +struct sort_entry sort_global_p_stage_cyc = {
> + .se_header = "Pipeline Stage Cycle",
> + .se_cmp = sort__p_stage_cyc_cmp,
> + .se_snprintf = hist_entry__global_p_stage_cyc_snprintf,
> + .se_width_idx = HISTC_GLOBAL_P_STAGE_CYC,
> };
>
> struct sort_entry sort_mem_daddr_sym = {
> @@ -1858,7 +1873,8 @@ static struct sort_dimension common_sort_dimensions[] = {
> DIM(SORT_CODE_PAGE_SIZE, "code_page_size", sort_code_page_size),
> DIM(SORT_LOCAL_INS_LAT, "local_ins_lat", sort_local_ins_lat),
> DIM(SORT_GLOBAL_INS_LAT, "ins_lat", sort_global_ins_lat),
> - DIM(SORT_PIPELINE_STAGE_CYC, "p_stage_cyc", sort_p_stage_cyc),
> + DIM(SORT_LOCAL_PIPELINE_STAGE_CYC, "local_p_stage_cyc", sort_local_p_stage_cyc),
> + DIM(SORT_GLOBAL_PIPELINE_STAGE_CYC, "p_stage_cyc", sort_global_p_stage_cyc),
> };
>
> #undef DIM
> diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
> index 7b7145501933..f994261888e1 100644
> --- a/tools/perf/util/sort.h
> +++ b/tools/perf/util/sort.h
> @@ -235,7 +235,8 @@ enum sort_type {
> SORT_CODE_PAGE_SIZE,
> SORT_LOCAL_INS_LAT,
> SORT_GLOBAL_INS_LAT,
> - SORT_PIPELINE_STAGE_CYC,
> + SORT_LOCAL_PIPELINE_STAGE_CYC,
> + SORT_GLOBAL_PIPELINE_STAGE_CYC,
>
> /* branch stack specific sort keys */
> __SORT_BRANCH_STACK,
> --
> 2.33.0
--
- Arnaldo
^ permalink raw reply
* Re: [patch V2 01/23] powerpc/4xx: Remove MSI support which never worked
From: Cédric Le Goater @ 2021-12-07 15:50 UTC (permalink / raw)
To: Michael Ellerman, Thomas Gleixner, LKML
Cc: linux-hyperv, Paul Mackerras, sparclinux, Wei Liu, Ashok Raj,
Marc Zygnier, x86, Christian Borntraeger, Bjorn Helgaas,
Megha Dey, Jason Gunthorpe, linux-pci, xen-devel, ath11k,
Kevin Tian, Heiko Carstens, Alex Williamson, Kalle Valo,
Juergen Gross, Thomas Bogendoerfer, Greg Kroah-Hartman,
linux-mips, linuxppc-dev
In-Reply-To: <87ilw0odel.fsf@mpe.ellerman.id.au>
On 12/7/21 12:36, Michael Ellerman wrote:
> Cédric Le Goater <clg@kaod.org> writes:
>> Hello Thomas,
>>
>> On 12/6/21 23:27, Thomas Gleixner wrote:
>>> This code is broken since day one. ppc4xx_setup_msi_irqs() has the
>>> following gems:
>>>
>>> 1) The handling of the result of msi_bitmap_alloc_hwirqs() is completely
>>> broken:
>>>
>>> When the result is greater than or equal 0 (bitmap allocation
>>> successful) then the loop terminates and the function returns 0
>>> (success) despite not having installed an interrupt.
>>>
>>> When the result is less than 0 (bitmap allocation fails), it prints an
>>> error message and continues to "work" with that error code which would
>>> eventually end up in the MSI message data.
>>>
>>> 2) On every invocation the file global pp4xx_msi::msi_virqs bitmap is
>>> allocated thereby leaking the previous one.
>>>
>>> IOW, this has never worked and for more than 10 years nobody cared. Remove
>>> the gunk.
>>>
>>> Fixes: 3fb7933850fa ("powerpc/4xx: Adding PCIe MSI support")
>>
>> Shouldn't we remove all of it ? including the updates in the device trees
>> and the Kconfig changes under :
>>
>> arch/powerpc/platforms/44x/Kconfig: select PPC4xx_MSI
>> arch/powerpc/platforms/44x/Kconfig: select PPC4xx_MSI
>> arch/powerpc/platforms/44x/Kconfig: select PPC4xx_MSI
>> arch/powerpc/platforms/44x/Kconfig: select PPC4xx_MSI
>> arch/powerpc/platforms/40x/Kconfig: select PPC4xx_MSI
>
> This patch should drop those selects I guess. Can you send an
> incremental diff for Thomas to squash in?
Sure.
> Removing all the tendrils in various device tree files will probably
> require some archaeology, and it should be perfectly safe to leave those
> in the tree with the driver gone. So I think we can do that as a
> subsequent patch, rather than in this series.
Here are the changes. Compiled tested with ppc40x and ppc44x defconfigs.
Thanks,
C.
diff --git a/arch/powerpc/boot/dts/bluestone.dts b/arch/powerpc/boot/dts/bluestone.dts
index aa1ae94cd776..6971595319c1 100644
--- a/arch/powerpc/boot/dts/bluestone.dts
+++ b/arch/powerpc/boot/dts/bluestone.dts
@@ -366,30 +366,5 @@ PCIE0: pcie@d00000000 {
0x0 0x0 0x0 0x3 &UIC3 0xe 0x4 /* swizzled int C */
0x0 0x0 0x0 0x4 &UIC3 0xf 0x4 /* swizzled int D */>;
};
-
- MSI: ppc4xx-msi@C10000000 {
- compatible = "amcc,ppc4xx-msi", "ppc4xx-msi";
- reg = < 0xC 0x10000000 0x100
- 0xC 0x10000000 0x100>;
- sdr-base = <0x36C>;
- msi-data = <0x00004440>;
- msi-mask = <0x0000ffe0>;
- interrupts =<0 1 2 3 4 5 6 7>;
- interrupt-parent = <&MSI>;
- #interrupt-cells = <1>;
- #address-cells = <0>;
- #size-cells = <0>;
- msi-available-ranges = <0x0 0x100>;
- interrupt-map = <
- 0 &UIC3 0x18 1
- 1 &UIC3 0x19 1
- 2 &UIC3 0x1A 1
- 3 &UIC3 0x1B 1
- 4 &UIC3 0x1C 1
- 5 &UIC3 0x1D 1
- 6 &UIC3 0x1E 1
- 7 &UIC3 0x1F 1
- >;
- };
};
};
diff --git a/arch/powerpc/boot/dts/canyonlands.dts b/arch/powerpc/boot/dts/canyonlands.dts
index c5fbb08e0a6e..5db1bff6b23d 100644
--- a/arch/powerpc/boot/dts/canyonlands.dts
+++ b/arch/powerpc/boot/dts/canyonlands.dts
@@ -544,23 +544,5 @@ PCIE1: pcie@d20000000 {
0x0 0x0 0x0 0x3 &UIC3 0x12 0x4 /* swizzled int C */
0x0 0x0 0x0 0x4 &UIC3 0x13 0x4 /* swizzled int D */>;
};
-
- MSI: ppc4xx-msi@C10000000 {
- compatible = "amcc,ppc4xx-msi", "ppc4xx-msi";
- reg = < 0xC 0x10000000 0x100>;
- sdr-base = <0x36C>;
- msi-data = <0x00000000>;
- msi-mask = <0x44440000>;
- interrupt-count = <3>;
- interrupts = <0 1 2 3>;
- interrupt-parent = <&UIC3>;
- #interrupt-cells = <1>;
- #address-cells = <0>;
- #size-cells = <0>;
- interrupt-map = <0 &UIC3 0x18 1
- 1 &UIC3 0x19 1
- 2 &UIC3 0x1A 1
- 3 &UIC3 0x1B 1>;
- };
};
};
diff --git a/arch/powerpc/boot/dts/katmai.dts b/arch/powerpc/boot/dts/katmai.dts
index a8f353229fb7..4262b2bbd6de 100644
--- a/arch/powerpc/boot/dts/katmai.dts
+++ b/arch/powerpc/boot/dts/katmai.dts
@@ -442,24 +442,6 @@ PCIE2: pcie@d40000000 {
0x0 0x0 0x0 0x4 &UIC3 0xb 0x4 /* swizzled int D */>;
};
- MSI: ppc4xx-msi@400300000 {
- compatible = "amcc,ppc4xx-msi", "ppc4xx-msi";
- reg = < 0x4 0x00300000 0x100>;
- sdr-base = <0x3B0>;
- msi-data = <0x00000000>;
- msi-mask = <0x44440000>;
- interrupt-count = <3>;
- interrupts =<0 1 2 3>;
- interrupt-parent = <&UIC0>;
- #interrupt-cells = <1>;
- #address-cells = <0>;
- #size-cells = <0>;
- interrupt-map = <0 &UIC0 0xC 1
- 1 &UIC0 0x0D 1
- 2 &UIC0 0x0E 1
- 3 &UIC0 0x0F 1>;
- };
-
I2O: i2o@400100000 {
compatible = "ibm,i2o-440spe";
reg = <0x00000004 0x00100000 0x100>;
diff --git a/arch/powerpc/boot/dts/kilauea.dts b/arch/powerpc/boot/dts/kilauea.dts
index a709fb47a180..c07a7525a72c 100644
--- a/arch/powerpc/boot/dts/kilauea.dts
+++ b/arch/powerpc/boot/dts/kilauea.dts
@@ -403,33 +403,5 @@ PCIE1: pcie@c0000000 {
0x0 0x0 0x0 0x3 &UIC2 0xd 0x4 /* swizzled int C */
0x0 0x0 0x0 0x4 &UIC2 0xe 0x4 /* swizzled int D */>;
};
-
- MSI: ppc4xx-msi@C10000000 {
- compatible = "amcc,ppc4xx-msi", "ppc4xx-msi";
- reg = <0xEF620000 0x100>;
- sdr-base = <0x4B0>;
- msi-data = <0x00000000>;
- msi-mask = <0x44440000>;
- interrupt-count = <12>;
- interrupts = <0 1 2 3 4 5 6 7 8 9 0xA 0xB 0xC 0xD>;
- interrupt-parent = <&UIC2>;
- #interrupt-cells = <1>;
- #address-cells = <0>;
- #size-cells = <0>;
- interrupt-map = <0 &UIC2 0x10 1
- 1 &UIC2 0x11 1
- 2 &UIC2 0x12 1
- 2 &UIC2 0x13 1
- 2 &UIC2 0x14 1
- 2 &UIC2 0x15 1
- 2 &UIC2 0x16 1
- 2 &UIC2 0x17 1
- 2 &UIC2 0x18 1
- 2 &UIC2 0x19 1
- 2 &UIC2 0x1A 1
- 2 &UIC2 0x1B 1
- 2 &UIC2 0x1C 1
- 3 &UIC2 0x1D 1>;
- };
};
};
diff --git a/arch/powerpc/boot/dts/redwood.dts b/arch/powerpc/boot/dts/redwood.dts
index f38035a1f4a1..3c849e23e5f3 100644
--- a/arch/powerpc/boot/dts/redwood.dts
+++ b/arch/powerpc/boot/dts/redwood.dts
@@ -358,25 +358,6 @@ PCIE2: pcie@d40000000 {
0x0 0x0 0x0 0x4 &UIC3 0xb 0x4 /* swizzled int D */>;
};
- MSI: ppc4xx-msi@400300000 {
- compatible = "amcc,ppc4xx-msi", "ppc4xx-msi";
- reg = < 0x4 0x00300000 0x100
- 0x4 0x00300000 0x100>;
- sdr-base = <0x3B0>;
- msi-data = <0x00000000>;
- msi-mask = <0x44440000>;
- interrupt-count = <3>;
- interrupts =<0 1 2 3>;
- interrupt-parent = <&UIC0>;
- #interrupt-cells = <1>;
- #address-cells = <0>;
- #size-cells = <0>;
- interrupt-map = <0 &UIC0 0xC 1
- 1 &UIC0 0x0D 1
- 2 &UIC0 0x0E 1
- 3 &UIC0 0x0F 1>;
- };
-
};
diff --git a/arch/powerpc/platforms/40x/Kconfig b/arch/powerpc/platforms/40x/Kconfig
index e3e5217c9822..614ea6dc994c 100644
--- a/arch/powerpc/platforms/40x/Kconfig
+++ b/arch/powerpc/platforms/40x/Kconfig
@@ -23,7 +23,6 @@ config KILAUEA
select PPC4xx_PCI_EXPRESS
select FORCE_PCI
select PCI_MSI
- select PPC4xx_MSI
help
This option enables support for the AMCC PPC405EX evaluation board.
diff --git a/arch/powerpc/platforms/44x/Kconfig b/arch/powerpc/platforms/44x/Kconfig
index 83975ef50975..25b80cd558f8 100644
--- a/arch/powerpc/platforms/44x/Kconfig
+++ b/arch/powerpc/platforms/44x/Kconfig
@@ -23,7 +23,6 @@ config BLUESTONE
select APM821xx
select FORCE_PCI
select PCI_MSI
- select PPC4xx_MSI
select PPC4xx_PCI_EXPRESS
select IBM_EMAC_RGMII if IBM_EMAC
help
@@ -73,7 +72,6 @@ config KATMAI
select FORCE_PCI
select PPC4xx_PCI_EXPRESS
select PCI_MSI
- select PPC4xx_MSI
help
This option enables support for the AMCC PPC440SPe evaluation board.
@@ -115,7 +113,6 @@ config CANYONLANDS
select FORCE_PCI
select PPC4xx_PCI_EXPRESS
select PCI_MSI
- select PPC4xx_MSI
select IBM_EMAC_RGMII if IBM_EMAC
select IBM_EMAC_ZMII if IBM_EMAC
help
@@ -141,7 +138,6 @@ config REDWOOD
select FORCE_PCI
select PPC4xx_PCI_EXPRESS
select PCI_MSI
- select PPC4xx_MSI
help
This option enables support for the AMCC PPC460SX Redwood board.
--
2.31.1
^ permalink raw reply related
* Re: [PATCH v3] powerpc/pseries: read the lpar name from the firmware
From: Laurent Dufour @ 2021-12-07 16:07 UTC (permalink / raw)
To: Nathan Lynch; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <87bl1so588.fsf@linux.ibm.com>
On 07/12/2021, 15:32:39, Nathan Lynch wrote:
> Hi Laurent,
>
> Laurent Dufour <ldufour@linux.ibm.com> writes:
>> +/*
>> + * PAPR defines, in section "7.3.16 System Parameters Option", the token 55 to
>> + * read the LPAR name.
>> + */
>> +#define SPLPAR_LPAR_NAME_TOKEN 55
>> +static void read_lpar_name(struct seq_file *m)
>> +{
>> + int rc, len, token;
>> + union {
>> + char raw_buffer[RTAS_DATA_BUF_SIZE];
>> + struct {
>> + __be16 len;
>
> This:
>
>> + char name[RTAS_DATA_BUF_SIZE-2];
> ^^^^^^
>
> should be 4000, not (4K - 2), according to PAPR (it's weird and I don't
> know the reason).
That's true, PAPR defines the largest output buffer for
ibm,get-system-parameter to 4002, so we could limit this to 4002, not sure
whether this would make a big difference here. Anyway I will limit that
buffer size this way.
>
>
>> + };
>> + } *local_buffer;
>> +
>> + token = rtas_token("ibm,get-system-parameter");
>> + if (token == RTAS_UNKNOWN_SERVICE)
>> + return;
>> +
>> + local_buffer = kmalloc(sizeof(*local_buffer), GFP_KERNEL);
>> + if (!local_buffer)
>> + return;
>> +
>> + do {
>> + spin_lock(&rtas_data_buf_lock);
>> + memset(rtas_data_buf, 0, RTAS_DATA_BUF_SIZE);
>> + rc = rtas_call(token, 3, 1, NULL, SPLPAR_LPAR_NAME_TOKEN,
>> + __pa(rtas_data_buf), RTAS_DATA_BUF_SIZE);
>> + if (!rc)
>> + memcpy(local_buffer->raw_buffer, rtas_data_buf,
>> + RTAS_DATA_BUF_SIZE);
>> + spin_unlock(&rtas_data_buf_lock);
>> + } while (rtas_busy_delay(rc));
>> +
>> + if (rc != 0) {
>> + pr_err_once(
>> + "%s %s Error calling get-system-parameter (0x%x)\n",
>> + __FILE__, __func__, rc);
>
> The __FILE__ and __func__ in the message seem unnecessary, and rc should
> be reported in decimal so the error meaning is apparent.
Fair enough.
> Is there a reasonable fallback for VMs where this parameter doesn't
> exist? PowerVM partitions should always have it, but what do we want the
> behavior to be on other hypervisors?
In that case, there is no value displayed in the /proc/powerpc/lparcfg and
the lparstat -i command will fall back to the device tree value. I can't
see any valid reason to report the value defined in the device tree here.
>
>
>> + } else {
>> + /* Force end of string */
>> + len = be16_to_cpu(local_buffer->len);
>> + if (len >= (RTAS_DATA_BUF_SIZE-2))
>> + len = RTAS_DATA_BUF_SIZE-2;
>
> Could use min() or clamp(), and it would be better to build the
> expression using the value of sizeof(local_buffer->name).
Fair enough.
>
>> + local_buffer->name[len] = '\0';
>
> If 'len' can be (RTAS_DATA_BUF_SIZE - 2), then this writes past the end
> of the buffer, no?
Oh yes, the previous test should be
if (len >= (RTAS_DATA_BUF_SIZE-2))
len = RTAS_DATA_BUF_SIZE-3;
Thanks,
Laurent.
^ permalink raw reply
* Re: [PATCH v2 6/6] module: Move duplicate mod_check_sig users code to mod_parse_sig
From: Philipp Rudo @ 2021-12-07 16:10 UTC (permalink / raw)
To: Michal Suchanek
Cc: Nayna, Mimi Zohar, David Howells, keyrings, Paul Mackerras,
Alexander Gordeev, Rob Herring, Herbert Xu, Baoquan He,
Christian Borntraeger, James Morris, Lakshmi Ramasubramanian,
Christian Borntraeger, Serge E. Hallyn, Vasily Gorbik, linux-s390,
Heiko Carstens, linux-crypto, Hari Bathini, Daniel Axtens,
Dmitry Kasatkin, Frank van der Linden, kexec, linux-kernel,
Luis Chamberlain, Sven Schnelle, linux-security-module,
Jessica Yu, linux-integrity, linuxppc-dev, David S. Miller,
Thiago Jung Bauermann, buendgen
In-Reply-To: <d464e1f45d21a29cbbe828dea412206cdc94866b.1637862358.git.msuchanek@suse.de>
Hi Michal,
On Thu, 25 Nov 2021 19:02:44 +0100
Michal Suchanek <msuchanek@suse.de> wrote:
> Multiple users of mod_check_sig check for the marker, then call
> mod_check_sig, extract signature length, and remove the signature.
>
> Put this code in one place together with mod_check_sig.
>
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
> include/linux/module_signature.h | 1 +
> kernel/module_signature.c | 56 ++++++++++++++++++++++++++++-
> kernel/module_signing.c | 26 +++-----------
> security/integrity/ima/ima_modsig.c | 22 ++----------
> 4 files changed, 63 insertions(+), 42 deletions(-)
>
> diff --git a/include/linux/module_signature.h b/include/linux/module_signature.h
> index 7eb4b00381ac..1343879b72b3 100644
> --- a/include/linux/module_signature.h
> +++ b/include/linux/module_signature.h
> @@ -42,5 +42,6 @@ struct module_signature {
>
> int mod_check_sig(const struct module_signature *ms, size_t file_len,
> const char *name);
> +int mod_parse_sig(const void *data, size_t *len, size_t *sig_len, const char *name);
>
> #endif /* _LINUX_MODULE_SIGNATURE_H */
> diff --git a/kernel/module_signature.c b/kernel/module_signature.c
> index 00132d12487c..784b40575ee4 100644
> --- a/kernel/module_signature.c
> +++ b/kernel/module_signature.c
> @@ -8,14 +8,36 @@
>
> #include <linux/errno.h>
> #include <linux/printk.h>
> +#include <linux/string.h>
> #include <linux/module_signature.h>
> #include <asm/byteorder.h>
>
> +/**
> + * mod_check_sig_marker - check that the given data has signature marker at the end
> + *
> + * @data: Data with appended signature
> + * @len: Length of data. Signature marker length is subtracted on success.
> + */
> +static inline int mod_check_sig_marker(const void *data, size_t *len)
I personally don't like it when a function has a "check" in it's name
as it doesn't describe what the function is checking for. For me
mod_has_sig_marker is much more precise. I would use that instead.
Thanks
Philipp
> +{
> + const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
> +
> + if (markerlen > *len)
> + return -ENODATA;
> +
> + if (memcmp(data + *len - markerlen, MODULE_SIG_STRING,
> + markerlen))
> + return -ENODATA;
> +
> + *len -= markerlen;
> + return 0;
> +}
> +
> /**
> * mod_check_sig - check that the given signature is sane
> *
> * @ms: Signature to check.
> - * @file_len: Size of the file to which @ms is appended.
> + * @file_len: Size of the file to which @ms is appended (without the marker).
> * @name: What is being checked. Used for error messages.
> */
> int mod_check_sig(const struct module_signature *ms, size_t file_len,
> @@ -44,3 +66,35 @@ int mod_check_sig(const struct module_signature *ms, size_t file_len,
>
> return 0;
> }
> +
> +/**
> + * mod_parse_sig - check that the given signature is sane and determine signature length
> + *
> + * @data: Data with appended signature.
> + * @len: Length of data. Signature and marker length is subtracted on success.
> + * @sig_len: Length of signature. Filled on success.
> + * @name: What is being checked. Used for error messages.
> + */
> +int mod_parse_sig(const void *data, size_t *len, size_t *sig_len, const char *name)
> +{
> + const struct module_signature *sig;
> + int rc;
> +
> + rc = mod_check_sig_marker(data, len);
> + if (rc)
> + return rc;
> +
> + if (*len < sizeof(*sig))
> + return -ENODATA;
> +
> + sig = (const struct module_signature *)(data + (*len - sizeof(*sig)));
> +
> + rc = mod_check_sig(sig, *len, name);
> + if (rc)
> + return rc;
> +
> + *sig_len = be32_to_cpu(sig->sig_len);
> + *len -= *sig_len + sizeof(*sig);
> +
> + return 0;
> +}
> diff --git a/kernel/module_signing.c b/kernel/module_signing.c
> index cef72a6f6b5d..02bbca90f467 100644
> --- a/kernel/module_signing.c
> +++ b/kernel/module_signing.c
> @@ -25,35 +25,17 @@ int verify_appended_signature(const void *data, size_t *len,
> struct key *trusted_keys,
> enum key_being_used_for purpose)
> {
> - const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
> struct module_signature ms;
> - size_t sig_len, modlen = *len;
> + size_t sig_len;
> int ret;
>
> - pr_devel("==>%s %s(,%zu)\n", __func__, key_being_used_for[purpose], modlen);
> + pr_devel("==>%s %s(,%zu)\n", __func__, key_being_used_for[purpose], *len);
>
> - if (markerlen > modlen)
> - return -ENODATA;
> -
> - if (memcmp(data + modlen - markerlen, MODULE_SIG_STRING,
> - markerlen))
> - return -ENODATA;
> - modlen -= markerlen;
> -
> - if (modlen <= sizeof(ms))
> - return -EBADMSG;
> -
> - memcpy(&ms, data + (modlen - sizeof(ms)), sizeof(ms));
> -
> - ret = mod_check_sig(&ms, modlen, key_being_used_for[purpose]);
> + ret = mod_parse_sig(data, len, &sig_len, key_being_used_for[purpose]);
> if (ret)
> return ret;
>
> - sig_len = be32_to_cpu(ms.sig_len);
> - modlen -= sig_len + sizeof(ms);
> - *len = modlen;
> -
> - return verify_pkcs7_signature(data, modlen, data + modlen, sig_len,
> + return verify_pkcs7_signature(data, *len, data + *len, sig_len,
> trusted_keys,
> purpose,
> NULL, NULL);
> diff --git a/security/integrity/ima/ima_modsig.c b/security/integrity/ima/ima_modsig.c
> index fb25723c65bc..46917eb37fd8 100644
> --- a/security/integrity/ima/ima_modsig.c
> +++ b/security/integrity/ima/ima_modsig.c
> @@ -37,33 +37,17 @@ struct modsig {
> *
> * Return: 0 on success, error code otherwise.
> */
> -int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
> +int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t len,
> struct modsig **modsig)
> {
> - const size_t marker_len = strlen(MODULE_SIG_STRING);
> - const struct module_signature *sig;
> struct modsig *hdr;
> - size_t sig_len;
> - const void *p;
> + size_t sig_len, buf_len = len;
> int rc;
>
> - if (buf_len <= marker_len + sizeof(*sig))
> - return -ENOENT;
> -
> - p = buf + buf_len - marker_len;
> - if (memcmp(p, MODULE_SIG_STRING, marker_len))
> - return -ENOENT;
> -
> - buf_len -= marker_len;
> - sig = (const struct module_signature *)(p - sizeof(*sig));
> -
> - rc = mod_check_sig(sig, buf_len, func_tokens[func]);
> + rc = mod_parse_sig(buf, &buf_len, &sig_len, func_tokens[func]);
> if (rc)
> return rc;
>
> - sig_len = be32_to_cpu(sig->sig_len);
> - buf_len -= sig_len + sizeof(*sig);
> -
> /* Allocate sig_len additional bytes to hold the raw PKCS#7 data. */
> hdr = kzalloc(sizeof(*hdr) + sig_len, GFP_KERNEL);
> if (!hdr)
^ permalink raw reply
* Re: [PATCH v2 0/6] KEXEC_SIG with appended signature
From: Philipp Rudo @ 2021-12-07 16:10 UTC (permalink / raw)
To: Michal Suchanek
Cc: Nayna, Mimi Zohar, David Howells, keyrings, Paul Mackerras,
Alexander Gordeev, Rob Herring, Herbert Xu, Baoquan He,
Christian Borntraeger, James Morris, Lakshmi Ramasubramanian,
Christian Borntraeger, Serge E. Hallyn, Vasily Gorbik, linux-s390,
Heiko Carstens, linux-crypto, Hari Bathini, Daniel Axtens,
Dmitry Kasatkin, Frank van der Linden, kexec, linux-kernel,
Luis Chamberlain, Sven Schnelle, linux-security-module,
Jessica Yu, linux-integrity, linuxppc-dev, David S. Miller,
Thiago Jung Bauermann, buendgen
In-Reply-To: <cover.1637862358.git.msuchanek@suse.de>
Hi Michal,
i finally had the time to take a closer look at the series. Except for
the nit in patch 4 and my personal preference in patch 6 the code looks
good to me.
What I don't like are the commit messages on the first commits. In my
opinion they are so short that they are almost useless. For example in
patch 2 there is absolutely no explanation why you can simply copy the
s390 over to ppc. Or in patch 3 you are silently changing the error
code in kexec from EKEYREJECT to ENODATA. So I would appreciate it if
you could improve them a little.
Thanks
Philipp
On Thu, 25 Nov 2021 19:02:38 +0100
Michal Suchanek <msuchanek@suse.de> wrote:
> Hello,
>
> This is resend of the KEXEC_SIG patchset.
>
> The first patch is new because it'a a cleanup that does not require any
> change to the module verification code.
>
> The second patch is the only one that is intended to change any
> functionality.
>
> The rest only deduplicates code but I did not receive any review on that
> part so I don't know if it's desirable as implemented.
>
> The first two patches can be applied separately without the rest.
>
> Thanks
>
> Michal
>
> Michal Suchanek (6):
> s390/kexec_file: Don't opencode appended signature check.
> powerpc/kexec_file: Add KEXEC_SIG support.
> kexec_file: Don't opencode appended signature verification.
> module: strip the signature marker in the verification function.
> module: Use key_being_used_for for log messages in
> verify_appended_signature
> module: Move duplicate mod_check_sig users code to mod_parse_sig
>
> arch/powerpc/Kconfig | 11 +++++
> arch/powerpc/kexec/elf_64.c | 14 ++++++
> arch/s390/kernel/machine_kexec_file.c | 42 ++----------------
> crypto/asymmetric_keys/asymmetric_type.c | 1 +
> include/linux/module_signature.h | 1 +
> include/linux/verification.h | 4 ++
> kernel/module-internal.h | 2 -
> kernel/module.c | 12 +++--
> kernel/module_signature.c | 56 +++++++++++++++++++++++-
> kernel/module_signing.c | 33 +++++++-------
> security/integrity/ima/ima_modsig.c | 22 ++--------
> 11 files changed, 113 insertions(+), 85 deletions(-)
>
^ permalink raw reply
* Re: [PATCH v2 4/6] module: strip the signature marker in the verification function.
From: Philipp Rudo @ 2021-12-07 16:11 UTC (permalink / raw)
To: Michal Suchanek
Cc: Nayna, Mimi Zohar, David Howells, keyrings, Paul Mackerras,
Alexander Gordeev, Rob Herring, Herbert Xu, Baoquan He,
Christian Borntraeger, James Morris, Lakshmi Ramasubramanian,
Christian Borntraeger, Serge E. Hallyn, Vasily Gorbik, linux-s390,
Heiko Carstens, linux-crypto, Hari Bathini, Daniel Axtens,
Dmitry Kasatkin, Frank van der Linden, kexec, linux-kernel,
Luis Chamberlain, Sven Schnelle, linux-security-module,
Jessica Yu, linux-integrity, linuxppc-dev, David S. Miller,
Thiago Jung Bauermann, buendgen
In-Reply-To: <0f9bbbc4800d5329485b6bdabbbe1ef3b2169b02.1637862358.git.msuchanek@suse.de>
Hi Michal,
On Thu, 25 Nov 2021 19:02:42 +0100
Michal Suchanek <msuchanek@suse.de> wrote:
> It is stripped by each caller separately.
>
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
> arch/powerpc/kexec/elf_64.c | 9 ---------
> arch/s390/kernel/machine_kexec_file.c | 9 ---------
> kernel/module.c | 7 +------
> kernel/module_signing.c | 12 ++++++++++--
kernel/module_signing.c is only compiled with MODULE_SIG enabled but
KEXEC_SIG only selects MODULE_SIG_FORMAT. In the unlikely case that
KEXEC_SIG is enabled but MODULE_SIG isn't this causes a build breakage.
So you need to update KEXEC_SIG to select MODULE_SIG instead of
MODULE_SIG_FORMAT for s390 and ppc.
Thanks
Philipp
> 4 files changed, 11 insertions(+), 26 deletions(-)
>
> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
> index 266cb26d3ca0..63634c95265d 100644
> --- a/arch/powerpc/kexec/elf_64.c
> +++ b/arch/powerpc/kexec/elf_64.c
> @@ -156,15 +156,6 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
> int elf64_verify_sig(const char *kernel, unsigned long length)
> {
> size_t kernel_len = length;
> - const unsigned long marker_len = sizeof(MODULE_SIG_STRING) - 1;
> -
> - if (marker_len > kernel_len)
> - return -EKEYREJECTED;
> -
> - if (memcmp(kernel + kernel_len - marker_len, MODULE_SIG_STRING,
> - marker_len))
> - return -EKEYREJECTED;
> - kernel_len -= marker_len;
>
> return verify_appended_signature(kernel, &kernel_len, VERIFY_USE_PLATFORM_KEYRING,
> "kexec_file");
> diff --git a/arch/s390/kernel/machine_kexec_file.c b/arch/s390/kernel/machine_kexec_file.c
> index 432797249db3..c4632c1a1b59 100644
> --- a/arch/s390/kernel/machine_kexec_file.c
> +++ b/arch/s390/kernel/machine_kexec_file.c
> @@ -27,20 +27,11 @@ const struct kexec_file_ops * const kexec_file_loaders[] = {
> int s390_verify_sig(const char *kernel, unsigned long length)
> {
> size_t kernel_len = length;
> - const unsigned long marker_len = sizeof(MODULE_SIG_STRING) - 1;
>
> /* Skip signature verification when not secure IPLed. */
> if (!ipl_secure_flag)
> return 0;
>
> - if (marker_len > kernel_len)
> - return -EKEYREJECTED;
> -
> - if (memcmp(kernel + kernel_len - marker_len, MODULE_SIG_STRING,
> - marker_len))
> - return -EKEYREJECTED;
> - kernel_len -= marker_len;
> -
> return verify_appended_signature(kernel, &kernel_len, VERIFY_USE_PLATFORM_KEYRING,
> "kexec_file");
> }
> diff --git a/kernel/module.c b/kernel/module.c
> index 8481933dfa92..d91ca0f93a40 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2882,7 +2882,6 @@ static inline void kmemleak_load_module(const struct module *mod,
> static int module_sig_check(struct load_info *info, int flags)
> {
> int err = -ENODATA;
> - const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
> const char *reason;
> const void *mod = info->hdr;
>
> @@ -2890,11 +2889,7 @@ static int module_sig_check(struct load_info *info, int flags)
> * Require flags == 0, as a module with version information
> * removed is no longer the module that was signed
> */
> - if (flags == 0 &&
> - info->len > markerlen &&
> - memcmp(mod + info->len - markerlen, MODULE_SIG_STRING, markerlen) == 0) {
> - /* We truncate the module to discard the signature */
> - info->len -= markerlen;
> + if (flags == 0) {
> err = verify_appended_signature(mod, &info->len,
> VERIFY_USE_SECONDARY_KEYRING, "module");
> if (!err) {
> diff --git a/kernel/module_signing.c b/kernel/module_signing.c
> index f492e410564d..4c28cb55275f 100644
> --- a/kernel/module_signing.c
> +++ b/kernel/module_signing.c
> @@ -15,8 +15,7 @@
> #include "module-internal.h"
>
> /**
> - * verify_appended_signature - Verify the signature on a module with the
> - * signature marker stripped.
> + * verify_appended_signature - Verify the signature on a module
> * @data: The data to be verified
> * @len: Size of @data.
> * @trusted_keys: Keyring to use for verification
> @@ -25,12 +24,21 @@
> int verify_appended_signature(const void *data, size_t *len,
> struct key *trusted_keys, const char *what)
> {
> + const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
> struct module_signature ms;
> size_t sig_len, modlen = *len;
> int ret;
>
> pr_devel("==>%s(,%zu)\n", __func__, modlen);
>
> + if (markerlen > modlen)
> + return -ENODATA;
> +
> + if (memcmp(data + modlen - markerlen, MODULE_SIG_STRING,
> + markerlen))
> + return -ENODATA;
> + modlen -= markerlen;
> +
> if (modlen <= sizeof(ms))
> return -EBADMSG;
>
^ permalink raw reply
* Re: [PATCH] powerpc: platforms: cell: pervasive: fix clang -Wimplicit-fallthrough
From: Nathan Chancellor @ 2021-12-07 16:27 UTC (permalink / raw)
To: Anders Roxell
Cc: arnd, Naresh Kamboju, llvm, ndesaulniers, linux-kernel,
linuxppc-dev
In-Reply-To: <20211207110228.698956-1-anders.roxell@linaro.org>
On Tue, Dec 07, 2021 at 12:02:28PM +0100, Anders Roxell wrote:
> Clang warns:
>
> arch/powerpc/platforms/cell/pervasive.c:81:2: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
> case SRR1_WAKEEE:
> ^
> arch/powerpc/platforms/cell/pervasive.c:81:2: note: insert 'break;' to avoid fall-through
> case SRR1_WAKEEE:
> ^
> break;
> 1 error generated.
>
> Clang is more pedantic than GCC, which does not warn when failing
> through to a case that is just break or return. Clang's version
> is more in line with the kernel's own stance in deprecated.rst.
> Add athe missing break to silence the warning.
^ small typo, probably not worth a resend
>
> Fixes: 6e83985b0f6e ("powerpc/cbe: Do not process external or decremeter interrupts from sreset")
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
Reviewed-by: Nathan Chancellor <nathan@kernel.org>
> ---
> arch/powerpc/platforms/cell/pervasive.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/powerpc/platforms/cell/pervasive.c b/arch/powerpc/platforms/cell/pervasive.c
> index 5b9a7e9f144b..dff8d5e7ab82 100644
> --- a/arch/powerpc/platforms/cell/pervasive.c
> +++ b/arch/powerpc/platforms/cell/pervasive.c
> @@ -78,6 +78,7 @@ static int cbe_system_reset_exception(struct pt_regs *regs)
> switch (regs->msr & SRR1_WAKEMASK) {
> case SRR1_WAKEDEC:
> set_dec(1);
> + break;
> case SRR1_WAKEEE:
> /*
> * Handle these when interrupts get re-enabled and we take
> --
> 2.33.0
>
^ permalink raw reply
* Re: [PATCH v3] powerpc/pseries: read the lpar name from the firmware
From: Nathan Lynch @ 2021-12-07 17:07 UTC (permalink / raw)
To: Laurent Dufour; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <bbaa0d78-a09f-3ce3-25a9-67434039b741@linux.ibm.com>
Laurent Dufour <ldufour@linux.ibm.com> writes:
> On 07/12/2021, 15:32:39, Nathan Lynch wrote:
>> Is there a reasonable fallback for VMs where this parameter doesn't
>> exist? PowerVM partitions should always have it, but what do we want the
>> behavior to be on other hypervisors?
>
> In that case, there is no value displayed in the /proc/powerpc/lparcfg and
> the lparstat -i command will fall back to the device tree value. I can't
> see any valid reason to report the value defined in the device tree
> here.
Here's a valid reason :-)
lparstat isn't the only possible consumer of the interface, and the
'ibm,partition-name' property and the dynamic system parameter clearly
serve a common purpose. 'ibm,partition-name' is provided by qemu.
In any case, the function should not print an error when the return
value is -3 (parameter not supported).
^ permalink raw reply
* [PATCH v4] powerpc/pseries: read the lpar name from the firmware
From: Laurent Dufour @ 2021-12-07 17:11 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nathan Lynch, linux-kernel
The LPAR name may be changed after the LPAR has been started in the HMC.
In that case lparstat command is not reporting the updated value because it
reads it from the device tree which is read at boot time.
However this value could be read from RTAS.
Adding this value in the /proc/powerpc/lparcfg output allows to read the
updated value.
Cc: Nathan Lynch <nathanl@linux.ibm.com>
Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
---
v4:
address Nathan's new comments limiting size of the buffer.
v3:
address Michael's comments.
v2:
address Nathan's comments.
change title to partition_name aligning with existing partition_id
---
arch/powerpc/platforms/pseries/lparcfg.c | 54 ++++++++++++++++++++++++
1 file changed, 54 insertions(+)
diff --git a/arch/powerpc/platforms/pseries/lparcfg.c b/arch/powerpc/platforms/pseries/lparcfg.c
index f71eac74ea92..058d9a5fe545 100644
--- a/arch/powerpc/platforms/pseries/lparcfg.c
+++ b/arch/powerpc/platforms/pseries/lparcfg.c
@@ -311,6 +311,59 @@ static void parse_mpp_x_data(struct seq_file *m)
seq_printf(m, "coalesce_pool_spurr=%ld\n", mpp_x_data.pool_spurr_cycles);
}
+/*
+ * PAPR defines, in section "7.3.16 System Parameters Option", the token 55 to
+ * read the LPAR name, and the largest output data to 4000 + 2 bytes length.
+ */
+#define SPLPAR_LPAR_NAME_TOKEN 55
+#define GET_SYS_PARM_BUF_SIZE 4002
+#if GET_SYS_PARM_BUF_SIZE > RTAS_DATA_BUF_SIZE
+#error "GET_SYS_PARM_BUF_SIZE is larger than RTAS_DATA_BUF_SIZE"
+#endif
+static void read_lpar_name(struct seq_file *m)
+{
+ int rc, len, token;
+ union {
+ char raw_buffer[GET_SYS_PARM_BUF_SIZE];
+ struct {
+ __be16 len;
+ char name[GET_SYS_PARM_BUF_SIZE-2];
+ };
+ } *local_buffer;
+
+ token = rtas_token("ibm,get-system-parameter");
+ if (token == RTAS_UNKNOWN_SERVICE)
+ return;
+
+ local_buffer = kmalloc(sizeof(*local_buffer), GFP_KERNEL);
+ if (!local_buffer)
+ return;
+
+ do {
+ spin_lock(&rtas_data_buf_lock);
+ memset(rtas_data_buf, 0, sizeof(*local_buffer));
+ rc = rtas_call(token, 3, 1, NULL, SPLPAR_LPAR_NAME_TOKEN,
+ __pa(rtas_data_buf), sizeof(*local_buffer));
+ if (!rc)
+ memcpy(local_buffer->raw_buffer, rtas_data_buf,
+ sizeof(local_buffer->raw_buffer));
+ spin_unlock(&rtas_data_buf_lock);
+ } while (rtas_busy_delay(rc));
+
+ if (!rc) {
+ /* Force end of string */
+ len = min((int) be16_to_cpu(local_buffer->len),
+ (int) sizeof(local_buffer->name)-1);
+ local_buffer->name[len] = '\0';
+
+ seq_printf(m, "partition_name=%s\n", local_buffer->name);
+ } else
+ pr_err_once("Error calling get-system-parameter (0x%x)\n", rc);
+
+ kfree(local_buffer);
+}
+
+
#define SPLPAR_CHARACTERISTICS_TOKEN 20
#define SPLPAR_MAXLENGTH 1026*(sizeof(char))
@@ -496,6 +549,7 @@ static int pseries_lparcfg_data(struct seq_file *m, void *v)
if (firmware_has_feature(FW_FEATURE_SPLPAR)) {
/* this call handles the ibm,get-system-parameter contents */
+ read_lpar_name(m);
parse_system_parameter_string(m);
parse_ppp_data(m);
parse_mpp_data(m);
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v3] powerpc/pseries: read the lpar name from the firmware
From: Laurent Dufour @ 2021-12-07 17:18 UTC (permalink / raw)
To: Nathan Lynch; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <878rwwny1l.fsf@linux.ibm.com>
On 07/12/2021, 18:07:50, Nathan Lynch wrote:
> Laurent Dufour <ldufour@linux.ibm.com> writes:
>> On 07/12/2021, 15:32:39, Nathan Lynch wrote:
>>> Is there a reasonable fallback for VMs where this parameter doesn't
>>> exist? PowerVM partitions should always have it, but what do we want the
>>> behavior to be on other hypervisors?
>>
>> In that case, there is no value displayed in the /proc/powerpc/lparcfg and
>> the lparstat -i command will fall back to the device tree value. I can't
>> see any valid reason to report the value defined in the device tree
>> here.
>
> Here's a valid reason :-)
>
> lparstat isn't the only possible consumer of the interface, and the
> 'ibm,partition-name' property and the dynamic system parameter clearly
> serve a common purpose. 'ibm,partition-name' is provided by qemu.
If the hypervisor is not providing this value, this is not the goal of this
interface to fetch it from the device tree.
Any consumer should be able to fall back on the device tree value, and
there is no added value to do such a trick in the kernel when it can be
done in the user space.
> In any case, the function should not print an error when the return
> value is -3 (parameter not supported).
That's a valid requirement.
^ permalink raw reply
* Re: [PATCH v3] powerpc/pseries: read the lpar name from the firmware
From: Laurent Dufour @ 2021-12-07 17:24 UTC (permalink / raw)
To: Nathan Lynch; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <21eb4749-42b1-da78-8833-00d360fa36e5@linux.ibm.com>
On 07/12/2021, 18:18:40, Laurent Dufour wrote:
> On 07/12/2021, 18:07:50, Nathan Lynch wrote:
>> Laurent Dufour <ldufour@linux.ibm.com> writes:
>>> On 07/12/2021, 15:32:39, Nathan Lynch wrote:
>>>> Is there a reasonable fallback for VMs where this parameter doesn't
>>>> exist? PowerVM partitions should always have it, but what do we want the
>>>> behavior to be on other hypervisors?
>>>
>>> In that case, there is no value displayed in the /proc/powerpc/lparcfg and
>>> the lparstat -i command will fall back to the device tree value. I can't
>>> see any valid reason to report the value defined in the device tree
>>> here.
>>
>> Here's a valid reason :-)
>>
>> lparstat isn't the only possible consumer of the interface, and the
>> 'ibm,partition-name' property and the dynamic system parameter clearly
>> serve a common purpose. 'ibm,partition-name' is provided by qemu.
>
> If the hypervisor is not providing this value, this is not the goal of this
> interface to fetch it from the device tree.
>
> Any consumer should be able to fall back on the device tree value, and
> there is no added value to do such a trick in the kernel when it can be
> done in the user space.
>
>> In any case, the function should not print an error when the return
>> value is -3 (parameter not supported).
>
> That's a valid requirement.
I sent a v4 which is printing an error message even if the parameter is not
supported by the hypervisor.
This is unlikely and since this a call to pr_err_once(), it would be
printed only once, so not really annoying. I don't think a v5 is required
for such a minor message.
^ permalink raw reply
* Re: [PATCH v2 0/6] KEXEC_SIG with appended signature
From: Michal Suchánek @ 2021-12-07 17:32 UTC (permalink / raw)
To: Philipp Rudo
Cc: Nayna, Mimi Zohar, David Howells, keyrings, Paul Mackerras,
Alexander Gordeev, Rob Herring, Herbert Xu, Baoquan He,
Christian Borntraeger, James Morris, Lakshmi Ramasubramanian,
Christian Borntraeger, Serge E. Hallyn, Vasily Gorbik, linux-s390,
Heiko Carstens, linux-crypto, Hari Bathini, Daniel Axtens,
Dmitry Kasatkin, Frank van der Linden, kexec, linux-kernel,
Luis Chamberlain, Sven Schnelle, linux-security-module,
Jessica Yu, linux-integrity, linuxppc-dev, David S. Miller,
Thiago Jung Bauermann, buendgen
In-Reply-To: <20211207171014.2cfc4a54@rhtmp>
On Tue, Dec 07, 2021 at 05:10:14PM +0100, Philipp Rudo wrote:
> Hi Michal,
>
> i finally had the time to take a closer look at the series. Except for
> the nit in patch 4 and my personal preference in patch 6 the code looks
> good to me.
>
> What I don't like are the commit messages on the first commits. In my
> opinion they are so short that they are almost useless. For example in
> patch 2 there is absolutely no explanation why you can simply copy the
> s390 over to ppc.
They use the same signature format. I suppose I can add a note saying
that.
> Or in patch 3 you are silently changing the error
> code in kexec from EKEYREJECT to ENODATA. So I would appreciate it if
Not sure what I should do about this. The different implementations use
different random error codes, and when they are unified the error code
clearly changes for one or the other.
Does anything depend on a particular error code returned?
Thanks
Michal
> you could improve them a little.
>
> Thanks
> Philipp
>
> On Thu, 25 Nov 2021 19:02:38 +0100
> Michal Suchanek <msuchanek@suse.de> wrote:
>
> > Hello,
> >
> > This is resend of the KEXEC_SIG patchset.
> >
> > The first patch is new because it'a a cleanup that does not require any
> > change to the module verification code.
> >
> > The second patch is the only one that is intended to change any
> > functionality.
> >
> > The rest only deduplicates code but I did not receive any review on that
> > part so I don't know if it's desirable as implemented.
> >
> > The first two patches can be applied separately without the rest.
> >
> > Thanks
> >
> > Michal
> >
> > Michal Suchanek (6):
> > s390/kexec_file: Don't opencode appended signature check.
> > powerpc/kexec_file: Add KEXEC_SIG support.
> > kexec_file: Don't opencode appended signature verification.
> > module: strip the signature marker in the verification function.
> > module: Use key_being_used_for for log messages in
> > verify_appended_signature
> > module: Move duplicate mod_check_sig users code to mod_parse_sig
> >
> > arch/powerpc/Kconfig | 11 +++++
> > arch/powerpc/kexec/elf_64.c | 14 ++++++
> > arch/s390/kernel/machine_kexec_file.c | 42 ++----------------
> > crypto/asymmetric_keys/asymmetric_type.c | 1 +
> > include/linux/module_signature.h | 1 +
> > include/linux/verification.h | 4 ++
> > kernel/module-internal.h | 2 -
> > kernel/module.c | 12 +++--
> > kernel/module_signature.c | 56 +++++++++++++++++++++++-
> > kernel/module_signing.c | 33 +++++++-------
> > security/integrity/ima/ima_modsig.c | 22 ++--------
> > 11 files changed, 113 insertions(+), 85 deletions(-)
> >
>
^ permalink raw reply
* Re: [PATCH] powerpc/603: Fix boot failure with DEBUG_PAGEALLOC and KFENCE
From: Christophe Leroy @ 2021-12-07 17:49 UTC (permalink / raw)
To: mbizon@freebox.fr, Michael Ellerman, Aneesh Kumar K.V
Cc: linuxppc-dev@lists.ozlabs.org, Paul Mackerras,
linux-kernel@vger.kernel.org, stable@vger.kernel.org
In-Reply-To: <12988dafdf7e14ba6db69ab483a2eb53e411fc0d.camel@freebox.fr>
Le 07/12/2021 à 11:34, Maxime Bizon a écrit :
>
> On Tue, 2021-12-07 at 06:10 +0000, Christophe Leroy wrote:
>
> Hello,
>
> With the patch applied and
>
> CONFIG_DEBUG_PAGEALLOC=y
> CONFIG_DEBUG_PAGEALLOC_ENABLE_DEFAULT=y
> CONFIG_DEBUG_VM=y
>
> I get tons of this during boot:
>
> [ 0.000000] Dentry cache hash table entries: 262144 (order: 8, 1048576 bytes, linear)
> [ 0.000000] Inode-cache hash table entries: 131072 (order: 7, 524288 bytes, linear)
> [ 0.000000] mem auto-init: stack:off, heap alloc:off, heap free:off
> [ 0.000000] ------------[ cut here ]------------
> [ 0.000000] WARNING: CPU: 0 PID: 0 at arch/powerpc/mm/pgtable.c:194 set_pte_at+0x18/0x160
> [ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 5.15.0+ #442
> [ 0.000000] NIP: 80015ebc LR: 80016728 CTR: 800166e4
> [ 0.000000] REGS: 80751dd0 TRAP: 0700 Not tainted (5.15.0+)
> [ 0.000000] MSR: 00021032 <ME,IR,DR,RI> CR: 42228882 XER: 20000000
> [ 0.000000]
> [ 0.000000] GPR00: 800b8dc8 80751e80 806c6300 807311d8 807a1000 8ffffe84 80751ea8 00000000
> [ 0.000000] GPR08: 007a1591 00000001 007a1180 00000000 42224882 00000000 3ff9c608 3fffd79c
> [ 0.000000] GPR16: 00000000 00000000 00000000 00000000 00000000 00000000 800166e4 807a2000
> [ 0.000000] GPR24: 807a1fff 807311d8 807311d8 807a2000 80768804 00000000 807a1000 007a1180
> [ 0.000000] NIP [80015ebc] set_pte_at+0x18/0x160
> [ 0.000000] LR [80016728] set_page_attr+0x44/0xc0
> [ 0.000000] Call Trace:
> [ 0.000000] [80751e80] [80058570] console_unlock+0x340/0x428 (unreliable)
> [ 0.000000] [80751ea0] [00000000] 0x0
> [ 0.000000] [80751ec0] [800b8dc8] __apply_to_page_range+0x144/0x2a8
> [ 0.000000] [80751f00] [80016918] __kernel_map_pages+0x54/0x64
> [ 0.000000] [80751f10] [800cfeb0] __free_pages_ok+0x1b0/0x440
> [ 0.000000] [80751f50] [805cfc8c] memblock_free_all+0x1d8/0x274
> [ 0.000000] [80751f90] [805c5e0c] mem_init+0x3c/0xd0
> [ 0.000000] [80751fb0] [805c0bdc] start_kernel+0x404/0x5c4
> [ 0.000000] [80751ff0] [000033f0] 0x33f0
> [ 0.000000] Instruction dump:
> [ 0.000000] 7c630034 83e1000c 5463d97e 7c0803a6 38210010 4e800020 9421ffe0 93e1001c
> [ 0.000000] 83e60000 81250000 71290001 41820014 <0fe00000> 7c0802a6 93c10018 90010024
>
>
That's unrelated to this patch.
The problem is linked to patch c988cfd38e48 ("powerpc/32: use
set_memory_attr()"), which changed from using __set_pte_at() to using
set_memory_attr() which uses set_pte_at().
set_pte_at() has additional checks and shall not be used to updating an
existing PTE.
Wondering if I should just use __set_pte_at() instead like in the past,
or do like commit 9f7853d7609d ("powerpc/mm: Fix set_memory_*() against
concurrent accesses") and use pte_update()
Michael, Aneesh, any suggestion ?
Thanks
Christophe
^ 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