|
|
Created:
10 years, 3 months ago by Randall Spangler Modified:
9 years, 7 months ago Reviewers:
Luigi Semenzato CC:
chromium-os-reviews_chromium.org, Randall Spangler, gauravsh, Bill Richardson, Luigi Semenzato Visibility:
Public. |
Descriptionuse continue self test
Change-Id: I4785a6326017c63d83a8eb153d6b90ee82e5f839
BUG=chromeos-partner:222
TEST=manual (build FW, make sure system boots and tpmc prints good values)
Patch Set 1 #Patch Set 2 : Use continue self test in startup, full self test in S3 #Patch Set 3 : Use continue self test everywhere #
Messages
Total messages: 9 (0 generated)
While this is surely fine for boot, I am not completely comfortable it will work properly for S3 resume. The reason is that the specs allows a command to fail with a DOING_SELFTEST error if the command needs some TPM resource that has not been tested yet. Now, with NVRAM access and TPM_Extend, we know that they work immediately after a ContinueSelfTest, and any other commands execute much later when booting. We also know that the Infineon full self test runs for a good part of a second---so I don't know what will happen if user-level programs access the TPM while still self-testing. I guess we either go for it and see what happens, or we do a full self test at S3 resume (and take the hit). I can also investigate the code (both the driver and trousers) to see if it copes with it. On 2010/09/08 23:52:32, Randall Spangler wrote: >
On Wed, Sep 8, 2010 at 5:16 PM, <semenzato@chromium.org> wrote: > While this is surely fine for boot, I am not completely comfortable it will > work > properly for S3 resume. The reason is that the specs allows a command to > fail > with a DOING_SELFTEST error if the command needs some TPM resource that has > not > been tested yet. Now, with NVRAM access and TPM_Extend, we know that they > work > immediately after a ContinueSelfTest, and any other commands execute much > later > when booting. We also know that the Infineon full self test runs for a > good > part of a second---so I don't know what will happen if user-level programs > access the TPM while still self-testing. > > I guess we either go for it and see what happens, or we do a full self test > at > S3 resume (and take the hit). I can also investigate the code (both the > driver > and trousers) to see if it copes with it. SGTM for going for it. If the code isn't tolerant of it yet, we should make it tolerant. Worst-case, won't your current TPM hack do a SelfTestFull at driver-time? - Randall > > On 2010/09/08 23:52:32, Randall Spangler wrote: > > > > > http://codereview.chromium.org/3367020/show >
Wait... I was being stupid. We don't need to run the test after S3 because we don't power the TPM off. Let me check the current code. On 2010/09/09 00:26:07, Randall wrote: > On Wed, Sep 8, 2010 at 5:16 PM, <mailto:semenzato@chromium.org> wrote: > > > While this is surely fine for boot, I am not completely comfortable it will > > work > > properly for S3 resume. The reason is that the specs allows a command to > > fail > > with a DOING_SELFTEST error if the command needs some TPM resource that has > > not > > been tested yet. Now, with NVRAM access and TPM_Extend, we know that they > > work > > immediately after a ContinueSelfTest, and any other commands execute much > > later > > when booting. We also know that the Infineon full self test runs for a > > good > > part of a second---so I don't know what will happen if user-level programs > > access the TPM while still self-testing. > > > > I guess we either go for it and see what happens, or we do a full self test > > at > > S3 resume (and take the hit). I can also investigate the code (both the > > driver > > and trousers) to see if it copes with it. > > > SGTM for going for it. If the code isn't tolerant of it yet, we should make > it tolerant. Worst-case, won't your current TPM hack do a SelfTestFull at > driver-time? > > - Randall > > > > > > On 2010/09/08 23:52:32, Randall Spangler wrote: > > > > > > > > > > http://codereview.chromium.org/3367020/show > > >
OK, here's my proposal. Let's be conservative and always do the full self test on S3 resume. That code will eventually be dead anyway, it's only there to support legacy prototype hardware. Do we care if S3 resume will take an extra 0.7s on those machines? Do we need to look further in the future? (i.e. the possibility that future hardware will power off the TPM by design?) On 2010/09/09 00:32:36, Luigi Semenzato wrote: > Wait... I was being stupid. We don't need to run the test after S3 because we > don't power the TPM off. Let me check the current code. > > On 2010/09/09 00:26:07, Randall wrote: > > On Wed, Sep 8, 2010 at 5:16 PM, <mailto:semenzato@chromium.org> wrote: > > > > > While this is surely fine for boot, I am not completely comfortable it will > > > work > > > properly for S3 resume. The reason is that the specs allows a command to > > > fail > > > with a DOING_SELFTEST error if the command needs some TPM resource that has > > > not > > > been tested yet. Now, with NVRAM access and TPM_Extend, we know that they > > > work > > > immediately after a ContinueSelfTest, and any other commands execute much > > > later > > > when booting. We also know that the Infineon full self test runs for a > > > good > > > part of a second---so I don't know what will happen if user-level programs > > > access the TPM while still self-testing. > > > > > > I guess we either go for it and see what happens, or we do a full self test > > > at > > > S3 resume (and take the hit). I can also investigate the code (both the > > > driver > > > and trousers) to see if it copes with it. > > > > > > SGTM for going for it. If the code isn't tolerant of it yet, we should make > > it tolerant. Worst-case, won't your current TPM hack do a SelfTestFull at > > driver-time? > > > > - Randall > > > > > > > > > > On 2010/09/08 23:52:32, Randall Spangler wrote: > > > > > > > > > > > > > > > http://codereview.chromium.org/3367020/show > > > > >
On Wed, Sep 8, 2010 at 5:40 PM, <semenzato@chromium.org> wrote: > OK, here's my proposal. Let's be conservative and always do the full self > test > on S3 resume. That code will eventually be dead anyway, it's only there to > support legacy prototype hardware. Do we care if S3 resume will take an > extra > 0.7s on those machines? Do we need to look further in the future? (i.e. the > possibility that future hardware will power off the TPM by design?) To answer that question, Yes. 0.7 seconds is a long time to be adding to the resume. Users are more likely to do an S3 resume than a full reboot. So resume time matters and we probably want to aim for much less than what we have right now. > > On 2010/09/09 00:32:36, Luigi Semenzato wrote: >> >> Wait... I was being stupid. We don't need to run the test after S3 >> because we >> don't power the TPM off. Let me check the current code. > >> On 2010/09/09 00:26:07, Randall wrote: >> > On Wed, Sep 8, 2010 at 5:16 PM, <mailto:semenzato@chromium.org> wrote: >> > >> > > While this is surely fine for boot, I am not completely comfortable it > > will >> >> > > work >> > > properly for S3 resume. The reason is that the specs allows a command >> > > to >> > > fail >> > > with a DOING_SELFTEST error if the command needs some TPM resource >> > > that > > has >> >> > > not >> > > been tested yet. Now, with NVRAM access and TPM_Extend, we know that >> > > they >> > > work >> > > immediately after a ContinueSelfTest, and any other commands execute >> > > much >> > > later >> > > when booting. We also know that the Infineon full self test runs for >> > > a >> > > good >> > > part of a second---so I don't know what will happen if user-level >> > > programs >> > > access the TPM while still self-testing. >> > > >> > > I guess we either go for it and see what happens, or we do a full self > > test >> >> > > at >> > > S3 resume (and take the hit). I can also investigate the code (both >> > > the >> > > driver >> > > and trousers) to see if it copes with it. >> > >> > >> > SGTM for going for it. If the code isn't tolerant of it yet, we should >> > make >> > it tolerant. Worst-case, won't your current TPM hack do a SelfTestFull >> > at >> > driver-time? >> > >> > - Randall >> > >> > >> > > >> > > On 2010/09/08 23:52:32, Randall Spangler wrote: >> > > >> > > >> > > >> > > >> > > http://codereview.chromium.org/3367020/show >> > > >> > > > > > http://codereview.chromium.org/3367020/show > -- -g
Some more info: it appears that neither the driver or trousers deal with the DOING_SELFTEST error (which they could do by waiting and retrying). I doubt that applications deal with it. It would be reasonable to make this change to the driver and upstream it. In any case I propose that in our firmware we do a SelfTestFull for now. On 2010/09/09 00:40:34, Luigi Semenzato wrote: > OK, here's my proposal. Let's be conservative and always do the full self test > on S3 resume. That code will eventually be dead anyway, it's only there to > support legacy prototype hardware. Do we care if S3 resume will take an extra > 0.7s on those machines? Do we need to look further in the future? (i.e. the > possibility that future hardware will power off the TPM by design?) > > On 2010/09/09 00:32:36, Luigi Semenzato wrote: > > Wait... I was being stupid. We don't need to run the test after S3 because we > > don't power the TPM off. Let me check the current code. > > > > On 2010/09/09 00:26:07, Randall wrote: > > > On Wed, Sep 8, 2010 at 5:16 PM, <mailto:semenzato@chromium.org> wrote: > > > > > > > While this is surely fine for boot, I am not completely comfortable it > will > > > > work > > > > properly for S3 resume. The reason is that the specs allows a command to > > > > fail > > > > with a DOING_SELFTEST error if the command needs some TPM resource that > has > > > > not > > > > been tested yet. Now, with NVRAM access and TPM_Extend, we know that they > > > > work > > > > immediately after a ContinueSelfTest, and any other commands execute much > > > > later > > > > when booting. We also know that the Infineon full self test runs for a > > > > good > > > > part of a second---so I don't know what will happen if user-level programs > > > > access the TPM while still self-testing. > > > > > > > > I guess we either go for it and see what happens, or we do a full self > test > > > > at > > > > S3 resume (and take the hit). I can also investigate the code (both the > > > > driver > > > > and trousers) to see if it copes with it. > > > > > > > > > SGTM for going for it. If the code isn't tolerant of it yet, we should make > > > it tolerant. Worst-case, won't your current TPM hack do a SelfTestFull at > > > driver-time? > > > > > > - Randall > > > > > > > > > > > > > > On 2010/09/08 23:52:32, Randall Spangler wrote: > > > > > > > > > > > > > > > > > > > > http://codereview.chromium.org/3367020/show > > > > > > >
To answer Gaurav's comment: I agree 0.7s is a long time for a production device, but this would happen only on the current prototypes. Is that acceptable? If in future production devices we decide to turn off the TPM during S3, we'll have to revisit this choice, but it seems fair to postpone this. On 2010/09/09 00:53:53, Luigi Semenzato wrote: > Some more info: it appears that neither the driver or trousers deal with the > DOING_SELFTEST error (which they could do by waiting and retrying). I doubt > that applications deal with it. It would be reasonable to make this change to > the driver and upstream it. In any case I propose that in our firmware we do a > SelfTestFull for now. > > On 2010/09/09 00:40:34, Luigi Semenzato wrote: > > OK, here's my proposal. Let's be conservative and always do the full self > test > > on S3 resume. That code will eventually be dead anyway, it's only there to > > support legacy prototype hardware. Do we care if S3 resume will take an extra > > 0.7s on those machines? Do we need to look further in the future? (i.e. the > > possibility that future hardware will power off the TPM by design?) > > > > On 2010/09/09 00:32:36, Luigi Semenzato wrote: > > > Wait... I was being stupid. We don't need to run the test after S3 because > we > > > don't power the TPM off. Let me check the current code. > > > > > > On 2010/09/09 00:26:07, Randall wrote: > > > > On Wed, Sep 8, 2010 at 5:16 PM, <mailto:semenzato@chromium.org> wrote: > > > > > > > > > While this is surely fine for boot, I am not completely comfortable it > > will > > > > > work > > > > > properly for S3 resume. The reason is that the specs allows a command > to > > > > > fail > > > > > with a DOING_SELFTEST error if the command needs some TPM resource that > > has > > > > > not > > > > > been tested yet. Now, with NVRAM access and TPM_Extend, we know that > they > > > > > work > > > > > immediately after a ContinueSelfTest, and any other commands execute > much > > > > > later > > > > > when booting. We also know that the Infineon full self test runs for a > > > > > good > > > > > part of a second---so I don't know what will happen if user-level > programs > > > > > access the TPM while still self-testing. > > > > > > > > > > I guess we either go for it and see what happens, or we do a full self > > test > > > > > at > > > > > S3 resume (and take the hit). I can also investigate the code (both the > > > > > driver > > > > > and trousers) to see if it copes with it. > > > > > > > > > > > > SGTM for going for it. If the code isn't tolerant of it yet, we should > make > > > > it tolerant. Worst-case, won't your current TPM hack do a SelfTestFull at > > > > driver-time? > > > > > > > > - Randall > > > > > > > > > > > > > > > > > > On 2010/09/08 23:52:32, Randall Spangler wrote: > > > > > > > > > > > > > > > > > > > > > > > > > http://codereview.chromium.org/3367020/show > > > > > > > > >
LGTM with either SelfTestFull or ContinueSelfTest on S3 resume. |