[PATCH v3 03/25] mbedtls: add mbedtls into the build system

[Raymond Mao]

Hi Andy and Ilias,

On Wed, 5 Jun 2024 at 09:31, Ilias Apalodimas <ilias.apalodimas at linaro.org>
wrote:

> Hi Andy,
>
> [...]
>
> > > > > > Is this approach maintainable?
> > > > > > I don't remember if we have similar in Linux kernel, for example.
> > > > > > (There are few candidates like compression algorithms that are
> usually
> > > > > > being
> > > > > >  hosted elsewhere)
> > > >
> > > > No answer?
> > >
> > > subtrees is what was decided on OF_UPSRTEAM as well. If you have a
> > > better idea feel free to propose it, but for the sake of conformance
> > > we are better off doing the same thing on every external tree we pull
> > > in
> >
> > How do they will (or already do) maintain this?
> >
>
> There's a script to do that in dts/update-dts-subtree.sh
>
> > At least it's a good to have a few words on the choice made in the cover
> > letter, so we will have no questions on it.
> >
> > > > > > > Moreover, due to the Windows-style files from mbedtls git repo,
> > > > > > > we need to convert the CRLF endings to LF and do a commit
> manually:
> > > > > > >
> > > > > > > $ git add --renormalize .
> > > > > > > $ git commit
> >
> > ...
> >
> > > > > > >  lib/mbedtls/mbedtls_def_config.h | 4262
> ++++++++++++++++++++++++++++++
> > > > > >
> > > > > > This is ridiculously HUGE! This is unreviewable. Moreover, this
> is even
> > > > > > hard to
> > > > > > configure by the user! Can you rather make it modular and maybe
> create a
> > > > > > separate documentation for the most important options (I do not
> believe one
> > > > > > needs _all_ of them to be set / tuned)?
> > > > > >
> > > > > > This is a file from MbedTLS and follows its own style.
> > > > > And this is how MbedTLS is configured - with all features listed
> in a
> > > > > config file and
> > > > > commenting out the unused features with "//").
> > > > > The modification here is just to control those existing options
> with
> > > > > Kconfigs.
> > > >
> > > > And why should we blindly follow this nonsense?
> > >
> > > It's easier to follow up future changes tbh. But I do agree the config
> > > file is huge. Perhaps splitting in 2 files is going to be easier
> > > mbedtls_def_config.h -> contains all the options that rarely need
> > > tuning, which I assume is the majority of the header
> > > mbedtls_usef_config.h -> contains the imporant options, crypto,
> > > checksum algorithms etc
> > >
> > > Thoughts?
> >
> > The problem is on who decides which are "rarely need".
>
> Us. Sure it's going to take some time, but eventually we should be
> able to isolate options that rarely need a change
>
> > The feasible (to me) approach is to split per domain. Like you listed at
> the very end of your reply.
> > We can also learn from managing MTA configurations, such as Exim4 where
> user
> > may decide if they want a single file or split version.
>
> I am not sure tbh. Let me backpaddle from my previous suggestion :).
> Looking at it closer config.h is copied from mbedTLS, commenting out
> features we don't want.
>
> What we could do instead is either trim those comments out and only
> define the options we need for now. That would make the patch easily
> reviewable. If in the future we decide we need a more modular
> approach, we can retrofit it without breaking anything.
>
> Alternatively, we can undef the options we don't want in our header,
> but I am not sure how often these change or how scalable this is going
> to end up being. I prefer option #1
>
> I can remove the disabled features and only keep the ones that should be
controlled by the Kconfigs if you don't want a huge list for user's
fine-tune.
Currently most of them can be regarded as default setting by U-Boot.
In the future if any of them needs to be exposed as a Kconfig for tuning,
we can add them back.

Regards,
Raymond