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

On Wed, 31 Aug 2016 15:04:13 +0800 Jike Song <jike.song(a)intel.com&gt; wrote: The purpose of the sub-directories is that libvirt doesn't need to pass arbitrary, vendor strings to the create function, the attributes of the mdev device created are defined by the attributes in the sysfs directory where the create is done. The user only provides a uuid for the device. Arbitrary vendor parameters are a barrier, libvirt may not need to know the meaning, but would need to know when to apply them, which is just as bad. Ultimately we want libvirt to be able to interact with sysfs without having an vendor specific knowledge. Possible yes, but why would the vendor driver report types that the user cannot create? It just seems like superfluous information (well, except for the use I discover below). My expectation of your example, where I'm assuming you have 1G of total memory that can be divided between the mdev devices would be: 128M: 8 256M: 4 512M: 2 If a 512M mdev device is created, this becomes: 128M: 4 256M: 2 512M: 1 Creating a 128M mdev device from that becomes: 128M: 3 256M: 1 512M: 0 It's not great, but I don't know how to do it better without the user having a clear understanding of the algorithm and resources required for each mdev device. For instance, the size here, presumably the framebuffer size, is just one attribute in the device directory, the user won't know that this attribute is the key to the available instances. I don't particularly like the idea of a writeable max_instances, the user can simply create instances of the type and see the results. Just thought of another thing; do we need some way to determine the type of an mdev device from sysfs or is this implicit knowledge for the user that created the device? For instance, we create a 512M device and it becomes a child device to the parent, so we can associate to the parent, but if we come back later, how do we know it's a 512M device? Perhaps this is a reason to keep the type directories around and we can cross link the device to the type and create a devices subdirectory under each type. Perhaps then "max_instances" becomes "available_instances" (ie. how many left we can create) and we don't need a "current_instances" because we can simply look in the devices directory. Great! Sure, this is just another way of doing UUID+instance. Nit, it might look more like: /sys/bus/pci/devices/<sbdf>/mdev/ |-- uuid1/ | `-- group_uuid `-- uuid2/ `-- group_uuid Where each mdev device is actually referenced by its UUID name then we'd have some writable attribute under the device where mdev devices sharing the same group UUID are handled together. There's a problem here though that vfio doesn't know about this level of grouping, so uuid1 and uuid2 could actually be given to different users despite the grouping here, which results in one or both devices not working or creating security issues. That sort of implies that this would necessarily need to be exposed as iommu grouping. This factors into why it seems like a good idea to make the start/stop implicit within the interface. In that way each mdev device is fungible as far as a user like libvirt is concerned, internal details like peer-to-peer resources are handled automatically as the devices are accessed. I agree, there's lots of potential and it's extra incentive to create an interface that's going to make sense long term. Ideally we only need to create the kernel and libvirt infrastructure once and we can handle any type of mediated driver. Thanks, Alex