[PATCH] Add option to only execute CBFS option ROMs

[Timothy Pearson]

All,

This is my first time submitting a patch to SeaBIOS for 
review/inclusion; I assume this list is the correct initial contact?

I have run into several situations where it is neccessary to execute 
only the relevant option ROMs contained in CBFS while ignoring any 
option ROM binary code present on PCI/PCIe devices; e.g. when the option 
ROM provided by the manufacturer is buggy but a FOSS replacement is 
available that works properly.

This patch adds an option to only execute option ROMs contained in CBFS. 
  Tested on ASUS KFSN4-DRE with iPXE ROMs built in to CBFS; with this 
option set the on-board network ROMs were ignored while the iPXE ROMs 
executed normally.

[Paolo Bonzini]

On 06/02/2015 22:13, Timothy Pearson wrote:
> config CBFS_OPTIONROMS_ONLY
>    depends on OPTIONROMS

Probably "depends on OPTIONROMS && COREBOOT_FLASH" is better.

Paolo
>    bool "Only execute option ROMs stored in CBFS"
>    default "n"
>    help

[Timothy Pearson]

On 02/09/2015 03:28 AM, Paolo Bonzini wrote:
> On 06/02/2015 22:13, Timothy Pearson wrote:
> > config CBFS_OPTIONROMS_ONLY
> >    depends on OPTIONROMS
>
> Probably "depends on OPTIONROMS&&  COREBOOT_FLASH" is better.
>
> Paolo
> >    bool "Only execute option ROMs stored in CBFS"
> >    default "n"
> >    help

Do I need to send in an updated patch?  Is there anything else that 
needs to be done before this can be merged?

Thanks!

[Paul Menzel]

Am Montag, den 09.02.2015, 11:57 -0600 schrieb Timothy Pearson:
> On 02/09/2015 03:28 AM, Paolo Bonzini wrote:
> > On 06/02/2015 22:13, Timothy Pearson wrote:
> > > config CBFS_OPTIONROMS_ONLY
> > >    depends on OPTIONROMS
> >
> > Probably "depends on OPTIONROMS&&  COREBOOT_FLASH" is better.
> > >    bool "Only execute option ROMs stored in CBFS"
> > >    default "n"
> > >    help
>
> Do I need to send in an updated patch?

Yes, that would be great! Please tag it as `v2`, i. e., `[PATCH v2]`. If
you use `git format-patch`, you can use the switch `--subject-prefix`
for that. See `git help format-patch` for more information. But for just
one patch it is probably easier to manually edit the subject line in
your mail client.

Please send patches that can be applied with `git am`. I think you patch
cannot be applied easily as Git does not detect the commit message.
> Is there anything else that needs to be done before this can be
> merged?

Normally, Kevin, the SeaBIOS main developer, is pretty quick with
reviewing patches. As he has not done so yet, I guess he is on vacation,
sick or traveling. So just wait a little longer. ;-)

Thanks,

Paul

[PATCH v2] Add option to only execute CBFS option ROMs

[Timothy Pearson]

On 02/09/2015 04:53 PM, Paul Menzel wrote:
> Am Montag, den 09.02.2015, 11:57 -0600 schrieb Timothy Pearson:
> > On 02/09/2015 03:28 AM, Paolo Bonzini wrote:
> > > On 06/02/2015 22:13, Timothy Pearson wrote:
> > > > config CBFS_OPTIONROMS_ONLY
> > > >    depends on OPTIONROMS
> > >
> > > Probably "depends on OPTIONROMS&&   COREBOOT_FLASH" is better.
> > > >    bool "Only execute option ROMs stored in CBFS"
> > > >    default "n"
> > > >    help
> >
> > Do I need to send in an updated patch?
>
> Yes, that would be great! Please tag it as `v2`, i. e., `[PATCH v2]`. If
> you use `git format-patch`, you can use the switch `--subject-prefix`
> for that. See `git help format-patch` for more information. But for just
> one patch it is probably easier to manually edit the subject line in
> your mail client.
>
> Please send patches that can be applied with `git am`. I think you patch
> cannot be applied easily as Git does not detect the commit message.
> > Is there anything else that needs to be done before this can be
> > merged?
>
> Normally, Kevin, the SeaBIOS main developer, is pretty quick with
> reviewing patches. As he has not done so yet, I guess he is on vacation,
> sick or traveling. So just wait a little longer. ;-)
>
> Thanks,
>
> Paul

Change applied and patch reformatted.  Is this more what you were 
looking for?

Thanks!

[PATCH v2] Add option to only execute CBFS option ROMs

[Paul Menzel]

Am Montag, den 09.02.2015, 22:53 -0600 schrieb Timothy Pearson:

[…]
> Change applied and patch reformatted.  Is this more what you were 
> looking for?

Mostly, yes. It looks like you only copied the output of `git show` or
something similar and did not use `git format-patch -1`. This causes the
commit message to be indented which is not wanted and `git am` cannot be
used. It’d be great if you could reply with the output file of `git
format-patch` attached.

Thanks,

Paul

[Peter Stuge]

Timothy Pearson wrote:
> Is there anything else that needs to be done before this can be merged?

Have you considered creating a more fine-grained control knob than
simply global on/off?

Maybe a BDF blacklist, perhaps stored in CBFS?

//Peter

[PATCH v2] Add option to only execute CBFS option ROMs

[Timothy Pearson]

On 02/10/2015 06:23 PM, Paul Menzel wrote:
> Am Montag, den 09.02.2015, 22:53 -0600 schrieb Timothy Pearson:
>
> […]
> > Change applied and patch reformatted.  Is this more what you were
> > looking for?
>
> Mostly, yes. It looks like you only copied the output of `git show` or
> something similar and did not use `git format-patch -1`. This causes the
> commit message to be indented which is not wanted and `git am` cannot be
> used. It’d be great if you could reply with the output file of `git
> format-patch` attached.
>
> Thanks,
>
> Paul

OK.  I haven't had to submit patches from GIT over a mailing list 
before, so let's see if I got it right this time...

Thanks!

[Timothy Pearson]

On 02/10/2015 06:45 PM, Peter Stuge wrote:
> Timothy Pearson wrote:
> > Is there anything else that needs to be done before this can be merged?
>
> Have you considered creating a more fine-grained control knob than
> simply global on/off?
>
> Maybe a BDF blacklist, perhaps stored in CBFS?
>
> //Peter
>
> SeaBIOS mailing list
> [e-mail]
> http://www.seabios.org/mailman/listinfo/seabios

I might implement something like that in the future if I have 
time/inclination, but for now the on/off switch is sufficient.  The 
proposed patch also allows the user to have a completely blob-free 
system if desired.

[Peter Stuge]

Timothy Pearson wrote:
> > > Is there anything else that needs to be done before this can be merged?
> >
> > Have you considered creating a more fine-grained control knob than
> > simply global on/off?
> >
> > Maybe a BDF blacklist, perhaps stored in CBFS?
>
> I might implement something like that in the future if I have 
> time/inclination, but for now the on/off switch is sufficient.

Sufficient sure, but it is certainly using a sledgehammer to pound a nail.

Adding a blacklist instead is probably very quick. Pretty please? :)
(Ultimately Kevin will decide, but maybe he also likes a blacklist.)
> The proposed patch also allows the user to have a completely
> blob-free system if desired.

More general wins over fan(boy|girl) idealistic every time with me.

//Peter

[Timothy Pearson]

On 02/10/2015 08:02 PM, Peter Stuge wrote:
> Timothy Pearson wrote:
> > > > Is there anything else that needs to be done before this can be merged?
> > >
> > > Have you considered creating a more fine-grained control knob than
> > > simply global on/off?
> > >
> > > Maybe a BDF blacklist, perhaps stored in CBFS?
> >
> > I might implement something like that in the future if I have
> > time/inclination, but for now the on/off switch is sufficient.
>
> Sufficient sure, but it is certainly using a sledgehammer to pound a nail.
>
> Adding a blacklist instead is probably very quick. Pretty please? :)
> (Ultimately Kevin will decide, but maybe he also likes a blacklist.)

I can try.  I am nowhere near as familiar with SeaBIOS as I am with 
coreboot so this might take longer than expected.
> > The proposed patch also allows the user to have a completely
> > blob-free system if desired.
>
> More general wins over fan(boy|girl) idealistic every time with me.

There are reasons to want a blob-free system, including security.  I 
would hardly call it "fanboy idealism" to, for instance, not want to 
rely on a signed closed-source Microsoft bootloader to launch Linux, yet 
the situation here is not all that different in that we rely on 
untrusted, unknown code to initialize hardware before passing control on 
to a FOSS payload.  When there are FOSS replacements available it can 
make sense to eliminate the blobs.

[Peter Stuge]

Timothy Pearson wrote:
> > > > Maybe a BDF blacklist, perhaps stored in CBFS?
> > >
> > > I might implement something like that in the future if I have
> > > time/inclination, but for now the on/off switch is sufficient.
> >
> > Sufficient sure, but it is certainly using a sledgehammer to pound a nail.
> >
> > Adding a blacklist instead is probably very quick. Pretty please? :)
> > (Ultimately Kevin will decide, but maybe he also likes a blacklist.)
>
> I can try.  I am nowhere near as familiar with SeaBIOS as I am with 
> coreboot so this might take longer than expected.

Fair enough! I think there will be a lot of reusable code in SeaBIOS
already.

For accessing a CBFS file try: git grep romfile_loadfile

For parsing file into a string array see loadBootOrder() in src/boot.c.
(Not so pretty, basically open-coding strtok(), but gets the job done.)
> > > The proposed patch also allows the user to have a completely
> > > blob-free system if desired.
> >
> > More general wins over fan(boy|girl) idealistic every time with me.
>
> There are reasons to want a blob-free system, including security.

Agreed.

//Peter

[PATCH v3] Add option to only execute CBFS option ROMs

[Timothy Pearson]

On 02/10/2015 10:26 PM, Peter Stuge wrote:
> Timothy Pearson wrote:
> > I can try.  I am nowhere near as familiar with SeaBIOS as I am with
> > coreboot so this might take longer than expected.
>
> Fair enough! I think there will be a lot of reusable code in SeaBIOS
> already.
>
> For accessing a CBFS file try: git grep romfile_loadfile
>
> For parsing file into a string array see loadBootOrder() in src/boot.c.
> (Not so pretty, basically open-coding strtok(), but gets the job done.)
> > There are reasons to want a blob-free system, including security.
>
> Agreed.
>
> //Peter

Please find the two patches attached.  The first is the sledgehammer for 
security nuts, and the second is the fine-grained blacklist for everyone 
else.

[PATCH v2] Add option to only execute CBFS option ROMs

[Paul Menzel]

Am Dienstag, den 10.02.2015, 19:50 -0600 schrieb Timothy Pearson:
> On 02/10/2015 06:23 PM, Paul Menzel wrote:
> > Am Montag, den 09.02.2015, 22:53 -0600 schrieb Timothy Pearson:
> >
> > […]
> > > Change applied and patch reformatted.  Is this more what you were
> > > looking for?
> >
> > Mostly, yes. It looks like you only copied the output of `git show` or
> > something similar and did not use `git format-patch -1`. This causes the
> > commit message to be indented which is not wanted and `git am` cannot be
> > used. It’d be great if you could reply with the output file of `git
> > format-patch` attached.

[…]
> OK.  I haven't had to submit patches from GIT over a mailing list 
> before, so let's see if I got it right this time...

It looks like you did! The cherry on top would be, that it’s just one
mail without an attachment. ;-)

Looks good to me!

Thanks,

Paul

[PATCH v3] Add option to only execute CBFS option ROMs

[Paul Menzel]

Am Mittwoch, den 11.02.2015, 15:55 -0600 schrieb Timothy Pearson:

[…]
> Please find the two patches attached.  The first is the sledgehammer for 
> security nuts, and the second is the fine-grained blacklist for everyone 
> else.

Thanks for implementing Peter’s idea.

Please put one patch in one email message (with the appropriate subject)
as otherwise the second patch might go unnoticed.

Thanks,

Paul

[PATCH v3] Add option to only execute CBFS option ROMs

[Timothy Pearson]

On 02/11/2015 04:37 PM, Paul Menzel wrote:
> Am Mittwoch, den 11.02.2015, 15:55 -0600 schrieb Timothy Pearson:
>
> […]
> > Please find the two patches attached.  The first is the sledgehammer for
> > security nuts, and the second is the fine-grained blacklist for everyone
> > else.
>
> Thanks for implementing Peter’s idea.
>
> Please put one patch in one email message (with the appropriate subject)
> as otherwise the second patch might go unnoticed.
>
> Thanks,
>
> Paul

Patches sent.  I assume this method was correct?

Thanks!