Re: [libvirt] [Qemu-devel] [PATCH v7 0/4] Add Mediated device support

On Wed, 7 Sep 2016 01:05:11 +0530 Kirti Wankhede <kwankhede(a)nvidia.com&gt; wrote: What constraints does a group, which is not an iommu group, place on the usage of the mdev devices? What happens if we put two mdev devices in the same "mdev group" and then assign them to separate VMs/users? I believe that the answer is that this theoretical "mdev group" doesn't actually impose any constraints on the devices within the group or how they're used. vfio knows about iommu groups and we consider an iommu group to be the unit of ownership for userspace. Therefore by placing multiple mdev devices within the same iommu group we can be assured that there's only one user for that group. Furthermore, the specific case for this association on NVIDIA is to couple the hardware peer-to-peer resources for the individual mdev devices. Therefore this particular grouping does imply a lack of isolation between those mdev devices involved in the group. For mdev devices which are actually isolated from one another, where they don't poke these p2p holes, placing them in the same iommu group is definitely an abuse of the interface and is going to lead to problems with a single iommu context. But how does libvirt know that one type of mdev device needs to be grouped while another type doesn't? There's really not much that I like about using iommu groups in this way, it's just that they seem to solve this particular problem of enforcing how such a group can be used and imposing a second form of grouping onto the vfio infrastructure seems much too complex. In previous replies I had proposed that a group could be an additional argument when we write the mdev UUID to the create entry in sysfs. This is specifically why I listed only the UUID when creating the first mdev device and UUID:group when creating the second. The user would need to go determine the group ID allocated for the first entry to specify creating the second within that same group. I have no love for this proposal, it's functional but not elegant and again leaves libvirt lost in trying to determine which devices need to be grouped together and which have no business being grouped together. Let's think through this further and let me make a couple assumptions to get started: 1) iommu groups are the way that we want to group NVIDIA vGPUs because: a) The peer-to-peer resources represent an isolation gap between mdev devices, iommu groups represent sets of isolated devices. b) The 1:1 mapping of an iommu group to a user matches the NVIDIA device model. c) iommu_group_for_each_dev() gives the vendor driver the functionality it needs to perform a first-open/last-close device walk for configuring these p2p resources. 2) iommu groups as used by mdev devices should contain the minimum number of devices in order to provide the maximum iommu context flexibility. Do we agree on these? The corollary is that NVIDIA is going to suffer reduced iommu granularity exactly because of the requirement to setup p2p resources between mdev devices within the same VM. This has implications when guest iommus are in play (viommu). So by default we want an iommu group per mdev. This works for all mdev devices as far as we know, including NVIDIA with the constraint that we only have a single NVIDIA device per VM. What if we want multiple NVIDIA devices? We either need to create the additional devices with a property which will place them into the same iommu group or allow the iommu groups to be manipulated dynamically. The trouble I see with the former (creating a device into a group) is that it becomes part of the "create" syntax, which is global for all mdev devices. It's the same functional, but non-elegant solution I proposed previously. What if we allow groups to be manipulated dynamically? In this case I envision an attribute under the mdev device with read/write access. The existence of the attribute indicates to libvirt that this device requires such handling and allows reading and setting the association. To be clear, the attribute would only exist on mdev devices requiring this handling. I'm always a fan of naming things after what they do, so rather than making this attribute reference an iommu group, I might actually call it "peer_to_peer_resource_uuid". So the process might look something like this: # create 2 mdev devices echo $UUID0 > /sys/devices/mdev/<s:b:d.f>/types/1/create echo $UUID1 > /sys/devices/mdev/<s:b:d.f>/types/1/create # move $UUID1 to the same group as $UUID0 P2P_UUID=$(cat /sys/devices/mdev/<s:b:d.f>/types/1/devices/$UUID0/peer_to_peer_resource_uuid) echo $P2P_UUID > \ /sys/devices/mdev/<s:b:d.f>/types/1/devices/$UUID1/peer_to_peer_resource_uuid Alternatively we could have used uuidgen to create a UUID then moved both to the new UUID. Within the mdev vendor driver this would walk through the mdev devices, find the matching peer_to_peer_resource_uuid (generated randomly at create time by default) and add the device to the iommu group for devices sharing that p2p uuid. When removed from the VM, libvirt could simply echo the output of uuidgen to each to split them again. So from a libvirt perspective, special handling would need to invoked that when this p2p attribute is found, all devices for a given VM would need to share the same p2p uuid. libvirt would be free to use an existing p2p uuid or generate a new one. The vendor driver should enforce a write failure if the device cannot be added to the p2p uuid (for example devices within the p2p uuid are already opened). Maybe this is similar to your proposal and even goes back to vm_uuid, but under the covers the vendor driver needs to be manipulating iommu grouping based on this parameter and there's no concept of an "mdev group" in the base API (nor vm_uuid), this is an extension keyed by the additional sysfs attribute. Are we getting closer? Thanks, Alex