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>
        <device_api>vfio-pci</device_api>
        <available_instances>16</available_instances>
      </type>
        ...
    </capability>
    <iommuGroup number='20'>
      <address domain='0x0000' bus='0x06' slot='0x00' function='0x0'/>
    </iommuGroup>
    ...
  </capability>
</device>

mdev child:
<device>
  <name>mdev_ef1212ff_ff23_4534_ffcd_01ff12017801</name>
  <path>/sys/devices/.../ef1212ff-ff23-4534-ffcd-01ff12017801</path>
  <parent>pci_0000_06_00_0</parent>
  <driver>
    <name>vfio_mdev</name>
  </driver>
  <capability type='mdev'>
    <type id='nvidia-18'/>
    <iommuGroup number='63'/>
  </capability>
</device>

Also, since we didn't have any node device driver documentation, I created a
stub (comments are very welcome, you can shred it to pieces ;)) focusing on the
PCI devices and then adding the mdev part into that. As I said, it's still a
stub that needs lots of work on it, namely adding USBs and SCSI devices, but I
figured that the fact some parts are missing should not be a show stopper for
the nodedev-mdev patches.

Thanks,
Erik

Erik Skultety (11):
  nodedev: Fix guideline violations in nodedev modules
  nodedev: Make use of the compile-time missing enum in switch error
  conf: nodedev: Split virNodeDeviceDefFormat into more functions
  nodedev: udevProcessPCI: Drop syspath variable
  docs: Use our XSLT template to generate list of supported pool types
  nodedev: Introduce the mdev capability to the nodedev driver structure
  nodedev: Fill in the mdev info for the parent PCI device
  nodedev: Introduce udevProcessMediatedDevice
  nodedev: Format the mdev capability of the PCI parent device
  docs: Provide a nodedev driver stub documentation
  docs: Document the mediated devices within the nodedev driver

 docs/drivers.html.in                      |   6 +-
 docs/drvnodedev.html.in                   | 275 +++++++++++++
 docs/storage.html.in                      |  62 +--
 include/libvirt/libvirt-nodedev.h         |   1 +
 src/conf/node_device_conf.c               | 625 ++++++++++++++++++------------
 src/conf/node_device_conf.h               |  21 +-
 src/conf/virnodedeviceobj.c               |   3 +-
 src/libvirt-nodedev.c                     |   1 +
 src/libvirt_private.syms                  |   1 +
 src/node_device/node_device_driver.c      |  30 +-
 src/node_device/node_device_driver.h      |  82 ++--
 src/node_device/node_device_hal.c         |   9 +
 src/node_device/node_device_linux_sysfs.c |   1 +
 src/node_device/node_device_linux_sysfs.h |   9 +-
 src/node_device/node_device_udev.c        | 456 ++++++++++++++++------
 tools/virsh-nodedev.c                     |   2 +
 16 files changed, 1110 insertions(+), 474 deletions(-)
 create mode 100644 docs/drvnodedev.html.in

--
2.12.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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.

So, NACK to including a description element with this kind of content.

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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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.

Erik

> So, NACK to including a description element with this kind of content.
>
> 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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.

> > So, NACK to including a description element with this kind of content.

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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, 5 Apr 2017 09:21:17 +0100
"Daniel P. Berrange" <berrange@redhat.com> 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
course there's about zero chance that a type will be perfectly stable,
but hopefully it's a rare event and they stabilize pretty quickly.  It'd
be nice to one day revisit that classification and per class attributes
problem in the kernel, but maybe we can let userspace figure out a
common, useful set of attribute per class first.  Thanks,

Alex

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 12/04/17 16:19 -0600, Alex Williamson wrote:
>On Wed, 5 Apr 2017 09:21:17 +0100
>"Daniel P. Berrange" <berrange@redhat.com> 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.

If such database existed, there is no reason for libvirt not to use it
and report the data in some sane format.

>course there's about zero chance that a type will be perfectly stable,
>but hopefully it's a rare event and they stabilize pretty quickly.  It'd
>be nice to one day revisit that classification and per class attributes
>problem in the kernel, but maybe we can let userspace figure out a
>common, useful set of attribute per class first.  Thanks,
>
>Alex
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@redhat.com> 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.
> 
> 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@redhat.com> 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, 18 Apr 2017 09:52:38 +0100
"Daniel P. Berrange" <berrange@redhat.com> 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@redhat.com> 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.
> > 
> > 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.

...and a pony too!  Seriously, "requesting" this feature isn't
necessarily going to make it happen.  How do we classify devices?  How
do we define attributes per class that multiple vendors can agree to
and are extensible to future vendors?  This seems exactly like the sort
of thing that needs to be flushed out in userspace before committing to
poorly defined ABIs in the kernel.  Thanks,

Alex

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list