Paravirt_ops patch bugs.patch

RFC PATCH In review Zachary Amsden 2 participants 5 messages Apr 20, 2007

Subsystems (2)

virtio core PARAVIRT_OPS interface

lore

Share & tools

lore

Zachary Amsden

author

2007-04-20 01:52 UTC #0 lore raw

RFC PATCH 2/5 Paravirt_ops patch bugs.patch

Failing to patch because not enough space is available for a call or jump or because the site clobbers do not allow the target clobbers to fit is a fatal error; it means the kernel can not be properly virtualized.

In patch_insns, we just keep a warning instead; it may not be fatal, as they paravirt-ops backend may just be trying to insert a greedy but not necessary inline function.

Signed-off-by Zachary Amsden <[email protected]>

arch/i386/kernel/paravirt.c +7 -7

diff -r fcb59055d2b2 arch/i386/kernel/paravirt.c
@@ -132,10 +140,8 @@ unsigned paravirt_patch_call(void *targe
 	unsigned char *call = site;
 	unsigned long delta = (unsigned long)target - (unsigned long)(call+5);
 
-	if (tgt_clobbers & ~site_clobbers)
-		return len;	/* target would clobber too much for this site */
-	if (len < 5)
-		return len;	/* call too long for patch site */
+	BUG_ON(tgt_clobbers & ~site_clobbers);
+	BUG_ON(len < 5);
 
 	*call++ = 0xe8;		/* call */
 	*(unsigned long *)call = delta;

@@ -148,8 +154,7 @@ unsigned paravirt_patch_jmp(void *target
 	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 */
+	BUG_ON(len < 5);
 
 	*jmp++ = 0xe9;		/* jmp */
 	*(unsigned long *)jmp = delta;

@@ -186,7 +191,10 @@ unsigned paravirt_patch_insns(void *site
 {
 	unsigned insn_len = end - start;
 
-	if (insn_len > len || start == NULL)
+	WARN_ON(insn_len > len);
+	BUG_ON(start == NULL);
+
+	if (insn_len > len)
 		insn_len = len;
 	else
 		memcpy(site, start, insn_len);

Jeremy Fitzhardinge

2007-04-20 04:39 UTC #1 lore raw

Quoted 4 lines · Zachary Amsden

Failing to patch because not enough space is available for a call or jump or because the site clobbers do not allow the target clobbers to fit is a fatal error; it means the kernel can not be properly virtualized.

No, that doesn't follow. If the original site was:

patchable_start:
push %eax
push %ecx
push %edx
call *paravirt_ops + thingy
pop
pop
pop
patchable_end:

then its perfectly OK to leave it as-is, even if the direct call's destination clobbers are mismatched. If the patcher wants to generate a call to a C function in a context which can't deal with normal C calling conventions, then it needs to also patch in appropriate save/restores.

Zachary Amsden

author

2007-04-20 05:09 UTC #2 lore raw

Quoted 20 lines · Jeremy Fitzhardinge

Quoted 5 lines · Zachary Amsden
Failing to patch because not enough space is available for a call or jump or because the site clobbers do not allow the target clobbers to fit is a fatal error; it means the kernel can not be properly virtualized.
No, that doesn't follow. If the original site was:
patchable_start:
push %eax
push %ecx
push %edx
call *paravirt_ops + thingy
pop
pop
pop
patchable_end:

Quoted 5 lines

then its perfectly OK to leave it as-is, even if the direct call's destination clobbers are mismatched. If the patcher wants to generate a call to a C function in a context which can't deal with normal C calling conventions, then it needs to also patch in appropriate save/restores.

The example is a bit misconstrued. In this case, the clobbers for the patchable region are CLBR_ALL - so there is no possibility of mismatch because of expanded clobber list. If the patchable region consisted of this, it would be bad:

push %eax
push %ecx
patchable_start:
push %edx
call *paravirt_ops + thingy
pop
patchable_end: (note - site clobber EDX ok)
pop
pop

But, why would you do that?

Calls through paravirt_ops function pointers are C function calls. Failing to provide a patchable region which can make a C function call is a BUG().

Zach

Jeremy Fitzhardinge

2007-04-20 05:19 UTC #3 lore raw

Quoted 27 lines · Zachary Amsden

Quoted 20 lines · Jeremy Fitzhardinge
Quoted 5 lines · Zachary Amsden
Failing to patch because not enough space is available for a call or jump
or because the site clobbers do not allow the target clobbers to fit is a fatal error; it means the kernel can not be properly virtualized.
No, that doesn't follow. If the original site was:
patchable_start:
push %eax
push %ecx
push %edx
call *paravirt_ops + thingy
pop
pop
pop
patchable_end:
Quoted 5 lines
then its perfectly OK to leave it as-is, even if the direct call's destination clobbers are mismatched. If the patcher wants to generate a call to a C function in a context which can't deal with normal C calling conventions, then it needs to also patch in appropriate save/restores.

Quoted 3 lines

The example is a bit misconstrued. In this case, the clobbers for the patchable region are CLBR_ALL - so there is no possibility of mismatch because of expanded clobber list.

In my example, it would be CLBR_NONE, meaning that the surrounding code expects no registers to be clobbered. We have constructions like this for paravirt_ops calls in entry.S, in contexts where no clobbers are acceptable, let alone normal C calls.

Quoted 13 lines

If the patchable region consisted of this, it would be bad:
push %eax
push %ecx
patchable_start:
push %edx
call *paravirt_ops + thingy
pop
patchable_end: (note - site clobber EDX ok)
pop
pop

Quoted 1 line

But, why would you do that?

We have done that in the past, to give the inlined code free access to some scratch registers, but without clobbering everything.

Quoted 3 lines

Calls through paravirt_ops function pointers are C function calls. Failing to provide a patchable region which can make a C function call is a BUG().

That's not the case at the moment.

Zachary Amsden

author

2007-04-20 05:28 UTC #4 lore raw

Quoted 55 lines · Jeremy Fitzhardinge

Quoted 29 lines · Zachary Amsden
Quoted 21 lines · Jeremy Fitzhardinge
Quoted 6 lines · Zachary Amsden
Failing to patch because not enough space is available for a call or jump
or because the site clobbers do not allow the target clobbers to fit is a fatal error; it means the kernel can not be properly virtualized.
No, that doesn't follow. If the original site was:
patchable_start:
push %eax
push %ecx
push %edx
call *paravirt_ops + thingy
pop
pop
pop
patchable_end:
Quoted 6 lines
then its perfectly OK to leave it as-is, even if the direct call's destination clobbers are mismatched. If the patcher wants to generate a call to a C function in a context which can't deal with normal C calling conventions, then it needs to also patch in appropriate save/restores.
Quoted 4 lines
The example is a bit misconstrued. In this case, the clobbers for the patchable region are CLBR_ALL - so there is no possibility of mismatch because of expanded clobber list.
In my example, it would be CLBR_NONE, meaning that the surrounding code expects no registers to be clobbered. We have constructions like this for paravirt_ops calls in entry.S, in contexts where no clobbers are acceptable, let alone normal C calls.
Quoted 13 lines
If the patchable region consisted of this, it would be bad:
push %eax
push %ecx
patchable_start:
push %edx
call *paravirt_ops + thingy
pop
patchable_end: (note - site clobber EDX ok)
pop
pop

Quoted 6 lines

Quoted 2 lines
But, why would you do that?
We have done that in the past, to give the inlined code free access to some scratch registers, but without clobbering everything.

Yes, we still do that, but..

Quoted 6 lines

Quoted 3 lines
Calls through paravirt_ops function pointers are C function calls. Failing to provide a patchable region which can make a C function call is a BUG().
That's not the case at the moment.

All of the patch sites can make C function calls, perhaps with fewer clobbers. If the implementation cannot fit a proper save / restore and call to the C function into the allotted space, it still is a BUG(). If it has unique clobber requirements which go beyond the patch site clobbers, surely it can't use the generic patch code for those operations. But it still must have the space.

Zach