Re: More hpcboot memory/ELF loading errors?

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