[SLOF] [PATCH v3 01/17] Add a TPM driver implementation

[Stefan Berger]

Hon c Lo/Poughkeepsie/IBM wrote on 03/15/2016 09:22:39 PM:


> >+ * Send a message via CRQ and wait for the response
> >+ */
> >+static bool spapr_send_crq_and_wait(unsigned long unit,
> >+ struct crq *crq,
> >+ struct crq **response,
> >+ unsigned timeout,
> >+ vtpm_drv_state state1,
> >+ vtpm_drv_state state2)
> >+{
> >+ long rc;
> >+ unsigned i;
> >+
> >+ *response = get_response_crq();
> >+
> >+ vtpm_drv_state_set(state1, VTPM_DRV_ERROR_NO_FAILURE);
> >+
> >+ rc = hv_send_crq(unit, (uint64_t *)crq);
> >+ if (rc != H_SUCCESS) {
> >+ vtpm_drv_state_set(VTPM_DRV_STATE_WAIT_INIT,
> >+ VTPM_DRV_ERROR_TPM_CRQ_ERROR);
> >+ return false;
> >+ }
> >+
> >+ vtpm_drv_state_set(state2, VTPM_DRV_ERROR_NO_FAILURE);
> >+
> >+ for (i = 0; i < timeout; i += 1000) {
> >+ if (((*response)->valid & PAPR_VTPM_MSG_RESULT))
> >+ return true;
> >+ SLOF_usleep(1000);
> >+ }

> 
> The 'timeout' is given the value of '10' in number of the calls to 
> the spapr_send_crq_and_wait(), which means the loop will be executed
> one time exactly.
> Should SLOF_usleep(1000) be done before the if-block? 

We are polling for a result here and it should stay in the loop to delay 
the polling a bit. The timeout counter should be increased by '1' only. I 
think the results come back immediately after sending the CRQ, so it 
didn't cause an error.


> 
> >+
> >+/*
> >+ * Get parameters from the CRQ
> >+ */
> >+static bool spapr_vtpm_get_params(void)
> >+{
> >+ struct crq crq, *response;
> >+ static bool completed = false; /* only once */
> >+
> >+ if (completed)
> >+ return true;
> >+
> >+ /* get the TPM version */
> >+ crq.valid = PAPR_VTPM_VALID_COMMAND;
> >+ crq.msg = PAPR_VTPM_GET_VERSION;
> >+
> >+ if (!spapr_send_crq_and_wait(spapr_vtpm.unit, &crq, &response, 10,
> >+ VTPM_DRV_STATE_SEND_GET_VERSION,
> >+ VTPM_DRV_STATE_WAIT_VERSION)) {
> >+ dprintf("Failure getting TPM version from CRQ\n");
> >+ return false;
> >+ }
> >+
> >+ vtpm_drv_state_set(VTPM_DRV_STATE_CHECK_VERSION,
> >+ VTPM_DRV_ERROR_NO_FAILURE);
> >+
> >+ spapr_vtpm.tpm_version = be32_to_cpu(response->data);
> >+ dprintf("TPM backend version: %d\n", spapr_vtpm.tpm_version);
> >+
> 
> This is minor.  dprintf() can use %u format here.
> 
> 

Fixed.

> >+ if (!spapr_send_crq_and_wait(spapr_vtpm.unit, &crq, &response, 10,
> >+ VTPM_DRV_STATE_SEND_BUFSIZE_REQ,
> >+ VTPM_DRV_STATE_WAIT_BUFSIZE)) {
> >+ dprintf("Failure getting RTCE buffer size from CRQ\n");
> >+ return false;
> >+ }
> >+
> >+ vtpm_drv_state_set(VTPM_DRV_STATE_ALLOC_RTCE_BUF,
> >+ VTPM_DRV_ERROR_NO_FAILURE);
> >+
> >+ dprintf("RTCE buffer size: %u\n", be16_to_cpu(response->len));
> >+ spapr_vtpm.buffer_size = response->len;
> 
> Should the above line be  spapr_vtpm.buffer_size = be16_to_cpu
> (response->len) ?
>

Yes. This didn't cause a fault since both sides are big endian.
 
> >+static bool spapr_vtpm_probe(void)
> >+{
> >+ spapr_vtpm_init();
> >+
> >+ if (!spapr_vtpm.qaddr) {
> >+ spapr_vtpm.qaddr = SLOF_alloc_mem(spapr_vtpm.qsize);
> 
> It's better with a cast to 'unsigned long' here, before the assignment.

qaddr is unsigned char * and SLOF_alloc_mem returns void *, so no casting 
is needed.

> 
> >+ spapr_vtpm.response = get_response_crq();
> >+ /* 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(len);
> >+ crq.data = cpu_to_be32((uint64_t)data);
> 
> The cast should be of type uint32_t instead?

Data is a pointer and the pointer cannot be cast to uint32_t. We could 
cast to (uint32_)(uint64_t), but that's not necessary. The provided 
address is 32bits only, so we can cut the top off.

In file included from tpm_drivers.c:19:0:
tpm_drivers.c: In function ‘spapr_vtpm_senddata’:
tpm_drivers.c:362:25: warning: cast from pointer to integer of different 
size [-Wpointer-to-int-cast]
  crq.data = cpu_to_be32((uint32_t)data);
                         ^
/root/src/SLOF-vTPM.20150805/include/byteorder.h:63:25: note: in 
definition of macro ‘cpu_to_be32’
 #define cpu_to_be32(x) (x)


> 
> >+static bool spapr_vtpm_readresponse(uint8_t *buffer, uint32_t *len)
> >+{
> >+ if (vtpm_drv_error_get() != VTPM_DRV_ERROR_NO_FAILURE) {
> >+ printf("VTPM CRQ: In failure mode\n");
> >+ return false;
> >+ }
> >+
> >+ /* response CRQ has been set */
> >+ *len = MIN(*len, be32_to_cpu(spapr_vtpm.response->len));
> >+
> 
> Should it be using be16_to_cpu() for data instead?

Yes.

> 
> >+ memcpy(buffer, (void *)(uint64_t)spapr_vtpm.response->data, *len);
> >+
> 
> Should it be casted to the type of uint32_t instead?

We cannot do that:

tpm_drivers.c: In function ‘spapr_vtpm_readresponse’:
tpm_drivers.c:421:17: warning: cast to pointer from integer of different 
size [-Wint-to-pointer-cast]
  memcpy(buffer, (void *)(uint32_t)spapr_vtpm.response->data, *len);
                 ^

> 
> >+bool spapr_is_vtpm_present(void)
> >+{
> >+ bool rc = false;
> >+
> >+ if (spapr_vtpm_probe()) {
> >+ spapr_vtpm_init();
> >+ rc = true;
> >+ }

So this function then simply becomes 'return spapr_vtpm_probe()'

Thanks for the review.

    Stefan


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/slof/attachments/20160316/b977f2ed/attachment-0001.html>