From b20f3678ddafe3a50a168f2b0b913498357fb29c Mon Sep 17 00:00:00 2001 From: Konrad Sztyber Date: Fri, 5 Aug 2022 05:48:17 +0200 Subject: [PATCH] env/pci: method for registering PCI device providers The primary motivation for this patch is to allow the VMD driver to be notified of when users wants to attach a device under a given BDF and to make it more similar to the regular PCI path. Currently, the way the VMD driver scans for the devices is a little bit different. The initial scan is done during initialization and there's a separate poller for checking hotplugs. Also, there's no device_attach() interface, so with hotplug poller disabled, it isn't possible to attach to a device not present in the initial scan, even if the BDF is known. This causes a few issues. First of all, the VMD library isn't notified when a device is stopped being used (i.e. user calls spdk_pci_device_detach()), so when such a device is hotremoved, it never gets unhooked. But we cannot simply add a spdk_pci_device.detach() callback, as this would break cases when user detaches a device (without hotremove) and then tries to reattach it again (via spdk_pci_device_attach()), as the VMD doesn't get notified about the device_attach() call. So, in order to resolve this, a device_attach() callback is added, which will notify the VMD library that the user wants to attach a device under a specific PCI address. Then, in subsequent patches, a spdk_pci_device_provider.detach_cb() callback is added to make sure that devices are unhooked once they're no longer used. Once that is done, it'll be also possible to get rid of the VMD hotplug poller by adding something like scan_cb() to spdk_pci_device_provider and call it from spdk_pci_enumerate(). Signed-off-by: Konrad Sztyber Change-Id: I084a27dcd12455f0f841440b7692375e80d07e84 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/13883 Community-CI: Mellanox Build Bot Tested-by: SPDK CI Jenkins Reviewed-by: Jim Harris Reviewed-by: Tom Nabarro Reviewed-by: Ben Walker --- include/spdk/env.h | 28 ++++++++++++++ lib/env_dpdk/Makefile | 2 +- lib/env_dpdk/pci.c | 68 +++++++++++++++++++++++++--------- lib/env_dpdk/spdk_env_dpdk.map | 1 + 4 files changed, 80 insertions(+), 19 deletions(-) diff --git a/include/spdk/env.h b/include/spdk/env.h index 104cfd581..2e17d26c2 100644 --- a/include/spdk/env.h +++ b/include/spdk/env.h @@ -1168,6 +1168,34 @@ void spdk_pci_unhook_device(struct spdk_pci_device *dev); */ const char *spdk_pci_device_get_type(const struct spdk_pci_device *dev); +struct spdk_pci_device_provider { + const char *name; + + /** + * Callback executed to attach a PCI device on a given address. + * + * \param addr address of the device. + * + * \return 0 if the device was attached successfully, negative errno otherwise. + */ + int (*attach_cb)(const struct spdk_pci_addr *addr); + + TAILQ_ENTRY(spdk_pci_device_provider) tailq; +}; + +/** + * Register a PCI device provdier. + * + * \param provider PCI device provider. + */ +void spdk_pci_register_device_provider(struct spdk_pci_device_provider *provider); + +#define SPDK_PCI_REGISTER_DEVICE_PROVIDER(name, provider) \ + static void __attribute__((constructor)) _spdk_pci_register_device_provider_##name(void) \ + { \ + spdk_pci_register_device_provider(provider); \ + } + /** * Remove any CPU affinity from the current thread. */ diff --git a/lib/env_dpdk/Makefile b/lib/env_dpdk/Makefile index f1a604abd..97e5bca13 100644 --- a/lib/env_dpdk/Makefile +++ b/lib/env_dpdk/Makefile @@ -7,7 +7,7 @@ SPDK_ROOT_DIR := $(abspath $(CURDIR)/../..) include $(SPDK_ROOT_DIR)/mk/spdk.common.mk SO_VER := 9 -SO_MINOR := 0 +SO_MINOR := 1 CFLAGS += $(ENV_CFLAGS) C_SRCS = env.c memory.c pci.c init.c threads.c diff --git a/lib/env_dpdk/pci.c b/lib/env_dpdk/pci.c index ddbd866f2..16fa2de94 100644 --- a/lib/env_dpdk/pci.c +++ b/lib/env_dpdk/pci.c @@ -35,6 +35,8 @@ static TAILQ_HEAD(, spdk_pci_device) g_pci_devices = TAILQ_HEAD_INITIALIZER(g_pc static TAILQ_HEAD(, spdk_pci_device) g_pci_hotplugged_devices = TAILQ_HEAD_INITIALIZER(g_pci_hotplugged_devices); static TAILQ_HEAD(, spdk_pci_driver) g_pci_drivers = TAILQ_HEAD_INITIALIZER(g_pci_drivers); +static TAILQ_HEAD(, spdk_pci_device_provider) g_pci_device_providers = + TAILQ_HEAD_INITIALIZER(g_pci_device_providers); struct env_devargs { struct rte_bus *bus; @@ -606,18 +608,45 @@ scan_pci_bus(bool delay_init) return 0; } +static int +pci_attach_rte(const struct spdk_pci_addr *addr) +{ + char bdf[32]; + int rc, i = 0; + + spdk_pci_addr_fmt(bdf, sizeof(bdf), addr); + + do { + rc = rte_eal_hotplug_add("pci", bdf, ""); + } while (rc == -ENOMSG && ++i <= DPDK_HOTPLUG_RETRY_COUNT); + + if (i > 1 && rc == -EEXIST) { + /* Even though the previous request timed out, the device + * was attached successfully. + */ + rc = 0; + } + + return rc; +} + +static struct spdk_pci_device_provider g_pci_rte_provider = { + .name = "pci", + .attach_cb = pci_attach_rte, +}; + +SPDK_PCI_REGISTER_DEVICE_PROVIDER(pci, &g_pci_rte_provider); + int spdk_pci_device_attach(struct spdk_pci_driver *driver, spdk_pci_enum_cb enum_cb, void *enum_ctx, struct spdk_pci_addr *pci_address) { struct spdk_pci_device *dev; + struct spdk_pci_device_provider *provider; struct rte_pci_device *rte_dev; struct rte_devargs *da; int rc; - char bdf[32]; - - spdk_pci_addr_fmt(bdf, sizeof(bdf), pci_address); cleanup_pci_devices(); @@ -645,17 +674,12 @@ spdk_pci_device_attach(struct spdk_pci_driver *driver, driver->cb_fn = enum_cb; driver->cb_arg = enum_ctx; - int i = 0; - - do { - rc = rte_eal_hotplug_add("pci", bdf, ""); - } while (rc == -ENOMSG && ++i <= DPDK_HOTPLUG_RETRY_COUNT); - - if (i > 1 && rc == -EEXIST) { - /* Even though the previous request timed out, the device - * was attached successfully. - */ - rc = 0; + rc = -ENODEV; + TAILQ_FOREACH(provider, &g_pci_device_providers, tailq) { + rc = provider->attach_cb(pci_address); + if (rc == 0) { + break; + } } driver->cb_arg = NULL; @@ -678,10 +702,12 @@ spdk_pci_device_attach(struct spdk_pci_driver *driver, assert(dev != NULL); rte_dev = dev->dev_handle; - da = rte_dev->device.devargs; - if (da && get_allowed_at(da)) { - set_allowed_at(da, spdk_get_ticks()); - da->policy = RTE_DEV_ALLOWED; + if (rte_dev != NULL) { + da = rte_dev->device.devargs; + if (da && get_allowed_at(da)) { + set_allowed_at(da, spdk_get_ticks()); + da->policy = RTE_DEV_ALLOWED; + } } return 0; @@ -1139,6 +1165,12 @@ spdk_pci_unhook_device(struct spdk_pci_device *dev) TAILQ_REMOVE(&g_pci_devices, dev, internal.tailq); } +void +spdk_pci_register_device_provider(struct spdk_pci_device_provider *provider) +{ + TAILQ_INSERT_TAIL(&g_pci_device_providers, provider, tailq); +} + const char * spdk_pci_device_get_type(const struct spdk_pci_device *dev) { diff --git a/lib/env_dpdk/spdk_env_dpdk.map b/lib/env_dpdk/spdk_env_dpdk.map index d6be0394b..1fc151df8 100644 --- a/lib/env_dpdk/spdk_env_dpdk.map +++ b/lib/env_dpdk/spdk_env_dpdk.map @@ -97,6 +97,7 @@ spdk_pci_hook_device; spdk_pci_unhook_device; spdk_pci_device_get_type; + spdk_pci_register_device_provider; spdk_unaffinitize_thread; spdk_call_unaffinitized; spdk_mem_map_alloc;