Add support for TPM 1.2 and TPM 2 Physical Presence interface (PPI).
A shared memory structure is located at 0xfffe f000 - 0xfffe f3ff
that SeaBIOS initializes (unless it has already been intialized) and
then searches for a code it is supposed to act upon. A code typically
requires that one or more TPM commands are being sent.

The underlying spec can be accessed from this page here:

https://trustedcomputinggroup.org/tcg-physical-presence-interface-specification/

Version 1.30 is implemented.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 src/post.c     |  4 +++
 src/std/acpi.h | 10 ++++++
 src/std/tcg.h  | 31 ++++++++++++++++++
 src/tcgbios.c  | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/tcgbios.h  |  3 ++
 5 files changed, 147 insertions(+)

diff --git a/src/post.c b/src/post.c
index f93106a..f451013 100644
--- a/src/post.c
+++ b/src/post.c
@@ -201,6 +201,7 @@ maininit(void)
 
     // Setup platform devices.
     platform_hardware_setup();
+    tpm_ppi_init();
 
     // Start hardware initialization (if threads allowed during optionroms)
     if (threads_during_optionroms())
@@ -220,6 +221,9 @@ maininit(void)
     // Run option roms
     optionrom_setup();
 
+    // Process user-requested TPM state change
+    tpm_ppi_process();
+
     // Allow user to modify overall boot order.
     interactive_bootmenu();
     wait_threads();
diff --git a/src/std/acpi.h b/src/std/acpi.h
index c01fa7b..5f2e8b7 100644
--- a/src/std/acpi.h
+++ b/src/std/acpi.h
@@ -320,4 +320,14 @@ struct tpm2_descriptor_rev2
     u64  log_area_start_address;
 } PACKED;
 
+#define QEMU_SIGNATURE 0x554d4551
+struct qemu_descriptor
+{
+    ACPI_TABLE_HEADER_DEF
+    u32 tpmppi_address;
+    u8 tpm_version; /* 1 = 1.2, 2 = 2 */
+    u8 tpmppi_version;
+#define TPM_PPI_VERSION_1_30   1
+} PACKED;
+
 #endif // acpi.h
diff --git a/src/std/tcg.h b/src/std/tcg.h
index 09a92d8..22353a9 100644
--- a/src/std/tcg.h
+++ b/src/std/tcg.h
@@ -551,4 +551,35 @@ struct pcctes_romex
 #define TPM_PPI_OP_SET_OWNERINSTALL_TRUE 8
 #define TPM_PPI_OP_SET_OWNERINSTALL_FALSE 9
 
+struct tpm_ppi {
+    u8 ppin;            /*  0: 1 = initialized */
+    u32 ppip;           /*  1: not used */
+    u32 pprp;           /*  5: response from TPM; set by BIOS */
+    u32 pprq;           /*  9: opcode; set by ACPI */
+    u32 pprm;           /* 13: parameter for opcode; set by ACPI */
+    u32 lppr;           /* 17: last opcode; set by BIOS */
+    u32 fret;           /* 21: not used */
+    u8 res1;            /* 25: reserved */
+    u32 res2[4];        /* 26: reserved */
+    u8 res3[214];       /* 42: reserved */
+    u8 func[256];       /* 256: per function implementation flags; set by BIOS */
+/* indication whether function is implemented; bit 0 */
+#define TPM_PPI_FUNC_IMPLEMENTED       (1 << 0)
+/* actions OS should take to transition to the pre-OS env.; bits 1, 2 */
+#define TPM_PPI_FUNC_ACTION_SHUTDOWN   (1 << 1)
+#define TPM_PPI_FUNC_ACTION_REBOOT     (2 << 1)
+#define TPM_PPI_FUNC_ACTION_VENDOR     (3 << 1)
+#define TPM_PPI_FUNC_ACTION_MASK       (3 << 1)
+/* whether function is blocked by BIOS settings; bits 3,4,5 */
+#define TPM_PPI_FUNC_NOT_IMPLEMENTED     (0 << 3)
+#define TPM_PPI_FUNC_BIOS_ONLY           (1 << 3)
+#define TPM_PPI_FUNC_BLOCKED             (2 << 3)
+#define TPM_PPI_FUNC_ALLOWED_USR_REQ     (3 << 3)
+#define TPM_PPI_FUNC_ALLOWED_USR_NOT_REQ (4 << 3)
+#define TPM_PPI_FUNC_MASK                (7 << 3)
+} PACKED;
+
+void tpm_ppi_init(void);
+void tpm_ppi_process(void);
+
 #endif // tcg.h
diff --git a/src/tcgbios.c b/src/tcgbios.c
index 730b5e7..c8e6ca2 100644
--- a/src/tcgbios.c
+++ b/src/tcgbios.c
@@ -1783,6 +1783,18 @@ tpm20_process_cfg(tpm_ppi_code msgCode, int verbose, u32 *returnCode)
 }
 
 static int
+tpm_process_cfg(tpm_ppi_code msgCode, int verbose, u32 *returnCode)
+{
+    switch (TPM_version) {
+    case TPM_VERSION_1_2:
+        return tpm12_process_cfg(msgCode, verbose, returnCode);
+    case TPM_VERSION_2:
+        return tpm20_process_cfg(msgCode, verbose, returnCode);
+    }
+    return -1;
+}
+
+static int
 tpm12_get_tpm_state(void)
 {
     int state = 0;
@@ -2021,3 +2033,90 @@ tpm_can_show_menu(void)
     }
     return 0;
 }
+
+static struct tpm_ppi *tp;
+static u8 nextStep = TPM_PPI_OP_NOOP; /* opcode to execute after reboot */
+
+#define FLAGS (TPM_PPI_FUNC_IMPLEMENTED | \
+               TPM_PPI_FUNC_ACTION_REBOOT | \
+               TPM_PPI_FUNC_ALLOWED_USR_NOT_REQ)
+
+static const u8 tpm12_ppi_funcs[] = {
+    [TPM_PPI_OP_NOOP] = TPM_PPI_FUNC_IMPLEMENTED |
+                        TPM_PPI_FUNC_ALLOWED_USR_NOT_REQ,
+    [TPM_PPI_OP_ENABLE]  = FLAGS,
+    [TPM_PPI_OP_DISABLE] = FLAGS,
+    [TPM_PPI_OP_ACTIVATE] = FLAGS,
+    [TPM_PPI_OP_DEACTIVATE] = FLAGS,
+    [TPM_PPI_OP_CLEAR] = FLAGS,
+    [TPM_PPI_OP_SET_OWNERINSTALL_TRUE] = FLAGS,
+    [TPM_PPI_OP_SET_OWNERINSTALL_FALSE] = FLAGS,
+};
+
+static const u8 tpm2_ppi_funcs[] = {
+    [TPM_PPI_OP_CLEAR] = FLAGS,
+};
+
+void
+tpm_ppi_init(void)
+{
+    struct qemu_descriptor *qemu = NULL;
+
+    while (1) {
+        qemu  = find_acpi_table_iter(QEMU_SIGNATURE, qemu);
+        if (!qemu)
+            return;
+        if (!memcmp("QEMU", qemu->oem_id, 5) && !memcmp("CONF", qemu->oem_table_id, 5))
+            break;
+    }
+
+    tp = (struct tpm_ppi *)(u32)qemu->tpmppi_address;
+    dprintf(DEBUG_tcg, "TCGBIOS: TPM PPI struct at %p\n", tp);
+
+    memset(&tp->func, 0, sizeof(tp->func));
+    switch (qemu->tpmppi_version) {
+    case TPM_PPI_VERSION_1_30:
+        switch (qemu->tpm_version) {
+        case TPM_VERSION_1_2:
+            memcpy(&tp->func, tpm12_ppi_funcs, sizeof(tpm12_ppi_funcs));
+            break;
+        case TPM_VERSION_2:
+            memcpy(&tp->func, tpm2_ppi_funcs, sizeof(tpm2_ppi_funcs));
+            break;
+        }
+        break;
+    }
+
+    if (!tp->ppin) {
+        tp->ppin = 1;
+        tp->pprq = 0;
+        tp->lppr = 0;
+    }
+}
+
+void
+tpm_ppi_process(void)
+{
+   tpm_ppi_code op;
+
+   if (tp) {
+        op = tp->pprq;
+        if (!op) {
+            /* intermediate step after a reboot? */
+            op = nextStep;
+        } else {
+            /* last full opcode */
+            tp->lppr = op;
+        }
+        if (op) {
+            /*
+             * Reset the opcode so we don't permanently reboot upon
+             * code 3 (Activate).
+             */
+            tp->pprq = 0;
+
+            printf("Processing TPM PPI opcode %d\n", op);
+            tpm_process_cfg(op, 0, &tp->pprp);
+        }
+   }
+}
diff --git a/src/tcgbios.h b/src/tcgbios.h
index 32fb941..52b86f2 100644
--- a/src/tcgbios.h
+++ b/src/tcgbios.h
@@ -16,4 +16,7 @@ void tpm_option_rom(const void *addr, u32 len);
 int tpm_can_show_menu(void);
 void tpm_menu(void);
 
+void tpm_ppi_init(void);
+void tpm_ppi_process(void);
+
 #endif /* TCGBIOS_H */
-- 
2.5.5


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

On Tue, Jan 16, 2018 at 11:41:02AM -0500, Stefan Berger wrote:
> Add support for TPM 1.2 and TPM 2 Physical Presence interface (PPI).
> A shared memory structure is located at 0xfffe f000 - 0xfffe f3ff
> that SeaBIOS initializes (unless it has already been intialized) and
> then searches for a code it is supposed to act upon. A code typically
> requires that one or more TPM commands are being sent.

If I'm understanding the code correctly, it no longer hardcodes
0xfffef000 (great!).  The commit comment should also be updated.

> 
> The underlying spec can be accessed from this page here:
> 
> https://trustedcomputinggroup.org/tcg-physical-presence-interface-specification/
> 
> Version 1.30 is implemented.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
>  src/post.c     |  4 +++
>  src/std/acpi.h | 10 ++++++
>  src/std/tcg.h  | 31 ++++++++++++++++++
>  src/tcgbios.c  | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/tcgbios.h  |  3 ++
>  5 files changed, 147 insertions(+)
> 
> diff --git a/src/post.c b/src/post.c
> index f93106a..f451013 100644
> --- a/src/post.c
> +++ b/src/post.c
> @@ -201,6 +201,7 @@ maininit(void)
>  
>      // Setup platform devices.
>      platform_hardware_setup();
> +    tpm_ppi_init();
>  
>      // Start hardware initialization (if threads allowed during optionroms)
>      if (threads_during_optionroms())
> @@ -220,6 +221,9 @@ maininit(void)
>      // Run option roms
>      optionrom_setup();
>  
> +    // Process user-requested TPM state change
> +    tpm_ppi_process();

I think it would be better if these two calls were added to the
existing tpm_setup() call.

Also, function suffixes of "_init" and "_setup" have specific meaning
in seabios - see docs/Execution_and_code_flow.md - so you should not
export a function with those sufixes unless they follow the
convention.

[...]
> --- a/src/std/acpi.h
> +++ b/src/std/acpi.h
> @@ -320,4 +320,14 @@ struct tpm2_descriptor_rev2
>      u64  log_area_start_address;
>  } PACKED;
>  
> +#define QEMU_SIGNATURE 0x554d4551
> +struct qemu_descriptor
> +{
> +    ACPI_TABLE_HEADER_DEF
> +    u32 tpmppi_address;
> +    u8 tpm_version; /* 1 = 1.2, 2 = 2 */
> +    u8 tpmppi_version;
> +#define TPM_PPI_VERSION_1_30   1
> +} PACKED;

I'm confused at the purpose of this acpi table.  If I'm understanding
it correctly, it is purely to pass information from QEMU to SeaBIOS
(and perhaps OVMF?).  If so, I don't think this is a good way to do it
- a regular fw_cfg setting seems simpler (and less likely to cause
problems with OSes).

> +
>  #endif // acpi.h
> diff --git a/src/std/tcg.h b/src/std/tcg.h
> index 09a92d8..22353a9 100644
> --- a/src/std/tcg.h
> +++ b/src/std/tcg.h
> @@ -551,4 +551,35 @@ struct pcctes_romex
>  #define TPM_PPI_OP_SET_OWNERINSTALL_TRUE 8
>  #define TPM_PPI_OP_SET_OWNERINSTALL_FALSE 9
>  
> +struct tpm_ppi {
> +    u8 ppin;            /*  0: 1 = initialized */
> +    u32 ppip;           /*  1: not used */
> +    u32 pprp;           /*  5: response from TPM; set by BIOS */
> +    u32 pprq;           /*  9: opcode; set by ACPI */
> +    u32 pprm;           /* 13: parameter for opcode; set by ACPI */
> +    u32 lppr;           /* 17: last opcode; set by BIOS */
> +    u32 fret;           /* 21: not used */
> +    u8 res1;            /* 25: reserved */
> +    u32 res2[4];        /* 26: reserved */
> +    u8 res3[214];       /* 42: reserved */
> +    u8 func[256];       /* 256: per function implementation flags; set by BIOS */
> +/* indication whether function is implemented; bit 0 */
> +#define TPM_PPI_FUNC_IMPLEMENTED       (1 << 0)
> +/* actions OS should take to transition to the pre-OS env.; bits 1, 2 */
> +#define TPM_PPI_FUNC_ACTION_SHUTDOWN   (1 << 1)
> +#define TPM_PPI_FUNC_ACTION_REBOOT     (2 << 1)
> +#define TPM_PPI_FUNC_ACTION_VENDOR     (3 << 1)
> +#define TPM_PPI_FUNC_ACTION_MASK       (3 << 1)
> +/* whether function is blocked by BIOS settings; bits 3,4,5 */
> +#define TPM_PPI_FUNC_NOT_IMPLEMENTED     (0 << 3)
> +#define TPM_PPI_FUNC_BIOS_ONLY           (1 << 3)
> +#define TPM_PPI_FUNC_BLOCKED             (2 << 3)
> +#define TPM_PPI_FUNC_ALLOWED_USR_REQ     (3 << 3)
> +#define TPM_PPI_FUNC_ALLOWED_USR_NOT_REQ (4 << 3)
> +#define TPM_PPI_FUNC_MASK                (7 << 3)
> +} PACKED;
> +
> +void tpm_ppi_init(void);
> +void tpm_ppi_process(void);

The files in the std/ directory are for well defined specifications.
It should not be used to export seabios functions.  Also, it's not
clear to me if 'struct tpm_ppi' and the TPM_PPI_FUNC_* defines are a
qemu/acpi/seabios interface or a TPM standard.

> +
>  #endif // tcg.h
> diff --git a/src/tcgbios.c b/src/tcgbios.c
> index 730b5e7..c8e6ca2 100644
> --- a/src/tcgbios.c
> +++ b/src/tcgbios.c
> @@ -1783,6 +1783,18 @@ tpm20_process_cfg(tpm_ppi_code msgCode, int verbose, u32 *returnCode)
>  }
>  
>  static int
> +tpm_process_cfg(tpm_ppi_code msgCode, int verbose, u32 *returnCode)
> +{
> +    switch (TPM_version) {
> +    case TPM_VERSION_1_2:
> +        return tpm12_process_cfg(msgCode, verbose, returnCode);
> +    case TPM_VERSION_2:
> +        return tpm20_process_cfg(msgCode, verbose, returnCode);
> +    }
> +    return -1;
> +}
> +
> +static int
>  tpm12_get_tpm_state(void)
>  {
>      int state = 0;
> @@ -2021,3 +2033,90 @@ tpm_can_show_menu(void)
>      }
>      return 0;
>  }
> +
> +static struct tpm_ppi *tp;
> +static u8 nextStep = TPM_PPI_OP_NOOP; /* opcode to execute after reboot */
> +
> +#define FLAGS (TPM_PPI_FUNC_IMPLEMENTED | \
> +               TPM_PPI_FUNC_ACTION_REBOOT | \
> +               TPM_PPI_FUNC_ALLOWED_USR_NOT_REQ)
> +
> +static const u8 tpm12_ppi_funcs[] = {
> +    [TPM_PPI_OP_NOOP] = TPM_PPI_FUNC_IMPLEMENTED |
> +                        TPM_PPI_FUNC_ALLOWED_USR_NOT_REQ,
> +    [TPM_PPI_OP_ENABLE]  = FLAGS,
> +    [TPM_PPI_OP_DISABLE] = FLAGS,
> +    [TPM_PPI_OP_ACTIVATE] = FLAGS,
> +    [TPM_PPI_OP_DEACTIVATE] = FLAGS,
> +    [TPM_PPI_OP_CLEAR] = FLAGS,
> +    [TPM_PPI_OP_SET_OWNERINSTALL_TRUE] = FLAGS,
> +    [TPM_PPI_OP_SET_OWNERINSTALL_FALSE] = FLAGS,
> +};
> +
> +static const u8 tpm2_ppi_funcs[] = {
> +    [TPM_PPI_OP_CLEAR] = FLAGS,
> +};
> +
> +void
> +tpm_ppi_init(void)
> +{
> +    struct qemu_descriptor *qemu = NULL;
> +
> +    while (1) {
> +        qemu  = find_acpi_table_iter(QEMU_SIGNATURE, qemu);
> +        if (!qemu)
> +            return;
> +        if (!memcmp("QEMU", qemu->oem_id, 5) && !memcmp("CONF", qemu->oem_table_id, 5))
> +            break;
> +    }
> +
> +    tp = (struct tpm_ppi *)(u32)qemu->tpmppi_address;
> +    dprintf(DEBUG_tcg, "TCGBIOS: TPM PPI struct at %p\n", tp);
> +
> +    memset(&tp->func, 0, sizeof(tp->func));
> +    switch (qemu->tpmppi_version) {
> +    case TPM_PPI_VERSION_1_30:
> +        switch (qemu->tpm_version) {
> +        case TPM_VERSION_1_2:
> +            memcpy(&tp->func, tpm12_ppi_funcs, sizeof(tpm12_ppi_funcs));
> +            break;
> +        case TPM_VERSION_2:
> +            memcpy(&tp->func, tpm2_ppi_funcs, sizeof(tpm2_ppi_funcs));
> +            break;
> +        }

Can you elaborate on what this does?  Why is SeaBIOS filling a memory
addreses created by QEMU?  Why wouldn't QEMU just fill it with what it
wants directly?

-Kevin


> +        break;
> +    }
> +
> +    if (!tp->ppin) {
> +        tp->ppin = 1;
> +        tp->pprq = 0;
> +        tp->lppr = 0;
> +    }
> +}
> +
> +void
> +tpm_ppi_process(void)
> +{
> +   tpm_ppi_code op;
> +
> +   if (tp) {
> +        op = tp->pprq;
> +        if (!op) {
> +            /* intermediate step after a reboot? */
> +            op = nextStep;
> +        } else {
> +            /* last full opcode */
> +            tp->lppr = op;
> +        }
> +        if (op) {
> +            /*
> +             * Reset the opcode so we don't permanently reboot upon
> +             * code 3 (Activate).
> +             */
> +            tp->pprq = 0;
> +
> +            printf("Processing TPM PPI opcode %d\n", op);
> +            tpm_process_cfg(op, 0, &tp->pprp);
> +        }
> +   }
> +}
> diff --git a/src/tcgbios.h b/src/tcgbios.h
> index 32fb941..52b86f2 100644
> --- a/src/tcgbios.h
> +++ b/src/tcgbios.h
> @@ -16,4 +16,7 @@ void tpm_option_rom(const void *addr, u32 len);
>  int tpm_can_show_menu(void);
>  void tpm_menu(void);
>  
> +void tpm_ppi_init(void);
> +void tpm_ppi_process(void);
> +
>  #endif /* TCGBIOS_H */
> -- 
> 2.5.5
> 

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

On 01/16/18 19:36, Kevin O'Connor wrote:
> On Tue, Jan 16, 2018 at 11:41:02AM -0500, Stefan Berger wrote:
>> Add support for TPM 1.2 and TPM 2 Physical Presence interface (PPI).
>> A shared memory structure is located at 0xfffe f000 - 0xfffe f3ff
>> that SeaBIOS initializes (unless it has already been intialized) and
>> then searches for a code it is supposed to act upon. A code typically
>> requires that one or more TPM commands are being sent.
> 
> If I'm understanding the code correctly, it no longer hardcodes
> 0xfffef000 (great!).  The commit comment should also be updated.
> 
>>
>> The underlying spec can be accessed from this page here:
>>
>> https://trustedcomputinggroup.org/tcg-physical-presence-interface-specification/
>>
>> Version 1.30 is implemented.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> ---
>>  src/post.c     |  4 +++
>>  src/std/acpi.h | 10 ++++++
>>  src/std/tcg.h  | 31 ++++++++++++++++++
>>  src/tcgbios.c  | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  src/tcgbios.h  |  3 ++
>>  5 files changed, 147 insertions(+)

[...]

>> --- a/src/std/acpi.h
>> +++ b/src/std/acpi.h
>> @@ -320,4 +320,14 @@ struct tpm2_descriptor_rev2
>>      u64  log_area_start_address;
>>  } PACKED;
>>  
>> +#define QEMU_SIGNATURE 0x554d4551
>> +struct qemu_descriptor
>> +{
>> +    ACPI_TABLE_HEADER_DEF
>> +    u32 tpmppi_address;
>> +    u8 tpm_version; /* 1 = 1.2, 2 = 2 */
>> +    u8 tpmppi_version;
>> +#define TPM_PPI_VERSION_1_30   1
>> +} PACKED;
> 
> I'm confused at the purpose of this acpi table.  If I'm understanding
> it correctly, it is purely to pass information from QEMU to SeaBIOS
> (and perhaps OVMF?).  If so, I don't think this is a good way to do it
> - a regular fw_cfg setting seems simpler (and less likely to cause
> problems with OSes).

I agree; if the firmware is supposed to consume information from QEMU
for locating the register block of this platform device, please expose
the address in a new fw_cfg file.

Thanks!
Laszlo

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

On 01/16/2018 01:36 PM, Kevin O'Connor wrote:
> On Tue, Jan 16, 2018 at 11:41:02AM -0500, Stefan Berger wrote:
>> Add support for TPM 1.2 and TPM 2 Physical Presence interface (PPI).
>> A shared memory structure is located at 0xfffe f000 - 0xfffe f3ff
>> that SeaBIOS initializes (unless it has already been intialized) and
>> then searches for a code it is supposed to act upon. A code typically
>> requires that one or more TPM commands are being sent.
> If I'm understanding the code correctly, it no longer hardcodes
> 0xfffef000 (great!).  The commit comment should also be updated.
>
>> The underlying spec can be accessed from this page here:
>>
>> https://trustedcomputinggroup.org/tcg-physical-presence-interface-specification/
>>
>> Version 1.30 is implemented.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> ---
>>   src/post.c     |  4 +++
>>   src/std/acpi.h | 10 ++++++
>>   src/std/tcg.h  | 31 ++++++++++++++++++
>>   src/tcgbios.c  | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   src/tcgbios.h  |  3 ++
>>   5 files changed, 147 insertions(+)
>>
>> diff --git a/src/post.c b/src/post.c
>> index f93106a..f451013 100644
>> --- a/src/post.c
>> +++ b/src/post.c
>> @@ -201,6 +201,7 @@ maininit(void)
>>   
>>       // Setup platform devices.
>>       platform_hardware_setup();
>> +    tpm_ppi_init();
>>   
>>       // Start hardware initialization (if threads allowed during optionroms)
>>       if (threads_during_optionroms())
>> @@ -220,6 +221,9 @@ maininit(void)
>>       // Run option roms
>>       optionrom_setup();
>>   
>> +    // Process user-requested TPM state change
>> +    tpm_ppi_process();
> I think it would be better if these two calls were added to the
> existing tpm_setup() call.

Done.

>
> Also, function suffixes of "_init" and "_setup" have specific meaning
> in seabios - see docs/Execution_and_code_flow.md - so you should not
> export a function with those sufixes unless they follow the
> convention.
>
> [...]
>> --- a/src/std/acpi.h
>> +++ b/src/std/acpi.h
>> @@ -320,4 +320,14 @@ struct tpm2_descriptor_rev2
>>       u64  log_area_start_address;
>>   } PACKED;
>>   
>> +#define QEMU_SIGNATURE 0x554d4551
>> +struct qemu_descriptor
>> +{
>> +    ACPI_TABLE_HEADER_DEF
>> +    u32 tpmppi_address;
>> +    u8 tpm_version; /* 1 = 1.2, 2 = 2 */
>> +    u8 tpmppi_version;
>> +#define TPM_PPI_VERSION_1_30   1
>> +} PACKED;
> I'm confused at the purpose of this acpi table.  If I'm understanding
> it correctly, it is purely to pass information from QEMU to SeaBIOS
> (and perhaps OVMF?).  If so, I don't think this is a good way to do it
> - a regular fw_cfg setting seems simpler (and less likely to cause
> problems with OSes).
>
>> +
>>   #endif // acpi.h
>> diff --git a/src/std/tcg.h b/src/std/tcg.h
>> index 09a92d8..22353a9 100644
>> --- a/src/std/tcg.h
>> +++ b/src/std/tcg.h
>> @@ -551,4 +551,35 @@ struct pcctes_romex
>>   #define TPM_PPI_OP_SET_OWNERINSTALL_TRUE 8
>>   #define TPM_PPI_OP_SET_OWNERINSTALL_FALSE 9
>>   
>> +struct tpm_ppi {
>> +    u8 ppin;            /*  0: 1 = initialized */
>> +    u32 ppip;           /*  1: not used */
>> +    u32 pprp;           /*  5: response from TPM; set by BIOS */
>> +    u32 pprq;           /*  9: opcode; set by ACPI */
>> +    u32 pprm;           /* 13: parameter for opcode; set by ACPI */
>> +    u32 lppr;           /* 17: last opcode; set by BIOS */
>> +    u32 fret;           /* 21: not used */
>> +    u8 res1;            /* 25: reserved */
>> +    u32 res2[4];        /* 26: reserved */
>> +    u8 res3[214];       /* 42: reserved */
>> +    u8 func[256];       /* 256: per function implementation flags; set by BIOS */
>> +/* indication whether function is implemented; bit 0 */
>> +#define TPM_PPI_FUNC_IMPLEMENTED       (1 << 0)
>> +/* actions OS should take to transition to the pre-OS env.; bits 1, 2 */
>> +#define TPM_PPI_FUNC_ACTION_SHUTDOWN   (1 << 1)
>> +#define TPM_PPI_FUNC_ACTION_REBOOT     (2 << 1)
>> +#define TPM_PPI_FUNC_ACTION_VENDOR     (3 << 1)
>> +#define TPM_PPI_FUNC_ACTION_MASK       (3 << 1)
>> +/* whether function is blocked by BIOS settings; bits 3,4,5 */
>> +#define TPM_PPI_FUNC_NOT_IMPLEMENTED     (0 << 3)
>> +#define TPM_PPI_FUNC_BIOS_ONLY           (1 << 3)
>> +#define TPM_PPI_FUNC_BLOCKED             (2 << 3)
>> +#define TPM_PPI_FUNC_ALLOWED_USR_REQ     (3 << 3)
>> +#define TPM_PPI_FUNC_ALLOWED_USR_NOT_REQ (4 << 3)
>> +#define TPM_PPI_FUNC_MASK                (7 << 3)
>> +} PACKED;
>> +
>> +void tpm_ppi_init(void);
>> +void tpm_ppi_process(void);
> The files in the std/ directory are for well defined specifications.
> It should not be used to export seabios functions.  Also, it's not
> clear to me if 'struct tpm_ppi' and the TPM_PPI_FUNC_* defines are a
> qemu/acpi/seabios interface or a TPM standard.

Move the definition of tpm_ppi struct to tcgbios.c. It's shared between 
ACPI and firmware.

>
>> +
>>   #endif // tcg.h
>> diff --git a/src/tcgbios.c b/src/tcgbios.c
>> index 730b5e7..c8e6ca2 100644
>> --- a/src/tcgbios.c
>> +++ b/src/tcgbios.c
>> @@ -1783,6 +1783,18 @@ tpm20_process_cfg(tpm_ppi_code msgCode, int verbose, u32 *returnCode)
>>   }
>>   
>>   static int
>> +tpm_process_cfg(tpm_ppi_code msgCode, int verbose, u32 *returnCode)
>> +{
>> +    switch (TPM_version) {
>> +    case TPM_VERSION_1_2:
>> +        return tpm12_process_cfg(msgCode, verbose, returnCode);
>> +    case TPM_VERSION_2:
>> +        return tpm20_process_cfg(msgCode, verbose, returnCode);
>> +    }
>> +    return -1;
>> +}
>> +
>> +static int
>>   tpm12_get_tpm_state(void)
>>   {
>>       int state = 0;
>> @@ -2021,3 +2033,90 @@ tpm_can_show_menu(void)
>>       }
>>       return 0;
>>   }
>> +
>> +static struct tpm_ppi *tp;
>> +static u8 nextStep = TPM_PPI_OP_NOOP; /* opcode to execute after reboot */
>> +
>> +#define FLAGS (TPM_PPI_FUNC_IMPLEMENTED | \
>> +               TPM_PPI_FUNC_ACTION_REBOOT | \
>> +               TPM_PPI_FUNC_ALLOWED_USR_NOT_REQ)
>> +
>> +static const u8 tpm12_ppi_funcs[] = {
>> +    [TPM_PPI_OP_NOOP] = TPM_PPI_FUNC_IMPLEMENTED |
>> +                        TPM_PPI_FUNC_ALLOWED_USR_NOT_REQ,
>> +    [TPM_PPI_OP_ENABLE]  = FLAGS,
>> +    [TPM_PPI_OP_DISABLE] = FLAGS,
>> +    [TPM_PPI_OP_ACTIVATE] = FLAGS,
>> +    [TPM_PPI_OP_DEACTIVATE] = FLAGS,
>> +    [TPM_PPI_OP_CLEAR] = FLAGS,
>> +    [TPM_PPI_OP_SET_OWNERINSTALL_TRUE] = FLAGS,
>> +    [TPM_PPI_OP_SET_OWNERINSTALL_FALSE] = FLAGS,
>> +};
>> +
>> +static const u8 tpm2_ppi_funcs[] = {
>> +    [TPM_PPI_OP_CLEAR] = FLAGS,
>> +};
>> +
>> +void
>> +tpm_ppi_init(void)
>> +{
>> +    struct qemu_descriptor *qemu = NULL;
>> +
>> +    while (1) {
>> +        qemu  = find_acpi_table_iter(QEMU_SIGNATURE, qemu);
>> +        if (!qemu)
>> +            return;
>> +        if (!memcmp("QEMU", qemu->oem_id, 5) && !memcmp("CONF", qemu->oem_table_id, 5))
>> +            break;
>> +    }
>> +
>> +    tp = (struct tpm_ppi *)(u32)qemu->tpmppi_address;
>> +    dprintf(DEBUG_tcg, "TCGBIOS: TPM PPI struct at %p\n", tp);
>> +
>> +    memset(&tp->func, 0, sizeof(tp->func));
>> +    switch (qemu->tpmppi_version) {
>> +    case TPM_PPI_VERSION_1_30:
>> +        switch (qemu->tpm_version) {
>> +        case TPM_VERSION_1_2:
>> +            memcpy(&tp->func, tpm12_ppi_funcs, sizeof(tpm12_ppi_funcs));
>> +            break;
>> +        case TPM_VERSION_2:
>> +            memcpy(&tp->func, tpm2_ppi_funcs, sizeof(tpm2_ppi_funcs));
>> +            break;
>> +        }
> Can you elaborate on what this does?  Why is SeaBIOS filling a memory
> addreses created by QEMU?  Why wouldn't QEMU just fill it with what it
> wants directly?

QEMU merely creates the device and the ACPI code. SeaBIOS implements 
which codes the user can send. The above array inside the virtual device 
contains flags that describe the codes that the user can send. If OVMF 
was to implement less codes, less bytes would be set. If another 
firmware implemented the possibility to prevent the user from sending 
certain codes, the TPM_PPI_FUNC_BLOCKED flag could be set for example so 
that ACPI would refuse to set the code for the next reboot. It's 
parametrizing the interface between ACPI and firmware.

    Stefan

> -Kevin
>
>
>> +        break;
>> +    }
>> +
>> +    if (!tp->ppin) {
>> +        tp->ppin = 1;
>> +        tp->pprq = 0;
>> +        tp->lppr = 0;
>> +    }
>> +}
>> +
>> +void
>> +tpm_ppi_process(void)
>> +{
>> +   tpm_ppi_code op;
>> +
>> +   if (tp) {
>> +        op = tp->pprq;
>> +        if (!op) {
>> +            /* intermediate step after a reboot? */
>> +            op = nextStep;
>> +        } else {
>> +            /* last full opcode */
>> +            tp->lppr = op;
>> +        }
>> +        if (op) {
>> +            /*
>> +             * Reset the opcode so we don't permanently reboot upon
>> +             * code 3 (Activate).
>> +             */
>> +            tp->pprq = 0;
>> +
>> +            printf("Processing TPM PPI opcode %d\n", op);
>> +            tpm_process_cfg(op, 0, &tp->pprp);
>> +        }
>> +   }
>> +}
>> diff --git a/src/tcgbios.h b/src/tcgbios.h
>> index 32fb941..52b86f2 100644
>> --- a/src/tcgbios.h
>> +++ b/src/tcgbios.h
>> @@ -16,4 +16,7 @@ void tpm_option_rom(const void *addr, u32 len);
>>   int tpm_can_show_menu(void);
>>   void tpm_menu(void);
>>   
>> +void tpm_ppi_init(void);
>> +void tpm_ppi_process(void);
>> +
>>   #endif /* TCGBIOS_H */
>> -- 
>> 2.5.5
>>


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

On Tue, Jan 16, 2018 at 05:01:51PM -0500, Stefan Berger wrote:
> On 01/16/2018 01:36 PM, Kevin O'Connor wrote:
> > On Tue, Jan 16, 2018 at 11:41:02AM -0500, Stefan Berger wrote:
> > > +    tp = (struct tpm_ppi *)(u32)qemu->tpmppi_address;
> > > +    dprintf(DEBUG_tcg, "TCGBIOS: TPM PPI struct at %p\n", tp);
> > > +
> > > +    memset(&tp->func, 0, sizeof(tp->func));
> > > +    switch (qemu->tpmppi_version) {
> > > +    case TPM_PPI_VERSION_1_30:
> > > +        switch (qemu->tpm_version) {
> > > +        case TPM_VERSION_1_2:
> > > +            memcpy(&tp->func, tpm12_ppi_funcs, sizeof(tpm12_ppi_funcs));
> > > +            break;
> > > +        case TPM_VERSION_2:
> > > +            memcpy(&tp->func, tpm2_ppi_funcs, sizeof(tpm2_ppi_funcs));
> > > +            break;
> > > +        }
> > Can you elaborate on what this does?  Why is SeaBIOS filling a memory
> > addreses created by QEMU?  Why wouldn't QEMU just fill it with what it
> > wants directly?
> 
> QEMU merely creates the device and the ACPI code. SeaBIOS implements which
> codes the user can send. The above array inside the virtual device contains
> flags that describe the codes that the user can send. If OVMF was to
> implement less codes, less bytes would be set. If another firmware
> implemented the possibility to prevent the user from sending certain codes,
> the TPM_PPI_FUNC_BLOCKED flag could be set for example so that ACPI would
> refuse to set the code for the next reboot. It's parametrizing the interface
> between ACPI and firmware.

Is this the right approach?  If QEMU has all the code to emulate the
TPM hardware and QEMU has all the code to build the ACPI tables
describing that hardware - why doesn't it just implement the PPI
functions itself on a reboot?  I'll defer to Laszlo, Igor, Michael,
etc, but this seems like a complex firmware<->qemu interface and I'm
not sure I understand the gain.

The above aside, if there's a need to communicate firmware
capabilities to QEMU (so that QEMU can build the ACPI tables
describing that capability), can we reuse the writable fw_cfg file
system to transfer that information (instead of having the firmware
fill an area of memory)?

-Kevin

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

On 01/16/2018 05:35 PM, Kevin O'Connor wrote:
> On Tue, Jan 16, 2018 at 05:01:51PM -0500, Stefan Berger wrote:
>> On 01/16/2018 01:36 PM, Kevin O'Connor wrote:
>>> On Tue, Jan 16, 2018 at 11:41:02AM -0500, Stefan Berger wrote:
>>>> +    tp = (struct tpm_ppi *)(u32)qemu->tpmppi_address;
>>>> +    dprintf(DEBUG_tcg, "TCGBIOS: TPM PPI struct at %p\n", tp);
>>>> +
>>>> +    memset(&tp->func, 0, sizeof(tp->func));
>>>> +    switch (qemu->tpmppi_version) {
>>>> +    case TPM_PPI_VERSION_1_30:
>>>> +        switch (qemu->tpm_version) {
>>>> +        case TPM_VERSION_1_2:
>>>> +            memcpy(&tp->func, tpm12_ppi_funcs, sizeof(tpm12_ppi_funcs));
>>>> +            break;
>>>> +        case TPM_VERSION_2:
>>>> +            memcpy(&tp->func, tpm2_ppi_funcs, sizeof(tpm2_ppi_funcs));
>>>> +            break;
>>>> +        }
>>> Can you elaborate on what this does?  Why is SeaBIOS filling a memory
>>> addreses created by QEMU?  Why wouldn't QEMU just fill it with what it
>>> wants directly?
>> QEMU merely creates the device and the ACPI code. SeaBIOS implements which
>> codes the user can send. The above array inside the virtual device contains
>> flags that describe the codes that the user can send. If OVMF was to
>> implement less codes, less bytes would be set. If another firmware
>> implemented the possibility to prevent the user from sending certain codes,
>> the TPM_PPI_FUNC_BLOCKED flag could be set for example so that ACPI would
>> refuse to set the code for the next reboot. It's parametrizing the interface
>> between ACPI and firmware.
> Is this the right approach?  If QEMU has all the code to emulate the
> TPM hardware and QEMU has all the code to build the ACPI tables
> describing that hardware - why doesn't it just implement the PPI
> functions itself on a reboot?  I'll defer to Laszlo, Igor, Michael,

QEMU does implement the ACPI code. The firmware may implement a subset 
of what the specs are describing. The array of flags helps to decouple 
the ACPI code and the firmware and allows OVMF and SeaBIOS and possibly 
others to only implement a subset of the functionality.

> etc, but this seems like a complex firmware<->qemu interface and I'm
> not sure I understand the gain.
> The above aside, if there's a need to communicate firmware
> capabilities to QEMU (so that QEMU can build the ACPI tables
> describing that capability), can we reuse the writable fw_cfg file
> system to transfer that information (instead of having the firmware
> fill an area of memory)?

Its SeaBIOS vor OVMF reporting to ACPI what it implements. QEMU doesn't 
know what the firmware implements.

    Stefan

>
> -Kevin
>


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios