<tt><font size=2>"Kevin O'Connor" <kevin@koconnor.net>
wrote on 11/30/2015 10:05:22 AM:<br><br>> From: "Kevin O'Connor" <kevin@koconnor.net></font></tt><br><tt><font size=2>> To: Stefan Berger/Watson/IBM@IBMUS</font></tt><br><tt><font size=2>> Cc: seabios@seabios.org, Stefan Berger <stefanb@linux.vnet.ibm.com></font></tt><br><tt><font size=2>> Date: 11/30/2015 10:05 AM</font></tt><br><tt><font size=2>> Subject: Re: [PATCH 3/3] tpm: Add a menu for
TPM configuration</font></tt><br><tt><font size=2>> <br>> On Sun, Nov 29, 2015 at 11:49:43PM -0500, Stefan Berger wrote:<br>> > From: Stefan Berger <stefanb@linux.vnet.ibm.com><br>> > <br>> > This patch adds an new menu entry to the main menu. This menu
item enables<br>> > the user to enter a TPM control menu which allows control of
those aspects<br>> > of the TPM's state that can only be controlled while in the firmware<br>> > and while physical presence can be asserted.<br>> [...]<br>> > --- a/src/std/tcg.h<br>> > +++ b/src/std/tcg.h<br>> [...]<br>> > @@ -325,4 +348,22 @@ struct tpm_res_sha1complete {<br>> >      u8     hash[20];<br>> >  } PACKED;<br>> >  <br>> > +#define TPM_STATE_ENABLED 1<br>> > +#define TPM_STATE_ACTIVE 2<br>> > +#define TPM_STATE_OWNED 4<br>> > +#define TPM_STATE_OWNERINSTALL 8<br>> > +<br>> > +/*<br>> > + * physical presence interface<br>> > + */<br>> > +<br>> > +#define TPM_PPI_OP_NOOP 0<br>> > +#define TPM_PPI_OP_ENABLE 1<br>> > +#define TPM_PPI_OP_DISABLE 2<br>> > +#define TPM_PPI_OP_ACTIVATE 3<br>> > +#define TPM_PPI_OP_DEACTIVATE 4<br>> > +#define TPM_PPI_OP_CLEAR 5<br>> > +#define TPM_PPI_OP_SET_OWNERINSTALL_TRUE 8<br>> > +#define TPM_PPI_OP_SET_OWNERINSTALL_FALSE 9<br>> <br>> Are the above definitions part of the standard, or internal to the<br>> implementation?  If the latter, they should go into tcgbios.[ch]</font></tt><br><br><tt><font size=2>These are actually the 'message codes' from the physical
presence interface spec that can be sent from the OS</font></tt><br><tt><font size=2>to the BIOS and on which the BIOS is supposed to act
upon reboot. You may remember the ACPI patches I had for QEMU</font></tt><br><tt><font size=2>where ACPI would write one of the above number into
an allocated memory area for the BIOS to find. I also built</font></tt><br><tt><font size=2>the menu using those 'message codes'.</font></tt><br><tt><font size=2><br>> <br>> > --- a/src/tcgbios.c<br>> > +++ b/src/tcgbios.c<br>> > @@ -23,6 +23,8 @@<br>> >  #include "string.h" // checksum<br>> >  #include "tcgbios.h"// tpm_*, prototypes<br>> >  #include "util.h" // printf, get_keystroke<br>> > +#include "malloc.h" // malloc_*<br>> > +#include "stacks.h" // wait_threads<br>> <br>> Doesn't look like malloc_x() is used in this code.<br>> <br>> [...]<br>> > +typedef struct {<br>> > +    u8  op;<br>> > +} tpm_bios_cfg;<br>> <br>> What is the purpose of this struct?<br>></font></tt><br><br><tt><font size=2>This datatype is used to send messages from menu item
selections to the part that processes above 'message codes'.</font></tt><br><tt><font size=2> <br>> > +extern void reset_vector(void) __noreturn;<br>> <br>> The code should use reset() (defined in stacks.h).  Directly
calling<br>> reset_vector() in 32bit mode isn't strictly correct.</font></tt><br><br><tt><font size=2>Ok, will change it. (Worked so far...)</font></tt><br><br><br><tt><font size=2>   Stefan</font></tt><br><tt><font size=2><br>> <br>> Looks good to me.<br>> -Kevin<br>> <br></font></tt><BR>