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

Enable cr50 tpm drivers #823

Merged
merged 1 commit into from
Feb 11, 2022
Merged

Conversation

10ne1
Copy link
Contributor

@10ne1 10ne1 commented Sep 25, 2021

HI,

This is the kernelci-core counterpart to the bootrr PR #12.

Here is an example run of the bootrr test with these configs and tpm2-tools on my dev setup.

This adds the required configs to enable the cr50 i2c and spi
TPM chips present on Chromebook devices. The drivers expose
/dev/tpm devices which will then be used by tpm2-tools in
Debian images or a custom TPM userspace stack in ChromeOS
images exercisable via the TAST test suite.

Signed-off-by: Adrian Ratiu <[email protected]>
@10ne1 10ne1 requested review from eballetbo and gctucker September 25, 2021 17:56
@10ne1 10ne1 self-assigned this Sep 25, 2021
@eballetbo
Copy link

See my comment in bootrr PR #12 about tpm2-tools. I think is not needed.

@10ne1
Copy link
Contributor Author

10ne1 commented Sep 27, 2021

@eballetbo I responded to bootrr PR #12. I think we need to keep tpm2-tools because not all drivers expose the same /sys/class/tpm info and by using tpm2-tols we use a standard open source userspace software stack to sanity check any driver on any devices (including non-Chromebooks) by validating communication with the /dev/tpm device directly.

@eballetbo
Copy link

LGTM

@gctucker gctucker requested a review from a team September 27, 2021 13:59
@gctucker
Copy link
Contributor

The drivers expose /dev/tpm devices which will then be used by tpm2-tools in Debian images or a custom TPM userspace stack in ChromeOS images exercisable via the TAST test suite.

Are all the config options useful for tpm2-tools or are some only required for TAST?

Anything that is solely to enable TAST to run should be used only on the chromeos.kernelci.org instance to keep linux.kernelci.org as close to mainline defconfigs as possible.

@10ne1
Copy link
Contributor Author

10ne1 commented Oct 11, 2021

The drivers expose /dev/tpm devices which will then be used by tpm2-tools in Debian images or a custom TPM userspace stack in ChromeOS images exercisable via the TAST test suite.

Are all the config options useful for tpm2-tools or are some only required for TAST?

Anything that is solely to enable TAST to run should be used only on the chromeos.kernelci.org instance to keep linux.kernelci.org as close to mainline defconfigs as possible.

Yes, all these config options are required by tpm2-tools to enable various TPM chips on boards via i2s and i2c.

The choice of userspace stack (tpm2-tools or TAST / google tpm) is actually irrelevant, by enabling these configs any standards compliant userspace implementation should be able to talk to the TPM thips via these drivers. TAST + Google userspace is only available via ChromeOS, but the tpm2-tools is available everywhere including debian & buildroot.

@10ne1 10ne1 force-pushed the dev/aratiu/enable-cr50-tpm branch from 4cad0b3 to 2ac1a33 Compare October 14, 2021 09:21
@10ne1 10ne1 changed the title Enable cr50 tpm drivers and add tpm2-tools to images Enable cr50 tpm drivers Oct 14, 2021
@gctucker
Copy link
Contributor

We'll need a respin of the buildroot image used on staging with the fix in bootrr to test this again. If it's all looking good we could have it merged tomorrow and deployed in production on Monday.

@gctucker
Copy link
Contributor

Sorry for the delay on this. A new buildroot will be tested next week with all the pending changes.

@gctucker
Copy link
Contributor

gctucker commented Nov 5, 2021

We had issues creating new buildroot images as it's now being done with kci_rootfs and the changes didn't work initially. Now this is all being resolved so hopefully all the PRs that depend on buildroot will also get resolved soon.

@nfraprado
Copy link
Contributor

So, I was working on adding some kselftests and totally missed this PR, redoing the work of finding the missing configs. But since this PR is already here, I've rebased my work on top of it.

Since this PR was changed to only add the required kernel configs for adding support for TPM, it is standalone and doesn't require any other PR to get merged (ie it's not blocked). And the configs added in this PR would be useful even without merging the PRs related to the bootrr TPM test, since I've been working on #1025 and #1034 to add the tpm2 kselftest to some platforms and that test requires the configs added here. This also solves the question about the configs being useful upstream, since the tpm2 kselftest is part of the upstream kernel, so the changes in this PR are definitely useful for testing the upstream kernel.

Just to do a breakdown of the configs added here:

Needed for sona (#1034):

CONFIG_TCG_TIS_SPI=y
CONFIG_TCG_TIS_SPI_CR50=y
CONFIG_SPI=y
CONFIG_SPI_PXA2XX=y

Needed for grunt (#1025):

CONFIG_TCG_TIS_I2C_CR50=y

The following configs are needed for both, since they enable basic TPM operation. In theory we could get away without them because they're enabled by default in our current defconfig, but it's a good idea to explicitly enable them so that some future defconfig change doesn't break our TPM tests.

CONFIG_TCG_TPM=y
CONFIG_TCG_TIS=y

So I'd suggest we just merge this PR as is.

@gctucker
Copy link
Contributor

The only thing, reading the discussion history, is that the TPM tests are being enabled in bootrr and that requires an updated buildroot image to confirm the config changes are working as expected. It just got side-tracked, now we have buildroot support in kci_rootfs so it's easier to create new images. It's something we should be able to all test and merge this week.

@nfraprado
Copy link
Contributor

Right, so kernelci/bootrr#12 and kernelci/buildroot#13 which enable a TPM test in bootrr depend on this PR. What I'm saying is that this PR doesn't depend on those two. It just adds some configs, which should just make the TPM module usable in tests, but not change anything else. And for validation that the configs in this PR work, we could use #1025 which also depends on this PR.

But in any case, I'm not trying to rush anything and it's fine to me whichever way it goes, I'm just clarifying that this particular PR is not blocked on anything, and instead is a blocker for both bootrr's and kselftest's TPM tests.

@gctucker
Copy link
Contributor

Yes but if we enable these configs and the TPM tests still don't produce the expected results then there's a problem somewhere. That's why I'd like to have the results before merging this PR, at the same time as the bootrr ones.

Copy link
Contributor

@gctucker gctucker left a comment

Choose a reason for hiding this comment

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

Tested OK on staging with manual build with bootrr and buildroot required changes:
https://lava.collabora.co.uk/scheduler/job/5674285

@gctucker gctucker merged commit 84ad97c into kernelci:main Feb 11, 2022
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.

4 participants