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

[Nicolin Chen <nicolinc@nvidia.com>]

On Fri, Sep 06, 2024 at 05:39:03PM +0100, Robin Murphy wrote:
> On 06/09/2024 1:55 pm, Will Deacon wrote:
> > > > > > +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.
> > 
> > My only real concern is the fragility of using bitfields. I don't _think_
> > the compiler is obliged to lay them out in the obvious way and I can't
> > think of anything worse than being given a bad pretty-print when debugging
> > a real driver issue!

Well, if compiler is going to be an issue, I can't disagree with
your point.

Any reference to some existing issue with compiler failing to lay
out properly? I wonder how other headers could define their unions
using bitfields and stay safe..

> Shall I mention big-endian? :D

Ah, right. queue_read() converts le64 to native CPU format..

> FWIW I'm not a fan of the bitfield trickery either. However, unions

I am trying to learn here: apart from what Will mentioned above,
is there any other reason to stay away from bitfield?

Thanks!
Nicolin

> aside, I do still think it would be a good idea for the raw event data
> storage to be in struct arm_smmu_event itself - having one local
> variable carry a pointer to another local variable in the same scope
> seems a bit silly.
> 
> Thanks,
> Robin.