[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] expose SR IOV physical/virtual function relationships



Daniel P. Berrange wrote:
On Tue, Dec 01, 2009 at 12:03:49PM -0500, Dave Allan wrote:
Daniel P. Berrange wrote:
On Tue, Dec 01, 2009 at 10:33:08AM -0500, Dave Allan wrote:
Attached is a patch that exposes the relationships between physical and virtual functions on SR IOV capable devices.


+            if (data->pci_dev.physical_function) {
+ virBufferEscapeString(&buf, " <physical_function>%s</physical_function>\n",
+                                      data->pci_dev.physical_function);
+            }
+            if (data->pci_dev.num_virtual_functions > 0) {
+ for (i = 0 ; i < data->pci_dev.num_virtual_functions ; i++) { + virBufferEscapeString(&buf, " <virtual_function>%s</virtual_function>\n", + data->pci_dev.virtual_functions[i]);
+                }
+            }

What is the actual content of those two new elements ?  Are they
the names of the corresponding device, suitble for a virNodeDevLooupByName() ?
They are the PCI device BDF or config address, e.g.:

    <virtual_function>0000:01:10.0</virtual_function>

so, no, they aren't suitable for lookup by name. Using the names of the corresponding devices was my first attempt, but there is a dependency problem there. At the time that we process one device, the others aren't necessarily created in libvirt, so it's not possible to look them up. If we wanted to go that route, we'd have to do additional work at the time of dumping the xml, which I think is a little kludgy. I'd rather we created an API to lookup a libvirt device by its BDF.

I don't much like including the raw BDF format, because it is effectively
adding a 3rd way of specifying PCI addresses in libvirt XML. We already
have 2 different ways, which is 1 too many

Node dev XML

  <capability type='pci'>
    <domain>0</domain>
    <bus>0</bus>
    <slot>31</slot>
    <function>1</function>
  </capability>


Domain XML for host devices & guest addresses

  <address domain='0x0000' bus='0x00' slot='0x1f' function='0x5'/>

Yes, my bad for screwing up the nodedev  XML format when I designed
it, stupidly choosing decimal instead of hex :-(

I think we should make the virtual/physical function follow the domain
XML style, with an <address/> element. Perhaps also use nested capabilities
in the way we did for NPIV host/vport stuff

  <capability type='physical_function'>
    <address domain='0x0000' bus='0x00' slot='0x1f' function='0x1'/>
  </capability>

  <capability type='virtual_functions'>
    <address domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/>
    <address domain='0x0000' bus='0x00' slot='0x1f' function='0x3'/>
    <address domain='0x0000' bus='0x00' slot='0x1f' function='0x4'/>
  </capability>


I'd even be inclined to retro-fit the existing <capability type='pci'>
XML to duplicate the address info in this saner format eg

  <capability type='pci'>
    <address domain='0x0000' bus='0x00' slot='0x1f' function='0x1' />
    <domain>0</domain>
    <bus>0</bus>
    <slot>31</slot>
    <function>1</function>
  </capability>


So, a PCI card with SR-IOV would end up looking like

  <capability type='pci'>
    <domain>0</domain>
    <bus>0</bus>
    <slot>31</slot>
    <function>1</function>
    <address domain='0x0000' bus='0x00' slot='0x1f' function='0x1' />

    <capability type='virtual_functions'>
      <address domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/>
      <address domain='0x0000' bus='0x00' slot='0x1f' function='0x3'/>
      <address domain='0x0000' bus='0x00' slot='0x1f' function='0x4'/>
    </capability>
  </capability>

The nice thing about this kind of nesting is that it would let us show
that a card is capable of exposing virtual functions, even if it does
not currently have any enabled

That makes for a bit more work, but I agree it's a good way to do it. Updated patch forthcoming.

Dave




Regards,
Daniel


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]