* [Qemu-devel] [RFC 01/19] qdev: Replace cannot_instantiate_with_device_add_yet with !user_creatable
2017-04-01 0:46 [Qemu-devel] [RFC 00/19] sysbus: Don't allow -device/device_add by default Eduardo Habkost
@ 2017-04-01 0:46 ` Eduardo Habkost
2017-04-03 17:16 ` Alistair Francis
2017-04-01 0:46 ` [Qemu-devel] [RFC 02/19] s390: Add FIXME for unexplained user_creatable=false line Eduardo Habkost
` (17 subsequent siblings)
18 siblings, 1 reply; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-01 0:46 UTC (permalink / raw)
To: qemu-devel
Cc: Laszlo Ersek, Alexander Graf, Marcel Apfelbaum, Thomas Huth,
Peter Maydell, Markus Armbruster
cannot_instantiate_with_device_add_yet was introduced by commit
837d37167dc446af8a91189108b363c04609e296 to replace no_user. It
was supposed to be a temporary measure.
When it was introduced, we had 54
cannot_instantiate_with_device_add_yet=true lines in the code.
Today (3 years later) this number has not shrinked: we now have
57 cannot_instantiate_with_device_add_yet=true lines. I think it
is safe to say it is not a temporary measure, and we won't see
the flag go away soon.
Instead of a long field name that misleads people to believe it
is temporary, replace it a shorter and less misleading field:
user_creatable.
Except for code comments, changes were generated using the
following Coccinelle patch:
@@
expression DC;
@@
(
-DC->cannot_instantiate_with_device_add_yet = false;
+DC->user_creatable = true;
|
-DC->cannot_instantiate_with_device_add_yet = true;
+DC->user_creatable = false;
)
@@
typedef ObjectClass;
expression dc;
identifier class, data;
@@
static void device_class_init(ObjectClass *class, void *data)
{
...
dc->hotpluggable = true;
+dc->user_creatable = true;
...
}
@@
@@
struct DeviceClass {
...
-bool cannot_instantiate_with_device_add_yet;
+bool user_creatable;
...
}
@@
expression DC;
@@
(
-!DC->cannot_instantiate_with_device_add_yet
+DC->user_creatable
|
-DC->cannot_instantiate_with_device_add_yet
+!DC->user_creatable
)
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Thomas Huth <thuth@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
include/hw/qdev-core.h | 10 +++++-----
include/hw/qdev-properties.h | 4 ++--
hw/acpi/piix4.c | 2 +-
hw/arm/spitz.c | 2 +-
hw/audio/marvell_88w8618.c | 2 +-
hw/audio/pcspk.c | 2 +-
hw/core/or-irq.c | 2 +-
hw/core/qdev.c | 1 +
hw/core/register.c | 2 +-
hw/dma/i8257.c | 2 +-
hw/dma/sparc32_dma.c | 2 +-
hw/gpio/omap_gpio.c | 4 ++--
hw/i2c/omap_i2c.c | 2 +-
hw/i2c/smbus_eeprom.c | 2 +-
hw/i2c/smbus_ich9.c | 2 +-
hw/i386/pc.c | 2 +-
hw/input/vmmouse.c | 2 +-
hw/intc/apic_common.c | 2 +-
hw/intc/etraxfs_pic.c | 2 +-
hw/intc/grlib_irqmp.c | 2 +-
hw/intc/i8259_common.c | 2 +-
hw/intc/nios2_iic.c | 2 +-
hw/intc/omap_intc.c | 4 ++--
hw/isa/lpc_ich9.c | 2 +-
hw/isa/piix4.c | 2 +-
hw/isa/vt82c686.c | 2 +-
hw/mips/gt64xxx_pci.c | 2 +-
hw/misc/vmport.c | 2 +-
hw/net/dp8393x.c | 2 +-
hw/net/etraxfs_eth.c | 2 +-
hw/net/lance.c | 2 +-
hw/pci-bridge/dec.c | 2 +-
hw/pci-bridge/pci_expander_bridge.c | 2 +-
hw/pci-host/apb.c | 2 +-
hw/pci-host/bonito.c | 2 +-
hw/pci-host/gpex.c | 2 +-
hw/pci-host/grackle.c | 2 +-
hw/pci-host/piix.c | 6 +++---
hw/pci-host/ppce500.c | 2 +-
hw/pci-host/prep.c | 2 +-
hw/pci-host/q35.c | 4 ++--
hw/pci-host/uninorth.c | 8 ++++----
hw/pci-host/versatile.c | 2 +-
hw/pci-host/xilinx-pcie.c | 2 +-
hw/ppc/ppc4xx_pci.c | 2 +-
hw/ppc/spapr_drc.c | 2 +-
hw/s390x/s390-pci-bus.c | 2 +-
hw/sd/milkymist-memcard.c | 2 +-
hw/sd/pl181.c | 2 +-
hw/sh4/sh_pci.c | 2 +-
hw/timer/i8254_common.c | 2 +-
hw/timer/mc146818rtc.c | 2 +-
monitor.c | 2 +-
qdev-monitor.c | 6 +++---
qom/cpu.c | 2 +-
target/i386/cpu.c | 2 +-
56 files changed, 71 insertions(+), 70 deletions(-)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index b44b476765..bc4e6f0486 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -103,16 +103,16 @@ typedef struct DeviceClass {
Property *props;
/*
- * Shall we hide this device model from -device / device_add?
+ * Can this device be instantiated with -device / device_add?
* All devices should support instantiation with device_add, and
* this flag should not exist. But we're not there, yet. Some
* devices fail to instantiate with cryptic error messages.
* Others instantiate, but don't work. Exposing users to such
- * behavior would be cruel; this flag serves to protect them. It
- * should never be set without a comment explaining why it is set.
- * TODO remove once we're there
+ * behavior would be cruel; clearing this flag will protect them.
+ * It should never be cleared without a comment explaining why it
+ * is cleared.
*/
- bool cannot_instantiate_with_device_add_yet;
+ bool user_creatable;
/*
* Does this device model survive object_unref(object_new(TNAME))?
* All device models should, and this flag shouldn't exist. Some
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 7ac315331a..5c6e13b993 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -134,12 +134,12 @@ extern PropertyInfo qdev_prop_arraylen;
* device_add, so add code like this:
* |* Reason: pointer property "NAME-OF-YOUR-PROP" *|
* DeviceClass *dc = DEVICE_CLASS(class);
- * dc->cannot_instantiate_with_device_add_yet = true;
+ * dc->user_creatable = false;
*
* - If the property may safely remain null, document it like this:
* |*
* * Note: pointer property "interrupt_vector" may remain null, thus
- * * no need for dc->cannot_instantiate_with_device_add_yet = true;
+ * * no need for dc->user_creatable = false;
* *|
*/
#define DEFINE_PROP_PTR(_n, _s, _f) \
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index a553a7e110..f4fd5907b8 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -700,7 +700,7 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
* Reason: part of PIIX4 southbridge, needs to be wired up,
* e.g. by mips_malta_init()
*/
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
dc->hotpluggable = false;
hc->plug = piix4_device_plug_cb;
hc->unplug_request = piix4_device_unplug_request_cb;
diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
index fe2d5a764c..324626847c 100644
--- a/hw/arm/spitz.c
+++ b/hw/arm/spitz.c
@@ -1076,7 +1076,7 @@ static void sl_nand_class_init(ObjectClass *klass, void *data)
dc->vmsd = &vmstate_sl_nand_info;
dc->props = sl_nand_properties;
/* Reason: init() method uses drive_get() */
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo sl_nand_info = {
diff --git a/hw/audio/marvell_88w8618.c b/hw/audio/marvell_88w8618.c
index 511b004287..4f65f8c199 100644
--- a/hw/audio/marvell_88w8618.c
+++ b/hw/audio/marvell_88w8618.c
@@ -292,7 +292,7 @@ static void mv88w8618_audio_class_init(ObjectClass *klass, void *data)
dc->vmsd = &mv88w8618_audio_vmsd;
dc->props = mv88w8618_audio_properties;
/* Reason: pointer property "wm8750" */
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo mv88w8618_audio_info = {
diff --git a/hw/audio/pcspk.c b/hw/audio/pcspk.c
index 798002277b..9b99358d87 100644
--- a/hw/audio/pcspk.c
+++ b/hw/audio/pcspk.c
@@ -223,7 +223,7 @@ static void pcspk_class_initfn(ObjectClass *klass, void *data)
dc->vmsd = &vmstate_spk;
dc->props = pcspk_properties;
/* Reason: realize sets global pcspk_state */
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo pcspk_info = {
diff --git a/hw/core/or-irq.c b/hw/core/or-irq.c
index 1485d5b285..f9d76c4641 100644
--- a/hw/core/or-irq.c
+++ b/hw/core/or-irq.c
@@ -91,7 +91,7 @@ static void or_irq_class_init(ObjectClass *klass, void *data)
dc->vmsd = &vmstate_or_irq;
/* Reason: Needs to be wired up to work, e.g. see stm32f205_soc.c */
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo or_irq_type_info = {
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 1e7fb33246..4132a8bee3 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -1159,6 +1159,7 @@ static void device_class_init(ObjectClass *class, void *data)
* should override it in their class_init()
*/
dc->hotpluggable = true;
+ dc->user_creatable = true;
}
void device_reset(DeviceState *dev)
diff --git a/hw/core/register.c b/hw/core/register.c
index dc335a79a9..da38ef3a54 100644
--- a/hw/core/register.c
+++ b/hw/core/register.c
@@ -288,7 +288,7 @@ static void register_class_init(ObjectClass *oc, void *data)
DeviceClass *dc = DEVICE_CLASS(oc);
/* Reason: needs to be wired up to work */
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo register_info = {
diff --git a/hw/dma/i8257.c b/hw/dma/i8257.c
index 8bd82e8bc8..bd23e893bf 100644
--- a/hw/dma/i8257.c
+++ b/hw/dma/i8257.c
@@ -601,7 +601,7 @@ static void i8257_class_init(ObjectClass *klass, void *data)
idc->schedule = i8257_dma_schedule;
idc->register_channel = i8257_dma_register_channel;
/* Reason: needs to be wired up by isa_bus_dma() to work */
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo i8257_info = {
diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c
index 9d545e412e..9c6bdc6295 100644
--- a/hw/dma/sparc32_dma.c
+++ b/hw/dma/sparc32_dma.c
@@ -305,7 +305,7 @@ static void sparc32_dma_class_init(ObjectClass *klass, void *data)
dc->vmsd = &vmstate_dma;
dc->props = sparc32_dma_properties;
/* Reason: pointer property "iommu_opaque" */
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo sparc32_dma_info = {
diff --git a/hw/gpio/omap_gpio.c b/hw/gpio/omap_gpio.c
index dabef4a119..1df394eb12 100644
--- a/hw/gpio/omap_gpio.c
+++ b/hw/gpio/omap_gpio.c
@@ -773,7 +773,7 @@ static void omap_gpio_class_init(ObjectClass *klass, void *data)
dc->reset = omap_gpif_reset;
dc->props = omap_gpio_properties;
/* Reason: pointer property "clk" */
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo omap_gpio_info = {
@@ -804,7 +804,7 @@ static void omap2_gpio_class_init(ObjectClass *klass, void *data)
dc->reset = omap2_gpif_reset;
dc->props = omap2_gpio_properties;
/* Reason: pointer properties "iclk", "fclk0", ..., "fclk5" */
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo omap2_gpio_info = {
diff --git a/hw/i2c/omap_i2c.c b/hw/i2c/omap_i2c.c
index f7c92ea00c..f6e80bee25 100644
--- a/hw/i2c/omap_i2c.c
+++ b/hw/i2c/omap_i2c.c
@@ -491,7 +491,7 @@ static void omap_i2c_class_init(ObjectClass *klass, void *data)
dc->props = omap_i2c_properties;
dc->reset = omap_i2c_reset;
/* Reason: pointer properties "iclk", "fclk" */
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
dc->realize = omap_i2c_realize;
}
diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index 5b7bd891bc..b13ec0fe7a 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -123,7 +123,7 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
sc->read_data = eeprom_read_data;
dc->props = smbus_eeprom_properties;
/* Reason: pointer property "data" */
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo smbus_eeprom_info = {
diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
index 48fab22625..ea51e09186 100644
--- a/hw/i2c/smbus_ich9.c
+++ b/hw/i2c/smbus_ich9.c
@@ -103,7 +103,7 @@ static void ich9_smb_class_init(ObjectClass *klass, void *data)
* Reason: part of ICH9 southbridge, needs to be wired up by
* pc_q35_init()
*/
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
I2CBus *ich9_smb_init(PCIBus *bus, int devfn, uint32_t smb_io_base)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index d24388e05f..610050eb4f 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -597,7 +597,7 @@ static void port92_class_initfn(ObjectClass *klass, void *data)
* wiring: its A20 output line needs to be wired up by
* port92_init().
*/
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo port92_info = {
diff --git a/hw/input/vmmouse.c b/hw/input/vmmouse.c
index 6d15a887c6..4747da9a8d 100644
--- a/hw/input/vmmouse.c
+++ b/hw/input/vmmouse.c
@@ -286,7 +286,7 @@ static void vmmouse_class_initfn(ObjectClass *klass, void *data)
dc->vmsd = &vmstate_vmmouse;
dc->props = vmmouse_properties;
/* Reason: pointer property "ps2_mouse" */
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo vmmouse_info = {
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index c3829e31b5..1ef56f8d10 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -501,7 +501,7 @@ static void apic_common_class_init(ObjectClass *klass, void *data)
* Reason: APIC and CPU need to be wired up by
* x86_cpu_apic_create()
*/
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo apic_common_type = {
diff --git a/hw/intc/etraxfs_pic.c b/hw/intc/etraxfs_pic.c
index 64a6f4b4ba..1bfde2f09e 100644
--- a/hw/intc/etraxfs_pic.c
+++ b/hw/intc/etraxfs_pic.c
@@ -173,7 +173,7 @@ static void etraxfs_pic_class_init(ObjectClass *klass, void *data)
dc->props = etraxfs_pic_properties;
/*
* Note: pointer property "interrupt_vector" may remain null, thus
- * no need for dc->cannot_instantiate_with_device_add_yet = true;
+ * no need for dc->user_creatable = false;
*/
}
diff --git a/hw/intc/grlib_irqmp.c b/hw/intc/grlib_irqmp.c
index ac7e63f38b..94659ee256 100644
--- a/hw/intc/grlib_irqmp.c
+++ b/hw/intc/grlib_irqmp.c
@@ -360,7 +360,7 @@ static void grlib_irqmp_class_init(ObjectClass *klass, void *data)
dc->reset = grlib_irqmp_reset;
dc->props = grlib_irqmp_properties;
/* Reason: pointer properties "set_pil_in", "set_pil_in_opaque" */
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
dc->realize = grlib_irqmp_realize;
}
diff --git a/hw/intc/i8259_common.c b/hw/intc/i8259_common.c
index d9a5e8b217..c2fd563b5b 100644
--- a/hw/intc/i8259_common.c
+++ b/hw/intc/i8259_common.c
@@ -144,7 +144,7 @@ static void pic_common_class_init(ObjectClass *klass, void *data)
* wiring of the slave to the master is hard-coded in device model
* code.
*/
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo pic_common_type = {
diff --git a/hw/intc/nios2_iic.c b/hw/intc/nios2_iic.c
index 190b6fdbf3..016426f964 100644
--- a/hw/intc/nios2_iic.c
+++ b/hw/intc/nios2_iic.c
@@ -80,7 +80,7 @@ static void altera_iic_class_init(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
/* Reason: needs to be wired up, e.g. by nios2_10m50_ghrd_init() */
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
dc->realize = altera_iic_realize;
}
diff --git a/hw/intc/omap_intc.c b/hw/intc/omap_intc.c
index 877be67971..ccdda89dab 100644
--- a/hw/intc/omap_intc.c
+++ b/hw/intc/omap_intc.c
@@ -401,7 +401,7 @@ static void omap_intc_class_init(ObjectClass *klass, void *data)
dc->reset = omap_inth_reset;
dc->props = omap_intc_properties;
/* Reason: pointer property "clk" */
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
dc->realize = omap_intc_realize;
}
@@ -656,7 +656,7 @@ static void omap2_intc_class_init(ObjectClass *klass, void *data)
dc->reset = omap_inth_reset;
dc->props = omap2_intc_properties;
/* Reason: pointer property "iclk", "fclk" */
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
dc->realize = omap2_intc_realize;
}
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 59930dd9d0..cdc854a822 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -810,7 +810,7 @@ static void ich9_lpc_class_init(ObjectClass *klass, void *data)
* Reason: part of ICH9 southbridge, needs to be wired up by
* pc_q35_init()
*/
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
hc->plug = ich9_pm_device_plug_cb;
hc->unplug_request = ich9_pm_device_unplug_request_cb;
hc->unplug = ich9_pm_device_unplug_cb;
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 5500fcc4d6..f811eba59d 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -123,7 +123,7 @@ static void piix4_class_init(ObjectClass *klass, void *data)
* Reason: part of PIIX4 southbridge, needs to be wired up,
* e.g. by mips_malta_init()
*/
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
dc->hotpluggable = false;
}
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 41d5254f8e..50dc83df77 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -494,7 +494,7 @@ static void via_class_init(ObjectClass *klass, void *data)
* Reason: part of VIA VT82C686 southbridge, needs to be wired up,
* e.g. by mips_fulong2e_init()
*/
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo via_info = {
diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
index 4811843ab6..e8b2eef688 100644
--- a/hw/mips/gt64xxx_pci.c
+++ b/hw/mips/gt64xxx_pci.c
@@ -1224,7 +1224,7 @@ static void gt64120_pci_class_init(ObjectClass *klass, void *data)
* PCI-facing part of the host bridge, not usable without the
* host-facing part, which can't be device_add'ed, yet.
*/
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo gt64120_pci_info = {
diff --git a/hw/misc/vmport.c b/hw/misc/vmport.c
index be40930b8b..165500223f 100644
--- a/hw/misc/vmport.c
+++ b/hw/misc/vmport.c
@@ -163,7 +163,7 @@ static void vmport_class_initfn(ObjectClass *klass, void *data)
dc->realize = vmport_realizefn;
/* Reason: realize sets global port_state */
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo vmport_info = {
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index efa33ad40a..b53fcaa8bc 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -934,7 +934,7 @@ static void dp8393x_class_init(ObjectClass *klass, void *data)
dc->vmsd = &vmstate_dp8393x;
dc->props = dp8393x_properties;
/* Reason: dma_mr property can't be set */
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo dp8393x_info = {
diff --git a/hw/net/etraxfs_eth.c b/hw/net/etraxfs_eth.c
index efaa49faae..013c8d0a41 100644
--- a/hw/net/etraxfs_eth.c
+++ b/hw/net/etraxfs_eth.c
@@ -630,7 +630,7 @@ static void etraxfs_eth_class_init(ObjectClass *klass, void *data)
k->init = fs_eth_init;
dc->props = etraxfs_eth_properties;
/* Reason: pointer properties "dma_out", "dma_in" */
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo etraxfs_eth_info = {
diff --git a/hw/net/lance.c b/hw/net/lance.c
index 573d724bcf..92b0c68274 100644
--- a/hw/net/lance.c
+++ b/hw/net/lance.c
@@ -165,7 +165,7 @@ static void lance_class_init(ObjectClass *klass, void *data)
dc->vmsd = &vmstate_lance;
dc->props = lance_properties;
/* Reason: pointer property "dma" */
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo lance_info = {
diff --git a/hw/pci-bridge/dec.c b/hw/pci-bridge/dec.c
index 840c96198a..cca93620ac 100644
--- a/hw/pci-bridge/dec.c
+++ b/hw/pci-bridge/dec.c
@@ -128,7 +128,7 @@ static void dec_21154_pci_host_class_init(ObjectClass *klass, void *data)
* PCI-facing part of the host bridge, not usable without the
* host-facing part, which can't be device_add'ed, yet.
*/
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo dec_21154_pci_host_info = {
diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index 6ac187fa32..ff59abf208 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -150,7 +150,7 @@ static void pxb_host_class_init(ObjectClass *class, void *data)
dc->fw_name = "pci";
/* Reason: Internal part of the pxb/pxb-pcie device, not usable by itself */
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
sbc->explicit_ofw_unit_address = pxb_host_ofw_unit_address;
hc->root_bus_path = pxb_host_root_bus_path;
}
diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c
index 653e711121..edc88f4c65 100644
--- a/hw/pci-host/apb.c
+++ b/hw/pci-host/apb.c
@@ -810,7 +810,7 @@ static void pbm_pci_host_class_init(ObjectClass *klass, void *data)
* PCI-facing part of the host bridge, not usable without the
* host-facing part, which can't be device_add'ed, yet.
*/
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo pbm_pci_host_info = {
diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
index 1999ece590..85a3bb0dd2 100644
--- a/hw/pci-host/bonito.c
+++ b/hw/pci-host/bonito.c
@@ -825,7 +825,7 @@ static void bonito_class_init(ObjectClass *klass, void *data)
* PCI-facing part of the host bridge, not usable without the
* host-facing part, which can't be device_add'ed, yet.
*/
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo bonito_info = {
diff --git a/hw/pci-host/gpex.c b/hw/pci-host/gpex.c
index 66055ee5cc..e2629ce70d 100644
--- a/hw/pci-host/gpex.c
+++ b/hw/pci-host/gpex.c
@@ -136,7 +136,7 @@ static void gpex_root_class_init(ObjectClass *klass, void *data)
* PCI-facing part of the host bridge, not usable without the
* host-facing part, which can't be device_add'ed, yet.
*/
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo gpex_root_info = {
diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c
index 2c8acdaaca..2e281f6155 100644
--- a/hw/pci-host/grackle.c
+++ b/hw/pci-host/grackle.c
@@ -134,7 +134,7 @@ static void grackle_pci_class_init(ObjectClass *klass, void *data)
* PCI-facing part of the host bridge, not usable without the
* host-facing part, which can't be device_add'ed, yet.
*/
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo grackle_pci_info = {
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index f9218aa952..81f3a9e211 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -691,7 +691,7 @@ static void pci_piix3_class_init(ObjectClass *klass, void *data)
* Reason: part of PIIX3 southbridge, needs to be wired up by
* pc_piix.c's pc_init1()
*/
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo piix3_pci_type_info = {
@@ -745,7 +745,7 @@ static void i440fx_class_init(ObjectClass *klass, void *data)
* PCI-facing part of the host bridge, not usable without the
* host-facing part, which can't be device_add'ed, yet.
*/
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
dc->hotpluggable = false;
}
@@ -874,7 +874,7 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
dc->fw_name = "pci";
dc->props = i440fx_props;
/* Reason: needs to be wired up by pc_init1 */
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo i440fx_pcihost_info = {
diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
index e502bc0505..becc0eeb76 100644
--- a/hw/pci-host/ppce500.c
+++ b/hw/pci-host/ppce500.c
@@ -508,7 +508,7 @@ static void e500_host_bridge_class_init(ObjectClass *klass, void *data)
* PCI-facing part of the host bridge, not usable without the
* host-facing part, which can't be device_add'ed, yet.
*/
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo e500_host_bridge_info = {
diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
index 260a119a9e..900a6edfcf 100644
--- a/hw/pci-host/prep.c
+++ b/hw/pci-host/prep.c
@@ -364,7 +364,7 @@ static void raven_class_init(ObjectClass *klass, void *data)
* Reason: PCI-facing part of the host bridge, not usable without
* the host-facing part, which can't be device_add'ed, yet.
*/
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo raven_info = {
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 344f77b10c..cd5c49616e 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -156,7 +156,7 @@ static void q35_host_class_init(ObjectClass *klass, void *data)
dc->realize = q35_host_realize;
dc->props = mch_props;
/* Reason: needs to be wired up by pc_q35_init */
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
dc->fw_name = "pci";
}
@@ -549,7 +549,7 @@ static void mch_class_init(ObjectClass *klass, void *data)
* PCI-facing part of the host bridge, not usable without the
* host-facing part, which can't be device_add'ed, yet.
*/
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo mch_info = {
diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c
index df342ac3cb..6cf5e59f86 100644
--- a/hw/pci-host/uninorth.c
+++ b/hw/pci-host/uninorth.c
@@ -366,7 +366,7 @@ static void unin_main_pci_host_class_init(ObjectClass *klass, void *data)
* PCI-facing part of the host bridge, not usable without the
* host-facing part, which can't be device_add'ed, yet.
*/
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo unin_main_pci_host_info = {
@@ -390,7 +390,7 @@ static void u3_agp_pci_host_class_init(ObjectClass *klass, void *data)
* PCI-facing part of the host bridge, not usable without the
* host-facing part, which can't be device_add'ed, yet.
*/
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo u3_agp_pci_host_info = {
@@ -414,7 +414,7 @@ static void unin_agp_pci_host_class_init(ObjectClass *klass, void *data)
* PCI-facing part of the host bridge, not usable without the
* host-facing part, which can't be device_add'ed, yet.
*/
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo unin_agp_pci_host_info = {
@@ -438,7 +438,7 @@ static void unin_internal_pci_host_class_init(ObjectClass *klass, void *data)
* PCI-facing part of the host bridge, not usable without the
* host-facing part, which can't be device_add'ed, yet.
*/
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo unin_internal_pci_host_info = {
diff --git a/hw/pci-host/versatile.c b/hw/pci-host/versatile.c
index 467cbb9cb8..cc51fc87a3 100644
--- a/hw/pci-host/versatile.c
+++ b/hw/pci-host/versatile.c
@@ -479,7 +479,7 @@ static void versatile_pci_host_class_init(ObjectClass *klass, void *data)
* PCI-facing part of the host bridge, not usable without the
* host-facing part, which can't be device_add'ed, yet.
*/
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo versatile_pci_host_info = {
diff --git a/hw/pci-host/xilinx-pcie.c b/hw/pci-host/xilinx-pcie.c
index 8b71e2d950..a968cea2af 100644
--- a/hw/pci-host/xilinx-pcie.c
+++ b/hw/pci-host/xilinx-pcie.c
@@ -309,7 +309,7 @@ static void xilinx_pcie_root_class_init(ObjectClass *klass, void *data)
* PCI-facing part of the host bridge, not usable without the
* host-facing part, which can't be device_add'ed, yet.
*/
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo xilinx_pcie_root_info = {
diff --git a/hw/ppc/ppc4xx_pci.c b/hw/ppc/ppc4xx_pci.c
index dc19682970..6953f8b9ac 100644
--- a/hw/ppc/ppc4xx_pci.c
+++ b/hw/ppc/ppc4xx_pci.c
@@ -351,7 +351,7 @@ static void ppc4xx_host_bridge_class_init(ObjectClass *klass, void *data)
* PCI-facing part of the host bridge, not usable without the
* host-facing part, which can't be device_add'ed, yet.
*/
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo ppc4xx_host_bridge_info = {
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index a1cdc875b1..9fa5545991 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -675,7 +675,7 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
/*
* Reason: it crashes FIXME find and document the real reason
*/
- dk->cannot_instantiate_with_device_add_yet = true;
+ dk->user_creatable = false;
}
static const TypeInfo spapr_dr_connector_info = {
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 69b0291e8a..1ec30c45ce 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -867,7 +867,7 @@ static void s390_pcihost_class_init(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
dc->reset = s390_pcihost_reset;
k->init = s390_pcihost_init;
hc->plug = s390_pcihost_hot_plug;
diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
index 1f2f0ed44a..4008c81002 100644
--- a/hw/sd/milkymist-memcard.c
+++ b/hw/sd/milkymist-memcard.c
@@ -299,7 +299,7 @@ static void milkymist_memcard_class_init(ObjectClass *klass, void *data)
dc->reset = milkymist_memcard_reset;
dc->vmsd = &vmstate_milkymist_memcard;
/* Reason: init() method uses drive_get_next() */
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo milkymist_memcard_info = {
diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
index 82c63a4fb5..55c8098ecd 100644
--- a/hw/sd/pl181.c
+++ b/hw/sd/pl181.c
@@ -515,7 +515,7 @@ static void pl181_class_init(ObjectClass *klass, void *data)
k->vmsd = &vmstate_pl181;
k->reset = pl181_reset;
/* Reason: init() method uses drive_get_next() */
- k->cannot_instantiate_with_device_add_yet = true;
+ k->user_creatable = false;
k->realize = pl181_realize;
}
diff --git a/hw/sh4/sh_pci.c b/hw/sh4/sh_pci.c
index 1747628f3d..38395c082b 100644
--- a/hw/sh4/sh_pci.c
+++ b/hw/sh4/sh_pci.c
@@ -171,7 +171,7 @@ static void sh_pci_host_class_init(ObjectClass *klass, void *data)
* PCI-facing part of the host bridge, not usable without the
* host-facing part, which can't be device_add'ed, yet.
*/
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo sh_pci_host_info = {
diff --git a/hw/timer/i8254_common.c b/hw/timer/i8254_common.c
index e18299a482..976d5200f1 100644
--- a/hw/timer/i8254_common.c
+++ b/hw/timer/i8254_common.c
@@ -287,7 +287,7 @@ static void pit_common_class_init(ObjectClass *klass, void *data)
* wired to the HPET, and because of that, some wiring is always
* done by board code.
*/
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo pit_common_type = {
diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 4165450250..93de3e1cc5 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -973,7 +973,7 @@ static void rtc_class_initfn(ObjectClass *klass, void *data)
dc->vmsd = &vmstate_rtc;
dc->props = mc146818rtc_properties;
/* Reason: needs to be wired up by rtc_init() */
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static void rtc_finalize(Object *obj)
diff --git a/monitor.c b/monitor.c
index be282ecb80..e06edec2bd 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3156,7 +3156,7 @@ void device_add_completion(ReadLineState *rs, int nb_args, const char *str)
TYPE_DEVICE);
name = object_class_get_name(OBJECT_CLASS(dc));
- if (!dc->cannot_instantiate_with_device_add_yet
+ if (dc->user_creatable
&& !strncmp(name, str, len)) {
readline_add_completion(rs, name);
}
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 5f2fcdfc45..e4c180c6bc 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -113,7 +113,7 @@ static void qdev_print_devinfo(DeviceClass *dc)
if (dc->desc) {
error_printf(", desc \"%s\"", dc->desc);
}
- if (dc->cannot_instantiate_with_device_add_yet) {
+ if (!dc->user_creatable) {
error_printf(", no-user");
}
error_printf("\n");
@@ -155,7 +155,7 @@ static void qdev_print_devinfos(bool show_no_user)
? !test_bit(i, dc->categories)
: !bitmap_empty(dc->categories, DEVICE_CATEGORY_MAX))
|| (!show_no_user
- && dc->cannot_instantiate_with_device_add_yet)) {
+ && !dc->user_creatable)) {
continue;
}
if (!cat_printed) {
@@ -240,7 +240,7 @@ static DeviceClass *qdev_get_device_class(const char **driver, Error **errp)
}
dc = DEVICE_CLASS(oc);
- if (dc->cannot_instantiate_with_device_add_yet ||
+ if (!dc->user_creatable ||
(qdev_hotplug && !dc->hotpluggable)) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
"pluggable device type");
diff --git a/qom/cpu.c b/qom/cpu.c
index f02e9c0fae..73ae140d7e 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -449,7 +449,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
* Reason: CPUs still need special care by board code: wiring up
* IRQs, adding reset handlers, halting non-first CPUs, ...
*/
- dc->cannot_instantiate_with_device_add_yet = true;
+ dc->user_creatable = false;
}
static const TypeInfo cpu_type_info = {
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 13c0985f11..4b3bfb3802 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4066,7 +4066,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
cc->cpu_exec_enter = x86_cpu_exec_enter;
cc->cpu_exec_exit = x86_cpu_exec_exit;
- dc->cannot_instantiate_with_device_add_yet = false;
+ dc->user_creatable = true;
}
static const TypeInfo x86_cpu_type_info = {
--
2.11.0.259.g40922b1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [RFC 01/19] qdev: Replace cannot_instantiate_with_device_add_yet with !user_creatable
2017-04-01 0:46 ` [Qemu-devel] [RFC 01/19] qdev: Replace cannot_instantiate_with_device_add_yet with !user_creatable Eduardo Habkost
@ 2017-04-03 17:16 ` Alistair Francis
0 siblings, 0 replies; 55+ messages in thread
From: Alistair Francis @ 2017-04-03 17:16 UTC (permalink / raw)
To: Eduardo Habkost
Cc: qemu-devel@nongnu.org Developers, Peter Maydell, Thomas Huth,
Alexander Graf, Markus Armbruster, Marcel Apfelbaum,
Laszlo Ersek
On Fri, Mar 31, 2017 at 5:46 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> cannot_instantiate_with_device_add_yet was introduced by commit
> 837d37167dc446af8a91189108b363c04609e296 to replace no_user. It
> was supposed to be a temporary measure.
>
> When it was introduced, we had 54
> cannot_instantiate_with_device_add_yet=true lines in the code.
> Today (3 years later) this number has not shrinked: we now have
> 57 cannot_instantiate_with_device_add_yet=true lines. I think it
> is safe to say it is not a temporary measure, and we won't see
> the flag go away soon.
>
> Instead of a long field name that misleads people to believe it
> is temporary, replace it a shorter and less misleading field:
> user_creatable.
I haven't tested this, but I do like the new name and think it makes
more sense then what we have now.
Acked-by: Alistair Francis <alistair.francis@xilinx.com>
Thanks,
Alistair
>
> Except for code comments, changes were generated using the
> following Coccinelle patch:
>
> @@
> expression DC;
> @@
> (
> -DC->cannot_instantiate_with_device_add_yet = false;
> +DC->user_creatable = true;
> |
> -DC->cannot_instantiate_with_device_add_yet = true;
> +DC->user_creatable = false;
> )
>
> @@
> typedef ObjectClass;
> expression dc;
> identifier class, data;
> @@
> static void device_class_init(ObjectClass *class, void *data)
> {
> ...
> dc->hotpluggable = true;
> +dc->user_creatable = true;
> ...
> }
>
> @@
> @@
> struct DeviceClass {
> ...
> -bool cannot_instantiate_with_device_add_yet;
> +bool user_creatable;
> ...
> }
>
> @@
> expression DC;
> @@
> (
> -!DC->cannot_instantiate_with_device_add_yet
> +DC->user_creatable
> |
> -DC->cannot_instantiate_with_device_add_yet
> +!DC->user_creatable
> )
>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Marcel Apfelbaum <marcel@redhat.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> include/hw/qdev-core.h | 10 +++++-----
> include/hw/qdev-properties.h | 4 ++--
> hw/acpi/piix4.c | 2 +-
> hw/arm/spitz.c | 2 +-
> hw/audio/marvell_88w8618.c | 2 +-
> hw/audio/pcspk.c | 2 +-
> hw/core/or-irq.c | 2 +-
> hw/core/qdev.c | 1 +
> hw/core/register.c | 2 +-
> hw/dma/i8257.c | 2 +-
> hw/dma/sparc32_dma.c | 2 +-
> hw/gpio/omap_gpio.c | 4 ++--
> hw/i2c/omap_i2c.c | 2 +-
> hw/i2c/smbus_eeprom.c | 2 +-
> hw/i2c/smbus_ich9.c | 2 +-
> hw/i386/pc.c | 2 +-
> hw/input/vmmouse.c | 2 +-
> hw/intc/apic_common.c | 2 +-
> hw/intc/etraxfs_pic.c | 2 +-
> hw/intc/grlib_irqmp.c | 2 +-
> hw/intc/i8259_common.c | 2 +-
> hw/intc/nios2_iic.c | 2 +-
> hw/intc/omap_intc.c | 4 ++--
> hw/isa/lpc_ich9.c | 2 +-
> hw/isa/piix4.c | 2 +-
> hw/isa/vt82c686.c | 2 +-
> hw/mips/gt64xxx_pci.c | 2 +-
> hw/misc/vmport.c | 2 +-
> hw/net/dp8393x.c | 2 +-
> hw/net/etraxfs_eth.c | 2 +-
> hw/net/lance.c | 2 +-
> hw/pci-bridge/dec.c | 2 +-
> hw/pci-bridge/pci_expander_bridge.c | 2 +-
> hw/pci-host/apb.c | 2 +-
> hw/pci-host/bonito.c | 2 +-
> hw/pci-host/gpex.c | 2 +-
> hw/pci-host/grackle.c | 2 +-
> hw/pci-host/piix.c | 6 +++---
> hw/pci-host/ppce500.c | 2 +-
> hw/pci-host/prep.c | 2 +-
> hw/pci-host/q35.c | 4 ++--
> hw/pci-host/uninorth.c | 8 ++++----
> hw/pci-host/versatile.c | 2 +-
> hw/pci-host/xilinx-pcie.c | 2 +-
> hw/ppc/ppc4xx_pci.c | 2 +-
> hw/ppc/spapr_drc.c | 2 +-
> hw/s390x/s390-pci-bus.c | 2 +-
> hw/sd/milkymist-memcard.c | 2 +-
> hw/sd/pl181.c | 2 +-
> hw/sh4/sh_pci.c | 2 +-
> hw/timer/i8254_common.c | 2 +-
> hw/timer/mc146818rtc.c | 2 +-
> monitor.c | 2 +-
> qdev-monitor.c | 6 +++---
> qom/cpu.c | 2 +-
> target/i386/cpu.c | 2 +-
> 56 files changed, 71 insertions(+), 70 deletions(-)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index b44b476765..bc4e6f0486 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -103,16 +103,16 @@ typedef struct DeviceClass {
> Property *props;
>
> /*
> - * Shall we hide this device model from -device / device_add?
> + * Can this device be instantiated with -device / device_add?
> * All devices should support instantiation with device_add, and
> * this flag should not exist. But we're not there, yet. Some
> * devices fail to instantiate with cryptic error messages.
> * Others instantiate, but don't work. Exposing users to such
> - * behavior would be cruel; this flag serves to protect them. It
> - * should never be set without a comment explaining why it is set.
> - * TODO remove once we're there
> + * behavior would be cruel; clearing this flag will protect them.
> + * It should never be cleared without a comment explaining why it
> + * is cleared.
> */
> - bool cannot_instantiate_with_device_add_yet;
> + bool user_creatable;
> /*
> * Does this device model survive object_unref(object_new(TNAME))?
> * All device models should, and this flag shouldn't exist. Some
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 7ac315331a..5c6e13b993 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -134,12 +134,12 @@ extern PropertyInfo qdev_prop_arraylen;
> * device_add, so add code like this:
> * |* Reason: pointer property "NAME-OF-YOUR-PROP" *|
> * DeviceClass *dc = DEVICE_CLASS(class);
> - * dc->cannot_instantiate_with_device_add_yet = true;
> + * dc->user_creatable = false;
> *
> * - If the property may safely remain null, document it like this:
> * |*
> * * Note: pointer property "interrupt_vector" may remain null, thus
> - * * no need for dc->cannot_instantiate_with_device_add_yet = true;
> + * * no need for dc->user_creatable = false;
> * *|
> */
> #define DEFINE_PROP_PTR(_n, _s, _f) \
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index a553a7e110..f4fd5907b8 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -700,7 +700,7 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
> * Reason: part of PIIX4 southbridge, needs to be wired up,
> * e.g. by mips_malta_init()
> */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> dc->hotpluggable = false;
> hc->plug = piix4_device_plug_cb;
> hc->unplug_request = piix4_device_unplug_request_cb;
> diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
> index fe2d5a764c..324626847c 100644
> --- a/hw/arm/spitz.c
> +++ b/hw/arm/spitz.c
> @@ -1076,7 +1076,7 @@ static void sl_nand_class_init(ObjectClass *klass, void *data)
> dc->vmsd = &vmstate_sl_nand_info;
> dc->props = sl_nand_properties;
> /* Reason: init() method uses drive_get() */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo sl_nand_info = {
> diff --git a/hw/audio/marvell_88w8618.c b/hw/audio/marvell_88w8618.c
> index 511b004287..4f65f8c199 100644
> --- a/hw/audio/marvell_88w8618.c
> +++ b/hw/audio/marvell_88w8618.c
> @@ -292,7 +292,7 @@ static void mv88w8618_audio_class_init(ObjectClass *klass, void *data)
> dc->vmsd = &mv88w8618_audio_vmsd;
> dc->props = mv88w8618_audio_properties;
> /* Reason: pointer property "wm8750" */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo mv88w8618_audio_info = {
> diff --git a/hw/audio/pcspk.c b/hw/audio/pcspk.c
> index 798002277b..9b99358d87 100644
> --- a/hw/audio/pcspk.c
> +++ b/hw/audio/pcspk.c
> @@ -223,7 +223,7 @@ static void pcspk_class_initfn(ObjectClass *klass, void *data)
> dc->vmsd = &vmstate_spk;
> dc->props = pcspk_properties;
> /* Reason: realize sets global pcspk_state */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo pcspk_info = {
> diff --git a/hw/core/or-irq.c b/hw/core/or-irq.c
> index 1485d5b285..f9d76c4641 100644
> --- a/hw/core/or-irq.c
> +++ b/hw/core/or-irq.c
> @@ -91,7 +91,7 @@ static void or_irq_class_init(ObjectClass *klass, void *data)
> dc->vmsd = &vmstate_or_irq;
>
> /* Reason: Needs to be wired up to work, e.g. see stm32f205_soc.c */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo or_irq_type_info = {
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 1e7fb33246..4132a8bee3 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -1159,6 +1159,7 @@ static void device_class_init(ObjectClass *class, void *data)
> * should override it in their class_init()
> */
> dc->hotpluggable = true;
> + dc->user_creatable = true;
> }
>
> void device_reset(DeviceState *dev)
> diff --git a/hw/core/register.c b/hw/core/register.c
> index dc335a79a9..da38ef3a54 100644
> --- a/hw/core/register.c
> +++ b/hw/core/register.c
> @@ -288,7 +288,7 @@ static void register_class_init(ObjectClass *oc, void *data)
> DeviceClass *dc = DEVICE_CLASS(oc);
>
> /* Reason: needs to be wired up to work */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo register_info = {
> diff --git a/hw/dma/i8257.c b/hw/dma/i8257.c
> index 8bd82e8bc8..bd23e893bf 100644
> --- a/hw/dma/i8257.c
> +++ b/hw/dma/i8257.c
> @@ -601,7 +601,7 @@ static void i8257_class_init(ObjectClass *klass, void *data)
> idc->schedule = i8257_dma_schedule;
> idc->register_channel = i8257_dma_register_channel;
> /* Reason: needs to be wired up by isa_bus_dma() to work */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo i8257_info = {
> diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c
> index 9d545e412e..9c6bdc6295 100644
> --- a/hw/dma/sparc32_dma.c
> +++ b/hw/dma/sparc32_dma.c
> @@ -305,7 +305,7 @@ static void sparc32_dma_class_init(ObjectClass *klass, void *data)
> dc->vmsd = &vmstate_dma;
> dc->props = sparc32_dma_properties;
> /* Reason: pointer property "iommu_opaque" */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo sparc32_dma_info = {
> diff --git a/hw/gpio/omap_gpio.c b/hw/gpio/omap_gpio.c
> index dabef4a119..1df394eb12 100644
> --- a/hw/gpio/omap_gpio.c
> +++ b/hw/gpio/omap_gpio.c
> @@ -773,7 +773,7 @@ static void omap_gpio_class_init(ObjectClass *klass, void *data)
> dc->reset = omap_gpif_reset;
> dc->props = omap_gpio_properties;
> /* Reason: pointer property "clk" */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo omap_gpio_info = {
> @@ -804,7 +804,7 @@ static void omap2_gpio_class_init(ObjectClass *klass, void *data)
> dc->reset = omap2_gpif_reset;
> dc->props = omap2_gpio_properties;
> /* Reason: pointer properties "iclk", "fclk0", ..., "fclk5" */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo omap2_gpio_info = {
> diff --git a/hw/i2c/omap_i2c.c b/hw/i2c/omap_i2c.c
> index f7c92ea00c..f6e80bee25 100644
> --- a/hw/i2c/omap_i2c.c
> +++ b/hw/i2c/omap_i2c.c
> @@ -491,7 +491,7 @@ static void omap_i2c_class_init(ObjectClass *klass, void *data)
> dc->props = omap_i2c_properties;
> dc->reset = omap_i2c_reset;
> /* Reason: pointer properties "iclk", "fclk" */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> dc->realize = omap_i2c_realize;
> }
>
> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> index 5b7bd891bc..b13ec0fe7a 100644
> --- a/hw/i2c/smbus_eeprom.c
> +++ b/hw/i2c/smbus_eeprom.c
> @@ -123,7 +123,7 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
> sc->read_data = eeprom_read_data;
> dc->props = smbus_eeprom_properties;
> /* Reason: pointer property "data" */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo smbus_eeprom_info = {
> diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
> index 48fab22625..ea51e09186 100644
> --- a/hw/i2c/smbus_ich9.c
> +++ b/hw/i2c/smbus_ich9.c
> @@ -103,7 +103,7 @@ static void ich9_smb_class_init(ObjectClass *klass, void *data)
> * Reason: part of ICH9 southbridge, needs to be wired up by
> * pc_q35_init()
> */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> I2CBus *ich9_smb_init(PCIBus *bus, int devfn, uint32_t smb_io_base)
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index d24388e05f..610050eb4f 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -597,7 +597,7 @@ static void port92_class_initfn(ObjectClass *klass, void *data)
> * wiring: its A20 output line needs to be wired up by
> * port92_init().
> */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo port92_info = {
> diff --git a/hw/input/vmmouse.c b/hw/input/vmmouse.c
> index 6d15a887c6..4747da9a8d 100644
> --- a/hw/input/vmmouse.c
> +++ b/hw/input/vmmouse.c
> @@ -286,7 +286,7 @@ static void vmmouse_class_initfn(ObjectClass *klass, void *data)
> dc->vmsd = &vmstate_vmmouse;
> dc->props = vmmouse_properties;
> /* Reason: pointer property "ps2_mouse" */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo vmmouse_info = {
> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> index c3829e31b5..1ef56f8d10 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -501,7 +501,7 @@ static void apic_common_class_init(ObjectClass *klass, void *data)
> * Reason: APIC and CPU need to be wired up by
> * x86_cpu_apic_create()
> */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo apic_common_type = {
> diff --git a/hw/intc/etraxfs_pic.c b/hw/intc/etraxfs_pic.c
> index 64a6f4b4ba..1bfde2f09e 100644
> --- a/hw/intc/etraxfs_pic.c
> +++ b/hw/intc/etraxfs_pic.c
> @@ -173,7 +173,7 @@ static void etraxfs_pic_class_init(ObjectClass *klass, void *data)
> dc->props = etraxfs_pic_properties;
> /*
> * Note: pointer property "interrupt_vector" may remain null, thus
> - * no need for dc->cannot_instantiate_with_device_add_yet = true;
> + * no need for dc->user_creatable = false;
> */
> }
>
> diff --git a/hw/intc/grlib_irqmp.c b/hw/intc/grlib_irqmp.c
> index ac7e63f38b..94659ee256 100644
> --- a/hw/intc/grlib_irqmp.c
> +++ b/hw/intc/grlib_irqmp.c
> @@ -360,7 +360,7 @@ static void grlib_irqmp_class_init(ObjectClass *klass, void *data)
> dc->reset = grlib_irqmp_reset;
> dc->props = grlib_irqmp_properties;
> /* Reason: pointer properties "set_pil_in", "set_pil_in_opaque" */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> dc->realize = grlib_irqmp_realize;
> }
>
> diff --git a/hw/intc/i8259_common.c b/hw/intc/i8259_common.c
> index d9a5e8b217..c2fd563b5b 100644
> --- a/hw/intc/i8259_common.c
> +++ b/hw/intc/i8259_common.c
> @@ -144,7 +144,7 @@ static void pic_common_class_init(ObjectClass *klass, void *data)
> * wiring of the slave to the master is hard-coded in device model
> * code.
> */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo pic_common_type = {
> diff --git a/hw/intc/nios2_iic.c b/hw/intc/nios2_iic.c
> index 190b6fdbf3..016426f964 100644
> --- a/hw/intc/nios2_iic.c
> +++ b/hw/intc/nios2_iic.c
> @@ -80,7 +80,7 @@ static void altera_iic_class_init(ObjectClass *klass, void *data)
> DeviceClass *dc = DEVICE_CLASS(klass);
>
> /* Reason: needs to be wired up, e.g. by nios2_10m50_ghrd_init() */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> dc->realize = altera_iic_realize;
> }
>
> diff --git a/hw/intc/omap_intc.c b/hw/intc/omap_intc.c
> index 877be67971..ccdda89dab 100644
> --- a/hw/intc/omap_intc.c
> +++ b/hw/intc/omap_intc.c
> @@ -401,7 +401,7 @@ static void omap_intc_class_init(ObjectClass *klass, void *data)
> dc->reset = omap_inth_reset;
> dc->props = omap_intc_properties;
> /* Reason: pointer property "clk" */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> dc->realize = omap_intc_realize;
> }
>
> @@ -656,7 +656,7 @@ static void omap2_intc_class_init(ObjectClass *klass, void *data)
> dc->reset = omap_inth_reset;
> dc->props = omap2_intc_properties;
> /* Reason: pointer property "iclk", "fclk" */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> dc->realize = omap2_intc_realize;
> }
>
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 59930dd9d0..cdc854a822 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -810,7 +810,7 @@ static void ich9_lpc_class_init(ObjectClass *klass, void *data)
> * Reason: part of ICH9 southbridge, needs to be wired up by
> * pc_q35_init()
> */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> hc->plug = ich9_pm_device_plug_cb;
> hc->unplug_request = ich9_pm_device_unplug_request_cb;
> hc->unplug = ich9_pm_device_unplug_cb;
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index 5500fcc4d6..f811eba59d 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -123,7 +123,7 @@ static void piix4_class_init(ObjectClass *klass, void *data)
> * Reason: part of PIIX4 southbridge, needs to be wired up,
> * e.g. by mips_malta_init()
> */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> dc->hotpluggable = false;
> }
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 41d5254f8e..50dc83df77 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -494,7 +494,7 @@ static void via_class_init(ObjectClass *klass, void *data)
> * Reason: part of VIA VT82C686 southbridge, needs to be wired up,
> * e.g. by mips_fulong2e_init()
> */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo via_info = {
> diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
> index 4811843ab6..e8b2eef688 100644
> --- a/hw/mips/gt64xxx_pci.c
> +++ b/hw/mips/gt64xxx_pci.c
> @@ -1224,7 +1224,7 @@ static void gt64120_pci_class_init(ObjectClass *klass, void *data)
> * PCI-facing part of the host bridge, not usable without the
> * host-facing part, which can't be device_add'ed, yet.
> */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo gt64120_pci_info = {
> diff --git a/hw/misc/vmport.c b/hw/misc/vmport.c
> index be40930b8b..165500223f 100644
> --- a/hw/misc/vmport.c
> +++ b/hw/misc/vmport.c
> @@ -163,7 +163,7 @@ static void vmport_class_initfn(ObjectClass *klass, void *data)
>
> dc->realize = vmport_realizefn;
> /* Reason: realize sets global port_state */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo vmport_info = {
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index efa33ad40a..b53fcaa8bc 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -934,7 +934,7 @@ static void dp8393x_class_init(ObjectClass *klass, void *data)
> dc->vmsd = &vmstate_dp8393x;
> dc->props = dp8393x_properties;
> /* Reason: dma_mr property can't be set */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo dp8393x_info = {
> diff --git a/hw/net/etraxfs_eth.c b/hw/net/etraxfs_eth.c
> index efaa49faae..013c8d0a41 100644
> --- a/hw/net/etraxfs_eth.c
> +++ b/hw/net/etraxfs_eth.c
> @@ -630,7 +630,7 @@ static void etraxfs_eth_class_init(ObjectClass *klass, void *data)
> k->init = fs_eth_init;
> dc->props = etraxfs_eth_properties;
> /* Reason: pointer properties "dma_out", "dma_in" */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo etraxfs_eth_info = {
> diff --git a/hw/net/lance.c b/hw/net/lance.c
> index 573d724bcf..92b0c68274 100644
> --- a/hw/net/lance.c
> +++ b/hw/net/lance.c
> @@ -165,7 +165,7 @@ static void lance_class_init(ObjectClass *klass, void *data)
> dc->vmsd = &vmstate_lance;
> dc->props = lance_properties;
> /* Reason: pointer property "dma" */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo lance_info = {
> diff --git a/hw/pci-bridge/dec.c b/hw/pci-bridge/dec.c
> index 840c96198a..cca93620ac 100644
> --- a/hw/pci-bridge/dec.c
> +++ b/hw/pci-bridge/dec.c
> @@ -128,7 +128,7 @@ static void dec_21154_pci_host_class_init(ObjectClass *klass, void *data)
> * PCI-facing part of the host bridge, not usable without the
> * host-facing part, which can't be device_add'ed, yet.
> */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo dec_21154_pci_host_info = {
> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
> index 6ac187fa32..ff59abf208 100644
> --- a/hw/pci-bridge/pci_expander_bridge.c
> +++ b/hw/pci-bridge/pci_expander_bridge.c
> @@ -150,7 +150,7 @@ static void pxb_host_class_init(ObjectClass *class, void *data)
>
> dc->fw_name = "pci";
> /* Reason: Internal part of the pxb/pxb-pcie device, not usable by itself */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> sbc->explicit_ofw_unit_address = pxb_host_ofw_unit_address;
> hc->root_bus_path = pxb_host_root_bus_path;
> }
> diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c
> index 653e711121..edc88f4c65 100644
> --- a/hw/pci-host/apb.c
> +++ b/hw/pci-host/apb.c
> @@ -810,7 +810,7 @@ static void pbm_pci_host_class_init(ObjectClass *klass, void *data)
> * PCI-facing part of the host bridge, not usable without the
> * host-facing part, which can't be device_add'ed, yet.
> */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo pbm_pci_host_info = {
> diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
> index 1999ece590..85a3bb0dd2 100644
> --- a/hw/pci-host/bonito.c
> +++ b/hw/pci-host/bonito.c
> @@ -825,7 +825,7 @@ static void bonito_class_init(ObjectClass *klass, void *data)
> * PCI-facing part of the host bridge, not usable without the
> * host-facing part, which can't be device_add'ed, yet.
> */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo bonito_info = {
> diff --git a/hw/pci-host/gpex.c b/hw/pci-host/gpex.c
> index 66055ee5cc..e2629ce70d 100644
> --- a/hw/pci-host/gpex.c
> +++ b/hw/pci-host/gpex.c
> @@ -136,7 +136,7 @@ static void gpex_root_class_init(ObjectClass *klass, void *data)
> * PCI-facing part of the host bridge, not usable without the
> * host-facing part, which can't be device_add'ed, yet.
> */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo gpex_root_info = {
> diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c
> index 2c8acdaaca..2e281f6155 100644
> --- a/hw/pci-host/grackle.c
> +++ b/hw/pci-host/grackle.c
> @@ -134,7 +134,7 @@ static void grackle_pci_class_init(ObjectClass *klass, void *data)
> * PCI-facing part of the host bridge, not usable without the
> * host-facing part, which can't be device_add'ed, yet.
> */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo grackle_pci_info = {
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index f9218aa952..81f3a9e211 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -691,7 +691,7 @@ static void pci_piix3_class_init(ObjectClass *klass, void *data)
> * Reason: part of PIIX3 southbridge, needs to be wired up by
> * pc_piix.c's pc_init1()
> */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo piix3_pci_type_info = {
> @@ -745,7 +745,7 @@ static void i440fx_class_init(ObjectClass *klass, void *data)
> * PCI-facing part of the host bridge, not usable without the
> * host-facing part, which can't be device_add'ed, yet.
> */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> dc->hotpluggable = false;
> }
>
> @@ -874,7 +874,7 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
> dc->fw_name = "pci";
> dc->props = i440fx_props;
> /* Reason: needs to be wired up by pc_init1 */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo i440fx_pcihost_info = {
> diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
> index e502bc0505..becc0eeb76 100644
> --- a/hw/pci-host/ppce500.c
> +++ b/hw/pci-host/ppce500.c
> @@ -508,7 +508,7 @@ static void e500_host_bridge_class_init(ObjectClass *klass, void *data)
> * PCI-facing part of the host bridge, not usable without the
> * host-facing part, which can't be device_add'ed, yet.
> */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo e500_host_bridge_info = {
> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
> index 260a119a9e..900a6edfcf 100644
> --- a/hw/pci-host/prep.c
> +++ b/hw/pci-host/prep.c
> @@ -364,7 +364,7 @@ static void raven_class_init(ObjectClass *klass, void *data)
> * Reason: PCI-facing part of the host bridge, not usable without
> * the host-facing part, which can't be device_add'ed, yet.
> */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo raven_info = {
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 344f77b10c..cd5c49616e 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -156,7 +156,7 @@ static void q35_host_class_init(ObjectClass *klass, void *data)
> dc->realize = q35_host_realize;
> dc->props = mch_props;
> /* Reason: needs to be wired up by pc_q35_init */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> dc->fw_name = "pci";
> }
> @@ -549,7 +549,7 @@ static void mch_class_init(ObjectClass *klass, void *data)
> * PCI-facing part of the host bridge, not usable without the
> * host-facing part, which can't be device_add'ed, yet.
> */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo mch_info = {
> diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c
> index df342ac3cb..6cf5e59f86 100644
> --- a/hw/pci-host/uninorth.c
> +++ b/hw/pci-host/uninorth.c
> @@ -366,7 +366,7 @@ static void unin_main_pci_host_class_init(ObjectClass *klass, void *data)
> * PCI-facing part of the host bridge, not usable without the
> * host-facing part, which can't be device_add'ed, yet.
> */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo unin_main_pci_host_info = {
> @@ -390,7 +390,7 @@ static void u3_agp_pci_host_class_init(ObjectClass *klass, void *data)
> * PCI-facing part of the host bridge, not usable without the
> * host-facing part, which can't be device_add'ed, yet.
> */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo u3_agp_pci_host_info = {
> @@ -414,7 +414,7 @@ static void unin_agp_pci_host_class_init(ObjectClass *klass, void *data)
> * PCI-facing part of the host bridge, not usable without the
> * host-facing part, which can't be device_add'ed, yet.
> */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo unin_agp_pci_host_info = {
> @@ -438,7 +438,7 @@ static void unin_internal_pci_host_class_init(ObjectClass *klass, void *data)
> * PCI-facing part of the host bridge, not usable without the
> * host-facing part, which can't be device_add'ed, yet.
> */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo unin_internal_pci_host_info = {
> diff --git a/hw/pci-host/versatile.c b/hw/pci-host/versatile.c
> index 467cbb9cb8..cc51fc87a3 100644
> --- a/hw/pci-host/versatile.c
> +++ b/hw/pci-host/versatile.c
> @@ -479,7 +479,7 @@ static void versatile_pci_host_class_init(ObjectClass *klass, void *data)
> * PCI-facing part of the host bridge, not usable without the
> * host-facing part, which can't be device_add'ed, yet.
> */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo versatile_pci_host_info = {
> diff --git a/hw/pci-host/xilinx-pcie.c b/hw/pci-host/xilinx-pcie.c
> index 8b71e2d950..a968cea2af 100644
> --- a/hw/pci-host/xilinx-pcie.c
> +++ b/hw/pci-host/xilinx-pcie.c
> @@ -309,7 +309,7 @@ static void xilinx_pcie_root_class_init(ObjectClass *klass, void *data)
> * PCI-facing part of the host bridge, not usable without the
> * host-facing part, which can't be device_add'ed, yet.
> */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo xilinx_pcie_root_info = {
> diff --git a/hw/ppc/ppc4xx_pci.c b/hw/ppc/ppc4xx_pci.c
> index dc19682970..6953f8b9ac 100644
> --- a/hw/ppc/ppc4xx_pci.c
> +++ b/hw/ppc/ppc4xx_pci.c
> @@ -351,7 +351,7 @@ static void ppc4xx_host_bridge_class_init(ObjectClass *klass, void *data)
> * PCI-facing part of the host bridge, not usable without the
> * host-facing part, which can't be device_add'ed, yet.
> */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo ppc4xx_host_bridge_info = {
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index a1cdc875b1..9fa5545991 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -675,7 +675,7 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> /*
> * Reason: it crashes FIXME find and document the real reason
> */
> - dk->cannot_instantiate_with_device_add_yet = true;
> + dk->user_creatable = false;
> }
>
> static const TypeInfo spapr_dr_connector_info = {
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 69b0291e8a..1ec30c45ce 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -867,7 +867,7 @@ static void s390_pcihost_class_init(ObjectClass *klass, void *data)
> DeviceClass *dc = DEVICE_CLASS(klass);
> HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
>
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> dc->reset = s390_pcihost_reset;
> k->init = s390_pcihost_init;
> hc->plug = s390_pcihost_hot_plug;
> diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
> index 1f2f0ed44a..4008c81002 100644
> --- a/hw/sd/milkymist-memcard.c
> +++ b/hw/sd/milkymist-memcard.c
> @@ -299,7 +299,7 @@ static void milkymist_memcard_class_init(ObjectClass *klass, void *data)
> dc->reset = milkymist_memcard_reset;
> dc->vmsd = &vmstate_milkymist_memcard;
> /* Reason: init() method uses drive_get_next() */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo milkymist_memcard_info = {
> diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
> index 82c63a4fb5..55c8098ecd 100644
> --- a/hw/sd/pl181.c
> +++ b/hw/sd/pl181.c
> @@ -515,7 +515,7 @@ static void pl181_class_init(ObjectClass *klass, void *data)
> k->vmsd = &vmstate_pl181;
> k->reset = pl181_reset;
> /* Reason: init() method uses drive_get_next() */
> - k->cannot_instantiate_with_device_add_yet = true;
> + k->user_creatable = false;
> k->realize = pl181_realize;
> }
>
> diff --git a/hw/sh4/sh_pci.c b/hw/sh4/sh_pci.c
> index 1747628f3d..38395c082b 100644
> --- a/hw/sh4/sh_pci.c
> +++ b/hw/sh4/sh_pci.c
> @@ -171,7 +171,7 @@ static void sh_pci_host_class_init(ObjectClass *klass, void *data)
> * PCI-facing part of the host bridge, not usable without the
> * host-facing part, which can't be device_add'ed, yet.
> */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo sh_pci_host_info = {
> diff --git a/hw/timer/i8254_common.c b/hw/timer/i8254_common.c
> index e18299a482..976d5200f1 100644
> --- a/hw/timer/i8254_common.c
> +++ b/hw/timer/i8254_common.c
> @@ -287,7 +287,7 @@ static void pit_common_class_init(ObjectClass *klass, void *data)
> * wired to the HPET, and because of that, some wiring is always
> * done by board code.
> */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo pit_common_type = {
> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index 4165450250..93de3e1cc5 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -973,7 +973,7 @@ static void rtc_class_initfn(ObjectClass *klass, void *data)
> dc->vmsd = &vmstate_rtc;
> dc->props = mc146818rtc_properties;
> /* Reason: needs to be wired up by rtc_init() */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static void rtc_finalize(Object *obj)
> diff --git a/monitor.c b/monitor.c
> index be282ecb80..e06edec2bd 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3156,7 +3156,7 @@ void device_add_completion(ReadLineState *rs, int nb_args, const char *str)
> TYPE_DEVICE);
> name = object_class_get_name(OBJECT_CLASS(dc));
>
> - if (!dc->cannot_instantiate_with_device_add_yet
> + if (dc->user_creatable
> && !strncmp(name, str, len)) {
> readline_add_completion(rs, name);
> }
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 5f2fcdfc45..e4c180c6bc 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -113,7 +113,7 @@ static void qdev_print_devinfo(DeviceClass *dc)
> if (dc->desc) {
> error_printf(", desc \"%s\"", dc->desc);
> }
> - if (dc->cannot_instantiate_with_device_add_yet) {
> + if (!dc->user_creatable) {
> error_printf(", no-user");
> }
> error_printf("\n");
> @@ -155,7 +155,7 @@ static void qdev_print_devinfos(bool show_no_user)
> ? !test_bit(i, dc->categories)
> : !bitmap_empty(dc->categories, DEVICE_CATEGORY_MAX))
> || (!show_no_user
> - && dc->cannot_instantiate_with_device_add_yet)) {
> + && !dc->user_creatable)) {
> continue;
> }
> if (!cat_printed) {
> @@ -240,7 +240,7 @@ static DeviceClass *qdev_get_device_class(const char **driver, Error **errp)
> }
>
> dc = DEVICE_CLASS(oc);
> - if (dc->cannot_instantiate_with_device_add_yet ||
> + if (!dc->user_creatable ||
> (qdev_hotplug && !dc->hotpluggable)) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
> "pluggable device type");
> diff --git a/qom/cpu.c b/qom/cpu.c
> index f02e9c0fae..73ae140d7e 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -449,7 +449,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
> * Reason: CPUs still need special care by board code: wiring up
> * IRQs, adding reset handlers, halting non-first CPUs, ...
> */
> - dc->cannot_instantiate_with_device_add_yet = true;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo cpu_type_info = {
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 13c0985f11..4b3bfb3802 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -4066,7 +4066,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
> cc->cpu_exec_enter = x86_cpu_exec_enter;
> cc->cpu_exec_exit = x86_cpu_exec_exit;
>
> - dc->cannot_instantiate_with_device_add_yet = false;
> + dc->user_creatable = true;
> }
>
> static const TypeInfo x86_cpu_type_info = {
> --
> 2.11.0.259.g40922b1
>
>
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] [RFC 02/19] s390: Add FIXME for unexplained user_creatable=false line
2017-04-01 0:46 [Qemu-devel] [RFC 00/19] sysbus: Don't allow -device/device_add by default Eduardo Habkost
2017-04-01 0:46 ` [Qemu-devel] [RFC 01/19] qdev: Replace cannot_instantiate_with_device_add_yet with !user_creatable Eduardo Habkost
@ 2017-04-01 0:46 ` Eduardo Habkost
2017-04-03 8:55 ` Cornelia Huck
2017-04-01 0:46 ` [Qemu-devel] [RFC 03/19] sysbus: Set user_creatable=false by default on TYPE_SYS_BUS_DEVICE Eduardo Habkost
` (16 subsequent siblings)
18 siblings, 1 reply; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-01 0:46 UTC (permalink / raw)
To: qemu-devel
Cc: Laszlo Ersek, Alexander Graf, Marcel Apfelbaum, Thomas Huth,
Peter Maydell, Markus Armbruster, Frank Blaschka, Cornelia Huck,
Christian Borntraeger, Richard Henderson
TYPE_S390_PCI_HOST_BRIDGE has user_creatable=false but has
no comment explaining why. Add a FIXME to document that.
Cc: Frank Blaschka <frank.blaschka@de.ibm.com>
Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Alexander Graf <agraf@suse.de>
Cc: Richard Henderson <rth@twiddle.net>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
hw/s390x/s390-pci-bus.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 1ec30c45ce..2c3b960bdf 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -867,7 +867,7 @@ static void s390_pcihost_class_init(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
- dc->user_creatable = false;
+ dc->user_creatable = false; /*FIXME: explain why */
dc->reset = s390_pcihost_reset;
k->init = s390_pcihost_init;
hc->plug = s390_pcihost_hot_plug;
--
2.11.0.259.g40922b1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [RFC 02/19] s390: Add FIXME for unexplained user_creatable=false line
2017-04-01 0:46 ` [Qemu-devel] [RFC 02/19] s390: Add FIXME for unexplained user_creatable=false line Eduardo Habkost
@ 2017-04-03 8:55 ` Cornelia Huck
2017-04-03 19:20 ` Eduardo Habkost
0 siblings, 1 reply; 55+ messages in thread
From: Cornelia Huck @ 2017-04-03 8:55 UTC (permalink / raw)
To: Eduardo Habkost
Cc: qemu-devel, Peter Maydell, Thomas Huth, Christian Borntraeger,
Alexander Graf, Markus Armbruster, Marcel Apfelbaum,
Laszlo Ersek, Richard Henderson, Yi Min Zhao, Pierre Morel
On Fri, 31 Mar 2017 21:46:07 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:
> TYPE_S390_PCI_HOST_BRIDGE has user_creatable=false but has
> no comment explaining why. Add a FIXME to document that.
>
> Cc: Frank Blaschka <frank.blaschka@de.ibm.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Richard Henderson <rth@twiddle.net>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> hw/s390x/s390-pci-bus.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 1ec30c45ce..2c3b960bdf 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -867,7 +867,7 @@ static void s390_pcihost_class_init(ObjectClass *klass, void *data)
> DeviceClass *dc = DEVICE_CLASS(klass);
> HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
>
> - dc->user_creatable = false;
> + dc->user_creatable = false; /*FIXME: explain why */
> dc->reset = s390_pcihost_reset;
> k->init = s390_pcihost_init;
> hc->plug = s390_pcihost_hot_plug;
(adding some more possibly interested parties)
We currently have one master s390 phb (and it's been that way since
s390 pci was introduced). Recently, there has been some remodelling
going on to make this more similar to what sPAPR does. I think we could
make this even more similar to sPAPR and have this user createable; but
I'm currently not sure it's worth the effort. Opinions?
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [RFC 02/19] s390: Add FIXME for unexplained user_creatable=false line
2017-04-03 8:55 ` Cornelia Huck
@ 2017-04-03 19:20 ` Eduardo Habkost
2017-04-04 6:46 ` Cornelia Huck
0 siblings, 1 reply; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-03 19:20 UTC (permalink / raw)
To: Cornelia Huck
Cc: qemu-devel, Peter Maydell, Thomas Huth, Christian Borntraeger,
Alexander Graf, Markus Armbruster, Marcel Apfelbaum,
Laszlo Ersek, Richard Henderson, Yi Min Zhao, Pierre Morel
On Mon, Apr 03, 2017 at 10:55:38AM +0200, Cornelia Huck wrote:
> On Fri, 31 Mar 2017 21:46:07 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> > TYPE_S390_PCI_HOST_BRIDGE has user_creatable=false but has
> > no comment explaining why. Add a FIXME to document that.
> >
> > Cc: Frank Blaschka <frank.blaschka@de.ibm.com>
> > Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> > Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> > Cc: Alexander Graf <agraf@suse.de>
> > Cc: Richard Henderson <rth@twiddle.net>
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > hw/s390x/s390-pci-bus.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> > index 1ec30c45ce..2c3b960bdf 100644
> > --- a/hw/s390x/s390-pci-bus.c
> > +++ b/hw/s390x/s390-pci-bus.c
> > @@ -867,7 +867,7 @@ static void s390_pcihost_class_init(ObjectClass *klass, void *data)
> > DeviceClass *dc = DEVICE_CLASS(klass);
> > HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
> >
> > - dc->user_creatable = false;
> > + dc->user_creatable = false; /*FIXME: explain why */
> > dc->reset = s390_pcihost_reset;
> > k->init = s390_pcihost_init;
> > hc->plug = s390_pcihost_hot_plug;
>
> (adding some more possibly interested parties)
>
> We currently have one master s390 phb (and it's been that way since
> s390 pci was introduced). Recently, there has been some remodelling
> going on to make this more similar to what sPAPR does. I think we could
> make this even more similar to sPAPR and have this user createable; but
> I'm currently not sure it's worth the effort. Opinions?
It looks -device s390-pcihost was never possible, anyway, because
no s390x machine has has_dynamic_sysbus=1, and TYPE_PCI_HOST_BRIDGE
is a sys-bus-device.
Also, patch 03/19 on this series would make the explicit
user_creatable=false assignment in s390_pcihost_class_init()
unnecessary.
I don't think it is worth the effort to change that, unless you
have a specific use case that would benefit from it.
--
Eduardo
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [RFC 02/19] s390: Add FIXME for unexplained user_creatable=false line
2017-04-03 19:20 ` Eduardo Habkost
@ 2017-04-04 6:46 ` Cornelia Huck
0 siblings, 0 replies; 55+ messages in thread
From: Cornelia Huck @ 2017-04-04 6:46 UTC (permalink / raw)
To: Eduardo Habkost
Cc: qemu-devel, Peter Maydell, Thomas Huth, Christian Borntraeger,
Alexander Graf, Markus Armbruster, Marcel Apfelbaum,
Laszlo Ersek, Richard Henderson, Yi Min Zhao, Pierre Morel
On Mon, 3 Apr 2017 16:20:11 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Mon, Apr 03, 2017 at 10:55:38AM +0200, Cornelia Huck wrote:
> > On Fri, 31 Mar 2017 21:46:07 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> > > TYPE_S390_PCI_HOST_BRIDGE has user_creatable=false but has
> > > no comment explaining why. Add a FIXME to document that.
> > >
> > > Cc: Frank Blaschka <frank.blaschka@de.ibm.com>
> > > Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> > > Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> > > Cc: Alexander Graf <agraf@suse.de>
> > > Cc: Richard Henderson <rth@twiddle.net>
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > > hw/s390x/s390-pci-bus.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> > > index 1ec30c45ce..2c3b960bdf 100644
> > > --- a/hw/s390x/s390-pci-bus.c
> > > +++ b/hw/s390x/s390-pci-bus.c
> > > @@ -867,7 +867,7 @@ static void s390_pcihost_class_init(ObjectClass *klass, void *data)
> > > DeviceClass *dc = DEVICE_CLASS(klass);
> > > HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
> > >
> > > - dc->user_creatable = false;
> > > + dc->user_creatable = false; /*FIXME: explain why */
> > > dc->reset = s390_pcihost_reset;
> > > k->init = s390_pcihost_init;
> > > hc->plug = s390_pcihost_hot_plug;
> >
> > (adding some more possibly interested parties)
> >
> > We currently have one master s390 phb (and it's been that way since
> > s390 pci was introduced). Recently, there has been some remodelling
> > going on to make this more similar to what sPAPR does. I think we could
> > make this even more similar to sPAPR and have this user createable; but
> > I'm currently not sure it's worth the effort. Opinions?
>
> It looks -device s390-pcihost was never possible, anyway, because
> no s390x machine has has_dynamic_sysbus=1, and TYPE_PCI_HOST_BRIDGE
> is a sys-bus-device.
Yes.
>
> Also, patch 03/19 on this series would make the explicit
> user_creatable=false assignment in s390_pcihost_class_init()
> unnecessary.
If we switch to "sysbus devices are never user creatable, except for a
select few" anyway, I think we can just get rid of this.
>
> I don't think it is worth the effort to change that, unless you
> have a specific use case that would benefit from it.
Not really. We're building a highly artificical "topology" that does
not exist on real hardware (and is not seen by the guest) here, so we
can basically do whatever works best. We _might_ want to be more
similar to sPAPR, so that we're not the complete oddball, but it seems
that we can make everything work with the current setup already.
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] [RFC 03/19] sysbus: Set user_creatable=false by default on TYPE_SYS_BUS_DEVICE
2017-04-01 0:46 [Qemu-devel] [RFC 00/19] sysbus: Don't allow -device/device_add by default Eduardo Habkost
2017-04-01 0:46 ` [Qemu-devel] [RFC 01/19] qdev: Replace cannot_instantiate_with_device_add_yet with !user_creatable Eduardo Habkost
2017-04-01 0:46 ` [Qemu-devel] [RFC 02/19] s390: Add FIXME for unexplained user_creatable=false line Eduardo Habkost
@ 2017-04-01 0:46 ` Eduardo Habkost
2017-04-03 19:49 ` Peter Maydell
2017-04-01 0:46 ` [Qemu-devel] [RFC 04/19] fdc: Remove user_creatable flag from sysbus-fdc & SUNW, fdtwo Eduardo Habkost
` (15 subsequent siblings)
18 siblings, 1 reply; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-01 0:46 UTC (permalink / raw)
To: qemu-devel
Cc: Laszlo Ersek, Alexander Graf, Marcel Apfelbaum, Thomas Huth,
Peter Maydell, Markus Armbruster, John Snow, Kevin Wolf,
Max Reitz, Paolo Bonzini, Richard Henderson, Michael S. Tsirkin,
Scott Wood, Jason Wang, David Gibson, Gerd Hoffmann,
Alex Williamson, qemu-block, qemu-ppc, Alistair Francis,
Beniamino Galvani, Edgar E. Iglesias, Gabriel L . Somlo,
Igor Mammedov, Prasad J Pandit, qemu-arm, Shannon Zhao
commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
cannot_instantiate_with_device_add_yet in TYPE_SYSBUS, making
all kinds of untested devices available to -device and
device_add.
The problem with that is: setting has_dynamic_sysbus on a
machine-type lets it accept all the 288 sysbus device types we
have in QEMU, and most of them were never meant to be used with
-device. That's a lot of untested code.
Fortunately today we have just a few has_dynamic_sysbus=1
machines: virt, pc-q35-*, ppce500, and spapr.
virt, ppce500, and spapr have extra checks to ensure just a few
device types can be instantiated:
* virt supports only TYPE_VFIO_CALXEDA_XGMAC, TYPE_VFIO_AMD_XGBE.
* ppce500 supports only TYPE_ETSEC_COMMON.
* spapr supports only TYPE_SPAPR_PCI_HOST_BRIDGE.
q35 has no code to block unsupported sysbus devices, however, and
accepts all device types. Fortunately, only the following 20
device types are compiled into the qemu-system-x86_64 and
qemu-system-i386 binaries:
* allwinner-ahci
* amd-iommu
* cfi.pflash01
* esp
* fw_cfg_io
* fw_cfg_mem
* generic-sdhci
* hpet
* intel-iommu
* ioapic
* isabus-bridge
* kvmclock
* kvm-ioapic
* kvmvapic
* SUNW,fdtwo
* sysbus-ahci
* sysbus-fdc
* sysbus-ohci
* unimplemented-device
* virtio-mmio
Instead of requiring each machine-type with has_dynamic_sysbus=1
to implement its own mechanism to block unsupported devices, we
can use the user_creatable flag to ensure we won't let the user
plug anything that will never work.
This patch adds user_creatable=true explicitly to the
24 device types mentioned above, to keep compatibility, and set
user_creatable=false on TYPE_SYS_BUS_DEVICE.
Subsequent patches will remove user_creatable=true from the
devices that were not meant to be creatable using -device
despite being currently accepted by q35.
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Alexander Graf <agraf@suse.de>
Cc: John Snow <jsnow@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Scott Wood <scottwood@freescale.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: qemu-block@nongnu.org
Cc: qemu-ppc@nongnu.org
Cc: Alistair Francis <alistair.francis@xilinx.com>
Cc: Beniamino Galvani <b.galvani@gmail.com>
Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
Cc: Gabriel L. Somlo <somlo@cmu.edu>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: Prasad J Pandit <pjp@fedoraproject.org>
Cc: qemu-arm@nongnu.org
Cc: Shannon Zhao <zhaoshenglong@huawei.com>
Cc: Thomas Huth <thuth@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
hw/block/fdc.c | 10 ++++++++++
hw/block/pflash_cfi01.c | 5 +++++
hw/core/sysbus.c | 11 +++++++++++
hw/i386/amd_iommu.c | 5 +++++
hw/i386/intel_iommu.c | 5 +++++
hw/i386/kvm/clock.c | 5 +++++
hw/i386/kvm/ioapic.c | 5 +++++
hw/i386/kvmvapic.c | 5 +++++
hw/ide/ahci.c | 10 ++++++++++
hw/intc/ioapic.c | 5 +++++
hw/isa/isa-bus.c | 5 +++++
hw/misc/unimp.c | 5 +++++
hw/net/fsl_etsec/etsec.c | 1 +
hw/nvram/fw_cfg.c | 10 ++++++++++
hw/ppc/spapr_pci.c | 1 +
hw/scsi/esp.c | 5 +++++
hw/sd/sdhci.c | 5 +++++
hw/timer/hpet.c | 5 +++++
hw/usb/hcd-ohci.c | 5 +++++
hw/vfio/amd-xgbe.c | 1 +
hw/vfio/calxeda-xgmac.c | 1 +
hw/virtio/virtio-mmio.c | 5 +++++
22 files changed, 115 insertions(+)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index a328693d15..a06c8e358c 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2880,6 +2880,11 @@ static void sysbus_fdc_class_init(ObjectClass *klass, void *data)
dc->props = sysbus_fdc_properties;
set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+ /*
+ * FIXME: Set only for compatibility on q35 machine-type.
+ * Probably never meant to be user-creatable
+ */
+ dc->user_creatable = true;
}
static const TypeInfo sysbus_fdc_info = {
@@ -2906,6 +2911,11 @@ static void sun4m_fdc_class_init(ObjectClass *klass, void *data)
dc->props = sun4m_fdc_properties;
set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+ /*
+ * FIXME: Set only for compatibility on q35 machine-type.
+ * Probably never meant to be user-creatable
+ */
+ dc->user_creatable = true;
}
static const TypeInfo sun4m_fdc_info = {
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 594d4cf6fe..f48dc20035 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -927,6 +927,11 @@ static void pflash_cfi01_class_init(ObjectClass *klass, void *data)
dc->props = pflash_cfi01_properties;
dc->vmsd = &vmstate_pflash;
set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+ /*
+ * FIXME: Set only for compatibility on q35 machine-type.
+ * Probably never meant to be user-creatable
+ */
+ dc->user_creatable = true;
}
diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index c0f560b289..2d4121dd67 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -326,6 +326,17 @@ static void sysbus_device_class_init(ObjectClass *klass, void *data)
DeviceClass *k = DEVICE_CLASS(klass);
k->init = sysbus_device_init;
k->bus_type = TYPE_SYSTEM_BUS;
+ /*
+ * device_add plugs devices into suitable bus. For "real" buses,
+ * that actually connects the device. For sysbus, the connections
+ * need to be made separately, and device_add can't do that. The
+ * device would be left unconnected, and will probably not work
+ *
+ * However, a few machines and a few devices can handle a few sysbus
+ * devices, if the device subclass overrides user_creatable=true and
+ * the machine has has_dynamic_sysbus=1.
+ */
+ k->user_creatable = false;
}
static const TypeInfo sysbus_device_type_info = {
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index f86a40aa30..1d139f3f6a 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1186,6 +1186,11 @@ static void amdvi_class_init(ObjectClass *klass, void* data)
dc->vmsd = &vmstate_amdvi;
dc->hotpluggable = false;
dc_class->realize = amdvi_realize;
+ /*
+ * FIXME: Set only for compatibility on q35 machine-type.
+ * Probably never meant to be user-creatable
+ */
+ dc->user_creatable = true;
}
static const TypeInfo amdvi = {
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 22d8226e43..5e5125f34b 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2601,6 +2601,11 @@ static void vtd_class_init(ObjectClass *klass, void *data)
dc->hotpluggable = false;
x86_class->realize = vtd_realize;
x86_class->int_remap = vtd_int_remap;
+ /*
+ * FIXME: Set only for compatibility on q35 machine-type.
+ * Probably never meant to be user-creatable
+ */
+ dc->user_creatable = true;
}
static const TypeInfo vtd_info = {
diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
index 13eca374cd..0dbf083b17 100644
--- a/hw/i386/kvm/clock.c
+++ b/hw/i386/kvm/clock.c
@@ -286,6 +286,11 @@ static void kvmclock_class_init(ObjectClass *klass, void *data)
dc->realize = kvmclock_realize;
dc->vmsd = &kvmclock_vmsd;
dc->props = kvmclock_properties;
+ /*
+ * FIXME: Set only for compatibility on q35 machine-type.
+ * Probably never meant to be user-creatable
+ */
+ dc->user_creatable = true;
}
static const TypeInfo kvmclock_info = {
diff --git a/hw/i386/kvm/ioapic.c b/hw/i386/kvm/ioapic.c
index 98ca480792..cdc35de661 100644
--- a/hw/i386/kvm/ioapic.c
+++ b/hw/i386/kvm/ioapic.c
@@ -167,6 +167,11 @@ static void kvm_ioapic_class_init(ObjectClass *klass, void *data)
k->post_load = kvm_ioapic_put;
dc->reset = kvm_ioapic_reset;
dc->props = kvm_ioapic_properties;
+ /*
+ * FIXME: Set only for compatibility on q35 machine-type.
+ * Probably never meant to be user-creatable
+ */
+ dc->user_creatable = true;
}
static const TypeInfo kvm_ioapic_info = {
diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
index 82a49556af..fbada19253 100644
--- a/hw/i386/kvmvapic.c
+++ b/hw/i386/kvmvapic.c
@@ -856,6 +856,11 @@ static void vapic_class_init(ObjectClass *klass, void *data)
dc->reset = vapic_reset;
dc->vmsd = &vmstate_vapic;
dc->realize = vapic_realize;
+ /*
+ * FIXME: Set only for compatibility on q35 machine-type.
+ * Probably never meant to be user-creatable
+ */
+ dc->user_creatable = true;
}
static const TypeInfo vapic_type = {
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index f60826d6e0..b1b7780d56 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1721,6 +1721,11 @@ static void sysbus_ahci_class_init(ObjectClass *klass, void *data)
dc->props = sysbus_ahci_properties;
dc->reset = sysbus_ahci_reset;
set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+ /*
+ * FIXME: Set only for compatibility on q35 machine-type.
+ * Probably never meant to be user-creatable
+ */
+ dc->user_creatable = true;
}
static const TypeInfo sysbus_ahci_info = {
@@ -1815,6 +1820,11 @@ static void allwinner_ahci_class_init(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
dc->vmsd = &vmstate_allwinner_ahci;
+ /*
+ * FIXME: Set only for compatibility on q35 machine-type.
+ * Probably never meant to be user-creatable
+ */
+ dc->user_creatable = true;
}
static const TypeInfo allwinner_ahci_info = {
diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
index 37c4386ae3..38af393538 100644
--- a/hw/intc/ioapic.c
+++ b/hw/intc/ioapic.c
@@ -448,6 +448,11 @@ static void ioapic_class_init(ObjectClass *klass, void *data)
k->post_load = ioapic_update_kvm_routes;
dc->reset = ioapic_reset_common;
dc->props = ioapic_properties;
+ /*
+ * FIXME: Set only for compatibility on q35 machine-type.
+ * Probably never meant to be user-creatable
+ */
+ dc->user_creatable = true;
}
static const TypeInfo ioapic_info = {
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 348e0eab9d..576dbf1763 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -221,6 +221,11 @@ static void isabus_bridge_class_init(ObjectClass *klass, void *data)
set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
dc->fw_name = "isa";
+ /*
+ * FIXME: Set only for compatibility on q35 machine-type.
+ * Probably never meant to be user-creatable
+ */
+ dc->user_creatable = true;
}
static const TypeInfo isabus_bridge_info = {
diff --git a/hw/misc/unimp.c b/hw/misc/unimp.c
index bcbb585888..284f096beb 100644
--- a/hw/misc/unimp.c
+++ b/hw/misc/unimp.c
@@ -90,6 +90,11 @@ static void unimp_class_init(ObjectClass *klass, void *data)
dc->realize = unimp_realize;
dc->props = unimp_properties;
+ /*
+ * FIXME: Set only for compatibility on q35 machine-type.
+ * Probably never meant to be user-creatable
+ */
+ dc->user_creatable = true;
}
static const TypeInfo unimp_info = {
diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
index aa2b0d5a85..b230b88753 100644
--- a/hw/net/fsl_etsec/etsec.c
+++ b/hw/net/fsl_etsec/etsec.c
@@ -416,6 +416,7 @@ static void etsec_class_init(ObjectClass *klass, void *data)
dc->realize = etsec_realize;
dc->reset = etsec_reset;
dc->props = etsec_properties;
+ dc->user_creatable = true;
}
static TypeInfo etsec_info = {
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 316fca9bc1..60bf4fdd2e 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -1101,6 +1101,11 @@ static void fw_cfg_io_class_init(ObjectClass *klass, void *data)
dc->realize = fw_cfg_io_realize;
dc->props = fw_cfg_io_properties;
+ /*
+ * FIXME: Set only for compatibility on q35 machine-type.
+ * Probably never meant to be user-creatable
+ */
+ dc->user_creatable = true;
}
static const TypeInfo fw_cfg_io_info = {
@@ -1167,6 +1172,11 @@ static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)
dc->realize = fw_cfg_mem_realize;
dc->props = fw_cfg_mem_properties;
+ /*
+ * FIXME: Set only for compatibility on q35 machine-type.
+ * Probably never meant to be user-creatable
+ */
+ dc->user_creatable = true;
}
static const TypeInfo fw_cfg_mem_info = {
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 98c52e411f..360eeb2e40 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1990,6 +1990,7 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
dc->props = spapr_phb_properties;
dc->reset = spapr_phb_reset;
dc->vmsd = &vmstate_spapr_pci;
+ dc->user_creatable = true;
set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
hp->plug = spapr_phb_hot_plug_child;
hp->unplug = spapr_phb_hot_unplug_child;
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index eee831efeb..1087b8f14f 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -728,6 +728,11 @@ static void sysbus_esp_class_init(ObjectClass *klass, void *data)
dc->reset = sysbus_esp_hard_reset;
dc->vmsd = &vmstate_sysbus_esp_scsi;
set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+ /*
+ * FIXME: Set only for compatibility on q35 machine-type.
+ * Probably never meant to be user-creatable
+ */
+ dc->user_creatable = true;
}
static const TypeInfo sysbus_esp_info = {
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 6d6a791ee9..186555188b 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1360,6 +1360,11 @@ static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
dc->props = sdhci_sysbus_properties;
dc->realize = sdhci_sysbus_realize;
dc->reset = sdhci_poweron_reset;
+ /*
+ * FIXME: Set only for compatibility on q35 machine-type.
+ * Probably never meant to be user-creatable
+ */
+ dc->user_creatable = true;
}
static const TypeInfo sdhci_sysbus_info = {
diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index a2c18b30c3..b5dab1c8e2 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -771,6 +771,11 @@ static void hpet_device_class_init(ObjectClass *klass, void *data)
dc->reset = hpet_reset;
dc->vmsd = &vmstate_hpet;
dc->props = hpet_device_properties;
+ /*
+ * FIXME: Set only for compatibility on q35 machine-type.
+ * Probably never meant to be user-creatable
+ */
+ dc->user_creatable = true;
}
static const TypeInfo hpet_device_info = {
diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index 3ada35e954..0e17fb1e60 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -2159,6 +2159,11 @@ static void ohci_sysbus_class_init(ObjectClass *klass, void *data)
dc->desc = "OHCI USB Controller";
dc->props = ohci_sysbus_properties;
dc->reset = usb_ohci_reset_sysbus;
+ /*
+ * FIXME: Set only for compatibility on q35 machine-type.
+ * Probably never meant to be user-creatable
+ */
+ dc->user_creatable = true;
}
static const TypeInfo ohci_sysbus_info = {
diff --git a/hw/vfio/amd-xgbe.c b/hw/vfio/amd-xgbe.c
index 2c60310cf9..f0b6624530 100644
--- a/hw/vfio/amd-xgbe.c
+++ b/hw/vfio/amd-xgbe.c
@@ -38,6 +38,7 @@ static void vfio_amd_xgbe_class_init(ObjectClass *klass, void *data)
dc->realize = amd_xgbe_realize;
dc->desc = "VFIO AMD XGBE";
dc->vmsd = &vfio_platform_amd_xgbe_vmstate;
+ dc->user_creatable = true;
}
static const TypeInfo vfio_amd_xgbe_dev_info = {
diff --git a/hw/vfio/calxeda-xgmac.c b/hw/vfio/calxeda-xgmac.c
index bb15d588e5..6fae3c5772 100644
--- a/hw/vfio/calxeda-xgmac.c
+++ b/hw/vfio/calxeda-xgmac.c
@@ -38,6 +38,7 @@ static void vfio_calxeda_xgmac_class_init(ObjectClass *klass, void *data)
dc->realize = calxeda_xgmac_realize;
dc->desc = "VFIO Calxeda XGMAC";
dc->vmsd = &vfio_platform_calxeda_xgmac_vmstate;
+ dc->user_creatable = true;
}
static const TypeInfo vfio_calxeda_xgmac_dev_info = {
diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index 5807aa87fe..b595512a3d 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -450,6 +450,11 @@ static void virtio_mmio_class_init(ObjectClass *klass, void *data)
dc->reset = virtio_mmio_reset;
set_bit(DEVICE_CATEGORY_MISC, dc->categories);
dc->props = virtio_mmio_properties;
+ /*
+ * FIXME: Set only for compatibility on q35 machine-type.
+ * Probably never meant to be user-creatable
+ */
+ dc->user_creatable = true;
}
static const TypeInfo virtio_mmio_info = {
--
2.11.0.259.g40922b1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [RFC 03/19] sysbus: Set user_creatable=false by default on TYPE_SYS_BUS_DEVICE
2017-04-01 0:46 ` [Qemu-devel] [RFC 03/19] sysbus: Set user_creatable=false by default on TYPE_SYS_BUS_DEVICE Eduardo Habkost
@ 2017-04-03 19:49 ` Peter Maydell
2017-04-03 20:10 ` Eduardo Habkost
0 siblings, 1 reply; 55+ messages in thread
From: Peter Maydell @ 2017-04-03 19:49 UTC (permalink / raw)
To: Eduardo Habkost
Cc: QEMU Developers, Laszlo Ersek, Alexander Graf, Marcel Apfelbaum,
Thomas Huth, Markus Armbruster, John Snow, Kevin Wolf, Max Reitz,
Paolo Bonzini, Richard Henderson, Michael S. Tsirkin, Scott Wood,
Jason Wang, David Gibson, Gerd Hoffmann, Alex Williamson,
Qemu-block, qemu-ppc, Alistair Francis, Beniamino Galvani,
Edgar E. Iglesias, Gabriel L . Somlo, Igor Mammedov,
Prasad J Pandit, qemu-arm, Shannon Zhao
On 1 April 2017 at 01:46, Eduardo Habkost <ehabkost@redhat.com> wrote:
> commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
> cannot_instantiate_with_device_add_yet in TYPE_SYSBUS, making
> all kinds of untested devices available to -device and
> device_add.
>
> The problem with that is: setting has_dynamic_sysbus on a
> machine-type lets it accept all the 288 sysbus device types we
> have in QEMU, and most of them were never meant to be used with
> -device. That's a lot of untested code.
>
> Fortunately today we have just a few has_dynamic_sysbus=1
> machines: virt, pc-q35-*, ppce500, and spapr.
>
> virt, ppce500, and spapr have extra checks to ensure just a few
> device types can be instantiated:
>
> * virt supports only TYPE_VFIO_CALXEDA_XGMAC, TYPE_VFIO_AMD_XGBE.
> * ppce500 supports only TYPE_ETSEC_COMMON.
> * spapr supports only TYPE_SPAPR_PCI_HOST_BRIDGE.
>
> q35 has no code to block unsupported sysbus devices, however, and
> accepts all device types. Fortunately, only the following 20
> device types are compiled into the qemu-system-x86_64 and
> qemu-system-i386 binaries:
>
> * allwinner-ahci
> * amd-iommu
> * cfi.pflash01
> * esp
> * fw_cfg_io
> * fw_cfg_mem
> * generic-sdhci
> * hpet
> * intel-iommu
> * ioapic
> * isabus-bridge
> * kvmclock
> * kvm-ioapic
> * kvmvapic
> * SUNW,fdtwo
> * sysbus-ahci
> * sysbus-fdc
> * sysbus-ohci
> * unimplemented-device
> * virtio-mmio
>
> Instead of requiring each machine-type with has_dynamic_sysbus=1
> to implement its own mechanism to block unsupported devices, we
> can use the user_creatable flag to ensure we won't let the user
> plug anything that will never work.
How does this work? Which devices can be dynamically
plugged is machine dependent. You can't dynamically-plug
an intel-iommu on the ARM virt board, and you can't
dynamically-plug the vfio-calxeda-xgmac on the spapr
board, and so on. So I don't see how we can just have
a flag on the device itself that controls whether
it can be dynamically plugged.
So I'm definitely coming around to the opinion that
it's just a bug in the q35 board that it doesn't have
any device whitelisting, and we should fix that.
thanks
-- PMM
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [RFC 03/19] sysbus: Set user_creatable=false by default on TYPE_SYS_BUS_DEVICE
2017-04-03 19:49 ` Peter Maydell
@ 2017-04-03 20:10 ` Eduardo Habkost
2017-04-03 20:15 ` Alexander Graf
0 siblings, 1 reply; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-03 20:10 UTC (permalink / raw)
To: Peter Maydell
Cc: QEMU Developers, Laszlo Ersek, Alexander Graf, Marcel Apfelbaum,
Thomas Huth, Markus Armbruster, John Snow, Kevin Wolf, Max Reitz,
Paolo Bonzini, Richard Henderson, Michael S. Tsirkin, Scott Wood,
Jason Wang, David Gibson, Gerd Hoffmann, Alex Williamson,
Qemu-block, qemu-ppc, Alistair Francis, Beniamino Galvani,
Edgar E. Iglesias, Gabriel L . Somlo, Igor Mammedov,
Prasad J Pandit, qemu-arm, Shannon Zhao
On Mon, Apr 03, 2017 at 08:49:16PM +0100, Peter Maydell wrote:
> On 1 April 2017 at 01:46, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
> > cannot_instantiate_with_device_add_yet in TYPE_SYSBUS, making
> > all kinds of untested devices available to -device and
> > device_add.
> >
> > The problem with that is: setting has_dynamic_sysbus on a
> > machine-type lets it accept all the 288 sysbus device types we
> > have in QEMU, and most of them were never meant to be used with
> > -device. That's a lot of untested code.
> >
> > Fortunately today we have just a few has_dynamic_sysbus=1
> > machines: virt, pc-q35-*, ppce500, and spapr.
> >
> > virt, ppce500, and spapr have extra checks to ensure just a few
> > device types can be instantiated:
> >
> > * virt supports only TYPE_VFIO_CALXEDA_XGMAC, TYPE_VFIO_AMD_XGBE.
> > * ppce500 supports only TYPE_ETSEC_COMMON.
> > * spapr supports only TYPE_SPAPR_PCI_HOST_BRIDGE.
> >
> > q35 has no code to block unsupported sysbus devices, however, and
> > accepts all device types. Fortunately, only the following 20
> > device types are compiled into the qemu-system-x86_64 and
> > qemu-system-i386 binaries:
> >
> > * allwinner-ahci
> > * amd-iommu
> > * cfi.pflash01
> > * esp
> > * fw_cfg_io
> > * fw_cfg_mem
> > * generic-sdhci
> > * hpet
> > * intel-iommu
> > * ioapic
> > * isabus-bridge
> > * kvmclock
> > * kvm-ioapic
> > * kvmvapic
> > * SUNW,fdtwo
> > * sysbus-ahci
> > * sysbus-fdc
> > * sysbus-ohci
> > * unimplemented-device
> > * virtio-mmio
> >
> > Instead of requiring each machine-type with has_dynamic_sysbus=1
> > to implement its own mechanism to block unsupported devices, we
> > can use the user_creatable flag to ensure we won't let the user
> > plug anything that will never work.
>
> How does this work? Which devices can be dynamically
> plugged is machine dependent. You can't dynamically-plug
> an intel-iommu on the ARM virt board, and you can't
> dynamically-plug the vfio-calxeda-xgmac on the spapr
> board, and so on. So I don't see how we can just have
> a flag on the device itself that controls whether
> it can be dynamically plugged.
>
> So I'm definitely coming around to the opinion that
> it's just a bug in the q35 board that it doesn't have
> any device whitelisting, and we should fix that.
OK, let's assume q35 must implement a whitelist:
To build that whitelist, we need to be able to know what should
be in the whitelist, or not. And nobody knew for sure what was
user-creatable in q35 by accident, and what was really supposed
to be user-creatable in q35. See the "q35 and sysbus devices"
thread I started ~2 weeks ago.
Building a q35 whitelist will be much easier if make
sys-bus-devices non-user-creatable by default.
--
Eduardo
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [RFC 03/19] sysbus: Set user_creatable=false by default on TYPE_SYS_BUS_DEVICE
2017-04-03 20:10 ` Eduardo Habkost
@ 2017-04-03 20:15 ` Alexander Graf
2017-04-03 21:00 ` Eduardo Habkost
0 siblings, 1 reply; 55+ messages in thread
From: Alexander Graf @ 2017-04-03 20:15 UTC (permalink / raw)
To: Eduardo Habkost, Peter Maydell
Cc: QEMU Developers, Laszlo Ersek, Marcel Apfelbaum, Thomas Huth,
Markus Armbruster, John Snow, Kevin Wolf, Max Reitz,
Paolo Bonzini, Richard Henderson, Michael S. Tsirkin, Scott Wood,
Jason Wang, David Gibson, Gerd Hoffmann, Alex Williamson,
Qemu-block, qemu-ppc, Alistair Francis, Beniamino Galvani,
Edgar E. Iglesias, Gabriel L . Somlo, Igor Mammedov,
Prasad J Pandit, qemu-arm, Shannon Zhao
On 03.04.17 22:10, Eduardo Habkost wrote:
> On Mon, Apr 03, 2017 at 08:49:16PM +0100, Peter Maydell wrote:
>> On 1 April 2017 at 01:46, Eduardo Habkost <ehabkost@redhat.com> wrote:
>>> commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
>>> cannot_instantiate_with_device_add_yet in TYPE_SYSBUS, making
>>> all kinds of untested devices available to -device and
>>> device_add.
>>>
>>> The problem with that is: setting has_dynamic_sysbus on a
>>> machine-type lets it accept all the 288 sysbus device types we
>>> have in QEMU, and most of them were never meant to be used with
>>> -device. That's a lot of untested code.
>>>
>>> Fortunately today we have just a few has_dynamic_sysbus=1
>>> machines: virt, pc-q35-*, ppce500, and spapr.
>>>
>>> virt, ppce500, and spapr have extra checks to ensure just a few
>>> device types can be instantiated:
>>>
>>> * virt supports only TYPE_VFIO_CALXEDA_XGMAC, TYPE_VFIO_AMD_XGBE.
>>> * ppce500 supports only TYPE_ETSEC_COMMON.
>>> * spapr supports only TYPE_SPAPR_PCI_HOST_BRIDGE.
>>>
>>> q35 has no code to block unsupported sysbus devices, however, and
>>> accepts all device types. Fortunately, only the following 20
>>> device types are compiled into the qemu-system-x86_64 and
>>> qemu-system-i386 binaries:
>>>
>>> * allwinner-ahci
>>> * amd-iommu
>>> * cfi.pflash01
>>> * esp
>>> * fw_cfg_io
>>> * fw_cfg_mem
>>> * generic-sdhci
>>> * hpet
>>> * intel-iommu
>>> * ioapic
>>> * isabus-bridge
>>> * kvmclock
>>> * kvm-ioapic
>>> * kvmvapic
>>> * SUNW,fdtwo
>>> * sysbus-ahci
>>> * sysbus-fdc
>>> * sysbus-ohci
>>> * unimplemented-device
>>> * virtio-mmio
>>>
>>> Instead of requiring each machine-type with has_dynamic_sysbus=1
>>> to implement its own mechanism to block unsupported devices, we
>>> can use the user_creatable flag to ensure we won't let the user
>>> plug anything that will never work.
>>
>> How does this work? Which devices can be dynamically
>> plugged is machine dependent. You can't dynamically-plug
>> an intel-iommu on the ARM virt board, and you can't
>> dynamically-plug the vfio-calxeda-xgmac on the spapr
>> board, and so on. So I don't see how we can just have
>> a flag on the device itself that controls whether
>> it can be dynamically plugged.
>>
>> So I'm definitely coming around to the opinion that
>> it's just a bug in the q35 board that it doesn't have
>> any device whitelisting, and we should fix that.
>
> OK, let's assume q35 must implement a whitelist:
>
> To build that whitelist, we need to be able to know what should
> be in the whitelist, or not. And nobody knew for sure what was
> user-creatable in q35 by accident, and what was really supposed
> to be user-creatable in q35. See the "q35 and sysbus devices"
> thread I started ~2 weeks ago.
>
> Building a q35 whitelist will be much easier if make
> sys-bus-devices non-user-creatable by default.
So why are they user creatable in the first place?
We used to have boards that were dynamic sysbus aware (ppce500, virt)
that allowed dynamic creation and every other board did not. I don't
remember the exact mechanism behind it though.
When did that behavior change? It sounds like a regression somewhere.
Alex
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [RFC 03/19] sysbus: Set user_creatable=false by default on TYPE_SYS_BUS_DEVICE
2017-04-03 20:15 ` Alexander Graf
@ 2017-04-03 21:00 ` Eduardo Habkost
2017-04-04 6:53 ` Alexander Graf
0 siblings, 1 reply; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-03 21:00 UTC (permalink / raw)
To: Alexander Graf
Cc: Peter Maydell, QEMU Developers, Laszlo Ersek, Marcel Apfelbaum,
Thomas Huth, Markus Armbruster, John Snow, Kevin Wolf, Max Reitz,
Paolo Bonzini, Richard Henderson, Michael S. Tsirkin, Scott Wood,
Jason Wang, David Gibson, Gerd Hoffmann, Alex Williamson,
Qemu-block, qemu-ppc, Alistair Francis, Beniamino Galvani,
Edgar E. Iglesias, Gabriel L . Somlo, Igor Mammedov,
Prasad J Pandit, qemu-arm, Shannon Zhao
On Mon, Apr 03, 2017 at 10:15:44PM +0200, Alexander Graf wrote:
>
>
> On 03.04.17 22:10, Eduardo Habkost wrote:
> > On Mon, Apr 03, 2017 at 08:49:16PM +0100, Peter Maydell wrote:
> > > On 1 April 2017 at 01:46, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
> > > > cannot_instantiate_with_device_add_yet in TYPE_SYSBUS, making
> > > > all kinds of untested devices available to -device and
> > > > device_add.
> > > >
> > > > The problem with that is: setting has_dynamic_sysbus on a
> > > > machine-type lets it accept all the 288 sysbus device types we
> > > > have in QEMU, and most of them were never meant to be used with
> > > > -device. That's a lot of untested code.
> > > >
> > > > Fortunately today we have just a few has_dynamic_sysbus=1
> > > > machines: virt, pc-q35-*, ppce500, and spapr.
> > > >
> > > > virt, ppce500, and spapr have extra checks to ensure just a few
> > > > device types can be instantiated:
> > > >
> > > > * virt supports only TYPE_VFIO_CALXEDA_XGMAC, TYPE_VFIO_AMD_XGBE.
> > > > * ppce500 supports only TYPE_ETSEC_COMMON.
> > > > * spapr supports only TYPE_SPAPR_PCI_HOST_BRIDGE.
> > > >
> > > > q35 has no code to block unsupported sysbus devices, however, and
> > > > accepts all device types. Fortunately, only the following 20
> > > > device types are compiled into the qemu-system-x86_64 and
> > > > qemu-system-i386 binaries:
> > > >
> > > > * allwinner-ahci
> > > > * amd-iommu
> > > > * cfi.pflash01
> > > > * esp
> > > > * fw_cfg_io
> > > > * fw_cfg_mem
> > > > * generic-sdhci
> > > > * hpet
> > > > * intel-iommu
> > > > * ioapic
> > > > * isabus-bridge
> > > > * kvmclock
> > > > * kvm-ioapic
> > > > * kvmvapic
> > > > * SUNW,fdtwo
> > > > * sysbus-ahci
> > > > * sysbus-fdc
> > > > * sysbus-ohci
> > > > * unimplemented-device
> > > > * virtio-mmio
> > > >
> > > > Instead of requiring each machine-type with has_dynamic_sysbus=1
> > > > to implement its own mechanism to block unsupported devices, we
> > > > can use the user_creatable flag to ensure we won't let the user
> > > > plug anything that will never work.
> > >
> > > How does this work? Which devices can be dynamically
> > > plugged is machine dependent. You can't dynamically-plug
> > > an intel-iommu on the ARM virt board, and you can't
> > > dynamically-plug the vfio-calxeda-xgmac on the spapr
> > > board, and so on. So I don't see how we can just have
> > > a flag on the device itself that controls whether
> > > it can be dynamically plugged.
> > >
> > > So I'm definitely coming around to the opinion that
> > > it's just a bug in the q35 board that it doesn't have
> > > any device whitelisting, and we should fix that.
> >
> > OK, let's assume q35 must implement a whitelist:
> >
> > To build that whitelist, we need to be able to know what should
> > be in the whitelist, or not. And nobody knew for sure what was
> > user-creatable in q35 by accident, and what was really supposed
> > to be user-creatable in q35. See the "q35 and sysbus devices"
> > thread I started ~2 weeks ago.
> >
> > Building a q35 whitelist will be much easier if make
> > sys-bus-devices non-user-creatable by default.
>
> So why are they user creatable in the first place?
>
> We used to have boards that were dynamic sysbus aware (ppce500, virt) that
> allowed dynamic creation and every other board did not. I don't remember the
> exact mechanism behind it though.
>
> When did that behavior change? It sounds like a regression somewhere.
See patch description:
> > > > commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
> > > > cannot_instantiate_with_device_add_yet in TYPE_SYSBUS,
Note that the commit above is not a regression[1] (because q35
didn't have has_dynamic_sysbus=1 yet), but having sysbus devices
have cannot_instantiate_with_device_add_yet=false/user_creatable=true
by default makes it harder to build the whitelist for q35 (or
other machines that will have has_dynamic_sysbus=1 in the
future).
[1] Well, it was a regression on the "info qdm" HMP command
output, which is a minor bug. But it's still a bug we still
want to fix anyway.
--
Eduardo
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [RFC 03/19] sysbus: Set user_creatable=false by default on TYPE_SYS_BUS_DEVICE
2017-04-03 21:00 ` Eduardo Habkost
@ 2017-04-04 6:53 ` Alexander Graf
2017-04-04 6:58 ` Thomas Huth
2017-04-04 13:05 ` Eduardo Habkost
0 siblings, 2 replies; 55+ messages in thread
From: Alexander Graf @ 2017-04-04 6:53 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Peter Maydell, QEMU Developers, Laszlo Ersek, Marcel Apfelbaum,
Thomas Huth, Markus Armbruster, John Snow, Kevin Wolf, Max Reitz,
Paolo Bonzini, Richard Henderson, Michael S. Tsirkin, Scott Wood,
Jason Wang, David Gibson, Gerd Hoffmann, Alex Williamson,
Qemu-block, qemu-ppc, Alistair Francis, Beniamino Galvani,
Edgar E. Iglesias, Gabriel L . Somlo, Igor Mammedov,
Prasad J Pandit, qemu-arm, Shannon Zhao
On 03.04.17 23:00, Eduardo Habkost wrote:
> On Mon, Apr 03, 2017 at 10:15:44PM +0200, Alexander Graf wrote:
>>
>>
>> On 03.04.17 22:10, Eduardo Habkost wrote:
>>> On Mon, Apr 03, 2017 at 08:49:16PM +0100, Peter Maydell wrote:
>>>> On 1 April 2017 at 01:46, Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>>> commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
>>>>> cannot_instantiate_with_device_add_yet in TYPE_SYSBUS, making
>>>>> all kinds of untested devices available to -device and
>>>>> device_add.
>>>>>
>>>>> The problem with that is: setting has_dynamic_sysbus on a
>>>>> machine-type lets it accept all the 288 sysbus device types we
>>>>> have in QEMU, and most of them were never meant to be used with
>>>>> -device. That's a lot of untested code.
>>>>>
>>>>> Fortunately today we have just a few has_dynamic_sysbus=1
>>>>> machines: virt, pc-q35-*, ppce500, and spapr.
>>>>>
>>>>> virt, ppce500, and spapr have extra checks to ensure just a few
>>>>> device types can be instantiated:
>>>>>
>>>>> * virt supports only TYPE_VFIO_CALXEDA_XGMAC, TYPE_VFIO_AMD_XGBE.
>>>>> * ppce500 supports only TYPE_ETSEC_COMMON.
>>>>> * spapr supports only TYPE_SPAPR_PCI_HOST_BRIDGE.
>>>>>
>>>>> q35 has no code to block unsupported sysbus devices, however, and
>>>>> accepts all device types. Fortunately, only the following 20
>>>>> device types are compiled into the qemu-system-x86_64 and
>>>>> qemu-system-i386 binaries:
>>>>>
>>>>> * allwinner-ahci
>>>>> * amd-iommu
>>>>> * cfi.pflash01
>>>>> * esp
>>>>> * fw_cfg_io
>>>>> * fw_cfg_mem
>>>>> * generic-sdhci
>>>>> * hpet
>>>>> * intel-iommu
>>>>> * ioapic
>>>>> * isabus-bridge
>>>>> * kvmclock
>>>>> * kvm-ioapic
>>>>> * kvmvapic
>>>>> * SUNW,fdtwo
>>>>> * sysbus-ahci
>>>>> * sysbus-fdc
>>>>> * sysbus-ohci
>>>>> * unimplemented-device
>>>>> * virtio-mmio
>>>>>
>>>>> Instead of requiring each machine-type with has_dynamic_sysbus=1
>>>>> to implement its own mechanism to block unsupported devices, we
>>>>> can use the user_creatable flag to ensure we won't let the user
>>>>> plug anything that will never work.
>>>>
>>>> How does this work? Which devices can be dynamically
>>>> plugged is machine dependent. You can't dynamically-plug
>>>> an intel-iommu on the ARM virt board, and you can't
>>>> dynamically-plug the vfio-calxeda-xgmac on the spapr
>>>> board, and so on. So I don't see how we can just have
>>>> a flag on the device itself that controls whether
>>>> it can be dynamically plugged.
>>>>
>>>> So I'm definitely coming around to the opinion that
>>>> it's just a bug in the q35 board that it doesn't have
>>>> any device whitelisting, and we should fix that.
>>>
>>> OK, let's assume q35 must implement a whitelist:
>>>
>>> To build that whitelist, we need to be able to know what should
>>> be in the whitelist, or not. And nobody knew for sure what was
>>> user-creatable in q35 by accident, and what was really supposed
>>> to be user-creatable in q35. See the "q35 and sysbus devices"
>>> thread I started ~2 weeks ago.
>>>
>>> Building a q35 whitelist will be much easier if make
>>> sys-bus-devices non-user-creatable by default.
>>
>> So why are they user creatable in the first place?
>>
>> We used to have boards that were dynamic sysbus aware (ppce500, virt) that
>> allowed dynamic creation and every other board did not. I don't remember the
>> exact mechanism behind it though.
>>
>> When did that behavior change? It sounds like a regression somewhere.
>
> See patch description:
>
>>>>> commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
>>>>> cannot_instantiate_with_device_add_yet in TYPE_SYSBUS,
>
> Note that the commit above is not a regression[1] (because q35
> didn't have has_dynamic_sysbus=1 yet), but having sysbus devices
> have cannot_instantiate_with_device_add_yet=false/user_creatable=true
> by default makes it harder to build the whitelist for q35 (or
> other machines that will have has_dynamic_sysbus=1 in the
> future).
I seem to miss the bigger picture.
Why would anyone set has_dynamic_sysbus=1 in a board file without an
explicit whitelist? The whitelist is *not* device specific. It's board
specific, because your board needs to know how to wire up a device and
how to expose the fact that it exists to the OS.
So the real bug is that someone set has_dynamic_sysbus=1 in q35 without
implementing all of the dynamic sysbus logic, no?
Alex
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [RFC 03/19] sysbus: Set user_creatable=false by default on TYPE_SYS_BUS_DEVICE
2017-04-04 6:53 ` Alexander Graf
@ 2017-04-04 6:58 ` Thomas Huth
2017-04-04 7:02 ` Alexander Graf
2017-04-04 13:05 ` Eduardo Habkost
1 sibling, 1 reply; 55+ messages in thread
From: Thomas Huth @ 2017-04-04 6:58 UTC (permalink / raw)
To: Alexander Graf, Eduardo Habkost
Cc: Peter Maydell, QEMU Developers, Laszlo Ersek, Marcel Apfelbaum,
Markus Armbruster, John Snow, Kevin Wolf, Max Reitz,
Paolo Bonzini, Richard Henderson, Michael S. Tsirkin, Scott Wood,
Jason Wang, David Gibson, Gerd Hoffmann, Alex Williamson,
Qemu-block, qemu-ppc, Alistair Francis, Beniamino Galvani,
Edgar E. Iglesias, Gabriel L . Somlo, Igor Mammedov,
Prasad J Pandit, qemu-arm, Shannon Zhao
On 04.04.2017 08:53, Alexander Graf wrote:
>
>
> On 03.04.17 23:00, Eduardo Habkost wrote:
>> On Mon, Apr 03, 2017 at 10:15:44PM +0200, Alexander Graf wrote:
>>>
>>>
>>> On 03.04.17 22:10, Eduardo Habkost wrote:
>>>> On Mon, Apr 03, 2017 at 08:49:16PM +0100, Peter Maydell wrote:
>>>>> On 1 April 2017 at 01:46, Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>>>> commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
>>>>>> cannot_instantiate_with_device_add_yet in TYPE_SYSBUS, making
>>>>>> all kinds of untested devices available to -device and
>>>>>> device_add.
>>>>>>
>>>>>> The problem with that is: setting has_dynamic_sysbus on a
>>>>>> machine-type lets it accept all the 288 sysbus device types we
>>>>>> have in QEMU, and most of them were never meant to be used with
>>>>>> -device. That's a lot of untested code.
>>>>>>
>>>>>> Fortunately today we have just a few has_dynamic_sysbus=1
>>>>>> machines: virt, pc-q35-*, ppce500, and spapr.
>>>>>>
>>>>>> virt, ppce500, and spapr have extra checks to ensure just a few
>>>>>> device types can be instantiated:
>>>>>>
>>>>>> * virt supports only TYPE_VFIO_CALXEDA_XGMAC, TYPE_VFIO_AMD_XGBE.
>>>>>> * ppce500 supports only TYPE_ETSEC_COMMON.
>>>>>> * spapr supports only TYPE_SPAPR_PCI_HOST_BRIDGE.
>>>>>>
>>>>>> q35 has no code to block unsupported sysbus devices, however, and
>>>>>> accepts all device types. Fortunately, only the following 20
>>>>>> device types are compiled into the qemu-system-x86_64 and
>>>>>> qemu-system-i386 binaries:
>>>>>>
>>>>>> * allwinner-ahci
>>>>>> * amd-iommu
>>>>>> * cfi.pflash01
>>>>>> * esp
>>>>>> * fw_cfg_io
>>>>>> * fw_cfg_mem
>>>>>> * generic-sdhci
>>>>>> * hpet
>>>>>> * intel-iommu
>>>>>> * ioapic
>>>>>> * isabus-bridge
>>>>>> * kvmclock
>>>>>> * kvm-ioapic
>>>>>> * kvmvapic
>>>>>> * SUNW,fdtwo
>>>>>> * sysbus-ahci
>>>>>> * sysbus-fdc
>>>>>> * sysbus-ohci
>>>>>> * unimplemented-device
>>>>>> * virtio-mmio
>>>>>>
>>>>>> Instead of requiring each machine-type with has_dynamic_sysbus=1
>>>>>> to implement its own mechanism to block unsupported devices, we
>>>>>> can use the user_creatable flag to ensure we won't let the user
>>>>>> plug anything that will never work.
>>>>>
>>>>> How does this work? Which devices can be dynamically
>>>>> plugged is machine dependent. You can't dynamically-plug
>>>>> an intel-iommu on the ARM virt board, and you can't
>>>>> dynamically-plug the vfio-calxeda-xgmac on the spapr
>>>>> board, and so on. So I don't see how we can just have
>>>>> a flag on the device itself that controls whether
>>>>> it can be dynamically plugged.
>>>>>
>>>>> So I'm definitely coming around to the opinion that
>>>>> it's just a bug in the q35 board that it doesn't have
>>>>> any device whitelisting, and we should fix that.
>>>>
>>>> OK, let's assume q35 must implement a whitelist:
>>>>
>>>> To build that whitelist, we need to be able to know what should
>>>> be in the whitelist, or not. And nobody knew for sure what was
>>>> user-creatable in q35 by accident, and what was really supposed
>>>> to be user-creatable in q35. See the "q35 and sysbus devices"
>>>> thread I started ~2 weeks ago.
>>>>
>>>> Building a q35 whitelist will be much easier if make
>>>> sys-bus-devices non-user-creatable by default.
>>>
>>> So why are they user creatable in the first place?
>>>
>>> We used to have boards that were dynamic sysbus aware (ppce500, virt)
>>> that
>>> allowed dynamic creation and every other board did not. I don't
>>> remember the
>>> exact mechanism behind it though.
>>>
>>> When did that behavior change? It sounds like a regression somewhere.
>>
>> See patch description:
>>
>>>>>> commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
>>>>>> cannot_instantiate_with_device_add_yet in TYPE_SYSBUS,
>>
>> Note that the commit above is not a regression[1] (because q35
>> didn't have has_dynamic_sysbus=1 yet), but having sysbus devices
>> have cannot_instantiate_with_device_add_yet=false/user_creatable=true
>> by default makes it harder to build the whitelist for q35 (or
>> other machines that will have has_dynamic_sysbus=1 in the
>> future).
>
> I seem to miss the bigger picture.
>
> Why would anyone set has_dynamic_sysbus=1 in a board file without an
> explicit whitelist? The whitelist is *not* device specific. It's board
> specific, because your board needs to know how to wire up a device and
> how to expose the fact that it exists to the OS.
>
> So the real bug is that someone set has_dynamic_sysbus=1 in q35 without
> implementing all of the dynamic sysbus logic, no?
According to commit bf8d492405feaee2c1685b3b9d5e03228ed3e47f this was
just introduced for allowing the "intel-iommu" device, so I guess this
is the device that we want to have in a whitelist there?
Thomas
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [RFC 03/19] sysbus: Set user_creatable=false by default on TYPE_SYS_BUS_DEVICE
2017-04-04 6:58 ` Thomas Huth
@ 2017-04-04 7:02 ` Alexander Graf
2017-04-04 12:59 ` Eduardo Habkost
0 siblings, 1 reply; 55+ messages in thread
From: Alexander Graf @ 2017-04-04 7:02 UTC (permalink / raw)
To: Thomas Huth, Eduardo Habkost
Cc: Peter Maydell, QEMU Developers, Laszlo Ersek, Marcel Apfelbaum,
Markus Armbruster, John Snow, Kevin Wolf, Max Reitz,
Paolo Bonzini, Richard Henderson, Michael S. Tsirkin, Scott Wood,
Jason Wang, David Gibson, Gerd Hoffmann, Alex Williamson,
Qemu-block, qemu-ppc, Alistair Francis, Beniamino Galvani,
Edgar E. Iglesias, Gabriel L . Somlo, Igor Mammedov,
Prasad J Pandit, qemu-arm, Shannon Zhao
On 04.04.17 08:58, Thomas Huth wrote:
> On 04.04.2017 08:53, Alexander Graf wrote:
>>
>>
>> On 03.04.17 23:00, Eduardo Habkost wrote:
>>> On Mon, Apr 03, 2017 at 10:15:44PM +0200, Alexander Graf wrote:
>>>>
>>>>
>>>> On 03.04.17 22:10, Eduardo Habkost wrote:
>>>>> On Mon, Apr 03, 2017 at 08:49:16PM +0100, Peter Maydell wrote:
>>>>>> On 1 April 2017 at 01:46, Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>>>>> commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
>>>>>>> cannot_instantiate_with_device_add_yet in TYPE_SYSBUS, making
>>>>>>> all kinds of untested devices available to -device and
>>>>>>> device_add.
>>>>>>>
>>>>>>> The problem with that is: setting has_dynamic_sysbus on a
>>>>>>> machine-type lets it accept all the 288 sysbus device types we
>>>>>>> have in QEMU, and most of them were never meant to be used with
>>>>>>> -device. That's a lot of untested code.
>>>>>>>
>>>>>>> Fortunately today we have just a few has_dynamic_sysbus=1
>>>>>>> machines: virt, pc-q35-*, ppce500, and spapr.
>>>>>>>
>>>>>>> virt, ppce500, and spapr have extra checks to ensure just a few
>>>>>>> device types can be instantiated:
>>>>>>>
>>>>>>> * virt supports only TYPE_VFIO_CALXEDA_XGMAC, TYPE_VFIO_AMD_XGBE.
>>>>>>> * ppce500 supports only TYPE_ETSEC_COMMON.
>>>>>>> * spapr supports only TYPE_SPAPR_PCI_HOST_BRIDGE.
>>>>>>>
>>>>>>> q35 has no code to block unsupported sysbus devices, however, and
>>>>>>> accepts all device types. Fortunately, only the following 20
>>>>>>> device types are compiled into the qemu-system-x86_64 and
>>>>>>> qemu-system-i386 binaries:
>>>>>>>
>>>>>>> * allwinner-ahci
>>>>>>> * amd-iommu
>>>>>>> * cfi.pflash01
>>>>>>> * esp
>>>>>>> * fw_cfg_io
>>>>>>> * fw_cfg_mem
>>>>>>> * generic-sdhci
>>>>>>> * hpet
>>>>>>> * intel-iommu
>>>>>>> * ioapic
>>>>>>> * isabus-bridge
>>>>>>> * kvmclock
>>>>>>> * kvm-ioapic
>>>>>>> * kvmvapic
>>>>>>> * SUNW,fdtwo
>>>>>>> * sysbus-ahci
>>>>>>> * sysbus-fdc
>>>>>>> * sysbus-ohci
>>>>>>> * unimplemented-device
>>>>>>> * virtio-mmio
>>>>>>>
>>>>>>> Instead of requiring each machine-type with has_dynamic_sysbus=1
>>>>>>> to implement its own mechanism to block unsupported devices, we
>>>>>>> can use the user_creatable flag to ensure we won't let the user
>>>>>>> plug anything that will never work.
>>>>>>
>>>>>> How does this work? Which devices can be dynamically
>>>>>> plugged is machine dependent. You can't dynamically-plug
>>>>>> an intel-iommu on the ARM virt board, and you can't
>>>>>> dynamically-plug the vfio-calxeda-xgmac on the spapr
>>>>>> board, and so on. So I don't see how we can just have
>>>>>> a flag on the device itself that controls whether
>>>>>> it can be dynamically plugged.
>>>>>>
>>>>>> So I'm definitely coming around to the opinion that
>>>>>> it's just a bug in the q35 board that it doesn't have
>>>>>> any device whitelisting, and we should fix that.
>>>>>
>>>>> OK, let's assume q35 must implement a whitelist:
>>>>>
>>>>> To build that whitelist, we need to be able to know what should
>>>>> be in the whitelist, or not. And nobody knew for sure what was
>>>>> user-creatable in q35 by accident, and what was really supposed
>>>>> to be user-creatable in q35. See the "q35 and sysbus devices"
>>>>> thread I started ~2 weeks ago.
>>>>>
>>>>> Building a q35 whitelist will be much easier if make
>>>>> sys-bus-devices non-user-creatable by default.
>>>>
>>>> So why are they user creatable in the first place?
>>>>
>>>> We used to have boards that were dynamic sysbus aware (ppce500, virt)
>>>> that
>>>> allowed dynamic creation and every other board did not. I don't
>>>> remember the
>>>> exact mechanism behind it though.
>>>>
>>>> When did that behavior change? It sounds like a regression somewhere.
>>>
>>> See patch description:
>>>
>>>>>>> commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
>>>>>>> cannot_instantiate_with_device_add_yet in TYPE_SYSBUS,
>>>
>>> Note that the commit above is not a regression[1] (because q35
>>> didn't have has_dynamic_sysbus=1 yet), but having sysbus devices
>>> have cannot_instantiate_with_device_add_yet=false/user_creatable=true
>>> by default makes it harder to build the whitelist for q35 (or
>>> other machines that will have has_dynamic_sysbus=1 in the
>>> future).
>>
>> I seem to miss the bigger picture.
>>
>> Why would anyone set has_dynamic_sysbus=1 in a board file without an
>> explicit whitelist? The whitelist is *not* device specific. It's board
>> specific, because your board needs to know how to wire up a device and
>> how to expose the fact that it exists to the OS.
>>
>> So the real bug is that someone set has_dynamic_sysbus=1 in q35 without
>> implementing all of the dynamic sysbus logic, no?
>
> According to commit bf8d492405feaee2c1685b3b9d5e03228ed3e47f this was
> just introduced for allowing the "intel-iommu" device, so I guess this
> is the device that we want to have in a whitelist there?
If you want a dynamic intel-iommu, you also need to have dynamic ACPI
generation to create DMAR tables the OS can use to drive the IOMMU, no?
At last at that point, someone should have realized that a "whitelist"
is mandatory.
But yes, please just only add a whitelist for intel-iommu to the q35
board. That is the real bug.
The basic idea behind dynamic sysbus is that you move the responsibility
of sysbus spawnability checks from the sysbus layer to the board file.
If the board file ignores to do those checks, it's at fault.
Alex
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [RFC 03/19] sysbus: Set user_creatable=false by default on TYPE_SYS_BUS_DEVICE
2017-04-04 7:02 ` Alexander Graf
@ 2017-04-04 12:59 ` Eduardo Habkost
2017-04-04 13:06 ` Alexander Graf
0 siblings, 1 reply; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-04 12:59 UTC (permalink / raw)
To: Alexander Graf
Cc: Thomas Huth, Peter Maydell, QEMU Developers, Laszlo Ersek,
Marcel Apfelbaum, Markus Armbruster, John Snow, Kevin Wolf,
Max Reitz, Paolo Bonzini, Richard Henderson, Michael S. Tsirkin,
Scott Wood, Jason Wang, David Gibson, Gerd Hoffmann,
Alex Williamson, Qemu-block, qemu-ppc, Alistair Francis,
Beniamino Galvani, Edgar E. Iglesias, Gabriel L . Somlo,
Igor Mammedov, Prasad J Pandit, qemu-arm, Shannon Zhao
On Tue, Apr 04, 2017 at 09:02:28AM +0200, Alexander Graf wrote:
>
>
> On 04.04.17 08:58, Thomas Huth wrote:
> > On 04.04.2017 08:53, Alexander Graf wrote:
> > >
> > >
> > > On 03.04.17 23:00, Eduardo Habkost wrote:
> > > > On Mon, Apr 03, 2017 at 10:15:44PM +0200, Alexander Graf wrote:
> > > > >
> > > > >
> > > > > On 03.04.17 22:10, Eduardo Habkost wrote:
> > > > > > On Mon, Apr 03, 2017 at 08:49:16PM +0100, Peter Maydell wrote:
> > > > > > > On 1 April 2017 at 01:46, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > > > > > commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
> > > > > > > > cannot_instantiate_with_device_add_yet in TYPE_SYSBUS, making
> > > > > > > > all kinds of untested devices available to -device and
> > > > > > > > device_add.
> > > > > > > >
> > > > > > > > The problem with that is: setting has_dynamic_sysbus on a
> > > > > > > > machine-type lets it accept all the 288 sysbus device types we
> > > > > > > > have in QEMU, and most of them were never meant to be used with
> > > > > > > > -device. That's a lot of untested code.
> > > > > > > >
> > > > > > > > Fortunately today we have just a few has_dynamic_sysbus=1
> > > > > > > > machines: virt, pc-q35-*, ppce500, and spapr.
> > > > > > > >
> > > > > > > > virt, ppce500, and spapr have extra checks to ensure just a few
> > > > > > > > device types can be instantiated:
> > > > > > > >
> > > > > > > > * virt supports only TYPE_VFIO_CALXEDA_XGMAC, TYPE_VFIO_AMD_XGBE.
> > > > > > > > * ppce500 supports only TYPE_ETSEC_COMMON.
> > > > > > > > * spapr supports only TYPE_SPAPR_PCI_HOST_BRIDGE.
> > > > > > > >
> > > > > > > > q35 has no code to block unsupported sysbus devices, however, and
> > > > > > > > accepts all device types. Fortunately, only the following 20
> > > > > > > > device types are compiled into the qemu-system-x86_64 and
> > > > > > > > qemu-system-i386 binaries:
> > > > > > > >
> > > > > > > > * allwinner-ahci
> > > > > > > > * amd-iommu
> > > > > > > > * cfi.pflash01
> > > > > > > > * esp
> > > > > > > > * fw_cfg_io
> > > > > > > > * fw_cfg_mem
> > > > > > > > * generic-sdhci
> > > > > > > > * hpet
> > > > > > > > * intel-iommu
> > > > > > > > * ioapic
> > > > > > > > * isabus-bridge
> > > > > > > > * kvmclock
> > > > > > > > * kvm-ioapic
> > > > > > > > * kvmvapic
> > > > > > > > * SUNW,fdtwo
> > > > > > > > * sysbus-ahci
> > > > > > > > * sysbus-fdc
> > > > > > > > * sysbus-ohci
> > > > > > > > * unimplemented-device
> > > > > > > > * virtio-mmio
> > > > > > > >
> > > > > > > > Instead of requiring each machine-type with has_dynamic_sysbus=1
> > > > > > > > to implement its own mechanism to block unsupported devices, we
> > > > > > > > can use the user_creatable flag to ensure we won't let the user
> > > > > > > > plug anything that will never work.
> > > > > > >
> > > > > > > How does this work? Which devices can be dynamically
> > > > > > > plugged is machine dependent. You can't dynamically-plug
> > > > > > > an intel-iommu on the ARM virt board, and you can't
> > > > > > > dynamically-plug the vfio-calxeda-xgmac on the spapr
> > > > > > > board, and so on. So I don't see how we can just have
> > > > > > > a flag on the device itself that controls whether
> > > > > > > it can be dynamically plugged.
> > > > > > >
> > > > > > > So I'm definitely coming around to the opinion that
> > > > > > > it's just a bug in the q35 board that it doesn't have
> > > > > > > any device whitelisting, and we should fix that.
> > > > > >
> > > > > > OK, let's assume q35 must implement a whitelist:
> > > > > >
> > > > > > To build that whitelist, we need to be able to know what should
> > > > > > be in the whitelist, or not. And nobody knew for sure what was
> > > > > > user-creatable in q35 by accident, and what was really supposed
> > > > > > to be user-creatable in q35. See the "q35 and sysbus devices"
> > > > > > thread I started ~2 weeks ago.
> > > > > >
> > > > > > Building a q35 whitelist will be much easier if make
> > > > > > sys-bus-devices non-user-creatable by default.
> > > > >
> > > > > So why are they user creatable in the first place?
> > > > >
> > > > > We used to have boards that were dynamic sysbus aware (ppce500, virt)
> > > > > that
> > > > > allowed dynamic creation and every other board did not. I don't
> > > > > remember the
> > > > > exact mechanism behind it though.
> > > > >
> > > > > When did that behavior change? It sounds like a regression somewhere.
> > > >
> > > > See patch description:
> > > >
> > > > > > > > commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
> > > > > > > > cannot_instantiate_with_device_add_yet in TYPE_SYSBUS,
> > > >
> > > > Note that the commit above is not a regression[1] (because q35
> > > > didn't have has_dynamic_sysbus=1 yet), but having sysbus devices
> > > > have cannot_instantiate_with_device_add_yet=false/user_creatable=true
> > > > by default makes it harder to build the whitelist for q35 (or
> > > > other machines that will have has_dynamic_sysbus=1 in the
> > > > future).
> > >
> > > I seem to miss the bigger picture.
> > >
> > > Why would anyone set has_dynamic_sysbus=1 in a board file without an
> > > explicit whitelist? The whitelist is *not* device specific. It's board
> > > specific, because your board needs to know how to wire up a device and
> > > how to expose the fact that it exists to the OS.
> > >
> > > So the real bug is that someone set has_dynamic_sysbus=1 in q35 without
> > > implementing all of the dynamic sysbus logic, no?
> >
> > According to commit bf8d492405feaee2c1685b3b9d5e03228ed3e47f this was
> > just introduced for allowing the "intel-iommu" device, so I guess this
> > is the device that we want to have in a whitelist there?
>
> If you want a dynamic intel-iommu, you also need to have dynamic ACPI
> generation to create DMAR tables the OS can use to drive the IOMMU, no? At
> last at that point, someone should have realized that a "whitelist" is
> mandatory.
>
> But yes, please just only add a whitelist for intel-iommu to the q35 board.
> That is the real bug.
Look, I don't disagree that we need a whitelist on q35. But I
don't know why you assume it is as simple as adding intel-iommu.
We *don't know* what should be on q35 whitelist until we review
each device that is accepted by q35 today, and make sure it
really is not supposed to be supported on -device.
Making has_dynamic_sysbus/user_creatable consistent on sysbus
devices helps on that. It is not necessary nor sufficient to fix
q35, that's true, but it helps *a lot*.
See, after I start this series, I already found two exceptions to
the "just add intel-iommu" rule:
1) amd-iommu
2) xen-backend
I'm not sure yet if we have others, until people review the other
"Remove user_creatable from <device>" patches in this
series.
> The basic idea behind dynamic sysbus is that you move the responsibility of
> sysbus spawnability checks from the sysbus layer to the board file. If the
> board file ignores to do those checks, it's at fault.
I think I will do this:
I will submit v2 of this thread pretending it is just going to
fix the "info qdm" regression introduced by commit
bf8d492405feaee2c1685b3b9d5e03228ed3e47f, and remove any mention
of the q35 bug from the series and patch description.
I hopt this will make us stop diverging the discussion to "you
should add a whitelist to q35 first", and stop ignoring that:
1) cannot_instantiate_with_device_add_yet is being set
incorrectly on the sysbus device classes and that's a bad
idea;
2) cannot_instantiate_with_device_add_yet is an awful name.
--
Eduardo
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [RFC 03/19] sysbus: Set user_creatable=false by default on TYPE_SYS_BUS_DEVICE
2017-04-04 12:59 ` Eduardo Habkost
@ 2017-04-04 13:06 ` Alexander Graf
2017-04-04 14:44 ` Eduardo Habkost
0 siblings, 1 reply; 55+ messages in thread
From: Alexander Graf @ 2017-04-04 13:06 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Thomas Huth, Peter Maydell, QEMU Developers, Laszlo Ersek,
Marcel Apfelbaum, Markus Armbruster, John Snow, Kevin Wolf,
Max Reitz, Paolo Bonzini, Richard Henderson, Michael S. Tsirkin,
Scott Wood, Jason Wang, David Gibson, Gerd Hoffmann,
Alex Williamson, Qemu-block, qemu-ppc, Alistair Francis,
Beniamino Galvani, Edgar E. Iglesias, Gabriel L . Somlo,
Igor Mammedov, Prasad J Pandit, qemu-arm, Shannon Zhao
On 04/04/2017 02:59 PM, Eduardo Habkost wrote:
> On Tue, Apr 04, 2017 at 09:02:28AM +0200, Alexander Graf wrote:
>>
>> On 04.04.17 08:58, Thomas Huth wrote:
>>> On 04.04.2017 08:53, Alexander Graf wrote:
>>>>
>>>> On 03.04.17 23:00, Eduardo Habkost wrote:
>>>>> On Mon, Apr 03, 2017 at 10:15:44PM +0200, Alexander Graf wrote:
>>>>>>
>>>>>> On 03.04.17 22:10, Eduardo Habkost wrote:
>>>>>>> On Mon, Apr 03, 2017 at 08:49:16PM +0100, Peter Maydell wrote:
>>>>>>>> On 1 April 2017 at 01:46, Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>>>>>>> commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
>>>>>>>>> cannot_instantiate_with_device_add_yet in TYPE_SYSBUS, making
>>>>>>>>> all kinds of untested devices available to -device and
>>>>>>>>> device_add.
>>>>>>>>>
>>>>>>>>> The problem with that is: setting has_dynamic_sysbus on a
>>>>>>>>> machine-type lets it accept all the 288 sysbus device types we
>>>>>>>>> have in QEMU, and most of them were never meant to be used with
>>>>>>>>> -device. That's a lot of untested code.
>>>>>>>>>
>>>>>>>>> Fortunately today we have just a few has_dynamic_sysbus=1
>>>>>>>>> machines: virt, pc-q35-*, ppce500, and spapr.
>>>>>>>>>
>>>>>>>>> virt, ppce500, and spapr have extra checks to ensure just a few
>>>>>>>>> device types can be instantiated:
>>>>>>>>>
>>>>>>>>> * virt supports only TYPE_VFIO_CALXEDA_XGMAC, TYPE_VFIO_AMD_XGBE.
>>>>>>>>> * ppce500 supports only TYPE_ETSEC_COMMON.
>>>>>>>>> * spapr supports only TYPE_SPAPR_PCI_HOST_BRIDGE.
>>>>>>>>>
>>>>>>>>> q35 has no code to block unsupported sysbus devices, however, and
>>>>>>>>> accepts all device types. Fortunately, only the following 20
>>>>>>>>> device types are compiled into the qemu-system-x86_64 and
>>>>>>>>> qemu-system-i386 binaries:
>>>>>>>>>
>>>>>>>>> * allwinner-ahci
>>>>>>>>> * amd-iommu
>>>>>>>>> * cfi.pflash01
>>>>>>>>> * esp
>>>>>>>>> * fw_cfg_io
>>>>>>>>> * fw_cfg_mem
>>>>>>>>> * generic-sdhci
>>>>>>>>> * hpet
>>>>>>>>> * intel-iommu
>>>>>>>>> * ioapic
>>>>>>>>> * isabus-bridge
>>>>>>>>> * kvmclock
>>>>>>>>> * kvm-ioapic
>>>>>>>>> * kvmvapic
>>>>>>>>> * SUNW,fdtwo
>>>>>>>>> * sysbus-ahci
>>>>>>>>> * sysbus-fdc
>>>>>>>>> * sysbus-ohci
>>>>>>>>> * unimplemented-device
>>>>>>>>> * virtio-mmio
>>>>>>>>>
>>>>>>>>> Instead of requiring each machine-type with has_dynamic_sysbus=1
>>>>>>>>> to implement its own mechanism to block unsupported devices, we
>>>>>>>>> can use the user_creatable flag to ensure we won't let the user
>>>>>>>>> plug anything that will never work.
>>>>>>>> How does this work? Which devices can be dynamically
>>>>>>>> plugged is machine dependent. You can't dynamically-plug
>>>>>>>> an intel-iommu on the ARM virt board, and you can't
>>>>>>>> dynamically-plug the vfio-calxeda-xgmac on the spapr
>>>>>>>> board, and so on. So I don't see how we can just have
>>>>>>>> a flag on the device itself that controls whether
>>>>>>>> it can be dynamically plugged.
>>>>>>>>
>>>>>>>> So I'm definitely coming around to the opinion that
>>>>>>>> it's just a bug in the q35 board that it doesn't have
>>>>>>>> any device whitelisting, and we should fix that.
>>>>>>> OK, let's assume q35 must implement a whitelist:
>>>>>>>
>>>>>>> To build that whitelist, we need to be able to know what should
>>>>>>> be in the whitelist, or not. And nobody knew for sure what was
>>>>>>> user-creatable in q35 by accident, and what was really supposed
>>>>>>> to be user-creatable in q35. See the "q35 and sysbus devices"
>>>>>>> thread I started ~2 weeks ago.
>>>>>>>
>>>>>>> Building a q35 whitelist will be much easier if make
>>>>>>> sys-bus-devices non-user-creatable by default.
>>>>>> So why are they user creatable in the first place?
>>>>>>
>>>>>> We used to have boards that were dynamic sysbus aware (ppce500, virt)
>>>>>> that
>>>>>> allowed dynamic creation and every other board did not. I don't
>>>>>> remember the
>>>>>> exact mechanism behind it though.
>>>>>>
>>>>>> When did that behavior change? It sounds like a regression somewhere.
>>>>> See patch description:
>>>>>
>>>>>>>>> commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
>>>>>>>>> cannot_instantiate_with_device_add_yet in TYPE_SYSBUS,
>>>>> Note that the commit above is not a regression[1] (because q35
>>>>> didn't have has_dynamic_sysbus=1 yet), but having sysbus devices
>>>>> have cannot_instantiate_with_device_add_yet=false/user_creatable=true
>>>>> by default makes it harder to build the whitelist for q35 (or
>>>>> other machines that will have has_dynamic_sysbus=1 in the
>>>>> future).
>>>> I seem to miss the bigger picture.
>>>>
>>>> Why would anyone set has_dynamic_sysbus=1 in a board file without an
>>>> explicit whitelist? The whitelist is *not* device specific. It's board
>>>> specific, because your board needs to know how to wire up a device and
>>>> how to expose the fact that it exists to the OS.
>>>>
>>>> So the real bug is that someone set has_dynamic_sysbus=1 in q35 without
>>>> implementing all of the dynamic sysbus logic, no?
>>> According to commit bf8d492405feaee2c1685b3b9d5e03228ed3e47f this was
>>> just introduced for allowing the "intel-iommu" device, so I guess this
>>> is the device that we want to have in a whitelist there?
>> If you want a dynamic intel-iommu, you also need to have dynamic ACPI
>> generation to create DMAR tables the OS can use to drive the IOMMU, no? At
>> last at that point, someone should have realized that a "whitelist" is
>> mandatory.
>>
>> But yes, please just only add a whitelist for intel-iommu to the q35 board.
>> That is the real bug.
> Look, I don't disagree that we need a whitelist on q35. But I
> don't know why you assume it is as simple as adding intel-iommu.
So why has intel-iommu worked in the first place? Who wired up its
memory regions? Who wired up fault IRQs? Who added the DMAR table to
ACPI when it was created?
> We *don't know* what should be on q35 whitelist until we review
> each device that is accepted by q35 today, and make sure it
> really is not supposed to be supported on -device.
If in doubt, not a single sysbus device should have ever worked without
explicit code around it. Really :).
> Making has_dynamic_sysbus/user_creatable consistent on sysbus
> devices helps on that. It is not necessary nor sufficient to fix
> q35, that's true, but it helps *a lot*.
>
> See, after I start this series, I already found two exceptions to
> the "just add intel-iommu" rule:
>
> 1) amd-iommu
> 2) xen-backend
>
> I'm not sure yet if we have others, until people review the other
> "Remove user_creatable from <device>" patches in this
> series.
Same question as above there. How do they get mapped? How does the OS
learn they exist?
>
>> The basic idea behind dynamic sysbus is that you move the responsibility of
>> sysbus spawnability checks from the sysbus layer to the board file. If the
>> board file ignores to do those checks, it's at fault.
> I think I will do this:
>
> I will submit v2 of this thread pretending it is just going to
> fix the "info qdm" regression introduced by commit
> bf8d492405feaee2c1685b3b9d5e03228ed3e47f, and remove any mention
> of the q35 bug from the series and patch description.
>
> I hopt this will make us stop diverging the discussion to "you
> should add a whitelist to q35 first", and stop ignoring that:
> 1) cannot_instantiate_with_device_add_yet is being set
> incorrectly on the sysbus device classes and that's a bad
> idea;
> 2) cannot_instantiate_with_device_add_yet is an awful name.
We need to make sure that the board has control over which devices are
spawnable. I fail to see how this series helps or achieves that, but I'm
happy to learn.
Alex
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [RFC 03/19] sysbus: Set user_creatable=false by default on TYPE_SYS_BUS_DEVICE
2017-04-04 13:06 ` Alexander Graf
@ 2017-04-04 14:44 ` Eduardo Habkost
0 siblings, 0 replies; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-04 14:44 UTC (permalink / raw)
To: Alexander Graf
Cc: Thomas Huth, Peter Maydell, QEMU Developers, Laszlo Ersek,
Marcel Apfelbaum, Markus Armbruster, John Snow, Kevin Wolf,
Max Reitz, Paolo Bonzini, Richard Henderson, Michael S. Tsirkin,
Scott Wood, Jason Wang, David Gibson, Gerd Hoffmann,
Alex Williamson, Qemu-block, qemu-ppc, Alistair Francis,
Beniamino Galvani, Edgar E. Iglesias, Gabriel L . Somlo,
Igor Mammedov, Prasad J Pandit, qemu-arm, Shannon Zhao
On Tue, Apr 04, 2017 at 03:06:30PM +0200, Alexander Graf wrote:
> On 04/04/2017 02:59 PM, Eduardo Habkost wrote:
> > On Tue, Apr 04, 2017 at 09:02:28AM +0200, Alexander Graf wrote:
> > >
> > > On 04.04.17 08:58, Thomas Huth wrote:
> > > > On 04.04.2017 08:53, Alexander Graf wrote:
> > > > >
> > > > > On 03.04.17 23:00, Eduardo Habkost wrote:
> > > > > > On Mon, Apr 03, 2017 at 10:15:44PM +0200, Alexander Graf wrote:
> > > > > > >
> > > > > > > On 03.04.17 22:10, Eduardo Habkost wrote:
> > > > > > > > On Mon, Apr 03, 2017 at 08:49:16PM +0100, Peter Maydell wrote:
> > > > > > > > > On 1 April 2017 at 01:46, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > > > > > > > commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
> > > > > > > > > > cannot_instantiate_with_device_add_yet in TYPE_SYSBUS, making
> > > > > > > > > > all kinds of untested devices available to -device and
> > > > > > > > > > device_add.
> > > > > > > > > >
> > > > > > > > > > The problem with that is: setting has_dynamic_sysbus on a
> > > > > > > > > > machine-type lets it accept all the 288 sysbus device types we
> > > > > > > > > > have in QEMU, and most of them were never meant to be used with
> > > > > > > > > > -device. That's a lot of untested code.
> > > > > > > > > >
> > > > > > > > > > Fortunately today we have just a few has_dynamic_sysbus=1
> > > > > > > > > > machines: virt, pc-q35-*, ppce500, and spapr.
> > > > > > > > > >
> > > > > > > > > > virt, ppce500, and spapr have extra checks to ensure just a few
> > > > > > > > > > device types can be instantiated:
> > > > > > > > > >
> > > > > > > > > > * virt supports only TYPE_VFIO_CALXEDA_XGMAC, TYPE_VFIO_AMD_XGBE.
> > > > > > > > > > * ppce500 supports only TYPE_ETSEC_COMMON.
> > > > > > > > > > * spapr supports only TYPE_SPAPR_PCI_HOST_BRIDGE.
> > > > > > > > > >
> > > > > > > > > > q35 has no code to block unsupported sysbus devices, however, and
> > > > > > > > > > accepts all device types. Fortunately, only the following 20
> > > > > > > > > > device types are compiled into the qemu-system-x86_64 and
> > > > > > > > > > qemu-system-i386 binaries:
> > > > > > > > > >
> > > > > > > > > > * allwinner-ahci
> > > > > > > > > > * amd-iommu
> > > > > > > > > > * cfi.pflash01
> > > > > > > > > > * esp
> > > > > > > > > > * fw_cfg_io
> > > > > > > > > > * fw_cfg_mem
> > > > > > > > > > * generic-sdhci
> > > > > > > > > > * hpet
> > > > > > > > > > * intel-iommu
> > > > > > > > > > * ioapic
> > > > > > > > > > * isabus-bridge
> > > > > > > > > > * kvmclock
> > > > > > > > > > * kvm-ioapic
> > > > > > > > > > * kvmvapic
> > > > > > > > > > * SUNW,fdtwo
> > > > > > > > > > * sysbus-ahci
> > > > > > > > > > * sysbus-fdc
> > > > > > > > > > * sysbus-ohci
> > > > > > > > > > * unimplemented-device
> > > > > > > > > > * virtio-mmio
> > > > > > > > > >
> > > > > > > > > > Instead of requiring each machine-type with has_dynamic_sysbus=1
> > > > > > > > > > to implement its own mechanism to block unsupported devices, we
> > > > > > > > > > can use the user_creatable flag to ensure we won't let the user
> > > > > > > > > > plug anything that will never work.
> > > > > > > > > How does this work? Which devices can be dynamically
> > > > > > > > > plugged is machine dependent. You can't dynamically-plug
> > > > > > > > > an intel-iommu on the ARM virt board, and you can't
> > > > > > > > > dynamically-plug the vfio-calxeda-xgmac on the spapr
> > > > > > > > > board, and so on. So I don't see how we can just have
> > > > > > > > > a flag on the device itself that controls whether
> > > > > > > > > it can be dynamically plugged.
> > > > > > > > >
> > > > > > > > > So I'm definitely coming around to the opinion that
> > > > > > > > > it's just a bug in the q35 board that it doesn't have
> > > > > > > > > any device whitelisting, and we should fix that.
> > > > > > > > OK, let's assume q35 must implement a whitelist:
> > > > > > > >
> > > > > > > > To build that whitelist, we need to be able to know what should
> > > > > > > > be in the whitelist, or not. And nobody knew for sure what was
> > > > > > > > user-creatable in q35 by accident, and what was really supposed
> > > > > > > > to be user-creatable in q35. See the "q35 and sysbus devices"
> > > > > > > > thread I started ~2 weeks ago.
> > > > > > > >
> > > > > > > > Building a q35 whitelist will be much easier if make
> > > > > > > > sys-bus-devices non-user-creatable by default.
> > > > > > > So why are they user creatable in the first place?
> > > > > > >
> > > > > > > We used to have boards that were dynamic sysbus aware (ppce500, virt)
> > > > > > > that
> > > > > > > allowed dynamic creation and every other board did not. I don't
> > > > > > > remember the
> > > > > > > exact mechanism behind it though.
> > > > > > >
> > > > > > > When did that behavior change? It sounds like a regression somewhere.
> > > > > > See patch description:
> > > > > >
> > > > > > > > > > commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
> > > > > > > > > > cannot_instantiate_with_device_add_yet in TYPE_SYSBUS,
> > > > > > Note that the commit above is not a regression[1] (because q35
> > > > > > didn't have has_dynamic_sysbus=1 yet), but having sysbus devices
> > > > > > have cannot_instantiate_with_device_add_yet=false/user_creatable=true
> > > > > > by default makes it harder to build the whitelist for q35 (or
> > > > > > other machines that will have has_dynamic_sysbus=1 in the
> > > > > > future).
> > > > > I seem to miss the bigger picture.
> > > > >
> > > > > Why would anyone set has_dynamic_sysbus=1 in a board file without an
> > > > > explicit whitelist? The whitelist is *not* device specific. It's board
> > > > > specific, because your board needs to know how to wire up a device and
> > > > > how to expose the fact that it exists to the OS.
> > > > >
> > > > > So the real bug is that someone set has_dynamic_sysbus=1 in q35 without
> > > > > implementing all of the dynamic sysbus logic, no?
> > > > According to commit bf8d492405feaee2c1685b3b9d5e03228ed3e47f this was
> > > > just introduced for allowing the "intel-iommu" device, so I guess this
> > > > is the device that we want to have in a whitelist there?
> > > If you want a dynamic intel-iommu, you also need to have dynamic ACPI
> > > generation to create DMAR tables the OS can use to drive the IOMMU, no? At
> > > last at that point, someone should have realized that a "whitelist" is
> > > mandatory.
> > >
> > > But yes, please just only add a whitelist for intel-iommu to the q35 board.
> > > That is the real bug.
> > Look, I don't disagree that we need a whitelist on q35. But I
> > don't know why you assume it is as simple as adding intel-iommu.
>
> So why has intel-iommu worked in the first place?
Most of the code required to make it work is simply triggered by
its realize() method (that only works with TYPE_PC_MACHINE
machines).
> Who wired up its memory regions?
Its realize() method (vtd_realize()).
> Who wired up fault IRQs?
I don't know the answer for that, but probably it is at the
apic_get_class()->send_msi() call at vtd_generate_interrupt().
> Who added the DMAR table to ACPI when it was created?
acpi-build.c code calls calls x86_iommu_get_default(), which
returns the value set by x86_iommu_set_default(), which is called
by TYPE_X86_IOMMU_DEVICE's realize() method
(x86_iommu_realize()).
>
> > We *don't know* what should be on q35 whitelist until we review
> > each device that is accepted by q35 today, and make sure it
> > really is not supposed to be supported on -device.
>
> If in doubt, not a single sysbus device should have ever worked without
> explicit code around it. Really :).
If by "code around it", you mean something as clear as a
foreach_dynamic_sysbus_device() call somewhere that handles each
type of sysbus device, then that's not true in practice.
If by "code around it" you mean code buried inside board-specific
code (like the x86_iommu_get_default() calls inside
hw/i386/acpi-build.c), then you're right.
(Don't ask me why it works that way, I didn't write that code, I
am just trying to clean up the q35 mess. :) )
>
> > Making has_dynamic_sysbus/user_creatable consistent on sysbus
> > devices helps on that. It is not necessary nor sufficient to fix
> > q35, that's true, but it helps *a lot*.
> >
> > See, after I start this series, I already found two exceptions to
> > the "just add intel-iommu" rule:
> >
> > 1) amd-iommu
> > 2) xen-backend
> >
> > I'm not sure yet if we have others, until people review the other
> > "Remove user_creatable from <device>" patches in this
> > series.
>
> Same question as above there. How do they get mapped? How does the OS learn
> they exist?
In the case of *-iommu, the realize() method seems to trigger
everything we need. In the case of xen-backend, I guess the Xen
magic is triggered by the xen_backend.c code.
Some cases may be more subtle, like kvmclock:
kvmclock _could_ work using -device already, because it simply
provides a few MSRs to the guest. The only thing that let me
conclude that -device kvmclock is not useful is that it doesn't
set the kvmclock CPUID bits in the VCPU.
If kvmclock added the CPUID bits from its realize() method,
-device kvmclock would already working out of the box and I
wouldn't be able to remove it from the whitelist.
> >
> > > The basic idea behind dynamic sysbus is that you move the responsibility of
> > > sysbus spawnability checks from the sysbus layer to the board file. If the
> > > board file ignores to do those checks, it's at fault.
> > I think I will do this:
> >
> > I will submit v2 of this thread pretending it is just going to
> > fix the "info qdm" regression introduced by commit
> > bf8d492405feaee2c1685b3b9d5e03228ed3e47f, and remove any mention
> > of the q35 bug from the series and patch description.
> >
> > I hopt this will make us stop diverging the discussion to "you
> > should add a whitelist to q35 first", and stop ignoring that:
> > 1) cannot_instantiate_with_device_add_yet is being set
> > incorrectly on the sysbus device classes and that's a bad
> > idea;
> > 2) cannot_instantiate_with_device_add_yet is an awful name.
>
>
> We need to make sure that the board has control over which devices are
> spawnable. I fail to see how this series helps or achieves that, but I'm
> happy to learn.
The series doesn't help the board have control over which devices
are spawnable. In addition to fixing (1) and (2) above, the
series helps us (humans) reduce the list of candidates for the
q35 whitelist we need to build.
--
Eduardo
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [RFC 03/19] sysbus: Set user_creatable=false by default on TYPE_SYS_BUS_DEVICE
2017-04-04 6:53 ` Alexander Graf
2017-04-04 6:58 ` Thomas Huth
@ 2017-04-04 13:05 ` Eduardo Habkost
1 sibling, 0 replies; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-04 13:05 UTC (permalink / raw)
To: Alexander Graf
Cc: Peter Maydell, QEMU Developers, Laszlo Ersek, Marcel Apfelbaum,
Thomas Huth, Markus Armbruster, John Snow, Kevin Wolf, Max Reitz,
Paolo Bonzini, Richard Henderson, Michael S. Tsirkin, Scott Wood,
Jason Wang, David Gibson, Gerd Hoffmann, Alex Williamson,
Qemu-block, qemu-ppc, Alistair Francis, Beniamino Galvani,
Edgar E. Iglesias, Gabriel L . Somlo, Igor Mammedov,
Prasad J Pandit, qemu-arm, Shannon Zhao
On Tue, Apr 04, 2017 at 08:53:43AM +0200, Alexander Graf wrote:
>
>
> On 03.04.17 23:00, Eduardo Habkost wrote:
> > On Mon, Apr 03, 2017 at 10:15:44PM +0200, Alexander Graf wrote:
> > >
> > >
> > > On 03.04.17 22:10, Eduardo Habkost wrote:
> > > > On Mon, Apr 03, 2017 at 08:49:16PM +0100, Peter Maydell wrote:
> > > > > On 1 April 2017 at 01:46, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > > > commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
> > > > > > cannot_instantiate_with_device_add_yet in TYPE_SYSBUS, making
> > > > > > all kinds of untested devices available to -device and
> > > > > > device_add.
> > > > > >
> > > > > > The problem with that is: setting has_dynamic_sysbus on a
> > > > > > machine-type lets it accept all the 288 sysbus device types we
> > > > > > have in QEMU, and most of them were never meant to be used with
> > > > > > -device. That's a lot of untested code.
> > > > > >
> > > > > > Fortunately today we have just a few has_dynamic_sysbus=1
> > > > > > machines: virt, pc-q35-*, ppce500, and spapr.
> > > > > >
> > > > > > virt, ppce500, and spapr have extra checks to ensure just a few
> > > > > > device types can be instantiated:
> > > > > >
> > > > > > * virt supports only TYPE_VFIO_CALXEDA_XGMAC, TYPE_VFIO_AMD_XGBE.
> > > > > > * ppce500 supports only TYPE_ETSEC_COMMON.
> > > > > > * spapr supports only TYPE_SPAPR_PCI_HOST_BRIDGE.
> > > > > >
> > > > > > q35 has no code to block unsupported sysbus devices, however, and
> > > > > > accepts all device types. Fortunately, only the following 20
> > > > > > device types are compiled into the qemu-system-x86_64 and
> > > > > > qemu-system-i386 binaries:
> > > > > >
> > > > > > * allwinner-ahci
> > > > > > * amd-iommu
> > > > > > * cfi.pflash01
> > > > > > * esp
> > > > > > * fw_cfg_io
> > > > > > * fw_cfg_mem
> > > > > > * generic-sdhci
> > > > > > * hpet
> > > > > > * intel-iommu
> > > > > > * ioapic
> > > > > > * isabus-bridge
> > > > > > * kvmclock
> > > > > > * kvm-ioapic
> > > > > > * kvmvapic
> > > > > > * SUNW,fdtwo
> > > > > > * sysbus-ahci
> > > > > > * sysbus-fdc
> > > > > > * sysbus-ohci
> > > > > > * unimplemented-device
> > > > > > * virtio-mmio
> > > > > >
> > > > > > Instead of requiring each machine-type with has_dynamic_sysbus=1
> > > > > > to implement its own mechanism to block unsupported devices, we
> > > > > > can use the user_creatable flag to ensure we won't let the user
> > > > > > plug anything that will never work.
> > > > >
> > > > > How does this work? Which devices can be dynamically
> > > > > plugged is machine dependent. You can't dynamically-plug
> > > > > an intel-iommu on the ARM virt board, and you can't
> > > > > dynamically-plug the vfio-calxeda-xgmac on the spapr
> > > > > board, and so on. So I don't see how we can just have
> > > > > a flag on the device itself that controls whether
> > > > > it can be dynamically plugged.
> > > > >
> > > > > So I'm definitely coming around to the opinion that
> > > > > it's just a bug in the q35 board that it doesn't have
> > > > > any device whitelisting, and we should fix that.
> > > >
> > > > OK, let's assume q35 must implement a whitelist:
> > > >
> > > > To build that whitelist, we need to be able to know what should
> > > > be in the whitelist, or not. And nobody knew for sure what was
> > > > user-creatable in q35 by accident, and what was really supposed
> > > > to be user-creatable in q35. See the "q35 and sysbus devices"
> > > > thread I started ~2 weeks ago.
> > > >
> > > > Building a q35 whitelist will be much easier if make
> > > > sys-bus-devices non-user-creatable by default.
> > >
> > > So why are they user creatable in the first place?
> > >
> > > We used to have boards that were dynamic sysbus aware (ppce500, virt) that
> > > allowed dynamic creation and every other board did not. I don't remember the
> > > exact mechanism behind it though.
> > >
> > > When did that behavior change? It sounds like a regression somewhere.
> >
> > See patch description:
> >
> > > > > > commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
> > > > > > cannot_instantiate_with_device_add_yet in TYPE_SYSBUS,
> >
> > Note that the commit above is not a regression[1] (because q35
> > didn't have has_dynamic_sysbus=1 yet), but having sysbus devices
> > have cannot_instantiate_with_device_add_yet=false/user_creatable=true
> > by default makes it harder to build the whitelist for q35 (or
> > other machines that will have has_dynamic_sysbus=1 in the
> > future).
>
> I seem to miss the bigger picture.
>
> Why would anyone set has_dynamic_sysbus=1 in a board file without an
> explicit whitelist?
I guess it was because this was not documented anywhere. Note
that q35 is not the only case today: see
xen_set_dynamic_sysbus().
> The whitelist is *not* device specific. It's board
> specific, because your board needs to know how to wire up a device and how
> to expose the fact that it exists to the OS.
That part is true.
>
> So the real bug is that someone set has_dynamic_sysbus=1 in q35 without
> implementing all of the dynamic sysbus logic, no?
This part I disagree: we don't have "a real bug", we have two
different bugs that we are mixing them up:
1) q35 and the xen_set_dynamic_sysbus() hack have no whitelist.
2) cannot_instantiate_with_device_add_yet is not set correctly on
sysbus device classes (breaking "info qdm", and maybe other
code that depends on cannot_instantiate_with_device_add_yet).
It's my fault that we're mixing them up, because I am using bug
#1 as justification for the #2 fix (this seris). But fixing #2 is
not necessary for fixing #1, it just helps a lot.
--
Eduardo
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] [RFC 04/19] fdc: Remove user_creatable flag from sysbus-fdc & SUNW, fdtwo
2017-04-01 0:46 [Qemu-devel] [RFC 00/19] sysbus: Don't allow -device/device_add by default Eduardo Habkost
` (2 preceding siblings ...)
2017-04-01 0:46 ` [Qemu-devel] [RFC 03/19] sysbus: Set user_creatable=false by default on TYPE_SYS_BUS_DEVICE Eduardo Habkost
@ 2017-04-01 0:46 ` Eduardo Habkost
2017-04-01 0:46 ` [Qemu-devel] [RFC 05/19] pflash_cfi01: Remove user_creatable flag Eduardo Habkost
` (14 subsequent siblings)
18 siblings, 0 replies; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-01 0:46 UTC (permalink / raw)
To: qemu-devel
Cc: Laszlo Ersek, Alexander Graf, Marcel Apfelbaum, Thomas Huth,
Peter Maydell, Markus Armbruster, John Snow, Kevin Wolf,
Max Reitz, qemu-block
sysbus-fdc and SUNW,fdtwo devices need to be wired by the fdc
initialization code, and can't be used with -device. Unset
user_creatable on their device classes.
Cc: John Snow <jsnow@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Cc: qemu-block@nongnu.org
Cc: Thomas Huth <thuth@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
hw/block/fdc.c | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index a06c8e358c..a328693d15 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2880,11 +2880,6 @@ static void sysbus_fdc_class_init(ObjectClass *klass, void *data)
dc->props = sysbus_fdc_properties;
set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
- /*
- * FIXME: Set only for compatibility on q35 machine-type.
- * Probably never meant to be user-creatable
- */
- dc->user_creatable = true;
}
static const TypeInfo sysbus_fdc_info = {
@@ -2911,11 +2906,6 @@ static void sun4m_fdc_class_init(ObjectClass *klass, void *data)
dc->props = sun4m_fdc_properties;
set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
- /*
- * FIXME: Set only for compatibility on q35 machine-type.
- * Probably never meant to be user-creatable
- */
- dc->user_creatable = true;
}
static const TypeInfo sun4m_fdc_info = {
--
2.11.0.259.g40922b1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [RFC 05/19] pflash_cfi01: Remove user_creatable flag
2017-04-01 0:46 [Qemu-devel] [RFC 00/19] sysbus: Don't allow -device/device_add by default Eduardo Habkost
` (3 preceding siblings ...)
2017-04-01 0:46 ` [Qemu-devel] [RFC 04/19] fdc: Remove user_creatable flag from sysbus-fdc & SUNW, fdtwo Eduardo Habkost
@ 2017-04-01 0:46 ` Eduardo Habkost
2017-04-03 12:43 ` Philippe Mathieu-Daudé
2017-04-03 15:45 ` Laszlo Ersek
2017-04-01 0:46 ` [Qemu-devel] [RFC 06/19] iommu: Remove FIXME comment about user_creatable=true Eduardo Habkost
` (13 subsequent siblings)
18 siblings, 2 replies; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-01 0:46 UTC (permalink / raw)
To: qemu-devel
Cc: Laszlo Ersek, Alexander Graf, Marcel Apfelbaum, Thomas Huth,
Peter Maydell, Markus Armbruster, Kevin Wolf, Max Reitz,
qemu-block
TYPE_CFI_PFLASH01 devices need to be mapped by
pflash_cfi01_register() and can't be used with -device. Remove
user_creatable from the device class.
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Cc: qemu-block@nongnu.org
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
hw/block/pflash_cfi01.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index f48dc20035..594d4cf6fe 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -927,11 +927,6 @@ static void pflash_cfi01_class_init(ObjectClass *klass, void *data)
dc->props = pflash_cfi01_properties;
dc->vmsd = &vmstate_pflash;
set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
- /*
- * FIXME: Set only for compatibility on q35 machine-type.
- * Probably never meant to be user-creatable
- */
- dc->user_creatable = true;
}
--
2.11.0.259.g40922b1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [RFC 05/19] pflash_cfi01: Remove user_creatable flag
2017-04-01 0:46 ` [Qemu-devel] [RFC 05/19] pflash_cfi01: Remove user_creatable flag Eduardo Habkost
@ 2017-04-03 12:43 ` Philippe Mathieu-Daudé
2017-04-03 15:45 ` Laszlo Ersek
1 sibling, 0 replies; 55+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-04-03 12:43 UTC (permalink / raw)
To: Eduardo Habkost, qemu-devel
Cc: Kevin Wolf, Peter Maydell, Thomas Huth, qemu-block,
Alexander Graf, Markus Armbruster, Marcel Apfelbaum, Max Reitz,
Laszlo Ersek
On 03/31/2017 09:46 PM, Eduardo Habkost wrote:
> TYPE_CFI_PFLASH01 devices need to be mapped by
> pflash_cfi01_register() and can't be used with -device. Remove
> user_creatable from the device class.
>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Max Reitz <mreitz@redhat.com>
> Cc: qemu-block@nongnu.org
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/block/pflash_cfi01.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index f48dc20035..594d4cf6fe 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -927,11 +927,6 @@ static void pflash_cfi01_class_init(ObjectClass *klass, void *data)
> dc->props = pflash_cfi01_properties;
> dc->vmsd = &vmstate_pflash;
> set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> - /*
> - * FIXME: Set only for compatibility on q35 machine-type.
> - * Probably never meant to be user-creatable
> - */
> - dc->user_creatable = true;
> }
>
>
>
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [RFC 05/19] pflash_cfi01: Remove user_creatable flag
2017-04-01 0:46 ` [Qemu-devel] [RFC 05/19] pflash_cfi01: Remove user_creatable flag Eduardo Habkost
2017-04-03 12:43 ` Philippe Mathieu-Daudé
@ 2017-04-03 15:45 ` Laszlo Ersek
1 sibling, 0 replies; 55+ messages in thread
From: Laszlo Ersek @ 2017-04-03 15:45 UTC (permalink / raw)
To: Eduardo Habkost, qemu-devel
Cc: Alexander Graf, Marcel Apfelbaum, Thomas Huth, Peter Maydell,
Markus Armbruster, Kevin Wolf, Max Reitz, qemu-block
On 04/01/17 02:46, Eduardo Habkost wrote:
> TYPE_CFI_PFLASH01 devices need to be mapped by
> pflash_cfi01_register() and can't be used with -device. Remove
> user_creatable from the device class.
>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Max Reitz <mreitz@redhat.com>
> Cc: qemu-block@nongnu.org
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> hw/block/pflash_cfi01.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index f48dc20035..594d4cf6fe 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -927,11 +927,6 @@ static void pflash_cfi01_class_init(ObjectClass *klass, void *data)
> dc->props = pflash_cfi01_properties;
> dc->vmsd = &vmstate_pflash;
> set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> - /*
> - * FIXME: Set only for compatibility on q35 machine-type.
> - * Probably never meant to be user-creatable
> - */
> - dc->user_creatable = true;
> }
>
>
>
The commit message is not entirely correct; for example "virt" creates
each flash device in the create_one_flash() function, with manual
qdev_create / qdev_prop_set_* / qdev_init_nofail /
memory_region_add_subregion.
So I recommend updating the commit message to say,
... TYPE_CFI_PFLASH01 devices need to be mapped by
pflash_cfi01_register *or equivalent* and can't be used...
With that,
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] [RFC 06/19] iommu: Remove FIXME comment about user_creatable=true
2017-04-01 0:46 [Qemu-devel] [RFC 00/19] sysbus: Don't allow -device/device_add by default Eduardo Habkost
` (4 preceding siblings ...)
2017-04-01 0:46 ` [Qemu-devel] [RFC 05/19] pflash_cfi01: Remove user_creatable flag Eduardo Habkost
@ 2017-04-01 0:46 ` Eduardo Habkost
2017-04-01 0:46 ` [Qemu-devel] [RFC 07/19] kvmclock: Remove user_creatable flag Eduardo Habkost
` (12 subsequent siblings)
18 siblings, 0 replies; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-01 0:46 UTC (permalink / raw)
To: qemu-devel
Cc: Laszlo Ersek, Alexander Graf, Marcel Apfelbaum, Thomas Huth,
Peter Maydell, Markus Armbruster, Michael S. Tsirkin
amd-iommu and intel-iommu are really meant to be used with
-device, so they need user_creatable=true. Remove the FIXME
comment.
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
hw/i386/amd_iommu.c | 4 ----
hw/i386/intel_iommu.c | 4 ----
2 files changed, 8 deletions(-)
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 1d139f3f6a..748c3648bb 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1186,10 +1186,6 @@ static void amdvi_class_init(ObjectClass *klass, void* data)
dc->vmsd = &vmstate_amdvi;
dc->hotpluggable = false;
dc_class->realize = amdvi_realize;
- /*
- * FIXME: Set only for compatibility on q35 machine-type.
- * Probably never meant to be user-creatable
- */
dc->user_creatable = true;
}
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 5e5125f34b..1db33df5b7 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2601,10 +2601,6 @@ static void vtd_class_init(ObjectClass *klass, void *data)
dc->hotpluggable = false;
x86_class->realize = vtd_realize;
x86_class->int_remap = vtd_int_remap;
- /*
- * FIXME: Set only for compatibility on q35 machine-type.
- * Probably never meant to be user-creatable
- */
dc->user_creatable = true;
}
--
2.11.0.259.g40922b1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [RFC 07/19] kvmclock: Remove user_creatable flag
2017-04-01 0:46 [Qemu-devel] [RFC 00/19] sysbus: Don't allow -device/device_add by default Eduardo Habkost
` (5 preceding siblings ...)
2017-04-01 0:46 ` [Qemu-devel] [RFC 06/19] iommu: Remove FIXME comment about user_creatable=true Eduardo Habkost
@ 2017-04-01 0:46 ` Eduardo Habkost
2017-04-01 0:46 ` [Qemu-devel] [RFC 08/19] ioapic: " Eduardo Habkost
` (11 subsequent siblings)
18 siblings, 0 replies; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-01 0:46 UTC (permalink / raw)
To: qemu-devel
Cc: Laszlo Ersek, Alexander Graf, Marcel Apfelbaum, Thomas Huth,
Peter Maydell, Markus Armbruster, Michael S. Tsirkin,
Paolo Bonzini, Richard Henderson
kvmclock should be used by guests only when the appropriate CPUID
feature flags are set on the VCPU, and it is automatically
created by kvmclock_create() when those feature flags are set.
This means creating a kvmclock device using -device is useless.
Remove user_creatable from its device class.
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
hw/i386/kvm/clock.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
index 0dbf083b17..13eca374cd 100644
--- a/hw/i386/kvm/clock.c
+++ b/hw/i386/kvm/clock.c
@@ -286,11 +286,6 @@ static void kvmclock_class_init(ObjectClass *klass, void *data)
dc->realize = kvmclock_realize;
dc->vmsd = &kvmclock_vmsd;
dc->props = kvmclock_properties;
- /*
- * FIXME: Set only for compatibility on q35 machine-type.
- * Probably never meant to be user-creatable
- */
- dc->user_creatable = true;
}
static const TypeInfo kvmclock_info = {
--
2.11.0.259.g40922b1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [RFC 08/19] ioapic: Remove user_creatable flag
2017-04-01 0:46 [Qemu-devel] [RFC 00/19] sysbus: Don't allow -device/device_add by default Eduardo Habkost
` (6 preceding siblings ...)
2017-04-01 0:46 ` [Qemu-devel] [RFC 07/19] kvmclock: Remove user_creatable flag Eduardo Habkost
@ 2017-04-01 0:46 ` Eduardo Habkost
2017-04-01 0:46 ` [Qemu-devel] [RFC 09/19] kvmvapic: " Eduardo Habkost
` (10 subsequent siblings)
18 siblings, 0 replies; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-01 0:46 UTC (permalink / raw)
To: qemu-devel
Cc: Laszlo Ersek, Alexander Graf, Marcel Apfelbaum, Thomas Huth,
Peter Maydell, Markus Armbruster, Igor Mammedov,
Michael S. Tsirkin, Paolo Bonzini, Richard Henderson
An ioapic device is already created by the q35 initialization
code, and using "-device ioapic" or "-device kvm-ioapic" will
always fail with "Only 1 ioapics allowed". Remove the
user_creatable flag from the ioapic device classes.
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
hw/i386/kvm/ioapic.c | 5 -----
hw/intc/ioapic.c | 5 -----
2 files changed, 10 deletions(-)
diff --git a/hw/i386/kvm/ioapic.c b/hw/i386/kvm/ioapic.c
index cdc35de661..98ca480792 100644
--- a/hw/i386/kvm/ioapic.c
+++ b/hw/i386/kvm/ioapic.c
@@ -167,11 +167,6 @@ static void kvm_ioapic_class_init(ObjectClass *klass, void *data)
k->post_load = kvm_ioapic_put;
dc->reset = kvm_ioapic_reset;
dc->props = kvm_ioapic_properties;
- /*
- * FIXME: Set only for compatibility on q35 machine-type.
- * Probably never meant to be user-creatable
- */
- dc->user_creatable = true;
}
static const TypeInfo kvm_ioapic_info = {
diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
index 38af393538..37c4386ae3 100644
--- a/hw/intc/ioapic.c
+++ b/hw/intc/ioapic.c
@@ -448,11 +448,6 @@ static void ioapic_class_init(ObjectClass *klass, void *data)
k->post_load = ioapic_update_kvm_routes;
dc->reset = ioapic_reset_common;
dc->props = ioapic_properties;
- /*
- * FIXME: Set only for compatibility on q35 machine-type.
- * Probably never meant to be user-creatable
- */
- dc->user_creatable = true;
}
static const TypeInfo ioapic_info = {
--
2.11.0.259.g40922b1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [RFC 09/19] kvmvapic: Remove user_creatable flag
2017-04-01 0:46 [Qemu-devel] [RFC 00/19] sysbus: Don't allow -device/device_add by default Eduardo Habkost
` (7 preceding siblings ...)
2017-04-01 0:46 ` [Qemu-devel] [RFC 08/19] ioapic: " Eduardo Habkost
@ 2017-04-01 0:46 ` Eduardo Habkost
2017-04-01 0:46 ` [Qemu-devel] [RFC 10/19] sysbus-ahci: " Eduardo Habkost
` (9 subsequent siblings)
18 siblings, 0 replies; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-01 0:46 UTC (permalink / raw)
To: qemu-devel
Cc: Laszlo Ersek, Alexander Graf, Marcel Apfelbaum, Thomas Huth,
Peter Maydell, Markus Armbruster, Igor Mammedov, Paolo Bonzini,
Richard Henderson, Michael S. Tsirkin
The kvmvapic device needs to be created by apic_common_realize()
when apic-common.vapic=on is set, not using -device. Remove the
user_creatable flag from the device class.
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
hw/i386/kvmvapic.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
index fbada19253..82a49556af 100644
--- a/hw/i386/kvmvapic.c
+++ b/hw/i386/kvmvapic.c
@@ -856,11 +856,6 @@ static void vapic_class_init(ObjectClass *klass, void *data)
dc->reset = vapic_reset;
dc->vmsd = &vmstate_vapic;
dc->realize = vapic_realize;
- /*
- * FIXME: Set only for compatibility on q35 machine-type.
- * Probably never meant to be user-creatable
- */
- dc->user_creatable = true;
}
static const TypeInfo vapic_type = {
--
2.11.0.259.g40922b1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [RFC 10/19] sysbus-ahci: Remove user_creatable flag
2017-04-01 0:46 [Qemu-devel] [RFC 00/19] sysbus: Don't allow -device/device_add by default Eduardo Habkost
` (8 preceding siblings ...)
2017-04-01 0:46 ` [Qemu-devel] [RFC 09/19] kvmvapic: " Eduardo Habkost
@ 2017-04-01 0:46 ` Eduardo Habkost
2017-04-03 17:19 ` Alistair Francis
2017-04-01 0:46 ` [Qemu-devel] [RFC 11/19] allwinner-ahci: " Eduardo Habkost
` (8 subsequent siblings)
18 siblings, 1 reply; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-01 0:46 UTC (permalink / raw)
To: qemu-devel
Cc: Laszlo Ersek, Alexander Graf, Marcel Apfelbaum, Thomas Huth,
Peter Maydell, Markus Armbruster, John Snow, qemu-block,
Rob Herring, Alistair Francis, Edgar E. Iglesias
The sysbus-ahci devices are supposed to be created and wired by
code from other devices, like calxeda_init() and
xlnx_zynqmp_realize(), and won't work with -device. Remove the
user_creatable flag from the device class.
Cc: John Snow <jsnow@redhat.com>
Cc: qemu-block@nongnu.org
Cc: Rob Herring <robh@kernel.org>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Alistair Francis <alistair.francis@xilinx.com>
Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
hw/ide/ahci.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index b1b7780d56..68f2ce09ee 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1721,11 +1721,6 @@ static void sysbus_ahci_class_init(ObjectClass *klass, void *data)
dc->props = sysbus_ahci_properties;
dc->reset = sysbus_ahci_reset;
set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
- /*
- * FIXME: Set only for compatibility on q35 machine-type.
- * Probably never meant to be user-creatable
- */
- dc->user_creatable = true;
}
static const TypeInfo sysbus_ahci_info = {
--
2.11.0.259.g40922b1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [RFC 10/19] sysbus-ahci: Remove user_creatable flag
2017-04-01 0:46 ` [Qemu-devel] [RFC 10/19] sysbus-ahci: " Eduardo Habkost
@ 2017-04-03 17:19 ` Alistair Francis
0 siblings, 0 replies; 55+ messages in thread
From: Alistair Francis @ 2017-04-03 17:19 UTC (permalink / raw)
To: Eduardo Habkost
Cc: qemu-devel@nongnu.org Developers, Peter Maydell, Thomas Huth,
qemu-block, Rob Herring, John Snow, Alexander Graf,
Markus Armbruster, Marcel Apfelbaum, Edgar E. Iglesias,
Alistair Francis, Laszlo Ersek
On Fri, Mar 31, 2017 at 5:46 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> The sysbus-ahci devices are supposed to be created and wired by
> code from other devices, like calxeda_init() and
> xlnx_zynqmp_realize(), and won't work with -device. Remove the
> user_creatable flag from the device class.
>
> Cc: John Snow <jsnow@redhat.com>
> Cc: qemu-block@nongnu.org
> Cc: Rob Herring <robh@kernel.org>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Alistair Francis <alistair.francis@xilinx.com>
> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> hw/ide/ahci.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index b1b7780d56..68f2ce09ee 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1721,11 +1721,6 @@ static void sysbus_ahci_class_init(ObjectClass *klass, void *data)
> dc->props = sysbus_ahci_properties;
> dc->reset = sysbus_ahci_reset;
> set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> - /*
> - * FIXME: Set only for compatibility on q35 machine-type.
> - * Probably never meant to be user-creatable
> - */
> - dc->user_creatable = true;
Part of me thinks this is a step backwards by marking it unsupported.
On the other hand I don't see how this could usefully be connected any
other way.
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
Thanks,
Alistair
> }
>
> static const TypeInfo sysbus_ahci_info = {
> --
> 2.11.0.259.g40922b1
>
>
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] [RFC 11/19] allwinner-ahci: Remove user_creatable flag
2017-04-01 0:46 [Qemu-devel] [RFC 00/19] sysbus: Don't allow -device/device_add by default Eduardo Habkost
` (9 preceding siblings ...)
2017-04-01 0:46 ` [Qemu-devel] [RFC 10/19] sysbus-ahci: " Eduardo Habkost
@ 2017-04-01 0:46 ` Eduardo Habkost
2017-04-01 0:46 ` [Qemu-devel] [RFC 12/19] isabus-bridge: " Eduardo Habkost
` (7 subsequent siblings)
18 siblings, 0 replies; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-01 0:46 UTC (permalink / raw)
To: qemu-devel
Cc: Laszlo Ersek, Alexander Graf, Marcel Apfelbaum, Thomas Huth,
Peter Maydell, Markus Armbruster, John Snow, qemu-block,
Beniamino Galvani, qemu-arm
allwinner-ahci needs to be created and wired by the alwinner-a10
device, and won't work with -device. Remove the user_creatable
flag from the device class.
Cc: John Snow <jsnow@redhat.com>
Cc: qemu-block@nongnu.org
Cc: Beniamino Galvani <b.galvani@gmail.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
hw/ide/ahci.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 68f2ce09ee..f60826d6e0 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1815,11 +1815,6 @@ static void allwinner_ahci_class_init(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
dc->vmsd = &vmstate_allwinner_ahci;
- /*
- * FIXME: Set only for compatibility on q35 machine-type.
- * Probably never meant to be user-creatable
- */
- dc->user_creatable = true;
}
static const TypeInfo allwinner_ahci_info = {
--
2.11.0.259.g40922b1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [RFC 12/19] isabus-bridge: Remove user_creatable flag
2017-04-01 0:46 [Qemu-devel] [RFC 00/19] sysbus: Don't allow -device/device_add by default Eduardo Habkost
` (10 preceding siblings ...)
2017-04-01 0:46 ` [Qemu-devel] [RFC 11/19] allwinner-ahci: " Eduardo Habkost
@ 2017-04-01 0:46 ` Eduardo Habkost
2017-04-01 0:46 ` [Qemu-devel] [RFC 13/19] unimplemented-device: " Eduardo Habkost
` (6 subsequent siblings)
18 siblings, 0 replies; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-01 0:46 UTC (permalink / raw)
To: qemu-devel
Cc: Laszlo Ersek, Alexander Graf, Marcel Apfelbaum, Thomas Huth,
Peter Maydell, Markus Armbruster, Michael S. Tsirkin
isabus-bridge needs to be created by isa_bus_new(), and won't
work with -device, as it won't create the TYPE_ISA_BUS bus
itself. Remove the user_creatable flag from the device class.
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
hw/isa/isa-bus.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 576dbf1763..348e0eab9d 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -221,11 +221,6 @@ static void isabus_bridge_class_init(ObjectClass *klass, void *data)
set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
dc->fw_name = "isa";
- /*
- * FIXME: Set only for compatibility on q35 machine-type.
- * Probably never meant to be user-creatable
- */
- dc->user_creatable = true;
}
static const TypeInfo isabus_bridge_info = {
--
2.11.0.259.g40922b1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [RFC 13/19] unimplemented-device: Remove user_creatable flag
2017-04-01 0:46 [Qemu-devel] [RFC 00/19] sysbus: Don't allow -device/device_add by default Eduardo Habkost
` (11 preceding siblings ...)
2017-04-01 0:46 ` [Qemu-devel] [RFC 12/19] isabus-bridge: " Eduardo Habkost
@ 2017-04-01 0:46 ` Eduardo Habkost
2017-04-03 12:44 ` Philippe Mathieu-Daudé
2017-04-03 12:57 ` Peter Maydell
2017-04-01 0:46 ` [Qemu-devel] [RFC 14/19] fw_cfg: " Eduardo Habkost
` (5 subsequent siblings)
18 siblings, 2 replies; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-01 0:46 UTC (permalink / raw)
To: qemu-devel
Cc: Laszlo Ersek, Alexander Graf, Marcel Apfelbaum, Thomas Huth,
Peter Maydell, Markus Armbruster
unimplemented-device needs to be created and mapped using
create_unimplemented_device(), and won't work with -device.
Remove the user_creatable flag from the device class.
Cc: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
hw/misc/unimp.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/hw/misc/unimp.c b/hw/misc/unimp.c
index 284f096beb..bcbb585888 100644
--- a/hw/misc/unimp.c
+++ b/hw/misc/unimp.c
@@ -90,11 +90,6 @@ static void unimp_class_init(ObjectClass *klass, void *data)
dc->realize = unimp_realize;
dc->props = unimp_properties;
- /*
- * FIXME: Set only for compatibility on q35 machine-type.
- * Probably never meant to be user-creatable
- */
- dc->user_creatable = true;
}
static const TypeInfo unimp_info = {
--
2.11.0.259.g40922b1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [RFC 13/19] unimplemented-device: Remove user_creatable flag
2017-04-01 0:46 ` [Qemu-devel] [RFC 13/19] unimplemented-device: " Eduardo Habkost
@ 2017-04-03 12:44 ` Philippe Mathieu-Daudé
2017-04-03 12:57 ` Peter Maydell
1 sibling, 0 replies; 55+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-04-03 12:44 UTC (permalink / raw)
To: Eduardo Habkost, qemu-devel
Cc: Peter Maydell, Thomas Huth, Alexander Graf, Markus Armbruster,
Marcel Apfelbaum, Laszlo Ersek
On 03/31/2017 09:46 PM, Eduardo Habkost wrote:
> unimplemented-device needs to be created and mapped using
> create_unimplemented_device(), and won't work with -device.
> Remove the user_creatable flag from the device class.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/misc/unimp.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/hw/misc/unimp.c b/hw/misc/unimp.c
> index 284f096beb..bcbb585888 100644
> --- a/hw/misc/unimp.c
> +++ b/hw/misc/unimp.c
> @@ -90,11 +90,6 @@ static void unimp_class_init(ObjectClass *klass, void *data)
>
> dc->realize = unimp_realize;
> dc->props = unimp_properties;
> - /*
> - * FIXME: Set only for compatibility on q35 machine-type.
> - * Probably never meant to be user-creatable
> - */
> - dc->user_creatable = true;
> }
>
> static const TypeInfo unimp_info = {
>
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [RFC 13/19] unimplemented-device: Remove user_creatable flag
2017-04-01 0:46 ` [Qemu-devel] [RFC 13/19] unimplemented-device: " Eduardo Habkost
2017-04-03 12:44 ` Philippe Mathieu-Daudé
@ 2017-04-03 12:57 ` Peter Maydell
2017-04-03 13:34 ` Eduardo Habkost
1 sibling, 1 reply; 55+ messages in thread
From: Peter Maydell @ 2017-04-03 12:57 UTC (permalink / raw)
To: Eduardo Habkost
Cc: QEMU Developers, Laszlo Ersek, Alexander Graf, Marcel Apfelbaum,
Thomas Huth, Markus Armbruster
On 1 April 2017 at 01:46, Eduardo Habkost <ehabkost@redhat.com> wrote:
> unimplemented-device needs to be created and mapped using
> create_unimplemented_device()
This isn't correct -- create_unimplemented_device() is
just a utility function; you can create, configure, initialize
and map it "by hand" if you want.
> and won't work with -device.
This is true, though, as with any sysbus device that has
a memory region.
> Remove the user_creatable flag from the device class.
I'm obviously missing context, because in master it doesn't
have code to set the user_creatable flag in the first place.
Wouldn't it be better to just not add that, rather than
add it and then delete it?
thanks
-- PMM
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [RFC 13/19] unimplemented-device: Remove user_creatable flag
2017-04-03 12:57 ` Peter Maydell
@ 2017-04-03 13:34 ` Eduardo Habkost
2017-04-03 13:38 ` Peter Maydell
0 siblings, 1 reply; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-03 13:34 UTC (permalink / raw)
To: Peter Maydell
Cc: QEMU Developers, Laszlo Ersek, Alexander Graf, Marcel Apfelbaum,
Thomas Huth, Markus Armbruster
On Mon, Apr 03, 2017 at 01:57:06PM +0100, Peter Maydell wrote:
> On 1 April 2017 at 01:46, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > unimplemented-device needs to be created and mapped using
> > create_unimplemented_device()
>
> This isn't correct -- create_unimplemented_device() is
> just a utility function; you can create, configure, initialize
> and map it "by hand" if you want.
OK, I will rewrite it.
>
> > and won't work with -device.
>
> This is true, though, as with any sysbus device that has
> a memory region.
>
> > Remove the user_creatable flag from the device class.
>
> I'm obviously missing context, because in master it doesn't
> have code to set the user_creatable flag in the first place.
This is in patch 01/19: cannot_instantiate_with_device_add_yet
was replaced with !user_creatable.
> Wouldn't it be better to just not add that, rather than
> add it and then delete it?
The device was already user-creatable, but it was not explicit in
the code. Patch 03/19 sets user_creatable=false on sysbus but
adds explicit user_creatable=true on the 20 devices that are
currently accepted by qemu-system-x86_64. The remaining patches
clear user_creatable on individual devices types.
--
Eduardo
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [RFC 13/19] unimplemented-device: Remove user_creatable flag
2017-04-03 13:34 ` Eduardo Habkost
@ 2017-04-03 13:38 ` Peter Maydell
2017-04-03 13:54 ` Eduardo Habkost
0 siblings, 1 reply; 55+ messages in thread
From: Peter Maydell @ 2017-04-03 13:38 UTC (permalink / raw)
To: Eduardo Habkost
Cc: QEMU Developers, Laszlo Ersek, Alexander Graf, Marcel Apfelbaum,
Thomas Huth, Markus Armbruster
On 3 April 2017 at 14:34, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> Wouldn't it be better to just not add that, rather than
>> add it and then delete it?
>
> The device was already user-creatable
It doesn't seem to be:
./build/x86/arm-softmmu/qemu-system-arm -M virt -device
unimplemented-device,size=1024,name=foo
qemu-system-arm: Device unimplemented-device can not be dynamically instantiated
thanks
-- PMM
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [RFC 13/19] unimplemented-device: Remove user_creatable flag
2017-04-03 13:38 ` Peter Maydell
@ 2017-04-03 13:54 ` Eduardo Habkost
2017-04-03 14:08 ` Peter Maydell
0 siblings, 1 reply; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-03 13:54 UTC (permalink / raw)
To: Peter Maydell
Cc: QEMU Developers, Laszlo Ersek, Alexander Graf, Marcel Apfelbaum,
Thomas Huth, Markus Armbruster
On Mon, Apr 03, 2017 at 02:38:15PM +0100, Peter Maydell wrote:
> On 3 April 2017 at 14:34, Eduardo Habkost <ehabkost@redhat.com> wrote:
> >> Wouldn't it be better to just not add that, rather than
> >> add it and then delete it?
> >
> > The device was already user-creatable
>
> It doesn't seem to be:
>
> ./build/x86/arm-softmmu/qemu-system-arm -M virt -device
> unimplemented-device,size=1024,name=foo
> qemu-system-arm: Device unimplemented-device can not be dynamically instantiated
This restriction is implemented by the "virt" machine, not by the
device type itself.
This, on the other hand, currently works:
$ qemu-system-x86_64 -M q35 -device unimplemented-device,size=1024,name=foo
--
Eduardo
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [RFC 13/19] unimplemented-device: Remove user_creatable flag
2017-04-03 13:54 ` Eduardo Habkost
@ 2017-04-03 14:08 ` Peter Maydell
2017-04-03 18:30 ` Eduardo Habkost
0 siblings, 1 reply; 55+ messages in thread
From: Peter Maydell @ 2017-04-03 14:08 UTC (permalink / raw)
To: Eduardo Habkost
Cc: QEMU Developers, Laszlo Ersek, Alexander Graf, Marcel Apfelbaum,
Thomas Huth, Markus Armbruster
On 3 April 2017 at 14:54, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Mon, Apr 03, 2017 at 02:38:15PM +0100, Peter Maydell wrote:
>> On 3 April 2017 at 14:34, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> >> Wouldn't it be better to just not add that, rather than
>> >> add it and then delete it?
>> >
>> > The device was already user-creatable
>>
>> It doesn't seem to be:
>>
>> ./build/x86/arm-softmmu/qemu-system-arm -M virt -device
>> unimplemented-device,size=1024,name=foo
>> qemu-system-arm: Device unimplemented-device can not be dynamically instantiated
>
> This restriction is implemented by the "virt" machine, not by the
> device type itself.
>
> This, on the other hand, currently works:
> $ qemu-system-x86_64 -M q35 -device unimplemented-device,size=1024,name=foo
That's a bug in the q35 machine or the handling of -device
on non-platform-bus systems, though, isn't it? The default
for all sysbus devices has always been "you can't use
-device with this", there's never been a requirement to
mark them as such separately (because it would require
touching the files for a huge number of devices for no
particularly good reason when you can default the whole
set of devices subclassing SysBusDevice).
thanks
-- PMM
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [RFC 13/19] unimplemented-device: Remove user_creatable flag
2017-04-03 14:08 ` Peter Maydell
@ 2017-04-03 18:30 ` Eduardo Habkost
2017-04-03 19:42 ` Peter Maydell
0 siblings, 1 reply; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-03 18:30 UTC (permalink / raw)
To: Peter Maydell
Cc: QEMU Developers, Laszlo Ersek, Alexander Graf, Marcel Apfelbaum,
Thomas Huth, Markus Armbruster
On Mon, Apr 03, 2017 at 03:08:06PM +0100, Peter Maydell wrote:
> On 3 April 2017 at 14:54, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Mon, Apr 03, 2017 at 02:38:15PM +0100, Peter Maydell wrote:
> >> On 3 April 2017 at 14:34, Eduardo Habkost <ehabkost@redhat.com> wrote:
> >> >> Wouldn't it be better to just not add that, rather than
> >> >> add it and then delete it?
> >> >
> >> > The device was already user-creatable
> >>
> >> It doesn't seem to be:
> >>
> >> ./build/x86/arm-softmmu/qemu-system-arm -M virt -device
> >> unimplemented-device,size=1024,name=foo
> >> qemu-system-arm: Device unimplemented-device can not be dynamically instantiated
> >
> > This restriction is implemented by the "virt" machine, not by the
> > device type itself.
> >
> > This, on the other hand, currently works:
> > $ qemu-system-x86_64 -M q35 -device unimplemented-device,size=1024,name=foo
>
> That's a bug in the q35 machine or the handling of -device
> on non-platform-bus systems, though, isn't it? The default
> for all sysbus devices has always been "you can't use
> -device with this", [...]
This was true until 2014, only. commit
33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 changed sys-bus-device
to have cannot_instantiate_with_device_add_yet=false. See patch
03/19 for more detailed info.
> [...] there's never been a requirement to
> mark them as such separately (because it would require
> touching the files for a huge number of devices for no
> particularly good reason when you can default the whole
> set of devices subclassing SysBusDevice).
Yes, and this is the whole point of this series. At the end of
this series, only the few devices that are actually usable with
some machine will have an explicit user_creatable=true
assignment. See cover letter for details.
--
Eduardo
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [RFC 13/19] unimplemented-device: Remove user_creatable flag
2017-04-03 18:30 ` Eduardo Habkost
@ 2017-04-03 19:42 ` Peter Maydell
2017-04-03 20:05 ` Eduardo Habkost
0 siblings, 1 reply; 55+ messages in thread
From: Peter Maydell @ 2017-04-03 19:42 UTC (permalink / raw)
To: Eduardo Habkost
Cc: QEMU Developers, Laszlo Ersek, Alexander Graf, Marcel Apfelbaum,
Thomas Huth, Markus Armbruster
On 3 April 2017 at 19:30, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Mon, Apr 03, 2017 at 03:08:06PM +0100, Peter Maydell wrote:
>> On 3 April 2017 at 14:54, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> > This, on the other hand, currently works:
>> > $ qemu-system-x86_64 -M q35 -device unimplemented-device,size=1024,name=foo
>>
>> That's a bug in the q35 machine or the handling of -device
>> on non-platform-bus systems, though, isn't it? The default
>> for all sysbus devices has always been "you can't use
>> -device with this", [...]
>
> This was true until 2014, only. commit
> 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 changed sys-bus-device
> to have cannot_instantiate_with_device_add_yet=false. See patch
> 03/19 for more detailed info.
That commit shouldn't cause this problem -- unless the
machine sets has_dynamic_sysbus then the machine_init_notify()
that commit adds will cause it to complain.
I think the bug was introduced much later, in bf8d4924 in 2016,
when q35 had the has_dynamic_sysbus flag added but without
any code to restruct this to a whitelist of working
devices. (In fact you can see in that commit a very
minimal attempt to blacklist a few devices.)
The commit message says only intel-iommu was supposed to
be -device creatable, so it should have been the only
thing on the whitelist. Commit 9e3f9733 shows how this
should be done (that's where spapr got has_dynamic_sysbus).
Maybe we should just fix the q35 bug first?
>> [...] there's never been a requirement to
>> mark them as such separately (because it would require
>> touching the files for a huge number of devices for no
>> particularly good reason when you can default the whole
>> set of devices subclassing SysBusDevice).
>
> Yes, and this is the whole point of this series. At the end of
> this series, only the few devices that are actually usable with
> some machine will have an explicit user_creatable=true
> assignment. See cover letter for details.
...OK, but in that case why not just set that where it should
be set, rather than having a big long patchset that touches
a lot of devices that in the end are right back where
they started? I think a lot of why I'm confused by
this patchset is that it seems like it's changing
behaviour on devices like this one which don't need
any changes...
thanks
-- PMM
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [RFC 13/19] unimplemented-device: Remove user_creatable flag
2017-04-03 19:42 ` Peter Maydell
@ 2017-04-03 20:05 ` Eduardo Habkost
2017-04-04 7:05 ` Thomas Huth
0 siblings, 1 reply; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-03 20:05 UTC (permalink / raw)
To: Peter Maydell
Cc: QEMU Developers, Laszlo Ersek, Alexander Graf, Marcel Apfelbaum,
Thomas Huth, Markus Armbruster
On Mon, Apr 03, 2017 at 08:42:12PM +0100, Peter Maydell wrote:
> On 3 April 2017 at 19:30, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Mon, Apr 03, 2017 at 03:08:06PM +0100, Peter Maydell wrote:
> >> On 3 April 2017 at 14:54, Eduardo Habkost <ehabkost@redhat.com> wrote:
> >> > This, on the other hand, currently works:
> >> > $ qemu-system-x86_64 -M q35 -device unimplemented-device,size=1024,name=foo
> >>
> >> That's a bug in the q35 machine or the handling of -device
> >> on non-platform-bus systems, though, isn't it? The default
> >> for all sysbus devices has always been "you can't use
> >> -device with this", [...]
> >
> > This was true until 2014, only. commit
> > 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 changed sys-bus-device
> > to have cannot_instantiate_with_device_add_yet=false. See patch
> > 03/19 for more detailed info.
>
> That commit shouldn't cause this problem -- unless the
> machine sets has_dynamic_sysbus then the machine_init_notify()
> that commit adds will cause it to complain.
>
> I think the bug was introduced much later, in bf8d4924 in 2016,
> when q35 had the has_dynamic_sysbus flag added but without
> any code to restruct this to a whitelist of working
> devices. (In fact you can see in that commit a very
> minimal attempt to blacklist a few devices.)
It's true that the problem happened when q35 set
has_dynamic_sysbus=1 without a whitelist. The point of this
series is to make a per-machine-type whitelist unnecessary.
>
> The commit message says only intel-iommu was supposed to
> be -device creatable, so it should have been the only
> thing on the whitelist.
There was a thread about it (Subject: "q35 and sysbus devices"),
and nobody knew for sure if the q35 whitelist was supposed to
have *-iommu only, or not.
The conclusion of that thread is that we can simply use
cannot_instantiate_with_device_add_yet to build the whitelist, if
the field was set consistently. This series renames
cannot_instantiate_with_device_add_yet to user_creatable, and
sets it consistently.
> Commit 9e3f9733 shows how this
> should be done (that's where spapr got has_dynamic_sysbus).
>
> Maybe we should just fix the q35 bug first?
This fixes the q35 bug by setting user_creatable correctly on
sys-bus-devices, and makes a q35-specific whitelist unnecessary.
>
> >> [...] there's never been a requirement to
> >> mark them as such separately (because it would require
> >> touching the files for a huge number of devices for no
> >> particularly good reason when you can default the whole
> >> set of devices subclassing SysBusDevice).
> >
> > Yes, and this is the whole point of this series. At the end of
> > this series, only the few devices that are actually usable with
> > some machine will have an explicit user_creatable=true
> > assignment. See cover letter for details.
>
> ...OK, but in that case why not just set that where it should
> be set, rather than having a big long patchset that touches
> a lot of devices that in the end are right back where
> they started? I think a lot of why I'm confused by
> this patchset is that it seems like it's changing
> behaviour on devices like this one which don't need
> any changes...
The devices don't get back right where they started: they start
with cannot_instantiate_with_device_add_yet=false (meaning they
are user-creatable in q35), and end up with user_creatable=false
(meaning they become non-user-creatable in all machines).
I could squash patches 04/19 to 19/19 into patch 03/19, it's
true. But then I would probably not get confirmation from you
(and other developers) that unimplemented-device (and other
devices) is really not supposed to be user-creatable. I wouldn't
know if I am breaking a valid q35 use case, or not.
--
Eduardo
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [RFC 13/19] unimplemented-device: Remove user_creatable flag
2017-04-03 20:05 ` Eduardo Habkost
@ 2017-04-04 7:05 ` Thomas Huth
2017-04-04 7:12 ` Alexander Graf
0 siblings, 1 reply; 55+ messages in thread
From: Thomas Huth @ 2017-04-04 7:05 UTC (permalink / raw)
To: Eduardo Habkost, Peter Maydell
Cc: QEMU Developers, Laszlo Ersek, Alexander Graf, Marcel Apfelbaum,
Markus Armbruster
On 03.04.2017 22:05, Eduardo Habkost wrote:
> On Mon, Apr 03, 2017 at 08:42:12PM +0100, Peter Maydell wrote:
>> On 3 April 2017 at 19:30, Eduardo Habkost <ehabkost@redhat.com> wrote:
>>> On Mon, Apr 03, 2017 at 03:08:06PM +0100, Peter Maydell wrote:
>>>> On 3 April 2017 at 14:54, Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>>> This, on the other hand, currently works:
>>>>> $ qemu-system-x86_64 -M q35 -device unimplemented-device,size=1024,name=foo
>>>>
>>>> That's a bug in the q35 machine or the handling of -device
>>>> on non-platform-bus systems, though, isn't it? The default
>>>> for all sysbus devices has always been "you can't use
>>>> -device with this", [...]
>>>
>>> This was true until 2014, only. commit
>>> 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 changed sys-bus-device
>>> to have cannot_instantiate_with_device_add_yet=false. See patch
>>> 03/19 for more detailed info.
>>
>> That commit shouldn't cause this problem -- unless the
>> machine sets has_dynamic_sysbus then the machine_init_notify()
>> that commit adds will cause it to complain.
>>
>> I think the bug was introduced much later, in bf8d4924 in 2016,
>> when q35 had the has_dynamic_sysbus flag added but without
>> any code to restruct this to a whitelist of working
>> devices. (In fact you can see in that commit a very
>> minimal attempt to blacklist a few devices.)
>
> It's true that the problem happened when q35 set
> has_dynamic_sysbus=1 without a whitelist. The point of this
> series is to make a per-machine-type whitelist unnecessary.
>
>>
>> The commit message says only intel-iommu was supposed to
>> be -device creatable, so it should have been the only
>> thing on the whitelist.
>
> There was a thread about it (Subject: "q35 and sysbus devices"),
> and nobody knew for sure if the q35 whitelist was supposed to
> have *-iommu only, or not.
>
> The conclusion of that thread is that we can simply use
> cannot_instantiate_with_device_add_yet to build the whitelist, if
> the field was set consistently. This series renames
> cannot_instantiate_with_device_add_yet to user_creatable, and
> sets it consistently.
I don't think that this will always work. While you can clearly mark
those devices that can never be added dynamically, there might also be
some devices that only work on certain machines. For example, there
might be devices that only work on the "ppce500" machine, but do not
work on the "pseries" machine. So we always need some kind of
whitelisting for the machines that have a dynamic sysbus.
Thomas
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [RFC 13/19] unimplemented-device: Remove user_creatable flag
2017-04-04 7:05 ` Thomas Huth
@ 2017-04-04 7:12 ` Alexander Graf
2017-04-04 13:12 ` Eduardo Habkost
0 siblings, 1 reply; 55+ messages in thread
From: Alexander Graf @ 2017-04-04 7:12 UTC (permalink / raw)
To: Thomas Huth
Cc: Eduardo Habkost, Peter Maydell, QEMU Developers, Laszlo Ersek,
Marcel Apfelbaum, Markus Armbruster
> Am 04.04.2017 um 09:05 schrieb Thomas Huth <thuth@redhat.com>:
>
>> On 03.04.2017 22:05, Eduardo Habkost wrote:
>>> On Mon, Apr 03, 2017 at 08:42:12PM +0100, Peter Maydell wrote:
>>>> On 3 April 2017 at 19:30, Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>>> On Mon, Apr 03, 2017 at 03:08:06PM +0100, Peter Maydell wrote:
>>>>>> On 3 April 2017 at 14:54, Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>>>> This, on the other hand, currently works:
>>>>>> $ qemu-system-x86_64 -M q35 -device unimplemented-device,size=1024,name=foo
>>>>>
>>>>> That's a bug in the q35 machine or the handling of -device
>>>>> on non-platform-bus systems, though, isn't it? The default
>>>>> for all sysbus devices has always been "you can't use
>>>>> -device with this", [...]
>>>>
>>>> This was true until 2014, only. commit
>>>> 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 changed sys-bus-device
>>>> to have cannot_instantiate_with_device_add_yet=false. See patch
>>>> 03/19 for more detailed info.
>>>
>>> That commit shouldn't cause this problem -- unless the
>>> machine sets has_dynamic_sysbus then the machine_init_notify()
>>> that commit adds will cause it to complain.
>>>
>>> I think the bug was introduced much later, in bf8d4924 in 2016,
>>> when q35 had the has_dynamic_sysbus flag added but without
>>> any code to restruct this to a whitelist of working
>>> devices. (In fact you can see in that commit a very
>>> minimal attempt to blacklist a few devices.)
>>
>> It's true that the problem happened when q35 set
>> has_dynamic_sysbus=1 without a whitelist. The point of this
>> series is to make a per-machine-type whitelist unnecessary.
>>
>>>
>>> The commit message says only intel-iommu was supposed to
>>> be -device creatable, so it should have been the only
>>> thing on the whitelist.
>>
>> There was a thread about it (Subject: "q35 and sysbus devices"),
>> and nobody knew for sure if the q35 whitelist was supposed to
>> have *-iommu only, or not.
>>
>> The conclusion of that thread is that we can simply use
>> cannot_instantiate_with_device_add_yet to build the whitelist, if
>> the field was set consistently. This series renames
>> cannot_instantiate_with_device_add_yet to user_creatable, and
>> sets it consistently.
>
> I don't think that this will always work. While you can clearly mark
> those devices that can never be added dynamically, there might also be
> some devices that only work on certain machines. For example, there
> might be devices that only work on the "ppce500" machine, but do not
> work on the "pseries" machine. So we always need some kind of
> whitelisting for the machines that have a dynamic sysbus.
It's even worse. Sysbus needs explicit logic to wire up memory regions and irqs. That logic is board specific (or at least was last time I checked).
So your board *needs* explicit logic for every sysbus device it spawns. And that's why you also get a board whitelist.
Alex
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [RFC 13/19] unimplemented-device: Remove user_creatable flag
2017-04-04 7:12 ` Alexander Graf
@ 2017-04-04 13:12 ` Eduardo Habkost
0 siblings, 0 replies; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-04 13:12 UTC (permalink / raw)
To: Alexander Graf
Cc: Thomas Huth, Peter Maydell, QEMU Developers, Laszlo Ersek,
Marcel Apfelbaum, Markus Armbruster
On Tue, Apr 04, 2017 at 09:12:59AM +0200, Alexander Graf wrote:
>
>
> > Am 04.04.2017 um 09:05 schrieb Thomas Huth <thuth@redhat.com>:
> >
> >> On 03.04.2017 22:05, Eduardo Habkost wrote:
> >>> On Mon, Apr 03, 2017 at 08:42:12PM +0100, Peter Maydell wrote:
> >>>> On 3 April 2017 at 19:30, Eduardo Habkost <ehabkost@redhat.com> wrote:
> >>>>> On Mon, Apr 03, 2017 at 03:08:06PM +0100, Peter Maydell wrote:
> >>>>>> On 3 April 2017 at 14:54, Eduardo Habkost <ehabkost@redhat.com> wrote:
> >>>>>> This, on the other hand, currently works:
> >>>>>> $ qemu-system-x86_64 -M q35 -device unimplemented-device,size=1024,name=foo
> >>>>>
> >>>>> That's a bug in the q35 machine or the handling of -device
> >>>>> on non-platform-bus systems, though, isn't it? The default
> >>>>> for all sysbus devices has always been "you can't use
> >>>>> -device with this", [...]
> >>>>
> >>>> This was true until 2014, only. commit
> >>>> 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 changed sys-bus-device
> >>>> to have cannot_instantiate_with_device_add_yet=false. See patch
> >>>> 03/19 for more detailed info.
> >>>
> >>> That commit shouldn't cause this problem -- unless the
> >>> machine sets has_dynamic_sysbus then the machine_init_notify()
> >>> that commit adds will cause it to complain.
> >>>
> >>> I think the bug was introduced much later, in bf8d4924 in 2016,
> >>> when q35 had the has_dynamic_sysbus flag added but without
> >>> any code to restruct this to a whitelist of working
> >>> devices. (In fact you can see in that commit a very
> >>> minimal attempt to blacklist a few devices.)
> >>
> >> It's true that the problem happened when q35 set
> >> has_dynamic_sysbus=1 without a whitelist. The point of this
> >> series is to make a per-machine-type whitelist unnecessary.
> >>
> >>>
> >>> The commit message says only intel-iommu was supposed to
> >>> be -device creatable, so it should have been the only
> >>> thing on the whitelist.
> >>
> >> There was a thread about it (Subject: "q35 and sysbus devices"),
> >> and nobody knew for sure if the q35 whitelist was supposed to
> >> have *-iommu only, or not.
> >>
> >> The conclusion of that thread is that we can simply use
> >> cannot_instantiate_with_device_add_yet to build the whitelist, if
> >> the field was set consistently. This series renames
> >> cannot_instantiate_with_device_add_yet to user_creatable, and
> >> sets it consistently.
> >
> > I don't think that this will always work. While you can clearly mark
> > those devices that can never be added dynamically, there might also be
> > some devices that only work on certain machines. For example, there
> > might be devices that only work on the "ppce500" machine, but do not
> > work on the "pseries" machine. So we always need some kind of
> > whitelisting for the machines that have a dynamic sysbus.
>
> It's even worse. Sysbus needs explicit logic to wire up memory
> regions and irqs. That logic is board specific (or at least was
> last time I checked).
>
> So your board *needs* explicit logic for every sysbus device it
> spawns. And that's why you also get a board whitelist.
You're correct. My suggestion is that we (humans, not the machine
code) can build the whitelist inside q35 code by looking at the
remaining user-creatable devices (after this series is reviewed).
If I suggested that fixing cannot_instantiate_with_device_add_yet
is sufficient to fix the q35 bug, that was a mistake.
--
Eduardo
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] [RFC 14/19] fw_cfg: Remove user_creatable flag
2017-04-01 0:46 [Qemu-devel] [RFC 00/19] sysbus: Don't allow -device/device_add by default Eduardo Habkost
` (12 preceding siblings ...)
2017-04-01 0:46 ` [Qemu-devel] [RFC 13/19] unimplemented-device: " Eduardo Habkost
@ 2017-04-01 0:46 ` Eduardo Habkost
2017-04-03 15:46 ` Laszlo Ersek
2017-04-01 0:46 ` [Qemu-devel] [RFC 15/19] esp: " Eduardo Habkost
` (4 subsequent siblings)
18 siblings, 1 reply; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-01 0:46 UTC (permalink / raw)
To: qemu-devel
Cc: Laszlo Ersek, Alexander Graf, Marcel Apfelbaum, Thomas Huth,
Peter Maydell, Markus Armbruster, Michael S. Tsirkin,
Gabriel L . Somlo
fw_cfg won't work with -device, as:
* fw_cfg_init1() won't get called for the device;
* The device won't appear at /machine/fw_cfg, and won't work with
the -fw_cfg command-line option.
Remove the user_creatable flag from the device class.
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Gabriel L. Somlo <somlo@cmu.edu>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
hw/nvram/fw_cfg.c | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 60bf4fdd2e..316fca9bc1 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -1101,11 +1101,6 @@ static void fw_cfg_io_class_init(ObjectClass *klass, void *data)
dc->realize = fw_cfg_io_realize;
dc->props = fw_cfg_io_properties;
- /*
- * FIXME: Set only for compatibility on q35 machine-type.
- * Probably never meant to be user-creatable
- */
- dc->user_creatable = true;
}
static const TypeInfo fw_cfg_io_info = {
@@ -1172,11 +1167,6 @@ static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)
dc->realize = fw_cfg_mem_realize;
dc->props = fw_cfg_mem_properties;
- /*
- * FIXME: Set only for compatibility on q35 machine-type.
- * Probably never meant to be user-creatable
- */
- dc->user_creatable = true;
}
static const TypeInfo fw_cfg_mem_info = {
--
2.11.0.259.g40922b1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [RFC 14/19] fw_cfg: Remove user_creatable flag
2017-04-01 0:46 ` [Qemu-devel] [RFC 14/19] fw_cfg: " Eduardo Habkost
@ 2017-04-03 15:46 ` Laszlo Ersek
0 siblings, 0 replies; 55+ messages in thread
From: Laszlo Ersek @ 2017-04-03 15:46 UTC (permalink / raw)
To: Eduardo Habkost, qemu-devel
Cc: Alexander Graf, Marcel Apfelbaum, Thomas Huth, Peter Maydell,
Markus Armbruster, Michael S. Tsirkin, Gabriel L . Somlo
On 04/01/17 02:46, Eduardo Habkost wrote:
> fw_cfg won't work with -device, as:
> * fw_cfg_init1() won't get called for the device;
> * The device won't appear at /machine/fw_cfg, and won't work with
> the -fw_cfg command-line option.
>
> Remove the user_creatable flag from the device class.
>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Gabriel L. Somlo <somlo@cmu.edu>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> hw/nvram/fw_cfg.c | 10 ----------
> 1 file changed, 10 deletions(-)
>
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 60bf4fdd2e..316fca9bc1 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -1101,11 +1101,6 @@ static void fw_cfg_io_class_init(ObjectClass *klass, void *data)
>
> dc->realize = fw_cfg_io_realize;
> dc->props = fw_cfg_io_properties;
> - /*
> - * FIXME: Set only for compatibility on q35 machine-type.
> - * Probably never meant to be user-creatable
> - */
> - dc->user_creatable = true;
> }
>
> static const TypeInfo fw_cfg_io_info = {
> @@ -1172,11 +1167,6 @@ static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)
>
> dc->realize = fw_cfg_mem_realize;
> dc->props = fw_cfg_mem_properties;
> - /*
> - * FIXME: Set only for compatibility on q35 machine-type.
> - * Probably never meant to be user-creatable
> - */
> - dc->user_creatable = true;
> }
>
> static const TypeInfo fw_cfg_mem_info = {
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] [RFC 15/19] esp: Remove user_creatable flag
2017-04-01 0:46 [Qemu-devel] [RFC 00/19] sysbus: Don't allow -device/device_add by default Eduardo Habkost
` (13 preceding siblings ...)
2017-04-01 0:46 ` [Qemu-devel] [RFC 14/19] fw_cfg: " Eduardo Habkost
@ 2017-04-01 0:46 ` Eduardo Habkost
2017-04-01 0:46 ` [Qemu-devel] [RFC 16/19] generic-sdhci: " Eduardo Habkost
` (3 subsequent siblings)
18 siblings, 0 replies; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-01 0:46 UTC (permalink / raw)
To: qemu-devel
Cc: Laszlo Ersek, Alexander Graf, Marcel Apfelbaum, Thomas Huth,
Peter Maydell, Markus Armbruster, Paolo Bonzini
esp devices aren't going to work with -device, as they need to be
wired by esp_init(). Remove the user_creatable flag from the
device class.
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
hw/scsi/esp.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 1087b8f14f..eee831efeb 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -728,11 +728,6 @@ static void sysbus_esp_class_init(ObjectClass *klass, void *data)
dc->reset = sysbus_esp_hard_reset;
dc->vmsd = &vmstate_sysbus_esp_scsi;
set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
- /*
- * FIXME: Set only for compatibility on q35 machine-type.
- * Probably never meant to be user-creatable
- */
- dc->user_creatable = true;
}
static const TypeInfo sysbus_esp_info = {
--
2.11.0.259.g40922b1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [RFC 16/19] generic-sdhci: Remove user_creatable flag
2017-04-01 0:46 [Qemu-devel] [RFC 00/19] sysbus: Don't allow -device/device_add by default Eduardo Habkost
` (14 preceding siblings ...)
2017-04-01 0:46 ` [Qemu-devel] [RFC 15/19] esp: " Eduardo Habkost
@ 2017-04-01 0:46 ` Eduardo Habkost
2017-04-03 17:20 ` Alistair Francis
2017-04-01 0:46 ` [Qemu-devel] [RFC 17/19] hpet: " Eduardo Habkost
` (2 subsequent siblings)
18 siblings, 1 reply; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-01 0:46 UTC (permalink / raw)
To: qemu-devel
Cc: Laszlo Ersek, Alexander Graf, Marcel Apfelbaum, Thomas Huth,
Peter Maydell, Markus Armbruster, Edgar E. Iglesias,
David Gibson, Michael S. Tsirkin, Prasad J Pandit,
Alistair Francis
generic-sdhci needs to be wired by other devices' code, so it
can't be used with -device. Remove the user_creatable flag from
the device class.
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Alexander Graf <agraf@suse.de>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: Prasad J Pandit <pjp@fedoraproject.org>
Cc: Alistair Francis <alistair.francis@xilinx.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
hw/sd/sdhci.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 186555188b..6d6a791ee9 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1360,11 +1360,6 @@ static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
dc->props = sdhci_sysbus_properties;
dc->realize = sdhci_sysbus_realize;
dc->reset = sdhci_poweron_reset;
- /*
- * FIXME: Set only for compatibility on q35 machine-type.
- * Probably never meant to be user-creatable
- */
- dc->user_creatable = true;
}
static const TypeInfo sdhci_sysbus_info = {
--
2.11.0.259.g40922b1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [RFC 16/19] generic-sdhci: Remove user_creatable flag
2017-04-01 0:46 ` [Qemu-devel] [RFC 16/19] generic-sdhci: " Eduardo Habkost
@ 2017-04-03 17:20 ` Alistair Francis
0 siblings, 0 replies; 55+ messages in thread
From: Alistair Francis @ 2017-04-03 17:20 UTC (permalink / raw)
To: Eduardo Habkost
Cc: qemu-devel@nongnu.org Developers, Peter Maydell, Thomas Huth,
Prasad J Pandit, Michael S. Tsirkin, Alexander Graf,
Markus Armbruster, Marcel Apfelbaum, Edgar E. Iglesias,
Alistair Francis, Laszlo Ersek, David Gibson
On Fri, Mar 31, 2017 at 5:46 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> generic-sdhci needs to be wired by other devices' code, so it
> can't be used with -device. Remove the user_creatable flag from
> the device class.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Marcel Apfelbaum <marcel@redhat.com>
> Cc: Prasad J Pandit <pjp@fedoraproject.org>
> Cc: Alistair Francis <alistair.francis@xilinx.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
Thanks,
Alistair
> ---
> hw/sd/sdhci.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 186555188b..6d6a791ee9 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1360,11 +1360,6 @@ static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
> dc->props = sdhci_sysbus_properties;
> dc->realize = sdhci_sysbus_realize;
> dc->reset = sdhci_poweron_reset;
> - /*
> - * FIXME: Set only for compatibility on q35 machine-type.
> - * Probably never meant to be user-creatable
> - */
> - dc->user_creatable = true;
> }
>
> static const TypeInfo sdhci_sysbus_info = {
> --
> 2.11.0.259.g40922b1
>
>
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] [RFC 17/19] hpet: Remove user_creatable flag
2017-04-01 0:46 [Qemu-devel] [RFC 00/19] sysbus: Don't allow -device/device_add by default Eduardo Habkost
` (15 preceding siblings ...)
2017-04-01 0:46 ` [Qemu-devel] [RFC 16/19] generic-sdhci: " Eduardo Habkost
@ 2017-04-01 0:46 ` Eduardo Habkost
2017-04-01 0:46 ` [Qemu-devel] [RFC 18/19] sysbus-ohci: " Eduardo Habkost
2017-04-01 0:46 ` [Qemu-devel] [RFC 19/19] virtio-mmio: " Eduardo Habkost
18 siblings, 0 replies; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-01 0:46 UTC (permalink / raw)
To: qemu-devel
Cc: Laszlo Ersek, Alexander Graf, Marcel Apfelbaum, Thomas Huth,
Peter Maydell, Markus Armbruster, Michael S. Tsirkin,
Paolo Bonzini
hpet needs to be mappend and wired by the board code and won't
work with -device. Remove the user_creatable flag from the device
class.
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
hw/timer/hpet.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index b5dab1c8e2..a2c18b30c3 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -771,11 +771,6 @@ static void hpet_device_class_init(ObjectClass *klass, void *data)
dc->reset = hpet_reset;
dc->vmsd = &vmstate_hpet;
dc->props = hpet_device_properties;
- /*
- * FIXME: Set only for compatibility on q35 machine-type.
- * Probably never meant to be user-creatable
- */
- dc->user_creatable = true;
}
static const TypeInfo hpet_device_info = {
--
2.11.0.259.g40922b1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [RFC 18/19] sysbus-ohci: Remove user_creatable flag
2017-04-01 0:46 [Qemu-devel] [RFC 00/19] sysbus: Don't allow -device/device_add by default Eduardo Habkost
` (16 preceding siblings ...)
2017-04-01 0:46 ` [Qemu-devel] [RFC 17/19] hpet: " Eduardo Habkost
@ 2017-04-01 0:46 ` Eduardo Habkost
2017-04-01 0:46 ` [Qemu-devel] [RFC 19/19] virtio-mmio: " Eduardo Habkost
18 siblings, 0 replies; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-01 0:46 UTC (permalink / raw)
To: qemu-devel
Cc: Laszlo Ersek, Alexander Graf, Marcel Apfelbaum, Thomas Huth,
Peter Maydell, Markus Armbruster, Gerd Hoffmann
sysbus-ohci needs to be mappend and wired by device or board
code, and won't work with -device. Remove the user_creatable flag
from the device class.
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
hw/usb/hcd-ohci.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index 0e17fb1e60..3ada35e954 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -2159,11 +2159,6 @@ static void ohci_sysbus_class_init(ObjectClass *klass, void *data)
dc->desc = "OHCI USB Controller";
dc->props = ohci_sysbus_properties;
dc->reset = usb_ohci_reset_sysbus;
- /*
- * FIXME: Set only for compatibility on q35 machine-type.
- * Probably never meant to be user-creatable
- */
- dc->user_creatable = true;
}
static const TypeInfo ohci_sysbus_info = {
--
2.11.0.259.g40922b1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [RFC 19/19] virtio-mmio: Remove user_creatable flag
2017-04-01 0:46 [Qemu-devel] [RFC 00/19] sysbus: Don't allow -device/device_add by default Eduardo Habkost
` (17 preceding siblings ...)
2017-04-01 0:46 ` [Qemu-devel] [RFC 18/19] sysbus-ohci: " Eduardo Habkost
@ 2017-04-01 0:46 ` Eduardo Habkost
2017-04-03 15:50 ` Laszlo Ersek
18 siblings, 1 reply; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-01 0:46 UTC (permalink / raw)
To: qemu-devel
Cc: Laszlo Ersek, Alexander Graf, Marcel Apfelbaum, Thomas Huth,
Peter Maydell, Markus Armbruster, Shannon Zhao,
Michael S. Tsirkin
virtio-mmio needs to be wired and mapped by other device or board
code, and won't work with -device. Remove the user_creatable flag
from the device class.
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Shannon Zhao <zhaoshenglong@huawei.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
hw/virtio/virtio-mmio.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index b595512a3d..5807aa87fe 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -450,11 +450,6 @@ static void virtio_mmio_class_init(ObjectClass *klass, void *data)
dc->reset = virtio_mmio_reset;
set_bit(DEVICE_CATEGORY_MISC, dc->categories);
dc->props = virtio_mmio_properties;
- /*
- * FIXME: Set only for compatibility on q35 machine-type.
- * Probably never meant to be user-creatable
- */
- dc->user_creatable = true;
}
static const TypeInfo virtio_mmio_info = {
--
2.11.0.259.g40922b1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [RFC 19/19] virtio-mmio: Remove user_creatable flag
2017-04-01 0:46 ` [Qemu-devel] [RFC 19/19] virtio-mmio: " Eduardo Habkost
@ 2017-04-03 15:50 ` Laszlo Ersek
2017-04-04 19:35 ` Eduardo Habkost
0 siblings, 1 reply; 55+ messages in thread
From: Laszlo Ersek @ 2017-04-03 15:50 UTC (permalink / raw)
To: Eduardo Habkost, qemu-devel
Cc: Alexander Graf, Marcel Apfelbaum, Thomas Huth, Peter Maydell,
Markus Armbruster, Shannon Zhao, Michael S. Tsirkin
On 04/01/17 02:46, Eduardo Habkost wrote:
> virtio-mmio needs to be wired and mapped by other device or board
> code, and won't work with -device. Remove the user_creatable flag
> from the device class.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Shannon Zhao <zhaoshenglong@huawei.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> hw/virtio/virtio-mmio.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
> index b595512a3d..5807aa87fe 100644
> --- a/hw/virtio/virtio-mmio.c
> +++ b/hw/virtio/virtio-mmio.c
> @@ -450,11 +450,6 @@ static void virtio_mmio_class_init(ObjectClass *klass, void *data)
> dc->reset = virtio_mmio_reset;
> set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> dc->props = virtio_mmio_properties;
> - /*
> - * FIXME: Set only for compatibility on q35 machine-type.
> - * Probably never meant to be user-creatable
> - */
> - dc->user_creatable = true;
> }
>
> static const TypeInfo virtio_mmio_info = {
>
Possible addition to the commit message: a reference to commit
587078f0ed63 ("hw/arm/virt: explain device-to-transport mapping in
create_virtio_devices()", 2015-02-05).
That's another example showing that board code has to participate in
virtio-mmio transport placement.
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [RFC 19/19] virtio-mmio: Remove user_creatable flag
2017-04-03 15:50 ` Laszlo Ersek
@ 2017-04-04 19:35 ` Eduardo Habkost
2017-04-05 8:30 ` Laszlo Ersek
0 siblings, 1 reply; 55+ messages in thread
From: Eduardo Habkost @ 2017-04-04 19:35 UTC (permalink / raw)
To: Laszlo Ersek
Cc: qemu-devel, Peter Maydell, Thomas Huth, Michael S. Tsirkin,
Alexander Graf, Markus Armbruster, Shannon Zhao,
Marcel Apfelbaum
On Mon, Apr 03, 2017 at 05:50:13PM +0200, Laszlo Ersek wrote:
> On 04/01/17 02:46, Eduardo Habkost wrote:
> > virtio-mmio needs to be wired and mapped by other device or board
> > code, and won't work with -device. Remove the user_creatable flag
> > from the device class.
> >
> > Cc: Peter Maydell <peter.maydell@linaro.org>
> > Cc: Shannon Zhao <zhaoshenglong@huawei.com>
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > hw/virtio/virtio-mmio.c | 5 -----
> > 1 file changed, 5 deletions(-)
> >
> > diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
> > index b595512a3d..5807aa87fe 100644
> > --- a/hw/virtio/virtio-mmio.c
> > +++ b/hw/virtio/virtio-mmio.c
> > @@ -450,11 +450,6 @@ static void virtio_mmio_class_init(ObjectClass *klass, void *data)
> > dc->reset = virtio_mmio_reset;
> > set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> > dc->props = virtio_mmio_properties;
> > - /*
> > - * FIXME: Set only for compatibility on q35 machine-type.
> > - * Probably never meant to be user-creatable
> > - */
> > - dc->user_creatable = true;
> > }
> >
> > static const TypeInfo virtio_mmio_info = {
> >
>
> Possible addition to the commit message: a reference to commit
> 587078f0ed63 ("hw/arm/virt: explain device-to-transport mapping in
> create_virtio_devices()", 2015-02-05).
I'm confused by the comments there: it says "Unfortunately, we
can't counteract the kernel change by reversing the loop; it
would break existing command lines". But, which comment-lines it
would break, if "-device virtio-mmio" was never supported?
>
> That's another example showing that board code has to participate in
> virtio-mmio transport placement.
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks!
--
Eduardo
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [RFC 19/19] virtio-mmio: Remove user_creatable flag
2017-04-04 19:35 ` Eduardo Habkost
@ 2017-04-05 8:30 ` Laszlo Ersek
0 siblings, 0 replies; 55+ messages in thread
From: Laszlo Ersek @ 2017-04-05 8:30 UTC (permalink / raw)
To: Eduardo Habkost
Cc: qemu-devel, Peter Maydell, Thomas Huth, Michael S. Tsirkin,
Alexander Graf, Markus Armbruster, Shannon Zhao,
Marcel Apfelbaum
On 04/04/17 21:35, Eduardo Habkost wrote:
> On Mon, Apr 03, 2017 at 05:50:13PM +0200, Laszlo Ersek wrote:
>> On 04/01/17 02:46, Eduardo Habkost wrote:
>>> virtio-mmio needs to be wired and mapped by other device or board
>>> code, and won't work with -device. Remove the user_creatable flag
>>> from the device class.
>>>
>>> Cc: Peter Maydell <peter.maydell@linaro.org>
>>> Cc: Shannon Zhao <zhaoshenglong@huawei.com>
>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>>> ---
>>> hw/virtio/virtio-mmio.c | 5 -----
>>> 1 file changed, 5 deletions(-)
>>>
>>> diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
>>> index b595512a3d..5807aa87fe 100644
>>> --- a/hw/virtio/virtio-mmio.c
>>> +++ b/hw/virtio/virtio-mmio.c
>>> @@ -450,11 +450,6 @@ static void virtio_mmio_class_init(ObjectClass *klass, void *data)
>>> dc->reset = virtio_mmio_reset;
>>> set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>>> dc->props = virtio_mmio_properties;
>>> - /*
>>> - * FIXME: Set only for compatibility on q35 machine-type.
>>> - * Probably never meant to be user-creatable
>>> - */
>>> - dc->user_creatable = true;
>>> }
>>>
>>> static const TypeInfo virtio_mmio_info = {
>>>
>>
>> Possible addition to the commit message: a reference to commit
>> 587078f0ed63 ("hw/arm/virt: explain device-to-transport mapping in
>> create_virtio_devices()", 2015-02-05).
>
> I'm confused by the comments there: it says "Unfortunately, we
> can't counteract the kernel change by reversing the loop; it
> would break existing command lines". But, which comment-lines it
> would break,
Reversing the order in which the platform code generates the virtio-mmio
transports would break guest kernel command lines that identify the root
filesystem by virtio device name, such as /dev/vda1 vs. /dev/vdb1.
And, a QEMU command line can actually contain the guest kernel command
line; consider -kernel / -initrd / -append. In that sense, if you break
the guest kernel command line, you break the QEMU command line as well.
> if "-device virtio-mmio" was never supported?
The relevant "-device" options on the qemu command line are not "-device
virtio-mmio"; they are "-device virtio-blk-device". QEMU first
auto-generates the virtio-mmio transports, then assigns those virtio
block devices to the transports. The comment documents the interplay between
- order of virtio mmio transport creation (QEMU),
- order of assigning virtio block devices to transports (QEMU),
- order of naming disks based on transport address (guest kernel).
Anyway, it's not important to include a reference to commit 587078f0ed63.
Thanks
Laszlo
>
>>
>> That's another example showing that board code has to participate in
>> virtio-mmio transport placement.
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> Thanks!
>
^ permalink raw reply [flat|nested] 55+ messages in thread