Change in coreboot[master]: libpayload: cbmem_console: Drop loglevel markers from snapshot

[Julius Werner (Code Review)]

Attention is currently required from: Raul Rangel, Yu-Ping Wu.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62506 )

Change subject: libpayload: cbmem_console: Drop loglevel markers from snapshot
......................................................................

Patch Set 1:

(3 comments)

File payloads/libpayload/drivers/cbmem_console.c:

https://review.coreboot.org/c/coreboot/+/62506/comment/611ba8a9_55bc67d6 
PS1, Line 120: /* Slight memory corruption may occur between reboots and give us a few
             : 	   unprintable characters like '\0'. Replace them with '?' on output. */
             : 	for (cursor = 0; cursor < size; cursor++)
             : 		if (!isprint(console_c[cursor]) && !isspace(console_c[cursor]))
             : 			console_c[cursor] = '?';
> Are you suggesting a callback?

Yes, including <commonlib/loglevel.h> directly in libpayload.h would be a problem. However, depthcharge is GPL so including <commonlib/loglevel.h> directly from the respective depthcharge file interpreting this should be fine. A problem is just that the commonlib integration (see CB:59697, CB:59916 and CB:60171) currently only covers commonlib/bsd... so if you wanted to do this you'd have to extend that to also cover commonlib/include directly (and that part should be gated by CONFIG_LP_GPL, which we are setting for Chrome OS).

I'm fine with either option but I don't have the time to implement a more complicated solution here, so let me know if someone else wants to do that and I'll abandon this.

https://review.coreboot.org/c/coreboot/+/62506/comment/a41258f6_ee303ce5 
PS1, Line 85: Due to stupid
            : 	   licensing restrictions
> Why can you add a BSD version in libpayload? You are the copyright holder. […]

Yeah, I've thought about several different ways to do this before deciding not to bother. BIOS_LOG_IS_MARKER is my copyright, but it depends on BIOS_LOG_PREFIX_MAX_LEVEL (also mine) which depends on BIOS_LOG_SPEW (not mine) -- so basically I'd have to relicense the whole file. The BIOS_LOG_xxx integers alone could probably be copied without exceeding the copyright threshold, but for the documentation comments on them that's not so clear (which were written by some guy in 2015 who as far as I know isn't currently active in the community). So I basically started rewriting all those documentation comments in my own words trying to say the same thing without copying any phrase directly, and then I realized that I have better things I could waste my time on.

https://review.coreboot.org/c/coreboot/+/62506/comment/b91508f6_79bf9ab8 
PS1, Line 90: memory corruption may occur between reboots
> Side note: I just learned that the kernel ramoops has an ECC function to correct errors like this. […]

I don't really think this is a big enough problem in practice to be worth that effort, tbh. That would probably be a bunch of extra code you have to link into and run in every coreboot stage. When I've seen corruption in practice, it's usually just a few sprinkled single characters that don't really hurt readability. This code is just here because when one of those few characters flipped to '\0', you don't want it to cut off the rest of the log.

(Also, this pretty much only used to happen for cold resets... and it seems on our more recent devices DRAM usually just can't survive a cold reset at all instead. I'm not quite sure where that came from, I think it may have to do with LPDDR3 being more fragile to this than the older technologies or something.)