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

[Jeremy Fitzhardinge <jeremy@goop.org>]

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