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

Bump bindgen and update bindings #439

Merged
merged 3 commits into from
Oct 5, 2023
Merged

Bump bindgen and update bindings #439

merged 3 commits into from
Oct 5, 2023

Conversation

gowthamsk-arm
Copy link
Contributor

Signed-off-by: Gowtham Suresh Kumar [email protected]

Superhepper
Superhepper reviewed Sep 29, 2023
View reviewed changes
tss-esapi-sys/src/bindings/x86_64-unknown-linux-gnu.rs Outdated
@@ -10798,20 +10774,6 @@ extern "C" {
nonceTPM: *mut *mut TPM2B_NONCE,
) -> TSS2_RC;
}
extern "C" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is something strange with these generated files. What version of TSS are these created from and what version do we intend to support in this branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just ran the regenerate-bindings.sh script which has TPM2_TSS_VERSION="2.3.3". Is there some external dependency that needs to be considered before generating the script?

Copy link
Member

@ionut-arm ionut-arm Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is unfortunate, it might've been a mistake when we last updated these, but in 2.3.3 these functions are not present. Perhaps we ran with 2.4.0? Given that we'll be doing a minor update on v7, maybe we can bump to the latest v2 of tpm2-tss, 2.4.6. What do you think?

Then we'll have additions to the API, but that's fine.

Superhepper
Superhepper reviewed Sep 29, 2023
View reviewed changes
tss-esapi-sys/src/bindings/x86_64-unknown-linux-gnu.rs Outdated
@@ -3498,13 +3474,13 @@ impl Default for TPMS_AUTH_RESPONSE {
}
pub type TPMI_AES_KEY_BITS = TPM2_KEY_BITS;
pub type TPMI_SM4_KEY_BITS = TPM2_KEY_BITS;
pub type TPMI_CAMELLIA_KEY_BITS = TPM2_KEY_BITS;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This kind of name change is a breaking change because these constants and names are exposed through the crate.

@gowthamsk-arm
Copy link
Contributor Author

The x86_64-unknown-linux-gnu.rs already was having some differences compared to the other 3 binding files.

  • TPMI_CAMELLIA_KEY_BITS is only present in the x86 linux binding file. The other bindings use TPMI_TPM2_CAMELLIA_KEY_BITS.
  • Esys_TR_GetTpmHandle, Esys_TRSess_GetAuthRequired and Esys_GetSysContext are only present for the x86 linux bindings.

Was this done intentionally by any chance?

@ionut-arm
Copy link
Member

@wiktor-k @Superhepper - any thoughts on moving to v2.4.6 of tpm2-tss?

@Superhepper
Copy link
Collaborator

None other then it would require a major version bump.

@wiktor-k
Copy link
Collaborator

wiktor-k commented Oct 2, 2023

@wiktor-k @Superhepper - any thoughts on moving to v2.4.6 of tpm2-tss?

In general I'm in favor of upgrading what is possible and in this case I don't see any downsides. Go head 👍

@ionut-arm
Copy link
Member

it would require a major version bump.

Why? I'm hoping we'd only have additions to the bindings, nothing removed (or if it would be removed, for example to rename from TPMI_CAMELLIA_KEY_BITS to TPMI_TPM2_CAMELLIA_KEY_BITS we can probably keep both), which would be compatible with a minor version bump.

@Superhepper
Copy link
Collaborator

If it is only an addition and it still works with the old version even if we do not support it then I guess no major version change is required.

@gowthamsk-arm
Copy link
Contributor Author

gowthamsk-arm commented Oct 3, 2023 •
edited
Loading

So after generating the new bindings with the 2.4.6 version of the library, I noticed that the x86 bindings were already generated for 2.4.6 and the rest of the bindings were not updated to 2.4.6 along with x86. This explains the discrepancies between the bindings.

The new patch updates the other bindings to 2.4.6. As discussed, I have manually edited the bindings file to not introduce any breaking change. But one thing that is a bit tricky is TPMI_TPM2_CAMELLIA_KEY_BITS. This has been changed to TPMI_CAMELLIA_KEY_BITS in the newer version. Currently I have just added the new definition and didn't change the struct which is using the old TPMI_TPM2_CAMELLIA_KEY_BITS.

pub union TPMU_SYM_KEY_BITS {
    pub aes: TPMI_AES_KEY_BITS,
    pub sm4: TPMI_SM4_KEY_BITS,
    pub camellia: TPMI_TPM2_CAMELLIA_KEY_BITS,
    pub sym: TPM2_KEY_BITS,
    pub exclusiveOr: TPMI_ALG_HASH,
}

@gowthamsk-arm gowthamsk-arm mentioned this pull request Oct 3, 2023
Merged
@gowthamsk-arm
Bump bindgen and generate bindings to 2.4.6 TSS library
e9954e4 
The x86_64-unknown-linux-gnu.rs bindings are already generated from 2.4.6 version
of the library. This patch generates the other target bindings for the same version.
In order to avoid any breaking changes, the new bindings generated are manually
edited.

Bidgen is also updated to the 0.66.1 which introduces minor changes.

Signed-off-by: Gowtham Suresh Kumar <[email protected]>
@gowthamsk-arm
Update TSS version in dockerfile and script
263a31d 
Signed-off-by: Gowtham Suresh Kumar <[email protected]>
@tgonzalezorlandoarm tgonzalezorlandoarm self-requested a review October 3, 2023 14:02
tgonzalezorlandoarm
tgonzalezorlandoarm approved these changes Oct 3, 2023
View reviewed changes
Copy link
Member

@tgonzalezorlandoarm tgonzalezorlandoarm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, Thanks!

@gowthamsk-arm gowthamsk-arm force-pushed the bump_bindgen_7 branch 2 times, most recently from 7240cec to 5f85c85 Compare October 3, 2023 14:47
@gowthamsk-arm
Copy link
Contributor Author

Any idea why the ubuntu test is failing?

./configure: line 17494: JSON_C_LIBS: command not found
checking for JSON_C... no
/tmp/rust-tss-esapi/tss-esapi/tests/pkg-config: 4: /tmp/rust-tss-esapi/tss-esapi/tests/pkg-config: SYSROOT: not found
configure: error: Package requirements (json-c) were not met:

/tmp/rust-tss-esapi/tss-esapi/tests/pkg-config: 4: /tmp/rust-tss-esapi/tss-esapi/tests/pkg-config: SYSROOT: not found
No package 'json-c' found

This happens for tpm2-tss 2.4.6 cross compilation.
Should I use another docker base image for ubuntucontainer?

@tgonzalezorlandoarm
Copy link
Member

@Superhepper CI is passing now and I think all issues are resolved

@gowthamsk-arm
Copy link
Contributor Author

@ionut-arm @wiktor-k @Superhepper can I get a review, please? :)

Superhepper
Superhepper approved these changes Oct 4, 2023
View reviewed changes
Copy link
Collaborator

@Superhepper Superhepper left a comment

Choose a reason for hiding this comment

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

LGTM

@tgonzalezorlandoarm tgonzalezorlandoarm merged commit aa3c821 into parallaxsecond:7.x.y Oct 5, 2023
9 checks passed
@gowthamsk-arm gowthamsk-arm deleted the bump_bindgen_7 branch October 6, 2023 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ionut-arm ionut-arm ionut-arm left review comments

@tgonzalezorlandoarm tgonzalezorlandoarm tgonzalezorlandoarm approved these changes

@Superhepper Superhepper Superhepper approved these changes

Assignees

@gowthamsk-arm gowthamsk-arm

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gowthamsk-arm @ionut-arm @Superhepper @wiktor-k @tgonzalezorlandoarm