* [Qemu-devel] [PATCH RFC 1/9] qdev: Replace no_user by cannot_instantiate_with_device_add_yet
2013-10-10 14:42 [Qemu-devel] [PATCH RFC 0/9] Clean up and fix no_user armbru
@ 2013-10-10 14:42 ` armbru
2013-10-10 14:42 ` [Qemu-devel] [PATCH RFC 2/9] sysbus: Set cannot_instantiate_with_device_add_yet armbru
` (9 subsequent siblings)
10 siblings, 0 replies; 38+ messages in thread
From: armbru @ 2013-10-10 14:42 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, afaerber, anthony, peter.maydell
From: Markus Armbruster <armbru@redhat.com>
DeviceClass member no_user used to make device models unavailable with
-device / device_add, but that regressed in commit 18b6dad. All
that's left is the device model is still omitted from help.
Attempts to fix the regression have been rejected with the argument
that the purpose of no_user isn't clear, and it's prone to misuse.
This commit clarifies no_user's purpose. Anthony suggested to rename
it cannot_instantiate_with_device_add_yet_due_to_internal_bugs, which
I shorten somewhat to keep checkpatch happy. While there, make it
bool.
Every use of cannot_instantiate_with_device_add_yet gets a FIXME
comment asking for rationale. The next few commits will clean them
up, either by providing a rationale, or by getting rid of the use.
With that done, the regression fix is hopefully acceptable.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/acpi/piix4.c | 2 +-
hw/alpha/typhoon.c | 2 +-
hw/arm/versatilepb.c | 2 +-
hw/audio/pcspk.c | 2 +-
hw/audio/pl041.c | 2 +-
hw/block/fdc.c | 2 +-
hw/display/pl110.c | 2 +-
hw/dma/pl080.c | 2 +-
hw/i2c/smbus_ich9.c | 2 +-
hw/i386/kvm/clock.c | 2 +-
hw/i386/kvmvapic.c | 2 +-
hw/i386/pc.c | 2 +-
hw/ide/piix.c | 6 +++---
hw/ide/via.c | 2 +-
hw/input/pckbd.c | 2 +-
hw/input/vmmouse.c | 2 +-
hw/intc/apic_common.c | 2 +-
hw/intc/arm_gic.c | 2 +-
hw/intc/arm_gic_common.c | 2 +-
hw/intc/arm_gic_kvm.c | 2 +-
hw/intc/i8259_common.c | 2 +-
hw/intc/ioapic_common.c | 2 +-
hw/intc/pl190.c | 2 +-
hw/isa/isa-bus.c | 2 +-
hw/isa/lpc_ich9.c | 2 +-
hw/isa/piix4.c | 2 +-
hw/isa/vt82c686.c | 2 +-
hw/misc/arm_l2x0.c | 2 +-
hw/misc/vmport.c | 2 +-
hw/nvram/fw_cfg.c | 2 +-
hw/pci-host/bonito.c | 4 ++--
hw/pci-host/grackle.c | 4 ++--
hw/pci-host/piix.c | 8 ++++----
hw/pci-host/prep.c | 4 ++--
hw/ppc/spapr_vio.c | 2 +-
hw/s390x/ipl.c | 2 +-
hw/s390x/s390-virtio-bus.c | 2 +-
hw/s390x/virtio-ccw.c | 2 +-
hw/sd/pl181.c | 2 +-
hw/timer/arm_mptimer.c | 2 +-
hw/timer/hpet.c | 2 +-
hw/timer/i8254_common.c | 2 +-
hw/timer/m48t59.c | 2 +-
hw/timer/mc146818rtc.c | 2 +-
hw/timer/pl031.c | 2 +-
include/hw/qdev-core.h | 13 ++++++++++++-
qdev-monitor.c | 5 +++--
qom/cpu.c | 2 +-
48 files changed, 69 insertions(+), 57 deletions(-)
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index b46bd5e..c29a703 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -508,7 +508,7 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
k->revision = 0x03;
k->class_id = PCI_CLASS_BRIDGE_OTHER;
dc->desc = "PM";
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->vmsd = &vmstate_acpi;
dc->props = piix4_pm_properties;
}
diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c
index aac9a32..40798d0 100644
--- a/hw/alpha/typhoon.c
+++ b/hw/alpha/typhoon.c
@@ -938,7 +938,7 @@ static void typhoon_pcihost_class_init(ObjectClass *klass, void *data)
SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
k->init = typhoon_pcihost_init;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo typhoon_pcihost_info = {
diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
index f7e8b7e..bb0c0ba 100644
--- a/hw/arm/versatilepb.c
+++ b/hw/arm/versatilepb.c
@@ -390,7 +390,7 @@ static void vpb_sic_class_init(ObjectClass *klass, void *data)
SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
k->init = vpb_sic_init;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->vmsd = &vmstate_vpb_sic;
}
diff --git a/hw/audio/pcspk.c b/hw/audio/pcspk.c
index 9004ce3..8e3e178 100644
--- a/hw/audio/pcspk.c
+++ b/hw/audio/pcspk.c
@@ -192,7 +192,7 @@ static void pcspk_class_initfn(ObjectClass *klass, void *data)
dc->realize = pcspk_realizefn;
set_bit(DEVICE_CATEGORY_SOUND, dc->categories);
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->props = pcspk_properties;
}
diff --git a/hw/audio/pl041.c b/hw/audio/pl041.c
index 5393b52..8ba661a 100644
--- a/hw/audio/pl041.c
+++ b/hw/audio/pl041.c
@@ -632,7 +632,7 @@ static void pl041_device_class_init(ObjectClass *klass, void *data)
k->init = pl041_init;
set_bit(DEVICE_CATEGORY_SOUND, dc->categories);
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->reset = pl041_device_reset;
dc->vmsd = &vmstate_pl041;
dc->props = pl041_device_properties;
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index c5a6c21..86f4920 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2234,7 +2234,7 @@ static void isabus_fdc_class_init(ObjectClass *klass, void *data)
dc->realize = isabus_fdc_realize;
dc->fw_name = "fdc";
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->reset = fdctrl_external_reset_isa;
dc->vmsd = &vmstate_isa_fdc;
dc->props = isa_fdc_properties;
diff --git a/hw/display/pl110.c b/hw/display/pl110.c
index 790e510..7ad5972 100644
--- a/hw/display/pl110.c
+++ b/hw/display/pl110.c
@@ -496,7 +496,7 @@ static void pl110_class_init(ObjectClass *klass, void *data)
k->init = pl110_initfn;
set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->vmsd = &vmstate_pl110;
}
diff --git a/hw/dma/pl080.c b/hw/dma/pl080.c
index 35b9015..a515621 100644
--- a/hw/dma/pl080.c
+++ b/hw/dma/pl080.c
@@ -381,7 +381,7 @@ static void pl080_class_init(ObjectClass *oc, void *data)
{
DeviceClass *dc = DEVICE_CLASS(oc);
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->vmsd = &vmstate_pl080;
}
diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
index ca22978..c1ffa34 100644
--- a/hw/i2c/smbus_ich9.c
+++ b/hw/i2c/smbus_ich9.c
@@ -97,7 +97,7 @@ static void ich9_smb_class_init(ObjectClass *klass, void *data)
k->device_id = PCI_DEVICE_ID_INTEL_ICH9_6;
k->revision = ICH9_A2_SMB_REVISION;
k->class_id = PCI_CLASS_SERIAL_SMBUS;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->vmsd = &vmstate_ich9_smbus;
dc->desc = "ICH9 SMBUS Bridge";
k->init = ich9_smbus_initfn;
diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
index 383938d..abd2ce8 100644
--- a/hw/i386/kvm/clock.c
+++ b/hw/i386/kvm/clock.c
@@ -114,7 +114,7 @@ static void kvmclock_class_init(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
dc->realize = kvmclock_realize;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->vmsd = &kvmclock_vmsd;
}
diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
index 1c2dbf5..8ec3151 100644
--- a/hw/i386/kvmvapic.c
+++ b/hw/i386/kvmvapic.c
@@ -824,7 +824,7 @@ static void vapic_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->reset = vapic_reset;
dc->vmsd = &vmstate_vapic;
dc->realize = vapic_realize;
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 0c313fe..fe33843 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -544,7 +544,7 @@ static void port92_class_initfn(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->realize = port92_realizefn;
dc->reset = port92_reset;
dc->vmsd = &vmstate_port92_isa;
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index ab36749..27b08e1 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -248,7 +248,7 @@ static void piix3_ide_class_init(ObjectClass *klass, void *data)
k->device_id = PCI_DEVICE_ID_INTEL_82371SB_1;
k->class_id = PCI_CLASS_STORAGE_IDE;
set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo piix3_ide_info = {
@@ -267,7 +267,7 @@ static void piix3_ide_xen_class_init(ObjectClass *klass, void *data)
k->device_id = PCI_DEVICE_ID_INTEL_82371SB_1;
k->class_id = PCI_CLASS_STORAGE_IDE;
set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->unplug = pci_piix3_xen_ide_unplug;
}
@@ -289,7 +289,7 @@ static void piix4_ide_class_init(ObjectClass *klass, void *data)
k->device_id = PCI_DEVICE_ID_INTEL_82371AB;
k->class_id = PCI_CLASS_STORAGE_IDE;
set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo piix4_ide_info = {
diff --git a/hw/ide/via.c b/hw/ide/via.c
index 99468c7..b556c14 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -225,7 +225,7 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
k->revision = 0x06;
k->class_id = PCI_CLASS_STORAGE_IDE;
set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo via_ide_info = {
diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index ce86237..dee31a6 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -522,7 +522,7 @@ static void i8042_class_initfn(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
dc->realize = i8042_realizefn;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->vmsd = &vmstate_kbd_isa;
}
diff --git a/hw/input/vmmouse.c b/hw/input/vmmouse.c
index abd032b..600e4a2 100644
--- a/hw/input/vmmouse.c
+++ b/hw/input/vmmouse.c
@@ -282,7 +282,7 @@ static void vmmouse_class_initfn(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
dc->realize = vmmouse_realizefn;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->reset = vmmouse_reset;
dc->vmsd = &vmstate_vmmouse;
dc->props = vmmouse_properties;
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index a0beb10..ea420c7 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -386,7 +386,7 @@ static void apic_common_class_init(ObjectClass *klass, void *data)
dc->vmsd = &vmstate_apic_common;
dc->reset = apic_reset_common;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->props = apic_properties_common;
idc->init = apic_init_common;
}
diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index d431b7a..24ad276 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -704,7 +704,7 @@ static void arm_gic_class_init(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
ARMGICClass *agc = ARM_GIC_CLASS(klass);
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
agc->parent_realize = dc->realize;
dc->realize = arm_gic_realize;
}
diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index 709b5c2..9047143 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -156,7 +156,7 @@ static void arm_gic_common_class_init(ObjectClass *klass, void *data)
dc->realize = arm_gic_common_realize;
dc->props = arm_gic_common_properties;
dc->vmsd = &vmstate_gic;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo arm_gic_common_type = {
diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
index f713975..a0bbf12 100644
--- a/hw/intc/arm_gic_kvm.c
+++ b/hw/intc/arm_gic_kvm.c
@@ -150,7 +150,7 @@ static void kvm_arm_gic_class_init(ObjectClass *klass, void *data)
kgc->parent_reset = dc->reset;
dc->realize = kvm_arm_gic_realize;
dc->reset = kvm_arm_gic_reset;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo kvm_arm_gic_info = {
diff --git a/hw/intc/i8259_common.c b/hw/intc/i8259_common.c
index 803d037..2acdbfe 100644
--- a/hw/intc/i8259_common.c
+++ b/hw/intc/i8259_common.c
@@ -135,7 +135,7 @@ static void pic_common_class_init(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
dc->vmsd = &vmstate_pic_common;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->props = pic_properties_common;
dc->realize = pic_common_realize;
}
diff --git a/hw/intc/ioapic_common.c b/hw/intc/ioapic_common.c
index 6b705c1..cc5a80d 100644
--- a/hw/intc/ioapic_common.c
+++ b/hw/intc/ioapic_common.c
@@ -98,7 +98,7 @@ static void ioapic_common_class_init(ObjectClass *klass, void *data)
dc->realize = ioapic_common_realize;
dc->vmsd = &vmstate_ioapic_common;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo ioapic_common_type = {
diff --git a/hw/intc/pl190.c b/hw/intc/pl190.c
index 329680d..b16bc02 100644
--- a/hw/intc/pl190.c
+++ b/hw/intc/pl190.c
@@ -273,7 +273,7 @@ static void pl190_class_init(ObjectClass *klass, void *data)
SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
k->init = pl190_init;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->reset = pl190_reset;
dc->vmsd = &vmstate_pl190;
}
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 9e104eb..6b2114d 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -197,7 +197,7 @@ static void isabus_bridge_class_init(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
dc->fw_name = "isa";
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo isabus_bridge_info = {
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 5633d08..ad841b5 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -604,7 +604,7 @@ static void ich9_lpc_class_init(ObjectClass *klass, void *data)
dc->reset = ich9_lpc_reset;
k->init = ich9_lpc_initfn;
dc->vmsd = &vmstate_ich9_lpc;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
k->config_write = ich9_lpc_config_write;
dc->desc = "ICH9 LPC bridge";
k->vendor_id = PCI_VENDOR_ID_INTEL;
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 1a1d451..d9dac61 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -113,7 +113,7 @@ static void piix4_class_init(ObjectClass *klass, void *data)
k->device_id = PCI_DEVICE_ID_INTEL_82371AB_0;
k->class_id = PCI_CLASS_BRIDGE_ISA;
dc->desc = "ISA bridge";
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->vmsd = &vmstate_piix4;
}
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 8fe4fcb..3e8ec80 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -480,7 +480,7 @@ static void via_class_init(ObjectClass *klass, void *data)
k->class_id = PCI_CLASS_BRIDGE_ISA;
k->revision = 0x40;
dc->desc = "ISA bridge";
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->vmsd = &vmstate_via;
}
diff --git a/hw/misc/arm_l2x0.c b/hw/misc/arm_l2x0.c
index 8e192cd..ceea99d 100644
--- a/hw/misc/arm_l2x0.c
+++ b/hw/misc/arm_l2x0.c
@@ -179,7 +179,7 @@ static void l2x0_class_init(ObjectClass *klass, void *data)
k->init = l2x0_priv_init;
dc->vmsd = &vmstate_l2x0;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->props = l2x0_properties;
dc->reset = l2x0_priv_reset;
}
diff --git a/hw/misc/vmport.c b/hw/misc/vmport.c
index 0b5a564..94ae6ae 100644
--- a/hw/misc/vmport.c
+++ b/hw/misc/vmport.c
@@ -162,7 +162,7 @@ static void vmport_class_initfn(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
dc->realize = vmport_realizefn;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo vmport_info = {
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index d0820e5..553599f 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -576,7 +576,7 @@ static void fw_cfg_class_init(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
dc->realize = fw_cfg_realize;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->reset = fw_cfg_reset;
dc->vmsd = &vmstate_fw_cfg;
dc->props = fw_cfg_properties;
diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
index 5086d42..2e08e9d 100644
--- a/hw/pci-host/bonito.c
+++ b/hw/pci-host/bonito.c
@@ -806,7 +806,7 @@ static void bonito_class_init(ObjectClass *klass, void *data)
k->revision = 0x01;
k->class_id = PCI_CLASS_BRIDGE_HOST;
dc->desc = "Host bridge";
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->vmsd = &vmstate_bonito;
}
@@ -823,7 +823,7 @@ static void bonito_pcihost_class_init(ObjectClass *klass, void *data)
SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
k->init = bonito_pcihost_initfn;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo bonito_pcihost_info = {
diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c
index 4991ec4..a2c5f56 100644
--- a/hw/pci-host/grackle.c
+++ b/hw/pci-host/grackle.c
@@ -130,7 +130,7 @@ static void grackle_pci_class_init(ObjectClass *klass, void *data)
k->device_id = PCI_DEVICE_ID_MOTOROLA_MPC106;
k->revision = 0x00;
k->class_id = PCI_CLASS_BRIDGE_HOST;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo grackle_pci_info = {
@@ -146,7 +146,7 @@ static void pci_grackle_class_init(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
k->init = pci_grackle_init_device;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo grackle_pci_host_info = {
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index c041149..697de65 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -644,7 +644,7 @@ static void piix3_class_init(ObjectClass *klass, void *data)
dc->desc = "ISA bridge";
dc->vmsd = &vmstate_piix3;
- dc->no_user = 1,
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
k->no_hotplug = 1;
k->init = piix3_initfn;
k->config_write = piix3_write_config;
@@ -668,7 +668,7 @@ static void piix3_xen_class_init(ObjectClass *klass, void *data)
dc->desc = "ISA bridge";
dc->vmsd = &vmstate_piix3;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
k->no_hotplug = 1;
k->init = piix3_initfn;
k->config_write = piix3_write_config_xen;
@@ -698,7 +698,7 @@ static void i440fx_class_init(ObjectClass *klass, void *data)
k->revision = 0x02;
k->class_id = PCI_CLASS_BRIDGE_HOST;
dc->desc = "Host bridge";
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->vmsd = &vmstate_i440fx;
}
@@ -730,7 +730,7 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
hc->root_bus_path = i440fx_pcihost_root_bus_path;
dc->realize = i440fx_pcihost_realize;
dc->fw_name = "pci";
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->props = i440fx_props;
}
diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
index 0e71fdb..58b8c5e 100644
--- a/hw/pci-host/prep.c
+++ b/hw/pci-host/prep.c
@@ -198,7 +198,7 @@ static void raven_class_init(ObjectClass *klass, void *data)
k->class_id = PCI_CLASS_BRIDGE_HOST;
dc->desc = "PReP Host Bridge - Motorola Raven";
dc->vmsd = &vmstate_raven;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo raven_info = {
@@ -215,7 +215,7 @@ static void raven_pcihost_class_init(ObjectClass *klass, void *data)
set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
dc->realize = raven_pcihost_realizefn;
dc->fw_name = "pci";
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo raven_pcihost_info = {
diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index a6a0a51..1e33819 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -532,7 +532,7 @@ static void spapr_vio_bridge_class_init(ObjectClass *klass, void *data)
SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
k->init = spapr_vio_bridge_init;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo spapr_vio_bridge_info = {
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index d69adb2..f86a4af 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -181,7 +181,7 @@ static void s390_ipl_class_init(ObjectClass *klass, void *data)
k->init = s390_ipl_init;
dc->props = s390_ipl_properties;
dc->reset = s390_ipl_reset;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo s390_ipl_info = {
diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index 6a83111..eccc3e7 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -680,7 +680,7 @@ static void s390_virtio_bridge_class_init(ObjectClass *klass, void *data)
SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
k->init = s390_virtio_bridge_init;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo s390_virtio_bridge_info = {
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index cd67db5..df13b70 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1278,7 +1278,7 @@ static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
k->init = virtual_css_bridge_init;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo virtual_css_bridge_info = {
diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
index 03875bf..6342c8b 100644
--- a/hw/sd/pl181.c
+++ b/hw/sd/pl181.c
@@ -502,7 +502,7 @@ static void pl181_class_init(ObjectClass *klass, void *data)
sdc->init = pl181_init;
k->vmsd = &vmstate_pl181;
k->reset = pl181_reset;
- k->no_user = 1;
+ k->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo pl181_info = {
diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
index 8020c9f..f9cdeea 100644
--- a/hw/timer/arm_mptimer.c
+++ b/hw/timer/arm_mptimer.c
@@ -297,7 +297,7 @@ static void arm_mptimer_class_init(ObjectClass *klass, void *data)
sbc->init = arm_mptimer_init;
dc->vmsd = &vmstate_arm_mptimer;
dc->reset = arm_mptimer_reset;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->props = arm_mptimer_properties;
}
diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index fcd22ae..3777764 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -751,7 +751,7 @@ static void hpet_device_class_init(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
dc->realize = hpet_realize;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->reset = hpet_reset;
dc->vmsd = &vmstate_hpet;
dc->props = hpet_device_properties;
diff --git a/hw/timer/i8254_common.c b/hw/timer/i8254_common.c
index e8fb971..dc2196c 100644
--- a/hw/timer/i8254_common.c
+++ b/hw/timer/i8254_common.c
@@ -282,7 +282,7 @@ static void pit_common_class_init(ObjectClass *klass, void *data)
dc->realize = pit_common_realize;
dc->vmsd = &vmstate_pit_common;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo pit_common_type = {
diff --git a/hw/timer/m48t59.c b/hw/timer/m48t59.c
index d3d78ec..f81cf48 100644
--- a/hw/timer/m48t59.c
+++ b/hw/timer/m48t59.c
@@ -750,7 +750,7 @@ static void m48t59_isa_class_init(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
dc->realize = m48t59_isa_realize;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->reset = m48t59_reset_isa;
dc->props = m48t59_isa_properties;
}
diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 7230a6e..2f58220 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -906,7 +906,7 @@ static void rtc_class_initfn(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
dc->realize = rtc_realizefn;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->vmsd = &vmstate_rtc;
dc->props = mc146818rtc_properties;
}
diff --git a/hw/timer/pl031.c b/hw/timer/pl031.c
index 65928a4..2f7360c 100644
--- a/hw/timer/pl031.c
+++ b/hw/timer/pl031.c
@@ -251,7 +251,7 @@ static void pl031_class_init(ObjectClass *klass, void *data)
SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
k->init = pl031_init;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->vmsd = &vmstate_pl031;
}
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index e191ca0..2b571d7 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -97,7 +97,18 @@ typedef struct DeviceClass {
const char *fw_name;
const char *desc;
Property *props;
- int no_user;
+
+ /*
+ * Shall we hide this device model from -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
+ */
+ bool cannot_instantiate_with_device_add_yet;
/* callbacks */
void (*reset)(DeviceState *dev);
diff --git a/qdev-monitor.c b/qdev-monitor.c
index a02c925..36f6f09 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -87,7 +87,7 @@ static void qdev_print_devinfo(DeviceClass *dc)
if (dc->desc) {
error_printf(", desc \"%s\"", dc->desc);
}
- if (dc->no_user) {
+ if (dc->cannot_instantiate_with_device_add_yet) {
error_printf(", no-user");
}
error_printf("\n");
@@ -127,7 +127,8 @@ static void qdev_print_devinfos(bool show_no_user)
if ((i < DEVICE_CATEGORY_MAX
? !test_bit(i, dc->categories)
: !bitmap_empty(dc->categories, DEVICE_CATEGORY_MAX))
- || (!show_no_user && dc->no_user)) {
+ || (!show_no_user
+ && dc->cannot_instantiate_with_device_add_yet)) {
continue;
}
if (!cat_printed) {
diff --git a/qom/cpu.c b/qom/cpu.c
index 818fb26..09c15e6 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -254,7 +254,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
k->gdb_read_register = cpu_common_gdb_read_register;
k->gdb_write_register = cpu_common_gdb_write_register;
dc->realize = cpu_common_realizefn;
- dc->no_user = 1;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo cpu_type_info = {
--
1.8.1.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH RFC 2/9] sysbus: Set cannot_instantiate_with_device_add_yet
2013-10-10 14:42 [Qemu-devel] [PATCH RFC 0/9] Clean up and fix no_user armbru
2013-10-10 14:42 ` [Qemu-devel] [PATCH RFC 1/9] qdev: Replace no_user by cannot_instantiate_with_device_add_yet armbru
@ 2013-10-10 14:42 ` armbru
2013-10-10 14:42 ` [Qemu-devel] [PATCH RFC 3/9] apic: Document why cannot_instantiate_with_device_add_yet armbru
` (8 subsequent siblings)
10 siblings, 0 replies; 38+ messages in thread
From: armbru @ 2013-10-10 14:42 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, afaerber, anthony, peter.maydell
From: Markus Armbruster <armbru@redhat.com>
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 unconncected, and could not possibly work.
Many, but not all sysbus devices alreasy set
cannot_instantiate_with_device_add_yet in their class init function.
Set it in their abstract base's class init function
sysbus_device_class_init(), and remove the now redundant assignments
from device class init functions.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/alpha/typhoon.c | 2 --
hw/arm/versatilepb.c | 1 -
hw/audio/pl041.c | 1 -
hw/core/sysbus.c | 7 +++++++
hw/display/pl110.c | 1 -
hw/dma/pl080.c | 1 -
hw/i386/kvm/clock.c | 1 -
hw/i386/kvmvapic.c | 1 -
hw/intc/arm_gic.c | 1 -
hw/intc/arm_gic_common.c | 1 -
hw/intc/arm_gic_kvm.c | 1 -
hw/intc/ioapic_common.c | 1 -
hw/intc/pl190.c | 1 -
hw/isa/isa-bus.c | 1 -
hw/misc/arm_l2x0.c | 1 -
hw/nvram/fw_cfg.c | 1 -
hw/pci-host/bonito.c | 2 --
hw/pci-host/grackle.c | 2 --
hw/pci-host/piix.c | 1 -
hw/pci-host/prep.c | 1 -
hw/ppc/spapr_vio.c | 2 --
hw/s390x/ipl.c | 1 -
hw/s390x/s390-virtio-bus.c | 2 --
hw/s390x/virtio-ccw.c | 2 --
hw/sd/pl181.c | 1 -
hw/timer/arm_mptimer.c | 1 -
hw/timer/hpet.c | 1 -
hw/timer/pl031.c | 1 -
28 files changed, 7 insertions(+), 33 deletions(-)
diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c
index 40798d0..bb58a22 100644
--- a/hw/alpha/typhoon.c
+++ b/hw/alpha/typhoon.c
@@ -934,11 +934,9 @@ static int typhoon_pcihost_init(SysBusDevice *dev)
static void typhoon_pcihost_class_init(ObjectClass *klass, void *data)
{
- DeviceClass *dc = DEVICE_CLASS(klass);
SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
k->init = typhoon_pcihost_init;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo typhoon_pcihost_info = {
diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
index bb0c0ba..aef2bde 100644
--- a/hw/arm/versatilepb.c
+++ b/hw/arm/versatilepb.c
@@ -390,7 +390,6 @@ static void vpb_sic_class_init(ObjectClass *klass, void *data)
SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
k->init = vpb_sic_init;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->vmsd = &vmstate_vpb_sic;
}
diff --git a/hw/audio/pl041.c b/hw/audio/pl041.c
index 8ba661a..ed82be5 100644
--- a/hw/audio/pl041.c
+++ b/hw/audio/pl041.c
@@ -632,7 +632,6 @@ static void pl041_device_class_init(ObjectClass *klass, void *data)
k->init = pl041_init;
set_bit(DEVICE_CATEGORY_SOUND, dc->categories);
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->reset = pl041_device_reset;
dc->vmsd = &vmstate_pl041;
dc->props = pl041_device_properties;
diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index b84cd4a..6e880a8 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -257,6 +257,13 @@ 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 unconncected, and could not possibly work.
+ */
+ k->cannot_instantiate_with_device_add_yet = true;
}
static const TypeInfo sysbus_device_type_info = {
diff --git a/hw/display/pl110.c b/hw/display/pl110.c
index 7ad5972..ab689e9 100644
--- a/hw/display/pl110.c
+++ b/hw/display/pl110.c
@@ -496,7 +496,6 @@ static void pl110_class_init(ObjectClass *klass, void *data)
k->init = pl110_initfn;
set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->vmsd = &vmstate_pl110;
}
diff --git a/hw/dma/pl080.c b/hw/dma/pl080.c
index a515621..cb7bda9 100644
--- a/hw/dma/pl080.c
+++ b/hw/dma/pl080.c
@@ -381,7 +381,6 @@ static void pl080_class_init(ObjectClass *oc, void *data)
{
DeviceClass *dc = DEVICE_CLASS(oc);
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->vmsd = &vmstate_pl080;
}
diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
index abd2ce8..892aa02 100644
--- a/hw/i386/kvm/clock.c
+++ b/hw/i386/kvm/clock.c
@@ -114,7 +114,6 @@ static void kvmclock_class_init(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
dc->realize = kvmclock_realize;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->vmsd = &kvmclock_vmsd;
}
diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
index 8ec3151..5cc894a 100644
--- a/hw/i386/kvmvapic.c
+++ b/hw/i386/kvmvapic.c
@@ -824,7 +824,6 @@ static void vapic_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->reset = vapic_reset;
dc->vmsd = &vmstate_vapic;
dc->realize = vapic_realize;
diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 24ad276..f13a927 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -704,7 +704,6 @@ static void arm_gic_class_init(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
ARMGICClass *agc = ARM_GIC_CLASS(klass);
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
agc->parent_realize = dc->realize;
dc->realize = arm_gic_realize;
}
diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index 9047143..84aa7fc 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -156,7 +156,6 @@ static void arm_gic_common_class_init(ObjectClass *klass, void *data)
dc->realize = arm_gic_common_realize;
dc->props = arm_gic_common_properties;
dc->vmsd = &vmstate_gic;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo arm_gic_common_type = {
diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
index a0bbf12..59a3da5 100644
--- a/hw/intc/arm_gic_kvm.c
+++ b/hw/intc/arm_gic_kvm.c
@@ -150,7 +150,6 @@ static void kvm_arm_gic_class_init(ObjectClass *klass, void *data)
kgc->parent_reset = dc->reset;
dc->realize = kvm_arm_gic_realize;
dc->reset = kvm_arm_gic_reset;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo kvm_arm_gic_info = {
diff --git a/hw/intc/ioapic_common.c b/hw/intc/ioapic_common.c
index cc5a80d..9ba1a26 100644
--- a/hw/intc/ioapic_common.c
+++ b/hw/intc/ioapic_common.c
@@ -98,7 +98,6 @@ static void ioapic_common_class_init(ObjectClass *klass, void *data)
dc->realize = ioapic_common_realize;
dc->vmsd = &vmstate_ioapic_common;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo ioapic_common_type = {
diff --git a/hw/intc/pl190.c b/hw/intc/pl190.c
index b16bc02..2bf359a 100644
--- a/hw/intc/pl190.c
+++ b/hw/intc/pl190.c
@@ -273,7 +273,6 @@ static void pl190_class_init(ObjectClass *klass, void *data)
SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
k->init = pl190_init;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->reset = pl190_reset;
dc->vmsd = &vmstate_pl190;
}
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 6b2114d..55d0100 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -197,7 +197,6 @@ static void isabus_bridge_class_init(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
dc->fw_name = "isa";
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo isabus_bridge_info = {
diff --git a/hw/misc/arm_l2x0.c b/hw/misc/arm_l2x0.c
index ceea99d..9e220c9 100644
--- a/hw/misc/arm_l2x0.c
+++ b/hw/misc/arm_l2x0.c
@@ -179,7 +179,6 @@ static void l2x0_class_init(ObjectClass *klass, void *data)
k->init = l2x0_priv_init;
dc->vmsd = &vmstate_l2x0;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->props = l2x0_properties;
dc->reset = l2x0_priv_reset;
}
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 553599f..9bcfde2 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -576,7 +576,6 @@ static void fw_cfg_class_init(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
dc->realize = fw_cfg_realize;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->reset = fw_cfg_reset;
dc->vmsd = &vmstate_fw_cfg;
dc->props = fw_cfg_properties;
diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
index 2e08e9d..bfb9820 100644
--- a/hw/pci-host/bonito.c
+++ b/hw/pci-host/bonito.c
@@ -819,11 +819,9 @@ static const TypeInfo bonito_info = {
static void bonito_pcihost_class_init(ObjectClass *klass, void *data)
{
- DeviceClass *dc = DEVICE_CLASS(klass);
SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
k->init = bonito_pcihost_initfn;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo bonito_pcihost_info = {
diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c
index a2c5f56..c178375 100644
--- a/hw/pci-host/grackle.c
+++ b/hw/pci-host/grackle.c
@@ -143,10 +143,8 @@ static const TypeInfo grackle_pci_info = {
static void pci_grackle_class_init(ObjectClass *klass, void *data)
{
SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
- DeviceClass *dc = DEVICE_CLASS(klass);
k->init = pci_grackle_init_device;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo grackle_pci_host_info = {
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 697de65..21ffe97 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -730,7 +730,6 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
hc->root_bus_path = i440fx_pcihost_root_bus_path;
dc->realize = i440fx_pcihost_realize;
dc->fw_name = "pci";
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->props = i440fx_props;
}
diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
index 58b8c5e..ebc40c6 100644
--- a/hw/pci-host/prep.c
+++ b/hw/pci-host/prep.c
@@ -215,7 +215,6 @@ static void raven_pcihost_class_init(ObjectClass *klass, void *data)
set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
dc->realize = raven_pcihost_realizefn;
dc->fw_name = "pci";
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo raven_pcihost_info = {
diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index 1e33819..9ac15b5 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -528,11 +528,9 @@ static int spapr_vio_bridge_init(SysBusDevice *dev)
static void spapr_vio_bridge_class_init(ObjectClass *klass, void *data)
{
- DeviceClass *dc = DEVICE_CLASS(klass);
SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
k->init = spapr_vio_bridge_init;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo spapr_vio_bridge_info = {
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index f86a4af..cc29d8e 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -181,7 +181,6 @@ static void s390_ipl_class_init(ObjectClass *klass, void *data)
k->init = s390_ipl_init;
dc->props = s390_ipl_properties;
dc->reset = s390_ipl_reset;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo s390_ipl_info = {
diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index eccc3e7..46c5ff1 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -676,11 +676,9 @@ static int s390_virtio_bridge_init(SysBusDevice *dev)
static void s390_virtio_bridge_class_init(ObjectClass *klass, void *data)
{
- DeviceClass *dc = DEVICE_CLASS(klass);
SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
k->init = s390_virtio_bridge_init;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo s390_virtio_bridge_info = {
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index df13b70..71a7e66 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1274,11 +1274,9 @@ static int virtual_css_bridge_init(SysBusDevice *dev)
static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
{
- DeviceClass *dc = DEVICE_CLASS(klass);
SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
k->init = virtual_css_bridge_init;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo virtual_css_bridge_info = {
diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
index 6342c8b..fcede25 100644
--- a/hw/sd/pl181.c
+++ b/hw/sd/pl181.c
@@ -502,7 +502,6 @@ static void pl181_class_init(ObjectClass *klass, void *data)
sdc->init = pl181_init;
k->vmsd = &vmstate_pl181;
k->reset = pl181_reset;
- k->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo pl181_info = {
diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
index f9cdeea..4c59699 100644
--- a/hw/timer/arm_mptimer.c
+++ b/hw/timer/arm_mptimer.c
@@ -297,7 +297,6 @@ static void arm_mptimer_class_init(ObjectClass *klass, void *data)
sbc->init = arm_mptimer_init;
dc->vmsd = &vmstate_arm_mptimer;
dc->reset = arm_mptimer_reset;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->props = arm_mptimer_properties;
}
diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 3777764..446a591 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -751,7 +751,6 @@ static void hpet_device_class_init(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
dc->realize = hpet_realize;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->reset = hpet_reset;
dc->vmsd = &vmstate_hpet;
dc->props = hpet_device_properties;
diff --git a/hw/timer/pl031.c b/hw/timer/pl031.c
index 2f7360c..34d9b44 100644
--- a/hw/timer/pl031.c
+++ b/hw/timer/pl031.c
@@ -251,7 +251,6 @@ static void pl031_class_init(ObjectClass *klass, void *data)
SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
k->init = pl031_init;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->vmsd = &vmstate_pl031;
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH RFC 3/9] apic: Document why cannot_instantiate_with_device_add_yet
2013-10-10 14:42 [Qemu-devel] [PATCH RFC 0/9] Clean up and fix no_user armbru
2013-10-10 14:42 ` [Qemu-devel] [PATCH RFC 1/9] qdev: Replace no_user by cannot_instantiate_with_device_add_yet armbru
2013-10-10 14:42 ` [Qemu-devel] [PATCH RFC 2/9] sysbus: Set cannot_instantiate_with_device_add_yet armbru
@ 2013-10-10 14:42 ` armbru
2013-10-10 14:42 ` [Qemu-devel] [PATCH RFC 4/9] pci-host: Consistently set cannot_instantiate_with_device_add_yet armbru
` (7 subsequent siblings)
10 siblings, 0 replies; 38+ messages in thread
From: armbru @ 2013-10-10 14:42 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, afaerber, anthony, peter.maydell
From: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/intc/apic_common.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index ea420c7..aaef054 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -386,9 +386,13 @@ static void apic_common_class_init(ObjectClass *klass, void *data)
dc->vmsd = &vmstate_apic_common;
dc->reset = apic_reset_common;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->props = apic_properties_common;
idc->init = apic_init_common;
+ /*
+ * Reason: APIC and CPU need to be wired up by
+ * x86_cpu_apic_create()
+ */
+ dc->cannot_instantiate_with_device_add_yet = true;
}
static const TypeInfo apic_common_type = {
--
1.8.1.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH RFC 4/9] pci-host: Consistently set cannot_instantiate_with_device_add_yet
2013-10-10 14:42 [Qemu-devel] [PATCH RFC 0/9] Clean up and fix no_user armbru
` (2 preceding siblings ...)
2013-10-10 14:42 ` [Qemu-devel] [PATCH RFC 3/9] apic: Document why cannot_instantiate_with_device_add_yet armbru
@ 2013-10-10 14:42 ` armbru
2013-10-10 14:42 ` [Qemu-devel] [PATCH RFC 5/9] ich9: Document why cannot_instantiate_with_device_add_yet armbru
` (6 subsequent siblings)
10 siblings, 0 replies; 38+ messages in thread
From: armbru @ 2013-10-10 14:42 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, afaerber, anthony, peter.maydell
From: Markus Armbruster <armbru@redhat.com>
Many PCI host bridges consist of a sysbus device and a PCI device.
You need both for the thing to work. Arguably, these bridges should
be modelled as a single, composite devices instead of pairs of
seemingly independent devices you can only use together, but we're not
there, yet.
Since the sysbus part can't be instantiated with device_add, yet,
permitting it with the PCI part is useless. We shouldn't offer
useless options to the user, so let's set
cannot_instantiate_with_device_add_yet for them.
It's already set for Bonito, grackle, i440FX, and raven. Document
why.
Set it for the others: dec-21154, e500-host-bridge, gt64120_pci,
pbm-pci, ppc4xx-host-bridge, sh_pci_host, u3-agp, uni-north-agp,
uni-north-internal-pci, uni-north-pci, and versatile_pci_host.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/mips/gt64xxx_pci.c | 5 +++++
hw/pci-bridge/dec.c | 5 +++++
hw/pci-host/apb.c | 5 +++++
hw/pci-host/bonito.c | 6 +++++-
hw/pci-host/grackle.c | 6 +++++-
hw/pci-host/piix.c | 6 +++++-
hw/pci-host/ppce500.c | 5 +++++
hw/pci-host/prep.c | 6 +++++-
hw/pci-host/q35.c | 5 +++++
hw/pci-host/uninorth.c | 20 ++++++++++++++++++++
hw/pci-host/versatile.c | 5 +++++
hw/ppc/ppc4xx_pci.c | 5 +++++
hw/sh4/sh_pci.c | 5 +++++
13 files changed, 80 insertions(+), 4 deletions(-)
diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
index 3da2e67..4756bf1 100644
--- a/hw/mips/gt64xxx_pci.c
+++ b/hw/mips/gt64xxx_pci.c
@@ -1157,6 +1157,11 @@ static void gt64120_pci_class_init(ObjectClass *klass, void *data)
k->device_id = PCI_DEVICE_ID_MARVELL_GT6412X;
k->revision = 0x10;
k->class_id = PCI_CLASS_BRIDGE_HOST;
+ /*
+ * PCI-facing part of the host bridge, not usable without the
+ * host-facing part, which can't be device_add'ed, yet.
+ */
+ k->parent_class.cannot_instantiate_with_device_add_yet = true;
}
static const TypeInfo gt64120_pci_info = {
diff --git a/hw/pci-bridge/dec.c b/hw/pci-bridge/dec.c
index e5e3be8..e27ecfc 100644
--- a/hw/pci-bridge/dec.c
+++ b/hw/pci-bridge/dec.c
@@ -123,6 +123,11 @@ static void dec_21154_pci_host_class_init(ObjectClass *klass, void *data)
k->revision = 0x02;
k->class_id = PCI_CLASS_BRIDGE_PCI;
k->is_bridge = 1;
+ /*
+ * PCI-facing part of the host bridge, not usable without the
+ * host-facing part, which can't be device_add'ed, yet.
+ */
+ k->parent_class.cannot_instantiate_with_device_add_yet = true;
}
static const TypeInfo dec_21154_pci_host_info = {
diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c
index 92f289f..b8df0a6 100644
--- a/hw/pci-host/apb.c
+++ b/hw/pci-host/apb.c
@@ -521,6 +521,11 @@ static void pbm_pci_host_class_init(ObjectClass *klass, void *data)
k->vendor_id = PCI_VENDOR_ID_SUN;
k->device_id = PCI_DEVICE_ID_SUN_SABRE;
k->class_id = PCI_CLASS_BRIDGE_HOST;
+ /*
+ * PCI-facing part of the host bridge, not usable without the
+ * host-facing part, which can't be device_add'ed, yet.
+ */
+ k->parent_class.cannot_instantiate_with_device_add_yet = true;
}
static const TypeInfo pbm_pci_host_info = {
diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
index bfb9820..902441f 100644
--- a/hw/pci-host/bonito.c
+++ b/hw/pci-host/bonito.c
@@ -806,8 +806,12 @@ static void bonito_class_init(ObjectClass *klass, void *data)
k->revision = 0x01;
k->class_id = PCI_CLASS_BRIDGE_HOST;
dc->desc = "Host bridge";
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->vmsd = &vmstate_bonito;
+ /*
+ * 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;
}
static const TypeInfo bonito_info = {
diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c
index c178375..7d95821 100644
--- a/hw/pci-host/grackle.c
+++ b/hw/pci-host/grackle.c
@@ -130,7 +130,11 @@ static void grackle_pci_class_init(ObjectClass *klass, void *data)
k->device_id = PCI_DEVICE_ID_MOTOROLA_MPC106;
k->revision = 0x00;
k->class_id = PCI_CLASS_BRIDGE_HOST;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
+ /*
+ * 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;
}
static const TypeInfo grackle_pci_info = {
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 21ffe97..8089fd6 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -698,8 +698,12 @@ static void i440fx_class_init(ObjectClass *klass, void *data)
k->revision = 0x02;
k->class_id = PCI_CLASS_BRIDGE_HOST;
dc->desc = "Host bridge";
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->vmsd = &vmstate_i440fx;
+ /*
+ * 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;
}
static const TypeInfo i440fx_info = {
diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
index f00793d..c80b7cb 100644
--- a/hw/pci-host/ppce500.c
+++ b/hw/pci-host/ppce500.c
@@ -387,6 +387,11 @@ static void e500_host_bridge_class_init(ObjectClass *klass, void *data)
k->device_id = PCI_DEVICE_ID_MPC8533E;
k->class_id = PCI_CLASS_PROCESSOR_POWERPC;
dc->desc = "Host bridge";
+ /*
+ * 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;
}
static const TypeInfo e500_host_bridge_info = {
diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
index ebc40c6..042dc8f 100644
--- a/hw/pci-host/prep.c
+++ b/hw/pci-host/prep.c
@@ -198,7 +198,11 @@ static void raven_class_init(ObjectClass *klass, void *data)
k->class_id = PCI_CLASS_BRIDGE_HOST;
dc->desc = "PReP Host Bridge - Motorola Raven";
dc->vmsd = &vmstate_raven;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
+ /*
+ * 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;
}
static const TypeInfo raven_info = {
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index ad703a4..4dd75c6 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -390,6 +390,11 @@ static void mch_class_init(ObjectClass *klass, void *data)
k->device_id = PCI_DEVICE_ID_INTEL_Q35_MCH;
k->revision = MCH_HOST_BRIDGE_REVISION_DEFAULT;
k->class_id = PCI_CLASS_BRIDGE_HOST;
+ /*
+ * 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;
}
static const TypeInfo mch_info = {
diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c
index 91530cd..2c20d06 100644
--- a/hw/pci-host/uninorth.c
+++ b/hw/pci-host/uninorth.c
@@ -357,6 +357,11 @@ static void unin_main_pci_host_class_init(ObjectClass *klass, void *data)
k->device_id = PCI_DEVICE_ID_APPLE_UNI_N_PCI;
k->revision = 0x00;
k->class_id = PCI_CLASS_BRIDGE_HOST;
+ /*
+ * PCI-facing part of the host bridge, not usable without the
+ * host-facing part, which can't be device_add'ed, yet.
+ */
+ k->parent_class.cannot_instantiate_with_device_add_yet = true;
}
static const TypeInfo unin_main_pci_host_info = {
@@ -375,6 +380,11 @@ static void u3_agp_pci_host_class_init(ObjectClass *klass, void *data)
k->device_id = PCI_DEVICE_ID_APPLE_U3_AGP;
k->revision = 0x00;
k->class_id = PCI_CLASS_BRIDGE_HOST;
+ /*
+ * PCI-facing part of the host bridge, not usable without the
+ * host-facing part, which can't be device_add'ed, yet.
+ */
+ k->parent_class.cannot_instantiate_with_device_add_yet = true;
}
static const TypeInfo u3_agp_pci_host_info = {
@@ -393,6 +403,11 @@ static void unin_agp_pci_host_class_init(ObjectClass *klass, void *data)
k->device_id = PCI_DEVICE_ID_APPLE_UNI_N_AGP;
k->revision = 0x00;
k->class_id = PCI_CLASS_BRIDGE_HOST;
+ /*
+ * PCI-facing part of the host bridge, not usable without the
+ * host-facing part, which can't be device_add'ed, yet.
+ */
+ k->parent_class.cannot_instantiate_with_device_add_yet = true;
}
static const TypeInfo unin_agp_pci_host_info = {
@@ -411,6 +426,11 @@ static void unin_internal_pci_host_class_init(ObjectClass *klass, void *data)
k->device_id = PCI_DEVICE_ID_APPLE_UNI_N_I_PCI;
k->revision = 0x00;
k->class_id = PCI_CLASS_BRIDGE_HOST;
+ /*
+ * PCI-facing part of the host bridge, not usable without the
+ * host-facing part, which can't be device_add'ed, yet.
+ */
+ k->parent_class.cannot_instantiate_with_device_add_yet = true;
}
static const TypeInfo unin_internal_pci_host_info = {
diff --git a/hw/pci-host/versatile.c b/hw/pci-host/versatile.c
index 6b28929..de22417 100644
--- a/hw/pci-host/versatile.c
+++ b/hw/pci-host/versatile.c
@@ -472,6 +472,11 @@ static void versatile_pci_host_class_init(ObjectClass *klass, void *data)
k->vendor_id = PCI_VENDOR_ID_XILINX;
k->device_id = PCI_DEVICE_ID_XILINX_XC2VP30;
k->class_id = PCI_CLASS_PROCESSOR_CO;
+ /*
+ * PCI-facing part of the host bridge, not usable without the
+ * host-facing part, which can't be device_add'ed, yet.
+ */
+ k->parent_class.cannot_instantiate_with_device_add_yet = true;
}
static const TypeInfo versatile_pci_host_info = {
diff --git a/hw/ppc/ppc4xx_pci.c b/hw/ppc/ppc4xx_pci.c
index d2d6f65..4cb7851 100644
--- a/hw/ppc/ppc4xx_pci.c
+++ b/hw/ppc/ppc4xx_pci.c
@@ -380,6 +380,11 @@ static void ppc4xx_host_bridge_class_init(ObjectClass *klass, void *data)
k->vendor_id = PCI_VENDOR_ID_IBM;
k->device_id = PCI_DEVICE_ID_IBM_440GX;
k->class_id = PCI_CLASS_BRIDGE_OTHER;
+ /*
+ * 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;
}
static const TypeInfo ppc4xx_host_bridge_info = {
diff --git a/hw/sh4/sh_pci.c b/hw/sh4/sh_pci.c
index e81176a..72fbe61 100644
--- a/hw/sh4/sh_pci.c
+++ b/hw/sh4/sh_pci.c
@@ -166,6 +166,11 @@ static void sh_pci_host_class_init(ObjectClass *klass, void *data)
k->init = sh_pci_host_init;
k->vendor_id = PCI_VENDOR_ID_HITACHI;
k->device_id = PCI_DEVICE_ID_HITACHI_SH7751R;
+ /*
+ * PCI-facing part of the host bridge, not usable without the
+ * host-facing part, which can't be device_add'ed, yet.
+ */
+ k->parent_class.cannot_instantiate_with_device_add_yet = true;
}
static const TypeInfo sh_pci_host_info = {
--
1.8.1.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH RFC 5/9] ich9: Document why cannot_instantiate_with_device_add_yet
2013-10-10 14:42 [Qemu-devel] [PATCH RFC 0/9] Clean up and fix no_user armbru
` (3 preceding siblings ...)
2013-10-10 14:42 ` [Qemu-devel] [PATCH RFC 4/9] pci-host: Consistently set cannot_instantiate_with_device_add_yet armbru
@ 2013-10-10 14:42 ` armbru
2013-10-10 16:01 ` Paolo Bonzini
2013-10-10 14:42 ` [Qemu-devel] [PATCH RFC 6/9] piix3 piix4: " armbru
` (5 subsequent siblings)
10 siblings, 1 reply; 38+ messages in thread
From: armbru @ 2013-10-10 14:42 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, afaerber, anthony, peter.maydell
From: Markus Armbruster <armbru@redhat.com>
An ICH9 southbridge contains several PCI devices, some of them with
multiple functions. We model each function as a separate qdev. Two
of them need some special wiring set up in pc_q35_init() to work: the
LPC controller at 00:1f.0, and the SMBus controller at 00:1f.3.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/i2c/smbus_ich9.c | 6 +++++-
hw/isa/lpc_ich9.c | 7 +++++--
2 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
index c1ffa34..8d47eaf 100644
--- a/hw/i2c/smbus_ich9.c
+++ b/hw/i2c/smbus_ich9.c
@@ -97,11 +97,15 @@ static void ich9_smb_class_init(ObjectClass *klass, void *data)
k->device_id = PCI_DEVICE_ID_INTEL_ICH9_6;
k->revision = ICH9_A2_SMB_REVISION;
k->class_id = PCI_CLASS_SERIAL_SMBUS;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->vmsd = &vmstate_ich9_smbus;
dc->desc = "ICH9 SMBUS Bridge";
k->init = ich9_smbus_initfn;
k->config_write = ich9_smbus_write_config;
+ /*
+ * Reason: part of ICH9 southbridge, needs to be wired up by
+ * pc_q35_init()
+ */
+ dc->cannot_instantiate_with_device_add_yet = true;
}
i2c_bus *ich9_smb_init(PCIBus *bus, int devfn, uint32_t smb_io_base)
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index ad841b5..d00d698 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -604,14 +604,17 @@ static void ich9_lpc_class_init(ObjectClass *klass, void *data)
dc->reset = ich9_lpc_reset;
k->init = ich9_lpc_initfn;
dc->vmsd = &vmstate_ich9_lpc;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
k->config_write = ich9_lpc_config_write;
dc->desc = "ICH9 LPC bridge";
k->vendor_id = PCI_VENDOR_ID_INTEL;
k->device_id = PCI_DEVICE_ID_INTEL_ICH9_8;
k->revision = ICH9_A2_LPC_REVISION;
k->class_id = PCI_CLASS_BRIDGE_ISA;
-
+ /*
+ * Reason: part of ICH9 southbridge, needs to be wired up by
+ * pc_q35_init()
+ */
+ dc->cannot_instantiate_with_device_add_yet = true;
}
static const TypeInfo ich9_lpc_info = {
--
1.8.1.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH RFC 5/9] ich9: Document why cannot_instantiate_with_device_add_yet
2013-10-10 14:42 ` [Qemu-devel] [PATCH RFC 5/9] ich9: Document why cannot_instantiate_with_device_add_yet armbru
@ 2013-10-10 16:01 ` Paolo Bonzini
2013-10-10 16:03 ` Paolo Bonzini
0 siblings, 1 reply; 38+ messages in thread
From: Paolo Bonzini @ 2013-10-10 16:01 UTC (permalink / raw)
To: armbru; +Cc: peter.maydell, borntraeger, qemu-devel, anthony, afaerber
Il 10/10/2013 16:42, armbru@redhat.com ha scritto:
> From: Markus Armbruster <armbru@redhat.com>
>
> An ICH9 southbridge contains several PCI devices, some of them with
> multiple functions. We model each function as a separate qdev. Two
> of them need some special wiring set up in pc_q35_init() to work: the
> LPC controller at 00:1f.0, and the SMBus controller at 00:1f.3.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> hw/i2c/smbus_ich9.c | 6 +++++-
> hw/isa/lpc_ich9.c | 7 +++++--
> 2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
> index c1ffa34..8d47eaf 100644
> --- a/hw/i2c/smbus_ich9.c
> +++ b/hw/i2c/smbus_ich9.c
> @@ -97,11 +97,15 @@ static void ich9_smb_class_init(ObjectClass *klass, void *data)
> k->device_id = PCI_DEVICE_ID_INTEL_ICH9_6;
> k->revision = ICH9_A2_SMB_REVISION;
> k->class_id = PCI_CLASS_SERIAL_SMBUS;
> - dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
> dc->vmsd = &vmstate_ich9_smbus;
> dc->desc = "ICH9 SMBUS Bridge";
> k->init = ich9_smbus_initfn;
> k->config_write = ich9_smbus_write_config;
> + /*
> + * Reason: part of ICH9 southbridge, needs to be wired up by
> + * pc_q35_init()
> + */
> + dc->cannot_instantiate_with_device_add_yet = true;
> }
>
> i2c_bus *ich9_smb_init(PCIBus *bus, int devfn, uint32_t smb_io_base)
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index ad841b5..d00d698 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -604,14 +604,17 @@ static void ich9_lpc_class_init(ObjectClass *klass, void *data)
> dc->reset = ich9_lpc_reset;
> k->init = ich9_lpc_initfn;
> dc->vmsd = &vmstate_ich9_lpc;
> - dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
> k->config_write = ich9_lpc_config_write;
> dc->desc = "ICH9 LPC bridge";
> k->vendor_id = PCI_VENDOR_ID_INTEL;
> k->device_id = PCI_DEVICE_ID_INTEL_ICH9_8;
> k->revision = ICH9_A2_LPC_REVISION;
> k->class_id = PCI_CLASS_BRIDGE_ISA;
> -
> + /*
> + * Reason: part of ICH9 southbridge, needs to be wired up by
> + * pc_q35_init()
> + */
> + dc->cannot_instantiate_with_device_add_yet = true;
> }
>
> static const TypeInfo ich9_lpc_info = {
>
I think SMBus is fine. Even though the standard configuration of the
Q35 SMBus cannot be reproduced with -device, that's a problem of
hw/i2c/smbus_eeprom.c, not of the smbus bridge itself. I think
hw/i2c/smbus_eeprom.c should rather be marked as
cannot_instantiate_with_device_add_yet (which makes sense, because it
has a PROP_PTR).
Alternatively, you can resuscitate the first two patches of
http://lists.gnu.org/archive/html/qemu-devel/2012-02/msg00449.html
Paolo
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH RFC 5/9] ich9: Document why cannot_instantiate_with_device_add_yet
2013-10-10 16:01 ` Paolo Bonzini
@ 2013-10-10 16:03 ` Paolo Bonzini
2013-10-11 6:13 ` Markus Armbruster
0 siblings, 1 reply; 38+ messages in thread
From: Paolo Bonzini @ 2013-10-10 16:03 UTC (permalink / raw)
Cc: peter.maydell, qemu-devel, armbru, borntraeger, anthony, afaerber
Il 10/10/2013 18:01, Paolo Bonzini ha scritto:
> Il 10/10/2013 16:42, armbru@redhat.com ha scritto:
>> From: Markus Armbruster <armbru@redhat.com>
>>
>> An ICH9 southbridge contains several PCI devices, some of them with
>> multiple functions. We model each function as a separate qdev. Two
>> of them need some special wiring set up in pc_q35_init() to work: the
>> LPC controller at 00:1f.0, and the SMBus controller at 00:1f.3.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> hw/i2c/smbus_ich9.c | 6 +++++-
>> hw/isa/lpc_ich9.c | 7 +++++--
>> 2 files changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
>> index c1ffa34..8d47eaf 100644
>> --- a/hw/i2c/smbus_ich9.c
>> +++ b/hw/i2c/smbus_ich9.c
>> @@ -97,11 +97,15 @@ static void ich9_smb_class_init(ObjectClass *klass, void *data)
>> k->device_id = PCI_DEVICE_ID_INTEL_ICH9_6;
>> k->revision = ICH9_A2_SMB_REVISION;
>> k->class_id = PCI_CLASS_SERIAL_SMBUS;
>> - dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
>> dc->vmsd = &vmstate_ich9_smbus;
>> dc->desc = "ICH9 SMBUS Bridge";
>> k->init = ich9_smbus_initfn;
>> k->config_write = ich9_smbus_write_config;
>> + /*
>> + * Reason: part of ICH9 southbridge, needs to be wired up by
>> + * pc_q35_init()
>> + */
>> + dc->cannot_instantiate_with_device_add_yet = true;
>> }
>>
>> i2c_bus *ich9_smb_init(PCIBus *bus, int devfn, uint32_t smb_io_base)
>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>> index ad841b5..d00d698 100644
>> --- a/hw/isa/lpc_ich9.c
>> +++ b/hw/isa/lpc_ich9.c
>> @@ -604,14 +604,17 @@ static void ich9_lpc_class_init(ObjectClass *klass, void *data)
>> dc->reset = ich9_lpc_reset;
>> k->init = ich9_lpc_initfn;
>> dc->vmsd = &vmstate_ich9_lpc;
>> - dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
>> k->config_write = ich9_lpc_config_write;
>> dc->desc = "ICH9 LPC bridge";
>> k->vendor_id = PCI_VENDOR_ID_INTEL;
>> k->device_id = PCI_DEVICE_ID_INTEL_ICH9_8;
>> k->revision = ICH9_A2_LPC_REVISION;
>> k->class_id = PCI_CLASS_BRIDGE_ISA;
>> -
>> + /*
>> + * Reason: part of ICH9 southbridge, needs to be wired up by
>> + * pc_q35_init()
>> + */
>> + dc->cannot_instantiate_with_device_add_yet = true;
>> }
>>
>> static const TypeInfo ich9_lpc_info = {
>>
>
> I think SMBus is fine. Even though the standard configuration of the
> Q35 SMBus cannot be reproduced with -device, that's a problem of
> hw/i2c/smbus_eeprom.c, not of the smbus bridge itself. I think
> hw/i2c/smbus_eeprom.c should rather be marked as
> cannot_instantiate_with_device_add_yet (which makes sense, because it
> has a PROP_PTR).
>
> Alternatively, you can resuscitate the first two patches of
> http://lists.gnu.org/archive/html/qemu-devel/2012-02/msg00449.html
Ah, got it---the SMBus and LPC need to be connected to each other.
Still, the SMBus EEPROM is a candidate for
cannot_instantiate_with_device_add_yet.
Paolo
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH RFC 5/9] ich9: Document why cannot_instantiate_with_device_add_yet
2013-10-10 16:03 ` Paolo Bonzini
@ 2013-10-11 6:13 ` Markus Armbruster
0 siblings, 0 replies; 38+ messages in thread
From: Markus Armbruster @ 2013-10-11 6:13 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: peter.maydell, afaerber, qemu-devel, anthony, borntraeger
Paolo Bonzini <pbonzini@redhat.com> writes:
> Il 10/10/2013 18:01, Paolo Bonzini ha scritto:
>> Il 10/10/2013 16:42, armbru@redhat.com ha scritto:
>>> From: Markus Armbruster <armbru@redhat.com>
>>>
>>> An ICH9 southbridge contains several PCI devices, some of them with
>>> multiple functions. We model each function as a separate qdev. Two
>>> of them need some special wiring set up in pc_q35_init() to work: the
>>> LPC controller at 00:1f.0, and the SMBus controller at 00:1f.3.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>> hw/i2c/smbus_ich9.c | 6 +++++-
>>> hw/isa/lpc_ich9.c | 7 +++++--
>>> 2 files changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
>>> index c1ffa34..8d47eaf 100644
>>> --- a/hw/i2c/smbus_ich9.c
>>> +++ b/hw/i2c/smbus_ich9.c
>>> @@ -97,11 +97,15 @@ static void ich9_smb_class_init(ObjectClass *klass, void *data)
>>> k->device_id = PCI_DEVICE_ID_INTEL_ICH9_6;
>>> k->revision = ICH9_A2_SMB_REVISION;
>>> k->class_id = PCI_CLASS_SERIAL_SMBUS;
>>> - dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
>>> dc->vmsd = &vmstate_ich9_smbus;
>>> dc->desc = "ICH9 SMBUS Bridge";
>>> k->init = ich9_smbus_initfn;
>>> k->config_write = ich9_smbus_write_config;
>>> + /*
>>> + * Reason: part of ICH9 southbridge, needs to be wired up by
>>> + * pc_q35_init()
>>> + */
>>> + dc->cannot_instantiate_with_device_add_yet = true;
>>> }
>>>
>>> i2c_bus *ich9_smb_init(PCIBus *bus, int devfn, uint32_t smb_io_base)
>>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>>> index ad841b5..d00d698 100644
>>> --- a/hw/isa/lpc_ich9.c
>>> +++ b/hw/isa/lpc_ich9.c
>>> @@ -604,14 +604,17 @@ static void ich9_lpc_class_init(ObjectClass *klass, void *data)
>>> dc->reset = ich9_lpc_reset;
>>> k->init = ich9_lpc_initfn;
>>> dc->vmsd = &vmstate_ich9_lpc;
>>> - dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
>>> k->config_write = ich9_lpc_config_write;
>>> dc->desc = "ICH9 LPC bridge";
>>> k->vendor_id = PCI_VENDOR_ID_INTEL;
>>> k->device_id = PCI_DEVICE_ID_INTEL_ICH9_8;
>>> k->revision = ICH9_A2_LPC_REVISION;
>>> k->class_id = PCI_CLASS_BRIDGE_ISA;
>>> -
>>> + /*
>>> + * Reason: part of ICH9 southbridge, needs to be wired up by
>>> + * pc_q35_init()
>>> + */
>>> + dc->cannot_instantiate_with_device_add_yet = true;
>>> }
>>>
>>> static const TypeInfo ich9_lpc_info = {
>>>
>>
>> I think SMBus is fine. Even though the standard configuration of the
>> Q35 SMBus cannot be reproduced with -device, that's a problem of
>> hw/i2c/smbus_eeprom.c, not of the smbus bridge itself. I think
>> hw/i2c/smbus_eeprom.c should rather be marked as
>> cannot_instantiate_with_device_add_yet (which makes sense, because it
>> has a PROP_PTR).
>>
>> Alternatively, you can resuscitate the first two patches of
>> http://lists.gnu.org/archive/html/qemu-devel/2012-02/msg00449.html
Getting rid of pointer properties is useful work, but outside the scope
of this series.
> Ah, got it---the SMBus and LPC need to be connected to each other.
Yup, that's the reason.
> Still, the SMBus EEPROM is a candidate for
> cannot_instantiate_with_device_add_yet.
I'll add it in the next version, along with all other devices with
pointer properties. Thanks!
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH RFC 6/9] piix3 piix4: Document why cannot_instantiate_with_device_add_yet
2013-10-10 14:42 [Qemu-devel] [PATCH RFC 0/9] Clean up and fix no_user armbru
` (4 preceding siblings ...)
2013-10-10 14:42 ` [Qemu-devel] [PATCH RFC 5/9] ich9: Document why cannot_instantiate_with_device_add_yet armbru
@ 2013-10-10 14:42 ` armbru
2013-10-10 14:42 ` [Qemu-devel] [PATCH RFC 7/9] vt82c686: " armbru
` (4 subsequent siblings)
10 siblings, 0 replies; 38+ messages in thread
From: armbru @ 2013-10-10 14:42 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, afaerber, anthony, peter.maydell
From: Markus Armbruster <armbru@redhat.com>
A PIIX3/PIIX4 southbridge has multiple functions. We model each
function as a separate qdev. Two of them need some special wiring set
up in pc_init1() or mips_malta_init() to work: the ISA bridge at 01.0,
and the SMBus controller at 01.3.
Additionally, the IDE controller at 01.1 has always had
cannot_instantiate_with_device_add_yet set, but it's not obvious to me
why, so keep /* FIXME explain why */ for them.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/acpi/piix4.c | 7 ++++++-
hw/isa/piix4.c | 6 +++++-
hw/pci-host/piix.c | 12 ++++++++++--
3 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index c29a703..c0ad010 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -508,9 +508,14 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
k->revision = 0x03;
k->class_id = PCI_CLASS_BRIDGE_OTHER;
dc->desc = "PM";
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
+ dc->cannot_instantiate_with_device_add_yet = true;
dc->vmsd = &vmstate_acpi;
dc->props = piix4_pm_properties;
+ /*
+ * Reason: part of PIIX4 southbridge, needs to be wired up,
+ * e.g. by mips_malta_init()
+ */
+ dc->cannot_instantiate_with_device_add_yet = true;
}
static const TypeInfo piix4_pm_info = {
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index d9dac61..def6fe3 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -113,8 +113,12 @@ static void piix4_class_init(ObjectClass *klass, void *data)
k->device_id = PCI_DEVICE_ID_INTEL_82371AB_0;
k->class_id = PCI_CLASS_BRIDGE_ISA;
dc->desc = "ISA bridge";
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->vmsd = &vmstate_piix4;
+ /*
+ * Reason: part of PIIX4 southbridge, needs to be wired up,
+ * e.g. by mips_malta_init()
+ */
+ dc->cannot_instantiate_with_device_add_yet = true;
}
static const TypeInfo piix4_info = {
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 8089fd6..1526fd4 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -644,7 +644,6 @@ static void piix3_class_init(ObjectClass *klass, void *data)
dc->desc = "ISA bridge";
dc->vmsd = &vmstate_piix3;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
k->no_hotplug = 1;
k->init = piix3_initfn;
k->config_write = piix3_write_config;
@@ -652,6 +651,11 @@ static void piix3_class_init(ObjectClass *klass, void *data)
/* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */
k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0;
k->class_id = PCI_CLASS_BRIDGE_ISA;
+ /*
+ * 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;
}
static const TypeInfo piix3_info = {
@@ -668,7 +672,6 @@ static void piix3_xen_class_init(ObjectClass *klass, void *data)
dc->desc = "ISA bridge";
dc->vmsd = &vmstate_piix3;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
k->no_hotplug = 1;
k->init = piix3_initfn;
k->config_write = piix3_write_config_xen;
@@ -676,6 +679,11 @@ static void piix3_xen_class_init(ObjectClass *klass, void *data)
/* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */
k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0;
k->class_id = PCI_CLASS_BRIDGE_ISA;
+ /*
+ * 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;
};
static const TypeInfo piix3_xen_info = {
--
1.8.1.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH RFC 7/9] vt82c686: Document why cannot_instantiate_with_device_add_yet
2013-10-10 14:42 [Qemu-devel] [PATCH RFC 0/9] Clean up and fix no_user armbru
` (5 preceding siblings ...)
2013-10-10 14:42 ` [Qemu-devel] [PATCH RFC 6/9] piix3 piix4: " armbru
@ 2013-10-10 14:42 ` armbru
2013-10-10 14:42 ` [Qemu-devel] [PATCH RFC 8/9] isa: Clean up use of cannot_instantiate_with_device_add_yet armbru
` (3 subsequent siblings)
10 siblings, 0 replies; 38+ messages in thread
From: armbru @ 2013-10-10 14:42 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, afaerber, anthony, peter.maydell
From: Markus Armbruster <armbru@redhat.com>
A VT82C686B southbridge has multiple functions. We model each
function as a separate qdev. One of them need some special wiring set
up in mips_fulong2e_init() to work: the ISA bridge at 05.0.
Additionally, the IDE controller at 05.1 has always had
cannot_instantiate_with_device_add_yet set, but it's not obvious to me
why, so keep /* FIXME explain why */ for them.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/isa/vt82c686.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 3e8ec80..ec7c259 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -480,8 +480,12 @@ static void via_class_init(ObjectClass *klass, void *data)
k->class_id = PCI_CLASS_BRIDGE_ISA;
k->revision = 0x40;
dc->desc = "ISA bridge";
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->vmsd = &vmstate_via;
+ /*
+ * 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;
}
static const TypeInfo via_info = {
--
1.8.1.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH RFC 8/9] isa: Clean up use of cannot_instantiate_with_device_add_yet
2013-10-10 14:42 [Qemu-devel] [PATCH RFC 0/9] Clean up and fix no_user armbru
` (6 preceding siblings ...)
2013-10-10 14:42 ` [Qemu-devel] [PATCH RFC 7/9] vt82c686: " armbru
@ 2013-10-10 14:42 ` armbru
2013-10-15 12:43 ` [Qemu-devel] Should the i8259 devices remain no-user? (was: [PATCH RFC 8/9] isa: Clean up use of cannot_instantiate_with_device_add_yet) Markus Armbruster
2013-10-10 14:42 ` [Qemu-devel] [PATCH RFC 9/9] qdev: Do not let the user try to device_add when it cannot work armbru
` (2 subsequent siblings)
10 siblings, 1 reply; 38+ messages in thread
From: armbru @ 2013-10-10 14:42 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, afaerber, anthony, peter.maydell
From: Markus Armbruster <armbru@redhat.com>
Drop it when there's no obvious reason why device_add could not work.
Else keep and document why.
* isa-fdc, port92, i8042, m48t59_isa, mc146818rtc, isa-pit, kvm-pit:
drop (the last two by dropping it from their abstract base
pit-common)
* pcspk: keep because of pointer property pit, and because realize
sets global pcspk_state
* vmmouse: keep because of pointer property ps2_mouse
* vmport: keep because realize sets global port_state
* pic-common, isa-i8259, kvm-i8259: move from abstract base pic-common
to its subtypes, keep in isa-i8259 because init sets global isa_pic
and slave_pic, keep in kvm-i8259 with /* FIXME explain why */
Perhaps I should split this patch.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/audio/pcspk.c | 3 ++-
hw/block/fdc.c | 1 -
hw/i386/kvm/i8259.c | 1 +
hw/i386/pc.c | 1 -
hw/input/pckbd.c | 1 -
hw/input/vmmouse.c | 3 ++-
hw/intc/i8259.c | 2 ++
hw/intc/i8259_common.c | 1 -
hw/misc/vmport.c | 3 ++-
hw/timer/i8254_common.c | 1 -
hw/timer/m48t59.c | 1 -
hw/timer/mc146818rtc.c | 1 -
12 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/hw/audio/pcspk.c b/hw/audio/pcspk.c
index 8e3e178..f980d66 100644
--- a/hw/audio/pcspk.c
+++ b/hw/audio/pcspk.c
@@ -192,8 +192,9 @@ static void pcspk_class_initfn(ObjectClass *klass, void *data)
dc->realize = pcspk_realizefn;
set_bit(DEVICE_CATEGORY_SOUND, dc->categories);
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->props = pcspk_properties;
+ /* Reason: pointer property "pit", realize sets global pcspk_state */
+ dc->cannot_instantiate_with_device_add_yet = true;
}
static const TypeInfo pcspk_info = {
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 86f4920..592b58f 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2234,7 +2234,6 @@ static void isabus_fdc_class_init(ObjectClass *klass, void *data)
dc->realize = isabus_fdc_realize;
dc->fw_name = "fdc";
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->reset = fdctrl_external_reset_isa;
dc->vmsd = &vmstate_isa_fdc;
dc->props = isa_fdc_properties;
diff --git a/hw/i386/kvm/i8259.c b/hw/i386/kvm/i8259.c
index 53e3ca8..6aa343a 100644
--- a/hw/i386/kvm/i8259.c
+++ b/hw/i386/kvm/i8259.c
@@ -144,6 +144,7 @@ static void kvm_i8259_class_init(ObjectClass *klass, void *data)
dc->realize = kvm_pic_realize;
k->pre_save = kvm_pic_get;
k->post_load = kvm_pic_put;
+ dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo kvm_i8259_info = {
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index fe33843..bebda79 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -544,7 +544,6 @@ static void port92_class_initfn(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->realize = port92_realizefn;
dc->reset = port92_reset;
dc->vmsd = &vmstate_port92_isa;
diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index dee31a6..655b8c5 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -522,7 +522,6 @@ static void i8042_class_initfn(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
dc->realize = i8042_realizefn;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->vmsd = &vmstate_kbd_isa;
}
diff --git a/hw/input/vmmouse.c b/hw/input/vmmouse.c
index 600e4a2..6a50533 100644
--- a/hw/input/vmmouse.c
+++ b/hw/input/vmmouse.c
@@ -282,10 +282,11 @@ static void vmmouse_class_initfn(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
dc->realize = vmmouse_realizefn;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->reset = vmmouse_reset;
dc->vmsd = &vmstate_vmmouse;
dc->props = vmmouse_properties;
+ /* Reason: pointer property "ps2_mouse" */
+ dc->cannot_instantiate_with_device_add_yet = true;
}
static const TypeInfo vmmouse_info = {
diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
index c6f248b..3321d10 100644
--- a/hw/intc/i8259.c
+++ b/hw/intc/i8259.c
@@ -504,6 +504,8 @@ static void i8259_class_init(ObjectClass *klass, void *data)
k->parent_realize = dc->realize;
dc->realize = pic_realize;
dc->reset = pic_reset;
+ /* Reason: init sets global isa_pic, slave_pic */
+ dc->cannot_instantiate_with_device_add_yet = true;
}
static const TypeInfo i8259_info = {
diff --git a/hw/intc/i8259_common.c b/hw/intc/i8259_common.c
index 2acdbfe..0e35c14 100644
--- a/hw/intc/i8259_common.c
+++ b/hw/intc/i8259_common.c
@@ -135,7 +135,6 @@ static void pic_common_class_init(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
dc->vmsd = &vmstate_pic_common;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->props = pic_properties_common;
dc->realize = pic_common_realize;
}
diff --git a/hw/misc/vmport.c b/hw/misc/vmport.c
index 94ae6ae..cd5716a 100644
--- a/hw/misc/vmport.c
+++ b/hw/misc/vmport.c
@@ -162,7 +162,8 @@ static void vmport_class_initfn(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
dc->realize = vmport_realizefn;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
+ /* Reason: realize sets global port_state */
+ dc->cannot_instantiate_with_device_add_yet = true;
}
static const TypeInfo vmport_info = {
diff --git a/hw/timer/i8254_common.c b/hw/timer/i8254_common.c
index dc2196c..2236369 100644
--- a/hw/timer/i8254_common.c
+++ b/hw/timer/i8254_common.c
@@ -282,7 +282,6 @@ static void pit_common_class_init(ObjectClass *klass, void *data)
dc->realize = pit_common_realize;
dc->vmsd = &vmstate_pit_common;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo pit_common_type = {
diff --git a/hw/timer/m48t59.c b/hw/timer/m48t59.c
index f81cf48..c0dfb51 100644
--- a/hw/timer/m48t59.c
+++ b/hw/timer/m48t59.c
@@ -750,7 +750,6 @@ static void m48t59_isa_class_init(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
dc->realize = m48t59_isa_realize;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->reset = m48t59_reset_isa;
dc->props = m48t59_isa_properties;
}
diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 2f58220..2faef13 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -906,7 +906,6 @@ static void rtc_class_initfn(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
dc->realize = rtc_realizefn;
- dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->vmsd = &vmstate_rtc;
dc->props = mc146818rtc_properties;
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] Should the i8259 devices remain no-user? (was: [PATCH RFC 8/9] isa: Clean up use of cannot_instantiate_with_device_add_yet)
2013-10-10 14:42 ` [Qemu-devel] [PATCH RFC 8/9] isa: Clean up use of cannot_instantiate_with_device_add_yet armbru
@ 2013-10-15 12:43 ` Markus Armbruster
2013-10-15 12:54 ` [Qemu-devel] Should the i8259 devices remain no-user? Paolo Bonzini
0 siblings, 1 reply; 38+ messages in thread
From: Markus Armbruster @ 2013-10-15 12:43 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Andreas Färber, Anthony Liguori
Paolo, or maybe Andreas,
To go beyond RFC with this series, I need to explain why isa-i8259 and
kvm-i8259 cannot_instantiate_with_device_add_yet, or drop that. I'd
appreciate your help.
Both are derived from TYPE_PIC_COMMON, which is derived from
TYPE_ISA_DEVICE.
I figure isa-i8259 cannot_instantiate_with_device_add_yet, because it
sets global isa_pic and slave_pic. slave_pic appears to be a lame way
to wire the slave PIC to the master PIC behind QOM's back. isa_pic
appears to be a lame way to wire the master PIC to whatever it needs to
be wired to. Is that a fair description?
If yes, is it sufficient reason for
cannot_instantiate_with_device_add_yet?
kvm-i8259 is the same device implemented with kernel support. Does it
have its own reason for cannot_instantiate_with_device_add_yet?
If not, should it keep cannot_instantiate_with_device_add_yet for
symmetry with isa-i8259?
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] Should the i8259 devices remain no-user?
2013-10-15 12:43 ` [Qemu-devel] Should the i8259 devices remain no-user? (was: [PATCH RFC 8/9] isa: Clean up use of cannot_instantiate_with_device_add_yet) Markus Armbruster
@ 2013-10-15 12:54 ` Paolo Bonzini
2013-10-16 9:51 ` Markus Armbruster
0 siblings, 1 reply; 38+ messages in thread
From: Paolo Bonzini @ 2013-10-15 12:54 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Anthony Liguori, Andreas Färber
Il 15/10/2013 14:43, Markus Armbruster ha scritto:
> Paolo, or maybe Andreas,
>
> To go beyond RFC with this series, I need to explain why isa-i8259 and
> kvm-i8259 cannot_instantiate_with_device_add_yet, or drop that. I'd
> appreciate your help.
>
> Both are derived from TYPE_PIC_COMMON, which is derived from
> TYPE_ISA_DEVICE.
>
> I figure isa-i8259 cannot_instantiate_with_device_add_yet, because it
> sets global isa_pic and slave_pic. slave_pic appears to be a lame way
> to wire the slave PIC to the master PIC behind QOM's back. isa_pic
> appears to be a lame way to wire the master PIC to whatever it needs to
> be wired to. Is that a fair description?
Yes.
> If yes, is it sufficient reason for
> cannot_instantiate_with_device_add_yet?
>
> kvm-i8259 is the same device implemented with kernel support. Does it
> have its own reason for cannot_instantiate_with_device_add_yet?
>
> If not, should it keep cannot_instantiate_with_device_add_yet for
> symmetry with isa-i8259?
Both i8259 implementations have to be matched with an appropriate array
of qemu_irqs, such as the one returned by kvm_i8259_init. I think this
is the reason why kvm-i8259 cannot be instantiated from the command-line.
Paolo
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] Should the i8259 devices remain no-user?
2013-10-15 12:54 ` [Qemu-devel] Should the i8259 devices remain no-user? Paolo Bonzini
@ 2013-10-16 9:51 ` Markus Armbruster
2013-10-16 11:06 ` Paolo Bonzini
0 siblings, 1 reply; 38+ messages in thread
From: Markus Armbruster @ 2013-10-16 9:51 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Anthony Liguori, Andreas Färber
Paolo Bonzini <pbonzini@redhat.com> writes:
> Il 15/10/2013 14:43, Markus Armbruster ha scritto:
>> Paolo, or maybe Andreas,
>>
>> To go beyond RFC with this series, I need to explain why isa-i8259 and
>> kvm-i8259 cannot_instantiate_with_device_add_yet, or drop that. I'd
>> appreciate your help.
>>
>> Both are derived from TYPE_PIC_COMMON, which is derived from
>> TYPE_ISA_DEVICE.
>>
>> I figure isa-i8259 cannot_instantiate_with_device_add_yet, because it
>> sets global isa_pic and slave_pic. slave_pic appears to be a lame way
>> to wire the slave PIC to the master PIC behind QOM's back. isa_pic
>> appears to be a lame way to wire the master PIC to whatever it needs to
>> be wired to. Is that a fair description?
>
> Yes.
>
>> If yes, is it sufficient reason for
>> cannot_instantiate_with_device_add_yet?
>>
>> kvm-i8259 is the same device implemented with kernel support. Does it
>> have its own reason for cannot_instantiate_with_device_add_yet?
>>
>> If not, should it keep cannot_instantiate_with_device_add_yet for
>> symmetry with isa-i8259?
>
> Both i8259 implementations have to be matched with an appropriate array
> of qemu_irqs, such as the one returned by kvm_i8259_init. I think this
> is the reason why kvm-i8259 cannot be instantiated from the command-line.
Let me try to elaborate, to make sure I understand.
Unlike ordinary ISA devices, the i8259 devices need additional wiring,
done by code.
For instance, board code like pc_q35_init(), pc_piix.c's pc_init1(),
mips_malta_init(), ... wire up their IRQ input lines. The slave's IRQ
output line is wired to the master's IRQ2 in hw/intc/i8259.c for
isa-i8259, and the kernel for kvm-i8259. The master's IRQ output line
is wired up by board code (it's complicated).
Correct? If yes, I can turn it into a suitable comment.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] Should the i8259 devices remain no-user?
2013-10-16 9:51 ` Markus Armbruster
@ 2013-10-16 11:06 ` Paolo Bonzini
2013-10-16 16:12 ` Markus Armbruster
2013-10-16 16:21 ` BALATON Zoltan
0 siblings, 2 replies; 38+ messages in thread
From: Paolo Bonzini @ 2013-10-16 11:06 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Anthony Liguori, Andreas Färber
Il 16/10/2013 11:51, Markus Armbruster ha scritto:
> Let me try to elaborate, to make sure I understand.
>
> Unlike ordinary ISA devices, the i8259 devices need additional wiring,
> done by code.
>
> For instance, board code like pc_q35_init(), pc_piix.c's pc_init1(),
> mips_malta_init(), ... wire up their IRQ input lines. The slave's IRQ
> output line is wired to the master's IRQ2 in hw/intc/i8259.c for
> isa-i8259, and the kernel for kvm-i8259. The master's IRQ output line
> is wired up by board code (it's complicated).
>
> Correct? If yes, I can turn it into a suitable comment.
The wiring of the slave to the master is hardcoded into i8259 code.
The wiring of all 16 lines is set up by board code.
Paolo
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] Should the i8259 devices remain no-user?
2013-10-16 11:06 ` Paolo Bonzini
@ 2013-10-16 16:12 ` Markus Armbruster
2013-10-16 16:21 ` BALATON Zoltan
1 sibling, 0 replies; 38+ messages in thread
From: Markus Armbruster @ 2013-10-16 16:12 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Anthony Liguori, Andreas Färber
Paolo Bonzini <pbonzini@redhat.com> writes:
> Il 16/10/2013 11:51, Markus Armbruster ha scritto:
>> Let me try to elaborate, to make sure I understand.
>>
>> Unlike ordinary ISA devices, the i8259 devices need additional wiring,
>> done by code.
>>
>> For instance, board code like pc_q35_init(), pc_piix.c's pc_init1(),
>> mips_malta_init(), ... wire up their IRQ input lines. The slave's IRQ
>> output line is wired to the master's IRQ2 in hw/intc/i8259.c for
>> isa-i8259, and the kernel for kvm-i8259. The master's IRQ output line
>> is wired up by board code (it's complicated).
>>
>> Correct? If yes, I can turn it into a suitable comment.
>
> The wiring of the slave to the master is hardcoded into i8259 code.
>
> The wiring of all 16 lines is set up by board code.
Got it, thanks!
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] Should the i8259 devices remain no-user?
2013-10-16 11:06 ` Paolo Bonzini
2013-10-16 16:12 ` Markus Armbruster
@ 2013-10-16 16:21 ` BALATON Zoltan
2013-10-16 16:23 ` Paolo Bonzini
1 sibling, 1 reply; 38+ messages in thread
From: BALATON Zoltan @ 2013-10-16 16:21 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Andreas Färber, Markus Armbruster, Anthony Liguori,
qemu-devel
On Wed, 16 Oct 2013, Paolo Bonzini wrote:
> Il 16/10/2013 11:51, Markus Armbruster ha scritto:
>> Let me try to elaborate, to make sure I understand.
>>
>> Unlike ordinary ISA devices, the i8259 devices need additional wiring,
>> done by code.
>>
>> For instance, board code like pc_q35_init(), pc_piix.c's pc_init1(),
>> mips_malta_init(), ... wire up their IRQ input lines. The slave's IRQ
>> output line is wired to the master's IRQ2 in hw/intc/i8259.c for
>> isa-i8259, and the kernel for kvm-i8259. The master's IRQ output line
>> is wired up by board code (it's complicated).
>>
>> Correct? If yes, I can turn it into a suitable comment.
>
> The wiring of the slave to the master is hardcoded into i8259 code.
A bit off topic but this reminded me of these patches:
http://patchwork.ozlabs.org/patch/206753/
http://patchwork.ozlabs.org/patch/208252/
which never got merged. Is there a chance that these fixes get merged
sometimes or is there an explanation why it won't be fixed? As far as I
remember the patches were reviewed and multiple versions were proposed but
at the end no decision was reached on which one to merge and it was just
left uncorrected.
Thanks,
BALATON Zoltan
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] Should the i8259 devices remain no-user?
2013-10-16 16:21 ` BALATON Zoltan
@ 2013-10-16 16:23 ` Paolo Bonzini
2013-10-26 20:00 ` [Qemu-devel] fix clearing i8259 IRQ lines (Was: Should the i8259 devices remain no-user?) Matthew Ogilvie
0 siblings, 1 reply; 38+ messages in thread
From: Paolo Bonzini @ 2013-10-16 16:23 UTC (permalink / raw)
To: BALATON Zoltan
Cc: Andreas Färber, Markus Armbruster, Anthony Liguori,
qemu-devel
Il 16/10/2013 18:21, BALATON Zoltan ha scritto:
> On Wed, 16 Oct 2013, Paolo Bonzini wrote:
>> Il 16/10/2013 11:51, Markus Armbruster ha scritto:
>>> Let me try to elaborate, to make sure I understand.
>>>
>>> Unlike ordinary ISA devices, the i8259 devices need additional wiring,
>>> done by code.
>>>
>>> For instance, board code like pc_q35_init(), pc_piix.c's pc_init1(),
>>> mips_malta_init(), ... wire up their IRQ input lines. The slave's IRQ
>>> output line is wired to the master's IRQ2 in hw/intc/i8259.c for
>>> isa-i8259, and the kernel for kvm-i8259. The master's IRQ output line
>>> is wired up by board code (it's complicated).
>>>
>>> Correct? If yes, I can turn it into a suitable comment.
>>
>> The wiring of the slave to the master is hardcoded into i8259 code.
>
> A bit off topic but this reminded me of these patches:
>
> http://patchwork.ozlabs.org/patch/206753/
> http://patchwork.ozlabs.org/patch/208252/
>
> which never got merged. Is there a chance that these fixes get merged
> sometimes or is there an explanation why it won't be fixed? As far as I
> remember the patches were reviewed and multiple versions were proposed
> but at the end no decision was reached on which one to merge and it was
> just left uncorrected.
Right, thank you very much. ISTR the unanswered question was what to do
about migration, but I need to reread all the threads.
Paolo
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] fix clearing i8259 IRQ lines (Was: Should the i8259 devices remain no-user?)
2013-10-16 16:23 ` Paolo Bonzini
@ 2013-10-26 20:00 ` Matthew Ogilvie
2013-10-29 16:27 ` BALATON Zoltan
0 siblings, 1 reply; 38+ messages in thread
From: Matthew Ogilvie @ 2013-10-26 20:00 UTC (permalink / raw)
To: Paolo Bonzini
Cc: qemu-devel, Markus Armbruster, Andreas Färber,
Anthony Liguori
On Wed, Oct 16, 2013 at 06:23:11PM +0200, Paolo Bonzini wrote:
> Il 16/10/2013 18:21, BALATON Zoltan ha scritto:
> > A bit off topic but this reminded me of these patches:
> >
> > http://patchwork.ozlabs.org/patch/206753/
> > http://patchwork.ozlabs.org/patch/208252/
> >
> > which never got merged. Is there a chance that these fixes get merged
> > sometimes or is there an explanation why it won't be fixed? As far as I
> > remember the patches were reviewed and multiple versions were proposed
> > but at the end no decision was reached on which one to merge and it was
> > just left uncorrected.
>
> Right, thank you very much. ISTR the unanswered question was what to do
> about migration, but I need to reread all the threads.
>
> Paolo
Essentially correct.
Although the 8259 (interrupts) model is clearly wrong with respect
to clearing an IRQ request line, only one ancient unimportant guest
(Microport UNIX ca. 1987) seems to care, and there are potentially
significant risks to more important guests if we try to fix it:
Risks: The 8254 (timers) model is wrong in various ways, some of
which are hidden by the incorrect 8259 model, and fixing it could
potentially break migration, depending on exact circumstances.
Also, it isn't clear if there are other device models depending
on the incorrect 8259 that would also need to be fixed.
Similar changes are needed in KVM for consistency, although some of
the 8254 modes are implemented in a more simplistic way (pulses
handled "as fast as possible" directly, instead of
1-millisecond-long pulses on real hardware). Note that I was
never able to get my guest running successfully under KVM; I'm
not sure what the remaining problems were.
Also, the patch series included a few other things:
- A couple of low priority fixes that can still be worked
around without code changes, but could probably qualify
as "trivial patches".
- Some test cases to test for the 8259 problem.
- Plus an optional VGA hack to make it work when
my ancient guest tries to directly (no BIOS) configure it
for CGA text mode.
I didn't get much feedback about these.
-----
If someone actually showed real interest in actually merging
these, including the selection of a migration compatibility
strategy they would actually be willing to merge (and above:
other devices, KVM, etc), I could look into updating
the patches to match. But the "if" parts aren't looking
particularly likely. This seems like a rather core-level
wide-implication change for a newbie to be messing
with. (I've already spent noticably more time on qemu
patches than I had intended to spend total on playing with
this guest, although I may continue if I have a clearly
defined strategy.)
- Matthew Ogilvie
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] fix clearing i8259 IRQ lines (Was: Should the i8259 devices remain no-user?)
2013-10-26 20:00 ` [Qemu-devel] fix clearing i8259 IRQ lines (Was: Should the i8259 devices remain no-user?) Matthew Ogilvie
@ 2013-10-29 16:27 ` BALATON Zoltan
0 siblings, 0 replies; 38+ messages in thread
From: BALATON Zoltan @ 2013-10-29 16:27 UTC (permalink / raw)
To: Matthew Ogilvie
Cc: qemu-devel, Paolo Bonzini, Andreas Färber, Anthony Liguori,
Markus Armbruster
On Sat, 26 Oct 2013, Matthew Ogilvie wrote:
> Although the 8259 (interrupts) model is clearly wrong with respect
> to clearing an IRQ request line, only one ancient unimportant guest
> (Microport UNIX ca. 1987) seems to care, and there are potentially
> significant risks to more important guests if we try to fix it:
There's at least one more guest that cares I know about which is less
ancient but maybe just as unimportant: OPENSTEP for Mach. But nevertheless
it still is a now known bug which just seems to be tolerated by the OS-es
that are most commonly run under Qemu. What was not clear to me is how
significant are the risks of the fix and if they were considered or the
patch was just forgotten without ever getting the thought about merging
it.
> Risks: The 8254 (timers) model is wrong in various ways, some of
> which are hidden by the incorrect 8259 model, and fixing it could
> potentially break migration, depending on exact circumstances.
> Also, it isn't clear if there are other device models depending
> on the incorrect 8259 that would also need to be fixed.
I had the impression from previous discussion that the main risk was a
potential lost timer interrupt in some circumstances at migration which
may affect some guests but it was not clear (to me at least) how big of a
risk is this. IMO if other models depend on a bug they are also buggy and
should be fixed but I don't know how many models could that affect.
> If someone actually showed real interest in actually merging
> these, including the selection of a migration compatibility
> strategy they would actually be willing to merge (and above:
> other devices, KVM, etc), I could look into updating
> the patches to match. But the "if" parts aren't looking
> particularly likely. This seems like a rather core-level
> wide-implication change for a newbie to be messing
> with. (I've already spent noticably more time on qemu
> patches than I had intended to spend total on playing with
> this guest, although I may continue if I have a clearly
> defined strategy.)
I think you have already provided detailed analysis, test cases and
multiple options and patch versions so it is not you who should spend more
time on this now. What I think would be needed is that people who have the
knowledge and insight to analyse and decide about the patches give it some
time to think about it and come to a decision then tell what to do or why
it's better to leave it unfixed. Can this be done in this thread? Or maybe
on one of the upcoming phone conferences where the right people are
together anyway to discuss it?
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH RFC 9/9] qdev: Do not let the user try to device_add when it cannot work
2013-10-10 14:42 [Qemu-devel] [PATCH RFC 0/9] Clean up and fix no_user armbru
` (7 preceding siblings ...)
2013-10-10 14:42 ` [Qemu-devel] [PATCH RFC 8/9] isa: Clean up use of cannot_instantiate_with_device_add_yet armbru
@ 2013-10-10 14:42 ` armbru
2013-10-15 12:24 ` [Qemu-devel] Why is TYPE_CPU no-user? (was: [PATCH RFC 0/9] Clean up and fix no_user) Markus Armbruster
2013-10-15 13:21 ` [Qemu-devel] Which functions of southbridges should be no-user? (was: [PATCH RFC 0/9] Clean up and fix no_user) Markus Armbruster
10 siblings, 0 replies; 38+ messages in thread
From: armbru @ 2013-10-10 14:42 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, afaerber, anthony, peter.maydell
From: Markus Armbruster <armbru@redhat.com>
Such devices have always been unavailable and omitted from the list of
available devices shown by device_add help. Until commit 18b6dad
silently broke the former, setting up nasty traps for unwary users,
like this one:
$ qemu-system-x86_64 -nodefaults -monitor stdio -display none
QEMU 1.6.50 monitor - type 'help' for more information
(qemu) device_add apic
Segmentation fault (core dumped)
I call that a regression. Fix it.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
qdev-monitor.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 36f6f09..c538fec 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -477,7 +477,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
}
}
- if (!obj) {
+ if (!obj || DEVICE_CLASS(obj)->cannot_instantiate_with_device_add_yet) {
qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver", "device type");
return NULL;
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] Why is TYPE_CPU no-user? (was: [PATCH RFC 0/9] Clean up and fix no_user)
2013-10-10 14:42 [Qemu-devel] [PATCH RFC 0/9] Clean up and fix no_user armbru
` (8 preceding siblings ...)
2013-10-10 14:42 ` [Qemu-devel] [PATCH RFC 9/9] qdev: Do not let the user try to device_add when it cannot work armbru
@ 2013-10-15 12:24 ` Markus Armbruster
2013-10-15 12:37 ` Peter Maydell
2013-10-15 13:08 ` Andreas Färber
2013-10-15 13:21 ` [Qemu-devel] Which functions of southbridges should be no-user? (was: [PATCH RFC 0/9] Clean up and fix no_user) Markus Armbruster
10 siblings, 2 replies; 38+ messages in thread
From: Markus Armbruster @ 2013-10-15 12:24 UTC (permalink / raw)
To: qemu-devel; +Cc: afaerber
Andreas,
To go beyond RFC with this series, I need to explain why TYPE_CPU
cannot_instantiate_with_device_add_yet. Would you be so kind and help
me out with a suitable comment?
You can find examples in PATCH 2-7/9.
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] Why is TYPE_CPU no-user? (was: [PATCH RFC 0/9] Clean up and fix no_user)
2013-10-15 12:24 ` [Qemu-devel] Why is TYPE_CPU no-user? (was: [PATCH RFC 0/9] Clean up and fix no_user) Markus Armbruster
@ 2013-10-15 12:37 ` Peter Maydell
2013-10-15 14:01 ` [Qemu-devel] Why is TYPE_CPU no-user? Markus Armbruster
2013-10-15 13:08 ` Andreas Färber
1 sibling, 1 reply; 38+ messages in thread
From: Peter Maydell @ 2013-10-15 12:37 UTC (permalink / raw)
To: Markus Armbruster; +Cc: QEMU Developers, Andreas Färber
On 15 October 2013 13:24, Markus Armbruster <armbru@redhat.com> wrote:
> To go beyond RFC with this series, I need to explain why TYPE_CPU
> cannot_instantiate_with_device_add_yet.
...isn't this just because it's an abstract type?
-- PMM
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] Why is TYPE_CPU no-user?
2013-10-15 12:37 ` Peter Maydell
@ 2013-10-15 14:01 ` Markus Armbruster
0 siblings, 0 replies; 38+ messages in thread
From: Markus Armbruster @ 2013-10-15 14:01 UTC (permalink / raw)
To: Peter Maydell; +Cc: QEMU Developers, Andreas Färber
Peter Maydell <peter.maydell@linaro.org> writes:
> On 15 October 2013 13:24, Markus Armbruster <armbru@redhat.com> wrote:
>> To go beyond RFC with this series, I need to explain why TYPE_CPU
>> cannot_instantiate_with_device_add_yet.
>
> ...isn't this just because it's an abstract type?
Abstract types have TypeInfo member abstract set. Not inherited by
subtypes, obviously.
DeviceClass member cannot_instantiate_with_device_add_yet is something
else entirely. After this series, it means what the name suggests.
Unlike abstract, it's effectively inherited.
TypeInfo's abstract is here to stay, but DeviceClass's
cannot_instantiate_with_device_add_yet should eventually go away.
TYPE_CPU has both of them set.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] Why is TYPE_CPU no-user?
2013-10-15 12:24 ` [Qemu-devel] Why is TYPE_CPU no-user? (was: [PATCH RFC 0/9] Clean up and fix no_user) Markus Armbruster
2013-10-15 12:37 ` Peter Maydell
@ 2013-10-15 13:08 ` Andreas Färber
2013-10-16 9:55 ` Markus Armbruster
1 sibling, 1 reply; 38+ messages in thread
From: Andreas Färber @ 2013-10-15 13:08 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Peter Maydell, qemu-devel
Hi Markus,
Am 15.10.2013 14:24, schrieb Markus Armbruster:
> To go beyond RFC with this series, I need to explain why TYPE_CPU
> cannot_instantiate_with_device_add_yet. Would you be so kind and help
> me out with a suitable comment?
>From what I remember this was done when I started the whole process and
most CPU subtypes did not yet use the QOM instance_init for
initialization. Most importantly x86 still is not yet self-contained,
nor is sparc. Such targets need to use cpu_init() et al. rather than
-device. (This became visible in the first s390x vCPU hotplug series.)
Most boards rely on being able to do postprocessing after they have
instantiated the CPU: wiring up IRQs, adding reset handlers, halting
non-first CPUs, ...
-device would skip that.
Another aspect is that no CPU subtype has been proven hot-pluggable with
device_add yet. For s390x we're the closest to date.
We could move the flag from the base type to the targets' base types if
you prefer. Then we can knock the bad values out one by one rather than
overriding the inherited value with an explicit positive one.
Cheers,
Andreas
>
> You can find examples in PATCH 2-7/9.
>
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] Why is TYPE_CPU no-user?
2013-10-15 13:08 ` Andreas Färber
@ 2013-10-16 9:55 ` Markus Armbruster
0 siblings, 0 replies; 38+ messages in thread
From: Markus Armbruster @ 2013-10-16 9:55 UTC (permalink / raw)
To: Andreas Färber; +Cc: Peter Maydell, qemu-devel
Andreas Färber <afaerber@suse.de> writes:
> Hi Markus,
>
> Am 15.10.2013 14:24, schrieb Markus Armbruster:
>> To go beyond RFC with this series, I need to explain why TYPE_CPU
>> cannot_instantiate_with_device_add_yet. Would you be so kind and help
>> me out with a suitable comment?
>
>>From what I remember this was done when I started the whole process and
> most CPU subtypes did not yet use the QOM instance_init for
> initialization. Most importantly x86 still is not yet self-contained,
> nor is sparc. Such targets need to use cpu_init() et al. rather than
> -device. (This became visible in the first s390x vCPU hotplug series.)
>
> Most boards rely on being able to do postprocessing after they have
> instantiated the CPU: wiring up IRQs, adding reset handlers, halting
> non-first CPUs, ...
> -device would skip that.
This is the prime reason for cannot_instantiate_with_device_add_yet.
> Another aspect is that no CPU subtype has been proven hot-pluggable with
> device_add yet. For s390x we're the closest to date.
Hot-plug support is orthogonal. We got plenty of devices that support
device-add, but only cold plug.
> We could move the flag from the base type to the targets' base types if
> you prefer. Then we can knock the bad values out one by one rather than
> overriding the inherited value with an explicit positive one.
I'd prefer to make that move when the first CPU is ready.
Thanks!
[...]
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] Which functions of southbridges should be no-user? (was: [PATCH RFC 0/9] Clean up and fix no_user)
2013-10-10 14:42 [Qemu-devel] [PATCH RFC 0/9] Clean up and fix no_user armbru
` (9 preceding siblings ...)
2013-10-15 12:24 ` [Qemu-devel] Why is TYPE_CPU no-user? (was: [PATCH RFC 0/9] Clean up and fix no_user) Markus Armbruster
@ 2013-10-15 13:21 ` Markus Armbruster
2013-10-15 13:31 ` [Qemu-devel] Which functions of southbridges should be no-user? Andreas Färber
10 siblings, 1 reply; 38+ messages in thread
From: Markus Armbruster @ 2013-10-15 13:21 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, peter.maydell, Andreas Färber, Anthony Liguori
Andreas,
To go beyond RFC with this series, I need to explain why the IDE
controller functions of southbridges piix3-ide, piix3-ide-xen, piix4-ide
and via-ide cannot_instantiate_with_device_add_yet, or drop that. I'd
appreciate your help.
Our modelling of PCI devices is weird, to put it politely. One of many
weird things is that we don't distinguish between a function and a
complete device: our "function models" are actually PCI device models,
and can be used as such, even though they only exist as functions of a
multifunction device in the real world. We permit collecting aribitrary
PCI devices into multifunction devices.
One instance of multifunction PCI devices are southbridges. For
example, the ICH9 southbridge's PCI device 00:1F consists of ISA bridge
("ICH9 LPC"), IDE controller ("ich9-ahci"), SMB controller ("ICH9 SMB"),
and Thermal Subsystem (which we don't implement). The PIIX3 southbridge
consists of ISA bridge ("PIIX3", IDE controller ("piix3-ide"), USB
controller ("piix3-usb-uhci", can be suppressed), and SMB controller
("PIIX4_PM", can be suppressed).
Some functions of southbridges still need to be wired up in code ("ICH9
LPC", "ICH9 SMB", "PIIX4_PM", "PIIX4", "PIIX3", "PIIX3-xen", see PATCH
5-6/9), thus cannot_instantiate_with_device_add_yet.
The IDE controller functions have always been
cannot_instantiate_with_device_add_yet, but it's not obvious to me why.
The other functions are available with device-add. Users device-add'ing
them would of course be odd, but if it works... I don't actually know
whether it works for all of them.
Should all southbridge functions be made unavailable with device-add for
consistency, at least for now?
Or should all functions be made available, except for the ones that
cannot possibly work with device-add?
If the latter, can you think of any specific reason why the IDE
controllers couldn't work?
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] Which functions of southbridges should be no-user?
2013-10-15 13:21 ` [Qemu-devel] Which functions of southbridges should be no-user? (was: [PATCH RFC 0/9] Clean up and fix no_user) Markus Armbruster
@ 2013-10-15 13:31 ` Andreas Färber
2013-10-15 14:41 ` Kevin Wolf
0 siblings, 1 reply; 38+ messages in thread
From: Andreas Färber @ 2013-10-15 13:31 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: Kevin Wolf, Peter Maydell, Anthony Liguori
Am 15.10.2013 15:21, schrieb Markus Armbruster:
> Andreas,
>
> To go beyond RFC with this series, I need to explain why the IDE
> controller functions of southbridges piix3-ide, piix3-ide-xen, piix4-ide
> and via-ide cannot_instantiate_with_device_add_yet, or drop that. I'd
> appreciate your help.
>
> Our modelling of PCI devices is weird, to put it politely. One of many
> weird things is that we don't distinguish between a function and a
> complete device: our "function models" are actually PCI device models,
> and can be used as such, even though they only exist as functions of a
> multifunction device in the real world. We permit collecting aribitrary
> PCI devices into multifunction devices.
>
> One instance of multifunction PCI devices are southbridges. For
> example, the ICH9 southbridge's PCI device 00:1F consists of ISA bridge
> ("ICH9 LPC"), IDE controller ("ich9-ahci"), SMB controller ("ICH9 SMB"),
> and Thermal Subsystem (which we don't implement). The PIIX3 southbridge
> consists of ISA bridge ("PIIX3", IDE controller ("piix3-ide"), USB
> controller ("piix3-usb-uhci", can be suppressed), and SMB controller
> ("PIIX4_PM", can be suppressed).
>
> Some functions of southbridges still need to be wired up in code ("ICH9
> LPC", "ICH9 SMB", "PIIX4_PM", "PIIX4", "PIIX3", "PIIX3-xen", see PATCH
> 5-6/9), thus cannot_instantiate_with_device_add_yet.
>
> The IDE controller functions have always been
> cannot_instantiate_with_device_add_yet, but it's not obvious to me why.
>
> The other functions are available with device-add. Users device-add'ing
> them would of course be odd, but if it works... I don't actually know
> whether it works for all of them.
>
> Should all southbridge functions be made unavailable with device-add for
> consistency, at least for now?
>
> Or should all functions be made available, except for the ones that
> cannot possibly work with device-add?
>
> If the latter, can you think of any specific reason why the IDE
> controllers couldn't work?
I would've thought you and Kevin know more about IDE than me. ;)
No idea why it is how it is.
Two aspects:
1) PCI devices/functions can technically be hotplugged.
2) Drivers might not expect such devices to be hot-added/removed.
I would tend for the latter proposal.
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] Which functions of southbridges should be no-user?
2013-10-15 13:31 ` [Qemu-devel] Which functions of southbridges should be no-user? Andreas Färber
@ 2013-10-15 14:41 ` Kevin Wolf
2013-10-15 14:53 ` Andreas Färber
2013-10-15 15:09 ` Anthony Liguori
0 siblings, 2 replies; 38+ messages in thread
From: Kevin Wolf @ 2013-10-15 14:41 UTC (permalink / raw)
To: Andreas Färber
Cc: Peter Maydell, Markus Armbruster, Anthony Liguori, qemu-devel
Am 15.10.2013 um 15:31 hat Andreas Färber geschrieben:
> Am 15.10.2013 15:21, schrieb Markus Armbruster:
> > Andreas,
> >
> > To go beyond RFC with this series, I need to explain why the IDE
> > controller functions of southbridges piix3-ide, piix3-ide-xen, piix4-ide
> > and via-ide cannot_instantiate_with_device_add_yet, or drop that. I'd
> > appreciate your help.
> >
> > Our modelling of PCI devices is weird, to put it politely. One of many
> > weird things is that we don't distinguish between a function and a
> > complete device: our "function models" are actually PCI device models,
> > and can be used as such, even though they only exist as functions of a
> > multifunction device in the real world. We permit collecting aribitrary
> > PCI devices into multifunction devices.
> >
> > One instance of multifunction PCI devices are southbridges. For
> > example, the ICH9 southbridge's PCI device 00:1F consists of ISA bridge
> > ("ICH9 LPC"), IDE controller ("ich9-ahci"), SMB controller ("ICH9 SMB"),
> > and Thermal Subsystem (which we don't implement). The PIIX3 southbridge
> > consists of ISA bridge ("PIIX3", IDE controller ("piix3-ide"), USB
> > controller ("piix3-usb-uhci", can be suppressed), and SMB controller
> > ("PIIX4_PM", can be suppressed).
> >
> > Some functions of southbridges still need to be wired up in code ("ICH9
> > LPC", "ICH9 SMB", "PIIX4_PM", "PIIX4", "PIIX3", "PIIX3-xen", see PATCH
> > 5-6/9), thus cannot_instantiate_with_device_add_yet.
> >
> > The IDE controller functions have always been
> > cannot_instantiate_with_device_add_yet, but it's not obvious to me why.
> >
> > The other functions are available with device-add. Users device-add'ing
> > them would of course be odd, but if it works... I don't actually know
> > whether it works for all of them.
> >
> > Should all southbridge functions be made unavailable with device-add for
> > consistency, at least for now?
> >
> > Or should all functions be made available, except for the ones that
> > cannot possibly work with device-add?
> >
> > If the latter, can you think of any specific reason why the IDE
> > controllers couldn't work?
>
> I would've thought you and Kevin know more about IDE than me. ;)
> No idea why it is how it is.
>
> Two aspects:
> 1) PCI devices/functions can technically be hotplugged.
> 2) Drivers might not expect such devices to be hot-added/removed.
>
> I would tend for the latter proposal.
I'm not sure how IDE hardware really works, but I don't think you can
handle it as a pure PCI device. On PCs, the IDE controller still provides
the good old IDE registers on the I/O ports that were already used in
ISA times, and they are not described in any BARs, for example. From a
software point of view, the PCI device is just for configuring Busmaster
DMA. Perhaps in reality it should be two devices, one for DMA on the PCI
bus and another one for the core on sysbus or ISA?
Anyway, having two IDE controllers using the same I/O ports won't work,
obviously. So if you would allow -device or device-add for them, you'd
need options to configure the ports at least.
Kevin
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] Which functions of southbridges should be no-user?
2013-10-15 14:41 ` Kevin Wolf
@ 2013-10-15 14:53 ` Andreas Färber
2013-10-16 9:58 ` Markus Armbruster
2013-10-15 15:09 ` Anthony Liguori
1 sibling, 1 reply; 38+ messages in thread
From: Andreas Färber @ 2013-10-15 14:53 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Peter Maydell, Markus Armbruster, Anthony Liguori, qemu-devel
Am 15.10.2013 16:41, schrieb Kevin Wolf:
> having two IDE controllers using the same I/O ports won't work,
> obviously. So if you would allow -device or device-add for them, you'd
> need options to configure the ports at least.
Or a new can_only_be_instantiated_once flag. :)
Or proper error handling to have it fail gracefully at least.
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] Which functions of southbridges should be no-user?
2013-10-15 14:53 ` Andreas Färber
@ 2013-10-16 9:58 ` Markus Armbruster
0 siblings, 0 replies; 38+ messages in thread
From: Markus Armbruster @ 2013-10-16 9:58 UTC (permalink / raw)
To: Andreas Färber
Cc: Kevin Wolf, Peter Maydell, qemu-devel, Anthony Liguori
Andreas Färber <afaerber@suse.de> writes:
> Am 15.10.2013 16:41, schrieb Kevin Wolf:
>> having two IDE controllers using the same I/O ports won't work,
>> obviously. So if you would allow -device or device-add for them, you'd
>> need options to configure the ports at least.
>
> Or a new can_only_be_instantiated_once flag. :)
> Or proper error handling to have it fail gracefully at least.
can_only_be_instantiated_once helps users only in special cases of the
general resource conflict problem. Proper handling of resource
conflicts helps them always.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] Which functions of southbridges should be no-user?
2013-10-15 14:41 ` Kevin Wolf
2013-10-15 14:53 ` Andreas Färber
@ 2013-10-15 15:09 ` Anthony Liguori
2013-10-16 10:00 ` Markus Armbruster
1 sibling, 1 reply; 38+ messages in thread
From: Anthony Liguori @ 2013-10-15 15:09 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-devel, Peter Maydell, Andreas Färber, Anthony Liguori,
Markus Armbruster
On Tue, Oct 15, 2013 at 7:41 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 15.10.2013 um 15:31 hat Andreas Färber geschrieben:
>> Am 15.10.2013 15:21, schrieb Markus Armbruster:
>> > Andreas,
>> >
>> > To go beyond RFC with this series, I need to explain why the IDE
>> > controller functions of southbridges piix3-ide, piix3-ide-xen, piix4-ide
>> > and via-ide cannot_instantiate_with_device_add_yet, or drop that. I'd
>> > appreciate your help.
>> >
>> > Our modelling of PCI devices is weird, to put it politely. One of many
>> > weird things is that we don't distinguish between a function and a
>> > complete device: our "function models" are actually PCI device models,
>> > and can be used as such, even though they only exist as functions of a
>> > multifunction device in the real world. We permit collecting aribitrary
>> > PCI devices into multifunction devices.
>> >
>> > One instance of multifunction PCI devices are southbridges. For
>> > example, the ICH9 southbridge's PCI device 00:1F consists of ISA bridge
>> > ("ICH9 LPC"), IDE controller ("ich9-ahci"), SMB controller ("ICH9 SMB"),
>> > and Thermal Subsystem (which we don't implement). The PIIX3 southbridge
>> > consists of ISA bridge ("PIIX3", IDE controller ("piix3-ide"), USB
>> > controller ("piix3-usb-uhci", can be suppressed), and SMB controller
>> > ("PIIX4_PM", can be suppressed).
>> >
>> > Some functions of southbridges still need to be wired up in code ("ICH9
>> > LPC", "ICH9 SMB", "PIIX4_PM", "PIIX4", "PIIX3", "PIIX3-xen", see PATCH
>> > 5-6/9), thus cannot_instantiate_with_device_add_yet.
>> >
>> > The IDE controller functions have always been
>> > cannot_instantiate_with_device_add_yet, but it's not obvious to me why.
>> >
>> > The other functions are available with device-add. Users device-add'ing
>> > them would of course be odd, but if it works... I don't actually know
>> > whether it works for all of them.
>> >
>> > Should all southbridge functions be made unavailable with device-add for
>> > consistency, at least for now?
>> >
>> > Or should all functions be made available, except for the ones that
>> > cannot possibly work with device-add?
>> >
>> > If the latter, can you think of any specific reason why the IDE
>> > controllers couldn't work?
>>
>> I would've thought you and Kevin know more about IDE than me. ;)
>> No idea why it is how it is.
>>
>> Two aspects:
>> 1) PCI devices/functions can technically be hotplugged.
>> 2) Drivers might not expect such devices to be hot-added/removed.
>>
>> I would tend for the latter proposal.
>
> I'm not sure how IDE hardware really works, but I don't think you can
> handle it as a pure PCI device. On PCs, the IDE controller still provides
> the good old IDE registers on the I/O ports that were already used in
> ISA times, and they are not described in any BARs, for example.
Legacy IDE and VGA accesses are positively decoded in the i440fx and
dispatched to the first PCI device with the appropriate class code.
> From a
> software point of view, the PCI device is just for configuring Busmaster
> DMA. Perhaps in reality it should be two devices, one for DMA on the PCI
> bus and another one for the core on sysbus or ISA?
It's definitely all a PCI device. It could realistically be a
discrete device too that's plugged into a PCI bus that lacks a super
I/O chipset.
>
> Anyway, having two IDE controllers using the same I/O ports won't work,
> obviously. So if you would allow -device or device-add for them, you'd
> need options to configure the ports at least.
It's allowed but the PCI bus will only route the legacy requests to one of them.
Regards,
Anthony Liguori
> Kevin
>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] Which functions of southbridges should be no-user?
2013-10-15 15:09 ` Anthony Liguori
@ 2013-10-16 10:00 ` Markus Armbruster
2013-10-16 11:02 ` Andreas Färber
2013-10-16 17:53 ` Anthony Liguori
0 siblings, 2 replies; 38+ messages in thread
From: Markus Armbruster @ 2013-10-16 10:00 UTC (permalink / raw)
To: Anthony Liguori
Cc: Kevin Wolf, Peter Maydell, Andreas Färber, Anthony Liguori,
qemu-devel
Anthony Liguori <anthony@codemonkey.ws> writes:
> On Tue, Oct 15, 2013 at 7:41 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 15.10.2013 um 15:31 hat Andreas Färber geschrieben:
>>> Am 15.10.2013 15:21, schrieb Markus Armbruster:
>>> > Andreas,
>>> >
>>> > To go beyond RFC with this series, I need to explain why the IDE
>>> > controller functions of southbridges piix3-ide, piix3-ide-xen, piix4-ide
>>> > and via-ide cannot_instantiate_with_device_add_yet, or drop that. I'd
>>> > appreciate your help.
>>> >
>>> > Our modelling of PCI devices is weird, to put it politely. One of many
>>> > weird things is that we don't distinguish between a function and a
>>> > complete device: our "function models" are actually PCI device models,
>>> > and can be used as such, even though they only exist as functions of a
>>> > multifunction device in the real world. We permit collecting aribitrary
>>> > PCI devices into multifunction devices.
>>> >
>>> > One instance of multifunction PCI devices are southbridges. For
>>> > example, the ICH9 southbridge's PCI device 00:1F consists of ISA bridge
>>> > ("ICH9 LPC"), IDE controller ("ich9-ahci"), SMB controller ("ICH9 SMB"),
>>> > and Thermal Subsystem (which we don't implement). The PIIX3 southbridge
>>> > consists of ISA bridge ("PIIX3", IDE controller ("piix3-ide"), USB
>>> > controller ("piix3-usb-uhci", can be suppressed), and SMB controller
>>> > ("PIIX4_PM", can be suppressed).
>>> >
>>> > Some functions of southbridges still need to be wired up in code ("ICH9
>>> > LPC", "ICH9 SMB", "PIIX4_PM", "PIIX4", "PIIX3", "PIIX3-xen", see PATCH
>>> > 5-6/9), thus cannot_instantiate_with_device_add_yet.
>>> >
>>> > The IDE controller functions have always been
>>> > cannot_instantiate_with_device_add_yet, but it's not obvious to me why.
>>> >
>>> > The other functions are available with device-add. Users device-add'ing
>>> > them would of course be odd, but if it works... I don't actually know
>>> > whether it works for all of them.
>>> >
>>> > Should all southbridge functions be made unavailable with device-add for
>>> > consistency, at least for now?
>>> >
>>> > Or should all functions be made available, except for the ones that
>>> > cannot possibly work with device-add?
>>> >
>>> > If the latter, can you think of any specific reason why the IDE
>>> > controllers couldn't work?
>>>
>>> I would've thought you and Kevin know more about IDE than me. ;)
>>> No idea why it is how it is.
>>>
>>> Two aspects:
>>> 1) PCI devices/functions can technically be hotplugged.
>>> 2) Drivers might not expect such devices to be hot-added/removed.
>>>
>>> I would tend for the latter proposal.
>>
>> I'm not sure how IDE hardware really works, but I don't think you can
>> handle it as a pure PCI device. On PCs, the IDE controller still provides
>> the good old IDE registers on the I/O ports that were already used in
>> ISA times, and they are not described in any BARs, for example.
>
> Legacy IDE and VGA accesses are positively decoded in the i440fx and
> dispatched to the first PCI device with the appropriate class code.
>
>> From a
>> software point of view, the PCI device is just for configuring Busmaster
>> DMA. Perhaps in reality it should be two devices, one for DMA on the PCI
>> bus and another one for the core on sysbus or ISA?
>
> It's definitely all a PCI device. It could realistically be a
> discrete device too that's plugged into a PCI bus that lacks a super
> I/O chipset.
>
>>
>> Anyway, having two IDE controllers using the same I/O ports won't work,
>> obviously. So if you would allow -device or device-add for them, you'd
>> need options to configure the ports at least.
>
> It's allowed but the PCI bus will only route the legacy requests to one of them.
Any objections to dropping cannot_instantiate_with_device_add_yet from
all IDE controller southbridge functions? These are: piix3-ide,
piix3-ide-xen, piix4-ide and via-ide.
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] Which functions of southbridges should be no-user?
2013-10-16 10:00 ` Markus Armbruster
@ 2013-10-16 11:02 ` Andreas Färber
2013-10-17 9:47 ` Markus Armbruster
2013-10-16 17:53 ` Anthony Liguori
1 sibling, 1 reply; 38+ messages in thread
From: Andreas Färber @ 2013-10-16 11:02 UTC (permalink / raw)
To: Markus Armbruster, Anthony Liguori
Cc: Kevin Wolf, Peter Maydell, qemu-devel, Anthony Liguori
Am 16.10.2013 12:00, schrieb Markus Armbruster:
> Anthony Liguori <anthony@codemonkey.ws> writes:
>
>> On Tue, Oct 15, 2013 at 7:41 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>>> Am 15.10.2013 um 15:31 hat Andreas Färber geschrieben:
>>>> Am 15.10.2013 15:21, schrieb Markus Armbruster:
>>>>> Andreas,
>>>>>
>>>>> To go beyond RFC with this series, I need to explain why the IDE
>>>>> controller functions of southbridges piix3-ide, piix3-ide-xen, piix4-ide
>>>>> and via-ide cannot_instantiate_with_device_add_yet, or drop that. I'd
>>>>> appreciate your help.
>>>>>
>>>>> Our modelling of PCI devices is weird, to put it politely. One of many
>>>>> weird things is that we don't distinguish between a function and a
>>>>> complete device: our "function models" are actually PCI device models,
>>>>> and can be used as such, even though they only exist as functions of a
>>>>> multifunction device in the real world. We permit collecting aribitrary
>>>>> PCI devices into multifunction devices.
>>>>>
>>>>> One instance of multifunction PCI devices are southbridges. For
>>>>> example, the ICH9 southbridge's PCI device 00:1F consists of ISA bridge
>>>>> ("ICH9 LPC"), IDE controller ("ich9-ahci"), SMB controller ("ICH9 SMB"),
>>>>> and Thermal Subsystem (which we don't implement). The PIIX3 southbridge
>>>>> consists of ISA bridge ("PIIX3", IDE controller ("piix3-ide"), USB
>>>>> controller ("piix3-usb-uhci", can be suppressed), and SMB controller
>>>>> ("PIIX4_PM", can be suppressed).
>>>>>
>>>>> Some functions of southbridges still need to be wired up in code ("ICH9
>>>>> LPC", "ICH9 SMB", "PIIX4_PM", "PIIX4", "PIIX3", "PIIX3-xen", see PATCH
>>>>> 5-6/9), thus cannot_instantiate_with_device_add_yet.
>>>>>
>>>>> The IDE controller functions have always been
>>>>> cannot_instantiate_with_device_add_yet, but it's not obvious to me why.
>>>>>
>>>>> The other functions are available with device-add. Users device-add'ing
>>>>> them would of course be odd, but if it works... I don't actually know
>>>>> whether it works for all of them.
>>>>>
>>>>> Should all southbridge functions be made unavailable with device-add for
>>>>> consistency, at least for now?
>>>>>
>>>>> Or should all functions be made available, except for the ones that
>>>>> cannot possibly work with device-add?
>>>>>
>>>>> If the latter, can you think of any specific reason why the IDE
>>>>> controllers couldn't work?
>>>>
>>>> I would've thought you and Kevin know more about IDE than me. ;)
>>>> No idea why it is how it is.
>>>>
>>>> Two aspects:
>>>> 1) PCI devices/functions can technically be hotplugged.
>>>> 2) Drivers might not expect such devices to be hot-added/removed.
>>>>
>>>> I would tend for the latter proposal.
>>>
>>> I'm not sure how IDE hardware really works, but I don't think you can
>>> handle it as a pure PCI device. On PCs, the IDE controller still provides
>>> the good old IDE registers on the I/O ports that were already used in
>>> ISA times, and they are not described in any BARs, for example.
>>
>> Legacy IDE and VGA accesses are positively decoded in the i440fx and
>> dispatched to the first PCI device with the appropriate class code.
>>
>>> From a
>>> software point of view, the PCI device is just for configuring Busmaster
>>> DMA. Perhaps in reality it should be two devices, one for DMA on the PCI
>>> bus and another one for the core on sysbus or ISA?
>>
>> It's definitely all a PCI device. It could realistically be a
>> discrete device too that's plugged into a PCI bus that lacks a super
>> I/O chipset.
>>
>>>
>>> Anyway, having two IDE controllers using the same I/O ports won't work,
>>> obviously. So if you would allow -device or device-add for them, you'd
>>> need options to configure the ports at least.
>>
>> It's allowed but the PCI bus will only route the legacy requests to one of them.
>
> Any objections to dropping cannot_instantiate_with_device_add_yet from
> all IDE controller southbridge functions? These are: piix3-ide,
> piix3-ide-xen, piix4-ide and via-ide.
I understood Anthony as describing expected hardware behavior.
At least with the old ioport API registering a port twice used to abort.
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] Which functions of southbridges should be no-user?
2013-10-16 11:02 ` Andreas Färber
@ 2013-10-17 9:47 ` Markus Armbruster
0 siblings, 0 replies; 38+ messages in thread
From: Markus Armbruster @ 2013-10-17 9:47 UTC (permalink / raw)
To: Andreas Färber
Cc: Kevin Wolf, Peter Maydell, Anthony Liguori, Anthony Liguori,
qemu-devel
Andreas Färber <afaerber@suse.de> writes:
> Am 16.10.2013 12:00, schrieb Markus Armbruster:
>> Anthony Liguori <anthony@codemonkey.ws> writes:
>>
>>> On Tue, Oct 15, 2013 at 7:41 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>>>> Am 15.10.2013 um 15:31 hat Andreas Färber geschrieben:
>>>>> Am 15.10.2013 15:21, schrieb Markus Armbruster:
>>>>>> Andreas,
>>>>>>
>>>>>> To go beyond RFC with this series, I need to explain why the IDE
>>>>>> controller functions of southbridges piix3-ide, piix3-ide-xen, piix4-ide
>>>>>> and via-ide cannot_instantiate_with_device_add_yet, or drop that. I'd
>>>>>> appreciate your help.
>>>>>>
>>>>>> Our modelling of PCI devices is weird, to put it politely. One of many
>>>>>> weird things is that we don't distinguish between a function and a
>>>>>> complete device: our "function models" are actually PCI device models,
>>>>>> and can be used as such, even though they only exist as functions of a
>>>>>> multifunction device in the real world. We permit collecting aribitrary
>>>>>> PCI devices into multifunction devices.
>>>>>>
>>>>>> One instance of multifunction PCI devices are southbridges. For
>>>>>> example, the ICH9 southbridge's PCI device 00:1F consists of ISA bridge
>>>>>> ("ICH9 LPC"), IDE controller ("ich9-ahci"), SMB controller ("ICH9 SMB"),
>>>>>> and Thermal Subsystem (which we don't implement). The PIIX3 southbridge
>>>>>> consists of ISA bridge ("PIIX3", IDE controller ("piix3-ide"), USB
>>>>>> controller ("piix3-usb-uhci", can be suppressed), and SMB controller
>>>>>> ("PIIX4_PM", can be suppressed).
>>>>>>
>>>>>> Some functions of southbridges still need to be wired up in code ("ICH9
>>>>>> LPC", "ICH9 SMB", "PIIX4_PM", "PIIX4", "PIIX3", "PIIX3-xen", see PATCH
>>>>>> 5-6/9), thus cannot_instantiate_with_device_add_yet.
>>>>>>
>>>>>> The IDE controller functions have always been
>>>>>> cannot_instantiate_with_device_add_yet, but it's not obvious to me why.
>>>>>>
>>>>>> The other functions are available with device-add. Users device-add'ing
>>>>>> them would of course be odd, but if it works... I don't actually know
>>>>>> whether it works for all of them.
>>>>>>
>>>>>> Should all southbridge functions be made unavailable with device-add for
>>>>>> consistency, at least for now?
>>>>>>
>>>>>> Or should all functions be made available, except for the ones that
>>>>>> cannot possibly work with device-add?
>>>>>>
>>>>>> If the latter, can you think of any specific reason why the IDE
>>>>>> controllers couldn't work?
>>>>>
>>>>> I would've thought you and Kevin know more about IDE than me. ;)
>>>>> No idea why it is how it is.
>>>>>
>>>>> Two aspects:
>>>>> 1) PCI devices/functions can technically be hotplugged.
>>>>> 2) Drivers might not expect such devices to be hot-added/removed.
>>>>>
>>>>> I would tend for the latter proposal.
>>>>
>>>> I'm not sure how IDE hardware really works, but I don't think you can
>>>> handle it as a pure PCI device. On PCs, the IDE controller still provides
>>>> the good old IDE registers on the I/O ports that were already used in
>>>> ISA times, and they are not described in any BARs, for example.
>>>
>>> Legacy IDE and VGA accesses are positively decoded in the i440fx and
>>> dispatched to the first PCI device with the appropriate class code.
>>>
>>>> From a
>>>> software point of view, the PCI device is just for configuring Busmaster
>>>> DMA. Perhaps in reality it should be two devices, one for DMA on the PCI
>>>> bus and another one for the core on sysbus or ISA?
>>>
>>> It's definitely all a PCI device. It could realistically be a
>>> discrete device too that's plugged into a PCI bus that lacks a super
>>> I/O chipset.
>>>
>>>>
>>>> Anyway, having two IDE controllers using the same I/O ports won't work,
>>>> obviously. So if you would allow -device or device-add for them, you'd
>>>> need options to configure the ports at least.
>>>
>>> It's allowed but the PCI bus will only route the legacy requests to
>>> one of them.
>>
>> Any objections to dropping cannot_instantiate_with_device_add_yet from
>> all IDE controller southbridge functions? These are: piix3-ide,
>> piix3-ide-xen, piix4-ide and via-ide.
>
> I understood Anthony as describing expected hardware behavior.
>
> At least with the old ioport API registering a port twice used to abort.
If I add a second piix3-ide controller in the "wrong" order, the BIOS
attempts to boot from the wrong one, and fails.
If I add it in the other order, Linux starts booting, but fails to pivot
the root device, and drops me in an emergency shell. I can see both
controllers in /sys/devices/ there.
Unfortunately, I don't have time today to figure out how to make the
second controller work.
Good enough to drop cannot_instantiate_with_device_add_yet?
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] Which functions of southbridges should be no-user?
2013-10-16 10:00 ` Markus Armbruster
2013-10-16 11:02 ` Andreas Färber
@ 2013-10-16 17:53 ` Anthony Liguori
2013-10-17 9:49 ` Markus Armbruster
1 sibling, 1 reply; 38+ messages in thread
From: Anthony Liguori @ 2013-10-16 17:53 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, Peter Maydell, Andreas Färber, Anthony Liguori,
qemu-devel
On Wed, Oct 16, 2013 at 3:00 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Anthony Liguori <anthony@codemonkey.ws> writes:
>
>> On Tue, Oct 15, 2013 at 7:41 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>>> Am 15.10.2013 um 15:31 hat Andreas Färber geschrieben:
>>>> Am 15.10.2013 15:21, schrieb Markus Armbruster:
>>>> > Andreas,
>>>> >
>>>> > To go beyond RFC with this series, I need to explain why the IDE
>>>> > controller functions of southbridges piix3-ide, piix3-ide-xen, piix4-ide
>>>> > and via-ide cannot_instantiate_with_device_add_yet, or drop that. I'd
>>>> > appreciate your help.
>>>> >
>>>> > Our modelling of PCI devices is weird, to put it politely. One of many
>>>> > weird things is that we don't distinguish between a function and a
>>>> > complete device: our "function models" are actually PCI device models,
>>>> > and can be used as such, even though they only exist as functions of a
>>>> > multifunction device in the real world. We permit collecting aribitrary
>>>> > PCI devices into multifunction devices.
>>>> >
>>>> > One instance of multifunction PCI devices are southbridges. For
>>>> > example, the ICH9 southbridge's PCI device 00:1F consists of ISA bridge
>>>> > ("ICH9 LPC"), IDE controller ("ich9-ahci"), SMB controller ("ICH9 SMB"),
>>>> > and Thermal Subsystem (which we don't implement). The PIIX3 southbridge
>>>> > consists of ISA bridge ("PIIX3", IDE controller ("piix3-ide"), USB
>>>> > controller ("piix3-usb-uhci", can be suppressed), and SMB controller
>>>> > ("PIIX4_PM", can be suppressed).
>>>> >
>>>> > Some functions of southbridges still need to be wired up in code ("ICH9
>>>> > LPC", "ICH9 SMB", "PIIX4_PM", "PIIX4", "PIIX3", "PIIX3-xen", see PATCH
>>>> > 5-6/9), thus cannot_instantiate_with_device_add_yet.
>>>> >
>>>> > The IDE controller functions have always been
>>>> > cannot_instantiate_with_device_add_yet, but it's not obvious to me why.
>>>> >
>>>> > The other functions are available with device-add. Users device-add'ing
>>>> > them would of course be odd, but if it works... I don't actually know
>>>> > whether it works for all of them.
>>>> >
>>>> > Should all southbridge functions be made unavailable with device-add for
>>>> > consistency, at least for now?
>>>> >
>>>> > Or should all functions be made available, except for the ones that
>>>> > cannot possibly work with device-add?
>>>> >
>>>> > If the latter, can you think of any specific reason why the IDE
>>>> > controllers couldn't work?
>>>>
>>>> I would've thought you and Kevin know more about IDE than me. ;)
>>>> No idea why it is how it is.
>>>>
>>>> Two aspects:
>>>> 1) PCI devices/functions can technically be hotplugged.
>>>> 2) Drivers might not expect such devices to be hot-added/removed.
>>>>
>>>> I would tend for the latter proposal.
>>>
>>> I'm not sure how IDE hardware really works, but I don't think you can
>>> handle it as a pure PCI device. On PCs, the IDE controller still provides
>>> the good old IDE registers on the I/O ports that were already used in
>>> ISA times, and they are not described in any BARs, for example.
>>
>> Legacy IDE and VGA accesses are positively decoded in the i440fx and
>> dispatched to the first PCI device with the appropriate class code.
>>
>>> From a
>>> software point of view, the PCI device is just for configuring Busmaster
>>> DMA. Perhaps in reality it should be two devices, one for DMA on the PCI
>>> bus and another one for the core on sysbus or ISA?
>>
>> It's definitely all a PCI device. It could realistically be a
>> discrete device too that's plugged into a PCI bus that lacks a super
>> I/O chipset.
>>
>>>
>>> Anyway, having two IDE controllers using the same I/O ports won't work,
>>> obviously. So if you would allow -device or device-add for them, you'd
>>> need options to configure the ports at least.
>>
>> It's allowed but the PCI bus will only route the legacy requests to one of them.
>
> Any objections to dropping cannot_instantiate_with_device_add_yet from
> all IDE controller southbridge functions? These are: piix3-ide,
> piix3-ide-xen, piix4-ide and via-ide.
No, I don't object. As Andreas points out, we may have problems today
if you try to add two IDE controllers but those are bugs that should
be fixed.
It used to be possible to buy secondary PCI IDE controllers to add
more than 4 disks. You could only access them with proper drivers (so
you couldn't boot from the 5th disk) but otherwise it worked.
Regards,
Anthony Liguori
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] Which functions of southbridges should be no-user?
2013-10-16 17:53 ` Anthony Liguori
@ 2013-10-17 9:49 ` Markus Armbruster
0 siblings, 0 replies; 38+ messages in thread
From: Markus Armbruster @ 2013-10-17 9:49 UTC (permalink / raw)
To: Anthony Liguori
Cc: Kevin Wolf, Peter Maydell, Andreas Färber, Anthony Liguori,
qemu-devel
Anthony Liguori <anthony@codemonkey.ws> writes:
> On Wed, Oct 16, 2013 at 3:00 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> Anthony Liguori <anthony@codemonkey.ws> writes:
>>
>>> On Tue, Oct 15, 2013 at 7:41 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>>>> Am 15.10.2013 um 15:31 hat Andreas Färber geschrieben:
>>>>> Am 15.10.2013 15:21, schrieb Markus Armbruster:
>>>>> > Andreas,
>>>>> >
>>>>> > To go beyond RFC with this series, I need to explain why the IDE
>>>>> > controller functions of southbridges piix3-ide, piix3-ide-xen, piix4-ide
>>>>> > and via-ide cannot_instantiate_with_device_add_yet, or drop that. I'd
>>>>> > appreciate your help.
>>>>> >
>>>>> > Our modelling of PCI devices is weird, to put it politely. One of many
>>>>> > weird things is that we don't distinguish between a function and a
>>>>> > complete device: our "function models" are actually PCI device models,
>>>>> > and can be used as such, even though they only exist as functions of a
>>>>> > multifunction device in the real world. We permit collecting aribitrary
>>>>> > PCI devices into multifunction devices.
>>>>> >
>>>>> > One instance of multifunction PCI devices are southbridges. For
>>>>> > example, the ICH9 southbridge's PCI device 00:1F consists of ISA bridge
>>>>> > ("ICH9 LPC"), IDE controller ("ich9-ahci"), SMB controller ("ICH9 SMB"),
>>>>> > and Thermal Subsystem (which we don't implement). The PIIX3 southbridge
>>>>> > consists of ISA bridge ("PIIX3", IDE controller ("piix3-ide"), USB
>>>>> > controller ("piix3-usb-uhci", can be suppressed), and SMB controller
>>>>> > ("PIIX4_PM", can be suppressed).
>>>>> >
>>>>> > Some functions of southbridges still need to be wired up in code ("ICH9
>>>>> > LPC", "ICH9 SMB", "PIIX4_PM", "PIIX4", "PIIX3", "PIIX3-xen", see PATCH
>>>>> > 5-6/9), thus cannot_instantiate_with_device_add_yet.
>>>>> >
>>>>> > The IDE controller functions have always been
>>>>> > cannot_instantiate_with_device_add_yet, but it's not obvious to me why.
>>>>> >
>>>>> > The other functions are available with device-add. Users device-add'ing
>>>>> > them would of course be odd, but if it works... I don't actually know
>>>>> > whether it works for all of them.
>>>>> >
>>>>> > Should all southbridge functions be made unavailable with device-add for
>>>>> > consistency, at least for now?
>>>>> >
>>>>> > Or should all functions be made available, except for the ones that
>>>>> > cannot possibly work with device-add?
>>>>> >
>>>>> > If the latter, can you think of any specific reason why the IDE
>>>>> > controllers couldn't work?
>>>>>
>>>>> I would've thought you and Kevin know more about IDE than me. ;)
>>>>> No idea why it is how it is.
>>>>>
>>>>> Two aspects:
>>>>> 1) PCI devices/functions can technically be hotplugged.
>>>>> 2) Drivers might not expect such devices to be hot-added/removed.
>>>>>
>>>>> I would tend for the latter proposal.
>>>>
>>>> I'm not sure how IDE hardware really works, but I don't think you can
>>>> handle it as a pure PCI device. On PCs, the IDE controller still provides
>>>> the good old IDE registers on the I/O ports that were already used in
>>>> ISA times, and they are not described in any BARs, for example.
>>>
>>> Legacy IDE and VGA accesses are positively decoded in the i440fx and
>>> dispatched to the first PCI device with the appropriate class code.
>>>
>>>> From a
>>>> software point of view, the PCI device is just for configuring Busmaster
>>>> DMA. Perhaps in reality it should be two devices, one for DMA on the PCI
>>>> bus and another one for the core on sysbus or ISA?
>>>
>>> It's definitely all a PCI device. It could realistically be a
>>> discrete device too that's plugged into a PCI bus that lacks a super
>>> I/O chipset.
>>>
>>>>
>>>> Anyway, having two IDE controllers using the same I/O ports won't work,
>>>> obviously. So if you would allow -device or device-add for them, you'd
>>>> need options to configure the ports at least.
>>>
>>> It's allowed but the PCI bus will only route the legacy requests to
>>> one of them.
>>
>> Any objections to dropping cannot_instantiate_with_device_add_yet from
>> all IDE controller southbridge functions? These are: piix3-ide,
>> piix3-ide-xen, piix4-ide and via-ide.
>
> No, I don't object. As Andreas points out, we may have problems today
> if you try to add two IDE controllers but those are bugs that should
> be fixed.
The mission of cannot_instantiate_with_device_add_yet is protecting
users from device-adds that cannot possibly work. So, *if* these
devices actually have bugs that are so severe that device-add becomes
effectively useless, then we should keep
cannot_instantiate_with_device_add_yet for now. But that's detail.
In my very limited testing, I wasn't able to get a system with two
piix3-ide to boot, but it didn't look flat out impossible. See my reply
to Andreas.
> It used to be possible to buy secondary PCI IDE controllers to add
> more than 4 disks. You could only access them with proper drivers (so
> you couldn't boot from the 5th disk) but otherwise it worked.
Yes, but these IDE boards didn't identify as intel 82371SB. For what
it's worth, we have an IDE controller device that isn't a southbridge
function: cmd646-ide.
^ permalink raw reply [flat|nested] 38+ messages in thread