[PATCH v2 0/2] Separate memory access logs from guest_errors

[PATCH v2 0/2] Separate memory access logs from guest_errors

[BALATON Zoltan]

Originally memory access logs were a debug define that then were
converted to log messages but were classified as guest_errors which
already logs misc errors. As invalid memory access logs can come from
accessing not emulated peripherals or memory areas, these often
generate a lot of messages that are better be controlled separately
from other errors to avoid obscuring those. As an example try
'qemu-system-ppc -d guest_errors' to see the problem. After this
series the actual guest error logs are easier to spot. I've tried to
submit this before but there were some people who liked the current
behaviour so now this series has another patch that preserves the old
option printing a warning to allow time to get used to the new
behaviour (which actually brings back the old behaviour when mem
access logs were a debug define). This second patch is optional if
changing the behaviour without notice is acceptable. As these are
debug switches no deprecation period is needed so the second patch
could be omitted. I leave that decision to the maintainers.

v2: Rename the option from memaccess to invalid_mem as suggested by
Peter Maydell

Regards,
BALATON Zoltan

BALATON Zoltan (2):
  log: Add separate debug option for logging invalid memory accesses
  log: Suggest using -d guest_error,invalid_mem instead of guest_errors

 docs/devel/secure-coding-practices.rst | 2 +-
 include/qemu/log.h                     | 1 +
 system/memory.c                        | 6 +++---
 system/physmem.c                       | 2 +-
 tests/avocado/smmu.py                  | 2 +-
 tests/qtest/pnv-host-i2c-test.c        | 2 +-
 util/log.c                             | 8 +++++++-
 7 files changed, 15 insertions(+), 8 deletions(-)

-- 
2.30.9

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

[BALATON Zoltan]

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 invalid_mem 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..60da703e67 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_INVALID_MEM    (1 << 23)
 
 /* Lock/unlock output. */
 
diff --git a/system/memory.c b/system/memory.c
index 85f6834cb3..a789064fbf 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1412,7 +1412,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_INVALID_MEM, "Invalid %s at addr 0x%" HWADDR_PRIX
                       ", size %u, region '%s', reason: rejected\n",
                       is_write ? "write" : "read",
                       addr, size, memory_region_name(mr));
@@ -1420,7 +1420,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_INVALID_MEM, "Invalid %s at addr 0x%" HWADDR_PRIX
                       ", size %u, region '%s', reason: unaligned\n",
                       is_write ? "write" : "read",
                       addr, size, memory_region_name(mr));
@@ -1434,7 +1434,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_INVALID_MEM, "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..4bc0228a50 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_INVALID_MEM,
                   "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..b87d399e4c 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_INVALID_MEM, "invalid_mem",
+      "log invalid memory accesses" },
     { 0, NULL, NULL },
 };
 
-- 
2.30.9

[PATCH v2 2/2] log: Suggest using -d guest_error, invalid_mem instead of guest_errors

[BALATON Zoltan]

Rename guest_errors to guest_error to match the log constant and print
a warning for -d guest_errors to remind using guest_error,invalid_mem
instead but preserve previous behaviour for convenience.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
This patch is optional, only to preserve current behaviour if that's
desired.

 docs/devel/secure-coding-practices.rst | 2 +-
 tests/avocado/smmu.py                  | 2 +-
 tests/qtest/pnv-host-i2c-test.c        | 2 +-
 util/log.c                             | 6 +++++-
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/docs/devel/secure-coding-practices.rst b/docs/devel/secure-coding-practices.rst
index 0454cc527e..b330b01956 100644
--- a/docs/devel/secure-coding-practices.rst
+++ b/docs/devel/secure-coding-practices.rst
@@ -85,7 +85,7 @@ request completes.  Unexpected accesses must not cause memory corruption or
 leaks in QEMU.
 
 Invalid device register accesses can be reported with
-``qemu_log_mask(LOG_GUEST_ERROR, ...)``.  The ``-d guest_errors`` command-line
+``qemu_log_mask(LOG_GUEST_ERROR, ...)``.  The ``-d guest_error`` command-line
 option enables these log messages.
 
 Live Migration
diff --git a/tests/avocado/smmu.py b/tests/avocado/smmu.py
index 83fd79e922..1632e8cfbc 100644
--- a/tests/avocado/smmu.py
+++ b/tests/avocado/smmu.py
@@ -46,7 +46,7 @@ def common_vm_setup(self, custom_kernel=False):
         self.vm.add_args("-accel", "kvm")
         self.vm.add_args("-cpu", "host")
         self.vm.add_args("-machine", "iommu=smmuv3")
-        self.vm.add_args("-d", "guest_errors")
+        self.vm.add_args("-d", "guest_error,invalid_mem")
         self.vm.add_args('-bios', os.path.join(BUILD_DIR, 'pc-bios',
                          'edk2-aarch64-code.fd'))
         self.vm.add_args('-device', 'virtio-rng-pci,rng=rng0')
diff --git a/tests/qtest/pnv-host-i2c-test.c b/tests/qtest/pnv-host-i2c-test.c
index 7f64d597ac..316933c873 100644
--- a/tests/qtest/pnv-host-i2c-test.c
+++ b/tests/qtest/pnv-host-i2c-test.c
@@ -418,7 +418,7 @@ static void test_host_i2c(const void *data)
 
     qts = qtest_initf("-M %s -smp %d,cores=1,threads=%d -nographic "
                       "-nodefaults -serial mon:stdio -S "
-                      "-d guest_errors",
+                      "-d guest_error,invalid_mem",
                       machine, SMT, SMT);
 
     /* Check the I2C master status registers after POR */
diff --git a/util/log.c b/util/log.c
index b87d399e4c..f05e36e7ec 100644
--- a/util/log.c
+++ b/util/log.c
@@ -486,7 +486,7 @@ const QEMULogItem qemu_log_items[] = {
       "show CPU state before CPU resets" },
     { LOG_UNIMP, "unimp",
       "log unimplemented functionality" },
-    { LOG_GUEST_ERROR, "guest_errors",
+    { LOG_GUEST_ERROR, "guest_error",
       "log when the guest OS does something invalid (eg accessing a\n"
       "non-existent register)" },
     { CPU_LOG_PAGE, "page",
@@ -521,6 +521,10 @@ int qemu_str_to_log_mask(const char *str)
             for (item = qemu_log_items; item->mask != 0; item++) {
                 mask |= item->mask;
             }
+        } else if (g_str_equal(*tmp, "guest_errors")) {
+            warn_report("Log option guest_errors is deprecated. "
+                        "Use guest_error,invalid_mem instead.");
+            mask |= LOG_GUEST_ERROR | LOG_INVALID_MEM;
 #ifdef CONFIG_TRACE_LOG
         } else if (g_str_has_prefix(*tmp, "trace:") && (*tmp)[6] != '\0') {
             trace_enable_events((*tmp) + 6);
-- 
2.30.9

Re: [PATCH v2 0/2] Separate memory access logs from guest_errors

[BALATON Zoltan]

On Sat, 2 Nov 2024, BALATON Zoltan wrote:
> Originally memory access logs were a debug define that then were
> converted to log messages but were classified as guest_errors which
> already logs misc errors. As invalid memory access logs can come from
> accessing not emulated peripherals or memory areas, these often
> generate a lot of messages that are better be controlled separately
> from other errors to avoid obscuring those. As an example try
> 'qemu-system-ppc -d guest_errors' to see the problem. After this
> series the actual guest error logs are easier to spot. I've tried to
> submit this before but there were some people who liked the current
> behaviour so now this series has another patch that preserves the old
> option printing a warning to allow time to get used to the new
> behaviour (which actually brings back the old behaviour when mem
> access logs were a debug define). This second patch is optional if
> changing the behaviour without notice is acceptable. As these are
> debug switches no deprecation period is needed so the second patch
> could be omitted. I leave that decision to the maintainers.
>
> v2: Rename the option from memaccess to invalid_mem as suggested by
> Peter Maydell

Ping? <https://patchew.org/QEMU/cover.1730549443.git.balaton@eik.bme.hu/>

Regards,
BALATON Zoltan

> BALATON Zoltan (2):
>  log: Add separate debug option for logging invalid memory accesses
>  log: Suggest using -d guest_error,invalid_mem instead of guest_errors
>
> docs/devel/secure-coding-practices.rst | 2 +-
> include/qemu/log.h                     | 1 +
> system/memory.c                        | 6 +++---
> system/physmem.c                       | 2 +-
> tests/avocado/smmu.py                  | 2 +-
> tests/qtest/pnv-host-i2c-test.c        | 2 +-
> util/log.c                             | 8 +++++++-
> 7 files changed, 15 insertions(+), 8 deletions(-)
>
>

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

[Peter Maydell]

On Sat, 2 Nov 2024 at 12:18, 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 invalid_mem option to make it possible
> to control it independently of other guest error logs.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v2 0/2] Separate memory access logs from guest_errors

[Peter Maydell]

On Fri, 13 Dec 2024 at 12:41, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> On Sat, 2 Nov 2024, BALATON Zoltan wrote:
> > Originally memory access logs were a debug define that then were
> > converted to log messages but were classified as guest_errors which
> > already logs misc errors. As invalid memory access logs can come from
> > accessing not emulated peripherals or memory areas, these often
> > generate a lot of messages that are better be controlled separately
> > from other errors to avoid obscuring those. As an example try
> > 'qemu-system-ppc -d guest_errors' to see the problem. After this
> > series the actual guest error logs are easier to spot. I've tried to
> > submit this before but there were some people who liked the current
> > behaviour so now this series has another patch that preserves the old
> > option printing a warning to allow time to get used to the new
> > behaviour (which actually brings back the old behaviour when mem
> > access logs were a debug define). This second patch is optional if
> > changing the behaviour without notice is acceptable. As these are
> > debug switches no deprecation period is needed so the second patch
> > could be omitted. I leave that decision to the maintainers.
> >
> > v2: Rename the option from memaccess to invalid_mem as suggested by
> > Peter Maydell
>
> Ping? <https://patchew.org/QEMU/cover.1730549443.git.balaton@eik.bme.hu/>

Personally I do not think we need the back-compat handling so
I would be happy with just patch 1.

(I think the way I envisaged this working was that if there
were cases where some missing device generates a ton of
errors that we would stick in a TYPE_UNIMPLEMENTED_DEVICE in
that region, so that the logging in the invalid access
paths in memory.c would not be hit except where the guest
was doing something wrong. But obviously we haven't actually
done that, and I don't think there's a strong reason not
to split out the invalid-access logging into its own
category.)

thanks
-- PMM