[RFC 0/1] SPI support in QEMU TPM

[RFC 0/1] SPI support in QEMU TPM

[Iris Chen]

From: Iris Chen <irischenlj@fb.com>

Hey everyone,

Thanks for all your comments on the SPI GPIO model. I am working through them.

As for adding support for SPI-based TPMs in QEMU, this RFC patch adds SPI
support in the QEMU TPM implementation via tpm_tis_spi.c.

The QEMU tree already has support for two connection methods to the TPM:
mmio (isa for x86, sysbus for arm) and “spapr”. This patch adds a new SPI
bus implementation for the TPM. Specifically, this SPI bus implementation
connects to the Yosemite-v3.5 model using the SPI-GPIO model sent earlier
last week. I have already tested these implementations locally together
and can verify that the Linux kernel can successfully probe the TPM device
on Yosemite-v3.5 and we can run the TPM command line tools to interact with it.

Please let me know what you think about this!

Thanks,
Iris

Iris Chen (1):
  hw: tpmtisspi: add SPI support to QEMU TPM implementation

 configs/devices/arm-softmmu/default.mak |   1 +
 hw/arm/Kconfig                          |   5 +
 hw/tpm/Kconfig                          |   5 +
 hw/tpm/meson.build                      |   1 +
 hw/tpm/tpm_tis_spi.c                    | 311 ++++++++++++++++++++++++
 include/sysemu/tpm.h                    |   3 +
 6 files changed, 326 insertions(+)
 create mode 100644 hw/tpm/tpm_tis_spi.c

--
2.30.2

[RFC 1/1] hw: tpmtisspi: add SPI support to QEMU TPM implementation

[Iris Chen]

From: Iris Chen <irischenlj@fb.com>

Signed-off-by: Iris Chen <irischenlj@fb.com>
---
 configs/devices/arm-softmmu/default.mak |   1 +
 hw/arm/Kconfig                          |   5 +
 hw/tpm/Kconfig                          |   5 +
 hw/tpm/meson.build                      |   1 +
 hw/tpm/tpm_tis_spi.c                    | 311 ++++++++++++++++++++++++
 include/sysemu/tpm.h                    |   3 +
 6 files changed, 326 insertions(+)
 create mode 100644 hw/tpm/tpm_tis_spi.c

diff --git a/configs/devices/arm-softmmu/default.mak b/configs/devices/arm-softmmu/default.mak
index 6985a25377..80d2841568 100644
--- a/configs/devices/arm-softmmu/default.mak
+++ b/configs/devices/arm-softmmu/default.mak
@@ -42,3 +42,4 @@ CONFIG_FSL_IMX6UL=y
 CONFIG_SEMIHOSTING=y
 CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y
 CONFIG_ALLWINNER_H3=y
+CONFIG_FBOBMC_AST=y
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 15fa79afd3..193decaec1 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -458,6 +458,11 @@ config ASPEED_SOC
     select PMBUS
     select MAX31785
 
+config FBOBMC_AST
+    bool
+    select ASPEED_SOC
+    select TPM_TIS_SPI
+
 config MPS2
     bool
     imply I2C_DEVICES
diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
index 29e82f3c92..370a43f045 100644
--- a/hw/tpm/Kconfig
+++ b/hw/tpm/Kconfig
@@ -8,6 +8,11 @@ config TPM_TIS_SYSBUS
     depends on TPM
     select TPM_TIS
 
+config TPM_TIS_SPI
+    bool
+    depends on TPM
+    select TPM_TIS
+
 config TPM_TIS
     bool
     depends on TPM
diff --git a/hw/tpm/meson.build b/hw/tpm/meson.build
index 1c68d81d6a..1a057f4e36 100644
--- a/hw/tpm/meson.build
+++ b/hw/tpm/meson.build
@@ -2,6 +2,7 @@ softmmu_ss.add(when: 'CONFIG_TPM_TIS', if_true: files('tpm_tis_common.c'))
 softmmu_ss.add(when: 'CONFIG_TPM_TIS_ISA', if_true: files('tpm_tis_isa.c'))
 softmmu_ss.add(when: 'CONFIG_TPM_TIS_SYSBUS', if_true: files('tpm_tis_sysbus.c'))
 softmmu_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_crb.c'))
+softmmu_ss.add(when: 'CONFIG_TPM_TIS_SPI', if_true: files('tpm_tis_spi.c'))
 
 specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_TIS'], if_true: files('tpm_ppi.c'))
 specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_CRB'], if_true: files('tpm_ppi.c'))
diff --git a/hw/tpm/tpm_tis_spi.c b/hw/tpm/tpm_tis_spi.c
new file mode 100644
index 0000000000..c98ddcfddb
--- /dev/null
+++ b/hw/tpm/tpm_tis_spi.c
@@ -0,0 +1,311 @@
+#include "qemu/osdep.h"
+#include "hw/qdev-properties.h"
+#include "migration/vmstate.h"
+#include "hw/acpi/tpm.h"
+#include "tpm_prop.h"
+#include "tpm_tis.h"
+#include "qom/object.h"
+#include "hw/ssi/ssi.h"
+#include "hw/ssi/spi_gpio.h"
+
+#define TPM_TIS_SPI_ADDR_BYTES 3
+#define SPI_WRITE 0
+
+typedef enum {
+    TIS_SPI_PKT_STATE_DEACTIVATED = 0,
+    TIS_SPI_PKT_STATE_START,
+    TIS_SPI_PKT_STATE_ADDRESS,
+    TIS_SPI_PKT_STATE_DATA_WR,
+    TIS_SPI_PKT_STATE_DATA_RD,
+    TIS_SPI_PKT_STATE_DONE,
+} TpmTisSpiPktState;
+
+union TpmTisRWSizeByte {
+    uint8_t byte;
+    struct {
+        uint8_t data_expected_size:6;
+        uint8_t resv:1;
+        uint8_t rwflag:1;
+    };
+};
+
+union TpmTisSpiHwAddr {
+    hwaddr addr;
+    uint8_t bytes[sizeof(hwaddr)];
+};
+
+union TpmTisSpiData {
+    uint32_t data;
+    uint8_t bytes[64];
+};
+
+struct TpmTisSpiState {
+    /*< private >*/
+    SSIPeripheral parent_obj;
+
+    /*< public >*/
+    TPMState tpm_state; /* not a QOM object */
+    TpmTisSpiPktState tpm_tis_spi_state;
+
+    union TpmTisRWSizeByte first_byte;
+    union TpmTisSpiHwAddr addr;
+    union TpmTisSpiData data;
+
+    uint32_t data_size;
+    uint8_t data_idx;
+    uint8_t addr_idx;
+};
+
+struct TpmTisSpiClass {
+    SSIPeripheralClass parent_class;
+};
+
+OBJECT_DECLARE_TYPE(TpmTisSpiState, TpmTisSpiClass, TPM_TIS_SPI)
+
+static void tpm_tis_spi_mmio_read(TpmTisSpiState *tts)
+{
+    uint16_t offset = tts->addr.addr & 0xffc;
+
+    switch (offset) {
+    case TPM_TIS_REG_DATA_FIFO:
+        for (uint8_t i = 0; i < tts->data_size; i++) {
+            tts->data.bytes[i] = (uint8_t)tpm_tis_memory_ops.read(
+                                          &tts->tpm_state,
+                                          tts->addr.addr,
+                                          1);
+        }
+        break;
+    default:
+        tts->data.data = (uint32_t)tpm_tis_memory_ops.read(
+                                   &tts->tpm_state,
+                                   tts->addr.addr,
+                                   tts->data_size);
+    }
+}
+
+static void tpm_tis_spi_mmio_write(TpmTisSpiState *tts)
+{
+    uint16_t offset = tts->addr.addr & 0xffc;
+
+    switch (offset) {
+    case TPM_TIS_REG_DATA_FIFO:
+        for (uint8_t i = 0; i < tts->data_size; i++) {
+            tpm_tis_memory_ops.write(&tts->tpm_state,
+                                     tts->addr.addr,
+                                     tts->data.bytes[i],
+                                     1);
+        }
+        break;
+    default:
+        tpm_tis_memory_ops.write(&tts->tpm_state,
+                                 tts->addr.addr,
+                                 tts->data.data,
+                                 tts->data_size);
+        }
+}
+
+static uint32_t tpm_tis_spi_transfer8(SSIPeripheral *ss, uint32_t tx)
+{
+    TpmTisSpiState *tts = TPM_TIS_SPI(ss);
+    uint32_t r = 1;
+
+    switch (tts->tpm_tis_spi_state) {
+    case TIS_SPI_PKT_STATE_START:
+        tts->first_byte.byte = (uint8_t)tx;
+        tts->data_size = tts->first_byte.data_expected_size + 1;
+        tts->data_idx = 0;
+        tts->addr_idx = TPM_TIS_SPI_ADDR_BYTES;
+        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_ADDRESS;
+        break;
+    case TIS_SPI_PKT_STATE_ADDRESS:
+        assert(tts->addr_idx > 0);
+        tts->addr.bytes[--tts->addr_idx] = (uint8_t)tx;
+
+        if (tts->addr_idx == 0) {
+            if (tts->first_byte.rwflag == SPI_WRITE) {
+                tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DATA_WR;
+            } else { /* read and get the data ready */
+                tpm_tis_spi_mmio_read(tts);
+                tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DATA_RD;
+            }
+        }
+        break;
+    case TIS_SPI_PKT_STATE_DATA_WR:
+        tts->data.bytes[tts->data_idx++] = (uint8_t)tx;
+        if (tts->data_idx == tts->data_size) {
+            tpm_tis_spi_mmio_write(tts);
+            tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DONE;
+        }
+        break;
+    case TIS_SPI_PKT_STATE_DATA_RD:
+        r = tts->data.bytes[tts->data_idx++];
+        if (tts->data_idx == tts->data_size) {
+            tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DONE;
+        }
+        break;
+    default:
+        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DEACTIVATED;
+        r = (uint32_t) -1;
+    }
+
+    return r;
+}
+
+/*
+ * Pre-reading logic for transfer:
+ * This is to fix the transaction between reading and writing.
+ * The first byte is arbitrarily inserted so we need to
+ * shift the all the output bytes (timeline) one byte right.
+ */
+static uint32_t tpm_tis_spi_transfer8_ex(SSIPeripheral *ss, uint32_t tx)
+{
+    TpmTisSpiState *tts = TPM_TIS_SPI(ss);
+    SSIBus *ssibus = (SSIBus *)qdev_get_parent_bus(DEVICE(tts));
+
+    TpmTisSpiPktState prev_state = tts->tpm_tis_spi_state;
+    uint32_t r = tpm_tis_spi_transfer8(ss, tx);
+    TpmTisSpiPktState curr_state = tts->tpm_tis_spi_state;
+
+    if (ssibus->preread &&
+       /* cmd state has changed into DATA reading state */
+       prev_state != TIS_SPI_PKT_STATE_DATA_RD &&
+       curr_state == TIS_SPI_PKT_STATE_DATA_RD) {
+        r = tpm_tis_spi_transfer8(ss, 0xFF);
+    }
+
+    return r;
+}
+
+static int tpm_tis_spi_cs(SSIPeripheral *ss, bool select)
+{
+    TpmTisSpiState *tts = TPM_TIS_SPI(ss);
+
+    if (select) { /* cs de-assert */
+        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DEACTIVATED;
+    } else {
+        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_START;
+        tts->first_byte.byte = 0;
+        tts->addr.addr = 0;
+        tts->data.data = 0;
+    }
+
+    return 0;
+}
+
+static int tpm_tis_pre_save_spi(void *opaque)
+{
+    TpmTisSpiState *sbdev = opaque;
+
+    return tpm_tis_pre_save(&sbdev->tpm_state);
+}
+
+static const VMStateDescription vmstate_tpm_tis_spi = {
+    .name = "tpm-tis-spi",
+    .version_id = 0,
+    .pre_save  = tpm_tis_pre_save_spi,
+    .fields = (VMStateField[]) {
+        VMSTATE_BUFFER(tpm_state.buffer, TpmTisSpiState),
+        VMSTATE_UINT16(tpm_state.rw_offset, TpmTisSpiState),
+        VMSTATE_UINT8(tpm_state.active_locty, TpmTisSpiState),
+        VMSTATE_UINT8(tpm_state.aborting_locty, TpmTisSpiState),
+        VMSTATE_UINT8(tpm_state.next_locty, TpmTisSpiState),
+
+        VMSTATE_STRUCT_ARRAY(tpm_state.loc, TpmTisSpiState, TPM_TIS_NUM_LOCALITIES,
+                             0, vmstate_locty, TPMLocality),
+
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void tpm_tis_spi_request_completed(TPMIf *ti, int ret)
+{
+    TpmTisSpiState *sbdev = TPM_TIS_SPI(ti);
+    TPMState *s = &sbdev->tpm_state;
+
+    tpm_tis_request_completed(s, ret);
+}
+
+static enum TPMVersion tpm_tis_spi_get_tpm_version(TPMIf *ti)
+{
+    TpmTisSpiState *sbdev = TPM_TIS_SPI(ti);
+    TPMState *s = &sbdev->tpm_state;
+
+    return tpm_tis_get_tpm_version(s);
+}
+
+static void tpm_tis_spi_reset(DeviceState *dev)
+{
+    TpmTisSpiState *sbdev = TPM_TIS_SPI(dev);
+    TPMState *s = &sbdev->tpm_state;
+
+    return tpm_tis_reset(s);
+}
+
+static Property tpm_tis_spi_properties[] = {
+    DEFINE_PROP_UINT32("irq", TpmTisSpiState, tpm_state.irq_num, TPM_TIS_IRQ),
+    DEFINE_PROP_TPMBE("tpmdev", TpmTisSpiState, tpm_state.be_driver),
+    DEFINE_PROP_BOOL("ppi", TpmTisSpiState, tpm_state.ppi_enabled, false),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void tpm_tis_spi_realizefn(SSIPeripheral *ss, Error **errp)
+{
+    TpmTisSpiState *sbdev = TPM_TIS_SPI(ss);
+
+    if (!tpm_find()) {
+        error_setg(errp, "at most one TPM device is permitted");
+        return;
+    }
+
+    if (!sbdev->tpm_state.be_driver) {
+        error_setg(errp, "'tpmdev' property is required");
+        return;
+    }
+
+    DeviceState *spi_gpio = qdev_find_recursive(sysbus_get_default(),
+                                                TYPE_SPI_GPIO);
+    qdev_connect_gpio_out_named(spi_gpio,
+                                "SPI_CS_out", 0,
+                                qdev_get_gpio_in_named(DEVICE(ss),
+                                SSI_GPIO_CS, 0));
+}
+
+static void tpm_tis_spi_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    SSIPeripheralClass *k = SSI_PERIPHERAL_CLASS(klass);
+    TPMIfClass *tc = TPM_IF_CLASS(klass);
+
+    device_class_set_props(dc, tpm_tis_spi_properties);
+    k->realize = tpm_tis_spi_realizefn;
+    k->transfer = tpm_tis_spi_transfer8_ex;
+    k->set_cs = tpm_tis_spi_cs;
+    k->cs_polarity = SSI_CS_LOW;
+
+    dc->vmsd  = &vmstate_tpm_tis_spi;
+    tc->model = TPM_MODEL_TPM_TIS;
+    dc->user_creatable = true;
+    dc->reset = tpm_tis_spi_reset;
+    tc->request_completed = tpm_tis_spi_request_completed;
+    tc->get_version = tpm_tis_spi_get_tpm_version;
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+}
+
+static const TypeInfo tpm_tis_spi_info = {
+    .name = TYPE_TPM_TIS_SPI,
+    .parent = TYPE_SSI_PERIPHERAL,
+    .instance_size = sizeof(TpmTisSpiState),
+    .class_size = sizeof(TpmTisSpiClass),
+    .class_init  = tpm_tis_spi_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_TPM_IF },
+        { }
+    }
+};
+
+static void tpm_tis_spi_register(void)
+{
+    type_register_static(&tpm_tis_spi_info);
+}
+
+type_init(tpm_tis_spi_register)
diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
index fb40e30ff6..6a6b311e47 100644
--- a/include/sysemu/tpm.h
+++ b/include/sysemu/tpm.h
@@ -46,6 +46,7 @@ struct TPMIfClass {
 
 #define TYPE_TPM_TIS_ISA            "tpm-tis"
 #define TYPE_TPM_TIS_SYSBUS         "tpm-tis-device"
+#define TYPE_TPM_TIS_SPI            "tpm-tis-spi-device"
 #define TYPE_TPM_CRB                "tpm-crb"
 #define TYPE_TPM_SPAPR              "tpm-spapr"
 
@@ -53,6 +54,8 @@ struct TPMIfClass {
     object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_ISA)
 #define TPM_IS_TIS_SYSBUS(chr)                      \
     object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SYSBUS)
+#define TPM_IS_TIS_SPI(chr)                      \
+    object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SPI)
 #define TPM_IS_CRB(chr)                             \
     object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB)
 #define TPM_IS_SPAPR(chr)                           \
-- 
2.30.2

Re: [RFC 1/1] hw: tpmtisspi: add SPI support to QEMU TPM implementation

[Cédric Le Goater]

On 8/3/22 04:32, Iris Chen wrote:
> From: Iris Chen <irischenlj@fb.com>

A commit log telling us about this new device would be good to have.


> Signed-off-by: Iris Chen <irischenlj@fb.com>
> ---
>   configs/devices/arm-softmmu/default.mak |   1 +
>   hw/arm/Kconfig                          |   5 +
>   hw/tpm/Kconfig                          |   5 +
>   hw/tpm/meson.build                      |   1 +
>   hw/tpm/tpm_tis_spi.c                    | 311 ++++++++++++++++++++++++
>   include/sysemu/tpm.h                    |   3 +
>   6 files changed, 326 insertions(+)
>   create mode 100644 hw/tpm/tpm_tis_spi.c
> 
> diff --git a/configs/devices/arm-softmmu/default.mak b/configs/devices/arm-softmmu/default.mak
> index 6985a25377..80d2841568 100644
> --- a/configs/devices/arm-softmmu/default.mak
> +++ b/configs/devices/arm-softmmu/default.mak
> @@ -42,3 +42,4 @@ CONFIG_FSL_IMX6UL=y
>   CONFIG_SEMIHOSTING=y
>   CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y
>   CONFIG_ALLWINNER_H3=y
> +CONFIG_FBOBMC_AST=y

I don't think this extra config is useful for now

> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 15fa79afd3..193decaec1 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -458,6 +458,11 @@ config ASPEED_SOC
>       select PMBUS
>       select MAX31785
>   
> +config FBOBMC_AST
> +    bool
> +    select ASPEED_SOC
> +    select TPM_TIS_SPI
> +
>   config MPS2
>       bool
>       imply I2C_DEVICES
> diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
> index 29e82f3c92..370a43f045 100644
> --- a/hw/tpm/Kconfig
> +++ b/hw/tpm/Kconfig
> @@ -8,6 +8,11 @@ config TPM_TIS_SYSBUS
>       depends on TPM
>       select TPM_TIS
>   
> +config TPM_TIS_SPI
> +    bool
> +    depends on TPM
> +    select TPM_TIS
> +
>   config TPM_TIS
>       bool
>       depends on TPM
> diff --git a/hw/tpm/meson.build b/hw/tpm/meson.build
> index 1c68d81d6a..1a057f4e36 100644
> --- a/hw/tpm/meson.build
> +++ b/hw/tpm/meson.build
> @@ -2,6 +2,7 @@ softmmu_ss.add(when: 'CONFIG_TPM_TIS', if_true: files('tpm_tis_common.c'))
>   softmmu_ss.add(when: 'CONFIG_TPM_TIS_ISA', if_true: files('tpm_tis_isa.c'))
>   softmmu_ss.add(when: 'CONFIG_TPM_TIS_SYSBUS', if_true: files('tpm_tis_sysbus.c'))
>   softmmu_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_crb.c'))
> +softmmu_ss.add(when: 'CONFIG_TPM_TIS_SPI', if_true: files('tpm_tis_spi.c'))
>   
>   specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_TIS'], if_true: files('tpm_ppi.c'))
>   specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_CRB'], if_true: files('tpm_ppi.c'))
> diff --git a/hw/tpm/tpm_tis_spi.c b/hw/tpm/tpm_tis_spi.c
> new file mode 100644
> index 0000000000..c98ddcfddb
> --- /dev/null
> +++ b/hw/tpm/tpm_tis_spi.c
> @@ -0,0 +1,311 @@
> +#include "qemu/osdep.h"
> +#include "hw/qdev-properties.h"
> +#include "migration/vmstate.h"
> +#include "hw/acpi/tpm.h"
> +#include "tpm_prop.h"
> +#include "tpm_tis.h"
> +#include "qom/object.h"
> +#include "hw/ssi/ssi.h"
> +#include "hw/ssi/spi_gpio.h"
> +
> +#define TPM_TIS_SPI_ADDR_BYTES 3
> +#define SPI_WRITE 0
> +
> +typedef enum {
> +    TIS_SPI_PKT_STATE_DEACTIVATED = 0,
> +    TIS_SPI_PKT_STATE_START,
> +    TIS_SPI_PKT_STATE_ADDRESS,
> +    TIS_SPI_PKT_STATE_DATA_WR,
> +    TIS_SPI_PKT_STATE_DATA_RD,
> +    TIS_SPI_PKT_STATE_DONE,
> +} TpmTisSpiPktState;
> +
> +union TpmTisRWSizeByte {
> +    uint8_t byte;
> +    struct {
> +        uint8_t data_expected_size:6;
> +        uint8_t resv:1;
> +        uint8_t rwflag:1;
> +    };
> +};
> +
> +union TpmTisSpiHwAddr {
> +    hwaddr addr;
> +    uint8_t bytes[sizeof(hwaddr)];
> +};
> +
> +union TpmTisSpiData {
> +    uint32_t data;
> +    uint8_t bytes[64];
> +};
> +
> +struct TpmTisSpiState {
> +    /*< private >*/
> +    SSIPeripheral parent_obj;
> +
> +    /*< public >*/
> +    TPMState tpm_state; /* not a QOM object */
> +    TpmTisSpiPktState tpm_tis_spi_state;
> +
> +    union TpmTisRWSizeByte first_byte;
> +    union TpmTisSpiHwAddr addr;
> +    union TpmTisSpiData data;

Are these device registers ? I am not sure the unions are very useful.

> +    uint32_t data_size;
> +    uint8_t data_idx;
> +    uint8_t addr_idx;
> +};
> +
> +struct TpmTisSpiClass {
> +    SSIPeripheralClass parent_class;
> +};
> +
> +OBJECT_DECLARE_TYPE(TpmTisSpiState, TpmTisSpiClass, TPM_TIS_SPI)
> +
> +static void tpm_tis_spi_mmio_read(TpmTisSpiState *tts)
> +{
> +    uint16_t offset = tts->addr.addr & 0xffc;
> +
> +    switch (offset) {
> +    case TPM_TIS_REG_DATA_FIFO:
> +        for (uint8_t i = 0; i < tts->data_size; i++) {
> +            tts->data.bytes[i] = (uint8_t)tpm_tis_memory_ops.read(


you should add an address space for these memory transactions. Look for
address_space_read/write calls, in the Aspeed I2C model for example.

> +                                          &tts->tpm_state,
> +                                          tts->addr.addr,
> +                                          1);
> +        }
> +        break;
> +    default:
> +        tts->data.data = (uint32_t)tpm_tis_memory_ops.read(
> +                                   &tts->tpm_state,
> +                                   tts->addr.addr,
> +                                   tts->data_size);
> +    }
> +}
> +
> +static void tpm_tis_spi_mmio_write(TpmTisSpiState *tts)
> +{
> +    uint16_t offset = tts->addr.addr & 0xffc;
> +
> +    switch (offset) {
> +    case TPM_TIS_REG_DATA_FIFO:
> +        for (uint8_t i = 0; i < tts->data_size; i++) {
> +            tpm_tis_memory_ops.write(&tts->tpm_state,
> +                                     tts->addr.addr,
> +                                     tts->data.bytes[i],
> +                                     1);
> +        }
> +        break;
> +    default:
> +        tpm_tis_memory_ops.write(&tts->tpm_state,
> +                                 tts->addr.addr,
> +                                 tts->data.data,
> +                                 tts->data_size);
> +        }
> +}
> +
> +static uint32_t tpm_tis_spi_transfer8(SSIPeripheral *ss, uint32_t tx)
> +{
> +    TpmTisSpiState *tts = TPM_TIS_SPI(ss);
> +    uint32_t r = 1;
> +
> +    switch (tts->tpm_tis_spi_state) {
> +    case TIS_SPI_PKT_STATE_START:
> +        tts->first_byte.byte = (uint8_t)tx;
> +        tts->data_size = tts->first_byte.data_expected_size + 1;
> +        tts->data_idx = 0;
> +        tts->addr_idx = TPM_TIS_SPI_ADDR_BYTES;
> +        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_ADDRESS;
> +        break;
> +    case TIS_SPI_PKT_STATE_ADDRESS:
> +        assert(tts->addr_idx > 0);
> +        tts->addr.bytes[--tts->addr_idx] = (uint8_t)tx;
> +
> +        if (tts->addr_idx == 0) {
> +            if (tts->first_byte.rwflag == SPI_WRITE) {
> +                tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DATA_WR;
> +            } else { /* read and get the data ready */
> +                tpm_tis_spi_mmio_read(tts);
> +                tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DATA_RD;
> +            }
> +        }
> +        break;
> +    case TIS_SPI_PKT_STATE_DATA_WR:
> +        tts->data.bytes[tts->data_idx++] = (uint8_t)tx;
> +        if (tts->data_idx == tts->data_size) {
> +            tpm_tis_spi_mmio_write(tts);
> +            tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DONE;
> +        }
> +        break;
> +    case TIS_SPI_PKT_STATE_DATA_RD:
> +        r = tts->data.bytes[tts->data_idx++];
> +        if (tts->data_idx == tts->data_size) {
> +            tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DONE;
> +        }
> +        break;
> +    default:
> +        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DEACTIVATED;
> +        r = (uint32_t) -1;
> +    }
> +
> +    return r;
> +}
> +
> +/*
> + * Pre-reading logic for transfer:
> + * This is to fix the transaction between reading and writing.
> + * The first byte is arbitrarily inserted so we need to
> + * shift the all the output bytes (timeline) one byte right.
> + */
> +static uint32_t tpm_tis_spi_transfer8_ex(SSIPeripheral *ss, uint32_t tx)
> +{
> +    TpmTisSpiState *tts = TPM_TIS_SPI(ss);
> +    SSIBus *ssibus = (SSIBus *)qdev_get_parent_bus(DEVICE(tts));
> +
> +    TpmTisSpiPktState prev_state = tts->tpm_tis_spi_state;
> +    uint32_t r = tpm_tis_spi_transfer8(ss, tx);
> +    TpmTisSpiPktState curr_state = tts->tpm_tis_spi_state;
> +
> +    if (ssibus->preread &&
> +       /* cmd state has changed into DATA reading state */
> +       prev_state != TIS_SPI_PKT_STATE_DATA_RD &&
> +       curr_state == TIS_SPI_PKT_STATE_DATA_RD) {
> +        r = tpm_tis_spi_transfer8(ss, 0xFF);
> +    }
> +
> +    return r;
> +}
> +
> +static int tpm_tis_spi_cs(SSIPeripheral *ss, bool select)
> +{
> +    TpmTisSpiState *tts = TPM_TIS_SPI(ss);
> +
> +    if (select) { /* cs de-assert */
> +        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DEACTIVATED;
> +    } else {
> +        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_START;
> +        tts->first_byte.byte = 0;
> +        tts->addr.addr = 0;
> +        tts->data.data = 0;
> +    }
> +
> +    return 0;
> +}
> +
> +static int tpm_tis_pre_save_spi(void *opaque)
> +{
> +    TpmTisSpiState *sbdev = opaque;
> +
> +    return tpm_tis_pre_save(&sbdev->tpm_state);
> +}
> +
> +static const VMStateDescription vmstate_tpm_tis_spi = {
> +    .name = "tpm-tis-spi",
> +    .version_id = 0,
> +    .pre_save  = tpm_tis_pre_save_spi,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_BUFFER(tpm_state.buffer, TpmTisSpiState),
> +        VMSTATE_UINT16(tpm_state.rw_offset, TpmTisSpiState),
> +        VMSTATE_UINT8(tpm_state.active_locty, TpmTisSpiState),
> +        VMSTATE_UINT8(tpm_state.aborting_locty, TpmTisSpiState),
> +        VMSTATE_UINT8(tpm_state.next_locty, TpmTisSpiState),
> +
> +        VMSTATE_STRUCT_ARRAY(tpm_state.loc, TpmTisSpiState, TPM_TIS_NUM_LOCALITIES,
> +                             0, vmstate_locty, TPMLocality),
> +
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void tpm_tis_spi_request_completed(TPMIf *ti, int ret)
> +{
> +    TpmTisSpiState *sbdev = TPM_TIS_SPI(ti);
> +    TPMState *s = &sbdev->tpm_state;
> +
> +    tpm_tis_request_completed(s, ret);
> +}
> +
> +static enum TPMVersion tpm_tis_spi_get_tpm_version(TPMIf *ti)
> +{
> +    TpmTisSpiState *sbdev = TPM_TIS_SPI(ti);
> +    TPMState *s = &sbdev->tpm_state;
> +
> +    return tpm_tis_get_tpm_version(s);
> +}
> +
> +static void tpm_tis_spi_reset(DeviceState *dev)
> +{
> +    TpmTisSpiState *sbdev = TPM_TIS_SPI(dev);
> +    TPMState *s = &sbdev->tpm_state;
> +
> +    return tpm_tis_reset(s);
> +}
> +
> +static Property tpm_tis_spi_properties[] = {
> +    DEFINE_PROP_UINT32("irq", TpmTisSpiState, tpm_state.irq_num, TPM_TIS_IRQ),
> +    DEFINE_PROP_TPMBE("tpmdev", TpmTisSpiState, tpm_state.be_driver),
> +    DEFINE_PROP_BOOL("ppi", TpmTisSpiState, tpm_state.ppi_enabled, false),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void tpm_tis_spi_realizefn(SSIPeripheral *ss, Error **errp)
> +{
> +    TpmTisSpiState *sbdev = TPM_TIS_SPI(ss);
> +
> +    if (!tpm_find()) {
> +        error_setg(errp, "at most one TPM device is permitted");
> +        return;
> +    }
> +
> +    if (!sbdev->tpm_state.be_driver) {
> +        error_setg(errp, "'tpmdev' property is required");
> +        return;
> +    }
> +
> +    DeviceState *spi_gpio = qdev_find_recursive(sysbus_get_default(),
> +                                                TYPE_SPI_GPIO);
> +    qdev_connect_gpio_out_named(spi_gpio,
> +                                "SPI_CS_out", 0,
> +                                qdev_get_gpio_in_named(DEVICE(ss),
> +                                SSI_GPIO_CS, 0));
Typically, this part is done at the machine/board level because it
has knowledge of all devices but it is possible with the TPM devices?

How do you invoke QEMU ?

Thanks,

C.

> +}
> +
> +static void tpm_tis_spi_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    SSIPeripheralClass *k = SSI_PERIPHERAL_CLASS(klass);
> +    TPMIfClass *tc = TPM_IF_CLASS(klass);
> +
> +    device_class_set_props(dc, tpm_tis_spi_properties);
> +    k->realize = tpm_tis_spi_realizefn;
> +    k->transfer = tpm_tis_spi_transfer8_ex;
> +    k->set_cs = tpm_tis_spi_cs;
> +    k->cs_polarity = SSI_CS_LOW;
> +
> +    dc->vmsd  = &vmstate_tpm_tis_spi;
> +    tc->model = TPM_MODEL_TPM_TIS;
> +    dc->user_creatable = true;
> +    dc->reset = tpm_tis_spi_reset;
> +    tc->request_completed = tpm_tis_spi_request_completed;
> +    tc->get_version = tpm_tis_spi_get_tpm_version;
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +}
> +
> +static const TypeInfo tpm_tis_spi_info = {
> +    .name = TYPE_TPM_TIS_SPI,
> +    .parent = TYPE_SSI_PERIPHERAL,
> +    .instance_size = sizeof(TpmTisSpiState),
> +    .class_size = sizeof(TpmTisSpiClass),
> +    .class_init  = tpm_tis_spi_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_TPM_IF },
> +        { }
> +    }
> +};
> +
> +static void tpm_tis_spi_register(void)
> +{
> +    type_register_static(&tpm_tis_spi_info);
> +}
> +
> +type_init(tpm_tis_spi_register)
> diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
> index fb40e30ff6..6a6b311e47 100644
> --- a/include/sysemu/tpm.h
> +++ b/include/sysemu/tpm.h
> @@ -46,6 +46,7 @@ struct TPMIfClass {
>   
>   #define TYPE_TPM_TIS_ISA            "tpm-tis"
>   #define TYPE_TPM_TIS_SYSBUS         "tpm-tis-device"
> +#define TYPE_TPM_TIS_SPI            "tpm-tis-spi-device"
>   #define TYPE_TPM_CRB                "tpm-crb"
>   #define TYPE_TPM_SPAPR              "tpm-spapr"
>   
> @@ -53,6 +54,8 @@ struct TPMIfClass {
>       object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_ISA)
>   #define TPM_IS_TIS_SYSBUS(chr)                      \
>       object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SYSBUS)
> +#define TPM_IS_TIS_SPI(chr)                      \
> +    object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SPI)
>   #define TPM_IS_CRB(chr)                             \
>       object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB)
>   #define TPM_IS_SPAPR(chr)                           \

Re: [RFC 1/1] hw: tpmtisspi: add SPI support to QEMU TPM implementation

[Peter Delevoryas]

On Wed, Aug 03, 2022 at 10:52:23AM +0200, Cédric Le Goater wrote:
> On 8/3/22 04:32, Iris Chen wrote:
> > From: Iris Chen <irischenlj@fb.com>
> 
> A commit log telling us about this new device would be good to have.
> 
> 
> > Signed-off-by: Iris Chen <irischenlj@fb.com>
> > ---
> >   configs/devices/arm-softmmu/default.mak |   1 +
> >   hw/arm/Kconfig                          |   5 +
> >   hw/tpm/Kconfig                          |   5 +
> >   hw/tpm/meson.build                      |   1 +
> >   hw/tpm/tpm_tis_spi.c                    | 311 ++++++++++++++++++++++++
> >   include/sysemu/tpm.h                    |   3 +
> >   6 files changed, 326 insertions(+)
> >   create mode 100644 hw/tpm/tpm_tis_spi.c
> > 
> > diff --git a/configs/devices/arm-softmmu/default.mak b/configs/devices/arm-softmmu/default.mak
> > index 6985a25377..80d2841568 100644
> > --- a/configs/devices/arm-softmmu/default.mak
> > +++ b/configs/devices/arm-softmmu/default.mak
> > @@ -42,3 +42,4 @@ CONFIG_FSL_IMX6UL=y
> >   CONFIG_SEMIHOSTING=y
> >   CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y
> >   CONFIG_ALLWINNER_H3=y
> > +CONFIG_FBOBMC_AST=y
> 
> I don't think this extra config is useful for now
> 
> > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> > index 15fa79afd3..193decaec1 100644
> > --- a/hw/arm/Kconfig
> > +++ b/hw/arm/Kconfig
> > @@ -458,6 +458,11 @@ config ASPEED_SOC
> >       select PMBUS
> >       select MAX31785
> > +config FBOBMC_AST
> > +    bool
> > +    select ASPEED_SOC
> > +    select TPM_TIS_SPI
> > +
> >   config MPS2
> >       bool
> >       imply I2C_DEVICES
> > diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
> > index 29e82f3c92..370a43f045 100644
> > --- a/hw/tpm/Kconfig
> > +++ b/hw/tpm/Kconfig
> > @@ -8,6 +8,11 @@ config TPM_TIS_SYSBUS
> >       depends on TPM
> >       select TPM_TIS
> > +config TPM_TIS_SPI
> > +    bool
> > +    depends on TPM
> > +    select TPM_TIS
> > +
> >   config TPM_TIS
> >       bool
> >       depends on TPM
> > diff --git a/hw/tpm/meson.build b/hw/tpm/meson.build
> > index 1c68d81d6a..1a057f4e36 100644
> > --- a/hw/tpm/meson.build
> > +++ b/hw/tpm/meson.build
> > @@ -2,6 +2,7 @@ softmmu_ss.add(when: 'CONFIG_TPM_TIS', if_true: files('tpm_tis_common.c'))
> >   softmmu_ss.add(when: 'CONFIG_TPM_TIS_ISA', if_true: files('tpm_tis_isa.c'))
> >   softmmu_ss.add(when: 'CONFIG_TPM_TIS_SYSBUS', if_true: files('tpm_tis_sysbus.c'))
> >   softmmu_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_crb.c'))
> > +softmmu_ss.add(when: 'CONFIG_TPM_TIS_SPI', if_true: files('tpm_tis_spi.c'))
> >   specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_TIS'], if_true: files('tpm_ppi.c'))
> >   specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_CRB'], if_true: files('tpm_ppi.c'))
> > diff --git a/hw/tpm/tpm_tis_spi.c b/hw/tpm/tpm_tis_spi.c
> > new file mode 100644
> > index 0000000000..c98ddcfddb
> > --- /dev/null
> > +++ b/hw/tpm/tpm_tis_spi.c
> > @@ -0,0 +1,311 @@
> > +#include "qemu/osdep.h"
> > +#include "hw/qdev-properties.h"
> > +#include "migration/vmstate.h"
> > +#include "hw/acpi/tpm.h"
> > +#include "tpm_prop.h"
> > +#include "tpm_tis.h"
> > +#include "qom/object.h"
> > +#include "hw/ssi/ssi.h"
> > +#include "hw/ssi/spi_gpio.h"
> > +
> > +#define TPM_TIS_SPI_ADDR_BYTES 3
> > +#define SPI_WRITE 0
> > +
> > +typedef enum {
> > +    TIS_SPI_PKT_STATE_DEACTIVATED = 0,
> > +    TIS_SPI_PKT_STATE_START,
> > +    TIS_SPI_PKT_STATE_ADDRESS,
> > +    TIS_SPI_PKT_STATE_DATA_WR,
> > +    TIS_SPI_PKT_STATE_DATA_RD,
> > +    TIS_SPI_PKT_STATE_DONE,
> > +} TpmTisSpiPktState;
> > +
> > +union TpmTisRWSizeByte {
> > +    uint8_t byte;
> > +    struct {
> > +        uint8_t data_expected_size:6;
> > +        uint8_t resv:1;
> > +        uint8_t rwflag:1;
> > +    };
> > +};
> > +
> > +union TpmTisSpiHwAddr {
> > +    hwaddr addr;
> > +    uint8_t bytes[sizeof(hwaddr)];
> > +};
> > +
> > +union TpmTisSpiData {
> > +    uint32_t data;
> > +    uint8_t bytes[64];
> > +};
> > +
> > +struct TpmTisSpiState {
> > +    /*< private >*/
> > +    SSIPeripheral parent_obj;
> > +
> > +    /*< public >*/
> > +    TPMState tpm_state; /* not a QOM object */
> > +    TpmTisSpiPktState tpm_tis_spi_state;
> > +
> > +    union TpmTisRWSizeByte first_byte;
> > +    union TpmTisSpiHwAddr addr;
> > +    union TpmTisSpiData data;
> 
> Are these device registers ? I am not sure the unions are very useful.

+1, I don't think we should be using unions, instead we should split out
all the relevant fields we want to store and use extract32/deposit32/etc
if necessary.

> 
> > +    uint32_t data_size;
> > +    uint8_t data_idx;
> > +    uint8_t addr_idx;
> > +};
> > +
> > +struct TpmTisSpiClass {
> > +    SSIPeripheralClass parent_class;
> > +};
> > +
> > +OBJECT_DECLARE_TYPE(TpmTisSpiState, TpmTisSpiClass, TPM_TIS_SPI)
> > +
> > +static void tpm_tis_spi_mmio_read(TpmTisSpiState *tts)
> > +{
> > +    uint16_t offset = tts->addr.addr & 0xffc;
> > +
> > +    switch (offset) {
> > +    case TPM_TIS_REG_DATA_FIFO:
> > +        for (uint8_t i = 0; i < tts->data_size; i++) {
> > +            tts->data.bytes[i] = (uint8_t)tpm_tis_memory_ops.read(
> 
> 
> you should add an address space for these memory transactions. Look for
> address_space_read/write calls, in the Aspeed I2C model for example.

Yeah, or an even better example might be tpm_tis_isa. I feel like a
shortcut might have been taken here to pull in tpm_tis_memory_ops
directly, but we should have been treating the interface with the TPM as
a generic address space.

> 
> > +                                          &tts->tpm_state,
> > +                                          tts->addr.addr,
> > +                                          1);
> > +        }
> > +        break;
> > +    default:
> > +        tts->data.data = (uint32_t)tpm_tis_memory_ops.read(
> > +                                   &tts->tpm_state,
> > +                                   tts->addr.addr,
> > +                                   tts->data_size);
> > +    }
> > +}
> > +
> > +static void tpm_tis_spi_mmio_write(TpmTisSpiState *tts)
> > +{
> > +    uint16_t offset = tts->addr.addr & 0xffc;
> > +
> > +    switch (offset) {
> > +    case TPM_TIS_REG_DATA_FIFO:
> > +        for (uint8_t i = 0; i < tts->data_size; i++) {
> > +            tpm_tis_memory_ops.write(&tts->tpm_state,
> > +                                     tts->addr.addr,
> > +                                     tts->data.bytes[i],
> > +                                     1);
> > +        }
> > +        break;
> > +    default:
> > +        tpm_tis_memory_ops.write(&tts->tpm_state,
> > +                                 tts->addr.addr,
> > +                                 tts->data.data,
> > +                                 tts->data_size);
> > +        }
> > +}
> > +
> > +static uint32_t tpm_tis_spi_transfer8(SSIPeripheral *ss, uint32_t tx)
> > +{
> > +    TpmTisSpiState *tts = TPM_TIS_SPI(ss);
> > +    uint32_t r = 1;
> > +
> > +    switch (tts->tpm_tis_spi_state) {
> > +    case TIS_SPI_PKT_STATE_START:
> > +        tts->first_byte.byte = (uint8_t)tx;
> > +        tts->data_size = tts->first_byte.data_expected_size + 1;
> > +        tts->data_idx = 0;
> > +        tts->addr_idx = TPM_TIS_SPI_ADDR_BYTES;
> > +        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_ADDRESS;
> > +        break;
> > +    case TIS_SPI_PKT_STATE_ADDRESS:
> > +        assert(tts->addr_idx > 0);
> > +        tts->addr.bytes[--tts->addr_idx] = (uint8_t)tx;
> > +
> > +        if (tts->addr_idx == 0) {
> > +            if (tts->first_byte.rwflag == SPI_WRITE) {
> > +                tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DATA_WR;
> > +            } else { /* read and get the data ready */
> > +                tpm_tis_spi_mmio_read(tts);
> > +                tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DATA_RD;
> > +            }
> > +        }
> > +        break;
> > +    case TIS_SPI_PKT_STATE_DATA_WR:
> > +        tts->data.bytes[tts->data_idx++] = (uint8_t)tx;
> > +        if (tts->data_idx == tts->data_size) {
> > +            tpm_tis_spi_mmio_write(tts);
> > +            tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DONE;
> > +        }
> > +        break;
> > +    case TIS_SPI_PKT_STATE_DATA_RD:
> > +        r = tts->data.bytes[tts->data_idx++];
> > +        if (tts->data_idx == tts->data_size) {
> > +            tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DONE;
> > +        }
> > +        break;
> > +    default:
> > +        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DEACTIVATED;
> > +        r = (uint32_t) -1;
> > +    }
> > +
> > +    return r;
> > +}
> > +
> > +/*
> > + * Pre-reading logic for transfer:
> > + * This is to fix the transaction between reading and writing.
> > + * The first byte is arbitrarily inserted so we need to
> > + * shift the all the output bytes (timeline) one byte right.
> > + */
> > +static uint32_t tpm_tis_spi_transfer8_ex(SSIPeripheral *ss, uint32_t tx)
> > +{
> > +    TpmTisSpiState *tts = TPM_TIS_SPI(ss);
> > +    SSIBus *ssibus = (SSIBus *)qdev_get_parent_bus(DEVICE(tts));
> > +
> > +    TpmTisSpiPktState prev_state = tts->tpm_tis_spi_state;
> > +    uint32_t r = tpm_tis_spi_transfer8(ss, tx);
> > +    TpmTisSpiPktState curr_state = tts->tpm_tis_spi_state;
> > +
> > +    if (ssibus->preread &&
> > +       /* cmd state has changed into DATA reading state */
> > +       prev_state != TIS_SPI_PKT_STATE_DATA_RD &&
> > +       curr_state == TIS_SPI_PKT_STATE_DATA_RD) {
> > +        r = tpm_tis_spi_transfer8(ss, 0xFF);
> > +    }
> > +
> > +    return r;
> > +}
> > +
> > +static int tpm_tis_spi_cs(SSIPeripheral *ss, bool select)
> > +{
> > +    TpmTisSpiState *tts = TPM_TIS_SPI(ss);
> > +
> > +    if (select) { /* cs de-assert */
> > +        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DEACTIVATED;
> > +    } else {
> > +        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_START;
> > +        tts->first_byte.byte = 0;
> > +        tts->addr.addr = 0;
> > +        tts->data.data = 0;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static int tpm_tis_pre_save_spi(void *opaque)
> > +{
> > +    TpmTisSpiState *sbdev = opaque;
> > +
> > +    return tpm_tis_pre_save(&sbdev->tpm_state);
> > +}
> > +
> > +static const VMStateDescription vmstate_tpm_tis_spi = {
> > +    .name = "tpm-tis-spi",
> > +    .version_id = 0,
> > +    .pre_save  = tpm_tis_pre_save_spi,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_BUFFER(tpm_state.buffer, TpmTisSpiState),
> > +        VMSTATE_UINT16(tpm_state.rw_offset, TpmTisSpiState),
> > +        VMSTATE_UINT8(tpm_state.active_locty, TpmTisSpiState),
> > +        VMSTATE_UINT8(tpm_state.aborting_locty, TpmTisSpiState),
> > +        VMSTATE_UINT8(tpm_state.next_locty, TpmTisSpiState),
> > +
> > +        VMSTATE_STRUCT_ARRAY(tpm_state.loc, TpmTisSpiState, TPM_TIS_NUM_LOCALITIES,
> > +                             0, vmstate_locty, TPMLocality),
> > +
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> > +static void tpm_tis_spi_request_completed(TPMIf *ti, int ret)
> > +{
> > +    TpmTisSpiState *sbdev = TPM_TIS_SPI(ti);
> > +    TPMState *s = &sbdev->tpm_state;
> > +
> > +    tpm_tis_request_completed(s, ret);
> > +}
> > +
> > +static enum TPMVersion tpm_tis_spi_get_tpm_version(TPMIf *ti)
> > +{
> > +    TpmTisSpiState *sbdev = TPM_TIS_SPI(ti);
> > +    TPMState *s = &sbdev->tpm_state;
> > +
> > +    return tpm_tis_get_tpm_version(s);
> > +}
> > +
> > +static void tpm_tis_spi_reset(DeviceState *dev)
> > +{
> > +    TpmTisSpiState *sbdev = TPM_TIS_SPI(dev);
> > +    TPMState *s = &sbdev->tpm_state;
> > +
> > +    return tpm_tis_reset(s);
> > +}
> > +
> > +static Property tpm_tis_spi_properties[] = {
> > +    DEFINE_PROP_UINT32("irq", TpmTisSpiState, tpm_state.irq_num, TPM_TIS_IRQ),
> > +    DEFINE_PROP_TPMBE("tpmdev", TpmTisSpiState, tpm_state.be_driver),
> > +    DEFINE_PROP_BOOL("ppi", TpmTisSpiState, tpm_state.ppi_enabled, false),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > +static void tpm_tis_spi_realizefn(SSIPeripheral *ss, Error **errp)
> > +{
> > +    TpmTisSpiState *sbdev = TPM_TIS_SPI(ss);
> > +
> > +    if (!tpm_find()) {
> > +        error_setg(errp, "at most one TPM device is permitted");
> > +        return;
> > +    }
> > +
> > +    if (!sbdev->tpm_state.be_driver) {
> > +        error_setg(errp, "'tpmdev' property is required");
> > +        return;
> > +    }
> > +
> > +    DeviceState *spi_gpio = qdev_find_recursive(sysbus_get_default(),
> > +                                                TYPE_SPI_GPIO);
> > +    qdev_connect_gpio_out_named(spi_gpio,
> > +                                "SPI_CS_out", 0,
> > +                                qdev_get_gpio_in_named(DEVICE(ss),
> > +                                SSI_GPIO_CS, 0));
> Typically, this part is done at the machine/board level because it
> has knowledge of all devices but it is possible with the TPM devices?

+1, yeah this part should be removed.

> 
> How do you invoke QEMU ?

We have some hard-coded machine/board level code that wires up stuff,
and then I think Iris was hard-coding this stuff in the TPM TIS SPI
model so that the whole thing could be hidden behind a Kconfig and wired
in automatically when the Kconfig was enabled. But we should require
users to wire it up themselves in the board code.

> 
> Thanks,
> 
> C.
> 
> > +}
> > +
> > +static void tpm_tis_spi_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +    SSIPeripheralClass *k = SSI_PERIPHERAL_CLASS(klass);
> > +    TPMIfClass *tc = TPM_IF_CLASS(klass);
> > +
> > +    device_class_set_props(dc, tpm_tis_spi_properties);
> > +    k->realize = tpm_tis_spi_realizefn;
> > +    k->transfer = tpm_tis_spi_transfer8_ex;
> > +    k->set_cs = tpm_tis_spi_cs;
> > +    k->cs_polarity = SSI_CS_LOW;
> > +
> > +    dc->vmsd  = &vmstate_tpm_tis_spi;
> > +    tc->model = TPM_MODEL_TPM_TIS;
> > +    dc->user_creatable = true;
> > +    dc->reset = tpm_tis_spi_reset;
> > +    tc->request_completed = tpm_tis_spi_request_completed;
> > +    tc->get_version = tpm_tis_spi_get_tpm_version;
> > +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> > +}
> > +
> > +static const TypeInfo tpm_tis_spi_info = {
> > +    .name = TYPE_TPM_TIS_SPI,
> > +    .parent = TYPE_SSI_PERIPHERAL,
> > +    .instance_size = sizeof(TpmTisSpiState),
> > +    .class_size = sizeof(TpmTisSpiClass),
> > +    .class_init  = tpm_tis_spi_class_init,
> > +    .interfaces = (InterfaceInfo[]) {
> > +        { TYPE_TPM_IF },
> > +        { }
> > +    }
> > +};
> > +
> > +static void tpm_tis_spi_register(void)
> > +{
> > +    type_register_static(&tpm_tis_spi_info);
> > +}
> > +
> > +type_init(tpm_tis_spi_register)
> > diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
> > index fb40e30ff6..6a6b311e47 100644
> > --- a/include/sysemu/tpm.h
> > +++ b/include/sysemu/tpm.h
> > @@ -46,6 +46,7 @@ struct TPMIfClass {
> >   #define TYPE_TPM_TIS_ISA            "tpm-tis"
> >   #define TYPE_TPM_TIS_SYSBUS         "tpm-tis-device"
> > +#define TYPE_TPM_TIS_SPI            "tpm-tis-spi-device"
> >   #define TYPE_TPM_CRB                "tpm-crb"
> >   #define TYPE_TPM_SPAPR              "tpm-spapr"
> > @@ -53,6 +54,8 @@ struct TPMIfClass {
> >       object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_ISA)
> >   #define TPM_IS_TIS_SYSBUS(chr)                      \
> >       object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SYSBUS)
> > +#define TPM_IS_TIS_SPI(chr)                      \
> > +    object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SPI)
> >   #define TPM_IS_CRB(chr)                             \
> >       object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB)
> >   #define TPM_IS_SPAPR(chr)                           \
> 

Re: [RFC 1/1] hw: tpmtisspi: add SPI support to QEMU TPM implementation

[Dan Zhang]

On Wed, Aug 3, 2022 at 10:30 AM Peter Delevoryas <peter@pjd.dev> wrote:
>
> On Wed, Aug 03, 2022 at 10:52:23AM +0200, Cédric Le Goater wrote:
> > On 8/3/22 04:32, Iris Chen wrote:
> > > From: Iris Chen <irischenlj@fb.com>
> >
> > A commit log telling us about this new device would be good to have.
> >
> >
> > > Signed-off-by: Iris Chen <irischenlj@fb.com>
> > > ---
> > >   configs/devices/arm-softmmu/default.mak |   1 +
> > >   hw/arm/Kconfig                          |   5 +
> > >   hw/tpm/Kconfig                          |   5 +
> > >   hw/tpm/meson.build                      |   1 +
> > >   hw/tpm/tpm_tis_spi.c                    | 311 ++++++++++++++++++++++++
> > >   include/sysemu/tpm.h                    |   3 +
> > >   6 files changed, 326 insertions(+)
> > >   create mode 100644 hw/tpm/tpm_tis_spi.c
> > >
> > > diff --git a/configs/devices/arm-softmmu/default.mak b/configs/devices/arm-softmmu/default.mak
> > > index 6985a25377..80d2841568 100644
> > > --- a/configs/devices/arm-softmmu/default.mak
> > > +++ b/configs/devices/arm-softmmu/default.mak
> > > @@ -42,3 +42,4 @@ CONFIG_FSL_IMX6UL=y
> > >   CONFIG_SEMIHOSTING=y
> > >   CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y
> > >   CONFIG_ALLWINNER_H3=y
> > > +CONFIG_FBOBMC_AST=y
> >
> > I don't think this extra config is useful for now
> >
> > > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> > > index 15fa79afd3..193decaec1 100644
> > > --- a/hw/arm/Kconfig
> > > +++ b/hw/arm/Kconfig
> > > @@ -458,6 +458,11 @@ config ASPEED_SOC
> > >       select PMBUS
> > >       select MAX31785
> > > +config FBOBMC_AST
> > > +    bool
> > > +    select ASPEED_SOC
> > > +    select TPM_TIS_SPI
> > > +
> > >   config MPS2
> > >       bool
> > >       imply I2C_DEVICES
> > > diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
> > > index 29e82f3c92..370a43f045 100644
> > > --- a/hw/tpm/Kconfig
> > > +++ b/hw/tpm/Kconfig
> > > @@ -8,6 +8,11 @@ config TPM_TIS_SYSBUS
> > >       depends on TPM
> > >       select TPM_TIS
> > > +config TPM_TIS_SPI
> > > +    bool
> > > +    depends on TPM
> > > +    select TPM_TIS
> > > +
> > >   config TPM_TIS
> > >       bool
> > >       depends on TPM
> > > diff --git a/hw/tpm/meson.build b/hw/tpm/meson.build
> > > index 1c68d81d6a..1a057f4e36 100644
> > > --- a/hw/tpm/meson.build
> > > +++ b/hw/tpm/meson.build
> > > @@ -2,6 +2,7 @@ softmmu_ss.add(when: 'CONFIG_TPM_TIS', if_true: files('tpm_tis_common.c'))
> > >   softmmu_ss.add(when: 'CONFIG_TPM_TIS_ISA', if_true: files('tpm_tis_isa.c'))
> > >   softmmu_ss.add(when: 'CONFIG_TPM_TIS_SYSBUS', if_true: files('tpm_tis_sysbus.c'))
> > >   softmmu_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_crb.c'))
> > > +softmmu_ss.add(when: 'CONFIG_TPM_TIS_SPI', if_true: files('tpm_tis_spi.c'))
> > >   specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_TIS'], if_true: files('tpm_ppi.c'))
> > >   specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_CRB'], if_true: files('tpm_ppi.c'))
> > > diff --git a/hw/tpm/tpm_tis_spi.c b/hw/tpm/tpm_tis_spi.c
> > > new file mode 100644
> > > index 0000000000..c98ddcfddb
> > > --- /dev/null
> > > +++ b/hw/tpm/tpm_tis_spi.c
> > > @@ -0,0 +1,311 @@
> > > +#include "qemu/osdep.h"
> > > +#include "hw/qdev-properties.h"
> > > +#include "migration/vmstate.h"
> > > +#include "hw/acpi/tpm.h"
> > > +#include "tpm_prop.h"
> > > +#include "tpm_tis.h"
> > > +#include "qom/object.h"
> > > +#include "hw/ssi/ssi.h"
> > > +#include "hw/ssi/spi_gpio.h"
> > > +
> > > +#define TPM_TIS_SPI_ADDR_BYTES 3
> > > +#define SPI_WRITE 0
> > > +
> > > +typedef enum {
> > > +    TIS_SPI_PKT_STATE_DEACTIVATED = 0,
> > > +    TIS_SPI_PKT_STATE_START,
> > > +    TIS_SPI_PKT_STATE_ADDRESS,
> > > +    TIS_SPI_PKT_STATE_DATA_WR,
> > > +    TIS_SPI_PKT_STATE_DATA_RD,
> > > +    TIS_SPI_PKT_STATE_DONE,
> > > +} TpmTisSpiPktState;
> > > +
> > > +union TpmTisRWSizeByte {
> > > +    uint8_t byte;
> > > +    struct {
> > > +        uint8_t data_expected_size:6;
> > > +        uint8_t resv:1;
> > > +        uint8_t rwflag:1;
> > > +    };
> > > +};
> > > +
> > > +union TpmTisSpiHwAddr {
> > > +    hwaddr addr;
> > > +    uint8_t bytes[sizeof(hwaddr)];
> > > +};
> > > +
> > > +union TpmTisSpiData {
> > > +    uint32_t data;
> > > +    uint8_t bytes[64];
> > > +};
> > > +
> > > +struct TpmTisSpiState {
> > > +    /*< private >*/
> > > +    SSIPeripheral parent_obj;
> > > +
> > > +    /*< public >*/
> > > +    TPMState tpm_state; /* not a QOM object */
> > > +    TpmTisSpiPktState tpm_tis_spi_state;
> > > +
> > > +    union TpmTisRWSizeByte first_byte;
> > > +    union TpmTisSpiHwAddr addr;
> > > +    union TpmTisSpiData data;
> >
> > Are these device registers ? I am not sure the unions are very useful.
>
> +1, I don't think we should be using unions, instead we should split out
> all the relevant fields we want to store and use extract32/deposit32/etc
> if necessary.
These union is used to saving us code to extract the bits from first byte
and assembling the hwaddr and unint32_t data from bytes.
I think as the bitfields has compiler implementation dependency of endianness
we can switch to use extract8. or handle it for gcc
https://stackoverflow.com/questions/47600584/bitfield-endianness-in-gcc

>
> >
> > > +    uint32_t data_size;
> > > +    uint8_t data_idx;
> > > +    uint8_t addr_idx;
> > > +};
> > > +
> > > +struct TpmTisSpiClass {
> > > +    SSIPeripheralClass parent_class;
> > > +};
> > > +
> > > +OBJECT_DECLARE_TYPE(TpmTisSpiState, TpmTisSpiClass, TPM_TIS_SPI)
> > > +
> > > +static void tpm_tis_spi_mmio_read(TpmTisSpiState *tts)
> > > +{
> > > +    uint16_t offset = tts->addr.addr & 0xffc;
> > > +
> > > +    switch (offset) {
> > > +    case TPM_TIS_REG_DATA_FIFO:
> > > +        for (uint8_t i = 0; i < tts->data_size; i++) {
> > > +            tts->data.bytes[i] = (uint8_t)tpm_tis_memory_ops.read(
> >
> >
> > you should add an address space for these memory transactions. Look for
> > address_space_read/write calls, in the Aspeed I2C model for example.
>
> Yeah, or an even better example might be tpm_tis_isa. I feel like a
> shortcut might have been taken here to pull in tpm_tis_memory_ops
> directly, but we should have been treating the interface with the TPM as
> a generic address space.
>
No, in our application we did not have those memory region
[0xFFD4_0000 ~ 0xFFD4_FFFF]
here we mapping the TPM TIS SPI message operations into mmio operations is to
reuse the TIS registers models already implemented in tpm_tis_common.c
> >
> > > +                                          &tts->tpm_state,
> > > +                                          tts->addr.addr,
> > > +                                          1);
> > > +        }
> > > +        break;
> > > +    default:
> > > +        tts->data.data = (uint32_t)tpm_tis_memory_ops.read(
> > > +                                   &tts->tpm_state,
> > > +                                   tts->addr.addr,
> > > +                                   tts->data_size);
> > > +    }
> > > +}
> > > +
> > > +static void tpm_tis_spi_mmio_write(TpmTisSpiState *tts)
> > > +{
> > > +    uint16_t offset = tts->addr.addr & 0xffc;
> > > +
> > > +    switch (offset) {
> > > +    case TPM_TIS_REG_DATA_FIFO:
> > > +        for (uint8_t i = 0; i < tts->data_size; i++) {
> > > +            tpm_tis_memory_ops.write(&tts->tpm_state,
> > > +                                     tts->addr.addr,
> > > +                                     tts->data.bytes[i],
> > > +                                     1);
> > > +        }
> > > +        break;
> > > +    default:
> > > +        tpm_tis_memory_ops.write(&tts->tpm_state,
> > > +                                 tts->addr.addr,
> > > +                                 tts->data.data,
> > > +                                 tts->data_size);
> > > +        }
> > > +}
> > > +
> > > +static uint32_t tpm_tis_spi_transfer8(SSIPeripheral *ss, uint32_t tx)
> > > +{
> > > +    TpmTisSpiState *tts = TPM_TIS_SPI(ss);
> > > +    uint32_t r = 1;
> > > +
> > > +    switch (tts->tpm_tis_spi_state) {
> > > +    case TIS_SPI_PKT_STATE_START:
> > > +        tts->first_byte.byte = (uint8_t)tx;
> > > +        tts->data_size = tts->first_byte.data_expected_size + 1;
> > > +        tts->data_idx = 0;
> > > +        tts->addr_idx = TPM_TIS_SPI_ADDR_BYTES;
> > > +        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_ADDRESS;
> > > +        break;
> > > +    case TIS_SPI_PKT_STATE_ADDRESS:
> > > +        assert(tts->addr_idx > 0);
> > > +        tts->addr.bytes[--tts->addr_idx] = (uint8_t)tx;
> > > +
> > > +        if (tts->addr_idx == 0) {
> > > +            if (tts->first_byte.rwflag == SPI_WRITE) {
> > > +                tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DATA_WR;
> > > +            } else { /* read and get the data ready */
> > > +                tpm_tis_spi_mmio_read(tts);
> > > +                tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DATA_RD;
> > > +            }
> > > +        }
> > > +        break;
> > > +    case TIS_SPI_PKT_STATE_DATA_WR:
> > > +        tts->data.bytes[tts->data_idx++] = (uint8_t)tx;
> > > +        if (tts->data_idx == tts->data_size) {
> > > +            tpm_tis_spi_mmio_write(tts);
> > > +            tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DONE;
> > > +        }
> > > +        break;
> > > +    case TIS_SPI_PKT_STATE_DATA_RD:
> > > +        r = tts->data.bytes[tts->data_idx++];
> > > +        if (tts->data_idx == tts->data_size) {
> > > +            tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DONE;
> > > +        }
> > > +        break;
> > > +    default:
> > > +        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DEACTIVATED;
> > > +        r = (uint32_t) -1;
> > > +    }
> > > +
> > > +    return r;
> > > +}
> > > +
> > > +/*
> > > + * Pre-reading logic for transfer:
> > > + * This is to fix the transaction between reading and writing.
> > > + * The first byte is arbitrarily inserted so we need to
> > > + * shift the all the output bytes (timeline) one byte right.
> > > + */
> > > +static uint32_t tpm_tis_spi_transfer8_ex(SSIPeripheral *ss, uint32_t tx)
> > > +{
> > > +    TpmTisSpiState *tts = TPM_TIS_SPI(ss);
> > > +    SSIBus *ssibus = (SSIBus *)qdev_get_parent_bus(DEVICE(tts));
> > > +
> > > +    TpmTisSpiPktState prev_state = tts->tpm_tis_spi_state;
> > > +    uint32_t r = tpm_tis_spi_transfer8(ss, tx);
> > > +    TpmTisSpiPktState curr_state = tts->tpm_tis_spi_state;
> > > +
> > > +    if (ssibus->preread &&
> > > +       /* cmd state has changed into DATA reading state */
> > > +       prev_state != TIS_SPI_PKT_STATE_DATA_RD &&
> > > +       curr_state == TIS_SPI_PKT_STATE_DATA_RD) {
> > > +        r = tpm_tis_spi_transfer8(ss, 0xFF);
> > > +    }
> > > +
> > > +    return r;
> > > +}
> > > +
> > > +static int tpm_tis_spi_cs(SSIPeripheral *ss, bool select)
> > > +{
> > > +    TpmTisSpiState *tts = TPM_TIS_SPI(ss);
> > > +
> > > +    if (select) { /* cs de-assert */
> > > +        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DEACTIVATED;
> > > +    } else {
> > > +        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_START;
> > > +        tts->first_byte.byte = 0;
> > > +        tts->addr.addr = 0;
> > > +        tts->data.data = 0;
> > > +    }
> > > +
> > > +    return 0;
> > > +}
> > > +
> > > +static int tpm_tis_pre_save_spi(void *opaque)
> > > +{
> > > +    TpmTisSpiState *sbdev = opaque;
> > > +
> > > +    return tpm_tis_pre_save(&sbdev->tpm_state);
> > > +}
> > > +
> > > +static const VMStateDescription vmstate_tpm_tis_spi = {
> > > +    .name = "tpm-tis-spi",
> > > +    .version_id = 0,
> > > +    .pre_save  = tpm_tis_pre_save_spi,
> > > +    .fields = (VMStateField[]) {
> > > +        VMSTATE_BUFFER(tpm_state.buffer, TpmTisSpiState),
> > > +        VMSTATE_UINT16(tpm_state.rw_offset, TpmTisSpiState),
> > > +        VMSTATE_UINT8(tpm_state.active_locty, TpmTisSpiState),
> > > +        VMSTATE_UINT8(tpm_state.aborting_locty, TpmTisSpiState),
> > > +        VMSTATE_UINT8(tpm_state.next_locty, TpmTisSpiState),
> > > +
> > > +        VMSTATE_STRUCT_ARRAY(tpm_state.loc, TpmTisSpiState, TPM_TIS_NUM_LOCALITIES,
> > > +                             0, vmstate_locty, TPMLocality),
> > > +
> > > +        VMSTATE_END_OF_LIST()
> > > +    }
> > > +};
> > > +
> > > +static void tpm_tis_spi_request_completed(TPMIf *ti, int ret)
> > > +{
> > > +    TpmTisSpiState *sbdev = TPM_TIS_SPI(ti);
> > > +    TPMState *s = &sbdev->tpm_state;
> > > +
> > > +    tpm_tis_request_completed(s, ret);
> > > +}
> > > +
> > > +static enum TPMVersion tpm_tis_spi_get_tpm_version(TPMIf *ti)
> > > +{
> > > +    TpmTisSpiState *sbdev = TPM_TIS_SPI(ti);
> > > +    TPMState *s = &sbdev->tpm_state;
> > > +
> > > +    return tpm_tis_get_tpm_version(s);
> > > +}
> > > +
> > > +static void tpm_tis_spi_reset(DeviceState *dev)
> > > +{
> > > +    TpmTisSpiState *sbdev = TPM_TIS_SPI(dev);
> > > +    TPMState *s = &sbdev->tpm_state;
> > > +
> > > +    return tpm_tis_reset(s);
> > > +}
> > > +
> > > +static Property tpm_tis_spi_properties[] = {
> > > +    DEFINE_PROP_UINT32("irq", TpmTisSpiState, tpm_state.irq_num, TPM_TIS_IRQ),
> > > +    DEFINE_PROP_TPMBE("tpmdev", TpmTisSpiState, tpm_state.be_driver),
> > > +    DEFINE_PROP_BOOL("ppi", TpmTisSpiState, tpm_state.ppi_enabled, false),
> > > +    DEFINE_PROP_END_OF_LIST(),
> > > +};
> > > +
> > > +static void tpm_tis_spi_realizefn(SSIPeripheral *ss, Error **errp)
> > > +{
> > > +    TpmTisSpiState *sbdev = TPM_TIS_SPI(ss);
> > > +
> > > +    if (!tpm_find()) {
> > > +        error_setg(errp, "at most one TPM device is permitted");
> > > +        return;
> > > +    }
> > > +
> > > +    if (!sbdev->tpm_state.be_driver) {
> > > +        error_setg(errp, "'tpmdev' property is required");
> > > +        return;
> > > +    }
> > > +
> > > +    DeviceState *spi_gpio = qdev_find_recursive(sysbus_get_default(),
> > > +                                                TYPE_SPI_GPIO);
> > > +    qdev_connect_gpio_out_named(spi_gpio,
> > > +                                "SPI_CS_out", 0,
> > > +                                qdev_get_gpio_in_named(DEVICE(ss),
> > > +                                SSI_GPIO_CS, 0));
> > Typically, this part is done at the machine/board level because it
> > has knowledge of all devices but it is possible with the TPM devices?
>
> +1, yeah this part should be removed.
>
> >
> > How do you invoke QEMU ?
>
> We have some hard-coded machine/board level code that wires up stuff,
> and then I think Iris was hard-coding this stuff in the TPM TIS SPI
> model so that the whole thing could be hidden behind a Kconfig and wired
> in automatically when the Kconfig was enabled. But we should require
> users to wire it up themselves in the board code.
>
> >
> > Thanks,
> >
> > C.
> >
> > > +}
> > > +
> > > +static void tpm_tis_spi_class_init(ObjectClass *klass, void *data)
> > > +{
> > > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > > +    SSIPeripheralClass *k = SSI_PERIPHERAL_CLASS(klass);
> > > +    TPMIfClass *tc = TPM_IF_CLASS(klass);
> > > +
> > > +    device_class_set_props(dc, tpm_tis_spi_properties);
> > > +    k->realize = tpm_tis_spi_realizefn;
> > > +    k->transfer = tpm_tis_spi_transfer8_ex;
> > > +    k->set_cs = tpm_tis_spi_cs;
> > > +    k->cs_polarity = SSI_CS_LOW;
> > > +
> > > +    dc->vmsd  = &vmstate_tpm_tis_spi;
> > > +    tc->model = TPM_MODEL_TPM_TIS;
> > > +    dc->user_creatable = true;
> > > +    dc->reset = tpm_tis_spi_reset;
> > > +    tc->request_completed = tpm_tis_spi_request_completed;
> > > +    tc->get_version = tpm_tis_spi_get_tpm_version;
> > > +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> > > +}
> > > +
> > > +static const TypeInfo tpm_tis_spi_info = {
> > > +    .name = TYPE_TPM_TIS_SPI,
> > > +    .parent = TYPE_SSI_PERIPHERAL,
> > > +    .instance_size = sizeof(TpmTisSpiState),
> > > +    .class_size = sizeof(TpmTisSpiClass),
> > > +    .class_init  = tpm_tis_spi_class_init,
> > > +    .interfaces = (InterfaceInfo[]) {
> > > +        { TYPE_TPM_IF },
> > > +        { }
> > > +    }
> > > +};
> > > +
> > > +static void tpm_tis_spi_register(void)
> > > +{
> > > +    type_register_static(&tpm_tis_spi_info);
> > > +}
> > > +
> > > +type_init(tpm_tis_spi_register)
> > > diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
> > > index fb40e30ff6..6a6b311e47 100644
> > > --- a/include/sysemu/tpm.h
> > > +++ b/include/sysemu/tpm.h
> > > @@ -46,6 +46,7 @@ struct TPMIfClass {
> > >   #define TYPE_TPM_TIS_ISA            "tpm-tis"
> > >   #define TYPE_TPM_TIS_SYSBUS         "tpm-tis-device"
> > > +#define TYPE_TPM_TIS_SPI            "tpm-tis-spi-device"
> > >   #define TYPE_TPM_CRB                "tpm-crb"
> > >   #define TYPE_TPM_SPAPR              "tpm-spapr"
> > > @@ -53,6 +54,8 @@ struct TPMIfClass {
> > >       object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_ISA)
> > >   #define TPM_IS_TIS_SYSBUS(chr)                      \
> > >       object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SYSBUS)
> > > +#define TPM_IS_TIS_SPI(chr)                      \
> > > +    object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SPI)
> > >   #define TPM_IS_CRB(chr)                             \
> > >       object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB)
> > >   #define TPM_IS_SPAPR(chr)                           \
> >

Re: [RFC 1/1] hw: tpmtisspi: add SPI support to QEMU TPM implementation

[Peter Delevoryas]

On Thu, Aug 04, 2022 at 11:07:10AM -0700, Dan Zhang wrote:
> On Wed, Aug 3, 2022 at 10:30 AM Peter Delevoryas <peter@pjd.dev> wrote:
> >
> > On Wed, Aug 03, 2022 at 10:52:23AM +0200, Cédric Le Goater wrote:
> > > On 8/3/22 04:32, Iris Chen wrote:
> > > > From: Iris Chen <irischenlj@fb.com>
> > >
> > > A commit log telling us about this new device would be good to have.
> > >
> > >
> > > > Signed-off-by: Iris Chen <irischenlj@fb.com>
> > > > ---
> > > >   configs/devices/arm-softmmu/default.mak |   1 +
> > > >   hw/arm/Kconfig                          |   5 +
> > > >   hw/tpm/Kconfig                          |   5 +
> > > >   hw/tpm/meson.build                      |   1 +
> > > >   hw/tpm/tpm_tis_spi.c                    | 311 ++++++++++++++++++++++++
> > > >   include/sysemu/tpm.h                    |   3 +
> > > >   6 files changed, 326 insertions(+)
> > > >   create mode 100644 hw/tpm/tpm_tis_spi.c
> > > >
> > > > diff --git a/configs/devices/arm-softmmu/default.mak b/configs/devices/arm-softmmu/default.mak
> > > > index 6985a25377..80d2841568 100644
> > > > --- a/configs/devices/arm-softmmu/default.mak
> > > > +++ b/configs/devices/arm-softmmu/default.mak
> > > > @@ -42,3 +42,4 @@ CONFIG_FSL_IMX6UL=y
> > > >   CONFIG_SEMIHOSTING=y
> > > >   CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y
> > > >   CONFIG_ALLWINNER_H3=y
> > > > +CONFIG_FBOBMC_AST=y
> > >
> > > I don't think this extra config is useful for now
> > >
> > > > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> > > > index 15fa79afd3..193decaec1 100644
> > > > --- a/hw/arm/Kconfig
> > > > +++ b/hw/arm/Kconfig
> > > > @@ -458,6 +458,11 @@ config ASPEED_SOC
> > > >       select PMBUS
> > > >       select MAX31785
> > > > +config FBOBMC_AST
> > > > +    bool
> > > > +    select ASPEED_SOC
> > > > +    select TPM_TIS_SPI
> > > > +
> > > >   config MPS2
> > > >       bool
> > > >       imply I2C_DEVICES
> > > > diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
> > > > index 29e82f3c92..370a43f045 100644
> > > > --- a/hw/tpm/Kconfig
> > > > +++ b/hw/tpm/Kconfig
> > > > @@ -8,6 +8,11 @@ config TPM_TIS_SYSBUS
> > > >       depends on TPM
> > > >       select TPM_TIS
> > > > +config TPM_TIS_SPI
> > > > +    bool
> > > > +    depends on TPM
> > > > +    select TPM_TIS
> > > > +
> > > >   config TPM_TIS
> > > >       bool
> > > >       depends on TPM
> > > > diff --git a/hw/tpm/meson.build b/hw/tpm/meson.build
> > > > index 1c68d81d6a..1a057f4e36 100644
> > > > --- a/hw/tpm/meson.build
> > > > +++ b/hw/tpm/meson.build
> > > > @@ -2,6 +2,7 @@ softmmu_ss.add(when: 'CONFIG_TPM_TIS', if_true: files('tpm_tis_common.c'))
> > > >   softmmu_ss.add(when: 'CONFIG_TPM_TIS_ISA', if_true: files('tpm_tis_isa.c'))
> > > >   softmmu_ss.add(when: 'CONFIG_TPM_TIS_SYSBUS', if_true: files('tpm_tis_sysbus.c'))
> > > >   softmmu_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_crb.c'))
> > > > +softmmu_ss.add(when: 'CONFIG_TPM_TIS_SPI', if_true: files('tpm_tis_spi.c'))
> > > >   specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_TIS'], if_true: files('tpm_ppi.c'))
> > > >   specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_CRB'], if_true: files('tpm_ppi.c'))
> > > > diff --git a/hw/tpm/tpm_tis_spi.c b/hw/tpm/tpm_tis_spi.c
> > > > new file mode 100644
> > > > index 0000000000..c98ddcfddb
> > > > --- /dev/null
> > > > +++ b/hw/tpm/tpm_tis_spi.c
> > > > @@ -0,0 +1,311 @@
> > > > +#include "qemu/osdep.h"
> > > > +#include "hw/qdev-properties.h"
> > > > +#include "migration/vmstate.h"
> > > > +#include "hw/acpi/tpm.h"
> > > > +#include "tpm_prop.h"
> > > > +#include "tpm_tis.h"
> > > > +#include "qom/object.h"
> > > > +#include "hw/ssi/ssi.h"
> > > > +#include "hw/ssi/spi_gpio.h"
> > > > +
> > > > +#define TPM_TIS_SPI_ADDR_BYTES 3
> > > > +#define SPI_WRITE 0
> > > > +
> > > > +typedef enum {
> > > > +    TIS_SPI_PKT_STATE_DEACTIVATED = 0,
> > > > +    TIS_SPI_PKT_STATE_START,
> > > > +    TIS_SPI_PKT_STATE_ADDRESS,
> > > > +    TIS_SPI_PKT_STATE_DATA_WR,
> > > > +    TIS_SPI_PKT_STATE_DATA_RD,
> > > > +    TIS_SPI_PKT_STATE_DONE,
> > > > +} TpmTisSpiPktState;
> > > > +
> > > > +union TpmTisRWSizeByte {
> > > > +    uint8_t byte;
> > > > +    struct {
> > > > +        uint8_t data_expected_size:6;
> > > > +        uint8_t resv:1;
> > > > +        uint8_t rwflag:1;
> > > > +    };
> > > > +};
> > > > +
> > > > +union TpmTisSpiHwAddr {
> > > > +    hwaddr addr;
> > > > +    uint8_t bytes[sizeof(hwaddr)];
> > > > +};
> > > > +
> > > > +union TpmTisSpiData {
> > > > +    uint32_t data;
> > > > +    uint8_t bytes[64];
> > > > +};
> > > > +
> > > > +struct TpmTisSpiState {
> > > > +    /*< private >*/
> > > > +    SSIPeripheral parent_obj;
> > > > +
> > > > +    /*< public >*/
> > > > +    TPMState tpm_state; /* not a QOM object */
> > > > +    TpmTisSpiPktState tpm_tis_spi_state;
> > > > +
> > > > +    union TpmTisRWSizeByte first_byte;
> > > > +    union TpmTisSpiHwAddr addr;
> > > > +    union TpmTisSpiData data;
> > >
> > > Are these device registers ? I am not sure the unions are very useful.
> >
> > +1, I don't think we should be using unions, instead we should split out
> > all the relevant fields we want to store and use extract32/deposit32/etc
> > if necessary.
> These union is used to saving us code to extract the bits from first byte
> and assembling the hwaddr and unint32_t data from bytes.
> I think as the bitfields has compiler implementation dependency of endianness
> we can switch to use extract8. or handle it for gcc
> https://stackoverflow.com/questions/47600584/bitfield-endianness-in-gcc

Hmmm ok, I'll let Cedric say what the preference would be in general since
I'm not totally sure.

> 
> >
> > >
> > > > +    uint32_t data_size;
> > > > +    uint8_t data_idx;
> > > > +    uint8_t addr_idx;
> > > > +};
> > > > +
> > > > +struct TpmTisSpiClass {
> > > > +    SSIPeripheralClass parent_class;
> > > > +};
> > > > +
> > > > +OBJECT_DECLARE_TYPE(TpmTisSpiState, TpmTisSpiClass, TPM_TIS_SPI)
> > > > +
> > > > +static void tpm_tis_spi_mmio_read(TpmTisSpiState *tts)
> > > > +{
> > > > +    uint16_t offset = tts->addr.addr & 0xffc;
> > > > +
> > > > +    switch (offset) {
> > > > +    case TPM_TIS_REG_DATA_FIFO:
> > > > +        for (uint8_t i = 0; i < tts->data_size; i++) {
> > > > +            tts->data.bytes[i] = (uint8_t)tpm_tis_memory_ops.read(
> > >
> > >
> > > you should add an address space for these memory transactions. Look for
> > > address_space_read/write calls, in the Aspeed I2C model for example.
> >
> > Yeah, or an even better example might be tpm_tis_isa. I feel like a
> > shortcut might have been taken here to pull in tpm_tis_memory_ops
> > directly, but we should have been treating the interface with the TPM as
> > a generic address space.
> >
> No, in our application we did not have those memory region
> [0xFFD4_0000 ~ 0xFFD4_FFFF]
> here we mapping the TPM TIS SPI message operations into mmio operations is to
> reuse the TIS registers models already implemented in tpm_tis_common.c

Ehhh, I think you're misunderstanding. Yes, the ISA model is mapping the
TPM MMIO address space into the ISA address space, and the SPI model
doesn't have an address space like that, but we should still be using an
MMIO address space to perform MMIO reads and writes, instead of calling
the MMIO's callbacks directly.

I think the motivation for this is that tpm_tis_common.c shouldn't even
expose tpm_tis_memory_ops directly, it should just expose an address
space to write to. But, I might be misunderstanding.

> > >
> > > > +                                          &tts->tpm_state,
> > > > +                                          tts->addr.addr,
> > > > +                                          1);
> > > > +        }
> > > > +        break;
> > > > +    default:
> > > > +        tts->data.data = (uint32_t)tpm_tis_memory_ops.read(
> > > > +                                   &tts->tpm_state,
> > > > +                                   tts->addr.addr,
> > > > +                                   tts->data_size);
> > > > +    }
> > > > +}
> > > > +
> > > > +static void tpm_tis_spi_mmio_write(TpmTisSpiState *tts)
> > > > +{
> > > > +    uint16_t offset = tts->addr.addr & 0xffc;
> > > > +
> > > > +    switch (offset) {
> > > > +    case TPM_TIS_REG_DATA_FIFO:
> > > > +        for (uint8_t i = 0; i < tts->data_size; i++) {
> > > > +            tpm_tis_memory_ops.write(&tts->tpm_state,
> > > > +                                     tts->addr.addr,
> > > > +                                     tts->data.bytes[i],
> > > > +                                     1);
> > > > +        }
> > > > +        break;
> > > > +    default:
> > > > +        tpm_tis_memory_ops.write(&tts->tpm_state,
> > > > +                                 tts->addr.addr,
> > > > +                                 tts->data.data,
> > > > +                                 tts->data_size);
> > > > +        }
> > > > +}
> > > > +
> > > > +static uint32_t tpm_tis_spi_transfer8(SSIPeripheral *ss, uint32_t tx)
> > > > +{
> > > > +    TpmTisSpiState *tts = TPM_TIS_SPI(ss);
> > > > +    uint32_t r = 1;
> > > > +
> > > > +    switch (tts->tpm_tis_spi_state) {
> > > > +    case TIS_SPI_PKT_STATE_START:
> > > > +        tts->first_byte.byte = (uint8_t)tx;
> > > > +        tts->data_size = tts->first_byte.data_expected_size + 1;
> > > > +        tts->data_idx = 0;
> > > > +        tts->addr_idx = TPM_TIS_SPI_ADDR_BYTES;
> > > > +        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_ADDRESS;
> > > > +        break;
> > > > +    case TIS_SPI_PKT_STATE_ADDRESS:
> > > > +        assert(tts->addr_idx > 0);
> > > > +        tts->addr.bytes[--tts->addr_idx] = (uint8_t)tx;
> > > > +
> > > > +        if (tts->addr_idx == 0) {
> > > > +            if (tts->first_byte.rwflag == SPI_WRITE) {
> > > > +                tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DATA_WR;
> > > > +            } else { /* read and get the data ready */
> > > > +                tpm_tis_spi_mmio_read(tts);
> > > > +                tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DATA_RD;
> > > > +            }
> > > > +        }
> > > > +        break;
> > > > +    case TIS_SPI_PKT_STATE_DATA_WR:
> > > > +        tts->data.bytes[tts->data_idx++] = (uint8_t)tx;
> > > > +        if (tts->data_idx == tts->data_size) {
> > > > +            tpm_tis_spi_mmio_write(tts);
> > > > +            tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DONE;
> > > > +        }
> > > > +        break;
> > > > +    case TIS_SPI_PKT_STATE_DATA_RD:
> > > > +        r = tts->data.bytes[tts->data_idx++];
> > > > +        if (tts->data_idx == tts->data_size) {
> > > > +            tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DONE;
> > > > +        }
> > > > +        break;
> > > > +    default:
> > > > +        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DEACTIVATED;
> > > > +        r = (uint32_t) -1;
> > > > +    }
> > > > +
> > > > +    return r;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Pre-reading logic for transfer:
> > > > + * This is to fix the transaction between reading and writing.
> > > > + * The first byte is arbitrarily inserted so we need to
> > > > + * shift the all the output bytes (timeline) one byte right.
> > > > + */
> > > > +static uint32_t tpm_tis_spi_transfer8_ex(SSIPeripheral *ss, uint32_t tx)
> > > > +{
> > > > +    TpmTisSpiState *tts = TPM_TIS_SPI(ss);
> > > > +    SSIBus *ssibus = (SSIBus *)qdev_get_parent_bus(DEVICE(tts));
> > > > +
> > > > +    TpmTisSpiPktState prev_state = tts->tpm_tis_spi_state;
> > > > +    uint32_t r = tpm_tis_spi_transfer8(ss, tx);
> > > > +    TpmTisSpiPktState curr_state = tts->tpm_tis_spi_state;
> > > > +
> > > > +    if (ssibus->preread &&
> > > > +       /* cmd state has changed into DATA reading state */
> > > > +       prev_state != TIS_SPI_PKT_STATE_DATA_RD &&
> > > > +       curr_state == TIS_SPI_PKT_STATE_DATA_RD) {
> > > > +        r = tpm_tis_spi_transfer8(ss, 0xFF);
> > > > +    }
> > > > +
> > > > +    return r;
> > > > +}
> > > > +
> > > > +static int tpm_tis_spi_cs(SSIPeripheral *ss, bool select)
> > > > +{
> > > > +    TpmTisSpiState *tts = TPM_TIS_SPI(ss);
> > > > +
> > > > +    if (select) { /* cs de-assert */
> > > > +        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DEACTIVATED;
> > > > +    } else {
> > > > +        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_START;
> > > > +        tts->first_byte.byte = 0;
> > > > +        tts->addr.addr = 0;
> > > > +        tts->data.data = 0;
> > > > +    }
> > > > +
> > > > +    return 0;
> > > > +}
> > > > +
> > > > +static int tpm_tis_pre_save_spi(void *opaque)
> > > > +{
> > > > +    TpmTisSpiState *sbdev = opaque;
> > > > +
> > > > +    return tpm_tis_pre_save(&sbdev->tpm_state);
> > > > +}
> > > > +
> > > > +static const VMStateDescription vmstate_tpm_tis_spi = {
> > > > +    .name = "tpm-tis-spi",
> > > > +    .version_id = 0,
> > > > +    .pre_save  = tpm_tis_pre_save_spi,
> > > > +    .fields = (VMStateField[]) {
> > > > +        VMSTATE_BUFFER(tpm_state.buffer, TpmTisSpiState),
> > > > +        VMSTATE_UINT16(tpm_state.rw_offset, TpmTisSpiState),
> > > > +        VMSTATE_UINT8(tpm_state.active_locty, TpmTisSpiState),
> > > > +        VMSTATE_UINT8(tpm_state.aborting_locty, TpmTisSpiState),
> > > > +        VMSTATE_UINT8(tpm_state.next_locty, TpmTisSpiState),
> > > > +
> > > > +        VMSTATE_STRUCT_ARRAY(tpm_state.loc, TpmTisSpiState, TPM_TIS_NUM_LOCALITIES,
> > > > +                             0, vmstate_locty, TPMLocality),
> > > > +
> > > > +        VMSTATE_END_OF_LIST()
> > > > +    }
> > > > +};
> > > > +
> > > > +static void tpm_tis_spi_request_completed(TPMIf *ti, int ret)
> > > > +{
> > > > +    TpmTisSpiState *sbdev = TPM_TIS_SPI(ti);
> > > > +    TPMState *s = &sbdev->tpm_state;
> > > > +
> > > > +    tpm_tis_request_completed(s, ret);
> > > > +}
> > > > +
> > > > +static enum TPMVersion tpm_tis_spi_get_tpm_version(TPMIf *ti)
> > > > +{
> > > > +    TpmTisSpiState *sbdev = TPM_TIS_SPI(ti);
> > > > +    TPMState *s = &sbdev->tpm_state;
> > > > +
> > > > +    return tpm_tis_get_tpm_version(s);
> > > > +}
> > > > +
> > > > +static void tpm_tis_spi_reset(DeviceState *dev)
> > > > +{
> > > > +    TpmTisSpiState *sbdev = TPM_TIS_SPI(dev);
> > > > +    TPMState *s = &sbdev->tpm_state;
> > > > +
> > > > +    return tpm_tis_reset(s);
> > > > +}
> > > > +
> > > > +static Property tpm_tis_spi_properties[] = {
> > > > +    DEFINE_PROP_UINT32("irq", TpmTisSpiState, tpm_state.irq_num, TPM_TIS_IRQ),
> > > > +    DEFINE_PROP_TPMBE("tpmdev", TpmTisSpiState, tpm_state.be_driver),
> > > > +    DEFINE_PROP_BOOL("ppi", TpmTisSpiState, tpm_state.ppi_enabled, false),
> > > > +    DEFINE_PROP_END_OF_LIST(),
> > > > +};
> > > > +
> > > > +static void tpm_tis_spi_realizefn(SSIPeripheral *ss, Error **errp)
> > > > +{
> > > > +    TpmTisSpiState *sbdev = TPM_TIS_SPI(ss);
> > > > +
> > > > +    if (!tpm_find()) {
> > > > +        error_setg(errp, "at most one TPM device is permitted");
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    if (!sbdev->tpm_state.be_driver) {
> > > > +        error_setg(errp, "'tpmdev' property is required");
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    DeviceState *spi_gpio = qdev_find_recursive(sysbus_get_default(),
> > > > +                                                TYPE_SPI_GPIO);
> > > > +    qdev_connect_gpio_out_named(spi_gpio,
> > > > +                                "SPI_CS_out", 0,
> > > > +                                qdev_get_gpio_in_named(DEVICE(ss),
> > > > +                                SSI_GPIO_CS, 0));
> > > Typically, this part is done at the machine/board level because it
> > > has knowledge of all devices but it is possible with the TPM devices?
> >
> > +1, yeah this part should be removed.
> >
> > >
> > > How do you invoke QEMU ?
> >
> > We have some hard-coded machine/board level code that wires up stuff,
> > and then I think Iris was hard-coding this stuff in the TPM TIS SPI
> > model so that the whole thing could be hidden behind a Kconfig and wired
> > in automatically when the Kconfig was enabled. But we should require
> > users to wire it up themselves in the board code.
> >
> > >
> > > Thanks,
> > >
> > > C.
> > >
> > > > +}
> > > > +
> > > > +static void tpm_tis_spi_class_init(ObjectClass *klass, void *data)
> > > > +{
> > > > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > > > +    SSIPeripheralClass *k = SSI_PERIPHERAL_CLASS(klass);
> > > > +    TPMIfClass *tc = TPM_IF_CLASS(klass);
> > > > +
> > > > +    device_class_set_props(dc, tpm_tis_spi_properties);
> > > > +    k->realize = tpm_tis_spi_realizefn;
> > > > +    k->transfer = tpm_tis_spi_transfer8_ex;
> > > > +    k->set_cs = tpm_tis_spi_cs;
> > > > +    k->cs_polarity = SSI_CS_LOW;
> > > > +
> > > > +    dc->vmsd  = &vmstate_tpm_tis_spi;
> > > > +    tc->model = TPM_MODEL_TPM_TIS;
> > > > +    dc->user_creatable = true;
> > > > +    dc->reset = tpm_tis_spi_reset;
> > > > +    tc->request_completed = tpm_tis_spi_request_completed;
> > > > +    tc->get_version = tpm_tis_spi_get_tpm_version;
> > > > +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> > > > +}
> > > > +
> > > > +static const TypeInfo tpm_tis_spi_info = {
> > > > +    .name = TYPE_TPM_TIS_SPI,
> > > > +    .parent = TYPE_SSI_PERIPHERAL,
> > > > +    .instance_size = sizeof(TpmTisSpiState),
> > > > +    .class_size = sizeof(TpmTisSpiClass),
> > > > +    .class_init  = tpm_tis_spi_class_init,
> > > > +    .interfaces = (InterfaceInfo[]) {
> > > > +        { TYPE_TPM_IF },
> > > > +        { }
> > > > +    }
> > > > +};
> > > > +
> > > > +static void tpm_tis_spi_register(void)
> > > > +{
> > > > +    type_register_static(&tpm_tis_spi_info);
> > > > +}
> > > > +
> > > > +type_init(tpm_tis_spi_register)
> > > > diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
> > > > index fb40e30ff6..6a6b311e47 100644
> > > > --- a/include/sysemu/tpm.h
> > > > +++ b/include/sysemu/tpm.h
> > > > @@ -46,6 +46,7 @@ struct TPMIfClass {
> > > >   #define TYPE_TPM_TIS_ISA            "tpm-tis"
> > > >   #define TYPE_TPM_TIS_SYSBUS         "tpm-tis-device"
> > > > +#define TYPE_TPM_TIS_SPI            "tpm-tis-spi-device"
> > > >   #define TYPE_TPM_CRB                "tpm-crb"
> > > >   #define TYPE_TPM_SPAPR              "tpm-spapr"
> > > > @@ -53,6 +54,8 @@ struct TPMIfClass {
> > > >       object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_ISA)
> > > >   #define TPM_IS_TIS_SYSBUS(chr)                      \
> > > >       object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SYSBUS)
> > > > +#define TPM_IS_TIS_SPI(chr)                      \
> > > > +    object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SPI)
> > > >   #define TPM_IS_CRB(chr)                             \
> > > >       object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB)
> > > >   #define TPM_IS_SPAPR(chr)                           \
> > >

Re: [RFC 1/1] hw: tpmtisspi: add SPI support to QEMU TPM implementation

[Dan Zhang]

On Thu, Aug 4, 2022 at 4:21 PM Peter Delevoryas <peter@pjd.dev> wrote:
>
> On Thu, Aug 04, 2022 at 11:07:10AM -0700, Dan Zhang wrote:
> > On Wed, Aug 3, 2022 at 10:30 AM Peter Delevoryas <peter@pjd.dev> wrote:
> > >
> > > On Wed, Aug 03, 2022 at 10:52:23AM +0200, Cédric Le Goater wrote:
> > > > On 8/3/22 04:32, Iris Chen wrote:
> > > > > From: Iris Chen <irischenlj@fb.com>
> > > >
> > > > A commit log telling us about this new device would be good to have.
> > > >
> > > >
> > > > > Signed-off-by: Iris Chen <irischenlj@fb.com>
> > > > > ---
> > > > >   configs/devices/arm-softmmu/default.mak |   1 +
> > > > >   hw/arm/Kconfig                          |   5 +
> > > > >   hw/tpm/Kconfig                          |   5 +
> > > > >   hw/tpm/meson.build                      |   1 +
> > > > >   hw/tpm/tpm_tis_spi.c                    | 311 ++++++++++++++++++++++++
> > > > >   include/sysemu/tpm.h                    |   3 +
> > > > >   6 files changed, 326 insertions(+)
> > > > >   create mode 100644 hw/tpm/tpm_tis_spi.c
> > > > >
> > > > > diff --git a/configs/devices/arm-softmmu/default.mak b/configs/devices/arm-softmmu/default.mak
> > > > > index 6985a25377..80d2841568 100644
> > > > > --- a/configs/devices/arm-softmmu/default.mak
> > > > > +++ b/configs/devices/arm-softmmu/default.mak
> > > > > @@ -42,3 +42,4 @@ CONFIG_FSL_IMX6UL=y
> > > > >   CONFIG_SEMIHOSTING=y
> > > > >   CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y
> > > > >   CONFIG_ALLWINNER_H3=y
> > > > > +CONFIG_FBOBMC_AST=y
> > > >
> > > > I don't think this extra config is useful for now
> > > >
> > > > > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> > > > > index 15fa79afd3..193decaec1 100644
> > > > > --- a/hw/arm/Kconfig
> > > > > +++ b/hw/arm/Kconfig
> > > > > @@ -458,6 +458,11 @@ config ASPEED_SOC
> > > > >       select PMBUS
> > > > >       select MAX31785
> > > > > +config FBOBMC_AST
> > > > > +    bool
> > > > > +    select ASPEED_SOC
> > > > > +    select TPM_TIS_SPI
> > > > > +
> > > > >   config MPS2
> > > > >       bool
> > > > >       imply I2C_DEVICES
> > > > > diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
> > > > > index 29e82f3c92..370a43f045 100644
> > > > > --- a/hw/tpm/Kconfig
> > > > > +++ b/hw/tpm/Kconfig
> > > > > @@ -8,6 +8,11 @@ config TPM_TIS_SYSBUS
> > > > >       depends on TPM
> > > > >       select TPM_TIS
> > > > > +config TPM_TIS_SPI
> > > > > +    bool
> > > > > +    depends on TPM
> > > > > +    select TPM_TIS
> > > > > +
> > > > >   config TPM_TIS
> > > > >       bool
> > > > >       depends on TPM
> > > > > diff --git a/hw/tpm/meson.build b/hw/tpm/meson.build
> > > > > index 1c68d81d6a..1a057f4e36 100644
> > > > > --- a/hw/tpm/meson.build
> > > > > +++ b/hw/tpm/meson.build
> > > > > @@ -2,6 +2,7 @@ softmmu_ss.add(when: 'CONFIG_TPM_TIS', if_true: files('tpm_tis_common.c'))
> > > > >   softmmu_ss.add(when: 'CONFIG_TPM_TIS_ISA', if_true: files('tpm_tis_isa.c'))
> > > > >   softmmu_ss.add(when: 'CONFIG_TPM_TIS_SYSBUS', if_true: files('tpm_tis_sysbus.c'))
> > > > >   softmmu_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_crb.c'))
> > > > > +softmmu_ss.add(when: 'CONFIG_TPM_TIS_SPI', if_true: files('tpm_tis_spi.c'))
> > > > >   specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_TIS'], if_true: files('tpm_ppi.c'))
> > > > >   specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_CRB'], if_true: files('tpm_ppi.c'))
> > > > > diff --git a/hw/tpm/tpm_tis_spi.c b/hw/tpm/tpm_tis_spi.c
> > > > > new file mode 100644
> > > > > index 0000000000..c98ddcfddb
> > > > > --- /dev/null
> > > > > +++ b/hw/tpm/tpm_tis_spi.c
> > > > > @@ -0,0 +1,311 @@
> > > > > +#include "qemu/osdep.h"
> > > > > +#include "hw/qdev-properties.h"
> > > > > +#include "migration/vmstate.h"
> > > > > +#include "hw/acpi/tpm.h"
> > > > > +#include "tpm_prop.h"
> > > > > +#include "tpm_tis.h"
> > > > > +#include "qom/object.h"
> > > > > +#include "hw/ssi/ssi.h"
> > > > > +#include "hw/ssi/spi_gpio.h"
> > > > > +
> > > > > +#define TPM_TIS_SPI_ADDR_BYTES 3
> > > > > +#define SPI_WRITE 0
> > > > > +
> > > > > +typedef enum {
> > > > > +    TIS_SPI_PKT_STATE_DEACTIVATED = 0,
> > > > > +    TIS_SPI_PKT_STATE_START,
> > > > > +    TIS_SPI_PKT_STATE_ADDRESS,
> > > > > +    TIS_SPI_PKT_STATE_DATA_WR,
> > > > > +    TIS_SPI_PKT_STATE_DATA_RD,
> > > > > +    TIS_SPI_PKT_STATE_DONE,
> > > > > +} TpmTisSpiPktState;
> > > > > +
> > > > > +union TpmTisRWSizeByte {
> > > > > +    uint8_t byte;
> > > > > +    struct {
> > > > > +        uint8_t data_expected_size:6;
> > > > > +        uint8_t resv:1;
> > > > > +        uint8_t rwflag:1;
> > > > > +    };
> > > > > +};
> > > > > +
> > > > > +union TpmTisSpiHwAddr {
> > > > > +    hwaddr addr;
> > > > > +    uint8_t bytes[sizeof(hwaddr)];
> > > > > +};
> > > > > +
> > > > > +union TpmTisSpiData {
> > > > > +    uint32_t data;
> > > > > +    uint8_t bytes[64];
> > > > > +};
> > > > > +
> > > > > +struct TpmTisSpiState {
> > > > > +    /*< private >*/
> > > > > +    SSIPeripheral parent_obj;
> > > > > +
> > > > > +    /*< public >*/
> > > > > +    TPMState tpm_state; /* not a QOM object */
> > > > > +    TpmTisSpiPktState tpm_tis_spi_state;
> > > > > +
> > > > > +    union TpmTisRWSizeByte first_byte;
> > > > > +    union TpmTisSpiHwAddr addr;
> > > > > +    union TpmTisSpiData data;
> > > >
> > > > Are these device registers ? I am not sure the unions are very useful.
> > >
> > > +1, I don't think we should be using unions, instead we should split out
> > > all the relevant fields we want to store and use extract32/deposit32/etc
> > > if necessary.
> > These union is used to saving us code to extract the bits from first byte
> > and assembling the hwaddr and unint32_t data from bytes.
> > I think as the bitfields has compiler implementation dependency of endianness
> > we can switch to use extract8. or handle it for gcc
> > https://stackoverflow.com/questions/47600584/bitfield-endianness-in-gcc
>
> Hmmm ok, I'll let Cedric say what the preference would be in general since
> I'm not totally sure.
>
> >
> > >
> > > >
> > > > > +    uint32_t data_size;
> > > > > +    uint8_t data_idx;
> > > > > +    uint8_t addr_idx;
> > > > > +};
> > > > > +
> > > > > +struct TpmTisSpiClass {
> > > > > +    SSIPeripheralClass parent_class;
> > > > > +};
> > > > > +
> > > > > +OBJECT_DECLARE_TYPE(TpmTisSpiState, TpmTisSpiClass, TPM_TIS_SPI)
> > > > > +
> > > > > +static void tpm_tis_spi_mmio_read(TpmTisSpiState *tts)
> > > > > +{
> > > > > +    uint16_t offset = tts->addr.addr & 0xffc;
> > > > > +
> > > > > +    switch (offset) {
> > > > > +    case TPM_TIS_REG_DATA_FIFO:
> > > > > +        for (uint8_t i = 0; i < tts->data_size; i++) {
> > > > > +            tts->data.bytes[i] = (uint8_t)tpm_tis_memory_ops.read(
> > > >
> > > >
> > > > you should add an address space for these memory transactions. Look for
> > > > address_space_read/write calls, in the Aspeed I2C model for example.
> > >
> > > Yeah, or an even better example might be tpm_tis_isa. I feel like a
> > > shortcut might have been taken here to pull in tpm_tis_memory_ops
> > > directly, but we should have been treating the interface with the TPM as
> > > a generic address space.
> > >
> > No, in our application we did not have those memory region
> > [0xFFD4_0000 ~ 0xFFD4_FFFF]
> > here we mapping the TPM TIS SPI message operations into mmio operations is to
> > reuse the TIS registers models already implemented in tpm_tis_common.c
>
> Ehhh, I think you're misunderstanding. Yes, the ISA model is mapping the
> TPM MMIO address space into the ISA address space, and the SPI model
> doesn't have an address space like that, but we should still be using an
> MMIO address space to perform MMIO reads and writes, instead of calling
> the MMIO's callbacks directly.
>
> I think the motivation for this is that tpm_tis_common.c shouldn't even
> expose tpm_tis_memory_ops directly, it should just expose an address
> space to write to. But, I might be misunderstanding.
>
tpm_tis_common.c exposing the mmio_ops
(https://github.com/qemu/qemu/blob/master/hw/tpm/tpm_tis.h#L83)
essentially it is implementing the standard TPM TIS model (those tpm
interface registers i.e. TPM_STS etc)
while the tpm_tis_xxxx.c front ends modeling these registers' accessing method
i.e. host directly read / write memory address 0xFFD4_xxxx to access
these register,
this is modeled by tpm_tis_isa.c (x86) or tpm_tis_sysbus.c (arm).
Thus these model need create mmio region.

while BMC is sending the TPM TIS accessing SPI message to access these
registers,
NOT  mmio access to these registers, so tpmi_tis_spi model shall NOT
create memory region.

BTW, The TCG TPM TIS spec chapter 6 ( especially 6.4.6 SPI Bit Protocol) specify
the SPI messages to read/write the TPM TIS registers.
And, on the PC or ARM host which using the mmio, under the hood, it is
the host-adapter
i.e. on PC the PCH or South Bridge, which implements the mmio,
converting the memory
 read/write operation into SPI messages. This is where the TPM
locality coming from.
> > > >
> > > > > +                                          &tts->tpm_state,
> > > > > +                                          tts->addr.addr,
> > > > > +                                          1);
> > > > > +        }
> > > > > +        break;
> > > > > +    default:
> > > > > +        tts->data.data = (uint32_t)tpm_tis_memory_ops.read(
> > > > > +                                   &tts->tpm_state,
> > > > > +                                   tts->addr.addr,
> > > > > +                                   tts->data_size);
> > > > > +    }
> > > > > +}
> > > > > +
> > > > > +static void tpm_tis_spi_mmio_write(TpmTisSpiState *tts)
> > > > > +{
> > > > > +    uint16_t offset = tts->addr.addr & 0xffc;
> > > > > +
> > > > > +    switch (offset) {
> > > > > +    case TPM_TIS_REG_DATA_FIFO:
> > > > > +        for (uint8_t i = 0; i < tts->data_size; i++) {
> > > > > +            tpm_tis_memory_ops.write(&tts->tpm_state,
> > > > > +                                     tts->addr.addr,
> > > > > +                                     tts->data.bytes[i],
> > > > > +                                     1);
> > > > > +        }
> > > > > +        break;
> > > > > +    default:
> > > > > +        tpm_tis_memory_ops.write(&tts->tpm_state,
> > > > > +                                 tts->addr.addr,
> > > > > +                                 tts->data.data,
> > > > > +                                 tts->data_size);
> > > > > +        }
> > > > > +}
> > > > > +
> > > > > +static uint32_t tpm_tis_spi_transfer8(SSIPeripheral *ss, uint32_t tx)
> > > > > +{
> > > > > +    TpmTisSpiState *tts = TPM_TIS_SPI(ss);
> > > > > +    uint32_t r = 1;
> > > > > +
> > > > > +    switch (tts->tpm_tis_spi_state) {
> > > > > +    case TIS_SPI_PKT_STATE_START:
> > > > > +        tts->first_byte.byte = (uint8_t)tx;
> > > > > +        tts->data_size = tts->first_byte.data_expected_size + 1;
> > > > > +        tts->data_idx = 0;
> > > > > +        tts->addr_idx = TPM_TIS_SPI_ADDR_BYTES;
> > > > > +        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_ADDRESS;
> > > > > +        break;
> > > > > +    case TIS_SPI_PKT_STATE_ADDRESS:
> > > > > +        assert(tts->addr_idx > 0);
> > > > > +        tts->addr.bytes[--tts->addr_idx] = (uint8_t)tx;
> > > > > +
> > > > > +        if (tts->addr_idx == 0) {
> > > > > +            if (tts->first_byte.rwflag == SPI_WRITE) {
> > > > > +                tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DATA_WR;
> > > > > +            } else { /* read and get the data ready */
> > > > > +                tpm_tis_spi_mmio_read(tts);
> > > > > +                tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DATA_RD;
> > > > > +            }
> > > > > +        }
> > > > > +        break;
> > > > > +    case TIS_SPI_PKT_STATE_DATA_WR:
> > > > > +        tts->data.bytes[tts->data_idx++] = (uint8_t)tx;
> > > > > +        if (tts->data_idx == tts->data_size) {
> > > > > +            tpm_tis_spi_mmio_write(tts);
> > > > > +            tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DONE;
> > > > > +        }
> > > > > +        break;
> > > > > +    case TIS_SPI_PKT_STATE_DATA_RD:
> > > > > +        r = tts->data.bytes[tts->data_idx++];
> > > > > +        if (tts->data_idx == tts->data_size) {
> > > > > +            tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DONE;
> > > > > +        }
> > > > > +        break;
> > > > > +    default:
> > > > > +        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DEACTIVATED;
> > > > > +        r = (uint32_t) -1;
> > > > > +    }
> > > > > +
> > > > > +    return r;
> > > > > +}
> > > > > +
> > > > > +/*
> > > > > + * Pre-reading logic for transfer:
> > > > > + * This is to fix the transaction between reading and writing.
> > > > > + * The first byte is arbitrarily inserted so we need to
> > > > > + * shift the all the output bytes (timeline) one byte right.
> > > > > + */
> > > > > +static uint32_t tpm_tis_spi_transfer8_ex(SSIPeripheral *ss, uint32_t tx)
> > > > > +{
> > > > > +    TpmTisSpiState *tts = TPM_TIS_SPI(ss);
> > > > > +    SSIBus *ssibus = (SSIBus *)qdev_get_parent_bus(DEVICE(tts));
> > > > > +
> > > > > +    TpmTisSpiPktState prev_state = tts->tpm_tis_spi_state;
> > > > > +    uint32_t r = tpm_tis_spi_transfer8(ss, tx);
> > > > > +    TpmTisSpiPktState curr_state = tts->tpm_tis_spi_state;
> > > > > +
> > > > > +    if (ssibus->preread &&
> > > > > +       /* cmd state has changed into DATA reading state */
> > > > > +       prev_state != TIS_SPI_PKT_STATE_DATA_RD &&
> > > > > +       curr_state == TIS_SPI_PKT_STATE_DATA_RD) {
> > > > > +        r = tpm_tis_spi_transfer8(ss, 0xFF);
> > > > > +    }
> > > > > +
> > > > > +    return r;
> > > > > +}
> > > > > +
> > > > > +static int tpm_tis_spi_cs(SSIPeripheral *ss, bool select)
> > > > > +{
> > > > > +    TpmTisSpiState *tts = TPM_TIS_SPI(ss);
> > > > > +
> > > > > +    if (select) { /* cs de-assert */
> > > > > +        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DEACTIVATED;
> > > > > +    } else {
> > > > > +        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_START;
> > > > > +        tts->first_byte.byte = 0;
> > > > > +        tts->addr.addr = 0;
> > > > > +        tts->data.data = 0;
> > > > > +    }
> > > > > +
> > > > > +    return 0;
> > > > > +}
> > > > > +
> > > > > +static int tpm_tis_pre_save_spi(void *opaque)
> > > > > +{
> > > > > +    TpmTisSpiState *sbdev = opaque;
> > > > > +
> > > > > +    return tpm_tis_pre_save(&sbdev->tpm_state);
> > > > > +}
> > > > > +
> > > > > +static const VMStateDescription vmstate_tpm_tis_spi = {
> > > > > +    .name = "tpm-tis-spi",
> > > > > +    .version_id = 0,
> > > > > +    .pre_save  = tpm_tis_pre_save_spi,
> > > > > +    .fields = (VMStateField[]) {
> > > > > +        VMSTATE_BUFFER(tpm_state.buffer, TpmTisSpiState),
> > > > > +        VMSTATE_UINT16(tpm_state.rw_offset, TpmTisSpiState),
> > > > > +        VMSTATE_UINT8(tpm_state.active_locty, TpmTisSpiState),
> > > > > +        VMSTATE_UINT8(tpm_state.aborting_locty, TpmTisSpiState),
> > > > > +        VMSTATE_UINT8(tpm_state.next_locty, TpmTisSpiState),
> > > > > +
> > > > > +        VMSTATE_STRUCT_ARRAY(tpm_state.loc, TpmTisSpiState, TPM_TIS_NUM_LOCALITIES,
> > > > > +                             0, vmstate_locty, TPMLocality),
> > > > > +
> > > > > +        VMSTATE_END_OF_LIST()
> > > > > +    }
> > > > > +};
> > > > > +
> > > > > +static void tpm_tis_spi_request_completed(TPMIf *ti, int ret)
> > > > > +{
> > > > > +    TpmTisSpiState *sbdev = TPM_TIS_SPI(ti);
> > > > > +    TPMState *s = &sbdev->tpm_state;
> > > > > +
> > > > > +    tpm_tis_request_completed(s, ret);
> > > > > +}
> > > > > +
> > > > > +static enum TPMVersion tpm_tis_spi_get_tpm_version(TPMIf *ti)
> > > > > +{
> > > > > +    TpmTisSpiState *sbdev = TPM_TIS_SPI(ti);
> > > > > +    TPMState *s = &sbdev->tpm_state;
> > > > > +
> > > > > +    return tpm_tis_get_tpm_version(s);
> > > > > +}
> > > > > +
> > > > > +static void tpm_tis_spi_reset(DeviceState *dev)
> > > > > +{
> > > > > +    TpmTisSpiState *sbdev = TPM_TIS_SPI(dev);
> > > > > +    TPMState *s = &sbdev->tpm_state;
> > > > > +
> > > > > +    return tpm_tis_reset(s);
> > > > > +}
> > > > > +
> > > > > +static Property tpm_tis_spi_properties[] = {
> > > > > +    DEFINE_PROP_UINT32("irq", TpmTisSpiState, tpm_state.irq_num, TPM_TIS_IRQ),
> > > > > +    DEFINE_PROP_TPMBE("tpmdev", TpmTisSpiState, tpm_state.be_driver),
> > > > > +    DEFINE_PROP_BOOL("ppi", TpmTisSpiState, tpm_state.ppi_enabled, false),
> > > > > +    DEFINE_PROP_END_OF_LIST(),
> > > > > +};
> > > > > +
> > > > > +static void tpm_tis_spi_realizefn(SSIPeripheral *ss, Error **errp)
> > > > > +{
> > > > > +    TpmTisSpiState *sbdev = TPM_TIS_SPI(ss);
> > > > > +
> > > > > +    if (!tpm_find()) {
> > > > > +        error_setg(errp, "at most one TPM device is permitted");
> > > > > +        return;
> > > > > +    }
> > > > > +
> > > > > +    if (!sbdev->tpm_state.be_driver) {
> > > > > +        error_setg(errp, "'tpmdev' property is required");
> > > > > +        return;
> > > > > +    }
> > > > > +
> > > > > +    DeviceState *spi_gpio = qdev_find_recursive(sysbus_get_default(),
> > > > > +                                                TYPE_SPI_GPIO);
> > > > > +    qdev_connect_gpio_out_named(spi_gpio,
> > > > > +                                "SPI_CS_out", 0,
> > > > > +                                qdev_get_gpio_in_named(DEVICE(ss),
> > > > > +                                SSI_GPIO_CS, 0));
> > > > Typically, this part is done at the machine/board level because it
> > > > has knowledge of all devices but it is possible with the TPM devices?
> > >
> > > +1, yeah this part should be removed.
> > >
> > > >
> > > > How do you invoke QEMU ?
> > >
> > > We have some hard-coded machine/board level code that wires up stuff,
> > > and then I think Iris was hard-coding this stuff in the TPM TIS SPI
> > > model so that the whole thing could be hidden behind a Kconfig and wired
> > > in automatically when the Kconfig was enabled. But we should require
> > > users to wire it up themselves in the board code.
> > >
> > > >
> > > > Thanks,
> > > >
> > > > C.
> > > >
> > > > > +}
> > > > > +
> > > > > +static void tpm_tis_spi_class_init(ObjectClass *klass, void *data)
> > > > > +{
> > > > > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > > > > +    SSIPeripheralClass *k = SSI_PERIPHERAL_CLASS(klass);
> > > > > +    TPMIfClass *tc = TPM_IF_CLASS(klass);
> > > > > +
> > > > > +    device_class_set_props(dc, tpm_tis_spi_properties);
> > > > > +    k->realize = tpm_tis_spi_realizefn;
> > > > > +    k->transfer = tpm_tis_spi_transfer8_ex;
> > > > > +    k->set_cs = tpm_tis_spi_cs;
> > > > > +    k->cs_polarity = SSI_CS_LOW;
> > > > > +
> > > > > +    dc->vmsd  = &vmstate_tpm_tis_spi;
> > > > > +    tc->model = TPM_MODEL_TPM_TIS;
> > > > > +    dc->user_creatable = true;
> > > > > +    dc->reset = tpm_tis_spi_reset;
> > > > > +    tc->request_completed = tpm_tis_spi_request_completed;
> > > > > +    tc->get_version = tpm_tis_spi_get_tpm_version;
> > > > > +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> > > > > +}
> > > > > +
> > > > > +static const TypeInfo tpm_tis_spi_info = {
> > > > > +    .name = TYPE_TPM_TIS_SPI,
> > > > > +    .parent = TYPE_SSI_PERIPHERAL,
> > > > > +    .instance_size = sizeof(TpmTisSpiState),
> > > > > +    .class_size = sizeof(TpmTisSpiClass),
> > > > > +    .class_init  = tpm_tis_spi_class_init,
> > > > > +    .interfaces = (InterfaceInfo[]) {
> > > > > +        { TYPE_TPM_IF },
> > > > > +        { }
> > > > > +    }
> > > > > +};
> > > > > +
> > > > > +static void tpm_tis_spi_register(void)
> > > > > +{
> > > > > +    type_register_static(&tpm_tis_spi_info);
> > > > > +}
> > > > > +
> > > > > +type_init(tpm_tis_spi_register)
> > > > > diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
> > > > > index fb40e30ff6..6a6b311e47 100644
> > > > > --- a/include/sysemu/tpm.h
> > > > > +++ b/include/sysemu/tpm.h
> > > > > @@ -46,6 +46,7 @@ struct TPMIfClass {
> > > > >   #define TYPE_TPM_TIS_ISA            "tpm-tis"
> > > > >   #define TYPE_TPM_TIS_SYSBUS         "tpm-tis-device"
> > > > > +#define TYPE_TPM_TIS_SPI            "tpm-tis-spi-device"
> > > > >   #define TYPE_TPM_CRB                "tpm-crb"
> > > > >   #define TYPE_TPM_SPAPR              "tpm-spapr"
> > > > > @@ -53,6 +54,8 @@ struct TPMIfClass {
> > > > >       object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_ISA)
> > > > >   #define TPM_IS_TIS_SYSBUS(chr)                      \
> > > > >       object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SYSBUS)
> > > > > +#define TPM_IS_TIS_SPI(chr)                      \
> > > > > +    object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SPI)
> > > > >   #define TPM_IS_CRB(chr)                             \
> > > > >       object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB)
> > > > >   #define TPM_IS_SPAPR(chr)                           \
> > > >

Re: [RFC 1/1] hw: tpmtisspi: add SPI support to QEMU TPM implementation

[Stefan Berger]

On 8/3/22 04:52, Cédric Le Goater wrote:
> On 8/3/22 04:32, Iris Chen wrote:
>> From: Iris Chen <irischenlj@fb.com>
> 
> A commit log telling us about this new device would be good to have.
> 
> 
>> Signed-off-by: Iris Chen <irischenlj@fb.com>
>> ---
>>   configs/devices/arm-softmmu/default.mak |   1 +
>>   hw/arm/Kconfig                          |   5 +
>>   hw/tpm/Kconfig                          |   5 +
>>   hw/tpm/meson.build                      |   1 +
>>   hw/tpm/tpm_tis_spi.c                    | 311 ++++++++++++++++++++++++
>>   include/sysemu/tpm.h                    |   3 +
>>   6 files changed, 326 insertions(+)
>>   create mode 100644 hw/tpm/tpm_tis_spi.c
>>
>> diff --git a/configs/devices/arm-softmmu/default.mak 
>> b/configs/devices/arm-softmmu/default.mak
>> index 6985a25377..80d2841568 100644
>> --- a/configs/devices/arm-softmmu/default.mak
>> +++ b/configs/devices/arm-softmmu/default.mak
>> @@ -42,3 +42,4 @@ CONFIG_FSL_IMX6UL=y
>>   CONFIG_SEMIHOSTING=y
>>   CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y
>>   CONFIG_ALLWINNER_H3=y
>> +CONFIG_FBOBMC_AST=y
> 
> I don't think this extra config is useful for now
> 
>> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
>> index 15fa79afd3..193decaec1 100644
>> --- a/hw/arm/Kconfig
>> +++ b/hw/arm/Kconfig
>> @@ -458,6 +458,11 @@ config ASPEED_SOC
>>       select PMBUS
>>       select MAX31785
>> +config FBOBMC_AST
>> +    bool
>> +    select ASPEED_SOC
>> +    select TPM_TIS_SPI
>> +
>>   config MPS2
>>       bool
>>       imply I2C_DEVICES
>> diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
>> index 29e82f3c92..370a43f045 100644
>> --- a/hw/tpm/Kconfig
>> +++ b/hw/tpm/Kconfig
>> @@ -8,6 +8,11 @@ config TPM_TIS_SYSBUS
>>       depends on TPM
>>       select TPM_TIS
>> +config TPM_TIS_SPI
>> +    bool
>> +    depends on TPM
>> +    select TPM_TIS
>> +
>>   config TPM_TIS
>>       bool
>>       depends on TPM
>> diff --git a/hw/tpm/meson.build b/hw/tpm/meson.build
>> index 1c68d81d6a..1a057f4e36 100644
>> --- a/hw/tpm/meson.build
>> +++ b/hw/tpm/meson.build
>> @@ -2,6 +2,7 @@ softmmu_ss.add(when: 'CONFIG_TPM_TIS', if_true: 
>> files('tpm_tis_common.c'))
>>   softmmu_ss.add(when: 'CONFIG_TPM_TIS_ISA', if_true: 
>> files('tpm_tis_isa.c'))
>>   softmmu_ss.add(when: 'CONFIG_TPM_TIS_SYSBUS', if_true: 
>> files('tpm_tis_sysbus.c'))
>>   softmmu_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_crb.c'))
>> +softmmu_ss.add(when: 'CONFIG_TPM_TIS_SPI', if_true: 
>> files('tpm_tis_spi.c'))
>>   specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_TIS'], if_true: 
>> files('tpm_ppi.c'))
>>   specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_CRB'], if_true: 
>> files('tpm_ppi.c'))
>> diff --git a/hw/tpm/tpm_tis_spi.c b/hw/tpm/tpm_tis_spi.c
>> new file mode 100644
>> index 0000000000..c98ddcfddb
>> --- /dev/null
>> +++ b/hw/tpm/tpm_tis_spi.c
>> @@ -0,0 +1,311 @@
>> +#include "qemu/osdep.h"
>> +#include "hw/qdev-properties.h"
>> +#include "migration/vmstate.h"
>> +#include "hw/acpi/tpm.h"
>> +#include "tpm_prop.h"
>> +#include "tpm_tis.h"
>> +#include "qom/object.h"
>> +#include "hw/ssi/ssi.h"
>> +#include "hw/ssi/spi_gpio.h"
>> +
>> +#define TPM_TIS_SPI_ADDR_BYTES 3
>> +#define SPI_WRITE 0
>> +
>> +typedef enum {
>> +    TIS_SPI_PKT_STATE_DEACTIVATED = 0,
>> +    TIS_SPI_PKT_STATE_START,
>> +    TIS_SPI_PKT_STATE_ADDRESS,
>> +    TIS_SPI_PKT_STATE_DATA_WR,
>> +    TIS_SPI_PKT_STATE_DATA_RD,
>> +    TIS_SPI_PKT_STATE_DONE,
>> +} TpmTisSpiPktState;
>> +
>> +union TpmTisRWSizeByte {
>> +    uint8_t byte;
>> +    struct {
>> +        uint8_t data_expected_size:6;
>> +        uint8_t resv:1;
>> +        uint8_t rwflag:1;
>> +    };
>> +};
>> +
>> +union TpmTisSpiHwAddr {
>> +    hwaddr addr;
>> +    uint8_t bytes[sizeof(hwaddr)];
>> +};
>> +
>> +union TpmTisSpiData {
>> +    uint32_t data;
>> +    uint8_t bytes[64];
>> +};
>> +
>> +struct TpmTisSpiState {
>> +    /*< private >*/
>> +    SSIPeripheral parent_obj;
>> +
>> +    /*< public >*/
>> +    TPMState tpm_state; /* not a QOM object */
>> +    TpmTisSpiPktState tpm_tis_spi_state;
>> +
>> +    union TpmTisRWSizeByte first_byte;
>> +    union TpmTisSpiHwAddr addr;
>> +    union TpmTisSpiData data;
> 
> Are these device registers ? I am not sure the unions are very useful.


> 
>> +    uint32_t data_size;
>> +    uint8_t data_idx;
>> +    uint8_t addr_idx; >> +};

I suppose that these registers will also have to be stored as part of 
the device state (for suspend/resume).

>> +/*
>> + * Pre-reading logic for transfer:
>> + * This is to fix the transaction between reading and writing.
>> + * The first byte is arbitrarily inserted so we need to
>> + * shift the all the output bytes (timeline) one byte right.

-> shift all the output bytes (timeline) one byte to the right

>> +
>> +static void tpm_tis_spi_realizefn(SSIPeripheral *ss, Error **errp)
>> +{
>> +    TpmTisSpiState *sbdev = TPM_TIS_SPI(ss);
>> +
>> +    if (!tpm_find()) {
>> +        error_setg(errp, "at most one TPM device is permitted");
>> +        return;
>> +    }
>> +
>> +    if (!sbdev->tpm_state.be_driver) {
>> +        error_setg(errp, "'tpmdev' property is required");
>> +        return;
>> +    }
>> +
>> +    DeviceState *spi_gpio = qdev_find_recursive(sysbus_get_default(),
>> +                                                TYPE_SPI_GPIO);
>> +    qdev_connect_gpio_out_named(spi_gpio,
>> +                                "SPI_CS_out", 0,
>> +                                qdev_get_gpio_in_named(DEVICE(ss),
>> +                                SSI_GPIO_CS, 0));
> Typically, this part is done at the machine/board level because it
> has knowledge of all devices but it is possible with the TPM devices?
> 
> How do you invoke QEMU ?

I you could extend the documentation for how to start QEMU with this 
device I think that would be very helpful: docs/specs/tpm.rst.

Did you get Linux to work with it? Poeple are particularly interested in 
getting Windows 11 on ARM to use a TPM 2: 
https://github.com/stefanberger/swtpm/issues/493

Thanks,

    Stefan

> 
> Thanks,
> 
> C.
> 
>> +}
>> +
>> +static void tpm_tis_spi_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    SSIPeripheralClass *k = SSI_PERIPHERAL_CLASS(klass);
>> +    TPMIfClass *tc = TPM_IF_CLASS(klass);
>> +
>> +    device_class_set_props(dc, tpm_tis_spi_properties);
>> +    k->realize = tpm_tis_spi_realizefn;
>> +    k->transfer = tpm_tis_spi_transfer8_ex;
>> +    k->set_cs = tpm_tis_spi_cs;
>> +    k->cs_polarity = SSI_CS_LOW;
>> +
>> +    dc->vmsd  = &vmstate_tpm_tis_spi;
>> +    tc->model = TPM_MODEL_TPM_TIS;
>> +    dc->user_creatable = true;
>> +    dc->reset = tpm_tis_spi_reset;
>> +    tc->request_completed = tpm_tis_spi_request_completed;
>> +    tc->get_version = tpm_tis_spi_get_tpm_version;
>> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>> +}
>> +
>> +static const TypeInfo tpm_tis_spi_info = {
>> +    .name = TYPE_TPM_TIS_SPI,
>> +    .parent = TYPE_SSI_PERIPHERAL,
>> +    .instance_size = sizeof(TpmTisSpiState),
>> +    .class_size = sizeof(TpmTisSpiClass),
>> +    .class_init  = tpm_tis_spi_class_init,
>> +    .interfaces = (InterfaceInfo[]) {
>> +        { TYPE_TPM_IF },
>> +        { }
>> +    }
>> +};
>> +
>> +static void tpm_tis_spi_register(void)
>> +{
>> +    type_register_static(&tpm_tis_spi_info);
>> +}
>> +
>> +type_init(tpm_tis_spi_register)
>> diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
>> index fb40e30ff6..6a6b311e47 100644
>> --- a/include/sysemu/tpm.h
>> +++ b/include/sysemu/tpm.h
>> @@ -46,6 +46,7 @@ struct TPMIfClass {
>>   #define TYPE_TPM_TIS_ISA            "tpm-tis"
>>   #define TYPE_TPM_TIS_SYSBUS         "tpm-tis-device"
>> +#define TYPE_TPM_TIS_SPI            "tpm-tis-spi-device"
>>   #define TYPE_TPM_CRB                "tpm-crb"
>>   #define TYPE_TPM_SPAPR              "tpm-spapr"
>> @@ -53,6 +54,8 @@ struct TPMIfClass {
>>       object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_ISA)
>>   #define TPM_IS_TIS_SYSBUS(chr)                      \
>>       object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SYSBUS)
>> +#define TPM_IS_TIS_SPI(chr)                      \
>> +    object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SPI)
>>   #define TPM_IS_CRB(chr)                             \
>>       object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB)
>>   #define TPM_IS_SPAPR(chr)                           \
> 

Re: [RFC 1/1] hw: tpmtisspi: add SPI support to QEMU TPM implementation

[Stefan Berger]

On 8/3/22 04:52, Cédric Le Goater wrote:
> On 8/3/22 04:32, Iris Chen wrote:
>> From: Iris Chen <irischenlj@fb.com>
> 

>> +++ b/hw/tpm/tpm_tis_spi.c
>> @@ -0,0 +1,311 @@
>> +#include "qemu/osdep.h"
>> +#include "hw/qdev-properties.h"
>> +#include "migration/vmstate.h"
>> +#include "hw/acpi/tpm.h"
>> +#include "tpm_prop.h"
>> +#include "tpm_tis.h"
>> +#include "qom/object.h"
>> +#include "hw/ssi/ssi.h"
>> +#include "hw/ssi/spi_gpio.h"
>> +
>> +#define TPM_TIS_SPI_ADDR_BYTES 3
>> +#define SPI_WRITE 0
>> +
>> +typedef enum {
>> +    TIS_SPI_PKT_STATE_DEACTIVATED = 0,
>> +    TIS_SPI_PKT_STATE_START,
>> +    TIS_SPI_PKT_STATE_ADDRESS,
>> +    TIS_SPI_PKT_STATE_DATA_WR,
>> +    TIS_SPI_PKT_STATE_DATA_RD,
>> +    TIS_SPI_PKT_STATE_DONE,
>> +} TpmTisSpiPktState;
>> +
>> +union TpmTisRWSizeByte {
>> +    uint8_t byte;
>> +    struct {
>> +        uint8_t data_expected_size:6;
>> +        uint8_t resv:1;
>> +        uint8_t rwflag:1;
>> +    };

I think it would be better to define a mask for the number of bytes and 
a flag for read/write rather than using bitfields. It should better for 
portability.

    Stefan

Re: [RFC 1/1] hw: tpmtisspi: add SPI support to QEMU TPM implementation

[Guenter Roeck]

Hi,

On Tue, Aug 02, 2022 at 07:32:41PM -0700, Iris Chen wrote:
> From: Iris Chen <irischenlj@fb.com>
> 
> Signed-off-by: Iris Chen <irischenlj@fb.com>
> ---

Are there any plans to submit a new version of this patch ?

Thanks,
Guenter

>  configs/devices/arm-softmmu/default.mak |   1 +
>  hw/arm/Kconfig                          |   5 +
>  hw/tpm/Kconfig                          |   5 +
>  hw/tpm/meson.build                      |   1 +
>  hw/tpm/tpm_tis_spi.c                    | 311 ++++++++++++++++++++++++
>  include/sysemu/tpm.h                    |   3 +
>  6 files changed, 326 insertions(+)
>  create mode 100644 hw/tpm/tpm_tis_spi.c
> 
> diff --git a/configs/devices/arm-softmmu/default.mak b/configs/devices/arm-softmmu/default.mak
> index 6985a25377..80d2841568 100644
> --- a/configs/devices/arm-softmmu/default.mak
> +++ b/configs/devices/arm-softmmu/default.mak
> @@ -42,3 +42,4 @@ CONFIG_FSL_IMX6UL=y
>  CONFIG_SEMIHOSTING=y
>  CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y
>  CONFIG_ALLWINNER_H3=y
> +CONFIG_FBOBMC_AST=y
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 15fa79afd3..193decaec1 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -458,6 +458,11 @@ config ASPEED_SOC
>      select PMBUS
>      select MAX31785
>  
> +config FBOBMC_AST
> +    bool
> +    select ASPEED_SOC
> +    select TPM_TIS_SPI
> +
>  config MPS2
>      bool
>      imply I2C_DEVICES
> diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
> index 29e82f3c92..370a43f045 100644
> --- a/hw/tpm/Kconfig
> +++ b/hw/tpm/Kconfig
> @@ -8,6 +8,11 @@ config TPM_TIS_SYSBUS
>      depends on TPM
>      select TPM_TIS
>  
> +config TPM_TIS_SPI
> +    bool
> +    depends on TPM
> +    select TPM_TIS
> +
>  config TPM_TIS
>      bool
>      depends on TPM
> diff --git a/hw/tpm/meson.build b/hw/tpm/meson.build
> index 1c68d81d6a..1a057f4e36 100644
> --- a/hw/tpm/meson.build
> +++ b/hw/tpm/meson.build
> @@ -2,6 +2,7 @@ softmmu_ss.add(when: 'CONFIG_TPM_TIS', if_true: files('tpm_tis_common.c'))
>  softmmu_ss.add(when: 'CONFIG_TPM_TIS_ISA', if_true: files('tpm_tis_isa.c'))
>  softmmu_ss.add(when: 'CONFIG_TPM_TIS_SYSBUS', if_true: files('tpm_tis_sysbus.c'))
>  softmmu_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_crb.c'))
> +softmmu_ss.add(when: 'CONFIG_TPM_TIS_SPI', if_true: files('tpm_tis_spi.c'))
>  
>  specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_TIS'], if_true: files('tpm_ppi.c'))
>  specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_CRB'], if_true: files('tpm_ppi.c'))
> diff --git a/hw/tpm/tpm_tis_spi.c b/hw/tpm/tpm_tis_spi.c
> new file mode 100644
> index 0000000000..c98ddcfddb
> --- /dev/null
> +++ b/hw/tpm/tpm_tis_spi.c
> @@ -0,0 +1,311 @@
> +#include "qemu/osdep.h"
> +#include "hw/qdev-properties.h"
> +#include "migration/vmstate.h"
> +#include "hw/acpi/tpm.h"
> +#include "tpm_prop.h"
> +#include "tpm_tis.h"
> +#include "qom/object.h"
> +#include "hw/ssi/ssi.h"
> +#include "hw/ssi/spi_gpio.h"
> +
> +#define TPM_TIS_SPI_ADDR_BYTES 3
> +#define SPI_WRITE 0
> +
> +typedef enum {
> +    TIS_SPI_PKT_STATE_DEACTIVATED = 0,
> +    TIS_SPI_PKT_STATE_START,
> +    TIS_SPI_PKT_STATE_ADDRESS,
> +    TIS_SPI_PKT_STATE_DATA_WR,
> +    TIS_SPI_PKT_STATE_DATA_RD,
> +    TIS_SPI_PKT_STATE_DONE,
> +} TpmTisSpiPktState;
> +
> +union TpmTisRWSizeByte {
> +    uint8_t byte;
> +    struct {
> +        uint8_t data_expected_size:6;
> +        uint8_t resv:1;
> +        uint8_t rwflag:1;
> +    };
> +};
> +
> +union TpmTisSpiHwAddr {
> +    hwaddr addr;
> +    uint8_t bytes[sizeof(hwaddr)];
> +};
> +
> +union TpmTisSpiData {
> +    uint32_t data;
> +    uint8_t bytes[64];
> +};
> +
> +struct TpmTisSpiState {
> +    /*< private >*/
> +    SSIPeripheral parent_obj;
> +
> +    /*< public >*/
> +    TPMState tpm_state; /* not a QOM object */
> +    TpmTisSpiPktState tpm_tis_spi_state;
> +
> +    union TpmTisRWSizeByte first_byte;
> +    union TpmTisSpiHwAddr addr;
> +    union TpmTisSpiData data;
> +
> +    uint32_t data_size;
> +    uint8_t data_idx;
> +    uint8_t addr_idx;
> +};
> +
> +struct TpmTisSpiClass {
> +    SSIPeripheralClass parent_class;
> +};
> +
> +OBJECT_DECLARE_TYPE(TpmTisSpiState, TpmTisSpiClass, TPM_TIS_SPI)
> +
> +static void tpm_tis_spi_mmio_read(TpmTisSpiState *tts)
> +{
> +    uint16_t offset = tts->addr.addr & 0xffc;
> +
> +    switch (offset) {
> +    case TPM_TIS_REG_DATA_FIFO:
> +        for (uint8_t i = 0; i < tts->data_size; i++) {
> +            tts->data.bytes[i] = (uint8_t)tpm_tis_memory_ops.read(
> +                                          &tts->tpm_state,
> +                                          tts->addr.addr,
> +                                          1);
> +        }
> +        break;
> +    default:
> +        tts->data.data = (uint32_t)tpm_tis_memory_ops.read(
> +                                   &tts->tpm_state,
> +                                   tts->addr.addr,
> +                                   tts->data_size);
> +    }
> +}
> +
> +static void tpm_tis_spi_mmio_write(TpmTisSpiState *tts)
> +{
> +    uint16_t offset = tts->addr.addr & 0xffc;
> +
> +    switch (offset) {
> +    case TPM_TIS_REG_DATA_FIFO:
> +        for (uint8_t i = 0; i < tts->data_size; i++) {
> +            tpm_tis_memory_ops.write(&tts->tpm_state,
> +                                     tts->addr.addr,
> +                                     tts->data.bytes[i],
> +                                     1);
> +        }
> +        break;
> +    default:
> +        tpm_tis_memory_ops.write(&tts->tpm_state,
> +                                 tts->addr.addr,
> +                                 tts->data.data,
> +                                 tts->data_size);
> +        }
> +}
> +
> +static uint32_t tpm_tis_spi_transfer8(SSIPeripheral *ss, uint32_t tx)
> +{
> +    TpmTisSpiState *tts = TPM_TIS_SPI(ss);
> +    uint32_t r = 1;
> +
> +    switch (tts->tpm_tis_spi_state) {
> +    case TIS_SPI_PKT_STATE_START:
> +        tts->first_byte.byte = (uint8_t)tx;
> +        tts->data_size = tts->first_byte.data_expected_size + 1;
> +        tts->data_idx = 0;
> +        tts->addr_idx = TPM_TIS_SPI_ADDR_BYTES;
> +        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_ADDRESS;
> +        break;
> +    case TIS_SPI_PKT_STATE_ADDRESS:
> +        assert(tts->addr_idx > 0);
> +        tts->addr.bytes[--tts->addr_idx] = (uint8_t)tx;
> +
> +        if (tts->addr_idx == 0) {
> +            if (tts->first_byte.rwflag == SPI_WRITE) {
> +                tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DATA_WR;
> +            } else { /* read and get the data ready */
> +                tpm_tis_spi_mmio_read(tts);
> +                tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DATA_RD;
> +            }
> +        }
> +        break;
> +    case TIS_SPI_PKT_STATE_DATA_WR:
> +        tts->data.bytes[tts->data_idx++] = (uint8_t)tx;
> +        if (tts->data_idx == tts->data_size) {
> +            tpm_tis_spi_mmio_write(tts);
> +            tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DONE;
> +        }
> +        break;
> +    case TIS_SPI_PKT_STATE_DATA_RD:
> +        r = tts->data.bytes[tts->data_idx++];
> +        if (tts->data_idx == tts->data_size) {
> +            tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DONE;
> +        }
> +        break;
> +    default:
> +        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DEACTIVATED;
> +        r = (uint32_t) -1;
> +    }
> +
> +    return r;
> +}
> +
> +/*
> + * Pre-reading logic for transfer:
> + * This is to fix the transaction between reading and writing.
> + * The first byte is arbitrarily inserted so we need to
> + * shift the all the output bytes (timeline) one byte right.
> + */
> +static uint32_t tpm_tis_spi_transfer8_ex(SSIPeripheral *ss, uint32_t tx)
> +{
> +    TpmTisSpiState *tts = TPM_TIS_SPI(ss);
> +    SSIBus *ssibus = (SSIBus *)qdev_get_parent_bus(DEVICE(tts));
> +
> +    TpmTisSpiPktState prev_state = tts->tpm_tis_spi_state;
> +    uint32_t r = tpm_tis_spi_transfer8(ss, tx);
> +    TpmTisSpiPktState curr_state = tts->tpm_tis_spi_state;
> +
> +    if (ssibus->preread &&
> +       /* cmd state has changed into DATA reading state */
> +       prev_state != TIS_SPI_PKT_STATE_DATA_RD &&
> +       curr_state == TIS_SPI_PKT_STATE_DATA_RD) {
> +        r = tpm_tis_spi_transfer8(ss, 0xFF);
> +    }
> +
> +    return r;
> +}
> +
> +static int tpm_tis_spi_cs(SSIPeripheral *ss, bool select)
> +{
> +    TpmTisSpiState *tts = TPM_TIS_SPI(ss);
> +
> +    if (select) { /* cs de-assert */
> +        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DEACTIVATED;
> +    } else {
> +        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_START;
> +        tts->first_byte.byte = 0;
> +        tts->addr.addr = 0;
> +        tts->data.data = 0;
> +    }
> +
> +    return 0;
> +}
> +
> +static int tpm_tis_pre_save_spi(void *opaque)
> +{
> +    TpmTisSpiState *sbdev = opaque;
> +
> +    return tpm_tis_pre_save(&sbdev->tpm_state);
> +}
> +
> +static const VMStateDescription vmstate_tpm_tis_spi = {
> +    .name = "tpm-tis-spi",
> +    .version_id = 0,
> +    .pre_save  = tpm_tis_pre_save_spi,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_BUFFER(tpm_state.buffer, TpmTisSpiState),
> +        VMSTATE_UINT16(tpm_state.rw_offset, TpmTisSpiState),
> +        VMSTATE_UINT8(tpm_state.active_locty, TpmTisSpiState),
> +        VMSTATE_UINT8(tpm_state.aborting_locty, TpmTisSpiState),
> +        VMSTATE_UINT8(tpm_state.next_locty, TpmTisSpiState),
> +
> +        VMSTATE_STRUCT_ARRAY(tpm_state.loc, TpmTisSpiState, TPM_TIS_NUM_LOCALITIES,
> +                             0, vmstate_locty, TPMLocality),
> +
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void tpm_tis_spi_request_completed(TPMIf *ti, int ret)
> +{
> +    TpmTisSpiState *sbdev = TPM_TIS_SPI(ti);
> +    TPMState *s = &sbdev->tpm_state;
> +
> +    tpm_tis_request_completed(s, ret);
> +}
> +
> +static enum TPMVersion tpm_tis_spi_get_tpm_version(TPMIf *ti)
> +{
> +    TpmTisSpiState *sbdev = TPM_TIS_SPI(ti);
> +    TPMState *s = &sbdev->tpm_state;
> +
> +    return tpm_tis_get_tpm_version(s);
> +}
> +
> +static void tpm_tis_spi_reset(DeviceState *dev)
> +{
> +    TpmTisSpiState *sbdev = TPM_TIS_SPI(dev);
> +    TPMState *s = &sbdev->tpm_state;
> +
> +    return tpm_tis_reset(s);
> +}
> +
> +static Property tpm_tis_spi_properties[] = {
> +    DEFINE_PROP_UINT32("irq", TpmTisSpiState, tpm_state.irq_num, TPM_TIS_IRQ),
> +    DEFINE_PROP_TPMBE("tpmdev", TpmTisSpiState, tpm_state.be_driver),
> +    DEFINE_PROP_BOOL("ppi", TpmTisSpiState, tpm_state.ppi_enabled, false),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void tpm_tis_spi_realizefn(SSIPeripheral *ss, Error **errp)
> +{
> +    TpmTisSpiState *sbdev = TPM_TIS_SPI(ss);
> +
> +    if (!tpm_find()) {
> +        error_setg(errp, "at most one TPM device is permitted");
> +        return;
> +    }
> +
> +    if (!sbdev->tpm_state.be_driver) {
> +        error_setg(errp, "'tpmdev' property is required");
> +        return;
> +    }
> +
> +    DeviceState *spi_gpio = qdev_find_recursive(sysbus_get_default(),
> +                                                TYPE_SPI_GPIO);
> +    qdev_connect_gpio_out_named(spi_gpio,
> +                                "SPI_CS_out", 0,
> +                                qdev_get_gpio_in_named(DEVICE(ss),
> +                                SSI_GPIO_CS, 0));
> +}
> +
> +static void tpm_tis_spi_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    SSIPeripheralClass *k = SSI_PERIPHERAL_CLASS(klass);
> +    TPMIfClass *tc = TPM_IF_CLASS(klass);
> +
> +    device_class_set_props(dc, tpm_tis_spi_properties);
> +    k->realize = tpm_tis_spi_realizefn;
> +    k->transfer = tpm_tis_spi_transfer8_ex;
> +    k->set_cs = tpm_tis_spi_cs;
> +    k->cs_polarity = SSI_CS_LOW;
> +
> +    dc->vmsd  = &vmstate_tpm_tis_spi;
> +    tc->model = TPM_MODEL_TPM_TIS;
> +    dc->user_creatable = true;
> +    dc->reset = tpm_tis_spi_reset;
> +    tc->request_completed = tpm_tis_spi_request_completed;
> +    tc->get_version = tpm_tis_spi_get_tpm_version;
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +}
> +
> +static const TypeInfo tpm_tis_spi_info = {
> +    .name = TYPE_TPM_TIS_SPI,
> +    .parent = TYPE_SSI_PERIPHERAL,
> +    .instance_size = sizeof(TpmTisSpiState),
> +    .class_size = sizeof(TpmTisSpiClass),
> +    .class_init  = tpm_tis_spi_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_TPM_IF },
> +        { }
> +    }
> +};
> +
> +static void tpm_tis_spi_register(void)
> +{
> +    type_register_static(&tpm_tis_spi_info);
> +}
> +
> +type_init(tpm_tis_spi_register)
> diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
> index fb40e30ff6..6a6b311e47 100644
> --- a/include/sysemu/tpm.h
> +++ b/include/sysemu/tpm.h
> @@ -46,6 +46,7 @@ struct TPMIfClass {
>  
>  #define TYPE_TPM_TIS_ISA            "tpm-tis"
>  #define TYPE_TPM_TIS_SYSBUS         "tpm-tis-device"
> +#define TYPE_TPM_TIS_SPI            "tpm-tis-spi-device"
>  #define TYPE_TPM_CRB                "tpm-crb"
>  #define TYPE_TPM_SPAPR              "tpm-spapr"
>  
> @@ -53,6 +54,8 @@ struct TPMIfClass {
>      object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_ISA)
>  #define TPM_IS_TIS_SYSBUS(chr)                      \
>      object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SYSBUS)
> +#define TPM_IS_TIS_SPI(chr)                      \
> +    object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SPI)
>  #define TPM_IS_CRB(chr)                             \
>      object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB)
>  #define TPM_IS_SPAPR(chr)                           \
> -- 
> 2.30.2
> 
> 

Re: [RFC 1/1] hw: tpmtisspi: add SPI support to QEMU TPM implementation

[Iris Chen]

[-- Attachment #1: Type: text/plain, Size: 14966 bytes --]

Hi Guenter,

Not from me at this moment :)

Thanks,
Iris

On Wed, Dec 13, 2023 at 9:39 AM Guenter Roeck <linux@roeck-us.net> wrote:

> Hi,
>
> On Tue, Aug 02, 2022 at 07:32:41PM -0700, Iris Chen wrote:
> > From: Iris Chen <irischenlj@fb.com>
> >
> > Signed-off-by: Iris Chen <irischenlj@fb.com>
> > ---
>
> Are there any plans to submit a new version of this patch ?
>
> Thanks,
> Guenter
>
> >  configs/devices/arm-softmmu/default.mak |   1 +
> >  hw/arm/Kconfig                          |   5 +
> >  hw/tpm/Kconfig                          |   5 +
> >  hw/tpm/meson.build                      |   1 +
> >  hw/tpm/tpm_tis_spi.c                    | 311 ++++++++++++++++++++++++
> >  include/sysemu/tpm.h                    |   3 +
> >  6 files changed, 326 insertions(+)
> >  create mode 100644 hw/tpm/tpm_tis_spi.c
> >
> > diff --git a/configs/devices/arm-softmmu/default.mak
> b/configs/devices/arm-softmmu/default.mak
> > index 6985a25377..80d2841568 100644
> > --- a/configs/devices/arm-softmmu/default.mak
> > +++ b/configs/devices/arm-softmmu/default.mak
> > @@ -42,3 +42,4 @@ CONFIG_FSL_IMX6UL=y
> >  CONFIG_SEMIHOSTING=y
> >  CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y
> >  CONFIG_ALLWINNER_H3=y
> > +CONFIG_FBOBMC_AST=y
> > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> > index 15fa79afd3..193decaec1 100644
> > --- a/hw/arm/Kconfig
> > +++ b/hw/arm/Kconfig
> > @@ -458,6 +458,11 @@ config ASPEED_SOC
> >      select PMBUS
> >      select MAX31785
> >
> > +config FBOBMC_AST
> > +    bool
> > +    select ASPEED_SOC
> > +    select TPM_TIS_SPI
> > +
> >  config MPS2
> >      bool
> >      imply I2C_DEVICES
> > diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
> > index 29e82f3c92..370a43f045 100644
> > --- a/hw/tpm/Kconfig
> > +++ b/hw/tpm/Kconfig
> > @@ -8,6 +8,11 @@ config TPM_TIS_SYSBUS
> >      depends on TPM
> >      select TPM_TIS
> >
> > +config TPM_TIS_SPI
> > +    bool
> > +    depends on TPM
> > +    select TPM_TIS
> > +
> >  config TPM_TIS
> >      bool
> >      depends on TPM
> > diff --git a/hw/tpm/meson.build b/hw/tpm/meson.build
> > index 1c68d81d6a..1a057f4e36 100644
> > --- a/hw/tpm/meson.build
> > +++ b/hw/tpm/meson.build
> > @@ -2,6 +2,7 @@ softmmu_ss.add(when: 'CONFIG_TPM_TIS', if_true:
> files('tpm_tis_common.c'))
> >  softmmu_ss.add(when: 'CONFIG_TPM_TIS_ISA', if_true:
> files('tpm_tis_isa.c'))
> >  softmmu_ss.add(when: 'CONFIG_TPM_TIS_SYSBUS', if_true:
> files('tpm_tis_sysbus.c'))
> >  softmmu_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_crb.c'))
> > +softmmu_ss.add(when: 'CONFIG_TPM_TIS_SPI', if_true:
> files('tpm_tis_spi.c'))
> >
> >  specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_TIS'], if_true:
> files('tpm_ppi.c'))
> >  specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_CRB'], if_true:
> files('tpm_ppi.c'))
> > diff --git a/hw/tpm/tpm_tis_spi.c b/hw/tpm/tpm_tis_spi.c
> > new file mode 100644
> > index 0000000000..c98ddcfddb
> > --- /dev/null
> > +++ b/hw/tpm/tpm_tis_spi.c
> > @@ -0,0 +1,311 @@
> > +#include "qemu/osdep.h"
> > +#include "hw/qdev-properties.h"
> > +#include "migration/vmstate.h"
> > +#include "hw/acpi/tpm.h"
> > +#include "tpm_prop.h"
> > +#include "tpm_tis.h"
> > +#include "qom/object.h"
> > +#include "hw/ssi/ssi.h"
> > +#include "hw/ssi/spi_gpio.h"
> > +
> > +#define TPM_TIS_SPI_ADDR_BYTES 3
> > +#define SPI_WRITE 0
> > +
> > +typedef enum {
> > +    TIS_SPI_PKT_STATE_DEACTIVATED = 0,
> > +    TIS_SPI_PKT_STATE_START,
> > +    TIS_SPI_PKT_STATE_ADDRESS,
> > +    TIS_SPI_PKT_STATE_DATA_WR,
> > +    TIS_SPI_PKT_STATE_DATA_RD,
> > +    TIS_SPI_PKT_STATE_DONE,
> > +} TpmTisSpiPktState;
> > +
> > +union TpmTisRWSizeByte {
> > +    uint8_t byte;
> > +    struct {
> > +        uint8_t data_expected_size:6;
> > +        uint8_t resv:1;
> > +        uint8_t rwflag:1;
> > +    };
> > +};
> > +
> > +union TpmTisSpiHwAddr {
> > +    hwaddr addr;
> > +    uint8_t bytes[sizeof(hwaddr)];
> > +};
> > +
> > +union TpmTisSpiData {
> > +    uint32_t data;
> > +    uint8_t bytes[64];
> > +};
> > +
> > +struct TpmTisSpiState {
> > +    /*< private >*/
> > +    SSIPeripheral parent_obj;
> > +
> > +    /*< public >*/
> > +    TPMState tpm_state; /* not a QOM object */
> > +    TpmTisSpiPktState tpm_tis_spi_state;
> > +
> > +    union TpmTisRWSizeByte first_byte;
> > +    union TpmTisSpiHwAddr addr;
> > +    union TpmTisSpiData data;
> > +
> > +    uint32_t data_size;
> > +    uint8_t data_idx;
> > +    uint8_t addr_idx;
> > +};
> > +
> > +struct TpmTisSpiClass {
> > +    SSIPeripheralClass parent_class;
> > +};
> > +
> > +OBJECT_DECLARE_TYPE(TpmTisSpiState, TpmTisSpiClass, TPM_TIS_SPI)
> > +
> > +static void tpm_tis_spi_mmio_read(TpmTisSpiState *tts)
> > +{
> > +    uint16_t offset = tts->addr.addr & 0xffc;
> > +
> > +    switch (offset) {
> > +    case TPM_TIS_REG_DATA_FIFO:
> > +        for (uint8_t i = 0; i < tts->data_size; i++) {
> > +            tts->data.bytes[i] = (uint8_t)tpm_tis_memory_ops.read(
> > +                                          &tts->tpm_state,
> > +                                          tts->addr.addr,
> > +                                          1);
> > +        }
> > +        break;
> > +    default:
> > +        tts->data.data = (uint32_t)tpm_tis_memory_ops.read(
> > +                                   &tts->tpm_state,
> > +                                   tts->addr.addr,
> > +                                   tts->data_size);
> > +    }
> > +}
> > +
> > +static void tpm_tis_spi_mmio_write(TpmTisSpiState *tts)
> > +{
> > +    uint16_t offset = tts->addr.addr & 0xffc;
> > +
> > +    switch (offset) {
> > +    case TPM_TIS_REG_DATA_FIFO:
> > +        for (uint8_t i = 0; i < tts->data_size; i++) {
> > +            tpm_tis_memory_ops.write(&tts->tpm_state,
> > +                                     tts->addr.addr,
> > +                                     tts->data.bytes[i],
> > +                                     1);
> > +        }
> > +        break;
> > +    default:
> > +        tpm_tis_memory_ops.write(&tts->tpm_state,
> > +                                 tts->addr.addr,
> > +                                 tts->data.data,
> > +                                 tts->data_size);
> > +        }
> > +}
> > +
> > +static uint32_t tpm_tis_spi_transfer8(SSIPeripheral *ss, uint32_t tx)
> > +{
> > +    TpmTisSpiState *tts = TPM_TIS_SPI(ss);
> > +    uint32_t r = 1;
> > +
> > +    switch (tts->tpm_tis_spi_state) {
> > +    case TIS_SPI_PKT_STATE_START:
> > +        tts->first_byte.byte = (uint8_t)tx;
> > +        tts->data_size = tts->first_byte.data_expected_size + 1;
> > +        tts->data_idx = 0;
> > +        tts->addr_idx = TPM_TIS_SPI_ADDR_BYTES;
> > +        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_ADDRESS;
> > +        break;
> > +    case TIS_SPI_PKT_STATE_ADDRESS:
> > +        assert(tts->addr_idx > 0);
> > +        tts->addr.bytes[--tts->addr_idx] = (uint8_t)tx;
> > +
> > +        if (tts->addr_idx == 0) {
> > +            if (tts->first_byte.rwflag == SPI_WRITE) {
> > +                tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DATA_WR;
> > +            } else { /* read and get the data ready */
> > +                tpm_tis_spi_mmio_read(tts);
> > +                tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DATA_RD;
> > +            }
> > +        }
> > +        break;
> > +    case TIS_SPI_PKT_STATE_DATA_WR:
> > +        tts->data.bytes[tts->data_idx++] = (uint8_t)tx;
> > +        if (tts->data_idx == tts->data_size) {
> > +            tpm_tis_spi_mmio_write(tts);
> > +            tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DONE;
> > +        }
> > +        break;
> > +    case TIS_SPI_PKT_STATE_DATA_RD:
> > +        r = tts->data.bytes[tts->data_idx++];
> > +        if (tts->data_idx == tts->data_size) {
> > +            tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DONE;
> > +        }
> > +        break;
> > +    default:
> > +        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DEACTIVATED;
> > +        r = (uint32_t) -1;
> > +    }
> > +
> > +    return r;
> > +}
> > +
> > +/*
> > + * Pre-reading logic for transfer:
> > + * This is to fix the transaction between reading and writing.
> > + * The first byte is arbitrarily inserted so we need to
> > + * shift the all the output bytes (timeline) one byte right.
> > + */
> > +static uint32_t tpm_tis_spi_transfer8_ex(SSIPeripheral *ss, uint32_t tx)
> > +{
> > +    TpmTisSpiState *tts = TPM_TIS_SPI(ss);
> > +    SSIBus *ssibus = (SSIBus *)qdev_get_parent_bus(DEVICE(tts));
> > +
> > +    TpmTisSpiPktState prev_state = tts->tpm_tis_spi_state;
> > +    uint32_t r = tpm_tis_spi_transfer8(ss, tx);
> > +    TpmTisSpiPktState curr_state = tts->tpm_tis_spi_state;
> > +
> > +    if (ssibus->preread &&
> > +       /* cmd state has changed into DATA reading state */
> > +       prev_state != TIS_SPI_PKT_STATE_DATA_RD &&
> > +       curr_state == TIS_SPI_PKT_STATE_DATA_RD) {
> > +        r = tpm_tis_spi_transfer8(ss, 0xFF);
> > +    }
> > +
> > +    return r;
> > +}
> > +
> > +static int tpm_tis_spi_cs(SSIPeripheral *ss, bool select)
> > +{
> > +    TpmTisSpiState *tts = TPM_TIS_SPI(ss);
> > +
> > +    if (select) { /* cs de-assert */
> > +        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_DEACTIVATED;
> > +    } else {
> > +        tts->tpm_tis_spi_state = TIS_SPI_PKT_STATE_START;
> > +        tts->first_byte.byte = 0;
> > +        tts->addr.addr = 0;
> > +        tts->data.data = 0;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static int tpm_tis_pre_save_spi(void *opaque)
> > +{
> > +    TpmTisSpiState *sbdev = opaque;
> > +
> > +    return tpm_tis_pre_save(&sbdev->tpm_state);
> > +}
> > +
> > +static const VMStateDescription vmstate_tpm_tis_spi = {
> > +    .name = "tpm-tis-spi",
> > +    .version_id = 0,
> > +    .pre_save  = tpm_tis_pre_save_spi,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_BUFFER(tpm_state.buffer, TpmTisSpiState),
> > +        VMSTATE_UINT16(tpm_state.rw_offset, TpmTisSpiState),
> > +        VMSTATE_UINT8(tpm_state.active_locty, TpmTisSpiState),
> > +        VMSTATE_UINT8(tpm_state.aborting_locty, TpmTisSpiState),
> > +        VMSTATE_UINT8(tpm_state.next_locty, TpmTisSpiState),
> > +
> > +        VMSTATE_STRUCT_ARRAY(tpm_state.loc, TpmTisSpiState,
> TPM_TIS_NUM_LOCALITIES,
> > +                             0, vmstate_locty, TPMLocality),
> > +
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> > +static void tpm_tis_spi_request_completed(TPMIf *ti, int ret)
> > +{
> > +    TpmTisSpiState *sbdev = TPM_TIS_SPI(ti);
> > +    TPMState *s = &sbdev->tpm_state;
> > +
> > +    tpm_tis_request_completed(s, ret);
> > +}
> > +
> > +static enum TPMVersion tpm_tis_spi_get_tpm_version(TPMIf *ti)
> > +{
> > +    TpmTisSpiState *sbdev = TPM_TIS_SPI(ti);
> > +    TPMState *s = &sbdev->tpm_state;
> > +
> > +    return tpm_tis_get_tpm_version(s);
> > +}
> > +
> > +static void tpm_tis_spi_reset(DeviceState *dev)
> > +{
> > +    TpmTisSpiState *sbdev = TPM_TIS_SPI(dev);
> > +    TPMState *s = &sbdev->tpm_state;
> > +
> > +    return tpm_tis_reset(s);
> > +}
> > +
> > +static Property tpm_tis_spi_properties[] = {
> > +    DEFINE_PROP_UINT32("irq", TpmTisSpiState, tpm_state.irq_num,
> TPM_TIS_IRQ),
> > +    DEFINE_PROP_TPMBE("tpmdev", TpmTisSpiState, tpm_state.be_driver),
> > +    DEFINE_PROP_BOOL("ppi", TpmTisSpiState, tpm_state.ppi_enabled,
> false),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > +static void tpm_tis_spi_realizefn(SSIPeripheral *ss, Error **errp)
> > +{
> > +    TpmTisSpiState *sbdev = TPM_TIS_SPI(ss);
> > +
> > +    if (!tpm_find()) {
> > +        error_setg(errp, "at most one TPM device is permitted");
> > +        return;
> > +    }
> > +
> > +    if (!sbdev->tpm_state.be_driver) {
> > +        error_setg(errp, "'tpmdev' property is required");
> > +        return;
> > +    }
> > +
> > +    DeviceState *spi_gpio = qdev_find_recursive(sysbus_get_default(),
> > +                                                TYPE_SPI_GPIO);
> > +    qdev_connect_gpio_out_named(spi_gpio,
> > +                                "SPI_CS_out", 0,
> > +                                qdev_get_gpio_in_named(DEVICE(ss),
> > +                                SSI_GPIO_CS, 0));
> > +}
> > +
> > +static void tpm_tis_spi_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +    SSIPeripheralClass *k = SSI_PERIPHERAL_CLASS(klass);
> > +    TPMIfClass *tc = TPM_IF_CLASS(klass);
> > +
> > +    device_class_set_props(dc, tpm_tis_spi_properties);
> > +    k->realize = tpm_tis_spi_realizefn;
> > +    k->transfer = tpm_tis_spi_transfer8_ex;
> > +    k->set_cs = tpm_tis_spi_cs;
> > +    k->cs_polarity = SSI_CS_LOW;
> > +
> > +    dc->vmsd  = &vmstate_tpm_tis_spi;
> > +    tc->model = TPM_MODEL_TPM_TIS;
> > +    dc->user_creatable = true;
> > +    dc->reset = tpm_tis_spi_reset;
> > +    tc->request_completed = tpm_tis_spi_request_completed;
> > +    tc->get_version = tpm_tis_spi_get_tpm_version;
> > +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> > +}
> > +
> > +static const TypeInfo tpm_tis_spi_info = {
> > +    .name = TYPE_TPM_TIS_SPI,
> > +    .parent = TYPE_SSI_PERIPHERAL,
> > +    .instance_size = sizeof(TpmTisSpiState),
> > +    .class_size = sizeof(TpmTisSpiClass),
> > +    .class_init  = tpm_tis_spi_class_init,
> > +    .interfaces = (InterfaceInfo[]) {
> > +        { TYPE_TPM_IF },
> > +        { }
> > +    }
> > +};
> > +
> > +static void tpm_tis_spi_register(void)
> > +{
> > +    type_register_static(&tpm_tis_spi_info);
> > +}
> > +
> > +type_init(tpm_tis_spi_register)
> > diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
> > index fb40e30ff6..6a6b311e47 100644
> > --- a/include/sysemu/tpm.h
> > +++ b/include/sysemu/tpm.h
> > @@ -46,6 +46,7 @@ struct TPMIfClass {
> >
> >  #define TYPE_TPM_TIS_ISA            "tpm-tis"
> >  #define TYPE_TPM_TIS_SYSBUS         "tpm-tis-device"
> > +#define TYPE_TPM_TIS_SPI            "tpm-tis-spi-device"
> >  #define TYPE_TPM_CRB                "tpm-crb"
> >  #define TYPE_TPM_SPAPR              "tpm-spapr"
> >
> > @@ -53,6 +54,8 @@ struct TPMIfClass {
> >      object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_ISA)
> >  #define TPM_IS_TIS_SYSBUS(chr)                      \
> >      object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SYSBUS)
> > +#define TPM_IS_TIS_SPI(chr)                      \
> > +    object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SPI)
> >  #define TPM_IS_CRB(chr)                             \
> >      object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB)
> >  #define TPM_IS_SPAPR(chr)                           \
> > --
> > 2.30.2
> >
> >
>

[-- Attachment #2: Type: text/html, Size: 19189 bytes --]

Re: [RFC 0/1] SPI support in QEMU TPM

[Cédric Le Goater]

[ Adding Stefan, TPM maintainer ]

On 8/3/22 04:32, Iris Chen wrote:
> From: Iris Chen <irischenlj@fb.com>
> 
> Hey everyone,
> 
> Thanks for all your comments on the SPI GPIO model. I am working through them.
> 
> As for adding support for SPI-based TPMs in QEMU, this RFC patch adds SPI
> support in the QEMU TPM implementation via tpm_tis_spi.c.
> 
> The QEMU tree already has support for two connection methods to the TPM:
> mmio (isa for x86, sysbus for arm) and “spapr”. This patch adds a new SPI
> bus implementation for the TPM. Specifically, this SPI bus implementation
> connects to the Yosemite-v3.5 model using the SPI-GPIO model sent earlier
> last week. I have already tested these implementations locally together
> and can verify that the Linux kernel can successfully probe the TPM device
> on Yosemite-v3.5 and we can run the TPM command line tools to interact with it.
> 
> Please let me know what you think about this!
> 
> Thanks,
> Iris
> 
> Iris Chen (1):
>    hw: tpmtisspi: add SPI support to QEMU TPM implementation
> 
>   configs/devices/arm-softmmu/default.mak |   1 +
>   hw/arm/Kconfig                          |   5 +
>   hw/tpm/Kconfig                          |   5 +
>   hw/tpm/meson.build                      |   1 +
>   hw/tpm/tpm_tis_spi.c                    | 311 ++++++++++++++++++++++++
>   include/sysemu/tpm.h                    |   3 +
>   6 files changed, 326 insertions(+)
>   create mode 100644 hw/tpm/tpm_tis_spi.c
> 
> --
> 2.30.2