[RFC, PATCH 2/5] Paravirt_ops patch bugs.patch

[RFC, PATCH 2/5] Paravirt_ops patch bugs.patch

[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.

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 <zach@vmware.com>

diff -r fcb59055d2b2 arch/i386/kernel/paravirt.c
--- a/arch/i386/kernel/paravirt.c	Thu Apr 19 12:36:06 2007 -0700
+++ b/arch/i386/kernel/paravirt.c	Thu Apr 19 15:42:02 2007 -0700
@@ -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);

Re: [RFC, PATCH 2/5] Paravirt_ops patch bugs.patch

[Jeremy Fitzhardinge]

Zachary Amsden wrote:
> 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.

    J

Re: [RFC, PATCH 2/5] Paravirt_ops patch bugs.patch

[Zachary Amsden]

Jeremy Fitzhardinge wrote:
> Zachary Amsden wrote:
>   
>> 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.
>   

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

Re: [RFC, PATCH 2/5] Paravirt_ops patch bugs.patch

[Jeremy Fitzhardinge]

Zachary Amsden wrote:
> Jeremy Fitzhardinge wrote:
>> Zachary Amsden wrote:
>>  
>>> 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.
>>   
>
> 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.

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

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

> 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.

    J

Re: [RFC, PATCH 2/5] Paravirt_ops patch bugs.patch

[Zachary Amsden]

Jeremy Fitzhardinge wrote:
> Zachary Amsden wrote:
>   
>> Jeremy Fitzhardinge wrote:
>>     
>>> Zachary Amsden wrote:
>>>  
>>>       
>>>> 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.
>>>   
>>>       
>> 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.
>
>   
>> 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?
>>     
>
> 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..


>> 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