Re: [PATCH v2 1/2] iommu/arm-smmu-v3: Print better events records

[Nicolin Chen <nicolinc@nvidia.com>]

On Thu, Sep 05, 2024 at 04:06:24PM +0000, Pranjal Shrivastava wrote:

> > > > > Also, the printing would become
> > > > > more complicated as we'd have to log different fields for different
> > > > > events. Additionally, I don't see that many unions being defined
> > > > > elsewhere in the kernel.
> > > >
> > > > OK. That's a fair point. I think we could have just one common
> > > > union for the "good stuff" fields. Then, if something isn't in
> > > > the common union, do a FIELD_GET(raw)?
> > > >
> > >
> > > I'm not sure if I get this right, but are you suggesting something like:
> > >
> > > +struct arm_smmu_event {
> > > +       union {
> > > +       u64 raw_evt[4];
> > > +       struct {
> > > +               /* "Good stuff" fields */
> > > +       };
> > > +};
> > >
> > > and then based on the fault type we can use the "good stuff" or raw_evt?
> > > So, basically just add a union between raw_evt and the other fields to
> > > improve struct arm_smmu_event present in v2?
> >
> > Yea, I attached a test code at the EOM for your reference. Please
> > feel free to drop fields if they aren't common enough, and confirm
> > those bits are correctly written too.
> >
> 
> Hmm, so you're suggesting to make the event a union instead of a struct
> thus, making reading event fields easy.
> 
> The only thing I'm slightly worried about is if the bitfields don't
> remain constant if more events are added in the future.
> 
> For example, the "IPA" field is in dword[3] for now, but if it changes
> in the future, it may get difficult to scale the union for future events
> If we can guarantee that it doesn't happen then I think union is better.

I believe HW has its own reason to put those fields constantly
following the same pattern, and must have its tendency to keep
in that way.

The "union { u64 raw; struct {} }" way isn't that uncommon in
the kernel, here are a few existing examples:
	drivers/crypto/marvell/octeontx2/otx2_cpt_hw_types.h
	drivers/crypto/cavium/nitrox/nitrox_req.h
	include/asm-generic/hyperv-tlfs.h

Taking a step back, if one field in a new event in the future
happens to be odd, we could still fetch field in its own way:
event->evt[4] is still available to extract via FIELD_GET; or
just define another union exclusively for that event, neither
of which sounds like the end of world.

Having said that, I'd defer to Will. So, here is just my two
cents.

> > > I'm just wondering if that'd be too much to print the "master_name", I
> > > mean what if we simply save the master_name in `struct arm_smmu_event`?
> > > That way, we can keep the locking as is in `arm_smmu_handle_evt` and
> > > simply print the stored "master_name".
> > >
> > > Note: I'm suggesting to store the entire string and not just the ptr
> > > returned by dev_name(master->dev)), something like:
> > >
> > > `strcpy(event->master_name, dev_name(master->dev))`
> >
> > I'd probably move the dump() call inside arm_smmu_handle_evt(), and
> > within the lock to avoid strcpy. And eventually it would be located
> > at "else { /* Unhandled events should be pinned */ ret = -EFAULT; }:
> > https://lore.kernel.org/linux-iommu/8b93be1d913f9e227748de2d07e8540ddc2372ab.1724777091.git.nicolinc@nvidia.com/
> >
> 
> I think we'll still need to dump the event for other cases, such as the
> case where the master wasn't found, we return `-EINVAL` and the case
> where EVTQ_1_S2 is set. I guess, we should have a case under the label
> `out_unlock` we can call the dump function based on the ret value. E.g.:
> 
> out_unlock:
> +       if (ret)
> +               arm_smmu_dump_event(smmu, evt);
>         mutex_unlock(&smmu->streams_mutex);
>         return ret;

Ah, that's thoughtful. I missed it. I agree.

Thanks
Nicolin