Hi Varun,

Thanks for the feedback.

On 2/9/23 15:54, Varun Wadekar wrote:

Hi,

Thanks for the initiative. I agree with it in principle but have some suggestions.

1. We should allow platform owners to enforce or waive this policy for code that is encapsulated with a platform port. E.g one might choose to implement a plat/XX/common with weak functions and derive platforms ports from it. We should allow that.

I agree. I'll amend the patch in this regard.

2. The strong implementation under plat/common needs to fine grained to allow platforms to use "just enough" instead of it being an "all or nothing" solution. Weak functions under plat/common allow us the flexibility to implement only the differences and still include other functionality form the same files. I'm afraid platform owners will lose this flexibility. This is a huge loss and should be carefully weighed before moving forward.

Yes, that's on our mind. Although we've not completely thought through all the details, we suspect we will have to isolate each strong implementation under plat/common/ in a separate file such that platforms can pick and choose just the files (and thus, just the functions) they need.

I agree that losing this flexibility would be unacceptable.

Happy to hear other implementation suggestions around that!

Best regards, Sandrine

-Varun

-----Original Message----- From: Sandrine Bailleux via TF-A tf-a@lists.trustedfirmware.org Sent: Thursday, February 9, 2023 2:31 PM To: tf-a tf-a@lists.trustedfirmware.org Subject: [TF-A] Coding guidelines update: Discouraging usage of weak functions

External email: Use caution opening links or attachments

Hi all,

As you may know, the TF-A project historically used to make heavy use of weak functions. The code base has numerous examples of them. However, based on previous discussions, I think we have general consensus in the TF-A community that the disadvantages of weak functions outweigh their benefits and for this reason we should discourage introducing new ones.

In practice, I think this policy is already enforced most of the time in code reviews but I've now posted a patch that makes it explicit in the project's coding guidelines and provide the rationale behind it.

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freview.tru...

Note that TF-A maintainers would still be responsible for enforcing this policy, as the CI system won't flag that for them, mainly because I think there might be legitimate cases for weak functions in rare instances so we'd get some false positives from such checks.

We (at Arm) have got plans to gradually convert existing weak functions to strongly-defined implementations across the code base but this will take time.

Like I said, I don't expect this change to be too controversial but please raise any concerns you may have in this email thread or on Gerrit.

Best regards, Sandrine -- TF-A mailing list -- tf-a@lists.trustedfirmware.org To unsubscribe send an email to tf-a-leave@lists.trustedfirmware.org

Hi Varun,

On 2/9/23 16:20, Varun Wadekar wrote:

Hi,

...

...

Although we've not completely thought through all the details, we suspect we will have to isolate each strong implementation under plat/common/ in a separate file such that platforms can pick and choose just the files (and thus, just the functions) they need.

[VW] It would be interesting to see if a function pointer based solution, similar to psci_ops, would be useful.

I see that Raghu raised the same idea in Gerrit, quoting his comment for reference:

Another alternative to providing interfaces is using const function pointers. People may barf at function pointers as a security issue, but conscious use of const function pointers that defines platform interfaces, that must be filled, is another way to get similar effect to weak functions without breaking security or potential for causing bugs.

I agree that *const* function pointers sound safe from a security point of view.

I just have a doubt about the performance impact. Would there be any? Or would the "constness" of these function pointers allow the compiler to optimize and remove any indirection we'd get with mutable function pointers?

Best regards, Sandrine

Thanks Chris.

Then I think it would be worth exploring this option.

The advantage I see is, this would make the platform interfaces more obvious, as they would all be gathered in a single place / structure.

However, this might require more engineering work, as for any framework that tries to solve a problem in a generic manner, and I would imagine there will platform-specific exceptions we will have to cater for.

In comparison, the initial solution I proposed sounds easier to implement to me. I am not saying this should rule out the pointer-based solution, just sharing my early thoughts on this.

What I'll do for the time being, I will mention both approaches in the coding guidelines for now. When we start the weak function removal effort in the code base, we can prototype both approaches and revisit these recommendations if one approach stands out.

Best regards, Sandrine

On 2/10/23 14:04, Chris Kay wrote:

...

I just have a doubt about the performance impact. Would there be any? Or

would the "constness" of these function pointers allow the compiler to optimize and remove any indirection we'd get with mutable function pointers?

Both GCC and Clang can// inline indirect function calls so long as they know that the function pointer is immutable for the lifetime of the program (i.e. const static). [https://gcc.godbolt.org/z/z78M57bq9 https://gcc.godbolt.org/z/z78M57bq9]

Without IPO/LTO, however, the compiler cannot know the destination of the function pointer across compilation unit boundaries. [https://gcc.godbolt.org/z/WWKP6ncx6 https://gcc.godbolt.org/z/WWKP6ncx6]

So with IPO/LTO I would expect to see little to no performance impact, but without it might be a bit more substantial. There aren’t many reasons not to use LTO outside of debugging, so this might be an acceptable performance cost. Size cost might be more problematic for debug images, but it’s difficult to estimate that.

Chris

*From: *Sandrine Bailleux via TF-A tf-a@lists.trustedfirmware.org *Date: *Friday, 10 February 2023 at 12:16 *To: *Varun Wadekar vwadekar@nvidia.com, tf-a tf-a@lists.trustedfirmware.org *Subject: *[TF-A] Re: Coding guidelines update: Discouraging usage of weak functions

Hi Varun,

On 2/9/23 16:20, Varun Wadekar wrote:

...

Hi,

...

...

Although we've not completely thought through all the details, we suspect we will have to isolate each strong implementation under plat/common/ in a separate file such that platforms can pick and choose just the files (and thus, just the functions) they need.

[VW] It would be interesting to see if a function pointer based solution, similar to psci_ops, would be useful.

I see that Raghu raised the same idea in Gerrit, quoting his comment for reference:

...

Another alternative to providing interfaces is using const function pointers. People may barf at function pointers as a security issue, but conscious use of const function pointers that defines platform interfaces, that must be filled, is another way to get similar effect to weak functions without breaking security or

potential for causing bugs.

I agree that *const* function pointers sound safe from a security point of view.

I just have a doubt about the performance impact. Would there be any? Or would the "constness" of these function pointers allow the compiler to optimize and remove any indirection we'd get with mutable function pointers?

Best regards, Sandrine -- TF-A mailing list -- tf-a@lists.trustedfirmware.org To unsubscribe send an email to tf-a-leave@lists.trustedfirmware.org