diff mbox series

[v7,06/13] vfio: Add VM state change handler to know state of VM

Message ID 1562665760-26158-7-git-send-email-kwankhede@nvidia.com
State New
Headers show
Series Add migration support for VFIO device | expand

Commit Message

Kirti Wankhede July 9, 2019, 9:49 a.m. UTC 
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(+)

Comments

Dr. David Alan Gilbert July 11, 2019, 12:13 p.m. UTC | #1 
* 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.

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
Kirti Wankhede July 11, 2019, 7:14 p.m. UTC | #2 
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


> 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
>
Alex Williamson July 16, 2019, 10:03 p.m. UTC | #3 
On Tue, 9 Jul 2019 15:19:13 +0530
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;

Do we need to re-read device_state after error?  We defined _SAVING |
_RESUMING as STATE_INVALID, is that only for user writes, ie. the
device can never transition to that state to indicate a fault?  I was
thinking that was one if its use cases.  Thanks,

Alex

> +    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);
> +
> +    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 {
Yan Zhao July 22, 2019, 8:23 a.m. UTC | #4 
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.

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
> >
Yan Zhao July 22, 2019, 8:37 a.m. UTC | #5 
On Tue, Jul 09, 2019 at 05:49:13PM +0800, Kirti Wankhede 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;
should be
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);
> +
> +    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
>
Kirti Wankhede Aug. 20, 2019, 8:31 p.m. UTC | #6 
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
>>>
Kirti Wankhede Aug. 20, 2019, 8:33 p.m. UTC | #7 
On 7/22/2019 2:07 PM, Yan Zhao wrote:
> On Tue, Jul 09, 2019 at 05:49:13PM +0800, Kirti Wankhede 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;
> should be
> dev_state |= VFIO_DEVICE_STATE_RUNNING; ?
> 

vfio_migration_set_state() takes case of ORing.

Thanks,
Kirti

>> +        } 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);
>> +
>> +    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
>>
Yan Zhao Aug. 23, 2019, 1:32 a.m. UTC | #8 
On Wed, Aug 21, 2019 at 04:33:50AM +0800, Kirti Wankhede wrote:
> 
> 
> On 7/22/2019 2:07 PM, Yan Zhao wrote:
> > On Tue, Jul 09, 2019 at 05:49:13PM +0800, Kirti Wankhede 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;
> > should be
> > dev_state |= VFIO_DEVICE_STATE_RUNNING; ?
> > 
> 
> vfio_migration_set_state() takes case of ORing.
>
if previous dev_state is VFIO_DEVICE_STATE_SAVING (without RUNNING), and
vfio_migration_set_state(VFIO_DEVICE_STATE_RUNNING) is called here, do
you mean vfio_migration_set_state() will change the device state to
VFIO_DEVICE_STATE_RUNNING | VFIO_DEVICE_STATE_SAVING ?

Thanks
Yan


> Thanks,
> Kirti
> 
> >> +        } 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);
> >> +
> >> +    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
> >>
> 
> -----------------------------------------------------------------------------------
> This email message is for the sole use of the intended recipient(s) and may contain
> confidential information.  Any unauthorized review, use, disclosure or distribution
> is prohibited.  If you are not the intended recipient, please contact the sender by
> reply email and destroy all copies of the original message.
> -----------------------------------------------------------------------------------
diff mbox series

Patch

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