[PATCH 0/4] VFIO device init cleanup

[PATCH 0/4] VFIO device init cleanup

[Zhenzhong Duan]

Hi,

This is a clean up based on Cedric's suggestion at
https://lists.gnu.org/archive/html/qemu-devel/2023-11/msg02722.html

VFIO device initializations are all moved from realize to instance_init.

Based on https://github.com/legoater/qemu/commits/vfio-8.2

Thanks
Zhenzhong

Zhenzhong Duan (4):
  vfio/pci: Move VFIODevice initializations in vfio_instance_init
  vfio/platform: Move VFIODevice initializations in
    vfio_platform_instance_init
  vfio/ap: Move VFIODevice initializations in vfio_ap_instance_init
  vfio/ccw: Move VFIODevice initializations in vfio_ccw_instance_init

 hw/vfio/ap.c       | 26 +++++++++++++-------------
 hw/vfio/ccw.c      | 30 +++++++++++++++---------------
 hw/vfio/pci.c      | 10 ++++++----
 hw/vfio/platform.c | 10 +++++-----
 4 files changed, 39 insertions(+), 37 deletions(-)

-- 
2.34.1

[PATCH 1/4] vfio/pci: Move VFIODevice initializations in vfio_instance_init

[Zhenzhong Duan]

Some of the VFIODevice initializations is in vfio_realize,
move all of them in vfio_instance_init.

No functional change intended.

Suggested-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/pci.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index b23b492cce..5a2b7a2d6b 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2969,9 +2969,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
     if (vfio_device_get_name(vbasedev, errp)) {
         return;
     }
-    vbasedev->ops = &vfio_pci_ops;
-    vbasedev->type = VFIO_DEVICE_TYPE_PCI;
-    vbasedev->dev = DEVICE(vdev);
 
     /*
      * Mediated devices *might* operate compatibly with discarding of RAM, but
@@ -3320,6 +3317,7 @@ static void vfio_instance_init(Object *obj)
 {
     PCIDevice *pci_dev = PCI_DEVICE(obj);
     VFIOPCIDevice *vdev = VFIO_PCI(obj);
+    VFIODevice *vbasedev = &vdev->vbasedev;
 
     device_add_bootindex_property(obj, &vdev->bootindex,
                                   "bootindex", NULL,
@@ -3328,7 +3326,11 @@ static void vfio_instance_init(Object *obj)
     vdev->host.bus = ~0U;
     vdev->host.slot = ~0U;
     vdev->host.function = ~0U;
-    vdev->vbasedev.fd = -1;
+
+    vbasedev->type = VFIO_DEVICE_TYPE_PCI;
+    vbasedev->ops = &vfio_pci_ops;
+    vbasedev->dev = DEVICE(vdev);
+    vbasedev->fd = -1;
 
     vdev->nv_gpudirect_clique = 0xFF;
 
-- 
2.34.1

Re: [PATCH 1/4] vfio/pci: Move VFIODevice initializations in vfio_instance_init

[Cédric Le Goater]

On 11/15/23 09:32, Zhenzhong Duan wrote:
> Some of the VFIODevice initializations is in vfio_realize,
> move all of them in vfio_instance_init.
> 
> No functional change intended.
> 
> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   hw/vfio/pci.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index b23b492cce..5a2b7a2d6b 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2969,9 +2969,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>       if (vfio_device_get_name(vbasedev, errp)) {
>           return;
>       }
> -    vbasedev->ops = &vfio_pci_ops;
> -    vbasedev->type = VFIO_DEVICE_TYPE_PCI;
> -    vbasedev->dev = DEVICE(vdev);
>   
>       /*
>        * Mediated devices *might* operate compatibly with discarding of RAM, but
> @@ -3320,6 +3317,7 @@ static void vfio_instance_init(Object *obj)
>   {
>       PCIDevice *pci_dev = PCI_DEVICE(obj);
>       VFIOPCIDevice *vdev = VFIO_PCI(obj);
> +    VFIODevice *vbasedev = &vdev->vbasedev;
>   
>       device_add_bootindex_property(obj, &vdev->bootindex,
>                                     "bootindex", NULL,
> @@ -3328,7 +3326,11 @@ static void vfio_instance_init(Object *obj)
>       vdev->host.bus = ~0U;
>       vdev->host.slot = ~0U;
>       vdev->host.function = ~0U;
> -    vdev->vbasedev.fd = -1;
> +
> +    vbasedev->type = VFIO_DEVICE_TYPE_PCI;
> +    vbasedev->ops = &vfio_pci_ops;
> +    vbasedev->dev = DEVICE(vdev);
> +    vbasedev->fd = -1;

VFIODevice is similar to a base QOM parent. Could we introduce an helper
routine like we did with vfio_device_set_fd() ?

Thanks,

C.

   
>       vdev->nv_gpudirect_clique = 0xFF;
>   

RE: [PATCH 1/4] vfio/pci: Move VFIODevice initializations in vfio_instance_init

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Sent: Wednesday, November 15, 2023 9:12 PM
>Subject: Re: [PATCH 1/4] vfio/pci: Move VFIODevice initializations in
>vfio_instance_init
>
>On 11/15/23 09:32, Zhenzhong Duan wrote:
>> Some of the VFIODevice initializations is in vfio_realize,
>> move all of them in vfio_instance_init.
>>
>> No functional change intended.
>>
>> Suggested-by: Cédric Le Goater <clg@redhat.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   hw/vfio/pci.c | 10 ++++++----
>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index b23b492cce..5a2b7a2d6b 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2969,9 +2969,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>>       if (vfio_device_get_name(vbasedev, errp)) {
>>           return;
>>       }
>> -    vbasedev->ops = &vfio_pci_ops;
>> -    vbasedev->type = VFIO_DEVICE_TYPE_PCI;
>> -    vbasedev->dev = DEVICE(vdev);
>>
>>       /*
>>        * Mediated devices *might* operate compatibly with discarding of RAM,
>but
>> @@ -3320,6 +3317,7 @@ static void vfio_instance_init(Object *obj)
>>   {
>>       PCIDevice *pci_dev = PCI_DEVICE(obj);
>>       VFIOPCIDevice *vdev = VFIO_PCI(obj);
>> +    VFIODevice *vbasedev = &vdev->vbasedev;
>>
>>       device_add_bootindex_property(obj, &vdev->bootindex,
>>                                     "bootindex", NULL,
>> @@ -3328,7 +3326,11 @@ static void vfio_instance_init(Object *obj)
>>       vdev->host.bus = ~0U;
>>       vdev->host.slot = ~0U;
>>       vdev->host.function = ~0U;
>> -    vdev->vbasedev.fd = -1;
>> +
>> +    vbasedev->type = VFIO_DEVICE_TYPE_PCI;
>> +    vbasedev->ops = &vfio_pci_ops;
>> +    vbasedev->dev = DEVICE(vdev);
>> +    vbasedev->fd = -1;
>
>VFIODevice is similar to a base QOM parent. Could we introduce an helper
>routine like we did with vfio_device_set_fd() ?

Sure, will do.

Thanks
Zhenzhong

Re: [PATCH 1/4] vfio/pci: Move VFIODevice initializations in vfio_instance_init

[Cédric Le Goater]

On 11/16/23 03:16, Duan, Zhenzhong wrote:
> 
> 
>> -----Original Message-----
>> From: Cédric Le Goater <clg@redhat.com>
>> Sent: Wednesday, November 15, 2023 9:12 PM
>> Subject: Re: [PATCH 1/4] vfio/pci: Move VFIODevice initializations in
>> vfio_instance_init
>>
>> On 11/15/23 09:32, Zhenzhong Duan wrote:
>>> Some of the VFIODevice initializations is in vfio_realize,
>>> move all of them in vfio_instance_init.
>>>
>>> No functional change intended.
>>>
>>> Suggested-by: Cédric Le Goater <clg@redhat.com>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>> ---
>>>    hw/vfio/pci.c | 10 ++++++----
>>>    1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>> index b23b492cce..5a2b7a2d6b 100644
>>> --- a/hw/vfio/pci.c
>>> +++ b/hw/vfio/pci.c
>>> @@ -2969,9 +2969,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>>>        if (vfio_device_get_name(vbasedev, errp)) {
>>>            return;
>>>        }
>>> -    vbasedev->ops = &vfio_pci_ops;
>>> -    vbasedev->type = VFIO_DEVICE_TYPE_PCI;
>>> -    vbasedev->dev = DEVICE(vdev);
>>>
>>>        /*
>>>         * Mediated devices *might* operate compatibly with discarding of RAM,
>> but
>>> @@ -3320,6 +3317,7 @@ static void vfio_instance_init(Object *obj)
>>>    {
>>>        PCIDevice *pci_dev = PCI_DEVICE(obj);
>>>        VFIOPCIDevice *vdev = VFIO_PCI(obj);
>>> +    VFIODevice *vbasedev = &vdev->vbasedev;
>>>
>>>        device_add_bootindex_property(obj, &vdev->bootindex,
>>>                                      "bootindex", NULL,
>>> @@ -3328,7 +3326,11 @@ static void vfio_instance_init(Object *obj)
>>>        vdev->host.bus = ~0U;
>>>        vdev->host.slot = ~0U;
>>>        vdev->host.function = ~0U;
>>> -    vdev->vbasedev.fd = -1;
>>> +
>>> +    vbasedev->type = VFIO_DEVICE_TYPE_PCI;
>>> +    vbasedev->ops = &vfio_pci_ops;
>>> +    vbasedev->dev = DEVICE(vdev);
>>> +    vbasedev->fd = -1;
>>
>> VFIODevice is similar to a base QOM parent. Could we introduce an helper
>> routine like we did with vfio_device_set_fd() ?
> 
> Sure, will do.

Since this series is reviewed, could you please consolidate with an extra
patch on top of this v1 ?

Thanks,

C.

RE: [PATCH 1/4] vfio/pci: Move VFIODevice initializations in vfio_instance_init

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Sent: Thursday, November 16, 2023 3:29 PM
>To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu-devel@nongnu.org
>Cc: alex.williamson@redhat.com; jgg@nvidia.com; nicolinc@nvidia.com;
>joao.m.martins@oracle.com; eric.auger@redhat.com; peterx@redhat.com;
>jasowang@redhat.com; Tian, Kevin <kevin.tian@intel.com>; Liu, Yi L
><yi.l.liu@intel.com>; Sun, Yi Y <yi.y.sun@intel.com>; Peng, Chao P
><chao.p.peng@intel.com>
>Subject: Re: [PATCH 1/4] vfio/pci: Move VFIODevice initializations in
>vfio_instance_init
>
>On 11/16/23 03:16, Duan, Zhenzhong wrote:
>>
>>
>>> -----Original Message-----
>>> From: Cédric Le Goater <clg@redhat.com>
>>> Sent: Wednesday, November 15, 2023 9:12 PM
>>> Subject: Re: [PATCH 1/4] vfio/pci: Move VFIODevice initializations in
>>> vfio_instance_init
>>>
>>> On 11/15/23 09:32, Zhenzhong Duan wrote:
>>>> Some of the VFIODevice initializations is in vfio_realize,
>>>> move all of them in vfio_instance_init.
>>>>
>>>> No functional change intended.
>>>>
>>>> Suggested-by: Cédric Le Goater <clg@redhat.com>
>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>> ---
>>>>    hw/vfio/pci.c | 10 ++++++----
>>>>    1 file changed, 6 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>>> index b23b492cce..5a2b7a2d6b 100644
>>>> --- a/hw/vfio/pci.c
>>>> +++ b/hw/vfio/pci.c
>>>> @@ -2969,9 +2969,6 @@ static void vfio_realize(PCIDevice *pdev, Error
>**errp)
>>>>        if (vfio_device_get_name(vbasedev, errp)) {
>>>>            return;
>>>>        }
>>>> -    vbasedev->ops = &vfio_pci_ops;
>>>> -    vbasedev->type = VFIO_DEVICE_TYPE_PCI;
>>>> -    vbasedev->dev = DEVICE(vdev);
>>>>
>>>>        /*
>>>>         * Mediated devices *might* operate compatibly with discarding of RAM,
>>> but
>>>> @@ -3320,6 +3317,7 @@ static void vfio_instance_init(Object *obj)
>>>>    {
>>>>        PCIDevice *pci_dev = PCI_DEVICE(obj);
>>>>        VFIOPCIDevice *vdev = VFIO_PCI(obj);
>>>> +    VFIODevice *vbasedev = &vdev->vbasedev;
>>>>
>>>>        device_add_bootindex_property(obj, &vdev->bootindex,
>>>>                                      "bootindex", NULL,
>>>> @@ -3328,7 +3326,11 @@ static void vfio_instance_init(Object *obj)
>>>>        vdev->host.bus = ~0U;
>>>>        vdev->host.slot = ~0U;
>>>>        vdev->host.function = ~0U;
>>>> -    vdev->vbasedev.fd = -1;
>>>> +
>>>> +    vbasedev->type = VFIO_DEVICE_TYPE_PCI;
>>>> +    vbasedev->ops = &vfio_pci_ops;
>>>> +    vbasedev->dev = DEVICE(vdev);
>>>> +    vbasedev->fd = -1;
>>>
>>> VFIODevice is similar to a base QOM parent. Could we introduce an helper
>>> routine like we did with vfio_device_set_fd() ?
>>
>> Sure, will do.
>
>Since this series is reviewed, could you please consolidate with an extra
>patch on top of this v1 ?

Got it.

Thanks
Zhenzhong

[PATCH 2/4] vfio/platform: Move VFIODevice initializations in vfio_platform_instance_init

[Zhenzhong Duan]

Some of the VFIODevice initializations is in vfio_platform_realize,
move all of them in vfio_platform_instance_init.

No functional change intended.

Suggested-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/platform.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index a97d9c6234..506eb8193f 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -581,10 +581,6 @@ static void vfio_platform_realize(DeviceState *dev, Error **errp)
     VFIODevice *vbasedev = &vdev->vbasedev;
     int i, ret;
 
-    vbasedev->type = VFIO_DEVICE_TYPE_PLATFORM;
-    vbasedev->dev = dev;
-    vbasedev->ops = &vfio_platform_ops;
-
     qemu_mutex_init(&vdev->intp_mutex);
 
     trace_vfio_platform_realize(vbasedev->sysfsdev ?
@@ -659,8 +655,12 @@ static Property vfio_platform_dev_properties[] = {
 static void vfio_platform_instance_init(Object *obj)
 {
     VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(obj);
+    VFIODevice *vbasedev = &vdev->vbasedev;
 
-    vdev->vbasedev.fd = -1;
+    vbasedev->type = VFIO_DEVICE_TYPE_PLATFORM;
+    vbasedev->ops = &vfio_platform_ops;
+    vbasedev->dev = DEVICE(vdev);
+    vbasedev->fd = -1;
 }
 
 #ifdef CONFIG_IOMMUFD
-- 
2.34.1

[PATCH 3/4] vfio/ap: Move VFIODevice initializations in vfio_ap_instance_init

[Zhenzhong Duan]

Some of the VFIODevice initializations is in vfio_ap_realize,
move all of them in vfio_ap_instance_init.

No functional change intended.

Suggested-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/ap.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
index b21f92291e..31ea9644c5 100644
--- a/hw/vfio/ap.c
+++ b/hw/vfio/ap.c
@@ -164,18 +164,6 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    vbasedev->ops = &vfio_ap_ops;
-    vbasedev->type = VFIO_DEVICE_TYPE_AP;
-    vbasedev->dev = dev;
-
-    /*
-     * vfio-ap devices operate in a way compatible with discarding of
-     * memory in RAM blocks, as no pages are pinned in the host.
-     * This needs to be set before vfio_get_device() for vfio common to
-     * handle ram_block_discard_disable().
-     */
-    vapdev->vdev.ram_block_discard_allowed = true;
-
     ret = vfio_attach_device(vbasedev->name, vbasedev,
                              &address_space_memory, errp);
     if (ret) {
@@ -236,8 +224,20 @@ static const VMStateDescription vfio_ap_vmstate = {
 static void vfio_ap_instance_init(Object *obj)
 {
     VFIOAPDevice *vapdev = VFIO_AP_DEVICE(obj);
+    VFIODevice *vbasedev = &vapdev->vdev;
 
-    vapdev->vdev.fd = -1;
+    vbasedev->type = VFIO_DEVICE_TYPE_AP;
+    vbasedev->ops = &vfio_ap_ops;
+    vbasedev->dev = DEVICE(vapdev);
+    vbasedev->fd = -1;
+
+    /*
+     * vfio-ap devices operate in a way compatible with discarding of
+     * memory in RAM blocks, as no pages are pinned in the host.
+     * This needs to be set before vfio_get_device() for vfio common to
+     * handle ram_block_discard_disable().
+     */
+    vbasedev->ram_block_discard_allowed = true;
 }
 
 #ifdef CONFIG_IOMMUFD
-- 
2.34.1

Re: [PATCH 3/4] vfio/ap: Move VFIODevice initializations in vfio_ap_instance_init

[Eric Farman]

On Wed, 2023-11-15 at 16:32 +0800, Zhenzhong Duan wrote:
> Some of the VFIODevice initializations is in vfio_ap_realize,
> move all of them in vfio_ap_instance_init.
> 
> No functional change intended.
> 
> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  hw/vfio/ap.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)

Reviewed-by: Eric Farman <farman@linux.ibm.com>

[PATCH 4/4] vfio/ccw: Move VFIODevice initializations in vfio_ccw_instance_init

[Zhenzhong Duan]

Some of the VFIODevice initializations is in vfio_ccw_realize,
move all of them in vfio_ccw_instance_init.

No functional change intended.

Suggested-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/ccw.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index b116b10fe7..8de2fd809b 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -594,20 +594,6 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    vbasedev->ops = &vfio_ccw_ops;
-    vbasedev->type = VFIO_DEVICE_TYPE_CCW;
-    vbasedev->dev = dev;
-
-    /*
-     * All vfio-ccw devices are believed to operate in a way compatible with
-     * discarding of memory in RAM blocks, ie. pages pinned in the host are
-     * in the current working set of the guest driver and therefore never
-     * overlap e.g., with pages available to the guest balloon driver.  This
-     * needs to be set before vfio_get_device() for vfio common to handle
-     * ram_block_discard_disable().
-     */
-    vbasedev->ram_block_discard_allowed = true;
-
     ret = vfio_attach_device(cdev->mdevid, vbasedev,
                              &address_space_memory, errp);
     if (ret) {
@@ -695,8 +681,22 @@ static const VMStateDescription vfio_ccw_vmstate = {
 static void vfio_ccw_instance_init(Object *obj)
 {
     VFIOCCWDevice *vcdev = VFIO_CCW(obj);
+    VFIODevice *vbasedev = &vcdev->vdev;
+
+    vbasedev->type = VFIO_DEVICE_TYPE_CCW;
+    vbasedev->ops = &vfio_ccw_ops;
+    vbasedev->dev = DEVICE(vcdev);
+    vbasedev->fd = -1;
 
-    vcdev->vdev.fd = -1;
+    /*
+     * All vfio-ccw devices are believed to operate in a way compatible with
+     * discarding of memory in RAM blocks, ie. pages pinned in the host are
+     * in the current working set of the guest driver and therefore never
+     * overlap e.g., with pages available to the guest balloon driver.  This
+     * needs to be set before vfio_get_device() for vfio common to handle
+     * ram_block_discard_disable().
+     */
+    vbasedev->ram_block_discard_allowed = true;
 }
 
 #ifdef CONFIG_IOMMUFD
-- 
2.34.1

Re: [PATCH 4/4] vfio/ccw: Move VFIODevice initializations in vfio_ccw_instance_init

[Eric Farman]

On Wed, 2023-11-15 at 16:32 +0800, Zhenzhong Duan wrote:
> Some of the VFIODevice initializations is in vfio_ccw_realize,
> move all of them in vfio_ccw_instance_init.
> 
> No functional change intended.
> 
> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  hw/vfio/ccw.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)

Reviewed-by: Eric Farman <farman@linux.ibm.com>

Re: [PATCH 0/4] VFIO device init cleanup

[Philippe Mathieu-Daudé]

On 15/11/23 09:32, Zhenzhong Duan wrote:

> Zhenzhong Duan (4):
>    vfio/pci: Move VFIODevice initializations in vfio_instance_init
>    vfio/platform: Move VFIODevice initializations in
>      vfio_platform_instance_init
>    vfio/ap: Move VFIODevice initializations in vfio_ap_instance_init
>    vfio/ccw: Move VFIODevice initializations in vfio_ccw_instance_init

Series:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>