Re: [PATCH V7 29/29] cpr: only-cpr-capable option

[Steven Sistare <steven.sistare@oracle.com>]

On 2/18/2022 4:43 AM, Guoyi Tu wrote:
> On Wed, 2021-12-22 at 11:05 -0800, Steve Sistare wrote:
>> Add the only-cpr-capable option, which causes qemu to exit with an
>> error
>> if any devices that are not capable of cpr are added.  This
>> guarantees that
>> a cpr-exec operation will not fail with an unsupported device error.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>>  MAINTAINERS             |  1 +
>>  chardev/char-socket.c   |  4 ++++
>>  hw/vfio/common.c        |  6 ++++++
>>  include/sysemu/sysemu.h |  1 +
>>  migration/migration.c   |  5 +++++
>>  qemu-options.hx         |  8 ++++++++
>>  softmmu/globals.c       |  1 +
>>  softmmu/physmem.c       |  5 +++++
>>  softmmu/vl.c            | 14 +++++++++++++-
>>  stubs/cpr.c             |  3 +++
>>  stubs/meson.build       |  1 +
>>  11 files changed, 48 insertions(+), 1 deletion(-)
>>  create mode 100644 stubs/cpr.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index feed239..af5abc3 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -2998,6 +2998,7 @@ F: migration/cpr.c
>>  F: qapi/cpr.json
>>  F: migration/cpr-state.c
>>  F: stubs/cpr-state.c
>> +F: stubs/cpr.c
>>  
>>  Record/replay
>>  M: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
>> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
>> index c111e17..a4513a7 100644
>> --- a/chardev/char-socket.c
>> +++ b/chardev/char-socket.c
>> @@ -34,6 +34,7 @@
>>  #include "qapi/clone-visitor.h"
>>  #include "qapi/qapi-visit-sockets.h"
>>  #include "qemu/yank.h"
>> +#include "sysemu/sysemu.h"
>>  
>>  #include "chardev/char-io.h"
>>  #include "chardev/char-socket.h"
>> @@ -1416,6 +1417,9 @@ static void qmp_chardev_open_socket(Chardev
>> *chr,
>>  
>>      if (!s->tls_creds && !s->is_websock) {
>>          qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_CPR);
>> +    } else if (only_cpr_capable) {
>> +        error_setg(errp, "error: socket %s is not cpr capable due to
>> %s option",
>> +                   chr->label, (s->tls_creds ? "TLS" :
>> "websocket"));
> 
> Should the error be ignored if reopen-on-cpr is set.

Yes!  Good catch, thanks.

>>      }
>>  
>>      /* be isn't opened until we get a connection */
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index f2b4a81..605ffbb 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -38,6 +38,7 @@
>>  #include "sysemu/kvm.h"
>>  #include "sysemu/reset.h"
>>  #include "sysemu/runstate.h"
>> +#include "sysemu/sysemu.h"
>>  #include "trace.h"
>>  #include "qapi/error.h"
>>  #include "migration/migration.h"
>> @@ -1923,12 +1924,17 @@ static void
>> vfio_put_address_space(VFIOAddressSpace *space)
>>  static int vfio_get_iommu_type(VFIOContainer *container,
>>                                 Error **errp)
>>  {
>> +    ERRP_GUARD();
>>      int iommu_types[] = { VFIO_TYPE1v2_IOMMU, VFIO_TYPE1_IOMMU,
>>                            VFIO_SPAPR_TCE_v2_IOMMU,
>> VFIO_SPAPR_TCE_IOMMU };
>>      int i;
>>  
>>      for (i = 0; i < ARRAY_SIZE(iommu_types); i++) {
>>          if (ioctl(container->fd, VFIO_CHECK_EXTENSION,
>> iommu_types[i])) {
>> +            if (only_cpr_capable && !vfio_is_cpr_capable(container,
>> errp)) {
>> +                error_prepend(errp, "only-cpr-capable is specified:
>> ");
>> +                return -EINVAL;
>> +            }
>>              return iommu_types[i];
>>          }
>>      }
>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>> index 8fae667..6241c20 100644
>> --- a/include/sysemu/sysemu.h
>> +++ b/include/sysemu/sysemu.h
>> @@ -9,6 +9,7 @@
>>  /* vl.c */
>>  
>>  extern int only_migratable;
>> +extern bool only_cpr_capable;
>>  extern const char *qemu_name;
>>  extern QemuUUID qemu_uuid;
>>  extern bool qemu_uuid_set;
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 3de11ae..f08db0d 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1257,6 +1257,11 @@ static bool migrate_caps_check(bool *cap_list,
>>          return false;
>>      }
>>  
>> +    if (cap_list[MIGRATION_CAPABILITY_X_COLO] && only_cpr_capable) {
>> +        error_setg(errp, "x-colo is not compatible with -only-cpr-
>> capable");
>> +        return false;
>> +    }
>> +
>>      return true;
>>  }
>>  
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 1859b55..0cbf2e3 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -4434,6 +4434,14 @@ SRST
>>      an unmigratable state.
>>  ERST
>>  
>> +DEF("only-cpr-capable", 0, QEMU_OPTION_only_cpr_capable, \
>> +    "-only-cpr-capable    allow only cpr capable devices\n",
>> QEMU_ARCH_ALL)
>> +SRST
>> +``-only-cpr-capable``
>> +    Only allow cpr capable devices, which guarantees that cpr-save
>> and
>> +    cpr-exec will not fail with an unsupported device error.
>> +ERST
>> +
>>  DEF("nodefaults", 0, QEMU_OPTION_nodefaults, \
>>      "-nodefaults     don't create default devices\n", QEMU_ARCH_ALL)
>>  SRST
>> diff --git a/softmmu/globals.c b/softmmu/globals.c
>> index 7d0fc81..a18fd8d 100644
>> --- a/softmmu/globals.c
>> +++ b/softmmu/globals.c
>> @@ -59,6 +59,7 @@ int boot_menu;
>>  bool boot_strict;
>>  uint8_t *boot_splash_filedata;
>>  int only_migratable; /* turn it off unless user states otherwise */
>> +bool only_cpr_capable;
>>  int icount_align_option;
>>  
>>  /* The bytes in qemu_uuid are in the order specified by RFC4122,
>> _not_ in the
>> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
>> index e227195..e7869f8 100644
>> --- a/softmmu/physmem.c
>> +++ b/softmmu/physmem.c
>> @@ -47,6 +47,7 @@
>>  #include "sysemu/dma.h"
>>  #include "sysemu/hostmem.h"
>>  #include "sysemu/hw_accel.h"
>> +#include "sysemu/sysemu.h"
>>  #include "sysemu/xen-mapcache.h"
>>  #include "trace/trace-root.h"
>>  
>> @@ -2010,6 +2011,10 @@ static void ram_block_add(RAMBlock *new_block,
>> Error **errp)
>>                  addr = file_ram_alloc(new_block, maxlen, mfd,
>>                                        false, false, 0, errp);
>>                  trace_anon_memfd_alloc(name, maxlen, addr, mfd);
>> +            } else if (only_cpr_capable) {
>> +                error_setg(errp,
>> +                    "only-cpr-capable requires -machine memfd-
>> alloc=on");
>> +                return;
>>              } else {
>>                  addr = qemu_anon_ram_alloc(maxlen, &mr->align,
>>                                             shared, noreserve);
>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>> index 4319e1a..f14e29e 100644
>> --- a/softmmu/vl.c
>> +++ b/softmmu/vl.c
>> @@ -2743,11 +2743,20 @@ void qmp_x_exit_preconfig(Error **errp)
>>      qemu_create_cli_devices();
>>      qemu_machine_creation_done();
>>  
>> +    if (only_cpr_capable && !qemu_chr_is_cpr_capable(errp)) {
>> +        ;    /* not reached due to error_fatal */
>> +    }
>> +
>>      if (loadvm) {
>>          load_snapshot(loadvm, NULL, false, NULL, &error_fatal);
>>      }
>>      if (replay_mode != REPLAY_MODE_NONE) {
>> -        replay_vmstate_init();
>> +        if (only_cpr_capable) {
>> +            error_setg(errp, "replay is not compatible with -only-
>> cpr-capable");
>> +            /* not reached due to error_fatal */
>> +        } else {
>> +            replay_vmstate_init();
>> +        }
>>      }
>>  
>>      if (incoming) {
>> @@ -3507,6 +3516,9 @@ void qemu_init(int argc, char **argv, char
>> **envp)
>>              case QEMU_OPTION_only_migratable:
>>                  only_migratable = 1;
>>                  break;
>> +            case QEMU_OPTION_only_cpr_capable:
>> +                only_cpr_capable = true;
>> +                break;
>>              case QEMU_OPTION_nodefaults:
>>                  has_defaults = 0;
>>                  break;
>> diff --git a/stubs/cpr.c b/stubs/cpr.c
>> new file mode 100644
>> index 0000000..aaa189e
>> --- /dev/null
>> +++ b/stubs/cpr.c
>> @@ -0,0 +1,3 @@
>> +#include "qemu/osdep.h"
>> +
>> +bool only_cpr_capable;
>> diff --git a/stubs/meson.build b/stubs/meson.build
>> index 9565c7d..4c9c4ea 100644
>> --- a/stubs/meson.build
>> +++ b/stubs/meson.build
>> @@ -4,6 +4,7 @@ stub_ss.add(files('blk-exp-close-all.c'))
>>  stub_ss.add(files('blockdev-close-all-bdrv-states.c'))
>>  stub_ss.add(files('change-state-handler.c'))
>>  stub_ss.add(files('cmos.c'))
>> +stub_ss.add(files('cpr.c'))
>>  stub_ss.add(files('cpr-state.c'))
>>  stub_ss.add(files('cpu-get-clock.c'))
>>  stub_ss.add(files('cpus-get-virtual-clock.c'))
> 
> The only-cpr-capable option is a good way to prevent qemu from starting
> if some device don't support cpr. But if this option is not provided,
> the user still can perform cpr-xxx operation even there are devices
> don't support cpr, in this case, the exec() will fail and the original
> process cannot recovery.
> 
> How about introducing a cpr blocker (as migration blocker does) to
> prevent the user from performing cpr-xxx operaton to address the
> problem

Sure.  I will add a call to qemu_chr_is_cpr_capable() in cpr_save().

Thanks very much for your careful review of the chardev patches.

- Steve