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

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(a)eik.bme.hu&gt; >>>>>>> >>>>>>> Ping? Could somebody review and pick it up please? >>>>>> >>>>>> Ping? >>>>> >>>>> Patch makes sense to me and looks fine, so: >>>>> >>>>> Reviewed-by: Thomas Huth <thuth(a)redhat.com&gt; >>>>> >>>>> ... 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(a)linaro.org&gt; >> 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.