Re: [PATCH 00/14] Add support for suppressing warning backtraces
Guenter Roeck writes: > On 3/14/24 07:37, Guenter Roeck wrote: >> On 3/14/24 06:36, Geert Uytterhoeven wrote: >>> Hi Günter, >>> >>> On Tue, Mar 12, 2024 at 6:03 PM Guenter Roeck wrote: Some unit tests intentionally trigger warning backtraces by passing bad parameters to kernel API functions. Such unit tests typically check the return value from such calls, not the existence of the warning backtrace. Such intentionally generated warning backtraces are neither desirable nor useful for a number of reasons. - They can result in overlooked real problems. - A warning that suddenly starts to show up in unit tests needs to be investigated and has to be marked to be ignored, for example by adjusting filter scripts. Such filters are ad-hoc because there is no real standard format for warnings. On top of that, such filter scripts would require constant maintenance. One option to address problem would be to add messages such as "expected warning backtraces start / end here" to the kernel log. However, that would again require filter scripts, it might result in missing real problematic warning backtraces triggered while the test is running, and the irrelevant backtrace(s) would still clog the kernel log. Solve the problem by providing a means to identify and suppress specific warning backtraces while executing test code. Support suppressing multiple backtraces while at the same time limiting changes to generic code to the absolute minimum. Architecture specific changes are kept at minimum by retaining function names only if both CONFIG_DEBUG_BUGVERBOSE and CONFIG_KUNIT are enabled. The first patch of the series introduces the necessary infrastructure. The second patch introduces support for counting suppressed backtraces. This capability is used in patch three to implement unit tests. Patch four documents the new API. The next two patches add support for suppressing backtraces in drm_rect and dev_addr_lists unit tests. These patches are intended to serve as examples for the use of the functionality introduced with this series. The remaining patches implement the necessary changes for all architectures with GENERIC_BUG support. >>> >>> Thanks for your series! >>> >>> I gave it a try on m68k, just running backtrace-suppression-test, >>> and that seems to work fine. >>> Design note: Function pointers are only added to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled to avoid image size increases if CONFIG_KUNIT=n. There would be some benefits to adding those pointers all the time (reduced complexity, ability to display function names in BUG/WARNING messages). That change, if desired, can be made later. >>> >>> Unfortunately this also increases kernel size in the CONFIG_KUNIT=m >>> case (ca. 80 KiB for atari_defconfig), making it less attractive to have >>> kunit and all tests enabled as modules in my standard kernel. >>> >> >> Good point. Indeed, it does. I wanted to avoid adding a configuration option, >> but maybe I should add it after all. How about something like this ? >> >> +config KUNIT_SUPPRESS_BACKTRACE >> + bool "KUnit - Enable backtrace suppression" >> + default y >> + help >> + Enable backtrace suppression for KUnit. If enabled, backtraces >> + generated intentionally by KUnit tests can be suppressed. Disable >> + to reduce kernel image size if image size is more important than >> + suppression of backtraces generated by KUnit tests. > > Any more comments / feedback on this ? Otherwise I'll introduce the > above configuration option in v2 of the series. > > In this context, any suggestions if it should be enabled or disabled by > default ? I personally think it would be more important to be able to > suppress backtraces, but I understand that others may not be willing to > accept a ~1% image size increase with CONFIG_KUNIT=m unless > KUNIT_SUPPRESS_BACKTRACE is explicitly disabled. Please enable it by default. There are multiple CI systems that will benefit from it, whereas the number of users enabling KUNIT in severely spaced constrainted environments is surely small - perhaps just Geert ;). cheers
Re: [PATCH v1 1/3] powerpc/code-patching: Test patch_instructions() during boot
Benjamin Gray writes:
> On Mon, 2024-03-18 at 08:38 +1100, Benjamin Gray wrote:
>> On Fri, 2024-03-15 at 07:14 +, Christophe Leroy wrote:
>> > Le 15/03/2024 à 03:57, Benjamin Gray a écrit :
>> > > diff --git a/arch/powerpc/lib/test-code-patching.c
>> > > b/arch/powerpc/lib/test-code-patching.c
>> > > index c44823292f73..35a3756272df 100644
>> > > --- a/arch/powerpc/lib/test-code-patching.c
>> > > +++ b/arch/powerpc/lib/test-code-patching.c
>> > > @@ -347,6 +347,97 @@ static void __init
>> > > test_prefixed_patching(void)
>> > > check(!memcmp(iptr, expected, sizeof(expected)));
>> > > }
>> > >
>> > > +static void __init test_multi_instruction_patching(void)
>> > > +{
>> > > +u32 code[256];
>> >
>> > Build failure:
>> >
>> > CC arch/powerpc/lib/test-code-patching.o
>> > arch/powerpc/lib/test-code-patching.c: In function
>> > 'test_multi_instruction_patching':
>> > arch/powerpc/lib/test-code-patching.c:439:1: error: the frame size
>> > of
>> > 1040 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
>> > 439 | }
>> > | ^
>> > cc1: all warnings being treated as errors
>> > make[4]: *** [scripts/Makefile.build:243:
>> > arch/powerpc/lib/test-code-patching.o] Error 1
>> >
>> >
>> > I have to avoid big arrays on the stack.
>>
>> All good, I can do that.
>>
>> I do run my patches through a couple of 32-bit configs, but I didn't
>> see this error. Is this a standard config I should be testing with?
>
> Specifically pmac32_defconfig and ppc44x_defconfig
Both of those have CONFIG_FRAME_WARN=1024, so should have caught this.
But neither have CONFIG_CODE_PATCHING_SELFTEST=y, so I suspect that's
why you didn't see it.
I recommend ppc32_allmodconfig.
cheers
[powerpc:next] BUILD SUCCESS 5c4233cc0920cc90787aafe950b90f6c57a35b88
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next branch HEAD: 5c4233cc0920cc90787aafe950b90f6c57a35b88 powerpc/kdump: Split KEXEC_CORE and CRASH_DUMP dependency elapsed time: 724m configs tested: 223 configs skipped: 4 The following configs have been built successfully. More configs may be tested in the coming days. tested configs: alpha allnoconfig gcc alphaallyesconfig gcc alpha defconfig gcc arc allmodconfig gcc arc allnoconfig gcc arc allyesconfig gcc arc axs101_defconfig gcc arc defconfig gcc arc haps_hs_defconfig gcc arc haps_hs_smp_defconfig gcc arc nsimosci_hs_smp_defconfig gcc arc randconfig-001-20240317 gcc arc randconfig-002-20240317 gcc arm allmodconfig gcc arm allnoconfig clang arm allyesconfig gcc arm assabet_defconfig clang arm bcm2835_defconfig clang armclps711x_defconfig clang arm defconfig clang arm imxrt_defconfig clang arm integrator_defconfig clang arm moxart_defconfig gcc arm mxs_defconfig clang arm randconfig-001-20240317 clang arm randconfig-002-20240317 clang arm randconfig-003-20240317 gcc arm randconfig-004-20240317 clang armrealview_defconfig clang armvexpress_defconfig gcc armvt8500_v6_v7_defconfig gcc arm64allmodconfig clang arm64 allnoconfig gcc arm64allyesconfig clang arm64 defconfig gcc arm64 randconfig-001-20240317 clang arm64 randconfig-002-20240317 gcc arm64 randconfig-003-20240317 gcc arm64 randconfig-004-20240317 clang csky allmodconfig gcc csky allnoconfig gcc csky allyesconfig gcc cskydefconfig gcc csky randconfig-001-20240317 gcc csky randconfig-002-20240317 gcc hexagon allmodconfig clang hexagon allnoconfig clang hexagon allyesconfig clang hexagon defconfig clang hexagon randconfig-001-20240317 clang hexagon randconfig-002-20240317 clang i386 allmodconfig gcc i386 allnoconfig gcc i386 allyesconfig gcc i386 buildonly-randconfig-001-20240317 gcc i386 buildonly-randconfig-002-20240317 clang i386 buildonly-randconfig-003-20240317 gcc i386 buildonly-randconfig-004-20240317 clang i386 buildonly-randconfig-005-20240317 clang i386 buildonly-randconfig-006-20240317 gcc i386defconfig clang i386 randconfig-001-20240317 clang i386 randconfig-002-20240317 clang i386 randconfig-003-20240317 clang i386 randconfig-004-20240317 clang i386 randconfig-005-20240317 gcc i386 randconfig-006-20240317 gcc i386 randconfig-011-20240317 clang i386 randconfig-012-20240317 clang i386 randconfig-013-20240317 clang i386 randconfig-014-20240317 gcc i386 randconfig-015-20240317 gcc i386 randconfig-016-20240317 clang loongarchalldefconfig gcc loongarchallmodconfig gcc loongarch allnoconfig gcc loongarch defconfig gcc loongarch randconfig-001-20240317 gcc loongarch randconfig-002-20240317 gcc m68k allmodconfig gcc m68k allnoconfig gcc m68k allyesconfig gcc m68kdefconfig gcc m68k m5208evb_defconfig gcc m68kmvme147_defconfig gcc microblaze
[powerpc:merge] BUILD SUCCESS 1a843dadfaed8a6b758d27c3e755b9a62aef8b13
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git merge branch HEAD: 1a843dadfaed8a6b758d27c3e755b9a62aef8b13 Automatic merge of 'master' into merge (2024-03-17 13:34) elapsed time: 724m configs tested: 194 configs skipped: 4 The following configs have been built successfully. More configs may be tested in the coming days. tested configs: alpha allnoconfig gcc alphaallyesconfig gcc alpha defconfig gcc arc allmodconfig gcc arc allnoconfig gcc arc allyesconfig gcc arc defconfig gcc arc randconfig-001-20240317 gcc arc randconfig-002-20240317 gcc arm allmodconfig gcc arm allnoconfig clang arm allyesconfig gcc arm axm55xx_defconfig clang arm defconfig clang arm imx_v6_v7_defconfig clang armmvebu_v7_defconfig clang arm omap2plus_defconfig gcc arm randconfig-001-20240317 clang arm randconfig-002-20240317 clang arm randconfig-003-20240317 gcc arm randconfig-004-20240317 clang arm socfpga_defconfig gcc arm sp7021_defconfig gcc arm sunxi_defconfig gcc arm64allmodconfig clang arm64 allnoconfig gcc arm64 defconfig gcc arm64 randconfig-001-20240317 clang arm64 randconfig-002-20240317 gcc arm64 randconfig-003-20240317 gcc arm64 randconfig-004-20240317 clang csky allmodconfig gcc csky allnoconfig gcc csky allyesconfig gcc cskydefconfig gcc csky randconfig-001-20240317 gcc csky randconfig-002-20240317 gcc hexagon allmodconfig clang hexagon allnoconfig clang hexagon allyesconfig clang hexagon defconfig clang hexagon randconfig-001-20240317 clang hexagon randconfig-002-20240317 clang i386 allmodconfig gcc i386 allnoconfig gcc i386 allyesconfig gcc i386 buildonly-randconfig-001-20240317 gcc i386 buildonly-randconfig-002-20240317 clang i386 buildonly-randconfig-003-20240317 gcc i386 buildonly-randconfig-004-20240317 clang i386 buildonly-randconfig-005-20240317 clang i386 buildonly-randconfig-006-20240317 gcc i386defconfig clang i386 randconfig-001-20240317 clang i386 randconfig-002-20240317 clang i386 randconfig-003-20240317 clang i386 randconfig-004-20240317 clang i386 randconfig-005-20240317 gcc i386 randconfig-006-20240317 gcc i386 randconfig-011-20240317 clang i386 randconfig-012-20240317 clang i386 randconfig-013-20240317 clang i386 randconfig-014-20240317 gcc i386 randconfig-015-20240317 gcc i386 randconfig-016-20240317 clang loongarchallmodconfig gcc loongarch allnoconfig gcc loongarch defconfig gcc loongarch randconfig-001-20240317 gcc loongarch randconfig-002-20240317 gcc m68k allmodconfig gcc m68k allnoconfig gcc m68k allyesconfig gcc m68k apollo_defconfig gcc m68kdefconfig gcc m68kq40_defconfig gcc microblaze allmodconfig gcc microblazeallnoconfig gcc microblaze allyesconfig gcc microblaze defconfig gcc mips allnoconfig gcc mips allyesconfig gcc mips bmips_stb_defconfig clang mips ip22_defconfig gcc mips loongson2k_defconfig gcc mips
Re: [PATCH v1 1/3] powerpc/code-patching: Test patch_instructions() during boot
mjW5LaBPOdGiiDE1w95Ri9HRK27S2dRZpyib9L4mkfYWPAF41mTudjKmVpgtBLO//rO+zmF04OMB/4sWJhLfvhq1CXULDqw5dcuIAIYwf2ughOtyAPFK1ViDcMO5X1bVpNAFO5m4VBpZvFDQ0j0JfqfVBdL68uH05W1/8dMj76RaWj5m0rLM5slY1FQUPddSU+ic9vaZhlDepjU3ZyI8fmioofNGHaxJq6uNTytKdj87kwDV6PQ4hmuGtY56C7JCgjp053sRJ6sXqgKBWfe4ZOJH17mQm+fws93byLoZvvz4Z3im0Rb0MlFo/WirNyhu+TmTNLpnzFUZfenoKrqAkZLY8u1iCFquhgqA321P+sfYew66DtwQmaoi2GKmF89y2enXXzjLNKfLDKkuVoKxFSPeizYqrLi22R9iO8EGBYKACAWIQQ9K5v9I+L06Hi4yOJ5xrdpFsvehAUCYzuwkQIbAgCBCRB5xrdpFsvehHYgBBkRCAAdFiEESFUlaLYscsf4Dt5gaavCcpI6D/8FAmM7sJEACgkQaavCcpI6D/95UgEAqfSj0QhCrYfazQiLDKJstrz3oIKFjhB6+FYMZqt+K1MA/2ioFtHbypeeWbsqYYRhRyTjAKcvE1NZGtH/YWLgkViUidoBAN6gFX/P+VWB77/w8S/BnPmnJx45wmphlkCL8ckOyopFAQCj9eWamHCl2DSaASMSuoZed6C6Gm0OFtuZh/r8K485BQ==
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Date: Mon, 18 Mar 2024 08:55:02 +1100
MIME-Version: 1.0
User-Agent: Evolution 3.50.4 (3.50.4-1.fc39)
X-Trend-IP-HD:
ip=[9.192.253.14]helo={ozlabs.au.ibm.com}sender=(bg...@linux.ibm.com)recipient=
On Mon, 2024-03-18 at 08:38 +1100, Benjamin Gray wrote:
> On Fri, 2024-03-15 at 07:14 +, Christophe Leroy wrote:
> >=20
> >=20
> > Le 15/03/2024 =C3=A0 03:57, Benjamin Gray a =C3=A9crit=C2=A0:
> > > patch_instructions() introduces new behaviour with a couple of
> > > variations. Test each case of
> > >=20
> > > =C2=A0=C2=A0 * a repeated 32-bit instruction,
> > > =C2=A0=C2=A0 * a repeated 64-bit instruction (ppc64), and
> > > =C2=A0=C2=A0 * a copied sequence of instructions
> > >=20
> > > for both on a single page and when it crosses a page boundary.
> > >=20
> > > Signed-off-by: Benjamin Gray
> > > ---
> > > =C2=A0 arch/powerpc/lib/test-code-patching.c | 92
> > > +++
> > > =C2=A0 1 file changed, 92 insertions(+)
> > >=20
> > > diff --git a/arch/powerpc/lib/test-code-patching.c
> > > b/arch/powerpc/lib/test-code-patching.c
> > > index c44823292f73..35a3756272df 100644
> > > --- a/arch/powerpc/lib/test-code-patching.c
> > > +++ b/arch/powerpc/lib/test-code-patching.c
> > > @@ -347,6 +347,97 @@ static void __init
> > > test_prefixed_patching(void)
> > > =C2=A0=C2=A0 check(!memcmp(iptr, expected, sizeof(expected)));
> > > =C2=A0 }
> > > =C2=A0=20
> > > +static void __init test_multi_instruction_patching(void)
> > > +{
> > > + u32 code[256];
> >=20
> > Build failure:
> >=20
> > =C2=A0=C2=A0 CC=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 arch/powerpc/lib/test-cod=
e-patching.o
> > arch/powerpc/lib/test-code-patching.c: In function=20
> > 'test_multi_instruction_patching':
> > arch/powerpc/lib/test-code-patching.c:439:1: error: the frame size
> > of
> > 1040 bytes is larger than 1024 bytes [-Werror=3Dframe-larger-than=3D]
> > =C2=A0=C2=A0 439 | }
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | ^
> > cc1: all warnings being treated as errors
> > make[4]: *** [scripts/Makefile.build:243:=20
> > arch/powerpc/lib/test-code-patching.o] Error 1
> >=20
> >=20
> > I have to avoid big arrays on the stack.
>=20
> All good, I can do that.
>=20
> I do run my patches through a couple of 32-bit configs, but I didn't
> see this error. Is this a standard config I should be testing with?
>=20
Specifically I build pmac32_defconfig and ppc44x_defconfig
Re: [PATCH v1 1/3] powerpc/code-patching: Test patch_instructions() during boot
On Mon, 2024-03-18 at 08:38 +1100, Benjamin Gray wrote:
> On Fri, 2024-03-15 at 07:14 +, Christophe Leroy wrote:
> >
> >
> > Le 15/03/2024 à 03:57, Benjamin Gray a écrit :
> > > patch_instructions() introduces new behaviour with a couple of
> > > variations. Test each case of
> > >
> > > * a repeated 32-bit instruction,
> > > * a repeated 64-bit instruction (ppc64), and
> > > * a copied sequence of instructions
> > >
> > > for both on a single page and when it crosses a page boundary.
> > >
> > > Signed-off-by: Benjamin Gray
> > > ---
> > > arch/powerpc/lib/test-code-patching.c | 92
> > > +++
> > > 1 file changed, 92 insertions(+)
> > >
> > > diff --git a/arch/powerpc/lib/test-code-patching.c
> > > b/arch/powerpc/lib/test-code-patching.c
> > > index c44823292f73..35a3756272df 100644
> > > --- a/arch/powerpc/lib/test-code-patching.c
> > > +++ b/arch/powerpc/lib/test-code-patching.c
> > > @@ -347,6 +347,97 @@ static void __init
> > > test_prefixed_patching(void)
> > > check(!memcmp(iptr, expected, sizeof(expected)));
> > > }
> > >
> > > +static void __init test_multi_instruction_patching(void)
> > > +{
> > > + u32 code[256];
> >
> > Build failure:
> >
> > CC arch/powerpc/lib/test-code-patching.o
> > arch/powerpc/lib/test-code-patching.c: In function
> > 'test_multi_instruction_patching':
> > arch/powerpc/lib/test-code-patching.c:439:1: error: the frame size
> > of
> > 1040 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
> > 439 | }
> > | ^
> > cc1: all warnings being treated as errors
> > make[4]: *** [scripts/Makefile.build:243:
> > arch/powerpc/lib/test-code-patching.o] Error 1
> >
> >
> > I have to avoid big arrays on the stack.
>
> All good, I can do that.
>
> I do run my patches through a couple of 32-bit configs, but I didn't
> see this error. Is this a standard config I should be testing with?
Specifically pmac32_defconfig and ppc44x_defconfig
Re: [PATCH v1 3/3] powerpc/code-patching: Optimise patch_memcpy() to 4 byte chunks
On Fri, 2024-03-15 at 06:39 +, Christophe Leroy wrote:
>
>
> Le 15/03/2024 à 03:57, Benjamin Gray a écrit :
> > As we are patching instructions, we can assume the length is a
> > multiple
> > of 4 and the destination address is aligned.
> >
> > Atomicity of patching a prefixed instruction is not a concern, as
> > the
> > original implementation doesn't provide it anyway.
>
> This patch looks unnecessary.
>
> copy_to_kernel_nofault() is what you want to use instead.
Yeah, I would drop this patch when using copy_to_kernel_nofault()
>
> >
> > Signed-off-by: Benjamin Gray
> > ---
> > arch/powerpc/lib/code-patching.c | 8
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/powerpc/lib/code-patching.c
> > b/arch/powerpc/lib/code-patching.c
> > index c6633759b509..ed450a32918c 100644
> > --- a/arch/powerpc/lib/code-patching.c
> > +++ b/arch/powerpc/lib/code-patching.c
> > @@ -394,10 +394,10 @@ static int patch_memset32(u32 *addr, u32 val,
> > size_t count)
> > return -EPERM;
> > }
> >
> > -static int patch_memcpy(void *dst, void *src, size_t len)
> > +static int patch_memcpy32(u32 *dst, u32 *src, size_t count)
> > {
> > - for (void *end = src + len; src < end; dst++, src++)
> > - __put_kernel_nofault(dst, src, u8, failed);
> > + for (u32 *end = src + count; src < end; dst++, src++)
> > + __put_kernel_nofault(dst, src, u32, failed);
> >
> > return 0;
> >
> > @@ -424,7 +424,7 @@ static int __patch_instructions(u32
> > *patch_addr, u32 *code, size_t len, bool rep
> > err = patch_memset32(patch_addr, val, len
> > / 4);
> > }
> > } else {
> > - err = patch_memcpy(patch_addr, code, len);
> > + err = patch_memcpy32(patch_addr, code, len / 4);
> > }
> >
> > smp_wmb(); /* smp write barrier */
Re: [PATCH v1 2/3] powerpc/code-patching: Use dedicated memory routines for patching
On Fri, 2024-03-15 at 06:36 +, Christophe Leroy wrote:
>
>
> Le 15/03/2024 à 03:57, Benjamin Gray a écrit :
> > The patching page set up as a writable alias may be in quadrant 1
> > (userspace) if the temporary mm path is used. This causes sanitiser
> > failures if so. Sanitiser failures also occur on the non-mm path
> > because the plain memset family is instrumented, and KASAN treats
> > the
> > patching window as poisoned.
> >
> > Introduce locally defined patch_* variants of memset that perform
> > an
> > uninstrumented lower level set, as well as detecting write errors
> > like
> > the original single patch variant does.
> >
> > copy_to_user() is not correct here, as the PTE makes it a proper
> > kernel
> > page (the EEA is privileged access only, RW). It just happens to be
> > in
> > quadrant 1 because that's the hardware's mechanism for using the
> > current
> > PID vs PID 0 in translations. Importantly, it's incorrect to allow
> > user
> > page accesses.
> >
> > Now that the patching memsets are used, we also propagate a failure
> > up
> > to the caller as the single patch variant does.
> >
> > Signed-off-by: Benjamin Gray
> >
> > ---
> >
> > The patch_memcpy() can be optimised to 4 bytes at a time assuming
> > the
> > same requirements as regular instruction patching are being
> > followed
> > for the 'copy sequence of instructions' mode (i.e., they actually
> > are
> > instructions following instruction alignment rules).
>
> Why not use copy_to_kernel_nofault() ?
I had not come across copy_to_kernel_nofault(). It looks like the
optimised memcpy() I wanted, so thanks.
>
>
> > ---
> > arch/powerpc/lib/code-patching.c | 42
> > +---
> > 1 file changed, 38 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/powerpc/lib/code-patching.c
> > b/arch/powerpc/lib/code-patching.c
> > index c6ab46156cda..c6633759b509 100644
> > --- a/arch/powerpc/lib/code-patching.c
> > +++ b/arch/powerpc/lib/code-patching.c
> > @@ -372,9 +372,43 @@ int patch_instruction(u32 *addr, ppc_inst_t
> > instr)
> > }
> > NOKPROBE_SYMBOL(patch_instruction);
> >
> > +static int patch_memset64(u64 *addr, u64 val, size_t count)
> > +{
> > + for (u64 *end = addr + count; addr < end; addr++)
> > + __put_kernel_nofault(addr, , u64, failed);
> > +
> > + return 0;
> > +
> > +failed:
> > + return -EPERM;
>
> Is it correct ? Shouldn't it be -EFAULT ?
The single instruction patch returns EPERM, which was set this way to
align with ftrace's expectations. I think it's best to keep the
single/multi patching variants consistent with each other where
possible.
>
> > +}
> > +
> > +static int patch_memset32(u32 *addr, u32 val, size_t count)
> > +{
> > + for (u32 *end = addr + count; addr < end; addr++)
> > + __put_kernel_nofault(addr, , u32, failed);
> > +
> > + return 0;
> > +
> > +failed:
> > + return -EPERM;
> > +}
> > +
> > +static int patch_memcpy(void *dst, void *src, size_t len)
> > +{
> > + for (void *end = src + len; src < end; dst++, src++)
> > + __put_kernel_nofault(dst, src, u8, failed);
> > +
> > + return 0;
> > +
> > +failed:
> > + return -EPERM;
> > +}
> > +
> > static int __patch_instructions(u32 *patch_addr, u32 *code,
> > size_t len, bool repeat_instr)
> > {
> > unsigned long start = (unsigned long)patch_addr;
> > + int err;
> >
> > /* Repeat instruction */
> > if (repeat_instr) {
> > @@ -383,19 +417,19 @@ static int __patch_instructions(u32
> > *patch_addr, u32 *code, size_t len, bool rep
> > if (ppc_inst_prefixed(instr)) {
> > u64 val = ppc_inst_as_ulong(instr);
> >
> > - memset64((u64 *)patch_addr, val, len / 8);
> > + err = patch_memset64((u64 *)patch_addr,
> > val, len / 8);
> > } else {
> > u32 val = ppc_inst_val(instr);
> >
> > - memset32(patch_addr, val, len / 4);
> > + err = patch_memset32(patch_addr, val, len
> > / 4);
> > }
> > } else {
> > - memcpy(patch_addr, code, len);
> > + err = patch_memcpy(patch_addr, code, len);
>
> Use copy_to_kernel_nofault() instead of open coding a new less
> optimised
> version of it.
>
> > }
> >
> > smp_wmb(); /* smp write barrier */
> > flush_icache_range(start, start + len);
> > - return 0;
> > + return err;
> > }
> >
> > /*
Re: [PATCH v1 1/3] powerpc/code-patching: Test patch_instructions() during boot
On Fri, 2024-03-15 at 07:14 +, Christophe Leroy wrote:
>
>
> Le 15/03/2024 à 03:57, Benjamin Gray a écrit :
> > patch_instructions() introduces new behaviour with a couple of
> > variations. Test each case of
> >
> > * a repeated 32-bit instruction,
> > * a repeated 64-bit instruction (ppc64), and
> > * a copied sequence of instructions
> >
> > for both on a single page and when it crosses a page boundary.
> >
> > Signed-off-by: Benjamin Gray
> > ---
> > arch/powerpc/lib/test-code-patching.c | 92
> > +++
> > 1 file changed, 92 insertions(+)
> >
> > diff --git a/arch/powerpc/lib/test-code-patching.c
> > b/arch/powerpc/lib/test-code-patching.c
> > index c44823292f73..35a3756272df 100644
> > --- a/arch/powerpc/lib/test-code-patching.c
> > +++ b/arch/powerpc/lib/test-code-patching.c
> > @@ -347,6 +347,97 @@ static void __init
> > test_prefixed_patching(void)
> > check(!memcmp(iptr, expected, sizeof(expected)));
> > }
> >
> > +static void __init test_multi_instruction_patching(void)
> > +{
> > + u32 code[256];
>
> Build failure:
>
> CC arch/powerpc/lib/test-code-patching.o
> arch/powerpc/lib/test-code-patching.c: In function
> 'test_multi_instruction_patching':
> arch/powerpc/lib/test-code-patching.c:439:1: error: the frame size of
> 1040 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
> 439 | }
> | ^
> cc1: all warnings being treated as errors
> make[4]: *** [scripts/Makefile.build:243:
> arch/powerpc/lib/test-code-patching.o] Error 1
>
>
> I have to avoid big arrays on the stack.
All good, I can do that.
I do run my patches through a couple of 32-bit configs, but I didn't
see this error. Is this a standard config I should be testing with?
>
>
> > + void *buf;
> > + u32 *addr32;
> > + u64 *addr64;
> > + ppc_inst_t inst64 = ppc_inst_prefix(OP_PREFIX << 26 | 3UL
> > << 24, PPC_RAW_TRAP());
> > + u32 inst32 = PPC_RAW_NOP();
> > +
> > + buf = vzalloc(PAGE_SIZE * 8);
> > + check(buf);
> > + if (!buf)
> > + return;
> > +
> > + /* Test single page 32-bit repeated instruction */
> > + addr32 = buf + PAGE_SIZE;
> > + check(!patch_instructions(addr32 + 1, , 12, true));
> > +
> > + check(addr32[0] == 0);
> > + check(addr32[1] == inst32);
> > + check(addr32[2] == inst32);
> > + check(addr32[3] == inst32);
> > + check(addr32[4] == 0);
> > +
> > + /* Test single page 64-bit repeated instruction */
> > + if (IS_ENABLED(CONFIG_PPC64)) {
> > + check(ppc_inst_prefixed(inst64));
> > +
> > + addr64 = buf + PAGE_SIZE * 2;
> > + ppc_inst_write(code, inst64);
> > + check(!patch_instructions((u32 *)(addr64 + 1),
> > code, 24, true));
> > +
> > + check(addr64[0] == 0);
> > + check(ppc_inst_equal(ppc_inst_read((u32
> > *)[1]), inst64));
> > + check(ppc_inst_equal(ppc_inst_read((u32
> > *)[2]), inst64));
> > + check(ppc_inst_equal(ppc_inst_read((u32
> > *)[3]), inst64));
> > + check(addr64[4] == 0);
> > + }
> > +
> > + /* Test single page memcpy */
> > + addr32 = buf + PAGE_SIZE * 3;
> > +
> > + for (int i = 0; i < ARRAY_SIZE(code); i++)
> > + code[i] = i + 1;
> > +
> > + check(!patch_instructions(addr32 + 1, code, sizeof(code),
> > false));
> > +
> > + check(addr32[0] == 0);
> > + check(!memcmp([1], code, sizeof(code)));
> > + check(addr32[ARRAY_SIZE(code) + 1] == 0);
> > +
> > + /* Test multipage 32-bit repeated instruction */
> > + addr32 = buf + PAGE_SIZE * 4 - 8;
> > + check(!patch_instructions(addr32 + 1, , 12, true));
> > +
> > + check(addr32[0] == 0);
> > + check(addr32[1] == inst32);
> > + check(addr32[2] == inst32);
> > + check(addr32[3] == inst32);
> > + check(addr32[4] == 0);
> > +
> > + /* Test multipage 64-bit repeated instruction */
> > + if (IS_ENABLED(CONFIG_PPC64)) {
> > + check(ppc_inst_prefixed(inst64));
> > +
> > + addr64 = buf + PAGE_SIZE * 5 - 8;
> > + ppc_inst_write(code, inst64);
> > + check(!patch_instructions((u32 *)(addr64 + 1),
> > code, 24, true));
> > +
> > + check(addr64[0] == 0);
> > + check(ppc_inst_equal(ppc_inst_read((u32
> > *)[1]), inst64));
> > + check(ppc_inst_equal(ppc_inst_read((u32
> > *)[2]), inst64));
> > + check(ppc_inst_equal(ppc_inst_read((u32
> > *)[3]), inst64));
> > + check(addr64[4] == 0);
> > + }
> > +
> > + /* Test multipage memcpy */
> > + addr32 = buf + PAGE_SIZE * 6 - 12;
> > +
> > + for (int i = 0; i < ARRAY_SIZE(code); i++)
> > + code[i] = i + 1;
> > +
> > + check(!patch_instructions(addr32 + 1, code, sizeof(code),
> > false));
> > +
> > + check(addr32[0] == 0);
> > + check(!memcmp([1], code, sizeof(code)));
> > + check(addr32[ARRAY_SIZE(code) + 1] == 0);
> > +
> > + vfree(buf);
> > +}
> > +
> > static int __init test_code_patching(void)
> > {
> > pr_info("Running code
Re: [PATCH v1 2/2] powerpc/code-patching: Convert to open_patch_window()/close_patch_window()
On Sat, 2024-03-16 at 10:10 +, Christophe Leroy wrote:
>
>
> Le 15/03/2024 à 09:38, Christophe Leroy a écrit :
> >
> >
> > Le 15/03/2024 à 03:59, Benjamin Gray a écrit :
> > > The existing patching alias page setup and teardown sections can
> > > be
> > > simplified to make use of the new open_patch_window()
> > > abstraction.
> > >
> > > This eliminates the _mm variants of the helpers, consumers no
> > > longer
> > > need to check mm_patch_enabled(), and consumers no longer need to
> > > worry
> > > about synchronization and flushing beyond the changes they make
> > > in the
> > > patching window.
> >
> > With this patch, the time needed to activate or de-activate
> > function
> > tracer is approx 10% longer on powerpc 8xx.
>
> With the following changes, the performance is restored:
>
> diff --git a/arch/powerpc/lib/code-patching.c
> b/arch/powerpc/lib/code-patching.c
> index fd6f8576033a..bc92b85913d8 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -282,13 +282,13 @@ struct patch_window {
> * Interrupts must be disabled for the entire duration of the
> patching. The PIDR
> * is potentially changed during this time.
> */
> -static int open_patch_window(void *addr, struct patch_window *ctx)
> +static __always_inline int open_patch_window(void *addr, struct
> patch_window *ctx)
> {
> unsigned long pfn = get_patch_pfn(addr);
>
> lockdep_assert_irqs_disabled();
>
> - ctx->text_poke_addr = (unsigned
> long)__this_cpu_read(cpu_patching_context.addr);
> + ctx->text_poke_addr = (unsigned
> long)__this_cpu_read(cpu_patching_context.addr) & PAGE_MASK;
>
> if (!mm_patch_enabled()) {
> ctx->ptep =
> __this_cpu_read(cpu_patching_context.pte);
> @@ -331,7 +331,7 @@ static int open_patch_window(void *addr, struct
> patch_window *ctx)
> return 0;
> }
>
> -static void close_patch_window(struct patch_window *ctx)
> +static __always_inline void close_patch_window(struct patch_window
> *ctx)
> {
> lockdep_assert_irqs_disabled();
>
>
Thanks for checking that. I did restore the page mask optimisation in a
later patch while still developing, but the 64-bit assembly looked
slightly worse for it. I didn't check the 32-bit; no way to benchmark
it anyway.
