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

[Igor Mammedov]

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