Re: More hpcboot memory/ELF loading errors?

[Rafal Boni <rafal%pobox.com@localhost>]

Valeriy E. Ushakov wrote:
> On Wed, Mar 05, 2008 at 21:33:19 -0500, Rafal Boni wrote:
>
> > The issue appears to be that while the symbol header, symbol table and 
> > string table are all loaded in 3 separate passes, the size-estimation 
> > code returned the size of all 3 items as a single datum, ignoring the 
> > fact that each of those 3 components would need at least one page (and 
> > maybe more) to store the contents in the 'load chain'.
> >
> > Diff attached... comments welcome,
> > --rafal
>
> > Index: hpcboot/load_elf.cpp
> > ===================================================================
> > RCS file: /cvsroot/src/sys/arch/hpc/stand/hpcboot/load_elf.cpp,v
> > retrieving revision 1.17
> > diff -u -p -u -p -r1.17 load_elf.cpp
> > --- hpcboot/load_elf.cpp        5 Mar 2006 04:04:13 -0000       1.17
> > +++ hpcboot/load_elf.cpp        6 Mar 2008 02:01:36 -0000
> > @@ -123,10 +123,6 @@ ElfLoader::memorySize()
> >                         sz += _mem->roundPage(filesz);
> >                         // compensate for partial last tag
> >                         extra += _mem->getTaggedPageSize();
> > -                       if (filesz < ph->p_memsz)
> > -                               // compensate for zero clear
> > -                               extra += _mem->getTaggedPageSize();
> > -
> >                 }
> >         }
>
> Why have you removed this?  Zero-clear chunk occupies an extra tagged
> page in the chain.

I thought this was the case too,  but if you follow the code / chain, 
you'll note that they steal from the initial 'pvec' page allocated at 
the beginning (they just kidnap sizeof(PageTag) since there's no data to 
store).  So I adjusted for reality.

> > @@ -135,8 +131,9 @@ ElfLoader::memorySize()
> >         if (symblk_sz) {
> >                 sz += symblk_sz;
> >                 DPRINTF((TEXT(" = 0x%x]"), symblk_sz));
> > -               // XXX: compensate for partial tags after ELF header and symtab
> > -               extra += 2 * _mem->getTaggedPageSize();
> > +               // XXX: compensate for partial tags after ELF header, symtab
> > +               // and strtab
> > +               extra += 3 * _mem->getTaggedPageSize();
> >         }
>
> Does it work for you with this hunk alone?

I haven't tried just that one -- though even if it works, it seems a 
little like a rubber-chickens and voodoo-sticks change ;).  But I'll 
give it a try and report back in any case..

> >         sz += extra;
> > @@ -263,13 +260,17 @@ ElfLoader::symbol_block_size()
> >             ROUND4(_sym_blk.shsym->sh_size);
> >         _sym_blk.enable = TRUE;
> >  
> > -	DPRINTF((TEXT("+[ksyms: header 0x%x, symtab 0x%x, strtab 0x%x"),
> > +       DPRINTF((TEXT("+[ksyms: header 0x%x, symtab 0x%x, strtab 0x%x = 0x%x]"),
> >             _sym_blk.header_size, _sym_blk.shsym->sh_size,
> > -           _sym_blk.shstr->sh_size));
> > +	    _sym_blk.shstr->sh_size, _sym_blk.header_size + 
> > +	    ROUND4(_sym_blk.shsym->sh_size) + _sym_blk.shstr->sh_size));
>
> The sum is printed in the caller.  If you print it here, you need to
> remove the printf in the caller.

Oops, I thought I had done that; will update, thanks.

> > -       // return total amount of symbol block
> > -       return (_sym_blk.header_size + ROUND4(_sym_blk.shsym->sh_size) +
> > -           _sym_blk.shstr->sh_size);
> > +       // Round each of the three components to page_size since they're
> > +       // loaded as 3 different segments.
> > +       return (_mem->roundPage(_sym_blk.header_size) +
> > +               _mem->roundPage(ROUND4(_sym_blk.shsym->sh_size)) +
> > +               _mem->roundPage(_sym_blk.shstr->sh_size));
> > +	    
> >  }
>
> Is these roundPage necessary?  I really don't want to inflict on
> myself the pain of reading and understanding this code again, but IIRC
> extra pages added in the caller are supposed to take care of this.
> Moreover, IIRC, doing roundPage doesn't actually help the "partial
> tag" problem.

I think this is the crux of the problem, and yeah, I think it's 
necessary.  If you look at what ElfLoader::load_symbol_block() does, it 
loads (a) header, (b) symbol table, and (c) string table as individual 
segments, which means each will start on a new page in the tag chain.

So if you have e.g. 1320 + 1290 + 1350 bytes in those 3, before we'd 
return 1320 + ROUND4(1290) = 1292 + 1350 = 3962, which would look like 
it would fit in a single page (even in a single tagged page), but would 
actually require *3* tagged pages since each component would get it's 
own link (== tagged page) in the tag chain.

AAs for the 'partial tag' issue, I'm also not sure off the top of my 
head, but given the gyrations Architecture::allocateMemory goes through 
to convert bytes-needed-in-memory to tagged-pages-needed, I'd think as 
long as we rounded up all segments to the next unit of tagged-page-size 
(since after all that's what matters, not actual HW page size), there 
should be *no* page unaccounted for.  Rounding up to HW page size is 
maybe a little sloppy, but should give more *conservative* estimates, 
which from our POV is OK (after all, it's going to get rounded up to the 
next chunk of 64k anyway when we allocate pages from WinCE).

> I'd prefer we really understand were the overflow occurs before we
> commit anything to this code that is already quite hairy.

I'm all for that, which is why I'm sending this to the list rather than 
checking in... but I also want to minimize the amount of damage my 
forehead takes from beating it against this too much, and in the end I 
want something which works at least semi-reliably -- and this hasn't 
been the case with the hpcboot built from unmodified sources.

Thanks,
--rafal