Re: include/soc/fsl/qman.h:383:4: warning: field context_a within 'struct qm_fqd' is less aligned than 'union (unnamed union at include/soc/fsl/qman.h:365:2)' and is usually due to 'struct qm_fqd' being packed, which can lead to unaligned accesses

[Breno Leitao <leitao@debian.org>]

On Wed, Apr 09, 2025 at 03:01:38PM +0300, Vladimir Oltean wrote:
> Hi Breno,
> 
> On Wed, Apr 09, 2025 at 04:18:04AM -0700, Breno Leitao wrote:
> > On Wed, Apr 09, 2025 at 04:55:16PM +0800, kernel test robot wrote:
> > > >> include/soc/fsl/qman.h:383:4: warning: field context_a within 'struct qm_fqd' is less aligned than 'union (unnamed union at include/soc/fsl/qman.h:365:2)' and is usually due to 'struct qm_fqd' being packed, which can lead to unaligned accesses [-Wunaligned-access]
> > >            } context_a;
> > 
> > <snip>
> > 
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  346  
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  347  struct qm_fqd {
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  348  	/* _res[6-7], orprws[3-5], oa[2], olws[0-1] */
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  349  	u8 orpc;
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  350  	u8 cgid;
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  351  	__be16 fq_ctrl;	/* See QM_FQCTRL_<...> */
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  352  	__be16 dest_wq;	/* channel[3-15], wq[0-2] */
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  353  	__be16 ics_cred; /* 15-bit */
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  354  	/*
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  355  	 * For "Initialize Frame Queue" commands, the write-enable mask
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  356  	 * determines whether 'td' or 'oac_init' is observed. For query
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  357  	 * commands, this field is always 'td', and 'oac_query' (below) reflects
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  358  	 * the Overhead ACcounting values.
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  359  	 */
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  360  	union {
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  361  		__be16 td; /* "Taildrop": _res[13-15], mant[5-12], exp[0-4] */
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  362  		struct qm_fqd_oac oac_init;
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  363  	};
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  364  	__be32 context_b;
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  365  	union {
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  366  		/* Treat it as 64-bit opaque */
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  367  		__be64 opaque;
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  368  		struct {
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  369  			__be32 hi;
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  370  			__be32 lo;
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  371  		};
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  372  		/* Treat it as s/w portal stashing config */
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  373  		/* see "FQD Context_A field used for [...]" */
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  374  		struct {
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  375  			struct qm_fqd_stashing stashing;
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  376  			/*
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  377  			 * 48-bit address of FQ context to
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  378  			 * stash, must be cacheline-aligned
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  379  			 */
> > 
> > Looking at pahole, I am not sure this is caacheline-aligned, as
> > suggested above.
> > 
> > 	# pahole -C qm_fqd
> > 	struct qm_fqd {
> > 		u8                         orpc;                 /*     0     1 */
> > 		u8                         cgid;                 /*     1     1 */
> > 		__be16                     fq_ctrl;              /*     2     2 */
> > 		__be16                     dest_wq;              /*     4     2 */
> > 		__be16                     ics_cred;             /*     6     2 */
> > 		union {
> > 			__be16             td;                   /*     8     2 */
> > 			struct qm_fqd_oac  oac_init;             /*     8     2 */
> > 		};                                               /*     8     2 */
> > 		__be32                     context_b;            /*    10     4 */
> > 		union {
> > 			__be64             opaque;               /*    14     8 */
> > 			struct {
> > 				__be32     hi;                   /*    14     4 */
> > 				__be32     lo;                   /*    18     4 */
> > 			};                                       /*    14     8 */
> > 			struct {
> > 				struct qm_fqd_stashing stashing; /*    14     2 */
> > 				__be16     context_hi;           /*    16     2 */
> > 				__be32     context_lo;           /*    18     4 */
> > 			};                                       /*    14     8 */
> > 		} context_a;                                     /*    14     8 */
> > 		struct qm_fqd_oac          oac_query;            /*    22     2 */
> > 
> > 		/* size: 24, cachelines: 1, members: 9 */
> > 		/* last cacheline: 24 bytes */
> > 	} __attribute__((__packed__));
> > 
> > 
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  380  			__be16 context_hi;
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  381  			__be32 context_lo;
> > > c535e923bb97a4 Claudiu Manoil 2016-09-22  382  		} __packed;
> > 
> > I am wondering if we should use __attribute__((aligned(8))) instead of
> > __packed, according to the document above.
> 
> Since you've replied, could you please help me also understand the issue?
> It says "field context_a within 'struct qm_fqd' is less aligned than
> 'union (unnamed union at include/soc/fsl/qman.h:365:2)'", but at line 365,
> the unnamed union _is_ field context_a, no? What is it saying?
> 
> I _suspect_, but I'm not sure, that it's complaining about context_a.opaque
> being a 64-bit field which is not aligned to a 64-bit boundary. Though
> I'm not getting that from reading the description.

In fact, the whole union is not aligned, and because "struct qm_fqd" is
also packed, it cannot add a padding before the union (which would be
the compiler decision,  since the union contains a packed field).

> But struct qm_fqd is embedded in other structures, and the alignment
> of the fields changes in that case. I think that context_a.opaque is
> 64-bit aligned in all cases.

I don't think it is ever aligned. Check the pahole above, and the union
starts at byte 14th, which is not aligned if my math is correct.

--breno