Re: [Qemu-devel] [PATCH v2 4/5] tpm: add CRB device

[Marc-André Lureau]

On Mon, Jan 22, 2018 at 4:47 PM, Stefan Berger
<address@hidden> wrote:
> On 01/22/2018 10:08 AM, Marc-Andre Lureau wrote:
>>
>> Hi
>>
>> On Sun, Jan 21, 2018 at 11:01 PM, Stefan Berger
>> <address@hidden> wrote:
>>>
>>> On 01/21/2018 02:24 PM, Marc-Andre Lureau wrote:
>>>>
>>>> Hi
>>>>
>>>> On Sun, Jan 21, 2018 at 6:46 AM, Stefan Berger
>>>> <address@hidden> wrote:
>>>>>
>>>>> On 01/20/2018 07:54 AM, Philippe Mathieu-Daudé wrote:
>>>>>>
>>>>>> On 01/19/2018 11:11 AM, Marc-André Lureau 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)
>>>>>>>
>>>>>>> The command/reply memory region is statically allocated after the CRB
>>>>>>> registers address TPM_CRB_ADDR_BASE + sizeof(struct crb_regs) (I
>>>>>>> wonder if the BIOS could or should allocate it instead, or what size
>>>>>>> to use, again this seems to fit well expectations)
>>>>>>>
>>>>>>> The PTP doesn't specify a particular bus to put the device. So I
>>>>>>> added
>>>>>>> it on the system bus directly, so it could hopefully be used easily
>>>>>>> on
>>>>>>> a different platform than x86. Currently, it fails to init on piix,
>>>>>>> because error_on_sysbus_device() check. The check may be changed in a
>>>>>>> near future, see discussion on the qemu-devel ML.
>>>>>>>
>>>>>>> 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 <address@hidden>
>>>>>>> Signed-off-by: Stefan Berger <address@hidden>
>>>>>>> ---
>>>>>>>     qapi/tpm.json                      |   5 +-
>>>>>>>     include/hw/acpi/tpm.h              |  72 ++++++++
>>>>>>>     include/sysemu/tpm.h               |   3 +
>>>>>>>     hw/i386/acpi-build.c               |  34 +++-
>>>>>>>     hw/tpm/tpm_crb.c                   | 327
>>>>>>> +++++++++++++++++++++++++++++++++++++
>>>>>>>     default-configs/i386-softmmu.mak   |   1 +
>>>>>>>     default-configs/x86_64-softmmu.mak |   1 +
>>>>>>>     hw/tpm/Makefile.objs               |   1 +
>>>>>>>     8 files changed, 434 insertions(+), 10 deletions(-)
>>>>>>>     create mode 100644 hw/tpm/tpm_crb.c
>>>>>>>
>>>>>>> diff --git a/qapi/tpm.json b/qapi/tpm.json
>>>>>>> index 7093f268fb..d50deef5e9 100644
>>>>>>> --- a/qapi/tpm.json
>>>>>>> +++ b/qapi/tpm.json
>>>>>>> @@ -11,10 +11,11 @@
>>>>>>>     # An enumeration of TPM models
>>>>>>>     #
>>>>>>>     # @tpm-tis: TPM TIS model
>>>>>>> +# @tpm-crb: TPM CRB model (since 2.12)
>>>>>>>     #
>>>>>>>     # Since: 1.5
>>>>>>>     ##
>>>>>>> -{ 'enum': 'TpmModel', 'data': [ 'tpm-tis' ] }
>>>>>>> +{ 'enum': 'TpmModel', 'data': [ 'tpm-tis', 'tpm-crb' ] }
>>>>>>>       ##
>>>>>>>     # @query-tpm-models:
>>>>>>> @@ -28,7 +29,7 @@
>>>>>>>     # Example:
>>>>>>>     #
>>>>>>>     # -> { "execute": "query-tpm-models" }
>>>>>>> -# <- { "return": [ "tpm-tis" ] }
>>>>>>> +# <- { "return": [ "tpm-tis", "tpm-crb" ] }
>>>>>>>     #
>>>>>>>     ##
>>>>>>>     { 'command': 'query-tpm-models', 'returns': ['TpmModel'] }
>>>>>>> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
>>>>>>> index 6d516c6a7f..b0048515fa 100644
>>>>>>> --- a/include/hw/acpi/tpm.h
>>>>>>> +++ b/include/hw/acpi/tpm.h
>>>>>>> @@ -16,11 +16,82 @@
>>>>>>>     #ifndef HW_ACPI_TPM_H
>>>>>>>     #define HW_ACPI_TPM_H
>>>>>>>     +#include "qemu/osdep.h"
>>>>>>> +
>>>>>>>     #define TPM_TIS_ADDR_BASE           0xFED40000
>>>>>>>     #define TPM_TIS_ADDR_SIZE           0x5000
>>>>>>>       #define TPM_TIS_IRQ                 5
>>>>>>>     +struct crb_regs {
>>>>>>> +    union {
>>>>>>> +        uint32_t reg;
>>>>>>> +        struct {
>>>>>>> +            unsigned tpm_established:1;
>>>>>>> +            unsigned loc_assigned:1;
>>>>>>> +            unsigned active_locality:3;
>>>>>>> +            unsigned reserved:2;
>>>>>>> +            unsigned tpm_reg_valid_sts:1;
>>>>>>> +        } bits;
>>>>>>
>>>>>> I suppose this is little-endian layout, so this won't work on
>>>>>> big-endian
>>>>>> hosts.
>>>>>>
>>>>>> Can you add a qtest?
>>>>>>
>>>>>>> +    } loc_state;
>>>>>>> +    uint32_t reserved1;
>>>>>>> +    uint32_t loc_ctrl;
>>>>>>> +    union {
>>>>>>> +        uint32_t reg;
>>>>>>> +        struct {
>>>>>>> +            unsigned granted:1;
>>>>>>> +            unsigned been_seized:1;
>>>>>>> +        } bits;
>>>>>>
>>>>>>
>>>>>> This is unclear where you expect those bits (right/left aligned).
>>>>>>
>>>>>> Can you add an unnamed field to be more explicit?
>>>>>>
>>>>>> i.e. without using struct if left alignment expected:
>>>>>>
>>>>>>               unsigned granted:1, been_seized:1, :30;
>>>>>
>>>>>
>>>>>
>>>>> I got rid of all the bitfields and this patch here makes it work on a
>>>>> ppc64
>>>>> big endian and also x86_64 host:
>>>>>
>>>>>
>>>>>
>>>>> https://github.com/stefanberger/qemu-tpm/commit/28fc07f0d9314168986190effd6d72d9fd3972dd
>>>>>
>>>> Thank you Stefan! I am all for squashing this fix to the patch. You
>>>> should then add your signed-off to the commit.
>>>
>>>
>>> I'll do that.
>>>
>>> The TIS is an ISA Device and the CRB is similar. Considering the
>>
>> How much similarity is there between TIS and CRB is there? The two
>> devices look quite different to me, CRB is way simpler it seems. Or is
>> the CRB implementation just lacking many bells and whistles that TIS
>> has? Should we consider merging CRB in TIS?
>
>
> That is the question. It would be a pain for users to have to choose the
> interface. Basically TIS and CRB can be implemented in a single device and
> let the software choose the interface following flags that are set by the
> interface advertising whether it offers TIS and/or CRB. Maybe we could keep
> the CRB code in a separate file and, upon choosing the CRB interface call
> into CRB functions rather than TIS functions.

Afaik, most people want to use TPM2, either with Linux or Windows, and
CRB fits that job better at least on Windows. So unless TPM 1.2 and
legacy OS is required, I would recommend to use CRB only.

If there is a need to grow dynamic TIS/CRB switching, I would teach
the existing TIS device to call into CRB, as you proposed. In the
meantime, I think it's best to have a simple CRB-only device that is
known to work with latest Linux & Windows.




-- 
Marc-André Lureau