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

From: Vladimir Oltean
Date: Thu Apr 10 2025 - 16:55:14 EST

On Thu, Apr 10, 2025 at 05:37:59AM -0700, Breno Leitao wrote:
> 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

When it's part of struct qm_mcc_initfq or struct qm_mcr_queryfq, it's
actually aligned properly, because the outer structures (QMan management
commands) offer that alignment. It's ok to not add padding, you still
want to talk to the QMan properly...

The problem might be its standalone use, qpool_cleanup() and cgr_cleanup(),
where we give a pointer to an on-stack "fqd" variable to qman_query_fq(),
and we expect it to do the following copy:
*fqd = mcr->queryfq.fqd;
with "*fqd" being unaligned and mcr->queryfq.fqd being aligned.

At least on arm64, I see the code decompiles to the following.

*fqd = mcr->queryfq.fqd;
1d84: f841a109 ldur x9, [x8, #0x1a]
1d88: f841210a ldur x10, [x8, #0x12]
1d8c: f840a108 ldur x8, [x8, #0xa]
1d90: a900a66a stp x10, x9, [x19, #0x8]
1d94: f9000268 str x8, [x19]

Three 64-bit "load unscaled register" instructions to read
mcr->queryfq.fqd, and two instructions (store pair, store register) to
write to *fqd. So it's not doing the transfer field by field, and not
dereferencing the unaligned fqd.context_a in that process.

In fact, nowhere does the code seem to be dereferencing the 64-bit context_a.opaque,
so that can in principle be removed if that is a problem.

The code paths that dereference context_a through other mechanisms are:

qm_fqd_stashing_set64() - goes through the 16-bit context_hi and 32-bit context_lo
- used by qman_init_fq()

qm_fqd_context_a_set64() - goes through the 32-bit hi and 32-bit lo, used by:
- dpaa_fq_init()
- create_caam_req_fq()

qm_fqd_set_stashing() - goes through the struct qm_fqd_stashing that is
16 bits in size, used by:
- dpaa_fq_init()
- init_handler() in drivers/soc/fsl/qbman/qman_test_stash.c
- alloc_rsp_fq_cpu() in drivers/crypto/caam/qi.c

All of the above functions operate on the fqd structure embedded within
struct qm_mcc_initfq initfq, therefore prepend 10 more bytes and see how
that makes things properly aligned.

I'm not seeing alignment issues in actual use, and I still don't
understand exactly what the compiler is saying.