[SeaBIOS] SeaBIOS Digest, Vol 72, Issue 33

[Stefan Berger]

"Kevin O'Connor" <kevin at koconnor.net> wrote on 01/08/2016 01:05:05 PM:

> From: "Kevin O'Connor" <kevin at koconnor.net>
> To: Stefan Berger/Watson/IBM at IBMUS
> Cc: "seabios at seabios.org" <seabios at seabios.org>, Stefan Berger 
> <stefanb at linux.vnet.ibm.com>
> Date: 01/08/2016 01:05 PM
> Subject: Re: [SeaBIOS] SeaBIOS Digest, Vol 72, Issue 33
> 
> On Fri, Jan 08, 2016 at 12:19:52PM -0500, Stefan Berger wrote:
> > "Kevin O'Connor" <kevin at koconnor.net> wrote on 01/08/2016 11:41:13 AM:
> > > On Thu, Jan 07, 2016 at 03:39:13PM -0500, Stefan Berger wrote:
> > > > "Kevin O'Connor" <kevin at koconnor.net> wrote on 01/07/2016 03:14:37 
PM:
> > > > > I don't have input on what TPM2 organization should look like, 
> > mainly
> > > > > because I don't know what TPM2 entails.  I gather the TIS 
commands 
> > are
> > > > > changing, but what else changes?  Does the ACPI log, BIOS 
interface,
> > > > > or tpm menu change?  Do you have a pointer to the TPM2 spec 
(when I
> > > > > last looked it seemed that TPM2 was still being worked on).
> > > > 
> > > > The TIS got more registers; some flags allow detection of the TPM 
> > version.
> > > > 
> > > > All commands changed -- no backwards compatibility. The header 
> > 'fields' 
> > > > are the same, their ordinal and tag values are not.
> > > > 
> > > > Spec: 
> > > > 
> > 
http://www.trustedcomputinggroup.org/resources/tpm_library_specification
> > > 
> > > Thanks.  Does the hardware interface change as well (ie, is it still
> > > the same reads/writes to MMIO at 0xfed40000)?
> > >
> > 
> > It has the same address, but one or two more registers.
> 
> Does it require a different tpm_drivers.c implementation - with
> something like tpmhw1_transmit() and tpmhw2_transmit() functions?

No. Only an extension to the probing function that interprets the flags 
from the
new registers to determine TPM1.2 or TPM2.

> 
> > > My initial thought would be to do what you've proposed - have 
wrapper
> > > functions around the TPM commands (eg, tpm_extend, 
tpm_get_capability,
> > > read_permanent_flags) and teach those functions how to send the two
> > > different styles of commands (and translate the responses if
> > > necessary).
> > 
> > So the good thing is that some of the code can be shared between 1.2 
and 
> > 2.0,
> > to a certain 'depth' at least. An example of a shared function would 
be 
> > this one.
> > 
> > static void
> > tpm_add_event_separators(void)
> > {
> >     static const u8 evt_separator[] = {0xff,0xff,0xff,0xff};
> >     u32 pcrIndex;
> >     for (pcrIndex = 0; pcrIndex <= 7; pcrIndex++)
> >         tpm_add_measurement_to_log(pcrIndex, EV_SEPARATOR,
> >                                    NULL, 0,
> >                                    evt_separator,
> >                                    sizeof(evt_separator));
> > }
> > 
> > Following this function further down:
> > 
> > tpm_add_measurement_to_log() [on current master] can be completely
> > shared as well. tpm_log_extend_event would need to become a function 
that
> > branches into tpm12_log_extend_event and tpm2_log_extend_event, 
depending
> > on detected version of TPM. 
> 
> Sounds like a new tpm_extend() function could be made with just the
> hardware command.  And then it could handle the v1 and v2 cases and
> thus reduce the amount of duplicated code.

Yes, there should be a tpm_extend() function branching into tpm12_extend()
and tpm2_extend().

> 
> > tpm_log_event could again be shared since ACPI logging is the same.
> > Same for tpm_fill_hash for as long as we only support sha1.
> > 
> > Basically all functions where commands are created cannot be shared.
> > Also TPM 2's initialization is a bit different and it supports more
> > hashes.
> 
> If the init is notable different maybe just do tpm1_startup() and
> tpm2_startup()?

Yes, something like that.

> 
> > So it actually speaks against splitting this up into different files, 
but 
> > the
> > outcome may be that the code would show a mix of tpm12_*, tpm2_*, and
> > tpm_* functions in the format of
> > 
> > tpm12_foo() { [...] }
> > 
> > tpm2_foo() { [...] }
> > 
> > tpm_foo() {
> >    switch (tpmversion) {
> >    1.2:
> >       return tpm12_foo()
> >    2:
> >       return tpm2_foo()
> >    }
> > }
> 
> Okay.  But, I would say that if both tpm12_foo() and tpm2_foo() are
> both only a few lines that it might be better to just inline it all
> into tpm_foo().  An 'if' might be more succinct also - hopefully there
> isn't a tpm3 in the works..

Not that I know of :-)

> 
> > tpm_xyz() { [...] }
> > 
> > tpm12_bar() { [...] }
> > 
> > tpm2_bar() { [...] }
> > 
> > [...]
> > 
> > That's what I did before...
> 
> Oh, were there tpm2 patches available?  I must have missed them.

No, I never showed them.

   Stefan

> 
> > If none of the code could be shared the decision to split it up 
completely 
> > would be a lot easier.
> 
> Agreed.
> 
> -Kevin
> 


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.seabios.org/pipermail/seabios/attachments/20160108/b31b0261/attachment.html>