DRAFT: Timberland 1.0 final v5 works - doesn't work #3

johnmeneghini · 2023-11-01T21:25:25Z

Rebase all SPDK changes to the tip of the SPDK master branch and refactor code.

More work is needed to make these changes ready for upstream.

Signed-off-by: John Meneghini <jmeneghi@redhat.com>

Includes needed support for the 1.0 reference implementation of NVM Express Boot Specification support for Tianocore edk2 as a submodule library for the host driver. Support to populate NBFT for failed cases (#1) This change is to populate the details of failed connections to NBFT. This applies to such subsystems, for which attempt has been configured but the NVMe-oF driver was unable to create a successful connection to them. This change will enable the booting OS to get the details of such subsystems from NBFT and retry the connection to proceed with booting from them. --------- Signed-off-by: Amit Jain <amit_jain9@dell.com> Co-authored-by: Hrishikesh Gokhale <Hrishikesh_Gokhale@dell.com> Co-authored-by: Maciej Rabęda <maciej.rabeda@intel.com> Co-authored-by: Doug Farley <Douglas_Farley@dell.com> Co-authored-by: Martin Wilck <mwilck@suse.com> Signed-off-by: John Meneghini <jmeneghi@redhat.com>

johnmeneghini

These are some of the changes that will be needed to get this stuff upstream.

johnmeneghini · 2023-11-01T21:34:19Z

lib/nvme/nvme.c

@@ -622,11 +622,14 @@ nvme_driver_init(void)
 	nvme_robust_mutex_lock(&g_spdk_nvme_driver->lock);

 	g_spdk_nvme_driver->initialized = false;
+#ifdef _MSC_VER
+	g_spdk_nvme_driver->hotplug_fd = -1;


Why is this here?

johnmeneghini · 2023-11-01T21:35:48Z

lib/nvme/nvme_ctrlr.c

@@ -4275,7 +4283,7 @@ nvme_keep_alive_completion(void *cb_ctx, const struct spdk_nvme_cpl *cpl)
 * Check if we need to send a Keep Alive command.
 * Caller must hold ctrlr->ctrlr_lock.
 */
-static int
+int


This will show up as a unused global function so we may need to come up with a new global API for this.

johnmeneghini · 2023-11-01T22:02:48Z

lib/nvme/nvme_fabric.c

@@ -20,12 +20,17 @@ struct nvme_fabric_prop_ctx {
 	void			*cb_arg;
 };

+#ifdef _MSC_VER
+struct spdk_nvme_fail_trid   *fail_trid;
+LIST_ENTRY                   fail_conn;


This LIST_ENTRY stuff is completely bogus and need to be replaced with an SPDK function list head.
Something from include/spdk/queue_extras.h
which is also there in NetworkPkg/Library/DxeSpdkLib/SpdkShim/sys/queue.h

johnmeneghini · 2023-11-01T22:03:00Z

lib/nvme/nvme_fabric.c

+		SPDK_ERRLOG("Failed to allocate for failed trid info\n");
+	}
+	memcpy(&fail_trid->trid, &trid, sizeof(struct spdk_nvme_transport_id));
+	InsertTailList(&fail_conn, &fail_trid->link);


This InsertTailList() function comes from MdePkg/Include/Library/BaseLib.h and needs to be replaced with an SPDK queue function.

johnmeneghini · 2023-11-01T22:04:18Z

lib/nvme/nvme_fabric.c

@@ -341,7 +360,13 @@ nvme_fabric_discover_probe(struct spdk_nvmf_discovery_log_page_entry *entry,
 	/* Copy the priority from the discovery ctrlr */
 	trid.priority = discover_priority;

-	nvme_ctrlr_probe(&trid, probe_ctx, NULL);
+#ifdef _MSC_VER


We need to get rid of all these _MSC_VER compile statements. The upstream community won't accept these in the base library code.

johnmeneghini · 2023-11-01T22:05:26Z

lib/nvme/nvme_internal.h

+#ifdef _MSC_VER
+struct spdk_nvme_fail_trid {
+	struct spdk_nvme_transport_id  trid;
+	LIST_ENTRY                     link;


More bogus LIST_ENTRY stuff I had to ifdef out in order to get SPDK to compile.

johnmeneghini · 2023-11-01T23:04:18Z

lib/nvme/nvme_tcp.c

@@ -2005,6 +2005,8 @@ nvme_tcp_read_pdu(struct nvme_tcp_qpair *tqpair, uint32_t *reaped, uint32_t max_
 		/* Wait for the pdu specific header  */
 		case NVME_TCP_PDU_RECV_STATE_AWAIT_PDU_PSH:
 			assert(pdu->psh_valid_bytes < pdu->psh_len);
+			SPDK_DEBUGLOG(nvme, "Read Protocol Specific Header. psh_valid_bytes: %d tqpair:%p\n",


Can these extra debug statements be removed?

johnmeneghini · 2023-11-01T23:04:45Z

lib/nvme/nvme_tcp.c

@@ -2023,6 +2025,7 @@ nvme_tcp_read_pdu(struct nvme_tcp_qpair *tqpair, uint32_t *reaped, uint32_t max_
 			break;
 		case NVME_TCP_PDU_RECV_STATE_AWAIT_PDU_PAYLOAD:
 			/* check whether the data is valid, if not we just return */
+			SPDK_DEBUGLOG(nvme, "Read Payload Data\n");


Can these extra debug statements be removed?

johnmeneghini · 2023-11-01T23:04:58Z

lib/nvme/nvme_tcp.c

@@ -2590,6 +2596,8 @@ nvme_tcp_ctrlr_construct(const struct spdk_nvme_transport_id *trid,
 	struct nvme_tcp_ctrlr *tctrlr;
 	int rc;

+	SPDK_DEBUGLOG(nvme, "nvme_tcp_ctrlr_construct Entered\n");


Can these extra debug statements be removed?

johnmeneghini · 2023-11-01T23:06:16Z

lib/sock/Makefile

@@ -14,7 +14,7 @@ C_SRCS = sock.c sock_rpc.c

 LIBNAME = sock

-CFLAGS += -Wpointer-arith
+CFLAGS += -Wpointer-arith -Wno-unused-function


This can be removed once the _MSC_VER stuff is properly abstracted.

Signed-off-by: John Meneghini <jmeneghi@redhat.com>

This patch which changes the way definitions are included in nvme_internal.h is required in order to make EDK2 compile spdk correctly. The problem is, including this breaks the spdk native build. So I can't compile spdk with this patch, and I can't compile edk2 without this patch. Also with this final change edk2 compiles spdk successfully but then fails to link with the following unresolved symbol. Building ... /edk2/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.inf [X64] /edk2/NetworkPkg/Library/DxeSpdkLib/SpdkShim/edk_sock.c:923:4: error: ‘struct spdk_net_impl’ has no member named ‘get_placement_id’ .get_placement_id = edk_sock_get_placement_id, ^~~~~~~~~~~~~~~~ This is because of the following upstream change in spdk. Upstream they have removed the get_placement_id api from the spdk network library. They've replaced it with a new api called "group_impl_get_optimal". commit 5379aa9 Author: Ben Walker <benjamin.walker@intel.com> Date: Thu Apr 1 13:07:17 2021 -0700 sock: Each module now maintains its own sock_map This allows for different policies per module, as well as overlapped placement_id values. Change-Id: I0a9c83e68d22733d81f005eb054a4c5f236f88d9 Signed-off-by: Ben Walker <benjamin.walker@intel.com> Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/7221 Tested-by: SPDK CI Jenkins <sys_sgci@intel.com> Reviewed-by: Jim Harris <james.r.harris@intel.com> Reviewed-by: Aleksey Marchuk <alexeymar@mellanox.com> diff --git a/include/spdk_internal/sock.h b/include/spdk_internal/sock.h index 13249d007..d32ed0bab 100644 --- a/include/spdk_internal/sock.h +++ b/include/spdk_internal/sock.h @@ -84,6 +84,11 @@ struct spdk_sock_group_impl { STAILQ_ENTRY(spdk_sock_group_impl) link; }; +struct spdk_sock_map { + STAILQ_HEAD(, spdk_sock_placement_id_entry) entries; + pthread_mutex_t mtx; +}; + struct spdk_net_impl { const char *name; int priority; @@ -109,7 +114,7 @@ struct spdk_net_impl { bool (*is_ipv4)(struct spdk_sock *sock); bool (*is_connected)(struct spdk_sock *sock); - int (*get_placement_id)(struct spdk_sock *sock, int *placement_id); + struct spdk_sock_group *(*group_impl_get_optimal)(struct spdk_sock *sock); struct spdk_sock_group_impl *(*group_impl_create)(void); int (*group_impl_add_sock)(struct spdk_sock_group_impl *group, struct spdk_sock *sock); int (*group_impl_remove_sock)(struct spdk_sock_group_impl *group, struct spdk_sock *sock); @@ -300,8 +305,6 @@ spdk_sock_get_placement_id(int fd, enum spdk_placement_mode mode, int *placement } } -extern struct spdk_sock_map g_map; - Signed-off-by: John Meneghini <jmeneghi@redhat.com>

johnmeneghini · 2023-11-07T17:23:15Z

lib/nvme/nvme_internal.h

@@ -13,9 +13,11 @@

 #include "spdk/nvme.h"

+#ifdef _MSC_VER


This change breaks the SPDK upstream build. I can't compile spdk with this change, but I also can't compile spdk with edk2 unless I have it. So this is another change that we can't push upstream.

johnmeneghini added 2 commits November 1, 2023 16:13

Timberland 1.0 Makefile changes

ba2acd4

Signed-off-by: John Meneghini <jmeneghi@redhat.com>

johnmeneghini requested review from LennySzubowicz, cayton, rahlvers, mwilck, Douglas-Farley and trevor-cockrell November 1, 2023 21:25

johnmeneghini self-assigned this Nov 1, 2023

johnmeneghini mentioned this pull request Nov 1, 2023

DRAFT: Rebase spdk branch to SPDK 23.09 - doesn't work timberland-sig/edk2#27

Open

johnmeneghini commented Nov 1, 2023

View reviewed changes

johnmeneghini changed the base branch from master to timberland-1.0_final November 2, 2023 16:56

johnmeneghini changed the base branch from timberland-1.0_final to master November 2, 2023 16:57

johnmeneghini added 2 commits November 7, 2023 11:50

patch: fix spdk compile warning

619e89a

Signed-off-by: John Meneghini <jmeneghi@redhat.com>

johnmeneghini force-pushed the timberland-1.0_final_v5_works branch from 2441cc8 to 5702aa2 Compare November 7, 2023 17:04

johnmeneghini commented Nov 7, 2023

View reviewed changes

johnmeneghini changed the title ~~Timberland 1.0 final v5 works~~ DRAFT: Timberland 1.0 final v5 works - doesn't work Nov 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DRAFT: Timberland 1.0 final v5 works - doesn't work #3

DRAFT: Timberland 1.0 final v5 works - doesn't work #3

johnmeneghini commented Nov 1, 2023

johnmeneghini left a comment

johnmeneghini Nov 1, 2023

johnmeneghini Nov 1, 2023

johnmeneghini Nov 1, 2023

johnmeneghini Nov 1, 2023

johnmeneghini Nov 1, 2023

johnmeneghini Nov 1, 2023

johnmeneghini Nov 1, 2023

johnmeneghini Nov 1, 2023

johnmeneghini Nov 1, 2023

johnmeneghini Nov 1, 2023

johnmeneghini Nov 7, 2023

DRAFT: Timberland 1.0 final v5 works - doesn't work #3

Are you sure you want to change the base?

DRAFT: Timberland 1.0 final v5 works - doesn't work #3

Conversation

johnmeneghini commented Nov 1, 2023

johnmeneghini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment