Bug 237940 - struct nvme_health_information_page{} @ /usr/include/dev/nvme/nvme.h has alignment issues.
Summary: struct nvme_health_information_page{} @ /usr/include/dev/nvme/nvme.h has alig...
Status: New
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 12.0-RELEASE
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-scsi mailing list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-05-17 08:19 UTC by Pawel Kraszewski
Modified: 2019-05-17 18:21 UTC (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Pawel Kraszewski 2019-05-17 08:19:55 UTC
struct nvme_health_information_page {
 
        uint8_t                 critical_warning;
        uint16_t                temperature;
        uint8_t                 available_spare;
/* ... */
} __packed __aligned(4);

has issues with alignment: `temperature` field can't be properly aligned on any current architecture which seems to defeat the `__aligned(4)` hint.

What has bitten me today is that it confuses some code-generating tools, like Rust's bindgen. I already filed a bug to them, as they treat __packed __aligned(4) as __packed(4) - which has a *totally* different meaning -  but it would be nice to have some BSD compiler guru took a glimpse at this particular structure and shed some light on whether __aligned(4) is OK here. 

And yes, I know this structure layout is not BSD's idea ¯\_(ツ)_/¯
Comment 1 Conrad Meyer freebsd_committer 2019-05-17 18:21:00 UTC
Yes, the temperature field is inherently misaligned.

The __aligned(4) was added in r240671 as a hack for printing out the nvme_controller_data and nvme_namespace_data pages as 32-bit hex values with a naive cast -- avoiding a -Wcast-align warning.  At the time, nvme_health_information_page wasn't used by nvmecontrol -- it was probably just added at the same time because it was also __packed.

Today we mostly print the page using a specialized printer, print_log_health, rather than any uint32 cast hex hack.  print_hex -> print_dwords still uses this hack for the log page data if -x flag is specified, but print functions take the raw void* buffer pointer rather than a structure pointer, so we can probably drop the __aligned attribute.

Additionally, get_log_buffer() allocates the buffer using plain malloc() with the log page size.  This implicitly provides 4-byte alignment because FreeBSD only supports 32-bit and 64-bit architectures, and the log page is >4 bytes in size, so any standard C compliant malloc must provide 4-byte alignment on any platform FreeBSD runs on.  It might be more clear to use aligned_alloc() to provide that guarantee explicitly instead, but I don't feel strongly about it.

Tl;dr: I think we can drop the __aligned(4) attribute now, and probably on the other structures as well.