* Re: [PATCH v17 00/10] Carry forward IMA measurement log on kexec on ARM64
From: Lakshmi Ramasubramanian @ 2021-02-10 22:34 UTC (permalink / raw)
To: Mimi Zohar, Rob Herring
Cc: Mark Rutland, tao.li, Paul Mackerras, vincenzo.frascino,
Frank Rowand, Sasha Levin, Masahiro Yamada, James Morris,
AKASHI, Takahiro, linux-arm-kernel, Catalin Marinas,
Serge E. Hallyn, devicetree, Pavel Tatashin, Will Deacon,
Prakhar Srivastava, Hsin-Yi Wang, Allison Randal,
Christophe Leroy, Matthias Brugger, balajib, dmitry.kasatkin,
linux-kernel@vger.kernel.org, James Morse, Greg Kroah-Hartman,
Joe Perches, linux-integrity, linuxppc-dev, Thiago Jung Bauermann
In-Reply-To: <594445d01e085875b97b46be726247f89d1e6661.camel@linux.ibm.com>
On 2/10/21 1:39 PM, Mimi Zohar wrote:
> On Wed, 2021-02-10 at 15:55 -0500, Mimi Zohar wrote:
>> On Wed, 2021-02-10 at 14:42 -0600, Rob Herring wrote:
>>> On Wed, Feb 10, 2021 at 11:33 AM Lakshmi Ramasubramanian
>>
>>> Ideally, we don't apply the same patch in 2 branches. It looks like
>>> there's a conflict but no real dependence on the above patch (the
>>> ima_buffer part). The conflict seems trivial enough that Linus can
>>> resolve it in the merge window.
>>>
>>> Or Mimi can take the whole thing if preferred?
>>
>> How about I create a topic branch with just the two patches, allowing
>> both of us to merge it? There shouldn't be a problem with re-writing
>> next-integrity history.
>
> The 2 patches are now in the ima-kexec-fixes branch.
>
Thanks a lot Mimi.
Rob - I will address the 2 comments you'd provided today, and build the
patches in ima-kexec-fixes branch.
If you have more comments in the v17 patches, please let me know.
thanks,
-lakshmi
^ permalink raw reply
* Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function
From: Thiago Jung Bauermann @ 2021-02-10 23:24 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: mark.rutland, tao.li, zohar, paulus, vincenzo.frascino,
frowand.list, sashal, robh, masahiroy, jmorris, takahiro.akashi,
linux-arm-kernel, catalin.marinas, serge, devicetree,
pasha.tatashin, will, prsriva, hsinyi, allison, christophe.leroy,
mbrugger, balajib, dmitry.kasatkin, linux-kernel, james.morse,
gregkh, joe, linux-integrity, linuxppc-dev
In-Reply-To: <20210209182200.30606-3-nramas@linux.microsoft.com>
Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes:
> From: Rob Herring <robh@kernel.org>
>
> Both arm64 and powerpc do essentially the same FDT /chosen setup for
> kexec. The differences are either omissions that arm64 should have
> or additional properties that will be ignored. The setup code can be
> combined and shared by both powerpc and arm64.
>
> The differences relative to the arm64 version:
> - If /chosen doesn't exist, it will be created (should never happen).
> - Any old dtb and initrd reserved memory will be released.
> - The new initrd and elfcorehdr are marked reserved.
> - "linux,booted-from-kexec" is set.
>
> The differences relative to the powerpc version:
> - "kaslr-seed" and "rng-seed" may be set.
> - "linux,elfcorehdr" is set.
> - Any existing "linux,usable-memory-range" is removed.
>
> Combine the code for setting up the /chosen node in the FDT and updating
> the memory reservation for kexec, for powerpc and arm64, in
> of_kexec_alloc_and_setup_fdt() and move it to "drivers/of/kexec.c".
>
> Signed-off-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> ---
> drivers/of/Makefile | 6 ++
> drivers/of/kexec.c | 258 ++++++++++++++++++++++++++++++++++++++++++++
> include/linux/of.h | 13 +++
> 3 files changed, 277 insertions(+)
> create mode 100644 drivers/of/kexec.c
Reviewed-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH v17 03/10] arm64: Use common of_kexec_alloc_and_setup_fdt()
From: Thiago Jung Bauermann @ 2021-02-10 23:30 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: mark.rutland, tao.li, zohar, paulus, vincenzo.frascino,
frowand.list, sashal, robh, masahiroy, jmorris, takahiro.akashi,
linux-arm-kernel, catalin.marinas, serge, devicetree,
pasha.tatashin, will, prsriva, hsinyi, allison, christophe.leroy,
mbrugger, balajib, dmitry.kasatkin, linux-kernel, james.morse,
gregkh, joe, linux-integrity, linuxppc-dev
In-Reply-To: <20210209182200.30606-4-nramas@linux.microsoft.com>
Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes:
> From: Rob Herring <robh@kernel.org>
>
> The code for setting up the /chosen node in the device tree
> and updating the memory reservation for the next kernel has been
> moved to of_kexec_alloc_and_setup_fdt() defined in "drivers/of/kexec.c".
>
> Use the common of_kexec_alloc_and_setup_fdt() to setup the device tree
> and update the memory reservation for kexec for arm64.
>
> Signed-off-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> ---
> arch/arm64/kernel/machine_kexec_file.c | 180 ++-----------------------
> 1 file changed, 8 insertions(+), 172 deletions(-)
Reviewed-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: Declaring unrecoverable_exception() as __noreturn ?
From: Nicholas Piggin @ 2021-02-11 0:44 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <5ecc1a9a-92eb-7006-6c94-2b7b700d182a@csgroup.eu>
Excerpts from Christophe Leroy's message of February 11, 2021 2:44 am:
> As far as I can see, almost all callers of unrecoverable_exception() expect it to never return.
>
> Can we mark it __noreturn ?
I don't see why not, do_exit is noreturn. We could make die() noreturn
as well.
>
> Below is interrupt_exit_kernel_prepare() with then without unrecoverable_exception() declared as
> __noreturn. (CONFIG_PREEMPT_NONE, and with the BUG_ON() removed)
>
> With the __noreturn added, we get no stack frame on the likely path
Nice!
Thanks,
Nick
>
> 000003a8 <interrupt_exit_kernel_prepare>:
> 3a8: 81 43 00 84 lwz r10,132(r3)
> 3ac: 71 4a 00 02 andi. r10,r10,2
> 3b0: 41 82 00 30 beq 3e0 <interrupt_exit_kernel_prepare+0x38>
> 3b4: 80 62 00 70 lwz r3,112(r2)
> 3b8: 74 63 00 01 andis. r3,r3,1
> 3bc: 40 82 00 34 bne 3f0 <interrupt_exit_kernel_prepare+0x48>
> 3c0: 7d 40 00 a6 mfmsr r10
> 3c4: 55 4a 04 5e rlwinm r10,r10,0,17,15
> 3c8: 7d 40 01 24 mtmsr r10
> 3cc: 7d 20 00 a6 mfmsr r9
> 3d0: 55 29 07 fa rlwinm r9,r9,0,31,29
> 3d4: 55 29 04 5e rlwinm r9,r9,0,17,15
> 3d8: 7d 20 01 24 mtmsr r9
> 3dc: 4e 80 00 20 blr
> 3e0: 94 21 ff f0 stwu r1,-16(r1)
> 3e4: 7c 08 02 a6 mflr r0
> 3e8: 90 01 00 14 stw r0,20(r1)
> 3ec: 48 00 00 01 bl 3ec <interrupt_exit_kernel_prepare+0x44>
> 3ec: R_PPC_REL24 unrecoverable_exception
> 3f0: 38 e2 00 70 addi r7,r2,112
> 3f4: 3d 00 00 01 lis r8,1
> 3f8: 7c c0 38 28 lwarx r6,0,r7
> 3fc: 7c c6 40 78 andc r6,r6,r8
> 400: 7c c0 39 2d stwcx. r6,0,r7
> 404: 40 a2 ff f4 bne 3f8 <interrupt_exit_kernel_prepare+0x50>
> 408: 38 60 00 01 li r3,1
> 40c: 4b ff ff b4 b 3c0 <interrupt_exit_kernel_prepare+0x18>
>
> Without the modification:
>
> 000003a8 <interrupt_exit_kernel_prepare>:
> 3a8: 94 21 ff f0 stwu r1,-16(r1)
> 3ac: 93 e1 00 0c stw r31,12(r1)
> 3b0: 81 23 00 84 lwz r9,132(r3)
> 3b4: 71 29 00 02 andi. r9,r9,2
> 3b8: 41 82 00 38 beq 3f0 <interrupt_exit_kernel_prepare+0x48>
> 3bc: 81 22 00 70 lwz r9,112(r2)
> 3c0: 75 23 00 01 andis. r3,r9,1
> 3c4: 40 82 00 4c bne 410 <interrupt_exit_kernel_prepare+0x68>
> 3c8: 7d 20 00 a6 mfmsr r9
> 3cc: 55 29 04 5e rlwinm r9,r9,0,17,15
> 3d0: 7d 20 01 24 mtmsr r9
> 3d4: 7d 20 00 a6 mfmsr r9
> 3d8: 55 29 07 fa rlwinm r9,r9,0,31,29
> 3dc: 55 29 04 5e rlwinm r9,r9,0,17,15
> 3e0: 7d 20 01 24 mtmsr r9
> 3e4: 83 e1 00 0c lwz r31,12(r1)
> 3e8: 38 21 00 10 addi r1,r1,16
> 3ec: 4e 80 00 20 blr
> 3f0: 7c 08 02 a6 mflr r0
> 3f4: 90 01 00 14 stw r0,20(r1)
> 3f8: 48 00 00 01 bl 3f8 <interrupt_exit_kernel_prepare+0x50>
> 3f8: R_PPC_REL24 unrecoverable_exception
> 3fc: 81 22 00 70 lwz r9,112(r2)
> 400: 80 01 00 14 lwz r0,20(r1)
> 404: 75 23 00 01 andis. r3,r9,1
> 408: 7c 08 03 a6 mtlr r0
> 40c: 41 82 ff bc beq 3c8 <interrupt_exit_kernel_prepare+0x20>
> 410: 39 02 00 70 addi r8,r2,112
> 414: 3d 20 00 01 lis r9,1
> 418: 7c e0 40 28 lwarx r7,0,r8
> 41c: 7c e7 48 78 andc r7,r7,r9
> 420: 7c e0 41 2d stwcx. r7,0,r8
> 424: 40 a2 ff f4 bne 418 <interrupt_exit_kernel_prepare+0x70>
> 428: 38 60 00 01 li r3,1
> 42c: 7d 20 00 a6 mfmsr r9
> 430: 55 29 04 5e rlwinm r9,r9,0,17,15
> 434: 7d 20 01 24 mtmsr r9
> 438: 7d 20 00 a6 mfmsr r9
> 43c: 55 29 07 fa rlwinm r9,r9,0,31,29
> 440: 55 29 04 5e rlwinm r9,r9,0,17,15
> 444: 7d 20 01 24 mtmsr r9
> 448: 83 e1 00 0c lwz r31,12(r1)
> 44c: 38 21 00 10 addi r1,r1,16
> 450: 4e 80 00 20 blr
>
^ permalink raw reply
* Re: [PATCH v5 03/10] powerpc/signal64: Move non-inline functions out of setup_sigcontext()
From: Christopher M. Riedl @ 2021-02-10 23:51 UTC (permalink / raw)
To: Daniel Axtens, linuxppc-dev
In-Reply-To: <87a6sbeipz.fsf@dja-thinkpad.axtens.net>
On Wed Feb 10, 2021 at 3:06 PM CST, Daniel Axtens wrote:
> "Christopher M. Riedl" <cmr@codefail.de> writes:
>
> > On Sun Feb 7, 2021 at 10:44 PM CST, Daniel Axtens wrote:
> >> Hi Chris,
> >>
> >> These two paragraphs are a little confusing and they seem slightly
> >> repetitive. But I get the general idea. Two specific comments below:
> >
> > Umm... yeah only one of those was supposed to be sent. I will reword
> > this for the next spin and address the comment below about how it is
> > not entirely clear that the inline functions are being moved out.
> >
> >>
> >> > There are non-inline functions which get called in setup_sigcontext() to
> >> > save register state to the thread struct. Move these functions into a
> >> > separate prepare_setup_sigcontext() function so that
> >> > setup_sigcontext() can be refactored later into an "unsafe" version
> >> > which assumes an open uaccess window. Non-inline functions should be
> >> > avoided when uaccess is open.
> >>
> >> Why do we want to avoid non-inline functions? We came up with:
> >>
> >> - we want KUAP protection for as much of the kernel as possible: each
> >> extra bit of code run with the window open is another piece of attack
> >> surface.
> >>
> >> - non-inline functions default to traceable, which means we could end
> >> up ftracing while uaccess is enabled. That's a pretty big hole in the
> >> defences that KUAP provides.
> >>
> >> I think we've also had problems with the window being opened or closed
> >> unexpectedly by various bits of code? So the less code runs in uaccess
> >> context the less likely that is to occur.
> >
> > That is my understanding as well.
> >
> >>
> >> > The majority of setup_sigcontext() can be refactored to execute in an
> >> > "unsafe" context (uaccess window is opened) except for some non-inline
> >> > functions. Move these out into a separate prepare_setup_sigcontext()
> >> > function which must be called first and before opening up a uaccess
> >> > window. A follow-up commit converts setup_sigcontext() to be "unsafe".
> >>
> >> This was a bit confusing until we realise that you're moving the _calls_
> >> to the non-inline functions out, not the non-inline functions
> >> themselves.
> >>
> >> > Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> >> > ---
> >> > arch/powerpc/kernel/signal_64.c | 32 +++++++++++++++++++++-----------
> >> > 1 file changed, 21 insertions(+), 11 deletions(-)
> >> >
> >> > diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> >> > index f9e4a1ac440f..b211a8ea4f6e 100644
> >> > --- a/arch/powerpc/kernel/signal_64.c
> >> > +++ b/arch/powerpc/kernel/signal_64.c
> >> > @@ -79,6 +79,24 @@ static elf_vrreg_t __user *sigcontext_vmx_regs(struct sigcontext __user *sc)
> >> > }
> >> > #endif
> >> >
> >> > +static void prepare_setup_sigcontext(struct task_struct *tsk, int ctx_has_vsx_region)
> >>
> >> ctx_has_vsx_region should probably be a bool? Although setup_sigcontext
> >> also has it as an int so I guess that's arguable, and maybe it's better
> >> to stick with this for constency.
> >
> > I've been told not to introduce unrelated changes in my patches before
> > so chose to keep this as an int for consistency.
>
> Seems reasonable.
>
> >
> >>
> >> > +{
> >> > +#ifdef CONFIG_ALTIVEC
> >> > + /* save altivec registers */
> >> > + if (tsk->thread.used_vr)
> >> > + flush_altivec_to_thread(tsk);
> >> > + if (cpu_has_feature(CPU_FTR_ALTIVEC))
> >> > + tsk->thread.vrsave = mfspr(SPRN_VRSAVE);
> >> > +#endif /* CONFIG_ALTIVEC */
> >> > +
> >> > + flush_fp_to_thread(tsk);
> >> > +
> >> > +#ifdef CONFIG_VSX
> >> > + if (tsk->thread.used_vsr && ctx_has_vsx_region)
> >> > + flush_vsx_to_thread(tsk);
> >> > +#endif /* CONFIG_VSX */
> >>
> >> Alternatively, given that this is the only use of ctx_has_vsx_region,
> >> mpe suggested that perhaps we could drop it entirely and always
> >> flush_vsx if used_vsr. The function is only ever called with either
> >> `current` or wth ctx_has_vsx_region set to 1, so in either case I think
> >> that's safe? I'm not sure if it would have performance implications.
> >
> > I think that could work as long as we can guarantee that the context
> > passed to swapcontext will always be sufficiently sized if used_vsr,
> > which I think *has* to be the case?
>
> I think you're always guaranteed that you'll have a big enough one
> in your kernel thread, which is what you end up writing to, iiuc?
Ah yup you are correct. I confused myself with the comment in
swapcontext about the ctx_size. We call prepare_setup_sigcontext() with
current which will always have space. The ctx_size only matters on the
next call to setup_sigcontext() which ends up potentially copying the
VSX region to userspace (v_regs).
TL;DR - yes, I'll remove the ctx_has_vsx_region argument to
prepare_setup_sigcontext() with the next version. Thanks!
>
> >>
> >> Should we move this and the altivec ifdef to IS_ENABLED(CONFIG_VSX) etc?
> >> I'm not sure if that runs into any problems with things like 'used_vsr'
> >> only being defined if CONFIG_VSX is set, but I thought I'd ask.
> >
> > That's why I didn't use IS_ENABLED(CONFIG_...) here - all of these
> > field (used_vr, vrsave, used_vsr) declarations are guarded by #ifdefs :/
>
> Dang. Oh well.
> >
> >>
> >>
> >> > +}
> >> > +
> >> > /*
> >> > * Set up the sigcontext for the signal frame.
> >> > */
> >> > @@ -97,7 +115,6 @@ static long setup_sigcontext(struct sigcontext __user *sc,
> >> > */
> >> > #ifdef CONFIG_ALTIVEC
> >> > elf_vrreg_t __user *v_regs = sigcontext_vmx_regs(sc);
> >> > - unsigned long vrsave;
> >> > #endif
> >> > struct pt_regs *regs = tsk->thread.regs;
> >> > unsigned long msr = regs->msr;
> >> > @@ -112,7 +129,6 @@ static long setup_sigcontext(struct sigcontext __user *sc,
> >> >
> >> > /* save altivec registers */
> >> > if (tsk->thread.used_vr) {
> >> > - flush_altivec_to_thread(tsk);
> >> > /* Copy 33 vec registers (vr0..31 and vscr) to the stack */
> >> > err |= __copy_to_user(v_regs, &tsk->thread.vr_state,
> >> > 33 * sizeof(vector128));
> >> > @@ -124,17 +140,10 @@ static long setup_sigcontext(struct sigcontext __user *sc,
> >> > /* We always copy to/from vrsave, it's 0 if we don't have or don't
> >> > * use altivec.
> >> > */
> >> > - vrsave = 0;
> >> > - if (cpu_has_feature(CPU_FTR_ALTIVEC)) {
> >> > - vrsave = mfspr(SPRN_VRSAVE);
> >> > - tsk->thread.vrsave = vrsave;
> >> > - }
> >> > -
> >> > - err |= __put_user(vrsave, (u32 __user *)&v_regs[33]);
> >> > + err |= __put_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33]);
> >>
> >> Previously, if !cpu_has_feature(ALTIVEC), v_regs[33] had vrsave stored,
> >> which was set to 0 explicitly. Now we store thread.vrsave instead of the
> >> local vrsave. That should be safe - it is initalised to 0 elsewhere.
> >>
> >> So you don't have to do anything here, this is just letting you know
> >> that we checked it and thought about it.
> >
> > Thanks! I thought about adding a comment/note here as I had to convince
> > myself that thread.vrsave is indeed initialized to 0 before making this
> > change as well. I will mention it in the word-smithed commit message for
> > posterity.
> >
> >>
> >> > #else /* CONFIG_ALTIVEC */
> >> > err |= __put_user(0, &sc->v_regs);
> >> > #endif /* CONFIG_ALTIVEC */
> >> > - flush_fp_to_thread(tsk);
> >> > /* copy fpr regs and fpscr */
> >> > err |= copy_fpr_to_user(&sc->fp_regs, tsk);
> >> >
> >> > @@ -150,7 +159,6 @@ static long setup_sigcontext(struct sigcontext __user *sc,
> >> > * VMX data.
> >> > */
> >> > if (tsk->thread.used_vsr && ctx_has_vsx_region) {
> >> > - flush_vsx_to_thread(tsk);
> >> > v_regs += ELF_NVRREG;
> >> > err |= copy_vsx_to_user(v_regs, tsk);
> >> > /* set MSR_VSX in the MSR value in the frame to
> >> > @@ -655,6 +663,7 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
> >> > ctx_has_vsx_region = 1;
> >> >
> >> > if (old_ctx != NULL) {
> >> > + prepare_setup_sigcontext(current, ctx_has_vsx_region);
> >> > if (!access_ok(old_ctx, ctx_size)
> >> > || setup_sigcontext(&old_ctx->uc_mcontext, current, 0, NULL, 0,
> >> > ctx_has_vsx_region)
> >>
> >> I had a think about whether there was a problem with bubbling
> >> prepare_setup_sigcontext over the access_ok() test, but given that
> >> prepare_setup_sigcontext(current ...) doesn't access any of old_ctx, I'm
> >> satisfied that it's OK - no changes needed.
> >
> > Not sure I understand what you mean by 'bubbling over'?
>
>
> Yeah sorry, overly flowery language there. I mean that the accesses that
> prepare_setup_sigcontext does have moved up - like a bubble in fluid -
> from after access_ok to before access_ok.
>
> Kind regards,
> Daniel
> >>
> >>
> >> > @@ -842,6 +851,7 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
> >> > #endif
> >> > {
> >> > err |= __put_user(0, &frame->uc.uc_link);
> >> > + prepare_setup_sigcontext(tsk, 1);
> >>
> >> Why do we call with ctx_has_vsx_region = 1 here? It's not immediately
> >> clear to me why this is correct, but mpe and Mikey seem pretty convinced
> >> that it is.
> >
> > I think it's because we always have a "complete" sigcontext w/ the VSX
> > save area here, unlike in swapcontext where we have to check. Also, the
> > following unsafe_setup_sigcontext() is called with ctx_has_vsx_region=1
> > so assumes that the VSX data was copied by prepare_setup_sigcontext().
> >
> >>
> >> > err |= setup_sigcontext(&frame->uc.uc_mcontext, tsk, ksig->sig,
> >> > NULL, (unsigned long)ksig->ka.sa.sa_handler,
> >> > 1);
> >>
> >>
> >> Finally, it's a bit hard to figure out where to put this, but we spent
> >> some time making sure that the various things you moved into the
> >> prepare_setup_sigcontext() function were called under the same
> >> circumstances as they were before, and there were no concerns there.
> >
> > Thanks for reviewing and double checking my work :)
> >
> >>
> >> Kind regards,
> >> Daniel
^ permalink raw reply
* Re: [PATCH v17 04/10] powerpc: Use common of_kexec_alloc_and_setup_fdt()
From: Thiago Jung Bauermann @ 2021-02-11 1:42 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: mark.rutland, tao.li, zohar, paulus, vincenzo.frascino,
frowand.list, sashal, robh, masahiroy, jmorris, takahiro.akashi,
linux-arm-kernel, catalin.marinas, serge, devicetree,
pasha.tatashin, will, prsriva, hsinyi, allison, christophe.leroy,
mbrugger, balajib, dmitry.kasatkin, linux-kernel, james.morse,
gregkh, joe, linux-integrity, linuxppc-dev
In-Reply-To: <20210209182200.30606-5-nramas@linux.microsoft.com>
Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes:
> From: Rob Herring <robh@kernel.org>
>
> The code for setting up the /chosen node in the device tree
> and updating the memory reservation for the next kernel has been
> moved to of_kexec_alloc_and_setup_fdt() defined in "drivers/of/kexec.c".
>
> Use the common of_kexec_alloc_and_setup_fdt() to setup the device tree
> and update the memory reservation for kexec for powerpc.
>
> Signed-off-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> ---
> arch/powerpc/include/asm/kexec.h | 1 +
> arch/powerpc/kexec/elf_64.c | 29 ++++---
> arch/powerpc/kexec/file_load.c | 132 +-----------------------------
> arch/powerpc/kexec/file_load_64.c | 3 +
> 4 files changed, 25 insertions(+), 140 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/kexec.h b/arch/powerpc/include/asm/kexec.h
> index dbf09d2f36d0..bdd0ddb9ac4d 100644
> --- a/arch/powerpc/include/asm/kexec.h
> +++ b/arch/powerpc/include/asm/kexec.h
> @@ -111,6 +111,7 @@ struct kimage_arch {
> unsigned long elf_headers_mem;
> unsigned long elf_headers_sz;
> void *elf_headers;
> + void *fdt;
>
> #ifdef CONFIG_IMA_KEXEC
> phys_addr_t ima_buffer_addr;
> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
> index d0e459bb2f05..bfabd06f99b1 100644
> --- a/arch/powerpc/kexec/elf_64.c
> +++ b/arch/powerpc/kexec/elf_64.c
> @@ -19,6 +19,7 @@
> #include <linux/kexec.h>
> #include <linux/libfdt.h>
> #include <linux/module.h>
> +#include <linux/of.h>
> #include <linux/of_fdt.h>
> #include <linux/slab.h>
> #include <linux/types.h>
> @@ -29,7 +30,6 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
> unsigned long cmdline_len)
> {
> int ret;
> - unsigned int fdt_size;
> unsigned long kernel_load_addr;
> unsigned long initrd_load_addr = 0, fdt_load_addr;
> void *fdt;
> @@ -102,19 +102,13 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
> pr_debug("Loaded initrd at 0x%lx\n", initrd_load_addr);
> }
>
> - fdt_size = fdt_totalsize(initial_boot_params) * 2;
> - fdt = kmalloc(fdt_size, GFP_KERNEL);
> + fdt = of_kexec_alloc_and_setup_fdt(image, initrd_load_addr,
> + initrd_len, cmdline);
> if (!fdt) {
> pr_err("Not enough memory for the device tree.\n");
This error string can be a bit misleading now, since
of_kexec_alloc_and_setup_fdt() can fail for reasons other than lack of
memory. I suggest changing it to the error string from fdt_open_into()
below:
pr_err("Error setting up the new device tree.\n");
With this change:
Reviewed-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
And also:
Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH v17 04/10] powerpc: Use common of_kexec_alloc_and_setup_fdt()
From: Lakshmi Ramasubramanian @ 2021-02-11 1:50 UTC (permalink / raw)
To: Thiago Jung Bauermann
Cc: mark.rutland, tao.li, zohar, paulus, vincenzo.frascino,
frowand.list, sashal, robh, masahiroy, jmorris, takahiro.akashi,
linux-arm-kernel, catalin.marinas, serge, devicetree,
pasha.tatashin, will, prsriva, hsinyi, allison, christophe.leroy,
mbrugger, balajib, dmitry.kasatkin, linux-kernel, james.morse,
gregkh, joe, linux-integrity, linuxppc-dev
In-Reply-To: <87sg63nzwc.fsf@manicouagan.localdomain>
On 2/10/21 5:42 PM, Thiago Jung Bauermann wrote:
>
> Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes:
>
>> From: Rob Herring <robh@kernel.org>
>>
>> The code for setting up the /chosen node in the device tree
>> and updating the memory reservation for the next kernel has been
>> moved to of_kexec_alloc_and_setup_fdt() defined in "drivers/of/kexec.c".
>>
>> Use the common of_kexec_alloc_and_setup_fdt() to setup the device tree
>> and update the memory reservation for kexec for powerpc.
>>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
>> ---
>> arch/powerpc/include/asm/kexec.h | 1 +
>> arch/powerpc/kexec/elf_64.c | 29 ++++---
>> arch/powerpc/kexec/file_load.c | 132 +-----------------------------
>> arch/powerpc/kexec/file_load_64.c | 3 +
>> 4 files changed, 25 insertions(+), 140 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/kexec.h b/arch/powerpc/include/asm/kexec.h
>> index dbf09d2f36d0..bdd0ddb9ac4d 100644
>> --- a/arch/powerpc/include/asm/kexec.h
>> +++ b/arch/powerpc/include/asm/kexec.h
>> @@ -111,6 +111,7 @@ struct kimage_arch {
>> unsigned long elf_headers_mem;
>> unsigned long elf_headers_sz;
>> void *elf_headers;
>> + void *fdt;
>>
>> #ifdef CONFIG_IMA_KEXEC
>> phys_addr_t ima_buffer_addr;
>> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
>> index d0e459bb2f05..bfabd06f99b1 100644
>> --- a/arch/powerpc/kexec/elf_64.c
>> +++ b/arch/powerpc/kexec/elf_64.c
>> @@ -19,6 +19,7 @@
>> #include <linux/kexec.h>
>> #include <linux/libfdt.h>
>> #include <linux/module.h>
>> +#include <linux/of.h>
>> #include <linux/of_fdt.h>
>> #include <linux/slab.h>
>> #include <linux/types.h>
>> @@ -29,7 +30,6 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
>> unsigned long cmdline_len)
>> {
>> int ret;
>> - unsigned int fdt_size;
>> unsigned long kernel_load_addr;
>> unsigned long initrd_load_addr = 0, fdt_load_addr;
>> void *fdt;
>> @@ -102,19 +102,13 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
>> pr_debug("Loaded initrd at 0x%lx\n", initrd_load_addr);
>> }
>>
>> - fdt_size = fdt_totalsize(initial_boot_params) * 2;
>> - fdt = kmalloc(fdt_size, GFP_KERNEL);
>> + fdt = of_kexec_alloc_and_setup_fdt(image, initrd_load_addr,
>> + initrd_len, cmdline);
>> if (!fdt) {
>> pr_err("Not enough memory for the device tree.\n");
>
> This error string can be a bit misleading now, since
> of_kexec_alloc_and_setup_fdt() can fail for reasons other than lack of
> memory. I suggest changing it to the error string from fdt_open_into()
> below:
>
> pr_err("Error setting up the new device tree.\n");
>
> With this change:
Agreed - I will make this change.
>
> Reviewed-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
>
> And also:
>
> Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
>
Thanks a lot for your help Thiago.
-lakshmi
^ permalink raw reply
* Re: [PATCH v17 06/10] powerpc: Enable passing IMA log to next kernel on kexec
From: Thiago Jung Bauermann @ 2021-02-11 1:51 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: mark.rutland, tao.li, zohar, paulus, vincenzo.frascino,
frowand.list, sashal, robh, masahiroy, jmorris, takahiro.akashi,
linux-arm-kernel, catalin.marinas, serge, devicetree,
pasha.tatashin, will, prsriva, hsinyi, allison, christophe.leroy,
mbrugger, balajib, dmitry.kasatkin, linux-kernel, james.morse,
gregkh, joe, linux-integrity, linuxppc-dev
In-Reply-To: <20210209182200.30606-7-nramas@linux.microsoft.com>
Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes:
> CONFIG_HAVE_IMA_KEXEC is enabled to indicate that the IMA measurement
> log information is present in the device tree. This should be selected
> only if CONFIG_IMA is enabled.
>
> Update CONFIG_KEXEC_FILE to select CONFIG_HAVE_IMA_KEXEC, if CONFIG_IMA
> is enabled, to indicate that the IMA measurement log information is
> present in the device tree for powerpc.
>
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> Suggested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> ---
> arch/powerpc/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* [powerpc:next-test] BUILD SUCCESS ea721ec55c8a4a166373978b9c8ce77374d684d6
From: kernel test robot @ 2021-02-11 2:22 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next-test
branch HEAD: ea721ec55c8a4a166373978b9c8ce77374d684d6 selftests/powerpc: Test for spurious kernel memory faults on radix
elapsed time: 822m
configs tested: 195
configs skipped: 2
The following configs have been built successfully.
More configs may be tested in the coming days.
gcc tested configs:
arm defconfig
arm64 allyesconfig
arm64 defconfig
arm allyesconfig
arm allmodconfig
arm shannon_defconfig
sh rsk7203_defconfig
sparc defconfig
mips bmips_stb_defconfig
arm alldefconfig
arm sama5_defconfig
riscv nommu_virt_defconfig
m68k mvme147_defconfig
arm badge4_defconfig
sh dreamcast_defconfig
openrisc alldefconfig
arm bcm2835_defconfig
powerpc klondike_defconfig
c6x dsk6455_defconfig
mips malta_defconfig
powerpc cell_defconfig
powerpc sam440ep_defconfig
m68k amiga_defconfig
sh apsh4ad0a_defconfig
arm tegra_defconfig
mips decstation_r4k_defconfig
sh defconfig
powerpc powernv_defconfig
arm palmz72_defconfig
mips bmips_be_defconfig
mips bcm47xx_defconfig
openrisc or1ksim_defconfig
openrisc or1klitex_defconfig
powerpc obs600_defconfig
m68k m5249evb_defconfig
powerpc mpc7448_hpc2_defconfig
arc nsimosci_defconfig
nios2 3c120_defconfig
arm s3c6400_defconfig
arm vf610m4_defconfig
arc alldefconfig
c6x evmc6474_defconfig
arc haps_hs_smp_defconfig
mips bigsur_defconfig
mips maltaaprp_defconfig
arc axs103_defconfig
mips tb0219_defconfig
m68k q40_defconfig
mips qi_lb60_defconfig
arm sunxi_defconfig
powerpc mpc834x_itx_defconfig
arm socfpga_defconfig
mips db1xxx_defconfig
m68k sun3_defconfig
powerpc mpc834x_itxgp_defconfig
powerpc64 defconfig
mips xway_defconfig
mips e55_defconfig
sh sh7785lcr_32bit_defconfig
mips nlm_xlp_defconfig
xtensa virt_defconfig
arm magician_defconfig
mips ip32_defconfig
powerpc tqm8540_defconfig
sh kfr2r09_defconfig
nios2 10m50_defconfig
powerpc mpc885_ads_defconfig
arc haps_hs_defconfig
arm prima2_defconfig
powerpc g5_defconfig
arm colibri_pxa300_defconfig
sh rsk7269_defconfig
xtensa nommu_kc705_defconfig
powerpc allnoconfig
powerpc bamboo_defconfig
arm colibri_pxa270_defconfig
sh shx3_defconfig
sh allmodconfig
xtensa audio_kc705_defconfig
arm vt8500_v6_v7_defconfig
arm realview_defconfig
arc vdk_hs38_defconfig
mips rs90_defconfig
powerpc sequoia_defconfig
powerpc taishan_defconfig
alpha defconfig
mips tb0287_defconfig
powerpc socrates_defconfig
mips ar7_defconfig
powerpc sbc8548_defconfig
powerpc tqm8555_defconfig
powerpc allmodconfig
powerpc ps3_defconfig
powerpc walnut_defconfig
powerpc mvme5100_defconfig
sh microdev_defconfig
mips loongson1c_defconfig
nds32 alldefconfig
sh landisk_defconfig
arm corgi_defconfig
arm at91_dt_defconfig
ia64 bigsur_defconfig
arm integrator_defconfig
sh se7750_defconfig
sh sh7763rdp_defconfig
m68k m5275evb_defconfig
sparc64 alldefconfig
powerpc mpc836x_mds_defconfig
nios2 alldefconfig
mips ip27_defconfig
powerpc katmai_defconfig
xtensa common_defconfig
riscv alldefconfig
arm vexpress_defconfig
m68k mac_defconfig
arm pxa_defconfig
powerpc mpc832x_rdb_defconfig
sh se7751_defconfig
ia64 gensparse_defconfig
m68k alldefconfig
m68k apollo_defconfig
m68k bvme6000_defconfig
mips ci20_defconfig
ia64 allmodconfig
ia64 defconfig
ia64 allyesconfig
m68k allmodconfig
m68k defconfig
m68k allyesconfig
nios2 defconfig
arc allyesconfig
nds32 allnoconfig
c6x allyesconfig
nds32 defconfig
nios2 allyesconfig
csky defconfig
alpha allyesconfig
xtensa allyesconfig
h8300 allyesconfig
arc defconfig
parisc defconfig
s390 allyesconfig
s390 allmodconfig
parisc allyesconfig
s390 defconfig
i386 allyesconfig
sparc allyesconfig
i386 tinyconfig
i386 defconfig
mips allyesconfig
mips allmodconfig
powerpc allyesconfig
x86_64 randconfig-a006-20210209
x86_64 randconfig-a001-20210209
x86_64 randconfig-a005-20210209
x86_64 randconfig-a004-20210209
x86_64 randconfig-a002-20210209
x86_64 randconfig-a003-20210209
i386 randconfig-a001-20210209
i386 randconfig-a005-20210209
i386 randconfig-a003-20210209
i386 randconfig-a002-20210209
i386 randconfig-a006-20210209
i386 randconfig-a004-20210209
i386 randconfig-a016-20210209
i386 randconfig-a013-20210209
i386 randconfig-a012-20210209
i386 randconfig-a014-20210209
i386 randconfig-a011-20210209
i386 randconfig-a015-20210209
i386 randconfig-a016-20210210
i386 randconfig-a014-20210210
i386 randconfig-a012-20210210
i386 randconfig-a013-20210210
i386 randconfig-a011-20210210
i386 randconfig-a015-20210210
riscv nommu_k210_defconfig
riscv allyesconfig
riscv allnoconfig
riscv defconfig
riscv rv32_defconfig
riscv allmodconfig
x86_64 rhel
x86_64 allyesconfig
x86_64 rhel-7.6-kselftests
x86_64 defconfig
x86_64 rhel-8.3
x86_64 rhel-8.3-kbuiltin
x86_64 kexec
clang tested configs:
x86_64 randconfig-a013-20210209
x86_64 randconfig-a014-20210209
x86_64 randconfig-a015-20210209
x86_64 randconfig-a012-20210209
x86_64 randconfig-a016-20210209
x86_64 randconfig-a011-20210209
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply
* Re: Declaring unrecoverable_exception() as __noreturn ?
From: Michael Ellerman @ 2021-02-11 4:41 UTC (permalink / raw)
To: Nicholas Piggin, Christophe Leroy, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <1613004125.9jpd8u2w0w.astroid@bobo.none>
Nicholas Piggin <npiggin@gmail.com> writes:
> Excerpts from Christophe Leroy's message of February 11, 2021 2:44 am:
>> As far as I can see, almost all callers of unrecoverable_exception() expect it to never return.
>>
>> Can we mark it __noreturn ?
>
> I don't see why not, do_exit is noreturn. We could make die() noreturn
> as well.
I'm always nervous about that, because we can return if a debugger is
involved:
DEFINE_INTERRUPT_HANDLER(unrecoverable_exception)
{
pr_emerg("Unrecoverable exception %lx at %lx (msr=%lx)\n",
regs->trap, regs->nip, regs->msr);
die("Unrecoverable exception", regs, SIGABRT);
}
void die(const char *str, struct pt_regs *regs, long err)
{
unsigned long flags;
/*
* system_reset_excption handles debugger, crash dump, panic, for 0x100
*/
if (TRAP(regs) != 0x100) {
if (debugger(regs))
return;
We obviously don't want to optimise for that case, but it worries me
slightly if we're marking things noreturn when they can actually return.
cheers
^ permalink raw reply
* Re: [PATCH v17 07/10] powerpc: Move arch independent ima kexec functions to drivers/of/kexec.c
From: Thiago Jung Bauermann @ 2021-02-11 5:07 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: mark.rutland, tao.li, zohar, paulus, vincenzo.frascino,
frowand.list, sashal, robh, masahiroy, jmorris, takahiro.akashi,
linux-arm-kernel, catalin.marinas, serge, devicetree,
pasha.tatashin, will, prsriva, hsinyi, allison, christophe.leroy,
mbrugger, balajib, dmitry.kasatkin, linux-kernel, james.morse,
gregkh, joe, linux-integrity, linuxppc-dev
In-Reply-To: <20210209182200.30606-8-nramas@linux.microsoft.com>
Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes:
> The functions defined in "arch/powerpc/kexec/ima.c" handle setting up
> and freeing the resources required to carry over the IMA measurement
> list from the current kernel to the next kernel across kexec system call.
> These functions do not have architecture specific code, but are
> currently limited to powerpc.
>
> Move remove_ima_buffer() and setup_ima_buffer() calls into
> of_kexec_alloc_and_setup_fdt() defined in "drivers/of/kexec.c".
>
> Move the remaining architecture independent functions from
> "arch/powerpc/kexec/ima.c" to "drivers/of/kexec.c".
> Delete "arch/powerpc/kexec/ima.c" and "arch/powerpc/include/asm/ima.h".
> Remove references to the deleted files and functions in powerpc and
> in ima.
>
> Co-developed-by: Prakhar Srivastava <prsriva@linux.microsoft.com>
> Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com>
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> ---
> arch/powerpc/include/asm/ima.h | 27 ----
> arch/powerpc/include/asm/kexec.h | 3 -
> arch/powerpc/kexec/Makefile | 7 -
> arch/powerpc/kexec/file_load.c | 25 ----
> arch/powerpc/kexec/file_load_64.c | 4 -
> arch/powerpc/kexec/ima.c | 202 -------------------------
> drivers/of/kexec.c | 239 ++++++++++++++++++++++++++++++
> include/linux/of.h | 2 +
> security/integrity/ima/ima.h | 4 -
> 9 files changed, 241 insertions(+), 272 deletions(-)
> delete mode 100644 arch/powerpc/include/asm/ima.h
> delete mode 100644 arch/powerpc/kexec/ima.c
Reviewed-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH v17 09/10] powerpc: Delete unused function delete_fdt_mem_rsv()
From: Thiago Jung Bauermann @ 2021-02-11 5:11 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: mark.rutland, tao.li, zohar, paulus, vincenzo.frascino,
frowand.list, sashal, robh, masahiroy, jmorris, takahiro.akashi,
linux-arm-kernel, catalin.marinas, serge, devicetree,
pasha.tatashin, will, prsriva, hsinyi, allison, christophe.leroy,
mbrugger, balajib, dmitry.kasatkin, linux-kernel, james.morse,
gregkh, joe, linux-integrity, linuxppc-dev
In-Reply-To: <20210209182200.30606-10-nramas@linux.microsoft.com>
Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes:
> delete_fdt_mem_rsv() defined in "arch/powerpc/kexec/file_load.c"
> has been renamed to fdt_find_and_del_mem_rsv(), and moved to
> "drivers/of/kexec.c".
>
> Remove delete_fdt_mem_rsv() in "arch/powerpc/kexec/file_load.c".
>
> Co-developed-by: Prakhar Srivastava <prsriva@linux.microsoft.com>
> Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com>
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> ---
> arch/powerpc/include/asm/kexec.h | 1 -
> arch/powerpc/kexec/file_load.c | 32 --------------------------------
> 2 files changed, 33 deletions(-)
Reviewed-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH v17 10/10] arm64: Enable passing IMA log to next kernel on kexec
From: Thiago Jung Bauermann @ 2021-02-11 5:13 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: mark.rutland, tao.li, zohar, paulus, vincenzo.frascino,
frowand.list, sashal, robh, masahiroy, jmorris, takahiro.akashi,
linux-arm-kernel, catalin.marinas, serge, devicetree,
pasha.tatashin, will, prsriva, hsinyi, allison, christophe.leroy,
mbrugger, balajib, dmitry.kasatkin, linux-kernel, james.morse,
gregkh, joe, linux-integrity, linuxppc-dev
In-Reply-To: <20210209182200.30606-11-nramas@linux.microsoft.com>
Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes:
> Update CONFIG_KEXEC_FILE to select CONFIG_HAVE_IMA_KEXEC, if CONFIG_IMA
> is enabled, to indicate that the IMA measurement log information is
> present in the device tree for ARM64.
>
> Co-developed-by: Prakhar Srivastava <prsriva@linux.microsoft.com>
> Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com>
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> Suggested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> ---
> arch/arm64/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
Reviewed-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: Declaring unrecoverable_exception() as __noreturn ?
From: Christophe Leroy @ 2021-02-11 6:13 UTC (permalink / raw)
To: Michael Ellerman, Nicholas Piggin, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <87mtwbnrlf.fsf@mpe.ellerman.id.au>
Le 11/02/2021 à 05:41, Michael Ellerman a écrit :
> Nicholas Piggin <npiggin@gmail.com> writes:
>> Excerpts from Christophe Leroy's message of February 11, 2021 2:44 am:
>>> As far as I can see, almost all callers of unrecoverable_exception() expect it to never return.
>>>
>>> Can we mark it __noreturn ?
>>
>> I don't see why not, do_exit is noreturn. We could make die() noreturn
>> as well.
>
> I'm always nervous about that, because we can return if a debugger is
> involved:
>
> DEFINE_INTERRUPT_HANDLER(unrecoverable_exception)
Hum ... Is that correct to define it as an interrupt handler ?
Also, I see it declared a second time in interrupt.c, this time not as an interrupt handler. Is that
wanted ?
> {
> pr_emerg("Unrecoverable exception %lx at %lx (msr=%lx)\n",
> regs->trap, regs->nip, regs->msr);
> die("Unrecoverable exception", regs, SIGABRT);
> }
>
> void die(const char *str, struct pt_regs *regs, long err)
> {
> unsigned long flags;
>
> /*
> * system_reset_excption handles debugger, crash dump, panic, for 0x100
> */
> if (TRAP(regs) != 0x100) {
> if (debugger(regs))
> return;
>
>
> We obviously don't want to optimise for that case, but it worries me
> slightly if we're marking things noreturn when they can actually return.
>
I don't think I want to declare die() as __noreturn, need to look at it more in details first.
Christophe
^ permalink raw reply
* [PATCH] powerpc/traps: Declare unrecoverable_exception() as __noreturn
From: Christophe Leroy @ 2021-02-11 6:34 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, npiggin
Cc: linuxppc-dev, linux-kernel
unrecoverable_exception() is never expected to return, most callers
have an infiniteloop in case it returns.
Ensure it really never returns by terminating it with a BUG(), and
declare it __no_return.
It always GCC to really simplify functions calling it. In the exemple below,
it avoids the stack frame in the likely fast path and avoids code duplication
for the exit.
With this patch:
00000348 <interrupt_exit_kernel_prepare>:
348: 81 43 00 84 lwz r10,132(r3)
34c: 71 48 00 02 andi. r8,r10,2
350: 41 82 00 2c beq 37c <interrupt_exit_kernel_prepare+0x34>
354: 71 4a 40 00 andi. r10,r10,16384
358: 40 82 00 20 bne 378 <interrupt_exit_kernel_prepare+0x30>
35c: 80 62 00 70 lwz r3,112(r2)
360: 74 63 00 01 andis. r3,r3,1
364: 40 82 00 28 bne 38c <interrupt_exit_kernel_prepare+0x44>
368: 7d 40 00 a6 mfmsr r10
36c: 7c 11 13 a6 mtspr 81,r0
370: 7c 12 13 a6 mtspr 82,r0
374: 4e 80 00 20 blr
378: 48 00 00 00 b 378 <interrupt_exit_kernel_prepare+0x30>
37c: 94 21 ff f0 stwu r1,-16(r1)
380: 7c 08 02 a6 mflr r0
384: 90 01 00 14 stw r0,20(r1)
388: 48 00 00 01 bl 388 <interrupt_exit_kernel_prepare+0x40>
388: R_PPC_REL24 unrecoverable_exception
38c: 38 e2 00 70 addi r7,r2,112
390: 3d 00 00 01 lis r8,1
394: 7c c0 38 28 lwarx r6,0,r7
398: 7c c6 40 78 andc r6,r6,r8
39c: 7c c0 39 2d stwcx. r6,0,r7
3a0: 40 a2 ff f4 bne 394 <interrupt_exit_kernel_prepare+0x4c>
3a4: 38 60 00 01 li r3,1
3a8: 4b ff ff c0 b 368 <interrupt_exit_kernel_prepare+0x20>
Without this patch:
00000348 <interrupt_exit_kernel_prepare>:
348: 94 21 ff f0 stwu r1,-16(r1)
34c: 93 e1 00 0c stw r31,12(r1)
350: 7c 7f 1b 78 mr r31,r3
354: 81 23 00 84 lwz r9,132(r3)
358: 71 2a 00 02 andi. r10,r9,2
35c: 41 82 00 34 beq 390 <interrupt_exit_kernel_prepare+0x48>
360: 71 29 40 00 andi. r9,r9,16384
364: 40 82 00 28 bne 38c <interrupt_exit_kernel_prepare+0x44>
368: 80 62 00 70 lwz r3,112(r2)
36c: 74 63 00 01 andis. r3,r3,1
370: 40 82 00 3c bne 3ac <interrupt_exit_kernel_prepare+0x64>
374: 7d 20 00 a6 mfmsr r9
378: 7c 11 13 a6 mtspr 81,r0
37c: 7c 12 13 a6 mtspr 82,r0
380: 83 e1 00 0c lwz r31,12(r1)
384: 38 21 00 10 addi r1,r1,16
388: 4e 80 00 20 blr
38c: 48 00 00 00 b 38c <interrupt_exit_kernel_prepare+0x44>
390: 7c 08 02 a6 mflr r0
394: 90 01 00 14 stw r0,20(r1)
398: 48 00 00 01 bl 398 <interrupt_exit_kernel_prepare+0x50>
398: R_PPC_REL24 unrecoverable_exception
39c: 80 01 00 14 lwz r0,20(r1)
3a0: 81 3f 00 84 lwz r9,132(r31)
3a4: 7c 08 03 a6 mtlr r0
3a8: 4b ff ff b8 b 360 <interrupt_exit_kernel_prepare+0x18>
3ac: 39 02 00 70 addi r8,r2,112
3b0: 3d 40 00 01 lis r10,1
3b4: 7c e0 40 28 lwarx r7,0,r8
3b8: 7c e7 50 78 andc r7,r7,r10
3bc: 7c e0 41 2d stwcx. r7,0,r8
3c0: 40 a2 ff f4 bne 3b4 <interrupt_exit_kernel_prepare+0x6c>
3c4: 38 60 00 01 li r3,1
3c8: 4b ff ff ac b 374 <interrupt_exit_kernel_prepare+0x2c>
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/include/asm/interrupt.h | 2 +-
arch/powerpc/kernel/interrupt.c | 1 -
arch/powerpc/kernel/traps.c | 2 ++
3 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
index dcff30e3919b..fa8bfb91f8df 100644
--- a/arch/powerpc/include/asm/interrupt.h
+++ b/arch/powerpc/include/asm/interrupt.h
@@ -411,7 +411,7 @@ DECLARE_INTERRUPT_HANDLER(altivec_assist_exception);
DECLARE_INTERRUPT_HANDLER(CacheLockingException);
DECLARE_INTERRUPT_HANDLER(SPEFloatingPointException);
DECLARE_INTERRUPT_HANDLER(SPEFloatingPointRoundException);
-DECLARE_INTERRUPT_HANDLER(unrecoverable_exception);
+DECLARE_INTERRUPT_HANDLER(unrecoverable_exception) __noreturn;
DECLARE_INTERRUPT_HANDLER(WatchdogException);
DECLARE_INTERRUPT_HANDLER(kernel_bad_stack);
diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
index eca3be36c18c..7e7106641ca9 100644
--- a/arch/powerpc/kernel/interrupt.c
+++ b/arch/powerpc/kernel/interrupt.c
@@ -440,7 +440,6 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned
return ret;
}
-void unrecoverable_exception(struct pt_regs *regs);
void preempt_schedule_irq(void);
notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsigned long msr)
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 2afa05ad21c8..1ff776e9e8e3 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -2173,6 +2173,8 @@ DEFINE_INTERRUPT_HANDLER(unrecoverable_exception)
pr_emerg("Unrecoverable exception %lx at %lx (msr=%lx)\n",
regs->trap, regs->nip, regs->msr);
die("Unrecoverable exception", regs, SIGABRT);
+ /* die() should not return */
+ BUG();
}
NOKPROBE_SYMBOL(unrecoverable_exception);
--
2.25.0
^ permalink raw reply related
* [PATCH] powerpc: remove interrupt handler functions from the noinstr section
From: Nicholas Piggin @ 2021-02-11 6:36 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Stephen Rothwell, Nicholas Piggin
The allyesconfig ppc64 kernel fails to link with relocations unable to
fit after commit 3a96570ffceb ("powerpc: convert interrupt handlers to
use wrappers"), which is due to the interrupt handler functions being
put into the .noinstr.text section, which the linker script places on
the opposite side of the main .text section from the interrupt entry
asm code which calls the handlers.
This results in a lot of linker stubs that overwhelm the 252-byte sized
space we allow for them, or in the case of BE a .opd relocation link
error for some reason.
It's not required to put interrupt handlers in the .noinstr section,
previously they used NOKPROBE_SYMBOL, so take them out and replace
with a NOKPROBE_SYMBOL in the wrapper macro. Remove the explicit
NOKPROBE_SYMBOL macros in the interrupt handler functions. This makes
a number of interrupt handlers nokprobe that were not prior to the
interrupt wrappers commit, but since that commit they were made
nokprobe due to being in .noinstr.text, so this fix does not change
that.
The fixes tag is different to the commit that first exposes the problem
because it is where the wrapper macros were introduced.
Fixes: 8d41fc618ab8 ("powerpc: interrupt handler wrapper functions")
Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/interrupt.h | 25 ++++++++++++++++++++-----
arch/powerpc/kernel/traps.c | 9 ---------
arch/powerpc/mm/fault.c | 1 -
3 files changed, 20 insertions(+), 15 deletions(-)
diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
index 4badb3e51c19..ffb568587553 100644
--- a/arch/powerpc/include/asm/interrupt.h
+++ b/arch/powerpc/include/asm/interrupt.h
@@ -6,6 +6,7 @@
#include <linux/hardirq.h>
#include <asm/cputime.h>
#include <asm/ftrace.h>
+#include <asm/kprobes.h>
#include <asm/runlatch.h>
struct interrupt_state {
@@ -164,6 +165,15 @@ static inline void interrupt_nmi_exit_prepare(struct pt_regs *regs, struct inter
#endif
}
+/*
+ * Don't use like to use noinstr here like x86, but rather add NOKPROBE_SYMBOL
+ * to each function definition. The reason for this is the noinstr section
+ * is placed after the main text section, i.e., very far away from the
+ * interrupt entry asm. That creates problems with fitting linker stubs when
+ * building large kernels.
+ */
+#define interrupt_handler __visible noinline notrace __no_kcsan __no_sanitize_address
+
/**
* DECLARE_INTERRUPT_HANDLER_RAW - Declare raw interrupt handler function
* @func: Function name of the entry point
@@ -198,7 +208,7 @@ static inline void interrupt_nmi_exit_prepare(struct pt_regs *regs, struct inter
#define DEFINE_INTERRUPT_HANDLER_RAW(func) \
static __always_inline long ____##func(struct pt_regs *regs); \
\
-__visible noinstr long func(struct pt_regs *regs) \
+interrupt_handler long func(struct pt_regs *regs) \
{ \
long ret; \
\
@@ -206,6 +216,7 @@ __visible noinstr long func(struct pt_regs *regs) \
\
return ret; \
} \
+NOKPROBE_SYMBOL(func); \
\
static __always_inline long ____##func(struct pt_regs *regs)
@@ -228,7 +239,7 @@ static __always_inline long ____##func(struct pt_regs *regs)
#define DEFINE_INTERRUPT_HANDLER(func) \
static __always_inline void ____##func(struct pt_regs *regs); \
\
-__visible noinstr void func(struct pt_regs *regs) \
+interrupt_handler void func(struct pt_regs *regs) \
{ \
struct interrupt_state state; \
\
@@ -238,6 +249,7 @@ __visible noinstr void func(struct pt_regs *regs) \
\
interrupt_exit_prepare(regs, &state); \
} \
+NOKPROBE_SYMBOL(func); \
\
static __always_inline void ____##func(struct pt_regs *regs)
@@ -262,7 +274,7 @@ static __always_inline void ____##func(struct pt_regs *regs)
#define DEFINE_INTERRUPT_HANDLER_RET(func) \
static __always_inline long ____##func(struct pt_regs *regs); \
\
-__visible noinstr long func(struct pt_regs *regs) \
+interrupt_handler long func(struct pt_regs *regs) \
{ \
struct interrupt_state state; \
long ret; \
@@ -275,6 +287,7 @@ __visible noinstr long func(struct pt_regs *regs) \
\
return ret; \
} \
+NOKPROBE_SYMBOL(func); \
\
static __always_inline long ____##func(struct pt_regs *regs)
@@ -297,7 +310,7 @@ static __always_inline long ____##func(struct pt_regs *regs)
#define DEFINE_INTERRUPT_HANDLER_ASYNC(func) \
static __always_inline void ____##func(struct pt_regs *regs); \
\
-__visible noinstr void func(struct pt_regs *regs) \
+interrupt_handler void func(struct pt_regs *regs) \
{ \
struct interrupt_state state; \
\
@@ -307,6 +320,7 @@ __visible noinstr void func(struct pt_regs *regs) \
\
interrupt_async_exit_prepare(regs, &state); \
} \
+NOKPROBE_SYMBOL(func); \
\
static __always_inline void ____##func(struct pt_regs *regs)
@@ -331,7 +345,7 @@ static __always_inline void ____##func(struct pt_regs *regs)
#define DEFINE_INTERRUPT_HANDLER_NMI(func) \
static __always_inline long ____##func(struct pt_regs *regs); \
\
-__visible noinstr long func(struct pt_regs *regs) \
+interrupt_handler long func(struct pt_regs *regs) \
{ \
struct interrupt_nmi_state state; \
long ret; \
@@ -344,6 +358,7 @@ __visible noinstr long func(struct pt_regs *regs) \
\
return ret; \
} \
+NOKPROBE_SYMBOL(func); \
\
static __always_inline long ____##func(struct pt_regs *regs)
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 39c8b7e9b91a..1583fd1c6010 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -513,7 +513,6 @@ DEFINE_INTERRUPT_HANDLER_NMI(system_reset_exception)
return 0;
}
-NOKPROBE_SYMBOL(system_reset_exception);
/*
* I/O accesses can cause machine checks on powermacs.
@@ -798,7 +797,6 @@ void die_mce(const char *str, struct pt_regs *regs, long err)
nmi_exit();
die(str, regs, err);
}
-NOKPROBE_SYMBOL(die_mce);
/*
* BOOK3S_64 does not call this handler as a non-maskable interrupt
@@ -851,7 +849,6 @@ DEFINE_INTERRUPT_HANDLER_NMI(machine_check_exception)
return 0;
#endif
}
-NOKPROBE_SYMBOL(machine_check_exception);
DEFINE_INTERRUPT_HANDLER(SMIException) /* async? */
{
@@ -1113,7 +1110,6 @@ DEFINE_INTERRUPT_HANDLER(single_step_exception)
_exception(SIGTRAP, regs, TRAP_TRACE, regs->nip);
}
-NOKPROBE_SYMBOL(single_step_exception);
/*
* After we have successfully emulated an instruction, we have to
@@ -1556,7 +1552,6 @@ DEFINE_INTERRUPT_HANDLER(program_check_exception)
{
do_program_check(regs);
}
-NOKPROBE_SYMBOL(program_check_exception);
/*
* This occurs when running in hypervisor mode on POWER6 or later
@@ -1567,7 +1562,6 @@ DEFINE_INTERRUPT_HANDLER(emulation_assist_interrupt)
regs->msr |= REASON_ILLEGAL;
do_program_check(regs);
}
-NOKPROBE_SYMBOL(emulation_assist_interrupt);
DEFINE_INTERRUPT_HANDLER(alignment_exception)
{
@@ -2034,7 +2028,6 @@ DEFINE_INTERRUPT_HANDLER(DebugException)
} else
handle_debug(regs, debug_status);
}
-NOKPROBE_SYMBOL(DebugException);
#endif /* CONFIG_PPC_ADV_DEBUG_REGS */
#ifdef CONFIG_ALTIVEC
@@ -2183,7 +2176,6 @@ DEFINE_INTERRUPT_HANDLER(unrecoverable_exception)
regs->trap, regs->nip, regs->msr);
die("Unrecoverable exception", regs, SIGABRT);
}
-NOKPROBE_SYMBOL(unrecoverable_exception);
#if defined(CONFIG_BOOKE_WDT) || defined(CONFIG_40x)
/*
@@ -2214,7 +2206,6 @@ DEFINE_INTERRUPT_HANDLER(kernel_bad_stack)
regs->gpr[1], regs->nip);
die("Bad kernel stack pointer", regs, SIGABRT);
}
-NOKPROBE_SYMBOL(kernel_bad_stack);
void __init trap_init(void)
{
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index b26a7643fc6e..bb368257b55c 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -566,7 +566,6 @@ DEFINE_INTERRUPT_HANDLER_RET(do_page_fault)
{
return __do_page_fault(regs);
}
-NOKPROBE_SYMBOL(do_page_fault);
#ifdef CONFIG_PPC_BOOK3S_64
/* Same as do_page_fault but interrupt entry has already run in do_hash_fault */
--
2.23.0
^ permalink raw reply related
* Re: [PATCH] powerpc/traps: Declare unrecoverable_exception() as __noreturn
From: Christophe Leroy @ 2021-02-11 7:17 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, npiggin
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <f46a01750b1a00c9c43725899c9cf8eb6c6a0587.1613025208.git.christophe.leroy@csgroup.eu>
Le 11/02/2021 à 07:34, Christophe Leroy a écrit :
> unrecoverable_exception() is never expected to return, most callers
> have an infiniteloop in case it returns.
>
> Ensure it really never returns by terminating it with a BUG(), and
> declare it __no_return.
Not so easy, gcc complains about DEFINE_INTERRUPT_HANDLER() returning while the function is declared
__noreturn, __noreturn is needed there too.
>
> It always GCC to really simplify functions calling it. In the exemple below,
> it avoids the stack frame in the likely fast path and avoids code duplication
> for the exit.
>
> With this patch:
>
> 00000348 <interrupt_exit_kernel_prepare>:
> 348: 81 43 00 84 lwz r10,132(r3)
> 34c: 71 48 00 02 andi. r8,r10,2
> 350: 41 82 00 2c beq 37c <interrupt_exit_kernel_prepare+0x34>
> 354: 71 4a 40 00 andi. r10,r10,16384
> 358: 40 82 00 20 bne 378 <interrupt_exit_kernel_prepare+0x30>
> 35c: 80 62 00 70 lwz r3,112(r2)
> 360: 74 63 00 01 andis. r3,r3,1
> 364: 40 82 00 28 bne 38c <interrupt_exit_kernel_prepare+0x44>
> 368: 7d 40 00 a6 mfmsr r10
> 36c: 7c 11 13 a6 mtspr 81,r0
> 370: 7c 12 13 a6 mtspr 82,r0
> 374: 4e 80 00 20 blr
> 378: 48 00 00 00 b 378 <interrupt_exit_kernel_prepare+0x30>
> 37c: 94 21 ff f0 stwu r1,-16(r1)
> 380: 7c 08 02 a6 mflr r0
> 384: 90 01 00 14 stw r0,20(r1)
> 388: 48 00 00 01 bl 388 <interrupt_exit_kernel_prepare+0x40>
> 388: R_PPC_REL24 unrecoverable_exception
> 38c: 38 e2 00 70 addi r7,r2,112
> 390: 3d 00 00 01 lis r8,1
> 394: 7c c0 38 28 lwarx r6,0,r7
> 398: 7c c6 40 78 andc r6,r6,r8
> 39c: 7c c0 39 2d stwcx. r6,0,r7
> 3a0: 40 a2 ff f4 bne 394 <interrupt_exit_kernel_prepare+0x4c>
> 3a4: 38 60 00 01 li r3,1
> 3a8: 4b ff ff c0 b 368 <interrupt_exit_kernel_prepare+0x20>
>
> Without this patch:
>
> 00000348 <interrupt_exit_kernel_prepare>:
> 348: 94 21 ff f0 stwu r1,-16(r1)
> 34c: 93 e1 00 0c stw r31,12(r1)
> 350: 7c 7f 1b 78 mr r31,r3
> 354: 81 23 00 84 lwz r9,132(r3)
> 358: 71 2a 00 02 andi. r10,r9,2
> 35c: 41 82 00 34 beq 390 <interrupt_exit_kernel_prepare+0x48>
> 360: 71 29 40 00 andi. r9,r9,16384
> 364: 40 82 00 28 bne 38c <interrupt_exit_kernel_prepare+0x44>
> 368: 80 62 00 70 lwz r3,112(r2)
> 36c: 74 63 00 01 andis. r3,r3,1
> 370: 40 82 00 3c bne 3ac <interrupt_exit_kernel_prepare+0x64>
> 374: 7d 20 00 a6 mfmsr r9
> 378: 7c 11 13 a6 mtspr 81,r0
> 37c: 7c 12 13 a6 mtspr 82,r0
> 380: 83 e1 00 0c lwz r31,12(r1)
> 384: 38 21 00 10 addi r1,r1,16
> 388: 4e 80 00 20 blr
> 38c: 48 00 00 00 b 38c <interrupt_exit_kernel_prepare+0x44>
> 390: 7c 08 02 a6 mflr r0
> 394: 90 01 00 14 stw r0,20(r1)
> 398: 48 00 00 01 bl 398 <interrupt_exit_kernel_prepare+0x50>
> 398: R_PPC_REL24 unrecoverable_exception
> 39c: 80 01 00 14 lwz r0,20(r1)
> 3a0: 81 3f 00 84 lwz r9,132(r31)
> 3a4: 7c 08 03 a6 mtlr r0
> 3a8: 4b ff ff b8 b 360 <interrupt_exit_kernel_prepare+0x18>
> 3ac: 39 02 00 70 addi r8,r2,112
> 3b0: 3d 40 00 01 lis r10,1
> 3b4: 7c e0 40 28 lwarx r7,0,r8
> 3b8: 7c e7 50 78 andc r7,r7,r10
> 3bc: 7c e0 41 2d stwcx. r7,0,r8
> 3c0: 40 a2 ff f4 bne 3b4 <interrupt_exit_kernel_prepare+0x6c>
> 3c4: 38 60 00 01 li r3,1
> 3c8: 4b ff ff ac b 374 <interrupt_exit_kernel_prepare+0x2c>
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/include/asm/interrupt.h | 2 +-
> arch/powerpc/kernel/interrupt.c | 1 -
> arch/powerpc/kernel/traps.c | 2 ++
> 3 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
> index dcff30e3919b..fa8bfb91f8df 100644
> --- a/arch/powerpc/include/asm/interrupt.h
> +++ b/arch/powerpc/include/asm/interrupt.h
> @@ -411,7 +411,7 @@ DECLARE_INTERRUPT_HANDLER(altivec_assist_exception);
> DECLARE_INTERRUPT_HANDLER(CacheLockingException);
> DECLARE_INTERRUPT_HANDLER(SPEFloatingPointException);
> DECLARE_INTERRUPT_HANDLER(SPEFloatingPointRoundException);
> -DECLARE_INTERRUPT_HANDLER(unrecoverable_exception);
> +DECLARE_INTERRUPT_HANDLER(unrecoverable_exception) __noreturn;
> DECLARE_INTERRUPT_HANDLER(WatchdogException);
> DECLARE_INTERRUPT_HANDLER(kernel_bad_stack);
>
> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
> index eca3be36c18c..7e7106641ca9 100644
> --- a/arch/powerpc/kernel/interrupt.c
> +++ b/arch/powerpc/kernel/interrupt.c
> @@ -440,7 +440,6 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned
> return ret;
> }
>
> -void unrecoverable_exception(struct pt_regs *regs);
> void preempt_schedule_irq(void);
>
> notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsigned long msr)
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index 2afa05ad21c8..1ff776e9e8e3 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -2173,6 +2173,8 @@ DEFINE_INTERRUPT_HANDLER(unrecoverable_exception)
> pr_emerg("Unrecoverable exception %lx at %lx (msr=%lx)\n",
> regs->trap, regs->nip, regs->msr);
> die("Unrecoverable exception", regs, SIGABRT);
> + /* die() should not return */
> + BUG();
> }
> NOKPROBE_SYMBOL(unrecoverable_exception);
>
>
^ permalink raw reply
* [PATCH v2] powerpc/traps: Declare unrecoverable_exception() as __noreturn
From: Christophe Leroy @ 2021-02-11 7:40 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, npiggin
Cc: linuxppc-dev, linux-kernel
unrecoverable_exception() is never expected to return, most callers
have an infiniteloop in case it returns.
Ensure it really never returns by terminating it with a BUG(), and
declare it __no_return.
It always GCC to really simplify functions calling it. In the exemple below,
it avoids the stack frame in the likely fast path and avoids code duplication
for the exit.
With this patch:
00000348 <interrupt_exit_kernel_prepare>:
348: 81 43 00 84 lwz r10,132(r3)
34c: 71 48 00 02 andi. r8,r10,2
350: 41 82 00 2c beq 37c <interrupt_exit_kernel_prepare+0x34>
354: 71 4a 40 00 andi. r10,r10,16384
358: 40 82 00 20 bne 378 <interrupt_exit_kernel_prepare+0x30>
35c: 80 62 00 70 lwz r3,112(r2)
360: 74 63 00 01 andis. r3,r3,1
364: 40 82 00 28 bne 38c <interrupt_exit_kernel_prepare+0x44>
368: 7d 40 00 a6 mfmsr r10
36c: 7c 11 13 a6 mtspr 81,r0
370: 7c 12 13 a6 mtspr 82,r0
374: 4e 80 00 20 blr
378: 48 00 00 00 b 378 <interrupt_exit_kernel_prepare+0x30>
37c: 94 21 ff f0 stwu r1,-16(r1)
380: 7c 08 02 a6 mflr r0
384: 90 01 00 14 stw r0,20(r1)
388: 48 00 00 01 bl 388 <interrupt_exit_kernel_prepare+0x40>
388: R_PPC_REL24 unrecoverable_exception
38c: 38 e2 00 70 addi r7,r2,112
390: 3d 00 00 01 lis r8,1
394: 7c c0 38 28 lwarx r6,0,r7
398: 7c c6 40 78 andc r6,r6,r8
39c: 7c c0 39 2d stwcx. r6,0,r7
3a0: 40 a2 ff f4 bne 394 <interrupt_exit_kernel_prepare+0x4c>
3a4: 38 60 00 01 li r3,1
3a8: 4b ff ff c0 b 368 <interrupt_exit_kernel_prepare+0x20>
Without this patch:
00000348 <interrupt_exit_kernel_prepare>:
348: 94 21 ff f0 stwu r1,-16(r1)
34c: 93 e1 00 0c stw r31,12(r1)
350: 7c 7f 1b 78 mr r31,r3
354: 81 23 00 84 lwz r9,132(r3)
358: 71 2a 00 02 andi. r10,r9,2
35c: 41 82 00 34 beq 390 <interrupt_exit_kernel_prepare+0x48>
360: 71 29 40 00 andi. r9,r9,16384
364: 40 82 00 28 bne 38c <interrupt_exit_kernel_prepare+0x44>
368: 80 62 00 70 lwz r3,112(r2)
36c: 74 63 00 01 andis. r3,r3,1
370: 40 82 00 3c bne 3ac <interrupt_exit_kernel_prepare+0x64>
374: 7d 20 00 a6 mfmsr r9
378: 7c 11 13 a6 mtspr 81,r0
37c: 7c 12 13 a6 mtspr 82,r0
380: 83 e1 00 0c lwz r31,12(r1)
384: 38 21 00 10 addi r1,r1,16
388: 4e 80 00 20 blr
38c: 48 00 00 00 b 38c <interrupt_exit_kernel_prepare+0x44>
390: 7c 08 02 a6 mflr r0
394: 90 01 00 14 stw r0,20(r1)
398: 48 00 00 01 bl 398 <interrupt_exit_kernel_prepare+0x50>
398: R_PPC_REL24 unrecoverable_exception
39c: 80 01 00 14 lwz r0,20(r1)
3a0: 81 3f 00 84 lwz r9,132(r31)
3a4: 7c 08 03 a6 mtlr r0
3a8: 4b ff ff b8 b 360 <interrupt_exit_kernel_prepare+0x18>
3ac: 39 02 00 70 addi r8,r2,112
3b0: 3d 40 00 01 lis r10,1
3b4: 7c e0 40 28 lwarx r7,0,r8
3b8: 7c e7 50 78 andc r7,r7,r10
3bc: 7c e0 41 2d stwcx. r7,0,r8
3c0: 40 a2 ff f4 bne 3b4 <interrupt_exit_kernel_prepare+0x6c>
3c4: 38 60 00 01 li r3,1
3c8: 4b ff ff ac b 374 <interrupt_exit_kernel_prepare+0x2c>
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v2: Also add __noreturn to the definition
---
arch/powerpc/include/asm/interrupt.h | 2 +-
arch/powerpc/kernel/interrupt.c | 1 -
arch/powerpc/kernel/traps.c | 4 +++-
3 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
index dcff30e3919b..e6950352347d 100644
--- a/arch/powerpc/include/asm/interrupt.h
+++ b/arch/powerpc/include/asm/interrupt.h
@@ -411,7 +411,7 @@ DECLARE_INTERRUPT_HANDLER(altivec_assist_exception);
DECLARE_INTERRUPT_HANDLER(CacheLockingException);
DECLARE_INTERRUPT_HANDLER(SPEFloatingPointException);
DECLARE_INTERRUPT_HANDLER(SPEFloatingPointRoundException);
-DECLARE_INTERRUPT_HANDLER(unrecoverable_exception);
+__noreturn DECLARE_INTERRUPT_HANDLER(unrecoverable_exception);
DECLARE_INTERRUPT_HANDLER(WatchdogException);
DECLARE_INTERRUPT_HANDLER(kernel_bad_stack);
diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
index eca3be36c18c..7e7106641ca9 100644
--- a/arch/powerpc/kernel/interrupt.c
+++ b/arch/powerpc/kernel/interrupt.c
@@ -440,7 +440,6 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned
return ret;
}
-void unrecoverable_exception(struct pt_regs *regs);
void preempt_schedule_irq(void);
notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsigned long msr)
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 2afa05ad21c8..22486d27fa82 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -2168,11 +2168,13 @@ DEFINE_INTERRUPT_HANDLER(SPEFloatingPointRoundException)
* in the MSR is 0. This indicates that SRR0/1 are live, and that
* we therefore lost state by taking this exception.
*/
-DEFINE_INTERRUPT_HANDLER(unrecoverable_exception)
+__noreturn DEFINE_INTERRUPT_HANDLER(unrecoverable_exception)
{
pr_emerg("Unrecoverable exception %lx at %lx (msr=%lx)\n",
regs->trap, regs->nip, regs->msr);
die("Unrecoverable exception", regs, SIGABRT);
+ /* die() should not return */
+ BUG();
}
NOKPROBE_SYMBOL(unrecoverable_exception);
--
2.25.0
^ permalink raw reply related
* [PATCH] powerpc/bug: Remove specific powerpc BUG_ON()
From: Christophe Leroy @ 2021-02-11 7:41 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, npiggin
Cc: linuxppc-dev, linux-kernel
powerpc BUG_ON() is based on using twnei or tdnei instruction,
which obliges gcc to format the condition into a 0 or 1 value
in a register.
By using a generic implementation, gcc will generate a branch
to the unconditional trap generated by BUG().
As modern powerpc implement branch folding, that's even more efficient.
See below the difference at the entry of system_call_exception.
With the patch:
00000000 <system_call_exception>:
0: 81 6a 00 84 lwz r11,132(r10)
4: 90 6a 00 88 stw r3,136(r10)
8: 71 60 00 02 andi. r0,r11,2
c: 41 82 00 70 beq 7c <system_call_exception+0x7c>
10: 71 60 40 00 andi. r0,r11,16384
14: 41 82 00 6c beq 80 <system_call_exception+0x80>
18: 71 6b 80 00 andi. r11,r11,32768
1c: 41 82 00 68 beq 84 <system_call_exception+0x84>
20: 94 21 ff e0 stwu r1,-32(r1)
24: 93 e1 00 1c stw r31,28(r1)
28: 7d 8c 42 e6 mftb r12
...
7c: 0f e0 00 00 twui r0,0
80: 0f e0 00 00 twui r0,0
84: 0f e0 00 00 twui r0,0
Without the patch:
00000000 <system_call_exception>:
0: 94 21 ff e0 stwu r1,-32(r1)
4: 93 e1 00 1c stw r31,28(r1)
8: 90 6a 00 88 stw r3,136(r10)
c: 81 6a 00 84 lwz r11,132(r10)
10: 69 60 00 02 xori r0,r11,2
14: 54 00 ff fe rlwinm r0,r0,31,31,31
18: 0f 00 00 00 twnei r0,0
1c: 69 60 40 00 xori r0,r11,16384
20: 54 00 97 fe rlwinm r0,r0,18,31,31
24: 0f 00 00 00 twnei r0,0
28: 69 6b 80 00 xori r11,r11,32768
2c: 55 6b 8f fe rlwinm r11,r11,17,31,31
30: 0f 0b 00 00 twnei r11,0
34: 7d 8c 42 e6 mftb r12
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/include/asm/bug.h | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
index d1635ffbb179..21103d3e1f29 100644
--- a/arch/powerpc/include/asm/bug.h
+++ b/arch/powerpc/include/asm/bug.h
@@ -69,15 +69,6 @@
unreachable(); \
} while (0)
-#define BUG_ON(x) do { \
- if (__builtin_constant_p(x)) { \
- if (x) \
- BUG(); \
- } else { \
- BUG_ENTRY(PPC_TLNEI " %4, 0", 0, "r" ((__force long)(x))); \
- } \
-} while (0)
-
#define __WARN_FLAGS(flags) BUG_ENTRY("twi 31, 0, 0", BUGFLAG_WARNING | (flags))
#define WARN_ON(x) ({ \
@@ -94,7 +85,6 @@
})
#define HAVE_ARCH_BUG
-#define HAVE_ARCH_BUG_ON
#define HAVE_ARCH_WARN_ON
#endif /* __ASSEMBLY __ */
#else
--
2.25.0
^ permalink raw reply related
* Re: [PATCH] powerpc/traps: Declare unrecoverable_exception() as __noreturn
From: Gabriel Paubert @ 2021-02-11 7:47 UTC (permalink / raw)
To: Christophe Leroy; +Cc: linux-kernel, npiggin, Paul Mackerras, linuxppc-dev
In-Reply-To: <f46a01750b1a00c9c43725899c9cf8eb6c6a0587.1613025208.git.christophe.leroy@csgroup.eu>
On Thu, Feb 11, 2021 at 06:34:43AM +0000, Christophe Leroy wrote:
> unrecoverable_exception() is never expected to return, most callers
> have an infiniteloop in case it returns.
>
> Ensure it really never returns by terminating it with a BUG(), and
> declare it __no_return.
>
> It always GCC to really simplify functions calling it. In the exemple below,
s/always/allows ?
(Otherwise I can't parse it.)
> it avoids the stack frame in the likely fast path and avoids code duplication
> for the exit.
Indeed, nice code generation improvement.
>
> With this patch:
>
> 00000348 <interrupt_exit_kernel_prepare>:
> 348: 81 43 00 84 lwz r10,132(r3)
> 34c: 71 48 00 02 andi. r8,r10,2
> 350: 41 82 00 2c beq 37c <interrupt_exit_kernel_prepare+0x34>
> 354: 71 4a 40 00 andi. r10,r10,16384
> 358: 40 82 00 20 bne 378 <interrupt_exit_kernel_prepare+0x30>
> 35c: 80 62 00 70 lwz r3,112(r2)
> 360: 74 63 00 01 andis. r3,r3,1
> 364: 40 82 00 28 bne 38c <interrupt_exit_kernel_prepare+0x44>
> 368: 7d 40 00 a6 mfmsr r10
> 36c: 7c 11 13 a6 mtspr 81,r0
> 370: 7c 12 13 a6 mtspr 82,r0
> 374: 4e 80 00 20 blr
> 378: 48 00 00 00 b 378 <interrupt_exit_kernel_prepare+0x30>
Infinite loop (seems to be on test of MSR_PR)?
Gabriel
> 37c: 94 21 ff f0 stwu r1,-16(r1)
> 380: 7c 08 02 a6 mflr r0
> 384: 90 01 00 14 stw r0,20(r1)
> 388: 48 00 00 01 bl 388 <interrupt_exit_kernel_prepare+0x40>
> 388: R_PPC_REL24 unrecoverable_exception
> 38c: 38 e2 00 70 addi r7,r2,112
> 390: 3d 00 00 01 lis r8,1
> 394: 7c c0 38 28 lwarx r6,0,r7
> 398: 7c c6 40 78 andc r6,r6,r8
> 39c: 7c c0 39 2d stwcx. r6,0,r7
> 3a0: 40 a2 ff f4 bne 394 <interrupt_exit_kernel_prepare+0x4c>
> 3a4: 38 60 00 01 li r3,1
> 3a8: 4b ff ff c0 b 368 <interrupt_exit_kernel_prepare+0x20>
>
> Without this patch:
>
> 00000348 <interrupt_exit_kernel_prepare>:
> 348: 94 21 ff f0 stwu r1,-16(r1)
> 34c: 93 e1 00 0c stw r31,12(r1)
> 350: 7c 7f 1b 78 mr r31,r3
> 354: 81 23 00 84 lwz r9,132(r3)
> 358: 71 2a 00 02 andi. r10,r9,2
> 35c: 41 82 00 34 beq 390 <interrupt_exit_kernel_prepare+0x48>
> 360: 71 29 40 00 andi. r9,r9,16384
> 364: 40 82 00 28 bne 38c <interrupt_exit_kernel_prepare+0x44>
> 368: 80 62 00 70 lwz r3,112(r2)
> 36c: 74 63 00 01 andis. r3,r3,1
> 370: 40 82 00 3c bne 3ac <interrupt_exit_kernel_prepare+0x64>
> 374: 7d 20 00 a6 mfmsr r9
> 378: 7c 11 13 a6 mtspr 81,r0
> 37c: 7c 12 13 a6 mtspr 82,r0
> 380: 83 e1 00 0c lwz r31,12(r1)
> 384: 38 21 00 10 addi r1,r1,16
> 388: 4e 80 00 20 blr
> 38c: 48 00 00 00 b 38c <interrupt_exit_kernel_prepare+0x44>
> 390: 7c 08 02 a6 mflr r0
> 394: 90 01 00 14 stw r0,20(r1)
> 398: 48 00 00 01 bl 398 <interrupt_exit_kernel_prepare+0x50>
> 398: R_PPC_REL24 unrecoverable_exception
> 39c: 80 01 00 14 lwz r0,20(r1)
> 3a0: 81 3f 00 84 lwz r9,132(r31)
> 3a4: 7c 08 03 a6 mtlr r0
> 3a8: 4b ff ff b8 b 360 <interrupt_exit_kernel_prepare+0x18>
> 3ac: 39 02 00 70 addi r8,r2,112
> 3b0: 3d 40 00 01 lis r10,1
> 3b4: 7c e0 40 28 lwarx r7,0,r8
> 3b8: 7c e7 50 78 andc r7,r7,r10
> 3bc: 7c e0 41 2d stwcx. r7,0,r8
> 3c0: 40 a2 ff f4 bne 3b4 <interrupt_exit_kernel_prepare+0x6c>
> 3c4: 38 60 00 01 li r3,1
> 3c8: 4b ff ff ac b 374 <interrupt_exit_kernel_prepare+0x2c>
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/include/asm/interrupt.h | 2 +-
> arch/powerpc/kernel/interrupt.c | 1 -
> arch/powerpc/kernel/traps.c | 2 ++
> 3 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
> index dcff30e3919b..fa8bfb91f8df 100644
> --- a/arch/powerpc/include/asm/interrupt.h
> +++ b/arch/powerpc/include/asm/interrupt.h
> @@ -411,7 +411,7 @@ DECLARE_INTERRUPT_HANDLER(altivec_assist_exception);
> DECLARE_INTERRUPT_HANDLER(CacheLockingException);
> DECLARE_INTERRUPT_HANDLER(SPEFloatingPointException);
> DECLARE_INTERRUPT_HANDLER(SPEFloatingPointRoundException);
> -DECLARE_INTERRUPT_HANDLER(unrecoverable_exception);
> +DECLARE_INTERRUPT_HANDLER(unrecoverable_exception) __noreturn;
> DECLARE_INTERRUPT_HANDLER(WatchdogException);
> DECLARE_INTERRUPT_HANDLER(kernel_bad_stack);
>
> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
> index eca3be36c18c..7e7106641ca9 100644
> --- a/arch/powerpc/kernel/interrupt.c
> +++ b/arch/powerpc/kernel/interrupt.c
> @@ -440,7 +440,6 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned
> return ret;
> }
>
> -void unrecoverable_exception(struct pt_regs *regs);
> void preempt_schedule_irq(void);
>
> notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsigned long msr)
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index 2afa05ad21c8..1ff776e9e8e3 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -2173,6 +2173,8 @@ DEFINE_INTERRUPT_HANDLER(unrecoverable_exception)
> pr_emerg("Unrecoverable exception %lx at %lx (msr=%lx)\n",
> regs->trap, regs->nip, regs->msr);
> die("Unrecoverable exception", regs, SIGABRT);
> + /* die() should not return */
> + BUG();
> }
> NOKPROBE_SYMBOL(unrecoverable_exception);
>
> --
> 2.25.0
>
^ permalink raw reply
* Re: [PATCH] powerpc/traps: Declare unrecoverable_exception() as __noreturn
From: Christophe Leroy @ 2021-02-11 9:02 UTC (permalink / raw)
To: Gabriel Paubert; +Cc: linux-kernel, npiggin, Paul Mackerras, linuxppc-dev
In-Reply-To: <20210211074723.GA16987@lt-gp.iram.es>
Le 11/02/2021 à 08:47, Gabriel Paubert a écrit :
> On Thu, Feb 11, 2021 at 06:34:43AM +0000, Christophe Leroy wrote:
>> unrecoverable_exception() is never expected to return, most callers
>> have an infiniteloop in case it returns.
>>
>> Ensure it really never returns by terminating it with a BUG(), and
>> declare it __no_return.
>>
>> It always GCC to really simplify functions calling it. In the exemple below,
>
> s/always/allows ?
Yes
>
> (Otherwise I can't parse it.)
>
>> it avoids the stack frame in the likely fast path and avoids code duplication
>> for the exit.
>
> Indeed, nice code generation improvement.
>
>>
>> With this patch:
>>
>> 00000348 <interrupt_exit_kernel_prepare>:
>> 348: 81 43 00 84 lwz r10,132(r3)
>> 34c: 71 48 00 02 andi. r8,r10,2
>> 350: 41 82 00 2c beq 37c <interrupt_exit_kernel_prepare+0x34>
>> 354: 71 4a 40 00 andi. r10,r10,16384
>> 358: 40 82 00 20 bne 378 <interrupt_exit_kernel_prepare+0x30>
>> 35c: 80 62 00 70 lwz r3,112(r2)
>> 360: 74 63 00 01 andis. r3,r3,1
>> 364: 40 82 00 28 bne 38c <interrupt_exit_kernel_prepare+0x44>
>> 368: 7d 40 00 a6 mfmsr r10
>> 36c: 7c 11 13 a6 mtspr 81,r0
>> 370: 7c 12 13 a6 mtspr 82,r0
>> 374: 4e 80 00 20 blr
>> 378: 48 00 00 00 b 378 <interrupt_exit_kernel_prepare+0x30>
>
> Infinite loop (seems to be on test of MSR_PR)?
Yes, that's what you get when CONFIG_BUG is not selected.
/include/asm-generic/bug.h:
#ifndef HAVE_ARCH_BUG
#define BUG() do {} while (1)
#endif
>
> Gabriel
>
>> 37c: 94 21 ff f0 stwu r1,-16(r1)
>> 380: 7c 08 02 a6 mflr r0
>> 384: 90 01 00 14 stw r0,20(r1)
>> 388: 48 00 00 01 bl 388 <interrupt_exit_kernel_prepare+0x40>
>> 388: R_PPC_REL24 unrecoverable_exception
>> 38c: 38 e2 00 70 addi r7,r2,112
>> 390: 3d 00 00 01 lis r8,1
>> 394: 7c c0 38 28 lwarx r6,0,r7
>> 398: 7c c6 40 78 andc r6,r6,r8
>> 39c: 7c c0 39 2d stwcx. r6,0,r7
>> 3a0: 40 a2 ff f4 bne 394 <interrupt_exit_kernel_prepare+0x4c>
>> 3a4: 38 60 00 01 li r3,1
>> 3a8: 4b ff ff c0 b 368 <interrupt_exit_kernel_prepare+0x20>
>>
>> Without this patch:
>>
>> 00000348 <interrupt_exit_kernel_prepare>:
>> 348: 94 21 ff f0 stwu r1,-16(r1)
>> 34c: 93 e1 00 0c stw r31,12(r1)
>> 350: 7c 7f 1b 78 mr r31,r3
>> 354: 81 23 00 84 lwz r9,132(r3)
>> 358: 71 2a 00 02 andi. r10,r9,2
>> 35c: 41 82 00 34 beq 390 <interrupt_exit_kernel_prepare+0x48>
>> 360: 71 29 40 00 andi. r9,r9,16384
>> 364: 40 82 00 28 bne 38c <interrupt_exit_kernel_prepare+0x44>
>> 368: 80 62 00 70 lwz r3,112(r2)
>> 36c: 74 63 00 01 andis. r3,r3,1
>> 370: 40 82 00 3c bne 3ac <interrupt_exit_kernel_prepare+0x64>
>> 374: 7d 20 00 a6 mfmsr r9
>> 378: 7c 11 13 a6 mtspr 81,r0
>> 37c: 7c 12 13 a6 mtspr 82,r0
>> 380: 83 e1 00 0c lwz r31,12(r1)
>> 384: 38 21 00 10 addi r1,r1,16
>> 388: 4e 80 00 20 blr
>> 38c: 48 00 00 00 b 38c <interrupt_exit_kernel_prepare+0x44>
>> 390: 7c 08 02 a6 mflr r0
>> 394: 90 01 00 14 stw r0,20(r1)
>> 398: 48 00 00 01 bl 398 <interrupt_exit_kernel_prepare+0x50>
>> 398: R_PPC_REL24 unrecoverable_exception
>> 39c: 80 01 00 14 lwz r0,20(r1)
>> 3a0: 81 3f 00 84 lwz r9,132(r31)
>> 3a4: 7c 08 03 a6 mtlr r0
>> 3a8: 4b ff ff b8 b 360 <interrupt_exit_kernel_prepare+0x18>
>> 3ac: 39 02 00 70 addi r8,r2,112
>> 3b0: 3d 40 00 01 lis r10,1
>> 3b4: 7c e0 40 28 lwarx r7,0,r8
>> 3b8: 7c e7 50 78 andc r7,r7,r10
>> 3bc: 7c e0 41 2d stwcx. r7,0,r8
>> 3c0: 40 a2 ff f4 bne 3b4 <interrupt_exit_kernel_prepare+0x6c>
>> 3c4: 38 60 00 01 li r3,1
>> 3c8: 4b ff ff ac b 374 <interrupt_exit_kernel_prepare+0x2c>
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> arch/powerpc/include/asm/interrupt.h | 2 +-
>> arch/powerpc/kernel/interrupt.c | 1 -
>> arch/powerpc/kernel/traps.c | 2 ++
>> 3 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
>> index dcff30e3919b..fa8bfb91f8df 100644
>> --- a/arch/powerpc/include/asm/interrupt.h
>> +++ b/arch/powerpc/include/asm/interrupt.h
>> @@ -411,7 +411,7 @@ DECLARE_INTERRUPT_HANDLER(altivec_assist_exception);
>> DECLARE_INTERRUPT_HANDLER(CacheLockingException);
>> DECLARE_INTERRUPT_HANDLER(SPEFloatingPointException);
>> DECLARE_INTERRUPT_HANDLER(SPEFloatingPointRoundException);
>> -DECLARE_INTERRUPT_HANDLER(unrecoverable_exception);
>> +DECLARE_INTERRUPT_HANDLER(unrecoverable_exception) __noreturn;
>> DECLARE_INTERRUPT_HANDLER(WatchdogException);
>> DECLARE_INTERRUPT_HANDLER(kernel_bad_stack);
>>
>> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
>> index eca3be36c18c..7e7106641ca9 100644
>> --- a/arch/powerpc/kernel/interrupt.c
>> +++ b/arch/powerpc/kernel/interrupt.c
>> @@ -440,7 +440,6 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned
>> return ret;
>> }
>>
>> -void unrecoverable_exception(struct pt_regs *regs);
>> void preempt_schedule_irq(void);
>>
>> notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsigned long msr)
>> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
>> index 2afa05ad21c8..1ff776e9e8e3 100644
>> --- a/arch/powerpc/kernel/traps.c
>> +++ b/arch/powerpc/kernel/traps.c
>> @@ -2173,6 +2173,8 @@ DEFINE_INTERRUPT_HANDLER(unrecoverable_exception)
>> pr_emerg("Unrecoverable exception %lx at %lx (msr=%lx)\n",
>> regs->trap, regs->nip, regs->msr);
>> die("Unrecoverable exception", regs, SIGABRT);
>> + /* die() should not return */
>> + BUG();
>> }
>> NOKPROBE_SYMBOL(unrecoverable_exception);
>>
>> --
>> 2.25.0
>>
>
>
^ permalink raw reply
* Re: [PATCH] powerpc/bug: Remove specific powerpc BUG_ON()
From: Nicholas Piggin @ 2021-02-11 10:04 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <694c7195c81d1bcc781b3c14f452886683d6c524.1613029237.git.christophe.leroy@csgroup.eu>
Excerpts from Christophe Leroy's message of February 11, 2021 5:41 pm:
> powerpc BUG_ON() is based on using twnei or tdnei instruction,
> which obliges gcc to format the condition into a 0 or 1 value
> in a register.
>
> By using a generic implementation, gcc will generate a branch
> to the unconditional trap generated by BUG().
We don't want to do this on 64s because that will lose the useful CFAR
contents.
Unfortunately the code generation is not great and the registers that
give some useful information about the condition are often mangled :(
It would be nice if we could have a __builtin_trap_if that gcc would use
conditional traps with, (and which never assumes following code is
unreachable even for constant true, so we can use it with WARN and put
explicit unreachable for BUG).
>
> As modern powerpc implement branch folding, that's even more efficient.
I think POWER will speculate conditional traps as non faulting always
so it should be just as good if not better than the branch.
Thanks,
Nick
^ permalink raw reply
* [PATCH 1/3] powerpc/perf: Adds support for programming of Thresholding in P10
From: Michael Ellerman @ 2021-02-11 11:27 UTC (permalink / raw)
To: linuxppc-dev
From: Kajol Jain <kjain@linux.ibm.com>
Thresholding, a performance monitoring unit feature, can be
used to identify marked instructions which take more than
expected cycles between start event and end event.
Threshold compare (thresh_cmp) bits are programmed in MMCRA
register. In Power9, thresh_cmp bits were part of the
event code. But in case of P10, thresh_cmp are not part of
event code due to inclusion of MMCR3 bits.
Patch here adds an option to use attr.config1 variable
to be used to pass thresh_cmp value to be programmed in
MMCRA register. A new ppmu flag called PPMU_HAS_ATTR_CONFIG1
has been added and this flag is used to notify the use of
attr.config1 variable.
Patch has extended the parameter list of 'compute_mmcr',
to include power_pmu's 'flags' element and parameter list of
get_constraint to include attr.config1 value. It also extend
parameter list of power_check_constraints inorder to pass
perf_event list.
As stated by commit ef0e3b650f8d ("powerpc/perf: Fix Threshold
Event Counter Multiplier width for P10"), constraint bits for
thresh_cmp is also needed to be increased to 11 bits, which is
handled as part of this patch. We added bit number 53 as part
of constraint bits of thresh_cmp for power10 to make it an
11 bit field.
Updated layout for p10:
/*
* Layout of constraint bits:
*
* 60 56 52 48 44 40 36 32
* | - - - - | - - - - | - - - - | - - - - | - - - - | - - - - | - - - - | - - - - |
* [ fab_match ] [ thresh_cmp ] [ thresh_ctl ] [ ]
* | |
* [ thresh_cmp bits for p10] thresh_sel -*
*
* 28 24 20 16 12 8 4 0
* | - - - - | - - - - | - - - - | - - - - | - - - - | - - - - | - - - - | - - - - |
* [ ] | [ ] | [ sample ] [ ] [6] [5] [4] [3] [2] [1]
* | | | | |
* BHRB IFM -* | | |*radix_scope | Count of events for each PMC.
* EBB -* | | p1, p2, p3, p4, p5, p6.
* L1 I/D qualifier -* |
* nc - number of counters -*
*
* The PMC fields P1..P6, and NC, are adder fields. As we accumulate constraints
* we want the low bit of each field to be added to any existing value.
*
* Everything else is a value field.
*/
Result:
command#: cat /sys/devices/cpu/format/thresh_cmp
config1:0-17
ex. usage:
command#: perf record -I --weight -d -e
cpu/event=0x67340101EC,thresh_cmp=500/ ./ebizzy -S 2 -t 1 -s 4096
1826636 records/s
real 2.00 s
user 2.00 s
sys 0.00 s
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.038 MB perf.data (61 samples) ]
Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Link: https://lore.kernel.org/r/20210209095234.837356-1-kjain@linux.ibm.com
---
arch/powerpc/include/asm/perf_event_server.h | 5 +-
arch/powerpc/perf/core-book3s.c | 15 +++--
arch/powerpc/perf/isa207-common.c | 67 +++++++++++++++++---
arch/powerpc/perf/isa207-common.h | 15 +++--
arch/powerpc/perf/mpc7450-pmu.c | 5 +-
arch/powerpc/perf/power10-pmu.c | 4 +-
arch/powerpc/perf/power5+-pmu.c | 5 +-
arch/powerpc/perf/power5-pmu.c | 5 +-
arch/powerpc/perf/power6-pmu.c | 5 +-
arch/powerpc/perf/power7-pmu.c | 5 +-
arch/powerpc/perf/ppc970-pmu.c | 5 +-
11 files changed, 102 insertions(+), 34 deletions(-)
diff --git a/arch/powerpc/include/asm/perf_event_server.h b/arch/powerpc/include/asm/perf_event_server.h
index 3b7baba01c92..00e7e671bb4b 100644
--- a/arch/powerpc/include/asm/perf_event_server.h
+++ b/arch/powerpc/include/asm/perf_event_server.h
@@ -36,9 +36,9 @@ struct power_pmu {
unsigned long test_adder;
int (*compute_mmcr)(u64 events[], int n_ev,
unsigned int hwc[], struct mmcr_regs *mmcr,
- struct perf_event *pevents[]);
+ struct perf_event *pevents[], u32 flags);
int (*get_constraint)(u64 event_id, unsigned long *mskp,
- unsigned long *valp);
+ unsigned long *valp, u64 event_config1);
int (*get_alternatives)(u64 event_id, unsigned int flags,
u64 alt[]);
void (*get_mem_data_src)(union perf_mem_data_src *dsrc,
@@ -83,6 +83,7 @@ struct power_pmu {
#define PPMU_NO_SIAR 0x00000100 /* Do not use SIAR */
#define PPMU_ARCH_31 0x00000200 /* Has MMCR3, SIER2 and SIER3 */
#define PPMU_P10_DD1 0x00000400 /* Is power10 DD1 processor version */
+#define PPMU_HAS_ATTR_CONFIG1 0x00000800 /* Using config1 attribute */
/*
* Values for flags to get_alternatives()
diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index 0e31aaa0a0d2..4b4319d84c54 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -916,7 +916,7 @@ void perf_event_print_debug(void)
*/
static int power_check_constraints(struct cpu_hw_events *cpuhw,
u64 event_id[], unsigned int cflags[],
- int n_ev)
+ int n_ev, struct perf_event **event)
{
unsigned long mask, value, nv;
unsigned long smasks[MAX_HWEVENTS], svalues[MAX_HWEVENTS];
@@ -939,7 +939,7 @@ static int power_check_constraints(struct cpu_hw_events *cpuhw,
event_id[i] = cpuhw->alternatives[i][0];
}
if (ppmu->get_constraint(event_id[i], &cpuhw->amasks[i][0],
- &cpuhw->avalues[i][0]))
+ &cpuhw->avalues[i][0], event[i]->attr.config1))
return -1;
}
value = mask = 0;
@@ -974,7 +974,8 @@ static int power_check_constraints(struct cpu_hw_events *cpuhw,
for (j = 1; j < n_alt[i]; ++j)
ppmu->get_constraint(cpuhw->alternatives[i][j],
&cpuhw->amasks[i][j],
- &cpuhw->avalues[i][j]);
+ &cpuhw->avalues[i][j],
+ event[i]->attr.config1);
}
/* enumerate all possibilities and see if any will work */
@@ -1392,7 +1393,7 @@ static void power_pmu_enable(struct pmu *pmu)
memset(&cpuhw->mmcr, 0, sizeof(cpuhw->mmcr));
if (ppmu->compute_mmcr(cpuhw->events, cpuhw->n_events, hwc_index,
- &cpuhw->mmcr, cpuhw->event)) {
+ &cpuhw->mmcr, cpuhw->event, ppmu->flags)) {
/* shouldn't ever get here */
printk(KERN_ERR "oops compute_mmcr failed\n");
goto out;
@@ -1580,7 +1581,7 @@ static int power_pmu_add(struct perf_event *event, int ef_flags)
if (check_excludes(cpuhw->event, cpuhw->flags, n0, 1))
goto out;
- if (power_check_constraints(cpuhw, cpuhw->events, cpuhw->flags, n0 + 1))
+ if (power_check_constraints(cpuhw, cpuhw->events, cpuhw->flags, n0 + 1, cpuhw->event))
goto out;
event->hw.config = cpuhw->events[n0];
@@ -1790,7 +1791,7 @@ static int power_pmu_commit_txn(struct pmu *pmu)
n = cpuhw->n_events;
if (check_excludes(cpuhw->event, cpuhw->flags, 0, n))
return -EAGAIN;
- i = power_check_constraints(cpuhw, cpuhw->events, cpuhw->flags, n);
+ i = power_check_constraints(cpuhw, cpuhw->events, cpuhw->flags, n, cpuhw->event);
if (i < 0)
return -EAGAIN;
@@ -2028,7 +2029,7 @@ static int power_pmu_event_init(struct perf_event *event)
local_irq_save(irq_flags);
cpuhw = this_cpu_ptr(&cpu_hw_events);
- err = power_check_constraints(cpuhw, events, cflags, n + 1);
+ err = power_check_constraints(cpuhw, events, cflags, n + 1, ctrs);
if (has_branch_stack(event)) {
u64 bhrb_filter = -1;
diff --git a/arch/powerpc/perf/isa207-common.c b/arch/powerpc/perf/isa207-common.c
index 6ab5b272090a..e4f577da33d8 100644
--- a/arch/powerpc/perf/isa207-common.c
+++ b/arch/powerpc/perf/isa207-common.c
@@ -108,12 +108,57 @@ static void mmcra_sdar_mode(u64 event, unsigned long *mmcra)
*mmcra |= MMCRA_SDAR_MODE_TLB;
}
+static u64 p10_thresh_cmp_val(u64 value)
+{
+ int exp = 0;
+ u64 result = value;
+
+ if (!value)
+ return value;
+
+ /*
+ * Incase of P10, thresh_cmp value is not part of raw event code
+ * and provided via attr.config1 parameter. To program threshold in MMCRA,
+ * take a 18 bit number N and shift right 2 places and increment
+ * the exponent E by 1 until the upper 10 bits of N are zero.
+ * Write E to the threshold exponent and write the lower 8 bits of N
+ * to the threshold mantissa.
+ * The max threshold that can be written is 261120.
+ */
+ if (cpu_has_feature(CPU_FTR_ARCH_31)) {
+ if (value > 261120)
+ value = 261120;
+ while ((64 - __builtin_clzl(value)) > 8) {
+ exp++;
+ value >>= 2;
+ }
+
+ /*
+ * Note that it is invalid to write a mantissa with the
+ * upper 2 bits of mantissa being zero, unless the
+ * exponent is also zero.
+ */
+ if (!(value & 0xC0) && exp)
+ result = 0;
+ else
+ result = (exp << 8) | value;
+ }
+ return result;
+}
+
static u64 thresh_cmp_val(u64 value)
{
+ if (cpu_has_feature(CPU_FTR_ARCH_31))
+ value = p10_thresh_cmp_val(value);
+
+ /*
+ * Since location of threshold compare bits in MMCRA
+ * is different for p8, using different shift value.
+ */
if (cpu_has_feature(CPU_FTR_ARCH_300))
return value << p9_MMCRA_THR_CMP_SHIFT;
-
- return value << MMCRA_THR_CMP_SHIFT;
+ else
+ return value << MMCRA_THR_CMP_SHIFT;
}
static unsigned long combine_from_event(u64 event)
@@ -141,13 +186,13 @@ static bool is_thresh_cmp_valid(u64 event)
{
unsigned int cmp, exp;
+ if (cpu_has_feature(CPU_FTR_ARCH_31))
+ return p10_thresh_cmp_val(event) != 0;
+
/*
* Check the mantissa upper two bits are not zero, unless the
* exponent is also zero. See the THRESH_CMP_MANTISSA doc.
- * Power10: thresh_cmp is replaced by l2_l3 event select.
*/
- if (cpu_has_feature(CPU_FTR_ARCH_31))
- return false;
cmp = (event >> EVENT_THR_CMP_SHIFT) & EVENT_THR_CMP_MASK;
exp = cmp >> 7;
@@ -256,7 +301,7 @@ void isa207_get_mem_weight(u64 *weight)
*weight = mantissa << (2 * exp);
}
-int isa207_get_constraint(u64 event, unsigned long *maskp, unsigned long *valp)
+int isa207_get_constraint(u64 event, unsigned long *maskp, unsigned long *valp, u64 event_config1)
{
unsigned int unit, pmc, cache, ebb;
unsigned long mask, value;
@@ -355,9 +400,11 @@ int isa207_get_constraint(u64 event, unsigned long *maskp, unsigned long *valp)
}
if (cpu_has_feature(CPU_FTR_ARCH_31)) {
- if (event_is_threshold(event)) {
+ if (event_is_threshold(event) && is_thresh_cmp_valid(event_config1)) {
mask |= CNST_THRESH_CTL_SEL_MASK;
value |= CNST_THRESH_CTL_SEL_VAL(event >> EVENT_THRESH_SHIFT);
+ mask |= p10_CNST_THRESH_CMP_MASK;
+ value |= p10_CNST_THRESH_CMP_VAL(p10_thresh_cmp_val(event_config1));
}
} else if (cpu_has_feature(CPU_FTR_ARCH_300)) {
if (event_is_threshold(event) && is_thresh_cmp_valid(event)) {
@@ -411,7 +458,7 @@ int isa207_get_constraint(u64 event, unsigned long *maskp, unsigned long *valp)
int isa207_compute_mmcr(u64 event[], int n_ev,
unsigned int hwc[], struct mmcr_regs *mmcr,
- struct perf_event *pevents[])
+ struct perf_event *pevents[], u32 flags)
{
unsigned long mmcra, mmcr1, mmcr2, unit, combine, psel, cache, val;
unsigned long mmcr3;
@@ -504,6 +551,10 @@ int isa207_compute_mmcr(u64 event[], int n_ev,
val = (event[i] >> EVENT_THR_CMP_SHIFT) &
EVENT_THR_CMP_MASK;
mmcra |= thresh_cmp_val(val);
+ } else if (flags & PPMU_HAS_ATTR_CONFIG1) {
+ val = (pevents[i]->attr.config1 >> p10_EVENT_THR_CMP_SHIFT) &
+ p10_EVENT_THR_CMP_MASK;
+ mmcra |= thresh_cmp_val(val);
}
}
diff --git a/arch/powerpc/perf/isa207-common.h b/arch/powerpc/perf/isa207-common.h
index 454b32c31440..1af0e8c97ac7 100644
--- a/arch/powerpc/perf/isa207-common.h
+++ b/arch/powerpc/perf/isa207-common.h
@@ -105,6 +105,10 @@
#define p10_EVENT_RADIX_SCOPE_QUAL_MASK 0x1
#define p10_MMCR1_RADIX_SCOPE_QUAL_SHIFT 45
+/* Event Threshold Compare bit constant for power10 in config1 attribute */
+#define p10_EVENT_THR_CMP_SHIFT 0
+#define p10_EVENT_THR_CMP_MASK 0x3FFFFull
+
#define p10_EVENT_VALID_MASK \
((p10_SDAR_MODE_MASK << p10_SDAR_MODE_SHIFT | \
(p10_EVENT_THRESH_MASK << EVENT_THRESH_SHIFT) | \
@@ -124,8 +128,8 @@
* 60 56 52 48 44 40 36 32
* | - - - - | - - - - | - - - - | - - - - | - - - - | - - - - | - - - - | - - - - |
* [ fab_match ] [ thresh_cmp ] [ thresh_ctl ] [ ]
- * |
- * thresh_sel -*
+ * | |
+ * [ thresh_cmp bits for p10] thresh_sel -*
*
* 28 24 20 16 12 8 4 0
* | - - - - | - - - - | - - - - | - - - - | - - - - | - - - - | - - - - | - - - - |
@@ -152,6 +156,9 @@
#define CNST_THRESH_CTL_SEL_VAL(v) (((v) & 0x7ffull) << 32)
#define CNST_THRESH_CTL_SEL_MASK CNST_THRESH_CTL_SEL_VAL(0x7ff)
+#define p10_CNST_THRESH_CMP_VAL(v) (((v) & 0x7ffull) << 43)
+#define p10_CNST_THRESH_CMP_MASK p10_CNST_THRESH_CMP_VAL(0x7ff)
+
#define CNST_EBB_VAL(v) (((v) & EVENT_EBB_MASK) << 24)
#define CNST_EBB_MASK CNST_EBB_VAL(EVENT_EBB_MASK)
@@ -262,10 +269,10 @@
#define PH(a, b) (P(LVL, HIT) | P(a, b))
#define PM(a, b) (P(LVL, MISS) | P(a, b))
-int isa207_get_constraint(u64 event, unsigned long *maskp, unsigned long *valp);
+int isa207_get_constraint(u64 event, unsigned long *maskp, unsigned long *valp, u64 event_config1);
int isa207_compute_mmcr(u64 event[], int n_ev,
unsigned int hwc[], struct mmcr_regs *mmcr,
- struct perf_event *pevents[]);
+ struct perf_event *pevents[], u32 flags);
void isa207_disable_pmc(unsigned int pmc, struct mmcr_regs *mmcr);
int isa207_get_alternatives(u64 event, u64 alt[], int size, unsigned int flags,
const unsigned int ev_alt[][MAX_ALT]);
diff --git a/arch/powerpc/perf/mpc7450-pmu.c b/arch/powerpc/perf/mpc7450-pmu.c
index 1919e9df9165..e39b15b79a83 100644
--- a/arch/powerpc/perf/mpc7450-pmu.c
+++ b/arch/powerpc/perf/mpc7450-pmu.c
@@ -148,7 +148,7 @@ static u32 classbits[N_CLASSES - 1][2] = {
};
static int mpc7450_get_constraint(u64 event, unsigned long *maskp,
- unsigned long *valp)
+ unsigned long *valp, u64 event_config1 __maybe_unused)
{
int pmc, class;
u32 mask, value;
@@ -258,7 +258,8 @@ static const u32 pmcsel_mask[N_COUNTER] = {
*/
static int mpc7450_compute_mmcr(u64 event[], int n_ev, unsigned int hwc[],
struct mmcr_regs *mmcr,
- struct perf_event *pevents[])
+ struct perf_event *pevents[],
+ u32 flags __maybe_unused)
{
u8 event_index[N_CLASSES][N_COUNTER];
int n_classevent[N_CLASSES];
diff --git a/arch/powerpc/perf/power10-pmu.c b/arch/powerpc/perf/power10-pmu.c
index 79e0206ca454..a901c1348cad 100644
--- a/arch/powerpc/perf/power10-pmu.c
+++ b/arch/powerpc/perf/power10-pmu.c
@@ -216,6 +216,7 @@ PMU_FORMAT_ATTR(invert_bit, "config:47");
PMU_FORMAT_ATTR(src_mask, "config:48-53");
PMU_FORMAT_ATTR(src_match, "config:54-59");
PMU_FORMAT_ATTR(radix_scope, "config:9");
+PMU_FORMAT_ATTR(thresh_cmp, "config1:0-17");
static struct attribute *power10_pmu_format_attr[] = {
&format_attr_event.attr,
@@ -236,6 +237,7 @@ static struct attribute *power10_pmu_format_attr[] = {
&format_attr_src_mask.attr,
&format_attr_src_match.attr,
&format_attr_radix_scope.attr,
+ &format_attr_thresh_cmp.attr,
NULL,
};
@@ -550,7 +552,7 @@ static struct power_pmu power10_pmu = {
.get_mem_weight = isa207_get_mem_weight,
.disable_pmc = isa207_disable_pmc,
.flags = PPMU_HAS_SIER | PPMU_ARCH_207S |
- PPMU_ARCH_31,
+ PPMU_ARCH_31 | PPMU_HAS_ATTR_CONFIG1,
.n_generic = ARRAY_SIZE(power10_generic_events),
.generic_events = power10_generic_events,
.cache_events = &power10_cache_events,
diff --git a/arch/powerpc/perf/power5+-pmu.c b/arch/powerpc/perf/power5+-pmu.c
index 3e64b4a1511f..18732267993a 100644
--- a/arch/powerpc/perf/power5+-pmu.c
+++ b/arch/powerpc/perf/power5+-pmu.c
@@ -132,7 +132,7 @@ static unsigned long unit_cons[PM_LASTUNIT+1][2] = {
};
static int power5p_get_constraint(u64 event, unsigned long *maskp,
- unsigned long *valp)
+ unsigned long *valp, u64 event_config1 __maybe_unused)
{
int pmc, byte, unit, sh;
int bit, fmask;
@@ -451,7 +451,8 @@ static int power5p_marked_instr_event(u64 event)
static int power5p_compute_mmcr(u64 event[], int n_ev,
unsigned int hwc[], struct mmcr_regs *mmcr,
- struct perf_event *pevents[])
+ struct perf_event *pevents[],
+ u32 flags __maybe_unused)
{
unsigned long mmcr1 = 0;
unsigned long mmcra = 0;
diff --git a/arch/powerpc/perf/power5-pmu.c b/arch/powerpc/perf/power5-pmu.c
index 017bb19b73fb..cb611c1e7abe 100644
--- a/arch/powerpc/perf/power5-pmu.c
+++ b/arch/powerpc/perf/power5-pmu.c
@@ -136,7 +136,7 @@ static unsigned long unit_cons[PM_LASTUNIT+1][2] = {
};
static int power5_get_constraint(u64 event, unsigned long *maskp,
- unsigned long *valp)
+ unsigned long *valp, u64 event_config1 __maybe_unused)
{
int pmc, byte, unit, sh;
int bit, fmask;
@@ -382,7 +382,8 @@ static int power5_marked_instr_event(u64 event)
static int power5_compute_mmcr(u64 event[], int n_ev,
unsigned int hwc[], struct mmcr_regs *mmcr,
- struct perf_event *pevents[])
+ struct perf_event *pevents[],
+ u32 flags __maybe_unused)
{
unsigned long mmcr1 = 0;
unsigned long mmcra = MMCRA_SDAR_DCACHE_MISS | MMCRA_SDAR_ERAT_MISS;
diff --git a/arch/powerpc/perf/power6-pmu.c b/arch/powerpc/perf/power6-pmu.c
index 189974478e9f..69ef38216418 100644
--- a/arch/powerpc/perf/power6-pmu.c
+++ b/arch/powerpc/perf/power6-pmu.c
@@ -173,7 +173,8 @@ static int power6_marked_instr_event(u64 event)
* Assign PMC numbers and compute MMCR1 value for a set of events
*/
static int p6_compute_mmcr(u64 event[], int n_ev,
- unsigned int hwc[], struct mmcr_regs *mmcr, struct perf_event *pevents[])
+ unsigned int hwc[], struct mmcr_regs *mmcr, struct perf_event *pevents[],
+ u32 flags __maybe_unused)
{
unsigned long mmcr1 = 0;
unsigned long mmcra = MMCRA_SDAR_DCACHE_MISS | MMCRA_SDAR_ERAT_MISS;
@@ -266,7 +267,7 @@ static int p6_compute_mmcr(u64 event[], int n_ev,
* 32-34 select field: nest (subunit) event selector
*/
static int p6_get_constraint(u64 event, unsigned long *maskp,
- unsigned long *valp)
+ unsigned long *valp, u64 event_config1 __maybe_unused)
{
int pmc, byte, sh, subunit;
unsigned long mask = 0, value = 0;
diff --git a/arch/powerpc/perf/power7-pmu.c b/arch/powerpc/perf/power7-pmu.c
index bacfab104a1a..894c17f9a762 100644
--- a/arch/powerpc/perf/power7-pmu.c
+++ b/arch/powerpc/perf/power7-pmu.c
@@ -81,7 +81,7 @@ enum {
*/
static int power7_get_constraint(u64 event, unsigned long *maskp,
- unsigned long *valp)
+ unsigned long *valp, u64 event_config1 __maybe_unused)
{
int pmc, sh, unit;
unsigned long mask = 0, value = 0;
@@ -245,7 +245,8 @@ static int power7_marked_instr_event(u64 event)
static int power7_compute_mmcr(u64 event[], int n_ev,
unsigned int hwc[], struct mmcr_regs *mmcr,
- struct perf_event *pevents[])
+ struct perf_event *pevents[],
+ u32 flags __maybe_unused)
{
unsigned long mmcr1 = 0;
unsigned long mmcra = MMCRA_SDAR_DCACHE_MISS | MMCRA_SDAR_ERAT_MISS;
diff --git a/arch/powerpc/perf/ppc970-pmu.c b/arch/powerpc/perf/ppc970-pmu.c
index 7d78df97f272..1f8263785286 100644
--- a/arch/powerpc/perf/ppc970-pmu.c
+++ b/arch/powerpc/perf/ppc970-pmu.c
@@ -190,7 +190,7 @@ static unsigned long unit_cons[PM_LASTUNIT+1][2] = {
};
static int p970_get_constraint(u64 event, unsigned long *maskp,
- unsigned long *valp)
+ unsigned long *valp, u64 event_config1 __maybe_unused)
{
int pmc, byte, unit, sh, spcsel;
unsigned long mask = 0, value = 0;
@@ -256,7 +256,8 @@ static int p970_get_alternatives(u64 event, unsigned int flags, u64 alt[])
static int p970_compute_mmcr(u64 event[], int n_ev,
unsigned int hwc[], struct mmcr_regs *mmcr,
- struct perf_event *pevents[])
+ struct perf_event *pevents[],
+ u32 flags __maybe_unused)
{
unsigned long mmcr0 = 0, mmcr1 = 0, mmcra = 0;
unsigned int pmc, unit, byte, psel;
--
2.25.1
^ permalink raw reply related
* [PATCH] powerpc/powernv/pci: Use kzalloc() for phb related allocations
From: Michael Ellerman @ 2021-02-11 11:27 UTC (permalink / raw)
To: linuxppc-dev
As part of commit fbbefb320214 ("powerpc/pci: Move PHB discovery for
PCI_DN using platforms"), I switched some allocations from
memblock_alloc() to kmalloc(), otherwise memblock would warn that it
was being called after slab init.
However I missed that the code relied on the allocations being zeroed,
without which we could end up crashing:
pci_bus 0000:00: busn_res: [bus 00-ff] end is updated to ff
BUG: Unable to handle kernel data access on read at 0x6b6b6b6b6b6b6af7
Faulting instruction address: 0xc0000000000dbc90
Oops: Kernel access of bad area, sig: 11 [#1]
LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA PowerNV
...
NIP pnv_ioda_get_pe_state+0xe0/0x1d0
LR pnv_ioda_get_pe_state+0xb4/0x1d0
Call Trace:
pnv_ioda_get_pe_state+0xb4/0x1d0 (unreliable)
pnv_pci_config_check_eeh.isra.9+0x78/0x270
pnv_pci_read_config+0xf8/0x160
pci_bus_read_config_dword+0xa4/0x120
pci_bus_generic_read_dev_vendor_id+0x54/0x270
pci_scan_single_device+0xb8/0x140
pci_scan_slot+0x80/0x1b0
pci_scan_child_bus_extend+0x94/0x490
pcibios_scan_phb+0x1f8/0x3c0
pcibios_init+0x8c/0x12c
do_one_initcall+0x94/0x510
kernel_init_freeable+0x35c/0x3fc
kernel_init+0x2c/0x168
ret_from_kernel_thread+0x5c/0x70
Switch them to kzalloc().
Fixes: fbbefb320214 ("powerpc/pci: Move PHB discovery for PCI_DN using platforms")
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/platforms/powernv/pci-ioda.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index 7ee14ac275bd..f0f901683a2f 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -2921,7 +2921,7 @@ static void __init pnv_pci_init_ioda_phb(struct device_node *np,
phb_id = be64_to_cpup(prop64);
pr_debug(" PHB-ID : 0x%016llx\n", phb_id);
- phb = kmalloc(sizeof(*phb), GFP_KERNEL);
+ phb = kzalloc(sizeof(*phb), GFP_KERNEL);
if (!phb)
panic("%s: Failed to allocate %zu bytes\n", __func__,
sizeof(*phb));
@@ -2970,7 +2970,7 @@ static void __init pnv_pci_init_ioda_phb(struct device_node *np,
else
phb->diag_data_size = PNV_PCI_DIAG_BUF_SIZE;
- phb->diag_data = kmalloc(phb->diag_data_size, GFP_KERNEL);
+ phb->diag_data = kzalloc(phb->diag_data_size, GFP_KERNEL);
if (!phb->diag_data)
panic("%s: Failed to allocate %u bytes\n", __func__,
phb->diag_data_size);
@@ -3032,7 +3032,7 @@ static void __init pnv_pci_init_ioda_phb(struct device_node *np,
}
pemap_off = size;
size += phb->ioda.total_pe_num * sizeof(struct pnv_ioda_pe);
- aux = kmalloc(size, GFP_KERNEL);
+ aux = kzalloc(size, GFP_KERNEL);
if (!aux)
panic("%s: Failed to allocate %lu bytes\n", __func__, size);
--
2.25.1
^ permalink raw reply related
* Re: [PATCH] powerpc/bug: Remove specific powerpc BUG_ON()
From: Segher Boessenkool @ 2021-02-11 11:49 UTC (permalink / raw)
To: Christophe Leroy; +Cc: linux-kernel, npiggin, Paul Mackerras, linuxppc-dev
In-Reply-To: <694c7195c81d1bcc781b3c14f452886683d6c524.1613029237.git.christophe.leroy@csgroup.eu>
On Thu, Feb 11, 2021 at 07:41:52AM +0000, Christophe Leroy wrote:
> powerpc BUG_ON() is based on using twnei or tdnei instruction,
> which obliges gcc to format the condition into a 0 or 1 value
> in a register.
Huh? Why is that?
Will it work better if this used __builtin_trap? Or does the kernel only
detect very specific forms of trap instructions?
> By using a generic implementation, gcc will generate a branch
> to the unconditional trap generated by BUG().
That is many more instructions than ideal.
> As modern powerpc implement branch folding, that's even more efficient.
What PowerPC cpus implement branch folding? I know none.
Some example code generated via __builtin_trap:
void trap(void) { __builtin_trap(); }
void trap_if_0(int x) { if (x == 0) __builtin_trap(); }
void trap_if_not_0(int x) { if (x != 0) __builtin_trap(); }
-m64:
trap:
trap
trap_if_0:
tdeqi 3,0
blr
trap_if_not_0:
tdnei 3,0
blr
-m32:
trap:
trap
trap_if_0:
tweqi 3,0
blr
trap_if_not_0:
twnei 3,0
blr
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