Re: [PATCH 1/2] log: Add separate debug option for logging invalid memory accesses

[BALATON Zoltan]

On Tue, 31 Jan 2023, BALATON Zoltan wrote:
> On Thu, 19 Jan 2023, BALATON Zoltan wrote:
> > Currently -d guest_errors enables logging of different invalid actions
> > by the guest such as misusing hardware, accessing missing features or
> > invalid memory areas. The memory access logging can be quite verbose
> > which obscures the other messages enabled by this debug switch so
> > separate it by adding a new -d memaccess option to make it possible to
> > control it independently of other guest error logs.
> >
> > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>
> Ping? Could somebody review and pick it up please?

Ping?

Regards,
BALATON Zoltan

> > ---
> > include/qemu/log.h | 1 +
> > softmmu/memory.c   | 6 +++---
> > softmmu/physmem.c  | 2 +-
> > util/log.c         | 2 ++
> > 4 files changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/qemu/log.h b/include/qemu/log.h
> > index c5643d8dd5..4bf0a65a85 100644
> > --- a/include/qemu/log.h
> > +++ b/include/qemu/log.h
> > @@ -35,6 +35,7 @@ bool qemu_log_separate(void);
> > /* LOG_STRACE is used for user-mode strace logging. */
> > #define LOG_STRACE         (1 << 19)
> > #define LOG_PER_THREAD     (1 << 20)
> > +#define LOG_MEM_ACCESS     (1 << 21)
> >
> > /* Lock/unlock output. */
> >
> > diff --git a/softmmu/memory.c b/softmmu/memory.c
> > index 9d64efca26..0a9fa67d32 100644
> > --- a/softmmu/memory.c
> > +++ b/softmmu/memory.c
> > @@ -1379,7 +1379,7 @@ bool memory_region_access_valid(MemoryRegion *mr,
> > {
> >     if (mr->ops->valid.accepts
> >         && !mr->ops->valid.accepts(mr->opaque, addr, size, is_write, 
> > attrs)) {
> > -        qemu_log_mask(LOG_GUEST_ERROR, "Invalid %s at addr 0x%" 
> > HWADDR_PRIX
> > +        qemu_log_mask(LOG_MEM_ACCESS, "Invalid %s at addr 0x%" HWADDR_PRIX
> >                       ", size %u, region '%s', reason: rejected\n",
> >                       is_write ? "write" : "read",
> >                       addr, size, memory_region_name(mr));
> > @@ -1387,7 +1387,7 @@ bool memory_region_access_valid(MemoryRegion *mr,
> >     }
> >
> >     if (!mr->ops->valid.unaligned && (addr & (size - 1))) {
> > -        qemu_log_mask(LOG_GUEST_ERROR, "Invalid %s at addr 0x%" 
> > HWADDR_PRIX
> > +        qemu_log_mask(LOG_MEM_ACCESS, "Invalid %s at addr 0x%" HWADDR_PRIX
> >                       ", size %u, region '%s', reason: unaligned\n",
> >                       is_write ? "write" : "read",
> >                       addr, size, memory_region_name(mr));
> > @@ -1401,7 +1401,7 @@ bool memory_region_access_valid(MemoryRegion *mr,
> >
> >     if (size > mr->ops->valid.max_access_size
> >         || size < mr->ops->valid.min_access_size) {
> > -        qemu_log_mask(LOG_GUEST_ERROR, "Invalid %s at addr 0x%" 
> > HWADDR_PRIX
> > +        qemu_log_mask(LOG_MEM_ACCESS, "Invalid %s at addr 0x%" HWADDR_PRIX
> >                       ", size %u, region '%s', reason: invalid size "
> >                       "(min:%u max:%u)\n",
> >                       is_write ? "write" : "read",
> > diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> > index bf585e45a8..bca679ee01 100644
> > --- a/softmmu/physmem.c
> > +++ b/softmmu/physmem.c
> > @@ -2792,7 +2792,7 @@ static bool flatview_access_allowed(MemoryRegion *mr, 
> > MemTxAttrs attrs,
> >     if (memory_region_is_ram(mr)) {
> >         return true;
> >     }
> > -    qemu_log_mask(LOG_GUEST_ERROR,
> > +    qemu_log_mask(LOG_MEM_ACCESS,
> >                   "Invalid access to non-RAM device at "
> >                   "addr 0x%" HWADDR_PRIX ", size %" HWADDR_PRIu ", "
> >                   "region '%s'\n", addr, len, memory_region_name(mr));
> > diff --git a/util/log.c b/util/log.c
> > index 7837ff9917..a3c097f320 100644
> > --- a/util/log.c
> > +++ b/util/log.c
> > @@ -495,6 +495,8 @@ const QEMULogItem qemu_log_items[] = {
> >       "log every user-mode syscall, its input, and its result" },
> >     { LOG_PER_THREAD, "tid",
> >       "open a separate log file per thread; filename must contain '%d'" },
> > +    { LOG_MEM_ACCESS, "memaccess",
> > +      "log invalid memory accesses" },
> >     { 0, NULL, NULL },
> > };