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

xtensa/esp32: write encrypt func implementation #15896

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

Conversation

sdc-g
Copy link
Contributor

@sdc-g sdc-g commented Feb 24, 2025

Summary

based on spec, 16 bytes alignment is checked.

Impact

both block write and byte write are supported now, but 16 bytes alignment is required.

Testing

tested by internal test case to write encrypted data

Just list example code here:

{
    int ret = 0;
    uint8_t read_buf[0x10] = {0};
    uint8_t write_buf[0x10] = {0x31, 0x07, 0x01, 0x02, 0x04, 0x00, 0x20, 0x00,
                               0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xca, 0x9f};
    struct mtd_dev_s *mtd = esp32_spiflash_get_mtd();
    struct mtd_dev_s *mtd_encrypt = esp32_spiflash_encrypt_get_mtd();

    ret = MTD_ERASE(mtd, 0x6ec000 / SEC_FLASH_SECTOR_SIZE,
                    0x1000 / SEC_FLASH_SECTOR_SIZE);
    printf("MTD_ERASE(mtd, 0x6ec000/%d, 0x1000/%d) ret 0x%x\n\n",
           SEC_FLASH_SECTOR_SIZE, SEC_FLASH_SECTOR_SIZE, ret);

    ret = MTD_READ(mtd, 0x6ec000, 0x10, read_buf);
    printf("MTD_READ(mtd, 0x6ec000, 0x10, read_buf) ret 0x%x\n", ret);
    printf("read_buf[] : ");
    for (int i = 0; i < 0x10; i++) {
      printf("%02x ", read_buf[i]);
    }
    printf("\n\n");

    ret = MTD_READ(mtd_encrypt, 0x6ec0f0, 0x10, read_buf);
    printf("MTD_READ(mtd_encrypt, 0x6ec0f0, 0x10, read_buf) ret 0x%x\n", ret);
    printf("read_buf[] : ");
    for (int i = 0; i < 0x10; i++) {
      printf("%02x ", read_buf[i]);
    }
    printf("\n\n");

    ret = MTD_WRITE(mtd_encrypt, 0x6ec0f0, 0x10, write_buf);
    printf("MTD_WRITE(mtd_encrypt, 0x6ec0f0, 0x10, write_buf) ret 0x%x\n", ret);
    printf("write_buf[] : ");
    for (int i = 0; i < 0x10; i++) {
      printf("%02x ", write_buf[i]);
    }
    printf("\n\n");

    ret = MTD_READ(mtd, 0x6ec000, 0x10, read_buf);
    printf("MTD_READ(mtd, 0x6ec000, 0x10, read_buf) ret 0x%x\n", ret);
    printf("read_buf[] : ");
    for (int i = 0; i < 0x10; i++) {
      printf("%02x ", read_buf[i]);
    }
    printf("\n\n");

    ret = MTD_READ(mtd_encrypt, 0x6ec000, 0x10, read_buf);
    printf("MTD_READ(mtd_encrypt, 0x6ec000, 0x10, read_buf) ret 0x%x\n", ret);
    printf("read_buf[] : ");
    for (int i = 0; i < 0x10; i++) {
      printf("%02x ", read_buf[i]);
    }
    printf("\n\n");

    ret = MTD_READ(mtd_encrypt, 0x6ec0f0, 0x10, read_buf);
    printf("MTD_READ(mtd_encrypt, 0x6ec0f0, 0x10, read_buf) ret 0x%x\n", ret);
    printf("read_buf[] : ");
    for (int i = 0; i < 0x10; i++) {
      printf("%02x ", read_buf[i]);
    }
    printf("\n\n");
  }

@github-actions github-actions bot added Arch: xtensa Issues related to the Xtensa architecture Size: S The size of the change in this PR is small labels Feb 24, 2025
Copy link
Contributor

@jerpelea jerpelea left a comment

Choose a reason for hiding this comment

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

please sign the commit with
git commit -s

@nuttxpr
Copy link

nuttxpr commented Feb 24, 2025

[Experimental Bot, please feedback here]

No, this PR description does not fully meet the NuttX requirements. It is far too brief and lacks critical information. Here's a breakdown of the missing pieces:

Summary:

  • Missing: The summary is extremely vague. What specific change is being made? What problem does it solve (fix)? Or what new capability does it provide (new feature)? The phrase "based on spec, 16 bytes alignment is checked" doesn't tell us what "spec" or what is being checked for that alignment. What was the behavior before this change?
  • Missing: What functional part of the code is changed? Mention the specific files/modules/functions affected. e.g., "Changes the block driver interface," or "Modifies the encryption handling in the flash driver."
  • Missing: How does the change work? Saying "both block write and byte write are supported" is a result, not an explanation. Describe the mechanism used to achieve this. Did you add a new function? Modify an existing one? Change a data structure?
  • Missing: Links to related NuttX issues or NuttX Apps issues/PRs.

Impact:

While the PR touches on some impact areas, the answers are too short to be helpful. "YES" should always be followed by a detailed explanation.

  • Missing Detail: "both block write and byte write are supported now, but 16 bytes alignment is required" should be broken down. Is this a new requirement? Will any existing code break because of this requirement? Which architectures/boards are affected?
  • All other impact areas are effectively unanswered. Even if the answer is NO, briefly explain why. For example, "Impact on build: NO (No changes to Makefiles or build configuration)"

Testing:

  • Insufficient: "tested by internal test case" is not enough. What internal test case? Give its name or location.
  • Missing Logs: Placeholder logs are present, but actual logs demonstrating the before/after behavior are missing. The logs should show that the intended change was made and that it works correctly. Include relevant information like inputs, outputs, and error messages.
  • Missing Details on Test Environment: Be much more specific about the build host and target. Instead of just "Linux," specify the distribution and version (e.g., "Ubuntu 22.04"). Provide the compiler version (e.g., "GCC 11.2.0"). For the target, specify the architecture, board, and configuration (e.g., "sim:qemu-x86_64:nsh").

In short, the PR description needs significant expansion to meet the NuttX requirements. It needs to be more detailed, specific, and provide evidence of testing and consideration of the impact of the changes.

@fdcavalcanti
Copy link
Contributor

Hello @sdc-g, can you post here test output?
Also improve the PR description as suggested by PR bot

based on spec, 16 bytes alignment is checked.
@sdc-g sdc-g force-pushed the bugfix/esp32-write_encrypt branch from 8b395f9 to b3c3fa2 Compare February 25, 2025 05:31
@sdc-g
Copy link
Contributor Author

sdc-g commented Feb 25, 2025

Hello @sdc-g, can you post here test output? Also improve the PR description as suggested by PR bot

Hi @fdcavalcanti
Please check the test code I shared.

Regarding of PR bot error message, I also checked and fixed.
Sorry for it, since I ported the code from one early version.

Please review again, thanks!

@fdcavalcanti
Copy link
Contributor

@tmedicci can you take a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: xtensa Issues related to the Xtensa architecture Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants