* [Qemu-devel] [PATCH 01/15] pc: merge pc_piix.c into pc.c
2012-01-26 19:00 [Qemu-devel] [RFC 00/15] Refactor PC machine to take advantage of QOM Anthony Liguori
@ 2012-01-26 19:00 ` Anthony Liguori
2012-01-26 19:40 ` Anthony Liguori
2012-01-27 8:50 ` Jan Kiszka
2012-01-26 19:00 ` [Qemu-devel] [PATCH 02/15] pc: make some functions static Anthony Liguori
` (14 subsequent siblings)
15 siblings, 2 replies; 55+ messages in thread
From: Anthony Liguori @ 2012-01-26 19:00 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Anthony Liguori, Jan Kiszka, Markus Armbruster,
Avi Kivity, Paolo Bonzini
A long time ago, there was a grand plan to merge q35 chipset support. The start
of that series was a refactoring of pc.c which split a bunch of the "common"
functionality into a separate file that could be shared by the two.
But q35 never got merged and the refactoring, in retrospect, just made things
worse. Making things proper objects and using composition is the right way
to share common devices.
By pulling these files back together, we can start to fix some of this mess.
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
Makefile.target | 1 -
hw/pc.c | 644 +++++++++++++++++++++++++++++++++++++++++++++++++++
hw/pc_piix.c | 686 -------------------------------------------------------
3 files changed, 644 insertions(+), 687 deletions(-)
delete mode 100644 hw/pc_piix.c
diff --git a/Makefile.target b/Makefile.target
index 2c763a1..63afaf9 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -235,7 +235,6 @@ obj-i386-y += sga.o apic_common.o apic.o ioapic_common.o ioapic.o piix_pci.o
obj-i386-y += vmport.o
obj-i386-y += pci-hotplug.o smbios.o wdt_ib700.o
obj-i386-y += debugcon.o multiboot.o
-obj-i386-y += pc_piix.o
obj-i386-$(CONFIG_KVM) += kvm/clock.o kvm/apic.o kvm/i8259.o kvm/ioapic.o
obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
diff --git a/hw/pc.c b/hw/pc.c
index 7f3aa65..3d40aca 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -44,6 +44,12 @@
#include "ui/qemu-spice.h"
#include "memory.h"
#include "exec-memory.h"
+#include "kvm/clock.h"
+#include "xen.h"
+#include "arch_init.h"
+#include "usb-uhci.h"
+#include "smbus.h"
+#include "boards.h"
/* output Bochs bios info messages */
//#define DEBUG_BIOS
@@ -75,6 +81,8 @@
#define E820_NR_ENTRIES 16
+#define MAX_IDE_BUS 2
+
struct e820_entry {
uint64_t address;
uint64_t length;
@@ -86,6 +94,10 @@ struct e820_table {
struct e820_entry entry[E820_NR_ENTRIES];
} QEMU_PACKED __attribute((__aligned__(4)));
+static const int ide_iobase[MAX_IDE_BUS] = { 0x1f0, 0x170 };
+static const int ide_iobase2[MAX_IDE_BUS] = { 0x3f6, 0x376 };
+static const int ide_irq[MAX_IDE_BUS] = { 14, 15 };
+
static struct e820_table e820_table;
struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
@@ -1210,3 +1222,635 @@ void pc_pci_device_init(PCIBus *pci_bus)
pci_create_simple(pci_bus, -1, "lsi53c895a");
}
}
+
+static void kvm_piix3_setup_irq_routing(bool pci_enabled)
+{
+#ifdef CONFIG_KVM
+ KVMState *s = kvm_state;
+ int ret, i;
+
+ if (kvm_check_extension(s, KVM_CAP_IRQ_ROUTING)) {
+ for (i = 0; i < 8; ++i) {
+ if (i == 2) {
+ continue;
+ }
+ kvm_irqchip_add_route(s, i, KVM_IRQCHIP_PIC_MASTER, i);
+ }
+ for (i = 8; i < 16; ++i) {
+ kvm_irqchip_add_route(s, i, KVM_IRQCHIP_PIC_SLAVE, i - 8);
+ }
+ if (pci_enabled) {
+ for (i = 0; i < 24; ++i) {
+ if (i == 0) {
+ kvm_irqchip_add_route(s, i, KVM_IRQCHIP_IOAPIC, 2);
+ } else if (i != 2) {
+ kvm_irqchip_add_route(s, i, KVM_IRQCHIP_IOAPIC, i);
+ }
+ }
+ }
+ ret = kvm_irqchip_commit_routes(s);
+ if (ret < 0) {
+ hw_error("KVM IRQ routing setup failed");
+ }
+ }
+#endif /* CONFIG_KVM */
+}
+
+static void kvm_piix3_gsi_handler(void *opaque, int n, int level)
+{
+ GSIState *s = opaque;
+
+ if (n < ISA_NUM_IRQS) {
+ /* Kernel will forward to both PIC and IOAPIC */
+ qemu_set_irq(s->i8259_irq[n], level);
+ } else {
+ qemu_set_irq(s->ioapic_irq[n], level);
+ }
+}
+
+static void ioapic_init(GSIState *gsi_state)
+{
+ DeviceState *dev;
+ SysBusDevice *d;
+ unsigned int i;
+
+ if (kvm_enabled() && kvm_irqchip_in_kernel()) {
+ dev = qdev_create(NULL, "kvm-ioapic");
+ } else {
+ dev = qdev_create(NULL, "ioapic");
+ }
+ qdev_init_nofail(dev);
+ d = sysbus_from_qdev(dev);
+ sysbus_mmio_map(d, 0, 0xfec00000);
+
+ for (i = 0; i < IOAPIC_NUM_PINS; i++) {
+ gsi_state->ioapic_irq[i] = qdev_get_gpio_in(dev, i);
+ }
+}
+
+/* PC hardware initialisation */
+static void pc_init1(MemoryRegion *system_memory,
+ MemoryRegion *system_io,
+ ram_addr_t ram_size,
+ const char *boot_device,
+ const char *kernel_filename,
+ const char *kernel_cmdline,
+ const char *initrd_filename,
+ const char *cpu_model,
+ int pci_enabled,
+ int kvmclock_enabled)
+{
+ int i;
+ ram_addr_t below_4g_mem_size, above_4g_mem_size;
+ PCIBus *pci_bus;
+ ISABus *isa_bus;
+ PCII440FXState *i440fx_state;
+ int piix3_devfn = -1;
+ qemu_irq *cpu_irq;
+ qemu_irq *gsi;
+ qemu_irq *i8259;
+ qemu_irq *cmos_s3;
+ qemu_irq *smi_irq;
+ GSIState *gsi_state;
+ DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
+ BusState *idebus[MAX_IDE_BUS];
+ ISADevice *rtc_state;
+ ISADevice *floppy;
+ MemoryRegion *ram_memory;
+ MemoryRegion *pci_memory;
+ MemoryRegion *rom_memory;
+ DeviceState *dev;
+
+ pc_cpus_init(cpu_model);
+
+ if (kvmclock_enabled) {
+ kvmclock_create();
+ }
+
+ if (ram_size >= 0xe0000000 ) {
+ above_4g_mem_size = ram_size - 0xe0000000;
+ below_4g_mem_size = 0xe0000000;
+ } else {
+ above_4g_mem_size = 0;
+ below_4g_mem_size = ram_size;
+ }
+
+ if (pci_enabled) {
+ pci_memory = g_new(MemoryRegion, 1);
+ memory_region_init(pci_memory, "pci", INT64_MAX);
+ rom_memory = pci_memory;
+ } else {
+ pci_memory = NULL;
+ rom_memory = system_memory;
+ }
+
+ /* allocate ram and load rom/bios */
+ if (!xen_enabled()) {
+ pc_memory_init(system_memory,
+ kernel_filename, kernel_cmdline, initrd_filename,
+ below_4g_mem_size, above_4g_mem_size,
+ pci_enabled ? rom_memory : system_memory, &ram_memory);
+ }
+
+ gsi_state = g_malloc0(sizeof(*gsi_state));
+ if (kvm_enabled() && kvm_irqchip_in_kernel()) {
+ kvm_piix3_setup_irq_routing(pci_enabled);
+ gsi = qemu_allocate_irqs(kvm_piix3_gsi_handler, gsi_state,
+ GSI_NUM_PINS);
+ } else {
+ gsi = qemu_allocate_irqs(gsi_handler, gsi_state, GSI_NUM_PINS);
+ }
+
+ if (pci_enabled) {
+ pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,
+ system_memory, system_io, ram_size,
+ below_4g_mem_size,
+ 0x100000000ULL - below_4g_mem_size,
+ 0x100000000ULL + above_4g_mem_size,
+ (sizeof(target_phys_addr_t) == 4
+ ? 0
+ : ((uint64_t)1 << 62)),
+ pci_memory, ram_memory);
+ } else {
+ pci_bus = NULL;
+ i440fx_state = NULL;
+ isa_bus = isa_bus_new(NULL, system_io);
+ no_hpet = 1;
+ }
+ isa_bus_irqs(isa_bus, gsi);
+
+ if (kvm_enabled() && kvm_irqchip_in_kernel()) {
+ i8259 = kvm_i8259_init(isa_bus);
+ } else if (xen_enabled()) {
+ i8259 = xen_interrupt_controller_init();
+ } else {
+ cpu_irq = pc_allocate_cpu_irq();
+ i8259 = i8259_init(isa_bus, cpu_irq[0]);
+ }
+
+ for (i = 0; i < ISA_NUM_IRQS; i++) {
+ gsi_state->i8259_irq[i] = i8259[i];
+ }
+ if (pci_enabled) {
+ ioapic_init(gsi_state);
+ }
+
+ pc_register_ferr_irq(gsi[13]);
+
+ dev = pc_vga_init(isa_bus, pci_enabled ? pci_bus : NULL);
+ if (dev) {
+ object_property_add_child(object_get_root(), "vga", OBJECT(dev), NULL);
+ }
+
+ if (xen_enabled()) {
+ pci_create_simple(pci_bus, -1, "xen-platform");
+ }
+
+ /* init basic PC hardware */
+ pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, xen_enabled());
+
+ for(i = 0; i < nb_nics; i++) {
+ NICInfo *nd = &nd_table[i];
+
+ if (!pci_enabled || (nd->model && strcmp(nd->model, "ne2k_isa") == 0))
+ pc_init_ne2k_isa(isa_bus, nd);
+ else
+ pci_nic_init_nofail(nd, "e1000", NULL);
+ }
+
+ ide_drive_get(hd, MAX_IDE_BUS);
+ if (pci_enabled) {
+ PCIDevice *dev;
+ if (xen_enabled()) {
+ dev = pci_piix3_xen_ide_init(pci_bus, hd, piix3_devfn + 1);
+ } else {
+ dev = pci_piix3_ide_init(pci_bus, hd, piix3_devfn + 1);
+ }
+ idebus[0] = qdev_get_child_bus(&dev->qdev, "ide.0");
+ idebus[1] = qdev_get_child_bus(&dev->qdev, "ide.1");
+
+ /* FIXME there's some major spaghetti here. Somehow we create the
+ * devices on the PIIX before we actually create it. We create the
+ * PIIX3 deep in the recess of the i440fx creation too and then lose
+ * the DeviceState.
+ *
+ * For now, let's "fix" this by making judicious use of paths. This
+ * is not generally the right way to do this.
+ */
+ object_property_add_child(object_resolve_path("/i440fx/piix3", NULL),
+ "rtc", (Object *)rtc_state, NULL);
+ } else {
+ for(i = 0; i < MAX_IDE_BUS; i++) {
+ ISADevice *dev;
+ dev = isa_ide_init(isa_bus, ide_iobase[i], ide_iobase2[i],
+ ide_irq[i],
+ hd[MAX_IDE_DEVS * i], hd[MAX_IDE_DEVS * i + 1]);
+ idebus[i] = qdev_get_child_bus(&dev->qdev, "ide.0");
+ }
+ }
+
+ audio_init(isa_bus, pci_enabled ? pci_bus : NULL);
+
+ pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device,
+ floppy, idebus[0], idebus[1], rtc_state);
+
+ if (pci_enabled && usb_enabled) {
+ usb_uhci_piix3_init(pci_bus, piix3_devfn + 2);
+ }
+
+ if (pci_enabled && acpi_enabled) {
+ i2c_bus *smbus;
+
+ if (!xen_enabled()) {
+ cmos_s3 = qemu_allocate_irqs(pc_cmos_set_s3_resume, rtc_state, 1);
+ } else {
+ cmos_s3 = qemu_allocate_irqs(xen_cmos_set_s3_resume, rtc_state, 1);
+ }
+ smi_irq = qemu_allocate_irqs(pc_acpi_smi_interrupt, first_cpu, 1);
+ /* TODO: Populate SPD eeprom data. */
+ smbus = piix4_pm_init(pci_bus, piix3_devfn + 3, 0xb100,
+ gsi[9], *cmos_s3, *smi_irq,
+ kvm_enabled());
+ smbus_eeprom_init(smbus, 8, NULL, 0);
+ }
+
+ if (pci_enabled) {
+ pc_pci_device_init(pci_bus);
+ }
+}
+
+static void pc_init_pci(ram_addr_t ram_size,
+ const char *boot_device,
+ const char *kernel_filename,
+ const char *kernel_cmdline,
+ const char *initrd_filename,
+ const char *cpu_model)
+{
+ pc_init1(get_system_memory(),
+ get_system_io(),
+ ram_size, boot_device,
+ kernel_filename, kernel_cmdline,
+ initrd_filename, cpu_model, 1, 1);
+}
+
+static void pc_init_pci_no_kvmclock(ram_addr_t ram_size,
+ const char *boot_device,
+ const char *kernel_filename,
+ const char *kernel_cmdline,
+ const char *initrd_filename,
+ const char *cpu_model)
+{
+ pc_init1(get_system_memory(),
+ get_system_io(),
+ ram_size, boot_device,
+ kernel_filename, kernel_cmdline,
+ initrd_filename, cpu_model, 1, 0);
+}
+
+static void pc_init_isa(ram_addr_t ram_size,
+ const char *boot_device,
+ const char *kernel_filename,
+ const char *kernel_cmdline,
+ const char *initrd_filename,
+ const char *cpu_model)
+{
+ if (cpu_model == NULL)
+ cpu_model = "486";
+ pc_init1(get_system_memory(),
+ get_system_io(),
+ ram_size, boot_device,
+ kernel_filename, kernel_cmdline,
+ initrd_filename, cpu_model, 0, 1);
+}
+
+#ifdef CONFIG_XEN
+static void pc_xen_hvm_init(ram_addr_t ram_size,
+ const char *boot_device,
+ const char *kernel_filename,
+ const char *kernel_cmdline,
+ const char *initrd_filename,
+ const char *cpu_model)
+{
+ if (xen_hvm_init() != 0) {
+ hw_error("xen hardware virtual machine initialisation failed");
+ }
+ pc_init_pci_no_kvmclock(ram_size, boot_device,
+ kernel_filename, kernel_cmdline,
+ initrd_filename, cpu_model);
+ xen_vcpu_init();
+}
+#endif
+
+static QEMUMachine pc_machine_v1_0 = {
+ .name = "pc-1.0",
+ .alias = "pc",
+ .desc = "Standard PC",
+ .init = pc_init_pci,
+ .max_cpus = 255,
+ .is_default = 1,
+};
+
+static QEMUMachine pc_machine_v0_15 = {
+ .name = "pc-0.15",
+ .desc = "Standard PC",
+ .init = pc_init_pci,
+ .max_cpus = 255,
+ .is_default = 1,
+};
+
+static QEMUMachine pc_machine_v0_14 = {
+ .name = "pc-0.14",
+ .desc = "Standard PC",
+ .init = pc_init_pci,
+ .max_cpus = 255,
+ .compat_props = (GlobalProperty[]) {
+ {
+ .driver = "qxl",
+ .property = "revision",
+ .value = stringify(2),
+ },{
+ .driver = "qxl-vga",
+ .property = "revision",
+ .value = stringify(2),
+ },{
+ .driver = "virtio-blk-pci",
+ .property = "event_idx",
+ .value = "off",
+ },{
+ .driver = "virtio-serial-pci",
+ .property = "event_idx",
+ .value = "off",
+ },{
+ .driver = "virtio-net-pci",
+ .property = "event_idx",
+ .value = "off",
+ },{
+ .driver = "virtio-balloon-pci",
+ .property = "event_idx",
+ .value = "off",
+ },
+ { /* end of list */ }
+ },
+};
+
+static QEMUMachine pc_machine_v0_13 = {
+ .name = "pc-0.13",
+ .desc = "Standard PC",
+ .init = pc_init_pci_no_kvmclock,
+ .max_cpus = 255,
+ .compat_props = (GlobalProperty[]) {
+ {
+ .driver = "virtio-9p-pci",
+ .property = "vectors",
+ .value = stringify(0),
+ },{
+ .driver = "VGA",
+ .property = "rombar",
+ .value = stringify(0),
+ },{
+ .driver = "vmware-svga",
+ .property = "rombar",
+ .value = stringify(0),
+ },{
+ .driver = "PCI",
+ .property = "command_serr_enable",
+ .value = "off",
+ },{
+ .driver = "virtio-blk-pci",
+ .property = "event_idx",
+ .value = "off",
+ },{
+ .driver = "virtio-serial-pci",
+ .property = "event_idx",
+ .value = "off",
+ },{
+ .driver = "virtio-net-pci",
+ .property = "event_idx",
+ .value = "off",
+ },{
+ .driver = "virtio-balloon-pci",
+ .property = "event_idx",
+ .value = "off",
+ },{
+ .driver = "AC97",
+ .property = "use_broken_id",
+ .value = stringify(1),
+ },
+ { /* end of list */ }
+ },
+};
+
+static QEMUMachine pc_machine_v0_12 = {
+ .name = "pc-0.12",
+ .desc = "Standard PC",
+ .init = pc_init_pci_no_kvmclock,
+ .max_cpus = 255,
+ .compat_props = (GlobalProperty[]) {
+ {
+ .driver = "virtio-serial-pci",
+ .property = "max_ports",
+ .value = stringify(1),
+ },{
+ .driver = "virtio-serial-pci",
+ .property = "vectors",
+ .value = stringify(0),
+ },{
+ .driver = "VGA",
+ .property = "rombar",
+ .value = stringify(0),
+ },{
+ .driver = "vmware-svga",
+ .property = "rombar",
+ .value = stringify(0),
+ },{
+ .driver = "PCI",
+ .property = "command_serr_enable",
+ .value = "off",
+ },{
+ .driver = "virtio-blk-pci",
+ .property = "event_idx",
+ .value = "off",
+ },{
+ .driver = "virtio-serial-pci",
+ .property = "event_idx",
+ .value = "off",
+ },{
+ .driver = "virtio-net-pci",
+ .property = "event_idx",
+ .value = "off",
+ },{
+ .driver = "virtio-balloon-pci",
+ .property = "event_idx",
+ .value = "off",
+ },{
+ .driver = "AC97",
+ .property = "use_broken_id",
+ .value = stringify(1),
+ },
+ { /* end of list */ }
+ }
+};
+
+static QEMUMachine pc_machine_v0_11 = {
+ .name = "pc-0.11",
+ .desc = "Standard PC, qemu 0.11",
+ .init = pc_init_pci_no_kvmclock,
+ .max_cpus = 255,
+ .compat_props = (GlobalProperty[]) {
+ {
+ .driver = "virtio-blk-pci",
+ .property = "vectors",
+ .value = stringify(0),
+ },{
+ .driver = "virtio-serial-pci",
+ .property = "max_ports",
+ .value = stringify(1),
+ },{
+ .driver = "virtio-serial-pci",
+ .property = "vectors",
+ .value = stringify(0),
+ },{
+ .driver = "ide-drive",
+ .property = "ver",
+ .value = "0.11",
+ },{
+ .driver = "scsi-disk",
+ .property = "ver",
+ .value = "0.11",
+ },{
+ .driver = "PCI",
+ .property = "rombar",
+ .value = stringify(0),
+ },{
+ .driver = "PCI",
+ .property = "command_serr_enable",
+ .value = "off",
+ },{
+ .driver = "virtio-blk-pci",
+ .property = "event_idx",
+ .value = "off",
+ },{
+ .driver = "virtio-serial-pci",
+ .property = "event_idx",
+ .value = "off",
+ },{
+ .driver = "virtio-net-pci",
+ .property = "event_idx",
+ .value = "off",
+ },{
+ .driver = "virtio-balloon-pci",
+ .property = "event_idx",
+ .value = "off",
+ },{
+ .driver = "AC97",
+ .property = "use_broken_id",
+ .value = stringify(1),
+ },
+ { /* end of list */ }
+ }
+};
+
+static QEMUMachine pc_machine_v0_10 = {
+ .name = "pc-0.10",
+ .desc = "Standard PC, qemu 0.10",
+ .init = pc_init_pci_no_kvmclock,
+ .max_cpus = 255,
+ .compat_props = (GlobalProperty[]) {
+ {
+ .driver = "virtio-blk-pci",
+ .property = "class",
+ .value = stringify(PCI_CLASS_STORAGE_OTHER),
+ },{
+ .driver = "virtio-serial-pci",
+ .property = "class",
+ .value = stringify(PCI_CLASS_DISPLAY_OTHER),
+ },{
+ .driver = "virtio-serial-pci",
+ .property = "max_ports",
+ .value = stringify(1),
+ },{
+ .driver = "virtio-serial-pci",
+ .property = "vectors",
+ .value = stringify(0),
+ },{
+ .driver = "virtio-net-pci",
+ .property = "vectors",
+ .value = stringify(0),
+ },{
+ .driver = "virtio-blk-pci",
+ .property = "vectors",
+ .value = stringify(0),
+ },{
+ .driver = "ide-drive",
+ .property = "ver",
+ .value = "0.10",
+ },{
+ .driver = "scsi-disk",
+ .property = "ver",
+ .value = "0.10",
+ },{
+ .driver = "PCI",
+ .property = "rombar",
+ .value = stringify(0),
+ },{
+ .driver = "PCI",
+ .property = "command_serr_enable",
+ .value = "off",
+ },{
+ .driver = "virtio-blk-pci",
+ .property = "event_idx",
+ .value = "off",
+ },{
+ .driver = "virtio-serial-pci",
+ .property = "event_idx",
+ .value = "off",
+ },{
+ .driver = "virtio-net-pci",
+ .property = "event_idx",
+ .value = "off",
+ },{
+ .driver = "virtio-balloon-pci",
+ .property = "event_idx",
+ .value = "off",
+ },{
+ .driver = "AC97",
+ .property = "use_broken_id",
+ .value = stringify(1),
+ },
+ { /* end of list */ }
+ },
+};
+
+static QEMUMachine isapc_machine = {
+ .name = "isapc",
+ .desc = "ISA-only PC",
+ .init = pc_init_isa,
+ .max_cpus = 1,
+};
+
+#ifdef CONFIG_XEN
+static QEMUMachine xenfv_machine = {
+ .name = "xenfv",
+ .desc = "Xen Fully-virtualized PC",
+ .init = pc_xen_hvm_init,
+ .max_cpus = HVM_MAX_VCPUS,
+ .default_machine_opts = "accel=xen",
+};
+#endif
+
+static void pc_machine_init(void)
+{
+ qemu_register_machine(&pc_machine_v1_0);
+ qemu_register_machine(&pc_machine_v0_15);
+ qemu_register_machine(&pc_machine_v0_14);
+ qemu_register_machine(&pc_machine_v0_13);
+ qemu_register_machine(&pc_machine_v0_12);
+ qemu_register_machine(&pc_machine_v0_11);
+ qemu_register_machine(&pc_machine_v0_10);
+ qemu_register_machine(&isapc_machine);
+#ifdef CONFIG_XEN
+ qemu_register_machine(&xenfv_machine);
+#endif
+}
+
+machine_init(pc_machine_init);
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
deleted file mode 100644
index c06f1b5..0000000
--- a/hw/pc_piix.c
+++ /dev/null
@@ -1,686 +0,0 @@
-/*
- * QEMU PC System Emulator
- *
- * Copyright (c) 2003-2004 Fabrice Bellard
- *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to deal
- * in the Software without restriction, including without limitation the rights
- * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
- * copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
- * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
- * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
- * THE SOFTWARE.
- */
-
-#include <glib.h>
-
-#include "hw.h"
-#include "pc.h"
-#include "apic.h"
-#include "pci.h"
-#include "usb-uhci.h"
-#include "usb-ohci.h"
-#include "net.h"
-#include "boards.h"
-#include "ide.h"
-#include "kvm.h"
-#include "kvm/clock.h"
-#include "sysemu.h"
-#include "sysbus.h"
-#include "arch_init.h"
-#include "blockdev.h"
-#include "smbus.h"
-#include "xen.h"
-#include "memory.h"
-#include "exec-memory.h"
-#ifdef CONFIG_XEN
-# include <xen/hvm/hvm_info_table.h>
-#endif
-
-#define MAX_IDE_BUS 2
-
-static const int ide_iobase[MAX_IDE_BUS] = { 0x1f0, 0x170 };
-static const int ide_iobase2[MAX_IDE_BUS] = { 0x3f6, 0x376 };
-static const int ide_irq[MAX_IDE_BUS] = { 14, 15 };
-
-static void kvm_piix3_setup_irq_routing(bool pci_enabled)
-{
-#ifdef CONFIG_KVM
- KVMState *s = kvm_state;
- int ret, i;
-
- if (kvm_check_extension(s, KVM_CAP_IRQ_ROUTING)) {
- for (i = 0; i < 8; ++i) {
- if (i == 2) {
- continue;
- }
- kvm_irqchip_add_route(s, i, KVM_IRQCHIP_PIC_MASTER, i);
- }
- for (i = 8; i < 16; ++i) {
- kvm_irqchip_add_route(s, i, KVM_IRQCHIP_PIC_SLAVE, i - 8);
- }
- if (pci_enabled) {
- for (i = 0; i < 24; ++i) {
- if (i == 0) {
- kvm_irqchip_add_route(s, i, KVM_IRQCHIP_IOAPIC, 2);
- } else if (i != 2) {
- kvm_irqchip_add_route(s, i, KVM_IRQCHIP_IOAPIC, i);
- }
- }
- }
- ret = kvm_irqchip_commit_routes(s);
- if (ret < 0) {
- hw_error("KVM IRQ routing setup failed");
- }
- }
-#endif /* CONFIG_KVM */
-}
-
-static void kvm_piix3_gsi_handler(void *opaque, int n, int level)
-{
- GSIState *s = opaque;
-
- if (n < ISA_NUM_IRQS) {
- /* Kernel will forward to both PIC and IOAPIC */
- qemu_set_irq(s->i8259_irq[n], level);
- } else {
- qemu_set_irq(s->ioapic_irq[n], level);
- }
-}
-
-static void ioapic_init(GSIState *gsi_state)
-{
- DeviceState *dev;
- SysBusDevice *d;
- unsigned int i;
-
- if (kvm_enabled() && kvm_irqchip_in_kernel()) {
- dev = qdev_create(NULL, "kvm-ioapic");
- } else {
- dev = qdev_create(NULL, "ioapic");
- }
- qdev_init_nofail(dev);
- d = sysbus_from_qdev(dev);
- sysbus_mmio_map(d, 0, 0xfec00000);
-
- for (i = 0; i < IOAPIC_NUM_PINS; i++) {
- gsi_state->ioapic_irq[i] = qdev_get_gpio_in(dev, i);
- }
-}
-
-/* PC hardware initialisation */
-static void pc_init1(MemoryRegion *system_memory,
- MemoryRegion *system_io,
- ram_addr_t ram_size,
- const char *boot_device,
- const char *kernel_filename,
- const char *kernel_cmdline,
- const char *initrd_filename,
- const char *cpu_model,
- int pci_enabled,
- int kvmclock_enabled)
-{
- int i;
- ram_addr_t below_4g_mem_size, above_4g_mem_size;
- PCIBus *pci_bus;
- ISABus *isa_bus;
- PCII440FXState *i440fx_state;
- int piix3_devfn = -1;
- qemu_irq *cpu_irq;
- qemu_irq *gsi;
- qemu_irq *i8259;
- qemu_irq *cmos_s3;
- qemu_irq *smi_irq;
- GSIState *gsi_state;
- DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
- BusState *idebus[MAX_IDE_BUS];
- ISADevice *rtc_state;
- ISADevice *floppy;
- MemoryRegion *ram_memory;
- MemoryRegion *pci_memory;
- MemoryRegion *rom_memory;
- DeviceState *dev;
-
- pc_cpus_init(cpu_model);
-
- if (kvmclock_enabled) {
- kvmclock_create();
- }
-
- if (ram_size >= 0xe0000000 ) {
- above_4g_mem_size = ram_size - 0xe0000000;
- below_4g_mem_size = 0xe0000000;
- } else {
- above_4g_mem_size = 0;
- below_4g_mem_size = ram_size;
- }
-
- if (pci_enabled) {
- pci_memory = g_new(MemoryRegion, 1);
- memory_region_init(pci_memory, "pci", INT64_MAX);
- rom_memory = pci_memory;
- } else {
- pci_memory = NULL;
- rom_memory = system_memory;
- }
-
- /* allocate ram and load rom/bios */
- if (!xen_enabled()) {
- pc_memory_init(system_memory,
- kernel_filename, kernel_cmdline, initrd_filename,
- below_4g_mem_size, above_4g_mem_size,
- pci_enabled ? rom_memory : system_memory, &ram_memory);
- }
-
- gsi_state = g_malloc0(sizeof(*gsi_state));
- if (kvm_enabled() && kvm_irqchip_in_kernel()) {
- kvm_piix3_setup_irq_routing(pci_enabled);
- gsi = qemu_allocate_irqs(kvm_piix3_gsi_handler, gsi_state,
- GSI_NUM_PINS);
- } else {
- gsi = qemu_allocate_irqs(gsi_handler, gsi_state, GSI_NUM_PINS);
- }
-
- if (pci_enabled) {
- pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,
- system_memory, system_io, ram_size,
- below_4g_mem_size,
- 0x100000000ULL - below_4g_mem_size,
- 0x100000000ULL + above_4g_mem_size,
- (sizeof(target_phys_addr_t) == 4
- ? 0
- : ((uint64_t)1 << 62)),
- pci_memory, ram_memory);
- } else {
- pci_bus = NULL;
- i440fx_state = NULL;
- isa_bus = isa_bus_new(NULL, system_io);
- no_hpet = 1;
- }
- isa_bus_irqs(isa_bus, gsi);
-
- if (kvm_enabled() && kvm_irqchip_in_kernel()) {
- i8259 = kvm_i8259_init(isa_bus);
- } else if (xen_enabled()) {
- i8259 = xen_interrupt_controller_init();
- } else {
- cpu_irq = pc_allocate_cpu_irq();
- i8259 = i8259_init(isa_bus, cpu_irq[0]);
- }
-
- for (i = 0; i < ISA_NUM_IRQS; i++) {
- gsi_state->i8259_irq[i] = i8259[i];
- }
- if (pci_enabled) {
- ioapic_init(gsi_state);
- }
-
- pc_register_ferr_irq(gsi[13]);
-
- dev = pc_vga_init(isa_bus, pci_enabled ? pci_bus : NULL);
- if (dev) {
- object_property_add_child(object_get_root(), "vga", OBJECT(dev), NULL);
- }
-
- if (xen_enabled()) {
- pci_create_simple(pci_bus, -1, "xen-platform");
- }
-
- /* init basic PC hardware */
- pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, xen_enabled());
-
- for(i = 0; i < nb_nics; i++) {
- NICInfo *nd = &nd_table[i];
-
- if (!pci_enabled || (nd->model && strcmp(nd->model, "ne2k_isa") == 0))
- pc_init_ne2k_isa(isa_bus, nd);
- else
- pci_nic_init_nofail(nd, "e1000", NULL);
- }
-
- ide_drive_get(hd, MAX_IDE_BUS);
- if (pci_enabled) {
- PCIDevice *dev;
- if (xen_enabled()) {
- dev = pci_piix3_xen_ide_init(pci_bus, hd, piix3_devfn + 1);
- } else {
- dev = pci_piix3_ide_init(pci_bus, hd, piix3_devfn + 1);
- }
- idebus[0] = qdev_get_child_bus(&dev->qdev, "ide.0");
- idebus[1] = qdev_get_child_bus(&dev->qdev, "ide.1");
-
- /* FIXME there's some major spaghetti here. Somehow we create the
- * devices on the PIIX before we actually create it. We create the
- * PIIX3 deep in the recess of the i440fx creation too and then lose
- * the DeviceState.
- *
- * For now, let's "fix" this by making judicious use of paths. This
- * is not generally the right way to do this.
- */
- object_property_add_child(object_resolve_path("/i440fx/piix3", NULL),
- "rtc", (Object *)rtc_state, NULL);
- } else {
- for(i = 0; i < MAX_IDE_BUS; i++) {
- ISADevice *dev;
- dev = isa_ide_init(isa_bus, ide_iobase[i], ide_iobase2[i],
- ide_irq[i],
- hd[MAX_IDE_DEVS * i], hd[MAX_IDE_DEVS * i + 1]);
- idebus[i] = qdev_get_child_bus(&dev->qdev, "ide.0");
- }
- }
-
- audio_init(isa_bus, pci_enabled ? pci_bus : NULL);
-
- pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device,
- floppy, idebus[0], idebus[1], rtc_state);
-
- if (pci_enabled && usb_enabled) {
- usb_uhci_piix3_init(pci_bus, piix3_devfn + 2);
- }
-
- if (pci_enabled && acpi_enabled) {
- i2c_bus *smbus;
-
- if (!xen_enabled()) {
- cmos_s3 = qemu_allocate_irqs(pc_cmos_set_s3_resume, rtc_state, 1);
- } else {
- cmos_s3 = qemu_allocate_irqs(xen_cmos_set_s3_resume, rtc_state, 1);
- }
- smi_irq = qemu_allocate_irqs(pc_acpi_smi_interrupt, first_cpu, 1);
- /* TODO: Populate SPD eeprom data. */
- smbus = piix4_pm_init(pci_bus, piix3_devfn + 3, 0xb100,
- gsi[9], *cmos_s3, *smi_irq,
- kvm_enabled());
- smbus_eeprom_init(smbus, 8, NULL, 0);
- }
-
- if (pci_enabled) {
- pc_pci_device_init(pci_bus);
- }
-}
-
-static void pc_init_pci(ram_addr_t ram_size,
- const char *boot_device,
- const char *kernel_filename,
- const char *kernel_cmdline,
- const char *initrd_filename,
- const char *cpu_model)
-{
- pc_init1(get_system_memory(),
- get_system_io(),
- ram_size, boot_device,
- kernel_filename, kernel_cmdline,
- initrd_filename, cpu_model, 1, 1);
-}
-
-static void pc_init_pci_no_kvmclock(ram_addr_t ram_size,
- const char *boot_device,
- const char *kernel_filename,
- const char *kernel_cmdline,
- const char *initrd_filename,
- const char *cpu_model)
-{
- pc_init1(get_system_memory(),
- get_system_io(),
- ram_size, boot_device,
- kernel_filename, kernel_cmdline,
- initrd_filename, cpu_model, 1, 0);
-}
-
-static void pc_init_isa(ram_addr_t ram_size,
- const char *boot_device,
- const char *kernel_filename,
- const char *kernel_cmdline,
- const char *initrd_filename,
- const char *cpu_model)
-{
- if (cpu_model == NULL)
- cpu_model = "486";
- pc_init1(get_system_memory(),
- get_system_io(),
- ram_size, boot_device,
- kernel_filename, kernel_cmdline,
- initrd_filename, cpu_model, 0, 1);
-}
-
-#ifdef CONFIG_XEN
-static void pc_xen_hvm_init(ram_addr_t ram_size,
- const char *boot_device,
- const char *kernel_filename,
- const char *kernel_cmdline,
- const char *initrd_filename,
- const char *cpu_model)
-{
- if (xen_hvm_init() != 0) {
- hw_error("xen hardware virtual machine initialisation failed");
- }
- pc_init_pci_no_kvmclock(ram_size, boot_device,
- kernel_filename, kernel_cmdline,
- initrd_filename, cpu_model);
- xen_vcpu_init();
-}
-#endif
-
-static QEMUMachine pc_machine_v1_0 = {
- .name = "pc-1.0",
- .alias = "pc",
- .desc = "Standard PC",
- .init = pc_init_pci,
- .max_cpus = 255,
- .is_default = 1,
-};
-
-static QEMUMachine pc_machine_v0_15 = {
- .name = "pc-0.15",
- .desc = "Standard PC",
- .init = pc_init_pci,
- .max_cpus = 255,
- .is_default = 1,
-};
-
-static QEMUMachine pc_machine_v0_14 = {
- .name = "pc-0.14",
- .desc = "Standard PC",
- .init = pc_init_pci,
- .max_cpus = 255,
- .compat_props = (GlobalProperty[]) {
- {
- .driver = "qxl",
- .property = "revision",
- .value = stringify(2),
- },{
- .driver = "qxl-vga",
- .property = "revision",
- .value = stringify(2),
- },{
- .driver = "virtio-blk-pci",
- .property = "event_idx",
- .value = "off",
- },{
- .driver = "virtio-serial-pci",
- .property = "event_idx",
- .value = "off",
- },{
- .driver = "virtio-net-pci",
- .property = "event_idx",
- .value = "off",
- },{
- .driver = "virtio-balloon-pci",
- .property = "event_idx",
- .value = "off",
- },
- { /* end of list */ }
- },
-};
-
-static QEMUMachine pc_machine_v0_13 = {
- .name = "pc-0.13",
- .desc = "Standard PC",
- .init = pc_init_pci_no_kvmclock,
- .max_cpus = 255,
- .compat_props = (GlobalProperty[]) {
- {
- .driver = "virtio-9p-pci",
- .property = "vectors",
- .value = stringify(0),
- },{
- .driver = "VGA",
- .property = "rombar",
- .value = stringify(0),
- },{
- .driver = "vmware-svga",
- .property = "rombar",
- .value = stringify(0),
- },{
- .driver = "PCI",
- .property = "command_serr_enable",
- .value = "off",
- },{
- .driver = "virtio-blk-pci",
- .property = "event_idx",
- .value = "off",
- },{
- .driver = "virtio-serial-pci",
- .property = "event_idx",
- .value = "off",
- },{
- .driver = "virtio-net-pci",
- .property = "event_idx",
- .value = "off",
- },{
- .driver = "virtio-balloon-pci",
- .property = "event_idx",
- .value = "off",
- },{
- .driver = "AC97",
- .property = "use_broken_id",
- .value = stringify(1),
- },
- { /* end of list */ }
- },
-};
-
-static QEMUMachine pc_machine_v0_12 = {
- .name = "pc-0.12",
- .desc = "Standard PC",
- .init = pc_init_pci_no_kvmclock,
- .max_cpus = 255,
- .compat_props = (GlobalProperty[]) {
- {
- .driver = "virtio-serial-pci",
- .property = "max_ports",
- .value = stringify(1),
- },{
- .driver = "virtio-serial-pci",
- .property = "vectors",
- .value = stringify(0),
- },{
- .driver = "VGA",
- .property = "rombar",
- .value = stringify(0),
- },{
- .driver = "vmware-svga",
- .property = "rombar",
- .value = stringify(0),
- },{
- .driver = "PCI",
- .property = "command_serr_enable",
- .value = "off",
- },{
- .driver = "virtio-blk-pci",
- .property = "event_idx",
- .value = "off",
- },{
- .driver = "virtio-serial-pci",
- .property = "event_idx",
- .value = "off",
- },{
- .driver = "virtio-net-pci",
- .property = "event_idx",
- .value = "off",
- },{
- .driver = "virtio-balloon-pci",
- .property = "event_idx",
- .value = "off",
- },{
- .driver = "AC97",
- .property = "use_broken_id",
- .value = stringify(1),
- },
- { /* end of list */ }
- }
-};
-
-static QEMUMachine pc_machine_v0_11 = {
- .name = "pc-0.11",
- .desc = "Standard PC, qemu 0.11",
- .init = pc_init_pci_no_kvmclock,
- .max_cpus = 255,
- .compat_props = (GlobalProperty[]) {
- {
- .driver = "virtio-blk-pci",
- .property = "vectors",
- .value = stringify(0),
- },{
- .driver = "virtio-serial-pci",
- .property = "max_ports",
- .value = stringify(1),
- },{
- .driver = "virtio-serial-pci",
- .property = "vectors",
- .value = stringify(0),
- },{
- .driver = "ide-drive",
- .property = "ver",
- .value = "0.11",
- },{
- .driver = "scsi-disk",
- .property = "ver",
- .value = "0.11",
- },{
- .driver = "PCI",
- .property = "rombar",
- .value = stringify(0),
- },{
- .driver = "PCI",
- .property = "command_serr_enable",
- .value = "off",
- },{
- .driver = "virtio-blk-pci",
- .property = "event_idx",
- .value = "off",
- },{
- .driver = "virtio-serial-pci",
- .property = "event_idx",
- .value = "off",
- },{
- .driver = "virtio-net-pci",
- .property = "event_idx",
- .value = "off",
- },{
- .driver = "virtio-balloon-pci",
- .property = "event_idx",
- .value = "off",
- },{
- .driver = "AC97",
- .property = "use_broken_id",
- .value = stringify(1),
- },
- { /* end of list */ }
- }
-};
-
-static QEMUMachine pc_machine_v0_10 = {
- .name = "pc-0.10",
- .desc = "Standard PC, qemu 0.10",
- .init = pc_init_pci_no_kvmclock,
- .max_cpus = 255,
- .compat_props = (GlobalProperty[]) {
- {
- .driver = "virtio-blk-pci",
- .property = "class",
- .value = stringify(PCI_CLASS_STORAGE_OTHER),
- },{
- .driver = "virtio-serial-pci",
- .property = "class",
- .value = stringify(PCI_CLASS_DISPLAY_OTHER),
- },{
- .driver = "virtio-serial-pci",
- .property = "max_ports",
- .value = stringify(1),
- },{
- .driver = "virtio-serial-pci",
- .property = "vectors",
- .value = stringify(0),
- },{
- .driver = "virtio-net-pci",
- .property = "vectors",
- .value = stringify(0),
- },{
- .driver = "virtio-blk-pci",
- .property = "vectors",
- .value = stringify(0),
- },{
- .driver = "ide-drive",
- .property = "ver",
- .value = "0.10",
- },{
- .driver = "scsi-disk",
- .property = "ver",
- .value = "0.10",
- },{
- .driver = "PCI",
- .property = "rombar",
- .value = stringify(0),
- },{
- .driver = "PCI",
- .property = "command_serr_enable",
- .value = "off",
- },{
- .driver = "virtio-blk-pci",
- .property = "event_idx",
- .value = "off",
- },{
- .driver = "virtio-serial-pci",
- .property = "event_idx",
- .value = "off",
- },{
- .driver = "virtio-net-pci",
- .property = "event_idx",
- .value = "off",
- },{
- .driver = "virtio-balloon-pci",
- .property = "event_idx",
- .value = "off",
- },{
- .driver = "AC97",
- .property = "use_broken_id",
- .value = stringify(1),
- },
- { /* end of list */ }
- },
-};
-
-static QEMUMachine isapc_machine = {
- .name = "isapc",
- .desc = "ISA-only PC",
- .init = pc_init_isa,
- .max_cpus = 1,
-};
-
-#ifdef CONFIG_XEN
-static QEMUMachine xenfv_machine = {
- .name = "xenfv",
- .desc = "Xen Fully-virtualized PC",
- .init = pc_xen_hvm_init,
- .max_cpus = HVM_MAX_VCPUS,
- .default_machine_opts = "accel=xen",
-};
-#endif
-
-static void pc_machine_init(void)
-{
- qemu_register_machine(&pc_machine_v1_0);
- qemu_register_machine(&pc_machine_v0_15);
- qemu_register_machine(&pc_machine_v0_14);
- qemu_register_machine(&pc_machine_v0_13);
- qemu_register_machine(&pc_machine_v0_12);
- qemu_register_machine(&pc_machine_v0_11);
- qemu_register_machine(&pc_machine_v0_10);
- qemu_register_machine(&isapc_machine);
-#ifdef CONFIG_XEN
- qemu_register_machine(&xenfv_machine);
-#endif
-}
-
-machine_init(pc_machine_init);
--
1.7.4.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [PATCH 01/15] pc: merge pc_piix.c into pc.c
2012-01-26 19:00 ` [Qemu-devel] [PATCH 01/15] pc: merge pc_piix.c into pc.c Anthony Liguori
@ 2012-01-26 19:40 ` Anthony Liguori
2012-01-27 8:50 ` Jan Kiszka
1 sibling, 0 replies; 55+ messages in thread
From: Anthony Liguori @ 2012-01-26 19:40 UTC (permalink / raw)
To: Anthony Liguori
Cc: Peter Maydell, Jan Kiszka, qemu-devel, Markus Armbruster,
Avi Kivity, Paolo Bonzini
On 01/26/2012 01:00 PM, Anthony Liguori wrote:
> A long time ago, there was a grand plan to merge q35 chipset support. The start
> of that series was a refactoring of pc.c which split a bunch of the "common"
> functionality into a separate file that could be shared by the two.
>
> But q35 never got merged and the refactoring, in retrospect, just made things
> worse. Making things proper objects and using composition is the right way
> to share common devices.
>
> By pulling these files back together, we can start to fix some of this mess.
>
> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
BTW, the whole series is also available at:
https://github.com/aliguori/qemu/tree/qom-rebase.10
If you just want to skip ahead to the good stuff, look at:
https://github.com/aliguori/qemu/blob/qom-rebase.10/hw/piix_pci.c#L384
Compared to:
http://git.qemu.org/?p=qemu.git;a=blob;f=hw/piix_pci.c;h=3652522e58f0bbed35d687013af9978bdb4901f7;hb=HEAD#l252
Regards,
Anthony Liguori
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [PATCH 01/15] pc: merge pc_piix.c into pc.c
2012-01-26 19:00 ` [Qemu-devel] [PATCH 01/15] pc: merge pc_piix.c into pc.c Anthony Liguori
2012-01-26 19:40 ` Anthony Liguori
@ 2012-01-27 8:50 ` Jan Kiszka
2012-01-27 13:07 ` Anthony Liguori
1 sibling, 1 reply; 55+ messages in thread
From: Jan Kiszka @ 2012-01-27 8:50 UTC (permalink / raw)
To: Anthony Liguori
Cc: Peter Maydell, Avi Kivity, Paolo Bonzini, qemu-devel,
Markus Armbruster
On 2012-01-26 20:00, Anthony Liguori wrote:
> A long time ago, there was a grand plan to merge q35 chipset support. The start
> of that series was a refactoring of pc.c which split a bunch of the "common"
> functionality into a separate file that could be shared by the two.
>
> But q35 never got merged and the refactoring, in retrospect, just made things
> worse. Making things proper objects and using composition is the right way
> to share common devices.
>
> By pulling these files back together, we can start to fix some of this mess.
There are surely things to clean up and improve, but a clear NACK for
the general direction.
It's undoubted that we need a more modern chipset than this ancient
PIIX3, rather sooner than later. And it is clear that there is a good
amount of generic functions in pc.c for building a PC, even a fairly
modern one. So we need a common lib for PC chipsets and would only
revert what you start here.
Please name what you do not like, and then we can fix that concretely.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [PATCH 01/15] pc: merge pc_piix.c into pc.c
2012-01-27 8:50 ` Jan Kiszka
@ 2012-01-27 13:07 ` Anthony Liguori
2012-01-27 13:32 ` Jan Kiszka
2012-01-27 14:03 ` Andreas Färber
0 siblings, 2 replies; 55+ messages in thread
From: Anthony Liguori @ 2012-01-27 13:07 UTC (permalink / raw)
To: Jan Kiszka
Cc: Peter Maydell, Anthony Liguori, qemu-devel, Markus Armbruster,
Avi Kivity, Paolo Bonzini
On 01/27/2012 02:50 AM, Jan Kiszka wrote:
> On 2012-01-26 20:00, Anthony Liguori wrote:
>> A long time ago, there was a grand plan to merge q35 chipset support. The start
>> of that series was a refactoring of pc.c which split a bunch of the "common"
>> functionality into a separate file that could be shared by the two.
>>
>> But q35 never got merged and the refactoring, in retrospect, just made things
>> worse. Making things proper objects and using composition is the right way
>> to share common devices.
>>
>> By pulling these files back together, we can start to fix some of this mess.
>
> There are surely things to clean up and improve, but a clear NACK for
> the general direction.
Hi Jan,
I think you're missing the bigger picture here. Once this refactoring finishes,
here's what we'll be left with:
1) pc_init creates an I440FX, any bus devices (ISA serial port, PCI vga and
nics, etc.), sets properties appropriately, and realizes the devices.
2) I440FX is-a PCIHost, has-a I440FX-PMC, has-a PIIX3
3) PIIX3 has-a RTC, has-a I8042, has-a DMAController, etc.
Memory creation is done by the I440FX-PMC.
Now it's true that a newer chipset is going to be similar. It will likely have
a SuperI/O chip that looks similar to PIIX3. The right way to share code would
be to move most of the PIIX3 functionality to a base class (PCSuperIO) that
PIIX3 inherits from.
This is probably how to support ISAPC properly too. Once I sort out interrupts,
I'll attempt to tackle that. My guess is that a SuperIO chip could be an
ISADevice and that we could simply make the PIIX3 has-a SuperIO. Then the ISAPC
would have a trivial ISA chipset that has-a SuperIO.
This is fairly trivial to do once we have the right structure to the code.
But the current code has the wrong structure which is why there's so much
pointer chasing and passing.
>
> It's undoubted that we need a more modern chipset than this ancient
> PIIX3, rather sooner than later. And it is clear that there is a good
> amount of generic functions in pc.c for building a PC, even a fairly
> modern one. So we need a common lib for PC chipsets and would only
> revert what you start here.
Sorry, but I don't view this as a useful requirement. Today we support two
types of PCs: an i440fx based system and an ISA-only system. We should
concentrate on modeling those two systems in the most natural way sharing as
much code as possible.
When we add a third system, we'll figure out the best way to create additional
abstractions. But inhibiting the proper modeling of what we support today in
order to support some future thing is a bit backwards to me.
Regards,
Anthony Liguori
> Please name what you do not like, and then we can fix that concretely.
>
> Jan
>
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [PATCH 01/15] pc: merge pc_piix.c into pc.c
2012-01-27 13:07 ` Anthony Liguori
@ 2012-01-27 13:32 ` Jan Kiszka
2012-01-27 14:06 ` Anthony Liguori
2012-01-27 14:03 ` Andreas Färber
1 sibling, 1 reply; 55+ messages in thread
From: Jan Kiszka @ 2012-01-27 13:32 UTC (permalink / raw)
To: Anthony Liguori
Cc: Peter Maydell, Anthony Liguori, qemu-devel@nongnu.org,
Markus Armbruster, Avi Kivity, Paolo Bonzini
On 2012-01-27 14:07, Anthony Liguori wrote:
> On 01/27/2012 02:50 AM, Jan Kiszka wrote:
>> On 2012-01-26 20:00, Anthony Liguori wrote:
>>> A long time ago, there was a grand plan to merge q35 chipset support. The start
>>> of that series was a refactoring of pc.c which split a bunch of the "common"
>>> functionality into a separate file that could be shared by the two.
>>>
>>> But q35 never got merged and the refactoring, in retrospect, just made things
>>> worse. Making things proper objects and using composition is the right way
>>> to share common devices.
>>>
>>> By pulling these files back together, we can start to fix some of this mess.
>>
>> There are surely things to clean up and improve, but a clear NACK for
>> the general direction.
>
> Hi Jan,
>
> I think you're missing the bigger picture here. Once this refactoring finishes,
> here's what we'll be left with:
>
> 1) pc_init creates an I440FX, any bus devices (ISA serial port, PCI vga and
> nics, etc.), sets properties appropriately, and realizes the devices.
>
> 2) I440FX is-a PCIHost, has-a I440FX-PMC, has-a PIIX3
>
> 3) PIIX3 has-a RTC, has-a I8042, has-a DMAController, etc.
>
> Memory creation is done by the I440FX-PMC.
>
> Now it's true that a newer chipset is going to be similar. It will likely have
> a SuperI/O chip that looks similar to PIIX3. The right way to share code would
> be to move most of the PIIX3 functionality to a base class (PCSuperIO) that
> PIIX3 inherits from.
>
> This is probably how to support ISAPC properly too.
The ISAPC is differently composed. The board creates all those
individual chips that are otherwise part of the SuperIO block of the
chipset. And IRQ wiring is different. So, no, I don't think it is the
right model. I would rather think of a pc_isa.c that does the proper
composing.
> Once I sort out interrupts,
> I'll attempt to tackle that. My guess is that a SuperIO chip could be an
> ISADevice and that we could simply make the PIIX3 has-a SuperIO. Then the ISAPC
> would have a trivial ISA chipset that has-a SuperIO.
>
> This is fairly trivial to do once we have the right structure to the code.
>
> But the current code has the wrong structure which is why there's so much
> pointer chasing and passing.
Just look at your code and count the generic, PIIX3-independent
functions. Keep them in pc.c, move the rest to pc_piix.c. You could try
to model the ISA accordingly. I think some pc_isa.c would help to
establish a good split-up already now, in the absence of a third chipset.
>
>>
>> It's undoubted that we need a more modern chipset than this ancient
>> PIIX3, rather sooner than later. And it is clear that there is a good
>> amount of generic functions in pc.c for building a PC, even a fairly
>> modern one. So we need a common lib for PC chipsets and would only
>> revert what you start here.
>
> Sorry, but I don't view this as a useful requirement. Today we support two
> types of PCs: an i440fx based system and an ISA-only system. We should
> concentrate on modeling those two systems in the most natural way sharing as
> much code as possible.
A modern chipset is the only sane way to add things like PCIe,
hotplugging, power management, etc., and to enable standard PC
components like AHCI or EHCI/xHCI by default.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [PATCH 01/15] pc: merge pc_piix.c into pc.c
2012-01-27 13:32 ` Jan Kiszka
@ 2012-01-27 14:06 ` Anthony Liguori
2012-01-27 14:15 ` Jan Kiszka
0 siblings, 1 reply; 55+ messages in thread
From: Anthony Liguori @ 2012-01-27 14:06 UTC (permalink / raw)
To: Jan Kiszka
Cc: qemu-devel@nongnu.org, Peter Maydell, Avi Kivity,
Markus Armbruster, Paolo Bonzini
On 01/27/2012 07:32 AM, Jan Kiszka wrote:
> On 2012-01-27 14:07, Anthony Liguori wrote:
>> On 01/27/2012 02:50 AM, Jan Kiszka wrote:
>>> On 2012-01-26 20:00, Anthony Liguori wrote:
>>>> A long time ago, there was a grand plan to merge q35 chipset support. The start
>>>> of that series was a refactoring of pc.c which split a bunch of the "common"
>>>> functionality into a separate file that could be shared by the two.
>>>>
>>>> But q35 never got merged and the refactoring, in retrospect, just made things
>>>> worse. Making things proper objects and using composition is the right way
>>>> to share common devices.
>>>>
>>>> By pulling these files back together, we can start to fix some of this mess.
>>>
>>> There are surely things to clean up and improve, but a clear NACK for
>>> the general direction.
>>
>> Hi Jan,
>>
>> I think you're missing the bigger picture here. Once this refactoring finishes,
>> here's what we'll be left with:
>>
>> 1) pc_init creates an I440FX, any bus devices (ISA serial port, PCI vga and
>> nics, etc.), sets properties appropriately, and realizes the devices.
>>
>> 2) I440FX is-a PCIHost, has-a I440FX-PMC, has-a PIIX3
>>
>> 3) PIIX3 has-a RTC, has-a I8042, has-a DMAController, etc.
>>
>> Memory creation is done by the I440FX-PMC.
>>
>> Now it's true that a newer chipset is going to be similar. It will likely have
>> a SuperI/O chip that looks similar to PIIX3. The right way to share code would
>> be to move most of the PIIX3 functionality to a base class (PCSuperIO) that
>> PIIX3 inherits from.
>>
>> This is probably how to support ISAPC properly too.
>
> The ISAPC is differently composed. The board creates all those
> individual chips that are otherwise part of the SuperIO block of the
> chipset. And IRQ wiring is different.
A SuperIO device can still have irq properties for each device that determines
which ISA bus irq is used.
The other alternative is to stop making devices ISADevice, and then having
SuperIO expose a bunch of device specific IRQs. Those IRQs can then be routed
in whatever way makes sense.
This is really the Right Approach but it's most likely a compatibility breaker
so I'm trying my best to avoid these types of refactorings right now.
> So, no, I don't think it is the
> right model. I would rather think of a pc_isa.c that does the proper
> composing.
Objects compose other objects. Functions should not be creating devices. So if
you're view of pc_isa.c that there's a pc_basic_init() that takes a bunch of
devicestate pointers, then that's definitely not the direction I'm heading.
Note that if we ever want to get to a board configuration file, we need to have
three explicit steps in device creation:
1) device initialization where devices are allocates and the composition tree is
filled out (which makes sure that every device has an addressable path)
2) device property setting (which requires all devices have an addressable path)
3) device realization where property settings are validated, and then any
initialization that depends on properties is done.
The problem with the current code is that it doesn't split up these phases.
Modeling composition really helps to get this split because it forces you to
think about things in terms of distinct phases.
>> Once I sort out interrupts,
>> I'll attempt to tackle that. My guess is that a SuperIO chip could be an
>> ISADevice and that we could simply make the PIIX3 has-a SuperIO. Then the ISAPC
>> would have a trivial ISA chipset that has-a SuperIO.
>>
>> This is fairly trivial to do once we have the right structure to the code.
>>
>> But the current code has the wrong structure which is why there's so much
>> pointer chasing and passing.
>
> Just look at your code and count the generic, PIIX3-independent
> functions.
I see zero.
"PIIX3-independent" means that some other piece of code would consume it in
exactly the same way. pc.c had a bunch of spaghetti in it doing tricks with if
(pci_enabled) that ended up encoding two very separate paths into one maze of code.
> Keep them in pc.c, move the rest to pc_piix.c. You could try
> to model the ISA accordingly. I think some pc_isa.c would help to
> establish a good split-up already now, in the absence of a third chipset.
I think creating a proper ISA model is a good idea and I'll certainly get there.
Regards,
Anthony Liguori
>>
>>>
>>> It's undoubted that we need a more modern chipset than this ancient
>>> PIIX3, rather sooner than later. And it is clear that there is a good
>>> amount of generic functions in pc.c for building a PC, even a fairly
>>> modern one. So we need a common lib for PC chipsets and would only
>>> revert what you start here.
>>
>> Sorry, but I don't view this as a useful requirement. Today we support two
>> types of PCs: an i440fx based system and an ISA-only system. We should
>> concentrate on modeling those two systems in the most natural way sharing as
>> much code as possible.
>
> A modern chipset is the only sane way to add things like PCIe,
> hotplugging, power management, etc., and to enable standard PC
> components like AHCI or EHCI/xHCI by default.
>
> Jan
>
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [PATCH 01/15] pc: merge pc_piix.c into pc.c
2012-01-27 14:06 ` Anthony Liguori
@ 2012-01-27 14:15 ` Jan Kiszka
2012-01-27 14:23 ` Anthony Liguori
0 siblings, 1 reply; 55+ messages in thread
From: Jan Kiszka @ 2012-01-27 14:15 UTC (permalink / raw)
To: Anthony Liguori
Cc: qemu-devel@nongnu.org, Peter Maydell, Avi Kivity,
Markus Armbruster, Paolo Bonzini
On 2012-01-27 15:06, Anthony Liguori wrote:
> On 01/27/2012 07:32 AM, Jan Kiszka wrote:
>> On 2012-01-27 14:07, Anthony Liguori wrote:
>>> On 01/27/2012 02:50 AM, Jan Kiszka wrote:
>>>> On 2012-01-26 20:00, Anthony Liguori wrote:
>>>>> A long time ago, there was a grand plan to merge q35 chipset support. The start
>>>>> of that series was a refactoring of pc.c which split a bunch of the "common"
>>>>> functionality into a separate file that could be shared by the two.
>>>>>
>>>>> But q35 never got merged and the refactoring, in retrospect, just made things
>>>>> worse. Making things proper objects and using composition is the right way
>>>>> to share common devices.
>>>>>
>>>>> By pulling these files back together, we can start to fix some of this mess.
>>>>
>>>> There are surely things to clean up and improve, but a clear NACK for
>>>> the general direction.
>>>
>>> Hi Jan,
>>>
>>> I think you're missing the bigger picture here. Once this refactoring finishes,
>>> here's what we'll be left with:
>>>
>>> 1) pc_init creates an I440FX, any bus devices (ISA serial port, PCI vga and
>>> nics, etc.), sets properties appropriately, and realizes the devices.
>>>
>>> 2) I440FX is-a PCIHost, has-a I440FX-PMC, has-a PIIX3
>>>
>>> 3) PIIX3 has-a RTC, has-a I8042, has-a DMAController, etc.
>>>
>>> Memory creation is done by the I440FX-PMC.
>>>
>>> Now it's true that a newer chipset is going to be similar. It will likely have
>>> a SuperI/O chip that looks similar to PIIX3. The right way to share code would
>>> be to move most of the PIIX3 functionality to a base class (PCSuperIO) that
>>> PIIX3 inherits from.
>>>
>>> This is probably how to support ISAPC properly too.
>>
>> The ISAPC is differently composed. The board creates all those
>> individual chips that are otherwise part of the SuperIO block of the
>> chipset. And IRQ wiring is different.
>
> A SuperIO device can still have irq properties for each device that determines
> which ISA bus irq is used.
>
> The other alternative is to stop making devices ISADevice, and then having
> SuperIO expose a bunch of device specific IRQs. Those IRQs can then be routed
> in whatever way makes sense.
>
> This is really the Right Approach but it's most likely a compatibility breaker
> so I'm trying my best to avoid these types of refactorings right now.
>
>> So, no, I don't think it is the
>> right model. I would rather think of a pc_isa.c that does the proper
>> composing.
>
> Objects compose other objects. Functions should not be creating devices. So if
> you're view of pc_isa.c that there's a pc_basic_init() that takes a bunch of
> devicestate pointers, then that's definitely not the direction I'm heading.
>
> Note that if we ever want to get to a board configuration file, we need to have
> three explicit steps in device creation:
>
> 1) device initialization where devices are allocates and the composition tree is
> filled out (which makes sure that every device has an addressable path)
>
> 2) device property setting (which requires all devices have an addressable path)
>
> 3) device realization where property settings are validated, and then any
> initialization that depends on properties is done.
>
> The problem with the current code is that it doesn't split up these phases.
> Modeling composition really helps to get this split because it forces you to
> think about things in terms of distinct phases.
>
>>> Once I sort out interrupts,
>>> I'll attempt to tackle that. My guess is that a SuperIO chip could be an
>>> ISADevice and that we could simply make the PIIX3 has-a SuperIO. Then the ISAPC
>>> would have a trivial ISA chipset that has-a SuperIO.
>>>
>>> This is fairly trivial to do once we have the right structure to the code.
>>>
>>> But the current code has the wrong structure which is why there's so much
>>> pointer chasing and passing.
>>
>> Just look at your code and count the generic, PIIX3-independent
>> functions.
>
> I see zero.
Then you should look more carefully:
- GSI handler
- ferr
- tsc
- smm
- pic irq handling
- cmos
- boot devices
- port92
- a20
well, this gets boring.
>
> "PIIX3-independent" means that some other piece of code would consume it in
> exactly the same way. pc.c had a bunch of spaghetti in it doing tricks with if
> (pci_enabled) that ended up encoding two very separate paths into one maze of code.
Right, the still existing pci_enabled mess should be resolved via
separate board instantiations that are based on common building blocks.
That avoids also regression in the ISA PC due to refactorings in the
PIIX3 as happened recently.
>
>> Keep them in pc.c, move the rest to pc_piix.c. You could try
>> to model the ISA accordingly. I think some pc_isa.c would help to
>> establish a good split-up already now, in the absence of a third chipset.
>
> I think creating a proper ISA model is a good idea and I'll certainly get there.
At least here we agree. :)
BTW, a third board we already have today is Xen. Same story.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [PATCH 01/15] pc: merge pc_piix.c into pc.c
2012-01-27 14:15 ` Jan Kiszka
@ 2012-01-27 14:23 ` Anthony Liguori
0 siblings, 0 replies; 55+ messages in thread
From: Anthony Liguori @ 2012-01-27 14:23 UTC (permalink / raw)
To: Jan Kiszka
Cc: Peter Maydell, Anthony Liguori, qemu-devel@nongnu.org,
Markus Armbruster, Avi Kivity, Paolo Bonzini
On 01/27/2012 08:15 AM, Jan Kiszka wrote:
> On 2012-01-27 15:06, Anthony Liguori wrote:
>> On 01/27/2012 07:32 AM, Jan Kiszka wrote:
>>> On 2012-01-27 14:07, Anthony Liguori wrote:
>>>> On 01/27/2012 02:50 AM, Jan Kiszka wrote:
>>>>> On 2012-01-26 20:00, Anthony Liguori wrote:
>>>>>> A long time ago, there was a grand plan to merge q35 chipset support. The start
>>>>>> of that series was a refactoring of pc.c which split a bunch of the "common"
>>>>>> functionality into a separate file that could be shared by the two.
>>>>>>
>>>>>> But q35 never got merged and the refactoring, in retrospect, just made things
>>>>>> worse. Making things proper objects and using composition is the right way
>>>>>> to share common devices.
>>>>>>
>>>>>> By pulling these files back together, we can start to fix some of this mess.
>>>>>
>>>>> There are surely things to clean up and improve, but a clear NACK for
>>>>> the general direction.
>>>>
>>>> Hi Jan,
>>>>
>>>> I think you're missing the bigger picture here. Once this refactoring finishes,
>>>> here's what we'll be left with:
>>>>
>>>> 1) pc_init creates an I440FX, any bus devices (ISA serial port, PCI vga and
>>>> nics, etc.), sets properties appropriately, and realizes the devices.
>>>>
>>>> 2) I440FX is-a PCIHost, has-a I440FX-PMC, has-a PIIX3
>>>>
>>>> 3) PIIX3 has-a RTC, has-a I8042, has-a DMAController, etc.
>>>>
>>>> Memory creation is done by the I440FX-PMC.
>>>>
>>>> Now it's true that a newer chipset is going to be similar. It will likely have
>>>> a SuperI/O chip that looks similar to PIIX3. The right way to share code would
>>>> be to move most of the PIIX3 functionality to a base class (PCSuperIO) that
>>>> PIIX3 inherits from.
>>>>
>>>> This is probably how to support ISAPC properly too.
>>>
>>> The ISAPC is differently composed. The board creates all those
>>> individual chips that are otherwise part of the SuperIO block of the
>>> chipset. And IRQ wiring is different.
>>
>> A SuperIO device can still have irq properties for each device that determines
>> which ISA bus irq is used.
>>
>> The other alternative is to stop making devices ISADevice, and then having
>> SuperIO expose a bunch of device specific IRQs. Those IRQs can then be routed
>> in whatever way makes sense.
>>
>> This is really the Right Approach but it's most likely a compatibility breaker
>> so I'm trying my best to avoid these types of refactorings right now.
>>
>>> So, no, I don't think it is the
>>> right model. I would rather think of a pc_isa.c that does the proper
>>> composing.
>>
>> Objects compose other objects. Functions should not be creating devices. So if
>> you're view of pc_isa.c that there's a pc_basic_init() that takes a bunch of
>> devicestate pointers, then that's definitely not the direction I'm heading.
>>
>> Note that if we ever want to get to a board configuration file, we need to have
>> three explicit steps in device creation:
>>
>> 1) device initialization where devices are allocates and the composition tree is
>> filled out (which makes sure that every device has an addressable path)
>>
>> 2) device property setting (which requires all devices have an addressable path)
>>
>> 3) device realization where property settings are validated, and then any
>> initialization that depends on properties is done.
>>
>> The problem with the current code is that it doesn't split up these phases.
>> Modeling composition really helps to get this split because it forces you to
>> think about things in terms of distinct phases.
>>
>>>> Once I sort out interrupts,
>>>> I'll attempt to tackle that. My guess is that a SuperIO chip could be an
>>>> ISADevice and that we could simply make the PIIX3 has-a SuperIO. Then the ISAPC
>>>> would have a trivial ISA chipset that has-a SuperIO.
>>>>
>>>> This is fairly trivial to do once we have the right structure to the code.
>>>>
>>>> But the current code has the wrong structure which is why there's so much
>>>> pointer chasing and passing.
>>>
>>> Just look at your code and count the generic, PIIX3-independent
>>> functions.
>>
>> I see zero.
>
> Then you should look more carefully:
>
> - GSI handler
> - ferr
> - tsc
> - smm
> - pic irq handling
> - cmos
> - boot devices
> - port92
> - a20
>
> well, this gets boring.
Most of these things fall under the "we're not doing it right today" category IMHO.
But I'm still working on this so let's wait to have this discussion...
>>
>> "PIIX3-independent" means that some other piece of code would consume it in
>> exactly the same way. pc.c had a bunch of spaghetti in it doing tricks with if
>> (pci_enabled) that ended up encoding two very separate paths into one maze of code.
>
> Right, the still existing pci_enabled mess should be resolved via
> separate board instantiations that are based on common building blocks.
> That avoids also regression in the ISA PC due to refactorings in the
> PIIX3 as happened recently.
Yes, we're both on the same page here. I think maybe we just view the paths
there a bit differently.
Anyway, this was an RFC for a reason. I was more interested in demonstrating
what more intensively QOM-ified devices looked like than presenting a coherent
end-to-end proposal for refactoring PC initialization. I plan on doing that,
but I'm not entirely prepared for that yet.
Regards,
Anthony Liguori
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [PATCH 01/15] pc: merge pc_piix.c into pc.c
2012-01-27 13:07 ` Anthony Liguori
2012-01-27 13:32 ` Jan Kiszka
@ 2012-01-27 14:03 ` Andreas Färber
2012-01-27 14:14 ` Anthony Liguori
1 sibling, 1 reply; 55+ messages in thread
From: Andreas Färber @ 2012-01-27 14:03 UTC (permalink / raw)
To: Anthony Liguori
Cc: Peter Maydell, Anthony Liguori, Jan Kiszka, qemu-devel,
Markus Armbruster, Hervé Poussineau, Avi Kivity,
Paolo Bonzini
Am 27.01.2012 14:07, schrieb Anthony Liguori:
> [...] My guess is that a SuperIO
> chip could be an ISADevice and that we could simply make the PIIX3 has-a
> SuperIO. Then the ISAPC would have a trivial ISA chipset that has-a
> SuperIO.
That sounds pretty much like our construction site for PReP...
Would you say that the SuperIO is-a ISADevice and has-a ISADevice or
would you want to remodel all ISADevices associated with a Super I/O
chipset as private devices to mess with their internals without the
whole enable/disable, etc. ugliness we ran into?
I somewhat doubt that we can find a generic "SuperIO" base class btw.
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] 55+ messages in thread
* Re: [Qemu-devel] [PATCH 01/15] pc: merge pc_piix.c into pc.c
2012-01-27 14:03 ` Andreas Färber
@ 2012-01-27 14:14 ` Anthony Liguori
0 siblings, 0 replies; 55+ messages in thread
From: Anthony Liguori @ 2012-01-27 14:14 UTC (permalink / raw)
To: Andreas Färber
Cc: Peter Maydell, Anthony Liguori, Jan Kiszka, qemu-devel,
Markus Armbruster, Hervé Poussineau, Avi Kivity,
Paolo Bonzini
On 01/27/2012 08:03 AM, Andreas Färber wrote:
> Am 27.01.2012 14:07, schrieb Anthony Liguori:
>> [...] My guess is that a SuperIO
>> chip could be an ISADevice and that we could simply make the PIIX3 has-a
>> SuperIO. Then the ISAPC would have a trivial ISA chipset that has-a
>> SuperIO.
>
> That sounds pretty much like our construction site for PReP...
Yup. It basically boils down to:
Root Complex[1] (PCI Host)
- Northbridge (Memory Controller)
- Southbridge (Super I/O chip)
Most platforms will follow this type of composition model with peripheral
devices hanging off a bus in the Southbridge or directly from the Root Complex.
> Would you say that the SuperIO is-a ISADevice
The SuperIO device is-a Root Complex device. With the I440FX, the Root Complex
was PCI bus so the SuperIO chip (the PIIX3) is-a PCIDevice. In older PCIs, the
Root Complex was ISA (sort of).
[1] A better name for this is System Bus but I'm using Root Complex to avoid
confusion with sysbus.
But for the PC, we can make the SuperIO chip be is-a DeviceState and just have
it expose a MemoryRegion and a bunch of IRQs. That would trivialize the
implementation of an PIIX3 such that it has-a PCSuperIO and then just routes
IRQs appropriately.
> and has-a ISADevice or
> would you want to remodel all ISADevices associated with a Super I/O
> chipset as private devices to mess with their internals without the
> whole enable/disable, etc. ugliness we ran into?
>
> I somewhat doubt that we can find a generic "SuperIO" base class btw.
Right, it's unlikely that a PC SuperIO chip would be useful outside of a PC.
But you may find certain classes of platforms all have a common SuperI/O chip
and can model similar things.
Regards,
Anthony Liguori
> Andreas
>
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] [PATCH 02/15] pc: make some functions static
2012-01-26 19:00 [Qemu-devel] [RFC 00/15] Refactor PC machine to take advantage of QOM Anthony Liguori
2012-01-26 19:00 ` [Qemu-devel] [PATCH 01/15] pc: merge pc_piix.c into pc.c Anthony Liguori
@ 2012-01-26 19:00 ` Anthony Liguori
2012-01-26 19:00 ` [Qemu-devel] [PATCH 03/15] piix3: make PIIX3-xen a subclass of PIIX3 Anthony Liguori
` (13 subsequent siblings)
15 siblings, 0 replies; 55+ messages in thread
From: Anthony Liguori @ 2012-01-26 19:00 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Anthony Liguori, Jan Kiszka, Markus Armbruster,
Avi Kivity, Paolo Bonzini
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
hw/pc.c | 24 ++++++++++++------------
hw/pc.h | 26 --------------------------
2 files changed, 12 insertions(+), 38 deletions(-)
diff --git a/hw/pc.c b/hw/pc.c
index 3d40aca..4b11e44 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -101,7 +101,7 @@ static const int ide_irq[MAX_IDE_BUS] = { 14, 15 };
static struct e820_table e820_table;
struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
-void gsi_handler(void *opaque, int n, int level)
+static void gsi_handler(void *opaque, int n, int level)
{
GSIState *s = opaque;
@@ -119,7 +119,7 @@ static void ioport80_write(void *opaque, uint32_t addr, uint32_t data)
/* MSDOS compatibility mode FPU exception support */
static qemu_irq ferr_irq;
-void pc_register_ferr_irq(qemu_irq irq)
+static void pc_register_ferr_irq(qemu_irq irq)
{
ferr_irq = irq;
}
@@ -342,7 +342,7 @@ static void pc_cmos_init_late(void *opaque)
qemu_unregister_reset(pc_cmos_init_late, opaque);
}
-void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
+static void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
const char *boot_device,
ISADevice *floppy, BusState *idebus0, BusState *idebus1,
ISADevice *s)
@@ -870,7 +870,7 @@ static const int ne2000_irq[NE2000_NB_MAX] = { 9, 10, 11, 3, 4, 5 };
static const int parallel_io[MAX_PARALLEL_PORTS] = { 0x378, 0x278, 0x3bc };
static const int parallel_irq[MAX_PARALLEL_PORTS] = { 7, 7, 7 };
-void pc_init_ne2k_isa(ISABus *bus, NICInfo *nd)
+static void pc_init_ne2k_isa(ISABus *bus, NICInfo *nd)
{
static int nb_ne2k = 0;
@@ -938,7 +938,7 @@ void pc_cmos_set_s3_resume(void *opaque, int irq, int level)
}
}
-void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
+static void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
{
CPUState *s = opaque;
@@ -972,7 +972,7 @@ static CPUState *pc_new_cpu(const char *cpu_model)
return env;
}
-void pc_cpus_init(const char *cpu_model)
+static void pc_cpus_init(const char *cpu_model)
{
int i;
@@ -990,7 +990,7 @@ void pc_cpus_init(const char *cpu_model)
}
}
-void pc_memory_init(MemoryRegion *system_memory,
+static void pc_memory_init(MemoryRegion *system_memory,
const char *kernel_filename,
const char *kernel_cmdline,
const char *initrd_filename,
@@ -1093,12 +1093,12 @@ void pc_memory_init(MemoryRegion *system_memory,
}
}
-qemu_irq *pc_allocate_cpu_irq(void)
+static qemu_irq *pc_allocate_cpu_irq(void)
{
return qemu_allocate_irqs(pic_irq_request, NULL, 1);
}
-DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus)
+static DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus)
{
DeviceState *dev = NULL;
@@ -1142,7 +1142,7 @@ static void cpu_request_exit(void *opaque, int irq, int level)
}
}
-void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
+static void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
ISADevice **rtc_state,
ISADevice **floppy,
bool no_vmport)
@@ -1212,7 +1212,7 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
*floppy = fdctrl_init_isa(isa_bus, fd);
}
-void pc_pci_device_init(PCIBus *pci_bus)
+static void pc_pci_device_init(PCIBus *pci_bus)
{
int max_bus;
int bus;
@@ -1316,7 +1316,7 @@ static void pc_init1(MemoryRegion *system_memory,
BusState *idebus[MAX_IDE_BUS];
ISADevice *rtc_state;
ISADevice *floppy;
- MemoryRegion *ram_memory;
+ MemoryRegion *ram_memory = NULL;
MemoryRegion *pci_memory;
MemoryRegion *rom_memory;
DeviceState *dev;
diff --git a/hw/pc.h b/hw/pc.h
index c666ec9..e269012 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -79,8 +79,6 @@ typedef struct GSIState {
qemu_irq ioapic_irq[IOAPIC_NUM_PINS];
} GSIState;
-void gsi_handler(void *opaque, int n, int level);
-
/* i8254.c */
#define PIT_FREQ 1193182
@@ -127,31 +125,7 @@ void i8042_setup_a20_line(ISADevice *dev, qemu_irq *a20_out);
/* pc.c */
extern int fd_bootchk;
-void pc_register_ferr_irq(qemu_irq irq);
void pc_cmos_set_s3_resume(void *opaque, int irq, int level);
-void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
-
-void pc_cpus_init(const char *cpu_model);
-void pc_memory_init(MemoryRegion *system_memory,
- const char *kernel_filename,
- const char *kernel_cmdline,
- const char *initrd_filename,
- ram_addr_t below_4g_mem_size,
- ram_addr_t above_4g_mem_size,
- MemoryRegion *rom_memory,
- MemoryRegion **ram_memory);
-qemu_irq *pc_allocate_cpu_irq(void);
-DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
-void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
- ISADevice **rtc_state,
- ISADevice **floppy,
- bool no_vmport);
-void pc_init_ne2k_isa(ISABus *bus, NICInfo *nd);
-void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
- const char *boot_device,
- ISADevice *floppy, BusState *ide0, BusState *ide1,
- ISADevice *s);
-void pc_pci_device_init(PCIBus *pci_bus);
typedef void (*cpu_set_smm_t)(int smm, void *arg);
void cpu_smm_register(cpu_set_smm_t callback, void *arg);
--
1.7.4.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [PATCH 03/15] piix3: make PIIX3-xen a subclass of PIIX3
2012-01-26 19:00 [Qemu-devel] [RFC 00/15] Refactor PC machine to take advantage of QOM Anthony Liguori
2012-01-26 19:00 ` [Qemu-devel] [PATCH 01/15] pc: merge pc_piix.c into pc.c Anthony Liguori
2012-01-26 19:00 ` [Qemu-devel] [PATCH 02/15] pc: make some functions static Anthony Liguori
@ 2012-01-26 19:00 ` Anthony Liguori
2012-01-26 19:00 ` [Qemu-devel] [PATCH 04/15] piix: prepare for composition Anthony Liguori
` (12 subsequent siblings)
15 siblings, 0 replies; 55+ messages in thread
From: Anthony Liguori @ 2012-01-26 19:00 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Anthony Liguori, Jan Kiszka, Markus Armbruster,
Avi Kivity, Paolo Bonzini
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
hw/piix_pci.c | 11 +----------
1 files changed, 1 insertions(+), 10 deletions(-)
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 1906427..9065632 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -527,23 +527,14 @@ static TypeInfo piix3_info = {
static void piix3_xen_class_init(ObjectClass *klass, void *data)
{
- DeviceClass *dc = DEVICE_CLASS(klass);
PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
- dc->desc = "ISA bridge";
- dc->vmsd = &vmstate_piix3;
- dc->no_user = 1;
- k->no_hotplug = 1;
- k->init = piix3_initfn;
k->config_write = piix3_write_config_xen;
- k->vendor_id = PCI_VENDOR_ID_INTEL;
- k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0; // 82371SB PIIX3 PCI-to-ISA bridge (Step A1)
- k->class_id = PCI_CLASS_BRIDGE_ISA;
};
static TypeInfo piix3_xen_info = {
.name = "PIIX3-xen",
- .parent = TYPE_PCI_DEVICE,
+ .parent = "PIIX3",
.instance_size = sizeof(PIIX3State),
.class_init = piix3_xen_class_init,
};
--
1.7.4.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [PATCH 04/15] piix: prepare for composition
2012-01-26 19:00 [Qemu-devel] [RFC 00/15] Refactor PC machine to take advantage of QOM Anthony Liguori
` (2 preceding siblings ...)
2012-01-26 19:00 ` [Qemu-devel] [PATCH 03/15] piix3: make PIIX3-xen a subclass of PIIX3 Anthony Liguori
@ 2012-01-26 19:00 ` Anthony Liguori
2012-01-26 19:00 ` [Qemu-devel] [PATCH 05/15] piix: create the HPET and RTC through composition Anthony Liguori
` (11 subsequent siblings)
15 siblings, 0 replies; 55+ messages in thread
From: Anthony Liguori @ 2012-01-26 19:00 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Anthony Liguori, Jan Kiszka, Markus Armbruster,
Avi Kivity, Paolo Bonzini
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
hw/piix_pci.c | 26 +++++++++++++++++---------
1 files changed, 17 insertions(+), 9 deletions(-)
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 9065632..ec75725 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -43,6 +43,9 @@ typedef PCIHostState I440FXState;
#define XEN_PIIX_NUM_PIRQS 128ULL
#define PIIX_PIRQC 0x60
+#define TYPE_PIIX3 "PIIX3"
+#define PIIX3(obj) OBJECT_CHECK(PIIX3State, (obj), TYPE_PIIX3)
+
typedef struct PIIX3State {
PCIDevice dev;
@@ -420,9 +423,9 @@ static void piix3_write_config_xen(PCIDevice *dev,
piix3_write_config(dev, address, val, len);
}
-static void piix3_reset(void *opaque)
+static void piix3_reset(DeviceState *dev)
{
- PIIX3State *d = opaque;
+ PIIX3State *d = PIIX3(dev);
uint8_t *pci_conf = d->dev.config;
pci_conf[0x04] = 0x07; // master, memory and I/O
@@ -479,7 +482,7 @@ static void piix3_pre_save(void *opaque)
}
static const VMStateDescription vmstate_piix3 = {
- .name = "PIIX3",
+ .name = TYPE_PIIX3,
.version_id = 3,
.minimum_version_id = 2,
.minimum_version_id_old = 2,
@@ -493,15 +496,18 @@ static const VMStateDescription vmstate_piix3 = {
}
};
-static int piix3_initfn(PCIDevice *dev)
+static int piix3_realize(PCIDevice *dev)
{
PIIX3State *d = DO_UPCAST(PIIX3State, dev, dev);
isa_bus_new(&d->dev.qdev, pci_address_space_io(dev));
- qemu_register_reset(piix3_reset, d);
return 0;
}
+static void piix3_initfn(Object *obj)
+{
+}
+
static void piix3_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
@@ -509,9 +515,10 @@ static void piix3_class_init(ObjectClass *klass, void *data)
dc->desc = "ISA bridge";
dc->vmsd = &vmstate_piix3;
- dc->no_user = 1,
+ dc->no_user = 1;
+ dc->reset = piix3_reset;
k->no_hotplug = 1;
- k->init = piix3_initfn;
+ k->init = piix3_realize;
k->config_write = piix3_write_config;
k->vendor_id = PCI_VENDOR_ID_INTEL;
k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0; // 82371SB PIIX3 PCI-to-ISA bridge (Step A1)
@@ -519,9 +526,10 @@ static void piix3_class_init(ObjectClass *klass, void *data)
}
static TypeInfo piix3_info = {
- .name = "PIIX3",
+ .name = TYPE_PIIX3,
.parent = TYPE_PCI_DEVICE,
.instance_size = sizeof(PIIX3State),
+ .instance_init = piix3_initfn,
.class_init = piix3_class_init,
};
@@ -534,7 +542,7 @@ static void piix3_xen_class_init(ObjectClass *klass, void *data)
static TypeInfo piix3_xen_info = {
.name = "PIIX3-xen",
- .parent = "PIIX3",
+ .parent = TYPE_PIIX3,
.instance_size = sizeof(PIIX3State),
.class_init = piix3_xen_class_init,
};
--
1.7.4.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [PATCH 05/15] piix: create the HPET and RTC through composition
2012-01-26 19:00 [Qemu-devel] [RFC 00/15] Refactor PC machine to take advantage of QOM Anthony Liguori
` (3 preceding siblings ...)
2012-01-26 19:00 ` [Qemu-devel] [PATCH 04/15] piix: prepare for composition Anthony Liguori
@ 2012-01-26 19:00 ` Anthony Liguori
2012-01-31 14:26 ` Jan Kiszka
2012-01-26 19:00 ` [Qemu-devel] [PATCH 06/15] piix: create i8254 " Anthony Liguori
` (10 subsequent siblings)
15 siblings, 1 reply; 55+ messages in thread
From: Anthony Liguori @ 2012-01-26 19:00 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Anthony Liguori, Jan Kiszka, Markus Armbruster,
Avi Kivity, Paolo Bonzini
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
hw/hpet.c | 38 +-------------------------
hw/hpet_emul.h | 40 ++++++++++++++++++++++++++++
hw/mc146818rtc.c | 30 ++-------------------
hw/mc146818rtc.h | 27 +++++++++++++++++++
hw/pc.c | 38 +++++----------------------
hw/piix_pci.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++-------
6 files changed, 145 insertions(+), 104 deletions(-)
diff --git a/hw/hpet.c b/hw/hpet.c
index b6ace4e..c5b8b9e 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -41,40 +41,6 @@
#define HPET_MSI_SUPPORT 0
-struct HPETState;
-typedef struct HPETTimer { /* timers */
- uint8_t tn; /*timer number*/
- QEMUTimer *qemu_timer;
- struct HPETState *state;
- /* Memory-mapped, software visible timer registers */
- uint64_t config; /* configuration/cap */
- uint64_t cmp; /* comparator */
- uint64_t fsb; /* FSB route */
- /* Hidden register state */
- uint64_t period; /* Last value written to comparator */
- uint8_t wrap_flag; /* timer pop will indicate wrap for one-shot 32-bit
- * mode. Next pop will be actual timer expiration.
- */
-} HPETTimer;
-
-typedef struct HPETState {
- SysBusDevice busdev;
- MemoryRegion iomem;
- uint64_t hpet_offset;
- qemu_irq irqs[HPET_NUM_IRQ_ROUTES];
- uint32_t flags;
- uint8_t rtc_irq_level;
- uint8_t num_timers;
- HPETTimer timer[HPET_MAX_TIMERS];
-
- /* Memory-mapped, software visible registers */
- uint64_t capability; /* capabilities */
- uint64_t config; /* configuration */
- uint64_t isr; /* interrupt status reg */
- uint64_t hpet_counter; /* main counter */
- uint8_t hpet_id; /* instance id */
-} HPETState;
-
static uint32_t hpet_in_legacy_mode(HPETState *s)
{
return s->config & HPET_CFG_LEGACY;
@@ -258,7 +224,7 @@ static const VMStateDescription vmstate_hpet_timer = {
};
static const VMStateDescription vmstate_hpet = {
- .name = "hpet",
+ .name = TYPE_HPET,
.version_id = 2,
.minimum_version_id = 1,
.minimum_version_id_old = 1,
@@ -714,7 +680,7 @@ static void hpet_device_class_init(ObjectClass *klass, void *data)
}
static TypeInfo hpet_device_info = {
- .name = "hpet",
+ .name = TYPE_HPET,
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(HPETState),
.class_init = hpet_device_class_init,
diff --git a/hw/hpet_emul.h b/hw/hpet_emul.h
index 6128702..da8ecdc 100644
--- a/hw/hpet_emul.h
+++ b/hw/hpet_emul.h
@@ -13,6 +13,9 @@
#ifndef QEMU_HPET_EMUL_H
#define QEMU_HPET_EMUL_H
+#include "hw.h"
+#include "sysbus.h"
+
#define HPET_BASE 0xfed00000
#define HPET_CLK_PERIOD 10000000ULL /* 10000000 femtoseconds == 10ns*/
@@ -68,4 +71,41 @@ struct hpet_fw_config
} QEMU_PACKED;
extern struct hpet_fw_config hpet_cfg;
+
+#define TYPE_HPET "hpet"
+
+struct HPETState;
+typedef struct HPETTimer { /* timers */
+ uint8_t tn; /*timer number*/
+ QEMUTimer *qemu_timer;
+ struct HPETState *state;
+ /* Memory-mapped, software visible timer registers */
+ uint64_t config; /* configuration/cap */
+ uint64_t cmp; /* comparator */
+ uint64_t fsb; /* FSB route */
+ /* Hidden register state */
+ uint64_t period; /* Last value written to comparator */
+ uint8_t wrap_flag; /* timer pop will indicate wrap for one-shot 32-bit
+ * mode. Next pop will be actual timer expiration.
+ */
+} HPETTimer;
+
+typedef struct HPETState {
+ SysBusDevice busdev;
+ MemoryRegion iomem;
+ uint64_t hpet_offset;
+ qemu_irq irqs[HPET_NUM_IRQ_ROUTES];
+ uint32_t flags;
+ uint8_t rtc_irq_level;
+ uint8_t num_timers;
+ HPETTimer timer[HPET_MAX_TIMERS];
+
+ /* Memory-mapped, software visible registers */
+ uint64_t capability; /* capabilities */
+ uint64_t config; /* configuration */
+ uint64_t isr; /* interrupt status reg */
+ uint64_t hpet_counter; /* main counter */
+ uint8_t hpet_id; /* instance id */
+} HPETState;
+
#endif
diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index 346a95e..f967e05 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -79,30 +79,6 @@
#define REG_C_PF 0x40
#define REG_C_AF 0x20
-typedef struct RTCState {
- ISADevice dev;
- MemoryRegion io;
- uint8_t cmos_data[128];
- uint8_t cmos_index;
- struct tm current_tm;
- int32_t base_year;
- qemu_irq irq;
- qemu_irq sqw_irq;
- int it_shift;
- /* periodic timer */
- QEMUTimer *periodic_timer;
- int64_t next_periodic_time;
- /* second update */
- int64_t next_second_time;
- uint16_t irq_reinject_on_ack_count;
- uint32_t irq_coalesced;
- uint32_t period;
- QEMUTimer *coalesced_timer;
- QEMUTimer *second_timer;
- QEMUTimer *second_timer2;
- Notifier clock_reset_notifier;
-} RTCState;
-
static void rtc_set_time(RTCState *s);
static void rtc_copy_date(RTCState *s);
@@ -553,7 +529,7 @@ static int rtc_post_load(void *opaque, int version_id)
}
static const VMStateDescription vmstate_rtc = {
- .name = "mc146818rtc",
+ .name = TYPE_RTC,
.version_id = 2,
.minimum_version_id = 1,
.minimum_version_id_old = 1,
@@ -687,7 +663,7 @@ ISADevice *rtc_init(ISABus *bus, int base_year, qemu_irq intercept_irq)
ISADevice *dev;
RTCState *s;
- dev = isa_create(bus, "mc146818rtc");
+ dev = isa_create(bus, TYPE_RTC);
s = DO_UPCAST(RTCState, dev, dev);
qdev_prop_set_int32(&dev->qdev, "base_year", base_year);
qdev_init_nofail(&dev->qdev);
@@ -715,7 +691,7 @@ static void rtc_class_initfn(ObjectClass *klass, void *data)
}
static TypeInfo mc146818rtc_info = {
- .name = "mc146818rtc",
+ .name = TYPE_RTC,
.parent = TYPE_ISA_DEVICE,
.instance_size = sizeof(RTCState),
.class_init = rtc_class_initfn,
diff --git a/hw/mc146818rtc.h b/hw/mc146818rtc.h
index f119930..ce807c0 100644
--- a/hw/mc146818rtc.h
+++ b/hw/mc146818rtc.h
@@ -2,9 +2,36 @@
#define MC146818RTC_H
#include "isa.h"
+#include "notify.h"
#define RTC_ISA_IRQ 8
+#define TYPE_RTC "mc146818rtc"
+
+typedef struct RTCState {
+ ISADevice dev;
+ MemoryRegion io;
+ uint8_t cmos_data[128];
+ uint8_t cmos_index;
+ struct tm current_tm;
+ int32_t base_year;
+ qemu_irq irq;
+ qemu_irq sqw_irq;
+ int it_shift;
+ /* periodic timer */
+ QEMUTimer *periodic_timer;
+ int64_t next_periodic_time;
+ /* second update */
+ int64_t next_second_time;
+ uint16_t irq_reinject_on_ack_count;
+ uint32_t irq_coalesced;
+ uint32_t period;
+ QEMUTimer *coalesced_timer;
+ QEMUTimer *second_timer;
+ QEMUTimer *second_timer2;
+ Notifier clock_reset_notifier;
+} RTCState;
+
ISADevice *rtc_init(ISABus *bus, int base_year, qemu_irq intercept_irq);
void rtc_set_memory(ISADevice *dev, int addr, int val);
void rtc_set_date(ISADevice *dev, const struct tm *tm);
diff --git a/hw/pc.c b/hw/pc.c
index 4b11e44..d3eba63 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -1143,13 +1143,11 @@ static void cpu_request_exit(void *opaque, int irq, int level)
}
static void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
- ISADevice **rtc_state,
- ISADevice **floppy,
- bool no_vmport)
+ ISADevice **floppy,
+ bool no_vmport)
{
int i;
DriveInfo *fd[MAX_FD];
- qemu_irq rtc_irq = NULL;
qemu_irq *a20_line;
ISADevice *i8042, *port92, *vmmouse, *pit;
qemu_irq *cpu_exit_irq;
@@ -1158,20 +1156,6 @@ static void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
register_ioport_write(0xf0, 1, 1, ioportF0_write, NULL);
- if (!no_hpet) {
- DeviceState *hpet = sysbus_try_create_simple("hpet", HPET_BASE, NULL);
-
- if (hpet) {
- for (i = 0; i < GSI_NUM_PINS; i++) {
- sysbus_connect_irq(sysbus_from_qdev(hpet), i, gsi[i]);
- }
- rtc_irq = qdev_get_gpio_in(hpet, 0);
- }
- }
- *rtc_state = rtc_init(isa_bus, 2000, rtc_irq);
-
- qemu_register_boot_set(pc_boot_set, *rtc_state);
-
pit = pit_init(isa_bus, 0x40, 0);
pcspk_init(pit);
@@ -1377,7 +1361,6 @@ static void pc_init1(MemoryRegion *system_memory,
isa_bus = isa_bus_new(NULL, system_io);
no_hpet = 1;
}
- isa_bus_irqs(isa_bus, gsi);
if (kvm_enabled() && kvm_irqchip_in_kernel()) {
i8259 = kvm_i8259_init(isa_bus);
@@ -1407,7 +1390,7 @@ static void pc_init1(MemoryRegion *system_memory,
}
/* init basic PC hardware */
- pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, xen_enabled());
+ pc_basic_device_init(isa_bus, gsi, &floppy, xen_enabled());
for(i = 0; i < nb_nics; i++) {
NICInfo *nd = &nd_table[i];
@@ -1428,17 +1411,6 @@ static void pc_init1(MemoryRegion *system_memory,
}
idebus[0] = qdev_get_child_bus(&dev->qdev, "ide.0");
idebus[1] = qdev_get_child_bus(&dev->qdev, "ide.1");
-
- /* FIXME there's some major spaghetti here. Somehow we create the
- * devices on the PIIX before we actually create it. We create the
- * PIIX3 deep in the recess of the i440fx creation too and then lose
- * the DeviceState.
- *
- * For now, let's "fix" this by making judicious use of paths. This
- * is not generally the right way to do this.
- */
- object_property_add_child(object_resolve_path("/i440fx/piix3", NULL),
- "rtc", (Object *)rtc_state, NULL);
} else {
for(i = 0; i < MAX_IDE_BUS; i++) {
ISADevice *dev;
@@ -1449,6 +1421,10 @@ static void pc_init1(MemoryRegion *system_memory,
}
}
+ /* FIXME */
+ rtc_state = ISA_DEVICE(object_resolve_path("rtc", NULL));
+ qemu_register_boot_set(pc_boot_set, rtc_state);
+
audio_init(isa_bus, pci_enabled ? pci_bus : NULL);
pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device,
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index ec75725..5d7d175 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -30,6 +30,8 @@
#include "sysbus.h"
#include "range.h"
#include "xen.h"
+#include "hpet_emul.h"
+#include "mc146818rtc.h"
/*
* I440FX chipset data sheet.
@@ -63,6 +65,13 @@ typedef struct PIIX3State {
#endif
uint64_t pic_levels;
+ bool hpet_enable;
+
+ HPETState hpet;
+ RTCState rtc;
+
+ ISABus *bus;
+
qemu_irq *pic;
/* This member isn't used. Just for save/load compatibility */
@@ -309,20 +318,28 @@ static PCIBus *i440fx_common_init(const char *device_name,
* connected to the IOAPIC directly.
* These additional routes can be discovered through ACPI. */
if (xen_enabled()) {
- piix3 = DO_UPCAST(PIIX3State, dev,
- pci_create_simple_multifunction(b, -1, true, "PIIX3-xen"));
+ piix3 = PIIX3(object_new("PIIX3-xen"));
+ } else {
+ piix3 = PIIX3(object_new("PIIX3"));
+ }
+
+ /* FIXME make this a property */
+ piix3->pic = pic;
+ qdev_prop_set_uint32(DEVICE(piix3), "addr", PCI_DEVFN(1, 0));
+ qdev_prop_set_bit(DEVICE(piix3), "multifunction", true);
+ qdev_set_parent_bus(DEVICE(piix3), BUS(s->bus));
+ qdev_init_nofail(DEVICE(piix3));
+
+ if (xen_enabled()) {
pci_bus_irqs(b, xen_piix3_set_irq, xen_pci_slot_get_pirq,
- piix3, XEN_PIIX_NUM_PIRQS);
+ piix3, XEN_PIIX_NUM_PIRQS);
} else {
- piix3 = DO_UPCAST(PIIX3State, dev,
- pci_create_simple_multifunction(b, -1, true, "PIIX3"));
pci_bus_irqs(b, piix3_set_irq, pci_slot_get_pirq, piix3,
PIIX_NUM_PIRQS);
}
+
object_property_add_child(OBJECT(dev), "piix3", OBJECT(piix3), NULL);
- piix3->pic = pic;
- *isa_bus = DO_UPCAST(ISABus, qbus,
- qdev_get_child_bus(&piix3->dev.qdev, "isa.0"));
+ *isa_bus = piix3->bus;
*piix3_devfn = piix3->dev.devfn;
@@ -498,14 +515,53 @@ static const VMStateDescription vmstate_piix3 = {
static int piix3_realize(PCIDevice *dev)
{
- PIIX3State *d = DO_UPCAST(PIIX3State, dev, dev);
+ PIIX3State *s = PIIX3(dev);
+ qemu_irq rtc_irq;
+
+ /* Initialize ISA Bus */
+ s->bus = isa_bus_new(DEVICE(dev), pci_address_space_io(dev));
+ isa_bus_irqs(s->bus, s->pic);
+
+ /* Realize the RTC */
+ qdev_set_parent_bus(DEVICE(&s->rtc), BUS(s->bus));
+ qdev_init_nofail(DEVICE(&s->rtc));
+
+ /* Realize HPET */
+ if (s->hpet_enable) {
+ int i;
+
+ /* We need to introduce a proper IRQ and Memory QOM infrastructure
+ * so that the HPET isn't a sysbus device */
+ qdev_set_parent_bus(DEVICE(&s->hpet), sysbus_get_default());
+ qdev_init_nofail(DEVICE(&s->hpet));
+
+ sysbus_mmio_map(SYS_BUS_DEVICE(&s->hpet), 0, HPET_BASE);
+ for (i = 0; i < GSI_NUM_PINS; i++) {
+ sysbus_connect_irq(SYS_BUS_DEVICE(&s->hpet), i, s->pic[i]);
+ }
+
+ rtc_irq = qdev_get_gpio_in(DEVICE(&s->hpet), 0);
+ } else {
+ isa_init_irq(ISA_DEVICE(&s->rtc), &rtc_irq, RTC_ISA_IRQ);
+ }
+
+ /* Setup the RTC IRQ */
+ s->rtc.irq = rtc_irq;
- isa_bus_new(&d->dev.qdev, pci_address_space_io(dev));
return 0;
}
static void piix3_initfn(Object *obj)
{
+ PIIX3State *s = PIIX3(obj);
+
+ object_initialize(&s->hpet, TYPE_HPET);
+ object_property_add_child(obj, "hpet", OBJECT(&s->hpet), NULL);
+
+ object_initialize(&s->rtc, TYPE_RTC);
+ object_property_add_child(obj, "rtc", OBJECT(&s->rtc), NULL);
+
+ qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
}
static void piix3_class_init(ObjectClass *klass, void *data)
--
1.7.4.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [PATCH 05/15] piix: create the HPET and RTC through composition
2012-01-26 19:00 ` [Qemu-devel] [PATCH 05/15] piix: create the HPET and RTC through composition Anthony Liguori
@ 2012-01-31 14:26 ` Jan Kiszka
2012-01-31 14:43 ` Anthony Liguori
2012-01-31 16:02 ` Jan Kiszka
0 siblings, 2 replies; 55+ messages in thread
From: Jan Kiszka @ 2012-01-31 14:26 UTC (permalink / raw)
To: Anthony Liguori
Cc: Peter Maydell, Avi Kivity, Paolo Bonzini, qemu-devel,
Markus Armbruster
On 2012-01-26 20:00, Anthony Liguori wrote:
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
> hw/hpet.c | 38 +-------------------------
> hw/hpet_emul.h | 40 ++++++++++++++++++++++++++++
> hw/mc146818rtc.c | 30 ++-------------------
> hw/mc146818rtc.h | 27 +++++++++++++++++++
> hw/pc.c | 38 +++++----------------------
> hw/piix_pci.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++-------
> 6 files changed, 145 insertions(+), 104 deletions(-)
>
> diff --git a/hw/hpet.c b/hw/hpet.c
> index b6ace4e..c5b8b9e 100644
> --- a/hw/hpet.c
> +++ b/hw/hpet.c
> @@ -41,40 +41,6 @@
>
> #define HPET_MSI_SUPPORT 0
>
> -struct HPETState;
> -typedef struct HPETTimer { /* timers */
> - uint8_t tn; /*timer number*/
> - QEMUTimer *qemu_timer;
> - struct HPETState *state;
> - /* Memory-mapped, software visible timer registers */
> - uint64_t config; /* configuration/cap */
> - uint64_t cmp; /* comparator */
> - uint64_t fsb; /* FSB route */
> - /* Hidden register state */
> - uint64_t period; /* Last value written to comparator */
> - uint8_t wrap_flag; /* timer pop will indicate wrap for one-shot 32-bit
> - * mode. Next pop will be actual timer expiration.
> - */
> -} HPETTimer;
> -
> -typedef struct HPETState {
> - SysBusDevice busdev;
> - MemoryRegion iomem;
> - uint64_t hpet_offset;
> - qemu_irq irqs[HPET_NUM_IRQ_ROUTES];
> - uint32_t flags;
> - uint8_t rtc_irq_level;
> - uint8_t num_timers;
> - HPETTimer timer[HPET_MAX_TIMERS];
> -
> - /* Memory-mapped, software visible registers */
> - uint64_t capability; /* capabilities */
> - uint64_t config; /* configuration */
> - uint64_t isr; /* interrupt status reg */
> - uint64_t hpet_counter; /* main counter */
> - uint8_t hpet_id; /* instance id */
> -} HPETState;
> -
Both structs are private and should remain so, same for similar patches
in this series. Does your composition concept requires publicizing them?
If yes, can't it be fixed. Would be a step backward if not.
Also note that the HPET is not a part of the PIIX, so composition is
wrong here. The RTC is again.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [PATCH 05/15] piix: create the HPET and RTC through composition
2012-01-31 14:26 ` Jan Kiszka
@ 2012-01-31 14:43 ` Anthony Liguori
2012-01-31 14:49 ` Jan Kiszka
2012-01-31 16:02 ` Jan Kiszka
1 sibling, 1 reply; 55+ messages in thread
From: Anthony Liguori @ 2012-01-31 14:43 UTC (permalink / raw)
To: Jan Kiszka
Cc: Peter Maydell, Anthony Liguori, qemu-devel, Markus Armbruster,
Avi Kivity, Paolo Bonzini
On 01/31/2012 08:26 AM, Jan Kiszka wrote:
> On 2012-01-26 20:00, Anthony Liguori wrote:
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>> ---
>> hw/hpet.c | 38 +-------------------------
>> hw/hpet_emul.h | 40 ++++++++++++++++++++++++++++
>> hw/mc146818rtc.c | 30 ++-------------------
>> hw/mc146818rtc.h | 27 +++++++++++++++++++
>> hw/pc.c | 38 +++++----------------------
>> hw/piix_pci.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++-------
>> 6 files changed, 145 insertions(+), 104 deletions(-)
>>
>> diff --git a/hw/hpet.c b/hw/hpet.c
>> index b6ace4e..c5b8b9e 100644
>> --- a/hw/hpet.c
>> +++ b/hw/hpet.c
>> @@ -41,40 +41,6 @@
>>
>> #define HPET_MSI_SUPPORT 0
>>
>> -struct HPETState;
>> -typedef struct HPETTimer { /* timers */
>> - uint8_t tn; /*timer number*/
>> - QEMUTimer *qemu_timer;
>> - struct HPETState *state;
>> - /* Memory-mapped, software visible timer registers */
>> - uint64_t config; /* configuration/cap */
>> - uint64_t cmp; /* comparator */
>> - uint64_t fsb; /* FSB route */
>> - /* Hidden register state */
>> - uint64_t period; /* Last value written to comparator */
>> - uint8_t wrap_flag; /* timer pop will indicate wrap for one-shot 32-bit
>> - * mode. Next pop will be actual timer expiration.
>> - */
>> -} HPETTimer;
>> -
>> -typedef struct HPETState {
>> - SysBusDevice busdev;
>> - MemoryRegion iomem;
>> - uint64_t hpet_offset;
>> - qemu_irq irqs[HPET_NUM_IRQ_ROUTES];
>> - uint32_t flags;
>> - uint8_t rtc_irq_level;
>> - uint8_t num_timers;
>> - HPETTimer timer[HPET_MAX_TIMERS];
>> -
>> - /* Memory-mapped, software visible registers */
>> - uint64_t capability; /* capabilities */
>> - uint64_t config; /* configuration */
>> - uint64_t isr; /* interrupt status reg */
>> - uint64_t hpet_counter; /* main counter */
>> - uint8_t hpet_id; /* instance id */
>> -} HPETState;
>> -
>
> Both structs are private and should remain so, same for similar patches
> in this series. Does your composition concept requires publicizing them?
> If yes, can't it be fixed. Would be a step backward if not.
It doesn't strictly require it, no, but I like it. It encourages using proper
interfaces like:
void rtc_set_memory(RTCState *rtc, int addr, int val);
Instead of:
void rtc_set_memory(ISADevice *dev, int addr, int val);
Yes, we can achieve the same thing with forward declarations. The second thing
I like about this style is that it makes it easier to use a code generator to
generate serialization functions. Finally, I think embedded a devices memory
within its parent device provides a certain level of elegance.
> Also note that the HPET is not a part of the PIIX, so composition is
> wrong here.
There is no HPET in an i440fx system. The HPET usually sits on the LPC bus
(which replaces ISA in modern systems). It's sometimes a dedicated chip but can
certain co-exist in a Super IO chip. I think in terms of where it would live in
this hypothetical device model, putting it in the PIIX is rational.
> The RTC is again.
-ENOPARSE
Regards,
Anthony Liguori
>
> Jan
>
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [PATCH 05/15] piix: create the HPET and RTC through composition
2012-01-31 14:43 ` Anthony Liguori
@ 2012-01-31 14:49 ` Jan Kiszka
2012-01-31 14:54 ` Anthony Liguori
0 siblings, 1 reply; 55+ messages in thread
From: Jan Kiszka @ 2012-01-31 14:49 UTC (permalink / raw)
To: Anthony Liguori
Cc: Peter Maydell, Anthony Liguori, qemu-devel@nongnu.org,
Markus Armbruster, Blue Swirl, Avi Kivity, Paolo Bonzini
On 2012-01-31 15:43, Anthony Liguori wrote:
> On 01/31/2012 08:26 AM, Jan Kiszka wrote:
>> On 2012-01-26 20:00, Anthony Liguori wrote:
>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>> ---
>>> hw/hpet.c | 38 +-------------------------
>>> hw/hpet_emul.h | 40 ++++++++++++++++++++++++++++
>>> hw/mc146818rtc.c | 30 ++-------------------
>>> hw/mc146818rtc.h | 27 +++++++++++++++++++
>>> hw/pc.c | 38 +++++----------------------
>>> hw/piix_pci.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++-------
>>> 6 files changed, 145 insertions(+), 104 deletions(-)
>>>
>>> diff --git a/hw/hpet.c b/hw/hpet.c
>>> index b6ace4e..c5b8b9e 100644
>>> --- a/hw/hpet.c
>>> +++ b/hw/hpet.c
>>> @@ -41,40 +41,6 @@
>>>
>>> #define HPET_MSI_SUPPORT 0
>>>
>>> -struct HPETState;
>>> -typedef struct HPETTimer { /* timers */
>>> - uint8_t tn; /*timer number*/
>>> - QEMUTimer *qemu_timer;
>>> - struct HPETState *state;
>>> - /* Memory-mapped, software visible timer registers */
>>> - uint64_t config; /* configuration/cap */
>>> - uint64_t cmp; /* comparator */
>>> - uint64_t fsb; /* FSB route */
>>> - /* Hidden register state */
>>> - uint64_t period; /* Last value written to comparator */
>>> - uint8_t wrap_flag; /* timer pop will indicate wrap for one-shot 32-bit
>>> - * mode. Next pop will be actual timer expiration.
>>> - */
>>> -} HPETTimer;
>>> -
>>> -typedef struct HPETState {
>>> - SysBusDevice busdev;
>>> - MemoryRegion iomem;
>>> - uint64_t hpet_offset;
>>> - qemu_irq irqs[HPET_NUM_IRQ_ROUTES];
>>> - uint32_t flags;
>>> - uint8_t rtc_irq_level;
>>> - uint8_t num_timers;
>>> - HPETTimer timer[HPET_MAX_TIMERS];
>>> -
>>> - /* Memory-mapped, software visible registers */
>>> - uint64_t capability; /* capabilities */
>>> - uint64_t config; /* configuration */
>>> - uint64_t isr; /* interrupt status reg */
>>> - uint64_t hpet_counter; /* main counter */
>>> - uint8_t hpet_id; /* instance id */
>>> -} HPETState;
>>> -
>>
>> Both structs are private and should remain so, same for similar patches
>> in this series. Does your composition concept requires publicizing them?
>> If yes, can't it be fixed. Would be a step backward if not.
>
> It doesn't strictly require it, no, but I like it. It encourages using proper
> interfaces like:
>
> void rtc_set_memory(RTCState *rtc, int addr, int val);
>
> Instead of:
>
> void rtc_set_memory(ISADevice *dev, int addr, int val);
>
> Yes, we can achieve the same thing with forward declarations. The second thing
> I like about this style is that it makes it easier to use a code generator to
> generate serialization functions. Finally, I think embedded a devices memory
> within its parent device provides a certain level of elegance.
It reopens the door for poking inside the device states. That was closed
(widely) by privatizing the states (I think mostly driven by Blue). I'm
not convinced yet that being able to embed the struct into a containing
device is worth giving up on this.
>
>> Also note that the HPET is not a part of the PIIX, so composition is
>> wrong here.
>
> There is no HPET in an i440fx system. The HPET usually sits on the LPC bus
> (which replaces ISA in modern systems). It's sometimes a dedicated chip but can
> certain co-exist in a Super IO chip. I think in terms of where it would live in
> this hypothetical device model, putting it in the PIIX is rational.
Does it buy us anything? I don't see the advantage of this imprecision.
If the model works well, it should be able to cover the real
architecture elegantly, too.
>
>> The RTC is again.
>
> -ENOPARSE
I meant that the RTC was correctly moved into the PIIX.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [PATCH 05/15] piix: create the HPET and RTC through composition
2012-01-31 14:49 ` Jan Kiszka
@ 2012-01-31 14:54 ` Anthony Liguori
2012-01-31 14:56 ` Jan Kiszka
0 siblings, 1 reply; 55+ messages in thread
From: Anthony Liguori @ 2012-01-31 14:54 UTC (permalink / raw)
To: Jan Kiszka
Cc: Peter Maydell, Anthony Liguori, qemu-devel@nongnu.org,
Markus Armbruster, Blue Swirl, Avi Kivity, Paolo Bonzini
On 01/31/2012 08:49 AM, Jan Kiszka wrote:
> On 2012-01-31 15:43, Anthony Liguori wrote:
>> On 01/31/2012 08:26 AM, Jan Kiszka wrote:
>>> On 2012-01-26 20:00, Anthony Liguori wrote:
>>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>>> ---
>>>> hw/hpet.c | 38 +-------------------------
>>>> hw/hpet_emul.h | 40 ++++++++++++++++++++++++++++
>>>> hw/mc146818rtc.c | 30 ++-------------------
>>>> hw/mc146818rtc.h | 27 +++++++++++++++++++
>>>> hw/pc.c | 38 +++++----------------------
>>>> hw/piix_pci.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++-------
>>>> 6 files changed, 145 insertions(+), 104 deletions(-)
>>>>
>>>> diff --git a/hw/hpet.c b/hw/hpet.c
>>>> index b6ace4e..c5b8b9e 100644
>>>> --- a/hw/hpet.c
>>>> +++ b/hw/hpet.c
>>>> @@ -41,40 +41,6 @@
>>>>
>>>> #define HPET_MSI_SUPPORT 0
>>>>
>>>> -struct HPETState;
>>>> -typedef struct HPETTimer { /* timers */
>>>> - uint8_t tn; /*timer number*/
>>>> - QEMUTimer *qemu_timer;
>>>> - struct HPETState *state;
>>>> - /* Memory-mapped, software visible timer registers */
>>>> - uint64_t config; /* configuration/cap */
>>>> - uint64_t cmp; /* comparator */
>>>> - uint64_t fsb; /* FSB route */
>>>> - /* Hidden register state */
>>>> - uint64_t period; /* Last value written to comparator */
>>>> - uint8_t wrap_flag; /* timer pop will indicate wrap for one-shot 32-bit
>>>> - * mode. Next pop will be actual timer expiration.
>>>> - */
>>>> -} HPETTimer;
>>>> -
>>>> -typedef struct HPETState {
>>>> - SysBusDevice busdev;
>>>> - MemoryRegion iomem;
>>>> - uint64_t hpet_offset;
>>>> - qemu_irq irqs[HPET_NUM_IRQ_ROUTES];
>>>> - uint32_t flags;
>>>> - uint8_t rtc_irq_level;
>>>> - uint8_t num_timers;
>>>> - HPETTimer timer[HPET_MAX_TIMERS];
>>>> -
>>>> - /* Memory-mapped, software visible registers */
>>>> - uint64_t capability; /* capabilities */
>>>> - uint64_t config; /* configuration */
>>>> - uint64_t isr; /* interrupt status reg */
>>>> - uint64_t hpet_counter; /* main counter */
>>>> - uint8_t hpet_id; /* instance id */
>>>> -} HPETState;
>>>> -
>>>
>>> Both structs are private and should remain so, same for similar patches
>>> in this series. Does your composition concept requires publicizing them?
>>> If yes, can't it be fixed. Would be a step backward if not.
>>
>> It doesn't strictly require it, no, but I like it. It encourages using proper
>> interfaces like:
>>
>> void rtc_set_memory(RTCState *rtc, int addr, int val);
>>
>> Instead of:
>>
>> void rtc_set_memory(ISADevice *dev, int addr, int val);
>>
>> Yes, we can achieve the same thing with forward declarations. The second thing
>> I like about this style is that it makes it easier to use a code generator to
>> generate serialization functions. Finally, I think embedded a devices memory
>> within its parent device provides a certain level of elegance.
>
> It reopens the door for poking inside the device states. That was closed
> (widely) by privatizing the states (I think mostly driven by Blue). I'm
> not convinced yet that being able to embed the struct into a containing
> device is worth giving up on this.
>
>>
>>> Also note that the HPET is not a part of the PIIX, so composition is
>>> wrong here.
>>
>> There is no HPET in an i440fx system. The HPET usually sits on the LPC bus
>> (which replaces ISA in modern systems). It's sometimes a dedicated chip but can
>> certain co-exist in a Super IO chip. I think in terms of where it would live in
>> this hypothetical device model, putting it in the PIIX is rational.
>
> Does it buy us anything? I don't see the advantage of this imprecision.
> If the model works well, it should be able to cover the real
> architecture elegantly, too.
We could move the HPET to a child of the 440fx-pmc. That's probably more correct.
I don't think it's worth modeling an LPC bus. LPC is just a spec for allowing
third party chips, it's not mandated that all HPET chips have a pin-out that's
LPC compatible. I don't think there's any guest-visible state that comes from a
device being on the LPC verses being hard wired in the north bridge.
Regards,
Anthony Liguori
>
>>
>>> The RTC is again.
>>
>> -ENOPARSE
>
> I meant that the RTC was correctly moved into the PIIX.
>
> Jan
>
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [PATCH 05/15] piix: create the HPET and RTC through composition
2012-01-31 14:54 ` Anthony Liguori
@ 2012-01-31 14:56 ` Jan Kiszka
2012-01-31 15:04 ` Anthony Liguori
0 siblings, 1 reply; 55+ messages in thread
From: Jan Kiszka @ 2012-01-31 14:56 UTC (permalink / raw)
To: Anthony Liguori
Cc: Peter Maydell, Anthony Liguori, qemu-devel@nongnu.org,
Markus Armbruster, Blue Swirl, Avi Kivity, Paolo Bonzini
On 2012-01-31 15:54, Anthony Liguori wrote:
> On 01/31/2012 08:49 AM, Jan Kiszka wrote:
>> On 2012-01-31 15:43, Anthony Liguori wrote:
>>> On 01/31/2012 08:26 AM, Jan Kiszka wrote:
>>>> On 2012-01-26 20:00, Anthony Liguori wrote:
>>>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>>>> ---
>>>>> hw/hpet.c | 38 +-------------------------
>>>>> hw/hpet_emul.h | 40 ++++++++++++++++++++++++++++
>>>>> hw/mc146818rtc.c | 30 ++-------------------
>>>>> hw/mc146818rtc.h | 27 +++++++++++++++++++
>>>>> hw/pc.c | 38 +++++----------------------
>>>>> hw/piix_pci.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++-------
>>>>> 6 files changed, 145 insertions(+), 104 deletions(-)
>>>>>
>>>>> diff --git a/hw/hpet.c b/hw/hpet.c
>>>>> index b6ace4e..c5b8b9e 100644
>>>>> --- a/hw/hpet.c
>>>>> +++ b/hw/hpet.c
>>>>> @@ -41,40 +41,6 @@
>>>>>
>>>>> #define HPET_MSI_SUPPORT 0
>>>>>
>>>>> -struct HPETState;
>>>>> -typedef struct HPETTimer { /* timers */
>>>>> - uint8_t tn; /*timer number*/
>>>>> - QEMUTimer *qemu_timer;
>>>>> - struct HPETState *state;
>>>>> - /* Memory-mapped, software visible timer registers */
>>>>> - uint64_t config; /* configuration/cap */
>>>>> - uint64_t cmp; /* comparator */
>>>>> - uint64_t fsb; /* FSB route */
>>>>> - /* Hidden register state */
>>>>> - uint64_t period; /* Last value written to comparator */
>>>>> - uint8_t wrap_flag; /* timer pop will indicate wrap for one-shot 32-bit
>>>>> - * mode. Next pop will be actual timer expiration.
>>>>> - */
>>>>> -} HPETTimer;
>>>>> -
>>>>> -typedef struct HPETState {
>>>>> - SysBusDevice busdev;
>>>>> - MemoryRegion iomem;
>>>>> - uint64_t hpet_offset;
>>>>> - qemu_irq irqs[HPET_NUM_IRQ_ROUTES];
>>>>> - uint32_t flags;
>>>>> - uint8_t rtc_irq_level;
>>>>> - uint8_t num_timers;
>>>>> - HPETTimer timer[HPET_MAX_TIMERS];
>>>>> -
>>>>> - /* Memory-mapped, software visible registers */
>>>>> - uint64_t capability; /* capabilities */
>>>>> - uint64_t config; /* configuration */
>>>>> - uint64_t isr; /* interrupt status reg */
>>>>> - uint64_t hpet_counter; /* main counter */
>>>>> - uint8_t hpet_id; /* instance id */
>>>>> -} HPETState;
>>>>> -
>>>>
>>>> Both structs are private and should remain so, same for similar patches
>>>> in this series. Does your composition concept requires publicizing them?
>>>> If yes, can't it be fixed. Would be a step backward if not.
>>>
>>> It doesn't strictly require it, no, but I like it. It encourages using proper
>>> interfaces like:
>>>
>>> void rtc_set_memory(RTCState *rtc, int addr, int val);
>>>
>>> Instead of:
>>>
>>> void rtc_set_memory(ISADevice *dev, int addr, int val);
>>>
>>> Yes, we can achieve the same thing with forward declarations. The second thing
>>> I like about this style is that it makes it easier to use a code generator to
>>> generate serialization functions. Finally, I think embedded a devices memory
>>> within its parent device provides a certain level of elegance.
>>
>> It reopens the door for poking inside the device states. That was closed
>> (widely) by privatizing the states (I think mostly driven by Blue). I'm
>> not convinced yet that being able to embed the struct into a containing
>> device is worth giving up on this.
>>
>>>
>>>> Also note that the HPET is not a part of the PIIX, so composition is
>>>> wrong here.
>>>
>>> There is no HPET in an i440fx system. The HPET usually sits on the LPC bus
>>> (which replaces ISA in modern systems). It's sometimes a dedicated chip but can
>>> certain co-exist in a Super IO chip. I think in terms of where it would live in
>>> this hypothetical device model, putting it in the PIIX is rational.
>>
>> Does it buy us anything? I don't see the advantage of this imprecision.
>> If the model works well, it should be able to cover the real
>> architecture elegantly, too.
>
> We could move the HPET to a child of the 440fx-pmc. That's probably more correct.
Nope, it was a separate chip in such systems. It sits on the board
(today our sysbus), nakedly and alone.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [PATCH 05/15] piix: create the HPET and RTC through composition
2012-01-31 14:56 ` Jan Kiszka
@ 2012-01-31 15:04 ` Anthony Liguori
0 siblings, 0 replies; 55+ messages in thread
From: Anthony Liguori @ 2012-01-31 15:04 UTC (permalink / raw)
To: Jan Kiszka
Cc: Peter Maydell, Anthony Liguori, qemu-devel@nongnu.org,
Markus Armbruster, Blue Swirl, Avi Kivity, Paolo Bonzini
On 01/31/2012 08:56 AM, Jan Kiszka wrote:
> On 2012-01-31 15:54, Anthony Liguori wrote:
>> On 01/31/2012 08:49 AM, Jan Kiszka wrote:
>>> On 2012-01-31 15:43, Anthony Liguori wrote:
>>>> On 01/31/2012 08:26 AM, Jan Kiszka wrote:
>>>>> On 2012-01-26 20:00, Anthony Liguori wrote:
>>>>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>>>>> ---
>>>>>> hw/hpet.c | 38 +-------------------------
>>>>>> hw/hpet_emul.h | 40 ++++++++++++++++++++++++++++
>>>>>> hw/mc146818rtc.c | 30 ++-------------------
>>>>>> hw/mc146818rtc.h | 27 +++++++++++++++++++
>>>>>> hw/pc.c | 38 +++++----------------------
>>>>>> hw/piix_pci.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++-------
>>>>>> 6 files changed, 145 insertions(+), 104 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/hpet.c b/hw/hpet.c
>>>>>> index b6ace4e..c5b8b9e 100644
>>>>>> --- a/hw/hpet.c
>>>>>> +++ b/hw/hpet.c
>>>>>> @@ -41,40 +41,6 @@
>>>>>>
>>>>>> #define HPET_MSI_SUPPORT 0
>>>>>>
>>>>>> -struct HPETState;
>>>>>> -typedef struct HPETTimer { /* timers */
>>>>>> - uint8_t tn; /*timer number*/
>>>>>> - QEMUTimer *qemu_timer;
>>>>>> - struct HPETState *state;
>>>>>> - /* Memory-mapped, software visible timer registers */
>>>>>> - uint64_t config; /* configuration/cap */
>>>>>> - uint64_t cmp; /* comparator */
>>>>>> - uint64_t fsb; /* FSB route */
>>>>>> - /* Hidden register state */
>>>>>> - uint64_t period; /* Last value written to comparator */
>>>>>> - uint8_t wrap_flag; /* timer pop will indicate wrap for one-shot 32-bit
>>>>>> - * mode. Next pop will be actual timer expiration.
>>>>>> - */
>>>>>> -} HPETTimer;
>>>>>> -
>>>>>> -typedef struct HPETState {
>>>>>> - SysBusDevice busdev;
>>>>>> - MemoryRegion iomem;
>>>>>> - uint64_t hpet_offset;
>>>>>> - qemu_irq irqs[HPET_NUM_IRQ_ROUTES];
>>>>>> - uint32_t flags;
>>>>>> - uint8_t rtc_irq_level;
>>>>>> - uint8_t num_timers;
>>>>>> - HPETTimer timer[HPET_MAX_TIMERS];
>>>>>> -
>>>>>> - /* Memory-mapped, software visible registers */
>>>>>> - uint64_t capability; /* capabilities */
>>>>>> - uint64_t config; /* configuration */
>>>>>> - uint64_t isr; /* interrupt status reg */
>>>>>> - uint64_t hpet_counter; /* main counter */
>>>>>> - uint8_t hpet_id; /* instance id */
>>>>>> -} HPETState;
>>>>>> -
>>>>>
>>>>> Both structs are private and should remain so, same for similar patches
>>>>> in this series. Does your composition concept requires publicizing them?
>>>>> If yes, can't it be fixed. Would be a step backward if not.
>>>>
>>>> It doesn't strictly require it, no, but I like it. It encourages using proper
>>>> interfaces like:
>>>>
>>>> void rtc_set_memory(RTCState *rtc, int addr, int val);
>>>>
>>>> Instead of:
>>>>
>>>> void rtc_set_memory(ISADevice *dev, int addr, int val);
>>>>
>>>> Yes, we can achieve the same thing with forward declarations. The second thing
>>>> I like about this style is that it makes it easier to use a code generator to
>>>> generate serialization functions. Finally, I think embedded a devices memory
>>>> within its parent device provides a certain level of elegance.
>>>
>>> It reopens the door for poking inside the device states. That was closed
>>> (widely) by privatizing the states (I think mostly driven by Blue). I'm
>>> not convinced yet that being able to embed the struct into a containing
>>> device is worth giving up on this.
>>>
>>>>
>>>>> Also note that the HPET is not a part of the PIIX, so composition is
>>>>> wrong here.
>>>>
>>>> There is no HPET in an i440fx system. The HPET usually sits on the LPC bus
>>>> (which replaces ISA in modern systems). It's sometimes a dedicated chip but can
>>>> certain co-exist in a Super IO chip. I think in terms of where it would live in
>>>> this hypothetical device model, putting it in the PIIX is rational.
>>>
>>> Does it buy us anything? I don't see the advantage of this imprecision.
>>> If the model works well, it should be able to cover the real
>>> architecture elegantly, too.
>>
>> We could move the HPET to a child of the 440fx-pmc. That's probably more correct.
>
> Nope, it was a separate chip in such systems. It sits on the board
> (today our sysbus), nakedly and alone.
So the northbridge would need to implement an LPC bus. This can be as simple as
having an LPC interface (which just consists of a few MemoryRegions and Pins)
and then a few link<LPCDevice> in the i440fx-pmc for expansion.
Regards,
Anthony Liguori
>
> Jan
>
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [PATCH 05/15] piix: create the HPET and RTC through composition
2012-01-31 14:26 ` Jan Kiszka
2012-01-31 14:43 ` Anthony Liguori
@ 2012-01-31 16:02 ` Jan Kiszka
1 sibling, 0 replies; 55+ messages in thread
From: Jan Kiszka @ 2012-01-31 16:02 UTC (permalink / raw)
To: Anthony Liguori
Cc: Peter Maydell, Avi Kivity, Paolo Bonzini, qemu-devel,
Markus Armbruster
On 2012-01-31 15:26, Jan Kiszka wrote:
> Also note that the HPET is not a part of the PIIX, so composition is
> wrong here. The RTC is again.
Err, forgot my nonsense. The HPET is part of the PIIX. Dunno, I was
somehow thinking of the IOAPIC while reading "HPET". Too few sleep, I
guess...
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] [PATCH 06/15] piix: create i8254 through composition
2012-01-26 19:00 [Qemu-devel] [RFC 00/15] Refactor PC machine to take advantage of QOM Anthony Liguori
` (4 preceding siblings ...)
2012-01-26 19:00 ` [Qemu-devel] [PATCH 05/15] piix: create the HPET and RTC through composition Anthony Liguori
@ 2012-01-26 19:00 ` Anthony Liguori
2012-01-31 14:34 ` Jan Kiszka
2012-01-26 19:00 ` [Qemu-devel] [PATCH 07/15] i440fx: eliminate i440fx_common_init Anthony Liguori
` (9 subsequent siblings)
15 siblings, 1 reply; 55+ messages in thread
From: Anthony Liguori @ 2012-01-26 19:00 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Anthony Liguori, Jan Kiszka, Markus Armbruster,
Avi Kivity, Paolo Bonzini
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
hw/i8254.c | 35 +++--------------------------------
hw/i8254.h | 38 ++++++++++++++++++++++++++++++++++++++
hw/pc.c | 5 +----
hw/piix_pci.c | 15 ++++++++++++++-
4 files changed, 56 insertions(+), 37 deletions(-)
create mode 100644 hw/i8254.h
diff --git a/hw/i8254.c b/hw/i8254.c
index 522fed8..fa70ff0 100644
--- a/hw/i8254.c
+++ b/hw/i8254.c
@@ -21,10 +21,9 @@
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/
-#include "hw.h"
+
+#include "i8254.h"
#include "pc.h"
-#include "isa.h"
-#include "qemu-timer.h"
//#define DEBUG_PIT
@@ -33,34 +32,6 @@
#define RW_STATE_WORD0 3
#define RW_STATE_WORD1 4
-typedef struct PITChannelState {
- int count; /* can be 65536 */
- uint16_t latched_count;
- uint8_t count_latched;
- uint8_t status_latched;
- uint8_t status;
- uint8_t read_state;
- uint8_t write_state;
- uint8_t write_latch;
- uint8_t rw_mode;
- uint8_t mode;
- uint8_t bcd; /* not supported */
- uint8_t gate; /* timer start */
- int64_t count_load_time;
- /* irq handling */
- int64_t next_transition_time;
- QEMUTimer *irq_timer;
- qemu_irq irq;
-} PITChannelState;
-
-typedef struct PITState {
- ISADevice dev;
- MemoryRegion ioports;
- uint32_t irq;
- uint32_t iobase;
- PITChannelState channels[3];
-} PITState;
-
static PITState pit_state;
static void pit_irq_timer_update(PITChannelState *s, int64_t current_time);
@@ -553,7 +524,7 @@ static void pit_class_initfn(ObjectClass *klass, void *data)
}
static TypeInfo pit_info = {
- .name = "isa-pit",
+ .name = TYPE_PIT,
.parent = TYPE_ISA_DEVICE,
.instance_size = sizeof(PITState),
.class_init = pit_class_initfn,
diff --git a/hw/i8254.h b/hw/i8254.h
new file mode 100644
index 0000000..1c3656d
--- /dev/null
+++ b/hw/i8254.h
@@ -0,0 +1,38 @@
+#ifndef QEMU_I8254_H
+#define QEMU_I8254_H
+
+#include "hw.h"
+#include "isa.h"
+#include "qemu-timer.h"
+
+#define TYPE_PIT "isa-pit"
+
+typedef struct PITChannelState {
+ int count; /* can be 65536 */
+ uint16_t latched_count;
+ uint8_t count_latched;
+ uint8_t status_latched;
+ uint8_t status;
+ uint8_t read_state;
+ uint8_t write_state;
+ uint8_t write_latch;
+ uint8_t rw_mode;
+ uint8_t mode;
+ uint8_t bcd; /* not supported */
+ uint8_t gate; /* timer start */
+ int64_t count_load_time;
+ /* irq handling */
+ int64_t next_transition_time;
+ QEMUTimer *irq_timer;
+ qemu_irq irq;
+} PITChannelState;
+
+typedef struct PITState {
+ ISADevice dev;
+ MemoryRegion ioports;
+ uint32_t irq;
+ uint32_t iobase;
+ PITChannelState channels[3];
+} PITState;
+
+#endif
diff --git a/hw/pc.c b/hw/pc.c
index d3eba63..95dd582 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -1149,16 +1149,13 @@ static void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
int i;
DriveInfo *fd[MAX_FD];
qemu_irq *a20_line;
- ISADevice *i8042, *port92, *vmmouse, *pit;
+ ISADevice *i8042, *port92, *vmmouse;
qemu_irq *cpu_exit_irq;
register_ioport_write(0x80, 1, 1, ioport80_write, NULL);
register_ioport_write(0xf0, 1, 1, ioportF0_write, NULL);
- pit = pit_init(isa_bus, 0x40, 0);
- pcspk_init(pit);
-
for(i = 0; i < MAX_SERIAL_PORTS; i++) {
if (serial_hds[i]) {
serial_isa_init(isa_bus, i, serial_hds[i]);
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 5d7d175..4735d9c 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -32,6 +32,7 @@
#include "xen.h"
#include "hpet_emul.h"
#include "mc146818rtc.h"
+#include "i8254.h"
/*
* I440FX chipset data sheet.
@@ -69,6 +70,7 @@ typedef struct PIIX3State {
HPETState hpet;
RTCState rtc;
+ PITState pit;
ISABus *bus;
@@ -548,6 +550,13 @@ static int piix3_realize(PCIDevice *dev)
/* Setup the RTC IRQ */
s->rtc.irq = rtc_irq;
+ /* Realize the PIT */
+ qdev_set_parent_bus(DEVICE(&s->pit), BUS(s->bus));
+ qdev_init_nofail(DEVICE(&s->pit));
+
+ /* FIXME this should be refactored */
+ pcspk_init(ISA_DEVICE(&s->pit));
+
return 0;
}
@@ -560,8 +569,12 @@ static void piix3_initfn(Object *obj)
object_initialize(&s->rtc, TYPE_RTC);
object_property_add_child(obj, "rtc", OBJECT(&s->rtc), NULL);
-
qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
+
+ object_initialize(&s->pit, TYPE_PIT);
+ object_property_add_child(obj, "pit", OBJECT(&s->pit), NULL);
+ qdev_prop_set_uint32(DEVICE(&s->pit), "iobase", 0x40);
+ qdev_prop_set_uint32(DEVICE(&s->pit), "irq", 0);
}
static void piix3_class_init(ObjectClass *klass, void *data)
--
1.7.4.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [PATCH 06/15] piix: create i8254 through composition
2012-01-26 19:00 ` [Qemu-devel] [PATCH 06/15] piix: create i8254 " Anthony Liguori
@ 2012-01-31 14:34 ` Jan Kiszka
2012-01-31 14:47 ` Anthony Liguori
0 siblings, 1 reply; 55+ messages in thread
From: Jan Kiszka @ 2012-01-31 14:34 UTC (permalink / raw)
To: Anthony Liguori
Cc: Peter Maydell, Avi Kivity, Paolo Bonzini, qemu-devel,
Markus Armbruster
On 2012-01-26 20:00, Anthony Liguori wrote:
> @@ -548,6 +550,13 @@ static int piix3_realize(PCIDevice *dev)
> /* Setup the RTC IRQ */
> s->rtc.irq = rtc_irq;
>
> + /* Realize the PIT */
> + qdev_set_parent_bus(DEVICE(&s->pit), BUS(s->bus));
> + qdev_init_nofail(DEVICE(&s->pit));
> +
> + /* FIXME this should be refactored */
> + pcspk_init(ISA_DEVICE(&s->pit));
Fixing ATM, ie. converting to qdev/QOM.
Q: How do I use qdev_property_add_link & Co. to establish the relation
from the speaker port device to the PIT?
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [PATCH 06/15] piix: create i8254 through composition
2012-01-31 14:34 ` Jan Kiszka
@ 2012-01-31 14:47 ` Anthony Liguori
2012-01-31 14:51 ` Jan Kiszka
0 siblings, 1 reply; 55+ messages in thread
From: Anthony Liguori @ 2012-01-31 14:47 UTC (permalink / raw)
To: Jan Kiszka
Cc: Peter Maydell, Anthony Liguori, qemu-devel, Markus Armbruster,
Avi Kivity, Paolo Bonzini
On 01/31/2012 08:34 AM, Jan Kiszka wrote:
> On 2012-01-26 20:00, Anthony Liguori wrote:
>> @@ -548,6 +550,13 @@ static int piix3_realize(PCIDevice *dev)
>> /* Setup the RTC IRQ */
>> s->rtc.irq = rtc_irq;
>>
>> + /* Realize the PIT */
>> + qdev_set_parent_bus(DEVICE(&s->pit), BUS(s->bus));
>> + qdev_init_nofail(DEVICE(&s->pit));
>> +
>> + /* FIXME this should be refactored */
>> + pcspk_init(ISA_DEVICE(&s->pit));
>
> Fixing ATM, ie. converting to qdev/QOM.
>
> Q: How do I use qdev_property_add_link& Co. to establish the relation
> from the speaker port device to the PIT?
In the state structure, have:
struct PCSpkState {
...
PITState *pit;
};
In the pcspk instance_init, do:
object_property_add_link(obj, "pit", TYPE_PIT, (Object **)&s->pit, NULL);
In the pcspk realize function (DeviceClass::init), do:
assert(s->pit != NULL); // make sure the pit link is set
And that's it.
You can set the s->pit field directly. You are not required to use any special
QOM function to interact with link properties.
BTW, this is yet another benefit of making structures public. You can take the
address of a child and set link fields directly without accessors.
Regards,
Anthony LIguori
>
> Jan
>
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [PATCH 06/15] piix: create i8254 through composition
2012-01-31 14:47 ` Anthony Liguori
@ 2012-01-31 14:51 ` Jan Kiszka
2012-01-31 14:56 ` Anthony Liguori
2012-01-31 14:58 ` Paolo Bonzini
0 siblings, 2 replies; 55+ messages in thread
From: Jan Kiszka @ 2012-01-31 14:51 UTC (permalink / raw)
To: Anthony Liguori
Cc: Peter Maydell, Anthony Liguori, qemu-devel@nongnu.org,
Markus Armbruster, Avi Kivity, Paolo Bonzini
On 2012-01-31 15:47, Anthony Liguori wrote:
> On 01/31/2012 08:34 AM, Jan Kiszka wrote:
>> On 2012-01-26 20:00, Anthony Liguori wrote:
>>> @@ -548,6 +550,13 @@ static int piix3_realize(PCIDevice *dev)
>>> /* Setup the RTC IRQ */
>>> s->rtc.irq = rtc_irq;
>>>
>>> + /* Realize the PIT */
>>> + qdev_set_parent_bus(DEVICE(&s->pit), BUS(s->bus));
>>> + qdev_init_nofail(DEVICE(&s->pit));
>>> +
>>> + /* FIXME this should be refactored */
>>> + pcspk_init(ISA_DEVICE(&s->pit));
>>
>> Fixing ATM, ie. converting to qdev/QOM.
>>
>> Q: How do I use qdev_property_add_link& Co. to establish the relation
>> from the speaker port device to the PIT?
>
> In the state structure, have:
>
> struct PCSpkState {
> ...
> PITState *pit;
> };
>
> In the pcspk instance_init, do:
>
> object_property_add_link(obj, "pit", TYPE_PIT, (Object **)&s->pit, NULL);
>
> In the pcspk realize function (DeviceClass::init), do:
>
> assert(s->pit != NULL); // make sure the pit link is set
>
> And that's it.
>
> You can set the s->pit field directly. You are not required to use any special
> QOM function to interact with link properties.
>
> BTW, this is yet another benefit of making structures public. You can take the
> address of a child and set link fields directly without accessors.
Well, that has two sides. We introduced properties to avoid this direct
messing.
Does linking also work without exposing internals?
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [PATCH 06/15] piix: create i8254 through composition
2012-01-31 14:51 ` Jan Kiszka
@ 2012-01-31 14:56 ` Anthony Liguori
2012-01-31 16:42 ` Jan Kiszka
2012-01-31 14:58 ` Paolo Bonzini
1 sibling, 1 reply; 55+ messages in thread
From: Anthony Liguori @ 2012-01-31 14:56 UTC (permalink / raw)
To: Jan Kiszka
Cc: Peter Maydell, Anthony Liguori, qemu-devel@nongnu.org,
Markus Armbruster, Avi Kivity, Paolo Bonzini
On 01/31/2012 08:51 AM, Jan Kiszka wrote:
> On 2012-01-31 15:47, Anthony Liguori wrote:
>> On 01/31/2012 08:34 AM, Jan Kiszka wrote:
>>> On 2012-01-26 20:00, Anthony Liguori wrote:
>>>> @@ -548,6 +550,13 @@ static int piix3_realize(PCIDevice *dev)
>>>> /* Setup the RTC IRQ */
>>>> s->rtc.irq = rtc_irq;
>>>>
>>>> + /* Realize the PIT */
>>>> + qdev_set_parent_bus(DEVICE(&s->pit), BUS(s->bus));
>>>> + qdev_init_nofail(DEVICE(&s->pit));
>>>> +
>>>> + /* FIXME this should be refactored */
>>>> + pcspk_init(ISA_DEVICE(&s->pit));
>>>
>>> Fixing ATM, ie. converting to qdev/QOM.
>>>
>>> Q: How do I use qdev_property_add_link& Co. to establish the relation
>>> from the speaker port device to the PIT?
>>
>> In the state structure, have:
>>
>> struct PCSpkState {
>> ...
>> PITState *pit;
>> };
>>
>> In the pcspk instance_init, do:
>>
>> object_property_add_link(obj, "pit", TYPE_PIT, (Object **)&s->pit, NULL);
>>
>> In the pcspk realize function (DeviceClass::init), do:
>>
>> assert(s->pit != NULL); // make sure the pit link is set
>>
>> And that's it.
>>
>> You can set the s->pit field directly. You are not required to use any special
>> QOM function to interact with link properties.
>>
>> BTW, this is yet another benefit of making structures public. You can take the
>> address of a child and set link fields directly without accessors.
>
> Well, that has two sides. We introduced properties to avoid this direct
> messing.
>
> Does linking also work without exposing internals?
Yes, you can set links through properties (although I haven't added those
accessors yet).
But... you lose type safety because now you're dealing with strings. If we
want to avoid exposing internals, we should provide accessors to get/set links. So:
typedef struct PCSpkState PCSpkState;
void pcspk_set_pit(PCSpkState *s, PITState *pit);
Regards,
Anthony Liguori
>
> Jan
>
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [PATCH 06/15] piix: create i8254 through composition
2012-01-31 14:56 ` Anthony Liguori
@ 2012-01-31 16:42 ` Jan Kiszka
2012-01-31 16:49 ` Anthony Liguori
0 siblings, 1 reply; 55+ messages in thread
From: Jan Kiszka @ 2012-01-31 16:42 UTC (permalink / raw)
To: Anthony Liguori
Cc: Peter Maydell, Anthony Liguori, qemu-devel@nongnu.org,
Markus Armbruster, Avi Kivity, Paolo Bonzini
On 2012-01-31 15:56, Anthony Liguori wrote:
> On 01/31/2012 08:51 AM, Jan Kiszka wrote:
>> On 2012-01-31 15:47, Anthony Liguori wrote:
>>> On 01/31/2012 08:34 AM, Jan Kiszka wrote:
>>>> On 2012-01-26 20:00, Anthony Liguori wrote:
>>>>> @@ -548,6 +550,13 @@ static int piix3_realize(PCIDevice *dev)
>>>>> /* Setup the RTC IRQ */
>>>>> s->rtc.irq = rtc_irq;
>>>>>
>>>>> + /* Realize the PIT */
>>>>> + qdev_set_parent_bus(DEVICE(&s->pit), BUS(s->bus));
>>>>> + qdev_init_nofail(DEVICE(&s->pit));
>>>>> +
>>>>> + /* FIXME this should be refactored */
>>>>> + pcspk_init(ISA_DEVICE(&s->pit));
>>>>
>>>> Fixing ATM, ie. converting to qdev/QOM.
>>>>
>>>> Q: How do I use qdev_property_add_link& Co. to establish the relation
>>>> from the speaker port device to the PIT?
>>>
>>> In the state structure, have:
>>>
>>> struct PCSpkState {
>>> ...
>>> PITState *pit;
>>> };
>>>
>>> In the pcspk instance_init, do:
>>>
>>> object_property_add_link(obj, "pit", TYPE_PIT, (Object **)&s->pit, NULL);
>>>
>>> In the pcspk realize function (DeviceClass::init), do:
>>>
>>> assert(s->pit != NULL); // make sure the pit link is set
>>>
>>> And that's it.
>>>
>>> You can set the s->pit field directly. You are not required to use any special
>>> QOM function to interact with link properties.
>>>
>>> BTW, this is yet another benefit of making structures public. You can take the
>>> address of a child and set link fields directly without accessors.
>>
>> Well, that has two sides. We introduced properties to avoid this direct
>> messing.
>>
>> Does linking also work without exposing internals?
>
> Yes, you can set links through properties (although I haven't added those
> accessors yet).
>
> But... you lose type safety because now you're dealing with strings.
I don't get yet why we have to give up on type safety here. Isn't all
information stored in the property entry? Can't some
object_set_property() service take the object pointer and validate its
type before writing at the target location? I'm not worried about
lacking compile-time checks if we keep them for runtime.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [PATCH 06/15] piix: create i8254 through composition
2012-01-31 16:42 ` Jan Kiszka
@ 2012-01-31 16:49 ` Anthony Liguori
2012-01-31 16:56 ` Jan Kiszka
0 siblings, 1 reply; 55+ messages in thread
From: Anthony Liguori @ 2012-01-31 16:49 UTC (permalink / raw)
To: Jan Kiszka
Cc: Peter Maydell, Anthony Liguori, qemu-devel@nongnu.org,
Markus Armbruster, Avi Kivity, Paolo Bonzini
On 01/31/2012 10:42 AM, Jan Kiszka wrote:
> On 2012-01-31 15:56, Anthony Liguori wrote:
>> On 01/31/2012 08:51 AM, Jan Kiszka wrote:
>>> On 2012-01-31 15:47, Anthony Liguori wrote:
>>>> On 01/31/2012 08:34 AM, Jan Kiszka wrote:
>>>>> On 2012-01-26 20:00, Anthony Liguori wrote:
>>>>>> @@ -548,6 +550,13 @@ static int piix3_realize(PCIDevice *dev)
>>>>>> /* Setup the RTC IRQ */
>>>>>> s->rtc.irq = rtc_irq;
>>>>>>
>>>>>> + /* Realize the PIT */
>>>>>> + qdev_set_parent_bus(DEVICE(&s->pit), BUS(s->bus));
>>>>>> + qdev_init_nofail(DEVICE(&s->pit));
>>>>>> +
>>>>>> + /* FIXME this should be refactored */
>>>>>> + pcspk_init(ISA_DEVICE(&s->pit));
>>>>>
>>>>> Fixing ATM, ie. converting to qdev/QOM.
>>>>>
>>>>> Q: How do I use qdev_property_add_link& Co. to establish the relation
>>>>> from the speaker port device to the PIT?
>>>>
>>>> In the state structure, have:
>>>>
>>>> struct PCSpkState {
>>>> ...
>>>> PITState *pit;
>>>> };
>>>>
>>>> In the pcspk instance_init, do:
>>>>
>>>> object_property_add_link(obj, "pit", TYPE_PIT, (Object **)&s->pit, NULL);
>>>>
>>>> In the pcspk realize function (DeviceClass::init), do:
>>>>
>>>> assert(s->pit != NULL); // make sure the pit link is set
>>>>
>>>> And that's it.
>>>>
>>>> You can set the s->pit field directly. You are not required to use any special
>>>> QOM function to interact with link properties.
>>>>
>>>> BTW, this is yet another benefit of making structures public. You can take the
>>>> address of a child and set link fields directly without accessors.
>>>
>>> Well, that has two sides. We introduced properties to avoid this direct
>>> messing.
>>>
>>> Does linking also work without exposing internals?
>>
>> Yes, you can set links through properties (although I haven't added those
>> accessors yet).
>>
>> But... you lose type safety because now you're dealing with strings.
>
> I don't get yet why we have to give up on type safety here. Isn't all
> information stored in the property entry? Can't some
> object_set_property() service take the object pointer and validate its
> type before writing at the target location?
Already does that. You'll get a run time warning.
> I'm not worried about
> lacking compile-time checks if we keep them for runtime.
I'm worried about:
object_property_set_link(obj, "pci", OBJECT(&s->pic));
vs:
spk->pic = s->pic;
Or:
pcspk_set_pic(spk, &s->pic);
I tend to make a lot of typos, I like the compiler to catch them for me at build
time.
Regards,
Anthony Liguori
> Jan
>
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [PATCH 06/15] piix: create i8254 through composition
2012-01-31 16:49 ` Anthony Liguori
@ 2012-01-31 16:56 ` Jan Kiszka
0 siblings, 0 replies; 55+ messages in thread
From: Jan Kiszka @ 2012-01-31 16:56 UTC (permalink / raw)
To: Anthony Liguori
Cc: Peter Maydell, Anthony Liguori, qemu-devel@nongnu.org,
Markus Armbruster, Avi Kivity, Paolo Bonzini
On 2012-01-31 17:49, Anthony Liguori wrote:
> On 01/31/2012 10:42 AM, Jan Kiszka wrote:
>> On 2012-01-31 15:56, Anthony Liguori wrote:
>>> On 01/31/2012 08:51 AM, Jan Kiszka wrote:
>>>> On 2012-01-31 15:47, Anthony Liguori wrote:
>>>>> On 01/31/2012 08:34 AM, Jan Kiszka wrote:
>>>>>> On 2012-01-26 20:00, Anthony Liguori wrote:
>>>>>>> @@ -548,6 +550,13 @@ static int piix3_realize(PCIDevice *dev)
>>>>>>> /* Setup the RTC IRQ */
>>>>>>> s->rtc.irq = rtc_irq;
>>>>>>>
>>>>>>> + /* Realize the PIT */
>>>>>>> + qdev_set_parent_bus(DEVICE(&s->pit), BUS(s->bus));
>>>>>>> + qdev_init_nofail(DEVICE(&s->pit));
>>>>>>> +
>>>>>>> + /* FIXME this should be refactored */
>>>>>>> + pcspk_init(ISA_DEVICE(&s->pit));
>>>>>>
>>>>>> Fixing ATM, ie. converting to qdev/QOM.
>>>>>>
>>>>>> Q: How do I use qdev_property_add_link& Co. to establish the relation
>>>>>> from the speaker port device to the PIT?
>>>>>
>>>>> In the state structure, have:
>>>>>
>>>>> struct PCSpkState {
>>>>> ...
>>>>> PITState *pit;
>>>>> };
>>>>>
>>>>> In the pcspk instance_init, do:
>>>>>
>>>>> object_property_add_link(obj, "pit", TYPE_PIT, (Object **)&s->pit, NULL);
>>>>>
>>>>> In the pcspk realize function (DeviceClass::init), do:
>>>>>
>>>>> assert(s->pit != NULL); // make sure the pit link is set
>>>>>
>>>>> And that's it.
>>>>>
>>>>> You can set the s->pit field directly. You are not required to use any special
>>>>> QOM function to interact with link properties.
>>>>>
>>>>> BTW, this is yet another benefit of making structures public. You can take the
>>>>> address of a child and set link fields directly without accessors.
>>>>
>>>> Well, that has two sides. We introduced properties to avoid this direct
>>>> messing.
>>>>
>>>> Does linking also work without exposing internals?
>>>
>>> Yes, you can set links through properties (although I haven't added those
>>> accessors yet).
>>>
>>> But... you lose type safety because now you're dealing with strings.
>>
>> I don't get yet why we have to give up on type safety here. Isn't all
>> information stored in the property entry? Can't some
>> object_set_property() service take the object pointer and validate its
>> type before writing at the target location?
>
> Already does that. You'll get a run time warning.
Fine.
>
>> I'm not worried about
>> lacking compile-time checks if we keep them for runtime.
>
> I'm worried about:
>
> object_property_set_link(obj, "pci", OBJECT(&s->pic));
>
> vs:
>
> spk->pic = s->pic;
>
> Or:
>
> pcspk_set_pic(spk, &s->pic);
>
> I tend to make a lot of typos, I like the compiler to catch them for me at build
> time.
Well, but this enforces clean interfaces - just postpones the check.
Nothing can be set in the foreign device state outside its core.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [PATCH 06/15] piix: create i8254 through composition
2012-01-31 14:51 ` Jan Kiszka
2012-01-31 14:56 ` Anthony Liguori
@ 2012-01-31 14:58 ` Paolo Bonzini
2012-01-31 16:04 ` Jan Kiszka
2012-01-31 16:12 ` Anthony Liguori
1 sibling, 2 replies; 55+ messages in thread
From: Paolo Bonzini @ 2012-01-31 14:58 UTC (permalink / raw)
To: Jan Kiszka
Cc: Peter Maydell, Anthony Liguori, Markus Armbruster,
qemu-devel@nongnu.org, Avi Kivity
On 01/31/2012 03:51 PM, Jan Kiszka wrote:
>> >
>> > BTW, this is yet another benefit of making structures public. You can take the
>> > address of a child and set link fields directly without accessors.
> Well, that has two sides. We introduced properties to avoid this direct
> messing.
>
> Does linking also work without exposing internals?
Perhaps it doesn't need to expose internals. Just like we create
interfaces automatically when creating a parent object, perhaps we can
create children as well, like
TypeInfo type_piix3 = {
...
.children = {
{ "pic[0]", TYPE_I8259, offsetof(PIIX3, pic[0]) },
{ "pic[1]", TYPE_I8259, offsetof(PIIX3, pic[1]) },
{ "pit, TYPE_I8254, offsetof(PIIX3, pit) },
{ "rtc", TYPE_RTC, offsetof(PIIX3, rtc) },
{ }
}
}
QOM's object_init would allocate a single malloced block for all of
them, carve out space for the parent and all children, and add the
properties.
Paolo
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [PATCH 06/15] piix: create i8254 through composition
2012-01-31 14:58 ` Paolo Bonzini
@ 2012-01-31 16:04 ` Jan Kiszka
2012-01-31 16:12 ` Anthony Liguori
1 sibling, 0 replies; 55+ messages in thread
From: Jan Kiszka @ 2012-01-31 16:04 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Peter Maydell, Anthony Liguori, Markus Armbruster,
qemu-devel@nongnu.org, Avi Kivity
On 2012-01-31 15:58, Paolo Bonzini wrote:
> On 01/31/2012 03:51 PM, Jan Kiszka wrote:
>>>>
>>>> BTW, this is yet another benefit of making structures public. You can take the
>>>> address of a child and set link fields directly without accessors.
>> Well, that has two sides. We introduced properties to avoid this direct
>> messing.
>>
>> Does linking also work without exposing internals?
>
> Perhaps it doesn't need to expose internals. Just like we create
> interfaces automatically when creating a parent object, perhaps we can
> create children as well, like
>
> TypeInfo type_piix3 = {
> ...
> .children = {
> { "pic[0]", TYPE_I8259, offsetof(PIIX3, pic[0]) },
> { "pic[1]", TYPE_I8259, offsetof(PIIX3, pic[1]) },
> { "pit, TYPE_I8254, offsetof(PIIX3, pit) },
> { "rtc", TYPE_RTC, offsetof(PIIX3, rtc) },
> { }
> }
and .links = ...
Yep, such a thing looks better. I suppose that also opens the door for
(runtime) type checking, right?
> }
>
> QOM's object_init would allocate a single malloced block for all of
> them, carve out space for the parent and all children, and add the
> properties.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [PATCH 06/15] piix: create i8254 through composition
2012-01-31 14:58 ` Paolo Bonzini
2012-01-31 16:04 ` Jan Kiszka
@ 2012-01-31 16:12 ` Anthony Liguori
2012-01-31 16:19 ` Jan Kiszka
1 sibling, 1 reply; 55+ messages in thread
From: Anthony Liguori @ 2012-01-31 16:12 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Peter Maydell, Anthony Liguori, Jan Kiszka, qemu-devel@nongnu.org,
Markus Armbruster, Avi Kivity
On 01/31/2012 08:58 AM, Paolo Bonzini wrote:
> On 01/31/2012 03:51 PM, Jan Kiszka wrote:
>>> >
>>> > BTW, this is yet another benefit of making structures public. You can take the
>>> > address of a child and set link fields directly without accessors.
>> Well, that has two sides. We introduced properties to avoid this direct
>> messing.
>>
>> Does linking also work without exposing internals?
>
> Perhaps it doesn't need to expose internals. Just like we create interfaces
> automatically when creating a parent object, perhaps we can create children as
> well, like
>
> TypeInfo type_piix3 = {
> ...
> .children = {
> { "pic[0]", TYPE_I8259, offsetof(PIIX3, pic[0]) },
> { "pic[1]", TYPE_I8259, offsetof(PIIX3, pic[1]) },
> { "pit, TYPE_I8254, offsetof(PIIX3, pit) },
> { "rtc", TYPE_RTC, offsetof(PIIX3, rtc) },
Eeek. I absolutely want to avoid any offset based interfaces.
You can just as well do:
void object_property_add_child(Object *obj, const char *name,
const char *type, Object **child);
It could then do:
*child = object_new(type);
Regards,
Anthony Liguori
> { }
> }
> }
>
> QOM's object_init would allocate a single malloced block for all of them, carve
> out space for the parent and all children, and add the properties.
>
> Paolo
>
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [PATCH 06/15] piix: create i8254 through composition
2012-01-31 16:12 ` Anthony Liguori
@ 2012-01-31 16:19 ` Jan Kiszka
2012-01-31 16:47 ` Anthony Liguori
0 siblings, 1 reply; 55+ messages in thread
From: Jan Kiszka @ 2012-01-31 16:19 UTC (permalink / raw)
To: Anthony Liguori
Cc: Peter Maydell, Anthony Liguori, qemu-devel@nongnu.org,
Markus Armbruster, Avi Kivity, Paolo Bonzini
On 2012-01-31 17:12, Anthony Liguori wrote:
> On 01/31/2012 08:58 AM, Paolo Bonzini wrote:
>> On 01/31/2012 03:51 PM, Jan Kiszka wrote:
>>>>>
>>>>> BTW, this is yet another benefit of making structures public. You can take the
>>>>> address of a child and set link fields directly without accessors.
>>> Well, that has two sides. We introduced properties to avoid this direct
>>> messing.
>>>
>>> Does linking also work without exposing internals?
>>
>> Perhaps it doesn't need to expose internals. Just like we create interfaces
>> automatically when creating a parent object, perhaps we can create children as
>> well, like
>>
>> TypeInfo type_piix3 = {
>> ...
>> .children = {
>> { "pic[0]", TYPE_I8259, offsetof(PIIX3, pic[0]) },
>> { "pic[1]", TYPE_I8259, offsetof(PIIX3, pic[1]) },
>> { "pit, TYPE_I8254, offsetof(PIIX3, pit) },
>> { "rtc", TYPE_RTC, offsetof(PIIX3, rtc) },
>
>
> Eeek. I absolutely want to avoid any offset based interfaces.
Why?
>
> You can just as well do:
>
> void object_property_add_child(Object *obj, const char *name,
> const char *type, Object **child);
>
> It could then do:
>
> *child = object_new(type);
How does this resolve where the link is stored in the opaque state
structure?
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [PATCH 06/15] piix: create i8254 through composition
2012-01-31 16:19 ` Jan Kiszka
@ 2012-01-31 16:47 ` Anthony Liguori
2012-01-31 16:59 ` Paolo Bonzini
0 siblings, 1 reply; 55+ messages in thread
From: Anthony Liguori @ 2012-01-31 16:47 UTC (permalink / raw)
To: Jan Kiszka
Cc: Peter Maydell, Anthony Liguori, qemu-devel@nongnu.org,
Markus Armbruster, Avi Kivity, Paolo Bonzini
On 01/31/2012 10:19 AM, Jan Kiszka wrote:
> On 2012-01-31 17:12, Anthony Liguori wrote:
>> On 01/31/2012 08:58 AM, Paolo Bonzini wrote:
>>> On 01/31/2012 03:51 PM, Jan Kiszka wrote:
>>>>>>
>>>>>> BTW, this is yet another benefit of making structures public. You can take the
>>>>>> address of a child and set link fields directly without accessors.
>>>> Well, that has two sides. We introduced properties to avoid this direct
>>>> messing.
>>>>
>>>> Does linking also work without exposing internals?
>>>
>>> Perhaps it doesn't need to expose internals. Just like we create interfaces
>>> automatically when creating a parent object, perhaps we can create children as
>>> well, like
>>>
>>> TypeInfo type_piix3 = {
>>> ...
>>> .children = {
>>> { "pic[0]", TYPE_I8259, offsetof(PIIX3, pic[0]) },
>>> { "pic[1]", TYPE_I8259, offsetof(PIIX3, pic[1]) },
>>> { "pit, TYPE_I8254, offsetof(PIIX3, pit) },
>>> { "rtc", TYPE_RTC, offsetof(PIIX3, rtc) },
>>
>>
>> Eeek. I absolutely want to avoid any offset based interfaces.
>
> Why?
static void object_initialize(Object *obj)
{
PIIX3State *s = PIIX3(obj);
ChildProperty props[] = {
{ "pic[0]", TYPE_I8259, &s->pic[0] },
{ "pic[1]", TYPE_I8259, &s->pic[1] },
};
object_property_add_children(obj, props, ARRAY_SIZE(props));
}
Is much nicer IMHO.
>
>>
>> You can just as well do:
>>
>> void object_property_add_child(Object *obj, const char *name,
>> const char *type, Object **child);
>>
>> It could then do:
>>
>> *child = object_new(type);
>
> How does this resolve where the link is stored in the opaque state
> structure?
See above. Don't use an offset based interface and it's not a problem.
Regards,
Anthony Liguori
>
> Jan
>
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [PATCH 06/15] piix: create i8254 through composition
2012-01-31 16:47 ` Anthony Liguori
@ 2012-01-31 16:59 ` Paolo Bonzini
0 siblings, 0 replies; 55+ messages in thread
From: Paolo Bonzini @ 2012-01-31 16:59 UTC (permalink / raw)
To: Anthony Liguori
Cc: Peter Maydell, Anthony Liguori, Jan Kiszka, qemu-devel@nongnu.org,
Markus Armbruster, Avi Kivity
On 01/31/2012 05:47 PM, Anthony Liguori wrote:
>>
>
> static void object_initialize(Object *obj)
> {
> PIIX3State *s = PIIX3(obj);
> ChildProperty props[] = {
> { "pic[0]", TYPE_I8259, &s->pic[0] },
> { "pic[1]", TYPE_I8259, &s->pic[1] },
> };
>
> object_property_add_children(obj, props, ARRAY_SIZE(props));
> }
>
> Is much nicer IMHO.
Fine, but then children devices will not use the same memory block as
the parent. Not a huge difference, but worth pointing it out.
Paolo
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] [PATCH 07/15] i440fx: eliminate i440fx_common_init
2012-01-26 19:00 [Qemu-devel] [RFC 00/15] Refactor PC machine to take advantage of QOM Anthony Liguori
` (5 preceding siblings ...)
2012-01-26 19:00 ` [Qemu-devel] [PATCH 06/15] piix: create i8254 " Anthony Liguori
@ 2012-01-26 19:00 ` Anthony Liguori
2012-01-26 19:00 ` [Qemu-devel] [PATCH 08/15] i440fx: introduce some saner naming conventions Anthony Liguori
` (8 subsequent siblings)
15 siblings, 0 replies; 55+ messages in thread
From: Anthony Liguori @ 2012-01-26 19:00 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Anthony Liguori, Jan Kiszka, Markus Armbruster,
Avi Kivity, Paolo Bonzini
---
hw/piix_pci.c | 48 ++++++++++++------------------------------------
1 files changed, 12 insertions(+), 36 deletions(-)
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 4735d9c..589708e 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -263,19 +263,17 @@ static int i440fx_initfn(PCIDevice *dev)
return 0;
}
-static PCIBus *i440fx_common_init(const char *device_name,
- PCII440FXState **pi440fx_state,
- int *piix3_devfn,
- ISABus **isa_bus, qemu_irq *pic,
- MemoryRegion *address_space_mem,
- MemoryRegion *address_space_io,
- ram_addr_t ram_size,
- target_phys_addr_t pci_hole_start,
- target_phys_addr_t pci_hole_size,
- target_phys_addr_t pci_hole64_start,
- target_phys_addr_t pci_hole64_size,
- MemoryRegion *pci_address_space,
- MemoryRegion *ram_memory)
+PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn,
+ ISABus **isa_bus, qemu_irq *pic,
+ MemoryRegion *address_space_mem,
+ MemoryRegion *address_space_io,
+ ram_addr_t ram_size,
+ target_phys_addr_t pci_hole_start,
+ target_phys_addr_t pci_hole_size,
+ target_phys_addr_t pci_hole64_start,
+ target_phys_addr_t pci_hole64_size,
+ MemoryRegion *pci_address_space, MemoryRegion *ram_memory)
+
{
DeviceState *dev;
PCIBus *b;
@@ -293,7 +291,7 @@ static PCIBus *i440fx_common_init(const char *device_name,
qdev_init_nofail(dev);
object_property_add_child(object_get_root(), "i440fx", OBJECT(dev), NULL);
- d = pci_create_simple(b, 0, device_name);
+ d = pci_create_simple(b, 0, "i440FX");
*pi440fx_state = DO_UPCAST(PCII440FXState, dev, d);
f = *pi440fx_state;
f->system_memory = address_space_mem;
@@ -355,28 +353,6 @@ static PCIBus *i440fx_common_init(const char *device_name,
return b;
}
-PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn,
- ISABus **isa_bus, qemu_irq *pic,
- MemoryRegion *address_space_mem,
- MemoryRegion *address_space_io,
- ram_addr_t ram_size,
- target_phys_addr_t pci_hole_start,
- target_phys_addr_t pci_hole_size,
- target_phys_addr_t pci_hole64_start,
- target_phys_addr_t pci_hole64_size,
- MemoryRegion *pci_memory, MemoryRegion *ram_memory)
-
-{
- PCIBus *b;
-
- b = i440fx_common_init("i440FX", pi440fx_state, piix3_devfn, isa_bus, pic,
- address_space_mem, address_space_io, ram_size,
- pci_hole_start, pci_hole_size,
- pci_hole64_size, pci_hole64_size,
- pci_memory, ram_memory);
- return b;
-}
-
/* PIIX3 PCI to ISA bridge */
static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
{
--
1.7.4.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [PATCH 08/15] i440fx: introduce some saner naming conventions
2012-01-26 19:00 [Qemu-devel] [RFC 00/15] Refactor PC machine to take advantage of QOM Anthony Liguori
` (6 preceding siblings ...)
2012-01-26 19:00 ` [Qemu-devel] [PATCH 07/15] i440fx: eliminate i440fx_common_init Anthony Liguori
@ 2012-01-26 19:00 ` Anthony Liguori
2012-01-26 19:00 ` [Qemu-devel] [PATCH 09/15] i440fx: create the PMC through composition Anthony Liguori
` (7 subsequent siblings)
15 siblings, 0 replies; 55+ messages in thread
From: Anthony Liguori @ 2012-01-26 19:00 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Anthony Liguori, Jan Kiszka, Markus Armbruster,
Avi Kivity, Paolo Bonzini
i440fx-pcihost => i440fx
i440fx => i440fx-pmc
It might seem like a small change, but it better reflects the fact that the PMC
is contained within the i440fx which we will now reflect in composition in the
next few changesets.
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
hw/pc.c | 2 +-
hw/pc.h | 6 +-
hw/piix_pci.c | 148 +++++++++++++++++++++++++++++++-------------------------
3 files changed, 86 insertions(+), 70 deletions(-)
diff --git a/hw/pc.c b/hw/pc.c
index 95dd582..b7542b4 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -1285,7 +1285,7 @@ static void pc_init1(MemoryRegion *system_memory,
ram_addr_t below_4g_mem_size, above_4g_mem_size;
PCIBus *pci_bus;
ISABus *isa_bus;
- PCII440FXState *i440fx_state;
+ I440FXPMCState *i440fx_state;
int piix3_devfn = -1;
qemu_irq *cpu_irq;
qemu_irq *gsi;
diff --git a/hw/pc.h b/hw/pc.h
index e269012..0764f55 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -153,10 +153,10 @@ void pcspk_init(ISADevice *pit);
int pcspk_audio_init(ISABus *bus);
/* piix_pci.c */
-struct PCII440FXState;
-typedef struct PCII440FXState PCII440FXState;
+struct I440FXPMCState;
+typedef struct I440FXPMCState I440FXPMCState;
-PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
+PCIBus *i440fx_init(I440FXPMCState **pi440fx_state, int *piix_devfn,
ISABus **isa_bus, qemu_irq *pic,
MemoryRegion *address_space_mem,
MemoryRegion *address_space_io,
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 589708e..aca4476 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -37,9 +37,43 @@
/*
* I440FX chipset data sheet.
* http://download.intel.com/design/chipsets/datashts/29054901.pdf
+ *
+ * The I440FX is a package that contains an integrated PCI Host controller,
+ * memory controller, and is usually packaged with a PCI-ISA bus and super I/O
+ * chipset.
+ *
+ * The "i440FX" device is the PCI host controller. On function 0.0, there is a
+ * memory controller called the Programmable Memory Controller (PMC). On
+ * function 1.0, there is the PCI-ISA bus/super I/O chip called the PIIX3.
*/
-typedef PCIHostState I440FXState;
+typedef struct I440FXState
+{
+ SysBusDevice busdev;
+ MemoryRegion conf_mem;
+ MemoryRegion data_mem;
+ MemoryRegion mmcfg;
+ MemoryRegion *address_space;
+ uint32_t config_reg;
+ PCIBus *bus;
+} I440FXState;
+
+typedef struct PAMMemoryRegion {
+ MemoryRegion mem;
+ bool initialized;
+} PAMMemoryRegion;
+
+struct I440FXPMCState {
+ PCIDevice dev;
+ MemoryRegion *system_memory;
+ MemoryRegion *pci_address_space;
+ MemoryRegion *ram_memory;
+ MemoryRegion pci_hole;
+ MemoryRegion pci_hole_64bit;
+ PAMMemoryRegion pam_regions[13];
+ MemoryRegion smram_region;
+ uint8_t smm_enabled;
+};
#define PIIX_NUM_PIC_IRQS 16 /* i8259 * 2 */
#define PIIX_NUM_PIRQS 4ULL /* PIRQ[A-D] */
@@ -80,24 +114,6 @@ typedef struct PIIX3State {
int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
} PIIX3State;
-typedef struct PAMMemoryRegion {
- MemoryRegion mem;
- bool initialized;
-} PAMMemoryRegion;
-
-struct PCII440FXState {
- PCIDevice dev;
- MemoryRegion *system_memory;
- MemoryRegion *pci_address_space;
- MemoryRegion *ram_memory;
- MemoryRegion pci_hole;
- MemoryRegion pci_hole_64bit;
- PAMMemoryRegion pam_regions[13];
- MemoryRegion smram_region;
- uint8_t smm_enabled;
-};
-
-
#define I440FX_PAM 0x59
#define I440FX_PAM_SIZE 7
#define I440FX_SMRAM 0x72
@@ -116,7 +132,7 @@ static int pci_slot_get_pirq(PCIDevice *pci_dev, int pci_intx)
return (pci_intx + slot_addend) & 3;
}
-static void update_pam(PCII440FXState *d, uint32_t start, uint32_t end, int r,
+static void update_pam(I440FXPMCState *d, uint32_t start, uint32_t end, int r,
PAMMemoryRegion *mem)
{
if (mem->initialized) {
@@ -149,7 +165,7 @@ static void update_pam(PCII440FXState *d, uint32_t start, uint32_t end, int r,
mem->initialized = true;
}
-static void i440fx_update_memory_mappings(PCII440FXState *d)
+static void i440fx_pmc_update_memory_mappings(I440FXPMCState *d)
{
int i, r;
uint32_t smram;
@@ -169,40 +185,40 @@ static void i440fx_update_memory_mappings(PCII440FXState *d)
memory_region_transaction_commit();
}
-static void i440fx_set_smm(int val, void *arg)
+static void i440fx_pmc_set_smm(int val, void *arg)
{
- PCII440FXState *d = arg;
+ I440FXPMCState *d = arg;
val = (val != 0);
if (d->smm_enabled != val) {
d->smm_enabled = val;
- i440fx_update_memory_mappings(d);
+ i440fx_pmc_update_memory_mappings(d);
}
}
-static void i440fx_write_config(PCIDevice *dev,
- uint32_t address, uint32_t val, int len)
+static void i440fx_pmc_write_config(PCIDevice *dev,
+ uint32_t address, uint32_t val, int len)
{
- PCII440FXState *d = DO_UPCAST(PCII440FXState, dev, dev);
+ I440FXPMCState *d = DO_UPCAST(I440FXPMCState, dev, dev);
/* XXX: implement SMRAM.D_LOCK */
pci_default_write_config(dev, address, val, len);
if (ranges_overlap(address, len, I440FX_PAM, I440FX_PAM_SIZE) ||
range_covers_byte(address, len, I440FX_SMRAM)) {
- i440fx_update_memory_mappings(d);
+ i440fx_pmc_update_memory_mappings(d);
}
}
-static int i440fx_load_old(QEMUFile* f, void *opaque, int version_id)
+static int i440fx_pmc_load_old(QEMUFile* f, void *opaque, int version_id)
{
- PCII440FXState *d = opaque;
+ I440FXPMCState *d = opaque;
int ret, i;
ret = pci_device_load(&d->dev, f);
if (ret < 0)
return ret;
- i440fx_update_memory_mappings(d);
+ i440fx_pmc_update_memory_mappings(d);
qemu_get_8s(f, &d->smm_enabled);
if (version_id == 2) {
@@ -214,29 +230,29 @@ static int i440fx_load_old(QEMUFile* f, void *opaque, int version_id)
return 0;
}
-static int i440fx_post_load(void *opaque, int version_id)
+static int i440fx_pmc_post_load(void *opaque, int version_id)
{
- PCII440FXState *d = opaque;
+ I440FXPMCState *d = opaque;
- i440fx_update_memory_mappings(d);
+ i440fx_pmc_update_memory_mappings(d);
return 0;
}
-static const VMStateDescription vmstate_i440fx = {
- .name = "I440FX",
+static const VMStateDescription vmstate_i440fx_pmc = {
+ .name = "I440FX", /* this is wrong but we can't change it */
.version_id = 3,
.minimum_version_id = 3,
.minimum_version_id_old = 1,
- .load_state_old = i440fx_load_old,
- .post_load = i440fx_post_load,
+ .load_state_old = i440fx_pmc_load_old,
+ .post_load = i440fx_pmc_post_load,
.fields = (VMStateField []) {
- VMSTATE_PCI_DEVICE(dev, PCII440FXState),
- VMSTATE_UINT8(smm_enabled, PCII440FXState),
+ VMSTATE_PCI_DEVICE(dev, I440FXPMCState),
+ VMSTATE_UINT8(smm_enabled, I440FXPMCState),
VMSTATE_END_OF_LIST()
}
};
-static int i440fx_pcihost_initfn(SysBusDevice *dev)
+static int i440fx_initfn(SysBusDevice *dev)
{
I440FXState *s = FROM_SYSBUS(I440FXState, dev);
@@ -253,17 +269,17 @@ static int i440fx_pcihost_initfn(SysBusDevice *dev)
return 0;
}
-static int i440fx_initfn(PCIDevice *dev)
+static int i440fx_pmc_initfn(PCIDevice *dev)
{
- PCII440FXState *d = DO_UPCAST(PCII440FXState, dev, dev);
+ I440FXPMCState *d = DO_UPCAST(I440FXPMCState, dev, dev);
d->dev.config[I440FX_SMRAM] = 0x02;
- cpu_smm_register(&i440fx_set_smm, d);
+ cpu_smm_register(&i440fx_pmc_set_smm, d);
return 0;
}
-PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn,
+PCIBus *i440fx_init(I440FXPMCState **pi440fx_state, int *piix3_devfn,
ISABus **isa_bus, qemu_irq *pic,
MemoryRegion *address_space_mem,
MemoryRegion *address_space_io,
@@ -280,9 +296,9 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn,
PCIDevice *d;
I440FXState *s;
PIIX3State *piix3;
- PCII440FXState *f;
+ I440FXPMCState *f;
- dev = qdev_create(NULL, "i440FX-pcihost");
+ dev = qdev_create(NULL, "i440FX");
s = FROM_SYSBUS(I440FXState, sysbus_from_qdev(dev));
s->address_space = address_space_mem;
b = pci_bus_new(&s->busdev.qdev, NULL, pci_address_space,
@@ -291,8 +307,8 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn,
qdev_init_nofail(dev);
object_property_add_child(object_get_root(), "i440fx", OBJECT(dev), NULL);
- d = pci_create_simple(b, 0, "i440FX");
- *pi440fx_state = DO_UPCAST(PCII440FXState, dev, d);
+ d = pci_create_simple(b, 0, "i440FX-PMC");
+ *pi440fx_state = DO_UPCAST(I440FXPMCState, dev, d);
f = *pi440fx_state;
f->system_memory = address_space_mem;
f->pci_address_space = pci_address_space;
@@ -348,7 +364,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn,
ram_size = 255;
(*pi440fx_state)->dev.config[0x57]=ram_size;
- i440fx_update_memory_mappings(f);
+ i440fx_pmc_update_memory_mappings(f);
return b;
}
@@ -592,52 +608,52 @@ static TypeInfo piix3_xen_info = {
.class_init = piix3_xen_class_init,
};
-static void i440fx_class_init(ObjectClass *klass, void *data)
+static void i440fx_pmc_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
k->no_hotplug = 1;
- k->init = i440fx_initfn;
- k->config_write = i440fx_write_config;
+ k->init = i440fx_pmc_initfn;
+ k->config_write = i440fx_pmc_write_config;
k->vendor_id = PCI_VENDOR_ID_INTEL;
k->device_id = PCI_DEVICE_ID_INTEL_82441;
k->revision = 0x02;
k->class_id = PCI_CLASS_BRIDGE_HOST;
dc->desc = "Host bridge";
dc->no_user = 1;
- dc->vmsd = &vmstate_i440fx;
+ dc->vmsd = &vmstate_i440fx_pmc;
}
-static TypeInfo i440fx_info = {
- .name = "i440FX",
+static TypeInfo i440fx_pmc_info = {
+ .name = "i440FX-PMC",
.parent = TYPE_PCI_DEVICE,
- .instance_size = sizeof(PCII440FXState),
- .class_init = i440fx_class_init,
+ .instance_size = sizeof(I440FXPMCState),
+ .class_init = i440fx_pmc_class_init,
};
-static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
+static void i440fx_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
- k->init = i440fx_pcihost_initfn;
+ k->init = i440fx_initfn;
dc->fw_name = "pci";
dc->no_user = 1;
}
-static TypeInfo i440fx_pcihost_info = {
- .name = "i440FX-pcihost",
+static TypeInfo i440fx_info = {
+ .name = "i440FX",
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(I440FXState),
- .class_init = i440fx_pcihost_class_init,
+ .class_init = i440fx_class_init,
};
-static void i440fx_register(void)
+static void register_devices(void)
{
type_register_static(&i440fx_info);
+ type_register_static(&i440fx_pmc_info);
type_register_static(&piix3_info);
type_register_static(&piix3_xen_info);
- type_register_static(&i440fx_pcihost_info);
}
-device_init(i440fx_register);
+device_init(register_devices);
--
1.7.4.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [PATCH 09/15] i440fx: create the PMC through composition
2012-01-26 19:00 [Qemu-devel] [RFC 00/15] Refactor PC machine to take advantage of QOM Anthony Liguori
` (7 preceding siblings ...)
2012-01-26 19:00 ` [Qemu-devel] [PATCH 08/15] i440fx: introduce some saner naming conventions Anthony Liguori
@ 2012-01-26 19:00 ` Anthony Liguori
2012-01-26 19:00 ` [Qemu-devel] [PATCH 10/15] i440fx: move some logic to realize and make inheritance from PCIHost explicit Anthony Liguori
` (6 subsequent siblings)
15 siblings, 0 replies; 55+ messages in thread
From: Anthony Liguori @ 2012-01-26 19:00 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Anthony Liguori, Jan Kiszka, Markus Armbruster,
Avi Kivity, Paolo Bonzini
---
hw/piix_pci.c | 122 ++++++++++++++++++++++++++++++++-------------------------
1 files changed, 69 insertions(+), 53 deletions(-)
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index aca4476..fdb372f 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -47,34 +47,6 @@
* function 1.0, there is the PCI-ISA bus/super I/O chip called the PIIX3.
*/
-typedef struct I440FXState
-{
- SysBusDevice busdev;
- MemoryRegion conf_mem;
- MemoryRegion data_mem;
- MemoryRegion mmcfg;
- MemoryRegion *address_space;
- uint32_t config_reg;
- PCIBus *bus;
-} I440FXState;
-
-typedef struct PAMMemoryRegion {
- MemoryRegion mem;
- bool initialized;
-} PAMMemoryRegion;
-
-struct I440FXPMCState {
- PCIDevice dev;
- MemoryRegion *system_memory;
- MemoryRegion *pci_address_space;
- MemoryRegion *ram_memory;
- MemoryRegion pci_hole;
- MemoryRegion pci_hole_64bit;
- PAMMemoryRegion pam_regions[13];
- MemoryRegion smram_region;
- uint8_t smm_enabled;
-};
-
#define PIIX_NUM_PIC_IRQS 16 /* i8259 * 2 */
#define PIIX_NUM_PIRQS 4ULL /* PIRQ[A-D] */
#define XEN_PIIX_NUM_PIRQS 128ULL
@@ -114,6 +86,41 @@ typedef struct PIIX3State {
int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
} PIIX3State;
+#define TYPE_I440FX_PMC "i440FX-PMC"
+#define I440FX_PMC(obj) OBJECT_CHECK(I440FXPMCState, (obj), TYPE_I440FX_PMC)
+
+typedef struct PAMMemoryRegion {
+ MemoryRegion mem;
+ bool initialized;
+} PAMMemoryRegion;
+
+struct I440FXPMCState {
+ PCIDevice dev;
+ MemoryRegion *system_memory;
+ MemoryRegion *pci_address_space;
+ MemoryRegion *ram_memory;
+ MemoryRegion pci_hole;
+ MemoryRegion pci_hole_64bit;
+ PAMMemoryRegion pam_regions[13];
+ MemoryRegion smram_region;
+ uint8_t smm_enabled;
+};
+
+#define TYPE_I440FX "i440FX"
+#define I440FX(obj) OBJECT_CHECK(I440FXState, (obj), TYPE_I440FX)
+
+typedef struct I440FXState
+{
+ SysBusDevice busdev;
+ MemoryRegion conf_mem;
+ MemoryRegion data_mem;
+ MemoryRegion mmcfg;
+ MemoryRegion *address_space;
+ uint32_t config_reg;
+ PCIBus *bus;
+ I440FXPMCState pmc;
+} I440FXState;
+
#define I440FX_PAM 0x59
#define I440FX_PAM_SIZE 7
#define I440FX_SMRAM 0x72
@@ -252,7 +259,7 @@ static const VMStateDescription vmstate_i440fx_pmc = {
}
};
-static int i440fx_initfn(SysBusDevice *dev)
+static int i440fx_realize(SysBusDevice *dev)
{
I440FXState *s = FROM_SYSBUS(I440FXState, dev);
@@ -269,6 +276,15 @@ static int i440fx_initfn(SysBusDevice *dev)
return 0;
}
+static void i440fx_initfn(Object *obj)
+{
+ I440FXState *s = I440FX(obj);
+
+ object_initialize(&s->pmc, TYPE_I440FX_PMC);
+ object_property_add_child(obj, "pmc", OBJECT(&s->pmc), NULL);
+ qdev_prop_set_uint32(DEVICE(&s->pmc), "addr", PCI_DEVFN(0, 0));
+}
+
static int i440fx_pmc_initfn(PCIDevice *dev)
{
I440FXPMCState *d = DO_UPCAST(I440FXPMCState, dev, dev);
@@ -293,12 +309,10 @@ PCIBus *i440fx_init(I440FXPMCState **pi440fx_state, int *piix3_devfn,
{
DeviceState *dev;
PCIBus *b;
- PCIDevice *d;
I440FXState *s;
PIIX3State *piix3;
- I440FXPMCState *f;
- dev = qdev_create(NULL, "i440FX");
+ dev = qdev_create(NULL, TYPE_I440FX);
s = FROM_SYSBUS(I440FXState, sysbus_from_qdev(dev));
s->address_space = address_space_mem;
b = pci_bus_new(&s->busdev.qdev, NULL, pci_address_space,
@@ -307,27 +321,28 @@ PCIBus *i440fx_init(I440FXPMCState **pi440fx_state, int *piix3_devfn,
qdev_init_nofail(dev);
object_property_add_child(object_get_root(), "i440fx", OBJECT(dev), NULL);
- d = pci_create_simple(b, 0, "i440FX-PMC");
- *pi440fx_state = DO_UPCAST(I440FXPMCState, dev, d);
- f = *pi440fx_state;
- f->system_memory = address_space_mem;
- f->pci_address_space = pci_address_space;
- f->ram_memory = ram_memory;
- memory_region_init_alias(&f->pci_hole, "pci-hole", f->pci_address_space,
+ qdev_set_parent_bus(DEVICE(&s->pmc), BUS(s->bus));
+ qdev_init_nofail(DEVICE(&s->pmc));
+
+ *pi440fx_state = &s->pmc;
+ s->pmc.system_memory = address_space_mem;
+ s->pmc.pci_address_space = pci_address_space;
+ s->pmc.ram_memory = ram_memory;
+ memory_region_init_alias(&s->pmc.pci_hole, "pci-hole", s->pmc.pci_address_space,
pci_hole_start, pci_hole_size);
- memory_region_add_subregion(f->system_memory, pci_hole_start, &f->pci_hole);
- memory_region_init_alias(&f->pci_hole_64bit, "pci-hole64",
- f->pci_address_space,
+ memory_region_add_subregion(s->pmc.system_memory, pci_hole_start, &s->pmc.pci_hole);
+ memory_region_init_alias(&s->pmc.pci_hole_64bit, "pci-hole64",
+ s->pmc.pci_address_space,
pci_hole64_start, pci_hole64_size);
if (pci_hole64_size) {
- memory_region_add_subregion(f->system_memory, pci_hole64_start,
- &f->pci_hole_64bit);
+ memory_region_add_subregion(s->pmc.system_memory, pci_hole64_start,
+ &s->pmc.pci_hole_64bit);
}
- memory_region_init_alias(&f->smram_region, "smram-region",
- f->pci_address_space, 0xa0000, 0x20000);
- memory_region_add_subregion_overlap(f->system_memory, 0xa0000,
- &f->smram_region, 1);
- memory_region_set_enabled(&f->smram_region, false);
+ memory_region_init_alias(&s->pmc.smram_region, "smram-region",
+ s->pmc.pci_address_space, 0xa0000, 0x20000);
+ memory_region_add_subregion_overlap(s->pmc.system_memory, 0xa0000,
+ &s->pmc.smram_region, 1);
+ memory_region_set_enabled(&s->pmc.smram_region, false);
/* Xen supports additional interrupt routes from the PCI devices to
* the IOAPIC: the four pins of each PCI device on the bus are also
@@ -364,7 +379,7 @@ PCIBus *i440fx_init(I440FXPMCState **pi440fx_state, int *piix3_devfn,
ram_size = 255;
(*pi440fx_state)->dev.config[0x57]=ram_size;
- i440fx_pmc_update_memory_mappings(f);
+ i440fx_pmc_update_memory_mappings(&s->pmc);
return b;
}
@@ -626,7 +641,7 @@ static void i440fx_pmc_class_init(ObjectClass *klass, void *data)
}
static TypeInfo i440fx_pmc_info = {
- .name = "i440FX-PMC",
+ .name = TYPE_I440FX_PMC,
.parent = TYPE_PCI_DEVICE,
.instance_size = sizeof(I440FXPMCState),
.class_init = i440fx_pmc_class_init,
@@ -637,15 +652,16 @@ static void i440fx_class_init(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
- k->init = i440fx_initfn;
+ k->init = i440fx_realize;
dc->fw_name = "pci";
dc->no_user = 1;
}
static TypeInfo i440fx_info = {
- .name = "i440FX",
+ .name = TYPE_I440FX,
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(I440FXState),
+ .instance_init = i440fx_initfn,
.class_init = i440fx_class_init,
};
--
1.7.4.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [PATCH 10/15] i440fx: move some logic to realize and make inheritance from PCIHost explicit
2012-01-26 19:00 [Qemu-devel] [RFC 00/15] Refactor PC machine to take advantage of QOM Anthony Liguori
` (8 preceding siblings ...)
2012-01-26 19:00 ` [Qemu-devel] [PATCH 09/15] i440fx: create the PMC through composition Anthony Liguori
@ 2012-01-26 19:00 ` Anthony Liguori
2012-01-26 19:00 ` [Qemu-devel] [PATCH 11/15] i440fx-pmc: refactor to take properties for memory geometry Anthony Liguori
` (5 subsequent siblings)
15 siblings, 0 replies; 55+ messages in thread
From: Anthony Liguori @ 2012-01-26 19:00 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Anthony Liguori, Jan Kiszka, Markus Armbruster,
Avi Kivity, Paolo Bonzini
---
hw/pci_host.c | 11 ++++++++
hw/pci_host.h | 3 ++
hw/piix_pci.c | 72 +++++++++++++++++++++++++++++++-------------------------
3 files changed, 54 insertions(+), 32 deletions(-)
diff --git a/hw/pci_host.c b/hw/pci_host.c
index 44c6c20..4c9fc49 100644
--- a/hw/pci_host.c
+++ b/hw/pci_host.c
@@ -162,4 +162,15 @@ const MemoryRegionOps pci_host_data_be_ops = {
.endianness = DEVICE_BIG_ENDIAN,
};
+static TypeInfo pci_host_type = {
+ .name = TYPE_PCI_HOST,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(PCIHostState),
+};
+
+static void register_devices(void)
+{
+ type_register_static(&pci_host_type);
+}
+device_init(register_devices);
diff --git a/hw/pci_host.h b/hw/pci_host.h
index 359e38f..fba9fde 100644
--- a/hw/pci_host.h
+++ b/hw/pci_host.h
@@ -30,6 +30,9 @@
#include "sysbus.h"
+#define TYPE_PCI_HOST "pci-host"
+#define PCI_HOST(obj) OBJECT_CHECK(PCIHostState, (obj), TYPE_PCI_HOST)
+
struct PCIHostState {
SysBusDevice busdev;
MemoryRegion conf_mem;
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index fdb372f..52bfc7a 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -111,13 +111,10 @@ struct I440FXPMCState {
typedef struct I440FXState
{
- SysBusDevice busdev;
- MemoryRegion conf_mem;
- MemoryRegion data_mem;
- MemoryRegion mmcfg;
- MemoryRegion *address_space;
- uint32_t config_reg;
- PCIBus *bus;
+ PCIHostState parent;
+
+ MemoryRegion *address_space_io;
+ MemoryRegion *pci_address_space;
I440FXPMCState pmc;
} I440FXState;
@@ -261,17 +258,28 @@ static const VMStateDescription vmstate_i440fx_pmc = {
static int i440fx_realize(SysBusDevice *dev)
{
- I440FXState *s = FROM_SYSBUS(I440FXState, dev);
+ I440FXState *s = I440FX(dev);
+ PCIHostState *h = PCI_HOST(s);
+
+ g_assert(s->pci_address_space != NULL);
+ g_assert(h->address_space != NULL);
+ g_assert(s->address_space_io != NULL);
- memory_region_init_io(&s->conf_mem, &pci_host_conf_le_ops, s,
+ h->bus = pci_bus_new(DEVICE(s), NULL, s->pci_address_space,
+ s->address_space_io, 0);
+
+ memory_region_init_io(&h->conf_mem, &pci_host_conf_le_ops, s,
"pci-conf-idx", 4);
- sysbus_add_io(dev, 0xcf8, &s->conf_mem);
- sysbus_init_ioports(&s->busdev, 0xcf8, 4);
+ sysbus_add_io(dev, 0xcf8, &h->conf_mem);
+ sysbus_init_ioports(&h->busdev, 0xcf8, 4);
- memory_region_init_io(&s->data_mem, &pci_host_data_le_ops, s,
+ memory_region_init_io(&h->data_mem, &pci_host_data_le_ops, s,
"pci-conf-data", 4);
- sysbus_add_io(dev, 0xcfc, &s->data_mem);
- sysbus_init_ioports(&s->busdev, 0xcfc, 4);
+ sysbus_add_io(dev, 0xcfc, &h->data_mem);
+ sysbus_init_ioports(&h->busdev, 0xcfc, 4);
+
+ qdev_set_parent_bus(DEVICE(&s->pmc), BUS(h->bus));
+ qdev_init_nofail(DEVICE(&s->pmc));
return 0;
}
@@ -307,22 +315,22 @@ PCIBus *i440fx_init(I440FXPMCState **pi440fx_state, int *piix3_devfn,
MemoryRegion *pci_address_space, MemoryRegion *ram_memory)
{
- DeviceState *dev;
- PCIBus *b;
I440FXState *s;
+ PCIHostState *h;
PIIX3State *piix3;
- dev = qdev_create(NULL, TYPE_I440FX);
- s = FROM_SYSBUS(I440FXState, sysbus_from_qdev(dev));
- s->address_space = address_space_mem;
- b = pci_bus_new(&s->busdev.qdev, NULL, pci_address_space,
- address_space_io, 0);
- s->bus = b;
- qdev_init_nofail(dev);
- object_property_add_child(object_get_root(), "i440fx", OBJECT(dev), NULL);
+ s = I440FX(object_new(TYPE_I440FX));
+ h = PCI_HOST(s);
- qdev_set_parent_bus(DEVICE(&s->pmc), BUS(s->bus));
- qdev_init_nofail(DEVICE(&s->pmc));
+ /* FIXME make a properties */
+ h->address_space = address_space_mem;
+ s->address_space_io = address_space_io;
+ s->pci_address_space = pci_address_space;
+
+ qdev_set_parent_bus(DEVICE(s), sysbus_get_default());
+ qdev_init_nofail(DEVICE(s));
+
+ object_property_add_child(object_get_root(), "i440fx", OBJECT(s), NULL);
*pi440fx_state = &s->pmc;
s->pmc.system_memory = address_space_mem;
@@ -358,18 +366,18 @@ PCIBus *i440fx_init(I440FXPMCState **pi440fx_state, int *piix3_devfn,
piix3->pic = pic;
qdev_prop_set_uint32(DEVICE(piix3), "addr", PCI_DEVFN(1, 0));
qdev_prop_set_bit(DEVICE(piix3), "multifunction", true);
- qdev_set_parent_bus(DEVICE(piix3), BUS(s->bus));
+ qdev_set_parent_bus(DEVICE(piix3), BUS(h->bus));
qdev_init_nofail(DEVICE(piix3));
if (xen_enabled()) {
- pci_bus_irqs(b, xen_piix3_set_irq, xen_pci_slot_get_pirq,
+ pci_bus_irqs(h->bus, xen_piix3_set_irq, xen_pci_slot_get_pirq,
piix3, XEN_PIIX_NUM_PIRQS);
} else {
- pci_bus_irqs(b, piix3_set_irq, pci_slot_get_pirq, piix3,
+ pci_bus_irqs(h->bus, piix3_set_irq, pci_slot_get_pirq, piix3,
PIIX_NUM_PIRQS);
}
- object_property_add_child(OBJECT(dev), "piix3", OBJECT(piix3), NULL);
+ object_property_add_child(OBJECT(s), "piix3", OBJECT(piix3), NULL);
*isa_bus = piix3->bus;
*piix3_devfn = piix3->dev.devfn;
@@ -381,7 +389,7 @@ PCIBus *i440fx_init(I440FXPMCState **pi440fx_state, int *piix3_devfn,
i440fx_pmc_update_memory_mappings(&s->pmc);
- return b;
+ return h->bus;
}
/* PIIX3 PCI to ISA bridge */
@@ -659,7 +667,7 @@ static void i440fx_class_init(ObjectClass *klass, void *data)
static TypeInfo i440fx_info = {
.name = TYPE_I440FX,
- .parent = TYPE_SYS_BUS_DEVICE,
+ .parent = TYPE_PCI_HOST,
.instance_size = sizeof(I440FXState),
.instance_init = i440fx_initfn,
.class_init = i440fx_class_init,
--
1.7.4.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [PATCH 11/15] i440fx-pmc: refactor to take properties for memory geometry
2012-01-26 19:00 [Qemu-devel] [RFC 00/15] Refactor PC machine to take advantage of QOM Anthony Liguori
` (9 preceding siblings ...)
2012-01-26 19:00 ` [Qemu-devel] [PATCH 10/15] i440fx: move some logic to realize and make inheritance from PCIHost explicit Anthony Liguori
@ 2012-01-26 19:00 ` Anthony Liguori
2012-01-26 19:00 ` [Qemu-devel] [PATCH 12/15] i440fx-pmc: calculate PCI memory hole directly Anthony Liguori
` (4 subsequent siblings)
15 siblings, 0 replies; 55+ messages in thread
From: Anthony Liguori @ 2012-01-26 19:00 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Anthony Liguori, Jan Kiszka, Markus Armbruster,
Avi Kivity, Paolo Bonzini
---
hw/piix_pci.c | 128 +++++++++++++++++++++++++++++++++------------------------
1 files changed, 74 insertions(+), 54 deletions(-)
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 52bfc7a..dddc95f 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -104,6 +104,11 @@ struct I440FXPMCState {
PAMMemoryRegion pam_regions[13];
MemoryRegion smram_region;
uint8_t smm_enabled;
+ ram_addr_t ram_size;
+ target_phys_addr_t pci_hole_start;
+ target_phys_addr_t pci_hole_size;
+ target_phys_addr_t pci_hole64_start;
+ target_phys_addr_t pci_hole64_size;
};
#define TYPE_I440FX "i440FX"
@@ -115,7 +120,9 @@ typedef struct I440FXState
MemoryRegion *address_space_io;
MemoryRegion *pci_address_space;
+
I440FXPMCState pmc;
+ PIIX3State piix3;
} I440FXState;
#define I440FX_PAM 0x59
@@ -281,6 +288,17 @@ static int i440fx_realize(SysBusDevice *dev)
qdev_set_parent_bus(DEVICE(&s->pmc), BUS(h->bus));
qdev_init_nofail(DEVICE(&s->pmc));
+ qdev_set_parent_bus(DEVICE(&s->piix3), BUS(h->bus));
+ qdev_init_nofail(DEVICE(&s->piix3));
+
+ if (xen_enabled()) {
+ pci_bus_irqs(h->bus, xen_piix3_set_irq, xen_pci_slot_get_pirq,
+ &s->piix3, XEN_PIIX_NUM_PIRQS);
+ } else {
+ pci_bus_irqs(h->bus, piix3_set_irq, pci_slot_get_pirq, &s->piix3,
+ PIIX_NUM_PIRQS);
+ }
+
return 0;
}
@@ -291,11 +309,50 @@ static void i440fx_initfn(Object *obj)
object_initialize(&s->pmc, TYPE_I440FX_PMC);
object_property_add_child(obj, "pmc", OBJECT(&s->pmc), NULL);
qdev_prop_set_uint32(DEVICE(&s->pmc), "addr", PCI_DEVFN(0, 0));
+
+ /* Xen supports additional interrupt routes from the PCI devices to
+ * the IOAPIC: the four pins of each PCI device on the bus are also
+ * connected to the IOAPIC directly.
+ * These additional routes can be discovered through ACPI. */
+ if (xen_enabled()) {
+ object_initialize(&s->piix3, "PIIX3-xen");
+ } else {
+ object_initialize(&s->piix3, "PIIX3");
+ }
+ object_property_add_child(OBJECT(s), "piix3", OBJECT(&s->piix3), NULL);
}
static int i440fx_pmc_initfn(PCIDevice *dev)
{
I440FXPMCState *d = DO_UPCAST(I440FXPMCState, dev, dev);
+ ram_addr_t ram_size;
+
+ g_assert(d->system_memory != NULL);
+ g_assert(d->pci_address_space != NULL);
+ g_assert(d->ram_memory != NULL);
+
+ memory_region_init_alias(&d->pci_hole, "pci-hole", d->pci_address_space,
+ d->pci_hole_start, d->pci_hole_size);
+ memory_region_add_subregion(d->system_memory, d->pci_hole_start, &d->pci_hole);
+ memory_region_init_alias(&d->pci_hole_64bit, "pci-hole64",
+ d->pci_address_space,
+ d->pci_hole64_start, d->pci_hole64_size);
+ if (d->pci_hole64_size) {
+ memory_region_add_subregion(d->system_memory, d->pci_hole64_start,
+ &d->pci_hole_64bit);
+ }
+ memory_region_init_alias(&d->smram_region, "smram-region",
+ d->pci_address_space, 0xa0000, 0x20000);
+ memory_region_add_subregion_overlap(d->system_memory, 0xa0000,
+ &d->smram_region, 1);
+ memory_region_set_enabled(&d->smram_region, false);
+
+ ram_size = d->ram_size / 8 / 1024 / 1024;
+ if (ram_size > 255)
+ ram_size = 255;
+ d->dev.config[0x57] = ram_size;
+
+ i440fx_pmc_update_memory_mappings(d);
d->dev.config[I440FX_SMRAM] = 0x02;
@@ -317,7 +374,6 @@ PCIBus *i440fx_init(I440FXPMCState **pi440fx_state, int *piix3_devfn,
{
I440FXState *s;
PCIHostState *h;
- PIIX3State *piix3;
s = I440FX(object_new(TYPE_I440FX));
h = PCI_HOST(s);
@@ -327,69 +383,30 @@ PCIBus *i440fx_init(I440FXPMCState **pi440fx_state, int *piix3_devfn,
s->address_space_io = address_space_io;
s->pci_address_space = pci_address_space;
- qdev_set_parent_bus(DEVICE(s), sysbus_get_default());
- qdev_init_nofail(DEVICE(s));
-
- object_property_add_child(object_get_root(), "i440fx", OBJECT(s), NULL);
+ s->piix3.pic = pic;
- *pi440fx_state = &s->pmc;
s->pmc.system_memory = address_space_mem;
s->pmc.pci_address_space = pci_address_space;
s->pmc.ram_memory = ram_memory;
- memory_region_init_alias(&s->pmc.pci_hole, "pci-hole", s->pmc.pci_address_space,
- pci_hole_start, pci_hole_size);
- memory_region_add_subregion(s->pmc.system_memory, pci_hole_start, &s->pmc.pci_hole);
- memory_region_init_alias(&s->pmc.pci_hole_64bit, "pci-hole64",
- s->pmc.pci_address_space,
- pci_hole64_start, pci_hole64_size);
- if (pci_hole64_size) {
- memory_region_add_subregion(s->pmc.system_memory, pci_hole64_start,
- &s->pmc.pci_hole_64bit);
- }
- memory_region_init_alias(&s->pmc.smram_region, "smram-region",
- s->pmc.pci_address_space, 0xa0000, 0x20000);
- memory_region_add_subregion_overlap(s->pmc.system_memory, 0xa0000,
- &s->pmc.smram_region, 1);
- memory_region_set_enabled(&s->pmc.smram_region, false);
-
- /* Xen supports additional interrupt routes from the PCI devices to
- * the IOAPIC: the four pins of each PCI device on the bus are also
- * connected to the IOAPIC directly.
- * These additional routes can be discovered through ACPI. */
- if (xen_enabled()) {
- piix3 = PIIX3(object_new("PIIX3-xen"));
- } else {
- piix3 = PIIX3(object_new("PIIX3"));
- }
- /* FIXME make this a property */
- piix3->pic = pic;
- qdev_prop_set_uint32(DEVICE(piix3), "addr", PCI_DEVFN(1, 0));
- qdev_prop_set_bit(DEVICE(piix3), "multifunction", true);
- qdev_set_parent_bus(DEVICE(piix3), BUS(h->bus));
- qdev_init_nofail(DEVICE(piix3));
+ s->pmc.ram_size = ram_size;
- if (xen_enabled()) {
- pci_bus_irqs(h->bus, xen_piix3_set_irq, xen_pci_slot_get_pirq,
- piix3, XEN_PIIX_NUM_PIRQS);
- } else {
- pci_bus_irqs(h->bus, piix3_set_irq, pci_slot_get_pirq, piix3,
- PIIX_NUM_PIRQS);
- }
-
- object_property_add_child(OBJECT(s), "piix3", OBJECT(piix3), NULL);
- *isa_bus = piix3->bus;
+ /* FIXME these should be derived */
+ s->pmc.pci_hole_start = pci_hole_start;
+ s->pmc.pci_hole_size = pci_hole_size;
+ s->pmc.pci_hole64_start = pci_hole64_start;
+ s->pmc.pci_hole64_size = pci_hole64_size;
- *piix3_devfn = piix3->dev.devfn;
+ qdev_set_parent_bus(DEVICE(s), sysbus_get_default());
+ qdev_init_nofail(DEVICE(s));
- ram_size = ram_size / 8 / 1024 / 1024;
- if (ram_size > 255)
- ram_size = 255;
- (*pi440fx_state)->dev.config[0x57]=ram_size;
+ object_property_add_child(object_get_root(), "i440fx", OBJECT(s), NULL);
- i440fx_pmc_update_memory_mappings(&s->pmc);
+ *isa_bus = s->piix3.bus;
+ *pi440fx_state = &s->pmc;
+ *piix3_devfn = s->piix3.dev.devfn;
- return h->bus;
+ return h->bus;
}
/* PIIX3 PCI to ISA bridge */
@@ -579,6 +596,9 @@ static void piix3_initfn(Object *obj)
{
PIIX3State *s = PIIX3(obj);
+ qdev_prop_set_uint32(DEVICE(s), "addr", PCI_DEVFN(1, 0));
+ qdev_prop_set_bit(DEVICE(s), "multifunction", true);
+
object_initialize(&s->hpet, TYPE_HPET);
object_property_add_child(obj, "hpet", OBJECT(&s->hpet), NULL);
--
1.7.4.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [PATCH 12/15] i440fx-pmc: calculate PCI memory hole directly
2012-01-26 19:00 [Qemu-devel] [RFC 00/15] Refactor PC machine to take advantage of QOM Anthony Liguori
` (10 preceding siblings ...)
2012-01-26 19:00 ` [Qemu-devel] [PATCH 11/15] i440fx-pmc: refactor to take properties for memory geometry Anthony Liguori
@ 2012-01-26 19:00 ` Anthony Liguori
2012-01-26 19:00 ` [Qemu-devel] [PATCH 13/15] i440fx: allocate MemoryRegion for pci memory space Anthony Liguori
` (3 subsequent siblings)
15 siblings, 0 replies; 55+ messages in thread
From: Anthony Liguori @ 2012-01-26 19:00 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Anthony Liguori, Jan Kiszka, Markus Armbruster,
Avi Kivity, Paolo Bonzini
---
hw/piix_pci.c | 47 +++++++++++++++++++++++++++++------------------
1 files changed, 29 insertions(+), 18 deletions(-)
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index dddc95f..b38904d 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -52,6 +52,9 @@
#define XEN_PIIX_NUM_PIRQS 128ULL
#define PIIX_PIRQC 0x60
+#define I440FX_PMC_PCI_HOLE 0xE0000000ULL
+#define I440FX_PMC_PCI_HOLE_END 0x100000000ULL
+
#define TYPE_PIIX3 "PIIX3"
#define PIIX3(obj) OBJECT_CHECK(PIIX3State, (obj), TYPE_PIIX3)
@@ -105,10 +108,6 @@ struct I440FXPMCState {
MemoryRegion smram_region;
uint8_t smm_enabled;
ram_addr_t ram_size;
- target_phys_addr_t pci_hole_start;
- target_phys_addr_t pci_hole_size;
- target_phys_addr_t pci_hole64_start;
- target_phys_addr_t pci_hole64_size;
};
#define TYPE_I440FX "i440FX"
@@ -285,6 +284,9 @@ static int i440fx_realize(SysBusDevice *dev)
sysbus_add_io(dev, 0xcfc, &h->data_mem);
sysbus_init_ioports(&h->busdev, 0xcfc, 4);
+ s->pmc.system_memory = h->address_space;
+ s->pmc.pci_address_space = s->pci_address_space;
+
qdev_set_parent_bus(DEVICE(&s->pmc), BUS(h->bus));
qdev_init_nofail(DEVICE(&s->pmc));
@@ -326,19 +328,37 @@ static int i440fx_pmc_initfn(PCIDevice *dev)
{
I440FXPMCState *d = DO_UPCAST(I440FXPMCState, dev, dev);
ram_addr_t ram_size;
+ uint64_t pci_hole_start, pci_hole_size;
+ uint64_t pci_hole64_start, pci_hole64_size;
+ g_assert(d->ram_size != 0);
g_assert(d->system_memory != NULL);
g_assert(d->pci_address_space != NULL);
g_assert(d->ram_memory != NULL);
+ /* Calculate PCI geometry from RAM size */
+ if (d->ram_size > I440FX_PMC_PCI_HOLE) {
+ pci_hole_start = I440FX_PMC_PCI_HOLE;
+ } else {
+ pci_hole_start = d->ram_size;
+ }
+ pci_hole_size = I440FX_PMC_PCI_HOLE_END - pci_hole_start;
+
+ pci_hole64_start = I440FX_PMC_PCI_HOLE_END + d->ram_size - pci_hole_start;
+ if (sizeof(target_phys_addr_t) == 4) {
+ pci_hole64_size = 0;
+ } else {
+ pci_hole64_size = (1ULL << 62);
+ }
+
memory_region_init_alias(&d->pci_hole, "pci-hole", d->pci_address_space,
- d->pci_hole_start, d->pci_hole_size);
- memory_region_add_subregion(d->system_memory, d->pci_hole_start, &d->pci_hole);
+ pci_hole_start, pci_hole_size);
+ memory_region_add_subregion(d->system_memory, pci_hole_start, &d->pci_hole);
memory_region_init_alias(&d->pci_hole_64bit, "pci-hole64",
d->pci_address_space,
- d->pci_hole64_start, d->pci_hole64_size);
- if (d->pci_hole64_size) {
- memory_region_add_subregion(d->system_memory, d->pci_hole64_start,
+ pci_hole64_start, pci_hole64_size);
+ if (pci_hole64_size) {
+ memory_region_add_subregion(d->system_memory, pci_hole64_start,
&d->pci_hole_64bit);
}
memory_region_init_alias(&d->smram_region, "smram-region",
@@ -385,18 +405,9 @@ PCIBus *i440fx_init(I440FXPMCState **pi440fx_state, int *piix3_devfn,
s->piix3.pic = pic;
- s->pmc.system_memory = address_space_mem;
- s->pmc.pci_address_space = pci_address_space;
s->pmc.ram_memory = ram_memory;
-
s->pmc.ram_size = ram_size;
- /* FIXME these should be derived */
- s->pmc.pci_hole_start = pci_hole_start;
- s->pmc.pci_hole_size = pci_hole_size;
- s->pmc.pci_hole64_start = pci_hole64_start;
- s->pmc.pci_hole64_size = pci_hole64_size;
-
qdev_set_parent_bus(DEVICE(s), sysbus_get_default());
qdev_init_nofail(DEVICE(s));
--
1.7.4.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [PATCH 13/15] i440fx: allocate MemoryRegion for pci memory space
2012-01-26 19:00 [Qemu-devel] [RFC 00/15] Refactor PC machine to take advantage of QOM Anthony Liguori
` (11 preceding siblings ...)
2012-01-26 19:00 ` [Qemu-devel] [PATCH 12/15] i440fx-pmc: calculate PCI memory hole directly Anthony Liguori
@ 2012-01-26 19:00 ` Anthony Liguori
2012-01-26 19:00 ` [Qemu-devel] [PATCH 14/15] i440fx: move bios loading to i440fx Anthony Liguori
` (2 subsequent siblings)
15 siblings, 0 replies; 55+ messages in thread
From: Anthony Liguori @ 2012-01-26 19:00 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Anthony Liguori, Jan Kiszka, Markus Armbruster,
Avi Kivity, Paolo Bonzini
---
hw/piix_pci.c | 13 ++++++-------
1 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index b38904d..855f402 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -118,7 +118,7 @@ typedef struct I440FXState
PCIHostState parent;
MemoryRegion *address_space_io;
- MemoryRegion *pci_address_space;
+ MemoryRegion pci_address_space;
I440FXPMCState pmc;
PIIX3State piix3;
@@ -267,11 +267,10 @@ static int i440fx_realize(SysBusDevice *dev)
I440FXState *s = I440FX(dev);
PCIHostState *h = PCI_HOST(s);
- g_assert(s->pci_address_space != NULL);
g_assert(h->address_space != NULL);
g_assert(s->address_space_io != NULL);
- h->bus = pci_bus_new(DEVICE(s), NULL, s->pci_address_space,
+ h->bus = pci_bus_new(DEVICE(s), NULL, &s->pci_address_space,
s->address_space_io, 0);
memory_region_init_io(&h->conf_mem, &pci_host_conf_le_ops, s,
@@ -285,7 +284,7 @@ static int i440fx_realize(SysBusDevice *dev)
sysbus_init_ioports(&h->busdev, 0xcfc, 4);
s->pmc.system_memory = h->address_space;
- s->pmc.pci_address_space = s->pci_address_space;
+ s->pmc.pci_address_space = &s->pci_address_space;
qdev_set_parent_bus(DEVICE(&s->pmc), BUS(h->bus));
qdev_init_nofail(DEVICE(&s->pmc));
@@ -322,6 +321,8 @@ static void i440fx_initfn(Object *obj)
object_initialize(&s->piix3, "PIIX3");
}
object_property_add_child(OBJECT(s), "piix3", OBJECT(&s->piix3), NULL);
+
+ memory_region_init(&s->pci_address_space, "pci", INT64_MAX);
}
static int i440fx_pmc_initfn(PCIDevice *dev)
@@ -401,10 +402,8 @@ PCIBus *i440fx_init(I440FXPMCState **pi440fx_state, int *piix3_devfn,
/* FIXME make a properties */
h->address_space = address_space_mem;
s->address_space_io = address_space_io;
- s->pci_address_space = pci_address_space;
-
s->piix3.pic = pic;
-
+ /* FIXME pmc should create ram_memory */
s->pmc.ram_memory = ram_memory;
s->pmc.ram_size = ram_size;
--
1.7.4.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [PATCH 14/15] i440fx: move bios loading to i440fx
2012-01-26 19:00 [Qemu-devel] [RFC 00/15] Refactor PC machine to take advantage of QOM Anthony Liguori
` (12 preceding siblings ...)
2012-01-26 19:00 ` [Qemu-devel] [PATCH 13/15] i440fx: allocate MemoryRegion for pci memory space Anthony Liguori
@ 2012-01-26 19:00 ` Anthony Liguori
2012-01-31 14:38 ` Jan Kiszka
2012-01-26 19:01 ` [Qemu-devel] [PATCH 15/15] i440fx: move ram initialization into i440fx-pmc Anthony Liguori
2012-01-26 19:12 ` [Qemu-devel] [RFC 00/15] Refactor PC machine to take advantage of QOM Peter Maydell
15 siblings, 1 reply; 55+ messages in thread
From: Anthony Liguori @ 2012-01-26 19:00 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Anthony Liguori, Jan Kiszka, Markus Armbruster,
Avi Kivity, Paolo Bonzini
---
hw/pc.c | 70 ++++--------------------------------------------------
hw/pc.h | 3 +-
hw/piix_pci.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
sysemu.h | 2 -
4 files changed, 79 insertions(+), 70 deletions(-)
diff --git a/hw/pc.c b/hw/pc.c
index b7542b4..4964a64 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -64,8 +64,6 @@
#define DPRINTF(fmt, ...)
#endif
-#define BIOS_FILENAME "bios.bin"
-
#define PC_MAX_BIOS_SIZE (4 * 1024 * 1024)
/* Leave a chunk of memory at the top of RAM for the BIOS ACPI tables. */
@@ -996,14 +994,10 @@ static void pc_memory_init(MemoryRegion *system_memory,
const char *initrd_filename,
ram_addr_t below_4g_mem_size,
ram_addr_t above_4g_mem_size,
- MemoryRegion *rom_memory,
MemoryRegion **ram_memory)
{
- char *filename;
- int ret, linux_boot, i;
- MemoryRegion *ram, *bios, *isa_bios, *option_rom_mr;
- MemoryRegion *ram_below_4g, *ram_above_4g;
- int bios_size, isa_bios_size;
+ int linux_boot, i;
+ MemoryRegion *ram, *ram_below_4g, *ram_above_4g;
void *fw_cfg;
linux_boot = (kernel_filename != NULL);
@@ -1029,58 +1023,6 @@ static void pc_memory_init(MemoryRegion *system_memory,
ram_above_4g);
}
- /* BIOS load */
- if (bios_name == NULL)
- bios_name = BIOS_FILENAME;
- filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
- if (filename) {
- bios_size = get_image_size(filename);
- } else {
- bios_size = -1;
- }
- if (bios_size <= 0 ||
- (bios_size % 65536) != 0) {
- goto bios_error;
- }
- bios = g_malloc(sizeof(*bios));
- memory_region_init_ram(bios, "pc.bios", bios_size);
- vmstate_register_ram_global(bios);
- memory_region_set_readonly(bios, true);
- ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1);
- if (ret != 0) {
- bios_error:
- fprintf(stderr, "qemu: could not load PC BIOS '%s'\n", bios_name);
- exit(1);
- }
- if (filename) {
- g_free(filename);
- }
- /* map the last 128KB of the BIOS in ISA space */
- isa_bios_size = bios_size;
- if (isa_bios_size > (128 * 1024))
- isa_bios_size = 128 * 1024;
- isa_bios = g_malloc(sizeof(*isa_bios));
- memory_region_init_alias(isa_bios, "isa-bios", bios,
- bios_size - isa_bios_size, isa_bios_size);
- memory_region_add_subregion_overlap(rom_memory,
- 0x100000 - isa_bios_size,
- isa_bios,
- 1);
- memory_region_set_readonly(isa_bios, true);
-
- option_rom_mr = g_malloc(sizeof(*option_rom_mr));
- memory_region_init_ram(option_rom_mr, "pc.rom", PC_ROM_SIZE);
- vmstate_register_ram_global(option_rom_mr);
- memory_region_add_subregion_overlap(rom_memory,
- PC_ROM_MIN_VGA,
- option_rom_mr,
- 1);
-
- /* map all the bios at the top of memory */
- memory_region_add_subregion(rom_memory,
- (uint32_t)(-bios_size),
- bios);
-
fw_cfg = bochs_bios_init();
rom_set_fw(fw_cfg);
@@ -1299,7 +1241,6 @@ static void pc_init1(MemoryRegion *system_memory,
ISADevice *floppy;
MemoryRegion *ram_memory = NULL;
MemoryRegion *pci_memory;
- MemoryRegion *rom_memory;
DeviceState *dev;
pc_cpus_init(cpu_model);
@@ -1319,10 +1260,8 @@ static void pc_init1(MemoryRegion *system_memory,
if (pci_enabled) {
pci_memory = g_new(MemoryRegion, 1);
memory_region_init(pci_memory, "pci", INT64_MAX);
- rom_memory = pci_memory;
} else {
pci_memory = NULL;
- rom_memory = system_memory;
}
/* allocate ram and load rom/bios */
@@ -1330,7 +1269,7 @@ static void pc_init1(MemoryRegion *system_memory,
pc_memory_init(system_memory,
kernel_filename, kernel_cmdline, initrd_filename,
below_4g_mem_size, above_4g_mem_size,
- pci_enabled ? rom_memory : system_memory, &ram_memory);
+ &ram_memory);
}
gsi_state = g_malloc0(sizeof(*gsi_state));
@@ -1351,7 +1290,8 @@ static void pc_init1(MemoryRegion *system_memory,
(sizeof(target_phys_addr_t) == 4
? 0
: ((uint64_t)1 << 62)),
- pci_memory, ram_memory);
+ pci_memory, ram_memory,
+ bios_name);
} else {
pci_bus = NULL;
i440fx_state = NULL;
diff --git a/hw/pc.h b/hw/pc.h
index 0764f55..530f417 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -166,7 +166,8 @@ PCIBus *i440fx_init(I440FXPMCState **pi440fx_state, int *piix_devfn,
target_phys_addr_t pci_hole64_start,
target_phys_addr_t pci_hole64_size,
MemoryRegion *pci_memory,
- MemoryRegion *ram_memory);
+ MemoryRegion *ram_memory,
+ const char *bios_name);
/* piix4.c */
extern PCIDevice *piix4_dev;
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 855f402..6dda019 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -33,6 +33,10 @@
#include "hpet_emul.h"
#include "mc146818rtc.h"
#include "i8254.h"
+#include "loader.h"
+#include "sysemu.h"
+
+#define BIOS_FILENAME "bios.bin"
/*
* I440FX chipset data sheet.
@@ -122,6 +126,13 @@ typedef struct I440FXState
I440FXPMCState pmc;
PIIX3State piix3;
+
+ /* Is this more appropriate for the PMC? */
+ MemoryRegion bios;
+ MemoryRegion isa_bios;
+ MemoryRegion option_roms;
+
+ char *bios_name;
} I440FXState;
#define I440FX_PAM 0x59
@@ -266,6 +277,9 @@ static int i440fx_realize(SysBusDevice *dev)
{
I440FXState *s = I440FX(dev);
PCIHostState *h = PCI_HOST(s);
+ int bios_size, isa_bios_size;
+ char *filename;
+ int ret;
g_assert(h->address_space != NULL);
g_assert(s->address_space_io != NULL);
@@ -300,6 +314,55 @@ static int i440fx_realize(SysBusDevice *dev)
PIIX_NUM_PIRQS);
}
+ /* BIOS load */
+ filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, s->bios_name);
+ if (filename) {
+ bios_size = get_image_size(filename);
+ } else {
+ bios_size = -1;
+ }
+ if (bios_size <= 0 ||
+ (bios_size % 65536) != 0) {
+ goto bios_error;
+ }
+ memory_region_init_ram(&s->bios, "pc.bios", bios_size);
+ vmstate_register_ram_global(&s->bios);
+ memory_region_set_readonly(&s->bios, true);
+ ret = rom_add_file_fixed(s->bios_name, (uint32_t)(-bios_size), -1);
+ if (ret != 0) {
+ bios_error:
+ fprintf(stderr, "qemu: could not load PC BIOS '%s'\n", s->bios_name);
+ exit(1);
+ }
+ if (filename) {
+ g_free(filename);
+ }
+
+ /* map the last 128KB of the BIOS in ISA space */
+ isa_bios_size = bios_size;
+ if (isa_bios_size > (128 * 1024)) {
+ isa_bios_size = 128 * 1024;
+ }
+ memory_region_init_alias(&s->isa_bios, "isa-bios", &s->bios,
+ bios_size - isa_bios_size, isa_bios_size);
+ memory_region_add_subregion_overlap(&s->pci_address_space,
+ 0x100000 - isa_bios_size,
+ &s->isa_bios,
+ 1);
+ memory_region_set_readonly(&s->isa_bios, true);
+
+ memory_region_init_ram(&s->option_roms, "pc.rom", PC_ROM_SIZE);
+ vmstate_register_ram_global(&s->option_roms);
+ memory_region_add_subregion_overlap(&s->pci_address_space,
+ PC_ROM_MIN_VGA,
+ &s->option_roms,
+ 1);
+
+ /* map all the bios at the top of memory */
+ memory_region_add_subregion(&s->pci_address_space,
+ (uint32_t)(-bios_size),
+ &s->bios);
+
return 0;
}
@@ -322,6 +385,8 @@ static void i440fx_initfn(Object *obj)
}
object_property_add_child(OBJECT(s), "piix3", OBJECT(&s->piix3), NULL);
+ s->bios_name = g_strdup(BIOS_FILENAME);
+
memory_region_init(&s->pci_address_space, "pci", INT64_MAX);
}
@@ -390,8 +455,8 @@ PCIBus *i440fx_init(I440FXPMCState **pi440fx_state, int *piix3_devfn,
target_phys_addr_t pci_hole_size,
target_phys_addr_t pci_hole64_start,
target_phys_addr_t pci_hole64_size,
- MemoryRegion *pci_address_space, MemoryRegion *ram_memory)
-
+ MemoryRegion *pci_address_space, MemoryRegion *ram_memory,
+ const char *bios_name)
{
I440FXState *s;
PCIHostState *h;
@@ -403,6 +468,11 @@ PCIBus *i440fx_init(I440FXPMCState **pi440fx_state, int *piix3_devfn,
h->address_space = address_space_mem;
s->address_space_io = address_space_io;
s->piix3.pic = pic;
+ if (bios_name) {
+ g_free(s->bios_name);
+ s->bios_name = g_strdup(bios_name);
+ }
+
/* FIXME pmc should create ram_memory */
s->pmc.ram_memory = ram_memory;
s->pmc.ram_size = ram_size;
diff --git a/sysemu.h b/sysemu.h
index caff268..5978e90 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -172,8 +172,6 @@ void usb_info(Monitor *mon);
void rtc_change_mon_event(struct tm *tm);
-void register_devices(void);
-
void add_boot_device_path(int32_t bootindex, DeviceState *dev,
const char *suffix);
char *get_boot_devices_list(uint32_t *size);
--
1.7.4.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [PATCH 14/15] i440fx: move bios loading to i440fx
2012-01-26 19:00 ` [Qemu-devel] [PATCH 14/15] i440fx: move bios loading to i440fx Anthony Liguori
@ 2012-01-31 14:38 ` Jan Kiszka
2012-01-31 14:50 ` Anthony Liguori
0 siblings, 1 reply; 55+ messages in thread
From: Jan Kiszka @ 2012-01-31 14:38 UTC (permalink / raw)
To: Anthony Liguori
Cc: Peter Maydell, Avi Kivity, Paolo Bonzini, qemu-devel,
Markus Armbruster
On 2012-01-26 20:00, Anthony Liguori wrote:
> ---
> hw/pc.c | 70 ++++--------------------------------------------------
> hw/pc.h | 3 +-
> hw/piix_pci.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> sysemu.h | 2 -
> 4 files changed, 79 insertions(+), 70 deletions(-)
How does the ISA PC get its BIOS after this change? Or did that change
in a step I miss right now?
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [PATCH 14/15] i440fx: move bios loading to i440fx
2012-01-31 14:38 ` Jan Kiszka
@ 2012-01-31 14:50 ` Anthony Liguori
2012-01-31 14:53 ` Jan Kiszka
0 siblings, 1 reply; 55+ messages in thread
From: Anthony Liguori @ 2012-01-31 14:50 UTC (permalink / raw)
To: Jan Kiszka
Cc: Peter Maydell, Anthony Liguori, qemu-devel, Markus Armbruster,
Avi Kivity, Paolo Bonzini
On 01/31/2012 08:38 AM, Jan Kiszka wrote:
> On 2012-01-26 20:00, Anthony Liguori wrote:
>> ---
>> hw/pc.c | 70 ++++--------------------------------------------------
>> hw/pc.h | 3 +-
>> hw/piix_pci.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>> sysemu.h | 2 -
>> 4 files changed, 79 insertions(+), 70 deletions(-)
>
> How does the ISA PC get its BIOS after this change? Or did that change
> in a step I miss right now?
Oh, I broke it. I made no attempt to keep ISA PC working.
The way I'd like to handle this is to introduce a ROM device so that this code
would be trivialized.
Regards,
Anthony Liguori
>
> Jan
>
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [PATCH 14/15] i440fx: move bios loading to i440fx
2012-01-31 14:50 ` Anthony Liguori
@ 2012-01-31 14:53 ` Jan Kiszka
2012-01-31 14:57 ` Anthony Liguori
0 siblings, 1 reply; 55+ messages in thread
From: Jan Kiszka @ 2012-01-31 14:53 UTC (permalink / raw)
To: Anthony Liguori
Cc: Peter Maydell, Anthony Liguori, qemu-devel@nongnu.org,
Markus Armbruster, Avi Kivity, Paolo Bonzini
On 2012-01-31 15:50, Anthony Liguori wrote:
> On 01/31/2012 08:38 AM, Jan Kiszka wrote:
>> On 2012-01-26 20:00, Anthony Liguori wrote:
>>> ---
>>> hw/pc.c | 70 ++++--------------------------------------------------
>>> hw/pc.h | 3 +-
>>> hw/piix_pci.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>> sysemu.h | 2 -
>>> 4 files changed, 79 insertions(+), 70 deletions(-)
>>
>> How does the ISA PC get its BIOS after this change? Or did that change
>> in a step I miss right now?
>
> Oh, I broke it. I made no attempt to keep ISA PC working.
>
> The way I'd like to handle this is to introduce a ROM device so that this code
> would be trivialized.
Or just keep the common pc.c library as I voted for. It has its purpose,
obviously.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [PATCH 14/15] i440fx: move bios loading to i440fx
2012-01-31 14:53 ` Jan Kiszka
@ 2012-01-31 14:57 ` Anthony Liguori
2012-01-31 15:01 ` Jan Kiszka
0 siblings, 1 reply; 55+ messages in thread
From: Anthony Liguori @ 2012-01-31 14:57 UTC (permalink / raw)
To: Jan Kiszka
Cc: Peter Maydell, Anthony Liguori, qemu-devel@nongnu.org,
Markus Armbruster, Avi Kivity, Paolo Bonzini
On 01/31/2012 08:53 AM, Jan Kiszka wrote:
> On 2012-01-31 15:50, Anthony Liguori wrote:
>> On 01/31/2012 08:38 AM, Jan Kiszka wrote:
>>> On 2012-01-26 20:00, Anthony Liguori wrote:
>>>> ---
>>>> hw/pc.c | 70 ++++--------------------------------------------------
>>>> hw/pc.h | 3 +-
>>>> hw/piix_pci.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>> sysemu.h | 2 -
>>>> 4 files changed, 79 insertions(+), 70 deletions(-)
>>>
>>> How does the ISA PC get its BIOS after this change? Or did that change
>>> in a step I miss right now?
>>
>> Oh, I broke it. I made no attempt to keep ISA PC working.
>>
>> The way I'd like to handle this is to introduce a ROM device so that this code
>> would be trivialized.
>
> Or just keep the common pc.c library as I voted for. It has its purpose,
> obviously.
Coding sharing needs to happen through device sharing. Otherwise we'll be stuck
in the magic device creation through arbitrary functions rut that we currently
find ourselves in.
Regards,
Anthony Liguori
>
> Jan
>
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [PATCH 14/15] i440fx: move bios loading to i440fx
2012-01-31 14:57 ` Anthony Liguori
@ 2012-01-31 15:01 ` Jan Kiszka
0 siblings, 0 replies; 55+ messages in thread
From: Jan Kiszka @ 2012-01-31 15:01 UTC (permalink / raw)
To: Anthony Liguori
Cc: Peter Maydell, Anthony Liguori, qemu-devel@nongnu.org,
Markus Armbruster, Avi Kivity, Paolo Bonzini
On 2012-01-31 15:57, Anthony Liguori wrote:
> On 01/31/2012 08:53 AM, Jan Kiszka wrote:
>> On 2012-01-31 15:50, Anthony Liguori wrote:
>>> On 01/31/2012 08:38 AM, Jan Kiszka wrote:
>>>> On 2012-01-26 20:00, Anthony Liguori wrote:
>>>>> ---
>>>>> hw/pc.c | 70 ++++--------------------------------------------------
>>>>> hw/pc.h | 3 +-
>>>>> hw/piix_pci.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>> sysemu.h | 2 -
>>>>> 4 files changed, 79 insertions(+), 70 deletions(-)
>>>>
>>>> How does the ISA PC get its BIOS after this change? Or did that change
>>>> in a step I miss right now?
>>>
>>> Oh, I broke it. I made no attempt to keep ISA PC working.
>>>
>>> The way I'd like to handle this is to introduce a ROM device so that this code
>>> would be trivialized.
>>
>> Or just keep the common pc.c library as I voted for. It has its purpose,
>> obviously.
>
> Coding sharing needs to happen through device sharing. Otherwise we'll be stuck
> in the magic device creation through arbitrary functions rut that we currently
> find ourselves in.
Well, let's see what this will mean in practice. I'm sure that that
there are steps in a PC construction that cannot be modeled reasonable
even as pseudo devices but at still shared among boards.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] [PATCH 15/15] i440fx: move ram initialization into i440fx-pmc
2012-01-26 19:00 [Qemu-devel] [RFC 00/15] Refactor PC machine to take advantage of QOM Anthony Liguori
` (13 preceding siblings ...)
2012-01-26 19:00 ` [Qemu-devel] [PATCH 14/15] i440fx: move bios loading to i440fx Anthony Liguori
@ 2012-01-26 19:01 ` Anthony Liguori
2012-01-26 19:12 ` [Qemu-devel] [RFC 00/15] Refactor PC machine to take advantage of QOM Peter Maydell
15 siblings, 0 replies; 55+ messages in thread
From: Anthony Liguori @ 2012-01-26 19:01 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Anthony Liguori, Jan Kiszka, Markus Armbruster,
Avi Kivity, Paolo Bonzini
---
hw/pc.c | 44 ++------------------------------------------
hw/pc.h | 6 ------
hw/piix_pci.c | 46 ++++++++++++++++++++++++++++++++--------------
3 files changed, 34 insertions(+), 62 deletions(-)
diff --git a/hw/pc.c b/hw/pc.c
index 4964a64..90bd638 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -993,36 +993,13 @@ static void pc_memory_init(MemoryRegion *system_memory,
const char *kernel_cmdline,
const char *initrd_filename,
ram_addr_t below_4g_mem_size,
- ram_addr_t above_4g_mem_size,
- MemoryRegion **ram_memory)
+ ram_addr_t above_4g_mem_size)
{
int linux_boot, i;
- MemoryRegion *ram, *ram_below_4g, *ram_above_4g;
void *fw_cfg;
linux_boot = (kernel_filename != NULL);
- /* Allocate RAM. We allocate it as a single memory region and use
- * aliases to address portions of it, mostly for backwards compatibility
- * with older qemus that used qemu_ram_alloc().
- */
- ram = g_malloc(sizeof(*ram));
- memory_region_init_ram(ram, "pc.ram",
- below_4g_mem_size + above_4g_mem_size);
- vmstate_register_ram_global(ram);
- *ram_memory = ram;
- ram_below_4g = g_malloc(sizeof(*ram_below_4g));
- memory_region_init_alias(ram_below_4g, "ram-below-4g", ram,
- 0, below_4g_mem_size);
- memory_region_add_subregion(system_memory, 0, ram_below_4g);
- if (above_4g_mem_size > 0) {
- ram_above_4g = g_malloc(sizeof(*ram_above_4g));
- memory_region_init_alias(ram_above_4g, "ram-above-4g", ram,
- below_4g_mem_size, above_4g_mem_size);
- memory_region_add_subregion(system_memory, 0x100000000ULL,
- ram_above_4g);
- }
-
fw_cfg = bochs_bios_init();
rom_set_fw(fw_cfg);
@@ -1239,8 +1216,6 @@ static void pc_init1(MemoryRegion *system_memory,
BusState *idebus[MAX_IDE_BUS];
ISADevice *rtc_state;
ISADevice *floppy;
- MemoryRegion *ram_memory = NULL;
- MemoryRegion *pci_memory;
DeviceState *dev;
pc_cpus_init(cpu_model);
@@ -1257,19 +1232,11 @@ static void pc_init1(MemoryRegion *system_memory,
below_4g_mem_size = ram_size;
}
- if (pci_enabled) {
- pci_memory = g_new(MemoryRegion, 1);
- memory_region_init(pci_memory, "pci", INT64_MAX);
- } else {
- pci_memory = NULL;
- }
-
/* allocate ram and load rom/bios */
if (!xen_enabled()) {
pc_memory_init(system_memory,
kernel_filename, kernel_cmdline, initrd_filename,
- below_4g_mem_size, above_4g_mem_size,
- &ram_memory);
+ below_4g_mem_size, above_4g_mem_size);
}
gsi_state = g_malloc0(sizeof(*gsi_state));
@@ -1284,13 +1251,6 @@ static void pc_init1(MemoryRegion *system_memory,
if (pci_enabled) {
pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,
system_memory, system_io, ram_size,
- below_4g_mem_size,
- 0x100000000ULL - below_4g_mem_size,
- 0x100000000ULL + above_4g_mem_size,
- (sizeof(target_phys_addr_t) == 4
- ? 0
- : ((uint64_t)1 << 62)),
- pci_memory, ram_memory,
bios_name);
} else {
pci_bus = NULL;
diff --git a/hw/pc.h b/hw/pc.h
index 530f417..71bbfb3 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -161,12 +161,6 @@ PCIBus *i440fx_init(I440FXPMCState **pi440fx_state, int *piix_devfn,
MemoryRegion *address_space_mem,
MemoryRegion *address_space_io,
ram_addr_t ram_size,
- target_phys_addr_t pci_hole_start,
- target_phys_addr_t pci_hole_size,
- target_phys_addr_t pci_hole64_start,
- target_phys_addr_t pci_hole64_size,
- MemoryRegion *pci_memory,
- MemoryRegion *ram_memory,
const char *bios_name);
/* piix4.c */
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 6dda019..0c2226b 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -111,7 +111,11 @@ struct I440FXPMCState {
PAMMemoryRegion pam_regions[13];
MemoryRegion smram_region;
uint8_t smm_enabled;
+
ram_addr_t ram_size;
+ MemoryRegion ram;
+ MemoryRegion ram_below_4g;
+ MemoryRegion ram_above_4g;
};
#define TYPE_I440FX "i440FX"
@@ -390,24 +394,46 @@ static void i440fx_initfn(Object *obj)
memory_region_init(&s->pci_address_space, "pci", INT64_MAX);
}
-static int i440fx_pmc_initfn(PCIDevice *dev)
+static int i440fx_pmc_realize(PCIDevice *dev)
{
I440FXPMCState *d = DO_UPCAST(I440FXPMCState, dev, dev);
ram_addr_t ram_size;
+ uint64_t below_4g_mem_size, above_4g_mem_size;
uint64_t pci_hole_start, pci_hole_size;
uint64_t pci_hole64_start, pci_hole64_size;
g_assert(d->ram_size != 0);
g_assert(d->system_memory != NULL);
g_assert(d->pci_address_space != NULL);
- g_assert(d->ram_memory != NULL);
- /* Calculate PCI geometry from RAM size */
+ /* Calculate memory geometry from RAM size */
if (d->ram_size > I440FX_PMC_PCI_HOLE) {
- pci_hole_start = I440FX_PMC_PCI_HOLE;
+ below_4g_mem_size = I440FX_PMC_PCI_HOLE;
+ above_4g_mem_size = d->ram_size - I440FX_PMC_PCI_HOLE;
} else {
- pci_hole_start = d->ram_size;
+ below_4g_mem_size = d->ram_size;
+ above_4g_mem_size = 0;
+ }
+
+ /* Allocate RAM. We allocate it as a single memory region and use
+ * aliases to address portions of it, mostly for backwards compatibility
+ * with older qemus that used qemu_ram_alloc().
+ */
+ memory_region_init_ram(&d->ram, "pc.ram",
+ below_4g_mem_size + above_4g_mem_size);
+ vmstate_register_ram_global(&d->ram);
+
+ memory_region_init_alias(&d->ram_below_4g, "ram-below-4g", &d->ram,
+ 0, below_4g_mem_size);
+ memory_region_add_subregion(d->system_memory, 0, &d->ram_below_4g);
+ if (above_4g_mem_size > 0) {
+ memory_region_init_alias(&d->ram_above_4g, "ram-above-4g", &d->ram,
+ below_4g_mem_size, above_4g_mem_size);
+ memory_region_add_subregion(d->system_memory, 0x100000000ULL,
+ &d->ram_above_4g);
}
+
+ pci_hole_start = below_4g_mem_size;
pci_hole_size = I440FX_PMC_PCI_HOLE_END - pci_hole_start;
pci_hole64_start = I440FX_PMC_PCI_HOLE_END + d->ram_size - pci_hole_start;
@@ -451,11 +477,6 @@ PCIBus *i440fx_init(I440FXPMCState **pi440fx_state, int *piix3_devfn,
MemoryRegion *address_space_mem,
MemoryRegion *address_space_io,
ram_addr_t ram_size,
- target_phys_addr_t pci_hole_start,
- target_phys_addr_t pci_hole_size,
- target_phys_addr_t pci_hole64_start,
- target_phys_addr_t pci_hole64_size,
- MemoryRegion *pci_address_space, MemoryRegion *ram_memory,
const char *bios_name)
{
I440FXState *s;
@@ -472,9 +493,6 @@ PCIBus *i440fx_init(I440FXPMCState **pi440fx_state, int *piix3_devfn,
g_free(s->bios_name);
s->bios_name = g_strdup(bios_name);
}
-
- /* FIXME pmc should create ram_memory */
- s->pmc.ram_memory = ram_memory;
s->pmc.ram_size = ram_size;
qdev_set_parent_bus(DEVICE(s), sysbus_get_default());
@@ -737,7 +755,7 @@ static void i440fx_pmc_class_init(ObjectClass *klass, void *data)
PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
k->no_hotplug = 1;
- k->init = i440fx_pmc_initfn;
+ k->init = i440fx_pmc_realize;
k->config_write = i440fx_pmc_write_config;
k->vendor_id = PCI_VENDOR_ID_INTEL;
k->device_id = PCI_DEVICE_ID_INTEL_82441;
--
1.7.4.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [RFC 00/15] Refactor PC machine to take advantage of QOM
2012-01-26 19:00 [Qemu-devel] [RFC 00/15] Refactor PC machine to take advantage of QOM Anthony Liguori
` (14 preceding siblings ...)
2012-01-26 19:01 ` [Qemu-devel] [PATCH 15/15] i440fx: move ram initialization into i440fx-pmc Anthony Liguori
@ 2012-01-26 19:12 ` Peter Maydell
2012-01-26 19:36 ` Anthony Liguori
2012-01-26 19:57 ` Markus Armbruster
15 siblings, 2 replies; 55+ messages in thread
From: Peter Maydell @ 2012-01-26 19:12 UTC (permalink / raw)
To: Anthony Liguori
Cc: Paolo Bonzini, Jan Kiszka, qemu-devel, Markus Armbruster,
Avi Kivity
On 26 January 2012 19:00, Anthony Liguori <aliguori@us.ibm.com> wrote:
> We need to modeled MemoryRegions and qemu_irq in QOM too.
+1 : this ought to let us get rid of SysBus...
> MemoryRegions
> shouldn't be that difficult. Our habit of passing qemu_irq's as arrays without
> an explicit size will probably require some refactoring but in principle,
> supporting irqs should be easy too.
I think that there are probably a lot of cases where we're using an array
of qemu_irqs now but should be using separately named signals of some sort
instead (particularly where we're using them for things which aren't actually
IRQs...)
-- PMM
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [RFC 00/15] Refactor PC machine to take advantage of QOM
2012-01-26 19:12 ` [Qemu-devel] [RFC 00/15] Refactor PC machine to take advantage of QOM Peter Maydell
@ 2012-01-26 19:36 ` Anthony Liguori
2012-01-29 10:42 ` Avi Kivity
2012-01-26 19:57 ` Markus Armbruster
1 sibling, 1 reply; 55+ messages in thread
From: Anthony Liguori @ 2012-01-26 19:36 UTC (permalink / raw)
To: Peter Maydell
Cc: Anthony Liguori, Jan Kiszka, qemu-devel, Markus Armbruster,
Avi Kivity, Paolo Bonzini
On 01/26/2012 01:12 PM, Peter Maydell wrote:
> On 26 January 2012 19:00, Anthony Liguori<aliguori@us.ibm.com> wrote:
>> We need to modeled MemoryRegions and qemu_irq in QOM too.
>
> +1 : this ought to let us get rid of SysBus...
>
>> MemoryRegions
>> shouldn't be that difficult. Our habit of passing qemu_irq's as arrays without
>> an explicit size will probably require some refactoring but in principle,
>> supporting irqs should be easy too.
>
> I think that there are probably a lot of cases where we're using an array
> of qemu_irqs now but should be using separately named signals of some sort
> instead (particularly where we're using them for things which aren't actually
> IRQs...)
I started hacking up a Pin object that used a Notifier. It's pretty easy to
plumb that to an existing qemu_irq so I think that's the way to go.
That way we could incrementally remove qemu_irq usage.
I started with this path but the pc initialization was so fubar that I ran into
too many problems. Now I think I can go back and do it again and it will be
more reasonable given this refactoring.
At a high level, a Pin object looks and feels like a qemu_irq. There's a
pin_raise, pin_set_level, etc. But there is also a pin_get_level() (it's
stateful) and there's a pin_add_level_change_notifier() which allows you to
register.
Pins are objects so they can be added to the composition tree which means they
can be addressed. If you have a truly unidirectional path, then you can just
use a child and link and connect them that way.
For a bidirectional path (where software controls the direction at run time),
you can have an intermediate Wire object which consists of two Pin links.
Regards,
Anthony Liguori
>
> -- PMM
>
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [RFC 00/15] Refactor PC machine to take advantage of QOM
2012-01-26 19:36 ` Anthony Liguori
@ 2012-01-29 10:42 ` Avi Kivity
0 siblings, 0 replies; 55+ messages in thread
From: Avi Kivity @ 2012-01-29 10:42 UTC (permalink / raw)
To: Anthony Liguori
Cc: Peter Maydell, Anthony Liguori, Jan Kiszka, qemu-devel,
Markus Armbruster, Paolo Bonzini
On 01/26/2012 09:36 PM, Anthony Liguori wrote:
> On 01/26/2012 01:12 PM, Peter Maydell wrote:
>> On 26 January 2012 19:00, Anthony Liguori<aliguori@us.ibm.com> wrote:
>>> We need to modeled MemoryRegions and qemu_irq in QOM too.
>>
>> +1 : this ought to let us get rid of SysBus...
>>
>>> MemoryRegions
>>> shouldn't be that difficult. Our habit of passing qemu_irq's as
>>> arrays without
>>> an explicit size will probably require some refactoring but in
>>> principle,
>>> supporting irqs should be easy too.
>>
>> I think that there are probably a lot of cases where we're using an
>> array
>> of qemu_irqs now but should be using separately named signals of some
>> sort
>> instead (particularly where we're using them for things which aren't
>> actually
>> IRQs...)
>
> I started hacking up a Pin object that used a Notifier. It's pretty
> easy to plumb that to an existing qemu_irq so I think that's the way
> to go.
>
> That way we could incrementally remove qemu_irq usage.
>
> I started with this path but the pc initialization was so fubar that I
> ran into too many problems. Now I think I can go back and do it again
> and it will be more reasonable given this refactoring.
>
> At a high level, a Pin object looks and feels like a qemu_irq.
> There's a pin_raise, pin_set_level, etc. But there is also a
> pin_get_level() (it's stateful) and there's a
> pin_add_level_change_notifier() which allows you to register.
>
> Pins are objects so they can be added to the composition tree which
> means they can be addressed. If you have a truly unidirectional path,
> then you can just use a child and link and connect them that way.
>
Like. But note: like a MemoryRegion, a Pin reflects state held
elsewhere, so it should not be saved/restored.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Qemu-devel] [RFC 00/15] Refactor PC machine to take advantage of QOM
2012-01-26 19:12 ` [Qemu-devel] [RFC 00/15] Refactor PC machine to take advantage of QOM Peter Maydell
2012-01-26 19:36 ` Anthony Liguori
@ 2012-01-26 19:57 ` Markus Armbruster
2012-01-26 20:00 ` Anthony Liguori
1 sibling, 1 reply; 55+ messages in thread
From: Markus Armbruster @ 2012-01-26 19:57 UTC (permalink / raw)
To: Peter Maydell
Cc: Avi Kivity, Paolo Bonzini, Anthony Liguori, qemu-devel,
Jan Kiszka
Peter Maydell <peter.maydell@linaro.org> writes:
> On 26 January 2012 19:00, Anthony Liguori <aliguori@us.ibm.com> wrote:
>> We need to modeled MemoryRegions and qemu_irq in QOM too.
>
> +1 : this ought to let us get rid of SysBus...
>
>> MemoryRegions
>> shouldn't be that difficult. Our habit of passing qemu_irq's as arrays without
>> an explicit size will probably require some refactoring but in principle,
>> supporting irqs should be easy too.
>
> I think that there are probably a lot of cases where we're using an array
> of qemu_irqs now but should be using separately named signals of some sort
> instead (particularly where we're using them for things which aren't actually
> IRQs...)
Apropos array of qemu_irq: we're leaking them in several places.
qemu_allocate_irqs() returns a pointer to an array of qemu_irq whose
elements to point into an array of *qemu_irq. Both arrays are malloced.
Many callers ask for just one IRQ like this:
*qemu_allocate_irqs(..., 1)
This leaks the array of qemu_irq.
There are other, less obvious leaks of the returned array.
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [Qemu-devel] [RFC 00/15] Refactor PC machine to take advantage of QOM
2012-01-26 19:57 ` Markus Armbruster
@ 2012-01-26 20:00 ` Anthony Liguori
0 siblings, 0 replies; 55+ messages in thread
From: Anthony Liguori @ 2012-01-26 20:00 UTC (permalink / raw)
To: Markus Armbruster
Cc: Peter Maydell, Anthony Liguori, Jan Kiszka, qemu-devel,
Avi Kivity, Paolo Bonzini
On 01/26/2012 01:57 PM, Markus Armbruster wrote:
> Peter Maydell<peter.maydell@linaro.org> writes:
>
>> On 26 January 2012 19:00, Anthony Liguori<aliguori@us.ibm.com> wrote:
>>> We need to modeled MemoryRegions and qemu_irq in QOM too.
>>
>> +1 : this ought to let us get rid of SysBus...
>>
>>> MemoryRegions
>>> shouldn't be that difficult. Our habit of passing qemu_irq's as arrays without
>>> an explicit size will probably require some refactoring but in principle,
>>> supporting irqs should be easy too.
>>
>> I think that there are probably a lot of cases where we're using an array
>> of qemu_irqs now but should be using separately named signals of some sort
>> instead (particularly where we're using them for things which aren't actually
>> IRQs...)
>
> Apropos array of qemu_irq: we're leaking them in several places.
> qemu_allocate_irqs() returns a pointer to an array of qemu_irq whose
> elements to point into an array of *qemu_irq. Both arrays are malloced.
>
> Many callers ask for just one IRQ like this:
>
> *qemu_allocate_irqs(..., 1)
>
> This leaks the array of qemu_irq.
>
> There are other, less obvious leaks of the returned array.
We leak lots of stuff...
One nice thing in QOM is that we can create child devices that use the parent
device's resources and tie the life cycles together.
So whereas we were leaking all sorts of MemoryRegions and qdev devices before,
with this series, we do it a whole lot less.
Same principle will apply to IRQs.
Regards,
Anthony Liguori
>
^ permalink raw reply [flat|nested] 55+ messages in thread