| Message ID | 1470438282-4226-7-git-send-email-daniel.kiper@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 06.08.16 at 01:04, <daniel.kiper@oracle.com> wrote: > --- 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) Conversion of alloc to be of pointer type (in the earlier patch), and then making the return type here and ... > +static u32 copy_mem(u32 src, u32 bytes) ... all of the types here follow suit would apparently be quite beneficial to the number of casts needed. Jan
>>> On 11.08.16 at 16:12, <JBeulich@suse.com> wrote: >>>> On 06.08.16 at 01:04, <daniel.kiper@oracle.com> wrote: >> --- 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) > > Conversion of alloc to be of pointer type (in the earlier patch), and > then making the return type here and ... > >> +static u32 copy_mem(u32 src, u32 bytes) > > ... all of the types here follow suit would apparently be quite > beneficial to the number of casts needed. Or maybe, considering patch 8, in a slight variation thereof: Do the conversion as suggested, but have a helper wrapper of the type above, taking care of all the casting. That way both the actual implementation and the callers can stay (mostly) cast free. Jan
On Thu, Aug 11, 2016 at 08:17:58AM -0600, Jan Beulich wrote: > >>> On 11.08.16 at 16:12, <JBeulich@suse.com> wrote: > >>>> On 06.08.16 at 01:04, <daniel.kiper@oracle.com> wrote: > >> --- 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) > > > > Conversion of alloc to be of pointer type (in the earlier patch), and > > then making the return type here and ... > > > >> +static u32 copy_mem(u32 src, u32 bytes) > > > > ... all of the types here follow suit would apparently be quite > > beneficial to the number of casts needed. > > Or maybe, considering patch 8, in a slight variation thereof: Do > the conversion as suggested, but have a helper wrapper of the > type above, taking care of all the casting. That way both the > actual implementation and the callers can stay (mostly) cast free. We should take into account patch 9 here too. Looking at code after it I think that right now it is very well optimized in terms of casts. I cannot see room for further improvement. Every change you proposed here and there does not improve final code. It justs move/change casts to/in different places. So, I think that it does not pay change casts here and in earlier patches. At least in the way you proposed until now. Daniel
>>> On 18.08.16 at 10:53, <daniel.kiper@oracle.com> wrote: > On Thu, Aug 11, 2016 at 08:17:58AM -0600, Jan Beulich wrote: >> >>> On 11.08.16 at 16:12, <JBeulich@suse.com> wrote: >> >>>> On 06.08.16 at 01:04, <daniel.kiper@oracle.com> wrote: >> >> --- 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) >> > >> > Conversion of alloc to be of pointer type (in the earlier patch), and >> > then making the return type here and ... >> > >> >> +static u32 copy_mem(u32 src, u32 bytes) >> > >> > ... all of the types here follow suit would apparently be quite >> > beneficial to the number of casts needed. >> >> Or maybe, considering patch 8, in a slight variation thereof: Do >> the conversion as suggested, but have a helper wrapper of the >> type above, taking care of all the casting. That way both the >> actual implementation and the callers can stay (mostly) cast free. > > We should take into account patch 9 here too. Looking at code after > it I think that right now it is very well optimized in terms of casts. > I cannot see room for further improvement. Every change you proposed > here and there does not improve final code. It justs move/change casts > to/in different places. So, I think that it does not pay change casts > here and in earlier patches. At least in the way you proposed until now. What I've suggested above at least makes both the actual function and its wrapper consistent, and hence usable (without casts) by callers dealing with either only numbers of only pointers. Are you saying there are no such "clean" callers? That would put the overall code in a pretty bad light imo. Jan
On Thu, Aug 18, 2016 at 03:41:06AM -0600, Jan Beulich wrote: > >>> On 18.08.16 at 10:53, <daniel.kiper@oracle.com> wrote: > > On Thu, Aug 11, 2016 at 08:17:58AM -0600, Jan Beulich wrote: > >> >>> On 11.08.16 at 16:12, <JBeulich@suse.com> wrote: > >> >>>> On 06.08.16 at 01:04, <daniel.kiper@oracle.com> wrote: > >> >> --- 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) > >> > > >> > Conversion of alloc to be of pointer type (in the earlier patch), and > >> > then making the return type here and ... > >> > > >> >> +static u32 copy_mem(u32 src, u32 bytes) > >> > > >> > ... all of the types here follow suit would apparently be quite > >> > beneficial to the number of casts needed. > >> > >> Or maybe, considering patch 8, in a slight variation thereof: Do > >> the conversion as suggested, but have a helper wrapper of the > >> type above, taking care of all the casting. That way both the > >> actual implementation and the callers can stay (mostly) cast free. > > > > We should take into account patch 9 here too. Looking at code after > > it I think that right now it is very well optimized in terms of casts. > > I cannot see room for further improvement. Every change you proposed > > here and there does not improve final code. It justs move/change casts > > to/in different places. So, I think that it does not pay change casts > > here and in earlier patches. At least in the way you proposed until now. > > What I've suggested above at least makes both the actual function > and its wrapper consistent, and hence usable (without casts) by > callers dealing with either only numbers of only pointers. Are you > saying there are no such "clean" callers? That would put the overall > code in a pretty bad light imo. alloc_mem() is mostly used by callers playing with numbers only. copy_mem() is only one user of it which plays with pointers. However, copy_mem() returns numbers, so, wrapper does not change a lot. It just moves casts to other places. Am I missing something? Daniel
>>> On 18.08.16 at 14:18, <daniel.kiper@oracle.com> wrote: > On Thu, Aug 18, 2016 at 03:41:06AM -0600, Jan Beulich wrote: >> >>> On 18.08.16 at 10:53, <daniel.kiper@oracle.com> wrote: >> > On Thu, Aug 11, 2016 at 08:17:58AM -0600, Jan Beulich wrote: >> >> >>> On 11.08.16 at 16:12, <JBeulich@suse.com> wrote: >> >> >>>> On 06.08.16 at 01:04, <daniel.kiper@oracle.com> wrote: >> >> >> --- 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) >> >> > >> >> > Conversion of alloc to be of pointer type (in the earlier patch), and >> >> > then making the return type here and ... >> >> > >> >> >> +static u32 copy_mem(u32 src, u32 bytes) >> >> > >> >> > ... all of the types here follow suit would apparently be quite >> >> > beneficial to the number of casts needed. >> >> >> >> Or maybe, considering patch 8, in a slight variation thereof: Do >> >> the conversion as suggested, but have a helper wrapper of the >> >> type above, taking care of all the casting. That way both the >> >> actual implementation and the callers can stay (mostly) cast free. >> > >> > We should take into account patch 9 here too. Looking at code after >> > it I think that right now it is very well optimized in terms of casts. >> > I cannot see room for further improvement. Every change you proposed >> > here and there does not improve final code. It justs move/change casts >> > to/in different places. So, I think that it does not pay change casts >> > here and in earlier patches. At least in the way you proposed until now. >> >> What I've suggested above at least makes both the actual function >> and its wrapper consistent, and hence usable (without casts) by >> callers dealing with either only numbers of only pointers. Are you >> saying there are no such "clean" callers? That would put the overall >> code in a pretty bad light imo. > > alloc_mem() is mostly used by callers playing with numbers only. copy_mem() > is only one user of it which plays with pointers. However, copy_mem() > returns > numbers, so, wrapper does not change a lot. It just moves casts to other > places. > Am I missing something? I can't easily tell without seeing a tree with everything applied. Jan
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(-)