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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

>
> 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.

~Andrew

> +
> +    return dst;
> +}
> +
> +static u32 copy_string(u32 src)
>  {
>      char *p;
> -    for ( p = old; *p != '\0'; p++ )
> +
> +    if ( src == 0 )
> +        return 0;
> +
> +    for ( p = (char *)src; *p != '\0'; p++ )
>          continue;
> -    return reloc_mbi_struct(old, p - old + 1);
> +
> +    return copy_struct(src, p - (char *)src + 1);
>  }
>  
>  multiboot_info_t *reloc(multiboot_info_t *mbi_old)
>  {
> -    multiboot_info_t *mbi = reloc_mbi_struct(mbi_old, sizeof(*mbi));
> +    multiboot_info_t *mbi = (multiboot_info_t *)copy_struct((u32)mbi_old, 
> sizeof(*mbi));
>      int i;
>  
>      if ( mbi->flags & MBI_CMDLINE )
> -        mbi->cmdline = (u32)reloc_mbi_string((char *)mbi->cmdline);
> +        mbi->cmdline = copy_string(mbi->cmdline);
>  
>      if ( mbi->flags & MBI_MODULES )
>      {
> -        module_t *mods = reloc_mbi_struct(
> -            (module_t *)mbi->mods_addr, mbi->mods_count * sizeof(module_t));
> +        module_t *mods = (module_t *)copy_struct(
> +            mbi->mods_addr, mbi->mods_count * sizeof(module_t));
>  
>          mbi->mods_addr = (u32)mods;
>  
>          for ( i = 0; i < mbi->mods_count; i++ )
>          {
>              if ( mods[i].string )
> -                mods[i].string = (u32)reloc_mbi_string((char 
> *)mods[i].string);
> +                mods[i].string = copy_string(mods[i].string);
>          }
>      }
>  
>      if ( mbi->flags & MBI_MEMMAP )
> -        mbi->mmap_addr = (u32)reloc_mbi_struct(
> -            (memory_map_t *)mbi->mmap_addr, mbi->mmap_length);
> +        mbi->mmap_addr = copy_struct(mbi->mmap_addr, mbi->mmap_length);
>  
>      if ( mbi->flags & MBI_LOADERNAME )
> -        mbi->boot_loader_name = (u32)reloc_mbi_string(
> -            (char *)mbi->boot_loader_name);
> +        mbi->boot_loader_name = copy_string(mbi->boot_loader_name);
>  
>      /* Mask features we don't understand or don't relocate. */
>      mbi->flags &= (MBI_MEMLIMITS |



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