lkml.org 
[lkml]   [2011]   [Dec]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRFC: Device isolation infrastructure

    Alex Williamson recently posted some patches adding a new hook to
    iommu_ops to identify groups of devices which cannot reliably be
    distinguished by the iommu, and therefore can only safely be assigned
    as a unit to a guest or userspace driver.

    I'm not very fond of that design, because:
    - It requires something else to scan the group ids to present the
    groups in a userspace discoverable manner, rather than handling that
    in the core.
    - It encorages manipulating devices individually, but only
    permitting certain actions if the whole group is together. This seems
    confusing and arse-backwards to me.
    - It does not provide an obvious means to prevent a kernel
    driver unsafely binding to a new device which is hotplugged into an
    iommu group already assigned to a guest.
    - It is inherently centred arounf the iommu structure, however
    there might be other platform dependent factors which also affect
    which devices can be safely isolated from others.


    These could probably each be addressed with various tweaks, but
    basically, I'm just not convinced that it represents the right
    structure for handling this. So, Alexey Kardashevskiy and myself havr
    made this patches, representing the outline of how I think this should
    be handled.


    Notes:
    * All names are negotiable. At the moment I'm thinking of this as
    the "device isolation subsystem" which handles "device isolation
    groups" (not to be confused with an iommu domain, which could contain
    one or more groups). A group is considered "detached" when it has
    been removed from the normal driver binding process, and therefore
    ready for assignment to a guest or userspace. I'm more than open to
    suggestions of better names.
    * This is just the generic infrastructure. Obviously, platform
    specific code will be needed as well to create the groups and assign
    devices to them. We have a proof of concept implementation for the
    power p5ioc2 pci host bridge, and I hope to post code for that and the
    p7ioc bridge shortly. I'll also be looking at how this would be used
    by Intel VTd, and AMD iommus.
    * isolation groups appear in /sys/isolation. Isolatable devices
    have a link to their group. The group contains links to all their
    devices, and a "detached" attribute, which can be used to view and
    control whether the group is detached.
    * At present groups are detached manually via sysfs, but the idea
    is a kernel system like vfio, could also directly request a group to
    be detached for its use.
    * when detached, all kernel drivers are unbound from every device
    in the group. No driver will match any device in the group until it
    is reattached.

    * There are probably stupid bugs, in particular I know some error
    paths are broken. By all means point these out to expedite fixing
    them.
    * The isolation_group structure is more or less a stub at this
    point. It will probably want a pointer to an iommu_ops and/or its own
    ops structure. The 'detached' bool may need to become some sort of
    state variable. We'll probably want some sort of "owner" field for
    what (e.g. vfio) is using the group when its detached.
    * I've also considered adding an "isolation strength" bitmask to
    indicate how well isolated the group is - e.g DMA isolated (iommu),
    irq isolated (irq remapping iommu, or other mechanism), error isolated
    (bus errors from this device can't affect other devices), ..

    Anyway, on with the code..

    From: David Gibson <david@gibson.dropbear.id.au>
    To: Alex Williamson <alex.williamson@redhat.com>,
    joerg.roedel@amd.com, dwmw2@infradead.org,
    iommu@lists.linux-foundation.org,
    aik@au1.ibm.com
    Cc: linux-kernel@vger.kernel.org, chrisw@redhat.com,
    agraf@suse.de, scottwood@freescale.com, B08248@freescale.com,
    benh@kernel.crashing.org
    Bcc:
    Subject: RFC: Device isolation infrastructure
    Reply-To:


    Alex Williamson recently posted some patches adding a new hook to
    iommu_ops to identify groups of devices which cannot reliably be
    distinguished by the iommu, and therefore can only safely be assigned
    as a unit to a guest or userspace driver.

    I'm not very fond of that design, because:
    - It requires something else to scan the group ids to present the
    groups in a userspace discoverable manner, rather than handling that
    in the core.
    - It encorages manipulating devices individually, but only
    permitting certain actions if the whole group is together. This seems
    confusing and arse-backwards to me.
    - It does not provide an obvious means to prevent a kernel
    driver unsafely binding to a new device which is hotplugged into an
    iommu group already assigned to a guest.
    - It is inherently centred arounf the iommu structure, however
    there might be other platform dependent factors which also affect
    which devices can be safely isolated from others.


    These could probably each be addressed with various tweaks, but
    basically, I'm just not convinced that it represents the right
    structure for handling this. So, Alexey Kardashevskiy and myself havr
    made this patches, representing the outline of how I think this should
    be handled.


    Notes:
    * All names are negotiable. At the moment I'm thinking of this as
    the "device isolation subsystem" which handles "device isolation
    groups" (not to be confused with an iommu domain, which could contain
    one or more groups). A group is considered "detached" when it has
    been removed from the normal driver binding process, and therefore
    ready for assignment to a guest or userspace. I'm more than open to
    suggestions of better names.
    * This is just the generic infrastructure. Obviously, platform
    specific code will be needed as well to create the groups and assign
    devices to them. We have a proof of concept implementation for the
    power p5ioc2 pci host bridge, and I hope to post code for that and the
    p7ioc bridge shortly. I'll also be looking at how this would be used
    by Intel VTd, and AMD iommus.
    * isolation groups appear in /sys/isolation. Isolatable devices
    have a link to their group. The group contains links to all their
    devices, and a "detached" attribute, which can be used to view and
    control whether the group is detached.
    * At present groups are detached manually via sysfs, but the idea
    is a kernel system like vfio, could also directly request a group to
    be detached for its use.
    * when detached, all kernel drivers are unbound from every device
    in the group. No driver will match any device in the group until it
    is reattached.

    * There are probably stupid bugs, in particular I know some error
    paths are broken. By all means point these out to expedite fixing
    them.
    * The isolation_group structure is more or less a stub at this
    point. It will probably want a pointer to an iommu_ops and/or its own
    ops structure. The 'detached' bool may need to become some sort of
    state variable. We'll probably want some sort of "owner" field for
    what (e.g. vfio) is using the group when its detached.
    * I've also considered adding an "isolation strength" bitmask to
    indicate how well isolated the group is - e.g DMA isolated (iommu),
    irq isolated (irq remapping iommu, or other mechanism), error isolated
    (bus errors from this device can't affect other devices), ..

    Anyway, on with the code..

    Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
    Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

    ---
    drivers/Kconfig | 2 +
    drivers/Makefile | 2 +
    drivers/base/base.h | 5 +
    drivers/base/core.c | 7 +
    drivers/base/init.c | 2 +
    drivers/isolation/Kconfig | 3 +
    drivers/isolation/Makefile | 2 +
    drivers/isolation/device_isolation.c | 216 ++++++++++++++++++++++++++++++++++
    include/linux/device.h | 5 +
    include/linux/device_isolation.h | 79 ++++++++++++
    10 files changed, 323 insertions(+), 0 deletions(-)
    create mode 100644 drivers/isolation/Kconfig
    create mode 100644 drivers/isolation/Makefile
    create mode 100644 drivers/isolation/device_isolation.c
    create mode 100644 include/linux/device_isolation.h

    diff --git a/drivers/Kconfig b/drivers/Kconfig
    index 95b9e7e..2ce5717 100644
    --- a/drivers/Kconfig
    +++ b/drivers/Kconfig
    @@ -130,4 +130,6 @@ source "drivers/iommu/Kconfig"

    source "drivers/virt/Kconfig"

    +source "drivers/isolation/Kconfig"
    +
    endmenu
    diff --git a/drivers/Makefile b/drivers/Makefile
    index 7fa433a..741874a 100644
    --- a/drivers/Makefile
    +++ b/drivers/Makefile
    @@ -127,3 +127,5 @@ obj-$(CONFIG_IOMMU_SUPPORT) += iommu/

    # Virtualization drivers
    obj-$(CONFIG_VIRT_DRIVERS) += virt/
    +
    +obj-$(CONFIG_DEVICE_ISOLATION) += isolation/
    diff --git a/drivers/base/base.h b/drivers/base/base.h
    index a34dca0..66a4ed2 100644
    --- a/drivers/base/base.h
    +++ b/drivers/base/base.h
    @@ -24,6 +24,9 @@
    * bus_type/class to be statically allocated safely. Nothing outside of the
    * driver core should ever touch these fields.
    */
    +
    +#include <linux/device_isolation.h>
    +
    struct subsys_private {
    struct kset subsys;
    struct kset *devices_kset;
    @@ -108,6 +111,8 @@ extern int driver_probe_device(struct device_driver *drv, struct device *dev);
    static inline int driver_match_device(struct device_driver *drv,
    struct device *dev)
    {
    + if (isolation_device_is_detached(dev))
    + return 0;
    return drv->bus->match ? drv->bus->match(dev, drv) : 1;
    }

    diff --git a/drivers/base/core.c b/drivers/base/core.c
    index bc8729d..45201c5 100644
    --- a/drivers/base/core.c
    +++ b/drivers/base/core.c
    @@ -22,6 +22,7 @@
    #include <linux/kallsyms.h>
    #include <linux/mutex.h>
    #include <linux/async.h>
    +#include <linux/device_isolation.h>

    #include "base.h"
    #include "power/power.h"
    @@ -593,6 +594,10 @@ void device_initialize(struct device *dev)
    lockdep_set_novalidate_class(&dev->mutex);
    spin_lock_init(&dev->devres_lock);
    INIT_LIST_HEAD(&dev->devres_head);
    +#ifdef CONFIG_DEVICE_ISOLATION
    + INIT_LIST_HEAD(&dev->isolation_list);
    + dev->isolation_group = NULL;
    +#endif
    device_pm_init(dev);
    set_dev_node(dev, -1);
    }
    @@ -993,6 +998,8 @@ int device_add(struct device *dev)
    class_intf->add_dev(dev, class_intf);
    mutex_unlock(&dev->class->p->class_mutex);
    }
    +
    + isolation_group_update_links(dev);
    done:
    put_device(dev);
    return error;
    diff --git a/drivers/base/init.c b/drivers/base/init.c
    index c8a934e..89f3e94 100644
    --- a/drivers/base/init.c
    +++ b/drivers/base/init.c
    @@ -8,6 +8,7 @@
    #include <linux/device.h>
    #include <linux/init.h>
    #include <linux/memory.h>
    +#include <linux/device_isolation.h>

    #include "base.h"

    @@ -24,6 +25,7 @@ void __init driver_init(void)
    devices_init();
    buses_init();
    classes_init();
    + isolation_init();
    firmware_init();
    hypervisor_init();

    diff --git a/drivers/isolation/Kconfig b/drivers/isolation/Kconfig
    new file mode 100644
    index 0000000..a2a7703
    --- /dev/null
    +++ b/drivers/isolation/Kconfig
    @@ -0,0 +1,3 @@
    +config DEVICE_ISOLATION
    + bool "Enable isolating devices for safe pass-through to guests or user space."
    +
    diff --git a/drivers/isolation/Makefile b/drivers/isolation/Makefile
    new file mode 100644
    index 0000000..8f1c6ec
    --- /dev/null
    +++ b/drivers/isolation/Makefile
    @@ -0,0 +1,2 @@
    +obj-$(CONFIG_DEVICE_ISOLATION) := device_isolation.o
    +
    diff --git a/drivers/isolation/device_isolation.c b/drivers/isolation/device_isolation.c
    new file mode 100644
    index 0000000..2b00ff7
    --- /dev/null
    +++ b/drivers/isolation/device_isolation.c
    @@ -0,0 +1,216 @@
    +#include <linux/slab.h>
    +#include <linux/device_isolation.h>
    +
    +#define KERN_DEBUG KERN_WARNING
    +
    +
    +static struct kset *isolation_kset;
    +
    +#define to_isolation_attr(_attr) container_of(_attr, \
    + struct isolation_attribute, attr)
    +
    +static ssize_t isolation_attr_show(struct kobject *kobj,
    + struct attribute *attr, char *buf)
    +{
    + struct isolation_attribute *isolation_attr = to_isolation_attr(attr);
    + struct isolation_group *group = container_of(kobj,
    + struct isolation_group, kobj);
    + ssize_t ret = -EIO;
    +
    + if (isolation_attr->show)
    + ret = isolation_attr->show(group, buf);
    + return ret;
    +}
    +
    +static ssize_t isolation_attr_store(struct kobject *kobj,
    + struct attribute *attr, const char *buf, size_t count)
    +{
    + struct isolation_attribute *isolation_attr = to_isolation_attr(attr);
    + struct isolation_group *group = container_of(kobj,
    + struct isolation_group, kobj);
    + ssize_t ret = -EIO;
    +
    + if (isolation_attr->store)
    + ret = isolation_attr->store(group, buf, count);
    + return ret;
    +}
    +
    +static void isolation_release(struct kobject *kobj)
    +{
    + struct isolation_group *group = container_of(kobj,
    + struct isolation_group, kobj);
    +
    + pr_debug("isolation : release.\n");
    +
    + kfree(group);
    +}
    +
    +static const struct sysfs_ops isolation_sysfs_ops = {
    + .show = isolation_attr_show,
    + .store = isolation_attr_store,
    +};
    +
    +static struct kobj_type isolation_ktype = {
    + .sysfs_ops = &isolation_sysfs_ops,
    + .release = isolation_release,
    +};
    +
    +static ssize_t group_showdetached(struct isolation_group *group,
    + char *buf)
    +{
    + return sprintf(buf, "%u", !!group->detached);
    +}
    +
    +static ssize_t group_setdetached(struct isolation_group *group,
    + const char *buf, size_t count)
    +{
    + switch (buf[0]) {
    + case '0':
    + isolation_group_reattach(group);
    + break;
    + case '1':
    + isolation_group_detach(group);
    + break;
    + default:
    + count = -EINVAL;
    + }
    + return count;
    +}
    +
    +static DEVICE_ISOLATION_ATTR(detached, S_IWUSR | S_IRUSR,
    + group_showdetached, group_setdetached);
    +
    +struct isolation_group *isolation_group_new(const char *name)
    +{
    + int ret;
    + struct isolation_group *group;
    +
    + group = kzalloc(sizeof(*group), GFP_KERNEL);
    + if (!group)
    + return NULL;
    +
    + spin_lock_init(&group->lock);
    +
    + group->kobj.kset = isolation_kset;
    + ret = kobject_init_and_add(&group->kobj, &isolation_ktype, NULL, name);
    + if (ret < 0) {
    + printk(KERN_ERR "device_isolation: kobject_init_and_add failed "
    + "for %s\n", group->kobj.name);
    + return NULL;
    + }
    +
    + ret = sysfs_create_file(&group->kobj, &isolation_attr_detached.attr);
    + if (0 > ret)
    + printk(KERN_WARNING "device_isolation: create \"detached\" failed "
    + "for %s, errno=%i\n", group->kobj.name, ret);
    +
    + INIT_LIST_HEAD(&group->devices);
    + printk(KERN_DEBUG "device_isolation: group %s created\n",
    + group->kobj.name);
    +
    + return group;
    +}
    +
    +void isolation_group_dev_add(struct isolation_group *group, struct device *dev)
    +{
    + printk(KERN_DEBUG "device_isolation: adding device %s to group %s\n",
    + dev->kobj.name, group->kobj.name);
    + spin_lock(&group->lock);
    +
    + list_add_tail(&dev->isolation_list, &group->devices);
    + dev->isolation_group = group;
    + /* Todo: remove links we already created */
    + spin_unlock(&group->lock);
    +}
    +
    +void isolation_group_dev_remove(struct device *dev)
    +{
    + list_del(&dev->isolation_list);
    +}
    +
    +int isolation_group_update_links(struct device *dev)
    +{
    + int error;
    + char *buf;
    + struct isolation_group *group = dev->isolation_group;
    +
    + if (!group) {
    + printk(KERN_DEBUG "device_isolation: no group for %s\n",
    + dev->kobj.name);
    + return 0;
    + }
    +
    + printk(KERN_DEBUG "device_isolation: adding device %s to group %s\n",
    + dev->kobj.name, group->kobj.name);
    +
    + spin_lock(&group->lock);
    +
    + error = sysfs_create_link(&dev->kobj, &group->kobj, "isolation_group");
    + if (0 > error)
    + printk(KERN_WARNING "device_isolation: create isolation_group "
    + "link failed for %s -> %s, errno=%i\n",
    + dev->kobj.name, group->kobj.name, error);
    +
    + buf = kasprintf(GFP_KERNEL, "%s-%p", dev->kobj.name, dev);
    + if (!buf) {
    + error = -ENOMEM;
    + printk(KERN_ERR "device_isolation: kasprintf failed\n");
    + goto out;
    + }
    +
    + error = sysfs_create_link(&group->kobj, &dev->kobj, buf);
    + kfree(buf);
    + if (0 > error)
    + printk(KERN_WARNING "device_isolation: create %s "
    + "link failed for %s -> %s, errno=%i\n",
    + buf, dev->kobj.name, group->kobj.name, error);
    +
    +out:
    + /* Todo: remove links we already created */
    + spin_unlock(&group->lock);
    + return error;
    +}
    +
    +int isolation_group_detach(struct isolation_group *group)
    +{
    + int error;
    + struct device *dev;
    +
    + spin_lock(&group->lock);
    +
    + group->detached = true;
    + list_for_each_entry(dev, &group->devices, isolation_list) {
    + error = device_reprobe(dev);
    + }
    +
    + spin_unlock(&group->lock);
    +
    + return 0;
    +}
    +
    +int isolation_group_reattach(struct isolation_group *group)
    +{
    + int error;
    + struct device *dev;
    +
    + spin_lock(&group->lock);
    +
    + group->detached = false;
    + list_for_each_entry(dev, &group->devices, isolation_list) {
    + printk("Reprobing %s\n", dev->kobj.name);
    + error = device_reprobe(dev);
    + }
    +
    + spin_unlock(&group->lock);
    +
    + return 0;
    +}
    +
    +int __init isolation_init(void)
    +{
    + isolation_kset = kset_create_and_add("isolation", NULL, NULL);
    + if (!isolation_kset)
    + return -ENOMEM;
    + return 0;
    +}
    +
    diff --git a/include/linux/device.h b/include/linux/device.h
    index c20dfbf..6238a13 100644
    --- a/include/linux/device.h
    +++ b/include/linux/device.h
    @@ -585,6 +585,11 @@ struct device {

    struct dma_coherent_mem *dma_mem; /* internal for coherent mem
    override */
    +#ifdef CONFIG_DEVICE_ISOLATION
    + struct isolation_group *isolation_group;
    + struct list_head isolation_list;
    +#endif
    +
    /* arch specific additions */
    struct dev_archdata archdata;

    diff --git a/include/linux/device_isolation.h b/include/linux/device_isolation.h
    new file mode 100644
    index 0000000..1dc7e68
    --- /dev/null
    +++ b/include/linux/device_isolation.h
    @@ -0,0 +1,79 @@
    +#ifndef _DEVICE_ISOLATION_H_
    +#define _DEVICE_ISOLATION_H_
    +
    +#include <linux/kobject.h>
    +#include <linux/list.h>
    +#include <linux/device.h>
    +
    +struct isolation_group {
    + struct kobject kobj;
    + struct list_head devices;
    + spinlock_t lock;
    + bool detached;
    +};
    +
    +struct isolation_attribute {
    + struct attribute attr;
    + ssize_t (*show)(struct isolation_group *group, char *buf);
    + ssize_t (*store)(struct isolation_group *group, const char *buf,
    + size_t count);
    +};
    +
    +#ifdef CONFIG_DEVICE_ISOLATION
    +
    +int __init isolation_init(void);
    +
    +struct isolation_group *isolation_group_new(const char *name);
    +int isolation_group_delete(struct isolation_group *group);
    +
    +void isolation_group_dev_add(struct isolation_group *group,
    + struct device *dev);
    +void isolation_group_dev_remove(struct device *dev);
    +
    +int isolation_group_update_links(struct device *dev);
    +int isolation_group_detach(struct isolation_group *group);
    +int isolation_group_reattach(struct isolation_group *group);
    +
    +#define DEVICE_ISOLATION_ATTR(_name, _mode, _show, _store) \
    + struct isolation_attribute isolation_attr_##_name = \
    + __ATTR(_name, _mode, _show, _store)
    +
    +static inline int isolation_device_is_detached(struct device *dev)
    +{
    + return dev->isolation_group && dev->isolation_group->detached;
    +}
    +
    +#else
    +
    +static inline int __init isolation_init(void)
    +{
    + return 0;
    +}
    +
    +static inline struct isolation_group *isolation_group_new(const char *name)
    +{
    + return NULL;
    +}
    +
    +/*void isolation_group_dev_add(struct isolation_group *group,
    + struct device *dev)
    +{
    +}*/
    +
    +static inline void isolation_group_dev_remove(struct device *dev)
    +{
    +}
    +
    +static inline int isolation_group_update_links(struct device *dev)
    +{
    + return 0;
    +}
    +
    +static inline int isolation_is_detached(struct device *dev)
    +{
    + return 0;
    +}
    +
    +#endif
    +
    +#endif
    --
    1.7.7.3

    --
    David Gibson | I'll have my music baroque, and my code
    david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
    | _way_ _around_!
    http://www.ozlabs.org/~dgibson

    \
     
     \ /
      Last update: 2011-12-07 05:01    [W:0.058 / U:62.100 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site