* [RFC V1 01/14] accel: encapsulate search state
2024-10-17 15:14 [RFC V1 00/14] precreate phase Steve Sistare
@ 2024-10-17 15:14 ` Steve Sistare
2024-10-21 20:03 ` Fabiano Rosas
2024-10-17 15:14 ` [RFC V1 02/14] accel: accel preinit function Steve Sistare
` (14 subsequent siblings)
15 siblings, 1 reply; 63+ messages in thread
From: Steve Sistare @ 2024-10-17 15:14 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster, Steve Sistare
Save the state of the search for a working machine accelerator in
the new structure AccelSearch. No functional change.
Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
system/vl.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/system/vl.c b/system/vl.c
index 7439916..8b345dd 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -2304,9 +2304,14 @@ static int accelerator_set_property(void *opaque,
return object_parse_property_opt(opaque, name, value, "accel", errp);
}
+typedef struct {
+ AccelState *accel;
+ bool init_failed;
+} AccelSearch;
+
static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
{
- bool *p_init_failed = opaque;
+ AccelSearch *acs = opaque;
const char *acc = qemu_opt_get(opts, "accel");
AccelClass *ac = accel_find(acc);
AccelState *accel;
@@ -2340,16 +2345,17 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
goto bad;
}
+ acs->accel = accel;
return 1;
bad:
- *p_init_failed = true;
+ acs->init_failed = true;
return 0;
}
static void configure_accelerators(const char *progname)
{
- bool init_failed = false;
+ AccelSearch acs = {0};
qemu_opts_foreach(qemu_find_opts("icount"),
do_configure_icount, NULL, &error_fatal);
@@ -2389,7 +2395,7 @@ static void configure_accelerators(const char *progname)
if (accel_find(*tmp)) {
qemu_opts_parse_noisily(qemu_find_opts("accel"), *tmp, true);
} else {
- init_failed = true;
+ acs.init_failed = true;
error_report("invalid accelerator %s", *tmp);
}
}
@@ -2402,15 +2408,15 @@ static void configure_accelerators(const char *progname)
}
if (!qemu_opts_foreach(qemu_find_opts("accel"),
- do_configure_accelerator, &init_failed, &error_fatal)) {
- if (!init_failed) {
+ do_configure_accelerator, &acs, &error_fatal)) {
+ if (!acs.init_failed) {
error_report("no accelerator found");
}
exit(1);
}
- if (init_failed && !qtest_chrdev) {
- error_report("falling back to %s", current_accel_name());
+ if (acs.init_failed && !qtest_chrdev) {
+ error_report("falling back to %s", ACCEL_GET_CLASS(acs.accel)->name);
}
if (icount_enabled() && !tcg_enabled()) {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 63+ messages in thread* Re: [RFC V1 01/14] accel: encapsulate search state
2024-10-17 15:14 ` [RFC V1 01/14] accel: encapsulate search state Steve Sistare
@ 2024-10-21 20:03 ` Fabiano Rosas
0 siblings, 0 replies; 63+ messages in thread
From: Fabiano Rosas @ 2024-10-21 20:03 UTC (permalink / raw)
To: Steve Sistare, qemu-devel
Cc: Peter Xu, David Hildenbrand, Marcel Apfelbaum, Eduardo Habkost,
Philippe Mathieu-Daude, Paolo Bonzini, Daniel P. Berrange,
Markus Armbruster, Steve Sistare
Steve Sistare <steven.sistare@oracle.com> writes:
> Save the state of the search for a working machine accelerator in
> the new structure AccelSearch. No functional change.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
^ permalink raw reply [flat|nested] 63+ messages in thread
* [RFC V1 02/14] accel: accel preinit function
2024-10-17 15:14 [RFC V1 00/14] precreate phase Steve Sistare
2024-10-17 15:14 ` [RFC V1 01/14] accel: encapsulate search state Steve Sistare
@ 2024-10-17 15:14 ` Steve Sistare
2024-10-17 15:26 ` Steven Sistare
` (2 more replies)
2024-10-17 15:14 ` [RFC V1 03/14] accel: split configure_accelerators Steve Sistare
` (13 subsequent siblings)
15 siblings, 3 replies; 63+ messages in thread
From: Steve Sistare @ 2024-10-17 15:14 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster, Steve Sistare
Extract the first part of the AccelState init_machine function into
a new function preinit, which can be called without knowing any
machine properties. For now call preinit and init_machine at the
same place, so no functional change.
Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
accel/accel-system.c | 6 +++++
accel/kvm/kvm-all.c | 58 +++++++++++++++++++++++++++++----------------
accel/xen/xen-all.c | 11 ++++++---
include/qemu/accel.h | 1 +
target/i386/nvmm/nvmm-all.c | 10 +++++++-
target/i386/whpx/whpx-all.c | 14 +++++++----
6 files changed, 70 insertions(+), 30 deletions(-)
diff --git a/accel/accel-system.c b/accel/accel-system.c
index f6c947d..fef6625 100644
--- a/accel/accel-system.c
+++ b/accel/accel-system.c
@@ -36,8 +36,14 @@ int accel_init_machine(AccelState *accel, MachineState *ms)
int ret;
ms->accelerator = accel;
*(acc->allowed) = true;
+ ret = acc->preinit(accel);
+ if (ret < 0) {
+ goto fail;
+ }
+
ret = acc->init_machine(ms);
if (ret < 0) {
+fail:
ms->accelerator = NULL;
*(acc->allowed) = false;
object_unref(OBJECT(accel));
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 905fb84..c7c6001 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2484,6 +2484,42 @@ static int kvm_setup_dirty_ring(KVMState *s)
return 0;
}
+static int kvm_preinit(AccelState *accel)
+{
+ int ret;
+ KVMState *s = KVM_STATE(accel);
+
+ s->fd = qemu_open_old(s->device ?: "/dev/kvm", O_RDWR);
+ if (s->fd == -1) {
+ error_report("Could not access KVM kernel module: %m");
+ ret = -errno;
+ goto err;
+ }
+
+ ret = kvm_ioctl(s, KVM_GET_API_VERSION, 0);
+ if (ret < KVM_API_VERSION) {
+ if (ret >= 0) {
+ ret = -EINVAL;
+ }
+ fprintf(stderr, "kvm version too old\n");
+ goto err;
+ }
+
+ if (ret > KVM_API_VERSION) {
+ ret = -EINVAL;
+ error_report("kvm version not supported");
+ goto err;
+ }
+ return 0;
+
+err:
+ assert(ret < 0);
+ if (s->fd != -1) {
+ close(s->fd);
+ }
+ return ret;
+}
+
static int kvm_init(MachineState *ms)
{
MachineClass *mc = MACHINE_GET_CLASS(ms);
@@ -2523,27 +2559,6 @@ static int kvm_init(MachineState *ms)
QTAILQ_INIT(&s->kvm_sw_breakpoints);
#endif
QLIST_INIT(&s->kvm_parked_vcpus);
- s->fd = qemu_open_old(s->device ?: "/dev/kvm", O_RDWR);
- if (s->fd == -1) {
- error_report("Could not access KVM kernel module: %m");
- ret = -errno;
- goto err;
- }
-
- ret = kvm_ioctl(s, KVM_GET_API_VERSION, 0);
- if (ret < KVM_API_VERSION) {
- if (ret >= 0) {
- ret = -EINVAL;
- }
- error_report("kvm version too old");
- goto err;
- }
-
- if (ret > KVM_API_VERSION) {
- ret = -EINVAL;
- error_report("kvm version not supported");
- goto err;
- }
kvm_supported_memory_attributes = kvm_check_extension(s, KVM_CAP_MEMORY_ATTRIBUTES);
kvm_guest_memfd_supported =
@@ -3891,6 +3906,7 @@ static void kvm_accel_class_init(ObjectClass *oc, void *data)
{
AccelClass *ac = ACCEL_CLASS(oc);
ac->name = "KVM";
+ ac->preinit = kvm_preinit;
ac->init_machine = kvm_init;
ac->has_memory = kvm_accel_has_memory;
ac->allowed = &kvm_allowed;
diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
index 0bdefce..dfcb90c 100644
--- a/accel/xen/xen-all.c
+++ b/accel/xen/xen-all.c
@@ -75,10 +75,8 @@ static void xen_setup_post(MachineState *ms, AccelState *accel)
}
}
-static int xen_init(MachineState *ms)
+static int xen_preinit(AccelState *accel)
{
- MachineClass *mc = MACHINE_GET_CLASS(ms);
-
xen_xc = xc_interface_open(0, 0, 0);
if (xen_xc == NULL) {
xen_pv_printf(NULL, 0, "can't open xen interface\n");
@@ -97,6 +95,12 @@ static int xen_init(MachineState *ms)
xc_interface_close(xen_xc);
return -1;
}
+ return 0;
+}
+
+static int xen_init(MachineState *ms)
+{
+ MachineClass *mc = MACHINE_GET_CLASS(ms);
/*
* The XenStore write would fail when running restricted so don't attempt
@@ -125,6 +129,7 @@ static void xen_accel_class_init(ObjectClass *oc, void *data)
};
ac->name = "Xen";
+ ac->preinit = xen_preinit;
ac->init_machine = xen_init;
ac->setup_post = xen_setup_post;
ac->allowed = &xen_allowed;
diff --git a/include/qemu/accel.h b/include/qemu/accel.h
index 972a849..6b3b1cf 100644
--- a/include/qemu/accel.h
+++ b/include/qemu/accel.h
@@ -37,6 +37,7 @@ typedef struct AccelClass {
/*< public >*/
const char *name;
+ int (*preinit)(AccelState *accel);
int (*init_machine)(MachineState *ms);
#ifndef CONFIG_USER_ONLY
void (*setup_post)(MachineState *ms, AccelState *accel);
diff --git a/target/i386/nvmm/nvmm-all.c b/target/i386/nvmm/nvmm-all.c
index 65768ac..ce858a0 100644
--- a/target/i386/nvmm/nvmm-all.c
+++ b/target/i386/nvmm/nvmm-all.c
@@ -1153,7 +1153,7 @@ static struct RAMBlockNotifier nvmm_ram_notifier = {
/* -------------------------------------------------------------------------- */
static int
-nvmm_accel_init(MachineState *ms)
+nvmm_accel_preinit(MachineState *ms)
{
int ret, err;
@@ -1178,6 +1178,13 @@ nvmm_accel_init(MachineState *ms)
error_report("NVMM: Wrong state size %u", qemu_mach.cap.state_size);
return -EPROGMISMATCH;
}
+ return 0;
+}
+
+static int
+nvmm_accel_init(MachineState *ms)
+{
+ int ret, err;
ret = nvmm_machine_create(&qemu_mach.mach);
if (ret == -1) {
@@ -1204,6 +1211,7 @@ nvmm_accel_class_init(ObjectClass *oc, void *data)
{
AccelClass *ac = ACCEL_CLASS(oc);
ac->name = "NVMM";
+ ac->preinit = nvmm_accel_preinit;
ac->init_machine = nvmm_accel_init;
ac->allowed = &nvmm_allowed;
}
diff --git a/target/i386/whpx/whpx-all.c b/target/i386/whpx/whpx-all.c
index a6674a8..50bfc19 100644
--- a/target/i386/whpx/whpx-all.c
+++ b/target/i386/whpx/whpx-all.c
@@ -2516,6 +2516,14 @@ static void whpx_set_kernel_irqchip(Object *obj, Visitor *v,
* Partition support
*/
+static int whpx_accel_preinit(AccelState *accel)
+{
+ if (!init_whp_dispatch()) {
+ return -ENOSYS;
+ }
+ return 0;
+}
+
static int whpx_accel_init(MachineState *ms)
{
struct whpx_state *whpx;
@@ -2529,11 +2537,6 @@ static int whpx_accel_init(MachineState *ms)
whpx = &whpx_global;
- if (!init_whp_dispatch()) {
- ret = -ENOSYS;
- goto error;
- }
-
whpx->mem_quota = ms->ram_size;
hr = whp_dispatch.WHvGetCapability(
@@ -2713,6 +2716,7 @@ static void whpx_accel_class_init(ObjectClass *oc, void *data)
{
AccelClass *ac = ACCEL_CLASS(oc);
ac->name = "WHPX";
+ ac->preinit = whpx_accel_preinit;
ac->init_machine = whpx_accel_init;
ac->allowed = &whpx_allowed;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 63+ messages in thread* Re: [RFC V1 02/14] accel: accel preinit function
2024-10-17 15:14 ` [RFC V1 02/14] accel: accel preinit function Steve Sistare
@ 2024-10-17 15:26 ` Steven Sistare
2024-10-21 14:54 ` Peter Xu
2024-10-23 15:53 ` Paolo Bonzini
2 siblings, 0 replies; 63+ messages in thread
From: Steven Sistare @ 2024-10-17 15:26 UTC (permalink / raw)
To: Reinoud Zandijk, Edgar E. Iglesias, Stefano Stabellini,
Anthony PERARD, Paul Durrant, Sunil Muthuswamy
Cc: Peter Xu, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster, qemu-devel
cc Xen, whpx, and nvmm accelerator maintainers for this accelerator-specific patch.
The cover letter for this series is here:
https://lore.kernel.org/qemu-devel/1729178055-207271-1-git-send-email-steven.sistare@oracle.com
- Steve
On 10/17/2024 11:14 AM, Steve Sistare wrote:
> Extract the first part of the AccelState init_machine function into
> a new function preinit, which can be called without knowing any
> machine properties. For now call preinit and init_machine at the
> same place, so no functional change.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
> accel/accel-system.c | 6 +++++
> accel/kvm/kvm-all.c | 58 +++++++++++++++++++++++++++++----------------
> accel/xen/xen-all.c | 11 ++++++---
> include/qemu/accel.h | 1 +
> target/i386/nvmm/nvmm-all.c | 10 +++++++-
> target/i386/whpx/whpx-all.c | 14 +++++++----
> 6 files changed, 70 insertions(+), 30 deletions(-)
>
> diff --git a/accel/accel-system.c b/accel/accel-system.c
> index f6c947d..fef6625 100644
> --- a/accel/accel-system.c
> +++ b/accel/accel-system.c
> @@ -36,8 +36,14 @@ int accel_init_machine(AccelState *accel, MachineState *ms)
> int ret;
> ms->accelerator = accel;
> *(acc->allowed) = true;
> + ret = acc->preinit(accel);
> + if (ret < 0) {
> + goto fail;
> + }
> +
> ret = acc->init_machine(ms);
> if (ret < 0) {
> +fail:
> ms->accelerator = NULL;
> *(acc->allowed) = false;
> object_unref(OBJECT(accel));
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 905fb84..c7c6001 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2484,6 +2484,42 @@ static int kvm_setup_dirty_ring(KVMState *s)
> return 0;
> }
>
> +static int kvm_preinit(AccelState *accel)
> +{
> + int ret;
> + KVMState *s = KVM_STATE(accel);
> +
> + s->fd = qemu_open_old(s->device ?: "/dev/kvm", O_RDWR);
> + if (s->fd == -1) {
> + error_report("Could not access KVM kernel module: %m");
> + ret = -errno;
> + goto err;
> + }
> +
> + ret = kvm_ioctl(s, KVM_GET_API_VERSION, 0);
> + if (ret < KVM_API_VERSION) {
> + if (ret >= 0) {
> + ret = -EINVAL;
> + }
> + fprintf(stderr, "kvm version too old\n");
> + goto err;
> + }
> +
> + if (ret > KVM_API_VERSION) {
> + ret = -EINVAL;
> + error_report("kvm version not supported");
> + goto err;
> + }
> + return 0;
> +
> +err:
> + assert(ret < 0);
> + if (s->fd != -1) {
> + close(s->fd);
> + }
> + return ret;
> +}
> +
> static int kvm_init(MachineState *ms)
> {
> MachineClass *mc = MACHINE_GET_CLASS(ms);
> @@ -2523,27 +2559,6 @@ static int kvm_init(MachineState *ms)
> QTAILQ_INIT(&s->kvm_sw_breakpoints);
> #endif
> QLIST_INIT(&s->kvm_parked_vcpus);
> - s->fd = qemu_open_old(s->device ?: "/dev/kvm", O_RDWR);
> - if (s->fd == -1) {
> - error_report("Could not access KVM kernel module: %m");
> - ret = -errno;
> - goto err;
> - }
> -
> - ret = kvm_ioctl(s, KVM_GET_API_VERSION, 0);
> - if (ret < KVM_API_VERSION) {
> - if (ret >= 0) {
> - ret = -EINVAL;
> - }
> - error_report("kvm version too old");
> - goto err;
> - }
> -
> - if (ret > KVM_API_VERSION) {
> - ret = -EINVAL;
> - error_report("kvm version not supported");
> - goto err;
> - }
>
> kvm_supported_memory_attributes = kvm_check_extension(s, KVM_CAP_MEMORY_ATTRIBUTES);
> kvm_guest_memfd_supported =
> @@ -3891,6 +3906,7 @@ static void kvm_accel_class_init(ObjectClass *oc, void *data)
> {
> AccelClass *ac = ACCEL_CLASS(oc);
> ac->name = "KVM";
> + ac->preinit = kvm_preinit;
> ac->init_machine = kvm_init;
> ac->has_memory = kvm_accel_has_memory;
> ac->allowed = &kvm_allowed;
> diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
> index 0bdefce..dfcb90c 100644
> --- a/accel/xen/xen-all.c
> +++ b/accel/xen/xen-all.c
> @@ -75,10 +75,8 @@ static void xen_setup_post(MachineState *ms, AccelState *accel)
> }
> }
>
> -static int xen_init(MachineState *ms)
> +static int xen_preinit(AccelState *accel)
> {
> - MachineClass *mc = MACHINE_GET_CLASS(ms);
> -
> xen_xc = xc_interface_open(0, 0, 0);
> if (xen_xc == NULL) {
> xen_pv_printf(NULL, 0, "can't open xen interface\n");
> @@ -97,6 +95,12 @@ static int xen_init(MachineState *ms)
> xc_interface_close(xen_xc);
> return -1;
> }
> + return 0;
> +}
> +
> +static int xen_init(MachineState *ms)
> +{
> + MachineClass *mc = MACHINE_GET_CLASS(ms);
>
> /*
> * The XenStore write would fail when running restricted so don't attempt
> @@ -125,6 +129,7 @@ static void xen_accel_class_init(ObjectClass *oc, void *data)
> };
>
> ac->name = "Xen";
> + ac->preinit = xen_preinit;
> ac->init_machine = xen_init;
> ac->setup_post = xen_setup_post;
> ac->allowed = &xen_allowed;
> diff --git a/include/qemu/accel.h b/include/qemu/accel.h
> index 972a849..6b3b1cf 100644
> --- a/include/qemu/accel.h
> +++ b/include/qemu/accel.h
> @@ -37,6 +37,7 @@ typedef struct AccelClass {
> /*< public >*/
>
> const char *name;
> + int (*preinit)(AccelState *accel);
> int (*init_machine)(MachineState *ms);
> #ifndef CONFIG_USER_ONLY
> void (*setup_post)(MachineState *ms, AccelState *accel);
> diff --git a/target/i386/nvmm/nvmm-all.c b/target/i386/nvmm/nvmm-all.c
> index 65768ac..ce858a0 100644
> --- a/target/i386/nvmm/nvmm-all.c
> +++ b/target/i386/nvmm/nvmm-all.c
> @@ -1153,7 +1153,7 @@ static struct RAMBlockNotifier nvmm_ram_notifier = {
> /* -------------------------------------------------------------------------- */
>
> static int
> -nvmm_accel_init(MachineState *ms)
> +nvmm_accel_preinit(MachineState *ms)
> {
> int ret, err;
>
> @@ -1178,6 +1178,13 @@ nvmm_accel_init(MachineState *ms)
> error_report("NVMM: Wrong state size %u", qemu_mach.cap.state_size);
> return -EPROGMISMATCH;
> }
> + return 0;
> +}
> +
> +static int
> +nvmm_accel_init(MachineState *ms)
> +{
> + int ret, err;
>
> ret = nvmm_machine_create(&qemu_mach.mach);
> if (ret == -1) {
> @@ -1204,6 +1211,7 @@ nvmm_accel_class_init(ObjectClass *oc, void *data)
> {
> AccelClass *ac = ACCEL_CLASS(oc);
> ac->name = "NVMM";
> + ac->preinit = nvmm_accel_preinit;
> ac->init_machine = nvmm_accel_init;
> ac->allowed = &nvmm_allowed;
> }
> diff --git a/target/i386/whpx/whpx-all.c b/target/i386/whpx/whpx-all.c
> index a6674a8..50bfc19 100644
> --- a/target/i386/whpx/whpx-all.c
> +++ b/target/i386/whpx/whpx-all.c
> @@ -2516,6 +2516,14 @@ static void whpx_set_kernel_irqchip(Object *obj, Visitor *v,
> * Partition support
> */
>
> +static int whpx_accel_preinit(AccelState *accel)
> +{
> + if (!init_whp_dispatch()) {
> + return -ENOSYS;
> + }
> + return 0;
> +}
> +
> static int whpx_accel_init(MachineState *ms)
> {
> struct whpx_state *whpx;
> @@ -2529,11 +2537,6 @@ static int whpx_accel_init(MachineState *ms)
>
> whpx = &whpx_global;
>
> - if (!init_whp_dispatch()) {
> - ret = -ENOSYS;
> - goto error;
> - }
> -
> whpx->mem_quota = ms->ram_size;
>
> hr = whp_dispatch.WHvGetCapability(
> @@ -2713,6 +2716,7 @@ static void whpx_accel_class_init(ObjectClass *oc, void *data)
> {
> AccelClass *ac = ACCEL_CLASS(oc);
> ac->name = "WHPX";
> + ac->preinit = whpx_accel_preinit;
> ac->init_machine = whpx_accel_init;
> ac->allowed = &whpx_allowed;
>
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [RFC V1 02/14] accel: accel preinit function
2024-10-17 15:14 ` [RFC V1 02/14] accel: accel preinit function Steve Sistare
2024-10-17 15:26 ` Steven Sistare
@ 2024-10-21 14:54 ` Peter Xu
2024-10-23 16:13 ` Steven Sistare
2024-10-23 15:53 ` Paolo Bonzini
2 siblings, 1 reply; 63+ messages in thread
From: Peter Xu @ 2024-10-21 14:54 UTC (permalink / raw)
To: Steve Sistare
Cc: qemu-devel, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster
On Thu, Oct 17, 2024 at 08:14:03AM -0700, Steve Sistare wrote:
> Extract the first part of the AccelState init_machine function into
> a new function preinit, which can be called without knowing any
> machine properties. For now call preinit and init_machine at the
> same place, so no functional change.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
> accel/accel-system.c | 6 +++++
> accel/kvm/kvm-all.c | 58 +++++++++++++++++++++++++++++----------------
> accel/xen/xen-all.c | 11 ++++++---
> include/qemu/accel.h | 1 +
> target/i386/nvmm/nvmm-all.c | 10 +++++++-
> target/i386/whpx/whpx-all.c | 14 +++++++----
> 6 files changed, 70 insertions(+), 30 deletions(-)
>
> diff --git a/accel/accel-system.c b/accel/accel-system.c
> index f6c947d..fef6625 100644
> --- a/accel/accel-system.c
> +++ b/accel/accel-system.c
> @@ -36,8 +36,14 @@ int accel_init_machine(AccelState *accel, MachineState *ms)
> int ret;
> ms->accelerator = accel;
> *(acc->allowed) = true;
> + ret = acc->preinit(accel);
> + if (ret < 0) {
> + goto fail;
> + }
> +
> ret = acc->init_machine(ms);
> if (ret < 0) {
> +fail:
Jump into another failure path's if clause might be error prone in the
future.
Maybe move the error handling out while at it, e.g.:
ret = acc->init_machine();
if (ret < 0) {
goto fail;
}
object_set_accelerator_compat_props(acc->compat_props);
return 0;
fail:
ms->accelerator = NULL;
*(acc->allowed) = false;
object_unref(OBJECT(accel));
return ret;
> ms->accelerator = NULL;
> *(acc->allowed) = false;
> object_unref(OBJECT(accel));
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 905fb84..c7c6001 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2484,6 +2484,42 @@ static int kvm_setup_dirty_ring(KVMState *s)
> return 0;
> }
>
> +static int kvm_preinit(AccelState *accel)
> +{
> + int ret;
> + KVMState *s = KVM_STATE(accel);
> +
> + s->fd = qemu_open_old(s->device ?: "/dev/kvm", O_RDWR);
> + if (s->fd == -1) {
> + error_report("Could not access KVM kernel module: %m");
> + ret = -errno;
> + goto err;
> + }
> +
> + ret = kvm_ioctl(s, KVM_GET_API_VERSION, 0);
> + if (ret < KVM_API_VERSION) {
> + if (ret >= 0) {
> + ret = -EINVAL;
> + }
> + fprintf(stderr, "kvm version too old\n");
> + goto err;
> + }
> +
> + if (ret > KVM_API_VERSION) {
> + ret = -EINVAL;
> + error_report("kvm version not supported");
> + goto err;
> + }
> + return 0;
> +
> +err:
> + assert(ret < 0);
> + if (s->fd != -1) {
> + close(s->fd);
> + }
> + return ret;
> +}
> +
> static int kvm_init(MachineState *ms)
> {
> MachineClass *mc = MACHINE_GET_CLASS(ms);
> @@ -2523,27 +2559,6 @@ static int kvm_init(MachineState *ms)
> QTAILQ_INIT(&s->kvm_sw_breakpoints);
> #endif
> QLIST_INIT(&s->kvm_parked_vcpus);
> - s->fd = qemu_open_old(s->device ?: "/dev/kvm", O_RDWR);
> - if (s->fd == -1) {
> - error_report("Could not access KVM kernel module: %m");
> - ret = -errno;
> - goto err;
> - }
> -
> - ret = kvm_ioctl(s, KVM_GET_API_VERSION, 0);
> - if (ret < KVM_API_VERSION) {
> - if (ret >= 0) {
> - ret = -EINVAL;
> - }
> - error_report("kvm version too old");
> - goto err;
> - }
> -
> - if (ret > KVM_API_VERSION) {
> - ret = -EINVAL;
> - error_report("kvm version not supported");
> - goto err;
> - }
>
> kvm_supported_memory_attributes = kvm_check_extension(s, KVM_CAP_MEMORY_ATTRIBUTES);
> kvm_guest_memfd_supported =
> @@ -3891,6 +3906,7 @@ static void kvm_accel_class_init(ObjectClass *oc, void *data)
> {
> AccelClass *ac = ACCEL_CLASS(oc);
> ac->name = "KVM";
> + ac->preinit = kvm_preinit;
> ac->init_machine = kvm_init;
> ac->has_memory = kvm_accel_has_memory;
> ac->allowed = &kvm_allowed;
> diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
> index 0bdefce..dfcb90c 100644
> --- a/accel/xen/xen-all.c
> +++ b/accel/xen/xen-all.c
> @@ -75,10 +75,8 @@ static void xen_setup_post(MachineState *ms, AccelState *accel)
> }
> }
>
> -static int xen_init(MachineState *ms)
> +static int xen_preinit(AccelState *accel)
> {
> - MachineClass *mc = MACHINE_GET_CLASS(ms);
> -
> xen_xc = xc_interface_open(0, 0, 0);
> if (xen_xc == NULL) {
> xen_pv_printf(NULL, 0, "can't open xen interface\n");
> @@ -97,6 +95,12 @@ static int xen_init(MachineState *ms)
> xc_interface_close(xen_xc);
> return -1;
> }
> + return 0;
> +}
> +
> +static int xen_init(MachineState *ms)
> +{
> + MachineClass *mc = MACHINE_GET_CLASS(ms);
>
> /*
> * The XenStore write would fail when running restricted so don't attempt
> @@ -125,6 +129,7 @@ static void xen_accel_class_init(ObjectClass *oc, void *data)
> };
>
> ac->name = "Xen";
> + ac->preinit = xen_preinit;
> ac->init_machine = xen_init;
> ac->setup_post = xen_setup_post;
> ac->allowed = &xen_allowed;
> diff --git a/include/qemu/accel.h b/include/qemu/accel.h
> index 972a849..6b3b1cf 100644
> --- a/include/qemu/accel.h
> +++ b/include/qemu/accel.h
> @@ -37,6 +37,7 @@ typedef struct AccelClass {
> /*< public >*/
>
> const char *name;
> + int (*preinit)(AccelState *accel);
> int (*init_machine)(MachineState *ms);
Might be nice to add some comment on what should be part of preinit() and
what should be part of init_machine().
AFAIU the preinit() was about probing whether an accel is supported. Maybe
rename it to probe()? Then it's also clear why some accel (e.g. tcg)
doesn't need that, because it is always supported and no probe is needed.
> #ifndef CONFIG_USER_ONLY
> void (*setup_post)(MachineState *ms, AccelState *accel);
> diff --git a/target/i386/nvmm/nvmm-all.c b/target/i386/nvmm/nvmm-all.c
> index 65768ac..ce858a0 100644
> --- a/target/i386/nvmm/nvmm-all.c
> +++ b/target/i386/nvmm/nvmm-all.c
> @@ -1153,7 +1153,7 @@ static struct RAMBlockNotifier nvmm_ram_notifier = {
> /* -------------------------------------------------------------------------- */
>
> static int
> -nvmm_accel_init(MachineState *ms)
> +nvmm_accel_preinit(MachineState *ms)
> {
> int ret, err;
>
> @@ -1178,6 +1178,13 @@ nvmm_accel_init(MachineState *ms)
> error_report("NVMM: Wrong state size %u", qemu_mach.cap.state_size);
> return -EPROGMISMATCH;
> }
> + return 0;
> +}
> +
> +static int
> +nvmm_accel_init(MachineState *ms)
> +{
> + int ret, err;
>
> ret = nvmm_machine_create(&qemu_mach.mach);
> if (ret == -1) {
> @@ -1204,6 +1211,7 @@ nvmm_accel_class_init(ObjectClass *oc, void *data)
> {
> AccelClass *ac = ACCEL_CLASS(oc);
> ac->name = "NVMM";
> + ac->preinit = nvmm_accel_preinit;
> ac->init_machine = nvmm_accel_init;
> ac->allowed = &nvmm_allowed;
> }
> diff --git a/target/i386/whpx/whpx-all.c b/target/i386/whpx/whpx-all.c
> index a6674a8..50bfc19 100644
> --- a/target/i386/whpx/whpx-all.c
> +++ b/target/i386/whpx/whpx-all.c
> @@ -2516,6 +2516,14 @@ static void whpx_set_kernel_irqchip(Object *obj, Visitor *v,
> * Partition support
> */
>
> +static int whpx_accel_preinit(AccelState *accel)
> +{
> + if (!init_whp_dispatch()) {
> + return -ENOSYS;
> + }
> + return 0;
> +}
> +
> static int whpx_accel_init(MachineState *ms)
> {
> struct whpx_state *whpx;
> @@ -2529,11 +2537,6 @@ static int whpx_accel_init(MachineState *ms)
>
> whpx = &whpx_global;
>
> - if (!init_whp_dispatch()) {
> - ret = -ENOSYS;
> - goto error;
> - }
> -
> whpx->mem_quota = ms->ram_size;
>
> hr = whp_dispatch.WHvGetCapability(
> @@ -2713,6 +2716,7 @@ static void whpx_accel_class_init(ObjectClass *oc, void *data)
> {
> AccelClass *ac = ACCEL_CLASS(oc);
> ac->name = "WHPX";
> + ac->preinit = whpx_accel_preinit;
> ac->init_machine = whpx_accel_init;
> ac->allowed = &whpx_allowed;
>
> --
> 1.8.3.1
>
--
Peter Xu
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [RFC V1 02/14] accel: accel preinit function
2024-10-21 14:54 ` Peter Xu
@ 2024-10-23 16:13 ` Steven Sistare
0 siblings, 0 replies; 63+ messages in thread
From: Steven Sistare @ 2024-10-23 16:13 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-devel, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster
On 10/21/2024 10:54 AM, Peter Xu wrote:
> On Thu, Oct 17, 2024 at 08:14:03AM -0700, Steve Sistare wrote:
>> Extract the first part of the AccelState init_machine function into
>> a new function preinit, which can be called without knowing any
>> machine properties. For now call preinit and init_machine at the
>> same place, so no functional change.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>> accel/accel-system.c | 6 +++++
>> accel/kvm/kvm-all.c | 58 +++++++++++++++++++++++++++++----------------
>> accel/xen/xen-all.c | 11 ++++++---
>> include/qemu/accel.h | 1 +
>> target/i386/nvmm/nvmm-all.c | 10 +++++++-
>> target/i386/whpx/whpx-all.c | 14 +++++++----
>> 6 files changed, 70 insertions(+), 30 deletions(-)
>>
>> diff --git a/accel/accel-system.c b/accel/accel-system.c
>> index f6c947d..fef6625 100644
>> --- a/accel/accel-system.c
>> +++ b/accel/accel-system.c
>> @@ -36,8 +36,14 @@ int accel_init_machine(AccelState *accel, MachineState *ms)
>> int ret;
>> ms->accelerator = accel;
>> *(acc->allowed) = true;
>> + ret = acc->preinit(accel);
>> + if (ret < 0) {
>> + goto fail;
>> + }
>> +
>> ret = acc->init_machine(ms);
>> if (ret < 0) {
>> +fail:
>
> Jump into another failure path's if clause might be error prone in the
> future.
I agree this is ugly, but the diff is small and easy to understand, and patch 3
makes it pretty again:
@@ -36,14 +36,8 @@ int accel_init_machine(AccelState *accel, MachineState *ms)
int ret;
ms->accelerator = accel;
*(acc->allowed) = true;
- ret = acc->preinit(accel);
- if (ret < 0) {
- goto fail;
- }
-
ret = acc->init_machine(ms);
if (ret < 0) {
-fail:
ms->accelerator = NULL;
- Steve
> Maybe move the error handling out while at it, e.g.:
>
> ret = acc->init_machine();
> if (ret < 0) {
> goto fail;
> }
> object_set_accelerator_compat_props(acc->compat_props);
> return 0;
>
> fail:
> ms->accelerator = NULL;
> *(acc->allowed) = false;
> object_unref(OBJECT(accel));
> return ret;
>
>> ms->accelerator = NULL;
>> *(acc->allowed) = false;
>> object_unref(OBJECT(accel));
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index 905fb84..c7c6001 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -2484,6 +2484,42 @@ static int kvm_setup_dirty_ring(KVMState *s)
>> return 0;
>> }
>>
>> +static int kvm_preinit(AccelState *accel)
>> +{
>> + int ret;
>> + KVMState *s = KVM_STATE(accel);
>> +
>> + s->fd = qemu_open_old(s->device ?: "/dev/kvm", O_RDWR);
>> + if (s->fd == -1) {
>> + error_report("Could not access KVM kernel module: %m");
>> + ret = -errno;
>> + goto err;
>> + }
>> +
>> + ret = kvm_ioctl(s, KVM_GET_API_VERSION, 0);
>> + if (ret < KVM_API_VERSION) {
>> + if (ret >= 0) {
>> + ret = -EINVAL;
>> + }
>> + fprintf(stderr, "kvm version too old\n");
>> + goto err;
>> + }
>> +
>> + if (ret > KVM_API_VERSION) {
>> + ret = -EINVAL;
>> + error_report("kvm version not supported");
>> + goto err;
>> + }
>> + return 0;
>> +
>> +err:
>> + assert(ret < 0);
>> + if (s->fd != -1) {
>> + close(s->fd);
>> + }
>> + return ret;
>> +}
>> +
>> static int kvm_init(MachineState *ms)
>> {
>> MachineClass *mc = MACHINE_GET_CLASS(ms);
>> @@ -2523,27 +2559,6 @@ static int kvm_init(MachineState *ms)
>> QTAILQ_INIT(&s->kvm_sw_breakpoints);
>> #endif
>> QLIST_INIT(&s->kvm_parked_vcpus);
>> - s->fd = qemu_open_old(s->device ?: "/dev/kvm", O_RDWR);
>> - if (s->fd == -1) {
>> - error_report("Could not access KVM kernel module: %m");
>> - ret = -errno;
>> - goto err;
>> - }
>> -
>> - ret = kvm_ioctl(s, KVM_GET_API_VERSION, 0);
>> - if (ret < KVM_API_VERSION) {
>> - if (ret >= 0) {
>> - ret = -EINVAL;
>> - }
>> - error_report("kvm version too old");
>> - goto err;
>> - }
>> -
>> - if (ret > KVM_API_VERSION) {
>> - ret = -EINVAL;
>> - error_report("kvm version not supported");
>> - goto err;
>> - }
>>
>> kvm_supported_memory_attributes = kvm_check_extension(s, KVM_CAP_MEMORY_ATTRIBUTES);
>> kvm_guest_memfd_supported =
>> @@ -3891,6 +3906,7 @@ static void kvm_accel_class_init(ObjectClass *oc, void *data)
>> {
>> AccelClass *ac = ACCEL_CLASS(oc);
>> ac->name = "KVM";
>> + ac->preinit = kvm_preinit;
>> ac->init_machine = kvm_init;
>> ac->has_memory = kvm_accel_has_memory;
>> ac->allowed = &kvm_allowed;
>> diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
>> index 0bdefce..dfcb90c 100644
>> --- a/accel/xen/xen-all.c
>> +++ b/accel/xen/xen-all.c
>> @@ -75,10 +75,8 @@ static void xen_setup_post(MachineState *ms, AccelState *accel)
>> }
>> }
>>
>> -static int xen_init(MachineState *ms)
>> +static int xen_preinit(AccelState *accel)
>> {
>> - MachineClass *mc = MACHINE_GET_CLASS(ms);
>> -
>> xen_xc = xc_interface_open(0, 0, 0);
>> if (xen_xc == NULL) {
>> xen_pv_printf(NULL, 0, "can't open xen interface\n");
>> @@ -97,6 +95,12 @@ static int xen_init(MachineState *ms)
>> xc_interface_close(xen_xc);
>> return -1;
>> }
>> + return 0;
>> +}
>> +
>> +static int xen_init(MachineState *ms)
>> +{
>> + MachineClass *mc = MACHINE_GET_CLASS(ms);
>>
>> /*
>> * The XenStore write would fail when running restricted so don't attempt
>> @@ -125,6 +129,7 @@ static void xen_accel_class_init(ObjectClass *oc, void *data)
>> };
>>
>> ac->name = "Xen";
>> + ac->preinit = xen_preinit;
>> ac->init_machine = xen_init;
>> ac->setup_post = xen_setup_post;
>> ac->allowed = &xen_allowed;
>> diff --git a/include/qemu/accel.h b/include/qemu/accel.h
>> index 972a849..6b3b1cf 100644
>> --- a/include/qemu/accel.h
>> +++ b/include/qemu/accel.h
>> @@ -37,6 +37,7 @@ typedef struct AccelClass {
>> /*< public >*/
>>
>> const char *name;
>> + int (*preinit)(AccelState *accel);
>> int (*init_machine)(MachineState *ms);
>
> Might be nice to add some comment on what should be part of preinit() and
> what should be part of init_machine().
>
> AFAIU the preinit() was about probing whether an accel is supported. Maybe
> rename it to probe()? Then it's also clear why some accel (e.g. tcg)
> doesn't need that, because it is always supported and no probe is needed.
>
>> #ifndef CONFIG_USER_ONLY
>> void (*setup_post)(MachineState *ms, AccelState *accel);
>> diff --git a/target/i386/nvmm/nvmm-all.c b/target/i386/nvmm/nvmm-all.c
>> index 65768ac..ce858a0 100644
>> --- a/target/i386/nvmm/nvmm-all.c
>> +++ b/target/i386/nvmm/nvmm-all.c
>> @@ -1153,7 +1153,7 @@ static struct RAMBlockNotifier nvmm_ram_notifier = {
>> /* -------------------------------------------------------------------------- */
>>
>> static int
>> -nvmm_accel_init(MachineState *ms)
>> +nvmm_accel_preinit(MachineState *ms)
>> {
>> int ret, err;
>>
>> @@ -1178,6 +1178,13 @@ nvmm_accel_init(MachineState *ms)
>> error_report("NVMM: Wrong state size %u", qemu_mach.cap.state_size);
>> return -EPROGMISMATCH;
>> }
>> + return 0;
>> +}
>> +
>> +static int
>> +nvmm_accel_init(MachineState *ms)
>> +{
>> + int ret, err;
>>
>> ret = nvmm_machine_create(&qemu_mach.mach);
>> if (ret == -1) {
>> @@ -1204,6 +1211,7 @@ nvmm_accel_class_init(ObjectClass *oc, void *data)
>> {
>> AccelClass *ac = ACCEL_CLASS(oc);
>> ac->name = "NVMM";
>> + ac->preinit = nvmm_accel_preinit;
>> ac->init_machine = nvmm_accel_init;
>> ac->allowed = &nvmm_allowed;
>> }
>> diff --git a/target/i386/whpx/whpx-all.c b/target/i386/whpx/whpx-all.c
>> index a6674a8..50bfc19 100644
>> --- a/target/i386/whpx/whpx-all.c
>> +++ b/target/i386/whpx/whpx-all.c
>> @@ -2516,6 +2516,14 @@ static void whpx_set_kernel_irqchip(Object *obj, Visitor *v,
>> * Partition support
>> */
>>
>> +static int whpx_accel_preinit(AccelState *accel)
>> +{
>> + if (!init_whp_dispatch()) {
>> + return -ENOSYS;
>> + }
>> + return 0;
>> +}
>> +
>> static int whpx_accel_init(MachineState *ms)
>> {
>> struct whpx_state *whpx;
>> @@ -2529,11 +2537,6 @@ static int whpx_accel_init(MachineState *ms)
>>
>> whpx = &whpx_global;
>>
>> - if (!init_whp_dispatch()) {
>> - ret = -ENOSYS;
>> - goto error;
>> - }
>> -
>> whpx->mem_quota = ms->ram_size;
>>
>> hr = whp_dispatch.WHvGetCapability(
>> @@ -2713,6 +2716,7 @@ static void whpx_accel_class_init(ObjectClass *oc, void *data)
>> {
>> AccelClass *ac = ACCEL_CLASS(oc);
>> ac->name = "WHPX";
>> + ac->preinit = whpx_accel_preinit;
>> ac->init_machine = whpx_accel_init;
>> ac->allowed = &whpx_allowed;
>>
>> --
>> 1.8.3.1
>>
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [RFC V1 02/14] accel: accel preinit function
2024-10-17 15:14 ` [RFC V1 02/14] accel: accel preinit function Steve Sistare
2024-10-17 15:26 ` Steven Sistare
2024-10-21 14:54 ` Peter Xu
@ 2024-10-23 15:53 ` Paolo Bonzini
2024-10-23 16:25 ` Steven Sistare
2 siblings, 1 reply; 63+ messages in thread
From: Paolo Bonzini @ 2024-10-23 15:53 UTC (permalink / raw)
To: Steve Sistare, qemu-devel
Cc: Peter Xu, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Daniel P. Berrange,
Markus Armbruster
On 10/17/24 17:14, Steve Sistare wrote:
> Extract the first part of the AccelState init_machine function into
> a new function preinit, which can be called without knowing any
> machine properties. For now call preinit and init_machine at the
> same place, so no functional change.
For KVM I would also include
missing_cap = kvm_check_extension_list(s, kvm_required_capabilites);
if (!missing_cap) {
missing_cap =
kvm_check_extension_list(s, kvm_arch_required_capabilities);
}
With this change, patches 1-3 are okay as a separate submission.
Paolo
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
> accel/accel-system.c | 6 +++++
> accel/kvm/kvm-all.c | 58 +++++++++++++++++++++++++++++----------------
> accel/xen/xen-all.c | 11 ++++++---
> include/qemu/accel.h | 1 +
> target/i386/nvmm/nvmm-all.c | 10 +++++++-
> target/i386/whpx/whpx-all.c | 14 +++++++----
> 6 files changed, 70 insertions(+), 30 deletions(-)
>
> diff --git a/accel/accel-system.c b/accel/accel-system.c
> index f6c947d..fef6625 100644
> --- a/accel/accel-system.c
> +++ b/accel/accel-system.c
> @@ -36,8 +36,14 @@ int accel_init_machine(AccelState *accel, MachineState *ms)
> int ret;
> ms->accelerator = accel;
> *(acc->allowed) = true;
> + ret = acc->preinit(accel);
> + if (ret < 0) {
> + goto fail;
> + }
> +
> ret = acc->init_machine(ms);
> if (ret < 0) {
> +fail:
> ms->accelerator = NULL;
> *(acc->allowed) = false;
> object_unref(OBJECT(accel));
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 905fb84..c7c6001 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2484,6 +2484,42 @@ static int kvm_setup_dirty_ring(KVMState *s)
> return 0;
> }
>
> +static int kvm_preinit(AccelState *accel)
> +{
> + int ret;
> + KVMState *s = KVM_STATE(accel);
> +
> + s->fd = qemu_open_old(s->device ?: "/dev/kvm", O_RDWR);
> + if (s->fd == -1) {
> + error_report("Could not access KVM kernel module: %m");
> + ret = -errno;
> + goto err;
> + }
> +
> + ret = kvm_ioctl(s, KVM_GET_API_VERSION, 0);
> + if (ret < KVM_API_VERSION) {
> + if (ret >= 0) {
> + ret = -EINVAL;
> + }
> + fprintf(stderr, "kvm version too old\n");
> + goto err;
> + }
> +
> + if (ret > KVM_API_VERSION) {
> + ret = -EINVAL;
> + error_report("kvm version not supported");
> + goto err;
> + }
> + return 0;
> +
> +err:
> + assert(ret < 0);
> + if (s->fd != -1) {
> + close(s->fd);
> + }
> + return ret;
> +}
> +
> static int kvm_init(MachineState *ms)
> {
> MachineClass *mc = MACHINE_GET_CLASS(ms);
> @@ -2523,27 +2559,6 @@ static int kvm_init(MachineState *ms)
> QTAILQ_INIT(&s->kvm_sw_breakpoints);
> #endif
> QLIST_INIT(&s->kvm_parked_vcpus);
> - s->fd = qemu_open_old(s->device ?: "/dev/kvm", O_RDWR);
> - if (s->fd == -1) {
> - error_report("Could not access KVM kernel module: %m");
> - ret = -errno;
> - goto err;
> - }
> -
> - ret = kvm_ioctl(s, KVM_GET_API_VERSION, 0);
> - if (ret < KVM_API_VERSION) {
> - if (ret >= 0) {
> - ret = -EINVAL;
> - }
> - error_report("kvm version too old");
> - goto err;
> - }
> -
> - if (ret > KVM_API_VERSION) {
> - ret = -EINVAL;
> - error_report("kvm version not supported");
> - goto err;
> - }
>
> kvm_supported_memory_attributes = kvm_check_extension(s, KVM_CAP_MEMORY_ATTRIBUTES);
> kvm_guest_memfd_supported =
> @@ -3891,6 +3906,7 @@ static void kvm_accel_class_init(ObjectClass *oc, void *data)
> {
> AccelClass *ac = ACCEL_CLASS(oc);
> ac->name = "KVM";
> + ac->preinit = kvm_preinit;
> ac->init_machine = kvm_init;
> ac->has_memory = kvm_accel_has_memory;
> ac->allowed = &kvm_allowed;
> diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
> index 0bdefce..dfcb90c 100644
> --- a/accel/xen/xen-all.c
> +++ b/accel/xen/xen-all.c
> @@ -75,10 +75,8 @@ static void xen_setup_post(MachineState *ms, AccelState *accel)
> }
> }
>
> -static int xen_init(MachineState *ms)
> +static int xen_preinit(AccelState *accel)
> {
> - MachineClass *mc = MACHINE_GET_CLASS(ms);
> -
> xen_xc = xc_interface_open(0, 0, 0);
> if (xen_xc == NULL) {
> xen_pv_printf(NULL, 0, "can't open xen interface\n");
> @@ -97,6 +95,12 @@ static int xen_init(MachineState *ms)
> xc_interface_close(xen_xc);
> return -1;
> }
> + return 0;
> +}
> +
> +static int xen_init(MachineState *ms)
> +{
> + MachineClass *mc = MACHINE_GET_CLASS(ms);
>
> /*
> * The XenStore write would fail when running restricted so don't attempt
> @@ -125,6 +129,7 @@ static void xen_accel_class_init(ObjectClass *oc, void *data)
> };
>
> ac->name = "Xen";
> + ac->preinit = xen_preinit;
> ac->init_machine = xen_init;
> ac->setup_post = xen_setup_post;
> ac->allowed = &xen_allowed;
> diff --git a/include/qemu/accel.h b/include/qemu/accel.h
> index 972a849..6b3b1cf 100644
> --- a/include/qemu/accel.h
> +++ b/include/qemu/accel.h
> @@ -37,6 +37,7 @@ typedef struct AccelClass {
> /*< public >*/
>
> const char *name;
> + int (*preinit)(AccelState *accel);
> int (*init_machine)(MachineState *ms);
> #ifndef CONFIG_USER_ONLY
> void (*setup_post)(MachineState *ms, AccelState *accel);
> diff --git a/target/i386/nvmm/nvmm-all.c b/target/i386/nvmm/nvmm-all.c
> index 65768ac..ce858a0 100644
> --- a/target/i386/nvmm/nvmm-all.c
> +++ b/target/i386/nvmm/nvmm-all.c
> @@ -1153,7 +1153,7 @@ static struct RAMBlockNotifier nvmm_ram_notifier = {
> /* -------------------------------------------------------------------------- */
>
> static int
> -nvmm_accel_init(MachineState *ms)
> +nvmm_accel_preinit(MachineState *ms)
> {
> int ret, err;
>
> @@ -1178,6 +1178,13 @@ nvmm_accel_init(MachineState *ms)
> error_report("NVMM: Wrong state size %u", qemu_mach.cap.state_size);
> return -EPROGMISMATCH;
> }
> + return 0;
> +}
> +
> +static int
> +nvmm_accel_init(MachineState *ms)
> +{
> + int ret, err;
>
> ret = nvmm_machine_create(&qemu_mach.mach);
> if (ret == -1) {
> @@ -1204,6 +1211,7 @@ nvmm_accel_class_init(ObjectClass *oc, void *data)
> {
> AccelClass *ac = ACCEL_CLASS(oc);
> ac->name = "NVMM";
> + ac->preinit = nvmm_accel_preinit;
> ac->init_machine = nvmm_accel_init;
> ac->allowed = &nvmm_allowed;
> }
> diff --git a/target/i386/whpx/whpx-all.c b/target/i386/whpx/whpx-all.c
> index a6674a8..50bfc19 100644
> --- a/target/i386/whpx/whpx-all.c
> +++ b/target/i386/whpx/whpx-all.c
> @@ -2516,6 +2516,14 @@ static void whpx_set_kernel_irqchip(Object *obj, Visitor *v,
> * Partition support
> */
>
> +static int whpx_accel_preinit(AccelState *accel)
> +{
> + if (!init_whp_dispatch()) {
> + return -ENOSYS;
> + }
> + return 0;
> +}
> +
> static int whpx_accel_init(MachineState *ms)
> {
> struct whpx_state *whpx;
> @@ -2529,11 +2537,6 @@ static int whpx_accel_init(MachineState *ms)
>
> whpx = &whpx_global;
>
> - if (!init_whp_dispatch()) {
> - ret = -ENOSYS;
> - goto error;
> - }
> -
> whpx->mem_quota = ms->ram_size;
>
> hr = whp_dispatch.WHvGetCapability(
> @@ -2713,6 +2716,7 @@ static void whpx_accel_class_init(ObjectClass *oc, void *data)
> {
> AccelClass *ac = ACCEL_CLASS(oc);
> ac->name = "WHPX";
> + ac->preinit = whpx_accel_preinit;
> ac->init_machine = whpx_accel_init;
> ac->allowed = &whpx_allowed;
>
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [RFC V1 02/14] accel: accel preinit function
2024-10-23 15:53 ` Paolo Bonzini
@ 2024-10-23 16:25 ` Steven Sistare
0 siblings, 0 replies; 63+ messages in thread
From: Steven Sistare @ 2024-10-23 16:25 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
Cc: Peter Xu, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Daniel P. Berrange,
Markus Armbruster
On 10/23/2024 11:53 AM, Paolo Bonzini wrote:
> On 10/17/24 17:14, Steve Sistare wrote:
>> Extract the first part of the AccelState init_machine function into
>> a new function preinit, which can be called without knowing any
>> machine properties. For now call preinit and init_machine at the
>> same place, so no functional change.
>
> For KVM I would also include
>
> missing_cap = kvm_check_extension_list(s, kvm_required_capabilites);
> if (!missing_cap) {
> missing_cap =
> kvm_check_extension_list(s, kvm_arch_required_capabilities);
> }
>
> With this change, patches 1-3 are okay as a separate submission.
Thanks, will do and will re-submit - steve
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>> accel/accel-system.c | 6 +++++
>> accel/kvm/kvm-all.c | 58 +++++++++++++++++++++++++++++----------------
>> accel/xen/xen-all.c | 11 ++++++---
>> include/qemu/accel.h | 1 +
>> target/i386/nvmm/nvmm-all.c | 10 +++++++-
>> target/i386/whpx/whpx-all.c | 14 +++++++----
>> 6 files changed, 70 insertions(+), 30 deletions(-)
>>
>> diff --git a/accel/accel-system.c b/accel/accel-system.c
>> index f6c947d..fef6625 100644
>> --- a/accel/accel-system.c
>> +++ b/accel/accel-system.c
>> @@ -36,8 +36,14 @@ int accel_init_machine(AccelState *accel, MachineState *ms)
>> int ret;
>> ms->accelerator = accel;
>> *(acc->allowed) = true;
>> + ret = acc->preinit(accel);
>> + if (ret < 0) {
>> + goto fail;
>> + }
>> +
>> ret = acc->init_machine(ms);
>> if (ret < 0) {
>> +fail:
>> ms->accelerator = NULL;
>> *(acc->allowed) = false;
>> object_unref(OBJECT(accel));
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index 905fb84..c7c6001 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -2484,6 +2484,42 @@ static int kvm_setup_dirty_ring(KVMState *s)
>> return 0;
>> }
>> +static int kvm_preinit(AccelState *accel)
>> +{
>> + int ret;
>> + KVMState *s = KVM_STATE(accel);
>> +
>> + s->fd = qemu_open_old(s->device ?: "/dev/kvm", O_RDWR);
>> + if (s->fd == -1) {
>> + error_report("Could not access KVM kernel module: %m");
>> + ret = -errno;
>> + goto err;
>> + }
>> +
>> + ret = kvm_ioctl(s, KVM_GET_API_VERSION, 0);
>> + if (ret < KVM_API_VERSION) {
>> + if (ret >= 0) {
>> + ret = -EINVAL;
>> + }
>> + fprintf(stderr, "kvm version too old\n");
>> + goto err;
>> + }
>> +
>> + if (ret > KVM_API_VERSION) {
>> + ret = -EINVAL;
>> + error_report("kvm version not supported");
>> + goto err;
>> + }
>> + return 0;
>> +
>> +err:
>> + assert(ret < 0);
>> + if (s->fd != -1) {
>> + close(s->fd);
>> + }
>> + return ret;
>> +}
>> +
>> static int kvm_init(MachineState *ms)
>> {
>> MachineClass *mc = MACHINE_GET_CLASS(ms);
>> @@ -2523,27 +2559,6 @@ static int kvm_init(MachineState *ms)
>> QTAILQ_INIT(&s->kvm_sw_breakpoints);
>> #endif
>> QLIST_INIT(&s->kvm_parked_vcpus);
>> - s->fd = qemu_open_old(s->device ?: "/dev/kvm", O_RDWR);
>> - if (s->fd == -1) {
>> - error_report("Could not access KVM kernel module: %m");
>> - ret = -errno;
>> - goto err;
>> - }
>> -
>> - ret = kvm_ioctl(s, KVM_GET_API_VERSION, 0);
>> - if (ret < KVM_API_VERSION) {
>> - if (ret >= 0) {
>> - ret = -EINVAL;
>> - }
>> - error_report("kvm version too old");
>> - goto err;
>> - }
>> -
>> - if (ret > KVM_API_VERSION) {
>> - ret = -EINVAL;
>> - error_report("kvm version not supported");
>> - goto err;
>> - }
>> kvm_supported_memory_attributes = kvm_check_extension(s, KVM_CAP_MEMORY_ATTRIBUTES);
>> kvm_guest_memfd_supported =
>> @@ -3891,6 +3906,7 @@ static void kvm_accel_class_init(ObjectClass *oc, void *data)
>> {
>> AccelClass *ac = ACCEL_CLASS(oc);
>> ac->name = "KVM";
>> + ac->preinit = kvm_preinit;
>> ac->init_machine = kvm_init;
>> ac->has_memory = kvm_accel_has_memory;
>> ac->allowed = &kvm_allowed;
>> diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
>> index 0bdefce..dfcb90c 100644
>> --- a/accel/xen/xen-all.c
>> +++ b/accel/xen/xen-all.c
>> @@ -75,10 +75,8 @@ static void xen_setup_post(MachineState *ms, AccelState *accel)
>> }
>> }
>> -static int xen_init(MachineState *ms)
>> +static int xen_preinit(AccelState *accel)
>> {
>> - MachineClass *mc = MACHINE_GET_CLASS(ms);
>> -
>> xen_xc = xc_interface_open(0, 0, 0);
>> if (xen_xc == NULL) {
>> xen_pv_printf(NULL, 0, "can't open xen interface\n");
>> @@ -97,6 +95,12 @@ static int xen_init(MachineState *ms)
>> xc_interface_close(xen_xc);
>> return -1;
>> }
>> + return 0;
>> +}
>> +
>> +static int xen_init(MachineState *ms)
>> +{
>> + MachineClass *mc = MACHINE_GET_CLASS(ms);
>> /*
>> * The XenStore write would fail when running restricted so don't attempt
>> @@ -125,6 +129,7 @@ static void xen_accel_class_init(ObjectClass *oc, void *data)
>> };
>> ac->name = "Xen";
>> + ac->preinit = xen_preinit;
>> ac->init_machine = xen_init;
>> ac->setup_post = xen_setup_post;
>> ac->allowed = &xen_allowed;
>> diff --git a/include/qemu/accel.h b/include/qemu/accel.h
>> index 972a849..6b3b1cf 100644
>> --- a/include/qemu/accel.h
>> +++ b/include/qemu/accel.h
>> @@ -37,6 +37,7 @@ typedef struct AccelClass {
>> /*< public >*/
>> const char *name;
>> + int (*preinit)(AccelState *accel);
>> int (*init_machine)(MachineState *ms);
>> #ifndef CONFIG_USER_ONLY
>> void (*setup_post)(MachineState *ms, AccelState *accel);
>> diff --git a/target/i386/nvmm/nvmm-all.c b/target/i386/nvmm/nvmm-all.c
>> index 65768ac..ce858a0 100644
>> --- a/target/i386/nvmm/nvmm-all.c
>> +++ b/target/i386/nvmm/nvmm-all.c
>> @@ -1153,7 +1153,7 @@ static struct RAMBlockNotifier nvmm_ram_notifier = {
>> /* -------------------------------------------------------------------------- */
>> static int
>> -nvmm_accel_init(MachineState *ms)
>> +nvmm_accel_preinit(MachineState *ms)
>> {
>> int ret, err;
>> @@ -1178,6 +1178,13 @@ nvmm_accel_init(MachineState *ms)
>> error_report("NVMM: Wrong state size %u", qemu_mach.cap.state_size);
>> return -EPROGMISMATCH;
>> }
>> + return 0;
>> +}
>> +
>> +static int
>> +nvmm_accel_init(MachineState *ms)
>> +{
>> + int ret, err;
>> ret = nvmm_machine_create(&qemu_mach.mach);
>> if (ret == -1) {
>> @@ -1204,6 +1211,7 @@ nvmm_accel_class_init(ObjectClass *oc, void *data)
>> {
>> AccelClass *ac = ACCEL_CLASS(oc);
>> ac->name = "NVMM";
>> + ac->preinit = nvmm_accel_preinit;
>> ac->init_machine = nvmm_accel_init;
>> ac->allowed = &nvmm_allowed;
>> }
>> diff --git a/target/i386/whpx/whpx-all.c b/target/i386/whpx/whpx-all.c
>> index a6674a8..50bfc19 100644
>> --- a/target/i386/whpx/whpx-all.c
>> +++ b/target/i386/whpx/whpx-all.c
>> @@ -2516,6 +2516,14 @@ static void whpx_set_kernel_irqchip(Object *obj, Visitor *v,
>> * Partition support
>> */
>> +static int whpx_accel_preinit(AccelState *accel)
>> +{
>> + if (!init_whp_dispatch()) {
>> + return -ENOSYS;
>> + }
>> + return 0;
>> +}
>> +
>> static int whpx_accel_init(MachineState *ms)
>> {
>> struct whpx_state *whpx;
>> @@ -2529,11 +2537,6 @@ static int whpx_accel_init(MachineState *ms)
>> whpx = &whpx_global;
>> - if (!init_whp_dispatch()) {
>> - ret = -ENOSYS;
>> - goto error;
>> - }
>> -
>> whpx->mem_quota = ms->ram_size;
>> hr = whp_dispatch.WHvGetCapability(
>> @@ -2713,6 +2716,7 @@ static void whpx_accel_class_init(ObjectClass *oc, void *data)
>> {
>> AccelClass *ac = ACCEL_CLASS(oc);
>> ac->name = "WHPX";
>> + ac->preinit = whpx_accel_preinit;
>> ac->init_machine = whpx_accel_init;
>> ac->allowed = &whpx_allowed;
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* [RFC V1 03/14] accel: split configure_accelerators
2024-10-17 15:14 [RFC V1 00/14] precreate phase Steve Sistare
2024-10-17 15:14 ` [RFC V1 01/14] accel: encapsulate search state Steve Sistare
2024-10-17 15:14 ` [RFC V1 02/14] accel: accel preinit function Steve Sistare
@ 2024-10-17 15:14 ` Steve Sistare
2024-10-17 15:14 ` [RFC V1 04/14] accel: set accelerator and machine props earlier Steve Sistare
` (12 subsequent siblings)
15 siblings, 0 replies; 63+ messages in thread
From: Steve Sistare @ 2024-10-17 15:14 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster, Steve Sistare
Split configure_accelerators into a first function (still named
configure_accelerators) that loops and chooses the first accelerator
for which preinit succeeds, and a second function create_accelerator
which calls machine_init on the chosen accelerator.
configure_accelerators can be called without knowing any machine
properties. create_accelerator cannot be called until the machine
properties are defined. For now call them at the same place, so
no functional change.
Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
accel/accel-system.c | 6 ------
system/vl.c | 27 +++++++++++++++++++++------
2 files changed, 21 insertions(+), 12 deletions(-)
diff --git a/accel/accel-system.c b/accel/accel-system.c
index fef6625..f6c947d 100644
--- a/accel/accel-system.c
+++ b/accel/accel-system.c
@@ -36,14 +36,8 @@ int accel_init_machine(AccelState *accel, MachineState *ms)
int ret;
ms->accelerator = accel;
*(acc->allowed) = true;
- ret = acc->preinit(accel);
- if (ret < 0) {
- goto fail;
- }
-
ret = acc->init_machine(ms);
if (ret < 0) {
-fail:
ms->accelerator = NULL;
*(acc->allowed) = false;
object_unref(OBJECT(accel));
diff --git a/system/vl.c b/system/vl.c
index 8b345dd..b94a6b9 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -182,6 +182,7 @@ static const char *log_file;
static bool list_data_dirs;
static const char *qtest_chrdev;
static const char *qtest_log;
+static AccelState *accel;
static int has_defaults = 1;
static int default_audio = 1;
@@ -2337,7 +2338,7 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
accel,
&error_fatal);
- ret = accel_init_machine(accel, current_machine);
+ ret = ac->preinit ? ac->preinit(accel) : 0;
if (ret < 0) {
if (!qtest_with_kvm || ret != -ENOENT) {
error_report("failed to initialize %s: %s", acc, strerror(-ret));
@@ -2353,13 +2354,10 @@ bad:
return 0;
}
-static void configure_accelerators(const char *progname)
+static AccelState *configure_accelerators(const char *progname)
{
AccelSearch acs = {0};
- qemu_opts_foreach(qemu_find_opts("icount"),
- do_configure_icount, NULL, &error_fatal);
-
if (QTAILQ_EMPTY(&qemu_accel_opts.head)) {
char **accel_list, **tmp;
@@ -2418,6 +2416,22 @@ static void configure_accelerators(const char *progname)
if (acs.init_failed && !qtest_chrdev) {
error_report("falling back to %s", ACCEL_GET_CLASS(acs.accel)->name);
}
+ return acs.accel;
+}
+
+static void create_accelerator(AccelState *accel)
+{
+ int ret;
+
+ qemu_opts_foreach(qemu_find_opts("icount"),
+ do_configure_icount, NULL, &error_fatal);
+
+ ret = accel_init_machine(accel, current_machine);
+ if (ret < 0) {
+ error_report("failed to initialize %s: %s",
+ ACCEL_GET_CLASS(accel)->name, strerror(-ret));
+ exit(1);
+ }
if (icount_enabled() && !tcg_enabled()) {
error_report("-icount is not allowed with hardware virtualization");
@@ -3731,7 +3745,8 @@ void qemu_init(int argc, char **argv)
* Note: uses machine properties such as kernel-irqchip, must run
* after qemu_apply_machine_options.
*/
- configure_accelerators(argv[0]);
+ accel = configure_accelerators(argv[0]);
+ create_accelerator(accel);
phase_advance(PHASE_ACCEL_CREATED);
/*
--
1.8.3.1
^ permalink raw reply related [flat|nested] 63+ messages in thread* [RFC V1 04/14] accel: set accelerator and machine props earlier
2024-10-17 15:14 [RFC V1 00/14] precreate phase Steve Sistare
` (2 preceding siblings ...)
2024-10-17 15:14 ` [RFC V1 03/14] accel: split configure_accelerators Steve Sistare
@ 2024-10-17 15:14 ` Steve Sistare
2024-10-18 15:08 ` Fabiano Rosas
` (2 more replies)
2024-10-17 15:14 ` [RFC V1 05/14] migration: init and listen during precreate Steve Sistare
` (11 subsequent siblings)
15 siblings, 3 replies; 63+ messages in thread
From: Steve Sistare @ 2024-10-17 15:14 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster, Steve Sistare
Make all global and compat properties available before the first objects
are created. Set accelerator compatibility properties in
configure_accelerators, when the accelerator is chosen, and call
configure_accelerators earlier. Set machine options earlier.
Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
accel/accel-system.c | 2 --
system/vl.c | 34 ++++++++++++++++++----------------
2 files changed, 18 insertions(+), 18 deletions(-)
diff --git a/accel/accel-system.c b/accel/accel-system.c
index f6c947d..c8aeae4 100644
--- a/accel/accel-system.c
+++ b/accel/accel-system.c
@@ -41,8 +41,6 @@ int accel_init_machine(AccelState *accel, MachineState *ms)
ms->accelerator = NULL;
*(acc->allowed) = false;
object_unref(OBJECT(accel));
- } else {
- object_set_accelerator_compat_props(acc->compat_props);
}
return ret;
}
diff --git a/system/vl.c b/system/vl.c
index b94a6b9..bca2292 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -2346,6 +2346,7 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
goto bad;
}
+ object_set_accelerator_compat_props(ac->compat_props);
acs->accel = accel;
return 1;
@@ -3728,29 +3729,14 @@ void qemu_init(int argc, char **argv)
parse_memory_options();
qemu_create_machine(machine_opts_dict);
-
- suspend_mux_open();
-
- qemu_disable_default_devices();
- qemu_setup_display();
- qemu_create_default_devices();
- qemu_create_early_backends();
-
qemu_apply_legacy_machine_options(machine_opts_dict);
qemu_apply_machine_options(machine_opts_dict);
qobject_unref(machine_opts_dict);
- phase_advance(PHASE_MACHINE_CREATED);
- /*
- * Note: uses machine properties such as kernel-irqchip, must run
- * after qemu_apply_machine_options.
- */
accel = configure_accelerators(argv[0]);
- create_accelerator(accel);
- phase_advance(PHASE_ACCEL_CREATED);
/*
- * Beware, QOM objects created before this point miss global and
+ * QOM objects created after this point see all global and
* compat properties.
*
* Global properties get set up by qdev_prop_register_global(),
@@ -3765,6 +3751,22 @@ void qemu_init(int argc, char **argv)
* called from do_configure_accelerator().
*/
+ suspend_mux_open();
+
+ qemu_disable_default_devices();
+ qemu_setup_display();
+ qemu_create_default_devices();
+ qemu_create_early_backends();
+
+ phase_advance(PHASE_MACHINE_CREATED);
+
+ /*
+ * Note: uses machine properties such as kernel-irqchip, must run
+ * after qemu_apply_machine_options.
+ */
+ create_accelerator(accel);
+ phase_advance(PHASE_ACCEL_CREATED);
+
machine_class = MACHINE_GET_CLASS(current_machine);
if (!qtest_enabled() && machine_class->deprecation_reason) {
warn_report("Machine type '%s' is deprecated: %s",
--
1.8.3.1
^ permalink raw reply related [flat|nested] 63+ messages in thread* Re: [RFC V1 04/14] accel: set accelerator and machine props earlier
2024-10-17 15:14 ` [RFC V1 04/14] accel: set accelerator and machine props earlier Steve Sistare
@ 2024-10-18 15:08 ` Fabiano Rosas
2024-10-18 15:32 ` Steven Sistare
2024-10-21 15:19 ` Peter Xu
2024-10-23 16:00 ` Paolo Bonzini
2 siblings, 1 reply; 63+ messages in thread
From: Fabiano Rosas @ 2024-10-18 15:08 UTC (permalink / raw)
To: Steve Sistare, qemu-devel
Cc: Peter Xu, David Hildenbrand, Marcel Apfelbaum, Eduardo Habkost,
Philippe Mathieu-Daude, Paolo Bonzini, Daniel P. Berrange,
Markus Armbruster, Steve Sistare
Steve Sistare <steven.sistare@oracle.com> writes:
> Make all global and compat properties available before the first objects
> are created. Set accelerator compatibility properties in
> configure_accelerators, when the accelerator is chosen, and call
> configure_accelerators earlier. Set machine options earlier.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
> accel/accel-system.c | 2 --
> system/vl.c | 34 ++++++++++++++++++----------------
> 2 files changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/accel/accel-system.c b/accel/accel-system.c
> index f6c947d..c8aeae4 100644
> --- a/accel/accel-system.c
> +++ b/accel/accel-system.c
> @@ -41,8 +41,6 @@ int accel_init_machine(AccelState *accel, MachineState *ms)
> ms->accelerator = NULL;
> *(acc->allowed) = false;
> object_unref(OBJECT(accel));
> - } else {
> - object_set_accelerator_compat_props(acc->compat_props);
> }
> return ret;
> }
> diff --git a/system/vl.c b/system/vl.c
> index b94a6b9..bca2292 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -2346,6 +2346,7 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
> goto bad;
> }
>
> + object_set_accelerator_compat_props(ac->compat_props);
> acs->accel = accel;
> return 1;
>
> @@ -3728,29 +3729,14 @@ void qemu_init(int argc, char **argv)
> parse_memory_options();
>
> qemu_create_machine(machine_opts_dict);
> -
> - suspend_mux_open();
> -
> - qemu_disable_default_devices();
> - qemu_setup_display();
> - qemu_create_default_devices();
> - qemu_create_early_backends();
> -
> qemu_apply_legacy_machine_options(machine_opts_dict);
> qemu_apply_machine_options(machine_opts_dict);
> qobject_unref(machine_opts_dict);
> - phase_advance(PHASE_MACHINE_CREATED);
>
> - /*
> - * Note: uses machine properties such as kernel-irqchip, must run
> - * after qemu_apply_machine_options.
> - */
> accel = configure_accelerators(argv[0]);
> - create_accelerator(accel);
> - phase_advance(PHASE_ACCEL_CREATED);
>
> /*
> - * Beware, QOM objects created before this point miss global and
> + * QOM objects created after this point see all global and
> * compat properties.
> *
> * Global properties get set up by qdev_prop_register_global(),
> @@ -3765,6 +3751,22 @@ void qemu_init(int argc, char **argv)
> * called from do_configure_accelerator().
> */
>
> + suspend_mux_open();
> +
> + qemu_disable_default_devices();
> + qemu_setup_display();
> + qemu_create_default_devices();
> + qemu_create_early_backends();
> +
> + phase_advance(PHASE_MACHINE_CREATED);
> +
> + /*
> + * Note: uses machine properties such as kernel-irqchip, must run
> + * after qemu_apply_machine_options.
> + */
> + create_accelerator(accel);
> + phase_advance(PHASE_ACCEL_CREATED);
> +
> machine_class = MACHINE_GET_CLASS(current_machine);
> if (!qtest_enabled() && machine_class->deprecation_reason) {
> warn_report("Machine type '%s' is deprecated: %s",
Hi Steve,
after this commit:
$ QTEST_QEMU_BINARY=./qemu-system-aarch64 ./tests/qtest/xlnx-can-test
# random seed: R02Saf9b44f2d88dd417052905692ee79981
1..5
# Start of aarch64 tests
# Start of net tests
# Start of can tests
# starting QEMU: exec ./qemu-system-aarch64 -qtest unix:/tmp/qtest-2396.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-2396.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine xlnx-zcu102 -object can-bus,id=canbus -machine canbus0=canbus -machine canbus1=canbus -accel qtest
qemu-system-aarch64: Device 'canbus' not found
I tried briefly to figure out what the issue is, but I don't really
understand the dependencies involved. Hope you can tell us.
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [RFC V1 04/14] accel: set accelerator and machine props earlier
2024-10-18 15:08 ` Fabiano Rosas
@ 2024-10-18 15:32 ` Steven Sistare
2024-10-18 15:40 ` Steven Sistare
0 siblings, 1 reply; 63+ messages in thread
From: Steven Sistare @ 2024-10-18 15:32 UTC (permalink / raw)
To: Fabiano Rosas, qemu-devel
Cc: Peter Xu, David Hildenbrand, Marcel Apfelbaum, Eduardo Habkost,
Philippe Mathieu-Daude, Paolo Bonzini, Daniel P. Berrange,
Markus Armbruster
On 10/18/2024 11:08 AM, Fabiano Rosas wrote:
> Steve Sistare <steven.sistare@oracle.com> writes:
>
>> Make all global and compat properties available before the first objects
>> are created. Set accelerator compatibility properties in
>> configure_accelerators, when the accelerator is chosen, and call
>> configure_accelerators earlier. Set machine options earlier.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>> accel/accel-system.c | 2 --
>> system/vl.c | 34 ++++++++++++++++++----------------
>> 2 files changed, 18 insertions(+), 18 deletions(-)
>>
>> diff --git a/accel/accel-system.c b/accel/accel-system.c
>> index f6c947d..c8aeae4 100644
>> --- a/accel/accel-system.c
>> +++ b/accel/accel-system.c
>> @@ -41,8 +41,6 @@ int accel_init_machine(AccelState *accel, MachineState *ms)
>> ms->accelerator = NULL;
>> *(acc->allowed) = false;
>> object_unref(OBJECT(accel));
>> - } else {
>> - object_set_accelerator_compat_props(acc->compat_props);
>> }
>> return ret;
>> }
>> diff --git a/system/vl.c b/system/vl.c
>> index b94a6b9..bca2292 100644
>> --- a/system/vl.c
>> +++ b/system/vl.c
>> @@ -2346,6 +2346,7 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>> goto bad;
>> }
>>
>> + object_set_accelerator_compat_props(ac->compat_props);
>> acs->accel = accel;
>> return 1;
>>
>> @@ -3728,29 +3729,14 @@ void qemu_init(int argc, char **argv)
>> parse_memory_options();
>>
>> qemu_create_machine(machine_opts_dict);
>> -
>> - suspend_mux_open();
>> -
>> - qemu_disable_default_devices();
>> - qemu_setup_display();
>> - qemu_create_default_devices();
>> - qemu_create_early_backends();
>> -
>> qemu_apply_legacy_machine_options(machine_opts_dict);
>> qemu_apply_machine_options(machine_opts_dict);
>> qobject_unref(machine_opts_dict);
>> - phase_advance(PHASE_MACHINE_CREATED);
>>
>> - /*
>> - * Note: uses machine properties such as kernel-irqchip, must run
>> - * after qemu_apply_machine_options.
>> - */
>> accel = configure_accelerators(argv[0]);
>> - create_accelerator(accel);
>> - phase_advance(PHASE_ACCEL_CREATED);
>>
>> /*
>> - * Beware, QOM objects created before this point miss global and
>> + * QOM objects created after this point see all global and
>> * compat properties.
>> *
>> * Global properties get set up by qdev_prop_register_global(),
>> @@ -3765,6 +3751,22 @@ void qemu_init(int argc, char **argv)
>> * called from do_configure_accelerator().
>> */
>>
>> + suspend_mux_open();
>> +
>> + qemu_disable_default_devices();
>> + qemu_setup_display();
>> + qemu_create_default_devices();
>> + qemu_create_early_backends();
>> +
>> + phase_advance(PHASE_MACHINE_CREATED);
>> +
>> + /*
>> + * Note: uses machine properties such as kernel-irqchip, must run
>> + * after qemu_apply_machine_options.
>> + */
>> + create_accelerator(accel);
>> + phase_advance(PHASE_ACCEL_CREATED);
>> +
>> machine_class = MACHINE_GET_CLASS(current_machine);
>> if (!qtest_enabled() && machine_class->deprecation_reason) {
>> warn_report("Machine type '%s' is deprecated: %s",
>
> Hi Steve,
>
> after this commit:
>
> $ QTEST_QEMU_BINARY=./qemu-system-aarch64 ./tests/qtest/xlnx-can-test
> # random seed: R02Saf9b44f2d88dd417052905692ee79981
> 1..5
> # Start of aarch64 tests
> # Start of net tests
> # Start of can tests
> # starting QEMU: exec ./qemu-system-aarch64 -qtest unix:/tmp/qtest-2396.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-2396.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine xlnx-zcu102 -object can-bus,id=canbus -machine canbus0=canbus -machine canbus1=canbus -accel qtest
> qemu-system-aarch64: Device 'canbus' not found
>
> I tried briefly to figure out what the issue is, but I don't really
> understand the dependencies involved. Hope you can tell us.
Thanks! I forgot to define the preinit method for the qtest accelerator in patch 1.
I'll verify that fixes the problem and send you a one-off patch if you want to continue
testing.
- Steve
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [RFC V1 04/14] accel: set accelerator and machine props earlier
2024-10-18 15:32 ` Steven Sistare
@ 2024-10-18 15:40 ` Steven Sistare
2024-10-18 19:15 ` Steven Sistare
0 siblings, 1 reply; 63+ messages in thread
From: Steven Sistare @ 2024-10-18 15:40 UTC (permalink / raw)
To: Fabiano Rosas, qemu-devel
Cc: Peter Xu, David Hildenbrand, Marcel Apfelbaum, Eduardo Habkost,
Philippe Mathieu-Daude, Paolo Bonzini, Daniel P. Berrange,
Markus Armbruster
On 10/18/2024 11:32 AM, Steven Sistare wrote:
> On 10/18/2024 11:08 AM, Fabiano Rosas wrote:
>> Steve Sistare <steven.sistare@oracle.com> writes:
>>
>>> Make all global and compat properties available before the first objects
>>> are created. Set accelerator compatibility properties in
>>> configure_accelerators, when the accelerator is chosen, and call
>>> configure_accelerators earlier. Set machine options earlier.
>>>
>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>> ---
>>> accel/accel-system.c | 2 --
>>> system/vl.c | 34 ++++++++++++++++++----------------
>>> 2 files changed, 18 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/accel/accel-system.c b/accel/accel-system.c
>>> index f6c947d..c8aeae4 100644
>>> --- a/accel/accel-system.c
>>> +++ b/accel/accel-system.c
>>> @@ -41,8 +41,6 @@ int accel_init_machine(AccelState *accel, MachineState *ms)
>>> ms->accelerator = NULL;
>>> *(acc->allowed) = false;
>>> object_unref(OBJECT(accel));
>>> - } else {
>>> - object_set_accelerator_compat_props(acc->compat_props);
>>> }
>>> return ret;
>>> }
>>> diff --git a/system/vl.c b/system/vl.c
>>> index b94a6b9..bca2292 100644
>>> --- a/system/vl.c
>>> +++ b/system/vl.c
>>> @@ -2346,6 +2346,7 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>>> goto bad;
>>> }
>>> + object_set_accelerator_compat_props(ac->compat_props);
>>> acs->accel = accel;
>>> return 1;
>>> @@ -3728,29 +3729,14 @@ void qemu_init(int argc, char **argv)
>>> parse_memory_options();
>>> qemu_create_machine(machine_opts_dict);
>>> -
>>> - suspend_mux_open();
>>> -
>>> - qemu_disable_default_devices();
>>> - qemu_setup_display();
>>> - qemu_create_default_devices();
>>> - qemu_create_early_backends();
>>> -
>>> qemu_apply_legacy_machine_options(machine_opts_dict);
>>> qemu_apply_machine_options(machine_opts_dict);
>>> qobject_unref(machine_opts_dict);
>>> - phase_advance(PHASE_MACHINE_CREATED);
>>> - /*
>>> - * Note: uses machine properties such as kernel-irqchip, must run
>>> - * after qemu_apply_machine_options.
>>> - */
>>> accel = configure_accelerators(argv[0]);
>>> - create_accelerator(accel);
>>> - phase_advance(PHASE_ACCEL_CREATED);
>>> /*
>>> - * Beware, QOM objects created before this point miss global and
>>> + * QOM objects created after this point see all global and
>>> * compat properties.
>>> *
>>> * Global properties get set up by qdev_prop_register_global(),
>>> @@ -3765,6 +3751,22 @@ void qemu_init(int argc, char **argv)
>>> * called from do_configure_accelerator().
>>> */
>>> + suspend_mux_open();
>>> +
>>> + qemu_disable_default_devices();
>>> + qemu_setup_display();
>>> + qemu_create_default_devices();
>>> + qemu_create_early_backends();
>>> +
>>> + phase_advance(PHASE_MACHINE_CREATED);
>>> +
>>> + /*
>>> + * Note: uses machine properties such as kernel-irqchip, must run
>>> + * after qemu_apply_machine_options.
>>> + */
>>> + create_accelerator(accel);
>>> + phase_advance(PHASE_ACCEL_CREATED);
>>> +
>>> machine_class = MACHINE_GET_CLASS(current_machine);
>>> if (!qtest_enabled() && machine_class->deprecation_reason) {
>>> warn_report("Machine type '%s' is deprecated: %s",
>>
>> Hi Steve,
>>
>> after this commit:
>>
>> $ QTEST_QEMU_BINARY=./qemu-system-aarch64 ./tests/qtest/xlnx-can-test
>> # random seed: R02Saf9b44f2d88dd417052905692ee79981
>> 1..5
>> # Start of aarch64 tests
>> # Start of net tests
>> # Start of can tests
>> # starting QEMU: exec ./qemu-system-aarch64 -qtest unix:/tmp/qtest-2396.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-2396.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine xlnx-zcu102 -object can-bus,id=canbus -machine canbus0=canbus -machine canbus1=canbus -accel qtest
>> qemu-system-aarch64: Device 'canbus' not found
>>
>> I tried briefly to figure out what the issue is, but I don't really
>> understand the dependencies involved. Hope you can tell us.
>
> Thanks! I forgot to define the preinit method for the qtest accelerator in patch 1.
> I'll verify that fixes the problem and send you a one-off patch if you want to continue
> testing.
Actually that is not a problem. qtest qtest_init_accel does nothing, so preinit will do
nothing, so it is OK to not define preinit.
Still looking.
- Steve
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [RFC V1 04/14] accel: set accelerator and machine props earlier
2024-10-18 15:40 ` Steven Sistare
@ 2024-10-18 19:15 ` Steven Sistare
2024-10-21 16:20 ` Peter Xu
2024-10-22 8:30 ` David Hildenbrand
0 siblings, 2 replies; 63+ messages in thread
From: Steven Sistare @ 2024-10-18 19:15 UTC (permalink / raw)
To: Fabiano Rosas, qemu-devel
Cc: Peter Xu, David Hildenbrand, Marcel Apfelbaum, Eduardo Habkost,
Philippe Mathieu-Daude, Paolo Bonzini, Daniel P. Berrange,
Markus Armbruster
On 10/18/2024 11:40 AM, Steven Sistare wrote:
> On 10/18/2024 11:32 AM, Steven Sistare wrote:
>> On 10/18/2024 11:08 AM, Fabiano Rosas wrote:
>>> Steve Sistare <steven.sistare@oracle.com> writes:
>>>
>>>> Make all global and compat properties available before the first objects
>>>> are created. Set accelerator compatibility properties in
>>>> configure_accelerators, when the accelerator is chosen, and call
>>>> configure_accelerators earlier. Set machine options earlier.
>>>>
>>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>>> ---
>>>> accel/accel-system.c | 2 --
>>>> system/vl.c | 34 ++++++++++++++++++----------------
>>>> 2 files changed, 18 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/accel/accel-system.c b/accel/accel-system.c
>>>> index f6c947d..c8aeae4 100644
>>>> --- a/accel/accel-system.c
>>>> +++ b/accel/accel-system.c
>>>> @@ -41,8 +41,6 @@ int accel_init_machine(AccelState *accel, MachineState *ms)
>>>> ms->accelerator = NULL;
>>>> *(acc->allowed) = false;
>>>> object_unref(OBJECT(accel));
>>>> - } else {
>>>> - object_set_accelerator_compat_props(acc->compat_props);
>>>> }
>>>> return ret;
>>>> }
>>>> diff --git a/system/vl.c b/system/vl.c
>>>> index b94a6b9..bca2292 100644
>>>> --- a/system/vl.c
>>>> +++ b/system/vl.c
>>>> @@ -2346,6 +2346,7 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>>>> goto bad;
>>>> }
>>>> + object_set_accelerator_compat_props(ac->compat_props);
>>>> acs->accel = accel;
>>>> return 1;
>>>> @@ -3728,29 +3729,14 @@ void qemu_init(int argc, char **argv)
>>>> parse_memory_options();
>>>> qemu_create_machine(machine_opts_dict);
>>>> -
>>>> - suspend_mux_open();
>>>> -
>>>> - qemu_disable_default_devices();
>>>> - qemu_setup_display();
>>>> - qemu_create_default_devices();
>>>> - qemu_create_early_backends();
>>>> -
>>>> qemu_apply_legacy_machine_options(machine_opts_dict);
>>>> qemu_apply_machine_options(machine_opts_dict);
>>>> qobject_unref(machine_opts_dict);
>>>> - phase_advance(PHASE_MACHINE_CREATED);
>>>> - /*
>>>> - * Note: uses machine properties such as kernel-irqchip, must run
>>>> - * after qemu_apply_machine_options.
>>>> - */
>>>> accel = configure_accelerators(argv[0]);
>>>> - create_accelerator(accel);
>>>> - phase_advance(PHASE_ACCEL_CREATED);
>>>> /*
>>>> - * Beware, QOM objects created before this point miss global and
>>>> + * QOM objects created after this point see all global and
>>>> * compat properties.
>>>> *
>>>> * Global properties get set up by qdev_prop_register_global(),
>>>> @@ -3765,6 +3751,22 @@ void qemu_init(int argc, char **argv)
>>>> * called from do_configure_accelerator().
>>>> */
>>>> + suspend_mux_open();
>>>> +
>>>> + qemu_disable_default_devices();
>>>> + qemu_setup_display();
>>>> + qemu_create_default_devices();
>>>> + qemu_create_early_backends();
>>>> +
>>>> + phase_advance(PHASE_MACHINE_CREATED);
>>>> +
>>>> + /*
>>>> + * Note: uses machine properties such as kernel-irqchip, must run
>>>> + * after qemu_apply_machine_options.
>>>> + */
>>>> + create_accelerator(accel);
>>>> + phase_advance(PHASE_ACCEL_CREATED);
>>>> +
>>>> machine_class = MACHINE_GET_CLASS(current_machine);
>>>> if (!qtest_enabled() && machine_class->deprecation_reason) {
>>>> warn_report("Machine type '%s' is deprecated: %s",
>>>
>>> Hi Steve,
>>>
>>> after this commit:
>>>
>>> $ QTEST_QEMU_BINARY=./qemu-system-aarch64 ./tests/qtest/xlnx-can-test
>>> # random seed: R02Saf9b44f2d88dd417052905692ee79981
>>> 1..5
>>> # Start of aarch64 tests
>>> # Start of net tests
>>> # Start of can tests
>>> # starting QEMU: exec ./qemu-system-aarch64 -qtest unix:/tmp/qtest-2396.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-2396.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine xlnx-zcu102 -object can-bus,id=canbus -machine canbus0=canbus -machine canbus1=canbus -accel qtest
>>> qemu-system-aarch64: Device 'canbus' not found
>>>
>>> I tried briefly to figure out what the issue is, but I don't really
>>> understand the dependencies involved. Hope you can tell us.
>>
>> Thanks! I forgot to define the preinit method for the qtest accelerator in patch 1.
>> I'll verify that fixes the problem and send you a one-off patch if you want to continue
>> testing.
>
> Actually that is not a problem. qtest qtest_init_accel does nothing, so preinit will do
> nothing, so it is OK to not define preinit.
>
> Still looking.
I understand this now. The old code worked in this order:
qemu_create_early_backends()
... creates "-object can-bus,id=canbus"
qemu_create_machine()
qemu_apply_machine_options()
applies link property "canbus0" with value canbus, finds canbus object
The new code fails:
qemu_create_machine()
qemu_apply_machine_options()
applies link property "canbus0" with value canbus,
error because fails to find canbus object
...
qemu_exit_precreate()
qemu_create_early_backends()
... creates "-object can-bus,id=canbus"
The fix is to provide a new function, called before qemu_create_machine,
which creates only the backends that are needed to create the machine.
AFAIK can-bus is the only one, because the xlnx-zcu102 machine has
link properties.
- Steve
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [RFC V1 04/14] accel: set accelerator and machine props earlier
2024-10-18 19:15 ` Steven Sistare
@ 2024-10-21 16:20 ` Peter Xu
2024-10-23 17:16 ` Paolo Bonzini
2024-10-22 8:30 ` David Hildenbrand
1 sibling, 1 reply; 63+ messages in thread
From: Peter Xu @ 2024-10-21 16:20 UTC (permalink / raw)
To: Steven Sistare
Cc: Fabiano Rosas, qemu-devel, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster
On Fri, Oct 18, 2024 at 03:15:56PM -0400, Steven Sistare wrote:
> I understand this now. The old code worked in this order:
>
> qemu_create_early_backends()
> ... creates "-object can-bus,id=canbus"
> qemu_create_machine()
> qemu_apply_machine_options()
> applies link property "canbus0" with value canbus, finds canbus object
>
> The new code fails:
>
> qemu_create_machine()
> qemu_apply_machine_options()
> applies link property "canbus0" with value canbus,
> error because fails to find canbus object
> ...
> qemu_exit_precreate()
> qemu_create_early_backends()
> ... creates "-object can-bus,id=canbus"
>
> The fix is to provide a new function, called before qemu_create_machine,
> which creates only the backends that are needed to create the machine.
> AFAIK can-bus is the only one, because the xlnx-zcu102 machine has
> link properties.
Wanna share more on the specific solution? I wonder if the plan is to add
one more special window for creating -object just for can-bus, and how to
justify that extra phase. Perhaps whatever that do not require fd to back
it (so not affected by CPR)? But not sure whether that's too special.
I wished it could be simply put into the "very early" stage (pre-sandbox),
but I think object_create_pre_sandbox() did mention that we need explicit
reasons for those, and I'm not sure whether this reason justifies either
for why can-bus is so special so can be created without the sandboxing.
--
Peter Xu
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [RFC V1 04/14] accel: set accelerator and machine props earlier
2024-10-21 16:20 ` Peter Xu
@ 2024-10-23 17:16 ` Paolo Bonzini
0 siblings, 0 replies; 63+ messages in thread
From: Paolo Bonzini @ 2024-10-23 17:16 UTC (permalink / raw)
To: Peter Xu, Steven Sistare
Cc: Fabiano Rosas, qemu-devel, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Daniel P. Berrange,
Markus Armbruster
On 10/21/24 18:20, Peter Xu wrote:
> On Fri, Oct 18, 2024 at 03:15:56PM -0400, Steven Sistare wrote:
>> I understand this now. The old code worked in this order:
>>
>> qemu_create_early_backends()
>> ... creates "-object can-bus,id=canbus"
>> qemu_create_machine()
>> qemu_apply_machine_options()
>> applies link property "canbus0" with value canbus, finds canbus object
>>
>> The new code fails:
>>
>> qemu_create_machine()
>> qemu_apply_machine_options()
>> applies link property "canbus0" with value canbus,
>> error because fails to find canbus object
>> ...
>> qemu_exit_precreate()
>> qemu_create_early_backends()
>> ... creates "-object can-bus,id=canbus"
>>
>> The fix is to provide a new function, called before qemu_create_machine,
>> which creates only the backends that are needed to create the machine.
>> AFAIK can-bus is the only one, because the xlnx-zcu102 machine has
>> link properties.
>
> Wanna share more on the specific solution? I wonder if the plan is to add
> one more special window for creating -object just for can-bus, and how to
> justify that extra phase.
Unfortunately, I don't think there's _anything_ that can justify an
extra phase of command line processing. The only sensible way to do
precreate is to get rid of as much as possible of the command line.
There is an old thread explaining what was fixed in preconfig in 2022
and what was missing. Start here:
https://patchew.org/QEMU/b31f442d28920447690a6b8cee865bdbacde1283.1635160056.git.mprivozn@redhat.com/#7f54174b-4f90-13bf-6905-6febb6203575@redhat.com
Already there I wrote "one thing I don't like of preconfig is that command line arguments linger until they are triggered by x-exit-preconfig".
Paolo
> Perhaps whatever that do not require fd to back
> it (so not affected by CPR)? But not sure whether that's too special.
>
> I wished it could be simply put into the "very early" stage (pre-sandbox),
> but I think object_create_pre_sandbox() did mention that we need explicit
> reasons for those, and I'm not sure whether this reason justifies either
> for why can-bus is so special so can be created without the sandboxing.
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [RFC V1 04/14] accel: set accelerator and machine props earlier
2024-10-18 19:15 ` Steven Sistare
2024-10-21 16:20 ` Peter Xu
@ 2024-10-22 8:30 ` David Hildenbrand
2024-10-23 20:28 ` Steven Sistare
1 sibling, 1 reply; 63+ messages in thread
From: David Hildenbrand @ 2024-10-22 8:30 UTC (permalink / raw)
To: Steven Sistare, Fabiano Rosas, qemu-devel
Cc: Peter Xu, Marcel Apfelbaum, Eduardo Habkost,
Philippe Mathieu-Daude, Paolo Bonzini, Daniel P. Berrange,
Markus Armbruster
On 18.10.24 21:15, Steven Sistare wrote:
> On 10/18/2024 11:40 AM, Steven Sistare wrote:
>> On 10/18/2024 11:32 AM, Steven Sistare wrote:
>>> On 10/18/2024 11:08 AM, Fabiano Rosas wrote:
>>>> Steve Sistare <steven.sistare@oracle.com> writes:
>>>>
>>>>> Make all global and compat properties available before the first objects
>>>>> are created. Set accelerator compatibility properties in
>>>>> configure_accelerators, when the accelerator is chosen, and call
>>>>> configure_accelerators earlier. Set machine options earlier.
>>>>>
>>>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>>>> ---
>>>>> accel/accel-system.c | 2 --
>>>>> system/vl.c | 34 ++++++++++++++++++----------------
>>>>> 2 files changed, 18 insertions(+), 18 deletions(-)
>>>>>
>>>>> diff --git a/accel/accel-system.c b/accel/accel-system.c
>>>>> index f6c947d..c8aeae4 100644
>>>>> --- a/accel/accel-system.c
>>>>> +++ b/accel/accel-system.c
>>>>> @@ -41,8 +41,6 @@ int accel_init_machine(AccelState *accel, MachineState *ms)
>>>>> ms->accelerator = NULL;
>>>>> *(acc->allowed) = false;
>>>>> object_unref(OBJECT(accel));
>>>>> - } else {
>>>>> - object_set_accelerator_compat_props(acc->compat_props);
>>>>> }
>>>>> return ret;
>>>>> }
>>>>> diff --git a/system/vl.c b/system/vl.c
>>>>> index b94a6b9..bca2292 100644
>>>>> --- a/system/vl.c
>>>>> +++ b/system/vl.c
>>>>> @@ -2346,6 +2346,7 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>>>>> goto bad;
>>>>> }
>>>>> + object_set_accelerator_compat_props(ac->compat_props);
>>>>> acs->accel = accel;
>>>>> return 1;
>>>>> @@ -3728,29 +3729,14 @@ void qemu_init(int argc, char **argv)
>>>>> parse_memory_options();
>>>>> qemu_create_machine(machine_opts_dict);
>>>>> -
>>>>> - suspend_mux_open();
>>>>> -
>>>>> - qemu_disable_default_devices();
>>>>> - qemu_setup_display();
>>>>> - qemu_create_default_devices();
>>>>> - qemu_create_early_backends();
>>>>> -
>>>>> qemu_apply_legacy_machine_options(machine_opts_dict);
>>>>> qemu_apply_machine_options(machine_opts_dict);
>>>>> qobject_unref(machine_opts_dict);
>>>>> - phase_advance(PHASE_MACHINE_CREATED);
>>>>> - /*
>>>>> - * Note: uses machine properties such as kernel-irqchip, must run
>>>>> - * after qemu_apply_machine_options.
>>>>> - */
>>>>> accel = configure_accelerators(argv[0]);
>>>>> - create_accelerator(accel);
>>>>> - phase_advance(PHASE_ACCEL_CREATED);
>>>>> /*
>>>>> - * Beware, QOM objects created before this point miss global and
>>>>> + * QOM objects created after this point see all global and
>>>>> * compat properties.
>>>>> *
>>>>> * Global properties get set up by qdev_prop_register_global(),
>>>>> @@ -3765,6 +3751,22 @@ void qemu_init(int argc, char **argv)
>>>>> * called from do_configure_accelerator().
>>>>> */
>>>>> + suspend_mux_open();
>>>>> +
>>>>> + qemu_disable_default_devices();
>>>>> + qemu_setup_display();
>>>>> + qemu_create_default_devices();
>>>>> + qemu_create_early_backends();
>>>>> +
>>>>> + phase_advance(PHASE_MACHINE_CREATED);
>>>>> +
>>>>> + /*
>>>>> + * Note: uses machine properties such as kernel-irqchip, must run
>>>>> + * after qemu_apply_machine_options.
>>>>> + */
>>>>> + create_accelerator(accel);
>>>>> + phase_advance(PHASE_ACCEL_CREATED);
>>>>> +
>>>>> machine_class = MACHINE_GET_CLASS(current_machine);
>>>>> if (!qtest_enabled() && machine_class->deprecation_reason) {
>>>>> warn_report("Machine type '%s' is deprecated: %s",
>>>>
>>>> Hi Steve,
>>>>
>>>> after this commit:
>>>>
>>>> $ QTEST_QEMU_BINARY=./qemu-system-aarch64 ./tests/qtest/xlnx-can-test
>>>> # random seed: R02Saf9b44f2d88dd417052905692ee79981
>>>> 1..5
>>>> # Start of aarch64 tests
>>>> # Start of net tests
>>>> # Start of can tests
>>>> # starting QEMU: exec ./qemu-system-aarch64 -qtest unix:/tmp/qtest-2396.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-2396.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine xlnx-zcu102 -object can-bus,id=canbus -machine canbus0=canbus -machine canbus1=canbus -accel qtest
>>>> qemu-system-aarch64: Device 'canbus' not found
>>>>
>>>> I tried briefly to figure out what the issue is, but I don't really
>>>> understand the dependencies involved. Hope you can tell us.
>>>
>>> Thanks! I forgot to define the preinit method for the qtest accelerator in patch 1.
>>> I'll verify that fixes the problem and send you a one-off patch if you want to continue
>>> testing.
>>
>> Actually that is not a problem. qtest qtest_init_accel does nothing, so preinit will do
>> nothing, so it is OK to not define preinit.
>>
>> Still looking.
>
> I understand this now. The old code worked in this order:
>
> qemu_create_early_backends()
> ... creates "-object can-bus,id=canbus"
> qemu_create_machine()
> qemu_apply_machine_options()
> applies link property "canbus0" with value canbus, finds canbus object
Now I am confused.
I think the current code does:
qemu_create_machine(machine_opts_dict);
qemu_create_early_backends();
qemu_apply_machine_options(machine_opts_dict);
Isn't the relevant part that we may only apply the machine options after
creating the early backends?
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [RFC V1 04/14] accel: set accelerator and machine props earlier
2024-10-22 8:30 ` David Hildenbrand
@ 2024-10-23 20:28 ` Steven Sistare
0 siblings, 0 replies; 63+ messages in thread
From: Steven Sistare @ 2024-10-23 20:28 UTC (permalink / raw)
To: David Hildenbrand, Fabiano Rosas, qemu-devel
Cc: Peter Xu, Marcel Apfelbaum, Eduardo Habkost,
Philippe Mathieu-Daude, Paolo Bonzini, Daniel P. Berrange,
Markus Armbruster
On 10/22/2024 4:30 AM, David Hildenbrand wrote:
> On 18.10.24 21:15, Steven Sistare wrote:
>> On 10/18/2024 11:40 AM, Steven Sistare wrote:
>>> On 10/18/2024 11:32 AM, Steven Sistare wrote:
>>>> On 10/18/2024 11:08 AM, Fabiano Rosas wrote:
>>>>> Steve Sistare <steven.sistare@oracle.com> writes:
>>>>>
>>>>>> Make all global and compat properties available before the first objects
>>>>>> are created. Set accelerator compatibility properties in
>>>>>> configure_accelerators, when the accelerator is chosen, and call
>>>>>> configure_accelerators earlier. Set machine options earlier.
>>>>>>
>>>>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>>>>> ---
>>>>>> accel/accel-system.c | 2 --
>>>>>> system/vl.c | 34 ++++++++++++++++++----------------
>>>>>> 2 files changed, 18 insertions(+), 18 deletions(-)
>>>>>>
>>>>>> diff --git a/accel/accel-system.c b/accel/accel-system.c
>>>>>> index f6c947d..c8aeae4 100644
>>>>>> --- a/accel/accel-system.c
>>>>>> +++ b/accel/accel-system.c
>>>>>> @@ -41,8 +41,6 @@ int accel_init_machine(AccelState *accel, MachineState *ms)
>>>>>> ms->accelerator = NULL;
>>>>>> *(acc->allowed) = false;
>>>>>> object_unref(OBJECT(accel));
>>>>>> - } else {
>>>>>> - object_set_accelerator_compat_props(acc->compat_props);
>>>>>> }
>>>>>> return ret;
>>>>>> }
>>>>>> diff --git a/system/vl.c b/system/vl.c
>>>>>> index b94a6b9..bca2292 100644
>>>>>> --- a/system/vl.c
>>>>>> +++ b/system/vl.c
>>>>>> @@ -2346,6 +2346,7 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>>>>>> goto bad;
>>>>>> }
>>>>>> + object_set_accelerator_compat_props(ac->compat_props);
>>>>>> acs->accel = accel;
>>>>>> return 1;
>>>>>> @@ -3728,29 +3729,14 @@ void qemu_init(int argc, char **argv)
>>>>>> parse_memory_options();
>>>>>> qemu_create_machine(machine_opts_dict);
>>>>>> -
>>>>>> - suspend_mux_open();
>>>>>> -
>>>>>> - qemu_disable_default_devices();
>>>>>> - qemu_setup_display();
>>>>>> - qemu_create_default_devices();
>>>>>> - qemu_create_early_backends();
>>>>>> -
>>>>>> qemu_apply_legacy_machine_options(machine_opts_dict);
>>>>>> qemu_apply_machine_options(machine_opts_dict);
>>>>>> qobject_unref(machine_opts_dict);
>>>>>> - phase_advance(PHASE_MACHINE_CREATED);
>>>>>> - /*
>>>>>> - * Note: uses machine properties such as kernel-irqchip, must run
>>>>>> - * after qemu_apply_machine_options.
>>>>>> - */
>>>>>> accel = configure_accelerators(argv[0]);
>>>>>> - create_accelerator(accel);
>>>>>> - phase_advance(PHASE_ACCEL_CREATED);
>>>>>> /*
>>>>>> - * Beware, QOM objects created before this point miss global and
>>>>>> + * QOM objects created after this point see all global and
>>>>>> * compat properties.
>>>>>> *
>>>>>> * Global properties get set up by qdev_prop_register_global(),
>>>>>> @@ -3765,6 +3751,22 @@ void qemu_init(int argc, char **argv)
>>>>>> * called from do_configure_accelerator().
>>>>>> */
>>>>>> + suspend_mux_open();
>>>>>> +
>>>>>> + qemu_disable_default_devices();
>>>>>> + qemu_setup_display();
>>>>>> + qemu_create_default_devices();
>>>>>> + qemu_create_early_backends();
>>>>>> +
>>>>>> + phase_advance(PHASE_MACHINE_CREATED);
>>>>>> +
>>>>>> + /*
>>>>>> + * Note: uses machine properties such as kernel-irqchip, must run
>>>>>> + * after qemu_apply_machine_options.
>>>>>> + */
>>>>>> + create_accelerator(accel);
>>>>>> + phase_advance(PHASE_ACCEL_CREATED);
>>>>>> +
>>>>>> machine_class = MACHINE_GET_CLASS(current_machine);
>>>>>> if (!qtest_enabled() && machine_class->deprecation_reason) {
>>>>>> warn_report("Machine type '%s' is deprecated: %s",
>>>>>
>>>>> Hi Steve,
>>>>>
>>>>> after this commit:
>>>>>
>>>>> $ QTEST_QEMU_BINARY=./qemu-system-aarch64 ./tests/qtest/xlnx-can-test
>>>>> # random seed: R02Saf9b44f2d88dd417052905692ee79981
>>>>> 1..5
>>>>> # Start of aarch64 tests
>>>>> # Start of net tests
>>>>> # Start of can tests
>>>>> # starting QEMU: exec ./qemu-system-aarch64 -qtest unix:/tmp/qtest-2396.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-2396.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine xlnx-zcu102 -object can-bus,id=canbus -machine canbus0=canbus -machine canbus1=canbus -accel qtest
>>>>> qemu-system-aarch64: Device 'canbus' not found
>>>>>
>>>>> I tried briefly to figure out what the issue is, but I don't really
>>>>> understand the dependencies involved. Hope you can tell us.
>>>>
>>>> Thanks! I forgot to define the preinit method for the qtest accelerator in patch 1.
>>>> I'll verify that fixes the problem and send you a one-off patch if you want to continue
>>>> testing.
>>>
>>> Actually that is not a problem. qtest qtest_init_accel does nothing, so preinit will do
>>> nothing, so it is OK to not define preinit.
>>>
>>> Still looking.
>>
>> I understand this now. The old code worked in this order:
>>
>> qemu_create_early_backends()
>> ... creates "-object can-bus,id=canbus"
>> qemu_create_machine()
>> qemu_apply_machine_options()
>> applies link property "canbus0" with value canbus, finds canbus object
>
> Now I am confused.
>
> I think the current code does:
>
> qemu_create_machine(machine_opts_dict);
> qemu_create_early_backends();
> qemu_apply_machine_options(machine_opts_dict);
>
> Isn't the relevant part that we may only apply the machine options after creating the early backends?
Yes, I showed the wrong order for the old code, but our explanations are equivalent.
Perhaps this could be fixed by moving qemu_apply_machine_options after
qemu_create_early_backends in qemu_exit_precreate. But that seems risky and
fragile, as there would be a large window of code between qemu_create_machine
and qemu_apply_machine_options where the machine has been created but the machine
options and not known.
- Steve
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [RFC V1 04/14] accel: set accelerator and machine props earlier
2024-10-17 15:14 ` [RFC V1 04/14] accel: set accelerator and machine props earlier Steve Sistare
2024-10-18 15:08 ` Fabiano Rosas
@ 2024-10-21 15:19 ` Peter Xu
2024-10-23 20:29 ` Steven Sistare
2024-10-23 16:00 ` Paolo Bonzini
2 siblings, 1 reply; 63+ messages in thread
From: Peter Xu @ 2024-10-21 15:19 UTC (permalink / raw)
To: Steve Sistare
Cc: qemu-devel, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster
On Thu, Oct 17, 2024 at 08:14:05AM -0700, Steve Sistare wrote:
> Make all global and compat properties available before the first objects
> are created. Set accelerator compatibility properties in
> configure_accelerators, when the accelerator is chosen, and call
> configure_accelerators earlier. Set machine options earlier.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
> accel/accel-system.c | 2 --
> system/vl.c | 34 ++++++++++++++++++----------------
> 2 files changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/accel/accel-system.c b/accel/accel-system.c
> index f6c947d..c8aeae4 100644
> --- a/accel/accel-system.c
> +++ b/accel/accel-system.c
> @@ -41,8 +41,6 @@ int accel_init_machine(AccelState *accel, MachineState *ms)
> ms->accelerator = NULL;
> *(acc->allowed) = false;
> object_unref(OBJECT(accel));
> - } else {
> - object_set_accelerator_compat_props(acc->compat_props);
> }
> return ret;
> }
> diff --git a/system/vl.c b/system/vl.c
> index b94a6b9..bca2292 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -2346,6 +2346,7 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
> goto bad;
> }
>
> + object_set_accelerator_compat_props(ac->compat_props);
This is the probe/preinit iterator, might be good to keep it simple to only
make the decision of choosing one accel, then move this line over to
configure_accelerators() at the end.
> acs->accel = accel;
> return 1;
>
> @@ -3728,29 +3729,14 @@ void qemu_init(int argc, char **argv)
> parse_memory_options();
>
> qemu_create_machine(machine_opts_dict);
> -
> - suspend_mux_open();
> -
> - qemu_disable_default_devices();
> - qemu_setup_display();
> - qemu_create_default_devices();
> - qemu_create_early_backends();
> -
> qemu_apply_legacy_machine_options(machine_opts_dict);
> qemu_apply_machine_options(machine_opts_dict);
> qobject_unref(machine_opts_dict);
> - phase_advance(PHASE_MACHINE_CREATED);
>
> - /*
> - * Note: uses machine properties such as kernel-irqchip, must run
> - * after qemu_apply_machine_options.
> - */
> accel = configure_accelerators(argv[0]);
> - create_accelerator(accel);
> - phase_advance(PHASE_ACCEL_CREATED);
>
> /*
> - * Beware, QOM objects created before this point miss global and
> + * QOM objects created after this point see all global and
> * compat properties.
> *
> * Global properties get set up by qdev_prop_register_global(),
> @@ -3765,6 +3751,22 @@ void qemu_init(int argc, char **argv)
> * called from do_configure_accelerator().
> */
>
> + suspend_mux_open();
> +
> + qemu_disable_default_devices();
> + qemu_setup_display();
> + qemu_create_default_devices();
> + qemu_create_early_backends();
> +
> + phase_advance(PHASE_MACHINE_CREATED);
> +
> + /*
> + * Note: uses machine properties such as kernel-irqchip, must run
> + * after qemu_apply_machine_options.
> + */
> + create_accelerator(accel);
> + phase_advance(PHASE_ACCEL_CREATED);
> +
> machine_class = MACHINE_GET_CLASS(current_machine);
> if (!qtest_enabled() && machine_class->deprecation_reason) {
> warn_report("Machine type '%s' is deprecated: %s",
> --
> 1.8.3.1
>
--
Peter Xu
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [RFC V1 04/14] accel: set accelerator and machine props earlier
2024-10-21 15:19 ` Peter Xu
@ 2024-10-23 20:29 ` Steven Sistare
0 siblings, 0 replies; 63+ messages in thread
From: Steven Sistare @ 2024-10-23 20:29 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-devel, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster
On 10/21/2024 11:19 AM, Peter Xu wrote:
> On Thu, Oct 17, 2024 at 08:14:05AM -0700, Steve Sistare wrote:
>> Make all global and compat properties available before the first objects
>> are created. Set accelerator compatibility properties in
>> configure_accelerators, when the accelerator is chosen, and call
>> configure_accelerators earlier. Set machine options earlier.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>> accel/accel-system.c | 2 --
>> system/vl.c | 34 ++++++++++++++++++----------------
>> 2 files changed, 18 insertions(+), 18 deletions(-)
>>
>> diff --git a/accel/accel-system.c b/accel/accel-system.c
>> index f6c947d..c8aeae4 100644
>> --- a/accel/accel-system.c
>> +++ b/accel/accel-system.c
>> @@ -41,8 +41,6 @@ int accel_init_machine(AccelState *accel, MachineState *ms)
>> ms->accelerator = NULL;
>> *(acc->allowed) = false;
>> object_unref(OBJECT(accel));
>> - } else {
>> - object_set_accelerator_compat_props(acc->compat_props);
>> }
>> return ret;
>> }
>> diff --git a/system/vl.c b/system/vl.c
>> index b94a6b9..bca2292 100644
>> --- a/system/vl.c
>> +++ b/system/vl.c
>> @@ -2346,6 +2346,7 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>> goto bad;
>> }
>>
>> + object_set_accelerator_compat_props(ac->compat_props);
>
> This is the probe/preinit iterator, might be good to keep it simple to only
> make the decision of choosing one accel, then move this line over to
> configure_accelerators() at the end.
It's actually simpler to leave it here. Hoisting object_set_accelerator_compat_props
would require extra code to derive the ac parameter.
- Steve
>> acs->accel = accel;
>> return 1;
>>
>> @@ -3728,29 +3729,14 @@ void qemu_init(int argc, char **argv)
>> parse_memory_options();
>>
>> qemu_create_machine(machine_opts_dict);
>> -
>> - suspend_mux_open();
>> -
>> - qemu_disable_default_devices();
>> - qemu_setup_display();
>> - qemu_create_default_devices();
>> - qemu_create_early_backends();
>> -
>> qemu_apply_legacy_machine_options(machine_opts_dict);
>> qemu_apply_machine_options(machine_opts_dict);
>> qobject_unref(machine_opts_dict);
>> - phase_advance(PHASE_MACHINE_CREATED);
>>
>> - /*
>> - * Note: uses machine properties such as kernel-irqchip, must run
>> - * after qemu_apply_machine_options.
>> - */
>> accel = configure_accelerators(argv[0]);
>> - create_accelerator(accel);
>> - phase_advance(PHASE_ACCEL_CREATED);
>>
>> /*
>> - * Beware, QOM objects created before this point miss global and
>> + * QOM objects created after this point see all global and
>> * compat properties.
>> *
>> * Global properties get set up by qdev_prop_register_global(),
>> @@ -3765,6 +3751,22 @@ void qemu_init(int argc, char **argv)
>> * called from do_configure_accelerator().
>> */
>>
>> + suspend_mux_open();
>> +
>> + qemu_disable_default_devices();
>> + qemu_setup_display();
>> + qemu_create_default_devices();
>> + qemu_create_early_backends();
>> +
>> + phase_advance(PHASE_MACHINE_CREATED);
>> +
>> + /*
>> + * Note: uses machine properties such as kernel-irqchip, must run
>> + * after qemu_apply_machine_options.
>> + */
>> + create_accelerator(accel);
>> + phase_advance(PHASE_ACCEL_CREATED);
>> +
>> machine_class = MACHINE_GET_CLASS(current_machine);
>> if (!qtest_enabled() && machine_class->deprecation_reason) {
>> warn_report("Machine type '%s' is deprecated: %s",
>> --
>> 1.8.3.1
>>
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [RFC V1 04/14] accel: set accelerator and machine props earlier
2024-10-17 15:14 ` [RFC V1 04/14] accel: set accelerator and machine props earlier Steve Sistare
2024-10-18 15:08 ` Fabiano Rosas
2024-10-21 15:19 ` Peter Xu
@ 2024-10-23 16:00 ` Paolo Bonzini
2024-10-23 17:18 ` Paolo Bonzini
2024-10-23 20:29 ` Steven Sistare
2 siblings, 2 replies; 63+ messages in thread
From: Paolo Bonzini @ 2024-10-23 16:00 UTC (permalink / raw)
To: Steve Sistare, qemu-devel
Cc: Peter Xu, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Daniel P. Berrange,
Markus Armbruster
On 10/17/24 17:14, Steve Sistare wrote:
> Make all global and compat properties available before the first objects
> are created. Set accelerator compatibility properties in
> configure_accelerators, when the accelerator is chosen, and call
> configure_accelerators earlier. Set machine options earlier.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
> accel/accel-system.c | 2 --
> system/vl.c | 34 ++++++++++++++++++----------------
> 2 files changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/accel/accel-system.c b/accel/accel-system.c
> index f6c947d..c8aeae4 100644
> --- a/accel/accel-system.c
> +++ b/accel/accel-system.c
> @@ -41,8 +41,6 @@ int accel_init_machine(AccelState *accel, MachineState *ms)
> ms->accelerator = NULL;
> *(acc->allowed) = false;
> object_unref(OBJECT(accel));
> - } else {
> - object_set_accelerator_compat_props(acc->compat_props);
> }
> return ret;
> }
> diff --git a/system/vl.c b/system/vl.c
> index b94a6b9..bca2292 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -2346,6 +2346,7 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
> goto bad;
> }
>
> + object_set_accelerator_compat_props(ac->compat_props);
> acs->accel = accel;
> return 1;
>
> @@ -3728,29 +3729,14 @@ void qemu_init(int argc, char **argv)
> parse_memory_options();
>
> qemu_create_machine(machine_opts_dict);
> -
> - suspend_mux_open();
> -
> - qemu_disable_default_devices();
> - qemu_setup_display();
> - qemu_create_default_devices();
> - qemu_create_early_backends();
> -
> qemu_apply_legacy_machine_options(machine_opts_dict);
> qemu_apply_machine_options(machine_opts_dict);
This makes it impossible to refer to a backend in a machine option from
the command line. While this is not done for now, it is not a
limitation I'd like to introduce.
Could qemu_apply_machine_options() be moved after
configure_accelerators(), and phase_advance(PHASE_MACHINE_CREATED) with it?
Paolo
> qobject_unref(machine_opts_dict);
> - phase_advance(PHASE_MACHINE_CREATED);
>
> - /*
> - * Note: uses machine properties such as kernel-irqchip, must run
> - * after qemu_apply_machine_options.
> - */
> accel = configure_accelerators(argv[0]);
> - create_accelerator(accel);
> - phase_advance(PHASE_ACCEL_CREATED);
>
> /*
> - * Beware, QOM objects created before this point miss global and
> + * QOM objects created after this point see all global and
> * compat properties.
> *
> * Global properties get set up by qdev_prop_register_global(),
> @@ -3765,6 +3751,22 @@ void qemu_init(int argc, char **argv)
> * called from do_configure_accelerator().
> */
>
> + suspend_mux_open();
> +
> + qemu_disable_default_devices();
> + qemu_setup_display();
> + qemu_create_default_devices();
> + qemu_create_early_backends();
> +
> + phase_advance(PHASE_MACHINE_CREATED);
> +
> + /*
> + * Note: uses machine properties such as kernel-irqchip, must run
> + * after qemu_apply_machine_options.
> + */
> + create_accelerator(accel);
> + phase_advance(PHASE_ACCEL_CREATED);
> +
> machine_class = MACHINE_GET_CLASS(current_machine);
> if (!qtest_enabled() && machine_class->deprecation_reason) {
> warn_report("Machine type '%s' is deprecated: %s",
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [RFC V1 04/14] accel: set accelerator and machine props earlier
2024-10-23 16:00 ` Paolo Bonzini
@ 2024-10-23 17:18 ` Paolo Bonzini
2024-10-23 20:29 ` Steven Sistare
1 sibling, 0 replies; 63+ messages in thread
From: Paolo Bonzini @ 2024-10-23 17:18 UTC (permalink / raw)
To: Steve Sistare, qemu-devel
Cc: Peter Xu, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Daniel P. Berrange,
Markus Armbruster
On Wed, Oct 23, 2024 at 6:00 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
> This makes it impossible to refer to a backend in a machine option from
> the command line. While this is not done for now, it is not a
> limitation I'd like to introduce.
I stand corrected; it's exactly what's happening with canbus.
Paolo
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [RFC V1 04/14] accel: set accelerator and machine props earlier
2024-10-23 16:00 ` Paolo Bonzini
2024-10-23 17:18 ` Paolo Bonzini
@ 2024-10-23 20:29 ` Steven Sistare
1 sibling, 0 replies; 63+ messages in thread
From: Steven Sistare @ 2024-10-23 20:29 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
Cc: Peter Xu, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Daniel P. Berrange,
Markus Armbruster
On 10/23/2024 12:00 PM, Paolo Bonzini wrote:
> On 10/17/24 17:14, Steve Sistare wrote:
>> Make all global and compat properties available before the first objects
>> are created. Set accelerator compatibility properties in
>> configure_accelerators, when the accelerator is chosen, and call
>> configure_accelerators earlier. Set machine options earlier.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>> accel/accel-system.c | 2 --
>> system/vl.c | 34 ++++++++++++++++++----------------
>> 2 files changed, 18 insertions(+), 18 deletions(-)
>>
>> diff --git a/accel/accel-system.c b/accel/accel-system.c
>> index f6c947d..c8aeae4 100644
>> --- a/accel/accel-system.c
>> +++ b/accel/accel-system.c
>> @@ -41,8 +41,6 @@ int accel_init_machine(AccelState *accel, MachineState *ms)
>> ms->accelerator = NULL;
>> *(acc->allowed) = false;
>> object_unref(OBJECT(accel));
>> - } else {
>> - object_set_accelerator_compat_props(acc->compat_props);
>> }
>> return ret;
>> }
>> diff --git a/system/vl.c b/system/vl.c
>> index b94a6b9..bca2292 100644
>> --- a/system/vl.c
>> +++ b/system/vl.c
>> @@ -2346,6 +2346,7 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>> goto bad;
>> }
>> + object_set_accelerator_compat_props(ac->compat_props);
>> acs->accel = accel;
>> return 1;
>> @@ -3728,29 +3729,14 @@ void qemu_init(int argc, char **argv)
>> parse_memory_options();
>> qemu_create_machine(machine_opts_dict);
>> -
>> - suspend_mux_open();
>> -
>> - qemu_disable_default_devices();
>> - qemu_setup_display();
>> - qemu_create_default_devices();
>> - qemu_create_early_backends();
>> -
>> qemu_apply_legacy_machine_options(machine_opts_dict);
>> qemu_apply_machine_options(machine_opts_dict);
>
> This makes it impossible to refer to a backend in a machine option from the command line. While this is not done for now, it is not a limitation I'd like to introduce.
>
> Could qemu_apply_machine_options() be moved after configure_accelerators(),
Yes, but why? I don't see any dependency between the two.
Keeping the machine and its options together would be nice.
> and phase_advance(PHASE_MACHINE_CREATED) with it?
Yes, I should phase_advance to PHASE_MACHINE_CREATED right after qemu_apply_machine_options.
I added PHASE_MACHINE_CREATED in qemu_exit_precreate for no good reason.
Yielding:
qemu_create_machine_objects(); // TBD
qemu_create_machine(machine_opts_dict);
qemu_apply_legacy_machine_options(machine_opts_dict);
qemu_apply_machine_options(machine_opts_dict);
qobject_unref(machine_opts_dict);
phase_advance(PHASE_MACHINE_CREATED)
accel = configure_accelerators(argv[0]);
qemu_create_machine_objects is my proposed fix for the canbus dependency,
which I know you are not keen on.
- Steve
>> qobject_unref(machine_opts_dict);
>> - phase_advance(PHASE_MACHINE_CREATED);
>> - /*
>> - * Note: uses machine properties such as kernel-irqchip, must run
>> - * after qemu_apply_machine_options.
>> - */
>> accel = configure_accelerators(argv[0]);
>> - create_accelerator(accel);
>> - phase_advance(PHASE_ACCEL_CREATED);
>> /*
>> - * Beware, QOM objects created before this point miss global and
>> + * QOM objects created after this point see all global and
>> * compat properties.
>> *
>> * Global properties get set up by qdev_prop_register_global(),
>> @@ -3765,6 +3751,22 @@ void qemu_init(int argc, char **argv)
>> * called from do_configure_accelerator().
>> */
>> + suspend_mux_open();
>> +
>> + qemu_disable_default_devices();
>> + qemu_setup_display();
>> + qemu_create_default_devices();
>> + qemu_create_early_backends();
>> +
>> + phase_advance(PHASE_MACHINE_CREATED);
>> +
>> + /*
>> + * Note: uses machine properties such as kernel-irqchip, must run
>> + * after qemu_apply_machine_options.
>> + */
>> + create_accelerator(accel);
>> + phase_advance(PHASE_ACCEL_CREATED);
>> +
>> machine_class = MACHINE_GET_CLASS(current_machine);
>> if (!qtest_enabled() && machine_class->deprecation_reason) {
>> warn_report("Machine type '%s' is deprecated: %s",
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* [RFC V1 05/14] migration: init and listen during precreate
2024-10-17 15:14 [RFC V1 00/14] precreate phase Steve Sistare
` (3 preceding siblings ...)
2024-10-17 15:14 ` [RFC V1 04/14] accel: set accelerator and machine props earlier Steve Sistare
@ 2024-10-17 15:14 ` Steve Sistare
2024-10-21 16:41 ` Peter Xu
2024-10-21 21:05 ` Fabiano Rosas
2024-10-17 15:14 ` [RFC V1 06/14] vl: precreate phase Steve Sistare
` (10 subsequent siblings)
15 siblings, 2 replies; 63+ messages in thread
From: Steve Sistare @ 2024-10-17 15:14 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster, Steve Sistare
Initialize the migration object as early as possible so that migration
configuration commands may be sent during the precreate phase. Also,
start listening for the incoming migration connection during precreate,
so that the listen port number is assigned (if dynamic), and the user
can discover it during precreate via query-migrate. The precreate phase
will be delineated in a subsequent patch.
The code previously called migration_object_init after memory backends
were created so that a subsequent migrate-set-capabilities call to set
MIGRATION_CAPABILITY_POSTCOPY_RAM would verify all backends support
postcopy. See migrate_caps_check and postcopy_ram_supported_by_host.
The new code calls migration_object_init before backends are created.
However, migrate-set-capabilities will only be received during the
precreate phase for CPR, and CPR does not support postcopy. If the
precreate phase is generalized in the future, then the ram compatibility
check must be deferred to the start of migration.
Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
system/vl.c | 35 +++++++++++++----------------------
1 file changed, 13 insertions(+), 22 deletions(-)
diff --git a/system/vl.c b/system/vl.c
index bca2292..d32203c 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -2753,17 +2753,7 @@ void qmp_x_exit_preconfig(Error **errp)
replay_vmstate_init();
}
- if (incoming) {
- Error *local_err = NULL;
- if (strcmp(incoming, "defer") != 0) {
- qmp_migrate_incoming(incoming, false, NULL, true, true,
- &local_err);
- if (local_err) {
- error_reportf_err(local_err, "-incoming %s: ", incoming);
- exit(1);
- }
- }
- } else if (autostart) {
+ if (!incoming && autostart) {
qmp_cont(NULL);
}
}
@@ -3751,6 +3741,18 @@ void qemu_init(int argc, char **argv)
* called from do_configure_accelerator().
*/
+ /* Creates a QOM object */
+ migration_object_init();
+
+ if (incoming && !g_str_equal(incoming, "defer")) {
+ Error *local_err = NULL;
+ qmp_migrate_incoming(incoming, false, NULL, true, true, &local_err);
+ if (local_err) {
+ error_reportf_err(local_err, "-incoming %s: ", incoming);
+ exit(1);
+ }
+ }
+
suspend_mux_open();
qemu_disable_default_devices();
@@ -3773,20 +3775,9 @@ void qemu_init(int argc, char **argv)
machine_class->name, machine_class->deprecation_reason);
}
- /*
- * Create backends before creating migration objects, so that it can
- * check against compatibilities on the backend memories (e.g. postcopy
- * over memory-backend-file objects).
- */
qemu_create_late_backends();
phase_advance(PHASE_LATE_BACKENDS_CREATED);
- /*
- * Note: creates a QOM object, must run only after global and
- * compat properties have been set up.
- */
- migration_object_init();
-
/* parse features once if machine provides default cpu_type */
current_machine->cpu_type = machine_class_default_cpu_type(machine_class);
if (cpu_option) {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 63+ messages in thread* Re: [RFC V1 05/14] migration: init and listen during precreate
2024-10-17 15:14 ` [RFC V1 05/14] migration: init and listen during precreate Steve Sistare
@ 2024-10-21 16:41 ` Peter Xu
2024-10-21 21:05 ` Fabiano Rosas
1 sibling, 0 replies; 63+ messages in thread
From: Peter Xu @ 2024-10-21 16:41 UTC (permalink / raw)
To: Steve Sistare
Cc: qemu-devel, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster
On Thu, Oct 17, 2024 at 08:14:06AM -0700, Steve Sistare wrote:
> Initialize the migration object as early as possible so that migration
> configuration commands may be sent during the precreate phase. Also,
> start listening for the incoming migration connection during precreate,
> so that the listen port number is assigned (if dynamic), and the user
> can discover it during precreate via query-migrate. The precreate phase
> will be delineated in a subsequent patch.
>
> The code previously called migration_object_init after memory backends
> were created so that a subsequent migrate-set-capabilities call to set
> MIGRATION_CAPABILITY_POSTCOPY_RAM would verify all backends support
> postcopy. See migrate_caps_check and postcopy_ram_supported_by_host.
> The new code calls migration_object_init before backends are created.
> However, migrate-set-capabilities will only be received during the
> precreate phase for CPR, and CPR does not support postcopy. If the
> precreate phase is generalized in the future, then the ram compatibility
> check must be deferred to the start of migration.
This makes sense to me on its own, so maybe we can have a seperate patch.
We should probably always do the check at start of migration, to avoid
postcopy-ram cap set followed by an memory plug which doesn't support
postcopy.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
> system/vl.c | 35 +++++++++++++----------------------
> 1 file changed, 13 insertions(+), 22 deletions(-)
>
> diff --git a/system/vl.c b/system/vl.c
> index bca2292..d32203c 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -2753,17 +2753,7 @@ void qmp_x_exit_preconfig(Error **errp)
> replay_vmstate_init();
> }
>
> - if (incoming) {
> - Error *local_err = NULL;
> - if (strcmp(incoming, "defer") != 0) {
> - qmp_migrate_incoming(incoming, false, NULL, true, true,
> - &local_err);
> - if (local_err) {
> - error_reportf_err(local_err, "-incoming %s: ", incoming);
> - exit(1);
> - }
> - }
> - } else if (autostart) {
> + if (!incoming && autostart) {
> qmp_cont(NULL);
> }
> }
> @@ -3751,6 +3741,18 @@ void qemu_init(int argc, char **argv)
> * called from do_configure_accelerator().
> */
>
> + /* Creates a QOM object */
Shall we still keep the ordering notes for global/compat properties to be
set before this one? "create QOM object" on its own isn't much of help as
a comment if the function has a proper name..
> + migration_object_init();
> +
> + if (incoming && !g_str_equal(incoming, "defer")) {
> + Error *local_err = NULL;
> + qmp_migrate_incoming(incoming, false, NULL, true, true, &local_err);
> + if (local_err) {
> + error_reportf_err(local_err, "-incoming %s: ", incoming);
> + exit(1);
> + }
> + }
> +
> suspend_mux_open();
>
> qemu_disable_default_devices();
> @@ -3773,20 +3775,9 @@ void qemu_init(int argc, char **argv)
> machine_class->name, machine_class->deprecation_reason);
> }
>
> - /*
> - * Create backends before creating migration objects, so that it can
> - * check against compatibilities on the backend memories (e.g. postcopy
> - * over memory-backend-file objects).
> - */
(so if there'll be a separate patch to delay postcopy ram check on src,
this removal can be part of it)
> qemu_create_late_backends();
> phase_advance(PHASE_LATE_BACKENDS_CREATED);
>
> - /*
> - * Note: creates a QOM object, must run only after global and
> - * compat properties have been set up.
> - */
> - migration_object_init();
> -
> /* parse features once if machine provides default cpu_type */
> current_machine->cpu_type = machine_class_default_cpu_type(machine_class);
> if (cpu_option) {
> --
> 1.8.3.1
>
--
Peter Xu
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [RFC V1 05/14] migration: init and listen during precreate
2024-10-17 15:14 ` [RFC V1 05/14] migration: init and listen during precreate Steve Sistare
2024-10-21 16:41 ` Peter Xu
@ 2024-10-21 21:05 ` Fabiano Rosas
2024-10-23 16:01 ` Steven Sistare
1 sibling, 1 reply; 63+ messages in thread
From: Fabiano Rosas @ 2024-10-21 21:05 UTC (permalink / raw)
To: Steve Sistare, qemu-devel
Cc: Peter Xu, David Hildenbrand, Marcel Apfelbaum, Eduardo Habkost,
Philippe Mathieu-Daude, Paolo Bonzini, Daniel P. Berrange,
Markus Armbruster, Steve Sistare
Steve Sistare <steven.sistare@oracle.com> writes:
> Initialize the migration object as early as possible so that migration
> configuration commands may be sent during the precreate phase. Also,
> start listening for the incoming migration connection during precreate,
> so that the listen port number is assigned (if dynamic), and the user
> can discover it during precreate via query-migrate. The precreate phase
> will be delineated in a subsequent patch.
>
> The code previously called migration_object_init after memory backends
> were created so that a subsequent migrate-set-capabilities call to set
> MIGRATION_CAPABILITY_POSTCOPY_RAM would verify all backends support
> postcopy. See migrate_caps_check and postcopy_ram_supported_by_host.
> The new code calls migration_object_init before backends are created.
> However, migrate-set-capabilities will only be received during the
> precreate phase for CPR, and CPR does not support postcopy. If the
> precreate phase is generalized in the future, then the ram compatibility
> check must be deferred to the start of migration.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
> system/vl.c | 35 +++++++++++++----------------------
> 1 file changed, 13 insertions(+), 22 deletions(-)
>
> diff --git a/system/vl.c b/system/vl.c
> index bca2292..d32203c 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -2753,17 +2753,7 @@ void qmp_x_exit_preconfig(Error **errp)
> replay_vmstate_init();
> }
>
> - if (incoming) {
> - Error *local_err = NULL;
> - if (strcmp(incoming, "defer") != 0) {
> - qmp_migrate_incoming(incoming, false, NULL, true, true,
> - &local_err);
> - if (local_err) {
> - error_reportf_err(local_err, "-incoming %s: ", incoming);
> - exit(1);
> - }
> - }
> - } else if (autostart) {
> + if (!incoming && autostart) {
> qmp_cont(NULL);
> }
> }
> @@ -3751,6 +3741,18 @@ void qemu_init(int argc, char **argv)
> * called from do_configure_accelerator().
> */
>
> + /* Creates a QOM object */
> + migration_object_init();
> +
> + if (incoming && !g_str_equal(incoming, "defer")) {
> + Error *local_err = NULL;
> + qmp_migrate_incoming(incoming, false, NULL, true, true, &local_err);
> + if (local_err) {
> + error_reportf_err(local_err, "-incoming %s: ", incoming);
> + exit(1);
> + }
> + }
Doesn't this break preconfig? Now migrate_incoming happens without user
input so there's no time to set migration options before incoming code
starts using them.
> +
> suspend_mux_open();
>
> qemu_disable_default_devices();
> @@ -3773,20 +3775,9 @@ void qemu_init(int argc, char **argv)
> machine_class->name, machine_class->deprecation_reason);
> }
>
> - /*
> - * Create backends before creating migration objects, so that it can
> - * check against compatibilities on the backend memories (e.g. postcopy
> - * over memory-backend-file objects).
> - */
> qemu_create_late_backends();
> phase_advance(PHASE_LATE_BACKENDS_CREATED);
>
> - /*
> - * Note: creates a QOM object, must run only after global and
> - * compat properties have been set up.
> - */
> - migration_object_init();
> -
> /* parse features once if machine provides default cpu_type */
> current_machine->cpu_type = machine_class_default_cpu_type(machine_class);
> if (cpu_option) {
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [RFC V1 05/14] migration: init and listen during precreate
2024-10-21 21:05 ` Fabiano Rosas
@ 2024-10-23 16:01 ` Steven Sistare
0 siblings, 0 replies; 63+ messages in thread
From: Steven Sistare @ 2024-10-23 16:01 UTC (permalink / raw)
To: Fabiano Rosas, qemu-devel
Cc: Peter Xu, David Hildenbrand, Marcel Apfelbaum, Eduardo Habkost,
Philippe Mathieu-Daude, Paolo Bonzini, Daniel P. Berrange,
Markus Armbruster
On 10/21/2024 5:05 PM, Fabiano Rosas wrote:
> Steve Sistare <steven.sistare@oracle.com> writes:
>
>> Initialize the migration object as early as possible so that migration
>> configuration commands may be sent during the precreate phase. Also,
>> start listening for the incoming migration connection during precreate,
>> so that the listen port number is assigned (if dynamic), and the user
>> can discover it during precreate via query-migrate. The precreate phase
>> will be delineated in a subsequent patch.
>>
>> The code previously called migration_object_init after memory backends
>> were created so that a subsequent migrate-set-capabilities call to set
>> MIGRATION_CAPABILITY_POSTCOPY_RAM would verify all backends support
>> postcopy. See migrate_caps_check and postcopy_ram_supported_by_host.
>> The new code calls migration_object_init before backends are created.
>> However, migrate-set-capabilities will only be received during the
>> precreate phase for CPR, and CPR does not support postcopy. If the
>> precreate phase is generalized in the future, then the ram compatibility
>> check must be deferred to the start of migration.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>> system/vl.c | 35 +++++++++++++----------------------
>> 1 file changed, 13 insertions(+), 22 deletions(-)
>>
>> diff --git a/system/vl.c b/system/vl.c
>> index bca2292..d32203c 100644
>> --- a/system/vl.c
>> +++ b/system/vl.c
>> @@ -2753,17 +2753,7 @@ void qmp_x_exit_preconfig(Error **errp)
>> replay_vmstate_init();
>> }
>>
>> - if (incoming) {
>> - Error *local_err = NULL;
>> - if (strcmp(incoming, "defer") != 0) {
>> - qmp_migrate_incoming(incoming, false, NULL, true, true,
>> - &local_err);
>> - if (local_err) {
>> - error_reportf_err(local_err, "-incoming %s: ", incoming);
>> - exit(1);
>> - }
>> - }
>> - } else if (autostart) {
>> + if (!incoming && autostart) {
>> qmp_cont(NULL);
>> }
>> }
>> @@ -3751,6 +3741,18 @@ void qemu_init(int argc, char **argv)
>> * called from do_configure_accelerator().
>> */
>>
>> + /* Creates a QOM object */
>> + migration_object_init();
>> +
>> + if (incoming && !g_str_equal(incoming, "defer")) {
>> + Error *local_err = NULL;
>> + qmp_migrate_incoming(incoming, false, NULL, true, true, &local_err);
>> + if (local_err) {
>> + error_reportf_err(local_err, "-incoming %s: ", incoming);
>> + exit(1);
>> + }
>> + }
>
> Doesn't this break preconfig? Now migrate_incoming happens without user
> input so there's no time to set migration options before incoming code
> starts using them.
This branch is never taken for preconfig, because preconfig requires defer:
qemu_validate_options()
if (incoming && preconfig_requested && strcmp(incoming, "defer") != 0) {
error_report("'preconfig' supports '-incoming defer' only");
- Steve
>> +
>> suspend_mux_open();
>>
>> qemu_disable_default_devices();
>> @@ -3773,20 +3775,9 @@ void qemu_init(int argc, char **argv)
>> machine_class->name, machine_class->deprecation_reason);
>> }
>>
>> - /*
>> - * Create backends before creating migration objects, so that it can
>> - * check against compatibilities on the backend memories (e.g. postcopy
>> - * over memory-backend-file objects).
>> - */
>> qemu_create_late_backends();
>> phase_advance(PHASE_LATE_BACKENDS_CREATED);
>>
>> - /*
>> - * Note: creates a QOM object, must run only after global and
>> - * compat properties have been set up.
>> - */
>> - migration_object_init();
>> -
>> /* parse features once if machine provides default cpu_type */
>> current_machine->cpu_type = machine_class_default_cpu_type(machine_class);
>> if (cpu_option) {
^ permalink raw reply [flat|nested] 63+ messages in thread
* [RFC V1 06/14] vl: precreate phase
2024-10-17 15:14 [RFC V1 00/14] precreate phase Steve Sistare
` (4 preceding siblings ...)
2024-10-17 15:14 ` [RFC V1 05/14] migration: init and listen during precreate Steve Sistare
@ 2024-10-17 15:14 ` Steve Sistare
2024-10-23 14:03 ` Fabiano Rosas
2024-10-17 15:14 ` [RFC V1 07/14] monitor: chardev name Steve Sistare
` (9 subsequent siblings)
15 siblings, 1 reply; 63+ messages in thread
From: Steve Sistare @ 2024-10-17 15:14 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster, Steve Sistare
Refactor qemu_init into actions performed during the precreate phase,
and actions performed when exiting precreate. For now, always exit
the precreate phase immediately at init time. Future patches will add
conditions that cause QEMU to linger in the precreate phase while running
the main loop.
Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
system/vl.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/system/vl.c b/system/vl.c
index d32203c..5f5e810 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -183,6 +183,7 @@ static bool list_data_dirs;
static const char *qtest_chrdev;
static const char *qtest_log;
static AccelState *accel;
+static FILE *vmstate_dump_file;
static int has_defaults = 1;
static int default_audio = 1;
@@ -2731,6 +2732,8 @@ static bool qemu_machine_creation_done(Error **errp)
return true;
}
+static void qemu_exit_precreate(void);
+
void qmp_x_exit_preconfig(Error **errp)
{
if (phase_check(PHASE_MACHINE_INITIALIZED)) {
@@ -2795,9 +2798,7 @@ void qemu_init(int argc, char **argv)
QemuOptsList *olist;
int optind;
const char *optarg;
- MachineClass *machine_class;
bool userconfig = true;
- FILE *vmstate_dump_file = NULL;
qemu_add_opts(&qemu_drive_opts);
qemu_add_drive_opts(&qemu_legacy_drive_opts);
@@ -3753,6 +3754,13 @@ void qemu_init(int argc, char **argv)
}
}
+ qemu_exit_precreate();
+}
+
+static void qemu_exit_precreate(void)
+{
+ MachineClass *machine_class;
+
suspend_mux_open();
qemu_disable_default_devices();
--
1.8.3.1
^ permalink raw reply related [flat|nested] 63+ messages in thread* Re: [RFC V1 06/14] vl: precreate phase
2024-10-17 15:14 ` [RFC V1 06/14] vl: precreate phase Steve Sistare
@ 2024-10-23 14:03 ` Fabiano Rosas
0 siblings, 0 replies; 63+ messages in thread
From: Fabiano Rosas @ 2024-10-23 14:03 UTC (permalink / raw)
To: Steve Sistare, qemu-devel
Cc: Peter Xu, David Hildenbrand, Marcel Apfelbaum, Eduardo Habkost,
Philippe Mathieu-Daude, Paolo Bonzini, Daniel P. Berrange,
Markus Armbruster, Steve Sistare
Steve Sistare <steven.sistare@oracle.com> writes:
> Refactor qemu_init into actions performed during the precreate phase,
> and actions performed when exiting precreate. For now, always exit
> the precreate phase immediately at init time. Future patches will add
> conditions that cause QEMU to linger in the precreate phase while running
> the main loop.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
^ permalink raw reply [flat|nested] 63+ messages in thread
* [RFC V1 07/14] monitor: chardev name
2024-10-17 15:14 [RFC V1 00/14] precreate phase Steve Sistare
` (5 preceding siblings ...)
2024-10-17 15:14 ` [RFC V1 06/14] vl: precreate phase Steve Sistare
@ 2024-10-17 15:14 ` Steve Sistare
2024-10-17 15:14 ` [RFC V1 08/14] qom: get properties Steve Sistare
` (8 subsequent siblings)
15 siblings, 0 replies; 63+ messages in thread
From: Steve Sistare @ 2024-10-17 15:14 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster, Steve Sistare
Define an accessor that returns the chardev name in monitor options.
Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
include/monitor/monitor.h | 1 +
monitor/monitor.c | 21 +++++++++++++++++++++
2 files changed, 22 insertions(+)
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index c3740ec..8edfd12 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -23,6 +23,7 @@ void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp);
void monitor_init_hmp(Chardev *chr, bool use_readline, Error **errp);
int monitor_init(MonitorOptions *opts, bool allow_hmp, Error **errp);
int monitor_init_opts(QemuOpts *opts, Error **errp);
+int monitor_chardev_name(QemuOpts *opts, char **name, Error **errp);
void monitor_cleanup(void);
int monitor_suspend(Monitor *mon);
diff --git a/monitor/monitor.c b/monitor/monitor.c
index db52a9c..8b356b6 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -777,6 +777,27 @@ int monitor_init_opts(QemuOpts *opts, Error **errp)
return ret;
}
+int monitor_chardev_name(QemuOpts *opts, char **namep, Error **errp)
+{
+ Visitor *v = opts_visitor_new(opts);
+ MonitorOptions *options;
+
+ visit_type_MonitorOptions(v, NULL, &options, errp);
+ visit_free(v);
+ if (!options) {
+ return -1;
+ }
+
+ if (options->chardev) {
+ *namep = g_strdup(options->chardev);
+ } else {
+ *namep = NULL;
+ }
+
+ qapi_free_MonitorOptions(options);
+ return 0;
+}
+
QemuOptsList qemu_mon_opts = {
.name = "mon",
.implied_opt_name = "chardev",
--
1.8.3.1
^ permalink raw reply related [flat|nested] 63+ messages in thread* [RFC V1 08/14] qom: get properties
2024-10-17 15:14 [RFC V1 00/14] precreate phase Steve Sistare
` (6 preceding siblings ...)
2024-10-17 15:14 ` [RFC V1 07/14] monitor: chardev name Steve Sistare
@ 2024-10-17 15:14 ` Steve Sistare
2024-10-17 15:14 ` [RFC V1 09/14] qemu-option: filtered foreach Steve Sistare
` (7 subsequent siblings)
15 siblings, 0 replies; 63+ messages in thread
From: Steve Sistare @ 2024-10-17 15:14 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster, Steve Sistare
Extract a subroutine from user_creatable_add_qapi that converts object
options to a dictionary of properties and returns them. Use g_autoptr
to simplify the code. No functional change.
Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
include/qapi/visitor.h | 1 +
include/qom/object_interfaces.h | 8 ++++++++
qom/object_interfaces.c | 27 ++++++++++++++-------------
3 files changed, 23 insertions(+), 13 deletions(-)
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 27b85d4..640b941 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -268,6 +268,7 @@ void visit_complete(Visitor *v, void *opaque);
*/
void visit_free(Visitor *v);
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(Visitor, visit_free)
/*** Visiting structures ***/
diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index 02b11a7..2384263 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -98,6 +98,14 @@ Object *user_creatable_add_type(const char *type, const char *id,
void user_creatable_add_qapi(ObjectOptions *options, Error **errp);
/**
+ * user_creatable_get_props:
+ * @options: the object definition
+ *
+ * Convert @options to a dictionary of properties and return it.
+ */
+QDict *user_creatable_get_props(ObjectOptions *options);
+
+/**
* user_creatable_parse_str:
* @str: the object definition string as passed on the command line
* @errp: if an error occurs, a pointer to an area to store the error
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index e0833c8..104d75c 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -139,26 +139,27 @@ out:
void user_creatable_add_qapi(ObjectOptions *options, Error **errp)
{
- Visitor *v;
- QObject *qobj;
- QDict *props;
- Object *obj;
+ g_autoptr(Visitor) v;
+ g_autoptr(Object) obj;
+ g_autoptr(QDict) props = user_creatable_get_props(options);
- v = qobject_output_visitor_new(&qobj);
- visit_type_ObjectOptions(v, NULL, &options, &error_abort);
- visit_complete(v, &qobj);
- visit_free(v);
-
- props = qobject_to(QDict, qobj);
qdict_del(props, "qom-type");
qdict_del(props, "id");
v = qobject_input_visitor_new(QOBJECT(props));
obj = user_creatable_add_type(ObjectType_str(options->qom_type),
options->id, props, v, errp);
- object_unref(obj);
- qobject_unref(qobj);
- visit_free(v);
+}
+
+QDict *user_creatable_get_props(ObjectOptions *options)
+{
+ g_autoptr(Visitor) v;
+ QObject *qobj;
+
+ v = qobject_output_visitor_new(&qobj);
+ visit_type_ObjectOptions(v, NULL, &options, &error_abort);
+ visit_complete(v, &qobj);
+ return qobject_to(QDict, qobj);
}
char *object_property_help(const char *name, const char *type,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 63+ messages in thread* [RFC V1 09/14] qemu-option: filtered foreach
2024-10-17 15:14 [RFC V1 00/14] precreate phase Steve Sistare
` (7 preceding siblings ...)
2024-10-17 15:14 ` [RFC V1 08/14] qom: get properties Steve Sistare
@ 2024-10-17 15:14 ` Steve Sistare
2024-10-17 15:14 ` [RFC V1 10/14] qemu-options: pass object to filter Steve Sistare
` (6 subsequent siblings)
15 siblings, 0 replies; 63+ messages in thread
From: Steve Sistare @ 2024-10-17 15:14 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster, Steve Sistare
Define an accessor to loop over an options list and call a function
for each element that passes a filter function.
Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
include/qemu/option.h | 5 +++++
util/qemu-option.c | 25 +++++++++++++++++++++++++
2 files changed, 30 insertions(+)
diff --git a/include/qemu/option.h b/include/qemu/option.h
index 01e673a..fe37d4c 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -138,9 +138,14 @@ QDict *qemu_opts_to_qdict_filtered(QemuOpts *opts, QDict *qdict,
QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict);
bool qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp);
+typedef bool (*qemu_opts_filterfunc)(void *opaque, QemuOpts *opts);
typedef int (*qemu_opts_loopfunc)(void *opaque, QemuOpts *opts, Error **errp);
int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func,
void *opaque, Error **errp);
+int qemu_opts_filter_foreach(QemuOptsList *list,
+ qemu_opts_filterfunc filter,
+ qemu_opts_loopfunc func,
+ void *opaque, Error **errp);
void qemu_opts_print(QemuOpts *opts, const char *sep);
void qemu_opts_print_help(QemuOptsList *list, bool print_caption);
void qemu_opts_free(QemuOptsList *list);
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 201f7a8..f2f02d6 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -1142,6 +1142,31 @@ int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func,
return rc;
}
+int qemu_opts_filter_foreach(QemuOptsList *list,
+ qemu_opts_filterfunc filter,
+ qemu_opts_loopfunc func,
+ void *opaque, Error **errp)
+{
+ Location loc;
+ QemuOpts *opts, *next;
+ int rc = 0;
+
+ loc_push_none(&loc);
+ QTAILQ_FOREACH_SAFE(opts, &list->head, next, next) {
+ if (!filter(opaque, opts)) {
+ continue;
+ }
+ loc_restore(&opts->loc);
+ rc = func(opaque, opts, errp);
+ if (rc) {
+ break;
+ }
+ assert(!errp || !*errp);
+ }
+ loc_pop(&loc);
+ return rc;
+}
+
static size_t count_opts_list(QemuOptsList *list)
{
QemuOptDesc *desc = NULL;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 63+ messages in thread* [RFC V1 10/14] qemu-options: pass object to filter
2024-10-17 15:14 [RFC V1 00/14] precreate phase Steve Sistare
` (8 preceding siblings ...)
2024-10-17 15:14 ` [RFC V1 09/14] qemu-option: filtered foreach Steve Sistare
@ 2024-10-17 15:14 ` Steve Sistare
2024-10-17 15:14 ` [RFC V1 11/14] monitor: connect in precreate Steve Sistare
` (5 subsequent siblings)
15 siblings, 0 replies; 63+ messages in thread
From: Steve Sistare @ 2024-10-17 15:14 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster, Steve Sistare
Pass the entire options object to the foreach filter function, rather
than just the type name, so more aspects of the object can be used as
filter criteria in future patches. No functional change.
Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
system/vl.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/system/vl.c b/system/vl.c
index 5f5e810..3c592b9 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -1801,13 +1801,13 @@ static void qemu_apply_legacy_machine_options(QDict *qdict)
}
}
-static void object_option_foreach_add(bool (*type_opt_predicate)(const char *))
+static void object_option_foreach_add(
+ bool (*type_opt_predicate)(const ObjectOption *opt))
{
ObjectOption *opt, *next;
QTAILQ_FOREACH_SAFE(opt, &object_opts, next, next) {
- const char *type = ObjectType_str(opt->opts->qom_type);
- if (type_opt_predicate(type)) {
+ if (type_opt_predicate(opt)) {
user_creatable_add_qapi(opt->opts, &error_fatal);
qapi_free_ObjectOptions(opt->opts);
QTAILQ_REMOVE(&object_opts, opt, next);
@@ -1859,8 +1859,10 @@ static void object_option_parse(const char *str)
/*
* Very early object creation, before the sandbox options have been activated.
*/
-static bool object_create_pre_sandbox(const char *type)
+static bool object_create_pre_sandbox(const ObjectOption *opt)
{
+ const char *type = ObjectType_str(opt->opts->qom_type);
+
/*
* Objects should in general not get initialized "too early" without
* a reason. If you add one, state the reason in a comment!
@@ -1884,15 +1886,17 @@ static bool object_create_pre_sandbox(const char *type)
* cannot be created here, as it depends on the chardev
* already existing.
*/
-static bool object_create_early(const char *type)
+static bool object_create_early(const ObjectOption *opt)
{
+ const char *type = ObjectType_str(opt->opts->qom_type);
+
/*
* Objects should not be made "delayed" without a reason. If you
* add one, state the reason in a comment!
*/
/* Reason: already created. */
- if (object_create_pre_sandbox(type)) {
+ if (object_create_pre_sandbox(opt)) {
return false;
}
@@ -2014,9 +2018,9 @@ static void qemu_create_early_backends(void)
* The remainder of object creation happens after the
* creation of chardev, fsdev, net clients and device data types.
*/
-static bool object_create_late(const char *type)
+static bool object_create_late(const ObjectOption *opt)
{
- return !object_create_early(type) && !object_create_pre_sandbox(type);
+ return !object_create_early(opt) && !object_create_pre_sandbox(opt);
}
static void qemu_create_late_backends(void)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 63+ messages in thread* [RFC V1 11/14] monitor: connect in precreate
2024-10-17 15:14 [RFC V1 00/14] precreate phase Steve Sistare
` (9 preceding siblings ...)
2024-10-17 15:14 ` [RFC V1 10/14] qemu-options: pass object to filter Steve Sistare
@ 2024-10-17 15:14 ` Steve Sistare
2024-10-21 19:28 ` Peter Xu
2024-10-23 16:05 ` Paolo Bonzini
2024-10-17 15:14 ` [RFC V1 12/14] qtest: " Steve Sistare
` (4 subsequent siblings)
15 siblings, 2 replies; 63+ messages in thread
From: Steve Sistare @ 2024-10-17 15:14 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster, Steve Sistare
Complete monitor connections as early as possible, prior to
qemu_create_early_backends, so the user can issue commands during the
precreate phase.
Make a list of the chardev's referenced by all monitors. Create the
chardevs, then create the monitors. Exclude monitor chardevs and
monitors from the later creation phases.
Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
system/vl.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 77 insertions(+), 4 deletions(-)
diff --git a/system/vl.c b/system/vl.c
index 3c592b9..a985ab8 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -1939,6 +1939,11 @@ static bool object_create_early(const ObjectOption *opt)
return false;
}
+ /* Reason: already created. */
+ if (g_str_equal(type, "mon")) {
+ return false;
+ }
+
return true;
}
@@ -1956,6 +1961,68 @@ static void qemu_apply_machine_options(QDict *qdict)
}
}
+typedef struct NamedElement {
+ char *name;
+ QTAILQ_ENTRY(NamedElement) next;
+} NamedElement;
+
+static QTAILQ_HEAD(, NamedElement) monitor_chardevs =
+ QTAILQ_HEAD_INITIALIZER(monitor_chardevs);
+
+static void chardev_add(const char *name)
+{
+ NamedElement *elem = g_new0(NamedElement, 1);
+
+ elem->name = g_strdup(name);
+ QTAILQ_INSERT_TAIL(&monitor_chardevs, elem, next);
+}
+
+static bool chardev_find(const char *name)
+{
+ NamedElement *elem;
+
+ QTAILQ_FOREACH(elem, &monitor_chardevs, next) {
+ if (g_str_equal(elem->name, name)) {
+ return true;
+ }
+ }
+ return false;
+}
+
+static int monitor_add_chardev(void *opaque, QemuOpts *opts, Error **errp)
+{
+ g_autofree char *chardev = NULL;
+ int ret = monitor_chardev_name(opts, &chardev, errp);
+
+ if (!ret && chardev) {
+ chardev_add(chardev);
+ }
+ return ret;
+}
+
+static bool option_is_monitor_chardev(void *opaque, QemuOpts *opts)
+{
+ return chardev_find(qemu_opts_id(opts));
+}
+
+static bool option_is_not_monitor_chardev(void *opaque, QemuOpts *opts)
+{
+ return !chardev_find(qemu_opts_id(opts));
+}
+
+static void qemu_create_monitors(void)
+{
+ qemu_opts_foreach(qemu_find_opts("mon"),
+ monitor_add_chardev, NULL, &error_fatal);
+
+ qemu_opts_filter_foreach(qemu_find_opts("chardev"),
+ option_is_monitor_chardev,
+ chardev_init_func, NULL, &error_fatal);
+
+ qemu_opts_foreach(qemu_find_opts("mon"),
+ mon_init_func, NULL, &error_fatal);
+}
+
static void qemu_create_early_backends(void)
{
MachineClass *machine_class = MACHINE_GET_CLASS(current_machine);
@@ -1994,7 +2061,8 @@ static void qemu_create_early_backends(void)
/* spice must initialize before chardevs (for spicevmc and spiceport) */
qemu_spice.init();
- qemu_opts_foreach(qemu_find_opts("chardev"),
+ qemu_opts_filter_foreach(qemu_find_opts("chardev"),
+ option_is_not_monitor_chardev,
chardev_init_func, NULL, &error_fatal);
#ifdef CONFIG_VIRTFS
@@ -2020,6 +2088,11 @@ static void qemu_create_early_backends(void)
*/
static bool object_create_late(const ObjectOption *opt)
{
+ /* Reason: already created. */
+ if (g_str_equal(ObjectType_str(opt->opts->qom_type), "mon")) {
+ return false;
+ }
+
return !object_create_early(opt) && !object_create_pre_sandbox(opt);
}
@@ -2045,9 +2118,6 @@ static void qemu_create_late_backends(void)
exit(1);
}
- qemu_opts_foreach(qemu_find_opts("mon"),
- mon_init_func, NULL, &error_fatal);
-
if (foreach_device_config(DEV_SERIAL, serial_parse) < 0)
exit(1);
if (foreach_device_config(DEV_PARALLEL, parallel_parse) < 0)
@@ -3730,6 +3800,9 @@ void qemu_init(int argc, char **argv)
accel = configure_accelerators(argv[0]);
+ os_setup_signal_handling();
+ qemu_create_monitors();
+
/*
* QOM objects created after this point see all global and
* compat properties.
--
1.8.3.1
^ permalink raw reply related [flat|nested] 63+ messages in thread* Re: [RFC V1 11/14] monitor: connect in precreate
2024-10-17 15:14 ` [RFC V1 11/14] monitor: connect in precreate Steve Sistare
@ 2024-10-21 19:28 ` Peter Xu
2024-10-23 17:34 ` Steven Sistare
2024-10-23 16:05 ` Paolo Bonzini
1 sibling, 1 reply; 63+ messages in thread
From: Peter Xu @ 2024-10-21 19:28 UTC (permalink / raw)
To: Steve Sistare
Cc: qemu-devel, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster
On Thu, Oct 17, 2024 at 08:14:12AM -0700, Steve Sistare wrote:
> Complete monitor connections as early as possible, prior to
> qemu_create_early_backends, so the user can issue commands during the
> precreate phase.
>
> Make a list of the chardev's referenced by all monitors. Create the
> chardevs, then create the monitors. Exclude monitor chardevs and
> monitors from the later creation phases.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
> system/vl.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 77 insertions(+), 4 deletions(-)
>
> diff --git a/system/vl.c b/system/vl.c
> index 3c592b9..a985ab8 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -1939,6 +1939,11 @@ static bool object_create_early(const ObjectOption *opt)
> return false;
> }
>
> + /* Reason: already created. */
> + if (g_str_equal(type, "mon")) {
> + return false;
> + }
Why monitor are part of "object"s? I thought it was only registered on
qemu_find_opts("mon").
Same question to object_create_late() below.
> +
> return true;
> }
>
> @@ -1956,6 +1961,68 @@ static void qemu_apply_machine_options(QDict *qdict)
> }
> }
>
> +typedef struct NamedElement {
> + char *name;
> + QTAILQ_ENTRY(NamedElement) next;
> +} NamedElement;
> +
> +static QTAILQ_HEAD(, NamedElement) monitor_chardevs =
> + QTAILQ_HEAD_INITIALIZER(monitor_chardevs);
> +
> +static void chardev_add(const char *name)
> +{
> + NamedElement *elem = g_new0(NamedElement, 1);
> +
> + elem->name = g_strdup(name);
> + QTAILQ_INSERT_TAIL(&monitor_chardevs, elem, next);
> +}
> +
> +static bool chardev_find(const char *name)
> +{
> + NamedElement *elem;
> +
> + QTAILQ_FOREACH(elem, &monitor_chardevs, next) {
> + if (g_str_equal(elem->name, name)) {
> + return true;
> + }
> + }
> + return false;
> +}
> +
> +static int monitor_add_chardev(void *opaque, QemuOpts *opts, Error **errp)
> +{
> + g_autofree char *chardev = NULL;
> + int ret = monitor_chardev_name(opts, &chardev, errp);
> +
> + if (!ret && chardev) {
> + chardev_add(chardev);
> + }
> + return ret;
> +}
> +
> +static bool option_is_monitor_chardev(void *opaque, QemuOpts *opts)
> +{
> + return chardev_find(qemu_opts_id(opts));
> +}
> +
> +static bool option_is_not_monitor_chardev(void *opaque, QemuOpts *opts)
> +{
> + return !chardev_find(qemu_opts_id(opts));
> +}
> +
> +static void qemu_create_monitors(void)
Would be good to add some generic comment on why monitors' chardev can be
created earlier before the rest.
PS: I'm not yet sure this is required for the initial support for
cpr-transfer, as there's no chardev fds involved yet? IOW, I am curious
what happens if this code init all chardevs instead of monitor-only.
> +{
> + qemu_opts_foreach(qemu_find_opts("mon"),
> + monitor_add_chardev, NULL, &error_fatal);
> +
> + qemu_opts_filter_foreach(qemu_find_opts("chardev"),
> + option_is_monitor_chardev,
> + chardev_init_func, NULL, &error_fatal);
> +
> + qemu_opts_foreach(qemu_find_opts("mon"),
> + mon_init_func, NULL, &error_fatal);
> +}
> +
> static void qemu_create_early_backends(void)
> {
> MachineClass *machine_class = MACHINE_GET_CLASS(current_machine);
> @@ -1994,7 +2061,8 @@ static void qemu_create_early_backends(void)
> /* spice must initialize before chardevs (for spicevmc and spiceport) */
> qemu_spice.init();
>
> - qemu_opts_foreach(qemu_find_opts("chardev"),
> + qemu_opts_filter_foreach(qemu_find_opts("chardev"),
> + option_is_not_monitor_chardev,
> chardev_init_func, NULL, &error_fatal);
>
> #ifdef CONFIG_VIRTFS
> @@ -2020,6 +2088,11 @@ static void qemu_create_early_backends(void)
> */
> static bool object_create_late(const ObjectOption *opt)
> {
> + /* Reason: already created. */
> + if (g_str_equal(ObjectType_str(opt->opts->qom_type), "mon")) {
> + return false;
> + }
> +
> return !object_create_early(opt) && !object_create_pre_sandbox(opt);
> }
>
> @@ -2045,9 +2118,6 @@ static void qemu_create_late_backends(void)
> exit(1);
> }
>
> - qemu_opts_foreach(qemu_find_opts("mon"),
> - mon_init_func, NULL, &error_fatal);
> -
> if (foreach_device_config(DEV_SERIAL, serial_parse) < 0)
> exit(1);
> if (foreach_device_config(DEV_PARALLEL, parallel_parse) < 0)
> @@ -3730,6 +3800,9 @@ void qemu_init(int argc, char **argv)
>
> accel = configure_accelerators(argv[0]);
>
> + os_setup_signal_handling();
Didn't immediately see why this line. Some explanations / comments could
be helpful..
> + qemu_create_monitors();
> +
> /*
> * QOM objects created after this point see all global and
> * compat properties.
> --
> 1.8.3.1
>
--
Peter Xu
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [RFC V1 11/14] monitor: connect in precreate
2024-10-21 19:28 ` Peter Xu
@ 2024-10-23 17:34 ` Steven Sistare
0 siblings, 0 replies; 63+ messages in thread
From: Steven Sistare @ 2024-10-23 17:34 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-devel, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster
On 10/21/2024 3:28 PM, Peter Xu wrote:
> On Thu, Oct 17, 2024 at 08:14:12AM -0700, Steve Sistare wrote:
>> Complete monitor connections as early as possible, prior to
>> qemu_create_early_backends, so the user can issue commands during the
>> precreate phase.
>>
>> Make a list of the chardev's referenced by all monitors. Create the
>> chardevs, then create the monitors. Exclude monitor chardevs and
>> monitors from the later creation phases.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>> system/vl.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 77 insertions(+), 4 deletions(-)
>>
>> diff --git a/system/vl.c b/system/vl.c
>> index 3c592b9..a985ab8 100644
>> --- a/system/vl.c
>> +++ b/system/vl.c
>> @@ -1939,6 +1939,11 @@ static bool object_create_early(const ObjectOption *opt)
>> return false;
>> }
>>
>> + /* Reason: already created. */
>> + if (g_str_equal(type, "mon")) {
>> + return false;
>> + }
>
> Why monitor are part of "object"s? I thought it was only registered on
> qemu_find_opts("mon").
>
> Same question to object_create_late() below.
Thanks, my mistake, I'll delete it in both places.
>> +
>> return true;
>> }
>>
>> @@ -1956,6 +1961,68 @@ static void qemu_apply_machine_options(QDict *qdict)
>> }
>> }
>>
>> +typedef struct NamedElement {
>> + char *name;
>> + QTAILQ_ENTRY(NamedElement) next;
>> +} NamedElement;
>> +
>> +static QTAILQ_HEAD(, NamedElement) monitor_chardevs =
>> + QTAILQ_HEAD_INITIALIZER(monitor_chardevs);
>> +
>> +static void chardev_add(const char *name)
>> +{
>> + NamedElement *elem = g_new0(NamedElement, 1);
>> +
>> + elem->name = g_strdup(name);
>> + QTAILQ_INSERT_TAIL(&monitor_chardevs, elem, next);
>> +}
>> +
>> +static bool chardev_find(const char *name)
>> +{
>> + NamedElement *elem;
>> +
>> + QTAILQ_FOREACH(elem, &monitor_chardevs, next) {
>> + if (g_str_equal(elem->name, name)) {
>> + return true;
>> + }
>> + }
>> + return false;
>> +}
>> +
>> +static int monitor_add_chardev(void *opaque, QemuOpts *opts, Error **errp)
>> +{
>> + g_autofree char *chardev = NULL;
>> + int ret = monitor_chardev_name(opts, &chardev, errp);
>> +
>> + if (!ret && chardev) {
>> + chardev_add(chardev);
>> + }
>> + return ret;
>> +}
>> +
>> +static bool option_is_monitor_chardev(void *opaque, QemuOpts *opts)
>> +{
>> + return chardev_find(qemu_opts_id(opts));
>> +}
>> +
>> +static bool option_is_not_monitor_chardev(void *opaque, QemuOpts *opts)
>> +{
>> + return !chardev_find(qemu_opts_id(opts));
>> +}
>> +
>> +static void qemu_create_monitors(void)
>
> Would be good to add some generic comment on why monitors' chardev can be
> created earlier before the rest.
>
> PS: I'm not yet sure this is required for the initial support for
> cpr-transfer, as there's no chardev fds involved yet? IOW, I am curious
> what happens if this code init all chardevs instead of monitor-only.
Sure, for now I will simplify this and init all chardevs early, and add
the filter in the future series when CPR preserves chardevs.
>> +{
>> + qemu_opts_foreach(qemu_find_opts("mon"),
>> + monitor_add_chardev, NULL, &error_fatal);
>> +
>> + qemu_opts_filter_foreach(qemu_find_opts("chardev"),
>> + option_is_monitor_chardev,
>> + chardev_init_func, NULL, &error_fatal);
>> +
>> + qemu_opts_foreach(qemu_find_opts("mon"),
>> + mon_init_func, NULL, &error_fatal);
>> +}
>> +
>> static void qemu_create_early_backends(void)
>> {
>> MachineClass *machine_class = MACHINE_GET_CLASS(current_machine);
>> @@ -1994,7 +2061,8 @@ static void qemu_create_early_backends(void)
>> /* spice must initialize before chardevs (for spicevmc and spiceport) */
>> qemu_spice.init();
>>
>> - qemu_opts_foreach(qemu_find_opts("chardev"),
>> + qemu_opts_filter_foreach(qemu_find_opts("chardev"),
>> + option_is_not_monitor_chardev,
>> chardev_init_func, NULL, &error_fatal);
>>
>> #ifdef CONFIG_VIRTFS
>> @@ -2020,6 +2088,11 @@ static void qemu_create_early_backends(void)
>> */
>> static bool object_create_late(const ObjectOption *opt)
>> {
>> + /* Reason: already created. */
>> + if (g_str_equal(ObjectType_str(opt->opts->qom_type), "mon")) {
>> + return false;
>> + }
>> +
>> return !object_create_early(opt) && !object_create_pre_sandbox(opt);
>> }
>>
>> @@ -2045,9 +2118,6 @@ static void qemu_create_late_backends(void)
>> exit(1);
>> }
>>
>> - qemu_opts_foreach(qemu_find_opts("mon"),
>> - mon_init_func, NULL, &error_fatal);
>> -
>> if (foreach_device_config(DEV_SERIAL, serial_parse) < 0)
>> exit(1);
>> if (foreach_device_config(DEV_PARALLEL, parallel_parse) < 0)
>> @@ -3730,6 +3800,9 @@ void qemu_init(int argc, char **argv)
>>
>> accel = configure_accelerators(argv[0]);
>>
>> + os_setup_signal_handling();
>
> Didn't immediately see why this line. Some explanations / comments could
> be helpful..
The TERM handler must be installed before the qtest monitor connects, to
catch failure during precreate. Previously it was not installed until
qemu_init_displays -> os_setup_signal_handling. The latter is still called,
because of the comment "must be after terminal init, SDL library changes
signal handlers". It is harmless to call os_setup_signal_handling twice.
- Steve
>> + qemu_create_monitors();
>> +
>> /*
>> * QOM objects created after this point see all global and
>> * compat properties.
>> --
>> 1.8.3.1
>>
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [RFC V1 11/14] monitor: connect in precreate
2024-10-17 15:14 ` [RFC V1 11/14] monitor: connect in precreate Steve Sistare
2024-10-21 19:28 ` Peter Xu
@ 2024-10-23 16:05 ` Paolo Bonzini
2024-10-23 17:35 ` Steven Sistare
1 sibling, 1 reply; 63+ messages in thread
From: Paolo Bonzini @ 2024-10-23 16:05 UTC (permalink / raw)
To: Steve Sistare, qemu-devel
Cc: Peter Xu, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Daniel P. Berrange,
Markus Armbruster
On 10/17/24 17:14, Steve Sistare wrote:
> Complete monitor connections as early as possible, prior to
> qemu_create_early_backends, so the user can issue commands during the
> precreate phase.
>
> Make a list of the chardev's referenced by all monitors. Create the
> chardevs, then create the monitors. Exclude monitor chardevs and
> monitors from the later creation phases.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
> system/vl.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 77 insertions(+), 4 deletions(-)
>
> diff --git a/system/vl.c b/system/vl.c
> index 3c592b9..a985ab8 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -1939,6 +1939,11 @@ static bool object_create_early(const ObjectOption *opt)
> return false;
> }
>
> + /* Reason: already created. */
> + if (g_str_equal(type, "mon")) {
> + return false;
> + }
This is incorrect as mentioned by Peter.
> return true;
> }
>
> @@ -1956,6 +1961,68 @@ static void qemu_apply_machine_options(QDict *qdict)
> }
> }
>
> +typedef struct NamedElement {
> + char *name;
> + QTAILQ_ENTRY(NamedElement) next;
> +} NamedElement;
> +
> +static QTAILQ_HEAD(, NamedElement) monitor_chardevs =
> + QTAILQ_HEAD_INITIALIZER(monitor_chardevs);
> +
> +static void chardev_add(const char *name)
> +{
> + NamedElement *elem = g_new0(NamedElement, 1);
> +
> + elem->name = g_strdup(name);
> + QTAILQ_INSERT_TAIL(&monitor_chardevs, elem, next);
> +}
> +
> +static bool chardev_find(const char *name)
> +{
> + NamedElement *elem;
> +
> + QTAILQ_FOREACH(elem, &monitor_chardevs, next) {
> + if (g_str_equal(elem->name, name)) {
> + return true;
> + }
> + }
> + return false;
> +}
No new special casing and no tricky differentiation of how a single
command line option is processed. If you need to create monitors so
early, create _all_ chardevs and _all_ monitors; same for qtest.
Paolo
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [RFC V1 11/14] monitor: connect in precreate
2024-10-23 16:05 ` Paolo Bonzini
@ 2024-10-23 17:35 ` Steven Sistare
0 siblings, 0 replies; 63+ messages in thread
From: Steven Sistare @ 2024-10-23 17:35 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
Cc: Peter Xu, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Daniel P. Berrange,
Markus Armbruster
On 10/23/2024 12:05 PM, Paolo Bonzini wrote:
> On 10/17/24 17:14, Steve Sistare wrote:
>> Complete monitor connections as early as possible, prior to
>> qemu_create_early_backends, so the user can issue commands during the
>> precreate phase.
>>
>> Make a list of the chardev's referenced by all monitors. Create the
>> chardevs, then create the monitors. Exclude monitor chardevs and
>> monitors from the later creation phases.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>> system/vl.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 77 insertions(+), 4 deletions(-)
>>
>> diff --git a/system/vl.c b/system/vl.c
>> index 3c592b9..a985ab8 100644
>> --- a/system/vl.c
>> +++ b/system/vl.c
>> @@ -1939,6 +1939,11 @@ static bool object_create_early(const ObjectOption *opt)
>> return false;
>> }
>> + /* Reason: already created. */
>> + if (g_str_equal(type, "mon")) {
>> + return false;
>> + }
>
> This is incorrect as mentioned by Peter.
Got it.
>> return true;
>> }
>> @@ -1956,6 +1961,68 @@ static void qemu_apply_machine_options(QDict *qdict)
>> }
>> }
>> +typedef struct NamedElement {
>> + char *name;
>> + QTAILQ_ENTRY(NamedElement) next;
>> +} NamedElement;
>> +
>> +static QTAILQ_HEAD(, NamedElement) monitor_chardevs =
>> + QTAILQ_HEAD_INITIALIZER(monitor_chardevs);
>> +
>> +static void chardev_add(const char *name)
>> +{
>> + NamedElement *elem = g_new0(NamedElement, 1);
>> +
>> + elem->name = g_strdup(name);
>> + QTAILQ_INSERT_TAIL(&monitor_chardevs, elem, next);
>> +}
>> +
>> +static bool chardev_find(const char *name)
>> +{
>> + NamedElement *elem;
>> +
>> + QTAILQ_FOREACH(elem, &monitor_chardevs, next) {
>> + if (g_str_equal(elem->name, name)) {
>> + return true;
>> + }
>> + }
>> + return false;
>> +}
>
> No new special casing and no tricky differentiation of how a single command line option is processed. If you need to create monitors so early, create _all_ chardevs and _all_ monitors; same for qtest.
I do create all monitors. But for chardevs, I only create those needed by the monitors
and qtest. That is so CPR can be used to preserve chardevs in a future patch. But I will
defer that functionality to simplify this series, and create all chardevs early.
But hey, you have to admit the filtering patches are pretty spiffy :)
- Steve
^ permalink raw reply [flat|nested] 63+ messages in thread
* [RFC V1 12/14] qtest: connect in precreate
2024-10-17 15:14 [RFC V1 00/14] precreate phase Steve Sistare
` (10 preceding siblings ...)
2024-10-17 15:14 ` [RFC V1 11/14] monitor: connect in precreate Steve Sistare
@ 2024-10-17 15:14 ` Steve Sistare
2024-10-17 15:14 ` [RFC V1 13/14] net: cleanup for precreate phase Steve Sistare
` (3 subsequent siblings)
15 siblings, 0 replies; 63+ messages in thread
From: Steve Sistare @ 2024-10-17 15:14 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster, Steve Sistare
Initialize the qtest connection and the chardev it depends on during the
precreate phase. Handle both forms of syntax:
-object qtest,id=<id>,chardev=<name>
-qtest chardev:<name>
Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
system/vl.c | 41 +++++++++++++++++++++++++++++++++++++----
1 file changed, 37 insertions(+), 4 deletions(-)
diff --git a/system/vl.c b/system/vl.c
index a985ab8..455c693 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -1816,6 +1816,19 @@ static void object_option_foreach_add(
}
}
+static void object_option_foreach(
+ bool (*type_opt_predicate)(const ObjectOption *opt),
+ void (*func)(ObjectOptions *opts))
+{
+ ObjectOption *opt, *next;
+
+ QTAILQ_FOREACH_SAFE(opt, &object_opts, next, next) {
+ if (type_opt_predicate(opt)) {
+ func(opt->opts);
+ }
+ }
+}
+
static void object_option_add_visitor(Visitor *v)
{
ObjectOption *opt = g_new0(ObjectOption, 1);
@@ -2000,6 +2013,18 @@ static int monitor_add_chardev(void *opaque, QemuOpts *opts, Error **errp)
return ret;
}
+static void qtest_add_chardev(ObjectOptions *opts)
+{
+ g_autoptr(QDict) props = user_creatable_get_props(opts);
+ const char *chardev = qdict_get_str(props, "chardev");
+ chardev_add(chardev);
+}
+
+static bool option_is_qtest(const ObjectOption *opt)
+{
+ return g_str_equal(ObjectType_str(opt->opts->qom_type), "qtest");
+}
+
static bool option_is_monitor_chardev(void *opaque, QemuOpts *opts)
{
return chardev_find(qemu_opts_id(opts));
@@ -2012,15 +2037,27 @@ static bool option_is_not_monitor_chardev(void *opaque, QemuOpts *opts)
static void qemu_create_monitors(void)
{
+ const char *name;
+
qemu_opts_foreach(qemu_find_opts("mon"),
monitor_add_chardev, NULL, &error_fatal);
+ object_option_foreach(option_is_qtest, qtest_add_chardev);
+
+ if (qtest_chrdev && strstart(qtest_chrdev, "chardev:", &name)) {
+ chardev_add(g_strdup(name));
+ }
+
qemu_opts_filter_foreach(qemu_find_opts("chardev"),
option_is_monitor_chardev,
chardev_init_func, NULL, &error_fatal);
qemu_opts_foreach(qemu_find_opts("mon"),
mon_init_func, NULL, &error_fatal);
+
+ if (qtest_chrdev) {
+ qtest_server_init(qtest_chrdev, qtest_log, &error_fatal);
+ }
}
static void qemu_create_early_backends(void)
@@ -2098,10 +2135,6 @@ static bool object_create_late(const ObjectOption *opt)
static void qemu_create_late_backends(void)
{
- if (qtest_chrdev) {
- qtest_server_init(qtest_chrdev, qtest_log, &error_fatal);
- }
-
net_init_clients();
object_option_foreach_add(object_create_late);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 63+ messages in thread* [RFC V1 13/14] net: cleanup for precreate phase
2024-10-17 15:14 [RFC V1 00/14] precreate phase Steve Sistare
` (11 preceding siblings ...)
2024-10-17 15:14 ` [RFC V1 12/14] qtest: " Steve Sistare
@ 2024-10-17 15:14 ` Steve Sistare
2024-10-17 15:27 ` Steven Sistare
2024-10-21 19:20 ` Peter Xu
2024-10-17 15:14 ` [RFC V1 14/14] migration: allow commands during precreate and preconfig Steve Sistare
` (2 subsequent siblings)
15 siblings, 2 replies; 63+ messages in thread
From: Steve Sistare @ 2024-10-17 15:14 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster, Steve Sistare
Guard against unconfigured state if cleanup is called early, such as
during the precreate phase.
Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
net/net.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/net.c b/net/net.c
index d9b23a8..207fdb0 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1687,7 +1687,9 @@ void net_cleanup(void)
}
}
- qemu_del_vm_change_state_handler(net_change_state_entry);
+ if (net_change_state_entry) {
+ qemu_del_vm_change_state_handler(net_change_state_entry);
+ }
}
void net_check_clients(void)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 63+ messages in thread* Re: [RFC V1 13/14] net: cleanup for precreate phase
2024-10-17 15:14 ` [RFC V1 13/14] net: cleanup for precreate phase Steve Sistare
@ 2024-10-17 15:27 ` Steven Sistare
2024-10-21 19:20 ` Peter Xu
1 sibling, 0 replies; 63+ messages in thread
From: Steven Sistare @ 2024-10-17 15:27 UTC (permalink / raw)
To: Jason Wang
Cc: Peter Xu, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster, qemu-devel
cc jason.
The cover letter for this series is here:
https://lore.kernel.org/qemu-devel/1729178055-207271-1-git-send-email-steven.sistare@oracle.com
- Steve
On 10/17/2024 11:14 AM, Steve Sistare wrote:
> Guard against unconfigured state if cleanup is called early, such as
> during the precreate phase.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
> net/net.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/net.c b/net/net.c
> index d9b23a8..207fdb0 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1687,7 +1687,9 @@ void net_cleanup(void)
> }
> }
>
> - qemu_del_vm_change_state_handler(net_change_state_entry);
> + if (net_change_state_entry) {
> + qemu_del_vm_change_state_handler(net_change_state_entry);
> + }
> }
>
> void net_check_clients(void)
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [RFC V1 13/14] net: cleanup for precreate phase
2024-10-17 15:14 ` [RFC V1 13/14] net: cleanup for precreate phase Steve Sistare
2024-10-17 15:27 ` Steven Sistare
@ 2024-10-21 19:20 ` Peter Xu
2024-10-23 17:43 ` Steven Sistare
1 sibling, 1 reply; 63+ messages in thread
From: Peter Xu @ 2024-10-21 19:20 UTC (permalink / raw)
To: Steve Sistare
Cc: qemu-devel, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster
On Thu, Oct 17, 2024 at 08:14:14AM -0700, Steve Sistare wrote:
> Guard against unconfigured state if cleanup is called early, such as
> during the precreate phase.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
One nitpick..
> ---
> net/net.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/net.c b/net/net.c
> index d9b23a8..207fdb0 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1687,7 +1687,9 @@ void net_cleanup(void)
> }
> }
>
> - qemu_del_vm_change_state_handler(net_change_state_entry);
> + if (net_change_state_entry) {
> + qemu_del_vm_change_state_handler(net_change_state_entry);
Can also rest net_change_state_entry to NULL.
> + }
> }
>
> void net_check_clients(void)
> --
> 1.8.3.1
>
--
Peter Xu
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [RFC V1 13/14] net: cleanup for precreate phase
2024-10-21 19:20 ` Peter Xu
@ 2024-10-23 17:43 ` Steven Sistare
0 siblings, 0 replies; 63+ messages in thread
From: Steven Sistare @ 2024-10-23 17:43 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-devel, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster
On 10/21/2024 3:20 PM, Peter Xu wrote:
> On Thu, Oct 17, 2024 at 08:14:14AM -0700, Steve Sistare wrote:
>> Guard against unconfigured state if cleanup is called early, such as
>> during the precreate phase.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
>
> One nitpick..
>
>> ---
>> net/net.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/net.c b/net/net.c
>> index d9b23a8..207fdb0 100644
>> --- a/net/net.c
>> +++ b/net/net.c
>> @@ -1687,7 +1687,9 @@ void net_cleanup(void)
>> }
>> }
>>
>> - qemu_del_vm_change_state_handler(net_change_state_entry);
>> + if (net_change_state_entry) {
>> + qemu_del_vm_change_state_handler(net_change_state_entry);
>
> Can also rest net_change_state_entry to NULL.
Definitely - steve
^ permalink raw reply [flat|nested] 63+ messages in thread
* [RFC V1 14/14] migration: allow commands during precreate and preconfig
2024-10-17 15:14 [RFC V1 00/14] precreate phase Steve Sistare
` (12 preceding siblings ...)
2024-10-17 15:14 ` [RFC V1 13/14] net: cleanup for precreate phase Steve Sistare
@ 2024-10-17 15:14 ` Steve Sistare
2024-10-21 19:36 ` Peter Xu
2024-10-17 15:19 ` [RFC V1 00/14] precreate phase Steven Sistare
2024-10-23 16:31 ` Paolo Bonzini
15 siblings, 1 reply; 63+ messages in thread
From: Steve Sistare @ 2024-10-17 15:14 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster, Steve Sistare
Allow various migration commands during the precreate and preconfig phases
so migration may be set up and initiated at that time.
Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
hmp-commands.hx | 2 ++
qapi/migration.json | 16 +++++++++++-----
qapi/misc.json | 3 ++-
3 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 06746f0..c0f34e9 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -959,6 +959,7 @@ ERST
.params = "uri",
.help = "Continue an incoming migration from an -incoming defer",
.cmd = hmp_migrate_incoming,
+ .flags = "p",
},
SRST
@@ -1000,6 +1001,7 @@ ERST
.help = "Enable/Disable the usage of a capability for migration",
.cmd = hmp_migrate_set_capability,
.command_completion = migrate_set_capability_completion,
+ .flags = "p",
},
SRST
diff --git a/qapi/migration.json b/qapi/migration.json
index 3af6aa1..0468c07 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -373,7 +373,8 @@
# }
# }
##
-{ 'command': 'query-migrate', 'returns': 'MigrationInfo' }
+{ 'command': 'query-migrate', 'returns': 'MigrationInfo',
+ 'allow-preconfig': true }
##
# @MigrationCapability:
@@ -527,7 +528,8 @@
# <- { "return": {} }
##
{ 'command': 'migrate-set-capabilities',
- 'data': { 'capabilities': ['MigrationCapabilityStatus'] } }
+ 'data': { 'capabilities': ['MigrationCapabilityStatus'] },
+ 'allow-preconfig': true }
##
# @query-migrate-capabilities:
@@ -551,7 +553,9 @@
# {"state": false, "capability": "x-colo"}
# ]}
##
-{ 'command': 'query-migrate-capabilities', 'returns': ['MigrationCapabilityStatus']}
+{ 'command': 'query-migrate-capabilities',
+ 'returns': ['MigrationCapabilityStatus'],
+ 'allow-preconfig': true }
##
# @MultiFDCompression:
@@ -1297,7 +1301,8 @@
# }
##
{ 'command': 'query-migrate-parameters',
- 'returns': 'MigrationParameters' }
+ 'returns': 'MigrationParameters',
+ 'allow-preconfig': true }
##
# @migrate-start-postcopy:
@@ -1751,7 +1756,8 @@
{ 'command': 'migrate-incoming',
'data': {'*uri': 'str',
'*channels': [ 'MigrationChannel' ],
- '*exit-on-error': 'bool' } }
+ '*exit-on-error': 'bool' },
+ 'allow-preconfig': true }
##
# @xen-save-devices-state:
diff --git a/qapi/misc.json b/qapi/misc.json
index 559b66f..ce60493 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -241,7 +241,8 @@
{ 'command': 'human-monitor-command',
'data': {'command-line': 'str', '*cpu-index': 'int'},
'returns': 'str',
- 'features': [ 'savevm-monitor-nodes' ] }
+ 'features': [ 'savevm-monitor-nodes' ],
+ 'allow-preconfig': true }
##
# @getfd:
--
1.8.3.1
^ permalink raw reply related [flat|nested] 63+ messages in thread* Re: [RFC V1 14/14] migration: allow commands during precreate and preconfig
2024-10-17 15:14 ` [RFC V1 14/14] migration: allow commands during precreate and preconfig Steve Sistare
@ 2024-10-21 19:36 ` Peter Xu
2024-10-23 17:50 ` Steven Sistare
0 siblings, 1 reply; 63+ messages in thread
From: Peter Xu @ 2024-10-21 19:36 UTC (permalink / raw)
To: Steve Sistare
Cc: qemu-devel, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster
On Thu, Oct 17, 2024 at 08:14:15AM -0700, Steve Sistare wrote:
> Allow various migration commands during the precreate and preconfig phases
> so migration may be set up and initiated at that time.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
> hmp-commands.hx | 2 ++
> qapi/migration.json | 16 +++++++++++-----
> qapi/misc.json | 3 ++-
> 3 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 06746f0..c0f34e9 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -959,6 +959,7 @@ ERST
> .params = "uri",
> .help = "Continue an incoming migration from an -incoming defer",
> .cmd = hmp_migrate_incoming,
> + .flags = "p",
> },
>
> SRST
> @@ -1000,6 +1001,7 @@ ERST
> .help = "Enable/Disable the usage of a capability for migration",
> .cmd = hmp_migrate_set_capability,
> .command_completion = migrate_set_capability_completion,
> + .flags = "p",
> },
How about "info migrate_capabilities / migrate_parameters"?
>
> SRST
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 3af6aa1..0468c07 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -373,7 +373,8 @@
> # }
> # }
> ##
> -{ 'command': 'query-migrate', 'returns': 'MigrationInfo' }
> +{ 'command': 'query-migrate', 'returns': 'MigrationInfo',
> + 'allow-preconfig': true }
>
> ##
> # @MigrationCapability:
> @@ -527,7 +528,8 @@
> # <- { "return": {} }
> ##
> { 'command': 'migrate-set-capabilities',
> - 'data': { 'capabilities': ['MigrationCapabilityStatus'] } }
> + 'data': { 'capabilities': ['MigrationCapabilityStatus'] },
> + 'allow-preconfig': true }
migrate-set-parameters?
>
> ##
> # @query-migrate-capabilities:
> @@ -551,7 +553,9 @@
> # {"state": false, "capability": "x-colo"}
> # ]}
> ##
> -{ 'command': 'query-migrate-capabilities', 'returns': ['MigrationCapabilityStatus']}
> +{ 'command': 'query-migrate-capabilities',
> + 'returns': ['MigrationCapabilityStatus'],
> + 'allow-preconfig': true }
>
> ##
> # @MultiFDCompression:
> @@ -1297,7 +1301,8 @@
> # }
> ##
> { 'command': 'query-migrate-parameters',
> - 'returns': 'MigrationParameters' }
> + 'returns': 'MigrationParameters',
> + 'allow-preconfig': true }
>
> ##
> # @migrate-start-postcopy:
> @@ -1751,7 +1756,8 @@
> { 'command': 'migrate-incoming',
> 'data': {'*uri': 'str',
> '*channels': [ 'MigrationChannel' ],
> - '*exit-on-error': 'bool' } }
> + '*exit-on-error': 'bool' },
> + 'allow-preconfig': true }
>
> ##
> # @xen-save-devices-state:
> diff --git a/qapi/misc.json b/qapi/misc.json
> index 559b66f..ce60493 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -241,7 +241,8 @@
> { 'command': 'human-monitor-command',
> 'data': {'command-line': 'str', '*cpu-index': 'int'},
> 'returns': 'str',
> - 'features': [ 'savevm-monitor-nodes' ] }
> + 'features': [ 'savevm-monitor-nodes' ],
> + 'allow-preconfig': true }
>
> ##
> # @getfd:
> --
> 1.8.3.1
>
--
Peter Xu
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [RFC V1 14/14] migration: allow commands during precreate and preconfig
2024-10-21 19:36 ` Peter Xu
@ 2024-10-23 17:50 ` Steven Sistare
0 siblings, 0 replies; 63+ messages in thread
From: Steven Sistare @ 2024-10-23 17:50 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-devel, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster
On 10/21/2024 3:36 PM, Peter Xu wrote:
> On Thu, Oct 17, 2024 at 08:14:15AM -0700, Steve Sistare wrote:
>> Allow various migration commands during the precreate and preconfig phases
>> so migration may be set up and initiated at that time.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>> hmp-commands.hx | 2 ++
>> qapi/migration.json | 16 +++++++++++-----
>> qapi/misc.json | 3 ++-
>> 3 files changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 06746f0..c0f34e9 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -959,6 +959,7 @@ ERST
>> .params = "uri",
>> .help = "Continue an incoming migration from an -incoming defer",
>> .cmd = hmp_migrate_incoming,
>> + .flags = "p",
>> },
>>
>> SRST
>> @@ -1000,6 +1001,7 @@ ERST
>> .help = "Enable/Disable the usage of a capability for migration",
>> .cmd = hmp_migrate_set_capability,
>> .command_completion = migrate_set_capability_completion,
>> + .flags = "p",
>> },
>
> How about "info migrate_capabilities / migrate_parameters"?
That's hard to do cleanly for hmp. I would need to allow all info sub-commands,
then check and return an error for everything except migrate_capabilities and
migrate_parameters. It is not safe to allow any info command, because some of
them reference state that is not initialized yet.
>> SRST
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index 3af6aa1..0468c07 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -373,7 +373,8 @@
>> # }
>> # }
>> ##
>> -{ 'command': 'query-migrate', 'returns': 'MigrationInfo' }
>> +{ 'command': 'query-migrate', 'returns': 'MigrationInfo',
>> + 'allow-preconfig': true }
>>
>> ##
>> # @MigrationCapability:
>> @@ -527,7 +528,8 @@
>> # <- { "return": {} }
>> ##
>> { 'command': 'migrate-set-capabilities',
>> - 'data': { 'capabilities': ['MigrationCapabilityStatus'] } }
>> + 'data': { 'capabilities': ['MigrationCapabilityStatus'] },
>> + 'allow-preconfig': true }
>
> migrate-set-parameters?
Sure, I'll add it.
- Steve
>> ##
>> # @query-migrate-capabilities:
>> @@ -551,7 +553,9 @@
>> # {"state": false, "capability": "x-colo"}
>> # ]}
>> ##
>> -{ 'command': 'query-migrate-capabilities', 'returns': ['MigrationCapabilityStatus']}
>> +{ 'command': 'query-migrate-capabilities',
>> + 'returns': ['MigrationCapabilityStatus'],
>> + 'allow-preconfig': true }
>>
>> ##
>> # @MultiFDCompression:
>> @@ -1297,7 +1301,8 @@
>> # }
>> ##
>> { 'command': 'query-migrate-parameters',
>> - 'returns': 'MigrationParameters' }
>> + 'returns': 'MigrationParameters',
>> + 'allow-preconfig': true }
>>
>> ##
>> # @migrate-start-postcopy:
>> @@ -1751,7 +1756,8 @@
>> { 'command': 'migrate-incoming',
>> 'data': {'*uri': 'str',
>> '*channels': [ 'MigrationChannel' ],
>> - '*exit-on-error': 'bool' } }
>> + '*exit-on-error': 'bool' },
>> + 'allow-preconfig': true }
>>
>> ##
>> # @xen-save-devices-state:
>> diff --git a/qapi/misc.json b/qapi/misc.json
>> index 559b66f..ce60493 100644
>> --- a/qapi/misc.json
>> +++ b/qapi/misc.json
>> @@ -241,7 +241,8 @@
>> { 'command': 'human-monitor-command',
>> 'data': {'command-line': 'str', '*cpu-index': 'int'},
>> 'returns': 'str',
>> - 'features': [ 'savevm-monitor-nodes' ] }
>> + 'features': [ 'savevm-monitor-nodes' ],
>> + 'allow-preconfig': true }
>>
>> ##
>> # @getfd:
>> --
>> 1.8.3.1
>>
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [RFC V1 00/14] precreate phase
2024-10-17 15:14 [RFC V1 00/14] precreate phase Steve Sistare
` (13 preceding siblings ...)
2024-10-17 15:14 ` [RFC V1 14/14] migration: allow commands during precreate and preconfig Steve Sistare
@ 2024-10-17 15:19 ` Steven Sistare
2024-10-17 15:53 ` Peter Xu
2024-10-23 16:31 ` Paolo Bonzini
15 siblings, 1 reply; 63+ messages in thread
From: Steven Sistare @ 2024-10-17 15:19 UTC (permalink / raw)
To: Peter Xu
Cc: Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster, qemu-devel
On 10/17/2024 11:14 AM, Steve Sistare wrote:
> Define a new qemu initialization phase called 'precreate' which occurs
> before most backends or devices have been created. The only exception
> is monitor and qtest devices and their associated chardevs.
>
> QEMU runs in the main loop during this phase. Monitor connections are
> active and can receive migration configuration commands. QEMU starts
> listening on the normal migration URI during this phase, which can come
> from either the QEMU command line or from a migrate_incoming command.
> Thus the user can issue query-migrate to get the socket-address for
> dynamically allocated port numbers during precreate.
>
> In this series QEMU passes through and does not linger in the precreate
> phase, and the user sees no change in behavior. The cpr-transfer series
> will linger in the phase for an incoming CPR operation, and exit the phase
> when the migrate command is send to source QEMU and causes destination QEMU
> to read CPR state.
Hi Peter, I rebased the cpr-transfer series on precreate. The
cpr-transfer migration-test now works. Do you want to see cpr-transfer
V3 now, or wait until we get feedback on precreate? The only significant
change is that I deleted the HUP synchronization, and I post an async
listen for the incoming cpr-uri connection.
- Steve
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [RFC V1 00/14] precreate phase
2024-10-17 15:19 ` [RFC V1 00/14] precreate phase Steven Sistare
@ 2024-10-17 15:53 ` Peter Xu
2024-10-21 15:56 ` Steven Sistare
0 siblings, 1 reply; 63+ messages in thread
From: Peter Xu @ 2024-10-17 15:53 UTC (permalink / raw)
To: Steven Sistare
Cc: Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster, qemu-devel
On Thu, Oct 17, 2024 at 11:19:51AM -0400, Steven Sistare wrote:
> On 10/17/2024 11:14 AM, Steve Sistare wrote:
> > Define a new qemu initialization phase called 'precreate' which occurs
> > before most backends or devices have been created. The only exception
> > is monitor and qtest devices and their associated chardevs.
> >
> > QEMU runs in the main loop during this phase. Monitor connections are
> > active and can receive migration configuration commands. QEMU starts
> > listening on the normal migration URI during this phase, which can come
> > from either the QEMU command line or from a migrate_incoming command.
> > Thus the user can issue query-migrate to get the socket-address for
> > dynamically allocated port numbers during precreate.
> >
> > In this series QEMU passes through and does not linger in the precreate
> > phase, and the user sees no change in behavior. The cpr-transfer series
> > will linger in the phase for an incoming CPR operation, and exit the phase
> > when the migrate command is send to source QEMU and causes destination QEMU
> > to read CPR state.
>
> Hi Peter, I rebased the cpr-transfer series on precreate. The
> cpr-transfer migration-test now works. Do you want to see cpr-transfer
> V3 now, or wait until we get feedback on precreate? The only significant
> change is that I deleted the HUP synchronization, and I post an async
> listen for the incoming cpr-uri connection.
Maybe you can still send it, because I remember there're some other
discussion that may not settled yet (e.g. how anon-memfd is applied, iirc),
then it can be reviewed and discussed concurrently when proper with the
precreate series.
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [RFC V1 00/14] precreate phase
2024-10-17 15:53 ` Peter Xu
@ 2024-10-21 15:56 ` Steven Sistare
0 siblings, 0 replies; 63+ messages in thread
From: Steven Sistare @ 2024-10-21 15:56 UTC (permalink / raw)
To: Peter Xu
Cc: Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Paolo Bonzini,
Daniel P. Berrange, Markus Armbruster, qemu-devel
On 10/17/2024 11:53 AM, Peter Xu wrote:
> On Thu, Oct 17, 2024 at 11:19:51AM -0400, Steven Sistare wrote:
>> On 10/17/2024 11:14 AM, Steve Sistare wrote:
>>> Define a new qemu initialization phase called 'precreate' which occurs
>>> before most backends or devices have been created. The only exception
>>> is monitor and qtest devices and their associated chardevs.
>>>
>>> QEMU runs in the main loop during this phase. Monitor connections are
>>> active and can receive migration configuration commands. QEMU starts
>>> listening on the normal migration URI during this phase, which can come
>>> from either the QEMU command line or from a migrate_incoming command.
>>> Thus the user can issue query-migrate to get the socket-address for
>>> dynamically allocated port numbers during precreate.
>>>
>>> In this series QEMU passes through and does not linger in the precreate
>>> phase, and the user sees no change in behavior. The cpr-transfer series
>>> will linger in the phase for an incoming CPR operation, and exit the phase
>>> when the migrate command is send to source QEMU and causes destination QEMU
>>> to read CPR state.
>>
>> Hi Peter, I rebased the cpr-transfer series on precreate. The
>> cpr-transfer migration-test now works. Do you want to see cpr-transfer
>> V3 now, or wait until we get feedback on precreate? The only significant
>> change is that I deleted the HUP synchronization, and I post an async
>> listen for the incoming cpr-uri connection.
>
> Maybe you can still send it, because I remember there're some other
> discussion that may not settled yet (e.g. how anon-memfd is applied, iirc),
> then it can be reviewed and discussed concurrently when proper with the
> precreate series.
For now I'll reply to the open email threads for cpr-transfer V2.
- Steve
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [RFC V1 00/14] precreate phase
2024-10-17 15:14 [RFC V1 00/14] precreate phase Steve Sistare
` (14 preceding siblings ...)
2024-10-17 15:19 ` [RFC V1 00/14] precreate phase Steven Sistare
@ 2024-10-23 16:31 ` Paolo Bonzini
2024-10-24 21:16 ` Steven Sistare
15 siblings, 1 reply; 63+ messages in thread
From: Paolo Bonzini @ 2024-10-23 16:31 UTC (permalink / raw)
To: Steve Sistare, qemu-devel
Cc: Peter Xu, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Daniel P. Berrange,
Markus Armbruster
On 10/17/24 17:14, Steve Sistare wrote:
> Define a new qemu initialization phase called 'precreate' which occurs
> before most backends or devices have been created. The only exception
> is monitor and qtest devices and their associated chardevs.
>
> QEMU runs in the main loop during this phase. Monitor connections are
> active and can receive migration configuration commands. QEMU starts
> listening on the normal migration URI during this phase, which can come
> from either the QEMU command line or from a migrate_incoming command.
> Thus the user can issue query-migrate to get the socket-address for
> dynamically allocated port numbers during precreate.
>
> In this series QEMU passes through and does not linger in the precreate
> phase, and the user sees no change in behavior. The cpr-transfer series
> will linger in the phase for an incoming CPR operation, and exit the phase
> when the migrate command is send to source QEMU and causes destination QEMU
> to read CPR state.
>
> A future series may wish to add a command-line argument and monitor
> command that enters and exits the precreate phase for general purpose use.
> That would enable the user to issue monitor commands that specify backend
> creation parameters.
I have a problem with the concept, which is that the split between
command line and monitor is much harder to understand.
Ideally, one would come entirely before the other; preconfig is already
ugly in how -device is processed later than everything else[1]. This
series makes this much more complex.
If I understand correctly, what you want is effectively to execute
monitor commands from the migration stream. If you want to do that, we
need to take more qemu_init code and turn it into QMP commands, so that
precreate can exit qemu_init very early and the "after precreate"
command line options can be replaced by interactions on the monitor.
This is the idea that drove the creation of -M smp.xxx, -M boot.xxx, -M
memory.xxx, etc. (see for example commit 8c4da4b5218, "machine: add boot
compound property", 2022-05-12). For example -semihosting-config,
-acpitable, -smbios, -fw_cfg, -option-rom, -rtc could all become -M
properties and handled by a single monitor command machine-set.
Of all the other options, -accel, -cpu and -display are the main missing
ones (-cpu is the very hard one); a full list is at
https://wiki.qemu.org/User:Paolo_Bonzini/Machine_init_sequence#QMP_configuration_flow.
Anyhow, at this point all that's needed is a -chardev/-mon pair (and I
guess -incoming defer) in order to bootstrap the monitor in precreate mode.
It's okay to prototype without full support for the options I've listed,
but if we want to go with precreate we should make most command line
options entirely incompatible with it, and also make it imply -nodefaults.
Paolo
[1] -loadvm and -incoming too; but for those two we could make their
monitor commands exit preconfig mode automatically, and invoke them from
the monitor instead of specifying them on the command line.
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [RFC V1 00/14] precreate phase
2024-10-23 16:31 ` Paolo Bonzini
@ 2024-10-24 21:16 ` Steven Sistare
2024-10-25 8:46 ` Daniel P. Berrangé
0 siblings, 1 reply; 63+ messages in thread
From: Steven Sistare @ 2024-10-24 21:16 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
Cc: Peter Xu, Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Daniel P. Berrange,
Markus Armbruster
On 10/23/2024 12:31 PM, Paolo Bonzini wrote:
> On 10/17/24 17:14, Steve Sistare wrote:
>> Define a new qemu initialization phase called 'precreate' which occurs
>> before most backends or devices have been created. The only exception
>> is monitor and qtest devices and their associated chardevs.
>>
>> QEMU runs in the main loop during this phase. Monitor connections are
>> active and can receive migration configuration commands. QEMU starts
>> listening on the normal migration URI during this phase, which can come
>> from either the QEMU command line or from a migrate_incoming command.
>> Thus the user can issue query-migrate to get the socket-address for
>> dynamically allocated port numbers during precreate.
>>
>> In this series QEMU passes through and does not linger in the precreate
>> phase, and the user sees no change in behavior. The cpr-transfer series
>> will linger in the phase for an incoming CPR operation, and exit the phase
>> when the migrate command is send to source QEMU and causes destination QEMU
>> to read CPR state.
>>
>> A future series may wish to add a command-line argument and monitor
>> command that enters and exits the precreate phase for general purpose use.
>> That would enable the user to issue monitor commands that specify backend
>> creation parameters.
>
> I have a problem with the concept, which is that the split between command line and monitor is much harder to understand.
>
> Ideally, one would come entirely before the other; preconfig is already ugly in how -device is processed later than everything else[1]. This series makes this much more complex.
>
> If I understand correctly, what you want is effectively to execute monitor commands from the migration stream. If you want to do that, we need to take more qemu_init code and turn it into QMP commands, so that precreate can exit qemu_init very early and the "after precreate" command line options can be replaced by interactions on the monitor.
>
> This is the idea that drove the creation of -M smp.xxx, -M boot.xxx, -M memory.xxx, etc. (see for example commit 8c4da4b5218, "machine: add boot compound property", 2022-05-12). For example -semihosting-config, -acpitable, -smbios, -fw_cfg, -option-rom, -rtc could all become -M properties and handled by a single monitor command machine-set.
>
> Of all the other options, -accel, -cpu and -display are the main missing ones (-cpu is the very hard one); a full list is at https://wiki.qemu.org/User:Paolo_Bonzini/Machine_init_sequence#QMP_configuration_flow.
>
> Anyhow, at this point all that's needed is a -chardev/-mon pair (and I guess -incoming defer) in order to bootstrap the monitor in precreate mode.
>
> It's okay to prototype without full support for the options I've listed, but if we want to go with precreate we should make most command line options entirely incompatible with it, and also make it imply -nodefaults.
>
> Paolo
>
> [1] -loadvm and -incoming too; but for those two we could make their monitor commands exit preconfig mode automatically, and invoke them from the monitor instead of specifying them on the command line.
Regarding: "what you want is effectively to execute monitor commands
from the migration stream"
That is not the goal of this series. It could be someone else's goal, when
fully developing a precreate phase, and in that context I understand and
agree with your comments. I have a narrower immediate problem to solve,
however.
For CPR, src qemu sends file descriptors to dst qemu using SCM_RIGHTS over
a dedicated channel, then src qemu sends migration state over the normal
migration channel.
Dst qemu reads the fds early, then calls the backend and device creation
functions which use them. Dst qemu then accepts and reads the migration
channel.
We need a way to send monitor commands that set dst migration capabilities,
before src qemu starts the migration. Hence the dst cannot proceed to
backend and device creation because the src has not sent fd's yet. Hence
we need a dst monitor before device creation. The precreate phase does that.
Regarding: "This series makes this much more complex."
I could simplify it if I abandon CPR for chardevs. Then qemu_create_early_backends
and other early dependencies can remain as is. I would drop the notion of
a precreate phase, and instead leverage the preconfig phase. I would move
qemu_create_late_backends, and a small part at the end of qemu_init, to
qmp_x_exit_preconfig.
These patches from the series (slightly renamed) would suffice.
The "move preconfig boundary" patch is new.
* migration: init and listen during preconfig
* vl: move preconfig boundary
* monitor: connect in preconfig
* net: cleanup for preconfig phase
* migration: allow commands during preconfig
Would you consider supporting that?
- Steve
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [RFC V1 00/14] precreate phase
2024-10-24 21:16 ` Steven Sistare
@ 2024-10-25 8:46 ` Daniel P. Berrangé
2024-10-25 13:33 ` Steven Sistare
0 siblings, 1 reply; 63+ messages in thread
From: Daniel P. Berrangé @ 2024-10-25 8:46 UTC (permalink / raw)
To: Steven Sistare
Cc: Paolo Bonzini, qemu-devel, Peter Xu, Fabiano Rosas,
David Hildenbrand, Marcel Apfelbaum, Eduardo Habkost,
Philippe Mathieu-Daude, Markus Armbruster
On Thu, Oct 24, 2024 at 05:16:14PM -0400, Steven Sistare wrote:
>
> Regarding: "what you want is effectively to execute monitor commands
> from the migration stream"
>
> That is not the goal of this series. It could be someone else's goal, when
> fully developing a precreate phase, and in that context I understand and
> agree with your comments. I have a narrower immediate problem to solve,
> however.
>
> For CPR, src qemu sends file descriptors to dst qemu using SCM_RIGHTS over
> a dedicated channel, then src qemu sends migration state over the normal
> migration channel.
>
> Dst qemu reads the fds early, then calls the backend and device creation
> functions which use them. Dst qemu then accepts and reads the migration
> channel.
>
> We need a way to send monitor commands that set dst migration capabilities,
> before src qemu starts the migration. Hence the dst cannot proceed to
> backend and device creation because the src has not sent fd's yet. Hence
> we need a dst monitor before device creation. The precreate phase does that.
Sigh, what we obviously need here, is what we've always talked about as our
long term design goal:
A way to launch QEMU with the CLI only specifying the QMP socket, and every
other config aspect done by issuing QMP commands, which are processed in the
order the mgmt app sends them, so QEMU hasn't have to hardcode processing
of different pieces in different phases.
Anything that isn't that, is piling more hacks on top of our existing
mountain of hacks. That's OK if it does something useful as a side effect
that moves us incrementally closer towards that desired end goal.
> Regarding: "This series makes this much more complex."
>
> I could simplify it if I abandon CPR for chardevs. Then qemu_create_early_backends
> and other early dependencies can remain as is. I would drop the notion of
> a precreate phase, and instead leverage the preconfig phase. I would move
> qemu_create_late_backends, and a small part at the end of qemu_init, to
> qmp_x_exit_preconfig.
Is CPR still going to useful enough in the real world if you drop chardev
support ? Every VM has at least one chardev for a serial device doesn't
it, and often more since we wire chardevs into all kinds of places.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [RFC V1 00/14] precreate phase
2024-10-25 8:46 ` Daniel P. Berrangé
@ 2024-10-25 13:33 ` Steven Sistare
2024-10-25 13:43 ` Daniel P. Berrangé
0 siblings, 1 reply; 63+ messages in thread
From: Steven Sistare @ 2024-10-25 13:33 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Paolo Bonzini, qemu-devel, Peter Xu, Fabiano Rosas,
David Hildenbrand, Marcel Apfelbaum, Eduardo Habkost,
Philippe Mathieu-Daude, Markus Armbruster
On 10/25/2024 4:46 AM, Daniel P. Berrangé wrote:
> On Thu, Oct 24, 2024 at 05:16:14PM -0400, Steven Sistare wrote:
>>
>> Regarding: "what you want is effectively to execute monitor commands
>> from the migration stream"
>>
>> That is not the goal of this series. It could be someone else's goal, when
>> fully developing a precreate phase, and in that context I understand and
>> agree with your comments. I have a narrower immediate problem to solve,
>> however.
>>
>> For CPR, src qemu sends file descriptors to dst qemu using SCM_RIGHTS over
>> a dedicated channel, then src qemu sends migration state over the normal
>> migration channel.
>>
>> Dst qemu reads the fds early, then calls the backend and device creation
>> functions which use them. Dst qemu then accepts and reads the migration
>> channel.
>>
>> We need a way to send monitor commands that set dst migration capabilities,
>> before src qemu starts the migration. Hence the dst cannot proceed to
>> backend and device creation because the src has not sent fd's yet. Hence
>> we need a dst monitor before device creation. The precreate phase does that.
>
> Sigh, what we obviously need here, is what we've always talked about as our
> long term design goal:
>
> A way to launch QEMU with the CLI only specifying the QMP socket, and every
> other config aspect done by issuing QMP commands, which are processed in the
> order the mgmt app sends them, so QEMU hasn't have to hardcode processing
> of different pieces in different phases.
>
> Anything that isn't that, is piling more hacks on top of our existing
> mountain of hacks. That's OK if it does something useful as a side effect
> that moves us incrementally closer towards that desired end goal.
>
>> Regarding: "This series makes this much more complex."
>>
>> I could simplify it if I abandon CPR for chardevs. Then qemu_create_early_backends
>> and other early dependencies can remain as is. I would drop the notion of
>> a precreate phase, and instead leverage the preconfig phase. I would move
>> qemu_create_late_backends, and a small part at the end of qemu_init, to
>> qmp_x_exit_preconfig.
>
> Is CPR still going to useful enough in the real world if you drop chardev
> support ? Every VM has at least one chardev for a serial device doesn't
> it, and often more since we wire chardevs into all kinds of places.
CPR for chardev is not as useful for cpr-transfer mode because the mgmt layer already
knows how to create and manage new connections to dest qemu, as it would for normal
migration.
CPR for chardev is very useful for cpr-exec mode. And cpr-exec mode does not need any
of these monitor patches, because old qemu exec's new qemu, and they are never active
at the same time. One must completely specify the migration using src qemu before
initiating the exec. I mourn cpr-exec mode.
Which begs the question, do we really need to allow migration parameters to be set
in the dest monitor when using cpr? CPR is a very restricted mode of migration.
Let me discuss this with Peter.
- Steve
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [RFC V1 00/14] precreate phase
2024-10-25 13:33 ` Steven Sistare
@ 2024-10-25 13:43 ` Daniel P. Berrangé
2024-10-25 14:32 ` Steven Sistare
` (2 more replies)
0 siblings, 3 replies; 63+ messages in thread
From: Daniel P. Berrangé @ 2024-10-25 13:43 UTC (permalink / raw)
To: Steven Sistare
Cc: Paolo Bonzini, qemu-devel, Peter Xu, Fabiano Rosas,
David Hildenbrand, Marcel Apfelbaum, Eduardo Habkost,
Philippe Mathieu-Daude, Markus Armbruster
On Fri, Oct 25, 2024 at 09:33:51AM -0400, Steven Sistare wrote:
> On 10/25/2024 4:46 AM, Daniel P. Berrangé wrote:
> > On Thu, Oct 24, 2024 at 05:16:14PM -0400, Steven Sistare wrote:
> > >
> > > Regarding: "what you want is effectively to execute monitor commands
> > > from the migration stream"
> > >
> > > That is not the goal of this series. It could be someone else's goal, when
> > > fully developing a precreate phase, and in that context I understand and
> > > agree with your comments. I have a narrower immediate problem to solve,
> > > however.
> > >
> > > For CPR, src qemu sends file descriptors to dst qemu using SCM_RIGHTS over
> > > a dedicated channel, then src qemu sends migration state over the normal
> > > migration channel.
> > >
> > > Dst qemu reads the fds early, then calls the backend and device creation
> > > functions which use them. Dst qemu then accepts and reads the migration
> > > channel.
> > >
> > > We need a way to send monitor commands that set dst migration capabilities,
> > > before src qemu starts the migration. Hence the dst cannot proceed to
> > > backend and device creation because the src has not sent fd's yet. Hence
> > > we need a dst monitor before device creation. The precreate phase does that.
> >
> > Sigh, what we obviously need here, is what we've always talked about as our
> > long term design goal:
> >
> > A way to launch QEMU with the CLI only specifying the QMP socket, and every
> > other config aspect done by issuing QMP commands, which are processed in the
> > order the mgmt app sends them, so QEMU hasn't have to hardcode processing
> > of different pieces in different phases.
> >
> > Anything that isn't that, is piling more hacks on top of our existing
> > mountain of hacks. That's OK if it does something useful as a side effect
> > that moves us incrementally closer towards that desired end goal.
> >
> > > Regarding: "This series makes this much more complex."
> > >
> > > I could simplify it if I abandon CPR for chardevs. Then qemu_create_early_backends
> > > and other early dependencies can remain as is. I would drop the notion of
> > > a precreate phase, and instead leverage the preconfig phase. I would move
> > > qemu_create_late_backends, and a small part at the end of qemu_init, to
> > > qmp_x_exit_preconfig.
> >
> > Is CPR still going to useful enough in the real world if you drop chardev
> > support ? Every VM has at least one chardev for a serial device doesn't
> > it, and often more since we wire chardevs into all kinds of places.
>
> CPR for chardev is not as useful for cpr-transfer mode because the mgmt layer already
> knows how to create and manage new connections to dest qemu, as it would for normal
> migration.
>
> CPR for chardev is very useful for cpr-exec mode. And cpr-exec mode does not need any
> of these monitor patches, because old qemu exec's new qemu, and they are never active
> at the same time. One must completely specify the migration using src qemu before
> initiating the exec. I mourn cpr-exec mode.
>
> Which begs the question, do we really need to allow migration parameters to be set
> in the dest monitor when using cpr? CPR is a very restricted mode of migration.
> Let me discuss this with Peter.
The migration QAPI design has always felt rather odd to me, in that we
have perfectly good commands "migrate" & "migrate-incoming" that are able
to accept an arbitrary list of parameters when invoked. Instead of passing
parameters to them though, we instead require apps use the separate
migreate-set-parameters/capabiltiies commands many times over to set
global variables which the later 'migrate' command then uses.
The reason for this is essentially a historical mistake - we copied the
way we did it from HMP, which was this way because HMP was bad at supporting
arbitrary customizable paramters to commands. I wish we hadn't copied this
design over to QMP.
To bring it back on topic, we need QMP on the dest to set parameters,
because -incoming was limited to only take the URI.
If the "migrate-incoming" command accepted all parameters directly,
then we could use QAPI visitor to usupport a "-incoming ..." command
that took an arbitrary JSON document and turned it into a call to
"migrate-incoming".
With that we would never need QMP on the target for cpr-exec, avoiding
this ordering poblem you're facing....assuming we put processing of
-incoming at the right point in the code flow
Can we fix this design and expose the full configurability on the
CLI using QAPI schema & inline JSON, like we do for other QAPI-ified
CLI args.
It seems entirely practical to me to add parameters to 'migrate-incoming'
in a backwards compatible manner and deprecate set-parameters/capabilities
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [RFC V1 00/14] precreate phase
2024-10-25 13:43 ` Daniel P. Berrangé
@ 2024-10-25 14:32 ` Steven Sistare
2024-10-25 14:49 ` Daniel P. Berrangé
2024-10-28 21:56 ` Peter Xu
2024-10-29 11:13 ` Daniel P. Berrangé
2 siblings, 1 reply; 63+ messages in thread
From: Steven Sistare @ 2024-10-25 14:32 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Paolo Bonzini, qemu-devel, Peter Xu, Fabiano Rosas,
David Hildenbrand, Marcel Apfelbaum, Eduardo Habkost,
Philippe Mathieu-Daude, Markus Armbruster
On 10/25/2024 9:43 AM, Daniel P. Berrangé wrote:
> On Fri, Oct 25, 2024 at 09:33:51AM -0400, Steven Sistare wrote:
>> On 10/25/2024 4:46 AM, Daniel P. Berrangé wrote:
>>> On Thu, Oct 24, 2024 at 05:16:14PM -0400, Steven Sistare wrote:
>>>>
>>>> Regarding: "what you want is effectively to execute monitor commands
>>>> from the migration stream"
>>>>
>>>> That is not the goal of this series. It could be someone else's goal, when
>>>> fully developing a precreate phase, and in that context I understand and
>>>> agree with your comments. I have a narrower immediate problem to solve,
>>>> however.
>>>>
>>>> For CPR, src qemu sends file descriptors to dst qemu using SCM_RIGHTS over
>>>> a dedicated channel, then src qemu sends migration state over the normal
>>>> migration channel.
>>>>
>>>> Dst qemu reads the fds early, then calls the backend and device creation
>>>> functions which use them. Dst qemu then accepts and reads the migration
>>>> channel.
>>>>
>>>> We need a way to send monitor commands that set dst migration capabilities,
>>>> before src qemu starts the migration. Hence the dst cannot proceed to
>>>> backend and device creation because the src has not sent fd's yet. Hence
>>>> we need a dst monitor before device creation. The precreate phase does that.
>>>
>>> Sigh, what we obviously need here, is what we've always talked about as our
>>> long term design goal:
>>>
>>> A way to launch QEMU with the CLI only specifying the QMP socket, and every
>>> other config aspect done by issuing QMP commands, which are processed in the
>>> order the mgmt app sends them, so QEMU hasn't have to hardcode processing
>>> of different pieces in different phases.
>>>
>>> Anything that isn't that, is piling more hacks on top of our existing
>>> mountain of hacks. That's OK if it does something useful as a side effect
>>> that moves us incrementally closer towards that desired end goal.
>>>
>>>> Regarding: "This series makes this much more complex."
>>>>
>>>> I could simplify it if I abandon CPR for chardevs. Then qemu_create_early_backends
>>>> and other early dependencies can remain as is. I would drop the notion of
>>>> a precreate phase, and instead leverage the preconfig phase. I would move
>>>> qemu_create_late_backends, and a small part at the end of qemu_init, to
>>>> qmp_x_exit_preconfig.
>>>
>>> Is CPR still going to useful enough in the real world if you drop chardev
>>> support ? Every VM has at least one chardev for a serial device doesn't
>>> it, and often more since we wire chardevs into all kinds of places.
>>
>> CPR for chardev is not as useful for cpr-transfer mode because the mgmt layer already
>> knows how to create and manage new connections to dest qemu, as it would for normal
>> migration.
>>
>> CPR for chardev is very useful for cpr-exec mode. And cpr-exec mode does not need any
>> of these monitor patches, because old qemu exec's new qemu, and they are never active
>> at the same time. One must completely specify the migration using src qemu before
>> initiating the exec. I mourn cpr-exec mode.
>>
>> Which begs the question, do we really need to allow migration parameters to be set
>> in the dest monitor when using cpr? CPR is a very restricted mode of migration.
>> Let me discuss this with Peter.
>
> The migration QAPI design has always felt rather odd to me, in that we
> have perfectly good commands "migrate" & "migrate-incoming" that are able
> to accept an arbitrary list of parameters when invoked. Instead of passing
> parameters to them though, we instead require apps use the separate
> migreate-set-parameters/capabiltiies commands many times over to set
> global variables which the later 'migrate' command then uses.
>
> The reason for this is essentially a historical mistake - we copied the
> way we did it from HMP, which was this way because HMP was bad at supporting
> arbitrary customizable paramters to commands. I wish we hadn't copied this
> design over to QMP.
>
> To bring it back on topic, we need QMP on the dest to set parameters,
> because -incoming was limited to only take the URI.
>
> If the "migrate-incoming" command accepted all parameters directly,
> then we could use QAPI visitor to usupport a "-incoming ..." command
> that took an arbitrary JSON document and turned it into a call to
> "migrate-incoming".
>
> With that we would never need QMP on the target for cpr-exec, avoiding
> this ordering poblem you're facing....assuming we put processing of
> -incoming at the right point in the code flow
>
> Can we fix this design and expose the full configurability on the
> CLI using QAPI schema & inline JSON, like we do for other QAPI-ified
> CLI args.
>
> It seems entirely practical to me to add parameters to 'migrate-incoming'
> in a backwards compatible manner and deprecate set-parameters/capabilities
Hi Daniel, should we ever need to set caps or parameters for CPR, that sounds
like a good way forward. And a good idea independently of CPR. However, I
am hoping to proceed with CPR with the initial restriction that one cannot
set them. The case that motivated my exploration of precreate is artificial --
qtest wanting to enable migration events -- and I can fix that. I know of no
real cases where caps must be set for CPR.
The other screw case which motivated this thread is a dynamically chosen TCP
port number for the migration listen socket. One must query dest qemu to get it.
Your suggestions here for new incoming syntax would not help. However, for CPR,
we always migrate to the same host, so a unix domain socket can be used.
- Steve
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [RFC V1 00/14] precreate phase
2024-10-25 14:32 ` Steven Sistare
@ 2024-10-25 14:49 ` Daniel P. Berrangé
0 siblings, 0 replies; 63+ messages in thread
From: Daniel P. Berrangé @ 2024-10-25 14:49 UTC (permalink / raw)
To: Steven Sistare
Cc: Paolo Bonzini, qemu-devel, Peter Xu, Fabiano Rosas,
David Hildenbrand, Marcel Apfelbaum, Eduardo Habkost,
Philippe Mathieu-Daude, Markus Armbruster
On Fri, Oct 25, 2024 at 10:32:15AM -0400, Steven Sistare wrote:
> On 10/25/2024 9:43 AM, Daniel P. Berrangé wrote:
> > On Fri, Oct 25, 2024 at 09:33:51AM -0400, Steven Sistare wrote:
> > > On 10/25/2024 4:46 AM, Daniel P. Berrangé wrote:
> > > > On Thu, Oct 24, 2024 at 05:16:14PM -0400, Steven Sistare wrote:
> > > > >
> > > > > Regarding: "what you want is effectively to execute monitor commands
> > > > > from the migration stream"
> > > > >
> > > > > That is not the goal of this series. It could be someone else's goal, when
> > > > > fully developing a precreate phase, and in that context I understand and
> > > > > agree with your comments. I have a narrower immediate problem to solve,
> > > > > however.
> > > > >
> > > > > For CPR, src qemu sends file descriptors to dst qemu using SCM_RIGHTS over
> > > > > a dedicated channel, then src qemu sends migration state over the normal
> > > > > migration channel.
> > > > >
> > > > > Dst qemu reads the fds early, then calls the backend and device creation
> > > > > functions which use them. Dst qemu then accepts and reads the migration
> > > > > channel.
> > > > >
> > > > > We need a way to send monitor commands that set dst migration capabilities,
> > > > > before src qemu starts the migration. Hence the dst cannot proceed to
> > > > > backend and device creation because the src has not sent fd's yet. Hence
> > > > > we need a dst monitor before device creation. The precreate phase does that.
> > > >
> > > > Sigh, what we obviously need here, is what we've always talked about as our
> > > > long term design goal:
> > > >
> > > > A way to launch QEMU with the CLI only specifying the QMP socket, and every
> > > > other config aspect done by issuing QMP commands, which are processed in the
> > > > order the mgmt app sends them, so QEMU hasn't have to hardcode processing
> > > > of different pieces in different phases.
> > > >
> > > > Anything that isn't that, is piling more hacks on top of our existing
> > > > mountain of hacks. That's OK if it does something useful as a side effect
> > > > that moves us incrementally closer towards that desired end goal.
> > > >
> > > > > Regarding: "This series makes this much more complex."
> > > > >
> > > > > I could simplify it if I abandon CPR for chardevs. Then qemu_create_early_backends
> > > > > and other early dependencies can remain as is. I would drop the notion of
> > > > > a precreate phase, and instead leverage the preconfig phase. I would move
> > > > > qemu_create_late_backends, and a small part at the end of qemu_init, to
> > > > > qmp_x_exit_preconfig.
> > > >
> > > > Is CPR still going to useful enough in the real world if you drop chardev
> > > > support ? Every VM has at least one chardev for a serial device doesn't
> > > > it, and often more since we wire chardevs into all kinds of places.
> > >
> > > CPR for chardev is not as useful for cpr-transfer mode because the mgmt layer already
> > > knows how to create and manage new connections to dest qemu, as it would for normal
> > > migration.
> > >
> > > CPR for chardev is very useful for cpr-exec mode. And cpr-exec mode does not need any
> > > of these monitor patches, because old qemu exec's new qemu, and they are never active
> > > at the same time. One must completely specify the migration using src qemu before
> > > initiating the exec. I mourn cpr-exec mode.
> > >
> > > Which begs the question, do we really need to allow migration parameters to be set
> > > in the dest monitor when using cpr? CPR is a very restricted mode of migration.
> > > Let me discuss this with Peter.
> >
> > The migration QAPI design has always felt rather odd to me, in that we
> > have perfectly good commands "migrate" & "migrate-incoming" that are able
> > to accept an arbitrary list of parameters when invoked. Instead of passing
> > parameters to them though, we instead require apps use the separate
> > migreate-set-parameters/capabiltiies commands many times over to set
> > global variables which the later 'migrate' command then uses.
> >
> > The reason for this is essentially a historical mistake - we copied the
> > way we did it from HMP, which was this way because HMP was bad at supporting
> > arbitrary customizable paramters to commands. I wish we hadn't copied this
> > design over to QMP.
> >
> > To bring it back on topic, we need QMP on the dest to set parameters,
> > because -incoming was limited to only take the URI.
> >
> > If the "migrate-incoming" command accepted all parameters directly,
> > then we could use QAPI visitor to usupport a "-incoming ..." command
> > that took an arbitrary JSON document and turned it into a call to
> > "migrate-incoming".
> >
> > With that we would never need QMP on the target for cpr-exec, avoiding
> > this ordering poblem you're facing....assuming we put processing of
> > -incoming at the right point in the code flow
> >
> > Can we fix this design and expose the full configurability on the
> > CLI using QAPI schema & inline JSON, like we do for other QAPI-ified
> > CLI args.
> >
> > It seems entirely practical to me to add parameters to 'migrate-incoming'
> > in a backwards compatible manner and deprecate set-parameters/capabilities
>
> Hi Daniel, should we ever need to set caps or parameters for CPR, that sounds
> like a good way forward. And a good idea independently of CPR. However, I
> am hoping to proceed with CPR with the initial restriction that one cannot
> set them. The case that motivated my exploration of precreate is artificial --
> qtest wanting to enable migration events -- and I can fix that. I know of no
> real cases where caps must be set for CPR.
>
> The other screw case which motivated this thread is a dynamically chosen TCP
> port number for the migration listen socket. One must query dest qemu to get it.
> Your suggestions here for new incoming syntax would not help. However, for CPR,
> we always migrate to the same host, so a unix domain socket can be used.
FWIW, at least in libvirt usage, IIRC, libvirt will choose the listener
port number upfront itself
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [RFC V1 00/14] precreate phase
2024-10-25 13:43 ` Daniel P. Berrangé
2024-10-25 14:32 ` Steven Sistare
@ 2024-10-28 21:56 ` Peter Xu
2024-10-29 9:09 ` Daniel P. Berrangé
2024-10-29 11:13 ` Daniel P. Berrangé
2 siblings, 1 reply; 63+ messages in thread
From: Peter Xu @ 2024-10-28 21:56 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Steven Sistare, Paolo Bonzini, qemu-devel, Fabiano Rosas,
David Hildenbrand, Marcel Apfelbaum, Eduardo Habkost,
Philippe Mathieu-Daude, Markus Armbruster
On Fri, Oct 25, 2024 at 02:43:06PM +0100, Daniel P. Berrangé wrote:
> On Fri, Oct 25, 2024 at 09:33:51AM -0400, Steven Sistare wrote:
> > On 10/25/2024 4:46 AM, Daniel P. Berrangé wrote:
> > > On Thu, Oct 24, 2024 at 05:16:14PM -0400, Steven Sistare wrote:
> > > >
> > > > Regarding: "what you want is effectively to execute monitor commands
> > > > from the migration stream"
> > > >
> > > > That is not the goal of this series. It could be someone else's goal, when
> > > > fully developing a precreate phase, and in that context I understand and
> > > > agree with your comments. I have a narrower immediate problem to solve,
> > > > however.
> > > >
> > > > For CPR, src qemu sends file descriptors to dst qemu using SCM_RIGHTS over
> > > > a dedicated channel, then src qemu sends migration state over the normal
> > > > migration channel.
> > > >
> > > > Dst qemu reads the fds early, then calls the backend and device creation
> > > > functions which use them. Dst qemu then accepts and reads the migration
> > > > channel.
> > > >
> > > > We need a way to send monitor commands that set dst migration capabilities,
> > > > before src qemu starts the migration. Hence the dst cannot proceed to
> > > > backend and device creation because the src has not sent fd's yet. Hence
> > > > we need a dst monitor before device creation. The precreate phase does that.
> > >
> > > Sigh, what we obviously need here, is what we've always talked about as our
> > > long term design goal:
> > >
> > > A way to launch QEMU with the CLI only specifying the QMP socket, and every
> > > other config aspect done by issuing QMP commands, which are processed in the
> > > order the mgmt app sends them, so QEMU hasn't have to hardcode processing
> > > of different pieces in different phases.
> > >
> > > Anything that isn't that, is piling more hacks on top of our existing
> > > mountain of hacks. That's OK if it does something useful as a side effect
> > > that moves us incrementally closer towards that desired end goal.
> > >
> > > > Regarding: "This series makes this much more complex."
> > > >
> > > > I could simplify it if I abandon CPR for chardevs. Then qemu_create_early_backends
> > > > and other early dependencies can remain as is. I would drop the notion of
> > > > a precreate phase, and instead leverage the preconfig phase. I would move
> > > > qemu_create_late_backends, and a small part at the end of qemu_init, to
> > > > qmp_x_exit_preconfig.
> > >
> > > Is CPR still going to useful enough in the real world if you drop chardev
> > > support ? Every VM has at least one chardev for a serial device doesn't
> > > it, and often more since we wire chardevs into all kinds of places.
> >
> > CPR for chardev is not as useful for cpr-transfer mode because the mgmt layer already
> > knows how to create and manage new connections to dest qemu, as it would for normal
> > migration.
> >
> > CPR for chardev is very useful for cpr-exec mode. And cpr-exec mode does not need any
> > of these monitor patches, because old qemu exec's new qemu, and they are never active
> > at the same time. One must completely specify the migration using src qemu before
> > initiating the exec. I mourn cpr-exec mode.
> >
> > Which begs the question, do we really need to allow migration parameters to be set
> > in the dest monitor when using cpr? CPR is a very restricted mode of migration.
> > Let me discuss this with Peter.
>
> The migration QAPI design has always felt rather odd to me, in that we
> have perfectly good commands "migrate" & "migrate-incoming" that are able
> to accept an arbitrary list of parameters when invoked. Instead of passing
> parameters to them though, we instead require apps use the separate
> migreate-set-parameters/capabiltiies commands many times over to set
> global variables which the later 'migrate' command then uses.
Just to mention, we will still need some special parameters that can change
during migration, like max-bandwidth, max-downtime etc. So not all of them
can be made into "migrate"/"migrate-incoming" arguments.
>
> The reason for this is essentially a historical mistake - we copied the
> way we did it from HMP, which was this way because HMP was bad at supporting
> arbitrary customizable paramters to commands. I wish we hadn't copied this
> design over to QMP.
>
> To bring it back on topic, we need QMP on the dest to set parameters,
> because -incoming was limited to only take the URI.
>
> If the "migrate-incoming" command accepted all parameters directly,
> then we could use QAPI visitor to usupport a "-incoming ..." command
> that took an arbitrary JSON document and turned it into a call to
> "migrate-incoming".
>
> With that we would never need QMP on the target for cpr-exec, avoiding
> this ordering poblem you're facing....assuming we put processing of
> -incoming at the right point in the code flow
>
> Can we fix this design and expose the full configurability on the
> CLI using QAPI schema & inline JSON, like we do for other QAPI-ified
> CLI args.
>
> It seems entirely practical to me to add parameters to 'migrate-incoming'
> in a backwards compatible manner and deprecate set-parameters/capabilities
Fabiano started working on handshake recently. After that is ready, we
should logically also achieve similar goal at least for dest qemu so no
cap/parameter is ever needed there.
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [RFC V1 00/14] precreate phase
2024-10-28 21:56 ` Peter Xu
@ 2024-10-29 9:09 ` Daniel P. Berrangé
0 siblings, 0 replies; 63+ messages in thread
From: Daniel P. Berrangé @ 2024-10-29 9:09 UTC (permalink / raw)
To: Peter Xu
Cc: Steven Sistare, Paolo Bonzini, qemu-devel, Fabiano Rosas,
David Hildenbrand, Marcel Apfelbaum, Eduardo Habkost,
Philippe Mathieu-Daude, Markus Armbruster
On Mon, Oct 28, 2024 at 05:56:10PM -0400, Peter Xu wrote:
> On Fri, Oct 25, 2024 at 02:43:06PM +0100, Daniel P. Berrangé wrote:
> > On Fri, Oct 25, 2024 at 09:33:51AM -0400, Steven Sistare wrote:
> > > On 10/25/2024 4:46 AM, Daniel P. Berrangé wrote:
> > > > On Thu, Oct 24, 2024 at 05:16:14PM -0400, Steven Sistare wrote:
> > > > >
> > > > > Regarding: "what you want is effectively to execute monitor commands
> > > > > from the migration stream"
> > > > >
> > > > > That is not the goal of this series. It could be someone else's goal, when
> > > > > fully developing a precreate phase, and in that context I understand and
> > > > > agree with your comments. I have a narrower immediate problem to solve,
> > > > > however.
> > > > >
> > > > > For CPR, src qemu sends file descriptors to dst qemu using SCM_RIGHTS over
> > > > > a dedicated channel, then src qemu sends migration state over the normal
> > > > > migration channel.
> > > > >
> > > > > Dst qemu reads the fds early, then calls the backend and device creation
> > > > > functions which use them. Dst qemu then accepts and reads the migration
> > > > > channel.
> > > > >
> > > > > We need a way to send monitor commands that set dst migration capabilities,
> > > > > before src qemu starts the migration. Hence the dst cannot proceed to
> > > > > backend and device creation because the src has not sent fd's yet. Hence
> > > > > we need a dst monitor before device creation. The precreate phase does that.
> > > >
> > > > Sigh, what we obviously need here, is what we've always talked about as our
> > > > long term design goal:
> > > >
> > > > A way to launch QEMU with the CLI only specifying the QMP socket, and every
> > > > other config aspect done by issuing QMP commands, which are processed in the
> > > > order the mgmt app sends them, so QEMU hasn't have to hardcode processing
> > > > of different pieces in different phases.
> > > >
> > > > Anything that isn't that, is piling more hacks on top of our existing
> > > > mountain of hacks. That's OK if it does something useful as a side effect
> > > > that moves us incrementally closer towards that desired end goal.
> > > >
> > > > > Regarding: "This series makes this much more complex."
> > > > >
> > > > > I could simplify it if I abandon CPR for chardevs. Then qemu_create_early_backends
> > > > > and other early dependencies can remain as is. I would drop the notion of
> > > > > a precreate phase, and instead leverage the preconfig phase. I would move
> > > > > qemu_create_late_backends, and a small part at the end of qemu_init, to
> > > > > qmp_x_exit_preconfig.
> > > >
> > > > Is CPR still going to useful enough in the real world if you drop chardev
> > > > support ? Every VM has at least one chardev for a serial device doesn't
> > > > it, and often more since we wire chardevs into all kinds of places.
> > >
> > > CPR for chardev is not as useful for cpr-transfer mode because the mgmt layer already
> > > knows how to create and manage new connections to dest qemu, as it would for normal
> > > migration.
> > >
> > > CPR for chardev is very useful for cpr-exec mode. And cpr-exec mode does not need any
> > > of these monitor patches, because old qemu exec's new qemu, and they are never active
> > > at the same time. One must completely specify the migration using src qemu before
> > > initiating the exec. I mourn cpr-exec mode.
> > >
> > > Which begs the question, do we really need to allow migration parameters to be set
> > > in the dest monitor when using cpr? CPR is a very restricted mode of migration.
> > > Let me discuss this with Peter.
> >
> > The migration QAPI design has always felt rather odd to me, in that we
> > have perfectly good commands "migrate" & "migrate-incoming" that are able
> > to accept an arbitrary list of parameters when invoked. Instead of passing
> > parameters to them though, we instead require apps use the separate
> > migreate-set-parameters/capabiltiies commands many times over to set
> > global variables which the later 'migrate' command then uses.
>
> Just to mention, we will still need some special parameters that can change
> during migration, like max-bandwidth, max-downtime etc. So not all of them
> can be made into "migrate"/"migrate-incoming" arguments.
I guess we can leave migrate-set-parameters with the sub-set of
parameters needed at runtime, or have a 'migrate-update' command
for those, to make it clear which are valid to set at runtime,
and which are not valid at initial start.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [RFC V1 00/14] precreate phase
2024-10-25 13:43 ` Daniel P. Berrangé
2024-10-25 14:32 ` Steven Sistare
2024-10-28 21:56 ` Peter Xu
@ 2024-10-29 11:13 ` Daniel P. Berrangé
2024-10-29 13:20 ` Fabiano Rosas
2 siblings, 1 reply; 63+ messages in thread
From: Daniel P. Berrangé @ 2024-10-29 11:13 UTC (permalink / raw)
To: Steven Sistare, Paolo Bonzini, qemu-devel, Peter Xu,
Fabiano Rosas, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Markus Armbruster
On Fri, Oct 25, 2024 at 02:43:06PM +0100, Daniel P. Berrangé wrote:
>
> The migration QAPI design has always felt rather odd to me, in that we
> have perfectly good commands "migrate" & "migrate-incoming" that are able
> to accept an arbitrary list of parameters when invoked. Instead of passing
> parameters to them though, we instead require apps use the separate
> migreate-set-parameters/capabiltiies commands many times over to set
> global variables which the later 'migrate' command then uses.
>
> The reason for this is essentially a historical mistake - we copied the
> way we did it from HMP, which was this way because HMP was bad at supporting
> arbitrary customizable paramters to commands. I wish we hadn't copied this
> design over to QMP.
>
> To bring it back on topic, we need QMP on the dest to set parameters,
> because -incoming was limited to only take the URI.
>
> If the "migrate-incoming" command accepted all parameters directly,
> then we could use QAPI visitor to usupport a "-incoming ..." command
> that took an arbitrary JSON document and turned it into a call to
> "migrate-incoming".
>
> With that we would never need QMP on the target for cpr-exec, avoiding
> this ordering poblem you're facing....assuming we put processing of
> -incoming at the right point in the code flow
>
> Can we fix this design and expose the full configurability on the
> CLI using QAPI schema & inline JSON, like we do for other QAPI-ified
> CLI args.
>
> It seems entirely practical to me to add parameters to 'migrate-incoming'
> in a backwards compatible manner and deprecate set-parameters/capabilities
Incidentally, if we were going to evolve the migration API at all, then
it probably ought to start making use of the async job infrastructure
we have available. This is use by block jobs, and by the internal snapshot
commands, and was intended to be used for any case where we had a long
running operation triggered by a command. Migration was a poster-child
example of what its intended for, but was left alone when we first
introduced the job APIs.
The 'job-cancel' API would obsolete 'migrate-cancel'.
The other interestnig thing is that the job framework creates a well
defined lifecycle for a job, that allows querying information about
the job after completeion, but without QEMU having to keep that info
around forever. ie once a job has finished, an app can query info
about completion, and when it no longer needs that info, it can
call 'job-dismiss' to tell QEMU to discard it.
If "MigrationState" were associated a job, then it would thus have a
clear 'creation' and 'deletion' time.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [RFC V1 00/14] precreate phase
2024-10-29 11:13 ` Daniel P. Berrangé
@ 2024-10-29 13:20 ` Fabiano Rosas
2024-10-29 15:18 ` Peter Xu
2024-10-29 15:58 ` Daniel P. Berrangé
0 siblings, 2 replies; 63+ messages in thread
From: Fabiano Rosas @ 2024-10-29 13:20 UTC (permalink / raw)
To: Daniel P. Berrangé, Steven Sistare, Paolo Bonzini,
qemu-devel, Peter Xu, David Hildenbrand, Marcel Apfelbaum,
Eduardo Habkost, Philippe Mathieu-Daude, Markus Armbruster
Daniel P. Berrangé <berrange@redhat.com> writes:
> On Fri, Oct 25, 2024 at 02:43:06PM +0100, Daniel P. Berrangé wrote:
>>
>> The migration QAPI design has always felt rather odd to me, in that we
>> have perfectly good commands "migrate" & "migrate-incoming" that are able
>> to accept an arbitrary list of parameters when invoked. Instead of passing
>> parameters to them though, we instead require apps use the separate
>> migreate-set-parameters/capabiltiies commands many times over to set
>> global variables which the later 'migrate' command then uses.
>>
>> The reason for this is essentially a historical mistake - we copied the
>> way we did it from HMP, which was this way because HMP was bad at supporting
>> arbitrary customizable paramters to commands. I wish we hadn't copied this
>> design over to QMP.
>>
>> To bring it back on topic, we need QMP on the dest to set parameters,
>> because -incoming was limited to only take the URI.
>>
>> If the "migrate-incoming" command accepted all parameters directly,
>> then we could use QAPI visitor to usupport a "-incoming ..." command
>> that took an arbitrary JSON document and turned it into a call to
>> "migrate-incoming".
>>
>> With that we would never need QMP on the target for cpr-exec, avoiding
>> this ordering poblem you're facing....assuming we put processing of
>> -incoming at the right point in the code flow
>>
>> Can we fix this design and expose the full configurability on the
>> CLI using QAPI schema & inline JSON, like we do for other QAPI-ified
>> CLI args.
>>
>> It seems entirely practical to me to add parameters to 'migrate-incoming'
>> in a backwards compatible manner and deprecate set-parameters/capabilities
>
> Incidentally, if we were going to evolve the migration API at all, then
> it probably ought to start making use of the async job infrastructure
> we have available. This is use by block jobs, and by the internal snapshot
I'm all for standardization on core infrastructure, but unfortunately
putting migration in a coroutine would open a can of worms. In fact,
we've been discussing about moving the incoming side out of coroutines
for a while.
> commands, and was intended to be used for any case where we had a long
> running operation triggered by a command. Migration was a poster-child
> example of what its intended for, but was left alone when we first
> introduced the job APIs.
>
> The 'job-cancel' API would obsolete 'migrate-cancel'.
>
> The other interestnig thing is that the job framework creates a well
> defined lifecycle for a job, that allows querying information about
> the job after completeion, but without QEMU having to keep that info
> around forever. ie once a job has finished, an app can query info
> about completion, and when it no longer needs that info, it can
> call 'job-dismiss' to tell QEMU to discard it.
>
> If "MigrationState" were associated a job, then it would thus have a
> clear 'creation' and 'deletion' time.
>
> With regards,
> Daniel
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [RFC V1 00/14] precreate phase
2024-10-29 13:20 ` Fabiano Rosas
@ 2024-10-29 15:18 ` Peter Xu
2024-10-29 15:58 ` Daniel P. Berrangé
1 sibling, 0 replies; 63+ messages in thread
From: Peter Xu @ 2024-10-29 15:18 UTC (permalink / raw)
To: Fabiano Rosas
Cc: Daniel P. Berrangé, Steven Sistare, Paolo Bonzini,
qemu-devel, David Hildenbrand, Marcel Apfelbaum, Eduardo Habkost,
Philippe Mathieu-Daude, Markus Armbruster
On Tue, Oct 29, 2024 at 10:20:24AM -0300, Fabiano Rosas wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
> > On Fri, Oct 25, 2024 at 02:43:06PM +0100, Daniel P. Berrangé wrote:
> >>
> >> The migration QAPI design has always felt rather odd to me, in that we
> >> have perfectly good commands "migrate" & "migrate-incoming" that are able
> >> to accept an arbitrary list of parameters when invoked. Instead of passing
> >> parameters to them though, we instead require apps use the separate
> >> migreate-set-parameters/capabiltiies commands many times over to set
> >> global variables which the later 'migrate' command then uses.
> >>
> >> The reason for this is essentially a historical mistake - we copied the
> >> way we did it from HMP, which was this way because HMP was bad at supporting
> >> arbitrary customizable paramters to commands. I wish we hadn't copied this
> >> design over to QMP.
> >>
> >> To bring it back on topic, we need QMP on the dest to set parameters,
> >> because -incoming was limited to only take the URI.
> >>
> >> If the "migrate-incoming" command accepted all parameters directly,
> >> then we could use QAPI visitor to usupport a "-incoming ..." command
> >> that took an arbitrary JSON document and turned it into a call to
> >> "migrate-incoming".
> >>
> >> With that we would never need QMP on the target for cpr-exec, avoiding
> >> this ordering poblem you're facing....assuming we put processing of
> >> -incoming at the right point in the code flow
> >>
> >> Can we fix this design and expose the full configurability on the
> >> CLI using QAPI schema & inline JSON, like we do for other QAPI-ified
> >> CLI args.
> >>
> >> It seems entirely practical to me to add parameters to 'migrate-incoming'
> >> in a backwards compatible manner and deprecate set-parameters/capabilities
> >
> > Incidentally, if we were going to evolve the migration API at all, then
> > it probably ought to start making use of the async job infrastructure
> > we have available. This is use by block jobs, and by the internal snapshot
>
> I'm all for standardization on core infrastructure, but unfortunately
> putting migration in a coroutine would open a can of worms. In fact,
> we've been discussing about moving the incoming side out of coroutines
> for a while.
Yes, I share the same concern. I think migration decided to go already
with as much thread model as possible that it can.
And I paused that attempt to move load() into a thread, as of now, finding
it's still non-trivial to work out the major issue: after dest load became
a thread, it means it can't take BQL for too long otherwise it blocks the
monitor.
It means we can't take bql for _any_ IO operation because it can stuck at
any IO waiting for the iochannel / NIC, aka, any qemu_get*() API invoked.
Meanwhile we still trivially need the bql from time to time, either in
pre_load() / post_load(), or some of VMStateInfo->get(). But still I can't
blindly take them, as in any VMStateInfo->get(), it can invoke qemu_get*()
itself.
So I am not sure whether what we can get from that is worthwhile yet on the
effort to make it work..
>
> > commands, and was intended to be used for any case where we had a long
> > running operation triggered by a command. Migration was a poster-child
> > example of what its intended for, but was left alone when we first
> > introduced the job APIs.
> >
> > The 'job-cancel' API would obsolete 'migrate-cancel'.
> >
> > The other interestnig thing is that the job framework creates a well
> > defined lifecycle for a job, that allows querying information about
> > the job after completeion, but without QEMU having to keep that info
> > around forever. ie once a job has finished, an app can query info
> > about completion, and when it no longer needs that info, it can
> > call 'job-dismiss' to tell QEMU to discard it.
> >
> > If "MigrationState" were associated a job, then it would thus have a
> > clear 'creation' and 'deletion' time.
It'll face the same challenge here on whether we can join() in the main
thread. IOW, job-cancel can take time which can also potentially block
qemu from quitting fast.
--
Peter Xu
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [RFC V1 00/14] precreate phase
2024-10-29 13:20 ` Fabiano Rosas
2024-10-29 15:18 ` Peter Xu
@ 2024-10-29 15:58 ` Daniel P. Berrangé
1 sibling, 0 replies; 63+ messages in thread
From: Daniel P. Berrangé @ 2024-10-29 15:58 UTC (permalink / raw)
To: Fabiano Rosas
Cc: Steven Sistare, Paolo Bonzini, qemu-devel, Peter Xu,
David Hildenbrand, Marcel Apfelbaum, Eduardo Habkost,
Philippe Mathieu-Daude, Markus Armbruster
On Tue, Oct 29, 2024 at 10:20:24AM -0300, Fabiano Rosas wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
> > On Fri, Oct 25, 2024 at 02:43:06PM +0100, Daniel P. Berrangé wrote:
> >>
> >> The migration QAPI design has always felt rather odd to me, in that we
> >> have perfectly good commands "migrate" & "migrate-incoming" that are able
> >> to accept an arbitrary list of parameters when invoked. Instead of passing
> >> parameters to them though, we instead require apps use the separate
> >> migreate-set-parameters/capabiltiies commands many times over to set
> >> global variables which the later 'migrate' command then uses.
> >>
> >> The reason for this is essentially a historical mistake - we copied the
> >> way we did it from HMP, which was this way because HMP was bad at supporting
> >> arbitrary customizable paramters to commands. I wish we hadn't copied this
> >> design over to QMP.
> >>
> >> To bring it back on topic, we need QMP on the dest to set parameters,
> >> because -incoming was limited to only take the URI.
> >>
> >> If the "migrate-incoming" command accepted all parameters directly,
> >> then we could use QAPI visitor to usupport a "-incoming ..." command
> >> that took an arbitrary JSON document and turned it into a call to
> >> "migrate-incoming".
> >>
> >> With that we would never need QMP on the target for cpr-exec, avoiding
> >> this ordering poblem you're facing....assuming we put processing of
> >> -incoming at the right point in the code flow
> >>
> >> Can we fix this design and expose the full configurability on the
> >> CLI using QAPI schema & inline JSON, like we do for other QAPI-ified
> >> CLI args.
> >>
> >> It seems entirely practical to me to add parameters to 'migrate-incoming'
> >> in a backwards compatible manner and deprecate set-parameters/capabilities
> >
> > Incidentally, if we were going to evolve the migration API at all, then
> > it probably ought to start making use of the async job infrastructure
> > we have available. This is use by block jobs, and by the internal snapshot
>
> I'm all for standardization on core infrastructure, but unfortunately
> putting migration in a coroutine would open a can of worms. In fact,
> we've been discussing about moving the incoming side out of coroutines
> for a while.
Yeah, I can understand that.
The job API at the QMP level has no association with coroutines. It
simply expresses a way to handle long running background jobs in a
standard manner.
The dependency on coroutines is purely the internal job APIs. I wonder
if it is at all practical to alter the job APIs to allow migration to
use them as it exists today, as the migration code already is quite
capable to running in the background, without adding more coroutine
usage.
It would be quite annoying if our general purpose QMP API cannot be
used because internal only impl limitations :-(
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 63+ messages in thread