[RFC 0/2] Intel IFC VF driver for vdpa

[RFC 0/2] Intel IFC VF driver for vdpa

[Zhu Lingshan]

Hi all:
 
This series intends to introduce Intel IFC VF NIC driver for Vhost
Data Plane Acceleration.
 
Here comes two main parts, one is ifcvf_base layer, which handles
hardware operations. The other is ifcvf_main layer handles VF
initialization, configuration and removal, which depends on
and complys to vhost_mdev https://lkml.org/lkml/2019/9/26/15 
 
This is a first RFC try, please help review.
 
Thanks!
BR
Zhu Lingshan


Zhu Lingshan (2):
  vhost: IFC VF hardware operation layer
  vhost: IFC VF vdpa layer

 drivers/vhost/ifcvf/ifcvf_base.c | 390 ++++++++++++++++++++++++++++
 drivers/vhost/ifcvf/ifcvf_base.h | 137 ++++++++++
 drivers/vhost/ifcvf/ifcvf_main.c | 541 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 1068 insertions(+)
 create mode 100644 drivers/vhost/ifcvf/ifcvf_base.c
 create mode 100644 drivers/vhost/ifcvf/ifcvf_base.h
 create mode 100644 drivers/vhost/ifcvf/ifcvf_main.c

-- 
2.16.4

[RFC 1/2] vhost: IFC VF hardware operation layer

[Zhu Lingshan]

This commit introduced ifcvf_base layer, which handles IFC VF NIC
hardware operations and configurations.

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 drivers/vhost/ifcvf/ifcvf_base.c | 390 +++++++++++++++++++++++++++++++++++++++
 drivers/vhost/ifcvf/ifcvf_base.h | 137 ++++++++++++++
 2 files changed, 527 insertions(+)
 create mode 100644 drivers/vhost/ifcvf/ifcvf_base.c
 create mode 100644 drivers/vhost/ifcvf/ifcvf_base.h

diff --git a/drivers/vhost/ifcvf/ifcvf_base.c b/drivers/vhost/ifcvf/ifcvf_base.c
new file mode 100644
index 000000000000..b85e14c9bdcf
--- /dev/null
+++ b/drivers/vhost/ifcvf/ifcvf_base.c
@@ -0,0 +1,390 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2019 Intel Corporation.
+ */
+
+#include "ifcvf_base.h"
+
+static void *get_cap_addr(struct ifcvf_hw *hw, struct virtio_pci_cap *cap)
+{
+	u8 bar = cap->bar;
+	u32 length = cap->length;
+	u32 offset = cap->offset;
+	struct ifcvf_adapter *ifcvf =
+		container_of(hw, struct ifcvf_adapter, vf);
+
+	if (bar >= IFCVF_PCI_MAX_RESOURCE) {
+		IFC_ERR(ifcvf->dev,
+			"Invalid bar number %u to get capabilities.\n", bar);
+		return NULL;
+	}
+
+	if (offset + length < offset) {
+		IFC_ERR(ifcvf->dev, "offset(%u) + length(%u) overflows\n",
+			offset, length);
+		return NULL;
+	}
+
+	if (offset + length > hw->mem_resource[cap->bar].len) {
+		IFC_ERR(ifcvf->dev,
+			"offset(%u) + len(%u) overflows bar%u to get capabilities.\n",
+			offset, length, bar);
+		return NULL;
+	}
+
+	return hw->mem_resource[bar].addr + offset;
+}
+
+int ifcvf_read_config_range(struct pci_dev *dev,
+			uint32_t *val, int size, int where)
+{
+	int i;
+
+	for (i = 0; i < size; i += 4) {
+		if (pci_read_config_dword(dev, where + i, val + i / 4) < 0)
+			return -1;
+	}
+	return 0;
+}
+
+int ifcvf_init_hw(struct ifcvf_hw *hw, struct pci_dev *dev)
+{
+	int ret;
+	u8 pos;
+	struct virtio_pci_cap cap;
+	u32 i;
+	u16 notify_off;
+
+	ret = pci_read_config_byte(dev, PCI_CAPABILITY_LIST, &pos);
+
+	if (ret < 0) {
+		IFC_ERR(&dev->dev, "failed to read PCI capability list.\n");
+		return -EIO;
+	}
+
+	while (pos) {
+		ret = ifcvf_read_config_range(dev, (u32 *)&cap,
+				sizeof(cap), pos);
+
+		if (ret < 0) {
+			IFC_ERR(&dev->dev, "failed to get PCI capability at %x",
+					pos);
+			break;
+		}
+
+		if (cap.cap_vndr != PCI_CAP_ID_VNDR)
+			goto next;
+
+		IFC_INFO(&dev->dev, "read PCI config:\n"
+					"config type: %u.\n"
+					"PCI bar: %u.\n"
+					"PCI bar offset: %u.\n"
+					"PCI config len: %u.\n",
+					cap.cfg_type, cap.bar,
+					cap.offset, cap.length);
+
+		switch (cap.cfg_type) {
+		case VIRTIO_PCI_CAP_COMMON_CFG:
+			hw->common_cfg = get_cap_addr(hw, &cap);
+			IFC_INFO(&dev->dev, "hw->common_cfg = %p.\n",
+					hw->common_cfg);
+			break;
+		case VIRTIO_PCI_CAP_NOTIFY_CFG:
+			pci_read_config_dword(dev, pos + sizeof(cap),
+				&hw->notify_off_multiplier);
+			hw->notify_bar = cap.bar;
+			hw->notify_base = get_cap_addr(hw, &cap);
+			IFC_INFO(&dev->dev, "hw->notify_base = %p.\n",
+					hw->notify_base);
+			break;
+		case VIRTIO_PCI_CAP_ISR_CFG:
+			hw->isr = get_cap_addr(hw, &cap);
+			IFC_INFO(&dev->dev, "hw->isr = %p.\n", hw->isr);
+			break;
+		case VIRTIO_PCI_CAP_DEVICE_CFG:
+			hw->dev_cfg = get_cap_addr(hw, &cap);
+			IFC_INFO(&dev->dev, "hw->dev_cfg = %p.\n", hw->dev_cfg);
+			break;
+		}
+next:
+		pos = cap.cap_next;
+	}
+
+	if (hw->common_cfg == NULL || hw->notify_base == NULL ||
+		hw->isr == NULL || hw->dev_cfg == NULL) {
+		IFC_ERR(&dev->dev, "Incomplete PCI capabilities.\n");
+		return -1;
+	}
+
+	for (i = 0; i < (IFCVF_MAX_QUEUE_PAIRS * 2); i++) {
+		iowrite16(i, &hw->common_cfg->queue_select);
+		notify_off = ioread16(&hw->common_cfg->queue_notify_off);
+		hw->notify_addr[i] = (void *)((u8 *)hw->notify_base +
+				notify_off * hw->notify_off_multiplier);
+	}
+
+	hw->lm_cfg = hw->mem_resource[4].addr;
+
+	IFC_INFO(&dev->dev, "PCI capability mapping:\n"
+				"common cfg: %p\n"
+				"notify base: %p\n"
+				"isr cfg: %p\n"
+				"device cfg: %p\n"
+				"multiplier: %u\n",
+				hw->common_cfg,
+				hw->notify_base,
+				hw->isr,
+				hw->dev_cfg,
+				hw->notify_off_multiplier);
+
+	return 0;
+}
+
+static u8 ifcvf_get_status(struct ifcvf_hw *hw)
+{
+	return ioread8(&hw->common_cfg->device_status);
+}
+
+static void ifcvf_set_status(struct ifcvf_hw *hw, u8 status)
+{
+	iowrite8(status, &hw->common_cfg->device_status);
+}
+
+static void ifcvf_reset(struct ifcvf_hw *hw)
+{
+	ifcvf_set_status(hw, 0);
+
+	/* flush status write */
+	ifcvf_get_status(hw);
+	hw->generation++;
+}
+
+static void ifcvf_add_status(struct ifcvf_hw *hw, u8 status)
+{
+	if (status != 0)
+		status |= ifcvf_get_status(hw);
+
+	ifcvf_set_status(hw, status);
+	ifcvf_get_status(hw);
+}
+
+u64 ifcvf_get_features(struct ifcvf_hw *hw)
+{
+	u32 features_lo, features_hi;
+	struct virtio_pci_common_cfg *cfg = hw->common_cfg;
+
+	iowrite32(0, &cfg->device_feature_select);
+	features_lo = ioread32(&cfg->device_feature);
+
+	iowrite32(1, &cfg->device_feature_select);
+	features_hi = ioread32(&cfg->device_feature);
+
+	return ((u64)features_hi << 32) | features_lo;
+}
+static int ifcvf_with_feature(struct ifcvf_hw *hw, u64 bit)
+{
+	return (hw->req_features & (1ULL << bit)) != 0;
+}
+
+static void ifcvf_read_dev_config(struct ifcvf_hw *hw, u64 offset,
+		       void *dst, int length)
+{
+	int i;
+	u8 *p;
+	u8 old_gen, new_gen;
+
+	do {
+		old_gen = ioread8(&hw->common_cfg->config_generation);
+
+		p = dst;
+		for (i = 0; i < length; i++)
+			*p++ = ioread8((u8 *)hw->dev_cfg + offset + i);
+
+		new_gen = ioread8(&hw->common_cfg->config_generation);
+	} while (old_gen != new_gen);
+}
+
+void ifcvf_get_linkstatus(struct ifcvf_hw *hw, u8 *is_linkup)
+{
+	u16 status;
+	u64 host_features;
+
+	host_features = ifcvf_get_features(hw);
+	if (ifcvf_with_feature(hw, VIRTIO_NET_F_STATUS)) {
+		ifcvf_read_dev_config(hw,
+				offsetof(struct ifcvf_net_config, status),
+				&status, sizeof(status));
+		if ((status & VIRTIO_NET_S_LINK_UP) == 0)
+			(*is_linkup) = 1;
+		else
+			(*is_linkup) = 0;
+	} else
+		(*is_linkup) = 0;
+}
+
+static void ifcvf_set_features(struct ifcvf_hw *hw, u64 features)
+{
+	struct virtio_pci_common_cfg *cfg = hw->common_cfg;
+
+	iowrite32(0, &cfg->guest_feature_select);
+	iowrite32(features & ((1ULL << 32) - 1), &cfg->guest_feature);
+
+	iowrite32(1, &cfg->guest_feature_select);
+	iowrite32(features >> 32, &cfg->guest_feature);
+}
+
+static int ifcvf_config_features(struct ifcvf_hw *hw)
+{
+	u64 host_features;
+	struct ifcvf_adapter *ifcvf =
+		container_of(hw, struct ifcvf_adapter, vf);
+
+	host_features = ifcvf_get_features(hw);
+	hw->req_features &= host_features;
+
+	ifcvf_set_features(hw, hw->req_features);
+	ifcvf_add_status(hw, VIRTIO_CONFIG_S_FEATURES_OK);
+
+	if (!(ifcvf_get_status(hw) & VIRTIO_CONFIG_S_FEATURES_OK)) {
+		IFC_ERR(ifcvf->dev, "Failed to set FEATURES_OK status\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static void io_write64_twopart(u64 val, u32 *lo, u32 *hi)
+{
+	iowrite32(val & ((1ULL << 32) - 1), lo);
+	iowrite32(val >> 32, hi);
+}
+
+static int ifcvf_hw_enable(struct ifcvf_hw *hw)
+{
+	struct virtio_pci_common_cfg *cfg;
+	u8 *lm_cfg;
+	u32 i;
+	struct ifcvf_adapter *ifcvf =
+		container_of(hw, struct ifcvf_adapter, vf);
+
+	cfg = hw->common_cfg;
+	lm_cfg = hw->lm_cfg;
+
+	iowrite16(IFCVF_MSI_CONFIG_OFF, &cfg->msix_config);
+	if (ioread16(&cfg->msix_config) == VIRTIO_MSI_NO_VECTOR) {
+		IFC_ERR(ifcvf->dev, "No msix vector for device config.\n");
+		return -1;
+	}
+
+	for (i = 0; i < hw->nr_vring; i++) {
+		iowrite16(i, &cfg->queue_select);
+		io_write64_twopart(hw->vring[i].desc, &cfg->queue_desc_lo,
+				&cfg->queue_desc_hi);
+		io_write64_twopart(hw->vring[i].avail, &cfg->queue_avail_lo,
+				&cfg->queue_avail_hi);
+		io_write64_twopart(hw->vring[i].used, &cfg->queue_used_lo,
+				&cfg->queue_used_hi);
+		iowrite16(hw->vring[i].size, &cfg->queue_size);
+
+		*(u32 *)(lm_cfg + IFCVF_LM_RING_STATE_OFFSET +
+				(i / 2) * IFCVF_LM_CFG_SIZE + (i % 2) * 4) =
+			(u32)hw->vring[i].last_avail_idx |
+			((u32)hw->vring[i].last_used_idx << 16);
+
+		iowrite16(i + IFCVF_MSI_QUEUE_OFF, &cfg->queue_msix_vector);
+		if (ioread16(&cfg->queue_msix_vector) ==
+				VIRTIO_MSI_NO_VECTOR) {
+			IFC_ERR(ifcvf->dev,
+				"No msix vector for queue %u.\n", i);
+			return -1;
+		}
+
+		iowrite16(1, &cfg->queue_enable);
+	}
+
+	return 0;
+}
+
+static void ifcvf_hw_disable(struct ifcvf_hw *hw)
+{
+	u32 i;
+	struct virtio_pci_common_cfg *cfg;
+
+	cfg = hw->common_cfg;
+
+	iowrite16(VIRTIO_MSI_NO_VECTOR, &cfg->msix_config);
+	for (i = 0; i < hw->nr_vring; i++) {
+		iowrite16(i, &cfg->queue_select);
+		iowrite16(0, &cfg->queue_enable);
+		iowrite16(VIRTIO_MSI_NO_VECTOR, &cfg->queue_msix_vector);
+	}
+}
+
+int ifcvf_start_hw(struct ifcvf_hw *hw)
+{
+	ifcvf_reset(hw);
+	ifcvf_add_status(hw, VIRTIO_CONFIG_S_ACKNOWLEDGE);
+	ifcvf_add_status(hw, VIRTIO_CONFIG_S_DRIVER);
+
+	if (ifcvf_config_features(hw) < 0)
+		return -1;
+
+	if (ifcvf_hw_enable(hw) < 0)
+		return -1;
+
+	ifcvf_add_status(hw, VIRTIO_CONFIG_S_DRIVER_OK);
+
+	return 0;
+}
+
+void ifcvf_stop_hw(struct ifcvf_hw *hw)
+{
+	ifcvf_hw_disable(hw);
+	ifcvf_reset(hw);
+}
+
+void ifcvf_enable_logging_vf(struct ifcvf_hw *hw, u64 log_base, u64 log_size)
+{
+	u8 *lm_cfg;
+
+	lm_cfg = hw->lm_cfg;
+
+	*(u32 *)(lm_cfg + IFCVF_LM_BASE_ADDR_LOW) =
+		log_base & IFCVF_32_BIT_MASK;
+
+	*(u32 *)(lm_cfg + IFCVF_LM_BASE_ADDR_HIGH) =
+		(log_base >> 32) & IFCVF_32_BIT_MASK;
+
+	*(u32 *)(lm_cfg + IFCVF_LM_END_ADDR_LOW) =
+		(log_base + log_size) & IFCVF_32_BIT_MASK;
+
+	*(u32 *)(lm_cfg + IFCVF_LM_END_ADDR_HIGH) =
+		((log_base + log_size) >> 32) & IFCVF_32_BIT_MASK;
+
+	*(u32 *)(lm_cfg + IFCVF_LM_LOGGING_CTRL) = IFCVF_LM_ENABLE_VF;
+}
+
+void ifcvf_disable_logging(struct ifcvf_hw *hw)
+{
+	u8 *lm_cfg;
+
+	lm_cfg = hw->lm_cfg;
+	*(u32 *)(lm_cfg + IFCVF_LM_LOGGING_CTRL) = IFCVF_LM_DISABLE;
+}
+
+void ifcvf_notify_queue(struct ifcvf_hw *hw, u16 qid)
+{
+
+	iowrite16(qid, hw->notify_addr[qid]);
+}
+
+u8 ifcvf_get_notify_region(struct ifcvf_hw *hw)
+{
+	return hw->notify_bar;
+}
+
+u64 ifcvf_get_queue_notify_off(struct ifcvf_hw *hw, int qid)
+{
+	return (u8 *)hw->notify_addr[qid] -
+		(u8 *)hw->mem_resource[hw->notify_bar].addr;
+}
diff --git a/drivers/vhost/ifcvf/ifcvf_base.h b/drivers/vhost/ifcvf/ifcvf_base.h
new file mode 100644
index 000000000000..1ab1a1c40f24
--- /dev/null
+++ b/drivers/vhost/ifcvf/ifcvf_base.h
@@ -0,0 +1,137 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Copyright (C) 2019 Intel Corporation.
+ */
+
+#ifndef _IFCVF_H_
+#define _IFCVF_H_
+
+#include <linux/virtio_mdev.h>
+#include <linux/pci.h>
+#include <linux/pci_regs.h>
+#include <uapi/linux/virtio_net.h>
+#include <uapi/linux/virtio_config.h>
+#include <uapi/linux/virtio_pci.h>
+
+#define IFCVF_VENDOR_ID         0x1AF4
+#define IFCVF_DEVICE_ID         0x1041
+#define IFCVF_SUBSYS_VENDOR_ID  0x8086
+#define IFCVF_SUBSYS_DEVICE_ID  0x001A
+
+/*
+ * Some ifcvf feature bits (currently bits 28 through 31) are
+ * reserved for the transport being used (eg. ifcvf_ring), the
+ * rest are per-device feature bits.
+ */
+#define IFCVF_TRANSPORT_F_START 28
+#define IFCVF_TRANSPORT_F_END   34
+
+#define IFC_SUPPORTED_FEATURES \
+		((1ULL << VIRTIO_NET_F_MAC)			| \
+		 (1ULL << VIRTIO_F_ANY_LAYOUT)			| \
+		 (1ULL << VIRTIO_F_VERSION_1)			| \
+		 (1ULL << VHOST_F_LOG_ALL)			| \
+		 (1ULL << VIRTIO_NET_F_GUEST_ANNOUNCE)		| \
+		 (1ULL << VIRTIO_NET_F_CTRL_VQ)			| \
+		 (1ULL << VIRTIO_NET_F_STATUS)			| \
+		 (1ULL << VIRTIO_NET_F_MRG_RXBUF)) /* not fully supported */
+
+#define IFCVF_MAX_QUEUE_PAIRS		1
+#define IFCVF_MAX_QUEUES		2
+
+#define IFCVF_QUEUE_ALIGNMENT		PAGE_SIZE
+
+#define IFCVF_MSI_CONFIG_OFF	0
+#define IFCVF_MSI_QUEUE_OFF	1
+#define IFCVF_PCI_MAX_RESOURCE	6
+
+/* 46 bit CPU physical address, avoid overlap */
+#define LM_IOVA 0x400000000000
+
+#define IFCVF_LM_CFG_SIZE		0x40
+#define IFCVF_LM_RING_STATE_OFFSET	0x20
+
+#define IFCVF_LM_LOGGING_CTRL		0x0
+
+#define IFCVF_LM_BASE_ADDR_LOW		0x10
+#define IFCVF_LM_BASE_ADDR_HIGH		0x14
+#define IFCVF_LM_END_ADDR_LOW		0x18
+#define IFCVF_LM_END_ADDR_HIGH		0x1c
+
+#define IFCVF_LM_DISABLE		0x0
+#define IFCVF_LM_ENABLE_VF		0x1
+#define IFCVF_LM_ENABLE_PF		0x3
+
+#define IFCVF_32_BIT_MASK		0xffffffff
+
+#define IFC_ERR(dev, fmt, ...)	dev_err(dev, fmt, ##__VA_ARGS__)
+#define IFC_INFO(dev, fmt, ...)	dev_info(dev, fmt, ##__VA_ARGS__)
+
+struct ifcvf_net_config {
+	u8    mac[6];
+	u16   status;
+	u16   max_virtqueue_pairs;
+} __packed;
+
+struct ifcvf_pci_mem_resource {
+	u64      phys_addr; /**< Physical address, 0 if not resource. */
+	u64      len;       /**< Length of the resource. */
+	u8       *addr;     /**< Virtual address, NULL when not mapped. */
+};
+
+struct vring_info {
+	u64 desc;
+	u64 avail;
+	u64 used;
+	u16 size;
+	u16 last_avail_idx;
+	u16 last_used_idx;
+	bool ready;
+	char msix_name[256];
+	struct virtio_mdev_callback cb;
+};
+
+struct ifcvf_hw {
+	u8	*isr;
+	u8	notify_bar;
+	u8	*lm_cfg;
+	u8	status;
+	u8	nr_vring;
+	u16	*notify_base;
+	u16	*notify_addr[IFCVF_MAX_QUEUE_PAIRS * 2];
+	u32	generation;
+	u32	notify_off_multiplier;
+	u64	req_features;
+	struct	virtio_pci_common_cfg *common_cfg;
+	struct	ifcvf_net_config *dev_cfg;
+	struct	vring_info vring[IFCVF_MAX_QUEUE_PAIRS * 2];
+	struct	ifcvf_pci_mem_resource mem_resource[IFCVF_PCI_MAX_RESOURCE];
+};
+
+#define IFC_PRIVATE_TO_VF(adapter) \
+	(&((struct ifcvf_adapter *)adapter)->vf)
+
+#define IFCVF_MAX_INTR (IFCVF_MAX_QUEUE_PAIRS * 2 + 1)
+
+struct ifcvf_adapter {
+	struct	device *dev;
+	struct	mutex mdev_lock;
+	int	mdev_count;
+	struct	list_head dma_maps;
+	int	vectors;
+	struct	ifcvf_hw vf;
+};
+
+int ifcvf_init_hw(struct ifcvf_hw *hw, struct pci_dev *dev);
+u64 ifcvf_get_features(struct ifcvf_hw *hw);
+int ifcvf_start_hw(struct ifcvf_hw *hw);
+void ifcvf_stop_hw(struct ifcvf_hw *hw);
+void ifcvf_enable_logging(struct ifcvf_hw *hw, u64 log_base, u64 log_size);
+void ifcvf_enable_logging_vf(struct ifcvf_hw *hw, u64 log_base, u64 log_size);
+void ifcvf_disable_logging(struct ifcvf_hw *hw);
+void ifcvf_notify_queue(struct ifcvf_hw *hw, u16 qid);
+void ifcvf_get_linkstatus(struct ifcvf_hw *hw, u8 *is_linkup);
+u8 ifcvf_get_notify_region(struct ifcvf_hw *hw);
+u64 ifcvf_get_queue_notify_off(struct ifcvf_hw *hw, int qid);
+
+#endif /* _IFCVF_H_ */
-- 
2.16.4

Re: [RFC 1/2] vhost: IFC VF hardware operation layer

[Stephen Hemminger]

On Wed, 16 Oct 2019 09:03:17 +0800
Zhu Lingshan <lingshan.zhu@intel.com> wrote:

> +int ifcvf_init_hw(struct ifcvf_hw *hw, struct pci_dev *dev)
> +{
> +	int ret;
> +	u8 pos;
> +	struct virtio_pci_cap cap;
> +	u32 i;
> +	u16 notify_off;

For network code, the preferred declaration style is
reverse christmas tree.

Re: [RFC 1/2] vhost: IFC VF hardware operation layer

[Stephen Hemminger]

On Wed, 16 Oct 2019 09:03:17 +0800
Zhu Lingshan <lingshan.zhu@intel.com> wrote:

> +	IFC_INFO(&dev->dev, "PCI capability mapping:\n"
> +				"common cfg: %p\n"
> +				"notify base: %p\n"
> +				"isr cfg: %p\n"
> +				"device cfg: %p\n"
> +				"multiplier: %u\n",
> +				hw->common_cfg,
> +				hw->notify_base,
> +				hw->isr,
> +				hw->dev_cfg,
> +				hw->notify_off_multiplier);

Since kernel messages go to syslog, syslog does not handle multi-line
messages very well. This should be a single line.

Also, this is the kind of message that should be at the debug
level; something that is useful to the driver developers
but not something that needs to be filling up every end users log.

Re: [RFC 1/2] vhost: IFC VF hardware operation layer

[Zhu, Lingshan]

On 10/16/2019 10:06 AM, Stephen Hemminger wrote:
> On Wed, 16 Oct 2019 09:03:17 +0800 Zhu Lingshan 
> <lingshan.zhu@intel.com> wrote:
>> + IFC_INFO(&dev->dev, "PCI capability mapping:\n" + "common cfg: 
>> %p\n" + "notify base: %p\n" + "isr cfg: %p\n" + "device cfg: %p\n" + 
>> "multiplier: %u\n", + hw->common_cfg, + hw->notify_base, + hw->isr, + 
>> hw->dev_cfg, + hw->notify_off_multiplier); 
> Since kernel messages go to syslog, syslog does not handle multi-line 
> messages very well. This should be a single line. Also, this is the 
> kind of message that should be at the debug level; something that is 
> useful to the driver developers but not something that needs to be 
> filling up every end users log.
Hi Stephen

Thanks for your comments, I have changed them in RFC V2, will send the 
patchset soon.
Thanks,
BR
Zhu Lingshan

[RFC 2/2] vhost: IFC VF vdpa layer

[Zhu Lingshan]

This commit introduced IFC VF operations for vdpa, which complys to
vhost_mdev interfaces, handles IFC VF initialization,
configuration and removal.

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 drivers/vhost/ifcvf/ifcvf_main.c | 541 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 541 insertions(+)
 create mode 100644 drivers/vhost/ifcvf/ifcvf_main.c

diff --git a/drivers/vhost/ifcvf/ifcvf_main.c b/drivers/vhost/ifcvf/ifcvf_main.c
new file mode 100644
index 000000000000..c48a29969a85
--- /dev/null
+++ b/drivers/vhost/ifcvf/ifcvf_main.c
@@ -0,0 +1,541 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2019 Intel Corporation.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/mdev.h>
+#include <linux/pci.h>
+#include <linux/sysfs.h>
+
+#include "ifcvf_base.h"
+
+#define VERSION_STRING	"0.1"
+#define DRIVER_AUTHOR	"Intel Corporation"
+#define IFCVF_DRIVER_NAME	"ifcvf"
+
+static irqreturn_t ifcvf_intr_handler(int irq, void *arg)
+{
+	struct vring_info *vring = arg;
+
+	if (vring->cb.callback)
+		return vring->cb.callback(vring->cb.private);
+
+	return IRQ_HANDLED;
+}
+
+static u64 ifcvf_mdev_get_features(struct mdev_device *mdev)
+{
+	return IFC_SUPPORTED_FEATURES;
+}
+
+static int ifcvf_mdev_set_features(struct mdev_device *mdev, u64 features)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	vf->req_features = features;
+
+	return 0;
+}
+
+static u64 ifcvf_mdev_get_vq_state(struct mdev_device *mdev, u16 qid)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	return vf->vring[qid].last_avail_idx;
+}
+
+static int ifcvf_mdev_set_vq_state(struct mdev_device *mdev, u16 qid, u64 num)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	vf->vring[qid].last_used_idx = num;
+	vf->vring[qid].last_avail_idx = num;
+
+	return 0;
+}
+
+static int ifcvf_mdev_set_vq_address(struct mdev_device *mdev, u16 idx,
+				     u64 desc_area, u64 driver_area,
+				     u64 device_area)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	vf->vring[idx].desc = desc_area;
+	vf->vring[idx].avail = driver_area;
+	vf->vring[idx].used = device_area;
+
+	return 0;
+}
+
+static void ifcvf_mdev_set_vq_num(struct mdev_device *mdev, u16 qid, u32 num)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	vf->vring[qid].size = num;
+}
+
+static void ifcvf_mdev_set_vq_ready(struct mdev_device *mdev,
+				u16 qid, bool ready)
+{
+
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	vf->vring[qid].ready = ready;
+}
+
+static bool ifcvf_mdev_get_vq_ready(struct mdev_device *mdev, u16 qid)
+{
+
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	return vf->vring[qid].ready;
+}
+
+static void ifcvf_mdev_set_vq_cb(struct mdev_device *mdev, u16 idx,
+				 struct virtio_mdev_callback *cb)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	vf->vring[idx].cb = *cb;
+}
+
+static void ifcvf_mdev_kick_vq(struct mdev_device *mdev, u16 idx)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	ifcvf_notify_queue(vf, idx);
+}
+
+static u8 ifcvf_mdev_get_status(struct mdev_device *mdev)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	return vf->status;
+}
+
+static u32 ifcvf_mdev_get_generation(struct mdev_device *mdev)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	return vf->generation;
+}
+
+static int ifcvf_mdev_get_version(struct mdev_device *mdev)
+{
+	return VIRTIO_MDEV_VERSION;
+}
+
+static u32 ifcvf_mdev_get_device_id(struct mdev_device *mdev)
+{
+	return IFCVF_DEVICE_ID;
+}
+
+static u32 ifcvf_mdev_get_vendor_id(struct mdev_device *mdev)
+{
+	return IFCVF_VENDOR_ID;
+}
+
+static u16 ifcvf_mdev_get_vq_align(struct mdev_device *mdev)
+{
+	return IFCVF_QUEUE_ALIGNMENT;
+}
+
+static int ifcvf_start_datapath(void *private)
+{
+	int i, ret;
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(private);
+
+	for (i = 0; i < (IFCVF_MAX_QUEUE_PAIRS * 2); i++) {
+		if (!vf->vring[i].ready)
+			break;
+
+		if (!vf->vring[i].size)
+			break;
+
+		if (!vf->vring[i].desc || !vf->vring[i].avail ||
+			!vf->vring[i].used)
+			break;
+	}
+	vf->nr_vring = i;
+
+	ret = ifcvf_start_hw(vf);
+	return ret;
+}
+
+static int ifcvf_stop_datapath(void *private)
+{
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(private);
+	int i;
+
+	for (i = 0; i < IFCVF_MAX_QUEUES; i++)
+		vf->vring[i].cb.callback = NULL;
+
+	ifcvf_stop_hw(vf);
+
+	return 0;
+}
+
+static void ifcvf_reset_vring(struct ifcvf_adapter *adapter)
+{
+	int i;
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
+		vf->vring[i].last_used_idx = 0;
+		vf->vring[i].last_avail_idx = 0;
+		vf->vring[i].desc = 0;
+		vf->vring[i].avail = 0;
+		vf->vring[i].used = 0;
+		vf->vring[i].ready = 0;
+		vf->vring->cb.callback = NULL;
+		vf->vring->cb.private = NULL;
+	}
+}
+
+static void ifcvf_mdev_set_status(struct mdev_device *mdev, u8 status)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	vf->status = status;
+
+	if (status == 0) {
+		ifcvf_stop_datapath(adapter);
+		ifcvf_reset_vring(adapter);
+		return;
+	}
+
+	if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
+		ifcvf_start_datapath(adapter);
+		return;
+	}
+}
+
+static u16 ifcvf_mdev_get_queue_max(struct mdev_device *mdev)
+{
+	return IFCVF_MAX_QUEUES;
+}
+
+static struct virtio_mdev_device_ops ifc_mdev_ops = {
+	.get_features  = ifcvf_mdev_get_features,
+	.set_features  = ifcvf_mdev_set_features,
+	.get_status    = ifcvf_mdev_get_status,
+	.set_status    = ifcvf_mdev_set_status,
+	.get_queue_max = ifcvf_mdev_get_queue_max,
+	.get_vq_state   = ifcvf_mdev_get_vq_state,
+	.set_vq_state   = ifcvf_mdev_set_vq_state,
+	.set_vq_cb      = ifcvf_mdev_set_vq_cb,
+	.set_vq_ready   = ifcvf_mdev_set_vq_ready,
+	.get_vq_ready	= ifcvf_mdev_get_vq_ready,
+	.set_vq_num     = ifcvf_mdev_set_vq_num,
+	.set_vq_address = ifcvf_mdev_set_vq_address,
+	.kick_vq        = ifcvf_mdev_kick_vq,
+	.get_generation	= ifcvf_mdev_get_generation,
+	.get_version	= ifcvf_mdev_get_version,
+	.get_device_id	= ifcvf_mdev_get_device_id,
+	.get_vendor_id	= ifcvf_mdev_get_vendor_id,
+	.get_vq_align	= ifcvf_mdev_get_vq_align,
+};
+
+static int ifcvf_init_msix(struct ifcvf_adapter *adapter)
+{
+	int vector, i, ret, irq;
+	struct pci_dev *pdev = to_pci_dev(adapter->dev);
+	struct ifcvf_hw *vf = &adapter->vf;
+
+	ret = pci_alloc_irq_vectors(pdev, IFCVF_MAX_INTR,
+			IFCVF_MAX_INTR, PCI_IRQ_MSIX);
+	if (ret < 0) {
+		IFC_ERR(adapter->dev, "Failed to alloc irq vectors.\n");
+		return ret;
+	}
+
+	for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
+		vector = i + IFCVF_MSI_QUEUE_OFF;
+		irq = pci_irq_vector(pdev, vector);
+		ret = request_irq(irq, ifcvf_intr_handler, 0,
+				pci_name(pdev), &vf->vring[i]);
+		if (ret) {
+			IFC_ERR(adapter->dev,
+				"Failed to request irq for vq %d.\n", i);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static void ifcvf_destroy_adapter(struct ifcvf_adapter *adapter)
+{
+	int i, vector, irq;
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+	struct pci_dev *pdev = to_pci_dev(adapter->dev);
+
+	for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
+		vector = i + IFCVF_MSI_QUEUE_OFF;
+		irq = pci_irq_vector(pdev, vector);
+		free_irq(irq, &vf->vring[i]);
+	}
+}
+
+static ssize_t name_show(struct kobject *kobj, struct device *dev, char *buf)
+{
+	const char *name = "vhost accelerator (virtio ring compatible)";
+
+	return sprintf(buf, "%s\n", name);
+}
+MDEV_TYPE_ATTR_RO(name);
+
+static ssize_t device_api_show(struct kobject *kobj, struct device *dev,
+			       char *buf)
+{
+	return sprintf(buf, "%s\n", VIRTIO_MDEV_DEVICE_API_STRING);
+}
+MDEV_TYPE_ATTR_RO(device_api);
+
+static ssize_t available_instances_show(struct kobject *kobj,
+					struct device *dev, char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
+
+	return sprintf(buf, "%d\n", adapter->mdev_count);
+}
+
+MDEV_TYPE_ATTR_RO(available_instances);
+
+static ssize_t type_show(struct kobject *kobj,
+			struct device *dev, char *buf)
+{
+	return sprintf(buf, "%s\n", "net");
+}
+
+MDEV_TYPE_ATTR_RO(type);
+
+
+static struct attribute *mdev_types_attrs[] = {
+	&mdev_type_attr_name.attr,
+	&mdev_type_attr_device_api.attr,
+	&mdev_type_attr_available_instances.attr,
+	&mdev_type_attr_type.attr,
+	NULL,
+};
+
+static struct attribute_group mdev_type_group = {
+	.name  = "vdpa_virtio",
+	.attrs = mdev_types_attrs,
+};
+
+static struct attribute_group *mdev_type_groups[] = {
+	&mdev_type_group,
+	NULL,
+};
+
+const struct attribute_group *mdev_dev_groups[] = {
+	NULL,
+};
+
+static int ifcvf_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
+{
+	struct device *dev = mdev_parent_dev(mdev);
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
+	int ret = 0;
+
+	mutex_lock(&adapter->mdev_lock);
+
+	if (adapter->mdev_count < 1) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	mdev_set_class_id(mdev, MDEV_ID_VHOST);
+	mdev_set_dev_ops(mdev, &ifc_mdev_ops);
+
+	mdev_set_drvdata(mdev, adapter);
+	mdev_set_iommu_device(mdev_dev(mdev), dev);
+
+	INIT_LIST_HEAD(&adapter->dma_maps);
+	adapter->mdev_count--;
+
+out:
+	mutex_unlock(&adapter->mdev_lock);
+	return ret;
+}
+
+static int ifcvf_mdev_remove(struct mdev_device *mdev)
+{
+	struct device *dev = mdev_parent_dev(mdev);
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
+
+	mutex_lock(&adapter->mdev_lock);
+	adapter->mdev_count++;
+	mutex_unlock(&adapter->mdev_lock);
+
+	return 0;
+}
+
+static struct mdev_parent_ops ifcvf_mdev_fops = {
+	.owner			= THIS_MODULE,
+	.supported_type_groups	= mdev_type_groups,
+	.mdev_attr_groups	= mdev_dev_groups,
+	.create			= ifcvf_mdev_create,
+	.remove			= ifcvf_mdev_remove,
+};
+
+static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+	struct device *dev = &pdev->dev;
+	struct ifcvf_adapter *adapter;
+	struct ifcvf_hw *vf;
+	int ret, i;
+
+	adapter = kzalloc(sizeof(struct ifcvf_adapter), GFP_KERNEL);
+	if (adapter == NULL) {
+		ret = -ENOMEM;
+		goto fail;
+	}
+
+	mutex_init(&adapter->mdev_lock);
+	adapter->mdev_count = 1;
+	adapter->dev = dev;
+
+	pci_set_drvdata(pdev, adapter);
+
+	ret = pci_enable_device(pdev);
+	if (ret) {
+		IFC_ERR(adapter->dev, "Failed to enable device.\n");
+		goto free_adapter;
+	}
+
+	ret = pci_request_regions(pdev, IFCVF_DRIVER_NAME);
+	if (ret) {
+		IFC_ERR(adapter->dev, "Failed to request MMIO region.\n");
+		goto disable_device;
+	}
+
+	pci_set_master(pdev);
+
+	ret = ifcvf_init_msix(adapter);
+	if (ret) {
+		IFC_ERR(adapter->dev, "Failed to initialize MSIX.\n");
+		goto free_msix;
+	}
+
+	vf = &adapter->vf;
+	for (i = 0; i < IFCVF_PCI_MAX_RESOURCE; i++) {
+		vf->mem_resource[i].phys_addr = pci_resource_start(pdev, i);
+		vf->mem_resource[i].len = pci_resource_len(pdev, i);
+		if (!vf->mem_resource[i].len) {
+			vf->mem_resource[i].addr = NULL;
+			continue;
+		}
+
+		vf->mem_resource[i].addr = pci_iomap_range(pdev, i, 0,
+				vf->mem_resource[i].len);
+		if (!vf->mem_resource[i].addr) {
+			IFC_ERR(adapter->dev, "Failed to map IO resource %d\n",
+				i);
+			return -1;
+		}
+	}
+
+	if (ifcvf_init_hw(vf, pdev) < 0)
+		return -1;
+
+	ret = mdev_register_device(dev, &ifcvf_mdev_fops);
+	if (ret) {
+		IFC_ERR(adapter->dev,  "Failed to register mdev device\n");
+		goto destroy_adapter;
+	}
+
+	return 0;
+
+destroy_adapter:
+	ifcvf_destroy_adapter(adapter);
+free_msix:
+	pci_free_irq_vectors(pdev);
+	pci_release_regions(pdev);
+disable_device:
+	pci_disable_device(pdev);
+free_adapter:
+	kfree(adapter);
+fail:
+	return ret;
+}
+
+static void ifcvf_remove(struct pci_dev *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
+	struct ifcvf_hw *vf;
+	int i;
+
+	mdev_unregister_device(dev);
+
+	vf = &adapter->vf;
+	for (i = 0; i < IFCVF_PCI_MAX_RESOURCE; i++) {
+		if (vf->mem_resource[i].addr) {
+			pci_iounmap(pdev, vf->mem_resource[i].addr);
+			vf->mem_resource[i].addr = NULL;
+		}
+	}
+
+	ifcvf_destroy_adapter(adapter);
+	pci_free_irq_vectors(pdev);
+
+	pci_release_regions(pdev);
+	pci_disable_device(pdev);
+
+	kfree(adapter);
+}
+
+static struct pci_device_id ifcvf_pci_ids[] = {
+	{ PCI_DEVICE_SUB(IFCVF_VENDOR_ID,
+			IFCVF_DEVICE_ID,
+			IFCVF_SUBSYS_VENDOR_ID,
+			IFCVF_SUBSYS_DEVICE_ID) },
+	{ 0 },
+};
+MODULE_DEVICE_TABLE(pci, ifcvf_pci_ids);
+
+static struct pci_driver ifcvf_driver = {
+	.name     = IFCVF_DRIVER_NAME,
+	.id_table = ifcvf_pci_ids,
+	.probe    = ifcvf_probe,
+	.remove   = ifcvf_remove,
+};
+
+static int __init ifcvf_init_module(void)
+{
+	int ret;
+
+	ret = pci_register_driver(&ifcvf_driver);
+	return ret;
+}
+
+static void __exit ifcvf_exit_module(void)
+{
+	pci_unregister_driver(&ifcvf_driver);
+}
+
+module_init(ifcvf_init_module);
+module_exit(ifcvf_exit_module);
+
+MODULE_LICENSE("GPL v2");
+MODULE_VERSION(VERSION_STRING);
+MODULE_AUTHOR(DRIVER_AUTHOR);
-- 
2.16.4

Re: [RFC 2/2] vhost: IFC VF vdpa layer

[Simon Horman]

Hi Zhu,

thanks for your patch.

On Wed, Oct 16, 2019 at 09:03:18AM +0800, Zhu Lingshan wrote:
> This commit introduced IFC VF operations for vdpa, which complys to
> vhost_mdev interfaces, handles IFC VF initialization,
> configuration and removal.
> 
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  drivers/vhost/ifcvf/ifcvf_main.c | 541 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 541 insertions(+)
>  create mode 100644 drivers/vhost/ifcvf/ifcvf_main.c
> 
> diff --git a/drivers/vhost/ifcvf/ifcvf_main.c b/drivers/vhost/ifcvf/ifcvf_main.c
> new file mode 100644
> index 000000000000..c48a29969a85
> --- /dev/null
> +++ b/drivers/vhost/ifcvf/ifcvf_main.c
> @@ -0,0 +1,541 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2019 Intel Corporation.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/mdev.h>
> +#include <linux/pci.h>
> +#include <linux/sysfs.h>
> +
> +#include "ifcvf_base.h"
> +
> +#define VERSION_STRING	"0.1"
> +#define DRIVER_AUTHOR	"Intel Corporation"
> +#define IFCVF_DRIVER_NAME	"ifcvf"
> +
> +static irqreturn_t ifcvf_intr_handler(int irq, void *arg)
> +{
> +	struct vring_info *vring = arg;
> +
> +	if (vring->cb.callback)
> +		return vring->cb.callback(vring->cb.private);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static u64 ifcvf_mdev_get_features(struct mdev_device *mdev)
> +{
> +	return IFC_SUPPORTED_FEATURES;
> +}
> +
> +static int ifcvf_mdev_set_features(struct mdev_device *mdev, u64 features)
> +{
> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);

Perhaps a helper that takes a struct mdev_device would be useful.
The pattern above seems to be repeated many times.

> +
> +	vf->req_features = features;
> +
> +	return 0;
> +}
> +
> +static u64 ifcvf_mdev_get_vq_state(struct mdev_device *mdev, u16 qid)
> +{
> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> +	return vf->vring[qid].last_avail_idx;
> +}
> +
> +static int ifcvf_mdev_set_vq_state(struct mdev_device *mdev, u16 qid, u64 num)
> +{
> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> +	vf->vring[qid].last_used_idx = num;
> +	vf->vring[qid].last_avail_idx = num;
> +
> +	return 0;
> +}
> +
> +static int ifcvf_mdev_set_vq_address(struct mdev_device *mdev, u16 idx,
> +				     u64 desc_area, u64 driver_area,
> +				     u64 device_area)
> +{
> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> +	vf->vring[idx].desc = desc_area;
> +	vf->vring[idx].avail = driver_area;
> +	vf->vring[idx].used = device_area;
> +
> +	return 0;
> +}
> +
> +static void ifcvf_mdev_set_vq_num(struct mdev_device *mdev, u16 qid, u32 num)
> +{
> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> +	vf->vring[qid].size = num;
> +}
> +
> +static void ifcvf_mdev_set_vq_ready(struct mdev_device *mdev,
> +				u16 qid, bool ready)

u16 should be vertically whitespace aligned with struct mdev_device.

> +{
> +
> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> +	vf->vring[qid].ready = ready;
> +}
> +
> +static bool ifcvf_mdev_get_vq_ready(struct mdev_device *mdev, u16 qid)
> +{
> +
> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> +	return vf->vring[qid].ready;
> +}
> +
> +static void ifcvf_mdev_set_vq_cb(struct mdev_device *mdev, u16 idx,
> +				 struct virtio_mdev_callback *cb)
> +{
> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> +	vf->vring[idx].cb = *cb;
> +}
> +
> +static void ifcvf_mdev_kick_vq(struct mdev_device *mdev, u16 idx)
> +{
> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> +	ifcvf_notify_queue(vf, idx);
> +}
> +
> +static u8 ifcvf_mdev_get_status(struct mdev_device *mdev)
> +{
> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> +	return vf->status;
> +}
> +
> +static u32 ifcvf_mdev_get_generation(struct mdev_device *mdev)
> +{
> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> +	return vf->generation;
> +}
> +
> +static int ifcvf_mdev_get_version(struct mdev_device *mdev)
> +{
> +	return VIRTIO_MDEV_VERSION;
> +}
> +
> +static u32 ifcvf_mdev_get_device_id(struct mdev_device *mdev)
> +{
> +	return IFCVF_DEVICE_ID;
> +}
> +
> +static u32 ifcvf_mdev_get_vendor_id(struct mdev_device *mdev)
> +{
> +	return IFCVF_VENDOR_ID;
> +}
> +
> +static u16 ifcvf_mdev_get_vq_align(struct mdev_device *mdev)
> +{
> +	return IFCVF_QUEUE_ALIGNMENT;
> +}
> +
> +static int ifcvf_start_datapath(void *private)
> +{
> +	int i, ret;
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(private);
> +
> +	for (i = 0; i < (IFCVF_MAX_QUEUE_PAIRS * 2); i++) {

The inner parentheses above seem unnecessary.

> +		if (!vf->vring[i].ready)
> +			break;
> +
> +		if (!vf->vring[i].size)
> +			break;
> +
> +		if (!vf->vring[i].desc || !vf->vring[i].avail ||
> +			!vf->vring[i].used)
> +			break;
> +	}
> +	vf->nr_vring = i;
> +
> +	ret = ifcvf_start_hw(vf);
> +	return ret;
> +}
> +
> +static int ifcvf_stop_datapath(void *private)
> +{
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(private);
> +	int i;
> +
> +	for (i = 0; i < IFCVF_MAX_QUEUES; i++)
> +		vf->vring[i].cb.callback = NULL;
> +
> +	ifcvf_stop_hw(vf);
> +
> +	return 0;
> +}
> +
> +static void ifcvf_reset_vring(struct ifcvf_adapter *adapter)
> +{
> +	int i;
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> +	for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
> +		vf->vring[i].last_used_idx = 0;
> +		vf->vring[i].last_avail_idx = 0;
> +		vf->vring[i].desc = 0;
> +		vf->vring[i].avail = 0;
> +		vf->vring[i].used = 0;
> +		vf->vring[i].ready = 0;
> +		vf->vring->cb.callback = NULL;
> +		vf->vring->cb.private = NULL;
> +	}
> +}
> +
> +static void ifcvf_mdev_set_status(struct mdev_device *mdev, u8 status)
> +{
> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> +	vf->status = status;
> +
> +	if (status == 0) {
> +		ifcvf_stop_datapath(adapter);
> +		ifcvf_reset_vring(adapter);
> +		return;
> +	}
> +
> +	if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
> +		ifcvf_start_datapath(adapter);
> +		return;
> +	}
> +}
> +
> +static u16 ifcvf_mdev_get_queue_max(struct mdev_device *mdev)
> +{
> +	return IFCVF_MAX_QUEUES;
> +}
> +
> +static struct virtio_mdev_device_ops ifc_mdev_ops = {
> +	.get_features  = ifcvf_mdev_get_features,
> +	.set_features  = ifcvf_mdev_set_features,
> +	.get_status    = ifcvf_mdev_get_status,
> +	.set_status    = ifcvf_mdev_set_status,
> +	.get_queue_max = ifcvf_mdev_get_queue_max,
> +	.get_vq_state   = ifcvf_mdev_get_vq_state,
> +	.set_vq_state   = ifcvf_mdev_set_vq_state,
> +	.set_vq_cb      = ifcvf_mdev_set_vq_cb,
> +	.set_vq_ready   = ifcvf_mdev_set_vq_ready,
> +	.get_vq_ready	= ifcvf_mdev_get_vq_ready,
> +	.set_vq_num     = ifcvf_mdev_set_vq_num,
> +	.set_vq_address = ifcvf_mdev_set_vq_address,
> +	.kick_vq        = ifcvf_mdev_kick_vq,
> +	.get_generation	= ifcvf_mdev_get_generation,
> +	.get_version	= ifcvf_mdev_get_version,
> +	.get_device_id	= ifcvf_mdev_get_device_id,
> +	.get_vendor_id	= ifcvf_mdev_get_vendor_id,
> +	.get_vq_align	= ifcvf_mdev_get_vq_align,
> +};
> +
> +static int ifcvf_init_msix(struct ifcvf_adapter *adapter)
> +{
> +	int vector, i, ret, irq;
> +	struct pci_dev *pdev = to_pci_dev(adapter->dev);
> +	struct ifcvf_hw *vf = &adapter->vf;
> +
> +	ret = pci_alloc_irq_vectors(pdev, IFCVF_MAX_INTR,
> +			IFCVF_MAX_INTR, PCI_IRQ_MSIX);
> +	if (ret < 0) {
> +		IFC_ERR(adapter->dev, "Failed to alloc irq vectors.\n");
> +		return ret;
> +	}
> +
> +	for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
> +		vector = i + IFCVF_MSI_QUEUE_OFF;
> +		irq = pci_irq_vector(pdev, vector);
> +		ret = request_irq(irq, ifcvf_intr_handler, 0,
> +				pci_name(pdev), &vf->vring[i]);
> +		if (ret) {
> +			IFC_ERR(adapter->dev,
> +				"Failed to request irq for vq %d.\n", i);
> +			return ret;

Is it worthwhile unwinding earlier successful calls to
request_irq() and pci_alloc_irq_vectors() here?
It seems that some resources may be leaked and
the system can otherwise continue.

> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static void ifcvf_destroy_adapter(struct ifcvf_adapter *adapter)
> +{
> +	int i, vector, irq;
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +	struct pci_dev *pdev = to_pci_dev(adapter->dev);
> +
> +	for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
> +		vector = i + IFCVF_MSI_QUEUE_OFF;
> +		irq = pci_irq_vector(pdev, vector);
> +		free_irq(irq, &vf->vring[i]);
> +	}
> +}
> +
> +static ssize_t name_show(struct kobject *kobj, struct device *dev, char *buf)
> +{
> +	const char *name = "vhost accelerator (virtio ring compatible)";
> +
> +	return sprintf(buf, "%s\n", name);
> +}
> +MDEV_TYPE_ATTR_RO(name);
> +
> +static ssize_t device_api_show(struct kobject *kobj, struct device *dev,
> +			       char *buf)
> +{
> +	return sprintf(buf, "%s\n", VIRTIO_MDEV_DEVICE_API_STRING);
> +}
> +MDEV_TYPE_ATTR_RO(device_api);
> +
> +static ssize_t available_instances_show(struct kobject *kobj,
> +					struct device *dev, char *buf)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
> +
> +	return sprintf(buf, "%d\n", adapter->mdev_count);
> +}
> +
> +MDEV_TYPE_ATTR_RO(available_instances);
> +
> +static ssize_t type_show(struct kobject *kobj,
> +			struct device *dev, char *buf)
> +{
> +	return sprintf(buf, "%s\n", "net");
> +}
> +
> +MDEV_TYPE_ATTR_RO(type);
> +
> +
> +static struct attribute *mdev_types_attrs[] = {
> +	&mdev_type_attr_name.attr,
> +	&mdev_type_attr_device_api.attr,
> +	&mdev_type_attr_available_instances.attr,
> +	&mdev_type_attr_type.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group mdev_type_group = {
> +	.name  = "vdpa_virtio",
> +	.attrs = mdev_types_attrs,
> +};
> +
> +static struct attribute_group *mdev_type_groups[] = {
> +	&mdev_type_group,
> +	NULL,
> +};
> +
> +const struct attribute_group *mdev_dev_groups[] = {
> +	NULL,
> +};
> +
> +static int ifcvf_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
> +{
> +	struct device *dev = mdev_parent_dev(mdev);
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
> +	int ret = 0;
> +
> +	mutex_lock(&adapter->mdev_lock);
> +
> +	if (adapter->mdev_count < 1) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	mdev_set_class_id(mdev, MDEV_ID_VHOST);
> +	mdev_set_dev_ops(mdev, &ifc_mdev_ops);
> +
> +	mdev_set_drvdata(mdev, adapter);
> +	mdev_set_iommu_device(mdev_dev(mdev), dev);
> +
> +	INIT_LIST_HEAD(&adapter->dma_maps);

dma_maps appears to be initialised but otherwise unused.

If it is needed would it make more sense to initialise it in the probe
function? That seems to be where adapter is initialised.

> +	adapter->mdev_count--;
> +
> +out:
> +	mutex_unlock(&adapter->mdev_lock);
> +	return ret;
> +}
> +
> +static int ifcvf_mdev_remove(struct mdev_device *mdev)
> +{
> +	struct device *dev = mdev_parent_dev(mdev);
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
> +
> +	mutex_lock(&adapter->mdev_lock);
> +	adapter->mdev_count++;
> +	mutex_unlock(&adapter->mdev_lock);
> +
> +	return 0;
> +}
> +
> +static struct mdev_parent_ops ifcvf_mdev_fops = {
> +	.owner			= THIS_MODULE,
> +	.supported_type_groups	= mdev_type_groups,
> +	.mdev_attr_groups	= mdev_dev_groups,
> +	.create			= ifcvf_mdev_create,
> +	.remove			= ifcvf_mdev_remove,
> +};
> +
> +static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ifcvf_adapter *adapter;
> +	struct ifcvf_hw *vf;
> +	int ret, i;
> +
> +	adapter = kzalloc(sizeof(struct ifcvf_adapter), GFP_KERNEL);
> +	if (adapter == NULL) {
> +		ret = -ENOMEM;
> +		goto fail;

I think it would be cleaner to just return here.

> +	}
> +
> +	mutex_init(&adapter->mdev_lock);
> +	adapter->mdev_count = 1;
> +	adapter->dev = dev;
> +
> +	pci_set_drvdata(pdev, adapter);
> +
> +	ret = pci_enable_device(pdev);
> +	if (ret) {
> +		IFC_ERR(adapter->dev, "Failed to enable device.\n");
> +		goto free_adapter;
> +	}
> +
> +	ret = pci_request_regions(pdev, IFCVF_DRIVER_NAME);
> +	if (ret) {
> +		IFC_ERR(adapter->dev, "Failed to request MMIO region.\n");
> +		goto disable_device;
> +	}
> +
> +	pci_set_master(pdev);
> +
> +	ret = ifcvf_init_msix(adapter);
> +	if (ret) {
> +		IFC_ERR(adapter->dev, "Failed to initialize MSIX.\n");
> +		goto free_msix;
> +	}
> +
> +	vf = &adapter->vf;
> +	for (i = 0; i < IFCVF_PCI_MAX_RESOURCE; i++) {
> +		vf->mem_resource[i].phys_addr = pci_resource_start(pdev, i);
> +		vf->mem_resource[i].len = pci_resource_len(pdev, i);
> +		if (!vf->mem_resource[i].len) {
> +			vf->mem_resource[i].addr = NULL;
> +			continue;
> +		}
> +
> +		vf->mem_resource[i].addr = pci_iomap_range(pdev, i, 0,
> +				vf->mem_resource[i].len);
> +		if (!vf->mem_resource[i].addr) {
> +			IFC_ERR(adapter->dev, "Failed to map IO resource %d\n",
> +				i);

There is cleanup code in this function. But in this case, and one more
below, the function simply returns on error. At the very least this seems
inconsistent.

> +			return -1;
> +		}
> +	}
> +
> +	if (ifcvf_init_hw(vf, pdev) < 0)
> +		return -1;
> +
> +	ret = mdev_register_device(dev, &ifcvf_mdev_fops);
> +	if (ret) {
> +		IFC_ERR(adapter->dev,  "Failed to register mdev device\n");
> +		goto destroy_adapter;
> +	}
> +
> +	return 0;
> +
> +destroy_adapter:
> +	ifcvf_destroy_adapter(adapter);
> +free_msix:
> +	pci_free_irq_vectors(pdev);
> +	pci_release_regions(pdev);
> +disable_device:
> +	pci_disable_device(pdev);
> +free_adapter:
> +	kfree(adapter);
> +fail:
> +	return ret;
> +}
> +
> +static void ifcvf_remove(struct pci_dev *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
> +	struct ifcvf_hw *vf;
> +	int i;
> +
> +	mdev_unregister_device(dev);
> +
> +	vf = &adapter->vf;
> +	for (i = 0; i < IFCVF_PCI_MAX_RESOURCE; i++) {
> +		if (vf->mem_resource[i].addr) {
> +			pci_iounmap(pdev, vf->mem_resource[i].addr);
> +			vf->mem_resource[i].addr = NULL;
> +		}
> +	}
> +
> +	ifcvf_destroy_adapter(adapter);
> +	pci_free_irq_vectors(pdev);
> +
> +	pci_release_regions(pdev);
> +	pci_disable_device(pdev);
> +
> +	kfree(adapter);
> +}
> +
> +static struct pci_device_id ifcvf_pci_ids[] = {
> +	{ PCI_DEVICE_SUB(IFCVF_VENDOR_ID,
> +			IFCVF_DEVICE_ID,
> +			IFCVF_SUBSYS_VENDOR_ID,
> +			IFCVF_SUBSYS_DEVICE_ID) },
> +	{ 0 },
> +};
> +MODULE_DEVICE_TABLE(pci, ifcvf_pci_ids);
> +
> +static struct pci_driver ifcvf_driver = {
> +	.name     = IFCVF_DRIVER_NAME,
> +	.id_table = ifcvf_pci_ids,
> +	.probe    = ifcvf_probe,
> +	.remove   = ifcvf_remove,
> +};
> +
> +static int __init ifcvf_init_module(void)
> +{
> +	int ret;
> +
> +	ret = pci_register_driver(&ifcvf_driver);
> +	return ret;
> +}
> +
> +static void __exit ifcvf_exit_module(void)
> +{
> +	pci_unregister_driver(&ifcvf_driver);
> +}
> +
> +module_init(ifcvf_init_module);
> +module_exit(ifcvf_exit_module);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_VERSION(VERSION_STRING);
> +MODULE_AUTHOR(DRIVER_AUTHOR);
> -- 
> 2.16.4
> 

Re: [RFC 2/2] vhost: IFC VF vdpa layer

[Zhu, Lingshan]

On 10/16/2019 5:53 PM, Simon Horman wrote:
> Hi Zhu,
>
> thanks for your patch.
>
> On Wed, Oct 16, 2019 at 09:03:18AM +0800, Zhu Lingshan wrote:
>> This commit introduced IFC VF operations for vdpa, which complys to
>> vhost_mdev interfaces, handles IFC VF initialization,
>> configuration and removal.
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>>   drivers/vhost/ifcvf/ifcvf_main.c | 541 +++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 541 insertions(+)
>>   create mode 100644 drivers/vhost/ifcvf/ifcvf_main.c
>>
>> diff --git a/drivers/vhost/ifcvf/ifcvf_main.c b/drivers/vhost/ifcvf/ifcvf_main.c
>> new file mode 100644
>> index 000000000000..c48a29969a85
>> --- /dev/null
>> +++ b/drivers/vhost/ifcvf/ifcvf_main.c
>> @@ -0,0 +1,541 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2019 Intel Corporation.
>> + */
>> +
>> +#include <linux/interrupt.h>
>> +#include <linux/module.h>
>> +#include <linux/mdev.h>
>> +#include <linux/pci.h>
>> +#include <linux/sysfs.h>
>> +
>> +#include "ifcvf_base.h"
>> +
>> +#define VERSION_STRING	"0.1"
>> +#define DRIVER_AUTHOR	"Intel Corporation"
>> +#define IFCVF_DRIVER_NAME	"ifcvf"
>> +
>> +static irqreturn_t ifcvf_intr_handler(int irq, void *arg)
>> +{
>> +	struct vring_info *vring = arg;
>> +
>> +	if (vring->cb.callback)
>> +		return vring->cb.callback(vring->cb.private);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static u64 ifcvf_mdev_get_features(struct mdev_device *mdev)
>> +{
>> +	return IFC_SUPPORTED_FEATURES;
>> +}
>> +
>> +static int ifcvf_mdev_set_features(struct mdev_device *mdev, u64 features)
>> +{
>> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> Perhaps a helper that takes a struct mdev_device would be useful.
> The pattern above seems to be repeated many times.

Hi Simon, thanks a lot for your comments.

sure, can do

>
>> +
>> +	vf->req_features = features;
>> +
>> +	return 0;
>> +}
>> +
>> +static u64 ifcvf_mdev_get_vq_state(struct mdev_device *mdev, u16 qid)
>> +{
>> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +	return vf->vring[qid].last_avail_idx;
>> +}
>> +
>> +static int ifcvf_mdev_set_vq_state(struct mdev_device *mdev, u16 qid, u64 num)
>> +{
>> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +	vf->vring[qid].last_used_idx = num;
>> +	vf->vring[qid].last_avail_idx = num;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ifcvf_mdev_set_vq_address(struct mdev_device *mdev, u16 idx,
>> +				     u64 desc_area, u64 driver_area,
>> +				     u64 device_area)
>> +{
>> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +	vf->vring[idx].desc = desc_area;
>> +	vf->vring[idx].avail = driver_area;
>> +	vf->vring[idx].used = device_area;
>> +
>> +	return 0;
>> +}
>> +
>> +static void ifcvf_mdev_set_vq_num(struct mdev_device *mdev, u16 qid, u32 num)
>> +{
>> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +	vf->vring[qid].size = num;
>> +}
>> +
>> +static void ifcvf_mdev_set_vq_ready(struct mdev_device *mdev,
>> +				u16 qid, bool ready)
> u16 should be vertically whitespace aligned with struct mdev_device.
>
>> +{
>> +
>> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +	vf->vring[qid].ready = ready;
>> +}
>> +
>> +static bool ifcvf_mdev_get_vq_ready(struct mdev_device *mdev, u16 qid)
>> +{
>> +
>> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +	return vf->vring[qid].ready;
>> +}
>> +
>> +static void ifcvf_mdev_set_vq_cb(struct mdev_device *mdev, u16 idx,
>> +				 struct virtio_mdev_callback *cb)
>> +{
>> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +	vf->vring[idx].cb = *cb;
>> +}
>> +
>> +static void ifcvf_mdev_kick_vq(struct mdev_device *mdev, u16 idx)
>> +{
>> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +	ifcvf_notify_queue(vf, idx);
>> +}
>> +
>> +static u8 ifcvf_mdev_get_status(struct mdev_device *mdev)
>> +{
>> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +	return vf->status;
>> +}
>> +
>> +static u32 ifcvf_mdev_get_generation(struct mdev_device *mdev)
>> +{
>> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +	return vf->generation;
>> +}
>> +
>> +static int ifcvf_mdev_get_version(struct mdev_device *mdev)
>> +{
>> +	return VIRTIO_MDEV_VERSION;
>> +}
>> +
>> +static u32 ifcvf_mdev_get_device_id(struct mdev_device *mdev)
>> +{
>> +	return IFCVF_DEVICE_ID;
>> +}
>> +
>> +static u32 ifcvf_mdev_get_vendor_id(struct mdev_device *mdev)
>> +{
>> +	return IFCVF_VENDOR_ID;
>> +}
>> +
>> +static u16 ifcvf_mdev_get_vq_align(struct mdev_device *mdev)
>> +{
>> +	return IFCVF_QUEUE_ALIGNMENT;
>> +}
>> +
>> +static int ifcvf_start_datapath(void *private)
>> +{
>> +	int i, ret;
>> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(private);
>> +
>> +	for (i = 0; i < (IFCVF_MAX_QUEUE_PAIRS * 2); i++) {
> The inner parentheses above seem unnecessary.
>
>> +		if (!vf->vring[i].ready)
>> +			break;
>> +
>> +		if (!vf->vring[i].size)
>> +			break;
>> +
>> +		if (!vf->vring[i].desc || !vf->vring[i].avail ||
>> +			!vf->vring[i].used)
>> +			break;
>> +	}
>> +	vf->nr_vring = i;
>> +
>> +	ret = ifcvf_start_hw(vf);
>> +	return ret;
>> +}
>> +
>> +static int ifcvf_stop_datapath(void *private)
>> +{
>> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(private);
>> +	int i;
>> +
>> +	for (i = 0; i < IFCVF_MAX_QUEUES; i++)
>> +		vf->vring[i].cb.callback = NULL;
>> +
>> +	ifcvf_stop_hw(vf);
>> +
>> +	return 0;
>> +}
>> +
>> +static void ifcvf_reset_vring(struct ifcvf_adapter *adapter)
>> +{
>> +	int i;
>> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +	for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
>> +		vf->vring[i].last_used_idx = 0;
>> +		vf->vring[i].last_avail_idx = 0;
>> +		vf->vring[i].desc = 0;
>> +		vf->vring[i].avail = 0;
>> +		vf->vring[i].used = 0;
>> +		vf->vring[i].ready = 0;
>> +		vf->vring->cb.callback = NULL;
>> +		vf->vring->cb.private = NULL;
>> +	}
>> +}
>> +
>> +static void ifcvf_mdev_set_status(struct mdev_device *mdev, u8 status)
>> +{
>> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +	vf->status = status;
>> +
>> +	if (status == 0) {
>> +		ifcvf_stop_datapath(adapter);
>> +		ifcvf_reset_vring(adapter);
>> +		return;
>> +	}
>> +
>> +	if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
>> +		ifcvf_start_datapath(adapter);
>> +		return;
>> +	}
>> +}
>> +
>> +static u16 ifcvf_mdev_get_queue_max(struct mdev_device *mdev)
>> +{
>> +	return IFCVF_MAX_QUEUES;
>> +}
>> +
>> +static struct virtio_mdev_device_ops ifc_mdev_ops = {
>> +	.get_features  = ifcvf_mdev_get_features,
>> +	.set_features  = ifcvf_mdev_set_features,
>> +	.get_status    = ifcvf_mdev_get_status,
>> +	.set_status    = ifcvf_mdev_set_status,
>> +	.get_queue_max = ifcvf_mdev_get_queue_max,
>> +	.get_vq_state   = ifcvf_mdev_get_vq_state,
>> +	.set_vq_state   = ifcvf_mdev_set_vq_state,
>> +	.set_vq_cb      = ifcvf_mdev_set_vq_cb,
>> +	.set_vq_ready   = ifcvf_mdev_set_vq_ready,
>> +	.get_vq_ready	= ifcvf_mdev_get_vq_ready,
>> +	.set_vq_num     = ifcvf_mdev_set_vq_num,
>> +	.set_vq_address = ifcvf_mdev_set_vq_address,
>> +	.kick_vq        = ifcvf_mdev_kick_vq,
>> +	.get_generation	= ifcvf_mdev_get_generation,
>> +	.get_version	= ifcvf_mdev_get_version,
>> +	.get_device_id	= ifcvf_mdev_get_device_id,
>> +	.get_vendor_id	= ifcvf_mdev_get_vendor_id,
>> +	.get_vq_align	= ifcvf_mdev_get_vq_align,
>> +};
>> +
>> +static int ifcvf_init_msix(struct ifcvf_adapter *adapter)
>> +{
>> +	int vector, i, ret, irq;
>> +	struct pci_dev *pdev = to_pci_dev(adapter->dev);
>> +	struct ifcvf_hw *vf = &adapter->vf;
>> +
>> +	ret = pci_alloc_irq_vectors(pdev, IFCVF_MAX_INTR,
>> +			IFCVF_MAX_INTR, PCI_IRQ_MSIX);
>> +	if (ret < 0) {
>> +		IFC_ERR(adapter->dev, "Failed to alloc irq vectors.\n");
>> +		return ret;
>> +	}
>> +
>> +	for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
>> +		vector = i + IFCVF_MSI_QUEUE_OFF;
>> +		irq = pci_irq_vector(pdev, vector);
>> +		ret = request_irq(irq, ifcvf_intr_handler, 0,
>> +				pci_name(pdev), &vf->vring[i]);
>> +		if (ret) {
>> +			IFC_ERR(adapter->dev,
>> +				"Failed to request irq for vq %d.\n", i);
>> +			return ret;
> Is it worthwhile unwinding earlier successful calls to
> request_irq() and pci_alloc_irq_vectors() here?
> It seems that some resources may be leaked and
> the system can otherwise continue.
there are error handlers in the probe function, it will release 
resources if failed.
>
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void ifcvf_destroy_adapter(struct ifcvf_adapter *adapter)
>> +{
>> +	int i, vector, irq;
>> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +	struct pci_dev *pdev = to_pci_dev(adapter->dev);
>> +
>> +	for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
>> +		vector = i + IFCVF_MSI_QUEUE_OFF;
>> +		irq = pci_irq_vector(pdev, vector);
>> +		free_irq(irq, &vf->vring[i]);
>> +	}
>> +}
>> +
>> +static ssize_t name_show(struct kobject *kobj, struct device *dev, char *buf)
>> +{
>> +	const char *name = "vhost accelerator (virtio ring compatible)";
>> +
>> +	return sprintf(buf, "%s\n", name);
>> +}
>> +MDEV_TYPE_ATTR_RO(name);
>> +
>> +static ssize_t device_api_show(struct kobject *kobj, struct device *dev,
>> +			       char *buf)
>> +{
>> +	return sprintf(buf, "%s\n", VIRTIO_MDEV_DEVICE_API_STRING);
>> +}
>> +MDEV_TYPE_ATTR_RO(device_api);
>> +
>> +static ssize_t available_instances_show(struct kobject *kobj,
>> +					struct device *dev, char *buf)
>> +{
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +	struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
>> +
>> +	return sprintf(buf, "%d\n", adapter->mdev_count);
>> +}
>> +
>> +MDEV_TYPE_ATTR_RO(available_instances);
>> +
>> +static ssize_t type_show(struct kobject *kobj,
>> +			struct device *dev, char *buf)
>> +{
>> +	return sprintf(buf, "%s\n", "net");
>> +}
>> +
>> +MDEV_TYPE_ATTR_RO(type);
>> +
>> +
>> +static struct attribute *mdev_types_attrs[] = {
>> +	&mdev_type_attr_name.attr,
>> +	&mdev_type_attr_device_api.attr,
>> +	&mdev_type_attr_available_instances.attr,
>> +	&mdev_type_attr_type.attr,
>> +	NULL,
>> +};
>> +
>> +static struct attribute_group mdev_type_group = {
>> +	.name  = "vdpa_virtio",
>> +	.attrs = mdev_types_attrs,
>> +};
>> +
>> +static struct attribute_group *mdev_type_groups[] = {
>> +	&mdev_type_group,
>> +	NULL,
>> +};
>> +
>> +const struct attribute_group *mdev_dev_groups[] = {
>> +	NULL,
>> +};
>> +
>> +static int ifcvf_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
>> +{
>> +	struct device *dev = mdev_parent_dev(mdev);
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +	struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
>> +	int ret = 0;
>> +
>> +	mutex_lock(&adapter->mdev_lock);
>> +
>> +	if (adapter->mdev_count < 1) {
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
>> +
>> +	mdev_set_class_id(mdev, MDEV_ID_VHOST);
>> +	mdev_set_dev_ops(mdev, &ifc_mdev_ops);
>> +
>> +	mdev_set_drvdata(mdev, adapter);
>> +	mdev_set_iommu_device(mdev_dev(mdev), dev);
>> +
>> +	INIT_LIST_HEAD(&adapter->dma_maps);
> dma_maps appears to be initialised but otherwise unused.
>
> If it is needed would it make more sense to initialise it in the probe
> function? That seems to be where adapter is initialised.
I will re-format the patches, thx!
>
>> +	adapter->mdev_count--;
>> +
>> +out:
>> +	mutex_unlock(&adapter->mdev_lock);
>> +	return ret;
>> +}
>> +
>> +static int ifcvf_mdev_remove(struct mdev_device *mdev)
>> +{
>> +	struct device *dev = mdev_parent_dev(mdev);
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +	struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
>> +
>> +	mutex_lock(&adapter->mdev_lock);
>> +	adapter->mdev_count++;
>> +	mutex_unlock(&adapter->mdev_lock);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct mdev_parent_ops ifcvf_mdev_fops = {
>> +	.owner			= THIS_MODULE,
>> +	.supported_type_groups	= mdev_type_groups,
>> +	.mdev_attr_groups	= mdev_dev_groups,
>> +	.create			= ifcvf_mdev_create,
>> +	.remove			= ifcvf_mdev_remove,
>> +};
>> +
>> +static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct ifcvf_adapter *adapter;
>> +	struct ifcvf_hw *vf;
>> +	int ret, i;
>> +
>> +	adapter = kzalloc(sizeof(struct ifcvf_adapter), GFP_KERNEL);
>> +	if (adapter == NULL) {
>> +		ret = -ENOMEM;
>> +		goto fail;
> I think it would be cleaner to just return here.
agreed!
>
>> +	}
>> +
>> +	mutex_init(&adapter->mdev_lock);
>> +	adapter->mdev_count = 1;
>> +	adapter->dev = dev;
>> +
>> +	pci_set_drvdata(pdev, adapter);
>> +
>> +	ret = pci_enable_device(pdev);
>> +	if (ret) {
>> +		IFC_ERR(adapter->dev, "Failed to enable device.\n");
>> +		goto free_adapter;
>> +	}
>> +
>> +	ret = pci_request_regions(pdev, IFCVF_DRIVER_NAME);
>> +	if (ret) {
>> +		IFC_ERR(adapter->dev, "Failed to request MMIO region.\n");
>> +		goto disable_device;
>> +	}
>> +
>> +	pci_set_master(pdev);
>> +
>> +	ret = ifcvf_init_msix(adapter);
>> +	if (ret) {
>> +		IFC_ERR(adapter->dev, "Failed to initialize MSIX.\n");
>> +		goto free_msix;
>> +	}
>> +
>> +	vf = &adapter->vf;
>> +	for (i = 0; i < IFCVF_PCI_MAX_RESOURCE; i++) {
>> +		vf->mem_resource[i].phys_addr = pci_resource_start(pdev, i);
>> +		vf->mem_resource[i].len = pci_resource_len(pdev, i);
>> +		if (!vf->mem_resource[i].len) {
>> +			vf->mem_resource[i].addr = NULL;
>> +			continue;
>> +		}
>> +
>> +		vf->mem_resource[i].addr = pci_iomap_range(pdev, i, 0,
>> +				vf->mem_resource[i].len);
>> +		if (!vf->mem_resource[i].addr) {
>> +			IFC_ERR(adapter->dev, "Failed to map IO resource %d\n",
>> +				i);
> There is cleanup code in this function. But in this case, and one more
> below, the function simply returns on error. At the very least this seems
> inconsistent.
I will check, thx
>
>> +			return -1;
>> +		}
>> +	}
>> +
>> +	if (ifcvf_init_hw(vf, pdev) < 0)
>> +		return -1;
>> +
>> +	ret = mdev_register_device(dev, &ifcvf_mdev_fops);
>> +	if (ret) {
>> +		IFC_ERR(adapter->dev,  "Failed to register mdev device\n");
>> +		goto destroy_adapter;
>> +	}
>> +
>> +	return 0;
>> +
>> +destroy_adapter:
>> +	ifcvf_destroy_adapter(adapter);
>> +free_msix:
>> +	pci_free_irq_vectors(pdev);
>> +	pci_release_regions(pdev);
>> +disable_device:
>> +	pci_disable_device(pdev);
>> +free_adapter:
>> +	kfree(adapter);
>> +fail:
>> +	return ret;
>> +}
>> +
>> +static void ifcvf_remove(struct pci_dev *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
>> +	struct ifcvf_hw *vf;
>> +	int i;
>> +
>> +	mdev_unregister_device(dev);
>> +
>> +	vf = &adapter->vf;
>> +	for (i = 0; i < IFCVF_PCI_MAX_RESOURCE; i++) {
>> +		if (vf->mem_resource[i].addr) {
>> +			pci_iounmap(pdev, vf->mem_resource[i].addr);
>> +			vf->mem_resource[i].addr = NULL;
>> +		}
>> +	}
>> +
>> +	ifcvf_destroy_adapter(adapter);
>> +	pci_free_irq_vectors(pdev);
>> +
>> +	pci_release_regions(pdev);
>> +	pci_disable_device(pdev);
>> +
>> +	kfree(adapter);
>> +}
>> +
>> +static struct pci_device_id ifcvf_pci_ids[] = {
>> +	{ PCI_DEVICE_SUB(IFCVF_VENDOR_ID,
>> +			IFCVF_DEVICE_ID,
>> +			IFCVF_SUBSYS_VENDOR_ID,
>> +			IFCVF_SUBSYS_DEVICE_ID) },
>> +	{ 0 },
>> +};
>> +MODULE_DEVICE_TABLE(pci, ifcvf_pci_ids);
>> +
>> +static struct pci_driver ifcvf_driver = {
>> +	.name     = IFCVF_DRIVER_NAME,
>> +	.id_table = ifcvf_pci_ids,
>> +	.probe    = ifcvf_probe,
>> +	.remove   = ifcvf_remove,
>> +};
>> +
>> +static int __init ifcvf_init_module(void)
>> +{
>> +	int ret;
>> +
>> +	ret = pci_register_driver(&ifcvf_driver);
>> +	return ret;
>> +}
>> +
>> +static void __exit ifcvf_exit_module(void)
>> +{
>> +	pci_unregister_driver(&ifcvf_driver);
>> +}
>> +
>> +module_init(ifcvf_init_module);
>> +module_exit(ifcvf_exit_module);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_VERSION(VERSION_STRING);
>> +MODULE_AUTHOR(DRIVER_AUTHOR);
>> -- 
>> 2.16.4
>>

[RFC 0/2] Intel IFC VF driver for vdpa

[Zhu Lingshan]

Hi all:
 
This series intends to introduce Intel IFC VF NIC driver for Vhost
Data Plane Acceleration.
 
Here comes two main parts, one is ifcvf_base layer, which handles
hardware operations. The other is ifcvf_main layer handles VF
initialization, configuration and removal, which depends on
and complys to vhost_mdev https://lkml.org/lkml/2019/9/26/15 
 
This is a first RFC try, please help review.
 
Thanks!
BR
Zhu Lingshan


Zhu Lingshan (2):
  vhost: IFC VF hardware operation layer
  vhost: IFC VF vdpa layer

 drivers/vhost/ifcvf/ifcvf_base.c | 390 ++++++++++++++++++++++++++++
 drivers/vhost/ifcvf/ifcvf_base.h | 137 ++++++++++
 drivers/vhost/ifcvf/ifcvf_main.c | 541 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 1068 insertions(+)
 create mode 100644 drivers/vhost/ifcvf/ifcvf_base.c
 create mode 100644 drivers/vhost/ifcvf/ifcvf_base.h
 create mode 100644 drivers/vhost/ifcvf/ifcvf_main.c

-- 
2.16.4

[RFC 2/2] vhost: IFC VF vdpa layer

[Zhu Lingshan]

This commit introduced IFC VF operations for vdpa, which complys to
vhost_mdev interfaces, handles IFC VF initialization,
configuration and removal.

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 drivers/vhost/ifcvf/ifcvf_main.c | 541 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 541 insertions(+)
 create mode 100644 drivers/vhost/ifcvf/ifcvf_main.c

diff --git a/drivers/vhost/ifcvf/ifcvf_main.c b/drivers/vhost/ifcvf/ifcvf_main.c
new file mode 100644
index 000000000000..c48a29969a85
--- /dev/null
+++ b/drivers/vhost/ifcvf/ifcvf_main.c
@@ -0,0 +1,541 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2019 Intel Corporation.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/mdev.h>
+#include <linux/pci.h>
+#include <linux/sysfs.h>
+
+#include "ifcvf_base.h"
+
+#define VERSION_STRING	"0.1"
+#define DRIVER_AUTHOR	"Intel Corporation"
+#define IFCVF_DRIVER_NAME	"ifcvf"
+
+static irqreturn_t ifcvf_intr_handler(int irq, void *arg)
+{
+	struct vring_info *vring = arg;
+
+	if (vring->cb.callback)
+		return vring->cb.callback(vring->cb.private);
+
+	return IRQ_HANDLED;
+}
+
+static u64 ifcvf_mdev_get_features(struct mdev_device *mdev)
+{
+	return IFC_SUPPORTED_FEATURES;
+}
+
+static int ifcvf_mdev_set_features(struct mdev_device *mdev, u64 features)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	vf->req_features = features;
+
+	return 0;
+}
+
+static u64 ifcvf_mdev_get_vq_state(struct mdev_device *mdev, u16 qid)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	return vf->vring[qid].last_avail_idx;
+}
+
+static int ifcvf_mdev_set_vq_state(struct mdev_device *mdev, u16 qid, u64 num)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	vf->vring[qid].last_used_idx = num;
+	vf->vring[qid].last_avail_idx = num;
+
+	return 0;
+}
+
+static int ifcvf_mdev_set_vq_address(struct mdev_device *mdev, u16 idx,
+				     u64 desc_area, u64 driver_area,
+				     u64 device_area)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	vf->vring[idx].desc = desc_area;
+	vf->vring[idx].avail = driver_area;
+	vf->vring[idx].used = device_area;
+
+	return 0;
+}
+
+static void ifcvf_mdev_set_vq_num(struct mdev_device *mdev, u16 qid, u32 num)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	vf->vring[qid].size = num;
+}
+
+static void ifcvf_mdev_set_vq_ready(struct mdev_device *mdev,
+				u16 qid, bool ready)
+{
+
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	vf->vring[qid].ready = ready;
+}
+
+static bool ifcvf_mdev_get_vq_ready(struct mdev_device *mdev, u16 qid)
+{
+
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	return vf->vring[qid].ready;
+}
+
+static void ifcvf_mdev_set_vq_cb(struct mdev_device *mdev, u16 idx,
+				 struct virtio_mdev_callback *cb)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	vf->vring[idx].cb = *cb;
+}
+
+static void ifcvf_mdev_kick_vq(struct mdev_device *mdev, u16 idx)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	ifcvf_notify_queue(vf, idx);
+}
+
+static u8 ifcvf_mdev_get_status(struct mdev_device *mdev)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	return vf->status;
+}
+
+static u32 ifcvf_mdev_get_generation(struct mdev_device *mdev)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	return vf->generation;
+}
+
+static int ifcvf_mdev_get_version(struct mdev_device *mdev)
+{
+	return VIRTIO_MDEV_VERSION;
+}
+
+static u32 ifcvf_mdev_get_device_id(struct mdev_device *mdev)
+{
+	return IFCVF_DEVICE_ID;
+}
+
+static u32 ifcvf_mdev_get_vendor_id(struct mdev_device *mdev)
+{
+	return IFCVF_VENDOR_ID;
+}
+
+static u16 ifcvf_mdev_get_vq_align(struct mdev_device *mdev)
+{
+	return IFCVF_QUEUE_ALIGNMENT;
+}
+
+static int ifcvf_start_datapath(void *private)
+{
+	int i, ret;
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(private);
+
+	for (i = 0; i < (IFCVF_MAX_QUEUE_PAIRS * 2); i++) {
+		if (!vf->vring[i].ready)
+			break;
+
+		if (!vf->vring[i].size)
+			break;
+
+		if (!vf->vring[i].desc || !vf->vring[i].avail ||
+			!vf->vring[i].used)
+			break;
+	}
+	vf->nr_vring = i;
+
+	ret = ifcvf_start_hw(vf);
+	return ret;
+}
+
+static int ifcvf_stop_datapath(void *private)
+{
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(private);
+	int i;
+
+	for (i = 0; i < IFCVF_MAX_QUEUES; i++)
+		vf->vring[i].cb.callback = NULL;
+
+	ifcvf_stop_hw(vf);
+
+	return 0;
+}
+
+static void ifcvf_reset_vring(struct ifcvf_adapter *adapter)
+{
+	int i;
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
+		vf->vring[i].last_used_idx = 0;
+		vf->vring[i].last_avail_idx = 0;
+		vf->vring[i].desc = 0;
+		vf->vring[i].avail = 0;
+		vf->vring[i].used = 0;
+		vf->vring[i].ready = 0;
+		vf->vring->cb.callback = NULL;
+		vf->vring->cb.private = NULL;
+	}
+}
+
+static void ifcvf_mdev_set_status(struct mdev_device *mdev, u8 status)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	vf->status = status;
+
+	if (status == 0) {
+		ifcvf_stop_datapath(adapter);
+		ifcvf_reset_vring(adapter);
+		return;
+	}
+
+	if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
+		ifcvf_start_datapath(adapter);
+		return;
+	}
+}
+
+static u16 ifcvf_mdev_get_queue_max(struct mdev_device *mdev)
+{
+	return IFCVF_MAX_QUEUES;
+}
+
+static struct virtio_mdev_device_ops ifc_mdev_ops = {
+	.get_features  = ifcvf_mdev_get_features,
+	.set_features  = ifcvf_mdev_set_features,
+	.get_status    = ifcvf_mdev_get_status,
+	.set_status    = ifcvf_mdev_set_status,
+	.get_queue_max = ifcvf_mdev_get_queue_max,
+	.get_vq_state   = ifcvf_mdev_get_vq_state,
+	.set_vq_state   = ifcvf_mdev_set_vq_state,
+	.set_vq_cb      = ifcvf_mdev_set_vq_cb,
+	.set_vq_ready   = ifcvf_mdev_set_vq_ready,
+	.get_vq_ready	= ifcvf_mdev_get_vq_ready,
+	.set_vq_num     = ifcvf_mdev_set_vq_num,
+	.set_vq_address = ifcvf_mdev_set_vq_address,
+	.kick_vq        = ifcvf_mdev_kick_vq,
+	.get_generation	= ifcvf_mdev_get_generation,
+	.get_version	= ifcvf_mdev_get_version,
+	.get_device_id	= ifcvf_mdev_get_device_id,
+	.get_vendor_id	= ifcvf_mdev_get_vendor_id,
+	.get_vq_align	= ifcvf_mdev_get_vq_align,
+};
+
+static int ifcvf_init_msix(struct ifcvf_adapter *adapter)
+{
+	int vector, i, ret, irq;
+	struct pci_dev *pdev = to_pci_dev(adapter->dev);
+	struct ifcvf_hw *vf = &adapter->vf;
+
+	ret = pci_alloc_irq_vectors(pdev, IFCVF_MAX_INTR,
+			IFCVF_MAX_INTR, PCI_IRQ_MSIX);
+	if (ret < 0) {
+		IFC_ERR(adapter->dev, "Failed to alloc irq vectors.\n");
+		return ret;
+	}
+
+	for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
+		vector = i + IFCVF_MSI_QUEUE_OFF;
+		irq = pci_irq_vector(pdev, vector);
+		ret = request_irq(irq, ifcvf_intr_handler, 0,
+				pci_name(pdev), &vf->vring[i]);
+		if (ret) {
+			IFC_ERR(adapter->dev,
+				"Failed to request irq for vq %d.\n", i);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static void ifcvf_destroy_adapter(struct ifcvf_adapter *adapter)
+{
+	int i, vector, irq;
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+	struct pci_dev *pdev = to_pci_dev(adapter->dev);
+
+	for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
+		vector = i + IFCVF_MSI_QUEUE_OFF;
+		irq = pci_irq_vector(pdev, vector);
+		free_irq(irq, &vf->vring[i]);
+	}
+}
+
+static ssize_t name_show(struct kobject *kobj, struct device *dev, char *buf)
+{
+	const char *name = "vhost accelerator (virtio ring compatible)";
+
+	return sprintf(buf, "%s\n", name);
+}
+MDEV_TYPE_ATTR_RO(name);
+
+static ssize_t device_api_show(struct kobject *kobj, struct device *dev,
+			       char *buf)
+{
+	return sprintf(buf, "%s\n", VIRTIO_MDEV_DEVICE_API_STRING);
+}
+MDEV_TYPE_ATTR_RO(device_api);
+
+static ssize_t available_instances_show(struct kobject *kobj,
+					struct device *dev, char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
+
+	return sprintf(buf, "%d\n", adapter->mdev_count);
+}
+
+MDEV_TYPE_ATTR_RO(available_instances);
+
+static ssize_t type_show(struct kobject *kobj,
+			struct device *dev, char *buf)
+{
+	return sprintf(buf, "%s\n", "net");
+}
+
+MDEV_TYPE_ATTR_RO(type);
+
+
+static struct attribute *mdev_types_attrs[] = {
+	&mdev_type_attr_name.attr,
+	&mdev_type_attr_device_api.attr,
+	&mdev_type_attr_available_instances.attr,
+	&mdev_type_attr_type.attr,
+	NULL,
+};
+
+static struct attribute_group mdev_type_group = {
+	.name  = "vdpa_virtio",
+	.attrs = mdev_types_attrs,
+};
+
+static struct attribute_group *mdev_type_groups[] = {
+	&mdev_type_group,
+	NULL,
+};
+
+const struct attribute_group *mdev_dev_groups[] = {
+	NULL,
+};
+
+static int ifcvf_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
+{
+	struct device *dev = mdev_parent_dev(mdev);
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
+	int ret = 0;
+
+	mutex_lock(&adapter->mdev_lock);
+
+	if (adapter->mdev_count < 1) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	mdev_set_class_id(mdev, MDEV_ID_VHOST);
+	mdev_set_dev_ops(mdev, &ifc_mdev_ops);
+
+	mdev_set_drvdata(mdev, adapter);
+	mdev_set_iommu_device(mdev_dev(mdev), dev);
+
+	INIT_LIST_HEAD(&adapter->dma_maps);
+	adapter->mdev_count--;
+
+out:
+	mutex_unlock(&adapter->mdev_lock);
+	return ret;
+}
+
+static int ifcvf_mdev_remove(struct mdev_device *mdev)
+{
+	struct device *dev = mdev_parent_dev(mdev);
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
+
+	mutex_lock(&adapter->mdev_lock);
+	adapter->mdev_count++;
+	mutex_unlock(&adapter->mdev_lock);
+
+	return 0;
+}
+
+static struct mdev_parent_ops ifcvf_mdev_fops = {
+	.owner			= THIS_MODULE,
+	.supported_type_groups	= mdev_type_groups,
+	.mdev_attr_groups	= mdev_dev_groups,
+	.create			= ifcvf_mdev_create,
+	.remove			= ifcvf_mdev_remove,
+};
+
+static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+	struct device *dev = &pdev->dev;
+	struct ifcvf_adapter *adapter;
+	struct ifcvf_hw *vf;
+	int ret, i;
+
+	adapter = kzalloc(sizeof(struct ifcvf_adapter), GFP_KERNEL);
+	if (adapter == NULL) {
+		ret = -ENOMEM;
+		goto fail;
+	}
+
+	mutex_init(&adapter->mdev_lock);
+	adapter->mdev_count = 1;
+	adapter->dev = dev;
+
+	pci_set_drvdata(pdev, adapter);
+
+	ret = pci_enable_device(pdev);
+	if (ret) {
+		IFC_ERR(adapter->dev, "Failed to enable device.\n");
+		goto free_adapter;
+	}
+
+	ret = pci_request_regions(pdev, IFCVF_DRIVER_NAME);
+	if (ret) {
+		IFC_ERR(adapter->dev, "Failed to request MMIO region.\n");
+		goto disable_device;
+	}
+
+	pci_set_master(pdev);
+
+	ret = ifcvf_init_msix(adapter);
+	if (ret) {
+		IFC_ERR(adapter->dev, "Failed to initialize MSIX.\n");
+		goto free_msix;
+	}
+
+	vf = &adapter->vf;
+	for (i = 0; i < IFCVF_PCI_MAX_RESOURCE; i++) {
+		vf->mem_resource[i].phys_addr = pci_resource_start(pdev, i);
+		vf->mem_resource[i].len = pci_resource_len(pdev, i);
+		if (!vf->mem_resource[i].len) {
+			vf->mem_resource[i].addr = NULL;
+			continue;
+		}
+
+		vf->mem_resource[i].addr = pci_iomap_range(pdev, i, 0,
+				vf->mem_resource[i].len);
+		if (!vf->mem_resource[i].addr) {
+			IFC_ERR(adapter->dev, "Failed to map IO resource %d\n",
+				i);
+			return -1;
+		}
+	}
+
+	if (ifcvf_init_hw(vf, pdev) < 0)
+		return -1;
+
+	ret = mdev_register_device(dev, &ifcvf_mdev_fops);
+	if (ret) {
+		IFC_ERR(adapter->dev,  "Failed to register mdev device\n");
+		goto destroy_adapter;
+	}
+
+	return 0;
+
+destroy_adapter:
+	ifcvf_destroy_adapter(adapter);
+free_msix:
+	pci_free_irq_vectors(pdev);
+	pci_release_regions(pdev);
+disable_device:
+	pci_disable_device(pdev);
+free_adapter:
+	kfree(adapter);
+fail:
+	return ret;
+}
+
+static void ifcvf_remove(struct pci_dev *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
+	struct ifcvf_hw *vf;
+	int i;
+
+	mdev_unregister_device(dev);
+
+	vf = &adapter->vf;
+	for (i = 0; i < IFCVF_PCI_MAX_RESOURCE; i++) {
+		if (vf->mem_resource[i].addr) {
+			pci_iounmap(pdev, vf->mem_resource[i].addr);
+			vf->mem_resource[i].addr = NULL;
+		}
+	}
+
+	ifcvf_destroy_adapter(adapter);
+	pci_free_irq_vectors(pdev);
+
+	pci_release_regions(pdev);
+	pci_disable_device(pdev);
+
+	kfree(adapter);
+}
+
+static struct pci_device_id ifcvf_pci_ids[] = {
+	{ PCI_DEVICE_SUB(IFCVF_VENDOR_ID,
+			IFCVF_DEVICE_ID,
+			IFCVF_SUBSYS_VENDOR_ID,
+			IFCVF_SUBSYS_DEVICE_ID) },
+	{ 0 },
+};
+MODULE_DEVICE_TABLE(pci, ifcvf_pci_ids);
+
+static struct pci_driver ifcvf_driver = {
+	.name     = IFCVF_DRIVER_NAME,
+	.id_table = ifcvf_pci_ids,
+	.probe    = ifcvf_probe,
+	.remove   = ifcvf_remove,
+};
+
+static int __init ifcvf_init_module(void)
+{
+	int ret;
+
+	ret = pci_register_driver(&ifcvf_driver);
+	return ret;
+}
+
+static void __exit ifcvf_exit_module(void)
+{
+	pci_unregister_driver(&ifcvf_driver);
+}
+
+module_init(ifcvf_init_module);
+module_exit(ifcvf_exit_module);
+
+MODULE_LICENSE("GPL v2");
+MODULE_VERSION(VERSION_STRING);
+MODULE_AUTHOR(DRIVER_AUTHOR);
-- 
2.16.4

[RFC 0/2] Intel IFC VF driver for vdpa

[Zhu Lingshan]

Hi all:
 
This series intends to introduce Intel IFC VF NIC driver for Vhost
Data Plane Acceleration.
 
Here comes two main parts, one is ifcvf_base layer, which handles
hardware operations. The other is ifcvf_main layer handles VF
initialization, configuration and removal, which depends on
and complys to vhost_mdev https://lkml.org/lkml/2019/9/26/15 
 
This is a first RFC try, please help review.
 
Thanks!
BR
Zhu Lingshan


Zhu Lingshan (2):
  vhost: IFC VF hardware operation layer
  vhost: IFC VF vdpa layer

 drivers/vhost/ifcvf/ifcvf_base.c | 390 ++++++++++++++++++++++++++++
 drivers/vhost/ifcvf/ifcvf_base.h | 137 ++++++++++
 drivers/vhost/ifcvf/ifcvf_main.c | 541 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 1068 insertions(+)
 create mode 100644 drivers/vhost/ifcvf/ifcvf_base.c
 create mode 100644 drivers/vhost/ifcvf/ifcvf_base.h
 create mode 100644 drivers/vhost/ifcvf/ifcvf_main.c

-- 
2.16.4

[RFC 2/2] vhost: IFC VF vdpa layer

[Zhu Lingshan]

This commit introduced IFC VF operations for vdpa, which complys to
vhost_mdev interfaces, handles IFC VF initialization,
configuration and removal.

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 drivers/vhost/ifcvf/ifcvf_main.c | 541 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 541 insertions(+)
 create mode 100644 drivers/vhost/ifcvf/ifcvf_main.c

diff --git a/drivers/vhost/ifcvf/ifcvf_main.c b/drivers/vhost/ifcvf/ifcvf_main.c
new file mode 100644
index 000000000000..c48a29969a85
--- /dev/null
+++ b/drivers/vhost/ifcvf/ifcvf_main.c
@@ -0,0 +1,541 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2019 Intel Corporation.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/mdev.h>
+#include <linux/pci.h>
+#include <linux/sysfs.h>
+
+#include "ifcvf_base.h"
+
+#define VERSION_STRING	"0.1"
+#define DRIVER_AUTHOR	"Intel Corporation"
+#define IFCVF_DRIVER_NAME	"ifcvf"
+
+static irqreturn_t ifcvf_intr_handler(int irq, void *arg)
+{
+	struct vring_info *vring = arg;
+
+	if (vring->cb.callback)
+		return vring->cb.callback(vring->cb.private);
+
+	return IRQ_HANDLED;
+}
+
+static u64 ifcvf_mdev_get_features(struct mdev_device *mdev)
+{
+	return IFC_SUPPORTED_FEATURES;
+}
+
+static int ifcvf_mdev_set_features(struct mdev_device *mdev, u64 features)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	vf->req_features = features;
+
+	return 0;
+}
+
+static u64 ifcvf_mdev_get_vq_state(struct mdev_device *mdev, u16 qid)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	return vf->vring[qid].last_avail_idx;
+}
+
+static int ifcvf_mdev_set_vq_state(struct mdev_device *mdev, u16 qid, u64 num)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	vf->vring[qid].last_used_idx = num;
+	vf->vring[qid].last_avail_idx = num;
+
+	return 0;
+}
+
+static int ifcvf_mdev_set_vq_address(struct mdev_device *mdev, u16 idx,
+				     u64 desc_area, u64 driver_area,
+				     u64 device_area)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	vf->vring[idx].desc = desc_area;
+	vf->vring[idx].avail = driver_area;
+	vf->vring[idx].used = device_area;
+
+	return 0;
+}
+
+static void ifcvf_mdev_set_vq_num(struct mdev_device *mdev, u16 qid, u32 num)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	vf->vring[qid].size = num;
+}
+
+static void ifcvf_mdev_set_vq_ready(struct mdev_device *mdev,
+				u16 qid, bool ready)
+{
+
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	vf->vring[qid].ready = ready;
+}
+
+static bool ifcvf_mdev_get_vq_ready(struct mdev_device *mdev, u16 qid)
+{
+
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	return vf->vring[qid].ready;
+}
+
+static void ifcvf_mdev_set_vq_cb(struct mdev_device *mdev, u16 idx,
+				 struct virtio_mdev_callback *cb)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	vf->vring[idx].cb = *cb;
+}
+
+static void ifcvf_mdev_kick_vq(struct mdev_device *mdev, u16 idx)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	ifcvf_notify_queue(vf, idx);
+}
+
+static u8 ifcvf_mdev_get_status(struct mdev_device *mdev)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	return vf->status;
+}
+
+static u32 ifcvf_mdev_get_generation(struct mdev_device *mdev)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	return vf->generation;
+}
+
+static int ifcvf_mdev_get_version(struct mdev_device *mdev)
+{
+	return VIRTIO_MDEV_VERSION;
+}
+
+static u32 ifcvf_mdev_get_device_id(struct mdev_device *mdev)
+{
+	return IFCVF_DEVICE_ID;
+}
+
+static u32 ifcvf_mdev_get_vendor_id(struct mdev_device *mdev)
+{
+	return IFCVF_VENDOR_ID;
+}
+
+static u16 ifcvf_mdev_get_vq_align(struct mdev_device *mdev)
+{
+	return IFCVF_QUEUE_ALIGNMENT;
+}
+
+static int ifcvf_start_datapath(void *private)
+{
+	int i, ret;
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(private);
+
+	for (i = 0; i < (IFCVF_MAX_QUEUE_PAIRS * 2); i++) {
+		if (!vf->vring[i].ready)
+			break;
+
+		if (!vf->vring[i].size)
+			break;
+
+		if (!vf->vring[i].desc || !vf->vring[i].avail ||
+			!vf->vring[i].used)
+			break;
+	}
+	vf->nr_vring = i;
+
+	ret = ifcvf_start_hw(vf);
+	return ret;
+}
+
+static int ifcvf_stop_datapath(void *private)
+{
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(private);
+	int i;
+
+	for (i = 0; i < IFCVF_MAX_QUEUES; i++)
+		vf->vring[i].cb.callback = NULL;
+
+	ifcvf_stop_hw(vf);
+
+	return 0;
+}
+
+static void ifcvf_reset_vring(struct ifcvf_adapter *adapter)
+{
+	int i;
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
+		vf->vring[i].last_used_idx = 0;
+		vf->vring[i].last_avail_idx = 0;
+		vf->vring[i].desc = 0;
+		vf->vring[i].avail = 0;
+		vf->vring[i].used = 0;
+		vf->vring[i].ready = 0;
+		vf->vring->cb.callback = NULL;
+		vf->vring->cb.private = NULL;
+	}
+}
+
+static void ifcvf_mdev_set_status(struct mdev_device *mdev, u8 status)
+{
+	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+
+	vf->status = status;
+
+	if (status == 0) {
+		ifcvf_stop_datapath(adapter);
+		ifcvf_reset_vring(adapter);
+		return;
+	}
+
+	if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
+		ifcvf_start_datapath(adapter);
+		return;
+	}
+}
+
+static u16 ifcvf_mdev_get_queue_max(struct mdev_device *mdev)
+{
+	return IFCVF_MAX_QUEUES;
+}
+
+static struct virtio_mdev_device_ops ifc_mdev_ops = {
+	.get_features  = ifcvf_mdev_get_features,
+	.set_features  = ifcvf_mdev_set_features,
+	.get_status    = ifcvf_mdev_get_status,
+	.set_status    = ifcvf_mdev_set_status,
+	.get_queue_max = ifcvf_mdev_get_queue_max,
+	.get_vq_state   = ifcvf_mdev_get_vq_state,
+	.set_vq_state   = ifcvf_mdev_set_vq_state,
+	.set_vq_cb      = ifcvf_mdev_set_vq_cb,
+	.set_vq_ready   = ifcvf_mdev_set_vq_ready,
+	.get_vq_ready	= ifcvf_mdev_get_vq_ready,
+	.set_vq_num     = ifcvf_mdev_set_vq_num,
+	.set_vq_address = ifcvf_mdev_set_vq_address,
+	.kick_vq        = ifcvf_mdev_kick_vq,
+	.get_generation	= ifcvf_mdev_get_generation,
+	.get_version	= ifcvf_mdev_get_version,
+	.get_device_id	= ifcvf_mdev_get_device_id,
+	.get_vendor_id	= ifcvf_mdev_get_vendor_id,
+	.get_vq_align	= ifcvf_mdev_get_vq_align,
+};
+
+static int ifcvf_init_msix(struct ifcvf_adapter *adapter)
+{
+	int vector, i, ret, irq;
+	struct pci_dev *pdev = to_pci_dev(adapter->dev);
+	struct ifcvf_hw *vf = &adapter->vf;
+
+	ret = pci_alloc_irq_vectors(pdev, IFCVF_MAX_INTR,
+			IFCVF_MAX_INTR, PCI_IRQ_MSIX);
+	if (ret < 0) {
+		IFC_ERR(adapter->dev, "Failed to alloc irq vectors.\n");
+		return ret;
+	}
+
+	for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
+		vector = i + IFCVF_MSI_QUEUE_OFF;
+		irq = pci_irq_vector(pdev, vector);
+		ret = request_irq(irq, ifcvf_intr_handler, 0,
+				pci_name(pdev), &vf->vring[i]);
+		if (ret) {
+			IFC_ERR(adapter->dev,
+				"Failed to request irq for vq %d.\n", i);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static void ifcvf_destroy_adapter(struct ifcvf_adapter *adapter)
+{
+	int i, vector, irq;
+	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
+	struct pci_dev *pdev = to_pci_dev(adapter->dev);
+
+	for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
+		vector = i + IFCVF_MSI_QUEUE_OFF;
+		irq = pci_irq_vector(pdev, vector);
+		free_irq(irq, &vf->vring[i]);
+	}
+}
+
+static ssize_t name_show(struct kobject *kobj, struct device *dev, char *buf)
+{
+	const char *name = "vhost accelerator (virtio ring compatible)";
+
+	return sprintf(buf, "%s\n", name);
+}
+MDEV_TYPE_ATTR_RO(name);
+
+static ssize_t device_api_show(struct kobject *kobj, struct device *dev,
+			       char *buf)
+{
+	return sprintf(buf, "%s\n", VIRTIO_MDEV_DEVICE_API_STRING);
+}
+MDEV_TYPE_ATTR_RO(device_api);
+
+static ssize_t available_instances_show(struct kobject *kobj,
+					struct device *dev, char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
+
+	return sprintf(buf, "%d\n", adapter->mdev_count);
+}
+
+MDEV_TYPE_ATTR_RO(available_instances);
+
+static ssize_t type_show(struct kobject *kobj,
+			struct device *dev, char *buf)
+{
+	return sprintf(buf, "%s\n", "net");
+}
+
+MDEV_TYPE_ATTR_RO(type);
+
+
+static struct attribute *mdev_types_attrs[] = {
+	&mdev_type_attr_name.attr,
+	&mdev_type_attr_device_api.attr,
+	&mdev_type_attr_available_instances.attr,
+	&mdev_type_attr_type.attr,
+	NULL,
+};
+
+static struct attribute_group mdev_type_group = {
+	.name  = "vdpa_virtio",
+	.attrs = mdev_types_attrs,
+};
+
+static struct attribute_group *mdev_type_groups[] = {
+	&mdev_type_group,
+	NULL,
+};
+
+const struct attribute_group *mdev_dev_groups[] = {
+	NULL,
+};
+
+static int ifcvf_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
+{
+	struct device *dev = mdev_parent_dev(mdev);
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
+	int ret = 0;
+
+	mutex_lock(&adapter->mdev_lock);
+
+	if (adapter->mdev_count < 1) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	mdev_set_class_id(mdev, MDEV_ID_VHOST);
+	mdev_set_dev_ops(mdev, &ifc_mdev_ops);
+
+	mdev_set_drvdata(mdev, adapter);
+	mdev_set_iommu_device(mdev_dev(mdev), dev);
+
+	INIT_LIST_HEAD(&adapter->dma_maps);
+	adapter->mdev_count--;
+
+out:
+	mutex_unlock(&adapter->mdev_lock);
+	return ret;
+}
+
+static int ifcvf_mdev_remove(struct mdev_device *mdev)
+{
+	struct device *dev = mdev_parent_dev(mdev);
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
+
+	mutex_lock(&adapter->mdev_lock);
+	adapter->mdev_count++;
+	mutex_unlock(&adapter->mdev_lock);
+
+	return 0;
+}
+
+static struct mdev_parent_ops ifcvf_mdev_fops = {
+	.owner			= THIS_MODULE,
+	.supported_type_groups	= mdev_type_groups,
+	.mdev_attr_groups	= mdev_dev_groups,
+	.create			= ifcvf_mdev_create,
+	.remove			= ifcvf_mdev_remove,
+};
+
+static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+	struct device *dev = &pdev->dev;
+	struct ifcvf_adapter *adapter;
+	struct ifcvf_hw *vf;
+	int ret, i;
+
+	adapter = kzalloc(sizeof(struct ifcvf_adapter), GFP_KERNEL);
+	if (adapter == NULL) {
+		ret = -ENOMEM;
+		goto fail;
+	}
+
+	mutex_init(&adapter->mdev_lock);
+	adapter->mdev_count = 1;
+	adapter->dev = dev;
+
+	pci_set_drvdata(pdev, adapter);
+
+	ret = pci_enable_device(pdev);
+	if (ret) {
+		IFC_ERR(adapter->dev, "Failed to enable device.\n");
+		goto free_adapter;
+	}
+
+	ret = pci_request_regions(pdev, IFCVF_DRIVER_NAME);
+	if (ret) {
+		IFC_ERR(adapter->dev, "Failed to request MMIO region.\n");
+		goto disable_device;
+	}
+
+	pci_set_master(pdev);
+
+	ret = ifcvf_init_msix(adapter);
+	if (ret) {
+		IFC_ERR(adapter->dev, "Failed to initialize MSIX.\n");
+		goto free_msix;
+	}
+
+	vf = &adapter->vf;
+	for (i = 0; i < IFCVF_PCI_MAX_RESOURCE; i++) {
+		vf->mem_resource[i].phys_addr = pci_resource_start(pdev, i);
+		vf->mem_resource[i].len = pci_resource_len(pdev, i);
+		if (!vf->mem_resource[i].len) {
+			vf->mem_resource[i].addr = NULL;
+			continue;
+		}
+
+		vf->mem_resource[i].addr = pci_iomap_range(pdev, i, 0,
+				vf->mem_resource[i].len);
+		if (!vf->mem_resource[i].addr) {
+			IFC_ERR(adapter->dev, "Failed to map IO resource %d\n",
+				i);
+			return -1;
+		}
+	}
+
+	if (ifcvf_init_hw(vf, pdev) < 0)
+		return -1;
+
+	ret = mdev_register_device(dev, &ifcvf_mdev_fops);
+	if (ret) {
+		IFC_ERR(adapter->dev,  "Failed to register mdev device\n");
+		goto destroy_adapter;
+	}
+
+	return 0;
+
+destroy_adapter:
+	ifcvf_destroy_adapter(adapter);
+free_msix:
+	pci_free_irq_vectors(pdev);
+	pci_release_regions(pdev);
+disable_device:
+	pci_disable_device(pdev);
+free_adapter:
+	kfree(adapter);
+fail:
+	return ret;
+}
+
+static void ifcvf_remove(struct pci_dev *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
+	struct ifcvf_hw *vf;
+	int i;
+
+	mdev_unregister_device(dev);
+
+	vf = &adapter->vf;
+	for (i = 0; i < IFCVF_PCI_MAX_RESOURCE; i++) {
+		if (vf->mem_resource[i].addr) {
+			pci_iounmap(pdev, vf->mem_resource[i].addr);
+			vf->mem_resource[i].addr = NULL;
+		}
+	}
+
+	ifcvf_destroy_adapter(adapter);
+	pci_free_irq_vectors(pdev);
+
+	pci_release_regions(pdev);
+	pci_disable_device(pdev);
+
+	kfree(adapter);
+}
+
+static struct pci_device_id ifcvf_pci_ids[] = {
+	{ PCI_DEVICE_SUB(IFCVF_VENDOR_ID,
+			IFCVF_DEVICE_ID,
+			IFCVF_SUBSYS_VENDOR_ID,
+			IFCVF_SUBSYS_DEVICE_ID) },
+	{ 0 },
+};
+MODULE_DEVICE_TABLE(pci, ifcvf_pci_ids);
+
+static struct pci_driver ifcvf_driver = {
+	.name     = IFCVF_DRIVER_NAME,
+	.id_table = ifcvf_pci_ids,
+	.probe    = ifcvf_probe,
+	.remove   = ifcvf_remove,
+};
+
+static int __init ifcvf_init_module(void)
+{
+	int ret;
+
+	ret = pci_register_driver(&ifcvf_driver);
+	return ret;
+}
+
+static void __exit ifcvf_exit_module(void)
+{
+	pci_unregister_driver(&ifcvf_driver);
+}
+
+module_init(ifcvf_init_module);
+module_exit(ifcvf_exit_module);
+
+MODULE_LICENSE("GPL v2");
+MODULE_VERSION(VERSION_STRING);
+MODULE_AUTHOR(DRIVER_AUTHOR);
-- 
2.16.4

Re: [RFC 2/2] vhost: IFC VF vdpa layer

[Jason Wang]

On 2019/10/16 上午9:30, Zhu Lingshan wrote:
> This commit introduced IFC VF operations for vdpa, which complys to
> vhost_mdev interfaces, handles IFC VF initialization,
> configuration and removal.
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>   drivers/vhost/ifcvf/ifcvf_main.c | 541 +++++++++++++++++++++++++++++++++++++++
>   1 file changed, 541 insertions(+)
>   create mode 100644 drivers/vhost/ifcvf/ifcvf_main.c
>
> diff --git a/drivers/vhost/ifcvf/ifcvf_main.c b/drivers/vhost/ifcvf/ifcvf_main.c
> new file mode 100644
> index 000000000000..c48a29969a85
> --- /dev/null
> +++ b/drivers/vhost/ifcvf/ifcvf_main.c
> @@ -0,0 +1,541 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2019 Intel Corporation.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/mdev.h>
> +#include <linux/pci.h>
> +#include <linux/sysfs.h>
> +
> +#include "ifcvf_base.h"
> +
> +#define VERSION_STRING	"0.1"
> +#define DRIVER_AUTHOR	"Intel Corporation"
> +#define IFCVF_DRIVER_NAME	"ifcvf"
> +
> +static irqreturn_t ifcvf_intr_handler(int irq, void *arg)
> +{
> +	struct vring_info *vring = arg;
> +
> +	if (vring->cb.callback)
> +		return vring->cb.callback(vring->cb.private);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static u64 ifcvf_mdev_get_features(struct mdev_device *mdev)
> +{
> +	return IFC_SUPPORTED_FEATURES;


I would expect this should be done by querying the hw. Or IFC VF can't 
get any update through its firmware?


> +}
> +
> +static int ifcvf_mdev_set_features(struct mdev_device *mdev, u64 features)
> +{
> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> +	vf->req_features = features;
> +
> +	return 0;
> +}
> +
> +static u64 ifcvf_mdev_get_vq_state(struct mdev_device *mdev, u16 qid)
> +{
> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> +	return vf->vring[qid].last_avail_idx;


Does this really work? I'd expect it should be fetched from hw since 
it's an internal state.


> +}
> +
> +static int ifcvf_mdev_set_vq_state(struct mdev_device *mdev, u16 qid, u64 num)
> +{
> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> +	vf->vring[qid].last_used_idx = num;


I fail to understand why last_used_idx is needed. It looks to me the 
used idx in the used ring is sufficient.


> +	vf->vring[qid].last_avail_idx = num;


Do we need a synchronization with hw immediately here?


> +
> +	return 0;
> +}
> +
> +static int ifcvf_mdev_set_vq_address(struct mdev_device *mdev, u16 idx,
> +				     u64 desc_area, u64 driver_area,
> +				     u64 device_area)
> +{
> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> +	vf->vring[idx].desc = desc_area;
> +	vf->vring[idx].avail = driver_area;
> +	vf->vring[idx].used = device_area;
> +
> +	return 0;
> +}
> +
> +static void ifcvf_mdev_set_vq_num(struct mdev_device *mdev, u16 qid, u32 num)
> +{
> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> +	vf->vring[qid].size = num;
> +}
> +
> +static void ifcvf_mdev_set_vq_ready(struct mdev_device *mdev,
> +				u16 qid, bool ready)
> +{
> +
> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> +	vf->vring[qid].ready = ready;
> +}
> +
> +static bool ifcvf_mdev_get_vq_ready(struct mdev_device *mdev, u16 qid)
> +{
> +
> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> +	return vf->vring[qid].ready;
> +}
> +
> +static void ifcvf_mdev_set_vq_cb(struct mdev_device *mdev, u16 idx,
> +				 struct virtio_mdev_callback *cb)
> +{
> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> +	vf->vring[idx].cb = *cb;
> +}
> +
> +static void ifcvf_mdev_kick_vq(struct mdev_device *mdev, u16 idx)
> +{
> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> +	ifcvf_notify_queue(vf, idx);
> +}
> +
> +static u8 ifcvf_mdev_get_status(struct mdev_device *mdev)
> +{
> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> +	return vf->status;
> +}
> +
> +static u32 ifcvf_mdev_get_generation(struct mdev_device *mdev)
> +{
> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> +	return vf->generation;
> +}
> +
> +static int ifcvf_mdev_get_version(struct mdev_device *mdev)
> +{
> +	return VIRTIO_MDEV_VERSION;
> +}
> +
> +static u32 ifcvf_mdev_get_device_id(struct mdev_device *mdev)
> +{
> +	return IFCVF_DEVICE_ID;
> +}
> +
> +static u32 ifcvf_mdev_get_vendor_id(struct mdev_device *mdev)
> +{
> +	return IFCVF_VENDOR_ID;
> +}
> +
> +static u16 ifcvf_mdev_get_vq_align(struct mdev_device *mdev)
> +{
> +	return IFCVF_QUEUE_ALIGNMENT;
> +}
> +
> +static int ifcvf_start_datapath(void *private)
> +{
> +	int i, ret;
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(private);
> +
> +	for (i = 0; i < (IFCVF_MAX_QUEUE_PAIRS * 2); i++) {
> +		if (!vf->vring[i].ready)
> +			break;


Looks like error should be returned here?


> +
> +		if (!vf->vring[i].size)
> +			break;
> +
> +		if (!vf->vring[i].desc || !vf->vring[i].avail ||
> +			!vf->vring[i].used)
> +			break;
> +	}
> +	vf->nr_vring = i;
> +
> +	ret = ifcvf_start_hw(vf);
> +	return ret;
> +}
> +
> +static int ifcvf_stop_datapath(void *private)
> +{
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(private);
> +	int i;
> +
> +	for (i = 0; i < IFCVF_MAX_QUEUES; i++)
> +		vf->vring[i].cb.callback = NULL;


Any synchronization is needed for the vq irq handler?


> +
> +	ifcvf_stop_hw(vf);
> +
> +	return 0;
> +}
> +
> +static void ifcvf_reset_vring(struct ifcvf_adapter *adapter)
> +{
> +	int i;
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> +	for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
> +		vf->vring[i].last_used_idx = 0;
> +		vf->vring[i].last_avail_idx = 0;
> +		vf->vring[i].desc = 0;
> +		vf->vring[i].avail = 0;
> +		vf->vring[i].used = 0;
> +		vf->vring[i].ready = 0;
> +		vf->vring->cb.callback = NULL;
> +		vf->vring->cb.private = NULL;
> +	}
> +}
> +
> +static void ifcvf_mdev_set_status(struct mdev_device *mdev, u8 status)
> +{
> +	struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> +	vf->status = status;
> +
> +	if (status == 0) {
> +		ifcvf_stop_datapath(adapter);
> +		ifcvf_reset_vring(adapter);
> +		return;
> +	}
> +
> +	if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
> +		ifcvf_start_datapath(adapter);
> +		return;
> +	}
> +}
> +
> +static u16 ifcvf_mdev_get_queue_max(struct mdev_device *mdev)
> +{
> +	return IFCVF_MAX_QUEUES;


The name is confusing, it was used to return the maximum queue size. In 
new version of virtio-mdev, the callback was renamed as get_vq_num_max().


> +}
> +
> +static struct virtio_mdev_device_ops ifc_mdev_ops = {
> +	.get_features  = ifcvf_mdev_get_features,
> +	.set_features  = ifcvf_mdev_set_features,
> +	.get_status    = ifcvf_mdev_get_status,
> +	.set_status    = ifcvf_mdev_set_status,
> +	.get_queue_max = ifcvf_mdev_get_queue_max,
> +	.get_vq_state   = ifcvf_mdev_get_vq_state,
> +	.set_vq_state   = ifcvf_mdev_set_vq_state,
> +	.set_vq_cb      = ifcvf_mdev_set_vq_cb,
> +	.set_vq_ready   = ifcvf_mdev_set_vq_ready,
> +	.get_vq_ready	= ifcvf_mdev_get_vq_ready,
> +	.set_vq_num     = ifcvf_mdev_set_vq_num,
> +	.set_vq_address = ifcvf_mdev_set_vq_address,
> +	.kick_vq        = ifcvf_mdev_kick_vq,
> +	.get_generation	= ifcvf_mdev_get_generation,
> +	.get_version	= ifcvf_mdev_get_version,
> +	.get_device_id	= ifcvf_mdev_get_device_id,
> +	.get_vendor_id	= ifcvf_mdev_get_vendor_id,
> +	.get_vq_align	= ifcvf_mdev_get_vq_align,
> +};


set_config/get_config is missing. It looks to me they are not hard, just 
implementing the access to dev_cfg. It's key to make kernel virtio 
driver to work.

And in the new version of virito-mdev, features like _F_LOG_ALL should 
be advertised through get_mdev_features.


> +
> +static int ifcvf_init_msix(struct ifcvf_adapter *adapter)
> +{
> +	int vector, i, ret, irq;
> +	struct pci_dev *pdev = to_pci_dev(adapter->dev);
> +	struct ifcvf_hw *vf = &adapter->vf;
> +
> +	ret = pci_alloc_irq_vectors(pdev, IFCVF_MAX_INTR,
> +			IFCVF_MAX_INTR, PCI_IRQ_MSIX);
> +	if (ret < 0) {
> +		IFC_ERR(adapter->dev, "Failed to alloc irq vectors.\n");
> +		return ret;
> +	}
> +
> +	for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
> +		vector = i + IFCVF_MSI_QUEUE_OFF;
> +		irq = pci_irq_vector(pdev, vector);
> +		ret = request_irq(irq, ifcvf_intr_handler, 0,
> +				pci_name(pdev), &vf->vring[i]);
> +		if (ret) {
> +			IFC_ERR(adapter->dev,
> +				"Failed to request irq for vq %d.\n", i);
> +			return ret;
> +		}
> +	}


Do we need to provide fallback when we can't do per vq MSIX?


> +
> +	return 0;
> +}
> +
> +static void ifcvf_destroy_adapter(struct ifcvf_adapter *adapter)
> +{
> +	int i, vector, irq;
> +	struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +	struct pci_dev *pdev = to_pci_dev(adapter->dev);
> +
> +	for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
> +		vector = i + IFCVF_MSI_QUEUE_OFF;
> +		irq = pci_irq_vector(pdev, vector);
> +		free_irq(irq, &vf->vring[i]);
> +	}
> +}
> +
> +static ssize_t name_show(struct kobject *kobj, struct device *dev, char *buf)
> +{
> +	const char *name = "vhost accelerator (virtio ring compatible)";
> +
> +	return sprintf(buf, "%s\n", name);
> +}
> +MDEV_TYPE_ATTR_RO(name);
> +
> +static ssize_t device_api_show(struct kobject *kobj, struct device *dev,
> +			       char *buf)
> +{
> +	return sprintf(buf, "%s\n", VIRTIO_MDEV_DEVICE_API_STRING);
> +}
> +MDEV_TYPE_ATTR_RO(device_api);
> +
> +static ssize_t available_instances_show(struct kobject *kobj,
> +					struct device *dev, char *buf)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
> +
> +	return sprintf(buf, "%d\n", adapter->mdev_count);
> +}
> +
> +MDEV_TYPE_ATTR_RO(available_instances);
> +
> +static ssize_t type_show(struct kobject *kobj,
> +			struct device *dev, char *buf)
> +{
> +	return sprintf(buf, "%s\n", "net");
> +}
> +
> +MDEV_TYPE_ATTR_RO(type);
> +
> +
> +static struct attribute *mdev_types_attrs[] = {
> +	&mdev_type_attr_name.attr,
> +	&mdev_type_attr_device_api.attr,
> +	&mdev_type_attr_available_instances.attr,
> +	&mdev_type_attr_type.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group mdev_type_group = {
> +	.name  = "vdpa_virtio",


To be consistent, it should be "vhost" or "virtio".


> +	.attrs = mdev_types_attrs,
> +};
> +
> +static struct attribute_group *mdev_type_groups[] = {
> +	&mdev_type_group,
> +	NULL,
> +};
> +
> +const struct attribute_group *mdev_dev_groups[] = {
> +	NULL,
> +};
> +
> +static int ifcvf_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
> +{
> +	struct device *dev = mdev_parent_dev(mdev);
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
> +	int ret = 0;
> +
> +	mutex_lock(&adapter->mdev_lock);
> +
> +	if (adapter->mdev_count < 1) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	mdev_set_class_id(mdev, MDEV_ID_VHOST);
> +	mdev_set_dev_ops(mdev, &ifc_mdev_ops);
> +
> +	mdev_set_drvdata(mdev, adapter);
> +	mdev_set_iommu_device(mdev_dev(mdev), dev);
> +
> +	INIT_LIST_HEAD(&adapter->dma_maps);
> +	adapter->mdev_count--;
> +
> +out:
> +	mutex_unlock(&adapter->mdev_lock);
> +	return ret;
> +}
> +
> +static int ifcvf_mdev_remove(struct mdev_device *mdev)
> +{
> +	struct device *dev = mdev_parent_dev(mdev);
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
> +
> +	mutex_lock(&adapter->mdev_lock);
> +	adapter->mdev_count++;
> +	mutex_unlock(&adapter->mdev_lock);
> +
> +	return 0;
> +}
> +
> +static struct mdev_parent_ops ifcvf_mdev_fops = {
> +	.owner			= THIS_MODULE,
> +	.supported_type_groups	= mdev_type_groups,
> +	.mdev_attr_groups	= mdev_dev_groups,
> +	.create			= ifcvf_mdev_create,
> +	.remove			= ifcvf_mdev_remove,
> +};
> +
> +static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ifcvf_adapter *adapter;
> +	struct ifcvf_hw *vf;
> +	int ret, i;
> +
> +	adapter = kzalloc(sizeof(struct ifcvf_adapter), GFP_KERNEL);
> +	if (adapter == NULL) {
> +		ret = -ENOMEM;
> +		goto fail;
> +	}
> +
> +	mutex_init(&adapter->mdev_lock);
> +	adapter->mdev_count = 1;


So this is per VF based vDPA implementation, which seems not convenient 
for management.  Anyhow we can control the creation in PF?

Thanks


> +	adapter->dev = dev;
> +
> +	pci_set_drvdata(pdev, adapter);
> +
> +	ret = pci_enable_device(pdev);
> +	if (ret) {
> +		IFC_ERR(adapter->dev, "Failed to enable device.\n");
> +		goto free_adapter;
> +	}
> +
> +	ret = pci_request_regions(pdev, IFCVF_DRIVER_NAME);
> +	if (ret) {
> +		IFC_ERR(adapter->dev, "Failed to request MMIO region.\n");
> +		goto disable_device;
> +	}
> +
> +	pci_set_master(pdev);
> +
> +	ret = ifcvf_init_msix(adapter);
> +	if (ret) {
> +		IFC_ERR(adapter->dev, "Failed to initialize MSIX.\n");
> +		goto free_msix;
> +	}
> +
> +	vf = &adapter->vf;
> +	for (i = 0; i < IFCVF_PCI_MAX_RESOURCE; i++) {
> +		vf->mem_resource[i].phys_addr = pci_resource_start(pdev, i);
> +		vf->mem_resource[i].len = pci_resource_len(pdev, i);
> +		if (!vf->mem_resource[i].len) {
> +			vf->mem_resource[i].addr = NULL;
> +			continue;
> +		}
> +
> +		vf->mem_resource[i].addr = pci_iomap_range(pdev, i, 0,
> +				vf->mem_resource[i].len);
> +		if (!vf->mem_resource[i].addr) {
> +			IFC_ERR(adapter->dev, "Failed to map IO resource %d\n",
> +				i);
> +			return -1;
> +		}
> +	}
> +
> +	if (ifcvf_init_hw(vf, pdev) < 0)
> +		return -1;
> +
> +	ret = mdev_register_device(dev, &ifcvf_mdev_fops);
> +	if (ret) {
> +		IFC_ERR(adapter->dev,  "Failed to register mdev device\n");
> +		goto destroy_adapter;
> +	}
> +
> +	return 0;
> +
> +destroy_adapter:
> +	ifcvf_destroy_adapter(adapter);
> +free_msix:
> +	pci_free_irq_vectors(pdev);
> +	pci_release_regions(pdev);
> +disable_device:
> +	pci_disable_device(pdev);
> +free_adapter:
> +	kfree(adapter);
> +fail:
> +	return ret;
> +}
> +
> +static void ifcvf_remove(struct pci_dev *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
> +	struct ifcvf_hw *vf;
> +	int i;
> +
> +	mdev_unregister_device(dev);
> +
> +	vf = &adapter->vf;
> +	for (i = 0; i < IFCVF_PCI_MAX_RESOURCE; i++) {
> +		if (vf->mem_resource[i].addr) {
> +			pci_iounmap(pdev, vf->mem_resource[i].addr);
> +			vf->mem_resource[i].addr = NULL;
> +		}
> +	}
> +
> +	ifcvf_destroy_adapter(adapter);
> +	pci_free_irq_vectors(pdev);
> +
> +	pci_release_regions(pdev);
> +	pci_disable_device(pdev);
> +
> +	kfree(adapter);
> +}
> +
> +static struct pci_device_id ifcvf_pci_ids[] = {
> +	{ PCI_DEVICE_SUB(IFCVF_VENDOR_ID,
> +			IFCVF_DEVICE_ID,
> +			IFCVF_SUBSYS_VENDOR_ID,
> +			IFCVF_SUBSYS_DEVICE_ID) },
> +	{ 0 },
> +};
> +MODULE_DEVICE_TABLE(pci, ifcvf_pci_ids);
> +
> +static struct pci_driver ifcvf_driver = {
> +	.name     = IFCVF_DRIVER_NAME,
> +	.id_table = ifcvf_pci_ids,
> +	.probe    = ifcvf_probe,
> +	.remove   = ifcvf_remove,
> +};
> +
> +static int __init ifcvf_init_module(void)
> +{
> +	int ret;
> +
> +	ret = pci_register_driver(&ifcvf_driver);
> +	return ret;
> +}
> +
> +static void __exit ifcvf_exit_module(void)
> +{
> +	pci_unregister_driver(&ifcvf_driver);
> +}
> +
> +module_init(ifcvf_init_module);
> +module_exit(ifcvf_exit_module);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_VERSION(VERSION_STRING);
> +MODULE_AUTHOR(DRIVER_AUTHOR);

Re: [RFC 2/2] vhost: IFC VF vdpa layer

[Zhu, Lingshan]

Hello Jason,

Thanks for your comments, I am on a conference travel, I will reply next 
Monday.

Thanks,
BR
Zhu Lingshan
On 10/16/2019 6:19 PM, Jason Wang wrote:
>
> On 2019/10/16 上午9:30, Zhu Lingshan wrote:
>> This commit introduced IFC VF operations for vdpa, which complys to
>> vhost_mdev interfaces, handles IFC VF initialization,
>> configuration and removal.
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>>   drivers/vhost/ifcvf/ifcvf_main.c | 541 
>> +++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 541 insertions(+)
>>   create mode 100644 drivers/vhost/ifcvf/ifcvf_main.c
>>
>> diff --git a/drivers/vhost/ifcvf/ifcvf_main.c 
>> b/drivers/vhost/ifcvf/ifcvf_main.c
>> new file mode 100644
>> index 000000000000..c48a29969a85
>> --- /dev/null
>> +++ b/drivers/vhost/ifcvf/ifcvf_main.c
>> @@ -0,0 +1,541 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2019 Intel Corporation.
>> + */
>> +
>> +#include <linux/interrupt.h>
>> +#include <linux/module.h>
>> +#include <linux/mdev.h>
>> +#include <linux/pci.h>
>> +#include <linux/sysfs.h>
>> +
>> +#include "ifcvf_base.h"
>> +
>> +#define VERSION_STRING    "0.1"
>> +#define DRIVER_AUTHOR    "Intel Corporation"
>> +#define IFCVF_DRIVER_NAME    "ifcvf"
>> +
>> +static irqreturn_t ifcvf_intr_handler(int irq, void *arg)
>> +{
>> +    struct vring_info *vring = arg;
>> +
>> +    if (vring->cb.callback)
>> +        return vring->cb.callback(vring->cb.private);
>> +
>> +    return IRQ_HANDLED;
>> +}
>> +
>> +static u64 ifcvf_mdev_get_features(struct mdev_device *mdev)
>> +{
>> +    return IFC_SUPPORTED_FEATURES;
>
>
> I would expect this should be done by querying the hw. Or IFC VF can't 
> get any update through its firmware?
>
>
>> +}
>> +
>> +static int ifcvf_mdev_set_features(struct mdev_device *mdev, u64 
>> features)
>> +{
>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +    vf->req_features = features;
>> +
>> +    return 0;
>> +}
>> +
>> +static u64 ifcvf_mdev_get_vq_state(struct mdev_device *mdev, u16 qid)
>> +{
>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +    return vf->vring[qid].last_avail_idx;
>
>
> Does this really work? I'd expect it should be fetched from hw since 
> it's an internal state.
>
>
>> +}
>> +
>> +static int ifcvf_mdev_set_vq_state(struct mdev_device *mdev, u16 
>> qid, u64 num)
>> +{
>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +    vf->vring[qid].last_used_idx = num;
>
>
> I fail to understand why last_used_idx is needed. It looks to me the 
> used idx in the used ring is sufficient.
>
>
>> +    vf->vring[qid].last_avail_idx = num;
>
>
> Do we need a synchronization with hw immediately here?
>
>
>> +
>> +    return 0;
>> +}
>> +
>> +static int ifcvf_mdev_set_vq_address(struct mdev_device *mdev, u16 idx,
>> +                     u64 desc_area, u64 driver_area,
>> +                     u64 device_area)
>> +{
>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +    vf->vring[idx].desc = desc_area;
>> +    vf->vring[idx].avail = driver_area;
>> +    vf->vring[idx].used = device_area;
>> +
>> +    return 0;
>> +}
>> +
>> +static void ifcvf_mdev_set_vq_num(struct mdev_device *mdev, u16 qid, 
>> u32 num)
>> +{
>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +    vf->vring[qid].size = num;
>> +}
>> +
>> +static void ifcvf_mdev_set_vq_ready(struct mdev_device *mdev,
>> +                u16 qid, bool ready)
>> +{
>> +
>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +    vf->vring[qid].ready = ready;
>> +}
>> +
>> +static bool ifcvf_mdev_get_vq_ready(struct mdev_device *mdev, u16 qid)
>> +{
>> +
>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +    return vf->vring[qid].ready;
>> +}
>> +
>> +static void ifcvf_mdev_set_vq_cb(struct mdev_device *mdev, u16 idx,
>> +                 struct virtio_mdev_callback *cb)
>> +{
>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +    vf->vring[idx].cb = *cb;
>> +}
>> +
>> +static void ifcvf_mdev_kick_vq(struct mdev_device *mdev, u16 idx)
>> +{
>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +    ifcvf_notify_queue(vf, idx);
>> +}
>> +
>> +static u8 ifcvf_mdev_get_status(struct mdev_device *mdev)
>> +{
>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +    return vf->status;
>> +}
>> +
>> +static u32 ifcvf_mdev_get_generation(struct mdev_device *mdev)
>> +{
>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +    return vf->generation;
>> +}
>> +
>> +static int ifcvf_mdev_get_version(struct mdev_device *mdev)
>> +{
>> +    return VIRTIO_MDEV_VERSION;
>> +}
>> +
>> +static u32 ifcvf_mdev_get_device_id(struct mdev_device *mdev)
>> +{
>> +    return IFCVF_DEVICE_ID;
>> +}
>> +
>> +static u32 ifcvf_mdev_get_vendor_id(struct mdev_device *mdev)
>> +{
>> +    return IFCVF_VENDOR_ID;
>> +}
>> +
>> +static u16 ifcvf_mdev_get_vq_align(struct mdev_device *mdev)
>> +{
>> +    return IFCVF_QUEUE_ALIGNMENT;
>> +}
>> +
>> +static int ifcvf_start_datapath(void *private)
>> +{
>> +    int i, ret;
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(private);
>> +
>> +    for (i = 0; i < (IFCVF_MAX_QUEUE_PAIRS * 2); i++) {
>> +        if (!vf->vring[i].ready)
>> +            break;
>
>
> Looks like error should be returned here?
>
>
>> +
>> +        if (!vf->vring[i].size)
>> +            break;
>> +
>> +        if (!vf->vring[i].desc || !vf->vring[i].avail ||
>> +            !vf->vring[i].used)
>> +            break;
>> +    }
>> +    vf->nr_vring = i;
>> +
>> +    ret = ifcvf_start_hw(vf);
>> +    return ret;
>> +}
>> +
>> +static int ifcvf_stop_datapath(void *private)
>> +{
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(private);
>> +    int i;
>> +
>> +    for (i = 0; i < IFCVF_MAX_QUEUES; i++)
>> +        vf->vring[i].cb.callback = NULL;
>
>
> Any synchronization is needed for the vq irq handler?
>
>
>> +
>> +    ifcvf_stop_hw(vf);
>> +
>> +    return 0;
>> +}
>> +
>> +static void ifcvf_reset_vring(struct ifcvf_adapter *adapter)
>> +{
>> +    int i;
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +    for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
>> +        vf->vring[i].last_used_idx = 0;
>> +        vf->vring[i].last_avail_idx = 0;
>> +        vf->vring[i].desc = 0;
>> +        vf->vring[i].avail = 0;
>> +        vf->vring[i].used = 0;
>> +        vf->vring[i].ready = 0;
>> +        vf->vring->cb.callback = NULL;
>> +        vf->vring->cb.private = NULL;
>> +    }
>> +}
>> +
>> +static void ifcvf_mdev_set_status(struct mdev_device *mdev, u8 status)
>> +{
>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +    vf->status = status;
>> +
>> +    if (status == 0) {
>> +        ifcvf_stop_datapath(adapter);
>> +        ifcvf_reset_vring(adapter);
>> +        return;
>> +    }
>> +
>> +    if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
>> +        ifcvf_start_datapath(adapter);
>> +        return;
>> +    }
>> +}
>> +
>> +static u16 ifcvf_mdev_get_queue_max(struct mdev_device *mdev)
>> +{
>> +    return IFCVF_MAX_QUEUES;
>
>
> The name is confusing, it was used to return the maximum queue size. 
> In new version of virtio-mdev, the callback was renamed as 
> get_vq_num_max().
>
>
>> +}
>> +
>> +static struct virtio_mdev_device_ops ifc_mdev_ops = {
>> +    .get_features  = ifcvf_mdev_get_features,
>> +    .set_features  = ifcvf_mdev_set_features,
>> +    .get_status    = ifcvf_mdev_get_status,
>> +    .set_status    = ifcvf_mdev_set_status,
>> +    .get_queue_max = ifcvf_mdev_get_queue_max,
>> +    .get_vq_state   = ifcvf_mdev_get_vq_state,
>> +    .set_vq_state   = ifcvf_mdev_set_vq_state,
>> +    .set_vq_cb      = ifcvf_mdev_set_vq_cb,
>> +    .set_vq_ready   = ifcvf_mdev_set_vq_ready,
>> +    .get_vq_ready    = ifcvf_mdev_get_vq_ready,
>> +    .set_vq_num     = ifcvf_mdev_set_vq_num,
>> +    .set_vq_address = ifcvf_mdev_set_vq_address,
>> +    .kick_vq        = ifcvf_mdev_kick_vq,
>> +    .get_generation    = ifcvf_mdev_get_generation,
>> +    .get_version    = ifcvf_mdev_get_version,
>> +    .get_device_id    = ifcvf_mdev_get_device_id,
>> +    .get_vendor_id    = ifcvf_mdev_get_vendor_id,
>> +    .get_vq_align    = ifcvf_mdev_get_vq_align,
>> +};
>
>
> set_config/get_config is missing. It looks to me they are not hard, 
> just implementing the access to dev_cfg. It's key to make kernel 
> virtio driver to work.
>
> And in the new version of virito-mdev, features like _F_LOG_ALL should 
> be advertised through get_mdev_features.
>
>
>> +
>> +static int ifcvf_init_msix(struct ifcvf_adapter *adapter)
>> +{
>> +    int vector, i, ret, irq;
>> +    struct pci_dev *pdev = to_pci_dev(adapter->dev);
>> +    struct ifcvf_hw *vf = &adapter->vf;
>> +
>> +    ret = pci_alloc_irq_vectors(pdev, IFCVF_MAX_INTR,
>> +            IFCVF_MAX_INTR, PCI_IRQ_MSIX);
>> +    if (ret < 0) {
>> +        IFC_ERR(adapter->dev, "Failed to alloc irq vectors.\n");
>> +        return ret;
>> +    }
>> +
>> +    for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
>> +        vector = i + IFCVF_MSI_QUEUE_OFF;
>> +        irq = pci_irq_vector(pdev, vector);
>> +        ret = request_irq(irq, ifcvf_intr_handler, 0,
>> +                pci_name(pdev), &vf->vring[i]);
>> +        if (ret) {
>> +            IFC_ERR(adapter->dev,
>> +                "Failed to request irq for vq %d.\n", i);
>> +            return ret;
>> +        }
>> +    }
>
>
> Do we need to provide fallback when we can't do per vq MSIX?
>
>
>> +
>> +    return 0;
>> +}
>> +
>> +static void ifcvf_destroy_adapter(struct ifcvf_adapter *adapter)
>> +{
>> +    int i, vector, irq;
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +    struct pci_dev *pdev = to_pci_dev(adapter->dev);
>> +
>> +    for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
>> +        vector = i + IFCVF_MSI_QUEUE_OFF;
>> +        irq = pci_irq_vector(pdev, vector);
>> +        free_irq(irq, &vf->vring[i]);
>> +    }
>> +}
>> +
>> +static ssize_t name_show(struct kobject *kobj, struct device *dev, 
>> char *buf)
>> +{
>> +    const char *name = "vhost accelerator (virtio ring compatible)";
>> +
>> +    return sprintf(buf, "%s\n", name);
>> +}
>> +MDEV_TYPE_ATTR_RO(name);
>> +
>> +static ssize_t device_api_show(struct kobject *kobj, struct device 
>> *dev,
>> +                   char *buf)
>> +{
>> +    return sprintf(buf, "%s\n", VIRTIO_MDEV_DEVICE_API_STRING);
>> +}
>> +MDEV_TYPE_ATTR_RO(device_api);
>> +
>> +static ssize_t available_instances_show(struct kobject *kobj,
>> +                    struct device *dev, char *buf)
>> +{
>> +    struct pci_dev *pdev = to_pci_dev(dev);
>> +    struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
>> +
>> +    return sprintf(buf, "%d\n", adapter->mdev_count);
>> +}
>> +
>> +MDEV_TYPE_ATTR_RO(available_instances);
>> +
>> +static ssize_t type_show(struct kobject *kobj,
>> +            struct device *dev, char *buf)
>> +{
>> +    return sprintf(buf, "%s\n", "net");
>> +}
>> +
>> +MDEV_TYPE_ATTR_RO(type);
>> +
>> +
>> +static struct attribute *mdev_types_attrs[] = {
>> +    &mdev_type_attr_name.attr,
>> +    &mdev_type_attr_device_api.attr,
>> +    &mdev_type_attr_available_instances.attr,
>> +    &mdev_type_attr_type.attr,
>> +    NULL,
>> +};
>> +
>> +static struct attribute_group mdev_type_group = {
>> +    .name  = "vdpa_virtio",
>
>
> To be consistent, it should be "vhost" or "virtio".
>
>
>> +    .attrs = mdev_types_attrs,
>> +};
>> +
>> +static struct attribute_group *mdev_type_groups[] = {
>> +    &mdev_type_group,
>> +    NULL,
>> +};
>> +
>> +const struct attribute_group *mdev_dev_groups[] = {
>> +    NULL,
>> +};
>> +
>> +static int ifcvf_mdev_create(struct kobject *kobj, struct 
>> mdev_device *mdev)
>> +{
>> +    struct device *dev = mdev_parent_dev(mdev);
>> +    struct pci_dev *pdev = to_pci_dev(dev);
>> +    struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
>> +    int ret = 0;
>> +
>> +    mutex_lock(&adapter->mdev_lock);
>> +
>> +    if (adapter->mdev_count < 1) {
>> +        ret = -EINVAL;
>> +        goto out;
>> +    }
>> +
>> +    mdev_set_class_id(mdev, MDEV_ID_VHOST);
>> +    mdev_set_dev_ops(mdev, &ifc_mdev_ops);
>> +
>> +    mdev_set_drvdata(mdev, adapter);
>> +    mdev_set_iommu_device(mdev_dev(mdev), dev);
>> +
>> +    INIT_LIST_HEAD(&adapter->dma_maps);
>> +    adapter->mdev_count--;
>> +
>> +out:
>> +    mutex_unlock(&adapter->mdev_lock);
>> +    return ret;
>> +}
>> +
>> +static int ifcvf_mdev_remove(struct mdev_device *mdev)
>> +{
>> +    struct device *dev = mdev_parent_dev(mdev);
>> +    struct pci_dev *pdev = to_pci_dev(dev);
>> +    struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
>> +
>> +    mutex_lock(&adapter->mdev_lock);
>> +    adapter->mdev_count++;
>> +    mutex_unlock(&adapter->mdev_lock);
>> +
>> +    return 0;
>> +}
>> +
>> +static struct mdev_parent_ops ifcvf_mdev_fops = {
>> +    .owner            = THIS_MODULE,
>> +    .supported_type_groups    = mdev_type_groups,
>> +    .mdev_attr_groups    = mdev_dev_groups,
>> +    .create            = ifcvf_mdev_create,
>> +    .remove            = ifcvf_mdev_remove,
>> +};
>> +
>> +static int ifcvf_probe(struct pci_dev *pdev, const struct 
>> pci_device_id *id)
>> +{
>> +    struct device *dev = &pdev->dev;
>> +    struct ifcvf_adapter *adapter;
>> +    struct ifcvf_hw *vf;
>> +    int ret, i;
>> +
>> +    adapter = kzalloc(sizeof(struct ifcvf_adapter), GFP_KERNEL);
>> +    if (adapter == NULL) {
>> +        ret = -ENOMEM;
>> +        goto fail;
>> +    }
>> +
>> +    mutex_init(&adapter->mdev_lock);
>> +    adapter->mdev_count = 1;
>
>
> So this is per VF based vDPA implementation, which seems not 
> convenient for management.  Anyhow we can control the creation in PF?
>
> Thanks
>
>
>> +    adapter->dev = dev;
>> +
>> +    pci_set_drvdata(pdev, adapter);
>> +
>> +    ret = pci_enable_device(pdev);
>> +    if (ret) {
>> +        IFC_ERR(adapter->dev, "Failed to enable device.\n");
>> +        goto free_adapter;
>> +    }
>> +
>> +    ret = pci_request_regions(pdev, IFCVF_DRIVER_NAME);
>> +    if (ret) {
>> +        IFC_ERR(adapter->dev, "Failed to request MMIO region.\n");
>> +        goto disable_device;
>> +    }
>> +
>> +    pci_set_master(pdev);
>> +
>> +    ret = ifcvf_init_msix(adapter);
>> +    if (ret) {
>> +        IFC_ERR(adapter->dev, "Failed to initialize MSIX.\n");
>> +        goto free_msix;
>> +    }
>> +
>> +    vf = &adapter->vf;
>> +    for (i = 0; i < IFCVF_PCI_MAX_RESOURCE; i++) {
>> +        vf->mem_resource[i].phys_addr = pci_resource_start(pdev, i);
>> +        vf->mem_resource[i].len = pci_resource_len(pdev, i);
>> +        if (!vf->mem_resource[i].len) {
>> +            vf->mem_resource[i].addr = NULL;
>> +            continue;
>> +        }
>> +
>> +        vf->mem_resource[i].addr = pci_iomap_range(pdev, i, 0,
>> +                vf->mem_resource[i].len);
>> +        if (!vf->mem_resource[i].addr) {
>> +            IFC_ERR(adapter->dev, "Failed to map IO resource %d\n",
>> +                i);
>> +            return -1;
>> +        }
>> +    }
>> +
>> +    if (ifcvf_init_hw(vf, pdev) < 0)
>> +        return -1;
>> +
>> +    ret = mdev_register_device(dev, &ifcvf_mdev_fops);
>> +    if (ret) {
>> +        IFC_ERR(adapter->dev,  "Failed to register mdev device\n");
>> +        goto destroy_adapter;
>> +    }
>> +
>> +    return 0;
>> +
>> +destroy_adapter:
>> +    ifcvf_destroy_adapter(adapter);
>> +free_msix:
>> +    pci_free_irq_vectors(pdev);
>> +    pci_release_regions(pdev);
>> +disable_device:
>> +    pci_disable_device(pdev);
>> +free_adapter:
>> +    kfree(adapter);
>> +fail:
>> +    return ret;
>> +}
>> +
>> +static void ifcvf_remove(struct pci_dev *pdev)
>> +{
>> +    struct device *dev = &pdev->dev;
>> +    struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
>> +    struct ifcvf_hw *vf;
>> +    int i;
>> +
>> +    mdev_unregister_device(dev);
>> +
>> +    vf = &adapter->vf;
>> +    for (i = 0; i < IFCVF_PCI_MAX_RESOURCE; i++) {
>> +        if (vf->mem_resource[i].addr) {
>> +            pci_iounmap(pdev, vf->mem_resource[i].addr);
>> +            vf->mem_resource[i].addr = NULL;
>> +        }
>> +    }
>> +
>> +    ifcvf_destroy_adapter(adapter);
>> +    pci_free_irq_vectors(pdev);
>> +
>> +    pci_release_regions(pdev);
>> +    pci_disable_device(pdev);
>> +
>> +    kfree(adapter);
>> +}
>> +
>> +static struct pci_device_id ifcvf_pci_ids[] = {
>> +    { PCI_DEVICE_SUB(IFCVF_VENDOR_ID,
>> +            IFCVF_DEVICE_ID,
>> +            IFCVF_SUBSYS_VENDOR_ID,
>> +            IFCVF_SUBSYS_DEVICE_ID) },
>> +    { 0 },
>> +};
>> +MODULE_DEVICE_TABLE(pci, ifcvf_pci_ids);
>> +
>> +static struct pci_driver ifcvf_driver = {
>> +    .name     = IFCVF_DRIVER_NAME,
>> +    .id_table = ifcvf_pci_ids,
>> +    .probe    = ifcvf_probe,
>> +    .remove   = ifcvf_remove,
>> +};
>> +
>> +static int __init ifcvf_init_module(void)
>> +{
>> +    int ret;
>> +
>> +    ret = pci_register_driver(&ifcvf_driver);
>> +    return ret;
>> +}
>> +
>> +static void __exit ifcvf_exit_module(void)
>> +{
>> +    pci_unregister_driver(&ifcvf_driver);
>> +}
>> +
>> +module_init(ifcvf_init_module);
>> +module_exit(ifcvf_exit_module);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_VERSION(VERSION_STRING);
>> +MODULE_AUTHOR(DRIVER_AUTHOR);

Re: [RFC 2/2] vhost: IFC VF vdpa layer

[Zhu, Lingshan]

On 10/16/2019 6:19 PM, Jason Wang wrote:
>
> On 2019/10/16 上午9:30, Zhu Lingshan wrote:
>> This commit introduced IFC VF operations for vdpa, which complys to
>> vhost_mdev interfaces, handles IFC VF initialization,
>> configuration and removal.
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>>   drivers/vhost/ifcvf/ifcvf_main.c | 541 
>> +++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 541 insertions(+)
>>   create mode 100644 drivers/vhost/ifcvf/ifcvf_main.c
>>
>> diff --git a/drivers/vhost/ifcvf/ifcvf_main.c 
>> b/drivers/vhost/ifcvf/ifcvf_main.c
>> new file mode 100644
>> index 000000000000..c48a29969a85
>> --- /dev/null
>> +++ b/drivers/vhost/ifcvf/ifcvf_main.c
>> @@ -0,0 +1,541 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2019 Intel Corporation.
>> + */
>> +
>> +#include <linux/interrupt.h>
>> +#include <linux/module.h>
>> +#include <linux/mdev.h>
>> +#include <linux/pci.h>
>> +#include <linux/sysfs.h>
>> +
>> +#include "ifcvf_base.h"
>> +
>> +#define VERSION_STRING    "0.1"
>> +#define DRIVER_AUTHOR    "Intel Corporation"
>> +#define IFCVF_DRIVER_NAME    "ifcvf"
>> +
>> +static irqreturn_t ifcvf_intr_handler(int irq, void *arg)
>> +{
>> +    struct vring_info *vring = arg;
>> +
>> +    if (vring->cb.callback)
>> +        return vring->cb.callback(vring->cb.private);
>> +
>> +    return IRQ_HANDLED;
>> +}
>> +
>> +static u64 ifcvf_mdev_get_features(struct mdev_device *mdev)
>> +{
>> +    return IFC_SUPPORTED_FEATURES;
>
>
> I would expect this should be done by querying the hw. Or IFC VF can't 
> get any update through its firmware?

Hi Jason,

Thanks for your comments, for now driver just support these features.

>
>
>> +}
>> +
>> +static int ifcvf_mdev_set_features(struct mdev_device *mdev, u64 
>> features)
>> +{
>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +    vf->req_features = features;
>> +
>> +    return 0;
>> +}
>> +
>> +static u64 ifcvf_mdev_get_vq_state(struct mdev_device *mdev, u16 qid)
>> +{
>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +    return vf->vring[qid].last_avail_idx;
>
>
> Does this really work? I'd expect it should be fetched from hw since 
> it's an internal state.
for now, it's working, we intend to support LM in next version drivers.
>
>
>> +}
>> +
>> +static int ifcvf_mdev_set_vq_state(struct mdev_device *mdev, u16 
>> qid, u64 num)
>> +{
>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +    vf->vring[qid].last_used_idx = num;
>
>
> I fail to understand why last_used_idx is needed. It looks to me the 
> used idx in the used ring is sufficient.
I will remove it.
>
>
>> +    vf->vring[qid].last_avail_idx = num;
>
>
> Do we need a synchronization with hw immediately here?
>
>
>> +
>> +    return 0;
>> +}
>> +
>> +static int ifcvf_mdev_set_vq_address(struct mdev_device *mdev, u16 idx,
>> +                     u64 desc_area, u64 driver_area,
>> +                     u64 device_area)
>> +{
>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +    vf->vring[idx].desc = desc_area;
>> +    vf->vring[idx].avail = driver_area;
>> +    vf->vring[idx].used = device_area;
>> +
>> +    return 0;
>> +}
>> +
>> +static void ifcvf_mdev_set_vq_num(struct mdev_device *mdev, u16 qid, 
>> u32 num)
>> +{
>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +    vf->vring[qid].size = num;
>> +}
>> +
>> +static void ifcvf_mdev_set_vq_ready(struct mdev_device *mdev,
>> +                u16 qid, bool ready)
>> +{
>> +
>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +    vf->vring[qid].ready = ready;
>> +}
>> +
>> +static bool ifcvf_mdev_get_vq_ready(struct mdev_device *mdev, u16 qid)
>> +{
>> +
>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +    return vf->vring[qid].ready;
>> +}
>> +
>> +static void ifcvf_mdev_set_vq_cb(struct mdev_device *mdev, u16 idx,
>> +                 struct virtio_mdev_callback *cb)
>> +{
>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +    vf->vring[idx].cb = *cb;
>> +}
>> +
>> +static void ifcvf_mdev_kick_vq(struct mdev_device *mdev, u16 idx)
>> +{
>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +    ifcvf_notify_queue(vf, idx);
>> +}
>> +
>> +static u8 ifcvf_mdev_get_status(struct mdev_device *mdev)
>> +{
>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +    return vf->status;
>> +}
>> +
>> +static u32 ifcvf_mdev_get_generation(struct mdev_device *mdev)
>> +{
>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +    return vf->generation;
>> +}
>> +
>> +static int ifcvf_mdev_get_version(struct mdev_device *mdev)
>> +{
>> +    return VIRTIO_MDEV_VERSION;
>> +}
>> +
>> +static u32 ifcvf_mdev_get_device_id(struct mdev_device *mdev)
>> +{
>> +    return IFCVF_DEVICE_ID;
>> +}
>> +
>> +static u32 ifcvf_mdev_get_vendor_id(struct mdev_device *mdev)
>> +{
>> +    return IFCVF_VENDOR_ID;
>> +}
>> +
>> +static u16 ifcvf_mdev_get_vq_align(struct mdev_device *mdev)
>> +{
>> +    return IFCVF_QUEUE_ALIGNMENT;
>> +}
>> +
>> +static int ifcvf_start_datapath(void *private)
>> +{
>> +    int i, ret;
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(private);
>> +
>> +    for (i = 0; i < (IFCVF_MAX_QUEUE_PAIRS * 2); i++) {
>> +        if (!vf->vring[i].ready)
>> +            break;
>
>
> Looks like error should be returned here?
agreed!
>
>
>> +
>> +        if (!vf->vring[i].size)
>> +            break;
>> +
>> +        if (!vf->vring[i].desc || !vf->vring[i].avail ||
>> +            !vf->vring[i].used)
>> +            break;
>> +    }
>> +    vf->nr_vring = i;
>> +
>> +    ret = ifcvf_start_hw(vf);
>> +    return ret;
>> +}
>> +
>> +static int ifcvf_stop_datapath(void *private)
>> +{
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(private);
>> +    int i;
>> +
>> +    for (i = 0; i < IFCVF_MAX_QUEUES; i++)
>> +        vf->vring[i].cb.callback = NULL;
>
>
> Any synchronization is needed for the vq irq handler?
I think even we set callback = NULL, the code is still there, on-going 
routines would not be effected.
>
>
>> +
>> +    ifcvf_stop_hw(vf);
>> +
>> +    return 0;
>> +}
>> +
>> +static void ifcvf_reset_vring(struct ifcvf_adapter *adapter)
>> +{
>> +    int i;
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +    for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
>> +        vf->vring[i].last_used_idx = 0;
>> +        vf->vring[i].last_avail_idx = 0;
>> +        vf->vring[i].desc = 0;
>> +        vf->vring[i].avail = 0;
>> +        vf->vring[i].used = 0;
>> +        vf->vring[i].ready = 0;
>> +        vf->vring->cb.callback = NULL;
>> +        vf->vring->cb.private = NULL;
>> +    }
>> +}
>> +
>> +static void ifcvf_mdev_set_status(struct mdev_device *mdev, u8 status)
>> +{
>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +
>> +    vf->status = status;
>> +
>> +    if (status == 0) {
>> +        ifcvf_stop_datapath(adapter);
>> +        ifcvf_reset_vring(adapter);
>> +        return;
>> +    }
>> +
>> +    if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
>> +        ifcvf_start_datapath(adapter);
>> +        return;
>> +    }
>> +}
>> +
>> +static u16 ifcvf_mdev_get_queue_max(struct mdev_device *mdev)
>> +{
>> +    return IFCVF_MAX_QUEUES;
>
>
> The name is confusing, it was used to return the maximum queue size. 
> In new version of virtio-mdev, the callback was renamed as 
> get_vq_num_max().
will change that.
>
>
>> +}
>> +
>> +static struct virtio_mdev_device_ops ifc_mdev_ops = {
>> +    .get_features  = ifcvf_mdev_get_features,
>> +    .set_features  = ifcvf_mdev_set_features,
>> +    .get_status    = ifcvf_mdev_get_status,
>> +    .set_status    = ifcvf_mdev_set_status,
>> +    .get_queue_max = ifcvf_mdev_get_queue_max,
>> +    .get_vq_state   = ifcvf_mdev_get_vq_state,
>> +    .set_vq_state   = ifcvf_mdev_set_vq_state,
>> +    .set_vq_cb      = ifcvf_mdev_set_vq_cb,
>> +    .set_vq_ready   = ifcvf_mdev_set_vq_ready,
>> +    .get_vq_ready    = ifcvf_mdev_get_vq_ready,
>> +    .set_vq_num     = ifcvf_mdev_set_vq_num,
>> +    .set_vq_address = ifcvf_mdev_set_vq_address,
>> +    .kick_vq        = ifcvf_mdev_kick_vq,
>> +    .get_generation    = ifcvf_mdev_get_generation,
>> +    .get_version    = ifcvf_mdev_get_version,
>> +    .get_device_id    = ifcvf_mdev_get_device_id,
>> +    .get_vendor_id    = ifcvf_mdev_get_vendor_id,
>> +    .get_vq_align    = ifcvf_mdev_get_vq_align,
>> +};
>
>
> set_config/get_config is missing. It looks to me they are not hard, 
> just implementing the access to dev_cfg. It's key to make kernel 
> virtio driver to work.
>
> And in the new version of virito-mdev, features like _F_LOG_ALL should 
> be advertised through get_mdev_features.
IMHO, currently the driver can work without set/get_config, vhost_mdev 
doesn't call them for now.
>
>
>> +
>> +static int ifcvf_init_msix(struct ifcvf_adapter *adapter)
>> +{
>> +    int vector, i, ret, irq;
>> +    struct pci_dev *pdev = to_pci_dev(adapter->dev);
>> +    struct ifcvf_hw *vf = &adapter->vf;
>> +
>> +    ret = pci_alloc_irq_vectors(pdev, IFCVF_MAX_INTR,
>> +            IFCVF_MAX_INTR, PCI_IRQ_MSIX);
>> +    if (ret < 0) {
>> +        IFC_ERR(adapter->dev, "Failed to alloc irq vectors.\n");
>> +        return ret;
>> +    }
>> +
>> +    for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
>> +        vector = i + IFCVF_MSI_QUEUE_OFF;
>> +        irq = pci_irq_vector(pdev, vector);
>> +        ret = request_irq(irq, ifcvf_intr_handler, 0,
>> +                pci_name(pdev), &vf->vring[i]);
>> +        if (ret) {
>> +            IFC_ERR(adapter->dev,
>> +                "Failed to request irq for vq %d.\n", i);
>> +            return ret;
>> +        }
>> +    }
>
>
> Do we need to provide fallback when we can't do per vq MSIX?
I think it would be very rarely that can not get enough vectors.
>
>
>> +
>> +    return 0;
>> +}
>> +
>> +static void ifcvf_destroy_adapter(struct ifcvf_adapter *adapter)
>> +{
>> +    int i, vector, irq;
>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>> +    struct pci_dev *pdev = to_pci_dev(adapter->dev);
>> +
>> +    for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
>> +        vector = i + IFCVF_MSI_QUEUE_OFF;
>> +        irq = pci_irq_vector(pdev, vector);
>> +        free_irq(irq, &vf->vring[i]);
>> +    }
>> +}
>> +
>> +static ssize_t name_show(struct kobject *kobj, struct device *dev, 
>> char *buf)
>> +{
>> +    const char *name = "vhost accelerator (virtio ring compatible)";
>> +
>> +    return sprintf(buf, "%s\n", name);
>> +}
>> +MDEV_TYPE_ATTR_RO(name);
>> +
>> +static ssize_t device_api_show(struct kobject *kobj, struct device 
>> *dev,
>> +                   char *buf)
>> +{
>> +    return sprintf(buf, "%s\n", VIRTIO_MDEV_DEVICE_API_STRING);
>> +}
>> +MDEV_TYPE_ATTR_RO(device_api);
>> +
>> +static ssize_t available_instances_show(struct kobject *kobj,
>> +                    struct device *dev, char *buf)
>> +{
>> +    struct pci_dev *pdev = to_pci_dev(dev);
>> +    struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
>> +
>> +    return sprintf(buf, "%d\n", adapter->mdev_count);
>> +}
>> +
>> +MDEV_TYPE_ATTR_RO(available_instances);
>> +
>> +static ssize_t type_show(struct kobject *kobj,
>> +            struct device *dev, char *buf)
>> +{
>> +    return sprintf(buf, "%s\n", "net");
>> +}
>> +
>> +MDEV_TYPE_ATTR_RO(type);
>> +
>> +
>> +static struct attribute *mdev_types_attrs[] = {
>> +    &mdev_type_attr_name.attr,
>> +    &mdev_type_attr_device_api.attr,
>> +    &mdev_type_attr_available_instances.attr,
>> +    &mdev_type_attr_type.attr,
>> +    NULL,
>> +};
>> +
>> +static struct attribute_group mdev_type_group = {
>> +    .name  = "vdpa_virtio",
>
>
> To be consistent, it should be "vhost" or "virtio".
agreed!
>
>
>> +    .attrs = mdev_types_attrs,
>> +};
>> +
>> +static struct attribute_group *mdev_type_groups[] = {
>> +    &mdev_type_group,
>> +    NULL,
>> +};
>> +
>> +const struct attribute_group *mdev_dev_groups[] = {
>> +    NULL,
>> +};
>> +
>> +static int ifcvf_mdev_create(struct kobject *kobj, struct 
>> mdev_device *mdev)
>> +{
>> +    struct device *dev = mdev_parent_dev(mdev);
>> +    struct pci_dev *pdev = to_pci_dev(dev);
>> +    struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
>> +    int ret = 0;
>> +
>> +    mutex_lock(&adapter->mdev_lock);
>> +
>> +    if (adapter->mdev_count < 1) {
>> +        ret = -EINVAL;
>> +        goto out;
>> +    }
>> +
>> +    mdev_set_class_id(mdev, MDEV_ID_VHOST);
>> +    mdev_set_dev_ops(mdev, &ifc_mdev_ops);
>> +
>> +    mdev_set_drvdata(mdev, adapter);
>> +    mdev_set_iommu_device(mdev_dev(mdev), dev);
>> +
>> +    INIT_LIST_HEAD(&adapter->dma_maps);
>> +    adapter->mdev_count--;
>> +
>> +out:
>> +    mutex_unlock(&adapter->mdev_lock);
>> +    return ret;
>> +}
>> +
>> +static int ifcvf_mdev_remove(struct mdev_device *mdev)
>> +{
>> +    struct device *dev = mdev_parent_dev(mdev);
>> +    struct pci_dev *pdev = to_pci_dev(dev);
>> +    struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
>> +
>> +    mutex_lock(&adapter->mdev_lock);
>> +    adapter->mdev_count++;
>> +    mutex_unlock(&adapter->mdev_lock);
>> +
>> +    return 0;
>> +}
>> +
>> +static struct mdev_parent_ops ifcvf_mdev_fops = {
>> +    .owner            = THIS_MODULE,
>> +    .supported_type_groups    = mdev_type_groups,
>> +    .mdev_attr_groups    = mdev_dev_groups,
>> +    .create            = ifcvf_mdev_create,
>> +    .remove            = ifcvf_mdev_remove,
>> +};
>> +
>> +static int ifcvf_probe(struct pci_dev *pdev, const struct 
>> pci_device_id *id)
>> +{
>> +    struct device *dev = &pdev->dev;
>> +    struct ifcvf_adapter *adapter;
>> +    struct ifcvf_hw *vf;
>> +    int ret, i;
>> +
>> +    adapter = kzalloc(sizeof(struct ifcvf_adapter), GFP_KERNEL);
>> +    if (adapter == NULL) {
>> +        ret = -ENOMEM;
>> +        goto fail;
>> +    }
>> +
>> +    mutex_init(&adapter->mdev_lock);
>> +    adapter->mdev_count = 1;
>
>
> So this is per VF based vDPA implementation, which seems not 
> convenient for management.  Anyhow we can control the creation in PF?
>
> Thanks
the driver scope for now doesn't support that, we can add these feature 
in next releases.
>
>
>> +    adapter->dev = dev;
>> +
>> +    pci_set_drvdata(pdev, adapter);
>> +
>> +    ret = pci_enable_device(pdev);
>> +    if (ret) {
>> +        IFC_ERR(adapter->dev, "Failed to enable device.\n");
>> +        goto free_adapter;
>> +    }
>> +
>> +    ret = pci_request_regions(pdev, IFCVF_DRIVER_NAME);
>> +    if (ret) {
>> +        IFC_ERR(adapter->dev, "Failed to request MMIO region.\n");
>> +        goto disable_device;
>> +    }
>> +
>> +    pci_set_master(pdev);
>> +
>> +    ret = ifcvf_init_msix(adapter);
>> +    if (ret) {
>> +        IFC_ERR(adapter->dev, "Failed to initialize MSIX.\n");
>> +        goto free_msix;
>> +    }
>> +
>> +    vf = &adapter->vf;
>> +    for (i = 0; i < IFCVF_PCI_MAX_RESOURCE; i++) {
>> +        vf->mem_resource[i].phys_addr = pci_resource_start(pdev, i);
>> +        vf->mem_resource[i].len = pci_resource_len(pdev, i);
>> +        if (!vf->mem_resource[i].len) {
>> +            vf->mem_resource[i].addr = NULL;
>> +            continue;
>> +        }
>> +
>> +        vf->mem_resource[i].addr = pci_iomap_range(pdev, i, 0,
>> +                vf->mem_resource[i].len);
>> +        if (!vf->mem_resource[i].addr) {
>> +            IFC_ERR(adapter->dev, "Failed to map IO resource %d\n",
>> +                i);
>> +            return -1;
>> +        }
>> +    }
>> +
>> +    if (ifcvf_init_hw(vf, pdev) < 0)
>> +        return -1;
>> +
>> +    ret = mdev_register_device(dev, &ifcvf_mdev_fops);
>> +    if (ret) {
>> +        IFC_ERR(adapter->dev,  "Failed to register mdev device\n");
>> +        goto destroy_adapter;
>> +    }
>> +
>> +    return 0;
>> +
>> +destroy_adapter:
>> +    ifcvf_destroy_adapter(adapter);
>> +free_msix:
>> +    pci_free_irq_vectors(pdev);
>> +    pci_release_regions(pdev);
>> +disable_device:
>> +    pci_disable_device(pdev);
>> +free_adapter:
>> +    kfree(adapter);
>> +fail:
>> +    return ret;
>> +}
>> +
>> +static void ifcvf_remove(struct pci_dev *pdev)
>> +{
>> +    struct device *dev = &pdev->dev;
>> +    struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
>> +    struct ifcvf_hw *vf;
>> +    int i;
>> +
>> +    mdev_unregister_device(dev);
>> +
>> +    vf = &adapter->vf;
>> +    for (i = 0; i < IFCVF_PCI_MAX_RESOURCE; i++) {
>> +        if (vf->mem_resource[i].addr) {
>> +            pci_iounmap(pdev, vf->mem_resource[i].addr);
>> +            vf->mem_resource[i].addr = NULL;
>> +        }
>> +    }
>> +
>> +    ifcvf_destroy_adapter(adapter);
>> +    pci_free_irq_vectors(pdev);
>> +
>> +    pci_release_regions(pdev);
>> +    pci_disable_device(pdev);
>> +
>> +    kfree(adapter);
>> +}
>> +
>> +static struct pci_device_id ifcvf_pci_ids[] = {
>> +    { PCI_DEVICE_SUB(IFCVF_VENDOR_ID,
>> +            IFCVF_DEVICE_ID,
>> +            IFCVF_SUBSYS_VENDOR_ID,
>> +            IFCVF_SUBSYS_DEVICE_ID) },
>> +    { 0 },
>> +};
>> +MODULE_DEVICE_TABLE(pci, ifcvf_pci_ids);
>> +
>> +static struct pci_driver ifcvf_driver = {
>> +    .name     = IFCVF_DRIVER_NAME,
>> +    .id_table = ifcvf_pci_ids,
>> +    .probe    = ifcvf_probe,
>> +    .remove   = ifcvf_remove,
>> +};
>> +
>> +static int __init ifcvf_init_module(void)
>> +{
>> +    int ret;
>> +
>> +    ret = pci_register_driver(&ifcvf_driver);
>> +    return ret;
>> +}
>> +
>> +static void __exit ifcvf_exit_module(void)
>> +{
>> +    pci_unregister_driver(&ifcvf_driver);
>> +}
>> +
>> +module_init(ifcvf_init_module);
>> +module_exit(ifcvf_exit_module);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_VERSION(VERSION_STRING);
>> +MODULE_AUTHOR(DRIVER_AUTHOR);

Re: [RFC 2/2] vhost: IFC VF vdpa layer

[Jason Wang]

On 2019/10/21 下午5:53, Zhu, Lingshan wrote:
>
> On 10/16/2019 6:19 PM, Jason Wang wrote:
>>
>> On 2019/10/16 上午9:30, Zhu Lingshan wrote:
>>> This commit introduced IFC VF operations for vdpa, which complys to
>>> vhost_mdev interfaces, handles IFC VF initialization,
>>> configuration and removal.
>>>
>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>> ---
>>>   drivers/vhost/ifcvf/ifcvf_main.c | 541 
>>> +++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 541 insertions(+)
>>>   create mode 100644 drivers/vhost/ifcvf/ifcvf_main.c
>>>
>>> diff --git a/drivers/vhost/ifcvf/ifcvf_main.c 
>>> b/drivers/vhost/ifcvf/ifcvf_main.c
>>> new file mode 100644
>>> index 000000000000..c48a29969a85
>>> --- /dev/null
>>> +++ b/drivers/vhost/ifcvf/ifcvf_main.c
>>> @@ -0,0 +1,541 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Copyright (C) 2019 Intel Corporation.
>>> + */
>>> +
>>> +#include <linux/interrupt.h>
>>> +#include <linux/module.h>
>>> +#include <linux/mdev.h>
>>> +#include <linux/pci.h>
>>> +#include <linux/sysfs.h>
>>> +
>>> +#include "ifcvf_base.h"
>>> +
>>> +#define VERSION_STRING    "0.1"
>>> +#define DRIVER_AUTHOR    "Intel Corporation"
>>> +#define IFCVF_DRIVER_NAME    "ifcvf"
>>> +
>>> +static irqreturn_t ifcvf_intr_handler(int irq, void *arg)
>>> +{
>>> +    struct vring_info *vring = arg;
>>> +
>>> +    if (vring->cb.callback)
>>> +        return vring->cb.callback(vring->cb.private);
>>> +
>>> +    return IRQ_HANDLED;
>>> +}
>>> +
>>> +static u64 ifcvf_mdev_get_features(struct mdev_device *mdev)
>>> +{
>>> +    return IFC_SUPPORTED_FEATURES;
>>
>>
>> I would expect this should be done by querying the hw. Or IFC VF 
>> can't get any update through its firmware?
>
> Hi Jason,
>
> Thanks for your comments, for now driver just support these features.


Ok, it should work but less flexible, we can change it in the future.


>
>>
>>
>>> +}
>>> +
>>> +static int ifcvf_mdev_set_features(struct mdev_device *mdev, u64 
>>> features)
>>> +{
>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>> +
>>> +    vf->req_features = features;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static u64 ifcvf_mdev_get_vq_state(struct mdev_device *mdev, u16 qid)
>>> +{
>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>> +
>>> +    return vf->vring[qid].last_avail_idx;
>>
>>
>> Does this really work? I'd expect it should be fetched from hw since 
>> it's an internal state.
> for now, it's working, we intend to support LM in next version drivers.


I'm not sure I understand here, I don't see any synchronization between 
the hardware and last_avail_idx, so last_avail_idx should not change.

Btw, what did "LM" mean :) ?


>>
>>
>>> +}
>>> +
>>> +static int ifcvf_mdev_set_vq_state(struct mdev_device *mdev, u16 
>>> qid, u64 num)
>>> +{
>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>> +
>>> +    vf->vring[qid].last_used_idx = num;
>>
>>
>> I fail to understand why last_used_idx is needed. It looks to me the 
>> used idx in the used ring is sufficient.
> I will remove it.
>>
>>
>>> +    vf->vring[qid].last_avail_idx = num;
>>
>>
>> Do we need a synchronization with hw immediately here?
>>
>>
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int ifcvf_mdev_set_vq_address(struct mdev_device *mdev, u16 
>>> idx,
>>> +                     u64 desc_area, u64 driver_area,
>>> +                     u64 device_area)
>>> +{
>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>> +
>>> +    vf->vring[idx].desc = desc_area;
>>> +    vf->vring[idx].avail = driver_area;
>>> +    vf->vring[idx].used = device_area;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static void ifcvf_mdev_set_vq_num(struct mdev_device *mdev, u16 
>>> qid, u32 num)
>>> +{
>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>> +
>>> +    vf->vring[qid].size = num;
>>> +}
>>> +
>>> +static void ifcvf_mdev_set_vq_ready(struct mdev_device *mdev,
>>> +                u16 qid, bool ready)
>>> +{
>>> +
>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>> +
>>> +    vf->vring[qid].ready = ready;
>>> +}
>>> +
>>> +static bool ifcvf_mdev_get_vq_ready(struct mdev_device *mdev, u16 qid)
>>> +{
>>> +
>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>> +
>>> +    return vf->vring[qid].ready;
>>> +}
>>> +
>>> +static void ifcvf_mdev_set_vq_cb(struct mdev_device *mdev, u16 idx,
>>> +                 struct virtio_mdev_callback *cb)
>>> +{
>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>> +
>>> +    vf->vring[idx].cb = *cb;
>>> +}
>>> +
>>> +static void ifcvf_mdev_kick_vq(struct mdev_device *mdev, u16 idx)
>>> +{
>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>> +
>>> +    ifcvf_notify_queue(vf, idx);
>>> +}
>>> +
>>> +static u8 ifcvf_mdev_get_status(struct mdev_device *mdev)
>>> +{
>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>> +
>>> +    return vf->status;
>>> +}
>>> +
>>> +static u32 ifcvf_mdev_get_generation(struct mdev_device *mdev)
>>> +{
>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>> +
>>> +    return vf->generation;
>>> +}
>>> +
>>> +static int ifcvf_mdev_get_version(struct mdev_device *mdev)
>>> +{
>>> +    return VIRTIO_MDEV_VERSION;
>>> +}
>>> +
>>> +static u32 ifcvf_mdev_get_device_id(struct mdev_device *mdev)
>>> +{
>>> +    return IFCVF_DEVICE_ID;
>>> +}
>>> +
>>> +static u32 ifcvf_mdev_get_vendor_id(struct mdev_device *mdev)
>>> +{
>>> +    return IFCVF_VENDOR_ID;
>>> +}
>>> +
>>> +static u16 ifcvf_mdev_get_vq_align(struct mdev_device *mdev)
>>> +{
>>> +    return IFCVF_QUEUE_ALIGNMENT;
>>> +}
>>> +
>>> +static int ifcvf_start_datapath(void *private)
>>> +{
>>> +    int i, ret;
>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(private);
>>> +
>>> +    for (i = 0; i < (IFCVF_MAX_QUEUE_PAIRS * 2); i++) {
>>> +        if (!vf->vring[i].ready)
>>> +            break;
>>
>>
>> Looks like error should be returned here?
> agreed!
>>
>>
>>> +
>>> +        if (!vf->vring[i].size)
>>> +            break;
>>> +
>>> +        if (!vf->vring[i].desc || !vf->vring[i].avail ||
>>> +            !vf->vring[i].used)
>>> +            break;
>>> +    }
>>> +    vf->nr_vring = i;
>>> +
>>> +    ret = ifcvf_start_hw(vf);
>>> +    return ret;
>>> +}
>>> +
>>> +static int ifcvf_stop_datapath(void *private)
>>> +{
>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(private);
>>> +    int i;
>>> +
>>> +    for (i = 0; i < IFCVF_MAX_QUEUES; i++)
>>> +        vf->vring[i].cb.callback = NULL;
>>
>>
>> Any synchronization is needed for the vq irq handler?
> I think even we set callback = NULL, the code is still there, on-going 
> routines would not be effected.


Ok I think you mean when ifcvf_stop_hw() return, hardware will not 
respond to e.g kick and other events etc.


>>
>>
>>> +
>>> +    ifcvf_stop_hw(vf);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static void ifcvf_reset_vring(struct ifcvf_adapter *adapter)
>>> +{
>>> +    int i;
>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>> +
>>> +    for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
>>> +        vf->vring[i].last_used_idx = 0;
>>> +        vf->vring[i].last_avail_idx = 0;
>>> +        vf->vring[i].desc = 0;
>>> +        vf->vring[i].avail = 0;
>>> +        vf->vring[i].used = 0;
>>> +        vf->vring[i].ready = 0;
>>> +        vf->vring->cb.callback = NULL;
>>> +        vf->vring->cb.private = NULL;
>>> +    }
>>> +}
>>> +
>>> +static void ifcvf_mdev_set_status(struct mdev_device *mdev, u8 status)
>>> +{
>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>> +
>>> +    vf->status = status;
>>> +
>>> +    if (status == 0) {
>>> +        ifcvf_stop_datapath(adapter);
>>> +        ifcvf_reset_vring(adapter);
>>> +        return;
>>> +    }
>>> +
>>> +    if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
>>> +        ifcvf_start_datapath(adapter);
>>> +        return;
>>> +    }
>>> +}
>>> +
>>> +static u16 ifcvf_mdev_get_queue_max(struct mdev_device *mdev)
>>> +{
>>> +    return IFCVF_MAX_QUEUES;
>>
>>
>> The name is confusing, it was used to return the maximum queue size. 
>> In new version of virtio-mdev, the callback was renamed as 
>> get_vq_num_max().
> will change that.
>>
>>
>>> +}
>>> +
>>> +static struct virtio_mdev_device_ops ifc_mdev_ops = {
>>> +    .get_features  = ifcvf_mdev_get_features,
>>> +    .set_features  = ifcvf_mdev_set_features,
>>> +    .get_status    = ifcvf_mdev_get_status,
>>> +    .set_status    = ifcvf_mdev_set_status,
>>> +    .get_queue_max = ifcvf_mdev_get_queue_max,
>>> +    .get_vq_state   = ifcvf_mdev_get_vq_state,
>>> +    .set_vq_state   = ifcvf_mdev_set_vq_state,
>>> +    .set_vq_cb      = ifcvf_mdev_set_vq_cb,
>>> +    .set_vq_ready   = ifcvf_mdev_set_vq_ready,
>>> +    .get_vq_ready    = ifcvf_mdev_get_vq_ready,
>>> +    .set_vq_num     = ifcvf_mdev_set_vq_num,
>>> +    .set_vq_address = ifcvf_mdev_set_vq_address,
>>> +    .kick_vq        = ifcvf_mdev_kick_vq,
>>> +    .get_generation    = ifcvf_mdev_get_generation,
>>> +    .get_version    = ifcvf_mdev_get_version,
>>> +    .get_device_id    = ifcvf_mdev_get_device_id,
>>> +    .get_vendor_id    = ifcvf_mdev_get_vendor_id,
>>> +    .get_vq_align    = ifcvf_mdev_get_vq_align,
>>> +};
>>
>>
>> set_config/get_config is missing. It looks to me they are not hard, 
>> just implementing the access to dev_cfg. It's key to make kernel 
>> virtio driver to work.
>>
>> And in the new version of virito-mdev, features like _F_LOG_ALL 
>> should be advertised through get_mdev_features.
> IMHO, currently the driver can work without set/get_config, vhost_mdev 
> doesn't call them for now.


Yes, but it was required by virtio_mdev for host driver to work, and it 
looks to me it's not hard to add them. If possible please add them and 
"virtio" type then we can use the ops for both the case of VM and 
containers.


>>
>>
>>> +
>>> +static int ifcvf_init_msix(struct ifcvf_adapter *adapter)
>>> +{
>>> +    int vector, i, ret, irq;
>>> +    struct pci_dev *pdev = to_pci_dev(adapter->dev);
>>> +    struct ifcvf_hw *vf = &adapter->vf;
>>> +
>>> +    ret = pci_alloc_irq_vectors(pdev, IFCVF_MAX_INTR,
>>> +            IFCVF_MAX_INTR, PCI_IRQ_MSIX);
>>> +    if (ret < 0) {
>>> +        IFC_ERR(adapter->dev, "Failed to alloc irq vectors.\n");
>>> +        return ret;
>>> +    }
>>> +
>>> +    for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
>>> +        vector = i + IFCVF_MSI_QUEUE_OFF;
>>> +        irq = pci_irq_vector(pdev, vector);
>>> +        ret = request_irq(irq, ifcvf_intr_handler, 0,
>>> +                pci_name(pdev), &vf->vring[i]);
>>> +        if (ret) {
>>> +            IFC_ERR(adapter->dev,
>>> +                "Failed to request irq for vq %d.\n", i);
>>> +            return ret;
>>> +        }
>>> +    }
>>
>>
>> Do we need to provide fallback when we can't do per vq MSIX?
> I think it would be very rarely that can not get enough vectors.


Right.


>>
>>
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static void ifcvf_destroy_adapter(struct ifcvf_adapter *adapter)
>>> +{
>>> +    int i, vector, irq;
>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>> +    struct pci_dev *pdev = to_pci_dev(adapter->dev);
>>> +
>>> +    for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
>>> +        vector = i + IFCVF_MSI_QUEUE_OFF;
>>> +        irq = pci_irq_vector(pdev, vector);
>>> +        free_irq(irq, &vf->vring[i]);
>>> +    }
>>> +}
>>> +
>>> +static ssize_t name_show(struct kobject *kobj, struct device *dev, 
>>> char *buf)
>>> +{
>>> +    const char *name = "vhost accelerator (virtio ring compatible)";
>>> +
>>> +    return sprintf(buf, "%s\n", name);
>>> +}
>>> +MDEV_TYPE_ATTR_RO(name);
>>> +
>>> +static ssize_t device_api_show(struct kobject *kobj, struct device 
>>> *dev,
>>> +                   char *buf)
>>> +{
>>> +    return sprintf(buf, "%s\n", VIRTIO_MDEV_DEVICE_API_STRING);
>>> +}
>>> +MDEV_TYPE_ATTR_RO(device_api);
>>> +
>>> +static ssize_t available_instances_show(struct kobject *kobj,
>>> +                    struct device *dev, char *buf)
>>> +{
>>> +    struct pci_dev *pdev = to_pci_dev(dev);
>>> +    struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
>>> +
>>> +    return sprintf(buf, "%d\n", adapter->mdev_count);
>>> +}
>>> +
>>> +MDEV_TYPE_ATTR_RO(available_instances);
>>> +
>>> +static ssize_t type_show(struct kobject *kobj,
>>> +            struct device *dev, char *buf)
>>> +{
>>> +    return sprintf(buf, "%s\n", "net");
>>> +}
>>> +
>>> +MDEV_TYPE_ATTR_RO(type);
>>> +
>>> +
>>> +static struct attribute *mdev_types_attrs[] = {
>>> +    &mdev_type_attr_name.attr,
>>> +    &mdev_type_attr_device_api.attr,
>>> +    &mdev_type_attr_available_instances.attr,
>>> +    &mdev_type_attr_type.attr,
>>> +    NULL,
>>> +};
>>> +
>>> +static struct attribute_group mdev_type_group = {
>>> +    .name  = "vdpa_virtio",
>>
>>
>> To be consistent, it should be "vhost" or "virtio".
> agreed!
>>
>>
>>> +    .attrs = mdev_types_attrs,
>>> +};
>>> +
>>> +static struct attribute_group *mdev_type_groups[] = {
>>> +    &mdev_type_group,
>>> +    NULL,
>>> +};
>>> +
>>> +const struct attribute_group *mdev_dev_groups[] = {
>>> +    NULL,
>>> +};
>>> +
>>> +static int ifcvf_mdev_create(struct kobject *kobj, struct 
>>> mdev_device *mdev)
>>> +{
>>> +    struct device *dev = mdev_parent_dev(mdev);
>>> +    struct pci_dev *pdev = to_pci_dev(dev);
>>> +    struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
>>> +    int ret = 0;
>>> +
>>> +    mutex_lock(&adapter->mdev_lock);
>>> +
>>> +    if (adapter->mdev_count < 1) {
>>> +        ret = -EINVAL;
>>> +        goto out;
>>> +    }
>>> +
>>> +    mdev_set_class_id(mdev, MDEV_ID_VHOST);
>>> +    mdev_set_dev_ops(mdev, &ifc_mdev_ops);
>>> +
>>> +    mdev_set_drvdata(mdev, adapter);
>>> +    mdev_set_iommu_device(mdev_dev(mdev), dev);
>>> +
>>> +    INIT_LIST_HEAD(&adapter->dma_maps);
>>> +    adapter->mdev_count--;
>>> +
>>> +out:
>>> +    mutex_unlock(&adapter->mdev_lock);
>>> +    return ret;
>>> +}
>>> +
>>> +static int ifcvf_mdev_remove(struct mdev_device *mdev)
>>> +{
>>> +    struct device *dev = mdev_parent_dev(mdev);
>>> +    struct pci_dev *pdev = to_pci_dev(dev);
>>> +    struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
>>> +
>>> +    mutex_lock(&adapter->mdev_lock);
>>> +    adapter->mdev_count++;
>>> +    mutex_unlock(&adapter->mdev_lock);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static struct mdev_parent_ops ifcvf_mdev_fops = {
>>> +    .owner            = THIS_MODULE,
>>> +    .supported_type_groups    = mdev_type_groups,
>>> +    .mdev_attr_groups    = mdev_dev_groups,
>>> +    .create            = ifcvf_mdev_create,
>>> +    .remove            = ifcvf_mdev_remove,
>>> +};
>>> +
>>> +static int ifcvf_probe(struct pci_dev *pdev, const struct 
>>> pci_device_id *id)
>>> +{
>>> +    struct device *dev = &pdev->dev;
>>> +    struct ifcvf_adapter *adapter;
>>> +    struct ifcvf_hw *vf;
>>> +    int ret, i;
>>> +
>>> +    adapter = kzalloc(sizeof(struct ifcvf_adapter), GFP_KERNEL);
>>> +    if (adapter == NULL) {
>>> +        ret = -ENOMEM;
>>> +        goto fail;
>>> +    }
>>> +
>>> +    mutex_init(&adapter->mdev_lock);
>>> +    adapter->mdev_count = 1;
>>
>>
>> So this is per VF based vDPA implementation, which seems not 
>> convenient for management.  Anyhow we can control the creation in PF?
>>
>> Thanks
> the driver scope for now doesn't support that, we can add these 
> feature in next releases.


Not a must for this series, but to have a better interaction with 
management like libvirt, it's better.

Btw, do you have the plan to post PF drivers?

Thanks

Re: [RFC 2/2] vhost: IFC VF vdpa layer

[Zhu Lingshan]

On 10/21/2019 6:19 PM, Jason Wang wrote:
>
> On 2019/10/21 下午5:53, Zhu, Lingshan wrote:
>>
>> On 10/16/2019 6:19 PM, Jason Wang wrote:
>>>
>>> On 2019/10/16 上午9:30, Zhu Lingshan wrote:
>>>> This commit introduced IFC VF operations for vdpa, which complys to
>>>> vhost_mdev interfaces, handles IFC VF initialization,
>>>> configuration and removal.
>>>>
>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>> ---
>>>>   drivers/vhost/ifcvf/ifcvf_main.c | 541 
>>>> +++++++++++++++++++++++++++++++++++++++
>>>>   1 file changed, 541 insertions(+)
>>>>   create mode 100644 drivers/vhost/ifcvf/ifcvf_main.c
>>>>
>>>> diff --git a/drivers/vhost/ifcvf/ifcvf_main.c 
>>>> b/drivers/vhost/ifcvf/ifcvf_main.c
>>>> new file mode 100644
>>>> index 000000000000..c48a29969a85
>>>> --- /dev/null
>>>> +++ b/drivers/vhost/ifcvf/ifcvf_main.c
>>>> @@ -0,0 +1,541 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * Copyright (C) 2019 Intel Corporation.
>>>> + */
>>>> +
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/mdev.h>
>>>> +#include <linux/pci.h>
>>>> +#include <linux/sysfs.h>
>>>> +
>>>> +#include "ifcvf_base.h"
>>>> +
>>>> +#define VERSION_STRING    "0.1"
>>>> +#define DRIVER_AUTHOR    "Intel Corporation"
>>>> +#define IFCVF_DRIVER_NAME    "ifcvf"
>>>> +
>>>> +static irqreturn_t ifcvf_intr_handler(int irq, void *arg)
>>>> +{
>>>> +    struct vring_info *vring = arg;
>>>> +
>>>> +    if (vring->cb.callback)
>>>> +        return vring->cb.callback(vring->cb.private);
>>>> +
>>>> +    return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static u64 ifcvf_mdev_get_features(struct mdev_device *mdev)
>>>> +{
>>>> +    return IFC_SUPPORTED_FEATURES;
>>>
>>>
>>> I would expect this should be done by querying the hw. Or IFC VF 
>>> can't get any update through its firmware?
>>
>> Hi Jason,
>>
>> Thanks for your comments, for now driver just support these features.
>
>
> Ok, it should work but less flexible, we can change it in the future.
sure!
>
>
>>
>>>
>>>
>>>> +}
>>>> +
>>>> +static int ifcvf_mdev_set_features(struct mdev_device *mdev, u64 
>>>> features)
>>>> +{
>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>> +
>>>> +    vf->req_features = features;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static u64 ifcvf_mdev_get_vq_state(struct mdev_device *mdev, u16 qid)
>>>> +{
>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>> +
>>>> +    return vf->vring[qid].last_avail_idx;
>>>
>>>
>>> Does this really work? I'd expect it should be fetched from hw since 
>>> it's an internal state.
>> for now, it's working, we intend to support LM in next version drivers.
>
>
> I'm not sure I understand here, I don't see any synchronization 
> between the hardware and last_avail_idx, so last_avail_idx should not 
> change.
>
> Btw, what did "LM" mean :) ?

I can add bar IO operations here, LM = live migration, sorry for the 
abbreviation.

>
>
>>>
>>>
>>>> +}
>>>> +
>>>> +static int ifcvf_mdev_set_vq_state(struct mdev_device *mdev, u16 
>>>> qid, u64 num)
>>>> +{
>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>> +
>>>> +    vf->vring[qid].last_used_idx = num;
>>>
>>>
>>> I fail to understand why last_used_idx is needed. It looks to me the 
>>> used idx in the used ring is sufficient.
>> I will remove it.
>>>
>>>
>>>> + vf->vring[qid].last_avail_idx = num;
>>>
>>>
>>> Do we need a synchronization with hw immediately here?
>>>
>>>
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int ifcvf_mdev_set_vq_address(struct mdev_device *mdev, u16 
>>>> idx,
>>>> +                     u64 desc_area, u64 driver_area,
>>>> +                     u64 device_area)
>>>> +{
>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>> +
>>>> +    vf->vring[idx].desc = desc_area;
>>>> +    vf->vring[idx].avail = driver_area;
>>>> +    vf->vring[idx].used = device_area;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static void ifcvf_mdev_set_vq_num(struct mdev_device *mdev, u16 
>>>> qid, u32 num)
>>>> +{
>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>> +
>>>> +    vf->vring[qid].size = num;
>>>> +}
>>>> +
>>>> +static void ifcvf_mdev_set_vq_ready(struct mdev_device *mdev,
>>>> +                u16 qid, bool ready)
>>>> +{
>>>> +
>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>> +
>>>> +    vf->vring[qid].ready = ready;
>>>> +}
>>>> +
>>>> +static bool ifcvf_mdev_get_vq_ready(struct mdev_device *mdev, u16 
>>>> qid)
>>>> +{
>>>> +
>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>> +
>>>> +    return vf->vring[qid].ready;
>>>> +}
>>>> +
>>>> +static void ifcvf_mdev_set_vq_cb(struct mdev_device *mdev, u16 idx,
>>>> +                 struct virtio_mdev_callback *cb)
>>>> +{
>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>> +
>>>> +    vf->vring[idx].cb = *cb;
>>>> +}
>>>> +
>>>> +static void ifcvf_mdev_kick_vq(struct mdev_device *mdev, u16 idx)
>>>> +{
>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>> +
>>>> +    ifcvf_notify_queue(vf, idx);
>>>> +}
>>>> +
>>>> +static u8 ifcvf_mdev_get_status(struct mdev_device *mdev)
>>>> +{
>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>> +
>>>> +    return vf->status;
>>>> +}
>>>> +
>>>> +static u32 ifcvf_mdev_get_generation(struct mdev_device *mdev)
>>>> +{
>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>> +
>>>> +    return vf->generation;
>>>> +}
>>>> +
>>>> +static int ifcvf_mdev_get_version(struct mdev_device *mdev)
>>>> +{
>>>> +    return VIRTIO_MDEV_VERSION;
>>>> +}
>>>> +
>>>> +static u32 ifcvf_mdev_get_device_id(struct mdev_device *mdev)
>>>> +{
>>>> +    return IFCVF_DEVICE_ID;
>>>> +}
>>>> +
>>>> +static u32 ifcvf_mdev_get_vendor_id(struct mdev_device *mdev)
>>>> +{
>>>> +    return IFCVF_VENDOR_ID;
>>>> +}
>>>> +
>>>> +static u16 ifcvf_mdev_get_vq_align(struct mdev_device *mdev)
>>>> +{
>>>> +    return IFCVF_QUEUE_ALIGNMENT;
>>>> +}
>>>> +
>>>> +static int ifcvf_start_datapath(void *private)
>>>> +{
>>>> +    int i, ret;
>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(private);
>>>> +
>>>> +    for (i = 0; i < (IFCVF_MAX_QUEUE_PAIRS * 2); i++) {
>>>> +        if (!vf->vring[i].ready)
>>>> +            break;
>>>
>>>
>>> Looks like error should be returned here?
>> agreed!
>>>
>>>
>>>> +
>>>> +        if (!vf->vring[i].size)
>>>> +            break;
>>>> +
>>>> +        if (!vf->vring[i].desc || !vf->vring[i].avail ||
>>>> +            !vf->vring[i].used)
>>>> +            break;
>>>> +    }
>>>> +    vf->nr_vring = i;
>>>> +
>>>> +    ret = ifcvf_start_hw(vf);
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static int ifcvf_stop_datapath(void *private)
>>>> +{
>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(private);
>>>> +    int i;
>>>> +
>>>> +    for (i = 0; i < IFCVF_MAX_QUEUES; i++)
>>>> +        vf->vring[i].cb.callback = NULL;
>>>
>>>
>>> Any synchronization is needed for the vq irq handler?
>> I think even we set callback = NULL, the code is still there, 
>> on-going routines would not be effected.
>
>
> Ok I think you mean when ifcvf_stop_hw() return, hardware will not 
> respond to e.g kick and other events etc.
Yes, ifcvf_stop_hw() would disable the hw.
>
>
>>>
>>>
>>>> +
>>>> +    ifcvf_stop_hw(vf);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static void ifcvf_reset_vring(struct ifcvf_adapter *adapter)
>>>> +{
>>>> +    int i;
>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>> +
>>>> +    for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
>>>> +        vf->vring[i].last_used_idx = 0;
>>>> +        vf->vring[i].last_avail_idx = 0;
>>>> +        vf->vring[i].desc = 0;
>>>> +        vf->vring[i].avail = 0;
>>>> +        vf->vring[i].used = 0;
>>>> +        vf->vring[i].ready = 0;
>>>> +        vf->vring->cb.callback = NULL;
>>>> +        vf->vring->cb.private = NULL;
>>>> +    }
>>>> +}
>>>> +
>>>> +static void ifcvf_mdev_set_status(struct mdev_device *mdev, u8 
>>>> status)
>>>> +{
>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>> +
>>>> +    vf->status = status;
>>>> +
>>>> +    if (status == 0) {
>>>> +        ifcvf_stop_datapath(adapter);
>>>> +        ifcvf_reset_vring(adapter);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
>>>> +        ifcvf_start_datapath(adapter);
>>>> +        return;
>>>> +    }
>>>> +}
>>>> +
>>>> +static u16 ifcvf_mdev_get_queue_max(struct mdev_device *mdev)
>>>> +{
>>>> +    return IFCVF_MAX_QUEUES;
>>>
>>>
>>> The name is confusing, it was used to return the maximum queue size. 
>>> In new version of virtio-mdev, the callback was renamed as 
>>> get_vq_num_max().
>> will change that.
>>>
>>>
>>>> +}
>>>> +
>>>> +static struct virtio_mdev_device_ops ifc_mdev_ops = {
>>>> +    .get_features  = ifcvf_mdev_get_features,
>>>> +    .set_features  = ifcvf_mdev_set_features,
>>>> +    .get_status    = ifcvf_mdev_get_status,
>>>> +    .set_status    = ifcvf_mdev_set_status,
>>>> +    .get_queue_max = ifcvf_mdev_get_queue_max,
>>>> +    .get_vq_state   = ifcvf_mdev_get_vq_state,
>>>> +    .set_vq_state   = ifcvf_mdev_set_vq_state,
>>>> +    .set_vq_cb      = ifcvf_mdev_set_vq_cb,
>>>> +    .set_vq_ready   = ifcvf_mdev_set_vq_ready,
>>>> +    .get_vq_ready    = ifcvf_mdev_get_vq_ready,
>>>> +    .set_vq_num     = ifcvf_mdev_set_vq_num,
>>>> +    .set_vq_address = ifcvf_mdev_set_vq_address,
>>>> +    .kick_vq        = ifcvf_mdev_kick_vq,
>>>> +    .get_generation    = ifcvf_mdev_get_generation,
>>>> +    .get_version    = ifcvf_mdev_get_version,
>>>> +    .get_device_id    = ifcvf_mdev_get_device_id,
>>>> +    .get_vendor_id    = ifcvf_mdev_get_vendor_id,
>>>> +    .get_vq_align    = ifcvf_mdev_get_vq_align,
>>>> +};
>>>
>>>
>>> set_config/get_config is missing. It looks to me they are not hard, 
>>> just implementing the access to dev_cfg. It's key to make kernel 
>>> virtio driver to work.
>>>
>>> And in the new version of virito-mdev, features like _F_LOG_ALL 
>>> should be advertised through get_mdev_features.
>> IMHO, currently the driver can work without set/get_config, 
>> vhost_mdev doesn't call them for now.
>
>
> Yes, but it was required by virtio_mdev for host driver to work, and 
> it looks to me it's not hard to add them. If possible please add them 
> and "virtio" type then we can use the ops for both the case of VM and 
> containers.
sure
>
>
>>>
>>>
>>>> +
>>>> +static int ifcvf_init_msix(struct ifcvf_adapter *adapter)
>>>> +{
>>>> +    int vector, i, ret, irq;
>>>> +    struct pci_dev *pdev = to_pci_dev(adapter->dev);
>>>> +    struct ifcvf_hw *vf = &adapter->vf;
>>>> +
>>>> +    ret = pci_alloc_irq_vectors(pdev, IFCVF_MAX_INTR,
>>>> +            IFCVF_MAX_INTR, PCI_IRQ_MSIX);
>>>> +    if (ret < 0) {
>>>> +        IFC_ERR(adapter->dev, "Failed to alloc irq vectors.\n");
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
>>>> +        vector = i + IFCVF_MSI_QUEUE_OFF;
>>>> +        irq = pci_irq_vector(pdev, vector);
>>>> +        ret = request_irq(irq, ifcvf_intr_handler, 0,
>>>> +                pci_name(pdev), &vf->vring[i]);
>>>> +        if (ret) {
>>>> +            IFC_ERR(adapter->dev,
>>>> +                "Failed to request irq for vq %d.\n", i);
>>>> +            return ret;
>>>> +        }
>>>> +    }
>>>
>>>
>>> Do we need to provide fallback when we can't do per vq MSIX?
>> I think it would be very rarely that can not get enough vectors.
>
>
> Right.
>
>
>>>
>>>
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static void ifcvf_destroy_adapter(struct ifcvf_adapter *adapter)
>>>> +{
>>>> +    int i, vector, irq;
>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>> +    struct pci_dev *pdev = to_pci_dev(adapter->dev);
>>>> +
>>>> +    for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
>>>> +        vector = i + IFCVF_MSI_QUEUE_OFF;
>>>> +        irq = pci_irq_vector(pdev, vector);
>>>> +        free_irq(irq, &vf->vring[i]);
>>>> +    }
>>>> +}
>>>> +
>>>> +static ssize_t name_show(struct kobject *kobj, struct device *dev, 
>>>> char *buf)
>>>> +{
>>>> +    const char *name = "vhost accelerator (virtio ring compatible)";
>>>> +
>>>> +    return sprintf(buf, "%s\n", name);
>>>> +}
>>>> +MDEV_TYPE_ATTR_RO(name);
>>>> +
>>>> +static ssize_t device_api_show(struct kobject *kobj, struct device 
>>>> *dev,
>>>> +                   char *buf)
>>>> +{
>>>> +    return sprintf(buf, "%s\n", VIRTIO_MDEV_DEVICE_API_STRING);
>>>> +}
>>>> +MDEV_TYPE_ATTR_RO(device_api);
>>>> +
>>>> +static ssize_t available_instances_show(struct kobject *kobj,
>>>> +                    struct device *dev, char *buf)
>>>> +{
>>>> +    struct pci_dev *pdev = to_pci_dev(dev);
>>>> +    struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
>>>> +
>>>> +    return sprintf(buf, "%d\n", adapter->mdev_count);
>>>> +}
>>>> +
>>>> +MDEV_TYPE_ATTR_RO(available_instances);
>>>> +
>>>> +static ssize_t type_show(struct kobject *kobj,
>>>> +            struct device *dev, char *buf)
>>>> +{
>>>> +    return sprintf(buf, "%s\n", "net");
>>>> +}
>>>> +
>>>> +MDEV_TYPE_ATTR_RO(type);
>>>> +
>>>> +
>>>> +static struct attribute *mdev_types_attrs[] = {
>>>> +    &mdev_type_attr_name.attr,
>>>> +    &mdev_type_attr_device_api.attr,
>>>> +    &mdev_type_attr_available_instances.attr,
>>>> +    &mdev_type_attr_type.attr,
>>>> +    NULL,
>>>> +};
>>>> +
>>>> +static struct attribute_group mdev_type_group = {
>>>> +    .name  = "vdpa_virtio",
>>>
>>>
>>> To be consistent, it should be "vhost" or "virtio".
>> agreed!
>>>
>>>
>>>> +    .attrs = mdev_types_attrs,
>>>> +};
>>>> +
>>>> +static struct attribute_group *mdev_type_groups[] = {
>>>> +    &mdev_type_group,
>>>> +    NULL,
>>>> +};
>>>> +
>>>> +const struct attribute_group *mdev_dev_groups[] = {
>>>> +    NULL,
>>>> +};
>>>> +
>>>> +static int ifcvf_mdev_create(struct kobject *kobj, struct 
>>>> mdev_device *mdev)
>>>> +{
>>>> +    struct device *dev = mdev_parent_dev(mdev);
>>>> +    struct pci_dev *pdev = to_pci_dev(dev);
>>>> +    struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
>>>> +    int ret = 0;
>>>> +
>>>> +    mutex_lock(&adapter->mdev_lock);
>>>> +
>>>> +    if (adapter->mdev_count < 1) {
>>>> +        ret = -EINVAL;
>>>> +        goto out;
>>>> +    }
>>>> +
>>>> +    mdev_set_class_id(mdev, MDEV_ID_VHOST);
>>>> +    mdev_set_dev_ops(mdev, &ifc_mdev_ops);
>>>> +
>>>> +    mdev_set_drvdata(mdev, adapter);
>>>> +    mdev_set_iommu_device(mdev_dev(mdev), dev);
>>>> +
>>>> +    INIT_LIST_HEAD(&adapter->dma_maps);
>>>> +    adapter->mdev_count--;
>>>> +
>>>> +out:
>>>> +    mutex_unlock(&adapter->mdev_lock);
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static int ifcvf_mdev_remove(struct mdev_device *mdev)
>>>> +{
>>>> +    struct device *dev = mdev_parent_dev(mdev);
>>>> +    struct pci_dev *pdev = to_pci_dev(dev);
>>>> +    struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
>>>> +
>>>> +    mutex_lock(&adapter->mdev_lock);
>>>> +    adapter->mdev_count++;
>>>> +    mutex_unlock(&adapter->mdev_lock);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static struct mdev_parent_ops ifcvf_mdev_fops = {
>>>> +    .owner            = THIS_MODULE,
>>>> +    .supported_type_groups    = mdev_type_groups,
>>>> +    .mdev_attr_groups    = mdev_dev_groups,
>>>> +    .create            = ifcvf_mdev_create,
>>>> +    .remove            = ifcvf_mdev_remove,
>>>> +};
>>>> +
>>>> +static int ifcvf_probe(struct pci_dev *pdev, const struct 
>>>> pci_device_id *id)
>>>> +{
>>>> +    struct device *dev = &pdev->dev;
>>>> +    struct ifcvf_adapter *adapter;
>>>> +    struct ifcvf_hw *vf;
>>>> +    int ret, i;
>>>> +
>>>> +    adapter = kzalloc(sizeof(struct ifcvf_adapter), GFP_KERNEL);
>>>> +    if (adapter == NULL) {
>>>> +        ret = -ENOMEM;
>>>> +        goto fail;
>>>> +    }
>>>> +
>>>> +    mutex_init(&adapter->mdev_lock);
>>>> +    adapter->mdev_count = 1;
>>>
>>>
>>> So this is per VF based vDPA implementation, which seems not 
>>> convenient for management.  Anyhow we can control the creation in PF?
>>>
>>> Thanks
>> the driver scope for now doesn't support that, we can add these 
>> feature in next releases.
>
>
> Not a must for this series, but to have a better interaction with 
> management like libvirt, it's better.
>
> Btw, do you have the plan to post PF drivers?
>
> Thanks
Maybe we can workout PF driver in next release :)
THanks,
BR
Zhu Lingshan
>
>

Re: [RFC 2/2] vhost: IFC VF vdpa layer

[Jason Wang]

On 2019/10/22 下午2:53, Zhu Lingshan wrote:
>
> On 10/21/2019 6:19 PM, Jason Wang wrote:
>>
>> On 2019/10/21 下午5:53, Zhu, Lingshan wrote:
>>>
>>> On 10/16/2019 6:19 PM, Jason Wang wrote:
>>>>
>>>> On 2019/10/16 上午9:30, Zhu Lingshan wrote:
>>>>> This commit introduced IFC VF operations for vdpa, which complys to
>>>>> vhost_mdev interfaces, handles IFC VF initialization,
>>>>> configuration and removal.
>>>>>
>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>> ---


[...]


>>
>>
>>>
>>>>
>>>>
>>>>> +}
>>>>> +
>>>>> +static int ifcvf_mdev_set_features(struct mdev_device *mdev, u64 
>>>>> features)
>>>>> +{
>>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>>> +
>>>>> +    vf->req_features = features;
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static u64 ifcvf_mdev_get_vq_state(struct mdev_device *mdev, u16 
>>>>> qid)
>>>>> +{
>>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>>> +
>>>>> +    return vf->vring[qid].last_avail_idx;
>>>>
>>>>
>>>> Does this really work? I'd expect it should be fetched from hw 
>>>> since it's an internal state.
>>> for now, it's working, we intend to support LM in next version drivers.
>>
>>
>> I'm not sure I understand here, I don't see any synchronization 
>> between the hardware and last_avail_idx, so last_avail_idx should not 
>> change.
>>
>> Btw, what did "LM" mean :) ?
>
> I can add bar IO operations here, LM = live migration, sorry for the 
> abbreviation.


Just make sure I understand here, I believe you mean reading 
last_avail_idx through IO bar here?

Thanks

Re: [RFC 2/2] vhost: IFC VF vdpa layer

[Zhu, Lingshan]

On 10/22/2019 9:05 PM, Jason Wang wrote:
>
> On 2019/10/22 下午2:53, Zhu Lingshan wrote:
>>
>> On 10/21/2019 6:19 PM, Jason Wang wrote:
>>>
>>> On 2019/10/21 下午5:53, Zhu, Lingshan wrote:
>>>>
>>>> On 10/16/2019 6:19 PM, Jason Wang wrote:
>>>>>
>>>>> On 2019/10/16 上午9:30, Zhu Lingshan wrote:
>>>>>> This commit introduced IFC VF operations for vdpa, which complys to
>>>>>> vhost_mdev interfaces, handles IFC VF initialization,
>>>>>> configuration and removal.
>>>>>>
>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>>> ---
>
>
> [...]
>
>
>>>
>>>
>>>>
>>>>>
>>>>>
>>>>>> +}
>>>>>> +
>>>>>> +static int ifcvf_mdev_set_features(struct mdev_device *mdev, u64 
>>>>>> features)
>>>>>> +{
>>>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>>>> +
>>>>>> +    vf->req_features = features;
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static u64 ifcvf_mdev_get_vq_state(struct mdev_device *mdev, u16 
>>>>>> qid)
>>>>>> +{
>>>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>>>> +
>>>>>> +    return vf->vring[qid].last_avail_idx;
>>>>>
>>>>>
>>>>> Does this really work? I'd expect it should be fetched from hw 
>>>>> since it's an internal state.
>>>> for now, it's working, we intend to support LM in next version 
>>>> drivers.
>>>
>>>
>>> I'm not sure I understand here, I don't see any synchronization 
>>> between the hardware and last_avail_idx, so last_avail_idx should 
>>> not change.
>>>
>>> Btw, what did "LM" mean :) ?
>>
>> I can add bar IO operations here, LM = live migration, sorry for the 
>> abbreviation.
>
>
> Just make sure I understand here, I believe you mean reading 
> last_avail_idx through IO bar here?
>
> Thanks

Hi Jason,

Yes, I mean last_avail_idx. is that correct?

THanks

>
>

Re: [RFC 2/2] vhost: IFC VF vdpa layer

[Jason Wang]

On 2019/10/23 下午2:19, Zhu, Lingshan wrote:
>
> On 10/22/2019 9:05 PM, Jason Wang wrote:
>>
>> On 2019/10/22 下午2:53, Zhu Lingshan wrote:
>>>
>>> On 10/21/2019 6:19 PM, Jason Wang wrote:
>>>>
>>>> On 2019/10/21 下午5:53, Zhu, Lingshan wrote:
>>>>>
>>>>> On 10/16/2019 6:19 PM, Jason Wang wrote:
>>>>>>
>>>>>> On 2019/10/16 上午9:30, Zhu Lingshan wrote:
>>>>>>> This commit introduced IFC VF operations for vdpa, which complys to
>>>>>>> vhost_mdev interfaces, handles IFC VF initialization,
>>>>>>> configuration and removal.
>>>>>>>
>>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>>>> ---
>>
>>
>> [...]
>>
>>
>>>>
>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int ifcvf_mdev_set_features(struct mdev_device *mdev, 
>>>>>>> u64 features)
>>>>>>> +{
>>>>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>>>>> +
>>>>>>> +    vf->req_features = features;
>>>>>>> +
>>>>>>> +    return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static u64 ifcvf_mdev_get_vq_state(struct mdev_device *mdev, 
>>>>>>> u16 qid)
>>>>>>> +{
>>>>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>>>>> +
>>>>>>> +    return vf->vring[qid].last_avail_idx;
>>>>>>
>>>>>>
>>>>>> Does this really work? I'd expect it should be fetched from hw 
>>>>>> since it's an internal state.
>>>>> for now, it's working, we intend to support LM in next version 
>>>>> drivers.
>>>>
>>>>
>>>> I'm not sure I understand here, I don't see any synchronization 
>>>> between the hardware and last_avail_idx, so last_avail_idx should 
>>>> not change.
>>>>
>>>> Btw, what did "LM" mean :) ?
>>>
>>> I can add bar IO operations here, LM = live migration, sorry for the 
>>> abbreviation.
>>
>>
>> Just make sure I understand here, I believe you mean reading 
>> last_avail_idx through IO bar here?
>>
>> Thanks
>
> Hi Jason,
>
> Yes, I mean last_avail_idx. is that correct?
>
> THanks


Yes.

Thanks


>
>>
>>

Re: [RFC 2/2] vhost: IFC VF vdpa layer

[Zhu, Lingshan]

On 10/22/2019 2:53 PM, Zhu Lingshan wrote:
>
> On 10/21/2019 6:19 PM, Jason Wang wrote:
>>
>> On 2019/10/21 下午5:53, Zhu, Lingshan wrote:
>>>
>>> On 10/16/2019 6:19 PM, Jason Wang wrote:
>>>>
>>>> On 2019/10/16 上午9:30, Zhu Lingshan wrote:
>>>>> This commit introduced IFC VF operations for vdpa, which complys to
>>>>> vhost_mdev interfaces, handles IFC VF initialization,
>>>>> configuration and removal.
>>>>>
>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>> ---
>>>>>   drivers/vhost/ifcvf/ifcvf_main.c | 541 
>>>>> +++++++++++++++++++++++++++++++++++++++
>>>>>   1 file changed, 541 insertions(+)
>>>>>   create mode 100644 drivers/vhost/ifcvf/ifcvf_main.c
>>>>>
>>>>> diff --git a/drivers/vhost/ifcvf/ifcvf_main.c 
>>>>> b/drivers/vhost/ifcvf/ifcvf_main.c
>>>>> new file mode 100644
>>>>> index 000000000000..c48a29969a85
>>>>> --- /dev/null
>>>>> +++ b/drivers/vhost/ifcvf/ifcvf_main.c
>>>>> @@ -0,0 +1,541 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>>> +/*
>>>>> + * Copyright (C) 2019 Intel Corporation.
>>>>> + */
>>>>> +
>>>>> +#include <linux/interrupt.h>
>>>>> +#include <linux/module.h>
>>>>> +#include <linux/mdev.h>
>>>>> +#include <linux/pci.h>
>>>>> +#include <linux/sysfs.h>
>>>>> +
>>>>> +#include "ifcvf_base.h"
>>>>> +
>>>>> +#define VERSION_STRING    "0.1"
>>>>> +#define DRIVER_AUTHOR    "Intel Corporation"
>>>>> +#define IFCVF_DRIVER_NAME    "ifcvf"
>>>>> +
>>>>> +static irqreturn_t ifcvf_intr_handler(int irq, void *arg)
>>>>> +{
>>>>> +    struct vring_info *vring = arg;
>>>>> +
>>>>> +    if (vring->cb.callback)
>>>>> +        return vring->cb.callback(vring->cb.private);
>>>>> +
>>>>> +    return IRQ_HANDLED;
>>>>> +}
>>>>> +
>>>>> +static u64 ifcvf_mdev_get_features(struct mdev_device *mdev)
>>>>> +{
>>>>> +    return IFC_SUPPORTED_FEATURES;
>>>>
>>>>
>>>> I would expect this should be done by querying the hw. Or IFC VF 
>>>> can't get any update through its firmware?
>>>
>>> Hi Jason,
>>>
>>> Thanks for your comments, for now driver just support these features.
>>
>>
>> Ok, it should work but less flexible, we can change it in the future.
> sure!
>>
>>
>>>
>>>>
>>>>
>>>>> +}
>>>>> +
>>>>> +static int ifcvf_mdev_set_features(struct mdev_device *mdev, u64 
>>>>> features)
>>>>> +{
>>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>>> +
>>>>> +    vf->req_features = features;
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static u64 ifcvf_mdev_get_vq_state(struct mdev_device *mdev, u16 
>>>>> qid)
>>>>> +{
>>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>>> +
>>>>> +    return vf->vring[qid].last_avail_idx;
>>>>
>>>>
>>>> Does this really work? I'd expect it should be fetched from hw 
>>>> since it's an internal state.
>>> for now, it's working, we intend to support LM in next version drivers.
>>
>>
>> I'm not sure I understand here, I don't see any synchronization 
>> between the hardware and last_avail_idx, so last_avail_idx should not 
>> change.
>>
>> Btw, what did "LM" mean :) ?
>
> I can add bar IO operations here, LM = live migration, sorry for the 
> abbreviation.
>
>>
>>
>>>>
>>>>
>>>>> +}
>>>>> +
>>>>> +static int ifcvf_mdev_set_vq_state(struct mdev_device *mdev, u16 
>>>>> qid, u64 num)
>>>>> +{
>>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>>> +
>>>>> +    vf->vring[qid].last_used_idx = num;
>>>>
>>>>
>>>> I fail to understand why last_used_idx is needed. It looks to me 
>>>> the used idx in the used ring is sufficient.
>>> I will remove it.
>>>>
>>>>
>>>>> + vf->vring[qid].last_avail_idx = num;
>>>>
>>>>
>>>> Do we need a synchronization with hw immediately here?
>>>>
>>>>
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static int ifcvf_mdev_set_vq_address(struct mdev_device *mdev, 
>>>>> u16 idx,
>>>>> +                     u64 desc_area, u64 driver_area,
>>>>> +                     u64 device_area)
>>>>> +{
>>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>>> +
>>>>> +    vf->vring[idx].desc = desc_area;
>>>>> +    vf->vring[idx].avail = driver_area;
>>>>> +    vf->vring[idx].used = device_area;
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static void ifcvf_mdev_set_vq_num(struct mdev_device *mdev, u16 
>>>>> qid, u32 num)
>>>>> +{
>>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>>> +
>>>>> +    vf->vring[qid].size = num;
>>>>> +}
>>>>> +
>>>>> +static void ifcvf_mdev_set_vq_ready(struct mdev_device *mdev,
>>>>> +                u16 qid, bool ready)
>>>>> +{
>>>>> +
>>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>>> +
>>>>> +    vf->vring[qid].ready = ready;
>>>>> +}
>>>>> +
>>>>> +static bool ifcvf_mdev_get_vq_ready(struct mdev_device *mdev, u16 
>>>>> qid)
>>>>> +{
>>>>> +
>>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>>> +
>>>>> +    return vf->vring[qid].ready;
>>>>> +}
>>>>> +
>>>>> +static void ifcvf_mdev_set_vq_cb(struct mdev_device *mdev, u16 idx,
>>>>> +                 struct virtio_mdev_callback *cb)
>>>>> +{
>>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>>> +
>>>>> +    vf->vring[idx].cb = *cb;
>>>>> +}
>>>>> +
>>>>> +static void ifcvf_mdev_kick_vq(struct mdev_device *mdev, u16 idx)
>>>>> +{
>>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>>> +
>>>>> +    ifcvf_notify_queue(vf, idx);
>>>>> +}
>>>>> +
>>>>> +static u8 ifcvf_mdev_get_status(struct mdev_device *mdev)
>>>>> +{
>>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>>> +
>>>>> +    return vf->status;
>>>>> +}
>>>>> +
>>>>> +static u32 ifcvf_mdev_get_generation(struct mdev_device *mdev)
>>>>> +{
>>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>>> +
>>>>> +    return vf->generation;
>>>>> +}
>>>>> +
>>>>> +static int ifcvf_mdev_get_version(struct mdev_device *mdev)
>>>>> +{
>>>>> +    return VIRTIO_MDEV_VERSION;
>>>>> +}
>>>>> +
>>>>> +static u32 ifcvf_mdev_get_device_id(struct mdev_device *mdev)
>>>>> +{
>>>>> +    return IFCVF_DEVICE_ID;
>>>>> +}
>>>>> +
>>>>> +static u32 ifcvf_mdev_get_vendor_id(struct mdev_device *mdev)
>>>>> +{
>>>>> +    return IFCVF_VENDOR_ID;
>>>>> +}
>>>>> +
>>>>> +static u16 ifcvf_mdev_get_vq_align(struct mdev_device *mdev)
>>>>> +{
>>>>> +    return IFCVF_QUEUE_ALIGNMENT;
>>>>> +}
>>>>> +
>>>>> +static int ifcvf_start_datapath(void *private)
>>>>> +{
>>>>> +    int i, ret;
>>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(private);
>>>>> +
>>>>> +    for (i = 0; i < (IFCVF_MAX_QUEUE_PAIRS * 2); i++) {
>>>>> +        if (!vf->vring[i].ready)
>>>>> +            break;
>>>>
>>>>
>>>> Looks like error should be returned here?
>>> agreed!
>>>>
>>>>
>>>>> +
>>>>> +        if (!vf->vring[i].size)
>>>>> +            break;
>>>>> +
>>>>> +        if (!vf->vring[i].desc || !vf->vring[i].avail ||
>>>>> +            !vf->vring[i].used)
>>>>> +            break;
>>>>> +    }
>>>>> +    vf->nr_vring = i;
>>>>> +
>>>>> +    ret = ifcvf_start_hw(vf);
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>> +static int ifcvf_stop_datapath(void *private)
>>>>> +{
>>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(private);
>>>>> +    int i;
>>>>> +
>>>>> +    for (i = 0; i < IFCVF_MAX_QUEUES; i++)
>>>>> +        vf->vring[i].cb.callback = NULL;
>>>>
>>>>
>>>> Any synchronization is needed for the vq irq handler?
>>> I think even we set callback = NULL, the code is still there, 
>>> on-going routines would not be effected.
>>
>>
>> Ok I think you mean when ifcvf_stop_hw() return, hardware will not 
>> respond to e.g kick and other events etc.
> Yes, ifcvf_stop_hw() would disable the hw.
>>
>>
>>>>
>>>>
>>>>> +
>>>>> +    ifcvf_stop_hw(vf);
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static void ifcvf_reset_vring(struct ifcvf_adapter *adapter)
>>>>> +{
>>>>> +    int i;
>>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>>> +
>>>>> +    for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
>>>>> +        vf->vring[i].last_used_idx = 0;
>>>>> +        vf->vring[i].last_avail_idx = 0;
>>>>> +        vf->vring[i].desc = 0;
>>>>> +        vf->vring[i].avail = 0;
>>>>> +        vf->vring[i].used = 0;
>>>>> +        vf->vring[i].ready = 0;
>>>>> +        vf->vring->cb.callback = NULL;
>>>>> +        vf->vring->cb.private = NULL;
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> +static void ifcvf_mdev_set_status(struct mdev_device *mdev, u8 
>>>>> status)
>>>>> +{
>>>>> +    struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
>>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>>> +
>>>>> +    vf->status = status;
>>>>> +
>>>>> +    if (status == 0) {
>>>>> +        ifcvf_stop_datapath(adapter);
>>>>> +        ifcvf_reset_vring(adapter);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
>>>>> +        ifcvf_start_datapath(adapter);
>>>>> +        return;
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> +static u16 ifcvf_mdev_get_queue_max(struct mdev_device *mdev)
>>>>> +{
>>>>> +    return IFCVF_MAX_QUEUES;
>>>>
>>>>
>>>> The name is confusing, it was used to return the maximum queue 
>>>> size. In new version of virtio-mdev, the callback was renamed as 
>>>> get_vq_num_max().
>>> will change that.
>>>>
>>>>
>>>>> +}
>>>>> +
>>>>> +static struct virtio_mdev_device_ops ifc_mdev_ops = {
>>>>> +    .get_features  = ifcvf_mdev_get_features,
>>>>> +    .set_features  = ifcvf_mdev_set_features,
>>>>> +    .get_status    = ifcvf_mdev_get_status,
>>>>> +    .set_status    = ifcvf_mdev_set_status,
>>>>> +    .get_queue_max = ifcvf_mdev_get_queue_max,
>>>>> +    .get_vq_state   = ifcvf_mdev_get_vq_state,
>>>>> +    .set_vq_state   = ifcvf_mdev_set_vq_state,
>>>>> +    .set_vq_cb      = ifcvf_mdev_set_vq_cb,
>>>>> +    .set_vq_ready   = ifcvf_mdev_set_vq_ready,
>>>>> +    .get_vq_ready    = ifcvf_mdev_get_vq_ready,
>>>>> +    .set_vq_num     = ifcvf_mdev_set_vq_num,
>>>>> +    .set_vq_address = ifcvf_mdev_set_vq_address,
>>>>> +    .kick_vq        = ifcvf_mdev_kick_vq,
>>>>> +    .get_generation    = ifcvf_mdev_get_generation,
>>>>> +    .get_version    = ifcvf_mdev_get_version,
>>>>> +    .get_device_id    = ifcvf_mdev_get_device_id,
>>>>> +    .get_vendor_id    = ifcvf_mdev_get_vendor_id,
>>>>> +    .get_vq_align    = ifcvf_mdev_get_vq_align,
>>>>> +};
>>>>
>>>>
>>>> set_config/get_config is missing. It looks to me they are not hard, 
>>>> just implementing the access to dev_cfg. It's key to make kernel 
>>>> virtio driver to work.
>>>>
>>>> And in the new version of virito-mdev, features like _F_LOG_ALL 
>>>> should be advertised through get_mdev_features.
>>> IMHO, currently the driver can work without set/get_config, 
>>> vhost_mdev doesn't call them for now.
>>
>>
>> Yes, but it was required by virtio_mdev for host driver to work, and 
>> it looks to me it's not hard to add them. If possible please add them 
>> and "virtio" type then we can use the ops for both the case of VM and 
>> containers.
> sure

Hello Jason,

Just want to double confirm the implementation of set/get_config, for 
now, dev_cfg only contains mac[6], status and max_virtqueue_pairs, is 
that enough to support virtio_mdev?

THanks!

>>
>>
>>>>
>>>>
>>>>> +
>>>>> +static int ifcvf_init_msix(struct ifcvf_adapter *adapter)
>>>>> +{
>>>>> +    int vector, i, ret, irq;
>>>>> +    struct pci_dev *pdev = to_pci_dev(adapter->dev);
>>>>> +    struct ifcvf_hw *vf = &adapter->vf;
>>>>> +
>>>>> +    ret = pci_alloc_irq_vectors(pdev, IFCVF_MAX_INTR,
>>>>> +            IFCVF_MAX_INTR, PCI_IRQ_MSIX);
>>>>> +    if (ret < 0) {
>>>>> +        IFC_ERR(adapter->dev, "Failed to alloc irq vectors.\n");
>>>>> +        return ret;
>>>>> +    }
>>>>> +
>>>>> +    for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
>>>>> +        vector = i + IFCVF_MSI_QUEUE_OFF;
>>>>> +        irq = pci_irq_vector(pdev, vector);
>>>>> +        ret = request_irq(irq, ifcvf_intr_handler, 0,
>>>>> +                pci_name(pdev), &vf->vring[i]);
>>>>> +        if (ret) {
>>>>> +            IFC_ERR(adapter->dev,
>>>>> +                "Failed to request irq for vq %d.\n", i);
>>>>> +            return ret;
>>>>> +        }
>>>>> +    }
>>>>
>>>>
>>>> Do we need to provide fallback when we can't do per vq MSIX?
>>> I think it would be very rarely that can not get enough vectors.
>>
>>
>> Right.
>>
>>
>>>>
>>>>
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static void ifcvf_destroy_adapter(struct ifcvf_adapter *adapter)
>>>>> +{
>>>>> +    int i, vector, irq;
>>>>> +    struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
>>>>> +    struct pci_dev *pdev = to_pci_dev(adapter->dev);
>>>>> +
>>>>> +    for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
>>>>> +        vector = i + IFCVF_MSI_QUEUE_OFF;
>>>>> +        irq = pci_irq_vector(pdev, vector);
>>>>> +        free_irq(irq, &vf->vring[i]);
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> +static ssize_t name_show(struct kobject *kobj, struct device 
>>>>> *dev, char *buf)
>>>>> +{
>>>>> +    const char *name = "vhost accelerator (virtio ring compatible)";
>>>>> +
>>>>> +    return sprintf(buf, "%s\n", name);
>>>>> +}
>>>>> +MDEV_TYPE_ATTR_RO(name);
>>>>> +
>>>>> +static ssize_t device_api_show(struct kobject *kobj, struct 
>>>>> device *dev,
>>>>> +                   char *buf)
>>>>> +{
>>>>> +    return sprintf(buf, "%s\n", VIRTIO_MDEV_DEVICE_API_STRING);
>>>>> +}
>>>>> +MDEV_TYPE_ATTR_RO(device_api);
>>>>> +
>>>>> +static ssize_t available_instances_show(struct kobject *kobj,
>>>>> +                    struct device *dev, char *buf)
>>>>> +{
>>>>> +    struct pci_dev *pdev = to_pci_dev(dev);
>>>>> +    struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
>>>>> +
>>>>> +    return sprintf(buf, "%d\n", adapter->mdev_count);
>>>>> +}
>>>>> +
>>>>> +MDEV_TYPE_ATTR_RO(available_instances);
>>>>> +
>>>>> +static ssize_t type_show(struct kobject *kobj,
>>>>> +            struct device *dev, char *buf)
>>>>> +{
>>>>> +    return sprintf(buf, "%s\n", "net");
>>>>> +}
>>>>> +
>>>>> +MDEV_TYPE_ATTR_RO(type);
>>>>> +
>>>>> +
>>>>> +static struct attribute *mdev_types_attrs[] = {
>>>>> +    &mdev_type_attr_name.attr,
>>>>> +    &mdev_type_attr_device_api.attr,
>>>>> + &mdev_type_attr_available_instances.attr,
>>>>> +    &mdev_type_attr_type.attr,
>>>>> +    NULL,
>>>>> +};
>>>>> +
>>>>> +static struct attribute_group mdev_type_group = {
>>>>> +    .name  = "vdpa_virtio",
>>>>
>>>>
>>>> To be consistent, it should be "vhost" or "virtio".
>>> agreed!
>>>>
>>>>
>>>>> +    .attrs = mdev_types_attrs,
>>>>> +};
>>>>> +
>>>>> +static struct attribute_group *mdev_type_groups[] = {
>>>>> +    &mdev_type_group,
>>>>> +    NULL,
>>>>> +};
>>>>> +
>>>>> +const struct attribute_group *mdev_dev_groups[] = {
>>>>> +    NULL,
>>>>> +};
>>>>> +
>>>>> +static int ifcvf_mdev_create(struct kobject *kobj, struct 
>>>>> mdev_device *mdev)
>>>>> +{
>>>>> +    struct device *dev = mdev_parent_dev(mdev);
>>>>> +    struct pci_dev *pdev = to_pci_dev(dev);
>>>>> +    struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
>>>>> +    int ret = 0;
>>>>> +
>>>>> +    mutex_lock(&adapter->mdev_lock);
>>>>> +
>>>>> +    if (adapter->mdev_count < 1) {
>>>>> +        ret = -EINVAL;
>>>>> +        goto out;
>>>>> +    }
>>>>> +
>>>>> +    mdev_set_class_id(mdev, MDEV_ID_VHOST);
>>>>> +    mdev_set_dev_ops(mdev, &ifc_mdev_ops);
>>>>> +
>>>>> +    mdev_set_drvdata(mdev, adapter);
>>>>> +    mdev_set_iommu_device(mdev_dev(mdev), dev);
>>>>> +
>>>>> +    INIT_LIST_HEAD(&adapter->dma_maps);
>>>>> +    adapter->mdev_count--;
>>>>> +
>>>>> +out:
>>>>> +    mutex_unlock(&adapter->mdev_lock);
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>> +static int ifcvf_mdev_remove(struct mdev_device *mdev)
>>>>> +{
>>>>> +    struct device *dev = mdev_parent_dev(mdev);
>>>>> +    struct pci_dev *pdev = to_pci_dev(dev);
>>>>> +    struct ifcvf_adapter *adapter = pci_get_drvdata(pdev);
>>>>> +
>>>>> +    mutex_lock(&adapter->mdev_lock);
>>>>> +    adapter->mdev_count++;
>>>>> +    mutex_unlock(&adapter->mdev_lock);
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static struct mdev_parent_ops ifcvf_mdev_fops = {
>>>>> +    .owner            = THIS_MODULE,
>>>>> +    .supported_type_groups    = mdev_type_groups,
>>>>> +    .mdev_attr_groups    = mdev_dev_groups,
>>>>> +    .create            = ifcvf_mdev_create,
>>>>> +    .remove            = ifcvf_mdev_remove,
>>>>> +};
>>>>> +
>>>>> +static int ifcvf_probe(struct pci_dev *pdev, const struct 
>>>>> pci_device_id *id)
>>>>> +{
>>>>> +    struct device *dev = &pdev->dev;
>>>>> +    struct ifcvf_adapter *adapter;
>>>>> +    struct ifcvf_hw *vf;
>>>>> +    int ret, i;
>>>>> +
>>>>> +    adapter = kzalloc(sizeof(struct ifcvf_adapter), GFP_KERNEL);
>>>>> +    if (adapter == NULL) {
>>>>> +        ret = -ENOMEM;
>>>>> +        goto fail;
>>>>> +    }
>>>>> +
>>>>> +    mutex_init(&adapter->mdev_lock);
>>>>> +    adapter->mdev_count = 1;
>>>>
>>>>
>>>> So this is per VF based vDPA implementation, which seems not 
>>>> convenient for management.  Anyhow we can control the creation in PF?
>>>>
>>>> Thanks
>>> the driver scope for now doesn't support that, we can add these 
>>> feature in next releases.
>>
>>
>> Not a must for this series, but to have a better interaction with 
>> management like libvirt, it's better.
>>
>> Btw, do you have the plan to post PF drivers?
>>
>> Thanks
> Maybe we can workout PF driver in next release :)
> THanks,
> BR
> Zhu Lingshan
>>
>>

Re: [RFC 2/2] vhost: IFC VF vdpa layer

[Jason Wang]

On 2019/10/23 下午5:24, Zhu, Lingshan wrote:
>>>>>
>>>>>
>>>>> set_config/get_config is missing. It looks to me they are not 
>>>>> hard, just implementing the access to dev_cfg. It's key to make 
>>>>> kernel virtio driver to work.
>>>>>
>>>>> And in the new version of virito-mdev, features like _F_LOG_ALL 
>>>>> should be advertised through get_mdev_features.
>>>> IMHO, currently the driver can work without set/get_config, 
>>>> vhost_mdev doesn't call them for now.
>>>
>>>
>>> Yes, but it was required by virtio_mdev for host driver to work, and 
>>> it looks to me it's not hard to add them. If possible please add 
>>> them and "virtio" type then we can use the ops for both the case of 
>>> VM and containers.
>> sure
>
> Hello Jason,
>
> Just want to double confirm the implementation of set/get_config, for 
> now, dev_cfg only contains mac[6], status and max_virtqueue_pairs, is 
> that enough to support virtio_mdev?
>
> THanks!


Yes, and it depends on the features that you want to advertise. If you 
don't want to advertise MQ, there's no need to expose max_virtqueue_pairs.

Thanks