[SLOF] [PATCH v6 4/7] tpm: Add TPM CRQ driver implementation

[Alexey Kardashevskiy]

On 21/01/2020 02:45, Stefan Berger wrote:
> On 1/20/20 3:09 AM, Alexey Kardashevskiy wrote:
>>
>> All this can be reduces to "QEMU has vTPM support since the v5.0.0
>> release", or similar.
> 
> 
> I will do that then.
> 
> 
>>
>>> +
>>> +To start a QEMU VM with an attached vTPM (swtpm), run the following
>>> commands
>>> +as 'root'.
>> Why as 'root'? Will swtpm forward these requests to an actual tpm
>> device? How is it expected to work on the real system?
> 
> 
> I will modify this to use sudo for qemu.
> 
> The hardware TPM cannot be used for virtualization so we won't forward
> anything to it.


So KVM+QEMU have to be trusted? The scope of the feature is not
extremely clear but I am learning as we speak so may be it is.


> 
> 
>> +#undef PAPR_VTPM_DEBUG
>> +//#define PAPR_VTPM_DEBUG
>> +#ifdef PAPR_VTPM_DEBUG
>> +#define dprintf(_x ...) do { printf("VTPM CRQ: " _x); } while(0)
>> +#else
>> +#define dprintf(_x ...)
>> +#endif
>> +
>> +#define MIN(a, b) ((a) > (b) ? (b) : (a))
>> +
>> +/* layout of the command request queue for vTPM */
>>
>> Is every single packed struct added by this patch defined as a big
>> endian?
> 
> Every structure interacting with the hypervisor/QEMU is defined as big
> endian and this is the only packed data structure in this patch, so yes,
> it is big endian. The rest can be native.
> 
> [TPM 2 commands are all in big endian.]


It is not that clear what structures are for TPM2 commands and which are
from the TCG specs with that little endian log and so on.


> 
> 
>> Or everything with the "tpm2_" prefix is little endian, and the rest is
>> big endian?
>>
>>
>>> +struct crq {
>>> +    uint8_t valid;
>>> +    uint8_t msg;
>>> +    uint16_t len;
>>> +    uint32_t data;
>>> +    uint64_t reserved;
>>> +} __attribute__((packed));
>>> +
>>> +#define PAPR_VTPM_INIT_CRQ_COMMAND      0xC0
>>> +#define PAPR_VTPM_VALID_COMMAND         0x80
>>> +#define PAPR_VTPM_MSG_RESULT            0x80
>>> +
>>> +/* crq.msg request types when crq.valid = PAPR_VTPM_INIT_CRQ_COMMAND */
>>> +#define PAPR_VTPM_INIT_CRQ_RESULT       0x1
>>> +
>>> +/* crq.msg request types when crq.valid = PAPR_VTPM_VALID_COMMAND */
>>> +#define PAPR_VTPM_GET_VERSION           0x1
>>> +#define PAPR_VTPM_TPM_COMMAND           0x2
>>> +#define PAPR_VTPM_GET_RTCE_BUFFER_SIZE  0x3
>>> +
>>> +#define TPM2_DEFAULT_DURATION_SHORT       750000 /* us */
>>> +#define TPM2_DEFAULT_DURATION_MEDIUM     2000000 /* us */
>>> +#define TPM2_DEFAULT_DURATION_LONG       2000000 /* us */
>>> +
>>> +static const uint32_t tpm2_durations[3] = {
>>> +    TPM2_DEFAULT_DURATION_SHORT,
>>> +    TPM2_DEFAULT_DURATION_MEDIUM,
>>> +    TPM2_DEFAULT_DURATION_LONG,
>>> +};
>>> +
>>> +#define QUEUE_SIZE 4096
>>> +
>>> +/* state of the PAPR CRQ VTPM driver */
>>> +static struct spapr_vtpm_driver_state {
>> s/spapr_vtpm_driver_state// as this name is not used anyway anywhere.
> 
> 
> I removed the name from it.
> 
> 
>>
>>
>>> +    /* whether it driver been initialized */
>>> +    bool initialized;
>>> +
>>> +    /* unit number */
>>> +    unsigned long unit;
>>> +
>>> +    /* CRQ queue address and size */
>>> +    unsigned char *qaddr;
>>> +    unsigned long qsize;
>>
>> The whole struct is static so you could simply do qaddr[QUEUE_SIZE] and
>> drop @qsize and allocation.
> 
> I have tried to do this with a 32-byte, 4096-byte and 65536-byte aligned
> queue embedded in this struct, but it's rejected by the HV when trying
> to register the CRQ. The addresses were as follows:
> 
> 32 byte: 0x7dc61e40
> 
> 4096 byte: 0x7dc64000
> 
> 65536 byte: 0x7dc80000
> 
> The allocated queue at 0x7e469000, which seems to be 4096 byte aligned,
> is the only one accepted.


Ah I see, ok. Thanks,


-- 
Alexey