[libvirt] [PATCH 2/2] nodedev: Expose PCI header type

[Martin Kletzander]

On Tue, Mar 15, 2016 at 04:37:55PM +0000, Daniel P. Berrange wrote:
>On Tue, Mar 15, 2016 at 05:23:30PM +0100, Martin Kletzander wrote:
>> If we expose this information, which is one byte in every PCI config
>> file, we let all mgmt apps know whether the device itself is an endpoint
>> or not so it's easier for them to decide whether such device can be
>> passed through into a VM (endpoint) or not (*-bridge).
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1317531
>>
>> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
>> ---
>>  docs/schemas/nodedev.rng                           | 17 ++++++++++
>>  src/conf/node_device_conf.c                        | 37 +++++++++++++++++++++
>>  src/conf/node_device_conf.h                        |  2 ++
>>  src/libvirt_private.syms                           |  3 ++
>>  src/node_device/node_device_udev.c                 |  5 +++
>>  src/util/virpci.c                                  | 38 ++++++++++++++++++++++
>>  src/util/virpci.h                                  | 12 +++++++
>>  .../pci_0000_00_02_0_header_type.xml               | 16 +++++++++
>>  .../pci_0000_00_1c_0_header_type.xml               | 22 +++++++++++++
>>  tests/nodedevxml2xmltest.c                         |  2 ++
>>  10 files changed, 154 insertions(+)
>>  create mode 100644 tests/nodedevschemadata/pci_0000_00_02_0_header_type.xml
>>  create mode 100644 tests/nodedevschemadata/pci_0000_00_1c_0_header_type.xml
>>
>> diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng
>> index 744dccdf5fa9..7aec2adf48e9 100644
>> --- a/docs/schemas/nodedev.rng
>> +++ b/docs/schemas/nodedev.rng
>> @@ -169,6 +169,23 @@
>>      </optional>
>>
>>      <optional>
>> +      <element name='header'>
>> +        <attribute name='type'>
>> +          <choice>
>> +            <value>endpoint</value>
>> +            <value>pci-bridge</value>
>> +            <value>cardbus-bridge</value>
>> +          </choice>
>> +        </attribute>
>
>We already have nested <capability> elements in the node device
>data, and reporting this header type just looks like another
>set of capabilities to me. ie <capability type='endpoint'/>
>

But the devices that this is valid for are exactly only PCI ones.  And
all PCI ones.  That's why I included it in capability type='pci'.  It's
not capability by itself, it's just information extracted from one Byte
in PCI config.

>> +        <optional>
>> +          <element name='multifunction'>
>
>Your commit message mentioned nothing about adding a <multifunction/>
>element. It looks tangential to the goal of adding info about the
>device type, so should be a separate patch.
>

OK, I should've explained it more clearly.  This information is taken
from the first bit of the PCI config header type field.  That's what I
meant in the commit message as exposing all the information we can get
from that one Byte as it's part of that.  That's also why that element
is included *inside* the header element as it's part of that information.

>In any case we already have a
>
>
>    <capability type='virt_functions'>
>      <address domain='0x0000' bus='0x05' slot='0x10' function='0x0'/>
>      <address domain='0x0000' bus='0x05' slot='0x10' function='0x4'/>
>      <address domain='0x0000' bus='0x05' slot='0x11' function='0x0'/>
>      <address domain='0x0000' bus='0x05' slot='0x11' function='0x4'/>
>    </capability>
>
>which is reported against physical functions. What's missing
>is that we omit the capability entirely if no functions are
>currently enabled. We also don't report the total number of
>functions that are possible. IMHO we should address it those
>problems rather than addign a new element.
>

The 'multifunction' element I added has nothing to do with virtual
functions.  It merely indicates that the physical device is supposed to
be multifunction; as in there is another device with the same address,
but different value of 'function' part of that address.

I think explaining all this in the commit message would be enough as I
believe the patch is still correct.  Let me know if that's fine or if I
misunderstood, thanks.

>
>Regards,
>Daniel
>--
>|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
>|: http://libvirt.org              -o-             http://virt-manager.org :|
>|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
>|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160316/7ab32dd6/attachment-0001.sig>