[PATCH][ELF] Correct space calculation for symtab when BSD_SYMTAB=yes

[PATCH][ELF] Correct space calculation for symtab when BSD_SYMTAB=yes

[Christoph Egger]

[-- Attachment #1: Type: text/plain, Size: 892 bytes --]


Hi!

If there is a string table for section headers, it also gets loaded.
Therefore take it into account in size calculation for kernel symtab.

Also there is no need to call elf_set_verbose() a second time
after elf_init() (First call happens within elf_init()).

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>

Keir: Can you also apply changeset 15672 and this patch
to Xen 3.1-stable, since these fixes an regression for Xen 3.1, please?


-- 
AMD Saxony, Dresden, Germany
Operating System Research Center

Legal Information:
AMD Saxony Limited Liability Company & Co. KG
Sitz (Geschäftsanschrift):
   Wilschdorfer Landstr. 101, 01109 Dresden, Deutschland
Registergericht Dresden: HRA 4896
vertretungsberechtigter Komplementär:
   AMD Saxony LLC (Sitz Wilmington, Delaware, USA)
Geschäftsführer der AMD Saxony LLC:
   Dr. Hans-R. Deppe, Thomas McCoy

[-- Attachment #2: xen_symtab.diff --]
[-- Type: text/x-diff, Size: 1106 bytes --]

diff -r 88bb0d305308 xen/arch/x86/domain_build.c
--- a/xen/arch/x86/domain_build.c	Wed Aug 01 15:47:54 2007 +0100
+++ b/xen/arch/x86/domain_build.c	Thu Aug 02 09:31:58 2007 +0200
@@ -268,9 +268,6 @@ int __init construct_dom0(
 
     if ( (rc = elf_init(&elf, image_start, image_len)) != 0 )
         return rc;
-#ifdef VERBOSE
-    elf_set_verbose(&elf);
-#endif
     elf_parse_binary(&elf);
     if ( (rc = elf_xen_parse(&elf, &parms)) != 0 )
         return rc;
diff -r 88bb0d305308 xen/common/libelf/libelf-loader.c
--- a/xen/common/libelf/libelf-loader.c	Wed Aug 01 15:47:54 2007 +0100
+++ b/xen/common/libelf/libelf-loader.c	Thu Aug 02 09:30:52 2007 +0200
@@ -53,8 +53,10 @@ int elf_init(struct elf_binary *elf, con
     /* Find section string table. */
     section = elf_uval(elf, elf->ehdr, e_shstrndx);
     shdr = elf_shdr_by_index(elf, section);
-    if ( shdr != NULL )
+    if ( shdr != NULL ) {
         elf->sec_strtab = elf_section_start(elf, shdr);
+        high = low = (unsigned long)shdr;
+    }
 
     /* Find symbol table and symbol string table. */
     count = elf_shdr_count(elf);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH][ELF] Correct space calculation for symtab when BSD_SYMTAB=yes

[Keir Fraser]

On 2/8/07 09:03, "Christoph Egger" <Christoph.Egger@amd.com> wrote:

> If there is a string table for section headers, it also gets loaded.
> Therefore take it into account in size calculation for kernel symtab.

I don't see how this works. In fact the whole sstart,send calculation looks
broken. What do the virtual addresses of the symtab/strtab sections have to
do with their location in the final address space, after loading? And since
you pack the sections into the domain address space in
elf_xen_dom_load_binary(), should you not simply sum the sizes of the
sections, then sstart=virt_kend and send=sstart+size_sum, rather than taking
max(end addresses in elf image) minus min(start addresses in elf image).

Does the section-header string table have to be loaded, or is that just an
artefact of the loader code scanning for all SYMTAB/STRTAB? Shouldn't the
loader fix up e_shnum and symtab's sh_link in the Elf metadata that it
generates? Couldn't the loader just use the saved elf->sym_tab and
elf->sym_strtab to directly find the interesting two sections, rather than
needing to do yet another full scan?

> Keir: Can you also apply changeset 15672 and this patch
> to Xen 3.1-stable, since these fixes an regression for Xen 3.1, please?

I don't think so!

 -- Keir

Re: [PATCH][ELF] Correct space calculation for symtab when BSD_SYMTAB=yes

[Christoph Egger]

[-- Attachment #1: Type: text/plain, Size: 4810 bytes --]

On Thursday 02 August 2007 10:49:57 Keir Fraser wrote:
> On 2/8/07 09:03, "Christoph Egger" <Christoph.Egger@amd.com> wrote:
> > If there is a string table for section headers, it also gets loaded.
> > Therefore take it into account in size calculation for kernel symtab.
>
> I don't see how this works.

Basically, I do it the same way as Xen 3.0.4 did: First calculate the space
you need, then fill the space with ELF header, ELF section headers and
the symbol and string sections itself.

> In fact the whole sstart,send calculation looks broken.

Well, I did it analogous to elf_parse_binary() how it calculates the space
for phdr.

> What do the virtual addresses of the symtab/strtab sections have to 
> do with their location in the final address space, after loading? And since
> you pack the sections into the domain address space in
> elf_xen_dom_load_binary(), should you not simply sum the sizes of the
> sections, then sstart=virt_kend and send=sstart+size_sum, rather than
> taking max(end addresses in elf image) minus min(start addresses in elf
> image).

You're right. Attached is a patch, which does it that way.

(Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>)

Oh and c/s 15677 in the staging tree removes the wrong elf_set_verbose() call.
Without it, you will never see the elf_msg() in elf_init().
The correct duplicate to remove is the one right after the use of elf_init(). 
My patch removes it.

> Does the section-header string table have to be loaded, or is that just an
> artefact of the loader code scanning for all SYMTAB/STRTAB? Shouldn't the
> loader fix up e_shnum and symtab's sh_link in the Elf metadata that it
> generates?

I think, Xen should just pass to Dom0 what it has and let Dom0 decide what
it wants to use.

> Couldn't the loader just use the saved elf->sym_tab and 
> elf->sym_strtab to directly find the interesting two sections, rather than
> needing to do yet another full scan?

It needs to go through it, because it must adjust the sh_offset accordingly
in the section headers.

> > Keir: Can you also apply changeset 15672 and this patch
> > to Xen 3.1-stable, since these fixes an regression for Xen 3.1, please?
>
> I don't think so!

I *do* think so :-) Look (I marked the most interesting lines with an arrow):


With Xen 3.0.4 I get this:

[...]
(XEN) Xen is relinquishing VGA console.
(XEN) *** Serial input -> DOM0 (type 'CTRL-a' three times to switch input to 
Xen).
(XEN) Freed 92kB init memory.
Loaded initial symtab at 0xc09d7ad8, strtab at 0xc0a38cb4, # entries 23398  <=
Copyright (c) 1996, 1997, 1998, 1999, 2000, 2001, 2002, 2003, 2004, 2005,
    2006, 2007
    The NetBSD Foundation, Inc.  All rights reserved.
Copyright (c) 1982, 1986, 1989, 1991, 1993
    The Regents of the University of California.  All rights reserved.

NetBSD 4.99.26 (XEN3_DOM0) #70: Wed Jul 25 15:54:08 UTC 2007
        root@tulln:/usr/src/sys/arch/i386/compile/XEN3_DOM0
[...]

With Xen 3.1 I get this:

[...]
(XEN) Xen is relinquishing VGA console.
(XEN) *** Serial input -> DOM0 (type 'CTRL-a' three times to switch input to 
Xen).
(XEN) Freed 92kB init memory.
[ Kernel symbol table missing! ]                                       <=
Copyright (c) 1996, 1997, 1998, 1999, 2000, 2001, 2002, 2003, 2004, 2005,
    2006, 2007
    The NetBSD Foundation, Inc.  All rights reserved.
Copyright (c) 1982, 1986, 1989, 1991, 1993
    The Regents of the University of California.  All rights reserved.

NetBSD 4.99.26 (XEN3_DOM0) #74: Wed Aug  1 10:57:21 UTC 2007
        root@tulln:/usr/src/sys/arch/i386/compile/XEN3_DOM0
[...]


and in -unstable (with my patches) I get this:

[...]
(XEN) Xen is relinquishing VGA console.
(XEN) *** Serial input -> DOM0 (type 'CTRL-a' three times to switch input to 
Xen).
(XEN) Freed 108kB init memory.
Loaded initial symtab at 0xc09decd8, strtab at 0xc0a4001c, # entries 23398  <=
Copyright (c) 1996, 1997, 1998, 1999, 2000, 2001, 2002, 2003, 2004, 2005,
    2006, 2007
    The NetBSD Foundation, Inc.  All rights reserved.
Copyright (c) 1982, 1986, 1989, 1991, 1993
    The Regents of the University of California.  All rights reserved.

NetBSD 4.99.26 (XEN3_DOM0) #74: Wed Aug  1 10:57:21 UTC 2007
        root@tulln:/usr/src/sys/arch/i386/compile/XEN3_DOM0
[...]




Christoph


-- 
AMD Saxony, Dresden, Germany
Operating System Research Center

Legal Information:
AMD Saxony Limited Liability Company & Co. KG
Sitz (Geschäftsanschrift):
   Wilschdorfer Landstr. 101, 01109 Dresden, Deutschland
Registergericht Dresden: HRA 4896
vertretungsberechtigter Komplementär:
   AMD Saxony LLC (Sitz Wilmington, Delaware, USA)
Geschäftsführer der AMD Saxony LLC:
   Dr. Hans-R. Deppe, Thomas McCoy

[-- Attachment #2: xen_symtab.diff --]
[-- Type: text/x-diff, Size: 4526 bytes --]

diff -r 88bb0d305308 xen/arch/x86/domain_build.c
--- a/xen/arch/x86/domain_build.c	Wed Aug 01 15:47:54 2007 +0100
+++ b/xen/arch/x86/domain_build.c	Thu Aug 02 13:47:54 2007 +0200
@@ -268,9 +268,6 @@ int __init construct_dom0(
 
     if ( (rc = elf_init(&elf, image_start, image_len)) != 0 )
         return rc;
-#ifdef VERBOSE
-    elf_set_verbose(&elf);
-#endif
     elf_parse_binary(&elf);
     if ( (rc = elf_xen_parse(&elf, &parms)) != 0 )
         return rc;
@@ -318,7 +315,7 @@ int __init construct_dom0(
            elf.pstart, elf.pend);
     if ( parms.bsd_symtab )
         printk(" Dom0 symbol map 0x%" PRIx64 " -> 0x%" PRIx64 "\n",
-               elf.sstart, elf.send);
+               parms.virt_kend, parms.virt_end);
 
     if ( !compatible )
     {
diff -r 88bb0d305308 xen/common/libelf/libelf-dominfo.c
--- a/xen/common/libelf/libelf-dominfo.c	Wed Aug 01 15:47:54 2007 +0100
+++ b/xen/common/libelf/libelf-dominfo.c	Thu Aug 02 13:46:24 2007 +0200
@@ -337,7 +337,7 @@ static void elf_xen_loadsymtab(struct el
 static void elf_xen_loadsymtab(struct elf_binary *elf,
                                struct elf_dom_parms *parms)
 {
-    unsigned long maxva, len;
+    unsigned long maxva;
 
     if ( !parms->bsd_symtab )
         return;
@@ -353,8 +353,7 @@ static void elf_xen_loadsymtab(struct el
     maxva = elf_round_up(elf, maxva);
 
     /* Space for the symbol and string tabs */
-    len = (unsigned long)elf->send - (unsigned long)elf->sstart;
-    maxva = elf_round_up(elf, maxva + len);
+    maxva = elf_round_up(elf, maxva + elf->symsize);
 
     /* The address the kernel must expanded to */
     parms->virt_end = maxva;
diff -r 88bb0d305308 xen/common/libelf/libelf-loader.c
--- a/xen/common/libelf/libelf-loader.c	Wed Aug 01 15:47:54 2007 +0100
+++ b/xen/common/libelf/libelf-loader.c	Thu Aug 02 13:46:05 2007 +0200
@@ -10,8 +10,7 @@ int elf_init(struct elf_binary *elf, con
 {
     const elf_shdr *shdr;
     uint64_t i, count, section, offset;
-    uint64_t low = -1;
-    uint64_t high = 0;
+    uint64_t symsize = 0;
 
     if ( !elf_is_elfbinary(image) )
     {
@@ -53,8 +52,12 @@ int elf_init(struct elf_binary *elf, con
     /* Find section string table. */
     section = elf_uval(elf, elf->ehdr, e_shstrndx);
     shdr = elf_shdr_by_index(elf, section);
-    if ( shdr != NULL )
+    if ( shdr != NULL ) {
         elf->sec_strtab = elf_section_start(elf, shdr);
+        /* Size of section string table */
+        symsize = ((unsigned long)elf_section_end(elf, shdr)
+                   - (unsigned long)elf_section_start(elf, shdr));
+    }
 
     /* Find symbol table and symbol string table. */
     count = elf_shdr_count(elf);
@@ -77,15 +80,10 @@ int elf_init(struct elf_binary *elf, con
         elf->sym_strtab = elf_section_start(elf, shdr);
         sh_strend = (const char *)elf_section_end(elf, shdr);
 
-        if ( low > (unsigned long)elf->sym_tab )
-            low = (unsigned long)elf->sym_tab;
-        if ( low > (unsigned long)shdr )
-            low = (unsigned long)shdr;
-
-        if ( high < ((unsigned long)sh_symend) )
-            high = (unsigned long)sh_symend;
-        if ( high < ((unsigned long)sh_strend) )
-            high = (unsigned long)sh_strend;
+        /* Symbol and string table sizes */
+        symsize += ((unsigned long)sh_strend - (unsigned long)elf->sym_strtab);
+        symsize += ((unsigned long)sh_symend
+                    - (unsigned long)elf_section_start(elf, elf->sym_tab));
 
         elf_msg(elf, "%s: shdr: sym_tab=%p size=0x%" PRIx64 "\n",
                 __FUNCTION__, elf->sym_tab,
@@ -93,10 +91,11 @@ int elf_init(struct elf_binary *elf, con
         elf_msg(elf, "%s: shdr: str_tab=%p size=0x%" PRIx64 "\n",
                 __FUNCTION__, elf->sym_strtab, elf_uval(elf, shdr, sh_size));
 
-        elf->sstart = low;
-        elf->send = high;
+        elf->symsize = symsize;
         elf_msg(elf, "%s: symbol map: 0x%" PRIx64 " -> 0x%" PRIx64 "\n",
-                __FUNCTION__, elf->sstart, elf->send);
+                __FUNCTION__, elf->pend, elf->pend + elf->symsize);
+
+        break;
     }
 
     return 0;
diff -r 88bb0d305308 xen/include/public/libelf.h
--- a/xen/include/public/libelf.h	Wed Aug 01 15:47:54 2007 +0100
+++ b/xen/include/public/libelf.h	Thu Aug 02 13:27:11 2007 +0200
@@ -65,8 +65,7 @@ struct elf_binary {
 
     /* loaded to */
     char *dest;
-    uint64_t sstart;
-    uint64_t send;
+    uint64_t symsize;
     uint64_t pstart;
     uint64_t pend;
     uint64_t reloc_offset;

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH][ELF] Correct space calculation for symtab when BSD_SYMTAB=yes

[Keir Fraser]

On 2/8/07 13:26, "Christoph Egger" <Christoph.Egger@amd.com> wrote:

>> What do the virtual addresses of the symtab/strtab sections have to
>> do with their location in the final address space, after loading? And since
>> you pack the sections into the domain address space in
>> elf_xen_dom_load_binary(), should you not simply sum the sizes of the
>> sections, then sstart=virt_kend and send=sstart+size_sum, rather than
>> taking max(end addresses in elf image) minus min(start addresses in elf
>> image).
> 
> You're right. Attached is a patch, which does it that way.

Please try changeset 15679 in the -unstable staging tree. It's less code and
also fixed up to work with the domU domain builder too. Except it's not
actually been tested with a bsd_symtab guest kernel. It shouldn't need more
than a few lines of fixing to get it working though, if that.

 -- Keir

Re: [PATCH][ELF] Correct space calculation for symtab when BSD_SYMTAB=yes

[Christoph Egger]

On Thursday 02 August 2007 15:21:34 Keir Fraser wrote:
> On 2/8/07 13:26, "Christoph Egger" <Christoph.Egger@amd.com> wrote:
> >> What do the virtual addresses of the symtab/strtab sections have to
> >> do with their location in the final address space, after loading? And
> >> since you pack the sections into the domain address space in
> >> elf_xen_dom_load_binary(), should you not simply sum the sizes of the
> >> sections, then sstart=virt_kend and send=sstart+size_sum, rather than
> >> taking max(end addresses in elf image) minus min(start addresses in elf
> >> image).
> >
> > You're right. Attached is a patch, which does it that way.
>
> Please try changeset 15679 in the -unstable staging tree. It's less code
> and also fixed up to work with the domU domain builder too. Except it's not
> actually been tested with a bsd_symtab guest kernel. It shouldn't need more
> than a few lines of fixing to get it working though, if that.
>
>  -- Keir

I tested this both with a Dom0 and DomU guest domain. It works!
Thanks a lot.


Christoph



-- 
AMD Saxony, Dresden, Germany
Operating System Research Center

Legal Information:
AMD Saxony Limited Liability Company & Co. KG
Sitz (Geschäftsanschrift):
   Wilschdorfer Landstr. 101, 01109 Dresden, Deutschland
Registergericht Dresden: HRA 4896
vertretungsberechtigter Komplementär:
   AMD Saxony LLC (Sitz Wilmington, Delaware, USA)
Geschäftsführer der AMD Saxony LLC:
   Dr. Hans-R. Deppe, Thomas McCoy