Re: [Qemu-riscv] [Qemu-devel] [PATCH] riscv: htif: use qemu_log_mask instead of qemu_log

[Philippe Mathieu-Daudé]

Cc'ing Stefan

On 7/20/19 11:44 AM, Philippe Mathieu-Daudé wrote:
> Hi Frederic,
> 
> On 7/20/19 8:18 AM, KONRAD Frederic wrote:
>> There are some debug trace appearing when using GDB with the HTIF mapped @0:
>>  Invalid htif read: address 0000000000000002
>>  Invalid htif read: address 0000000000000006
>>  Invalid htif read: address 000000000000000a
>>  Invalid htif read: address 000000000000000e
>>
>> So don't show them unconditionally.
>>
>> Signed-off-by: KONRAD Frederic <address@hidden>
>> ---
>>  hw/riscv/riscv_htif.c | 21 ++++++++++++---------
>>  1 file changed, 12 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/riscv/riscv_htif.c b/hw/riscv/riscv_htif.c
>> index 4f7b11d..6a8f0c2 100644
>> --- a/hw/riscv/riscv_htif.c
>> +++ b/hw/riscv/riscv_htif.c
>> @@ -119,7 +119,8 @@ static void htif_handle_tohost_write(HTIFState 
>> *htifstate, uint64_t val_written)
>>      int resp = 0;
>>  
>>      HTIF_DEBUG("mtohost write: device: %d cmd: %d what: %02" PRIx64
>> -        " -payload: %016" PRIx64 "\n", device, cmd, payload & 0xFF, 
>> payload);
>> +               " -payload: %016" PRIx64 "\n", device, cmd, payload & 0xFF,
>> +               payload);
>>  
>>      /*
>>       * Currently, there is a fixed mapping of devices:
>> @@ -137,7 +138,7 @@ static void htif_handle_tohost_write(HTIFState 
>> *htifstate, uint64_t val_written)
>>                  qemu_log_mask(LOG_UNIMP, "pk syscall proxy not 
>> supported\n");
>>              }
>>          } else {
>> -            qemu_log("HTIF device %d: unknown command\n", device);
>> +            HTIF_DEBUG("HTIF device %d: unknown command\n", device);
> 
> Generally, please don't go back to using DPRINTF() but use trace-events
> instead.

Oh, now I see that HTIF_DEBUG() is just an obscure way to report
crippled log trace-events, not portable to all trace backends.

It is only used in 2 files:

- hw/riscv/riscv_htif.c
- target/riscv/pmp.c as PMP_DEBUG()

I'd rather remove these macros and use trace-events the same way the
rest of the codebase use them, usable by all backends.

> However in this calls it seems using qemu_log_mask(LOG_UNIMP) or
> qemu_log_mask(LOG_GUEST_ERROR) is appropriate.
> 
>>          }
>>      } else if (likely(device == 0x1)) {
>>          /* HTIF Console */
>> @@ -150,12 +151,13 @@ static void htif_handle_tohost_write(HTIFState 
>> *htifstate, uint64_t val_written)
>>              qemu_chr_fe_write(&htifstate->chr, (uint8_t *)&payload, 1);
>>              resp = 0x100 | (uint8_t)payload;
>>          } else {
>> -            qemu_log("HTIF device %d: unknown command\n", device);
>> +            HTIF_DEBUG("HTIF device %d: unknown command\n", device);
>>          }
>>      } else {
>> -        qemu_log("HTIF unknown device or command\n");
>> +        HTIF_DEBUG("HTIF unknown device or command\n");
>>          HTIF_DEBUG("device: %d cmd: %d what: %02" PRIx64
>> -            " payload: %016" PRIx64, device, cmd, payload & 0xFF, payload);
>> +                   " payload: %016" PRIx64, device, cmd, payload & 0xFF,
>> +                   payload);
>>      }
>>      /*
>>       * - latest bbl does not set fromhost to 0 if there is a value in tohost
>> @@ -180,6 +182,7 @@ static void htif_handle_tohost_write(HTIFState 
>> *htifstate, uint64_t val_written)
>>  static uint64_t htif_mm_read(void *opaque, hwaddr addr, unsigned size)
>>  {
>>      HTIFState *htifstate = opaque;
>> +
>>      if (addr == TOHOST_OFFSET1) {
>>          return htifstate->env->mtohost & 0xFFFFFFFF;
>>      } else if (addr == TOHOST_OFFSET2) {
>> @@ -189,8 +192,8 @@ static uint64_t htif_mm_read(void *opaque, hwaddr addr, 
>> unsigned size)
>>      } else if (addr == FROMHOST_OFFSET2) {
>>          return (htifstate->env->mfromhost >> 32) & 0xFFFFFFFF;
>>      } else {
>> -        qemu_log("Invalid htif read: address %016" PRIx64 "\n",
>> -            (uint64_t)addr);
>> +        HTIF_DEBUG("Invalid htif read: address %016" PRIx64 "\n",
>> +                   (uint64_t)addr);
>>          return 0;
>>      }
>>  }
>> @@ -219,8 +222,8 @@ static void htif_mm_write(void *opaque, hwaddr addr,
>>          htifstate->env->mfromhost |= value << 32;
>>          htifstate->fromhost_inprogress = 0;
>>      } else {
>> -        qemu_log("Invalid htif write: address %016" PRIx64 "\n",
>> -            (uint64_t)addr);
>> +        HTIF_DEBUG("Invalid htif write: address %016" PRIx64 "\n",
>> +                   (uint64_t)addr);
>>      }
>>  }
>>  
>>