[PATCH 00/15] Q35 and I440FX host bridge QOM cleanup

[PATCH 00/15] Q35 and I440FX host bridge QOM cleanup

[Bernhard Beschow]

This series resolves the legacy i440fx_init() function and instantiates the
I440FX host bridge the QOM way. As a preparation the Q35 host bridge receives
some cleanup as well.

Most of the Q35 patches have been submitted under [1] before. This series
incorporates only the changes making the two device models consistent with
each other.

The original plan of [1] was to clean up Q35 first and then submit a separate
follow-up I440FX QOM'ification series. This series takes a more direct approach
by cutting down on the changes in both device models while still allowing both
device models to be instantiated the same way. The remaining patches in [1]
would still be doable.

The series is structured as follows: The first 5 patches clean up Q35, the next
patch massages Q35 to share its property names with I440FX. The rest of the
series resolves i440fx_init().

Tesging done:
* `make check`
* `make check-avocado`
* Run `xl create` under Xen with the following config:
    name = "Manjaro"
    type = 'hvm'
    memory = 1536
    apic = 1
    usb = 1
    disk = [ "file:manjaro-kde-21.2.6-220416-linux515.iso,hdc:cdrom,r" ]
    device_model_override = "/usr/bin/qemu-system-x86_64"
    vga = "stdvga"
    sdl = 1

[1] https://patchew.org/QEMU/20230304152648.103749-1-shentey@gmail.com/

Bernhard Beschow (15):
  hw/i386/pc_q35: Resolve redundant q35_host variable
  hw/pci-host/q35: Fix double, contradicting .endianness assignment
  hw/pci-host/q35: Initialize PCMachineState::bus in board code
  hw/pci/pci_host: Introduce PCI_HOST_BYPASS_IOMMU macro
  hw/pci-host/q35: Initialize PCI_HOST_BYPASS_IOMMU property from board
    code
  hw/pci-host/q35: Make some property name macros reusable by i440fx
  hw/pci-host/i440fx: Replace magic values by existing constants
  hw/pci-host/i440fx: Have common names for some local variables
  hw/pci-host/i440fx: Move i440fx_realize() into PCII440FXState section
  hw/pci-host/i440fx: Make MemoryRegion pointers accessible as
    properties
  hw/pci-host/i440fx: Add PCI_HOST_PROP_IO_MEM property
  hw/pci-host/i440fx: Add PCI_HOST_{ABOVE,BELOW}_4G_MEM_SIZE properties
  hw/pci-host/i440fx: Add I440FX_HOST_PROP_PCI_TYPE property
  hw/pci-host/i440fx: Resolve i440fx_init()
  hw/i386/pc_piix: Move i440fx' realize near its qdev_new()

 include/hw/i386/pc.h         |   4 ++
 include/hw/pci-host/i440fx.h |  16 +----
 include/hw/pci-host/q35.h    |   5 --
 include/hw/pci/pci_host.h    |   2 +
 hw/i386/pc_piix.c            |  48 ++++++++-----
 hw/i386/pc_q35.c             |  31 +++++----
 hw/pci-host/i440fx.c         | 130 +++++++++++++++++++----------------
 hw/pci-host/q35.c            |  13 ++--
 hw/pci/pci_host.c            |   2 +-
 9 files changed, 134 insertions(+), 117 deletions(-)

-- 
2.41.0

[PATCH 01/15] hw/i386/pc_q35: Resolve redundant q35_host variable

[Bernhard Beschow]

The variable is redundant to "phb" and is never used by its real type.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/i386/pc_q35.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 6155427e48..62c5d652b7 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -120,8 +120,7 @@ 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;
+    Object *phb;
     PCIBus *host_bus;
     PCIDevice *lpc;
     DeviceState *lpc_dev;
@@ -207,10 +206,10 @@ static void pc_q35_init(MachineState *machine)
     }
 
     /* create pci host bus */
-    q35_host = Q35_HOST_DEVICE(qdev_new(TYPE_Q35_HOST_DEVICE));
+    phb = OBJECT(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(phb,
                                                    PCI_HOST_PROP_PCI_HOLE64_SIZE,
                                                    &error_abort);
     }
@@ -218,23 +217,23 @@ static void pc_q35_init(MachineState *machine)
     /* allocate ram and load rom/bios */
     pc_memory_init(pcms, system_memory, rom_memory, pci_hole64_size);
 
-    object_property_add_child(OBJECT(machine), "q35", OBJECT(q35_host));
-    object_property_set_link(OBJECT(q35_host), MCH_HOST_PROP_RAM_MEM,
+    object_property_add_child(OBJECT(machine), "q35", phb);
+    object_property_set_link(phb, MCH_HOST_PROP_RAM_MEM,
                              OBJECT(machine->ram), NULL);
-    object_property_set_link(OBJECT(q35_host), MCH_HOST_PROP_PCI_MEM,
+    object_property_set_link(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(phb, MCH_HOST_PROP_SYSTEM_MEM,
                              OBJECT(system_memory), NULL);
-    object_property_set_link(OBJECT(q35_host), MCH_HOST_PROP_IO_MEM,
+    object_property_set_link(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(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(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);
-    host_bus = phb->bus;
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(phb), &error_fatal);
+    host_bus = PCI_BUS(qdev_get_child_bus(DEVICE(phb), "pcie.0"));
     /* create ISA bus */
     lpc = pci_new_multifunction(PCI_DEVFN(ICH9_LPC_DEV, ICH9_LPC_FUNC), true,
                                 TYPE_ICH9_LPC_DEVICE);
-- 
2.41.0

[PATCH 02/15] hw/pci-host/q35: Fix double, contradicting .endianness assignment

[Bernhard Beschow]

Fixes the following clangd warning (-Winitializer-overrides):

  q35.c:297:19: Initializer overrides prior initialization of this subobject
  q35.c:292:19: previous initialization is here

Settle on native endian which causes the least overhead.

Fixes: bafc90bdc594 ("q35: implement TSEG")
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/pci-host/q35.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index fd18920e7f..859c197f25 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -290,7 +290,6 @@ static const MemoryRegionOps blackhole_ops = {
     .valid.max_access_size = 4,
     .impl.min_access_size = 4,
     .impl.max_access_size = 4,
-    .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
 /* PCIe MMCFG */
-- 
2.41.0

Re: [PATCH 02/15] hw/pci-host/q35: Fix double, contradicting .endianness assignment

[Igor Mammedov]

On Sun, 11 Jun 2023 12:33:59 +0200
Bernhard Beschow <shentey@gmail.com> wrote:

> Fixes the following clangd warning (-Winitializer-overrides):
> 
>   q35.c:297:19: Initializer overrides prior initialization of this subobject
>   q35.c:292:19: previous initialization is here
> 
> Settle on native endian which causes the least overhead.
indeed it doesn't matter which way we read all ones, so that should work.
but does it really matter (I mean the overhead/what workload)?
If not, I'd prefer explicit LE as it's now to be consistent
the the rest of memops on Q35.

> 
> Fixes: bafc90bdc594 ("q35: implement TSEG")
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>  hw/pci-host/q35.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index fd18920e7f..859c197f25 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -290,7 +290,6 @@ static const MemoryRegionOps blackhole_ops = {
>      .valid.max_access_size = 4,
>      .impl.min_access_size = 4,
>      .impl.max_access_size = 4,
> -    .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>  
>  /* PCIe MMCFG */

Re: [PATCH 02/15] hw/pci-host/q35: Fix double, contradicting .endianness assignment

[Bernhard Beschow]

[-- Attachment #1: Type: text/plain, Size: 1612 bytes --]

On Mon, Jun 12, 2023 at 3:01 PM Igor Mammedov <imammedo@redhat.com> wrote:

> On Sun, 11 Jun 2023 12:33:59 +0200
> Bernhard Beschow <shentey@gmail.com> wrote:
>
> > Fixes the following clangd warning (-Winitializer-overrides):
> >
> >   q35.c:297:19: Initializer overrides prior initialization of this
> subobject
> >   q35.c:292:19: previous initialization is here
> >
> > Settle on native endian which causes the least overhead.
> indeed it doesn't matter which way we read all ones, so that should work.
> but does it really matter (I mean the overhead/what workload)?
> If not, I'd prefer explicit LE as it's now to be consistent
> the the rest of memops on Q35.
>

I got a comment from Michael about this in [1], so I've changed it. I don't
mind changing it either way.

Best regards,
Bernhard

[1]
https://patchew.org/QEMU/20230214131441.101760-1-shentey@gmail.com/20230214131441.101760-3-shentey@gmail.com/#20230301164339-mutt-send-email-mst@kernel.org

>
> >
> > Fixes: bafc90bdc594 ("q35: implement TSEG")
> > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > ---
> >  hw/pci-host/q35.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> > index fd18920e7f..859c197f25 100644
> > --- a/hw/pci-host/q35.c
> > +++ b/hw/pci-host/q35.c
> > @@ -290,7 +290,6 @@ static const MemoryRegionOps blackhole_ops = {
> >      .valid.max_access_size = 4,
> >      .impl.min_access_size = 4,
> >      .impl.max_access_size = 4,
> > -    .endianness = DEVICE_LITTLE_ENDIAN,
> >  };
> >
> >  /* PCIe MMCFG */
>
>

[-- Attachment #2: Type: text/html, Size: 2559 bytes --]

Re: [PATCH 02/15] hw/pci-host/q35: Fix double, contradicting .endianness assignment

[Michael S. Tsirkin]

On Tue, Jun 13, 2023 at 09:46:53AM +0200, Bernhard Beschow wrote:
> On Mon, Jun 12, 2023 at 3:01 PM Igor Mammedov <imammedo@redhat.com> wrote:
> 
>     On Sun, 11 Jun 2023 12:33:59 +0200
>     Bernhard Beschow <shentey@gmail.com> wrote:
> 
>     > Fixes the following clangd warning (-Winitializer-overrides):
>     >
>     >   q35.c:297:19: Initializer overrides prior initialization of this
>     subobject
>     >   q35.c:292:19: previous initialization is here
>     >
>     > Settle on native endian which causes the least overhead.
>     indeed it doesn't matter which way we read all ones, so that should work.
>     but does it really matter (I mean the overhead/what workload)?
>     If not, I'd prefer explicit LE as it's now to be consistent
>     the the rest of memops on Q35.
> 
> 
> I got a comment from Michael about this in [1], so I've changed it. I don't
> mind changing it either way.
> 
> Best regards,
> Bernhard
> 
> [1] https://patchew.org/QEMU/20230214131441.101760-1-shentey@gmail.com/
> 20230214131441.101760-3-shentey@gmail.com/#
> 20230301164339-mutt-send-email-mst@kernel.org

Hmm it's not terribly important, and the optimization is trivial,
but yes people tend to copy code, good point. Maybe add a comment?
/*
 * Note: don't copy this!  normally use DEVICE_LITTLE_ENDIAN. This only
 * works because we don't allow writes and always read all-ones.
 */


> 
>     >
>     > Fixes: bafc90bdc594 ("q35: implement TSEG")
>     > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>     > ---
>     >  hw/pci-host/q35.c | 1 -
>     >  1 file changed, 1 deletion(-)
>     >
>     > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
>     > index fd18920e7f..859c197f25 100644
>     > --- a/hw/pci-host/q35.c
>     > +++ b/hw/pci-host/q35.c
>     > @@ -290,7 +290,6 @@ static const MemoryRegionOps blackhole_ops = {
>     >      .valid.max_access_size = 4,
>     >      .impl.min_access_size = 4,
>     >      .impl.max_access_size = 4,
>     > -    .endianness = DEVICE_LITTLE_ENDIAN,
>     >  };
>     > 
>     >  /* PCIe MMCFG */
> 
> 

Re: [PATCH 02/15] hw/pci-host/q35: Fix double, contradicting .endianness assignment

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2427 bytes --]

On Tue, 13 Jun 2023, Michael S. Tsirkin wrote:
> On Tue, Jun 13, 2023 at 09:46:53AM +0200, Bernhard Beschow wrote:
>> On Mon, Jun 12, 2023 at 3:01 PM Igor Mammedov <imammedo@redhat.com> wrote:
>>
>>     On Sun, 11 Jun 2023 12:33:59 +0200
>>     Bernhard Beschow <shentey@gmail.com> wrote:
>>
>>    > Fixes the following clangd warning (-Winitializer-overrides):
>>    >
>>    >   q35.c:297:19: Initializer overrides prior initialization of this
>>     subobject
>>    >   q35.c:292:19: previous initialization is here
>>    >
>>    > Settle on native endian which causes the least overhead.
>>     indeed it doesn't matter which way we read all ones, so that should work.
>>     but does it really matter (I mean the overhead/what workload)?
>>     If not, I'd prefer explicit LE as it's now to be consistent
>>     the the rest of memops on Q35.
>>
>>
>> I got a comment from Michael about this in [1], so I've changed it. I don't
>> mind changing it either way.
>>
>> Best regards,
>> Bernhard
>>
>> [1] https://patchew.org/QEMU/20230214131441.101760-1-shentey@gmail.com/
>> 20230214131441.101760-3-shentey@gmail.com/#
>> 20230301164339-mutt-send-email-mst@kernel.org
>
> Hmm it's not terribly important, and the optimization is trivial,
> but yes people tend to copy code, good point. Maybe add a comment?
> /*
> * Note: don't copy this!  normally use DEVICE_LITTLE_ENDIAN. This only
> * works because we don't allow writes and always read all-ones.
> */

Why don't you leave DEVICE_LITTLE_ENDIAN and remove DEVICE_NATIVE_ENDIAN 
instead? If this only returns all 1s then it does not matter and also 
DEVICE_LITTLE_ENDIAN was the last assignment so likely that was effective 
so far anyway.

Regards,
BALATON Zoltan

>>
>>    >
>>    > Fixes: bafc90bdc594 ("q35: implement TSEG")
>>    > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>    > ---
>>    >  hw/pci-host/q35.c | 1 -
>>    >  1 file changed, 1 deletion(-)
>>    >
>>    > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
>>    > index fd18920e7f..859c197f25 100644
>>    > --- a/hw/pci-host/q35.c
>>    > +++ b/hw/pci-host/q35.c
>>    > @@ -290,7 +290,6 @@ static const MemoryRegionOps blackhole_ops = {
>>    >      .valid.max_access_size = 4,
>>    >      .impl.min_access_size = 4,
>>    >      .impl.max_access_size = 4,
>>    > -    .endianness = DEVICE_LITTLE_ENDIAN,
>>    >  };
>>    > 
>>    >  /* PCIe MMCFG */
>>
>>
>
>
>

Re: [PATCH 02/15] hw/pci-host/q35: Fix double, contradicting .endianness assignment

[Igor Mammedov]

On Tue, 13 Jun 2023 13:07:17 +0200 (CEST)
BALATON Zoltan <balaton@eik.bme.hu> wrote:

> On Tue, 13 Jun 2023, Michael S. Tsirkin wrote:
> > On Tue, Jun 13, 2023 at 09:46:53AM +0200, Bernhard Beschow wrote:  
> >> On Mon, Jun 12, 2023 at 3:01 PM Igor Mammedov <imammedo@redhat.com> wrote:
> >>
> >>     On Sun, 11 Jun 2023 12:33:59 +0200
> >>     Bernhard Beschow <shentey@gmail.com> wrote:
> >>  
> >>    > Fixes the following clangd warning (-Winitializer-overrides):
> >>    >
> >>    >   q35.c:297:19: Initializer overrides prior initialization of this  
> >>     subobject  
> >>    >   q35.c:292:19: previous initialization is here
> >>    >
> >>    > Settle on native endian which causes the least overhead.  
> >>     indeed it doesn't matter which way we read all ones, so that should work.
> >>     but does it really matter (I mean the overhead/what workload)?
> >>     If not, I'd prefer explicit LE as it's now to be consistent
> >>     the the rest of memops on Q35.
> >>
> >>
> >> I got a comment from Michael about this in [1], so I've changed it. I don't
> >> mind changing it either way.
> >>
> >> Best regards,
> >> Bernhard
> >>
> >> [1] https://patchew.org/QEMU/20230214131441.101760-1-shentey@gmail.com/
> >> 20230214131441.101760-3-shentey@gmail.com/#
> >> 20230301164339-mutt-send-email-mst@kernel.org  
> >
> > Hmm it's not terribly important, and the optimization is trivial,
> > but yes people tend to copy code, good point. Maybe add a comment?
> > /*
> > * Note: don't copy this!  normally use DEVICE_LITTLE_ENDIAN. This only
> > * works because we don't allow writes and always read all-ones.
> > */  
> 
> Why don't you leave DEVICE_LITTLE_ENDIAN and remove DEVICE_NATIVE_ENDIAN 
> instead? If this only returns all 1s then it does not matter and also 
> DEVICE_LITTLE_ENDIAN was the last assignment so likely that was effective 
> so far anyway.

I'm in favor of this as well,
the 'optimization' and then piling comments on top to clarify confusion
should be justified by usefulness of it (no perf numbers/usecase were present so far).
In absence of above, I'd prefer real hw behavior (LE in this case).

> 
> Regards,
> BALATON Zoltan
> 
> >>  
> >>    >
> >>    > Fixes: bafc90bdc594 ("q35: implement TSEG")
> >>    > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> >>    > ---
> >>    >  hw/pci-host/q35.c | 1 -
> >>    >  1 file changed, 1 deletion(-)
> >>    >
> >>    > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> >>    > index fd18920e7f..859c197f25 100644
> >>    > --- a/hw/pci-host/q35.c
> >>    > +++ b/hw/pci-host/q35.c
> >>    > @@ -290,7 +290,6 @@ static const MemoryRegionOps blackhole_ops = {
> >>    >      .valid.max_access_size = 4,
> >>    >      .impl.min_access_size = 4,
> >>    >      .impl.max_access_size = 4,
> >>    > -    .endianness = DEVICE_LITTLE_ENDIAN,
> >>    >  };
> >>    > 
> >>    >  /* PCIe MMCFG */  
> >>
> >>  
> >
> >
> >  

Re: [PATCH 02/15] hw/pci-host/q35: Fix double, contradicting .endianness assignment

[Philippe Mathieu-Daudé]

On 13/6/23 15:05, Igor Mammedov wrote:
> On Tue, 13 Jun 2023 13:07:17 +0200 (CEST)
> BALATON Zoltan <balaton@eik.bme.hu> wrote:
> 
>> On Tue, 13 Jun 2023, Michael S. Tsirkin wrote:
>>> On Tue, Jun 13, 2023 at 09:46:53AM +0200, Bernhard Beschow wrote:
>>>> On Mon, Jun 12, 2023 at 3:01 PM Igor Mammedov <imammedo@redhat.com> wrote:
>>>>
>>>>      On Sun, 11 Jun 2023 12:33:59 +0200
>>>>      Bernhard Beschow <shentey@gmail.com> wrote:
>>>>   
>>>>     > Fixes the following clangd warning (-Winitializer-overrides):
>>>>     >
>>>>     >   q35.c:297:19: Initializer overrides prior initialization of this
>>>>      subobject
>>>>     >   q35.c:292:19: previous initialization is here
>>>>     >
>>>>     > Settle on native endian which causes the least overhead.
>>>>      indeed it doesn't matter which way we read all ones, so that should work.
>>>>      but does it really matter (I mean the overhead/what workload)?
>>>>      If not, I'd prefer explicit LE as it's now to be consistent
>>>>      the the rest of memops on Q35.

Meaning trying to optimize this single MR on big-endian is irrelevant :)

>>>>
>>>> I got a comment from Michael about this in [1], so I've changed it. I don't
>>>> mind changing it either way.
>>>>
>>>> Best regards,
>>>> Bernhard
>>>>
>>>> [1] https://patchew.org/QEMU/20230214131441.101760-1-shentey@gmail.com/
>>>> 20230214131441.101760-3-shentey@gmail.com/#
>>>> 20230301164339-mutt-send-email-mst@kernel.org
>>>
>>> Hmm it's not terribly important, and the optimization is trivial,
>>> but yes people tend to copy code, good point. Maybe add a comment?
>>> /*
>>> * Note: don't copy this!  normally use DEVICE_LITTLE_ENDIAN. This only
>>> * works because we don't allow writes and always read all-ones.
>>> */
>>
>> Why don't you leave DEVICE_LITTLE_ENDIAN and remove DEVICE_NATIVE_ENDIAN
>> instead? If this only returns all 1s then it does not matter and also
>> DEVICE_LITTLE_ENDIAN was the last assignment so likely that was effective
>> so far anyway.
> 
> I'm in favor of this as well,
> the 'optimization' and then piling comments on top to clarify confusion
> should be justified by usefulness of it (no perf numbers/usecase were present so far).
> In absence of above, I'd prefer real hw behavior (LE in this case).

Re: [PATCH 02/15] hw/pci-host/q35: Fix double, contradicting .endianness assignment

[Michael S. Tsirkin]

On Tue, Jun 13, 2023 at 03:40:28PM +0200, Philippe Mathieu-Daudé wrote:
> On 13/6/23 15:05, Igor Mammedov wrote:
> > On Tue, 13 Jun 2023 13:07:17 +0200 (CEST)
> > BALATON Zoltan <balaton@eik.bme.hu> wrote:
> > 
> > > On Tue, 13 Jun 2023, Michael S. Tsirkin wrote:
> > > > On Tue, Jun 13, 2023 at 09:46:53AM +0200, Bernhard Beschow wrote:
> > > > > On Mon, Jun 12, 2023 at 3:01 PM Igor Mammedov <imammedo@redhat.com> wrote:
> > > > > 
> > > > >      On Sun, 11 Jun 2023 12:33:59 +0200
> > > > >      Bernhard Beschow <shentey@gmail.com> wrote:
> > > > >     > Fixes the following clangd warning (-Winitializer-overrides):
> > > > >     >
> > > > >     >   q35.c:297:19: Initializer overrides prior initialization of this
> > > > >      subobject
> > > > >     >   q35.c:292:19: previous initialization is here
> > > > >     >
> > > > >     > Settle on native endian which causes the least overhead.
> > > > >      indeed it doesn't matter which way we read all ones, so that should work.
> > > > >      but does it really matter (I mean the overhead/what workload)?
> > > > >      If not, I'd prefer explicit LE as it's now to be consistent
> > > > >      the the rest of memops on Q35.
> 
> Meaning trying to optimize this single MR on big-endian is irrelevant :)
> 
> > > > > 
> > > > > I got a comment from Michael about this in [1], so I've changed it. I don't
> > > > > mind changing it either way.
> > > > > 
> > > > > Best regards,
> > > > > Bernhard
> > > > > 
> > > > > [1] https://patchew.org/QEMU/20230214131441.101760-1-shentey@gmail.com/
> > > > > 20230214131441.101760-3-shentey@gmail.com/#
> > > > > 20230301164339-mutt-send-email-mst@kernel.org
> > > > 
> > > > Hmm it's not terribly important, and the optimization is trivial,
> > > > but yes people tend to copy code, good point. Maybe add a comment?
> > > > /*
> > > > * Note: don't copy this!  normally use DEVICE_LITTLE_ENDIAN. This only
> > > > * works because we don't allow writes and always read all-ones.
> > > > */
> > > 
> > > Why don't you leave DEVICE_LITTLE_ENDIAN and remove DEVICE_NATIVE_ENDIAN
> > > instead? If this only returns all 1s then it does not matter and also
> > > DEVICE_LITTLE_ENDIAN was the last assignment so likely that was effective
> > > so far anyway.
> > 
> > I'm in favor of this as well,
> > the 'optimization' and then piling comments on top to clarify confusion
> > should be justified by usefulness of it (no perf numbers/usecase were present so far).
> > In absence of above, I'd prefer real hw behavior (LE in this case).


OK I'm fine with this too. Sorry about leading you astray.

-- 
MST

Re: [PATCH 02/15] hw/pci-host/q35: Fix double, contradicting .endianness assignment

[Bernhard Beschow]

Am 13. Juni 2023 15:01:40 UTC schrieb "Michael S. Tsirkin" <mst@redhat.com>:
>On Tue, Jun 13, 2023 at 03:40:28PM +0200, Philippe Mathieu-Daudé wrote:
>> On 13/6/23 15:05, Igor Mammedov wrote:
>> > On Tue, 13 Jun 2023 13:07:17 +0200 (CEST)
>> > BALATON Zoltan <balaton@eik.bme.hu> wrote:
>> > 
>> > > On Tue, 13 Jun 2023, Michael S. Tsirkin wrote:
>> > > > On Tue, Jun 13, 2023 at 09:46:53AM +0200, Bernhard Beschow wrote:
>> > > > > On Mon, Jun 12, 2023 at 3:01 PM Igor Mammedov <imammedo@redhat.com> wrote:
>> > > > > 
>> > > > >      On Sun, 11 Jun 2023 12:33:59 +0200
>> > > > >      Bernhard Beschow <shentey@gmail.com> wrote:
>> > > > >     > Fixes the following clangd warning (-Winitializer-overrides):
>> > > > >     >
>> > > > >     >   q35.c:297:19: Initializer overrides prior initialization of this
>> > > > >      subobject
>> > > > >     >   q35.c:292:19: previous initialization is here
>> > > > >     >
>> > > > >     > Settle on native endian which causes the least overhead.
>> > > > >      indeed it doesn't matter which way we read all ones, so that should work.
>> > > > >      but does it really matter (I mean the overhead/what workload)?
>> > > > >      If not, I'd prefer explicit LE as it's now to be consistent
>> > > > >      the the rest of memops on Q35.
>> 
>> Meaning trying to optimize this single MR on big-endian is irrelevant :)
>> 
>> > > > > 
>> > > > > I got a comment from Michael about this in [1], so I've changed it. I don't
>> > > > > mind changing it either way.
>> > > > > 
>> > > > > Best regards,
>> > > > > Bernhard
>> > > > > 
>> > > > > [1] https://patchew.org/QEMU/20230214131441.101760-1-shentey@gmail.com/
>> > > > > 20230214131441.101760-3-shentey@gmail.com/#
>> > > > > 20230301164339-mutt-send-email-mst@kernel.org
>> > > > 
>> > > > Hmm it's not terribly important, and the optimization is trivial,
>> > > > but yes people tend to copy code, good point. Maybe add a comment?
>> > > > /*
>> > > > * Note: don't copy this!  normally use DEVICE_LITTLE_ENDIAN. This only
>> > > > * works because we don't allow writes and always read all-ones.
>> > > > */
>> > > 
>> > > Why don't you leave DEVICE_LITTLE_ENDIAN and remove DEVICE_NATIVE_ENDIAN
>> > > instead? If this only returns all 1s then it does not matter and also
>> > > DEVICE_LITTLE_ENDIAN was the last assignment so likely that was effective
>> > > so far anyway.
>> > 
>> > I'm in favor of this as well,
>> > the 'optimization' and then piling comments on top to clarify confusion
>> > should be justified by usefulness of it (no perf numbers/usecase were present so far).
>> > In absence of above, I'd prefer real hw behavior (LE in this case).
>
>
>OK I'm fine with this too. Sorry about leading you astray.

No worries. I'm happy to change it back to LE.

Best regards,
Bernhard

[PATCH 03/15] hw/pci-host/q35: Initialize PCMachineState::bus in board code

[Bernhard Beschow]

The Q35 PCI host currently sets the PC machine's PCI bus attribute
through global state, thereby assuming the machine to be a PC machine.
The Q35 machine code already holds on to Q35's pci bus attribute, so can
easily set its own property while preserving encapsulation.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/pc_q35.c  | 4 +++-
 hw/pci-host/q35.c | 1 -
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 62c5d652b7..29b46d3e1c 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -230,10 +230,12 @@ static void pc_q35_init(MachineState *machine)
                             x86ms->below_4g_mem_size, NULL);
     object_property_set_int(phb, PCI_HOST_ABOVE_4G_MEM_SIZE,
                             x86ms->above_4g_mem_size, NULL);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(phb), &error_fatal);
 
     /* pci */
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(phb), &error_fatal);
     host_bus = PCI_BUS(qdev_get_child_bus(DEVICE(phb), "pcie.0"));
+    pcms->bus = host_bus;
+
     /* create ISA bus */
     lpc = pci_new_multifunction(PCI_DEVFN(ICH9_LPC_DEV, ICH9_LPC_FUNC), true,
                                 TYPE_ICH9_LPC_DEVICE);
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 859c197f25..23b689dba3 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -66,7 +66,6 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
                                 s->mch.pci_address_space,
                                 s->mch.address_space_io,
                                 0, TYPE_PCIE_BUS);
-    PC_MACHINE(qdev_get_machine())->bus = pci->bus;
     pci->bypass_iommu =
         PC_MACHINE(qdev_get_machine())->default_bus_bypass_iommu;
     qdev_realize(DEVICE(&s->mch), BUS(pci->bus), &error_fatal);
-- 
2.41.0

Re: [PATCH 03/15] hw/pci-host/q35: Initialize PCMachineState::bus in board code

[Philippe Mathieu-Daudé]

On 11/6/23 12:34, Bernhard Beschow wrote:
> The Q35 PCI host currently sets the PC machine's PCI bus attribute
> through global state, thereby assuming the machine to be a PC machine.
> The Q35 machine code already holds on to Q35's pci bus attribute, so can
> easily set its own property while preserving encapsulation.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/i386/pc_q35.c  | 4 +++-
>   hw/pci-host/q35.c | 1 -
>   2 files changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 03/15] hw/pci-host/q35: Initialize PCMachineState::bus in board code

[Igor Mammedov]

On Sun, 11 Jun 2023 12:34:00 +0200
Bernhard Beschow <shentey@gmail.com> wrote:

> The Q35 PCI host currently sets the PC machine's PCI bus attribute
> through global state, thereby assuming the machine to be a PC machine.
> The Q35 machine code already holds on to Q35's pci bus attribute, so can
> easily set its own property while preserving encapsulation.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/i386/pc_q35.c  | 4 +++-
>  hw/pci-host/q35.c | 1 -
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 62c5d652b7..29b46d3e1c 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -230,10 +230,12 @@ static void pc_q35_init(MachineState *machine)
>                              x86ms->below_4g_mem_size, NULL);
>      object_property_set_int(phb, PCI_HOST_ABOVE_4G_MEM_SIZE,
>                              x86ms->above_4g_mem_size, NULL);
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(phb), &error_fatal);
>  
>      /* pci */
> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(phb), &error_fatal);
>      host_bus = PCI_BUS(qdev_get_child_bus(DEVICE(phb), "pcie.0"));
> +    pcms->bus = host_bus;
> +
>      /* create ISA bus */
>      lpc = pci_new_multifunction(PCI_DEVFN(ICH9_LPC_DEV, ICH9_LPC_FUNC), true,
>                                  TYPE_ICH9_LPC_DEVICE);
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 859c197f25..23b689dba3 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -66,7 +66,6 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
>                                  s->mch.pci_address_space,
>                                  s->mch.address_space_io,
>                                  0, TYPE_PCIE_BUS);
> -    PC_MACHINE(qdev_get_machine())->bus = pci->bus;
>      pci->bypass_iommu =
>          PC_MACHINE(qdev_get_machine())->default_bus_bypass_iommu;
>      qdev_realize(DEVICE(&s->mch), BUS(pci->bus), &error_fatal);

[PATCH 04/15] hw/pci/pci_host: Introduce PCI_HOST_BYPASS_IOMMU macro

[Bernhard Beschow]

Introduce a macro to avoid copy and pasting strings which can easily
cause typos.

Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/pci/pci_host.h | 2 ++
 hw/pci/pci_host.c         | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
index c6f4eb4585..e52d8ec2cd 100644
--- a/include/hw/pci/pci_host.h
+++ b/include/hw/pci/pci_host.h
@@ -31,6 +31,8 @@
 #include "hw/sysbus.h"
 #include "qom/object.h"
 
+#define PCI_HOST_BYPASS_IOMMU "bypass-iommu"
+
 #define TYPE_PCI_HOST_BRIDGE "pci-host-bridge"
 OBJECT_DECLARE_TYPE(PCIHostState, PCIHostBridgeClass, PCI_HOST_BRIDGE)
 
diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index dfd185bbb4..7af8afdcbe 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -232,7 +232,7 @@ const VMStateDescription vmstate_pcihost = {
 static Property pci_host_properties_common[] = {
     DEFINE_PROP_BOOL("x-config-reg-migration-enabled", PCIHostState,
                      mig_enabled, true),
-    DEFINE_PROP_BOOL("bypass-iommu", PCIHostState, bypass_iommu, false),
+    DEFINE_PROP_BOOL(PCI_HOST_BYPASS_IOMMU, PCIHostState, bypass_iommu, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.41.0

Re: [PATCH 04/15] hw/pci/pci_host: Introduce PCI_HOST_BYPASS_IOMMU macro

[Igor Mammedov]

On Sun, 11 Jun 2023 12:34:01 +0200
Bernhard Beschow <shentey@gmail.com> wrote:

> Introduce a macro to avoid copy and pasting strings which can easily
> cause typos.
> 
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  include/hw/pci/pci_host.h | 2 ++
>  hw/pci/pci_host.c         | 2 +-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
> index c6f4eb4585..e52d8ec2cd 100644
> --- a/include/hw/pci/pci_host.h
> +++ b/include/hw/pci/pci_host.h
> @@ -31,6 +31,8 @@
>  #include "hw/sysbus.h"
>  #include "qom/object.h"
>  
> +#define PCI_HOST_BYPASS_IOMMU "bypass-iommu"
> +
>  #define TYPE_PCI_HOST_BRIDGE "pci-host-bridge"
>  OBJECT_DECLARE_TYPE(PCIHostState, PCIHostBridgeClass, PCI_HOST_BRIDGE)
>  
> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> index dfd185bbb4..7af8afdcbe 100644
> --- a/hw/pci/pci_host.c
> +++ b/hw/pci/pci_host.c
> @@ -232,7 +232,7 @@ const VMStateDescription vmstate_pcihost = {
>  static Property pci_host_properties_common[] = {
>      DEFINE_PROP_BOOL("x-config-reg-migration-enabled", PCIHostState,
>                       mig_enabled, true),
> -    DEFINE_PROP_BOOL("bypass-iommu", PCIHostState, bypass_iommu, false),
> +    DEFINE_PROP_BOOL(PCI_HOST_BYPASS_IOMMU, PCIHostState, bypass_iommu, false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  

[PATCH 05/15] hw/pci-host/q35: Initialize PCI_HOST_BYPASS_IOMMU property from board code

[Bernhard Beschow]

The Q35 PCI host already has a PCI_HOST_BYPASS_IOMMU property. However, the
host initializes this property itself by accessing global machine state,
thereby assuming it to be a PC machine. Avoid this by having board code
set this property.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/pc_q35.c  | 2 ++
 hw/pci-host/q35.c | 3 +--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 29b46d3e1c..5220b535b2 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -230,6 +230,8 @@ static void pc_q35_init(MachineState *machine)
                             x86ms->below_4g_mem_size, NULL);
     object_property_set_int(phb, PCI_HOST_ABOVE_4G_MEM_SIZE,
                             x86ms->above_4g_mem_size, NULL);
+    object_property_set_bool(phb, PCI_HOST_BYPASS_IOMMU,
+                             pcms->default_bus_bypass_iommu, NULL);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(phb), &error_fatal);
 
     /* pci */
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 23b689dba3..7980ddde69 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -66,8 +66,7 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
                                 s->mch.pci_address_space,
                                 s->mch.address_space_io,
                                 0, TYPE_PCIE_BUS);
-    pci->bypass_iommu =
-        PC_MACHINE(qdev_get_machine())->default_bus_bypass_iommu;
+
     qdev_realize(DEVICE(&s->mch), BUS(pci->bus), &error_fatal);
 }
 
-- 
2.41.0

Re: [PATCH 05/15] hw/pci-host/q35: Initialize PCI_HOST_BYPASS_IOMMU property from board code

[Philippe Mathieu-Daudé]

On 11/6/23 12:34, Bernhard Beschow wrote:
> The Q35 PCI host already has a PCI_HOST_BYPASS_IOMMU property. However, the
> host initializes this property itself by accessing global machine state,
> thereby assuming it to be a PC machine. Avoid this by having board code
> set this property.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/i386/pc_q35.c  | 2 ++
>   hw/pci-host/q35.c | 3 +--
>   2 files changed, 3 insertions(+), 2 deletions(-)

:)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 05/15] hw/pci-host/q35: Initialize PCI_HOST_BYPASS_IOMMU property from board code

[Igor Mammedov]

On Sun, 11 Jun 2023 12:34:02 +0200
Bernhard Beschow <shentey@gmail.com> wrote:

> The Q35 PCI host already has a PCI_HOST_BYPASS_IOMMU property. However, the
> host initializes this property itself by accessing global machine state,
> thereby assuming it to be a PC machine. Avoid this by having board code
> set this property.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/i386/pc_q35.c  | 2 ++
>  hw/pci-host/q35.c | 3 +--
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 29b46d3e1c..5220b535b2 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -230,6 +230,8 @@ static void pc_q35_init(MachineState *machine)
>                              x86ms->below_4g_mem_size, NULL);
>      object_property_set_int(phb, PCI_HOST_ABOVE_4G_MEM_SIZE,
>                              x86ms->above_4g_mem_size, NULL);
> +    object_property_set_bool(phb, PCI_HOST_BYPASS_IOMMU,
> +                             pcms->default_bus_bypass_iommu, NULL);
>      sysbus_realize_and_unref(SYS_BUS_DEVICE(phb), &error_fatal);
>  
>      /* pci */
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 23b689dba3..7980ddde69 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -66,8 +66,7 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
>                                  s->mch.pci_address_space,
>                                  s->mch.address_space_io,
>                                  0, TYPE_PCIE_BUS);
> -    pci->bypass_iommu =
> -        PC_MACHINE(qdev_get_machine())->default_bus_bypass_iommu;
> +
>      qdev_realize(DEVICE(&s->mch), BUS(pci->bus), &error_fatal);
>  }
>  

[PATCH 06/15] hw/pci-host/q35: Make some property name macros reusable by i440fx

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/i386/pc.h      | 4 ++++
 include/hw/pci-host/q35.h | 5 -----
 hw/i386/pc_q35.c          | 8 ++++----
 hw/pci-host/q35.c         | 8 ++++----
 4 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index c661e9cc80..812613cc07 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -145,6 +145,10 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
 
 void pc_guest_info_init(PCMachineState *pcms);
 
+#define PCI_HOST_PROP_RAM_MEM          "ram-mem"
+#define PCI_HOST_PROP_PCI_MEM          "pci-mem"
+#define PCI_HOST_PROP_SYSTEM_MEM       "system-mem"
+#define PCI_HOST_PROP_IO_MEM           "io-mem"
 #define PCI_HOST_PROP_PCI_HOLE_START   "pci-hole-start"
 #define PCI_HOST_PROP_PCI_HOLE_END     "pci-hole-end"
 #define PCI_HOST_PROP_PCI_HOLE64_START "pci-hole64-start"
diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index e89329c51e..1d98bbfe0d 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -74,11 +74,6 @@ struct Q35PCIHost {
  * gmch part
  */
 
-#define MCH_HOST_PROP_RAM_MEM "ram-mem"
-#define MCH_HOST_PROP_PCI_MEM "pci-mem"
-#define MCH_HOST_PROP_SYSTEM_MEM "system-mem"
-#define MCH_HOST_PROP_IO_MEM "io-mem"
-
 /* PCI configuration */
 #define MCH_HOST_BRIDGE                        "MCH"
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 5220b535b2..8bfe388c9e 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -218,13 +218,13 @@ static void pc_q35_init(MachineState *machine)
     pc_memory_init(pcms, system_memory, rom_memory, pci_hole64_size);
 
     object_property_add_child(OBJECT(machine), "q35", phb);
-    object_property_set_link(phb, MCH_HOST_PROP_RAM_MEM,
+    object_property_set_link(phb, PCI_HOST_PROP_RAM_MEM,
                              OBJECT(machine->ram), NULL);
-    object_property_set_link(phb, MCH_HOST_PROP_PCI_MEM,
+    object_property_set_link(phb, PCI_HOST_PROP_PCI_MEM,
                              OBJECT(pci_memory), NULL);
-    object_property_set_link(phb, MCH_HOST_PROP_SYSTEM_MEM,
+    object_property_set_link(phb, PCI_HOST_PROP_SYSTEM_MEM,
                              OBJECT(system_memory), NULL);
-    object_property_set_link(phb, MCH_HOST_PROP_IO_MEM,
+    object_property_set_link(phb, PCI_HOST_PROP_IO_MEM,
                              OBJECT(system_io), NULL);
     object_property_set_int(phb, PCI_HOST_BELOW_4G_MEM_SIZE,
                             x86ms->below_4g_mem_size, NULL);
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 7980ddde69..ded3f6e4f4 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -240,19 +240,19 @@ static void q35_host_initfn(Object *obj)
     object_property_add_uint64_ptr(obj, PCIE_HOST_MCFG_SIZE,
                                    &pehb->size, OBJ_PROP_FLAG_READ);
 
-    object_property_add_link(obj, MCH_HOST_PROP_RAM_MEM, TYPE_MEMORY_REGION,
+    object_property_add_link(obj, PCI_HOST_PROP_RAM_MEM, TYPE_MEMORY_REGION,
                              (Object **) &s->mch.ram_memory,
                              qdev_prop_allow_set_link_before_realize, 0);
 
-    object_property_add_link(obj, MCH_HOST_PROP_PCI_MEM, TYPE_MEMORY_REGION,
+    object_property_add_link(obj, PCI_HOST_PROP_PCI_MEM, TYPE_MEMORY_REGION,
                              (Object **) &s->mch.pci_address_space,
                              qdev_prop_allow_set_link_before_realize, 0);
 
-    object_property_add_link(obj, MCH_HOST_PROP_SYSTEM_MEM, TYPE_MEMORY_REGION,
+    object_property_add_link(obj, PCI_HOST_PROP_SYSTEM_MEM, TYPE_MEMORY_REGION,
                              (Object **) &s->mch.system_memory,
                              qdev_prop_allow_set_link_before_realize, 0);
 
-    object_property_add_link(obj, MCH_HOST_PROP_IO_MEM, TYPE_MEMORY_REGION,
+    object_property_add_link(obj, PCI_HOST_PROP_IO_MEM, TYPE_MEMORY_REGION,
                              (Object **) &s->mch.address_space_io,
                              qdev_prop_allow_set_link_before_realize, 0);
 }
-- 
2.41.0

[PATCH 07/15] hw/pci-host/i440fx: Replace magic values by existing constants

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/pci-host/i440fx.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
index 61e7b97ff4..daa4d11104 100644
--- a/hw/pci-host/i440fx.c
+++ b/hw/pci-host/i440fx.c
@@ -277,8 +277,8 @@ PCIBus *i440fx_init(const char *pci_type,
 
     /* if *disabled* show SMRAM to all CPUs */
     memory_region_init_alias(&f->smram_region, OBJECT(d), "smram-region",
-                             f->pci_address_space, 0xa0000, 0x20000);
-    memory_region_add_subregion_overlap(f->system_memory, 0xa0000,
+                             f->pci_address_space, SMRAM_C_BASE, SMRAM_C_SIZE);
+    memory_region_add_subregion_overlap(f->system_memory, SMRAM_C_BASE,
                                         &f->smram_region, 1);
     memory_region_set_enabled(&f->smram_region, true);
 
@@ -286,9 +286,9 @@ PCIBus *i440fx_init(const char *pci_type,
     memory_region_init(&f->smram, OBJECT(d), "smram", 4 * GiB);
     memory_region_set_enabled(&f->smram, true);
     memory_region_init_alias(&f->low_smram, OBJECT(d), "smram-low",
-                             f->ram_memory, 0xa0000, 0x20000);
+                             f->ram_memory, SMRAM_C_BASE, SMRAM_C_SIZE);
     memory_region_set_enabled(&f->low_smram, true);
-    memory_region_add_subregion(&f->smram, 0xa0000, &f->low_smram);
+    memory_region_add_subregion(&f->smram, SMRAM_C_BASE, &f->low_smram);
     object_property_add_const_link(qdev_get_machine(), "smram",
                                    OBJECT(&f->smram));
 
-- 
2.41.0

Re: [PATCH 07/15] hw/pci-host/i440fx: Replace magic values by existing constants

[Philippe Mathieu-Daudé]

On 11/6/23 12:34, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/pci-host/i440fx.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)

:)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 07/15] hw/pci-host/i440fx: Replace magic values by existing constants

[Igor Mammedov]

On Sun, 11 Jun 2023 12:34:04 +0200
Bernhard Beschow <shentey@gmail.com> wrote:

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/pci-host/i440fx.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
> index 61e7b97ff4..daa4d11104 100644
> --- a/hw/pci-host/i440fx.c
> +++ b/hw/pci-host/i440fx.c
> @@ -277,8 +277,8 @@ PCIBus *i440fx_init(const char *pci_type,
>  
>      /* if *disabled* show SMRAM to all CPUs */
>      memory_region_init_alias(&f->smram_region, OBJECT(d), "smram-region",
> -                             f->pci_address_space, 0xa0000, 0x20000);
> -    memory_region_add_subregion_overlap(f->system_memory, 0xa0000,
> +                             f->pci_address_space, SMRAM_C_BASE, SMRAM_C_SIZE);
> +    memory_region_add_subregion_overlap(f->system_memory, SMRAM_C_BASE,
>                                          &f->smram_region, 1);
>      memory_region_set_enabled(&f->smram_region, true);
>  
> @@ -286,9 +286,9 @@ PCIBus *i440fx_init(const char *pci_type,
>      memory_region_init(&f->smram, OBJECT(d), "smram", 4 * GiB);
>      memory_region_set_enabled(&f->smram, true);
>      memory_region_init_alias(&f->low_smram, OBJECT(d), "smram-low",
> -                             f->ram_memory, 0xa0000, 0x20000);
> +                             f->ram_memory, SMRAM_C_BASE, SMRAM_C_SIZE);
>      memory_region_set_enabled(&f->low_smram, true);
> -    memory_region_add_subregion(&f->smram, 0xa0000, &f->low_smram);
> +    memory_region_add_subregion(&f->smram, SMRAM_C_BASE, &f->low_smram);
>      object_property_add_const_link(qdev_get_machine(), "smram",
>                                     OBJECT(&f->smram));
>  

[PATCH 08/15] hw/pci-host/i440fx: Have common names for some local variables

[Bernhard Beschow]

`PCIHostState` is often referred to as `phb`, own device state usually as `s`.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/pci-host/i440fx.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
index daa4d11104..88beaf99c4 100644
--- a/hw/pci-host/i440fx.c
+++ b/hw/pci-host/i440fx.c
@@ -205,28 +205,28 @@ static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v,
 
 static void i440fx_pcihost_initfn(Object *obj)
 {
-    PCIHostState *s = PCI_HOST_BRIDGE(obj);
+    PCIHostState *phb = PCI_HOST_BRIDGE(obj);
 
-    memory_region_init_io(&s->conf_mem, obj, &pci_host_conf_le_ops, s,
+    memory_region_init_io(&phb->conf_mem, obj, &pci_host_conf_le_ops, phb,
                           "pci-conf-idx", 4);
-    memory_region_init_io(&s->data_mem, obj, &pci_host_data_le_ops, s,
+    memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops, phb,
                           "pci-conf-data", 4);
 }
 
 static void i440fx_pcihost_realize(DeviceState *dev, Error **errp)
 {
-    PCIHostState *s = PCI_HOST_BRIDGE(dev);
+    PCIHostState *phb = PCI_HOST_BRIDGE(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
-    memory_region_add_subregion(s->bus->address_space_io, 0xcf8, &s->conf_mem);
+    memory_region_add_subregion(phb->bus->address_space_io, 0xcf8, &phb->conf_mem);
     sysbus_init_ioports(sbd, 0xcf8, 4);
 
-    memory_region_add_subregion(s->bus->address_space_io, 0xcfc, &s->data_mem);
+    memory_region_add_subregion(phb->bus->address_space_io, 0xcfc, &phb->data_mem);
     sysbus_init_ioports(sbd, 0xcfc, 4);
 
     /* register i440fx 0xcf8 port as coalesced pio */
-    memory_region_set_flush_coalesced(&s->data_mem);
-    memory_region_add_coalescing(&s->conf_mem, 0, 4);
+    memory_region_set_flush_coalesced(&phb->data_mem);
+    memory_region_add_coalescing(&phb->conf_mem, 0, 4);
 }
 
 static void i440fx_realize(PCIDevice *dev, Error **errp)
@@ -248,17 +248,16 @@ PCIBus *i440fx_init(const char *pci_type,
                     MemoryRegion *pci_address_space,
                     MemoryRegion *ram_memory)
 {
+    I440FXState *s = I440FX_PCI_HOST_BRIDGE(dev);
+    PCIHostState *phb = PCI_HOST_BRIDGE(dev);
     PCIBus *b;
     PCIDevice *d;
-    PCIHostState *s;
     PCII440FXState *f;
     unsigned i;
-    I440FXState *i440fx;
 
-    s = PCI_HOST_BRIDGE(dev);
     b = pci_root_bus_new(dev, NULL, pci_address_space,
                          address_space_io, 0, TYPE_PCI_BUS);
-    s->bus = b;
+    phb->bus = b;
     object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev));
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
 
@@ -268,8 +267,7 @@ PCIBus *i440fx_init(const char *pci_type,
     f->pci_address_space = pci_address_space;
     f->ram_memory = ram_memory;
 
-    i440fx = I440FX_PCI_HOST_BRIDGE(dev);
-    range_set_bounds(&i440fx->pci_hole, below_4g_mem_size,
+    range_set_bounds(&s->pci_hole, below_4g_mem_size,
                      IO_APIC_DEFAULT_ADDRESS - 1);
 
     /* setup pci memory mapping */
-- 
2.41.0

Re: [PATCH 08/15] hw/pci-host/i440fx: Have common names for some local variables

[Philippe Mathieu-Daudé]

On 11/6/23 12:34, Bernhard Beschow wrote:
> `PCIHostState` is often referred to as `phb`, own device state usually as `s`.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/pci-host/i440fx.c | 26 ++++++++++++--------------
>   1 file changed, 12 insertions(+), 14 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 09/15] hw/pci-host/i440fx: Move i440fx_realize() into PCII440FXState section

[Bernhard Beschow]

i440fx_realize() realizes the PCI device inside the host bridge
(PCII440FXState), but is implemented between i440fx_pcihost_realize() and
i440fx_init() which deal with the host bridge itself (I440FXState). Since we
want to append i440fx_init() to i440fx_pcihost_realize() later let's move
i440fx_realize() out of the way.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/pci-host/i440fx.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
index 88beaf99c4..9df4688b2e 100644
--- a/hw/pci-host/i440fx.c
+++ b/hw/pci-host/i440fx.c
@@ -65,6 +65,15 @@ struct I440FXState {
  */
 #define I440FX_COREBOOT_RAM_SIZE 0x57
 
+static void i440fx_realize(PCIDevice *dev, Error **errp)
+{
+    dev->config[I440FX_SMRAM] = 0x02;
+
+    if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) {
+        warn_report("i440fx doesn't support emulated iommu");
+    }
+}
+
 static void i440fx_update_memory_mappings(PCII440FXState *d)
 {
     int i;
@@ -229,15 +238,6 @@ static void i440fx_pcihost_realize(DeviceState *dev, Error **errp)
     memory_region_add_coalescing(&phb->conf_mem, 0, 4);
 }
 
-static void i440fx_realize(PCIDevice *dev, Error **errp)
-{
-    dev->config[I440FX_SMRAM] = 0x02;
-
-    if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) {
-        warn_report("i440fx doesn't support emulated iommu");
-    }
-}
-
 PCIBus *i440fx_init(const char *pci_type,
                     DeviceState *dev,
                     MemoryRegion *address_space_mem,
-- 
2.41.0

Re: [PATCH 09/15] hw/pci-host/i440fx: Move i440fx_realize() into PCII440FXState section

[Philippe Mathieu-Daudé]

On 11/6/23 12:34, Bernhard Beschow wrote:
> i440fx_realize() realizes the PCI device inside the host bridge
> (PCII440FXState), but is implemented between i440fx_pcihost_realize() and
> i440fx_init() which deal with the host bridge itself (I440FXState). Since we
> want to append i440fx_init() to i440fx_pcihost_realize() later let's move
> i440fx_realize() out of the way.

Yes, i440fx_foo is is PCI PF#0 (physical function) of the
i440fx_pcihost_foo (host bridge) device. We could rename
i440fx_foo -> i440fx_pf0_foo / i440fx_pcifunc0_foo / i440fx_func0_foo
for clarity.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/pci-host/i440fx.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)

[PATCH 10/15] hw/pci-host/i440fx: Make MemoryRegion pointers accessible as properties

[Bernhard Beschow]

The goal is to eliminate i440fx_init() which is a legacy init function. This
neccessitates the memory regions to be properties, like in Q35, which will be
assigned in board code.

Since i440fx needs different PCI devices in Xen mode, and since i440fx shall
be self-contained, the PCI device will be created during realization of the
host. Thus the pointers need to be moved to the host structure to be usable as
properties.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/pci-host/i440fx.h |  3 ---
 hw/pci-host/i440fx.c         | 44 ++++++++++++++++++++++++++----------
 2 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/include/hw/pci-host/i440fx.h b/include/hw/pci-host/i440fx.h
index bf57216c78..e3a550021e 100644
--- a/include/hw/pci-host/i440fx.h
+++ b/include/hw/pci-host/i440fx.h
@@ -25,9 +25,6 @@ struct PCII440FXState {
     PCIDevice parent_obj;
     /*< public >*/
 
-    MemoryRegion *system_memory;
-    MemoryRegion *pci_address_space;
-    MemoryRegion *ram_memory;
     PAMMemoryRegion pam_regions[PAM_REGIONS_COUNT];
     MemoryRegion smram_region;
     MemoryRegion smram, low_smram;
diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
index 9df4688b2e..050200cc46 100644
--- a/hw/pci-host/i440fx.c
+++ b/hw/pci-host/i440fx.c
@@ -46,7 +46,13 @@
 OBJECT_DECLARE_SIMPLE_TYPE(I440FXState, I440FX_PCI_HOST_BRIDGE)
 
 struct I440FXState {
+    /*< private >*/
     PCIHostState parent_obj;
+    /*< public >*/
+
+    MemoryRegion *system_memory;
+    MemoryRegion *pci_address_space;
+    MemoryRegion *ram_memory;
     Range pci_hole;
     uint64_t pci_hole64_size;
     bool pci_hole64_fix;
@@ -214,12 +220,25 @@ static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v,
 
 static void i440fx_pcihost_initfn(Object *obj)
 {
+    I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
     PCIHostState *phb = PCI_HOST_BRIDGE(obj);
 
     memory_region_init_io(&phb->conf_mem, obj, &pci_host_conf_le_ops, phb,
                           "pci-conf-idx", 4);
     memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops, phb,
                           "pci-conf-data", 4);
+
+    object_property_add_link(obj, PCI_HOST_PROP_RAM_MEM, TYPE_MEMORY_REGION,
+                             (Object **) &s->ram_memory,
+                             qdev_prop_allow_set_link_before_realize, 0);
+
+    object_property_add_link(obj, PCI_HOST_PROP_PCI_MEM, TYPE_MEMORY_REGION,
+                             (Object **) &s->pci_address_space,
+                             qdev_prop_allow_set_link_before_realize, 0);
+
+    object_property_add_link(obj, PCI_HOST_PROP_SYSTEM_MEM, TYPE_MEMORY_REGION,
+                             (Object **) &s->system_memory,
+                             qdev_prop_allow_set_link_before_realize, 0);
 }
 
 static void i440fx_pcihost_realize(DeviceState *dev, Error **errp)
@@ -255,7 +274,11 @@ PCIBus *i440fx_init(const char *pci_type,
     PCII440FXState *f;
     unsigned i;
 
-    b = pci_root_bus_new(dev, NULL, pci_address_space,
+    s->system_memory = address_space_mem;
+    s->pci_address_space = pci_address_space;
+    s->ram_memory = ram_memory;
+
+    b = pci_root_bus_new(dev, NULL, s->pci_address_space,
                          address_space_io, 0, TYPE_PCI_BUS);
     phb->bus = b;
     object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev));
@@ -263,20 +286,17 @@ PCIBus *i440fx_init(const char *pci_type,
 
     d = pci_create_simple(b, 0, pci_type);
     f = I440FX_PCI_DEVICE(d);
-    f->system_memory = address_space_mem;
-    f->pci_address_space = pci_address_space;
-    f->ram_memory = ram_memory;
 
     range_set_bounds(&s->pci_hole, below_4g_mem_size,
                      IO_APIC_DEFAULT_ADDRESS - 1);
 
     /* setup pci memory mapping */
-    pc_pci_as_mapping_init(f->system_memory, f->pci_address_space);
+    pc_pci_as_mapping_init(s->system_memory, s->pci_address_space);
 
     /* if *disabled* show SMRAM to all CPUs */
     memory_region_init_alias(&f->smram_region, OBJECT(d), "smram-region",
-                             f->pci_address_space, SMRAM_C_BASE, SMRAM_C_SIZE);
-    memory_region_add_subregion_overlap(f->system_memory, SMRAM_C_BASE,
+                             s->pci_address_space, SMRAM_C_BASE, SMRAM_C_SIZE);
+    memory_region_add_subregion_overlap(s->system_memory, SMRAM_C_BASE,
                                         &f->smram_region, 1);
     memory_region_set_enabled(&f->smram_region, true);
 
@@ -284,17 +304,17 @@ PCIBus *i440fx_init(const char *pci_type,
     memory_region_init(&f->smram, OBJECT(d), "smram", 4 * GiB);
     memory_region_set_enabled(&f->smram, true);
     memory_region_init_alias(&f->low_smram, OBJECT(d), "smram-low",
-                             f->ram_memory, SMRAM_C_BASE, SMRAM_C_SIZE);
+                             s->ram_memory, SMRAM_C_BASE, SMRAM_C_SIZE);
     memory_region_set_enabled(&f->low_smram, true);
     memory_region_add_subregion(&f->smram, SMRAM_C_BASE, &f->low_smram);
     object_property_add_const_link(qdev_get_machine(), "smram",
                                    OBJECT(&f->smram));
 
-    init_pam(&f->pam_regions[0], OBJECT(d), f->ram_memory, f->system_memory,
-             f->pci_address_space, PAM_BIOS_BASE, PAM_BIOS_SIZE);
+    init_pam(&f->pam_regions[0], OBJECT(d), s->ram_memory, s->system_memory,
+             s->pci_address_space, PAM_BIOS_BASE, PAM_BIOS_SIZE);
     for (i = 0; i < ARRAY_SIZE(f->pam_regions) - 1; ++i) {
-        init_pam(&f->pam_regions[i + 1], OBJECT(d), f->ram_memory,
-                 f->system_memory, f->pci_address_space,
+        init_pam(&f->pam_regions[i + 1], OBJECT(d), s->ram_memory,
+                 s->system_memory, s->pci_address_space,
                  PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
     }
 
-- 
2.41.0

Re: [PATCH 10/15] hw/pci-host/i440fx: Make MemoryRegion pointers accessible as properties

[Philippe Mathieu-Daudé]

On 11/6/23 12:34, Bernhard Beschow wrote:
> The goal is to eliminate i440fx_init() which is a legacy init function. This
> neccessitates the memory regions to be properties, like in Q35, which will be
> assigned in board code.
> 
> Since i440fx needs different PCI devices in Xen mode, and since i440fx shall
> be self-contained, the PCI device will be created during realization of the
> host. Thus the pointers need to be moved to the host structure to be usable as
> properties.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   include/hw/pci-host/i440fx.h |  3 ---
>   hw/pci-host/i440fx.c         | 44 ++++++++++++++++++++++++++----------
>   2 files changed, 32 insertions(+), 15 deletions(-)


> @@ -214,12 +220,25 @@ static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v,
>   
>   static void i440fx_pcihost_initfn(Object *obj)
>   {
> +    I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
>       PCIHostState *phb = PCI_HOST_BRIDGE(obj);
>   
>       memory_region_init_io(&phb->conf_mem, obj, &pci_host_conf_le_ops, phb,
>                             "pci-conf-idx", 4);
>       memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops, phb,
>                             "pci-conf-data", 4);
> +
> +    object_property_add_link(obj, PCI_HOST_PROP_RAM_MEM, TYPE_MEMORY_REGION,
> +                             (Object **) &s->ram_memory,
> +                             qdev_prop_allow_set_link_before_realize, 0);
> +
> +    object_property_add_link(obj, PCI_HOST_PROP_PCI_MEM, TYPE_MEMORY_REGION,
> +                             (Object **) &s->pci_address_space,
> +                             qdev_prop_allow_set_link_before_realize, 0);
> +
> +    object_property_add_link(obj, PCI_HOST_PROP_SYSTEM_MEM, TYPE_MEMORY_REGION,
> +                             (Object **) &s->system_memory,
> +                             qdev_prop_allow_set_link_before_realize, 0);

I wonder why we can't simply use device_class_set_props() in 
i440fx_pcihost_class_init().

Anyhow,
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 11/15] hw/pci-host/i440fx: Add PCI_HOST_PROP_IO_MEM property

[Bernhard Beschow]

Introduce the property in anticipation of QOM'ification; Q35 has the same
property.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/pci-host/i440fx.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
index 050200cc46..67eeffb421 100644
--- a/hw/pci-host/i440fx.c
+++ b/hw/pci-host/i440fx.c
@@ -27,7 +27,6 @@
 #include "qemu/range.h"
 #include "hw/i386/pc.h"
 #include "hw/pci/pci.h"
-#include "hw/pci/pci_bus.h"
 #include "hw/pci/pci_host.h"
 #include "hw/pci-host/i440fx.h"
 #include "hw/qdev-properties.h"
@@ -51,6 +50,7 @@ struct I440FXState {
     /*< public >*/
 
     MemoryRegion *system_memory;
+    MemoryRegion *address_space_io;
     MemoryRegion *pci_address_space;
     MemoryRegion *ram_memory;
     Range pci_hole;
@@ -239,17 +239,22 @@ static void i440fx_pcihost_initfn(Object *obj)
     object_property_add_link(obj, PCI_HOST_PROP_SYSTEM_MEM, TYPE_MEMORY_REGION,
                              (Object **) &s->system_memory,
                              qdev_prop_allow_set_link_before_realize, 0);
+
+    object_property_add_link(obj, PCI_HOST_PROP_IO_MEM, TYPE_MEMORY_REGION,
+                             (Object **) &s->address_space_io,
+                             qdev_prop_allow_set_link_before_realize, 0);
 }
 
 static void i440fx_pcihost_realize(DeviceState *dev, Error **errp)
 {
+    I440FXState *s = I440FX_PCI_HOST_BRIDGE(dev);
     PCIHostState *phb = PCI_HOST_BRIDGE(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
-    memory_region_add_subregion(phb->bus->address_space_io, 0xcf8, &phb->conf_mem);
+    memory_region_add_subregion(s->address_space_io, 0xcf8, &phb->conf_mem);
     sysbus_init_ioports(sbd, 0xcf8, 4);
 
-    memory_region_add_subregion(phb->bus->address_space_io, 0xcfc, &phb->data_mem);
+    memory_region_add_subregion(s->address_space_io, 0xcfc, &phb->data_mem);
     sysbus_init_ioports(sbd, 0xcfc, 4);
 
     /* register i440fx 0xcf8 port as coalesced pio */
@@ -275,11 +280,12 @@ PCIBus *i440fx_init(const char *pci_type,
     unsigned i;
 
     s->system_memory = address_space_mem;
+    s->address_space_io = address_space_io;
     s->pci_address_space = pci_address_space;
     s->ram_memory = ram_memory;
 
     b = pci_root_bus_new(dev, NULL, s->pci_address_space,
-                         address_space_io, 0, TYPE_PCI_BUS);
+                         s->address_space_io, 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);
-- 
2.41.0

Re: [PATCH 11/15] hw/pci-host/i440fx: Add PCI_HOST_PROP_IO_MEM property

[Philippe Mathieu-Daudé]

On 11/6/23 12:34, Bernhard Beschow wrote:
> Introduce the property in anticipation of QOM'ification; Q35 has the same
> property.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/pci-host/i440fx.c | 14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)


> @@ -51,6 +50,7 @@ struct I440FXState {
>       /*< public >*/
>   
>       MemoryRegion *system_memory;
> +    MemoryRegion *address_space_io;

"io_mem" like the property? (this is not an AddressSpace)

>       MemoryRegion *pci_address_space;

(pre-existing misleading name)

>       MemoryRegion *ram_memory;
>       Range pci_hole;

Re: [PATCH 11/15] hw/pci-host/i440fx: Add PCI_HOST_PROP_IO_MEM property

[Bernhard Beschow]

Am 12. Juni 2023 10:31:48 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 11/6/23 12:34, Bernhard Beschow wrote:
>> Introduce the property in anticipation of QOM'ification; Q35 has the same
>> property.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   hw/pci-host/i440fx.c | 14 ++++++++++----
>>   1 file changed, 10 insertions(+), 4 deletions(-)
>
>
>> @@ -51,6 +50,7 @@ struct I440FXState {
>>       /*< public >*/
>>         MemoryRegion *system_memory;
>> +    MemoryRegion *address_space_io;
>
>"io_mem" like the property? (this is not an AddressSpace)

Agreed. I'd name it "io_memory" to be consistent with above attribute.

Best regards,
Bernhard

>
>>       MemoryRegion *pci_address_space;
>
>(pre-existing misleading name)
>
>>       MemoryRegion *ram_memory;
>>       Range pci_hole;
>

[PATCH 12/15] hw/pci-host/i440fx: Add PCI_HOST_{ABOVE, BELOW}_4G_MEM_SIZE properties

[Bernhard Beschow]

Introduce the properties in anticipation of QOM'ification; Q35 has the same
properties.

Note that we want to avoid a "ram size" property in the QOM interface since it
seems redundant to both properties introduced in this change. Thus the removal
of the ram_size parameter. We assume the invariant of both properties to sum up
to "ram size" which is already asserted in pc_memory_init(). Under Xen the
invariant seems to hold as well, so we now also check it there.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/pci-host/i440fx.h |  1 -
 hw/i386/pc_piix.c            |  5 ++++-
 hw/pci-host/i440fx.c         | 12 ++++++++++--
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/include/hw/pci-host/i440fx.h b/include/hw/pci-host/i440fx.h
index e3a550021e..7e38456ebb 100644
--- a/include/hw/pci-host/i440fx.h
+++ b/include/hw/pci-host/i440fx.h
@@ -36,7 +36,6 @@ PCIBus *i440fx_init(const char *pci_type,
                     DeviceState *dev,
                     MemoryRegion *address_space_mem,
                     MemoryRegion *address_space_io,
-                    ram_addr_t ram_size,
                     ram_addr_t below_4g_mem_size,
                     ram_addr_t above_4g_mem_size,
                     MemoryRegion *pci_memory,
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 42af03dbb4..f9141536cf 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -227,6 +227,9 @@ static void pc_init1(MachineState *machine,
     if (!xen_enabled()) {
         pc_memory_init(pcms, system_memory, rom_memory, hole64_size);
     } else {
+        assert(machine->ram_size == x86ms->below_4g_mem_size +
+                                    x86ms->above_4g_mem_size);
+
         pc_system_flash_cleanup_unused(pcms);
         if (machine->kernel_filename != NULL) {
             /* For xen HVM direct kernel boot, load linux here */
@@ -242,7 +245,7 @@ static void pc_init1(MachineState *machine,
 
         pci_bus = i440fx_init(pci_type,
                               i440fx_host,
-                              system_memory, system_io, machine->ram_size,
+                              system_memory, system_io,
                               x86ms->below_4g_mem_size,
                               x86ms->above_4g_mem_size,
                               pci_memory, ram_memory);
diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
index 67eeffb421..3c44f24d2a 100644
--- a/hw/pci-host/i440fx.c
+++ b/hw/pci-host/i440fx.c
@@ -54,6 +54,8 @@ struct I440FXState {
     MemoryRegion *pci_address_space;
     MemoryRegion *ram_memory;
     Range pci_hole;
+    uint64_t below_4g_mem_size;
+    uint64_t above_4g_mem_size;
     uint64_t pci_hole64_size;
     bool pci_hole64_fix;
     uint32_t short_root_bus;
@@ -266,7 +268,6 @@ PCIBus *i440fx_init(const char *pci_type,
                     DeviceState *dev,
                     MemoryRegion *address_space_mem,
                     MemoryRegion *address_space_io,
-                    ram_addr_t ram_size,
                     ram_addr_t below_4g_mem_size,
                     ram_addr_t above_4g_mem_size,
                     MemoryRegion *pci_address_space,
@@ -283,6 +284,8 @@ PCIBus *i440fx_init(const char *pci_type,
     s->address_space_io = 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;
 
     b = pci_root_bus_new(dev, NULL, s->pci_address_space,
                          s->address_space_io, 0, TYPE_PCI_BUS);
@@ -293,7 +296,7 @@ PCIBus *i440fx_init(const char *pci_type,
     d = pci_create_simple(b, 0, pci_type);
     f = I440FX_PCI_DEVICE(d);
 
-    range_set_bounds(&s->pci_hole, below_4g_mem_size,
+    range_set_bounds(&s->pci_hole, s->below_4g_mem_size,
                      IO_APIC_DEFAULT_ADDRESS - 1);
 
     /* setup pci memory mapping */
@@ -324,6 +327,7 @@ PCIBus *i440fx_init(const char *pci_type,
                  PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
     }
 
+    ram_addr_t ram_size = s->below_4g_mem_size + s->above_4g_mem_size;
     ram_size = ram_size / 8 / 1024 / 1024;
     if (ram_size > 255) {
         ram_size = 255;
@@ -383,6 +387,10 @@ static Property i440fx_props[] = {
     DEFINE_PROP_SIZE(PCI_HOST_PROP_PCI_HOLE64_SIZE, I440FXState,
                      pci_hole64_size, I440FX_PCI_HOST_HOLE64_SIZE_DEFAULT),
     DEFINE_PROP_UINT32("short_root_bus", I440FXState, short_root_bus, 0),
+    DEFINE_PROP_SIZE(PCI_HOST_BELOW_4G_MEM_SIZE, I440FXState,
+                     below_4g_mem_size, 0),
+    DEFINE_PROP_SIZE(PCI_HOST_ABOVE_4G_MEM_SIZE, I440FXState,
+                     above_4g_mem_size, 0),
     DEFINE_PROP_BOOL("x-pci-hole64-fix", I440FXState, pci_hole64_fix, true),
     DEFINE_PROP_END_OF_LIST(),
 };
-- 
2.41.0

[PATCH 13/15] hw/pci-host/i440fx: Add I440FX_HOST_PROP_PCI_TYPE property

[Bernhard Beschow]

I440FX needs a different PCI device model if the "igd-passthru" property is
enabled. The type name is currently passed as a parameter to i440fx_init(). This
parameter will be replaced by a property assignment once i440fx_init() gets
resolved.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/pci-host/i440fx.h | 2 ++
 hw/pci-host/i440fx.c         | 6 +++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/hw/pci-host/i440fx.h b/include/hw/pci-host/i440fx.h
index 7e38456ebb..2d7bae5a45 100644
--- a/include/hw/pci-host/i440fx.h
+++ b/include/hw/pci-host/i440fx.h
@@ -15,6 +15,8 @@
 #include "hw/pci-host/pam.h"
 #include "qom/object.h"
 
+#define I440FX_HOST_PROP_PCI_TYPE "pci-type"
+
 #define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost"
 #define TYPE_I440FX_PCI_DEVICE "i440FX"
 
diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
index 3c44f24d2a..44ff3f7ea0 100644
--- a/hw/pci-host/i440fx.c
+++ b/hw/pci-host/i440fx.c
@@ -59,6 +59,8 @@ struct I440FXState {
     uint64_t pci_hole64_size;
     bool pci_hole64_fix;
     uint32_t short_root_bus;
+
+    char *pci_type;
 };
 
 #define I440FX_PAM      0x59
@@ -286,6 +288,7 @@ PCIBus *i440fx_init(const char *pci_type,
     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->address_space_io, 0, TYPE_PCI_BUS);
@@ -293,7 +296,7 @@ PCIBus *i440fx_init(const char *pci_type,
     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, pci_type);
+    d = pci_create_simple(b, 0, s->pci_type);
     f = I440FX_PCI_DEVICE(d);
 
     range_set_bounds(&s->pci_hole, s->below_4g_mem_size,
@@ -392,6 +395,7 @@ static Property i440fx_props[] = {
     DEFINE_PROP_SIZE(PCI_HOST_ABOVE_4G_MEM_SIZE, I440FXState,
                      above_4g_mem_size, 0),
     DEFINE_PROP_BOOL("x-pci-hole64-fix", I440FXState, pci_hole64_fix, true),
+    DEFINE_PROP_STRING(I440FX_HOST_PROP_PCI_TYPE, I440FXState, pci_type),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.41.0

[PATCH 14/15] hw/pci-host/i440fx: Resolve i440fx_init()

[Bernhard Beschow]

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 f9141536cf..22173b122b 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);
+        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 44ff3f7ea0..e1708bf2c9 100644
--- a/hw/pci-host/i440fx.c
+++ b/hw/pci-host/i440fx.c
@@ -251,9 +251,14 @@ static void i440fx_pcihost_initfn(Object *obj)
 
 static void i440fx_pcihost_realize(DeviceState *dev, Error **errp)
 {
+    ERRP_GUARD();
     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->address_space_io, 0xcf8, &phb->conf_mem);
     sysbus_init_ioports(sbd, 0xcf8, 4);
@@ -264,37 +269,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->address_space_io = 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->address_space_io, 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);
@@ -338,8 +316,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)
-- 
2.41.0

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

[Bernhard Beschow]

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);
     } else {
         pci_memory = NULL;
         rom_memory = system_memory;
-        i440fx_host = NULL;
+        pci_bus = NULL;
         hole64_size = 0;
     }
 
@@ -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);
 
-- 
2.41.0

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

[Philippe Mathieu-Daudé]

On 11/6/23 12:34, Bernhard Beschow 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(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

[Igor Mammedov]

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.

>      } 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?

>      }
>  
> @@ -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 15/15] hw/i386/pc_piix: Move i440fx' realize near its qdev_new()

[Igor Mammedov]

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. 

> 
> >      } 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?
> 
> >      }
> >  
> > @@ -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 15/15] hw/i386/pc_piix: Move i440fx' realize near its qdev_new()

[Bernhard Beschow]

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.

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

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

[Bernhard Beschow]

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