[coreboot] Leadership meeting -- BigBlueButton mobile support

[Julius Werner]

Hi,

After we just tried out BigBlueButton for the meeting, I have to say
that I found its mobile support unfortunately lacking. It doesn't have
an official Android app as far as I can tell, and the only unofficial
one I found 
(https://play.google.com/store/apps/details?id=com.bigbluebuttonmobile)
says "this item isn't available in your country" when I try to install
it in the US. I can open the website from a mobile browser but then it
doesn't have any mobile-specific UI or features (like using the "phone
call" speaker instead of the big one), so it's pretty useless for
that.

I usually need to take part of the meeting on the go, and it seems
that the only useable option for that is the POTS phone bridge which
is obviously not a great experience (can't raise hands, can't see when
other people have hands raised, etc.). I'm not sure for what reasons
other people were asking to switch away from Google Meet, but I just
wanted to add the data point that at least for my use case, it worked
way better.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: QEMU x86 i440fx/piix4 build fails for >= 32MB ROMs - Assertion IS_HOST_SPACE_ADDRESS(host_space_address) failed

[Julius Werner]

; >> >> in the SPI controller that I'd expect to be present if it supports the 4
> >> >> byte address mode.
> >> >>
> >> >> Regards,
> >> >> Felix
> >> >>
> >> >> On 19/02/2024 19:55, Mike Banon wrote:
> >> >> > Theoretically - yes, if someone finds & solders there a 32 MB (256
> >> >> > megabit) SPI Flash chip with 8 pins. Hopefully, as the proprietary
> >> >> > UEFIs become more & more bloated, these large capacity chips will
> >> >> > become more widely available in the near future. And, since a coreboot
> >> >> > itself consumes less than 1MB on these "opensource AGESA" AMD systems
> >> >> > such as G505S and A88XM-E, all this room will allow some very
> >> >> > interesting experiments! If even 3 MB is enough for me to put 9 of 10
> >> >> > floppies of the collection described here (thanks to LZMA compression)
> >> >> > - 
> >> >> > http://dangerousprototypes.com/docs/Lenovo_G505S_hacking#Useful_floppies
> >> >> > , guess what wonders we can do with 31 MB... ;-)
> >> >> >
> >> >> > On Mon, Feb 19, 2024 at 7:17 PM ron minnich  
> >> >> > wrote:
> >> >> >>
> >> >> >> Can the system you are discussing actually use larger than 16 MB rom?
> >> >> >>
> >> >> >>   I am wondering about your use of the phrase “out of curiosity”
> >> >> >>
> >> >> >> On Mon, Feb 19, 2024 at 07:05 Mike Banon  wrote:
> >> >> >>>
> >> >> >>> Small bump, I am still having this error while (out of curiosity)
> >> >> >>> trying to build the Lenovo G505S ROM for 32 MB or 64 MB spi flash:
> >> >> >>>
> >> >> >>>  OBJCOPYbootblock.raw.bin
> >> >> >>> Created CBFS (capacity = 33488356 bytes)
> >> >> >>>  BOOTBLOCK
> >> >> >>>  CBFS   cbfs_master_header
> >> >> >>>  CBFS   fallback/romstage
> >> >> >>> Image SIZE 33554432
> >> >> >>> cbfstool: 
> >> >> >>> /media/mint/2183183a-158f-476a-81af-b42534a68706/shared/core/coreboot/util/cbfstool/cbfstool.c:1186:
> >> >> >>> cbfstool_convert_mkstage: Assertion
> >> >> >>> `IS_HOST_SPACE_ADDRESS(host_space_address)' failed.
> >> >> >>> Aborted (core dumped)
> >> >> >>> make: *** [Makefile.mk:1210: build/coreboot.pre] Error 134
> >> >> >>>
> >> >> >>> Meanwhile, it builds fine for 4 MB / 8 MB / 16 MB , only these large
> >> >> >>> sizes are a problem
> >> >> >>>
> >> >> >>> On Sat, Jun 25, 2022 at 12:55 AM Julius Werner 
> >> >> >>>  wrote:
> >> >> >>>>
> >> >> >>>> I can see a little bug that makes this return a confusing error (it
> >> >> >>>> should have really failed with `SPI flash address(0x300) not in any
> >> >> >>>> mmap window!`), and we can fix that if you want. But that still 
> >> >> >>>> won't
> >> >> >>>> make this build (and my patch didn't cause the underlying problem,
> >> >> >>>> before that it may have built an image but it probably wouldn't 
> >> >> >>>> have
> >> >> >>>> booted). By default cbfstool only expects the top 16MB of the 
> >> >> >>>> flash to
> >> >> >>>> be memory-mapped, so it cannot link XIP stages into areas outside 
> >> >> >>>> of
> >> >> >>>> that. The real solution is to either change your FMAP to put the
> >> >> >>>> COREBOOT section into the top 16MB (we might want to change the
> >> >> >>>> auto-generated default FMAP to do that), or pass
> >> >> >>>> --ext-win-base/--ext-win-size options to cbfstool to tell it how to
> >> >> >>>> map areas below the top 16MB.
> >> >> >>>>
> >> >> >>>> On Thu, Jun 23, 2022 at 1:09 AM Paul Menzel 
> >> >> >>>>  wrote:
> >> >> >>>>>
> >&g

[coreboot] Re: coreboot's role in the boot process -- is it time for a coreboot spec?

[Julius Werner]

> I don't like superlatives. I don't think it needs to be "completely
> separate". For instance, when somebody discusses coreboot for a new
> platform behind closed doors[1]. And they implement something on the
> same code base. If they did that according to spec, it would be more
> likely to get accepted upstream, wouldn't it?

I think whether stuff gets accepted upstream or not depends on whether
it follows coreboot coding conventions, fits in with our existing APIs
and general architecture, etc. I don't think it's really feasible to
write everything that goes into the code review and design discussion
process down in advance, and even if we could I'm not sure we'd really
want to either. Do we want to create a situation where someone uploads
code they developed in the dark and then tries to force us to take it
because "it matches the spec", even though we don't like it for some
good reason that we didn't anticipate in advance when writing the
spec? I think developing in the open and seeking consensus among all
upstream reviewers should continue to remain the officially
recommended way to develop in coreboot, and anyone who for whatever
reason develops stuff behind closed doors instead needs to understand
that they're responsible for dealing with other opinions when they
eventually decide to upload, including the risk that the majority of
the community says "we don't want this at all" or "we want a complete
redesign from the ground up".

If you want to add documentation that explains how coreboot code
should fit together and where to integrate certain new features, or
just lists general best practices, I'm all for that. We have a bit of
that already and we could certainly always use more, or try to make it
more organized and discoverable. I would just be wary of calling it
anything that makes it sound official and authoritative. The term
"specification" usually implies that as long as you follow this thing
to the letter, you can _demand_ that your implementation should be
considered correct and everyone else who doesn't accept it is wrong. I
don't think we want anything like that for the coreboot development
process. Helping people do the right thing from the start is great,
but it should always remain understood that not everything that goes
into the process can be written down in advance and that the final
decision is done for each individual patch at review time.

Or is the question more about "up to what point are people allowed to
say 'it runs coreboot' when they have out-of-tree code"? I'd say
that's an entirely different thing (that I'm less interested in tbh,
but maybe others are). I don't think we really have that problem in
practice yet, and if we ever get to the point where we do I think just
drawing the line at "runs 100% upstream code" should be good enough?
coreboot is GPL so if people don't upstream their code there's really
only two possible reasons, either they're lazy and unreciprocating, or
their code is junk that wouldn't get accepted upstream.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: coreboot's role in the boot process -- is it time for a coreboot spec?

[Julius Werner]

Sounds to me like what you're asking for is really documentation, not
a spec? Or maybe project-internal rules about what individual platform
code may and may not do (but that's still not really a spec)?

In my understanding, a specification is always something defining a
standard that allows interoperability _between_ different
implementations. Something that only applies to a single
implementation (i.e. a single code base) can't really be a
specification. Making a "coreboot specification" would mean that
someone else could then take that and implement their own "coreboot"
in a completely separate cleanroom code base from scratch, and I don't
think that makes sense. (We could create a specification for the
coreboot payload handoff interface (i.e. coreboot tables) so that
other people could write their own firmware stack that can run
coreboot payloads. I don't think that would make much sense, though.
If we wanted payload interoperability we should probably rather attach
to one of the various other "universal payload" proposals that were
going around, although we've had discussions about that before where I
at least argued that I don't think that makes much sense for
coreboot.)
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: [RFC] vboot recovery reason clearing

[Julius Werner]

> Payload and OS integration is one thing. What I still didn't figure out is how
> to tell vboot to check if RW partition is valid? Imagine the platform is in
> recovery mode and I have flashed the RW with correctly signed image. vboot 
> will
> not attempt to boot from RW, because of the recovery reason being non-zero. 
> Thus
> the only way I see is to clear the recovery reason. How it is solved on 
> ChromeOS
> systems after updating with correct RW firmware? Is there any flag (in vboot
> shared data/workbuf) to tell vboot to attempt RW check despite recovery?

Well, in our flow we always clear the recovery reason from the payload
(as part of running through vb2api_kernel_phase2()). So if you aren't
using the kernel verification part but otherwise want the equivalent
of that, then the solution would be to make vb2_clear_recovery()
available as a top-level API function and call it from your payload.

Alternatively, you could also just have your OS run `crossystem
recovery_request=0` after it has reinstalled the RW firmware.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: [RFC] vboot recovery reason clearing

[Julius Werner]

Yeah, we moved the point where the recovery reason gets cleared to the
payload a while ago, because otherwise it got lost when the platform
decides to do extra reboots. For example, some Intel SoCs do CSE sync
in ramstage, and sometimes when they do that they need to do an extra
reboot. When we used to clear the recovery reason in verstage, that
extra reboot after that point meant we weren't in recovery mode
anymore when we finally reached the payload.

I don't mind renaming vb2_clear_recovery() to vb2api_clear_recovery()
and making it available in the public API. Then you can either add
code somewhere to the end of coreboot to call it (but with a Kconfig
that excludes ChromeOS) or link your payload to vboot and call it from
there (which might be useful if your payload also has situations where
it may need to do an extra reboot). Happy to review a patch to vboot
if you want to send one.

In general though, I think you really need payload or OS integration
if you want to have a useful recovery mode. Recovery mode usually
means something is actually broken that needs to be fixed (e.g. RW
partition corrupted from a bad update), and coreboot alone can't fix
it. So we can clear the recovery condition wherever it works best, but
you still need to implement the part that will actually fix the system
somewhere.

On Mon, Apr 3, 2023 at 2:48 AM Michał Żygowski
 wrote:
>
> Hi coreboot folks,
>
> I have recently stumbled upon an issue that non-ChromeOS platforms, once 
> entered
> into recovery mode, cannot leave this state, despite the RW partition being
> updated with correctly signed firmware copy. I.e. imagine situation where RW A
> (and B) is not valid, vboot logic causes to boot into recovery. Flash is 
> updated
> with valid RW A (and B) but the vboot logic does not try to verify the RW
> partition, instead is stuck in recovery mode due to VBOOT NVRAM content.
>
> For ChromeOS platform the recovery reason is cleared in vb2api_kernel_phase2
> but vb2api_kernel_phase2 is probably not used anywhere except depthcharge (or
> whatever is loading the ChromeOS kernel). So non-ChromeOS platform using vboot
> have no option to get out of recovery. Unless I am missing something, then
> please correct me.
>
> My suggestion would be to add vb2_clear_recovery to vb2api exposed to the
> coreboot and let the platform code decide when the recovery request should be
> cleared. Also coreboot can attempt to verify RW partition despite recovery
> reason, but it would probably be inefficient and lead to situations where
> recovery mode should be entered, but wasn't entered.
>
> Dear ChromeOS firmware experts, your opinion is highly appreciated.
>
> Best regards,
> --
> Michał Żygowski
> Firmware Engineer
> GPG: 6B5BA214D21FCEB2
> https://3mdeb.com | @3mdeb_com
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: How can I set up a userspace test for coreboot code in 2023?

[Julius Werner]

We actually have a real unit test framework for coreboot now, so you
should be using that rather than trying to set up something
standalone. Hopefully that will make it easier to get things to
compile correctly as well. Check out the guide at
Documentation/tutorial/part3.md and the existing tests under tests/.

On Sun, Apr 2, 2023 at 8:36 AM Nico Huber  wrote:
>
> Hi Keith,
>
> On 02.04.23 15:33, Keith Hui wrote:
> > Here is the command line I figured would work, gathered with "make -n":
> >
> > gcc -Isrc -Isrc/include -Isrc/commonlib/include
> > -Isrc/commonlib/bsd/include -Ibuild -I3rdparty/vboot/firmware/include
> > -include src/include/kconfig.h -include src/include/rules.h -include
> > src/commonlib/bsd/include/commonlib/bsd/compiler.h -I3rdparty
> > -D__BUILD_DIR__=\"build\" -Isrc/arch/x86/include -D__ARCH_x86_32__
> > -Isrc/device/oprom/include -nostdinc -pipe -g -std=gnu11 -nostdlib
> > -Wall -Wundef -Wstrict-prototypes -Wmissing-prototypes -Wwrite-strings
> > -Wredundant-decls -Wno-trigraphs -Wimplicit-fallthrough -Wshadow
> > -Wdate-time -Wtype-limits -Wvla -Wold-style-definition -Wdangling-else
> > -Wmissing-include-dirs -fno-common -ffreestanding -fno-builtin
> > -fomit-frame-pointer -fstrict-aliasing -ffunction-sections
> > -fdata-sections -fno-pie -Wno-packed-not-aligned -fconserve-stack
> > -Wnull-dereference -Wreturn-type -Wlogical-op -Wduplicated-cond
> > -Wno-unused-but-set-variable -Werror -Os -Wno-address-of-packed-member
> >  -m32  -fuse-ld=bfd -fno-stack-protector -Wl,--build-id=none
> > -fno-delete-null-pointer-checks -Wlogical-op -march=i686 -mno-mmx
> > --param asan-globals=0 -D__RAMSTAGE__ -include build/config.h
> > src/northbridge/intel/i440bx/raminit.c
> > src/northbridge/intel/i440bx/raminittest.c -o raminittest2023
> >
> > The C test code is attached.
>
> I'm seeing two errors, smbus_read_byte() not being declared and
>  missing.
>
> For the former, you should build with -D__ROMSTAGE__ (not R*A*MSTAGE).
>
> For the latter, I guess it might be better to compile `raminit.c` with
> the coreboot include paths and your `raminittest.c` with your host
> headers, i.e. *without* `-nostdinc` and coreboot paths. If that actually
> works depends a lot on details; it would be best to compile everything
> with the host's standard headers and only use those coreboot headers
> that are coreboot specific. However, both kinds of headers are mixed
> in coreboot's directory structure and some headers in coreboot that have
> standard names are actually not standard :-/
>
> With separate compilation, I get to
>
>   fatal error: standalonetest.h: No such file or directory
>
> ;)
>
> Cheers,
> Nico
>
> ___
> coreboot mailing list -- coreboot@coreboot.org
> To unsubscribe send an email to coreboot-le...@coreboot.org
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] [coreboot - Feature #433] Unify TPM drivers in coreboot

[Julius Werner]

Issue #433 has been updated by Julius Werner.





If we want to do major changes to the TPM API I would prefer to use that 
opportunity to rather redesign it from scratch instead of perpetuating a bunch 
of weird design choices that haven't made sense in a while (or ever, really). A 
lot of that code was haphazardly copied from U-Boot in the early prototyping 
phase for TPM support and then never cleaned up or reevaluated to check if it 
actually makes any sense for coreboot.



For example, why do we have tis_init(), tis_open() and tis_close()? init() and 
open() are always called right after each other, and nothing in coreboot ever 
calls close(). The tpm_chip structure also makes no sense when it's just a 
container for tpm_vendor_specific where all the relevant things are stored in 
(and which isn't actually vendor-specific in all cases). The name "tis" (which 
technically stands for TPM Interface Specification) is also used in places 
where that descriptor doesn't actually make sense (to distinguish from the 
things just prefixed "tpm_").



For coreboot, the unifying TPM layer we have is in src/security/tss, 
specifically tpm_process_command() and tlcl_lib_init(). I don't think we really 
need any more interface-independent layers beneath that, those two can directly 
call into an init() and a sendrecv() implemented by the individual drivers (and 
those drivers can just keep what information they need in global variables 
because they're never instantiated more than once, no need for some complicated 
partially-common/partially-driver-specific structure construction). If you want 
to be able to enable more then one driver, then tlcl_lib_init() could call the 
init function for all of them and have the one that succeeds return a function 
pointer that is then used for sendrecv() or something like that.





Feature #433: Unify TPM drivers in coreboot

https://ticket.coreboot.org/issues/433#change-1223



* Author: Michał Żygowski

* Status: New

* Priority: Normal

* Target version: none

* Start date: 2022-10-24



Add an option to compile all drivers for TPM 1.2, 2.0 TIS and CRB. The 
motivation is to not build multiple coreboot ROMs for each possible TPM 
supported by the platform.



The tasks would include:

- runtime TPM detection (probing TPM_INTF_CAPABILITY and TPM_INTERFACE_ID)

- rename the TPM driver functions, make them static and expose them as a driver 
structure, e.g.



struct tpm_driver {

void (*init)(void);

int (*open)(void);

int (*close)(void);

int (*sendrecv)(const uint8_t *sendbuf, size_t send_size, uint8_t 
*recvbuf, size_t *recv_len);

}



- based on the detected TPM, hook the tpm_driver functions to provide the 
global TPM API: tis_open, tis_close, tis_init, tis_sendrecv. Some additional 
API to get vendor/device name could also be considered.













-- 

You have received this notification because you have either subscribed to it, 
or are involved in it.

To change your notification preferences, please click here: 
https://ticket.coreboot.org/my/account

___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] [coreboot - Cleanup #421] Change API of functions taking hash as an argument

[Julius Werner]

Issue #421 has been updated by Julius Werner.


> However, instead of following existing standards, be it TCG or coreboot, such 
> approach creates yet another one. Having the ability to use more than one 
> would make transition to TPM2.0 easier, if not no-op. Since we are going to 
> have to change event log generation code anyway, we want to do it properly, 
> instead of putting another half-measure in place.

Sorry, I still get the impression that we have a fundamental misunderstanding 
here. The TCG does *not* dictate how many hashes need to be logged in the TCPA 
log! (Or does it? If I'm wrong about this please clarify the exact spec and 
section that defines what hash algorithms *must* be present in the long, 
because I am not aware of any such requirement.) The TCG defined a log format 
that *allows* an implementation to log one or more hashes of different 
algorithms for each measurement entry. What exact algorithms and how many of 
them to use is entirely up to the implementation.

So no, we would not be "putting another half-measure in place" that creates 
"yet another" standard. We would be switching to the exact TCG standard that 
you want (which I am generally not opposed to at all), we would use that exact 
format, and we would just *choose* to only log one hash for one algorithm in 
that data structure that is designed to hold one or more hashes depending on 
how the writer decides to fill it out. Because we don't have a use case for 
more than one hash. That's all I'm talking about.


Cleanup #421: Change API of functions taking hash as an argument
https://ticket.coreboot.org/issues/421#change-1220

* Author: Krystian Hebel
* Status: New
* Priority: Normal
* Target version: none
* Start date: 2022-10-12

All existing functions that take a digest as an input assume that only one 
hashing algorithm is used at a time. Crypto agile format entry can (and should) 
log every used PCR bank in one entry for a given measurement. To make it work, 
some of the arguments must be changed, e.g.:

- pass number of algorithms used;
- instead of algorithm ID, pass a pointer to array of such IDs, with size equal 
to above;
- instead of hash, pass a pointer to array of hashes, with size and order as 
above.



-- 
You have received this notification because you have either subscribed to it, 
or are involved in it.
To change your notification preferences, please click here: 
https://ticket.coreboot.org/my/account
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] [coreboot - Cleanup #421] Change API of functions taking hash as an argument

[Julius Werner]

Issue #421 has been updated by Julius Werner.


> It would actually simplify the API by making parameter lists shorter and 
> input data better grouped.

Having to construct a separate parameter struct rather than just throwing in 
two scalars is not "simpler".

> `skiboot` writes both SHA1 and SHA256 hashes to TPM2 log. I didn't count it 
> as a use case because so far we were using TPM1.2, but it does show existence 
> of logs with multiple hashes in the wild.

I don't know what skiboot is... is that coreboot? Do they have a real use case 
for having both hashes in the log or is it just another bootloader where 
someone decided "might as well write all the hashes in advance just because the 
spec technically allows for it"?

My question is: is there any user of coreboot right now who would actually turn 
on multiple hashes for production purposes because otherwise something they 
need doesn't work for them?


Cleanup #421: Change API of functions taking hash as an argument
https://ticket.coreboot.org/issues/421#change-1209

* Author: Krystian Hebel
* Status: New
* Priority: Normal
* Target version: none
* Start date: 2022-10-12

All existing functions that take a digest as an input assume that only one 
hashing algorithm is used at a time. Crypto agile format entry can (and should) 
log every used PCR bank in one entry for a given measurement. To make it work, 
some of the arguments must be changed, e.g.:

- pass number of algorithms used;
- instead of algorithm ID, pass a pointer to array of such IDs, with size equal 
to above;
- instead of hash, pass a pointer to array of hashes, with size and order as 
above.



-- 
You have received this notification because you have either subscribed to it, 
or are involved in it.
To change your notification preferences, please click here: 
https://ticket.coreboot.org/my/account
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] [coreboot - Cleanup #421] Change API of functions taking hash as an argument

[Julius Werner]

Issue #421 has been updated by Julius Werner.


> I think we might as well implement agile format properly (no fixed-size 
> buffers in structures, any number of algorithms) right away.

You are implementing the format properly (the format defines how the TCPA log 
is supposed to look in memory, not what kind of APIs the code that writes it 
internally need to provide). You are just not implementing all possible ways 
for coreboot to fill it out.

I still feel strongly that we shouldn't overcomplicate APIs and increase 
maintenance burden by implementing options that nobody has a use case for right 
now and likely never will. https://en.wikipedia.org/wiki/YAGNI


Cleanup #421: Change API of functions taking hash as an argument
https://ticket.coreboot.org/issues/421#change-1207

* Author: Krystian Hebel
* Status: New
* Priority: Normal
* Target version: none
* Start date: 2022-10-12

All existing functions that take a digest as an input assume that only one 
hashing algorithm is used at a time. Crypto agile format entry can (and should) 
log every used PCR bank in one entry for a given measurement. To make it work, 
some of the arguments must be changed, e.g.:

- pass number of algorithms used;
- instead of algorithm ID, pass a pointer to array of such IDs, with size equal 
to above;
- instead of hash, pass a pointer to array of hashes, with size and order as 
above.



-- 
You have received this notification because you have either subscribed to it, 
or are involved in it.
To change your notification preferences, please click here: 
https://ticket.coreboot.org/my/account
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] [coreboot - Cleanup #421] Change API of functions taking hash as an argument

[Julius Werner]

Issue #421 has been updated by Julius Werner.


> > Can you explain what use case you have that requires you to use multiple 
> > algorithms?
> 
> This allows for greater flexibility, where multiple coexisting programs may 
> have different expectations, e.g. one is old enough to not know anything but 
> SHA1, and another that considers SHA1 not secure enough.

Sorry, but that sounds kinda vague... I mean, do you actually have a case right 
now where you need this on one of the coreboot platforms you're building? And 
how is the algorithm supposed to get in there? Right now we just have a 
TPM_MEASURE_ALGO constant that's either SHA1 for TPM 1.2 or SHA256 for TPM 
2.0... are you planning to make a bunch of Kconfigs to select this instead or 
something?

Basically, I understand that the log format *allows* multiple algorithms, and 
that's fine. And I'm also not saying that we can never expand it to allow 
logging multiple algorithms if a real need comes up in the future. I'm just 
saying there's no need to make things any more complicated than they need to be 
right now and implement support for a bunch of stuff in the lower level APIs 
that the higher level APIs wouldn't actually be using yet. If your goal for the 
time being is just to support the new log format, why don't you just do that in 
a way where tpm_extend_pcr() always creates a log entry with exactly one 
algorithm? If we ever get to the point where we actually need to log multiple 
algorithms somewhere we can still expand that later.


Cleanup #421: Change API of functions taking hash as an argument
https://ticket.coreboot.org/issues/421#change-1195

* Author: Krystian Hebel
* Status: New
* Priority: Normal
* Target version: none
* Start date: 2022-10-12

All existing functions that take a digest as an input assume that only one 
hashing algorithm is used at a time. Crypto agile format entry can (and should) 
log every used PCR bank in one entry for a given measurement. To make it work, 
some of the arguments must be changed, e.g.:

- pass number of algorithms used;
- instead of algorithm ID, pass a pointer to array of such IDs, with size equal 
to above;
- instead of hash, pass a pointer to array of hashes, with size and order as 
above.



-- 
You have received this notification because you have either subscribed to it, 
or are involved in it.
To change your notification preferences, please click here: 
https://ticket.coreboot.org/my/account
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] [coreboot - Cleanup #421] Change API of functions taking hash as an argument

[Julius Werner]

Issue #421 has been updated by Julius Werner.


Can you explain what use case you have that requires you to use multiple 
algorithms? And why is it not enough to just call tpm_extend_pcr() several 
times, once for each algorithm?

Let's clarify what your high-level goal here is first before we discuss how to 
modify low-level function APIs to enable it.


Cleanup #421: Change API of functions taking hash as an argument
https://ticket.coreboot.org/issues/421#change-1183

* Author: Krystian Hebel
* Status: New
* Priority: Normal
* Target version: none
* Start date: 2022-10-12

All existing functions that take a digest as an input assume that only one 
hashing algorithm is used at a time. Crypto agile format entry can (and should) 
log every used PCR bank in one entry for a given measurement. To make it work, 
some of the arguments must be changed, e.g.:

- pass number of algorithms used;
- instead of algorithm ID, pass a pointer to array of such IDs, with size equal 
to above;
- instead of hash, pass a pointer to array of hashes, with size and order as 
above.



-- 
You have received this notification because you have either subscribed to it, 
or are involved in it.
To change your notification preferences, please click here: 
https://ticket.coreboot.org/my/account
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] [coreboot - Support #387] Support Framework Laptop

[Julius Werner]

Issue #387 has been updated by Julius Werner.


Chromebooks never use BootGuard, so the firmware on those devices should be 
fully replaceable and they should support all the usual Chromebook developer 
features (e.g. 
https://chromium.googlesource.com/chromiumos/third_party/hdctools/+/HEAD/docs/ccd.md).


Support #387: Support Framework Laptop
https://ticket.coreboot.org/issues/387#change-1104

* Author: Jun Aruga
* Status: New
* Priority: Normal
* Category: board support
* Target version: none
* Start date: 2022-06-05
* Affected hardware: Framework Laptop

Dear coreboot developers,

I am a user of Framework Laptop[1][2]. Thank you for working to make coreboot 
work on Framework Laptop! This ticket is to track the task, as I didn't see any 
other issue tickets about Framework Laptop here. According to the Framework 
founder's comment[3] below, Framework provided Framework Laptops to the 
coreboot community.

> We've handed three systems that can boot unsigned bootloaders to folks in the 
> coreboot community. Our plan in the near term is to help them create a shim 
> loader that can be signed to run on any Framework Laptop, which then enables 
> anyone to do further coreboot development.

Then I saw Matthew's try to make the coreboot work on Framework Laptop,[4] but 
unfortunately it didn't work at that time.[5]

How is the current status? What prevents coreboot from working on the Framework 
Laptop? How can we, people in the Framework community, help you? As a 
reference, there is a coreboot specific thread on the Framework community 
forum.[6]

## References

* [1] https://frame.work/
* [2] Framework Computer - Wikipedia - 
https://en.wikipedia.org/wiki/Framework_Computer
* [3] Framework Laptop Mainboard, Hacker News, April 20, 2022 - 
https://news.ycombinator.com/item?id=31097434
* [4] Matthew tries to port Coreboot to the Framework laptop - February 27, 
2022 - https://www.youtube.com/watch?v=Jf_6xW-8tfQ
* [5] Matthew Garrett on Twitter, February 27, 2022 - 
https://twitter.com/mjg59/status/1497788538212917250
* [6] Free the EC! and Coreboot Only - 
https://community.frame.work/t/free-the-ec-and-coreboot-only/791

---Files
Emails_with_Framework_customer_support_about_coreboot.pdf (283 KB)


-- 
You have received this notification because you have either subscribed to it, 
or are involved in it.
To change your notification preferences, please click here: 
https://ticket.coreboot.org/my/account
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Can we make unit test failures more obvious in Gerrit?

[Julius Werner via coreboot]

Right now, when you have a CL that builds fine but fails unit tests,
Jenkins with Verified -1 it with a failure page like this
https://qa.coreboot.org/job/coreboot-gerrit/215770/ , which just says
"Test Result (no failures)". If you know your way around Jenkins you
can click on the "Console Output" link on the left and scroll down to
see the actual unit test errors, but most people who see this for the
first time will probably be very confused and have a hard chance
finding that on their own.

Is there any way we could make that more obvious and display the unit
tests as a real element among the test results (or at least put some
sort of "click Console Output to see errors" message on there)?
Unfortunately I have no idea how anything on that page is configured
or generated -- Patrick or Martin, I hope one of you guys would know
more?


smime.p7s
Description: S/MIME Cryptographic Signature
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: Measured boot and TPM standards

[Julius Werner]

This was just brought up in the biweekly meeting but it seemed like
none of us there really knew why +Philipp implemented it this way. My
guess(?) would be that they tried to follow the TPM 1.2 standard, but
maybe some mistakes crept in, and then I guess that code was just
reused for TPM 2.0 cases which maybe wasn't the right thing to do. I
don't know if he or 9elements have any specific reason to stick with
the existing format.

vboot itself isn't really involved with this... it provides some of
the low-level hashing and TPM access routines but the code defining
the TCPA format is separate. We do have that feature that PCR0 and
PCR1 are used to log some vboot-specific data, and we'd like to keep
that option available since we're using it for Chrome OS, but I'm fine
with making it configurable if other platforms want to use these PCRs
for their TCG standard uses instead.

On Wed, Jul 13, 2022 at 9:29 AM Krystian Hebel  wrote:
>
> TL;DR: why doesn't vboot follow existing standards and tries to create
> its own instead? [XKCD]
>
> During our adventures with enabling coreboot port for Talos 2 we got to
> the point of enabling TPM, and with it, measured boot. coreboot's part
> worked without too many issues other than those resulting from
> assumptions about endianness, Skiboot's (payload) - not so much, at
> least it seemed that way. Hardware access worked after adding a driver
> implementation for new module, but it failed to parse event log and in
> result disabled TPM.
>
> Skiboot (payload) expects Agile format [TPM20] of event log, while
> vboot provides its own format [VBOOT], compatible with neither Agile
> nor legacy format used by TPM 1.2 [TPM12]. Even worse, address of this
> log is written to TCPA ACPI table, from which it is read and parsed (or
> at least tried) by kernel as TPM 1.2 format [LINUX].
>
> At first I thought custom format was developed to add support for
> SHA-256, but only TPM 2.0 uses it, and it already has new format,
> capable of storing bigger hashes. Which brings up the question: why use
> own format that nothing else knows how to parse? Are there any reasons
> against switching to Agile format? AFAICT only cbmem can parse the
> current form.
>
> Another problem is PCR assignments: on one hand, [VBOOT] says that
> "coreboot uses the SHA-1 or SHA-256 hash algorithm depending on the
> _TPM specification_ for measurements", and then doesn't follow any
> existing specification, even though both families have predefined
> and mostly compatible with each other uses for PCRs (section 3.3.3 in
> [TPM12] and 3.3.4 in [TPM20]). Measuring GBB flags to PCR0 is dubious
> since PCR0 is usually used for code at or near RTM.
>
> Speaking of RTM, what the heck with those SRTM/DRTM definitions? When
> reading it, I got the feeling that there was misunderstanding and
> someone thought that S stands for software, D - discrete, although
> expansions of those abbreviations in the vboot documentation are
> correct. [DRTM] definitely isn't able to start from IBB, as is
> suggested in that documentation.
>
> We would like to work on mentioned issues, both implementation and
> documentation. Before we invest time in it, we would like to know
> whether there are any reasons that would require keeping current form,
> any caveats or design decisions we've missed.
>
> [XKCD]  https://xkcd.com/927/
> [TPM20]
> https://trustedcomputinggroup.org/wp-content/uploads/TCG_PCClient_PFP_r1p05_v23_pub.pdf
> [VBOOT]
> https://doc.coreboot.org/security/vboot/measured_boot.html#tcpa-eventlog
> [TPM12]
> https://trustedcomputinggroup.org/wp-content/uploads/TCG_PCClientImplementation_1-21_1_00.pdf
> [LINUX]
> https://github.com/torvalds/linux/blob/master/drivers/char/tpm/eventlog/acpi.c#L105
> [DRTM]
> https://trustedcomputinggroup.org/wp-content/uploads/TCG_D-RTM_Architecture_v1-0_Published_06172013.pdf
>
> --
> Krystian Hebel
> Firmware Engineer
> https://3mdeb.com | @3mdeb_com
>
> ___
> coreboot mailing list -- coreboot@coreboot.org
> To unsubscribe send an email to coreboot-le...@coreboot.org
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: QEMU x86 i440fx/piix4 build fails for >= 32MB ROMs - Assertion IS_HOST_SPACE_ADDRESS(host_space_address) failed

[Julius Werner]

I can see a little bug that makes this return a confusing error (it
should have really failed with `SPI flash address(0x300) not in any
mmap window!`), and we can fix that if you want. But that still won't
make this build (and my patch didn't cause the underlying problem,
before that it may have built an image but it probably wouldn't have
booted). By default cbfstool only expects the top 16MB of the flash to
be memory-mapped, so it cannot link XIP stages into areas outside of
that. The real solution is to either change your FMAP to put the
COREBOOT section into the top 16MB (we might want to change the
auto-generated default FMAP to do that), or pass
--ext-win-base/--ext-win-size options to cbfstool to tell it how to
map areas below the top 16MB.

On Thu, Jun 23, 2022 at 1:09 AM Paul Menzel  wrote:
>
> Dear Mike,
>
>
> Am 23.06.22 um 09:49 schrieb Mike Banon:
> > If I use a default config for i440fx/piix4, building a 16MB ROM works
> > fine, but 32MB or 64MB doesn't work anymore:
> >
> > ...
> >  CC postcar/southbridge/intel/common/rtc.o
> >  LINK   cbfs/fallback/postcar.debug
> >  OBJCOPYcbfs/fallback/romstage.elf
> >  CREATE build/mainboard/emulation/qemu-i440fx/cbfs-file.vqaXlP.out 
> > (from /home/my_custom_path_to/coreboot/.config)
> >  CC+STRIP   src/lib/cbfs_master_header.c
> >  OBJCOPYcbfs/fallback/bootblock.elf
> >  OBJCOPYbootblock.raw.elf
> >  OBJCOPYbootblock.raw.bin
> > Created CBFS (capacity = 33553892 bytes)
> >  BOOTBLOCK
> >  CBFS   cbfs_master_header
> >  CBFS   fallback/romstage
> > cbfstool: /home/my_custom_path_to/coreboot/util/cbfstool/cbfstool.c:1145: 
> > cbfstool_convert_mkstage: Assertion 
> > `IS_HOST_SPACE_ADDRESS(host_space_address)' failed.
> > make: *** [Makefile.inc:1116: build/coreboot.pre] Aborted
>
> Thank you for the report. It looks like a regression of commit
> 20ad36547e25 (cbfstool: Do host space address conversion earlier when
> adding files) [1].
>
> Building a 32 MB ROM also fails for emulation/qemu-q35
> (`CONFIG_BOARD_EMULATION_QEMU_X86_Q35=y`).
>
>
> Kind regards,
>
> Paul
>
>
> [1]: https://review.coreboot.org/c/coreboot/+/60018
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] RFC: Clarifying use of GCC extensions in the coding style

[Julius Werner]

Hi,

We occasionally get into discussions in code reviews when code uses a
GCC extension, and a reviewer asks that it be rewritten to be C
standard compliant instead. A recent example was on the use of `void
*` arithmetic in this patch
https://review.coreboot.org/c/coreboot/+/56791/9/src/soc/mediatek/common/pcie.c#109
, and there have been others in the past. I would like to come to a
consensus on this topic here and add some blurb to the coding style
document so we don't have to rehash the same discussion over and over
again.

In my opinion, GCC extensions are absolutely necessary for coreboot
development and there should be no reason to reject them. Inline
assembly is the most obvious example -- without it we would have to
convert a ton of static inline functions that wrap special
instructions into full linker-level functions in a separate assembly
file instead, and eat all the unnecessary function call overhead that
comes with that. Others enable such important features that it would
become much more dangerous and cumbersome to develop without them --
most importantly statement expressions
(https://gcc.gnu.org/onlinedocs/gcc-11.2.0/gcc/Statement-Exprs.html)
which are necessary to write things like double-evaluation safe
macros, expression-assertions like dead_code_t() and simple
convenience shortcuts like wait_us(). And some extensions just offer a
small bit of convenience to reduce boilerplate -- for example, `void
*` arithmetic just tends to be useful to prevent cluttering the code
with a bunch of unnecessary casts to other types that don't add any
additional meaning to the data (e.g. for an unspecified buffer of
opaque data I think `void *` is a much more appropriate type than
`uint8_t *`, even if I want to add a byte offset to it), and I've
never seen a case where I think it would have actually been unclear to
anyone what step width the pointer arithmetic was done at (there's no
reason to assume anything other than bytewise for a `void *`).

If we need some extensions anyway, and coreboot will never be "fully C
standards compliant" (not that that would actually be very useful for
anything in practice), I don't see a reason why we should still avoid
some extensions when we're using others. I think if an (official,
long-term supported) extension exists and it allows us to write better
code than standard C, there should be no reason not to use it. (Note
that for those people who are trying to get coreboot working with
clang, it generally supports all the same extensions as GCC.) I've
written a draft CL to add a section for this to the coding style,
please let me know what you think:
https://review.coreboot.org/c/coreboot/+/63660
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] libpayload CBFS API deprecation; please port payloads to new API

[Julius Werner]

The libpayload CBFS stack ("libcbfs") has recently been rewritten from
scratch in order to eliminate a bunch of very old warts and better
match the new CBFS API in coreboot
(https://review.coreboot.org/59497). For the time being, the old
libcbfs code remains side-by-side to the new one so that the old API
functions still work for existing payloads. However, we would like to
remove this code eventually as it still presents a maintenance burden.

All payloads that use the CBFS API in libpayload should please upgrade
to the new API. Among the payloads hooked up to the coreboot build
system, I think this only affects depthcharge (we already ported that
one), BOOTBOOT and FILO. See
https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/payloads/libpayload/include/cbfs.h
for available interfaces (they match the coreboot API which is
documented in more detail in
https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src/include/cbfs.h).
If you have a use case that cannot be implemented with this interface
please let me know.

If there are no objections I'd suggest that we target the official
deprecation and removal of the old API for the release after next,
i.e. 4.18 around the end of this year.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: Does PCI driver code belong in coreboot (ARM)?

[Julius Werner]

> But, currently, selecting Google Asurada in Kconfig (`make menuconfig`),
> display initialization cannot be disabled, and I do not see a way to
> disable USB init either.

That's a fair point, I think that's just not implemented because
nobody needed it yet. Display init is already globally guarded by the
display_init_required() function in src/lib/bootmode.c, so if anyone
wants to add a Kconfig in there that's easy to do. (Maybe it can be
tied to the NO_GFX_INIT option if we untangle how that interacts with
MAINBOARD_FORCE_NATIVE_VGA_INIT a bit.)

For USB, I think usually it just doesn't take any notable amount of
time so nobody has bothered to make it optional yet. But that could
certainly be done too if there was sufficient interest.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: Does PCI driver code belong in coreboot (ARM)?

[Julius Werner]

> If I remember correctly, coreboot’s goal to only do minimal hardware
> initialization originally meant, that the payload/OS does PCI
> initialization.

FWIW, coreboot does do device initialization for things that are only
needed by the payload in other cases too: we've been doing display and
USB initialization that way for years. This only works in those cases
where you need to do a lot of very platform-specific stuff to turn
something on, but then after that it presents a very simple generic
API (like a framebuffer or standardized host controller interface),
but I think PCI also falls in that area. I think it's useful so that
payloads don't all need to implement that super SoC-specific stuff
individually.

In general, I don't think we should be too strict about what coreboot
should or shouldn't be in cases where someone just wants to add an
optional feature that doesn't introduce a huge maintenance burden on
the core framework. If someone doesn't like it they can just disable
the Kconfig and do PCI init in the payload / the kernel / via node.js
or whatever instead. This has clearly been useful on x86 platforms for
years, so I don't see why Arm platforms shouldn't be allowed to do it
as well.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: [arm64] queries on current support and future direction

[Julius Werner]

> After some more community discussion about what's going on here, a proposal:

Can you link to that discussion? I feel like I'm missing most of
what's going on here.

In general, I can second what Patrick and Nico said: the notion of
running coreboot "after" BL2 doesn't make any sense, because coreboot
is supposed to be what BL1 and BL2 represent in the Trusted Firmware
boot stage model. coreboot is not some application that you squeeze in
somewhere in your existing firmware stack so you can put a  "coreboot
support" sticker on the box or whatever... coreboot is the core of the
firmware stack that does enough platform initialization to allow other
applications (what we call "payloads") to run. If you already have
other code (like a traditional TF-A BL1/BL2) that does all platform
initialization for you, there's no real point in you running coreboot.
If you want to run coreboot, it doesn't really make sense to have
another early platform initialization component in the same firmware
stack. (At most, some platforms will call into a separate proprietary
DDR initialization blob in the middle of coreboot's romstage for
platforms that aren't willing to open source their DDR init code...
but that should only be done in narrow cases where the
responsibilities of that blob are clearly bounded and most of the
platform initialization tasks are still handled in coreboot code, we
don't want coreboot to just be an empty wrapper for something else.)
In particular, I don't think having a "ramstage only" coreboot on an
Arm platform makes much sense because the ramstage tends to be pretty
empty on Arm platforms anyway (since they usually don't need to
enumerate a tree of PCI devices, which is the biggest part of the
ramstage on x86).

> What we could do to reduce your maintenance burden is to extend the coreboot 
> build system so that it can include sources from TF-A as part of its romstage 
> build. There'd be some work required to paper over the API differences, and 
> the end result (the coreboot-style romstage containing TF-A code) would be a 
> combined work under GPLv2.

Again I'm probably missing too much of the context here, but I'm not
sure this is a good direction either. Is the goal to have a platform
support both a normal Trusted Firmware BL2 and coreboot and somehow
reuse the same code for both? That would probably become a pretty big
mess in practice, similar to what we have in vendorcode on some
platforms. coreboot has a standard way of doing certain things (e.g.
the common APIs for GPIOs or SPI/I2C transfers), and you can't just
integrate some other bit of code unchanged into it if it would need to
use any of these functionalities that coreboot expects to be designed
in a certain way. If you try to write a coreboot platform port that
doesn't actually use any coreboot conventions or tie into any common
coreboot frameworks, you're pretty much losing all the benefits of
using coreboot in the first place. I could maybe see this work for
some very isolated subsystem (e.g. DDR init), but more than that and
you're gonna run into trouble.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: [SPECIFICATION RFC v3] The firmware and bootloader log specification

[Julius Werner]

> Since it doesn't seem possible to have each boot component using the same log
> format, we added a log_format and log_phys_addr fields to give flexibility in
> how logs are stored. An example of a different log format that can be used is
> the cbmem_console log format used by coreboot:

I am not exactly sure how you expect this interoperability you seem to
be suggesting here to work. Are you saying that your bf_log_header can
sometimes point to the bf_log_buffer structure you define, and
sometimes to a coreboot CBMEM console buffer? But that's a completely
different format that requires a different reader implementation, how
is that supposed to work? If this proposal is just "we define a
wrapper structure that points to everyone's custom firmware log
implementation", then I don't really see the point (the only benefit
still left then might be discovery of the log buffer, but that's the
part you leave open in your design while all those other
implementations already have working discovery mechanisms of their own
anyway).

For the other structures you have defined, the same feedback that I
think was already mentioned on the last iteration of this thread still
applies: it seems incredibly bloated for a simple firmware logging
mechanism. You have a whooping 24+n bytes of overhead *per line* which
probably comes out to somewhere between 30-50% of total space wasted
on overhead for the average log buffer. I guess there are just
fundamentally different opinions on how featureful a firmware log
mechanism needs to be so we're probably not gonna find a format that
makes everyone happy here, but at least for the coreboot project I see
little reason for us to implement something like this when we already
have a well-working existing solution with tooling and wideranged
support.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: Adding support for asynchronous operations

[Julius Werner]

Thanks, I think it looks pretty great! That solves exactly the
concerns I had with the other implementation.

On Mon, Jul 12, 2021 at 3:06 PM Raul Rangel  wrote:
>
> So I've spun a patch train that uses threads:
> https://review.coreboot.org/c/coreboot/+/56225/1
>
> Let me know what you think.
>
> Thanks!
>
> On Wed, Jul 7, 2021 at 7:24 PM Julius Werner  wrote:
> >
> > Pulling out some of the discussions that we already had inside Google
> > to this wider audience, I am strongly against parallelization
> > mechanisms that pull this much extra complexity into existing code
> > (particularly the already very complicated CBFS framework). I think
> > https://review.coreboot.org/c/coreboot/+/56049 is a perfect example of
> > the maintainability drawbacks of this approach, requiring to weave
> > large new chunks of code throughout the whole stack downwards, from
> > CBFS into the rdev layer and finally into the platform SPI driver,
> > just to be able to asynchronously execute a single read operation. The
> > CBFS patch isn't even complete, there will be more code needed to
> > support other primitives (e.g. cbfs_map()), decompression, etc. And
> > it's duplicating a lot of the existing flow from the synchronous code
> > in _cbfs_alloc().
> >
> > I think if we want a parallelization mechanism, we should converge
> > onto a single one that can solve a variety of current and future use
> > cases, and can be applied optionally where individual platforms need
> > it without having to grow tentacles into every other part of coreboot.
> > And I think the much better model for that would be cooperative
> > multithreading (e.g. setting up a second stack and execution context
> > and then having the CPU ping-pong back and forth between them while
> > the other one is idle and waiting on hardware somewhere). In fact
> > coreboot already has all this implemented in src/lib/threads.c
> > (CONFIG_COOP_MULTITASKING), although it seems that nobody has really
> > used it in a while.
> >
> > The clear advantage of cooperative multithreading is that almost none
> > of the code, especially not most of these in-between layers like CBFS
> > and rdev, need to be modified to support it. You can create a thread
> > and send it off to, say, begin loading the payload while the rest of
> > the boot state machine is still running platform initialization code,
> > and it can run through all the CBFS motions (including verification
> > and whatever other optional features are enabled) without any of those
> > having been written specifically with asynchronous operation in mind.
> > You still need to make sure that the two threads don't both try to
> > access shared resources at the same time, but you need to do that with
> > futures too. In the file loading case, it would probably be enough to
> > put a few critical sections in the SPI driver itself and there should
> > be no need to pull it up into all the platform-agnostic general
> > framework parts.
> >
> > The cost of cooperative multithreading is mostly just that you need
> > the space for a second stack. In ramstage that should be a given
> > anyway, and in romstage it can probably still be easily done on most
> > (particularly recent) platforms. For an optional performance
> > enhancement feature, I think that trade-off makes sense.
> >
> > On Wed, Jul 7, 2021 at 1:18 PM Raul Rangel  wrote:
> > >
> > > On Wed, Jul 7, 2021 at 12:49 PM Peter Stuge  wrote:
> > > >
> > > > Raul Rangel wrote:
> > > > > I'm currently working on improving the boot time for the AMD Cezanne
> > > > > platform.
> > > > ..
> > > > > Another difference between the latest AMD SoCs (Picasso, Cezanne), is
> > > > > that RAM is available in bootblock.
> > > >
> > > > As I have understood, the PSP has both trained RAM and copied firmware 
> > > > from
> > > > SPI to RAM when x86 comes out of reset.
> > > >
> > > > Is that accurate, false, or only partially accurate?
> > >
> > > It's partially accurate. The PSP will load bootblock into RAM. So
> > > coreboot still needs to access the SPI flash to load everything else.
> > >
> > > >
> > > > > One place where we spend a decent amount of time is reading from SPI
> > > > > flash. We have the SPI speed/modes set to the optimal settings for
> > > > > our platforms, but there is still room for improvement.
> > > >
> > > > Please provide numbers?
> > >
&

[coreboot] Re: Adding support for asynchronous operations

[Julius Werner]

Pulling out some of the discussions that we already had inside Google
to this wider audience, I am strongly against parallelization
mechanisms that pull this much extra complexity into existing code
(particularly the already very complicated CBFS framework). I think
https://review.coreboot.org/c/coreboot/+/56049 is a perfect example of
the maintainability drawbacks of this approach, requiring to weave
large new chunks of code throughout the whole stack downwards, from
CBFS into the rdev layer and finally into the platform SPI driver,
just to be able to asynchronously execute a single read operation. The
CBFS patch isn't even complete, there will be more code needed to
support other primitives (e.g. cbfs_map()), decompression, etc. And
it's duplicating a lot of the existing flow from the synchronous code
in _cbfs_alloc().

I think if we want a parallelization mechanism, we should converge
onto a single one that can solve a variety of current and future use
cases, and can be applied optionally where individual platforms need
it without having to grow tentacles into every other part of coreboot.
And I think the much better model for that would be cooperative
multithreading (e.g. setting up a second stack and execution context
and then having the CPU ping-pong back and forth between them while
the other one is idle and waiting on hardware somewhere). In fact
coreboot already has all this implemented in src/lib/threads.c
(CONFIG_COOP_MULTITASKING), although it seems that nobody has really
used it in a while.

The clear advantage of cooperative multithreading is that almost none
of the code, especially not most of these in-between layers like CBFS
and rdev, need to be modified to support it. You can create a thread
and send it off to, say, begin loading the payload while the rest of
the boot state machine is still running platform initialization code,
and it can run through all the CBFS motions (including verification
and whatever other optional features are enabled) without any of those
having been written specifically with asynchronous operation in mind.
You still need to make sure that the two threads don't both try to
access shared resources at the same time, but you need to do that with
futures too. In the file loading case, it would probably be enough to
put a few critical sections in the SPI driver itself and there should
be no need to pull it up into all the platform-agnostic general
framework parts.

The cost of cooperative multithreading is mostly just that you need
the space for a second stack. In ramstage that should be a given
anyway, and in romstage it can probably still be easily done on most
(particularly recent) platforms. For an optional performance
enhancement feature, I think that trade-off makes sense.

On Wed, Jul 7, 2021 at 1:18 PM Raul Rangel  wrote:
>
> On Wed, Jul 7, 2021 at 12:49 PM Peter Stuge  wrote:
> >
> > Raul Rangel wrote:
> > > I'm currently working on improving the boot time for the AMD Cezanne
> > > platform.
> > ..
> > > Another difference between the latest AMD SoCs (Picasso, Cezanne), is
> > > that RAM is available in bootblock.
> >
> > As I have understood, the PSP has both trained RAM and copied firmware from
> > SPI to RAM when x86 comes out of reset.
> >
> > Is that accurate, false, or only partially accurate?
>
> It's partially accurate. The PSP will load bootblock into RAM. So
> coreboot still needs to access the SPI flash to load everything else.
>
> >
> > > One place where we spend a decent amount of time is reading from SPI
> > > flash. We have the SPI speed/modes set to the optimal settings for
> > > our platforms, but there is still room for improvement.
> >
> > Please provide numbers?
>
> Sorry, that sentence didn't come out how I wanted. I was just saying
> that we could improve the boot time by exploring other avenues.
>
> >
> > > The question is, how do we model these asynchronous operations, how is
> > > data ownership handled, and how does the BSP know the operation is
> > > done?
> >
> > I haven't yet reveiewed your API proposal, but find it an absolutely
> > horrible idea to create a *general* API for asynchronous operations in
> > coreboot, because - as you recognize - it can easily be misused to great
> > detriment of the codebase, which already suffers chronically from such
> > trivial problems as copy-paste:itis. Don't do it.
> >
> > There is zero incentive for developers to improve the source beyond
> > making it work for their deadline; more complexity *will* create more
> > problems. (I understand that you have good intentions proposing this
> > change!)
>
> There has been a lot of work in refactoring the AMD codebases and
> fixing things throughout the stack. We have reduced a good amount of
> copy/paste when bringing up a new AMD SoC. So I don't agree that we
> are all writing abandonware. Please have a look at the proposal before
> making such hasty judgments.
>
> >
> > > I'm curious to see what the community thinks, and welcome any feedback.
> 

[coreboot] Re: cgit on review.coreboot.org

[Julius Werner]

> but as an immediate workaround, all our repos are mirrored ~hourly to 
> github.com/coreboot, too (and therefore are available through 
> raw.githubusercontent.com)

Oh, perfect! At least for my purposes that's good enough. Thanks!
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: cgit on review.coreboot.org

[Julius Werner]

> Hi Julius,
> Appending "?format=TEXT" to the file link in Gitiles (the "txt" link at the 
> bottom of the page) will give a base64-encoded copy of the file.

Yeah, I wish they just had a ?format=raw instead, I don't get why they
don't implement the most obvious option. Seems like they're not really
interested in doing that though
(https://github.com/google/gitiles/issues/106).
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: cgit on review.coreboot.org

[Julius Werner]

> why can't you use the gerrit 'download' button to either cherry pick the 
> patch or pull the stack?

I want to download the raw file, not the Git patch. I want to be able
to tell someone who needs this blob "just click this link to download
it". Right now I have to say "click this link to download a tarball
and then extract this file from it" which is a bit more annoying.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] cgit on review.coreboot.org

[Julius Werner]

Hi Patrick, Martin,

I just noticed that we seem to have dropped support for cgit on the
coreboot Gerrit. I don't exactly remember how it worked, but I seem to
have been able to get a link like
https://review.coreboot.org/cgit/qc_blobs.git/plain/sc7180/qtiseclib/libqtisec.a
some time last year. Now that just redirects to Gitiles.

The problem with Gitiles is it's impossible to just get a direct
download link to a file -- you can only see source files in the
browser view (with the Gitiles UI on the side), but you can't download
the raw file exactly as it is in the repo. This is especially
problematic for binary files where there is (as far as I can tell?
Maybe I just can't find it...) no way to get their contents at all
without downloading a whole tarball of a directory and extracting it
(which is pretty cumbersome).

Do you guys know why this happened and is there any chance we could
get cgit back?

Thanks,
Julius
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] arm-trusted-firmware mirror seems to have stopped syncing

[Julius Werner]

Hi Patrick, Martin,

The coreboot.org mirror of the arm-trusted-firmware repo
(https://review.coreboot.org/admin/repos/arm-trusted-firmware) seems
to be half a year out of date. According to the description (although
that may be out of date) it's still syncing from the old GitHub
location (https://github.com/ARM-software/arm-trusted-firmware.git).
The Trusted Firmware project switched a while ago to their own hosting
solution and I probably forgot to let you know. The new upstream
repository this should sync from is at:
https://review.trustedfirmware.org/TF-A/trusted-firmware-a

Then again, it looks like the GitHub location is still kept up to date
as a read-only mirror
(https://github.com/ARM-software/arm-trusted-firmware/commits/master),
so maybe this isn't the real source of the problem? Anyway, can you
please take a look and figure out how to fix the syncing? (Or is this
supposed to be done manually somehow?)

Thanks,
Julius
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: [RFC] How should we manage tree-wide changes?

[Julius Werner]

> > Rebasing for security seems odd. Usually one has to re-evaluate security
> > completely after a rebase. In my experience, security features randomly
> > break upstream like everything else. There is no stability guarantee.
>
> Maybe it is odd, but backporting Intel Boot Guard or vboot to old branch
> and supporting it there seem to be equally odd. I also had in mind
> security bug fixes, which also may be not easy to back port in light of
> missive tree changes and lack of QA to confirm things works in the same
> way. Of course in security bug fixes would be way easier to backport
> then features.

Well, okay, I don't think that's what anyone here meant when we said
"backport security fixes". I meant actual bug fixes, like there was a
missed size check leading to a potential buffer overflow somewhere --
that's something that you can relatively easily backport most of the
time. And for that, maintaining stable branches so not everybody has
to do the tracking and backporting on their own would be great (if
someone has the time to do it).

But of course you can't backport things like vboot or BootGuard
support to an older branch -- those are huge features that dig deep
into coreboot internals in many places. Those features are exactly the
kind of things that require these tree-wide API changes that this
discussion started about. So... I'm not really sure what you want
here, tbh. If you want to get all these big new features on your
board, then you should forward-port your out-of-tree patches to a
newer release, and you'll have to deal with the problems caused by all
the big API changes. If you don't want to deal with big API changes,
then you should keep your stuff on a stabilization branch and only
backport specific bug fixes that you need -- in that case, you'll of
course not get any big new features. I understand that you might like
to have both but I think that's just fundamentally impossible -- big
new features just tend to require deep, invasive changes.

> Do you place spatch in this category?
>
> I mean, do you see it as too burdensome to mandate that changes
> affecting the tree more than some TBD threshold are to be generated
> by an spatch which must also be contributed?

I think we could encourage that, I don't think it's really something
you can make a hard requirement. spatches just don't work well for all
kinds of API changes. Starting this as a sort of "experiment" like you
suggested to see how it goes sounds like a good idea.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: [RFC] How should we manage tree-wide changes?

[Julius Werner]

> > As an
> > open source project, coreboot doesn't have anywhere near the resources
> > to do enough QA to guarantee that the tip of the master branch (or any
> > branch or tag, for that matter) was stable enough to be shipped in a
> > product at any point in time... even Linux cannot do that, and they
> > have way more resources than we do. It's always best effort, and if
> > you want to put this on something you want to sell for money, you'll
> > have to pick a point where you take control of it (i.e. cut a branch)
> > and then do your own QA on that.
>
> This is what we doing right now.
>
> I wonder if community and leadership agrees with "setup your own QA"
> approach.
>
> We will advocate for improved and extended QA for coreboot and any other
> OSF, since without that working and doing business is nightmare simply
> blocking growth.

I mean, I don't think anyone here is going to argue against better QA,
it's just hard to do in practice. This is definitely not a burden you
can just push on developers -- coreboot supports hundreds of
mainboards and most of us only own a handful of those. It's just not
practically possible to make everyone who checks in a patch guarantee
that they don't break anyone else on any board with it. We all do our
best but accidents do and always will happen. The only way to get
consistently more confidence in the code base is through automated
systems, and those are expensive... we already have good build
testing, at least, and our recent unit test efforts should also help a
lot. But we don't have any real hardware testing other than those few
machines 9elements sponsored which only run after the patch is merged
(and which many committers don't pay attention to, I think). If you
want a big lab where every patch gets tested on every mainboard,
someone needs to set all that up and someone needs to pay for it. I'm
actually involved in a similar thing with the trustedfirmware.org
project right now who are in the process of setting up such a lab, and
I'm not sure if I'm allowed to share exact numbers, but you're quickly
in the range of thousands of dollars per mainboard per year just for
maintaining it (to say nothing of the upfront development cost).

> There are many reasons for rebasing or updating firmware to name few
> security and maintainability. Second case is interesting since, if you
> maintain 5 projects which are 4.12 based it is way different then
> maintain 4.0, 4.6, 4.9 etc.

> 3mdeb doing Linux maintenance for industrial embedded systems, so we can
> easily compare efforts related to coreboot and Linux maintanance. IMO
> Linux doing quite well and setting up stable, LTS and SLTS (10 years
> support) is huge win and clear understanding expansion of project to the
> realms where stability is key factor. Linux can be maintained way easier
> and came with more and more QA guarantees.

So, I actually get the feeling that what you really want is
well-maintained stable/LTS branches for coreboot releases (like Linux
has)? Because for security and bug fixes in a real product, always
rebasing onto master is just a bad idea in general. coreboot changes
all the time, features get added, changed, serialized data formats
differ... you really don't want to keep pushing all those changes onto
your finished product and figure out how they affect it every time.
You really just want to stick with what you have and only pull in
security and bug fixes as they come up, I think.

To that I would say: yeah, stable branches are great! It would be
really cool if we had them! The problem is just... someone has to step
up and do it. This is a volunteer project so generally things don't
get done unless someone who wants them to happen takes the time and
does it. Linux has stable branch maintainers who do a lot of work
pulling in all security/bugfix patches and backporting them as
necessary. If we want the same for coreboot, we'll need someone to
step up and do that job. Maybe patch contributors can help a bit --
e.g. in Linux, submitters add a `cc: stable` or `should be backported
up to 3.4` to their commit message, which then tells the stable branch
maintainers to pick that up. We could probably do something similar.
But we still need someone setting up and maintaining the branch first.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: [RFC] How should we manage tree-wide changes?

[Julius Werner]

Sorry for being a bit late here, but I wanted to second what Nico
said. It's important to not add undue burden to the development
process. I think the master branch is meant for development, not for
shipping long-term stable products. If you're installing coreboot in a
train or medical device, then why on Earth would you want to rebase
that onto the latest master after you have stabilized? Cut yourself a
private branch and cherry-pick fixes back to that as necessary. As an
open source project, coreboot doesn't have anywhere near the resources
to do enough QA to guarantee that the tip of the master branch (or any
branch or tag, for that matter) was stable enough to be shipped in a
product at any point in time... even Linux cannot do that, and they
have way more resources than we do. It's always best effort, and if
you want to put this on something you want to sell for money, you'll
have to pick a point where you take control of it (i.e. cut a branch)
and then do your own QA on that.

For the argument of supporting out-of-tree development, honestly, I
think coreboot is a GPL project and out-of-tree development is opposed
to the spirit of the GPL, if not the letter. The whole point of being
open source and copyleft is that we can all work together on one tree
and integrate with each others' changes in real time. If someone wants
to develop their own patches in a secret cave for a year and then dump
them all on the coreboot Gerrit all at once, and they can square that
away legally somehow, fine... we can't stop them. But I think the
extra friction caused by that is on them and we shouldn't make work
for mainline developers harder to support that case.

For the case mentioned with ACPI compatibility, I think it's a bit
different -- since coreboot versions can't be tied directly to OS
versions, there's value in trying to maintain some back and forwards
compatibility for the interfaces crossing that boundary, and I think
we generally try to do that where we can. We can try to codify that if
people want to. But I think it should be something that encourages
maintaining compatibility while still allowing for flexibility where
necessary... i.e. the guidelines should be written mostly with
"should" and not "must".

I'm okay with maintaining a "running log of major changes" as long as
it doesn't create too much of a hassle to maintain. coccinelle
spatches can be encouraged where it's useful but I think they should
always be optional... some migrations can be easily represented like
that but others not so much. And if I'm flipping the arguments in a
function that's only used 2 or 3 times in the whole tree, it's kind of
overkill to write an spatch.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: Chrome OS altfw questions (was: Can we get rid of SMMSTORE_IN_CBFS?)

[Julius Werner]

> The issue is that Tianocore fails to execute (hangs the system) when
> used as a legacy boot/alternative bootloader entry on 90% of CrOS
> devices which support the alternative bootloader feature, and since
> coreboot disables console output via the CPU UART, I don't have a good
> way to debug the issue (ie, no CCD output). The exact same Tianocore
> payload works as the primary/only payload with upstream coreboot on
> these platforms (all GeminiLake, Kabylake, Cometlake and probably
> other boards). The only boards it works on are some (but not all) AMD
> Stoneyridge boards (google/kahlee) and Intel Whiskeylake
> (google/sarien).

Well, if you want to track this down your best bet is probably to
recompile coreboot with serial output enabled. If you cannot reproduce
this with upstream, you can build from the respective Chromium OS
firmware branch for that device. Then you'll have exactly the same
code we build. (You could also try extracting the depthcharge binary
from a Chrome OS image and inserting it into an upstream coreboot
image you built, if you think it's a problem specifically with how
depthcharge loads the payload. But I think it's more likely to be a
difference in coreboot.)

I can also just send you Chromium firmware images with serial output
enabled for specific boards if that helps (we have those readily
available, we just don't have a system to make them public). Let me
know which ones you need.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Chrome OS altfw questions (was: Can we get rid of SMMSTORE_IN_CBFS?)

[Julius Werner via coreboot]

> Unrelated -- who can I talk to about fixing the state of launching
> something other than u-boot from the Alternative Bootloader Menu?
> Tianocore has only ever worked on a handful of devices, and the lack
> of console output on release firmware makes it difficult to debug.

I can try answering that -- let's fork this off into a new thread. Not
exactly sure what your question is, though? Is there some technical
problem with the altfw code that doesn't allow you to launch other
bootloaders? It should allow you to install and launch anything that
can run as a coreboot payload, and on more recent Chromebooks you
should be able to install those side-by-side with U-Boot and select on
each boot from the menu.

If you're asking whether Google will start pre-installing more
alternative bootloaders on Chromebooks, unfortunately I don't think
there are any plans to do that. We currently see the alternative
bootloader feature as an option to allow users to run their own code
on Chromebooks, and we really appreciate the work of people like you
in developing and maintaining custom builds for that -- but we don't
have the bandwidth to maintain alternative bootloaders ourselves for
each board. There's just not enough business incentive for Google to
invest that effort, basically. Maybe +Dossym can comment on what, if
anything, would need to happen for us to reconsider that position, but
I don't think there's a high chance (there are just not enough
Chromebook users that would care about these compared to the necessary
effort).

For console output, of course the alternative bootloader should do its
own console initialization and after that it shouldn't matter whether
coreboot set up a console anymore. If you want to see things that got
caught in depthcharge's exception handler before your console
initialization succeeded, one hacky workaround that should work is to
trigger the error, then soft-reset the machine (e.g. via the
Alt+VolUp+R key combination), then boot into normal Chrome OS
developer mode and read the last boot's persistent CBMEM console
output from /sys/firmware/log. (Of course you can also just recompile
coreboot and depthcharge with console output enabled if you prefer.)


smime.p7s
Description: S/MIME Cryptographic Signature
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Can we get rid of SMMSTORE_IN_CBFS?

[Julius Werner via coreboot]

As I'm trying to port all existing CBFS uses to the new APIs that
support verification, the CONFIG_SMMSTORE_IN_CBFS option is a bit of a
problem: It's the only CBFS use case where coreboot is actually trying
to write back into CBFS, and thus needs access to the raw flash
offsets of files (which is something I'm trying to stop from leaking
out of the abstraction).

Does anyone still need this? As far as I know it was just a hack
invented to backport SMMSTORE onto a Chromebook that couldn't make
FMAP changes anymore, and we never ended up using it after all. Anyone
still using SMMSTORE today should be putting it in a separate FMAP
section. Would anyone mind if I just remove the CBFS option?


smime.p7s
Description: S/MIME Cryptographic Signature
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: On handling vendorcode

[Julius Werner]

>> I think we still need to have a difference between hacky vendor stuff
>> and normal coreboot code. For example, the Eltan mboot stuff is
>> something we didn't really want to have in coreboot in that form, and
>> so they kinda put it in vendorcode as a compromise. We should make
>> sure it remains clear that that code isn't "proper" coreboot code and
>> didn't go through the same level of review.
>
> It might have started that way, but I don't think that's an accurate 
> portrayal of eltan's work at this point:
> The eltan code uses vboot for the cryptographic primitives these days and as 
> far as I can see, extends it for measured boot - which vboot itself doesn't 
> do, ever.

No, that wasn't the only problem with that code. It is implementing
things that we already have an official solution for and doing that in
ways that aren't really safe and hinder development on core
components. For example, this is the only user of prog_locate_hook(),
a function that doesn't even really make sense anymore because I
recently removed the prog_locate() function it was based on, and that
was always a bad design to begin with because it fundamentally
requires the same data to be loaded more than once to do the stuff
that Eltan's mboot is trying to do with it (which is both inefficient
and an opening for TOCTOU attacks).

We already have a canonical verification solution (CONFIG_VBOOT) and a
canonical measurement solution (CONFIG_TPM_MEASURED_BOOT), and we
should be aiming to make those flexible enough to solve everyone's use
case rather than have everyone implement the same thing slightly
differently. It's much better to have one flexible, well-maintained
solution than a dozen different, poorly-maintained solutions. Those
problems I mentioned about double loading and TOCTOU safety can all be
solved, and I'm aiming to do that with the CBFS verification work I'm
currently doing, but it's complicated and it requires integration into
CBFS internals. I only want to do and maintain that once and not once
for every different vendor implementation. And doing development on
this stuff is a lot easier when you don't need to maintain weird,
badly designed hooks that someone once crammed in there to support a
one-off solution.

Eltan already signaled they were going to deprecate the mboot stuff
for future platforms anyway (see
https://review.coreboot.org/c/coreboot/+/38590/comment/7f384f61_597e79c9/),
so I'm hoping we can eventually get rid of it (and just drag it along
until then by hiding it in vendorcode and trying to maintain
prog_locate_hook() with minimal effort). In the future, we will
hopefully have good and flexible measurement and verification
solutions ready to use (IIRC this was one of the reasons Eltan
implemented the mboot stuff in the first place, because Philipp's
measurement stuff wasn't quite finished back then yet), and then
nobody will feel the need to implement their own thing again.

> Also, regarding your point on gerrit (collecting arguments in this thread) 
> that we don't have duplicate things, look no further than graphics init:
> - src/device/oprom/realmode
> - src/device/oprom/yabel
> - src/drivers/intel/gma
> - FSP can do the graphics init, too (and report it in cbtables)

Don't most of those target different hardware generations? Then I
don't really think that's comparable... of course if we're trying to
support hardware with widely different graphics interfaces that may
result in different driver code. But for pure, hardware-independent
feature stuff like verification or measurement, I think we should aim
to converge into a single implementation that can support everyone's
use cases. We don't support two different kinds of timestamp tables or
two dynamic persistent memory allocators (CBMEM) either. (Also, like I
mentioned the problem with this security stuff is just that it needs
to be so tightly integrated with the CBFS stack. In a different, more
isolated area I might consider this less of a problem, but the Eltan
mboot stuff has actually been causing practical problems a couple of
times already, in ways that we mostly already pointed out as concerns
when it was added. I'd prefer to get rid of it long-term, not make it
any more official.)
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: On handling vendorcode

[Julius Werner]

> Any objections to moving the code out there that has no other upstream (e.g. 
> src/vendorcode/google/chromeos or src/vendorcode/eltan, I think?) while 
> moving in code from elsewhere in the tree that fits the "it has a foreign 
> upstream" description (e.g. the lzma library)?

Sorry, I just responded to this on
https://review.coreboot.org/c/coreboot/+/51827 before I saw this mail.

I think we still need to have a difference between hacky vendor stuff
and normal coreboot code. For example, the Eltan mboot stuff is
something we didn't really want to have in coreboot in that form, and
so they kinda put it in vendorcode as a compromise. We should make
sure it remains clear that that code isn't "proper" coreboot code and
didn't go through the same level of review.

If you want to separate the two cases, maybe just make a
src/vendorcode/mirrored/ subdirectory and put the stuff that was
mirrored from a different upstream under there?
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: coding style discussions, again.

[Julius Werner]

> > https://review.coreboot.org/c/coreboot/+/51825 proposes getting rid of the 
> > rule to make if-statement blocks (and the like) as short as possible.
> > The rationale is to encourage a style that avoids subtle bugs which then 
> > need to be found by tooling such as Coverity Scan and fixed by commits like 
> > https://review.coreboot.org/51786.

My main case is really just that I think the whole premise is
incorrect: these kinds of errors can be found by checkpatch, at upload
time. There should be no risk of them slipping into the code and thus
we don't need to add any inconvenience for humans to prevent something
that can already be automatically be prevented by a computer.

I've double-checked that now and realized that checkpatch currently
doesn't look for this like I thought it did. But there is a fix, and
it works very well (I've found no real false-positives in the whole
coreboot tree): https://review.coreboot.org/c/coreboot/+/51838. I am
also (re-)submitting this to Linux so hopefully we wouldn't be out of
sync for too long if we want to apply it immediately.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: Running QEMU targets in Jenkins

[Julius Werner]

>
> https://qa.coreboot.org/job/coreboot-boot-test/ sends off ToT builds to
> 9esec's Lava system where they are run on some virtual and real devices.
> See for example the comments to
> https://review.coreboot.org/c/coreboot/+/51189 where Lava reports passing
> on 5 qemu configs and 3 real devices.
>
> 
>
Right, but that's different... those tests only run on patches after they
are merged (and IIRC they do get broken occasionally and I don't think many
people pay attention to them. When I see those broken on one of my patches
I tend to just assume there was an existing breakage in the tree already).

I understand that there may be good reasons why we can't run the real
hardware tests on every patch set and make them block Verified+1. But for
QEMU that shouldn't be too hard? We already build images for the emulation
boards anyway, running QEMU to boot them just takes a second, and it
shouldn't require anything that isn't already available on the machines
that do all the compiling. Why can't we do that as part of that step
already?
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Running QEMU targets in Jenkins

[Julius Werner]

I'm just curious... have we ever considered booting some of our QEMU
targets as part of the Jenkins CI? I know they don't do a lot but they
do cover some stuff (e.g. CBFS). I randomly happened to boot one of my
in-flight patch trains on qemu-i440fx recently and discovered that I
accidentally broke rmodule loading. Would be nice if Jenkins could
just do that for you automatically.

Just wanted to know whether this had been discussed before and people
have come up with good reasons not to do it, or if it's just a matter
of nobody had time to implement it yet. (And if someone wanted to
implement it, what would be the best hook point? Put it into abuild?)
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] [RFC] Coding Style: Chain-includes, style questions not defined in the guide and cleanup patches

[Julius Werner]

Hi,

I recently landed a coding style change to define some rules about how
#include directives should be written and when it's okay to
"chain-include" a header, because this has been a frequent topic of
discussion and cleanup patches in the past year(s) and people seemed
to keep having different opinions about it. The patch caused a bit
more of a stir than I anticipated because not everyone saw it before
going in -- apologies for not circulating it more widely before. If
you are interested in the topic, please join the discussion on
https://review.coreboot.org/50247 and I'll try to write a fixup patch
for it once we can clarify some consensus (contention seems to be
mostly about whether headers should be alphabetized or not right now).

However, in the course of that we also got into a more general
discussion about "cleanup patches" and when it should be okay to write
them (because sometimes people try to "clean up" things that other
people intentionally wrote that way and that tends to cause some
contention). I've uploaded a new coding style change about this as
https://review.coreboot.org/c/coreboot/+/50966 which is trying to
define some ground rules for this (and for how the style guide should
be applied in general). The gist of it is that I think "cleanup
patches" should focus on the things that are actually defined in the
coding style, and for the things that aren't we should generally aim
to honor the original author's intentions and not "bulk clean"
existing files. If there are things in existing files that we think
are worth cleaning up, we should first find some consensus about what
is wrong with them and put a rule about that in the coding style, so
that different people don't keep "cleaning up" the existing codebase
to their personal preferences which keeps pulling it in different
directions.

If you're interested in this topic please join the discussion on
Gerrit and let me know what you think.

Thanks,
Julius
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: "Fixing" `1 << 31` (technically undefined behavior with known implementation-specific results)

[Julius Werner]

I'm someone who really doesn't like it. The U in 1U << 31 is
unnecessary visual clutter that really makes the number harder to read
(I guess 1u is slightly better but not by that much). I think we
should define our code style first and foremost to make code easily
written and easily read (with no ambiguity and no risk for issues).
This isn't a "real" issue in any way -- both GCC and clang handle this
the right way, and they even intentionally provide warning switches to
allow this (-Wshift-overflow=1 vs -Wshift-overflow=2). So this is
perfectly well supported and there's no indication it ever wouldn't in
the future. The only thing that could be said against it is that it's
not official standard C, and to that I would say that the official C
standard is just dumb and incomplete in general. We already frequently
use plenty of GCC extensions throughout coreboot, because the
equivalent code can either not be written or not be written as cleanly
or safely in fully standard-compliant C. In my book, cleanliness and
readability always win against just doing what the standard says for
the standard's sake.

I am okay with BIT(), I think that's a fair alternative and I wouldn't
mind existing cases being rewritten if someone really wants to do
that. But that still leaves the problem of 3 << 30 and things like
that, so I don't think it's a full solution. (You can't really expand
the macro to the multi-bit case without defining what's essentially
just a wrapper macro for the shift operator, and at that point I don't
think it makes sense anymore.)

One example of things that happen when you insist on 1U is that
constants cannot be shared between C and assembly code anymore
(because the assembler doesn't support the 1U syntax). If you wanna
solve this you need to define a macro that either does or doesn't add
the U based on whether it's being built for assembler or C (and I
think even that doesn't work for inline assembly?), and then you have
something like U(1) << 31 everywhere. You can see how that looks in
the Trusted Firmware project which does it that way (e.g.
https://review.trustedfirmware.org/plugins/gitiles/TF-A/trusted-firmware-a/+/refs/heads/master/include/arch/aarch32/arch.h#126)...
if you ask me it's ugly, harder to read and unnecessarily confusing to
new contributors.

(Also, slight tangent but maybe this is a good opportunity to shill
the new DEFINE_BITFIELD/SET32_BITFIELDS() API we added a year ago:
https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src/include/device/mmio.h#60
? By defining bit fields semantically you don't have to deal with raw
shifts at all anymore, which is both safer and in my opinion more
readable. It already has unsigned casts wrapped in the macros so
there's no reason to add a U manually anywhere there anymore.)
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: Master currently broken on Thinkpad X230 when using option table

[Julius Werner]

Hi Prasun,

So just to understand your setup correctly, you're saying that you are
using vboot in an RW-A/B configuration -- but your RO image is some
old coreboot version, and you just updated the RW-A to the newest one?
Unfortunately, that's not how vboot can work -- when building coreboot
master, you always need to use RO and RW-A/B from the same commit.
There are numerous interaction points between the RO and RW stages
(e.g. the memory layout in CAR) that need to fit together perfectly to
make it all work. We make changes to coreboot that are incompatible
with older RO versions all the time (my patch series added a CBFS
mcache area to the CAR layout, that's probably what's causing your
issue), if you have been running this configuration for a while it was
just pure luck that you didn't see it fail yet.

vboot was designed so you would create a branch when you build your RO
version, and then the RW-A/B should only be updated with bug fixes
built from that branch where you only cherry-pick in patches to solve
specific small issues, and you need to carefully make sure that none
of those patches touch the RO/RW interface. It's not designed to just
keep updating the RW part to the newest master. We want to keep
developing across all stages for master so unfortunately that's really
the only way it can work. (I guess we could start having official
RW-update branches for each coreboot release tag to pick in RW-safe
fixes and small features... I think there just hasn't been enough
interest in upstream vboot to do that for now.)

On Thu, Dec 10, 2020 at 2:24 AM Prasun Gera  wrote:
>
> This is still broken for my vboot setup on Thinkpad T530. I have pulled in 
> the most recent fixes (i.e., https://review.coreboot.org/48482). I flashed to 
> RW_A, which does not boot. My old build on RO still works fine. The RO build 
> is a few months old. On IRC, one suggestion was to try with NO_CBFS_MCACHE, 
> which does work. That is, building with NO_CBFS_MCACHE and flashing to RW_A 
> works. Any ideas ? It's quite difficult to open this laptop. So I would like 
> to not touch RO if possible.
>
> My defconfig:
> # CONFIG_USE_BLOBS is not set
> CONFIG_VENDOR_LENOVO=y
> CONFIG_FMDFILE="src/mainboard/lenovo/t530/vboot-rwab.fmd"
> CONFIG_VBOOT=y
> CONFIG_HAVE_IFD_BIN=y
> CONFIG_BOARD_LENOVO_T530=y
> CONFIG_PCIEXP_L1_SUB_STATE=y
> CONFIG_PCIEXP_CLK_PM=y
> CONFIG_H8_SUPPORT_BT_ON_WIFI=y
> CONFIG_H8_FN_KEY_AS_VBOOT_RECOVERY_SW=y
> CONFIG_HAVE_ME_BIN=y
> CONFIG_CHECK_ME=y
> CONFIG_USE_ME_CLEANER=y
> CONFIG_HAVE_GBE_BIN=y
> CONFIG_DRIVERS_PS2_KEYBOARD=y
> CONFIG_COREINFO_SECONDARY_PAYLOAD=y
> CONFIG_MEMTEST_SECONDARY_PAYLOAD=y
> CONFIG_NVRAMCUI_SECONDARY_PAYLOAD=y
> CONFIG_TINT_SECONDARY_PAYLOAD=y
> CONFIG_MEMTEST_MASTER=y
>
>
> On Mon, Dec 7, 2020 at 3:48 PM Julius Werner  wrote:
>>
>> Sorry for the breakage and thanks for narrowing down the issue. I
>> think Arthur had also just figured out the same problem and uploaded a
>> quick fix here: https://review.coreboot.org/48407
>>
>> On Sun, Dec 6, 2020 at 11:09 PM Iru Cai  wrote:
>> >
>> > By using gdb, I can debug on QEMU. I can see in bootblock, romstage and 
>> > postcar,
>> > when USE_OPTION_TABLE is set, the debug_level option is always read, so 
>> > there
>> > is a cbfs_map_ro() in each stage. The buggy thing is in postcar stage, the 
>> > cbfs mcache
>> > cannot be found, so its size becomes zero, then all the files in the cbfs 
>> > cannot be loaded
>> > because of the mcache overflow, which results in failing to load the 
>> > ramstage.
>> >
>> > On Mon, Dec 7, 2020 at 12:18 PM Iru Cai  wrote:
>> >>
>> >> Confirmed on qemu-i440fx. It's strange that it already has different
>> >> behavior in romstage between setting and not setting
>> >> USE_OPTION_TABLE. I still don't know what is broken in this commit.
>> >>
>> >> On Sun, Dec 06, 2020 at 11:24:11PM +0100, Merlin Büge wrote:
>> >> >
>> >> > 9d0cc2aea9 cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()
>> >> > https://review.coreboot.org/c/coreboot/+/39306
>> >> >
>> >
>> >
>> >
>> > --
>> > My website: https://vimacs.lcpu.club
>> >
>> > Please do not send me Microsoft Office/Apple iWork documents. Send 
>> > OpenDocument instead! http://fsf.org/campaigns/opendocument/
>> ___
>> coreboot mailing list -- coreboot@coreboot.org
>> To unsubscribe send an email to coreboot-le...@coreboot.org
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: Master currently broken on Thinkpad X230 when using option table

[Julius Werner]

Sorry for the breakage and thanks for narrowing down the issue. I
think Arthur had also just figured out the same problem and uploaded a
quick fix here: https://review.coreboot.org/48407

On Sun, Dec 6, 2020 at 11:09 PM Iru Cai  wrote:
>
> By using gdb, I can debug on QEMU. I can see in bootblock, romstage and 
> postcar,
> when USE_OPTION_TABLE is set, the debug_level option is always read, so there
> is a cbfs_map_ro() in each stage. The buggy thing is in postcar stage, the 
> cbfs mcache
> cannot be found, so its size becomes zero, then all the files in the cbfs 
> cannot be loaded
> because of the mcache overflow, which results in failing to load the ramstage.
>
> On Mon, Dec 7, 2020 at 12:18 PM Iru Cai  wrote:
>>
>> Confirmed on qemu-i440fx. It's strange that it already has different
>> behavior in romstage between setting and not setting
>> USE_OPTION_TABLE. I still don't know what is broken in this commit.
>>
>> On Sun, Dec 06, 2020 at 11:24:11PM +0100, Merlin Büge wrote:
>> >
>> > 9d0cc2aea9 cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()
>> > https://review.coreboot.org/c/coreboot/+/39306
>> >
>
>
>
> --
> My website: https://vimacs.lcpu.club
>
> Please do not send me Microsoft Office/Apple iWork documents. Send 
> OpenDocument instead! http://fsf.org/campaigns/opendocument/
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: [SPECIFICATION RFC] The firmware and bootloader log specification

[Julius Werner]

Standardizing in-memory logging sounds like an interesting idea,
especially with regards to components that can run on top of different
firmware stacks (things like GRUB or TF-A). But I would be a bit wary
of creating a "new standard to rule them all" and then expecting all
projects to switch what they have over to that. I think we all know
https://xkcd.com/927/.

Have you looked around and evaluated existing solutions that already
have some proliferation first? I think it would be much easier to
convince people to standardize on something that already has existing
users and drivers available in multiple projects.

In coreboot we're using a very simple character ring buffer that only
has two 4-byte header fields: total size and cursor (i.e. current
position where you would write the next character). The top 4 bits of
the cursor field are reserved for flags, one of which is the
"overflow" flag that tells you whether the ring-buffer already
overflowed or not (so readers know whether to print the whole ring
buffer, or only from the start to the current cursor). We try to
dimension the buffers so they don't overflow on a single boot, but
this approach allows us to log multiple boots on platforms that can
persist memory across reboots, which sometimes comes in handy.

The disadvantages of that approach compared to your proposal are lack
of some features, like the facilities field (although one can still
just print a tag like "<0>" or "<4>" behind each newline) or
timestamps (coreboot instead has separate timestamp logging). But I
think a really big advantage is size: in early firmware environments
before DDR training, space is often very precious and we struggle to
find more than a couple of kilobytes for the log buffer. If I look at
the structure you proposed, that's already 24 bytes of overhead per
individual message. If we were hooking that up to our normal printk()
facility in coreboot (such that each invocation creates a new message
header), that would probably waste about a third of the whole log
buffer on overhead. I think a complicated, syslog-style logging
structure that stores individual message blocks instead of a
continuous character string isn't really suitable for firmware
logging.

FWIW the coreboot console has existing support in Linux
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/firmware/google/memconsole-coreboot.c),
SeaBIOS 
(https://github.com/coreboot/seabios/blob/master/src/fw/coreboot.c#L219),
TF-A 
(https://github.com/ARM-software/arm-trusted-firmware/blob/master/drivers/coreboot/cbmem_console/aarch64/cbmem_console.S),
GRUB 
(https://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/term/i386/coreboot/cbmemc.c),
U-Boot 
(https://github.com/u-boot/u-boot/blob/master/drivers/misc/cbmem_console.c)
and probably a couple of others I'm not aware of. And the code to add
support (especially when only appending) is so simple that it just
takes a couple of lines to implement (binary code size to implement
the format is also always a concern for firmware environments).

On Wed, Nov 18, 2020 at 7:04 AM Heinrich Schuchardt  wrote:
>
> On 14.11.20 00:52, Daniel Kiper wrote:
> > Hey,
> >
> > This is next attempt to create firmware and bootloader log specification.
> > Due to high interest among industry it is an extension to the initial
> > bootloader log only specification. It takes into the account most of the
> > comments which I got up until now.
> >
> > The goal is to pass all logs produced by various boot components to the
> > running OS. The OS kernel should expose these logs to the user space
> > and/or process them internally if needed. The content of these logs
> > should be human readable. However, they should also contain the
> > information which allows admins to do e.g. boot time analysis.
> >
> > The log specification should be as much as possible platform agnostic
> > and self contained. The final version of this spec should be merged into
> > existing specifications, e.g. UEFI, ACPI, Multiboot2, or be a standalone
> > spec, e.g. as a part of OASIS Standards. The former seems better but is
> > not perfect too...
> >
> > Here is the description (pseudocode) of the structures which will be
> > used to store the log data.
>
> Hello Daniel,
>
> thanks for your suggestion which makes good sense to me.
>
> Why can't we simply use the message format defined in "The Syslog
> Protocol", https://tools.ietf.org/html/rfc5424?
>
> >
> >   struct bf_log
> >   {
> > uint32_t   version;
> > char   producer[64];
> > uint64_t   flags;
> > uint64_t   next_bf_log_addr;
> > uint32_t   next_msg_off;
> > bf_log_msg msgs[];
>
> As bf_log_msg is does not have defined length msgs[] cannot be an array.
>
> >   }
> >
> >   struct bf_log_msg
> >   {
> > uint32_t size;
> > uint64_t ts_nsec;
> > uint32_t level;
> > uint32_t facility;
> > uint32_t msg_off;
> > char strings[];
> >   }
> >
> > The members of struct bf_log:
> >   

[coreboot] FYI: CBFS metadata caching landed, let me know if something breaks

[Julius Werner]

I just landed a somewhat large-ish feature that enables CBFS metadata
caching across most boards (this will avoid reloading CBFS file
headers from flash on every file lookup and ultimately increase boot
speed): https://review.coreboot.org/c/coreboot/+/38424

I wrote and tested it best as I could, but with the pandemic my access
to different test platforms is limited. If anyone sees their platform
break starting with this patch, please let me know, and if you need a
quick fix just add `select NO_CBFS_MCACHE` to the respective
board/platform Kconfig (this effectively disables the feature but
without having to revert the whole patch and potential dependencies
back out).
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: System gcc requirements

[Julius Werner]

> > > Having an undocumented (or silently installed, therefore unexpected)
> > > dependency is undesirable (especially for a firmware), no question
> > > about that.
> >
> > Sorry, I still get the impression that there is a fundamental
> > misunderstanding about what Git submodules are here.
>
> I know how submodules work, I believe Nico too. I also know the
> internal git data model very well.

Sorry, there were multiple streams of discussion in here, I just want
to make sure we're all on the same page on what it actually is. I felt
the characterization of "silently installed" that I quoted made it
sound like something else.

> > the Git SHA of the submodule HEAD is stored in the main coreboot repo.
>
> My argument is solely on complexity, but please don't trust that hash too 
> much.

I was trying to say that it is just as secure and trustworthy as code
stored in the main Git repository. I don't think we need to get into
the details of SHA1 collision resistance here, it's just how Git works
right now for everything it does (submodules and normal files). I just
wanted to clarify that there should be no security (or availability)
concerns with this "dependency" because that was mentioned somewhere
above.

> Sure, but more source code and in particular across more repositories
> is a lot more complexity than less source code in a single repository.

I mean, the specific functions that cbfstool uses from vboot are not
very complex at all (and since it's a submodule they're right there,
you can just look at them, they're just stored under 3rdparty/ instead
of under src/). And the main reason we're doing this is to avoid
reimplementing the same thing in coreboot again, which would add a lot
more complexity to builds that are using vboot for verification -- not
only would you have the same algorithm twice, there would also likely
be API differences (e.g. how hashes are handled or hash types are
encoded) that then require complicated translation whenever coreboot
and vboot parts try to talk to each other. You can basically: a) have
a coreboot hash API and a vboot hash API, and coreboot code calling
into vboot will need to do a lot of translation, or b) have a coreboot
hash API for other stuff but use the vboot API for vboot-specific
coreboot code, meaning that we'd confusingly use two separate hash
APIs within coreboot (and sometimes they may still need to interwork
and require translation), or c) just make the vboot API available to
all of coreboot and use it everywhere. I really think that last one
ends up being the least complex and confusing, overall.

> > And if they do, and you can tell me what it is, we are happy to
> > apply the same standard to vboot going forward.
>
> I think that's a fantastic promise! I also understand and agree
> with your request for standardization/documentation, something to
> set expectations, that's 100% reasonable.

Like I said in my first mail here, I'm really not trying to make this
hard for you! I'm really trying to keep things smooth and problem-free
both for the people who do and especially also for those who don't
want to use vboot stuff. Just please let me find other ways to do that
rather than having to hide every small use of utility functions behind
a hard switch to turn it off, because in practice that leads to a lot
of detail problems that make things very hard to implement and
maintain. We should be able to integrate this such that it will just
build and link quietly in the background without causing anyone any
troubles, same as all of the other support code that's in the main
coreboot repository. This thread was the first time I heard that the
__attribute__((fallthrough)) is causing a lot of people pain, and
that's an absolute non-issue to fix... just give me a day or two and
it'll be gone. If there are more issues causing pain with any specific
compiler or build environment, let me know and I'll try to help.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: System gcc requirements

[Julius Werner]

> Having an undocumented (or silently installed, therefore unexpected) 
> dependency is undesirable (especially for a firmware), no question about that.

Sorry, I still get the impression that there is a fundamental
misunderstanding about what Git submodules are here. It's *just*
source code! It's source code that comes cryptographically guaranteed
from the main coreboot Git repository, exactly like all the normal
coreboot source code. Nothing is "silently installed" on your system
here or anything. It's just a way to organize some source code as a
separate unit that can manually be resynced with changes from another
upstream. It's not really different from what we did for LZ4
compression code in cbfstool, except that it makes organization and
regular updates easier than copying in the whole thing directly. And I
don't know what you mean by "undocumented", we have more documentation
on the vboot stuff than most parts of coreboot (see
Documentation/security/vboot... also, there's
Documentation/getting_started/submodules.md).

> Can you guarantee that a silently installed submodule's repo won't get hacked 
> and replaced with malicious code for example? We have seen that happening 
> with other repos already (github, sourceforge and other webhosts too). The 
> fewer dependency you have, the less are the chances for a vulnerability or 
> sechole, simple as that.

Yes, because the Git SHA of the submodule HEAD is stored in the main
coreboot repo. I explained this in my last mail already. This is not
node.js, if the submodule changes externally that will not affect
coreboot's use of it until a coreboot developer intentionally pushes
an update to the coreboot repository itself.

> Julius also mentioned elsewhere that the same issues that can sneak into 
> vboot can hit us anywhere else. That's generally true but, IMHO, highly 
> unlikely. When we work on central parts of the build process (e.g. cbfstool) 
> and review patches there, we take care of portability and keep it to standard 
> C, FWIW. (I know people like to use packed structs, but that's not too funky, 
> I guess.)

No, I don't think most people submitting to cbfstool are somehow
applying some magic unwritten portability standard that vboot is
lacking to be honest. And if they do, and you can tell me what it is,
we are happy to apply the same standard to vboot going forward. Like I
said earlier, I am perfectly happy to align vboot standards and
practices to coreboot to solve this kind of interworking friction, the
problem is just that *we don't have any standards*! I mean, it's not
like we somehow blindly added __attribute__((fallback)) to vboot
thinking it was standard C89, we were fully aware that this was a
somewhat modern feature in GCC and clang, and everyone we talked to
back then (I think this was on Gerrit somewhere, not sure how to find
it right now, sorry) agreed that this was fine and that aligning
HOSTCC requirements to crossgcc made sense.

Since apparently enough people here feel that it's a big problem all
of a sudden (even though I think it landed close to a year ago) I'm
going to take it out again now and then hopefully we can move on. And
if people are concerned about getting hit by problems like this in the
future (both in submodules and normal coreboot code) it would be great
if someone could just put something in Documentation/ to define what
we're intending to support, so we don't need to have discussions again
about what some but clearly not all people might maybe be looking out
for in reviews if they happen to spot it. I don't think "just write it
fully portable" is a reasonable goal since clearly we need
__attributes__ like ((packed)), and there are other common extensions
without which writing C code is just a huge pain in general (e.g.
compound statements for double-evaluation safe MIN()/MAX()), so I'd
prefer if we could just pick a minimum GCC and clang version number.
And then maybe one day we could even get Jenkins to test that.

(Also, while we're on the subject of submodules causing pain, Nico...
whenever I want to build test older Intel generations I have to first
figure out again which of them don't rely on libgfxinit, or how to
hack around in their Kconfigs to disable it, because unlike everything
else you need to build coreboot there seems to be no way to get an ADA
toolchain from crossgcc. All the problems we have been discussing in
this thread can be worked around easily (for 99% of people's host
machines, at least) by putting a simple
CC=util/crossgcc/xgcc/bin/x86_64-elf-gcc on the command line, but if
you try to build in an environment that doesn't bring its own ADA
compiler you're just SOL. So I really don't think vboot deserves the
award for most cumbersome submodule right now.)
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: System gcc requirements

[Julius Werner]

> One is what you describe - a generic utility supporting everything
> that gets installed into say /usr/local/bin for lots of different
> invocations to do lots of different things with lots of different
> coreboot images.
>
> Two is specifically what is required to complete the configured build.
>
> It's certainly useful, and I argue highly desirable, to consider
> these two cases distinct.

Yes, those are different cases and they could use differently
configured builds of cbfstool. Whether that's desirable is a different
question. Adding a configuration system to cbfstool would come with a
lot of maintenance overhead -- rather than just making sure the tool
as a whole keeps working, we'd have to ensure all possible
configurations keep working independently. The way cbfstool is
currently written doesn't really lend itself to easily split out all
these kinds of optional things. You're asking for a lot of work,
basically, and I fail to see the benefit on the other side. The
current monolithic design shouldn't really cause problems or drawbacks
to anyone (and quite frankly, I think it doesn't -- as mentioned
before I think the issues discussed in this thread don't really have
much to do with how big cbfstool is, and it doesn't really seem like a
good "solution" to the HOSTCC problem to just allow switches to stop
building parts that happened to cause compiler problems in this
specific instance. Sooner or later some compiler-specific code would
be added to a core function that you can't configure out).

> Even worse is that you find it acceptable, or desirable, that a submodule
> is silently pulled in during the build. That may be typical for web
> development, but nothing that should be proliferated.

Yeah, I really don't understand your concern here. Is there maybe some
confusion about how Git submodules work? You seem to imply that this
may cause some kind of security or availability problem where there is
none: submodule code is mirrored on and downloaded from coreboot.org,
same as the main repository. The currently active submodule repository
HEAD is stored in the main coreboot repository (as a Git SHA), so you
will always get exactly the version of the code you're supposed to
have, and updating that is a step that is manually committed to the
main coreboot repository. Submodules cannot suddenly become
unavailable or taken over by malicious actors due to actions outside
coreboot.org's control -- Git is not node.js. If you're just someone
building code from source in your local checkout, it really makes zero
practical difference to you whether that code comes out of the main
coreboot repository or a submodule (both of them are already on your
disk as part of the checkout and cryptographically guaranteed by
coreboot.org, nobody else).

Really, your host system's local glibc (which is also linked into
cbfstool) is a lot more unpredictable and potentially problematic here
than the parts of vboot that it includes.

> > I am just saying I don't think this discussion should be about vboot
>
> It is because that's what consistently causes me extra work and
> frustration every time I want to build a minimal coreboot.

Well then let's please talk about those specific frustrations and try
to find practical solutions instead. Like I said earlier, we're really
not trying to make vboot annoying (or even visible) to those who don't
want to deal with it, and I'm convinced that it shouldn't be. The
submodule setup we have right now really works very well in general
and there's basically no difference for you in whether something is
built out of coreboot/src or coreboot/3rdparty/xxx. If it does cause
friction for you, let me know and I'll try my best to help solve that.
I understand that there's the HOSTCC issue, but as I said it really
doesn't have anything to do with vboot, and there's nothing vboot
alone could do to solve it, nor would somehow removing vboot really
solve anything about that.

FWIW I do seem to recall that this already came up back when
__attribute__((fallthrough)) was added to vboot, and back then
everyone seemed to agree that it was reasonable to assume the same
version for the host compiler that we use in crossgcc. If we now
changed our mind and think this keeps causing too much pain to people
all the time, I'm happy to take that back out of vboot and make it
compatible with whatever older version you care about if that would
alleviate people's concerns. We're perfectly willing to fully align
vboot's code base on whatever the coreboot policy is on these sort of
things, the problem is just that coreboot can't seem to decide on a
clear policy for the host compiler, and whenever this is brought back
up again somehow vboot always gets all the blame.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: System gcc requirements

[Julius Werner]

> > vboot is used for more than just boot verification these days, we use
> > it as a sort of generic crypto toolbox for all of coreboot's crypto
> > needs (because it wouldn't make sense to implement, say, SHA-256
> > algorithms twice). For host utilities in particular, some of these are
> > needed in cbfstool (e.g. for the --hash-algorithm parameter to add a
> > hash attribute to a file), and there is no Kconfig cbfstool so you
> > can't just configure it out if you don't need it.
>
> It is clear that I don't need that functionality when I build a
> coreboot version without any vboot code, right?

It's just an optional feature of cbfstool, it actually doesn't have
anything to do with the firmware verification part of vboot. I mean,
yes, you may not need it, but you may just as likely not need
--alignment or --topswap-size or --empty-fits, or the locate, compact
and add-flat-binary commands, or any other optional niche case feature
that is supported in cbfstool. cbfstool is a toolbox utility that
supports everything people may want to do with CBFS images, not all of
which everyone necessarily needs. And it currently doesn't have a
configuration infrastructure like Kconfig to disable individual
features (and I hope that shouldn't become necessary either, because
that would just make it complicated and confusing).

The goal with linking vboot into cbfstool is generally to be
transparent, it's just pulling a few routines from a submodule, you're
not really supposed to notice it. Just like when you run `make
unit-tests` it's pulling in a third-party testing library from a
submodule but generally you don't need to care about those details
either. Unfortunately, if there is a situation where you can run into
issues that Jenkins couldn't test for, you may see those issues
anywhere in the code you build including inside those submodules, but
I think that's really a problem with Jenkins and not one one with
using submodules.

I am just saying I don't think this discussion should be about vboot
just because an issue that could have occurred in literally any piece
of code happened to occur in vboot code.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: System gcc requirements

[Julius Werner]

>> Whenever I want a build without vboot I get really annoyed about this
>> hardcoded dependency, even when vboot is disabled in Kconfig.
>>
>> Would a patch to make the dependency conditional on Kconfig get accepted?
>
> I should hope so, though I recommend starting a new thread to see if experts 
> on vboot can chime in and explain why this is or isn't a good idea.

vboot is used for more than just boot verification these days, we use
it as a sort of generic crypto toolbox for all of coreboot's crypto
needs (because it wouldn't make sense to implement, say, SHA-256
algorithms twice). For host utilities in particular, some of these are
needed in cbfstool (e.g. for the --hash-algorithm parameter to add a
hash attribute to a file), and there is no Kconfig cbfstool so you
can't just configure it out if you don't need it.

I don't think it's fair to single in on vboot as the big problem here.
We vboot developers are definitely not trying to make this hard or
obnoxious for anyone, and while it is hosted in a separate upstream
everyone is welcome to submit patches to that just like you can do to
coreboot (e.g. Idwer submitted some stuff to fix compilation under
FreeBSD recently) and I'll do what I can to help get them landed. But
none of these problems are really vboot-specific -- the situation is
just that there is no official toolchain for coreboot host utilities,
and the only testing we have is for exactly what Jenkins uses (which I
believe is the same thing used for crossgcc? +Patrick to keep me
honest). Anyone can land a patch that uses a GCC 8+-only feature in
cbfstool just as well as in vboot and it will cause exactly the same
problem.

So I think the "official" rule is basically that the minimum
requirement for host utilities is the same compiler and version that
crossgcc uses, which I think makes some amount of sense (otherwise
we'll just have to keep tracking and deploying two separate versions).
If you guys want to change that we can have that discussion, but
whatever we decide it's probably not going to be a very effective
decision unless someone puts in the work to really make Jenkins test
that continuously. And either way, I don't think any of this is
vboot's fault in particular (vboot uprevs are tested by Jenkins just
as much as any other patch and have to pass any compatibility tests
enforced there).
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] [RFC] Change in CBFS "stage" binary format

[Julius Werner]

Hi folks,

As some of you may know, I've been working on a way to add
verification directly into CBFS (as opposed to the current vboot that
can only verify images as a whole) for a while (I have presented at
OSFC 2019: https://www.youtube.com/watch?v=Hs_EhewBgtM and published a
general design document:
https://osfc.io/uploads/talk/paper/47/The_future_of_firmware_verification_in_coreboot.pdf).
As part of this work I have been very careful to keep all these
changes fully backwards-compatible so that existing payloads with
their own CBFS implementations will continue to work.

I have now come across a fundamental problem that I cannot solve
without introducing a small backwards-incompatible change: the CBFS
"stage" format that is used to store the code for coreboot stages
(e.g. romstage, ramstage) consists of a small "stage header"
containing things like load address and size in front of the actual
binary code. From the point of view of the generic CBFS structure,
both that header and the code are part of the "file data" (and not the
generic CBFS "file header" that contains things like the file name or
generic CBFS attributes).

This creates a tough chicken-and-egg problem when verifying pre-RAM
stages, because (for platforms that actually load pre-RAM stages and
don't execute in-place, like Arm platforms running from SRAM) the
stage needs to be directly loaded into the final location it should
execute out of, but the load address containing that location is in
the stage header. My verification design works on whole files, so the
stage header cannot be trusted before the whole file was loaded and
hashed. But I can't load it without information from the stage header,
and there's generally not enough scratch space in these pre-RAM
environments to first load it somewhere else and then copy it over to
its final destination after verification.

My solution to this problem is to move the stage header out of the
file data into a CBFS attribute that is stored in the generic CBFS
file header. File headers are verified separately so they can already
be trusted by the time the file data needs to be loaded. I think this
is arguably a somewhat cleaner approach anyway (the concept of CBFS
attributes just didn't exist yet when the stage format was originally
designed, so it wasn't written that way to start with), but of course
these new stages will be incompatible to the old ones. I believe this
is okay because the "stage" format is generally only used for parts of
coreboot itself (e.g. romstage, ramstage), whereas the payload and
other binaries the payload may want to chain-load should be using the
"simple ELF" (SELF) format anyway. Since coreboot is always built as a
whole, when you're gonna build the new version all your stages will be
in the new format and all code that loads them will know how to load
the new format, so you should be fine. Tying this into a payload build
system that is not aware of these changes should be okay because that
payload shouldn't try to add extra files using the "stage" format, and
shouldn't try to load any stages (if it wants to chain-load things it
should be using the SELF format with cbfstool add-payload, not
add-stage).

But this file format has remained untouched for the whole 10+ years
CBFS existed, so I thought I should ask before I do it. If you know
any payloads that do use the "stage" format for files outside of
coreboot itself and that couldn't adapt to this change for some
reason, or have any other concerns about unexpected problems this
could be causing you, please let me know here or on the CL:
https://review.coreboot.org/46484

Note that for cbfstool, this will also mean that newer versions of
cbfstool will only support the new format. I think this should also be
okay because coreboot is built as a whole, i.e. all your stages are
added with a cbfstool version that was built from the same code base
version as the coreboot code trying to load these stages. But it does
mean that if you have an old version of cbfstool stashed away
somewhere and try to use it to cbfstool print -v or cbfstool extract a
new coreboot image, it won't be able to show load address or entry
point for the new stages or convert them back into an ELF file on
extraction (or vice versa for new versions of cbfstool on old images).
So if you do a lot of after-the-fact cbfstool manipulation on
different versions of coreboot images (which is probably only done by
people trying to debug something?), you may need to keep two cbfstool
versions to switch back-and-forth between for a while. It would be
possible to make the new cbfstool version support both formats for
reading/extracting but that's a bunch of extra work that I hope I can
avoid unless people really feel like this is gonna be a widespread
problem.

Also note that I'm not planning to touch the SELF (payload) format
(which also has a header in the "file data" part) -- I will keep that
as it is because I think it's much more likely that external payloads

[coreboot] Re: [RFC] Replace strapping entries in coreboot table

[Julius Werner]

> Hi Julius,
>
> wasn't sure if you are addressing me personally. I'll assume these
> questions are meant for me as you replied to my mail.

I got the impression that the core discussion in this thread was
centered around whether payloads may contain board-specific drivers
(or hardcode board-specific information), vs. coreboot having to
gather all that information and pass it to the payload in a
board-agnostic way. I just tied in to your earlier email to respond to
that general question, rather than the more specific details about
parsing ACPI tables or whatever that were discussed later. I just
wanted to say that I think hardcoding board-specific stuff in payloads
is perfectly fine and something we have been doing for a long time (I
guess "always" is too long a timeframe for a project as old as
coreboot, but at least depthcharge has done this for as long as it
existed).

> > We have always had a split of responsibility between
> > coreboot and the payload -- for example, USB, storage devices, audio
> > devices, etc. have always only been initialized in the payload and
> > coreboot generally doesn't know anything about them.
>
> Let's digest this slowly, as I really don't know anything but x86 well
> and don't want to make false assumptions. Do you mean initialization
> as something that needs to be done so the OS can operate the device
> (this is what Intel would call "silicon init"). Or just so that the
> payload can operate the device?

I mean just for allowing the payload to operate the device. I agree
that the "silicon init" part has traditionally all been in coreboot,
but sometimes you have devices that don't require any "silicon init"
(and therefore coreboot doesn't really know or care about it), but the
payload may still want to use it, and therefore may need to know
chipset- or board-specific details about it.

> > How else is the
> > payload supposed to do that other than hardcoding board-specific
> > knowledge?
>
> More generic tables, for instance? On x86 we have ACPI, and you also
> mentioned kernel device trees. And I assumed so far that the intention
> of the coreboot tables also was something like this: to explicitly
> state the information a driver needs, and not implied by an id.

Using generic tables is fine when they exist, but they don't always
and I don't think coreboot needs to invent them wherever they don't.
In practice we often use these ID numbers to enable workarounds for
some odd and subtle bugs on certain board revisions that we've never
quite encountered in that form before -- if we were trying to design a
new generic table for each of those cases all the time, we'd be adding
three coreboot table entry types per platform that we bring up. These
IDs are usually not the first resort for tying things together, we
just like to have them available as the last.

Also, you still need to hardcode *some* board-specific information
anyway. For example, you can't just bundle every audio codec driver
that you support -- you'd be bloating your payload size unnecessarily.
You want to only compile in the one this board is actually using,
which is a form of hardcoding. If you're going that far already, what
difference does it make if you hardcode a little more information
(e.g. what I2C bus it's connected on) when there's no good existing
mechanism to pass that information through from coreboot?

> > (Note that on Arm devices we take this even further, and
> > those strapping IDs are also used to look up the right kernel device
> > tree to make sure the kernel can have the right information about
> > those strapping-dependent devices. It's board-specific information all
> > the way down.)
>
> Is it the kernel that looks up the correct device tree or is this
> still done in the payload?

The kernel image has a bunch of possible device trees bundled in it
and the payload picks the right one from those based on an identifying
string containing these IDs (e.g. "google,bob-rev2-sku1"). The payload
looks it up but the device tree itself (including that identifying
string) is defined in the kernel repo, so the kernel also "knows"
about these IDs (at least in its source repository, not in the
resulting executable).

> > We've been using this set of strapping IDs for many years, Tim's patch
> > is just more efficiently organizing a set of existing concepts that
> > had been slowly added piece by piece over time. I hope it shouldn't be
> > too controversial.
>
> Sorry if I made the impression. I don't think it's controversial. It
> seems like the right step for the path taken. I didn't understand why
> this path was taken and Tim asked for comments, hence I thought it's
> a good opportunity to discuss that as well. Not to criticize or bike-
> shed anything, but just to have talked about it and learn from one
> another :)

Okay, fair enough. Angel commented on the CL that this email thread
needs to get resolved before the patch can land, so I wanted to try to
help resolve it. Sounds like we're all 

[coreboot] Re: [RFC] Replace strapping entries in coreboot table

[Julius Werner]

> >> Generally, I wouldn't assume / want board-specific drivers outside of
> >> coreboot. It seems board-specific table entries would invite people to
> >> write such
> >>
> >
> > I don't think this is encouraging board-specific drivers; just trying to
> > pass information to the payload here, that's all.
>
> Wouldn't that payload then naturally contain a board-specific driver? I
> mean is it just displaying or storing the information or is it acting on
> it?

Yes, payloads contain board-specific drivers. Isn't that how it's
always been? We have always had a split of responsibility between
coreboot and the payload -- for example, USB, storage devices, audio
devices, etc. have always only been initialized in the payload and
coreboot generally doesn't know anything about them. How else is the
payload supposed to do that other than hardcoding board-specific
knowledge? On Arm platforms generally none of these devices are
enumerable, so the payload needs to know what address to find the eMMC
controller at, needs to have a driver to talk to it and needs to know
the maximum speed supported by the eMMC part on that board. We do all
of this by hardcoding board-specific information, and occasionally
(e.g. when multi-sourcing eMMC parts on what's otherwise the same
board) it is convenient to support multiple board SKUs in the same
image by looking up this information in a table indexed by a strapping
ID. When the device this information belongs to is wholly controlled
and initialized by the payload and coreboot doesn't know anything
about it, I think it makes sense for that table to also be in the
payload. (Note that on Arm devices we take this even further, and
those strapping IDs are also used to look up the right kernel device
tree to make sure the kernel can have the right information about
those strapping-dependent devices. It's board-specific information all
the way down.)

We've been using this set of strapping IDs for many years, Tim's patch
is just more efficiently organizing a set of existing concepts that
had been slowly added piece by piece over time. I hope it shouldn't be
too controversial.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: Vboot: Phase 3 unsuccessful with RO_SECTION only

[Julius Werner]

> Actually the behaviour you described in the 'third combination' I've been 
> able to achieve by having a tiny RO_SECTION and a large RW_A and excluding 
> the payload from being written to the RO_SECTION. It just felt a bit like 
> cheating but I may invest more time into it to see if its usable.

Well yeah, you can leave out the payload and that may be the biggest
part for you. But technically you could also leave out romstage and
ramstage in that situation, and the build system currently doesn't yet
offer an option to allow that.

> Ultimately the goal (at this time) is to have measured boot by expanding 
> hashs into PCR's which can be verified by the end user using TOTP.

Note that measured boot is independent from verified boot. The main
point of verified boot is to allow keeping a part of the flash
writable so it can be updated but is still cryptographically verified.
If you don't care about that, you can just write-protect your whole
flash and only enable CONFIG_TPM_MEASURED_BOOT. (Or, you know, not
write-protect anything, but then both measured and verified boot
become somewhat pointless because your trust anchor is not secure.)

> Another question if I may, does the behaviour you described apply to 4.12 
> also? I ask as there are a lot of boards that have a vboot-ro.fmd. Would 
> these also fail for the reasons you have described or is there better support 
> for this in 4.12 opposed to 4.11?

Are you talking about coreboot versions? Sorry, I don't follow the
tags we cut super closely. The behavior I described has been pretty
much unchanged since 2018, I think (so long before 4.12 or 4.11).
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: Vboot: Phase 3 unsuccessful with RO_SECTION only

[Julius Werner]

Hi Thomas,

I think the combination you're trying to build just doesn't work and
isn't really supposed to work, honestly. Unfortunately, the current
state of those Kconfig options was implemented by contributors who
weren't the primary vboot developers and pushed through somewhat
hastily without really working out what option combinations make
sense. There have been a couple of discussions about how this should
be changed for the better (although I can't really find them anymore
right now, some random Gerrit CLs two years ago) but I think basically
it's just been stuck like this for a while with nobody having the time
or pressure to clean things up.

vboot was originally designed to be used with VBOOT_SLOTS_RW_AB,
that's the most supported and most tested mode. I guess running it
with VBOOT_SLOTS_RW_A should also work, although some of the internal
logic is going to get a bit confused about the missing slot B, but
eventually it should end up falling into the correct high-level
behavior (that when slot A is broken, it falls into recovery mode).
But building vboot without any RW slots makes no sense: the whole
point of vboot is to verify RW slots. Without RW slots you don't need
vboot. If your whole flash is write-protected (and therefore trusted)
anyway, there's no reason to cryptographically verify any of it on
boot.

The third combination that would actually make sense is to have one RW
slot but to have no recovery mode fallback in the RO_SECTION. That
would be what you want for the tiniest possible flash usage, basically
(accepting the risk that if the RW_A section becomes corrupted your
device would just be a brick). But that combination is currently not
implemented.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: google/veyron: How to use emulator for development?

[Julius Werner]

> As I have access to a DediProg EM100Pro [1] including a 10-Pin Split
> Cable with a grabber on the hold pin [2], and a clip, I was wondering if
> I can hook it up without soldering to boot from (like an external chip).
> On some boards, the wiring does not support In-system programming (ISP),
> as some other parts would for example be powered.

I mean... I can at least tell you that we do this in practice all the
time (with generally no issues). The SPI flash is not electrically
isolated, so yes, you're gonna leak some current into the SoC when you
do this (there are 100Ohm limiting series resistors so it's not a
complete short). I've never heard of a Chromebook board dying from
this but I can't legally guarantee you anything, of course.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: [PATCH v4 1/2] firmware: google: Expose CBMEM over sysfs

[Julius Werner]

> Ok. Regardless of the concern of the physical address is there any usage
> of this attribute by userspace? The description makes it sound like it's
> a pure debug feature, which implies that it should be in debugfs and not
> in sysfs.

I'll leave that up to Patrick. I doubt we'd want to create a whole
separate debugfs hierarchy just for this. Like I said you can just
read it out of the log too, this would just make it a little bit more
convenient. It's not like it would be the only informational attribute
in sysfs...
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: [PATCH v4 1/2] firmware: google: Expose CBMEM over sysfs

[Julius Werner]

> > +What:  /sys/bus/coreboot/devices/.../cbmem_attributes/address
> > +Date:  Apr 2020
> > +KernelVersion: 5.6
> > +Contact:   Patrick Rudolph 
> > +Description:
> > +   coreboot device directory can contain a file named
> > +   cbmem_attributes/address if the device corresponds to a 
> > CBMEM
> > +   buffer.
> > +   The file holds an ASCII representation of the physical 
> > address
> > +   of the CBMEM buffer in hex (e.g. 0x8000d000) and 
> > should
> > +   be used for debugging only.
>
> If this is for debugging purposes only perhaps it should go into
> debugfs. We try to not leak information about physical addresses to
> userspace and this would let an attacker understand where memory may be.
> That's not ideal and should be avoided.

This is memory allocated by firmware and not subject to (k)ASLR, so
nothing valuable can be leaked here. The same addresses could already
be parsed out of /sys/firmware/log. Before this interface we usually
accessed this stuff via /dev/mem (and tools that want to remain
backwards-compatible will probably want to keep doing that), so having
a quick shorthand to grab physical addresses can be convenient.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: Supporting blobs with licenses that you agree to on download

[Julius Werner]

> I will create a repo for the qc stuff (let's discuss the details offline)

Okay, cool, thank you. Repo is created and I uploaded a patch to hook
it up to the build system here: https://review.coreboot.org/42548

> It may be good to get the various blob owners on board with such a license so 
> that we can have a single one that allows mere (re)distribution with little 
> effort on their and our part. Could you ask the QC folks if there's interest 
> in sorting this out?

I have mentioned this option to them already but they didn't really
pick it up. It seems to be *really* hard for them to make even the
slightest changes to any license text (I mean months to just change a
sentence or two), so I think the separate repository approach will be
the best we can do in the near term. On a scale of years we can keep
pointing out the friction caused by this and try to convince them to
move to a simpler license like Intel did (but remember how long that
took).

This still leaves the fbg1701 blob in the main blobs repository...
have you decided what to do with that?
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: Supporting blobs with licenses that you agree to on download

[Julius Werner]

> Here is what I suggested somewhere on Gerrit [1]:
>
>   I was thinking we could move the current `3rdparty/blobs`
>   to something like `3rdparty/limited_blobs` or `3rdparty/
>   restrictive_blobs`. And guard it like the `amd_blobs`.
>   Then move anything without doubts about redistribution
>   to a new `3rdparty/blobs`.
>
> I guess this would be a good first step. If we want to further separate
> the restrictive blobs by their licenses, we can still do that later.
> However, if it turns out that there is close to nothing to move to the
> new repository, we could as well just keep the current one and disable
> its automatic download (and announce that people should stop mirroring
> it).

Okay, sounds like we have general agreement on the second repository
approach? I don't think we'd want to disable automatic downloading for
the primary blobs repository because I think many people value that
for convenience. I agree that we can start with a single
restricted_blobs repo for now and see if there is a need to split it
up further later.

Patrick, any further concerns from your side? If not, would you mind
creating a new repository for this? I can write the patches to move
blobs and adjust the Makefiles afterwards.

> That would have been me on Gerrit. I'm not 100% sure where distribution
> ends and redistribution starts. But some of the AMD licenses in the old
> blobs repository are either absolutely limited in redistribution or not
> redistributable at all (by any meaningful definition of the term). They
> explicitly limit distribution to be used with the distributors products
> "that incorporate AMD products". That thing is written for ODMs/OEMs but
> no-one else. And said distribution would not happen under the terms of
> the same license but an EULA embedded into it.

Yeah, upon reading the AMD license a bit more closely I agree the
redistribution parts look a bit concerning to me as well. I don't know
exactly what we'd want to do with that, but let's please keep that
discussion separate from the one about other blobs that only have the
"must agree to license when downloading" problem (I am mostly just
trying to find a way to get my Qualcomm board supported right now).
The amd_blobs already have a separate repo, so I guess someone should
clean up the affected stuff from the main blobs repo for now and then
we can decide what to do with the amd_blobs repo (maybe we put a big
warning on it not to clone and rehost this publicly or something).
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: Supporting blobs with licenses that you agree to on download

[Julius Werner]

On Wed, Jun 10, 2020 at 12:11 AM Wim Vervoorn  wrote:
> You only need a single mainboard to be in the tree. A mainboard can trigger 
> cloning a specific branch of this repository after warning for the license.

So I think you're basically just suggesting to use branches instead of
different repositories to separate them, but still separate them all
individually. I don't think it makes much of a difference, just that
branches are usually used in Git to track different versions of the
same thing, so I think it might be confusing to use them to track
different things instead. I think if we decide that every affected
vendor should have their blobs isolated by themselves, we might as
well just make them different repositories (unless Patrick has any
preferences about what scales better on the infra side there).

>> > Would it be enough to just create a second repository
>> > (3rdparty/restrictive_blobs or something like that) which is not
>> > automatically checked out by CONFIG_USE_BLOBS so people can make a
>> > separate conscious decision if they want to check it out?
>
> If it doesn't allow redistribution, we'd have to check if coreboot.org can 
> host such repos (because we redistribute all the time) or if there's some 
> implied license by the licensor (they pushed it for redistribution after 
> all), and if we can mirror it to github.com and other places (or if that's 
> not implied anymore). As coreboot.org maintainers we won't accept a special 
> "redistribution by coreboot.org allowed" type of license: if those bits are 
> _that_ precious, we don't want them.

No, wait, sorry, I never said they don't allow redistribution. I think
it's clear that we can't host them if we can't redistribute them.
(Note that blobs with licenses like this are hosted in other projects'
big blob repos like
https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree
too.)

These licenses explicitly *do* allow unlimited redistribution. It's
just that the license text says you're only allowed to download it if
you're agreeing to the license (whether that's enforceable in each
jurisdiction is a different question, of course). So if anything this
is on the downloader, not on the redistributor. Personally, I think
this isn't much different than one of those bundled EULAs that say "if
you don't agree to this, you must bring the CD back to the store"...
but I'm not a lawyer and I can understand that some people may feel
more concerned about it, so I'm hoping we can find a solution that
allows those people to avoid downloading these blobs unintentionally.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: Supporting blobs with licenses that you agree to on download

[Julius Werner]

[resend to mailing list with approved address]

On Tue, Jun 9, 2020 at 6:41 PM Julius Werner  wrote:
>
> Trying to generalize the discussion from
> https://review.coreboot.org/c/blobs/+/41379 here.
>
> Some of the blobs in our 3rdparty/blobs repository have license
> language that basically says you have to agree to the license terms to
> even download the file, and otherwise you're not allowed to possess
> it. Some example language from the fbg1701 license:
>
> > Do not use or load software from this site or any associated materials 
> > (collectively, the "Software") until you have carefully read the following 
> > terms and conditions. By loading or using the Software, you agree to the 
> > terms of this Agreement. If you do not wish to so agree, do not install or 
> > use the Software.
>
> As far as I can tell this affects
> 3rdparty/blobs/mainboard/facebook/fbg1701/license.txt and (with
> slightly more ambiguous language) almost all AMD licenses (e.g.
> 3rdparty/blobs/soc/amd/stoneyridge/license.txt). We're trying to add a
> new blob needed to support a Qualcomm platform that comes with similar
> language.
>
> Some people pointed out on that CL that they are uncomfortable with
> licenses like this in the blobs directory, since it means they cannot
> clone the whole repository without agreeing to all licenses with this
> sort of language in the repo (even if they only want to use a
> completely unrelated blob). The concern was also raised that this
> violates the binary policy (the "unlimited redistribution" part)... I
> guess it's a matter of interpretation whether a license that allows
> you to redistribute the binary *if you agree to it* is still
> "unlimited". It seems that there were already similar concerns raised
> when the fbg1701 license landed (https://review.coreboot.org/34441)
> but it was submitted despite the unresolved disagreement.
>
> Can we come up with and implement a solution here that both respects
> people's concerns and still allows us to support the affected
> platforms? Clearly, the rules should be the same for all blobs, so if
> some blobs with language like this are already in the repository, it
> shouldn't be grounds to reject new blobs from landing. If we can come
> up with an alternative that people would feel more comfortable with,
> we should also apply it to those existing cases.
>
> Would it be enough to just create a second repository
> (3rdparty/restrictive_blobs or something like that) which is not
> automatically checked out by CONFIG_USE_BLOBS so people can make a
> separate conscious decision if they want to check it out? It seems
> that something similar to this was already attempted with the
> 3rdparty/amd_blobs repository (but it looks like the work wasn't
> finished because those blobs are still in 3rdparty/blobs as well?). Is
> it enough to put all these blobs into a single separate repository or
> do we need to make a separate repository per license (that might be
> okay for the big AMD case but it probably wouldn't scale well for
> small one-offs)?
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: What does mirror_payload() do and do we still need it?

[Julius Werner]

[resend with right account]

On Fri, Mar 6, 2020 at 5:50 PM Julius Werner  wrote:
>
> Hi,
>
> I'm trying to refactor some CBFS code (which necessarily leaks into
> the prog_loaders) and
> mirror_payload()/CONFIG_MIRROR_PAYLOAD_TO_RAM_BEFORE_LOADING is a bit
> of a complication. I'm trying to understand if this option is actually
> used by anyone. It seems to be "default n" in Kconfig and I don't see
> any mainboard/SoC select it specifically. It is not user-visible in
> menuconfig and the Chrome OS specific defconfig files don't select it
> either. In the original patch
> (https://review.coreboot.org/c/coreboot/+/5305) there seems to have
> been some discussion about which boards can even benefit from this,
> and I don't see any indication that it was ever really enabled
> anywhere.
>
> So is this actually used anywhere, by anyone? Can I just rip it out?
>
> Thanks,
> Julius
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Jenkins doesn't send emails anymore?

[Julius Werner]

I recently noticed that I never get Gerrit notification emails when
Jenkins sets a CL Verified+1 or Verified-1 anymore. It used to do
that. Does anyone know if this is some accidental/intentional change
on the server side (either with Gerrit or with Jenkins), or is that
something wrong in my configuration? I can't find any setting for
Gerrit on this and I'm pretty sure I don't have an email filter
affecting it either.

Not getting the Verified-1s is really annoying because then I never
notice my patches still need work and forget about them.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: Copy-first platform additions (was: Re: Re: Proposal to add teeth to our Gerrit guidelines)

[Julius Werner]

Looks like I'm too late to the party here, but I just read this and
wanted to also come out in support of what Nico said. I think the
endless copy that seems to have happened 5-10 years ago in
coreboot left us with pretty awful maintainability consequences and we
should (and I think we mostly are?) definitely move away from that.
It's not just about correctness, it's also a huge obstacle for anyone
trying to refactor some common interface that is used by hundreds of
almost-identical board or chipset versions and actively hindering us
from developing common code forward. Maybe it served a purpose at the
time when the project was smaller and there were less people with time
to review, but these days we are a pretty mature project with a lot of
industry support (i.e. new boards and chipsets mostly get added by
salaried vendor engineers rather than reverse-engineering hobbyists)
and should be able to afford higher code quality and reuseability
standards.

I think most of this is changing and has changed already (like Patrick
said it happened gradually) -- these days most chipset code for both
x86 and Arm is in src/soc/vendor/common, and most new mainboards use
the variant system. Whenever I see a patch of someone copy a
lot of code I definitely ask them to find a way to factor it out and
make it common instead. But if there is still uncertainty about what
the general right approach for new code is, I think we should decide
(and maybe write it down somewhere) that it should be code reuse and
not copy
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: MIPS architecture support is rotting away... should we consider dropping it?

[Julius Werner]

Okay, so all I've heard for now are supporting voices, and it doesn't
sound like anyone is particularly concerned about timeline (i.e.
needing to wait until after the next release or something). I've
uploaded a patch to kill it as https://review.coreboot.org/34919. If
anyone feels that we shouldn't do this, please yell.

On Tue, Aug 13, 2019 at 1:11 PM Julius Werner  wrote:
>
> > In the meantime, how are we doing to deal with the drivers in depthcharge 
> > using write32(v, a)?
> > Should we just remove them as well?
>
> Yes, when the architecture gets removed, all SoC and board code
> (including in payloads) that depends on it has to be removed as well,
> of course.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: MIPS architecture support is rotting away... should we consider dropping it?

[Julius Werner]

> In the meantime, how are we doing to deal with the drivers in depthcharge 
> using write32(v, a)?
> Should we just remove them as well?

Yes, when the architecture gets removed, all SoC and board code
(including in payloads) that depends on it has to be removed as well,
of course.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] MIPS architecture support is rotting away... should we consider dropping it?

[Julius Werner]

Hi,

I've just been bitten by build problems with outdated MIPS code for
one of my CLs (not for the first time), and I've been wondering if
it's maybe time that we drop the architecture port completely. It was
added 5 years ago to support a Chrome OS project that never ended up
going anywhere and has been sitting in the tree unused and unsupported
ever since. The only SoC/board building it is that old abandoned
Chrome OS project for which there's probably not even any hardware
around anymore (or if there is, nobody wants to spend time with it).
There's no maintainer or even anybody who really reads MIPS assembly
or understands the architecture's intricacies paying attention to it,
and it keeps causing trouble when we try to pull it along with
overarching refactorings. I think at some point, if nobody wants to
use it and there's no future use case on the horizon, we should
consider pulling the plug.

Some examples of stuff that causes problems:

1. There's no 64-bit division support (even soft-division) because we
have no code to implement the required libgcc function, and nobody
knows enough MIPS assembly to fix that. We need to keep several hacks
around in generic code (e.g. printf()) where code doesn't use 64-bit
division even though it probably should or has a special #if
CONFIG(ARCH_MIPS) case to deal with this.
2. The read/write8/16/32() functions in libpayload take arguments in
(value, addr) order, whereas for all other architectures they have
long since been standardized to take (addr, value). This means you
can't submit any generic code that does direct MMIO without wrapping
it in #if !CONFIG(ARCH_MIPS). There are a bunch of depthcharge drivers
left still using that old convention... I don't have time/interest
refactoring all of those and I don't think anybody else does either.

We could either drop it right away, or (if we think that's too sudden)
schedule it for deletion after the next coreboot release (4.11 in
December(?)). What do people think?
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: Suggestion Computer-On-Module implementations

[Julius Werner]

> It's not related to a project but general support for computer-on-module 
> (COM) boards. COM board is a single-board computers containing CPU, Memory, 
> Chipset (SoC). These boards don't contain the standard connectors for I/O. 
> These boards need to be placed on a carrier board (baseboad), which provides 
> the standard I/O connectors and the power.

Thanks, sorry, I really misunderstood what this was about then. In
this case, if the same baseboard can be used with different SOMs (like
Nico said), I agree that adding a different toplevel directory makes
sense. (Otherwise, the variant approach should be good enough.)
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: Suggestion Computer-On-Module implementations

[Julius Werner]

Could you guys explain your board layout a little more for those of us
not familiar with your project? Is this SOM/COM something that runs
coreboot code, or just something that the SoC running coreboot code
talks to? In the latter case, I would say that it's a peripheral and
thus its code belongs under src/drivers/ (e.g. maybe
src/drivers/som//). (Although there is some precedent
with src/ec and src/superio to have this stuff at top-level... I guess
the question is whether this module has the same kind of exceptional
position on the board as those other chips.)

On Tue, Jul 9, 2019 at 3:42 AM Patrick Rudolph  wrote:
>
> Hi Frans and Felix,
>
> On 2019-07-09 11:56 AM, Felix Held wrote:
> > Hi Frans!
> >
> >> Proposal is creating a COM directory ‘src/com’ where the module
> >> manufacturer and module name are used. (Similar to mainboard).
> >> In this src/com directory common module support is placed.
> >> mainboard will use this COM module and contains the ‘variant’ code.
> >
> > Sounds like a good idea to me; I wouldn't call the directory src/com
> > though since my first association with that name would be some sort of
> > communication device support. Maybe src/som (system on module)?
> >
>
> Technically a SOM/COM wont work at all without a carrier board.
> The configuration (devicetree.cb) heavily depends on the carrier board,
> but on the
> other hand the GPIO configuration is likely SOM specific.
>
> I guess it's a good idea to put *non mainboard-specific* code in src/som
> (or whatever).
> On the other hand as already said the variant scheme works quite well.
>
> > I think I talked with Nico and siro on that some months ago on IRC, so
> > it would be good if they could also comment on this.
> >
> > I think the other option we talked about was having the module as a
> > base and the official carrier board as mainboard variant and then use
> > the module code in a mainboard with the vendor being the vendor of the
> > carrier module which is a variant of the other mainboard code.
> > Putting this into a separate directory is probably the cleaner option
> > though.
> >
> > If the variants approach works well for that, I'd like to keep using
> > that and not introducing some new infrastructure; if that causes some
> > major pain, it might also be worth investigating how to improve things
> > there.
> >
> > Regards
> > Felix
>
>
> If we decide to go either way the documentation should be updated to
> explain this decision.
>
> Regards,
> Patrick
> ___
> coreboot mailing list -- coreboot@coreboot.org
> To unsubscribe send an email to coreboot-le...@coreboot.org
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: compiler/toolchain issue?

[Julius Werner]

> Using git blame, all of the code (cbfstool & commonlib headers) was last
> modified 2015 and 2017, so figure it is toolchain and/or my host, which
> is Ubuntu 16.04.

No, the code it's choking on was actually added in April:
https://review.coreboot.org/32027

I think I cherry-picked and modified a very old CL from 2015 (where I
had already tried doing that once and gave up at some point) as a
starting point to develop that patch, and it seems like the authoring
date from that stuck around in Git, so it looks like it's from 2015 in
blame. Somewhat unfortunate but can't fix it now. (Really, I'm
surprised that git-blame it uses that date at all, rather than the
commit date, but that's on Git.)

Someone already reported the GCC incompatibility on that patch, it
requires at least GCC 6.3.0 (possibly works with 5.0 as well, don't
think anybody tested that). Since the last Ubuntu LTS that comes with
an older version is already out-of-support and other distros have
presumably likewise been at a newer version for a while, we decided
that it doesn't need fixing.

Can you deal with it locally (e.g. manually install the GCC package from 17.04)?
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: More coding style change proposals

[Julius Werner]

> we mandate comments as follows:
> /*
>  * something
>  */

No we don't. We had a long discussion about multi-line comment styles
some years back and I made the same argument there. ;) That's why our
coding style now allows two versions so you can pick whatever suits
the situation at hand better:

/*
 * long
 * form
 */

/* short
   form */

> I note that the changing our comment style would have zero impact on
> code safety. The improvement of requiring a { on the ifs is known to
> have positive impact; it's why Rust and Go both require it to my
> understanding.

How is this actually "known"? Does that take the (relatively recent)
-Wmisleading-indentation into account? I think after all these years
where this has not (to my knowledge) caused any problems to coreboot,
"fixing" it now that we even have a compiler check to catch it seems
weird. It's complicating things for humans in favor of something that
a machine can (and already does) solve just as well.

> As for "the kernel" and its coding style, we are going to increasingly
> see people coming from worlds where "the kernel" and its coding style
> matter less and less. Maybe adherence to "the kernel" coding style
> mattered ten years ago; I don't think it is as important now. In my
> case, increasingly, "the kernel" coding style is looking a bit dated.

Are we? Why? I've helped dozens of SoC vendor engineers get started
with coreboot in the last couple of years and they all came from
kernel-style backgrounds (Linux, U-Boot or other projects using the
same style). Of course there are also people coming from elsewhere,
but I don't see kernel style becoming "unimportant" anytime soon. I'd
bet 95+% of coreboot systems run Linux afterwards, and when adding
support for new platforms it's just normal that the hardware vendor
has the same people working on both kernel and firmware drivers for
the same component.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: More coding style change proposals

[Julius Werner]

Doesn't -Wmisleading-indentation already catch all of this? That's
enabled by default on the coreboot gcc. I don't think "it's just a
heuristic" should be a concern unless anyone knows of a real example
that is otherwise valid coreboot code style but not caught by this
heuristic. (If we're worried about examples that are not valid code
style, then changing the code style to make them even more forbidden
doesn't help... so I think weird cases that mix tab and space
indentation or the like should count in favor of this.)

If we're concerned that clang-format might cement errors automatically
then that's a reason for not using clang-format that way, but I don't
see how changing the coding style would solve it. clang-format's whole
job is to take whatever input and transform it into the coding style,
so the input is likely not style-compliant yet.

Forcing braces on single-line statements adds an extra line of
whitespace where it would otherwise not necessarily belong, which
hurts readability. How much of a function can fit on a single screen
is a real readability concern, and I think this style change would
harm it. That's the same reason we write

 while {

instead of

 while
 {

like some other projects. (Of course blank lines can also help
readability, but only in the *right* places and not randomly injected
by style rules.) It would also move us yet again further away from
kernel style, causing more issues for people coming from other
projects.

On Thu, Jun 20, 2019 at 2:54 PM ron minnich  wrote:
>
> On Thu, Jun 20, 2019 at 1:12 PM Stefan Reinauer
>  wrote:
> >
> >
> >
> > On 20 Jun 2019 08:26, ron minnich  wrote:
> >
> > clang-format is not a textual preprocessor. It is basically the ast
> > builder of followed by output.
> >
> > So in your case, I just tried it
> > main() {
> >
> >   if (foo)
> > bar();
> > baz();
> > }
> >
> > and got the right thing, i.e. braces around bar but not baz.
> >
> >
> > The right thing (e.g. the obviously intended thing) would be too have 
> > braces around both.
> >
> > clang-format in this case masks the problem and makes it harder to identify 
> > by expanding the syntax of the unwanted behavior to look intentional.
>
> Nico and Stefan, you make a good point, but I would then argue for
> even better tools, like clang-tidy:
> /tmp/x.c:13:10: warning: statement should be inside braces
> [readability-braces-around-statements]
> if (foo)
> ^
>  {
>
> In this case, there is a warning thrown, and the author has to clean it up.
>
> I don't believe, based on a lot of the history of this sort of problem
> in C, that we should depend on human reviewers to catch mistakes like
> this. These tools exist because of a demonstrated need. I think
> coreboot could benefit from their proper application.
>
> You've presented a good case, but what about something like this:
> if (foo)
> bar();
> baz();
>
> what's intended? There's an indent, but it's partial. I would not want
> to guess. But I agree with you that an invisible fixup would be
> inappropriate.
> ___
> coreboot mailing list -- coreboot@coreboot.org
> To unsubscribe send an email to coreboot-le...@coreboot.org
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: Does NSA contribute to Coreboot?

[Julius Werner]

> We're reviewing the STM code, of course.

While we're on the topic, can someone please ask the NSA to honor our
coding style? ;) I don't want to get involved because it's really not
my area, but it looks pretty terrible at the moment (full of camelCase
and ALL_CAPS identifiers, C99 comments, typedefs to non-coreboot
types, commented-out code, incorrect or missing license headers,
#pragma pack() instead of __packed, etc.). If they just want to
copy wholesale UEFI files to coreboot, they should dump them in
vendorcode instead.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: coreboot leadership meeting minutes for May 8 & May 22

[Julius Werner]

Thanks for the detailed explanation Ron. FWIW I agree that removing
guards like in acpi.h makes sense.

> I did a simple test: apply #pragma once to coreboot. A coreboot build
> for watson opens 80K .h files today. #pragma once makes barely any
> difference; this says we are doing a good job in how we use our .h
> files.

AFAIK GCC nowadays has special support to detect the usual kind of
header guard that spans the whole file because it is such a common
pattern, and essentially treats it like #pragma once. That may be why
you saw little difference.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: coreboot leadership meeting minutes for May 8 & May 22

[Julius Werner]

> > * Can we adopt on principle removing all cpp guards on protos?
> Why would this be a good idea?
> What does "cpp guards" actually refer to?

Upon re-reading this I *think* what they mean is stuff like

#if CONFIG(SOME_OPTION)
void function_prototype(void);
#endif

and not what I'd usually call "header guards":

#ifndef THIS_FILE_H
#define THIS_FILE_H

...file contents

#endif /* THIS_FILE_H */

Cleaning up the former sounds reasonable to me, we already tend to
avoid that for new code because it's just less clutter without really
causing a problem (declaring extra prototypes don't hurt anyone), and
it avoids problems with code that is unreachable but still compiled
when CONFIG_SOME_OPTION is disabled.

> Guys, it is a bit tricky to prevent discussions in a call when someone brings 
> up a topic, and I'm not sure that blocking that is useful. The meetings are 
> open and particularly people commenting here have been invited to the calls. 
> Please make use of that opportunity and join, if you think that you have a 
> strong opinion.

I think I got a bit spooked because the minutes were hard to
understand and the "no decision made for now" sounds like another big
change to how we are writing code at the moment might be coming down
there in the future. If it's just a cleanup to conform to what has
mostly been recent practice anyway it's not that big of a deal.

In general I think it's fine if you discuss things on the call as they
come up, but if you notice that it's something that would interest the
community at large (e.g. proposed style changes) it would be nice if
you could summarize any discussion/opinions you had in an email
afterwards and then continue further discussion on the mailing list. I
guess that's somewhat what these minutes are, they were just a little
unclear in this case (and it would probably better to have a separate
topic for things of wider interest).
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: coreboot leadership meeting minutes for May 8 & May 22

[Julius Werner]

> >* We should be more strict about how we define our blocks’ APIs.
>
> Ok, but.. I just don't understand this proposal? Can someone explain?

I second this... this sounds like another code style
discussion/change. Can we please keep all code style discussions on
the mailing list? Or if there really needs to be a VC meeting for such
a discussion, can it be pre-announced so everyone interested can
participate?
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: Votes on copyright lines, line lengths and automatic formatting

[Julius Werner]

> On Tue, May 14, 2019 at 12:49:17PM -0700, Julius Werner wrote:
> > One thing I'm missing is an option to maintain clang-format as an
> > optional pre-submit hook that people can choose run over only the
> > lines their patch is touching if they want to, like I suggested in
> > that thread above.
> Since everybody is free to do that without project coordination, we
> don't need to ask for that to be project policy, no?

Well, we could still maintain a default .clang-format file and prepare
a ready-to-install commit hook so that people don't have to figure out
the details of clang-format-diff theirselves. I think it presents a
valid third way of "how do we want to deal with auto-formatting".

> I don't think there's anything bad enough to warrant calling off the
> current round though, which would only create additional confusion.

Yes, I didn't want to suggest that. I guess people who agree that
formatting should be optional can vote against it here and then we
could discuss details later if the option wins.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: Votes on copyright lines, line lengths and automatic formatting

[Julius Werner]

Hi Patrick,

Thanks for setting this up! I think it's very important that we give
everyone a voice in these matters. Let me link some old discussions in
case people want more context:

https://mail.coreboot.org/hyperkitty/list/coreboot@coreboot.org/thread/4FAD25MJPI2XSSWHDDI5CSJR6SKCRFWW/#FDPB2PVVV7AB2CPJSGX4S2SFA4WNURN4
https://review.coreboot.org/c/coreboot/+/27533
https://mail.coreboot.org/hyperkitty/list/coreboot@coreboot.org/thread/DXEYNXR37SW4H5QLREDFTOZRUUBJ4QLW/#3ZLYUYAP6WMXIPADPR5WUSBRASIKODD3

One thing I'm missing is an option to maintain clang-format as an
optional pre-submit hook that people can choose run over only the
lines their patch is touching if they want to, like I suggested in
that thread above. I think if we have so many options per question
anyway, that one would have been nice to add. Maybe next time we can
post the draft questions and give people a chance to comment first.
But the most important thing is that we're doing it at all.

From: Patrick Georgi via coreboot 
Date: Tue, May 14, 2019 at 11:11 AM
To: coreboot

> Hi everybody,
>
> The project leadership asked me to get the developer community's
> opinion on a couple of coding style related issues that were under
> discussion for a long time without clear resolution.
>
> To that end I just setup three elections on
> https://civs.cs.cornell.edu. They're ranked votes using the Condorcet
> method with a single winner per question. Voting ends on end of May 21
> AoE (https://en.wikipedia.org/wiki/Anywhere_on_Earth).
>
> The voting population was determined by Martin and covers everybody
> who made 10 contributions (changes, review flags) on
> review.coreboot.org over the last 2 years.
>
> These eligible developers should have received an email from me last
> week to introduce the process and three emails today with their
> personalized link to the elections, one for each question. If you
> think you're eligible but didn't get these emails, please reach out to
> me directly.
>
> For reference (but there won't be a vote here on this list!), the
> questions posed are:
>
> # How to handle copyright notices
> The coreboot project has had issues with copyright notices for a long
> time, and we’d like to start to address it.  We currently don’t have
> any guide on when it’s ok to add a copyright, and when it isn’t. This
> and other issues like refactoring patches in gerrit lead to things
> like:
> * Copyright on blank files
> * Adding a copyright line when deleting code
> * Adding a copyright line for trivial changes such as spelling fixes
>
> The copyright laws around the world differ, but it seems like most of
> them agree that copyright is specifically for acts of creativity.
> None of the above examples are creative, so that would exclude them
> from copyright protection.
>
> Another issue is that we’d rather not have an ever-growing list of
> copyright notices at the top of each file.  This has been addressed in
> other projects by having an AUTHORS file, and it’s been suggested that
> coreboot should follow this.
>
> The AUTHORS file would contain the list of copyright holders, based on
> the existing copyright lines.
>
> # This is the list of coreboot authors for copyright purposes.
> #
> # This does not necessarily list everyone who has contributed code, since in
> # some cases, their employer may be the copyright holder.  To see the full 
> list
> # of contributors, see the revision history in source control.
> Ronald G. Minnich 
> Kyösti Mälkki 
> SUSE LINUX AG
> coresystems GmbH
>
> All existing copyright notices would be replaced with text  “Copyright
> 2019 The coreboot project Authors.”
>
> Arguments for replacing existing copyright lines with the AUTHORS file:
> * This resolves many of the current issues with copyright notices.
>  * Without copyright notices in each individual file, once a name is
> in the AUTHORS file, the copyright holder doesn’t have to update it
> again.
>  * The headers won’t grow because there’s just one static line.
> * Copyright dates won’t have to be updated in multiple files anymore
>
> Arguments for leaving the copyright lines:
> * A single AUTHORS file doesn’t capture the amount of work that people
> have put into the codebase.  The copyright line allows people to see
> their contributions to coreboot.
>
> Please rank these choices:
> * Create an authors file and remove all existing copyright lines
> * Create an authors file but leave existing copyright lines
> * Don’t create an authors file or change existing copyright lines
>
>
> # Line length
> There have been arguments recently that the line length is a bit too
> restrictive and we should change the coding style to allow for longer
> lines.
>
> We currently have line length exceptions for comments and print format 
> strings.
>
> Arguments for longer line lengths:
> * We have large monitors and can easily see more than 80 characters now.
> * It’s better to just make the code look good than to worry about
> breaking the line up because it’s 

[coreboot] Re: What maintenance is expected from coreboot developers & companies

[Julius Werner]

> True, however we can still set expectation even if there is no way to enforce 
> it.  This would be more of a social norm than a hard requirement, but it does 
> help to know that if you are e.g. pushing an entire board into coreboot, that 
> to keep it in tree you are expected to do  amount of work over the coming 
> years.  That in turn may (or may not) influence the decisions to try to do a 
> port / push the board upstream in the first place, or in a corporate setting 
> potentially get the additional resources allocated to avoid merge / remove / 
> merge cycles that are more expensive than merge / maintain.

I mean, I think I'm trying to say that the we should put this as "if
you (or someone else who happens to be interested enough in your
platform) don't keep doing this, this and this, your platform will get
deleted". I don't think we can really put a timeframe to it either...
the board will just stay in the tree for as long as it gets
sufficiently maintained. (We can point out that maintenance burden
always tends to rise with age, though.) If an SoC doesn't get
maintained at all I'd expect that it may on average survive somewhere
around 3-5 years before people get too fed up with it, but you can't
really put a hard number on that... it always depends on what sort of
core API changes we end up making and how hard it is to pull that
particular SoC along (of course following best practices and trying to
stay within the common frameworks also naturally increases a
platform's chance of surviving longer).
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: What maintenance is expected from coreboot developers & companies

[Julius Werner]

I don't think as an open-source project we can really set hard
expectations of what a contributor does after their patch is landed,
regardless of whether it's an individual or a company. We don't make
our contributors sign contracts, after all. Any work on coreboot by
anyone is best effort, and anyone's priorities may change. I don't
think this is any different in other projects (e.g. Linux) either.

I think the only thing that matters in terms of "is being maintained"
or not is at what point we remove code from the tree. To a certain
extent, every committer has an obligation to make sure that new
patches they add don't break existing platforms but of course there is
a limit to how much a single person can test and how deep they can dig
into unfamiliar code. A maintainer is mostly a person that can help
committers make these kinds of changes in a way that won't break their
platform (through reviews), can regularly test their platforms to make
sure no silent breakages crept in (and fix those that did), and can
work on supporting larger API changes that we decide as a community in
their platform (e.g. something like dropping LATE_CBMEM_INIT).
Whenever we have code somewhere that's clearly in need of someone like
that but has nobody stepping up, we should consider removing it (which
should be a case by case decision based on how much trouble it causes
and how valuable it still is).

On Thu, May 2, 2019 at 2:48 PM Timothy Pearson
 wrote:
>
> I would suggest the same industry standard maintenance period that would 
> normally apply to a commercial, closed source product.  For instance, 
> consumer hardware might only receive a year or two of support from initial 
> release, but an embedded system might be more typically see 3-5 years or 
> more.  Especially with the focus on using binary-only libraries with newer 
> e.g. Intel platforms, I don't see how maintenance of the coreboot portions 
> can reasonably be expected beyond the support period of the upstream software 
> vendor.  At the same time, if a commercial product would see some degree of 
> maintenance, we should expect the same level of maintenance in the open 
> product (coreboot) where feasible.
>
> One interesting issue brought up is whether we should be expecting 
> maintainers to rewrite large chunks of code as the underlying coreboot APIs 
> change.  This is where things differ from a normal closed source lifecycle, 
> in that in the closed source world "maintenance" can be as simple as applying 
> small patches to a mothballed, static codebase and recompiling.  In the open 
> source world, maintenance normally involves a continual treadmill of making 
> sure the board support is kept up to date as the underlying codebase 
> continually changes.  While I am not advocating for the static approach, it 
> should be recognized that continual changes do cost something, and perhaps 
> there should be more emphasis on developers that are pushing widespread 
> changes also ensuring that all officially supported boards / chipsets / etc. 
> are fixed up at the same time.
>
> Just my $0.02, as a somewhat time strapped and not very good de facto 
> maintainer of parts of the legacy AMD codebase. :)
>
> - Original Message -
> > From: "Martin Roth" 
> > To: "Peter Stuge" 
> > Cc: "coreboot" 
> > Sent: Thursday, May 2, 2019 4:16:41 PM
> > Subject: [coreboot] Re: What maintenance is expected from coreboot 
> > developers & companies
>
> > Thanks Peter,
> >  Is there a time limit that you think is appropriate to request
> > maintenance for?
> >
> >  Obviously it's not reasonable to request that they maintain it
> > forever, but is 2 years after the initial push reasonable?  3 years?
> >  What level of maintenance would be expected?   Would we just require
> > bugfixes, or would adding new features to the chip be expected?  Or
> > even just reviews of code affecting their chip?
> >
> > Martin
> >
> >
> > On Wed, May 1, 2019 at 10:01 AM Peter Stuge  wrote:
> >>
> >> Martin Roth wrote:
> >> > I'd like to discuss the issue of what's expected from developers after
> >> > code is added to the coreboot tree.
> >>
> >> Thanks for bringing this up.
> >>
> >>
> >> > It seems like there's a feeling
> >> > that if a company pushes code to coreboot, or hires someone to push
> >> > code to coreboot that there's an obligation to help maintain that code
> >> > going forward.  This seems to be different than if an individual adds
> >> > code, but maybe I'm wrong.
> >>
> >> That's about right, at least for me.
> >>
> >> I assume that no company contributes to coreboot purely out of altruism,
> >> in fact I believe that doing so could violate tax code in some 
> >> jursidictions.
> >>
> >> Given that companies engage in coreboot development with a resource (money)
> >> profit motive I find it fair and just to expect that some of that profit
> >> will be contributed back into the project, through "public good"
> >> contributions, which can include janitorial maintenance 

[coreboot] Re: Syntax highlighting screwed up (orange function parameters)?

[Julius Werner]

> We do not have coreboot specific changes to the syntax highlighting code 
> (e.g. this is stock Gerrit). 2.16.7 had a number of changes in their syntax 
> highlighting though, and typically Chromium is not running on a release build 
> but something in between releases or highly customized.

Ah, okay. I think it's this:
https://bugs.chromium.org/p/gerrit/issues/detail?id=10658

That still doesn't explain why it's not matching braces, but I guess
the parser may have always been broken and we just didn't notice
because it uses the same color for so many things.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: Syntax highlighting screwed up (orange function parameters)?

[Julius Werner]

> Wanna file a bug at https://bugs.chromium.org/p/gerrit/issues/list ?

Are we sure this is a general Gerrit bug? I've not seen the same thing
on the Chromium Gerrit so I'm assuming it's specific to the coreboot
installation. I've opened a coreboot bug for now
(https://ticket.coreboot.org/issues/205), if Patrick (or whoever
manages that stuff) decides it's not specific to our setup we can
still complain to Gerrit directly.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Syntax highlighting screwed up (orange function parameters)?

[Julius Werner]

Has anything changed with the syntax highlighting on Gerrit recently?
I'm seeing many function parameters in orange (which is hard to read
on a green background), and signs that it misparsed the code somehow
(e.g. coloring opening braces different than closing ones). Example
attached.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: Please inform me about C100PA.

[Julius Werner]

> I've C100PA and have been searching about this laptop. I assume probably this 
> uses coreboot, not u-boot. I am not sure yet even such things, though,  I 
> would like to make this laptop as secure as possible.
>
> The question is: firstly, I want to grasp whether C100PA has binary blob or 
> not and how many proprietary software C100PA has in the initial state, or 
> how. About the proprietary software, I am not sure if it is proper to ask 
> here.

Yes, this is a Chromebook that is shipped with coreboot by the vendor.
The codename used in coreboot is veyron_minnie (i.e. the code is in
src/mainboard/google/veyron). This board does not use any proprietary
firmware or blobs (neither with factory firmware nor if you reflash it
with a current version of coreboot). It does however need proprietary
kernel drivers if you want to use the GPU. You can find some guides
about using upstream Arch Linux on this board which may help here:
https://archlinuxarm.org/platforms/armv7/rockchip/asus-chromebook-flip-c100p
(I believe the "veyron-libgl" package mentioned there contains the
proprietary GPU drivers.)

Note that the factory firmware on this laptop is already "as secure as
possible". Chromebooks are among the most secure computers you can buy
if you use them with the native Chrome OS. However, if you want to
install your own GNU/Linux you will need to enable "developer mode",
which must disable the built-in security features to allow you to boot
your own operating system. Note that this still doesn't make it any
less secure than most other GNU/Linux computers -- other than Chrome
OS, I'm not aware of any Linux desktop/laptop distribution that has
any sort of "secure boot" solution, so they're all equally "insecure".
Most people don't worry too much about exploit persistence and so
they're okay with that. A Linux distribution installed on a Chromebook
in developer mode is still no more or less secure than the same Linux
distribution installed on a laptop that originally shipped with
Windows.

That said, there are actually ways you can set up your own secure boot
with a Chromebook in developer mode, but they are complicated and not
well documented (see FWMP_DEV_USE_KEY_HASH at
https://www.chromium.org/chromium-os/fwmp). Managing a secure boot
solution that's actually secure against persistent exploits on your
own is very hard, so if you're a beginner I would just not worry about
that... as long as you install a well-maintained Linux distribution,
apply updates regularly and don't install software from unknown
sources, you're still plenty secure without "secure boot".
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: Coding style and automatic code formatting

[Julius Werner]

> traded in for more consistency and less friction over everything code 
> formatting

But this sounds like a pretty theoretical problem to me. Do we
regularly have review friction over the kinds of issues that humans
and formatter might disagree about (e.g. how to exactly align a
continuation line)? In my experience those are just left to the author
as long as it looks somewhat sane. The only style comments I usually
see on reviews are unambiguous things like line length or brace
placement, where the formatter will do the right thing when it's
configured right.

Why not try making it optional first and see what friction remains in practice?
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: Coding style and automatic code formatting

[Julius Werner]

> Do we want to enforce a single editor / IDE + configuration for coreboot
> contributions? For instance, Vim is quite configurable and helpful when
> writing code. This could make all tools for later processing unneces-
> sary.

I don't think this is something we'd ever want. It would be an
enormous barrier to entry for new developers.

> Do we want to enforce a single tool, e.g. clang-format, that does the
> job for us after editing a source file?

I'm not really a fan of auto-formatters because they can just never be
as good as a human in all cases. The line length issue already shows
that -- you're supposed to limit to 80 characters *unless* exceeding
that significantly increases readability. There are certainly
exceptions where that makes the code better to read, like when you
have a giant table in a header where aligning equal elements in the
same column really helps to see what is what (e.g. something like
this: 
https://review.coreboot.org/cgit/coreboot.git/tree/src/mainboard/getac/p470/irq_tables.c#n32
). There are also other alignment issues where tool output often
doesn't look as good as handcrafted code, e.g. when you're aligning
enum assignments at the equals sign or align the values in a long list
of #defines to start on the same column. I understand that not
everyone always wants to hand-align everything and it's not a
submission requirement, but forcing a formatter means those who want
to do that can't anymore, and the code looks worse for it. It also
means we could never make a judgement call on a specific situation
anymore if the formatter disagrees, no matter how much sense it would
make or how unreadable the auto-formatted version is.

The main impetus for this is to allow people who don't want to worry
about formatting to automate it away, right? Then why not make it
optional? As I understand it, clang-format has options to analyze a
patch and only reformat the lines that the patch touches. Why don't we
just create a pre-commit hook with that which people can optionally
install? Then you can run clang-format over your patches if you want
to, and people who want to hand-format code can continue to do that.
Nobody is going to complain about unrelated whitespace changes in
patches if it only reformats the lines that the patch already touches.
And in cases where we really want to format something different than
the formatter wants to, it's easy to skip the hook.

> Do we just want to keep check-patch and let authors / their editors
> format the code?

I think the current checkpatch system works pretty well, honestly. It
catches most common mistakes and the false-positive rate is low and
easy to ignore. I don't think we really have a problem that needs
fixing in terms of code style enforcement -- in my reviewing
experience, I rarely find myself having to point out style issues
manually. At most I may be asking people to please pay attention to
the checkpatch warnings, but usually they figure that out pretty
quickly anyway. It's true that checkpatch is no full C parser, but it
seems to be doing its job well enough and I don't see any drawbacks
that are actively forcing us to move away from it. (If you're worried
about excessive spam making reviews hard, maybe we can just add some
sort of rate limit to the code that posts checkpatch results as
comments? For example if there are more than 10 warnings of the same
type, just post the raw checkpatch output for all of those in a single
comment at the top of the file rather than each on the affected line.
In my experience this is only a problem when patches are superbly
borked anyway, such as when a file is full of DOS line endings.)

> Do we want to encourage reviewers to educate their fellows on code
> style (for instance, wrt. line length, less indentation levels, shorter,
> more meaningful identifiers, etc.)?

Most of this is a normal part reviewing and will never be automated
away anyway, right? E.g. I don't think we'll have a tool making good
judgement calls on when indentation depth has become so deep that you
should factor out another function or how to best name a variable
anytime soon. I've always been looking at these things in my reviews
and plan on continuing to do so.

Line length is the one thing that's easy to automate (at least as a
hard limit), and we've already done that with checkpatch. So I guess
it's checkpatch doing the educating, not the reviewer, but I think
that works just as well. All the reviewer has to do is to make sure
the checkpatch warnings have been addressed before submission (either
by fixing them or by agreeing that ignoring the warning is fine in
that case).
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: IS_ENABLED() vs CONFIG()

[Julius Werner]

> Now, when we run "coreboot$ ./util/lint/kconfig_lint " , we can see :
> #! Warning: CONFIG() used on unknown value (option) at 
> src/include/kconfig.h:21.
> #! Warning: CONFIG() used on unknown value (%02x) at 
> src/northbridge/amd/amdfam10/util.c:138.

Yes... that's why it's a warning and not an error. ;) It's hard to
completely avoid false positives with the limited parsing capabilities
of a Perl script.

I still think this is valuable enough as a warning because for new CLs
it should point out a real problem most of the time and will help
people avoid errors. We have plenty of other spurious warnings when
you run the linter across the whole tree right now (e.g. a page full
of "Unused symbol" that someone should probably clean up at some
point), so I hope adding two more isn't going to be a huge problem?
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: RFC: Replacing IS_ENABLED(CONFIG_XXX) with the shorter CONFIG(XXX)

[Julius Werner]

(dropped Hung-Te off the list somehow...)

On Wed, Mar 6, 2019 at 12:49 PM Julius Werner  wrote:
>
> > > Sounds like a good plan. Please keep Documentation in sync:
> > > Documentation/core/Kconfig.md seems to cover the implementation details.
>
> Uhh... where did you find that one? I don't see it in my tree anywhere...
>
> > Will it be more explicit if we call it HAS_CONFIG(XXX) ? Or HAS_KCONFIG(XXX)
> > CONFIG(XXX) seems too generic to me that some drivers may wan to use it for 
> > wrapping a reference to config tables, like GPIO(XXX).
>
> Hmm... I would really like to keep it as short as possible, and that's
> another 4 chars. Also, I feel HAS_CONFIG may be a bit confusing (e.g.
> may sound more like whether the config is used at all in a particular
> board/SoC/arch, rather than whether it is enabled). I guess we could
> go with KCONFIG(XXX) if you prefer, but I kinda liked the symmetry
> between CONFIG_XXX and CONFIG(XXX).
>
> I would say that the rules for the global namespace are first come,
> first pick. There are currently no macros named CONFIG() in the tree,
> and after this is introduced, nobody can add another one (since
> kconfig.h is force-included in every file and would lead to a
> duplicate definition). Of course normally we want to avoid names that
> are too generic in the global namespace, but for something as
> ubiquitous and useful as this I think we can make an exception
> (because I doubt any other use case could have a better justification
> for claiming this name).
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: RFC: Replacing IS_ENABLED(CONFIG_XXX) with the shorter CONFIG(XXX)

[Julius Werner]

> > Sounds like a good plan. Please keep Documentation in sync:
> > Documentation/core/Kconfig.md seems to cover the implementation details.

Uhh... where did you find that one? I don't see it in my tree anywhere...

> Will it be more explicit if we call it HAS_CONFIG(XXX) ? Or HAS_KCONFIG(XXX)
> CONFIG(XXX) seems too generic to me that some drivers may wan to use it for 
> wrapping a reference to config tables, like GPIO(XXX).

Hmm... I would really like to keep it as short as possible, and that's
another 4 chars. Also, I feel HAS_CONFIG may be a bit confusing (e.g.
may sound more like whether the config is used at all in a particular
board/SoC/arch, rather than whether it is enabled). I guess we could
go with KCONFIG(XXX) if you prefer, but I kinda liked the symmetry
between CONFIG_XXX and CONFIG(XXX).

I would say that the rules for the global namespace are first come,
first pick. There are currently no macros named CONFIG() in the tree,
and after this is introduced, nobody can add another one (since
kconfig.h is force-included in every file and would lead to a
duplicate definition). Of course normally we want to avoid names that
are too generic in the global namespace, but for something as
ubiquitous and useful as this I think we can make an exception
(because I doubt any other use case could have a better justification
for claiming this name).
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] RFC: Replacing IS_ENABLED(CONFIG_XXX) with the shorter CONFIG(XXX)

[Julius Werner]

Hi folks,

I'm proposing a small policy change for the way we refer to boolean Kconfig
variables in code. Right now, the directive is to always use the
IS_ENABLED() macro. I would like to replace this with a shorter macro
CONFIG() that also removes the need to type the CONFIG_ prefix again. The
idea is mostly to save some horizontal space and the occasional extra line
break for this boilerplate and make it a little easier to type.

It's a very simple change (patch train starts at
https://review.coreboot.org/c/coreboot/+/31773), but since it affects
pretty much all code in coreboot and payloads I wanted to send out a quick
FYI. Please let me know if anyone has any concerns with this.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: Upgrade the 12 years old LZMA libraries - should we do it?

[Julius Werner]

Generally, if the new code is just plain better I'm all for upgrading.
We should just make sure that it is really better from coreboot's
perspective... the needs of a userspace decompression tool may be
quite different from ours. In particular, we should make sure that
memory requirements (that 16KB scratchpad) do not grow, that code size
stays reasonable, and that it doesn't use any fancy CPU features we
might not be able to support early (e.g. SSE... or does that work out
of the box?).

Note that for LZ4 we use the exact same decompression code (from
src/commonlib/) in coreboot and in cbfstool. cbfstool always checks
that a compressed file can be decompressed again after adding it, so
by checking that with the very same code you can be pretty certain
that you won't suddenly run into unfortunate surprises during boot. If
we did the same thing with the LZMA code (currently we don't),
switching out the implementation under the hood would become a lot
less scary.

On Thu, Jan 24, 2019 at 1:08 AM Ivan Ivanov  wrote:
>
> Igor Pavlov (7z/LZMA SDK author) told me a few months ago that
> > Decompression speed for lzma:
> > lzma sdk 18.03 C code - about +120% speed increase from 4.42.
> > lzma sdk 18.03 asm-x64 code - about +180% speed increase from 4.42.
> Although he didn't mention any compression ratio changes, maybe
> because they're not big enough,
> this such a huge speed increase alone should be a good reason to
> upgrade these LZMA libraries.
>
> ср, 4 апр. 2018 г. в 11:19, Carl-Daniel Hailfinger
> :
> >
> > Hi Ivan,
> >
> > On 03.04.2018 20:03, Ivan Ivanov wrote:
> > > I have noticed that both coreboot and seabios are using the very old
> > > versions of LZMA SDK.
> >
> > True. I introduced the lzma code in coreboot (back when it was called
> > LinuxBIOS) when we were working on OLPC XO-1 support.
> >
> >
> > > If we will upgrade our LZMA libraries from the
> > > outdated-by-12-years 4.42 to the current version 18.04 , speed and
> > > compression ratio should improve and maybe a few bugs will be fixed.
> >
> > Do you have any numbers for this? An improved compression ratio and
> > improved speed would be nice indeed, but how does the size of the
> > decompression code change? If the decompression code grows more than the
> > size reduction from better compression, it would be a net loss. A
> > significantly reduced decompression speed would also be a problem.
> > Decompression speed would have to be measured both for stream
> > decompression (i.e. the decompressor gets the compressed data in
> > single-byte or multibyte chunks) as well as full-size decompression
> > (i.e. the decompressor can access all compressed data at once). We also
> > have to make sure that stream decompression still works after the change.
> >
> >
> > > Do you think it should be done, or you are OK with using such an
> > > outdated version?
> >
> > A size benefit for the resulting image is a good reason to switch.
> >
> > Regards,
> > Carl-Daniel
> ___
> coreboot mailing list -- coreboot@coreboot.org
> To unsubscribe send an email to coreboot-le...@coreboot.org
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: Fallback mechanisms on x86 with C_ENVIRONMENT_BOOTBLOCK

[Julius Werner]

> What does that practically look like? Every time we have to re-walk we have 
> to reverify the integrity of the metadata.

I mean, that is exactly what we're doing right now anyway (unless
something significantly changed in CBFS code since the last time I
checked). For every single CBFS file access we start at the root and
we walk the chain until we find the file we're trying to load (and all
of this are real SPI transfers, it's not cached anywhere). Hashing
times are generally insignificant compared to SPI access times IIRC
(especially since this should be a raw hash, not an RSA signature like
we use in current vboot). Validating the hash means we have to walk
the whole CBFS directory rather than stopping at the file when we find
it, but since we generally don't really pay attention for CBFS file
placement for performance right now that's presumably not a big deal
(would only double cost on average).

And there are certainly still ways to improve this with the right
caching, and ways to do that in a safe way even if verification is
involved, which we could explore if we wanted to. I'm just saying as a
baseline that the cost of CBFS walks seems to have never bothered us
in the past, so the comparable cost of reverifying metadata probably
shouldn't be a major criterion to reject the per-file approach.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: Fallback mechanisms on x86 with C_ENVIRONMENT_BOOTBLOCK

[Julius Werner]

> For 1, this is attempting to protect physical attack. Obviously this 
> particular problem can't be solved in isolation, but it's something to think 
> about.

But isn't this something that per-file hashing would probably make
easier to protect against, not harder? I mean, right now we just hash
the whole region once and then assume it stays good -- there is no
protection. I doubt this would really become easier to solve if you
split the CBFS up into chunks... you'd have to somehow built a system
where a whole chunk is loaded into memory at once, verified there, and
then every file we may want to access from it is accessed in that same
stage from that cached version in memory.

The file is the natural unit which is loaded at a time, so I'd think
scoping the verification to that would make it easiest to verify on
load. I mean, on Arm we already always load whole files at a time
anyway, so it's really just a matter of inserting the verification
step between load and decompression on the buffer we already have. (On
x86 it may be a bit different, but it should still be easier to find
space to load a file at a time than to load a larger CBFS chunk at a
time.)

> When discussing 2 from a practical matter, we need to pass on the metadata 
> information across stages to help mitigate 1 and ensure integrity of the 
> hashes are correct.

This is true -- but isn't this the same for all solutions? No matter
how you scope verification, if you want to verify things at load time
then every stage needs to be able to run verification, and it needs
some kind of trust anchor passed in from a previous stage for that. I
also don't think this should be a huge hurdle... we're already passing
vboot workbuffer metadata, this just means passing something more in
there. (For per-file hashing, I'd assume you'd just pass the single
hash covering all the metadata, and then all the metadata is verified
again on every CBFS walk.)

> Similarly, limiting complexity is also important. If we can group the assets 
> together that are tied to the boot flow then it's conceptually easier to 
> limit access to regions that haven't been checked yet or shouldn't be 
> accessed. I do think per-file metadata hashing brings a lot of complications 
> in implementation. When in limited resource environments chunking cbfs into 
> multiple regions lends itself well to accomplishing that while also 
> restricting access to data/regions that aren't needed yet when thinking about 
> limiting #1.

Fair enough. I agree limiting complexity is always important, I'm just
not ad-hoc convinced that a solution like you describe would really be
less complex than per-file hashing. I think that while it makes the
CBFS access code itself a bit more complex, it would save complexity
in other areas (e.g. if you have arbitrary chunks then something must
decide which chunk to use at what time and which file to pack into
which chunk, all of which is extra code). Assuming we can "get it
right once", I think per-file hashing should be a solution that will
"just work" for whatever future platform ports and general features
want to put in CBFS (whereas a solution where everyone who wants to
put another file into CBFS must understand the verification solution
well enough to make an informed decision on which chunk to place
something into may end up pushing more complexity onto more people).

Anyway, I didn't want to derail this thread into discussing CBFS
verification, I just wanted to mention that I still think the per-file
hashing is a good idea and worth discussing. We should have a larger
discussion about the pros and cons of possible approaches before we
decide what we're planning to do (and then someone still needs to find
time to do it, of course ;) ).
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Re: Fallback mechanisms on x86 with C_ENVIRONMENT_BOOTBLOCK

[Julius Werner]

> There's a trycount that I think defaults to 10. After 10 failing tries it 
> should switch slots. There's also a 'set good firmware' notion which signals 
> to the firmware one was able to boot. This is picked up on the next reboot 
> and the information is passed through  vboot non volatile storage. I can dig 
> up specific pointers in the code, but you may not have tried enough times to 
> see things switch.

Try counters rely on userspace controlling them. You need to have the
crossystem utility (part of vboot) available in userspace and all
hooked up correctly so that coreboot and crossystem read and write the
same NVRAM (I think this sort of works by default with x86 CMOS, on
Arm not so much). Then you can run stuff like "crossystem
fw_try_next=B fw_try_count=1", it should boot RW_B once and boot RW_A
again on the next boot (assuming your hang happens after verstage, of
course). (If you set fw_try_count=0, it will just keep booting the
current slot forever. Note that there's no way to automatically fall
to recovery mode (RO) with this, that only happens when explicitly
triggered by code.)

However, vboot isn't really trying to solve this kind of problem and I
don't think we should try to bend it to be something else. I agree
that it would be better to implement this separately, and that
separate FMAP sections would be a cleaner implementation. (It would be
nice if we could eventually retire CONFIG_CBFS_PREFIX then, the whole
fallback/... thing in all our images is just confusing to most
people.)

> FWIW, it's my opinion I think we'll need to start splitting cbfs into smaller 
> ones. This isn't specific to this situation, but splitting slots into 
> multiple cbfses  (rw-a-1, rw-a-2, etc) allows one to chain/group resources as 
> they are used along with more flexibility for signing/verification methods. 
> What you wrote above seems sane, but I think you'll run into build 
> limitations that don't allow one to target fmap regions for different assets. 
> It's a lot of Make w/ some special casing currently which is because people 
> didn't want another tool at the time -- however, once you need more cbfs 
> regions of different granularity I think having better tooling around 
> targeting final destination for assets is required.

Are we abandoning the idea to verify individual files instead (once
someone has time to implement it) then? I'd still think that would be
a nicer solution to get more flexible verification.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org

[coreboot] Prevent out-of-order submissions on Gerrit

[Julius Werner]

When you have a series of patches uploaded to Gerrit, there is an
implied dependency between them, and most tools respect that (i.e.
when Jenkins build tests them, it tests the current patch together
with all its ancestors). However, I can still submit any patch from
the middle of a patch train and it will cherry-pick just that one,
meaning that the state the tree ends up in will be different than the
one Jenkins tested (which can lead to breakages like I caused
accidentally with https://review.coreboot.org/29299).

Can we please configure Gerrit so that it just doesn't allow you to
submit patches with unsubmitted dependencies? I'm pretty sure the
Chromium Gerrit enforces that, so I assume there is an option
somewhere. It's just unsafe and pretty much never the right thing (if
you wanted to submit patches in a different order than they were
uploaded, you can just reupload them instead).

-- 
coreboot mailing list: coreboot@coreboot.org
https://mail.coreboot.org/mailman/listinfo/coreboot

Re: [coreboot] GSOC submission

[Julius Werner]

> - QEMU power9 support / initial openpower support (hard I guess?)

How about QEMU MIPS support? We have all the MIPS architecture code in
coreboot, but it's falling into complete disrepair because the only
supported board is some old aborted Google project that nobody has
hardware for anymore. It would be a shame if we eventually just have
to throw the whole architecture port away because we have no way to
test it anymore. Getting a working emulation board up for it would
mean that we can keep it alive and healthy until the next time we add
an actual MIPS board.

-- 
coreboot mailing list: coreboot@coreboot.org
https://mail.coreboot.org/mailman/listinfo/coreboot

Re: [coreboot] Coreboots Board Status have privacy issues for contributors

[Julius Werner]

Sounds like something that should be pretty simple to automate in the
uploader script? While it's probably good to also have a warning and
clarify that the final obligation lies with the uploader, there's no
reason we can't help them by adding sanitization for common issues as
we find them.

We're doing something similar when we collect Chrome OS crash reports
(these don't get made public so the impact isn't as high, but the
basic idea is the same), so we could just steal or at least take
inspiration from that code:
https://chromium.googlesource.com/chromiumos/platform2/+/master/crash-reporter/crash_collector.cc#262

(Note in particular the extra care taken to distinguish MAC addresses
from ATA ACPI commands, that's probably useful for our case as well?
Although maybe not anymore these days...)
On Sun, Nov 25, 2018 at 10:42 PM David Hendricks
 wrote:
>
>
>
> On Sun, Nov 25, 2018 at 9:25 AM  wrote:
>>
>> I was thinking of contributing to the Board Status but i dont want to
>> release any private data and wont contribute now. What is the usage of
>> the world to know what mac address the people are using?
>
>
> Thanks for pointing out these issues.
>
> For what it's worth, the user must use the '-u' option to upload results. And 
> as Arthur points out you can edit logs and such yourself to scrub any private 
> data. The script just automates a few steps for convenience, though obviously 
> we'd like a reasonably uniform data set to compare with. You're right that we 
> don't need to know anyone's MAC address for coreboot development; however as 
> others have pointed out a full kernel log is useful since firmware issues 
> often manifest themselves there (memory map incorrect, devices not enabled, 
> etc) so it's good to have them for comparison.
>
> Still, a pause as Mike suggested and perhaps a scary warning or two could be 
> useful.
>
>> Then there can be for example a simple live linux iso that people can boot
>> with LAN cable connected. No requirement of installation software, of
>> setting things up or anything like that.
>
>
> There is one - See util/board_status/set_up_live_image.sh .
>
> --
> coreboot mailing list: coreboot@coreboot.org
> https://mail.coreboot.org/mailman/listinfo/coreboot

-- 
coreboot mailing list: coreboot@coreboot.org
https://mail.coreboot.org/mailman/listinfo/coreboot