[Qemu-devel] [PATCH 0/7] vfio: some trivial fixes

[Qemu-devel] [PATCH 0/7] vfio: some trivial fixes

[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
  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(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/7] vfio-pci: make "vfio-pci-nohotplug" as MACRO

[Li Qiang]

Signed-off-by: Li Qiang <liq3ea@gmail.com>
---
 hw/vfio/pci.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 8b73582..1f05b57 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -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,
 };
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/7] vfio: ap-device: make it more QOMConventional

[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 <liq3ea@gmail.com>
---
 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(-)

diff --git a/hw/s390x/ap-device.c b/hw/s390x/ap-device.c
index f5ac8db..64a5f11 100644
--- a/hw/s390x/ap-device.c
+++ b/hw/s390x/ap-device.c
@@ -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),
diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
index 3962bb7..b332395 100644
--- a/hw/vfio/ap.c
+++ b/hw/vfio/ap.c
@@ -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,
 };
diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h
index 765e908..d7eced4 100644
--- a/include/hw/s390x/ap-device.h
+++ b/include/hw/s390x/ap-device.h
@@ -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 */
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/7] vfio: drop TYPE_FOO MACRO in VMStateDescription

[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 <liq3ea@gmail.com>
---
 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(-)

diff --git a/hw/vfio/amd-xgbe.c b/hw/vfio/amd-xgbe.c
index ee64a3b..1b06c0f 100644
--- a/hw/vfio/amd-xgbe.c
+++ b/hw/vfio/amd-xgbe.c
@@ -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,
 };
 
diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
index b332395..35b01b3 100644
--- a/hw/vfio/ap.c
+++ b/hw/vfio/ap.c
@@ -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,
 };
 
diff --git a/hw/vfio/calxeda-xgmac.c b/hw/vfio/calxeda-xgmac.c
index e7767c4..6cc608b 100644
--- a/hw/vfio/calxeda-xgmac.c
+++ b/hw/vfio/calxeda-xgmac.c
@@ -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,
 };
 
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 9246729..4786d46 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -466,7 +466,7 @@ static Property vfio_ccw_properties[] = {
 };
 
 static const VMStateDescription vfio_ccw_vmstate = {
-    .name = TYPE_VFIO_CCW,
+    .name = "vfio-ccw",
     .unmigratable = 1,
 };
 
diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index 64c1af6..ba03dcd 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -697,7 +697,7 @@ out:
 }
 
 static const VMStateDescription vfio_platform_vmstate = {
-    .name = TYPE_VFIO_PLATFORM,
+    .name = "vfio-platform",
     .unmigratable = 1,
 };
 
-- 
1.8.3.1

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

[Li Qiang]

Signed-off-by: Li Qiang <liq3ea@gmail.com>
---
 hw/vfio/platform.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index ba03dcd..5992fe7 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -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;
         }
     }
-- 
1.8.3.1

[Qemu-devel] [PATCH 5/7] vfio: platform: cleanup the notifier in error path

[Li Qiang]

Signed-off-by: Li Qiang <liq3ea@gmail.com>
---
 hw/vfio/platform.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index 5992fe7..6a4fd7b 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -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);
-- 
1.8.3.1

[Qemu-devel] [PATCH 6/7] vfio: platform: free timer in error path

[Li Qiang]

Signed-off-by: Li Qiang <liq3ea@gmail.com>
---
 hw/vfio/platform.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index 6a4fd7b..ba19143 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -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);
-- 
1.8.3.1

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

[Li Qiang]

Signed-off-by: Li Qiang <liq3ea@gmail.com>
---
 hw/vfio/platform.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index ba19143..e9d9e80 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -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 {
-- 
1.8.3.1

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

[Philippe Mathieu-Daudé]

On 19/10/2018 07:20, Li Qiang wrote:
> Signed-off-by: Li Qiang <liq3ea@gmail.com>
> ---
>  hw/vfio/platform.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
> index ba03dcd..5992fe7 100644
> --- a/hw/vfio/platform.c
> +++ b/hw/vfio/platform.c
> @@ -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;
>          }
>      }
> 

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

Re: [Qemu-devel] [PATCH 0/7] vfio: some trivial fixes

[Philippe Mathieu-Daudé]

On 19/10/2018 07:20, Li Qiang wrote:
> 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é <philmd@redhat.com>

>   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 ;)

> 
>  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(-)
> 

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

[Auger Eric]

Hi Li,

On 10/19/18 7:20 AM, Li Qiang wrote:
> Signed-off-by: Li Qiang <liq3ea@gmail.com>
> ---
>  hw/vfio/platform.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
> index ba19143..e9d9e80 100644
> --- a/hw/vfio/platform.c
> +++ b/hw/vfio/platform.c
> @@ -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
>          }
>          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 {
> 

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

[Auger Eric]

Hi,

On 10/19/18 7:20 AM, Li Qiang wrote:
> Signed-off-by: Li Qiang <liq3ea@gmail.com>
> ---
>  hw/vfio/platform.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
> index ba03dcd..5992fe7 100644
> --- a/hw/vfio/platform.c
> +++ b/hw/vfio/platform.c
> @@ -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;
>          }
>      }
> 
s/palform/platform in the commit message.

Besides
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

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

[Li Qiang]

Hello Auger,

Auger Eric <eric.auger@redhat.com> 于2018年10月20日周六 上午12:41写道：

> Hi Li,
>
> On 10/19/18 7:20 AM, Li Qiang wrote:
> > Signed-off-by: Li Qiang <liq3ea@gmail.com>
> > ---
> >  hw/vfio/platform.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
> > index ba19143..e9d9e80 100644
> > --- a/hw/vfio/platform.c
> > +++ b/hw/vfio/platform.c
> > @@ -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.


Got this.


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


Agree.


Thanks,
Li Qiang


> 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
> >          }
> >          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 {
> >
>