[SeaBIOS] [PATCH 0/7] Some TPM code reorganization

[Stefan Berger]

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:
>>>
>>> https://github.com/KevinOConnor/seabios/tree/testing
>> 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