[coreboot] CBFS bullfart (forked from GSoC-2014 Project for Coreboot)

[Aaron Durbin]

On Fri, Mar 14, 2014 at 11:53 AM, mrnuke <mr.nuke.me at gmail.com> wrote:
> Fucking Kmail is broken. How many times did it reply to the original sender
> instead of the list?
>
> On Friday, March 14, 2014 11:51:54 AM you wrote:
>> On Friday, March 14, 2014 08:58:01 AM you wrote:
>> > CBFS is definitely not friendly to environments that can't map() the
>> > storage area of the CBFS itself. Beyond that in-storage format with
>> > metadata tightly coupled to the data itself leads to needing to stream
>> > large amounts of data through the working set just to locate the piece
>> > that is desired.
>> >
>> > On top of that the CBFS API doesn't allow for freeing up of used
>> > resources. Yes, there is the construct of map() and unmap(), but the
>> > used API in the code base returns pointers from map() and that means
>> > there is no way to free up the internal cache.
>>
>> I found out that keeping track of the last map() pointer allows you to free
>> the cache during unmap(), as currently, operations come in map/unap pairs.
>> Sadly, this is just an accident of how CBFS core works. This design also
>> places the burden of cache management on the backend, which also has to deal
>> with whatever device it deals with. A pretty bad abstraction model.

Those map/unmap pairs are only when walking. The following functions
offer no ability to free the used resources in the common case usage
of CBFS_DEFAULT_MEDIA:

struct cbfs_file *cbfs_get_file(struct cbfs_media *media, const char *name);
void *cbfs_get_file_content(struct cbfs_media *media, const char
*name, int type, size_t *sz);
const struct cbfs_header *cbfs_get_header(struct cbfs_media *media);

If one is using cbfs with those functions resources are leaked. I
don't think the cache management is a bad abstraction. It allows for
the backing store to optimize, but I can see the downsides as well.
All this talk is very cbfs-centric, but if something is needing
multiple pieces of data at once from that then those buffers need to
be managed. Either that is done explicitly by the caller or internally
to the storage supplier. The issue is finding a solution that targets
the least common denominator: not everything has a large amount of
memory to play with at the time of cbfs accesses.

>> > However, read() doesn't
>> > solve the map() problem. read() and map() both need memory so in a
>> > constrained environment w/o memory-mapped access to the CBFS storage.
>>
>> The CBFS core doing map() + memcpy() doubles the resource requirements with
>> no real gain.

I agree, but depending on where those memcpy()'s are happening it is
somewhat minor (which I think is the current reality). I will point
out that cbfs_meda has a read() function. You could have addressed
that with a patch, no?

>>
>> > Lastly, the big reason for large map() requests is because we don't
>> > have a pipelined lzma path.
>>
>> Compression is "don't care" for the CBFS core until the stage is
>> decompressed. By the time you realize you don't need to decompress, you've
>> already map()ed the whole of romstage, which you now need to memcpy() to
>> its destination.

Agreed, but see my list of functions above which are the cause of
that. e.g. cbfs_load_stage() calls cbfs_get_file_content(). That's
your big mapping. However, the compression case I brought up is still
not possible to pipeline because of the lzma infrastructure we have.
That still needs to be addressed.

>> > For the non-lzma files read() will work exactly as you described.
>>
>> Not for non-lzma stages, which is the relevant bit for the bootblock, before
>> you get to romstage, where supposedly, you'll have RAM later on.
>>
>> I don't care that much anymore about this problem. I found a workaround for
>> it, but someone else will drop the soap when doing the next little SoC.
>>
>> > I noticed in your code [1] you just read in all of the CBFS in-core.
>> > Or did I misread that? And your init_default_cbfs_media() always
>> > re-initializes and reads the CBFS again? I take it that function only
>> > gets called once? Most likely because that is the bootblock.
>>
>> I only initialize the CBFS mirror once. If the code finds a valid CBFS
>> signature in RAM, it skips the whole re-read-this step.

Duh. I missed this piece:

if (CBFS_HEADER_MAGIC == ntohl(header->magic)) {
printk(BIOS_ERR, "Already have CBFS master header at %p\n", header);
return 0;
}

>>
>> > > [1]
>> > > https://github.com/mrnuke/coreboot/blob/369998920685852246dcae4c3e88b268
>> > > c1
>> > > c9f2dc/src/cpu/allwinner/a10/bootblock_media.c
>