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

Add Widevine related device tree options support #6379

Merged
merged 1 commit into from Feb 9, 2024

Conversation

qazwsxedcrfvtg14
Copy link
Contributor

On the new ChromeOS mediatek platform, we will use the device tree to pass hardware unique key and the parameters for widevine TA.

BTW, should we also put the device tree binding document in the OP-TEE repo?
Because the Linux upstream says the OP-TEE only DT bindings don't belong in the kernel.
https://lore.kernel.org/all/20230918194235.GA1548023-robh@kernel.org/

Copy link
Contributor

@jbech-linaro jbech-linaro left a comment

Choose a reason for hiding this comment

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

As a generic comment I think we'd like to change this to transfer lists, transfer elements in the future. But until that is in place DT works.

core/pta/widevine.c Outdated Show resolved Hide resolved
core/pta/widevine.c Outdated Show resolved Hide resolved
core/pta/widevine.c Outdated Show resolved Hide resolved
core/pta/widevine.c Outdated Show resolved Hide resolved
core/pta/widevine.c Outdated Show resolved Hide resolved
core/arch/arm/plat-mediatek/cros_huk.c Outdated Show resolved Hide resolved
mk/config.mk Outdated Show resolved Hide resolved
core/arch/arm/plat-mediatek/cros_huk.c Outdated Show resolved Hide resolved
core/arch/arm/plat-mediatek/cros_huk.c Outdated Show resolved Hide resolved
core/arch/arm/plat-mediatek/cros_huk.c Outdated Show resolved Hide resolved
core/pta/widevine.c Outdated Show resolved Hide resolved
core/pta/widevine.c Outdated Show resolved Hide resolved
@qazwsxedcrfvtg14
Copy link
Contributor Author

Added a new commit to resolve comments.

@jbech-linaro
Copy link
Contributor

I think this is good now, please add my tag, rebase it and squash the patches accordingly so we merge it. Thanks!

Reviewed-by: Joakim Bech <joakim.bech@linaro.org>

@qazwsxedcrfvtg14
Copy link
Contributor Author

Rebased and squashed the patches.
Thanks!

@jforissier
Copy link
Contributor

@qazwsxedcrfvtg14 please address the checkpatch issues (CI / Code style job). You may use the sentence you put in this PR ("On the new ChromeOS mediatek platform, we will use the device tree to pass hardware unique key and the parameters for widevine TA.").
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.

Are these DT bindings (DT node pathes as /options/ and property names) generic? Are they OP-TEE specific (e.g optee-hardware-unique-key) ? If these are to be deprecated once Firmware Handoff Transfer List is merged, is it worth merging this change now?

The commit message lacks a description, see CI Code Style result that also reports another issue).

core/pta/widevine.c Show resolved Hide resolved
core/pta/widevine.c Outdated Show resolved Hide resolved
core/pta/widevine.c Outdated Show resolved Hide resolved
mk/config.mk Outdated Show resolved Hide resolved
core/drivers/widevine_huk.c Show resolved Hide resolved
core/pta/widevine.c Outdated Show resolved Hide resolved
@qazwsxedcrfvtg14
Copy link
Contributor Author

Resolved the comments.

BTW, I'm not sure about the status of "Firmware Handoff Transfer List"...
@Narflex do we have any plans for that?

@Narflex
Copy link
Contributor

Narflex commented Nov 3, 2023

Resolved the comments.

BTW, I'm not sure about the status of "Firmware Handoff Transfer List"... @Narflex do we have any plans for that?

I'm not sure about our future plans for that, but it's not in the timeline for this change, so please proceed with this change as-is.

@etienne-lms
Copy link
Contributor

so please proceed with this change as-is.

Please check first that such /options/ DT node paths and new DT property are not a open door to messy DT bindings.
DT bindings is a spec. It should remain somewhat consistent. Thanks for #6418, that helps discussion on a real proposal. Is it clear who is in charge of maintaining these bindings evolution and consistency. It seems the pointed discussion thread did not lead to a definitive consensus. Unless I missed something in which case I apologies for being noisy.

@Narflex
Copy link
Contributor

Narflex commented Nov 3, 2023

so please proceed with this change as-is.

Please check first that such /options/ DT node paths and new DT property are not a open door to messy DT bindings. DT bindings is a spec. It should remain somewhat consistent. Thanks for #6418, that helps discussion on a real proposal. Is it clear who is in charge of maintaining these bindings evolution and consistency. It seems the pointed discussion thread did not lead to a definitive consensus. Unless I missed something in which case I apologies for being noisy.

The DT bindings being added here are being documented in OP-TEE as you pointed out, so I would figure OP-TEE maintainers would be the ones maintaining the evolution/consistency of that if anything else gets added there in the future. Originally we tried to get these documented in the linux kernel, but got pushback on that which is why they ended up in OP-TEE....and I think that stance will continue to be taken where DT bindings should get added to kernel documentation unless there is pushback on that end, in which case they'd end up in OP-TEE.

core/drivers/widevine_huk.c Outdated Show resolved Hide resolved
core/drivers/widevine_huk.c Outdated Show resolved Hide resolved
core/pta/widevine.c Outdated Show resolved Hide resolved
core/pta/widevine.c Show resolved Hide resolved
core/pta/widevine.c Outdated Show resolved Hide resolved
core/pta/widevine.c Outdated Show resolved Hide resolved
core/pta/widevine.c Show resolved Hide resolved
lib/libutee/include/pta_widevine.h Show resolved Hide resolved
mk/config.mk Outdated Show resolved Hide resolved
core/pta/widevine.c Show resolved Hide resolved
core/drivers/widevine_huk.c Show resolved Hide resolved
core/drivers/widevine_huk.c Outdated Show resolved Hide resolved
lib/libutee/include/pta_widevine.h Outdated Show resolved Hide resolved
mk/config.mk Outdated Show resolved Hide resolved
core/pta/widevine.c Outdated Show resolved Hide resolved
@qazwsxedcrfvtg14
Copy link
Contributor Author

Rebased and resolved the conflicts with master branch.

core/drivers/sub.mk Show resolved Hide resolved
core/pta/widevine.c Outdated Show resolved Hide resolved
core/pta/widevine.c Outdated Show resolved Hide resolved
core/pta/widevine.c Outdated Show resolved Hide resolved
mk/config.mk Outdated Show resolved Hide resolved
core/drivers/widevine_huk.c Outdated Show resolved Hide resolved
@qazwsxedcrfvtg14
Copy link
Contributor Author

Resolved the comments.

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.

I would rather wait the DT bindings are acked before merging this change.

I still find curious that Widevine mandates the whole OP-TEE to rely on a HUK provided by DT "/options/op-tee/widevine" node without other strict constraints on how this HUK is derived. I also think it puts some restrictions on integration of other TAs when the Widevine TA is to be supported, which maybe makes sense.

@jbech-linaro
Copy link
Contributor

I'm adding the "enhancement" tag, so this won't be closed automatically and since I want this to be merged in one or another way at some point. I do believe this will take some time to sort out. We've just this week had a first call between stakeholders interested in this and we concluded that we have to start with working out the Linux kernel side of things. The secure side, will be a topic in the future when the Linux kernel side of story seems to fall in place. For more information (and a recording of the meeting), please see the topics discussed here.

@qazwsxedcrfvtg14
Copy link
Contributor Author

The device tree binding is merged in the optee_docs:
OP-TEE/optee_docs#230

And rebased the PR.

@jbech-linaro
Copy link
Contributor

@jforissier @jenswi-linaro anything else you'd like to add?

core/pta/widevine.c Outdated Show resolved Hide resolved
core/pta/widevine.c Outdated Show resolved Hide resolved
@jenswi-linaro
Copy link
Contributor

Acked-by: Jens Wiklander <jens.wiklander@linaro.org>

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.

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

On the new ChromeOS mediatek platform, we will use the device tree to
pass hardware unique key and the parameters for widevine TAs.

Signed-off-by: Yi Chou <yich@google.com>
Reviewed-by: Joakim Bech <joakim.bech@linaro.org>
Acked-by: Jens Wiklander <jens.wiklander@linaro.org>
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
@Narflex
Copy link
Contributor

Narflex commented Feb 8, 2024

Can we get this PR merged now?

@jforissier
Copy link
Contributor

@Narflex yes, sorry for the delay.

@jforissier jforissier merged commit ad19495 into OP-TEE:master Feb 9, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants