From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH for-xen-4.5 v4 05/18] x86/boot/reloc: create generic alloc and copy functions Date: Fri, 17 Oct 2014 18:22:48 +0100 Message-ID: <54415068.6080102@citrix.com> References: <1413555132-22138-1-git-send-email-daniel.kiper@oracle.com> <1413555132-22138-6-git-send-email-daniel.kiper@oracle.com> <54413E11.70307@citrix.com> <20141017171102.GI3467@olila.local.net-space.pl> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XfBEt-0007zN-8t for xen-devel@lists.xenproject.org; Fri, 17 Oct 2014 17:22:55 +0000 In-Reply-To: <20141017171102.GI3467@olila.local.net-space.pl> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Daniel Kiper Cc: jgross@suse.com, keir@xen.org, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, ross.philipson@citrix.com, roy.franz@linaro.org, ning.sun@intel.com, jbeulich@suse.com, xen-devel@lists.xenproject.org, qiaowei.ren@intel.com, richard.l.maliszewski@intel.com, gang.wei@intel.com, fu.wei@linaro.org List-Id: xen-devel@lists.xenproject.org On 17/10/14 18:11, Daniel Kiper wrote: > On Fri, Oct 17, 2014 at 05:04:33PM +0100, Andrew Cooper wrote: >> On 17/10/14 15:11, Daniel Kiper wrote: >>> Create generic alloc and copy functions. We need them to >>> introduce MBD struct and multiboot2 protocol. Please >>> check later patches for more details. >> As I asked before, you need to make some comment about changing the >> types and introducing loads of casts. Its fine to say that the eventual >> code will use u32s everywhere, but this information must be in the >> commit message. > OK. > >>> Signed-off-by: Daniel Kiper >>> --- >>> xen/arch/x86/boot/reloc.c | 52 ++++++++++++++++++++++++++++----------------- >>> 1 file changed, 33 insertions(+), 19 deletions(-) >>> >>> diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c >>> index 4609e55..e1c83f4 100644 >>> --- a/xen/arch/x86/boot/reloc.c >>> +++ b/xen/arch/x86/boot/reloc.c >>> @@ -33,9 +33,10 @@ asm ( >>> typedef unsigned int u32; >>> #include "../../../include/xen/multiboot.h" >>> >>> -static void *reloc_mbi_struct(void *old, unsigned int bytes) >>> +static u32 alloc_struct(u32 bytes) >>> { >>> - void *new; >>> + u32 s; >>> + >>> asm( >>> " call 1f \n" >>> "1: pop %%edx \n" >>> @@ -43,50 +44,63 @@ static void *reloc_mbi_struct(void *old, unsigned int bytes) >>> " sub %1,%0 \n" >>> " and $~15,%0 \n" >>> " mov %0,alloc-1b(%%edx) \n" >>> - " mov %0,%%edi \n" >>> - " rep movsb \n" >>> - : "=&r" (new), "+c" (bytes), "+S" (old) >>> - : : "edx", "edi"); >>> - return new; >>> + : "=&r" (s) : "r" (bytes) : "edx"); >>> + >>> + return s; >>> } >>> >>> -static char *reloc_mbi_string(char *old) >>> +static u32 copy_struct(u32 src, u32 bytes) >>> +{ >>> + u32 dst, dst_asm; >>> + >>> + dst = alloc_struct(bytes); >>> + dst_asm = dst; >>> + >>> + asm volatile("rep movsb" : "+S" (src), "+D" (dst_asm), "+c" (bytes)); >> All 3 registers are only used as inputs, and the modified values are not >> useful. You can drop dst_asm entirely. See the stack resync code in >> __start_xen() as an example. > "rep movsb" clobbers S, D and c. If you put them as inputs then you are > not able to tell GCC that they are clobbered. It means that GCC will assume > that e.g. D contents, which in turn is assigned to dst variable, is the same > before and after "asm volatile" which is not true. So, I do not know how > 'asm volatile("rep movsb"...' in __start_xen() works and nothing has not > blown out yet... Probably it is just fortunate coincidence. In my case it > did not work (to be precise, sometime it worked). Hence, that is why I must > put all input data as input/output and define additional variable which > is used by "asm volatile" only. Maybe we should consider fixing this stuff > in __start_xen() too... Are there other places with that bad guys? Hmm - is appears that __start_xen() is saved because it only clobbers the registers which are also clobbered by the unconditional call to bootstrap_unmap() following it. I will take a look for other issues. > > However, now I am thinking that we should add "memory" to clobbers when > "rep movsb" and similar thing are used here. Yes - the memory clobber is very important. ~Andrew