[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 15/15] hw/i386/pc_piix: Move i440fx' realize near its qdev_ne
From: |
Bernhard Beschow |
Subject: |
Re: [PATCH 15/15] hw/i386/pc_piix: Move i440fx' realize near its qdev_new() |
Date: |
Mon, 26 Jun 2023 06:50:02 +0000 |
Am 13. Juni 2023 09:52:50 UTC schrieb Igor Mammedov <imammedo@redhat.com>:
>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.
Hi Igor,
sorry for the late answer. I think I have missed your reply so far due to KVM
forum ;)
The order in which the overlapped mappings are added shouldn't matter as long
as different priorities are supplied. AFAIR adding overlapping regions with the
same priority would be a programming mistake (in existing code). To rule this
out I compared the memory mappings before and after the patch and put the
result in the commit message. It was the same for `info mtree -f`: no
difference except the order of the printout.
I might be able to send an updated version of this series later today. If you'd
have further comments after it is out we can continue discussing there.
Best regards,
Bernhard
>
>>
>> >
>> >>
>> >> > } 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);
>> >> >
>> >>
>> >
>>
>
- Re: [PATCH 09/15] hw/pci-host/i440fx: Move i440fx_realize() into PCII440FXState section, (continued)
[PATCH 12/15] hw/pci-host/i440fx: Add PCI_HOST_{ABOVE, BELOW}_4G_MEM_SIZE properties, Bernhard Beschow, 2023/06/11
[PATCH 14/15] hw/pci-host/i440fx: Resolve i440fx_init(), Bernhard Beschow, 2023/06/11
[PATCH 13/15] hw/pci-host/i440fx: Add I440FX_HOST_PROP_PCI_TYPE property, Bernhard Beschow, 2023/06/11