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.sist...@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.dovga...@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
