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

[Thomas Huth <thuth@redhat.com>]

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