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

[Zachary Amsden <zach@vmware.com>]

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