[PATCH 00/12] Q35 PCI host fixes and QOM cleanup

[PATCH 00/12] Q35 PCI host fixes and QOM cleanup

[Bernhard Beschow]

This series mostly cleans up QOM-related initialization code. It also performs
some modernization and fixing.

The first patch originates from "PC and ICH9 clanups" series [1] which has been
dropped in v3 in favor of another series [2]. Review comments in [2] suggest it
needs more work, so bring the patch back here.

Patch 2 fixes a clangd warning and patch 3 modernizes usage of the memory API.

Patches 4-9 clean up initialization code.

The last four patches also clean up initialization code with the last patch
doing the actual cleanup.

Based-on: <20230213162004.2797-1-shentey@gmail.com>
         "[PATCH v4 0/9] PC cleanups"

Testing done:
* `make check`
* `make check-avocado`
* `qemu-system-x86_64 -M q35 -m 2G -cdrom \
     manjaro-kde-21.3.2-220704-linux515.iso`

[1] https://lore.kernel.org/qemu-devel/20230131115326.12454-1-shentey@gmail.com/
[2] https://lore.kernel.org/qemu-devel/20230203180914.49112-1-philmd@linaro.org/

Bernhard Beschow (12):
  hw/i386/pc_q35: Resolve redundant q35_host variable
  hw/pci-host/q35: Fix contradicting .endianness assignment
  hw/pci-host/q35: Use memory_region_set_address() also for
    tseg_blackhole
  hw/pci-host/q35: Initialize PCMachineState::bus in board code
  hw/pci-host/q35: Initialize "bypass-iommu" property from board code
  hw/pci-host/q35: Initialize properties just once
  hw/pci-host/q35: Initialize PCI hole boundaries just once
  hw/pci-host/q35: Turn PCI hole properties into class properties
  hw/pci-host/q35: Rename local variable to more idiomatic "phb"
  hw/pci-host/q35: Propagate to errp rather than doing error_fatal
  hw/pci-host/q35: Merge mch_realize() into q35_host_realize()
  hw/pci-host/q35: Move MemoryRegion pointers to host device

 include/hw/pci-host/q35.h |  17 +-
 hw/i386/pc_q35.c          |  33 ++--
 hw/pci-host/q35.c         | 325 ++++++++++++++++++--------------------
 3 files changed, 178 insertions(+), 197 deletions(-)

-- 
2.39.1

[PATCH 01/12] 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 | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index a5ffb77ed8..1ce9b16c53 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -118,8 +118,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;
@@ -206,10 +205,10 @@ static void pc_q35_init(MachineState *machine)
     }
 
     /* create pci host bus */
-    q35_host = Q35_HOST_DEVICE(qdev_new(TYPE_Q35_HOST_DEVICE));
+    phb = 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);
     }
@@ -217,25 +216,25 @@ 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_SMRAM_MEM,
+    object_property_set_link(phb, MCH_HOST_PROP_SMRAM_MEM,
                              OBJECT(&x86ms->smram), 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_create_simple_multifunction(host_bus, PCI_DEVFN(ICH9_LPC_DEV,
                                           ICH9_LPC_FUNC), true,
-- 
2.39.1

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

[Bernhard Beschow]

Settle on little endian which is consistent with using
pci_host_conf_le_ops.

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 83f2a98c71..3124cad60f 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -289,7 +289,6 @@ static void blackhole_write(void *opaque, hwaddr addr, uint64_t val,
 static const MemoryRegionOps blackhole_ops = {
     .read = blackhole_read,
     .write = blackhole_write,
-    .endianness = DEVICE_NATIVE_ENDIAN,
     .valid.min_access_size = 1,
     .valid.max_access_size = 4,
     .impl.min_access_size = 4,
-- 
2.39.1

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

[Michael S. Tsirkin]

On Tue, Feb 14, 2023 at 02:14:31PM +0100, Bernhard Beschow wrote:
> Settle on little endian which is consistent with using
> pci_host_conf_le_ops.
> 
> Fixes: bafc90bdc594 'q35: implement TSEG'

incorrect formatting for the fixes tag


> 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 83f2a98c71..3124cad60f 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -289,7 +289,6 @@ static void blackhole_write(void *opaque, hwaddr addr, uint64_t val,
>  static const MemoryRegionOps blackhole_ops = {
>      .read = blackhole_read,
>      .write = blackhole_write,
> -    .endianness = DEVICE_NATIVE_ENDIAN,
>      .valid.min_access_size = 1,
>      .valid.max_access_size = 4,
>      .impl.min_access_size = 4,
> -- 
> 2.39.1

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

[Michael S. Tsirkin]

On Tue, Feb 14, 2023 at 02:14:31PM +0100, Bernhard Beschow wrote:
> Settle on little endian which is consistent with using
> pci_host_conf_le_ops.
> 
> Fixes: bafc90bdc594 'q35: implement TSEG'
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

I think it's native because native is a bit cheaper and
it's just 0xffffffff anyway.
Why change? A comment would be a good idea though.

> ---
>  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 83f2a98c71..3124cad60f 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -289,7 +289,6 @@ static void blackhole_write(void *opaque, hwaddr addr, uint64_t val,
>  static const MemoryRegionOps blackhole_ops = {
>      .read = blackhole_read,
>      .write = blackhole_write,
> -    .endianness = DEVICE_NATIVE_ENDIAN,
>      .valid.min_access_size = 1,
>      .valid.max_access_size = 4,
>      .impl.min_access_size = 4,
> -- 
> 2.39.1

[PATCH 03/12] hw/pci-host/q35: Use memory_region_set_address() also for tseg_blackhole

[Bernhard Beschow]

Deleting from and adding to the parent memory region seems to be the old
way of changing a memory region's address which is superseeded by
memory_region_set_address(). Moreover, memory_region_set_address() is
already used for tseg_window which is tseg_blackhole's counterpart in
SMM space.

Ammends: bafc90bdc594 'q35: implement TSEG'
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/pci-host/q35.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 3124cad60f..0384ce4350 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -404,12 +404,11 @@ static void mch_update_smram(MCHPCIState *mch)
     } else {
         tseg_size = 0;
     }
-    memory_region_del_subregion(mch->system_memory, &mch->tseg_blackhole);
+
     memory_region_set_enabled(&mch->tseg_blackhole, tseg_size);
     memory_region_set_size(&mch->tseg_blackhole, tseg_size);
-    memory_region_add_subregion_overlap(mch->system_memory,
-                                        mch->below_4g_mem_size - tseg_size,
-                                        &mch->tseg_blackhole, 1);
+    memory_region_set_address(&mch->tseg_blackhole,
+                              mch->below_4g_mem_size - tseg_size);
 
     memory_region_set_enabled(&mch->tseg_window, tseg_size);
     memory_region_set_size(&mch->tseg_window, tseg_size);
-- 
2.39.1

Re: [PATCH 03/12] hw/pci-host/q35: Use memory_region_set_address() also for tseg_blackhole

[Michael S. Tsirkin]

On Tue, Feb 14, 2023 at 02:14:32PM +0100, Bernhard Beschow wrote:
> Deleting from and adding to the parent memory region seems to be the old
> way of changing a memory region's address which is superseeded by
> memory_region_set_address(). Moreover, memory_region_set_address() is
> already used for tseg_window which is tseg_blackhole's counterpart in
> SMM space.
> 
> Ammends: bafc90bdc594 'q35: implement TSEG'

I don't really see what purpose does this tag serve but
if you want it use the standard format pls.


> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>  hw/pci-host/q35.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 3124cad60f..0384ce4350 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -404,12 +404,11 @@ static void mch_update_smram(MCHPCIState *mch)
>      } else {
>          tseg_size = 0;
>      }
> -    memory_region_del_subregion(mch->system_memory, &mch->tseg_blackhole);
> +
>      memory_region_set_enabled(&mch->tseg_blackhole, tseg_size);
>      memory_region_set_size(&mch->tseg_blackhole, tseg_size);
> -    memory_region_add_subregion_overlap(mch->system_memory,
> -                                        mch->below_4g_mem_size - tseg_size,
> -                                        &mch->tseg_blackhole, 1);
> +    memory_region_set_address(&mch->tseg_blackhole,
> +                              mch->below_4g_mem_size - tseg_size);
>  
>      memory_region_set_enabled(&mch->tseg_window, tseg_size);
>      memory_region_set_size(&mch->tseg_window, tseg_size);
> -- 
> 2.39.1

[PATCH 04/12] 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  | 6 ++++--
 hw/pci-host/q35.c | 1 -
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 1ce9b16c53..c2dc87acee 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -231,10 +231,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);
-
-    /* pci */
     sysbus_realize_and_unref(SYS_BUS_DEVICE(phb), &error_fatal);
+
+    /* pci */
     host_bus = PCI_BUS(qdev_get_child_bus(DEVICE(phb), "pcie.0"));
+    pcms->bus = host_bus;
+
     /* create ISA bus */
     lpc = pci_create_simple_multifunction(host_bus, PCI_DEVFN(ICH9_LPC_DEV,
                                           ICH9_LPC_FUNC), true,
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 0384ce4350..26e9e28e0e 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.39.1

[PATCH 05/12] hw/pci-host/q35: Initialize "bypass-iommu" property from board code

[Bernhard Beschow]

The Q35 PCI host already has a "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 c2dc87acee..b3c55012d4 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -231,6 +231,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, "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 26e9e28e0e..0e198f97a7 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.39.1

Re: [PATCH 05/12] hw/pci-host/q35: Initialize "bypass-iommu" property from board code

[Michael S. Tsirkin]

On Tue, Feb 14, 2023 at 02:14:34PM +0100, Bernhard Beschow wrote:
> The Q35 PCI host already has a "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 c2dc87acee..b3c55012d4 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -231,6 +231,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, "bypass-iommu",
> +                             pcms->default_bus_bypass_iommu, NULL);

Can we use a macro to avoid duplicating the property name?

>      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 26e9e28e0e..0e198f97a7 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.39.1

[PATCH 06/12] hw/pci-host/q35: Initialize properties just once

[Bernhard Beschow]

Although not used there, the attributes for Q35's "pci-hole64-size" and
"short_root_bus" properties currently reside in its child device. This
causes the default values to be overwritten during the child's
object_initialize() phase. Avoid this by moving both attributes into the
host device.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/pci-host/q35.h |  5 +++--
 hw/pci-host/q35.c         | 20 +++++---------------
 2 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index fcbe57b42d..93e41ffbee 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -54,8 +54,6 @@ struct MCHPCIState {
     Range pci_hole;
     uint64_t below_4g_mem_size;
     uint64_t above_4g_mem_size;
-    uint64_t pci_hole64_size;
-    uint32_t short_root_bus;
     uint16_t ext_tseg_mbytes;
 };
 
@@ -64,7 +62,10 @@ struct Q35PCIHost {
     PCIExpressHost parent_obj;
     /*< public >*/
 
+    uint64_t pci_hole64_size;
+    uint32_t short_root_bus;
     bool pci_hole64_fix;
+
     MCHPCIState mch;
 };
 
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 0e198f97a7..03aa08dae5 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -76,7 +76,7 @@ static const char *q35_host_root_bus_path(PCIHostState *host_bridge,
     Q35PCIHost *s = Q35_HOST_DEVICE(host_bridge);
 
      /* For backwards compat with old device paths */
-    if (s->mch.short_root_bus) {
+    if (s->short_root_bus) {
         return "0000";
     }
     return "0000:00";
@@ -161,27 +161,19 @@ static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v,
 
     pci_bus_get_w64_range(h->bus, &w64);
     value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
-    hole64_end = ROUND_UP(hole64_start + s->mch.pci_hole64_size, 1ULL << 30);
+    hole64_end = ROUND_UP(hole64_start + s->pci_hole64_size, 1ULL << 30);
     if (s->pci_hole64_fix && value < hole64_end) {
         value = hole64_end;
     }
     visit_type_uint64(v, name, &value, errp);
 }
 
-/*
- * NOTE: setting defaults for the mch.* fields in this table
- * doesn't work, because mch is a separate QOM object that is
- * zeroed by the object_initialize(&s->mch, ...) call inside
- * q35_host_initfn().  The default values for those
- * properties need to be initialized manually by
- * q35_host_initfn() after the object_initialize() call.
- */
 static Property q35_host_props[] = {
     DEFINE_PROP_UINT64(PCIE_HOST_MCFG_BASE, Q35PCIHost, parent_obj.base_addr,
                         MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT),
     DEFINE_PROP_SIZE(PCI_HOST_PROP_PCI_HOLE64_SIZE, Q35PCIHost,
-                     mch.pci_hole64_size, Q35_PCI_HOST_HOLE64_SIZE_DEFAULT),
-    DEFINE_PROP_UINT32("short_root_bus", Q35PCIHost, mch.short_root_bus, 0),
+                     pci_hole64_size, Q35_PCI_HOST_HOLE64_SIZE_DEFAULT),
+    DEFINE_PROP_UINT32("short_root_bus", Q35PCIHost, short_root_bus, 0),
     DEFINE_PROP_SIZE(PCI_HOST_BELOW_4G_MEM_SIZE, Q35PCIHost,
                      mch.below_4g_mem_size, 0),
     DEFINE_PROP_SIZE(PCI_HOST_ABOVE_4G_MEM_SIZE, Q35PCIHost,
@@ -218,9 +210,7 @@ static void q35_host_initfn(Object *obj)
     object_initialize_child(OBJECT(s), "mch", &s->mch, TYPE_MCH_PCI_DEVICE);
     qdev_prop_set_int32(DEVICE(&s->mch), "addr", PCI_DEVFN(0, 0));
     qdev_prop_set_bit(DEVICE(&s->mch), "multifunction", false);
-    /* mch's object_initialize resets the default value, set it again */
-    qdev_prop_set_uint64(DEVICE(s), PCI_HOST_PROP_PCI_HOLE64_SIZE,
-                         Q35_PCI_HOST_HOLE64_SIZE_DEFAULT);
+
     object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_START, "uint32",
                         q35_host_get_pci_hole_start,
                         NULL, NULL, NULL);
-- 
2.39.1

Re: [PATCH 06/12] hw/pci-host/q35: Initialize properties just once

[Michael S. Tsirkin]

On Tue, Feb 14, 2023 at 02:14:35PM +0100, Bernhard Beschow wrote:
> Although not used there, the attributes for Q35's "pci-hole64-size" and
> "short_root_bus" properties currently reside in its child device. This
> causes the default values to be overwritten during the child's
> object_initialize() phase.

pls add explanation why this is a problem.

> Avoid this by moving both attributes into the
> host device.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>  include/hw/pci-host/q35.h |  5 +++--
>  hw/pci-host/q35.c         | 20 +++++---------------
>  2 files changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
> index fcbe57b42d..93e41ffbee 100644
> --- a/include/hw/pci-host/q35.h
> +++ b/include/hw/pci-host/q35.h
> @@ -54,8 +54,6 @@ struct MCHPCIState {
>      Range pci_hole;
>      uint64_t below_4g_mem_size;
>      uint64_t above_4g_mem_size;
> -    uint64_t pci_hole64_size;
> -    uint32_t short_root_bus;
>      uint16_t ext_tseg_mbytes;
>  };
>  
> @@ -64,7 +62,10 @@ struct Q35PCIHost {
>      PCIExpressHost parent_obj;
>      /*< public >*/
>  
> +    uint64_t pci_hole64_size;
> +    uint32_t short_root_bus;
>      bool pci_hole64_fix;
> +
>      MCHPCIState mch;
>  };
>  
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 0e198f97a7..03aa08dae5 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -76,7 +76,7 @@ static const char *q35_host_root_bus_path(PCIHostState *host_bridge,
>      Q35PCIHost *s = Q35_HOST_DEVICE(host_bridge);
>  
>       /* For backwards compat with old device paths */
> -    if (s->mch.short_root_bus) {
> +    if (s->short_root_bus) {
>          return "0000";
>      }
>      return "0000:00";
> @@ -161,27 +161,19 @@ static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v,
>  
>      pci_bus_get_w64_range(h->bus, &w64);
>      value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
> -    hole64_end = ROUND_UP(hole64_start + s->mch.pci_hole64_size, 1ULL << 30);
> +    hole64_end = ROUND_UP(hole64_start + s->pci_hole64_size, 1ULL << 30);
>      if (s->pci_hole64_fix && value < hole64_end) {
>          value = hole64_end;
>      }
>      visit_type_uint64(v, name, &value, errp);
>  }
>  
> -/*
> - * NOTE: setting defaults for the mch.* fields in this table
> - * doesn't work, because mch is a separate QOM object that is
> - * zeroed by the object_initialize(&s->mch, ...) call inside
> - * q35_host_initfn().  The default values for those
> - * properties need to be initialized manually by
> - * q35_host_initfn() after the object_initialize() call.
> - */
>  static Property q35_host_props[] = {
>      DEFINE_PROP_UINT64(PCIE_HOST_MCFG_BASE, Q35PCIHost, parent_obj.base_addr,
>                          MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT),
>      DEFINE_PROP_SIZE(PCI_HOST_PROP_PCI_HOLE64_SIZE, Q35PCIHost,
> -                     mch.pci_hole64_size, Q35_PCI_HOST_HOLE64_SIZE_DEFAULT),
> -    DEFINE_PROP_UINT32("short_root_bus", Q35PCIHost, mch.short_root_bus, 0),
> +                     pci_hole64_size, Q35_PCI_HOST_HOLE64_SIZE_DEFAULT),
> +    DEFINE_PROP_UINT32("short_root_bus", Q35PCIHost, short_root_bus, 0),
>      DEFINE_PROP_SIZE(PCI_HOST_BELOW_4G_MEM_SIZE, Q35PCIHost,
>                       mch.below_4g_mem_size, 0),
>      DEFINE_PROP_SIZE(PCI_HOST_ABOVE_4G_MEM_SIZE, Q35PCIHost,
> @@ -218,9 +210,7 @@ static void q35_host_initfn(Object *obj)
>      object_initialize_child(OBJECT(s), "mch", &s->mch, TYPE_MCH_PCI_DEVICE);
>      qdev_prop_set_int32(DEVICE(&s->mch), "addr", PCI_DEVFN(0, 0));
>      qdev_prop_set_bit(DEVICE(&s->mch), "multifunction", false);
> -    /* mch's object_initialize resets the default value, set it again */
> -    qdev_prop_set_uint64(DEVICE(s), PCI_HOST_PROP_PCI_HOLE64_SIZE,
> -                         Q35_PCI_HOST_HOLE64_SIZE_DEFAULT);
> +
>      object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_START, "uint32",
>                          q35_host_get_pci_hole_start,
>                          NULL, NULL, NULL);
> -- 
> 2.39.1

[PATCH 07/12] hw/pci-host/q35: Initialize PCI hole boundaries just once

[Bernhard Beschow]

The boundaries of the PCI hole depend on a property only which doesn't
change at runtime. There is no need to reevaluate the boundaries
whenever the PCI configuration space changes.

While at it, move the pci_hole attribute into the host device since it
is only used there.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/pci-host/q35.h |  2 +-
 hw/pci-host/q35.c         | 21 +++++++++------------
 2 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index 93e41ffbee..a04d5f1a17 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -51,7 +51,6 @@ struct MCHPCIState {
     MemoryRegion tseg_blackhole, tseg_window;
     MemoryRegion smbase_blackhole, smbase_window;
     bool has_smram_at_smbase;
-    Range pci_hole;
     uint64_t below_4g_mem_size;
     uint64_t above_4g_mem_size;
     uint16_t ext_tseg_mbytes;
@@ -62,6 +61,7 @@ struct Q35PCIHost {
     PCIExpressHost parent_obj;
     /*< public >*/
 
+    Range pci_hole;
     uint64_t pci_hole64_size;
     uint32_t short_root_bus;
     bool pci_hole64_fix;
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 03aa08dae5..468bbfde51 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -62,6 +62,13 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
     memory_region_set_flush_coalesced(&pci->data_mem);
     memory_region_add_coalescing(&pci->conf_mem, 0, 4);
 
+    /*
+     * pci hole goes from end-of-low-ram to io-apic.
+     * mmconfig will be excluded by the dsdt builder.
+     */
+    range_set_bounds(&s->pci_hole, s->mch.below_4g_mem_size,
+                     IO_APIC_DEFAULT_ADDRESS - 1);
+
     pci->bus = pci_root_bus_new(DEVICE(s), "pcie.0",
                                 s->mch.pci_address_space,
                                 s->mch.address_space_io,
@@ -90,8 +97,7 @@ static void q35_host_get_pci_hole_start(Object *obj, Visitor *v,
     uint64_t val64;
     uint32_t value;
 
-    val64 = range_is_empty(&s->mch.pci_hole)
-        ? 0 : range_lob(&s->mch.pci_hole);
+    val64 = range_is_empty(&s->pci_hole) ? 0 : range_lob(&s->pci_hole);
     value = val64;
     assert(value == val64);
     visit_type_uint32(v, name, &value, errp);
@@ -105,8 +111,7 @@ static void q35_host_get_pci_hole_end(Object *obj, Visitor *v,
     uint64_t val64;
     uint32_t value;
 
-    val64 = range_is_empty(&s->mch.pci_hole)
-        ? 0 : range_upb(&s->mch.pci_hole) + 1;
+    val64 = range_is_empty(&s->pci_hole) ? 0 : range_upb(&s->pci_hole) + 1;
     value = val64;
     assert(value == val64);
     visit_type_uint32(v, name, &value, errp);
@@ -498,14 +503,6 @@ static void mch_update(MCHPCIState *mch)
     mch_update_smram(mch);
     mch_update_ext_tseg_mbytes(mch);
     mch_update_smbase_smram(mch);
-
-    /*
-     * pci hole goes from end-of-low-ram to io-apic.
-     * mmconfig will be excluded by the dsdt builder.
-     */
-    range_set_bounds(&mch->pci_hole,
-                     mch->below_4g_mem_size,
-                     IO_APIC_DEFAULT_ADDRESS - 1);
 }
 
 static int mch_post_load(void *opaque, int version_id)
-- 
2.39.1

[PATCH 08/12] hw/pci-host/q35: Turn PCI hole properties into class properties

[Bernhard Beschow]

These properties are class properties in i440fx. No need to handle them
differently in q35.

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

diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 468bbfde51..2fc047a9c6 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -199,6 +199,22 @@ static void q35_host_class_init(ObjectClass *klass, void *data)
     dc->user_creatable = false;
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
     dc->fw_name = "pci";
+
+    object_class_property_add(klass, PCI_HOST_PROP_PCI_HOLE_START, "uint32",
+                              q35_host_get_pci_hole_start,
+                              NULL, NULL, NULL);
+
+    object_class_property_add(klass, PCI_HOST_PROP_PCI_HOLE_END, "uint32",
+                              q35_host_get_pci_hole_end,
+                              NULL, NULL, NULL);
+
+    object_class_property_add(klass, PCI_HOST_PROP_PCI_HOLE64_START, "uint64",
+                              q35_host_get_pci_hole64_start,
+                              NULL, NULL, NULL);
+
+    object_class_property_add(klass, PCI_HOST_PROP_PCI_HOLE64_END, "uint64",
+                              q35_host_get_pci_hole64_end,
+                              NULL, NULL, NULL);
 }
 
 static void q35_host_initfn(Object *obj)
@@ -216,22 +232,6 @@ static void q35_host_initfn(Object *obj)
     qdev_prop_set_int32(DEVICE(&s->mch), "addr", PCI_DEVFN(0, 0));
     qdev_prop_set_bit(DEVICE(&s->mch), "multifunction", false);
 
-    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_START, "uint32",
-                        q35_host_get_pci_hole_start,
-                        NULL, NULL, NULL);
-
-    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_END, "uint32",
-                        q35_host_get_pci_hole_end,
-                        NULL, NULL, NULL);
-
-    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE64_START, "uint64",
-                        q35_host_get_pci_hole64_start,
-                        NULL, NULL, NULL);
-
-    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE64_END, "uint64",
-                        q35_host_get_pci_hole64_end,
-                        NULL, NULL, NULL);
-
     object_property_add_uint64_ptr(obj, PCIE_HOST_MCFG_SIZE,
                                    &pehb->size, OBJ_PROP_FLAG_READ);
 
-- 
2.39.1

[PATCH 09/12] hw/pci-host/q35: Rename local variable to more idiomatic "phb"

[Bernhard Beschow]

Variables of type PCIHostState* are typically named "phb" in QEMU.
Follow this convention here as well for consistency.

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

diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 2fc047a9c6..8f81debfa5 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -46,21 +46,21 @@
 
 static void q35_host_realize(DeviceState *dev, Error **errp)
 {
-    PCIHostState *pci = PCI_HOST_BRIDGE(dev);
     Q35PCIHost *s = Q35_HOST_DEVICE(dev);
+    PCIHostState *phb = PCI_HOST_BRIDGE(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
     memory_region_add_subregion(s->mch.address_space_io,
-                                MCH_HOST_BRIDGE_CONFIG_ADDR, &pci->conf_mem);
+                                MCH_HOST_BRIDGE_CONFIG_ADDR, &phb->conf_mem);
     sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_ADDR, 4);
 
     memory_region_add_subregion(s->mch.address_space_io,
-                                MCH_HOST_BRIDGE_CONFIG_DATA, &pci->data_mem);
+                                MCH_HOST_BRIDGE_CONFIG_DATA, &phb->data_mem);
     sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, 4);
 
     /* register q35 0xcf8 port as coalesced pio */
-    memory_region_set_flush_coalesced(&pci->data_mem);
-    memory_region_add_coalescing(&pci->conf_mem, 0, 4);
+    memory_region_set_flush_coalesced(&phb->data_mem);
+    memory_region_add_coalescing(&phb->conf_mem, 0, 4);
 
     /*
      * pci hole goes from end-of-low-ram to io-apic.
@@ -69,12 +69,12 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
     range_set_bounds(&s->pci_hole, s->mch.below_4g_mem_size,
                      IO_APIC_DEFAULT_ADDRESS - 1);
 
-    pci->bus = pci_root_bus_new(DEVICE(s), "pcie.0",
+    phb->bus = pci_root_bus_new(DEVICE(s), "pcie.0",
                                 s->mch.pci_address_space,
                                 s->mch.address_space_io,
                                 0, TYPE_PCIE_BUS);
 
-    qdev_realize(DEVICE(&s->mch), BUS(pci->bus), &error_fatal);
+    qdev_realize(DEVICE(&s->mch), BUS(phb->bus), &error_fatal);
 }
 
 static const char *q35_host_root_bus_path(PCIHostState *host_bridge,
-- 
2.39.1

[PATCH 10/12] hw/pci-host/q35: Propagate to errp rather than doing error_fatal

[Bernhard Beschow]

q35_host_realize() has an errp parameter. Use that to be able to
propagate the error instead of terminating abruptly.

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

diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 8f81debfa5..d517f5622b 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -46,6 +46,7 @@
 
 static void q35_host_realize(DeviceState *dev, Error **errp)
 {
+    ERRP_GUARD();
     Q35PCIHost *s = Q35_HOST_DEVICE(dev);
     PCIHostState *phb = PCI_HOST_BRIDGE(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
@@ -74,7 +75,7 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
                                 s->mch.address_space_io,
                                 0, TYPE_PCIE_BUS);
 
-    qdev_realize(DEVICE(&s->mch), BUS(phb->bus), &error_fatal);
+    qdev_realize(DEVICE(&s->mch), BUS(phb->bus), errp);
 }
 
 static const char *q35_host_root_bus_path(PCIHostState *host_bridge,
-- 
2.39.1

[PATCH 11/12] hw/pci-host/q35: Merge mch_realize() into q35_host_realize()

[Bernhard Beschow]

This patch prepares movement of the MemoryRegion pointers (which are set
through properties) into the host state. Moreover, it's usually the
parent device which maps the memory regions of its child devices into
its address space. Do the same in q35.

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

diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index d517f5622b..3a7f9185a3 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -44,12 +44,40 @@
 
 #define Q35_PCI_HOST_HOLE64_SIZE_DEFAULT (1ULL << 35)
 
+static uint64_t blackhole_read(void *ptr, hwaddr reg, unsigned size)
+{
+    return 0xffffffff;
+}
+
+static void blackhole_write(void *opaque, hwaddr addr, uint64_t val,
+                            unsigned width)
+{
+    /* nothing */
+}
+
+static const MemoryRegionOps blackhole_ops = {
+    .read = blackhole_read,
+    .write = blackhole_write,
+    .valid.min_access_size = 1,
+    .valid.max_access_size = 4,
+    .impl.min_access_size = 4,
+    .impl.max_access_size = 4,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
 static void q35_host_realize(DeviceState *dev, Error **errp)
 {
     ERRP_GUARD();
     Q35PCIHost *s = Q35_HOST_DEVICE(dev);
     PCIHostState *phb = PCI_HOST_BRIDGE(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    int i;
+
+    if (s->mch.ext_tseg_mbytes > MCH_HOST_BRIDGE_EXT_TSEG_MBYTES_MAX) {
+        error_setg(errp, "invalid extended-tseg-mbytes value: %" PRIu16,
+                   s->mch.ext_tseg_mbytes);
+        return;
+    }
 
     memory_region_add_subregion(s->mch.address_space_io,
                                 MCH_HOST_BRIDGE_CONFIG_ADDR, &phb->conf_mem);
@@ -70,6 +98,79 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
     range_set_bounds(&s->pci_hole, s->mch.below_4g_mem_size,
                      IO_APIC_DEFAULT_ADDRESS - 1);
 
+    /* setup pci memory mapping */
+    pc_pci_as_mapping_init(s->mch.system_memory, s->mch.pci_address_space);
+
+    /* if *disabled* show SMRAM to all CPUs */
+    memory_region_init_alias(&s->mch.smram_region, OBJECT(s), "smram-region",
+                             s->mch.pci_address_space, MCH_HOST_BRIDGE_SMRAM_C_BASE,
+                             MCH_HOST_BRIDGE_SMRAM_C_SIZE);
+    memory_region_add_subregion_overlap(s->mch.system_memory, MCH_HOST_BRIDGE_SMRAM_C_BASE,
+                                        &s->mch.smram_region, 1);
+    memory_region_set_enabled(&s->mch.smram_region, true);
+
+    memory_region_init_alias(&s->mch.open_high_smram, OBJECT(s), "smram-open-high",
+                             s->mch.ram_memory, MCH_HOST_BRIDGE_SMRAM_C_BASE,
+                             MCH_HOST_BRIDGE_SMRAM_C_SIZE);
+    memory_region_add_subregion_overlap(s->mch.system_memory, 0xfeda0000,
+                                        &s->mch.open_high_smram, 1);
+    memory_region_set_enabled(&s->mch.open_high_smram, false);
+
+    /* smram, as seen by SMM CPUs */
+    memory_region_init_alias(&s->mch.low_smram, OBJECT(s), "smram-low",
+                             s->mch.ram_memory, MCH_HOST_BRIDGE_SMRAM_C_BASE,
+                             MCH_HOST_BRIDGE_SMRAM_C_SIZE);
+    memory_region_set_enabled(&s->mch.low_smram, true);
+    memory_region_add_subregion(s->mch.smram, MCH_HOST_BRIDGE_SMRAM_C_BASE,
+                                &s->mch.low_smram);
+    memory_region_init_alias(&s->mch.high_smram, OBJECT(s), "smram-high",
+                             s->mch.ram_memory, MCH_HOST_BRIDGE_SMRAM_C_BASE,
+                             MCH_HOST_BRIDGE_SMRAM_C_SIZE);
+    memory_region_set_enabled(&s->mch.high_smram, true);
+    memory_region_add_subregion(s->mch.smram, 0xfeda0000, &s->mch.high_smram);
+
+    memory_region_init_io(&s->mch.tseg_blackhole, OBJECT(s),
+                          &blackhole_ops, NULL, "tseg-blackhole", 0);
+    memory_region_set_enabled(&s->mch.tseg_blackhole, false);
+    memory_region_add_subregion_overlap(s->mch.system_memory,
+                                        s->mch.below_4g_mem_size,
+                                        &s->mch.tseg_blackhole, 1);
+
+    memory_region_init_alias(&s->mch.tseg_window, OBJECT(s), "tseg-window",
+                             s->mch.ram_memory, s->mch.below_4g_mem_size, 0);
+    memory_region_set_enabled(&s->mch.tseg_window, false);
+    memory_region_add_subregion(s->mch.smram, s->mch.below_4g_mem_size,
+                                &s->mch.tseg_window);
+
+    /*
+     * This is not what hardware does, so it's QEMU specific hack.
+     * See commit message for details.
+     */
+    memory_region_init_io(&s->mch.smbase_blackhole, OBJECT(s), &blackhole_ops,
+                          NULL, "smbase-blackhole",
+                          MCH_HOST_BRIDGE_SMBASE_SIZE);
+    memory_region_set_enabled(&s->mch.smbase_blackhole, false);
+    memory_region_add_subregion_overlap(s->mch.system_memory,
+                                        MCH_HOST_BRIDGE_SMBASE_ADDR,
+                                        &s->mch.smbase_blackhole, 1);
+
+    memory_region_init_alias(&s->mch.smbase_window, OBJECT(s),
+                             "smbase-window", s->mch.ram_memory,
+                             MCH_HOST_BRIDGE_SMBASE_ADDR,
+                             MCH_HOST_BRIDGE_SMBASE_SIZE);
+    memory_region_set_enabled(&s->mch.smbase_window, false);
+    memory_region_add_subregion(s->mch.smram, MCH_HOST_BRIDGE_SMBASE_ADDR,
+                                &s->mch.smbase_window);
+
+    init_pam(&s->mch.pam_regions[0], OBJECT(s), s->mch.ram_memory,
+             s->mch.system_memory, s->mch.pci_address_space,
+             PAM_BIOS_BASE, PAM_BIOS_SIZE);
+    for (i = 0; i < ARRAY_SIZE(s->mch.pam_regions) - 1; ++i) {
+        init_pam(&s->mch.pam_regions[i + 1], OBJECT(s), s->mch.ram_memory,
+                 s->mch.system_memory, s->mch.pci_address_space,
+                 PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
+    }
+
     phb->bus = pci_root_bus_new(DEVICE(s), "pcie.0",
                                 s->mch.pci_address_space,
                                 s->mch.address_space_io,
@@ -269,27 +370,6 @@ static const TypeInfo q35_host_info = {
  * MCH D0:F0
  */
 
-static uint64_t blackhole_read(void *ptr, hwaddr reg, unsigned size)
-{
-    return 0xffffffff;
-}
-
-static void blackhole_write(void *opaque, hwaddr addr, uint64_t val,
-                            unsigned width)
-{
-    /* nothing */
-}
-
-static const MemoryRegionOps blackhole_ops = {
-    .read = blackhole_read,
-    .write = blackhole_write,
-    .valid.min_access_size = 1,
-    .valid.max_access_size = 4,
-    .impl.min_access_size = 4,
-    .impl.max_access_size = 4,
-    .endianness = DEVICE_LITTLE_ENDIAN,
-};
-
 /* PCIe MMCFG */
 static void mch_update_pciexbar(MCHPCIState *mch)
 {
@@ -552,92 +632,6 @@ static void mch_reset(DeviceState *qdev)
     mch_update(mch);
 }
 
-static void mch_realize(PCIDevice *d, Error **errp)
-{
-    int i;
-    MCHPCIState *mch = MCH_PCI_DEVICE(d);
-
-    if (mch->ext_tseg_mbytes > MCH_HOST_BRIDGE_EXT_TSEG_MBYTES_MAX) {
-        error_setg(errp, "invalid extended-tseg-mbytes value: %" PRIu16,
-                   mch->ext_tseg_mbytes);
-        return;
-    }
-
-    /* setup pci memory mapping */
-    pc_pci_as_mapping_init(mch->system_memory, mch->pci_address_space);
-
-    /* if *disabled* show SMRAM to all CPUs */
-    memory_region_init_alias(&mch->smram_region, OBJECT(mch), "smram-region",
-                             mch->pci_address_space, MCH_HOST_BRIDGE_SMRAM_C_BASE,
-                             MCH_HOST_BRIDGE_SMRAM_C_SIZE);
-    memory_region_add_subregion_overlap(mch->system_memory, MCH_HOST_BRIDGE_SMRAM_C_BASE,
-                                        &mch->smram_region, 1);
-    memory_region_set_enabled(&mch->smram_region, true);
-
-    memory_region_init_alias(&mch->open_high_smram, OBJECT(mch), "smram-open-high",
-                             mch->ram_memory, MCH_HOST_BRIDGE_SMRAM_C_BASE,
-                             MCH_HOST_BRIDGE_SMRAM_C_SIZE);
-    memory_region_add_subregion_overlap(mch->system_memory, 0xfeda0000,
-                                        &mch->open_high_smram, 1);
-    memory_region_set_enabled(&mch->open_high_smram, false);
-
-    /* smram, as seen by SMM CPUs */
-    memory_region_init_alias(&mch->low_smram, OBJECT(mch), "smram-low",
-                             mch->ram_memory, MCH_HOST_BRIDGE_SMRAM_C_BASE,
-                             MCH_HOST_BRIDGE_SMRAM_C_SIZE);
-    memory_region_set_enabled(&mch->low_smram, true);
-    memory_region_add_subregion(mch->smram, MCH_HOST_BRIDGE_SMRAM_C_BASE,
-                                &mch->low_smram);
-    memory_region_init_alias(&mch->high_smram, OBJECT(mch), "smram-high",
-                             mch->ram_memory, MCH_HOST_BRIDGE_SMRAM_C_BASE,
-                             MCH_HOST_BRIDGE_SMRAM_C_SIZE);
-    memory_region_set_enabled(&mch->high_smram, true);
-    memory_region_add_subregion(mch->smram, 0xfeda0000, &mch->high_smram);
-
-    memory_region_init_io(&mch->tseg_blackhole, OBJECT(mch),
-                          &blackhole_ops, NULL,
-                          "tseg-blackhole", 0);
-    memory_region_set_enabled(&mch->tseg_blackhole, false);
-    memory_region_add_subregion_overlap(mch->system_memory,
-                                        mch->below_4g_mem_size,
-                                        &mch->tseg_blackhole, 1);
-
-    memory_region_init_alias(&mch->tseg_window, OBJECT(mch), "tseg-window",
-                             mch->ram_memory, mch->below_4g_mem_size, 0);
-    memory_region_set_enabled(&mch->tseg_window, false);
-    memory_region_add_subregion(mch->smram, mch->below_4g_mem_size,
-                                &mch->tseg_window);
-
-    /*
-     * This is not what hardware does, so it's QEMU specific hack.
-     * See commit message for details.
-     */
-    memory_region_init_io(&mch->smbase_blackhole, OBJECT(mch), &blackhole_ops,
-                          NULL, "smbase-blackhole",
-                          MCH_HOST_BRIDGE_SMBASE_SIZE);
-    memory_region_set_enabled(&mch->smbase_blackhole, false);
-    memory_region_add_subregion_overlap(mch->system_memory,
-                                        MCH_HOST_BRIDGE_SMBASE_ADDR,
-                                        &mch->smbase_blackhole, 1);
-
-    memory_region_init_alias(&mch->smbase_window, OBJECT(mch),
-                             "smbase-window", mch->ram_memory,
-                             MCH_HOST_BRIDGE_SMBASE_ADDR,
-                             MCH_HOST_BRIDGE_SMBASE_SIZE);
-    memory_region_set_enabled(&mch->smbase_window, false);
-    memory_region_add_subregion(mch->smram, MCH_HOST_BRIDGE_SMBASE_ADDR,
-                                &mch->smbase_window);
-
-    init_pam(&mch->pam_regions[0], OBJECT(mch), mch->ram_memory,
-             mch->system_memory, mch->pci_address_space,
-             PAM_BIOS_BASE, PAM_BIOS_SIZE);
-    for (i = 0; i < ARRAY_SIZE(mch->pam_regions) - 1; ++i) {
-        init_pam(&mch->pam_regions[i + 1], OBJECT(mch), mch->ram_memory,
-                 mch->system_memory, mch->pci_address_space,
-                 PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
-    }
-}
-
 uint64_t mch_mcfg_base(void)
 {
     bool ambiguous;
@@ -660,7 +654,6 @@ static void mch_class_init(ObjectClass *klass, void *data)
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
 
-    k->realize = mch_realize;
     k->config_write = mch_write_config;
     dc->reset = mch_reset;
     device_class_set_props(dc, mch_props);
-- 
2.39.1

[PATCH 12/12] hw/pci-host/q35: Move MemoryRegion pointers to host device

[Bernhard Beschow]

The pointers are set through the host device's properties and are only
used during its realization phase.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/pci-host/q35.h | 10 +++----
 hw/pci-host/q35.c         | 56 +++++++++++++++++++--------------------
 2 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index a04d5f1a17..9b9ce48ca8 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -40,11 +40,6 @@ struct MCHPCIState {
     PCIDevice parent_obj;
     /*< public >*/
 
-    MemoryRegion *ram_memory;
-    MemoryRegion *pci_address_space;
-    MemoryRegion *system_memory;
-    MemoryRegion *address_space_io;
-    MemoryRegion *smram;
     PAMMemoryRegion pam_regions[PAM_REGIONS_COUNT];
     MemoryRegion smram_region, open_high_smram;
     MemoryRegion low_smram, high_smram;
@@ -61,6 +56,11 @@ struct Q35PCIHost {
     PCIExpressHost parent_obj;
     /*< public >*/
 
+    MemoryRegion *ram_memory;
+    MemoryRegion *pci_address_space;
+    MemoryRegion *system_memory;
+    MemoryRegion *address_space_io;
+    MemoryRegion *smram;
     Range pci_hole;
     uint64_t pci_hole64_size;
     uint32_t short_root_bus;
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 3a7f9185a3..cb8ea58c25 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -79,11 +79,11 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    memory_region_add_subregion(s->mch.address_space_io,
+    memory_region_add_subregion(s->address_space_io,
                                 MCH_HOST_BRIDGE_CONFIG_ADDR, &phb->conf_mem);
     sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_ADDR, 4);
 
-    memory_region_add_subregion(s->mch.address_space_io,
+    memory_region_add_subregion(s->address_space_io,
                                 MCH_HOST_BRIDGE_CONFIG_DATA, &phb->data_mem);
     sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, 4);
 
@@ -99,47 +99,47 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
                      IO_APIC_DEFAULT_ADDRESS - 1);
 
     /* setup pci memory mapping */
-    pc_pci_as_mapping_init(s->mch.system_memory, s->mch.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(&s->mch.smram_region, OBJECT(s), "smram-region",
-                             s->mch.pci_address_space, MCH_HOST_BRIDGE_SMRAM_C_BASE,
+                             s->pci_address_space, MCH_HOST_BRIDGE_SMRAM_C_BASE,
                              MCH_HOST_BRIDGE_SMRAM_C_SIZE);
-    memory_region_add_subregion_overlap(s->mch.system_memory, MCH_HOST_BRIDGE_SMRAM_C_BASE,
+    memory_region_add_subregion_overlap(s->system_memory, MCH_HOST_BRIDGE_SMRAM_C_BASE,
                                         &s->mch.smram_region, 1);
     memory_region_set_enabled(&s->mch.smram_region, true);
 
     memory_region_init_alias(&s->mch.open_high_smram, OBJECT(s), "smram-open-high",
-                             s->mch.ram_memory, MCH_HOST_BRIDGE_SMRAM_C_BASE,
+                             s->ram_memory, MCH_HOST_BRIDGE_SMRAM_C_BASE,
                              MCH_HOST_BRIDGE_SMRAM_C_SIZE);
-    memory_region_add_subregion_overlap(s->mch.system_memory, 0xfeda0000,
+    memory_region_add_subregion_overlap(s->system_memory, 0xfeda0000,
                                         &s->mch.open_high_smram, 1);
     memory_region_set_enabled(&s->mch.open_high_smram, false);
 
     /* smram, as seen by SMM CPUs */
     memory_region_init_alias(&s->mch.low_smram, OBJECT(s), "smram-low",
-                             s->mch.ram_memory, MCH_HOST_BRIDGE_SMRAM_C_BASE,
+                             s->ram_memory, MCH_HOST_BRIDGE_SMRAM_C_BASE,
                              MCH_HOST_BRIDGE_SMRAM_C_SIZE);
     memory_region_set_enabled(&s->mch.low_smram, true);
-    memory_region_add_subregion(s->mch.smram, MCH_HOST_BRIDGE_SMRAM_C_BASE,
+    memory_region_add_subregion(s->smram, MCH_HOST_BRIDGE_SMRAM_C_BASE,
                                 &s->mch.low_smram);
     memory_region_init_alias(&s->mch.high_smram, OBJECT(s), "smram-high",
-                             s->mch.ram_memory, MCH_HOST_BRIDGE_SMRAM_C_BASE,
+                             s->ram_memory, MCH_HOST_BRIDGE_SMRAM_C_BASE,
                              MCH_HOST_BRIDGE_SMRAM_C_SIZE);
     memory_region_set_enabled(&s->mch.high_smram, true);
-    memory_region_add_subregion(s->mch.smram, 0xfeda0000, &s->mch.high_smram);
+    memory_region_add_subregion(s->smram, 0xfeda0000, &s->mch.high_smram);
 
     memory_region_init_io(&s->mch.tseg_blackhole, OBJECT(s),
                           &blackhole_ops, NULL, "tseg-blackhole", 0);
     memory_region_set_enabled(&s->mch.tseg_blackhole, false);
-    memory_region_add_subregion_overlap(s->mch.system_memory,
+    memory_region_add_subregion_overlap(s->system_memory,
                                         s->mch.below_4g_mem_size,
                                         &s->mch.tseg_blackhole, 1);
 
     memory_region_init_alias(&s->mch.tseg_window, OBJECT(s), "tseg-window",
-                             s->mch.ram_memory, s->mch.below_4g_mem_size, 0);
+                             s->ram_memory, s->mch.below_4g_mem_size, 0);
     memory_region_set_enabled(&s->mch.tseg_window, false);
-    memory_region_add_subregion(s->mch.smram, s->mch.below_4g_mem_size,
+    memory_region_add_subregion(s->smram, s->mch.below_4g_mem_size,
                                 &s->mch.tseg_window);
 
     /*
@@ -150,30 +150,30 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
                           NULL, "smbase-blackhole",
                           MCH_HOST_BRIDGE_SMBASE_SIZE);
     memory_region_set_enabled(&s->mch.smbase_blackhole, false);
-    memory_region_add_subregion_overlap(s->mch.system_memory,
+    memory_region_add_subregion_overlap(s->system_memory,
                                         MCH_HOST_BRIDGE_SMBASE_ADDR,
                                         &s->mch.smbase_blackhole, 1);
 
     memory_region_init_alias(&s->mch.smbase_window, OBJECT(s),
-                             "smbase-window", s->mch.ram_memory,
+                             "smbase-window", s->ram_memory,
                              MCH_HOST_BRIDGE_SMBASE_ADDR,
                              MCH_HOST_BRIDGE_SMBASE_SIZE);
     memory_region_set_enabled(&s->mch.smbase_window, false);
-    memory_region_add_subregion(s->mch.smram, MCH_HOST_BRIDGE_SMBASE_ADDR,
+    memory_region_add_subregion(s->smram, MCH_HOST_BRIDGE_SMBASE_ADDR,
                                 &s->mch.smbase_window);
 
-    init_pam(&s->mch.pam_regions[0], OBJECT(s), s->mch.ram_memory,
-             s->mch.system_memory, s->mch.pci_address_space,
+    init_pam(&s->mch.pam_regions[0], OBJECT(s), s->ram_memory,
+             s->system_memory, s->pci_address_space,
              PAM_BIOS_BASE, PAM_BIOS_SIZE);
     for (i = 0; i < ARRAY_SIZE(s->mch.pam_regions) - 1; ++i) {
-        init_pam(&s->mch.pam_regions[i + 1], OBJECT(s), s->mch.ram_memory,
-                 s->mch.system_memory, s->mch.pci_address_space,
+        init_pam(&s->mch.pam_regions[i + 1], OBJECT(s), s->ram_memory,
+                 s->system_memory, s->pci_address_space,
                  PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
     }
 
     phb->bus = pci_root_bus_new(DEVICE(s), "pcie.0",
-                                s->mch.pci_address_space,
-                                s->mch.address_space_io,
+                                s->pci_address_space,
+                                s->address_space_io,
                                 0, TYPE_PCIE_BUS);
 
     qdev_realize(DEVICE(&s->mch), BUS(phb->bus), errp);
@@ -338,23 +338,23 @@ static void q35_host_initfn(Object *obj)
                                    &pehb->size, OBJ_PROP_FLAG_READ);
 
     object_property_add_link(obj, MCH_HOST_PROP_RAM_MEM, TYPE_MEMORY_REGION,
-                             (Object **) &s->mch.ram_memory,
+                             (Object **) &s->ram_memory,
                              qdev_prop_allow_set_link_before_realize, 0);
 
     object_property_add_link(obj, MCH_HOST_PROP_SMRAM_MEM, TYPE_MEMORY_REGION,
-                             (Object **) &s->mch.smram,
+                             (Object **) &s->smram,
                              qdev_prop_allow_set_link_before_realize, 0);
 
     object_property_add_link(obj, MCH_HOST_PROP_PCI_MEM, TYPE_MEMORY_REGION,
-                             (Object **) &s->mch.pci_address_space,
+                             (Object **) &s->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 **) &s->mch.system_memory,
+                             (Object **) &s->system_memory,
                              qdev_prop_allow_set_link_before_realize, 0);
 
     object_property_add_link(obj, MCH_HOST_PROP_IO_MEM, TYPE_MEMORY_REGION,
-                             (Object **) &s->mch.address_space_io,
+                             (Object **) &s->address_space_io,
                              qdev_prop_allow_set_link_before_realize, 0);
 }
 
-- 
2.39.1

Re: [PATCH 00/12] Q35 PCI host fixes and QOM cleanup

[Bernhard Beschow]

Am 14. Februar 2023 13:14:29 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>This series mostly cleans up QOM-related initialization code. It also performs
>
>some modernization and fixing.
>
>
>
>The first patch originates from "PC and ICH9 clanups" series [1] which has been
>
>dropped in v3 in favor of another series [2]. Review comments in [2] suggest it
>
>needs more work, so bring the patch back here.
>
>
>
>Patch 2 fixes a clangd warning and patch 3 modernizes usage of the memory API.
>
>
>
>Patches 4-9 clean up initialization code.
>
>
>
>The last four patches also clean up initialization code with the last patch
>
>doing the actual cleanup.
>

Ping

>
>
>Based-on: <20230213162004.2797-1-shentey@gmail.com>
>
>         "[PATCH v4 0/9] PC cleanups"
>
>
>
>Testing done:
>
>* `make check`
>
>* `make check-avocado`
>
>* `qemu-system-x86_64 -M q35 -m 2G -cdrom \
>
>     manjaro-kde-21.3.2-220704-linux515.iso`
>
>
>
>[1] https://lore.kernel.org/qemu-devel/20230131115326.12454-1-shentey@gmail.com/
>
>[2] https://lore.kernel.org/qemu-devel/20230203180914.49112-1-philmd@linaro.org/
>
>
>
>Bernhard Beschow (12):
>
>  hw/i386/pc_q35: Resolve redundant q35_host variable
>
>  hw/pci-host/q35: Fix contradicting .endianness assignment
>
>  hw/pci-host/q35: Use memory_region_set_address() also for
>
>    tseg_blackhole
>
>  hw/pci-host/q35: Initialize PCMachineState::bus in board code
>
>  hw/pci-host/q35: Initialize "bypass-iommu" property from board code
>
>  hw/pci-host/q35: Initialize properties just once
>
>  hw/pci-host/q35: Initialize PCI hole boundaries just once
>
>  hw/pci-host/q35: Turn PCI hole properties into class properties
>
>  hw/pci-host/q35: Rename local variable to more idiomatic "phb"
>
>  hw/pci-host/q35: Propagate to errp rather than doing error_fatal
>
>  hw/pci-host/q35: Merge mch_realize() into q35_host_realize()
>
>  hw/pci-host/q35: Move MemoryRegion pointers to host device
>
>
>
> include/hw/pci-host/q35.h |  17 +-
>
> hw/i386/pc_q35.c          |  33 ++--
>
> hw/pci-host/q35.c         | 325 ++++++++++++++++++--------------------
>
> 3 files changed, 178 insertions(+), 197 deletions(-)
>
>
>
>-- >
>2.39.1
>
>
>

Re: [PATCH 00/12] Q35 PCI host fixes and QOM cleanup

[Michael S. Tsirkin]

On Tue, Feb 21, 2023 at 03:39:28PM +0000, Bernhard Beschow wrote:
> 
> 
> Am 14. Februar 2023 13:14:29 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
> >This series mostly cleans up QOM-related initialization code. It also performs
> >
> >some modernization and fixing.
> >
> >
> >
> >The first patch originates from "PC and ICH9 clanups" series [1] which has been
> >
> >dropped in v3 in favor of another series [2]. Review comments in [2] suggest it
> >
> >needs more work, so bring the patch back here.
> >
> >
> >
> >Patch 2 fixes a clangd warning and patch 3 modernizes usage of the memory API.
> >
> >
> >
> >Patches 4-9 clean up initialization code.
> >
> >
> >
> >The last four patches also clean up initialization code with the last patch
> >
> >doing the actual cleanup.
> >
> 
> Ping


sent some comments. Philippe was reviewing related patches maybe
he wants to poke at these too.

> >
> >
> >Based-on: <20230213162004.2797-1-shentey@gmail.com>
> >
> >         "[PATCH v4 0/9] PC cleanups"
> >
> >
> >
> >Testing done:
> >
> >* `make check`
> >
> >* `make check-avocado`
> >
> >* `qemu-system-x86_64 -M q35 -m 2G -cdrom \
> >
> >     manjaro-kde-21.3.2-220704-linux515.iso`
> >
> >
> >
> >[1] https://lore.kernel.org/qemu-devel/20230131115326.12454-1-shentey@gmail.com/
> >
> >[2] https://lore.kernel.org/qemu-devel/20230203180914.49112-1-philmd@linaro.org/
> >
> >
> >
> >Bernhard Beschow (12):
> >
> >  hw/i386/pc_q35: Resolve redundant q35_host variable
> >
> >  hw/pci-host/q35: Fix contradicting .endianness assignment
> >
> >  hw/pci-host/q35: Use memory_region_set_address() also for
> >
> >    tseg_blackhole
> >
> >  hw/pci-host/q35: Initialize PCMachineState::bus in board code
> >
> >  hw/pci-host/q35: Initialize "bypass-iommu" property from board code
> >
> >  hw/pci-host/q35: Initialize properties just once
> >
> >  hw/pci-host/q35: Initialize PCI hole boundaries just once
> >
> >  hw/pci-host/q35: Turn PCI hole properties into class properties
> >
> >  hw/pci-host/q35: Rename local variable to more idiomatic "phb"
> >
> >  hw/pci-host/q35: Propagate to errp rather than doing error_fatal
> >
> >  hw/pci-host/q35: Merge mch_realize() into q35_host_realize()
> >
> >  hw/pci-host/q35: Move MemoryRegion pointers to host device
> >
> >
> >
> > include/hw/pci-host/q35.h |  17 +-
> >
> > hw/i386/pc_q35.c          |  33 ++--
> >
> > hw/pci-host/q35.c         | 325 ++++++++++++++++++--------------------
> >
> > 3 files changed, 178 insertions(+), 197 deletions(-)
> >
> >
> >
> >-- >
> >2.39.1
> >
> >
> >

Re: [PATCH 00/12] Q35 PCI host fixes and QOM cleanup

[Bernhard Beschow]

Am 1. März 2023 21:49:37 UTC schrieb "Michael S. Tsirkin" <mst@redhat.com>:
>On Tue, Feb 21, 2023 at 03:39:28PM +0000, Bernhard Beschow wrote:
>> 
>> 
>> Am 14. Februar 2023 13:14:29 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>> >This series mostly cleans up QOM-related initialization code. It also performs
>> >
>> >some modernization and fixing.
>> >
>> >
>> >
>> >The first patch originates from "PC and ICH9 clanups" series [1] which has been
>> >
>> >dropped in v3 in favor of another series [2]. Review comments in [2] suggest it
>> >
>> >needs more work, so bring the patch back here.
>> >
>> >
>> >
>> >Patch 2 fixes a clangd warning and patch 3 modernizes usage of the memory API.
>> >
>> >
>> >
>> >Patches 4-9 clean up initialization code.
>> >
>> >
>> >
>> >The last four patches also clean up initialization code with the last patch
>> >
>> >doing the actual cleanup.
>> >
>> 
>> Ping
>
>
>sent some comments.

I'll look into it over the weekend. I'm quite busy right now.

Best regards,
Bernhard

>Philippe was reviewing related patches maybe
>he wants to poke at these too.
>
>> >
>> >
>> >Based-on: <20230213162004.2797-1-shentey@gmail.com>
>> >
>> >         "[PATCH v4 0/9] PC cleanups"
>> >
>> >
>> >
>> >Testing done:
>> >
>> >* `make check`
>> >
>> >* `make check-avocado`
>> >
>> >* `qemu-system-x86_64 -M q35 -m 2G -cdrom \
>> >
>> >     manjaro-kde-21.3.2-220704-linux515.iso`
>> >
>> >
>> >
>> >[1] https://lore.kernel.org/qemu-devel/20230131115326.12454-1-shentey@gmail.com/
>> >
>> >[2] https://lore.kernel.org/qemu-devel/20230203180914.49112-1-philmd@linaro.org/
>> >
>> >
>> >
>> >Bernhard Beschow (12):
>> >
>> >  hw/i386/pc_q35: Resolve redundant q35_host variable
>> >
>> >  hw/pci-host/q35: Fix contradicting .endianness assignment
>> >
>> >  hw/pci-host/q35: Use memory_region_set_address() also for
>> >
>> >    tseg_blackhole
>> >
>> >  hw/pci-host/q35: Initialize PCMachineState::bus in board code
>> >
>> >  hw/pci-host/q35: Initialize "bypass-iommu" property from board code
>> >
>> >  hw/pci-host/q35: Initialize properties just once
>> >
>> >  hw/pci-host/q35: Initialize PCI hole boundaries just once
>> >
>> >  hw/pci-host/q35: Turn PCI hole properties into class properties
>> >
>> >  hw/pci-host/q35: Rename local variable to more idiomatic "phb"
>> >
>> >  hw/pci-host/q35: Propagate to errp rather than doing error_fatal
>> >
>> >  hw/pci-host/q35: Merge mch_realize() into q35_host_realize()
>> >
>> >  hw/pci-host/q35: Move MemoryRegion pointers to host device
>> >
>> >
>> >
>> > include/hw/pci-host/q35.h |  17 +-
>> >
>> > hw/i386/pc_q35.c          |  33 ++--
>> >
>> > hw/pci-host/q35.c         | 325 ++++++++++++++++++--------------------
>> >
>> > 3 files changed, 178 insertions(+), 197 deletions(-)
>> >
>> >
>> >
>> >-- >
>> >2.39.1
>> >
>> >
>> >
>

Re: [PATCH 00/12] Q35 PCI host fixes and QOM cleanup

[Bernhard Beschow]

Am 2. März 2023 21:54:56 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>
>
>Am 1. März 2023 21:49:37 UTC schrieb "Michael S. Tsirkin" <mst@redhat.com>:
>>On Tue, Feb 21, 2023 at 03:39:28PM +0000, Bernhard Beschow wrote:
>>> 
>>> 
>>> Am 14. Februar 2023 13:14:29 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>>> >This series mostly cleans up QOM-related initialization code. It also performs
>>> >
>>> >some modernization and fixing.
>>> >
>>> >
>>> >
>>> >The first patch originates from "PC and ICH9 clanups" series [1] which has been
>>> >
>>> >dropped in v3 in favor of another series [2]. Review comments in [2] suggest it
>>> >
>>> >needs more work, so bring the patch back here.
>>> >
>>> >
>>> >
>>> >Patch 2 fixes a clangd warning and patch 3 modernizes usage of the memory API.
>>> >
>>> >
>>> >
>>> >Patches 4-9 clean up initialization code.
>>> >
>>> >
>>> >
>>> >The last four patches also clean up initialization code with the last patch
>>> >
>>> >doing the actual cleanup.
>>> >
>>> 
>>> Ping
>>
>>
>>sent some comments.
>
>I'll look into it over the weekend. I'm quite busy right now.

Forgot to mention: v2 is out!

>
>Best regards,
>Bernhard
>
>>Philippe was reviewing related patches maybe
>>he wants to poke at these too.
>>
>>> >
>>> >
>>> >Based-on: <20230213162004.2797-1-shentey@gmail.com>
>>> >
>>> >         "[PATCH v4 0/9] PC cleanups"
>>> >
>>> >
>>> >
>>> >Testing done:
>>> >
>>> >* `make check`
>>> >
>>> >* `make check-avocado`
>>> >
>>> >* `qemu-system-x86_64 -M q35 -m 2G -cdrom \
>>> >
>>> >     manjaro-kde-21.3.2-220704-linux515.iso`
>>> >
>>> >
>>> >
>>> >[1] https://lore.kernel.org/qemu-devel/20230131115326.12454-1-shentey@gmail.com/
>>> >
>>> >[2] https://lore.kernel.org/qemu-devel/20230203180914.49112-1-philmd@linaro.org/
>>> >
>>> >
>>> >
>>> >Bernhard Beschow (12):
>>> >
>>> >  hw/i386/pc_q35: Resolve redundant q35_host variable
>>> >
>>> >  hw/pci-host/q35: Fix contradicting .endianness assignment
>>> >
>>> >  hw/pci-host/q35: Use memory_region_set_address() also for
>>> >
>>> >    tseg_blackhole
>>> >
>>> >  hw/pci-host/q35: Initialize PCMachineState::bus in board code
>>> >
>>> >  hw/pci-host/q35: Initialize "bypass-iommu" property from board code
>>> >
>>> >  hw/pci-host/q35: Initialize properties just once
>>> >
>>> >  hw/pci-host/q35: Initialize PCI hole boundaries just once
>>> >
>>> >  hw/pci-host/q35: Turn PCI hole properties into class properties
>>> >
>>> >  hw/pci-host/q35: Rename local variable to more idiomatic "phb"
>>> >
>>> >  hw/pci-host/q35: Propagate to errp rather than doing error_fatal
>>> >
>>> >  hw/pci-host/q35: Merge mch_realize() into q35_host_realize()
>>> >
>>> >  hw/pci-host/q35: Move MemoryRegion pointers to host device
>>> >
>>> >
>>> >
>>> > include/hw/pci-host/q35.h |  17 +-
>>> >
>>> > hw/i386/pc_q35.c          |  33 ++--
>>> >
>>> > hw/pci-host/q35.c         | 325 ++++++++++++++++++--------------------
>>> >
>>> > 3 files changed, 178 insertions(+), 197 deletions(-)
>>> >
>>> >
>>> >
>>> >-- >
>>> >2.39.1
>>> >
>>> >
>>> >
>>

Re: [PATCH 00/12] Q35 PCI host fixes and QOM cleanup

[Michael S. Tsirkin]

On Tue, Feb 14, 2023 at 02:14:29PM +0100, Bernhard Beschow wrote:
> This series mostly cleans up QOM-related initialization code. It also performs
> some modernization and fixing.
> 
> The first patch originates from "PC and ICH9 clanups" series [1] which has been
> dropped in v3 in favor of another series [2]. Review comments in [2] suggest it
> needs more work, so bring the patch back here.
> 
> Patch 2 fixes a clangd warning and patch 3 modernizes usage of the memory API.

what's the warning? commit log says nothing about it.

> Patches 4-9 clean up initialization code.
> 
> The last four patches also clean up initialization code with the last patch
> doing the actual cleanup.
> 
> Based-on: <20230213162004.2797-1-shentey@gmail.com>
>          "[PATCH v4 0/9] PC cleanups"
> 
> Testing done:
> * `make check`
> * `make check-avocado`
> * `qemu-system-x86_64 -M q35 -m 2G -cdrom \
>      manjaro-kde-21.3.2-220704-linux515.iso`
> 
> [1] https://lore.kernel.org/qemu-devel/20230131115326.12454-1-shentey@gmail.com/
> [2] https://lore.kernel.org/qemu-devel/20230203180914.49112-1-philmd@linaro.org/
> 
> Bernhard Beschow (12):
>   hw/i386/pc_q35: Resolve redundant q35_host variable
>   hw/pci-host/q35: Fix contradicting .endianness assignment
>   hw/pci-host/q35: Use memory_region_set_address() also for
>     tseg_blackhole
>   hw/pci-host/q35: Initialize PCMachineState::bus in board code
>   hw/pci-host/q35: Initialize "bypass-iommu" property from board code
>   hw/pci-host/q35: Initialize properties just once
>   hw/pci-host/q35: Initialize PCI hole boundaries just once
>   hw/pci-host/q35: Turn PCI hole properties into class properties
>   hw/pci-host/q35: Rename local variable to more idiomatic "phb"
>   hw/pci-host/q35: Propagate to errp rather than doing error_fatal
>   hw/pci-host/q35: Merge mch_realize() into q35_host_realize()
>   hw/pci-host/q35: Move MemoryRegion pointers to host device
> 
>  include/hw/pci-host/q35.h |  17 +-
>  hw/i386/pc_q35.c          |  33 ++--
>  hw/pci-host/q35.c         | 325 ++++++++++++++++++--------------------
>  3 files changed, 178 insertions(+), 197 deletions(-)
> 
> -- 
> 2.39.1
>