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 +
 system/memory.c    | 6 +++---
 system/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 e10e24cd4f..6122e34bdd 100644
--- a/include/qemu/log.h
+++ b/include/qemu/log.h
@@ -37,6 +37,7 @@ bool qemu_log_separate(void);
 #define LOG_PER_THREAD     (1 << 20)
 #define CPU_LOG_TB_VPU     (1 << 21)
 #define LOG_TB_OP_PLUGIN   (1 << 22)
+#define LOG_MEM_ACCESS     (1 << 23)
 
 /* Lock/unlock output. */
 
diff --git a/system/memory.c b/system/memory.c
index f6f6fee6d8..e35a473248 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1380,7 +1380,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));
@@ -1388,7 +1388,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));
@@ -1402,7 +1402,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/system/physmem.c b/system/physmem.c
index dc1db3a384..0f0e8f3bed 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -2745,7 +2745,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 6219819855..1aa7396277 100644
--- a/util/log.c
+++ b/util/log.c
@@ -503,6 +503,8 @@ const QEMULogItem qemu_log_items[] = {
       "open a separate log file per thread; filename must contain '%d'" },
     { CPU_LOG_TB_VPU, "vpu",
       "include VPU registers in the 'cpu' logging" },
+    { LOG_MEM_ACCESS, "memaccess",
+      "log invalid memory accesses" },
     { 0, NULL, NULL },
 };
 
-- 
2.30.9

On Sun, 6 Oct 2024 at 17:50, BALATON Zoltan <balaton@eik.bme.hu> 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 +
>  system/memory.c    | 6 +++---
>  system/physmem.c   | 2 +-
>  util/log.c         | 2 ++
>  4 files changed, 7 insertions(+), 4 deletions(-)

It seems reasonable to me to separate out "log invalid
memory accesses", yes.

> diff --git a/util/log.c b/util/log.c
> index 6219819855..1aa7396277 100644
> --- a/util/log.c
> +++ b/util/log.c
> @@ -503,6 +503,8 @@ const QEMULogItem qemu_log_items[] = {
>        "open a separate log file per thread; filename must contain '%d'" },
>      { CPU_LOG_TB_VPU, "vpu",
>        "include VPU registers in the 'cpu' logging" },
> +    { LOG_MEM_ACCESS, "memaccess",
> +      "log invalid memory accesses" },
>      { 0, NULL, NULL },

As a naming thing, these are logging specifically invalid
memory accesses, not all accesses, so we should have
both the user-facing name and the LOG_ constant be
named appropriately. Perhaps
  LOG_INV_MEM_ACCESS, "invalid_mem_accesses" ?

thanks
-- PMM

On Mon, 14 Oct 2024, Peter Maydell wrote:
> On Sun, 6 Oct 2024 at 17:50, BALATON Zoltan <balaton@eik.bme.hu> 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 +
>>  system/memory.c    | 6 +++---
>>  system/physmem.c   | 2 +-
>>  util/log.c         | 2 ++
>>  4 files changed, 7 insertions(+), 4 deletions(-)
>
> It seems reasonable to me to separate out "log invalid
> memory accesses", yes.
>
>> diff --git a/util/log.c b/util/log.c
>> index 6219819855..1aa7396277 100644
>> --- a/util/log.c
>> +++ b/util/log.c
>> @@ -503,6 +503,8 @@ const QEMULogItem qemu_log_items[] = {
>>        "open a separate log file per thread; filename must contain '%d'" },
>>      { CPU_LOG_TB_VPU, "vpu",
>>        "include VPU registers in the 'cpu' logging" },
>> +    { LOG_MEM_ACCESS, "memaccess",
>> +      "log invalid memory accesses" },
>>      { 0, NULL, NULL },
>
> As a naming thing, these are logging specifically invalid
> memory accesses, not all accesses, so we should have
> both the user-facing name and the LOG_ constant be
> named appropriately. Perhaps
>  LOG_INV_MEM_ACCESS, "invalid_mem_accesses" ?

That's way too long to type and remember. Maybe invalid_mem could do?

Regards,
BALATON Zoltan

> thanks
> -- PMM
>
>