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

[Stefan Berger <stefanb@linux.ibm.com>]

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)                           \
>