[PATCH v2] x86/paravirt: Use relative reference for original instruction

[PATCH v2] x86/paravirt: Use relative reference for original instruction

[Hou Wenlong]

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[],
-- 
2.31.1

Re: [PATCH v2] x86/paravirt: Use relative reference for original instruction

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 533 bytes --]

On 24.11.22 12:51, Hou Wenlong 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>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2] x86/paravirt: Use relative reference for original instruction

[Juergen Gross via Virtualization]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 533 bytes --]

On 24.11.22 12:51, Hou Wenlong 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>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen


[-- Attachment #1.1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] x86/paravirt: Use relative reference for original instruction

[H. Peter Anvin]

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?

Re: [PATCH v2] x86/paravirt: Use relative reference for original instruction

[H. Peter Anvin]

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?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] x86/paravirt: Use relative reference for original instruction

[Hou Wenlong]

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()?

Re: [PATCH v2] x86/paravirt: Use relative reference for original instruction

[H. Peter Anvin]

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.

Re: [PATCH v2] x86/paravirt: Use relative reference for original instruction

[H. Peter Anvin]

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.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] x86/paravirt: Use relative reference for original instruction

[Hou Wenlong]

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?