Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 26 Jun 2022 09:57:52 +0200
From:   Alexander Gordeev <agordeev@linux.ibm.com>
To:     Ard Biesheuvel <ardb@kernel.org>
Cc:     linux-kernel@vger.kernel.org,
        Peter Zijlstra <peterz@infradead.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
        Heiko Carstens <hca@linux.ibm.com>,
        Vasily Gorbik <gor@linux.ibm.com>,
        Christian Borntraeger <borntraeger@linux.ibm.com>,
        Sven Schnelle <svens@linux.ibm.com>,
        linux-mips@vger.kernel.org, linux-s390@vger.kernel.org
Subject: Re: [PATCH v2 1/3] jump_label: s390: avoid pointless initial NOP
 patching
Message-ID: <YrgRgKgBPs+oOVB3@li-4a3a4a4c-28e5-11b2-a85c-a8d192c6f089.ibm.com>
References: <20220615154142.1574619-1-ardb@kernel.org>
 <20220615154142.1574619-2-ardb@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220615154142.1574619-2-ardb@kernel.org>
Precedence: bulk

On Wed, Jun 15, 2022 at 05:41:40PM +0200, Ard Biesheuvel wrote:
> Patching NOPs into other NOPs at boot time serves no purpose, so let's
> use the same NOP encodings at compile time and runtime.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  arch/s390/include/asm/jump_label.h |  5 ++---
>  arch/s390/kernel/jump_label.c      | 23 +++++---------------
>  2 files changed, 7 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/s390/include/asm/jump_label.h b/arch/s390/include/asm/jump_label.h
> index 916cfcb36d8a..895f774bbcc5 100644
> --- a/arch/s390/include/asm/jump_label.h
> +++ b/arch/s390/include/asm/jump_label.h
> @@ -10,7 +10,6 @@
>  #include <linux/stringify.h>
>  
>  #define JUMP_LABEL_NOP_SIZE 6
> -#define JUMP_LABEL_NOP_OFFSET 2
>  
>  #ifdef CONFIG_CC_IS_CLANG
>  #define JUMP_LABEL_STATIC_KEY_CONSTRAINT "i"
> @@ -21,12 +20,12 @@
>  #endif
>  
>  /*
> - * We use a brcl 0,2 instruction for jump labels at compile time so it
> + * We use a brcl 0,<offset> instruction for jump labels so it
>   * can be easily distinguished from a hotpatch generated instruction.
>   */
>  static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
>  {
> -	asm_volatile_goto("0:	brcl	0,"__stringify(JUMP_LABEL_NOP_OFFSET)"\n"
> +	asm_volatile_goto("0:	brcl 0,%l[label]\n"

Please, use tab after brcl, not space.

>  			  ".pushsection __jump_table,\"aw\"\n"
>  			  ".balign	8\n"
>  			  ".long	0b-.,%l[label]-.\n"
> diff --git a/arch/s390/kernel/jump_label.c b/arch/s390/kernel/jump_label.c
> index 6bec000c6c1c..d764f0d229ab 100644
> --- a/arch/s390/kernel/jump_label.c
> +++ b/arch/s390/kernel/jump_label.c
> @@ -44,14 +44,8 @@ static void jump_label_bug(struct jump_entry *entry, struct insn *expected,
>  	panic("Corrupted kernel text");
>  }
>  
> -static struct insn orignop = {
> -	.opcode = 0xc004,
> -	.offset = JUMP_LABEL_NOP_OFFSET >> 1,
> -};
> -
>  static void jump_label_transform(struct jump_entry *entry,
> -				 enum jump_label_type type,
> -				 int init)
> +				 enum jump_label_type type)
>  {
>  	void *code = (void *)jump_entry_code(entry);
>  	struct insn old, new;
> @@ -63,27 +57,22 @@ static void jump_label_transform(struct jump_entry *entry,
>  		jump_label_make_branch(entry, &old);
>  		jump_label_make_nop(entry, &new);
>  	}
> -	if (init) {
> -		if (memcmp(code, &orignop, sizeof(orignop)))
> -			jump_label_bug(entry, &orignop, &new);
> -	} else {
> -		if (memcmp(code, &old, sizeof(old)))
> -			jump_label_bug(entry, &old, &new);
> -	}
> +	if (memcmp(code, &old, sizeof(old)))
> +		jump_label_bug(entry, &old, &new);
>  	s390_kernel_write(code, &new, sizeof(new));
>  }
>  
>  void arch_jump_label_transform(struct jump_entry *entry,
>  			       enum jump_label_type type)
>  {
> -	jump_label_transform(entry, type, 0);
> +	jump_label_transform(entry, type);
>  	text_poke_sync();
>  }
>  
>  bool arch_jump_label_transform_queue(struct jump_entry *entry,
>  				     enum jump_label_type type)
>  {
> -	jump_label_transform(entry, type, 0);
> +	jump_label_transform(entry, type);
>  	return true;
>  }
>  
> @@ -95,6 +84,4 @@ void arch_jump_label_transform_apply(void)
>  void __init_or_module arch_jump_label_transform_static(struct jump_entry *entry,
>  						       enum jump_label_type type)
>  {
> -	jump_label_transform(entry, type, 1);
> -	text_poke_sync();
>  }


With the comment above:

Acked-by: Alexander Gordeev <agordeev@linux.ibm.com>

Thanks!

> -- 
> 2.35.1
>