* Re: [PATCH] powerpc: Fix instruction encoding for lis in ppc_function_entry()
From: Christophe Leroy @ 2021-03-05 11:37 UTC (permalink / raw)
To: Naveen N. Rao, linuxppc-dev; +Cc: Jiri Olsa
In-Reply-To: <20210304020411.16796-1-naveen.n.rao@linux.vnet.ibm.com>
Le 04/03/2021 à 03:04, Naveen N. Rao a écrit :
> 'lis r2,N' is 'addis r2,0,N' and the instruction encoding in the macro
> LIS_R2 is incorrect (it currently maps to 'addis 0,r2,N'). Fix the same.
>
> Fixes: c71b7eff426fa7 ("powerpc: Add ABIv2 support to ppc_function_entry")
> Reported-by: Jiri Olsa <jolsa@redhat.com>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
> arch/powerpc/include/asm/code-patching.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
> index eacc9102c2515c..d5b3c3bb95b400 100644
> --- a/arch/powerpc/include/asm/code-patching.h
> +++ b/arch/powerpc/include/asm/code-patching.h
> @@ -73,7 +73,7 @@ void __patch_exception(int exc, unsigned long addr);
> #endif
>
> #define OP_RT_RA_MASK 0xffff0000UL
> -#define LIS_R2 0x3c020000UL
> +#define LIS_R2 0x3c400000UL
> #define ADDIS_R2_R12 0x3c4c0000UL
> #define ADDI_R2_R2 0x38420000UL
That probably goes beyond the scope of this patch, but it would be more readable and less error
prone to use macros defined in ppc-opcode.h just like kernel/module_64.c does for instance:
#define LIS_R2 (PPC_INST_ADDIS | __PPC_RT(R2))
#define ADDIS_R2_R12 (PPC_INST_ADDIS | __PPC_RT(R2) | __PPC_RA(R12))
#define ADDI_R2_R2 (PPC_INST_ADDI | __PPC_RT(R2) | __PPC_RA(R2))
>
>
> base-commit: 91966823812efbd175f904599e5cf2a854b39809
>
^ permalink raw reply
* Re: Build regressions/improvements in v5.12-rc1
From: Christophe Leroy @ 2021-03-05 11:16 UTC (permalink / raw)
To: Geert Uytterhoeven, Alex Deucher
Cc: Alex Deucher, linuxppc-dev, LKML, amd-gfx list,
Christian König
In-Reply-To: <CAMuHMdVFstnce-WKmj=4h3ZdtSThJNOLz_f1ervcZxE6hg=KsA@mail.gmail.com>
Le 02/03/2021 à 21:01, Geert Uytterhoeven a écrit :
> Hi Alex,
>
> On Tue, Mar 2, 2021 at 8:30 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>> On Mon, Mar 1, 2021 at 9:21 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Mon, 1 Mar 2021, Geert Uytterhoeven wrote:
>>>> Below is the list of build error/warning regressions/improvements in
>>>> v5.12-rc1[1] compared to v5.11[2].
>>>>
>>>> Summarized:
>>>> - build errors: +2/-0
>>>
>>>> [1] http://kisskb.ellerman.id.au/kisskb/branch/linus/head/fe07bfda2fb9cdef8a4d4008a409bb02f35f1bd8/ (all 192 configs)
>>>> [2] http://kisskb.ellerman.id.au/kisskb/branch/linus/head/f40ddce88593482919761f74910f42f4b84c004b/ (all 192 configs)
>>>>
>>>>
>>>> *** ERRORS ***
>>>>
>>>> 2 error regressions:
>>>> + /kisskb/src/drivers/gpu/drm/amd/amdgpu/../display/dc/calcs/dcn_calcs.c: error: implicit declaration of function 'disable_kernel_vsx' [-Werror=implicit-function-declaration]: => 674:2
>>>> + /kisskb/src/drivers/gpu/drm/amd/amdgpu/../display/dc/calcs/dcn_calcs.c: error: implicit declaration of function 'enable_kernel_vsx' [-Werror=implicit-function-declaration]: => 638:2
>>>
>>> powerpc-gcc4.9/ppc64_book3e_allmodconfig
>>>
>>> This was fixed in v5.11-rc1, but reappeared in v5.12-rc1?
>>
>> Do you know what fixed in for 5.11? I guess for PPC64 we depend on CONFIG_VSX?
>
> Looking at the kisskb build logs for v5.11*, it seems compilation never
> got to drivers/gpu/drm/ due to internal compiler errors that weren't caught
> by my scripts. So the errors listed above were not really fixed.
>
As far as I can see, the problem has been there for any config without CONFIG_VSX from the beginning
ie since https://github.com/linuxppc/linux/commit/16a9dea110a6
The following should fix it up:
diff --git a/arch/powerpc/include/asm/switch_to.h b/arch/powerpc/include/asm/switch_to.h
index fdab93428372..9d1fbd8be1c7 100644
--- a/arch/powerpc/include/asm/switch_to.h
+++ b/arch/powerpc/include/asm/switch_to.h
@@ -71,6 +71,16 @@ static inline void disable_kernel_vsx(void)
{
msr_check_and_clear(MSR_FP|MSR_VEC|MSR_VSX);
}
+#else
+static inline void enable_kernel_vsx(void)
+{
+ BUILD_BUG();
+}
+
+static inline void disable_kernel_vsx(void)
+{
+ BUILD_BUG();
+}
#endif
#ifdef CONFIG_SPE
---
Christophe
Christophe
^ permalink raw reply related
* [PATCH] powerpc: Fix instruction encoding for lis in ppc_function_entry()
From: Naveen N. Rao @ 2021-03-04 2:04 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Jiri Olsa
'lis r2,N' is 'addis r2,0,N' and the instruction encoding in the macro
LIS_R2 is incorrect (it currently maps to 'addis 0,r2,N'). Fix the same.
Fixes: c71b7eff426fa7 ("powerpc: Add ABIv2 support to ppc_function_entry")
Reported-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/code-patching.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
index eacc9102c2515c..d5b3c3bb95b400 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h
@@ -73,7 +73,7 @@ void __patch_exception(int exc, unsigned long addr);
#endif
#define OP_RT_RA_MASK 0xffff0000UL
-#define LIS_R2 0x3c020000UL
+#define LIS_R2 0x3c400000UL
#define ADDIS_R2_R12 0x3c4c0000UL
#define ADDI_R2_R2 0x38420000UL
base-commit: 91966823812efbd175f904599e5cf2a854b39809
--
2.25.1
^ permalink raw reply related
* Re: [PATCH v3] powerpc/32: remove bogus ppc_select syscall
From: Christophe Leroy @ 2021-03-05 10:15 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linuxppc-dev, Paul Mackerras, linux-kernel@vger.kernel.org
In-Reply-To: <CAK8P3a2b+u+8smkKWB-V2Non+nnZmNG4dNi6cGpM8weYuY5j6A@mail.gmail.com>
Le 05/03/2021 à 11:06, Arnd Bergmann a écrit :
> On Fri, Mar 5, 2021 at 9:40 AM Christophe Leroy
> <christophe.leroy@csgroup.eu> wrote:
>>
>> From: Arnd Bergmann <arnd@arndb.de>
>>
>> The ppc_select function was introduced in linux-2.3.48 in order to support
>> code confusing the legacy select() calling convention with the standard one.
>> Even 24 years ago, all correctly built code should not have done this and
>> could have easily been phased out. Nothing that was compiled later should
>> actually try to use the old_select interface, and it would have been broken
>> already on all ppc64 kernels with the syscall emulation layer.
>>
>> This patch brings the 32 bit compat ABI and the native 32 bit ABI for
>> powerpc into a consistent state, by removing support for both the
>> old_select system call number and the handler for it.
>
> The description still seems wrong, please drop all the nonsense I wrote
> back then and explain what is actually going on.
>
> This is what I can see from the linux-history tree:
>
> - The original ppc32 port (linux-1.3.45) had a regular '__NR_select/sys_select'
> syscall at #82 and an unusable '__NR__newselect/sys_panic' syscall at #142,
> while i386 had the indirect '__NR_select/sys_oldselect' syscall at #82
> and the regular '__NR__newselect/sys_select' version at #142. This was
> rather confusing.
>
> - linux-2.1.48 changed both #82 and #142 to the ppc_select() version that
> tries to guess whether the x86 __NR_select/sys_oldselect() behavior or
> the regular __NR__newselect/sys_select() behavior is used.
>
> - linux-2.5.5 added ppc64 support, with a compat version of ppc_select()
> on both #82 and #142 that would either use the __NR__newselect/sys_select
> semantics or panic() when passed an invalud 'n'. The native ppc64
> port started out with just __NR__newselect/sys_select() on #142
>
> - linux-2.5.19 changed ppc64 compat mode to no longer panic(), making
> both #82 and #142 behave like __NR__newselect/sys_select().
>
> - glibc support for ppc32 gets merged during the linux-2.5 days, supporting
> only #142 with the new behavior.
>
> - linux-2.5.41 dropped support for #82 on ppc64 in compat mode but not
> native ppc32.
>
> - linux-2.6.14 merged the two architecture ports but kept the behavior
> unchanged for both.
>
> - linux-2.6.32 changed the native ppc32 #142 __NR__newselect to
> behave the same as compat mode and no longer emulate the
> x86 oldselect, but #82 remained unchanged.
>
> So we have changed behavior multiple times in the past, and the
> current state still theoretically allows running non-glibc binaries that
> ran on kernels before 2.1.48 that used either the original powerpc
> select or the i386 compatible oldselect semantics. Chances are that
> those binaries are broken for some other reason now.
>
Whaou, nice archeology, thanks. Do you mind if I copy the history you established ?
In your commit, you said 2.3.48. Here in the history you say 2.1.48. Which one is correct ?
Regardless of whethere binaries are broken or not for other reason, is that worth expecting an
almost 25 yr old binary to run on future kernels ? If one is able to put the necessary effort to
port you hardware to the latest kernel, can't he really port the binary as well ?
Thanks
Christophe
^ permalink raw reply
* Re: [PATCH v2 1/3] powerpc: sstep: Fix load and update emulation
From: Naveen N. Rao @ 2021-03-04 1:06 UTC (permalink / raw)
To: Segher Boessenkool
Cc: ravi.bangoria, ananth, jniethe5, paulus, Sandipan Das,
linuxppc-dev, dja
In-Reply-To: <20210304154535.GS29191@gate.crashing.org>
On 2021/03/04 09:45AM, Segher Boessenkool wrote:
> On Wed, Mar 03, 2021 at 10:01:27PM +0530, Naveen N. Rao wrote:
> > On 2021/03/01 08:37PM, Segher Boessenkool wrote:
> > > > And, r6 always ends up with 0xaea. It changes with the value I put into
> > > > r6 though.
> > >
> > > That is exactly the behaviour specified for p8. 0aaa+0040=0aea.
> > >
> > > > Granted, this is all up in the air, but it does look like there is more
> > > > going on and the value isn't the EA or the value at the address.
> > >
> > > That *is* the EA. The EA is the address the insn does the access at.
> >
> > I'm probably missing something here. 0xaaa is the value I stored at an
> > offset of 64 bytes from the stack pointer (r1 is copied into r6). In the
> > ldu instruction above, the EA is 64(r6), which should translate to
> > r1+64. The data returned by the load would be 0xaaa, which should be
> > discarded per the description you provided above. So, I would expect to
> > see a 0xc0.. address in r6.
>
> Yes, I misread your code it seems.
>
> > In fact, this looks to be the behavior documented for P9:
> >
> > > > Power9 does:
> > > >
> > > > Load with Update Instructions (RA = 0)
> > > > EA is placed into R0.
> > > > Load with Update Instructions (RA = RT)
> > > > The storage operand addressed by EA is accessed. The
> > > > displacement
> > > > field is added to the data returned by the load and placed into
> > > > RT.
>
> Yup. So on what cpu did you test?
I tested this on two processors:
2.0 (pvr 004d 0200)
2.1 (pvr 004b 0201)
I guess the behavior changed some time during P8, but I don't have a P9
to test this on.
In any case, this souldn't matter too much for us as you rightly point
out:
>
> Either way, the kernel should not emulate any particular cpu here, I'd
> say, esp. since recent cpus do different things for this invalid form.
Ack.
Thanks!
- Naveen
^ permalink raw reply
* Re: [PATCH v3] powerpc/32: remove bogus ppc_select syscall
From: Arnd Bergmann @ 2021-03-05 10:06 UTC (permalink / raw)
To: Christophe Leroy
Cc: linuxppc-dev, Paul Mackerras, linux-kernel@vger.kernel.org
In-Reply-To: <f08ef2b6f339ba19987cfef4307a4dd26b2faf97.1614933479.git.christophe.leroy@csgroup.eu>
On Fri, Mar 5, 2021 at 9:40 AM Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> The ppc_select function was introduced in linux-2.3.48 in order to support
> code confusing the legacy select() calling convention with the standard one.
> Even 24 years ago, all correctly built code should not have done this and
> could have easily been phased out. Nothing that was compiled later should
> actually try to use the old_select interface, and it would have been broken
> already on all ppc64 kernels with the syscall emulation layer.
>
> This patch brings the 32 bit compat ABI and the native 32 bit ABI for
> powerpc into a consistent state, by removing support for both the
> old_select system call number and the handler for it.
The description still seems wrong, please drop all the nonsense I wrote
back then and explain what is actually going on.
This is what I can see from the linux-history tree:
- The original ppc32 port (linux-1.3.45) had a regular '__NR_select/sys_select'
syscall at #82 and an unusable '__NR__newselect/sys_panic' syscall at #142,
while i386 had the indirect '__NR_select/sys_oldselect' syscall at #82
and the regular '__NR__newselect/sys_select' version at #142. This was
rather confusing.
- linux-2.1.48 changed both #82 and #142 to the ppc_select() version that
tries to guess whether the x86 __NR_select/sys_oldselect() behavior or
the regular __NR__newselect/sys_select() behavior is used.
- linux-2.5.5 added ppc64 support, with a compat version of ppc_select()
on both #82 and #142 that would either use the __NR__newselect/sys_select
semantics or panic() when passed an invalud 'n'. The native ppc64
port started out with just __NR__newselect/sys_select() on #142
- linux-2.5.19 changed ppc64 compat mode to no longer panic(), making
both #82 and #142 behave like __NR__newselect/sys_select().
- glibc support for ppc32 gets merged during the linux-2.5 days, supporting
only #142 with the new behavior.
- linux-2.5.41 dropped support for #82 on ppc64 in compat mode but not
native ppc32.
- linux-2.6.14 merged the two architecture ports but kept the behavior
unchanged for both.
- linux-2.6.32 changed the native ppc32 #142 __NR__newselect to
behave the same as compat mode and no longer emulate the
x86 oldselect, but #82 remained unchanged.
So we have changed behavior multiple times in the past, and the
current state still theoretically allows running non-glibc binaries that
ran on kernels before 2.1.48 that used either the original powerpc
select or the i386 compatible oldselect semantics. Chances are that
those binaries are broken for some other reason now.
Arnd
^ permalink raw reply
* Re: [Freedreno] [PATCH 16/17] iommu: remove DOMAIN_ATTR_IO_PGTABLE_CFG
From: Will Deacon @ 2021-03-05 10:00 UTC (permalink / raw)
To: Rob Clark
Cc: freedreno, Sai Prakash Ranjan, kvm, Joerg Roedel, linuxppc-dev,
dri-devel, Li Yang,
list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>, ,
netdev, David Woodhouse, linux-arm-msm, virtualization,
Robin Murphy, Christoph Hellwig,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <CAF6AEGtTs-=aO-Ntp0Qn6mYDSv4x0-q3y217QxU7kZ6H1b1fiQ@mail.gmail.com>
On Thu, Mar 04, 2021 at 03:11:08PM -0800, Rob Clark wrote:
> On Thu, Mar 4, 2021 at 7:48 AM Robin Murphy <robin.murphy@arm.com> wrote:
> >
> > On 2021-03-01 08:42, Christoph Hellwig wrote:
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> >
> > Moreso than the previous patch, where the feature is at least relatively
> > generic (note that there's a bunch of in-flight development around
> > DOMAIN_ATTR_NESTING), I'm really not convinced that it's beneficial to
> > bloat the generic iommu_ops structure with private driver-specific
> > interfaces. The attribute interface is a great compromise for these
> > kinds of things, and you can easily add type-checked wrappers around it
> > for external callers (maybe even make the actual attributes internal
> > between the IOMMU core and drivers) if that's your concern.
>
> I suppose if this is *just* for the GPU we could move it into adreno_smmu_priv..
>
> But one thing I'm not sure about is whether
> IO_PGTABLE_QUIRK_ARM_OUTER_WBWA is something that other devices
> *should* be using as well, but just haven't gotten around to yet.
The intention is certainly that this would be a place to collate per-domain
pgtable quirks, so I'd prefer not to tie that to the GPU.
Will
^ permalink raw reply
* [PATCH] ASoC: fsl_asrc_dma: request dma channel from specific controller
From: Shengjiu Wang @ 2021-03-05 9:19 UTC (permalink / raw)
To: timur, nicoleotsuka, Xiubo.Lee, festevam, broonie, perex, tiwai,
alsa-devel
Cc: linuxppc-dev, linux-kernel
From: Robin Gong <yibin.gong@nxp.com>
Request dma channel from specific dma controller instead of generic
dma controller list, otherwise, may get the wrong dma controller
if there are multi dma controllers such as i.MX8MP.
Signed-off-by: Robin Gong <yibin.gong@nxp.com>
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
sound/soc/fsl/fsl_asrc_dma.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/sound/soc/fsl/fsl_asrc_dma.c b/sound/soc/fsl/fsl_asrc_dma.c
index 29f91cdecbc3..c313a26c8f95 100644
--- a/sound/soc/fsl/fsl_asrc_dma.c
+++ b/sound/soc/fsl/fsl_asrc_dma.c
@@ -141,6 +141,7 @@ static int fsl_asrc_dma_hw_params(struct snd_soc_component *component,
struct dma_slave_config config_fe, config_be;
enum asrc_pair_index index = pair->index;
struct device *dev = component->dev;
+ struct device_node *of_dma_node;
int stream = substream->stream;
struct imx_dma_data *tmp_data;
struct snd_soc_dpcm *dpcm;
@@ -231,8 +232,10 @@ static int fsl_asrc_dma_hw_params(struct snd_soc_component *component,
pair->dma_data.priority = tmp_data->priority;
dma_release_channel(tmp_chan);
+ of_dma_node = pair->dma_chan[!dir]->device->dev->of_node;
pair->dma_chan[dir] =
- dma_request_channel(mask, filter, &pair->dma_data);
+ __dma_request_channel(&mask, filter, &pair->dma_data,
+ of_dma_node);
pair->req_dma_chan = true;
} else {
pair->dma_chan[dir] = tmp_chan;
--
2.27.0
^ permalink raw reply related
* [PATCH] powerpc/ptrace: Remove duplicate check from pt_regs_check()
From: Denis Efremov @ 2021-03-05 11:28 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: linuxppc-dev, Paul Mackerras, linux-kernel, Denis Efremov
"offsetof(struct pt_regs, msr) == offsetof(struct user_pt_regs, msr)"
checked in pt_regs_check() twice in a row. Remove the second check.
Signed-off-by: Denis Efremov <efremov@linux.com>
---
arch/powerpc/kernel/ptrace/ptrace.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/arch/powerpc/kernel/ptrace/ptrace.c b/arch/powerpc/kernel/ptrace/ptrace.c
index 4f3d4ff3728c..51801777906c 100644
--- a/arch/powerpc/kernel/ptrace/ptrace.c
+++ b/arch/powerpc/kernel/ptrace/ptrace.c
@@ -354,8 +354,6 @@ void __init pt_regs_check(void)
offsetof(struct user_pt_regs, nip));
BUILD_BUG_ON(offsetof(struct pt_regs, msr) !=
offsetof(struct user_pt_regs, msr));
- BUILD_BUG_ON(offsetof(struct pt_regs, msr) !=
- offsetof(struct user_pt_regs, msr));
BUILD_BUG_ON(offsetof(struct pt_regs, orig_gpr3) !=
offsetof(struct user_pt_regs, orig_gpr3));
BUILD_BUG_ON(offsetof(struct pt_regs, ctr) !=
--
2.26.2
^ permalink raw reply related
* Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
From: Marco Elver @ 2021-03-05 9:14 UTC (permalink / raw)
To: Christophe Leroy
Cc: LKML, kasan-dev, Alexander Potapenko, Paul Mackerras,
linuxppc-dev, Dmitry Vyukov
In-Reply-To: <f6e47f4f-6953-6584-f023-8b9c22d6974e@csgroup.eu>
On Fri, 5 Mar 2021 at 09:23, Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
>
>
> Le 05/03/2021 à 08:50, Marco Elver a écrit :
> > On Fri, Mar 05, 2021 at 04:01PM +1100, Michael Ellerman wrote:
> >> Marco Elver <elver@google.com> writes:
> >>> On Thu, Mar 04, 2021 at 12:48PM +0100, Christophe Leroy wrote:
> >>>> Le 04/03/2021 à 12:31, Marco Elver a écrit :
> >>>>> On Thu, 4 Mar 2021 at 12:23, Christophe Leroy
> >>>>> <christophe.leroy@csgroup.eu> wrote:
> >>>>>> Le 03/03/2021 à 11:56, Marco Elver a écrit :
> >>>>>>>
> >>>>>>> Somewhat tangentially, I also note that e.g. show_regs(regs) (which
> >>>>>>> was printed along the KFENCE report above) didn't include the top
> >>>>>>> frame in the "Call Trace", so this assumption is definitely not
> >>>>>>> isolated to KFENCE.
> >>>>>>>
> >>>>>>
> >>>>>> Now, I have tested PPC64 (with the patch I sent yesterday to modify save_stack_trace_regs()
> >>>>>> applied), and I get many failures. Any idea ?
> >>>>>>
> >>>>>> [ 17.653751][ T58] ==================================================================
> >>>>>> [ 17.654379][ T58] BUG: KFENCE: invalid free in .kfence_guarded_free+0x2e4/0x530
> >>>>>> [ 17.654379][ T58]
> >>>>>> [ 17.654831][ T58] Invalid free of 0xc00000003c9c0000 (in kfence-#77):
> >>>>>> [ 17.655358][ T58] .kfence_guarded_free+0x2e4/0x530
> >>>>>> [ 17.655775][ T58] .__slab_free+0x320/0x5a0
> >>>>>> [ 17.656039][ T58] .test_double_free+0xe0/0x198
> >>>>>> [ 17.656308][ T58] .kunit_try_run_case+0x80/0x110
> >>>>>> [ 17.656523][ T58] .kunit_generic_run_threadfn_adapter+0x38/0x50
> >>>>>> [ 17.657161][ T58] .kthread+0x18c/0x1a0
> >>>>>> [ 17.659148][ T58] .ret_from_kernel_thread+0x58/0x70
> >>>>>> [ 17.659869][ T58]
> >>> [...]
> >>>>>
> >>>>> Looks like something is prepending '.' to function names. We expect
> >>>>> the function name to appear as-is, e.g. "kfence_guarded_free",
> >>>>> "test_double_free", etc.
> >>>>>
> >>>>> Is there something special on ppc64, where the '.' is some convention?
> >>>>>
> >>>>
> >>>> I think so, see https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html#FUNC-DES
> >>>>
> >>>> Also see commit https://github.com/linuxppc/linux/commit/02424d896
> >>>
> >>> Thanks -- could you try the below patch? You'll need to define
> >>> ARCH_FUNC_PREFIX accordingly.
> >>>
> >>> We think, since there are only very few architectures that add a prefix,
> >>> requiring <asm/kfence.h> to define something like ARCH_FUNC_PREFIX is
> >>> the simplest option. Let me know if this works for you.
> >>>
> >>> There an alternative option, which is to dynamically figure out the
> >>> prefix, but if this simpler option is fine with you, we'd prefer it.
> >>
> >> We have rediscovered this problem in basically every tracing / debugging
> >> feature added in the last 20 years :)
> >>
> >> I think the simplest solution is the one tools/perf/util/symbol.c uses,
> >> which is to just skip a leading '.'.
> >>
> >> Does that work?
> >>
> >> diff --git a/mm/kfence/report.c b/mm/kfence/report.c
> >> index ab83d5a59bb1..67b49dc54b38 100644
> >> --- a/mm/kfence/report.c
> >> +++ b/mm/kfence/report.c
> >> @@ -67,6 +67,9 @@ static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries
> >> for (skipnr = 0; skipnr < num_entries; skipnr++) {
> >> int len = scnprintf(buf, sizeof(buf), "%ps", (void *)stack_entries[skipnr]);
> >>
> >> + if (buf[0] == '.')
> >> + buf++;
> >> +
> >
> > Unfortunately this does not work, since buf is an array. We'd need an
> > offset, and it should be determined outside the loop. I had a solution
> > like this, but it turned out quite complex (see below). And since most
> > architectures do not require this, decided that the safest option is to
> > use the macro approach with ARCH_FUNC_PREFIX, for which Christophe
> > already prepared a patch and tested:
> > https://lore.kernel.org/linux-mm/20210304144000.1148590-1-elver@google.com/
> > https://lkml.kernel.org/r/afaec81a551ef15345cb7d7563b3fac3d7041c3a.1614868445.git.christophe.leroy@csgroup.eu
> >
> > Since KFENCE requires <asm/kfence.h> anyway, we'd prefer this approach
> > (vs. dynamically detecting).
> >
> > Thanks,
> > -- Marco
> >
>
> What about
Sure something like that would work. But I explicitly did *not* want
to hard-code the '.' in non-arch code.
The choice is between:
1. ARCH_FUNC_PREFIX (as a matter of fact, the ARCH_FUNC_PREFIX patch
is already in -mm). Perhaps we could optimize it further, by checking
ARCH_FUNC_PREFIX in buf, and advancing buf like you propose, but I'm
not sure it's worth worrying about.
2. The dynamic solution that I proposed that does not use a hard-coded
'.' (or some variation thereof).
Please tell me which solution you prefer, 1 or 2 -- I'd like to stop
bikeshedding here. If there's a compelling argument for hard-coding
the '.' in non-arch code, please clarify, but otherwise I'd like to
keep arch-specific things out of generic code.
Thanks.
> diff --git a/mm/kfence/report.c b/mm/kfence/report.c
> index 519f037720f5..5e196625fb34 100644
> --- a/mm/kfence/report.c
> +++ b/mm/kfence/report.c
> @@ -43,7 +43,7 @@ static void seq_con_printf(struct seq_file *seq, const char *fmt, ...)
> static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries,
> const enum kfence_error_type *type)
> {
> - char buf[64];
> + char _buf[64];
> int skipnr, fallback = 0;
>
> if (type) {
> @@ -65,7 +65,11 @@ static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries
> }
>
> for (skipnr = 0; skipnr < num_entries; skipnr++) {
> - int len = scnprintf(buf, sizeof(buf), "%ps", (void *)stack_entries[skipnr]);
> + char *buf = _buf;
> + int len = scnprintf(_buf, sizeof(_buf), "%ps", (void *)stack_entries[skipnr]);
> +
> + if (_buf[0] == '.')
> + buf++, len--;
>
> if (str_has_prefix(buf, "kfence_") || str_has_prefix(buf, "__kfence_") ||
> !strncmp(buf, "__slab_free", len)) {
> ---
>
> Christophe
^ permalink raw reply
* Re: [PATCH v5 05/22] powerpc/irq: Add helper to set regs->softe
From: Christophe Leroy @ 2021-03-05 8:54 UTC (permalink / raw)
To: Nicholas Piggin, Benjamin Herrenschmidt, Michael Ellerman,
msuchanek, Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1612856863.0x6ebz3hce.astroid@bobo.none>
Le 09/02/2021 à 08:49, Nicholas Piggin a écrit :
> Excerpts from Christophe Leroy's message of February 9, 2021 4:18 pm:
>>
>>
>> Le 09/02/2021 à 02:11, Nicholas Piggin a écrit :
>>> Excerpts from Christophe Leroy's message of February 9, 2021 1:10 am:
>>>> regs->softe doesn't exist on PPC32.
>>>>
>>>> Add irq_soft_mask_regs_set_state() helper to set regs->softe.
>>>> This helper will void on PPC32.
>>>>
>>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>>> ---
>>>
>>> You could do the same with the kuap_ functions to change some ifdefs
>>> to IS_ENABLED.
>>>
>>> That's just my preference but if you prefer this way I guess that's
>>> okay.
>>>
>>
>>
>> That's also my preference on the long term.
>>
>> Here it is ephemeral, I have a follow up series implementing interrupt exit/entry in C and getting
>> rid of all the assembly kuap hence getting rid of those ifdefs.
>
> I thought it might have been because you hate ifdef more tha most :)
>
>> The issue I see when using IS_ENABLED() is that you have to indent to the right, then you interfere
>> with the file history and 'git blame'
>
> Valid point if it's just going to indent back the other way in your next
> series.
>
>> Thanks for reviewing my series and looking forward to your feedback on my series on the interrupt
>> entry/exit that I will likely release later today.
>
> Cool, I'm eager to see them.
>
Hi Nick, have you been able to look at it ?
https://patchwork.ozlabs.org/project/linuxppc-dev/cover/cover.1612864003.git.christophe.leroy@csgroup.eu/
Thanks
Christophe
^ permalink raw reply
* [PATCH v1] ibmvnic: remove excessive irqsave
From: angkery @ 2021-03-05 8:48 UTC (permalink / raw)
To: mpe, benh, paulus, drt, ljp, sukadev, davem, kuba
Cc: netdev, linuxppc-dev, linux-kernel, Junlin Yang
From: Junlin Yang <yangjunlin@yulong.com>
ibmvnic_remove locks multiple spinlocks while disabling interrupts:
spin_lock_irqsave(&adapter->state_lock, flags);
spin_lock_irqsave(&adapter->rwi_lock, flags);
As reported by coccinelle, the second _irqsave() overwrites the value
saved in 'flags' by the first _irqsave(), therefore when the second
_irqrestore() comes,the value in 'flags' is not valid,the value saved
by the first _irqsave() has been lost.
This likely leads to IRQs remaining disabled. So remove the second
_irqsave():
spin_lock_irqsave(&adapter->state_lock, flags);
spin_lock(&adapter->rwi_lock);
Generated by: ./scripts/coccinelle/locks/flags.cocci
./drivers/net/ethernet/ibm/ibmvnic.c:5413:1-18:
ERROR: nested lock+irqsave that reuses flags from line 5404.
Fixes: 4a41c421f367 ("ibmvnic: serialize access to work queue on remove")
Signed-off-by: Junlin Yang <yangjunlin@yulong.com>
---
Changes in v1:
a.According to Christophe Leroy's explanation, update the commit information.
b.Add fixes tags.
drivers/net/ethernet/ibm/ibmvnic.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index 2464c8a..a52668d 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -5408,9 +5408,9 @@ static void ibmvnic_remove(struct vio_dev *dev)
* after setting state, so __ibmvnic_reset() which is called
* from the flush_work() below, can make progress.
*/
- spin_lock_irqsave(&adapter->rwi_lock, flags);
+ spin_lock(&adapter->rwi_lock);
adapter->state = VNIC_REMOVING;
- spin_unlock_irqrestore(&adapter->rwi_lock, flags);
+ spin_unlock(&adapter->rwi_lock);
spin_unlock_irqrestore(&adapter->state_lock, flags);
--
1.9.1
^ permalink raw reply related
* [PATCH v3] powerpc/32: remove bogus ppc_select syscall
From: Christophe Leroy @ 2021-03-05 8:40 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Arnd Bergmann
Cc: linuxppc-dev, linux-kernel
From: Arnd Bergmann <arnd@arndb.de>
The ppc_select function was introduced in linux-2.3.48 in order to support
code confusing the legacy select() calling convention with the standard one.
Even 24 years ago, all correctly built code should not have done this and
could have easily been phased out. Nothing that was compiled later should
actually try to use the old_select interface, and it would have been broken
already on all ppc64 kernels with the syscall emulation layer.
This patch brings the 32 bit compat ABI and the native 32 bit ABI for
powerpc into a consistent state, by removing support for both the
old_select system call number and the handler for it.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
[chleroy: Rebased and updated the number of years elapsed and dropped last part of the commit message]
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
First version was in 2008, at that time it was rejected, see
http://patchwork.ozlabs.org/project/linuxppc-dev/patch/200809240839.14902.arnd@arndb.de/
A reduced version of it was merged as commit dad2f2fb0fc7 ("powerpc: Fix wrong error code from ppc32 select syscall")
If we decide to still keep this, then we'll have to:
- take into account -4096 < fd < 0 case
- use unsafe_get_user inside a uaccess_begin block
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/include/asm/asm-prototypes.h | 3 ---
arch/powerpc/kernel/syscalls.c | 25 -----------------------
arch/powerpc/kernel/syscalls/syscall.tbl | 4 +---
3 files changed, 1 insertion(+), 31 deletions(-)
diff --git a/arch/powerpc/include/asm/asm-prototypes.h b/arch/powerpc/include/asm/asm-prototypes.h
index 939f3c94c8f3..78e0a3bd448a 100644
--- a/arch/powerpc/include/asm/asm-prototypes.h
+++ b/arch/powerpc/include/asm/asm-prototypes.h
@@ -63,9 +63,6 @@ long sys_swapcontext(struct ucontext __user *old_ctx,
#ifdef CONFIG_PPC32
long sys_debug_setcontext(struct ucontext __user *ctx,
int ndbg, struct sig_dbg_op __user *dbg);
-int
-ppc_select(int n, fd_set __user *inp, fd_set __user *outp, fd_set __user *exp,
- struct __kernel_old_timeval __user *tvp);
unsigned long __init early_init(unsigned long dt_ptr);
void __init machine_init(u64 dt_ptr);
#endif
diff --git a/arch/powerpc/kernel/syscalls.c b/arch/powerpc/kernel/syscalls.c
index 078608ec2e92..70b0eb5bedfd 100644
--- a/arch/powerpc/kernel/syscalls.c
+++ b/arch/powerpc/kernel/syscalls.c
@@ -71,31 +71,6 @@ SYSCALL_DEFINE6(mmap, unsigned long, addr, size_t, len,
return do_mmap2(addr, len, prot, flags, fd, offset, PAGE_SHIFT);
}
-#ifdef CONFIG_PPC32
-/*
- * Due to some executables calling the wrong select we sometimes
- * get wrong args. This determines how the args are being passed
- * (a single ptr to them all args passed) then calls
- * sys_select() with the appropriate args. -- Cort
- */
-int
-ppc_select(int n, fd_set __user *inp, fd_set __user *outp, fd_set __user *exp, struct __kernel_old_timeval __user *tvp)
-{
- if ( (unsigned long)n >= 4096 )
- {
- unsigned long __user *buffer = (unsigned long __user *)n;
- if (!access_ok(buffer, 5*sizeof(unsigned long))
- || __get_user(n, buffer)
- || __get_user(inp, ((fd_set __user * __user *)(buffer+1)))
- || __get_user(outp, ((fd_set __user * __user *)(buffer+2)))
- || __get_user(exp, ((fd_set __user * __user *)(buffer+3)))
- || __get_user(tvp, ((struct __kernel_old_timeval __user * __user *)(buffer+4))))
- return -EFAULT;
- }
- return sys_select(n, inp, outp, exp, tvp);
-}
-#endif
-
#ifdef CONFIG_PPC64
long ppc64_personality(unsigned long personality)
{
diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
index 0b2480cf3e47..5bb0e90e502e 100644
--- a/arch/powerpc/kernel/syscalls/syscall.tbl
+++ b/arch/powerpc/kernel/syscalls/syscall.tbl
@@ -110,9 +110,7 @@
79 common settimeofday sys_settimeofday compat_sys_settimeofday
80 common getgroups sys_getgroups
81 common setgroups sys_setgroups
-82 32 select ppc_select sys_ni_syscall
-82 64 select sys_ni_syscall
-82 spu select sys_ni_syscall
+82 common select sys_ni_syscall
83 common symlink sys_symlink
84 32 oldlstat sys_lstat sys_ni_syscall
84 64 oldlstat sys_ni_syscall
--
2.25.0
^ permalink raw reply related
* Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
From: Christophe Leroy @ 2021-03-05 8:23 UTC (permalink / raw)
To: Marco Elver, Michael Ellerman
Cc: LKML, kasan-dev, Alexander Potapenko, Paul Mackerras,
linuxppc-dev, Dmitry Vyukov
In-Reply-To: <YEHiq1ALdPn2crvP@elver.google.com>
Le 05/03/2021 à 08:50, Marco Elver a écrit :
> On Fri, Mar 05, 2021 at 04:01PM +1100, Michael Ellerman wrote:
>> Marco Elver <elver@google.com> writes:
>>> On Thu, Mar 04, 2021 at 12:48PM +0100, Christophe Leroy wrote:
>>>> Le 04/03/2021 à 12:31, Marco Elver a écrit :
>>>>> On Thu, 4 Mar 2021 at 12:23, Christophe Leroy
>>>>> <christophe.leroy@csgroup.eu> wrote:
>>>>>> Le 03/03/2021 à 11:56, Marco Elver a écrit :
>>>>>>>
>>>>>>> Somewhat tangentially, I also note that e.g. show_regs(regs) (which
>>>>>>> was printed along the KFENCE report above) didn't include the top
>>>>>>> frame in the "Call Trace", so this assumption is definitely not
>>>>>>> isolated to KFENCE.
>>>>>>>
>>>>>>
>>>>>> Now, I have tested PPC64 (with the patch I sent yesterday to modify save_stack_trace_regs()
>>>>>> applied), and I get many failures. Any idea ?
>>>>>>
>>>>>> [ 17.653751][ T58] ==================================================================
>>>>>> [ 17.654379][ T58] BUG: KFENCE: invalid free in .kfence_guarded_free+0x2e4/0x530
>>>>>> [ 17.654379][ T58]
>>>>>> [ 17.654831][ T58] Invalid free of 0xc00000003c9c0000 (in kfence-#77):
>>>>>> [ 17.655358][ T58] .kfence_guarded_free+0x2e4/0x530
>>>>>> [ 17.655775][ T58] .__slab_free+0x320/0x5a0
>>>>>> [ 17.656039][ T58] .test_double_free+0xe0/0x198
>>>>>> [ 17.656308][ T58] .kunit_try_run_case+0x80/0x110
>>>>>> [ 17.656523][ T58] .kunit_generic_run_threadfn_adapter+0x38/0x50
>>>>>> [ 17.657161][ T58] .kthread+0x18c/0x1a0
>>>>>> [ 17.659148][ T58] .ret_from_kernel_thread+0x58/0x70
>>>>>> [ 17.659869][ T58]
>>> [...]
>>>>>
>>>>> Looks like something is prepending '.' to function names. We expect
>>>>> the function name to appear as-is, e.g. "kfence_guarded_free",
>>>>> "test_double_free", etc.
>>>>>
>>>>> Is there something special on ppc64, where the '.' is some convention?
>>>>>
>>>>
>>>> I think so, see https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html#FUNC-DES
>>>>
>>>> Also see commit https://github.com/linuxppc/linux/commit/02424d896
>>>
>>> Thanks -- could you try the below patch? You'll need to define
>>> ARCH_FUNC_PREFIX accordingly.
>>>
>>> We think, since there are only very few architectures that add a prefix,
>>> requiring <asm/kfence.h> to define something like ARCH_FUNC_PREFIX is
>>> the simplest option. Let me know if this works for you.
>>>
>>> There an alternative option, which is to dynamically figure out the
>>> prefix, but if this simpler option is fine with you, we'd prefer it.
>>
>> We have rediscovered this problem in basically every tracing / debugging
>> feature added in the last 20 years :)
>>
>> I think the simplest solution is the one tools/perf/util/symbol.c uses,
>> which is to just skip a leading '.'.
>>
>> Does that work?
>>
>> diff --git a/mm/kfence/report.c b/mm/kfence/report.c
>> index ab83d5a59bb1..67b49dc54b38 100644
>> --- a/mm/kfence/report.c
>> +++ b/mm/kfence/report.c
>> @@ -67,6 +67,9 @@ static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries
>> for (skipnr = 0; skipnr < num_entries; skipnr++) {
>> int len = scnprintf(buf, sizeof(buf), "%ps", (void *)stack_entries[skipnr]);
>>
>> + if (buf[0] == '.')
>> + buf++;
>> +
>
> Unfortunately this does not work, since buf is an array. We'd need an
> offset, and it should be determined outside the loop. I had a solution
> like this, but it turned out quite complex (see below). And since most
> architectures do not require this, decided that the safest option is to
> use the macro approach with ARCH_FUNC_PREFIX, for which Christophe
> already prepared a patch and tested:
> https://lore.kernel.org/linux-mm/20210304144000.1148590-1-elver@google.com/
> https://lkml.kernel.org/r/afaec81a551ef15345cb7d7563b3fac3d7041c3a.1614868445.git.christophe.leroy@csgroup.eu
>
> Since KFENCE requires <asm/kfence.h> anyway, we'd prefer this approach
> (vs. dynamically detecting).
>
> Thanks,
> -- Marco
>
What about
diff --git a/mm/kfence/report.c b/mm/kfence/report.c
index 519f037720f5..5e196625fb34 100644
--- a/mm/kfence/report.c
+++ b/mm/kfence/report.c
@@ -43,7 +43,7 @@ static void seq_con_printf(struct seq_file *seq, const char *fmt, ...)
static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries,
const enum kfence_error_type *type)
{
- char buf[64];
+ char _buf[64];
int skipnr, fallback = 0;
if (type) {
@@ -65,7 +65,11 @@ static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries
}
for (skipnr = 0; skipnr < num_entries; skipnr++) {
- int len = scnprintf(buf, sizeof(buf), "%ps", (void *)stack_entries[skipnr]);
+ char *buf = _buf;
+ int len = scnprintf(_buf, sizeof(_buf), "%ps", (void *)stack_entries[skipnr]);
+
+ if (_buf[0] == '.')
+ buf++, len--;
if (str_has_prefix(buf, "kfence_") || str_has_prefix(buf, "__kfence_") ||
!strncmp(buf, "__slab_free", len)) {
---
Christophe
^ permalink raw reply related
* Re: [PATCH] powerpc/pseries: export LPAR security flavor in lparcfg
From: Laurent Dufour @ 2021-03-05 8:04 UTC (permalink / raw)
To: Michael Ellerman, benh, paulus, linuxppc-dev
Cc: nathanl, cheloha, linux-kernel
In-Reply-To: <871rcuruee.fsf@mpe.ellerman.id.au>
Le 05/03/2021 à 07:23, Michael Ellerman a écrit :
> Laurent Dufour <ldufour@linux.ibm.com> writes:
>> This is helpful to read the security flavor from inside the LPAR.
>
> We already have /sys/kernel/debug/powerpc/security_features.
>
> Is that not sufficient?
Not really, it only reports that security mitigation are on or off but not the
level set through the ASMI menu. Furthermore, reporting it through
/proc/powerpc/lparcfg allows an easy processing by the lparstat command (see below).
>
>> Export it like this in /proc/powerpc/lparcfg:
>>
>> $ grep security_flavor /proc/powerpc/lparcfg
>> security_flavor=1
>>
>> Value means:
>> 0 Speculative execution fully enabled
>> 1 Speculative execution controls to mitigate user-to-kernel attacks
>> 2 Speculative execution controls to mitigate user-to-kernel and
>> user-to-user side-channel attacks
>
> Those strings come from the FSP help, but we have no guarantee it won't
> mean something different in future.
I think this is nailed down, those strings came from:
https://www.ibm.com/support/pages/node/715841
Where it is written (regarding AIX):
On an LPAR, one can use lparstat -x to display the current mitigation mode:
0 = Speculative execution fully enabled
1 = Speculative execution controls to mitigate user-to-kernel side-channel attacks
2 = Speculative execution controls to mitigate user-to-kernel and user-to-user
side-channel attacks
We have been requested to provide almost the same, which I proposed in
powerpc-utils:
https://groups.google.com/g/powerpc-utils-devel/c/NaKXvdyl_UI/m/wa2stpIDAQAJ
Thanks,
Laurent.
^ permalink raw reply
* Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
From: Marco Elver @ 2021-03-05 7:50 UTC (permalink / raw)
To: Michael Ellerman
Cc: LKML, Paul Mackerras, kasan-dev, Alexander Potapenko,
linuxppc-dev, Dmitry Vyukov
In-Reply-To: <874khqry78.fsf@mpe.ellerman.id.au>
On Fri, Mar 05, 2021 at 04:01PM +1100, Michael Ellerman wrote:
> Marco Elver <elver@google.com> writes:
> > On Thu, Mar 04, 2021 at 12:48PM +0100, Christophe Leroy wrote:
> >> Le 04/03/2021 à 12:31, Marco Elver a écrit :
> >> > On Thu, 4 Mar 2021 at 12:23, Christophe Leroy
> >> > <christophe.leroy@csgroup.eu> wrote:
> >> > > Le 03/03/2021 à 11:56, Marco Elver a écrit :
> >> > > >
> >> > > > Somewhat tangentially, I also note that e.g. show_regs(regs) (which
> >> > > > was printed along the KFENCE report above) didn't include the top
> >> > > > frame in the "Call Trace", so this assumption is definitely not
> >> > > > isolated to KFENCE.
> >> > > >
> >> > >
> >> > > Now, I have tested PPC64 (with the patch I sent yesterday to modify save_stack_trace_regs()
> >> > > applied), and I get many failures. Any idea ?
> >> > >
> >> > > [ 17.653751][ T58] ==================================================================
> >> > > [ 17.654379][ T58] BUG: KFENCE: invalid free in .kfence_guarded_free+0x2e4/0x530
> >> > > [ 17.654379][ T58]
> >> > > [ 17.654831][ T58] Invalid free of 0xc00000003c9c0000 (in kfence-#77):
> >> > > [ 17.655358][ T58] .kfence_guarded_free+0x2e4/0x530
> >> > > [ 17.655775][ T58] .__slab_free+0x320/0x5a0
> >> > > [ 17.656039][ T58] .test_double_free+0xe0/0x198
> >> > > [ 17.656308][ T58] .kunit_try_run_case+0x80/0x110
> >> > > [ 17.656523][ T58] .kunit_generic_run_threadfn_adapter+0x38/0x50
> >> > > [ 17.657161][ T58] .kthread+0x18c/0x1a0
> >> > > [ 17.659148][ T58] .ret_from_kernel_thread+0x58/0x70
> >> > > [ 17.659869][ T58]
> > [...]
> >> >
> >> > Looks like something is prepending '.' to function names. We expect
> >> > the function name to appear as-is, e.g. "kfence_guarded_free",
> >> > "test_double_free", etc.
> >> >
> >> > Is there something special on ppc64, where the '.' is some convention?
> >> >
> >>
> >> I think so, see https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html#FUNC-DES
> >>
> >> Also see commit https://github.com/linuxppc/linux/commit/02424d896
> >
> > Thanks -- could you try the below patch? You'll need to define
> > ARCH_FUNC_PREFIX accordingly.
> >
> > We think, since there are only very few architectures that add a prefix,
> > requiring <asm/kfence.h> to define something like ARCH_FUNC_PREFIX is
> > the simplest option. Let me know if this works for you.
> >
> > There an alternative option, which is to dynamically figure out the
> > prefix, but if this simpler option is fine with you, we'd prefer it.
>
> We have rediscovered this problem in basically every tracing / debugging
> feature added in the last 20 years :)
>
> I think the simplest solution is the one tools/perf/util/symbol.c uses,
> which is to just skip a leading '.'.
>
> Does that work?
>
> diff --git a/mm/kfence/report.c b/mm/kfence/report.c
> index ab83d5a59bb1..67b49dc54b38 100644
> --- a/mm/kfence/report.c
> +++ b/mm/kfence/report.c
> @@ -67,6 +67,9 @@ static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries
> for (skipnr = 0; skipnr < num_entries; skipnr++) {
> int len = scnprintf(buf, sizeof(buf), "%ps", (void *)stack_entries[skipnr]);
>
> + if (buf[0] == '.')
> + buf++;
> +
Unfortunately this does not work, since buf is an array. We'd need an
offset, and it should be determined outside the loop. I had a solution
like this, but it turned out quite complex (see below). And since most
architectures do not require this, decided that the safest option is to
use the macro approach with ARCH_FUNC_PREFIX, for which Christophe
already prepared a patch and tested:
https://lore.kernel.org/linux-mm/20210304144000.1148590-1-elver@google.com/
https://lkml.kernel.org/r/afaec81a551ef15345cb7d7563b3fac3d7041c3a.1614868445.git.christophe.leroy@csgroup.eu
Since KFENCE requires <asm/kfence.h> anyway, we'd prefer this approach
(vs. dynamically detecting).
Thanks,
-- Marco
------ >8 ------
diff --git a/mm/kfence/report.c b/mm/kfence/report.c
index 519f037720f5..b0590199b039 100644
--- a/mm/kfence/report.c
+++ b/mm/kfence/report.c
@@ -43,8 +43,8 @@ static void seq_con_printf(struct seq_file *seq, const char *fmt, ...)
static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries,
const enum kfence_error_type *type)
{
+ int skipnr, fallback = 0, fprefix_chars = 0;
char buf[64];
- int skipnr, fallback = 0;
if (type) {
/* Depending on error type, find different stack entries. */
@@ -64,11 +64,24 @@ static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries
}
}
+ if (scnprintf(buf, sizeof(buf), "%ps", (void *)kfree)) {
+ /*
+ * Some architectures (e.g. ppc64) add a constant prefix to
+ * function names. Determine if such a prefix exists.
+ */
+ const char *str = strstr(buf, "kfree");
+
+ if (str)
+ fprefix_chars = str - buf;
+ }
+
for (skipnr = 0; skipnr < num_entries; skipnr++) {
- int len = scnprintf(buf, sizeof(buf), "%ps", (void *)stack_entries[skipnr]);
+ int len = scnprintf(buf, sizeof(buf), "%ps", (void *)stack_entries[skipnr]) -
+ fprefix_chars;
- if (str_has_prefix(buf, "kfence_") || str_has_prefix(buf, "__kfence_") ||
- !strncmp(buf, "__slab_free", len)) {
+ if (str_has_prefix(buf + fprefix_chars, "kfence_") ||
+ str_has_prefix(buf + fprefix_chars, "__kfence_") ||
+ !strncmp(buf + fprefix_chars, "__slab_free", len)) {
/*
* In case of tail calls from any of the below
* to any of the above.
@@ -77,10 +90,10 @@ static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries
}
/* Also the *_bulk() variants by only checking prefixes. */
- if (str_has_prefix(buf, "kfree") ||
- str_has_prefix(buf, "kmem_cache_free") ||
- str_has_prefix(buf, "__kmalloc") ||
- str_has_prefix(buf, "kmem_cache_alloc"))
+ if (str_has_prefix(buf + fprefix_chars, "kfree") ||
+ str_has_prefix(buf + fprefix_chars, "kmem_cache_free") ||
+ str_has_prefix(buf + fprefix_chars, "__kmalloc") ||
+ str_has_prefix(buf + fprefix_chars, "kmem_cache_alloc"))
goto found;
}
if (fallback < num_entries)
^ permalink raw reply related
* Re: [PATCH] ibmvnic: remove excessive irqsave
From: angkery @ 2021-03-05 7:03 UTC (permalink / raw)
To: Christophe Leroy
Cc: Junlin Yang, linux-kernel, kuba, netdev, ljp, drt, paulus,
sukadev, linuxppc-dev, davem
In-Reply-To: <67215668-0850-a0f3-06e1-49db590b8fcc@csgroup.eu>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=GB18030, Size: 2197 bytes --]
On Fri, 5 Mar 2021 06:49:14 +0100
Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> Le 05/03/2021 ¨¤ 02:43, angkery a ¨¦crit0„2:
> > From: Junlin Yang <yangjunlin@yulong.com>
> >
> > ibmvnic_remove locks multiple spinlocks while disabling interrupts:
> > spin_lock_irqsave(&adapter->state_lock, flags);
> > spin_lock_irqsave(&adapter->rwi_lock, flags);
> >
> > there is no need for the second irqsave,since interrupts are
> > disabled at that point, so remove the second irqsave:
>
> The probl¨¨me is not that there is no need. The problem is a lot more
> serious: As reported by coccinella, the second _irqsave() overwrites
> the value saved in 'flags' by the first _irqsave, therefore when the
> second _irqrestore comes, the value in 'flags' is not valid, the
> value saved by the first _irqsave has been lost. This likely leads to
> IRQs remaining disabled, which is _THE_ problem really.
>
Thank you for explaining the real problem.
I will update the commit information with your description.
> > spin_lock_irqsave(&adapter->state_lock, flags);
> > spin_lock(&adapter->rwi_lock);
> >
> > Generated by: ./scripts/coccinelle/locks/flags.cocci
> > ./drivers/net/ethernet/ibm/ibmvnic.c:5413:1-18:
> > ERROR: nested lock+irqsave that reuses flags from line 5404.
> >
> > Signed-off-by: Junlin Yang <yangjunlin@yulong.com>
> > ---
> > drivers/net/ethernet/ibm/ibmvnic.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/ibm/ibmvnic.c
> > b/drivers/net/ethernet/ibm/ibmvnic.c index 2464c8a..a52668d 100644
> > --- a/drivers/net/ethernet/ibm/ibmvnic.c
> > +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> > @@ -5408,9 +5408,9 @@ static void ibmvnic_remove(struct vio_dev
> > *dev)
> > * after setting state, so __ibmvnic_reset() which is
> > called
> > * from the flush_work() below, can make progress.
> > */
> > - spin_lock_irqsave(&adapter->rwi_lock, flags);
> > + spin_lock(&adapter->rwi_lock);
> > adapter->state = VNIC_REMOVING;
> > - spin_unlock_irqrestore(&adapter->rwi_lock, flags);
> > + spin_unlock(&adapter->rwi_lock);
> >
> > spin_unlock_irqrestore(&adapter->state_lock, flags);
> >
> >
^ permalink raw reply
* Re: [PATCH v2 09/37] KVM: PPC: Book3S 64: Move hcall early register setup to KVM
From: Daniel Axtens @ 2021-03-05 6:48 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev, Nicholas Piggin
In-Reply-To: <20210225134652.2127648-10-npiggin@gmail.com>
Nicholas Piggin <npiggin@gmail.com> writes:
> System calls / hcalls have a different calling convention than
> other interrupts, so there is code in the KVMTEST to massage these
> into the same form as other interrupt handlers.
>
> Move this work into the KVM hcall handler. This means teaching KVM
> a little more about the low level interrupt handler setup, PACA save
> areas, etc., although that's not obviously worse than the current
> approach of coming up with an entirely different interrupt register
> / save convention.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/include/asm/exception-64s.h | 13 +++++++
> arch/powerpc/kernel/exceptions-64s.S | 44 ++----------------------
> arch/powerpc/kvm/book3s_64_entry.S | 17 +++++++++
> 3 files changed, 32 insertions(+), 42 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
> index c1a8aac01cf9..bb6f78fcf981 100644
> --- a/arch/powerpc/include/asm/exception-64s.h
> +++ b/arch/powerpc/include/asm/exception-64s.h
> @@ -35,6 +35,19 @@
> /* PACA save area size in u64 units (exgen, exmc, etc) */
> #define EX_SIZE 10
>
> +/* PACA save area offsets */
> +#define EX_R9 0
> +#define EX_R10 8
> +#define EX_R11 16
> +#define EX_R12 24
> +#define EX_R13 32
> +#define EX_DAR 40
> +#define EX_DSISR 48
> +#define EX_CCR 52
> +#define EX_CFAR 56
> +#define EX_PPR 64
> +#define EX_CTR 72
I wonder - now that EX_blah and EX_SIZE both live in the one header
file, if we could calculate EX_SIZE based on the last offset... or have
a BUILD_BUG_ON or something? ... just trying not to lose the sanity
checking afforded to us by the .if macro we lose when moving these out
of exceptions-64s.S...
> +
> /*
> * maximum recursive depth of MCE exceptions
> */
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index 9ae463e8522b..b7092ba87da8 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -21,22 +21,6 @@
> #include <asm/feature-fixups.h>
> #include <asm/kup.h>
>
> -/* PACA save area offsets (exgen, exmc, etc) */
> -#define EX_R9 0
> -#define EX_R10 8
> -#define EX_R11 16
> -#define EX_R12 24
> -#define EX_R13 32
> -#define EX_DAR 40
> -#define EX_DSISR 48
> -#define EX_CCR 52
> -#define EX_CFAR 56
> -#define EX_PPR 64
> -#define EX_CTR 72
> -.if EX_SIZE != 10
> - .error "EX_SIZE is wrong"
> -.endif
> -
> /*
> * Following are fixed section helper macros.
> *
> @@ -1964,45 +1948,21 @@ EXC_VIRT_END(system_call, 0x4c00, 0x100)
>
> #ifdef CONFIG_KVM_BOOK3S_64_HANDLER
> TRAMP_REAL_BEGIN(system_call_kvm)
> - /*
> - * This is a hcall, so register convention is as above, with these
> - * differences:
> - * r13 = PACA
> - * ctr = orig r13
> - * orig r10 saved in PACA
> - */
> - /*
> - * Save the PPR (on systems that support it) before changing to
> - * HMT_MEDIUM. That allows the KVM code to save that value into the
> - * guest state (it is the guest's PPR value).
> - */
> -BEGIN_FTR_SECTION
> - mfspr r10,SPRN_PPR
> - std r10,HSTATE_PPR(r13)
> -END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> - HMT_MEDIUM
> mfctr r10
> - SET_SCRATCH0(r10)
> - mfcr r10
> - std r12,HSTATE_SCRATCH0(r13)
> - sldi r12,r10,32
> - ori r12,r12,0xc00
> + SET_SCRATCH0(r10) /* Save r13 in SCRATCH0 */
> #ifdef CONFIG_RELOCATABLE
> /*
> - * Requires __LOAD_FAR_HANDLER beause kvmppc_interrupt lives
> + * Requires __LOAD_FAR_HANDLER beause kvmppc_hcall lives
That change probably belongs in the previous patch :)
Beyond that, I'm still wrapping my head around the deep magic going on
here! Hopefully I'll get a bit a bit further next time I look.
Kind regards,
Daniel
> * outside the head section.
> */
> __LOAD_FAR_HANDLER(r10, kvmppc_hcall)
> mtctr r10
> - ld r10,PACA_EXGEN+EX_R10(r13)
> bctr
> #else
> - ld r10,PACA_EXGEN+EX_R10(r13)
> b kvmppc_hcall
> #endif
> #endif
>
> -
> /**
> * Interrupt 0xd00 - Trace Interrupt.
> * This is a synchronous interrupt in response to instruction step or
> diff --git a/arch/powerpc/kvm/book3s_64_entry.S b/arch/powerpc/kvm/book3s_64_entry.S
> index 9572f759255c..1c9518ab7d96 100644
> --- a/arch/powerpc/kvm/book3s_64_entry.S
> +++ b/arch/powerpc/kvm/book3s_64_entry.S
> @@ -13,6 +13,23 @@
> .global kvmppc_hcall
> .balign IFETCH_ALIGN_BYTES
> kvmppc_hcall:
> + /*
> + * This is a hcall, so register convention is as
> + * Documentation/powerpc/papr_hcalls.rst, with these additions:
> + * R13 = PACA
> + * guest R13 saved in SPRN_SCRATCH0
> + * R10 = free
> + */
> +BEGIN_FTR_SECTION
> + mfspr r10,SPRN_PPR
> + std r10,HSTATE_PPR(r13)
> +END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> + HMT_MEDIUM
> + mfcr r10
> + std r12,HSTATE_SCRATCH0(r13)
> + sldi r12,r10,32
> + ori r12,r12,0xc00
> + ld r10,PACA_EXGEN+EX_R10(r13)
>
> .global kvmppc_interrupt
> .balign IFETCH_ALIGN_BYTES
> --
> 2.23.0
^ permalink raw reply
* Re: [PATCH v1] powerpc: Include running function as first entry in save_stack_trace() and friends
From: Christophe Leroy @ 2021-03-05 6:38 UTC (permalink / raw)
To: Segher Boessenkool, Nick Desaulniers
Cc: Mark Rutland, Marco Elver, Will Deacon, Catalin Marinas, LKML,
kasan-dev, Mark Brown, Paul Mackerras, linux-toolchains,
linuxppc-dev, Linux ARM
In-Reply-To: <20210304192447.GT29191@gate.crashing.org>
Le 04/03/2021 à 20:24, Segher Boessenkool a écrit :
> On Thu, Mar 04, 2021 at 09:54:44AM -0800, Nick Desaulniers wrote:
>> On Thu, Mar 4, 2021 at 9:42 AM Marco Elver <elver@google.com> wrote:
>> include/linux/compiler.h:246:
>> prevent_tail_call_optimization
>>
>> commit a9a3ed1eff36 ("x86: Fix early boot crash on gcc-10, third try")
https://github.com/linuxppc/linux/commit/a9a3ed1eff36
>
> That is much heavier than needed (an mb()). You can just put an empty
> inline asm after a call before a return, and that call cannot be
> optimised to a sibling call: (the end of a function is an implicit
> return:)
>
> Instead of:
>
> void g(void);
> void f(int x)
> if (x)
> g();
> }
>
> Do:
>
> void g(void);
> void f(int x)
> if (x)
> g();
> asm("");
> }
>
> This costs no extra instructions, and certainly not something as heavy
> as an mb()! It works without the "if" as well, of course, but with it
> it is a more interesting example of a tail call.
In the commit mentionned at the top, it is said:
The next attempt to prevent compilers from tail-call optimizing
the last function call cpu_startup_entry(), ... , was to add an empty asm("").
This current solution was short and sweet, and reportedly, is supported
by both compilers but we didn't get very far this time: future (LTO?)
optimization passes could potentially eliminate this, which leads us
to the third attempt: having an actual memory barrier there which the
compiler cannot ignore or move around etc.
Christophe
^ permalink raw reply
* Re: linux-next: manual merge of the tty tree with the powerpc-fixes tree
From: Greg KH @ 2021-03-05 6:30 UTC (permalink / raw)
To: Stephen Rothwell
Cc: Uwe Kleine-König, Jiri Slaby, PowerPC,
Linux Kernel Mailing List, Linux Next Mailing List, Jiri Slaby
In-Reply-To: <20210305120523.0cb114b9@canb.auug.org.au>
On Fri, Mar 05, 2021 at 12:05:23PM +1100, Stephen Rothwell wrote:
> Hi all,
>
> Today's linux-next merge of the tty tree got a conflict in:
>
> drivers/tty/hvc/hvcs.c
>
> between commit:
>
> 386a966f5ce7 ("vio: make remove callback return void")
>
> from the powerpc-fixes tree and commit:
>
> fb8d350c291c ("tty: hvc, drop unneeded forward declarations")
>
> from the tty tree.
>
> I fixed it up (they both removed the forward decalrartion of
> hvcs_remove(), but the latter removed more) and can carry the fix as
> necessary. This is now fixed as far as linux-next is concerned, but any
> non trivial conflicts should be mentioned to your upstream maintainer
> when your tree is submitted for merging. You may also want to consider
> cooperating with the maintainer of the conflicting tree to minimise any
> particularly complex conflicts.
Thanks, that's the correct fix, we knew this was going to happen :(
greg k-h
^ permalink raw reply
* Re: [PATCH] powerpc/pseries: export LPAR security flavor in lparcfg
From: Michael Ellerman @ 2021-03-05 6:23 UTC (permalink / raw)
To: Laurent Dufour, benh, paulus, linuxppc-dev; +Cc: nathanl, cheloha, linux-kernel
In-Reply-To: <20210304114240.54112-1-ldufour@linux.ibm.com>
Laurent Dufour <ldufour@linux.ibm.com> writes:
> This is helpful to read the security flavor from inside the LPAR.
We already have /sys/kernel/debug/powerpc/security_features.
Is that not sufficient?
> Export it like this in /proc/powerpc/lparcfg:
>
> $ grep security_flavor /proc/powerpc/lparcfg
> security_flavor=1
>
> Value means:
> 0 Speculative execution fully enabled
> 1 Speculative execution controls to mitigate user-to-kernel attacks
> 2 Speculative execution controls to mitigate user-to-kernel and
> user-to-user side-channel attacks
Those strings come from the FSP help, but we have no guarantee it won't
mean something different in future.
cheers
^ permalink raw reply
* Re: [PATCH] ibmvnic: remove excessive irqsave
From: Lijun Pan @ 2021-03-05 6:04 UTC (permalink / raw)
To: Christophe Leroy
Cc: Junlin Yang, angkery, linux-kernel, Jakub Kicinski, netdev,
Lijun Pan, Dany Madden, paulus, Sukadev Bhattiprolu, linuxppc-dev,
David Miller
In-Reply-To: <67215668-0850-a0f3-06e1-49db590b8fcc@csgroup.eu>
> On Mar 4, 2021, at 11:49 PM, Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
>
>
>
> Le 05/03/2021 à 02:43, angkery a écrit :
>> From: Junlin Yang <yangjunlin@yulong.com>
>> ibmvnic_remove locks multiple spinlocks while disabling interrupts:
>> spin_lock_irqsave(&adapter->state_lock, flags);
>> spin_lock_irqsave(&adapter->rwi_lock, flags);
>> there is no need for the second irqsave,since interrupts are disabled
>> at that point, so remove the second irqsave:
>
> The problème is not that there is no need. The problem is a lot more serious:
> As reported by coccinella, the second _irqsave() overwrites the value saved in 'flags' by the first _irqsave, therefore when the second _irqrestore comes, the value in 'flags' is not valid, the value saved by the first _irqsave has been lost. This likely leads to IRQs remaining disabled, which is _THE_ problem really.
That does sounds like a more serious functional problem than coccinella check.
Thanks for your explanation.
>
>> spin_lock_irqsave(&adapter->state_lock, flags);
>> spin_lock(&adapter->rwi_lock);
>> Generated by: ./scripts/coccinelle/locks/flags.cocci
>> ./drivers/net/ethernet/ibm/ibmvnic.c:5413:1-18:
>> ERROR: nested lock+irqsave that reuses flags from line 5404.
>> Signed-off-by: Junlin Yang <yangjunlin@yulong.com>
>> ---
>> drivers/net/ethernet/ibm/ibmvnic.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
>> index 2464c8a..a52668d 100644
>> --- a/drivers/net/ethernet/ibm/ibmvnic.c
>> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
>> @@ -5408,9 +5408,9 @@ static void ibmvnic_remove(struct vio_dev *dev)
>> * after setting state, so __ibmvnic_reset() which is called
>> * from the flush_work() below, can make progress.
>> */
>> - spin_lock_irqsave(&adapter->rwi_lock, flags);
>> + spin_lock(&adapter->rwi_lock);
>> adapter->state = VNIC_REMOVING;
>> - spin_unlock_irqrestore(&adapter->rwi_lock, flags);
>> + spin_unlock(&adapter->rwi_lock);
>> spin_unlock_irqrestore(&adapter->state_lock, flags);
>>
^ permalink raw reply
* Re: [PATCH v2 08/37] KVM: PPC: Book3S 64: add hcall interrupt handler
From: Daniel Axtens @ 2021-03-05 6:03 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev, Nicholas Piggin, Fabiano Rosas
In-Reply-To: <20210225134652.2127648-9-npiggin@gmail.com>
Hi Nick,
> Add a separate hcall entry point. This can be used to deal with the
> different calling convention.
Looks good to me, makes sense, passes checkpatch.
Reviewed-by: Daniel Axtens <dja@axtens.net>
Kind regards,
Daniel
> Reviewed-by: Fabiano Rosas <farosas@linux.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/kernel/exceptions-64s.S | 4 ++--
> arch/powerpc/kvm/book3s_64_entry.S | 6 +++++-
> 2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index d956dd9ed61f..9ae463e8522b 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -1992,13 +1992,13 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> * Requires __LOAD_FAR_HANDLER beause kvmppc_interrupt lives
> * outside the head section.
> */
> - __LOAD_FAR_HANDLER(r10, kvmppc_interrupt)
> + __LOAD_FAR_HANDLER(r10, kvmppc_hcall)
> mtctr r10
> ld r10,PACA_EXGEN+EX_R10(r13)
> bctr
> #else
> ld r10,PACA_EXGEN+EX_R10(r13)
> - b kvmppc_interrupt
> + b kvmppc_hcall
> #endif
> #endif
>
> diff --git a/arch/powerpc/kvm/book3s_64_entry.S b/arch/powerpc/kvm/book3s_64_entry.S
> index c1276f616af4..9572f759255c 100644
> --- a/arch/powerpc/kvm/book3s_64_entry.S
> +++ b/arch/powerpc/kvm/book3s_64_entry.S
> @@ -7,9 +7,13 @@
> #include <asm/reg.h>
>
> /*
> - * This is branched to from interrupt handlers in exception-64s.S which set
> + * These are branched to from interrupt handlers in exception-64s.S which set
> * IKVM_REAL or IKVM_VIRT, if HSTATE_IN_GUEST was found to be non-zero.
> */
> +.global kvmppc_hcall
> +.balign IFETCH_ALIGN_BYTES
> +kvmppc_hcall:
> +
> .global kvmppc_interrupt
> .balign IFETCH_ALIGN_BYTES
> kvmppc_interrupt:
> --
> 2.23.0
^ permalink raw reply
* Re: [PATCH v2 07/37] KVM: PPC: Book3S 64: Move GUEST_MODE_SKIP test into KVM
From: Daniel Axtens @ 2021-03-05 5:54 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev, Nicholas Piggin, Fabiano Rosas
In-Reply-To: <20210225134652.2127648-8-npiggin@gmail.com>
Hi Nick,
> - .if IKVM_SKIP
> -89: mtocrf 0x80,r9
> - ld r10,IAREA+EX_CTR(r13)
> - mtctr r10
> - ld r9,IAREA+EX_R9(r13)
> - ld r10,IAREA+EX_R10(r13)
> - ld r11,IAREA+EX_R11(r13)
> - ld r12,IAREA+EX_R12(r13)
> - .if IHSRR_IF_HVMODE
> - BEGIN_FTR_SECTION
> - b kvmppc_skip_Hinterrupt
> - FTR_SECTION_ELSE
> - b kvmppc_skip_interrupt
> - ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE | CPU_FTR_ARCH_206)
I was initially concerned that you were pulling out the complexities
around IHSRR_IF_HVMODE, but I checked exceptions-64s.S and the only
exception handler that sets IHSRR_IF_HVMODE is hardware_interrupt and
that does not set IKVM_SKIP, so we are indeed fine to not keep this
complex little asm section.
> - .elseif IHSRR
> - b kvmppc_skip_Hinterrupt
> - .else
> - b kvmppc_skip_interrupt
> - .endif
> - .endif
> .endm
> +/*
> + * KVM uses a trick where it is running in MSR[HV]=1 mode in real-mode with the
> + * guest MMU context loaded, and it sets KVM_GUEST_MODE_SKIP and enables
> + * MSR[DR]=1 while leaving MSR[IR]=0, so it continues to fetch HV instructions
> + * but loads and stores will access the guest context. This is used to load
> + * the faulting instruction without walking page tables.
> + *
> + * However the guest context may not be able to translate, or it may cause a
> + * machine check or other issue, which will result in a fault in the host
> + * (even with KVM-HV).
> + *
> + * These faults are caught here and if the fault was (or was likely) due to
> + * that load, then we just return with the PC advanced +4 and skip the load,
> + * which then goes via the slow path.
> + */
This is a really helpful comment! Thanks!
> +.Lmaybe_skip:
> + cmpwi r12,BOOK3S_INTERRUPT_MACHINE_CHECK
> + beq 1f
> + cmpwi r12,BOOK3S_INTERRUPT_DATA_STORAGE
> + beq 1f
> + cmpwi r12,BOOK3S_INTERRUPT_DATA_SEGMENT
> + beq 1f
> +#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> + /* HSRR interrupts have 2 added to trap vector */
> + cmpwi r12,BOOK3S_INTERRUPT_H_DATA_STORAGE | 0x2
This took me by surprise for a while, but I checked how it works in
exceptions-64s.S and indeed the GEN_KVM macro will add 0x2 to the IVEC
if IHSRR is set, and it is set for h_data_storage.
So this checks out to me.
I have checked, to the best of my limited assembler capabilities that
the logic before and after matches. It seems good to me.
On that limited basis,
Reviewed-by: Daniel Axtens <dja@axtens.net>
Kind regards,
Daniel
> + beq 2f
> +#endif
> + b .Lno_skip
> +1: mfspr r9,SPRN_SRR0
> + addi r9,r9,4
> + mtspr SPRN_SRR0,r9
> + ld r12,HSTATE_SCRATCH0(r13)
> + ld r9,HSTATE_SCRATCH2(r13)
> + GET_SCRATCH0(r13)
> + RFI_TO_KERNEL
> +#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> +2: mfspr r9,SPRN_HSRR0
> + addi r9,r9,4
> + mtspr SPRN_HSRR0,r9
> + ld r12,HSTATE_SCRATCH0(r13)
> + ld r9,HSTATE_SCRATCH2(r13)
> + GET_SCRATCH0(r13)
> + HRFI_TO_KERNEL
> +#endif
> --
> 2.23.0
^ permalink raw reply
* Re: [PATCH] powerpc/perf: prevent mixed EBB and non-EBB events
From: Athira Rajeev @ 2021-03-05 5:50 UTC (permalink / raw)
To: Thadeu Lima de Souza Cascardo; +Cc: Peter Zijlstra, linuxppc-dev, linux-kernel
In-Reply-To: <20210224122116.221120-1-cascardo@canonical.com>
> On 24-Feb-2021, at 5:51 PM, Thadeu Lima de Souza Cascardo <cascardo@canonical.com> wrote:
>
> EBB events must be under exclusive groups, so there is no mix of EBB and
> non-EBB events on the same PMU. This requirement worked fine as perf core
> would not allow other pinned events to be scheduled together with exclusive
> events.
>
> This assumption was broken by commit 1908dc911792 ("perf: Tweak
> perf_event_attr::exclusive semantics").
>
> After that, the test cpu_event_pinned_vs_ebb_test started succeeding after
> read_events, but worse, the task would not have given access to PMC1, so
> when it tried to write to it, it was killed with "illegal instruction".
>
> Preventing mixed EBB and non-EBB events from being add to the same PMU will
> just revert to the previous behavior and the test will succeed.
Hi,
Thanks for checking this. I checked your patch which is fixing “check_excludes” to make
sure all events must agree on EBB. But in the PMU group constraints, we already have check for
EBB events. This is in arch/powerpc/perf/isa207-common.c ( isa207_get_constraint function ).
<<>>
mask |= CNST_EBB_VAL(ebb);
value |= CNST_EBB_MASK;
<<>>
But the above setting for mask and value is interchanged. We actually need to fix here.
Below patch should fix this:
diff --git a/arch/powerpc/perf/isa207-common.c b/arch/powerpc/perf/isa207-common.c
index e4f577da33d8..8b5eeb6fb2fb 100644
--- a/arch/powerpc/perf/isa207-common.c
+++ b/arch/powerpc/perf/isa207-common.c
@@ -447,8 +447,8 @@ int isa207_get_constraint(u64 event, unsigned long *maskp, unsigned long *valp,
* EBB events are pinned & exclusive, so this should never actually
* hit, but we leave it as a fallback in case.
*/
- mask |= CNST_EBB_VAL(ebb);
- value |= CNST_EBB_MASK;
+ mask |= CNST_EBB_MASK;
+ value |= CNST_EBB_VAL(ebb);
*maskp = mask;
*valp = value;
Can you please try with this patch.
Thanks
Athira
>
> Fixes: 1908dc911792 (perf: Tweak perf_event_attr::exclusive semantics)
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> ---
> arch/powerpc/perf/core-book3s.c | 20 ++++++++++++++++----
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> index 43599e671d38..d767f7944f85 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -1010,9 +1010,25 @@ static int check_excludes(struct perf_event **ctrs, unsigned int cflags[],
> int n_prev, int n_new)
> {
> int eu = 0, ek = 0, eh = 0;
> + bool ebb = false;
> int i, n, first;
> struct perf_event *event;
>
> + n = n_prev + n_new;
> + if (n <= 1)
> + return 0;
> +
> + first = 1;
> + for (i = 0; i < n; ++i) {
> + event = ctrs[i];
> + if (first) {
> + ebb = is_ebb_event(event);
> + first = 0;
> + } else if (is_ebb_event(event) != ebb) {
> + return -EAGAIN;
> + }
> + }
> +
> /*
> * If the PMU we're on supports per event exclude settings then we
> * don't need to do any of this logic. NB. This assumes no PMU has both
> @@ -1021,10 +1037,6 @@ static int check_excludes(struct perf_event **ctrs, unsigned int cflags[],
> if (ppmu->flags & PPMU_ARCH_207S)
> return 0;
>
> - n = n_prev + n_new;
> - if (n <= 1)
> - return 0;
> -
> first = 1;
> for (i = 0; i < n; ++i) {
> if (cflags[i] & PPMU_LIMITED_PMC_OK) {
> --
> 2.27.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