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