| Message ID | 20220115023747.1154635-2-kexin.hao@windriver.com |
|---|---|
| State | Accepted, archived |
| Commit | ee249ca4d5020c16100e07ee1b5974237de29e29 |
| Headers | show |
| Series | tune-cortexa72: Drop the crypto extension from the cortexa72 tune | expand |
On Sat, 2022-01-15 at 10:37 +0800, Kevin Hao wrote: > From: Jagadeesh Krishnanjanappa <workjagadeesh@gmail.com> > > The cryptographic unit is optional for the Cortex-A72, but it was > included by default previously. This breaks building systems that > lack this functionality when using tune-cortexa72.inc. > > To correct this, add a crypto entry in the tune file. Since CRC is > optional for ARMv8.0, do the same thing while we're at it. > > For platforms that had been happily using tune-cortexa72.inc, a > slight > degradation of performance will occur using the default. To correct > this, simply add: > DEFAULTTUNE = "cortexa72-crc-crypto" > I am not very familiar with ARM tunes but it sounds like you are changing behaviour for people who might already be using these tunes with these two patches ... That might not be suitable for stable branches. Perhaps Ross, Jon or others can comment if this is something that should be merged in stable branches. Thanks, Anuj > (From OE-Core rev: 2568d537087adb0b592aa250bf628a7b48c3a9d3) > > Signed-off-by: Jagadeesh Krishnanjanappa <workjagadeesh@gmail.com> > Signed-off-by: Jon Mason <jdmason@kudzu.us> (rewording commit > message) > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org> > [Kevin: Convert to the old style override syntax] > Signed-off-by: Kevin Hao <kexin.hao@windriver.com> > --- > meta/conf/machine/include/tune-cortexa72.inc | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/meta/conf/machine/include/tune-cortexa72.inc > b/meta/conf/machine/include/tune-cortexa72.inc > index b3f68ab6e3be..7608a20c43f4 100644 > --- a/meta/conf/machine/include/tune-cortexa72.inc > +++ b/meta/conf/machine/include/tune-cortexa72.inc > @@ -6,8 +6,16 @@ TUNE_CCARGS .= > "${@bb.utils.contains('TUNE_FEATURES', 'cortexa72', ' -mcpu=corte > require conf/machine/include/arm/arch-armv8a.inc > > # Little Endian base configs > -AVAILTUNES += "cortexa72" > +AVAILTUNES += "cortexa72 cortexa72-crc cortexa72-crc-crypto" > ARMPKGARCH_tune-cortexa72 = "cortexa72" > -TUNE_FEATURES_tune-cortexa72 = "${TUNE_FEATURES_tune- > armv8a-crc-crypto} cortexa72" > -PACKAGE_EXTRA_ARCHS_tune-cortexa72 = "${PACKAGE_EXTRA_ARCHS_tune- > armv8a-crc-crypto} cortexa72" > -BASE_LIB_tune-cortexa72 = "lib64" > +ARMPKGARCH_tune-cortexa72-crc = "cortexa72" > +ARMPKGARCH_tune-cortexa72-crc-crypto = "cortexa72" > +TUNE_FEATURES_tune-cortexa72 = "${TUNE_FEATURES_tune- > armv8a} cortexa72" > +TUNE_FEATURES_tune-cortexa72-crc = "${TUNE_FEATURES_tune- > cortexa72} crc" > +TUNE_FEATURES_tune-cortexa72-crc-crypto = "${TUNE_FEATURES_tune- > cortexa72} crc crypto" > +PACKAGE_EXTRA_ARCHS_tune-cortexa72 = > "${PACKAGE_EXTRA_ARCHS_tune-armv8} cortexa72" > +PACKAGE_EXTRA_ARCHS_tune-cortexa72-crc = > "${PACKAGE_EXTRA_ARCHS_tune-armv8a-crc} cortexa72 cortexa72-crc" > +PACKAGE_EXTRA_ARCHS_tune-cortexa72-crc-crypto = > "${PACKAGE_EXTRA_ARCHS_tune-armv8a-crc-crypto} cortexa72 cortexa72- > crc cortexa72-crc-crypto" > +BASE_LIB_tune-cortexa72 = "lib64" > +BASE_LIB_tune-cortexa72-crc = "lib64" > +BASE_LIB_tune-cortexa72-crc-crypto = "lib64"
On Sat, Jan 15, 2022 at 02:53:09AM +0000, Mittal, Anuj wrote: > [Please note: This e-mail is from an EXTERNAL e-mail address] > > On Sat, 2022-01-15 at 10:37 +0800, Kevin Hao wrote: > > From: Jagadeesh Krishnanjanappa <workjagadeesh@gmail.com> > > > > The cryptographic unit is optional for the Cortex-A72, but it was > > included by default previously. This breaks building systems that > > lack this functionality when using tune-cortexa72.inc. > > > > To correct this, add a crypto entry in the tune file. Since CRC is > > optional for ARMv8.0, do the same thing while we're at it. > > > > For platforms that had been happily using tune-cortexa72.inc, a > > slight > > degradation of performance will occur using the default. To correct > > this, simply add: > > DEFAULTTUNE = "cortexa72-crc-crypto" > > > > I am not very familiar with ARM tunes but it sounds like you are > changing behaviour for people who might already be using these tunes > with these two patches ... That might not be suitable for stable > branches. These two patches fix the application crash bug on the boards which have a cortexa72 core but don't have a crypto unit. IMHO they are definitely suitable for the stable branch. Yes, for the boards which do have a crypto unit, they would have to update their default tune to avoid the slight the degradation of performance. Thanks, Kevin > > Perhaps Ross, Jon or others can comment if this is something that > should be merged in stable branches. > > Thanks, > > Anuj > > > (From OE-Core rev: 2568d537087adb0b592aa250bf628a7b48c3a9d3) > > > > Signed-off-by: Jagadeesh Krishnanjanappa <workjagadeesh@gmail.com> > > Signed-off-by: Jon Mason <jdmason@kudzu.us> (rewording commit > > message) > > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org> > > [Kevin: Convert to the old style override syntax] > > Signed-off-by: Kevin Hao <kexin.hao@windriver.com> > > --- > > meta/conf/machine/include/tune-cortexa72.inc | 16 ++++++++++++---- > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > diff --git a/meta/conf/machine/include/tune-cortexa72.inc > > b/meta/conf/machine/include/tune-cortexa72.inc > > index b3f68ab6e3be..7608a20c43f4 100644 > > --- a/meta/conf/machine/include/tune-cortexa72.inc > > +++ b/meta/conf/machine/include/tune-cortexa72.inc > > @@ -6,8 +6,16 @@ TUNE_CCARGS .= > > "${@bb.utils.contains('TUNE_FEATURES', 'cortexa72', ' -mcpu=corte > > require conf/machine/include/arm/arch-armv8a.inc > > > > # Little Endian base configs > > -AVAILTUNES += "cortexa72" > > +AVAILTUNES += "cortexa72 cortexa72-crc cortexa72-crc-crypto" > > ARMPKGARCH_tune-cortexa72 = "cortexa72" > > -TUNE_FEATURES_tune-cortexa72 = "${TUNE_FEATURES_tune- > > armv8a-crc-crypto} cortexa72" > > -PACKAGE_EXTRA_ARCHS_tune-cortexa72 = "${PACKAGE_EXTRA_ARCHS_tune- > > armv8a-crc-crypto} cortexa72" > > -BASE_LIB_tune-cortexa72 = "lib64" > > +ARMPKGARCH_tune-cortexa72-crc = "cortexa72" > > +ARMPKGARCH_tune-cortexa72-crc-crypto = "cortexa72" > > +TUNE_FEATURES_tune-cortexa72 = "${TUNE_FEATURES_tune- > > armv8a} cortexa72" > > +TUNE_FEATURES_tune-cortexa72-crc = "${TUNE_FEATURES_tune- > > cortexa72} crc" > > +TUNE_FEATURES_tune-cortexa72-crc-crypto = "${TUNE_FEATURES_tune- > > cortexa72} crc crypto" > > +PACKAGE_EXTRA_ARCHS_tune-cortexa72 = > > "${PACKAGE_EXTRA_ARCHS_tune-armv8} cortexa72" > > +PACKAGE_EXTRA_ARCHS_tune-cortexa72-crc = > > "${PACKAGE_EXTRA_ARCHS_tune-armv8a-crc} cortexa72 cortexa72-crc" > > +PACKAGE_EXTRA_ARCHS_tune-cortexa72-crc-crypto = > > "${PACKAGE_EXTRA_ARCHS_tune-armv8a-crc-crypto} cortexa72 cortexa72- > > crc cortexa72-crc-crypto" > > +BASE_LIB_tune-cortexa72 = "lib64" > > +BASE_LIB_tune-cortexa72-crc = "lib64" > > +BASE_LIB_tune-cortexa72-crc-crypto = "lib64" >
On Sat, 2022-01-15 at 02:53 +0000, Anuj Mittal wrote: > On Sat, 2022-01-15 at 10:37 +0800, Kevin Hao wrote: > > From: Jagadeesh Krishnanjanappa <workjagadeesh@gmail.com> > > > > The cryptographic unit is optional for the Cortex-A72, but it was > > included by default previously. This breaks building systems that > > lack this functionality when using tune-cortexa72.inc. > > > > To correct this, add a crypto entry in the tune file. Since CRC is > > optional for ARMv8.0, do the same thing while we're at it. > > > > For platforms that had been happily using tune-cortexa72.inc, a > > slight > > degradation of performance will occur using the default. To correct > > this, simply add: > > DEFAULTTUNE = "cortexa72-crc-crypto" > > > > I am not very familiar with ARM tunes but it sounds like you are > changing behaviour for people who might already be using these tunes > with these two patches ... That might not be suitable for stable > branches. > > Perhaps Ross, Jon or others can comment if this is something that > should be merged in stable branches. You're right to be nervous of the change, I was with master. Having looked into the issue, it is a serious one and probably should be fixed. That said, I think there is perhaps a patch missing from this series, the one which removes the cortexa72-crc tune. I think this is fine even for a stable series since the patch adding it is also in this series. The patches do make more sense once looked at as a whole, we just didn't get this quite right for master at first. Cheers, Richard
On Sun, Jan 16, 2022 at 12:01:16PM +0000, Richard Purdie wrote: > [Please note: This e-mail is from an EXTERNAL e-mail address] > > On Sat, 2022-01-15 at 02:53 +0000, Anuj Mittal wrote: > > On Sat, 2022-01-15 at 10:37 +0800, Kevin Hao wrote: > > > From: Jagadeesh Krishnanjanappa <workjagadeesh@gmail.com> > > > > > > The cryptographic unit is optional for the Cortex-A72, but it was > > > included by default previously. This breaks building systems that > > > lack this functionality when using tune-cortexa72.inc. > > > > > > To correct this, add a crypto entry in the tune file. Since CRC is > > > optional for ARMv8.0, do the same thing while we're at it. > > > > > > For platforms that had been happily using tune-cortexa72.inc, a > > > slight > > > degradation of performance will occur using the default. To correct > > > this, simply add: > > > DEFAULTTUNE = "cortexa72-crc-crypto" > > > > > > > I am not very familiar with ARM tunes but it sounds like you are > > changing behaviour for people who might already be using these tunes > > with these two patches ... That might not be suitable for stable > > branches. > > > > Perhaps Ross, Jon or others can comment if this is something that > > should be merged in stable branches. > > You're right to be nervous of the change, I was with master. Having looked into > the issue, it is a serious one and probably should be fixed. > > That said, I think there is perhaps a patch missing from this series, the one > which removes the cortexa72-crc tune. I dropped that patch intentionally since it doesn't fix any real issue and may be not suitable for the stable branch. > I think this is fine even for a stable > series since the patch adding it is also in this series. The patches do make > more sense once looked at as a whole, Faire enough, I will send a v2 to include that patch. Thanks, Kevin > we just didn't get this quite right for > master at first. > > Cheers, > > Richard >
diff --git a/meta/conf/machine/include/tune-cortexa72.inc b/meta/conf/machine/include/tune-cortexa72.inc index b3f68ab6e3be..7608a20c43f4 100644 --- a/meta/conf/machine/include/tune-cortexa72.inc +++ b/meta/conf/machine/include/tune-cortexa72.inc @@ -6,8 +6,16 @@ TUNE_CCARGS .= "${@bb.utils.contains('TUNE_FEATURES', 'cortexa72', ' -mcpu=corte require conf/machine/include/arm/arch-armv8a.inc # Little Endian base configs -AVAILTUNES += "cortexa72" +AVAILTUNES += "cortexa72 cortexa72-crc cortexa72-crc-crypto" ARMPKGARCH_tune-cortexa72 = "cortexa72" -TUNE_FEATURES_tune-cortexa72 = "${TUNE_FEATURES_tune-armv8a-crc-crypto} cortexa72" -PACKAGE_EXTRA_ARCHS_tune-cortexa72 = "${PACKAGE_EXTRA_ARCHS_tune-armv8a-crc-crypto} cortexa72" -BASE_LIB_tune-cortexa72 = "lib64" +ARMPKGARCH_tune-cortexa72-crc = "cortexa72" +ARMPKGARCH_tune-cortexa72-crc-crypto = "cortexa72" +TUNE_FEATURES_tune-cortexa72 = "${TUNE_FEATURES_tune-armv8a} cortexa72" +TUNE_FEATURES_tune-cortexa72-crc = "${TUNE_FEATURES_tune-cortexa72} crc" +TUNE_FEATURES_tune-cortexa72-crc-crypto = "${TUNE_FEATURES_tune-cortexa72} crc crypto" +PACKAGE_EXTRA_ARCHS_tune-cortexa72 = "${PACKAGE_EXTRA_ARCHS_tune-armv8} cortexa72" +PACKAGE_EXTRA_ARCHS_tune-cortexa72-crc = "${PACKAGE_EXTRA_ARCHS_tune-armv8a-crc} cortexa72 cortexa72-crc" +PACKAGE_EXTRA_ARCHS_tune-cortexa72-crc-crypto = "${PACKAGE_EXTRA_ARCHS_tune-armv8a-crc-crypto} cortexa72 cortexa72-crc cortexa72-crc-crypto" +BASE_LIB_tune-cortexa72 = "lib64" +BASE_LIB_tune-cortexa72-crc = "lib64" +BASE_LIB_tune-cortexa72-crc-crypto = "lib64"