* [PATCH v2 0/2] Separate memory access logs from guest_errors
@ 2024-11-02 12:17 BALATON Zoltan
2024-11-02 12:17 ` [PATCH v2 1/2] log: Add separate debug option for logging invalid memory accesses BALATON Zoltan
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: BALATON Zoltan @ 2024-11-02 12:17 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, philmd
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
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v2 1/2] log: Add separate debug option for logging invalid memory accesses 2024-11-02 12:17 [PATCH v2 0/2] Separate memory access logs from guest_errors BALATON Zoltan @ 2024-11-02 12:17 ` BALATON Zoltan 2024-12-17 16:01 ` Peter Maydell 2024-11-02 12:17 ` [PATCH v2 2/2] log: Suggest using -d guest_error, invalid_mem instead of guest_errors BALATON Zoltan 2024-12-13 12:41 ` [PATCH v2 0/2] Separate memory access logs from guest_errors BALATON Zoltan 2 siblings, 1 reply; 6+ messages in thread From: BALATON Zoltan @ 2024-11-02 12:17 UTC (permalink / raw) To: qemu-devel; +Cc: Peter Maydell, philmd 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] log: Add separate debug option for logging invalid memory accesses 2024-11-02 12:17 ` [PATCH v2 1/2] log: Add separate debug option for logging invalid memory accesses BALATON Zoltan @ 2024-12-17 16:01 ` Peter Maydell 0 siblings, 0 replies; 6+ messages in thread From: Peter Maydell @ 2024-12-17 16:01 UTC (permalink / raw) To: BALATON Zoltan; +Cc: qemu-devel, philmd 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] log: Suggest using -d guest_error, invalid_mem instead of guest_errors 2024-11-02 12:17 [PATCH v2 0/2] Separate memory access logs from guest_errors BALATON Zoltan 2024-11-02 12:17 ` [PATCH v2 1/2] log: Add separate debug option for logging invalid memory accesses BALATON Zoltan @ 2024-11-02 12:17 ` BALATON Zoltan 2024-12-13 12:41 ` [PATCH v2 0/2] Separate memory access logs from guest_errors BALATON Zoltan 2 siblings, 0 replies; 6+ messages in thread From: BALATON Zoltan @ 2024-11-02 12:17 UTC (permalink / raw) To: qemu-devel; +Cc: Peter Maydell, philmd 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 0/2] Separate memory access logs from guest_errors 2024-11-02 12:17 [PATCH v2 0/2] Separate memory access logs from guest_errors BALATON Zoltan 2024-11-02 12:17 ` [PATCH v2 1/2] log: Add separate debug option for logging invalid memory accesses BALATON Zoltan 2024-11-02 12:17 ` [PATCH v2 2/2] log: Suggest using -d guest_error, invalid_mem instead of guest_errors BALATON Zoltan @ 2024-12-13 12:41 ` BALATON Zoltan 2024-12-17 16:04 ` Peter Maydell 2 siblings, 1 reply; 6+ messages in thread From: BALATON Zoltan @ 2024-12-13 12:41 UTC (permalink / raw) To: qemu-devel; +Cc: Peter Maydell, philmd 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(-) > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 0/2] Separate memory access logs from guest_errors 2024-12-13 12:41 ` [PATCH v2 0/2] Separate memory access logs from guest_errors BALATON Zoltan @ 2024-12-17 16:04 ` Peter Maydell 0 siblings, 0 replies; 6+ messages in thread From: Peter Maydell @ 2024-12-17 16:04 UTC (permalink / raw) To: BALATON Zoltan; +Cc: qemu-devel, philmd 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-12-17 16:11 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-11-02 12:17 [PATCH v2 0/2] Separate memory access logs from guest_errors BALATON Zoltan 2024-11-02 12:17 ` [PATCH v2 1/2] log: Add separate debug option for logging invalid memory accesses BALATON Zoltan 2024-12-17 16:01 ` Peter Maydell 2024-11-02 12:17 ` [PATCH v2 2/2] log: Suggest using -d guest_error, invalid_mem instead of guest_errors BALATON Zoltan 2024-12-13 12:41 ` [PATCH v2 0/2] Separate memory access logs from guest_errors BALATON Zoltan 2024-12-17 16:04 ` Peter Maydell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).