a simple zero fill patch

[Myles Watson]

This patch adds a config option for zero-filling coreboot.rom after
adding a payload.  It depends on having a payload so that you can't
end up with a file with no payload and no possiblility to add one.
The default is no zero-filling.

Myles

Signed-off-by: Myles Watson

[Carl-Daniel Hailfinger]

On 25.02.2008 23:13, Myles Watson wrote:
> This patch adds a config option for zero-filling coreboot.rom after
> adding a payload.  It depends on having a payload so that you can't
> end up with a file with no payload and no possiblility to add one.
> The default is no zero-filling.
>
> Myles
>
> Signed-off-by: Myles Watson

Missing patch.

[Myles Watson]

On Mon, Feb 25, 2008 at 3:20 PM, Carl-Daniel Hailfinger
 wrote:
> On 25.02.2008 23:13, Myles Watson wrote:
> > This patch adds a config option for zero-filling coreboot.rom after
> > adding a payload.  It depends on having a payload so that you can't
> > end up with a file with no payload and no possiblility to add one.
> > The default is no zero-filling.
> >
> > Myles
> >
> > Signed-off-by: Myles Watson
>
> Missing patch.

You're right, it wasn't _that_ simple.  :)

Myles
Signed-off-by: Myles Watson

[Carl-Daniel Hailfinger]

On 25.02.2008 23:26, Myles Watson wrote:
> On Mon, Feb 25, 2008 at 3:20 PM, Carl-Daniel Hailfinger
>  wrote:
> > On 25.02.2008 23:13, Myles Watson wrote:
> > > This patch adds a config option for zero-filling coreboot.rom after
> > > adding a payload.  It depends on having a payload so that you can't
> > > end up with a file with no payload and no possiblility to add one.
> > > The default is no zero-filling.
>
> Signed-off-by: Myles Watson
> Index: Kconfig
>
> --- Kconfig	(revision 620)
> +++ Kconfig	(working copy)
> @@ -148,5 +148,13 @@
>      payload in, coreboot can not parse it. We hope to remove ELF
>      entirely in the future.
>
> +config ZERO_AFTER_PAYLOAD
> bool "Zero fill lar after adding the payload"
> depends PAYLOAD_ELF
> default n
> help
>  This option speeds booting but makes it so that no further files may
>  be added to the lar.
>
> endmenu
> Index: arch/x86/Makefile
>
> --- arch/x86/Makefile	(revision 620)
> +++ arch/x86/Makefile	(working copy)
> @@ -87,6 +87,9 @@
>    	../util/lar/lar $(PARSEELF) $(COMPRESSFLAG) -a \
>    		../coreboot.rom normal/payload;
>  endif
> +ifeq ($(CONFIG_ZERO_AFTER_PAYLOAD),y)

Maybe add a message about zerofilling right here.
> $(Q)cd $(obj) && ./util/lar/lar -z ./coreboot.rom
>
> +endif
>    $(Q)# QEMU wants bios.bin:
>    $(Q)# Run "qemu -L build/ -serial stdio -hda /dev/zero".
>    $(Q)printf "  CP      $(subst $(shell pwd)/,,$(obj)/bios.bin)\n"

Otherwise, I like it.

Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel

[Myles Watson]

On Mon, Feb 25, 2008 at 3:42 PM, Carl-Daniel Hailfinger
 wrote:
> On 25.02.2008 23:26, Myles Watson wrote:
> > On Mon, Feb 25, 2008 at 3:20 PM, Carl-Daniel Hailfinger
> >  wrote:
> > > On 25.02.2008 23:13, Myles Watson wrote:
> > > > This patch adds a config option for zero-filling coreboot.rom after
> > > > adding a payload.  It depends on having a payload so that you can't
> > > > end up with a file with no payload and no possiblility to add one.
> > > > The default is no zero-filling.
> > Signed-off-by: Myles Watson
> > Index: Kconfig
> >
> > --- Kconfig   (revision 620)
> > +++ Kconfig   (working copy)
> > @@ -148,5 +148,13 @@
> >         payload in, coreboot can not parse it. We hope to remove ELF
> >         entirely in the future.
> >
> > +config ZERO_AFTER_PAYLOAD
> > bool "Zero fill lar after adding the payload"
> > depends PAYLOAD_ELF
> > default n
> > help
> >   This option speeds booting but makes it so that no further files may
> >   be added to the lar.
> >
> > endmenu
> > Index: arch/x86/Makefile
> >
> > --- arch/x86/Makefile (revision 620)
> > +++ arch/x86/Makefile (working copy)
> > @@ -87,6 +87,9 @@
> >               ../util/lar/lar $(PARSEELF) $(COMPRESSFLAG) -a \
> >                       ../coreboot.rom normal/payload;
> >  endif
> > +ifeq ($(CONFIG_ZERO_AFTER_PAYLOAD),y)
>
> Maybe add a message about zerofilling right here.

Done.
> > $(Q)cd $(obj) && ./util/lar/lar -z ./coreboot.rom
> >
> > +endif
> >       $(Q)# QEMU wants bios.bin:
> >       $(Q)# Run "qemu -L build/ -serial stdio -hda /dev/zero".
> >       $(Q)printf "  CP      $(subst $(shell pwd)/,,$(obj)/bios.bin)\n"
>
> Otherwise, I like it.
>
> Acked-by: Carl-Daniel Hailfinger

Rev 621.

Thanks,
Myles
> Regards,
>  Carl-Daniel

[Peter Stuge]

On Mon, Feb 25, 2008 at 03:26:52PM -0700, Myles Watson wrote:
> +config ZERO_AFTER_PAYLOAD
> bool "Zero fill lar after adding the payload"

Is lar -z actually using 0xff or 0x00 or undefined?

If 0xff I am strongly opposed to the use of the word zero in any form
when describing this lar feature, including z as short option.

Please comment.

//Peter

[Myles Watson]

> -----Original Message-----
> From: [e-mail] [mailto:[e-mail]]
> On Behalf Of Peter Stuge
> Sent: Monday, February 25, 2008 6:38 PM
> To: Coreboot
> Subject: Re: [coreboot] a simple zero fill patch
>
> On Mon, Feb 25, 2008 at 03:26:52PM -0700, Myles Watson wrote:
> > +config ZERO_AFTER_PAYLOAD
> > bool "Zero fill lar after adding the payload"
>
> Is lar -z actually using 0xff or 0x00 or undefined?
>
> If 0xff I am strongly opposed to the use of the word zero in any form
> when describing this lar feature, including z as short option.
>
> Please comment.

I have no idea.  I just implemented the Kconfig option and named it after
the option in lar.

Myles
> //Peter

[ron minnich]

On Tue, Feb 26, 2008 at 7:58 AM, Myles Watson  wrote:
> I have no idea.  I just implemented the Kconfig option and named it after
>  the option in lar.

how about we change it to empty fill? or
sfni (space filling non information) :-)

ron

[Myles Watson]

> -----Original Message-----
> From: ron minnich [mailto:[e-mail]]
> Sent: Tuesday, February 26, 2008 1:57 PM
> To: Myles Watson
> Cc: Peter Stuge; Coreboot
> Subject: Re: [coreboot] a simple zero fill patch
>
> On Tue, Feb 26, 2008 at 7:58 AM, Myles Watson  wrote:
> > I have no idea.  I just implemented the Kconfig option and named it
>
> after
> > the option in lar.
>
> how about we change it to empty fill? or
> sfni (space filling non information) :-)

Just "fill" would work fine for me too.  -f isn't taken.

Myles

[Peter Stuge]

On Tue, Feb 26, 2008 at 02:02:23PM -0700, Myles Watson wrote:
> Just "fill" would work fine for me too.  -f isn't taken.

I like that!

//Peter

[ron minnich]

one more idea: you could call it 'blank fill'.

and I still stop :-)

ron

[Peter Stuge]

On Tue, Feb 26, 2008 at 05:20:42PM -0800, ron minnich wrote:
> one more idea: you could call it 'blank fill'.

Even better. More clear!

//Peter

[Myles Watson]

> one more idea: you could call it 'blank fill'.
>
> and I still stop :-)

Could we just start with the image full of 0xff (when it is created) and
make this option go away?

Myles

[ron minnich]

On Wed, Feb 27, 2008 at 8:02 AM, Myles Watson  wrote:
> Could we just start with the image full of 0xff (when it is created) and
>  make this option go away?

no, because you need the lar header too.

ron

[Carl-Daniel Hailfinger]

On 27.02.2008 17:02, Myles Watson wrote:
> Could we just start with the image full of 0xff (when it is created)

That would be helpful in any case. Can you prepare a patch?
> and make this option go away?

Sorry, as Ron said, it's not that easy.

Regards,
Carl-Daniel

[Myles Watson]

On Wed, Feb 27, 2008 at 4:04 PM, Carl-Daniel Hailfinger
 wrote:
> On 27.02.2008 17:02, Myles Watson wrote:
> > Could we just start with the image full of 0xff (when it is created)
>
> That would be helpful in any case. Can you prepare a patch?

I should have looked first; it already does that in util/lar/stream.c

from lar_new_archive():

/* Fill the whole thing with flash friendly 0xFFs */
    memset(lar->map, 0xFF, lar->size);

Myles
> > and make this option go away?
>
> Sorry, as Ron said, it's not that easy.
>
> Regards,
>  Carl-Daniel