[Qemu-devel] [PATCH v3 1/3] tpm_tis: fix loop that cancels any seizure by a lower locality

[Qemu-devel] [PATCH v3 1/3] tpm_tis: fix loop that cancels any seizure by a lower locality

[Liam Merwick]

In tpm_tis_mmio_write() if the requesting locality is seizing
access, any seizure by a lower locality is cancelled.  However the
loop doing the seizure had an off-by-one error and the locality
immediately preceding the requesting locality was not being cleared.
This is fixed by adjusting the test in the for loop to check the
localities up to the requesting locality.

Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
---
 hw/tpm/tpm_tis.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index fd6bb9b59a96..61a130beef35 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -624,7 +624,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
                 }
 
                 /* cancel any seize by a lower locality */
-                for (l = 0; l < locty - 1; l++) {
+                for (l = 0; l < locty; l++) {
                     s->loc[l].access &= ~TPM_TIS_ACCESS_SEIZE;
                 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 2/3] tpm_tis: assert valid addr passed to tpm_tis_locality_from_addr()

[Liam Merwick]

Assert that the address passed in results in a valid locality value.
Current callers pass a valid address so this is just a defensive check
to prevent a future caller passing an incorrect address or catch if the
MMIO address parameters were not all modified correctly.  This is to
help static code analysis tools that report that no explicit checking
is being done.

Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
---
 hw/tpm/tpm_tis.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index 61a130beef35..772431f20874 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -100,7 +100,9 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr,
 
 static uint8_t tpm_tis_locality_from_addr(hwaddr addr)
 {
-    return (uint8_t)((addr >> TPM_TIS_LOCALITY_SHIFT) & 0x7);
+    uint8_t locty = (uint8_t)(addr >> TPM_TIS_LOCALITY_SHIFT);
+    assert(TPM_TIS_IS_VALID_LOCTY(locty));
+    return locty;
 }
 
 static void tpm_tis_show_buffer(const unsigned char *buffer,
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 3/3] tpm_tis: convert tpm_tis_show_buffer() to use trace event

[Liam Merwick]

cppcheck reports:

[hw/tpm/tpm_tis.c:113]: (warning) %d in format string (no. 2) requires 'int' but the argument type is 'unsigned int'

Rather than just converting the format specifier to use '%u", the
tpm_tis_show_buffer() function is converted to use trace points and
the two debug callers use the trace event infrastructure so that it's
available in production cases also and not just when DEBUG_TIS is enabled.

Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
---

Sample trace output

% echo tpm_tis_show_buffer > /tmp/events 
% export QTEST_QEMU_BINARY="/home/Development/qemu-upstream/x86_64-softmmu/qemu-system-x86_64 --trace events=/tmp/events"
% tests/tpm-tis-test 
/D=/home/Development/qemu-upstream/hw/tpm/tpm-tis/test_check_localities: OK
/D=/home/Development/qemu-upstream/hw/tpm/tpm-tis/test_check_access_reg: OK
/D=/home/Development/qemu-upstream/hw/tpm/tpm-tis/test_check_access_reg_seize: OK
/D=/home/Development/qemu-upstream/hw/tpm/tpm-tis/test_check_access_reg_release: OK
/D=/home/Development/qemu-upstream/hw/tpm/tpm-tis/test_check_transmit: OK
% ./scripts/simpletrace.py trace-events-all trace-30665
tpm_tis_show_buffer 0.000 pid=30665 string=tpm_tis: To TPM len=0xc line=80 01 00 00 00 0C 00 00 01 44 00 00 
tpm_tis_show_buffer 256.581 pid=30665 string=tpm_tis: From TPM len=0xa line=80 01 00 00 00 0A 00 00 01 01 

 hw/tpm/tpm_tis.c    | 25 +++++++++++++++++--------
 hw/tpm/trace-events |  1 +
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index 772431f20874..18e19d5f900d 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -108,17 +108,26 @@ static uint8_t tpm_tis_locality_from_addr(hwaddr addr)
 static void tpm_tis_show_buffer(const unsigned char *buffer,
                                 size_t buffer_size, const char *string)
 {
-    uint32_t len, i;
+    size_t len, i;
+    char *line_buffer, *p;
 
     len = MIN(tpm_cmd_get_size(buffer), buffer_size);
-    printf("tpm_tis: %s length = %d\n", string, len);
-    for (i = 0; i < len; i++) {
+
+    /*
+     * allocate enough room for 3 chars per buffer entry plus
+     * a newline after every 16 chars.
+     */
+    line_buffer = g_malloc(len * 3 + (len / 16));
+
+    for (i = 0, p = line_buffer; i < len; i++) {
         if (i && !(i % 16)) {
-            printf("\n");
+            p += sprintf(p, "\n");
         }
-        printf("%.2X ", buffer[i]);
+        p += sprintf(p, "%.2X ", buffer[i]);
     }
-    printf("\n");
+    trace_tpm_tis_show_buffer(string, len, line_buffer);
+
+    g_free(line_buffer);
 }
 
 /*
@@ -145,7 +154,7 @@ static void tpm_tis_sts_set(TPMLocality *l, uint32_t flags)
  */
 static void tpm_tis_tpm_send(TPMState *s, uint8_t locty)
 {
-    if (DEBUG_TIS) {
+    if (trace_event_get_state_backends(TRACE_TPM_TIS_SHOW_BUFFER)) {
         tpm_tis_show_buffer(s->buffer, s->be_buffer_size,
                             "tpm_tis: To TPM");
     }
@@ -315,7 +324,7 @@ static void tpm_tis_request_completed(TPMIf *ti, int ret)
     s->loc[locty].state = TPM_TIS_STATE_COMPLETION;
     s->rw_offset = 0;
 
-    if (DEBUG_TIS) {
+    if (trace_event_get_state_backends(TRACE_TPM_TIS_SHOW_BUFFER)) {
         tpm_tis_show_buffer(s->buffer, s->be_buffer_size,
                             "tpm_tis: From TPM");
     }
diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
index 920d32ad5514..c0267d075027 100644
--- a/hw/tpm/trace-events
+++ b/hw/tpm/trace-events
@@ -36,6 +36,7 @@ tpm_emulator_pre_save(void) ""
 tpm_emulator_inst_init(void) ""
 
 # hw/tpm/tpm_tis.c
+tpm_tis_show_buffer(const char *string, size_t len, const char *line) "tpm_tis: %s length = %zu\n%s"
 tpm_tis_raise_irq(uint32_t irqmask) "Raising IRQ for flag 0x%08x"
 tpm_tis_new_active_locality(uint8_t locty) "Active locality is now %d"
 tpm_tis_abort(uint8_t locty) "New active locality is %d"
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 3/3] tpm_tis: convert tpm_tis_show_buffer() to use trace event

[Stefan Berger]

On 2/12/19 3:48 PM, Liam Merwick wrote:
> cppcheck reports:
>
> [hw/tpm/tpm_tis.c:113]: (warning) %d in format string (no. 2) requires 'int' but the argument type is 'unsigned int'
>
> Rather than just converting the format specifier to use '%u", the
> tpm_tis_show_buffer() function is converted to use trace points and
> the two debug callers use the trace event infrastructure so that it's
> available in production cases also and not just when DEBUG_TIS is enabled.
>
> Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
> ---
>
> Sample trace output
>
> % echo tpm_tis_show_buffer > /tmp/events
> % export QTEST_QEMU_BINARY="/home/Development/qemu-upstream/x86_64-softmmu/qemu-system-x86_64 --trace events=/tmp/events"
> % tests/tpm-tis-test
> /D=/home/Development/qemu-upstream/hw/tpm/tpm-tis/test_check_localities: OK
> /D=/home/Development/qemu-upstream/hw/tpm/tpm-tis/test_check_access_reg: OK
> /D=/home/Development/qemu-upstream/hw/tpm/tpm-tis/test_check_access_reg_seize: OK
> /D=/home/Development/qemu-upstream/hw/tpm/tpm-tis/test_check_access_reg_release: OK
> /D=/home/Development/qemu-upstream/hw/tpm/tpm-tis/test_check_transmit: OK
> % ./scripts/simpletrace.py trace-events-all trace-30665
> tpm_tis_show_buffer 0.000 pid=30665 string=tpm_tis: To TPM len=0xc line=80 01 00 00 00 0C 00 00 01 44 00 00
> tpm_tis_show_buffer 256.581 pid=30665 string=tpm_tis: From TPM len=0xa line=80 01 00 00 00 0A 00 00 01 01


should now be 'length=10' and then a new line with just the bytes?


>
>   hw/tpm/tpm_tis.c    | 25 +++++++++++++++++--------
>   hw/tpm/trace-events |  1 +
>   2 files changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index 772431f20874..18e19d5f900d 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -108,17 +108,26 @@ static uint8_t tpm_tis_locality_from_addr(hwaddr addr)
>   static void tpm_tis_show_buffer(const unsigned char *buffer,
>                                   size_t buffer_size, const char *string)
>   {
> -    uint32_t len, i;
> +    size_t len, i;
> +    char *line_buffer, *p;
>   
>       len = MIN(tpm_cmd_get_size(buffer), buffer_size);
> -    printf("tpm_tis: %s length = %d\n", string, len);
> -    for (i = 0; i < len; i++) {
> +
> +    /*
> +     * allocate enough room for 3 chars per buffer entry plus
> +     * a newline after every 16 chars.


This is missing the terminating null byte.


> +     */
> +    line_buffer = g_malloc(len * 3 + (len / 16));
> +
> +    for (i = 0, p = line_buffer; i < len; i++) {
>           if (i && !(i % 16)) {
> -            printf("\n");
> +            p += sprintf(p, "\n");
>           }
> -        printf("%.2X ", buffer[i]);
> +        p += sprintf(p, "%.2X ", buffer[i]);
>       }
> -    printf("\n");
> +    trace_tpm_tis_show_buffer(string, len, line_buffer);
> +
> +    g_free(line_buffer);
>   }
>   
>   /*
> @@ -145,7 +154,7 @@ static void tpm_tis_sts_set(TPMLocality *l, uint32_t flags)
>    */
>   static void tpm_tis_tpm_send(TPMState *s, uint8_t locty)
>   {
> -    if (DEBUG_TIS) {
> +    if (trace_event_get_state_backends(TRACE_TPM_TIS_SHOW_BUFFER)) {
>           tpm_tis_show_buffer(s->buffer, s->be_buffer_size,
>                               "tpm_tis: To TPM");
>       }
> @@ -315,7 +324,7 @@ static void tpm_tis_request_completed(TPMIf *ti, int ret)
>       s->loc[locty].state = TPM_TIS_STATE_COMPLETION;
>       s->rw_offset = 0;
>   
> -    if (DEBUG_TIS) {
> +    if (trace_event_get_state_backends(TRACE_TPM_TIS_SHOW_BUFFER)) {
>           tpm_tis_show_buffer(s->buffer, s->be_buffer_size,
>                               "tpm_tis: From TPM");
>       }
> diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
> index 920d32ad5514..c0267d075027 100644
> --- a/hw/tpm/trace-events
> +++ b/hw/tpm/trace-events
> @@ -36,6 +36,7 @@ tpm_emulator_pre_save(void) ""
>   tpm_emulator_inst_init(void) ""
>   
>   # hw/tpm/tpm_tis.c
> +tpm_tis_show_buffer(const char *string, size_t len, const char *line) "tpm_tis: %s length = %zu\n%s"
>   tpm_tis_raise_irq(uint32_t irqmask) "Raising IRQ for flag 0x%08x"
>   tpm_tis_new_active_locality(uint8_t locty) "Active locality is now %d"
>   tpm_tis_abort(uint8_t locty) "New active locality is %d"

Re: [Qemu-devel] [PATCH v3 3/3] tpm_tis: convert tpm_tis_show_buffer() to use trace event

[Liam Merwick]

On 12/02/2019 21:01, Stefan Berger wrote:
> On 2/12/19 3:48 PM, Liam Merwick wrote:
>> cppcheck reports:
>>
>> [hw/tpm/tpm_tis.c:113]: (warning) %d in format string (no. 2) requires 
>> 'int' but the argument type is 'unsigned int'
>>
>> Rather than just converting the format specifier to use '%u", the
>> tpm_tis_show_buffer() function is converted to use trace points and
>> the two debug callers use the trace event infrastructure so that it's
>> available in production cases also and not just when DEBUG_TIS is 
>> enabled.
>>
>> Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
>> ---
>>
>> Sample trace output
>>
>> % echo tpm_tis_show_buffer > /tmp/events
>> % export 
>> QTEST_QEMU_BINARY="/home/Development/qemu-upstream/x86_64-softmmu/qemu-system-x86_64 
>> --trace events=/tmp/events"
>> % tests/tpm-tis-test
>> /D=/home/Development/qemu-upstream/hw/tpm/tpm-tis/test_check_localities: 
>> OK
>> /D=/home/Development/qemu-upstream/hw/tpm/tpm-tis/test_check_access_reg: 
>> OK
>> /D=/home/Development/qemu-upstream/hw/tpm/tpm-tis/test_check_access_reg_seize: 
>> OK
>> /D=/home/Development/qemu-upstream/hw/tpm/tpm-tis/test_check_access_reg_release: 
>> OK
>> /D=/home/Development/qemu-upstream/hw/tpm/tpm-tis/test_check_transmit: OK
>> % ./scripts/simpletrace.py trace-events-all trace-30665
>> tpm_tis_show_buffer 0.000 pid=30665 string=tpm_tis: To TPM len=0xc 
>> line=80 01 00 00 00 0C 00 00 01 44 00 00
>> tpm_tis_show_buffer 256.581 pid=30665 string=tpm_tis: From TPM len=0xa 
>> line=80 01 00 00 00 0A 00 00 01 01
> 
> 
> should now be 'length=10' and then a new line with just the bytes?
> 

The example I gave was using --enable-trace-backends=simple (which 
AFAICT doesn't use the format string, just the name/type of the parameters)

tpm_tis_show_buffer 0.000 pid=8416 direction=To TPM len=0xc line=80 01 
00 00 00 0C 00 00 01 44 00 00
tpm_tis_show_buffer 303.909 pid=8416 direction=From TPM len=0xa line=80 
01 00 00 00 0A 00 00 01 01

Below is the output with --enable-trace-backends=log which does include 
the newline

1255@1550051983.333949:tpm_tis_show_buffer direction: To TPM len: 12
buf: 80 01 00 00 00 0C 00 00 01 44 00 00
1255@1550051983.334213:tpm_tis_show_buffer direction: From TPM len: 10
buf: 80 01 00 00 00 0A 00 00 01 01

(those include a few tweaks for v4 I made based on investigating this 
question: dropped "tpm_tis: " since the function name is now printed by 
the tracing framework, renamed some of the parameters to 
trace_tpm_tis_show_buffer() and adjusted the format string)


diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index 18e19d5f900d..d7b9bee85741 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -155,8 +155,7 @@ static void tpm_tis_sts_set(TPMLocality *l, uint32_t 
flags)
  static void tpm_tis_tpm_send(TPMState *s, uint8_t locty)
  {
      if (trace_event_get_state_backends(TRACE_TPM_TIS_SHOW_BUFFER)) {
-        tpm_tis_show_buffer(s->buffer, s->be_buffer_size,
-                            "tpm_tis: To TPM");
+        tpm_tis_show_buffer(s->buffer, s->be_buffer_size, "To TPM");
      }

      /*
@@ -325,8 +324,7 @@ static void tpm_tis_request_completed(TPMIf *ti, int 
ret)
      s->rw_offset = 0;

      if (trace_event_get_state_backends(TRACE_TPM_TIS_SHOW_BUFFER)) {
-        tpm_tis_show_buffer(s->buffer, s->be_buffer_size,
-                            "tpm_tis: From TPM");
+        tpm_tis_show_buffer(s->buffer, s->be_buffer_size, "From TPM");
      }

      if (TPM_TIS_IS_VALID_LOCTY(s->next_locty)) {
diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
index c0267d075027..f45dcd220993 100644
--- a/hw/tpm/trace-events
+++ b/hw/tpm/trace-events
@@ -36,7 +36,7 @@ tpm_emulator_pre_save(void) ""
  tpm_emulator_inst_init(void) ""

  # hw/tpm/tpm_tis.c
-tpm_tis_show_buffer(const char *string, size_t len, const char *line) 
"tpm_tis: %s length = %zu\n%s"
+tpm_tis_show_buffer(const char *direction, size_t len, const char *buf) 
"direction: %s len: %zu\nbuf: %s


> 
>>
>>   hw/tpm/tpm_tis.c    | 25 +++++++++++++++++--------
>>   hw/tpm/trace-events |  1 +
>>   2 files changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
>> index 772431f20874..18e19d5f900d 100644
>> --- a/hw/tpm/tpm_tis.c
>> +++ b/hw/tpm/tpm_tis.c
>> @@ -108,17 +108,26 @@ static uint8_t tpm_tis_locality_from_addr(hwaddr 
>> addr)
>>   static void tpm_tis_show_buffer(const unsigned char *buffer,
>>                                   size_t buffer_size, const char *string)
>>   {
>> -    uint32_t len, i;
>> +    size_t len, i;
>> +    char *line_buffer, *p;
>>       len = MIN(tpm_cmd_get_size(buffer), buffer_size);
>> -    printf("tpm_tis: %s length = %d\n", string, len);
>> -    for (i = 0; i < len; i++) {
>> +
>> +    /*
>> +     * allocate enough room for 3 chars per buffer entry plus
>> +     * a newline after every 16 chars.
> 
> 
> This is missing the terminating null byte.
> 


will fix in v4

Regards,
Liam


> 
>> +     */
>> +    line_buffer = g_malloc(len * 3 + (len / 16));
>> +
>> +    for (i = 0, p = line_buffer; i < len; i++) {
>>           if (i && !(i % 16)) {
>> -            printf("\n");
>> +            p += sprintf(p, "\n");
>>           }
>> -        printf("%.2X ", buffer[i]);
>> +        p += sprintf(p, "%.2X ", buffer[i]);
>>       }
>> -    printf("\n");
>> +    trace_tpm_tis_show_buffer(string, len, line_buffer);
>> +
>> +    g_free(line_buffer);
>>   }
>>   /*
>> @@ -145,7 +154,7 @@ static void tpm_tis_sts_set(TPMLocality *l, 
>> uint32_t flags)
>>    */
>>   static void tpm_tis_tpm_send(TPMState *s, uint8_t locty)
>>   {
>> -    if (DEBUG_TIS) {
>> +    if (trace_event_get_state_backends(TRACE_TPM_TIS_SHOW_BUFFER)) {
>>           tpm_tis_show_buffer(s->buffer, s->be_buffer_size,
>>                               "tpm_tis: To TPM");
>>       }
>> @@ -315,7 +324,7 @@ static void tpm_tis_request_completed(TPMIf *ti, 
>> int ret)
>>       s->loc[locty].state = TPM_TIS_STATE_COMPLETION;
>>       s->rw_offset = 0;
>> -    if (DEBUG_TIS) {
>> +    if (trace_event_get_state_backends(TRACE_TPM_TIS_SHOW_BUFFER)) {
>>           tpm_tis_show_buffer(s->buffer, s->be_buffer_size,
>>                               "tpm_tis: From TPM");
>>       }
>> diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
>> index 920d32ad5514..c0267d075027 100644
>> --- a/hw/tpm/trace-events
>> +++ b/hw/tpm/trace-events
>> @@ -36,6 +36,7 @@ tpm_emulator_pre_save(void) ""
>>   tpm_emulator_inst_init(void) ""
>>   # hw/tpm/tpm_tis.c
>> +tpm_tis_show_buffer(const char *string, size_t len, const char *line) 
>> "tpm_tis: %s length = %zu\n%s"
>>   tpm_tis_raise_irq(uint32_t irqmask) "Raising IRQ for flag 0x%08x"
>>   tpm_tis_new_active_locality(uint8_t locty) "Active locality is now %d"
>>   tpm_tis_abort(uint8_t locty) "New active locality is %d"
> 
>