vfio: some trivial fixes

PATCH In review Li Qiang 3 participants 13 messages Oct 19, 2018 – Oct 22, 2018

lore

Share & tools

lore

Li Qiang

author

2018-10-19 05:20 UTC #0 lore raw

cover vfio: some trivial fixes

This patch set contains some trivial issue such as QOMConvetion, typo and resources leak in vfio.

Li Qiang (7):
vfio-pci: make "vfio-pci-nohotplug" as MACRO
vfio: ap-device: make it more QOMConventional
vfio: drop TYPE_FOO MACRO in VMStateDescription
vfio: paltform: fix a typo
vfio: platform: cleanup the notifier in error path
vfio: platform: free timer in error path
vfio: platform: destory mutex in error path

 hw/s390x/ap-device.c         |  2 +-
 hw/vfio/amd-xgbe.c           |  2 +-
 hw/vfio/ap.c                 | 12 ++++++------
 hw/vfio/calxeda-xgmac.c      |  2 +-
 hw/vfio/ccw.c                |  2 +-
 hw/vfio/pci.c                |  6 ++++--
 hw/vfio/platform.c           | 12 ++++++++----
 include/hw/s390x/ap-device.h |  4 ++--
 8 files changed, 24 insertions(+), 18 deletions(-)

Signature · 1 line

1.8.3.1

Li Qiang

author

2018-10-19 05:20 UTC #1 lore raw

PATCH 1/7 vfio-pci: make "vfio-pci-nohotplug" as MACRO

Signed-off-by Li Qiang <[email protected]>

---
 hw/vfio/pci.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

hw/vfio/pci.c +4 -2

index 8b73582..1f05b57 100644
@@ -40,6 +40,8 @@
 #define TYPE_VFIO_PCI "vfio-pci"
 #define PCI_VFIO(obj)    OBJECT_CHECK(VFIOPCIDevice, obj, TYPE_VFIO_PCI)
 
+#define TYPE_VFIO_PCI_NOHOTPLUG "vfio-pci-nohotplug"
+
 static void vfio_disable_interrupts(VFIOPCIDevice *vdev);
 static void vfio_mmap_set_enabled(VFIOPCIDevice *vdev, bool enabled);

@@ -3277,8 +3279,8 @@ static void vfio_pci_nohotplug_dev_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo vfio_pci_nohotplug_dev_info = {
-    .name = "vfio-pci-nohotplug",
-    .parent = "vfio-pci",
+    .name = TYPE_VFIO_PCI_NOHOTPLUG,
+    .parent = TYPE_VFIO_PCI,
     .instance_size = sizeof(VFIOPCIDevice),
     .class_init = vfio_pci_nohotplug_dev_class_init,
 };

Signature · 1 line

1.8.3.1

Li Qiang

author

2018-10-19 05:20 UTC #2 lore raw

PATCH 2/7 vfio: ap-device: make it more QOMConventional

In reply to Li Qiang

As the documentation says "use TYPE_FOO constants" This also changes the parent of ap-device's MACRO.

Signed-off-by Li Qiang <[email protected]>

---
 hw/s390x/ap-device.c         |  2 +-
 hw/vfio/ap.c                 | 12 ++++++------
 include/hw/s390x/ap-device.h |  4 ++--
 3 files changed, 9 insertions(+), 9 deletions(-)

hw/s390x/ap-device.c +1 -1

index f5ac8db..64a5f11 100644
@@ -22,7 +22,7 @@ static void ap_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo ap_device_info = {
-    .name = AP_DEVICE_TYPE,
+    .name = TYPE_AP_DEVICE,
     .parent = TYPE_DEVICE,
     .instance_size = sizeof(APDevice),
     .class_size = sizeof(DeviceClass),

hw/vfio/ap.c +6 -6

index 3962bb7..b332395 100644
@@ -28,7 +28,7 @@
 #include "hw/s390x/ap-bridge.h"
 #include "exec/address-spaces.h"
 
-#define VFIO_AP_DEVICE_TYPE      "vfio-ap"
+#define TYPE_VFIO_AP_DEVICE      "vfio-ap"
 
 typedef struct VFIOAPDevice {
     APDevice apdev;

@@ -36,7 +36,7 @@ typedef struct VFIOAPDevice {
 } VFIOAPDevice;
 
 #define VFIO_AP_DEVICE(obj) \
-        OBJECT_CHECK(VFIOAPDevice, (obj), VFIO_AP_DEVICE_TYPE)
+        OBJECT_CHECK(VFIOAPDevice, (obj), TYPE_VFIO_AP_DEVICE)
 
 static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
 {

@@ -69,7 +69,7 @@ static VFIOGroup *vfio_ap_get_group(VFIOAPDevice *vapdev, Error **errp)
 
     if (!group_path) {
         error_setg(errp, "%s: no iommu_group found for %s: %s",
-                   VFIO_AP_DEVICE_TYPE, vapdev->vdev.sysfsdev, gerror->message);
+                   TYPE_VFIO_AP_DEVICE, vapdev->vdev.sysfsdev, gerror->message);
         return NULL;
     }

@@ -147,7 +147,7 @@ static void vfio_ap_reset(DeviceState *dev)
 }
 
 static const VMStateDescription vfio_ap_vmstate = {
-    .name = VFIO_AP_DEVICE_TYPE,
+    .name = TYPE_VFIO_AP_DEVICE,
     .unmigratable = 1,
 };

@@ -167,8 +167,8 @@ static void vfio_ap_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo vfio_ap_info = {
-    .name = VFIO_AP_DEVICE_TYPE,
-    .parent = AP_DEVICE_TYPE,
+    .name = TYPE_VFIO_AP_DEVICE,
+    .parent = TYPE_AP_DEVICE,
     .instance_size = sizeof(VFIOAPDevice),
     .class_init = vfio_ap_class_init,
 };

include/hw/s390x/ap-device.h +2 -2

index 765e908..d7eced4 100644
@@ -10,13 +10,13 @@
 #ifndef HW_S390X_AP_DEVICE_H
 #define HW_S390X_AP_DEVICE_H
 
-#define AP_DEVICE_TYPE       "ap-device"
+#define TYPE_AP_DEVICE      "ap-device"
 
 typedef struct APDevice {
     DeviceState parent_obj;
 } APDevice;
 
 #define AP_DEVICE(obj) \
-    OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE)
+    OBJECT_CHECK(APDevice, (obj), TYPE_AP_DEVICE)
 
 #endif /* HW_S390X_AP_DEVICE_H */

Signature · 1 line

1.8.3.1

Li Qiang

author

2018-10-19 05:20 UTC #3 lore raw

PATCH 3/7 vfio: drop TYPE_FOO MACRO in VMStateDescription

In reply to Li Qiang

As the vmstate structure names aren't related with
the QOM type names.
Per Peter's mail:
-->https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg02175.html

Signed-off-by Li Qiang <[email protected]>

---
 hw/vfio/amd-xgbe.c      | 2 +-
 hw/vfio/ap.c            | 2 +-
 hw/vfio/calxeda-xgmac.c | 2 +-
 hw/vfio/ccw.c           | 2 +-
 hw/vfio/platform.c      | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

hw/vfio/amd-xgbe.c +1 -1

index ee64a3b..1b06c0f 100644
@@ -26,7 +26,7 @@ static void amd_xgbe_realize(DeviceState *dev, Error **errp)
 }
 
 static const VMStateDescription vfio_platform_amd_xgbe_vmstate = {
-    .name = TYPE_VFIO_AMD_XGBE,
+    .name = "vfio-amd-xgbe",
     .unmigratable = 1,
 };

hw/vfio/ap.c +1 -1

index b332395..35b01b3 100644
@@ -147,7 +147,7 @@ static void vfio_ap_reset(DeviceState *dev)
 }
 
 static const VMStateDescription vfio_ap_vmstate = {
-    .name = TYPE_VFIO_AP_DEVICE,
+    .name = "vfio-ap",
     .unmigratable = 1,
 };

hw/vfio/calxeda-xgmac.c +1 -1

index e7767c4..6cc608b 100644
@@ -26,7 +26,7 @@ static void calxeda_xgmac_realize(DeviceState *dev, Error **errp)
 }
 
 static const VMStateDescription vfio_platform_calxeda_xgmac_vmstate = {
-    .name = TYPE_VFIO_CALXEDA_XGMAC,
+    .name = "vfio-calxeda-xgmac",
     .unmigratable = 1,
 };

hw/vfio/ccw.c +1 -1

index 9246729..4786d46 100644
@@ -466,7 +466,7 @@ static Property vfio_ccw_properties[] = {
 };
 
 static const VMStateDescription vfio_ccw_vmstate = {
-    .name = TYPE_VFIO_CCW,
+    .name = "vfio-ccw",
     .unmigratable = 1,
 };

hw/vfio/platform.c +1 -1

index 64c1af6..ba03dcd 100644
@@ -697,7 +697,7 @@ out:
 }
 
 static const VMStateDescription vfio_platform_vmstate = {
-    .name = TYPE_VFIO_PLATFORM,
+    .name = "vfio-platform",
     .unmigratable = 1,
 };

Signature · 1 line

1.8.3.1

Li Qiang

author

2018-10-19 05:20 UTC #4 lore raw

PATCH 4/7 vfio: paltform: fix a typo

In reply to Li Qiang

Signed-off-by Li Qiang <[email protected]>

---
 hw/vfio/platform.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

hw/vfio/platform.c +2 -2

index ba03dcd..5992fe7 100644
@@ -72,7 +72,7 @@ static VFIOINTp *vfio_init_intp(VFIODevice *vbasedev,
         g_free(intp->interrupt);
         g_free(intp);
         error_setg_errno(errp, -ret,
-                         "failed to initialize trigger eventd notifier");
+                         "failed to initialize trigger eventfd notifier");
         return NULL;
     }
     if (vfio_irq_is_automasked(intp)) {

@@ -84,7 +84,7 @@ static VFIOINTp *vfio_init_intp(VFIODevice *vbasedev,
             g_free(intp->unmask);
             g_free(intp);
             error_setg_errno(errp, -ret,
-                             "failed to initialize resample eventd notifier");
+                             "failed to initialize resample eventfd notifier");
             return NULL;
         }
     }

Signature · 1 line

1.8.3.1

Philippe Mathieu-Daudé

2018-10-19 05:23 UTC #5 lore raw

Re: [Qemu-devel] [PATCH 4/7] vfio: paltform: fix a typo

Quoted 20 lines · Li Qiang

Signed-off-by Li Qiang <[email protected]>

---
 hw/vfio/platform.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

hw/vfio/platform.c +1 -1

index ba03dcd..5992fe7 100644
@@ -72,7 +72,7 @@ static VFIOINTp *vfio_init_intp(VFIODevice *vbasedev,
         g_free(intp->interrupt);
         g_free(intp);
         error_setg_errno(errp, -ret,
-                         "failed to initialize trigger eventd notifier");
+                         "failed to initialize trigger eventfd notifier");
         return NULL;
     }
     if (vfio_irq_is_automasked(intp)) {

hw/vfio/platform.c static VFIOINTp *vfio_init_intp(VFIODevice *vbasedev, · line 90

Quoted 8 lines

hw/vfio/platform.c +1 -1

@@ -84,7 +84,7 @@ static VFIOINTp *vfio_init_intp(VFIODevice *vbasedev,
             g_free(intp->unmask);
             g_free(intp);
             error_setg_errno(errp, -ret,
-                             "failed to initialize resample eventd notifier");
+                             "failed to initialize resample eventfd notifier");
             return NULL;
         }
     }

There is another in "sysemu/kvm.h".

Auger Eric

Reviewed-by on v1 4/7

2018-10-19 16:41 UTC #6 lore raw

Re: [Qemu-devel] [PATCH 4/7] vfio: paltform: fix a typo

In reply to Li Qiang

Hi,

Quoted 20 lines · Li Qiang

Signed-off-by Li Qiang <[email protected]>

---
 hw/vfio/platform.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

hw/vfio/platform.c +1 -1

index ba03dcd..5992fe7 100644
@@ -72,7 +72,7 @@ static VFIOINTp *vfio_init_intp(VFIODevice *vbasedev,
         g_free(intp->interrupt);
         g_free(intp);
         error_setg_errno(errp, -ret,
-                         "failed to initialize trigger eventd notifier");
+                         "failed to initialize trigger eventfd notifier");
         return NULL;
     }
     if (vfio_irq_is_automasked(intp)) {

hw/vfio/platform.c static VFIOINTp *vfio_init_intp(VFIODevice *vbasedev, · line 90

Quoted 8 lines

hw/vfio/platform.c +1 -1

@@ -84,7 +84,7 @@ static VFIOINTp *vfio_init_intp(VFIODevice *vbasedev,
             g_free(intp->unmask);
             g_free(intp);
             error_setg_errno(errp, -ret,
-                             "failed to initialize resample eventd notifier");
+                             "failed to initialize resample eventfd notifier");
             return NULL;
         }
     }

s/palform/platform in the commit message.

Besides

Reviewed-by Eric Auger <[email protected]>

Thanks

Eric

Li Qiang

author

2018-10-19 05:20 UTC #7 lore raw

PATCH 5/7 vfio: platform: cleanup the notifier in error path

In reply to Li Qiang

Signed-off-by Li Qiang <[email protected]>

---
 hw/vfio/platform.c | 1 +
 1 file changed, 1 insertion(+)

hw/vfio/platform.c +1 -0

index 5992fe7..6a4fd7b 100644
@@ -80,6 +80,7 @@ static VFIOINTp *vfio_init_intp(VFIODevice *vbasedev,
         intp->unmask = g_malloc0(sizeof(EventNotifier));
         ret = event_notifier_init(intp->unmask, 0);
         if (ret) {
+            event_notifier_cleanup(intp->interrupt);
             g_free(intp->interrupt);
             g_free(intp->unmask);
             g_free(intp);

Signature · 1 line

1.8.3.1

Li Qiang

author

2018-10-19 05:20 UTC #8 lore raw

PATCH 6/7 vfio: platform: free timer in error path

In reply to Li Qiang

Signed-off-by Li Qiang <[email protected]>

---
 hw/vfio/platform.c | 1 +
 1 file changed, 1 insertion(+)

hw/vfio/platform.c +1 -0

index 6a4fd7b..ba19143 100644
@@ -518,6 +518,7 @@ static int vfio_populate_device(VFIODevice *vbasedev, Error **errp)
     return 0;
 irq_err:
     timer_del(vdev->mmap_timer);
+    timer_free(vdev->mmap_timer);
     QLIST_FOREACH_SAFE(intp, &vdev->intp_list, next, tmp) {
         QLIST_REMOVE(intp, next);
         g_free(intp);

Signature · 1 line

1.8.3.1

Li Qiang

author

2018-10-19 05:20 UTC #9 lore raw

PATCH 7/7 vfio: platform: destory mutex in error path

In reply to Li Qiang

Signed-off-by Li Qiang <[email protected]>

---
 hw/vfio/platform.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

hw/vfio/platform.c +3 -1

index ba19143..e9d9e80 100644
@@ -668,7 +668,7 @@ static void vfio_platform_realize(DeviceState *dev, Error **errp)
             error_setg(errp, "%s", gerr->message);
             g_error_free(gerr);
             g_free(path);
-            return;
+            goto out;
         }
         g_free(path);
         vdev->compat = contents;

@@ -691,6 +691,8 @@ out:
         return;
     }
 
+    qemu_mutex_destroy(&vdev->intp_mutex);
+
     if (vdev->vbasedev.name) {
         error_prepend(errp, ERR_PREFIX, vdev->vbasedev.name);
     } else {

Signature · 1 line

1.8.3.1

Auger Eric

2018-10-19 16:41 UTC #10 lore raw

Re: [Qemu-devel] [PATCH 7/7] vfio: platform: destory mutex in error path

Hi Li,

Quoted 10 lines · Li Qiang

Signed-off-by Li Qiang <[email protected]>
---
 hw/vfio/platform.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
hw/vfio/platform.c +0 -0
index ba19143..e9d9e80 100644

hw/vfio/platform.c static void vfio_platform_realize(DeviceState *dev, Error **errp) · line 671

Quoted 5 lines

hw/vfio/platform.c +1 -1

@@ -668,7 +668,7 @@ static void vfio_platform_realize(DeviceState *dev, Error **errp)
             error_setg(errp, "%s", gerr->message);
             g_error_free(gerr);
             g_free(path);
-            return;
+            goto out;

You must set ret to != 0 otherwise the qemu_mutex_destroy will not be reached I think. Also this will fix the fact we are not prepending the vfio error prefix in that case, as we should.

Besides I am unsure about the cleanup strategy in case or error in vfio_platform_realize(). The qemu process should always exit in case of failure in vfio_platform_realize(). Platform devices can only be cold-plugged through the qemu CLI. Cleaning all the allocated resources may add a substantial amount of code. For instance resources allocated in vfio_base_device_init() are not freed either. Comprehensive free in realize() functions may only be needed in case of hotplug I think.

Thanks

Eric

Quoted 13 lines

hw/vfio/platform.c +2 -0

         }
         g_free(path);
         vdev->compat = contents;

@@ -691,6 +691,8 @@ out:
         return;
     }
 
+    qemu_mutex_destroy(&vdev->intp_mutex);
+
     if (vdev->vbasedev.name) {
         error_prepend(errp, ERR_PREFIX, vdev->vbasedev.name);
     } else {

Li Qiang

author

2018-10-22 02:14 UTC #11 lore raw

Re: [Qemu-devel] [PATCH 7/7] vfio: platform: destory mutex in error path

Hello Auger,

Quoted 14 lines · Auger Eric <[email protected]> 于2018年10月20日周六上午12:41写道：

Hi Li,
hw/vfio/platform.c static void vfio_platform_realize(DeviceState *dev,
Quoted 10 lines · Li Qiang
Signed-off-by Li Qiang <[email protected]>
---
 hw/vfio/platform.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
hw/vfio/platform.c +0 -0
index ba19143..e9d9e80 100644
@@ -668,7 +668,7 @@ static void vfio_platform_realize(DeviceState *dev,
Error **errp)

Quoted 7 lines

hw/vfio/platform.c static void vfio_platform_realize(DeviceState *dev, Error **errp) · line 671
Quoted 5 lines
hw/vfio/platform.c +1 -1
             error_setg(errp, "%s", gerr->message);
             g_error_free(gerr);
             g_free(path);
-            return;
+            goto out;
You must set ret to != 0 otherwise the qemu_mutex_destroy will not be reached I think.

Also this will fix the fact we are not prepending the

Quoted 6 lines

vfio error prefix in that case, as we should.
Besides I am unsure about the cleanup strategy in case or error in vfio_platform_realize(). The qemu process should always exit in case of failure in vfio_platform_realize(). Platform devices can only be cold-plugged through the qemu CLI.

Got this.

Quoted 2 lines

Cleaning all the allocated resources
may add a substantial amount of code.

Agree.

Thanks,
Li Qiang

Quoted 21 lines

For instance resources allocated
in vfio_base_device_init() are not freed either. Comprehensive free in realize() functions may only be needed in case of hotplug I think.
Thanks
Eric
Quoted 13 lines
hw/vfio/platform.c +2 -0
         }
         g_free(path);
         vdev->compat = contents;
@@ -691,6 +691,8 @@ out:
         return;
     }
 
+    qemu_mutex_destroy(&vdev->intp_mutex);
+
     if (vdev->vbasedev.name) {
         error_prepend(errp, ERR_PREFIX, vdev->vbasedev.name);
     } else {

Philippe Mathieu-Daudé

Reviewed-by on the v1 series

2018-10-19 05:25 UTC #12 lore raw

In reply to Li Qiang

Quoted 8 lines · Li Qiang

This patch set contains some trivial issue such as QOMConvetion, typo and resources leak in vfio.
Li Qiang (7):
vfio-pci: make "vfio-pci-nohotplug" as MACRO
vfio: ap-device: make it more QOMConventional
vfio: drop TYPE_FOO MACRO in VMStateDescription
vfio: paltform: fix a typo

For patches 1-4 that are indeed trivial:

Reviewed-by Philippe Mathieu-Daudé <[email protected]>

Quoted 3 lines

vfio: platform: cleanup the notifier in error path
vfio: platform: free timer in error path
vfio: platform: destory mutex in error path

Those are less trivial ;)

Quoted 11 lines

 hw/s390x/ap-device.c         |  2 +-
 hw/vfio/amd-xgbe.c           |  2 +-
 hw/vfio/ap.c                 | 12 ++++++------
 hw/vfio/calxeda-xgmac.c      |  2 +-
 hw/vfio/ccw.c                |  2 +-
 hw/vfio/pci.c                |  6 ++++--
 hw/vfio/platform.c           | 12 ++++++++----
 include/hw/s390x/ap-device.h |  4 ++--
 8 files changed, 24 insertions(+), 18 deletions(-)