[SLOF] [PATCH 01/16] Add a TPM driver implementation

[Stefan Berger]

On 11/09/2015 01:11 AM, Nikunj A Dadhania wrote:
> Stefan Berger <stefanb at linux.vnet.ibm.com> writes:
>
>> +
>> +#define dprintf(_x ...) \
>> +	if (PAPR_VTPM_DEBUG) { \
>> +		printf("VTPM CRQ: " _x); \
>> +	}
> Better one:
>
> #ifdef PAPR_VTPM_DEBUG
> #define dprintf(_x ...) do { printf(_x); } while(0)
> #else
> #define dprintf(_x ...)
> #endif

I'll adapt it to that; since usb uses the same name dprintf, I'll keep 
it at that.

>
>> +
>> +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
>> +
>> +/* msg types for valid = VALID_INIT_CRQ */
> Comment is confusing, are these return codes for
> PAPR_VTPM_INIT_CRQ_COMMAND ?

These are actually commands. I'll fix.

>> +	unsigned char *qaddr;
>> +	unsigned long qsize;
>> +	/* current q_entry */
>> +	unsigned int q_entry;
>> +	/* current response CRQ */
>> +	struct crq *response;
>> +	pfw_drv_state driver_state;
>> +	pfw_drv_error driver_error;
> what is pfw here ?

These two variables are related to Power Firmware specs, so that's where 
the prefix 'pfw' comes from.


>
>> +	/* version of the TPM we talk to -- from CRQ message */
>> +	uint32_t tpm_version;
>> +	/* buffer offset in buffer for sending */
>> +	unsigned int buffer_offset;
>> +	/* actual size of the buffer being used */
>> +	unsigned int buffer_size;
>> +	/* the buffer; may be bigger than buffer_size */
>> +	unsigned char buffer[PAPR_VTPM_MAX_BUFFER_SIZE];
>> +}
>
>> + spapr_vtpm = {
>> +	.qsize = PAGE_SIZE,
>> +	.driver_state = PFW_DRV_STATE_INVALID,
>> +	.driver_error = PFW_DRV_ERROR_NO_FAILURE,
>> +	.buffer_size = sizeof(spapr_vtpm.buffer),
>> +};
> With spapr_vtpm global you are assuming that you will only have one vtpm
> device added to the guest. Are you preventing adding more than one
> device in QEMU? Can you have more than one vtpm device attached to a VM ?

The Power Firmware spec states that only one device is supported 
(section on 'Additional driver requirements'); so QEMU doesn't not 
support more than one, either.

>
>
> Why PFW ? Please get rid of this suffix.

These would be the states and error codes defined by power firmware 
spec. Sure, will change it.

>
> +
> +static bool spapr_vtpm_activate(uint8_t locty)
> +{
> +	long rc;
> +	struct crq crq, *resp;
> +	static bool initialized = false; /* only one init */
> I would better limit number of devices in QEMU rather than limiting the
> driver.

Ok, I'll move this one into the global structure.


>
>> +
>> +	spapr_vtpm.buffer_offset = 0;
>> +
>> +	pfw_drv_state_set(PFW_DRV_STATE_REG_CRQ,
>> +			  PFW_DRV_ERROR_NO_FAILURE);
>> +
>> +	rc = hv_reg_crq(spapr_vtpm.vtpm_unit, (unsigned long)spapr_vtpm.qaddr,
>> +	                spapr_vtpm.qsize);
>> +	if (rc != H_SUCCESS) {
>> +		pfw_drv_state_set(PFW_DRV_STATE_WAIT_INIT,
>> +		                  PFW_DRV_ERROR_UNEXPECTED_REG_ERROR);
>> +		dprintf("CRQ registration failed\n");
>> +		return false;
>> +	}
>> +
>> +	/* we always start with q_entry 0 */
>> +	spapr_vtpm.q_entry = 0;
>> +
>> +	if (initialized)
>> +		goto skip_init;
>> +
>> +	crq.valid = PAPR_VTPM_INIT_CRQ_COMMAND;
>> +	crq.msg = PAPR_VTPM_INIT_CRQ_RESULT;
>> +
>> +	if (!spapr_send_crq_and_wait(spapr_vtpm.vtpm_unit,
>> +	                             &crq,
>> +	                             &resp,
>> +	                             10,
>> +	                             PFW_DRV_STATE_SEND_INIT,
>> +	                             PFW_DRV_STATE_WAIT_INIT_COMP)) {
>> +		dprintf("Initializing CRQ failed\n");
>> +		goto err_exit;
>> +	}
>> +	dprintf("Successfully initialized CRQ\n");
>> +
>> +	initialized = true;
>> +
>> +skip_init:
>> +	if (!spapr_vtpm_get_params())
>> +		goto err_exit;
>> +
>> +	return true;
>> +
>> +err_exit:
>> +	hv_free_crq(spapr_vtpm.vtpm_unit);
> and do we need to set "initialized = false"  here?

No, I do not think so.

>> +}
>> +
>> +static bool spapr_vtpm_senddata(const uint8_t *const data, uint32_t len)
>> +{
>> +	/*
>> +	 * we have to collect all data to be sent in a buffer
>> +	 */
>> +
>> +	if (spapr_vtpm.buffer_offset + len > spapr_vtpm.buffer_size) {
> what if the len > buffer_size ?

In this function we collect the data in a buffer. The check makes sure that
with the already accumulated data (buffer), the rest will fit into the 
buffer.
So the len > buffer_size is covered.

>
>> +		spapr_vtpm.buffer_offset = 0;
>> +		return false;
>> +	}
>> +
>> +	memcpy(&spapr_vtpm.buffer[spapr_vtpm.buffer_offset], data, len);
>> +
>> +	spapr_vtpm.buffer_offset += len;
>> +
>> +	return true;
>> +}
>> +
>> +static bool spapr_vtpm_transfer(void)
>> +{
>> +	struct crq crq;
>> +	long rc;
>> +
>> +	spapr_vtpm.response = get_response_crq();
> I see that "resp" and "response" is used interchangeably, for
> consistency use one of them.
>
>> +	/* response CRQ has been set and valid field cleared */
>> +
>> +	crq.valid = PAPR_VTPM_VALID_COMMAND;
>> +	crq.msg = PAPR_VTPM_TPM_COMMAND;
>> +	crq.len = cpu_to_be16(spapr_vtpm.buffer_offset);
>> +	crq.data = cpu_to_be32((uint64_t)spapr_vtpm.buffer);
>> +
>> +	pfw_drv_state_set(PFW_DRV_STATE_SEND_TPM_CMD,
>> +	                  PFW_DRV_ERROR_NO_FAILURE);
>> +
>> +	rc = hv_send_crq(spapr_vtpm.vtpm_unit, (uint64_t *)&crq);
>> +
>> +	if (rc == H_SUCCESS) {
>> +		pfw_drv_state_set(PFW_DRV_STATE_WAIT_TPM_RSP,
>> +		                  PFW_DRV_ERROR_NO_FAILURE);
>> +	} else {
>> +		/* per pfw doc, move to wait_init state */
> Is this available externally? where ?

It's not available externally. You should have received a copy, though.

I'll address all your other comments I deleted.

Thanks,
    Stefan