From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Fri, 24 May 2019 14:51:07 -0700 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9389F83F45; Fri, 24 May 2019 21:51:06 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-122-122.rdu2.redhat.com [10.10.122.122]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7032119729; Fri, 24 May 2019 21:51:04 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 0/3] update ArmSoftFloatLib to latest upstream version To: Ard Biesheuvel Cc: edk2-devel-groups-io , "Gao, Liming" , "Wang, Jian J" , Leif Lindholm , Michael D Kinney References: <20190524151140.23539-1-ard.biesheuvel@linaro.org> From: "Laszlo Ersek" Message-ID: <8e8dd012-e834-0e94-3e35-c10f27a18268@redhat.com> Date: Fri, 24 May 2019 23:51:03 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Fri, 24 May 2019 21:51:06 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 05/24/19 23:32, Ard Biesheuvel wrote: > On Fri, 24 May 2019 at 22:57, Laszlo Ersek wrote: >> >> Hi Ard, >> >> On 05/24/19 17:11, Ard Biesheuvel wrote: >>> Currently, our move to OpenSSL 1.1.1b is being blocked by an issue in >>> the ARM software floating point library, which lacks some intrinsics >>> that the ARM EABI spec defines. >>> >>> Since the code was in pretty sorry state, let's fix this by upgrading >>> to the very latest version of the core library this code is based on, >>> dated January 2018 (whereas the NetBSD fork of the old code dates back >>> to 2002) >> >> Thanks for this series! >> >> I've fetched your branch noted below, and build-tested it with >> ArmVirtQemu, ArmVirtQemuKernel, and ArmVirtXen. They all build fine. >> And, AIUI, ArmSoftFloatLib is only needed for 32-bit ARM (not AArch64), >> so I won't do other than build testing now. >> >> Build-tested-by: Laszlo Ersek >> >> I'll make a number of comments below. I'm not requesting that *you* do >> any of those, since you're already doing the community a favor, by >> putting out this fire. I'll just list what I think should be done. If >> there's agreement, I might take on a few of those. >> >> (1) We should file a new TianoCore BZ (Feature Request) for this >> ArmSoftFloatLib upgrade, and we should block TianoCore#1089 with that >> new BZ. >> >> (2) The new BZ should be referenced in all of the commit messages. >> >> (3) The new BZ should be added to the release planning wiki page. >> > > Fair enough. Thanks! I'll keep this tagged and seek to do (1) and (3) unless someone beats me to them until next Mon/Tues or so. >> (4) In the longer term, we should investigate whether this (large) >> library can be consumed as a git submodule. (Assuming that makes sense >> -- if we don't expect another upgrade anytime soon, then this may not be >> necessary.) >> > > This version of ArmSoftFloatLib implements all __aeabi routines that > are listed in the spec. Only a few of those are referenced by OpenSSL, > and in practice this code never gets exercised. So unless we grow > another user of this library, I have no intention of doing lots of > maintenance work on this library and (in response to your point below) > this is the reason I simply imported the whole library - to make > future upgrades, in case they do occur, as painless and > straightforward as possible. So I think a git submodule is overkill, > especially given the fact that there does not seem to be an > authoritative git upstream for this library. Sounds convincing, thank you. > >>> A few notable issues that may require some discussion: >>> - this code is made available under the 3-clause BSD license >> >> That should be OK; "Readme.md" white-lists the 3-clause BSD License. >> >>> - RVCT support is being dropped, since it is untested and nobody >>> appears to still care. >> >> (5) I'm OK with that, but we should file a separate TianoCore BZ for >> BaseTools, about RVCT removal. >> > > https://bugzilla.tianocore.org/show_bug.cgi?id=1750 Heh, reported about a month ago :) > >>> - no SPDX headers - this is left as an exercise for the steward. >> >> (6) Right, I've noticed that -- separate BZ (dependent on the one from >> (2)), or else it should be solved as the fourth patch in this series. >> > > Sure. The only downside to that is that it increases the delta with > the upstream library, so let's hope that this rebases cleanly if we do > end up upgrading. If we split the SPDX IDs to a separate patch now, then we could revert it temporarily at the time of the upgrade, apply the upgrade, and regenerate the SPDX tags right after the upgrade. And, these three patches could be squased into one before posting the upgrade. IOW, the "revert" would only be visible in isolation to the developer that implements the upgrade. (We could also attempt to contribute the SPDX IDs to the upstream ArmSoftFloatLib project, but you mention it doesn't have a central git repo, so it likely doesn't follow a distributed development model.) ... I've just grown so fond of the eminent greppability of SPDX IDs :) >>> >>> Code can be found here: >>> https://github.com/ardbiesheuvel/edk2/tree/bz_1089_upgrade_to_openssl_1_1_1b_v4 >>> >>> Cc: Laszlo Ersek >>> Cc: "Gao, Liming" >>> Cc: "Wang, Jian J" >>> Cc: Leif Lindholm >>> Cc: Michael D Kinney >>> >>> Ard Biesheuvel (3): >>> ArmPkg: import latest version (3e) of the Berkeley Softfloat library >>> ArmPkg/ArmSoftFloatLib: switch to new version of softfloat library >> >> (7) This patch (patch#2) uses designated initializers (in the >> initializers of the unions). I believe we never intend to build this >> library with anything else than GCC, but I think the coding style still >> requires us to avoid designated initializers. >> > > I can change that. Thanks! > >>> ArmPkg/ArmSoftFloatLib: remove source files that are no longer used >> >> OK, so here's a real scientific method that I used for determining >> whether the result of this patch was "minimal". It relies on the >> "strictatime" mount option -- I don't tolerate "relatime" or "noatime" >> exactly because the POSIX atime behavior is so useful for debugging file >> access. >> >> So, a few minutes passed between my checking out your branch, and >> starting the build tests. After the build tests above completed, I ran: >> >> $ find ArmPkg/Library/ArmSoftFloatLib/ -type f -printf '%A+ %p\n' \ >> | sort -r >> >> which sorted the regular files in decreasing access time order (most >> recent access near the top). The output suggests that the following >> files are also not needed for the build(s) (with the >> "ArmPkg/Library/ArmSoftFloatLib/SoftFloat-3e/" prefix stripped): >> >> 1 COPYING.txt >> 2 README.html >> 3 README.txt >> 4 build/Linux-386-GCC/Makefile >> 5 build/Linux-386-GCC/platform.h >> 6 build/Linux-386-SSE2-GCC/Makefile >> 7 build/Linux-386-SSE2-GCC/platform.h >> 8 build/Linux-ARM-VFPv2-GCC/Makefile >> 9 build/Linux-x86_64-GCC/Makefile >> 10 build/Linux-x86_64-GCC/platform.h >> 11 build/Win32-MinGW/Makefile >> 12 build/Win32-MinGW/platform.h >> 13 build/Win32-SSE2-MinGW/Makefile >> 14 build/Win32-SSE2-MinGW/platform.h >> 15 build/Win64-MinGW-w64/Makefile >> 16 build/Win64-MinGW-w64/platform.h >> 17 build/template-FAST_INT64/Makefile >> 18 build/template-FAST_INT64/platform.h >> 19 build/template-not-FAST_INT64/Makefile >> 20 build/template-not-FAST_INT64/platform.h >> 21 doc/SoftFloat-history.html >> 22 doc/SoftFloat-source.html >> 23 doc/SoftFloat.html >> 24 source/8086-SSE/extF80M_isSignalingNaN.c >> 25 source/8086-SSE/f128M_isSignalingNaN.c >> 26 source/8086-SSE/s_commonNaNToExtF80M.c >> 27 source/8086-SSE/s_commonNaNToExtF80UI.c >> 28 source/8086-SSE/s_commonNaNToF128M.c >> 29 source/8086-SSE/s_commonNaNToF128UI.c >> 30 source/8086-SSE/s_commonNaNToF16UI.c >> 31 source/8086-SSE/s_commonNaNToF32UI.c >> 32 source/8086-SSE/s_commonNaNToF64UI.c >> 33 source/8086-SSE/s_extF80MToCommonNaN.c >> 34 source/8086-SSE/s_extF80UIToCommonNaN.c >> 35 source/8086-SSE/s_f128MToCommonNaN.c >> 36 source/8086-SSE/s_f128UIToCommonNaN.c >> 37 source/8086-SSE/s_f16UIToCommonNaN.c >> 38 source/8086-SSE/s_f32UIToCommonNaN.c >> 39 source/8086-SSE/s_f64UIToCommonNaN.c >> 40 source/8086-SSE/s_propagateNaNExtF80M.c >> 41 source/8086-SSE/s_propagateNaNExtF80UI.c >> 42 source/8086-SSE/s_propagateNaNF128M.c >> 43 source/8086-SSE/s_propagateNaNF128UI.c >> 44 source/8086-SSE/s_propagateNaNF16UI.c >> 45 source/8086-SSE/s_propagateNaNF32UI.c >> 46 source/8086-SSE/s_propagateNaNF64UI.c >> 47 source/8086-SSE/softfloat_raiseFlags.c >> 48 source/8086-SSE/specialize.h >> 49 source/8086/extF80M_isSignalingNaN.c >> 50 source/8086/f128M_isSignalingNaN.c >> 51 source/8086/s_commonNaNToExtF80M.c >> 52 source/8086/s_commonNaNToExtF80UI.c >> 53 source/8086/s_commonNaNToF128M.c >> 54 source/8086/s_commonNaNToF128UI.c >> 55 source/8086/s_commonNaNToF16UI.c >> 56 source/8086/s_commonNaNToF32UI.c >> 57 source/8086/s_commonNaNToF64UI.c >> 58 source/8086/s_extF80MToCommonNaN.c >> 59 source/8086/s_extF80UIToCommonNaN.c >> 60 source/8086/s_f128MToCommonNaN.c >> 61 source/8086/s_f128UIToCommonNaN.c >> 62 source/8086/s_f16UIToCommonNaN.c >> 63 source/8086/s_f32UIToCommonNaN.c >> 64 source/8086/s_f64UIToCommonNaN.c >> 65 source/8086/s_propagateNaNExtF80M.c >> 66 source/8086/s_propagateNaNExtF80UI.c >> 67 source/8086/s_propagateNaNF128M.c >> 68 source/8086/s_propagateNaNF128UI.c >> 69 source/8086/s_propagateNaNF16UI.c >> 70 source/8086/s_propagateNaNF32UI.c >> 71 source/8086/s_propagateNaNF64UI.c >> 72 source/8086/softfloat_raiseFlags.c >> 73 source/8086/specialize.h >> 74 source/ARM-VFPv2-defaultNaN/extF80M_isSignalingNaN.c >> 75 source/ARM-VFPv2-defaultNaN/f128M_isSignalingNaN.c >> 76 source/ARM-VFPv2-defaultNaN/s_commonNaNToExtF80M.c >> 77 source/ARM-VFPv2-defaultNaN/s_commonNaNToExtF80UI.c >> 78 source/ARM-VFPv2-defaultNaN/s_commonNaNToF128M.c >> 79 source/ARM-VFPv2-defaultNaN/s_commonNaNToF128UI.c >> 80 source/ARM-VFPv2-defaultNaN/s_commonNaNToF16UI.c >> 81 source/ARM-VFPv2-defaultNaN/s_commonNaNToF32UI.c >> 82 source/ARM-VFPv2-defaultNaN/s_commonNaNToF64UI.c >> 83 source/ARM-VFPv2-defaultNaN/s_extF80MToCommonNaN.c >> 84 source/ARM-VFPv2-defaultNaN/s_extF80UIToCommonNaN.c >> 85 source/ARM-VFPv2-defaultNaN/s_f128MToCommonNaN.c >> 86 source/ARM-VFPv2-defaultNaN/s_f128UIToCommonNaN.c >> 87 source/ARM-VFPv2-defaultNaN/s_f16UIToCommonNaN.c >> 88 source/ARM-VFPv2-defaultNaN/s_f32UIToCommonNaN.c >> 89 source/ARM-VFPv2-defaultNaN/s_f64UIToCommonNaN.c >> 90 source/ARM-VFPv2-defaultNaN/s_propagateNaNExtF80M.c >> 91 source/ARM-VFPv2-defaultNaN/s_propagateNaNExtF80UI.c >> 92 source/ARM-VFPv2-defaultNaN/s_propagateNaNF128M.c >> 93 source/ARM-VFPv2-defaultNaN/s_propagateNaNF128UI.c >> 94 source/ARM-VFPv2-defaultNaN/s_propagateNaNF16UI.c >> 95 source/ARM-VFPv2-defaultNaN/s_propagateNaNF32UI.c >> 96 source/ARM-VFPv2-defaultNaN/s_propagateNaNF64UI.c >> 97 source/ARM-VFPv2-defaultNaN/softfloat_raiseFlags.c >> 98 source/ARM-VFPv2-defaultNaN/specialize.h >> 99 source/ARM-VFPv2/extF80M_isSignalingNaN.c >> 100 source/ARM-VFPv2/f128M_isSignalingNaN.c >> 101 source/ARM-VFPv2/s_commonNaNToExtF80M.c >> 102 source/ARM-VFPv2/s_commonNaNToExtF80UI.c >> 103 source/ARM-VFPv2/s_commonNaNToF128M.c >> 104 source/ARM-VFPv2/s_commonNaNToF128UI.c >> 105 source/ARM-VFPv2/s_commonNaNToF16UI.c >> 106 source/ARM-VFPv2/s_commonNaNToF32UI.c >> 107 source/ARM-VFPv2/s_commonNaNToF64UI.c >> 108 source/ARM-VFPv2/s_extF80MToCommonNaN.c >> 109 source/ARM-VFPv2/s_extF80UIToCommonNaN.c >> 110 source/ARM-VFPv2/s_f128MToCommonNaN.c >> 111 source/ARM-VFPv2/s_f128UIToCommonNaN.c >> 112 source/ARM-VFPv2/s_f16UIToCommonNaN.c >> 113 source/ARM-VFPv2/s_f32UIToCommonNaN.c >> 114 source/ARM-VFPv2/s_f64UIToCommonNaN.c >> 115 source/ARM-VFPv2/s_propagateNaNExtF80M.c >> 116 source/ARM-VFPv2/s_propagateNaNExtF80UI.c >> 117 source/ARM-VFPv2/s_propagateNaNF128M.c >> 118 source/ARM-VFPv2/s_propagateNaNF128UI.c >> 119 source/ARM-VFPv2/s_propagateNaNF16UI.c >> 120 source/ARM-VFPv2/s_propagateNaNF32UI.c >> >> (8) Should we remove the "build/" (lines 4 through 20) and "source/" >> (lines 24 through 120) subsets of this list, in patch #3? (Or maybe in a >> totally separate patch?) >> > > I'll let Leif chime in here. I'd be fine with removing them, not > adding them in the first place or leaving them where they are. For easy upgrades down the road, leaving these files in place, or at least removing them with a revertible stand-alone patch, would be better than never adding them. Anyway, I fully defer to you and Leif on this. Thanks! Laszlo