In situations where the userspace driver is stopped abnormally and the
VFIO platform device is released, the assigned HW device currently is
left running. As a consequence the HW device might continue issuing IRQs
and performing DMA accesses.
On release, no physical IRQ handler is setup anymore. Also the DMA buffers
are unmapped leading to IOMMU aborts. So there is no serious consequence.
However when assigning that HW device again to another userspace driver,
this latter might face some unexpected IRQs and DMA accesses, which are
the result of the previous assignment.
In virtualization use-case, a VM newly granted with that HW device may be
impacted by the assignment of that device to a previous VM:
- IRQs may be injected very early when booting the new guest, even before
the guest driver has initialized leading to possible driver state
inconsistency.
- DMA accesses may hit the newly mapped VM address space at addresses that
may jeopardize the integrity of the newly installed VM.
Obviously the criticity depends on the assigned HW device.
As opposed to PCI, there is no standard mechanism to reset the platform
device.
This series proposes to implement device specific reset functions in
separate vfio reset modules (in-kernel or external). The vfio-platform
driver holds a whitelist of implemented triplets (compat string, module
name, function name). When the vfio-platform driver is probed it identifies
the fellow reset module/function matching the compat string of the
device, if any, and forces the load of this reset module.
A first reset module is provided: the vfio-platform-calxedaxgmac
module which implements a basic reset for the Calxeda xgmac.
The series can be found at
https://git.linaro.org/people/eric.auger/linux.git/shortlog/refs/heads/v4.1-rc6-reset-v2
History:
v1 -> v2:
- much simplified compared to v1 although principle of external modules is
kept: removed mechanism of dynamic registration of reset functions
- list is replaced by whitelist lookup table
- name of the reset function also stored in the lookup table
- autoload of reset modules
RFC -> PATCH v1:
- solution now based on a lookup list instead of specialized driver
Eric Auger (4):
VFIO: platform: add reset struct and lookup table
VFIO: platform: add reset callback
VFIO: platform: populate the reset function on probe
VFIO: platform: Calxeda xgmac reset module
drivers/vfio/platform/Kconfig | 2 +
drivers/vfio/platform/Makefile | 2 +
drivers/vfio/platform/reset/Kconfig | 7 ++
drivers/vfio/platform/reset/Makefile | 5 ++
.../platform/reset/vfio_platform_calxedaxgmac.c | 98 ++++++++++++++++++++++
drivers/vfio/platform/vfio_platform_common.c | 75 ++++++++++++++++-
drivers/vfio/platform/vfio_platform_private.h | 14 ++++
7 files changed, 200 insertions(+), 3 deletions(-)
create mode 100644 drivers/vfio/platform/reset/Kconfig
create mode 100644 drivers/vfio/platform/reset/Makefile
create mode 100644 drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
--
1.9.1
This patch introduces the vfio_platform_reset_combo struct that
stores all the information useful to handle the reset modality:
compat string, name of the reset function, name of the module that
implements the reset function. A lookup table of such structures
is added, currently containing a single sentinel element. A new
type field is added in vfio_platform_device to store what kind of
reset is associated to the device, if any.
Signed-off-by: Eric Auger <[email protected]>
---
drivers/vfio/platform/vfio_platform_common.c | 6 ++++++
drivers/vfio/platform/vfio_platform_private.h | 12 ++++++++++++
2 files changed, 18 insertions(+)
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index abcff7a..d970776 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -25,6 +25,12 @@
static DEFINE_MUTEX(driver_lock);
+static const struct vfio_platform_reset_combo reset_lookup_table[] = {
+ {
+ .type = VFIO_PLATFORM_RESET_TYPE_MAX
+ },
+};
+
static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
{
int cnt = 0, i;
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index 5d31e04..d864124 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -49,6 +49,10 @@ struct vfio_platform_region {
void __iomem *ioaddr;
};
+enum vfio_platform_reset_type {
+ VFIO_PLATFORM_RESET_TYPE_MAX /* last element */,
+};
+
struct vfio_platform_device {
struct vfio_platform_region *regions;
u32 num_regions;
@@ -56,6 +60,7 @@ struct vfio_platform_device {
u32 num_irqs;
int refcnt;
struct mutex igate;
+ enum vfio_platform_reset_type type;
/*
* These fields should be filled by the bus specific binder
@@ -69,6 +74,13 @@ struct vfio_platform_device {
int (*get_irq)(struct vfio_platform_device *vdev, int i);
};
+struct vfio_platform_reset_combo {
+ enum vfio_platform_reset_type type;
+ char *compat;
+ char *reset_function_name;
+ char *module_name;
+};
+
extern int vfio_platform_probe_common(struct vfio_platform_device *vdev,
struct device *dev);
extern struct vfio_platform_device *vfio_platform_remove_common
--
1.9.1
A new reset callback is introduced. If this callback is populated,
the reset is invoked on device first open/last close or upon userspace
ioctl. The modality is exposed on VFIO_DEVICE_GET_INFO.
Signed-off-by: Eric Auger <[email protected]>
---
v1 -> v2:
- reset now is also called on first open on top of last close
- remove IS_ERR_OR_NULL
---
drivers/vfio/platform/vfio_platform_common.c | 15 +++++++++++++--
drivers/vfio/platform/vfio_platform_private.h | 1 +
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index d970776..995929b 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -106,6 +106,8 @@ static void vfio_platform_release(void *device_data)
mutex_lock(&driver_lock);
if (!(--vdev->refcnt)) {
+ if (vdev->reset)
+ vdev->reset(vdev);
vfio_platform_regions_cleanup(vdev);
vfio_platform_irq_cleanup(vdev);
}
@@ -133,6 +135,9 @@ static int vfio_platform_open(void *device_data)
ret = vfio_platform_irq_init(vdev);
if (ret)
goto err_irq;
+
+ if (vdev->reset)
+ vdev->reset(vdev);
}
vdev->refcnt++;
@@ -165,6 +170,8 @@ static long vfio_platform_ioctl(void *device_data,
if (info.argsz < minsz)
return -EINVAL;
+ if (vdev->reset)
+ vdev->flags |= VFIO_DEVICE_FLAGS_RESET;
info.flags = vdev->flags;
info.num_regions = vdev->num_regions;
info.num_irqs = vdev->num_irqs;
@@ -258,8 +265,12 @@ static long vfio_platform_ioctl(void *device_data,
return ret;
- } else if (cmd == VFIO_DEVICE_RESET)
- return -EINVAL;
+ } else if (cmd == VFIO_DEVICE_RESET) {
+ if (vdev->reset)
+ return vdev->reset(vdev);
+ else
+ return -EINVAL;
+ }
return -ENOTTY;
}
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index d864124..cccb3ba 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -72,6 +72,7 @@ struct vfio_platform_device {
struct resource*
(*get_resource)(struct vfio_platform_device *vdev, int i);
int (*get_irq)(struct vfio_platform_device *vdev, int i);
+ int (*reset)(struct vfio_platform_device *vdev);
};
struct vfio_platform_reset_combo {
--
1.9.1
The reset function lookup happens on vfio-platform probe. The reset
module load is requested and a reference to the function symbol is
hold. The reference is released on vfio-platform remove.
Signed-off-by: Eric Auger <[email protected]>
---
v1 -> v2:
- [get,put]_reset now is called once on probe/remove
- use request_module to automatically load the reset module that
matches the compatibility string
- lookup table is used instead of list
- remove registration mechanism: reset function name is stored in the
lookup table.
- use device_property_read_string instead of
device_property_read_string_array
---
drivers/vfio/platform/vfio_platform_common.c | 48 +++++++++++++++++++++++++++-
1 file changed, 47 insertions(+), 1 deletion(-)
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 995929b..d474d6a 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -31,6 +31,47 @@ static const struct vfio_platform_reset_combo reset_lookup_table[] = {
},
};
+static int vfio_platform_get_reset(struct vfio_platform_device *vdev,
+ struct device *dev)
+{
+ const char *compat;
+ const struct vfio_platform_reset_combo *iter = reset_lookup_table;
+ int (*reset)(struct vfio_platform_device *);
+ int ret;
+
+ vdev->type = VFIO_PLATFORM_RESET_TYPE_MAX;
+ ret = device_property_read_string(dev, "compatible", &compat);
+ if (ret)
+ return ret;
+
+ while (iter->type < VFIO_PLATFORM_RESET_TYPE_MAX) {
+ if (!strcmp(iter->compat, compat)) {
+ request_module(iter->module_name);
+ reset = __symbol_get(iter->reset_function_name);
+ if (reset) {
+ vdev->type = iter->type;
+ vdev->reset = reset;
+ return 0;
+ }
+ }
+ iter++;
+ }
+ return -1;
+}
+
+static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
+{
+ const struct vfio_platform_reset_combo *iter = reset_lookup_table;
+
+ while (iter->type < VFIO_PLATFORM_RESET_TYPE_MAX) {
+ if (iter->type == vdev->type) {
+ __symbol_put(iter->reset_function_name);
+ return;
+ }
+ iter++;
+ }
+}
+
static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
{
int cnt = 0, i;
@@ -519,6 +560,8 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
return ret;
}
+ vfio_platform_get_reset(vdev, dev);
+
mutex_init(&vdev->igate);
return 0;
@@ -530,8 +573,11 @@ struct vfio_platform_device *vfio_platform_remove_common(struct device *dev)
struct vfio_platform_device *vdev;
vdev = vfio_del_group_dev(dev);
- if (vdev)
+
+ if (vdev) {
+ vfio_platform_put_reset(vdev);
iommu_group_put(dev->iommu_group);
+ }
return vdev;
}
--
1.9.1
This patch introduces a module that registers and implements a basic
reset function for the Calxeda xgmac device. This latter basically disables
interrupts and stops DMA transfers.
The reset function code is inherited from the native calxeda xgmac driver.
Signed-off-by: Eric Auger <[email protected]>
---
v1 -> v2:
- remove registration mechanism. The module mainly exports the reset
function. module_init and module_exit now are void.
---
drivers/vfio/platform/Kconfig | 2 +
drivers/vfio/platform/Makefile | 2 +
drivers/vfio/platform/reset/Kconfig | 7 ++
drivers/vfio/platform/reset/Makefile | 5 ++
.../platform/reset/vfio_platform_calxedaxgmac.c | 98 ++++++++++++++++++++++
drivers/vfio/platform/vfio_platform_common.c | 6 ++
drivers/vfio/platform/vfio_platform_private.h | 1 +
7 files changed, 121 insertions(+)
create mode 100644 drivers/vfio/platform/reset/Kconfig
create mode 100644 drivers/vfio/platform/reset/Makefile
create mode 100644 drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
diff --git a/drivers/vfio/platform/Kconfig b/drivers/vfio/platform/Kconfig
index 9a4403e..1df7477 100644
--- a/drivers/vfio/platform/Kconfig
+++ b/drivers/vfio/platform/Kconfig
@@ -18,3 +18,5 @@ config VFIO_AMBA
framework.
If you don't know what to do here, say N.
+
+source "drivers/vfio/platform/reset/Kconfig"
diff --git a/drivers/vfio/platform/Makefile b/drivers/vfio/platform/Makefile
index 81de144..9ce8afe 100644
--- a/drivers/vfio/platform/Makefile
+++ b/drivers/vfio/platform/Makefile
@@ -2,7 +2,9 @@
vfio-platform-y := vfio_platform.o vfio_platform_common.o vfio_platform_irq.o
obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform.o
+obj-$(CONFIG_VFIO_PLATFORM) += reset/
vfio-amba-y := vfio_amba.o
obj-$(CONFIG_VFIO_AMBA) += vfio-amba.o
+obj-$(CONFIG_VFIO_AMBA) += reset/
diff --git a/drivers/vfio/platform/reset/Kconfig b/drivers/vfio/platform/reset/Kconfig
new file mode 100644
index 0000000..746b96b
--- /dev/null
+++ b/drivers/vfio/platform/reset/Kconfig
@@ -0,0 +1,7 @@
+config VFIO_PLATFORM_CALXEDAXGMAC_RESET
+ tristate "VFIO support for calxeda xgmac reset"
+ depends on VFIO_PLATFORM
+ help
+ Enables the VFIO platform driver to handle reset for Calxeda xgmac
+
+ If you don't know what to do here, say N.
diff --git a/drivers/vfio/platform/reset/Makefile b/drivers/vfio/platform/reset/Makefile
new file mode 100644
index 0000000..2a486af
--- /dev/null
+++ b/drivers/vfio/platform/reset/Makefile
@@ -0,0 +1,5 @@
+vfio-platform-calxedaxgmac-y := vfio_platform_calxedaxgmac.o
+
+ccflags-y += -Idrivers/vfio/platform
+
+obj-$(CONFIG_VFIO_PLATFORM_CALXEDAXGMAC_RESET) += vfio-platform-calxedaxgmac.o
diff --git a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
new file mode 100644
index 0000000..bdf4c7b
--- /dev/null
+++ b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
@@ -0,0 +1,98 @@
+/*
+ * VFIO platform driver specialized for Calxeda xgmac reset
+ * reset code is inherited from calxeda xgmac native driver
+ *
+ * Copyright 2010-2011 Calxeda, Inc.
+ * Copyright (c) 2015 Linaro Ltd.
+ * http://www.linaro.org
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/io.h>
+
+#include "vfio_platform_private.h"
+
+#define DRIVER_VERSION "0.1"
+#define DRIVER_AUTHOR "Eric Auger <[email protected]>"
+#define DRIVER_DESC "Reset support for Calxeda xgmac vfio platform device"
+
+#define CALXEDAXGMAC_COMPAT "calxeda,hb-xgmac"
+
+/* XGMAC Register definitions */
+#define XGMAC_CONTROL 0x00000000 /* MAC Configuration */
+
+/* DMA Control and Status Registers */
+#define XGMAC_DMA_CONTROL 0x00000f18 /* Ctrl (Operational Mode) */
+#define XGMAC_DMA_INTR_ENA 0x00000f1c /* Interrupt Enable */
+
+/* DMA Control registe defines */
+#define DMA_CONTROL_ST 0x00002000 /* Start/Stop Transmission */
+#define DMA_CONTROL_SR 0x00000002 /* Start/Stop Receive */
+
+/* Common MAC defines */
+#define MAC_ENABLE_TX 0x00000008 /* Transmitter Enable */
+#define MAC_ENABLE_RX 0x00000004 /* Receiver Enable */
+
+static inline void xgmac_mac_disable(void __iomem *ioaddr)
+{
+ u32 value = readl(ioaddr + XGMAC_DMA_CONTROL);
+
+ value &= ~(DMA_CONTROL_ST | DMA_CONTROL_SR);
+ writel(value, ioaddr + XGMAC_DMA_CONTROL);
+
+ value = readl(ioaddr + XGMAC_CONTROL);
+ value &= ~(MAC_ENABLE_TX | MAC_ENABLE_RX);
+ writel(value, ioaddr + XGMAC_CONTROL);
+}
+
+int vfio_platform_calxedaxgmac_reset(struct vfio_platform_device *vdev)
+{
+ struct vfio_platform_region reg = vdev->regions[0];
+
+ if (!reg.ioaddr) {
+ reg.ioaddr =
+ ioremap_nocache(reg.addr, reg.size);
+ if (!reg.ioaddr)
+ return -ENOMEM;
+ }
+
+ /* disable IRQ */
+ writel(0, reg.ioaddr + XGMAC_DMA_INTR_ENA);
+
+ /* Disable the MAC core */
+ xgmac_mac_disable(reg.ioaddr);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(vfio_platform_calxedaxgmac_reset);
+
+static int __init vfio_platform_calxedaxgmac_init(void)
+{
+ return 0;
+}
+
+static void __exit vfio_platform_calxedaxgmac_exit(void)
+{
+}
+
+module_init(vfio_platform_calxedaxgmac_init);
+module_exit(vfio_platform_calxedaxgmac_exit);
+
+MODULE_VERSION(DRIVER_VERSION);
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index d474d6a..ffd76d2 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -27,6 +27,12 @@ static DEFINE_MUTEX(driver_lock);
static const struct vfio_platform_reset_combo reset_lookup_table[] = {
{
+ .type = VFIO_PLATFORM_RESET_CALXEDA_XGMAC,
+ .compat = "calxeda,hb-xgmac",
+ .reset_function_name = "vfio_platform_calxedaxgmac_reset",
+ .module_name = "vfio-platform-calxedaxgmac",
+ },
+ {
.type = VFIO_PLATFORM_RESET_TYPE_MAX
},
};
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index cccb3ba..31f15c7 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -50,6 +50,7 @@ struct vfio_platform_region {
};
enum vfio_platform_reset_type {
+ VFIO_PLATFORM_RESET_CALXEDA_XGMAC,
VFIO_PLATFORM_RESET_TYPE_MAX /* last element */,
};
--
1.9.1
On Fri, Jun 5, 2015 at 10:06 AM, Eric Auger <[email protected]> wrote:
> In situations where the userspace driver is stopped abnormally and the
> VFIO platform device is released, the assigned HW device currently is
> left running. As a consequence the HW device might continue issuing IRQs
> and performing DMA accesses.
>
> On release, no physical IRQ handler is setup anymore. Also the DMA buffers
> are unmapped leading to IOMMU aborts. So there is no serious consequence.
>
> However when assigning that HW device again to another userspace driver,
> this latter might face some unexpected IRQs and DMA accesses, which are
> the result of the previous assignment.
In general, shouldn't it just be a requirement that the drivers handle
this condition. You have the same problem with firmware/bootloaders
leaving h/w not in reset state or kexec'ing to a new kernel.
Rob
> In virtualization use-case, a VM newly granted with that HW device may be
> impacted by the assignment of that device to a previous VM:
> - IRQs may be injected very early when booting the new guest, even before
> the guest driver has initialized leading to possible driver state
> inconsistency.
> - DMA accesses may hit the newly mapped VM address space at addresses that
> may jeopardize the integrity of the newly installed VM.
>
> Obviously the criticity depends on the assigned HW device.
>
> As opposed to PCI, there is no standard mechanism to reset the platform
> device.
>
> This series proposes to implement device specific reset functions in
> separate vfio reset modules (in-kernel or external). The vfio-platform
> driver holds a whitelist of implemented triplets (compat string, module
> name, function name). When the vfio-platform driver is probed it identifies
> the fellow reset module/function matching the compat string of the
> device, if any, and forces the load of this reset module.
>
> A first reset module is provided: the vfio-platform-calxedaxgmac
> module which implements a basic reset for the Calxeda xgmac.
>
> The series can be found at
> https://git.linaro.org/people/eric.auger/linux.git/shortlog/refs/heads/v4.1-rc6-reset-v2
>
> History:
> v1 -> v2:
> - much simplified compared to v1 although principle of external modules is
> kept: removed mechanism of dynamic registration of reset functions
> - list is replaced by whitelist lookup table
> - name of the reset function also stored in the lookup table
> - autoload of reset modules
>
> RFC -> PATCH v1:
> - solution now based on a lookup list instead of specialized driver
>
>
> Eric Auger (4):
> VFIO: platform: add reset struct and lookup table
> VFIO: platform: add reset callback
> VFIO: platform: populate the reset function on probe
> VFIO: platform: Calxeda xgmac reset module
>
> drivers/vfio/platform/Kconfig | 2 +
> drivers/vfio/platform/Makefile | 2 +
> drivers/vfio/platform/reset/Kconfig | 7 ++
> drivers/vfio/platform/reset/Makefile | 5 ++
> .../platform/reset/vfio_platform_calxedaxgmac.c | 98 ++++++++++++++++++++++
> drivers/vfio/platform/vfio_platform_common.c | 75 ++++++++++++++++-
> drivers/vfio/platform/vfio_platform_private.h | 14 ++++
> 7 files changed, 200 insertions(+), 3 deletions(-)
> create mode 100644 drivers/vfio/platform/reset/Kconfig
> create mode 100644 drivers/vfio/platform/reset/Makefile
> create mode 100644 drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
>
> --
> 1.9.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, 2015-06-05 at 13:05 -0500, Rob Herring wrote:
> On Fri, Jun 5, 2015 at 10:06 AM, Eric Auger <[email protected]>
> wrote:
> > In situations where the userspace driver is stopped abnormally and
> > the
> > VFIO platform device is released, the assigned HW device currently
> > is
> > left running. As a consequence the HW device might continue
> > issuing IRQs
> > and performing DMA accesses.
> >
> > On release, no physical IRQ handler is setup anymore. Also the DMA
> > buffers
> > are unmapped leading to IOMMU aborts. So there is no serious
> > consequence.
> >
> > However when assigning that HW device again to another userspace
> > driver,
> > this latter might face some unexpected IRQs and DMA accesses,
> > which are
> > the result of the previous assignment.
>
> In general, shouldn't it just be a requirement that the drivers
> handle
> this condition. You have the same problem with firmware/bootloaders
> leaving h/w not in reset state or kexec'ing to a new kernel.
It's not the same situation. Firmware may leave HW in a non-reset
state but it must not leave the HW doing DMA; there's nothing the OS
could do about that as the OS could get corrupted before the driver
has a chance to run (this is not fun to debug). Leaving interrupts
potentially asserted would be bad as well, especially if the interrupt
is shared.
Likewise, with normal kexec drivers are supposed to quiesce the
hardware first -- and with kdump, the affected DMA buffers are never
reused.
In order for the driver to handle this, it would need to reset/quiesce
the device itself before enabling an IOMMU mapping. How would that
work for virtualization scenarios where the guest does not see any
IOMMU, and all vfio mappings are handled by QEMU or equivalent?
-Scott
On Fri, 2015-06-05 at 17:06 +0200, Eric Auger wrote:
> --- /dev/null
> +++ b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
> +static int __init vfio_platform_calxedaxgmac_init(void)
> +{
> + return 0;
> +}
> +
> +static void __exit vfio_platform_calxedaxgmac_exit(void)
> +{
> +}
> +
> +module_init(vfio_platform_calxedaxgmac_init);
> +module_exit(vfio_platform_calxedaxgmac_exit);
Just a nit: I think it's OK to leave out these functions if they both do
nothing. See delete_module in kernel/module.c.
Paul Bolle
Hi Rob, Scott,
On 06/05/2015 11:14 PM, Scott Wood wrote:
> On Fri, 2015-06-05 at 13:05 -0500, Rob Herring wrote:
>> On Fri, Jun 5, 2015 at 10:06 AM, Eric Auger <[email protected]>
>> wrote:
>>> In situations where the userspace driver is stopped abnormally and
>>> the
>>> VFIO platform device is released, the assigned HW device currently
>>> is
>>> left running. As a consequence the HW device might continue
>>> issuing IRQs
>>> and performing DMA accesses.
>>>
>>> On release, no physical IRQ handler is setup anymore. Also the DMA
>>> buffers
>>> are unmapped leading to IOMMU aborts. So there is no serious
>>> consequence.
>>>
>>> However when assigning that HW device again to another userspace
>>> driver,
>>> this latter might face some unexpected IRQs and DMA accesses,
>>> which are
>>> the result of the previous assignment.
>>
>> In general, shouldn't it just be a requirement that the drivers
>> handle
>> this condition. You have the same problem with firmware/bootloaders
>> leaving h/w not in reset state or kexec'ing to a new kernel.
>
> It's not the same situation. Firmware may leave HW in a non-reset
> state but it must not leave the HW doing DMA; there's nothing the OS
> could do about that as the OS could get corrupted before the driver
> has a chance to run (this is not fun to debug). Leaving interrupts
> potentially asserted would be bad as well, especially if the interrupt
> is shared.
>
> Likewise, with normal kexec drivers are supposed to quiesce the
> hardware first -- and with kdump, the affected DMA buffers are never
> reused.
>
> In order for the driver to handle this, it would need to reset/quiesce
> the device itself before enabling an IOMMU mapping. How would that
> work for virtualization scenarios where the guest does not see any
> IOMMU, and all vfio mappings are handled by QEMU or equivalent?
This is also my understanding. In a KVM virtualization use case, the
guest potentially could be corrupted by previously set DMA accesses
before getting the chance to stop DMA/IRQs.
Thanks for your interest.
Best Regards
Eric
>
> -Scott
>
Hi Paul,
On 06/06/2015 02:57 PM, Paul Bolle wrote:
> On Fri, 2015-06-05 at 17:06 +0200, Eric Auger wrote:
>> --- /dev/null
>> +++ b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
>
>> +static int __init vfio_platform_calxedaxgmac_init(void)
>> +{
>> + return 0;
>> +}
>> +
>> +static void __exit vfio_platform_calxedaxgmac_exit(void)
>> +{
>> +}
>> +
>> +module_init(vfio_platform_calxedaxgmac_init);
>> +module_exit(vfio_platform_calxedaxgmac_exit);
>
> Just a nit: I think it's OK to leave out these functions if they both do
> nothing. See delete_module in kernel/module.c.
Indeed it works fine without. Thank you for the information!
Best Regards
Eric
>
>
> Paul Bolle
>
On Fri, 2015-06-05 at 17:06 +0200, Eric Auger wrote:
> The reset function lookup happens on vfio-platform probe. The reset
> module load is requested and a reference to the function symbol is
> hold. The reference is released on vfio-platform remove.
>
> Signed-off-by: Eric Auger <[email protected]>
>
> ---
>
> v1 -> v2:
> - [get,put]_reset now is called once on probe/remove
> - use request_module to automatically load the reset module that
> matches the compatibility string
> - lookup table is used instead of list
> - remove registration mechanism: reset function name is stored in the
> lookup table.
> - use device_property_read_string instead of
> device_property_read_string_array
> ---
> drivers/vfio/platform/vfio_platform_common.c | 48 +++++++++++++++++++++++++++-
> 1 file changed, 47 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> index 995929b..d474d6a 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -31,6 +31,47 @@ static const struct vfio_platform_reset_combo reset_lookup_table[] = {
> },
> };
>
> +static int vfio_platform_get_reset(struct vfio_platform_device *vdev,
> + struct device *dev)
> +{
> + const char *compat;
> + const struct vfio_platform_reset_combo *iter = reset_lookup_table;
> + int (*reset)(struct vfio_platform_device *);
> + int ret;
> +
> + vdev->type = VFIO_PLATFORM_RESET_TYPE_MAX;
> + ret = device_property_read_string(dev, "compatible", &compat);
> + if (ret)
> + return ret;
> +
> + while (iter->type < VFIO_PLATFORM_RESET_TYPE_MAX) {
> + if (!strcmp(iter->compat, compat)) {
> + request_module(iter->module_name);
> + reset = __symbol_get(iter->reset_function_name);
symbol_get() appears to be the more robust and dominant interface for
this, why use __symbol_get()?
> + if (reset) {
> + vdev->type = iter->type;
> + vdev->reset = reset;
> + return 0;
> + }
> + }
> + iter++;
> + }
> + return -1;
-ENODEV seems preferable to -1, but shouldn't this really be a void
function?
> +}
> +
> +static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
> +{
> + const struct vfio_platform_reset_combo *iter = reset_lookup_table;
> +
> + while (iter->type < VFIO_PLATFORM_RESET_TYPE_MAX) {
> + if (iter->type == vdev->type) {
Again, I don't see the value in storing the enum, since the table is
static, it could just as easily be the array index and avoid this loop,
but we can avoid it anyway with symbol_put_addr().
> + __symbol_put(iter->reset_function_name);
> + return;
> + }
> + iter++;
> + }
> +}
> +
> static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
> {
> int cnt = 0, i;
> @@ -519,6 +560,8 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
> return ret;
> }
>
> + vfio_platform_get_reset(vdev, dev);
> +
> mutex_init(&vdev->igate);
>
> return 0;
> @@ -530,8 +573,11 @@ struct vfio_platform_device *vfio_platform_remove_common(struct device *dev)
> struct vfio_platform_device *vdev;
>
> vdev = vfio_del_group_dev(dev);
> - if (vdev)
> +
> + if (vdev) {
> + vfio_platform_put_reset(vdev);
> iommu_group_put(dev->iommu_group);
> + }
>
> return vdev;
> }
On Fri, 2015-06-05 at 17:06 +0200, Eric Auger wrote:
> This patch introduces the vfio_platform_reset_combo struct that
> stores all the information useful to handle the reset modality:
> compat string, name of the reset function, name of the module that
> implements the reset function. A lookup table of such structures
> is added, currently containing a single sentinel element. A new
> type field is added in vfio_platform_device to store what kind of
> reset is associated to the device, if any.
>
> Signed-off-by: Eric Auger <[email protected]>
> ---
> drivers/vfio/platform/vfio_platform_common.c | 6 ++++++
> drivers/vfio/platform/vfio_platform_private.h | 12 ++++++++++++
> 2 files changed, 18 insertions(+)
>
> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> index abcff7a..d970776 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -25,6 +25,12 @@
>
> static DEFINE_MUTEX(driver_lock);
>
> +static const struct vfio_platform_reset_combo reset_lookup_table[] = {
> + {
> + .type = VFIO_PLATFORM_RESET_TYPE_MAX
> + },
> +};
> +
> static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
> {
> int cnt = 0, i;
> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
> index 5d31e04..d864124 100644
> --- a/drivers/vfio/platform/vfio_platform_private.h
> +++ b/drivers/vfio/platform/vfio_platform_private.h
> @@ -49,6 +49,10 @@ struct vfio_platform_region {
> void __iomem *ioaddr;
> };
>
> +enum vfio_platform_reset_type {
> + VFIO_PLATFORM_RESET_TYPE_MAX /* last element */,
> +};
> +
> struct vfio_platform_device {
> struct vfio_platform_region *regions;
> u32 num_regions;
> @@ -56,6 +60,7 @@ struct vfio_platform_device {
> u32 num_irqs;
> int refcnt;
> struct mutex igate;
> + enum vfio_platform_reset_type type;
>
> /*
> * These fields should be filled by the bus specific binder
> @@ -69,6 +74,13 @@ struct vfio_platform_device {
> int (*get_irq)(struct vfio_platform_device *vdev, int i);
> };
>
> +struct vfio_platform_reset_combo {
> + enum vfio_platform_reset_type type;
> + char *compat;
> + char *reset_function_name;
> + char *module_name;
> +};
> +
I don't really understand the benefit of vfio_platform_reset_type, what
does it add? If we want an array end marker, we could just use NULL,
but if we're dealing with a static table, we could always use ARRAY_SIZE
and avoid an end marker altogether. If the concern is matching the
symbol to put when the device is released, we could just use
symbol_put_addr() and avoid any sort of lookup. Seems like there could
also be a smattering of "const" in this struct definition too.
> extern int vfio_platform_probe_common(struct vfio_platform_device *vdev,
> struct device *dev);
> extern struct vfio_platform_device *vfio_platform_remove_common
On 06/09/2015 08:26 PM, Alex Williamson wrote:
> On Fri, 2015-06-05 at 17:06 +0200, Eric Auger wrote:
>> The reset function lookup happens on vfio-platform probe. The reset
>> module load is requested and a reference to the function symbol is
>> hold. The reference is released on vfio-platform remove.
>>
>> Signed-off-by: Eric Auger <[email protected]>
>>
>> ---
>>
>> v1 -> v2:
>> - [get,put]_reset now is called once on probe/remove
>> - use request_module to automatically load the reset module that
>> matches the compatibility string
>> - lookup table is used instead of list
>> - remove registration mechanism: reset function name is stored in the
>> lookup table.
>> - use device_property_read_string instead of
>> device_property_read_string_array
>> ---
>> drivers/vfio/platform/vfio_platform_common.c | 48 +++++++++++++++++++++++++++-
>> 1 file changed, 47 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>> index 995929b..d474d6a 100644
>> --- a/drivers/vfio/platform/vfio_platform_common.c
>> +++ b/drivers/vfio/platform/vfio_platform_common.c
>> @@ -31,6 +31,47 @@ static const struct vfio_platform_reset_combo reset_lookup_table[] = {
>> },
>> };
>>
>> +static int vfio_platform_get_reset(struct vfio_platform_device *vdev,
>> + struct device *dev)
>> +{
>> + const char *compat;
>> + const struct vfio_platform_reset_combo *iter = reset_lookup_table;
>> + int (*reset)(struct vfio_platform_device *);
>> + int ret;
>> +
>> + vdev->type = VFIO_PLATFORM_RESET_TYPE_MAX;
>> + ret = device_property_read_string(dev, "compatible", &compat);
>> + if (ret)
>> + return ret;
>> +
>> + while (iter->type < VFIO_PLATFORM_RESET_TYPE_MAX) {
>> + if (!strcmp(iter->compat, compat)) {
>> + request_module(iter->module_name);
>> + reset = __symbol_get(iter->reset_function_name);
>
> symbol_get() appears to be the more robust and dominant interface for
> this, why use __symbol_get()?
I used this because it takes a const char * as an argument and this is
what I use as a datatype for storing the reset function name. Symbol_get
is provided with the symbol directly? It is also used
drivers/mtd/chips/gen_probe.c.
>
>> + if (reset) {
>> + vdev->type = iter->type;
>> + vdev->reset = reset;
>> + return 0;
>> + }
>> + }
>> + iter++;
>> + }
>> + return -1;
>
> -ENODEV seems preferable to -1, but shouldn't this really be a void
> function?
yes indeed
>
>> +}
>> +
>> +static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
>> +{
>> + const struct vfio_platform_reset_combo *iter = reset_lookup_table;
>> +
>> + while (iter->type < VFIO_PLATFORM_RESET_TYPE_MAX) {
>> + if (iter->type == vdev->type) {
>
> Again, I don't see the value in storing the enum, since the table is
> static, it could just as easily be the array index and avoid this loop,
> but we can avoid it anyway with symbol_put_addr().
yes you're definitively right!
Thanks
Eric
>
>> + __symbol_put(iter->reset_function_name);
>> + return;
>> + }
>> + iter++;
>> + }
>> +}
>> +
>> static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
>> {
>> int cnt = 0, i;
>> @@ -519,6 +560,8 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>> return ret;
>> }
>>
>> + vfio_platform_get_reset(vdev, dev);
>> +
>> mutex_init(&vdev->igate);
>>
>> return 0;
>> @@ -530,8 +573,11 @@ struct vfio_platform_device *vfio_platform_remove_common(struct device *dev)
>> struct vfio_platform_device *vdev;
>>
>> vdev = vfio_del_group_dev(dev);
>> - if (vdev)
>> +
>> + if (vdev) {
>> + vfio_platform_put_reset(vdev);
>> iommu_group_put(dev->iommu_group);
>> + }
>>
>> return vdev;
>> }
>
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Hi Alex,
On 06/09/2015 08:26 PM, Alex Williamson wrote:
> On Fri, 2015-06-05 at 17:06 +0200, Eric Auger wrote:
>> This patch introduces the vfio_platform_reset_combo struct that
>> stores all the information useful to handle the reset modality:
>> compat string, name of the reset function, name of the module that
>> implements the reset function. A lookup table of such structures
>> is added, currently containing a single sentinel element. A new
>> type field is added in vfio_platform_device to store what kind of
>> reset is associated to the device, if any.
>>
>> Signed-off-by: Eric Auger <[email protected]>
>> ---
>> drivers/vfio/platform/vfio_platform_common.c | 6 ++++++
>> drivers/vfio/platform/vfio_platform_private.h | 12 ++++++++++++
>> 2 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>> index abcff7a..d970776 100644
>> --- a/drivers/vfio/platform/vfio_platform_common.c
>> +++ b/drivers/vfio/platform/vfio_platform_common.c
>> @@ -25,6 +25,12 @@
>>
>> static DEFINE_MUTEX(driver_lock);
>>
>> +static const struct vfio_platform_reset_combo reset_lookup_table[] = {
>> + {
>> + .type = VFIO_PLATFORM_RESET_TYPE_MAX
>> + },
>> +};
>> +
>> static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
>> {
>> int cnt = 0, i;
>> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
>> index 5d31e04..d864124 100644
>> --- a/drivers/vfio/platform/vfio_platform_private.h
>> +++ b/drivers/vfio/platform/vfio_platform_private.h
>> @@ -49,6 +49,10 @@ struct vfio_platform_region {
>> void __iomem *ioaddr;
>> };
>>
>> +enum vfio_platform_reset_type {
>> + VFIO_PLATFORM_RESET_TYPE_MAX /* last element */,
>> +};
>> +
>> struct vfio_platform_device {
>> struct vfio_platform_region *regions;
>> u32 num_regions;
>> @@ -56,6 +60,7 @@ struct vfio_platform_device {
>> u32 num_irqs;
>> int refcnt;
>> struct mutex igate;
>> + enum vfio_platform_reset_type type;
>>
>> /*
>> * These fields should be filled by the bus specific binder
>> @@ -69,6 +74,13 @@ struct vfio_platform_device {
>> int (*get_irq)(struct vfio_platform_device *vdev, int i);
>> };
>>
>> +struct vfio_platform_reset_combo {
>> + enum vfio_platform_reset_type type;
>> + char *compat;
>> + char *reset_function_name;
>> + char *module_name;
>> +};
>> +
>
> I don't really understand the benefit of vfio_platform_reset_type, what
> does it add? If we want an array end marker, we could just use NULL,
> but if we're dealing with a static table, we could always use ARRAY_SIZE
> and avoid an end marker altogether. If the concern is matching the
> symbol to put when the device is released, we could just use
> symbol_put_addr() and avoid any sort of lookup. Seems like there could
> also be a smattering of "const" in this struct definition too.
Agreed. I will respin accordingly.
Best Regards
Eric
>
>> extern int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>> struct device *dev);
>> extern struct vfio_platform_device *vfio_platform_remove_common
>
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
On Wed, 2015-06-10 at 13:44 +0200, Eric Auger wrote:
> On 06/09/2015 08:26 PM, Alex Williamson wrote:
> > On Fri, 2015-06-05 at 17:06 +0200, Eric Auger wrote:
> >> The reset function lookup happens on vfio-platform probe. The reset
> >> module load is requested and a reference to the function symbol is
> >> hold. The reference is released on vfio-platform remove.
> >>
> >> Signed-off-by: Eric Auger <[email protected]>
> >>
> >> ---
> >>
> >> v1 -> v2:
> >> - [get,put]_reset now is called once on probe/remove
> >> - use request_module to automatically load the reset module that
> >> matches the compatibility string
> >> - lookup table is used instead of list
> >> - remove registration mechanism: reset function name is stored in the
> >> lookup table.
> >> - use device_property_read_string instead of
> >> device_property_read_string_array
> >> ---
> >> drivers/vfio/platform/vfio_platform_common.c | 48 +++++++++++++++++++++++++++-
> >> 1 file changed, 47 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> >> index 995929b..d474d6a 100644
> >> --- a/drivers/vfio/platform/vfio_platform_common.c
> >> +++ b/drivers/vfio/platform/vfio_platform_common.c
> >> @@ -31,6 +31,47 @@ static const struct vfio_platform_reset_combo reset_lookup_table[] = {
> >> },
> >> };
> >>
> >> +static int vfio_platform_get_reset(struct vfio_platform_device *vdev,
> >> + struct device *dev)
> >> +{
> >> + const char *compat;
> >> + const struct vfio_platform_reset_combo *iter = reset_lookup_table;
> >> + int (*reset)(struct vfio_platform_device *);
> >> + int ret;
> >> +
> >> + vdev->type = VFIO_PLATFORM_RESET_TYPE_MAX;
> >> + ret = device_property_read_string(dev, "compatible", &compat);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + while (iter->type < VFIO_PLATFORM_RESET_TYPE_MAX) {
> >> + if (!strcmp(iter->compat, compat)) {
> >> + request_module(iter->module_name);
> >> + reset = __symbol_get(iter->reset_function_name);
> >
> > symbol_get() appears to be the more robust and dominant interface for
> > this, why use __symbol_get()?
> I used this because it takes a const char * as an argument and this is
> what I use as a datatype for storing the reset function name. Symbol_get
> is provided with the symbol directly? It is also used
> drivers/mtd/chips/gen_probe.c.
But symbol_get() is just some macro wrappers around __symbol_get() that
handles tool chains that precede symbols with underscores. I don't
really know if that's still a concern, but are you sure it doesn't work?
> >
> >> + if (reset) {
> >> + vdev->type = iter->type;
> >> + vdev->reset = reset;
> >> + return 0;
> >> + }
> >> + }
> >> + iter++;
> >> + }
> >> + return -1;
> >
> > -ENODEV seems preferable to -1, but shouldn't this really be a void
> > function?
> yes indeed
> >
> >> +}
> >> +
> >> +static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
> >> +{
> >> + const struct vfio_platform_reset_combo *iter = reset_lookup_table;
> >> +
> >> + while (iter->type < VFIO_PLATFORM_RESET_TYPE_MAX) {
> >> + if (iter->type == vdev->type) {
> >
> > Again, I don't see the value in storing the enum, since the table is
> > static, it could just as easily be the array index and avoid this loop,
> > but we can avoid it anyway with symbol_put_addr().
> yes you're definitively right!
>
> Thanks
>
> Eric
> >
> >> + __symbol_put(iter->reset_function_name);
> >> + return;
> >> + }
> >> + iter++;
> >> + }
> >> +}
> >> +
> >> static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
> >> {
> >> int cnt = 0, i;
> >> @@ -519,6 +560,8 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
> >> return ret;
> >> }
> >>
> >> + vfio_platform_get_reset(vdev, dev);
> >> +
> >> mutex_init(&vdev->igate);
> >>
> >> return 0;
> >> @@ -530,8 +573,11 @@ struct vfio_platform_device *vfio_platform_remove_common(struct device *dev)
> >> struct vfio_platform_device *vdev;
> >>
> >> vdev = vfio_del_group_dev(dev);
> >> - if (vdev)
> >> +
> >> + if (vdev) {
> >> + vfio_platform_put_reset(vdev);
> >> iommu_group_put(dev->iommu_group);
> >> + }
> >>
> >> return vdev;
> >> }
> >
> >
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >
>
On 06/10/2015 05:10 PM, Alex Williamson wrote:
> On Wed, 2015-06-10 at 13:44 +0200, Eric Auger wrote:
>> On 06/09/2015 08:26 PM, Alex Williamson wrote:
>>> On Fri, 2015-06-05 at 17:06 +0200, Eric Auger wrote:
>>>> The reset function lookup happens on vfio-platform probe. The reset
>>>> module load is requested and a reference to the function symbol is
>>>> hold. The reference is released on vfio-platform remove.
>>>>
>>>> Signed-off-by: Eric Auger <[email protected]>
>>>>
>>>> ---
>>>>
>>>> v1 -> v2:
>>>> - [get,put]_reset now is called once on probe/remove
>>>> - use request_module to automatically load the reset module that
>>>> matches the compatibility string
>>>> - lookup table is used instead of list
>>>> - remove registration mechanism: reset function name is stored in the
>>>> lookup table.
>>>> - use device_property_read_string instead of
>>>> device_property_read_string_array
>>>> ---
>>>> drivers/vfio/platform/vfio_platform_common.c | 48 +++++++++++++++++++++++++++-
>>>> 1 file changed, 47 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>>>> index 995929b..d474d6a 100644
>>>> --- a/drivers/vfio/platform/vfio_platform_common.c
>>>> +++ b/drivers/vfio/platform/vfio_platform_common.c
>>>> @@ -31,6 +31,47 @@ static const struct vfio_platform_reset_combo reset_lookup_table[] = {
>>>> },
>>>> };
>>>>
>>>> +static int vfio_platform_get_reset(struct vfio_platform_device *vdev,
>>>> + struct device *dev)
>>>> +{
>>>> + const char *compat;
>>>> + const struct vfio_platform_reset_combo *iter = reset_lookup_table;
>>>> + int (*reset)(struct vfio_platform_device *);
>>>> + int ret;
>>>> +
>>>> + vdev->type = VFIO_PLATFORM_RESET_TYPE_MAX;
>>>> + ret = device_property_read_string(dev, "compatible", &compat);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + while (iter->type < VFIO_PLATFORM_RESET_TYPE_MAX) {
>>>> + if (!strcmp(iter->compat, compat)) {
>>>> + request_module(iter->module_name);
>>>> + reset = __symbol_get(iter->reset_function_name);
>>>
>>> symbol_get() appears to be the more robust and dominant interface for
>>> this, why use __symbol_get()?
>> I used this because it takes a const char * as an argument and this is
>> what I use as a datatype for storing the reset function name. Symbol_get
>> is provided with the symbol directly? It is also used
>> drivers/mtd/chips/gen_probe.c.
>
> But symbol_get() is just some macro wrappers around __symbol_get() that
> handles tool chains that precede symbols with underscores. I don't
> really know if that's still a concern, but are you sure it doesn't work?
>
>>>
>>>> + if (reset) {
>>>> + vdev->type = iter->type;
>>>> + vdev->reset = reset;
>>>> + return 0;
>>>> + }
>>>> + }
>>>> + iter++;
>>>> + }
>>>> + return -1;
>>>
>>> -ENODEV seems preferable to -1, but shouldn't this really be a void
>>> function?
>> yes indeed
>>>
>>>> +}
>>>> +
>>>> +static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
>>>> +{
>>>> + const struct vfio_platform_reset_combo *iter = reset_lookup_table;
>>>> +
>>>> + while (iter->type < VFIO_PLATFORM_RESET_TYPE_MAX) {
>>>> + if (iter->type == vdev->type) {
>>>
>>> Again, I don't see the value in storing the enum, since the table is
>>> static, it could just as easily be the array index and avoid this loop,
>>> but we can avoid it anyway with symbol_put_addr().
>> yes you're definitively right!
to be honest I don't remember. I Will try anyway ;-)
Eric
>>
>> Thanks
>>
>> Eric
>>>
>>>> + __symbol_put(iter->reset_function_name);
>>>> + return;
>>>> + }
>>>> + iter++;
>>>> + }
>>>> +}
>>>> +
>>>> static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
>>>> {
>>>> int cnt = 0, i;
>>>> @@ -519,6 +560,8 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>>>> return ret;
>>>> }
>>>>
>>>> + vfio_platform_get_reset(vdev, dev);
>>>> +
>>>> mutex_init(&vdev->igate);
>>>>
>>>> return 0;
>>>> @@ -530,8 +573,11 @@ struct vfio_platform_device *vfio_platform_remove_common(struct device *dev)
>>>> struct vfio_platform_device *vdev;
>>>>
>>>> vdev = vfio_del_group_dev(dev);
>>>> - if (vdev)
>>>> +
>>>> + if (vdev) {
>>>> + vfio_platform_put_reset(vdev);
>>>> iommu_group_put(dev->iommu_group);
>>>> + }
>>>>
>>>> return vdev;
>>>> }
>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> [email protected]
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>
>>
>
>
>
Hi Alex,
On 06/10/2015 05:10 PM, Alex Williamson wrote:
> On Wed, 2015-06-10 at 13:44 +0200, Eric Auger wrote:
>> On 06/09/2015 08:26 PM, Alex Williamson wrote:
>>> On Fri, 2015-06-05 at 17:06 +0200, Eric Auger wrote:
>>>> The reset function lookup happens on vfio-platform probe. The reset
>>>> module load is requested and a reference to the function symbol is
>>>> hold. The reference is released on vfio-platform remove.
>>>>
>>>> Signed-off-by: Eric Auger <[email protected]>
>>>>
>>>> ---
>>>>
>>>> v1 -> v2:
>>>> - [get,put]_reset now is called once on probe/remove
>>>> - use request_module to automatically load the reset module that
>>>> matches the compatibility string
>>>> - lookup table is used instead of list
>>>> - remove registration mechanism: reset function name is stored in the
>>>> lookup table.
>>>> - use device_property_read_string instead of
>>>> device_property_read_string_array
>>>> ---
>>>> drivers/vfio/platform/vfio_platform_common.c | 48 +++++++++++++++++++++++++++-
>>>> 1 file changed, 47 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>>>> index 995929b..d474d6a 100644
>>>> --- a/drivers/vfio/platform/vfio_platform_common.c
>>>> +++ b/drivers/vfio/platform/vfio_platform_common.c
>>>> @@ -31,6 +31,47 @@ static const struct vfio_platform_reset_combo reset_lookup_table[] = {
>>>> },
>>>> };
>>>>
>>>> +static int vfio_platform_get_reset(struct vfio_platform_device *vdev,
>>>> + struct device *dev)
>>>> +{
>>>> + const char *compat;
>>>> + const struct vfio_platform_reset_combo *iter = reset_lookup_table;
>>>> + int (*reset)(struct vfio_platform_device *);
>>>> + int ret;
>>>> +
>>>> + vdev->type = VFIO_PLATFORM_RESET_TYPE_MAX;
>>>> + ret = device_property_read_string(dev, "compatible", &compat);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + while (iter->type < VFIO_PLATFORM_RESET_TYPE_MAX) {
>>>> + if (!strcmp(iter->compat, compat)) {
>>>> + request_module(iter->module_name);
>>>> + reset = __symbol_get(iter->reset_function_name);
>>>
>>> symbol_get() appears to be the more robust and dominant interface for
>>> this, why use __symbol_get()?
>> I used this because it takes a const char * as an argument and this is
>> what I use as a datatype for storing the reset function name. Symbol_get
>> is provided with the symbol directly? It is also used
>> drivers/mtd/chips/gen_probe.c.
>
> But symbol_get() is just some macro wrappers around __symbol_get() that
> handles tool chains that precede symbols with underscores. I don't
> really know if that's still a concern, but are you sure it doesn't work?
I confirm it doesn't work due the (typeof(&x)) which in my case matches
a const char *.
drivers/vfio/platform/vfio_platform_common.c:45:10: warning: assignment
from incompatible pointer type [enabled by default]
reset = symbol_get(
#define symbol_get(x) ((typeof(&x))(__symbol_get(VMLINUX_SYMBOL_STR(x))))
Best Regards
Eric
>
>>>
>>>> + if (reset) {
>>>> + vdev->type = iter->type;
>>>> + vdev->reset = reset;
>>>> + return 0;
>>>> + }
>>>> + }
>>>> + iter++;
>>>> + }
>>>> + return -1;
>>>
>>> -ENODEV seems preferable to -1, but shouldn't this really be a void
>>> function?
>> yes indeed
>>>
>>>> +}
>>>> +
>>>> +static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
>>>> +{
>>>> + const struct vfio_platform_reset_combo *iter = reset_lookup_table;
>>>> +
>>>> + while (iter->type < VFIO_PLATFORM_RESET_TYPE_MAX) {
>>>> + if (iter->type == vdev->type) {
>>>
>>> Again, I don't see the value in storing the enum, since the table is
>>> static, it could just as easily be the array index and avoid this loop,
>>> but we can avoid it anyway with symbol_put_addr().
>> yes you're definitively right!
>>
>> Thanks
>>
>> Eric
>>>
>>>> + __symbol_put(iter->reset_function_name);
>>>> + return;
>>>> + }
>>>> + iter++;
>>>> + }
>>>> +}
>>>> +
>>>> static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
>>>> {
>>>> int cnt = 0, i;
>>>> @@ -519,6 +560,8 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>>>> return ret;
>>>> }
>>>>
>>>> + vfio_platform_get_reset(vdev, dev);
>>>> +
>>>> mutex_init(&vdev->igate);
>>>>
>>>> return 0;
>>>> @@ -530,8 +573,11 @@ struct vfio_platform_device *vfio_platform_remove_common(struct device *dev)
>>>> struct vfio_platform_device *vdev;
>>>>
>>>> vdev = vfio_del_group_dev(dev);
>>>> - if (vdev)
>>>> +
>>>> + if (vdev) {
>>>> + vfio_platform_put_reset(vdev);
>>>> iommu_group_put(dev->iommu_group);
>>>> + }
>>>>
>>>> return vdev;
>>>> }
>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> [email protected]
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>
>>
>
>
>