| Message ID | 1471646606-28519-7-git-send-email-daniel.kiper@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 20.08.16 at 00:43, <daniel.kiper@oracle.com> 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 <daniel.kiper@oracle.com> 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
On Thu, Aug 25, 2016 at 05:34:31AM -0600, Jan Beulich wrote: > >>> On 20.08.16 at 00:43, <daniel.kiper@oracle.com> 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 <daniel.kiper@oracle.com> > > 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
>>> On 30.08.16 at 16:32, <daniel.kiper@oracle.com> wrote: > On Thu, Aug 25, 2016 at 05:34:31AM -0600, Jan Beulich wrote: >> >>> On 20.08.16 at 00:43, <daniel.kiper@oracle.com> 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 <daniel.kiper@oracle.com> >> >> 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
On Tue, Aug 30, 2016 at 09:12:45AM -0600, Jan Beulich wrote: > >>> On 30.08.16 at 16:32, <daniel.kiper@oracle.com> wrote: > > On Thu, Aug 25, 2016 at 05:34:31AM -0600, Jan Beulich wrote: > >> >>> On 20.08.16 at 00:43, <daniel.kiper@oracle.com> 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 <daniel.kiper@oracle.com> > >> > >> 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 <keir@xen.org> */ /* * 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; }
>>> On 31.08.16 at 17:13, <daniel.kiper@oracle.com> wrote: > On Tue, Aug 30, 2016 at 09:12:45AM -0600, Jan Beulich wrote: >> >>> On 30.08.16 at 16:32, <daniel.kiper@oracle.com> wrote: >> > On Thu, Aug 25, 2016 at 05:34:31AM -0600, Jan Beulich wrote: >> >> >>> On 20.08.16 at 00:43, <daniel.kiper@oracle.com> 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 <daniel.kiper@oracle.com> >> >> >> >> 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
On Wed, Aug 31, 2016 at 09:25:57AM -0600, Jan Beulich wrote: > >>> On 31.08.16 at 17:13, <daniel.kiper@oracle.com> wrote: > > On Tue, Aug 30, 2016 at 09:12:45AM -0600, Jan Beulich wrote: > >> >>> On 30.08.16 at 16:32, <daniel.kiper@oracle.com> wrote: > >> > On Thu, Aug 25, 2016 at 05:34:31AM -0600, Jan Beulich wrote: > >> >> >>> On 20.08.16 at 00:43, <daniel.kiper@oracle.com> 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 <daniel.kiper@oracle.com> > >> >> > >> >> 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 <keir@xen.org> * Daniel Kiper <daniel.kiper@oracle.com> */ /* * 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 modules. */ 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) ) if ( tag->type == MULTIBOOT2_TAG_TYPE_MODULE ) ++mbi_out->mods_count; else if ( tag->type == MULTIBOOT2_TAG_TYPE_END ) break; if ( mbi_out->mods_count ) { mbi_out->flags = MBI_MODULES; mbi_out->mods_addr = alloc_mem(mbi_out->mods_count * sizeof(module_t)); mbi_out_mods = (module_t *)mbi_out->mods_addr; } /* Skip Multiboot2 information fixed part. */ ptr = ALIGN_UP(mbi_in + sizeof(multiboot2_fixed_t), MULTIBOOT2_TAG_ALIGN); /* Put all needed data into mbi_out. */ 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) ) switch ( tag->type ) { case MULTIBOOT2_TAG_TYPE_BOOT_LOADER_NAME: mbi_out->flags |= MBI_LOADERNAME; ptr = get_mb2_string(tag, string, string); mbi_out->boot_loader_name = copy_string(ptr); break; case MULTIBOOT2_TAG_TYPE_CMDLINE: mbi_out->flags |= MBI_CMDLINE; ptr = get_mb2_string(tag, string, string); mbi_out->cmdline = copy_string(ptr); break; case MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO: mbi_out->flags |= MBI_MEMLIMITS; mbi_out->mem_lower = get_mb2_data(tag, basic_meminfo, mem_lower); mbi_out->mem_upper = get_mb2_data(tag, basic_meminfo, mem_upper); break; case MULTIBOOT2_TAG_TYPE_MMAP: if ( get_mb2_data(tag, mmap, entry_size) < sizeof(*mmap_src) ) break; mbi_out->flags |= MBI_MEMMAP; mbi_out->mmap_length = get_mb2_data(tag, mmap, size); mbi_out->mmap_length -= sizeof(multiboot2_tag_mmap_t); mbi_out->mmap_length /= get_mb2_data(tag, mmap, entry_size); mbi_out->mmap_length *= sizeof(memory_map_t); mbi_out->mmap_addr = alloc_mem(mbi_out->mmap_length); mmap_src = get_mb2_data(tag, mmap, entries); mmap_dst = (memory_map_t *)mbi_out->mmap_addr; for ( i = 0; i < mbi_out->mmap_length / sizeof(memory_map_t); i++ ) { /* Init size member properly. */ mmap_dst[i].size = sizeof(memory_map_t); mmap_dst[i].size -= sizeof(((memory_map_t){0}).size); /* Now copy a given region data. */ mmap_src = (void *)mmap_src + i * get_mb2_data(tag, mmap, entry_size); mmap_dst[i].base_addr_low = (u32)mmap_src->addr; mmap_dst[i].base_addr_high = (u32)(mmap_src->addr >> 32); mmap_dst[i].length_low = (u32)mmap_src->len; mmap_dst[i].length_high = (u32)(mmap_src->len >> 32); mmap_dst[i].type = mmap_src->type; } break; case MULTIBOOT2_TAG_TYPE_MODULE: mbi_out_mods[mod_idx].mod_start = get_mb2_data(tag, module, mod_start); mbi_out_mods[mod_idx].mod_end = get_mb2_data(tag, module, mod_end); ptr = get_mb2_string(tag, module, cmdline); mbi_out_mods[mod_idx].string = copy_string(ptr); mbi_out_mods[mod_idx].reserved = 0; ++mod_idx; break; case MULTIBOOT2_TAG_TYPE_END: return mbi_out; default: break; } return mbi_out; } multiboot_info_t __stdcall *reloc(u32 mb_magic, u32 mbi_in, u32 trampoline) { alloc = trampoline; if ( mb_magic == MULTIBOOT2_BOOTLOADER_MAGIC ) return mbi2_mbi(mbi_in); else return mbi_mbi(mbi_in); }
>>> On 31.08.16 at 21:39, <daniel.kiper@oracle.com> 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
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, <daniel.kiper@oracle.com> 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 <daniel.kiper@oracle.com> >> >> 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.
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 |
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 <daniel.kiper@oracle.com> --- 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(-)