[v2] LoongArch: Add jump-label implementation

  Add jump-label implementation based on the ARM64 version.

Signed-off-by: Youling Tang <tangyouling@loongson.cn>
---
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
  

On Wed, May 10, 2023 at 05:16:46PM +0800, Youling Tang wrote:
> Add jump-label implementation based on the ARM64 version.
> 
> Signed-off-by: Youling Tang <tangyouling@loongson.cn>

> 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
> +
> +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;
> +}

Seems simple enough; one change I did a while ago for the x86 version is
to put the __jump_table entry generation in a macro so it could be
shared between the (3 for x86) variants.

Not saying you have to do that, just saying it's an option.

> +#endif  /* __ASSEMBLY__ */
> +#endif	/* __ASM_JUMP_LABEL_H */

> 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)
> +		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);
> +}

This all implies Loongarch is fine with the nop<->b transition (much
like arm64 is), but I found no actual mention of what transitions are
valid for the architecture in your inst.c file -- perhaps you could put
a small comment there to elucidate the occasional reader that doesn't
have your arch manual memorized?


Anyway, as with most RISC implementations it's short and sweet.

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
  

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 <tangyouling@loongson.cn>
> ---
> 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);
> +}
  

Hi Peter,

My 2 cents:

On 2023/5/10 17:27, Peter Zijlstra wrote:
> On Wed, May 10, 2023 at 05:16:46PM +0800, Youling Tang wrote:
>> Add jump-label implementation based on the ARM64 version.
>>
>> Signed-off-by: Youling Tang <tangyouling@loongson.cn>
> 
>> <snip>
>>
>> +	if (type == JUMP_LABEL_JMP)
>> +		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);
>> +}
> 
> This all implies Loongarch is fine with the nop<->b transition (much
> like arm64 is), but I found no actual mention of what transitions are
> valid for the architecture in your inst.c file -- perhaps you could put
> a small comment there to elucidate the occasional reader that doesn't
> have your arch manual memorized?

Do you mean by "valid transition" something like "what kind of 
self-modification is architecturally sound, taking ICache / 
micro-architecture behavior etc. into consideration"? If so, I'd say 
things would be fine in LoongArch as long as an instruction fetch 
barrier is used. Maybe Youling can confirm and mention this in the next 
revision.
  

On Wed, May 10, 2023 at 05:34:33PM +0800, WANG Xuerui wrote:
> Hi Peter,
> 
> My 2 cents:
> 
> On 2023/5/10 17:27, Peter Zijlstra wrote:
> > On Wed, May 10, 2023 at 05:16:46PM +0800, Youling Tang wrote:
> > > Add jump-label implementation based on the ARM64 version.
> > > 
> > > Signed-off-by: Youling Tang <tangyouling@loongson.cn>
> > 
> > > <snip>
> > > 
> > > +	if (type == JUMP_LABEL_JMP)
> > > +		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);
> > > +}
> > 
> > This all implies Loongarch is fine with the nop<->b transition (much
> > like arm64 is), but I found no actual mention of what transitions are
> > valid for the architecture in your inst.c file -- perhaps you could put
> > a small comment there to elucidate the occasional reader that doesn't
> > have your arch manual memorized?
> 
> Do you mean by "valid transition" something like "what kind of
> self-modification is architecturally sound, taking ICache /
> micro-architecture behavior etc. into consideration"?

Yes that. There are definitely architectures that have limitations on
what instructions can be hot-patched in the face of concurrent execution
without jumping through too many hoops.
  

Hi, Peter

On 05/10/2023 05:27 PM, Peter Zijlstra wrote:
> On Wed, May 10, 2023 at 05:16:46PM +0800, Youling Tang wrote:
>> Add jump-label implementation based on the ARM64 version.
>>
>> Signed-off-by: Youling Tang <tangyouling@loongson.cn>
>
>> 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
>> +
>> +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;
>> +}
>
> Seems simple enough; one change I did a while ago for the x86 version is
> to put the __jump_table entry generation in a macro so it could be
> shared between the (3 for x86) variants.

Looks better, I will define JUMP_TABLE_ENTRY macro so that
arch_static_branch and arch_static_branch_jump can share.

Thanks,
Youling.

>
> Not saying you have to do that, just saying it's an option.
>
>> +#endif  /* __ASSEMBLY__ */
>> +#endif	/* __ASM_JUMP_LABEL_H */
>
>> 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)
>> +		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);
>> +}
>
> This all implies Loongarch is fine with the nop<->b transition (much
> like arm64 is), but I found no actual mention of what transitions are
> valid for the architecture in your inst.c file -- perhaps you could put
> a small comment there to elucidate the occasional reader that doesn't
> have your arch manual memorized?
>
>
> Anyway, as with most RISC implementations it's short and sweet.
>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>
  

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 <tangyouling@loongson.cn>
>> ---
>> 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/202305100412.gazWW71q-lkp@intel.com/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. :)
  

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)