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

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