Hi Youling,
On 2023/5/10 17:16, Youling Tang wrote:
> Add jump-label implementation based on the ARM64 version.
"Add support for jump labels based on ..." sounds better IMO.
>
> Signed-off-by: Youling Tang <[email protected]>
> ---
> Changes in v2:
> - Fix build errors.
> - fix comment.
>
> .../core/jump-labels/arch-support.txt | 2 +-
> arch/loongarch/Kconfig | 2 +
> arch/loongarch/configs/loongson3_defconfig | 1 +
> arch/loongarch/include/asm/jump_label.h | 51 +++++++++++++++++++
> arch/loongarch/kernel/Makefile | 2 +
> arch/loongarch/kernel/jump_label.c | 23 +++++++++
> 6 files changed, 80 insertions(+), 1 deletion(-)
> create mode 100644 arch/loongarch/include/asm/jump_label.h
> create mode 100644 arch/loongarch/kernel/jump_label.c
>
> diff --git a/Documentation/features/core/jump-labels/arch-support.txt b/Documentation/features/core/jump-labels/arch-support.txt
> index 2328eada3a49..94d9dece580f 100644
> --- a/Documentation/features/core/jump-labels/arch-support.txt
> +++ b/Documentation/features/core/jump-labels/arch-support.txt
> @@ -13,7 +13,7 @@
> | csky: | ok |
> | hexagon: | TODO |
> | ia64: | TODO |
> - | loongarch: | TODO |
> + | loongarch: | ok |
+1 for updating the docs along with the implementation!
> | m68k: | TODO |
> | microblaze: | TODO |
> | mips: | ok |
> diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
> index d38b066fc931..193a959a5611 100644
> --- a/arch/loongarch/Kconfig
> +++ b/arch/loongarch/Kconfig
> @@ -83,6 +83,8 @@ config LOONGARCH
> select GPIOLIB
> select HAS_IOPORT
> select HAVE_ARCH_AUDITSYSCALL
> + select HAVE_ARCH_JUMP_LABEL
> + select HAVE_ARCH_JUMP_LABEL_RELATIVE
> select HAVE_ARCH_MMAP_RND_BITS if MMU
> select HAVE_ARCH_SECCOMP_FILTER
> select HAVE_ARCH_TRACEHOOK
> diff --git a/arch/loongarch/configs/loongson3_defconfig b/arch/loongarch/configs/loongson3_defconfig
> index 6cd26dd3c134..33a0f5f742f6 100644
> --- a/arch/loongarch/configs/loongson3_defconfig
> +++ b/arch/loongarch/configs/loongson3_defconfig
> @@ -63,6 +63,7 @@ CONFIG_EFI_ZBOOT=y
> CONFIG_EFI_GENERIC_STUB_INITRD_CMDLINE_LOADER=y
> CONFIG_EFI_CAPSULE_LOADER=m
> CONFIG_EFI_TEST=m
> +CONFIG_JUMP_LABEL=y
> CONFIG_MODULES=y
> CONFIG_MODULE_FORCE_LOAD=y
> CONFIG_MODULE_UNLOAD=y
> diff --git a/arch/loongarch/include/asm/jump_label.h b/arch/loongarch/include/asm/jump_label.h
> new file mode 100644
> index 000000000000..2f9fdec256c5
> --- /dev/null
> +++ b/arch/loongarch/include/asm/jump_label.h
> @@ -0,0 +1,51 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2023 Loongson Technology Corporation Limited
> + *
> + * Based on arch/arm64/include/asm/jump_label.h
> + */
> +#ifndef __ASM_JUMP_LABEL_H
> +#define __ASM_JUMP_LABEL_H
> +
> +#ifndef __ASSEMBLY__
> +
> +#include <linux/types.h>
> +
> +#define JUMP_LABEL_NOP_SIZE 4
Saying LOONGARCH_INSN_SIZE here might be better for reducing redundancy,
although that'll necessitate an extra include of <asm/inst.h>. Leaving
the 4 alone won't cause much harm so I'm fine with either.
> +
> +static __always_inline bool arch_static_branch(struct static_key * const key,
> + const bool branch)
> +{
> + asm_volatile_goto(
> + "1: nop \n\t"
> + " .pushsection __jump_table, \"aw\" \n\t"
> + " .align 3 \n\t"
> + " .long 1b - ., %l[l_yes] - . \n\t"
> + " .quad %0 - . \n\t"
> + " .popsection \n\t"
> + : : "i"(&((char *)key)[branch]) : : l_yes);
> +
> + return false;
> +l_yes:
> + return true;
> +}
> +
> +static __always_inline bool arch_static_branch_jump(struct static_key * const key,
> + const bool branch)
> +{
> + asm_volatile_goto(
> + "1: b %l[l_yes] \n\t"
> + " .pushsection __jump_table, \"aw\" \n\t"
> + " .align 3 \n\t"
> + " .long 1b - ., %l[l_yes] - . \n\t"
> + " .quad %0 - . \n\t"
> + " .popsection \n\t"
> + : : "i"(&((char *)key)[branch]) : : l_yes);
> +
> + return false;
> +l_yes:
> + return true;
> +}
> +
> +#endif /* __ASSEMBLY__ */
> +#endif /* __ASM_JUMP_LABEL_H */
> diff --git a/arch/loongarch/kernel/Makefile b/arch/loongarch/kernel/Makefile
> index 9a72d91cd104..64ea76f60e2c 100644
> --- a/arch/loongarch/kernel/Makefile
> +++ b/arch/loongarch/kernel/Makefile
> @@ -54,4 +54,6 @@ obj-$(CONFIG_HAVE_HW_BREAKPOINT) += hw_breakpoint.o
>
> obj-$(CONFIG_KPROBES) += kprobes.o kprobes_trampoline.o
>
> +obj-$(CONFIG_JUMP_LABEL) += jump_label.o
> +
> CPPFLAGS_vmlinux.lds := $(KBUILD_CFLAGS)
> diff --git a/arch/loongarch/kernel/jump_label.c b/arch/loongarch/kernel/jump_label.c
> new file mode 100644
> index 000000000000..b06245955f7a
> --- /dev/null
> +++ b/arch/loongarch/kernel/jump_label.c
> @@ -0,0 +1,23 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2023 Loongson Technology Corporation Limited
> + *
> + * Based on arch/arm64/kernel/jump_label.c
> + */
> +#include <linux/jump_label.h>
> +#include <linux/kernel.h>
> +#include <asm/inst.h>
> +
> +void arch_jump_label_transform(struct jump_entry *entry,
> + enum jump_label_type type)
> +{
> + void *addr = (void *)jump_entry_code(entry);
> + u32 insn;
> +
> + if (type == JUMP_LABEL_JMP)
Please use a switch for dealing with enum-typed values.
> + insn = larch_insn_gen_b(jump_entry_code(entry), jump_entry_target(entry));
> + else
> + insn = larch_insn_gen_nop();
> +
> + larch_insn_patch_text(addr, insn);
> +}
--
WANG "xen0n" Xuerui
Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/
Hi, Xuerui
On 05/10/2023 05:28 PM, WANG Xuerui wrote:
> Hi Youling,
>
> On 2023/5/10 17:16, Youling Tang wrote:
>> Add jump-label implementation based on the ARM64 version.
>
> "Add support for jump labels based on ..." sounds better IMO.
OK.
>
>>
>> Signed-off-by: Youling Tang <[email protected]>
>> ---
>> Changes in v2:
>> - Fix build errors.
>> - fix comment.
>>
>> .../core/jump-labels/arch-support.txt | 2 +-
>> arch/loongarch/Kconfig | 2 +
>> arch/loongarch/configs/loongson3_defconfig | 1 +
>> arch/loongarch/include/asm/jump_label.h | 51 +++++++++++++++++++
>> arch/loongarch/kernel/Makefile | 2 +
>> arch/loongarch/kernel/jump_label.c | 23 +++++++++
>> 6 files changed, 80 insertions(+), 1 deletion(-)
>> create mode 100644 arch/loongarch/include/asm/jump_label.h
>> create mode 100644 arch/loongarch/kernel/jump_label.c
>>
>> diff --git a/Documentation/features/core/jump-labels/arch-support.txt
>> b/Documentation/features/core/jump-labels/arch-support.txt
>> index 2328eada3a49..94d9dece580f 100644
>> --- a/Documentation/features/core/jump-labels/arch-support.txt
>> +++ b/Documentation/features/core/jump-labels/arch-support.txt
>> @@ -13,7 +13,7 @@
>> | csky: | ok |
>> | hexagon: | TODO |
>> | ia64: | TODO |
>> - | loongarch: | TODO |
>> + | loongarch: | ok |
>
> +1 for updating the docs along with the implementation!
>
>> | m68k: | TODO |
>> | microblaze: | TODO |
>> | mips: | ok |
>> diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
>> index d38b066fc931..193a959a5611 100644
>> --- a/arch/loongarch/Kconfig
>> +++ b/arch/loongarch/Kconfig
>> @@ -83,6 +83,8 @@ config LOONGARCH
>> select GPIOLIB
>> select HAS_IOPORT
>> select HAVE_ARCH_AUDITSYSCALL
>> + select HAVE_ARCH_JUMP_LABEL
>> + select HAVE_ARCH_JUMP_LABEL_RELATIVE
>> select HAVE_ARCH_MMAP_RND_BITS if MMU
>> select HAVE_ARCH_SECCOMP_FILTER
>> select HAVE_ARCH_TRACEHOOK
>> diff --git a/arch/loongarch/configs/loongson3_defconfig
>> b/arch/loongarch/configs/loongson3_defconfig
>> index 6cd26dd3c134..33a0f5f742f6 100644
>> --- a/arch/loongarch/configs/loongson3_defconfig
>> +++ b/arch/loongarch/configs/loongson3_defconfig
>> @@ -63,6 +63,7 @@ CONFIG_EFI_ZBOOT=y
>> CONFIG_EFI_GENERIC_STUB_INITRD_CMDLINE_LOADER=y
>> CONFIG_EFI_CAPSULE_LOADER=m
>> CONFIG_EFI_TEST=m
>> +CONFIG_JUMP_LABEL=y
>> CONFIG_MODULES=y
>> CONFIG_MODULE_FORCE_LOAD=y
>> CONFIG_MODULE_UNLOAD=y
>> diff --git a/arch/loongarch/include/asm/jump_label.h
>> b/arch/loongarch/include/asm/jump_label.h
>> new file mode 100644
>> index 000000000000..2f9fdec256c5
>> --- /dev/null
>> +++ b/arch/loongarch/include/asm/jump_label.h
>> @@ -0,0 +1,51 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (C) 2023 Loongson Technology Corporation Limited
>> + *
>> + * Based on arch/arm64/include/asm/jump_label.h
>> + */
>> +#ifndef __ASM_JUMP_LABEL_H
>> +#define __ASM_JUMP_LABEL_H
>> +
>> +#ifndef __ASSEMBLY__
>> +
>> +#include <linux/types.h>
>> +
>> +#define JUMP_LABEL_NOP_SIZE 4
>
> Saying LOONGARCH_INSN_SIZE here might be better for reducing redundancy,
> although that'll necessitate an extra include of <asm/inst.h>. Leaving
> the 4 alone won't cause much harm so I'm fine with either.
Using LOONGARCH_INSN_SIZE in v1, but causing build errors due to
inclusion of <asm/inst.h> [1].
So I removed the <asm/inst.h> include and used hardcoded 4.
[1]:
https://lore.kernel.org/loongarch/[email protected]/T/#m0d8393aaf529aea0a4dcc5985469e698d63d66b3
>
>> +
>> +static __always_inline bool arch_static_branch(struct static_key *
>> const key,
>> + const bool branch)
>> +{
>> + asm_volatile_goto(
>> + "1: nop \n\t"
>> + " .pushsection __jump_table, \"aw\" \n\t"
>> + " .align 3 \n\t"
>> + " .long 1b - ., %l[l_yes] - . \n\t"
>> + " .quad %0 - . \n\t"
>> + " .popsection \n\t"
>> + : : "i"(&((char *)key)[branch]) : : l_yes);
>> +
>> + return false;
>> +l_yes:
>> + return true;
>> +}
>> +
>> +static __always_inline bool arch_static_branch_jump(struct static_key
>> * const key,
>> + const bool branch)
>> +{
>> + asm_volatile_goto(
>> + "1: b %l[l_yes] \n\t"
>> + " .pushsection __jump_table, \"aw\" \n\t"
>> + " .align 3 \n\t"
>> + " .long 1b - ., %l[l_yes] - . \n\t"
>> + " .quad %0 - . \n\t"
>> + " .popsection \n\t"
>> + : : "i"(&((char *)key)[branch]) : : l_yes);
>> +
>> + return false;
>> +l_yes:
>> + return true;
>> +}
>> +
>> +#endif /* __ASSEMBLY__ */
>> +#endif /* __ASM_JUMP_LABEL_H */
>> diff --git a/arch/loongarch/kernel/Makefile
>> b/arch/loongarch/kernel/Makefile
>> index 9a72d91cd104..64ea76f60e2c 100644
>> --- a/arch/loongarch/kernel/Makefile
>> +++ b/arch/loongarch/kernel/Makefile
>> @@ -54,4 +54,6 @@ obj-$(CONFIG_HAVE_HW_BREAKPOINT) += hw_breakpoint.o
>> obj-$(CONFIG_KPROBES) += kprobes.o kprobes_trampoline.o
>> +obj-$(CONFIG_JUMP_LABEL) += jump_label.o
>> +
>> CPPFLAGS_vmlinux.lds := $(KBUILD_CFLAGS)
>> diff --git a/arch/loongarch/kernel/jump_label.c
>> b/arch/loongarch/kernel/jump_label.c
>> new file mode 100644
>> index 000000000000..b06245955f7a
>> --- /dev/null
>> +++ b/arch/loongarch/kernel/jump_label.c
>> @@ -0,0 +1,23 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2023 Loongson Technology Corporation Limited
>> + *
>> + * Based on arch/arm64/kernel/jump_label.c
>> + */
>> +#include <linux/jump_label.h>
>> +#include <linux/kernel.h>
>> +#include <asm/inst.h>
>> +
>> +void arch_jump_label_transform(struct jump_entry *entry,
>> + enum jump_label_type type)
>> +{
>> + void *addr = (void *)jump_entry_code(entry);
>> + u32 insn;
>> +
>> + if (type == JUMP_LABEL_JMP)
>
> Please use a switch for dealing with enum-typed values.
Because the current type only has JUMP_LABEL_NOP and JUMP_LABEL_JMP,
using if may be simpler than switch.
Thanks,
Youling.
>
>> + insn = larch_insn_gen_b(jump_entry_code(entry),
>> jump_entry_target(entry));
>> + else
>> + insn = larch_insn_gen_nop();
>> +
>> + larch_insn_patch_text(addr, insn);
>> +}
>
On Thu, May 11, 2023 at 09:33:37AM +0800, Youling Tang wrote:
> > > +void arch_jump_label_transform(struct jump_entry *entry,
> > > + enum jump_label_type type)
> > > +{
> > > + void *addr = (void *)jump_entry_code(entry);
> > > + u32 insn;
> > > +
> > > + if (type == JUMP_LABEL_JMP)
> >
> > Please use a switch for dealing with enum-typed values.
>
> Because the current type only has JUMP_LABEL_NOP and JUMP_LABEL_JMP,
> using if may be simpler than switch.
IIRC we used an enum with descriptive names instead of a boolean because
true/false just doesn't tell you much.
The whole thing fundamentally is a boolean descision though, either
you write a JMP or a NOP, jump-labels don't have more options.
On 2023/5/11 15:43, Peter Zijlstra wrote:
> On Thu, May 11, 2023 at 09:33:37AM +0800, Youling Tang wrote:
>
>>>> +void arch_jump_label_transform(struct jump_entry *entry,
>>>> + enum jump_label_type type)
>>>> +{
>>>> + void *addr = (void *)jump_entry_code(entry);
>>>> + u32 insn;
>>>> +
>>>> + if (type == JUMP_LABEL_JMP)
>>>
>>> Please use a switch for dealing with enum-typed values.
>>
>> Because the current type only has JUMP_LABEL_NOP and JUMP_LABEL_JMP,
>> using if may be simpler than switch.
>
> IIRC we used an enum with descriptive names instead of a boolean because
> true/false just doesn't tell you much.
>
> The whole thing fundamentally is a boolean descision though, either
> you write a JMP or a NOP, jump-labels don't have more options.
Ah thanks for the background. My previous suggestion is just kinda
generally applicable software engineering best practice, so if the
actual enum is unlikely to get >2 variants then it should be fine to
keep using "if". Youling, feel free to ignore the piece of comment, and
sorry for not doing my archaeology beforehand. :)
--
WANG "xen0n" Xuerui
Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/
From: Youling Tang
> Sent: 11 May 2023 02:34
...
> >> + if (type == JUMP_LABEL_JMP)
> >
> > Please use a switch for dealing with enum-typed values.
>
> Because the current type only has JUMP_LABEL_NOP and JUMP_LABEL_JMP,
> using if may be simpler than switch.
The generated code will be pretty much the same.
Even if the compiler is allowed generate a jump table
(which is almost certainly disabled) it won't if there
are only two cases.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)