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

[Bernhard Beschow]

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