seemingly wrong code in src/arch/i386/boot/coreboot_table.c

[ron minnich]

I don't understand this code.

Here it is.

#if (HAVE_OPTION_TABLE == 1)
        {
                struct lb_record *rec_dest, *rec_src;
                /* Write the option config table... */
                rec_dest = lb_new_record(head);
                rec_src = (struct lb_record *)(void *)&option_table;
                memcpy(rec_dest,  rec_src, rec_src->size);
                /* Create cmos checksum entry in coreboot table */
                lb_cmos_checksum(head);
        }
#endif

Note the cast of rec_src.

But option_table is this:
unsigned char option_table[] = {
    0xc8,0x00,0x00,0x00,0x88,0x04,0x00,0x00,0x0c,0x00,

Why is this cast being done? This code is different in older versions
of this file.

Thanks

ron

[Myles Watson]

On Fri, May 8, 2009 at 9:06 PM, ron minnich  wrote:
> I don't understand this code.
>
> Here it is.
>
> #if (HAVE_OPTION_TABLE == 1)
>        {
>                struct lb_record *rec_dest, *rec_src;
>                /* Write the option config table... */
>                rec_dest = lb_new_record(head);
>                rec_src = (struct lb_record *)(void *)&option_table;
>                memcpy(rec_dest,  rec_src, rec_src->size);
>                /* Create cmos checksum entry in coreboot table */
>                lb_cmos_checksum(head);
>        }
> #endif
>
> Note the cast of rec_src.

I guess I'm clueless what that actually does.  It looks like an
unsigned char* getting cast to a void* then to a lb_record*.

I wouldn't expect that to change the value.
> But option_table is this:
> unsigned char option_table[] = {
>        0xc8,0x00,0x00,0x00,0x88,0x04,0x00,0x00,0x0c,0x00,
>
> Why is this cast being done?

No idea.
> This code is different in older versions
> of this file.

It looks like it's been done that way since rev 1786.  I don't have a
good idea when that was, but it was a while ago.

Myles

[ron minnich]

On Fri, May 8, 2009 at 8:35 PM, Myles Watson  wrote:
> On Fri, May 8, 2009 at 9:06 PM, ron minnich  wrote:
> > I don't understand this code.
> >
> > Here it is.
> >
> > #if (HAVE_OPTION_TABLE == 1)
> >        {
> >                struct lb_record *rec_dest, *rec_src;
> >                /* Write the option config table... */
> >                rec_dest = lb_new_record(head);
> >                rec_src = (struct lb_record *)(void *)&option_table;
> >                memcpy(rec_dest,  rec_src, rec_src->size);
> >                /* Create cmos checksum entry in coreboot table */
> >                lb_cmos_checksum(head);
> >        }
> > #endif
> >
> > Note the cast of rec_src.
>
> I guess I'm clueless what that actually does.  It looks like an
> unsigned char* getting cast to a void* then to a lb_record*.
>
> I wouldn't expect that to change the value.

This is an lb_record
struct lb_record {
        uint32_t tag;           /* tag ID */
        uint32_t size;          /* size of record (in bytes) */
};

option_table is an array of bytes, not a struct. I can see if there is
a filled-in option table this might work, but not in this case:
unsigned char option_table[] = {
        };

Which turns into a four byte quantity, which in terms of the lb_record
is a four byte tag of zero and then a length of ... whatever comes
after option_table!
This code needs some fixing. I have a simple hack but I'd rather do
something better than what we have now. It will need repair to the
build_opt_tbl tool.

ron

[ron minnich]

Found the problem. 4240 broke builtd_option_tbl due to the addition of
error checking which got the sense of fwrite wrong.
fwrite returns 0 on error, non-zero otherwise; the code was written to
assume non-zero was an error.

patch attached inline, qemu is booting again.
If somebody wants to ack this and commit it that is fine with me.

Signed-off-by: Ronald G. Minnich

Index: util/options/build_opt_tbl.c
===================================================================
--- util/options/build_opt_tbl.c	(revision 4263)
+++ util/options/build_opt_tbl.c	(working copy)
@@ -511,13 +511,13 @@
         	}
    	/* write the array values */
    	for(i=0;i<(ct->size-1);i++) {
-			if(!(i%10) && !err) err=fwrite("\n\t",1,2,fp);
+			if(!(i%10) && !err) err=!fwrite("\n\t",1,2,fp);
    		sprintf(buf,"0x%02x,",cmos_table[i]);
-			if(!err) err=fwrite(buf,1,5,fp);
+			if(!err) err=!fwrite(buf,1,5,fp);
    	}
    	/* write the end */
    	sprintf(buf,"0x%02x\n",cmos_table[i]);
-		if(!err) err=fwrite(buf,1,4,fp);
+		if(!err) err=!fwrite(buf,1,4,fp);
         	if(!fwrite("};\n",1,3,fp)) {
         	        perror("Error - Could not write image file");
         	        fclose(fp);

[Myles Watson]

> -----Original Message-----
> From: ron minnich [mailto:[e-mail]]
> Sent: Friday, May 08, 2009 11:27 PM
> To: Myles Watson
> Cc: coreboot
> Subject: Re: [coreboot] seemingly wrong code in
> src/arch/i386/boot/coreboot_table.c
>
> Found the problem. 4240 broke builtd_option_tbl due to the addition of
> error checking which got the sense of fwrite wrong.
> fwrite returns 0 on error, non-zero otherwise; the code was written to
> assume non-zero was an error.

Guilty.  Thanks for finding it.
> patch attached inline, qemu is booting again.
> If somebody wants to ack this and commit it that is fine with me.
>
> Signed-off-by: Ronald G. Minnich

Acked-by: Myles Watson

Thanks,
Myles
> Index: util/options/build_opt_tbl.c
>
> --- util/options/build_opt_tbl.c	(revision 4263)
> +++ util/options/build_opt_tbl.c	(working copy)
> @@ -511,13 +511,13 @@
>          	}
>    	/* write the array values */
>    	for(i=0;i<(ct->size-1);i++) {
> 	if(!(i%10) && !err) err=fwrite("\n\t",1,2,fp);
> 	if(!(i%10) && !err) err=!fwrite("\n\t",1,2,fp);
> sprintf(buf,"0x%02x,",cmos_table[i]);
> 	if(!err) err=fwrite(buf,1,5,fp);
> 	if(!err) err=!fwrite(buf,1,5,fp);
> }
> /* write the end */
> sprintf(buf,"0x%02x\n",cmos_table[i]);
> if(!err) err=fwrite(buf,1,4,fp);
> if(!err) err=!fwrite(buf,1,4,fp);
> 	if(!fwrite("};\n",1,3,fp)) {
> 	        perror("Error - Could not write image file");
> 	        fclose(fp);

[ron minnich]

On Sat, May 9, 2009 at 12:27 PM, Myles Watson  wrote:
> Acked-by: Myles Watson

Thanks, I got so worried about the problem that I went and self-acked,
I appreciate your ack.

Committed revision 4264.
ron