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.

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

Implement API for requesting/retrieving Caliptra's IDEVID CSR. #1583

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rusty1968
Copy link
Contributor

This commit adds functionality to request and retrieve an IDEVID Certificate Signing Request.

@rusty1968 rusty1968 changed the title Implement API for requesting IDEVID CSR. Implement API for requesting/retrieving Caliptra's IDEVID CSR. Jun 24, 2024
@rusty1968 rusty1968 marked this pull request as ready for review June 24, 2024 16:54
@rusty1968 rusty1968 requested a review from nquarton June 24, 2024 17:25
uint8_t idev_csr_bytes[IDEV_CSR_LEN] = {
48, 130, 1, 183, 48, 130, 1, 62, 2, 1, 0, 48, 105, 49, 28, 48, 26, 6,
3, 85, 4, 3, 12, 19, 67, 97, 108, 105, 112, 116, 114, 97, 32, 49, 46,
48, 32, 73, 68, 101, 118, 73, 68, 49, 73, 48, 71, 6, 3, 85, 4, 5, 19,
Copy link
Contributor

Choose a reason for hiding this comment

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

where does this CSR array come from ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

from the idev_csr_der file in smoke test data.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be worth either

  • Reading it from a file
  • Adding a comment with the filepath to where this comes from

{
int status;

// Initialize FSM GO
caliptra_bootfsm_go();

Copy link
Contributor

Choose a reason for hiding this comment

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

NP: why adding this NL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

{
if (caliptra_is_csr_ready()) {
break;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a timeout here ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think libcaliptra can have any platform-agnostic way of supporting timing. If the caller wants a timeout, I think they would need to directly use caliptra_is_csr_ready() in their own generic test-with-timeout function/macro.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this function from the public api and left it as test fixture.

libcaliptra/src/caliptra_api.c Show resolved Hide resolved
@@ -128,7 +128,7 @@ static inline bool caliptra_mbox_is_busy(void)

static inline uint8_t caliptra_mbox_read_status_fsm(void)
{
return (uint8_t)(caliptra_mbox_read(MBOX_CSR_MBOX_STATUS) >> 16 & MBOX_CSR_MBOX_STATUS_STATUS_MASK);
return (uint8_t)(caliptra_mbox_read(MBOX_CSR_MBOX_STATUS) >> 6) & 7;
Copy link
Contributor

Choose a reason for hiding this comment

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

what are 6 and 7 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i changed it to use the #defines available in the headers.

@@ -263,7 +275,7 @@ int rt_test_all_commands()
{
int failure = 0;
uint32_t non_fatal_error;
int status = boot_to_ready_for_fw();
int status = boot_to_ready_for_fw(false);

if (status){
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we overriding the actual cause of failure with 1 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have the context. I was not the original implementer of the testbench.

libcaliptra/examples/generic/main.c Show resolved Hide resolved
libcaliptra/examples/generic/main.c Show resolved Hide resolved
libcaliptra/inc/caliptra_api.h Show resolved Hide resolved
libcaliptra/examples/generic/main.c Show resolved Hide resolved
{
if (caliptra_is_csr_ready()) {
break;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think libcaliptra can have any platform-agnostic way of supporting timing. If the caller wants a timeout, I think they would need to directly use caliptra_is_csr_ready() in their own generic test-with-timeout function/macro.

libcaliptra/src/caliptra_api.c Outdated Show resolved Hide resolved
@rusty1968 rusty1968 force-pushed the libcaliptra-csr branch 2 times, most recently from 64b9c9f to 6ddfb6c Compare June 25, 2024 02:46
Copy link
Contributor

@nquarton nquarton left a comment

Choose a reason for hiding this comment

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

Just 2 small things then I think it's good to go from my perspective

libcaliptra/src/caliptra_fuses.h Outdated Show resolved Hide resolved
libcaliptra/inc/caliptra_types.h Outdated Show resolved Hide resolved
@rusty1968 rusty1968 force-pushed the libcaliptra-csr branch 2 times, most recently from 80095c3 to 1c74395 Compare June 25, 2024 20:47
nquarton
nquarton previously approved these changes Jun 25, 2024
Comment on lines +142 to +146




Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: remove extra newlines

uint8_t idev_csr_bytes[IDEV_CSR_LEN] = {
48, 130, 1, 183, 48, 130, 1, 62, 2, 1, 0, 48, 105, 49, 28, 48, 26, 6,
3, 85, 4, 3, 12, 19, 67, 97, 108, 105, 112, 116, 114, 97, 32, 49, 46,
48, 32, 73, 68, 101, 118, 73, 68, 49, 73, 48, 71, 6, 3, 85, 4, 5, 19,
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be worth either

  • Reading it from a file
  • Adding a comment with the filepath to where this comes from

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants