Support for VFIO variant drivers, Part 1

[libvirt PATCH v3 2/8] util: add stub driver name to virPCIDevice object

Posted by Laine Stump 2 years, 5 months ago

There can be many different drivers that are of the type "VFIO", so
add the driver name to the object and allow getting/setting it.

Signed-off-by: Laine Stump <laine@redhat.com>
---
 src/libvirt_private.syms |  2 ++
 src/util/virpci.c        | 16 ++++++++++++++++
 src/util/virpci.h        |  3 +++
 3 files changed, 21 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 983109df86..fad5389d68 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3072,6 +3072,7 @@ virPCIDeviceGetManaged;
 virPCIDeviceGetName;
 virPCIDeviceGetRemoveSlot;
 virPCIDeviceGetReprobe;
+virPCIDeviceGetStubDriverName;
 virPCIDeviceGetStubDriverType;
 virPCIDeviceGetUnbindFromStub;
 virPCIDeviceGetUsedBy;
@@ -3098,6 +3099,7 @@ virPCIDeviceReset;
 virPCIDeviceSetManaged;
 virPCIDeviceSetRemoveSlot;
 virPCIDeviceSetReprobe;
+virPCIDeviceSetStubDriverName;
 virPCIDeviceSetStubDriverType;
 virPCIDeviceSetUnbindFromStub;
 virPCIDeviceSetUsedBy;
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 88a020fb86..103bc4254e 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -88,6 +88,7 @@ struct _virPCIDevice {
     bool          managed;
 
     virPCIStubDriver stubDriverType;
+    char            *stubDriverName; /* if blank, use default for type */
 
     /* used by reattach function */
     bool          unbind_from_stub;
@@ -1507,6 +1508,7 @@ virPCIDeviceCopy(virPCIDevice *dev)
     copy->path = g_strdup(dev->path);
     copy->used_by_drvname = g_strdup(dev->used_by_drvname);
     copy->used_by_domname = g_strdup(dev->used_by_domname);
+    copy->stubDriverName = g_strdup(dev->stubDriverName);
     return copy;
 }
 
@@ -1521,6 +1523,7 @@ virPCIDeviceFree(virPCIDevice *dev)
     g_free(dev->path);
     g_free(dev->used_by_drvname);
     g_free(dev->used_by_domname);
+    g_free(dev->stubDriverName);
     g_free(dev);
 }
 
@@ -1580,6 +1583,19 @@ virPCIDeviceGetStubDriverType(virPCIDevice *dev)
     return dev->stubDriverType;
 }
 
+void
+virPCIDeviceSetStubDriverName(virPCIDevice *dev,
+                                   const char *driverName)
+{
+    dev->stubDriverName = g_strdup(driverName);
+}
+
+const char *
+virPCIDeviceGetStubDriverName(virPCIDevice *dev)
+{
+    return dev->stubDriverName;
+}
+
 bool
 virPCIDeviceGetUnbindFromStub(virPCIDevice *dev)
 {
diff --git a/src/util/virpci.h b/src/util/virpci.h
index 485f535bc9..f8f98f39de 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -137,6 +137,9 @@ bool virPCIDeviceGetManaged(virPCIDevice *dev);
 void virPCIDeviceSetStubDriverType(virPCIDevice *dev,
                                    virPCIStubDriver driverType);
 virPCIStubDriver virPCIDeviceGetStubDriverType(virPCIDevice *dev);
+void virPCIDeviceSetStubDriverName(virPCIDevice *dev,
+                                   const char *driverName);
+const char *virPCIDeviceGetStubDriverName(virPCIDevice *dev);
 virPCIDeviceAddress *virPCIDeviceGetAddress(virPCIDevice *dev);
 int virPCIDeviceSetUsedBy(virPCIDevice *dev,
                           const char *drv_name,
-- 
2.41.0

Re: [libvirt PATCH v3 2/8] util: add stub driver name to virPCIDevice object

Posted by Michal Prívozník 2 years, 5 months ago

On 8/21/23 21:32, Laine Stump wrote:
> There can be many different drivers that are of the type "VFIO", so
> add the driver name to the object and allow getting/setting it.
> 
> Signed-off-by: Laine Stump <laine@redhat.com>
> ---
>  src/libvirt_private.syms |  2 ++
>  src/util/virpci.c        | 16 ++++++++++++++++
>  src/util/virpci.h        |  3 +++
>  3 files changed, 21 insertions(+)
> 

> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 88a020fb86..103bc4254e 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c

> @@ -1580,6 +1583,19 @@ virPCIDeviceGetStubDriverType(virPCIDevice *dev)
>      return dev->stubDriverType;
>  }
>  
> +void
> +virPCIDeviceSetStubDriverName(virPCIDevice *dev,
> +                                   const char *driverName)
> +{
> +    dev->stubDriverName = g_strdup(driverName);
> +}


I think it's worth freeing dev->stubDriverName before setting another
one. Please consider squashing this in:

diff --git i/src/util/virpci.c w/src/util/virpci.c
index 3f207a24f3..15e53e6749 100644
--- i/src/util/virpci.c
+++ w/src/util/virpci.c
@@ -1586,8 +1586,9 @@ virPCIDeviceGetStubDriverType(virPCIDevice *dev)

 void
 virPCIDeviceSetStubDriverName(virPCIDevice *dev,
-                                   const char *driverName)
+                              const char *driverName)
 {
+    g_free(dev->stubDriverName);
     dev->stubDriverName = g_strdup(driverName);
 }


And just a thought (maybe it was covered in earlier discussions, maybe
it'll be covered in next patches) - what about the following scenario:

virPCIDeviceSetStubDriverType(&dev, VIR_PCI_STUB_DRIVER_VFIO);
virPCIDeviceSetStubDriverName(&dev, "blah");

Should the latter reset dev->stubDriverType to _NONE? Similarly, what if
the two are reversed? But I guess we can always fine tune this later.

Michal

Re: [libvirt PATCH v3 2/8] util: add stub driver name to virPCIDevice object

Posted by Laine Stump 2 years, 5 months ago

On 8/23/23 3:52 AM, Michal Prívozník wrote:
> On 8/21/23 21:32, Laine Stump wrote:
>> There can be many different drivers that are of the type "VFIO", so
>> add the driver name to the object and allow getting/setting it.
>>
>> Signed-off-by: Laine Stump <laine@redhat.com>
>> ---
>>   src/libvirt_private.syms |  2 ++
>>   src/util/virpci.c        | 16 ++++++++++++++++
>>   src/util/virpci.h        |  3 +++
>>   3 files changed, 21 insertions(+)
>>
> 
>> diff --git a/src/util/virpci.c b/src/util/virpci.c
>> index 88a020fb86..103bc4254e 100644
>> --- a/src/util/virpci.c
>> +++ b/src/util/virpci.c
> 
>> @@ -1580,6 +1583,19 @@ virPCIDeviceGetStubDriverType(virPCIDevice *dev)
>>       return dev->stubDriverType;
>>   }
>>   
>> +void
>> +virPCIDeviceSetStubDriverName(virPCIDevice *dev,
>> +                                   const char *driverName)
>> +{
>> +    dev->stubDriverName = g_strdup(driverName);
>> +}
> 
> 
> I think it's worth freeing dev->stubDriverName before setting another
> one. Please consider squashing this in:
> 
> diff --git i/src/util/virpci.c w/src/util/virpci.c
> index 3f207a24f3..15e53e6749 100644
> --- i/src/util/virpci.c
> +++ w/src/util/virpci.c
> @@ -1586,8 +1586,9 @@ virPCIDeviceGetStubDriverType(virPCIDevice *dev)
> 
>   void
>   virPCIDeviceSetStubDriverName(virPCIDevice *dev,
> -                                   const char *driverName)
> +                              const char *driverName)
>   {
> +    g_free(dev->stubDriverName);
>       dev->stubDriverName = g_strdup(driverName);
>   }

Sure, that seems prudent.

> 
> 
> And just a thought (maybe it was covered in earlier discussions, maybe
> it'll be covered in next patches) - what about the following scenario:
> 
> virPCIDeviceSetStubDriverType(&dev, VIR_PCI_STUB_DRIVER_VFIO);
> virPCIDeviceSetStubDriverName(&dev, "blah");
> 
> Should the latter reset dev->stubDriverType to _NONE? Similarly, what if
> the two are reversed? But I guess we can always fine tune this later.

Although the two settings are conceptually intertwined, I think the APIs 
should be orthogonal, with the setting of one not affecting the other; 
otherwise people would get confused about which one should come first 
and/or be surprised when it didn't do what they wanted.