Code review updates for bitfields and more

Author: chorkin

src/nvme/mi-mctp.c

@@ -128,7 +128,6 @@ static struct __mi_mctp_socket_ops ops = {
 	nvme_mi_aem_socket,
 	sendmsg,
 	recvmsg,
-	recv,
 	poll,
 	ioctl_tag,
 };
@@ -269,15 +268,24 @@ static int nvme_mi_mctp_aem_fd(struct nvme_mi_ep *ep)
 
 static int nvme_mi_mctp_aem_purge(struct nvme_mi_ep *ep)
 {
-	char buffer;
 	struct nvme_mi_transport_mctp *mctp = ep->transport_data;
+	struct msghdr msg = {0};
+	struct iovec iov;
+	char buffer;
+
+	iov.iov_base = &buffer;
+	iov.iov_len = sizeof(buffer);
+	msg.msg_iov = &iov;
+	msg.msg_iovlen = 1;
 
 	// Read until there is no more data
-	ops.recv(mctp->sd_aem, &buffer, sizeof(buffer), MSG_TRUNC);
+	while (ops.recvmsg(mctp->sd_aem, &msg, MSG_TRUNC) > 0)
+		;
 
 	return 0;
 }
 
+
 static int nvme_mi_mctp_aem_read(struct nvme_mi_ep *ep,
 			       struct nvme_mi_resp *resp)
 {

src/nvme/mi.c

@@ -23,6 +23,19 @@
 
 #define NUM_ENABLES    (256u)
 
+_Static_assert(sizeof(struct nvme_mi_aem_supported_list_header) == 5,
+	"size_of_nvme_mi_aem_supported_list_header_is_not_5_bytes");
+_Static_assert(sizeof(struct nvme_mi_aem_supported_item) == 3,
+	"sizeof_nvme_mi_aem_supported_item_is_not_3_bytes");
+_Static_assert(sizeof(struct nvme_mi_aem_enable_item) == 3,
+	"size_of_ae_enable_item_t_is_not_3_bytes");
+_Static_assert(sizeof(struct nvme_mi_aem_enable_list_header) == 5,
+	"size_of_nvme_mi_aem_enable_list_header_is_not_5_bytes");
+_Static_assert(sizeof(struct nvme_mi_aem_occ_data) == 9,
+	"size_of_nvme_mi_aem_occ_data_is_not_9_bytes");
+_Static_assert(sizeof(struct nvme_mi_aem_occ_list_hdr) == 7,
+	"size_of_nvme_mi_aem_occ_list_hdr_is_not_7_bytes");
+
 static int nvme_mi_get_async_message(nvme_mi_ep_t ep,
 	struct nvme_mi_aem_msg *aem_msg, size_t *aem_msg_len);
 
@@ -2172,17 +2185,19 @@ static int validate_occ_list_update_ctx(
 		//Make sure header is the right version and length
 		errno = EPROTO;
 		goto err_cleanup;
-	} else if (occ_header->aeolli.aeoltl > len) {
+	} else if (nvme_mi_aem_aeolli_get_aeoltl(occ_header->aeolli) > len) {
 		//Full length is bigger than the data that was received
 		errno = EPROTO;
 		goto err_cleanup;
 	} else if (check_generation_num &&
-				ctx->last_generation_num == (int) occ_header->aemti.aemgn) {
+		ctx->last_generation_num ==
+			(int) nvme_mi_aem_aemti_get_aemgn(occ_header->aemti)) {
 		//This is a duplicate and shouldn't be parsed.
 		//Let's just act like there's no updates
 		occ_header->numaeo = 0;
 	} else if (check_generation_num) {
-		ctx->last_generation_num = occ_header->aemti.aemgn;
+		ctx->last_generation_num = 
+			nvme_mi_aem_aemti_get_aemgn(occ_header->aemti);
 	}
 
 	//Header is fine.  Let's go through the data
@@ -2212,7 +2227,7 @@ static int validate_occ_list_update_ctx(
 		uint32_t offset = sizeof(*current) + current->aeosil + current->aeovsil;
 
 		bytes_so_far += offset;
-		if (bytes_so_far > occ_header->aeolli.aeoltl) {
+		if (bytes_so_far > nvme_mi_aem_aeolli_get_aeoltl(occ_header->aeolli)) {
 			errno = EPROTO;
 			goto err_cleanup;
 		}
@@ -2316,8 +2331,8 @@ static int aem_disable_enabled(nvme_mi_ep_t ep)
 
 	for (int i = 0; i < NUM_ENABLES; i++) {
 		if (already_enabled.enabled[i]) {
-			sync_data[sync_data_count].aeei.aeeid = i;
-			sync_data[sync_data_count].aeei.aee = false;
+			nvme_mi_aem_aeei_set_aeeid(i, &sync_data[sync_data_count]);
+			nvme_mi_aem_aeei_set_aee(false, &sync_data[sync_data_count]);
 			sync_data_count++;
 		}
 	}
@@ -2374,8 +2389,8 @@ int nvme_mi_aem_enable(nvme_mi_ep_t ep,
 	//Now, let's do a fresh enable of what's asked
 	for (int i = 0; i < NUM_ENABLES; i++) {
 		if (config->enabled_map.enabled[i]) {
-			sync_data[sync_data_count].aeei.aeeid = i;
-			sync_data[sync_data_count].aeei.aee = true;
+			nvme_mi_aem_aeei_set_aeeid(i, &sync_data[sync_data_count]);
+			nvme_mi_aem_aeei_set_aee(true, &sync_data[sync_data_count]);
 			sync_data_count++;
 		}
 	}
@@ -2438,8 +2453,11 @@ int nvme_mi_aem_get_enabled(nvme_mi_ep_t ep,
 	struct nvme_mi_aem_enable_item *items =
 		(struct nvme_mi_aem_enable_item *)(enabled_list + 1);
 
-	for (int i = 0; i < enabled_list->hdr.numaes; i++)
-		enabled_map->enabled[items[i].aeei.aeeid] = items[i].aeei.aee;
+	for (int i = 0; i < enabled_list->hdr.numaes; i++) {
+		__u8 aeeid = nvme_mi_aem_aeei_get_aeeid(items[i].aeei);
+		bool enabled = nvme_mi_aem_aeei_get_aee(items[i].aeei);
+		enabled_map->enabled[aeeid] = enabled;
+	}
 
 cleanup:
 	free(enabled_list);
@@ -2547,7 +2565,7 @@ int nvme_mi_aem_process(nvme_mi_ep_t ep, void *userdata)
 		if (action == NVME_MI_AEM_HNA_ACK) {
 			response_len = sizeof(response_buffer);
 
-			rc = aem_ack(ep, &response->occ_list_hdr, &response_len);
+			rc = nvme_mi_aem_ack(ep, &response->occ_list_hdr, &response_len);
 			if (rc)
 				goto cleanup;
 

src/nvme/mi.h

@@ -86,6 +86,7 @@
 
 #include <endian.h>
 #include <stdint.h>
+#include <ccan/endian/endian.h>
 
 #include "types.h"
 #include "tree.h"
@@ -327,9 +328,6 @@ struct nvme_mi_aem_supported_list_header {
 	__le16 aest;
 	__u8 aeslhl; //Should be 5
 } __attribute__((packed));
-_Static_assert(sizeof(struct nvme_mi_aem_supported_list_header) == 5,
-	"size_of_nvme_mi_aem_supported_list_header_is_not_5_bytes");
-
 
 /**
  * struct nvme_mi_aem_supported_item - AE Supported List Item
@@ -341,14 +339,33 @@ _Static_assert(sizeof(struct nvme_mi_aem_supported_list_header) == 5,
  */
 struct nvme_mi_aem_supported_item {
 	__u8 aesl;//Length of this item.  Set to 3
-	struct {
-		__le16 aeis     : 8; //Identifier of supported ae
-		__le16 reserved : 7;
-		__le16 aese     : 1; //AE Support Enabled
-	} __attribute__((packed)) aesi;
+	__le16 aesi;
 } __attribute__((packed));
-_Static_assert(sizeof(struct nvme_mi_aem_supported_item) == 3,
-	"sizeof_nvme_mi_aem_supported_item_is_not_3_bytes");
+
+static inline bool nvme_mi_aem_aesi_get_aese(__le16 aesi)
+{
+	return !!(le16_to_cpu(aesi) & 0x8000);
+}
+
+static inline __u8 nvme_mi_aem_aesi_get_aesid(__le16 aesi)
+{
+	return le16_to_cpu(aesi) & 0xff;
+}
+
+static inline void nvme_mi_aem_aesi_set_aesid(__u8 aesid, struct nvme_mi_aem_supported_item *item)
+{
+	__u16 temp = le16_to_cpu(item->aesi);
+
+	item->aesi = cpu_to_le16((temp & 0xFF00) | aesid);
+}
+
+static inline void nvme_mi_aem_aesi_set_aee(bool enabled, struct nvme_mi_aem_supported_item *item)
+{
+	__u16 temp = le16_to_cpu(item->aesi);
+	__u8 bit = (enabled) ? 1 : 0;
+
+	item->aesi = cpu_to_le16((temp & 0xFF) | (bit << 15));
+}
 
 /**
  * struct nvme_mi_aem_supported_list - AE Supported List received with GET CONFIG Asynchronous Event
@@ -369,14 +386,33 @@ struct nvme_mi_aem_supported_list {
  */
 struct nvme_mi_aem_enable_item {
 	__u8 aeel;
-	struct{
-		__le16 aeeid    : 8; //AE identifier
-		__le16 reserved : 7;
-		__le16 aee      : 1; //AE enabled bit
-	} __attribute__((packed)) aeei;
-};
-_Static_assert(sizeof(struct nvme_mi_aem_enable_item) == 3,
-	"size_of_ae_enable_item_t_is_not_3_bytes");
+	__le16 aeei;
+}__attribute__((packed)) ;
+
+static inline bool nvme_mi_aem_aeei_get_aee(__le16 aeei)
+{
+	return !!(le16_to_cpu(aeei) & 0x8000);
+}
+
+static inline __u8 nvme_mi_aem_aeei_get_aeeid(__le16 aeei)
+{
+	return (le16_to_cpu(aeei) & 0xFF);
+}
+
+static inline void nvme_mi_aem_aeei_set_aeeid(__u8 aeeid, struct nvme_mi_aem_enable_item *item)
+{
+	__u16 temp = le16_to_cpu(item->aeei);
+
+	item->aeei = cpu_to_le16((temp & 0xFF00) | aeeid);
+}
+
+static inline void nvme_mi_aem_aeei_set_aee(bool enabled, struct nvme_mi_aem_enable_item *item)
+{
+	__u16 temp = le16_to_cpu(item->aeei);
+	__u8 bit = (enabled) ? 1 : 0;
+
+	item->aeei = cpu_to_le16((temp & 0xFF) | (bit << 15));
+}
 
 /**
  * struct nvme_mi_aem_enable_list_header - AE Enable list header
@@ -391,8 +427,6 @@ struct nvme_mi_aem_enable_list_header {
 	__le16 aeetl;
 	__u8 aeelhl;
 } __attribute__((packed));
-_Static_assert(sizeof(struct nvme_mi_aem_enable_list_header) == 5,
-	"size_of_nvme_mi_aem_enable_list_header_is_not_5_bytes");
 
 /**
  * struct nvme_mi_aem_enable_list - AE enable list sent with SET CONFIG Asyncronous Event
@@ -428,8 +462,6 @@ struct nvme_mi_aem_occ_data {
 		__u8 aessi;
 	} __attribute__((packed)) aeoui;
 } __attribute__((packed));
-_Static_assert(sizeof(struct nvme_mi_aem_occ_data) == 9,
-	"size_of_nvme_mi_aem_occ_data_is_not_9_bytes");
 
 /**
  * struct nvme_mi_aem_occ_list_hdr - AE occurrence list header
@@ -446,18 +478,33 @@ _Static_assert(sizeof(struct nvme_mi_aem_occ_data) == 9,
 struct nvme_mi_aem_occ_list_hdr {
 	__u8 numaeo;
 	__u8 aelver;
-	struct {
-		unsigned int aeoltl: 23;
-		unsigned int overflow: 1;
-	} __attribute__((packed)) aeolli;
+	__u8 aeolli[3];//24-bits
 	__u8 aeolhl;
-	struct {
-		unsigned int aemrc: 3;
-		unsigned int aemgn: 5;
-	} __attribute__((packed)) aemti;
+	__u8 aemti;
 } __attribute__((packed));
-_Static_assert(sizeof(struct nvme_mi_aem_occ_list_hdr) == 7,
-	"size_of_nvme_mi_aem_occ_list_hdr_is_not_7_bytes");
+
+static inline __u8 nvme_mi_aem_aemti_get_aemgn(__u8 aemti)
+{
+	return aemti >> 3 & 0x1f;
+}
+
+static inline __u32 nvme_mi_aem_aeolli_get_aeoltl(__u8 *aeolli)
+{
+	//First 23-bits contain the aeoltl
+	__u32 aeoltl = le32_to_cpu(aeolli[0] | (aeolli[1] << 8) | (aeolli[2] << 16));
+
+	return aeoltl & 0x7FFFFF;
+}
+
+static inline void nvme_mi_aem_aeolli_set_aeoltl(__u32 aeoltl, struct nvme_mi_aem_occ_list_hdr *hdr)
+{
+	//First 23-bits contain the aeoltl
+	__u32 le_bytes = cpu_to_le32(aeoltl);
+
+	hdr->aeolli[0] = le_bytes & 0xFF;
+	hdr->aeolli[1] = (le_bytes >> 8) & 0xFF;
+	hdr->aeolli[2] = (hdr->aeolli[2] & 0b10000000) | ((le_bytes >> 16) & 0x7F);
+}
 
 
 /**
@@ -1013,7 +1060,7 @@ int nvme_mi_mi_read_mi_data_ctrl(nvme_mi_ep_t ep, __u16 ctrl_id,
  *
  * See &struct nvme_mi_nvm_ss_health_status.
  *
- * Return: The nvme command status if a response was received (see
+ * Return`The nvme command status if a response was received (see
  * &enum nvme_status_field) or -1 with errno set otherwise..
  */
 int nvme_mi_mi_subsystem_health_status_poll(nvme_mi_ep_t ep, bool clear,
@@ -1249,9 +1296,9 @@ int nvme_mi_mi_config_set_async_event(nvme_mi_ep_t ep,
 				struct nvme_mi_aem_occ_list_hdr *occ_list,
 				size_t *occ_list_num_bytes);
 
-static inline int aem_ack(nvme_mi_ep_t ep,
-							struct nvme_mi_aem_occ_list_hdr *occ_list,
-							size_t *occ_list_num_bytes)
+static inline int nvme_mi_aem_ack(nvme_mi_ep_t ep,
+				struct nvme_mi_aem_occ_list_hdr *occ_list,
+				size_t *occ_list_num_bytes)
 {
 	//An AEM Ack is defined as a SET CONFIG AE with no AE enable items
 	struct nvme_mi_aem_enable_list list = {0};

src/nvme/private.h

@@ -309,7 +309,6 @@ struct __mi_mctp_socket_ops {
 	int (*aem_socket)(__u8 eid, unsigned int network);
 	ssize_t (*sendmsg)(int, const struct msghdr *, int);
 	ssize_t (*recvmsg)(int, struct msghdr *, int);
-	ssize_t (*recv)(int __fd, void *__buf, size_t __n, int __flags);
 	int (*poll)(struct pollfd *, nfds_t, int);
 	int (*ioctl_tag)(int, unsigned long, struct mctp_ioc_tag_ctl *);
 };