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

plat-nuvoton: configuration change #6181

Merged
merged 3 commits into from
Aug 21, 2023
Merged

plat-nuvoton: configuration change #6181

merged 3 commits into from
Aug 21, 2023

Conversation

rutigl
Copy link
Contributor

@rutigl rutigl commented Jul 18, 2023 •
edited
Loading

Changes load address of OPTEE-OS from 0x36000000 to 0x02100000
Implements HUK reading from DME PCR0 located in PCI mailbox
Disables DT insecure warning

@jenswi-linaro
Copy link
Contributor

Please rebase this on the upstream master branch to resolve the conflicts.

@rutigl
Copy link
Contributor Author

rutigl commented Jul 23, 2023 •
edited
Loading

Please rebase this on the upstream master branch to resolve the conflicts.

Done

@rutigl rutigl force-pushed the npcm_3_21 branch 8 times, most recently from fa24cfc to fc9c8e5 Compare July 23, 2023 12:48
@jforissier
Copy link
Contributor

@rutigl you really need to learn how to use Git, GitHub and write proper commit subjects and descriptions. Otherwise your pull requests will take a long time to get reviewed and merged.

@rutigl rutigl force-pushed the npcm_3_21 branch 2 times, most recently from 5f9c304 to 2a45dab Compare July 23, 2023 13:10
@rutigl
Copy link
Contributor Author

rutigl commented Jul 23, 2023

@rutigl you really need to learn how to use Git, GitHub and write proper commit subjects and descriptions. Otherwise your pull requests will take a long time to get reviewed and merged.

Agree, I have not a big experience in GitHub PRs. Please tell me what to improve in current commits and I'll fix it

@rutigl rutigl force-pushed the npcm_3_21 branch 5 times, most recently from dbe2106 to 8c26f4f Compare July 24, 2023 06:46
@jforissier
Copy link
Contributor

plat-nuvoton: configuration change is doing several things:

  • It changes the load address of OP-TEE and the RAM size allocated to it. Why? Not explained in the commit description.
  • It moves the shared memory to a different address. Why? Not explained.
  • It moves the SDP memory and declares a size for it. Where was the size defined previously? was SDP used or not? does this enable SDP, or does it change its characteristics? Not explained.
  • It adds #include <mm/core_memprot.h>. Is this neeed? If so it should be in a separate commit.
  • It changes the banner text. That should be in a separate commit.

plat-nuvoton: added HUK reading -> plat-nuvoton: add HUK reading (use imperative mood in commit subject)

I had other comments but you force pushed new commits while I was reviewing so I lost them. As mentioned in the documentation, the preferred way to address comments is to push fixup commits that are squashed in the end. And before squashing/force pushing, write a comment asking if it is OK to do so.

@rutigl
Copy link
Contributor Author

rutigl commented Jul 24, 2023

  • Load address of OPTEE-OS was changed because we moved TF-A from SRAM to DRAM, and our new memory map is described in the TF-A code. Do you want me to put the whole new memory map in commit message or in the platform_config.h? Or enough just say the reason of moving in commit message?
  • Shared memory also moved because the same reason.
  • SDP size wasn't define before, it's a part of new memory map
  • memprot.h include is necessary for reading HUK, because it uses phys_to_virt function to access PCI Mailbox memory
  • banner text change is not necessary and I removed it
  • as for style, I'll immediately change it
    Thank you.

jforissier
jforissier reviewed Aug 4, 2023
View reviewed changes
Copy link
Contributor

@jforissier jforissier left a comment

Choose a reason for hiding this comment

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

Comments on "plat-nuvoton: add HUK reading"

core/arch/arm/plat-nuvoton/main.c Outdated

TEE_Result tee_otp_get_hw_unique_key(struct tee_hw_unique_key *hwkey)
{
void *vaddr;
Copy link
Contributor

Choose a reason for hiding this comment

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

= NULL;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

core/arch/arm/plat-nuvoton/main.c Outdated
{
void *vaddr;
TEE_Result res = TEE_SUCCESS;
uint8_t buf[NPCM_MEASURE_SIZE] = {0};
Copy link
Contributor

Choose a reason for hiding this comment

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

= { };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

core/arch/arm/plat-nuvoton/main.c Outdated
return TEE_ERROR_SECURITY;
}

memcpy(buf, vaddr, NPCM_MEASURE_SIZE);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this memcpy() needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, could be copied directly from vaddr

core/arch/arm/plat-nuvoton/main.c Outdated
void *vaddr;
TEE_Result res = TEE_SUCCESS;
uint8_t buf[NPCM_MEASURE_SIZE] = {0};
uint32_t bin[1 + HW_UNIQUE_KEY_LENGTH / sizeof(uint32_t)];
Copy link
Contributor

Choose a reason for hiding this comment

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

= { };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

core/arch/arm/plat-nuvoton/main.c Outdated
return TEE_ERROR_SECURITY;
}

memcpy(&hwkey->data[0], bin, HW_UNIQUE_KEY_LENGTH);
Copy link
Contributor

Choose a reason for hiding this comment

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

bin[0] is garbage at this point, isn't it? (well it will be 0 if you initialize bin[] properly as requested above, but is this intended?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree, fixed

core/arch/arm/plat-nuvoton/main.c

memcpy(&hwkey->data[0], bin, HW_UNIQUE_KEY_LENGTH);

IMSG("HUK Initialized");
Copy link
Contributor

Choose a reason for hiding this comment

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

"Initialized" sounds like this is something done only once but it isn't the case because tee_otp_get_hw_unique_key() may be called multiple times in case huk_subkey_derive() is called several times. So either this message has to be removed, or the HUK should be made static and computed only once (which might be the better option since it should not change anyways).

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 didn't know, that this function is called several times, so I have changed the logic accordingly.

@jforissier
Copy link
Contributor

The subject of the second "plat-nuvoton: configuration change" commit should be "platf-nuvoton: force CFG_EXTERNAL_DT=n". With that it is:
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

@jforissier
Copy link
Contributor

The subject of the first "plat-nuvoton: configuration change" commit should be more precise:
"plat-nuvoton: change load address, shared memory and SDP memory".
With that:
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

@rutigl
plat-nuvoton: change load address, shared memory and SDP memory
ec44e8b 
Changes load address of OPTEE-OS from 0x36000000 to 0x02100000
Moves shared memory to 0x06000000
Moves SDP memory to 0x05F00000

Co-developed-by: Hila Miranda-Kuzi <hila.miranda.kuzi1@gmail.com>
Signed-off-by: Hila Miranda-Kuzi <hila.miranda.kuzi1@gmail.com>
Signed-off-by: Margarita Glushkin <rutigl@gmail.com>
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
@rutigl
plat-nuvoton: add HUK reading
0c2792a 
Implements HUK reading from DME PCR0 located in the PCI mailbox

Co-developed-by: Hila Miranda-Kuzi <hila.miranda.kuzi1@gmail.com>
Signed-off-by: Hila Miranda-Kuzi <hila.miranda.kuzi1@gmail.com>
Signed-off-by: Margarita Glushkin <rutigl@gmail.com>
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
@rutigl
plat-nuvoton: force CFG_EXTERNAL_DT=n
004c60a 
Disables DT insecure warning

Co-developed-by: Hila Miranda-Kuzi <hila.miranda.kuzi1@gmail.com>
Signed-off-by: Hila Miranda-Kuzi <hila.miranda.kuzi1@gmail.com>
Signed-off-by: Margarita Glushkin <rutigl@gmail.com>
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
@jenswi-linaro
Copy link
Contributor

It looks like this is ready to be merged.

@jforissier
Copy link
Contributor

It looks like this is ready to be merged.

Indeed!

@jforissier jforissier merged commit 846a948 into OP-TEE:master Aug 21, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jforissier jforissier jforissier left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rutigl @jenswi-linaro @jforissier