Re: [PATCH v9 01/12] vfio: Mediated device Core driver

[Kirti Wankhede]

>> Medisted bus driver is responsible to add/delete mediated devices to/from
> 
> Medisted -> Mediated
>

Thanks for pointing out the typeo. Correcting it.


>> VFIO group when devices are bound and unbound to the driver.
>>
>> 2. Physical device driver interface
>> This interface provides vendor driver the set APIs to manage physical
>> device related work in its driver. APIs are :
>>
>> * dev_attr_groups: attributes of the parent device.
>> * mdev_attr_groups: attributes of the mediated device.
>> * supported_type_groups: attributes to define supported type. This is
>> 			 mandatory field.
>> * create: to allocate basic resources in driver for a mediated device.
> 
> in 'which driver'? it should be clear to remove 'in driver' here
> 
>> * remove: to free resources in driver when mediated device is destroyed.
>> * open: open callback of mediated device
>> * release: release callback of mediated device
>> * read : read emulation callback.
>> * write: write emulation callback.
>> * mmap: mmap emulation callback.
>> * ioctl: ioctl callback.
> 
> You only highlight 'mandatory field' for supported_type_groups. What
> about other fields? Are all of them optional? Please clarify and also
> stay consistent to later code comment.
> 

'create' and 'remove' are mandatory. Updating the description here. Rest
all are not cross-checked in mdev core driver, like 'create' and
'remove' but yes rest are optional. If vendor driver don't want to
support emulated region they don't need read/write callbacks. Similarly
if vendor driver don't want to support mmap region, they don't need mmap
callback.

Code comments are consistent with this description.

...
>> +
>> +config VFIO_MDEV
>> +    tristate "Mediated device driver framework"
>> +    depends on VFIO
>> +    default n
>> +    help
>> +        Provides a framework to virtualize devices which don't have SR_IOV
>> +	capability built-in.
> 
> This statement is not accurate. A device can support SR-IOV, but in the same
> time using this mediated technology w/ SR-IOV capability disabled.
> 

If SR-IOV is supported why would user use this framework? SR-IOV would
give better performance.

...
>> +
>> +static struct mdev_device *__find_mdev_device(struct parent_device *parent,
>> +					      uuid_le uuid)
> 
> parent_find_mdev_device?
> 

This function search for mdev device with given UUID, so I think its
consistent what we have below for parent, __find_parent_device().

...

>> +static int mdev_device_remove_ops(struct mdev_device *mdev, bool force_remove)
>> +{
> 
> 
> Can you add some comment here about when force_remove may be expected
> here, which would help others understand immediately instead of walking through
> the whole patch set?
>


mdev_device_remove_ops gets called from sysfs's 'remove' and when parent
device is being unregistered from mdev device framework.
- 'force_remove' is set to 'false' when called from sysfs's 'remove'
which indicates that if the mdev device is active, used by VMM or
userspace application, vendor driver could return error then don't
remove the device.
- 'force_remove' is set to 'true' when called from
mdev_unregister_device() which indicate that parent device is being
removed from mdev device framework so remove mdev device forcefully.

> 
>> +	struct parent_device *parent = mdev->parent;
>> +	int ret;
>> +
>> +	/*
>> +	 * Vendor driver can return error if VMM or userspace application is
>> +	 * using this mdev device.
>> +	 */
>> +	ret = parent->ops->remove(mdev);
> 
> what about passing force_remove flag to remove callback, so vendor driver
> can decide whether any force cleanup required?
>

'remove' getting called from sysfs is asynchronous, so vendor driver can
retrun failure in that case if vendor driver finds that mdev device is
being actively used.

mdev_unregister_device() is going to be called from vendor driver itself
when device is being unbound or driver is being unloaded. In this case
vendor driver can identify itself that its in its own teardown path.

So I feel there is no need to pass force_remove flag to 'remove' callback.



>> +int  mdev_create_sysfs_files(struct device *dev, struct mdev_type *type)
>> +{
>> +	int ret;
>> +
>> +	ret = sysfs_create_files(&dev->kobj, mdev_device_attrs);
>> +	if (ret) {
>> +		pr_err("Failed to create remove sysfs entry\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = sysfs_create_link(type->devices_kobj, &dev->kobj, dev_name(dev));
>> +	if (ret) {
>> +		pr_err("Failed to create symlink in types\n");
> 
> looks wrong place...
>

No, this is correct. Above function creates symlink in
mdev_supported_types/<type>/devices directory.

>> +		goto device_link_failed;
>> +	}
>> +
>> +	ret = sysfs_create_link(&dev->kobj, &type->kobj, "mdev_type");
>> +	if (ret) {
>> +		pr_err("Failed to create symlink in device directory\n");
> 
> exchange with above.
> 
Again this is also correct. Above creates 'mdev_type' symlink in mdev
device's directory.

Although these are correct, removing these error prints. You can find it
from its return type or if sysfs functions through warnings.

Kirti.