Re: [PATCH v5 0/3] iommu/arm-smmu-v3: Parse out event records

[Pranjal Shrivastava <praan@google.com>]

On Wed, Nov 20, 2024 at 12:24:43PM +0530, Pranjal Shrivastava wrote:
> Hi Daniel,
> 
> On Wed, Nov 20, 2024 at 10:15 AM Daniel Mentz <danielmentz@google.com> wrote:
> >
> > On Tue, Nov 12, 2024 at 12:30 AM Pranjal Shrivastava <praan@google.com> wrote:
> > >
> > > Enhance the arm-smmu-v3 driver to parse out useful information from
> > > event records into a structure for better event handling & logging.
> >
> > Hi Pranjal,
> >
> > Thank you for putting this together.
> >
> > >
> > > Some sample events, powered by QEMU:
> >
> > > 4. Translation Fault:
> > >
> > > [    7.586428] arm-smmu-v3 arm-smmu-v3.0.auto: event 0x10 received:
> > > [    7.587012] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000800000010
> > > [    7.587504] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000020000000000
> > > [    7.587986] arm-smmu-v3 arm-smmu-v3.0.auto:  0x00000000fffff040
> > > [    7.588745] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000000000000
> > > [    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: Event 0x10 received: F_TRANSLATION
> > > [    7.589219]  client: 0000:00:01.0 sid: 0x8 ssid: 0x0
> > > [    7.589219]  iova 0xfffff040 ipa 0x0
> > > [    7.589219]  Unpriv | Data | Write | S1 | Input address caused fault
> > > [    7.589219]  STAG: 0x0
> >
> > I find that "Event 0x10" is redundant and recommend removing it. Maybe
> > print just "Event F_TRANSLATION"
>

I agree, however, in the past reviews [1] we've been asked to keep the
existing log "as is" unless there's a particularly compelling reason.

> 
> >
> > Not printing the prefix "arm-smmu-v3 arm-smmu-v3.0.auto:" on
> > subsequent lines appears to be inconsistent with how the raw event
> > bits are dumped. Also, I typically run a command like "grep
> > 'arm-smmu-v3 arm-smmu-v3.0.auto' uart.log" while I'm developing smmu
> > driver code, and with the proposed change, the grep command wouldn't
> > print most of the event details, because they are not prefixed by
> > "arm-smmu-v3 arm-smmu-v3.0.auto"
> 
> I see. The prefix isn't getting picked up because I'm trying to print
> all the lines in
> a single `dev_err` in order to avoid other logs inter-mingling with
> these ones. I can add
> that if we are okay with using multiple `dev_err` calls (i.e.
> potentially allowing other logs
> to mix-in).  Otherwise, let me see how we can manually add the dev_fmt
> string in the log.
> 
> >
> > The words "client", "sid" and "ssid" are followed by a colon (":") but
> > iova and ipa are not.
> >
> 
> Ack. Will fix that.
> 
> > Can you put everything up to and including iova on the first line and
> > the rest on the second line?

Finally my email client is fine. 
Did you mean something like the following:
[    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: Event 0x10 received: F_TRANSLATION
[    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: client: 0000:00:01.0 sid: 0x8 ssid: 0x0 iova: 0xfffff040 ipa: 0x0
[    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: Unpriv | Data | Write | S1 | Input address caused fault | STAG: 0x0

Thanks.
Praan
 
[1] https://lore.kernel.org/all/08498356-d892-4756-89e1-45b2654faa42@arm.com/#:~:text=safest%20to%20leave%20any%20existing%20%0Amessages%20exactly