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-rc7-reset-v3
History:
v2 -> v3:
- remove void module_init/exit functions in calxeda reset module
- remove enum vfio_platform_reset_type
- for reset lookup, use ARRAY_SIZE
- in reset put use symbol_put_addr
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 | 86 ++++++++++++++++++++++
drivers/vfio/platform/vfio_platform_common.c | 60 ++++++++++++++-
drivers/vfio/platform/vfio_platform_private.h | 7 ++
7 files changed, 166 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]>
---
v2 -> v3:
- add const in struct vfio_platform_reset_combo
- remove enum vfio_platform_reset_type
v2: creation
---
drivers/vfio/platform/vfio_platform_common.c | 3 +++
drivers/vfio/platform/vfio_platform_private.h | 6 ++++++
2 files changed, 9 insertions(+)
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index abcff7a..611597e 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -25,6 +25,9 @@
static DEFINE_MUTEX(driver_lock);
+static const struct vfio_platform_reset_combo reset_lookup_table[] = {
+};
+
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..9e37b9f 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -69,6 +69,12 @@ struct vfio_platform_device {
int (*get_irq)(struct vfio_platform_device *vdev, int i);
};
+struct vfio_platform_reset_combo {
+ const char *compat;
+ const char *reset_function_name;
+ const 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 611597e..6393581 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -103,6 +103,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);
}
@@ -130,6 +132,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++;
@@ -162,6 +167,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;
@@ -255,8 +262,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 9e37b9f..1c9b3d5 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -67,6 +67,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]>
---
v2 -> v3:
- vfio_platform_get_reset becomes void
- use ARRAY_SIZE
- use symbol_put_addr
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 | 37 +++++++++++++++++++++++++++-
1 file changed, 36 insertions(+), 1 deletion(-)
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 6393581..f3391a9 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -28,6 +28,36 @@ static DEFINE_MUTEX(driver_lock);
static const struct vfio_platform_reset_combo reset_lookup_table[] = {
};
+static void vfio_platform_get_reset(struct vfio_platform_device *vdev,
+ struct device *dev)
+{
+ const char *compat;
+ int (*reset)(struct vfio_platform_device *);
+ int ret, i;
+
+ ret = device_property_read_string(dev, "compatible", &compat);
+ if (ret)
+ return;
+
+ for (i = 0 ; i < ARRAY_SIZE(reset_lookup_table); i++) {
+ if (!strcmp(reset_lookup_table[i].compat, compat)) {
+ request_module(reset_lookup_table[i].module_name);
+ reset = __symbol_get(
+ reset_lookup_table[i].reset_function_name);
+ if (reset) {
+ vdev->reset = reset;
+ return;
+ }
+ }
+ }
+}
+
+static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
+{
+ if (vdev->reset)
+ symbol_put_addr(vdev->reset);
+}
+
static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
{
int cnt = 0, i;
@@ -516,6 +546,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;
@@ -527,8 +559,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]>
---
v2 -> v3:
- remove void vfio_platform_calxedaxgmac_init and
vfio_platform_calxedaxgmac_exit
- no enum vfio_platform_reset_type type anymore
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 | 86 ++++++++++++++++++++++
drivers/vfio/platform/vfio_platform_common.c | 5 ++
6 files changed, 107 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..619dc7d
--- /dev/null
+++ b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
@@ -0,0 +1,86 @@
+/*
+ * 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);
+
+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 f3391a9..e43efb5 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -26,6 +26,11 @@
static DEFINE_MUTEX(driver_lock);
static const struct vfio_platform_reset_combo reset_lookup_table[] = {
+ {
+ .compat = "calxeda,hb-xgmac",
+ .reset_function_name = "vfio_platform_calxedaxgmac_reset",
+ .module_name = "vfio-platform-calxedaxgmac",
+ },
};
static void vfio_platform_get_reset(struct vfio_platform_device *vdev,
--
1.9.1
On Thu, 2015-06-11 at 14:08 +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.
The commit log no longer matches the code.
The only other thing I'm struggling with in this series is that in 0/4
you suggest that the reset modules can be in-kernel or external, but
we're making a static list here, so there's really no support for
random, user-provided reset modules. So are we missing the mark on the
requirements?
One way I thought you could achieve your requirement would be if we did
away with the lookup table and looked for the module and function using
a pre-defined transform on the compat ID. For instance, a compat ID of
"calxeda,hb-xgmac" would automatically request a module named
"vfio-platform-reset-calxedahb-xgmac" and look for a symbol of the same
name for the reset function (I wonder if we can actually have a module
and symbol of the same name). It seems fairly safe since an external
module would need to be explicitly placed in the search path for the
userspace module loader.
Otherwise the table would need to become a list, the external module
would need to be manually loaded, and the module_init() would need to
register an entry on that list. Thanks,
Alex
> Signed-off-by: Eric Auger <[email protected]>
>
> ---
>
> v2 -> v3:
> - add const in struct vfio_platform_reset_combo
> - remove enum vfio_platform_reset_type
>
> v2: creation
> ---
> drivers/vfio/platform/vfio_platform_common.c | 3 +++
> drivers/vfio/platform/vfio_platform_private.h | 6 ++++++
> 2 files changed, 9 insertions(+)
>
> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> index abcff7a..611597e 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -25,6 +25,9 @@
>
> static DEFINE_MUTEX(driver_lock);
>
> +static const struct vfio_platform_reset_combo reset_lookup_table[] = {
> +};
> +
> 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..9e37b9f 100644
> --- a/drivers/vfio/platform/vfio_platform_private.h
> +++ b/drivers/vfio/platform/vfio_platform_private.h
> @@ -69,6 +69,12 @@ struct vfio_platform_device {
> int (*get_irq)(struct vfio_platform_device *vdev, int i);
> };
>
> +struct vfio_platform_reset_combo {
> + const char *compat;
> + const char *reset_function_name;
> + const 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
Hi Alex,
On 06/11/2015 11:11 PM, Alex Williamson wrote:
> On Thu, 2015-06-11 at 14:08 +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.
>
> The commit log no longer matches the code.
yes indeed missed that.
>
> The only other thing I'm struggling with in this series is that in 0/4
> you suggest that the reset modules can be in-kernel or external, but
> we're making a static list here, so there's really no support for
> random, user-provided reset modules. So are we missing the mark on the
> requirements?
Well personally I do not have a MUST HAVE requirement for that feature.
My main requirement was to find a way to stop DMA/IRQ transfers
programmed by a previous VM potentially jeopardizing the integrity of a
second VM.
>
> One way I thought you could achieve your requirement would be if we did
> away with the lookup table and looked for the module and function using
> a pre-defined transform on the compat ID. For instance, a compat ID of
> "calxeda,hb-xgmac" would automatically request a module named
> "vfio-platform-reset-calxedahb-xgmac" and look for a symbol of the same
> name for the reset function (I wonder if we can actually have a module
> and symbol of the same name).
I just tried and it works
It seems fairly safe since an external
> module would need to be explicitly placed in the search path for the
> userspace module loader.
we talked together with Christoffer about such a technique at the very
beginning and he was not very fond of it, advising an approach using a
defined API.
>
> Otherwise the table would need to become a list, the external module
> would need to be manually loaded, and the module_init() would need to
> register an entry on that list.
Isn't it what I attempted to do in v2 or do you mean something else?
Having a whitelist brings the benefit to know which devices really are
used with vfio-platform (besides some devices may not need any reset
module). I don't know yet whether this is something that may slow down
the adoption.
Personally I would keep it as is for now and see how it gets used and
whether users complain, ...
If you agree I will respin just reworking the commit message.
Best Regards
Eric
Thanks,
>
> Alex
>
>> Signed-off-by: Eric Auger <[email protected]>
>>
>> ---
>>
>> v2 -> v3:
>> - add const in struct vfio_platform_reset_combo
>> - remove enum vfio_platform_reset_type
>>
>> v2: creation
>> ---
>> drivers/vfio/platform/vfio_platform_common.c | 3 +++
>> drivers/vfio/platform/vfio_platform_private.h | 6 ++++++
>> 2 files changed, 9 insertions(+)
>>
>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>> index abcff7a..611597e 100644
>> --- a/drivers/vfio/platform/vfio_platform_common.c
>> +++ b/drivers/vfio/platform/vfio_platform_common.c
>> @@ -25,6 +25,9 @@
>>
>> static DEFINE_MUTEX(driver_lock);
>>
>> +static const struct vfio_platform_reset_combo reset_lookup_table[] = {
>> +};
>> +
>> 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..9e37b9f 100644
>> --- a/drivers/vfio/platform/vfio_platform_private.h
>> +++ b/drivers/vfio/platform/vfio_platform_private.h
>> @@ -69,6 +69,12 @@ struct vfio_platform_device {
>> int (*get_irq)(struct vfio_platform_device *vdev, int i);
>> };
>>
>> +struct vfio_platform_reset_combo {
>> + const char *compat;
>> + const char *reset_function_name;
>> + const 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
>
>
>
On Fri, 2015-06-12 at 15:41 +0200, Eric Auger wrote:
> Hi Alex,
> On 06/11/2015 11:11 PM, Alex Williamson wrote:
> > On Thu, 2015-06-11 at 14:08 +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.
> >
> > The commit log no longer matches the code.
> yes indeed missed that.
> >
> > The only other thing I'm struggling with in this series is that in 0/4
> > you suggest that the reset modules can be in-kernel or external, but
> > we're making a static list here, so there's really no support for
> > random, user-provided reset modules. So are we missing the mark on the
> > requirements?
>
> Well personally I do not have a MUST HAVE requirement for that feature.
> My main requirement was to find a way to stop DMA/IRQ transfers
> programmed by a previous VM potentially jeopardizing the integrity of a
> second VM.
> >
> > One way I thought you could achieve your requirement would be if we did
> > away with the lookup table and looked for the module and function using
> > a pre-defined transform on the compat ID. For instance, a compat ID of
> > "calxeda,hb-xgmac" would automatically request a module named
> > "vfio-platform-reset-calxedahb-xgmac" and look for a symbol of the same
> > name for the reset function (I wonder if we can actually have a module
> > and symbol of the same name).
> I just tried and it works
> It seems fairly safe since an external
> > module would need to be explicitly placed in the search path for the
> > userspace module loader.
> we talked together with Christoffer about such a technique at the very
> beginning and he was not very fond of it, advising an approach using a
> defined API.
> >
> > Otherwise the table would need to become a list, the external module
> > would need to be manually loaded, and the module_init() would need to
> > register an entry on that list.
> Isn't it what I attempted to do in v2 or do you mean something else?
IIRC, v2 required all modules to be manually loaded, which I think was
bound to be a support issue. In-kernel modules should definitely have
the benefit of auto-loading, but if we want to support external modules,
I'm just hypothesizing that we either need to standardize naming so that
we can ask for them or require registration. In-kernel modules could
still effectively be pre-registered and requested on-demand.
> Having a whitelist brings the benefit to know which devices really are
> used with vfio-platform (besides some devices may not need any reset
> module). I don't know yet whether this is something that may slow down
> the adoption.
>
> Personally I would keep it as is for now and see how it gets used and
> whether users complain, ...
>
> If you agree I will respin just reworking the commit message.
If there's really no external module requirement, then your current
approach is fine. We're not creating anything here that can't later be
modified. Thanks,
Alex
> >> ---
> >>
> >> v2 -> v3:
> >> - add const in struct vfio_platform_reset_combo
> >> - remove enum vfio_platform_reset_type
> >>
> >> v2: creation
> >> ---
> >> drivers/vfio/platform/vfio_platform_common.c | 3 +++
> >> drivers/vfio/platform/vfio_platform_private.h | 6 ++++++
> >> 2 files changed, 9 insertions(+)
> >>
> >> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> >> index abcff7a..611597e 100644
> >> --- a/drivers/vfio/platform/vfio_platform_common.c
> >> +++ b/drivers/vfio/platform/vfio_platform_common.c
> >> @@ -25,6 +25,9 @@
> >>
> >> static DEFINE_MUTEX(driver_lock);
> >>
> >> +static const struct vfio_platform_reset_combo reset_lookup_table[] = {
> >> +};
> >> +
> >> 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..9e37b9f 100644
> >> --- a/drivers/vfio/platform/vfio_platform_private.h
> >> +++ b/drivers/vfio/platform/vfio_platform_private.h
> >> @@ -69,6 +69,12 @@ struct vfio_platform_device {
> >> int (*get_irq)(struct vfio_platform_device *vdev, int i);
> >> };
> >>
> >> +struct vfio_platform_reset_combo {
> >> + const char *compat;
> >> + const char *reset_function_name;
> >> + const 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
> >
> >
> >
>
Hi Alex,
On 06/12/2015 04:19 PM, Alex Williamson wrote:
> On Fri, 2015-06-12 at 15:41 +0200, Eric Auger wrote:
>> Hi Alex,
>> On 06/11/2015 11:11 PM, Alex Williamson wrote:
>>> On Thu, 2015-06-11 at 14:08 +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.
>>>
>>> The commit log no longer matches the code.
>> yes indeed missed that.
>>>
>>> The only other thing I'm struggling with in this series is that in 0/4
>>> you suggest that the reset modules can be in-kernel or external, but
>>> we're making a static list here, so there's really no support for
>>> random, user-provided reset modules. So are we missing the mark on the
>>> requirements?
>>
>> Well personally I do not have a MUST HAVE requirement for that feature.
>> My main requirement was to find a way to stop DMA/IRQ transfers
>> programmed by a previous VM potentially jeopardizing the integrity of a
>> second VM.
>>>
>>> One way I thought you could achieve your requirement would be if we did
>>> away with the lookup table and looked for the module and function using
>>> a pre-defined transform on the compat ID. For instance, a compat ID of
>>> "calxeda,hb-xgmac" would automatically request a module named
>>> "vfio-platform-reset-calxedahb-xgmac" and look for a symbol of the same
>>> name for the reset function (I wonder if we can actually have a module
>>> and symbol of the same name).
>> I just tried and it works
>> It seems fairly safe since an external
>>> module would need to be explicitly placed in the search path for the
>>> userspace module loader.
>> we talked together with Christoffer about such a technique at the very
>> beginning and he was not very fond of it, advising an approach using a
>> defined API.
>>>
>>> Otherwise the table would need to become a list, the external module
>>> would need to be manually loaded, and the module_init() would need to
>>> register an entry on that list.
>> Isn't it what I attempted to do in v2 or do you mean something else?
>
> IIRC, v2 required all modules to be manually loaded, which I think was
> bound to be a support issue.
In-kernel modules should definitely have
> the benefit of auto-loading,
yes you're right! thanks for your clarification.
but if we want to support external modules,
> I'm just hypothesizing that we either need to standardize naming so that
> we can ask for them or require registration. In-kernel modules could
> still effectively be pre-registered and requested on-demand.
>> Having a whitelist brings the benefit to know which devices really are
>> used with vfio-platform (besides some devices may not need any reset
>> module). I don't know yet whether this is something that may slow down
>> the adoption.
>>
>> Personally I would keep it as is for now and see how it gets used and
>> whether users complain, ...
>>
>> If you agree I will respin just reworking the commit message.
>
> If there's really no external module requirement, then your current
> approach is fine. We're not creating anything here that can't later be
> modified. Thanks,
OK. I will send v4 with updated commit messages early next week.
Have a nice week end.
Eric
>
> Alex
>
>
>>>> ---
>>>>
>>>> v2 -> v3:
>>>> - add const in struct vfio_platform_reset_combo
>>>> - remove enum vfio_platform_reset_type
>>>>
>>>> v2: creation
>>>> ---
>>>> drivers/vfio/platform/vfio_platform_common.c | 3 +++
>>>> drivers/vfio/platform/vfio_platform_private.h | 6 ++++++
>>>> 2 files changed, 9 insertions(+)
>>>>
>>>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>>>> index abcff7a..611597e 100644
>>>> --- a/drivers/vfio/platform/vfio_platform_common.c
>>>> +++ b/drivers/vfio/platform/vfio_platform_common.c
>>>> @@ -25,6 +25,9 @@
>>>>
>>>> static DEFINE_MUTEX(driver_lock);
>>>>
>>>> +static const struct vfio_platform_reset_combo reset_lookup_table[] = {
>>>> +};
>>>> +
>>>> 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..9e37b9f 100644
>>>> --- a/drivers/vfio/platform/vfio_platform_private.h
>>>> +++ b/drivers/vfio/platform/vfio_platform_private.h
>>>> @@ -69,6 +69,12 @@ struct vfio_platform_device {
>>>> int (*get_irq)(struct vfio_platform_device *vdev, int i);
>>>> };
>>>>
>>>> +struct vfio_platform_reset_combo {
>>>> + const char *compat;
>>>> + const char *reset_function_name;
>>>> + const 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
>>>
>>>
>>>
>>
>
>
>