Re: [Qemu-devel] [PATCH v2] hw/virtio/virtio-mmio: Convert DPRINTF to traces

[LI, BO XUAN]

Hi Yuval and Alex,

Thanks for the suggestions. I'll update and send a patch v3 soon.

Best regards,
Boxuan Li

On Wed, May 1, 2019 at 11:17 PM Alex Bennée <address@hidden> wrote:

>
> Yuval Shaia <address@hidden> writes:
>
> > On Wed, May 01, 2019 at 08:42:35PM +0800, LI, BO XUAN wrote:
> >>    On Wed, May 1, 2019 at 4:58 PM Yuval Shaia <[1]
> address@hidden>
> >>    wrote:
> >>
> >>      On Wed, May 01, 2019 at 04:10:39PM +0800, Boxuan Li wrote:
> >>      > Signed-off-by: Boxuan Li <address@hidden>
> >>      > ---
> >>      > v2: Instead of using conditional debugs, convert DPRINTF to
> traces
> >>      > ---
> >>      >  hw/virtio/trace-events  | 13 +++++++++++++
> >>      >  hw/virtio/virtio-mmio.c | 35 ++++++++++++-----------------------
> >>      >  2 files changed, 25 insertions(+), 23 deletions(-)
> >>      >
> >>      > diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> >>      > index 60c649c4bc..37c781b487 100644
> >>      > --- a/hw/virtio/trace-events
> >>      > +++ b/hw/virtio/trace-events
> >>      > @@ -46,3 +46,16 @@ virtio_balloon_handle_output(const char *name,
> >>      uint64_t gpa) "section name: %s g
> >>      >  virtio_balloon_get_config(uint32_t num_pages, uint32_t actual)
> >>      "num_pages: %d actual: %d"
> >>      >  virtio_balloon_set_config(uint32_t actual, uint32_t oldactual)
> >>      "actual: %d oldactual: %d"
> >>      >  virtio_balloon_to_target(uint64_t target, uint32_t num_pages)
> >>      "balloon target: 0x%"PRIx64" num_pages: %d"
> >>      > +
> >>      > +# virtio-mmio.c
> >>      > +virtio_mmio_read(int offset) "virtio_mmio_read offset 0x%x"
> >>      > +virtio_mmio_wrong_size_read(void) "wrong size access to
> >>      register!"
> >>      > +virtio_mmio_read_interrupt(void) "read of write-only register"
> >>      > +virtio_mmio_bad_read_offset(void) "bad register offset"
> >>      > +virtio_mmio_write_offset(int offset, uint64_t value)
> >>      "virtio_mmio_write offset 0x%x value 0x%" PRIx64
> >>      > +virtio_mmio_wrong_size_write(void) "wrong size access to
> >>      register!"
> >>      > +virtio_mmio_guest_page(uint64_t size, int shift) "guest page
> size
> >>      0x%" PRIx64 " shift %d"
> >>      > +virtio_mmio_queue_write(int value, int max_size) "mmio_queue
> >>      write %d max %d"
> >>      > +virtio_mmio_write_interrupt(void) "write to readonly register"
> >>      > +virtio_mmio_bad_write_offset(void) "bad register offset"
> >>      > +virtio_mmio_setting_irq(int level) "virtio_mmio setting IRQ %d"
> >>      > diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
> >>      > index 5807aa87fe..4251df399d 100644
> >>      > --- a/hw/virtio/virtio-mmio.c
> >>      > +++ b/hw/virtio/virtio-mmio.c
> >>      > @@ -27,16 +27,7 @@
> >>      >  #include "sysemu/kvm.h"
> >>      >  #include "hw/virtio/virtio-bus.h"
> >>      >  #include "qemu/error-report.h"
> >>      > -
> >>      > -/* #define DEBUG_VIRTIO_MMIO */
> >>      > -
> >>      > -#ifdef DEBUG_VIRTIO_MMIO
> >>      > -
> >>      > -#define DPRINTF(fmt, ...) \
> >>      > -do { printf("virtio_mmio: " fmt , ## __VA_ARGS__); } while (0)
> >>      > -#else
> >>      > -#define DPRINTF(fmt, ...) do {} while (0)
> >>      > -#endif
> >>      > +#include "trace.h"
> >>      >
> >>      >  /* QOM macros */
> >>      >  /* virtio-mmio-bus */
> >>      > @@ -107,7 +98,7 @@ static uint64_t virtio_mmio_read(void *opaque,
> >>      hwaddr offset, unsigned size)
> >>      >      VirtIOMMIOProxy *proxy = (VirtIOMMIOProxy *)opaque;
> >>      >      VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> >>      >
> >>      > -    DPRINTF("virtio_mmio_read offset 0x%x\n", (int)offset);
> >>      > +    trace_virtio_mmio_read((int)offset);
> >>      >
> >>      >      if (!vdev) {
> >>      >          /* If no backend is present, we treat most registers as
> >>      > @@ -144,7 +135,7 @@ static uint64_t virtio_mmio_read(void
> *opaque,
> >>      hwaddr offset, unsigned size)
> >>      >          }
> >>      >      }
> >>      >      if (size != 4) {
> >>      > -        DPRINTF("wrong size access to register!\n");
> >>      > +        trace_virtio_mmio_wrong_size_read();
> >>      Have you considered using qemu_error_report to report such errors?
> >>
> >>    Thanks for the suggestion. I am a newcomer here so my question might
> be
> >>    a bit dumb: I thought they are warnings instead of errors since
> return
> >>    values are 0. Do you suggest using error_report function and changing
> >>    return values from 0 to -1?
> >>    Best regards,
> >>    Boxuan Li
> >
> > I think that when driver store invalid data in device register it is an
> > error but the best is to check what other devices do.
>
> We could have:
>
>         qemu_log_mask(LOG_GUEST_ERROR, "%s: wrong size access to
> register!\n". __func__);
>
> --
> Alex Bennée
>