Re: [Qemu-devel] [PATCH v3] tpm: add CRB device

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

On 01/29/2018 11:24 AM, Igor Mammedov wrote:
> On Fri, 26 Jan 2018 13:03:06 +0100
> Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>
>> tpm_crb is a device for TPM 2.0 Command Response Buffer (CRB)
>> Interface as defined in TCG PC Client Platform TPM Profile (PTP)
>> Specification Family “2.0” Level 00 Revision 01.03 v22.
>>
>> The PTP allows device implementation to switch between TIS and CRB
>> model at run time, but given that CRB is a simpler device to
>> implement, I chose to implement it as a different device.
>>
>> The device doesn't implement other locality than 0 for now (my laptop
>> TPM doesn't either, so I assume this isn't so bad)
>>
>> Tested with some success with Linux upstream and Windows 10, seabios &
>> modified ovmf. The device is recognized and correctly transmit
>> command/response with passthrough & emu. However, we are missing PPI
>> ACPI part atm.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>
>> The patch is based on stefanb/tpm-next git branch.
>>
> [...]
>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index dc4b2b9ffe..ed78c4ed9f 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -2224,6 +2224,22 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>>               aml_append(sb_scope, scope);
>>           }
>>       }
>> +
>> +    if (TPM_IS_CRB(tpm_find())) {
>> +        dev = aml_device("TPM");
>> +        aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101")));
>> +        crs = aml_resource_template();
>> +        aml_append(crs, aml_memory32_fixed(TPM_CRB_ADDR_BASE,
>> +                                           TPM_CRB_ADDR_SIZE, AML_READ_WRITE));
>> +        aml_append(dev, aml_name_decl("_CRS", crs));
>> +
> [...]
>> +        method = aml_method("_STA", 0, AML_NOTSERIALIZED);
>> +        aml_append(method, aml_return(aml_int(0x0f)));
>> +        aml_append(dev, method);
> this is not needed as 0x0f is assumed default value if _STA is missing

Well, it seems to be in good neighborhood with other devices created 
that do the same thing: build_kbd_device_aml(), 
build_mouse_device_aml(), part of \_SB.PCI0.ISA...

>
> [...]
>
>> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
>> new file mode 100644
>> index 0000000000..687d2557b7
>> --- /dev/null
>> +++ b/hw/tpm/tpm_crb.c
> [...]
>
>> +static void tpm_crb_realize(DeviceState *dev, Error **errp)
>> +{
>> +    CRBState *s = CRB(dev);
>> +
>> +    if (!tpm_find()) {
>> +        error_setg(errp, "at most one TPM device is permitted");
>> +        return;
>> +    }
>> +    if (!s->tpmbe) {
>> +        error_setg(errp, "'tpmdev' property is required");
>> +        return;
>> +    }
>> +
>> +    memory_region_init_io(&s->mmio, OBJECT(s), &tpm_crb_memory_ops, s,
>> +        "tpm-crb-mmio", sizeof(s->regs));
>> +    memory_region_init_ram(&s->cmdmem, OBJECT(s),
>> +        "tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp);
>> +
>> +    memory_region_add_subregion(get_system_memory(),
>> +        TPM_CRB_ADDR_BASE, &s->mmio);
>> +    memory_region_add_subregion(get_system_memory(),
>> +        TPM_CRB_ADDR_BASE + sizeof(s->regs), &s->cmdmem);
> just curious, what if there is something else mapped at this addresses,
> will it fail or just ignore error?

All these memory APIs return void...



>
>> +
>> +    tpm_backend_reset(s->tpmbe);
>> +
>> +    ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
>> +                     InterfaceType, CRB_INTF_TYPE_CRB_ACTIVE);
>> +    ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
>> +                     InterfaceVersion, CRB_INTF_VERSION_CRB);
>> +    ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
>> +                     CapLocality, CRB_INTF_CAP_LOCALITY_0_ONLY);
>> +    ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
>> +                     CapCRBIdleBypass, CRB_INTF_CAP_IDLE_FAST);
>> +    ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
>> +                     CapDataXferSizeSupport, CRB_INTF_CAP_XFER_SIZE_64);
>> +    ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
>> +                     CapFIFO, CRB_INTF_CAP_FIFO_NOT_SUPPORTED);
>> +    ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
>> +                     CapCRB, CRB_INTF_CAP_CRB_SUPPORTED);
>> +    ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
>> +                     InterfaceSelector, CRB_INTF_IF_SELECTOR_CRB);
>> +    ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
>> +                     RID, 0b0000);
>> +    ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID2,
>> +                     VID, PCI_VENDOR_ID_IBM);
>> +
>> +    s->regs[R_CRB_CTRL_CMD_SIZE] = CRB_CTRL_CMD_SIZE;
>> +    s->regs[R_CRB_CTRL_CMD_LADDR] = TPM_CRB_ADDR_BASE + A_CRB_DATA_BUFFER;
>> +    s->regs[R_CRB_CTRL_RSP_SIZE] = CRB_CTRL_CMD_SIZE;
>> +    s->regs[R_CRB_CTRL_RSP_ADDR] = TPM_CRB_ADDR_BASE + A_CRB_DATA_BUFFER;
>> +
>> +    s->be_buffer_size = MIN(tpm_backend_get_buffer_size(s->tpmbe),
>> +                            CRB_CTRL_CMD_SIZE);
>> +
>> +    tpm_backend_startup_tpm(s->tpmbe, s->be_buffer_size);
>> +}
> [...]
>