Re: [PATCH v3 1/3] qemu: extract PCI handling structs

Laine Stump <laine@xxxxxxxxx> · Mon, 05 May 2014 17:00:34 +0300

On 05/05/2014 03:07 PM, Ján Tomko
      wrote:

    
    
      On 05/03/2014 06:31 PM, Roman Bogorodskiy wrote:

      
        Extract PCI handling related structs that could be shared
with other drivers.

List of structs moved to virpci.h and new names:

 qemuDomainPCIAddressBus -> virDomainPCIAddressBus
 qemuDomainPCIAddressBusPtr -> virDomainPCIAddressBusPtr
 _qemuDomainPCIAddressSet -> virDomainPCIAddressSet
 qemuDomainPCIAddressSetPtr -> virDomainPCIAddressSetPtr
 qemuDomainPCIConnectFlags -> virDomainPCIConnectFlags

      
      I would drop the 'Domain', to make the prefix match the file.
    
    

    I was thinking about that and came to a different opinion. The
    functions that are currently in virpci.c are dealing with
    manipulating and reporting about PCI devices on the *host* (reading
    and writing sysfs files to attach and detach drivers, determining
    the list of virtual functions for an SRIOV physical function, etc),
    while these functions that Roman is moving are only concerned with
    managing the allocation of PCI addresses to devices in a domain.

    

    Because of that, I think it's reasonable (a good idea really) to
    keep "Domain" in the function names.

    

    Beyond that, I was going to say that I think these functions belong
    in their own file, *not* virpci.c (and maybe we even want to rename
    virpci.c to virhostpci.c or something). I think it's *essential*
    that the two sets of functions are separated from each other, since
    what is in virpci.c is Linux-specific, but the virDomainPCI...
    functions should be host-agnostic.

    

    

    BTW, thanks for caving in to our suggestion so willingly, Roman! :-)

    

    

    
      

      
        Also, extract qemuDomainPCIAddressAsString function and rename
it into virPCIDeviceAddressAsString.

      
      If that was moved into the second patch, this one would touch two less files
(unless I missed something).
    
    

    One thing that's bothered me (and I *almost* did something about it
    the last time I sent patches that touched PCI code) is that there
    are two *almost* identical data structures in the code:

    

    virPCIDeviceAddress  (in virpci.h)

       has domain, bus, slot, function

    virDevicePCIAddress (in device_conf.h)

       has domain, bus, slot, function, _and multifunction_

    

    (there is also _virPCIDevice, but that is only used in virpci.c, and
    has quite a few more attributes in it)

    

    This leads to places where we assign from one to the other by
    individual attributes, rather than just assigning the entire object,
    argument lists that break out the individual attributes instead of
    passing an object, and confusion when you accidentally use the wrong
    one. 

    

    I'm thinking that virDevicePCIAddress should probably contain a
    virPCIDeviceAddress (and maybe have a name that is more different),
    and that virpci.c should have a few functions that format a
    virPCIDeviceAddress into a char* and a virBufferPtr.

    

    But that's not anything that should hold up this series, just
    thought I should mention it because it's in the same area...

    

    
      

      
        ---
 src/Makefile.am          |   2 +-
 src/libvirt_private.syms |   1 +
 src/qemu/qemu_command.c  | 201 +++++++++++++++++++----------------------------
 src/qemu/qemu_command.h  |  40 +++-------
 src/qemu/qemu_domain.h   |   5 +-
 src/util/virpci.c        |  14 ++++
 src/util/virpci.h        |  48 +++++++++++
 tools/Makefile.am        |   1 +
 8 files changed, 158 insertions(+), 154 deletions(-)

      
      
      
        @@ -2134,10 +2093,10 @@ int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr addrs,
      * function is only used for hot-plug, though, and hot-plug is
      * only supported for standard PCI devices, so we can safely use
      * the setting below */
-    qemuDomainPCIConnectFlags flags = (QEMU_PCI_CONNECT_HOTPLUGGABLE |
-                                       QEMU_PCI_CONNECT_TYPE_PCI);
+    virDomainPCIConnectFlags flags = (VIR_PCI_CONNECT_HOTPLUGGABLE |
+                                       VIR_PCI_CONNECT_TYPE_PCI);

      
      Indentation is off here.


      
         
-    if (!(addrStr = qemuDomainPCIAddressAsString(&dev->addr.pci)))
+    if (!(addrStr = virPCIDeviceAddressAsString(&dev->addr.pci)))
         goto cleanup;
 
     if (dev->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {

      
      
      
        diff --git a/src/util/virpci.h b/src/util/virpci.h
index 20ffe54..21fe261 100644

--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -26,6 +26,7 @@
 
 # include "internal.h"
 # include "virobject.h"
+# include "domain_conf.h"

      
      Hmm, this requires the Makefile changes just to get around the cross-inclusion
syntax check rule, that is probably not a good idea.

It's needed for virDevicePCIAddress and virDomainControllerModelPCI.

We have virDevicePCIAddress (defined in device_conf.h) for guest devices and
virPCIDeviceAddress (here in virpci.h) for host devices.

virDomainControllerModelPCI is needed by the PCIAddressBusSetModel set
function, which fills out the flags and {min,max}Slots and also the model,
but I can't find any function using it.

I think the right thing is to drop the include - change the guest devices to
use the address type defined here in virpci.h and stop storing the model with
the address bus.

Jan


      

      
      

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

  

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



