Re: [Qemu-devel] [PATCH v8 4/6] docs: Add Documentation for Mediated devices

Kirti Wankhede
2016-10-13T09:22:48Z

On 10/13/2016 3:14 AM, Alex Williamson wrote:
> On Thu, 13 Oct 2016 00:32:48 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>
>> On 10/12/2016 9:29 PM, Alex Williamson wrote:
>>> On Wed, 12 Oct 2016 20:43:48 +0530
>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>>>
>>>> On 10/12/2016 7:22 AM, Tian, Kevin wrote:
>>>>>> From: Kirti Wankhede [mailto:kwankhede@nvidia.com]
>>>>>> Sent: Wednesday, October 12, 2016 4:45 AM
>>>>>>>> +* mdev_supported_types:
>>>>>>>> + List of current supported mediated device types and its details are added
>>>>>>>> +in this directory in following format:
>>>>>>>> +
>>>>>>>> +|- <parent phy device>
>>>>>>>> +|--- Vendor-specific-attributes [optional]
>>>>>>>> +|--- mdev_supported_types
>>>>>>>> +| |--- <type id>
>>>>>>>> +| | |--- create
>>>>>>>> +| | |--- name
>>>>>>>> +| | |--- available_instances
>>>>>>>> +| | |--- description /class
>>>>>>>> +| | |--- [devices]
>>>>>>>> +| |--- <type id>
>>>>>>>> +| | |--- create
>>>>>>>> +| | |--- name
>>>>>>>> +| | |--- available_instances
>>>>>>>> +| | |--- description /class
>>>>>>>> +| | |--- [devices]
>>>>>>>> +| |--- <type id>
>>>>>>>> +| |--- create
>>>>>>>> +| |--- name
>>>>>>>> +| |--- available_instances
>>>>>>>> +| |--- description /class
>>>>>>>> +| |--- [devices]
>>>>>>>> +
>>>>>>>> +[TBD : description or class is yet to be decided. This will change.]
>>>>>>>
>>>>>>> I thought that in previous discussions we had agreed to drop
>>>>>>> the <type id> concept and use the name as the unique identifier.
>>>>>>> When reporting these types in libvirt we won't want to report
>>>>>>> the type id values - we'll want the name strings to be unique.
>>>>>>>
>>>>>>
>>>>>> The 'name' might not be unique but type_id will be. For example that Neo
>>>>>> pointed out in earlier discussion, virtual devices can come from two
>>>>>> different physical devices, end user would be presented with what they
>>>>>> had selected but there will be internal implementation differences. In
>>>>>> that case 'type_id' will be unique.
>>>>>>
>>>>>
>>>>> Hi, Kirti, my understanding is that Neo agreed to use an unique type
>>>>> string (if you still called it <type id>), and then no need of additional
>>>>> 'name' field which can be put inside 'description' field. See below quote:
>>>>>
>>>>
>>>> We had internal discussions about this within NVIDIA and found that
>>>> 'name' might not be unique where as 'type_id' would be unique. I'm
>>>> refering to Neo's mail after that, where Neo do pointed that out.
>>>>
>>>> https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg07714.html
>>>
>>> Everyone not privy to those internal discussions, including me, seems to
>>> think we dropped type_id and that if a vendor does not have a stable
>>> name, they can compose some sort of stable type description based on the
>>> name+id, or even vendor+id, ex. NVIDIA-11. So please share why we
>>> haven't managed to kill off type_id yet. No matter what internal
>>> representation each vendor driver has of "type_id" it seems possible
>>> for it to come up with stable string to define a given configuration.
>>
>>
>> The 'type_id' is unique and the 'name' are not, the name is just a
>> virtual device name/ human readable name. Because at this moment Intel
>> can't define a proper GPU class, we have to add a 'description' field
>> there as well to represent the features of this virtual device, once we
>> have all agreed with the GPU class and its mandatory attributes, the
>> 'description' field can be removed. Here is an example,
>> type_id/type_name = NVIDIA_11,
>> name=M60-M0Q,
>> description=2560x1600, 2 displays, 512MB"
>>
>> Neo's previous comment only applies to the situation where we will have
>> the GPU class or optional attributes defined and recognized by libvirt,
>> since that is not going to happen any time soon, we will have to have
>> the new 'description' field, and we don't want to have it mixed up with
>> 'name' field.
>>
>> We can definitely have something like name+id as Alex recommended to
>> remove the 'name' field, but it will just require libvirt to have more
>> logic to parse that string.
>
> Let's use the mtty example driver provided in patch 5 so we can all
> more clearly see how the interfaces work. I'll start from the
> beginning of my experience and work my way to the type/name thing.
>

Thanks for looking into it and getting feel of it. And I hope this helps
to understand that 'name' and 'type_id' are different.

> (please add a modules_install target to the Makefile)
>

This is an example and I feel it should not be installed in
/lib/modules/../build path. This should be used to understand the
interface and the flow of mdev device management life cycle. User can
use insmod to load driver:

# insmod mtty.ko

> # modprobe mtty
>
> Now what? It seems like I need to have prior knowledge that this
> drivers supports mdev devices and I need to go hunt for them. We need
> to create a class (ex. /sys/class/mdev/) where a user can find all the
> devices that participate in this mediated device infrastructure. That
> would point me to /sys/devices/mtty.
>

You can find devices registered to mdev framework by searching for
'mdev_supported_types' directory at the leaf nodes of devices in
/sys/devices directory. Yes, we can have 'mdev' class and links to
devices which are registered to mdev framework in /sys/class/mdev/.

> # tree /sys/devices/mtty
> /sys/devices/mtty
> |-- mdev_supported_types
> | `-- mtty1
> | |-- available_instances (1)
> | |-- create
> | |-- devices
> | `-- name ("Dual-port-serial")
> |-- mtty_dev
> | `-- sample_mtty_dev ("This is phy device")
> |-- power
> | |-- async
> | |-- autosuspend_delay_ms
> | |-- control
> | |-- runtime_active_kids
> | |-- runtime_active_time
> | |-- runtime_enabled
> | |-- runtime_status
> | |-- runtime_suspended_time
> | `-- runtime_usage
> `-- uevent
>
> Ok, but that was boring, we really need to have at least 2 supported
> types to validate the interface, so without changing the actual device
> backing, I pretended to have a single port vs dual port:
>
> /sys/devices/mtty
> |-- mdev_supported_types
> | |-- mtty1
> | | |-- available_instances (24)
> | | |-- create
> | | |-- devices
> | | `-- name (Single-port-serial)
> | `-- mtty2
> | |-- available_instances (12)
> | |-- create
> | |-- devices
> | `-- name (Dual-port-serial)
> [snip]
>
> I arbitrarily decides I have 24 ports and each single port uses 1 port
> and each dual port uses 2 ports.
>
> Before I start creating devices, what are we going to key the libvirt
> XML on? Can we do anything to prevent vendors from colliding or do we
> have any way to suggest meaningful and unique type_ids?

Libvirt would have parent and type_id in XML. No two vendors can own
same parent device. So I don't think vendors would collide even having
same type_id, since <parent, type_id> pair would always be unique.

Presumably if
> we had a PCI device hosting this, we would be rooted at that parent
> device, ex. /sys/devices/pci0000:00/0000:00:01.0/0000:01:00.0. Maybe
> the type_id should automatically be prefixed by the vendor module name,
> ex. mtty-1, i915-foo, nvidia-bar. There's something missing for
> deterministically creating a "XYZ" device and knowing exactly what that
> means and finding a parent device that supports it.
>

We can prefix type_id with module name, i.e using dev->driver->name, but
<parent, type_id> pair is unique so I don't see much benefit in doing
that.

> Let's get to mdev creating...
>
> # uuidgen > mdev_supported_types/mtty2/create
> # tree /sys/devices/mtty
> /sys/devices/mtty
> |-- e68189be-700e-41f7-93a3-b5351e79c470
> | |-- driver -> ../../../bus/mdev/drivers/vfio_mdev
> | |-- iommu_group -> ../../../kernel/iommu_groups/63
> | |-- mtty2 -> ../mdev_supported_types/mtty2
> | |-- power
> | | |-- async
> | | |-- autosuspend_delay_ms
> | | |-- control
> | | |-- runtime_active_kids
> | | |-- runtime_active_time
> | | |-- runtime_enabled
> | | |-- runtime_status
> | | |-- runtime_suspended_time
> | | `-- runtime_usage
> | |-- remove
> | |-- subsystem -> ../../../bus/mdev
> | |-- uevent
> | `-- vendor
> | `-- sample_mdev_dev ("This is MDEV e68189be-700e-41f7-93a3-b5351e79c470")
> |-- mdev_supported_types
> | |-- mtty1
> | | |-- available_instances (22)
> | | |-- create
> | | |-- devices
> | | `-- name
> | `-- mtty2
> | |-- available_instances (11)
> | |-- create
> | |-- devices
> | | `-- e68189be-700e-41f7-93a3-b5351e79c470 -> ../../../e68189be-700e-41f7-93a3-b5351e79c470
> | `-- name
>
> The mdev device was created directly under the parent, which seems like
> it's going to get messy to me (ie. imagine dropping a bunch of uuids
> into a PCI parent device's sysfs directory, how does a user know what
> they are?).
>

That is the way devices are placed in sysfs. For example below devices:

80:01.0 PCI bridge: Intel Corporation Xeon E5/Core i7 IIO PCI Express
Root Port 1a (rev 07)
80:02.0 PCI bridge: Intel Corporation Xeon E5/Core i7 IIO PCI Express
Root Port 2a (rev 07)
80:03.0 PCI bridge: Intel Corporation Xeon E5/Core i7 IIO PCI Express
Root Port 3a in PCI Express Mode (rev 07)
80:04.0 System peripheral: Intel Corporation Xeon E5/Core i7 DMA Channel
0 (rev 07)
80:04.1 System peripheral: Intel Corporation Xeon E5/Core i7 DMA Channel
1 (rev 07)
80:04.2 System peripheral: Intel Corporation Xeon E5/Core i7 DMA Channel
2 (rev 07)
80:04.3 System peripheral: Intel Corporation Xeon E5/Core i7 DMA Channel
3 (rev 07)
80:04.4 System peripheral: Intel Corporation Xeon E5/Core i7 DMA Channel
4 (rev 07)
80:04.5 System peripheral: Intel Corporation Xeon E5/Core i7 DMA Channel
5 (rev 07)
80:04.6 System peripheral: Intel Corporation Xeon E5/Core i7 DMA Channel
6 (rev 07)
80:04.7 System peripheral: Intel Corporation Xeon E5/Core i7 DMA Channel
7 (rev 07)
80:05.0 System peripheral: Intel Corporation Xeon E5/Core i7 Address
Map, VTd_Misc, System Management (rev 07)
80:05.2 System peripheral: Intel Corporation Xeon E5/Core i7 Control
Status and Global Errors (rev 07)
80:05.4 PIC: Intel Corporation Xeon E5/Core i7 I/O APIC (rev 07)

In sysfs, those are in same parent folder of its parent root port:

# ls /sys/devices/pci0000\:80/ -l
total 0
drwxr-xr-x 8 root root 0 Oct 13 12:08 0000:80:01.0
drwxr-xr-x 7 root root 0 Oct 13 12:08 0000:80:02.0
drwxr-xr-x 6 root root 0 Oct 13 12:08 0000:80:03.0
drwxr-xr-x 6 root root 0 Oct 13 12:08 0000:80:04.0
drwxr-xr-x 6 root root 0 Oct 13 12:08 0000:80:04.1
drwxr-xr-x 6 root root 0 Oct 13 12:08 0000:80:04.2
drwxr-xr-x 6 root root 0 Oct 13 12:08 0000:80:04.3
drwxr-xr-x 6 root root 0 Oct 13 12:08 0000:80:04.4
drwxr-xr-x 6 root root 0 Oct 13 12:08 0000:80:04.5
drwxr-xr-x 6 root root 0 Oct 13 12:08 0000:80:04.6
drwxr-xr-x 6 root root 0 Oct 13 12:08 0000:80:04.7
drwxr-xr-x 3 root root 0 Oct 13 12:08 0000:80:05.0
drwxr-xr-x 3 root root 0 Oct 13 12:08 0000:80:05.2
drwxr-xr-x 3 root root 0 Oct 13 12:08 0000:80:05.4
lrwxrwxrwx 1 root root 0 Oct 13 13:25 firmware_node ->
../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:01
drwxr-xr-x 3 root root 0 Oct 13 12:08 pci_bus
drwxr-xr-x 2 root root 0 Oct 13 12:08 power
-rw-r--r-- 1 root root 4096 Oct 13 13:25 uevent

> Under the device we have "mtty2", shouldn't that be
> "mdev_supported_type", which then links to mtty2? Otherwise a user
> needs to decode from the link what this attribute is.
>

I thought it should show type, so that by looking at 'ls' output user
should be able to find type_id.

> Also here's an example of those vendor sysfs entries per device. So
> long as the vendor never expects a tool like libvirt to manipulate
> attributes there, I can see how that could be pretty powerful.
>

Yes, it is good to have vendor specific entries, libvirt might not
report/use it. That would be more useful for system admin to get extra
information manually that libvirt doesn't report.

> Moving down to the mdev_supported_types, I've updated mtty so that it
> actually adjusts available instance, and we can now see a link under
> the devices for mtty2.
>
> Also worth noting is that a link for the device appears
> in /sys/bus/mdev/devices.
>
> BTW, specifying this device for QEMU vfio-pci is where the sysfsdev
> option comes into play:
>
> -device
> vfio-pci,sysfsdev=/sys/devices/mtty/e68189be-700e-41f7-93a3-b5351e79c470
>
> Which raises another question, we can tell through the vfio interfaces
> that this is exposes as a PCI device, by creating a container
> (open(/dev/vfio/vfio)), setting an iommu (ioctl(VFIO_SET_IOMMU)),
> adding the group to the container (ioctl(VFIO_GROUP_SET_CONTAINER)),
> getting the device (ioctl(VFIO_GROUP_GET_DEVICE_FD)), and finally
> getting the device info (ioctl(VFIO_DEVICE_GET_INFO)) and checking the
> flag bit that says the API is PCI. That's a long path to go and has
> stumbling blocks like the type of iommu that's available for the given
> platform. How do we make that manageable?

Do you want device type to be expressed in sysfs? Then that should be
done from vendor driver. vfio_mdev module is now a shim layer, so mdev
core or vfio_mdev module don't know what device type flag vendor driver
had set.

Thanks,
Kirti

> I don't think we want to
> create some artificial relationship that the type of the parent
> necessarily match the type of the child mdev, we've already broken that
> with a simple mdev tty driver.
>
> One more:
>
> # uuidgen > mdev_supported_types/mtty1/create
> # tree /sys/devices/mtty
> /sys/devices/mtty
> |-- a7ae17d1-2de4-44c2-ae58-20ae0a0befe8
> | |-- driver -> ../../../bus/mdev/drivers/vfio_mdev
> | |-- iommu_group -> ../../../kernel/iommu_groups/64
> | |-- mtty1 -> ../mdev_supported_types/mtty1
> | |-- power
> [snip]
> | |-- remove
> | |-- subsystem -> ../../../bus/mdev
> | |-- uevent
> | `-- vendor
> | `-- sample_mdev_dev ("This is MDEV a7ae17d1-2de4-44c2-ae58-20ae0a0befe8")
> |-- e68189be-700e-41f7-93a3-b5351e79c470
> | |-- driver -> ../../../bus/mdev/drivers/vfio_mdev
> | |-- iommu_group -> ../../../kernel/iommu_groups/63
> | |-- mtty2 -> ../mdev_supported_types/mtty2
> | |-- power
> [snip]
> | |-- remove
> | |-- subsystem -> ../../../bus/mdev
> | |-- uevent
> | `-- vendor
> | `-- sample_mdev_dev ("This is MDEV e68189be-700e-41f7-93a3-b5351e79c470")
> |-- mdev_supported_types
> | |-- mtty1
> | | |-- available_instances (21)
> | | |-- create
> | | |-- devices
> | | | `-- a7ae17d1-2de4-44c2-ae58-20ae0a0befe8 -> ../../../a7ae17d1-2de4-44c2-ae58-20ae0a0befe8
> | | `-- name
> | `-- mtty2
> | |-- available_instances (10)
> | |-- create
> | |-- devices
> | | `-- e68189be-700e-41f7-93a3-b5351e79c470 -> ../../../e68189be-700e-41f7-93a3-b5351e79c470
> | `-- name
>
> Hopefully as expected with the caveats for the first example.
>
> # echo 1 > a7ae17d1-2de4-44c2-ae58-20ae0a0befe8/remove
> # echo 1 > e68189be-700e-41f7-93a3-b5351e79c470/remove
>
> These do what they're supposed to, the devices are gone.
>
> Ok, I've identified some issues, let's figure out how to resolve them.
> Thanks,
>
> Alex
>
> (hack multi-port mtty patch attached)
>