Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Modify HW accelerator drivers to new error code #8643

Merged
merged 1 commit into from
Nov 26, 2018
Merged

Modify HW accelerator drivers to new error code #8643

merged 1 commit into from
Nov 26, 2018

Conversation

RonEld
Copy link
Contributor

@RonEld RonEld commented Nov 5, 2018

Description

Have the HW accelearation drivers return the platform module
errors about feature unsupported, and hw acceleration failure,
because the moduel specific errors will be removed.
Pending Mbed-TLS/mbedtls#2054 to be merged to the Mbed TLS repository, and for Mbed TLS to be updated in Mbed OS

CC @Patater @sbutcher-arm

Pull request type

[ ] Fix
[ ] Refactor
[x] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

@RonEld RonEld mentioned this pull request Nov 5, 2018
Merged
@RonEld RonEld force-pushed the feature_unsupported_error_support branch from 68ddf50 to 0d8e466 Compare November 6, 2018 12:18
@cmonr
Copy link
Contributor

cmonr commented Nov 9, 2018

@RonEld Is ARMmbed/mbed-os-example-tls#211 needed to put this through CI, or is the label just indicating that it should go in first?

@cmonr
Copy link
Contributor

cmonr commented Nov 9, 2018

@Patater @sbutcher-arm When y'all get a chance for a review.

RonEld
RonEld commented Nov 11, 2018
View reviewed changes
Copy link
Contributor Author

@RonEld RonEld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to rename the error codes from the deprecated one to the new on

features/cryptocell/FEATURE_CRYPTOCELL310/cc_internal.c Outdated
@@ -146,7 +147,7 @@ int convert_CrysError_to_mbedtls_err( CRYSError_t Crys_err )
return ( MBEDTLS_ERR_ECP_INVALID_KEY );

default:
return ( MBEDTLS_ERR_ECP_HW_ACCEL_FAILED );
return ( MBEDTLS_ERR_PLATFORM_HW_FAILED );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED

features/cryptocell/FEATURE_CRYPTOCELL310/cc_internal.h Outdated
@@ -122,7 +122,7 @@ uint32_t convert_mbedtls_to_cc_rand( void* mbedtls_rand, uint16_t outSizeBytes,
*
*
* \return \c The corresponding Mbed TLS error,
* MBEDTLS_ERR_ECP_HW_ACCEL_FAILED as default, if none found
* MBEDTLS_ERR_PLATFORM_HW_FAILED as default, if none found
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED

features/cryptocell/FEATURE_CRYPTOCELL310/ccm_alt.c Outdated
@@ -93,7 +93,7 @@ int mbedtls_ccm_encrypt_and_tag( mbedtls_ccm_context *ctx, size_t length,
CrysRet = CRYS_AESCCM( SASI_AES_ENCRYPT, ctx->cipher_key, ctx->keySize_ID,(uint8_t*)iv, iv_len,
(uint8_t*)add, add_len, (uint8_t*)input, length, output, tag_len, tag );
if( CrysRet != CRYS_OK )
return ( MBEDTLS_ERR_CCM_HW_ACCEL_FAILED );
return ( MBEDTLS_ERR_PLATFORM_HW_FAILED );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED

features/cryptocell/FEATURE_CRYPTOCELL310/ccm_alt.c Outdated
@@ -130,7 +130,7 @@ int mbedtls_ccm_auth_decrypt( mbedtls_ccm_context *ctx, size_t length,
CrysRet = CRYS_AESCCM( SASI_AES_DECRYPT, ctx->cipher_key, ctx->keySize_ID,(uint8_t*)iv, iv_len,
(uint8_t*)add, add_len, (uint8_t*)input, length, output, tag_len, (uint8_t*)tag );
if ( CrysRet != CRYS_OK )
return ( MBEDTLS_ERR_CCM_HW_ACCEL_FAILED );
return ( MBEDTLS_ERR_PLATFORM_HW_FAILED );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED

features/cryptocell/FEATURE_CRYPTOCELL310/sha1_alt.c Outdated
@@ -47,7 +47,7 @@ void mbedtls_sha1_clone( mbedtls_sha1_context *dst,
int mbedtls_sha1_starts_ret( mbedtls_sha1_context *ctx )
{
if( CRYS_HASH_Init( &ctx->crys_hash_ctx, CRYS_HASH_SHA1_mode ) != CRYS_OK )
return ( MBEDTLS_ERR_SHA1_HW_ACCEL_FAILED );
return ( MBEDTLS_ERR_PLATFORM_HW_FAILED );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED

features/cryptocell/FEATURE_CRYPTOCELL310/sha1_alt.c Outdated
}

int mbedtls_internal_sha1_process( mbedtls_sha1_context *ctx,
const unsigned char data[64] )
{
if( CRYS_HASH_Update( &ctx->crys_hash_ctx, (uint8_t*)data, 64 ) != CRYS_OK )
return ( MBEDTLS_ERR_SHA1_HW_ACCEL_FAILED );
return ( MBEDTLS_ERR_PLATFORM_HW_FAILED );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED

features/cryptocell/FEATURE_CRYPTOCELL310/sha256_alt.c Outdated
@@ -47,15 +47,15 @@ int mbedtls_sha256_starts_ret( mbedtls_sha256_context *ctx, int is224 )
{
if(CRYS_HASH_Init( &ctx->crys_hash_ctx, is224 ?
CRYS_HASH_SHA224_mode : CRYS_HASH_SHA256_mode ) != CRYS_OK )
return ( MBEDTLS_ERR_SHA256_HW_ACCEL_FAILED );
return ( MBEDTLS_ERR_PLATFORM_HW_FAILED );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED

features/cryptocell/FEATURE_CRYPTOCELL310/sha256_alt.c Outdated
return ( 0 );
}

int mbedtls_internal_sha256_process( mbedtls_sha256_context *ctx,
const unsigned char data[64] )
{
if( CRYS_HASH_Update( &ctx->crys_hash_ctx, (uint8_t*)data, 64 ) != CRYS_OK )
return ( MBEDTLS_ERR_SHA256_HW_ACCEL_FAILED );
return ( MBEDTLS_ERR_PLATFORM_HW_FAILED );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED

features/cryptocell/FEATURE_CRYPTOCELL310/sha256_alt.c Outdated
@@ -64,7 +64,7 @@ int mbedtls_sha256_update_ret( mbedtls_sha256_context *ctx,
size_t ilen )
{
if( CRYS_HASH_Update( &ctx->crys_hash_ctx, (uint8_t*)input, ilen ) != CRYS_OK )
return ( MBEDTLS_ERR_SHA256_HW_ACCEL_FAILED );
return ( MBEDTLS_ERR_PLATFORM_HW_FAILED );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED

features/cryptocell/FEATURE_CRYPTOCELL310/sha256_alt.c Outdated
@@ -80,7 +80,7 @@ int mbedtls_sha256_finish_ret( mbedtls_sha256_context *ctx,
return ( 0 );
}
else
return ( MBEDTLS_ERR_SHA256_HW_ACCEL_FAILED );
return ( MBEDTLS_ERR_PLATFORM_HW_FAILED );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED

@RonEld
Copy link
Contributor Author

RonEld commented Nov 11, 2018

@cmonr ARMmbed/mbed-os-example-tls#211 is dependent on this PR for the test to pass,( and Mbed-TLS/mbedtls#2054 for the build to pass ), so I guess we can pass it through CI

This PR is dependent on Mbed-TLS/mbedtls#2054 to build, however, since 2054 has changed, and is not as strict as it was before ( previously it removed the old error codes, now it mostly adds error codes), this PR will not fix any build failures, once the new Mbed TLS release will be merged, but it only improves to the correct implementation.

The label "needs: preceding PR" for this PR, is that it requires Mbed-TLS/mbedtls#2054 to be merged to Mbed TLS, and for the Mbed TLS release to be merged into Mbed OS.

BTW, I noticed a few mistakes in this Pr, since we deprecated an error code. I will amend my commit

Modify HW accelerator drivers to new error code
9f4e752 
Have the HW accelearation drivers return the platform module
errors about feature unsupported, and hw acceleration failure,
because the moduel specific errors will be removed.
@RonEld RonEld force-pushed the feature_unsupported_error_support branch from 0d8e466 to 9f4e752 Compare November 11, 2018 07:59
@RonEld
Copy link
Contributor Author

RonEld commented Nov 11, 2018

I amended my previous commit to fix the comments I added.

@cmonr Do you think we should have the partners review the relevant changes to their drivers?

@RonEld RonEld mentioned this pull request Nov 11, 2018
Merged
@cmonr
Copy link
Contributor

cmonr commented Nov 12, 2018

@RonEld I would think so, but this is also only a one line change, so I'm not entirely sure.

@MarceloSalazar @ashok-rao @screamerbg Thoughts? How much code changes to a partner's code can be made before they should be brought into the loop?

@MarceloSalazar
Copy link

It looks like a minor but breaking change. IMO should go to 5.11.0

@RonEld RonEld mentioned this pull request Nov 13, 2018
Merged
@adbridge
Copy link
Contributor

@Patater @sbutcher-arm This has been waiting on review for 9 days could you please take a look asap?

@NirSonnenschein
Copy link
Contributor

Reminder @Patater @sbutcher-arm This has been waiting on review for 13 days could you please take a look asap?

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 19, 2018

To summarize (I reread the thread to understand again why preceding PR is still valid):

The label "needs: preceding PR" for this PR, is that it requires Mbed-TLS/mbedtls#2054 to be merged to Mbed TLS, and for the Mbed TLS release to be merged into Mbed OS.

This is waiting for Mbed TLS version update. Plus reviewers approvals.

@RonEld
Copy link
Contributor Author

RonEld commented Nov 19, 2018

This is waiting for Mbed TLS version update.

Yes. This PR uses error codes that will be introduced only after the Mbed TLS update. If this PR will get merged earlier, build will break for these targets.

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 20, 2018

While we are waiting for preceding PR, can reviewers proceed with reviews please?

@cmonr
Copy link
Contributor

cmonr commented Nov 22, 2018

Restarting CI

@mbed-ci
Copy link

mbed-ci commented Nov 22, 2018

Test run: SUCCESS

Summary: 4 of 4 test jobs passed
Build number : 5
Build artifacts
Build logs

@RonEld
Copy link
Contributor Author

RonEld commented Nov 22, 2018

I am not sure why the CI failed, but I don't expect the build to succeed on several platforms, as the changes uses error codes that will be introduced only after Mbed TLS is updated

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 22, 2018

It was triggered automatically. We will wait until the preceding PR goes in

@RonEld
Copy link
Contributor Author

RonEld commented Nov 22, 2018

Sure no problem, it is just strange to me how the build succeeded.
Are the changed targets not part of the CI?

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 22, 2018

@OPpuolitaival can you review ^^

@RonEld
Copy link
Contributor Author

RonEld commented Nov 25, 2018

Since #8859 has been merged, we can remove the "needs:preceding PR" label.

@0xc0170 @cmonr @NirSonnenschein Can we start the CI?
We already have approval from @Patater

NirSonnenschein
NirSonnenschein approved these changes Nov 25, 2018
View reviewed changes
Copy link
Contributor

@NirSonnenschein NirSonnenschein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me

@NirSonnenschein
Copy link
Contributor

@RonEld : yes, removing preceding PR is fine.
I've just reviewed this, so it has the two required reviews -> moved to needs CI.
there are quite a few jobs waiting for CI, we are trying to get as many as possible in, so I can't guaranteed when it will start CI.

@mbed-ci
Copy link

mbed-ci commented Nov 25, 2018

Test run: SUCCESS

Summary: 4 of 4 test jobs passed
Build number : 6
Build artifacts
Build logs

@0xc0170 0xc0170 merged commit 7a77e66 into ARMmbed:master Nov 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Patater Patater Patater approved these changes

@NirSonnenschein NirSonnenschein NirSonnenschein approved these changes

@simonbutcher simonbutcher Awaiting requested review from simonbutcher

Assignees
No one assigned
Labels
release-version: 5.11.0-rc1 risk: A
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@RonEld @cmonr @MarceloSalazar @adbridge @NirSonnenschein @0xc0170 @mbed-ci @Patater