Re: [PATCH v29 05/17] vfio: Add VM state change handler to know state of VM

From: Kirti Wankhede <kwankhede@nvidia.com>
To: Alex Williamson <alex.williamson@redhat.com>
Cc: cohuck@redhat.com, cjia@nvidia.com, zhi.wang.linux@gmail.com,
	aik@ozlabs.ru, Zhengxiao.zx@Alibaba-inc.com,
	shuangtai.tst@alibaba-inc.com, qemu-devel@nongnu.org,
	peterx@redhat.com, eauger@redhat.com, artemp@nvidia.com,
	yi.l.liu@intel.com, quintela@redhat.com, ziye.yang@intel.com,
	armbru@redhat.com, mlevitsk@redhat.com, pasic@linux.ibm.com,
	felipe@nutanix.com, zhi.a.wang@intel.com, mcrossley@nvidia.com,
	kevin.tian@intel.com, yan.y.zhao@intel.com, dgilbert@redhat.com,
	changpeng.liu@intel.com, eskultet@redhat.com, Ken.Xue@amd.com,
	jonathan.davies@nutanix.com, pbonzini@redhat.com,
	dnigam@nvidia.com
Subject: Re: [PATCH v29 05/17] vfio: Add VM state change handler to know state of VM
Date: Mon, 26 Oct 2020 20:07:07 +0530	[thread overview]
Message-ID: <9c5e89fc-b247-5897-ef23-d198a97b48d3@nvidia.com> (raw)
In-Reply-To: <20201026080257.2f9d3506@w520.home>

On 10/26/2020 7:32 PM, Alex Williamson wrote:
> On Mon, 26 Oct 2020 19:18:51 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> 
>> On 10/26/2020 6:30 PM, Alex Williamson wrote:
>>> On Mon, 26 Oct 2020 15:06:15 +0530
>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>>>    
>>>> VM state change handler is called on change in VM's state. Based on
>>>> VM state, VFIO device state should be changed.
>>>> Added read/write helper functions for migration region.
>>>> Added function to set device_state.
>>>>
>>>> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
>>>> Reviewed-by: Neo Jia <cjia@nvidia.com>
>>>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>>> ---
>>>>    hw/vfio/migration.c           | 158 ++++++++++++++++++++++++++++++++++++++++++
>>>>    hw/vfio/trace-events          |   2 +
>>>>    include/hw/vfio/vfio-common.h |   4 ++
>>>>    3 files changed, 164 insertions(+)
>>>>
>>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>>>> index fd7faf423cdc..65ce735d667b 100644
>>>> --- a/hw/vfio/migration.c
>>>> +++ b/hw/vfio/migration.c
>>> [snip]
>>>> @@ -64,6 +216,9 @@ static int vfio_migration_init(VFIODevice *vbasedev,
>>>>            ret = -EINVAL;
>>>>            goto err;
>>>>        }
>>>> +
>>>> +    migration->vm_state = qemu_add_vm_change_state_handler(vfio_vmstate_change,
>>>> +                                                           vbasedev);
>>>>        return 0;
>>>>    
>>>>    err:
>>>
>>> Fails to build, @migration is not defined.  We could use
>>> vbasedev->migration or pull defining and setting @migration from patch
>>> 06.  Thanks,
>>>    
>>
>> Pulling and setting migration from patch 06 seems better option.
>> Should I resend patch 5 & 6 only?
> 
> I've resolved this locally as patch 05:
> 
> @@ -38,6 +190,7 @@ static int vfio_migration_init(VFIODevice *vbasedev,
>   {
>       int ret;
>       Object *obj;
> +    VFIOMigration *migration;
>   
>       if (!vbasedev->ops->vfio_get_object) {
>           return -EINVAL;
> @@ -64,6 +217,10 @@ static int vfio_migration_init(VFIODevice *vbasedev,
>           ret = -EINVAL;
>           goto err;
>       }
> +
> +    migration = vbasedev->migration;
> +    migration->vm_state = qemu_add_vm_change_state_handler(vfio_vmstate_change,
> +                                                           vbasedev);
>       return 0;
>   
>   err:
> 
> patch 06:
> 
> @@ -219,8 +243,11 @@ static int vfio_migration_init(VFIODevice *vbasedev,
>       }
>   
>       migration = vbasedev->migration;
> +    migration->vbasedev = vbasedev;
>       migration->vm_state = qemu_add_vm_change_state_handler(vfio_vmstate_change,
>                                                              vbasedev);
> +    migration->migration_state.notify = vfio_migration_state_notifier;
> +    add_migration_state_change_notifier(&migration->migration_state);
>       return 0;
>   
>   err:
> 
> If you're satisfied with that, no need to resend.  Thanks,
> 

Yes, this is exactly I was going to send.
Thanks for fixing it.

Thanks,
Kirti