H. Peter Anvin
2022-Nov-28 03:34 UTC
Re: [PATCH v2] x86/paravirt: Use relative reference for original instruction
On November 27, 2022 7:03:20 PM PST, Hou Wenlong <houwenlong.hwl at 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 at 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 at 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.