From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754396AbcJUCoa (ORCPT ); Thu, 20 Oct 2016 22:44:30 -0400 Received: from mga05.intel.com ([192.55.52.43]:32390 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751839AbcJUCo2 (ORCPT ); Thu, 20 Oct 2016 22:44:28 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,374,1473145200"; d="scan'208";a="182152438" Message-ID: <58098052.2080802@intel.com> Date: Fri, 21 Oct 2016 10:41:22 +0800 From: Jike Song User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:17.0) Gecko/20130801 Thunderbird/17.0.8 MIME-Version: 1.0 To: Alex Williamson CC: Kirti Wankhede , pbonzini@redhat.com, kraxel@redhat.com, cjia@nvidia.com, qemu-devel@nongnu.org, kvm@vger.kernel.org, kevin.tian@intel.com, bjsdjshi@linux.vnet.ibm.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH v9 01/12] vfio: Mediated device Core driver References: <1476739332-4911-1-git-send-email-kwankhede@nvidia.com> <1476739332-4911-2-git-send-email-kwankhede@nvidia.com> <58087109.8010308@intel.com> <20161020111231.6fbbd421@t450s.home> In-Reply-To: <20161020111231.6fbbd421@t450s.home> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 10/21/2016 01:12 AM, Alex Williamson wrote: > On Thu, 20 Oct 2016 15:23:53 +0800 > Jike Song wrote: > >> On 10/18/2016 05:22 AM, Kirti Wankhede wrote: >>> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c >>> new file mode 100644 >>> index 000000000000..7db5ec164aeb >>> --- /dev/null >>> +++ b/drivers/vfio/mdev/mdev_core.c >>> @@ -0,0 +1,372 @@ >>> +/* >>> + * Mediated device Core Driver >>> + * >>> + * Copyright (c) 2016, NVIDIA CORPORATION. All rights reserved. >>> + * Author: Neo Jia >>> + * Kirti Wankhede >>> + * >>> + * This program is free software; you can redistribute it and/or modify >>> + * it under the terms of the GNU General Public License version 2 as >>> + * published by the Free Software Foundation. >>> + */ >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +#include "mdev_private.h" >>> + >>> +#define DRIVER_VERSION "0.1" >>> +#define DRIVER_AUTHOR "NVIDIA Corporation" >>> +#define DRIVER_DESC "Mediated device Core Driver" >>> + >>> +static LIST_HEAD(parent_list); >>> +static DEFINE_MUTEX(parent_list_lock); >>> +static struct class_compat *mdev_bus_compat_class; >>> + >> >>> + >>> +/* >>> + * mdev_register_device : Register a device >>> + * @dev: device structure representing parent device. >>> + * @ops: Parent device operation structure to be registered. >>> + * >>> + * Add device to list of registered parent devices. >>> + * Returns a negative value on error, otherwise 0. >>> + */ >>> +int mdev_register_device(struct device *dev, const struct parent_ops *ops) >>> +{ >>> + int ret = 0; >>> + struct parent_device *parent; >>> + >>> + /* check for mandatory ops */ >>> + if (!ops || !ops->create || !ops->remove || !ops->supported_type_groups) >>> + return -EINVAL; >>> + >>> + dev = get_device(dev); >>> + if (!dev) >>> + return -EINVAL; >>> + >>> + mutex_lock(&parent_list_lock); >>> + >>> + /* Check for duplicate */ >>> + parent = __find_parent_device(dev); >>> + if (parent) { >>> + ret = -EEXIST; >>> + goto add_dev_err; >>> + } >>> + >>> + parent = kzalloc(sizeof(*parent), GFP_KERNEL); >>> + if (!parent) { >>> + ret = -ENOMEM; >>> + goto add_dev_err; >>> + } >>> + >>> + kref_init(&parent->ref); >>> + >>> + parent->dev = dev; >>> + parent->ops = ops; >>> + >>> + ret = parent_create_sysfs_files(parent); >>> + if (ret) { >>> + mutex_unlock(&parent_list_lock); >>> + mdev_put_parent(parent); >>> + return ret; >>> + } >>> + >>> + ret = class_compat_create_link(mdev_bus_compat_class, dev, NULL); >>> + if (ret) >>> + dev_warn(dev, "Failed to create compatibility class link\n"); >>> + >>> + list_add(&parent->next, &parent_list); >>> + mutex_unlock(&parent_list_lock); >>> + >>> + dev_info(dev, "MDEV: Registered\n"); >>> + return 0; >>> + >>> +add_dev_err: >>> + mutex_unlock(&parent_list_lock); >>> + put_device(dev); >>> + return ret; >>> +} >>> +EXPORT_SYMBOL(mdev_register_device); >> >>> +static int __init mdev_init(void) >>> +{ >>> + int ret; >>> + >>> + ret = mdev_bus_register(); >>> + if (ret) { >>> + pr_err("Failed to register mdev bus\n"); >>> + return ret; >>> + } >>> + >>> + mdev_bus_compat_class = class_compat_register("mdev_bus"); >>> + if (!mdev_bus_compat_class) { >>> + mdev_bus_unregister(); >>> + return -ENOMEM; >>> + } >>> + >>> + /* >>> + * Attempt to load known vfio_mdev. This gives us a working environment >>> + * without the user needing to explicitly load vfio_mdev driver. >>> + */ >>> + request_module_nowait("vfio_mdev"); >>> + >>> + return ret; >>> +} >>> + >>> +static void __exit mdev_exit(void) >>> +{ >>> + class_compat_unregister(mdev_bus_compat_class); >>> + mdev_bus_unregister(); >>> +} >>> + >>> +module_init(mdev_init) >>> +module_exit(mdev_exit) >> >> Hi Kirti, >> >> There is a possible issue: mdev_bus_register is called from mdev_init, >> a module_init, equal to device_initcall if builtin to vmlinux; however, >> the vendor driver, say i915.ko for intel case, have to call >> mdev_register_device from its module_init: at that time, mdev_init >> is still not called. >> >> Not sure if this issue exists with nvidia.ko. Though in most cases we >> are expecting users select mdev as a standalone module, we still won't >> break builtin case. >> >> >> Hi Alex, do you have any suggestion here? > > To fully solve the problem of built-in drivers making use of the mdev > infrastructure we'd need to make mdev itself builtin and possibly a > subsystem that is initialized prior to device drivers. Is that really > necessary? Even though i915.ko is often loaded as part of an > initramfs, most systems still build it as a module. I would expect > that standard module dependencies will pull in the necessary mdev and > vfio modules to make this work correctly. I can't say that I'm > prepared to make mdev be a subsystem as would be necessary for builtin > drivers to make use of. Perhaps if such a driver exists it could > somehow do late binding with mdev. i915 should certainly be tested as > a builtin driver to make sure it doesn't fail with mdev support added. > The kvm-vfio device (virt/kvm/vfio.c) makes use of symbol tricks to > avoid hard dependencies between kvm and vfio, perhaps when builtin to > the kernel, i915 could use something like that. Thanks, Fair enough, I'll use symbol_get to avoid the problem. Thanks! -- Thanks, Jike From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46989) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bxPov-0008Ee-Nz for qemu-devel@nongnu.org; Thu, 20 Oct 2016 22:44:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bxPos-0008PK-KO for qemu-devel@nongnu.org; Thu, 20 Oct 2016 22:44:33 -0400 Received: from mga05.intel.com ([192.55.52.43]:45863) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bxPos-0008NZ-AE for qemu-devel@nongnu.org; Thu, 20 Oct 2016 22:44:30 -0400 Message-ID: <58098052.2080802@intel.com> Date: Fri, 21 Oct 2016 10:41:22 +0800 From: Jike Song MIME-Version: 1.0 References: <1476739332-4911-1-git-send-email-kwankhede@nvidia.com> <1476739332-4911-2-git-send-email-kwankhede@nvidia.com> <58087109.8010308@intel.com> <20161020111231.6fbbd421@t450s.home> In-Reply-To: <20161020111231.6fbbd421@t450s.home> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v9 01/12] vfio: Mediated device Core driver List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson Cc: Kirti Wankhede , pbonzini@redhat.com, kraxel@redhat.com, cjia@nvidia.com, qemu-devel@nongnu.org, kvm@vger.kernel.org, kevin.tian@intel.com, bjsdjshi@linux.vnet.ibm.com, linux-kernel@vger.kernel.org On 10/21/2016 01:12 AM, Alex Williamson wrote: > On Thu, 20 Oct 2016 15:23:53 +0800 > Jike Song wrote: > >> On 10/18/2016 05:22 AM, Kirti Wankhede wrote: >>> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c >>> new file mode 100644 >>> index 000000000000..7db5ec164aeb >>> --- /dev/null >>> +++ b/drivers/vfio/mdev/mdev_core.c >>> @@ -0,0 +1,372 @@ >>> +/* >>> + * Mediated device Core Driver >>> + * >>> + * Copyright (c) 2016, NVIDIA CORPORATION. All rights reserved. >>> + * Author: Neo Jia >>> + * Kirti Wankhede >>> + * >>> + * This program is free software; you can redistribute it and/or modify >>> + * it under the terms of the GNU General Public License version 2 as >>> + * published by the Free Software Foundation. >>> + */ >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +#include "mdev_private.h" >>> + >>> +#define DRIVER_VERSION "0.1" >>> +#define DRIVER_AUTHOR "NVIDIA Corporation" >>> +#define DRIVER_DESC "Mediated device Core Driver" >>> + >>> +static LIST_HEAD(parent_list); >>> +static DEFINE_MUTEX(parent_list_lock); >>> +static struct class_compat *mdev_bus_compat_class; >>> + >> >>> + >>> +/* >>> + * mdev_register_device : Register a device >>> + * @dev: device structure representing parent device. >>> + * @ops: Parent device operation structure to be registered. >>> + * >>> + * Add device to list of registered parent devices. >>> + * Returns a negative value on error, otherwise 0. >>> + */ >>> +int mdev_register_device(struct device *dev, const struct parent_ops *ops) >>> +{ >>> + int ret = 0; >>> + struct parent_device *parent; >>> + >>> + /* check for mandatory ops */ >>> + if (!ops || !ops->create || !ops->remove || !ops->supported_type_groups) >>> + return -EINVAL; >>> + >>> + dev = get_device(dev); >>> + if (!dev) >>> + return -EINVAL; >>> + >>> + mutex_lock(&parent_list_lock); >>> + >>> + /* Check for duplicate */ >>> + parent = __find_parent_device(dev); >>> + if (parent) { >>> + ret = -EEXIST; >>> + goto add_dev_err; >>> + } >>> + >>> + parent = kzalloc(sizeof(*parent), GFP_KERNEL); >>> + if (!parent) { >>> + ret = -ENOMEM; >>> + goto add_dev_err; >>> + } >>> + >>> + kref_init(&parent->ref); >>> + >>> + parent->dev = dev; >>> + parent->ops = ops; >>> + >>> + ret = parent_create_sysfs_files(parent); >>> + if (ret) { >>> + mutex_unlock(&parent_list_lock); >>> + mdev_put_parent(parent); >>> + return ret; >>> + } >>> + >>> + ret = class_compat_create_link(mdev_bus_compat_class, dev, NULL); >>> + if (ret) >>> + dev_warn(dev, "Failed to create compatibility class link\n"); >>> + >>> + list_add(&parent->next, &parent_list); >>> + mutex_unlock(&parent_list_lock); >>> + >>> + dev_info(dev, "MDEV: Registered\n"); >>> + return 0; >>> + >>> +add_dev_err: >>> + mutex_unlock(&parent_list_lock); >>> + put_device(dev); >>> + return ret; >>> +} >>> +EXPORT_SYMBOL(mdev_register_device); >> >>> +static int __init mdev_init(void) >>> +{ >>> + int ret; >>> + >>> + ret = mdev_bus_register(); >>> + if (ret) { >>> + pr_err("Failed to register mdev bus\n"); >>> + return ret; >>> + } >>> + >>> + mdev_bus_compat_class = class_compat_register("mdev_bus"); >>> + if (!mdev_bus_compat_class) { >>> + mdev_bus_unregister(); >>> + return -ENOMEM; >>> + } >>> + >>> + /* >>> + * Attempt to load known vfio_mdev. This gives us a working environment >>> + * without the user needing to explicitly load vfio_mdev driver. >>> + */ >>> + request_module_nowait("vfio_mdev"); >>> + >>> + return ret; >>> +} >>> + >>> +static void __exit mdev_exit(void) >>> +{ >>> + class_compat_unregister(mdev_bus_compat_class); >>> + mdev_bus_unregister(); >>> +} >>> + >>> +module_init(mdev_init) >>> +module_exit(mdev_exit) >> >> Hi Kirti, >> >> There is a possible issue: mdev_bus_register is called from mdev_init, >> a module_init, equal to device_initcall if builtin to vmlinux; however, >> the vendor driver, say i915.ko for intel case, have to call >> mdev_register_device from its module_init: at that time, mdev_init >> is still not called. >> >> Not sure if this issue exists with nvidia.ko. Though in most cases we >> are expecting users select mdev as a standalone module, we still won't >> break builtin case. >> >> >> Hi Alex, do you have any suggestion here? > > To fully solve the problem of built-in drivers making use of the mdev > infrastructure we'd need to make mdev itself builtin and possibly a > subsystem that is initialized prior to device drivers. Is that really > necessary? Even though i915.ko is often loaded as part of an > initramfs, most systems still build it as a module. I would expect > that standard module dependencies will pull in the necessary mdev and > vfio modules to make this work correctly. I can't say that I'm > prepared to make mdev be a subsystem as would be necessary for builtin > drivers to make use of. Perhaps if such a driver exists it could > somehow do late binding with mdev. i915 should certainly be tested as > a builtin driver to make sure it doesn't fail with mdev support added. > The kvm-vfio device (virt/kvm/vfio.c) makes use of symbol tricks to > avoid hard dependencies between kvm and vfio, perhaps when builtin to > the kernel, i915 could use something like that. Thanks, Fair enough, I'll use symbol_get to avoid the problem. Thanks! -- Thanks, Jike