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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > ---
> >  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?

However, now I am thinking that we should add "memory" to clobbers when
"rep movsb" and similar thing are used here.

Daniel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel