rough sketch of revised patching infrastructure

PATCH Jeremy Fitzhardinge 4 participants 9 messages Feb 22, 2007

Subsystems (2)

virtio core PARAVIRT_OPS interface

lore

Share & tools

lore

Jeremy Fitzhardinge

author

2007-02-22 02:09 UTC #0 lore raw

Here's the new patching patch. It compiles, but it doesn't, you know, boot, as such.

The basic idea is to make the incremental cost of patching a new hook as small as possible. If a backend wants pure default patch handling, then it can simply call paravirt_patcher with a NULL patch table; this will end up patching calls to be direct calls to the paravirt op, unless the op is NULL or points to native_nop. Otherwise, a backend can set up a patch table to set specific handling for entrypoints, though the default is still to generate a direct call to the op.

The patch type is now derived from offsetof(paravirt_ops, op), so that there's no need to maintain a specific enumeration.

arch/i386/kernel/alternative.c +3 -5

diff -r d83f67d4b2b8 arch/i386/kernel/alternative.c
@@ -350,9 +350,9 @@ void alternatives_smp_switch(int smp)
 #endif
 
 #ifdef CONFIG_PARAVIRT
-void apply_paravirt(struct paravirt_patch *start, struct paravirt_patch *end)
-{
-	struct paravirt_patch *p;
+void apply_paravirt(struct paravirt_patch_site *start, struct paravirt_patch_site *end)
+{
+	struct paravirt_patch_site *p;
 
 	for (p = start; p < end; p++) {
 		unsigned int used;

@@ -379,8 +379,6 @@ void apply_paravirt(struct paravirt_patc
 	/* Sync to be conservative, in case we patched following instructions */
 	sync_core();
 }
-extern struct paravirt_patch __start_parainstructions[],
-	__stop_parainstructions[];
 #endif	/* CONFIG_PARAVIRT */
 
 void __init alternative_instructions(void)

arch/i386/kernel/asm-offsets.c +6 -0

diff -r d83f67d4b2b8 arch/i386/kernel/asm-offsets.c
@@ -110,5 +110,11 @@ void foo(void)
 	OFFSET(PARAVIRT_irq_enable_sysexit, paravirt_ops, irq_enable_sysexit);
 	OFFSET(PARAVIRT_iret, paravirt_ops, iret);
 	OFFSET(PARAVIRT_read_cr0, paravirt_ops, read_cr0);
+
+	DEFINE(PARAVIRT_IRQ_DISABLE, PARAVIRT_IRQ_DISABLE);
+	DEFINE(PARAVIRT_IRQ_ENABLE, PARAVIRT_IRQ_ENABLE);
+	DEFINE(PARAVIRT_STI_SYSEXIT, PARAVIRT_STI_SYSEXIT);
+	DEFINE(PARAVIRT_IRQ_DISABLE, PARAVIRT_IRQ_DISABLE);
+	DEFINE(PARAVIRT_INTERRUPT_RETURN, PARAVIRT_INTERRUPT_RETURN);
 #endif
 }

arch/i386/kernel/paravirt.c +96 ~~-24~~

130 lines not on this page · raw message

arch/i386/kernel/vmi.c +0 -4

diff -r d83f67d4b2b8 arch/i386/kernel/vmi.c
@@ -70,10 +70,6 @@ struct {
 	void (*set_initial_ap_state)(int, int);
 	void (*halt)(void);
 } vmi_ops;
-
-/* XXX move this to alternative.h */
-extern struct paravirt_patch __start_parainstructions[],
-	__stop_parainstructions[];
 
 /*
  * VMI patching routines.

include/asm-i386/alternative.h +3 -3

diff -r d83f67d4b2b8 include/asm-i386/alternative.h
@@ -118,12 +118,12 @@ static inline void alternatives_smp_swit
 #define LOCK_PREFIX ""
 #endif
 
-struct paravirt_patch;
+struct paravirt_patch_site;
 #ifdef CONFIG_PARAVIRT
-void apply_paravirt(struct paravirt_patch *start, struct paravirt_patch *end);
+void apply_paravirt(struct paravirt_patch_site *start, struct paravirt_patch_site *end);
 #else
 static inline void
-apply_paravirt(struct paravirt_patch *start, struct paravirt_patch *end)
+apply_paravirt(struct paravirt_patch_site *start, struct paravirt_patch_site *end)
 {}
 #define __start_parainstructions NULL
 #define __stop_parainstructions NULL

include/asm-i386/paravirt.h +283 ~~-99~~

560 lines not on this page · raw message

_______________________________________________
Virtualization mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/virtualization

Rusty Russell

2007-02-22 03:43 UTC #1 lore raw

Quoted 2 lines · Jeremy Fitzhardinge

Here's the new patching patch. It compiles, but it doesn't, you know, boot, as such.

OK, there are several separate things here.
(1) Get rid of the PARAVIRT_IRQ_DISABLE etc constants in favour of offsetof within the structure.
(2) Change (almost) all the paravirt hooks to be patchable.
(3) Genericise the infrastructure to table-driven.

These should probably become separate patches, in fact.

I'm sold on the first two, not on the last one. Perhaps the difference is as simple as using an explicit fn pointer rather than an enum, and exposing functions like "paravirt_nop_patch", "paravirt_no_patch", paravirt_jmp_patch, paravirt_call_patch, and indeed, paravirt_default_patch (which would call jmp or call) and even paravirt_native_patch.

BTW, here are the top 10 paravirt functions on native after a kernel compile on my setup (patched-out functions not included):

64895231: make_pte
39780056: set_pte_at
29604260: pgd_val
24518952: flush_tlb_kernel
23992028: set_pte
11385663: pte_val
8221901: read_cr2
7106629: read_tsc
6093958: nop
1858134: save_fl (presumably before patching)

Cheers!
Rusty.

Jeremy Fitzhardinge

author

2007-02-22 10:22 UTC #2 lore raw

Quoted 6 lines · Rusty Russell

Quoted 3 lines · Jeremy Fitzhardinge
Here's the new patching patch. It compiles, but it doesn't, you know, boot, as such.

Quoted 8 lines

OK, there are several separate things here.
(1) Get rid of the PARAVIRT_IRQ_DISABLE etc constants in favour of offsetof within the structure.
(2) Change (almost) all the paravirt hooks to be patchable.
(3) Genericise the infrastructure to table-driven.
These should probably become separate patches, in fact.

OK, here's something which is more presentable, and it even boots (at least under Xen; haven't tried native).

paravirt-patch-rename-paravirt_patch.patch paravirt-use-offset-site-ids.patch paravirt-fix-clobbers.patch
- misc cleanups
paravirt-patchable-call-wrappers.patch
- wrap a large number of the paravirt calls with the magic to make the
callsites patchable. More are possible; this is just a good first step. paravirt-patch-machinery.patch
- the actual patching machinery, which is function-pointer driven
rather than table driven now.

Rename struct paravirt_patch to paravirt_patch_site, so that it clearly refers to a callsite, and no the patch which may be applied to the callsite.

Signed-off-by Jeremy Fitzhardinge <[email protected]>

===================================================================

arch/i386/kernel/alternative.c +3 -5

@@ -350,9 +350,9 @@ void alternatives_smp_switch(int smp)
 #endif
 
 #ifdef CONFIG_PARAVIRT
-void apply_paravirt(struct paravirt_patch *start, struct paravirt_patch *end)
-{
-	struct paravirt_patch *p;
+void apply_paravirt(struct paravirt_patch_site *start, struct paravirt_patch_site *end)
+{
+	struct paravirt_patch_site *p;
 
 	for (p = start; p < end; p++) {
 		unsigned int used;

@@ -379,8 +379,6 @@ void apply_paravirt(struct paravirt_patc
 	/* Sync to be conservative, in case we patched following instructions */
 	sync_core();
 }
-extern struct paravirt_patch __start_parainstructions[],
-	__stop_parainstructions[];
 #endif	/* CONFIG_PARAVIRT */
 
 void __init alternative_instructions(void)

===================================================================

arch/i386/kernel/vmi.c +0 -4

@@ -70,10 +70,6 @@ struct {
 	void (*set_initial_ap_state)(int, int);
 	void (*halt)(void);
 } vmi_ops;
-
-/* XXX move this to alternative.h */
-extern struct paravirt_patch __start_parainstructions[],
-	__stop_parainstructions[];
 
 /*
  * VMI patching routines.

===================================================================

include/asm-i386/alternative.h +3 -3

@@ -118,12 +118,12 @@ static inline void alternatives_smp_swit
 #define LOCK_PREFIX ""
 #endif
 
-struct paravirt_patch;
+struct paravirt_patch_site;
 #ifdef CONFIG_PARAVIRT
-void apply_paravirt(struct paravirt_patch *start, struct paravirt_patch *end);
+void apply_paravirt(struct paravirt_patch_site *start, struct paravirt_patch_site *end);
 #else
 static inline void
-apply_paravirt(struct paravirt_patch *start, struct paravirt_patch *end)
+apply_paravirt(struct paravirt_patch_site *start, struct paravirt_patch_site *end)
 {}
 #define __start_parainstructions NULL
 #define __stop_parainstructions NULL

===================================================================

include/asm-i386/paravirt.h +4 -1

@@ -561,12 +561,15 @@ static inline void pmd_clear(pmd_t *pmdp
 #define arch_leave_lazy_mmu_mode() paravirt_ops.set_lazy_mode(PARAVIRT_LAZY_NONE)
 
 /* These all sit in the .parainstructions section to tell us what to patch. */
-struct paravirt_patch {
+struct paravirt_patch_site {
 	u8 *instr; 		/* original instructions */
 	u8 instrtype;		/* type of this instruction */
 	u8 len;			/* length of original instruction */
 	u16 clobbers;		/* what registers you may clobber */
 };
+
+extern struct paravirt_patch_site __start_parainstructions[],
+	__stop_parainstructions[];
 
 #define paravirt_alt(insn_string, typenum, clobber)	\
 	"771:\n\t" insn_string "\n" "772:\n"		\

Use patch type identifiers derived from the offset of the operation in the paravirt_ops structure. This avoids having to maintain a separate enum for patch types.

Signed-off-by Jeremy Fitzhardinge <[email protected]>

===================================================================

arch/i386/kernel/asm-offsets.c +6 -0

@@ -110,5 +110,11 @@ void foo(void)
 	OFFSET(PARAVIRT_irq_enable_sysexit, paravirt_ops, irq_enable_sysexit);
 	OFFSET(PARAVIRT_iret, paravirt_ops, iret);
 	OFFSET(PARAVIRT_read_cr0, paravirt_ops, read_cr0);
+
+	DEFINE(PARAVIRT_IRQ_DISABLE, PARAVIRT_IRQ_DISABLE);
+	DEFINE(PARAVIRT_IRQ_ENABLE, PARAVIRT_IRQ_ENABLE);
+	DEFINE(PARAVIRT_STI_SYSEXIT, PARAVIRT_STI_SYSEXIT);
+	DEFINE(PARAVIRT_IRQ_DISABLE, PARAVIRT_IRQ_DISABLE);
+	DEFINE(PARAVIRT_INTERRUPT_RETURN, PARAVIRT_INTERRUPT_RETURN);
 #endif
 }

===================================================================

include/asm-i386/paravirt.h +63 ~~-46~~

165 lines not on this page · raw message

Fix a few clobbers to include the return register. The clobbers set is the set of all registers modified (or may be modified) by the code snippet, regardless of whether it was deliberate or accidental.

Signed-off-by Jeremy Fitzhardinge <[email protected]>

===================================================================

include/asm-i386/paravirt.h +3 -3

@@ -597,7 +597,7 @@ static inline unsigned long __raw_local_
 					   "popl %%edx; popl %%ecx")
 			     : "=a"(f): "m"(paravirt_ops.save_fl),
 			       paravirt_type(PARAVIRT_SAVE_FLAGS),
-			       paravirt_clobber(CLBR_NONE)
+			       paravirt_clobber(CLBR_EAX)
 			     : "memory", "cc");
 	return f;
 }

@@ -647,7 +647,7 @@ static inline unsigned long __raw_local_
 			     : "m" (paravirt_ops.save_fl),
 			       "m" (paravirt_ops.irq_disable),
 			       paravirt_type(PARAVIRT_SAVE_FLAGS_IRQ_DISABLE),
-			       paravirt_clobber(CLBR_NONE)
+			       paravirt_clobber(CLBR_EAX)
 			     : "memory", "cc");
 	return f;
 }

@@ -658,7 +658,7 @@ static inline unsigned long __raw_local_
 				 "%c[paravirt_cli_type]", "%c[paravirt_clobber]")
 
 #define STI_STRING _paravirt_alt("pushl %%ecx; pushl %%edx;"		\
-				"call *paravirt_ops+%c[irq_enable];"	\
+				 "call *paravirt_ops+%c[irq_enable];"	\
 				 "popl %%edx; popl %%ecx",		\
 				 "%c[paravirt_cli_type]", "%c[paravirt_clobber]")

Wrap a set of interesting paravirt_ops calls in a wrapper which makes the callsites available for patching. Unfortunately this is pretty ugly because there's no way to get gcc to generate a function call, but also wrap just the callsite itself with the necessary labels.

This patch supports functions with 0-4 arguments, and either void or returning a value. 64-bit arguments must be split into a pair of 32-bit arguments (lower word first). Functions which return structures do so via the stack, with the first hidden argument being the pointer to the returned structure; this is explicitly visible in the wrapped calls.

Signed-off-by Jeremy Fitzhardinge <[email protected]>

===================================================================

include/asm-i386/paravirt.h +333 ~~-53~~

510 lines not on this page · raw message

Implement the actual patching machinery. paravirt_patcher() contains the logic to automatically patch a callsite based on a few simple rules:
- if the paravirt_op function is either NULL or native_nop, then patch nops
- if the paravirt_op function is a jmp target, then jmp to it
- if the paravirt_op function is callable and doesn't clobber too much
for the callsite, call it directly

These rules will remove most of the expensive indirect calls in favour of either a direct call or a pile of nops.

Paravirt backends can also pass in other patcher functions for specific operations if they want to insert literal machine code, or do more complex things. The function paravirt_patch_insns() is available to help with this.

Signed-off-by Jeremy Fitzhardinge <[email protected]>

===================================================================

arch/i386/kernel/paravirt.c +127 ~~-26~~

170 lines not on this page · raw message

===================================================================

arch/i386/xen/enlighten.c +4 -5

@@ -37,11 +37,10 @@ struct start_info *xen_start_info;
 struct start_info *xen_start_info;
 EXPORT_SYMBOL_GPL(xen_start_info);
 
-static unsigned xen_patch(u8 type, u16 clobber, void *firstinsn, unsigned len)
-{
-	/* Xen will require relocations to patch calls and jmps, and
-	   perhaps chunks of inline code */
-	return len;
+static unsigned xen_patch(u8 type, u16 clobber, void *insn, unsigned len)
+{
+	/* Use the default patcher's default options */
+	return paravirt_patcher(type, clobber, insn, len, NULL);
 }
 
 static void __init xen_banner(void)

===================================================================

include/asm-i386/paravirt.h +13 -0

@@ -865,6 +865,19 @@ struct paravirt_patch_site {
 	u16 clobbers;		/* what registers you may clobber */
 };
 
+typedef unsigned (*patcher_t)(void *opfunc, u16 clobbers, void *site, unsigned len);
+
+unsigned paravirt_patcher(u8 type, u16 clobbers, void *insns, unsigned len,
+			  patcher_t patch);
+
+unsigned paravirt_patch_nop(void *opfunc, u16 clobbers, void *site, unsigned len);
+unsigned paravirt_patch_ignore(void *opfunc, u16 clobbers, void *site, unsigned len);
+unsigned paravirt_patch_call(void *opfunc, u16 clobbers, void *site, unsigned len);
+unsigned paravirt_patch_jmp(void *opfunc, u16 clobbers, void *site, unsigned len);
+
+unsigned paravirt_patch_insns(void *site, unsigned len,
+			      const char *start, const char *end);
+
 extern struct paravirt_patch_site __start_parainstructions[],
 	__stop_parainstructions[];

_______________________________________________
Virtualization mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/virtualization

Rusty Russell

2007-02-22 10:55 UTC #3 lore raw

Quoted 13 lines · Jeremy Fitzhardinge

Quoted 12 lines · Rusty Russell
Quoted 3 lines · Jeremy Fitzhardinge
Here's the new patching patch. It compiles, but it doesn't, you know, boot, as such.
OK, there are several separate things here.
(1) Get rid of the PARAVIRT_IRQ_DISABLE etc constants in favour of offsetof within the structure.
(2) Change (almost) all the paravirt hooks to be patchable.
(3) Genericise the infrastructure to table-driven.

Quoted 7 lines

Quoted 2 lines
These should probably become separate patches, in fact.
OK, here's something which is more presentable, and it even boots (at least under Xen; haven't tried native).
paravirt-patch-rename-paravirt_patch.patch

Great.

Quoted 1 line

paravirt-use-offset-site-ids.patch

Cool.

Quoted 2 lines

paravirt-fix-clobbers.patch
- misc cleanups

Cool.

Quoted 3 lines

paravirt-patchable-call-wrappers.patch
- wrap a large number of the paravirt calls with the magic to make the
callsites patchable. More are possible; this is just a good first step.

Ugly, but I originally played with alternatives for a long time and mine were no prettier.

Quoted 3 lines

paravirt-patch-machinery.patch
- the actual patching machinery, which is function-pointer driven
rather than table driven now.

Not quite so sure on this one. We loop through calling paravirt_ops.patch() for each patch. In the native case, this calls paravirt_patcher with a fn pointer, which is simply called by paravirt_patcher.

I would think you want to replace paravirt_patcher's patch == NULL case with a special "paravity_patch_default", and then have to paravirt_ops patch function call that specifically when it wants a default.

Also, I think you can leave the native table almost as-is, eg:

static const struct native_insns

{
       const char *start, *end;
} native_insns[] = {

[PARAVIRT_PATCH(irq_disable)] = { start_cli, end_cli }, [PARAVIRT_PATCH(irq_enable)] = { start_sti, end_sti },
...
};
static unsigned native_patch(u8 type, u16 clobbers, void *insns, unsigned len)

{
       unsigned int insn_len;

       /* Don't touch it if we don't have a replacement */
       if (type >= ARRAY_SIZE(native_insns) || !native_insns[type].start)
               return paravirt_patch_default(type, clobbers, insns, len);
       insn_len = native_insns[type].end - native_insns[type].start;

       /* Similarly if we can't fit replacement. */
       if (len < insn_len)
               return paravirt_patch_default(type, clobbers, insns, len);

       return paravirt_patch_insns(insns, len, native_insns[type].start, native_insns[type].end);
}

Actually, your paravirt_patch_insns has similar logic anyway, so this code could collapse (it should fall back to paravirt_patch_default tho IMHO).

Thanks for doing this work!
Rusty.

Jeremy Fitzhardinge

author

2007-02-22 18:13 UTC #4 lore raw

Quoted 4 lines · Rusty Russell

Actually, your paravirt_patch_insns has similar logic anyway, so this code could collapse (it should fall back to paravirt_patch_default tho IMHO).

I wanted to get rid of the table because its now sparse, and possibly fairly large (since the special save flags & disable is at offset 0x80). And a switch would be pretty clean anyway.

How does this look? Compiles, but untested.

These rules will remove most of the expensive indirect calls in favour of either a direct call or a pile of nops.

Signed-off-by Jeremy Fitzhardinge <[email protected]>

arch/i386/kernel/paravirt.c +118 ~~-25~~

diff -r 30afc1621aa1 arch/i386/kernel/paravirt.c
@@ -53,6 +53,7 @@ char *memory_setup(void)
 #define DEF_NATIVE(name, code)					\
 	extern const char start_##name[], end_##name[];		\
 	asm("start_" #name ": " code "; end_" #name ":")
+
 DEF_NATIVE(cli, "cli");
 DEF_NATIVE(sti, "sti");
 DEF_NATIVE(popf, "push %eax; popf");

@@ -61,36 +62,128 @@ DEF_NATIVE(iret, "iret");
 DEF_NATIVE(iret, "iret");
 DEF_NATIVE(sti_sysexit, "sti; sysexit");
 
-static const struct native_insns
-{
-	const char *start, *end;
-} native_insns[] = {
-	[PARAVIRT_IRQ_DISABLE] = { start_cli, end_cli },
-	[PARAVIRT_IRQ_ENABLE] = { start_sti, end_sti },
-	[PARAVIRT_RESTORE_FLAGS] = { start_popf, end_popf },
-	[PARAVIRT_SAVE_FLAGS] = { start_pushf, end_pushf },
-	[PARAVIRT_SAVE_FLAGS_IRQ_DISABLE] = { start_pushf_cli, end_pushf_cli },
-	[PARAVIRT_INTERRUPT_RETURN] = { start_iret, end_iret },
-	[PARAVIRT_STI_SYSEXIT] = { start_sti_sysexit, end_sti_sysexit },
-};
+static unsigned native_patch_insns(u8 type, u16 clobbers, void *insns, unsigned len)
+{
+	const unsigned char *start, *end;
+
+	switch(type) {
+	case PARAVIRT_IRQ_DISABLE:
+		start = start_cli;
+		end = end_cli;
+		break;
+	case PARAVIRT_IRQ_ENABLE:
+		start = start_sti;
+		end = end_sti;
+		break;
+	case PARAVIRT_RESTORE_FLAGS:
+		start = start_popf;
+		end = end_popf;
+		break;
+	case PARAVIRT_SAVE_FLAGS:
+		start = start_pushf;
+		end = end_pushf;
+		break;
+	case PARAVIRT_SAVE_FLAGS_IRQ_DISABLE:
+		start = start_pushf_cli;
+		end = end_pushf_cli;
+		break;
+	case PARAVIRT_INTERRUPT_RETURN:
+		start = start_iret;
+		end = end_iret;
+		break;
+	case PARAVIRT_STI_SYSEXIT:
+		start = start_sti_sysexit;
+		end = end_sti_sysexit;
+		break;
+
+	default:
+		return 0;
+	}
+
+	return paravirt_patch_insns(insns, len, start, end);
+}
 
 static unsigned native_patch(u8 type, u16 clobbers, void *insns, unsigned len)
 {
-	unsigned int insn_len;
-
-	/* Don't touch it if we don't have a replacement */
-	if (type >= ARRAY_SIZE(native_insns) || !native_insns[type].start)
-		return len;
-
-	insn_len = native_insns[type].end - native_insns[type].start;
-
-	/* Similarly if we can't fit replacement. */
-	if (len < insn_len)
-		return len;
-
-	memcpy(insns, native_insns[type].start, insn_len);
+	unsigned ret = native_patch_insns(type, clobbers, insns, len);
+
+	if (ret == 0)
+		ret = paravirt_patch_default(type, clobbers, insns, len);
+
+	return ret;
+}
+
+unsigned paravirt_patch_nop(u8 type, u16 clobbers, void *site, unsigned len)
+{
+	return 0;
+}
+
+unsigned paravirt_patch_ignore(u8 type, u16 clobbers, void *site, unsigned len)
+{
+	return len;
+}
+
+unsigned paravirt_patch_call(u8 type, u16 clobbers, void *site, unsigned len)
+{
+	void *target = *((void **)&paravirt_ops + type);
+	unsigned char *call = site;
+	unsigned long delta = (unsigned long)target - (unsigned long)(call+5);
+
+	if (clobbers != CLBR_ANY)
+		return len;	/* calling C would clobber too much */
+	if (len < 5)
+		return len;	/* call too long for patch site */
+
+	*call++ = 0xe8;		/* call */
+	*(unsigned long *)call = delta;
+
+	return 5;
+}
+
+unsigned paravirt_patch_jmp(u8 type, u16 clobbers, void *site, unsigned len)
+{
+	void *target = *((void **)&paravirt_ops + type);
+	unsigned char *jmp = site;
+	unsigned long delta = (unsigned long)target - (unsigned long)(jmp+5);
+
+	if (len < 5)
+		return len;	/* call too long for patch site */
+
+	*jmp++ = 0xe9;		/* jmp */
+	*(unsigned long *)jmp = delta;
+
+	return 5;
+}
+
+unsigned paravirt_patch_default(u8 type, u16 clobbers, void *site, unsigned len)
+{
+	void *opfunc = *((void **)&paravirt_ops + type);
+	unsigned ret;
+
+	if (opfunc == (void *)native_nop || opfunc == NULL)
+		ret = paravirt_patch_nop(type, clobbers, site, len);
+	else if (type == PARAVIRT_PATCH(iret) ||
+		 type == PARAVIRT_PATCH(irq_enable_sysexit))
+		ret = paravirt_patch_jmp(type, clobbers, site, len);
+	else
+		ret = paravirt_patch_jmp(type, clobbers, site, len);
+
+	return ret;
+}
+
+unsigned paravirt_patch_insns(void *site, unsigned len,
+			      const char *start, const char *end)
+{
+	unsigned insn_len = end - start;
+
+	if (insn_len > len || start == NULL)
+		insn_len = len;
+	else
+		memcpy(site, start, insn_len);
+
 	return insn_len;
 }
+
 
 static unsigned long native_get_debugreg(int regno)
 {

arch/i386/xen/enlighten.c +1 -8

diff -r 30afc1621aa1 arch/i386/xen/enlighten.c
@@ -36,13 +36,6 @@ extern const char xen_sti_sysexit[];
 
 struct start_info *xen_start_info;
 EXPORT_SYMBOL_GPL(xen_start_info);
-
-static unsigned xen_patch(u8 type, u16 clobber, void *firstinsn, unsigned len)
-{
-	/* Xen will require relocations to patch calls and jmps, and
-	   perhaps chunks of inline code */
-	return len;
-}
 
 static void __init xen_banner(void)
 {

@@ -619,7 +612,7 @@ static const struct paravirt_ops xen_par
 	.name = "Xen",
 	.banner = xen_banner,
 
-	.patch = xen_patch,
+	.patch = paravirt_patch_default,
 
 	.memory_setup = xen_memory_setup,
 	.arch_setup = xen_arch_setup,

include/asm-i386/paravirt.h +11 -0

diff -r 30afc1621aa1 include/asm-i386/paravirt.h
@@ -865,6 +865,17 @@ struct paravirt_patch_site {
 	u16 clobbers;		/* what registers you may clobber */
 };
 
+typedef unsigned (*patcher_t)(u8 type, u16 clobbers, void *site, unsigned len);
+
+unsigned paravirt_patch_nop(u8 type, u16 clobbers, void *site, unsigned len);
+unsigned paravirt_patch_ignore(u8 type, u16 clobbers, void *site, unsigned len);
+unsigned paravirt_patch_call(u8 type, u16 clobbers, void *site, unsigned len);
+unsigned paravirt_patch_jmp(u8 type, u16 clobbers, void *site, unsigned len);
+unsigned paravirt_patch_default(u8 type, u16 clobbers, void *site, unsigned len);
+
+unsigned paravirt_patch_insns(void *site, unsigned len,
+			      const char *start, const char *end);
+
 extern struct paravirt_patch_site __start_parainstructions[],
 	__stop_parainstructions[];

_______________________________________________
Virtualization mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/virtualization

Zachary Amsden

2007-02-22 22:14 UTC #5 lore raw

Quoted 8 lines · Jeremy Fitzhardinge

Quoted 5 lines · Rusty Russell
Actually, your paravirt_patch_insns has similar logic anyway, so this code could collapse (it should fall back to paravirt_patch_default tho IMHO).

Quoted 6 lines

I wanted to get rid of the table because its now sparse, and possibly fairly large (since the special save flags & disable is at offset 0x80). And a switch would be pretty clean anyway.
How does this look? Compiles, but untested.

That's looking pretty spiffy, if you ask me.

Zach

Jeremy Fitzhardinge

author

2007-02-22 22:49 UTC #6 lore raw

Quoted 1 line · Zachary Amsden

That's looking pretty spiffy, if you ask me.

Cool. I had a quick look to see what you're doing in vmi and decided to let you work out what to do...

That version had a couple of bugs in it, but the pushed version works for me (PAE at least; fixing non-PAE at the moment). I also included Ingo's patch to use -freg-struct-return, with the corresponding updates to paravirt.h, since they make more of the native operations noops.

Ian Campbell

2007-02-22 22:51 UTC #7 lore raw

In reply to Jeremy Fitzhardinge

arch/i386/kernel/paravirt.c DEF_NATIVE(iret, "iret"); · line 169

Quoted 13 lines · Jeremy Fitzhardinge

unsigned paravirt_patch_default(u8 type, u16 clobbers, void *site, unsigned len)

arch/i386/kernel/paravirt.c +11 -0

+{
+       void *opfunc = *((void **)&paravirt_ops + type);
+       unsigned ret;
+
+       if (opfunc == (void *)native_nop || opfunc == NULL)
+               ret = paravirt_patch_nop(type, clobbers, site, len);
+       else if (type == PARAVIRT_PATCH(iret) ||
+                type == PARAVIRT_PATCH(irq_enable_sysexit))
+               ret = paravirt_patch_jmp(type, clobbers, site, len);
+       else
+               ret = paravirt_patch_jmp(type, clobbers, site, len);

Should be a paravirt_patch_call?

Ian.

Jeremy Fitzhardinge

author

2007-02-22 22:54 UTC #8 lore raw

Quoted 8 lines · Ian Campbell

arch/i386/kernel/paravirt.c DEF_NATIVE(iret, "iret"); · line 169
Quoted 6 lines
arch/i386/kernel/paravirt.c +5 -0
+       else if (type == PARAVIRT_PATCH(iret) ||
+                type == PARAVIRT_PATCH(irq_enable_sysexit))
+               ret = paravirt_patch_jmp(type, clobbers, site, len);
+       else
+               ret = paravirt_patch_jmp(type, clobbers, site, len);
Should be a paravirt_patch_call?

Yep. I noticed that after I posted it. Should work aside from that.