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

[Philippe Mathieu-Daudé]

On 13/2/23 15:58, Philippe Mathieu-Daudé wrote:
> On 13/2/23 15:47, 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.

- misusing hardware => LOG_GUEST_ERROR
- accessing missing features => LOG_UNIMP
- invalid memory areas => depending on the bus bridge, can be:

  #define MEMTX_OK              0
  #define MEMTX_ERROR          (1U << 0) /* device returned an error */
  #define MEMTX_DECODE_ERROR   (1U << 1) /* nothing at that address */
  #define MEMTX_ACCESS_ERROR   (1U << 2) /* access denied */

> > > > > > > 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

It is hard to justify we need a new mask and scripts have to adapt to
your new format. I assume you can not use trace-event because you want
this logging available in a RELEASE build, right?

> > but now that two of you did not get it 
> > maybe that message needs to be clarified instead?
>
> Is your use case memaccess enabled and guest-errors disabled?