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

[Pranjal Shrivastava <praan@google.com>]

On Thu, Oct 24, 2024 at 02:03:29PM -0300, Jason Gunthorpe wrote:
> On Thu, Oct 24, 2024 at 05:02:08PM +0000, Pranjal Shrivastava wrote:
> > 	/* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
> > 	bool                       stall;                /*    88     1 */
> > 	bool                       ssid_valid;           /*    89     1 */
> > 	bool                       privileged;           /*    90     1 */
> > 	bool                       instruction;          /*    91     1 */
> > 	bool                       s2;                   /*    92     1 */
> > 	bool                       read;                 /*    93     1 */
> > 	bool                       ttrnw;                /*    94     1 */
> > 	bool                       ttrnw_valid;          /*    95     1 */
> 
> Linus has had negative things to say about lists of bools in
> structs. Use a bitfield?

Hmm, I agree, each bool is a waste of 7-bits. I tried the following:

struct arm_smmu_event {
	u8				id;
	u8				class;
	u16				stag;

	/* Group the boolean values together */
	unsigned int stall:1;
	unsigned int ssid_valid:1;
	unsigned int privileged:1;
	unsigned int instruction:1;
	unsigned int s2:1;
	unsigned int read:1;
	unsigned int ttrnw:1;
	unsigned int ttrnw_valid:1;

	u32				sid;
	u32				ssid;
	u64				iova;
	u64				ipa;
	u64				raw[EVTQ_ENT_DWORDS];
	const char			*master_name;
	struct arm_smmu_device		*smmu;
	struct device			*dev;
};

This reduces the hole to a 3-byte (24-bit hole).
Also, since we have exactly 8 bits, we can pack them in a union too:

struct arm_smmu_event {
	u8				id;
	u8				class;
	u16				stag;
	
	/* Group the boolean values together */
	union {
		struct {
			unsigned int stall:1;
			unsigned int ssid_valid:1;
			unsigned int privileged:1;
			unsigned int instruction:1;
			unsigned int s2:1;
			unsigned int read:1;
			unsigned int ttrnw:1;
			unsigned int ttrnw_valid:1;
		};
		u8 flags;
	};

	u32				sid;
	u32				ssid;
	u64				iova;
	u64				ipa;
	u64				raw[EVTQ_ENT_DWORDS];
	const char			*master_name;
	struct arm_smmu_device		*smmu;
	struct device			*dev;
};

I like the former better since we won't have to change the union if
future revisions of the hw add new flags. Any preferences?

> 
> Jason

Thanks,
Praan