LinuxPPC-Dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH 2/2] powerpc: make 'boot_text_mapped' static
From: Segher Boessenkool @ 2021-04-09 12:02 UTC (permalink / raw)
  To: Christophe Leroy; +Cc: Yu Kuai, yi.zhang, linuxppc-dev, linux-kernel
In-Reply-To: <5b91cd3f-d171-f510-7dae-8cbabb13b23c@csgroup.eu>

Hi!

On Thu, Apr 08, 2021 at 07:04:35AM +0200, Christophe Leroy wrote:
> Le 08/04/2021 à 03:18, Yu Kuai a écrit :
> >-int boot_text_mapped __force_data = 0;
> >+static int boot_text_mapped __force_data;
> 
> Are you sure the initialisation to 0 can be removed ? Usually 
> initialisation to 0 is not needed because not initialised variables go in 
> the BSS section which is zeroed at startup. But here the variable is 
> flagged with __force_data so it is not going in the BSS section.

Any non-automatic (i.e. function-scope, not static) variable is
initialised to 0.  See e.g. C11 6.7.9/10 (this has been like that since
times immemorial, C90 anyway).


Segher

^ permalink raw reply

* Re: [PATCH] powerpc/perf: Fix PMU callbacks to clear pending PMI before resetting an overflown PMC
From: Athira Rajeev @ 2021-04-09 12:53 UTC (permalink / raw)
  To: Nicholas Piggin; +Cc: nasastry@in.ibm.com, Madhavan Srinivasan, linuxppc-dev
In-Reply-To: <1617927471.vhjclnvhj3.astroid@bobo.none>



> On 09-Apr-2021, at 6:38 AM, Nicholas Piggin <npiggin@gmail.com> wrote:
> 
Hi Nick,

Thanks for checking the patch and sharing review comments.

> I was going to nitpick "overflown" here as something birds do, but some
> sources says overflown is okay for past tense.
> 
> You could use "overflowed" for that, but I understand the issue with the 
> word: you are talking about counters that are currently in an "overflow" 
> state, but the overflow occurred in the past and is not still happening
> so you "overflowing" doesn't exactly fit either.
> 
> overflown kind of works for some reason you can kind of use it for
> present tense!

Ok sure, Yes counter is currently in an “overflow” state.

> 
> Excerpts from Athira Rajeev's message of April 7, 2021 12:47 am:
>> Running perf fuzzer showed below in dmesg logs:
>> "Can't find PMC that caused IRQ"
>> 
>> This means a PMU exception happened, but none of the PMC's (Performance
>> Monitor Counter) were found to be overflown. There are some corner cases
>> that clears the PMCs after PMI gets masked. In such cases, the perf
>> interrupt handler will not find the active PMC values that had caused
>> the overflow and thus leads to this message while replaying.
>> 
>> Case 1: PMU Interrupt happens during replay of other interrupts and
>> counter values gets cleared by PMU callbacks before replay:
>> 
>> During replay of interrupts like timer, __do_irq and doorbell exception, we
>> conditionally enable interrupts via may_hard_irq_enable(). This could
>> potentially create a window to generate a PMI. Since irq soft mask is set
>> to ALL_DISABLED, the PMI will get masked here.
> 
> I wonder if may_hard_irq_enable shouldn't enable if PMI is soft
> disabled. And also maybe replay should not set ALL_DISABLED if
> there are no PMI interrupts pending.
> 
> Still, I think those are a bit more tricky and might take a while
> to get right or just not be worth while, so I think your patch is
> fine.

Ok Nick.
> 
>> We could get IPIs run before
>> perf interrupt is replayed and the PMU events could deleted or stopped.
>> This will change the PMU SPR values and resets the counters. Snippet of
>> ftrace log showing PMU callbacks invoked in "__do_irq":
>> 
>> <idle>-0 [051] dns. 132025441306354: __do_irq <-call_do_irq
>> <idle>-0 [051] dns. 132025441306430: irq_enter <-__do_irq
>> <idle>-0 [051] dns. 132025441306503: irq_enter_rcu <-__do_irq
>> <idle>-0 [051] dnH. 132025441306599: xive_get_irq <-__do_irq
>> <<>>
>> <idle>-0 [051] dnH. 132025441307770: generic_smp_call_function_single_interrupt <-smp_ipi_demux_relaxed
>> <idle>-0 [051] dnH. 132025441307839: flush_smp_call_function_queue <-smp_ipi_demux_relaxed
>> <idle>-0 [051] dnH. 132025441308057: _raw_spin_lock <-event_function
>> <idle>-0 [051] dnH. 132025441308206: power_pmu_disable <-perf_pmu_disable
>> <idle>-0 [051] dnH. 132025441308337: power_pmu_del <-event_sched_out
>> <idle>-0 [051] dnH. 132025441308407: power_pmu_read <-power_pmu_del
>> <idle>-0 [051] dnH. 132025441308477: read_pmc <-power_pmu_read
>> <idle>-0 [051] dnH. 132025441308590: isa207_disable_pmc <-power_pmu_del
>> <idle>-0 [051] dnH. 132025441308663: write_pmc <-power_pmu_del
>> <idle>-0 [051] dnH. 132025441308787: power_pmu_event_idx <-perf_event_update_userpage
>> <idle>-0 [051] dnH. 132025441308859: rcu_read_unlock_strict <-perf_event_update_userpage
>> <idle>-0 [051] dnH. 132025441308975: power_pmu_enable <-perf_pmu_enable
>> <<>>
>> <idle>-0 [051] dnH. 132025441311108: irq_exit <-__do_irq
>> <idle>-0 [051] dns. 132025441311319: performance_monitor_exception <-replay_soft_interrupts
>> 
>> Case 2: PMI's masked during local_* operations, example local_add.
>> If the local_add operation happens within a local_irq_save, replay of
>> PMI will be during local_irq_restore. Similar to case 1, this could
>> also create a window before replay where PMU events gets deleted or
>> stopped.
> 
> Here as well perhaps PMIs should be replayed if they are unmasked
> even if other interrupts are still masked. Again that might be more
> complexity than it's worth.
Ok..

> 
>> 
>> Patch adds a fix to update the PMU callback functions (del,stop,enable) to
>> check for pending perf interrupt. If there is an overflown PMC and pending
>> perf interrupt indicated in Paca, clear the PMI bit in paca to drop that
>> sample. In case of power_pmu_del, also clear the MMCR0 PMAO bit which
>> otherwise could lead to spurious interrupts in some corner cases. Example,
>> a timer after power_pmu_del which will re-enable interrupts since PMI is
>> cleared and triggers a PMI again since PMAO bit is still set.
>> 
>> We can't just replay PMI any time. Hence this approach is preferred rather
>> than replaying PMI before resetting overflown PMC. Patch also documents
>> core-book3s on a race condition which can trigger these PMC messages during
>> idle path in PowerNV.
>> 
>> Fixes: f442d004806e ("powerpc/64s: Add support to mask perf interrupts and replay them")
>> Reported-by: Nageswara R Sastry <nasastry@in.ibm.com>
>> Suggested-by: Nicholas Piggin <npiggin@gmail.com>
>> Suggested-by: Madhavan Srinivasan <maddy@linux.ibm.com>
>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>> ---
>> arch/powerpc/include/asm/pmc.h  | 11 +++++++++
>> arch/powerpc/perf/core-book3s.c | 55 +++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 66 insertions(+)
>> 
>> diff --git a/arch/powerpc/include/asm/pmc.h b/arch/powerpc/include/asm/pmc.h
>> index c6bbe9778d3c..97b4bd8de25b 100644
>> --- a/arch/powerpc/include/asm/pmc.h
>> +++ b/arch/powerpc/include/asm/pmc.h
>> @@ -34,11 +34,22 @@ static inline void ppc_set_pmu_inuse(int inuse)
>> #endif
>> }
>> 
>> +static inline int clear_paca_irq_pmi(void)
>> +{
>> +	if (get_paca()->irq_happened & PACA_IRQ_PMI) {
>> +		WARN_ON_ONCE(mfmsr() & MSR_EE);
>> +		get_paca()->irq_happened &= ~PACA_IRQ_PMI;
>> +		return 1;
>> +	}
>> +	return 0;
>> +}
> 
> Could you put this in arch/powerpc/include/asm/hw_irq.h and
> rather than paca_irq, call it irq_pending perhaps
> 
>  clear_pmi_irq_pending()
> 
>  get_clear_pmi_irq_pending() if you're also testing it.

Sure,  I will use “get_clear_pmi_irq_pending()” and try with moving this to arch/powerpc/include/asm/hw_irq.h

> 
> Could you add a little comment about the corner cases above it too?
> The root cause seem to be interrupt replay while a masked PMI is
> pending can result in other interrupts arriving which clear the PMU
> overflow so the pending PMI must be cleared.

Ok, I will add comment and fix this in next version.

> 
>> +
>> extern void power4_enable_pmcs(void);
>> 
>> #else /* CONFIG_PPC64 */
>> 
>> static inline void ppc_set_pmu_inuse(int inuse) { }
>> +static inline int clear_paca_irq_pmi(void) { return 0; }
>> 
>> #endif
>> 
>> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
>> index 766f064f00fb..18ca3c90f866 100644
>> --- a/arch/powerpc/perf/core-book3s.c
>> +++ b/arch/powerpc/perf/core-book3s.c
>> @@ -847,6 +847,20 @@ static void write_pmc(int idx, unsigned long val)
>> 	}
>> }
>> 
>> +static int pmc_overflown(int idx)
>> +{
>> +	unsigned long val[8];
>> +	int i;
>> +
>> +	for (i = 0; i < ppmu->n_counter; i++)
>> +		val[i] = read_pmc(i + 1);
>> +
>> +	if ((int)val[idx-1] < 0)
>> +		return 1;
>> +
>> +	return 0;
>> +}
>> +
>> /* Called from sysrq_handle_showregs() */
>> void perf_event_print_debug(void)
>> {
>> @@ -1438,6 +1452,15 @@ static void power_pmu_enable(struct pmu *pmu)
>> 		event = cpuhw->event[i];
>> 		if (event->hw.idx && event->hw.idx != hwc_index[i] + 1) {
>> 			power_pmu_read(event);
>> +			/*
>> +			 * if the PMC corresponding to event->hw.idx is
>> +			 * overflown, check if there is any pending perf
>> +			 * interrupt set in paca. If so, disable the interrupt
>> +			 * by clearing the paca bit for PMI since we are going
>> +			 * to reset the PMC.
>> +			 */
>> +			if (pmc_overflown(event->hw.idx))
>> +				clear_paca_irq_pmi();
> 
> If the pmc is not overflown, could there still be a PMI pending?

I didn’t hit that scenario where PMI is pending without an overflown PMC.
Also I believe if such a case happens, we will need an investigation there. It could be a different case to be handled.

I actually considered below two points for adding this PMC check instead of just clearing the PMI.

1. Make sure we are not masking any bug here by just clearing PACA_IRQ_PMI.
Ideally if PMI is set in irq_happened, it means there was a counter overflow.
2. If there is more than one PMU event, say two events. Make sure we are clearing PMI only for the
event whose counter is overflown.
 
> 
>> 			write_pmc(event->hw.idx, 0);
>> 			event->hw.idx = 0;
>> 		}
>> @@ -1474,6 +1497,10 @@ static void power_pmu_enable(struct pmu *pmu)
>> 		event->hw.idx = idx;
>> 		if (event->hw.state & PERF_HES_STOPPED)
>> 			val = 0;
>> +
>> +		/* See above for clear_paca_irq_pmi */
>> +		if (pmc_overflown(event->hw.idx))
>> +			clear_paca_irq_pmi();
>> 		write_pmc(idx, val);
>> 
>> 		perf_event_update_userpage(event);
>> @@ -1619,6 +1646,7 @@ static void power_pmu_del(struct perf_event *event, int ef_flags)
>> 	struct cpu_hw_events *cpuhw;
>> 	long i;
>> 	unsigned long flags;
>> +	unsigned long val_mmcr0;
>> 
>> 	local_irq_save(flags);
>> 	perf_pmu_disable(event->pmu);
>> @@ -1636,6 +1664,22 @@ static void power_pmu_del(struct perf_event *event, int ef_flags)
>> 			--cpuhw->n_events;
>> 			ppmu->disable_pmc(event->hw.idx - 1, &cpuhw->mmcr);
>> 			if (event->hw.idx) {
>> +				/*
>> +				 * if the PMC corresponding to event->hw.idx is
>> +				 * overflown, check if there is any pending perf
>> +				 * interrupt set in paca. If so, disable the interrupt
>> +				 * and clear the MMCR0 PMAO bit since we are going
>> +				 * to reset the PMC and delete the event.
>> +				 */
>> +				if (pmc_overflown(event->hw.idx)) {
>> +					if (clear_paca_irq_pmi()) {
>> +						val_mmcr0 = mfspr(SPRN_MMCR0);
>> +						val_mmcr0 &= ~MMCR0_PMAO;
>> +						write_mmcr0(cpuhw, val_mmcr0);
>> +						mb();
>> +						isync();
> 
> I don't know the perf subsystem, but just out of curiosity why does
> MMCR0 need to be cleared only in this case?

I got a corner case in power_pmu_del, with only clearing PACA_IRQ_PMI and without resetting MMCR0 PMAO bit.
Here is the flow:

1. We clear the PMI bit Paca, but MMCR0 has the PMAO bit still set. PMAO bit indicates a PMI has occurred.
2. A timer interrupt is replayed after power_pmu_del which does a “may_hard_irq_enable”.
This will re-enable interrupts and triggers a PMI again since PMAO bit is still set.

So clear PMAO bit to avoid such spurious interrupts.
Ftrace logs showing the same with some debug trace_printks :

     <idle>-0    [134] d.h. 327287888478: power_pmu_del <-event_sched_out.isra.126
     <<>>    Here we cleared the PMI
     <idle>-0    [134] d.h. 327287889272: write_pmc <-power_pmu_del
     <idle>-0    [134] d.h. 327287889346: rcu_read_unlock_strict <-perf_event_update_userpage
     <idle>-0    [134] d.h. 327287889711: power_pmu_del: In power_pmu_del MMCR0 is 82004090, local_paca->irq_happened is 9
     <idle>-0    [134] d.h. 327287889811: power_pmu_enable <-perf_pmu_enable
     <idle>-0    [134] d.h. 327287889982: irq_exit <-doorbell_exception
     <idle>-0    [134] d... 327287890053: idle_cpu <-irq_exit
     <idle>-0    [134] d... 327287890158: tick_nohz_irq_exit <-irq_exit
     <idle>-0    [134] d... 327287890219: ktime_get <-tick_nohz_irq_exit
     <idle>-0    [134] d... 327287890328: replay_soft_interrupts <-interrupt_exit_kernel_prepare
     <idle>-0    [134] d... 327287890399: irq_enter <-timer_interrupt
     <<>>
     <idle>-0    [134] d.h. 327287891163: timer_interrupt: Before may_hard_irq_enable MMCR0 is 82004090, local_paca->irq_happened is 1
     <<>>
     <idle>-0    [134] d.h. 327287894310: timer_interrupt: After may_hard_irq_enable MMCR0 is 82004090, local_paca->irq_happened is 21

In case of other callbacks like pmu enable, we are programming MMCR0. But in case of event getting deleted, there is no
way we clear PMAO unless an event gets scheduled again in that cpu. Hence added this check only in pmu_del callback.
 

> What if we disabled MSR[EE]
> right before a perf interrupt came in, so we don't get a pending PMI
> but the condition is still close to the same.

Nick, I didn’t get this question exactly. Can you please help explain a bit ?
From my understanding, consider that we disabled MSR[EE] before perf interrupt came in.
So once the interrupts are re-enabled:

1. If soft mask is set to IRQS_DISABLED, perf interrupt will be triggered as NMI.
2. In case of ALL_DISABLED, it will be masked for replay. If PMU callbacks are invoked before replay,
our present patch will take care of clearing PMI in corner cases.

Thanks
Athira.
> 
>> +				}
>> 				write_pmc(event->hw.idx, 0);
>> 				event->hw.idx = 0;
>> 			}
>> @@ -1714,6 +1758,8 @@ static void power_pmu_stop(struct perf_event *event, int ef_flags)
>> 
>> 	local_irq_save(flags);
>> 	perf_pmu_disable(event->pmu);
>> +	if (pmc_overflown(event->hw.idx))
>> +		clear_paca_irq_pmi();
>> 
>> 	power_pmu_read(event);
>> 	event->hw.state |= PERF_HES_STOPPED | PERF_HES_UPTODATE;
>> @@ -2343,6 +2389,15 @@ static void __perf_event_interrupt(struct pt_regs *regs)
>> 			}
>> 		}
>> 	}
>> +
>> +	/*
>> +	 * During system wide profling or while specific CPU
>> +	 * is monitored for an event, some corner cases could
>> +	 * cause PMC to overflow in idle path. This will trigger
>> +	 * a PMI after waking up from idle. Since counter values
>> +	 * are _not_ saved/restored in idle path, can lead to
>> +	 * below "Can't find PMC" message.
>> +	 */
>> 	if (unlikely(!found) && !arch_irq_disabled_regs(regs))
>> 		printk_ratelimited(KERN_WARNING "Can't find PMC that caused IRQ\n");
>> 
>> -- 
>> 1.8.3.1


^ permalink raw reply

* [PATCH v2 1/1] kernel.h: Split out panic and oops helpers
From: Andy Shevchenko @ 2021-04-09 10:02 UTC (permalink / raw)
  To: Thomas Bogendoerfer, Alexander Lobakin, Wei Liu, Rasmus Villemoes,
	Arnd Bergmann, Bjorn Andersson, Christian Brauner,
	Andy Shevchenko, Andrew Morton, Kees Cook, Mike Rapoport,
	Corey Minyard, Michael Ellerman, Vasily Gorbik, Jason J. Herne,
	Joerg Roedel, Michael Kelley, Joe Perches, Florian Fainelli,
	Krzysztof Kozlowski, Greg Kroah-Hartman, Scott Branden,
	Olof Johansson, Mihai Carabas, Wang Wenhu, Marek Czerski,
	Hongbo Yao, Mathieu Poirier, Vineeth Vijayan, Heiko Carstens,
	Peter Oberparleiter, Alexander Egorenkov, Tetsuo Handa,
	Vlastimil Babka, Paul E. McKenney, Steven Rostedt (VMware),
	linux-alpha, linux-kernel, linux-arm-kernel, linux-mips,
	linux-parisc, linuxppc-dev, linux-s390, sparclinux, linux-um,
	linux-hyperv, xen-devel, linux-xtensa, openipmi-developer,
	linux-clk, linux-edac, coresight, linux-leds,
	bcm-kernel-feedback-list, netdev, linux-pm, linux-remoteproc,
	linux-staging, dri-devel, linux-fbdev, linux-arch, kexec, rcu,
	linux-fsdevel
  Cc: Alexander Shishkin, Catalin Marinas, Michael Turquette,
	Lai Jiangshan, James E.J. Bottomley, Max Filippov, Paul Mackerras,
	Pavel Machek, H. Peter Anvin, Joel Fernandes, K. Y. Srinivasan,
	Will Deacon, Boris Ostrovsky, Anton Ivanov, Stefano Stabellini,
	Stephen Hemminger, Daniel Drake, Jens Frederich,
	Richard Weinberger, Helge Deller, x86, Mike Rapoport,
	Christian Borntraeger, Ingo Molnar, Mathieu Desnoyers,
	Jakub Kicinski, Matt Turner, Iurii Zaikin, Haiyang Zhang,
	Mike Leach, Ohad Ben-Cohen, Robert Richter, Corey Minyard,
	Suzuki K Poulose, Jeff Dike, Josh Triplett, Alex Elder,
	Ivan Kokshaysky, Paul Walmsley, Thomas Gleixner,
	Mauro Carvalho Chehab, Richard Henderson, Juergen Gross,
	Chris Zankel, Tony Luck, Jon Nettleton, Stephen Boyd,
	Sebastian Reichel, Dinh Nguyen, Luis Chamberlain, James Morse,
	Eric Biederman, Leo Yan, Borislav Petkov, David S. Miller

kernel.h is being used as a dump for all kinds of stuff for a long time.
Here is the attempt to start cleaning it up by splitting out panic and
oops helpers.

There are several purposes of doing this:
- dropping dependency in bug.h
- dropping a loop by moving out panic_notifier.h
- unload kernel.h from something which has its own domain

At the same time convert users tree-wide to use new headers, although
for the time being include new header back to kernel.h to avoid twisted
indirected includes for existing users.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Acked-by: Mike Rapoport <rppt@linux.ibm.com>
Acked-by: Corey Minyard <cminyard@mvista.com>
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Kees Cook <keescook@chromium.org>
Acked-by: Wei Liu <wei.liu@kernel.org>
Acked-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
v2:
 - fixed all errors with allmodconfig on x86_64 (Andrew)
 - checked with allyesconfig on x86_64
 - additionally grepped source code for panic notifier list usage
   and converted all users
 - elaborated commit message (Luis)
 - collected given tags (incl. Andrew's SoB, see below)

I added Andrew's SoB since part of the fixes I took from him. Andrew,
feel free to amend or tell me how you want me to do.

 arch/alpha/kernel/setup.c                     |  2 +-
 arch/arm64/kernel/setup.c                     |  1 +
 arch/mips/kernel/relocate.c                   |  1 +
 arch/mips/sgi-ip22/ip22-reset.c               |  1 +
 arch/mips/sgi-ip32/ip32-reset.c               |  1 +
 arch/parisc/kernel/pdc_chassis.c              |  1 +
 arch/powerpc/kernel/setup-common.c            |  1 +
 arch/s390/kernel/ipl.c                        |  1 +
 arch/sparc/kernel/sstate.c                    |  1 +
 arch/um/drivers/mconsole_kern.c               |  1 +
 arch/um/kernel/um_arch.c                      |  1 +
 arch/x86/include/asm/desc.h                   |  1 +
 arch/x86/kernel/cpu/mshyperv.c                |  1 +
 arch/x86/kernel/setup.c                       |  1 +
 arch/x86/purgatory/purgatory.c                |  2 +
 arch/x86/xen/enlighten.c                      |  1 +
 arch/xtensa/platforms/iss/setup.c             |  1 +
 drivers/bus/brcmstb_gisb.c                    |  1 +
 drivers/char/ipmi/ipmi_msghandler.c           |  1 +
 drivers/clk/analogbits/wrpll-cln28hpc.c       |  4 +
 drivers/edac/altera_edac.c                    |  1 +
 drivers/firmware/google/gsmi.c                |  1 +
 drivers/hv/vmbus_drv.c                        |  1 +
 .../hwtracing/coresight/coresight-cpu-debug.c |  1 +
 drivers/leds/trigger/ledtrig-activity.c       |  1 +
 drivers/leds/trigger/ledtrig-heartbeat.c      |  1 +
 drivers/leds/trigger/ledtrig-panic.c          |  1 +
 drivers/misc/bcm-vk/bcm_vk_dev.c              |  1 +
 drivers/misc/ibmasm/heartbeat.c               |  1 +
 drivers/misc/pvpanic/pvpanic.c                |  1 +
 drivers/net/ipa/ipa_smp2p.c                   |  1 +
 drivers/parisc/power.c                        |  1 +
 drivers/power/reset/ltc2952-poweroff.c        |  1 +
 drivers/remoteproc/remoteproc_core.c          |  1 +
 drivers/s390/char/con3215.c                   |  1 +
 drivers/s390/char/con3270.c                   |  1 +
 drivers/s390/char/sclp.c                      |  1 +
 drivers/s390/char/sclp_con.c                  |  1 +
 drivers/s390/char/sclp_vt220.c                |  1 +
 drivers/s390/char/zcore.c                     |  1 +
 drivers/soc/bcm/brcmstb/pm/pm-arm.c           |  1 +
 drivers/staging/olpc_dcon/olpc_dcon.c         |  1 +
 drivers/video/fbdev/hyperv_fb.c               |  1 +
 include/asm-generic/bug.h                     |  3 +-
 include/linux/kernel.h                        | 84 +---------------
 include/linux/panic.h                         | 98 +++++++++++++++++++
 include/linux/panic_notifier.h                | 12 +++
 kernel/hung_task.c                            |  1 +
 kernel/kexec_core.c                           |  1 +
 kernel/panic.c                                |  1 +
 kernel/rcu/tree.c                             |  2 +
 kernel/sysctl.c                               |  1 +
 kernel/trace/trace.c                          |  1 +
 53 files changed, 167 insertions(+), 85 deletions(-)
 create mode 100644 include/linux/panic.h
 create mode 100644 include/linux/panic_notifier.h

diff --git a/arch/alpha/kernel/setup.c b/arch/alpha/kernel/setup.c
index 03dda3beb3bd..5d1296534682 100644
--- a/arch/alpha/kernel/setup.c
+++ b/arch/alpha/kernel/setup.c
@@ -28,6 +28,7 @@
 #include <linux/init.h>
 #include <linux/string.h>
 #include <linux/ioport.h>
+#include <linux/panic_notifier.h>
 #include <linux/platform_device.h>
 #include <linux/memblock.h>
 #include <linux/pci.h>
@@ -46,7 +47,6 @@
 #include <linux/log2.h>
 #include <linux/export.h>
 
-extern struct atomic_notifier_head panic_notifier_list;
 static int alpha_panic_event(struct notifier_block *, unsigned long, void *);
 static struct notifier_block alpha_panic_block = {
 	alpha_panic_event,
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 61845c0821d9..787bc0f601b3 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -23,6 +23,7 @@
 #include <linux/interrupt.h>
 #include <linux/smp.h>
 #include <linux/fs.h>
+#include <linux/panic_notifier.h>
 #include <linux/proc_fs.h>
 #include <linux/memblock.h>
 #include <linux/of_fdt.h>
diff --git a/arch/mips/kernel/relocate.c b/arch/mips/kernel/relocate.c
index 499a5357c09f..56b51de2dc51 100644
--- a/arch/mips/kernel/relocate.c
+++ b/arch/mips/kernel/relocate.c
@@ -18,6 +18,7 @@
 #include <linux/kernel.h>
 #include <linux/libfdt.h>
 #include <linux/of_fdt.h>
+#include <linux/panic_notifier.h>
 #include <linux/sched/task.h>
 #include <linux/start_kernel.h>
 #include <linux/string.h>
diff --git a/arch/mips/sgi-ip22/ip22-reset.c b/arch/mips/sgi-ip22/ip22-reset.c
index c374f3ceec38..9028dbbb45dd 100644
--- a/arch/mips/sgi-ip22/ip22-reset.c
+++ b/arch/mips/sgi-ip22/ip22-reset.c
@@ -12,6 +12,7 @@
 #include <linux/kernel.h>
 #include <linux/sched/signal.h>
 #include <linux/notifier.h>
+#include <linux/panic_notifier.h>
 #include <linux/pm.h>
 #include <linux/timer.h>
 
diff --git a/arch/mips/sgi-ip32/ip32-reset.c b/arch/mips/sgi-ip32/ip32-reset.c
index 20d8637340be..18d1c115cd53 100644
--- a/arch/mips/sgi-ip32/ip32-reset.c
+++ b/arch/mips/sgi-ip32/ip32-reset.c
@@ -12,6 +12,7 @@
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/panic_notifier.h>
 #include <linux/sched.h>
 #include <linux/sched/signal.h>
 #include <linux/notifier.h>
diff --git a/arch/parisc/kernel/pdc_chassis.c b/arch/parisc/kernel/pdc_chassis.c
index 75ae88d13909..da154406d368 100644
--- a/arch/parisc/kernel/pdc_chassis.c
+++ b/arch/parisc/kernel/pdc_chassis.c
@@ -20,6 +20,7 @@
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
+#include <linux/panic_notifier.h>
 #include <linux/reboot.h>
 #include <linux/notifier.h>
 #include <linux/cache.h>
diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
index 74a98fff2c2f..046fe21b5c3b 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -9,6 +9,7 @@
 #undef DEBUG
 
 #include <linux/export.h>
+#include <linux/panic_notifier.h>
 #include <linux/string.h>
 #include <linux/sched.h>
 #include <linux/init.h>
diff --git a/arch/s390/kernel/ipl.c b/arch/s390/kernel/ipl.c
index 7a21eca498aa..e0b675dd7080 100644
--- a/arch/s390/kernel/ipl.c
+++ b/arch/s390/kernel/ipl.c
@@ -13,6 +13,7 @@
 #include <linux/init.h>
 #include <linux/device.h>
 #include <linux/delay.h>
+#include <linux/panic_notifier.h>
 #include <linux/reboot.h>
 #include <linux/ctype.h>
 #include <linux/fs.h>
diff --git a/arch/sparc/kernel/sstate.c b/arch/sparc/kernel/sstate.c
index ac8677c3841e..3bcc4ddc6911 100644
--- a/arch/sparc/kernel/sstate.c
+++ b/arch/sparc/kernel/sstate.c
@@ -6,6 +6,7 @@
 
 #include <linux/kernel.h>
 #include <linux/notifier.h>
+#include <linux/panic_notifier.h>
 #include <linux/reboot.h>
 #include <linux/init.h>
 
diff --git a/arch/um/drivers/mconsole_kern.c b/arch/um/drivers/mconsole_kern.c
index 6d00af25ec6b..328b16f99b30 100644
--- a/arch/um/drivers/mconsole_kern.c
+++ b/arch/um/drivers/mconsole_kern.c
@@ -12,6 +12,7 @@
 #include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/notifier.h>
+#include <linux/panic_notifier.h>
 #include <linux/reboot.h>
 #include <linux/sched/debug.h>
 #include <linux/proc_fs.h>
diff --git a/arch/um/kernel/um_arch.c b/arch/um/kernel/um_arch.c
index 74e07e748a9b..9512253947d5 100644
--- a/arch/um/kernel/um_arch.c
+++ b/arch/um/kernel/um_arch.c
@@ -7,6 +7,7 @@
 #include <linux/init.h>
 #include <linux/mm.h>
 #include <linux/module.h>
+#include <linux/panic_notifier.h>
 #include <linux/seq_file.h>
 #include <linux/string.h>
 #include <linux/utsname.h>
diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h
index 476082a83d1c..ceb12683b6d1 100644
--- a/arch/x86/include/asm/desc.h
+++ b/arch/x86/include/asm/desc.h
@@ -9,6 +9,7 @@
 #include <asm/irq_vectors.h>
 #include <asm/cpu_entry_area.h>
 
+#include <linux/debug_locks.h>
 #include <linux/smp.h>
 #include <linux/percpu.h>
 
diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
index 22f13343b5da..9e5c6f2b044d 100644
--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c
@@ -17,6 +17,7 @@
 #include <linux/irq.h>
 #include <linux/kexec.h>
 #include <linux/i8253.h>
+#include <linux/panic_notifier.h>
 #include <linux/random.h>
 #include <asm/processor.h>
 #include <asm/hypervisor.h>
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 59e5e0903b0c..570699eecf90 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -14,6 +14,7 @@
 #include <linux/initrd.h>
 #include <linux/iscsi_ibft.h>
 #include <linux/memblock.h>
+#include <linux/panic_notifier.h>
 #include <linux/pci.h>
 #include <linux/root_dev.h>
 #include <linux/hugetlb.h>
diff --git a/arch/x86/purgatory/purgatory.c b/arch/x86/purgatory/purgatory.c
index f03b64d9cb51..7558139920f8 100644
--- a/arch/x86/purgatory/purgatory.c
+++ b/arch/x86/purgatory/purgatory.c
@@ -9,6 +9,8 @@
  */
 
 #include <linux/bug.h>
+#include <linux/kernel.h>
+#include <linux/types.h>
 #include <crypto/sha2.h>
 #include <asm/purgatory.h>
 
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index aa9f50fccc5d..c79bd0af2e8c 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -6,6 +6,7 @@
 #include <linux/cpu.h>
 #include <linux/kexec.h>
 #include <linux/slab.h>
+#include <linux/panic_notifier.h>
 
 #include <xen/xen.h>
 #include <xen/features.h>
diff --git a/arch/xtensa/platforms/iss/setup.c b/arch/xtensa/platforms/iss/setup.c
index ed519aee0ec8..d3433e1bb94e 100644
--- a/arch/xtensa/platforms/iss/setup.c
+++ b/arch/xtensa/platforms/iss/setup.c
@@ -14,6 +14,7 @@
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/notifier.h>
+#include <linux/panic_notifier.h>
 #include <linux/printk.h>
 #include <linux/string.h>
 
diff --git a/drivers/bus/brcmstb_gisb.c b/drivers/bus/brcmstb_gisb.c
index 7355fa2cb439..6551286a60cc 100644
--- a/drivers/bus/brcmstb_gisb.c
+++ b/drivers/bus/brcmstb_gisb.c
@@ -6,6 +6,7 @@
 #include <linux/init.h>
 #include <linux/types.h>
 #include <linux/module.h>
+#include <linux/panic_notifier.h>
 #include <linux/platform_device.h>
 #include <linux/interrupt.h>
 #include <linux/sysfs.h>
diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index 8a0e97b33cae..e96cb5c4f97a 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -16,6 +16,7 @@
 
 #include <linux/module.h>
 #include <linux/errno.h>
+#include <linux/panic_notifier.h>
 #include <linux/poll.h>
 #include <linux/sched.h>
 #include <linux/seq_file.h>
diff --git a/drivers/clk/analogbits/wrpll-cln28hpc.c b/drivers/clk/analogbits/wrpll-cln28hpc.c
index 776ead319ae9..7c64ea52a8d5 100644
--- a/drivers/clk/analogbits/wrpll-cln28hpc.c
+++ b/drivers/clk/analogbits/wrpll-cln28hpc.c
@@ -23,8 +23,12 @@
 
 #include <linux/bug.h>
 #include <linux/err.h>
+#include <linux/limits.h>
 #include <linux/log2.h>
 #include <linux/math64.h>
+#include <linux/math.h>
+#include <linux/minmax.h>
+
 #include <linux/clk/analogbits-wrpll-cln28hpc.h>
 
 /* MIN_INPUT_FREQ: minimum input clock frequency, in Hz (Fref_min) */
diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index 5f7fd79ec82f..61c21bd880a4 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -20,6 +20,7 @@
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
+#include <linux/panic_notifier.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/types.h>
diff --git a/drivers/firmware/google/gsmi.c b/drivers/firmware/google/gsmi.c
index bb6e77ee3898..adaa492c3d2d 100644
--- a/drivers/firmware/google/gsmi.c
+++ b/drivers/firmware/google/gsmi.c
@@ -19,6 +19,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/fs.h>
 #include <linux/slab.h>
+#include <linux/panic_notifier.h>
 #include <linux/ioctl.h>
 #include <linux/acpi.h>
 #include <linux/io.h>
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 004c3390c15f..7107b9b6eff4 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -25,6 +25,7 @@
 
 #include <linux/delay.h>
 #include <linux/notifier.h>
+#include <linux/panic_notifier.h>
 #include <linux/ptrace.h>
 #include <linux/screen_info.h>
 #include <linux/kdebug.h>
diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c
index 2dcf13de751f..9731d3a96073 100644
--- a/drivers/hwtracing/coresight/coresight-cpu-debug.c
+++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
@@ -17,6 +17,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/panic_notifier.h>
 #include <linux/pm_qos.h>
 #include <linux/slab.h>
 #include <linux/smp.h>
diff --git a/drivers/leds/trigger/ledtrig-activity.c b/drivers/leds/trigger/ledtrig-activity.c
index 14ba7faaed9e..30bc9df03636 100644
--- a/drivers/leds/trigger/ledtrig-activity.c
+++ b/drivers/leds/trigger/ledtrig-activity.c
@@ -11,6 +11,7 @@
 #include <linux/kernel_stat.h>
 #include <linux/leds.h>
 #include <linux/module.h>
+#include <linux/panic_notifier.h>
 #include <linux/reboot.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
diff --git a/drivers/leds/trigger/ledtrig-heartbeat.c b/drivers/leds/trigger/ledtrig-heartbeat.c
index 36b6709afe9f..7fe0a05574d2 100644
--- a/drivers/leds/trigger/ledtrig-heartbeat.c
+++ b/drivers/leds/trigger/ledtrig-heartbeat.c
@@ -11,6 +11,7 @@
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
+#include <linux/panic_notifier.h>
 #include <linux/slab.h>
 #include <linux/timer.h>
 #include <linux/sched.h>
diff --git a/drivers/leds/trigger/ledtrig-panic.c b/drivers/leds/trigger/ledtrig-panic.c
index 5751cd032f9d..64abf2e91608 100644
--- a/drivers/leds/trigger/ledtrig-panic.c
+++ b/drivers/leds/trigger/ledtrig-panic.c
@@ -8,6 +8,7 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/notifier.h>
+#include <linux/panic_notifier.h>
 #include <linux/leds.h>
 #include "../leds.h"
 
diff --git a/drivers/misc/bcm-vk/bcm_vk_dev.c b/drivers/misc/bcm-vk/bcm_vk_dev.c
index 6bfea3210389..ad639ee85b2a 100644
--- a/drivers/misc/bcm-vk/bcm_vk_dev.c
+++ b/drivers/misc/bcm-vk/bcm_vk_dev.c
@@ -9,6 +9,7 @@
 #include <linux/fs.h>
 #include <linux/idr.h>
 #include <linux/interrupt.h>
+#include <linux/panic_notifier.h>
 #include <linux/kref.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
diff --git a/drivers/misc/ibmasm/heartbeat.c b/drivers/misc/ibmasm/heartbeat.c
index 4f5f3bdc814d..59c9a0d95659 100644
--- a/drivers/misc/ibmasm/heartbeat.c
+++ b/drivers/misc/ibmasm/heartbeat.c
@@ -9,6 +9,7 @@
  */
 
 #include <linux/notifier.h>
+#include <linux/panic_notifier.h>
 #include "ibmasm.h"
 #include "dot_command.h"
 #include "lowlevel.h"
diff --git a/drivers/misc/pvpanic/pvpanic.c b/drivers/misc/pvpanic/pvpanic.c
index 65f70a4da8c0..793ea0c01193 100644
--- a/drivers/misc/pvpanic/pvpanic.c
+++ b/drivers/misc/pvpanic/pvpanic.c
@@ -13,6 +13,7 @@
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/panic_notifier.h>
 #include <linux/types.h>
 #include <linux/cdev.h>
 #include <linux/list.h>
diff --git a/drivers/net/ipa/ipa_smp2p.c b/drivers/net/ipa/ipa_smp2p.c
index a5f7a79a1923..34b68dc43886 100644
--- a/drivers/net/ipa/ipa_smp2p.c
+++ b/drivers/net/ipa/ipa_smp2p.c
@@ -8,6 +8,7 @@
 #include <linux/device.h>
 #include <linux/interrupt.h>
 #include <linux/notifier.h>
+#include <linux/panic_notifier.h>
 #include <linux/soc/qcom/smem.h>
 #include <linux/soc/qcom/smem_state.h>
 
diff --git a/drivers/parisc/power.c b/drivers/parisc/power.c
index ebaf6867b457..456776bd8ee6 100644
--- a/drivers/parisc/power.c
+++ b/drivers/parisc/power.c
@@ -38,6 +38,7 @@
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/notifier.h>
+#include <linux/panic_notifier.h>
 #include <linux/reboot.h>
 #include <linux/sched/signal.h>
 #include <linux/kthread.h>
diff --git a/drivers/power/reset/ltc2952-poweroff.c b/drivers/power/reset/ltc2952-poweroff.c
index d1495af30081..8688c8ba8894 100644
--- a/drivers/power/reset/ltc2952-poweroff.c
+++ b/drivers/power/reset/ltc2952-poweroff.c
@@ -52,6 +52,7 @@
 #include <linux/slab.h>
 #include <linux/kmod.h>
 #include <linux/module.h>
+#include <linux/panic_notifier.h>
 #include <linux/mod_devicetable.h>
 #include <linux/gpio/consumer.h>
 #include <linux/reboot.h>
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 626a6b90fba2..76dd8e2b1e7e 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -20,6 +20,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/device.h>
+#include <linux/panic_notifier.h>
 #include <linux/slab.h>
 #include <linux/mutex.h>
 #include <linux/dma-map-ops.h>
diff --git a/drivers/s390/char/con3215.c b/drivers/s390/char/con3215.c
index 1fd5bca9fa20..02523f4e29f4 100644
--- a/drivers/s390/char/con3215.c
+++ b/drivers/s390/char/con3215.c
@@ -19,6 +19,7 @@
 #include <linux/console.h>
 #include <linux/interrupt.h>
 #include <linux/err.h>
+#include <linux/panic_notifier.h>
 #include <linux/reboot.h>
 #include <linux/serial.h> /* ASYNC_* flags */
 #include <linux/slab.h>
diff --git a/drivers/s390/char/con3270.c b/drivers/s390/char/con3270.c
index e21962c0fd94..87cdbace1453 100644
--- a/drivers/s390/char/con3270.c
+++ b/drivers/s390/char/con3270.c
@@ -13,6 +13,7 @@
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/list.h>
+#include <linux/panic_notifier.h>
 #include <linux/types.h>
 #include <linux/slab.h>
 #include <linux/err.h>
diff --git a/drivers/s390/char/sclp.c b/drivers/s390/char/sclp.c
index 986bbbc23d0a..6627820a5eb9 100644
--- a/drivers/s390/char/sclp.c
+++ b/drivers/s390/char/sclp.c
@@ -11,6 +11,7 @@
 #include <linux/kernel_stat.h>
 #include <linux/module.h>
 #include <linux/err.h>
+#include <linux/panic_notifier.h>
 #include <linux/spinlock.h>
 #include <linux/interrupt.h>
 #include <linux/timer.h>
diff --git a/drivers/s390/char/sclp_con.c b/drivers/s390/char/sclp_con.c
index 9b852a47ccc1..cc01a7b8595d 100644
--- a/drivers/s390/char/sclp_con.c
+++ b/drivers/s390/char/sclp_con.c
@@ -10,6 +10,7 @@
 #include <linux/kmod.h>
 #include <linux/console.h>
 #include <linux/init.h>
+#include <linux/panic_notifier.h>
 #include <linux/timer.h>
 #include <linux/jiffies.h>
 #include <linux/termios.h>
diff --git a/drivers/s390/char/sclp_vt220.c b/drivers/s390/char/sclp_vt220.c
index 7f4445b0f819..5b8a7b090a97 100644
--- a/drivers/s390/char/sclp_vt220.c
+++ b/drivers/s390/char/sclp_vt220.c
@@ -9,6 +9,7 @@
 
 #include <linux/module.h>
 #include <linux/spinlock.h>
+#include <linux/panic_notifier.h>
 #include <linux/list.h>
 #include <linux/wait.h>
 #include <linux/timer.h>
diff --git a/drivers/s390/char/zcore.c b/drivers/s390/char/zcore.c
index bd3c724bf695..b5b0848da93b 100644
--- a/drivers/s390/char/zcore.c
+++ b/drivers/s390/char/zcore.c
@@ -15,6 +15,7 @@
 #include <linux/init.h>
 #include <linux/slab.h>
 #include <linux/debugfs.h>
+#include <linux/panic_notifier.h>
 #include <linux/reboot.h>
 
 #include <asm/asm-offsets.h>
diff --git a/drivers/soc/bcm/brcmstb/pm/pm-arm.c b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
index a673fdffe216..3cbb165d6e30 100644
--- a/drivers/soc/bcm/brcmstb/pm/pm-arm.c
+++ b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
@@ -28,6 +28,7 @@
 #include <linux/notifier.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/panic_notifier.h>
 #include <linux/platform_device.h>
 #include <linux/pm.h>
 #include <linux/printk.h>
diff --git a/drivers/staging/olpc_dcon/olpc_dcon.c b/drivers/staging/olpc_dcon/olpc_dcon.c
index 6d8e9a481786..7284cb4ac395 100644
--- a/drivers/staging/olpc_dcon/olpc_dcon.c
+++ b/drivers/staging/olpc_dcon/olpc_dcon.c
@@ -22,6 +22,7 @@
 #include <linux/device.h>
 #include <linux/uaccess.h>
 #include <linux/ctype.h>
+#include <linux/panic_notifier.h>
 #include <linux/reboot.h>
 #include <linux/olpc-ec.h>
 #include <asm/tsc.h>
diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
index 4dc9077dd2ac..ac06ee7c87d7 100644
--- a/drivers/video/fbdev/hyperv_fb.c
+++ b/drivers/video/fbdev/hyperv_fb.c
@@ -52,6 +52,7 @@
 #include <linux/completion.h>
 #include <linux/fb.h>
 #include <linux/pci.h>
+#include <linux/panic_notifier.h>
 #include <linux/efi.h>
 #include <linux/console.h>
 
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 76a10e0dca9f..719410b93f99 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -17,7 +17,8 @@
 #endif
 
 #ifndef __ASSEMBLY__
-#include <linux/kernel.h>
+#include <linux/panic.h>
+#include <linux/printk.h>
 
 #ifdef CONFIG_BUG
 
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 09035ac67d4b..6c5a05ac1ecb 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -14,6 +14,7 @@
 #include <linux/math.h>
 #include <linux/minmax.h>
 #include <linux/typecheck.h>
+#include <linux/panic.h>
 #include <linux/printk.h>
 #include <linux/build_bug.h>
 #include <linux/static_call_types.h>
@@ -70,7 +71,6 @@
 #define lower_32_bits(n) ((u32)((n) & 0xffffffff))
 
 struct completion;
-struct pt_regs;
 struct user;
 
 #ifdef CONFIG_PREEMPT_VOLUNTARY
@@ -175,14 +175,6 @@ void __might_fault(const char *file, int line);
 static inline void might_fault(void) { }
 #endif
 
-extern struct atomic_notifier_head panic_notifier_list;
-extern long (*panic_blink)(int state);
-__printf(1, 2)
-void panic(const char *fmt, ...) __noreturn __cold;
-void nmi_panic(struct pt_regs *regs, const char *msg);
-extern void oops_enter(void);
-extern void oops_exit(void);
-extern bool oops_may_print(void);
 void do_exit(long error_code) __noreturn;
 void complete_and_exit(struct completion *, long) __noreturn;
 
@@ -368,52 +360,8 @@ extern int __kernel_text_address(unsigned long addr);
 extern int kernel_text_address(unsigned long addr);
 extern int func_ptr_is_kernel_text(void *ptr);
 
-#ifdef CONFIG_SMP
-extern unsigned int sysctl_oops_all_cpu_backtrace;
-#else
-#define sysctl_oops_all_cpu_backtrace 0
-#endif /* CONFIG_SMP */
-
 extern void bust_spinlocks(int yes);
-extern int panic_timeout;
-extern unsigned long panic_print;
-extern int panic_on_oops;
-extern int panic_on_unrecovered_nmi;
-extern int panic_on_io_nmi;
-extern int panic_on_warn;
-extern unsigned long panic_on_taint;
-extern bool panic_on_taint_nousertaint;
-extern int sysctl_panic_on_rcu_stall;
-extern int sysctl_max_rcu_stall_to_panic;
-extern int sysctl_panic_on_stackoverflow;
-
-extern bool crash_kexec_post_notifiers;
 
-/*
- * panic_cpu is used for synchronizing panic() and crash_kexec() execution. It
- * holds a CPU number which is executing panic() currently. A value of
- * PANIC_CPU_INVALID means no CPU has entered panic() or crash_kexec().
- */
-extern atomic_t panic_cpu;
-#define PANIC_CPU_INVALID	-1
-
-/*
- * Only to be used by arch init code. If the user over-wrote the default
- * CONFIG_PANIC_TIMEOUT, honor it.
- */
-static inline void set_arch_panic_timeout(int timeout, int arch_default_timeout)
-{
-	if (panic_timeout == arch_default_timeout)
-		panic_timeout = timeout;
-}
-extern const char *print_tainted(void);
-enum lockdep_ok {
-	LOCKDEP_STILL_OK,
-	LOCKDEP_NOW_UNRELIABLE
-};
-extern void add_taint(unsigned flag, enum lockdep_ok);
-extern int test_taint(unsigned flag);
-extern unsigned long get_taint(void);
 extern int root_mountflags;
 
 extern bool early_boot_irqs_disabled;
@@ -432,36 +380,6 @@ extern enum system_states {
 	SYSTEM_SUSPEND,
 } system_state;
 
-/* This cannot be an enum because some may be used in assembly source. */
-#define TAINT_PROPRIETARY_MODULE	0
-#define TAINT_FORCED_MODULE		1
-#define TAINT_CPU_OUT_OF_SPEC		2
-#define TAINT_FORCED_RMMOD		3
-#define TAINT_MACHINE_CHECK		4
-#define TAINT_BAD_PAGE			5
-#define TAINT_USER			6
-#define TAINT_DIE			7
-#define TAINT_OVERRIDDEN_ACPI_TABLE	8
-#define TAINT_WARN			9
-#define TAINT_CRAP			10
-#define TAINT_FIRMWARE_WORKAROUND	11
-#define TAINT_OOT_MODULE		12
-#define TAINT_UNSIGNED_MODULE		13
-#define TAINT_SOFTLOCKUP		14
-#define TAINT_LIVEPATCH			15
-#define TAINT_AUX			16
-#define TAINT_RANDSTRUCT		17
-#define TAINT_FLAGS_COUNT		18
-#define TAINT_FLAGS_MAX			((1UL << TAINT_FLAGS_COUNT) - 1)
-
-struct taint_flag {
-	char c_true;	/* character printed when tainted */
-	char c_false;	/* character printed when not tainted */
-	bool module;	/* also show as a per-module taint flag */
-};
-
-extern const struct taint_flag taint_flags[TAINT_FLAGS_COUNT];
-
 extern const char hex_asc[];
 #define hex_asc_lo(x)	hex_asc[((x) & 0x0f)]
 #define hex_asc_hi(x)	hex_asc[((x) & 0xf0) >> 4]
diff --git a/include/linux/panic.h b/include/linux/panic.h
new file mode 100644
index 000000000000..f5844908a089
--- /dev/null
+++ b/include/linux/panic.h
@@ -0,0 +1,98 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_PANIC_H
+#define _LINUX_PANIC_H
+
+#include <linux/compiler_attributes.h>
+#include <linux/types.h>
+
+struct pt_regs;
+
+extern long (*panic_blink)(int state);
+__printf(1, 2)
+void panic(const char *fmt, ...) __noreturn __cold;
+void nmi_panic(struct pt_regs *regs, const char *msg);
+extern void oops_enter(void);
+extern void oops_exit(void);
+extern bool oops_may_print(void);
+
+#ifdef CONFIG_SMP
+extern unsigned int sysctl_oops_all_cpu_backtrace;
+#else
+#define sysctl_oops_all_cpu_backtrace 0
+#endif /* CONFIG_SMP */
+
+extern int panic_timeout;
+extern unsigned long panic_print;
+extern int panic_on_oops;
+extern int panic_on_unrecovered_nmi;
+extern int panic_on_io_nmi;
+extern int panic_on_warn;
+
+extern unsigned long panic_on_taint;
+extern bool panic_on_taint_nousertaint;
+
+extern int sysctl_panic_on_rcu_stall;
+extern int sysctl_max_rcu_stall_to_panic;
+extern int sysctl_panic_on_stackoverflow;
+
+extern bool crash_kexec_post_notifiers;
+
+/*
+ * panic_cpu is used for synchronizing panic() and crash_kexec() execution. It
+ * holds a CPU number which is executing panic() currently. A value of
+ * PANIC_CPU_INVALID means no CPU has entered panic() or crash_kexec().
+ */
+extern atomic_t panic_cpu;
+#define PANIC_CPU_INVALID	-1
+
+/*
+ * Only to be used by arch init code. If the user over-wrote the default
+ * CONFIG_PANIC_TIMEOUT, honor it.
+ */
+static inline void set_arch_panic_timeout(int timeout, int arch_default_timeout)
+{
+	if (panic_timeout == arch_default_timeout)
+		panic_timeout = timeout;
+}
+
+/* This cannot be an enum because some may be used in assembly source. */
+#define TAINT_PROPRIETARY_MODULE	0
+#define TAINT_FORCED_MODULE		1
+#define TAINT_CPU_OUT_OF_SPEC		2
+#define TAINT_FORCED_RMMOD		3
+#define TAINT_MACHINE_CHECK		4
+#define TAINT_BAD_PAGE			5
+#define TAINT_USER			6
+#define TAINT_DIE			7
+#define TAINT_OVERRIDDEN_ACPI_TABLE	8
+#define TAINT_WARN			9
+#define TAINT_CRAP			10
+#define TAINT_FIRMWARE_WORKAROUND	11
+#define TAINT_OOT_MODULE		12
+#define TAINT_UNSIGNED_MODULE		13
+#define TAINT_SOFTLOCKUP		14
+#define TAINT_LIVEPATCH			15
+#define TAINT_AUX			16
+#define TAINT_RANDSTRUCT		17
+#define TAINT_FLAGS_COUNT		18
+#define TAINT_FLAGS_MAX			((1UL << TAINT_FLAGS_COUNT) - 1)
+
+struct taint_flag {
+	char c_true;	/* character printed when tainted */
+	char c_false;	/* character printed when not tainted */
+	bool module;	/* also show as a per-module taint flag */
+};
+
+extern const struct taint_flag taint_flags[TAINT_FLAGS_COUNT];
+
+enum lockdep_ok {
+	LOCKDEP_STILL_OK,
+	LOCKDEP_NOW_UNRELIABLE,
+};
+
+extern const char *print_tainted(void);
+extern void add_taint(unsigned flag, enum lockdep_ok);
+extern int test_taint(unsigned flag);
+extern unsigned long get_taint(void);
+
+#endif	/* _LINUX_PANIC_H */
diff --git a/include/linux/panic_notifier.h b/include/linux/panic_notifier.h
new file mode 100644
index 000000000000..41e32483d7a7
--- /dev/null
+++ b/include/linux/panic_notifier.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_PANIC_NOTIFIERS_H
+#define _LINUX_PANIC_NOTIFIERS_H
+
+#include <linux/notifier.h>
+#include <linux/types.h>
+
+extern struct atomic_notifier_head panic_notifier_list;
+
+extern bool crash_kexec_post_notifiers;
+
+#endif	/* _LINUX_PANIC_NOTIFIERS_H */
diff --git a/kernel/hung_task.c b/kernel/hung_task.c
index bb2e3e15c84c..2871076e4d29 100644
--- a/kernel/hung_task.c
+++ b/kernel/hung_task.c
@@ -15,6 +15,7 @@
 #include <linux/kthread.h>
 #include <linux/lockdep.h>
 #include <linux/export.h>
+#include <linux/panic_notifier.h>
 #include <linux/sysctl.h>
 #include <linux/suspend.h>
 #include <linux/utsname.h>
diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index f099baee3578..4b34a9aa32bc 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -26,6 +26,7 @@
 #include <linux/suspend.h>
 #include <linux/device.h>
 #include <linux/freezer.h>
+#include <linux/panic_notifier.h>
 #include <linux/pm.h>
 #include <linux/cpu.h>
 #include <linux/uaccess.h>
diff --git a/kernel/panic.c b/kernel/panic.c
index 332736a72a58..edad89660a2b 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -23,6 +23,7 @@
 #include <linux/reboot.h>
 #include <linux/delay.h>
 #include <linux/kexec.h>
+#include <linux/panic_notifier.h>
 #include <linux/sched.h>
 #include <linux/sysrq.h>
 #include <linux/init.h>
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 03282196953a..67f8f65a9ada 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -32,6 +32,8 @@
 #include <linux/export.h>
 #include <linux/completion.h>
 #include <linux/moduleparam.h>
+#include <linux/panic.h>
+#include <linux/panic_notifier.h>
 #include <linux/percpu.h>
 #include <linux/notifier.h>
 #include <linux/cpu.h>
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 3601786ddaeb..e5cf9c4ef5e1 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -27,6 +27,7 @@
 #include <linux/sysctl.h>
 #include <linux/bitmap.h>
 #include <linux/signal.h>
+#include <linux/panic.h>
 #include <linux/printk.h>
 #include <linux/proc_fs.h>
 #include <linux/security.h>
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 507a30bf26e4..9612a1d8fa13 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -39,6 +39,7 @@
 #include <linux/slab.h>
 #include <linux/ctype.h>
 #include <linux/init.h>
+#include <linux/panic_notifier.h>
 #include <linux/poll.h>
 #include <linux/nmi.h>
 #include <linux/fs.h>
-- 
2.30.2


^ permalink raw reply related

* Re: [PATCH v2 1/1] kernel.h: Split out panic and oops helpers
From: Sebastian Reichel @ 2021-04-09 11:49 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Corey Minyard, dri-devel, Paul Mackerras, Pavel Machek,
	K. Y. Srinivasan, linux-clk, linux-arch, Wei Liu,
	Mauro Carvalho Chehab, Jens Frederich, Catalin Marinas, xen-devel,
	Matt Turner, Ohad Ben-Cohen, linux-pm, Lai Jiangshan, linux-um,
	Daniel Drake, Mihai Carabas, Thomas Gleixner, Richard Henderson,
	Alex Elder, Greg Kroah-Hartman, linux-kernel, Luis Chamberlain,
	Joe Perches, Andrew Morton, Marek Czerski, Tetsuo Handa,
	Jon Nettleton, Alexander Egorenkov, James E.J. Bottomley,
	Max Filippov, Christian Brauner, linux-s390, Stefano Stabellini,
	Stephen Hemminger, Corey Minyard, Krzysztof Kozlowski,
	Helge Deller, Suzuki K Poulose, Mathieu Desnoyers, Iurii Zaikin,
	linux-xtensa, Joerg Roedel, Vasily Gorbik, Scott Branden,
	coresight, linux-fsdevel, Boris Ostrovsky, linux-arm-kernel,
	Chris Zankel, Tony Luck, Mathieu Poirier, Stephen Boyd,
	Peter Oberparleiter, Dinh Nguyen, James Morse, Eric Biederman,
	Alexander Shishkin, Michael Turquette, Rasmus Villemoes,
	linux-hyperv, Vineeth Vijayan, Joel Fernandes, Will Deacon,
	Florian Fainelli, linux-staging, Christian Borntraeger,
	bcm-kernel-feedback-list, Juergen Gross, Robert Richter,
	Kees Cook, Arnd Bergmann, Haiyang Zhang, Josh Triplett,
	Steven Rostedt (VMware), rcu, Borislav Petkov, linux-fbdev,
	openipmi-developer, Michael Kelley, Thomas Bogendoerfer,
	linux-parisc, Hongbo Yao, linux-alpha, Olof Johansson,
	David S. Miller, Mike Rapoport, Alexander Lobakin,
	linux-remoteproc, Bjorn Andersson, H. Peter Anvin, sparclinux,
	linux-leds, Anton Ivanov, Richard Weinberger, x86, Mike Rapoport,
	Ingo Molnar, Jakub Kicinski, Wang Wenhu, Jeff Dike, Mike Leach,
	Paul E. McKenney, Heiko Carstens, Paul Walmsley, Vlastimil Babka,
	linux-edac, Jason J. Herne, netdev, kexec, linux-mips, Leo Yan,
	Ivan Kokshaysky, linuxppc-dev
In-Reply-To: <20210409100250.25922-1-andriy.shevchenko@linux.intel.com>

[-- Attachment #1: Type: text/plain, Size: 1763 bytes --]

Hi,

On Fri, Apr 09, 2021 at 01:02:50PM +0300, Andy Shevchenko wrote:
> kernel.h is being used as a dump for all kinds of stuff for a long time.
> Here is the attempt to start cleaning it up by splitting out panic and
> oops helpers.
> 
> There are several purposes of doing this:
> - dropping dependency in bug.h
> - dropping a loop by moving out panic_notifier.h
> - unload kernel.h from something which has its own domain
> 
> At the same time convert users tree-wide to use new headers, although
> for the time being include new header back to kernel.h to avoid twisted
> indirected includes for existing users.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Acked-by: Mike Rapoport <rppt@linux.ibm.com>
> Acked-by: Corey Minyard <cminyard@mvista.com>
> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> Acked-by: Kees Cook <keescook@chromium.org>
> Acked-by: Wei Liu <wei.liu@kernel.org>
> Acked-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> v2:
>  - fixed all errors with allmodconfig on x86_64 (Andrew)
>  - checked with allyesconfig on x86_64
>  - additionally grepped source code for panic notifier list usage
>    and converted all users
>  - elaborated commit message (Luis)
>  - collected given tags (incl. Andrew's SoB, see below)
> 
> I added Andrew's SoB since part of the fixes I took from him. Andrew,
> feel free to amend or tell me how you want me to do.
> 
> [...]
>  drivers/power/reset/ltc2952-poweroff.c        |  1 +
> [...]

Acked-by: Sebastian Reichel <sre@kernel.org>

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply

* [PATCH v4] powerpc/traps: Enhance readability for trap types
From: Xiongwei Song @ 2021-04-09 14:28 UTC (permalink / raw)
  To: mpe, benh, paulus, oleg, npiggin, christophe.leroy, aneesh.kumar,
	ravi.bangoria, mikey, haren, akpm, rppt, jniethe5, atrajeev,
	maddy, peterz, kjain, kan.liang, aik, alistair, pmladek,
	john.ogness
  Cc: Xiongwei Song, linuxppc-dev, linux-kernel

From: Xiongwei Song <sxwjean@gmail.com>

Create a new header named traps.h, define macros to list ppc interrupt
types in traps.h, replace the references of the trap hex values with these
macros.

Referred the hex numbers in arch/powerpc/kernel/exceptions-64e.S,
arch/powerpc/kernel/exceptions-64s.S and
arch/powerpc/include/asm/kvm_asm.h.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
---

v3-v4:
Fix compile issue:
arch/powerpc/kernel/process.c:1473:14: error: 'INTERRUPT_MACHINE_CHECK' undeclared (first use in this function); did you mean 'TAINT_MACHINE_CHECK'?
I didn't add "Reported-by: kernel test robot <lkp@intel.com>" here,
because it's improper for this patch.

v2-v3:
Correct the prefix of trap macros with INTERRUPT_, the previous prefix
is TRAP_, which is not precise. This is suggested by Segher Boessenkool
and Nicholas Piggin.

v1-v2:
Define more trap macros to replace more trap hexs in code, not just for
the __show_regs function. This is suggested by Christophe Leroy.

---
 arch/powerpc/include/asm/interrupt.h  |  9 +++++---
 arch/powerpc/include/asm/ptrace.h     |  3 ++-
 arch/powerpc/include/asm/traps.h      | 32 +++++++++++++++++++++++++++
 arch/powerpc/kernel/interrupt.c       |  3 ++-
 arch/powerpc/kernel/process.c         |  5 ++++-
 arch/powerpc/mm/book3s64/hash_utils.c |  5 +++--
 arch/powerpc/mm/fault.c               | 21 +++++++++++-------
 arch/powerpc/perf/core-book3s.c       |  5 +++--
 arch/powerpc/xmon/xmon.c              | 16 +++++++++++---
 9 files changed, 78 insertions(+), 21 deletions(-)
 create mode 100644 arch/powerpc/include/asm/traps.h

diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
index 05e7fc4ffb50..4fd904fb5d59 100644
--- a/arch/powerpc/include/asm/interrupt.h
+++ b/arch/powerpc/include/asm/interrupt.h
@@ -8,6 +8,7 @@
 #include <asm/ftrace.h>
 #include <asm/kprobes.h>
 #include <asm/runlatch.h>
+#include <asm/traps.h>
 
 static inline void nap_adjust_return(struct pt_regs *regs)
 {
@@ -70,7 +71,7 @@ static inline void interrupt_enter_prepare(struct pt_regs *regs, struct interrup
 		 * CT_WARN_ON comes here via program_check_exception,
 		 * so avoid recursion.
 		 */
-		if (TRAP(regs) != 0x700)
+		if (TRAP(regs) != INTERRUPT_PROGRAM)
 			CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
 	}
 #endif
@@ -175,7 +176,8 @@ static inline void interrupt_nmi_enter_prepare(struct pt_regs *regs, struct inte
 	/* Don't do any per-CPU operations until interrupt state is fixed */
 #endif
 	/* Allow DEC and PMI to be traced when they are soft-NMI */
-	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260) {
+	if (TRAP(regs) != INTERRUPT_DECREMENTER &&
+	    TRAP(regs) != INTERRUPT_PERFMON) {
 		state->ftrace_enabled = this_cpu_get_ftrace_enabled();
 		this_cpu_set_ftrace_enabled(0);
 	}
@@ -204,7 +206,8 @@ static inline void interrupt_nmi_exit_prepare(struct pt_regs *regs, struct inter
 	 */
 
 #ifdef CONFIG_PPC64
-	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260)
+	if (TRAP(regs) != INTERRUPT_DECREMENTER &&
+	    TRAP(regs) != INTERRUPT_PERFMON)
 		this_cpu_set_ftrace_enabled(state->ftrace_enabled);
 
 #ifdef CONFIG_PPC_BOOK3S_64
diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
index 95600f3a6523..07ff8629e776 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -21,6 +21,7 @@
 
 #include <uapi/asm/ptrace.h>
 #include <asm/asm-const.h>
+#include <asm/traps.h>
 
 #ifndef __ASSEMBLY__
 struct pt_regs
@@ -237,7 +238,7 @@ static inline bool trap_is_unsupported_scv(struct pt_regs *regs)
 
 static inline bool trap_is_syscall(struct pt_regs *regs)
 {
-	return (trap_is_scv(regs) || TRAP(regs) == 0xc00);
+	return (trap_is_scv(regs) || TRAP(regs) == INTERRUPT_SYSCALL);
 }
 
 static inline bool trap_norestart(struct pt_regs *regs)
diff --git a/arch/powerpc/include/asm/traps.h b/arch/powerpc/include/asm/traps.h
new file mode 100644
index 000000000000..2e64e10afcef
--- /dev/null
+++ b/arch/powerpc/include/asm/traps.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_PPC_TRAPS_H
+#define _ASM_PPC_TRAPS_H
+
+#if defined(CONFIG_BOOKE) || defined(CONFIG_4xx)
+#define INTERRUPT_MACHINE_CHECK   0x000
+#define INTERRUPT_CRITICAL_INPUT  0x100
+#define INTERRUPT_ALTIVEC_UNAVAIL 0x200
+#define INTERRUPT_PERFMON         0x260
+#define INTERRUPT_DOORBELL        0x280
+#define INTERRUPT_DEBUG           0xd00
+#else
+#define INTERRUPT_SYSTEM_RESET    0x100
+#define INTERRUPT_MACHINE_CHECK   0x200
+#define INTERRUPT_DATA_SEGMENT    0x380
+#define INTERRUPT_INST_SEGMENT    0x480
+#define INTERRUPT_DOORBELL        0xa00
+#define INTERRUPT_TRACE           0xd00
+#define INTERRUPT_H_DATA_STORAGE  0xe00
+#define INTERRUPT_PERFMON         0xf00
+#define INTERRUPT_H_FAC_UNAVAIL   0xf80
+#endif
+
+#define INTERRUPT_DATA_STORAGE    0x300
+#define INTERRUPT_INST_STORAGE    0x400
+#define INTERRUPT_ALIGNMENT       0x600
+#define INTERRUPT_PROGRAM         0x700
+#define INTERRUPT_FP_UNAVAIL      0x800
+#define INTERRUPT_DECREMENTER     0x900
+#define INTERRUPT_SYSCALL         0xc00
+
+#endif /* _ASM_PPC_TRAPS_H */
diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
index c4dd4b8f9cfa..72689f7ca7c8 100644
--- a/arch/powerpc/kernel/interrupt.c
+++ b/arch/powerpc/kernel/interrupt.c
@@ -19,6 +19,7 @@
 #include <asm/syscall.h>
 #include <asm/time.h>
 #include <asm/unistd.h>
+#include <asm/traps.h>
 
 #if defined(CONFIG_PPC_ADV_DEBUG_REGS) && defined(CONFIG_PPC32)
 unsigned long global_dbcr0[NR_CPUS];
@@ -456,7 +457,7 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsign
 	 * CT_WARN_ON comes here via program_check_exception,
 	 * so avoid recursion.
 	 */
-	if (TRAP(regs) != 0x700)
+	if (TRAP(regs) != INTERRUPT_PROGRAM)
 		CT_WARN_ON(ct_state() == CONTEXT_USER);
 
 	kuap = kuap_get_and_assert_locked();
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index b966c8e0cead..92cd49427b2f 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -64,6 +64,7 @@
 #include <asm/asm-prototypes.h>
 #include <asm/stacktrace.h>
 #include <asm/hw_breakpoint.h>
+#include <asm/traps.h>
 
 #include <linux/kprobes.h>
 #include <linux/kdebug.h>
@@ -1469,7 +1470,9 @@ static void __show_regs(struct pt_regs *regs)
 	trap = TRAP(regs);
 	if (!trap_is_syscall(regs) && cpu_has_feature(CPU_FTR_CFAR))
 		pr_cont("CFAR: "REG" ", regs->orig_gpr3);
-	if (trap == 0x200 || trap == 0x300 || trap == 0x600) {
+	if (trap == INTERRUPT_MACHINE_CHECK ||
+	    trap == INTERRUPT_DATA_STORAGE ||
+	    trap == INTERRUPT_ALIGNMENT) {
 		if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
 			pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->dsisr);
 		else
diff --git a/arch/powerpc/mm/book3s64/hash_utils.c b/arch/powerpc/mm/book3s64/hash_utils.c
index 7719995323c3..2bf06e01b309 100644
--- a/arch/powerpc/mm/book3s64/hash_utils.c
+++ b/arch/powerpc/mm/book3s64/hash_utils.c
@@ -64,6 +64,7 @@
 #include <asm/pte-walk.h>
 #include <asm/asm-prototypes.h>
 #include <asm/ultravisor.h>
+#include <asm/traps.h>
 
 #include <mm/mmu_decl.h>
 
@@ -1145,7 +1146,7 @@ unsigned int hash_page_do_lazy_icache(unsigned int pp, pte_t pte, int trap)
 
 	/* page is dirty */
 	if (!test_bit(PG_dcache_clean, &page->flags) && !PageReserved(page)) {
-		if (trap == 0x400) {
+		if (trap == INTERRUPT_INST_STORAGE) {
 			flush_dcache_icache_page(page);
 			set_bit(PG_dcache_clean, &page->flags);
 		} else
@@ -1545,7 +1546,7 @@ DEFINE_INTERRUPT_HANDLER_RET(__do_hash_fault)
 	if (user_mode(regs) || (region_id == USER_REGION_ID))
 		access &= ~_PAGE_PRIVILEGED;
 
-	if (TRAP(regs) == 0x400)
+	if (TRAP(regs) == INTERRUPT_INST_STORAGE)
 		access |= _PAGE_EXEC;
 
 	err = hash_page_mm(mm, ea, access, TRAP(regs), flags);
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 0c0b1c2cfb49..1215fa2a72a7 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -44,6 +44,7 @@
 #include <asm/debug.h>
 #include <asm/kup.h>
 #include <asm/inst.h>
+#include <asm/traps.h>
 
 
 /*
@@ -197,7 +198,7 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr,
 static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
 			     unsigned long address, bool is_write)
 {
-	int is_exec = TRAP(regs) == 0x400;
+	int is_exec = TRAP(regs) == INTERRUPT_INST_STORAGE;
 
 	/* NX faults set DSISR_PROTFAULT on the 8xx, DSISR_NOEXEC_OR_G on others */
 	if (is_exec && (error_code & (DSISR_NOEXEC_OR_G | DSISR_KEYFAULT |
@@ -391,7 +392,7 @@ static int ___do_page_fault(struct pt_regs *regs, unsigned long address,
 	struct vm_area_struct * vma;
 	struct mm_struct *mm = current->mm;
 	unsigned int flags = FAULT_FLAG_DEFAULT;
- 	int is_exec = TRAP(regs) == 0x400;
+	int is_exec = TRAP(regs) == INTERRUPT_INST_STORAGE;
 	int is_user = user_mode(regs);
 	int is_write = page_fault_is_write(error_code);
 	vm_fault_t fault, major = 0;
@@ -588,20 +589,24 @@ void __bad_page_fault(struct pt_regs *regs, int sig)
 	/* kernel has accessed a bad area */
 
 	switch (TRAP(regs)) {
-	case 0x300:
-	case 0x380:
-	case 0xe00:
+	case INTERRUPT_DATA_STORAGE:
+#if !defined(CONFIG_BOOKE) && !defined(CONFIG_4xx)
+	case INTERRUPT_DATA_SEGMENT:
+	case INTERRUPT_H_DATA_STORAGE:
+#endif
 		pr_alert("BUG: %s on %s at 0x%08lx\n",
 			 regs->dar < PAGE_SIZE ? "Kernel NULL pointer dereference" :
 			 "Unable to handle kernel data access",
 			 is_write ? "write" : "read", regs->dar);
 		break;
-	case 0x400:
-	case 0x480:
+	case INTERRUPT_INST_STORAGE:
+#if !defined(CONFIG_BOOKE) && !defined(CONFIG_4xx)
+	case INTERRUPT_INST_SEGMENT:
+#endif
 		pr_alert("BUG: Unable to handle kernel instruction fetch%s",
 			 regs->nip < PAGE_SIZE ? " (NULL pointer?)\n" : "\n");
 		break;
-	case 0x600:
+	case INTERRUPT_ALIGNMENT:
 		pr_alert("BUG: Unable to handle kernel unaligned access at 0x%08lx\n",
 			 regs->dar);
 		break;
diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index 766f064f00fb..6e34f5bba232 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -17,6 +17,7 @@
 #include <asm/firmware.h>
 #include <asm/ptrace.h>
 #include <asm/code-patching.h>
+#include <asm/traps.h>
 
 #ifdef CONFIG_PPC64
 #include "internal.h"
@@ -168,7 +169,7 @@ static bool regs_use_siar(struct pt_regs *regs)
 	 * they have not been setup using perf_read_regs() and so regs->result
 	 * is something random.
 	 */
-	return ((TRAP(regs) == 0xf00) && regs->result);
+	return ((TRAP(regs) == INTERRUPT_PERFMON) && regs->result);
 }
 
 /*
@@ -347,7 +348,7 @@ static inline void perf_read_regs(struct pt_regs *regs)
 	 * hypervisor samples as well as samples in the kernel with
 	 * interrupts off hence the userspace check.
 	 */
-	if (TRAP(regs) != 0xf00)
+	if (TRAP(regs) != INTERRUPT_PERFMON)
 		use_siar = 0;
 	else if ((ppmu->flags & PPMU_NO_SIAR))
 		use_siar = 0;
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 3fe37495f63d..59136634c5ce 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -54,6 +54,7 @@
 #include <asm/code-patching.h>
 #include <asm/sections.h>
 #include <asm/inst.h>
+#include <asm/traps.h>
 
 #ifdef CONFIG_PPC64
 #include <asm/hvcall.h>
@@ -1769,7 +1770,12 @@ static void excprint(struct pt_regs *fp)
 	printf("    sp: %lx\n", fp->gpr[1]);
 	printf("   msr: %lx\n", fp->msr);
 
-	if (trap == 0x300 || trap == 0x380 || trap == 0x600 || trap == 0x200) {
+	if (trap == INTERRUPT_DATA_STORAGE ||
+#if !defined(CONFIG_BOOKE) && !defined(CONFIG_4xx)
+	    trap == INTERRUPT_DATA_SEGMENT ||
+#endif
+	    trap == INTERRUPT_ALIGNMENT ||
+	    trap == INTERRUPT_MACHINE_CHECK) {
 		printf("   dar: %lx\n", fp->dar);
 		if (trap != 0x380)
 			printf(" dsisr: %lx\n", fp->dsisr);
@@ -1785,7 +1791,7 @@ static void excprint(struct pt_regs *fp)
 		       current->pid, current->comm);
 	}
 
-	if (trap == 0x700)
+	if (trap == INTERRUPT_PROGRAM)
 		print_bug_trap(fp);
 
 	printf(linux_banner);
@@ -1846,7 +1852,11 @@ static void prregs(struct pt_regs *fp)
 	printf("ctr = "REG"   xer = "REG"   trap = %4lx\n",
 	       fp->ctr, fp->xer, fp->trap);
 	trap = TRAP(fp);
-	if (trap == 0x300 || trap == 0x380 || trap == 0x600)
+	if (trap == INTERRUPT_DATA_STORAGE ||
+#if !defined(CONFIG_BOOKE) && !defined(CONFIG_4xx)
+	    trap == INTERRUPT_DATA_SEGMENT ||
+#endif
+	    trap == INTERRUPT_ALIGNMENT)
 		printf("dar = "REG"   dsisr = %.8lx\n", fp->dar, fp->dsisr);
 }
 
-- 
2.17.1


^ permalink raw reply related

* [Bug 212631] New: Misaligned floating point loads and store occasionally fail
From: bugzilla-daemon @ 2021-04-09 15:34 UTC (permalink / raw)
  To: linuxppc-dev

https://bugzilla.kernel.org/show_bug.cgi?id=212631

            Bug ID: 212631
           Summary: Misaligned floating point loads and store occasionally
                    fail
           Product: Platform Specific/Hardware
           Version: 2.5
    Kernel Version: 5.4.110
          Hardware: All
                OS: Linux
              Tree: Mainline
            Status: NEW
          Severity: normal
          Priority: P1
         Component: PPC-32
          Assignee: platform_ppc-32@kernel-bugs.osdl.org
          Reporter: trevor_davenport@selinc.com
        Regression: No

Created attachment 296311
  --> https://bugzilla.kernel.org/attachment.cgi?id=296311&action=edit
Floating point load test program

On a 32-bit PPC, e300c1 CPU, I am occasionally seeing misaligned floating point
loads/store occasionally fail.  It appears to happen when a process is
preempted.  Reducing preemption cause the issue to be fare less likely to
occur. 

When it fails(in the load case), I've traced it down to do_fp_load.  It fails
when it takes the false branch while it works when taking the true branch (see
https://elixir.bootlin.com/linux/v5.4.110/source/arch/powerpc/lib/sstep.c#L492).

The issue was originally seen on a preempt-rt kernel, but it can be reproduced
on a regular kernel compiled for low latency desktop as well.

The issue can be reproduced with the attached program which just performs an
unaligned FP load.  Eventually it will produce a zero and exit.  

Example output:
 11326b98
 22453191
 3357f78a
 446abd71
 557d70b6
 11326b98
 22453191
 3357f78a
 446abd71
 557d70b6
 11326b98
 22453191
 3357f78a
 00000000
 float-bug: float-bug.c:14: main: Assertion `b[0] != 00' failed.

This has been compiled with gcc 9.3.0.

-- 
You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

^ permalink raw reply

* [Bug 212631] Misaligned floating point loads and store occasionally fail
From: bugzilla-daemon @ 2021-04-09 15:47 UTC (permalink / raw)
  To: linuxppc-dev
In-Reply-To: <bug-212631-206035@https.bugzilla.kernel.org/>

https://bugzilla.kernel.org/show_bug.cgi?id=212631

--- Comment #1 from Trevor Davenport (trevor_davenport@selinc.com) ---
I can also reproduce this with kernel version 5.11.12.

-- 
You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

^ permalink raw reply

* Re: [PATCH v3] powerpc/traps: Enhance readability for trap types
From: Christophe Leroy @ 2021-04-09 16:14 UTC (permalink / raw)
  To: Xiongwei Song, mpe, benh, paulus, oleg, npiggin, aneesh.kumar,
	ravi.bangoria, mikey, haren, akpm, rppt, jniethe5, atrajeev,
	maddy, peterz, kjain, kan.liang, aik, alistair, pmladek,
	john.ogness
  Cc: Xiongwei Song, linuxppc-dev, linux-kernel
In-Reply-To: <20210408140750.26832-1-sxwjean@me.com>



Le 08/04/2021 à 16:07, Xiongwei Song a écrit :
> From: Xiongwei Song <sxwjean@gmail.com>
> 
> Create a new header named traps.h, define macros to list ppc interrupt
> types in traps.h, replace the reference of the trap hex values with these
> macros.
> 
> Referred the hex number in arch/powerpc/kernel/exceptions-64e.S,
> arch/powerpc/kernel/exceptions-64s.S and
> arch/powerpc/include/asm/kvm_asm.h.
> 
> v2-v3:
> Correct the prefix of trap macros with INTERRUPT_, the previous prefix
> is TRAP_, which is not precise. This is suggested by Segher Boessenkool
> and Nicholas Piggin.
> 
> v1-v2:
> Define more trap macros to replace more trap hexs in code, not just for
> the __show_regs function. This is suggested by Christophe Leroy.
> 
> Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
> ---
>   arch/powerpc/include/asm/interrupt.h  |  9 +++++---
>   arch/powerpc/include/asm/ptrace.h     |  3 ++-
>   arch/powerpc/include/asm/traps.h      | 32 +++++++++++++++++++++++++++
>   arch/powerpc/kernel/interrupt.c       |  3 ++-
>   arch/powerpc/kernel/process.c         |  5 ++++-
>   arch/powerpc/mm/book3s64/hash_utils.c |  5 +++--
>   arch/powerpc/mm/fault.c               | 21 +++++++++++-------
>   arch/powerpc/perf/core-book3s.c       |  5 +++--
>   arch/powerpc/xmon/xmon.c              | 16 +++++++++++---
>   9 files changed, 78 insertions(+), 21 deletions(-)
>   create mode 100644 arch/powerpc/include/asm/traps.h
> 
> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
> index 7c633896d758..5ce9898bc9a6 100644
> --- a/arch/powerpc/include/asm/interrupt.h
> +++ b/arch/powerpc/include/asm/interrupt.h
> @@ -8,6 +8,7 @@
>   #include <asm/ftrace.h>
>   #include <asm/kprobes.h>
>   #include <asm/runlatch.h>
> +#include <asm/traps.h>
>   
>   struct interrupt_state {
>   #ifdef CONFIG_PPC_BOOK3E_64
> @@ -59,7 +60,7 @@ static inline void interrupt_enter_prepare(struct pt_regs *regs, struct interrup
>   		 * CT_WARN_ON comes here via program_check_exception,
>   		 * so avoid recursion.
>   		 */
> -		if (TRAP(regs) != 0x700)
> +		if (TRAP(regs) != INTERRUPT_PROGRAM)
>   			CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
>   	}
>   #endif
> @@ -156,7 +157,8 @@ static inline void interrupt_nmi_enter_prepare(struct pt_regs *regs, struct inte
>   	/* Don't do any per-CPU operations until interrupt state is fixed */
>   #endif
>   	/* Allow DEC and PMI to be traced when they are soft-NMI */
> -	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260) {
> +	if (TRAP(regs) != INTERRUPT_DECREMENTER &&
> +	    TRAP(regs) != INTERRUPT_PERFMON) {

I think too long names hinder readability, see later for suggestions.

>   		state->ftrace_enabled = this_cpu_get_ftrace_enabled();
>   		this_cpu_set_ftrace_enabled(0);
>   	}
> @@ -180,7 +182,8 @@ static inline void interrupt_nmi_exit_prepare(struct pt_regs *regs, struct inter
>   		nmi_exit();
>   
>   #ifdef CONFIG_PPC64
> -	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260)
> +	if (TRAP(regs) != INTERRUPT_DECREMENTER &&
> +	    TRAP(regs) != INTERRUPT_PERFMON)
>   		this_cpu_set_ftrace_enabled(state->ftrace_enabled);
>   
>   #ifdef CONFIG_PPC_BOOK3S_64
> diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
> index f10498e1b3f6..7a17e0365d43 100644
> --- a/arch/powerpc/include/asm/ptrace.h
> +++ b/arch/powerpc/include/asm/ptrace.h
> @@ -21,6 +21,7 @@
>   
>   #include <uapi/asm/ptrace.h>
>   #include <asm/asm-const.h>
> +#include <asm/traps.h>
>   
>   #ifndef __ASSEMBLY__
>   struct pt_regs
> @@ -237,7 +238,7 @@ static inline bool trap_is_unsupported_scv(struct pt_regs *regs)
>   
>   static inline bool trap_is_syscall(struct pt_regs *regs)
>   {
> -	return (trap_is_scv(regs) || TRAP(regs) == 0xc00);
> +	return (trap_is_scv(regs) || TRAP(regs) == INTERRUPT_SYSCALL);
>   }
>   
>   static inline bool trap_norestart(struct pt_regs *regs)
> diff --git a/arch/powerpc/include/asm/traps.h b/arch/powerpc/include/asm/traps.h
> new file mode 100644
> index 000000000000..cb416a17097c
> --- /dev/null
> +++ b/arch/powerpc/include/asm/traps.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_PPC_TRAPS_H
> +#define _ASM_PPC_TRAPS_H
> +
> +#if defined(CONFIG_BOOKE) || defined(CONFIG_4xx)
> +#define INTERRUPT_MACHINE_CHECK   0x000

I'd prefer shorted names in order to not be obliged to split lines.
Here are some suggestions:

INT_MCE

> +#define INTERRUPT_CRITICAL_INPUT  0x100

INT_CRIT

> +#define INTERRUPT_ALTIVEC_UNAVAIL 0x200
> +#define INTERRUPT_PERFMON         0x260

INT_PERF

> +#define INTERRUPT_DOORBELL        0x280
> +#define INTERRUPT_DEBUG           0xd00
> +#elif defined(CONFIG_PPC_BOOK3S)
> +#define INTERRUPT_SYSTEM_RESET    0x100

INT_SRESET

> +#define INTERRUPT_MACHINE_CHECK   0x200

INT_MCE

> +#define INTERRUPT_DATA_SEGMENT    0x380

INT_DSEG

> +#define INTERRUPT_INST_SEGMENT    0x480

INT_ISEG

> +#define INTERRUPT_DOORBELL        0xa00

INT_DBELL

> +#define INTERRUPT_TRACE           0xd00

INT_TRACE

> +#define INTERRUPT_H_DATA_STORAGE  0xe00
> +#define INTERRUPT_PERFMON         0xf00

INT_PERF

> +#define INTERRUPT_H_FAC_UNAVAIL   0xf80
> +#endif
> +
> +#define INTERRUPT_DATA_STORAGE    0x300

INT_DSI

> +#define INTERRUPT_INST_STORAGE    0x400

INT_ISI

> +#define INTERRUPT_ALIGNMENT       0x600

INT_ALIGN

> +#define INTERRUPT_PROGRAM         0x700

INT_PROG

> +#define INTERRUPT_FP_UNAVAIL      0x800

INT_FP_UNAVAIL

> +#define INTERRUPT_DECREMENTER     0x900

INT_DEC

> +#define INTERRUPT_SYSCALL         0xc00

INT_SYSCALL


> +
> +#endif /* _ASM_PPC_TRAPS_H */

...

> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 0c0b1c2cfb49..641b3feef7ee 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -44,6 +44,7 @@
>   #include <asm/debug.h>
>   #include <asm/kup.h>
>   #include <asm/inst.h>
> +#include <asm/traps.h>
>   
>   
>   /*
> @@ -197,7 +198,7 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr,
>   static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
>   			     unsigned long address, bool is_write)
>   {
> -	int is_exec = TRAP(regs) == 0x400;
> +	int is_exec = TRAP(regs) == INTERRUPT_INST_STORAGE;
>   
>   	/* NX faults set DSISR_PROTFAULT on the 8xx, DSISR_NOEXEC_OR_G on others */
>   	if (is_exec && (error_code & (DSISR_NOEXEC_OR_G | DSISR_KEYFAULT |
> @@ -391,7 +392,7 @@ static int ___do_page_fault(struct pt_regs *regs, unsigned long address,
>   	struct vm_area_struct * vma;
>   	struct mm_struct *mm = current->mm;
>   	unsigned int flags = FAULT_FLAG_DEFAULT;
> - 	int is_exec = TRAP(regs) == 0x400;
> +	int is_exec = TRAP(regs) == INTERRUPT_INST_STORAGE;
>   	int is_user = user_mode(regs);
>   	int is_write = page_fault_is_write(error_code);
>   	vm_fault_t fault, major = 0;
> @@ -588,20 +589,24 @@ void __bad_page_fault(struct pt_regs *regs, int sig)
>   	/* kernel has accessed a bad area */
>   
>   	switch (TRAP(regs)) {
> -	case 0x300:
> -	case 0x380:
> -	case 0xe00:
> +	case INTERRUPT_DATA_STORAGE:
> +#ifdef CONFIG_PPC_BOOK3S
> +	case INTERRUPT_DATA_SEGMENT:
> +	case INTERRUPT_H_DATA_STORAGE:
> +#endif

It would be better to avoid #ifdefs when none where necessary before.


>   		pr_alert("BUG: %s on %s at 0x%08lx\n",
>   			 regs->dar < PAGE_SIZE ? "Kernel NULL pointer dereference" :
>   			 "Unable to handle kernel data access",
>   			 is_write ? "write" : "read", regs->dar);
>   		break;
> -	case 0x400:
> -	case 0x480:
> +	case INTERRUPT_INST_STORAGE:
> +#ifdef CONFIG_PPC_BOOK3S
> +	case INTERRUPT_INST_SEGMENT:
> +#endif

It would be better to avoid #ifdefs when none where necessary before.



>   		pr_alert("BUG: Unable to handle kernel instruction fetch%s",
>   			 regs->nip < PAGE_SIZE ? " (NULL pointer?)\n" : "\n");
>   		break;
> -	case 0x600:
> +	case INTERRUPT_ALIGNMENT:
>   		pr_alert("BUG: Unable to handle kernel unaligned access at 0x%08lx\n",
>   			 regs->dar);
>   		break;
> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> index 766f064f00fb..6e34f5bba232 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -17,6 +17,7 @@
>   #include <asm/firmware.h>
>   #include <asm/ptrace.h>
>   #include <asm/code-patching.h>
> +#include <asm/traps.h>
>   
>   #ifdef CONFIG_PPC64
>   #include "internal.h"
> @@ -168,7 +169,7 @@ static bool regs_use_siar(struct pt_regs *regs)
>   	 * they have not been setup using perf_read_regs() and so regs->result
>   	 * is something random.
>   	 */
> -	return ((TRAP(regs) == 0xf00) && regs->result);
> +	return ((TRAP(regs) == INTERRUPT_PERFMON) && regs->result);
>   }
>   
>   /*
> @@ -347,7 +348,7 @@ static inline void perf_read_regs(struct pt_regs *regs)
>   	 * hypervisor samples as well as samples in the kernel with
>   	 * interrupts off hence the userspace check.
>   	 */
> -	if (TRAP(regs) != 0xf00)
> +	if (TRAP(regs) != INTERRUPT_PERFMON)
>   		use_siar = 0;
>   	else if ((ppmu->flags & PPMU_NO_SIAR))
>   		use_siar = 0;
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index bf7d69625a2e..2a4f99e64bf3 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -54,6 +54,7 @@
>   #include <asm/code-patching.h>
>   #include <asm/sections.h>
>   #include <asm/inst.h>
> +#include <asm/traps.h>
>   
>   #ifdef CONFIG_PPC64
>   #include <asm/hvcall.h>
> @@ -1769,7 +1770,12 @@ static void excprint(struct pt_regs *fp)
>   	printf("    sp: %lx\n", fp->gpr[1]);
>   	printf("   msr: %lx\n", fp->msr);
>   
> -	if (trap == 0x300 || trap == 0x380 || trap == 0x600 || trap == 0x200) {
> +	if (trap == INTERRUPT_DATA_STORAGE ||
> +#ifdef CONFIG_PPC_BOOK3S
> +	    trap == INTERRUPT_DATA_SEGMENT ||
> +#endif
It would be better to avoid #ifdefs when none where necessary before.

And an #ifdef in the middle of a code line is awful for readability and maintainability.

> +	    trap == INTERRUPT_ALIGNMENT ||
> +	    trap == INTERRUPT_MACHINE_CHECK) {
>   		printf("   dar: %lx\n", fp->dar);
>   		if (trap != 0x380)
>   			printf(" dsisr: %lx\n", fp->dsisr);
> @@ -1785,7 +1791,7 @@ static void excprint(struct pt_regs *fp)
>   		       current->pid, current->comm);
>   	}
>   
> -	if (trap == 0x700)
> +	if (trap == INTERRUPT_PROGRAM)
>   		print_bug_trap(fp);
>   
>   	printf(linux_banner);
> @@ -1846,7 +1852,11 @@ static void prregs(struct pt_regs *fp)
>   	printf("ctr = "REG"   xer = "REG"   trap = %4lx\n",
>   	       fp->ctr, fp->xer, fp->trap);
>   	trap = TRAP(fp);
> -	if (trap == 0x300 || trap == 0x380 || trap == 0x600)
> +	if (trap == INTERRUPT_DATA_STORAGE ||
> +#ifdef CONFIG_PPC_BOOK3S
> +	    trap == INTERRUPT_DATA_SEGMENT ||
> +#endif
> +	    trap == INTERRUPT_ALIGNMENT)

It would be better to avoid #ifdefs when none where necessary before.

And an #ifdef in the middle of a code line is awful for readability and maintainability.


>   		printf("dar = "REG"   dsisr = %.8lx\n", fp->dar, fp->dsisr);
>   }
>   
> 

^ permalink raw reply

* Re: [PATCHv5 2/2] powerpc/pseries: update device tree before ejecting hotplug uevents
From: Michal Suchánek @ 2021-04-09 16:33 UTC (permalink / raw)
  To: Pingfan Liu
  Cc: Nathan Lynch, Kexec Mailing List, Nathan Fontenot, Laurent Dufour,
	linuxppc-dev, Hari Bathini
In-Reply-To: <CAFgQCTs58dOcO0CGVDD1WxiOE9yaPe_WG-U=t+=at45bAipQ_A@mail.gmail.com>

Hello,

On Fri, Aug 28, 2020 at 04:10:09PM +0800, Pingfan Liu wrote:
> On Thu, Aug 27, 2020 at 3:53 PM Laurent Dufour <ldufour@linux.ibm.com> wrote:
> >
> > Le 10/08/2020 à 10:52, Pingfan Liu a écrit :
> > > A bug is observed on pseries by taking the following steps on rhel:
> > > -1. drmgr -c mem -r -q 5
> > > -2. echo c > /proc/sysrq-trigger
> > >
> > > And then, the failure looks like:
> > > kdump: saving to /sysroot//var/crash/127.0.0.1-2020-01-16-02:06:14/
> > > kdump: saving vmcore-dmesg.txt
> > > kdump: saving vmcore-dmesg.txt complete
> > > kdump: saving vmcore
> > >   Checking for memory holes                         : [  0.0 %] /                   Checking for memory holes                         : [100.0 %] |                   Excluding unnecessary pages                       : [100.0 %] \                   Copying data                                      : [  0.3 %] -          eta: 38s[   44.337636] hash-mmu: mm: Hashing failure ! EA=0x7fffba400000 access=0x8000000000000004 current=makedumpfile
> > > [   44.337663] hash-mmu:     trap=0x300 vsid=0x13a109c ssize=1 base psize=2 psize 2 pte=0xc000000050000504
> > > [   44.337677] hash-mmu: mm: Hashing failure ! EA=0x7fffba400000 access=0x8000000000000004 current=makedumpfile
> > > [   44.337692] hash-mmu:     trap=0x300 vsid=0x13a109c ssize=1 base psize=2 psize 2 pte=0xc000000050000504
> > > [   44.337708] makedumpfile[469]: unhandled signal 7 at 00007fffba400000 nip 00007fffbbc4d7fc lr 000000011356ca3c code 2
> > > [   44.338548] Core dump to |/bin/false pipe failed
> > > /lib/kdump-lib-initramfs.sh: line 98:   469 Bus error               $CORE_COLLECTOR /proc/vmcore $_mp/$KDUMP_PATH/$HOST_IP-$DATEDIR/vmcore-incomplete
> > > kdump: saving vmcore failed
> > >
> > > * Root cause *
> > >    After analyzing, it turns out that in the current implementation,
> > > when hot-removing lmb, the KOBJ_REMOVE event ejects before the dt updating as
> > > the code __remove_memory() comes before drmem_update_dt().
> > > So in kdump kernel, when read_from_oldmem() resorts to
> > > pSeries_lpar_hpte_insert() to install hpte, but fails with -2 due to
> > > non-exist pfn. And finally, low_hash_fault() raise SIGBUS to process, as it
> > > can be observed "Bus error"
> > >
> > >  From a viewpoint of listener and publisher, the publisher notifies the
> > > listener before data is ready.  This introduces a problem where udev
> > > launches kexec-tools (due to KOBJ_REMOVE) and loads a stale dt before
> > > updating. And in capture kernel, makedumpfile will access the memory based
> > > on the stale dt info, and hit a SIGBUS error due to an un-existed lmb.
> > >
> > > * Fix *
> > > This bug is introduced by commit 063b8b1251fd
> > > ("powerpc/pseries/memory-hotplug: Only update DT once per memory DLPAR
> > > request"), which tried to combine all the dt updating into one.
> > >
> > > To fix this issue, meanwhile not to introduce a quadratic runtime
> > > complexity by the model:
> > >    dlpar_memory_add_by_count
> > >      for_each_drmem_lmb             <--
> > >        dlpar_add_lmb
> > >          drmem_update_dt(_v1|_v2)
> > >            for_each_drmem_lmb       <--
> > > The dt should still be only updated once, and just before the last memory
> > > online/offline event is ejected to user space. Achieve this by tracing the
> > > num of lmb added or removed.
> > >
> > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > > Cc: Michael Ellerman <mpe@ellerman.id.au>
> > > Cc: Hari Bathini <hbathini@linux.ibm.com>
> > > Cc: Nathan Lynch <nathanl@linux.ibm.com>
> > > Cc: Nathan Fontenot <nfont@linux.vnet.ibm.com>
> > > Cc: Laurent Dufour <ldufour@linux.ibm.com>
> > > To: linuxppc-dev@lists.ozlabs.org
> > > Cc: kexec@lists.infradead.org
> > > ---
> > > v4 -> v5: change dlpar_add_lmb()/dlpar_remove_lmb() prototype to report
> > >            whether dt is updated successfully.
> > >            Fix a condition boundary check bug
> > > v3 -> v4: resolve a quadratic runtime complexity issue.
> > >            This series is applied on next-test branch
> > >   arch/powerpc/platforms/pseries/hotplug-memory.c | 102 +++++++++++++++++++-----
> > >   1 file changed, 80 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
> > > index 46cbcd1..1567d9f 100644
> > > --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> > > +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> > > @@ -350,13 +350,22 @@ static bool lmb_is_removable(struct drmem_lmb *lmb)
> > >       return true;
> > >   }
> > >
> > > -static int dlpar_add_lmb(struct drmem_lmb *);
> > > +enum dt_update_status {
> > > +     DT_NOUPDATE,
> > > +     DT_TOUPDATE,
> > > +     DT_UPDATED,
> > > +};
> > > +
> > > +/* "*dt_update" returns DT_UPDATED if updated */
> > > +static int dlpar_add_lmb(struct drmem_lmb *lmb,
> > > +             enum dt_update_status *dt_update);
> > >
> > > -static int dlpar_remove_lmb(struct drmem_lmb *lmb)
> > > +static int dlpar_remove_lmb(struct drmem_lmb *lmb,
> > > +             enum dt_update_status *dt_update)
> > >   {
> > >       unsigned long block_sz;
> > >       phys_addr_t base_addr;
> > > -     int rc, nid;
> > > +     int rc, ret, nid;
> > >
> > >       if (!lmb_is_removable(lmb))
> > >               return -EINVAL;
> > > @@ -372,6 +381,13 @@ static int dlpar_remove_lmb(struct drmem_lmb *lmb)
> > >       invalidate_lmb_associativity_index(lmb);
> > >       lmb_clear_nid(lmb);
> > >       lmb->flags &= ~DRCONF_MEM_ASSIGNED;
> > > +     if (*dt_update) {
> Original, I plan to use it to exclude the case of DT_NOUPDATE, which is value 0.
> And I think it looks better by using if (*dt_update == DT_TOUPDATE)
> >
> > That test is wrong, you should do:
> >          if (*dt_update && *dt_update == DT_TOUPDATE) {
> I think you mean  if (dt_update && *dt_update == DT_TOUPDATE) {
> >
> > With the current code, the device tree is updated all the time.
> >
> > Another option would be to pass a valid pointer (!= NULL) only when DT update is
> > required, this way you don't need the DT_TOUPDATE value. The caller would have
> > to set the pointer accordingly. The advantage with this option is the caller is
> > guaranteed that its variable is not touched by the callee when no device tree is
> > requested. A simple boolean pointer would be enough without the need to this enum.
> It is expected that dlpar_remove_lmb/dlpar_add_lmb can report whether
> they successfully update dt or not. So the caller can handle the
> different cases.

Is there any plan to refresh this patch to apply to master?

I am using an older revision of this patch so I am not in the position
to repost an updated version.

I lack some otimization in my patch so I probably have the quadratic
coplexity of the add mentioned above.

Thanks

Michal

^ permalink raw reply

* Re: [PATCH v3] powerpc/traps: Enhance readability for trap types
From: Segher Boessenkool @ 2021-04-09 17:45 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: pmladek, peterz, linux-kernel, paulus, kan.liang, mikey, maddy,
	haren, aik, kjain, ravi.bangoria, john.ogness, alistair, npiggin,
	jniethe5, atrajeev, Xiongwei Song, Xiongwei Song, oleg,
	aneesh.kumar, akpm, linuxppc-dev, rppt
In-Reply-To: <70ece993-12bd-335c-d246-914564eb51dd@csgroup.eu>

On Fri, Apr 09, 2021 at 06:14:19PM +0200, Christophe Leroy wrote:
> >+#define INTERRUPT_SYSTEM_RESET    0x100
> 
> INT_SRESET

SRESET exists on many PowerPC, it means "soft reset".  Not the same
thing at all.

I think "INT" is not a great prefix fwiw, there are many things you can
abbr to "INT".

> >+#define INTERRUPT_DATA_SEGMENT    0x380
> 
> INT_DSEG

exceptions-64s.S calls this "DSLB" (I remember "DSSI" though -- but neither
is a very official name).  It probably is a good idea to look at that
existing code, not make up even more new names :-)

> >+#define INTERRUPT_DOORBELL        0xa00
> 
> INT_DBELL

That saves three characters and makes it very not understandable.


Segher

^ permalink raw reply

* [Bug 212631] Misaligned floating point loads and store occasionally fail
From: bugzilla-daemon @ 2021-04-09 18:10 UTC (permalink / raw)
  To: linuxppc-dev
In-Reply-To: <bug-212631-206035@https.bugzilla.kernel.org/>

https://bugzilla.kernel.org/show_bug.cgi?id=212631

--- Comment #2 from Trevor Davenport (trevor_davenport@selinc.com) ---
A git bisect found this has existed for quite a while. 

git bisect start
# bad: [0cc244011f40280b78fc344d5c2aac5a0c659f77] Linux 4.14.229
git bisect bad 0cc244011f40280b78fc344d5c2aac5a0c659f77
# good: [a0c646821e9dedc5368abd2f71f50ebe2c351d19] Linux 4.4.265
git bisect good a0c646821e9dedc5368abd2f71f50ebe2c351d19
# good: [afd2ff9b7e1b367172f18ba7f693dfb62bdcb2dc] Linux 4.4
git bisect good afd2ff9b7e1b367172f18ba7f693dfb62bdcb2dc
# good: [786a72d79140028537382fa63bea63d5640c27d6] Merge tag 'armsoc-dt' of
git://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc
git bisect good 786a72d79140028537382fa63bea63d5640c27d6
# good: [e0f25a3f2d052e36ff67a9b4db835c3e27e950d8] Merge tag 'hwlock-v4.13' of
git://github.com/andersson/remoteproc
git bisect good e0f25a3f2d052e36ff67a9b4db835c3e27e950d8
# bad: [dd9d064e34a1b1c96d631cca73e2a6efc5834f4a] Merge tag 'staging-4.14-rc6'
of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging
git bisect bad dd9d064e34a1b1c96d631cca73e2a6efc5834f4a
# good: [b88f55774f20c0c306e0a95d22ca9ab5f08187c7] Merge tag 'spi-v4.14' of
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi
git bisect good b88f55774f20c0c306e0a95d22ca9ab5f08187c7
# good: [b88f55774f20c0c306e0a95d22ca9ab5f08187c7] Merge tag 'spi-v4.14' of
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi
git bisect good b88f55774f20c0c306e0a95d22ca9ab5f08187c7
# bad: [cef5d0f952a03d42051141742632078d488b0c6b] Merge branch 'for-linus' of
git://git.kernel.org/pub/scm/linux/kernel/git/pmladek/printk
git bisect bad cef5d0f952a03d42051141742632078d488b0c6b
# good: [aae3dbb4776e7916b6cd442d00159bea27a695c1] Merge
git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next
git bisect good aae3dbb4776e7916b6cd442d00159bea27a695c1
# good: [aae3dbb4776e7916b6cd442d00159bea27a695c1] Merge
git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next
git bisect good aae3dbb4776e7916b6cd442d00159bea27a695c1
# bad: [3645e6d0dc80be4376f87acc9ee527768387c909] Merge tag 'md/4.14-rc1' of
git://git.kernel.org/pub/scm/linux/kernel/git/shli/md
git bisect bad 3645e6d0dc80be4376f87acc9ee527768387c909
# bad: [bac65d9d87b383471d8d29128319508d71b74180] Merge tag 'powerpc-4.14-1' of
git://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux
git bisect bad bac65d9d87b383471d8d29128319508d71b74180
# good: [57e88b43b81301d9b28f124a5576ac43a1cf9e8d] Merge branch
'x86-platform-for-linus' of
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
git bisect good 57e88b43b81301d9b28f124a5576ac43a1cf9e8d
# good: [f9065c83ccf4a6c1ff5419d216ad8276e99bee6c] powerpc/configs: Explicitly
drop CONFIG_INPUT_MOUSEDEV
git bisect good f9065c83ccf4a6c1ff5419d216ad8276e99bee6c
# good: [d1e1b351f50f9e5941f436f6c63949731979e00c] powerpc/xmon: Add ISA v3.0
SPRs to SPR dump
git bisect good d1e1b351f50f9e5941f436f6c63949731979e00c
# bad: [146e9f1b65478643f2729a97ccb8be60bb4492e5] crypto/nx: Add P9 NX specific
error codes for 842 engine
git bisect bad 146e9f1b65478643f2729a97ccb8be60bb4492e5
# good: [5762e08344bd7c5bfc41030f74c4ab6ce6e461d0] powerpc: Don't update CR0 in
emulation of popcnt, prty, bpermd instructions
git bisect good 5762e08344bd7c5bfc41030f74c4ab6ce6e461d0
# bad: [d2b65ac6526a82965212b632d42687251e122a36] powerpc: Emulate load/store
floating point as integer word instructions
git bisect bad d2b65ac6526a82965212b632d42687251e122a36
# good: [1f41fb790460acf432f826f4aeeff6f7da891ff7] powerpc: Emulate load/store
floating double pair instructions
git bisect good 1f41fb790460acf432f826f4aeeff6f7da891ff7
# good: [d955189ae42796621fb439e5e778ccaeebc2a1e7] powerpc: Handle
opposite-endian processes in emulation code
git bisect good d955189ae42796621fb439e5e778ccaeebc2a1e7
# bad: [31bfdb036f1281831db2532178f0da41f4dc9bed] powerpc: Use instruction
emulation infrastructure to handle alignment faults
git bisect bad 31bfdb036f1281831db2532178f0da41f4dc9bed
# good: [a53d5182e24c22986ad0e99e52f8fe343ee7d7ac] powerpc: Separate out
load/store emulation into its own function
git bisect good a53d5182e24c22986ad0e99e52f8fe343ee7d7ac
# first bad commit: [31bfdb036f1281831db2532178f0da41f4dc9bed] powerpc: Use
instruction emulation infrastructure to handle alignment faults


31bfdb036f1281831db2532178f0da41f4dc9bed is the first bad commit
commit 31bfdb036f1281831db2532178f0da41f4dc9bed
Author: Paul Mackerras <paulus@ozlabs.org>
Date:   Wed Aug 30 14:12:40 2017 +1000

    powerpc: Use instruction emulation infrastructure to handle alignment
faults

    This replaces almost all of the instruction emulation code in
    fix_alignment() with calls to analyse_instr(), emulate_loadstore()
    and emulate_dcbz().  The only emulation code left is the SPE
    emulation code; analyse_instr() etc. do not handle SPE instructions
    at present.

    One result of this is that we can now handle alignment faults on
    all the new VSX load and store instructions that were added in POWER9.
    VSX loads/stores will take alignment faults for unaligned accesses
    to cache-inhibited memory.

    Another effect is that we no longer rely on the DAR and DSISR values
    set by the processor.

    With this, we now need to include the instruction emulation code
    unconditionally.

    Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
    Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

 arch/powerpc/Kconfig        |   4 -
 arch/powerpc/kernel/align.c | 803 ++------------------------------------------
 arch/powerpc/lib/Makefile   |   4 +-
 3 files changed, 34 insertions(+), 777 deletions(-)

-- 
You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

^ permalink raw reply

* RE: [PATCH -next] soc: fsl: qe: use DEFINE_SPINLOCK() for spinlock
From: Leo Li @ 2021-04-09 21:56 UTC (permalink / raw)
  To: Ye Bin, Qiang Zhao
  Cc: kernel-janitors@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, Hulk Robot
In-Reply-To: <20210409095152.2294487-1-yebin10@huawei.com>



> -----Original Message-----
> From: Ye Bin <yebin10@huawei.com>
> Sent: Friday, April 9, 2021 4:52 AM
> To: yebin10@huawei.com; Qiang Zhao <qiang.zhao@nxp.com>; Leo Li
> <leoyang.li@nxp.com>
> Cc: linuxppc-dev@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel-janitors@vger.kernel.org; Hulk Robot
> <hulkci@huawei.com>
> Subject: [PATCH -next] soc: fsl: qe: use DEFINE_SPINLOCK() for spinlock
> 
> spinlock can be initialized automatically with DEFINE_SPINLOCK() rather than
> explicitly calling spin_lock_init().

The previous version has been applied.  Thanks.

> 
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> ---
>  drivers/soc/fsl/qe/qe_common.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/soc/fsl/qe/qe_common.c
> b/drivers/soc/fsl/qe/qe_common.c index 654e9246ce6b..a0cb8e746879
> 100644
> --- a/drivers/soc/fsl/qe/qe_common.c
> +++ b/drivers/soc/fsl/qe/qe_common.c
> @@ -26,7 +26,7 @@
>  #include <soc/fsl/qe/qe.h>
> 
>  static struct gen_pool *muram_pool;
> -static spinlock_t cpm_muram_lock;
> +static DEFINE_SPINLOCK(cpm_muram_lock);
>  static void __iomem *muram_vbase;
>  static phys_addr_t muram_pbase;
> 
> @@ -54,7 +54,6 @@ int cpm_muram_init(void)
>  	if (muram_pbase)
>  		return 0;
> 
> -	spin_lock_init(&cpm_muram_lock);
>  	np = of_find_compatible_node(NULL, NULL, "fsl,cpm-muram-data");
>  	if (!np) {
>  		/* try legacy bindings */


^ permalink raw reply

* Re: [PATCH v2 1/1] kernel.h: Split out panic and oops helpers
From: Stephen Boyd @ 2021-04-09 16:54 UTC (permalink / raw)
  To: Alexander Egorenkov, Alexander Lobakin, Andrew Morton,
	Andy Shevchenko, Arnd Bergmann, Bjorn Andersson,
	Christian Brauner, Corey Minyard, Florian Fainelli,
	Greg Kroah-Hartman, Heiko Carstens, Hongbo Yao, Jason J.Herne,
	Joe Perches, Joerg Roedel, Kees Cook, Krzysztof Kozlowski,
	Marek Czerski, Mathieu Poirier, Michael Ellerman, Michael Kelley,
	Mihai Carabas, Mike Rapoport, Olof Johansson, Paul E.McKenney,
	Peter Oberparleiter, Rasmus Villemoes, Scott Branden,
	Steven Rostedt, Tetsuo Handa, Thomas Bogendoerfer, Vasily Gorbik,
	Vineeth Vijayan, Vlastimil Babka, Wang Wenhu, Wei Liu,
	bcm-kernel-feedback-list, coresight, dri-devel, kexec,
	linux-alpha, linux-arch, linux-arm-kernel, linux-clk, linux-edac,
	linux-fbdev, linux-fsdevel, linux-hyperv, linux-kernel,
	linux-leds, linux-mips, linux-parisc, linux-pm, linux-remoteproc,
	ger.kernel.org, linux-s390, linux-staging, linux-um, linux-xtensa,
	linuxppc-dev, netdev, openipmi-developer, rcu, sparclinux,
	xen-devel
  Cc: Alexander Shishkin, Catalin Marinas, Michael Turquette,
	Lai Jiangshan, James E.J. Bottomley, Max Filippov, Paul Mackerras,
	Pavel Machek, H. Peter Anvin, Joel Fernandes, et,
	K. Y. Srinivasan, Will Deacon, Boris Ostrovsky, Anton Ivanov,
	Stefano Stabellini, Stephen Hemminger, Daniel Drake,
	Jens Frederich, Chris Zankel, Richard Weinberger, Helge Deller,
	x86, Mike Rapoport, Christian Borntraeger, Ingo Molnar,
	Mathieu Desnoyers, Jakub Kicinski, Matt Turner, Iurii Zaikin,
	Haiyang Zhang, Mike Leach, Ohad Ben-Cohen, Robert Richter,
	Corey Minyard, Suzuki K Poulose, Jeff Dike, Josh Triplett,
	Alex Elder, Ivan Kokshaysky, Paul Walmsley, Thomas Gleixner,
	Mauro Carvalho Chehab, Richard Henderson, Juergen Gross,
	Tony Luck, Jon Nettleton, Sebastian Reichel, Dinh Nguyen,
	Lui s Chamberlain, James Morse, Eric Biederman, Leo Yan,
	Borislav Petkov, David S. Miller
In-Reply-To: <20210409100250.25922-1-andriy.shevchenko@linux.intel.com>

Quoting Andy Shevchenko (2021-04-09 03:02:50)
> kernel.h is being used as a dump for all kinds of stuff for a long time.
> Here is the attempt to start cleaning it up by splitting out panic and
> oops helpers.
> 
> There are several purposes of doing this:
> - dropping dependency in bug.h
> - dropping a loop by moving out panic_notifier.h
> - unload kernel.h from something which has its own domain
> 
> At the same time convert users tree-wide to use new headers, although
> for the time being include new header back to kernel.h to avoid twisted
> indirected includes for existing users.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Acked-by: Mike Rapoport <rppt@linux.ibm.com>
> Acked-by: Corey Minyard <cminyard@mvista.com>
> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> Acked-by: Kees Cook <keescook@chromium.org>
> Acked-by: Wei Liu <wei.liu@kernel.org>
> Acked-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---

>  drivers/clk/analogbits/wrpll-cln28hpc.c       |  4 +

Acked-by: Stephen Boyd <sboyd@kernel.org>

^ permalink raw reply

* Re: [PATCH v2 1/1] kernel.h: Split out panic and oops helpers
From: Luis Chamberlain @ 2021-04-09 16:59 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Corey Minyard, dri-devel, Paul Mackerras, Pavel Machek,
	K. Y. Srinivasan, linux-clk, linux-arch, Wei Liu,
	Mauro Carvalho Chehab, Jens Frederich, Catalin Marinas, xen-devel,
	Matt Turner, Ohad Ben-Cohen, linux-pm, Lai Jiangshan, linux-um,
	Daniel Drake, Mihai Carabas, Thomas Gleixner, Richard Henderson,
	Alex Elder, Greg Kroah-Hartman, linux-kernel, Joe Perches,
	Andrew Morton, Marek Czerski, Tetsuo Handa, Jon Nettleton,
	Alexander Egorenkov, James E.J. Bottomley, Max Filippov,
	Christian Brauner, linux-s390, Stefano Stabellini,
	Stephen Hemminger, Corey Minyard, Krzysztof Kozlowski,
	Helge Deller, Suzuki K Poulose, Mathieu Desnoyers, Iurii Zaikin,
	linux-xtensa, Joerg Roedel, Vasily Gorbik, Scott Branden,
	coresight, linux-fsdevel, Boris Ostrovsky, linux-arm-kernel,
	Chris Zankel, Tony Luck, Mathieu Poirier, Stephen Boyd,
	Peter Oberparleiter, Dinh Nguyen, James Morse, Eric Biederman,
	Alexander Shishkin, Michael Turquette, Rasmus Villemoes,
	linux-hyperv, Vineeth Vijayan, Joel Fernandes, Will Deacon,
	Florian Fainelli, linux-staging, Christian Borntraeger,
	bcm-kernel-feedback-list, Juergen Gross, Robert Richter,
	Kees Cook, Arnd Bergmann, Haiyang Zhang, Josh Triplett,
	Steven Rostedt (VMware), rcu, Borislav Petkov, linux-fbdev,
	openipmi-developer, Michael Kelley, Thomas Bogendoerfer,
	linux-parisc, Hongbo Yao, Sebastian Reichel, linux-alpha,
	Olof Johansson, David S. Miller, Mike Rapoport, Alexander Lobakin,
	linux-remoteproc, Bjorn Andersson, H. Peter Anvin, sparclinux,
	linux-leds, Anton Ivanov, Richard Weinberger, x86, Mike Rapoport,
	Ingo Molnar, Jakub Kicinski, Wang Wenhu, Jeff Dike, Mike Leach,
	Paul E. McKenney, Heiko Carstens, Paul Walmsley, Vlastimil Babka,
	linux-edac, Jason J. Herne, netdev, kexec, linux-mips, Leo Yan,
	Ivan Kokshaysky, linuxppc-dev
In-Reply-To: <20210409100250.25922-1-andriy.shevchenko@linux.intel.com>

On Fri, Apr 09, 2021 at 01:02:50PM +0300, Andy Shevchenko wrote:
> kernel.h is being used as a dump for all kinds of stuff for a long time.
> Here is the attempt to start cleaning it up by splitting out panic and
> oops helpers.
> 
> There are several purposes of doing this:
> - dropping dependency in bug.h
> - dropping a loop by moving out panic_notifier.h
> - unload kernel.h from something which has its own domain
> 
> At the same time convert users tree-wide to use new headers, although
> for the time being include new header back to kernel.h to avoid twisted
> indirected includes for existing users.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Acked-by: Mike Rapoport <rppt@linux.ibm.com>
> Acked-by: Corey Minyard <cminyard@mvista.com>
> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> Acked-by: Kees Cook <keescook@chromium.org>
> Acked-by: Wei Liu <wei.liu@kernel.org>
> Acked-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

Acked-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

^ permalink raw reply

* Re: [PATCH v2 1/1] kernel.h: Split out panic and oops helpers
From: Helge Deller @ 2021-04-09 19:41 UTC (permalink / raw)
  To: Andy Shevchenko, Thomas Bogendoerfer, Alexander Lobakin, Wei Liu,
	Rasmus Villemoes, Arnd Bergmann, Bjorn Andersson,
	Christian Brauner, Andrew Morton, Kees Cook, Mike Rapoport,
	Corey Minyard, Michael Ellerman, Vasily Gorbik, Jason J. Herne,
	Joerg Roedel, Michael Kelley, Joe Perches, Florian Fainelli,
	Krzysztof Kozlowski, Greg Kroah-Hartman, Scott Branden,
	Olof Johansson, Mihai Carabas, Wang Wenhu, Marek Czerski,
	Hongbo Yao, Mathieu Poirier, Vineeth Vijayan, Heiko Carstens,
	Peter Oberparleiter, Alexander Egorenkov, Tetsuo Handa,
	Vlastimil Babka, Paul E. McKenney, Steven Rostedt (VMware),
	linux-alpha, linux-kernel, linux-arm-kernel, linux-mips,
	linux-parisc, linuxppc-dev, linux-s390, sparclinux, linux-um,
	linux-hyperv, xen-devel, linux-xtensa, openipmi-developer,
	linux-clk, linux-edac, coresight, linux-leds,
	bcm-kernel-feedback-list, netdev, linux-pm, linux-remoteproc,
	linux-staging, dri-devel, linux-fbdev, linux-arch, kexec, rcu,
	linux-fsdevel
  Cc: Alexander Shishkin, Catalin Marinas, Michael Turquette,
	Lai Jiangshan, James E.J. Bottomley, Max Filippov, Paul Mackerras,
	Pavel Machek, H. Peter Anvin, Joel Fernandes, K. Y. Srinivasan,
	Will Deacon, Boris Ostrovsky, Anton Ivanov, Stefano Stabellini,
	Stephen Hemminger, Daniel Drake, Jens Frederich,
	Richard Weinberger, x86, Mike Rapoport, Christian Borntraeger,
	Ingo Molnar, Mathieu Desnoyers, Jakub Kicinski, Matt Turner,
	Iurii Zaikin, Haiyang Zhang, Mike Leach, Ohad Ben-Cohen,
	Robert Richter, Corey Minyard, Suzuki K Poulose, Jeff Dike,
	Josh Triplett, Alex Elder, Ivan Kokshaysky, Paul Walmsley,
	Thomas Gleixner, Mauro Carvalho Chehab, Richard Henderson,
	Juergen Gross, Chris Zankel, Tony Luck, Jon Nettleton,
	Stephen Boyd, Sebastian Reichel, Dinh Nguyen, Luis Chamberlain,
	James Morse, Eric Biederman, Leo Yan, Borislav Petkov,
	David S. Miller
In-Reply-To: <20210409100250.25922-1-andriy.shevchenko@linux.intel.com>

On 4/9/21 12:02 PM, Andy Shevchenko wrote:
> kernel.h is being used as a dump for all kinds of stuff for a long time.
> Here is the attempt to start cleaning it up by splitting out panic and
> oops helpers.
>
> There are several purposes of doing this:
> - dropping dependency in bug.h
> - dropping a loop by moving out panic_notifier.h
> - unload kernel.h from something which has its own domain
>
> At the same time convert users tree-wide to use new headers, although
> for the time being include new header back to kernel.h to avoid twisted
> indirected includes for existing users.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Acked-by: Mike Rapoport <rppt@linux.ibm.com>
> Acked-by: Corey Minyard <cminyard@mvista.com>
> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> Acked-by: Kees Cook <keescook@chromium.org>
> Acked-by: Wei Liu <wei.liu@kernel.org>
> Acked-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

Acked-by: Helge Deller <deller@gmx.de> # parisc

Helge

^ permalink raw reply

* Re: [PATCH v2 1/1] kernel.h: Split out panic and oops helpers
From: Thomas Bogendoerfer @ 2021-04-09 22:39 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Corey Minyard, dri-devel, Paul Mackerras, Pavel Machek,
	K. Y. Srinivasan, linux-clk, linux-arch, Wei Liu,
	Mauro Carvalho Chehab, Jens Frederich, Catalin Marinas, xen-devel,
	Matt Turner, Ohad Ben-Cohen, linux-pm, Lai Jiangshan, linux-um,
	Daniel Drake, Mihai Carabas, Thomas Gleixner, Richard Henderson,
	Alex Elder, Greg Kroah-Hartman, linux-kernel, Luis Chamberlain,
	Joe Perches, Andrew Morton, Marek Czerski, Tetsuo Handa,
	Jon Nettleton, Alexander Egorenkov, James E.J. Bottomley,
	Max Filippov, Christian Brauner, linux-s390, Stefano Stabellini,
	Stephen Hemminger, Corey Minyard, Krzysztof Kozlowski,
	Helge Deller, Suzuki K Poulose, Mathieu Desnoyers, Iurii Zaikin,
	linux-xtensa, Joerg Roedel, Vasily Gorbik, Scott Branden,
	coresight, linux-fsdevel, Boris Ostrovsky, linux-arm-kernel,
	Chris Zankel, Tony Luck, Mathieu Poirier, Stephen Boyd,
	Peter Oberparleiter, Dinh Nguyen, James Morse, Eric Biederman,
	Alexander Shishkin, Michael Turquette, Rasmus Villemoes,
	linux-hyperv, Vineeth Vijayan, Joel Fernandes, Will Deacon,
	Florian Fainelli, linux-staging, Christian Borntraeger,
	bcm-kernel-feedback-list, Jason J. Herne, Robert Richter,
	Kees Cook, Arnd Bergmann, Haiyang Zhang, Josh Triplett,
	Steven Rostedt (VMware), rcu, Borislav Petkov, linux-fbdev,
	openipmi-developer, Michael Kelley, linux-parisc, Hongbo Yao,
	Sebastian Reichel, linux-alpha, Olof Johansson, David S. Miller,
	Mike Rapoport, Alexander Lobakin, linux-remoteproc,
	Bjorn Andersson, H. Peter Anvin, sparclinux, linux-leds,
	Anton Ivanov, Richard Weinberger, x86, Mike Rapoport, Ingo Molnar,
	Jakub Kicinski, Wang Wenhu, Jeff Dike, Mike Leach,
	Paul E. McKenney, Heiko Carstens, Paul Walmsley, Vlastimil Babka,
	linux-edac, Juergen Gross, netdev, kexec, linux-mips, Leo Yan,
	Ivan Kokshaysky, linuxppc-dev
In-Reply-To: <20210409100250.25922-1-andriy.shevchenko@linux.intel.com>

On Fri, Apr 09, 2021 at 01:02:50PM +0300, Andy Shevchenko wrote:
> kernel.h is being used as a dump for all kinds of stuff for a long time.
> Here is the attempt to start cleaning it up by splitting out panic and
> oops helpers.
> 
> There are several purposes of doing this:
> - dropping dependency in bug.h
> - dropping a loop by moving out panic_notifier.h
> - unload kernel.h from something which has its own domain
> 
> At the same time convert users tree-wide to use new headers, although
> for the time being include new header back to kernel.h to avoid twisted
> indirected includes for existing users.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Acked-by: Mike Rapoport <rppt@linux.ibm.com>
> Acked-by: Corey Minyard <cminyard@mvista.com>
> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> Acked-by: Kees Cook <keescook@chromium.org>
> Acked-by: Wei Liu <wei.liu@kernel.org>
> Acked-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> v2:
>  - fixed all errors with allmodconfig on x86_64 (Andrew)
>  - checked with allyesconfig on x86_64
>  - additionally grepped source code for panic notifier list usage
>    and converted all users
>  - elaborated commit message (Luis)
>  - collected given tags (incl. Andrew's SoB, see below)
> 
> I added Andrew's SoB since part of the fixes I took from him. Andrew,
> feel free to amend or tell me how you want me to do.
> 
>  arch/mips/kernel/relocate.c                   |  1 +
>  arch/mips/sgi-ip22/ip22-reset.c               |  1 +
>  arch/mips/sgi-ip32/ip32-reset.c               |  1 +

Acked-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

^ permalink raw reply

* Re: [PATCH v3] powerpc/traps: Enhance readability for trap types
From: Michael Ellerman @ 2021-04-10  0:04 UTC (permalink / raw)
  To: Christophe Leroy, Xiongwei Song, benh, paulus, oleg, npiggin,
	aneesh.kumar, ravi.bangoria, mikey, haren, akpm, rppt, jniethe5,
	atrajeev, maddy, peterz, kjain, kan.liang, aik, alistair, pmladek,
	john.ogness
  Cc: Xiongwei Song, linuxppc-dev, linux-kernel
In-Reply-To: <70ece993-12bd-335c-d246-914564eb51dd@csgroup.eu>

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 08/04/2021 à 16:07, Xiongwei Song a écrit :
>> From: Xiongwei Song <sxwjean@gmail.com>
>> 
>> Create a new header named traps.h, define macros to list ppc interrupt
>> types in traps.h, replace the reference of the trap hex values with these
>> macros.
...
>> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
>> index 7c633896d758..5ce9898bc9a6 100644
>> --- a/arch/powerpc/include/asm/interrupt.h
>> +++ b/arch/powerpc/include/asm/interrupt.h
>> @@ -8,6 +8,7 @@
>>   #include <asm/ftrace.h>
>>   #include <asm/kprobes.h>
>>   #include <asm/runlatch.h>
>> +#include <asm/traps.h>
>>   
>>   struct interrupt_state {
>>   #ifdef CONFIG_PPC_BOOK3E_64
>> @@ -59,7 +60,7 @@ static inline void interrupt_enter_prepare(struct pt_regs *regs, struct interrup
>>   		 * CT_WARN_ON comes here via program_check_exception,
>>   		 * so avoid recursion.
>>   		 */
>> -		if (TRAP(regs) != 0x700)
>> +		if (TRAP(regs) != INTERRUPT_PROGRAM)
>>   			CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
>>   	}
>>   #endif
>> @@ -156,7 +157,8 @@ static inline void interrupt_nmi_enter_prepare(struct pt_regs *regs, struct inte
>>   	/* Don't do any per-CPU operations until interrupt state is fixed */
>>   #endif
>>   	/* Allow DEC and PMI to be traced when they are soft-NMI */
>> -	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260) {
>> +	if (TRAP(regs) != INTERRUPT_DECREMENTER &&
>> +	    TRAP(regs) != INTERRUPT_PERFMON) {
>
> I think too long names hinder readability, see later for suggestions.

I asked for the longer names :)

I think they make it easier for people who are less familiar with the
architecture than us to make sense of the names.

And there's only a couple of cases where it requires splitting a line,
and they could be converted to use switch if we think it's a problem.

>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>> index 0c0b1c2cfb49..641b3feef7ee 100644
>> --- a/arch/powerpc/mm/fault.c
>> +++ b/arch/powerpc/mm/fault.c
>> @@ -588,20 +589,24 @@ void __bad_page_fault(struct pt_regs *regs, int sig)
>>   	/* kernel has accessed a bad area */
>>   
>>   	switch (TRAP(regs)) {
>> -	case 0x300:
>> -	case 0x380:
>> -	case 0xe00:
>> +	case INTERRUPT_DATA_STORAGE:
>> +#ifdef CONFIG_PPC_BOOK3S
>> +	case INTERRUPT_DATA_SEGMENT:
>> +	case INTERRUPT_H_DATA_STORAGE:
>> +#endif
>
> It would be better to avoid #ifdefs when none where necessary before.

Yes I agree.

I think these can all be avoided by defining most of the values
regardless of what platform we're building for. Only the values that
overlap need to be kept behind an ifdef.

cheers

^ permalink raw reply

* Re: [PATCH v4] powerpc/traps: Enhance readability for trap types
From: Nicholas Piggin @ 2021-04-10  0:35 UTC (permalink / raw)
  To: aik, akpm, alistair, aneesh.kumar, atrajeev, benh,
	christophe.leroy, haren, jniethe5, john.ogness, kan.liang, kjain,
	maddy, mikey, mpe, oleg, paulus, peterz, pmladek, ravi.bangoria,
	rppt, Xiongwei Song
  Cc: Xiongwei Song, linuxppc-dev, linux-kernel
In-Reply-To: <20210409142832.26063-1-sxwjean@me.com>

Thanks for working on this, I think it's a nice cleanup and helps
non-powerpc people understand the code a bit better.

Excerpts from Xiongwei Song's message of April 10, 2021 12:28 am:
> From: Xiongwei Song <sxwjean@gmail.com>
> 
> Create a new header named traps.h, define macros to list ppc interrupt
> types in traps.h, replace the references of the trap hex values with these
> macros.
> 
> Referred the hex numbers in arch/powerpc/kernel/exceptions-64e.S,
> arch/powerpc/kernel/exceptions-64s.S and
> arch/powerpc/include/asm/kvm_asm.h.
> 
> Reported-by: kernel test robot <lkp@intel.com>

It now looks like lkp asked for this whole cleanup patch. I would
put [kernel test robot <lkp@intel.com>] in your v3->4 changelog
item.

> Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
> ---
> 
> v3-v4:
> Fix compile issue:
> arch/powerpc/kernel/process.c:1473:14: error: 'INTERRUPT_MACHINE_CHECK' undeclared (first use in this function); did you mean 'TAINT_MACHINE_CHECK'?
> I didn't add "Reported-by: kernel test robot <lkp@intel.com>" here,
> because it's improper for this patch.

[...]

> diff --git a/arch/powerpc/include/asm/traps.h b/arch/powerpc/include/asm/traps.h
> new file mode 100644
> index 000000000000..2e64e10afcef
> --- /dev/null
> +++ b/arch/powerpc/include/asm/traps.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_PPC_TRAPS_H
> +#define _ASM_PPC_TRAPS_H

These could go in interrupt.h.

> +#if defined(CONFIG_BOOKE) || defined(CONFIG_4xx)
> +#define INTERRUPT_MACHINE_CHECK   0x000
> +#define INTERRUPT_CRITICAL_INPUT  0x100
> +#define INTERRUPT_ALTIVEC_UNAVAIL 0x200
> +#define INTERRUPT_PERFMON         0x260
> +#define INTERRUPT_DOORBELL        0x280
> +#define INTERRUPT_DEBUG           0xd00
> +#else
> +#define INTERRUPT_SYSTEM_RESET    0x100
> +#define INTERRUPT_MACHINE_CHECK   0x200

[...]

> @@ -1469,7 +1470,9 @@ static void __show_regs(struct pt_regs *regs)
>  	trap = TRAP(regs);
>  	if (!trap_is_syscall(regs) && cpu_has_feature(CPU_FTR_CFAR))
>  		pr_cont("CFAR: "REG" ", regs->orig_gpr3);
> -	if (trap == 0x200 || trap == 0x300 || trap == 0x600) {
> +	if (trap == INTERRUPT_MACHINE_CHECK ||
> +	    trap == INTERRUPT_DATA_STORAGE ||
> +	    trap == INTERRUPT_ALIGNMENT) {
>  		if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
>  			pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->dsisr);
>  		else

This is now a change in behaviour because previously BOOKE/4xx tested
0x200, but now it tests 0.

That looks wrong for 4xx. 64e does put 0x000 there but I wonder if it 
should use 0x200 instead. Bit difficult to test this stuff, I do have
some MCE injection patches for QEMU for 64s, might be able to look at
porting them to 64e although I have no idea about booke machine checks.

Anyway I don't think this patch should change generated code at all.
Either change the code first with smaller patches, or make sure you
keep the tests the same.

Thanks,
Nick

^ permalink raw reply

* Re: [PATCH v6 30/48] KVM: PPC: Book3S HV P9: Implement the rest of the P9 path in C
From: Nicholas Piggin @ 2021-04-10  0:37 UTC (permalink / raw)
  To: Alexey Kardashevskiy, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <e3be3cb3-23a1-2e28-1bda-a7437f2fd2ca@ozlabs.ru>

Excerpts from Alexey Kardashevskiy's message of April 9, 2021 1:57 pm:
> 
> 
> On 05/04/2021 11:19, Nicholas Piggin wrote:
>> Almost all logic is moved to C, by introducing a new in_guest mode for
>> the P9 path that branches very early in the KVM interrupt handler to
>> P9 exit code.
>> 
>> The main P9 entry and exit assembly is now only about 160 lines of low
>> level stack setup and register save/restore, plus a bad-interrupt
>> handler.
>> 
>> There are two motivations for this, the first is just make the code more
>> maintainable being in C. The second is to reduce the amount of code
>> running in a special KVM mode, "realmode". In quotes because with radix
>> it is no longer necessarily real-mode in the MMU, but it still has to be
>> treated specially because it may be in real-mode, and has various
>> important registers like PID, DEC, TB, etc set to guest. This is hostile
>> to the rest of Linux and can't use arbitrary kernel functionality or be
>> instrumented well.
>> 
>> This initial patch is a reasonably faithful conversion of the asm code,
>> but it does lack any loop to return quickly back into the guest without
>> switching out of realmode in the case of unimportant or easily handled
>> interrupts. As explained in previous changes, handling HV interrupts
>> in real mode is not so important for P9.
>> 
>> Use of Linux 64s interrupt entry code register conventions including
>> paca EX_ save areas are brought into the KVM code. There is no point
>> shuffling things into different paca save areas and making up a
>> different calling convention for KVM.
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>   arch/powerpc/include/asm/asm-prototypes.h |   3 +-
>>   arch/powerpc/include/asm/kvm_asm.h        |   3 +-
>>   arch/powerpc/include/asm/kvm_book3s_64.h  |   8 +
>>   arch/powerpc/include/asm/kvm_host.h       |   7 +-
>>   arch/powerpc/kernel/security.c            |   5 +-
>>   arch/powerpc/kvm/Makefile                 |   1 +
>>   arch/powerpc/kvm/book3s_64_entry.S        | 247 ++++++++++++++++++++++
>>   arch/powerpc/kvm/book3s_hv.c              |   9 +-
>>   arch/powerpc/kvm/book3s_hv_interrupt.c    | 218 +++++++++++++++++++
>>   arch/powerpc/kvm/book3s_hv_rmhandlers.S   | 125 +----------
>>   10 files changed, 501 insertions(+), 125 deletions(-)
>>   create mode 100644 arch/powerpc/kvm/book3s_hv_interrupt.c
>> 
>> diff --git a/arch/powerpc/include/asm/asm-prototypes.h b/arch/powerpc/include/asm/asm-prototypes.h
>> index 939f3c94c8f3..7c74c80ed994 100644
>> --- a/arch/powerpc/include/asm/asm-prototypes.h
>> +++ b/arch/powerpc/include/asm/asm-prototypes.h
>> @@ -122,6 +122,7 @@ extern s32 patch__call_flush_branch_caches3;
>>   extern s32 patch__flush_count_cache_return;
>>   extern s32 patch__flush_link_stack_return;
>>   extern s32 patch__call_kvm_flush_link_stack;
>> +extern s32 patch__call_kvm_flush_link_stack_p9;
>>   extern s32 patch__memset_nocache, patch__memcpy_nocache;
>>   
>>   extern long flush_branch_caches;
>> @@ -142,7 +143,7 @@ void kvmhv_load_host_pmu(void);
>>   void kvmhv_save_guest_pmu(struct kvm_vcpu *vcpu, bool pmu_in_use);
>>   void kvmhv_load_guest_pmu(struct kvm_vcpu *vcpu);
>>   
>> -int __kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu);
>> +void kvmppc_p9_enter_guest(struct kvm_vcpu *vcpu);
>>   
>>   long kvmppc_h_set_dabr(struct kvm_vcpu *vcpu, unsigned long dabr);
>>   long kvmppc_h_set_xdabr(struct kvm_vcpu *vcpu, unsigned long dabr,
>> diff --git a/arch/powerpc/include/asm/kvm_asm.h b/arch/powerpc/include/asm/kvm_asm.h
>> index a3633560493b..b4f9996bd331 100644
>> --- a/arch/powerpc/include/asm/kvm_asm.h
>> +++ b/arch/powerpc/include/asm/kvm_asm.h
>> @@ -146,7 +146,8 @@
>>   #define KVM_GUEST_MODE_GUEST	1
>>   #define KVM_GUEST_MODE_SKIP	2
>>   #define KVM_GUEST_MODE_GUEST_HV	3
>> -#define KVM_GUEST_MODE_HOST_HV	4
>> +#define KVM_GUEST_MODE_GUEST_HV_FAST	4 /* ISA v3.0 with host radix mode */
>> +#define KVM_GUEST_MODE_HOST_HV	5
>>   
>>   #define KVM_INST_FETCH_FAILED	-1
>>   
>> diff --git a/arch/powerpc/include/asm/kvm_book3s_64.h b/arch/powerpc/include/asm/kvm_book3s_64.h
>> index 9bb9bb370b53..c214bcffb441 100644
>> --- a/arch/powerpc/include/asm/kvm_book3s_64.h
>> +++ b/arch/powerpc/include/asm/kvm_book3s_64.h
>> @@ -153,9 +153,17 @@ static inline bool kvmhv_vcpu_is_radix(struct kvm_vcpu *vcpu)
>>   	return radix;
>>   }
>>   
>> +int __kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu);
>> +
>>   #define KVM_DEFAULT_HPT_ORDER	24	/* 16MB HPT by default */
>>   #endif
>>   
>> +/*
>> + * Invalid HDSISR value which is used to indicate when HW has not set the reg.
>> + * Used to work around an errata.
>> + */
>> +#define HDSISR_CANARY	0x7fff
>> +
>>   /*
>>    * We use a lock bit in HPTE dword 0 to synchronize updates and
>>    * accesses to each HPTE, and another bit to indicate non-present
>> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
>> index 05fb00d37609..fa0083345b11 100644
>> --- a/arch/powerpc/include/asm/kvm_host.h
>> +++ b/arch/powerpc/include/asm/kvm_host.h
>> @@ -690,7 +690,12 @@ struct kvm_vcpu_arch {
>>   	ulong fault_dar;
>>   	u32 fault_dsisr;
>>   	unsigned long intr_msr;
>> -	ulong fault_gpa;	/* guest real address of page fault (POWER9) */
>> +	/*
>> +	 * POWER9 and later, fault_gpa contains the guest real address of page
>> +	 * fault for a radix guest, or segment descriptor (equivalent to result
>> +	 * from slbmfev of SLB entry that translated the EA) for hash guests.
>> +	 */
>> +	ulong fault_gpa;
>>   #endif
>>   
>>   #ifdef CONFIG_BOOKE
>> diff --git a/arch/powerpc/kernel/security.c b/arch/powerpc/kernel/security.c
>> index e4e1a94ccf6a..3a607c11f20f 100644
>> --- a/arch/powerpc/kernel/security.c
>> +++ b/arch/powerpc/kernel/security.c
>> @@ -430,16 +430,19 @@ device_initcall(stf_barrier_debugfs_init);
>>   
>>   static void update_branch_cache_flush(void)
>>   {
>> -	u32 *site;
>> +	u32 *site, __maybe_unused *site2;
>>   
>>   #ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
>>   	site = &patch__call_kvm_flush_link_stack;
>> +	site2 = &patch__call_kvm_flush_link_stack_p9;
>>   	// This controls the branch from guest_exit_cont to kvm_flush_link_stack
>>   	if (link_stack_flush_type == BRANCH_CACHE_FLUSH_NONE) {
>>   		patch_instruction_site(site, ppc_inst(PPC_INST_NOP));
>> +		patch_instruction_site(site2, ppc_inst(PPC_INST_NOP));
>>   	} else {
>>   		// Could use HW flush, but that could also flush count cache
>>   		patch_branch_site(site, (u64)&kvm_flush_link_stack, BRANCH_SET_LINK);
>> +		patch_branch_site(site2, (u64)&kvm_flush_link_stack, BRANCH_SET_LINK);
>>   	}
>>   #endif
>>   
>> diff --git a/arch/powerpc/kvm/Makefile b/arch/powerpc/kvm/Makefile
>> index cdd119028f64..ca7c86aa9360 100644
>> --- a/arch/powerpc/kvm/Makefile
>> +++ b/arch/powerpc/kvm/Makefile
>> @@ -88,6 +88,7 @@ kvm-book3s_64-builtin-tm-objs-$(CONFIG_PPC_TRANSACTIONAL_MEM) += \
>>   
>>   ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
>>   kvm-book3s_64-builtin-objs-$(CONFIG_KVM_BOOK3S_64_HANDLER) += \
>> +	book3s_hv_interrupt.o \
>>   	book3s_hv_hmi.o \
>>   	book3s_hv_rmhandlers.o \
>>   	book3s_hv_rm_mmu.o \
>> diff --git a/arch/powerpc/kvm/book3s_64_entry.S b/arch/powerpc/kvm/book3s_64_entry.S
>> index 0c79c89c6a4b..d98ad580fd98 100644
>> --- a/arch/powerpc/kvm/book3s_64_entry.S
>> +++ b/arch/powerpc/kvm/book3s_64_entry.S
>> @@ -1,11 +1,16 @@
>>   /* SPDX-License-Identifier: GPL-2.0-only */
>>   #include <asm/asm-offsets.h>
>>   #include <asm/cache.h>
>> +#include <asm/code-patching-asm.h>
>>   #include <asm/exception-64s.h>
>> +#include <asm/export.h>
>>   #include <asm/kvm_asm.h>
>>   #include <asm/kvm_book3s_asm.h>
>> +#include <asm/mmu.h>
>>   #include <asm/ppc_asm.h>
>> +#include <asm/ptrace.h>
>>   #include <asm/reg.h>
>> +#include <asm/ultravisor-api.h>
>>   
>>   /*
>>    * These are branched to from interrupt handlers in exception-64s.S which set
>> @@ -29,10 +34,15 @@
>>   .global	kvmppc_hcall
>>   .balign IFETCH_ALIGN_BYTES
>>   kvmppc_hcall:
>> +	lbz	r10,HSTATE_IN_GUEST(r13)
>> +	cmpwi	r10,KVM_GUEST_MODE_GUEST_HV_FAST
>> +	beq	kvmppc_p9_exit_hcall
>>   	ld	r10,PACA_EXGEN+EX_R13(r13)
>>   	SET_SCRATCH0(r10)
>>   	li	r10,0xc00
>>   	/* Now we look like kvmppc_interrupt */
>> +	li	r11,PACA_EXGEN
>> +	b	1f
>>   
>>   /*
>>    * KVM interrupt entry occurs after GEN_INT_ENTRY runs, and follows that
>> @@ -53,6 +63,12 @@ kvmppc_hcall:
>>   .global	kvmppc_interrupt
>>   .balign IFETCH_ALIGN_BYTES
>>   kvmppc_interrupt:
>> +	std	r10,HSTATE_SCRATCH0(r13)
>> +	lbz	r10,HSTATE_IN_GUEST(r13)
>> +	cmpwi	r10,KVM_GUEST_MODE_GUEST_HV_FAST
>> +	beq	kvmppc_p9_exit_interrupt
>> +	ld	r10,HSTATE_SCRATCH0(r13)
>> +	lbz	r11,HSTATE_IN_GUEST(r13)
>>   	li	r11,PACA_EXGEN
>>   	cmpdi	r10,0x200
>>   	bgt+	1f
>> @@ -154,3 +170,234 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>>   	GET_SCRATCH0(r13)
>>   	HRFI_TO_KERNEL
>>   #endif
>> +
>> +/* Stack frame offsets for kvmppc_hv_entry */
>> +#define SFS			(144 + STACK_FRAME_MIN_SIZE)
>> +#define STACK_SLOT_NVGPRS	(SFS - 144)	/* 18 gprs */
>> +
>> +/*
>> + * void kvmppc_p9_enter_guest(struct vcpu *vcpu);
>> + *
>> + * Enter the guest on a ISAv3.0 or later system where we have exactly
>> + * one vcpu per vcore, and both the host and guest are radix, and threads
>> + * are set to "indepdent mode".
>> + */
>> +.balign	IFETCH_ALIGN_BYTES
>> +_GLOBAL(kvmppc_p9_enter_guest)
>> +EXPORT_SYMBOL_GPL(kvmppc_p9_enter_guest)
>> +	mflr	r0
>> +	std	r0,PPC_LR_STKOFF(r1)
>> +	stdu	r1,-SFS(r1)
>> +
>> +	std	r1,HSTATE_HOST_R1(r13)
>> +
>> +	mfcr	r4
>> +	stw	r4,SFS+8(r1)
>> +
>> +	reg = 14
>> +	.rept	18
>> +	std	reg,STACK_SLOT_NVGPRS + ((reg - 14) * 8)(r1)
>> +	reg = reg + 1
>> +	.endr
>> +
>> +	ld	r4,VCPU_LR(r3)
>> +	mtlr	r4
>> +	ld	r4,VCPU_CTR(r3)
>> +	mtctr	r4
>> +	ld	r4,VCPU_XER(r3)
>> +	mtspr	SPRN_XER,r4
>> +
>> +	ld	r1,VCPU_CR(r3)
>> +
>> +BEGIN_FTR_SECTION
>> +	ld	r4,VCPU_CFAR(r3)
>> +	mtspr	SPRN_CFAR,r4
>> +END_FTR_SECTION_IFSET(CPU_FTR_CFAR)
>> +BEGIN_FTR_SECTION
>> +	ld	r4,VCPU_PPR(r3)
>> +	mtspr	SPRN_PPR,r4
>> +END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>> +
>> +	reg = 4
>> +	.rept	28
>> +	ld	reg,__VCPU_GPR(reg)(r3)
>> +	reg = reg + 1
>> +	.endr
>> +
>> +	ld	r4,VCPU_KVM(r3)
>> +	lbz	r4,KVM_SECURE_GUEST(r4)
> 
> 
> This does not compile when CONFIG_KVM_BOOK3S_HV_POSSIBLE is not defined.

Thanks, I fixed that. I admittedly haven't tested all such combinations 
well, I'll have to script some more comprehensive compile jobs.

Thanks,
Nick

^ permalink raw reply

* Re: [PATCH v6 32/48] KVM: PPC: Book3S HV P9: Read machine check registers while MSR[RI] is 0
From: Nicholas Piggin @ 2021-04-10  0:39 UTC (permalink / raw)
  To: Alexey Kardashevskiy, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <0adc89d0-c765-d11b-ffe4-cbbf2f8f9c49@ozlabs.ru>

Excerpts from Alexey Kardashevskiy's message of April 9, 2021 6:55 pm:
> 
> 
> On 05/04/2021 11:19, Nicholas Piggin wrote:
>> SRR0/1, DAR, DSISR must all be protected from machine check which can
>> clobber them. Ensure MSR[RI] is clear while they are live.
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>   arch/powerpc/kvm/book3s_hv.c           | 11 +++++++--
>>   arch/powerpc/kvm/book3s_hv_interrupt.c | 33 +++++++++++++++++++++++---
>>   arch/powerpc/kvm/book3s_hv_ras.c       |  2 ++
>>   3 files changed, 41 insertions(+), 5 deletions(-)
>> 
>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>> index d6eecedaa5a5..5f0ac6567a06 100644
>> --- a/arch/powerpc/kvm/book3s_hv.c
>> +++ b/arch/powerpc/kvm/book3s_hv.c
>> @@ -3567,11 +3567,16 @@ static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
>>   	mtspr(SPRN_BESCR, vcpu->arch.bescr);
>>   	mtspr(SPRN_WORT, vcpu->arch.wort);
>>   	mtspr(SPRN_TIDR, vcpu->arch.tid);
>> -	mtspr(SPRN_DAR, vcpu->arch.shregs.dar);
>> -	mtspr(SPRN_DSISR, vcpu->arch.shregs.dsisr);
>>   	mtspr(SPRN_AMR, vcpu->arch.amr);
>>   	mtspr(SPRN_UAMOR, vcpu->arch.uamor);
>>   
>> +	/*
>> +	 * DAR, DSISR, and for nested HV, SPRGs must be set with MSR[RI]
>> +	 * clear (or hstate set appropriately to catch those registers
>> +	 * being clobbered if we take a MCE or SRESET), so those are done
>> +	 * later.
>> +	 */
>> +
>>   	if (!(vcpu->arch.ctrl & 1))
>>   		mtspr(SPRN_CTRLT, mfspr(SPRN_CTRLF) & ~1);
>>   
>> @@ -3614,6 +3619,8 @@ static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
>>   			hvregs.vcpu_token = vcpu->vcpu_id;
>>   		}
>>   		hvregs.hdec_expiry = time_limit;
>> +		mtspr(SPRN_DAR, vcpu->arch.shregs.dar);
>> +		mtspr(SPRN_DSISR, vcpu->arch.shregs.dsisr);
>>   		trap = plpar_hcall_norets(H_ENTER_NESTED, __pa(&hvregs),
>>   					  __pa(&vcpu->arch.regs));
>>   		kvmhv_restore_hv_return_state(vcpu, &hvregs);
>> diff --git a/arch/powerpc/kvm/book3s_hv_interrupt.c b/arch/powerpc/kvm/book3s_hv_interrupt.c
>> index 6fdd93936e16..e93d2a6456ff 100644
>> --- a/arch/powerpc/kvm/book3s_hv_interrupt.c
>> +++ b/arch/powerpc/kvm/book3s_hv_interrupt.c
>> @@ -132,6 +132,7 @@ int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpc
>>   	s64 hdec;
>>   	u64 tb, purr, spurr;
>>   	u64 *exsave;
>> +	bool ri_set;
>>   	unsigned long msr = mfmsr();
>>   	int trap;
>>   	unsigned long host_hfscr = mfspr(SPRN_HFSCR);
>> @@ -203,9 +204,6 @@ int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpc
>>   	 */
>>   	mtspr(SPRN_HDEC, hdec);
>>   
>> -	mtspr(SPRN_SRR0, vcpu->arch.shregs.srr0);
>> -	mtspr(SPRN_SRR1, vcpu->arch.shregs.srr1);
>> -
>>   	start_timing(vcpu, &vcpu->arch.rm_entry);
>>   
>>   	vcpu->arch.ceded = 0;
>> @@ -231,6 +229,13 @@ int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpc
>>   	 */
>>   	mtspr(SPRN_HDSISR, HDSISR_CANARY);
>>   
>> +	__mtmsrd(0, 1); /* clear RI */
>> +
>> +	mtspr(SPRN_DAR, vcpu->arch.shregs.dar);
>> +	mtspr(SPRN_DSISR, vcpu->arch.shregs.dsisr);
>> +	mtspr(SPRN_SRR0, vcpu->arch.shregs.srr0);
>> +	mtspr(SPRN_SRR1, vcpu->arch.shregs.srr1);
>> +
>>   	accumulate_time(vcpu, &vcpu->arch.guest_time);
>>   
>>   	local_paca->kvm_hstate.in_guest = KVM_GUEST_MODE_GUEST_HV_FAST;
>> @@ -248,7 +253,13 @@ int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpc
>>   
>>   	/* 0x2 bit for HSRR is only used by PR and P7/8 HV paths, clear it */
>>   	trap = local_paca->kvm_hstate.scratch0 & ~0x2;
>> +
>> +	/* HSRR interrupts leave MSR[RI] unchanged, SRR interrupts clear it. */
>> +	ri_set = false;
>>   	if (likely(trap > BOOK3S_INTERRUPT_MACHINE_CHECK)) {
>> +		if (trap != BOOK3S_INTERRUPT_SYSCALL &&
>> +				(vcpu->arch.shregs.msr & MSR_RI))
>> +			ri_set = true;
>>   		exsave = local_paca->exgen;
>>   	} else if (trap == BOOK3S_INTERRUPT_SYSTEM_RESET) {
>>   		exsave = local_paca->exnmi;
>> @@ -258,6 +269,22 @@ int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpc
>>   
>>   	vcpu->arch.regs.gpr[1] = local_paca->kvm_hstate.scratch1;
>>   	vcpu->arch.regs.gpr[3] = local_paca->kvm_hstate.scratch2;
>> +
>> +	/*
>> +	 * Only set RI after reading machine check regs (DAR, DSISR, SRR0/1)
>> +	 * and hstate scratch (which we need to move into exsave to make
>> +	 * re-entrant vs SRESET/MCE)
>> +	 */
>> +	if (ri_set) {
>> +		if (unlikely(!(mfmsr() & MSR_RI))) {
>> +			__mtmsrd(MSR_RI, 1);
>> +			WARN_ON_ONCE(1);
>> +		}
>> +	} else {
>> +		WARN_ON_ONCE(mfmsr() & MSR_RI);
>> +		__mtmsrd(MSR_RI, 1);
>> +	}
>> +
>>   	vcpu->arch.regs.gpr[9] = exsave[EX_R9/sizeof(u64)];
>>   	vcpu->arch.regs.gpr[10] = exsave[EX_R10/sizeof(u64)];
>>   	vcpu->arch.regs.gpr[11] = exsave[EX_R11/sizeof(u64)];
>> diff --git a/arch/powerpc/kvm/book3s_hv_ras.c b/arch/powerpc/kvm/book3s_hv_ras.c
>> index d4bca93b79f6..8d8a4d5f0b55 100644
>> --- a/arch/powerpc/kvm/book3s_hv_ras.c
>> +++ b/arch/powerpc/kvm/book3s_hv_ras.c
>> @@ -199,6 +199,8 @@ static void kvmppc_tb_resync_done(void)
>>    * know about the exact state of the TB value. Resync TB call will
>>    * restore TB to host timebase.
>>    *
>> + * This could use the new OPAL_HANDLE_HMI2 to avoid resyncing TB every time.
> 
> 
> Educating myself - is it because OPAL_HANDLE_HMI2 tells if it is TB/TOD 
> which is the problem so we can avoid calling opal_resync_timebase() if 
> it is not TB?

Yes.

> OPAL_HANDLE_HMI2 does not seem to resync TB itself. The 
> comment just does not seem related to the rest of the patch.

Yeah it's not related, I'll take it out.

> 
> Otherwise, looks good.
> 
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Thanks,
Nick

^ permalink raw reply

* Bogus struct page layout on 32-bit
From: Matthew Wilcox @ 2021-04-10  2:43 UTC (permalink / raw)
  To: kernel test robot
  Cc: kbuild-all, clang-built-linux, Jesper Dangaard Brouer,
	linux-kernel, linux-mm, Paul Mackerras, linux-fsdevel,
	linuxppc-dev, David S. Miller, linux-arm-kernel
In-Reply-To: <202104100656.N7EVvkNZ-lkp@intel.com>

On Sat, Apr 10, 2021 at 06:45:35AM +0800, kernel test robot wrote:
> >> include/linux/mm_types.h:274:1: error: static_assert failed due to requirement '__builtin_offsetof(struct page, lru) == __builtin_offsetof(struct folio, lru)' "offsetof(struct page, lru) == offsetof(struct folio, lru)"
>    FOLIO_MATCH(lru, lru);
>    include/linux/mm_types.h:272:2: note: expanded from macro 'FOLIO_MATCH'
>            static_assert(offsetof(struct page, pg) == offsetof(struct folio, fl))

Well, this is interesting.  pahole reports:

struct page {
        long unsigned int          flags;                /*     0     4 */
        /* XXX 4 bytes hole, try to pack */
        union {
                struct {
                        struct list_head lru;            /*     8     8 */
...
struct folio {
        union {
                struct {
                        long unsigned int flags;         /*     0     4 */
                        struct list_head lru;            /*     4     8 */

so this assert has absolutely done its job.

But why has this assert triggered?  Why is struct page layout not what
we thought it was?  Turns out it's the dma_addr added in 2019 by commit
c25fff7171be ("mm: add dma_addr_t to struct page").  On this particular
config, it's 64-bit, and ppc32 requires alignment to 64-bit.  So
the whole union gets moved out by 4 bytes.

Unfortunately, we can't just fix this by putting an 'unsigned long pad'
in front of it.  It still aligns the entire union to 8 bytes, and then
it skips another 4 bytes after the pad.

We can fix it like this ...

+++ b/include/linux/mm_types.h
@@ -96,11 +96,12 @@ struct page {
                        unsigned long private;
                };
                struct {        /* page_pool used by netstack */
+                       unsigned long _page_pool_pad;
                        /**
                         * @dma_addr: might require a 64-bit value even on
                         * 32-bit architectures.
                         */
-                       dma_addr_t dma_addr;
+                       dma_addr_t dma_addr __packed;
                };
                struct {        /* slab, slob and slub */
                        union {

but I don't know if GCC is smart enough to realise that dma_addr is now
on an 8 byte boundary and it can use a normal instruction to access it,
or whether it'll do something daft like use byte loads to access it.

We could also do:

+                       dma_addr_t dma_addr __packed __aligned(sizeof(void *));

and I see pahole, at least sees this correctly:

                struct {
                        long unsigned int _page_pool_pad; /*     4     4 */
                        dma_addr_t dma_addr __attribute__((__aligned__(4))); /*     8     8 */
                } __attribute__((__packed__)) __attribute__((__aligned__(4)));  

This presumably affects any 32-bit architecture with a 64-bit phys_addr_t
/ dma_addr_t.  Advice, please?

^ permalink raw reply

* Re: Bogus struct page layout on 32-bit
From: Jesper Dangaard Brouer @ 2021-04-10  6:21 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Ilias Apalodimas, kbuild-all, kernel test robot, Ivan Khoronzhuk,
	clang-built-linux, brouer, linux-kernel, linux-mm,
	netdev@vger.kernel.org, Paul Mackerras, linux-fsdevel,
	Matteo Croce, linuxppc-dev, David S. Miller, linux-arm-kernel
In-Reply-To: <20210410024313.GX2531743@casper.infradead.org>

On Sat, 10 Apr 2021 03:43:13 +0100
Matthew Wilcox <willy@infradead.org> wrote:

> On Sat, Apr 10, 2021 at 06:45:35AM +0800, kernel test robot wrote:
> > >> include/linux/mm_types.h:274:1: error: static_assert failed due to requirement '__builtin_offsetof(struct page, lru) == __builtin_offsetof(struct folio, lru)' "offsetof(struct page, lru) == offsetof(struct folio, lru)"  
> >    FOLIO_MATCH(lru, lru);
> >    include/linux/mm_types.h:272:2: note: expanded from macro 'FOLIO_MATCH'
> >            static_assert(offsetof(struct page, pg) == offsetof(struct folio, fl))  
> 
> Well, this is interesting.  pahole reports:
> 
> struct page {
>         long unsigned int          flags;                /*     0     4 */
>         /* XXX 4 bytes hole, try to pack */
>         union {
>                 struct {
>                         struct list_head lru;            /*     8     8 */
> ...
> struct folio {
>         union {
>                 struct {
>                         long unsigned int flags;         /*     0     4 */
>                         struct list_head lru;            /*     4     8 */
> 
> so this assert has absolutely done its job.
> 
> But why has this assert triggered?  Why is struct page layout not what
> we thought it was?  Turns out it's the dma_addr added in 2019 by commit
> c25fff7171be ("mm: add dma_addr_t to struct page").  On this particular
> config, it's 64-bit, and ppc32 requires alignment to 64-bit.  So
> the whole union gets moved out by 4 bytes.

Argh, good that you are catching this!

> Unfortunately, we can't just fix this by putting an 'unsigned long pad'
> in front of it.  It still aligns the entire union to 8 bytes, and then
> it skips another 4 bytes after the pad.
> 
> We can fix it like this ...
> 
> +++ b/include/linux/mm_types.h
> @@ -96,11 +96,12 @@ struct page {
>                         unsigned long private;
>                 };
>                 struct {        /* page_pool used by netstack */
> +                       unsigned long _page_pool_pad;

I'm fine with this pad.  Matteo is currently proposing[1] to add a 32-bit
value after @dma_addr, and he could use this area instead.

[1] https://lore.kernel.org/netdev/20210409223801.104657-3-mcroce@linux.microsoft.com/

When adding/changing this, we need to make sure that it doesn't overlap
member @index, because network stack use/check page_is_pfmemalloc().
As far as my calculations this is safe to add.  I always try to keep an
eye out for this, but I wonder if we could have a build check like yours.


>                         /**
>                          * @dma_addr: might require a 64-bit value even on
>                          * 32-bit architectures.
>                          */
> -                       dma_addr_t dma_addr;
> +                       dma_addr_t dma_addr __packed;
>                 };
>                 struct {        /* slab, slob and slub */
>                         union {
> 
> but I don't know if GCC is smart enough to realise that dma_addr is now
> on an 8 byte boundary and it can use a normal instruction to access it,
> or whether it'll do something daft like use byte loads to access it.
> 
> We could also do:
> 
> +                       dma_addr_t dma_addr __packed __aligned(sizeof(void *));
> 
> and I see pahole, at least sees this correctly:
> 
>                 struct {
>                         long unsigned int _page_pool_pad; /*     4     4 */
>                         dma_addr_t dma_addr __attribute__((__aligned__(4))); /*     8     8 */
>                 } __attribute__((__packed__)) __attribute__((__aligned__(4)));  
> 
> This presumably affects any 32-bit architecture with a 64-bit phys_addr_t
> / dma_addr_t.  Advice, please?

I'm not sure that the 32-bit behavior is with 64-bit (dma) addrs.

I don't have any 32-bit boards with 64-bit DMA.  Cc. Ivan, wasn't your
board (572x ?) 32-bit with driver 'cpsw' this case (where Ivan added
XDP+page_pool) ?

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer


^ permalink raw reply

* Re: [PATCH v3] powerpc/traps: Enhance readability for trap types
From: Xiongwei Song @ 2021-04-10  8:31 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: pmladek, peterz, linux-kernel, paulus, kan.liang, mikey, maddy,
	haren, aik, kjain, ravi.bangoria, john.ogness, alistair, npiggin,
	jniethe5, atrajeev, Xiongwei Song, oleg, aneesh.kumar, akpm,
	linuxppc-dev, rppt
In-Reply-To: <70ece993-12bd-335c-d246-914564eb51dd@csgroup.eu>


> On Apr 10, 2021, at 12:14 AM, Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> 
> 
> 
> Le 08/04/2021 à 16:07, Xiongwei Song a écrit :
>> From: Xiongwei Song <sxwjean@gmail.com>
>> Create a new header named traps.h, define macros to list ppc interrupt
>> types in traps.h, replace the reference of the trap hex values with these
>> macros.
>> Referred the hex number in arch/powerpc/kernel/exceptions-64e.S,
>> arch/powerpc/kernel/exceptions-64s.S and
>> arch/powerpc/include/asm/kvm_asm.h.
>> v2-v3:
>> Correct the prefix of trap macros with INTERRUPT_, the previous prefix
>> is TRAP_, which is not precise. This is suggested by Segher Boessenkool
>> and Nicholas Piggin.
>> v1-v2:
>> Define more trap macros to replace more trap hexs in code, not just for
>> the __show_regs function. This is suggested by Christophe Leroy.
>> Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
>> ---
>>  arch/powerpc/include/asm/interrupt.h  |  9 +++++---
>>  arch/powerpc/include/asm/ptrace.h     |  3 ++-
>>  arch/powerpc/include/asm/traps.h      | 32 +++++++++++++++++++++++++++
>>  arch/powerpc/kernel/interrupt.c       |  3 ++-
>>  arch/powerpc/kernel/process.c         |  5 ++++-
>>  arch/powerpc/mm/book3s64/hash_utils.c |  5 +++--
>>  arch/powerpc/mm/fault.c               | 21 +++++++++++-------
>>  arch/powerpc/perf/core-book3s.c       |  5 +++--
>>  arch/powerpc/xmon/xmon.c              | 16 +++++++++++---
>>  9 files changed, 78 insertions(+), 21 deletions(-)
>>  create mode 100644 arch/powerpc/include/asm/traps.h
>> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
>> index 7c633896d758..5ce9898bc9a6 100644
>> --- a/arch/powerpc/include/asm/interrupt.h
>> +++ b/arch/powerpc/include/asm/interrupt.h
>> @@ -8,6 +8,7 @@
>>  #include <asm/ftrace.h>
>>  #include <asm/kprobes.h>
>>  #include <asm/runlatch.h>
>> +#include <asm/traps.h>
>>    struct interrupt_state {
>>  #ifdef CONFIG_PPC_BOOK3E_64
>> @@ -59,7 +60,7 @@ static inline void interrupt_enter_prepare(struct pt_regs *regs, struct interrup
>>  		 * CT_WARN_ON comes here via program_check_exception,
>>  		 * so avoid recursion.
>>  		 */
>> -		if (TRAP(regs) != 0x700)
>> +		if (TRAP(regs) != INTERRUPT_PROGRAM)
>>  			CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
>>  	}
>>  #endif
>> @@ -156,7 +157,8 @@ static inline void interrupt_nmi_enter_prepare(struct pt_regs *regs, struct inte
>>  	/* Don't do any per-CPU operations until interrupt state is fixed */
>>  #endif
>>  	/* Allow DEC and PMI to be traced when they are soft-NMI */
>> -	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260) {
>> +	if (TRAP(regs) != INTERRUPT_DECREMENTER &&
>> +	    TRAP(regs) != INTERRUPT_PERFMON) {
> 
> I think too long names hinder readability, see later for suggestions.
> 
>>  		state->ftrace_enabled = this_cpu_get_ftrace_enabled();
>>  		this_cpu_set_ftrace_enabled(0);
>>  	}
>> @@ -180,7 +182,8 @@ static inline void interrupt_nmi_exit_prepare(struct pt_regs *regs, struct inter
>>  		nmi_exit();
>>    #ifdef CONFIG_PPC64
>> -	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260)
>> +	if (TRAP(regs) != INTERRUPT_DECREMENTER &&
>> +	    TRAP(regs) != INTERRUPT_PERFMON)
>>  		this_cpu_set_ftrace_enabled(state->ftrace_enabled);
>>    #ifdef CONFIG_PPC_BOOK3S_64
>> diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
>> index f10498e1b3f6..7a17e0365d43 100644
>> --- a/arch/powerpc/include/asm/ptrace.h
>> +++ b/arch/powerpc/include/asm/ptrace.h
>> @@ -21,6 +21,7 @@
>>    #include <uapi/asm/ptrace.h>
>>  #include <asm/asm-const.h>
>> +#include <asm/traps.h>
>>    #ifndef __ASSEMBLY__
>>  struct pt_regs
>> @@ -237,7 +238,7 @@ static inline bool trap_is_unsupported_scv(struct pt_regs *regs)
>>    static inline bool trap_is_syscall(struct pt_regs *regs)
>>  {
>> -	return (trap_is_scv(regs) || TRAP(regs) == 0xc00);
>> +	return (trap_is_scv(regs) || TRAP(regs) == INTERRUPT_SYSCALL);
>>  }
>>    static inline bool trap_norestart(struct pt_regs *regs)
>> diff --git a/arch/powerpc/include/asm/traps.h b/arch/powerpc/include/asm/traps.h
>> new file mode 100644
>> index 000000000000..cb416a17097c
>> --- /dev/null
>> +++ b/arch/powerpc/include/asm/traps.h
>> @@ -0,0 +1,32 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _ASM_PPC_TRAPS_H
>> +#define _ASM_PPC_TRAPS_H
>> +
>> +#if defined(CONFIG_BOOKE) || defined(CONFIG_4xx)
>> +#define INTERRUPT_MACHINE_CHECK   0x000
> 
> I'd prefer shorted names in order to not be obliged to split lines.
> Here are some suggestions:
> 
> INT_MCE
> 
>> +#define INTERRUPT_CRITICAL_INPUT  0x100
> 
> INT_CRIT
> 
>> +#define INTERRUPT_ALTIVEC_UNAVAIL 0x200
>> +#define INTERRUPT_PERFMON         0x260
> 
> INT_PERF
> 
>> +#define INTERRUPT_DOORBELL        0x280
>> +#define INTERRUPT_DEBUG           0xd00
>> +#elif defined(CONFIG_PPC_BOOK3S)
>> +#define INTERRUPT_SYSTEM_RESET    0x100
> 
> INT_SRESET
> 
>> +#define INTERRUPT_MACHINE_CHECK   0x200
> 
> INT_MCE
> 
>> +#define INTERRUPT_DATA_SEGMENT    0x380
> 
> INT_DSEG
> 
>> +#define INTERRUPT_INST_SEGMENT    0x480
> 
> INT_ISEG
> 
>> +#define INTERRUPT_DOORBELL        0xa00
> 
> INT_DBELL
> 
>> +#define INTERRUPT_TRACE           0xd00
> 
> INT_TRACE
> 
>> +#define INTERRUPT_H_DATA_STORAGE  0xe00
>> +#define INTERRUPT_PERFMON         0xf00
> 
> INT_PERF
> 
>> +#define INTERRUPT_H_FAC_UNAVAIL   0xf80
>> +#endif
>> +
>> +#define INTERRUPT_DATA_STORAGE    0x300
> 
> INT_DSI
> 
>> +#define INTERRUPT_INST_STORAGE    0x400
> 
> INT_ISI
> 
>> +#define INTERRUPT_ALIGNMENT       0x600
> 
> INT_ALIGN
> 
>> +#define INTERRUPT_PROGRAM         0x700
> 
> INT_PROG
> 
>> +#define INTERRUPT_FP_UNAVAIL      0x800
> 
> INT_FP_UNAVAIL
> 
>> +#define INTERRUPT_DECREMENTER     0x900
> 
> INT_DEC
> 
>> +#define INTERRUPT_SYSCALL         0xc00
> 
> INT_SYSCALL
> 
> 
>> +
>> +#endif /* _ASM_PPC_TRAPS_H */
> 
> ...
> 
>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>> index 0c0b1c2cfb49..641b3feef7ee 100644
>> --- a/arch/powerpc/mm/fault.c
>> +++ b/arch/powerpc/mm/fault.c
>> @@ -44,6 +44,7 @@
>>  #include <asm/debug.h>
>>  #include <asm/kup.h>
>>  #include <asm/inst.h>
>> +#include <asm/traps.h>
>>      /*
>> @@ -197,7 +198,7 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr,
>>  static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
>>  			     unsigned long address, bool is_write)
>>  {
>> -	int is_exec = TRAP(regs) == 0x400;
>> +	int is_exec = TRAP(regs) == INTERRUPT_INST_STORAGE;
>>    	/* NX faults set DSISR_PROTFAULT on the 8xx, DSISR_NOEXEC_OR_G on others */
>>  	if (is_exec && (error_code & (DSISR_NOEXEC_OR_G | DSISR_KEYFAULT |
>> @@ -391,7 +392,7 @@ static int ___do_page_fault(struct pt_regs *regs, unsigned long address,
>>  	struct vm_area_struct * vma;
>>  	struct mm_struct *mm = current->mm;
>>  	unsigned int flags = FAULT_FLAG_DEFAULT;
>> - 	int is_exec = TRAP(regs) == 0x400;
>> +	int is_exec = TRAP(regs) == INTERRUPT_INST_STORAGE;
>>  	int is_user = user_mode(regs);
>>  	int is_write = page_fault_is_write(error_code);
>>  	vm_fault_t fault, major = 0;
>> @@ -588,20 +589,24 @@ void __bad_page_fault(struct pt_regs *regs, int sig)
>>  	/* kernel has accessed a bad area */
>>    	switch (TRAP(regs)) {
>> -	case 0x300:
>> -	case 0x380:
>> -	case 0xe00:
>> +	case INTERRUPT_DATA_STORAGE:
>> +#ifdef CONFIG_PPC_BOOK3S
>> +	case INTERRUPT_DATA_SEGMENT:
>> +	case INTERRUPT_H_DATA_STORAGE:
>> +#endif
> 
> It would be better to avoid #ifdefs when none where necessary before.
> 
> 
>>  		pr_alert("BUG: %s on %s at 0x%08lx\n",
>>  			 regs->dar < PAGE_SIZE ? "Kernel NULL pointer dereference" :
>>  			 "Unable to handle kernel data access",
>>  			 is_write ? "write" : "read", regs->dar);
>>  		break;
>> -	case 0x400:
>> -	case 0x480:
>> +	case INTERRUPT_INST_STORAGE:
>> +#ifdef CONFIG_PPC_BOOK3S
>> +	case INTERRUPT_INST_SEGMENT:
>> +#endif
> 
> It would be better to avoid #ifdefs when none where necessary before.
> 
Good point. Will delete them.

Regards,
Xiongwei

> 
> 
>>  		pr_alert("BUG: Unable to handle kernel instruction fetch%s",
>>  			 regs->nip < PAGE_SIZE ? " (NULL pointer?)\n" : "\n");
>>  		break;
>> -	case 0x600:
>> +	case INTERRUPT_ALIGNMENT:
>>  		pr_alert("BUG: Unable to handle kernel unaligned access at 0x%08lx\n",
>>  			 regs->dar);
>>  		break;
>> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
>> index 766f064f00fb..6e34f5bba232 100644
>> --- a/arch/powerpc/perf/core-book3s.c
>> +++ b/arch/powerpc/perf/core-book3s.c
>> @@ -17,6 +17,7 @@
>>  #include <asm/firmware.h>
>>  #include <asm/ptrace.h>
>>  #include <asm/code-patching.h>
>> +#include <asm/traps.h>
>>    #ifdef CONFIG_PPC64
>>  #include "internal.h"
>> @@ -168,7 +169,7 @@ static bool regs_use_siar(struct pt_regs *regs)
>>  	 * they have not been setup using perf_read_regs() and so regs->result
>>  	 * is something random.
>>  	 */
>> -	return ((TRAP(regs) == 0xf00) && regs->result);
>> +	return ((TRAP(regs) == INTERRUPT_PERFMON) && regs->result);
>>  }
>>    /*
>> @@ -347,7 +348,7 @@ static inline void perf_read_regs(struct pt_regs *regs)
>>  	 * hypervisor samples as well as samples in the kernel with
>>  	 * interrupts off hence the userspace check.
>>  	 */
>> -	if (TRAP(regs) != 0xf00)
>> +	if (TRAP(regs) != INTERRUPT_PERFMON)
>>  		use_siar = 0;
>>  	else if ((ppmu->flags & PPMU_NO_SIAR))
>>  		use_siar = 0;
>> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
>> index bf7d69625a2e..2a4f99e64bf3 100644
>> --- a/arch/powerpc/xmon/xmon.c
>> +++ b/arch/powerpc/xmon/xmon.c
>> @@ -54,6 +54,7 @@
>>  #include <asm/code-patching.h>
>>  #include <asm/sections.h>
>>  #include <asm/inst.h>
>> +#include <asm/traps.h>
>>    #ifdef CONFIG_PPC64
>>  #include <asm/hvcall.h>
>> @@ -1769,7 +1770,12 @@ static void excprint(struct pt_regs *fp)
>>  	printf("    sp: %lx\n", fp->gpr[1]);
>>  	printf("   msr: %lx\n", fp->msr);
>>  -	if (trap == 0x300 || trap == 0x380 || trap == 0x600 || trap == 0x200) {
>> +	if (trap == INTERRUPT_DATA_STORAGE ||
>> +#ifdef CONFIG_PPC_BOOK3S
>> +	    trap == INTERRUPT_DATA_SEGMENT ||
>> +#endif
> It would be better to avoid #ifdefs when none where necessary before.
> 
> And an #ifdef in the middle of a code line is awful for readability and maintainability.
> 
>> +	    trap == INTERRUPT_ALIGNMENT ||
>> +	    trap == INTERRUPT_MACHINE_CHECK) {
>>  		printf("   dar: %lx\n", fp->dar);
>>  		if (trap != 0x380)
>>  			printf(" dsisr: %lx\n", fp->dsisr);
>> @@ -1785,7 +1791,7 @@ static void excprint(struct pt_regs *fp)
>>  		       current->pid, current->comm);
>>  	}
>>  -	if (trap == 0x700)
>> +	if (trap == INTERRUPT_PROGRAM)
>>  		print_bug_trap(fp);
>>    	printf(linux_banner);
>> @@ -1846,7 +1852,11 @@ static void prregs(struct pt_regs *fp)
>>  	printf("ctr = "REG"   xer = "REG"   trap = %4lx\n",
>>  	       fp->ctr, fp->xer, fp->trap);
>>  	trap = TRAP(fp);
>> -	if (trap == 0x300 || trap == 0x380 || trap == 0x600)
>> +	if (trap == INTERRUPT_DATA_STORAGE ||
>> +#ifdef CONFIG_PPC_BOOK3S
>> +	    trap == INTERRUPT_DATA_SEGMENT ||
>> +#endif
>> +	    trap == INTERRUPT_ALIGNMENT)
> 
> It would be better to avoid #ifdefs when none where necessary before.
> 
> And an #ifdef in the middle of a code line is awful for readability and maintainability.
> 
> 
>>  		printf("dar = "REG"   dsisr = %.8lx\n", fp->dar, fp->dsisr);
>>  }
>>  


^ permalink raw reply

* Re: [PATCH v3] powerpc/traps: Enhance readability for trap types
From: Xiongwei Song @ 2021-04-10  8:33 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: pmladek, peterz, linux-kernel, paulus, kan.liang, mikey, maddy,
	aneesh.kumar, haren, aik, kjain, ravi.bangoria, john.ogness,
	alistair, npiggin, jniethe5, atrajeev, Xiongwei Song, oleg, akpm,
	linuxppc-dev, rppt
In-Reply-To: <874kgfdmxq.fsf@mpe.ellerman.id.au>


> On Apr 10, 2021, at 8:04 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> 
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> Le 08/04/2021 à 16:07, Xiongwei Song a écrit :
>>> From: Xiongwei Song <sxwjean@gmail.com>
>>> 
>>> Create a new header named traps.h, define macros to list ppc interrupt
>>> types in traps.h, replace the reference of the trap hex values with these
>>> macros.
> ...
>>> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
>>> index 7c633896d758..5ce9898bc9a6 100644
>>> --- a/arch/powerpc/include/asm/interrupt.h
>>> +++ b/arch/powerpc/include/asm/interrupt.h
>>> @@ -8,6 +8,7 @@
>>>  #include <asm/ftrace.h>
>>>  #include <asm/kprobes.h>
>>>  #include <asm/runlatch.h>
>>> +#include <asm/traps.h>
>>> 
>>>  struct interrupt_state {
>>>  #ifdef CONFIG_PPC_BOOK3E_64
>>> @@ -59,7 +60,7 @@ static inline void interrupt_enter_prepare(struct pt_regs *regs, struct interrup
>>>  		 * CT_WARN_ON comes here via program_check_exception,
>>>  		 * so avoid recursion.
>>>  		 */
>>> -		if (TRAP(regs) != 0x700)
>>> +		if (TRAP(regs) != INTERRUPT_PROGRAM)
>>>  			CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
>>>  	}
>>>  #endif
>>> @@ -156,7 +157,8 @@ static inline void interrupt_nmi_enter_prepare(struct pt_regs *regs, struct inte
>>>  	/* Don't do any per-CPU operations until interrupt state is fixed */
>>>  #endif
>>>  	/* Allow DEC and PMI to be traced when they are soft-NMI */
>>> -	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260) {
>>> +	if (TRAP(regs) != INTERRUPT_DECREMENTER &&
>>> +	    TRAP(regs) != INTERRUPT_PERFMON) {
>> 
>> I think too long names hinder readability, see later for suggestions.
> 
> I asked for the longer names :)
> 
> I think they make it easier for people who are less familiar with the
> architecture than us to make sense of the names.
> 
> And there's only a couple of cases where it requires splitting a line,
> and they could be converted to use switch if we think it's a problem.
> 
>>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>>> index 0c0b1c2cfb49..641b3feef7ee 100644
>>> --- a/arch/powerpc/mm/fault.c
>>> +++ b/arch/powerpc/mm/fault.c
>>> @@ -588,20 +589,24 @@ void __bad_page_fault(struct pt_regs *regs, int sig)
>>>  	/* kernel has accessed a bad area */
>>> 
>>>  	switch (TRAP(regs)) {
>>> -	case 0x300:
>>> -	case 0x380:
>>> -	case 0xe00:
>>> +	case INTERRUPT_DATA_STORAGE:
>>> +#ifdef CONFIG_PPC_BOOK3S
>>> +	case INTERRUPT_DATA_SEGMENT:
>>> +	case INTERRUPT_H_DATA_STORAGE:
>>> +#endif
>> 
>> It would be better to avoid #ifdefs when none where necessary before.
> 
> Yes I agree.
> 
> I think these can all be avoided by defining most of the values
> regardless of what platform we're building for. Only the values that
> overlap need to be kept behind an ifdef.

Ok. 

Regards,
Xiongwei

> 
> cheers


^ permalink raw reply

* Re: [PATCH v4] powerpc/traps: Enhance readability for trap types
From: Xiongwei Song @ 2021-04-10  9:32 UTC (permalink / raw)
  To: Nicholas Piggin
  Cc: ravi.bangoria, aik, linux-kernel, paulus, kan.liang, mikey, maddy,
	haren, peterz, kjain, pmladek, john.ogness, alistair, jniethe5,
	atrajeev, Xiongwei Song, oleg, aneesh.kumar, akpm, linuxppc-dev,
	rppt
In-Reply-To: <1618014353.jyyolglj7u.astroid@bobo.none>


> On Apr 10, 2021, at 8:35 AM, Nicholas Piggin <npiggin@gmail.com> wrote:
> 
> Thanks for working on this, I think it's a nice cleanup and helps
> non-powerpc people understand the code a bit better.
> 

My pleasure.

> Excerpts from Xiongwei Song's message of April 10, 2021 12:28 am:
>> From: Xiongwei Song <sxwjean@gmail.com>
>> 
>> Create a new header named traps.h, define macros to list ppc interrupt
>> types in traps.h, replace the references of the trap hex values with these
>> macros.
>> 
>> Referred the hex numbers in arch/powerpc/kernel/exceptions-64e.S,
>> arch/powerpc/kernel/exceptions-64s.S and
>> arch/powerpc/include/asm/kvm_asm.h.
>> 
>> Reported-by: kernel test robot <lkp@intel.com>
> 
> It now looks like lkp asked for this whole cleanup patch. I would
> put [kernel test robot <lkp@intel.com>] in your v3->4 changelog
> item.
> 

Agree. I just forgot to delete this line in the patch.

>> Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
>> ---
>> 
>> v3-v4:
>> Fix compile issue:
>> arch/powerpc/kernel/process.c:1473:14: error: 'INTERRUPT_MACHINE_CHECK' undeclared (first use in this function); did you mean 'TAINT_MACHINE_CHECK'?
>> I didn't add "Reported-by: kernel test robot <lkp@intel.com>" here,
>> because it's improper for this patch.
> 
> [...]
> 
>> diff --git a/arch/powerpc/include/asm/traps.h b/arch/powerpc/include/asm/traps.h
>> new file mode 100644
>> index 000000000000..2e64e10afcef
>> --- /dev/null
>> +++ b/arch/powerpc/include/asm/traps.h
>> @@ -0,0 +1,32 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _ASM_PPC_TRAPS_H
>> +#define _ASM_PPC_TRAPS_H
> 
> These could go in interrupt.h.
> 
>> +#if defined(CONFIG_BOOKE) || defined(CONFIG_4xx)
>> +#define INTERRUPT_MACHINE_CHECK   0x000
>> +#define INTERRUPT_CRITICAL_INPUT  0x100
>> +#define INTERRUPT_ALTIVEC_UNAVAIL 0x200
>> +#define INTERRUPT_PERFMON         0x260
>> +#define INTERRUPT_DOORBELL        0x280
>> +#define INTERRUPT_DEBUG           0xd00
>> +#else
>> +#define INTERRUPT_SYSTEM_RESET    0x100
>> +#define INTERRUPT_MACHINE_CHECK   0x200
> 
> [...]
> 
>> @@ -1469,7 +1470,9 @@ static void __show_regs(struct pt_regs *regs)
>> 	trap = TRAP(regs);
>> 	if (!trap_is_syscall(regs) && cpu_has_feature(CPU_FTR_CFAR))
>> 		pr_cont("CFAR: "REG" ", regs->orig_gpr3);
>> -	if (trap == 0x200 || trap == 0x300 || trap == 0x600) {
>> +	if (trap == INTERRUPT_MACHINE_CHECK ||
>> +	    trap == INTERRUPT_DATA_STORAGE ||
>> +	    trap == INTERRUPT_ALIGNMENT) {
>> 		if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
>> 			pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->dsisr);
>> 		else
> 
> This is now a change in behaviour because previously BOOKE/4xx tested
> 0x200, but now it tests 0.

Yes. Previously BOOKE/4xx tested 0x200, but checked this line history, please see
the commit below:

commit c54006491dde7d1b8050c5542716b751be92ed80
Author: Anton Blanchard <anton@samba.org>
Date:   Fri Nov 15 15:41:19 2013 +1100

    powerpc: Print DAR and DSISR on machine check oopses
    
    Machine check exceptions set DAR and DSISR, so print them in our
    oops output.
    
    Signed-off-by: Anton Blanchard <anton@samba.org>
    Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 75c2d1009985..37c4103a8cff 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -864,7 +864,7 @@ void show_regs(struct pt_regs * regs)
        trap = TRAP(regs);
        if ((regs->trap != 0xc00) && cpu_has_feature(CPU_FTR_CFAR))
                printk("CFAR: "REG"\n", regs->orig_gpr3);
-       if (trap == 0x300 || trap == 0x600)
+       if (trap == 0x200 || trap == 0x300 || trap == 0x600)
 #if defined(CONFIG_4xx) || defined(CONFIG_BOOKE)
                printk("DEAR: "REG", ESR: "REG"\n", regs->dar, regs->dsisr);
 #else

0x200 aims to test Machine check, but for 64e, the regs->trap should be 0x000 here,
under the commit comments, I changed the code behavior. Sorry I didn’t add 
comments. 

> 
> That looks wrong for 4xx. 64e does put 0x000 there but I wonder if it 
> should use 0x200 instead.

Ok. Thanks for pointing this out, let me learn about 4xx. 

> Bit difficult to test this stuff, I do have
> some MCE injection patches for QEMU for 64s, might be able to look at
> porting them to 64e although I have no idea about booke machine checks.
> 

Yes, would appreciate your sharing.

> Anyway I don't think this patch should change generated code at all.
> Either change the code first with smaller patches, or make sure you
> keep the tests the same.

Agree.

Regards,
Xiongwei

> 
> Thanks,
> Nick


^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox