Thanks Sandrine. Patches look good.
I realized after looking at things a little closer that i had
misunderstood how fconf works for io policies. I thought the image id's
themselves came from the config files and not just the UUID's, which is
why i was worried about bounds check, since the id was coming from an
external source(trusted or untrusted, depending on if it is signed data
or not).
This also made me realize that we are using another table built into
code, to convert from image id to UUID for io policies. Is there a
reason image id's also can't be discovered from the config file?
-Raghu
On 4/2/20 7:17 AM, Sandrine Bailleux (Code Review) wrote:
> Hi guys,
>
> This is the patch I mentioned last Thursday at the TF-A tech call. Sorry
> it took me so long to post it.
>
> View Change
> <https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3836>
>
> To view, visit change 3836
> <https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3836>.
> To unsubscribe, or for help writing mail filters, visit settings
> <https://review.trustedfirmware.org/settings>.
>
> Gerrit-Project: TF-A/trusted-firmware-a
> Gerrit-Branch: integration
> Gerrit-Change-Id: Ic5ea20e43cf8ca959bb7f9b60de7c0839b390add
> Gerrit-Change-Number: 3836
> Gerrit-PatchSet: 1
> Gerrit-Owner: Sandrine Bailleux <sandrine.bailleux(a)arm.com>
> Gerrit-Reviewer: Louis Mayencourt <louis.mayencourt(a)arm.com>
> Gerrit-Reviewer: Raghu K <raghu.ncstate(a)icloud.com>
> Gerrit-Reviewer: Sandrine Bailleux <sandrine.bailleux(a)arm.com>
> Gerrit-Comment-Date: Thu, 02 Apr 2020 14:17:12 +0000
> Gerrit-HasComments: No
> Gerrit-Has-Labels: No
> Gerrit-MessageType: comment
Hi Joakim,
On 4/1/20 10:08 AM, Joakim Bech via TSC wrote:
> Hi Christian, Sandrine, all,
>
> On Thu, Mar 26, 2020 at 10:27:14AM +0100, Sandrine Bailleux wrote:
>> Hi Christian,
>>
>> Thanks a lot for the read and the comments!
>>
>> On 3/25/20 7:05 PM, Christian Daudt wrote:
>>> �The maintenance proposal looks great ! I have some feedback on
>>> specific portions:
>>> �1. maintainer/owner/author patches. " Note that roles can be
>>> cumulative, in particular the same individual can be both a code owner
>>> and a maintainer. In such a scenario, the individual would be able to
>>> self-merge a patch solely affecting his module, having the authority to
>>> approve it from both a code owner and maintainer's point of view.": I'm
>>> always leery of people self-approving their patches. At a minimum, all
>>> self-patches should be published and a minimum wait time provided for
>>> feedback. Or preferably that another maintainer does the merge (it does
>>> not need to be mandated but should be suggested).
>>
>> Yes, actually this is something that generated some disagreement inside Arm
>> as well and I am glad you're bringing this up here, as I'd like to hear more
>> opinions on this.
>>
>> I too have concerns about allowing self-reviewing. I am not so much
>> concerned about people potentially abusing of this situation to silently
>> merge patches, as I think we should trust our maintainers. But I am worried
>> that a self-review is rarely as good as a peer review, simply because it is
>> so easy to miss things when it's your own work. I believe several pair of
>> eyes is always better, as different people think differently, have different
>> perspectives and backgrounds, and are able to catch different issues.
>>
>> But to pull this off, we need enough people to do all these reviews. The
>> proposal currently allows self-review because some of us feared that
>> mandating 2 reviewers for every patch (especially pure platform patches)
>> would be impractical and too heavyweight, especially for the TF-M project in
>> its current contributors organization, as I understand. It would be great to
>> get more feedback from the TF-M community as to whether they think it could
>> work in the end.
>>
>> It's a difficult balance between having the best possible code review
>> practices, and realistically getting all the review work done in a timely
>> manner, avoiding bottlenecks on specific people and keeping the flow of
>> patches smooth.
>>
>> I like your idea of a minimum wait time provided for feedback. I think it
>> could be a good middle ground solution.
>>
> +1 for that, after silence for X weeks it should be OK to merge the
> patch. X would need to be number that is high enough for people to have
> a chance to find it and look into it, but shouldn't be too high, since
> there is a risk that it'll force the contributor to pile up things that
> might be dependent on this patch. To throw something out, I'd say ~2
> weeks sounds like a good number to me.
>
>> Your other suggestion of having a different maintainer doing the merge would
>> work as well IMO but requires more workforce. Again this comes down to
>> whether this can realistically be achieved for each project. This solution
>> was actually suggested within Arm as well (and even called out at the end of
>> the proposal ;) ).
>>
>> Bottom line is, in an ideal world I would like to condemn self-review
>> because I consider this as bad practice
> +1
>
>> , but I do not know whether this will
>> be practical and will work for TF-M as well.
>>
>>> �2. 'timely manner': This expectation should be more explicit - when
>>> the author can start requesting other maintainers to merge on assumption
>>> that silence == approval (or not). Such timeliness expectations are
>>> probably best set per project however.
>>
>> Yes, "timely manner" is definitely too vague and was actually left that way
>> on purpose at this stage to avoid touching upon what I think is a sensitive
>> subject! I am aware that some patches sometimes spend a long time in review,
>> definitely longer than they should and it understandably generates some
>> frustration. This is something we absolutely need to improve on IMO and
>> hopefully a bigger pool of maintainers will help solve this issue. But I
>> agree that the expected review timeline should be clearly established and it
>> is probably best to let each project decides theirs.
>>
>>> �3. The proposal does not address branching strategies. i.e. will
>>> there be separate maintainers for dev/master/stable branches? I don't
>>> think it needs to address it yet - keep it simpler for a start. But a
>>> todo saying something like "in the future this project maintenance
>>> proposal might be expanded to address multi-branch maintainership" would
>>> be good.
>>
>> Good point. A todo sounds good, I will add one in the last section of the
>> document.
>>
>>> �4. The platform lifecycle state machine has too many transitions.
>>> "Fully maintained" <-> "orphan" -> "out" seems sufficient to me.
>>
>> Hmm OK. There might be too many transitions but I feel we need something
>> between fully maintained and out, i.e. the limited support one.
>>
>> Julius Werner also pointed out on Thursday that orphan might be misplaced,
>> as all these other stages deal with some degrees of feature support (what's
>> known to work), whereas orphan is an orthogonal topic that is not directly
>> related to the level of supported features. For example, a platform could
>> have recently become orphan but all features and tests still work for some
>> time.
>>
> At one point in time in the OP-TEE project we tried to keep track of
> maintained platforms, by simply saying maintained "Yes" if they are
> maintained. However they're not maintained, we indicated that by stating
> the last known version where a platform was maintained. People can still
> find that information here [1] (not up-to-date). The intention was to
> give future users of an old platform a chance to know if it ever has
> been supported and what version that was. That could serve as a starting
> point in case someone is interested in bring a device/platform back to
> life.
Yes, I think such information can be very useful. It saves some "git
archeology" effort to try and dig this information afterwards. Also,
when someone starts looking at a project, I would expect this to be one
of the first thing they look up, they would want to know in which shape
the project is for the particular platform they are interested in.
That's almost as important in my eyes as a "getting started" guide.
We could have such a high-level table that just says whether a platform
is supported or not (just a yes/no) and have complementary, per-platform
documentation that goes into the details of what features are supported
exactly.
> How that works in practice is that all OP-TEE maintainers are adding
> their "Tested-by" (see example [2]) tag for the platform they maintain
> when we're doing a release. If there are platforms with no "Tested-by"
> tag, then they simply end up with the "last known version".
I think that's a very good idea!
> However, to keep that up-to-date, it requires some discipline from the
> people maintaining such a table ... something that we in the OP-TEE
> project haven't been very good at :)
Can't this be automated, such that it doesn't need to be manually kept
up-to-date? I imagine we could have some tools generating the platform
support table out of such a commit message.
> So, I'm not proposing something, it's just that I wanted to share what
> we've tried and it "works", but not easy to maintain (a release
> checklist could fix that).
>
> [1] https://optee.readthedocs.io/en/latest/general/platforms.html
> [2] https://github.com/OP-TEE/optee_os/pull/3309/commits/765b92604459240bed7fcf…
>
Hi Alexei,
I second Varun on this. The patch is huge. I recommend breaking it up
into multiple commits. I've reviewed it but since it is a large patch,
it might require a few more sittings to grasp all the changes(which also
means there may be some stupid review comments :)).
-Raghu
On 3/31/20 10:28 AM, Varun Wadekar via TF-A wrote:
> Hello Alexei,
>
> Just curious, the patch is huge and will take some time to review. Do
> you expect this change to be merged before the v2.3 release?
>
> -Varun
>
> *From:* TF-A <tf-a-bounces(a)lists.trustedfirmware.org> *On Behalf Of
> *Alexei Fedorov via TF-A
> *Sent:* Tuesday, March 31, 2020 7:19 AM
> *To:* tf-a(a)lists.trustedfirmware.org
> *Subject:* [TF-A] Event Log for Measured Boot
>
> *External email: Use caution opening links or attachments*
>
> Hi,
>
> Please review and provide your comments for the patch which adds
>
> Event Log generation for the Measured Boot.
>
> https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3806
>
> Thanks.
>
> Alexei
>
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy
> the information in any medium. Thank you.
>
> ------------------------------------------------------------------------
> This email message is for the sole use of the intended recipient(s) and
> may contain confidential information. Any unauthorized review, use,
> disclosure or distribution is prohibited. If you are not the intended
> recipient, please contact the sender by reply email and destroy all
> copies of the original message.
> ------------------------------------------------------------------------
>
Hello TF-A,
I understand most devs/reviewers will be busy working towards code freeze,
but if its possible can this patch-set be reviewed.
The patch-set is about "Add support for Broadcom platform".
Patch-set link
https://review.trustedfirmware.org/q/topic:%2522brcm_initial_support%2522
Thanks
Sheetal
Sandrine,
Really glad to see this being pulled together. A couple of areas of feedback around the Platform Support Life Cycle.
As previously mentioned there are two orthogonal concerns captured in the current life cycle: Support and Functionality.
I'd like to see these split out. For functionality, chip vendors may not have a business case for supporting all features on a given platform but they may provide full support for the features they have chosen to include.
A simple example would be supporting PSA FF Isolation Level 1 only due to lack of HW isolation support needed to achieve Isolation Level 2 or greater.
Also, I'd like to see a stronger standard put forth for platform documentation. If a platform is "supported," I believe the documentation should be complete and accurate. A lack of complete and clear documentation leaves open a wide door for misuse/misconfiguration which could result in a vulnerable system.
Here is a more concrete proposal:
Functional Support:
Each project shall provide a standard feature or functionality list.
Each platform shall include in its documentation a copy of this list with the supported functionality marked as supported.
The platform documentation may reference a ticket if support is planned but not yet present.
The platform documentation shall explicitly state if a feature or function has no plans for support.
The feature/functionality list shall be versioned, with the version tied to the release version(s) of the project.
In this way, it will be clear if a platform was last officially updated for version X but the project is currently at version Y > X.
Note: projects will need to adopt (if they have not already) a version scheme that distinguishes between feature updates and bug fixes.
Each project and platform shall use tags or similar functionality on tickets to associate tickets to features/functionality and platforms.
If the names of tags can't match the name of the feature or platform exactly then a mapping shall be provided in the appropriate document(s).
Life Cycle State
Fully Supported
There is (at least) one active code owner for this platform.
All supported features build and either all tests pass or failures are associated with tracked known issues.
Other (not associated to a test) Known Issues are tracked
Documentation is up to date
Note: Projects should document standards on how "active" code ownership is measured and
further document standards on how code owners are warned about impending life cycle state changes.
Orphan
There is no active code owner
All supported features build and either all tests pass or failures are associated with tracked known issues.
Other (not associated to a test) Known Issues may not have been maintained (as there is no active code owner)
Documentation status is unclear since there is no active code owner.
There has been no change to the feature/functionality list in the project since the platform was last "Fully Supported"
Out of date
Same as orphan, but either:
there have been changes to the feature/functionality list, or
there are failing tests without tracked tickets, or
there are known documentation issues.
Deprecated
Same as Out of Date, but the build is broken. Platform may be removed from the project codebase in the future.
Erik Shreve, PSEM
Software Security Engineer & Architect (CMCU Platform Development)
-----Original Message-----
From: TF-M [mailto:tf-m-bounces@lists.trustedfirmware.org] On Behalf Of Sandrine Bailleux via TF-M
Sent: Tuesday, March 24, 2020 4:42 AM
To: tf-a; tf-m(a)lists.trustedfirmware.org; tsc(a)lists.trustedfirmware.org; op-tee(a)linaro.org
Cc: nd(a)arm.com
Subject: [EXTERNAL] [TF-M] Project Maintenance Proposal for tf.org Projects
Hello all,
As the developers community at trustedfirmware.org is growing, there is
an increasing need to have work processes that are clearly documented,
feel smooth and scale well. We think that there is an opportunity to
improve the way the trustedfirmware.org projects are managed today.
That's why we are sharing a project maintenance proposal, focusing on
the TF-A and TF-M projects initially. The aim of this document is to
propose a set of rules, guidelines and processes to try and improve the
way we work together as a community today.
Note that this is an early draft at this stage. This is put up for
further discussion within the trustedfirmware.org community. Nothing is
set in stone yet and it is expected to go under change as feedback from
the community is incorporated.
Please find the initial proposal here:
https://developer.trustedfirmware.org/w/collaboration/project-maintenance-p…
Please provide any feedback you may have by replying to this email
thread, keeping all 4 mailing lists in the recipients list.
I will collate comments from the community and try to incorporate them
in the document, keeping you updated on changes made between revisions.
Regards,
Sandrine
--
TF-M mailing list
TF-M(a)lists.trustedfirmware.org
https://lists.trustedfirmware.org/mailman/listinfo/tf-m
Hi Andrej,
On 3/26/20 10:54 AM, Andrej Butok via TF-A wrote:
>> But I am worried that a self-review is rarely as good as a peer review
>
> On practice, unfortunately, some TF-M tasks are waiting weeks and even months for review and following approvals.
> If I were a maintainer & owner of my own TFM area, I do not want to wait & push & remind somebody else.
> Better to have a post-merge review for these cases, which does not limit and slow down the development.
Thanks for the feedback. That's not good, patches can't realistically
stay in review for weeks and even months, that's just not workable.
Worse, it might discourage developers to contribute to the project.
I can see that cumulating maintainer & owner roles would solve the
problem here but perhaps enlarging the pool of maintainers would as
well? Presumably, the situation is like that today because the current
maintainers of the project are overloaded and cannot get all reviews
done in a timely manner?
I am skeptical about a post-merge review process... Once a patch is
merged there is less urge and motivation (if any) for people to take a
look at it. I am worried that patches might never get reviewed that way.
Regards,
Sandrine
Hi Sandrine,
> Please find the initial proposal here:
>
> https://developer.trustedfirmware.org/w/collaboration/project-maintenance-p…
>
> Please provide any feedback you may have by replying to this email
> thread, keeping all 4 mailing lists in the recipients list.
>
> I will collate comments from the community and try to incorporate them
> in the document, keeping you updated on changes made between revisions.
The maintenance proposal looks great ! I have some feedback on specific portions:
1. maintainer/owner/author patches. " Note that roles can be cumulative, in particular the same individual can be both a code owner and a maintainer. In such a scenario, the individual would be able to self-merge a patch solely affecting his module, having the authority to approve it from both a code owner and maintainer's point of view.": I'm always leery of people self-approving their patches. At a minimum, all self-patches should be published and a minimum wait time provided for feedback. Or preferably that another maintainer does the merge (it does not need to be mandated but should be suggested).
2. 'timely manner': This expectation should be more explicit - when the author can start requesting other maintainers to merge on assumption that silence == approval (or not). Such timeliness expectations are probably best set per project however.
3. The proposal does not address branching strategies. i.e. will there be separate maintainers for dev/master/stable branches? I don't think it needs to address it yet - keep it simpler for a start. But a todo saying something like "in the future this project maintenance proposal might be expanded to address multi-branch maintainership" would be good.
4. The platform lifecycle state machine has too many transitions. "Fully maintained" <-> "orphan" -> "out" seems sufficient to me.
Thanks,
Christian.
This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
Hello Pankaj,
Hope you are doing well.
The initial email point to one change, but I see that as the tip of a patch series. I reviewed https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3370
and left some comments, but did not review the complete patch series.
Are you requesting a review of the entire patch series?
-Varun
From: TF-A <tf-a-bounces(a)lists.trustedfirmware.org> On Behalf Of Joanna Farley via TF-A
Sent: Tuesday, March 31, 2020 9:37 AM
To: Pankaj Gupta <pankaj.gupta(a)nxp.com>; tf-a(a)lists.trustedfirmware.org
Subject: Re: [TF-A] [EXT] Patch-set review request: New NXP Platform LX2120ARDB support on TFA
External email: Use caution opening links or attachments
Hi Pankaj,
We will try, one of the arm team has done some +1 reviews and I have just kicked of a CI+2 run on the top of the patch stack. As you can imagine the Arm team is pressed for time the closer to the freeze date.
Other TF-A contributors you all have +1 rights so if folks have time assistance with further +1 reviews would be appreciated as that would help in accelerating confidence is getting +2 and merging.
Traditionally the project has relied on Arm folks but for some time now all contributors have had +1 rights on each other patches and the new project maintenance proposal is set up to take us in the direction of enabling all contributors helping each other.
Thanks
Joanna
From: TF-A <tf-a-bounces(a)lists.trustedfirmware.org<mailto:tf-a-bounces@lists.trustedfirmware.org>> on behalf of Pankaj Gupta via TF-A <tf-a(a)lists.trustedfirmware.org<mailto:tf-a@lists.trustedfirmware.org>>
Reply to: Pankaj Gupta <pankaj.gupta(a)nxp.com<mailto:pankaj.gupta@nxp.com>>
Date: Monday, 30 March 2020 at 20:48
To: "tf-a(a)lists.trustedfirmware.org<mailto:tf-a@lists.trustedfirmware.org>" <tf-a(a)lists.trustedfirmware.org<mailto:tf-a@lists.trustedfirmware.org>>
Subject: Re: [TF-A] [EXT] Patch-set review request: New NXP Platform LX2120ARDB support on TFA
Please find the link to the review request.
https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3370
Regards
Pankaj
From: Pankaj Gupta via TF-A
Sent: Monday, 30 March, 22:57
Subject: [EXT] [TF-A] Patch-set review request: New NXP Platform LX2120ARDB support on TFA
To: tf-a(a)lists.trustedfirmware.org<mailto:tf-a@lists.trustedfirmware.org>
Caution: EXT Email
Hi All,
Please pick this review request so that code changes can be merged before code freeze.
Thanks.
Regards
Pankaj
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------