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…
>
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.
(Somewhat belatedly...)
Attendees
----------
David Brown (Linaro)
Mark Grosen (TI)
Bill Mills (TI)
Julius Werner (Google)
Kevin Townsend (Linaro)
Abhishek Pandit (Arm)
Dan Handley (Arm)
Sandrine Bailleux (Arm)
Joakim Bech (Linaro)
Eric Finco (ST)
Lionel Debieve (ST)
KangKang Shen (FutureWei)
Agenda
------
* Maintenance proposal
* Security process
* Misc
Maintenance proposal
--------------------
* SB presented slides (attached).
* JW: Main concern is maintainer bottleneck. Should there be a concept of platform maintainer that can only merge platform patches?
* SB: Idea is to increase the number of maintainers to reduce bottleneck.
* DH: Can layer concept of platform maintainers on top of this without defining specific role. E.g. can set the expectation that new platform maintainers should be available to review any platform changes instead of existing maintainers.
* JW: But if code has been approved by code owner then what value is maintainer adding?
* DH: Need someone to review high level aspects like licensing, IP concerns, if it fits in scope of project, etc...
* JW: Want to see if this works in practice.
* KKS: Need guidelines on how to review, e.g. to ensure important comments are addressed and not focus on small problems.
* JW: On the lifecycle, what's the point of keeping it in the tree if it doesn't build? Also what if you add generic API change - are you responsible for updating all platforms?
* SB: "Limited support" status would expect some features to still work.
* DH: Don't want to rip platforms out of the tree as soon as they stop working.
* JW: Shouldn't conflate the issues of "does CI work" and "code owner not responding".
* SB: Fair enough. On the generic API change thing, author can try to update all platforms but if changes are more involved then it's better for platform owner to do it.
* AB: Purpose of this meeting is to highlight the issues not do detailed review. Can save further comments for offline feedback.
* Discussion around how to review. Should we use Gerrit, Phabricator or email. Will start with email review to all the lists. Can revert to Gerrit if it gets out of control. Issue with Gerrit is would need to create new repo or choose temp place in existing repo.
[SB has now sought feedback on the lists:
https://developer.trustedfirmware.org/w/collaboration/project-maintenance-p…]
Security Process
---------------------
* EF: No concern on process itself. It's more a question of scope of security issues.
* EF: In particular, there's a grey area around what hardware/side-channel attacks we mitigate.
* DH: Yes, although hardware attacks were out of scope in the original design of e.g. TF-A, we have added piecemeal mitigations for certain issues (e.g. Spectre). Can give the false impression that the codebase is secure against all attacks in that class when it isn't necessarily.
* DH: Arm has internal threat models for TF-A and TF-M that we intend to publish when we can. This is really important in order to publicly communicate the classes of threats we think we've mitigated (and what we should continue to mitigate as changes are added). Also intend to publicise a threat model for upcoming Mbed TLS project.
* JB: OP-TEE currently doesn't have a threat model. It's been on the todo list for a long time.
* MB: PSA defines a number of threat models [for M Class] and has a potentially useful template that can be used. Can they be freely reused?
* Action: DH: Arm to find out.
* DH: Status on the process is I'm still making minor updates to cater for Mbed TLS. Main task now is to prototype how to execute process using Phabricator (or other tf.org tools) so we can activate it.
Misc
------
* JB: Is tf.org interested in being part of device tree consolidation work?
* DH: Yes, TF-A only added DTs to the repo for platforms that the kernel didn't want to host (e.g. models). If and when there's a new place for these, we should remove the ones from TF-A.
* General agreement
* JB: Should we have some kind of Change Log for TF-A?
* SB: We have one! https://trustedfirmware-a.readthedocs.io/en/latest/change-log.html
* DH: Would have liked to talk about tf.org tooling (e.g. Gerrit, Phabricator, cgit) but no time.
* AB: Could easily turn into a big discussion.
* DH: Agreed, maybe I can make a proposal and seek feedback from the project lists first before coming back to the TSC.
Hello,
>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,
Andrej Butok
-----Original Message-----
From: TF-M <tf-m-bounces(a)lists.trustedfirmware.org> On Behalf Of Sandrine Bailleux via TF-M
Sent: Thursday, March 26, 2020 10:28 AM
To: Christian Daudt <Christian.Daudt(a)cypress.com>; tf-a <tf-a(a)lists.trustedfirmware.org>; tf-m(a)lists.trustedfirmware.org; tsc(a)lists.trustedfirmware.org; op-tee(a)linaro.org
Cc: nd(a)arm.com
Subject: Re: [TF-M] Project Maintenance Proposal for tf.org Projects
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.
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, 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.
Regards,
Sandrine
--
TF-M mailing list
TF-M(a)lists.trustedfirmware.org
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.tru…
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
Hi
I'd like to discuss the tooling at TrustedFirmware.org (i.e. Cgit, Gerrit, Phabricator) to understand the rationale for the current toolset, what alternatives were considered/discounted and what future tools/enhancements could potentially help us. I expect any proposed changes will require much deeper analysis/discussion but it would be good to have an initial baseline discussion.
Regards
Dan.
From: TSC <tsc-bounces(a)lists.trustedfirmware.org> On Behalf Of Abhishek Pandit via TSC
Sent: 18 March 2020 17:13
To: tsc(a)lists.trustedfirmware.org
Subject: [TF-TSC] TSC Agenda 19 Mar 2020
Hi All,
Any agenda items for the TSC meeting this week?
TF-A & TF-M team have been working on defining a clear maintenance process. The initial draft has been uploaded for review -
https://developer.trustedfirmware.org/w/collaboration/project-maintenance-p…
We can have a quick overview in the meeting.
Thanks,
Abhishek
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.
Hi Abhishek,
I also propose we come back on the emails exchanged on the TSC mailing list concerning the on the Security incident handling process
Regards,
Eric Finco
[Description: Description: Description: Description: Description: Description: Description: Description: Description: Description: Description: Description: Description: logo_big5]
Eric FINCO | Tel: +33 (0)2 4402 7154
MDG | Technical Specialist
From: TSC <tsc-bounces(a)lists.trustedfirmware.org> On Behalf Of Abhishek Pandit via TSC
Sent: mercredi 18 mars 2020 18:13
To: tsc(a)lists.trustedfirmware.org
Subject: [TF-TSC] TSC Agenda 19 Mar 2020
Hi All,
Any agenda items for the TSC meeting this week?
TF-A & TF-M team have been working on defining a clear maintenance process. The initial draft has been uploaded for review -
https://developer.trustedfirmware.org/w/collaboration/project-maintenance-p…
We can have a quick overview in the meeting.
Thanks,
Abhishek
Hi All,
Any agenda items for the TSC meeting this week?
TF-A & TF-M team have been working on defining a clear maintenance process. The initial draft has been uploaded for review -
https://developer.trustedfirmware.org/w/collaboration/project-maintenance-p…
We can have a quick overview in the meeting.
Thanks,
Abhishek