Am 13. Juni 2023 09:52:50 UTC schrieb Igor Mammedov <imamm...@redhat.com>: >On Mon, 12 Jun 2023 17:49:10 +0000 >Bernhard Beschow <shen...@gmail.com> wrote: > >> Am 12. Juni 2023 15:21:19 UTC schrieb Igor Mammedov <imamm...@redhat.com>: >> >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. >> >> AFAICS both pc_memory_init() and i440fx_pcihost_realize() rely on >> memory_region_init*() having been called on these pointers already. All they >> seem to do is adding their sub regions. The order in which this happens >> seems to be irrelevant, otherwise we'd see changes in the QOM console calls >> I guess. > >that's why I said it might not matter, but ... >the thing is that now mapping into AS happens in reversed order >and with overlapped mappings reversed I'm quite unsure if >that is correct. Hi Igor, sorry for the late answer. I think I have missed your reply so far due to KVM forum ;) The order in which the overlapped mappings are added shouldn't matter as long as different priorities are supplied. AFAIR adding overlapping regions with the same priority would be a programming mistake (in existing code). To rule this out I compared the memory mappings before and after the patch and put the result in the commit message. It was the same for `info mtree -f`: no difference except the order of the printout. I might be able to send an updated version of this series later today. If you'd have further comments after it is out we can continue discussing there. Best regards, Bernhard > >> >> > >> >> >> >> > } 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? >> >> Sure, this is possible. I'd add another patch before this one. >> >> Best regards, >> Bernhard >> >> >> >> > } >> >> > >> >> > @@ -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); >> >> > >> >> >> > >> >