Re: [PATCH for-xen-4.5 v4 05/18] x86/boot/reloc: create generic alloc and copy functions

[Andrew Cooper <andrew.cooper3@citrix.com>]

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 <daniel.kiper@oracle.com>
>>> ---
>>>  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