[hardknott 0/2] tune-cortexa72: Drop the crypto extension from the cortexa72 tune

[hardknott 0/2] tune-cortexa72: Drop the crypto extension from the cortexa72 tune

[Kevin Hao]

Hi,

This patch series backport two patches from the master branch to fix the
crash issue on some cortexa72 boards due to the crypto intructions are
used.

Jagadeesh Krishnanjanappa (1):
  tune-cortexa72: remove crypto for the default cortex-a72

Kevin Hao (1):
  tune-cortexa72: Enable the crc extension by default for cortexa72

 meta/conf/machine/include/tune-cortexa72.inc | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

-- 
2.31.1

[hardknott 1/2] tune-cortexa72: remove crypto for the default cortex-a72

[Kevin Hao]

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"

(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"
-- 
2.31.1

Re: [hardknott 1/2] tune-cortexa72: remove crypto for the default cortex-a72

[Mittal, Anuj]

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"

Re: [hardknott 1/2] tune-cortexa72: remove crypto for the default cortex-a72

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 3937 bytes --]

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"
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [OE-core] [hardknott 1/2] tune-cortexa72: remove crypto for the default cortex-a72

[Richard Purdie]

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

Re: [OE-core] [hardknott 1/2] tune-cortexa72: remove crypto for the default cortex-a72

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 2011 bytes --]

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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[hardknott 2/2] tune-cortexa72: Enable the crc extension by default for cortexa72

[Kevin Hao]

The crc extension is optional for the ARMv8.0 but is mandatory for the
cortexa72, so there is no reason not to enable it for the cortexa72
tune. With this change, the cortexa72-crc seems redundant. But we
had better to keep it to be compatible with the BSP which already used
that tune.

(From OE-Core rev: ca50267ab568d2f688844cb7c6cd867ed34168db)

Signed-off-by: Kevin Hao <kexin.hao@windriver.com>
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 | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/meta/conf/machine/include/tune-cortexa72.inc b/meta/conf/machine/include/tune-cortexa72.inc
index 7608a20c43f4..30480efd8328 100644
--- a/meta/conf/machine/include/tune-cortexa72.inc
+++ b/meta/conf/machine/include/tune-cortexa72.inc
@@ -10,12 +10,12 @@ AVAILTUNES += "cortexa72 cortexa72-crc cortexa72-crc-crypto"
 ARMPKGARCH_tune-cortexa72             = "cortexa72"
 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"
+TUNE_FEATURES_tune-cortexa72          = "${TUNE_FEATURES_tune-armv8a-crc} cortexa72"
+TUNE_FEATURES_tune-cortexa72-crc      = "${TUNE_FEATURES_tune-cortexa72}"
+TUNE_FEATURES_tune-cortexa72-crc-crypto   = "${TUNE_FEATURES_tune-cortexa72} crypto"
+PACKAGE_EXTRA_ARCHS_tune-cortexa72        = "${PACKAGE_EXTRA_ARCHS_tune-armv8-crc} cortexa72"
+PACKAGE_EXTRA_ARCHS_tune-cortexa72-crc    = "${PACKAGE_EXTRA_ARCHS_tune-armv8a-crc} cortexa72"
+PACKAGE_EXTRA_ARCHS_tune-cortexa72-crc-crypto    = "${PACKAGE_EXTRA_ARCHS_tune-armv8a-crc-crypto} cortexa72 cortexa72-crc-crypto"
 BASE_LIB_tune-cortexa72            = "lib64"
 BASE_LIB_tune-cortexa72-crc        = "lib64"
 BASE_LIB_tune-cortexa72-crc-crypto = "lib64"
-- 
2.31.1