Commit 68ff34bc authored by Nick Connolly's avatar Nick Connolly Committed by Tomasz Zawadzki
Browse files

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: default avatarNick Connolly <nick.connolly@mayadata.io>
Change-Id: I7d8163c84b4f410fc95a5b7064506ad7b4b62c6c
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/6340


Community-CI: Broadcom CI
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
Reviewed-by: default avatarAleksey Marchuk <alexeymar@mellanox.com>
parent 424cbc39

include/spdk/nvme_spec.h

+7 −6
Original line number Original line Diff line number Diff line
@@ -712,13 +712,14 @@ union spdk_nvme_feat_async_event_configuration { 
	uint32_t raw;

	uint32_t raw;

	struct {

	struct {

		union spdk_nvme_critical_warning_state crit_warn;

		union spdk_nvme_critical_warning_state crit_warn;

		uint32_t ns_attr_notice		: 1;

		uint8_t ns_attr_notice		: 1;

		uint32_t fw_activation_notice	: 1;

		uint8_t fw_activation_notice	: 1;

		uint32_t telemetry_log_notice	: 1;

		uint8_t telemetry_log_notice	: 1;

		uint32_t ana_change_notice	: 1;

		uint8_t ana_change_notice	: 1;

		uint32_t reserved		: 19;

		uint8_t reserved1		: 4;

		uint16_t reserved2		: 15;

		/** Discovery log change (refer to the NVMe over Fabrics specification) */

		/** 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;

	} bits;

};

};

SPDK_STATIC_ASSERT(sizeof(union spdk_nvme_feat_async_event_configuration) == 4, "Incorrect size");

SPDK_STATIC_ASSERT(sizeof(union spdk_nvme_feat_async_event_configuration) == 4, "Incorrect size");

lib/nvmf/ctrlr.c

+2 −1
Original line number Original line Diff line number Diff line
@@ -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",

	SPDK_DEBUGLOG(nvmf, "Set Features - Async Event Configuration, cdw11 0x%08x\n",

		      cmd->cdw11);

		      cmd->cdw11);

	ctrlr->feat.async_event_configuration.raw = 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;

	return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE;

}

}