Re: [PATCH V2 11/11] vfio: allow cpr-reboot migration if suspended

[Steven Sistare]

On 1/16/2024 3:37 PM, Steven Sistare wrote:
> On 1/15/2024 2:33 AM, Peter Xu wrote:
>> On Fri, Jan 12, 2024 at 07:05:10AM -0800, Steve Sistare wrote:
>>> Allow cpr-reboot for vfio if the guest is in the suspended runstate.  The
>>> guest drivers' suspend methods flush outstanding requests and re-initialize
>>> the devices, and thus there is no device state to save and restore.  The
>>> user is responsible for suspending the guest before initiating cpr, such as
>>> by issuing guest-suspend-ram to the qemu guest agent.
>>>
>>> Relax the vfio blocker so it does not apply to cpr, and add a notifier that
>>> verifies the guest is suspended.  Skip dirty page tracking, which is N/A for
>>> cpr, to avoid ioctl errors.
>>>
>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>> ---
>>>  hw/vfio/common.c              |  2 +-
>>>  hw/vfio/cpr.c                 | 20 ++++++++++++++++++++
>>>  hw/vfio/migration.c           |  2 +-
>>>  include/hw/vfio/vfio-common.h |  1 +
>>>  migration/ram.c               |  9 +++++----
>>>  5 files changed, 28 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>> index 0b3352f..09af934 100644
>>> --- a/hw/vfio/common.c
>>> +++ b/hw/vfio/common.c
>>> @@ -128,7 +128,7 @@ int vfio_block_multiple_devices_migration(VFIODevice 
>>> *vbasedev, Error **errp)
>>>      error_setg(&multiple_devices_migration_blocker,
>>>                 "Multiple VFIO devices migration is supported only if all 
>>> of "
>>>                 "them support P2P migration");
>>> -    ret = migrate_add_blocker(&multiple_devices_migration_blocker, errp);
>>> +    ret = migrate_add_blocker_normal(&multiple_devices_migration_blocker, 
>>> errp);
>>>  
>>>      return ret;
>>>  }
>>> diff --git a/hw/vfio/cpr.c b/hw/vfio/cpr.c
>>> index bbd1c7a..9f4b1fe 100644
>>> --- a/hw/vfio/cpr.c
>>> +++ b/hw/vfio/cpr.c
>>> @@ -7,13 +7,33 @@
>>>  
>>>  #include "qemu/osdep.h"
>>>  #include "hw/vfio/vfio-common.h"
>>> +#include "migration/misc.h"
>>>  #include "qapi/error.h"
>>> +#include "sysemu/runstate.h"
>>> +
>>> +static int vfio_cpr_reboot_notifier(NotifierWithReturn *notifier,
>>> +                                    MigrationEvent *e, Error **errp)
>>> +{
>>> +    if (e->state == MIGRATION_STATUS_SETUP &&
>>> +        !runstate_check(RUN_STATE_SUSPENDED)) {
>>> +
>>> +        error_setg(errp,
>>> +            "VFIO device only supports cpr-reboot for runstate suspended");
>>> +
>>> +        return -1;
>>> +    }
>>
>> What happens if the guest is suspended during SETUP, but then quickly waked
>> up before CPR migration completes?
> 
> That is a management layer bug -- we told them the VM must be suspended.
> However, I will reject a wakeup request if migration is active and mode is 
> cpr.
> 
>>> +    return 0;
>>> +}
>>>  
>>>  int vfio_cpr_register_container(VFIOContainer *container, Error **errp)
>>>  {
>>> +    migration_add_notifier_mode(&container->cpr_reboot_notifier,
>>> +                                vfio_cpr_reboot_notifier,
>>> +                                MIG_MODE_CPR_REBOOT);
>>>      return 0;
>>>  }
>>>  
>>>  void vfio_cpr_unregister_container(VFIOContainer *container)
>>>  {
>>> +    migration_remove_notifier(&container->cpr_reboot_notifier);
>>>  }
>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>>> index 534fddf..488905d 100644
>>> --- a/hw/vfio/migration.c
>>> +++ b/hw/vfio/migration.c
>>> @@ -895,7 +895,7 @@ static int vfio_block_migration(VFIODevice *vbasedev, 
>>> Error *err, Error **errp)
>>>      vbasedev->migration_blocker = error_copy(err);
>>>      error_free(err);
>>>  
>>> -    return migrate_add_blocker(&vbasedev->migration_blocker, errp);
>>> +    return migrate_add_blocker_normal(&vbasedev->migration_blocker, errp);
>>>  }
>>>  
>>>  /* ---------------------------------------------------------------------- 
>>> */
>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>>> index 1add5b7..7a46e24 100644
>>> --- a/include/hw/vfio/vfio-common.h
>>> +++ b/include/hw/vfio/vfio-common.h
>>> @@ -78,6 +78,7 @@ struct VFIOGroup;
>>>  typedef struct VFIOContainer {
>>>      VFIOContainerBase bcontainer;
>>>      int fd; /* /dev/vfio/vfio, empowered by the attached groups */
>>> +    NotifierWithReturn cpr_reboot_notifier;
>>>      unsigned iommu_type;
>>>      QLIST_HEAD(, VFIOGroup) group_list;
>>>  } VFIOContainer;
>>> diff --git a/migration/ram.c b/migration/ram.c
>>> index 1923366..44ad324 100644
>>> --- a/migration/ram.c
>>> +++ b/migration/ram.c
>>> @@ -2392,8 +2392,8 @@ static void ram_save_cleanup(void *opaque)
>>>      RAMState **rsp = opaque;
>>>      RAMBlock *block;
>>>  
>>> -    /* We don't use dirty log with background snapshots */
>>> -    if (!migrate_background_snapshot()) {
>>> +    /* We don't use dirty log with background snapshots or cpr */
>>> +    if (!migrate_background_snapshot() && migrate_mode() == 
>>> MIG_MODE_NORMAL) {
>>
>> Same question here, on what happens if the user resumes the VM before
>> migration completes?  IIUC shared-ram is not required, then it means if
>> that happens the cpr migration image can contain corrupted data, and that
>> may be a problem.
>>
>> Background snapshot is special in that it relies on totally different
>> tracking facilities (userfault, rather than KVM_GET_DIRTY_LOG), so it
>> disabled dirty tracking completely.  I assume not the case for cpr.
>>
>> If cpr is not going to support that use case, IIUC it should fail that
>> system wakeup properly.  Or perhaps when CPR mode QEMU should instead
>> reject a wakeup?
> 
> Good catch, this hunk would break the non-vfio case where the guest can be
> running when migration is initiated.  I should narrow the logic to check for
> that:
> 
>     if (!migrate_background_snapshot() &&
>         (migrate_mode() == MIG_MODE_NORMAL || runstate_is_running()) {

Correction: I need to specifically check for the suspended state 
(RUNSTATE_SUSPENDED
or vm_was_suspended), since I will not reject "cont from stopped" requests.

- Steve

>         ... use dirty logging ...
> 
> That plus rejecting a wakeup request should be sufficient.
> 
>>>          /* caller have hold BQL or is in a bh, so there is
>>>           * no writing race against the migration bitmap
>>>           */
>>> @@ -2804,8 +2804,9 @@ static void ram_init_bitmaps(RAMState *rs)
>>>  
>>>      WITH_RCU_READ_LOCK_GUARD() {
>>>          ram_list_init_bitmaps();
>>> -        /* We don't use dirty log with background snapshots */
>>> -        if (!migrate_background_snapshot()) {
>>> +        /* We don't use dirty log with background snapshots or cpr */
>>> +        if (!migrate_background_snapshot() &&
>>> +            migrate_mode() == MIG_MODE_NORMAL) {
>>>              memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION);
>>>              migration_bitmap_sync_precopy(rs, false);
>>>          }
>>> -- 
>>> 1.8.3.1
>>>
>>