On Mon, 12 Jun 2023 16:51:55 +0200 Igor Mammedov <imamm...@redhat.com> wrote:
> On Sun, 11 Jun 2023 12:34:12 +0200 > Bernhard Beschow <shen...@gmail.com> wrote: > > > I440FX realization is currently mixed with PIIX3 creation. Furthermore, it > > is > > common practice to only set properties between a device's qdev_new() and > > qdev_realize(). Clean up to resolve both issues. > > > > Since I440FX spawns a PCI bus let's also move the pci_bus initialization > > there. > > > > Note that when running `qemu-system-x86_64 -M pc -S` before and after this > > patch, `info mtree` in the QEMU console doesn't show any differences except > > that > > the ordering is different. > > > > Signed-off-by: Bernhard Beschow <shen...@gmail.com> > > --- > > hw/i386/pc_piix.c | 57 ++++++++++++++++++++++++----------------------- > > 1 file changed, 29 insertions(+), 28 deletions(-) > > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > > index 22173b122b..23b9725c94 100644 > > --- a/hw/i386/pc_piix.c > > +++ b/hw/i386/pc_piix.c > > @@ -126,7 +126,6 @@ static void pc_init1(MachineState *machine, > > MemoryRegion *rom_memory; > > ram_addr_t lowmem; > > uint64_t hole64_size; > > - Object *i440fx_host; > > > > /* > > * Calculate ram split, for memory below and above 4G. It's a bit > > @@ -198,17 +197,43 @@ static void pc_init1(MachineState *machine, > > } > > > > if (pcmc->pci_enabled) { > > + Object *phb; > > + > > pci_memory = g_new(MemoryRegion, 1); > > memory_region_init(pci_memory, NULL, "pci", UINT64_MAX); > > rom_memory = pci_memory; > > - i440fx_host = OBJECT(qdev_new(host_type)); > > - hole64_size = object_property_get_uint(i440fx_host, > > + > > + phb = OBJECT(qdev_new(host_type)); > > + object_property_add_child(OBJECT(machine), "i440fx", phb); > > + object_property_set_link(phb, PCI_HOST_PROP_RAM_MEM, > > + OBJECT(ram_memory), &error_fatal); > > + object_property_set_link(phb, PCI_HOST_PROP_PCI_MEM, > > + OBJECT(pci_memory), &error_fatal); > > + object_property_set_link(phb, PCI_HOST_PROP_SYSTEM_MEM, > > + OBJECT(system_memory), &error_fatal); > > + object_property_set_link(phb, PCI_HOST_PROP_IO_MEM, > > + OBJECT(system_io), &error_fatal); > > + object_property_set_uint(phb, PCI_HOST_BELOW_4G_MEM_SIZE, > > + x86ms->below_4g_mem_size, &error_fatal); > > + object_property_set_uint(phb, PCI_HOST_ABOVE_4G_MEM_SIZE, > > + x86ms->above_4g_mem_size, &error_fatal); > > + object_property_set_str(phb, I440FX_HOST_PROP_PCI_TYPE, pci_type, > > + &error_fatal); > > + sysbus_realize_and_unref(SYS_BUS_DEVICE(phb), &error_fatal); > > + > > + pci_bus = PCI_BUS(qdev_get_child_bus(DEVICE(phb), "pci.0")); > > + pci_bus_map_irqs(pci_bus, > > + xen_enabled() ? xen_pci_slot_get_pirq > > + : pc_pci_slot_get_pirq); > > + pcms->bus = pci_bus; > > + > > + hole64_size = object_property_get_uint(phb, > > > > PCI_HOST_PROP_PCI_HOLE64_SIZE, > > &error_abort); > > before patch memory region links were set after the original > regions were initialized by pc_memory_init(), but after this > patch you 1st set links and only later pc_memory_init(). > I doesn't look to me as a safe thing to do. or maybe it doesn't matter, but still I have hard time convincing myself that it is so. > > > } else { > > > > pci_memory = NULL; > > rom_memory = system_memory; > > - i440fx_host = NULL; > > + pci_bus = NULL; > > hole64_size = 0; > > is it possible to turn these into initializers, and get rid of > 'else' branch? > > > } > > > > @@ -243,29 +268,6 @@ static void pc_init1(MachineState *machine, > > PIIX3State *piix3; > > PCIDevice *pci_dev; > > > > - object_property_add_child(OBJECT(machine), "i440fx", i440fx_host); > > - object_property_set_link(i440fx_host, PCI_HOST_PROP_RAM_MEM, > > - OBJECT(ram_memory), &error_fatal); > > - object_property_set_link(i440fx_host, PCI_HOST_PROP_PCI_MEM, > > - OBJECT(pci_memory), &error_fatal); > > - object_property_set_link(i440fx_host, PCI_HOST_PROP_SYSTEM_MEM, > > - OBJECT(system_memory), &error_fatal); > > - object_property_set_link(i440fx_host, PCI_HOST_PROP_IO_MEM, > > - OBJECT(system_io), &error_fatal); > > - object_property_set_uint(i440fx_host, PCI_HOST_BELOW_4G_MEM_SIZE, > > - x86ms->below_4g_mem_size, &error_fatal); > > - object_property_set_uint(i440fx_host, PCI_HOST_ABOVE_4G_MEM_SIZE, > > - x86ms->above_4g_mem_size, &error_fatal); > > - object_property_set_str(i440fx_host, I440FX_HOST_PROP_PCI_TYPE, > > - pci_type, &error_fatal); > > - sysbus_realize_and_unref(SYS_BUS_DEVICE(i440fx_host), > > &error_fatal); > > - > > - pci_bus = PCI_BUS(qdev_get_child_bus(DEVICE(i440fx_host), > > "pci.0")); > > - pci_bus_map_irqs(pci_bus, > > - xen_enabled() ? xen_pci_slot_get_pirq > > - : pc_pci_slot_get_pirq); > > - pcms->bus = pci_bus; > > - > > pci_dev = pci_create_simple_multifunction(pci_bus, -1, true, > > TYPE_PIIX3_DEVICE); > > > > @@ -290,7 +292,6 @@ static void pc_init1(MachineState *machine, > > rtc_state = > > ISA_DEVICE(object_resolve_path_component(OBJECT(pci_dev), > > "rtc")); > > } else { > > - pci_bus = NULL; > > isa_bus = isa_bus_new(NULL, system_memory, system_io, > > &error_abort); > > >