Re: [PATCH 15/15] hw/i386/pc_piix: Move i440fx' realize near its qdev_new()

[Igor Mammedov]

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);
>