On 11/22/2015 08:26 PM, Kevin O'Connor wrote:
On Sun, Nov 22, 2015 at 06:54:47PM -0500, Stefan Berger wrote:
On 11/22/2015 12:20 PM, Kevin O'Connor wrote:
Hi Stefan,
As part of trying to better understand the TPM code, I found some changes to tcgbios.c were helpful. It's mostly code movement. I've only compile tested these changes, but I think they are safe.
I don't want to conflict with any changes you may have pending. So, let me know if this is a problem.
I do have other changes and extensions pending, but go ahead and apply it.
The series is on top of your latest patches. (FYI, I made a minor change to a comment in your patch 2.) I've also put this series (and your series) up at:
I applied them locally. From what i can see from tests I have done, results are still like before.
ACK to series.
Thanks. I ran across a few other things - I sent an email series and put the changes up on github.
There were a few other things I noticed, but that I have not tried changing:
- It's odd that "Booting from CD ROM device" is added twice for cdrom boots. Is that intentional?
That is not necessary. We can remove one of these log messages.
- It's odd that is_tpm_present() is called from tpm_interrupt_handler32() , as that probes the hardware. I would think that if the has_working_tpm() check in tpm_interrupt_handler32() passes then is_tpm_present() would always pass.
correct.
- It seems the "TPM hardware interface" group of functions could be moved to tpm_drivers.c . Doing that seems like it could simpify the software/hardware interface as only a handful of functions would need to be exported (instead of the function table that tpm_drivers.c currently exports). Could tpm_drivers.c just export something like: tpmhw_probe(), tpmhw_set_timeouts(), tpmhw_transmit() ?
Yes.
I will hold off on further changes until you can merge any features you have pending.
It's mostly the menu that I would want to have merged next. All patches after that are not that important and I'll only resurrect them once the changes here settle.
I looked over and tried your 2nd series of 8 patches and the tests I did all had the same results as without these patches applied. So from my point of view, they can be applied. I leave it up to you regarding 4/8 and the fixed-size buffer on the stack. If we ever have a bigger message to pass then we can adjust the buffer following the warning the code is emitting, otherwise if we pass the event and its size around we won't have to do that. If stack usage is not an issue I think your changes in probably the right direction.
All the arrays at the beginning of the file are basically the bodies of TPM requests. We could extend those arrays by prefixing them with their headers and won't have to build the header in the transmit function anymore. Some messages will still need to be built, like TPM_Extend, but those can be hard-coded. Is that a change we would want?
I am also trying to add similar code to SLOF, which is under BSD. I hope that you agree that I can make similar transformations that you did to the pending SLOF patches as well.
Thanks and regards, Stefan