fix for cbfs error for longer file names

[ron minnich]

attached.

This came up when cbfs (correctly) diagnosed a rom as corrupt ... it
was a bug in cbfs, but that's how it's supposed to work -- catch a bad
rom at build time, not boot time.

ron

[Myles Watson]

> This came up when cbfs (correctly) diagnosed a rom as corrupt ... it
> was a bug in cbfs, but that's how it's supposed to work -- catch a bad
> rom at build time, not boot time.
> I still think the right fix is to fix cbfs file names to some value -- 
> e.g. 32 chars -- which would remove a lot of code. 32 chars should be long
> enough for a rom file system name.

I agree.  Let's do that first, since it would simplify this fix.  I'll
review and test.

Thanks,
Myles

[ron minnich]

On Mon, May 11, 2009 at 10:01 AM, Myles Watson  wrote:
> I agree.  Let's do that first, since it would simplify this fix.  I'll
> review and test.

Let's see what other comments we get ... anyone out there see an issue
with 32 byte character names? If you want, we can go with 40, which
will size the struct up to a nice simple 64 bytes ...

ron

[Kevin O'Connor]

On Mon, May 11, 2009 at 10:08:47AM -0700, ron minnich wrote:
> On Mon, May 11, 2009 at 10:01 AM, Myles Watson  wrote:
> > I agree.  Let's do that first, since it would simplify this fix.  I'll
> > review and test.
>
> Let's see what other comments we get ... anyone out there see an issue
> with 32 byte character names? If you want, we can go with 40, which
> will size the struct up to a nice simple 64 bytes ...

Just my 2 cents, but I like the variable length file names.

BTW, you can simplify the existing coreboot code a bit if you add a
"char filename[0];" entry to 'struct cbfs_file' - as SeaBIOS does.

-Kevin

[Myles Watson]

On Mon, May 11, 2009 at 10:32 AM, ron minnich  wrote:
> attached.
>
> This came up when cbfs (correctly) diagnosed a rom as corrupt ... it
> was a bug in cbfs, but that's how it's supposed to work -- catch a bad
> rom at build time, not boot time.

+	csize = headersize(name);
+
+	strcpy(c->magic, COMPONENT_MAGIC);
+
+	c->offset = htonl(csize);

I think this code would be clearer without csize.

+	c->offset = htonl(headersize(name));

The only other comment I have is that rom_add is pretty trivial now.
It could disappear since it is only a wrapper for rom_alloc.  Is there
any time we want to do rom_alloc when we don't want to copy in the
data?

Acked-by: Myles Watson
> BTW, you can simplify the existing coreboot code a bit if you add a
> "char filename[0];" entry to 'struct cbfs_file' - as SeaBIOS does.

I like that idea.

Thanks,
Myles

[ron minnich]

On Tue, May 12, 2009 at 6:05 AM, Myles Watson  wrote:
> On Mon, May 11, 2009 at 10:32 AM, ron minnich  wrote:
> > attached.
> >
> > This came up when cbfs (correctly) diagnosed a rom as corrupt ... it
> > was a bug in cbfs, but that's how it's supposed to work -- catch a bad
> > rom at build time, not boot time.
> csize = headersize(name);
> strcpy(c->magic, COMPONENT_MAGIC);
> c->offset = htonl(csize);
>
> I think this code would be clearer without csize.
> c->offset = htonl(headersize(name));

csize is used one other place in that function. I did not change it as
you recommend for that reason.
> The only other comment I have is that rom_add is pretty trivial now.
> It could disappear since it is only a wrapper for rom_alloc.  Is there
> any time we want to do rom_alloc when we don't want to copy in the
> data?

I don't know, I had the same question when I changed the code, but did
not want to rule out an "empty allocate", and decided not to change
it.
Note that the special case of create is an empty allocate.
> Acked-by: Myles Watson
> > BTW, you can simplify the existing coreboot code a bit if you add a
> > "char filename[0];" entry to 'struct cbfs_file' - as SeaBIOS does.
>
> I like that idea.

I do too; I want to get this change in but we ought to schedule this
fix for the next set.

The nice thing is that since cbfstool is decoupled from coreboot and
seabios code, these changes are easy to make.

Committed revision 4276.

The next big step in my view is setting a flash-friendly value of zero.

ron

[Myles Watson]

> > csize = headersize(name);
> > strcpy(c->magic, COMPONENT_MAGIC);
> > c->offset = htonl(csize);
> >
> > I think this code would be clearer without csize.
> > c->offset = htonl(headersize(name));
>
> csize is used one other place in that function. I did not change it as
> you recommend for that reason.

It looks like a separate (shadowed) declaration.
> > The only other comment I have is that rom_add is pretty trivial now.
> > It could disappear since it is only a wrapper for rom_alloc.  Is there
> > any time we want to do rom_alloc when we don't want to copy in the
> > data?
>
> I don't know, I had the same question when I changed the code, but did
> not want to rule out an "empty allocate", and decided not to change
> it.
> Note that the special case of create is an empty allocate.

I'm not sure we want that, but that's fine.
> > > BTW, you can simplify the existing coreboot code a bit if you add a
> > > "char filename[0];" entry to 'struct cbfs_file' - as SeaBIOS does.
> >
> > I like that idea.
>
> I do too; I want to get this change in but we ought to schedule this
> fix for the next set.

OK
> The nice thing is that since cbfstool is decoupled from coreboot and
> seabios code, these changes are easy to make.

Yes.
> Committed revision 4276.
>
> The next big step in my view is setting a flash-friendly value of zero.

Sounds good.

Myles

[Peter Stuge]

ron minnich wrote:
> > > BTW, you can simplify the existing coreboot code a bit if you
> > > add a "char filename[0];" entry to 'struct cbfs_file' - as
> > > SeaBIOS does.
>
> The next big step in my view is setting a flash-friendly value of
> zero.

I'm not sure it makes sense to try to be flash-friendly in cbfstool.

I think it makes more sense to just not care until flash write time,
at which point the flash writing software should understand cbfs and
do the right thing<tm> for the flash chip at hand.

//Peter

[ron minnich]

On Wed, May 13, 2009 at 4:39 PM, Peter Stuge  wrote:
> I think it makes more sense to just not care until flash write time,
> at which point the flash writing software should understand cbfs and
> do the right thing<tm> for the flash chip at hand.

There is a real problem with that magic flashrom approach. cbfs has
some notion of what should be 0x00 and what should be "space not yet
used". "space not yet used" can be anything. Space that should be 0x00
MUST be 0x00. The simplest example is the null termination for a file
name. It has to be zero. But there is no reason to set the whole file
name area to 0 -- it could just as well be 0xff.

CBFS is best equipped to make the distinction. I'm not comfortable
with having flashrom try to guess.

ron

[Peter Stuge]

> > > > add a "char filename[0];" entry to 'struct cbfs_file' - as
> >
> > The next big step in my view is setting a flash-friendly value of
> > zero.

ron minnich wrote:
> There is a real problem with that magic flashrom approach.

Maybe we are talking across each other.

I thought you meant that cbfs should decide on a fixed filename size
which would be flash chip friendly wrt erase block sizes.

I mean to say that cbfstool doesn't know what is useful. Only the
flashing tool does.
> cbfs has some notion of what should be 0x00 and what should be "space
> not yet used". "space not yet used" can be anything. Space that
> should be 0x00 MUST be 0x00. The simplest example is the null
> termination for a file name. It has to be zero. But there is no
> reason to set the whole file name area to 0 -- it could just as
> well be 0xff. 
> CBFS is best equipped to make the distinction. I'm not comfortable
> with having flashrom try to guess.

Right, that's not what I was saying at all. Sorry for the confusion.
Things such as the filename must be speced so that the flashing tool
knows how to interpret the cbfs contents correctly, and juggle
everything around, if neccessary, to suit the flash chip.

//Peter

[Tom Sylla]

On Tue, May 12, 2009 at 11:09 AM, ron minnich  wrote:
> The next big step in my view is setting a flash-friendly value of zero.

Do you mean flashrom-friendly value of 0xFF?

[ron minnich]

On Wed, May 13, 2009 at 5:59 PM, Tom Sylla  wrote:
> On Tue, May 12, 2009 at 11:09 AM, ron minnich  wrote:
> > The next big step in my view is setting a flash-friendly value of zero.
>
> Do you mean flashrom-friendly value of 0xFF?

flash-friendly value of 0xff for unitniialized areas. Sorry, I worded
that very poorly.

ron