From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=J6X8Iak8; spf=pass (domain: linaro.org, ip: 209.85.166.194, mailfrom: ard.biesheuvel@linaro.org) Received: from mail-it1-f194.google.com (mail-it1-f194.google.com [209.85.166.194]) by groups.io with SMTP; Fri, 24 May 2019 14:32:53 -0700 Received: by mail-it1-f194.google.com with SMTP id h11so15827307itf.5 for ; Fri, 24 May 2019 14:32:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=ATzeJ7OoKTlSMKaJFNypfUKjmNpeAQ2W8ZCLu3HxTMM=; b=J6X8Iak87B9GMlvi9XsgQODMN2s2gblRrurVlw9h5aiXSyqpam3ozEmvwt6w526keH LIl7ytuSLxZDNolE8KbD7C0Kibry0fp8H5mW/mXuDQqNN4TdK/1+AQg5dmEsewUb2sV7 YW04zIv475AdfchEUZm7zYIzJxvsnWomAXE7uZ4GU+9E1V1wK4F5/Z7vqAdtoW/S+/QD UryF/xG68Al9Knz6M/oB4FdNOirR74xR4GxsReZ0YwcJg4wcUOnvPkHfkgreQMXLEK/3 8JU4UhCt6VjAO1jNk26rznHoaVnQf4iMNNuE1z2ObVHRiS7crzlZoepHjhmUN+jAjhVj 1WLw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=ATzeJ7OoKTlSMKaJFNypfUKjmNpeAQ2W8ZCLu3HxTMM=; b=VtK25oWAaJMD4Fy0qEUa7jeW1dGbqrdsTaPAkymgr0ZpXBG8omYq9tLAgvrleX88xP Lj5SqZK55Xj460k6dIGeE2FRdl2XKmdQbL5eXq49Xo+UPeRu34g/DhVLXMUeCpedkBsk Py4zNROwdgp9GwIEEeVNKhzqjFnSZUh4/H69pbwKO/6HwlcrUXn0hscNxDsOMVdys35z tZ/6RgEHp0eMMIaQsZuGyD2hqOSp9JskpC2tcwXAbu5hHSW4p/EYVklw59zNZdtnc3YQ 6mXMnGROY0ItVvSfH0u5BX/hVKWcE3dt+SFd5fCfOhbHF1SvirTSh2iRHlOJevK+KWpv zXMQ== X-Gm-Message-State: APjAAAVW5J97ZRO+IKI0Ta0Nu6D76XC6f68hyzIZVgk6YE6bSDBkc3h5 pT3wSEE/+HvPxPA9KXrWkJ8H4FanTFaJEMVwI9JUkg== X-Google-Smtp-Source: APXvYqzYZCN4X+Aobbc0zTr83sIH9qcYfFkKyy9/y5JLNj1jErU2zs2fbyhyO4KWtaRIekqvgCbARV5bWpL9eoszTRo= X-Received: by 2002:a02:a494:: with SMTP id d20mr5150640jam.62.1558733572774; Fri, 24 May 2019 14:32:52 -0700 (PDT) MIME-Version: 1.0 References: <20190524151140.23539-1-ard.biesheuvel@linaro.org> In-Reply-To: From: "Ard Biesheuvel" Date: Fri, 24 May 2019 23:32:39 +0200 Message-ID: Subject: Re: [edk2-devel] [PATCH 0/3] update ArmSoftFloatLib to latest upstream version To: Laszlo Ersek Cc: edk2-devel-groups-io , "Gao, Liming" , "Wang, Jian J" , Leif Lindholm , Michael D Kinney Content-Type: text/plain; charset="UTF-8" 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. > (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. > > 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 > > - 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. > > > > 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. > > 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.