| Message ID | 20230127164718.98156-5-shentey@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | PC cleanups | expand |
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.) > 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; 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, >
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. > >> 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, >>
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, >>> > >
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; 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,
The variable is redundant to "phb" and is never used by its real type. Signed-off-by: Bernhard Beschow <shentey@gmail.com> --- hw/i386/pc_q35.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)