| Message ID | 20230611103412.12109-16-shentey@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Q35 and I440FX host bridge QOM cleanup | expand |
On 11/6/23 12:34, Bernhard Beschow 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 <shentey@gmail.com> > --- > hw/i386/pc_piix.c | 57 ++++++++++++++++++++++++----------------------- > 1 file changed, 29 insertions(+), 28 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On Sun, 11 Jun 2023 12:34:12 +0200 Bernhard Beschow <shentey@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 <shentey@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. > } 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); >
On Mon, 12 Jun 2023 16:51:55 +0200 Igor Mammedov <imammedo@redhat.com> wrote: > On Sun, 11 Jun 2023 12:34:12 +0200 > Bernhard Beschow <shentey@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 <shentey@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); > > >
Am 12. Juni 2023 15:21:19 UTC schrieb Igor Mammedov <imammedo@redhat.com>: >On Mon, 12 Jun 2023 16:51:55 +0200 >Igor Mammedov <imammedo@redhat.com> wrote: > >> On Sun, 11 Jun 2023 12:34:12 +0200 >> Bernhard Beschow <shentey@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 <shentey@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. > >> >> > } 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); >> > >> >
On Mon, 12 Jun 2023 17:49:10 +0000 Bernhard Beschow <shentey@gmail.com> wrote: > Am 12. Juni 2023 15:21:19 UTC schrieb Igor Mammedov <imammedo@redhat.com>: > >On Mon, 12 Jun 2023 16:51:55 +0200 > >Igor Mammedov <imammedo@redhat.com> wrote: > > > >> On Sun, 11 Jun 2023 12:34:12 +0200 > >> Bernhard Beschow <shentey@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 <shentey@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. > > > > >> > >> > } 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); > >> > > >> > > >
Am 13. Juni 2023 09:52:50 UTC schrieb Igor Mammedov <imammedo@redhat.com>: >On Mon, 12 Jun 2023 17:49:10 +0000 >Bernhard Beschow <shentey@gmail.com> wrote: > >> Am 12. Juni 2023 15:21:19 UTC schrieb Igor Mammedov <imammedo@redhat.com>: >> >On Mon, 12 Jun 2023 16:51:55 +0200 >> >Igor Mammedov <imammedo@redhat.com> wrote: >> > >> >> On Sun, 11 Jun 2023 12:34:12 +0200 >> >> Bernhard Beschow <shentey@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 <shentey@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); >> >> > >> >> >> > >> >
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); } else { pci_memory = NULL; rom_memory = system_memory; - i440fx_host = NULL; + pci_bus = NULL; hole64_size = 0; } @@ -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);
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 <shentey@gmail.com> --- hw/i386/pc_piix.c | 57 ++++++++++++++++++++++++----------------------- 1 file changed, 29 insertions(+), 28 deletions(-)