Re: [Qemu-devel] [PATCH v7 06/13] vfio: Add VM state change handler to know state of VM

[Kirti Wankhede <kwankhede@nvidia.com>]

On 7/22/2019 1:53 PM, Yan Zhao wrote:
> On Fri, Jul 12, 2019 at 03:14:03AM +0800, Kirti Wankhede wrote:
>>
>>
>> On 7/11/2019 5:43 PM, Dr. David Alan Gilbert wrote:
>>> * Kirti Wankhede (kwankhede@nvidia.com) wrote:
>>>> VM state change handler gets called on change in VM's state. This is used to set
>>>> VFIO device state to _RUNNING.
>>>> VM state change handler, migration state change handler and log_sync listener
>>>> are called asynchronously, which sometimes lead to data corruption in migration
>>>> region. Initialised mutex that is used to serialize operations on migration data
>>>> region during saving state.
>>>>
>>>> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
>>>> Reviewed-by: Neo Jia <cjia@nvidia.com>
>>>> ---
>>>>  hw/vfio/migration.c           | 64 +++++++++++++++++++++++++++++++++++++++++++
>>>>  hw/vfio/trace-events          |  2 ++
>>>>  include/hw/vfio/vfio-common.h |  4 +++
>>>>  3 files changed, 70 insertions(+)
>>>>
>>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>>>> index a2cfbd5af2e1..c01f08b659d0 100644
>>>> --- a/hw/vfio/migration.c
>>>> +++ b/hw/vfio/migration.c
>>>> @@ -78,6 +78,60 @@ err:
>>>>      return ret;
>>>>  }
>>>>  
>>>> +static int vfio_migration_set_state(VFIODevice *vbasedev, uint32_t state)
>>>> +{
>>>> +    VFIOMigration *migration = vbasedev->migration;
>>>> +    VFIORegion *region = &migration->region.buffer;
>>>> +    uint32_t device_state;
>>>> +    int ret = 0;
>>>> +
>>>> +    device_state = (state & VFIO_DEVICE_STATE_MASK) |
>>>> +                   (vbasedev->device_state & ~VFIO_DEVICE_STATE_MASK);
>>>> +
>>>> +    if ((device_state & VFIO_DEVICE_STATE_MASK) == VFIO_DEVICE_STATE_INVALID) {
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    ret = pwrite(vbasedev->fd, &device_state, sizeof(device_state),
>>>> +                 region->fd_offset + offsetof(struct vfio_device_migration_info,
>>>> +                                              device_state));
>>>> +    if (ret < 0) {
>>>> +        error_report("%s: Failed to set device state %d %s",
>>>> +                     vbasedev->name, ret, strerror(errno));
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    vbasedev->device_state = device_state;
>>>> +    trace_vfio_migration_set_state(vbasedev->name, device_state);
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static void vfio_vmstate_change(void *opaque, int running, RunState state)
>>>> +{
>>>> +    VFIODevice *vbasedev = opaque;
>>>> +
>>>> +    if ((vbasedev->vm_running != running)) {
>>>> +        int ret;
>>>> +        uint32_t dev_state;
>>>> +
>>>> +        if (running) {
>>>> +            dev_state = VFIO_DEVICE_STATE_RUNNING;
>>>> +        } else {
>>>> +            dev_state = (vbasedev->device_state & VFIO_DEVICE_STATE_MASK) &
>>>> +                     ~VFIO_DEVICE_STATE_RUNNING;
>>>> +        }
>>>> +
>>>> +        ret = vfio_migration_set_state(vbasedev, dev_state);
>>>> +        if (ret) {
>>>> +            error_report("%s: Failed to set device state 0x%x",
>>>> +                         vbasedev->name, dev_state);
>>>> +        }
>>>> +        vbasedev->vm_running = running;
>>>> +        trace_vfio_vmstate_change(vbasedev->name, running, RunState_str(state),
>>>> +                                  dev_state);
>>>> +    }
>>>> +}
>>>> +
>>>>  static int vfio_migration_init(VFIODevice *vbasedev,
>>>>                                 struct vfio_region_info *info)
>>>>  {
>>>> @@ -93,6 +147,11 @@ static int vfio_migration_init(VFIODevice *vbasedev,
>>>>          return ret;
>>>>      }
>>>>  
>>>> +    qemu_mutex_init(&vbasedev->migration->lock);
>>>
>>> Does this and it's friend below belong in this patch?  As far as I can
>>> tell you init/deinit the lock here but don't use it which is strange.
>>>
>>
>> This lock is used in
>> 0009-vfio-Add-save-state-functions-to-SaveVMHandlers.patch and
>> 0011-vfio-Add-function-to-get-dirty-page-list.patch
>>
>> Hm. I'll move this init/deinit to patch 0009 in next iteration.
>>
>> Thanks,
>> Kirti
>>
> This lock is used to protect vfio_save_buffer and read dirty page,
> right?
> if data subregion and bitmap subregion do not reuse "data_offset" in
> vfio_device_migration_info.
> It seems that this lock can be avoided.
> 

Same migration region either trapped or mapped can be used for device
data and dirty page. Its not just "data_offset"

Thanks,
Kirti


> Thanks
> Yan
> 
> 
>>
>>> Dave
>>>
>>>> +    vbasedev->vm_state = qemu_add_vm_change_state_handler(vfio_vmstate_change,
>>>> +                                                          vbasedev);
>>>> +
>>>>      return 0;
>>>>  }
>>>>  
>>>> @@ -135,11 +194,16 @@ void vfio_migration_finalize(VFIODevice *vbasedev)
>>>>          return;
>>>>      }
>>>>  
>>>> +    if (vbasedev->vm_state) {
>>>> +        qemu_del_vm_change_state_handler(vbasedev->vm_state);
>>>> +    }
>>>> +
>>>>      if (vbasedev->migration_blocker) {
>>>>          migrate_del_blocker(vbasedev->migration_blocker);
>>>>          error_free(vbasedev->migration_blocker);
>>>>      }
>>>>  
>>>> +    qemu_mutex_destroy(&vbasedev->migration->lock);
>>>>      vfio_migration_region_exit(vbasedev);
>>>>      g_free(vbasedev->migration);
>>>>  }
>>>> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
>>>> index 191a726a1312..3d15bacd031a 100644
>>>> --- a/hw/vfio/trace-events
>>>> +++ b/hw/vfio/trace-events
>>>> @@ -146,3 +146,5 @@ vfio_display_edid_write_error(void) ""
>>>>  
>>>>  # migration.c
>>>>  vfio_migration_probe(char *name, uint32_t index) " (%s) Region %d"
>>>> +vfio_migration_set_state(char *name, uint32_t state) " (%s) state %d"
>>>> +vfio_vmstate_change(char *name, int running, const char *reason, uint32_t dev_state) " (%s) running %d reason %s device state %d"
>>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>>>> index 152da3f8d6f3..f6c70db3a9c1 100644
>>>> --- a/include/hw/vfio/vfio-common.h
>>>> +++ b/include/hw/vfio/vfio-common.h
>>>> @@ -29,6 +29,7 @@
>>>>  #ifdef CONFIG_LINUX
>>>>  #include <linux/vfio.h>
>>>>  #endif
>>>> +#include "sysemu/sysemu.h"
>>>>  
>>>>  #define VFIO_MSG_PREFIX "vfio %s: "
>>>>  
>>>> @@ -124,6 +125,9 @@ typedef struct VFIODevice {
>>>>      unsigned int flags;
>>>>      VFIOMigration *migration;
>>>>      Error *migration_blocker;
>>>> +    uint32_t device_state;
>>>> +    VMChangeStateEntry *vm_state;
>>>> +    int vm_running;
>>>>  } VFIODevice;
>>>>  
>>>>  struct VFIODeviceOps {
>>>> -- 
>>>> 2.7.0
>>>>
>>> --
>>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>>>