Re: [LinuxBIOS] r2758 - in trunk: LinuxBIOSv2/util util

[Carl-Daniel Hailfinger]

On 05.09.2007 01:52, Stefan Reinauer wrote:
> Carl-Daniel Hailfinger  [070903 20:36]:
> > This commit removed probe_superio from v2. It has not been referenced as
> > svn:externals, so none of the v2 and v3 trees have probe_superio or
> > superiotool.
> > As of revision 2762, the v2 repository is still BROKEN and superiotool
> > doesn't appear in v3 either (checked revision 486).
> >
> > Such commits are one reason why I think self-acked commits should not be
> > allowed.
>
> Well. Half-hearted code reviews are an illusion of safety, too.

Yes.
> The current code's lar handling is pretty much broken even though that
> code was reviewed, but obviously nobody even tested it:

It seems I tested without a payload. Apologies.
> Wrote LinuxBIOS table at: 0x00000500 - 0x00000a54  checksum 401f
> Show all devs...
> cpus: Unknown device path type: 0
> Stage2 code done.
> LAR: Attempting to open 'normal/payload'.
> LAR: Attempting to open 'normal/payload/segment0'.
> LAR: load_file: No such file 'normal/payload/segment0'
>
> It just hangs somewhere bogus and never gets to the defined error in
>
> arch/x86/stage1.c:194: die("FATAL: No usable payload found.\n");
>
> also the current code contains quite some warnings again regarding
> int-to-pointer casts in lar.c and pointer-to-int cast and uninitialized
> variable use in stage1.c

Which gcc? I seem to have trouble getting my gcc to warn about anything
on i386. "gcc (GCC) 4.1.0 (SUSE Linux)". On my other machine, I get a
few warnings:
> /sources/LinuxBIOSv3/lib/lar.c: In function ‘find_file’:
> /sources/LinuxBIOSv3/lib/lar.c:110: warning: cast to pointer from integer of different size
> /sources/LinuxBIOSv3/lib/lar.c:111: warning: cast to pointer from integer of different size

resulting from this code:
> result->entry = (void *)ntohl(header->entry);
> result->loadaddress = (void *)ntohl(header->loadaddress);

Because we switched these lar header entries to u64 and struct mem_file
uses void * for the related entries, we are stuck. There are two ways
out of this problem:

struct mem_file {
        void *start;
        int len;
        u32 reallen;
        u32 compression;
        u64 entry;
        u64 loadaddress;
};

But that means we can't use ->entry and ->loadaddress as pointers
anymore. Sucks.

Or explicitly check if any of the upper 32 bits are set and if not,
convert to void *.
That "solution" breaks as soon as we want to make use of 64 bits. Sucks
even more.

So I fear we have to go for the first route and work with PAE once we
have an entry above 32 bits.
> /sources/LinuxBIOSv3/arch/x86/stage1.c: In function ‘legacy’:
> /sources/LinuxBIOSv3/arch/x86/stage1.c:66: warning: ‘result.reallen’ is used uninitialized in this function

resulting from this code:
> ret =  elfboot_mem(mem, where, result.reallen);

Not a problem right now because elfboot_mem doesn't use its third
parameter. But Ron might want to fix this in a future commit.
> /sources/LinuxBIOSv3/arch/x86/stage1.c: In function ‘stage1_main’:
> /sources/LinuxBIOSv3/arch/x86/stage1.c:185: warning: passing argument 1 of ‘printk’ makes integer from pointer without a cast

printk prefix was forgotten. Alex Beregszaszi has already sent a patch
for that: "[PATCH] stage1 multi-segment lar bug"

Carl-Daniel