2020-08-26 14:16:47

by Diana Madalina Craciun

[permalink] [raw]
Subject: [PATCH v4 00/10] vfio/fsl-mc: VFIO support for FSL-MC device

DPAA2 (Data Path Acceleration Architecture) consists in
mechanisms for processing Ethernet packets, queue management,
accelerators, etc.

The Management Complex (mc) is a hardware entity that manages the DPAA2
hardware resources. It provides an object-based abstraction for software
drivers to use the DPAA2 hardware. The MC mediates operations such as
create, discover, destroy of DPAA2 objects.
The MC provides memory-mapped I/O command interfaces (MC portals) which
DPAA2 software drivers use to operate on DPAA2 objects.

A DPRC is a container object that holds other types of DPAA2 objects.
Each object in the DPRC is a Linux device and bound to a driver.
The MC-bus driver is a platform driver (different from PCI or platform
bus). The DPRC driver does runtime management of a bus instance. It
performs the initial scan of the DPRC and handles changes in the DPRC
configuration (adding/removing objects).

All objects inside a container share the same hardware isolation
context, meaning that only an entire DPRC can be assigned to
a virtual machine.
When a container is assigned to a virtual machine, all the objects
within that container are assigned to that virtual machine.
The DPRC container assigned to the virtual machine is not allowed
to change contents (add/remove objects) by the guest. The restriction
is set by the host and enforced by the mc hardware.

The DPAA2 objects can be directly assigned to the guest. However
the MC portals (the memory mapped command interface to the MC) need
to be emulated because there are commands that configure the
interrupts and the isolation IDs which are virtual in the guest.

Example:
echo vfio-fsl-mc > /sys/bus/fsl-mc/devices/dprc.2/driver_override
echo dprc.2 > /sys/bus/fsl-mc/drivers/vfio-fsl-mc/bind

The dprc.2 is bound to the VFIO driver and all the objects within
dprc.2 are going to be bound to the VFIO driver.

More details about the DPAA2 objects can be found here:
Documentation/networking/device_drivers/freescale/dpaa2/overview.rst

The patches are dependent on some changes in the mc-bus (bus/fsl-mc)
driver. The changes were needed in order to re-use code and to export
some more functions that are needed by the VFIO driver.
Currenlty the mc-bus patches are under review:
https://www.spinics.net/lists/kernel/msg3639226.html

v3 --> v4
- use bus provided functions to tear down the DPRC
- added reset support

v2 --> v3
- There is no need to align region size to page size
- read/write implemented for all DPAA2 objects
- review fixes

v1 --> v2
- Fixed the container reset, a new flag added to the firmware command
- Implement a bus notifier for setting driver_override


Bharat Bhushan (1):
vfio/fsl-mc: Add VFIO framework skeleton for fsl-mc devices

Diana Craciun (9):
vfio/fsl-mc: Scan DPRC objects on vfio-fsl-mc driver bind
vfio/fsl-mc: Implement VFIO_DEVICE_GET_INFO ioctl
vfio/fsl-mc: Implement VFIO_DEVICE_GET_REGION_INFO ioctl call
vfio/fsl-mc: Allow userspace to MMAP fsl-mc device MMIO regions
vfio/fsl-mc: Added lock support in preparation for interrupt handling
vfio/fsl-mc: Add irq infrastructure for fsl-mc devices
vfio/fsl-mc: trigger an interrupt via eventfd
vfio/fsl-mc: Add read/write support for fsl-mc devices
vfio/fsl-mc: Add support for device reset

MAINTAINERS | 6 +
drivers/vfio/Kconfig | 1 +
drivers/vfio/Makefile | 1 +
drivers/vfio/fsl-mc/Kconfig | 9 +
drivers/vfio/fsl-mc/Makefile | 4 +
drivers/vfio/fsl-mc/vfio_fsl_mc.c | 684 ++++++++++++++++++++++
drivers/vfio/fsl-mc/vfio_fsl_mc_intr.c | 221 +++++++
drivers/vfio/fsl-mc/vfio_fsl_mc_private.h | 56 ++
include/uapi/linux/vfio.h | 1 +
9 files changed, 983 insertions(+)
create mode 100644 drivers/vfio/fsl-mc/Kconfig
create mode 100644 drivers/vfio/fsl-mc/Makefile
create mode 100644 drivers/vfio/fsl-mc/vfio_fsl_mc.c
create mode 100644 drivers/vfio/fsl-mc/vfio_fsl_mc_intr.c
create mode 100644 drivers/vfio/fsl-mc/vfio_fsl_mc_private.h

--
2.17.1


2020-08-26 14:17:01

by Diana Madalina Craciun

[permalink] [raw]
Subject: [PATCH v4 08/10] vfio/fsl-mc: trigger an interrupt via eventfd

This patch allows to set an eventfd for fsl-mc device interrupts
and also to trigger the interrupt eventfd from userspace for testing.

All fsl-mc device interrupts are MSIs. The MSIs are allocated from
the MSI domain only once per DPRC and used by all the DPAA2 objects.
The interrupts are managed by the DPRC in a pool of interrupts. Each
device requests interrupts from this pool. The pool is allocated
when the first virtual device is setting the interrupts.
The pool of interrupts is protected by a lock.

The DPRC has an interrupt of its own which indicates if the DPRC
contents have changed. However, currently, the contents of a DPRC
assigned to the guest cannot be changed at runtime, so this interrupt
is not configured.

Signed-off-by: Bharat Bhushan <[email protected]>
Signed-off-by: Diana Craciun <[email protected]>
---
drivers/vfio/fsl-mc/vfio_fsl_mc.c | 18 ++-
drivers/vfio/fsl-mc/vfio_fsl_mc_intr.c | 160 +++++++++++++++++++++-
drivers/vfio/fsl-mc/vfio_fsl_mc_private.h | 10 ++
3 files changed, 186 insertions(+), 2 deletions(-)

diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
index 42014297b484..73834f488a94 100644
--- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
+++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
@@ -147,12 +147,28 @@ static int vfio_fsl_mc_open(void *device_data)
static void vfio_fsl_mc_release(void *device_data)
{
struct vfio_fsl_mc_device *vdev = device_data;
+ int ret;

mutex_lock(&vdev->reflck->lock);

- if (!(--vdev->refcnt))
+ if (!(--vdev->refcnt)) {
+ struct fsl_mc_device *mc_dev = vdev->mc_dev;
+ struct device *cont_dev = fsl_mc_cont_dev(&mc_dev->dev);
+ struct fsl_mc_device *mc_cont = to_fsl_mc_device(cont_dev);
+
vfio_fsl_mc_regions_cleanup(vdev);

+ /* reset the device before cleaning up the interrupts */
+ ret = dprc_reset_container(mc_cont->mc_io, 0,
+ mc_cont->mc_handle,
+ mc_cont->obj_desc.id,
+ DPRC_RESET_OPTION_NON_RECURSIVE);
+
+ vfio_fsl_mc_irqs_cleanup(vdev);
+
+ fsl_mc_cleanup_irq_pool(mc_cont);
+ }
+
mutex_unlock(&vdev->reflck->lock);

module_put(THIS_MODULE);
diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc_intr.c b/drivers/vfio/fsl-mc/vfio_fsl_mc_intr.c
index 058aa97aa54a..409f3507fcf3 100644
--- a/drivers/vfio/fsl-mc/vfio_fsl_mc_intr.c
+++ b/drivers/vfio/fsl-mc/vfio_fsl_mc_intr.c
@@ -29,12 +29,149 @@ static int vfio_fsl_mc_irq_unmask(struct vfio_fsl_mc_device *vdev,
return -EINVAL;
}

+int vfio_fsl_mc_irqs_allocate(struct vfio_fsl_mc_device *vdev)
+{
+ struct fsl_mc_device *mc_dev = vdev->mc_dev;
+ struct vfio_fsl_mc_irq *mc_irq;
+ int irq_count;
+ int ret, i;
+
+ /* Device does not support any interrupt */
+ if (mc_dev->obj_desc.irq_count == 0)
+ return 0;
+
+ /* interrupts were already allocated for this device */
+ if (vdev->mc_irqs)
+ return 0;
+
+ irq_count = mc_dev->obj_desc.irq_count;
+
+ mc_irq = kcalloc(irq_count, sizeof(*mc_irq), GFP_KERNEL);
+ if (!mc_irq)
+ return -ENOMEM;
+
+ /* Allocate IRQs */
+ ret = fsl_mc_allocate_irqs(mc_dev);
+ if (ret) {
+ kfree(mc_irq);
+ return ret;
+ }
+
+ for (i = 0; i < irq_count; i++) {
+ mc_irq[i].count = 1;
+ mc_irq[i].flags = VFIO_IRQ_INFO_EVENTFD;
+ }
+
+ vdev->mc_irqs = mc_irq;
+
+ return 0;
+}
+
+static irqreturn_t vfio_fsl_mc_irq_handler(int irq_num, void *arg)
+{
+ struct vfio_fsl_mc_irq *mc_irq = (struct vfio_fsl_mc_irq *)arg;
+
+ eventfd_signal(mc_irq->trigger, 1);
+ return IRQ_HANDLED;
+}
+
+static int vfio_set_trigger(struct vfio_fsl_mc_device *vdev,
+ int index, int fd)
+{
+ struct vfio_fsl_mc_irq *irq = &vdev->mc_irqs[index];
+ struct eventfd_ctx *trigger;
+ int hwirq;
+ int ret;
+
+ hwirq = vdev->mc_dev->irqs[index]->msi_desc->irq;
+ if (irq->trigger) {
+ free_irq(hwirq, irq);
+ kfree(irq->name);
+ eventfd_ctx_put(irq->trigger);
+ irq->trigger = NULL;
+ }
+
+ if (fd < 0) /* Disable only */
+ return 0;
+
+ irq->name = kasprintf(GFP_KERNEL, "vfio-irq[%d](%s)",
+ hwirq, dev_name(&vdev->mc_dev->dev));
+ if (!irq->name)
+ return -ENOMEM;
+
+ trigger = eventfd_ctx_fdget(fd);
+ if (IS_ERR(trigger)) {
+ kfree(irq->name);
+ return PTR_ERR(trigger);
+ }
+
+ irq->trigger = trigger;
+
+ ret = request_irq(hwirq, vfio_fsl_mc_irq_handler, 0,
+ irq->name, irq);
+ if (ret) {
+ kfree(irq->name);
+ eventfd_ctx_put(trigger);
+ irq->trigger = NULL;
+ return ret;
+ }
+
+ return 0;
+}
+
static int vfio_fsl_mc_set_irq_trigger(struct vfio_fsl_mc_device *vdev,
unsigned int index, unsigned int start,
unsigned int count, u32 flags,
void *data)
{
- return -EINVAL;
+ struct fsl_mc_device *mc_dev = vdev->mc_dev;
+ int ret, hwirq;
+ struct vfio_fsl_mc_irq *irq;
+ struct device *cont_dev = fsl_mc_cont_dev(&mc_dev->dev);
+ struct fsl_mc_device *mc_cont = to_fsl_mc_device(cont_dev);
+
+ if (start != 0 || count != 1)
+ return -EINVAL;
+
+ mutex_lock(&vdev->reflck->lock);
+ ret = fsl_mc_populate_irq_pool(mc_cont,
+ FSL_MC_IRQ_POOL_MAX_TOTAL_IRQS);
+ if (ret)
+ goto unlock;
+
+ ret = vfio_fsl_mc_irqs_allocate(vdev);
+ if (ret)
+ goto unlock;
+ mutex_unlock(&vdev->reflck->lock);
+
+ if (!count && (flags & VFIO_IRQ_SET_DATA_NONE))
+ return vfio_set_trigger(vdev, index, -1);
+
+ if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
+ s32 fd = *(s32 *)data;
+
+ return vfio_set_trigger(vdev, index, fd);
+ }
+
+ hwirq = vdev->mc_dev->irqs[index]->msi_desc->irq;
+
+ irq = &vdev->mc_irqs[index];
+
+ if (flags & VFIO_IRQ_SET_DATA_NONE) {
+ vfio_fsl_mc_irq_handler(hwirq, irq);
+
+ } else if (flags & VFIO_IRQ_SET_DATA_BOOL) {
+ u8 trigger = *(u8 *)data;
+
+ if (trigger)
+ vfio_fsl_mc_irq_handler(hwirq, irq);
+ }
+
+ return 0;
+
+unlock:
+ mutex_unlock(&vdev->reflck->lock);
+ return ret;
}

int vfio_fsl_mc_set_irqs_ioctl(struct vfio_fsl_mc_device *vdev,
@@ -61,3 +198,24 @@ int vfio_fsl_mc_set_irqs_ioctl(struct vfio_fsl_mc_device *vdev,

return ret;
}
+
+/* Free All IRQs for the given MC object */
+void vfio_fsl_mc_irqs_cleanup(struct vfio_fsl_mc_device *vdev)
+{
+ struct fsl_mc_device *mc_dev = vdev->mc_dev;
+ int irq_count = mc_dev->obj_desc.irq_count;
+ int i;
+
+ /* Device does not support any interrupt or the interrupts
+ * were not configured
+ */
+ if (mc_dev->obj_desc.irq_count == 0 || !vdev->mc_irqs)
+ return;
+
+ for (i = 0; i < irq_count; i++)
+ vfio_set_trigger(vdev, i, -1);
+
+ fsl_mc_free_irqs(mc_dev);
+ kfree(vdev->mc_irqs);
+ vdev->mc_irqs = NULL;
+}
diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h b/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
index d5b6fe891a48..bbfca8b55f8a 100644
--- a/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
+++ b/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
@@ -15,6 +15,13 @@
#define VFIO_FSL_MC_INDEX_TO_OFFSET(index) \
((u64)(index) << VFIO_FSL_MC_OFFSET_SHIFT)

+struct vfio_fsl_mc_irq {
+ u32 flags;
+ u32 count;
+ struct eventfd_ctx *trigger;
+ char *name;
+};
+
struct vfio_fsl_mc_reflck {
struct kref kref;
struct mutex lock;
@@ -35,6 +42,7 @@ struct vfio_fsl_mc_device {
struct vfio_fsl_mc_region *regions;
struct vfio_fsl_mc_reflck *reflck;
struct mutex igate;
+ struct vfio_fsl_mc_irq *mc_irqs;
};

extern int vfio_fsl_mc_set_irqs_ioctl(struct vfio_fsl_mc_device *vdev,
@@ -42,4 +50,6 @@ extern int vfio_fsl_mc_set_irqs_ioctl(struct vfio_fsl_mc_device *vdev,
unsigned int start, unsigned int count,
void *data);

+void vfio_fsl_mc_irqs_cleanup(struct vfio_fsl_mc_device *vdev);
+
#endif /* VFIO_FSL_MC_PRIVATE_H */
--
2.17.1

2020-08-26 14:18:28

by Diana Madalina Craciun

[permalink] [raw]
Subject: [PATCH v4 01/10] vfio/fsl-mc: Add VFIO framework skeleton for fsl-mc devices

From: Bharat Bhushan <[email protected]>

DPAA2 (Data Path Acceleration Architecture) consists in
mechanisms for processing Ethernet packets, queue management,
accelerators, etc.

The Management Complex (mc) is a hardware entity that manages the DPAA2
hardware resources. It provides an object-based abstraction for software
drivers to use the DPAA2 hardware. The MC mediates operations such as
create, discover, destroy of DPAA2 objects.
The MC provides memory-mapped I/O command interfaces (MC portals) which
DPAA2 software drivers use to operate on DPAA2 objects.

A DPRC is a container object that holds other types of DPAA2 objects.
Each object in the DPRC is a Linux device and bound to a driver.
The MC-bus driver is a platform driver (different from PCI or platform
bus). The DPRC driver does runtime management of a bus instance. It
performs the initial scan of the DPRC and handles changes in the DPRC
configuration (adding/removing objects).

All objects inside a container share the same hardware isolation
context, meaning that only an entire DPRC can be assigned to
a virtual machine.
When a container is assigned to a virtual machine, all the objects
within that container are assigned to that virtual machine.
The DPRC container assigned to the virtual machine is not allowed
to change contents (add/remove objects) by the guest. The restriction
is set by the host and enforced by the mc hardware.

The DPAA2 objects can be directly assigned to the guest. However
the MC portals (the memory mapped command interface to the MC) need
to be emulated because there are commands that configure the
interrupts and the isolation IDs which are virtual in the guest.

Example:
echo vfio-fsl-mc > /sys/bus/fsl-mc/devices/dprc.2/driver_override
echo dprc.2 > /sys/bus/fsl-mc/drivers/vfio-fsl-mc/bind

The dprc.2 is bound to the VFIO driver and all the objects within
dprc.2 are going to be bound to the VFIO driver.

This patch adds the infrastructure for VFIO support for fsl-mc
devices. Subsequent patches will add support for binding and secure
assigning these devices using VFIO.

More details about the DPAA2 objects can be found here:
Documentation/networking/device_drivers/freescale/dpaa2/overview.rst

Signed-off-by: Bharat Bhushan <[email protected]>
Signed-off-by: Diana Craciun <[email protected]>
---
MAINTAINERS | 6 +
drivers/vfio/Kconfig | 1 +
drivers/vfio/Makefile | 1 +
drivers/vfio/fsl-mc/Kconfig | 9 ++
drivers/vfio/fsl-mc/Makefile | 4 +
drivers/vfio/fsl-mc/vfio_fsl_mc.c | 160 ++++++++++++++++++++++
drivers/vfio/fsl-mc/vfio_fsl_mc_private.h | 14 ++
include/uapi/linux/vfio.h | 1 +
8 files changed, 196 insertions(+)
create mode 100644 drivers/vfio/fsl-mc/Kconfig
create mode 100644 drivers/vfio/fsl-mc/Makefile
create mode 100644 drivers/vfio/fsl-mc/vfio_fsl_mc.c
create mode 100644 drivers/vfio/fsl-mc/vfio_fsl_mc_private.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 3b186ade3597..f3f9ea108588 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18229,6 +18229,12 @@ F: drivers/vfio/
F: include/linux/vfio.h
F: include/uapi/linux/vfio.h

+VFIO FSL-MC DRIVER
+M: Diana Craciun <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/vfio/fsl-mc/
+
VFIO MEDIATED DEVICE DRIVERS
M: Kirti Wankhede <[email protected]>
L: [email protected]
diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
index fd17db9b432f..5533df91b257 100644
--- a/drivers/vfio/Kconfig
+++ b/drivers/vfio/Kconfig
@@ -47,4 +47,5 @@ menuconfig VFIO_NOIOMMU
source "drivers/vfio/pci/Kconfig"
source "drivers/vfio/platform/Kconfig"
source "drivers/vfio/mdev/Kconfig"
+source "drivers/vfio/fsl-mc/Kconfig"
source "virt/lib/Kconfig"
diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
index de67c4725cce..fee73f3d9480 100644
--- a/drivers/vfio/Makefile
+++ b/drivers/vfio/Makefile
@@ -9,3 +9,4 @@ obj-$(CONFIG_VFIO_SPAPR_EEH) += vfio_spapr_eeh.o
obj-$(CONFIG_VFIO_PCI) += pci/
obj-$(CONFIG_VFIO_PLATFORM) += platform/
obj-$(CONFIG_VFIO_MDEV) += mdev/
+obj-$(CONFIG_VFIO_FSL_MC) += fsl-mc/
diff --git a/drivers/vfio/fsl-mc/Kconfig b/drivers/vfio/fsl-mc/Kconfig
new file mode 100644
index 000000000000..b1a527d6b6f2
--- /dev/null
+++ b/drivers/vfio/fsl-mc/Kconfig
@@ -0,0 +1,9 @@
+config VFIO_FSL_MC
+ tristate "VFIO support for QorIQ DPAA2 fsl-mc bus devices"
+ depends on VFIO && FSL_MC_BUS && EVENTFD
+ help
+ Driver to enable support for the VFIO QorIQ DPAA2 fsl-mc
+ (Management Complex) devices. This is required to passthrough
+ fsl-mc bus devices using the VFIO framework.
+
+ If you don't know what to do here, say N.
diff --git a/drivers/vfio/fsl-mc/Makefile b/drivers/vfio/fsl-mc/Makefile
new file mode 100644
index 000000000000..0c6e5d2ddaae
--- /dev/null
+++ b/drivers/vfio/fsl-mc/Makefile
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
+
+vfio-fsl-mc-y := vfio_fsl_mc.o
+obj-$(CONFIG_VFIO_FSL_MC) += vfio-fsl-mc.o
diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
new file mode 100644
index 000000000000..8b53c2a25b32
--- /dev/null
+++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
@@ -0,0 +1,160 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
+/*
+ * Copyright 2013-2016 Freescale Semiconductor Inc.
+ * Copyright 2016-2017,2019-2020 NXP
+ */
+
+#include <linux/device.h>
+#include <linux/iommu.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/vfio.h>
+#include <linux/fsl/mc.h>
+
+#include "vfio_fsl_mc_private.h"
+
+static int vfio_fsl_mc_open(void *device_data)
+{
+ if (!try_module_get(THIS_MODULE))
+ return -ENODEV;
+
+ return 0;
+}
+
+static void vfio_fsl_mc_release(void *device_data)
+{
+ module_put(THIS_MODULE);
+}
+
+static long vfio_fsl_mc_ioctl(void *device_data, unsigned int cmd,
+ unsigned long arg)
+{
+ switch (cmd) {
+ case VFIO_DEVICE_GET_INFO:
+ {
+ return -ENOTTY;
+ }
+ case VFIO_DEVICE_GET_REGION_INFO:
+ {
+ return -ENOTTY;
+ }
+ case VFIO_DEVICE_GET_IRQ_INFO:
+ {
+ return -ENOTTY;
+ }
+ case VFIO_DEVICE_SET_IRQS:
+ {
+ return -ENOTTY;
+ }
+ case VFIO_DEVICE_RESET:
+ {
+ return -ENOTTY;
+ }
+ default:
+ return -ENOTTY;
+ }
+}
+
+static ssize_t vfio_fsl_mc_read(void *device_data, char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ return -EINVAL;
+}
+
+static ssize_t vfio_fsl_mc_write(void *device_data, const char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ return -EINVAL;
+}
+
+static int vfio_fsl_mc_mmap(void *device_data, struct vm_area_struct *vma)
+{
+ return -EINVAL;
+}
+
+static const struct vfio_device_ops vfio_fsl_mc_ops = {
+ .name = "vfio-fsl-mc",
+ .open = vfio_fsl_mc_open,
+ .release = vfio_fsl_mc_release,
+ .ioctl = vfio_fsl_mc_ioctl,
+ .read = vfio_fsl_mc_read,
+ .write = vfio_fsl_mc_write,
+ .mmap = vfio_fsl_mc_mmap,
+};
+
+static int vfio_fsl_mc_probe(struct fsl_mc_device *mc_dev)
+{
+ struct iommu_group *group;
+ struct vfio_fsl_mc_device *vdev;
+ struct device *dev = &mc_dev->dev;
+ int ret;
+
+ group = vfio_iommu_group_get(dev);
+ if (!group) {
+ dev_err(dev, "%s: VFIO: No IOMMU group\n", __func__);
+ return -EINVAL;
+ }
+
+ vdev = devm_kzalloc(dev, sizeof(*vdev), GFP_KERNEL);
+ if (!vdev) {
+ vfio_iommu_group_put(group, dev);
+ return -ENOMEM;
+ }
+
+ vdev->mc_dev = mc_dev;
+
+ ret = vfio_add_group_dev(dev, &vfio_fsl_mc_ops, vdev);
+ if (ret) {
+ dev_err(dev, "%s: Failed to add to vfio group\n", __func__);
+ vfio_iommu_group_put(group, dev);
+ return ret;
+ }
+
+ return ret;
+}
+
+static int vfio_fsl_mc_remove(struct fsl_mc_device *mc_dev)
+{
+ struct vfio_fsl_mc_device *vdev;
+ struct device *dev = &mc_dev->dev;
+
+ vdev = vfio_del_group_dev(dev);
+ if (!vdev)
+ return -EINVAL;
+
+ vfio_iommu_group_put(mc_dev->dev.iommu_group, dev);
+
+ return 0;
+}
+
+/*
+ * vfio-fsl_mc is a meta-driver, so use driver_override interface to
+ * bind a fsl_mc container with this driver and match_id_table is NULL.
+ */
+static struct fsl_mc_driver vfio_fsl_mc_driver = {
+ .probe = vfio_fsl_mc_probe,
+ .remove = vfio_fsl_mc_remove,
+ .match_id_table = NULL,
+ .driver = {
+ .name = "vfio-fsl-mc",
+ .owner = THIS_MODULE,
+ },
+};
+
+static int __init vfio_fsl_mc_driver_init(void)
+{
+ return fsl_mc_driver_register(&vfio_fsl_mc_driver);
+}
+
+static void __exit vfio_fsl_mc_driver_exit(void)
+{
+ fsl_mc_driver_unregister(&vfio_fsl_mc_driver);
+}
+
+module_init(vfio_fsl_mc_driver_init);
+module_exit(vfio_fsl_mc_driver_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("VFIO for FSL-MC devices - User Level meta-driver");
diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h b/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
new file mode 100644
index 000000000000..e79cc116f6b8
--- /dev/null
+++ b/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
+/*
+ * Copyright 2013-2016 Freescale Semiconductor Inc.
+ * Copyright 2016,2019-2020 NXP
+ */
+
+#ifndef VFIO_FSL_MC_PRIVATE_H
+#define VFIO_FSL_MC_PRIVATE_H
+
+struct vfio_fsl_mc_device {
+ struct fsl_mc_device *mc_dev;
+};
+
+#endif /* VFIO_FSL_MC_PRIVATE_H */
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 920470502329..95deac891378 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -201,6 +201,7 @@ struct vfio_device_info {
#define VFIO_DEVICE_FLAGS_AMBA (1 << 3) /* vfio-amba device */
#define VFIO_DEVICE_FLAGS_CCW (1 << 4) /* vfio-ccw device */
#define VFIO_DEVICE_FLAGS_AP (1 << 5) /* vfio-ap device */
+#define VFIO_DEVICE_FLAGS_FSL_MC (1 << 6) /* vfio-fsl-mc device */
__u32 num_regions; /* Max region index + 1 */
__u32 num_irqs; /* Max IRQ index + 1 */
};
--
2.17.1

2020-09-03 15:01:36

by Eric Auger

[permalink] [raw]
Subject: Re: [PATCH v4 01/10] vfio/fsl-mc: Add VFIO framework skeleton for fsl-mc devices

Hi Diana,

On 8/26/20 11:33 AM, Diana Craciun wrote:
> From: Bharat Bhushan <[email protected]>
>
> DPAA2 (Data Path Acceleration Architecture) consists in
> mechanisms for processing Ethernet packets, queue management,
> accelerators, etc.
>
> The Management Complex (mc) is a hardware entity that manages the DPAA2
> hardware resources. It provides an object-based abstraction for software
> drivers to use the DPAA2 hardware. The MC mediates operations such as
> create, discover, destroy of DPAA2 objects.
> The MC provides memory-mapped I/O command interfaces (MC portals) which
> DPAA2 software drivers use to operate on DPAA2 objects.
>
> A DPRC is a container object that holds other types of DPAA2 objects.
> Each object in the DPRC is a Linux device and bound to a driver.
> The MC-bus driver is a platform driver (different from PCI or platform
> bus). The DPRC driver does runtime management of a bus instance. It
> performs the initial scan of the DPRC and handles changes in the DPRC
> configuration (adding/removing objects).
>
> All objects inside a container share the same hardware isolation
> context, meaning that only an entire DPRC can be assigned to
> a virtual machine.
> When a container is assigned to a virtual machine, all the objects
> within that container are assigned to that virtual machine.
> The DPRC container assigned to the virtual machine is not allowed
> to change contents (add/remove objects) by the guest. The restriction
> is set by the host and enforced by the mc hardware.
>
> The DPAA2 objects can be directly assigned to the guest. However
> the MC portals (the memory mapped command interface to the MC) need
> to be emulated because there are commands that configure the
> interrupts and the isolation IDs which are virtual in the guest.
>
> Example:
> echo vfio-fsl-mc > /sys/bus/fsl-mc/devices/dprc.2/driver_override
> echo dprc.2 > /sys/bus/fsl-mc/drivers/vfio-fsl-mc/bind
>
> The dprc.2 is bound to the VFIO driver and all the objects within
> dprc.2 are going to be bound to the VFIO driver.
>
> This patch adds the infrastructure for VFIO support for fsl-mc
> devices. Subsequent patches will add support for binding and secure
> assigning these devices using VFIO.
>
> More details about the DPAA2 objects can be found here:
> Documentation/networking/device_drivers/freescale/dpaa2/overview.rst
>
> Signed-off-by: Bharat Bhushan <[email protected]>
> Signed-off-by: Diana Craciun <[email protected]>
> ---
> MAINTAINERS | 6 +
> drivers/vfio/Kconfig | 1 +
> drivers/vfio/Makefile | 1 +
> drivers/vfio/fsl-mc/Kconfig | 9 ++
> drivers/vfio/fsl-mc/Makefile | 4 +
> drivers/vfio/fsl-mc/vfio_fsl_mc.c | 160 ++++++++++++++++++++++
> drivers/vfio/fsl-mc/vfio_fsl_mc_private.h | 14 ++
> include/uapi/linux/vfio.h | 1 +
> 8 files changed, 196 insertions(+)
> create mode 100644 drivers/vfio/fsl-mc/Kconfig
> create mode 100644 drivers/vfio/fsl-mc/Makefile
> create mode 100644 drivers/vfio/fsl-mc/vfio_fsl_mc.c
> create mode 100644 drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3b186ade3597..f3f9ea108588 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18229,6 +18229,12 @@ F: drivers/vfio/
> F: include/linux/vfio.h
> F: include/uapi/linux/vfio.h
>
> +VFIO FSL-MC DRIVER
> +M: Diana Craciun <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: drivers/vfio/fsl-mc/
> +
> VFIO MEDIATED DEVICE DRIVERS
> M: Kirti Wankhede <[email protected]>
> L: [email protected]
> diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
> index fd17db9b432f..5533df91b257 100644
> --- a/drivers/vfio/Kconfig
> +++ b/drivers/vfio/Kconfig
> @@ -47,4 +47,5 @@ menuconfig VFIO_NOIOMMU
> source "drivers/vfio/pci/Kconfig"
> source "drivers/vfio/platform/Kconfig"
> source "drivers/vfio/mdev/Kconfig"
> +source "drivers/vfio/fsl-mc/Kconfig"
> source "virt/lib/Kconfig"
> diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
> index de67c4725cce..fee73f3d9480 100644
> --- a/drivers/vfio/Makefile
> +++ b/drivers/vfio/Makefile
> @@ -9,3 +9,4 @@ obj-$(CONFIG_VFIO_SPAPR_EEH) += vfio_spapr_eeh.o
> obj-$(CONFIG_VFIO_PCI) += pci/
> obj-$(CONFIG_VFIO_PLATFORM) += platform/
> obj-$(CONFIG_VFIO_MDEV) += mdev/
> +obj-$(CONFIG_VFIO_FSL_MC) += fsl-mc/
> diff --git a/drivers/vfio/fsl-mc/Kconfig b/drivers/vfio/fsl-mc/Kconfig
> new file mode 100644
> index 000000000000..b1a527d6b6f2
> --- /dev/null
> +++ b/drivers/vfio/fsl-mc/Kconfig
> @@ -0,0 +1,9 @@
> +config VFIO_FSL_MC
> + tristate "VFIO support for QorIQ DPAA2 fsl-mc bus devices"
> + depends on VFIO && FSL_MC_BUS && EVENTFD
> + help
> + Driver to enable support for the VFIO QorIQ DPAA2 fsl-mc
> + (Management Complex) devices. This is required to passthrough
> + fsl-mc bus devices using the VFIO framework.
> +
> + If you don't know what to do here, say N.
> diff --git a/drivers/vfio/fsl-mc/Makefile b/drivers/vfio/fsl-mc/Makefile
> new file mode 100644
> index 000000000000..0c6e5d2ddaae
> --- /dev/null
> +++ b/drivers/vfio/fsl-mc/Makefile
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> +
> +vfio-fsl-mc-y := vfio_fsl_mc.o
> +obj-$(CONFIG_VFIO_FSL_MC) += vfio-fsl-mc.o
> diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
> new file mode 100644
> index 000000000000..8b53c2a25b32
> --- /dev/null
> +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
> @@ -0,0 +1,160 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> +/*
> + * Copyright 2013-2016 Freescale Semiconductor Inc.
> + * Copyright 2016-2017,2019-2020 NXP
> + */
> +
> +#include <linux/device.h>
> +#include <linux/iommu.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/vfio.h>
> +#include <linux/fsl/mc.h>
> +
> +#include "vfio_fsl_mc_private.h"
> +
> +static int vfio_fsl_mc_open(void *device_data)
> +{
> + if (!try_module_get(THIS_MODULE))
> + return -ENODEV;
> +
> + return 0;
> +}
> +
> +static void vfio_fsl_mc_release(void *device_data)
> +{
> + module_put(THIS_MODULE);
> +}
> +
> +static long vfio_fsl_mc_ioctl(void *device_data, unsigned int cmd,
> + unsigned long arg)
> +{
> + switch (cmd) {
> + case VFIO_DEVICE_GET_INFO:
> + {
> + return -ENOTTY;
> + }
> + case VFIO_DEVICE_GET_REGION_INFO:
> + {
> + return -ENOTTY;
> + }
> + case VFIO_DEVICE_GET_IRQ_INFO:
> + {
> + return -ENOTTY;
> + }
> + case VFIO_DEVICE_SET_IRQS:
> + {
> + return -ENOTTY;
> + }
> + case VFIO_DEVICE_RESET:
> + {
> + return -ENOTTY;
> + }
> + default:
> + return -ENOTTY;
> + }
> +}
> +
> +static ssize_t vfio_fsl_mc_read(void *device_data, char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + return -EINVAL;
> +}
> +
> +static ssize_t vfio_fsl_mc_write(void *device_data, const char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + return -EINVAL;
> +}
> +
> +static int vfio_fsl_mc_mmap(void *device_data, struct vm_area_struct *vma)
> +{
> + return -EINVAL;
> +}
> +
> +static const struct vfio_device_ops vfio_fsl_mc_ops = {
> + .name = "vfio-fsl-mc",
> + .open = vfio_fsl_mc_open,
> + .release = vfio_fsl_mc_release,
> + .ioctl = vfio_fsl_mc_ioctl,
> + .read = vfio_fsl_mc_read,
> + .write = vfio_fsl_mc_write,
> + .mmap = vfio_fsl_mc_mmap,
> +};
> +
> +static int vfio_fsl_mc_probe(struct fsl_mc_device *mc_dev)
> +{
> + struct iommu_group *group;
> + struct vfio_fsl_mc_device *vdev;
> + struct device *dev = &mc_dev->dev;
> + int ret;
> +
> + group = vfio_iommu_group_get(dev);
> + if (!group) {
> + dev_err(dev, "%s: VFIO: No IOMMU group\n", __func__);
> + return -EINVAL;
> + }
> +
> + vdev = devm_kzalloc(dev, sizeof(*vdev), GFP_KERNEL);
> + if (!vdev) {
> + vfio_iommu_group_put(group, dev);
> + return -ENOMEM;
> + }
> +
> + vdev->mc_dev = mc_dev;
> +
> + ret = vfio_add_group_dev(dev, &vfio_fsl_mc_ops, vdev);
> + if (ret) {
> + dev_err(dev, "%s: Failed to add to vfio group\n", __func__);
> + vfio_iommu_group_put(group, dev);
> + return ret;
> + }
> +
> + return ret;
nit: introduce out_group_put: as in other files
This will be usable also in subsequent patches
> +}
> +
> +static int vfio_fsl_mc_remove(struct fsl_mc_device *mc_dev)
> +{
> + struct vfio_fsl_mc_device *vdev;
> + struct device *dev = &mc_dev->dev;
> +
> + vdev = vfio_del_group_dev(dev);
> + if (!vdev)
> + return -EINVAL;
> +
> + vfio_iommu_group_put(mc_dev->dev.iommu_group, dev);
> +
> + return 0;
> +}
> +
> +/*
> + * vfio-fsl_mc is a meta-driver, so use driver_override interface to
> + * bind a fsl_mc container with this driver and match_id_table is NULL.
> + */
> +static struct fsl_mc_driver vfio_fsl_mc_driver = {
> + .probe = vfio_fsl_mc_probe,
> + .remove = vfio_fsl_mc_remove,
> + .match_id_table = NULL,
not needed?
> + .driver = {
> + .name = "vfio-fsl-mc",
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +static int __init vfio_fsl_mc_driver_init(void)
> +{
> + return fsl_mc_driver_register(&vfio_fsl_mc_driver);
> +}
> +
> +static void __exit vfio_fsl_mc_driver_exit(void)
> +{
> + fsl_mc_driver_unregister(&vfio_fsl_mc_driver);
> +}
> +
> +module_init(vfio_fsl_mc_driver_init);
> +module_exit(vfio_fsl_mc_driver_exit);
> +
> +MODULE_LICENSE("GPL v2");
Don't you need MODULE_LICENSE("Dual BSD/GPL"); ?
> +MODULE_DESCRIPTION("VFIO for FSL-MC devices - User Level meta-driver");
> diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h b/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
> new file mode 100644
> index 000000000000..e79cc116f6b8
> --- /dev/null
> +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
> +/*
> + * Copyright 2013-2016 Freescale Semiconductor Inc.
> + * Copyright 2016,2019-2020 NXP
> + */
> +
> +#ifndef VFIO_FSL_MC_PRIVATE_H
> +#define VFIO_FSL_MC_PRIVATE_H
> +
> +struct vfio_fsl_mc_device {
> + struct fsl_mc_device *mc_dev;
> +};
> +
> +#endif /* VFIO_FSL_MC_PRIVATE_H */
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 920470502329..95deac891378 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -201,6 +201,7 @@ struct vfio_device_info {
> #define VFIO_DEVICE_FLAGS_AMBA (1 << 3) /* vfio-amba device */
> #define VFIO_DEVICE_FLAGS_CCW (1 << 4) /* vfio-ccw device */
> #define VFIO_DEVICE_FLAGS_AP (1 << 5) /* vfio-ap device */
> +#define VFIO_DEVICE_FLAGS_FSL_MC (1 << 6) /* vfio-fsl-mc device */
> __u32 num_regions; /* Max region index + 1 */
> __u32 num_irqs; /* Max IRQ index + 1 */
> };
>
Thanks

Eric

2020-09-03 15:06:31

by Eric Auger

[permalink] [raw]
Subject: Re: [PATCH v4 00/10] vfio/fsl-mc: VFIO support for FSL-MC device

Hi Diana,

On 8/26/20 11:33 AM, Diana Craciun wrote:
> DPAA2 (Data Path Acceleration Architecture) consists in
> mechanisms for processing Ethernet packets, queue management,
> accelerators, etc.
>
> The Management Complex (mc) is a hardware entity that manages the DPAA2
> hardware resources. It provides an object-based abstraction for software
> drivers to use the DPAA2 hardware. The MC mediates operations such as
> create, discover, destroy of DPAA2 objects.
> The MC provides memory-mapped I/O command interfaces (MC portals) which
> DPAA2 software drivers use to operate on DPAA2 objects.
>
> A DPRC is a container object that holds other types of DPAA2 objects.
> Each object in the DPRC is a Linux device and bound to a driver.
> The MC-bus driver is a platform driver (different from PCI or platform
> bus). The DPRC driver does runtime management of a bus instance. It
> performs the initial scan of the DPRC and handles changes in the DPRC
> configuration (adding/removing objects).
>
> All objects inside a container share the same hardware isolation
> context, meaning that only an entire DPRC can be assigned to
> a virtual machine.
> When a container is assigned to a virtual machine, all the objects
> within that container are assigned to that virtual machine.
> The DPRC container assigned to the virtual machine is not allowed
> to change contents (add/remove objects) by the guest. The restriction
> is set by the host and enforced by the mc hardware.
>
> The DPAA2 objects can be directly assigned to the guest. However
> the MC portals (the memory mapped command interface to the MC) need
> to be emulated because there are commands that configure the
> interrupts and the isolation IDs which are virtual in the guest.
>
> Example:
> echo vfio-fsl-mc > /sys/bus/fsl-mc/devices/dprc.2/driver_override
> echo dprc.2 > /sys/bus/fsl-mc/drivers/vfio-fsl-mc/bind
>
> The dprc.2 is bound to the VFIO driver and all the objects within
> dprc.2 are going to be bound to the VFIO driver.
>
> More details about the DPAA2 objects can be found here:
> Documentation/networking/device_drivers/freescale/dpaa2/overview.rst
>
> The patches are dependent on some changes in the mc-bus (bus/fsl-mc)
> driver. The changes were needed in order to re-use code and to export
> some more functions that are needed by the VFIO driver.
> Currenlty the mc-bus patches are under review:
> https://www.spinics.net/lists/kernel/msg3639226.html
Could you share a branch with both series? This would help the review.

Thanks

Eric
>
> v3 --> v4
> - use bus provided functions to tear down the DPRC
> - added reset support
>
> v2 --> v3
> - There is no need to align region size to page size
> - read/write implemented for all DPAA2 objects
> - review fixes
>
> v1 --> v2
> - Fixed the container reset, a new flag added to the firmware command
> - Implement a bus notifier for setting driver_override
>
>
> Bharat Bhushan (1):
> vfio/fsl-mc: Add VFIO framework skeleton for fsl-mc devices
>
> Diana Craciun (9):
> vfio/fsl-mc: Scan DPRC objects on vfio-fsl-mc driver bind
> vfio/fsl-mc: Implement VFIO_DEVICE_GET_INFO ioctl
> vfio/fsl-mc: Implement VFIO_DEVICE_GET_REGION_INFO ioctl call
> vfio/fsl-mc: Allow userspace to MMAP fsl-mc device MMIO regions
> vfio/fsl-mc: Added lock support in preparation for interrupt handling
> vfio/fsl-mc: Add irq infrastructure for fsl-mc devices
> vfio/fsl-mc: trigger an interrupt via eventfd
> vfio/fsl-mc: Add read/write support for fsl-mc devices
> vfio/fsl-mc: Add support for device reset
>
> MAINTAINERS | 6 +
> drivers/vfio/Kconfig | 1 +
> drivers/vfio/Makefile | 1 +
> drivers/vfio/fsl-mc/Kconfig | 9 +
> drivers/vfio/fsl-mc/Makefile | 4 +
> drivers/vfio/fsl-mc/vfio_fsl_mc.c | 684 ++++++++++++++++++++++
> drivers/vfio/fsl-mc/vfio_fsl_mc_intr.c | 221 +++++++
> drivers/vfio/fsl-mc/vfio_fsl_mc_private.h | 56 ++
> include/uapi/linux/vfio.h | 1 +
> 9 files changed, 983 insertions(+)
> create mode 100644 drivers/vfio/fsl-mc/Kconfig
> create mode 100644 drivers/vfio/fsl-mc/Makefile
> create mode 100644 drivers/vfio/fsl-mc/vfio_fsl_mc.c
> create mode 100644 drivers/vfio/fsl-mc/vfio_fsl_mc_intr.c
> create mode 100644 drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
>

2020-09-04 08:04:20

by Eric Auger

[permalink] [raw]
Subject: Re: [PATCH v4 08/10] vfio/fsl-mc: trigger an interrupt via eventfd

Hi Diana,

On 8/26/20 11:33 AM, Diana Craciun wrote:
> This patch allows to set an eventfd for fsl-mc device interrupts
> and also to trigger the interrupt eventfd from userspace for testing.
>
> All fsl-mc device interrupts are MSIs. The MSIs are allocated from
> the MSI domain only once per DPRC and used by all the DPAA2 objects.
> The interrupts are managed by the DPRC in a pool of interrupts. Each
> device requests interrupts from this pool. The pool is allocated
> when the first virtual device is setting the interrupts.
> The pool of interrupts is protected by a lock.
>
> The DPRC has an interrupt of its own which indicates if the DPRC
> contents have changed. However, currently, the contents of a DPRC
> assigned to the guest cannot be changed at runtime, so this interrupt
> is not configured.
>
> Signed-off-by: Bharat Bhushan <[email protected]>
> Signed-off-by: Diana Craciun <[email protected]>
> ---
> drivers/vfio/fsl-mc/vfio_fsl_mc.c | 18 ++-
> drivers/vfio/fsl-mc/vfio_fsl_mc_intr.c | 160 +++++++++++++++++++++-
> drivers/vfio/fsl-mc/vfio_fsl_mc_private.h | 10 ++
> 3 files changed, 186 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
> index 42014297b484..73834f488a94 100644
> --- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
> +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
> @@ -147,12 +147,28 @@ static int vfio_fsl_mc_open(void *device_data)
> static void vfio_fsl_mc_release(void *device_data)
> {
> struct vfio_fsl_mc_device *vdev = device_data;
> + int ret;
>
> mutex_lock(&vdev->reflck->lock);
>
> - if (!(--vdev->refcnt))
> + if (!(--vdev->refcnt)) {
> + struct fsl_mc_device *mc_dev = vdev->mc_dev;
> + struct device *cont_dev = fsl_mc_cont_dev(&mc_dev->dev);
> + struct fsl_mc_device *mc_cont = to_fsl_mc_device(cont_dev);
> +
> vfio_fsl_mc_regions_cleanup(vdev);
>
> + /* reset the device before cleaning up the interrupts */
> + ret = dprc_reset_container(mc_cont->mc_io, 0,
> + mc_cont->mc_handle,
> + mc_cont->obj_desc.id,
> + DPRC_RESET_OPTION_NON_RECURSIVE);
shouldn't you test ret?
> +
> + vfio_fsl_mc_irqs_cleanup(vdev);
> +
> + fsl_mc_cleanup_irq_pool(mc_cont);
> + }
> +
> mutex_unlock(&vdev->reflck->lock);
>
> module_put(THIS_MODULE);
> diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc_intr.c b/drivers/vfio/fsl-mc/vfio_fsl_mc_intr.c
> index 058aa97aa54a..409f3507fcf3 100644
> --- a/drivers/vfio/fsl-mc/vfio_fsl_mc_intr.c
> +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc_intr.c
> @@ -29,12 +29,149 @@ static int vfio_fsl_mc_irq_unmask(struct vfio_fsl_mc_device *vdev,
> return -EINVAL;
> }
>
> +int vfio_fsl_mc_irqs_allocate(struct vfio_fsl_mc_device *vdev)
> +{
> + struct fsl_mc_device *mc_dev = vdev->mc_dev;
> + struct vfio_fsl_mc_irq *mc_irq;
> + int irq_count;
> + int ret, i;
> +
> + /* Device does not support any interrupt */
indent needs to be fixed
> + if (mc_dev->obj_desc.irq_count == 0)
> + return 0;
> +
> + /* interrupts were already allocated for this device */
> + if (vdev->mc_irqs)
> + return 0;
> +
> + irq_count = mc_dev->obj_desc.irq_count;
> +
> + mc_irq = kcalloc(irq_count, sizeof(*mc_irq), GFP_KERNEL);
> + if (!mc_irq)
> + return -ENOMEM;
> +
> + /* Allocate IRQs */
> + ret = fsl_mc_allocate_irqs(mc_dev);
> + if (ret) {
> + kfree(mc_irq);
> + return ret;
> + }
> +
> + for (i = 0; i < irq_count; i++) {
> + mc_irq[i].count = 1;
> + mc_irq[i].flags = VFIO_IRQ_INFO_EVENTFD;
> + }
> +
> + vdev->mc_irqs = mc_irq;
> +
> + return 0;
> +}
> +
> +static irqreturn_t vfio_fsl_mc_irq_handler(int irq_num, void *arg)
> +{
> + struct vfio_fsl_mc_irq *mc_irq = (struct vfio_fsl_mc_irq *)arg;
> +
> + eventfd_signal(mc_irq->trigger, 1);
> + return IRQ_HANDLED;
> +}
> +
> +static int vfio_set_trigger(struct vfio_fsl_mc_device *vdev,
> + int index, int fd)
> +{
> + struct vfio_fsl_mc_irq *irq = &vdev->mc_irqs[index];
> + struct eventfd_ctx *trigger;
> + int hwirq;
> + int ret;
> +
> + hwirq = vdev->mc_dev->irqs[index]->msi_desc->irq;
> + if (irq->trigger) {
> + free_irq(hwirq, irq);
> + kfree(irq->name);
> + eventfd_ctx_put(irq->trigger);
> + irq->trigger = NULL;
> + }
> +
> + if (fd < 0) /* Disable only */
> + return 0;
> +
> + irq->name = kasprintf(GFP_KERNEL, "vfio-irq[%d](%s)",
> + hwirq, dev_name(&vdev->mc_dev->dev));
> + if (!irq->name)
> + return -ENOMEM;
> +
> + trigger = eventfd_ctx_fdget(fd);
> + if (IS_ERR(trigger)) {
> + kfree(irq->name);
> + return PTR_ERR(trigger);
> + }
> +
> + irq->trigger = trigger;
> +
> + ret = request_irq(hwirq, vfio_fsl_mc_irq_handler, 0,
> + irq->name, irq);
> + if (ret) {
> + kfree(irq->name);
> + eventfd_ctx_put(trigger);
> + irq->trigger = NULL;
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> static int vfio_fsl_mc_set_irq_trigger(struct vfio_fsl_mc_device *vdev,
> unsigned int index, unsigned int start,
> unsigned int count, u32 flags,
> void *data)
> {
> - return -EINVAL;
> + struct fsl_mc_device *mc_dev = vdev->mc_dev;
> + int ret, hwirq;
> + struct vfio_fsl_mc_irq *irq;
> + struct device *cont_dev = fsl_mc_cont_dev(&mc_dev->dev);
> + struct fsl_mc_device *mc_cont = to_fsl_mc_device(cont_dev);
> +
> + if (start != 0 || count != 1)
> + return -EINVAL;
> +
> + mutex_lock(&vdev->reflck->lock);
> + ret = fsl_mc_populate_irq_pool(mc_cont,
> + FSL_MC_IRQ_POOL_MAX_TOTAL_IRQS);
> + if (ret)
> + goto unlock;
> +
> + ret = vfio_fsl_mc_irqs_allocate(vdev);
any reason the init is done in the set_irq() and not in the open() if
!vdev->refcnt?
> + if (ret)
> + goto unlock;
> + mutex_unlock(&vdev->reflck->lock);
> +
> + if (!count && (flags & VFIO_IRQ_SET_DATA_NONE))
> + return vfio_set_trigger(vdev, index, -1);
> +
> + if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
> + s32 fd = *(s32 *)data;
> +
> + return vfio_set_trigger(vdev, index, fd);
> + }
> +
> + hwirq = vdev->mc_dev->irqs[index]->msi_desc->irq;
> +
> + irq = &vdev->mc_irqs[index];
> +
> + if (flags & VFIO_IRQ_SET_DATA_NONE) {
> + vfio_fsl_mc_irq_handler(hwirq, irq);
> +
> + } else if (flags & VFIO_IRQ_SET_DATA_BOOL) {
> + u8 trigger = *(u8 *)data;
> +
> + if (trigger)
> + vfio_fsl_mc_irq_handler(hwirq, irq);
> + }
> +
> + return 0;
> +
> +unlock:
> + mutex_unlock(&vdev->reflck->lock);
> + return ret;
> }
>
> int vfio_fsl_mc_set_irqs_ioctl(struct vfio_fsl_mc_device *vdev,
> @@ -61,3 +198,24 @@ int vfio_fsl_mc_set_irqs_ioctl(struct vfio_fsl_mc_device *vdev,
>
> return ret;
> }
> +
> +/* Free All IRQs for the given MC object */
> +void vfio_fsl_mc_irqs_cleanup(struct vfio_fsl_mc_device *vdev)
> +{
> + struct fsl_mc_device *mc_dev = vdev->mc_dev;
> + int irq_count = mc_dev->obj_desc.irq_count;
> + int i;
> +
> + /* Device does not support any interrupt or the interrupts
> + * were not configured
> + */
> + if (mc_dev->obj_desc.irq_count == 0 || !vdev->mc_irqs)
> + return;
> +
> + for (i = 0; i < irq_count; i++)
> + vfio_set_trigger(vdev, i, -1);
> +
> + fsl_mc_free_irqs(mc_dev);
> + kfree(vdev->mc_irqs);
> + vdev->mc_irqs = NULL;
> +}
> diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h b/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
> index d5b6fe891a48..bbfca8b55f8a 100644
> --- a/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
> +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
> @@ -15,6 +15,13 @@
> #define VFIO_FSL_MC_INDEX_TO_OFFSET(index) \
> ((u64)(index) << VFIO_FSL_MC_OFFSET_SHIFT)
>
> +struct vfio_fsl_mc_irq {
> + u32 flags;
> + u32 count;
> + struct eventfd_ctx *trigger;
> + char *name;
> +};
> +
> struct vfio_fsl_mc_reflck {
> struct kref kref;
> struct mutex lock;
> @@ -35,6 +42,7 @@ struct vfio_fsl_mc_device {
> struct vfio_fsl_mc_region *regions;
> struct vfio_fsl_mc_reflck *reflck;
> struct mutex igate;
> + struct vfio_fsl_mc_irq *mc_irqs;
> };
>
> extern int vfio_fsl_mc_set_irqs_ioctl(struct vfio_fsl_mc_device *vdev,
> @@ -42,4 +50,6 @@ extern int vfio_fsl_mc_set_irqs_ioctl(struct vfio_fsl_mc_device *vdev,
> unsigned int start, unsigned int count,
> void *data);
>
> +void vfio_fsl_mc_irqs_cleanup(struct vfio_fsl_mc_device *vdev);
> +
> #endif /* VFIO_FSL_MC_PRIVATE_H */
>
Thanks

Eric

2020-09-04 08:05:26

by Diana Madalina Craciun

[permalink] [raw]
Subject: Re: [PATCH v4 00/10] vfio/fsl-mc: VFIO support for FSL-MC device

Hi Eric,

On 9/3/2020 4:40 PM, Auger Eric wrote:
>> The patches are dependent on some changes in the mc-bus (bus/fsl-mc)
>> driver. The changes were needed in order to re-use code and to export
>> some more functions that are needed by the VFIO driver. Currenlty the
>> mc-bus patches are under review:
>> https://www.spinics.net/lists/kernel/msg3639226.html
> Could you share a branch with both series? This would help the review.
> Thanks Eric

I have pushed both the series here:
https://source.codeaurora.org/external/qoriq/qoriq-components/linux-extras/log/?h=dpaa2_direct_assignment


Regards,

Diana

PS: I apologize if you received the message twice, I have sent it by
mistake as html first.

2020-09-04 08:27:08

by Eric Auger

[permalink] [raw]
Subject: Re: [PATCH v4 00/10] vfio/fsl-mc: VFIO support for FSL-MC device

Hi Diana,

On 9/4/20 10:03 AM, Diana Craciun OSS wrote:
> Hi Eric,
>
> On 9/3/2020 4:40 PM, Auger Eric wrote:
>>> The patches are dependent on some changes in the mc-bus (bus/fsl-mc)
>>> driver. The changes were needed in order to re-use code and to export
>>> some more functions that are needed by the VFIO driver. Currenlty the
>>> mc-bus patches are under review:
>>> https://www.spinics.net/lists/kernel/msg3639226.html
>> Could you share a branch with both series? This would help the review.
>> Thanks Eric
>
> I have pushed both the series here:
> https://source.codeaurora.org/external/qoriq/qoriq-components/linux-extras/log/?h=dpaa2_direct_assignment
>
>
> Regards,
>
> Diana
>
> PS: I apologize if you received the message twice, I have sent it by
> mistake as html first.
>
No Problem. Thank you for the branch. I have completed a first review
pass. Hope this helps.

Thanks

Eric

2020-09-04 14:02:32

by Diana Madalina Craciun

[permalink] [raw]
Subject: Re: [PATCH v4 01/10] vfio/fsl-mc: Add VFIO framework skeleton for fsl-mc devices

Hi Eric,

On 9/3/2020 5:06 PM, Auger Eric wrote:
> Hi Diana,
>
> On 8/26/20 11:33 AM, Diana Craciun wrote:
>> From: Bharat Bhushan <[email protected]>
>>
>> DPAA2 (Data Path Acceleration Architecture) consists in
>> mechanisms for processing Ethernet packets, queue management,
>> accelerators, etc.
>>
>> The Management Complex (mc) is a hardware entity that manages the DPAA2
>> hardware resources. It provides an object-based abstraction for software
>> drivers to use the DPAA2 hardware. The MC mediates operations such as
>> create, discover, destroy of DPAA2 objects.
>> The MC provides memory-mapped I/O command interfaces (MC portals) which
>> DPAA2 software drivers use to operate on DPAA2 objects.
>>
>> A DPRC is a container object that holds other types of DPAA2 objects.
>> Each object in the DPRC is a Linux device and bound to a driver.
>> The MC-bus driver is a platform driver (different from PCI or platform
>> bus). The DPRC driver does runtime management of a bus instance. It
>> performs the initial scan of the DPRC and handles changes in the DPRC
>> configuration (adding/removing objects).
>>
>> All objects inside a container share the same hardware isolation
>> context, meaning that only an entire DPRC can be assigned to
>> a virtual machine.
>> When a container is assigned to a virtual machine, all the objects
>> within that container are assigned to that virtual machine.
>> The DPRC container assigned to the virtual machine is not allowed
>> to change contents (add/remove objects) by the guest. The restriction
>> is set by the host and enforced by the mc hardware.
>>
>> The DPAA2 objects can be directly assigned to the guest. However
>> the MC portals (the memory mapped command interface to the MC) need
>> to be emulated because there are commands that configure the
>> interrupts and the isolation IDs which are virtual in the guest.
>>
>> Example:
>> echo vfio-fsl-mc > /sys/bus/fsl-mc/devices/dprc.2/driver_override
>> echo dprc.2 > /sys/bus/fsl-mc/drivers/vfio-fsl-mc/bind
>>
>> The dprc.2 is bound to the VFIO driver and all the objects within
>> dprc.2 are going to be bound to the VFIO driver.
>>
>> This patch adds the infrastructure for VFIO support for fsl-mc
>> devices. Subsequent patches will add support for binding and secure
>> assigning these devices using VFIO.
>>
>> More details about the DPAA2 objects can be found here:
>> Documentation/networking/device_drivers/freescale/dpaa2/overview.rst
>>
>> Signed-off-by: Bharat Bhushan <[email protected]>
>> Signed-off-by: Diana Craciun <[email protected]>
>> ---
>> MAINTAINERS | 6 +
>> drivers/vfio/Kconfig | 1 +
>> drivers/vfio/Makefile | 1 +
>> drivers/vfio/fsl-mc/Kconfig | 9 ++
>> drivers/vfio/fsl-mc/Makefile | 4 +
>> drivers/vfio/fsl-mc/vfio_fsl_mc.c | 160 ++++++++++++++++++++++
>> drivers/vfio/fsl-mc/vfio_fsl_mc_private.h | 14 ++
>> include/uapi/linux/vfio.h | 1 +
>> 8 files changed, 196 insertions(+)
>> create mode 100644 drivers/vfio/fsl-mc/Kconfig
>> create mode 100644 drivers/vfio/fsl-mc/Makefile
>> create mode 100644 drivers/vfio/fsl-mc/vfio_fsl_mc.c
>> create mode 100644 drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 3b186ade3597..f3f9ea108588 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -18229,6 +18229,12 @@ F: drivers/vfio/
>> F: include/linux/vfio.h
>> F: include/uapi/linux/vfio.h
>>
>> +VFIO FSL-MC DRIVER
>> +M: Diana Craciun <[email protected]>
>> +L: [email protected]
>> +S: Maintained
>> +F: drivers/vfio/fsl-mc/
>> +
>> VFIO MEDIATED DEVICE DRIVERS
>> M: Kirti Wankhede <[email protected]>
>> L: [email protected]
>> diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
>> index fd17db9b432f..5533df91b257 100644
>> --- a/drivers/vfio/Kconfig
>> +++ b/drivers/vfio/Kconfig
>> @@ -47,4 +47,5 @@ menuconfig VFIO_NOIOMMU
>> source "drivers/vfio/pci/Kconfig"
>> source "drivers/vfio/platform/Kconfig"
>> source "drivers/vfio/mdev/Kconfig"
>> +source "drivers/vfio/fsl-mc/Kconfig"
>> source "virt/lib/Kconfig"
>> diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
>> index de67c4725cce..fee73f3d9480 100644
>> --- a/drivers/vfio/Makefile
>> +++ b/drivers/vfio/Makefile
>> @@ -9,3 +9,4 @@ obj-$(CONFIG_VFIO_SPAPR_EEH) += vfio_spapr_eeh.o
>> obj-$(CONFIG_VFIO_PCI) += pci/
>> obj-$(CONFIG_VFIO_PLATFORM) += platform/
>> obj-$(CONFIG_VFIO_MDEV) += mdev/
>> +obj-$(CONFIG_VFIO_FSL_MC) += fsl-mc/
>> diff --git a/drivers/vfio/fsl-mc/Kconfig b/drivers/vfio/fsl-mc/Kconfig
>> new file mode 100644
>> index 000000000000..b1a527d6b6f2
>> --- /dev/null
>> +++ b/drivers/vfio/fsl-mc/Kconfig
>> @@ -0,0 +1,9 @@
>> +config VFIO_FSL_MC
>> + tristate "VFIO support for QorIQ DPAA2 fsl-mc bus devices"
>> + depends on VFIO && FSL_MC_BUS && EVENTFD
>> + help
>> + Driver to enable support for the VFIO QorIQ DPAA2 fsl-mc
>> + (Management Complex) devices. This is required to passthrough
>> + fsl-mc bus devices using the VFIO framework.
>> +
>> + If you don't know what to do here, say N.
>> diff --git a/drivers/vfio/fsl-mc/Makefile b/drivers/vfio/fsl-mc/Makefile
>> new file mode 100644
>> index 000000000000..0c6e5d2ddaae
>> --- /dev/null
>> +++ b/drivers/vfio/fsl-mc/Makefile
>> @@ -0,0 +1,4 @@
>> +# SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
>> +
>> +vfio-fsl-mc-y := vfio_fsl_mc.o
>> +obj-$(CONFIG_VFIO_FSL_MC) += vfio-fsl-mc.o
>> diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
>> new file mode 100644
>> index 000000000000..8b53c2a25b32
>> --- /dev/null
>> +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
>> @@ -0,0 +1,160 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
>> +/*
>> + * Copyright 2013-2016 Freescale Semiconductor Inc.
>> + * Copyright 2016-2017,2019-2020 NXP
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/iommu.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/slab.h>
>> +#include <linux/types.h>
>> +#include <linux/vfio.h>
>> +#include <linux/fsl/mc.h>
>> +
>> +#include "vfio_fsl_mc_private.h"
>> +
>> +static int vfio_fsl_mc_open(void *device_data)
>> +{
>> + if (!try_module_get(THIS_MODULE))
>> + return -ENODEV;
>> +
>> + return 0;
>> +}
>> +
>> +static void vfio_fsl_mc_release(void *device_data)
>> +{
>> + module_put(THIS_MODULE);
>> +}
>> +
>> +static long vfio_fsl_mc_ioctl(void *device_data, unsigned int cmd,
>> + unsigned long arg)
>> +{
>> + switch (cmd) {
>> + case VFIO_DEVICE_GET_INFO:
>> + {
>> + return -ENOTTY;
>> + }
>> + case VFIO_DEVICE_GET_REGION_INFO:
>> + {
>> + return -ENOTTY;
>> + }
>> + case VFIO_DEVICE_GET_IRQ_INFO:
>> + {
>> + return -ENOTTY;
>> + }
>> + case VFIO_DEVICE_SET_IRQS:
>> + {
>> + return -ENOTTY;
>> + }
>> + case VFIO_DEVICE_RESET:
>> + {
>> + return -ENOTTY;
>> + }
>> + default:
>> + return -ENOTTY;
>> + }
>> +}
>> +
>> +static ssize_t vfio_fsl_mc_read(void *device_data, char __user *buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + return -EINVAL;
>> +}
>> +
>> +static ssize_t vfio_fsl_mc_write(void *device_data, const char __user *buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + return -EINVAL;
>> +}
>> +
>> +static int vfio_fsl_mc_mmap(void *device_data, struct vm_area_struct *vma)
>> +{
>> + return -EINVAL;
>> +}
>> +
>> +static const struct vfio_device_ops vfio_fsl_mc_ops = {
>> + .name = "vfio-fsl-mc",
>> + .open = vfio_fsl_mc_open,
>> + .release = vfio_fsl_mc_release,
>> + .ioctl = vfio_fsl_mc_ioctl,
>> + .read = vfio_fsl_mc_read,
>> + .write = vfio_fsl_mc_write,
>> + .mmap = vfio_fsl_mc_mmap,
>> +};
>> +
>> +static int vfio_fsl_mc_probe(struct fsl_mc_device *mc_dev)
>> +{
>> + struct iommu_group *group;
>> + struct vfio_fsl_mc_device *vdev;
>> + struct device *dev = &mc_dev->dev;
>> + int ret;
>> +
>> + group = vfio_iommu_group_get(dev);
>> + if (!group) {
>> + dev_err(dev, "%s: VFIO: No IOMMU group\n", __func__);
>> + return -EINVAL;
>> + }
>> +
>> + vdev = devm_kzalloc(dev, sizeof(*vdev), GFP_KERNEL);
>> + if (!vdev) {
>> + vfio_iommu_group_put(group, dev);
>> + return -ENOMEM;
>> + }
>> +
>> + vdev->mc_dev = mc_dev;
>> +
>> + ret = vfio_add_group_dev(dev, &vfio_fsl_mc_ops, vdev);
>> + if (ret) {
>> + dev_err(dev, "%s: Failed to add to vfio group\n", __func__);
>> + vfio_iommu_group_put(group, dev);
>> + return ret;
>> + }
>> +
>> + return ret;
> nit: introduce out_group_put: as in other files
> This will be usable also in subsequent patches
>> +}
>> +
>> +static int vfio_fsl_mc_remove(struct fsl_mc_device *mc_dev)
>> +{
>> + struct vfio_fsl_mc_device *vdev;
>> + struct device *dev = &mc_dev->dev;
>> +
>> + vdev = vfio_del_group_dev(dev);
>> + if (!vdev)
>> + return -EINVAL;
>> +
>> + vfio_iommu_group_put(mc_dev->dev.iommu_group, dev);
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * vfio-fsl_mc is a meta-driver, so use driver_override interface to
>> + * bind a fsl_mc container with this driver and match_id_table is NULL.
>> + */
>> +static struct fsl_mc_driver vfio_fsl_mc_driver = {
>> + .probe = vfio_fsl_mc_probe,
>> + .remove = vfio_fsl_mc_remove,
>> + .match_id_table = NULL,
> not needed?

The driver will always match on driver_override, there is no need for
static ids.

>> + .driver = {
>> + .name = "vfio-fsl-mc",
>> + .owner = THIS_MODULE,
>> + },
>> +};
>> +
>> +static int __init vfio_fsl_mc_driver_init(void)
>> +{
>> + return fsl_mc_driver_register(&vfio_fsl_mc_driver);
>> +}
>> +
>> +static void __exit vfio_fsl_mc_driver_exit(void)
>> +{
>> + fsl_mc_driver_unregister(&vfio_fsl_mc_driver);
>> +}
>> +
>> +module_init(vfio_fsl_mc_driver_init);
>> +module_exit(vfio_fsl_mc_driver_exit);
>> +
>> +MODULE_LICENSE("GPL v2");
> Don't you need MODULE_LICENSE("Dual BSD/GPL"); ?

Yes, will change.

>> +MODULE_DESCRIPTION("VFIO for FSL-MC devices - User Level meta-driver");
>> diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h b/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
>> new file mode 100644
>> index 000000000000..e79cc116f6b8
>> --- /dev/null
>> +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
>> @@ -0,0 +1,14 @@
>> +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
>> +/*
>> + * Copyright 2013-2016 Freescale Semiconductor Inc.
>> + * Copyright 2016,2019-2020 NXP
>> + */
>> +
>> +#ifndef VFIO_FSL_MC_PRIVATE_H
>> +#define VFIO_FSL_MC_PRIVATE_H
>> +
>> +struct vfio_fsl_mc_device {
>> + struct fsl_mc_device *mc_dev;
>> +};
>> +
>> +#endif /* VFIO_FSL_MC_PRIVATE_H */
>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> index 920470502329..95deac891378 100644
>> --- a/include/uapi/linux/vfio.h
>> +++ b/include/uapi/linux/vfio.h
>> @@ -201,6 +201,7 @@ struct vfio_device_info {
>> #define VFIO_DEVICE_FLAGS_AMBA (1 << 3) /* vfio-amba device */
>> #define VFIO_DEVICE_FLAGS_CCW (1 << 4) /* vfio-ccw device */
>> #define VFIO_DEVICE_FLAGS_AP (1 << 5) /* vfio-ap device */
>> +#define VFIO_DEVICE_FLAGS_FSL_MC (1 << 6) /* vfio-fsl-mc device */
>> __u32 num_regions; /* Max region index + 1 */
>> __u32 num_irqs; /* Max IRQ index + 1 */
>> };
>>
> Thanks
>
> Eric
>

Thanks,
Diana

2020-09-04 14:17:13

by Eric Auger

[permalink] [raw]
Subject: Re: [PATCH v4 01/10] vfio/fsl-mc: Add VFIO framework skeleton for fsl-mc devices

Hi Diana,

On 9/4/20 3:59 PM, Diana Craciun OSS wrote:
> Hi Eric,
>
> On 9/3/2020 5:06 PM, Auger Eric wrote:
>> Hi Diana,
>>
>> On 8/26/20 11:33 AM, Diana Craciun wrote:
>>> From: Bharat Bhushan <[email protected]>
>>>
>>> DPAA2 (Data Path Acceleration Architecture) consists in
>>> mechanisms for processing Ethernet packets, queue management,
>>> accelerators, etc.
>>>
>>> The Management Complex (mc) is a hardware entity that manages the DPAA2
>>> hardware resources. It provides an object-based abstraction for software
>>> drivers to use the DPAA2 hardware. The MC mediates operations such as
>>> create, discover, destroy of DPAA2 objects.
>>> The MC provides memory-mapped I/O command interfaces (MC portals) which
>>> DPAA2 software drivers use to operate on DPAA2 objects.
>>>
>>> A DPRC is a container object that holds other types of DPAA2 objects.
>>> Each object in the DPRC is a Linux device and bound to a driver.
>>> The MC-bus driver is a platform driver (different from PCI or platform
>>> bus). The DPRC driver does runtime management of a bus instance. It
>>> performs the initial scan of the DPRC and handles changes in the DPRC
>>> configuration (adding/removing objects).
>>>
>>> All objects inside a container share the same hardware isolation
>>> context, meaning that only an entire DPRC can be assigned to
>>> a virtual machine.
>>> When a container is assigned to a virtual machine, all the objects
>>> within that container are assigned to that virtual machine.
>>> The DPRC container assigned to the virtual machine is not allowed
>>> to change contents (add/remove objects) by the guest. The restriction
>>> is set by the host and enforced by the mc hardware.
>>>
>>> The DPAA2 objects can be directly assigned to the guest. However
>>> the MC portals (the memory mapped command interface to the MC) need
>>> to be emulated because there are commands that configure the
>>> interrupts and the isolation IDs which are virtual in the guest.
>>>
>>> Example:
>>> echo vfio-fsl-mc > /sys/bus/fsl-mc/devices/dprc.2/driver_override
>>> echo dprc.2 > /sys/bus/fsl-mc/drivers/vfio-fsl-mc/bind
>>>
>>> The dprc.2 is bound to the VFIO driver and all the objects within
>>> dprc.2 are going to be bound to the VFIO driver.
>>>
>>> This patch adds the infrastructure for VFIO support for fsl-mc
>>> devices. Subsequent patches will add support for binding and secure
>>> assigning these devices using VFIO.
>>>
>>> More details about the DPAA2 objects can be found here:
>>> Documentation/networking/device_drivers/freescale/dpaa2/overview.rst
>>>
>>> Signed-off-by: Bharat Bhushan <[email protected]>
>>> Signed-off-by: Diana Craciun <[email protected]>
>>> ---
>>>   MAINTAINERS                               |   6 +
>>>   drivers/vfio/Kconfig                      |   1 +
>>>   drivers/vfio/Makefile                     |   1 +
>>>   drivers/vfio/fsl-mc/Kconfig               |   9 ++
>>>   drivers/vfio/fsl-mc/Makefile              |   4 +
>>>   drivers/vfio/fsl-mc/vfio_fsl_mc.c         | 160 ++++++++++++++++++++++
>>>   drivers/vfio/fsl-mc/vfio_fsl_mc_private.h |  14 ++
>>>   include/uapi/linux/vfio.h                 |   1 +
>>>   8 files changed, 196 insertions(+)
>>>   create mode 100644 drivers/vfio/fsl-mc/Kconfig
>>>   create mode 100644 drivers/vfio/fsl-mc/Makefile
>>>   create mode 100644 drivers/vfio/fsl-mc/vfio_fsl_mc.c
>>>   create mode 100644 drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 3b186ade3597..f3f9ea108588 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -18229,6 +18229,12 @@ F:    drivers/vfio/
>>>   F:    include/linux/vfio.h
>>>   F:    include/uapi/linux/vfio.h
>>>   +VFIO FSL-MC DRIVER
>>> +M:    Diana Craciun <[email protected]>
>>> +L:    [email protected]
>>> +S:    Maintained
>>> +F:    drivers/vfio/fsl-mc/
>>> +
>>>   VFIO MEDIATED DEVICE DRIVERS
>>>   M:    Kirti Wankhede <[email protected]>
>>>   L:    [email protected]
>>> diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
>>> index fd17db9b432f..5533df91b257 100644
>>> --- a/drivers/vfio/Kconfig
>>> +++ b/drivers/vfio/Kconfig
>>> @@ -47,4 +47,5 @@ menuconfig VFIO_NOIOMMU
>>>   source "drivers/vfio/pci/Kconfig"
>>>   source "drivers/vfio/platform/Kconfig"
>>>   source "drivers/vfio/mdev/Kconfig"
>>> +source "drivers/vfio/fsl-mc/Kconfig"
>>>   source "virt/lib/Kconfig"
>>> diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
>>> index de67c4725cce..fee73f3d9480 100644
>>> --- a/drivers/vfio/Makefile
>>> +++ b/drivers/vfio/Makefile
>>> @@ -9,3 +9,4 @@ obj-$(CONFIG_VFIO_SPAPR_EEH) += vfio_spapr_eeh.o
>>>   obj-$(CONFIG_VFIO_PCI) += pci/
>>>   obj-$(CONFIG_VFIO_PLATFORM) += platform/
>>>   obj-$(CONFIG_VFIO_MDEV) += mdev/
>>> +obj-$(CONFIG_VFIO_FSL_MC) += fsl-mc/
>>> diff --git a/drivers/vfio/fsl-mc/Kconfig b/drivers/vfio/fsl-mc/Kconfig
>>> new file mode 100644
>>> index 000000000000..b1a527d6b6f2
>>> --- /dev/null
>>> +++ b/drivers/vfio/fsl-mc/Kconfig
>>> @@ -0,0 +1,9 @@
>>> +config VFIO_FSL_MC
>>> +    tristate "VFIO support for QorIQ DPAA2 fsl-mc bus devices"
>>> +    depends on VFIO && FSL_MC_BUS && EVENTFD
>>> +    help
>>> +      Driver to enable support for the VFIO QorIQ DPAA2 fsl-mc
>>> +      (Management Complex) devices. This is required to passthrough
>>> +      fsl-mc bus devices using the VFIO framework.
>>> +
>>> +      If you don't know what to do here, say N.
>>> diff --git a/drivers/vfio/fsl-mc/Makefile b/drivers/vfio/fsl-mc/Makefile
>>> new file mode 100644
>>> index 000000000000..0c6e5d2ddaae
>>> --- /dev/null
>>> +++ b/drivers/vfio/fsl-mc/Makefile
>>> @@ -0,0 +1,4 @@
>>> +# SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
>>> +
>>> +vfio-fsl-mc-y := vfio_fsl_mc.o
>>> +obj-$(CONFIG_VFIO_FSL_MC) += vfio-fsl-mc.o
>>> diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
>>> b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
>>> new file mode 100644
>>> index 000000000000..8b53c2a25b32
>>> --- /dev/null
>>> +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
>>> @@ -0,0 +1,160 @@
>>> +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
>>> +/*
>>> + * Copyright 2013-2016 Freescale Semiconductor Inc.
>>> + * Copyright 2016-2017,2019-2020 NXP
>>> + */
>>> +
>>> +#include <linux/device.h>
>>> +#include <linux/iommu.h>
>>> +#include <linux/module.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/types.h>
>>> +#include <linux/vfio.h>
>>> +#include <linux/fsl/mc.h>
>>> +
>>> +#include "vfio_fsl_mc_private.h"
>>> +
>>> +static int vfio_fsl_mc_open(void *device_data)
>>> +{
>>> +    if (!try_module_get(THIS_MODULE))
>>> +        return -ENODEV;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static void vfio_fsl_mc_release(void *device_data)
>>> +{
>>> +    module_put(THIS_MODULE);
>>> +}
>>> +
>>> +static long vfio_fsl_mc_ioctl(void *device_data, unsigned int cmd,
>>> +                  unsigned long arg)
>>> +{
>>> +    switch (cmd) {
>>> +    case VFIO_DEVICE_GET_INFO:
>>> +    {
>>> +        return -ENOTTY;
>>> +    }
>>> +    case VFIO_DEVICE_GET_REGION_INFO:
>>> +    {
>>> +        return -ENOTTY;
>>> +    }
>>> +    case VFIO_DEVICE_GET_IRQ_INFO:
>>> +    {
>>> +        return -ENOTTY;
>>> +    }
>>> +    case VFIO_DEVICE_SET_IRQS:
>>> +    {
>>> +        return -ENOTTY;
>>> +    }
>>> +    case VFIO_DEVICE_RESET:
>>> +    {
>>> +        return -ENOTTY;
>>> +    }
>>> +    default:
>>> +        return -ENOTTY;
>>> +    }
>>> +}
>>> +
>>> +static ssize_t vfio_fsl_mc_read(void *device_data, char __user *buf,
>>> +                size_t count, loff_t *ppos)
>>> +{
>>> +    return -EINVAL;
>>> +}
>>> +
>>> +static ssize_t vfio_fsl_mc_write(void *device_data, const char
>>> __user *buf,
>>> +                 size_t count, loff_t *ppos)
>>> +{
>>> +    return -EINVAL;
>>> +}
>>> +
>>> +static int vfio_fsl_mc_mmap(void *device_data, struct vm_area_struct
>>> *vma)
>>> +{
>>> +    return -EINVAL;
>>> +}
>>> +
>>> +static const struct vfio_device_ops vfio_fsl_mc_ops = {
>>> +    .name        = "vfio-fsl-mc",
>>> +    .open        = vfio_fsl_mc_open,
>>> +    .release    = vfio_fsl_mc_release,
>>> +    .ioctl        = vfio_fsl_mc_ioctl,
>>> +    .read        = vfio_fsl_mc_read,
>>> +    .write        = vfio_fsl_mc_write,
>>> +    .mmap        = vfio_fsl_mc_mmap,
>>> +};
>>> +
>>> +static int vfio_fsl_mc_probe(struct fsl_mc_device *mc_dev)
>>> +{
>>> +    struct iommu_group *group;
>>> +    struct vfio_fsl_mc_device *vdev;
>>> +    struct device *dev = &mc_dev->dev;
>>> +    int ret;
>>> +
>>> +    group = vfio_iommu_group_get(dev);
>>> +    if (!group) {
>>> +        dev_err(dev, "%s: VFIO: No IOMMU group\n", __func__);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    vdev = devm_kzalloc(dev, sizeof(*vdev), GFP_KERNEL);
>>> +    if (!vdev) {
>>> +        vfio_iommu_group_put(group, dev);
>>> +        return -ENOMEM;
>>> +    }
>>> +
>>> +    vdev->mc_dev = mc_dev;
>>> +
>>> +    ret = vfio_add_group_dev(dev, &vfio_fsl_mc_ops, vdev);
>>> +    if (ret) {
>>> +        dev_err(dev, "%s: Failed to add to vfio group\n", __func__);
>>> +        vfio_iommu_group_put(group, dev);
>>> +        return ret;
>>> +    }
>>> +
>>> +    return ret;
>> nit: introduce out_group_put: as in other files
>> This will be usable also in subsequent patches
>>> +}
>>> +
>>> +static int vfio_fsl_mc_remove(struct fsl_mc_device *mc_dev)
>>> +{
>>> +    struct vfio_fsl_mc_device *vdev;
>>> +    struct device *dev = &mc_dev->dev;
>>> +
>>> +    vdev = vfio_del_group_dev(dev);
>>> +    if (!vdev)
>>> +        return -EINVAL;
>>> +
>>> +    vfio_iommu_group_put(mc_dev->dev.iommu_group, dev);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +/*
>>> + * vfio-fsl_mc is a meta-driver, so use driver_override interface to
>>> + * bind a fsl_mc container with this driver and match_id_table is NULL.
>>> + */
>>> +static struct fsl_mc_driver vfio_fsl_mc_driver = {
>>> +    .probe        = vfio_fsl_mc_probe,
>>> +    .remove        = vfio_fsl_mc_remove,
>>> +    .match_id_table = NULL,
>> not needed?
>
> The driver will always match on driver_override, there is no need for
> static ids.
I rather meant do you really need to initialize it? I guess it is the
same in vfio_platform and there is no such init.

Thanks

Eric
>
>>> +    .driver    = {
>>> +        .name    = "vfio-fsl-mc",
>>> +        .owner    = THIS_MODULE,
>>> +    },
>>> +};
>>> +
>>> +static int __init vfio_fsl_mc_driver_init(void)
>>> +{
>>> +    return fsl_mc_driver_register(&vfio_fsl_mc_driver);
>>> +}
>>> +
>>> +static void __exit vfio_fsl_mc_driver_exit(void)
>>> +{
>>> +    fsl_mc_driver_unregister(&vfio_fsl_mc_driver);
>>> +}
>>> +
>>> +module_init(vfio_fsl_mc_driver_init);
>>> +module_exit(vfio_fsl_mc_driver_exit);
>>> +
>>> +MODULE_LICENSE("GPL v2");
>> Don't you need MODULE_LICENSE("Dual BSD/GPL"); ?
>
> Yes, will change.
>
>>> +MODULE_DESCRIPTION("VFIO for FSL-MC devices - User Level meta-driver");
>>> diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
>>> b/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
>>> new file mode 100644
>>> index 000000000000..e79cc116f6b8
>>> --- /dev/null
>>> +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
>>> @@ -0,0 +1,14 @@
>>> +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
>>> +/*
>>> + * Copyright 2013-2016 Freescale Semiconductor Inc.
>>> + * Copyright 2016,2019-2020 NXP
>>> + */
>>> +
>>> +#ifndef VFIO_FSL_MC_PRIVATE_H
>>> +#define VFIO_FSL_MC_PRIVATE_H
>>> +
>>> +struct vfio_fsl_mc_device {
>>> +    struct fsl_mc_device        *mc_dev;
>>> +};
>>> +
>>> +#endif /* VFIO_FSL_MC_PRIVATE_H */
>>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>>> index 920470502329..95deac891378 100644
>>> --- a/include/uapi/linux/vfio.h
>>> +++ b/include/uapi/linux/vfio.h
>>> @@ -201,6 +201,7 @@ struct vfio_device_info {
>>>   #define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)    /* vfio-amba device */
>>>   #define VFIO_DEVICE_FLAGS_CCW    (1 << 4)    /* vfio-ccw device */
>>>   #define VFIO_DEVICE_FLAGS_AP    (1 << 5)    /* vfio-ap device */
>>> +#define VFIO_DEVICE_FLAGS_FSL_MC (1 << 6)    /* vfio-fsl-mc device */
>>>       __u32    num_regions;    /* Max region index + 1 */
>>>       __u32    num_irqs;    /* Max IRQ index + 1 */
>>>   };
>>>
>> Thanks
>>
>> Eric
>>
>
> Thanks,
> Diana
>

2020-09-07 17:19:50

by Diana Madalina Craciun

[permalink] [raw]
Subject: Re: [PATCH v4 08/10] vfio/fsl-mc: trigger an interrupt via eventfd

Hi Eric,

On 9/4/2020 11:02 AM, Auger Eric wrote:
> Hi Diana,
>
> On 8/26/20 11:33 AM, Diana Craciun wrote:
>> This patch allows to set an eventfd for fsl-mc device interrupts
>> and also to trigger the interrupt eventfd from userspace for testing.
>>
>> All fsl-mc device interrupts are MSIs. The MSIs are allocated from
>> the MSI domain only once per DPRC and used by all the DPAA2 objects.
>> The interrupts are managed by the DPRC in a pool of interrupts. Each
>> device requests interrupts from this pool. The pool is allocated
>> when the first virtual device is setting the interrupts.
>> The pool of interrupts is protected by a lock.
>>
>> The DPRC has an interrupt of its own which indicates if the DPRC
>> contents have changed. However, currently, the contents of a DPRC
>> assigned to the guest cannot be changed at runtime, so this interrupt
>> is not configured.
>>
>> Signed-off-by: Bharat Bhushan <[email protected]>
>> Signed-off-by: Diana Craciun <[email protected]>
>> ---
>> drivers/vfio/fsl-mc/vfio_fsl_mc.c | 18 ++-
>> drivers/vfio/fsl-mc/vfio_fsl_mc_intr.c | 160 +++++++++++++++++++++-
>> drivers/vfio/fsl-mc/vfio_fsl_mc_private.h | 10 ++
>> 3 files changed, 186 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
>> index 42014297b484..73834f488a94 100644
>> --- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
>> +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
>> @@ -147,12 +147,28 @@ static int vfio_fsl_mc_open(void *device_data)
>> static void vfio_fsl_mc_release(void *device_data)
>> {
>> struct vfio_fsl_mc_device *vdev = device_data;
>> + int ret;
>>
>> mutex_lock(&vdev->reflck->lock);
>>
>> - if (!(--vdev->refcnt))
>> + if (!(--vdev->refcnt)) {
>> + struct fsl_mc_device *mc_dev = vdev->mc_dev;
>> + struct device *cont_dev = fsl_mc_cont_dev(&mc_dev->dev);
>> + struct fsl_mc_device *mc_cont = to_fsl_mc_device(cont_dev);
>> +
>> vfio_fsl_mc_regions_cleanup(vdev);
>>
>> + /* reset the device before cleaning up the interrupts */
>> + ret = dprc_reset_container(mc_cont->mc_io, 0,
>> + mc_cont->mc_handle,
>> + mc_cont->obj_desc.id,
>> + DPRC_RESET_OPTION_NON_RECURSIVE);
> shouldn't you test ret?
>> +
>> + vfio_fsl_mc_irqs_cleanup(vdev);
>> +
>> + fsl_mc_cleanup_irq_pool(mc_cont);
>> + }
>> +
>> mutex_unlock(&vdev->reflck->lock);
>>
>> module_put(THIS_MODULE);
>> diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc_intr.c b/drivers/vfio/fsl-mc/vfio_fsl_mc_intr.c
>> index 058aa97aa54a..409f3507fcf3 100644
>> --- a/drivers/vfio/fsl-mc/vfio_fsl_mc_intr.c
>> +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc_intr.c
>> @@ -29,12 +29,149 @@ static int vfio_fsl_mc_irq_unmask(struct vfio_fsl_mc_device *vdev,
>> return -EINVAL;
>> }
>>
>> +int vfio_fsl_mc_irqs_allocate(struct vfio_fsl_mc_device *vdev)
>> +{
>> + struct fsl_mc_device *mc_dev = vdev->mc_dev;
>> + struct vfio_fsl_mc_irq *mc_irq;
>> + int irq_count;
>> + int ret, i;
>> +
>> + /* Device does not support any interrupt */
> indent needs to be fixed
>> + if (mc_dev->obj_desc.irq_count == 0)
>> + return 0;
>> +
>> + /* interrupts were already allocated for this device */
>> + if (vdev->mc_irqs)
>> + return 0;
>> +
>> + irq_count = mc_dev->obj_desc.irq_count;
>> +
>> + mc_irq = kcalloc(irq_count, sizeof(*mc_irq), GFP_KERNEL);
>> + if (!mc_irq)
>> + return -ENOMEM;
>> +
>> + /* Allocate IRQs */
>> + ret = fsl_mc_allocate_irqs(mc_dev);
>> + if (ret) {
>> + kfree(mc_irq);
>> + return ret;
>> + }
>> +
>> + for (i = 0; i < irq_count; i++) {
>> + mc_irq[i].count = 1;
>> + mc_irq[i].flags = VFIO_IRQ_INFO_EVENTFD;
>> + }
>> +
>> + vdev->mc_irqs = mc_irq;
>> +
>> + return 0;
>> +}
>> +
>> +static irqreturn_t vfio_fsl_mc_irq_handler(int irq_num, void *arg)
>> +{
>> + struct vfio_fsl_mc_irq *mc_irq = (struct vfio_fsl_mc_irq *)arg;
>> +
>> + eventfd_signal(mc_irq->trigger, 1);
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int vfio_set_trigger(struct vfio_fsl_mc_device *vdev,
>> + int index, int fd)
>> +{
>> + struct vfio_fsl_mc_irq *irq = &vdev->mc_irqs[index];
>> + struct eventfd_ctx *trigger;
>> + int hwirq;
>> + int ret;
>> +
>> + hwirq = vdev->mc_dev->irqs[index]->msi_desc->irq;
>> + if (irq->trigger) {
>> + free_irq(hwirq, irq);
>> + kfree(irq->name);
>> + eventfd_ctx_put(irq->trigger);
>> + irq->trigger = NULL;
>> + }
>> +
>> + if (fd < 0) /* Disable only */
>> + return 0;
>> +
>> + irq->name = kasprintf(GFP_KERNEL, "vfio-irq[%d](%s)",
>> + hwirq, dev_name(&vdev->mc_dev->dev));
>> + if (!irq->name)
>> + return -ENOMEM;
>> +
>> + trigger = eventfd_ctx_fdget(fd);
>> + if (IS_ERR(trigger)) {
>> + kfree(irq->name);
>> + return PTR_ERR(trigger);
>> + }
>> +
>> + irq->trigger = trigger;
>> +
>> + ret = request_irq(hwirq, vfio_fsl_mc_irq_handler, 0,
>> + irq->name, irq);
>> + if (ret) {
>> + kfree(irq->name);
>> + eventfd_ctx_put(trigger);
>> + irq->trigger = NULL;
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int vfio_fsl_mc_set_irq_trigger(struct vfio_fsl_mc_device *vdev,
>> unsigned int index, unsigned int start,
>> unsigned int count, u32 flags,
>> void *data)
>> {
>> - return -EINVAL;
>> + struct fsl_mc_device *mc_dev = vdev->mc_dev;
>> + int ret, hwirq;
>> + struct vfio_fsl_mc_irq *irq;
>> + struct device *cont_dev = fsl_mc_cont_dev(&mc_dev->dev);
>> + struct fsl_mc_device *mc_cont = to_fsl_mc_device(cont_dev);
>> +
>> + if (start != 0 || count != 1)
>> + return -EINVAL;
>> +
>> + mutex_lock(&vdev->reflck->lock);
>> + ret = fsl_mc_populate_irq_pool(mc_cont,
>> + FSL_MC_IRQ_POOL_MAX_TOTAL_IRQS);
>> + if (ret)
>> + goto unlock;
>> +
>> + ret = vfio_fsl_mc_irqs_allocate(vdev);
> any reason the init is done in the set_irq() and not in the open() if
> !vdev->refcnt?

Actually yes, there is a reason. It comes from the way the MSIs are
handled.

The DPAA2 devices (the devices that can be assigned to the userspace)
are organized in a hierarchical way. The DPRC is some kind of container
(parent) for child devices. Only the DPRC is allocating interrupts (it
allocates MSIs from the MSI domain). The MSIs cannot be allocated in the
open() because the MSI setup needs an IOMMU cookie which is set when the
IOMMU is set with VFIO_SET_IOMMU. But iommu is set later, after open(),
so the MSIs should be allocated afterwards.

So, the DPRC is allocating a pool of MSIs and each child object will
request a number of interrupts from that pool. This is what
vfio_fsl_mc_irqs_allocate does: it requests a number of interrupts from
the pool.


>> + if (ret)
>> + goto unlock;
>> + mutex_unlock(&vdev->reflck->lock);
>> +
>> + if (!count && (flags & VFIO_IRQ_SET_DATA_NONE))
>> + return vfio_set_trigger(vdev, index, -1);
>> +
>> + if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
>> + s32 fd = *(s32 *)data;
>> +
>> + return vfio_set_trigger(vdev, index, fd);
>> + }
>> +
>> + hwirq = vdev->mc_dev->irqs[index]->msi_desc->irq;
>> +
>> + irq = &vdev->mc_irqs[index];
>> +
>> + if (flags & VFIO_IRQ_SET_DATA_NONE) {
>> + vfio_fsl_mc_irq_handler(hwirq, irq);
>> +
>> + } else if (flags & VFIO_IRQ_SET_DATA_BOOL) {
>> + u8 trigger = *(u8 *)data;
>> +
>> + if (trigger)
>> + vfio_fsl_mc_irq_handler(hwirq, irq);
>> + }
>> +
>> + return 0;
>> +
>> +unlock:
>> + mutex_unlock(&vdev->reflck->lock);
>> + return ret;
>> }
>>
>> int vfio_fsl_mc_set_irqs_ioctl(struct vfio_fsl_mc_device *vdev,
>> @@ -61,3 +198,24 @@ int vfio_fsl_mc_set_irqs_ioctl(struct vfio_fsl_mc_device *vdev,
>>
>> return ret;
>> }
>> +
>> +/* Free All IRQs for the given MC object */
>> +void vfio_fsl_mc_irqs_cleanup(struct vfio_fsl_mc_device *vdev)
>> +{
>> + struct fsl_mc_device *mc_dev = vdev->mc_dev;
>> + int irq_count = mc_dev->obj_desc.irq_count;
>> + int i;
>> +
>> + /* Device does not support any interrupt or the interrupts
>> + * were not configured
>> + */
>> + if (mc_dev->obj_desc.irq_count == 0 || !vdev->mc_irqs)
>> + return;
>> +
>> + for (i = 0; i < irq_count; i++)
>> + vfio_set_trigger(vdev, i, -1);
>> +
>> + fsl_mc_free_irqs(mc_dev);
>> + kfree(vdev->mc_irqs);
>> + vdev->mc_irqs = NULL;
>> +}
>> diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h b/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
>> index d5b6fe891a48..bbfca8b55f8a 100644
>> --- a/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
>> +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
>> @@ -15,6 +15,13 @@
>> #define VFIO_FSL_MC_INDEX_TO_OFFSET(index) \
>> ((u64)(index) << VFIO_FSL_MC_OFFSET_SHIFT)
>>
>> +struct vfio_fsl_mc_irq {
>> + u32 flags;
>> + u32 count;
>> + struct eventfd_ctx *trigger;
>> + char *name;
>> +};
>> +
>> struct vfio_fsl_mc_reflck {
>> struct kref kref;
>> struct mutex lock;
>> @@ -35,6 +42,7 @@ struct vfio_fsl_mc_device {
>> struct vfio_fsl_mc_region *regions;
>> struct vfio_fsl_mc_reflck *reflck;
>> struct mutex igate;
>> + struct vfio_fsl_mc_irq *mc_irqs;
>> };
>>
>> extern int vfio_fsl_mc_set_irqs_ioctl(struct vfio_fsl_mc_device *vdev,
>> @@ -42,4 +50,6 @@ extern int vfio_fsl_mc_set_irqs_ioctl(struct vfio_fsl_mc_device *vdev,
>> unsigned int start, unsigned int count,
>> void *data);
>>
>> +void vfio_fsl_mc_irqs_cleanup(struct vfio_fsl_mc_device *vdev);
>> +
>> #endif /* VFIO_FSL_MC_PRIVATE_H */
>>
> Thanks
>
> Eric
>

Thanks,
Diana

2020-09-10 08:31:29

by Eric Auger

[permalink] [raw]
Subject: Re: [PATCH v4 08/10] vfio/fsl-mc: trigger an interrupt via eventfd

Hi Diana,

On 9/7/20 4:02 PM, Diana Craciun OSS wrote:
> Hi Eric,
>
> On 9/4/2020 11:02 AM, Auger Eric wrote:
>> Hi Diana,
>>
>> On 8/26/20 11:33 AM, Diana Craciun wrote:
>>> This patch allows to set an eventfd for fsl-mc device interrupts
>>> and also to trigger the interrupt eventfd from userspace for testing.
>>>
>>> All fsl-mc device interrupts are MSIs. The MSIs are allocated from
>>> the MSI domain only once per DPRC and used by all the DPAA2 objects.
>>> The interrupts are managed by the DPRC in a pool of interrupts. Each
>>> device requests interrupts from this pool. The pool is allocated
>>> when the first virtual device is setting the interrupts.
>>> The pool of interrupts is protected by a lock.
>>>
>>> The DPRC has an interrupt of its own which indicates if the DPRC
>>> contents have changed. However, currently, the contents of a DPRC
>>> assigned to the guest cannot be changed at runtime, so this interrupt
>>> is not configured.
>>>
>>> Signed-off-by: Bharat Bhushan <[email protected]>
>>> Signed-off-by: Diana Craciun <[email protected]>
>>> ---
>>>   drivers/vfio/fsl-mc/vfio_fsl_mc.c         |  18 ++-
>>>   drivers/vfio/fsl-mc/vfio_fsl_mc_intr.c    | 160 +++++++++++++++++++++-
>>>   drivers/vfio/fsl-mc/vfio_fsl_mc_private.h |  10 ++
>>>   3 files changed, 186 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
>>> b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
>>> index 42014297b484..73834f488a94 100644
>>> --- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
>>> +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
>>> @@ -147,12 +147,28 @@ static int vfio_fsl_mc_open(void *device_data)
>>>   static void vfio_fsl_mc_release(void *device_data)
>>>   {
>>>       struct vfio_fsl_mc_device *vdev = device_data;
>>> +    int ret;
>>>         mutex_lock(&vdev->reflck->lock);
>>>   -    if (!(--vdev->refcnt))
>>> +    if (!(--vdev->refcnt)) {
>>> +        struct fsl_mc_device *mc_dev = vdev->mc_dev;
>>> +        struct device *cont_dev = fsl_mc_cont_dev(&mc_dev->dev);
>>> +        struct fsl_mc_device *mc_cont = to_fsl_mc_device(cont_dev);
>>> +
>>>           vfio_fsl_mc_regions_cleanup(vdev);
>>>   +        /* reset the device before cleaning up the interrupts */
>>> +        ret = dprc_reset_container(mc_cont->mc_io, 0,
>>> +              mc_cont->mc_handle,
>>> +              mc_cont->obj_desc.id,
>>> +              DPRC_RESET_OPTION_NON_RECURSIVE);
>> shouldn't you test ret?
>>> +
>>> +        vfio_fsl_mc_irqs_cleanup(vdev);
>>> +
>>> +        fsl_mc_cleanup_irq_pool(mc_cont);
>>> +    }
>>> +
>>>       mutex_unlock(&vdev->reflck->lock);
>>>         module_put(THIS_MODULE);
>>> diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc_intr.c
>>> b/drivers/vfio/fsl-mc/vfio_fsl_mc_intr.c
>>> index 058aa97aa54a..409f3507fcf3 100644
>>> --- a/drivers/vfio/fsl-mc/vfio_fsl_mc_intr.c
>>> +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc_intr.c
>>> @@ -29,12 +29,149 @@ static int vfio_fsl_mc_irq_unmask(struct
>>> vfio_fsl_mc_device *vdev,
>>>       return -EINVAL;
>>>   }
>>>   +int vfio_fsl_mc_irqs_allocate(struct vfio_fsl_mc_device *vdev)
>>> +{
>>> +    struct fsl_mc_device *mc_dev = vdev->mc_dev;
>>> +    struct vfio_fsl_mc_irq *mc_irq;
>>> +    int irq_count;
>>> +    int ret, i;
>>> +
>>> +    /* Device does not support any interrupt */
>> indent needs to be fixed
>>> +    if (mc_dev->obj_desc.irq_count == 0)
>>> +        return 0;
>>> +
>>> +    /* interrupts were already allocated for this device */
>>> +    if (vdev->mc_irqs)
>>> +        return 0;
>>> +
>>> +    irq_count = mc_dev->obj_desc.irq_count;
>>> +
>>> +    mc_irq = kcalloc(irq_count, sizeof(*mc_irq), GFP_KERNEL);
>>> +    if (!mc_irq)
>>> +        return -ENOMEM;
>>> +
>>> +    /* Allocate IRQs */
>>> +    ret = fsl_mc_allocate_irqs(mc_dev);
>>> +    if (ret) {
>>> +        kfree(mc_irq);
>>> +        return ret;
>>> +    }
>>> +
>>> +    for (i = 0; i < irq_count; i++) {
>>> +        mc_irq[i].count = 1;
>>> +        mc_irq[i].flags = VFIO_IRQ_INFO_EVENTFD;
>>> +    }
>>> +
>>> +    vdev->mc_irqs = mc_irq;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static irqreturn_t vfio_fsl_mc_irq_handler(int irq_num, void *arg)
>>> +{
>>> +    struct vfio_fsl_mc_irq *mc_irq = (struct vfio_fsl_mc_irq *)arg;
>>> +
>>> +    eventfd_signal(mc_irq->trigger, 1);
>>> +    return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int vfio_set_trigger(struct vfio_fsl_mc_device *vdev,
>>> +                           int index, int fd)
>>> +{
>>> +    struct vfio_fsl_mc_irq *irq = &vdev->mc_irqs[index];
>>> +    struct eventfd_ctx *trigger;
>>> +    int hwirq;
>>> +    int ret;
>>> +
>>> +    hwirq = vdev->mc_dev->irqs[index]->msi_desc->irq;
>>> +    if (irq->trigger) {
>>> +        free_irq(hwirq, irq);
>>> +        kfree(irq->name);
>>> +        eventfd_ctx_put(irq->trigger);
>>> +        irq->trigger = NULL;
>>> +    }
>>> +
>>> +    if (fd < 0) /* Disable only */
>>> +        return 0;
>>> +
>>> +    irq->name = kasprintf(GFP_KERNEL, "vfio-irq[%d](%s)",
>>> +                hwirq, dev_name(&vdev->mc_dev->dev));
>>> +    if (!irq->name)
>>> +        return -ENOMEM;
>>> +
>>> +    trigger = eventfd_ctx_fdget(fd);
>>> +    if (IS_ERR(trigger)) {
>>> +        kfree(irq->name);
>>> +        return PTR_ERR(trigger);
>>> +    }
>>> +
>>> +    irq->trigger = trigger;
>>> +
>>> +    ret = request_irq(hwirq, vfio_fsl_mc_irq_handler, 0,
>>> +          irq->name, irq);
>>> +    if (ret) {
>>> +        kfree(irq->name);
>>> +        eventfd_ctx_put(trigger);
>>> +        irq->trigger = NULL;
>>> +        return ret;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   static int vfio_fsl_mc_set_irq_trigger(struct vfio_fsl_mc_device
>>> *vdev,
>>>                          unsigned int index, unsigned int start,
>>>                          unsigned int count, u32 flags,
>>>                          void *data)
>>>   {
>>> -    return -EINVAL;
>>> +    struct fsl_mc_device *mc_dev = vdev->mc_dev;
>>> +    int ret, hwirq;
>>> +    struct vfio_fsl_mc_irq *irq;
>>> +    struct device *cont_dev = fsl_mc_cont_dev(&mc_dev->dev);
>>> +    struct fsl_mc_device *mc_cont = to_fsl_mc_device(cont_dev);
>>> +
>>> +    if (start != 0 || count != 1)
>>> +        return -EINVAL;
>>> +
>>> +    mutex_lock(&vdev->reflck->lock);
>>> +    ret = fsl_mc_populate_irq_pool(mc_cont,
>>> +            FSL_MC_IRQ_POOL_MAX_TOTAL_IRQS);
>>> +    if (ret)
>>> +        goto unlock;
>>> +
>>> +    ret = vfio_fsl_mc_irqs_allocate(vdev);
>> any reason the init is done in the set_irq() and not in the open() if
>> !vdev->refcnt?
>
> Actually yes, there is a reason. It comes from the way the MSIs are
> handled.
>
> The DPAA2  devices (the devices that can be assigned to the userspace)
> are organized in a hierarchical way. The DPRC is some kind of container
> (parent) for child devices. Only the DPRC is allocating interrupts (it
> allocates MSIs from the MSI domain). The MSIs cannot be allocated in the
> open() because the MSI setup needs an IOMMU cookie which is set when the
> IOMMU is set with VFIO_SET_IOMMU. But iommu is set later, after open(),
> so the MSIs should be allocated afterwards.
>
> So, the DPRC is allocating a pool of MSIs and each child object will
> request a number of interrupts from that pool. This is what
> vfio_fsl_mc_irqs_allocate does: it requests a number of interrupts from
> the pool.

OK, thank you for the clarifications.

Thanks

Eric
>
>
>>> +    if (ret)
>>> +        goto unlock;
>>> +    mutex_unlock(&vdev->reflck->lock);
>>> +
>>> +    if (!count && (flags & VFIO_IRQ_SET_DATA_NONE))
>>> +        return vfio_set_trigger(vdev, index, -1);
>>> +
>>> +    if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
>>> +        s32 fd = *(s32 *)data;
>>> +
>>> +        return vfio_set_trigger(vdev, index, fd);
>>> +    }
>>> +
>>> +    hwirq = vdev->mc_dev->irqs[index]->msi_desc->irq;
>>> +
>>> +    irq = &vdev->mc_irqs[index];
>>> +
>>> +    if (flags & VFIO_IRQ_SET_DATA_NONE) {
>>> +        vfio_fsl_mc_irq_handler(hwirq, irq);
>>> +
>>> +    } else if (flags & VFIO_IRQ_SET_DATA_BOOL) {
>>> +        u8 trigger = *(u8 *)data;
>>> +
>>> +        if (trigger)
>>> +            vfio_fsl_mc_irq_handler(hwirq, irq);
>>> +    }
>>> +
>>> +    return 0;
>>> +
>>> +unlock:
>>> +    mutex_unlock(&vdev->reflck->lock);
>>> +    return ret;
>>>   }
>>>     int vfio_fsl_mc_set_irqs_ioctl(struct vfio_fsl_mc_device *vdev,
>>> @@ -61,3 +198,24 @@ int vfio_fsl_mc_set_irqs_ioctl(struct
>>> vfio_fsl_mc_device *vdev,
>>>         return ret;
>>>   }
>>> +
>>> +/* Free All IRQs for the given MC object */
>>> +void vfio_fsl_mc_irqs_cleanup(struct vfio_fsl_mc_device *vdev)
>>> +{
>>> +    struct fsl_mc_device *mc_dev = vdev->mc_dev;
>>> +    int irq_count = mc_dev->obj_desc.irq_count;
>>> +    int i;
>>> +
>>> +    /* Device does not support any interrupt or the interrupts
>>> +     * were not configured
>>> +     */
>>> +    if (mc_dev->obj_desc.irq_count == 0 || !vdev->mc_irqs)
>>> +        return;
>>> +
>>> +    for (i = 0; i < irq_count; i++)
>>> +        vfio_set_trigger(vdev, i, -1);
>>> +
>>> +    fsl_mc_free_irqs(mc_dev);
>>> +    kfree(vdev->mc_irqs);
>>> +    vdev->mc_irqs = NULL;
>>> +}
>>> diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
>>> b/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
>>> index d5b6fe891a48..bbfca8b55f8a 100644
>>> --- a/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
>>> +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
>>> @@ -15,6 +15,13 @@
>>>   #define VFIO_FSL_MC_INDEX_TO_OFFSET(index)    \
>>>       ((u64)(index) << VFIO_FSL_MC_OFFSET_SHIFT)
>>>   +struct vfio_fsl_mc_irq {
>>> +    u32         flags;
>>> +    u32         count;
>>> +    struct eventfd_ctx  *trigger;
>>> +    char            *name;
>>> +};
>>> +
>>>   struct vfio_fsl_mc_reflck {
>>>       struct kref        kref;
>>>       struct mutex        lock;
>>> @@ -35,6 +42,7 @@ struct vfio_fsl_mc_device {
>>>       struct vfio_fsl_mc_region    *regions;
>>>       struct vfio_fsl_mc_reflck   *reflck;
>>>       struct mutex         igate;
>>> +    struct vfio_fsl_mc_irq      *mc_irqs;
>>>   };
>>>     extern int vfio_fsl_mc_set_irqs_ioctl(struct vfio_fsl_mc_device
>>> *vdev,
>>> @@ -42,4 +50,6 @@ extern int vfio_fsl_mc_set_irqs_ioctl(struct
>>> vfio_fsl_mc_device *vdev,
>>>                      unsigned int start, unsigned int count,
>>>                      void *data);
>>>   +void vfio_fsl_mc_irqs_cleanup(struct vfio_fsl_mc_device *vdev);
>>> +
>>>   #endif /* VFIO_FSL_MC_PRIVATE_H */
>>>
>> Thanks
>>
>> Eric
>>
>
> Thanks,
> Diana
>