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 <alex.ben...@linaro.org> wrote: > > Yuval Shaia <yuval.sh...@oracle.com> 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] > yuval.sh...@oracle.com> > >> wrote: > >> > >> On Wed, May 01, 2019 at 04:10:39PM +0800, Boxuan Li wrote: > >> > Signed-off-by: Boxuan Li <[2]libox...@connect.hku.hk> > >> > --- > >> > 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 >