On Tue, Apr 18, 2017 at 09:52:38AM +0100, Daniel P. Berrange wrote: > On Tue, Apr 18, 2017 at 10:49:40AM +0200, Martin Polednik wrote: > > On 12/04/17 16:19 -0600, Alex Williamson wrote: > > > On Wed, 5 Apr 2017 09:21:17 +0100 > > > "Daniel P. Berrange" <berrange@xxxxxxxxxx> wrote: > > > > > > > On Wed, Apr 05, 2017 at 08:12:31AM +0200, Erik Skultety wrote: > > > > > On Tue, Apr 04, 2017 at 04:23:18PM +0100, Daniel P. Berrange wrote: > > > > > > On Wed, Mar 29, 2017 at 02:51:10PM +0200, Erik Skultety wrote: > > > > > > > This series enables the node device driver to report information about the > > > > > > > existing mediated devices on the host. There is no device creation involved > > > > > > > yet. The information reported by the node device driver is split into two > > > > > > > parts, one that is reported within the physical parent's capabilities > > > > > > > (the generic stuff that comes from the mdev types' sysfs attributes, note the > > > > > > > 'description' attribute which is verbatim - raw,unstructured string) and the > > > > > > > other that is reported within the mdev child device and merely contains the > > > > > > > mdev type id, which the device was instantiated from, and the iommu group > > > > > > > number. > > > > > > > > > > > > > > Basically, the format of the XML I went for is as follows: > > > > > > > > > > > > > > PCI parent: > > > > > > > <device> > > > > > > > <name>pci_0000_06_00_0</name> > > > > > > > <path>/sys/devices/.../0000:06:00.0</path> > > > > > > > <parent>pci_0000_05_08_0</parent> > > > > > > > ... > > > > > > > <capability type='pci'> > > > > > > > ... > > > > > > > <capability type='mdev'> > > > > > > > <type id='nvidia-11'> > > > > > > > <name>GRID M60-0B</name> > > > > > > > <description>num_heads=2, frl_config=45, framebuffer=512M, max_resolution=2560x1600, max_instance=16</description> > > > > > > > > > > > > This 'description' field is pretty horrific. > > > > > > > > > > > > We were quite clear that we were not going to expose arbitrary attributes > > > > > > in the XML without modelling them explicitly as XML elements. Using the > > > > > > description field in this way is just doing arbitrary attribute passthrough > > > > > > via the backdoor - it is clear that applications are doing to end up parsing > > > > > > this 'description' data and will them complain if we later change it. > > > > > > > > > > > > > > > > I remember us stating that, but this is the case with NVIDIA (who at least > > > > > reports some useful information in it) and Intel - what if other vendor comes > > > > > and creates a valid, human readable description of a device without specifying > > > > > any attributes like in the case above? Just because some vendors "abused" the > > > > > attribute doesn't mean we should stop reporting it completely. IMHO the name > > > > > "description" should mean that it's something raw, possibly unstructured, and > > > > > thus the applications should treat it that way. Now, this is my bad and I need > > > > > to revisit the last patch with documentation and add a paragraph for the > > > > > <description> element as an optional element with raw data. > > > > > > > > > > Until all the attributes are properly unified/standardized under sysfs ABI and > > > > > respected by vendors, I think we should report everything we're able to gather > > > > > about a device, description included. If properly documented, nobody can > > > > > complain about us breaking anything, since how do you guarantee format-less raw > > > > > string anyway? After all, applications will end up parsing it anyway, be it from > > > > > our XML or not. > > > > > > > > I understand your point, but I'm still not in favour of exposing this info > > > > because it is a clear cut attempt to do arbitrary attribute passthrough to > > > > libvirt. > > > > > > > > All the attributes show there can be determined by a simple lookup based on > > > > the name field "GRID M60-0B". So if apps want to know the number of heads, > > > > framebuffer size, etc, etc I think they should just maintain a lookup map > > > > based on name in their own code, until we explicitly model this stuff in > > > > the XML. > > > > > > > > Once we model the attributes in the XML, this description adds no useful > > > > info that we wouldn't already be reported, and before we model it in the > > > > XML, this is just clearly an abuse of our design statement that we are not > > > > doing generic attribute passthrough. > > > > > > I told Erik I'd jump in here, but I think I might agree with Dan. On > > > the kernel side, the description attribute was an attempt to pull > > > ourselves out of a rat hole of trying to figure out how to classify > > > devices and then figure out what attributes for each class might be > > > common across vendors. Clearly it'd be great to somehow know that a > > > device is a GPU and has a resolution and graphics memory attribute, but > > > getting there is hard. A free-form description field is sort of a > > > catch-all where the vendor can provide a stop-gap and it's useful for > > > human consumption. It would be inadvisable to parse it with machine > > > code though. Including it in the XML sort of invites that possibility. > > > > > > A given mdev type is supposed to be stable. It should always point to > > > a software equivalent device, including capabilities and resources. It > > > should therefore be valid for upper level software to use the type to > > > lookup from a human generated table the properties for that device. Of > > > > From an upper level software's perspective, this is super inefficient > > unless database like pci IDs exist. Each management software > > maintaining their possibly outdated lookup table may lead to > > inconsistence in the user experience and data. I agree, but the same stays true for any software in the application stack just as Dan pointed that out below. Erik > > > > If such database existed, there is no reason for libvirt not to use it > > and report the data in some sane format. > > IMHO it would be a waste of time for libvirt to do this. Effort would be > better spent implementing the solution we requested right at the start, > which is for the kernel to export the information in a stable format via > dedicated sysfs attributes. Any userspace DB in applications is just a > short term lack to workaround this missing kernel feature. > > > Regards, > Daniel > -- > |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| > |: http://libvirt.org -o- http://virt-manager.org :| > |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
