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.