From 68ff34bc669f3e889dc0ca98c27bdcf5a74f737d Mon Sep 17 00:00:00 2001 From: Nick Connolly Date: Tue, 9 Feb 2021 13:25:03 +0000 Subject: [PATCH] include/nvme_spec.h: improve portability Aspects of bit fields are 'implementation defined'. On some platforms alignment will occur if two adjacent fields are of different types. This occurs in spdk_nvme_feat_async_event_configutation after the crit_warn member which is effectively an int8_t, followed by an int16_t. There isn't a generic way of changing the compiler's behaviour, so the best options are: - Change crit_warn to a uint32_t bit field and copy the value to/from a spdk_nvme_critical_warning_state variable to use it. This requires changes to code using the field. - Adjust the structure definition to use smaller types to avoid the problem. This preserves existing semantics, but the field order will need to be reviewed if big-endian support is ever added (other places in nvme_spec.h will need similar attention). A second reserved field is required. Use smaller types which seems the most straightforward option. Adjust the use of the spdk_nvme_feat_async_event_configuration reserved fields in lib/nvmf/ctrlr.c. The new structure is binary compatible and the fields behave in the same way, with the exception of an additional reserved field, so updating CHANGELOG.md probably isn't necessary. Signed-off-by: Nick Connolly Change-Id: I7d8163c84b4f410fc95a5b7064506ad7b4b62c6c Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/6340 Community-CI: Broadcom CI Tested-by: SPDK CI Jenkins Reviewed-by: Jim Harris Reviewed-by: Aleksey Marchuk --- include/spdk/nvme_spec.h | 13 +++++++------ lib/nvmf/ctrlr.c | 3 ++- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/include/spdk/nvme_spec.h b/include/spdk/nvme_spec.h index ca91c8bc1..87c327903 100644 --- a/include/spdk/nvme_spec.h +++ b/include/spdk/nvme_spec.h @@ -712,13 +712,14 @@ union spdk_nvme_feat_async_event_configuration { uint32_t raw; struct { union spdk_nvme_critical_warning_state crit_warn; - uint32_t ns_attr_notice : 1; - uint32_t fw_activation_notice : 1; - uint32_t telemetry_log_notice : 1; - uint32_t ana_change_notice : 1; - uint32_t reserved : 19; + uint8_t ns_attr_notice : 1; + uint8_t fw_activation_notice : 1; + uint8_t telemetry_log_notice : 1; + uint8_t ana_change_notice : 1; + uint8_t reserved1 : 4; + uint16_t reserved2 : 15; /** Discovery log change (refer to the NVMe over Fabrics specification) */ - uint32_t discovery_log_change_notice : 1; + uint16_t discovery_log_change_notice : 1; } bits; }; SPDK_STATIC_ASSERT(sizeof(union spdk_nvme_feat_async_event_configuration) == 4, "Incorrect size"); diff --git a/lib/nvmf/ctrlr.c b/lib/nvmf/ctrlr.c index 29fa29e3c..252881baa 100644 --- a/lib/nvmf/ctrlr.c +++ b/lib/nvmf/ctrlr.c @@ -1655,7 +1655,8 @@ nvmf_ctrlr_set_features_async_event_configuration(struct spdk_nvmf_request *req) SPDK_DEBUGLOG(nvmf, "Set Features - Async Event Configuration, cdw11 0x%08x\n", cmd->cdw11); ctrlr->feat.async_event_configuration.raw = cmd->cdw11; - ctrlr->feat.async_event_configuration.bits.reserved = 0; + ctrlr->feat.async_event_configuration.bits.reserved1 = 0; + ctrlr->feat.async_event_configuration.bits.reserved2 = 0; return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE; }