On Wed, Aug 3, 2022 at 10:30 AM Peter Delevoryas <pe...@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 <irische...@fb.com>
> >
> > A commit log telling us about this new device would be good to have.
> >
> >
> > > Signed-off-by: Iris Chen <irische...@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) \
> >
