Patch Set 16:

(6 comments)

It seems a little weird to have the bulk of the implementation of fmap-from-rom here while only adding the CLI option in the next patch. Maybe it's a good idea to squash both?

Agreed.

View Change

7 comments:

• Commit Message:

  • Patch Set #16, Line 7: Add support to get layout from fmap (e.g. coreboot rom)

    Needs to be updated to mention fmap from rom functionality implementation.

    Done

• File fmap.h:

  • Patch Set #16, Line 1: /*

    Probably needs license from cbfstool here

    Done

• File fmap.c:

  • Patch Set #16, Line 1: /* Copyright 2015, Google Inc.

    probably needs licence from cbfstool.

    Done

• File layout.c:

  • Patch Set #16, Line 22: #include "flash.h"

    Added headers not needed anymore?

    Done

  • Patch Set #16, Line 104: /* returns the index of the entry (or a negative value if it is not found) */

    accidental newline?

    Done

• File libflashrom.c:

  • Patch Set #16, Line 456: fmap_bsearch

    This is no good for concatenated chips which can occur on Intel hardware :/

    Why is that? I don't usually work with concatenated chips...

  • Patch Set #16, Line 462: offset

    On Intel targets you probably only want and in some reasons even only can, if for instance ME region […]

    OK. I've also updated the public interface and CLI code to pass in offset and length. I broke apart the interface since it seemed clumsy to have filenames and offset/length arguments in the function signature.

    To address the problem you mention, I think we need to do a few things:
    1. Add region attributes to struct romentry which we can use for things like access permissions.
    2. Merge your linked list patch so we can incorporate layout info from multiple sources.
    3. Always read IFD when it's known to exist (e.g. all modern Intel-based systems) and set region attributes as needed.
    4. Add an error policy handling (something like https://gerrit.chromium.org/gerrit/12117 and https://gerrit.chromium.org/gerrit/46207). For example, in the ME region case we know that access to the ME region is blocked, so if the user is trying to read/write content then we know to err out but if we're just looking for an fmap then we can ignore it and keep searching.

    I'd still like to see this patch get merged soon as it addresses some immediate issues, but we should try to make layouts easier and more useful by doing these other things too.

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