<tt><font size=2>Hon c Lo/Poughkeepsie/IBM wrote on 03/15/2016 09:22:39
PM:<br><br></font></tt><br><tt><font size=2>> >+ * Send a message via CRQ and wait for the
response<br>> >+ */<br>> >+static bool spapr_send_crq_and_wait(unsigned long unit,<br>> >+ struct crq *crq,<br>> >+ struct crq **response,<br>> >+ unsigned timeout,<br>> >+ vtpm_drv_state state1,<br>> >+ vtpm_drv_state state2)<br>> >+{<br>> >+ long rc;<br>> >+ unsigned i;<br>> >+<br>> >+ *response = get_response_crq();<br>> >+<br>> >+ vtpm_drv_state_set(state1, VTPM_DRV_ERROR_NO_FAILURE);<br>> >+<br>> >+ rc = hv_send_crq(unit, (uint64_t *)crq);<br>> >+ if (rc != H_SUCCESS) {<br>> >+ vtpm_drv_state_set(VTPM_DRV_STATE_WAIT_INIT,<br>> >+ VTPM_DRV_ERROR_TPM_CRQ_ERROR);<br>> >+ return false;<br>> >+ }<br>> >+<br>> >+ vtpm_drv_state_set(state2, VTPM_DRV_ERROR_NO_FAILURE);<br>> >+<br>> >+ for (i = 0; i < timeout; i += 1000) {<br>> >+ if (((*response)->valid & PAPR_VTPM_MSG_RESULT))<br>> >+ return true;<br>> >+ SLOF_usleep(1000);<br>> >+ }<br></font></tt><br><tt><font size=2>> <br>> The 'timeout' is given the value of '10' in number of the calls to
<br>> the spapr_send_crq_and_wait(), which means the loop will be executed<br>> one time exactly.</font></tt><br><tt><font size=2>> Should SLOF_usleep(1000) be done before the if-block?
 </font></tt><br><br><tt><font size=2>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.</font></tt><br><br><br><tt><font size=2>> <br>> >+<br>> >+/*<br>> >+ * Get parameters from the CRQ<br>> >+ */<br>> >+static bool spapr_vtpm_get_params(void)<br>> >+{<br>> >+ struct crq crq, *response;<br>> >+ static bool completed = false; /* only once */<br>> >+<br>> >+ if (completed)<br>> >+ return true;<br>> >+<br>> >+ /* get the TPM version */<br>> >+ crq.valid = PAPR_VTPM_VALID_COMMAND;<br>> >+ crq.msg = PAPR_VTPM_GET_VERSION;<br>> >+<br>> >+ if (!spapr_send_crq_and_wait(spapr_vtpm.unit, &crq, &response,
10,<br>> >+ VTPM_DRV_STATE_SEND_GET_VERSION,<br>> >+ VTPM_DRV_STATE_WAIT_VERSION)) {<br>> >+ dprintf("Failure getting TPM version from CRQ\n");<br>> >+ return false;<br>> >+ }<br>> >+<br>> >+ vtpm_drv_state_set(VTPM_DRV_STATE_CHECK_VERSION,<br>> >+ VTPM_DRV_ERROR_NO_FAILURE);<br>> >+<br>> >+ spapr_vtpm.tpm_version = be32_to_cpu(response->data);<br>> >+ dprintf("TPM backend version: %d\n", spapr_vtpm.tpm_version);<br>> >+</font></tt><br><tt><font size=2>> <br>> This is minor.  dprintf() can use %u format here.</font></tt><br><tt><font size=2>> <br>> <br></font></tt><br><tt><font size=2>Fixed.</font></tt><br><br><tt><font size=2>> >+ if (!spapr_send_crq_and_wait(spapr_vtpm.unit,
&crq, &response, 10,<br>> >+ VTPM_DRV_STATE_SEND_BUFSIZE_REQ,<br>> >+ VTPM_DRV_STATE_WAIT_BUFSIZE)) {<br>> >+ dprintf("Failure getting RTCE buffer size from CRQ\n");<br>> >+ return false;<br>> >+ }<br>> >+<br>> >+ vtpm_drv_state_set(VTPM_DRV_STATE_ALLOC_RTCE_BUF,<br>> >+ VTPM_DRV_ERROR_NO_FAILURE);<br>> >+<br>> >+ dprintf("RTCE buffer size: %u\n", be16_to_cpu(response->len));<br>> >+ spapr_vtpm.buffer_size = response->len;</font></tt><br><tt><font size=2>> <br>> Should the above line be  spapr_vtpm.buffer_size = be16_to_cpu<br>> (response->len) ?</font></tt><br><tt><font size=2>></font></tt><br><br><tt><font size=2>Yes. This didn't cause a fault since both sides are
big endian.</font></tt><br><tt><font size=2> <br>> >+static bool spapr_vtpm_probe(void)</font></tt><br><tt><font size=2>> >+{<br>> >+ spapr_vtpm_init();<br>> >+<br>> >+ if (!spapr_vtpm.qaddr) {<br>> >+ spapr_vtpm.qaddr = SLOF_alloc_mem(spapr_vtpm.qsize);</font></tt><br><tt><font size=2>> <br>> It's better with a cast to 'unsigned long' here, before the assignment.</font></tt><br><br><tt><font size=2>qaddr is unsigned char * and SLOF_alloc_mem returns
void *, so no casting is needed.</font></tt><br><br><tt><font size=2>> <br>> >+ spapr_vtpm.response = get_response_crq();</font></tt><br><tt><font size=2>> >+ /* response CRQ has been set and valid
field cleared */<br>> >+<br>> >+ crq.valid = PAPR_VTPM_VALID_COMMAND;<br>> >+ crq.msg = PAPR_VTPM_TPM_COMMAND;<br>> >+ crq.len = cpu_to_be16(len);<br>> >+ crq.data = cpu_to_be32((uint64_t)data);</font></tt><br><tt><font size=2>> <br>> The cast should be of type uint32_t instead?</font></tt><br><br><tt><font size=2>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.</font></tt><br><br><font size=4 face="Lucida Console">In file included from tpm_drivers.c:19:0:</font><br><font size=4 face="Lucida Console">tpm_drivers.c: In function ‘spapr_vtpm_senddata’:</font><br><font size=4 face="Lucida Console">tpm_drivers.c:362:25: warning: cast
from pointer to integer of different size [-Wpointer-to-int-cast]</font><br><font size=4 face="Lucida Console">  crq.data = cpu_to_be32((uint32_t)data);</font><br><font size=4 face="Lucida Console">         
               ^</font><br><font size=4 face="Lucida Console">/root/src/SLOF-vTPM.20150805/include/byteorder.h:63:25:
note: in definition of macro ‘cpu_to_be32’</font><br><font size=4 face="Lucida Console"> #define cpu_to_be32(x) (x)</font><br><br><br><tt><font size=2>> <br>> >+static bool spapr_vtpm_readresponse(uint8_t *buffer, uint32_t
*len)</font></tt><br><tt><font size=2>> >+{<br>> >+ if (vtpm_drv_error_get() != VTPM_DRV_ERROR_NO_FAILURE) {<br>> >+ printf("VTPM CRQ: In failure mode\n");<br>> >+ return false;<br>> >+ }<br>> >+<br>> >+ /* response CRQ has been set */<br>> >+ *len = MIN(*len, be32_to_cpu(spapr_vtpm.response->len));<br>> >+</font></tt><br><tt><font size=2>> <br>> Should it be using be16_to_cpu() for data instead?</font></tt><br><br><tt><font size=2>Yes.</font></tt><br><br><tt><font size=2>> <br>> >+ memcpy(buffer, (void *)(uint64_t)spapr_vtpm.response->data,
*len);<br>> >+</font></tt><br><tt><font size=2>> <br>> Should it be casted to the type of uint32_t instead?</font></tt><br><br><tt><font size=2>We cannot do that:</font></tt><br><br><font size=4 face="Lucida Console">tpm_drivers.c: In function ‘spapr_vtpm_readresponse’:</font><br><font size=4 face="Lucida Console">tpm_drivers.c:421:17: warning: cast
to pointer from integer of different size [-Wint-to-pointer-cast]</font><br><font size=4 face="Lucida Console">  memcpy(buffer, (void *)(uint32_t)spapr_vtpm.response->data,
*len);</font><br><font size=4 face="Lucida Console">         
       ^</font><br><br><tt><font size=2>> <br>> >+bool spapr_is_vtpm_present(void)</font></tt><br><tt><font size=2>> >+{<br>> >+ bool rc = false;<br>> >+<br>> >+ if (spapr_vtpm_probe()) {<br>> >+ spapr_vtpm_init();</font></tt><br><tt><font size=2>> >+ rc = true;<br>> >+ }</font></tt><br><br><tt><font size=2>So this function then simply becomes 'return spapr_vtpm_probe()'</font></tt><br><br><tt><font size=2>Thanks for the review.</font></tt><br><br><tt><font size=2>    Stefan</font></tt><br><BR>