On Mon, Jul 22, 2019 at 2:29 AM KONRAD Frederic
<frederic.kon...@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.kon...@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);
> >>> }
> >>> }
> >>>
> >>>
>
