Re: [PATCH v4 1/3] iommu/arm-smmu-v3: Introduce struct arm_smmu_event

[Pranjal Shrivastava <praan@google.com>]

On Mon, Nov 11, 2024 at 08:01:58PM -0800, Daniel Mentz wrote:
> On Mon, Nov 11, 2024 at 4:52 PM Pranjal Shrivastava <praan@google.com> wrote:
> >
> > On Mon, Nov 11, 2024 at 02:20:46PM -0800, Daniel Mentz wrote:
> > > On Thu, Nov 7, 2024 at 6:57 AM Pranjal Shrivastava <praan@google.com> wrote:
> > > >
> > > > On Wed, Nov 06, 2024 at 04:16:19PM -0800, Daniel Mentz wrote:
> > > > > On Fri, Oct 18, 2024 at 11:00 AM Pranjal Shrivastava <praan@google.com> wrote:
> > > > > >
> > > > > > Introduce `struct arm_smmu_event` to represent event records.
> > > > > > Parse out relevant fields from raw event records for ease and
> > > > > > use the new `struct arm_smmu_event` instead.
> > > > > >
> > > > > > Signed-off-by: Daniel Mentz <danielmentz@google.com>
> > > > > > Signed-off-by: Pranjal Shrivastava <praan@google.com>
> > > > > > ---
> > > > >
> > > > > > +struct arm_smmu_event {
> > > > > > +       struct arm_smmu_device          *smmu;
> > > > > > +       u8                              id;
> > > > > > +       u8                              class;
> > > > > > +       u16                             stag;
> > > > > > +       u32                             sid;
> > > > > > +       u32                             ssid;
> > > > > > +       u64                             iova;
> > > > > > +       u64                             ipa;
> > > > > > +       u64                             raw[EVTQ_ENT_DWORDS];
> > > > >
> > > > > Consider removing the member named raw from struct arm_smmu_event.
> > > > > Compare this with struct arm_smmu_cmdq_ent and
> > > > > arm_smmu_cmdq_build_cmd() which keep the encoded and decoded versions
> > > > > separate.
> > > >
> > > > I had a similar implemntation in v3 [1] but it was decided [2]
> > > > to keep the "raw" event array within arm_smmu_event itself. Since
> > > > otherwise we'd have two variables, one pointing to the other when they
> > > > have the exact same scope and lifetime anyway.
> > >
> > > I understand that the concern in [2] was that "one [is] pointing to
> > > the other". At the time, I think you had a pointer in struct
> > > arm_smmu_event named raw, and the feedback was to embed the raw event
> > > data in the structure instead of having a pointer. What I'm proposing
> > > is to neither have a pointer nor embed it in the struct.
> > >
> > > >
> > > > Do we have a strong preference here?
> > >
> > > Not a strong preference, but I'd prefer to have the raw event and the
> > > decoded event separate.
> > >
> >
> > I see. So, do you suggest that we should have the raw print as it is and
> > then the pretty print separately like the following:
> > ---------------------------------------->8------------------------------------
> >
> >         while (!queue_remove_raw(q, raw_evt)) {
> >
> >                 arm_smmu_decode_event(raw_evt, &evt);
> >
> >                 if (arm_smmu_handle_evt(smmu, &evt)) {
> >                         dev_err(smmu->dev, "event 0x%02x received:\n", event->id);
> >                         for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
> >                                 dev_err(smmu->dev, "\t0x%016llx\n", raw_evt[i]);
> >                         arm_smmu_dump_event(smmu, &evt, &rs);
> >                 }
> >
> >                 put_device(evt.dev);
> >                 cond_resched();
> >         }
> >
> > ---------------------------------------->8------------------------------------
> >
> > OR should we pass the raw event to the dump_event function which, I
> > believe, is a little duplicative:
> >
> > ---------------------------------->8-----------------------------
> >
> >         while (!queue_remove_raw(q, raw_evt)) {
> >                 arm_smmu_decode_event(raw_evt, &evt);
> >                 if (arm_smmu_handle_evt(smmu, &evt))
> >                         arm_smmu_dump_event(smmu, &evt, raw_evt, &rs);
> >                 put_device(evt.dev);
> >                 cond_resched();
> >         }
> >
> > ---------------------------------->8-----------------------------
> 
> Your second option would be my preference despite the fact that you
> have to pass in an extra parameter. It makes it clear that
> arm_smmu_dump_event prints both. I would make sure that the order of
> &evt and raw_evt in the argument list matches the order in which
> arm_smmu_dump_event is printing them.
> 

Ack. I'll post a v5 soon and accommodate this with it! Thanks!

> >
> > > >
> > > > >
> > > > > > +       bool                            stall;
> > > > > > +       bool                            ssid_valid;
> > > > > > +       bool                            privileged;
> > > > > > +       bool                            instruction;
> > > > > > +       bool                            s2;
> > > > > > +       bool                            read;
> > > > > > +};
> > > >
> > > > Thanks,
> > > > Praan
> > > >
> > > > [1] https://lore.kernel.org/all/20240928005143.2378938-2-praan@google.com/
> > > > [2] https://lore.kernel.org/all/5eda3ba6-c35a-432b-be87-48bd8a0a3bf1@arm.com/