Hi Sandrine, Soby,
Is there anything pending on my part to have this feature patch-set merged in TF-A?
-Sumit
On Wed, 4 Mar 2020 at 21:25, Sumit Garg sumit.garg@linaro.org wrote:
Hi Sandrine,
On Wed, 4 Mar 2020 at 18:27, Sandrine Bailleux sandrine.bailleux@arm.com wrote:
Hi Sumit,
On 3/2/20 10:37 AM, Sumit Garg wrote:
I hope that I am able to address almost every comment with updated patch-set [1].
Thanks for reworking the patches, they look mostly good from my point of view.
Thanks for your review.
The last remaining item would be to address the MISRA violations that Coverity found, which I've copied & pasted for you on Gerrit in the top patch.
I have tried to address most of MISRA violations and updated the patch-set. But since I don't have access to Coverity tool you are using, so can you please check if there is any relevant MISRA violation that I missed?
I know Soby might still have some concerns, he will let you know the details soon.
Sure, I will follow-up with Soby.
So, would it be possible to have this feature incorporated in upcoming TF-A v2.3 release?
I think this is a reasonable target.
Thanks.
One thing, though. I think it's fair to say that this patch stack is quite complex and interacts with lots of different parts of TF-A. I personally don't think I have the full picture in my head yet.
Agree.
Also, this feature is only used on QEMU right now and I am not aware of anyone trying to enable it for their platforms just yet. Thus there is a risk we might have overlooked some issues that we'll discover at that time.
Socionext being a silicon vendor is actively looking for this feature and I think they will build upon this feature to enable firmware encryption on their platforms to meet DRM robustness rules.
Furthermore, I know that you've done some testing of this feature on QEMU but this is not integrated into the CI loop right now. Thus, there is a risk that we might break it in the future and this will go unnoticed, unless you plan to test it regularly on your end.
Yeah we should plan to enable testing for this feature in CI loop.
For all these reasons, I would feel more comfortable if the feature was tagged as experimental for now. Would you be open to that?
Overall I agree with your concerns and fine with tagging this feature as experimental for upcoming release.
We already have such experimental features in the tree like BTI, some part of PAuth, measured boot, DebugFS. Marking a feature as experimental mostly just lowers the users expectations, telling them that they are using this code at their own risks. It also means we don't provide any stability guarantees just yet and that we might break compatibility without any notice in order to make the interfaces and implementation evolve as we see fit.
Marking a feature as experimental is usually done by:
- Saying so in the documentation.
Updated documentation.
- Having the build system print a warning message when using the build
flags enabling the experimental feature.
Updated Makefile to reflect this feature as experimental.
- Saying so in the (upcoming) change log.
I hope you can take care of this.
-Sumit
Regards, Sandrine