| Message ID | 20240416190019.81016-1-raymond.mao@linaro.org |
|---|---|
| Headers | show |
| Series | Integrate MbedTLS v3.6 LTS with U-Boot | expand |
On Tue, Apr 16, 2024 at 11:59:56AM -0700, Raymond Mao wrote: > Integrate MbedTLS v3.6 LTS (currently v3.6.0-RC1) with U-Boot. > > Patch 01 and 02 are for introducing MbedTLS release package. > I have to split it into 2 parts due to the size limitation of Gmail. So to be clear, for v2 you need to switch this to subtrees, like we do for upstream dts files now. And a script to automate the "merge a new release" should be done as well. Thanks!
On Tue, Apr 16, 2024 at 11:59:56AM -0700, Raymond Mao wrote:
> Integrate MbedTLS v3.6 LTS (currently v3.6.0-RC1) with U-Boot.
Please look in to:
https://source.denx.de/u-boot/u-boot/-/jobs/818534
On Tue, Apr 16, 2024 at 11:59:56AM -0700, Raymond Mao wrote: [snip] > [1]: bloat-o-meter output between disabling/enabling MbedTLS > ``` > add/remove: 231/69 grow/shrink: 12/5 up/down: 60196/-11166 (49030) I don't know if this is qemu_arm64 or sandbox. With buildman's size comparison functions we see: qemu_arm64 : all +75537 bss -88 data +24 rodata +6349 text +69252 u-boot: add: 274/-12, grow: 12/-4 bytes: 72002/-3800 (68202) ... sandbox : all +57008 bss +32 data +1632 rodata +352 text +54992 u-boot: add: 143/-75, grow: 21/-18 bytes: 64058/-16523 (47535) So please look in to using buildman to get more details about what's changing, size-wise. Also, my goodness, that's far too much growth, and we aren't removing anything? This needs to be a whole switch, not just an addition. And then look in to what we can tweak / remove. Are we perhaps not getting our usual link time garbage collection done?
Hi Tom, On Tue, 16 Apr 2024 at 16:26, Tom Rini <trini@konsulko.com> wrote: > On Tue, Apr 16, 2024 at 11:59:56AM -0700, Raymond Mao wrote: > > > Integrate MbedTLS v3.6 LTS (currently v3.6.0-RC1) with U-Boot. > > Please look in to: > https://source.denx.de/u-boot/u-boot/-/jobs/818534 Yes. I am aware of the test failures of capsule UT and stated in the cover letter. This will be addressed when we have agreement on the current approach. Regards, Raymond
Hi Tom, On Tue, 16 Apr 2024 at 19:12, Tom Rini <trini@konsulko.com> wrote: > On Tue, Apr 16, 2024 at 11:59:56AM -0700, Raymond Mao wrote: > > [snip] > > [1]: bloat-o-meter output between disabling/enabling MbedTLS > > ``` > > add/remove: 231/69 grow/shrink: 12/5 up/down: 60196/-11166 (49030) > > I don't know if this is qemu_arm64 or sandbox. With buildman's size > comparison functions we see: > qemu_arm64 : all +75537 bss -88 data +24 rodata +6349 text > +69252 > u-boot: add: 274/-12, grow: 12/-4 bytes: 72002/-3800 (68202) > ... > sandbox : all +57008 bss +32 data +1632 rodata +352 > text +54992 > u-boot: add: 143/-75, grow: 21/-18 bytes: 64058/-16523 > (47535) > > So please look in to using buildman to get more details about what's > changing, size-wise. Also, my goodness, that's far too much growth, and > we aren't removing anything? This needs to be a whole switch, not just > an addition. And then look in to what we can tweak / remove. Are we > perhaps not getting our usual link time garbage collection done? > As stated in the cover letter, with this patch set, we still build the original libs (lib/rsa, lib/asn1_decoder.c, lib/crypto/rsa_helper.c, lib/md5.c, lib/sha1.c, lib/sha256.c, lib/sha512.c) for the components outside of EFI loader. Eventually all these will be completely switched and removed. But I think we should do this in a sparated patch set - It is too big for one patch set. So, as the first patch set, this one will introduce MbedTLS and enable it with EFI loader, after they are merged, the next patch set will switch other components to use MbedTLS and remove the original libs. What are your thoughts? Thanks and regards, Raymond
On Tue, Apr 16, 2024 at 07:47:44PM -0400, Raymond Mao wrote: > Hi Tom, > > On Tue, 16 Apr 2024 at 19:12, Tom Rini <trini@konsulko.com> wrote: > > > On Tue, Apr 16, 2024 at 11:59:56AM -0700, Raymond Mao wrote: > > > > [snip] > > > [1]: bloat-o-meter output between disabling/enabling MbedTLS > > > ``` > > > add/remove: 231/69 grow/shrink: 12/5 up/down: 60196/-11166 (49030) > > > > I don't know if this is qemu_arm64 or sandbox. With buildman's size > > comparison functions we see: > > qemu_arm64 : all +75537 bss -88 data +24 rodata +6349 text > > +69252 > > u-boot: add: 274/-12, grow: 12/-4 bytes: 72002/-3800 (68202) > > ... > > sandbox : all +57008 bss +32 data +1632 rodata +352 > > text +54992 > > u-boot: add: 143/-75, grow: 21/-18 bytes: 64058/-16523 > > (47535) > > > > So please look in to using buildman to get more details about what's > > changing, size-wise. Also, my goodness, that's far too much growth, and > > we aren't removing anything? This needs to be a whole switch, not just > > an addition. And then look in to what we can tweak / remove. Are we > > perhaps not getting our usual link time garbage collection done? > > > As stated in the cover letter, with this patch set, we still build the > original libs > (lib/rsa, lib/asn1_decoder.c, lib/crypto/rsa_helper.c, lib/md5.c, > lib/sha1.c, > lib/sha256.c, lib/sha512.c) for the components outside of EFI loader. > Eventually all these will be completely switched and removed. > But I think we should do this in a sparated patch set - It is too big for > one > patch set. > So, as the first patch set, this one will introduce MbedTLS and enable it > with > EFI loader, after they are merged, the next patch set will switch other > components > to use MbedTLS and remove the original libs. > What are your thoughts? My thoughts are that we need to implement this as the optional replacement for the existing libraries. We already have abstractions for most if not all of the algorithms I believe. And we need to be able to see how much size growth we get from this change because long term we don't really want to have two sets of libraries for this functionality (as a SW rather than HW implementation).
Hi Tom, On Tue, 16 Apr 2024 at 19:50, Tom Rini <trini@konsulko.com> wrote: > On Tue, Apr 16, 2024 at 07:47:44PM -0400, Raymond Mao wrote: > > Hi Tom, > > > > On Tue, 16 Apr 2024 at 19:12, Tom Rini <trini@konsulko.com> wrote: > > > > > On Tue, Apr 16, 2024 at 11:59:56AM -0700, Raymond Mao wrote: > > > > > > [snip] > > > > [1]: bloat-o-meter output between disabling/enabling MbedTLS > > > > ``` > > > > add/remove: 231/69 grow/shrink: 12/5 up/down: 60196/-11166 (49030) > > > > > > I don't know if this is qemu_arm64 or sandbox. With buildman's size > > > comparison functions we see: > > > qemu_arm64 : all +75537 bss -88 data +24 rodata +6349 > text > > > +69252 > > > u-boot: add: 274/-12, grow: 12/-4 bytes: 72002/-3800 > (68202) > > > ... > > > sandbox : all +57008 bss +32 data +1632 rodata +352 > > > text +54992 > > > u-boot: add: 143/-75, grow: 21/-18 bytes: 64058/-16523 > > > (47535) > > > > > > So please look in to using buildman to get more details about what's > > > changing, size-wise. Also, my goodness, that's far too much growth, and > > > we aren't removing anything? This needs to be a whole switch, not just > > > an addition. And then look in to what we can tweak / remove. Are we > > > perhaps not getting our usual link time garbage collection done? > > > > > As stated in the cover letter, with this patch set, we still build the > > original libs > > (lib/rsa, lib/asn1_decoder.c, lib/crypto/rsa_helper.c, lib/md5.c, > > lib/sha1.c, > > lib/sha256.c, lib/sha512.c) for the components outside of EFI loader. > > Eventually all these will be completely switched and removed. > > But I think we should do this in a sparated patch set - It is too big for > > one > > patch set. > > So, as the first patch set, this one will introduce MbedTLS and enable it > > with > > EFI loader, after they are merged, the next patch set will switch other > > components > > to use MbedTLS and remove the original libs. > > What are your thoughts? > > My thoughts are that we need to implement this as the optional > replacement for the existing libraries. We already have abstractions for > most if not all of the algorithms I believe. And we need to be able to > see how much size growth we get from this change because long term we > don't really want to have two sets of libraries for this functionality > (as a SW rather than HW implementation). > > I see. With this version most of the lib/crypto that are used by EFI loader (pkcs7, x509, public_key, mscode_parser, etc) are sharing the same abstractions except the ones for hash. I can replace them in v2, so that we can compare the final growth by switching on/off the MbedTLS build options. Thanks and regards, Raymond
On Tue, Apr 16, 2024 at 08:03:34PM -0400, Raymond Mao wrote: > Hi Tom, > > On Tue, 16 Apr 2024 at 19:50, Tom Rini <trini@konsulko.com> wrote: > > > On Tue, Apr 16, 2024 at 07:47:44PM -0400, Raymond Mao wrote: > > > Hi Tom, > > > > > > On Tue, 16 Apr 2024 at 19:12, Tom Rini <trini@konsulko.com> wrote: > > > > > > > On Tue, Apr 16, 2024 at 11:59:56AM -0700, Raymond Mao wrote: > > > > > > > > [snip] > > > > > [1]: bloat-o-meter output between disabling/enabling MbedTLS > > > > > ``` > > > > > add/remove: 231/69 grow/shrink: 12/5 up/down: 60196/-11166 (49030) > > > > > > > > I don't know if this is qemu_arm64 or sandbox. With buildman's size > > > > comparison functions we see: > > > > qemu_arm64 : all +75537 bss -88 data +24 rodata +6349 > > text > > > > +69252 > > > > u-boot: add: 274/-12, grow: 12/-4 bytes: 72002/-3800 > > (68202) > > > > ... > > > > sandbox : all +57008 bss +32 data +1632 rodata +352 > > > > text +54992 > > > > u-boot: add: 143/-75, grow: 21/-18 bytes: 64058/-16523 > > > > (47535) > > > > > > > > So please look in to using buildman to get more details about what's > > > > changing, size-wise. Also, my goodness, that's far too much growth, and > > > > we aren't removing anything? This needs to be a whole switch, not just > > > > an addition. And then look in to what we can tweak / remove. Are we > > > > perhaps not getting our usual link time garbage collection done? > > > > > > > As stated in the cover letter, with this patch set, we still build the > > > original libs > > > (lib/rsa, lib/asn1_decoder.c, lib/crypto/rsa_helper.c, lib/md5.c, > > > lib/sha1.c, > > > lib/sha256.c, lib/sha512.c) for the components outside of EFI loader. > > > Eventually all these will be completely switched and removed. > > > But I think we should do this in a sparated patch set - It is too big for > > > one > > > patch set. > > > So, as the first patch set, this one will introduce MbedTLS and enable it > > > with > > > EFI loader, after they are merged, the next patch set will switch other > > > components > > > to use MbedTLS and remove the original libs. > > > What are your thoughts? > > > > My thoughts are that we need to implement this as the optional > > replacement for the existing libraries. We already have abstractions for > > most if not all of the algorithms I believe. And we need to be able to > > see how much size growth we get from this change because long term we > > don't really want to have two sets of libraries for this functionality > > (as a SW rather than HW implementation). > > > > I see. With this version most of the lib/crypto that are used by EFI loader > (pkcs7, x509, public_key, mscode_parser, etc) are sharing the same > abstractions except the ones for hash. > I can replace them in v2, so that we can compare the final growth by > switching > on/off the MbedTLS build options. If there's abstractions we need first, lets get that done as a prereq series and then see what things are before/after. Thanks.
Hi Tom, On Tue, 16 Apr 2024 at 15:23, Tom Rini <trini@konsulko.com> wrote: > On Tue, Apr 16, 2024 at 11:59:56AM -0700, Raymond Mao wrote: > > > Integrate MbedTLS v3.6 LTS (currently v3.6.0-RC1) with U-Boot. > > > > Patch 01 and 02 are for introducing MbedTLS release package. > > I have to split it into 2 parts due to the size limitation of Gmail. > > So to be clear, for v2 you need to switch this to subtrees, like we do > for upstream dts files now. And a script to automate the "merge a new > release" should be done as well. Thanks! > > Just to confirm, for v2, I don't need to send the commit that was generated by "git subtree add ***" but just state the command as a prerequisite in the commit message, right? Like what was done for the commit id "5b825032957c2613ef2f8f639e949ae02cb5bdff". Thanks and regards, Raymond
On Wed, Apr 17, 2024 at 04:48:14PM -0400, Raymond Mao wrote: > Hi Tom, > > On Tue, 16 Apr 2024 at 15:23, Tom Rini <trini@konsulko.com> wrote: > > > On Tue, Apr 16, 2024 at 11:59:56AM -0700, Raymond Mao wrote: > > > > > Integrate MbedTLS v3.6 LTS (currently v3.6.0-RC1) with U-Boot. > > > > > > Patch 01 and 02 are for introducing MbedTLS release package. > > > I have to split it into 2 parts due to the size limitation of Gmail. > > > > So to be clear, for v2 you need to switch this to subtrees, like we do > > for upstream dts files now. And a script to automate the "merge a new > > release" should be done as well. Thanks! > > > Just to confirm, for v2, I don't need to send the commit that was generated > by "git subtree add ***" but just state the command as a prerequisite in the > commit message, right? Like what was done for the commit id > "5b825032957c2613ef2f8f639e949ae02cb5bdff". Yes, correct. For testing, people will be expected to either: - Manually "git subtree add ..." - Use the script that your series will add for syncing, to perform the initial import. The script for devicetree-rebasing handles this already so should be a good model, I believe.