Skip to content

plat-versal: add support for the Versal Net variant #6738

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

jcorbier
Copy link

@jcorbier jcorbier commented Mar 5, 2024

This series upgrades the AMD/Xilinx port with the following:

  • Add support for a new SoC in the Versal family
  • Add a hardware crypto accelerator driver for this specific SoC
  • Fixes for more recent versions of the Xilinx software environment (PLM APIs have changed in 2023)

@ldts
Copy link
Contributor

ldts commented Mar 6, 2024

thanks @jcorbier

I need to ask that the changes to support the more recent AMD/Xilinx tools maintain backwards compatibility. We should be able to query the ABI at runtime - maybe even propose whatever is needed to AMD/Xilinx https://github.com/Xilinx/embeddedsw .

I'd like to understand as well the level of testing that has been done with this software (just the output of xtest, to check if you encountered any regressions (ie this is the changelog for 4.1.0 #6574 (comment) ).

Thirdly is there anything that you also plan on posting to https://github.com/OP-TEE/optee_docs ?

@jcorbier
Copy link
Author

jcorbier commented Mar 6, 2024

Thanks @ldts for your feedback.

I need to ask that the changes to support the more recent AMD/Xilinx tools maintain backwards compatibility. We should be able to query the ABI at runtime - maybe even propose whatever is needed to AMD/Xilinx https://github.com/Xilinx/embeddedsw .

Noted. Let me see how best we can implement that.

I'd like to understand as well the level of testing that has been done with this software (just the output of xtest, to check if you encountered any regressions (ie this is the changelog for 4.1.0 #6574 (comment) ).

I don't have access to the logs right now but the current state is the same as for Versal in 4.1.0.

Thirdly is there anything that you also plan on posting to https://github.com/OP-TEE/optee_docs ?

Yes, a working version is available here https://github.com/ProvenRun/optee_docs/tree/versal_net_port
It needs some additional work before I create a pull request for it though.

Same thing for build and manifest repositories.

@ldts
Copy link
Contributor

ldts commented Mar 8, 2024

we should split the drivers (rng/nvm) into a different files (versal_net_rng, versal_net_nvm?)

@jcorbier
Copy link
Author

jcorbier commented Mar 8, 2024

we should split the drivers (rng/nvm) into a different files (versal_net_rng, versal_net_nvm?)

Agreed, the initial thinking for the current implementation was to avoid as much code duplication as possible between versal and versal_net but in the end it makes things much more complicated than needed.

@nathan-menhorn
Copy link

Hi @jcorbier any updates on this PR?

@jcorbier
Copy link
Author

Hi @jcorbier any updates on this PR?

Hi @nathan-menhorn, still working out the details of what needs to be done to properly split versal/versal-net code, including the TRNG update. I'll try and push an update to this PR by end of this week.

@ldts
Copy link
Contributor

ldts commented Mar 31, 2024

@etienne-lms could you hold your comments until the patchset is updated please?

There are a couple of functional changes that need addressing first

  1. ABI runtime detection (not only to support the older toolchain but also to be covered for changes in the future)
  2. use different files for Versal net instead of conditional macros (since some drivers are radically different).

So I suggest we wait for that before we go into details (ie default configs, coding standards and so on) as some files will change quite a bit

@jcorbier
Copy link
Author

jcorbier commented Apr 2, 2024

@etienne-lms could you hold your comments until the patchset is updated please?

Indeed, I'll be pusing fixup commits in the coming hours/days.

#define VERSAL_PM_MAJOR 0
#define VERSAL_PM_MINOR 1
#define VERSAL_PM_MAJOR 1
#define VERSAL_PM_MINOR 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Deserves a specific commit IMHO.

@nathan-menhorn
Copy link

Hi @jcorbier what's the current status of this PR? Thanks.

@nathan-menhorn
Copy link

Hi @jcorbier any updates on this PR? Are patches to address all the comments in the PR still estimated to come by the end of the month? Thanks.

return do_write_efuses_value(EFUSE_WRITE_MISC1_CTRL_BITS, val);
}

TEE_Result versal_efuse_write_offchip_ids(uint32_t id)
Copy link
Contributor

Choose a reason for hiding this comment

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

this function is incorrect, there are total 8 offchip_revoke_id, and we use the api to update values for certain id, the parameters is lacking of the values going into that offchip id.
Please refer to the implementation in versal_nvm.c

Copy link
Author

Choose a reason for hiding this comment

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

Hi @wangjudyw,

Thanks for your feedback. This implementation is a direct mapping of the API offered by the xilnvm service:

https://github.com/Xilinx/embeddedsw/blob/86a272bd9cd412bf1e5214f6098aefff301e58f0/lib/sw_services/xilnvm/src/versal_net/server/xnvm_efuse_cdohandler.c#L229

As you can see, it only expects an uint32_t for the ID to be written in the fuses (and a flag that is set by default by the do_write_efuses_value() helper function). Could you elaborate what you mean?

Thanks!

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

mainly coding style issues

Copy link

This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time.

@github-actions github-actions bot added the Stale label Jul 29, 2024
@nathan-menhorn
Copy link

Hi @jcorbier what's the status of this PR? Last we discussed updates were supposed to be pushed a few weeks ago? Thanks.

@github-actions github-actions bot removed the Stale label Jul 30, 2024
@ldts
Copy link
Contributor

ldts commented Aug 22, 2024

@jcorbier do you plan on folding the commits as per the initial patch-set for further review? I can then have a a better look - last time I checked I found a simple regression (easy to fix).

Also I was testing the Xen hypervisor with the tip of OP-TEE on the vck190 evaluation kit and I found it to be broken. I was wondering if this is a configuration (optee+xen on Versal) that you have tested? I believe probably nobody has yet (@nathan-menhorn ?)

@nathan-menhorn
Copy link

Hi @ldts no testing has been performed on Xen+optee yet as there haven't been any customers requests.

@nathan-menhorn
Copy link

@jcorbier @ldts @etienne-lms just keeping this PR alive. We should be expecting some input from @jcorbier soon.

@jcorbier
Copy link
Author

@jcorbier do you plan on folding the commits as per the initial patch-set for further review? I can then have a a better look - last time I checked I found a simple regression (easy to fix).

Yes, there a couple more things I want to fix then I'll force push a clean patchset to clean up the current fixup commits mess.

@@ -994,8 +1010,16 @@ TEE_Result versal_efuse_write_revoke_ppk(enum versal_nvm_ppk_type type)
return versal_efuse_write_misc(&misc_ctrl);
}

/*
* versal_efuse_write_revoke_id expects an efuse identifier between
* 1 and 256.

Choose a reason for hiding this comment

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

@jcorbier 0 to 255

TEE_Result versal_efuse_write_revoke_id(uint32_t id)
{
if ((id < VERSAL_NET_REVOKE_EFUSE_MIN) ||

Choose a reason for hiding this comment

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

@jcorbier check should be between 0 and 255.

I'm not sure why the AMD software was implemented this way as this is very confusing and it doesn't match the OFFCHIP_REVOKE function, which expects values from 1 - 256, but this function expects values from 0 to 255

See the error handling of
https://github.com/Xilinx/embeddedsw/blob/master/lib/sw_services/xilnvm/src/versal_net/server/xnvm_efuse.c#L615C21-L617
compared to
https://github.com/Xilinx/embeddedsw/blob/master/lib/sw_services/xilnvm/src/versal_net/server/xnvm_efuse.c#L701-L703

@@ -1012,12 +1014,12 @@ TEE_Result versal_efuse_write_revoke_ppk(enum versal_nvm_ppk_type type)

/*
* versal_efuse_write_revoke_id expects an efuse identifier between
* 1 and 256.
* 1 and 256.

Choose a reason for hiding this comment

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

0 - 255

Choose a reason for hiding this comment

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

@jcorbier please fix.

Comment on lines +1021 to +1024
if (id < VERSAL_NET_REVOKE_EFUSE_MIN ||
id > VERSAL_NET_REVOKE_EFUSE_MAX)

Choose a reason for hiding this comment

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

@jcorbier checks needs to be between 0 and 255 for this function.

Choose a reason for hiding this comment

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

@jcorbier please fix.

@ldts
Copy link
Contributor

ldts commented Sep 23, 2024

@jcorbier @nathan-menhorn I am not seeing the separate commit that updates versal to the new PLM - I dont think this should be introduced just as part of the versal_net platform.
having said that, if we want to drop the support for the previous toolchain (@nathan-menhorn?) then I suppose we could.

Re: is this all that is needed or something else coming (this breaks versal last time I tested it)
9267cb6

@nathan-menhorn
Copy link

@jcorbier @nathan-menhorn I am not seeing the separate commit that updates versal to the new PLM - I dont think this should be introduced just as part of the versal_net platform. having said that, if we want to drop the support for the previous toolchain (@nathan-menhorn?) then I suppose we could.

Re: is this all that is needed or something else coming (this breaks versal last time I tested it) 9267cb6

Hi @ldts we still need Versal support as customers are actively using the Versal version. If this (your link above) breaks your original port supported for the 2022.1 and 2022.2 Versal BSPs then this isn't good.

@ldts
Copy link
Contributor

ldts commented Sep 27, 2024

Hi @ldts we still need Versal support as customers are actively using the Versal version. If this (your link above) breaks your original port supported for the 2022.1 and 2022.2 Versal BSPs then this isn't good.

ok. I'll wait for the commits being fold, then validate and review the partitioning/integration - @etienne-lms has already done the heavy lifting.

do you know if anyone is looking into the xen support? as I said it broken but I dont think it should be much work to get it right

@nathan-menhorn
Copy link

Thanks @ldts. No one is looking into Xen + OP-TEE support. We don't have any customer requests and we don't have the resources to investigate this at this time.

@ldts
Copy link
Contributor

ldts commented Sep 30, 2024

Thanks @ldts. No one is looking into Xen + OP-TEE support. We don't have any customer requests and we don't have the resources to investigate this at this time.

um, that is a pity. Over the summer I did some prototyping - integrated OP-TEE on meta-xilinx booted xen and started debugging op-tee but then had to drop it.

Maybe I'll continue with it since I still have your board - need to check with my employer first if they allow me work on this on my spare time. will let you know

@nathan-menhorn
Copy link

Thanks @ldts. Feel free to give me an update offline through email.

@jenswi-linaro
Copy link
Contributor

I'm sorry, I missed there was an update in this pull request. Sometimes when only pushing changes to a pull request, github doesn't send updates. So the best is to add a comment after pushing changes to a PR to avoid unnecessary delays.

@ldts had a few questions:

That said, we are where we are, so moving forward: from your comment, it sounds like the current upstream Versal port is now obsolete (no users). If that’s the case, it may be time to consider dropping it entirely. @nathan-menhorn, do you know who will be maintaining Versal Net? Perhaps that team could also take over maintainership of Versal and its drivers and handle the necessary cleanup?

Do we need to choose between Versal and Versal Net to move forward here, or can both variants co-exist without breakage? Who decides to remove Versal if that's needed?

@ldts
Copy link
Contributor

ldts commented Mar 24, 2025

@jenswi-linaro I believe this decision is ultimately up to @nathan-menhorn.

I'm willing to step down as a Versal platform and crypto/driver maintainer to support the proposed strategy moving forward. I don't want to be a blocker for either Versal2 or Versal-Net. My only request is that the license remains in place, as I understand AMD intends to reuse all of the code.

@jenswi-linaro
Copy link
Contributor

Thank you @ldts, I fully support your request for the license.

@nathan-menhorn
Copy link

@jenswi-linaro I believe this decision is ultimately up to @nathan-menhorn.

I'm willing to step down as a Versal platform and crypto/driver maintainer to support the proposed strategy moving forward. I don't want to be a blocker for either Versal2 or Versal-Net. My only request is that the license remains in place, as I understand AMD intends to reuse all of the code.

@ldts @jenswi-linaro I agree that the license needs to remain in place as the code is heavily reused.

@jcorbier could you please provide an update with the license request? Thanks.

@nathan-menhorn
Copy link

I'm sorry, I missed there was an update in this pull request. Sometimes when only pushing changes to a pull request, github doesn't send updates. So the best is to add a comment after pushing changes to a PR to avoid unnecessary delays.

@ldts had a few questions:

That said, we are where we are, so moving forward: from your comment, it sounds like the current upstream Versal port is now obsolete (no users). If that’s the case, it may be time to consider dropping it entirely. @nathan-menhorn, do you know who will be maintaining Versal Net? Perhaps that team could also take over maintainership of Versal and its drivers and handle the necessary cleanup?

Do we need to choose between Versal and Versal Net to move forward here, or can both variants co-exist without breakage? Who decides to remove Versal if that's needed?

Hi @jenswi-linaro both variants can exist without breakage. I've done basic testing on my end and both variants compile as expected for the various boards. Plans for Versal NET maintenance are currently TBD, the main goal is to support the end customer by first getting this port upstreamed.

@jenswi-linaro
Copy link
Contributor

Hi @jenswi-linaro both variants can exist without breakage. I've done basic testing on my end and both variants compile as expected for the various boards. Plans for Versal NET maintenance are currently TBD, the main goal is to support the end customer by first getting this port upstreamed.

Thanks for testing. @nathan-menhorn, if @ldts is stepping down, can you be an interim maintainer for versal while future maintenance is sorted out?

@nathan-menhorn
Copy link

@jenswi-linaro I believe this decision is ultimately up to @nathan-menhorn.
I'm willing to step down as a Versal platform and crypto/driver maintainer to support the proposed strategy moving forward. I don't want to be a blocker for either Versal2 or Versal-Net. My only request is that the license remains in place, as I understand AMD intends to reuse all of the code.

@ldts @jenswi-linaro I agree that the license needs to remain in place as the code is heavily reused.

@jcorbier could you please provide an update with the license request? Thanks.

Just a minor correction on the license. I am checking with AMD legal on this. Thanks.

Copy link

@nathan-menhorn nathan-menhorn left a comment

Choose a reason for hiding this comment

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

Hi @jcorbier the TRNG KAT test is not passing.

.cfg.base = TRNG_BASE,
.cfg.len = TRNG_SIZE,
};
static TEE_Result trng_kat_test_v2(struct versal_trng *trng)

Choose a reason for hiding this comment

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

@jcorbier This code is not passing as expected on hardware causing OP-TEE to fail booting. Please recheck this on your board as the generated output does not match the expected output. Thanks.

Choose a reason for hiding this comment

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

I added some debug prints to see the KAT output and here are the results:

D/TC:00 0 do_init_calls:19 service_initcall level 2 check_ta_store()
D/TC:00 0 check_ta_store:449 TA store: "early TA"
D/TC:00 0 check_ta_store:449 TA store: "Secure Storage TA"
D/TC:00 0 check_ta_store:449 TA store: "REE"
D/TC:00 0 do_init_calls:19 service_initcall level 2 early_ta_init()
D/TC:00 0 early_ta_init:56 Early TA f04a0fe7-1f5d-4b9b-abf7-619b85b4ce8c size 48116 (compressed, uncompressed 109288)
D/TC:00 0 do_init_calls:19 service_initcall level 2 verify_pseudo_tas_conformance()
D/TC:00 0 do_init_calls:19 service_initcall level 3 tee_fs_init_key_manager()
I/TC: Using Development HUK
D/TC:00 0 boot_mem_release_tmp_alloc:324 Releasing 8192 bytes from va 0x223fe000
D/TC:00 0 do_init_calls:19 driver_initcall level 1 ecc_init()
E/TC:00 0 trng_kat_test_v2:1152 PRINTING TRNG KAT VECTOR
E/TC:00 0 trng_kat_test_v2:1155 D8
E/TC:00 0 trng_kat_test_v2:1155 F1
E/TC:00 0 trng_kat_test_v2:1155 6E
E/TC:00 0 trng_kat_test_v2:1155 04
E/TC:00 0 trng_kat_test_v2:1155 46
E/TC:00 0 trng_kat_test_v2:1155 8E
E/TC:00 0 trng_kat_test_v2:1155 6B
E/TC:00 0 trng_kat_test_v2:1155 73
E/TC:00 0 trng_kat_test_v2:1155 E6
E/TC:00 0 trng_kat_test_v2:1155 92
E/TC:00 0 trng_kat_test_v2:1155 BD
E/TC:00 0 trng_kat_test_v2:1155 3D
E/TC:00 0 trng_kat_test_v2:1155 A6
E/TC:00 0 trng_kat_test_v2:1155 28
E/TC:00 0 trng_kat_test_v2:1155 9C
E/TC:00 0 trng_kat_test_v2:1155 37
E/TC:00 0 trng_kat_test_v2:1155 5B
E/TC:00 0 trng_kat_test_v2:1155 9E
E/TC:00 0 trng_kat_test_v2:1155 6C
E/TC:00 0 trng_kat_test_v2:1155 3C
E/TC:00 0 trng_kat_test_v2:1155 89
E/TC:00 0 trng_kat_test_v2:1155 8D
E/TC:00 0 trng_kat_test_v2:1155 81
E/TC:00 0 trng_kat_test_v2:1155 9E
E/TC:00 0 trng_kat_test_v2:1155 D7
E/TC:00 0 trng_kat_test_v2:1155 E3
E/TC:00 0 trng_kat_test_v2:1155 89
E/TC:00 0 trng_kat_test_v2:1155 61
E/TC:00 0 trng_kat_test_v2:1155 6A
E/TC:00 0 trng_kat_test_v2:1155 58
E/TC:00 0 trng_kat_test_v2:1155 65
E/TC:00 0 trng_kat_test_v2:1155 09

E/TC:00 0 trng_kat_test_v2:1157
E/TC:00 0 trng_kat_test_v2:1158 K.A.T mismatch
E/TC:00 0 versal_trng_hw_init:1216 KAT Failed
E/TC:00 0 Panic at core/drivers/versal_trng.c:1217 <versal_trng_hw_init>
E/TC:00 0 TEE load address @ 0x22200000
E/TC:00 0 Call stack:
E/TC:00 0 0x222081d4
E/TC:00 0 0x22225cbc
E/TC:00 0 0x2221c89c
E/TC:00 0 0x222227cc
E/TC:00 0 0x22223180
E/TC:00 0 0x222219d4
E/TC:00 0 0x222277bc
E/TC:00 0 0x22207ee8

Copy link
Author

Choose a reason for hiding this comment

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

Hi @nathan-menhorn, thanks for the logs. It might be a merge issue caused by us not testing on the final hardware, sorry about that. Let me check.

@nathan-menhorn
Copy link

Hi @ldts could you please clearly specific which files are missing the license you mention and the specific license text you are looking to include. Thanks.

@nathan-menhorn
Copy link

@jcorbier any updates on the TRNG?

@ldts
Copy link
Contributor

ldts commented Apr 14, 2025

Hi @ldts could you please clearly specific which files are missing the license you mention and the specific license text you are looking to include. Thanks.

hi @nathan-menhorn I checked and I think it is all good now. Maybe I was confused with the versal2 PRs.

@nathan-menhorn
Copy link

Thanks @ldts for confirming.

@nathan-menhorn
Copy link

@jcorbier any updates on the TRNG? This is slipping schedules for our CAVP testing and slipping schedules for the end customer. Thanks.

@jcorbier
Copy link
Author

@jcorbier any updates on the TRNG? This is slipping schedules for our CAVP testing and slipping schedules for the end customer. Thanks.

Hi @nathan-menhorn, working on it. Debugging is taking a bit more time than expected. We hope we'll have pushed an update here by tomorrow morning your time.

@ldts
Copy link
Contributor

ldts commented Apr 16, 2025

@jcorbier can you check if this is required for Versal?
74c6ad9

I suspect it will be.

@jcorbier
Copy link
Author

@jcorbier can you check if this is required for Versal? 74c6ad9

I suspect it will be.

Indeed. Thanks for pointing this out.

@nathan-menhorn
Copy link

Hi @jcorbier any updates?

@nathan-menhorn
Copy link

Hi @jcorbier any updates on the TRNG KAT?

@nathan-menhorn
Copy link

@jcorbier any updates on the TRNG? This is slipping schedules for our CAVP testing and slipping schedules for the end customer. Thanks.

Hi @nathan-menhorn, working on it. Debugging is taking a bit more time than expected. We hope we'll have pushed an update here by tomorrow morning your time.

Hi @jcorbier From your comment last month, we were supposed to get updates in a day or so and it's now May and the customer is even further behind. Any updates on getting this fixed?

@nathan-menhorn
Copy link

@jcorbier

4 similar comments
@nathan-menhorn
Copy link

@jcorbier

@nathan-menhorn
Copy link

@jcorbier

@nathan-menhorn
Copy link

@jcorbier

@nathan-menhorn
Copy link

@jcorbier

Copy link

This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time.

@jcorbier
Copy link
Author

Pull request still being worked on in the background. More pushes to come in an estimated couple weeks

@huynhdanvo
Copy link

Hi @nathan-menhorn , I was able to produce the same issue with trng_kat_test_v2() panic issue. It seems to me that the hw is disabled.

I/TC: TRNG_OSC_EN after manual enable: 0x00000001
I/TC: TRNG_CTRL = 0x00000000
I/TC: Initial CTRL: 0x00000000
I/TC: CTRL after write: 0x00000000
I/TC: Dump before TRNG KAT
I/TC: ---- TRNG Register Dump ----
I/TC: TRNG_CTRL       : 0x00000000
I/TC: TRNG_STATUS     : 0x00000000
I/TC: TRNG_OSC_EN     : 0x00000001
I/TC: TRNG_CORE_OUTPUT: 0x00000000
I/TC: CFG_VERSAL_RNG_DRV_V2: 1
I/TC: TRNG_CTRL2       : 0x00000000
I/TC: TRNG_CTRL3       : 0x00000000
I/TC: TRNG_CTRL4       : 0x00000000
I/TC: -----------------------------

Could you please assist me on how to setup the right clock to enable the hw on my device?

Thanks,
Dan

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.

8 participants