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

[Peter Xu]

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.

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.

Thanks,

-- 
Peter Xu