On 10/13/24 10:36 PM, dan tan wrote:
Hi Stefan,
Thank you for the review comments! Please see my response below.
thank you,
---
dan tan
power simulation
phone:+1.7373.099.138
email:dan...@linux.ibm.com
On 2024-09-12 13:02, Stefan Berger wrote:
On 9/12/24 12:09 PM, dan tan wrote:
From: dan tan <dan...@linux.ibm.com>
SPI interface to TPM TIS implementation via swtpm
Apart from Cedric's comments:
Can you say a bit more about the specs you followed and details how
many localities are supported etc. Is this device pnv-specific or can
it be used on other platforms as well? If it can then the 'pnv'
prefix/suffix should probably disappear from functions in the test
code because we could probably start an aarch or x86_64 VM then and
call them as well.
Also, an entry in docs/specs/tpm.rst would be great.
This addition to the TPM device support is part of a bigger project
to support an IBM hypervisor stack. In this system, IBM uses Nuvoton
Technology's TPM. Per Nuvoton's documentation, the NPCT7/6/5/4xx devices
implement all TCG commands and functionality, as defined in the following
TCG specifications:
- [TPM2.0] Trusted Platform Module Library Specification, Family “2.0”,
Level 00, Revision 01.59
- Trusted Platform Module Library Specification, Family "2.0", Level 00,
Revision 01.38
- Trusted Platform Module Library Specification, Family “2.0”, Level 00,
Revision 01.16
https://trustedcomputinggroup.org/resource/tpm-library-specification/
The hypervisor only uses locality 0, thus, is the implementation as well
as the qtest.
If the device supports other localities, and I think it does/should,
then those should be tested as well. It should be easy to implement them
since tests already exist from which the SPI tests can be derived.
The interface via SSI SPI is only implemented on the PowerNV platform.
I will add a section in docs/specs/tpm.rst to describe this support
Signed-off-by: dan tan <dan...@linux.ibm.com>
---
include/sysemu/tpm.h | 3 +
hw/tpm/tpm_tis_spi.c | 347 +++++++++++++++++++++++++++++
tests/qtest/pnv-tpm-tis-spi-test.c | 223 ++++++++++++++++++
hw/ppc/Kconfig | 1 +
hw/tpm/Kconfig | 6 +
hw/tpm/meson.build | 1 +
tests/qtest/meson.build | 2 +
7 files changed, 583 insertions(+)
create mode 100644 hw/tpm/tpm_tis_spi.c
create mode 100644 tests/qtest/pnv-tpm-tis-spi-test.c
diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
index 1ee568b3b6..22b05110e2 100644
--- a/include/sysemu/tpm.h
+++ b/include/sysemu/tpm.h
@@ -49,6 +49,7 @@ struct TPMIfClass {
#define TYPE_TPM_CRB "tpm-crb"
#define TYPE_TPM_SPAPR "tpm-spapr"
#define TYPE_TPM_TIS_I2C "tpm-tis-i2c"
+#define TYPE_TPM_TIS_SPI "tpm-tis-spi"
#define TPM_IS_TIS_ISA(chr) \
object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_ISA)
@@ -60,6 +61,8 @@ struct TPMIfClass {
object_dynamic_cast(OBJECT(chr), TYPE_TPM_SPAPR)
#define TPM_IS_TIS_I2C(chr) \
object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_I2C)
+#define TPM_IS_TIS_SPI(chr) \
+ object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SPI)
/* returns NULL unless there is exactly one TPM device */
static inline TPMIf *tpm_find(void)
diff --git a/hw/tpm/tpm_tis_spi.c b/hw/tpm/tpm_tis_spi.c
new file mode 100644
index 0000000000..6e7f42b2db
--- /dev/null
+++ b/hw/tpm/tpm_tis_spi.c
@@ -0,0 +1,347 @@
+/*
+ * QEMU PowerPC SPI TPM 2.0 model
+ *
+ * Copyright (c) 2024, IBM Corporation.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "hw/sysbus.h"
+#include "hw/pci/pci_ids.h"
+#include "hw/acpi/tpm.h"
+#include "tpm_prop.h"
+#include "qemu/log.h"
+#include "tpm_tis.h"
+#include "hw/ssi/ssi.h"
+
+typedef struct TPMStateSPI {
+ /*< private >*/
+ SSIPeripheral parent_object;
+
+ uint8_t offset; /* offset into data[] */
+ uint8_t spi_state; /* READ / WRITE / IDLE */
+#define SPI_STATE_IDLE 0
+#define SPI_STATE_WRITE 1
+#define SPI_STATE_READ 2
+
+ bool command;
+
+ uint8_t loc_sel; /* Current locality */
+ uint32_t tis_addr; /* tis address including locty */
+
+ /*< public >*/
+ TPMState tpm_state; /* not a QOM object */
+
+} TPMStateSPI;
+
+typedef struct xfer_buffer xfer_buffer;
+
+#ifdef SPI_DEBUG_ENABLED
+#define SPI_DEBUG(x) (x)
+#else
+#define SPI_DEBUG(x)
+#endif
+
+DECLARE_INSTANCE_CHECKER(TPMStateSPI, TPM_TIS_SPI, TYPE_TPM_TIS_SPI)
+
+static inline void tpm_tis_spi_clear_data(TPMStateSPI *spist)
+{
+ spist->spi_state = 0;
+ spist->offset = 0;
+ spist->tis_addr = 0xffffffff;
+
+ return;
+}
+
+/* Callback from TPM to indicate that response is copied */
+static void tpm_tis_spi_request_completed(TPMIf *ti, int ret)
+{
+ TPMStateSPI *spist = TPM_TIS_SPI(ti);
+ TPMState *s = &spist->tpm_state;
+
+ /* Inform the common code. */
+ tpm_tis_request_completed(s, ret);
+}
+
+static enum TPMVersion tpm_tis_spi_get_tpm_version(TPMIf *ti)
+{
+ TPMStateSPI *spist = TPM_TIS_SPI(ti);
+ TPMState *s = &spist->tpm_state;
+
+ return tpm_tis_get_tpm_version(s);
+}
+
+/*
+ * TCG PC Client Platform TPM Profile Specification for TPM 2.0 ver
1.05 rev 14
+ *
+ * For system Software, the TPM has a 64-bit address of
0x0000_0000_FED4_xxxx.
+ * On SPI, the chipset passes the least significant 24 bits to the TPM.
+ * The upper bytes will be used by the chipset to select the TPM’s
SPI CS#
+ * signal. Table 9 shows the locality based on the 16 least
significant address
+ * bits and assume that either the LPC TPM sync or SPI TPM CS# is used.
+ *
+ */
+static void tpm_tis_spi_write(TPMStateSPI *spist, uint32_t addr,
uint8_t val)
+{
+ SPI_DEBUG(qemu_log("tpm_tis_spi_write addr:0x%8.8x, value:%2.2x\n",
+ addr, val));
+ TPMState *tpm_st = &spist->tpm_state;
First variable declation then empty line and then the SPI_DEBUG.
Not sure how this happened, good catch! I will correct it.
+ tpm_tis_write_data(tpm_st, addr, val, 1);
+}
+
+static uint8_t tpm_tis_spi_read(TPMStateSPI *spist, uint32_t addr)
+{
+ uint16_t offset = addr & 0xffc;
+ TPMState *tpm_st = &spist->tpm_state;
+ uint8_t data;
+ uint32_t did_vid;
+
+ SPI_DEBUG(qemu_log("tpm_tis_spi_read addr:0x%8.8x .... ", addr));
+ if (offset == TPM_TIS_REG_DID_VID) {
+ did_vid = (TPM_TIS_TPM_DID << 16) | TPM_TIS_TPM_VID;
+ data = (did_vid >> ((addr & 0x3) * 8)) & 0xff;
Why does this register need special handling? This function could just
be a 'return tpm_tis_read_data(tpm_st, addr, 1);' at this point, no?
Yes, you are right!
This is the code segment leftover from the hypervisor specific support.
Although it is an IBM Power platform, the TPM does have a different
vendor ID and device ID than IBM's. If we didn't intercept this request,
tpm_tis_read_data() would return IBM's vendor ID, and the validation in
the hypervisor would fail.
I will remove the _if_ block and verify the results.
+ } else {
+ data = tpm_tis_read_data(tpm_st, addr, 1);
+ }
+
+ return data;
+}
+
+static Property tpm_tis_spi_properties[] = {
+ DEFINE_PROP_TPMBE("tpmdev", TPMStateSPI, tpm_state.be_driver),
+ DEFINE_PROP_UINT32("irq", TPMStateSPI, tpm_state.irq_num, 0),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static void tpm_tis_spi_reset(DeviceState *dev)
+{
+ TPMStateSPI *spist = TPM_TIS_SPI(dev);
+ TPMState *s = &spist->tpm_state;
+
+ tpm_tis_spi_clear_data(spist);
+
+ spist->loc_sel = 0x00;
+
+ return tpm_tis_reset(s);
+}
+
+static uint32_t tpm_transfer(SSIPeripheral *ss, uint32_t tx)
+{
+ uint32_t rx = 0;
+ /* static variables are automatically initialized to zero */
+ static uint8_t byte_offset; /* byte offset in transfer */
+ static uint8_t wait_state_count; /* wait state counter */
+ static uint8_t xfer_size; /* data size of transfer */
+ static uint32_t reg_addr; /* register address of
transfer */
Shouldn't these statics be part of the TPMStateSPI so that they can be
saved if the VM was to suspend in the middle of a transfer. If this
cannot happen because you will always get all bytes passed to this
function then why are these static's?
You are absolutely right! I will move them to the TPMStateSPI struct.
+
+ TPMStateSPI *spist = TPM_TIS_SPI(ss);
+
+ uint8_t byte; /* reversed byte value */
+ uint8_t offset = 0; /* offset of byte in payload */
+ uint8_t index; /* index of data byte in transfer */
+
+ SPI_DEBUG(qemu_log("TPM SPI request from controller\n"));
+
+ /* new transfer or not */
+ if (spist->command) { /* new transfer start */
+ if (spist->spi_state != SPI_STATE_IDLE) {
+ qemu_log_mask(LOG_GUEST_ERROR, "unexpected new
transfer\n");
+ }
+ byte_offset = 0;
+ wait_state_count = 0;
+ }
+ /*
+ * Explanation of wait_state:
+ * The original TPM model did not have wait state or "flow
control" support
+ * built in. If you wanted to read a TPM register through SPI
you sent
+ * the first byte with the read/write bit and size, then three
address bytes
+ * and any additional bytes after that were don't care bytes for
reads and
+ * the model would begin returning byte data to the SPI reader
from the
+ * register address provided. In the real world this would mean
that a
+ * TPM device had only the time between the 31st clock and the
32nd clock
+ * to fetch the register data that it had to provide to SPI MISO
starting
+ * with the 32nd clock.
+ *
+ * In reality the TPM begins introducing a wait state at the
31st clock
+ * by holding MISO low. This is how it controls the "flow" of the
+ * operation. Once the data the TPM needs to return is ready it
will
+ * select bit 31 + (8*N) to send back a 1 which indicates that
it will
+ * now start returning data on MISO.
+ *
+ * The same wait states are applied to writes. In either the
read or write
+ * case the wait state occurs between the command+address (4
bytes) and the
+ * data (1-n bytes) sections of the SPI frame. The code below
introduces
+ * the support for a 32 bit wait state for P10. All reads and
writes
+ * through the SPI interface MUST now be aware of the need to do
flow
+ * control in order to use the TPM via SPI.
+ *
+ * In conjunction with these changes there were changes made to
the SPIM
+ * engine that was introduced in P10 to support the 6x op code
which is
+ * used to receive wait state 0s on the MISO line until it sees
the b'1'
+ * come back before continuing to read real data from the SPI
device(TPM).
+ */
+
+ SPI_DEBUG(qemu_log("Processing new payload current
byte_offset=%d\n",
+ byte_offset));
+ /* process payload data */
+ while (offset < 4) {
+ spist->command = false;
+ byte = (tx >> (24 - 8 * offset)) & 0xFF;
+ SPI_DEBUG(qemu_log("Extracted byte=0x%2.2x from payload
offset=%d\n",
+ byte, offset));
+ switch (byte_offset) {
+ case 0: /* command byte */
+ if ((byte >> 7) == 0) { /* bit-7 */
I think checking for bit 7 would be better like this:
#define CMD_BYTE_WRITE (1 << 7)
if ((byte & CMD_BYTE_WRITE) == 0)
Yes, that does look more readable. I will make the change.
+ spist->spi_state = SPI_STATE_WRITE;
+ SPI_DEBUG(qemu_log("spi write\n"));
+ } else {
+ spist->spi_state = SPI_STATE_READ;
+ SPI_DEBUG(qemu_log("spi read\n"));
+ }
# define CMD_BYTE_XFER_SZ_MASK 0x1f
xfter_size = (byte & CMD_BYTE_XFER_SZ_MASK)
ditto!
+ xfer_size = (byte & 0x1f) + 1; /* bits 5:0 */
+ SPI_DEBUG(qemu_log("xfer_size=%d\n", xfer_size));
+ break;
+ case 1: /* 1st address byte */
This is the from the TIS address 0fe >d4< 00 00.
Correct. I will add a comment
+ if (byte != 0xd4) {
+ qemu_log_mask(LOG_GUEST_ERROR, "incorrect high
address 0x%x\n",
+ byte);
+ }
+ reg_addr = byte << 16;
+ SPI_DEBUG(qemu_log("first addr byte=0x%x, reg_addr now
0x%8.8x\n",
+ byte, reg_addr));
+ break;
+ case 2: /* 2nd address byte */
+ reg_addr |= byte << 8;
+ SPI_DEBUG(qemu_log("second addr byte=0x%x, reg_addr now
0x%8.8x\n",
+ byte, reg_addr));
+ break;
+ case 3: /* 3rd address byte */
+ reg_addr |= byte;
+ SPI_DEBUG(qemu_log("third addr byte=0x%x, reg_addr now
0x%8.8x\n",
+ byte, reg_addr));
+ break;
+ default: /* data bytes */
+ if (wait_state_count < 4) {
+ wait_state_count++;
+ if (wait_state_count == 4) {
+ SPI_DEBUG(qemu_log("wait complete,
wait_state_count=0x%x\n",
+ wait_state_count));
+ rx = rx | (0x01 << (24 - offset * 8));
+ return rx;
+ } else {
+ SPI_DEBUG(qemu_log("in wait state,
wait_state_count=0x%x\n",
+ wait_state_count));
+ rx = 0;
+ }
+ } else {
+ index = byte_offset - 4;
+ SPI_DEBUG(qemu_log("data byte=0x%x for index=%d, "
+ "reg_addr now 0x%8.8x\n",
+ byte, index, reg_addr));
+
+ if (index >= xfer_size) {
+ /*
+ * SPI SSI framework limits both rx and tx
+ * to fixed 4-byte with each xfer
Per the setting of xfer_size above it can be up to 32 bytes?
This comment is made by the SPI SSI implementer. I believe this
is the SSI framework statement. The SPI max transfer size is much
smaller
+ */
+ SPI_DEBUG(qemu_log("data exceeds expected amount
%u\n",
+ xfer_size));
+ return rx;
+ }
+ spist->tis_addr = reg_addr + (index % 4);
+ if (spist->spi_state == SPI_STATE_WRITE) {
+ tpm_tis_spi_write(spist, spist->tis_addr, byte);
+ } else {
+ byte = tpm_tis_spi_read(spist, spist->tis_addr);
+ rx = rx | (byte << (24 - offset * 8));
+ SPI_DEBUG(qemu_log("added byte=0x%2.2x to
response payload"
+ " at offset=%d\n", byte, offset));
+ }
+ }
+ break;
+ }
+ if ((wait_state_count == 0) || (wait_state_count == 4)) {
Is there not a more obvious relationship between having to increase
offset and byte_offset other than making this depend on the
wait_states? Like make it depend on the byte_offset.
A wait state in a Serial Peripheral Interface (SPI) occurs when the main
must wait before issuing clock cycles to allow for a required waiting
period.
It does not have direct relationships with the byte_offset. With real
hardware,
a good example would be that if an analog-to-digital conversion is
required,
the main must wait for the conversion to complete before issuing clock
cycles.
/*
* advance offsets while handling command and address bytes and once
* 4 wait states have been reached.
*/
if (byte_offset <= 3 || wait_state_count == 4)
+ offset++;
+ byte_offset++;
+ } else {
+ break;
+ }
+ }
+ return rx;
+}
+
+static int tpm_cs(SSIPeripheral *ss, bool select)
+{
+ TPMStateSPI *spist = TPM_TIS_SPI(ss);
add empty line
yes, sir.
use ' ./scripts/checkpatch.pl *.patch ' to check your patches.
+ if (select) {
+ spist->command = false;
+ spist->spi_state = SPI_STATE_IDLE;
+ } else {
+ spist->command = true;
+ }
+ return 0;
+}
+
+
one too many empty lines
+static void tpm_realize(SSIPeripheral *dev, Error **errp)
+{
+ TPMStateSPI *spist = TPM_TIS_SPI(dev);
+ TPMState *s = &spist->tpm_state;
+
+ spist->command = true;
+ spist->spi_state = SPI_STATE_IDLE;
+
+ if (!tpm_find()) {
+ error_setg(errp, "at most one TPM device is permitted");
+ return;
+ }
+
+ s->be_driver = qemu_find_tpm_be("tpm0");
+
+ if (!s->be_driver) {
+ error_setg(errp, "unable to find tpm backend device");
+ return;
+ }
+}
+
+static void tpm_tis_spi_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ TPMIfClass *tc = TPM_IF_CLASS(klass);
+ SSIPeripheralClass *k = SSI_PERIPHERAL_CLASS(klass);
+
+ k->transfer = tpm_transfer;
+ k->realize = tpm_realize;
+ k->set_cs = tpm_cs;
+ k->cs_polarity = SSI_CS_LOW;
+
+ dc->reset = tpm_tis_spi_reset;
+ device_class_set_props(dc, tpm_tis_spi_properties);
+ set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+
+ dc->desc = "PowerNV SPI TPM";
Is it really a PowerNV specific device?
At current time, it is, as it relies on PNV_SPI
Though it follows a non-Power specific spec, so IMO it should be "SPI TPM"
+
+ tc->model = TPM_MODEL_TPM_TIS;
+ tc->request_completed = tpm_tis_spi_request_completed;
+ tc->get_version = tpm_tis_spi_get_tpm_version;
+}
+
+static const TypeInfo tpm_tis_spi_info = {
+ .name = TYPE_TPM_TIS_SPI,
+ .parent = TYPE_SSI_PERIPHERAL,
+ .instance_size = sizeof(TPMStateSPI),
+ .class_init = tpm_tis_spi_class_init,
+ .interfaces = (InterfaceInfo[]) {
+ { TYPE_TPM_IF },
+ { }
+ }
+};
+
+static void tpm_tis_spi_register_types(void)
+{
+ type_register_static(&tpm_tis_spi_info);
+}
+
+type_init(tpm_tis_spi_register_types)
diff --git a/tests/qtest/pnv-tpm-tis-spi-test.c
b/tests/qtest/pnv-tpm-tis-spi-test.c
new file mode 100644
index 0000000000..395c944044
--- /dev/null
+++ b/tests/qtest/pnv-tpm-tis-spi-test.c
Call it tpm-tis-spi-pnv-test.c ? with functions in namespace
tpm_tis_spi_pnv_... If this device is not PNV specific you should
probably drop pnv from the name.
It is PNV specific. It is only implemented and runs on PowerNV platform.
Given that, should I keep the pnv-tpm-tis-spi-test.c name, or change it
to tpm-tis-spi-pnv-test.c?
The latter is probaby better.
