| Message ID | 1563603512-5914-1-git-send-email-frederic.konrad@adacore.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: htif: use qemu_log_mask instead of qemu_log | expand |
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 <frederic.konrad@adacore.com> > --- > 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. 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); > } > } > >
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 <frederic.konrad@adacore.com> >> --- >> 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); >> } >> } >> >>
Hi Philippe, Le 7/20/19 à 11:50 AM, Philippe Mathieu-Daudé a écrit : > 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 <frederic.konrad@adacore.com> >>> --- >>> 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. > Ok why not.. If the RISC-V maintainers are happy with your suggestion I can do a patch. Cheers, Fred >> 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); >>> } >>> } >>> >>>
On Mon, Jul 22, 2019 at 2:29 AM KONRAD Frederic <frederic.konrad@adacore.com> wrote: > > Hi Philippe, > > Le 7/20/19 à 11:50 AM, Philippe Mathieu-Daudé a écrit : > > 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 <frederic.konrad@adacore.com> > >>> --- > >>> 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. > > > > Ok why not.. If the RISC-V maintainers are happy with your suggestion I can do > a patch. Yes please! Most of these look like qemu_log_mask(LOG_GUEST_ERROR) cases so that's probably the best bet. The others should probably be converted to traces. Alistair > > Cheers, > Fred > > >> 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); > >>> } > >>> } > >>> > >>> >
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); } } 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); } }
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 <frederic.konrad@adacore.com> --- hw/riscv/riscv_htif.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)