Re: [RFC v2 6/6] hw/arm/virt: vTPM support

[Auger Eric]

Hi Stefan,

On 2/16/20 7:47 PM, Stefan Berger wrote:
> On 2/14/20 1:37 PM, Eric Auger wrote:
>> Let the TPM TIS SYSBUS device be dynamically instantiable
>> in ARM virt.  A device tree node is dynamically created
>> (TPM via MMIO).
>>
>> The TPM Physical Presence interface (PPI) is not supported.
>>
>> To run with the swtmp TPM emulator, the qemu command line must
>> be augmented with:
>>
>>          -chardev socket,id=chrtpm,path=swtpm-sock \
>>          -tpmdev emulator,id=tpm0,chardev=chrtpm \
>>          -device tpm-tis-device,tpmdev=tpm0 \
>>
>> swtpm/libtpms command line example:
>>
>> swtpm socket --tpm2 -t -d --tpmstate dir=/tmp/tpm \
>> --ctrl type=unixio,path=swtpm-sock
> 
> 
> Can you also extend docs/spec/tpm.rst for the arm case?
Sure

 And the test
> cases should probably also cover the arm sysbus device.
Yes I agree. At the moment I have made sure existing x86-64 qtests were
running as before.

I will investigate what I can do to cover the sysbus device.

> 
> 
>>
>> Signed-off-by: Eric Auger <address@hidden>
>> ---
>>   hw/arm/Kconfig      |  1 +
>>   hw/arm/sysbus-fdt.c | 36 ++++++++++++++++++++++++++++++++++++
>>   hw/arm/virt.c       |  7 +++++++
>>   3 files changed, 44 insertions(+)
>>
>> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
>> index 3d86691ae0..b6f03f7f53 100644
>> --- a/hw/arm/Kconfig
>> +++ b/hw/arm/Kconfig
>> @@ -5,6 +5,7 @@ config ARM_VIRT
>>       imply VFIO_AMD_XGBE
>>       imply VFIO_PLATFORM
>>       imply VFIO_XGMAC
>> +    imply TPM_TIS_SYSBUS
>>       select A15MPCORE
>>       select ACPI
>>       select ARM_SMMUV3
>> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
>> index 022fc97ecd..adf50444c2 100644
>> --- a/hw/arm/sysbus-fdt.c
>> +++ b/hw/arm/sysbus-fdt.c
>> @@ -30,6 +30,7 @@
>>   #include "hw/arm/sysbus-fdt.h"
>>   #include "qemu/error-report.h"
>>   #include "sysemu/device_tree.h"
>> +#include "sysemu/tpm.h"
>>   #include "hw/platform-bus.h"
>>   #include "hw/vfio/vfio-platform.h"
>>   #include "hw/vfio/vfio-calxeda-xgmac.h"
>> @@ -434,6 +435,40 @@ static bool vfio_platform_match(SysBusDevice *sbdev,
>>   #define VFIO_PLATFORM_BINDING(compat, add_fn) \
>>       {TYPE_VFIO_PLATFORM, (compat), (add_fn), vfio_platform_match}
>>   +/*
>> + * add_tpm_tis_fdt_node: Create a DT node for TPM TIS
>> + *
>> + * See kernel documentation:
>> + * Documentation/devicetree/bindings/security/tpm/tpm_tis_mmio.txt
>> + * Optional interrupt for command completion is not exposed
>> + */
>> +static int add_tpm_tis_fdt_node(SysBusDevice *sbdev, void *opaque)
>> +{
>> +    PlatformBusFDTData *data = opaque;
>> +    PlatformBusDevice *pbus = data->pbus;
>> +    void *fdt = data->fdt;
>> +    const char *parent_node = data->pbus_node_name;
>> +    int compat_str_len;
>> +    char *nodename;
>> +    uint32_t reg_attr[2];
>> +    uint64_t mmio_base;
>> +
>> +    mmio_base = platform_bus_get_mmio_addr(pbus, sbdev, 0);
> 
> I suppose any conditional creation of this device tree entry is covered
> with the TYPE_BINDING below, meaning no device tree is created if the
> device wasn't added.
Yes exactly, the dt node is not created if the -device tpm-tis-device is
not passed in the qemu cmd line.
> 
> 
>> +    nodename = g_strdup_printf("%s/tpm_tis@%" PRIx64, parent_node,
>> mmio_base);
>> +    qemu_fdt_add_subnode(fdt, nodename);
>> +
>> +    compat_str_len = strlen("tcg,tpm-tis-mmio") + 1;
>> +    qemu_fdt_setprop(fdt, nodename, "compatible", "tcg,tpm-tis-mmio",
>> +                     compat_str_len);
> 
> You probably can use qemu_fdt_setprop_string()?
indeed!
> 
> 
>> +
>> +    reg_attr[0] = cpu_to_be32(mmio_base);
>> +    reg_attr[1] = cpu_to_be32(0x5000);
>> +    qemu_fdt_setprop(fdt, nodename, "reg", reg_attr, 2 *
>> sizeof(uint32_t));
>> +
>> +    g_free(nodename);
>> +    return 0;
>> +}
>> +
>>   #endif /* CONFIG_LINUX */
>>     static int no_fdt_node(SysBusDevice *sbdev, void *opaque)
>> @@ -455,6 +490,7 @@ static const BindingEntry bindings[] = {
>>       TYPE_BINDING(TYPE_VFIO_CALXEDA_XGMAC,
>> add_calxeda_midway_xgmac_fdt_node),
>>       TYPE_BINDING(TYPE_VFIO_AMD_XGBE, add_amd_xgbe_fdt_node),
>>       VFIO_PLATFORM_BINDING("amd,xgbe-seattle-v1a",
>> add_amd_xgbe_fdt_node),
>> +    TYPE_BINDING(TYPE_TPM_TIS_SYSBUS, add_tpm_tis_fdt_node),
>>   #endif
>>       TYPE_BINDING(TYPE_RAMFB_DEVICE, no_fdt_node),
>>       TYPE_BINDING("", NULL), /* last element */
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index f788fe27d6..4b967e39d1 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -47,6 +47,7 @@
>>   #include "sysemu/numa.h"
>>   #include "sysemu/runstate.h"
>>   #include "sysemu/sysemu.h"
>> +#include "sysemu/tpm.h"
>>   #include "sysemu/kvm.h"
>>   #include "hw/loader.h"
>>   #include "exec/address-spaces.h"
>> @@ -2041,6 +2042,7 @@ static void virt_machine_class_init(ObjectClass
>> *oc, void *data)
>>       machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_AMD_XGBE);
>>       machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
>>       machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_PLATFORM);
>> +    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS_SYSBUS);
>>       mc->block_default_type = IF_VIRTIO;
>>       mc->no_cdrom = 1;
>>       mc->pci_allow_0_address = true;
>> @@ -2153,6 +2155,11 @@ type_init(machvirt_machine_init);
>>     static void virt_machine_5_0_options(MachineClass *mc)
>>   {
>> +    static GlobalProperty compat[] = {
>> +        { TYPE_TPM_TIS_SYSBUS, "ppi", "false" },
>> +    };
>> +
>> +    compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));
>>   }
>>   DEFINE_VIRT_MACHINE_AS_LATEST(5, 0)
>>   
> 
> 
Thanks

Eric