| Message ID | 20230119214032.4BF1E7457E7@zero.eik.bme.hu |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] log: Add separate debug option for logging invalid memory accesses | expand |
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? 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 }, > }; > >
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 }, >> }; >> >> > >
On 07/02/2023 17.33, BALATON Zoltan wrote: > 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? Patch makes sense to me and looks fine, so: Reviewed-by: Thomas Huth <thuth@redhat.com> ... I think this should go via one of the "Memory API" maintainers branches? Paolo? Peter? David? Thomas >>> --- >>> 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 }, >>> };
On 19/1/23 22:40, 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> > --- > 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 Can we use LOG_GUEST_ERROR|LOG_MEM_ACCESS to keep current behavior?
On Mon, 13 Feb 2023, Philippe Mathieu-Daudé wrote: > On 19/1/23 22:40, 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> >> --- >> 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 > > Can we use LOG_GUEST_ERROR|LOG_MEM_ACCESS to keep current behavior? No, the point is that I don't want the current behaviour getting mem access logs when I want to see guest error logs as it's littering the output and the real guest error logs are then lost in the lot of unintresting mem accesses. You can still use -d guest_errors,memaccess for the current behaviour if you prefer that but without this patch I have no option for the other behaviour I prefer. Regards, BALATON Zoltan
On Mon, Feb 13, 2023 at 12:41:29PM +0100, Thomas Huth wrote: > On 07/02/2023 17.33, BALATON Zoltan wrote: > > 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? > > Patch makes sense to me and looks fine, so: > > Reviewed-by: Thomas Huth <thuth@redhat.com> > > ... I think this should go via one of the "Memory API" maintainers branches? > Paolo? Peter? David? Paolo normally does the pull, I assume that'll still be the case. The patch looks good to me if Phil's comment will be addressed on merging with the old mask, which makes sense to me: Acked-by: Peter Xu <peterx@redhat.com> Thanks,
On Mon, 13 Feb 2023, Peter Xu wrote: > On Mon, Feb 13, 2023 at 12:41:29PM +0100, Thomas Huth wrote: >> On 07/02/2023 17.33, BALATON Zoltan wrote: >>> 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? >> >> Patch makes sense to me and looks fine, so: >> >> Reviewed-by: Thomas Huth <thuth@redhat.com> >> >> ... I think this should go via one of the "Memory API" maintainers branches? >> Paolo? Peter? David? > > Paolo normally does the pull, I assume that'll still be the case. The > patch looks good to me if Phil's comment will be addressed on merging with > the old mask, which makes sense to me: Keeping the old mask kind of defies the purpose. I've tried to explain that in the commit message but now that two of you did not get it maybe that message needs to be clarified instead? Regards, BALATON Zoltan > Acked-by: Peter Xu <peterx@redhat.com> > > Thanks, > >
On 13/2/23 15:47, BALATON Zoltan wrote: > On Mon, 13 Feb 2023, Peter Xu wrote: >> On Mon, Feb 13, 2023 at 12:41:29PM +0100, Thomas Huth wrote: >>> On 07/02/2023 17.33, BALATON Zoltan wrote: >>>> 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? >>> >>> Patch makes sense to me and looks fine, so: >>> >>> Reviewed-by: Thomas Huth <thuth@redhat.com> >>> >>> ... I think this should go via one of the "Memory API" maintainers >>> branches? >>> Paolo? Peter? David? >> >> Paolo normally does the pull, I assume that'll still be the case. The >> patch looks good to me if Phil's comment will be addressed on merging >> with >> the old mask, which makes sense to me: > > Keeping the old mask kind of defies the purpose. I've tried to explain > that in the commit message but now that two of you did not get it maybe > that message needs to be clarified instead? Is your use case memaccess enabled and guest-errors disabled?
On 13/2/23 15:58, Philippe Mathieu-Daudé wrote: > On 13/2/23 15:47, BALATON Zoltan wrote: >> On Mon, 13 Feb 2023, Peter Xu wrote: >>> On Mon, Feb 13, 2023 at 12:41:29PM +0100, Thomas Huth wrote: >>>> On 07/02/2023 17.33, BALATON Zoltan wrote: >>>>> 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. - misusing hardware => LOG_GUEST_ERROR - accessing missing features => LOG_UNIMP - invalid memory areas => depending on the bus bridge, can be: #define MEMTX_OK 0 #define MEMTX_ERROR (1U << 0) /* device returned an error */ #define MEMTX_DECODE_ERROR (1U << 1) /* nothing at that address */ #define MEMTX_ACCESS_ERROR (1U << 2) /* access denied */ >>>>>>> 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? >>>> >>>> Patch makes sense to me and looks fine, so: >>>> >>>> Reviewed-by: Thomas Huth <thuth@redhat.com> >>>> >>>> ... I think this should go via one of the "Memory API" maintainers >>>> branches? >>>> Paolo? Peter? David? >>> >>> Paolo normally does the pull, I assume that'll still be the case. The >>> patch looks good to me if Phil's comment will be addressed on merging >>> with >>> the old mask, which makes sense to me: >> >> Keeping the old mask kind of defies the purpose. I've tried to explain >> that in the commit message It is hard to justify we need a new mask and scripts have to adapt to your new format. I assume you can not use trace-event because you want this logging available in a RELEASE build, right? >> but now that two of you did not get it >> maybe that message needs to be clarified instead? > > Is your use case memaccess enabled and guest-errors disabled?
On Mon, Feb 13, 2023 at 03:47:42PM +0100, BALATON Zoltan wrote: > On Mon, 13 Feb 2023, Peter Xu wrote: > > On Mon, Feb 13, 2023 at 12:41:29PM +0100, Thomas Huth wrote: > > > On 07/02/2023 17.33, BALATON Zoltan wrote: > > > > 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? > > > > > > Patch makes sense to me and looks fine, so: > > > > > > Reviewed-by: Thomas Huth <thuth@redhat.com> > > > > > > ... I think this should go via one of the "Memory API" maintainers branches? > > > Paolo? Peter? David? > > > > Paolo normally does the pull, I assume that'll still be the case. The > > patch looks good to me if Phil's comment will be addressed on merging with > > the old mask, which makes sense to me: > > Keeping the old mask kind of defies the purpose. I've tried to explain that > in the commit message but now that two of you did not get it maybe that > message needs to be clarified instead? I think it's clear enough. My fault to not read carefully into the message, sorry. However, could you explain why a memory_region_access_valid() failure shouldn't belong to LOG_GUEST_ERROR? commit e54eba1986f6c4bac2951e7f90a849cd842e25e4 Author: Peter Maydell <peter.maydell@linaro.org> Date: Thu Oct 18 14:11:35 2012 +0100 qemu-log: Add new log category for guest bugs Add a new category for device models to log guest behaviour which is likely to be a guest bug of some kind (accessing nonexistent registers, reading 32 bit wide registers with a byte access, etc). Making this its own log category allows those who care (mostly guest OS authors) to see the complaints without bothering most users. Such an illegal memory access is definitely a suitable candidate of guest misbehave to me. Not to mention Phil always have a good point that you may be violating others using guest_error already so what they wanted to capture can misterious going away without noticing, even if it may service your goal. IOW it's a slight ABI and I think we ned justification to break it. Thanks,
On Mon, 13 Feb 2023, Philippe Mathieu-Daudé wrote: > On 13/2/23 15:47, BALATON Zoltan wrote: >> On Mon, 13 Feb 2023, Peter Xu wrote: >>> On Mon, Feb 13, 2023 at 12:41:29PM +0100, Thomas Huth wrote: >>>> On 07/02/2023 17.33, BALATON Zoltan wrote: >>>>> 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? >>>> >>>> Patch makes sense to me and looks fine, so: >>>> >>>> Reviewed-by: Thomas Huth <thuth@redhat.com> >>>> >>>> ... I think this should go via one of the "Memory API" maintainers >>>> branches? >>>> Paolo? Peter? David? >>> >>> Paolo normally does the pull, I assume that'll still be the case. The >>> patch looks good to me if Phil's comment will be addressed on merging with >>> the old mask, which makes sense to me: >> >> Keeping the old mask kind of defies the purpose. I've tried to explain that >> in the commit message but now that two of you did not get it maybe that >> message needs to be clarified instead? > > Is your use case memaccess enabled and guest-errors disabled? Both, I want to be able to enable/disable these separately. Currently guest_errors just controls too much in one flag so I'd like to separate these from each other to be controllable separately. I'll continue in reply to other question. Regards, BALATON Zoltan
On Mon, 13 Feb 2023, Peter Xu wrote: > On Mon, Feb 13, 2023 at 03:47:42PM +0100, BALATON Zoltan wrote: >> On Mon, 13 Feb 2023, Peter Xu wrote: >>> On Mon, Feb 13, 2023 at 12:41:29PM +0100, Thomas Huth wrote: >>>> On 07/02/2023 17.33, BALATON Zoltan wrote: >>>>> 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? >>>> >>>> Patch makes sense to me and looks fine, so: >>>> >>>> Reviewed-by: Thomas Huth <thuth@redhat.com> >>>> >>>> ... I think this should go via one of the "Memory API" maintainers branches? >>>> Paolo? Peter? David? >>> >>> Paolo normally does the pull, I assume that'll still be the case. The >>> patch looks good to me if Phil's comment will be addressed on merging with >>> the old mask, which makes sense to me: >> >> Keeping the old mask kind of defies the purpose. I've tried to explain that >> in the commit message but now that two of you did not get it maybe that >> message needs to be clarified instead? > > I think it's clear enough. My fault to not read carefully into the > message, sorry. > > However, could you explain why a memory_region_access_valid() failure > shouldn't belong to LOG_GUEST_ERROR? > > commit e54eba1986f6c4bac2951e7f90a849cd842e25e4 > Author: Peter Maydell <peter.maydell@linaro.org> > Date: Thu Oct 18 14:11:35 2012 +0100 > > qemu-log: Add new log category for guest bugs > > Add a new category for device models to log guest behaviour > which is likely to be a guest bug of some kind (accessing > nonexistent registers, reading 32 bit wide registers with > a byte access, etc). Making this its own log category allows > those who care (mostly guest OS authors) to see the complaints > without bothering most users. > > Such an illegal memory access is definitely a suitable candidate of guest > misbehave to me. Problem is that a lot of machines have unimplemented hardware that are valid on real machine but we don't model them so running guests which access these generate constant flow of unassigned memory access log which obscures the actual guest_errors when an modelled device is accessed in unexpected ways. For an example you can try booting MorphOS on mac99,via=pmu as described here: http://zero.eik.bme.hu/~balaton/qemu/amiga/#morphos (or the pegasos2 command too). We could add dummy registers to silence these but I think it's better to either implement it correctly or leave it unimplemented so we don't hide errors by the dummy implementation. > Not to mention Phil always have a good point that you may be violating > others using guest_error already so what they wanted to capture can > misterious going away without noticing, even if it may service your goal. > IOW it's a slight ABI and I think we ned justification to break it. Probably this should be documented in changelog or do we need depracation for a debug option meant for developers mostly? I did not think so. Also I can't think of other way to solve this without changing what guest_erorrs do unless we change the name of that flag as well. Also not that when this was originally added it did not contain mem access logs as those were controlled by a define in memory.c until Philippe changed it and added them to guest_errors. So in a way I want the previous functionality back. Regards, BALATON Zoltan
On Mon, 13 Feb 2023, Philippe Mathieu-Daudé wrote: > On 13/2/23 15:58, Philippe Mathieu-Daudé wrote: >> On 13/2/23 15:47, BALATON Zoltan wrote: >>> On Mon, 13 Feb 2023, Peter Xu wrote: >>>> On Mon, Feb 13, 2023 at 12:41:29PM +0100, Thomas Huth wrote: >>>>> On 07/02/2023 17.33, BALATON Zoltan wrote: >>>>>> 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. > > - misusing hardware => LOG_GUEST_ERROR > - accessing missing features => LOG_UNIMP > - invalid memory areas => depending on the bus bridge, can be: > > #define MEMTX_OK 0 > #define MEMTX_ERROR (1U << 0) /* device returned an error */ > #define MEMTX_DECODE_ERROR (1U << 1) /* nothing at that address */ > #define MEMTX_ACCESS_ERROR (1U << 2) /* access denied */ > >>>>>>>> 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? >>>>> >>>>> Patch makes sense to me and looks fine, so: >>>>> >>>>> Reviewed-by: Thomas Huth <thuth@redhat.com> >>>>> >>>>> ... I think this should go via one of the "Memory API" maintainers >>>>> branches? >>>>> Paolo? Peter? David? >>>> >>>> Paolo normally does the pull, I assume that'll still be the case. The >>>> patch looks good to me if Phil's comment will be addressed on merging >>>> with >>>> the old mask, which makes sense to me: >>> >>> Keeping the old mask kind of defies the purpose. I've tried to explain >>> that in the commit message > > It is hard to justify we need a new mask and scripts have to adapt to > your new format. I assume you can not use trace-event because you want > this logging available in a RELEASE build, right? Guest errors are useful in a release build, unassigned access probably less so but since I want to separate them from guest_errors I don't see how logging mem access via trace would be any different for your concerns. Regards, BALATON Zoltan
On Mon, Feb 13, 2023 at 05:34:04PM +0100, BALATON Zoltan wrote: > On Mon, 13 Feb 2023, Peter Xu wrote: > > On Mon, Feb 13, 2023 at 03:47:42PM +0100, BALATON Zoltan wrote: > > > On Mon, 13 Feb 2023, Peter Xu wrote: > > > > On Mon, Feb 13, 2023 at 12:41:29PM +0100, Thomas Huth wrote: > > > > > On 07/02/2023 17.33, BALATON Zoltan wrote: > > > > > > 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? > > > > > > > > > > Patch makes sense to me and looks fine, so: > > > > > > > > > > Reviewed-by: Thomas Huth <thuth@redhat.com> > > > > > > > > > > ... I think this should go via one of the "Memory API" maintainers branches? > > > > > Paolo? Peter? David? > > > > > > > > Paolo normally does the pull, I assume that'll still be the case. The > > > > patch looks good to me if Phil's comment will be addressed on merging with > > > > the old mask, which makes sense to me: > > > > > > Keeping the old mask kind of defies the purpose. I've tried to explain that > > > in the commit message but now that two of you did not get it maybe that > > > message needs to be clarified instead? > > > > I think it's clear enough. My fault to not read carefully into the > > message, sorry. > > > > However, could you explain why a memory_region_access_valid() failure > > shouldn't belong to LOG_GUEST_ERROR? > > > > commit e54eba1986f6c4bac2951e7f90a849cd842e25e4 > > Author: Peter Maydell <peter.maydell@linaro.org> > > Date: Thu Oct 18 14:11:35 2012 +0100 > > > > qemu-log: Add new log category for guest bugs > > > > Add a new category for device models to log guest behaviour > > which is likely to be a guest bug of some kind (accessing > > nonexistent registers, reading 32 bit wide registers with > > a byte access, etc). Making this its own log category allows > > those who care (mostly guest OS authors) to see the complaints > > without bothering most users. > > > > Such an illegal memory access is definitely a suitable candidate of guest > > misbehave to me. > > Problem is that a lot of machines have unimplemented hardware that are valid > on real machine but we don't model them so running guests which access these > generate constant flow of unassigned memory access log which obscures the > actual guest_errors when an modelled device is accessed in unexpected ways. > For an example you can try booting MorphOS on mac99,via=pmu as described > here: http://zero.eik.bme.hu/~balaton/qemu/amiga/#morphos > (or the pegasos2 command too). We could add dummy registers to silence these > but I think it's better to either implement it correctly or leave it > unimplemented so we don't hide errors by the dummy implementation. > > > Not to mention Phil always have a good point that you may be violating > > others using guest_error already so what they wanted to capture can > > misterious going away without noticing, even if it may service your goal. > > IOW it's a slight ABI and I think we ned justification to break it. > > Probably this should be documented in changelog or do we need depracation > for a debug option meant for developers mostly? I did not think so. Also I > can't think of other way to solve this without changing what guest_erorrs do > unless we change the name of that flag as well. Also not that when this was > originally added it did not contain mem access logs as those were controlled > by a define in memory.c until Philippe changed it and added them to > guest_errors. So in a way I want the previous functionality back. I see, thanks. Indeed it's only a debug option, so I don't know whether the abi needs the attention here. I quickly looked at all the masks and afaict this is really a special and very useful one that if I'm a cloud provider I can run some script trying to capture those violations using this bit to identify suspecious guests. So I think it would still be great to not break it if possible, IMHO. Since currently I don't see an immediate limitation of having qemu log mask being a single bit for each of the entry, one way to satisfy your need (and also keep the old behavior, iiuc), is to make guest_errors a sugar syntax to cover 2 bits. It shouldn't be complicated at all, I assume: +/* This covers the generic guest errors besides memory violations */ #define LOG_GUEST_ERROR (1 << 11) +/* + * This covers the guest errors on memory violations; see LOG_GUEST_ERROR + * for generic guest errors. + */ +#define LOG_GUEST_ERROR_MEM (1 << 21) +#define LOG_GUEST_ERROR_ALL (LOG_GUEST_ERROR | LOG_GUEST_ERROR_MEM) - { LOG_GUEST_ERROR, "guest_errors", + { LOG_GUEST_ERROR_ALL, "guest_errors", Then somehow squashed with your changes. It'll make "guest_errors" not exactly matching the name of LOG_* but I think it may not be a big concern. Thanks,
On 13/2/23 18:17, Peter Xu wrote: > On Mon, Feb 13, 2023 at 05:34:04PM +0100, BALATON Zoltan wrote: >> On Mon, 13 Feb 2023, Peter Xu wrote: >>> On Mon, Feb 13, 2023 at 03:47:42PM +0100, BALATON Zoltan wrote: >>>> On Mon, 13 Feb 2023, Peter Xu wrote: >>>>> On Mon, Feb 13, 2023 at 12:41:29PM +0100, Thomas Huth wrote: >>>>>> On 07/02/2023 17.33, BALATON Zoltan wrote: >>>>>>> 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? >>>>>> >>>>>> Patch makes sense to me and looks fine, so: >>>>>> >>>>>> Reviewed-by: Thomas Huth <thuth@redhat.com> >>>>>> >>>>>> ... I think this should go via one of the "Memory API" maintainers branches? >>>>>> Paolo? Peter? David? >>>>> >>>>> Paolo normally does the pull, I assume that'll still be the case. The >>>>> patch looks good to me if Phil's comment will be addressed on merging with >>>>> the old mask, which makes sense to me: >>>> >>>> Keeping the old mask kind of defies the purpose. I've tried to explain that >>>> in the commit message but now that two of you did not get it maybe that >>>> message needs to be clarified instead? >>> >>> I think it's clear enough. My fault to not read carefully into the >>> message, sorry. >>> >>> However, could you explain why a memory_region_access_valid() failure >>> shouldn't belong to LOG_GUEST_ERROR? >>> >>> commit e54eba1986f6c4bac2951e7f90a849cd842e25e4 >>> Author: Peter Maydell <peter.maydell@linaro.org> >>> Date: Thu Oct 18 14:11:35 2012 +0100 >>> >>> qemu-log: Add new log category for guest bugs >>> >>> Add a new category for device models to log guest behaviour >>> which is likely to be a guest bug of some kind (accessing >>> nonexistent registers, reading 32 bit wide registers with >>> a byte access, etc). Making this its own log category allows >>> those who care (mostly guest OS authors) to see the complaints >>> without bothering most users. >>> >>> Such an illegal memory access is definitely a suitable candidate of guest >>> misbehave to me. >> >> Problem is that a lot of machines have unimplemented hardware that are valid >> on real machine but we don't model them so running guests which access these >> generate constant flow of unassigned memory access log which obscures the >> actual guest_errors when an modelled device is accessed in unexpected ways. >> For an example you can try booting MorphOS on mac99,via=pmu as described >> here: http://zero.eik.bme.hu/~balaton/qemu/amiga/#morphos >> (or the pegasos2 command too). We could add dummy registers to silence these >> but I think it's better to either implement it correctly or leave it >> unimplemented so we don't hide errors by the dummy implementation. >> >>> Not to mention Phil always have a good point that you may be violating >>> others using guest_error already so what they wanted to capture can >>> misterious going away without noticing, even if it may service your goal. >>> IOW it's a slight ABI and I think we ned justification to break it. >> >> Probably this should be documented in changelog or do we need depracation >> for a debug option meant for developers mostly? I did not think so. Also I >> can't think of other way to solve this without changing what guest_erorrs do >> unless we change the name of that flag as well. Also not that when this was >> originally added it did not contain mem access logs as those were controlled >> by a define in memory.c until Philippe changed it and added them to >> guest_errors. So in a way I want the previous functionality back. > > I see, thanks. > > Indeed it's only a debug option, so I don't know whether the abi needs the > attention here. > > I quickly looked at all the masks and afaict this is really a special and > very useful one that if I'm a cloud provider I can run some script trying > to capture those violations using this bit to identify suspecious guests. > > So I think it would still be great to not break it if possible, IMHO. > > Since currently I don't see an immediate limitation of having qemu log mask > being a single bit for each of the entry, one way to satisfy your need (and > also keep the old behavior, iiuc), is to make guest_errors a sugar syntax > to cover 2 bits. It shouldn't be complicated at all, I assume: > > +/* This covers the generic guest errors besides memory violations */ > #define LOG_GUEST_ERROR (1 << 11) > > +/* > + * This covers the guest errors on memory violations; see LOG_GUEST_ERROR > + * for generic guest errors. > + */ > +#define LOG_GUEST_ERROR_MEM (1 << 21) > +#define LOG_GUEST_ERROR_ALL (LOG_GUEST_ERROR | LOG_GUEST_ERROR_MEM) > > - { LOG_GUEST_ERROR, "guest_errors", > + { LOG_GUEST_ERROR_ALL, "guest_errors", > > Then somehow squashed with your changes. It'll make "guest_errors" not > exactly matching the name of LOG_* but I think it may not be a big concern. Thanks Peter for having a look. Cc'ing libvir-list@ to see if this change could have an impact. If no one else object I'm OK to take the patch as is, except if you (Peter) want the description to be reworded. Regards, Phil.
On Mon, 13 Feb 2023, Peter Xu wrote: > On Mon, Feb 13, 2023 at 05:34:04PM +0100, BALATON Zoltan wrote: >> On Mon, 13 Feb 2023, Peter Xu wrote: >>> On Mon, Feb 13, 2023 at 03:47:42PM +0100, BALATON Zoltan wrote: >>>> On Mon, 13 Feb 2023, Peter Xu wrote: >>>>> On Mon, Feb 13, 2023 at 12:41:29PM +0100, Thomas Huth wrote: >>>>>> On 07/02/2023 17.33, BALATON Zoltan wrote: >>>>>>> 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? >>>>>> >>>>>> Patch makes sense to me and looks fine, so: >>>>>> >>>>>> Reviewed-by: Thomas Huth <thuth@redhat.com> >>>>>> >>>>>> ... I think this should go via one of the "Memory API" maintainers branches? >>>>>> Paolo? Peter? David? >>>>> >>>>> Paolo normally does the pull, I assume that'll still be the case. The >>>>> patch looks good to me if Phil's comment will be addressed on merging with >>>>> the old mask, which makes sense to me: >>>> >>>> Keeping the old mask kind of defies the purpose. I've tried to explain that >>>> in the commit message but now that two of you did not get it maybe that >>>> message needs to be clarified instead? >>> >>> I think it's clear enough. My fault to not read carefully into the >>> message, sorry. >>> >>> However, could you explain why a memory_region_access_valid() failure >>> shouldn't belong to LOG_GUEST_ERROR? >>> >>> commit e54eba1986f6c4bac2951e7f90a849cd842e25e4 >>> Author: Peter Maydell <peter.maydell@linaro.org> >>> Date: Thu Oct 18 14:11:35 2012 +0100 >>> >>> qemu-log: Add new log category for guest bugs >>> >>> Add a new category for device models to log guest behaviour >>> which is likely to be a guest bug of some kind (accessing >>> nonexistent registers, reading 32 bit wide registers with >>> a byte access, etc). Making this its own log category allows >>> those who care (mostly guest OS authors) to see the complaints >>> without bothering most users. >>> >>> Such an illegal memory access is definitely a suitable candidate of guest >>> misbehave to me. >> >> Problem is that a lot of machines have unimplemented hardware that are valid >> on real machine but we don't model them so running guests which access these >> generate constant flow of unassigned memory access log which obscures the >> actual guest_errors when an modelled device is accessed in unexpected ways. >> For an example you can try booting MorphOS on mac99,via=pmu as described >> here: http://zero.eik.bme.hu/~balaton/qemu/amiga/#morphos >> (or the pegasos2 command too). We could add dummy registers to silence these >> but I think it's better to either implement it correctly or leave it >> unimplemented so we don't hide errors by the dummy implementation. >> >>> Not to mention Phil always have a good point that you may be violating >>> others using guest_error already so what they wanted to capture can >>> misterious going away without noticing, even if it may service your goal. >>> IOW it's a slight ABI and I think we ned justification to break it. >> >> Probably this should be documented in changelog or do we need depracation >> for a debug option meant for developers mostly? I did not think so. Also I >> can't think of other way to solve this without changing what guest_erorrs do >> unless we change the name of that flag as well. Also not that when this was >> originally added it did not contain mem access logs as those were controlled >> by a define in memory.c until Philippe changed it and added them to >> guest_errors. So in a way I want the previous functionality back. > > I see, thanks. > > Indeed it's only a debug option, so I don't know whether the abi needs the > attention here. > > I quickly looked at all the masks and afaict this is really a special and > very useful one that if I'm a cloud provider I can run some script trying > to capture those violations using this bit to identify suspecious guests. > > So I think it would still be great to not break it if possible, IMHO. > > Since currently I don't see an immediate limitation of having qemu log mask > being a single bit for each of the entry, one way to satisfy your need (and > also keep the old behavior, iiuc), is to make guest_errors a sugar syntax > to cover 2 bits. It shouldn't be complicated at all, I assume: > > +/* This covers the generic guest errors besides memory violations */ > #define LOG_GUEST_ERROR (1 << 11) > > +/* > + * This covers the guest errors on memory violations; see LOG_GUEST_ERROR > + * for generic guest errors. > + */ > +#define LOG_GUEST_ERROR_MEM (1 << 21) > +#define LOG_GUEST_ERROR_ALL (LOG_GUEST_ERROR | LOG_GUEST_ERROR_MEM) > > - { LOG_GUEST_ERROR, "guest_errors", > + { LOG_GUEST_ERROR_ALL, "guest_errors", > > Then somehow squashed with your changes. It'll make "guest_errors" not > exactly matching the name of LOG_* but I think it may not be a big concern. I'm not sure I understand this. So -d memaccess would give me the unassigned logs, that's fine and -d guest_errors are both LOG_GUEST_ERROR and memaccess like currently but what option would give me just the guest_Errors before mem access started to use this flag too? (I could not locate the commit that changed this but I remember previously the unassigned mem logs were enabled with a define in memory.c.) Do we need another -d option for just the guest errors then? What should that be called? Regards, BALATON Zoltan
On Mon, Feb 13, 2023 at 07:34:55PM +0100, BALATON Zoltan wrote: > On Mon, 13 Feb 2023, Peter Xu wrote: > > On Mon, Feb 13, 2023 at 05:34:04PM +0100, BALATON Zoltan wrote: > > > On Mon, 13 Feb 2023, Peter Xu wrote: > > > > On Mon, Feb 13, 2023 at 03:47:42PM +0100, BALATON Zoltan wrote: > > > > > On Mon, 13 Feb 2023, Peter Xu wrote: > > > > > > On Mon, Feb 13, 2023 at 12:41:29PM +0100, Thomas Huth wrote: > > > > > > > On 07/02/2023 17.33, BALATON Zoltan wrote: > > > > > > > > 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? > > > > > > > > > > > > > > Patch makes sense to me and looks fine, so: > > > > > > > > > > > > > > Reviewed-by: Thomas Huth <thuth@redhat.com> > > > > > > > > > > > > > > ... I think this should go via one of the "Memory API" maintainers branches? > > > > > > > Paolo? Peter? David? > > > > > > > > > > > > Paolo normally does the pull, I assume that'll still be the case. The > > > > > > patch looks good to me if Phil's comment will be addressed on merging with > > > > > > the old mask, which makes sense to me: > > > > > > > > > > Keeping the old mask kind of defies the purpose. I've tried to explain that > > > > > in the commit message but now that two of you did not get it maybe that > > > > > message needs to be clarified instead? > > > > > > > > I think it's clear enough. My fault to not read carefully into the > > > > message, sorry. > > > > > > > > However, could you explain why a memory_region_access_valid() failure > > > > shouldn't belong to LOG_GUEST_ERROR? > > > > > > > > commit e54eba1986f6c4bac2951e7f90a849cd842e25e4 > > > > Author: Peter Maydell <peter.maydell@linaro.org> > > > > Date: Thu Oct 18 14:11:35 2012 +0100 > > > > > > > > qemu-log: Add new log category for guest bugs > > > > > > > > Add a new category for device models to log guest behaviour > > > > which is likely to be a guest bug of some kind (accessing > > > > nonexistent registers, reading 32 bit wide registers with > > > > a byte access, etc). Making this its own log category allows > > > > those who care (mostly guest OS authors) to see the complaints > > > > without bothering most users. > > > > > > > > Such an illegal memory access is definitely a suitable candidate of guest > > > > misbehave to me. > > > > > > Problem is that a lot of machines have unimplemented hardware that are valid > > > on real machine but we don't model them so running guests which access these > > > generate constant flow of unassigned memory access log which obscures the > > > actual guest_errors when an modelled device is accessed in unexpected ways. > > > For an example you can try booting MorphOS on mac99,via=pmu as described > > > here: http://zero.eik.bme.hu/~balaton/qemu/amiga/#morphos > > > (or the pegasos2 command too). We could add dummy registers to silence these > > > but I think it's better to either implement it correctly or leave it > > > unimplemented so we don't hide errors by the dummy implementation. > > > > > > > Not to mention Phil always have a good point that you may be violating > > > > others using guest_error already so what they wanted to capture can > > > > misterious going away without noticing, even if it may service your goal. > > > > IOW it's a slight ABI and I think we ned justification to break it. > > > > > > Probably this should be documented in changelog or do we need depracation > > > for a debug option meant for developers mostly? I did not think so. Also I > > > can't think of other way to solve this without changing what guest_erorrs do > > > unless we change the name of that flag as well. Also not that when this was > > > originally added it did not contain mem access logs as those were controlled > > > by a define in memory.c until Philippe changed it and added them to > > > guest_errors. So in a way I want the previous functionality back. > > > > I see, thanks. > > > > Indeed it's only a debug option, so I don't know whether the abi needs the > > attention here. > > > > I quickly looked at all the masks and afaict this is really a special and > > very useful one that if I'm a cloud provider I can run some script trying > > to capture those violations using this bit to identify suspecious guests. > > > > So I think it would still be great to not break it if possible, IMHO. > > > > Since currently I don't see an immediate limitation of having qemu log mask > > being a single bit for each of the entry, one way to satisfy your need (and > > also keep the old behavior, iiuc), is to make guest_errors a sugar syntax > > to cover 2 bits. It shouldn't be complicated at all, I assume: > > > > +/* This covers the generic guest errors besides memory violations */ > > #define LOG_GUEST_ERROR (1 << 11) > > > > +/* > > + * This covers the guest errors on memory violations; see LOG_GUEST_ERROR > > + * for generic guest errors. > > + */ > > +#define LOG_GUEST_ERROR_MEM (1 << 21) > > +#define LOG_GUEST_ERROR_ALL (LOG_GUEST_ERROR | LOG_GUEST_ERROR_MEM) > > > > - { LOG_GUEST_ERROR, "guest_errors", > > + { LOG_GUEST_ERROR_ALL, "guest_errors", > > > > Then somehow squashed with your changes. It'll make "guest_errors" not > > exactly matching the name of LOG_* but I think it may not be a big concern. > > I'm not sure I understand this. So -d memaccess would give me the unassigned > logs, that's fine and -d guest_errors are both LOG_GUEST_ERROR and memaccess > like currently but what option would give me just the guest_Errors before > mem access started to use this flag too? (I could not locate the commit that > changed this but I remember previously the unassigned mem logs were enabled > with a define in memory.c.) Do we need another -d option for just the guest > errors then? What should that be called? I forgot to add those two definitions into qemu_log_items just now. It can be defined as: - "guest_errors_common" for !mem errors - "guest_errors_mem" for mem errors - "guest_errors" for mem+!mem (compatible to the old code) With the two lines added: - { LOG_GUEST_ERROR, "guest_errors", + { LOG_GUEST_ERROR_ALL, "guest_errors", "log when the guest OS does something invalid (eg accessing a\n" "non-existent register)" }, + { LOG_GUEST_ERROR, "guest_errors_common", "..." }, + { LOG_GUEST_ERROR_MEM, "guest_errors_mem", "..." }, I saw that Phil revoked his concern, I don't have a strong opinion personally, assuming Phil knows better on that since he modified the memory loggings before. If all are happy with this, please proceed with either way. Thanks,
On Mon, 13 Feb 2023, Peter Xu wrote: > On Mon, Feb 13, 2023 at 07:34:55PM +0100, BALATON Zoltan wrote: >> On Mon, 13 Feb 2023, Peter Xu wrote: >>> On Mon, Feb 13, 2023 at 05:34:04PM +0100, BALATON Zoltan wrote: >>>> On Mon, 13 Feb 2023, Peter Xu wrote: >>>>> On Mon, Feb 13, 2023 at 03:47:42PM +0100, BALATON Zoltan wrote: >>>>>> On Mon, 13 Feb 2023, Peter Xu wrote: >>>>>>> On Mon, Feb 13, 2023 at 12:41:29PM +0100, Thomas Huth wrote: >>>>>>>> On 07/02/2023 17.33, BALATON Zoltan wrote: >>>>>>>>> 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? >>>>>>>> >>>>>>>> Patch makes sense to me and looks fine, so: >>>>>>>> >>>>>>>> Reviewed-by: Thomas Huth <thuth@redhat.com> >>>>>>>> >>>>>>>> ... I think this should go via one of the "Memory API" maintainers branches? >>>>>>>> Paolo? Peter? David? >>>>>>> >>>>>>> Paolo normally does the pull, I assume that'll still be the case. The >>>>>>> patch looks good to me if Phil's comment will be addressed on merging with >>>>>>> the old mask, which makes sense to me: >>>>>> >>>>>> Keeping the old mask kind of defies the purpose. I've tried to explain that >>>>>> in the commit message but now that two of you did not get it maybe that >>>>>> message needs to be clarified instead? >>>>> >>>>> I think it's clear enough. My fault to not read carefully into the >>>>> message, sorry. >>>>> >>>>> However, could you explain why a memory_region_access_valid() failure >>>>> shouldn't belong to LOG_GUEST_ERROR? >>>>> >>>>> commit e54eba1986f6c4bac2951e7f90a849cd842e25e4 >>>>> Author: Peter Maydell <peter.maydell@linaro.org> >>>>> Date: Thu Oct 18 14:11:35 2012 +0100 >>>>> >>>>> qemu-log: Add new log category for guest bugs >>>>> >>>>> Add a new category for device models to log guest behaviour >>>>> which is likely to be a guest bug of some kind (accessing >>>>> nonexistent registers, reading 32 bit wide registers with >>>>> a byte access, etc). Making this its own log category allows >>>>> those who care (mostly guest OS authors) to see the complaints >>>>> without bothering most users. >>>>> >>>>> Such an illegal memory access is definitely a suitable candidate of guest >>>>> misbehave to me. >>>> >>>> Problem is that a lot of machines have unimplemented hardware that are valid >>>> on real machine but we don't model them so running guests which access these >>>> generate constant flow of unassigned memory access log which obscures the >>>> actual guest_errors when an modelled device is accessed in unexpected ways. >>>> For an example you can try booting MorphOS on mac99,via=pmu as described >>>> here: http://zero.eik.bme.hu/~balaton/qemu/amiga/#morphos >>>> (or the pegasos2 command too). We could add dummy registers to silence these >>>> but I think it's better to either implement it correctly or leave it >>>> unimplemented so we don't hide errors by the dummy implementation. >>>> >>>>> Not to mention Phil always have a good point that you may be violating >>>>> others using guest_error already so what they wanted to capture can >>>>> misterious going away without noticing, even if it may service your goal. >>>>> IOW it's a slight ABI and I think we ned justification to break it. >>>> >>>> Probably this should be documented in changelog or do we need depracation >>>> for a debug option meant for developers mostly? I did not think so. Also I >>>> can't think of other way to solve this without changing what guest_erorrs do >>>> unless we change the name of that flag as well. Also not that when this was >>>> originally added it did not contain mem access logs as those were controlled >>>> by a define in memory.c until Philippe changed it and added them to >>>> guest_errors. So in a way I want the previous functionality back. >>> >>> I see, thanks. >>> >>> Indeed it's only a debug option, so I don't know whether the abi needs the >>> attention here. >>> >>> I quickly looked at all the masks and afaict this is really a special and >>> very useful one that if I'm a cloud provider I can run some script trying >>> to capture those violations using this bit to identify suspecious guests. >>> >>> So I think it would still be great to not break it if possible, IMHO. >>> >>> Since currently I don't see an immediate limitation of having qemu log mask >>> being a single bit for each of the entry, one way to satisfy your need (and >>> also keep the old behavior, iiuc), is to make guest_errors a sugar syntax >>> to cover 2 bits. It shouldn't be complicated at all, I assume: >>> >>> +/* This covers the generic guest errors besides memory violations */ >>> #define LOG_GUEST_ERROR (1 << 11) >>> >>> +/* >>> + * This covers the guest errors on memory violations; see LOG_GUEST_ERROR >>> + * for generic guest errors. >>> + */ >>> +#define LOG_GUEST_ERROR_MEM (1 << 21) >>> +#define LOG_GUEST_ERROR_ALL (LOG_GUEST_ERROR | LOG_GUEST_ERROR_MEM) >>> >>> - { LOG_GUEST_ERROR, "guest_errors", >>> + { LOG_GUEST_ERROR_ALL, "guest_errors", >>> >>> Then somehow squashed with your changes. It'll make "guest_errors" not >>> exactly matching the name of LOG_* but I think it may not be a big concern. >> >> I'm not sure I understand this. So -d memaccess would give me the unassigned >> logs, that's fine and -d guest_errors are both LOG_GUEST_ERROR and memaccess >> like currently but what option would give me just the guest_Errors before >> mem access started to use this flag too? (I could not locate the commit that >> changed this but I remember previously the unassigned mem logs were enabled >> with a define in memory.c.) Do we need another -d option for just the guest >> errors then? What should that be called? > > I forgot to add those two definitions into qemu_log_items just now. It can > be defined as: > > - "guest_errors_common" for !mem errors > - "guest_errors_mem" for mem errors > - "guest_errors" for mem+!mem (compatible to the old code) OK I get that now but I would still like to call these differently. For one these are very long and also not quite clear. Unassigned mem access is most often not a guest error but error in emulating hardware, that is guest accessing devices we don't model will result in these so the guest does nothing wrong is't just missing emulation. Therefore I'd keep the flag called LOG_MEM_ACCESS and corresponding -d memaccess option. We can keep guest_errors to do what it does now setting both flags but then we need a new name for LOG_GUEST_ERROR alone. We could call that -d guest_error and then deprecate guest_errors to eventually get everybody convert to use -d guest_error,memeaccess instead which would also match the flags but I'm also OK with some other -d option name for just LOG_GUEST_ERROR however I don't think renaming the LOG_GUEST_ERROR flag is a good idea so that would result in mismatch between flag and option, but that could be OK. > With the two lines added: > > - { LOG_GUEST_ERROR, "guest_errors", > + { LOG_GUEST_ERROR_ALL, "guest_errors", > "log when the guest OS does something invalid (eg accessing a\n" > "non-existent register)" }, > + { LOG_GUEST_ERROR, "guest_errors_common", "..." }, > + { LOG_GUEST_ERROR_MEM, "guest_errors_mem", "..." }, > > I saw that Phil revoked his concern, I don't have a strong opinion > personally, assuming Phil knows better on that since he modified the memory > loggings before. If all are happy with this, please proceed with either > way. IIUC he was OK with adding memaccess as long as guest_errors will not change so together with this proposal but he can clarify what he prefers. I just want separate -d options for memaccess and the rest that's now in guest_errors the way it was before mem access errors were added to this flag. Regards, BALATON Zoltan
On Mon, 13 Feb 2023, BALATON Zoltan wrote: > On Mon, 13 Feb 2023, Peter Xu wrote: >> On Mon, Feb 13, 2023 at 07:34:55PM +0100, BALATON Zoltan wrote: >>> On Mon, 13 Feb 2023, Peter Xu wrote: >>>> On Mon, Feb 13, 2023 at 05:34:04PM +0100, BALATON Zoltan wrote: >>>>> On Mon, 13 Feb 2023, Peter Xu wrote: >>>>>> On Mon, Feb 13, 2023 at 03:47:42PM +0100, BALATON Zoltan wrote: >>>>>>> On Mon, 13 Feb 2023, Peter Xu wrote: >>>>>>>> On Mon, Feb 13, 2023 at 12:41:29PM +0100, Thomas Huth wrote: >>>>>>>>> On 07/02/2023 17.33, BALATON Zoltan wrote: >>>>>>>>>> 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? >>>>>>>>> >>>>>>>>> Patch makes sense to me and looks fine, so: >>>>>>>>> >>>>>>>>> Reviewed-by: Thomas Huth <thuth@redhat.com> >>>>>>>>> >>>>>>>>> ... I think this should go via one of the "Memory API" maintainers >>>>>>>>> branches? >>>>>>>>> Paolo? Peter? David? >>>>>>>> >>>>>>>> Paolo normally does the pull, I assume that'll still be the case. >>>>>>>> The >>>>>>>> patch looks good to me if Phil's comment will be addressed on merging >>>>>>>> with >>>>>>>> the old mask, which makes sense to me: >>>>>>> >>>>>>> Keeping the old mask kind of defies the purpose. I've tried to explain >>>>>>> that >>>>>>> in the commit message but now that two of you did not get it maybe >>>>>>> that >>>>>>> message needs to be clarified instead? >>>>>> >>>>>> I think it's clear enough. My fault to not read carefully into the >>>>>> message, sorry. >>>>>> >>>>>> However, could you explain why a memory_region_access_valid() failure >>>>>> shouldn't belong to LOG_GUEST_ERROR? >>>>>> >>>>>> commit e54eba1986f6c4bac2951e7f90a849cd842e25e4 >>>>>> Author: Peter Maydell <peter.maydell@linaro.org> >>>>>> Date: Thu Oct 18 14:11:35 2012 +0100 >>>>>> >>>>>> qemu-log: Add new log category for guest bugs >>>>>> >>>>>> Add a new category for device models to log guest behaviour >>>>>> which is likely to be a guest bug of some kind (accessing >>>>>> nonexistent registers, reading 32 bit wide registers with >>>>>> a byte access, etc). Making this its own log category allows >>>>>> those who care (mostly guest OS authors) to see the complaints >>>>>> without bothering most users. >>>>>> >>>>>> Such an illegal memory access is definitely a suitable candidate of >>>>>> guest >>>>>> misbehave to me. >>>>> >>>>> Problem is that a lot of machines have unimplemented hardware that are >>>>> valid >>>>> on real machine but we don't model them so running guests which access >>>>> these >>>>> generate constant flow of unassigned memory access log which obscures >>>>> the >>>>> actual guest_errors when an modelled device is accessed in unexpected >>>>> ways. >>>>> For an example you can try booting MorphOS on mac99,via=pmu as described >>>>> here: http://zero.eik.bme.hu/~balaton/qemu/amiga/#morphos >>>>> (or the pegasos2 command too). We could add dummy registers to silence >>>>> these >>>>> but I think it's better to either implement it correctly or leave it >>>>> unimplemented so we don't hide errors by the dummy implementation. >>>>> >>>>>> Not to mention Phil always have a good point that you may be violating >>>>>> others using guest_error already so what they wanted to capture can >>>>>> misterious going away without noticing, even if it may service your >>>>>> goal. >>>>>> IOW it's a slight ABI and I think we ned justification to break it. >>>>> >>>>> Probably this should be documented in changelog or do we need >>>>> depracation >>>>> for a debug option meant for developers mostly? I did not think so. Also >>>>> I >>>>> can't think of other way to solve this without changing what >>>>> guest_erorrs do >>>>> unless we change the name of that flag as well. Also not that when this >>>>> was >>>>> originally added it did not contain mem access logs as those were >>>>> controlled >>>>> by a define in memory.c until Philippe changed it and added them to >>>>> guest_errors. So in a way I want the previous functionality back. >>>> >>>> I see, thanks. >>>> >>>> Indeed it's only a debug option, so I don't know whether the abi needs >>>> the >>>> attention here. >>>> >>>> I quickly looked at all the masks and afaict this is really a special and >>>> very useful one that if I'm a cloud provider I can run some script trying >>>> to capture those violations using this bit to identify suspecious guests. >>>> >>>> So I think it would still be great to not break it if possible, IMHO. >>>> >>>> Since currently I don't see an immediate limitation of having qemu log >>>> mask >>>> being a single bit for each of the entry, one way to satisfy your need >>>> (and >>>> also keep the old behavior, iiuc), is to make guest_errors a sugar syntax >>>> to cover 2 bits. It shouldn't be complicated at all, I assume: >>>> >>>> +/* This covers the generic guest errors besides memory violations */ >>>> #define LOG_GUEST_ERROR (1 << 11) >>>> >>>> +/* >>>> + * This covers the guest errors on memory violations; see >>>> LOG_GUEST_ERROR >>>> + * for generic guest errors. >>>> + */ >>>> +#define LOG_GUEST_ERROR_MEM (1 << 21) >>>> +#define LOG_GUEST_ERROR_ALL (LOG_GUEST_ERROR | LOG_GUEST_ERROR_MEM) >>>> >>>> - { LOG_GUEST_ERROR, "guest_errors", >>>> + { LOG_GUEST_ERROR_ALL, "guest_errors", >>>> >>>> Then somehow squashed with your changes. It'll make "guest_errors" not >>>> exactly matching the name of LOG_* but I think it may not be a big >>>> concern. >>> >>> I'm not sure I understand this. So -d memaccess would give me the >>> unassigned >>> logs, that's fine and -d guest_errors are both LOG_GUEST_ERROR and >>> memaccess >>> like currently but what option would give me just the guest_Errors before >>> mem access started to use this flag too? (I could not locate the commit >>> that >>> changed this but I remember previously the unassigned mem logs were >>> enabled >>> with a define in memory.c.) Do we need another -d option for just the >>> guest >>> errors then? What should that be called? >> >> I forgot to add those two definitions into qemu_log_items just now. It can >> be defined as: >> >> - "guest_errors_common" for !mem errors >> - "guest_errors_mem" for mem errors >> - "guest_errors" for mem+!mem (compatible to the old code) > > OK I get that now but I would still like to call these differently. For one > these are very long and also not quite clear. Unassigned mem access is most > often not a guest error but error in emulating hardware, that is guest > accessing devices we don't model will result in these so the guest does > nothing wrong is't just missing emulation. Therefore I'd keep the flag called > LOG_MEM_ACCESS and corresponding -d memaccess option. We can keep > guest_errors to do what it does now setting both flags but then we need a new > name for LOG_GUEST_ERROR alone. We could call that -d guest_error and then > deprecate guest_errors to eventually get everybody convert to use -d > guest_error,memeaccess instead which would also match the flags but I'm also > OK with some other -d option name for just LOG_GUEST_ERROR however I don't > think renaming the LOG_GUEST_ERROR flag is a good idea so that would result > in mismatch between flag and option, but that could be OK. > >> With the two lines added: >> >> - { LOG_GUEST_ERROR, "guest_errors", >> + { LOG_GUEST_ERROR_ALL, "guest_errors", >> "log when the guest OS does something invalid (eg accessing a\n" >> "non-existent register)" }, >> + { LOG_GUEST_ERROR, "guest_errors_common", "..." }, >> + { LOG_GUEST_ERROR_MEM, "guest_errors_mem", "..." }, >> >> I saw that Phil revoked his concern, I don't have a strong opinion >> personally, assuming Phil knows better on that since he modified the memory >> loggings before. If all are happy with this, please proceed with either >> way. > > IIUC he was OK with adding memaccess as long as guest_errors will not change > so together with this proposal but he can clarify what he prefers. I just > want separate -d options for memaccess and the rest that's now in > guest_errors the way it was before mem access errors were added to this flag. This is less important now, I can carry this single patch a bit longer but if you can decide on the above I may be able to still change it and resubmit a new version. If you're too busy to think about it now we can come back to this in next devel cycle. Regards, BALATON Zoltan
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 }, };
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> --- include/qemu/log.h | 1 + softmmu/memory.c | 6 +++--- softmmu/physmem.c | 2 +- util/log.c | 2 ++ 4 files changed, 7 insertions(+), 4 deletions(-)