* Re: [PATCH v2 1/1] powerpc/kvm: Save Timebase Offset to fix sched_clock() while running guest code.
From: Leonardo Bras @ 2021-02-05 20:35 UTC (permalink / raw)
To: Fabiano Rosas, Paul Mackerras, Michael Ellerman,
Benjamin Herrenschmidt, Christophe Leroy, Athira Rajeev,
Aneesh Kumar K.V, Jordan Niethe, Nicholas Piggin,
Frederic Weisbecker, Thomas Gleixner, Geert Uytterhoeven
Cc: linuxppc-dev, linux-kernel, kvm-ppc
In-Reply-To: <874kiqy82t.fsf@linux.ibm.com>
Hello Fabiano,
Thanks for reviewing!
(answers inline)
On Fri, 2021-02-05 at 10:09 -0300, Fabiano Rosas wrote:
> Leonardo Bras <leobras.c@gmail.com> writes:
>
> > Before guest entry, TBU40 register is changed to reflect guest timebase.
> > After exitting guest, the register is reverted to it's original value.
> >
> > If one tries to get the timestamp from host between those changes, it
> > will present an incorrect value.
> >
> > An example would be trying to add a tracepoint in
> > kvmppc_guest_entry_inject_int(), which depending on last tracepoint
> > acquired could actually cause the host to crash.
> >
> > Save the Timebase Offset to PACA and use it on sched_clock() to always
> > get the correct timestamp.
> >
> > Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> > Suggested-by: Paul Mackerras <paulus@ozlabs.org>
> > ---
> > Changes since v1:
> > - Subtracts offset only when CONFIG_KVM_BOOK3S_HANDLER and
> > CONFIG_PPC_BOOK3S_64 are defined.
> > ---
> > arch/powerpc/include/asm/kvm_book3s_asm.h | 1 +
> > arch/powerpc/kernel/asm-offsets.c | 1 +
> > arch/powerpc/kernel/time.c | 8 +++++++-
> > arch/powerpc/kvm/book3s_hv.c | 2 ++
> > arch/powerpc/kvm/book3s_hv_rmhandlers.S | 2 ++
> > 5 files changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/powerpc/include/asm/kvm_book3s_asm.h b/arch/powerpc/include/asm/kvm_book3s_asm.h
> > index 078f4648ea27..e2c12a10eed2 100644
> > --- a/arch/powerpc/include/asm/kvm_book3s_asm.h
> > +++ b/arch/powerpc/include/asm/kvm_book3s_asm.h
> > @@ -131,6 +131,7 @@ struct kvmppc_host_state {
> > u64 cfar;
> > u64 ppr;
> > u64 host_fscr;
> > + u64 tb_offset; /* Timebase offset: keeps correct
> > timebase while on guest */
>
> Couldn't you use the vc->tb_offset_applied for this? We have a reference
> for the vcore in the hstate already.
But it's a pointer, which means we would have to keep checking for NULL
every time we need sched_clock().
Potentially it would cost a cache miss for PACA memory region that
contain vc, another for getting the part of *vc that contains the
tb_offset_applied, instead of only one for PACA struct region that
contains tb_offset.
On the other hand, it got me thinking: If the offset is applied per
cpu, why don't we get this info only in PACA, instead of in vc?
It could be a general way to get an offset applied for any purpose and
still get the sched_clock() right.
(Not that I have any idea of any other purpose we could use it)
Best regards!
Leonardo Bras
>
> > #endif
> > };
> >
> > diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
> > index b12d7c049bfe..0beb8fdc6352 100644
> > --- a/arch/powerpc/kernel/asm-offsets.c
> > +++ b/arch/powerpc/kernel/asm-offsets.c
> > @@ -706,6 +706,7 @@ int main(void)
> > HSTATE_FIELD(HSTATE_CFAR, cfar);
> > HSTATE_FIELD(HSTATE_PPR, ppr);
> > HSTATE_FIELD(HSTATE_HOST_FSCR, host_fscr);
> > + HSTATE_FIELD(HSTATE_TB_OFFSET, tb_offset);
> > #endif /* CONFIG_PPC_BOOK3S_64 */
> >
> > #else /* CONFIG_PPC_BOOK3S */
> > diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
> > index 67feb3524460..f27f0163792b 100644
> > --- a/arch/powerpc/kernel/time.c
> > +++ b/arch/powerpc/kernel/time.c
> > @@ -699,7 +699,13 @@ EXPORT_SYMBOL_GPL(tb_to_ns);
> > */
> > notrace unsigned long long sched_clock(void)
> > {
> > - return mulhdu(get_tb() - boot_tb, tb_to_ns_scale) << tb_to_ns_shift;
> > + u64 tb = get_tb() - boot_tb;
> > +
> > +#if defined(CONFIG_PPC_BOOK3S_64) && defined(CONFIG_KVM_BOOK3S_HANDLER)
> > + tb -= local_paca->kvm_hstate.tb_offset;
> > +#endif
> > +
> > + return mulhdu(tb, tb_to_ns_scale) << tb_to_ns_shift;
> > }
> >
> >
> > diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> > index b3731572295e..c08593c63353 100644
> > --- a/arch/powerpc/kvm/book3s_hv.c
> > +++ b/arch/powerpc/kvm/book3s_hv.c
> > @@ -3491,6 +3491,7 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
> > if ((tb & 0xffffff) < (new_tb & 0xffffff))
> > mtspr(SPRN_TBU40, new_tb + 0x1000000);
> > vc->tb_offset_applied = vc->tb_offset;
> > + local_paca->kvm_hstate.tb_offset = vc->tb_offset;
> > }
> >
> > if (vc->pcr)
> > @@ -3594,6 +3595,7 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
> > if ((tb & 0xffffff) < (new_tb & 0xffffff))
> > mtspr(SPRN_TBU40, new_tb + 0x1000000);
> > vc->tb_offset_applied = 0;
> > + local_paca->kvm_hstate.tb_offset = 0;
> > }
> >
> > mtspr(SPRN_HDEC, 0x7fffffff);
> > diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> > index b73140607875..8f7a9f7f4ee6 100644
> > --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> > +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> > @@ -632,6 +632,7 @@ END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_300)
> > cmpdi r8,0
> > beq 37f
> > std r8, VCORE_TB_OFFSET_APPL(r5)
> > + std r8, HSTATE_TB_OFFSET(r13)
> > mftb r6 /* current host timebase */
> > add r8,r8,r6
> > mtspr SPRN_TBU40,r8 /* update upper 40 bits */
> > @@ -1907,6 +1908,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
> > beq 17f
> > li r0, 0
> > std r0, VCORE_TB_OFFSET_APPL(r5)
> > + std r0, HSTATE_TB_OFFSET(r13)
> > mftb r6 /* current guest timebase */
> > subf r8,r8,r6
> > mtspr SPRN_TBU40,r8 /* update upper 40 bits */
^ permalink raw reply
* Re: [RFC PATCH 1/9] KVM: PPC: Book3S 64: move KVM interrupt entry to a common entry point
From: Fabiano Rosas @ 2021-02-05 21:28 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev, Nicholas Piggin
In-Reply-To: <20210202030313.3509446-2-npiggin@gmail.com>
Nicholas Piggin <npiggin@gmail.com> writes:
> Rather than bifurcate the call depending on whether or not HV is
> possible, and have the HV entry test for PR, just make a single
> common point which does the demultiplexing. This makes it simpler
> to add another type of exit handler.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Reviewed-by: Fabiano Rosas <farosas@linux.ibm.com>
> ---
> arch/powerpc/kernel/exceptions-64s.S | 8 +-----
> arch/powerpc/kvm/Makefile | 3 +++
> arch/powerpc/kvm/book3s_64_entry.S | 34 +++++++++++++++++++++++++
> arch/powerpc/kvm/book3s_hv_rmhandlers.S | 11 ++------
> 4 files changed, 40 insertions(+), 16 deletions(-)
> create mode 100644 arch/powerpc/kvm/book3s_64_entry.S
>
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index e02ad6fefa46..65659ea3cec4 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -212,7 +212,6 @@ do_define_int n
> .endm
>
> #ifdef CONFIG_KVM_BOOK3S_64_HANDLER
> -#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> /*
> * All interrupts which set HSRR registers, as well as SRESET and MCE and
> * syscall when invoked with "sc 1" switch to MSR[HV]=1 (HVMODE) to be taken,
> @@ -242,13 +241,8 @@ do_define_int n
>
> /*
> * If an interrupt is taken while a guest is running, it is immediately routed
> - * to KVM to handle. If both HV and PR KVM arepossible, KVM interrupts go first
> - * to kvmppc_interrupt_hv, which handles the PR guest case.
> + * to KVM to handle.
> */
> -#define kvmppc_interrupt kvmppc_interrupt_hv
> -#else
> -#define kvmppc_interrupt kvmppc_interrupt_pr
> -#endif
>
> .macro KVMTEST name
> lbz r10,HSTATE_IN_GUEST(r13)
> diff --git a/arch/powerpc/kvm/Makefile b/arch/powerpc/kvm/Makefile
> index 2bfeaa13befb..cdd119028f64 100644
> --- a/arch/powerpc/kvm/Makefile
> +++ b/arch/powerpc/kvm/Makefile
> @@ -59,6 +59,9 @@ kvm-pr-y := \
> kvm-book3s_64-builtin-objs-$(CONFIG_KVM_BOOK3S_64_HANDLER) += \
> tm.o
>
> +kvm-book3s_64-builtin-objs-y += \
> + book3s_64_entry.o
> +
> ifdef CONFIG_KVM_BOOK3S_PR_POSSIBLE
> kvm-book3s_64-builtin-objs-$(CONFIG_KVM_BOOK3S_64_HANDLER) += \
> book3s_rmhandlers.o
> diff --git a/arch/powerpc/kvm/book3s_64_entry.S b/arch/powerpc/kvm/book3s_64_entry.S
> new file mode 100644
> index 000000000000..22e34b95f478
> --- /dev/null
> +++ b/arch/powerpc/kvm/book3s_64_entry.S
> @@ -0,0 +1,34 @@
> +#include <asm/cache.h>
> +#include <asm/ppc_asm.h>
> +#include <asm/kvm_asm.h>
> +#include <asm/reg.h>
> +#include <asm/asm-offsets.h>
> +#include <asm/kvm_book3s_asm.h>
> +
> +/*
> + * We come here from the first-level interrupt handlers.
> + */
> +.global kvmppc_interrupt
> +.balign IFETCH_ALIGN_BYTES
> +kvmppc_interrupt:
> + /*
> + * Register contents:
> + * R12 = (guest CR << 32) | interrupt vector
> + * R13 = PACA
> + * guest R12 saved in shadow VCPU SCRATCH0
> + * guest R13 saved in SPRN_SCRATCH0
> + */
> +#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> + std r9, HSTATE_SCRATCH2(r13)
> + lbz r9, HSTATE_IN_GUEST(r13)
> + cmpwi r9, KVM_GUEST_MODE_HOST_HV
> + beq kvmppc_bad_host_intr
> +#ifdef CONFIG_KVM_BOOK3S_PR_POSSIBLE
> + cmpwi r9, KVM_GUEST_MODE_GUEST
> + ld r9, HSTATE_SCRATCH2(r13)
> + beq kvmppc_interrupt_pr
> +#endif
> + b kvmppc_interrupt_hv
> +#else
> + b kvmppc_interrupt_pr
> +#endif
> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> index 8cf1f69f442e..b9c4acd747f7 100644
> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> @@ -1255,16 +1255,8 @@ kvmppc_interrupt_hv:
> * R13 = PACA
> * guest R12 saved in shadow VCPU SCRATCH0
> * guest R13 saved in SPRN_SCRATCH0
> + * guest R9 saved in HSTATE_SCRATCH2
> */
> - std r9, HSTATE_SCRATCH2(r13)
> - lbz r9, HSTATE_IN_GUEST(r13)
> - cmpwi r9, KVM_GUEST_MODE_HOST_HV
> - beq kvmppc_bad_host_intr
> -#ifdef CONFIG_KVM_BOOK3S_PR_POSSIBLE
> - cmpwi r9, KVM_GUEST_MODE_GUEST
> - ld r9, HSTATE_SCRATCH2(r13)
> - beq kvmppc_interrupt_pr
> -#endif
> /* We're now back in the host but in guest MMU context */
> li r9, KVM_GUEST_MODE_HOST_HV
> stb r9, HSTATE_IN_GUEST(r13)
> @@ -3253,6 +3245,7 @@ END_FTR_SECTION_IFCLR(CPU_FTR_P9_TM_HV_ASSIST)
> * cfar is saved in HSTATE_CFAR(r13)
> * ppr is saved in HSTATE_PPR(r13)
> */
> +.global kvmppc_bad_host_intr
> kvmppc_bad_host_intr:
> /*
> * Switch to the emergency stack, but start half-way down in
^ permalink raw reply
* Re: [PATCH v2 1/1] powerpc/kvm: Save Timebase Offset to fix sched_clock() while running guest code.
From: Michael Ellerman @ 2021-02-05 23:32 UTC (permalink / raw)
To: Nicholas Piggin, Aneesh Kumar K.V, Athira Rajeev,
Benjamin Herrenschmidt, Christophe Leroy, Frederic Weisbecker,
Geert Uytterhoeven, Jordan Niethe, Leonardo Bras, Paul Mackerras,
Thomas Gleixner
Cc: linuxppc-dev, linux-kernel, kvm-ppc
In-Reply-To: <1612506268.6rrvx34gzu.astroid@bobo.none>
Nicholas Piggin <npiggin@gmail.com> writes:
> Excerpts from Leonardo Bras's message of February 5, 2021 4:06 pm:
>> Before guest entry, TBU40 register is changed to reflect guest timebase.
>> After exitting guest, the register is reverted to it's original value.
>>
>> If one tries to get the timestamp from host between those changes, it
>> will present an incorrect value.
>>
>> An example would be trying to add a tracepoint in
>> kvmppc_guest_entry_inject_int(), which depending on last tracepoint
>> acquired could actually cause the host to crash.
>>
>> Save the Timebase Offset to PACA and use it on sched_clock() to always
>> get the correct timestamp.
>
> Ouch. Not sure how reasonable it is to half switch into guest registers
> and expect to call into the wider kernel, fixing things up as we go.
Yeah it's not.
We need to disable tracing on those routines that are called in that
half-exited state.
cheers
^ permalink raw reply
* Re: [PATCH v7 39/42] powerpc: move NMI entry/exit code into wrapper
From: Michael Ellerman @ 2021-02-05 23:38 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev; +Cc: Athira Rajeev
In-Reply-To: <1612438069.44myr3nzfs.astroid@bobo.none>
Nicholas Piggin <npiggin@gmail.com> writes:
> Excerpts from Michael Ellerman's message of February 4, 2021 8:15 pm:
>> Nicholas Piggin <npiggin@gmail.com> writes:
>>> This moves the common NMI entry and exit code into the interrupt handler
>>> wrappers.
>>>
>>> This changes the behaviour of soft-NMI (watchdog) and HMI interrupts, and
>>> also MCE interrupts on 64e, by adding missing parts of the NMI entry to
>>> them.
>>>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>> arch/powerpc/include/asm/interrupt.h | 28 ++++++++++++++++++++++
>>> arch/powerpc/kernel/mce.c | 11 ---------
>>> arch/powerpc/kernel/traps.c | 35 +++++-----------------------
>>> arch/powerpc/kernel/watchdog.c | 10 ++++----
>>> 4 files changed, 38 insertions(+), 46 deletions(-)
>>
>> This is unhappy when injecting SLB multi-hits:
>>
>> root@p86-2:~# echo PPC_SLB_MULTIHIT > /sys/kernel/debug/provoke-crash/DIRECT
>> [ 312.496026][ T1344] kernel BUG at arch/powerpc/include/asm/interrupt.h:152!
>> [ 312.496037][ T1344] Oops: Exception in kernel mode, sig: 5 [#1]
>> [ 312.496045][ T1344] LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
>
> pseries hash. Blast!
The worst kind.
>> 147 static inline void interrupt_nmi_exit_prepare(struct pt_regs *regs, struct interrupt_nmi_state *state)
>> 148 {
>> 149 if (!IS_ENABLED(CONFIG_PPC_BOOK3S_64) ||
>> 150 !firmware_has_feature(FW_FEATURE_LPAR) ||
>> 151 radix_enabled() || (mfmsr() & MSR_DR))
>> 152 nmi_exit();
>>
>>
>> So presumably it's:
>>
>> #define __nmi_exit() \
>> do { \
>> BUG_ON(!in_nmi()); \
>
> Yes that would be it, pseries machine check enables MMU half way through
> so only one side of this triggers.
>
> The MSR_DR check is supposed to catch the other NMIs that run with MMU
> on (perf, watchdog, etc). Suppose it could test TRAP(regs) explicitly
> although I wonder if we should also do this to keep things balanced
Yeah I think I like that. I'll give it a test.
cheers
> diff --git a/arch/powerpc/platforms/pseries/ras.c b/arch/powerpc/platforms/pseries/ras.c
> index 149cec2212e6..f57ca0c570be 100644
> --- a/arch/powerpc/platforms/pseries/ras.c
> +++ b/arch/powerpc/platforms/pseries/ras.c
> @@ -719,6 +719,7 @@ static int mce_handle_err_virtmode(struct pt_regs *regs,
>
> static int mce_handle_error(struct pt_regs *regs, struct rtas_error_log *errp)
> {
> + unsigned long msr;
> struct pseries_errorlog *pseries_log;
> struct pseries_mc_errorlog *mce_log = NULL;
> int disposition = rtas_error_disposition(errp);
> @@ -747,9 +748,12 @@ static int mce_handle_error(struct pt_regs *regs, struct rtas_error_log *errp)
> * SLB multihit is done by now.
> */
> out:
> - mtmsr(mfmsr() | MSR_IR | MSR_DR);
> + msr = mfmsr();
> + mtmsr(msr | MSR_IR | MSR_DR);
> disposition = mce_handle_err_virtmode(regs, errp, mce_log,
> disposition);
> + mtmsr(msr);
> +
> return disposition;
> }
>
^ permalink raw reply
* Re: [PATCH v3 28/32] powerpc/64s: interrupt implement exit logic in C
From: Nicholas Piggin @ 2021-02-06 2:28 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev, Michael Ellerman; +Cc: Michal Suchanek
In-Reply-To: <d8e6b971-c783-fb5f-f9f2-24e7d8d0726d@csgroup.eu>
Excerpts from Christophe Leroy's message of February 5, 2021 4:04 pm:
>
>
> Le 05/02/2021 à 03:16, Nicholas Piggin a écrit :
>> Excerpts from Michael Ellerman's message of February 5, 2021 10:22 am:
>>> Nicholas Piggin <npiggin@gmail.com> writes:
>>>> Excerpts from Christophe Leroy's message of February 4, 2021 6:03 pm:
>>>>> Le 04/02/2021 à 04:27, Nicholas Piggin a écrit :
>>>>>> Excerpts from Christophe Leroy's message of February 4, 2021 2:25 am:
>>>>>>> Le 25/02/2020 à 18:35, Nicholas Piggin a écrit :
>>> ...
>>>>>>>> +
>>>>>>>> + /*
>>>>>>>> + * We don't need to restore AMR on the way back to userspace for KUAP.
>>>>>>>> + * The value of AMR only matters while we're in the kernel.
>>>>>>>> + */
>>>>>>>> + kuap_restore_amr(regs);
>>>>>>>
>>>>>>> Is that correct to restore KUAP state here ? Shouldn't we have it at lower level in assembly ?
>>>>>>>
>>>>>>> Isn't there a risk that someone manages to call interrupt_exit_kernel_prepare() or the end of it in
>>>>>>> a way or another, and get the previous KUAP state restored by this way ?
>>>>>>
>>>>>> I'm not sure if there much more risk if it's here rather than the
>>>>>> instruction being in another place in the code.
>>>>>>
>>>>>> There's a lot of user access around the kernel too if you want to find a
>>>>>> gadget to unlock KUAP then I suppose there is a pretty large attack
>>>>>> surface.
>>>>>
>>>>> My understanding is that user access scope is strictly limited, for instance we enforce the
>>>>> begin/end of user access to be in the same function, and we refrain from calling any other function
>>>>> inside the user access window. x86 even have 'objtool' to enforce it at build time. So in theory
>>>>> there is no way to get out of the function while user access is open.
>>>>>
>>>>> Here with the interrupt exit function it is free beer. You have a place where you re-open user
>>>>> access and return with a simple blr. So that's open bar. If someone manages to just call the
>>>>> interrupt exit function, then user access remains open
>>>>
>>>> Hmm okay maybe that's a good point.
>>>
>>> I don't think it's a very attractive gadget, it's not just a plain blr,
>>> it does a full stack frame tear down before the return. And there's no
>>> LR reloads anywhere very close.
>>>
>>> Obviously it depends on what the compiler decides to do, it's possible
>>> it could be a usable gadget. But there are other places that are more
>>> attractive I think, eg:
>>>
>>> c00000000061d768: a6 03 3d 7d mtspr 29,r9
>>> c00000000061d76c: 2c 01 00 4c isync
>>> c00000000061d770: 00 00 00 60 nop
>>> c00000000061d774: 30 00 21 38 addi r1,r1,48
>>> c00000000061d778: 20 00 80 4e blr
>>>
>>>
>>> So I don't think we should redesign the code *purely* because we're
>>> worried about interrupt_exit_kernel_prepare() being a useful gadget. If
>>> we can come up with a way to restructure it that reads well and is
>>> maintainable, and also reduces the chance of it being a good gadget then
>>> sure.
>>
>> Okay. That would be good if we can keep it in C, the pkeys + kuap combo
>> is fairly complicated and we might want to something cleverer with it,
>> so that would make it even more difficult in asm.
>>
>
> Ok.
>
> For ppc32, I prefer to keep it in assembly for the time being and move everything from ASM to C at
> once after porting syscall and interrupts to C and wrappers.
>
> Hope this is OK for you.
I don't see a problem with that.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH] powerpc/8xx: Fix software emulation interrupt
From: Nicholas Piggin @ 2021-02-06 2:30 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <ad782af87a222efc79cfb06079b0fd23d4224eaf.1612515180.git.christophe.leroy@csgroup.eu>
Excerpts from Christophe Leroy's message of February 5, 2021 6:56 pm:
> For unimplemented instructions or unimplemented SPRs, the 8xx triggers
> a "Software Emulation Exception" (0x1000). That interrupt doesn't set
> reason bits in SRR1 as the "Program Check Exception" does.
>
> Go through emulation_assist_interrupt() to set REASON_ILLEGAL.
>
> Fixes: fbbcc3bb139e ("powerpc/8xx: Remove SoftwareEmulation()")
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> I'm wondering whether it wouldn't be better to set REASON_ILLEGAL
> in the exception prolog and still call program_check_exception.
> And do the same in book3s/64 to avoid the nightmare of an
> INTERRUPT_HANDLER calling another INTERRUPT_HANDLER.
Hmm, I missed this. We just change program_check_exception to
a common function which is called by both.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v7 28/42] powerpc: convert interrupt handlers to use wrappers
From: Nicholas Piggin @ 2021-02-06 2:43 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: Athira Rajeev
In-Reply-To: <0e319d85-9fa0-ff97-03b2-93637ad89a99@csgroup.eu>
Excerpts from Christophe Leroy's message of February 5, 2021 6:09 pm:
>
>
> Le 30/01/2021 à 14:08, Nicholas Piggin a écrit :
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>
>> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
>> index f70d3f6174c8..7ff915aae8ec 100644
>> --- a/arch/powerpc/kernel/traps.c
>> +++ b/arch/powerpc/kernel/traps.c
>
>> @@ -1462,7 +1474,7 @@ static int emulate_math(struct pt_regs *regs)
>> static inline int emulate_math(struct pt_regs *regs) { return -1; }
>> #endif
>>
>> -void program_check_exception(struct pt_regs *regs)
>> +DEFINE_INTERRUPT_HANDLER(program_check_exception)
>> {
>> enum ctx_state prev_state = exception_enter();
>> unsigned int reason = get_reason(regs);
>> @@ -1587,14 +1599,14 @@ NOKPROBE_SYMBOL(program_check_exception);
>> * This occurs when running in hypervisor mode on POWER6 or later
>> * and an illegal instruction is encountered.
>> */
>> -void emulation_assist_interrupt(struct pt_regs *regs)
>> +DEFINE_INTERRUPT_HANDLER(emulation_assist_interrupt)
>> {
>> regs->msr |= REASON_ILLEGAL;
>> program_check_exception(regs);
>
> Is it correct that an INTERRUPT_HANDLER calls another INTERRUPT_HANDLER ?
No you're right, I'll have to send a patch.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v7 39/42] powerpc: move NMI entry/exit code into wrapper
From: Nicholas Piggin @ 2021-02-06 2:46 UTC (permalink / raw)
To: linuxppc-dev, Michael Ellerman; +Cc: Athira Rajeev
In-Reply-To: <875z36ozkq.fsf@mpe.ellerman.id.au>
Excerpts from Michael Ellerman's message of February 6, 2021 9:38 am:
> Nicholas Piggin <npiggin@gmail.com> writes:
>> Excerpts from Michael Ellerman's message of February 4, 2021 8:15 pm:
>>> Nicholas Piggin <npiggin@gmail.com> writes:
>>>> This moves the common NMI entry and exit code into the interrupt handler
>>>> wrappers.
>>>>
>>>> This changes the behaviour of soft-NMI (watchdog) and HMI interrupts, and
>>>> also MCE interrupts on 64e, by adding missing parts of the NMI entry to
>>>> them.
>>>>
>>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>> ---
>>>> arch/powerpc/include/asm/interrupt.h | 28 ++++++++++++++++++++++
>>>> arch/powerpc/kernel/mce.c | 11 ---------
>>>> arch/powerpc/kernel/traps.c | 35 +++++-----------------------
>>>> arch/powerpc/kernel/watchdog.c | 10 ++++----
>>>> 4 files changed, 38 insertions(+), 46 deletions(-)
>>>
>>> This is unhappy when injecting SLB multi-hits:
>>>
>>> root@p86-2:~# echo PPC_SLB_MULTIHIT > /sys/kernel/debug/provoke-crash/DIRECT
>>> [ 312.496026][ T1344] kernel BUG at arch/powerpc/include/asm/interrupt.h:152!
>>> [ 312.496037][ T1344] Oops: Exception in kernel mode, sig: 5 [#1]
>>> [ 312.496045][ T1344] LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
>>
>> pseries hash. Blast!
>
> The worst kind.
>
>>> 147 static inline void interrupt_nmi_exit_prepare(struct pt_regs *regs, struct interrupt_nmi_state *state)
>>> 148 {
>>> 149 if (!IS_ENABLED(CONFIG_PPC_BOOK3S_64) ||
>>> 150 !firmware_has_feature(FW_FEATURE_LPAR) ||
>>> 151 radix_enabled() || (mfmsr() & MSR_DR))
>>> 152 nmi_exit();
>>>
>>>
>>> So presumably it's:
>>>
>>> #define __nmi_exit() \
>>> do { \
>>> BUG_ON(!in_nmi()); \
>>
>> Yes that would be it, pseries machine check enables MMU half way through
>> so only one side of this triggers.
>>
>> The MSR_DR check is supposed to catch the other NMIs that run with MMU
>> on (perf, watchdog, etc). Suppose it could test TRAP(regs) explicitly
>> although I wonder if we should also do this to keep things balanced
>
> Yeah I think I like that. I'll give it a test.
The msr restore? Looking closer, pseries_machine_check_realmode may have
expected mce_handle_error to enable the MMU, because irq_work_queue uses
some per-cpu variables I think.
So the code might have to be rearranged a bit more than the patch below.
Thanks,
Nick
>
> cheers
>
>
>> diff --git a/arch/powerpc/platforms/pseries/ras.c b/arch/powerpc/platforms/pseries/ras.c
>> index 149cec2212e6..f57ca0c570be 100644
>> --- a/arch/powerpc/platforms/pseries/ras.c
>> +++ b/arch/powerpc/platforms/pseries/ras.c
>> @@ -719,6 +719,7 @@ static int mce_handle_err_virtmode(struct pt_regs *regs,
>>
>> static int mce_handle_error(struct pt_regs *regs, struct rtas_error_log *errp)
>> {
>> + unsigned long msr;
>> struct pseries_errorlog *pseries_log;
>> struct pseries_mc_errorlog *mce_log = NULL;
>> int disposition = rtas_error_disposition(errp);
>> @@ -747,9 +748,12 @@ static int mce_handle_error(struct pt_regs *regs, struct rtas_error_log *errp)
>> * SLB multihit is done by now.
>> */
>> out:
>> - mtmsr(mfmsr() | MSR_IR | MSR_DR);
>> + msr = mfmsr();
>> + mtmsr(msr | MSR_IR | MSR_DR);
>> disposition = mce_handle_err_virtmode(regs, errp, mce_log,
>> disposition);
>> + mtmsr(msr);
>> +
>> return disposition;
>> }
>>
>
^ permalink raw reply
* [PATCH v3] powerpc/kuap: Allow kernel thread to access userspace after kthread_use_mm
From: Aneesh Kumar K.V @ 2021-02-06 2:56 UTC (permalink / raw)
To: linuxppc-dev, mpe
Cc: Jens Axboe, Aneesh Kumar K.V, Zorro Lang, Nicholas Piggin
This fix the bad fault reported by KUAP when io_wqe_worker access userspace.
Bug: Read fault blocked by KUAP!
WARNING: CPU: 1 PID: 101841 at arch/powerpc/mm/fault.c:229 __do_page_fault+0x6b4/0xcd0
NIP [c00000000009e7e4] __do_page_fault+0x6b4/0xcd0
LR [c00000000009e7e0] __do_page_fault+0x6b0/0xcd0
..........
Call Trace:
[c000000016367330] [c00000000009e7e0] __do_page_fault+0x6b0/0xcd0 (unreliable)
[c0000000163673e0] [c00000000009ee3c] do_page_fault+0x3c/0x120
[c000000016367430] [c00000000000c848] handle_page_fault+0x10/0x2c
--- interrupt: 300 at iov_iter_fault_in_readable+0x148/0x6f0
..........
NIP [c0000000008e8228] iov_iter_fault_in_readable+0x148/0x6f0
LR [c0000000008e834c] iov_iter_fault_in_readable+0x26c/0x6f0
interrupt: 300
[c0000000163677e0] [c0000000007154a0] iomap_write_actor+0xc0/0x280
[c000000016367880] [c00000000070fc94] iomap_apply+0x1c4/0x780
[c000000016367990] [c000000000710330] iomap_file_buffered_write+0xa0/0x120
[c0000000163679e0] [c00800000040791c] xfs_file_buffered_aio_write+0x314/0x5e0 [xfs]
[c000000016367a90] [c0000000006d74bc] io_write+0x10c/0x460
[c000000016367bb0] [c0000000006d80e4] io_issue_sqe+0x8d4/0x1200
[c000000016367c70] [c0000000006d8ad0] io_wq_submit_work+0xc0/0x250
[c000000016367cb0] [c0000000006e2578] io_worker_handle_work+0x498/0x800
[c000000016367d40] [c0000000006e2cdc] io_wqe_worker+0x3fc/0x4f0
[c000000016367da0] [c0000000001cb0a4] kthread+0x1c4/0x1d0
[c000000016367e10] [c00000000000dbf0] ret_from_kernel_thread+0x5c/0x6c
The kernel consider thread AMR value for kernel thread to be
AMR_KUAP_BLOCKED. Hence access to userspace is denied. This
of course not correct and we should allow userspace access after
kthread_use_mm(). To be precise, kthread_use_mm() should inherit the
AMR value of the operating address space. But, the AMR value is
thread-specific and we inherit the address space and not thread
access restrictions. Because of this ignore AMR value when accessing
userspace via kernel thread.
current_thread_amr/iamr() are updated, because we use them in the
below stack.
....
[ 530.710838] CPU: 13 PID: 5587 Comm: io_wqe_worker-0 Tainted: G D 5.11.0-rc6+ #3
....
NIP [c0000000000aa0c8] pkey_access_permitted+0x28/0x90
LR [c0000000004b9278] gup_pte_range+0x188/0x420
--- interrupt: 700
[c00000001c4ef3f0] [0000000000000000] 0x0 (unreliable)
[c00000001c4ef490] [c0000000004bd39c] gup_pgd_range+0x3ac/0xa20
[c00000001c4ef5a0] [c0000000004bdd44] internal_get_user_pages_fast+0x334/0x410
[c00000001c4ef620] [c000000000852028] iov_iter_get_pages+0xf8/0x5c0
[c00000001c4ef6a0] [c0000000007da44c] bio_iov_iter_get_pages+0xec/0x700
[c00000001c4ef770] [c0000000006a325c] iomap_dio_bio_actor+0x2ac/0x4f0
[c00000001c4ef810] [c00000000069cd94] iomap_apply+0x2b4/0x740
[c00000001c4ef920] [c0000000006a38b8] __iomap_dio_rw+0x238/0x5c0
[c00000001c4ef9d0] [c0000000006a3c60] iomap_dio_rw+0x20/0x80
[c00000001c4ef9f0] [c008000001927a30] xfs_file_dio_aio_write+0x1f8/0x650 [xfs]
[c00000001c4efa60] [c0080000019284dc] xfs_file_write_iter+0xc4/0x130 [xfs]
[c00000001c4efa90] [c000000000669984] io_write+0x104/0x4b0
[c00000001c4efbb0] [c00000000066cea4] io_issue_sqe+0x3d4/0xf50
[c00000001c4efc60] [c000000000670200] io_wq_submit_work+0xb0/0x2f0
[c00000001c4efcb0] [c000000000674268] io_worker_handle_work+0x248/0x4a0
[c00000001c4efd30] [c0000000006746e8] io_wqe_worker+0x228/0x2a0
[c00000001c4efda0] [c00000000019d994] kthread+0x1b4/0x1c0
Cc: Zorro Lang <zlang@redhat.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
Changes from v2:
* Fix build error
Changes from v1:
* use default_amr/default_iamr for kthread.
arch/powerpc/include/asm/book3s/64/kup.h | 16 +++++++++++-----
arch/powerpc/include/asm/book3s/64/pkeys.h | 4 ----
arch/powerpc/mm/book3s64/pkeys.c | 1 +
3 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/kup.h b/arch/powerpc/include/asm/book3s/64/kup.h
index f50f72e535aa..7d1ef7b9754e 100644
--- a/arch/powerpc/include/asm/book3s/64/kup.h
+++ b/arch/powerpc/include/asm/book3s/64/kup.h
@@ -199,25 +199,31 @@ DECLARE_STATIC_KEY_FALSE(uaccess_flush_key);
#ifdef CONFIG_PPC_PKEY
+extern u64 __ro_after_init default_uamor;
+extern u64 __ro_after_init default_amr;
+extern u64 __ro_after_init default_iamr;
+
#include <asm/mmu.h>
#include <asm/ptrace.h>
-/*
- * For kernel thread that doesn't have thread.regs return
- * default AMR/IAMR values.
+/* usage of kthread_use_mm() should inherit the
+ * AMR value of the operating address space. But, the AMR value is
+ * thread-specific and we inherit the address space and not thread
+ * access restrictions. Because of this ignore AMR value when accessing
+ * userspace via kernel thread.
*/
static inline u64 current_thread_amr(void)
{
if (current->thread.regs)
return current->thread.regs->amr;
- return AMR_KUAP_BLOCKED;
+ return default_amr;
}
static inline u64 current_thread_iamr(void)
{
if (current->thread.regs)
return current->thread.regs->iamr;
- return AMR_KUEP_BLOCKED;
+ return default_iamr;
}
#endif /* CONFIG_PPC_PKEY */
diff --git a/arch/powerpc/include/asm/book3s/64/pkeys.h b/arch/powerpc/include/asm/book3s/64/pkeys.h
index 3b8640498f5b..5b178139f3c0 100644
--- a/arch/powerpc/include/asm/book3s/64/pkeys.h
+++ b/arch/powerpc/include/asm/book3s/64/pkeys.h
@@ -5,10 +5,6 @@
#include <asm/book3s/64/hash-pkey.h>
-extern u64 __ro_after_init default_uamor;
-extern u64 __ro_after_init default_amr;
-extern u64 __ro_after_init default_iamr;
-
static inline u64 vmflag_to_pte_pkey_bits(u64 vm_flags)
{
if (!mmu_has_feature(MMU_FTR_PKEY))
diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
index f1c6f264ed91..15dcc5ad91c5 100644
--- a/arch/powerpc/mm/book3s64/pkeys.c
+++ b/arch/powerpc/mm/book3s64/pkeys.c
@@ -31,6 +31,7 @@ static u32 initial_allocation_mask __ro_after_init;
u64 default_amr __ro_after_init = ~0x0UL;
u64 default_iamr __ro_after_init = 0x5555555555555555UL;
u64 default_uamor __ro_after_init;
+EXPORT_SYMBOL(default_amr);
/*
* Key used to implement PROT_EXEC mmap. Denies READ/WRITE
* We pick key 2 because 0 is special key and 1 is reserved as per ISA.
--
2.29.2
^ permalink raw reply related
* Re: [PATCH v2 1/1] powerpc/kvm: Save Timebase Offset to fix sched_clock() while running guest code.
From: Nicholas Piggin @ 2021-02-06 3:03 UTC (permalink / raw)
To: Aneesh Kumar K.V, Athira Rajeev, Benjamin Herrenschmidt,
Christophe Leroy, Frederic Weisbecker, Geert Uytterhoeven,
Jordan Niethe, Leonardo Bras, Michael Ellerman, Paul Mackerras,
Thomas Gleixner
Cc: linuxppc-dev, linux-kernel, kvm-ppc
In-Reply-To: <7e231b91e41c3f3586ba2fd604c40f1716db228d.camel@gmail.com>
Excerpts from Leonardo Bras's message of February 5, 2021 5:01 pm:
> Hey Nick, thanks for reviewing :)
>
> On Fri, 2021-02-05 at 16:28 +1000, Nicholas Piggin wrote:
>> Excerpts from Leonardo Bras's message of February 5, 2021 4:06 pm:
>> > Before guest entry, TBU40 register is changed to reflect guest timebase.
>> > After exitting guest, the register is reverted to it's original value.
>> >
>> > If one tries to get the timestamp from host between those changes, it
>> > will present an incorrect value.
>> >
>> > An example would be trying to add a tracepoint in
>> > kvmppc_guest_entry_inject_int(), which depending on last tracepoint
>> > acquired could actually cause the host to crash.
>> >
>> > Save the Timebase Offset to PACA and use it on sched_clock() to always
>> > get the correct timestamp.
>>
>> Ouch. Not sure how reasonable it is to half switch into guest registers
>> and expect to call into the wider kernel, fixing things up as we go.
>> What if mftb is used in other places?
>
> IIUC, the CPU is not supposed to call anything as host between guest
> entry and guest exit, except guest-related cases, like
When I say "call", I'm including tracing in that. If a function is not
marked as no trace, then it will call into the tracing subsystem.
> kvmppc_guest_entry_inject_int(), but anyway, if something calls mftb it
> will still get the same value as before.
Right, so it'll be out of whack again.
> This is only supposed to change stuff that depends on sched_clock, like
> Tracepoints, that can happen in those exceptions.
If they depend on sched_clock that's one thing. Do they definitely have
no dependencies on mftb from other calls?
>> Especially as it doesn't seem like there is a reason that function _has_
>> to be called after the timebase is switched to guest, that's just how
>> the code is structured.
>
> Correct, but if called, like in rb routines, used by tracepoints, the
> difference between last tb and current (lower) tb may cause the CPU to
> trap PROGRAM exception, crashing host.
Yes, so I agree with Michael any function that is involved when we begin
to switch into guest context (or have not completed switching back to
host going the other way) should be marked as no trace (noinstr even,
perhaps).
>> As a local hack to work out a bug okay. If you really need it upstream
>> could you put it under a debug config option?
>
> You mean something that is automatically selected whenever those
> configs are enabled?
>
> CONFIG_TRACEPOINT && CONFIG_KVM_BOOK3S_HANDLER && CONFIG_PPC_BOOK3S_64
>
> Or something the user need to select himself in menuconfig?
Yeah I meant a default n thing under powerpc kernel debugging somewhere.
Thanks,
Nick
^ permalink raw reply
* [PATCH v2] powerpc64/idle: Fix SP offsets when saving GPRs
From: Christopher M. Riedl @ 2021-02-06 7:23 UTC (permalink / raw)
To: linuxppc-dev
The idle entry/exit code saves/restores GPRs in the stack "red zone"
(Protected Zone according to PowerPC64 ELF ABI v2). However, the offset
used for the first GPR is incorrect and overwrites the back chain - the
Protected Zone actually starts below the current SP. In practice this is
probably not an issue, but it's still incorrect so fix it.
Also expand the comments to explain why using the stack "red zone"
instead of creating a new stackframe is appropriate here.
Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
---
arch/powerpc/kernel/idle_book3s.S | 138 ++++++++++++++++--------------
1 file changed, 73 insertions(+), 65 deletions(-)
diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S
index 22f249b6f58d..f9e6d83e6720 100644
--- a/arch/powerpc/kernel/idle_book3s.S
+++ b/arch/powerpc/kernel/idle_book3s.S
@@ -52,28 +52,32 @@ _GLOBAL(isa300_idle_stop_mayloss)
std r1,PACAR1(r13)
mflr r4
mfcr r5
- /* use stack red zone rather than a new frame for saving regs */
- std r2,-8*0(r1)
- std r14,-8*1(r1)
- std r15,-8*2(r1)
- std r16,-8*3(r1)
- std r17,-8*4(r1)
- std r18,-8*5(r1)
- std r19,-8*6(r1)
- std r20,-8*7(r1)
- std r21,-8*8(r1)
- std r22,-8*9(r1)
- std r23,-8*10(r1)
- std r24,-8*11(r1)
- std r25,-8*12(r1)
- std r26,-8*13(r1)
- std r27,-8*14(r1)
- std r28,-8*15(r1)
- std r29,-8*16(r1)
- std r30,-8*17(r1)
- std r31,-8*18(r1)
- std r4,-8*19(r1)
- std r5,-8*20(r1)
+ /*
+ * Use the stack red zone rather than a new frame for saving regs since
+ * in the case of no GPR loss the wakeup code branches directly back to
+ * the caller without deallocating the stack frame first.
+ */
+ std r2,-8*1(r1)
+ std r14,-8*2(r1)
+ std r15,-8*3(r1)
+ std r16,-8*4(r1)
+ std r17,-8*5(r1)
+ std r18,-8*6(r1)
+ std r19,-8*7(r1)
+ std r20,-8*8(r1)
+ std r21,-8*9(r1)
+ std r22,-8*10(r1)
+ std r23,-8*11(r1)
+ std r24,-8*12(r1)
+ std r25,-8*13(r1)
+ std r26,-8*14(r1)
+ std r27,-8*15(r1)
+ std r28,-8*16(r1)
+ std r29,-8*17(r1)
+ std r30,-8*18(r1)
+ std r31,-8*19(r1)
+ std r4,-8*20(r1)
+ std r5,-8*21(r1)
/* 168 bytes */
PPC_STOP
b . /* catch bugs */
@@ -89,8 +93,8 @@ _GLOBAL(isa300_idle_stop_mayloss)
*/
_GLOBAL(idle_return_gpr_loss)
ld r1,PACAR1(r13)
- ld r4,-8*19(r1)
- ld r5,-8*20(r1)
+ ld r4,-8*20(r1)
+ ld r5,-8*21(r1)
mtlr r4
mtcr r5
/*
@@ -98,25 +102,25 @@ _GLOBAL(idle_return_gpr_loss)
* from PACATOC. This could be avoided for that less common case
* if KVM saved its r2.
*/
- ld r2,-8*0(r1)
- ld r14,-8*1(r1)
- ld r15,-8*2(r1)
- ld r16,-8*3(r1)
- ld r17,-8*4(r1)
- ld r18,-8*5(r1)
- ld r19,-8*6(r1)
- ld r20,-8*7(r1)
- ld r21,-8*8(r1)
- ld r22,-8*9(r1)
- ld r23,-8*10(r1)
- ld r24,-8*11(r1)
- ld r25,-8*12(r1)
- ld r26,-8*13(r1)
- ld r27,-8*14(r1)
- ld r28,-8*15(r1)
- ld r29,-8*16(r1)
- ld r30,-8*17(r1)
- ld r31,-8*18(r1)
+ ld r2,-8*1(r1)
+ ld r14,-8*2(r1)
+ ld r15,-8*3(r1)
+ ld r16,-8*4(r1)
+ ld r17,-8*5(r1)
+ ld r18,-8*6(r1)
+ ld r19,-8*7(r1)
+ ld r20,-8*8(r1)
+ ld r21,-8*9(r1)
+ ld r22,-8*10(r1)
+ ld r23,-8*11(r1)
+ ld r24,-8*12(r1)
+ ld r25,-8*13(r1)
+ ld r26,-8*14(r1)
+ ld r27,-8*15(r1)
+ ld r28,-8*16(r1)
+ ld r29,-8*17(r1)
+ ld r30,-8*18(r1)
+ ld r31,-8*19(r1)
blr
/*
@@ -154,28 +158,32 @@ _GLOBAL(isa206_idle_insn_mayloss)
std r1,PACAR1(r13)
mflr r4
mfcr r5
- /* use stack red zone rather than a new frame for saving regs */
- std r2,-8*0(r1)
- std r14,-8*1(r1)
- std r15,-8*2(r1)
- std r16,-8*3(r1)
- std r17,-8*4(r1)
- std r18,-8*5(r1)
- std r19,-8*6(r1)
- std r20,-8*7(r1)
- std r21,-8*8(r1)
- std r22,-8*9(r1)
- std r23,-8*10(r1)
- std r24,-8*11(r1)
- std r25,-8*12(r1)
- std r26,-8*13(r1)
- std r27,-8*14(r1)
- std r28,-8*15(r1)
- std r29,-8*16(r1)
- std r30,-8*17(r1)
- std r31,-8*18(r1)
- std r4,-8*19(r1)
- std r5,-8*20(r1)
+ /*
+ * Use the stack red zone rather than a new frame for saving regs since
+ * in the case of no GPR loss the wakeup code branches directly back to
+ * the caller without deallocating the stack frame first.
+ */
+ std r2,-8*1(r1)
+ std r14,-8*2(r1)
+ std r15,-8*3(r1)
+ std r16,-8*4(r1)
+ std r17,-8*5(r1)
+ std r18,-8*6(r1)
+ std r19,-8*7(r1)
+ std r20,-8*8(r1)
+ std r21,-8*9(r1)
+ std r22,-8*10(r1)
+ std r23,-8*11(r1)
+ std r24,-8*12(r1)
+ std r25,-8*13(r1)
+ std r26,-8*14(r1)
+ std r27,-8*15(r1)
+ std r28,-8*16(r1)
+ std r29,-8*17(r1)
+ std r30,-8*18(r1)
+ std r31,-8*19(r1)
+ std r4,-8*20(r1)
+ std r5,-8*21(r1)
cmpwi r3,PNV_THREAD_NAP
bne 1f
IDLE_STATE_ENTER_SEQ_NORET(PPC_NAP)
--
2.26.1
^ permalink raw reply related
* [PATCH 3/3] powerpc/32s: Allow constant folding in mtsr()/mfsr()
From: Christophe Leroy @ 2021-02-06 11:47 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <72c7b9879e2e2e6f5c27dadda6486386c2b50f23.1612612022.git.christophe.leroy@csgroup.eu>
On the same way as we did in wrtee(), add an alternative
using mtsr/mfsr instructions instead of mtsrin/mfsrin
when the segment register can be determined at compile time.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/include/asm/reg.h | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index eeab7e7dc699..da103e92c112 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -1418,14 +1418,20 @@ static inline u32 mfsr(u32 idx)
{
u32 val;
- asm volatile("mfsrin %0, %1" : "=r" (val): "r" (idx));
+ if (__builtin_constant_p(idx))
+ asm volatile("mfsr %0, %1" : "=r" (val): "i" (idx >> 28));
+ else
+ asm volatile("mfsrin %0, %1" : "=r" (val): "r" (idx));
return val;
}
static inline void mtsr(u32 val, u32 idx)
{
- asm volatile("mtsrin %0, %1" : : "r" (val), "r" (idx));
+ if (__builtin_constant_p(idx))
+ asm volatile("mtsr %1, %0" : : "r" (val), "i" (idx >> 28));
+ else
+ asm volatile("mtsrin %0, %1" : : "r" (val), "r" (idx));
}
#endif
--
2.25.0
^ permalink raw reply related
* [PATCH 1/3] powerpc/32s: Change mfsrin() into a static inline function
From: Christophe Leroy @ 2021-02-06 11:47 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
mfsrin() is a macro.
Change in into an inline function to avoid conflicts in KVM
and make it more evolutive.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/include/asm/reg.h | 11 ++++++++---
arch/powerpc/kvm/book3s_emulate.c | 4 ----
2 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index d05dca30604d..f0fb03c9f289 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -1414,9 +1414,14 @@ static inline void msr_check_and_clear(unsigned long bits)
}
#ifdef CONFIG_PPC32
-#define mfsrin(v) ({unsigned int rval; \
- asm volatile("mfsrin %0,%1" : "=r" (rval) : "r" (v)); \
- rval;})
+static inline u32 mfsrin(u32 idx)
+{
+ u32 val;
+
+ asm volatile("mfsrin %0, %1" : "=r" (val): "r" (idx));
+
+ return val;
+}
static inline void mtsrin(u32 val, u32 idx)
{
diff --git a/arch/powerpc/kvm/book3s_emulate.c b/arch/powerpc/kvm/book3s_emulate.c
index b08cc15f31c7..fdb57be71aa6 100644
--- a/arch/powerpc/kvm/book3s_emulate.c
+++ b/arch/powerpc/kvm/book3s_emulate.c
@@ -61,10 +61,6 @@
#define SPRN_GQR6 918
#define SPRN_GQR7 919
-/* Book3S_32 defines mfsrin(v) - but that messes up our abstract
- * function pointers, so let's just disable the define. */
-#undef mfsrin
-
enum priv_level {
PRIV_PROBLEM = 0,
PRIV_SUPER = 1,
--
2.25.0
^ permalink raw reply related
* [PATCH 2/3] powerpc/32s: mfsrin()/mtsrin() become mfsr()/mtsr()
From: Christophe Leroy @ 2021-02-06 11:47 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <72c7b9879e2e2e6f5c27dadda6486386c2b50f23.1612612022.git.christophe.leroy@csgroup.eu>
Function names should tell what the function does, not how.
mfsrin() and mtsrin() are read/writing segment registers.
They are called that way because they are using mfsrin and mtsrin
instructions, but it doesn't matter for the caller.
In preparation of following patch, change their name to mfsr() and mtsr()
in order to make it obvious they manipulate segment registers without
messing up with how they do it.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/include/asm/book3s/32/kup.h | 8 ++++----
arch/powerpc/include/asm/reg.h | 4 ++--
arch/powerpc/mm/book3s32/mmu.c | 2 +-
arch/powerpc/mm/ptdump/segment_regs.c | 2 +-
arch/powerpc/xmon/xmon.c | 2 +-
5 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/32/kup.h b/arch/powerpc/include/asm/book3s/32/kup.h
index e8ea6ac809a9..d5e45eedf581 100644
--- a/arch/powerpc/include/asm/book3s/32/kup.h
+++ b/arch/powerpc/include/asm/book3s/32/kup.h
@@ -95,12 +95,12 @@ static inline void kuap_update_sr(u32 sr, u32 addr, u32 end)
addr &= 0xf0000000; /* align addr to start of segment */
barrier(); /* make sure thread.kuap is updated before playing with SRs */
while (addr < end) {
- mtsrin(sr, addr);
+ mtsr(sr, addr);
sr += 0x111; /* next VSID */
sr &= 0xf0ffffff; /* clear VSID overflow */
addr += 0x10000000; /* address of next segment */
}
- isync(); /* Context sync required after mtsrin() */
+ isync(); /* Context sync required after mtsr() */
}
static __always_inline void allow_user_access(void __user *to, const void __user *from,
@@ -122,7 +122,7 @@ static __always_inline void allow_user_access(void __user *to, const void __user
end = min(addr + size, TASK_SIZE);
current->thread.kuap = (addr & 0xf0000000) | ((((end - 1) >> 28) + 1) & 0xf);
- kuap_update_sr(mfsrin(addr) & ~SR_KS, addr, end); /* Clear Ks */
+ kuap_update_sr(mfsr(addr) & ~SR_KS, addr, end); /* Clear Ks */
}
static __always_inline void prevent_user_access(void __user *to, const void __user *from,
@@ -151,7 +151,7 @@ static __always_inline void prevent_user_access(void __user *to, const void __us
}
current->thread.kuap = 0;
- kuap_update_sr(mfsrin(addr) | SR_KS, addr, end); /* set Ks */
+ kuap_update_sr(mfsr(addr) | SR_KS, addr, end); /* set Ks */
}
static inline unsigned long prevent_user_access_return(void)
diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index f0fb03c9f289..eeab7e7dc699 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -1414,7 +1414,7 @@ static inline void msr_check_and_clear(unsigned long bits)
}
#ifdef CONFIG_PPC32
-static inline u32 mfsrin(u32 idx)
+static inline u32 mfsr(u32 idx)
{
u32 val;
@@ -1423,7 +1423,7 @@ static inline u32 mfsrin(u32 idx)
return val;
}
-static inline void mtsrin(u32 val, u32 idx)
+static inline void mtsr(u32 val, u32 idx)
{
asm volatile("mtsrin %0, %1" : : "r" (val), "r" (idx));
}
diff --git a/arch/powerpc/mm/book3s32/mmu.c b/arch/powerpc/mm/book3s32/mmu.c
index 859e5bd603ac..d7eb266a3f7a 100644
--- a/arch/powerpc/mm/book3s32/mmu.c
+++ b/arch/powerpc/mm/book3s32/mmu.c
@@ -234,7 +234,7 @@ void mmu_mark_initmem_nx(void)
if (is_module_segment(i << 28))
continue;
- mtsrin(mfsrin(i << 28) | 0x10000000, i << 28);
+ mtsr(mfsr(i << 28) | 0x10000000, i << 28);
}
}
diff --git a/arch/powerpc/mm/ptdump/segment_regs.c b/arch/powerpc/mm/ptdump/segment_regs.c
index dde2fe8de4b2..565048a0c9be 100644
--- a/arch/powerpc/mm/ptdump/segment_regs.c
+++ b/arch/powerpc/mm/ptdump/segment_regs.c
@@ -10,7 +10,7 @@
static void seg_show(struct seq_file *m, int i)
{
- u32 val = mfsrin(i << 28);
+ u32 val = mfsr(i << 28);
seq_printf(m, "0x%01x0000000-0x%01xfffffff ", i, i);
seq_printf(m, "Kern key %d ", (val >> 30) & 1);
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index cec432eb9189..3fe37495f63d 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -3719,7 +3719,7 @@ void dump_segments(void)
printf("sr0-15 =");
for (i = 0; i < 16; ++i)
- printf(" %x", mfsrin(i << 28));
+ printf(" %x", mfsr(i << 28));
printf("\n");
}
#endif
--
2.25.0
^ permalink raw reply related
* [GIT PULL] Please pull powerpc/linux.git powerpc-5.11-7 tag
From: Michael Ellerman @ 2021-02-06 12:50 UTC (permalink / raw)
To: Linus Torvalds
Cc: ravi.bangoria, masahiroy, linux-kernel, npiggin, linuxppc-dev,
raoni
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Hi Linus,
Please pull some more powerpc fixes for 5.11:
The following changes since commit 4025c784c573cab7e3f84746cc82b8033923ec62:
powerpc/64s: prevent recursive replay_soft_interrupts causing superfluous interrupt (2021-01-24 22:27:24 +1100)
are available in the git repository at:
https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git tags/powerpc-5.11-7
for you to fetch changes up to 24321ac668e452a4942598533d267805f291fdc9:
powerpc/64/signal: Fix regression in __kernel_sigtramp_rt64() semantics (2021-02-02 22:14:41 +1100)
- ------------------------------------------------------------------
powerpc fixes for 5.11 #7
A fix for a change we made to __kernel_sigtramp_rt64() which confused glibc's
backtrace logic, and also changed the semantics of that symbol, which was
arguably an ABI break.
A fix for a stack overwrite in our VSX instruction emulation.
A couple of fixes for the Makefile logic in the new C VDSO.
Thanks to: Masahiro Yamada, Naveen N. Rao, Raoni Fassina Firmino, Ravi Bangoria.
- ------------------------------------------------------------------
Masahiro Yamada (2):
powerpc/vdso: fix unnecessary rebuilds of vgettimeofday.o
powerpc/vdso64: remove meaningless vgettimeofday.o build rule
Raoni Fassina Firmino (1):
powerpc/64/signal: Fix regression in __kernel_sigtramp_rt64() semantics
Ravi Bangoria (1):
powerpc/sstep: Fix array out of bound warning
arch/powerpc/kernel/Makefile | 4 ++--
arch/powerpc/kernel/vdso32/Makefile | 5 +----
arch/powerpc/kernel/{vdso32 => }/vdso32_wrapper.S | 0
arch/powerpc/kernel/vdso64/Makefile | 8 +-------
arch/powerpc/kernel/vdso64/sigtramp.S | 11 ++++++++++-
arch/powerpc/kernel/vdso64/vdso64.lds.S | 2 +-
arch/powerpc/kernel/{vdso64 => }/vdso64_wrapper.S | 0
arch/powerpc/lib/sstep.c | 14 ++++++++------
8 files changed, 23 insertions(+), 21 deletions(-)
rename arch/powerpc/kernel/{vdso32 => }/vdso32_wrapper.S (100%)
rename arch/powerpc/kernel/{vdso64 => }/vdso64_wrapper.S (100%)
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEEJFGtCPCthwEv2Y/bUevqPMjhpYAFAmAekG8ACgkQUevqPMjh
pYAHKw/+JNrzuw4P67ZqPcOMKayKAmUzBvd1ED/NBQtvtZpJS78tUVfps8DIr01g
i+UnlOohaavtGT0ARPTp7wEGL2c7vyzoNyxPdVru0x3UWq8xvnpdKfiHut7LjLot
32/aS9/rChr++JE7UeVVabYxvZJy9RSQg1DjcEi08X+LOwe2yJorME/4hVD3qjEV
FOIPbdsOOFyI3RWMsp3UFG3rwdJreHImSfZIlHUBCGHbLs5bVLC2CyoXN2UYTToS
DrD4C1R8a9wBTYcHOEJHAPCoeYHlELROeyEWbTapGGFlpMERUyhWxoEJNrkPt7Ym
3wQjlfI2g1oTu1ewi4D4QUHsnZHfwXvVAMzY84LvPRFXZrzJuvh17Dx9VD65ALfc
zyzasuecdEXlbGYilJHAA1m8Qlgm4utxzA+5BDv4UgegBVCopWjo2I1Ai+7vFo6S
58joA+G0m1E2QfbCQeEderBqe34kGMGcbzQFb8CQq7UnIG88U4JclCFbogOxDCdW
Ogu93sJpMRT2pNorBOk//clNBgdzm+PLE1uywZ7t2rLUN58SehBWM8SLDJgVWVfY
iPpN5hRJmEKhkT5Zq+l8Muvn2XsCc9jW0wZJW5k4jcEnwFfIsKBej2Qgiqm1y0D6
oFk9Wsp0K2YTo7G4ViL45r6RjlXVVfGzkK0t9LzTFV9znYaI5NE=
=dwna
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [PATCH 2/8] powerpc/signal: Add unsafe_copy_{vsx, fpr}_from_user()
From: Christophe Leroy @ 2021-02-06 16:32 UTC (permalink / raw)
To: Christopher M. Riedl, linuxppc-dev
In-Reply-To: <C6HCJAJH1QOC.1U5G1AHURT3NJ@geist>
Le 20/10/2020 à 04:01, Christopher M. Riedl a écrit :
> On Fri Oct 16, 2020 at 10:48 AM CDT, Christophe Leroy wrote:
>>
>>
>> Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
>>> Reuse the "safe" implementation from signal.c except for calling
>>> unsafe_copy_from_user() to copy into a local buffer. Unlike the
>>> unsafe_copy_{vsx,fpr}_to_user() functions the "copy from" functions
>>> cannot use unsafe_get_user() directly to bypass the local buffer since
>>> doing so significantly reduces signal handling performance.
>>
>> Why can't the functions use unsafe_get_user(), why does it significantly
>> reduces signal handling
>> performance ? How much significant ? I would expect that not going
>> through an intermediate memory
>> area would be more efficient
>>
>
> Here is a comparison, 'unsafe-signal64-regs' avoids the intermediate buffer:
>
> | | hash | radix |
> | -------------------- | ------ | ------ |
> | linuxppc/next | 289014 | 158408 |
> | unsafe-signal64 | 298506 | 253053 |
> | unsafe-signal64-regs | 254898 | 220831 |
>
> I have not figured out the 'why' yet. As you mentioned in your series,
> technically calling __copy_tofrom_user() is overkill for these
> operations. The only obvious difference between unsafe_put_user() and
> unsafe_get_user() is that we don't have asm-goto for the 'get' variant.
> Instead we wrap with unsafe_op_wrap() which inserts a conditional and
> then goto to the label.
>
> Implemenations:
>
> #define unsafe_copy_fpr_from_user(task, from, label) do { \
> struct task_struct *__t = task; \
> u64 __user *buf = (u64 __user *)from; \
> int i; \
> \
> for (i = 0; i < ELF_NFPREG - 1; i++) \
> unsafe_get_user(__t->thread.TS_FPR(i), &buf[i], label); \
> unsafe_get_user(__t->thread.fp_state.fpscr, &buf[i], label); \
> } while (0)
>
> #define unsafe_copy_vsx_from_user(task, from, label) do { \
> struct task_struct *__t = task; \
> u64 __user *buf = (u64 __user *)from; \
> int i; \
> \
> for (i = 0; i < ELF_NVSRHALFREG ; i++) \
> unsafe_get_user(__t->thread.fp_state.fpr[i][TS_VSRLOWOFFSET], \
> &buf[i], label); \
> } while (0)
>
Do you have CONFIG_PROVE_LOCKING or CONFIG_DEBUG_ATOMIC_SLEEP enabled in your config ?
If yes, could you try together with the patch from Alexey
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20210204121612.32721-1-aik@ozlabs.ru/ ?
Thanks
Christophe
^ permalink raw reply
* Re: [PATCH 2/8] powerpc/signal: Add unsafe_copy_{vsx,fpr}_from_user()
From: Christopher M. Riedl @ 2021-02-06 17:39 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
In-Reply-To: <fce0b2d0-58a3-a94d-a8e9-d104fc2b3058@csgroup.eu>
On Sat Feb 6, 2021 at 10:32 AM CST, Christophe Leroy wrote:
>
>
> Le 20/10/2020 à 04:01, Christopher M. Riedl a écrit :
> > On Fri Oct 16, 2020 at 10:48 AM CDT, Christophe Leroy wrote:
> >>
> >>
> >> Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
> >>> Reuse the "safe" implementation from signal.c except for calling
> >>> unsafe_copy_from_user() to copy into a local buffer. Unlike the
> >>> unsafe_copy_{vsx,fpr}_to_user() functions the "copy from" functions
> >>> cannot use unsafe_get_user() directly to bypass the local buffer since
> >>> doing so significantly reduces signal handling performance.
> >>
> >> Why can't the functions use unsafe_get_user(), why does it significantly
> >> reduces signal handling
> >> performance ? How much significant ? I would expect that not going
> >> through an intermediate memory
> >> area would be more efficient
> >>
> >
> > Here is a comparison, 'unsafe-signal64-regs' avoids the intermediate buffer:
> >
> > | | hash | radix |
> > | -------------------- | ------ | ------ |
> > | linuxppc/next | 289014 | 158408 |
> > | unsafe-signal64 | 298506 | 253053 |
> > | unsafe-signal64-regs | 254898 | 220831 |
> >
> > I have not figured out the 'why' yet. As you mentioned in your series,
> > technically calling __copy_tofrom_user() is overkill for these
> > operations. The only obvious difference between unsafe_put_user() and
> > unsafe_get_user() is that we don't have asm-goto for the 'get' variant.
> > Instead we wrap with unsafe_op_wrap() which inserts a conditional and
> > then goto to the label.
> >
> > Implemenations:
> >
> > #define unsafe_copy_fpr_from_user(task, from, label) do { \
> > struct task_struct *__t = task; \
> > u64 __user *buf = (u64 __user *)from; \
> > int i; \
> > \
> > for (i = 0; i < ELF_NFPREG - 1; i++) \
> > unsafe_get_user(__t->thread.TS_FPR(i), &buf[i], label); \
> > unsafe_get_user(__t->thread.fp_state.fpscr, &buf[i], label); \
> > } while (0)
> >
> > #define unsafe_copy_vsx_from_user(task, from, label) do { \
> > struct task_struct *__t = task; \
> > u64 __user *buf = (u64 __user *)from; \
> > int i; \
> > \
> > for (i = 0; i < ELF_NVSRHALFREG ; i++) \
> > unsafe_get_user(__t->thread.fp_state.fpr[i][TS_VSRLOWOFFSET], \
> > &buf[i], label); \
> > } while (0)
> >
>
> Do you have CONFIG_PROVE_LOCKING or CONFIG_DEBUG_ATOMIC_SLEEP enabled in
> your config ?
I don't have these set in my config (ppc64le_defconfig). I think I
figured this out - the reason for the lower signal throughput is the
barrier_nospec() in __get_user_nocheck(). When looping we incur that
cost on every iteration. Commenting it out results in signal performance
of ~316K w/ hash on the unsafe-signal64-regs branch. Obviously the
barrier is there for a reason but it is quite costly.
This also explains why the copy_{fpr,vsx}_to_user() direction does not
suffer from the slowdown because there is no need for barrier_nospec().
>
> If yes, could you try together with the patch from Alexey
> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20210204121612.32721-1-aik@ozlabs.ru/
> ?
>
> Thanks
> Christophe
^ permalink raw reply
* Re: [PATCH v2] powerpc/uprobes: Validation for prefixed instruction
From: Oleg Nesterov @ 2021-02-06 18:06 UTC (permalink / raw)
To: Ravi Bangoria
Cc: linux-kernel, rostedt, paulus, sandipan, jniethe5, naveen.n.rao,
linuxppc-dev
In-Reply-To: <20210204104703.273429-1-ravi.bangoria@linux.ibm.com>
On 02/04, Ravi Bangoria wrote:
>
> +static int get_instr(struct mm_struct *mm, unsigned long addr, u32 *instr)
> +{
> + struct page *page;
> + struct vm_area_struct *vma;
> + void *kaddr;
> + unsigned int gup_flags = FOLL_FORCE | FOLL_SPLIT_PMD;
> +
> + if (get_user_pages_remote(mm, addr, 1, gup_flags, &page, &vma, NULL) <= 0)
> + return -EINVAL;
"vma" is not used, and I don't think you need FOLL_SPLIT_PMD.
Otherwise I can't really comment this ppc-specific change.
To be honest, I don't even understand why do we need this fix. Sure, the
breakpoint in the middle of 64-bit insn won't work, why do we care? The
user should know what does he do.
Not to mention we can't really trust get_user_pages() in that this page
can be modified by mm owner or debugger...
But I won't argue.
Oleg.
^ permalink raw reply
* [PATCH] ASoC: fsl: constify static snd_soc_dai_ops structs
From: Rikard Falkeborn @ 2021-02-06 22:58 UTC (permalink / raw)
To: Timur Tabi, Nicolin Chen, Xiubo Li, Mark Brown, Jaroslav Kysela,
Takashi Iwai
Cc: alsa-devel, linuxppc-dev, Liam Girdwood, Rikard Falkeborn,
linux-kernel, Fabio Estevam, Shengjiu Wang
The only usage of these is to assign their address to the 'ops' field in
the snd_soc_dai_driver struct, which is a pointer to const. Make them
const to allow the compiler to put them in read-only memory.
Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
---
sound/soc/fsl/fsl_easrc.c | 2 +-
sound/soc/fsl/fsl_micfil.c | 2 +-
sound/soc/fsl/fsl_xcvr.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/sound/soc/fsl/fsl_easrc.c b/sound/soc/fsl/fsl_easrc.c
index 60951a8aabd3..636a702f37a6 100644
--- a/sound/soc/fsl/fsl_easrc.c
+++ b/sound/soc/fsl/fsl_easrc.c
@@ -1530,7 +1530,7 @@ static int fsl_easrc_hw_free(struct snd_pcm_substream *substream,
return 0;
}
-static struct snd_soc_dai_ops fsl_easrc_dai_ops = {
+static const struct snd_soc_dai_ops fsl_easrc_dai_ops = {
.startup = fsl_easrc_startup,
.trigger = fsl_easrc_trigger,
.hw_params = fsl_easrc_hw_params,
diff --git a/sound/soc/fsl/fsl_micfil.c b/sound/soc/fsl/fsl_micfil.c
index 8aedf6590b28..5935af2e5ff4 100644
--- a/sound/soc/fsl/fsl_micfil.c
+++ b/sound/soc/fsl/fsl_micfil.c
@@ -381,7 +381,7 @@ static int fsl_micfil_set_dai_sysclk(struct snd_soc_dai *dai, int clk_id,
return ret;
}
-static struct snd_soc_dai_ops fsl_micfil_dai_ops = {
+static const struct snd_soc_dai_ops fsl_micfil_dai_ops = {
.startup = fsl_micfil_startup,
.trigger = fsl_micfil_trigger,
.hw_params = fsl_micfil_hw_params,
diff --git a/sound/soc/fsl/fsl_xcvr.c b/sound/soc/fsl/fsl_xcvr.c
index dd228b421e2c..6dd0a5fcd455 100644
--- a/sound/soc/fsl/fsl_xcvr.c
+++ b/sound/soc/fsl/fsl_xcvr.c
@@ -857,7 +857,7 @@ static struct snd_kcontrol_new fsl_xcvr_tx_ctls[] = {
},
};
-static struct snd_soc_dai_ops fsl_xcvr_dai_ops = {
+static const struct snd_soc_dai_ops fsl_xcvr_dai_ops = {
.prepare = fsl_xcvr_prepare,
.startup = fsl_xcvr_startup,
.shutdown = fsl_xcvr_shutdown,
--
2.30.0
^ permalink raw reply related
* Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.11-7 tag
From: pr-tracker-bot @ 2021-02-06 23:21 UTC (permalink / raw)
To: Michael Ellerman
Cc: ravi.bangoria, Linus Torvalds, masahiroy, linux-kernel, npiggin,
linuxppc-dev, raoni
In-Reply-To: <8735y9pdga.fsf@mpe.ellerman.id.au>
The pull request you sent on Sat, 06 Feb 2021 23:50:45 +1100:
> https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git tags/powerpc-5.11-7
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/f06279ea1908b9cd2d22645dc6d492e612b82744
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html
^ permalink raw reply
* [PATCH] powerpc/xive: Assign boolean values to a bool variable
From: Jiapeng Chong @ 2021-02-07 6:43 UTC (permalink / raw)
To: paulus; +Cc: Jiapeng Chong, linux-kernel, kvm-ppc, linuxppc-dev
Fix the following coccicheck warnings:
./arch/powerpc/kvm/book3s_xive.c:1856:2-17: WARNING: Assignment of 0/1
to bool variable.
./arch/powerpc/kvm/book3s_xive.c:1854:2-17: WARNING: Assignment of 0/1
to bool variable.
Reported-by: Abaci Robot <abaci@linux.alibaba.com>
Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
---
arch/powerpc/kvm/book3s_xive.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
index 30dfeac..e7219b6 100644
--- a/arch/powerpc/kvm/book3s_xive.c
+++ b/arch/powerpc/kvm/book3s_xive.c
@@ -1813,9 +1813,9 @@ int kvmppc_xive_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level,
return -EINVAL;
if ((level == 1 && state->lsi) || level == KVM_INTERRUPT_SET_LEVEL)
- state->asserted = 1;
+ state->asserted = true;
else if (level == 0 || level == KVM_INTERRUPT_UNSET) {
- state->asserted = 0;
+ state->asserted = false;
return 0;
}
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH 2/7] ASoC: fsl_rpmsg: Add CPU DAI driver for audio base on rpmsg
From: Shengjiu Wang @ 2021-02-07 9:49 UTC (permalink / raw)
To: Mark Brown
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
alsa-devel, Timur Tabi, Xiubo Li, Fabio Estevam, Shengjiu Wang,
Takashi Iwai, Liam Girdwood, linux-kernel, Nicolin Chen,
Rob Herring, linuxppc-dev
In-Reply-To: <20210205140251.GB4720@sirena.org.uk>
On Fri, Feb 5, 2021 at 10:04 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Feb 05, 2021 at 02:57:25PM +0800, Shengjiu Wang wrote:
> > This is a dummy cpu dai driver for rpmsg audio use case,
> > which is mainly used for getting the user's configuration
>
> This is actually doing stuff, it's not a dummy driver.
>
> > +static int fsl_rpmsg_remove(struct platform_device *pdev)
> > +{
> > + return 0;
> > +}
>
> If this isn't needed just remove it.
Thanks Mark. I will update them.
Best regards
Wang shengjiu
^ permalink raw reply
* Re: [PATCH 4/7] ASoC: imx-audio-rpmsg: Add rpmsg_driver for audio channel
From: Shengjiu Wang @ 2021-02-07 9:49 UTC (permalink / raw)
To: Mark Brown
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
alsa-devel, Timur Tabi, Xiubo Li, Fabio Estevam, Shengjiu Wang,
Takashi Iwai, Liam Girdwood, linux-kernel, Nicolin Chen,
Rob Herring, linuxppc-dev
In-Reply-To: <20210205142516.GC4720@sirena.org.uk>
On Fri, Feb 5, 2021 at 10:27 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Feb 05, 2021 at 02:57:27PM +0800, Shengjiu Wang wrote:
>
> > + /* TYPE C is notification from M core */
> > + if (r_msg->header.type == MSG_TYPE_C) {
> > + if (r_msg->header.cmd == TX_PERIOD_DONE) {
>
> > + } else if (r_msg->header.cmd == RX_PERIOD_DONE) {
>
> A switch statement would be clearer and more extensible...
>
> > + /* TYPE B is response msg */
> > + if (r_msg->header.type == MSG_TYPE_B) {
> > + memcpy(&info->r_msg, r_msg, sizeof(struct rpmsg_r_msg));
> > + complete(&info->cmd_complete);
> > + }
>
> ...and make this flow clearer for example. Do we need to warn on
> unknown messages?
Thanks for reviewing. I will update them.
Best regards
wang shengjiu
^ permalink raw reply
* Re: [PATCH 5/7] ASoC: imx-pcm-rpmsg: Add platform driver for audio base on rpmsg
From: Shengjiu Wang @ 2021-02-07 9:50 UTC (permalink / raw)
To: Mark Brown
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
alsa-devel, Timur Tabi, Xiubo Li, Fabio Estevam, Shengjiu Wang,
Takashi Iwai, Liam Girdwood, linux-kernel, Nicolin Chen,
Rob Herring, linuxppc-dev
In-Reply-To: <20210205145816.GD4720@sirena.org.uk>
On Fri, Feb 5, 2021 at 11:00 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Feb 05, 2021 at 02:57:28PM +0800, Shengjiu Wang wrote:
>
> > + if (params_format(params) == SNDRV_PCM_FORMAT_S16_LE)
> > + msg->s_msg.param.format = RPMSG_S16_LE;
> > + else if (params_format(params) == SNDRV_PCM_FORMAT_S24_LE)
>
> Again this should be a switch statement.
>
> > + if (params_channels(params) == 1)
> > + msg->s_msg.param.channels = RPMSG_CH_LEFT;
> > + else
> > + msg->s_msg.param.channels = RPMSG_CH_STEREO;
>
> Shouldn't this be reporting an error if the number of channels is more
> than 2?
>
> > + /*
> > + * if the data in the buffer is less than one period
> > + * send message immediately.
> > + * if there is more than one period data, delay one
> > + * period (timer) to send the message.
> > + */
> > + if ((avail - writen_num * period_size) <= period_size) {
> > + imx_rpmsg_insert_workqueue(substream, msg, info);
> > + } else if (rpmsg->force_lpa && !timer_pending(timer)) {
> > + int time_msec;
> > +
> > + time_msec = (int)(runtime->period_size * 1000 / runtime->rate);
> > + mod_timer(timer, jiffies + msecs_to_jiffies(time_msec));
> > + }
>
> The comment here is at least confusing - why would we not send a full
> buffer immediately if we have one? This sounds like it's the opposite
> way round to what we'd do if we were trying to cut down the number of
> messages. It might help to say which buffer and where?
>
> > + /**
> > + * Every work in the work queue, first we check if there
>
> /** comments are only for kerneldoc.
Thanks Mark, I will update them.
Best regards
wang shengjiu
^ permalink raw reply
* [PATCH] powerpc/uaccess: Perform barrier_nospec() in KUAP allowance helpers
From: Christophe Leroy @ 2021-02-07 10:08 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, cmr
Cc: linuxppc-dev, linux-kernel
barrier_nospec() in uaccess helpers is there to protect against
speculative accesses around access_ok().
When using user_access_begin() sequences together with
unsafe_get_user() like macros, barrier_nospec() is called for
every single read although we know the access_ok() is done
onece.
Since all user accesses must be granted by a call to either
allow_read_from_user() or allow_read_write_user() which will
always happen after the access_ok() check, move the barrier_nospec()
there.
Reported-by: Christopher M. Riedl <cmr@codefail.de>
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/include/asm/kup.h | 2 ++
arch/powerpc/include/asm/uaccess.h | 12 +-----------
2 files changed, 3 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h
index bf221a2a523e..7ec21af49a45 100644
--- a/arch/powerpc/include/asm/kup.h
+++ b/arch/powerpc/include/asm/kup.h
@@ -91,6 +91,7 @@ static __always_inline void setup_kup(void)
static inline void allow_read_from_user(const void __user *from, unsigned long size)
{
+ barrier_nospec();
allow_user_access(NULL, from, size, KUAP_READ);
}
@@ -102,6 +103,7 @@ static inline void allow_write_to_user(void __user *to, unsigned long size)
static inline void allow_read_write_user(void __user *to, const void __user *from,
unsigned long size)
{
+ barrier_nospec();
allow_user_access(to, from, size, KUAP_READ_WRITE);
}
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 501c9a79038c..46123ae6a4c9 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -315,7 +315,6 @@ do { \
__chk_user_ptr(__gu_addr); \
if (!is_kernel_addr((unsigned long)__gu_addr)) \
might_fault(); \
- barrier_nospec(); \
if (do_allow) \
__get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \
else \
@@ -333,10 +332,8 @@ do { \
__typeof__(size) __gu_size = (size); \
\
might_fault(); \
- if (access_ok(__gu_addr, __gu_size)) { \
- barrier_nospec(); \
+ if (access_ok(__gu_addr, __gu_size)) \
__get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \
- } \
(x) = (__force __typeof__(*(ptr)))__gu_val; \
\
__gu_err; \
@@ -350,7 +347,6 @@ do { \
__typeof__(size) __gu_size = (size); \
\
__chk_user_ptr(__gu_addr); \
- barrier_nospec(); \
__get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \
(x) = (__force __typeof__(*(ptr)))__gu_val; \
\
@@ -395,7 +391,6 @@ raw_copy_in_user(void __user *to, const void __user *from, unsigned long n)
{
unsigned long ret;
- barrier_nospec();
allow_read_write_user(to, from, n);
ret = __copy_tofrom_user(to, from, n);
prevent_read_write_user(to, from, n);
@@ -412,19 +407,15 @@ static inline unsigned long raw_copy_from_user(void *to,
switch (n) {
case 1:
- barrier_nospec();
__get_user_size(*(u8 *)to, from, 1, ret);
break;
case 2:
- barrier_nospec();
__get_user_size(*(u16 *)to, from, 2, ret);
break;
case 4:
- barrier_nospec();
__get_user_size(*(u32 *)to, from, 4, ret);
break;
case 8:
- barrier_nospec();
__get_user_size(*(u64 *)to, from, 8, ret);
break;
}
@@ -432,7 +423,6 @@ static inline unsigned long raw_copy_from_user(void *to,
return 0;
}
- barrier_nospec();
allow_read_from_user(from, n);
ret = __copy_tofrom_user((__force void __user *)to, from, n);
prevent_read_from_user(from, n);
--
2.25.0
^ permalink raw reply related
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