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

[Pranjal Shrivastava <praan@google.com>]

On Mon, Sep 09, 2024 at 03:45:06PM +0100, Will Deacon wrote:
> On Fri, Sep 06, 2024 at 11:42:31AM -0700, Nicolin Chen wrote:
> > 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..
> 
> It's not so much about buggy compilers, but more that I don't think the
> C standard defines the order and so relying on it can be fragile. If you
> fancy going to the effort of ensuring that LLVM and GCC will agree on
> the "obvious" layout forever more, then don't let me stop you ;)
> 

Hmmm.. +1
I had some time, so.. I dived into the C specification[1] (free draft)
to look for bitfield layouts, under section 6.7.2.1, point 11 
(page 101 of the pdf), and I see the following paragraph:

"An implementation may allocate any addressable storage unit large
enough to hold a bit-field. If enough space remains, a bit-field that
immediately follows another bit-field in a structure shall be packed
into adjacent bits of the same unit. If insufficient space remains,
whether a bit-field that does not fit is put into the next unit or
overlaps adjacent units is implementation-defined. The order of 
allocation of bit-fields within a unit (high-order to low-order or 
low-order to high-order) is implementation-defined. The alignment
of the addressable storage unit is unspecified."

However, I'm not sure how these "implementations" comprehend the spec.

> Will

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

One more thing I can think of is Alignment faults on certain archs.
Accessing individual bitfields within the struct could lead to unaligned
memory accesses. I have run into alignment faults on $ARCH=arm64 couple
of times while using packed structs in the kernel code.

[1]
 https://web.archive.org/web/20181230041359if_/http://www.open-std.org/jtc1/sc22/wg14/www/abq/c17_updated_proposed_fdis.pdf

Thanks,
Pranjal