Message-ID: <5811972A.6020600@intel.com>
Date: Thu, 27 Oct 2016 13:56:58 +0800
From: Jike Song <jike.song@intel.com>
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 <alex.williamson@redhat.com>
CC: Kirti Wankhede <kwankhede@nvidia.com>, 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
Content-Length: 5996
Lines: 198

On 10/21/2016 01:12 AM, Alex Williamson wrote:
> On Thu, 20 Oct 2016 15:23:53 +0800
> Jike Song <jike.song@intel.com> 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 <cjia@nvidia.com>
>>> + *	       Kirti Wankhede <kwankhede@nvidia.com>
>>> + *
>>> + * 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 <linux/module.h>
>>> +#include <linux/device.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/uuid.h>
>>> +#include <linux/sysfs.h>
>>> +#include <linux/mdev.h>
>>> +
>>> +#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.

Hi Alex,

I'm sorry to say that my previous understanding is not fully correct.
Current combination of mdev and i915 are prone to panic the system
as long as both built into vmlinux.

mdev_init:

	mdev_bus_register();
	mdev_bus_compat_class = class_compat_register("mdev_bus");
	request_module_nowait("vfio_mdev");

mdev_register_device:

	class_compat_create_link(mdev_bus_compat_class, dev, NULL);


If both mdev and i915 are builtin, the class_compat_create_link call
will simply panic the system. People having such .config will be annoyed,
for example, when he tries to bisect.

I'm not arguing that mdev should be a subsys here, but there still
be a possible way out. Adding the class_compat_register into
mdev_register_device if not yet registered, will help out. Maybe
it isn't the typical location to register a class, nonetheless it
fix the problem here with least impact.

Looking forward to your opinion :)

--
Thanks,
Jike