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

[Pranjal Shrivastava <praan@google.com>]

On Mon, Oct 28, 2024 at 09:23:53AM -0300, Jason Gunthorpe wrote:
> On Thu, Oct 24, 2024 at 05:37:11PM +0000, Pranjal Shrivastava wrote:
> > 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;
> 
> You can make this a u8

Ack, I'll update this with the v5 of this series.

> 
> > 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;
> 
> I wouldn't do this..

Ack.

> 
> Jason

Thanks,
Praan