Similarly to accel properties, move compat properties out of globals
registration, and apply the machine compat properties during
device_post_init().
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
include/hw/boards.h | 3 +-
include/hw/qdev-core.h | 1 +
hw/arm/virt.c | 16 +++++------
hw/core/machine.c | 19 +------------
hw/core/qdev.c | 9 ++++++
hw/i386/pc_piix.c | 56 +++++++++++++++++++-------------------
hw/i386/pc_q35.c | 20 +++++++-------
hw/ppc/spapr.c | 26 +++++++++---------
hw/s390x/s390-virtio-ccw.c | 20 +++++++-------
vl.c | 2 +-
10 files changed, 82 insertions(+), 90 deletions(-)
diff --git a/include/hw/boards.h b/include/hw/boards.h
index f82f28468b..28c2b0af41 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -69,7 +69,6 @@ int machine_kvm_shadow_mem(MachineState *machine);
int machine_phandle_start(MachineState *machine);
bool machine_dump_guest_core(MachineState *machine);
bool machine_mem_merge(MachineState *machine);
-void machine_register_compat_props(MachineState *machine);
HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine);
void machine_set_cpu_numa_node(MachineState *machine,
const CpuInstanceProperties *props,
@@ -191,7 +190,7 @@ struct MachineClass {
const char *default_machine_opts;
const char *default_boot_order;
const char *default_display;
- GArray *compat_props;
+ GPtrArray *compat_props;
const char *hw_version;
ram_addr_t default_ram_size;
const char *default_cpu_type;
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 82afd3c50d..f6a9503e23 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -279,6 +279,7 @@ typedef struct GlobalProperty {
} while (0)
void accel_register_compat_props(const GPtrArray *props);
+void machine_register_compat_props(const GPtrArray *props);
/*** Board API. This should go away once we have a machine config file. ***/
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index a2b8d8f7c2..d106a8a8c7 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1882,7 +1882,7 @@ static void virt_3_0_instance_init(Object *obj)
static void virt_machine_3_0_options(MachineClass *mc)
{
virt_machine_3_1_options(mc);
- SET_MACHINE_COMPAT(mc, VIRT_COMPAT_3_0);
+ SET_COMPAT(mc, VIRT_COMPAT_3_0);
}
DEFINE_VIRT_MACHINE(3, 0)
@@ -1899,7 +1899,7 @@ static void virt_machine_2_12_options(MachineClass *mc)
VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
virt_machine_3_0_options(mc);
- SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_12);
+ SET_COMPAT(mc, VIRT_COMPAT_2_12);
vmc->no_highmem_ecam = true;
mc->max_cpus = 255;
}
@@ -1918,7 +1918,7 @@ static void virt_machine_2_11_options(MachineClass *mc)
VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
virt_machine_2_12_options(mc);
- SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_11);
+ SET_COMPAT(mc, VIRT_COMPAT_2_11);
vmc->smbios_old_sys_ver = true;
}
DEFINE_VIRT_MACHINE(2, 11)
@@ -1934,7 +1934,7 @@ static void virt_2_10_instance_init(Object *obj)
static void virt_machine_2_10_options(MachineClass *mc)
{
virt_machine_2_11_options(mc);
- SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_10);
+ SET_COMPAT(mc, VIRT_COMPAT_2_10);
/* before 2.11 we never faulted accesses to bad addresses */
mc->ignore_memory_transaction_failures = true;
}
@@ -1951,7 +1951,7 @@ static void virt_2_9_instance_init(Object *obj)
static void virt_machine_2_9_options(MachineClass *mc)
{
virt_machine_2_10_options(mc);
- SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_9);
+ SET_COMPAT(mc, VIRT_COMPAT_2_9);
}
DEFINE_VIRT_MACHINE(2, 9)
@@ -1968,7 +1968,7 @@ static void virt_machine_2_8_options(MachineClass *mc)
VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
virt_machine_2_9_options(mc);
- SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_8);
+ SET_COMPAT(mc, VIRT_COMPAT_2_8);
/* For 2.8 and earlier we falsely claimed in the DT that
* our timers were edge-triggered, not level-triggered.
*/
@@ -1989,7 +1989,7 @@ static void virt_machine_2_7_options(MachineClass *mc)
VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
virt_machine_2_8_options(mc);
- SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_7);
+ SET_COMPAT(mc, VIRT_COMPAT_2_7);
/* ITS was introduced with 2.8 */
vmc->no_its = true;
/* Stick with 1K pages for migration compatibility */
@@ -2010,7 +2010,7 @@ static void virt_machine_2_6_options(MachineClass *mc)
VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
virt_machine_2_7_options(mc);
- SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_6);
+ SET_COMPAT(mc, VIRT_COMPAT_2_6);
vmc->disallow_affinity_adjustment = true;
/* Disable PMU for 2.6 as PMU support was first introduced in 2.7 */
vmc->no_pmu = true;
diff --git a/hw/core/machine.c b/hw/core/machine.c
index c51423b647..6e24924aba 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -647,6 +647,7 @@ static void machine_class_base_init(ObjectClass *oc, void *data)
assert(g_str_has_suffix(cname, TYPE_MACHINE_SUFFIX));
mc->name = g_strndup(cname,
strlen(cname) - strlen(TYPE_MACHINE_SUFFIX));
+ mc->compat_props = g_ptr_array_new();
}
}
@@ -834,24 +835,6 @@ void machine_run_board_init(MachineState *machine)
machine_class->init(machine);
}
-void machine_register_compat_props(MachineState *machine)
-{
- MachineClass *mc = MACHINE_GET_CLASS(machine);
- int i;
- GlobalProperty *p;
-
- if (!mc->compat_props) {
- return;
- }
-
- for (i = 0; i < mc->compat_props->len; i++) {
- p = g_array_index(mc->compat_props, GlobalProperty *, i);
- /* Machine compat_props must never cause errors: */
- p->errp = &error_abort;
- qdev_prop_register_global(p);
- }
-}
-
static const TypeInfo machine_info = {
.name = TYPE_MACHINE,
.parent = TYPE_OBJECT,
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 7066d28271..3b31b2c025 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -971,17 +971,26 @@ static void device_initfn(Object *obj)
}
static const GPtrArray *ac_compat_props;
+static const GPtrArray *mc_compat_props;
void accel_register_compat_props(const GPtrArray *props)
{
ac_compat_props = props;
}
+void machine_register_compat_props(const GPtrArray *props)
+{
+ mc_compat_props = props;
+}
+
static void device_post_init(Object *obj)
{
if (ac_compat_props) {
object_apply_global_props(obj, ac_compat_props, &error_abort);
}
+ if (mc_compat_props) {
+ object_apply_global_props(obj, mc_compat_props, &error_abort);
+ }
qdev_prop_set_globals(DEVICE(obj));
}
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 7092d6d13f..3cc92b8eec 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -443,7 +443,7 @@ static void pc_i440fx_3_0_machine_options(MachineClass *m)
pc_i440fx_3_1_machine_options(m);
m->is_default = 0;
m->alias = NULL;
- SET_MACHINE_COMPAT(m, PC_COMPAT_3_0);
+ SET_COMPAT(m, PC_COMPAT_3_0);
}
DEFINE_I440FX_MACHINE(v3_0, "pc-i440fx-3.0", NULL,
@@ -452,7 +452,7 @@ DEFINE_I440FX_MACHINE(v3_0, "pc-i440fx-3.0", NULL,
static void pc_i440fx_2_12_machine_options(MachineClass *m)
{
pc_i440fx_3_0_machine_options(m);
- SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
+ SET_COMPAT(m, PC_COMPAT_2_12);
}
DEFINE_I440FX_MACHINE(v2_12, "pc-i440fx-2.12", NULL,
@@ -461,7 +461,7 @@ DEFINE_I440FX_MACHINE(v2_12, "pc-i440fx-2.12", NULL,
static void pc_i440fx_2_11_machine_options(MachineClass *m)
{
pc_i440fx_2_12_machine_options(m);
- SET_MACHINE_COMPAT(m, PC_COMPAT_2_11);
+ SET_COMPAT(m, PC_COMPAT_2_11);
}
DEFINE_I440FX_MACHINE(v2_11, "pc-i440fx-2.11", NULL,
@@ -470,7 +470,7 @@ DEFINE_I440FX_MACHINE(v2_11, "pc-i440fx-2.11", NULL,
static void pc_i440fx_2_10_machine_options(MachineClass *m)
{
pc_i440fx_2_11_machine_options(m);
- SET_MACHINE_COMPAT(m, PC_COMPAT_2_10);
+ SET_COMPAT(m, PC_COMPAT_2_10);
m->auto_enable_numa_with_memhp = false;
}
@@ -480,7 +480,7 @@ DEFINE_I440FX_MACHINE(v2_10, "pc-i440fx-2.10", NULL,
static void pc_i440fx_2_9_machine_options(MachineClass *m)
{
pc_i440fx_2_10_machine_options(m);
- SET_MACHINE_COMPAT(m, PC_COMPAT_2_9);
+ SET_COMPAT(m, PC_COMPAT_2_9);
m->numa_auto_assign_ram = numa_legacy_auto_assign_ram;
}
@@ -490,7 +490,7 @@ DEFINE_I440FX_MACHINE(v2_9, "pc-i440fx-2.9", NULL,
static void pc_i440fx_2_8_machine_options(MachineClass *m)
{
pc_i440fx_2_9_machine_options(m);
- SET_MACHINE_COMPAT(m, PC_COMPAT_2_8);
+ SET_COMPAT(m, PC_COMPAT_2_8);
}
DEFINE_I440FX_MACHINE(v2_8, "pc-i440fx-2.8", NULL,
@@ -500,7 +500,7 @@ DEFINE_I440FX_MACHINE(v2_8, "pc-i440fx-2.8", NULL,
static void pc_i440fx_2_7_machine_options(MachineClass *m)
{
pc_i440fx_2_8_machine_options(m);
- SET_MACHINE_COMPAT(m, PC_COMPAT_2_7);
+ SET_COMPAT(m, PC_COMPAT_2_7);
}
DEFINE_I440FX_MACHINE(v2_7, "pc-i440fx-2.7", NULL,
@@ -513,7 +513,7 @@ static void pc_i440fx_2_6_machine_options(MachineClass *m)
pc_i440fx_2_7_machine_options(m);
pcmc->legacy_cpu_hotplug = true;
pcmc->linuxboot_dma_enabled = false;
- SET_MACHINE_COMPAT(m, PC_COMPAT_2_6);
+ SET_COMPAT(m, PC_COMPAT_2_6);
}
DEFINE_I440FX_MACHINE(v2_6, "pc-i440fx-2.6", NULL,
@@ -526,7 +526,7 @@ static void pc_i440fx_2_5_machine_options(MachineClass *m)
pc_i440fx_2_6_machine_options(m);
pcmc->save_tsc_khz = false;
m->legacy_fw_cfg_order = 1;
- SET_MACHINE_COMPAT(m, PC_COMPAT_2_5);
+ SET_COMPAT(m, PC_COMPAT_2_5);
}
DEFINE_I440FX_MACHINE(v2_5, "pc-i440fx-2.5", NULL,
@@ -539,7 +539,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass *m)
pc_i440fx_2_5_machine_options(m);
m->hw_version = "2.4.0";
pcmc->broken_reserved_end = true;
- SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
+ SET_COMPAT(m, PC_COMPAT_2_4);
}
DEFINE_I440FX_MACHINE(v2_4, "pc-i440fx-2.4", NULL,
@@ -550,7 +550,7 @@ static void pc_i440fx_2_3_machine_options(MachineClass *m)
{
pc_i440fx_2_4_machine_options(m);
m->hw_version = "2.3.0";
- SET_MACHINE_COMPAT(m, PC_COMPAT_2_3);
+ SET_COMPAT(m, PC_COMPAT_2_3);
}
DEFINE_I440FX_MACHINE(v2_3, "pc-i440fx-2.3", pc_compat_2_3,
@@ -562,7 +562,7 @@ static void pc_i440fx_2_2_machine_options(MachineClass *m)
PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
pc_i440fx_2_3_machine_options(m);
m->hw_version = "2.2.0";
- SET_MACHINE_COMPAT(m, PC_COMPAT_2_2);
+ SET_COMPAT(m, PC_COMPAT_2_2);
pcmc->rsdp_in_ram = false;
}
@@ -576,7 +576,7 @@ static void pc_i440fx_2_1_machine_options(MachineClass *m)
pc_i440fx_2_2_machine_options(m);
m->hw_version = "2.1.0";
m->default_display = NULL;
- SET_MACHINE_COMPAT(m, PC_COMPAT_2_1);
+ SET_COMPAT(m, PC_COMPAT_2_1);
pcmc->smbios_uuid_encoded = false;
pcmc->enforce_aligned_dimm = false;
}
@@ -591,7 +591,7 @@ static void pc_i440fx_2_0_machine_options(MachineClass *m)
PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
pc_i440fx_2_1_machine_options(m);
m->hw_version = "2.0.0";
- SET_MACHINE_COMPAT(m, PC_COMPAT_2_0);
+ SET_COMPAT(m, PC_COMPAT_2_0);
pcmc->smbios_legacy_mode = true;
pcmc->has_reserved_memory = false;
/* This value depends on the actual DSDT and SSDT compiled into
@@ -625,7 +625,7 @@ static void pc_i440fx_1_7_machine_options(MachineClass *m)
m->hw_version = "1.7.0";
m->default_machine_opts = NULL;
m->option_rom_has_mr = true;
- SET_MACHINE_COMPAT(m, PC_COMPAT_1_7);
+ SET_COMPAT(m, PC_COMPAT_1_7);
pcmc->smbios_defaults = false;
pcmc->gigabyte_align = false;
pcmc->legacy_acpi_table_size = 6414;
@@ -641,7 +641,7 @@ static void pc_i440fx_1_6_machine_options(MachineClass *m)
pc_i440fx_1_7_machine_options(m);
m->hw_version = "1.6.0";
m->rom_file_has_mr = false;
- SET_MACHINE_COMPAT(m, PC_COMPAT_1_6);
+ SET_COMPAT(m, PC_COMPAT_1_6);
pcmc->has_acpi_build = false;
}
@@ -653,7 +653,7 @@ static void pc_i440fx_1_5_machine_options(MachineClass *m)
{
pc_i440fx_1_6_machine_options(m);
m->hw_version = "1.5.0";
- SET_MACHINE_COMPAT(m, PC_COMPAT_1_5);
+ SET_COMPAT(m, PC_COMPAT_1_5);
}
DEFINE_I440FX_MACHINE(v1_5, "pc-i440fx-1.5", pc_compat_1_5,
@@ -665,7 +665,7 @@ static void pc_i440fx_1_4_machine_options(MachineClass *m)
pc_i440fx_1_5_machine_options(m);
m->hw_version = "1.4.0";
m->hot_add_cpu = NULL;
- SET_MACHINE_COMPAT(m, PC_COMPAT_1_4);
+ SET_COMPAT(m, PC_COMPAT_1_4);
}
DEFINE_I440FX_MACHINE(v1_4, "pc-i440fx-1.4", pc_compat_1_4,
@@ -697,7 +697,7 @@ static void pc_i440fx_1_3_machine_options(MachineClass *m)
{
pc_i440fx_1_4_machine_options(m);
m->hw_version = "1.3.0";
- SET_MACHINE_COMPAT(m, PC_COMPAT_1_3);
+ SET_COMPAT(m, PC_COMPAT_1_3);
}
DEFINE_I440FX_MACHINE(v1_3, "pc-1.3", pc_compat_1_3,
@@ -736,7 +736,7 @@ static void pc_i440fx_1_2_machine_options(MachineClass *m)
{
pc_i440fx_1_3_machine_options(m);
m->hw_version = "1.2.0";
- SET_MACHINE_COMPAT(m, PC_COMPAT_1_2);
+ SET_COMPAT(m, PC_COMPAT_1_2);
}
DEFINE_I440FX_MACHINE(v1_2, "pc-1.2", pc_compat_1_2,
@@ -779,7 +779,7 @@ static void pc_i440fx_1_1_machine_options(MachineClass *m)
{
pc_i440fx_1_2_machine_options(m);
m->hw_version = "1.1.0";
- SET_MACHINE_COMPAT(m, PC_COMPAT_1_1);
+ SET_COMPAT(m, PC_COMPAT_1_1);
}
DEFINE_I440FX_MACHINE(v1_1, "pc-1.1", pc_compat_1_2,
@@ -810,7 +810,7 @@ static void pc_i440fx_1_0_machine_options(MachineClass *m)
{
pc_i440fx_1_1_machine_options(m);
m->hw_version = "1.0";
- SET_MACHINE_COMPAT(m, PC_COMPAT_1_0);
+ SET_COMPAT(m, PC_COMPAT_1_0);
}
DEFINE_I440FX_MACHINE(v1_0, "pc-1.0", pc_compat_1_2,
@@ -824,7 +824,7 @@ static void pc_i440fx_0_15_machine_options(MachineClass *m)
{
pc_i440fx_1_0_machine_options(m);
m->hw_version = "0.15";
- SET_MACHINE_COMPAT(m, PC_COMPAT_0_15);
+ SET_COMPAT(m, PC_COMPAT_0_15);
}
DEFINE_I440FX_MACHINE(v0_15, "pc-0.15", pc_compat_1_2,
@@ -863,7 +863,7 @@ static void pc_i440fx_0_14_machine_options(MachineClass *m)
{
pc_i440fx_0_15_machine_options(m);
m->hw_version = "0.14";
- SET_MACHINE_COMPAT(m, PC_COMPAT_0_14);
+ SET_COMPAT(m, PC_COMPAT_0_14);
}
DEFINE_I440FX_MACHINE(v0_14, "pc-0.14", pc_compat_1_2,
@@ -899,7 +899,7 @@ static void pc_i440fx_0_13_machine_options(MachineClass *m)
PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
pc_i440fx_0_14_machine_options(m);
m->hw_version = "0.13";
- SET_MACHINE_COMPAT(m, PC_COMPAT_0_13);
+ SET_COMPAT(m, PC_COMPAT_0_13);
pcmc->kvmclock_enabled = false;
}
@@ -935,7 +935,7 @@ static void pc_i440fx_0_12_machine_options(MachineClass *m)
{
pc_i440fx_0_13_machine_options(m);
m->hw_version = "0.12";
- SET_MACHINE_COMPAT(m, PC_COMPAT_0_12);
+ SET_COMPAT(m, PC_COMPAT_0_12);
}
DEFINE_I440FX_MACHINE(v0_12, "pc-0.12", pc_compat_0_13,
@@ -967,7 +967,7 @@ static void pc_i440fx_0_11_machine_options(MachineClass *m)
pc_i440fx_0_12_machine_options(m);
m->hw_version = "0.11";
m->deprecation_reason = "use a newer machine type instead";
- SET_MACHINE_COMPAT(m, PC_COMPAT_0_11);
+ SET_COMPAT(m, PC_COMPAT_0_11);
}
DEFINE_I440FX_MACHINE(v0_11, "pc-0.11", pc_compat_0_13,
@@ -1002,7 +1002,7 @@ static void pc_i440fx_0_10_machine_options(MachineClass *m)
{
pc_i440fx_0_11_machine_options(m);
m->hw_version = "0.10";
- SET_MACHINE_COMPAT(m, PC_COMPAT_0_10);
+ SET_COMPAT(m, PC_COMPAT_0_10);
}
DEFINE_I440FX_MACHINE(v0_10, "pc-0.10", pc_compat_0_13,
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 4702bb13c4..19e6ec6675 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -324,7 +324,7 @@ static void pc_q35_3_0_machine_options(MachineClass *m)
{
pc_q35_3_1_machine_options(m);
m->alias = NULL;
- SET_MACHINE_COMPAT(m, PC_COMPAT_3_0);
+ SET_COMPAT(m, PC_COMPAT_3_0);
}
DEFINE_Q35_MACHINE(v3_0, "pc-q35-3.0", NULL,
@@ -333,7 +333,7 @@ DEFINE_Q35_MACHINE(v3_0, "pc-q35-3.0", NULL,
static void pc_q35_2_12_machine_options(MachineClass *m)
{
pc_q35_3_0_machine_options(m);
- SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
+ SET_COMPAT(m, PC_COMPAT_2_12);
}
DEFINE_Q35_MACHINE(v2_12, "pc-q35-2.12", NULL,
@@ -345,7 +345,7 @@ static void pc_q35_2_11_machine_options(MachineClass *m)
pc_q35_2_12_machine_options(m);
pcmc->default_nic_model = "e1000";
- SET_MACHINE_COMPAT(m, PC_COMPAT_2_11);
+ SET_COMPAT(m, PC_COMPAT_2_11);
}
DEFINE_Q35_MACHINE(v2_11, "pc-q35-2.11", NULL,
@@ -354,7 +354,7 @@ DEFINE_Q35_MACHINE(v2_11, "pc-q35-2.11", NULL,
static void pc_q35_2_10_machine_options(MachineClass *m)
{
pc_q35_2_11_machine_options(m);
- SET_MACHINE_COMPAT(m, PC_COMPAT_2_10);
+ SET_COMPAT(m, PC_COMPAT_2_10);
m->numa_auto_assign_ram = numa_legacy_auto_assign_ram;
m->auto_enable_numa_with_memhp = false;
}
@@ -365,7 +365,7 @@ DEFINE_Q35_MACHINE(v2_10, "pc-q35-2.10", NULL,
static void pc_q35_2_9_machine_options(MachineClass *m)
{
pc_q35_2_10_machine_options(m);
- SET_MACHINE_COMPAT(m, PC_COMPAT_2_9);
+ SET_COMPAT(m, PC_COMPAT_2_9);
}
DEFINE_Q35_MACHINE(v2_9, "pc-q35-2.9", NULL,
@@ -374,7 +374,7 @@ DEFINE_Q35_MACHINE(v2_9, "pc-q35-2.9", NULL,
static void pc_q35_2_8_machine_options(MachineClass *m)
{
pc_q35_2_9_machine_options(m);
- SET_MACHINE_COMPAT(m, PC_COMPAT_2_8);
+ SET_COMPAT(m, PC_COMPAT_2_8);
}
DEFINE_Q35_MACHINE(v2_8, "pc-q35-2.8", NULL,
@@ -384,7 +384,7 @@ static void pc_q35_2_7_machine_options(MachineClass *m)
{
pc_q35_2_8_machine_options(m);
m->max_cpus = 255;
- SET_MACHINE_COMPAT(m, PC_COMPAT_2_7);
+ SET_COMPAT(m, PC_COMPAT_2_7);
}
DEFINE_Q35_MACHINE(v2_7, "pc-q35-2.7", NULL,
@@ -396,7 +396,7 @@ static void pc_q35_2_6_machine_options(MachineClass *m)
pc_q35_2_7_machine_options(m);
pcmc->legacy_cpu_hotplug = true;
pcmc->linuxboot_dma_enabled = false;
- SET_MACHINE_COMPAT(m, PC_COMPAT_2_6);
+ SET_COMPAT(m, PC_COMPAT_2_6);
}
DEFINE_Q35_MACHINE(v2_6, "pc-q35-2.6", NULL,
@@ -408,7 +408,7 @@ static void pc_q35_2_5_machine_options(MachineClass *m)
pc_q35_2_6_machine_options(m);
pcmc->save_tsc_khz = false;
m->legacy_fw_cfg_order = 1;
- SET_MACHINE_COMPAT(m, PC_COMPAT_2_5);
+ SET_COMPAT(m, PC_COMPAT_2_5);
}
DEFINE_Q35_MACHINE(v2_5, "pc-q35-2.5", NULL,
@@ -420,7 +420,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
pc_q35_2_5_machine_options(m);
m->hw_version = "2.4.0";
pcmc->broken_reserved_end = true;
- SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
+ SET_COMPAT(m, PC_COMPAT_2_4);
}
DEFINE_Q35_MACHINE(v2_4, "pc-q35-2.4", NULL,
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 7afd1a175b..f36554687f 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3986,7 +3986,7 @@ static void spapr_machine_3_0_class_options(MachineClass *mc)
sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
spapr_machine_3_1_class_options(mc);
- SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_3_0);
+ SET_COMPAT(mc, SPAPR_COMPAT_3_0);
smc->legacy_irq_allocation = true;
smc->irq = &spapr_irq_xics_legacy;
@@ -4020,7 +4020,7 @@ static void spapr_machine_2_12_class_options(MachineClass *mc)
sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
spapr_machine_3_0_class_options(mc);
- SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_12);
+ SET_COMPAT(mc, SPAPR_COMPAT_2_12);
/* We depend on kvm_enabled() to choose a default value for the
* hpt-max-page-size capability. Of course we can't do it here
@@ -4066,7 +4066,7 @@ static void spapr_machine_2_11_class_options(MachineClass *mc)
spapr_machine_2_12_class_options(mc);
smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_ON;
- SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_11);
+ SET_COMPAT(mc, SPAPR_COMPAT_2_11);
}
DEFINE_SPAPR_MACHINE(2_11, "2.11", false);
@@ -4085,7 +4085,7 @@ static void spapr_machine_2_10_instance_options(MachineState *machine)
static void spapr_machine_2_10_class_options(MachineClass *mc)
{
spapr_machine_2_11_class_options(mc);
- SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_10);
+ SET_COMPAT(mc, SPAPR_COMPAT_2_10);
}
DEFINE_SPAPR_MACHINE(2_10, "2.10", false);
@@ -4111,7 +4111,7 @@ static void spapr_machine_2_9_class_options(MachineClass *mc)
sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
spapr_machine_2_10_class_options(mc);
- SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_9);
+ SET_COMPAT(mc, SPAPR_COMPAT_2_9);
mc->numa_auto_assign_ram = numa_legacy_auto_assign_ram;
smc->pre_2_10_has_unused_icps = true;
smc->resize_hpt_default = SPAPR_RESIZE_HPT_DISABLED;
@@ -4138,7 +4138,7 @@ static void spapr_machine_2_8_instance_options(MachineState *machine)
static void spapr_machine_2_8_class_options(MachineClass *mc)
{
spapr_machine_2_9_class_options(mc);
- SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_8);
+ SET_COMPAT(mc, SPAPR_COMPAT_2_8);
mc->numa_mem_align_shift = 23;
}
@@ -4233,7 +4233,7 @@ static void spapr_machine_2_7_class_options(MachineClass *mc)
spapr_machine_2_8_class_options(mc);
mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power7_v2.3");
- SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_7);
+ SET_COMPAT(mc, SPAPR_COMPAT_2_7);
smc->phb_placement = phb_placement_2_7;
}
@@ -4259,7 +4259,7 @@ static void spapr_machine_2_6_class_options(MachineClass *mc)
{
spapr_machine_2_7_class_options(mc);
mc->has_hotpluggable_cpus = false;
- SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_6);
+ SET_COMPAT(mc, SPAPR_COMPAT_2_6);
}
DEFINE_SPAPR_MACHINE(2_6, "2.6", false);
@@ -4286,7 +4286,7 @@ static void spapr_machine_2_5_class_options(MachineClass *mc)
spapr_machine_2_6_class_options(mc);
smc->use_ohci_by_default = true;
- SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_5);
+ SET_COMPAT(mc, SPAPR_COMPAT_2_5);
}
DEFINE_SPAPR_MACHINE(2_5, "2.5", false);
@@ -4308,7 +4308,7 @@ static void spapr_machine_2_4_class_options(MachineClass *mc)
spapr_machine_2_5_class_options(mc);
smc->dr_lmb_enabled = false;
- SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_4);
+ SET_COMPAT(mc, SPAPR_COMPAT_2_4);
}
DEFINE_SPAPR_MACHINE(2_4, "2.4", false);
@@ -4332,7 +4332,7 @@ static void spapr_machine_2_3_instance_options(MachineState *machine)
static void spapr_machine_2_3_class_options(MachineClass *mc)
{
spapr_machine_2_4_class_options(mc);
- SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_3);
+ SET_COMPAT(mc, SPAPR_COMPAT_2_3);
}
DEFINE_SPAPR_MACHINE(2_3, "2.3", false);
@@ -4357,7 +4357,7 @@ static void spapr_machine_2_2_instance_options(MachineState *machine)
static void spapr_machine_2_2_class_options(MachineClass *mc)
{
spapr_machine_2_3_class_options(mc);
- SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_2);
+ SET_COMPAT(mc, SPAPR_COMPAT_2_2);
}
DEFINE_SPAPR_MACHINE(2_2, "2.2", false);
@@ -4375,7 +4375,7 @@ static void spapr_machine_2_1_instance_options(MachineState *machine)
static void spapr_machine_2_1_class_options(MachineClass *mc)
{
spapr_machine_2_2_class_options(mc);
- SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_1);
+ SET_COMPAT(mc, SPAPR_COMPAT_2_1);
}
DEFINE_SPAPR_MACHINE(2_1, "2.1", false);
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index a0615a8b35..41b72356e9 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -762,7 +762,7 @@ static void ccw_machine_3_0_class_options(MachineClass *mc)
s390mc->hpage_1m_allowed = false;
ccw_machine_3_1_class_options(mc);
- SET_MACHINE_COMPAT(mc, CCW_COMPAT_3_0);
+ SET_COMPAT(mc, CCW_COMPAT_3_0);
}
DEFINE_CCW_MACHINE(3_0, "3.0", false);
@@ -776,7 +776,7 @@ static void ccw_machine_2_12_instance_options(MachineState *machine)
static void ccw_machine_2_12_class_options(MachineClass *mc)
{
ccw_machine_3_0_class_options(mc);
- SET_MACHINE_COMPAT(mc, CCW_COMPAT_2_12);
+ SET_COMPAT(mc, CCW_COMPAT_2_12);
}
DEFINE_CCW_MACHINE(2_12, "2.12", false);
@@ -792,7 +792,7 @@ static void ccw_machine_2_11_instance_options(MachineState *machine)
static void ccw_machine_2_11_class_options(MachineClass *mc)
{
ccw_machine_2_12_class_options(mc);
- SET_MACHINE_COMPAT(mc, CCW_COMPAT_2_11);
+ SET_COMPAT(mc, CCW_COMPAT_2_11);
}
DEFINE_CCW_MACHINE(2_11, "2.11", false);
@@ -804,7 +804,7 @@ static void ccw_machine_2_10_instance_options(MachineState *machine)
static void ccw_machine_2_10_class_options(MachineClass *mc)
{
ccw_machine_2_11_class_options(mc);
- SET_MACHINE_COMPAT(mc, CCW_COMPAT_2_10);
+ SET_COMPAT(mc, CCW_COMPAT_2_10);
}
DEFINE_CCW_MACHINE(2_10, "2.10", false);
@@ -823,7 +823,7 @@ static void ccw_machine_2_9_class_options(MachineClass *mc)
S390CcwMachineClass *s390mc = S390_MACHINE_CLASS(mc);
ccw_machine_2_10_class_options(mc);
- SET_MACHINE_COMPAT(mc, CCW_COMPAT_2_9);
+ SET_COMPAT(mc, CCW_COMPAT_2_9);
s390mc->css_migration_enabled = false;
}
DEFINE_CCW_MACHINE(2_9, "2.9", false);
@@ -836,7 +836,7 @@ static void ccw_machine_2_8_instance_options(MachineState *machine)
static void ccw_machine_2_8_class_options(MachineClass *mc)
{
ccw_machine_2_9_class_options(mc);
- SET_MACHINE_COMPAT(mc, CCW_COMPAT_2_8);
+ SET_COMPAT(mc, CCW_COMPAT_2_8);
}
DEFINE_CCW_MACHINE(2_8, "2.8", false);
@@ -851,7 +851,7 @@ static void ccw_machine_2_7_class_options(MachineClass *mc)
s390mc->cpu_model_allowed = false;
ccw_machine_2_8_class_options(mc);
- SET_MACHINE_COMPAT(mc, CCW_COMPAT_2_7);
+ SET_COMPAT(mc, CCW_COMPAT_2_7);
}
DEFINE_CCW_MACHINE(2_7, "2.7", false);
@@ -866,7 +866,7 @@ static void ccw_machine_2_6_class_options(MachineClass *mc)
s390mc->ri_allowed = false;
ccw_machine_2_7_class_options(mc);
- SET_MACHINE_COMPAT(mc, CCW_COMPAT_2_6);
+ SET_COMPAT(mc, CCW_COMPAT_2_6);
}
DEFINE_CCW_MACHINE(2_6, "2.6", false);
@@ -878,7 +878,7 @@ static void ccw_machine_2_5_instance_options(MachineState *machine)
static void ccw_machine_2_5_class_options(MachineClass *mc)
{
ccw_machine_2_6_class_options(mc);
- SET_MACHINE_COMPAT(mc, CCW_COMPAT_2_5);
+ SET_COMPAT(mc, CCW_COMPAT_2_5);
}
DEFINE_CCW_MACHINE(2_5, "2.5", false);
@@ -890,7 +890,7 @@ static void ccw_machine_2_4_instance_options(MachineState *machine)
static void ccw_machine_2_4_class_options(MachineClass *mc)
{
ccw_machine_2_5_class_options(mc);
- SET_MACHINE_COMPAT(mc, CCW_COMPAT_2_4);
+ SET_COMPAT(mc, CCW_COMPAT_2_4);
}
DEFINE_CCW_MACHINE(2_4, "2.4", false);
diff --git a/vl.c b/vl.c
index c06e94271c..943e9b6a69 100644
--- a/vl.c
+++ b/vl.c
@@ -2964,7 +2964,7 @@ static void user_register_global_props(void)
static void register_global_properties(MachineState *ms)
{
accel_register_compat_props(ACCEL_GET_CLASS(ms->accelerator)->compat_props);
- machine_register_compat_props(ms);
+ machine_register_compat_props(MACHINE_GET_CLASS(ms)->compat_props);
user_register_global_props();
}
--
2.20.0.rc1
On Tue, Nov 27, 2018 at 01:27:49PM +0400, Marc-André Lureau wrote:
> Similarly to accel properties, move compat properties out of globals
> registration, and apply the machine compat properties during
> device_post_init().
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
[...]
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 7066d28271..3b31b2c025 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -971,17 +971,26 @@ static void device_initfn(Object *obj)
> }
>
> static const GPtrArray *ac_compat_props;
> +static const GPtrArray *mc_compat_props;
>
> void accel_register_compat_props(const GPtrArray *props)
> {
> ac_compat_props = props;
> }
>
> +void machine_register_compat_props(const GPtrArray *props)
> +{
> + mc_compat_props = props;
> +}
> +
> static void device_post_init(Object *obj)
> {
> if (ac_compat_props) {
> object_apply_global_props(obj, ac_compat_props, &error_abort);
> }
Why not just use MACHINE(qdev_get_machine())->accel->compat_props
directly?
> + if (mc_compat_props) {
> + object_apply_global_props(obj, mc_compat_props, &error_abort);
> + }
Why not just use MACHINE(qdev_get_machine())->compat_props
directly?
>
> qdev_prop_set_globals(DEVICE(obj));
> }
[...]
--
Eduardo
On Tue, Nov 27, 2018 at 4:57 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> On Tue, Nov 27, 2018 at 01:27:49PM +0400, Marc-André Lureau wrote:
> > Similarly to accel properties, move compat properties out of globals
> > registration, and apply the machine compat properties during
> > device_post_init().
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> [...]
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index 7066d28271..3b31b2c025 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -971,17 +971,26 @@ static void device_initfn(Object *obj)
> > }
> >
> > static const GPtrArray *ac_compat_props;
> > +static const GPtrArray *mc_compat_props;
> >
> > void accel_register_compat_props(const GPtrArray *props)
> > {
> > ac_compat_props = props;
> > }
> >
> > +void machine_register_compat_props(const GPtrArray *props)
> > +{
> > + mc_compat_props = props;
> > +}
> > +
> > static void device_post_init(Object *obj)
> > {
> > if (ac_compat_props) {
> > object_apply_global_props(obj, ac_compat_props, &error_abort);
> > }
>
> Why not just use MACHINE(qdev_get_machine())->accel->compat_props
> directly?
>
> > + if (mc_compat_props) {
> > + object_apply_global_props(obj, mc_compat_props, &error_abort);
> > + }
>
> Why not just use MACHINE(qdev_get_machine())->compat_props
> directly?
This was the approach in v3, but Igor didn't quite like referencing
machine in qdev:
https://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg04774.html
>
> >
> > qdev_prop_set_globals(DEVICE(obj));
> > }
> [...]
>
> --
> Eduardo
On Tue, Nov 27, 2018 at 05:10:05PM +0400, Marc-André Lureau wrote:
> On Tue, Nov 27, 2018 at 4:57 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> > On Tue, Nov 27, 2018 at 01:27:49PM +0400, Marc-André Lureau wrote:
> > > Similarly to accel properties, move compat properties out of globals
> > > registration, and apply the machine compat properties during
> > > device_post_init().
> > >
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > [...]
> > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > > index 7066d28271..3b31b2c025 100644
> > > --- a/hw/core/qdev.c
> > > +++ b/hw/core/qdev.c
> > > @@ -971,17 +971,26 @@ static void device_initfn(Object *obj)
> > > }
> > >
> > > static const GPtrArray *ac_compat_props;
> > > +static const GPtrArray *mc_compat_props;
> > >
> > > void accel_register_compat_props(const GPtrArray *props)
> > > {
> > > ac_compat_props = props;
> > > }
> > >
> > > +void machine_register_compat_props(const GPtrArray *props)
> > > +{
> > > + mc_compat_props = props;
> > > +}
> > > +
> > > static void device_post_init(Object *obj)
> > > {
> > > if (ac_compat_props) {
> > > object_apply_global_props(obj, ac_compat_props, &error_abort);
> > > }
> >
> > Why not just use MACHINE(qdev_get_machine())->accel->compat_props
> > directly?
> >
> > > + if (mc_compat_props) {
> > > + object_apply_global_props(obj, mc_compat_props, &error_abort);
> > > + }
> >
> > Why not just use MACHINE(qdev_get_machine())->compat_props
> > directly?
>
> This was the approach in v3, but Igor didn't quite like referencing
> machine in qdev:
> https://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg04774.html
I disagree with Igor, here. Core qdev code already have multiple
references to machine, I don't see any problem with that.
The previous code was clearer and easier to follow, and wasn't
sensitive to subtle changes in initialization ordering (e.g. what
happens if we create a device before *_register_compat_props() is
called?).
>
> >
> > >
> > > qdev_prop_set_globals(DEVICE(obj));
> > > }
> > [...]
> >
> > --
> > Eduardo
--
Eduardo
On Tue, 27 Nov 2018 11:35:27 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Tue, Nov 27, 2018 at 05:10:05PM +0400, Marc-André Lureau wrote:
> > On Tue, Nov 27, 2018 at 4:57 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >
> > > On Tue, Nov 27, 2018 at 01:27:49PM +0400, Marc-André Lureau wrote:
> > > > Similarly to accel properties, move compat properties out of globals
> > > > registration, and apply the machine compat properties during
> > > > device_post_init().
> > > >
> > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > [...]
> > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > > > index 7066d28271..3b31b2c025 100644
> > > > --- a/hw/core/qdev.c
> > > > +++ b/hw/core/qdev.c
> > > > @@ -971,17 +971,26 @@ static void device_initfn(Object *obj)
> > > > }
> > > >
> > > > static const GPtrArray *ac_compat_props;
> > > > +static const GPtrArray *mc_compat_props;
why you didn't use just 'compat_props' for both?
(it would be cleaner have single registry for compat
properties, and the place that takes care of registration
will take care of necessary ordering)
> > > >
> > > > void accel_register_compat_props(const GPtrArray *props)
> > > > {
> > > > ac_compat_props = props;
> > > > }
> > > >
> > > > +void machine_register_compat_props(const GPtrArray *props)
> > > > +{
> > > > + mc_compat_props = props;
> > > > +}
> > > > +
> > > > static void device_post_init(Object *obj)
> > > > {
> > > > if (ac_compat_props) {
> > > > object_apply_global_props(obj, ac_compat_props, &error_abort);
> > > > }
> > >
> > > Why not just use MACHINE(qdev_get_machine())->accel->compat_props
> > > directly?
> > >
> > > > + if (mc_compat_props) {
> > > > + object_apply_global_props(obj, mc_compat_props, &error_abort);
> > > > + }
> > >
> > > Why not just use MACHINE(qdev_get_machine())->compat_props
> > > directly?
> >
> > This was the approach in v3, but Igor didn't quite like referencing
> > machine in qdev:
> > https://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg04774.html
>
> I disagree with Igor, here. Core qdev code already have multiple
> references to machine, I don't see any problem with that.
(There are only 3 calls to qdev_get_machine() in core qdev.c
blame me for adding one there. Which were hacks so we won't
have to re-factor core qdev code. But that doesn't justify adding more.)
This patch is an interim one and later in 25/28
device_post_init() content is moved to a more generic compat interface
implementation. That intended for use with types derived from Object
(i.e. not only qdev stuff). Hence I'd like to decouple it from
machine as a standalone feature as much as possible. So that
machine (or whatever else) will opt in in using facility.
> The previous code was clearer and easier to follow, and wasn't
> sensitive to subtle changes in initialization ordering (e.g. what
> happens if we create a device before *_register_compat_props() is
> called?).
Indeed It seems clearer to follow (that was my first impression as well),
until I went through whole series and thought it's basically the same,
So my choice was to use cleaner approach that we won't have to rewrite
in near future.
Thanks for bringing up ordering issue, we probably have one in this series.
But beside possible issue here, even with v3 variant we would still have
issues if objects are created before machine and accelerator instances are
created.
More correct way could be to register compat properties right away at
select_machine() time, we don't really need an instance for that, just access
to machine_class and do the same for 'accel' option. (that's probably doable
within this series) + some time later (on top of this series) a check that
no TYPE_COMPAT_PROPS were created at the moment compat properties are registered
so we would notice when we write something wrong.
If it's too much of refactoring (series is already big as it is), I would
compromise on qdev_get_machine() and adding TODO comments (or a series on top)
to make it correct and "race-resistant".
Marc are you sure it actually will work as expected with Object derived types?
register_global_properties()
is being called after
qemu_opts_foreach(... user_creatable_add_opts_foreach, object_create_initial ...)
so there is no compat properties registered when objects are created.
> >
> > >
> > > >
> > > > qdev_prop_set_globals(DEVICE(obj));
> > > > }
> > > [...]
> > >
> > > --
> > > Eduardo
>
On Wed, Nov 28, 2018 at 06:40:27PM +0100, Igor Mammedov wrote:
> On Tue, 27 Nov 2018 11:35:27 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> > On Tue, Nov 27, 2018 at 05:10:05PM +0400, Marc-André Lureau wrote:
> > > On Tue, Nov 27, 2018 at 4:57 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > >
> > > > On Tue, Nov 27, 2018 at 01:27:49PM +0400, Marc-André Lureau wrote:
> > > > > Similarly to accel properties, move compat properties out of globals
> > > > > registration, and apply the machine compat properties during
> > > > > device_post_init().
> > > > >
> > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > [...]
> > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > > > > index 7066d28271..3b31b2c025 100644
> > > > > --- a/hw/core/qdev.c
> > > > > +++ b/hw/core/qdev.c
> > > > > @@ -971,17 +971,26 @@ static void device_initfn(Object *obj)
> > > > > }
> > > > >
> > > > > static const GPtrArray *ac_compat_props;
> > > > > +static const GPtrArray *mc_compat_props;
> why you didn't use just 'compat_props' for both?
> (it would be cleaner have single registry for compat
> properties, and the place that takes care of registration
> will take care of necessary ordering)
>
> > > > >
> > > > > void accel_register_compat_props(const GPtrArray *props)
> > > > > {
> > > > > ac_compat_props = props;
> > > > > }
> > > > >
> > > > > +void machine_register_compat_props(const GPtrArray *props)
> > > > > +{
> > > > > + mc_compat_props = props;
> > > > > +}
> > > > > +
> > > > > static void device_post_init(Object *obj)
> > > > > {
> > > > > if (ac_compat_props) {
> > > > > object_apply_global_props(obj, ac_compat_props, &error_abort);
> > > > > }
> > > >
> > > > Why not just use MACHINE(qdev_get_machine())->accel->compat_props
> > > > directly?
> > > >
> > > > > + if (mc_compat_props) {
> > > > > + object_apply_global_props(obj, mc_compat_props, &error_abort);
> > > > > + }
> > > >
> > > > Why not just use MACHINE(qdev_get_machine())->compat_props
> > > > directly?
> > >
> > > This was the approach in v3, but Igor didn't quite like referencing
> > > machine in qdev:
> > > https://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg04774.html
> >
> > I disagree with Igor, here. Core qdev code already have multiple
> > references to machine, I don't see any problem with that.
> (There are only 3 calls to qdev_get_machine() in core qdev.c
> blame me for adding one there. Which were hacks so we won't
> have to re-factor core qdev code. But that doesn't justify adding more.)
>
> This patch is an interim one and later in 25/28
> device_post_init() content is moved to a more generic compat interface
> implementation. That intended for use with types derived from Object
> (i.e. not only qdev stuff). Hence I'd like to decouple it from
> machine as a standalone feature as much as possible. So that
> machine (or whatever else) will opt in in using facility.
Why do you want to decouple that from machine? The whole purpose
of the code being added by this patch (and of TYPE_COMPAT_PROPS)
is to have objects being affected by machine-provided and
accel-provided compat properties. Having a dependency on
machine+accel is part of its specification.
>
> > The previous code was clearer and easier to follow, and wasn't
> > sensitive to subtle changes in initialization ordering (e.g. what
> > happens if we create a device before *_register_compat_props() is
> > called?).
> Indeed It seems clearer to follow (that was my first impression as well),
> until I went through whole series and thought it's basically the same,
> So my choice was to use cleaner approach that we won't have to rewrite
> in near future.
Why you think it is cleaner? Copying data around to static
variables isn't cleaner and it's more fragile.
>
> Thanks for bringing up ordering issue, we probably have one in this series.
>
> But beside possible issue here, even with v3 variant we would still have
> issues if objects are created before machine and accelerator instances are
> created.
> More correct way could be to register compat properties right away at
> select_machine() time, we don't really need an instance for that, just access
> to machine_class and do the same for 'accel' option. (that's probably doable
> within this series) + some time later (on top of this series) a check that
> no TYPE_COMPAT_PROPS were created at the moment compat properties are registered
> so we would notice when we write something wrong.
What problem you are trying to solve with this extra complexity,
exactly?
>
> If it's too much of refactoring (series is already big as it is), I would
> compromise on qdev_get_machine() and adding TODO comments (or a series on top)
> to make it correct and "race-resistant".
It's not clear to me what the TODO comment would say. What's the
issue you're trying to solve?
>
> Marc are you sure it actually will work as expected with Object derived types?
> register_global_properties()
> is being called after
> qemu_opts_foreach(... user_creatable_add_opts_foreach, object_create_initial ...)
> so there is no compat properties registered when objects are created.
>
> > >
> > > >
> > > > >
> > > > > qdev_prop_set_globals(DEVICE(obj));
> > > > > }
> > > > [...]
> > > >
> > > > --
> > > > Eduardo
> >
>
--
Eduardo
Hi
On Wed, Nov 28, 2018 at 9:53 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Tue, 27 Nov 2018 11:35:27 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> > On Tue, Nov 27, 2018 at 05:10:05PM +0400, Marc-André Lureau wrote:
> > > On Tue, Nov 27, 2018 at 4:57 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > >
> > > > On Tue, Nov 27, 2018 at 01:27:49PM +0400, Marc-André Lureau wrote:
> > > > > Similarly to accel properties, move compat properties out of globals
> > > > > registration, and apply the machine compat properties during
> > > > > device_post_init().
> > > > >
> > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > [...]
> > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > > > > index 7066d28271..3b31b2c025 100644
> > > > > --- a/hw/core/qdev.c
> > > > > +++ b/hw/core/qdev.c
> > > > > @@ -971,17 +971,26 @@ static void device_initfn(Object *obj)
> > > > > }
> > > > >
> > > > > static const GPtrArray *ac_compat_props;
> > > > > +static const GPtrArray *mc_compat_props;
> why you didn't use just 'compat_props' for both?
> (it would be cleaner have single registry for compat
> properties, and the place that takes care of registration
> will take care of necessary ordering)
There are two arrays, one from the accelerator class, the other from
the machine class. We can't make it a singleton (all compats props for
the various machines would be mixed).
We could create a third array that would be the set of both, but that
would require more copy/allocation.
>
> > > > >
> > > > > void accel_register_compat_props(const GPtrArray *props)
> > > > > {
> > > > > ac_compat_props = props;
> > > > > }
> > > > >
> > > > > +void machine_register_compat_props(const GPtrArray *props)
> > > > > +{
> > > > > + mc_compat_props = props;
> > > > > +}
> > > > > +
> > > > > static void device_post_init(Object *obj)
> > > > > {
> > > > > if (ac_compat_props) {
> > > > > object_apply_global_props(obj, ac_compat_props, &error_abort);
> > > > > }
> > > >
> > > > Why not just use MACHINE(qdev_get_machine())->accel->compat_props
> > > > directly?
> > > >
> > > > > + if (mc_compat_props) {
> > > > > + object_apply_global_props(obj, mc_compat_props, &error_abort);
> > > > > + }
> > > >
> > > > Why not just use MACHINE(qdev_get_machine())->compat_props
> > > > directly?
> > >
> > > This was the approach in v3, but Igor didn't quite like referencing
> > > machine in qdev:
> > > https://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg04774.html
> >
> > I disagree with Igor, here. Core qdev code already have multiple
> > references to machine, I don't see any problem with that.
> (There are only 3 calls to qdev_get_machine() in core qdev.c
> blame me for adding one there. Which were hacks so we won't
> have to re-factor core qdev code. But that doesn't justify adding more.)
>
> This patch is an interim one and later in 25/28
> device_post_init() content is moved to a more generic compat interface
> implementation. That intended for use with types derived from Object
> (i.e. not only qdev stuff). Hence I'd like to decouple it from
> machine as a standalone feature as much as possible. So that
> machine (or whatever else) will opt in in using facility.
>
> > The previous code was clearer and easier to follow, and wasn't
> > sensitive to subtle changes in initialization ordering (e.g. what
> > happens if we create a device before *_register_compat_props() is
> > called?).
> Indeed It seems clearer to follow (that was my first impression as well),
> until I went through whole series and thought it's basically the same,
> So my choice was to use cleaner approach that we won't have to rewrite
> in near future.
>
> Thanks for bringing up ordering issue, we probably have one in this series.
>
> But beside possible issue here, even with v3 variant we would still have
> issues if objects are created before machine and accelerator instances are
> created.
> More correct way could be to register compat properties right away at
> select_machine() time, we don't really need an instance for that, just access
> to machine_class and do the same for 'accel' option. (that's probably doable
> within this series) + some time later (on top of this series) a check that
> no TYPE_COMPAT_PROPS were created at the moment compat properties are registered
> so we would notice when we write something wrong.
ok, I can look at that
>
> If it's too much of refactoring (series is already big as it is), I would
> compromise on qdev_get_machine() and adding TODO comments (or a series on top)
> to make it correct and "race-resistant".
>
> Marc are you sure it actually will work as expected with Object derived types?
> register_global_properties()
> is being called after
> qemu_opts_foreach(... user_creatable_add_opts_foreach, object_create_initial ...)
> so there is no compat properties registered when objects are created.
Good point, but in the case of hostmem, it works because
object_create_initial delays its creation.
> > >
> > > >
> > > > >
> > > > > qdev_prop_set_globals(DEVICE(obj));
> > > > > }
> > > > [...]
> > > >
> > > > --
> > > > Eduardo
> >
>
>
--
Marc-André Lureau
On Thu, Nov 29, 2018 at 02:32:18PM +0400, Marc-André Lureau wrote:
> Hi
> On Wed, Nov 28, 2018 at 9:53 PM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Tue, 27 Nov 2018 11:35:27 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> > > On Tue, Nov 27, 2018 at 05:10:05PM +0400, Marc-André Lureau wrote:
> > > > On Tue, Nov 27, 2018 at 4:57 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > >
> > > > > On Tue, Nov 27, 2018 at 01:27:49PM +0400, Marc-André Lureau wrote:
> > > > > > Similarly to accel properties, move compat properties out of globals
> > > > > > registration, and apply the machine compat properties during
> > > > > > device_post_init().
> > > > > >
> > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > [...]
> > > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > > > > > index 7066d28271..3b31b2c025 100644
> > > > > > --- a/hw/core/qdev.c
> > > > > > +++ b/hw/core/qdev.c
> > > > > > @@ -971,17 +971,26 @@ static void device_initfn(Object *obj)
> > > > > > }
> > > > > >
> > > > > > static const GPtrArray *ac_compat_props;
> > > > > > +static const GPtrArray *mc_compat_props;
> > why you didn't use just 'compat_props' for both?
> > (it would be cleaner have single registry for compat
> > properties, and the place that takes care of registration
> > will take care of necessary ordering)
>
> There are two arrays, one from the accelerator class, the other from
> the machine class. We can't make it a singleton (all compats props for
> the various machines would be mixed).
>
> We could create a third array that would be the set of both, but that
> would require more copy/allocation.
I am failing to see the advantage of replacing the `global_props`
static variable from qdev-properties.c with a collection of
separate static variables scattered around the code. I thought
the main point of the changes was to reduce the amount of
duplicate data stored in static variables.
I was expecting something like this:
accel.c:
void accel_apply_compat_props(AccelState *accel, Object *obj)
{
object_apply_global_props(obj, ACCEL_GET_CLASS(accel)->compat_props, &error_abort);
}
machine.c:
/* Apply compat properties and global properties to an object */
void machine_apply_compat_props(MachineState *ms, Object *obj)
{
accel_apply_compat_props(ms->accel, obj);
object_apply_global_props(obj, MACHINE_GET_CLASS(ms)->compat_props, &error_abort);
}
compat-props.c:
static void object_apply_compat_props(Object *obj)
{
MachineState *machine = MACHINE(qdev_get_machine());
machine_apply_compat_props(machine, obj);
}
qdev.c:
static void device_post_init(Object *obj)
{
object_apply_compat_props(obj);
apply_user_provided_globals(obj);
}
object_interface.c:
void user_creatable_complete(Object *obj, Error **errp)
{
object_apply_compat_props(obj);
...
ucc->complete(...)
}
--
Eduardo
Hi
On Thu, Nov 29, 2018 at 9:51 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> On Thu, Nov 29, 2018 at 02:32:18PM +0400, Marc-André Lureau wrote:
> > Hi
> > On Wed, Nov 28, 2018 at 9:53 PM Igor Mammedov <imammedo@redhat.com> wrote:
> > >
> > > On Tue, 27 Nov 2018 11:35:27 -0200
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >
> > > > On Tue, Nov 27, 2018 at 05:10:05PM +0400, Marc-André Lureau wrote:
> > > > > On Tue, Nov 27, 2018 at 4:57 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > > >
> > > > > > On Tue, Nov 27, 2018 at 01:27:49PM +0400, Marc-André Lureau wrote:
> > > > > > > Similarly to accel properties, move compat properties out of globals
> > > > > > > registration, and apply the machine compat properties during
> > > > > > > device_post_init().
> > > > > > >
> > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > > [...]
> > > > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > > > > > > index 7066d28271..3b31b2c025 100644
> > > > > > > --- a/hw/core/qdev.c
> > > > > > > +++ b/hw/core/qdev.c
> > > > > > > @@ -971,17 +971,26 @@ static void device_initfn(Object *obj)
> > > > > > > }
> > > > > > >
> > > > > > > static const GPtrArray *ac_compat_props;
> > > > > > > +static const GPtrArray *mc_compat_props;
> > > why you didn't use just 'compat_props' for both?
> > > (it would be cleaner have single registry for compat
> > > properties, and the place that takes care of registration
> > > will take care of necessary ordering)
> >
> > There are two arrays, one from the accelerator class, the other from
> > the machine class. We can't make it a singleton (all compats props for
> > the various machines would be mixed).
> >
> > We could create a third array that would be the set of both, but that
> > would require more copy/allocation.
>
> I am failing to see the advantage of replacing the `global_props`
> static variable from qdev-properties.c with a collection of
> separate static variables scattered around the code. I thought
> the main point of the changes was to reduce the amount of
> duplicate data stored in static variables.
>
> I was expecting something like this:
>
> accel.c:
>
> void accel_apply_compat_props(AccelState *accel, Object *obj)
> {
> object_apply_global_props(obj, ACCEL_GET_CLASS(accel)->compat_props, &error_abort);
> }
>
> machine.c:
>
> /* Apply compat properties and global properties to an object */
> void machine_apply_compat_props(MachineState *ms, Object *obj)
> {
> accel_apply_compat_props(ms->accel, obj);
> object_apply_global_props(obj, MACHINE_GET_CLASS(ms)->compat_props, &error_abort);
> }
>
> compat-props.c:
>
> static void object_apply_compat_props(Object *obj)
> {
> MachineState *machine = MACHINE(qdev_get_machine());
> machine_apply_compat_props(machine, obj);
> }
>
> qdev.c:
>
> static void device_post_init(Object *obj)
> {
> object_apply_compat_props(obj);
> apply_user_provided_globals(obj);
> }
>
> object_interface.c:
>
> void user_creatable_complete(Object *obj, Error **errp)
> {
> object_apply_compat_props(obj);
> ...
> ucc->complete(...)
> }
>
I like that solution too (which you also proposed in the other
thread). But we have to decide whether it's acceptable to reference
MachineState, or if the compat properties should be registered.
--
Marc-André Lureau
On Fri, 30 Nov 2018 01:36:03 +0400
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> Hi
>
> On Thu, Nov 29, 2018 at 9:51 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> > On Thu, Nov 29, 2018 at 02:32:18PM +0400, Marc-André Lureau wrote:
> > > Hi
> > > On Wed, Nov 28, 2018 at 9:53 PM Igor Mammedov <imammedo@redhat.com> wrote:
> > > >
> > > > On Tue, 27 Nov 2018 11:35:27 -0200
> > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > >
> > > > > On Tue, Nov 27, 2018 at 05:10:05PM +0400, Marc-André Lureau wrote:
> > > > > > On Tue, Nov 27, 2018 at 4:57 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > > > >
> > > > > > > On Tue, Nov 27, 2018 at 01:27:49PM +0400, Marc-André Lureau wrote:
> > > > > > > > Similarly to accel properties, move compat properties out of globals
> > > > > > > > registration, and apply the machine compat properties during
> > > > > > > > device_post_init().
> > > > > > > >
> > > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > > > [...]
> > > > > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > > > > > > > index 7066d28271..3b31b2c025 100644
> > > > > > > > --- a/hw/core/qdev.c
> > > > > > > > +++ b/hw/core/qdev.c
> > > > > > > > @@ -971,17 +971,26 @@ static void device_initfn(Object *obj)
> > > > > > > > }
> > > > > > > >
> > > > > > > > static const GPtrArray *ac_compat_props;
> > > > > > > > +static const GPtrArray *mc_compat_props;
> > > > why you didn't use just 'compat_props' for both?
> > > > (it would be cleaner have single registry for compat
> > > > properties, and the place that takes care of registration
> > > > will take care of necessary ordering)
> > >
> > > There are two arrays, one from the accelerator class, the other from
> > > the machine class. We can't make it a singleton (all compats props for
> > > the various machines would be mixed).
> > >
> > > We could create a third array that would be the set of both, but that
> > > would require more copy/allocation.
> >
> > I am failing to see the advantage of replacing the `global_props`
> > static variable from qdev-properties.c with a collection of
> > separate static variables scattered around the code. I thought
> > the main point of the changes was to reduce the amount of
> > duplicate data stored in static variables.
Main point was to use compats for backends then on top of that
we added split global from compat properties goal.
> > I was expecting something like this:
> >
> > accel.c:
> >
> > void accel_apply_compat_props(AccelState *accel, Object *obj)
> > {
> > object_apply_global_props(obj, ACCEL_GET_CLASS(accel)->compat_props, &error_abort);
> > }
> >
> > machine.c:
> >
> > /* Apply compat properties and global properties to an object */
> > void machine_apply_compat_props(MachineState *ms, Object *obj)
> > {
> > accel_apply_compat_props(ms->accel, obj);
> > object_apply_global_props(obj, MACHINE_GET_CLASS(ms)->compat_props, &error_abort);
> > }
> >
> > compat-props.c:
> >
> > static void object_apply_compat_props(Object *obj)
> > {
> > MachineState *machine = MACHINE(qdev_get_machine());
> > machine_apply_compat_props(machine, obj);
> > }
> >
> > qdev.c:
> >
> > static void device_post_init(Object *obj)
> > {
> > object_apply_compat_props(obj);
> > apply_user_provided_globals(obj);
> > }
> >
> > object_interface.c:
> >
> > void user_creatable_complete(Object *obj, Error **errp)
> > {
> > object_apply_compat_props(obj);
> > ...
> > ucc->complete(...)
> > }
> >
>
> I like that solution too (which you also proposed in the other
> thread). But we have to decide whether it's acceptable to reference
> MachineState, or if the compat properties should be registered.
I dislike pulling in machine into basic object code and
I think a separate compats would be cleaner interface without
layer violation. But to unstuck, lets go with qdev_get_machine()
for now.
> --
> Marc-André Lureau
>
On Fri, Nov 30, 2018 at 11:55:26AM +0100, Igor Mammedov wrote:
> On Fri, 30 Nov 2018 01:36:03 +0400
> Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
>
> > Hi
> >
> > On Thu, Nov 29, 2018 at 9:51 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >
> > > On Thu, Nov 29, 2018 at 02:32:18PM +0400, Marc-André Lureau wrote:
> > > > Hi
> > > > On Wed, Nov 28, 2018 at 9:53 PM Igor Mammedov <imammedo@redhat.com> wrote:
> > > > >
> > > > > On Tue, 27 Nov 2018 11:35:27 -0200
> > > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > >
> > > > > > On Tue, Nov 27, 2018 at 05:10:05PM +0400, Marc-André Lureau wrote:
> > > > > > > On Tue, Nov 27, 2018 at 4:57 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > > > > >
> > > > > > > > On Tue, Nov 27, 2018 at 01:27:49PM +0400, Marc-André Lureau wrote:
> > > > > > > > > Similarly to accel properties, move compat properties out of globals
> > > > > > > > > registration, and apply the machine compat properties during
> > > > > > > > > device_post_init().
> > > > > > > > >
> > > > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > > > > [...]
> > > > > > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > > > > > > > > index 7066d28271..3b31b2c025 100644
> > > > > > > > > --- a/hw/core/qdev.c
> > > > > > > > > +++ b/hw/core/qdev.c
> > > > > > > > > @@ -971,17 +971,26 @@ static void device_initfn(Object *obj)
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > static const GPtrArray *ac_compat_props;
> > > > > > > > > +static const GPtrArray *mc_compat_props;
> > > > > why you didn't use just 'compat_props' for both?
> > > > > (it would be cleaner have single registry for compat
> > > > > properties, and the place that takes care of registration
> > > > > will take care of necessary ordering)
> > > >
> > > > There are two arrays, one from the accelerator class, the other from
> > > > the machine class. We can't make it a singleton (all compats props for
> > > > the various machines would be mixed).
> > > >
> > > > We could create a third array that would be the set of both, but that
> > > > would require more copy/allocation.
> > >
> > > I am failing to see the advantage of replacing the `global_props`
> > > static variable from qdev-properties.c with a collection of
> > > separate static variables scattered around the code. I thought
> > > the main point of the changes was to reduce the amount of
> > > duplicate data stored in static variables.
> Main point was to use compats for backends then on top of that
> we added split global from compat properties goal.
>
> > > I was expecting something like this:
> > >
> > > accel.c:
> > >
> > > void accel_apply_compat_props(AccelState *accel, Object *obj)
> > > {
> > > object_apply_global_props(obj, ACCEL_GET_CLASS(accel)->compat_props, &error_abort);
> > > }
> > >
> > > machine.c:
> > >
> > > /* Apply compat properties and global properties to an object */
> > > void machine_apply_compat_props(MachineState *ms, Object *obj)
> > > {
> > > accel_apply_compat_props(ms->accel, obj);
> > > object_apply_global_props(obj, MACHINE_GET_CLASS(ms)->compat_props, &error_abort);
> > > }
> > >
> > > compat-props.c:
> > >
> > > static void object_apply_compat_props(Object *obj)
> > > {
> > > MachineState *machine = MACHINE(qdev_get_machine());
> > > machine_apply_compat_props(machine, obj);
> > > }
> > >
> > > qdev.c:
> > >
> > > static void device_post_init(Object *obj)
> > > {
> > > object_apply_compat_props(obj);
> > > apply_user_provided_globals(obj);
> > > }
> > >
> > > object_interface.c:
> > >
> > > void user_creatable_complete(Object *obj, Error **errp)
> > > {
> > > object_apply_compat_props(obj);
> > > ...
> > > ucc->complete(...)
> > > }
> > >
> >
> > I like that solution too (which you also proposed in the other
> > thread). But we have to decide whether it's acceptable to reference
> > MachineState, or if the compat properties should be registered.
> I dislike pulling in machine into basic object code and
> I think a separate compats would be cleaner interface without
> layer violation. But to unstuck, lets go with qdev_get_machine()
> for now.
Which basic object code? The only reference to machine above is
at compat-props.c. I don't see any layering violation here.
--
Eduardo
On Fri, 30 Nov 2018 09:41:42 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Fri, Nov 30, 2018 at 11:55:26AM +0100, Igor Mammedov wrote:
> > On Fri, 30 Nov 2018 01:36:03 +0400
> > Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> >
> > > Hi
> > >
> > > On Thu, Nov 29, 2018 at 9:51 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > >
> > > > On Thu, Nov 29, 2018 at 02:32:18PM +0400, Marc-André Lureau wrote:
> > > > > Hi
> > > > > On Wed, Nov 28, 2018 at 9:53 PM Igor Mammedov <imammedo@redhat.com> wrote:
> > > > > >
> > > > > > On Tue, 27 Nov 2018 11:35:27 -0200
> > > > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > > >
> > > > > > > On Tue, Nov 27, 2018 at 05:10:05PM +0400, Marc-André Lureau wrote:
> > > > > > > > On Tue, Nov 27, 2018 at 4:57 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > > > > > >
> > > > > > > > > On Tue, Nov 27, 2018 at 01:27:49PM +0400, Marc-André Lureau wrote:
> > > > > > > > > > Similarly to accel properties, move compat properties out of globals
> > > > > > > > > > registration, and apply the machine compat properties during
> > > > > > > > > > device_post_init().
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > > > > > [...]
> > > > > > > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > > > > > > > > > index 7066d28271..3b31b2c025 100644
> > > > > > > > > > --- a/hw/core/qdev.c
> > > > > > > > > > +++ b/hw/core/qdev.c
> > > > > > > > > > @@ -971,17 +971,26 @@ static void device_initfn(Object *obj)
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > static const GPtrArray *ac_compat_props;
> > > > > > > > > > +static const GPtrArray *mc_compat_props;
> > > > > > why you didn't use just 'compat_props' for both?
> > > > > > (it would be cleaner have single registry for compat
> > > > > > properties, and the place that takes care of registration
> > > > > > will take care of necessary ordering)
> > > > >
> > > > > There are two arrays, one from the accelerator class, the other from
> > > > > the machine class. We can't make it a singleton (all compats props for
> > > > > the various machines would be mixed).
> > > > >
> > > > > We could create a third array that would be the set of both, but that
> > > > > would require more copy/allocation.
> > > >
> > > > I am failing to see the advantage of replacing the `global_props`
> > > > static variable from qdev-properties.c with a collection of
> > > > separate static variables scattered around the code. I thought
> > > > the main point of the changes was to reduce the amount of
> > > > duplicate data stored in static variables.
> > Main point was to use compats for backends then on top of that
> > we added split global from compat properties goal.
> >
> > > > I was expecting something like this:
> > > >
> > > > accel.c:
> > > >
> > > > void accel_apply_compat_props(AccelState *accel, Object *obj)
> > > > {
> > > > object_apply_global_props(obj, ACCEL_GET_CLASS(accel)->compat_props, &error_abort);
> > > > }
> > > >
> > > > machine.c:
> > > >
> > > > /* Apply compat properties and global properties to an object */
> > > > void machine_apply_compat_props(MachineState *ms, Object *obj)
> > > > {
> > > > accel_apply_compat_props(ms->accel, obj);
> > > > object_apply_global_props(obj, MACHINE_GET_CLASS(ms)->compat_props, &error_abort);
> > > > }
> > > >
> > > > compat-props.c:
> > > >
> > > > static void object_apply_compat_props(Object *obj)
> > > > {
> > > > MachineState *machine = MACHINE(qdev_get_machine());
> > > > machine_apply_compat_props(machine, obj);
> > > > }
> > > >
> > > > qdev.c:
> > > >
> > > > static void device_post_init(Object *obj)
> > > > {
> > > > object_apply_compat_props(obj);
> > > > apply_user_provided_globals(obj);
> > > > }
> > > >
> > > > object_interface.c:
> > > >
> > > > void user_creatable_complete(Object *obj, Error **errp)
> > > > {
> > > > object_apply_compat_props(obj);
> > > > ...
> > > > ucc->complete(...)
> > > > }
> > > >
> > >
> > > I like that solution too (which you also proposed in the other
> > > thread). But we have to decide whether it's acceptable to reference
> > > MachineState, or if the compat properties should be registered.
> > I dislike pulling in machine into basic object code and
> > I think a separate compats would be cleaner interface without
> > layer violation. But to unstuck, lets go with qdev_get_machine()
> > for now.
>
> Which basic object code? The only reference to machine above is
> at compat-props.c. I don't see any layering violation here.
I see compat properties as just a set of arbitrary properties
that's applied to objects depending on configuration.
Which currently includes machine version and accel type
but in no way inherently tied to them.
As for layering violation, we do it in device model cases
as it's machine related (ugly 'acceptable' but helps to cut corners)
and with new interface it will be used wit backends which are not
machine related at all.
Doing like you suggest would make it usable with current machine
initialization order (with some fixes) but if one would consider
a bit earlier time (preconfig time) and creating machine step by
step from there it might be less usable (as it needs machine instance,
it still would be possible but probably more tricky (pure speculation)).
So I'd vote for abstracted from machine compat props that could be
called from any object that implements interface and explicit
compats registration (like we do now) and explicit registration failure
if compats happend to be applied to any object. But in case of this
series I'm fine with any approach just to get compat properties work
with backends for now.
On Tue, Dec 04, 2018 at 02:17:24PM +0100, Igor Mammedov wrote:
> On Fri, 30 Nov 2018 09:41:42 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> > On Fri, Nov 30, 2018 at 11:55:26AM +0100, Igor Mammedov wrote:
> > > On Fri, 30 Nov 2018 01:36:03 +0400
> > > Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> > >
> > > > Hi
> > > >
> > > > On Thu, Nov 29, 2018 at 9:51 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > >
> > > > > On Thu, Nov 29, 2018 at 02:32:18PM +0400, Marc-André Lureau wrote:
> > > > > > Hi
> > > > > > On Wed, Nov 28, 2018 at 9:53 PM Igor Mammedov <imammedo@redhat.com> wrote:
> > > > > > >
> > > > > > > On Tue, 27 Nov 2018 11:35:27 -0200
> > > > > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > > > >
> > > > > > > > On Tue, Nov 27, 2018 at 05:10:05PM +0400, Marc-André Lureau wrote:
> > > > > > > > > On Tue, Nov 27, 2018 at 4:57 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > > > > > > >
> > > > > > > > > > On Tue, Nov 27, 2018 at 01:27:49PM +0400, Marc-André Lureau wrote:
> > > > > > > > > > > Similarly to accel properties, move compat properties out of globals
> > > > > > > > > > > registration, and apply the machine compat properties during
> > > > > > > > > > > device_post_init().
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > > > > > > [...]
> > > > > > > > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > > > > > > > > > > index 7066d28271..3b31b2c025 100644
> > > > > > > > > > > --- a/hw/core/qdev.c
> > > > > > > > > > > +++ b/hw/core/qdev.c
> > > > > > > > > > > @@ -971,17 +971,26 @@ static void device_initfn(Object *obj)
> > > > > > > > > > > }
> > > > > > > > > > >
> > > > > > > > > > > static const GPtrArray *ac_compat_props;
> > > > > > > > > > > +static const GPtrArray *mc_compat_props;
> > > > > > > why you didn't use just 'compat_props' for both?
> > > > > > > (it would be cleaner have single registry for compat
> > > > > > > properties, and the place that takes care of registration
> > > > > > > will take care of necessary ordering)
> > > > > >
> > > > > > There are two arrays, one from the accelerator class, the other from
> > > > > > the machine class. We can't make it a singleton (all compats props for
> > > > > > the various machines would be mixed).
> > > > > >
> > > > > > We could create a third array that would be the set of both, but that
> > > > > > would require more copy/allocation.
> > > > >
> > > > > I am failing to see the advantage of replacing the `global_props`
> > > > > static variable from qdev-properties.c with a collection of
> > > > > separate static variables scattered around the code. I thought
> > > > > the main point of the changes was to reduce the amount of
> > > > > duplicate data stored in static variables.
> > > Main point was to use compats for backends then on top of that
> > > we added split global from compat properties goal.
> > >
> > > > > I was expecting something like this:
> > > > >
> > > > > accel.c:
> > > > >
> > > > > void accel_apply_compat_props(AccelState *accel, Object *obj)
> > > > > {
> > > > > object_apply_global_props(obj, ACCEL_GET_CLASS(accel)->compat_props, &error_abort);
> > > > > }
> > > > >
> > > > > machine.c:
> > > > >
> > > > > /* Apply compat properties and global properties to an object */
> > > > > void machine_apply_compat_props(MachineState *ms, Object *obj)
> > > > > {
> > > > > accel_apply_compat_props(ms->accel, obj);
> > > > > object_apply_global_props(obj, MACHINE_GET_CLASS(ms)->compat_props, &error_abort);
> > > > > }
> > > > >
> > > > > compat-props.c:
> > > > >
> > > > > static void object_apply_compat_props(Object *obj)
> > > > > {
> > > > > MachineState *machine = MACHINE(qdev_get_machine());
> > > > > machine_apply_compat_props(machine, obj);
> > > > > }
> > > > >
> > > > > qdev.c:
> > > > >
> > > > > static void device_post_init(Object *obj)
> > > > > {
> > > > > object_apply_compat_props(obj);
> > > > > apply_user_provided_globals(obj);
> > > > > }
> > > > >
> > > > > object_interface.c:
> > > > >
> > > > > void user_creatable_complete(Object *obj, Error **errp)
> > > > > {
> > > > > object_apply_compat_props(obj);
> > > > > ...
> > > > > ucc->complete(...)
> > > > > }
> > > > >
> > > >
> > > > I like that solution too (which you also proposed in the other
> > > > thread). But we have to decide whether it's acceptable to reference
> > > > MachineState, or if the compat properties should be registered.
> > > I dislike pulling in machine into basic object code and
> > > I think a separate compats would be cleaner interface without
> > > layer violation. But to unstuck, lets go with qdev_get_machine()
> > > for now.
> >
> > Which basic object code? The only reference to machine above is
> > at compat-props.c. I don't see any layering violation here.
> I see compat properties as just a set of arbitrary properties
> that's applied to objects depending on configuration.
> Which currently includes machine version and accel type
> but in no way inherently tied to them.
I understand you want to have a more generic abstraction layer,
but my question is: why do we need it? I still don't see which
problems it would solve.
>
> As for layering violation, we do it in device model cases
> as it's machine related (ugly 'acceptable' but helps to cut corners)
> and with new interface it will be used wit backends which are not
> machine related at all.
I don't see why it's ugly to have QEMU backend object code use
machine core code. What problems does it cause?
>
> Doing like you suggest would make it usable with current machine
> initialization order (with some fixes) but if one would consider
> a bit earlier time (preconfig time) and creating machine step by
> step from there it might be less usable (as it needs machine instance,
> it still would be possible but probably more tricky (pure speculation)).
I agree that this is a problem: we really have a
backend -> compat_props -> current_machine dependency chain here.
Using a registration mechanism that copies data, the dependency
would still exists, but be implicit in the initialization
ordering. Then the system would silently break as soon as we
reorder the code.
>
> So I'd vote for abstracted from machine compat props that could be
> called from any object that implements interface and explicit
> compats registration (like we do now) and explicit registration failure
> if compats happend to be applied to any object. But in case of this
> series I'm fine with any approach just to get compat properties work
> with backends for now.
Thanks. I agree we'll want to refactor this later. We may want
to be able to apply compat properties to backend objects before
creating machine and accel instances.
Or maybe we will want to apply only MachineClass::compat_props to
backend objects, and not AccelClass::compat_props? This is not
clear to me yet.
--
Eduardo
On Thu, 29 Nov 2018 14:32:18 +0400 Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > Hi > On Wed, Nov 28, 2018 at 9:53 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > On Tue, 27 Nov 2018 11:35:27 -0200 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Tue, Nov 27, 2018 at 05:10:05PM +0400, Marc-André Lureau wrote: > > > > On Tue, Nov 27, 2018 at 4:57 PM Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > > > > On Tue, Nov 27, 2018 at 01:27:49PM +0400, Marc-André Lureau wrote: > > > > > > Similarly to accel properties, move compat properties out of globals > > > > > > registration, and apply the machine compat properties during > > > > > > device_post_init(). > > > > > > > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > > [...] > > > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > > > > > > index 7066d28271..3b31b2c025 100644 > > > > > > --- a/hw/core/qdev.c > > > > > > +++ b/hw/core/qdev.c > > > > > > @@ -971,17 +971,26 @@ static void device_initfn(Object *obj) > > > > > > } > > > > > > > > > > > > static const GPtrArray *ac_compat_props; > > > > > > +static const GPtrArray *mc_compat_props; > > why you didn't use just 'compat_props' for both? > > (it would be cleaner have single registry for compat > > properties, and the place that takes care of registration > > will take care of necessary ordering) > > There are two arrays, one from the accelerator class, the other from > the machine class. We can't make it a singleton (all compats props for > the various machines would be mixed). My impression was that we register properties explicitly here, so only registered ones endup here and in the order they were added. Hence machine and accel compats could be merged here. > We could create a third array that would be the set of both, but that > would require more copy/allocation. [...]
On Tue, Nov 27, 2018 at 01:27:49PM +0400, Marc-André Lureau wrote:
> Similarly to accel properties, move compat properties out of globals
> registration, and apply the machine compat properties during
> device_post_init().
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
[...]
> @@ -191,7 +190,7 @@ struct MachineClass {
> const char *default_machine_opts;
> const char *default_boot_order;
> const char *default_display;
> - GArray *compat_props;
> + GPtrArray *compat_props;
What are the advantages/disadvantages of GArray vs GPtrArray vs
GList here? Why did you decide to change this from GArray to
GPtrArray in v4?
--
Eduardo
Hi
On Thu, Nov 29, 2018 at 8:11 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> On Tue, Nov 27, 2018 at 01:27:49PM +0400, Marc-André Lureau wrote:
> > Similarly to accel properties, move compat properties out of globals
> > registration, and apply the machine compat properties during
> > device_post_init().
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> [...]
> > @@ -191,7 +190,7 @@ struct MachineClass {
> > const char *default_machine_opts;
> > const char *default_boot_order;
> > const char *default_display;
> > - GArray *compat_props;
> > + GPtrArray *compat_props;
>
> What are the advantages/disadvantages of GArray vs GPtrArray vs
> GList here? Why did you decide to change this from GArray to
> GPtrArray in v4?
GList would be less efficient than an array (extra pointer, memory
allocations/fragmentation/cache etc).
GPtrArray is similar to GArray, it is just specialized for storing void*.
--
Marc-André Lureau
© 2016 - 2026 Red Hat, Inc.