bhyve: address-of-packed-member warnings

[tsoome]

Summary

/usr/src/usr.sbin/bhyve/pci_nvme.c:892:23: error: taking address of packed member 'status' of class or structure 'nvme_completion' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
                pci_nvme_status_tc(&compl->status, NVME_SCT_COMMAND_SPECIFIC,
                                    ^~~~~~~~~~~~~
/usr/src/usr.sbin/bhyve/pci_nvme.c:899:24: error: taking address of packed member 'status' of class or structure 'nvme_completion' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
        pci_nvme_status_genc(&compl->status, NVME_SC_SUCCESS);
                              ^~~~~~~~~~~~~
/usr/src/usr.sbin/bhyve/pci_nvme.c:915:24: error: taking address of packed member 'status' of class or structure 'nvme_completion' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
                        pci_nvme_status_tc(&compl->status,
                                            ^~~~~~~~~~~~~
/usr/src/usr.sbin/bhyve/pci_nvme.c:930:24: error: taking address of packed member 'status' of class or structure 'nvme_completion' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
                        pci_nvme_status_tc(&compl->status,
                                            ^~~~~~~~~~~~~
/usr/src/usr.sbin/bhyve/pci_nvme.c:939:24: error: taking address of packed member 'status' of class or structure 'nvme_completion' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
                        pci_nvme_status_tc(&compl->status,
                                            ^~~~~~~~~~~~~
/usr/src/usr.sbin/bhyve/pci_nvme.c:946:24: error: taking address of packed member 'status' of class or structure 'nvme_completion' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
                        pci_nvme_status_tc(&compl->status,
                                            ^~~~~~~~~~~~~
/usr/src/usr.sbin/bhyve/pci_nvme.c:960:25: error: taking address of packed member 'status' of class or structure 'nvme_completion' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
                pci_nvme_status_genc(&compl->status, NVME_SC_SUCCESS);
                                      ^~~~~~~~~~~~~
/usr/src/usr.sbin/bhyve/pci_nvme.c:972:25: error: taking address of packed member 'status' of class or structure 'nvme_completion' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
                pci_nvme_status_genc(&compl->status, NVME_SC_INVALID_FIELD);
                                      ^~~~~~~~~~~~~
/usr/src/usr.sbin/bhyve/pci_nvme.c:989:23: error: taking address of packed member 'status' of class or structure 'nvme_completion' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
                pci_nvme_status_tc(&compl->status, NVME_SCT_COMMAND_SPECIFIC,
                                    ^~~~~~~~~~~~~
/usr/src/usr.sbin/bhyve/pci_nvme.c:997:24: error: taking address of packed member 'status' of class or structure 'nvme_completion' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
                        pci_nvme_status_tc(&compl->status,
                                            ^~~~~~~~~~~~~
/usr/src/usr.sbin/bhyve/pci_nvme.c:1004:24: error: taking address of packed member 'status' of class or structure 'nvme_completion' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
        pci_nvme_status_genc(&compl->status, NVME_SC_SUCCESS);
                              ^~~~~~~~~~~~~
/usr/src/usr.sbin/bhyve/pci_nvme.c:1021:25: error: taking address of packed member 'status' of class or structure 'nvme_completion' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
                pci_nvme_status_genc(&compl->status, NVME_SC_INVALID_FIELD);
                                      ^~~~~~~~~~~~~
/usr/src/usr.sbin/bhyve/pci_nvme.c:1029:23: error: taking address of packed member 'status' of class or structure 'nvme_completion' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
                pci_nvme_status_tc(&compl->status,
                                    ^~~~~~~~~~~~~
/usr/src/usr.sbin/bhyve/pci_nvme.c:1039:23: error: taking address of packed member 'status' of class or structure 'nvme_completion' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
                pci_nvme_status_tc(&compl->status,
                                    ^~~~~~~~~~~~~
/usr/src/usr.sbin/bhyve/pci_nvme.c:1052:23: error: taking address of packed member 'status' of class or structure 'nvme_completion' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
                pci_nvme_status_tc(&compl->status,
                                    ^~~~~~~~~~~~~
/usr/src/usr.sbin/bhyve/pci_nvme.c:1062:24: error: taking address of packed member 'status' of class or structure 'nvme_completion' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
        pci_nvme_status_genc(&compl->status, NVME_SC_SUCCESS);
                              ^~~~~~~~~~~~~
/usr/src/usr.sbin/bhyve/pci_nvme.c:1077:24: error: taking address of packed member 'status' of class or structure 'nvme_completion' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
        pci_nvme_status_genc(&compl->status, NVME_SC_SUCCESS);
                              ^~~~~~~~~~~~~
/usr/src/usr.sbin/bhyve/pci_nvme.c:1120:23: error: taking address of packed member 'status' of class or structure 'nvme_completion' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
                pci_nvme_status_tc(&compl->status, NVME_SCT_COMMAND_SPECIFIC,
                                    ^~~~~~~~~~~~~
/usr/src/usr.sbin/bhyve/pci_nvme.c:1293:24: error: taking address of packed member 'status' of class or structure 'nvme_completion' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
        pci_nvme_status_genc(&compl->status, NVME_SC_INVALID_FIELD);
                              ^~~~~~~~~~~~~
fatal error: too many errors emitted, stopping now [-ferror-limit=]
20 errors generated.

pci_passthru.c:286:5: error: converting a packed 'struct msixcap' pointer (align
ment 1) to a 'uint32_t' {aka 'unsigned int'} pointer (alignment 4) may result in
 an unaligned pointer value [-Werror=address-of-packed-member]
  286 |     msixcap_ptr = (uint32_t*) &msixcap;
      |     ^~~~~~~~~~~
In file included from pci_passthru.c:65:
pci_emul.h:174:8: note: defined here
  174 | struct msixcap {
      |        ^~~~~~~
cc1: all warnings being treated as errors

pci_xhci.c: In function 'pci_xhci_portregs_read':
pci_xhci.c:2160:6: error: taking address of packed member of 'struct pci_xhci_po
rtregs' may result in an unaligned pointer value [-Werror=address-of-packed-memb
er]
 2160 |  p = &sc->portregs[port].portsc;
      |      ^~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

[grehan]

Yes for the bhyve parts.

[grehan]

Maybe ping ctuffli/imp for nvme.h, since that may involve removing other unecessary __packed's

[chuck]

sys/dev/nvme/nvme.h

635

Since struct nvme_completion is defined by the NVMe hardware specification, it needs the __packed attribute.

[chuck]

I'm happy to help out on the NVMe part but don't see this warning while building x86_64 on current. Can you point me in the direction of how to reproduce this?

[tsoome]

In D29126#652257, @chuck wrote:

I'm happy to help out on the NVMe part but don't see this warning while building x86_64 on current. Can you point me in the direction of how to reproduce this?

Should be simple - need to have -Waddress-of-packed-member effective:) The original findings were from building with gcc 10 (on illumos), but apparently even manual run with:

cc -O2 -pipe -fno-common -I/usr/src/usr.sbin/bhyve/../../contrib/lib9p -I/usr/src/sys -DINET -DINET6 -DNETGRAPH -I/usr/src/sys/dev/e1000 -I/usr/src/sys/dev/mii -I/usr/src/sys/dev/usb/controller -Waddress-of-packed-member -fPIE -g -MD -MF.depend.pci_xhci.o -MTpci_xhci.o -std=gnu99 -Wno-format-zero-length -fstack-protector-strong -Wsystem-headers -Werror -Wall -Wno-format-y2k -Wno-uninitialized -Wno-pointer-sign -Wno-empty-body -Wno-string-plus-int -Wno-unused-const-variable -Wno-tautological-compare -Wno-unused-value -Wno-parentheses-equality -Wno-unused-function -Wno-enum-conversion -Wno-unused-local-typedef -Wno-switch -Wno-switch-enum -Wno-knr-promoted-parameter -Qunused-arguments -c /usr/src/usr.sbin/bhyve/pci_nvme.c

should so just fine. Anyhow, if we must keep __packed, it means we need to make copy and write back or something -- we can not use take address like we do now.

[chuck]

I'm having trouble matching pci_nvme.c:903 to current, stable/12 or stable/13. Which branch is this? I'm curious to see why the compiler doesn't like this instance as (naively) everything should be nicely aligned.

[tsoome]

In D29126#652263, @chuck wrote:

I'm having trouble matching pci_nvme.c:903 to current, stable/12 or stable/13. Which branch is this? I'm curious to see why the compiler doesn't like this instance as (naively) everything should be nicely aligned.

Updated descriptions with errors from main branch.

[chuck]

Thanks for the update. If I'm understanding this compiler flag, it is not warning of an actual misalignment, more that this pattern may lead to unaligned accesses.

For the case of NVMe queues, the starting address for the queue will be aligned to the Memory Page Size value (i.e. 4K, 8K, 16K, ...). Since each Completion queue entry (e.g. struct nvme_completion) is 16 bytes, the status member will always be correctly (i.e. uint16_t) aligned.

Is there a way to signal to the compiler that a particular access is correct?

[jhb]

I would probably do the whitespace changes in a separate commit.

I think in FreeBSD we turn off this warning when using GCC because it has too many false positives. OTOH, packed isn't really what you want here Chuck, packed means "alignment of 1", not just "no padding". In practice though, there's no architecture supported by FreeBSD where leaving off packed would break. You are using fixed width types and the static assertion would catch any breakage here. I'm not sure if you could use 'packed aligned(4)'? Whatever you do, probably all of the structures in nvme.h need to be consistent, removing packed from just one will just make someone want to re-add it in the future.

[chuck]

In D29126#653594, @jhb wrote:

... OTOH, packed isn't really what you want here Chuck, packed means "alignment of 1", not just "no padding". In practice though, there's no architecture supported by FreeBSD where leaving off __packed would break.

OK, interesting.

You are using fixed width types and the static assertion would catch any breakage here. I'm not sure if you could use 'packed aligned(4)'?

I think this is probably closer to what I was expecting. Namely, the structure itself is nicely aligned but the elements inside have byte alignment.

Note that this header is for the NVMe kernel driver and (re)used by bhyve's NVMe emulation.