|
| From: | Avihai Horon |
| Subject: | Re: [PATCH v2 07/11] vfio/migration: Implement VFIO migration protocol v2 |
| Date: | Wed, 15 Jun 2022 09:40:25 +0300 |
| User-agent: | Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.9.1 |
On 6/14/2022 8:24 PM, Joao Martins wrote:
External email: Use caution opening links or attachments On 6/14/22 17:34, Avihai Horon wrote:On 6/14/2022 2:08 PM, Joao Martins wrote:External email: Use caution opening links or attachments On 5/30/22 18:07, Avihai Horon wrote:+static int vfio_save_complete_precopy(QEMUFile *f, void *opaque) +{ + VFIODevice *vbasedev = opaque; + enum vfio_device_mig_state recover_state; + int ret; + + /* We reach here with device state STOP or STOP_COPY only */ + recover_state = VFIO_DEVICE_STATE_STOP; + ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP_COPY, + recover_state); + if (ret) { + return ret; + } + + do { + ret = vfio_save_block(f, vbasedev->migration); + if (ret < 0) { + return ret; + } + } while (!ret); + + qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE); + ret = qemu_file_get_error(f); + if (ret) { + return ret; + } + + ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP, + recover_state);Is it expected that you are setting VFIO_DEVICE_STATE_STOP while @recover_state is the same value (VFIO_DEVICE_STATE_STOP) ?Yes. Transitioning to any other state from STOP_COPY will first go through STOP state (this is done internally by kernel). So there is no better option for the recover state but STOP.I was think about ERROR state given that you can transition there from any state, but wasn't quite sure if it's appropriate to make that arc while in stop copy migration phase.
Moving to ERROR is possible but it will just fail, triggering a hw_error() (and with following patch triggering a device reset). Failing to move to STOP recover state will go the same path - trigger a hw_error() and with following patch a device reset.
The only difference is that by moving to STOP recover state we try one more time to set it to STOP.
Probably it's a useless try, since we failed the first time.We can change the recover state to ERROR if it makes the code clearer and avoids the extra try.
+ if (ret) { + return ret; + } + + trace_vfio_save_complete_precopy(vbasedev->name); + + return 0;just a cosmetic nit: you could probably rewrite these last couple of lines as: if (!ret) { trace_vfio_save_complete_precopy(vbasedev->name); } return ret; Let's you avoid the double return path.
Ah thanks! Will change.
+} + static int vfio_v1_save_complete_precopy(QEMUFile *f, void *opaque) { VFIODevice *vbasedev = opaque; @@ -593,6 +775,14 @@ static void vfio_save_state(QEMUFile *f, void *opaque) } } +static int vfio_load_setup(QEMUFile *f, void *opaque) +{ + VFIODevice *vbasedev = opaque; + + return vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RESUMING, + vbasedev->migration->device_state); +} + static int vfio_v1_load_setup(QEMUFile *f, void *opaque) { VFIODevice *vbasedev = opaque; @@ -620,6 +810,15 @@ static int vfio_v1_load_setup(QEMUFile *f, void *opaque) return ret; } +static int vfio_load_cleanup(void *opaque) +{ + VFIODevice *vbasedev = opaque; + + vfio_migration_cleanup(vbasedev); + trace_vfio_load_cleanup(vbasedev->name); + return 0; +} + static int vfio_v1_load_cleanup(void *opaque) { VFIODevice *vbasedev = opaque; @@ -662,7 +861,11 @@ static int vfio_load_state(QEMUFile *f, void *opaque, int version_id) uint64_t data_size = qemu_get_be64(f); if (data_size) { - ret = vfio_v1_load_buffer(f, vbasedev, data_size); + if (vbasedev->migration->v2) { + ret = vfio_load_buffer(f, vbasedev, data_size); + } else { + ret = vfio_v1_load_buffer(f, vbasedev, data_size); + } if (ret < 0) { return ret; } @@ -683,6 +886,16 @@ static int vfio_load_state(QEMUFile *f, void *opaque, int version_id) return ret; } +static SaveVMHandlers savevm_vfio_handlers = { + .save_setup = vfio_save_setup, + .save_cleanup = vfio_save_cleanup, + .save_live_complete_precopy = vfio_save_complete_precopy, + .save_state = vfio_save_state, + .load_setup = vfio_load_setup, + .load_cleanup = vfio_load_cleanup, + .load_state = vfio_load_state, +}; + static SaveVMHandlers savevm_vfio_v1_handlers = { .save_setup = vfio_v1_save_setup, .save_cleanup = vfio_v1_save_cleanup, @@ -697,6 +910,34 @@ static SaveVMHandlers savevm_vfio_v1_handlers = { /* ---------------------------------------------------------------------- */ +static void vfio_vmstate_change(void *opaque, bool running, RunState state) +{ + VFIODevice *vbasedev = opaque; + enum vfio_device_mig_state new_state; + int ret; + + if (running) { + new_state = VFIO_DEVICE_STATE_RUNNING; + } else { + new_state = VFIO_DEVICE_STATE_STOP; + } + + ret = vfio_migration_set_state(vbasedev, new_state, + VFIO_DEVICE_STATE_ERROR); + if (ret) { + /* + * Migration should be aborted in this case, but vm_state_notify() + * currently does not support reporting failures. + */ + if (migrate_get_current()->to_dst_file) { + qemu_file_set_error(migrate_get_current()->to_dst_file, ret); + } + } + + trace_vfio_vmstate_change(vbasedev->name, running, RunState_str(state), + new_state); +} + static void vfio_v1_vmstate_change(void *opaque, bool running, RunState state) { VFIODevice *vbasedev = opaque; @@ -770,12 +1011,17 @@ static void vfio_migration_state_notifier(Notifier *notifier, void *data) case MIGRATION_STATUS_CANCELLED: case MIGRATION_STATUS_FAILED: bytes_transferred = 0; - ret = vfio_migration_v1_set_state(vbasedev, - ~(VFIO_DEVICE_STATE_V1_SAVING | - VFIO_DEVICE_STATE_V1_RESUMING), - VFIO_DEVICE_STATE_V1_RUNNING); - if (ret) { - error_report("%s: Failed to set state RUNNING", vbasedev->name); + if (migration->v2) { + vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RUNNING, + VFIO_DEVICE_STATE_ERROR);Perhaps you are discarding the error? Shouldn't it be: err = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RUNNING, VFIO_DEVICE_STATE_ERROR);+ } else { + ret = vfio_migration_v1_set_state(vbasedev, + ~(VFIO_DEVICE_STATE_V1_SAVING | + VFIO_DEVICE_STATE_V1_RESUMING), + VFIO_DEVICE_STATE_V1_RUNNING); + if (ret) { + error_report("%s: Failed to set state RUNNING", vbasedev->name); + }Perhaps this error_report and condition is in the wrong scope? Shouldn't it be more like this: if (migration->v2) { ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RUNNING, VFIO_DEVICE_STATE_ERROR); } else { ret = vfio_migration_v1_set_state(vbasedev, ~(VFIO_DEVICE_STATE_V1_SAVING | VFIO_DEVICE_STATE_V1_RESUMING), VFIO_DEVICE_STATE_V1_RUNNING); } if (ret) { error_report("%s: Failed to set state RUNNING", vbasedev->name); }It was intentionally discarded. The return value is used by v1 code to determine whether to print an error message or not. In v2 code the error message print is done inside vfio_migration_set_state(), so there is no need for the return value here.Oh yes, I forgot that other print.
| [Prev in Thread] | Current Thread | [Next in Thread] |