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:   Mon, 28 Nov 2022 12:19:38 +0800
From:   "Hou Wenlong" <houwenlong.hwl@antgroup.com>
To:     "H. Peter Anvin" <hpa@zytor.com>
Cc:     linux-kernel@vger.kernel.org, Juergen Gross <jgross@suse.com>,
        "Srivatsa S. Bhat (VMware)" <srivatsa@csail.mit.edu>,
        Alexey Makhalov <amakhalov@vmware.com>,
        VMware PV-Drivers Reviewers <pv-drivers@vmware.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        Peter Zijlstra <peterz@infradead.org>,
        Josh Poimboeuf <jpoimboe@kernel.org>,
        Kees Cook <keescook@chromium.org>, Song Liu <song@kernel.org>,
        Nadav Amit <namit@vmware.com>,
        virtualization@lists.linux-foundation.org
Subject: Re: [PATCH v2] x86/paravirt: Use relative reference for original
 instruction
Message-ID: <20221128041938.GA42449@k08j02272.eu95sqa>
References: <73c9ffac157087da78af9fca59cf7d8db7f17226.1669290510.git.houwenlong.hwl@antgroup.com>
 <DD7871BE-B969-4E2E-BDF0-C5D730F0B0AE@zytor.com>
 <20221128030320.GA101008@k08j02272.eu95sqa>
 <169A82BE-E5A9-4DB6-9CBE-055699F00213@zytor.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <169A82BE-E5A9-4DB6-9CBE-055699F00213@zytor.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

On Sun, Nov 27, 2022 at 07:34:39PM -0800, H. Peter Anvin wrote:
> On November 27, 2022 7:03:20 PM PST, Hou Wenlong <houwenlong.hwl@antgroup.com> wrote:
> >On Sun, Nov 27, 2022 at 09:24:34AM -0800, H. Peter Anvin wrote:
> >> On November 24, 2022 3:51:53 AM PST, Hou Wenlong <houwenlong.hwl@antgroup.com> wrote:
> >> >Similar to the alternative patching, use relative reference for original
> >> >instruction rather than absolute one, which saves 8 bytes for one entry
> >> >on x86_64.  And it could generate R_X86_64_PC32 relocation instead of
> >> >R_X86_64_64 relocation, which also reduces relocation metadata on
> >> >relocatable builds. And the alignment could be hard coded to be 4 now.
> >> >
> >> >Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
> >> >---
> >> > arch/x86/include/asm/paravirt.h       | 10 +++++-----
> >> > arch/x86/include/asm/paravirt_types.h |  8 ++++----
> >> > arch/x86/kernel/alternative.c         |  8 +++++---
> >> > 3 files changed, 14 insertions(+), 12 deletions(-)
> >> >
> >> >diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
> >> >index 2851bc2339d5..e56065ea73f2 100644
> >> >--- a/arch/x86/include/asm/paravirt.h
> >> >+++ b/arch/x86/include/asm/paravirt.h
> >> >@@ -735,16 +735,16 @@ extern void default_banner(void);
> >> > 
> >> > #else  /* __ASSEMBLY__ */
> >> > 
> >> >-#define _PVSITE(ptype, ops, word, algn)		\
> >> >+#define _PVSITE(ptype, ops)			\
> >> > 771:;						\
> >> > 	ops;					\
> >> > 772:;						\
> >> > 	.pushsection .parainstructions,"a";	\
> >> >-	 .align	algn;				\
> >> >-	 word 771b;				\
> >> >+	 .align	4;				\
> >> >+	 .long 771b-.;				\
> >> > 	 .byte ptype;				\
> >> > 	 .byte 772b-771b;			\
> >> >-	 _ASM_ALIGN;				\
> >> >+	 .align 4;				\
> >> > 	.popsection
> >> > 
> >> > 
> >> >@@ -752,7 +752,7 @@ extern void default_banner(void);
> >> > #ifdef CONFIG_PARAVIRT_XXL
> >> > 
> >> > #define PARA_PATCH(off)		((off) / 8)
> >> >-#define PARA_SITE(ptype, ops)	_PVSITE(ptype, ops, .quad, 8)
> >> >+#define PARA_SITE(ptype, ops)	_PVSITE(ptype, ops)
> >> > #define PARA_INDIRECT(addr)	*addr(%rip)
> >> > 
> >> > #ifdef CONFIG_DEBUG_ENTRY
> >> >diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
> >> >index 8c1da419260f..68952ae07a3f 100644
> >> >--- a/arch/x86/include/asm/paravirt_types.h
> >> >+++ b/arch/x86/include/asm/paravirt_types.h
> >> >@@ -5,7 +5,7 @@
> >> > #ifndef __ASSEMBLY__
> >> > /* These all sit in the .parainstructions section to tell us what to patch. */
> >> > struct paravirt_patch_site {
> >> >-	u8 *instr;		/* original instructions */
> >> >+	s32 instr_offset;	/* original instructions */
> >> > 	u8 type;		/* type of this instruction */
> >> > 	u8 len;			/* length of original instruction */
> >> > };
> >> >@@ -273,11 +273,11 @@ extern struct paravirt_patch_template pv_ops;
> >> > #define _paravirt_alt(insn_string, type)		\
> >> > 	"771:\n\t" insn_string "\n" "772:\n"		\
> >> > 	".pushsection .parainstructions,\"a\"\n"	\
> >> >-	_ASM_ALIGN "\n"					\
> >> >-	_ASM_PTR " 771b\n"				\
> >> >+	"  .align 4\n"					\
> >> >+	"  .long 771b-.\n"				\
> >> > 	"  .byte " type "\n"				\
> >> > 	"  .byte 772b-771b\n"				\
> >> >-	_ASM_ALIGN "\n"					\
> >> >+	"  .align 4\n"					\
> >> > 	".popsection\n"
> >> > 
> >> > /* Generate patchable code, with the default asm parameters. */
> >> >diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> >> >index 111b809f0ac2..6eea563a098d 100644
> >> >--- a/arch/x86/kernel/alternative.c
> >> >+++ b/arch/x86/kernel/alternative.c
> >> >@@ -1232,20 +1232,22 @@ void __init_or_module apply_paravirt(struct paravirt_patch_site *start,
> >> > {
> >> > 	struct paravirt_patch_site *p;
> >> > 	char insn_buff[MAX_PATCH_LEN];
> >> >+	u8 *instr;
> >> > 
> >> > 	for (p = start; p < end; p++) {
> >> > 		unsigned int used;
> >> > 
> >> >+		instr = (u8 *)&p->instr_offset + p->instr_offset;
> >> > 		BUG_ON(p->len > MAX_PATCH_LEN);
> >> > 		/* prep the buffer with the original instructions */
> >> >-		memcpy(insn_buff, p->instr, p->len);
> >> >-		used = paravirt_patch(p->type, insn_buff, (unsigned long)p->instr, p->len);
> >> >+		memcpy(insn_buff, instr, p->len);
> >> >+		used = paravirt_patch(p->type, insn_buff, (unsigned long)instr, p->len);
> >> > 
> >> > 		BUG_ON(used > p->len);
> >> > 
> >> > 		/* Pad the rest with nops */
> >> > 		add_nops(insn_buff + used, p->len - used);
> >> >-		text_poke_early(p->instr, insn_buff, p->len);
> >> >+		text_poke_early(instr, insn_buff, p->len);
> >> > 	}
> >> > }
> >> > extern struct paravirt_patch_site __start_parainstructions[],
> >> 
> >> Any reason that you couldn't use the same patching code?
> >
> >Sorry, what do you mean using the same patching code? Do you
> >mean that share some code between apply_alternatives() and
> >apply_paravirt()?
> 
> Yes. Abstract the facility rather than duplicate.

The structure of patching entry is different between paravirt
patching and alternative patching. The only same logic of those
two patching functions is iteration in the section. The patching
way is really diffferent. I can abstract the facility like this:

#define for_each_patch_entry(p, start, end, patch_func)    \
	for (p = start; p < end; p++) {                    \
		u8 *instr;                                 \
		char insn_buff[MAX_PATCH_LEN];             \
		instr = p->instr_offset + p->instr_offset; \
                BUG_ON(p->len > MAX_PATCH_LEN);            \
		used = patch_func(p, instr, insn_buff);    \
		add_nops(insn_buff + used, p->len - used); \
		text_poke_early(instr, insn_buff, p->len); \
	}

But I think it is ugly :(. Do you have any better ideas?