* [PATCH v2 02/11] powerpc/64e/interrupt: always save nvgprs on interrupt
From: Nicholas Piggin @ 2021-03-16 10:41 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Scott Wood, Nicholas Piggin
In-Reply-To: <20210316104206.407354-1-npiggin@gmail.com>
In order to use the C interrupt return, nvgprs must always be saved.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/ptrace.h | 9 +--------
arch/powerpc/kernel/entry_64.S | 13 -------------
arch/powerpc/kernel/exceptions-64e.S | 27 +++------------------------
3 files changed, 4 insertions(+), 45 deletions(-)
diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
index 1aca5fe79285..c5b3669918f4 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -186,18 +186,11 @@ static inline void regs_set_return_value(struct pt_regs *regs, unsigned long rc)
((struct pt_regs *)((unsigned long)task_stack_page(current) + THREAD_SIZE) - 1)
#ifdef __powerpc64__
-#ifdef CONFIG_PPC_BOOK3S
#define TRAP_FLAGS_MASK 0x10
#define TRAP(regs) ((regs)->trap & ~TRAP_FLAGS_MASK)
#define FULL_REGS(regs) true
#define SET_FULL_REGS(regs) do { } while (0)
-#else
-#define TRAP_FLAGS_MASK 0x11
-#define TRAP(regs) ((regs)->trap & ~TRAP_FLAGS_MASK)
-#define FULL_REGS(regs) (((regs)->trap & 1) == 0)
-#define SET_FULL_REGS(regs) ((regs)->trap &= ~1)
-#endif
-#define CHECK_FULL_REGS(regs) BUG_ON(!FULL_REGS(regs))
+#define CHECK_FULL_REGS(regs) do { } while (0)
#define NV_REG_POISON 0xdeadbeefdeadbeefUL
#else
/*
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 6c4d9e276c4d..853534b2ae2e 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -417,19 +417,6 @@ _GLOBAL(ret_from_kernel_thread)
li r3,0
b .Lsyscall_exit
-#ifdef CONFIG_PPC_BOOK3E
-/* Save non-volatile GPRs, if not already saved. */
-_GLOBAL(save_nvgprs)
- ld r11,_TRAP(r1)
- andi. r0,r11,1
- beqlr-
- SAVE_NVGPRS(r1)
- clrrdi r0,r11,1
- std r0,_TRAP(r1)
- blr
-_ASM_NOKPROBE_SYMBOL(save_nvgprs);
-#endif
-
#ifdef CONFIG_PPC_BOOK3S_64
#define FLUSH_COUNT_CACHE \
diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
index e8eb9992a270..a7d9ce9f7fdb 100644
--- a/arch/powerpc/kernel/exceptions-64e.S
+++ b/arch/powerpc/kernel/exceptions-64e.S
@@ -417,14 +417,15 @@ exc_##n##_common: \
std r6,_LINK(r1); \
std r7,_CTR(r1); \
std r8,_XER(r1); \
- li r3,(n)+1; /* indicate partial regs in trap */ \
+ li r3,(n); /* regs.trap vector */ \
std r9,0(r1); /* store stack frame back link */ \
std r10,_CCR(r1); /* store orig CR in stackframe */ \
std r9,GPR1(r1); /* store stack frame back link */ \
std r11,SOFTE(r1); /* and save it to stackframe */ \
std r12,STACK_FRAME_OVERHEAD-16(r1); /* mark the frame */ \
std r3,_TRAP(r1); /* set trap number */ \
- std r0,RESULT(r1); /* clear regs->result */
+ std r0,RESULT(r1); /* clear regs->result */ \
+ SAVE_NVGPRS(r1);
#define EXCEPTION_COMMON(n) \
EXCEPTION_COMMON_LVL(n, SPRN_SPRG_GEN_SCRATCH, PACA_EXGEN)
@@ -561,7 +562,6 @@ __end_interrupts:
CRIT_EXCEPTION_PROLOG(0x100, BOOKE_INTERRUPT_CRITICAL,
PROLOG_ADDITION_NONE)
EXCEPTION_COMMON_CRIT(0x100)
- bl save_nvgprs
bl special_reg_save
CHECK_NAPPING();
addi r3,r1,STACK_FRAME_OVERHEAD
@@ -573,7 +573,6 @@ __end_interrupts:
MC_EXCEPTION_PROLOG(0x000, BOOKE_INTERRUPT_MACHINE_CHECK,
PROLOG_ADDITION_NONE)
EXCEPTION_COMMON_MC(0x000)
- bl save_nvgprs
bl special_reg_save
CHECK_NAPPING();
addi r3,r1,STACK_FRAME_OVERHEAD
@@ -623,7 +622,6 @@ __end_interrupts:
std r14,_DSISR(r1)
addi r3,r1,STACK_FRAME_OVERHEAD
ld r14,PACA_EXGEN+EX_R14(r13)
- bl save_nvgprs
bl program_check_exception
b ret_from_except
@@ -639,7 +637,6 @@ __end_interrupts:
bl load_up_fpu
b fast_exception_return
1: INTS_DISABLE
- bl save_nvgprs
addi r3,r1,STACK_FRAME_OVERHEAD
bl kernel_fp_unavailable_exception
b ret_from_except
@@ -661,7 +658,6 @@ BEGIN_FTR_SECTION
END_FTR_SECTION_IFSET(CPU_FTR_ALTIVEC)
#endif
INTS_DISABLE
- bl save_nvgprs
addi r3,r1,STACK_FRAME_OVERHEAD
bl altivec_unavailable_exception
b ret_from_except
@@ -673,7 +669,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_ALTIVEC)
PROLOG_ADDITION_NONE)
EXCEPTION_COMMON(0x220)
INTS_DISABLE
- bl save_nvgprs
addi r3,r1,STACK_FRAME_OVERHEAD
#ifdef CONFIG_ALTIVEC
BEGIN_FTR_SECTION
@@ -698,7 +693,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_ALTIVEC)
CRIT_EXCEPTION_PROLOG(0x9f0, BOOKE_INTERRUPT_WATCHDOG,
PROLOG_ADDITION_NONE)
EXCEPTION_COMMON_CRIT(0x9f0)
- bl save_nvgprs
bl special_reg_save
CHECK_NAPPING();
addi r3,r1,STACK_FRAME_OVERHEAD
@@ -723,7 +717,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_ALTIVEC)
PROLOG_ADDITION_NONE)
EXCEPTION_COMMON(0xf20)
INTS_DISABLE
- bl save_nvgprs
addi r3,r1,STACK_FRAME_OVERHEAD
bl unknown_exception
b ret_from_except
@@ -792,7 +785,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_ALTIVEC)
addi r3,r1,STACK_FRAME_OVERHEAD
ld r14,PACA_EXCRIT+EX_R14(r13)
ld r15,PACA_EXCRIT+EX_R15(r13)
- bl save_nvgprs
bl DebugException
b ret_from_except
@@ -864,7 +856,6 @@ kernel_dbg_exc:
addi r3,r1,STACK_FRAME_OVERHEAD
ld r14,PACA_EXDBG+EX_R14(r13)
ld r15,PACA_EXDBG+EX_R15(r13)
- bl save_nvgprs
bl DebugException
b ret_from_except
@@ -887,7 +878,6 @@ kernel_dbg_exc:
CRIT_EXCEPTION_PROLOG(0x2a0, BOOKE_INTERRUPT_DOORBELL_CRITICAL,
PROLOG_ADDITION_NONE)
EXCEPTION_COMMON_CRIT(0x2a0)
- bl save_nvgprs
bl special_reg_save
CHECK_NAPPING();
addi r3,r1,STACK_FRAME_OVERHEAD
@@ -903,7 +893,6 @@ kernel_dbg_exc:
PROLOG_ADDITION_NONE)
EXCEPTION_COMMON(0x2c0)
addi r3,r1,STACK_FRAME_OVERHEAD
- bl save_nvgprs
INTS_RESTORE_HARD
bl unknown_exception
b ret_from_except
@@ -913,7 +902,6 @@ kernel_dbg_exc:
CRIT_EXCEPTION_PROLOG(0x2e0, BOOKE_INTERRUPT_GUEST_DBELL_CRIT,
PROLOG_ADDITION_NONE)
EXCEPTION_COMMON_CRIT(0x2e0)
- bl save_nvgprs
bl special_reg_save
CHECK_NAPPING();
addi r3,r1,STACK_FRAME_OVERHEAD
@@ -926,7 +914,6 @@ kernel_dbg_exc:
PROLOG_ADDITION_NONE)
EXCEPTION_COMMON(0x310)
addi r3,r1,STACK_FRAME_OVERHEAD
- bl save_nvgprs
INTS_RESTORE_HARD
bl unknown_exception
b ret_from_except
@@ -937,7 +924,6 @@ kernel_dbg_exc:
PROLOG_ADDITION_NONE)
EXCEPTION_COMMON(0x320)
addi r3,r1,STACK_FRAME_OVERHEAD
- bl save_nvgprs
INTS_RESTORE_HARD
bl unknown_exception
b ret_from_except
@@ -948,7 +934,6 @@ kernel_dbg_exc:
PROLOG_ADDITION_NONE)
EXCEPTION_COMMON(0x340)
addi r3,r1,STACK_FRAME_OVERHEAD
- bl save_nvgprs
INTS_RESTORE_HARD
bl unknown_exception
b ret_from_except
@@ -1014,7 +999,6 @@ storage_fault_common:
cmpdi r3,0
bne- 1f
b ret_from_except_lite
-1: bl save_nvgprs
mr r4,r3
addi r3,r1,STACK_FRAME_OVERHEAD
bl __bad_page_fault
@@ -1030,16 +1014,12 @@ alignment_more:
addi r3,r1,STACK_FRAME_OVERHEAD
ld r14,PACA_EXGEN+EX_R14(r13)
ld r15,PACA_EXGEN+EX_R15(r13)
- bl save_nvgprs
INTS_RESTORE_HARD
bl alignment_exception
b ret_from_except
.align 7
_GLOBAL(ret_from_except)
- ld r11,_TRAP(r1)
- andi. r0,r11,1
- bne ret_from_except_lite
REST_NVGPRS(r1)
_GLOBAL(ret_from_except_lite)
@@ -1080,7 +1060,6 @@ _GLOBAL(ret_from_except_lite)
SCHEDULE_USER
b ret_from_except_lite
2:
- bl save_nvgprs
/*
* Use a non volatile GPR to save and restore our thread_info flags
* across the call to restore_interrupts.
--
2.23.0
^ permalink raw reply related
* [PATCH v2 01/11] powerpc/syscall: switch user_exit_irqoff and trace_hardirqs_off order
From: Nicholas Piggin @ 2021-03-16 10:41 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Scott Wood, Nicholas Piggin
In-Reply-To: <20210316104206.407354-1-npiggin@gmail.com>
user_exit_irqoff() -> __context_tracking_exit -> vtime_user_exit
warns in __seqprop_assert due to lockdep thinking preemption is enabled
because trace_hardirqs_off() has not yet been called.
Switch the order of these two calls, which matches their ordering in
interrupt_enter_prepare.
Fixes: 5f0b6ac3905f ("powerpc/64/syscall: Reconcile interrupts")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/interrupt.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
index c4dd4b8f9cfa..fbabb49888d3 100644
--- a/arch/powerpc/kernel/interrupt.c
+++ b/arch/powerpc/kernel/interrupt.c
@@ -43,11 +43,11 @@ notrace long system_call_exception(long r3, long r4, long r5,
if (IS_ENABLED(CONFIG_PPC_IRQ_SOFT_MASK_DEBUG))
BUG_ON(irq_soft_mask_return() != IRQS_ALL_DISABLED);
+ trace_hardirqs_off(); /* finish reconciling */
+
CT_WARN_ON(ct_state() == CONTEXT_KERNEL);
user_exit_irqoff();
- trace_hardirqs_off(); /* finish reconciling */
-
if (!IS_ENABLED(CONFIG_BOOKE) && !IS_ENABLED(CONFIG_40x))
BUG_ON(!(regs->msr & MSR_RI));
BUG_ON(!(regs->msr & MSR_PR));
--
2.23.0
^ permalink raw reply related
* [PATCH v2 00/11] Move 64e to new interrupt return code
From: Nicholas Piggin @ 2021-03-16 10:41 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Scott Wood, Nicholas Piggin
Since RFC:
- Rebased on Christophe's v3 ppc32 conversion
- Fixed up small details, adds some powerpc-wide cleanups at the end.
Since v1:
- Christophe's review comments accounted for:
- Split common code interrupt changes for 64e from 64e conversion.
- Removed unnecessary ifdef additions from common interrupt code.
- Keep interrupt return context tracking debug check for 64e when
it switches to new context tracing.
- __bad_page_fault is made static in page fault cleanup.
- CPU time accounting asm and asm offsets can be removed with 64e
conversion.
Tested on qemu only (QEMU e500), which is not ideal for interrupt
handling particularly the critical interrupts which I don't know
whether it can generate.
Thanks,
Nick
Nicholas Piggin (11):
powerpc/syscall: switch user_exit_irqoff and trace_hardirqs_off order
powerpc/64e/interrupt: always save nvgprs on interrupt
powerpc/interrupt: update common interrupt code for 64e
powerpc/64e/interrupt: use new interrupt return
powerpc/64e/interrupt: NMI save irq soft-mask state in C
powerpc/64e/interrupt: reconcile irq soft-mask state in C
powerpc/64e/interrupt: Use new interrupt context tracking scheme
powerpc/64e/interrupt: handle bad_page_fault in C
powerpc: clean up do_page_fault
powerpc: remove partial register save logic
powerpc: move norestart trap flag to bit 0
arch/powerpc/include/asm/asm-prototypes.h | 2 -
arch/powerpc/include/asm/bug.h | 5 +-
arch/powerpc/include/asm/interrupt.h | 66 ++--
arch/powerpc/include/asm/ppc_asm.h | 20 -
arch/powerpc/include/asm/ptrace.h | 39 +-
arch/powerpc/kernel/align.c | 6 -
arch/powerpc/kernel/asm-offsets.c | 10 -
arch/powerpc/kernel/entry_64.S | 40 +-
arch/powerpc/kernel/exceptions-64e.S | 425 ++--------------------
arch/powerpc/kernel/interrupt.c | 26 +-
arch/powerpc/kernel/irq.c | 76 ----
arch/powerpc/kernel/process.c | 12 -
arch/powerpc/kernel/ptrace/ptrace-view.c | 21 --
arch/powerpc/kernel/ptrace/ptrace.c | 2 -
arch/powerpc/kernel/ptrace/ptrace32.c | 4 -
arch/powerpc/kernel/signal_32.c | 3 -
arch/powerpc/kernel/signal_64.c | 2 -
arch/powerpc/kernel/traps.c | 14 +-
arch/powerpc/lib/sstep.c | 4 -
arch/powerpc/mm/book3s64/hash_utils.c | 16 +-
arch/powerpc/mm/fault.c | 30 +-
arch/powerpc/xmon/xmon.c | 23 +-
22 files changed, 126 insertions(+), 720 deletions(-)
--
2.23.0
^ permalink raw reply
* [PATCH] ASoC: fsl_spdif: use snd_ctl_boolean_mono_info
From: Shengjiu Wang @ 2021-03-16 9:42 UTC (permalink / raw)
To: timur, nicoleotsuka, Xiubo.Lee, festevam, lgirdwood, broonie,
perex, tiwai, alsa-devel, linuxppc-dev, linux-kernel
From: Viorel Suman <viorel.suman@nxp.com>
Remove redundant code and use snd_ctl_boolean_mono_info
instead.
Signed-off-by: Viorel Suman <viorel.suman@nxp.com>
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
sound/soc/fsl/fsl_spdif.c | 30 +++---------------------------
1 file changed, 3 insertions(+), 27 deletions(-)
diff --git a/sound/soc/fsl/fsl_spdif.c b/sound/soc/fsl/fsl_spdif.c
index 174e558224d8..1cd3441d1c03 100644
--- a/sound/soc/fsl/fsl_spdif.c
+++ b/sound/soc/fsl/fsl_spdif.c
@@ -801,18 +801,6 @@ static int fsl_spdif_qget(struct snd_kcontrol *kcontrol,
return ret;
}
-/* Valid bit information */
-static int fsl_spdif_vbit_info(struct snd_kcontrol *kcontrol,
- struct snd_ctl_elem_info *uinfo)
-{
- uinfo->type = SNDRV_CTL_ELEM_TYPE_BOOLEAN;
- uinfo->count = 1;
- uinfo->value.integer.min = 0;
- uinfo->value.integer.max = 1;
-
- return 0;
-}
-
/* Get valid good bit from interrupt status register */
static int fsl_spdif_rx_vbit_get(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)
@@ -925,18 +913,6 @@ static int fsl_spdif_rxrate_get(struct snd_kcontrol *kcontrol,
return 0;
}
-/* User bit sync mode info */
-static int fsl_spdif_usync_info(struct snd_kcontrol *kcontrol,
- struct snd_ctl_elem_info *uinfo)
-{
- uinfo->type = SNDRV_CTL_ELEM_TYPE_BOOLEAN;
- uinfo->count = 1;
- uinfo->value.integer.min = 0;
- uinfo->value.integer.max = 1;
-
- return 0;
-}
-
/*
* User bit sync mode:
* 1 CD User channel subcode
@@ -1018,7 +994,7 @@ static struct snd_kcontrol_new fsl_spdif_ctrls[] = {
.name = "IEC958 RX V-Bit Errors",
.access = SNDRV_CTL_ELEM_ACCESS_READ |
SNDRV_CTL_ELEM_ACCESS_VOLATILE,
- .info = fsl_spdif_vbit_info,
+ .info = snd_ctl_boolean_mono_info,
.get = fsl_spdif_rx_vbit_get,
},
{
@@ -1027,7 +1003,7 @@ static struct snd_kcontrol_new fsl_spdif_ctrls[] = {
.access = SNDRV_CTL_ELEM_ACCESS_READ |
SNDRV_CTL_ELEM_ACCESS_WRITE |
SNDRV_CTL_ELEM_ACCESS_VOLATILE,
- .info = fsl_spdif_vbit_info,
+ .info = snd_ctl_boolean_mono_info,
.get = fsl_spdif_tx_vbit_get,
.put = fsl_spdif_tx_vbit_put,
},
@@ -1047,7 +1023,7 @@ static struct snd_kcontrol_new fsl_spdif_ctrls[] = {
.access = SNDRV_CTL_ELEM_ACCESS_READ |
SNDRV_CTL_ELEM_ACCESS_WRITE |
SNDRV_CTL_ELEM_ACCESS_VOLATILE,
- .info = fsl_spdif_usync_info,
+ .info = snd_ctl_boolean_mono_info,
.get = fsl_spdif_usync_get,
.put = fsl_spdif_usync_put,
},
--
2.27.0
^ permalink raw reply related
* [PATCH] ASoC: fsl_sai: remove reset code from dai_probe
From: Shengjiu Wang @ 2021-03-16 9:27 UTC (permalink / raw)
To: timur, nicoleotsuka, Xiubo.Lee, festevam, lgirdwood, broonie,
perex, tiwai, alsa-devel, linuxppc-dev, linux-kernel
From: Viorel Suman <viorel.suman@nxp.com>
SAI software reset is done in runtime resume,
there is no need to do it in fsl_sai_dai_probe.
Signed-off-by: Viorel Suman <viorel.suman@nxp.com>
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
sound/soc/fsl/fsl_sai.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
index 6ef2ce348232..9e7893f91882 100644
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -711,13 +711,6 @@ static int fsl_sai_dai_probe(struct snd_soc_dai *cpu_dai)
struct fsl_sai *sai = dev_get_drvdata(cpu_dai->dev);
unsigned int ofs = sai->soc_data->reg_offset;
- /* Software Reset for both Tx and Rx */
- regmap_write(sai->regmap, FSL_SAI_TCSR(ofs), FSL_SAI_CSR_SR);
- regmap_write(sai->regmap, FSL_SAI_RCSR(ofs), FSL_SAI_CSR_SR);
- /* Clear SR bit to finish the reset */
- regmap_write(sai->regmap, FSL_SAI_TCSR(ofs), 0);
- regmap_write(sai->regmap, FSL_SAI_RCSR(ofs), 0);
-
regmap_update_bits(sai->regmap, FSL_SAI_TCR1(ofs),
FSL_SAI_CR1_RFW_MASK(sai->soc_data->fifo_depth),
sai->soc_data->fifo_depth - FSL_SAI_MAXBURST_TX);
--
2.27.0
^ permalink raw reply related
* RE: [PATCH] powerpc/vdso32: Add missing _restgpr_31_x to fix build failure
From: David Laight @ 2021-03-16 9:35 UTC (permalink / raw)
To: 'Segher Boessenkool'
Cc: Paul Mackerras, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org, 'Rasmus Villemoes'
In-Reply-To: <20210315235947.GD16691@gate.crashing.org>
From: Segher Boessenkool
> Sent: 16 March 2021 00:00
...
> > Although you may need to disable loop unrolling (often dubious at best)
> > and either force or disable some function inlining.
>
> The cases where GCC does loop unrolling at -O2 always help quite a lot.
> Or, do you have a counter-example? We'd love to see one.
The real problem with loop unrolling is that quite often a modern
out-of-order superscaler processor actually has 'spare' execution
cycles where the loop control can be done 'for free'.
Sometimes you do need to unroll (or interleave) a couple of
times to get enough spare execution cycles.
But the unrolled loop has to read a lot more code into cache
- so unless the code is 'hot cache' (that is usually arranged
for benchmarking) those delays apply as well.
The larger code footprint also displaces other code.
My real annoyance with gcc is unrolling (and vectorizing)
loops that I know are never executed as many times as even one
copy of the unrolled loop.
As an example intel (ivy bridge onwards) cpu execute the
following code (the middle of the ip checksum) at 8 bytes/clock.
(Limited by the carry flag.)
It just doesn't need any further unrolling.
+ "10: jecxz 20f\n"
+ " adc (%[buff], %[len]), %[sum_0]\n"
+ " adc 8(%[buff], %[len]), %[sum_1]\n"
+ " lea 32(%[len]), %[len_tmp]\n"
+ " adc 16(%[buff], %[len]), %[sum_0]\n"
+ " adc 24(%[buff], %[len]), %[sum_1]\n"
+ " mov %[len_tmp], %[len]\n"
+ " jmp 10b\n"
Annoyingly that loop is slow on my 8-core atom.
The existing code only does 4 bytes/clock on intel cpu prior
to either broadwell or haswell (forgotten which) in spite
of much more unroling.
> And yup, inlining is hard. GCC's heuristics there are very good
> nowadays, but any single decision has big effects. Doing the important
> spots manually (always_inline or noinline) has good payoff.
Latest inline gripe was a function replicated about 20 times
when the non-inline version was a register load and 'tail call'.
The inlining is just bloat.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH 03/10] powerpc/64e/interrupt: use new interrupt return
From: Nicholas Piggin @ 2021-03-16 8:14 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: Scott Wood
In-Reply-To: <f452729c-6290-17e9-eb96-4c1ebffd4c7e@csgroup.eu>
Excerpts from Christophe Leroy's message of March 16, 2021 5:29 pm:
>
>
> Le 16/03/2021 à 08:25, Nicholas Piggin a écrit :
>> Excerpts from Nicholas Piggin's message of March 16, 2021 5:04 pm:
>>> Excerpts from Christophe Leroy's message of March 15, 2021 11:30 pm:
>>>>
>>>>
>>>> Le 15/03/2021 à 04:17, Nicholas Piggin a écrit :
>>>>> Update the new C and asm interrupt return code to account for 64e
>>>>> specifics, switch over to use it.
>>>>>
>>>>> The now-unused old ret_from_except code, that was moved to 64e after the
>>>>> 64s conversion, is removed.
>>>>>
>>>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>>> ---
>>>>> arch/powerpc/include/asm/asm-prototypes.h | 2 -
>>>>> arch/powerpc/kernel/entry_64.S | 9 +-
>>>>> arch/powerpc/kernel/exceptions-64e.S | 321 ++--------------------
>>>>> arch/powerpc/kernel/interrupt.c | 27 +-
>>>>> arch/powerpc/kernel/irq.c | 76 -----
>>>>> 5 files changed, 56 insertions(+), 379 deletions(-)
>>>>>
>>>>> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
>>>>> index fbabb49888d3..ae7b058b2970 100644
>>>>> --- a/arch/powerpc/kernel/interrupt.c
>>>>> +++ b/arch/powerpc/kernel/interrupt.c
>>>>> @@ -235,6 +235,10 @@ static notrace void booke_load_dbcr0(void)
>>>>> #endif
>>>>> }
>>>>>
>>>>> +/* temporary hack for context tracking, removed in later patch */
>>>>> +#include <linux/sched/debug.h>
>>>>> +asmlinkage __visible void __sched schedule_user(void);
>>>>> +
>>>>> /*
>>>>> * This should be called after a syscall returns, with r3 the return value
>>>>> * from the syscall. If this function returns non-zero, the system call
>>>>> @@ -292,7 +296,11 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
>>>>> while (unlikely(ti_flags & (_TIF_USER_WORK_MASK & ~_TIF_RESTORE_TM))) {
>>>>> local_irq_enable();
>>>>> if (ti_flags & _TIF_NEED_RESCHED) {
>>>>> +#ifdef CONFIG_PPC_BOOK3E_64
>>>>> + schedule_user();
>>>>> +#else
>>>>> schedule();
>>>>> +#endif
>>>>> } else {
>>>>> /*
>>>>> * SIGPENDING must restore signal handler function
>>>>> @@ -360,7 +368,6 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
>>>>> return ret;
>>>>> }
>>>>>
>>>>> -#ifndef CONFIG_PPC_BOOK3E_64 /* BOOK3E not yet using this */
>>>>> notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned long msr)
>>>>> {
>>>>> unsigned long ti_flags;
>>>>> @@ -372,7 +379,9 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned
>>>>> BUG_ON(!(regs->msr & MSR_PR));
>>>>> BUG_ON(!FULL_REGS(regs));
>>>>> BUG_ON(arch_irq_disabled_regs(regs));
>>>>> +#ifdef CONFIG_PPC_BOOK3S_64
>>>>
>>>> Shouldn't this go away in patch 6 as well ?
>>>> Or is that needed at all ? In syscall_exit_prepare() it is not ifdefed .
>>>
>>> Hmm, not sure. I'll take a look. It probably shouldn't be ifdefed at all
>>> but definitely by the end it should run without warning.
>>
>> Oh I got confused and thought that was the syscall exit. Interrupt exit
>> has to keep this until patch 6 because 64e context tracking does
>> everything in interrupt wrappers, so by the time we get here it will
>> already be set to CONTEXT_USER.
>>
>
> ok, but that it has to go in patch 6 ? At the time being the #ifdef is still there at the end of the
> series
>
Yes, that was an oversight. I'll remove it.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH 12/14] powerpc/64s: system call avoid setting MSR[RI] until we set MSR[EE]
From: Nicholas Piggin @ 2021-03-16 8:13 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
In-Reply-To: <722aeb8d-507f-6702-dd79-26242f987e3e@csgroup.eu>
Excerpts from Christophe Leroy's message of March 16, 2021 5:21 pm:
>
>
> Le 15/03/2021 à 23:04, Nicholas Piggin a écrit :
>> This extends the MSR[RI]=0 window a little further into the system
>> call in order to pair RI and EE enabling with a single mtmsrd.
>
> Time ago, I proposed to delay that on PPC32 and Michael objected, see
> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/9f9dd859d571e324c7412ed9db9da8cfba678257.1548956511.git.christophe.leroy@c-s.fr/
Yeah, it is a concern. The speedup should be at least 5% I think on
64s (have not measured in isolation yet), so might be worth it.
We'll see.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH] powerpc/64s: power4 nap fixup in C
From: Nicholas Piggin @ 2021-03-16 8:11 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
In-Reply-To: <6ab2073b-dd10-ef2c-375d-1300f071ae1a@csgroup.eu>
Excerpts from Christophe Leroy's message of March 16, 2021 5:16 pm:
>
>
> Le 12/03/2021 à 02:20, Nicholas Piggin a écrit :
>> There is no need for this to be in asm, use the new intrrupt entry wrapper.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> Hopefully this works on a real G5 now, but I couldn't reproduce the
>> problem with QEMU.
>>
>> Thanks,
>> Nick
>>
>> arch/powerpc/include/asm/interrupt.h | 19 +++++++++++
>> arch/powerpc/include/asm/processor.h | 1 +
>> arch/powerpc/include/asm/thread_info.h | 6 ++++
>> arch/powerpc/kernel/exceptions-64s.S | 45 --------------------------
>> arch/powerpc/kernel/idle_book3s.S | 4 +++
>> 5 files changed, 30 insertions(+), 45 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
>> index aedfba29e43a..ef015d3b5e39 100644
>> --- a/arch/powerpc/include/asm/interrupt.h
>> +++ b/arch/powerpc/include/asm/interrupt.h
>> @@ -9,6 +9,17 @@
>> #include <asm/kprobes.h>
>> #include <asm/runlatch.h>
>>
>> +static inline void nap_adjust_return(struct pt_regs *regs)
>> +{
>> +#ifdef CONFIG_PPC_970_NAP
>> + if (unlikely(test_thread_local_flags(_TLF_NAPPING))) {
>> + /* Can avoid a test-and-clear because NMIs do not call this */
>> + clear_thread_local_flags(_TLF_NAPPING);
>> + regs->nip = (unsigned long)power4_idle_nap_return;
>
> Why don't you do regs->nip = regs->link like PPC32 instead of going via an intermediate symbol that
> does nothing else than branching to LR ?
It is supposed to keep the return branch predictor balanced.
I don't know if these CPUs have one, if it gets lost during nap, or if
nap latency is so high it really doesn't matter. But I think it is good
practice to make a habit of keeping things balanced.
Thanks,
Nick
^ permalink raw reply
* [PATCH v1 4/4] powerpc: Fix arch_stack_walk() to have running function as first entry
From: Christophe Leroy @ 2021-03-16 7:57 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <955248c6423cb068c5965923121ba31d4dd2fdde.1615881400.git.christophe.leroy@csgroup.eu>
It seems like other architectures, namely x86 and arm64 and riscv
at least, include the running function as top entry when saving
stack trace with save_stack_trace_regs().
Functionnalities like KFENCE expect it.
Do the same on powerpc, it allows KFENCE and other users to
properly identify the faulting function as depicted below.
Before the patch KFENCE was identifying finish_task_switch.isra
as the faulting function.
[ 14.937370] ==================================================================
[ 14.948692] BUG: KFENCE: invalid read in test_invalid_access+0x54/0x108
[ 14.948692]
[ 14.956814] Invalid read at 0xdf98800a:
[ 14.960664] test_invalid_access+0x54/0x108
[ 14.964876] finish_task_switch.isra.0+0x54/0x23c
[ 14.969606] kunit_try_run_case+0x5c/0xd0
[ 14.973658] kunit_generic_run_threadfn_adapter+0x24/0x30
[ 14.979079] kthread+0x15c/0x174
[ 14.982342] ret_from_kernel_thread+0x14/0x1c
[ 14.986731]
[ 14.988236] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: G B 5.12.0-rc1-01537-g95f6e2088d7e-dirty #4682
[ 14.999795] NIP: c016ec2c LR: c02f517c CTR: c016ebd8
[ 15.004851] REGS: e2449d90 TRAP: 0301 Tainted: G B (5.12.0-rc1-01537-g95f6e2088d7e-dirty)
[ 15.015274] MSR: 00009032 <EE,ME,IR,DR,RI> CR: 22000004 XER: 00000000
[ 15.022043] DAR: df98800a DSISR: 20000000
[ 15.022043] GPR00: c02f517c e2449e50 c1142080 e100dd24 c084b13c 00000008 c084b32b c016ebd8
[ 15.022043] GPR08: c0850000 df988000 c0d10000 e2449eb0 22000288
[ 15.040581] NIP [c016ec2c] test_invalid_access+0x54/0x108
[ 15.046010] LR [c02f517c] kunit_try_run_case+0x5c/0xd0
[ 15.051181] Call Trace:
[ 15.053637] [e2449e50] [c005a68c] finish_task_switch.isra.0+0x54/0x23c (unreliable)
[ 15.061338] [e2449eb0] [c02f517c] kunit_try_run_case+0x5c/0xd0
[ 15.067215] [e2449ed0] [c02f648c] kunit_generic_run_threadfn_adapter+0x24/0x30
[ 15.074472] [e2449ef0] [c004e7b0] kthread+0x15c/0x174
[ 15.079571] [e2449f30] [c001317c] ret_from_kernel_thread+0x14/0x1c
[ 15.085798] Instruction dump:
[ 15.088784] 8129d608 38e7ebd8 81020280 911f004c 39000000 995f0024 907f0028 90ff001c
[ 15.096613] 3949000a 915f0020 3d40c0d1 3d00c085 <8929000a> 3908adb0 812a4b98 3d40c02f
[ 15.104612] ==================================================================
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Fixes: 35de3b1aa168 ("powerpc: Implement save_stack_trace_regs() to enable kprobe stack tracing")
Acked-by: Marco Elver <elver@google.com>
---
arch/powerpc/kernel/stacktrace.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c
index 80f92f5b5393..1deb1bf331dd 100644
--- a/arch/powerpc/kernel/stacktrace.c
+++ b/arch/powerpc/kernel/stacktrace.c
@@ -28,6 +28,9 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
{
unsigned long sp;
+ if (regs && !consume_entry(cookie, regs->nip))
+ return;
+
if (regs)
sp = regs->gpr[1];
else if (task == current)
--
2.25.0
^ permalink raw reply related
* [PATCH v1 2/4] powerpc: Rename 'tsk' parameter into 'task'
From: Christophe Leroy @ 2021-03-16 7:57 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <955248c6423cb068c5965923121ba31d4dd2fdde.1615881400.git.christophe.leroy@csgroup.eu>
To better match generic code, rename 'tsk' to 'task' in
some stacktrace functions in preparation of following
patch which converts powerpc to generic ARCH_STACKWALK.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/kernel/stacktrace.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c
index a2a050551109..5b93650bc16c 100644
--- a/arch/powerpc/kernel/stacktrace.c
+++ b/arch/powerpc/kernel/stacktrace.c
@@ -27,13 +27,13 @@
* Save stack-backtrace addresses into a stack_trace buffer.
*/
static void save_context_stack(struct stack_trace *trace, unsigned long sp,
- struct task_struct *tsk, int savesched)
+ struct task_struct *task, int savesched)
{
for (;;) {
unsigned long *stack = (unsigned long *) sp;
unsigned long newsp, ip;
- if (!validate_sp(sp, tsk, STACK_FRAME_OVERHEAD))
+ if (!validate_sp(sp, task, STACK_FRAME_OVERHEAD))
return;
newsp = stack[0];
@@ -94,18 +94,18 @@ EXPORT_SYMBOL_GPL(save_stack_trace_regs);
*
* If the task is not 'current', the caller *must* ensure the task is inactive.
*/
-static int __save_stack_trace_tsk_reliable(struct task_struct *tsk,
+static int __save_stack_trace_tsk_reliable(struct task_struct *task,
struct stack_trace *trace)
{
unsigned long sp;
unsigned long newsp;
- unsigned long stack_page = (unsigned long)task_stack_page(tsk);
+ unsigned long stack_page = (unsigned long)task_stack_page(task);
unsigned long stack_end;
int graph_idx = 0;
bool firstframe;
stack_end = stack_page + THREAD_SIZE;
- if (!is_idle_task(tsk)) {
+ if (!is_idle_task(task)) {
/*
* For user tasks, this is the SP value loaded on
* kernel entry, see "PACAKSAVE(r13)" in _switch() and
@@ -129,10 +129,10 @@ static int __save_stack_trace_tsk_reliable(struct task_struct *tsk,
stack_end -= STACK_FRAME_OVERHEAD;
}
- if (tsk == current)
+ if (task == current)
sp = current_stack_frame();
else
- sp = tsk->thread.ksp;
+ sp = task->thread.ksp;
if (sp < stack_page + sizeof(struct thread_struct) ||
sp > stack_end - STACK_FRAME_MIN_SIZE) {
@@ -181,7 +181,7 @@ static int __save_stack_trace_tsk_reliable(struct task_struct *tsk,
* FIXME: IMHO these tests do not belong in
* arch-dependent code, they are generic.
*/
- ip = ftrace_graph_ret_addr(tsk, &graph_idx, ip, stack);
+ ip = ftrace_graph_ret_addr(task, &graph_idx, ip, stack);
#ifdef CONFIG_KPROBES
/*
* Mark stacktraces with kretprobed functions on them
--
2.25.0
^ permalink raw reply related
* [PATCH v1 3/4] powerpc: Convert stacktrace to generic ARCH_STACKWALK
From: Christophe Leroy @ 2021-03-16 7:57 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <955248c6423cb068c5965923121ba31d4dd2fdde.1615881400.git.christophe.leroy@csgroup.eu>
This patch converts powerpc stacktrace to the generic ARCH_STACKWALK
implemented by commit 214d8ca6ee85 ("stacktrace: Provide common
infrastructure")
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/kernel/stacktrace.c | 91 ++++++--------------------------
2 files changed, 17 insertions(+), 75 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index bb7ca6fee885..60827904a816 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -145,6 +145,7 @@ config PPC
select ARCH_MIGHT_HAVE_PC_PARPORT
select ARCH_MIGHT_HAVE_PC_SERIO
select ARCH_OPTIONAL_KERNEL_RWX if ARCH_HAS_STRICT_KERNEL_RWX
+ select ARCH_STACKWALK
select ARCH_SUPPORTS_ATOMIC_RMW
select ARCH_SUPPORTS_DEBUG_PAGEALLOC if PPC32 || PPC_BOOK3S_64
select ARCH_USE_BUILTIN_BSWAP
diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c
index 5b93650bc16c..80f92f5b5393 100644
--- a/arch/powerpc/kernel/stacktrace.c
+++ b/arch/powerpc/kernel/stacktrace.c
@@ -23,12 +23,18 @@
#include <asm/paca.h>
-/*
- * Save stack-backtrace addresses into a stack_trace buffer.
- */
-static void save_context_stack(struct stack_trace *trace, unsigned long sp,
- struct task_struct *task, int savesched)
+void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
+ struct task_struct *task, struct pt_regs *regs)
{
+ unsigned long sp;
+
+ if (regs)
+ sp = regs->gpr[1];
+ else if (task == current)
+ sp = current_stack_frame();
+ else
+ sp = task->thread.ksp;
+
for (;;) {
unsigned long *stack = (unsigned long *) sp;
unsigned long newsp, ip;
@@ -39,63 +45,21 @@ static void save_context_stack(struct stack_trace *trace, unsigned long sp,
newsp = stack[0];
ip = stack[STACK_FRAME_LR_SAVE];
- if (savesched || !in_sched_functions(ip)) {
- if (!trace->skip)
- trace->entries[trace->nr_entries++] = ip;
- else
- trace->skip--;
- }
-
- if (trace->nr_entries >= trace->max_entries)
+ if (!consume_entry(cookie, ip))
return;
sp = newsp;
}
}
-void save_stack_trace(struct stack_trace *trace)
-{
- unsigned long sp;
-
- sp = current_stack_frame();
-
- save_context_stack(trace, sp, current, 1);
-}
-EXPORT_SYMBOL_GPL(save_stack_trace);
-
-void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
-{
- unsigned long sp;
-
- if (!try_get_task_stack(tsk))
- return;
-
- if (tsk == current)
- sp = current_stack_frame();
- else
- sp = tsk->thread.ksp;
-
- save_context_stack(trace, sp, tsk, 0);
-
- put_task_stack(tsk);
-}
-EXPORT_SYMBOL_GPL(save_stack_trace_tsk);
-
-void
-save_stack_trace_regs(struct pt_regs *regs, struct stack_trace *trace)
-{
- save_context_stack(trace, regs->gpr[1], current, 0);
-}
-EXPORT_SYMBOL_GPL(save_stack_trace_regs);
-
/*
* This function returns an error if it detects any unreliable features of the
* stack. Otherwise it guarantees that the stack trace is reliable.
*
* If the task is not 'current', the caller *must* ensure the task is inactive.
*/
-static int __save_stack_trace_tsk_reliable(struct task_struct *task,
- struct stack_trace *trace)
+int arch_stack_walk_reliable(stack_trace_consume_fn consume_entry,
+ void *cookie, struct task_struct *task)
{
unsigned long sp;
unsigned long newsp;
@@ -191,35 +155,12 @@ static int __save_stack_trace_tsk_reliable(struct task_struct *task,
return -EINVAL;
#endif
- if (trace->nr_entries >= trace->max_entries)
- return -E2BIG;
- if (!trace->skip)
- trace->entries[trace->nr_entries++] = ip;
- else
- trace->skip--;
+ if (!consume_entry(cookie, ip))
+ return -EINVAL;
}
return 0;
}
-int save_stack_trace_tsk_reliable(struct task_struct *tsk,
- struct stack_trace *trace)
-{
- int ret;
-
- /*
- * If the task doesn't have a stack (e.g., a zombie), the stack is
- * "reliably" empty.
- */
- if (!try_get_task_stack(tsk))
- return 0;
-
- ret = __save_stack_trace_tsk_reliable(tsk, trace);
-
- put_task_stack(tsk);
-
- return ret;
-}
-
#if defined(CONFIG_PPC_BOOK3S_64) && defined(CONFIG_NMI_IPI)
static void handle_backtrace_ipi(struct pt_regs *regs)
{
--
2.25.0
^ permalink raw reply related
* [PATCH v1 1/4] powerpc: Activate HAVE_RELIABLE_STACKTRACE for all
From: Christophe Leroy @ 2021-03-16 7:57 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
CONFIG_HAVE_RELIABLE_STACKTRACE is applicable to all, no
reason to limit it to book3s/64le
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/Kconfig | 2 +-
arch/powerpc/kernel/stacktrace.c | 2 --
2 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 67c47b60cc84..bb7ca6fee885 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -235,7 +235,7 @@ config PPC
select MMU_GATHER_RCU_TABLE_FREE
select MMU_GATHER_PAGE_SIZE
select HAVE_REGS_AND_STACK_ACCESS_API
- select HAVE_RELIABLE_STACKTRACE if PPC_BOOK3S_64 && CPU_LITTLE_ENDIAN
+ select HAVE_RELIABLE_STACKTRACE
select HAVE_SOFTIRQ_ON_OWN_STACK
select HAVE_SYSCALL_TRACEPOINTS
select HAVE_VIRT_CPU_ACCOUNTING
diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c
index b6440657ef92..a2a050551109 100644
--- a/arch/powerpc/kernel/stacktrace.c
+++ b/arch/powerpc/kernel/stacktrace.c
@@ -88,7 +88,6 @@ save_stack_trace_regs(struct pt_regs *regs, struct stack_trace *trace)
}
EXPORT_SYMBOL_GPL(save_stack_trace_regs);
-#ifdef CONFIG_HAVE_RELIABLE_STACKTRACE
/*
* This function returns an error if it detects any unreliable features of the
* stack. Otherwise it guarantees that the stack trace is reliable.
@@ -220,7 +219,6 @@ int save_stack_trace_tsk_reliable(struct task_struct *tsk,
return ret;
}
-#endif /* CONFIG_HAVE_RELIABLE_STACKTRACE */
#if defined(CONFIG_PPC_BOOK3S_64) && defined(CONFIG_NMI_IPI)
static void handle_backtrace_ipi(struct pt_regs *regs)
--
2.25.0
^ permalink raw reply related
* Re: [PATCH v3 28/32] powerpc/64s: interrupt implement exit logic in C
From: Nicholas Piggin @ 2021-03-16 7:36 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: Michal Suchanek
In-Reply-To: <2d68d9ad-c3a2-2372-a5b2-1a1e3fdb41e4@csgroup.eu>
Excerpts from Christophe Leroy's message of March 15, 2021 11:41 pm:
>
>
> Le 25/02/2020 à 18:35, Nicholas Piggin a écrit :
>> Implement the bulk of interrupt return logic in C. The asm return code
>> must handle a few cases: restoring full GPRs, and emulating stack store.
>>
>> The stack store emulation is significantly simplfied, rather than creating
>> a new return frame and switching to that before performing the store, it
>> uses the PACA to keep a scratch register around to perform thestore.
>>
>> The asm return code is moved into 64e for now. The new logic has made
>> allowance for 64e, but I don't have a full environment that works well
>> to test it, and even booting in emulated qemu is not great for stress
>> testing. 64e shouldn't be too far off working with this, given a bit
>> more testing and auditing of the logic.
>>
>> This is slightly faster on a POWER9 (page fault speed increases about
>> 1.1%), probably due to reduced mtmsrd.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
>> ---
>
> ...
>
>> +notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned long msr)
>> +{
>
> ...
>
>> +
>> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>> + local_paca->tm_scratch = regs->msr;
>> +#endif
>
> Could we define a helper for that in asm/tm.h, that voids when CONFIG_PPC_TRANSACTIONAL_MEM is not
> selected ?
Yeah I wanted to do something about that. I don't know what it's used
for here. I guess it saves the return MSR so if that causes a crash then
the next oops would see it, but I wonder if we can just get that from
SRR1 + program check error codes, or if there is something we can't
reconstruct from there. Have to check with someone who knows TM better.
Thanks,
Nick
^ permalink raw reply
* [PATCH] selftests: remove duplicate include
From: menglong8.dong @ 2021-03-16 7:33 UTC (permalink / raw)
To: pbonzini
Cc: kvm, linuxppc-dev, linux-kernel, zhang.yunkai, paulus,
linux-kselftest, akpm, ricardo.canuelo, shuah
From: Zhang Yunkai <zhang.yunkai@zte.com.cn>
'assert.h' included in 'sparsebit.c' is duplicated.
It is also included in the 161th line.
'string.h' included in 'mincore_selftest.c' is duplicated.
It is also included in the 15th line.
'sched.h' included in 'tlbie_test.c' is duplicated.
It is also included in the 33th line.
Signed-off-by: Zhang Yunkai <zhang.yunkai@zte.com.cn>
---
tools/testing/selftests/kvm/lib/sparsebit.c | 1 -
tools/testing/selftests/mincore/mincore_selftest.c | 1 -
tools/testing/selftests/powerpc/mm/tlbie_test.c | 1 -
3 files changed, 3 deletions(-)
diff --git a/tools/testing/selftests/kvm/lib/sparsebit.c b/tools/testing/selftests/kvm/lib/sparsebit.c
index 031ba3c932ed..a0d0c83d83de 100644
--- a/tools/testing/selftests/kvm/lib/sparsebit.c
+++ b/tools/testing/selftests/kvm/lib/sparsebit.c
@@ -1890,7 +1890,6 @@ void sparsebit_validate_internal(struct sparsebit *s)
*/
#include <stdlib.h>
-#include <assert.h>
struct range {
sparsebit_idx_t first, last;
diff --git a/tools/testing/selftests/mincore/mincore_selftest.c b/tools/testing/selftests/mincore/mincore_selftest.c
index 5a1e85ff5d32..e54106643337 100644
--- a/tools/testing/selftests/mincore/mincore_selftest.c
+++ b/tools/testing/selftests/mincore/mincore_selftest.c
@@ -14,7 +14,6 @@
#include <sys/mman.h>
#include <string.h>
#include <fcntl.h>
-#include <string.h>
#include "../kselftest.h"
#include "../kselftest_harness.h"
diff --git a/tools/testing/selftests/powerpc/mm/tlbie_test.c b/tools/testing/selftests/powerpc/mm/tlbie_test.c
index f85a0938ab25..48344a74b212 100644
--- a/tools/testing/selftests/powerpc/mm/tlbie_test.c
+++ b/tools/testing/selftests/powerpc/mm/tlbie_test.c
@@ -33,7 +33,6 @@
#include <sched.h>
#include <time.h>
#include <stdarg.h>
-#include <sched.h>
#include <pthread.h>
#include <signal.h>
#include <sys/prctl.h>
--
2.25.1
^ permalink raw reply related
* Re: [PATCH 03/10] powerpc/64e/interrupt: use new interrupt return
From: Christophe Leroy @ 2021-03-16 7:29 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev; +Cc: Scott Wood
In-Reply-To: <1615879421.ckimzb9u3c.astroid@bobo.none>
Le 16/03/2021 à 08:25, Nicholas Piggin a écrit :
> Excerpts from Nicholas Piggin's message of March 16, 2021 5:04 pm:
>> Excerpts from Christophe Leroy's message of March 15, 2021 11:30 pm:
>>>
>>>
>>> Le 15/03/2021 à 04:17, Nicholas Piggin a écrit :
>>>> Update the new C and asm interrupt return code to account for 64e
>>>> specifics, switch over to use it.
>>>>
>>>> The now-unused old ret_from_except code, that was moved to 64e after the
>>>> 64s conversion, is removed.
>>>>
>>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>> ---
>>>> arch/powerpc/include/asm/asm-prototypes.h | 2 -
>>>> arch/powerpc/kernel/entry_64.S | 9 +-
>>>> arch/powerpc/kernel/exceptions-64e.S | 321 ++--------------------
>>>> arch/powerpc/kernel/interrupt.c | 27 +-
>>>> arch/powerpc/kernel/irq.c | 76 -----
>>>> 5 files changed, 56 insertions(+), 379 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
>>>> index fbabb49888d3..ae7b058b2970 100644
>>>> --- a/arch/powerpc/kernel/interrupt.c
>>>> +++ b/arch/powerpc/kernel/interrupt.c
>>>> @@ -235,6 +235,10 @@ static notrace void booke_load_dbcr0(void)
>>>> #endif
>>>> }
>>>>
>>>> +/* temporary hack for context tracking, removed in later patch */
>>>> +#include <linux/sched/debug.h>
>>>> +asmlinkage __visible void __sched schedule_user(void);
>>>> +
>>>> /*
>>>> * This should be called after a syscall returns, with r3 the return value
>>>> * from the syscall. If this function returns non-zero, the system call
>>>> @@ -292,7 +296,11 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
>>>> while (unlikely(ti_flags & (_TIF_USER_WORK_MASK & ~_TIF_RESTORE_TM))) {
>>>> local_irq_enable();
>>>> if (ti_flags & _TIF_NEED_RESCHED) {
>>>> +#ifdef CONFIG_PPC_BOOK3E_64
>>>> + schedule_user();
>>>> +#else
>>>> schedule();
>>>> +#endif
>>>> } else {
>>>> /*
>>>> * SIGPENDING must restore signal handler function
>>>> @@ -360,7 +368,6 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
>>>> return ret;
>>>> }
>>>>
>>>> -#ifndef CONFIG_PPC_BOOK3E_64 /* BOOK3E not yet using this */
>>>> notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned long msr)
>>>> {
>>>> unsigned long ti_flags;
>>>> @@ -372,7 +379,9 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned
>>>> BUG_ON(!(regs->msr & MSR_PR));
>>>> BUG_ON(!FULL_REGS(regs));
>>>> BUG_ON(arch_irq_disabled_regs(regs));
>>>> +#ifdef CONFIG_PPC_BOOK3S_64
>>>
>>> Shouldn't this go away in patch 6 as well ?
>>> Or is that needed at all ? In syscall_exit_prepare() it is not ifdefed .
>>
>> Hmm, not sure. I'll take a look. It probably shouldn't be ifdefed at all
>> but definitely by the end it should run without warning.
>
> Oh I got confused and thought that was the syscall exit. Interrupt exit
> has to keep this until patch 6 because 64e context tracking does
> everything in interrupt wrappers, so by the time we get here it will
> already be set to CONTEXT_USER.
>
ok, but that it has to go in patch 6 ? At the time being the #ifdef is still there at the end of the
series
^ permalink raw reply
* Re: [PATCH 03/10] powerpc/64e/interrupt: use new interrupt return
From: Nicholas Piggin @ 2021-03-16 7:25 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: Scott Wood
In-Reply-To: <1615878222.rk6eq5hjpl.astroid@bobo.none>
Excerpts from Nicholas Piggin's message of March 16, 2021 5:04 pm:
> Excerpts from Christophe Leroy's message of March 15, 2021 11:30 pm:
>>
>>
>> Le 15/03/2021 à 04:17, Nicholas Piggin a écrit :
>>> Update the new C and asm interrupt return code to account for 64e
>>> specifics, switch over to use it.
>>>
>>> The now-unused old ret_from_except code, that was moved to 64e after the
>>> 64s conversion, is removed.
>>>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>> arch/powerpc/include/asm/asm-prototypes.h | 2 -
>>> arch/powerpc/kernel/entry_64.S | 9 +-
>>> arch/powerpc/kernel/exceptions-64e.S | 321 ++--------------------
>>> arch/powerpc/kernel/interrupt.c | 27 +-
>>> arch/powerpc/kernel/irq.c | 76 -----
>>> 5 files changed, 56 insertions(+), 379 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
>>> index fbabb49888d3..ae7b058b2970 100644
>>> --- a/arch/powerpc/kernel/interrupt.c
>>> +++ b/arch/powerpc/kernel/interrupt.c
>>> @@ -235,6 +235,10 @@ static notrace void booke_load_dbcr0(void)
>>> #endif
>>> }
>>>
>>> +/* temporary hack for context tracking, removed in later patch */
>>> +#include <linux/sched/debug.h>
>>> +asmlinkage __visible void __sched schedule_user(void);
>>> +
>>> /*
>>> * This should be called after a syscall returns, with r3 the return value
>>> * from the syscall. If this function returns non-zero, the system call
>>> @@ -292,7 +296,11 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
>>> while (unlikely(ti_flags & (_TIF_USER_WORK_MASK & ~_TIF_RESTORE_TM))) {
>>> local_irq_enable();
>>> if (ti_flags & _TIF_NEED_RESCHED) {
>>> +#ifdef CONFIG_PPC_BOOK3E_64
>>> + schedule_user();
>>> +#else
>>> schedule();
>>> +#endif
>>> } else {
>>> /*
>>> * SIGPENDING must restore signal handler function
>>> @@ -360,7 +368,6 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
>>> return ret;
>>> }
>>>
>>> -#ifndef CONFIG_PPC_BOOK3E_64 /* BOOK3E not yet using this */
>>> notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned long msr)
>>> {
>>> unsigned long ti_flags;
>>> @@ -372,7 +379,9 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned
>>> BUG_ON(!(regs->msr & MSR_PR));
>>> BUG_ON(!FULL_REGS(regs));
>>> BUG_ON(arch_irq_disabled_regs(regs));
>>> +#ifdef CONFIG_PPC_BOOK3S_64
>>
>> Shouldn't this go away in patch 6 as well ?
>> Or is that needed at all ? In syscall_exit_prepare() it is not ifdefed .
>
> Hmm, not sure. I'll take a look. It probably shouldn't be ifdefed at all
> but definitely by the end it should run without warning.
Oh I got confused and thought that was the syscall exit. Interrupt exit
has to keep this until patch 6 because 64e context tracking does
everything in interrupt wrappers, so by the time we get here it will
already be set to CONTEXT_USER.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v9 7/8] powerpc/mm: implement set_memory_attr()
From: Christophe Leroy @ 2021-03-16 7:25 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev
Cc: christophe.leroy, ajd, npiggin, kbuild test robot, naveen.n.rao,
dja
In-Reply-To: <20210316031741.1004850-7-jniethe5@gmail.com>
Le 16/03/2021 à 04:17, Jordan Niethe a écrit :
> From: Christophe Leroy <christophe.leroy@c-s.fr>
Can you please update the whole series with my new email address: christophe.leroy@csgroup.eu
>
> In addition to the set_memory_xx() functions which allows to change
> the memory attributes of not (yet) used memory regions, implement a
> set_memory_attr() function to:
> - set the final memory protection after init on currently used
> kernel regions.
> - enable/disable kernel memory regions in the scope of DEBUG_PAGEALLOC.
>
> Unlike the set_memory_xx() which can act in three step as the regions
> are unused, this function must modify 'on the fly' as the kernel is
> executing from them. At the moment only PPC32 will use it and changing
> page attributes on the fly is not an issue.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> Reported-by: kbuild test robot <lkp@intel.com>
> [ruscur: cast "data" to unsigned long instead of int]
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> arch/powerpc/include/asm/set_memory.h | 2 ++
> arch/powerpc/mm/pageattr.c | 33 +++++++++++++++++++++++++++
> 2 files changed, 35 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/set_memory.h b/arch/powerpc/include/asm/set_memory.h
> index 64011ea444b4..b040094f7920 100644
> --- a/arch/powerpc/include/asm/set_memory.h
> +++ b/arch/powerpc/include/asm/set_memory.h
> @@ -29,4 +29,6 @@ static inline int set_memory_x(unsigned long addr, int numpages)
> return change_memory_attr(addr, numpages, SET_MEMORY_X);
> }
>
> +int set_memory_attr(unsigned long addr, int numpages, pgprot_t prot);
> +
> #endif
> diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
> index 2da3fbab6ff7..2fde1b195c85 100644
> --- a/arch/powerpc/mm/pageattr.c
> +++ b/arch/powerpc/mm/pageattr.c
> @@ -79,3 +79,36 @@ int change_memory_attr(unsigned long addr, int numpages, long action)
> return apply_to_existing_page_range(&init_mm, start, sz,
> change_page_attr, (void *)action);
> }
> +
> +/*
> + * Set the attributes of a page:
> + *
> + * This function is used by PPC32 at the end of init to set final kernel memory
> + * protection. It includes changing the maping of the page it is executing from
> + * and data pages it is using.
> + */
> +static int set_page_attr(pte_t *ptep, unsigned long addr, void *data)
> +{
> + pgprot_t prot = __pgprot((unsigned long)data);
> +
> + spin_lock(&init_mm.page_table_lock);
> +
> + set_pte_at(&init_mm, addr, ptep, pte_modify(*ptep, prot));
> + flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
> +
> + spin_unlock(&init_mm.page_table_lock);
> +
> + return 0;
> +}
> +
> +int set_memory_attr(unsigned long addr, int numpages, pgprot_t prot)
> +{
> + unsigned long start = ALIGN_DOWN(addr, PAGE_SIZE);
> + unsigned long sz = numpages * PAGE_SIZE;
> +
> + if (numpages <= 0)
> + return 0;
> +
> + return apply_to_existing_page_range(&init_mm, start, sz, set_page_attr,
> + (void *)pgprot_val(prot));
> +}
>
^ permalink raw reply
* Re: [PATCH] scsi: ibmvfc: Switch to using the new API kobj_to_dev()
From: Martin K. Petersen @ 2021-03-16 3:15 UTC (permalink / raw)
To: Yang Li
Cc: tyreld, Martin K . Petersen, linux-scsi, jejb, linux-kernel,
paulus, linuxppc-dev
In-Reply-To: <1613976429-89853-1-git-send-email-yang.lee@linux.alibaba.com>
On Mon, 22 Feb 2021 14:47:09 +0800, Yang Li wrote:
> fixed the following coccicheck:
> ./drivers/scsi/ibmvscsi/ibmvfc.c:3161:60-61: WARNING opportunity for
> kobj_to_dev()
Applied to 5.13/scsi-queue, thanks!
[1/1] scsi: ibmvfc: Switch to using the new API kobj_to_dev()
https://git.kernel.org/mkp/scsi/c/18c2a59a4190
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply
* Re: [PATCH 12/14] powerpc/64s: system call avoid setting MSR[RI] until we set MSR[EE]
From: Christophe Leroy @ 2021-03-16 7:21 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev
In-Reply-To: <20210315220402.260594-13-npiggin@gmail.com>
Le 15/03/2021 à 23:04, Nicholas Piggin a écrit :
> This extends the MSR[RI]=0 window a little further into the system
> call in order to pair RI and EE enabling with a single mtmsrd.
Time ago, I proposed to delay that on PPC32 and Michael objected, see
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/9f9dd859d571e324c7412ed9db9da8cfba678257.1548956511.git.christophe.leroy@c-s.fr/
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/kernel/exceptions-64s.S | 2 --
> arch/powerpc/kernel/interrupt_64.S | 6 +++---
> 2 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index bd0c82ac9de5..2f14ac3c377c 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -1999,8 +1999,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_REAL_LE)
> mtctr r10
> bctr
> .else
> - li r10,MSR_RI
> - mtmsrd r10,1 /* Set RI (EE=0) */
> #ifdef CONFIG_RELOCATABLE
> __LOAD_HANDLER(r10, system_call_common)
> mtctr r10
> diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S
> index f28f41a1a85a..eef61800f734 100644
> --- a/arch/powerpc/kernel/interrupt_64.S
> +++ b/arch/powerpc/kernel/interrupt_64.S
> @@ -311,10 +311,10 @@ END_BTB_FLUSH_SECTION
> * nothing pending. system_call_exception() will call
> * trace_hardirqs_off().
> */
> - li r11,IRQS_ALL_DISABLED
> - li r12,PACA_IRQ_HARD_DIS
> + li r11,IRQS_DISABLED
> + li r12,-1 /* Set MSR_EE and MSR_RI */
> stb r11,PACAIRQSOFTMASK(r13)
> - stb r12,PACAIRQHAPPENED(r13)
> + mtmsrd r12,1
>
> ENTER_KERNEL_SECURITY_FALLBACK
>
>
^ permalink raw reply
* Re: [PATCH] powerpc/64s: power4 nap fixup in C
From: Christophe Leroy @ 2021-03-16 7:16 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev
In-Reply-To: <20210312012044.3660743-1-npiggin@gmail.com>
Le 12/03/2021 à 02:20, Nicholas Piggin a écrit :
> There is no need for this to be in asm, use the new intrrupt entry wrapper.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> Hopefully this works on a real G5 now, but I couldn't reproduce the
> problem with QEMU.
>
> Thanks,
> Nick
>
> arch/powerpc/include/asm/interrupt.h | 19 +++++++++++
> arch/powerpc/include/asm/processor.h | 1 +
> arch/powerpc/include/asm/thread_info.h | 6 ++++
> arch/powerpc/kernel/exceptions-64s.S | 45 --------------------------
> arch/powerpc/kernel/idle_book3s.S | 4 +++
> 5 files changed, 30 insertions(+), 45 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
> index aedfba29e43a..ef015d3b5e39 100644
> --- a/arch/powerpc/include/asm/interrupt.h
> +++ b/arch/powerpc/include/asm/interrupt.h
> @@ -9,6 +9,17 @@
> #include <asm/kprobes.h>
> #include <asm/runlatch.h>
>
> +static inline void nap_adjust_return(struct pt_regs *regs)
> +{
> +#ifdef CONFIG_PPC_970_NAP
> + if (unlikely(test_thread_local_flags(_TLF_NAPPING))) {
> + /* Can avoid a test-and-clear because NMIs do not call this */
> + clear_thread_local_flags(_TLF_NAPPING);
> + regs->nip = (unsigned long)power4_idle_nap_return;
Why don't you do regs->nip = regs->link like PPC32 instead of going via an intermediate symbol that
does nothing else than branching to LR ?
> + }
> +#endif
> +}
> +
> struct interrupt_state {
> #ifdef CONFIG_PPC_BOOK3E_64
> enum ctx_state ctx_state;
> @@ -111,6 +122,9 @@ static inline void interrupt_async_exit_prepare(struct pt_regs *regs, struct int
> {
> irq_exit();
> interrupt_exit_prepare(regs, state);
> +
> + /* Adjust at exit so the main handler sees the true NIA */
> + nap_adjust_return(regs);
> }
>
> struct interrupt_nmi_state {
> @@ -164,6 +178,11 @@ static inline void interrupt_nmi_exit_prepare(struct pt_regs *regs, struct inter
> radix_enabled() || (mfmsr() & MSR_DR))
> nmi_exit();
>
> + /*
> + * nmi does not call nap_adjust_return because nmi should not create
> + * new work to do (must use irq_work for that).
> + */
> +
> #ifdef CONFIG_PPC64
> if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260)
> this_cpu_set_ftrace_enabled(state->ftrace_enabled);
...
> diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S
> index f9e6d83e6720..abb719b21cae 100644
> --- a/arch/powerpc/kernel/idle_book3s.S
> +++ b/arch/powerpc/kernel/idle_book3s.S
> @@ -209,4 +209,8 @@ _GLOBAL(power4_idle_nap)
> mtmsrd r7
> isync
> b 1b
> +
> + .globl power4_idle_nap_return
> +power4_idle_nap_return:
> + blr
> #endif
>
^ permalink raw reply
* Re: [PATCH 10/10] powerpc: move norestart trap flag to bit 0
From: Christophe Leroy @ 2021-03-16 7:13 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev; +Cc: Scott Wood
In-Reply-To: <1615878424.0gp943h7l3.astroid@bobo.none>
Le 16/03/2021 à 08:11, Nicholas Piggin a écrit :
> Excerpts from Christophe Leroy's message of March 15, 2021 6:14 pm:
>>
>>
>> Le 15/03/2021 à 04:17, Nicholas Piggin a écrit :
>>> Compact the trap flags down to use the low 4 bits of regs.trap.
>>>
>>> A few 64e interrupt trap numbers set bit 4. Although they tended to be
>>> trivial so it wasn't a real problem[1], it is not the right thing to do,
>>> and confusing.
>>>
>>> [*] E.g., 0x310 hypercall goes to unknown_exception, which prints
>>> regs->trap directly so 0x310 will appear fine, and only the syscall
>>> interrupt will test norestart, so it won't be confused by 0x310.
>>>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>> arch/powerpc/include/asm/ptrace.h | 14 ++++++++++----
>>> 1 file changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
>>> index 91194fdd5d01..6a04abfe5eb6 100644
>>> --- a/arch/powerpc/include/asm/ptrace.h
>>> +++ b/arch/powerpc/include/asm/ptrace.h
>>> @@ -185,15 +185,21 @@ static inline void regs_set_return_value(struct pt_regs *regs, unsigned long rc)
>>> #define current_pt_regs() \
>>> ((struct pt_regs *)((unsigned long)task_stack_page(current) + THREAD_SIZE) - 1)
>>>
>>> +/*
>>> + * The 4 low bits (0xf) are available as flags to overload the trap word,
>>> + * because interrupt vectors have minimum alignment of 0x10. TRAP_FLAGS_MASK
>>> + * must cover the bits used as flags, including bit 0 which is used as the
>>> + * "norestart" bit.
>>> + */
>>> #ifdef __powerpc64__
>>> -#define TRAP_FLAGS_MASK 0x10
>>> +#define TRAP_FLAGS_MASK 0x1
>>> #define TRAP(regs) ((regs)->trap & ~TRAP_FLAGS_MASK)
>>> #else
>>> /*
>>> * On 4xx we use bit 1 in the trap word to indicate whether the exception
>>> * is a critical exception (1 means it is).
>>> */
>>> -#define TRAP_FLAGS_MASK 0x1E
>>> +#define TRAP_FLAGS_MASK 0xf
>>
>> Could we set 0xf for all and remove the ifdef __powerpc64__ ?
>
> I like that it documents the bit number allocation so I prefer to leave
> it, but TRAP() does not have to be defined twice at least.
>
>>
>>> #define TRAP(regs) ((regs)->trap & ~TRAP_FLAGS_MASK)
>>> #define IS_CRITICAL_EXC(regs) (((regs)->trap & 2) != 0)
>>> #define IS_MCHECK_EXC(regs) (((regs)->trap & 4) != 0)
>>> @@ -222,12 +228,12 @@ static inline bool trap_is_syscall(struct pt_regs *regs)
>>>
>>> static inline bool trap_norestart(struct pt_regs *regs)
>>> {
>>> - return regs->trap & 0x10;
>>> + return regs->trap & 0x1;
>>> }
>>>
>>> static inline void set_trap_norestart(struct pt_regs *regs)
>>> {
>>> - regs->trap |= 0x10;
>>> + regs->trap |= 0x1;
>>> }
>>>
>>> #define arch_has_single_step() (1)
>>>
>>
>> While we are playing with ->trap, in mm/book3s64/hash_utils.c there is an if (regs->trap == 0x400).
>> Should be TRAP(regs) == 0x400 ?
>
> Yes I would say so, if you want to do a patch you can add
> Acked-by: Nicholas Piggin <npiggin@gmail.com>
>
> Otherwise I can do it.
Yes please do.
Thanks
Christophe
^ permalink raw reply
* Re: [PATCH 10/10] powerpc: move norestart trap flag to bit 0
From: Nicholas Piggin @ 2021-03-16 7:11 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: Scott Wood
In-Reply-To: <99f15df0-dc86-4601-066f-a6c067ece8bf@csgroup.eu>
Excerpts from Christophe Leroy's message of March 15, 2021 6:14 pm:
>
>
> Le 15/03/2021 à 04:17, Nicholas Piggin a écrit :
>> Compact the trap flags down to use the low 4 bits of regs.trap.
>>
>> A few 64e interrupt trap numbers set bit 4. Although they tended to be
>> trivial so it wasn't a real problem[1], it is not the right thing to do,
>> and confusing.
>>
>> [*] E.g., 0x310 hypercall goes to unknown_exception, which prints
>> regs->trap directly so 0x310 will appear fine, and only the syscall
>> interrupt will test norestart, so it won't be confused by 0x310.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> arch/powerpc/include/asm/ptrace.h | 14 ++++++++++----
>> 1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
>> index 91194fdd5d01..6a04abfe5eb6 100644
>> --- a/arch/powerpc/include/asm/ptrace.h
>> +++ b/arch/powerpc/include/asm/ptrace.h
>> @@ -185,15 +185,21 @@ static inline void regs_set_return_value(struct pt_regs *regs, unsigned long rc)
>> #define current_pt_regs() \
>> ((struct pt_regs *)((unsigned long)task_stack_page(current) + THREAD_SIZE) - 1)
>>
>> +/*
>> + * The 4 low bits (0xf) are available as flags to overload the trap word,
>> + * because interrupt vectors have minimum alignment of 0x10. TRAP_FLAGS_MASK
>> + * must cover the bits used as flags, including bit 0 which is used as the
>> + * "norestart" bit.
>> + */
>> #ifdef __powerpc64__
>> -#define TRAP_FLAGS_MASK 0x10
>> +#define TRAP_FLAGS_MASK 0x1
>> #define TRAP(regs) ((regs)->trap & ~TRAP_FLAGS_MASK)
>> #else
>> /*
>> * On 4xx we use bit 1 in the trap word to indicate whether the exception
>> * is a critical exception (1 means it is).
>> */
>> -#define TRAP_FLAGS_MASK 0x1E
>> +#define TRAP_FLAGS_MASK 0xf
>
> Could we set 0xf for all and remove the ifdef __powerpc64__ ?
I like that it documents the bit number allocation so I prefer to leave
it, but TRAP() does not have to be defined twice at least.
>
>> #define TRAP(regs) ((regs)->trap & ~TRAP_FLAGS_MASK)
>> #define IS_CRITICAL_EXC(regs) (((regs)->trap & 2) != 0)
>> #define IS_MCHECK_EXC(regs) (((regs)->trap & 4) != 0)
>> @@ -222,12 +228,12 @@ static inline bool trap_is_syscall(struct pt_regs *regs)
>>
>> static inline bool trap_norestart(struct pt_regs *regs)
>> {
>> - return regs->trap & 0x10;
>> + return regs->trap & 0x1;
>> }
>>
>> static inline void set_trap_norestart(struct pt_regs *regs)
>> {
>> - regs->trap |= 0x10;
>> + regs->trap |= 0x1;
>> }
>>
>> #define arch_has_single_step() (1)
>>
>
> While we are playing with ->trap, in mm/book3s64/hash_utils.c there is an if (regs->trap == 0x400).
> Should be TRAP(regs) == 0x400 ?
Yes I would say so, if you want to do a patch you can add
Acked-by: Nicholas Piggin <npiggin@gmail.com>
Otherwise I can do it.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH 07/10] powerpc/64e/interrupt: handle bad_page_fault in C
From: Nicholas Piggin @ 2021-03-16 7:06 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: Scott Wood
In-Reply-To: <6e0873c6-7102-ff8b-1552-8ce158613fd7@csgroup.eu>
Excerpts from Christophe Leroy's message of March 16, 2021 12:07 am:
>
>
> Le 15/03/2021 à 04:17, Nicholas Piggin a écrit :
>> With non-volatile registers saved on interrupt, bad_page_fault
>> can now be called by do_page_fault.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> arch/powerpc/kernel/exceptions-64e.S | 6 ------
>> arch/powerpc/mm/fault.c | 5 +----
>> 2 files changed, 1 insertion(+), 10 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
>> index a059ab3542c2..b08c84e0fa56 100644
>> --- a/arch/powerpc/kernel/exceptions-64e.S
>> +++ b/arch/powerpc/kernel/exceptions-64e.S
>> @@ -937,12 +937,6 @@ storage_fault_common:
>> ld r14,PACA_EXGEN+EX_R14(r13)
>> ld r15,PACA_EXGEN+EX_R15(r13)
>> bl do_page_fault
>> - cmpdi r3,0
>> - bne- 1f
>> - b interrupt_return
>> - mr r4,r3
>> - addi r3,r1,STACK_FRAME_OVERHEAD
>> - bl __bad_page_fault
>
> Then __bad_page_fault() can be static now.
Good point, I'll change it.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH 03/10] powerpc/64e/interrupt: use new interrupt return
From: Nicholas Piggin @ 2021-03-16 7:04 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: Scott Wood
In-Reply-To: <a2c192a2-ebdb-d18a-6e21-b27d8890fe06@csgroup.eu>
Excerpts from Christophe Leroy's message of March 15, 2021 11:30 pm:
>
>
> Le 15/03/2021 à 04:17, Nicholas Piggin a écrit :
>> Update the new C and asm interrupt return code to account for 64e
>> specifics, switch over to use it.
>>
>> The now-unused old ret_from_except code, that was moved to 64e after the
>> 64s conversion, is removed.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> arch/powerpc/include/asm/asm-prototypes.h | 2 -
>> arch/powerpc/kernel/entry_64.S | 9 +-
>> arch/powerpc/kernel/exceptions-64e.S | 321 ++--------------------
>> arch/powerpc/kernel/interrupt.c | 27 +-
>> arch/powerpc/kernel/irq.c | 76 -----
>> 5 files changed, 56 insertions(+), 379 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
>> index fbabb49888d3..ae7b058b2970 100644
>> --- a/arch/powerpc/kernel/interrupt.c
>> +++ b/arch/powerpc/kernel/interrupt.c
>> @@ -235,6 +235,10 @@ static notrace void booke_load_dbcr0(void)
>> #endif
>> }
>>
>> +/* temporary hack for context tracking, removed in later patch */
>> +#include <linux/sched/debug.h>
>> +asmlinkage __visible void __sched schedule_user(void);
>> +
>> /*
>> * This should be called after a syscall returns, with r3 the return value
>> * from the syscall. If this function returns non-zero, the system call
>> @@ -292,7 +296,11 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
>> while (unlikely(ti_flags & (_TIF_USER_WORK_MASK & ~_TIF_RESTORE_TM))) {
>> local_irq_enable();
>> if (ti_flags & _TIF_NEED_RESCHED) {
>> +#ifdef CONFIG_PPC_BOOK3E_64
>> + schedule_user();
>> +#else
>> schedule();
>> +#endif
>> } else {
>> /*
>> * SIGPENDING must restore signal handler function
>> @@ -360,7 +368,6 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
>> return ret;
>> }
>>
>> -#ifndef CONFIG_PPC_BOOK3E_64 /* BOOK3E not yet using this */
>> notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned long msr)
>> {
>> unsigned long ti_flags;
>> @@ -372,7 +379,9 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned
>> BUG_ON(!(regs->msr & MSR_PR));
>> BUG_ON(!FULL_REGS(regs));
>> BUG_ON(arch_irq_disabled_regs(regs));
>> +#ifdef CONFIG_PPC_BOOK3S_64
>
> Shouldn't this go away in patch 6 as well ?
> Or is that needed at all ? In syscall_exit_prepare() it is not ifdefed .
Hmm, not sure. I'll take a look. It probably shouldn't be ifdefed at all
but definitely by the end it should run without warning.
Thanks,
Nick
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox