Re: [PATCH v2 14/16] hw/pci-host/i440fx: Resolve i440fx_init()

[Bernhard Beschow]

Am 29. Juni 2023 07:50:10 UTC schrieb "Philippe Mathieu-Daudé" 
<philmd@linaro.org>:
>Hi Bernhard,

Hi Phil,

>
>On 28/6/23 21:52, Bernhard Beschow wrote:
>> i440fx_init() is a legacy init function. The previous patches worked towards
>> TYPE_I440FX_PCI_HOST_BRIDGE to be instantiated the QOM way. Do this now by
>> transforming the parameters passed to i440fx_init() into property 
>> assignments.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   include/hw/pci-host/i440fx.h | 10 ----------
>>   hw/i386/pc_piix.c            | 30 +++++++++++++++++++++---------
>>   hw/pci-host/i440fx.c         | 34 +++++-----------------------------
>>   3 files changed, 26 insertions(+), 48 deletions(-)
>> 
>> diff --git a/include/hw/pci-host/i440fx.h b/include/hw/pci-host/i440fx.h
>> index 2d7bae5a45..c988f70890 100644
>> --- a/include/hw/pci-host/i440fx.h
>> +++ b/include/hw/pci-host/i440fx.h
>> @@ -34,14 +34,4 @@ struct PCII440FXState {
>>     #define TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE "igd-passthrough-i440FX"
>>   -PCIBus *i440fx_init(const char *pci_type,
>> -                    DeviceState *dev,
>> -                    MemoryRegion *address_space_mem,
>> -                    MemoryRegion *address_space_io,
>> -                    ram_addr_t below_4g_mem_size,
>> -                    ram_addr_t above_4g_mem_size,
>> -                    MemoryRegion *pci_memory,
>> -                    MemoryRegion *ram_memory);
>> -
>> -
>>   #endif
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 87bee368fc..1df309b8e2 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -126,7 +126,7 @@ static void pc_init1(MachineState *machine,
>>       MemoryRegion *rom_memory;
>>       ram_addr_t lowmem;
>>       uint64_t hole64_size;
>> -    DeviceState *i440fx_host;
>> +    Object *i440fx_host;
>>         /*
>>        * Calculate ram split, for memory below and above 4G.  It's a bit
>> @@ -201,8 +201,8 @@ static void pc_init1(MachineState *machine,
>>           pci_memory = g_new(MemoryRegion, 1);
>>           memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);
>>           rom_memory = pci_memory;
>> -        i440fx_host = qdev_new(host_type);
>> -        hole64_size = object_property_get_uint(OBJECT(i440fx_host),
>> +        i440fx_host = OBJECT(qdev_new(host_type));
>
>[*]
>
>> +        hole64_size = object_property_get_uint(i440fx_host,
>>                                                  
>> PCI_HOST_PROP_PCI_HOLE64_SIZE,
>>                                                  &error_abort);
>>       } else {
>> @@ -243,12 +243,24 @@ static void pc_init1(MachineState *machine,
>>           PIIX3State *piix3;
>>           PCIDevice *pci_dev;
>>   -        pci_bus = i440fx_init(pci_type,
>> -                              i440fx_host,
>> -                              system_memory, system_io,
>> -                              x86ms->below_4g_mem_size,
>> -                              x86ms->above_4g_mem_size,
>> -                              pci_memory, ram_memory);
>> +        object_property_add_child(OBJECT(machine), "i440fx", i440fx_host);
>
>I'd keep the object_property_add_child() close to qdev_new() in [*].
>Matter of taste...

Okay. I'd add a dedicated patch before this one since it has value in its own 
(removal of qdev_get_machine() usage *and* doing what you propose).

Best regards,
Bernhard

>
>> +        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);
>> diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
>> index e8e66afc11..62d6287681 100644
>> --- a/hw/pci-host/i440fx.c
>> +++ b/hw/pci-host/i440fx.c
>> @@ -249,9 +249,14 @@ static void i440fx_pcihost_initfn(Object *obj)
>>     static void i440fx_pcihost_realize(DeviceState *dev, Error **errp)
>>   {
>> +    ERRP_GUARD();
>
>Unrelated change?
>
>Otherwise:
>Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
>>       I440FXState *s = I440FX_PCI_HOST_BRIDGE(dev);
>>       PCIHostState *phb = PCI_HOST_BRIDGE(dev);
>>       SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>> +    PCIBus *b;
>> +    PCIDevice *d;
>> +    PCII440FXState *f;
>> +    unsigned i;
>>         memory_region_add_subregion(s->io_memory, 0xcf8, &phb->conf_mem);
>>       sysbus_init_ioports(sbd, 0xcf8, 4);
>> @@ -262,37 +267,10 @@ static void i440fx_pcihost_realize(DeviceState *dev, 
>> Error **errp)
>>       /* register i440fx 0xcf8 port as coalesced pio */
>>       memory_region_set_flush_coalesced(&phb->data_mem);
>>       memory_region_add_coalescing(&phb->conf_mem, 0, 4);
>> -}
>> -
>> -PCIBus *i440fx_init(const char *pci_type,
>> -                    DeviceState *dev,
>> -                    MemoryRegion *address_space_mem,
>> -                    MemoryRegion *address_space_io,
>> -                    ram_addr_t below_4g_mem_size,
>> -                    ram_addr_t above_4g_mem_size,
>> -                    MemoryRegion *pci_address_space,
>> -                    MemoryRegion *ram_memory)
>> -{
>> -    I440FXState *s = I440FX_PCI_HOST_BRIDGE(dev);
>> -    PCIHostState *phb = PCI_HOST_BRIDGE(dev);
>> -    PCIBus *b;
>> -    PCIDevice *d;
>> -    PCII440FXState *f;
>> -    unsigned i;
>> -
>> -    s->system_memory = address_space_mem;
>> -    s->io_memory = address_space_io;
>> -    s->pci_address_space = pci_address_space;
>> -    s->ram_memory = ram_memory;
>> -    s->below_4g_mem_size = below_4g_mem_size;
>> -    s->above_4g_mem_size = above_4g_mem_size;
>> -    s->pci_type = (char *)pci_type;
>>         b = pci_root_bus_new(dev, NULL, s->pci_address_space,
>>                            s->io_memory, 0, TYPE_PCI_BUS);
>>       phb->bus = b;
>> -    object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev));
>> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>>         d = pci_create_simple(b, 0, s->pci_type);
>>       f = I440FX_PCI_DEVICE(d);
>> @@ -336,8 +314,6 @@ PCIBus *i440fx_init(const char *pci_type,
>>       d->config[I440FX_COREBOOT_RAM_SIZE] = ram_size;
>>         i440fx_update_memory_mappings(f);
>> -
>> -    return b;
>>   }
>>     static void i440fx_class_init(ObjectClass *klass, void *data)
>