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

[KONRAD Frederic]

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 <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.

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);
> > >       }
> > >   }
> > >