From mboxrd@z Thu Jan 1 00:00:00 1970 From: Zachary Amsden Subject: Re: [RFC, PATCH 2/5] Paravirt_ops patch bugs.patch Date: Thu, 19 Apr 2007 22:28:52 -0700 Message-ID: <46284F94.7010901@vmware.com> References: <20070420015234.5D42EBFC@zach-dev2.vmware.com> <46284406.2080007@goop.org> <46284AFB.1000307@vmware.com> <46284D4C.1070007@goop.org> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <46284D4C.1070007@goop.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org To: Jeremy Fitzhardinge Cc: Andrew Morton , Petr Vandrovec , Chaz Masden , Virtualization Mailing List , Chris Wright , Andi Kleen , Ingo Molnar List-Id: virtualization@lists.linuxfoundation.org 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