Attention is currently required from: Jakub Czapiga.

View Change

20 comments:

• File payloads/libpayload/Makefile:

  • Patch Set #3, Line 294: $$(subst $(absobj)/,$(obj)/, \

    Should probably factor out the generic commonlib support changes into a separate patch.

• File payloads/libpayload/include/cbfs.h:

  • Patch Set #3, Line 42: * ---------------------------------------------------------------------------

    Since this is a new file, let's also use this opportunity to switch to SPDX here and get rid of the copyrights (they belong to cbfs_legacy.h now).

• File payloads/libpayload/include/cbfs_core.h:

  • Patch Set #3, Line 48: #include <commonlib/bsd/cbfs_serialized.h>

    Here too (in the headers) I think it would be better to just keep the legacy code and the new code completely separate. (I guess if there are naming clashes you may need to include cbfs_serialized.h here in the old header and remove the duplicates, but at least this header shouldn't be needed for any of the new code to make it easier to eventually just delete it.)

• File payloads/libpayload/include/cbfs_glue.h:

  • Patch Set #3:

    license

  • Patch Set #3, Line 32: static inline size_t cbfs_dev_offset(cbfs_dev_t dev)

    nit: not sure you really need this... the offset is only accessed by libpayload (or payload) which can depend on the internal details of this header. The read() and size() accessors are only abstracted so the commonlib/bsd code can call them.

  • Patch Set #3, Line 38: {

    This also needs to do range checking. I think

     if (offset + size < offset || offset + size > dev->size)
       return -CB_ERR_ARG;

    should be enough.

• File payloads/libpayload/libcbfs/Kconfig:

  • Patch Set #3, Line 12: bool

    This needs a prompt text to be selectable.

  • Patch Set #3, Line 16: acrive

    typo

    Also, should not reference cbfs_boot_locate (see CB:59682).

• File payloads/libpayload/libcbfs/cbfs.c:

  • Patch Set #1, Line 131: cbfs_err_t cbfs_walk(cbfs_dev_t dev,

    Ok, so I did it by changing the macro in the main Makefile. […]

    Try just adding these files to libcbfs-srcs directly and bypass the includemakefiles mechanism. In order to control where they're put in the build path, add a special case to the src-to-objs function that will map $(coreboottop)/commonlib/bsd (maybe useful to define an extra shorthand for that, e.g. $(commonlib_bsd)) to $(obj)/commonlib/bsd. It looks like src-to-obj was only partially copied from coreboot to libpayload and isn't actually used in create_cc_template, so you'll need to fix that too for this to work.

• File payloads/libpayload/libcbfs/cbfs.c:

  • Patch Set #3, Line 27: * SUCH DAMAGE.

    Same here

  • Patch Set #3, Line 29: #define LIBPAYLOAD

    Can get rid of all this cruft too. This file should never be built outside of libpayload nowadays.

  • Patch Set #3, Line 60: void cbfs_boot_device_find_mcache(struct cbfs_boot_device *cbd, bool is_ro);

    Why are these functions not static? Then you don't need prototypes.

  • Patch Set #3, Line 91: cbfs_boot_device_find_mcache(&rw, false);

    I think rather than making find_mcache() a separate function, it's easier to just set rw.dev.mcache and mcache_size here directly?

  • Patch Set #3, Line 134: ERROR("Metadata hash mismatch for '%s'\n", name);

    This doesn't print the name in coreboot because it's not a problem with the particular file, it's a problem with the whole CBFS. It would maybe make sense to print the FMAP section name here, but that's a bit hard to get at this point. So just don't print anything (the user will see the offsets in earlier log messages and can figure it out from there if necessary).

  • Patch Set #3, Line 141: dev->size = be32toh(mdata->h.len);

    cbfs_dev_t is not supposed to be an region_device equivalent and you shouldn't use it like that here... it should represent a whole CBFS, not just a file.

    I would just make this function return the offset as an ssize_t, and the caller can easily read the size out of mdata itself if needed.

  • Patch Set #3, Line 156: map

    nit: Just because it's not really a mapping here I'd call this different... maybe `scratch`?

  • Patch Set #3, Line 168: #if CONFIG(LP_LZ4)

    You should be able to implement this as

     if (!CONFIG(LP_LZ4)
       return 0;

    like in coreboot. Prefer C if() over preprocessor #if where possible.

  • Patch Set #3, Line 178: return 0;

    Since decompression works all the same for these algorithms, and there'd be more code duplicated here, maybe it would be cleaner to rearrange this function a bit:

     if (compression == CBFS_COMPRESS_NONE) {
       ...NONE case...
       return ...;
     }

     map = malloc(in_size);
     ...everything including libpayload_boot_device_read()...

     switch (compression) {
     case CBFS_COMPRESS_LZMA:
       if (!CONFIG(LP_LZMA)
         break;
       out_size = ulzman(...);
       break;
     ...
     }

     free(map);
     return out_size;

  • Patch Set #3, Line 266: **********************************************************************************************/

    I meant this too with moving to cbfs_legacy.c. Basically, move absolutely everything belonging to the old API to the side and start from scratch.

• File payloads/libpayload/libcbfs/cbfs_mcache.c:

  • Patch Set #3:

    This, too, should be pulled in from commonlib.

To view, visit change 59497. To unsubscribe, or for help writing mail filters, visit settings.