Re: [Xen-devel] [PATCH v5 06/16] x86/boot/reloc: create generic alloc and copy functions

2016-09-06 Thread Doug Goldstein 
On 8/30/16 9:32 AM, Daniel Kiper wrote:
> On Thu, Aug 25, 2016 at 05:34:31AM -0600, Jan Beulich wrote:
> On 20.08.16 at 00:43,  wrote:
>>> Create generic alloc and copy functions. We need
>>> separate tools for memory allocation and copy to
>>> provide multiboot2 protocol support.
>>>
>>> Signed-off-by: Daniel Kiper 
>>
>> The amount of casting in this patch alone looks very reasonable now.
>> Before ack-ing this and respective subsequent patches I'd like to see
>> the final result though. To facilitate that I have to re-raise a previously
>> asked question: Do you have a tree somewhere which one could use
>> to look at the final result?
> 
> Sadly no.
> 
> Daniel
> 

Daniel,

I'd encourage you to go to https://github.com/xen-project/xen and "fork"
the project and use that space to push your branches. It's free (as in
beer) hosting for any long series and you can enable
https://travis-ci.org to do some basic build tests on patch series
before you mail them out.

-- 
Doug Goldstein



signature.asc
Description: OpenPGP digital signature
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v5 06/16] x86/boot/reloc: create generic alloc and copy functions

2016-09-01 Thread Jan Beulich 
>>> On 31.08.16 at 21:39,  wrote:
> I understood that you need reloc.c after this patch but it looks
> that I was wrong. So, here it is after applying whole series.

Thanks. Here are my notes:

All callers of alloc_mem() cast the result to a pointer. While most of
them also need the result as an integer, there's one exception:

ptr = alloc_mem(sizeof(*mbi_out));
mbi_out = (multiboot_info_t *)ptr;
zero_mem(ptr, sizeof(*mbi_out));

Since this is also the only caller of zero_mem() it tells me that less
casting would be needed if alloc_mem() returned void *, and if
zero_mem() took void * for its first parameter.

Otoh it looks like copy_{mem,string}() are best left the way they are
now.

The amount of casting in e.g.

for ( tag = (multiboot2_tag_t *)ptr;
  (u32)tag - mbi_in < ((multiboot2_fixed_t *)mbi_in)->total_size;
  tag = (multiboot2_tag_t *)ALIGN_UP((u32)tag + tag->size, 
MULTIBOOT2_TAG_ALIGN) )

is still ugly, the more that the entire construct exists more than once.
One thing to consider (which would make things at least a little less
fragile) would be to make tag (and maybe then also mbi_in) a union of
u32 and multiboot2_fixed_t *. For parameter passing purposes from
reloc() it may then be desirable for this to actually be a transparent
union.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v5 06/16] x86/boot/reloc: create generic alloc and copy functions

2016-08-31 Thread Daniel Kiper 
On Wed, Aug 31, 2016 at 09:25:57AM -0600, Jan Beulich wrote:
> >>> On 31.08.16 at 17:13,  wrote:
> > On Tue, Aug 30, 2016 at 09:12:45AM -0600, Jan Beulich wrote:
> >> >>> On 30.08.16 at 16:32,  wrote:
> >> > On Thu, Aug 25, 2016 at 05:34:31AM -0600, Jan Beulich wrote:
> >> >> >>> On 20.08.16 at 00:43,  wrote:
> >> >> > Create generic alloc and copy functions. We need
> >> >> > separate tools for memory allocation and copy to
> >> >> > provide multiboot2 protocol support.
> >> >> >
> >> >> > Signed-off-by: Daniel Kiper 
> >> >>
> >> >> The amount of casting in this patch alone looks very reasonable now.
> >> >> Before ack-ing this and respective subsequent patches I'd like to see
> >> >> the final result though. To facilitate that I have to re-raise a 
> >> >> previously
> >> >> asked question: Do you have a tree somewhere which one could use
> >> >> to look at the final result?
> >> >
> >> > Sadly no.
> >>
> >> Alternatively, could you simply send the final resulting source file?
> >
> > Please look below...
>
> I don't think that was the file at the end of the series, as asked
> for above? There's no mb2 code in there afaics...

I understood that you need reloc.c after this patch but it looks
that I was wrong. So, here it is after applying whole series.

Daniel

/*
 * reloc.c
 *
 * 32-bit flat memory-map routines for relocating Multiboot structures
 * and modules. This is most easily done early with paging disabled.
 *
 * Copyright (c) 2009, Citrix Systems, Inc.
 * Copyright (c) 2013-2016 Oracle and/or its affiliates. All rights reserved.
 *
 * Authors:
 *Keir Fraser 
 *Daniel Kiper 
 */

/*
 * This entry point is entered from xen/arch/x86/boot/head.S with:
 *   - 0x4(%esp) = MULTIBOOT_MAGIC,
 *   - 0x8(%esp) = MULTIBOOT_INFORMATION_ADDRESS,
 *   - 0xc(%esp) = BOOT_TRAMPOLINE_ADDRESS.
 */
asm (
".text \n"
".globl _start \n"
"_start:   \n"
"jmp  reloc\n"
);

typedef unsigned int u32;
typedef unsigned long long u64;

#include "../../../include/xen/multiboot.h"
#include "../../../include/xen/multiboot2.h"

#define NULL((void *)0)

#define __stdcall   __attribute__((__stdcall__))

#define ALIGN_UP(arg, align) \
(((arg) + (align) - 1) & ~((typeof(arg))(align) - 1))

#define get_mb2_data(tag, type, member)   (((multiboot2_tag_##type##_t 
*)(tag))->member)
#define get_mb2_string(tag, type, member) ((u32)get_mb2_data(tag, type, member))

static u32 alloc;

static u32 alloc_mem(u32 bytes)
{
return alloc -= ALIGN_UP(bytes, 16);
}

static void zero_mem(u32 s, u32 bytes)
{
while ( bytes-- )
*(char *)s++ = 0;
}

static u32 copy_mem(u32 src, u32 bytes)
{
u32 dst, dst_ret;

dst = alloc_mem(bytes);
dst_ret = dst;

while ( bytes-- )
*(char *)dst++ = *(char *)src++;

return dst_ret;
}

static u32 copy_string(u32 src)
{
u32 p;

if ( src == 0 )
return 0;

for ( p = src; *(char *)p != '\0'; p++ )
continue;

return copy_mem(src, p - src + 1);
}

static multiboot_info_t *mbi_mbi(u32 mbi_in)
{
int i;
multiboot_info_t *mbi_out;

mbi_out = (multiboot_info_t *)copy_mem(mbi_in, sizeof(*mbi_out));

if ( mbi_out->flags & MBI_CMDLINE )
mbi_out->cmdline = copy_string(mbi_out->cmdline);

if ( mbi_out->flags & MBI_MODULES )
{
module_t *mods;

mbi_out->mods_addr = copy_mem(mbi_out->mods_addr,
  mbi_out->mods_count * sizeof(module_t));

mods = (module_t *)mbi_out->mods_addr;

for ( i = 0; i < mbi_out->mods_count; i++ )
{
if ( mods[i].string )
mods[i].string = copy_string(mods[i].string);
}
}

if ( mbi_out->flags & MBI_MEMMAP )
mbi_out->mmap_addr = copy_mem(mbi_out->mmap_addr, mbi_out->mmap_length);

if ( mbi_out->flags & MBI_LOADERNAME )
mbi_out->boot_loader_name = copy_string(mbi_out->boot_loader_name);

/* Mask features we don't understand or don't relocate. */
mbi_out->flags &= (MBI_MEMLIMITS |
   MBI_CMDLINE |
   MBI_MODULES |
   MBI_MEMMAP |
   MBI_LOADERNAME);

return mbi_out;
}

static multiboot_info_t *mbi2_mbi(u32 mbi_in)
{
const multiboot2_memory_map_t *mmap_src;
const multiboot2_tag_t *tag;
module_t *mbi_out_mods = NULL;
memory_map_t *mmap_dst;
multiboot_info_t *mbi_out;
u32 ptr;
unsigned int i, mod_idx = 0;

ptr = alloc_mem(sizeof(*mbi_out));
mbi_out = (multiboot_info_t *)ptr;
zero_mem(ptr, sizeof(*mbi_out));

/* Skip Multiboot2 information fixed part. */
ptr = ALIGN_UP(mbi_in + sizeof(multiboot2_fixed_t), MULTIBOOT2_TAG_ALIGN);

/* Get the number of

Re: [Xen-devel] [PATCH v5 06/16] x86/boot/reloc: create generic alloc and copy functions

2016-08-31 Thread Jan Beulich 
>>> On 31.08.16 at 17:13,  wrote:
> On Tue, Aug 30, 2016 at 09:12:45AM -0600, Jan Beulich wrote:
>> >>> On 30.08.16 at 16:32,  wrote:
>> > On Thu, Aug 25, 2016 at 05:34:31AM -0600, Jan Beulich wrote:
>> >> >>> On 20.08.16 at 00:43,  wrote:
>> >> > Create generic alloc and copy functions. We need
>> >> > separate tools for memory allocation and copy to
>> >> > provide multiboot2 protocol support.
>> >> >
>> >> > Signed-off-by: Daniel Kiper 
>> >>
>> >> The amount of casting in this patch alone looks very reasonable now.
>> >> Before ack-ing this and respective subsequent patches I'd like to see
>> >> the final result though. To facilitate that I have to re-raise a 
>> >> previously
>> >> asked question: Do you have a tree somewhere which one could use
>> >> to look at the final result?
>> >
>> > Sadly no.
>>
>> Alternatively, could you simply send the final resulting source file?
> 
> Please look below...

I don't think that was the file at the end of the series, as asked
for above? There's no mb2 code in there afaics...

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v5 06/16] x86/boot/reloc: create generic alloc and copy functions

2016-08-31 Thread Daniel Kiper 
On Tue, Aug 30, 2016 at 09:12:45AM -0600, Jan Beulich wrote:
> >>> On 30.08.16 at 16:32,  wrote:
> > On Thu, Aug 25, 2016 at 05:34:31AM -0600, Jan Beulich wrote:
> >> >>> On 20.08.16 at 00:43,  wrote:
> >> > Create generic alloc and copy functions. We need
> >> > separate tools for memory allocation and copy to
> >> > provide multiboot2 protocol support.
> >> >
> >> > Signed-off-by: Daniel Kiper 
> >>
> >> The amount of casting in this patch alone looks very reasonable now.
> >> Before ack-ing this and respective subsequent patches I'd like to see
> >> the final result though. To facilitate that I have to re-raise a previously
> >> asked question: Do you have a tree somewhere which one could use
> >> to look at the final result?
> >
> > Sadly no.
>
> Alternatively, could you simply send the final resulting source file?

Please look below...

Daniel

/*
 * reloc.c
 *
 * 32-bit flat memory-map routines for relocating Multiboot structures
 * and modules. This is most easily done early with paging disabled.
 *
 * Copyright (c) 2009, Citrix Systems, Inc.
 *
 * Authors:
 *Keir Fraser 
 */

/*
 * This entry point is entered from xen/arch/x86/boot/head.S with:
 *   - 0x4(%esp) = MULTIBOOT_INFORMATION_ADDRESS,
 *   - 0x8(%esp) = BOOT_TRAMPOLINE_ADDRESS.
 */
asm (
".text \n"
".globl _start \n"
"_start:   \n"
"jmp  reloc\n"
);

typedef unsigned int u32;
#include "../../../include/xen/multiboot.h"

#define __stdcall   __attribute__((__stdcall__))

#define ALIGN_UP(arg, align) \
(((arg) + (align) - 1) & ~((typeof(arg))(align) - 1))

static u32 alloc;

static u32 alloc_mem(u32 bytes)
{
return alloc -= ALIGN_UP(bytes, 16);
}

static u32 copy_mem(u32 src, u32 bytes)
{
u32 dst, dst_ret;

dst = alloc_mem(bytes);
dst_ret = dst;

while ( bytes-- )
*(char *)dst++ = *(char *)src++;

return dst_ret;
}

static u32 copy_string(u32 src)
{
u32 p;

if ( src == 0 )
return 0;

for ( p = src; *(char *)p != '\0'; p++ )
continue;

return copy_mem(src, p - src + 1);
}

multiboot_info_t __stdcall *reloc(u32 mbi_old, u32 trampoline)
{
multiboot_info_t *mbi;
int i;

alloc = trampoline;

mbi = (multiboot_info_t *)copy_mem(mbi_old, sizeof(*mbi));

if ( mbi->flags & MBI_CMDLINE )
mbi->cmdline = copy_string(mbi->cmdline);

if ( mbi->flags & MBI_MODULES )
{
module_t *mods;

mbi->mods_addr = copy_mem(mbi->mods_addr, mbi->mods_count * 
sizeof(module_t));

mods = (module_t *)mbi->mods_addr;

for ( i = 0; i < mbi->mods_count; i++ )
{
if ( mods[i].string )
mods[i].string = copy_string(mods[i].string);
}
}

if ( mbi->flags & MBI_MEMMAP )
mbi->mmap_addr = copy_mem(mbi->mmap_addr, mbi->mmap_length);

if ( mbi->flags & MBI_LOADERNAME )
mbi->boot_loader_name = copy_string(mbi->boot_loader_name);

/* Mask features we don't understand or don't relocate. */
mbi->flags &= (MBI_MEMLIMITS |
   MBI_CMDLINE |
   MBI_MODULES |
   MBI_MEMMAP |
   MBI_LOADERNAME);

return mbi;
}

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v5 06/16] x86/boot/reloc: create generic alloc and copy functions

2016-08-30 Thread Jan Beulich 
>>> On 30.08.16 at 16:32,  wrote:
> On Thu, Aug 25, 2016 at 05:34:31AM -0600, Jan Beulich wrote:
>> >>> On 20.08.16 at 00:43,  wrote:
>> > Create generic alloc and copy functions. We need
>> > separate tools for memory allocation and copy to
>> > provide multiboot2 protocol support.
>> >
>> > Signed-off-by: Daniel Kiper 
>>
>> The amount of casting in this patch alone looks very reasonable now.
>> Before ack-ing this and respective subsequent patches I'd like to see
>> the final result though. To facilitate that I have to re-raise a previously
>> asked question: Do you have a tree somewhere which one could use
>> to look at the final result?
> 
> Sadly no.

Alternatively, could you simply send the final resulting source file?

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v5 06/16] x86/boot/reloc: create generic alloc and copy functions

2016-08-30 Thread Daniel Kiper 
On Thu, Aug 25, 2016 at 05:34:31AM -0600, Jan Beulich wrote:
> >>> On 20.08.16 at 00:43,  wrote:
> > Create generic alloc and copy functions. We need
> > separate tools for memory allocation and copy to
> > provide multiboot2 protocol support.
> >
> > Signed-off-by: Daniel Kiper 
>
> The amount of casting in this patch alone looks very reasonable now.
> Before ack-ing this and respective subsequent patches I'd like to see
> the final result though. To facilitate that I have to re-raise a previously
> asked question: Do you have a tree somewhere which one could use
> to look at the final result?

Sadly no.

Daniel

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v5 06/16] x86/boot/reloc: create generic alloc and copy functions

2016-08-25 Thread Jan Beulich 
>>> On 20.08.16 at 00:43,  wrote:
> Create generic alloc and copy functions. We need
> separate tools for memory allocation and copy to
> provide multiboot2 protocol support.
> 
> Signed-off-by: Daniel Kiper 

The amount of casting in this patch alone looks very reasonable now.
Before ack-ing this and respective subsequent patches I'd like to see
the final result though. To facilitate that I have to re-raise a previously
asked question: Do you have a tree somewhere which one could use
to look at the final result?

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH v5 06/16] x86/boot/reloc: create generic alloc and copy functions

2016-08-19 Thread Daniel Kiper 
Create generic alloc and copy functions. We need
separate tools for memory allocation and copy to
provide multiboot2 protocol support.

Signed-off-by: Daniel Kiper 
---
v4 - suggestions/fixes:
   - avoid assembly usage.

v3 - suggestions/fixes:
   - use "g" constraint instead of "r" for alloc_mem() bytes argument
 (suggested by Jan Beulich).

v2 - suggestions/fixes:
   - generalize new functions names
 (suggested by Jan Beulich),
   - reduce number of casts
 (suggested by Jan Beulich).
---
 xen/arch/x86/boot/reloc.c |   51 ++---
 1 file changed, 30 insertions(+), 21 deletions(-)

diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
index 28c6cea..21b1f32 100644
--- a/xen/arch/x86/boot/reloc.c
+++ b/xen/arch/x86/boot/reloc.c
@@ -32,60 +32,69 @@ typedef unsigned int u32;
 
 static u32 alloc;
 
-static void *reloc_mbi_struct(void *old, unsigned int bytes)
+static u32 alloc_mem(u32 bytes)
 {
-void *new;
+return alloc -= ALIGN_UP(bytes, 16);
+}
 
-alloc -= ALIGN_UP(bytes, 16);
-new = (void *)alloc;
+static u32 copy_mem(u32 src, u32 bytes)
+{
+u32 dst, dst_ret;
+
+dst = alloc_mem(bytes);
+dst_ret = dst;
 
 while ( bytes-- )
-*(char *)new++ = *(char *)old++;
+*(char *)dst++ = *(char *)src++;
 
-return (void *)alloc;
+return dst_ret;
 }
 
-static char *reloc_mbi_string(char *old)
+static u32 copy_string(u32 src)
 {
-char *p;
-for ( p = old; *p != '\0'; p++ )
+u32 p;
+
+if ( src == 0 )
+return 0;
+
+for ( p = src; *(char *)p != '\0'; p++ )
 continue;
-return reloc_mbi_struct(old, p - old + 1);
+
+return copy_mem(src, p - src + 1);
 }
 
-multiboot_info_t __stdcall *reloc(multiboot_info_t *mbi_old, u32 trampoline)
+multiboot_info_t __stdcall *reloc(u32 mbi_old, u32 trampoline)
 {
 multiboot_info_t *mbi;
 int i;
 
 alloc = trampoline;
 
-mbi = reloc_mbi_struct(mbi_old, sizeof(*mbi));
+mbi = (multiboot_info_t *)copy_mem(mbi_old, sizeof(*mbi));
 
 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;
 
-mbi->mods_addr = (u32)mods;
+mbi->mods_addr = copy_mem(mbi->mods_addr, mbi->mods_count * 
sizeof(module_t));
+
+mods = (module_t *)mbi->mods_addr;
 
 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_mem(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 |
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel