Re: [PATCH 4/7] hw/i386/pc_q35: Resolve redundant q35_host variable

[BALATON Zoltan]

On Sun, 29 Jan 2023, Bernhard Beschow wrote:
> Am 27. Januar 2023 19:22:49 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
> > On Fri, 27 Jan 2023, Bernhard Beschow wrote:
> > > The variable is redundant to "phb" and is never used by its real type.
> >
> > Also replace qdev_get_machine() with reference already passed to init function. 
> > (Maybe worth mentioning in commit message even if too small change for a 
> > separate patch.)
>
> Indeed, this can easily be missed. A separate patch allows for clean 
> justification.

I think just adding a sentence to commit message to clarify it is enough 
no need to split it out but up to you.

Regards,
BALATON Zoltan

> > > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > > ---
> > > hw/i386/pc_q35.c | 22 ++++++++++------------
> > > 1 file changed, 10 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > > index 83c57c6eb1..dc94ce1081 100644
> > > --- a/hw/i386/pc_q35.c
> > > +++ b/hw/i386/pc_q35.c
> > > @@ -118,7 +118,6 @@ static void pc_q35_init(MachineState *machine)
> > >     PCMachineState *pcms = PC_MACHINE(machine);
> > >     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> > >     X86MachineState *x86ms = X86_MACHINE(machine);
> > > -    Q35PCIHost *q35_host;
> > >     PCIHostState *phb;
> >
> > The phb variable itself is only used once to get the bus from it and it's 
> > mostly cast to Object * so maybe store it in a variable of that type to remove 
> > most of the casts and IMO you can also remove PCIHostState *phb and just use:
> >
> > host_bus = PCI_HOST_BRIDGE(phost)->bus;
>
> Maybe one could also fish out the PCI bus using qdev_get_child_bus() like we do 
> in pc_piix for the ISA bus already. Hm...
>
> Best regards,
> Bernhard
> > Regards,
> > BALATON Zoltan
> >
> > >     PCIBus *host_bus;
> > >     PCIDevice *lpc;
> > > @@ -206,10 +205,10 @@ static void pc_q35_init(MachineState *machine)
> > >     }
> > >
> > >     /* create pci host bus */
> > > -    q35_host = Q35_HOST_DEVICE(qdev_new(TYPE_Q35_HOST_DEVICE));
> > > +    phb = PCI_HOST_BRIDGE(qdev_new(TYPE_Q35_HOST_DEVICE));
> > >
> > >     if (pcmc->pci_enabled) {
> > > -        pci_hole64_size = object_property_get_uint(OBJECT(q35_host),
> > > +        pci_hole64_size = object_property_get_uint(OBJECT(phb),
> > >                                                    
> > > PCI_HOST_PROP_PCI_HOLE64_SIZE,
> > >                                                    &error_abort);
> > >     }
> > > @@ -218,22 +217,21 @@ static void pc_q35_init(MachineState *machine)
> > >     pc_memory_init(pcms, get_system_memory(), rom_memory, &ram_memory,
> > >                    pci_hole64_size);
> > >
> > > -    object_property_add_child(qdev_get_machine(), "q35", OBJECT(q35_host));
> > > -    object_property_set_link(OBJECT(q35_host), MCH_HOST_PROP_RAM_MEM,
> > > +    object_property_add_child(OBJECT(machine), "q35", OBJECT(phb));
> > > +    object_property_set_link(OBJECT(phb), MCH_HOST_PROP_RAM_MEM,
> > >                              OBJECT(ram_memory), NULL);
> > > -    object_property_set_link(OBJECT(q35_host), MCH_HOST_PROP_PCI_MEM,
> > > +    object_property_set_link(OBJECT(phb), MCH_HOST_PROP_PCI_MEM,
> > >                              OBJECT(pci_memory), NULL);
> > > -    object_property_set_link(OBJECT(q35_host), MCH_HOST_PROP_SYSTEM_MEM,
> > > +    object_property_set_link(OBJECT(phb), MCH_HOST_PROP_SYSTEM_MEM,
> > >                              OBJECT(get_system_memory()), NULL);
> > > -    object_property_set_link(OBJECT(q35_host), MCH_HOST_PROP_IO_MEM,
> > > +    object_property_set_link(OBJECT(phb), MCH_HOST_PROP_IO_MEM,
> > >                              OBJECT(system_io), NULL);
> > > -    object_property_set_int(OBJECT(q35_host), PCI_HOST_BELOW_4G_MEM_SIZE,
> > > +    object_property_set_int(OBJECT(phb), PCI_HOST_BELOW_4G_MEM_SIZE,
> > >                             x86ms->below_4g_mem_size, NULL);
> > > -    object_property_set_int(OBJECT(q35_host), PCI_HOST_ABOVE_4G_MEM_SIZE,
> > > +    object_property_set_int(OBJECT(phb), PCI_HOST_ABOVE_4G_MEM_SIZE,
> > >                             x86ms->above_4g_mem_size, NULL);
> > >     /* pci */
> > > -    sysbus_realize_and_unref(SYS_BUS_DEVICE(q35_host), &error_fatal);
> > > -    phb = PCI_HOST_BRIDGE(q35_host);
> > > +    sysbus_realize_and_unref(SYS_BUS_DEVICE(phb), &error_fatal);
> > >     host_bus = phb->bus;
> > >     /* create ISA bus */
> > >     lpc = pci_create_simple_multifunction(host_bus, PCI_DEVFN(ICH9_LPC_DEV,