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

[Stefan Berger]

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.


> +#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.]


> 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.


>
>> +
>> +	/* current q_entry */
>> +	unsigned int curr_q_entry;
>> +
>> +	/* current response CRQ */
>> +	struct crq *response;
>> +
>> +	/* power firmware defined state and error code */
>> +	vtpm_drv_state driver_state;
>> +	vtpm_drv_error driver_error;
>> +
>> +	/* size of buffer supported by hypervisor */
>> +	unsigned int buffer_size;
>> +
>> +        /* buffer for commands and responses */
>> +        char *buffer;
>
> Indentation broke.

Fixed.


>
>> +
>> +	/* version of the TPM we talk to -- from CRQ message */
>> +	uint32_t tpm_version;
> It is always v2.0 for now.

Dropped the field but still querying the version from the HV.


>
>
>> +} spapr_vtpm = {
>> +	.qsize = QUEUE_SIZE,
>> +	.driver_state = VTPM_DRV_STATE_INVALID,
>> +	.driver_error = VTPM_DRV_ERROR_NO_FAILURE,
>> +};
>> +
>> +static void vtpm_drv_state_set(vtpm_drv_state s, vtpm_drv_error e)
>
>
> Some functions have "vtpm_" prefix, some "spapr_", some none (such as
> "get_crq").

I prefixed them with spapr_ now mostly except those two for driver state.


>
>
>> +{
>> +	spapr_vtpm.driver_state = s;
>> +	spapr_vtpm.driver_error = e;
>> +}
>> +
>> +static vtpm_drv_error vtpm_drv_error_get(void)
>> +{
>> +	return spapr_vtpm.driver_error;
>> +}
>> +
>> +static struct crq* get_crq(void *qaddr, unsigned long q_entry)
> Please s/crq* get_crq/crq *get_crq/g  as everywhere else.


Fixed it. This should have been the only occurrence.


>> +void spapr_vtpm_finalize(void)
>> +{
>> +	if (spapr_vtpm.unit)
>> +		hv_free_crq(spapr_vtpm.unit);
>
> spapr_vtpm.unit = 0; as above?

Fixed.


>
>> +}
>> +
>> +/*
>> + * Check whether we have a CRQ underneath us; if we do, the CRQ will
>> + * be left open.
>> + */
>> +static bool spapr_vtpm_probe(void)
>> +{
>> +	if (!spapr_vtpm.qaddr) {
>> +		spapr_vtpm.qaddr = SLOF_alloc_mem(spapr_vtpm.qsize);
>> +		if (!spapr_vtpm.qaddr) {
>> +			printf("%s: Unable to allocate memory\n", __func__);
>> +			return false;
>> +		}
>> +		memset(spapr_vtpm.qaddr, 0, spapr_vtpm.qsize);
>> +
>> +		dprintf("getting FORTH vtpm-unit\n");
>> +		spapr_vtpm.unit = SLOF_get_vtpm_unit();
>> +		if (!spapr_vtpm.unit) {
>> +			printf("%s: Could not get valid vtpm-unit\n", __func__);
>> +			return false;
>> +		}
>> +	}
>> +
>> +	dprintf("vtpm_unit = %lx, buffer = %p\n",
>> +		spapr_vtpm.unit, spapr_vtpm.qaddr);
>> +
>> +	if (!spapr_vtpm_activate())
>> +		return false;
>> +
>> +	return true;
>> +}
>> +
>> +static bool spapr_vtpm_senddata(const uint8_t *const data, uint32_t len)
>> +{
>> +	struct crq crq;
>> +	long rc;
>> +
>> +	if (vtpm_drv_error_get() != VTPM_DRV_ERROR_NO_FAILURE) {
>> +		printf("%s: VTPM CRQ: In failure mode\n", __func__);
>> +		return false;
>> +	}
>> +
>> +	if (len > spapr_vtpm.buffer_size) {
>> +		printf("%s: VTPM CRQ: Send buffer too large: %u > %u\n",
>> +		       __func__, len, spapr_vtpm.buffer_size);
>> +		return false;
>> +	}
>> +
>> +	spapr_vtpm.response = get_response_crq();
>> +	spapr_vtpm.response->data = (uint64_t)spapr_vtpm.buffer;
>> +	/* response CRQ has been set and valid field cleared */
>
> Where does "response CRQ has been set" happen? Is it hv_reg_crq() from
> spapr_vtpm_activate()? If that so, then I guess we can just drop all
> three "response CRQ has been set".

It happens in get_response_crq(). I dropped the comments.


> +
> +	vtpm_drv_state_set(VTPM_DRV_STATE_SEND_TPM_CMD,
> +			   VTPM_DRV_ERROR_NO_FAILURE);
> +
> +	rc = hv_send_crq(spapr_vtpm.unit, (uint64_t *)&crq);
> +
> +	if (rc == H_SUCCESS) {
> +		vtpm_drv_state_set(VTPM_DRV_STATE_WAIT_TPM_RSP,
> +				   VTPM_DRV_ERROR_NO_FAILURE);
> +	} else {
> +		vtpm_drv_state_set(VTPM_DRV_STATE_WAIT_INIT,
> +				   VTPM_DRV_ERROR_UNEXPECTED_SEND_ERROR);
> +	}
> No need in curly braces here.


Fixed.


Thanks.

    Stefan