[PATCH 0/9] Deprecate sysbus_get_default() and get_system_memory() et. al

[PATCH 0/9] Deprecate sysbus_get_default() and get_system_memory() et. al

[Bernhard Beschow]

In address-spaces.h it can be read that get_system_memory() and
get_system_io() are temporary interfaces which "should only be used temporarily
until a proper bus interface is available". This statement certainly extends to
the address_space_memory and address_space_io singletons. This series attempts
to stop further proliferation of their use by turning TYPE_SYSTEM_BUS into an
object-oriented, "proper bus interface" inspired by PCIBus.

While at it, also the main_system_bus singleton is turned into an attribute of
MachineState. Together, this resolves five singletons in total, making the
ownership relations much more obvious which helps comprehension.

The series is structured as follows: Patch 1 fixes a memory corruption issue
uncovered by running `make check` on the last but one patch of this series.
Patches 2 and 3 turn the main_system_bus singleton into an attribute of
MachineState which provides an alternative to sysbus_get_default(). Patches 4-7
resolve the address space singletons and deprecate the legacy
get_system_memory() et. al functions. Patch 8 attempts to optimize the new
implementations of these legacy functions.

Testing done:
* make check (passes without any issues)
* make check-avocado (no new issues seem to be introduced compared to master)

Bernhard Beschow (9):
  hw/riscv/sifive_e: Fix inheritance of SiFiveEState
  exec/hwaddr.h: Add missing include
  hw/core/sysbus: Resolve main_system_bus singleton
  hw/ppc/spapr: Fix code style problems reported by checkpatch
  exec/address-spaces: Wrap address space singletons into functions
  target/loongarch/cpu: Remove unneeded include directive
  hw/sysbus: Introduce dedicated struct SysBusState for TYPE_SYSTEM_BUS
  softmmu/physmem: Let SysBusState absorb memory region and address
    space singletons
  exec/address-spaces: Inline legacy functions

 accel/hvf/hvf-accel-ops.c            |  2 +-
 accel/kvm/kvm-all.c                  | 12 +++----
 hw/alpha/dp264.c                     |  4 +--
 hw/alpha/typhoon.c                   |  4 +--
 hw/arm/smmu-common.c                 |  4 +--
 hw/arm/smmuv3.c                      | 14 ++++----
 hw/arm/virt.c                        |  2 +-
 hw/char/goldfish_tty.c               |  4 +--
 hw/core/bus.c                        |  5 ++-
 hw/core/loader.c                     |  2 +-
 hw/core/machine.c                    |  3 ++
 hw/core/sysbus.c                     | 24 ++++----------
 hw/dma/pl330.c                       |  2 +-
 hw/dma/rc4030.c                      |  2 +-
 hw/dma/xlnx-zynq-devcfg.c            |  4 +--
 hw/dma/xlnx_dpdma.c                  |  8 ++---
 hw/hppa/machine.c                    |  4 +--
 hw/hyperv/hyperv.c                   |  2 +-
 hw/hyperv/vmbus.c                    |  2 +-
 hw/i386/amd_iommu.c                  | 18 +++++-----
 hw/i386/fw_cfg.c                     |  2 +-
 hw/i386/intel_iommu.c                | 24 +++++++-------
 hw/i386/microvm.c                    |  4 +--
 hw/i386/pc.c                         |  2 +-
 hw/i386/xen/xen-hvm.c                |  4 +--
 hw/ide/ahci.c                        |  2 +-
 hw/ide/macio.c                       | 10 +++---
 hw/intc/apic.c                       |  2 +-
 hw/intc/openpic_kvm.c                |  2 +-
 hw/intc/pnv_xive.c                   |  6 ++--
 hw/intc/pnv_xive2.c                  |  6 ++--
 hw/intc/riscv_aplic.c                |  2 +-
 hw/intc/spapr_xive.c                 |  2 +-
 hw/intc/xive.c                       |  4 +--
 hw/intc/xive2.c                      |  4 +--
 hw/mips/jazz.c                       |  4 +--
 hw/misc/lasi.c                       |  2 +-
 hw/misc/macio/mac_dbdma.c            |  8 ++---
 hw/net/ftgmac100.c                   | 16 ++++-----
 hw/net/i82596.c                      | 24 +++++++-------
 hw/net/imx_fec.c                     | 22 ++++++-------
 hw/net/lasi_i82596.c                 |  2 +-
 hw/net/npcm7xx_emc.c                 | 14 ++++----
 hw/openrisc/boot.c                   |  2 +-
 hw/pci-host/dino.c                   |  6 ++--
 hw/pci-host/pnv_phb3.c               |  6 ++--
 hw/pci-host/pnv_phb3_msi.c           |  6 ++--
 hw/pci-host/pnv_phb4.c               | 10 +++---
 hw/pci/pci.c                         |  2 +-
 hw/ppc/pnv_psi.c                     |  2 +-
 hw/ppc/spapr.c                       |  4 +--
 hw/ppc/spapr_events.c                |  2 +-
 hw/ppc/spapr_hcall.c                 |  4 +--
 hw/ppc/spapr_iommu.c                 |  4 +--
 hw/ppc/spapr_ovec.c                  |  8 ++---
 hw/ppc/spapr_rtas.c                  |  2 +-
 hw/remote/iommu.c                    |  2 +-
 hw/remote/message.c                  |  4 +--
 hw/remote/proxy-memory-listener.c    |  2 +-
 hw/riscv/boot.c                      |  6 ++--
 hw/riscv/sifive_e.c                  |  2 +-
 hw/riscv/sifive_u.c                  |  2 +-
 hw/riscv/virt.c                      |  2 +-
 hw/s390x/css.c                       | 16 ++++-----
 hw/s390x/ipl.h                       |  2 +-
 hw/s390x/s390-pci-bus.c              |  4 +--
 hw/s390x/s390-pci-inst.c             | 10 +++---
 hw/s390x/s390-skeys.c                |  2 +-
 hw/s390x/virtio-ccw.c                | 10 +++---
 hw/sd/sdhci.c                        |  2 +-
 hw/sh4/r2d.c                         |  4 +--
 hw/sparc/sun4m.c                     |  2 +-
 hw/sparc/sun4m_iommu.c               |  4 +--
 hw/sparc64/sun4u_iommu.c             |  4 +--
 hw/timer/hpet.c                      |  2 +-
 hw/usb/hcd-ehci-pci.c                |  2 +-
 hw/usb/hcd-ehci-sysbus.c             |  2 +-
 hw/usb/hcd-ohci.c                    |  2 +-
 hw/usb/hcd-xhci-sysbus.c             |  2 +-
 hw/vfio/ap.c                         |  2 +-
 hw/vfio/ccw.c                        |  2 +-
 hw/vfio/common.c                     |  8 ++---
 hw/vfio/platform.c                   |  2 +-
 hw/virtio/vhost-vdpa.c               |  2 +-
 hw/virtio/vhost.c                    |  2 +-
 hw/virtio/virtio-bus.c               |  4 +--
 hw/virtio/virtio-iommu.c             |  6 ++--
 hw/virtio/virtio-pci.c               |  2 +-
 hw/xen/xen_pt.c                      |  4 +--
 include/exec/address-spaces.h        | 49 +++++++++++++++++++++++-----
 include/exec/hwaddr.h                |  1 +
 include/hw/boards.h                  |  2 ++
 include/hw/elf_ops.h                 |  4 +--
 include/hw/misc/macio/macio.h        |  2 +-
 include/hw/ppc/spapr.h               |  6 ++--
 include/hw/ppc/vof.h                 |  4 +--
 include/hw/riscv/sifive_e.h          |  3 +-
 include/hw/sysbus.h                  | 14 ++++++--
 monitor/misc.c                       |  4 +--
 softmmu/ioport.c                     | 12 +++----
 softmmu/memory_mapping.c             |  2 +-
 softmmu/physmem.c                    | 41 ++++++++---------------
 target/arm/hvf/hvf.c                 |  4 +--
 target/arm/kvm.c                     |  4 +--
 target/avr/helper.c                  |  8 ++---
 target/i386/hax/hax-all.c            |  2 +-
 target/i386/hax/hax-mem.c            |  2 +-
 target/i386/hvf/hvf.c                |  2 +-
 target/i386/hvf/vmx.h                |  2 +-
 target/i386/hvf/x86_mmu.c            |  6 ++--
 target/i386/nvmm/nvmm-all.c          |  4 +--
 target/i386/sev.c                    |  4 +--
 target/i386/tcg/sysemu/misc_helper.c | 12 +++----
 target/i386/whpx/whpx-all.c          |  4 +--
 target/loongarch/cpu.h               |  1 -
 target/s390x/diag.c                  |  2 +-
 target/s390x/mmu_helper.c            |  2 +-
 target/s390x/sigp.c                  |  2 +-
 target/xtensa/dbg_helper.c           |  2 +-
 tests/qtest/fuzz/generic_fuzz.c      |  4 +--
 120 files changed, 355 insertions(+), 328 deletions(-)

-- 
2.37.3

[PATCH 1/9] hw/riscv/sifive_e: Fix inheritance of SiFiveEState

[Bernhard Beschow]

SiFiveEState inherits from SysBusDevice while it's TypeInfo claims it to
inherit from TYPE_MACHINE. This is an inconsistency which can cause
undefined behavior such as memory corruption.

Change SiFiveEState to inherit from MachineState since it is registered
as a machine.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/riscv/sifive_e.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/hw/riscv/sifive_e.h b/include/hw/riscv/sifive_e.h
index 83604da805..d738745925 100644
--- a/include/hw/riscv/sifive_e.h
+++ b/include/hw/riscv/sifive_e.h
@@ -22,6 +22,7 @@
 #include "hw/riscv/riscv_hart.h"
 #include "hw/riscv/sifive_cpu.h"
 #include "hw/gpio/sifive_gpio.h"
+#include "hw/boards.h"
 
 #define TYPE_RISCV_E_SOC "riscv.sifive.e.soc"
 #define RISCV_E_SOC(obj) \
@@ -41,7 +42,7 @@ typedef struct SiFiveESoCState {
 
 typedef struct SiFiveEState {
     /*< private >*/
-    SysBusDevice parent_obj;
+    MachineState parent_obj;
 
     /*< public >*/
     SiFiveESoCState soc;
-- 
2.37.3

[PATCH 2/9] exec/hwaddr.h: Add missing include

[Bernhard Beschow]

The next commit would not compile w/o the include directive.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/exec/hwaddr.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/exec/hwaddr.h b/include/exec/hwaddr.h
index 8f16d179a8..616255317c 100644
--- a/include/exec/hwaddr.h
+++ b/include/exec/hwaddr.h
@@ -3,6 +3,7 @@
 #ifndef HWADDR_H
 #define HWADDR_H
 
+#include "qemu/osdep.h"
 
 #define HWADDR_BITS 64
 /* hwaddr is the type of a physical address (its size can
-- 
2.37.3

[PATCH 3/9] hw/core/sysbus: Resolve main_system_bus singleton

[Bernhard Beschow]

In QEMU, a machine and the main_system_bus always go togehter. Usually
the bus is part of the machine which suggsts to host it there.

Since tere is already a current_machine singleton, all code that
accesses the main_system_bus can be changed (behind the scenes) to go
through current_machine. This resolves a singleton. Futhermore, by
reifying it in code, the every-machine-has-exactly-one-main-system-bus
relationship becomes very obvious.

Note that the main_system_bus attribute is a value rather than a
pointer. This trades pointer dereferences for pointer arithmetic. The
idea is to reduce cache misses - a rule of thumb says that
every pointer dereference causes a cache miss while arithmetic is
basically free.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/core/bus.c       |  5 ++++-
 hw/core/machine.c   |  3 +++
 hw/core/sysbus.c    | 22 +++++-----------------
 include/hw/boards.h |  1 +
 4 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/hw/core/bus.c b/hw/core/bus.c
index c7831b5293..e3e807946c 100644
--- a/hw/core/bus.c
+++ b/hw/core/bus.c
@@ -129,9 +129,12 @@ static void qbus_init_internal(BusState *bus, DeviceState *parent,
         bus->parent->num_child_bus++;
         object_property_add_child(OBJECT(bus->parent), bus->name, OBJECT(bus));
         object_unref(OBJECT(bus));
+
+        /* The only bus without a parent is the main system bus */
+        assert(sysbus_get_default());
     } else {
         /* The only bus without a parent is the main system bus */
-        assert(bus == sysbus_get_default());
+        assert(!sysbus_get_default());
     }
 }
 
diff --git a/hw/core/machine.c b/hw/core/machine.c
index aa520e74a8..ebd3e0ff08 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1097,6 +1097,9 @@ static void machine_initfn(Object *obj)
     ms->smp.threads = 1;
 
     machine_copy_boot_config(ms, &(BootConfiguration){ 0 });
+
+    qbus_init(&ms->main_system_bus, sizeof(ms->main_system_bus),
+              TYPE_SYSTEM_BUS, NULL, "main-system-bus");
 }
 
 static void machine_finalize(Object *obj)
diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index 05c1da3d31..16a9b4d7a0 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -20,6 +20,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qemu/module.h"
+#include "hw/boards.h"
 #include "hw/sysbus.h"
 #include "monitor/monitor.h"
 #include "exec/address-spaces.h"
@@ -336,26 +337,13 @@ static const TypeInfo sysbus_device_type_info = {
     .class_init = sysbus_device_class_init,
 };
 
-static BusState *main_system_bus;
-
-static void main_system_bus_create(void)
-{
-    /*
-     * assign main_system_bus before qbus_init()
-     * in order to make "if (bus != sysbus_get_default())" work
-     */
-    main_system_bus = g_malloc0(system_bus_info.instance_size);
-    qbus_init(main_system_bus, system_bus_info.instance_size,
-              TYPE_SYSTEM_BUS, NULL, "main-system-bus");
-    OBJECT(main_system_bus)->free = g_free;
-}
-
 BusState *sysbus_get_default(void)
 {
-    if (!main_system_bus) {
-        main_system_bus_create();
+    if (!current_machine) {
+        return NULL;
     }
-    return main_system_bus;
+
+    return &current_machine->main_system_bus;
 }
 
 static void sysbus_register_types(void)
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 311ed17e18..7af940102d 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -346,6 +346,7 @@ struct MachineState {
      */
     MemoryRegion *ram;
     DeviceMemoryState *device_memory;
+    BusState main_system_bus;
 
     ram_addr_t ram_size;
     ram_addr_t maxram_size;
-- 
2.37.3

[PATCH 4/9] hw/ppc/spapr: Fix code style problems reported by checkpatch

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/ppc/spapr.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 530d739b1d..04a95669ab 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -848,7 +848,8 @@ static inline uint64_t ppc64_phys_to_real(uint64_t addr)
 
 static inline uint32_t rtas_ld(target_ulong phys, int n)
 {
-    return ldl_be_phys(&address_space_memory, ppc64_phys_to_real(phys + 4*n));
+    return ldl_be_phys(&address_space_memory,
+                       ppc64_phys_to_real(phys + 4 * n));
 }
 
 static inline uint64_t rtas_ldq(target_ulong phys, int n)
@@ -858,7 +859,7 @@ static inline uint64_t rtas_ldq(target_ulong phys, int n)
 
 static inline void rtas_st(target_ulong phys, int n, uint32_t val)
 {
-    stl_be_phys(&address_space_memory, ppc64_phys_to_real(phys + 4*n), val);
+    stl_be_phys(&address_space_memory, ppc64_phys_to_real(phys + 4 * n), val);
 }
 
 typedef void (*spapr_rtas_fn)(PowerPCCPU *cpu, SpaprMachineState *sm,
-- 
2.37.3

[PATCH 6/9] target/loongarch/cpu: Remove unneeded include directive

[Bernhard Beschow]

The cpu is used in both user and system emulation context while sysbus.h
is system-only. Remove it since it's not needed anyway. Furthermore, it
would cause a compile error in the next commit.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 target/loongarch/cpu.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/target/loongarch/cpu.h b/target/loongarch/cpu.h
index dce999aaac..c9ed2cb3e7 100644
--- a/target/loongarch/cpu.h
+++ b/target/loongarch/cpu.h
@@ -13,7 +13,6 @@
 #include "hw/registerfields.h"
 #include "qemu/timer.h"
 #include "exec/memory.h"
-#include "hw/sysbus.h"
 
 #define IOCSRF_TEMP             0
 #define IOCSRF_NODECNT          1
-- 
2.37.3

[PATCH 7/9] hw/sysbus: Introduce dedicated struct SysBusState for TYPE_SYSTEM_BUS

[Bernhard Beschow]

With this out of the way, in the next step, SysBusState gains attributes
for its memory and address recouces.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/core/sysbus.c              | 4 ++--
 include/hw/boards.h           | 3 ++-
 include/hw/misc/macio/macio.h | 2 +-
 include/hw/sysbus.h           | 8 ++++++--
 4 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index 16a9b4d7a0..1100f3ad6c 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -84,7 +84,7 @@ static void system_bus_class_init(ObjectClass *klass, void *data)
 static const TypeInfo system_bus_info = {
     .name = TYPE_SYSTEM_BUS,
     .parent = TYPE_BUS,
-    .instance_size = sizeof(BusState),
+    .instance_size = sizeof(SysBusState),
     .class_init = system_bus_class_init,
 };
 
@@ -343,7 +343,7 @@ BusState *sysbus_get_default(void)
         return NULL;
     }
 
-    return &current_machine->main_system_bus;
+    return &current_machine->main_system_bus.parent_obj;
 }
 
 static void sysbus_register_types(void)
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 7af940102d..63a4f990ea 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -11,6 +11,7 @@
 #include "qemu/module.h"
 #include "qom/object.h"
 #include "hw/core/cpu.h"
+#include "hw/sysbus.h"
 
 #define TYPE_MACHINE_SUFFIX "-machine"
 
@@ -346,7 +347,7 @@ struct MachineState {
      */
     MemoryRegion *ram;
     DeviceMemoryState *device_memory;
-    BusState main_system_bus;
+    SysBusState main_system_bus;
 
     ram_addr_t ram_size;
     ram_addr_t maxram_size;
diff --git a/include/hw/misc/macio/macio.h b/include/hw/misc/macio/macio.h
index 6c05f3bfd2..0944be587f 100644
--- a/include/hw/misc/macio/macio.h
+++ b/include/hw/misc/macio/macio.h
@@ -44,7 +44,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(MacIOBusState, MACIO_BUS)
 
 struct MacIOBusState {
     /*< private >*/
-    BusState parent_obj;
+    SysBusState parent_obj;
 };
 
 /* MacIO IDE */
diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
index 3564b7b6a2..5bb3b88501 100644
--- a/include/hw/sysbus.h
+++ b/include/hw/sysbus.h
@@ -11,9 +11,13 @@
 #define QDEV_MAX_PIO 32
 
 #define TYPE_SYSTEM_BUS "System"
-DECLARE_INSTANCE_CHECKER(BusState, SYSTEM_BUS,
-                         TYPE_SYSTEM_BUS)
+OBJECT_DECLARE_SIMPLE_TYPE(SysBusState, SYSTEM_BUS)
 
+struct SysBusState {
+    /*< private >*/
+    BusState parent_obj;
+    /*< public >*/
+};
 
 #define TYPE_SYS_BUS_DEVICE "sys-bus-device"
 OBJECT_DECLARE_TYPE(SysBusDevice, SysBusDeviceClass,
-- 
2.37.3

[PATCH 8/9] softmmu/physmem: Let SysBusState absorb memory region and address space singletons

[Bernhard Beschow]

These singletons are actually properties of the system bus but so far it
hasn't been modelled that way. Fix this to make this relationship very
obvious.

The idea of the patch is to restrain futher proliferation of the use of
get_system_memory() and get_system_io() which are "temprary interfaces"
"until a proper bus interface is available". This should now be the
case.

Note that the new attributes are values rather than a pointers. This
trades pointer dereferences for pointer arithmetic. The idea is to
reduce cache misses - a rule of thumb says that every pointer
dereference causes a cache miss while arithmetic is basically free.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/exec/address-spaces.h | 19 ++++++++++++---
 include/hw/sysbus.h           |  6 +++++
 softmmu/physmem.c             | 46 ++++++++++++++++++-----------------
 3 files changed, 45 insertions(+), 26 deletions(-)

diff --git a/include/exec/address-spaces.h b/include/exec/address-spaces.h
index d5c8cbd718..b31bd8dcf0 100644
--- a/include/exec/address-spaces.h
+++ b/include/exec/address-spaces.h
@@ -23,17 +23,28 @@
 
 #ifndef CONFIG_USER_ONLY
 
-/* Get the root memory region.  This interface should only be used temporarily
- * until a proper bus interface is available.
+/**
+ * Get the root memory region.  This is a legacy function, provided for
+ * compatibility. Prefer using SysBusState::system_memory directly.
  */
 MemoryRegion *get_system_memory(void);
 
-/* Get the root I/O port region.  This interface should only be used
- * temporarily until a proper bus interface is available.
+/**
+ * Get the root I/O port region.  This is a legacy function, provided for
+ * compatibility. Prefer using SysBusState::system_io directly.
  */
 MemoryRegion *get_system_io(void);
 
+/**
+ * Get the root memory address space.  This is a legacy function, provided for
+ * compatibility. Prefer using SysBusState::address_space_memory directly.
+ */
 AddressSpace *get_address_space_memory(void);
+
+/**
+ * Get the root I/O port address space.  This is a legacy function, provided
+ * for compatibility. Prefer using SysBusState::address_space_io directly.
+ */
 AddressSpace *get_address_space_io(void);
 
 #endif
diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
index 5bb3b88501..516e9091dc 100644
--- a/include/hw/sysbus.h
+++ b/include/hw/sysbus.h
@@ -17,6 +17,12 @@ struct SysBusState {
     /*< private >*/
     BusState parent_obj;
     /*< public >*/
+
+    MemoryRegion system_memory;
+    MemoryRegion system_io;
+
+    AddressSpace address_space_io;
+    AddressSpace address_space_memory;
 };
 
 #define TYPE_SYS_BUS_DEVICE "sys-bus-device"
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 0ac920d446..07e9a9171c 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -86,12 +86,6 @@
  */
 RAMList ram_list = { .blocks = QLIST_HEAD_INITIALIZER(ram_list.blocks) };
 
-static MemoryRegion *system_memory;
-static MemoryRegion *system_io;
-
-static AddressSpace address_space_io;
-static AddressSpace address_space_memory;
-
 static MemoryRegion io_mem_unassigned;
 
 typedef struct PhysPageEntry PhysPageEntry;
@@ -146,7 +140,7 @@ typedef struct subpage_t {
 #define PHYS_SECTION_UNASSIGNED 0
 
 static void io_mem_init(void);
-static void memory_map_init(void);
+static void memory_map_init(SysBusState *sysbus);
 static void tcg_log_global_after_sync(MemoryListener *listener);
 static void tcg_commit(MemoryListener *listener);
 
@@ -2667,37 +2661,45 @@ static void tcg_commit(MemoryListener *listener)
     tlb_flush(cpuas->cpu);
 }
 
-static void memory_map_init(void)
+static void memory_map_init(SysBusState *sysbus)
 {
-    system_memory = g_malloc(sizeof(*system_memory));
+    MemoryRegion *system_memory = &sysbus->system_memory;
+    MemoryRegion *system_io = &sysbus->system_io;
 
     memory_region_init(system_memory, NULL, "system", UINT64_MAX);
-    address_space_init(&address_space_memory, system_memory, "memory");
+    address_space_init(&sysbus->address_space_memory, system_memory, "memory");
 
-    system_io = g_malloc(sizeof(*system_io));
     memory_region_init_io(system_io, NULL, &unassigned_io_ops, NULL, "io",
                           65536);
-    address_space_init(&address_space_io, system_io, "I/O");
+    address_space_init(&sysbus->address_space_io, system_io, "I/O");
 }
 
 MemoryRegion *get_system_memory(void)
 {
-    return system_memory;
+    assert(current_machine);
+
+    return &current_machine->main_system_bus.system_memory;
 }
 
 MemoryRegion *get_system_io(void)
 {
-    return system_io;
+    assert(current_machine);
+
+    return &current_machine->main_system_bus.system_io;
 }
 
 AddressSpace *get_address_space_memory(void)
 {
-    return &address_space_memory;
+    assert(current_machine);
+
+    return &current_machine->main_system_bus.address_space_memory;
 }
 
 AddressSpace *get_address_space_io(void)
 {
-    return &address_space_io;
+    assert(current_machine);
+
+    return &current_machine->main_system_bus.address_space_io;
 }
 
 static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
@@ -3003,7 +3005,7 @@ MemTxResult address_space_set(AddressSpace *as, hwaddr addr,
 void cpu_physical_memory_rw(hwaddr addr, void *buf,
                             hwaddr len, bool is_write)
 {
-    address_space_rw(&address_space_memory, addr, MEMTXATTRS_UNSPECIFIED,
+    address_space_rw(get_address_space_memory(), addr, MEMTXATTRS_UNSPECIFIED,
                      buf, len, is_write);
 }
 
@@ -3074,7 +3076,7 @@ void cpu_flush_icache_range(hwaddr start, hwaddr len)
         return;
     }
 
-    address_space_write_rom_internal(&address_space_memory,
+    address_space_write_rom_internal(get_address_space_memory(),
                                      start, MEMTXATTRS_UNSPECIFIED,
                                      NULL, len, FLUSH_CACHE);
 }
@@ -3140,7 +3142,7 @@ void cpu_exec_init_all(void)
      */
     finalize_target_page_bits();
     io_mem_init();
-    memory_map_init();
+    memory_map_init(&current_machine->main_system_bus);
     qemu_mutex_init(&map_client_list_lock);
 }
 
@@ -3322,14 +3324,14 @@ void *cpu_physical_memory_map(hwaddr addr,
                               hwaddr *plen,
                               bool is_write)
 {
-    return address_space_map(&address_space_memory, addr, plen, is_write,
+    return address_space_map(get_address_space_memory(), addr, plen, is_write,
                              MEMTXATTRS_UNSPECIFIED);
 }
 
 void cpu_physical_memory_unmap(void *buffer, hwaddr len,
                                bool is_write, hwaddr access_len)
 {
-    return address_space_unmap(&address_space_memory, buffer, len,
+    return address_space_unmap(get_address_space_memory(), buffer, len,
                                is_write, access_len);
 }
 
@@ -3554,7 +3556,7 @@ bool cpu_physical_memory_is_io(hwaddr phys_addr)
     bool res;
 
     RCU_READ_LOCK_GUARD();
-    mr = address_space_translate(&address_space_memory,
+    mr = address_space_translate(get_address_space_memory(),
                                  phys_addr, &phys_addr, &l, false,
                                  MEMTXATTRS_UNSPECIFIED);
 
-- 
2.37.3

[PATCH 9/9] exec/address-spaces: Inline legacy functions

[Bernhard Beschow]

The functions just access a global pointer and perform some pointer
arithmetic on top. Allow the compiler to see through this by inlining.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/exec/address-spaces.h | 30 ++++++++++++++++++++++++++----
 softmmu/physmem.c             | 28 ----------------------------
 2 files changed, 26 insertions(+), 32 deletions(-)

diff --git a/include/exec/address-spaces.h b/include/exec/address-spaces.h
index b31bd8dcf0..182af27cad 100644
--- a/include/exec/address-spaces.h
+++ b/include/exec/address-spaces.h
@@ -23,29 +23,51 @@
 
 #ifndef CONFIG_USER_ONLY
 
+#include "hw/boards.h"
+
 /**
  * Get the root memory region.  This is a legacy function, provided for
  * compatibility. Prefer using SysBusState::system_memory directly.
  */
-MemoryRegion *get_system_memory(void);
+inline MemoryRegion *get_system_memory(void)
+{
+    assert(current_machine);
+
+    return &current_machine->main_system_bus.system_memory;
+}
 
 /**
  * Get the root I/O port region.  This is a legacy function, provided for
  * compatibility. Prefer using SysBusState::system_io directly.
  */
-MemoryRegion *get_system_io(void);
+inline MemoryRegion *get_system_io(void)
+{
+    assert(current_machine);
+
+    return &current_machine->main_system_bus.system_io;
+}
 
 /**
  * Get the root memory address space.  This is a legacy function, provided for
  * compatibility. Prefer using SysBusState::address_space_memory directly.
  */
-AddressSpace *get_address_space_memory(void);
+inline AddressSpace *get_address_space_memory(void)
+{
+    assert(current_machine);
+
+    return &current_machine->main_system_bus.address_space_memory;
+}
 
 /**
  * Get the root I/O port address space.  This is a legacy function, provided
  * for compatibility. Prefer using SysBusState::address_space_io directly.
  */
-AddressSpace *get_address_space_io(void);
+inline AddressSpace *get_address_space_io(void)
+{
+    assert(current_machine);
+
+    return &current_machine->main_system_bus.address_space_io;
+}
 
 #endif
 
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 07e9a9171c..dce088f55c 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -2674,34 +2674,6 @@ static void memory_map_init(SysBusState *sysbus)
     address_space_init(&sysbus->address_space_io, system_io, "I/O");
 }
 
-MemoryRegion *get_system_memory(void)
-{
-    assert(current_machine);
-
-    return &current_machine->main_system_bus.system_memory;
-}
-
-MemoryRegion *get_system_io(void)
-{
-    assert(current_machine);
-
-    return &current_machine->main_system_bus.system_io;
-}
-
-AddressSpace *get_address_space_memory(void)
-{
-    assert(current_machine);
-
-    return &current_machine->main_system_bus.address_space_memory;
-}
-
-AddressSpace *get_address_space_io(void)
-{
-    assert(current_machine);
-
-    return &current_machine->main_system_bus.address_space_io;
-}
-
 static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
                                      hwaddr length)
 {
-- 
2.37.3

Re: [PATCH 1/9] hw/riscv/sifive_e: Fix inheritance of SiFiveEState

[Alistair Francis]

On Tue, Sep 20, 2022 at 9:18 AM Bernhard Beschow <shentey@gmail.com> wrote:
>
> SiFiveEState inherits from SysBusDevice while it's TypeInfo claims it to
> inherit from TYPE_MACHINE. This is an inconsistency which can cause
> undefined behavior such as memory corruption.
>
> Change SiFiveEState to inherit from MachineState since it is registered
> as a machine.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  include/hw/riscv/sifive_e.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/riscv/sifive_e.h b/include/hw/riscv/sifive_e.h
> index 83604da805..d738745925 100644
> --- a/include/hw/riscv/sifive_e.h
> +++ b/include/hw/riscv/sifive_e.h
> @@ -22,6 +22,7 @@
>  #include "hw/riscv/riscv_hart.h"
>  #include "hw/riscv/sifive_cpu.h"
>  #include "hw/gpio/sifive_gpio.h"
> +#include "hw/boards.h"
>
>  #define TYPE_RISCV_E_SOC "riscv.sifive.e.soc"
>  #define RISCV_E_SOC(obj) \
> @@ -41,7 +42,7 @@ typedef struct SiFiveESoCState {
>
>  typedef struct SiFiveEState {
>      /*< private >*/
> -    SysBusDevice parent_obj;
> +    MachineState parent_obj;
>
>      /*< public >*/
>      SiFiveESoCState soc;
> --
> 2.37.3
>
>

Re: [PATCH 1/9] hw/riscv/sifive_e: Fix inheritance of SiFiveEState

[Philippe Mathieu-Daudé]

On 20/9/22 01:17, Bernhard Beschow wrote:
> SiFiveEState inherits from SysBusDevice while it's TypeInfo claims it to
> inherit from TYPE_MACHINE. This is an inconsistency which can cause
> undefined behavior such as memory corruption.
> 
> Change SiFiveEState to inherit from MachineState since it is registered
> as a machine.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   include/hw/riscv/sifive_e.h | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/riscv/sifive_e.h b/include/hw/riscv/sifive_e.h
> index 83604da805..d738745925 100644
> --- a/include/hw/riscv/sifive_e.h
> +++ b/include/hw/riscv/sifive_e.h
> @@ -22,6 +22,7 @@
>   #include "hw/riscv/riscv_hart.h"
>   #include "hw/riscv/sifive_cpu.h"
>   #include "hw/gpio/sifive_gpio.h"
> +#include "hw/boards.h"
>   
>   #define TYPE_RISCV_E_SOC "riscv.sifive.e.soc"
>   #define RISCV_E_SOC(obj) \
> @@ -41,7 +42,7 @@ typedef struct SiFiveESoCState {
>   
>   typedef struct SiFiveEState {
>       /*< private >*/
> -    SysBusDevice parent_obj;
> +    MachineState parent_obj;

Ouch.

Fixes: 0869490b1c ("riscv: sifive_e: Manually define the machine")

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 2/9] exec/hwaddr.h: Add missing include

[Philippe Mathieu-Daudé]

On 20/9/22 01:17, Bernhard Beschow wrote:
> The next commit would not compile w/o the include directive.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   include/exec/hwaddr.h | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/include/exec/hwaddr.h b/include/exec/hwaddr.h
> index 8f16d179a8..616255317c 100644
> --- a/include/exec/hwaddr.h
> +++ b/include/exec/hwaddr.h
> @@ -3,6 +3,7 @@
>   #ifndef HWADDR_H
>   #define HWADDR_H
>   
> +#include "qemu/osdep.h"

NAck: This is an anti-pattern. "qemu/osdep.h" must not be included
in .h, only in .c.

Isn't including "hw/qdev-core.h" in "include/hw/boards.h" enough in
the next patch?

Re: [PATCH 3/9] hw/core/sysbus: Resolve main_system_bus singleton

[Philippe Mathieu-Daudé]

On 20/9/22 01:17, Bernhard Beschow wrote:
> In QEMU, a machine and the main_system_bus always go togehter. Usually
> the bus is part of the machine which suggsts to host it there.

"together", "suggests"

> Since tere is already a current_machine singleton, all code that
> accesses the main_system_bus can be changed (behind the scenes) to go
> through current_machine. This resolves a singleton. Futhermore, by

"Furthermore"

> reifying it in code, the every-machine-has-exactly-one-main-system-bus
> relationship becomes very obvious.
> 
> Note that the main_system_bus attribute is a value rather than a
> pointer. This trades pointer dereferences for pointer arithmetic. The
> idea is to reduce cache misses - a rule of thumb says that
> every pointer dereference causes a cache miss while arithmetic is
> basically free.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/core/bus.c       |  5 ++++-
>   hw/core/machine.c   |  3 +++
>   hw/core/sysbus.c    | 22 +++++-----------------
>   include/hw/boards.h |  1 +
>   4 files changed, 13 insertions(+), 18 deletions(-)

> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 311ed17e18..7af940102d 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h

Likely missing the BusState declaration:

   #include "hw/qdev-core.h"

> @@ -346,6 +346,7 @@ struct MachineState {
>        */
>       MemoryRegion *ram;
>       DeviceMemoryState *device_memory;
> +    BusState main_system_bus;
>   
>       ram_addr_t ram_size;
>       ram_addr_t maxram_size;

Re: [PATCH 6/9] target/loongarch/cpu: Remove unneeded include directive

[Philippe Mathieu-Daudé]

On 20/9/22 01:17, Bernhard Beschow wrote:
> The cpu is used in both user and system emulation context while sysbus.h
> is system-only. Remove it since it's not needed anyway. Furthermore, it
> would cause a compile error in the next commit.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   target/loongarch/cpu.h | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/target/loongarch/cpu.h b/target/loongarch/cpu.h
> index dce999aaac..c9ed2cb3e7 100644
> --- a/target/loongarch/cpu.h
> +++ b/target/loongarch/cpu.h
> @@ -13,7 +13,6 @@
>   #include "hw/registerfields.h"
>   #include "qemu/timer.h"
>   #include "exec/memory.h"
> -#include "hw/sysbus.h"
>   
>   #define IOCSRF_TEMP             0
>   #define IOCSRF_NODECNT          1

Renaming the subject as 'target: Remove unneeded "hw/sysbus.h" include 
directive' and fixing target/ppc/kvm.c:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 8/9] softmmu/physmem: Let SysBusState absorb memory region and address space singletons

[Philippe Mathieu-Daudé]

On 20/9/22 01:17, Bernhard Beschow wrote:
> These singletons are actually properties of the system bus but so far it
> hasn't been modelled that way. Fix this to make this relationship very
> obvious.
> 
> The idea of the patch is to restrain futher proliferation of the use of
> get_system_memory() and get_system_io() which are "temprary interfaces"

"further", "temporary"

> "until a proper bus interface is available". This should now be the
> case.
> 
> Note that the new attributes are values rather than a pointers. This
> trades pointer dereferences for pointer arithmetic. The idea is to
> reduce cache misses - a rule of thumb says that every pointer
> dereference causes a cache miss while arithmetic is basically free.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   include/exec/address-spaces.h | 19 ++++++++++++---
>   include/hw/sysbus.h           |  6 +++++
>   softmmu/physmem.c             | 46 ++++++++++++++++++-----------------
>   3 files changed, 45 insertions(+), 26 deletions(-)
> 
> diff --git a/include/exec/address-spaces.h b/include/exec/address-spaces.h
> index d5c8cbd718..b31bd8dcf0 100644
> --- a/include/exec/address-spaces.h
> +++ b/include/exec/address-spaces.h
> @@ -23,17 +23,28 @@
>   
>   #ifndef CONFIG_USER_ONLY
>   
> -/* Get the root memory region.  This interface should only be used temporarily
> - * until a proper bus interface is available.
> +/**
> + * Get the root memory region.  This is a legacy function, provided for
> + * compatibility. Prefer using SysBusState::system_memory directly.
>    */
>   MemoryRegion *get_system_memory(void);

> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
> index 5bb3b88501..516e9091dc 100644
> --- a/include/hw/sysbus.h
> +++ b/include/hw/sysbus.h
> @@ -17,6 +17,12 @@ struct SysBusState {
>       /*< private >*/
>       BusState parent_obj;
>       /*< public >*/
> +
> +    MemoryRegion system_memory;
> +    MemoryRegion system_io;
> +
> +    AddressSpace address_space_io;
> +    AddressSpace address_space_memory;

Alternatively (renaming doc accordingly):

        struct {
            MemoryRegion mr;
            AddressSpace as;
        } io, memory;

>   };
>   
>   #define TYPE_SYS_BUS_DEVICE "sys-bus-device"
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index 0ac920d446..07e9a9171c 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -86,12 +86,6 @@
>    */
>   RAMList ram_list = { .blocks = QLIST_HEAD_INITIALIZER(ram_list.blocks) };
>   
> -static MemoryRegion *system_memory;
> -static MemoryRegion *system_io;
> -
> -static AddressSpace address_space_io;
> -static AddressSpace address_space_memory;
> -
>   static MemoryRegion io_mem_unassigned;
>   
>   typedef struct PhysPageEntry PhysPageEntry;
> @@ -146,7 +140,7 @@ typedef struct subpage_t {
>   #define PHYS_SECTION_UNASSIGNED 0
>   
>   static void io_mem_init(void);
> -static void memory_map_init(void);
> +static void memory_map_init(SysBusState *sysbus);
>   static void tcg_log_global_after_sync(MemoryListener *listener);
>   static void tcg_commit(MemoryListener *listener);
>   
> @@ -2667,37 +2661,45 @@ static void tcg_commit(MemoryListener *listener)
>       tlb_flush(cpuas->cpu);
>   }
>   
> -static void memory_map_init(void)
> +static void memory_map_init(SysBusState *sysbus)
>   {

No need to pass a singleton by argument.

        assert(current_machine);

You can use get_system_memory() and get_system_io() in place :)

LGTM otherwise, great!

> -    system_memory = g_malloc(sizeof(*system_memory));
> +    MemoryRegion *system_memory = &sysbus->system_memory;
> +    MemoryRegion *system_io = &sysbus->system_io;
>   
>       memory_region_init(system_memory, NULL, "system", UINT64_MAX);
> -    address_space_init(&address_space_memory, system_memory, "memory");
> +    address_space_init(&sysbus->address_space_memory, system_memory, "memory");
>   
> -    system_io = g_malloc(sizeof(*system_io));
>       memory_region_init_io(system_io, NULL, &unassigned_io_ops, NULL, "io",
>                             65536);
> -    address_space_init(&address_space_io, system_io, "I/O");
> +    address_space_init(&sysbus->address_space_io, system_io, "I/O");
>   }
>   
>   MemoryRegion *get_system_memory(void)
>   {
> -    return system_memory;
> +    assert(current_machine);
> +
> +    return &current_machine->main_system_bus.system_memory;
>   }
>   
>   MemoryRegion *get_system_io(void)
>   {
> -    return system_io;
> +    assert(current_machine);
> +
> +    return &current_machine->main_system_bus.system_io;
>   }
>   
>   AddressSpace *get_address_space_memory(void)
>   {
> -    return &address_space_memory;
> +    assert(current_machine);
> +
> +    return &current_machine->main_system_bus.address_space_memory;
>   }
>   
>   AddressSpace *get_address_space_io(void)
>   {
> -    return &address_space_io;
> +    assert(current_machine);
> +
> +    return &current_machine->main_system_bus.address_space_io;
>   }

Re: [PATCH 9/9] exec/address-spaces: Inline legacy functions

[Philippe Mathieu-Daudé]

On 20/9/22 01:17, Bernhard Beschow wrote:
> The functions just access a global pointer and perform some pointer
> arithmetic on top. Allow the compiler to see through this by inlining.

I thought about this while reviewing the previous patch, ...

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   include/exec/address-spaces.h | 30 ++++++++++++++++++++++++++----
>   softmmu/physmem.c             | 28 ----------------------------
>   2 files changed, 26 insertions(+), 32 deletions(-)
> 
> diff --git a/include/exec/address-spaces.h b/include/exec/address-spaces.h
> index b31bd8dcf0..182af27cad 100644
> --- a/include/exec/address-spaces.h
> +++ b/include/exec/address-spaces.h
> @@ -23,29 +23,51 @@
>   
>   #ifndef CONFIG_USER_ONLY
>   
> +#include "hw/boards.h"

... but I'm not a fan of including this header here. It is restricted to 
system emulation, but still... Let see what the others think.

>   /**
>    * Get the root memory region.  This is a legacy function, provided for
>    * compatibility. Prefer using SysBusState::system_memory directly.
>    */
> -MemoryRegion *get_system_memory(void);
> +inline MemoryRegion *get_system_memory(void)
> +{
> +    assert(current_machine);
> +
> +    return &current_machine->main_system_bus.system_memory;
> +}
>   
>   /**
>    * Get the root I/O port region.  This is a legacy function, provided for
>    * compatibility. Prefer using SysBusState::system_io directly.
>    */
> -MemoryRegion *get_system_io(void);
> +inline MemoryRegion *get_system_io(void)
> +{
> +    assert(current_machine);
> +
> +    return &current_machine->main_system_bus.system_io;
> +}
>   
>   /**
>    * Get the root memory address space.  This is a legacy function, provided for
>    * compatibility. Prefer using SysBusState::address_space_memory directly.
>    */
> -AddressSpace *get_address_space_memory(void);
> +inline AddressSpace *get_address_space_memory(void)
> +{
> +    assert(current_machine);
> +
> +    return &current_machine->main_system_bus.address_space_memory;
> +}
>   
>   /**
>    * Get the root I/O port address space.  This is a legacy function, provided
>    * for compatibility. Prefer using SysBusState::address_space_io directly.
>    */
> -AddressSpace *get_address_space_io(void);
> +inline AddressSpace *get_address_space_io(void)
> +{
> +    assert(current_machine);
> +
> +    return &current_machine->main_system_bus.address_space_io;
> +}
>   
>   #endif
>   
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index 07e9a9171c..dce088f55c 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -2674,34 +2674,6 @@ static void memory_map_init(SysBusState *sysbus)
>       address_space_init(&sysbus->address_space_io, system_io, "I/O");
>   }
>   
> -MemoryRegion *get_system_memory(void)
> -{
> -    assert(current_machine);
> -
> -    return &current_machine->main_system_bus.system_memory;
> -}
> -
> -MemoryRegion *get_system_io(void)
> -{
> -    assert(current_machine);
> -
> -    return &current_machine->main_system_bus.system_io;
> -}
> -
> -AddressSpace *get_address_space_memory(void)
> -{
> -    assert(current_machine);
> -
> -    return &current_machine->main_system_bus.address_space_memory;
> -}
> -
> -AddressSpace *get_address_space_io(void)
> -{
> -    assert(current_machine);
> -
> -    return &current_machine->main_system_bus.address_space_io;
> -}
> -
>   static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
>                                        hwaddr length)
>   {

Re: [PATCH 9/9] exec/address-spaces: Inline legacy functions

[Philippe Mathieu-Daudé]

On 20/9/22 07:15, Philippe Mathieu-Daudé wrote:
> On 20/9/22 01:17, Bernhard Beschow wrote:
>> The functions just access a global pointer and perform some pointer
>> arithmetic on top. Allow the compiler to see through this by inlining.
> 
> I thought about this while reviewing the previous patch, ...
> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   include/exec/address-spaces.h | 30 ++++++++++++++++++++++++++----
>>   softmmu/physmem.c             | 28 ----------------------------
>>   2 files changed, 26 insertions(+), 32 deletions(-)
>>
>> diff --git a/include/exec/address-spaces.h 
>> b/include/exec/address-spaces.h
>> index b31bd8dcf0..182af27cad 100644
>> --- a/include/exec/address-spaces.h
>> +++ b/include/exec/address-spaces.h
>> @@ -23,29 +23,51 @@
>>   #ifndef CONFIG_USER_ONLY
>> +#include "hw/boards.h"
> 
> ... but I'm not a fan of including this header here. It is restricted to 
> system emulation, but still... Let see what the others think.
> 
>>   /**
>>    * Get the root memory region.  This is a legacy function, provided for
>>    * compatibility. Prefer using SysBusState::system_memory directly.
>>    */
>> -MemoryRegion *get_system_memory(void);
>> +inline MemoryRegion *get_system_memory(void)
>> +{
>> +    assert(current_machine);
>> +
>> +    return &current_machine->main_system_bus.system_memory;
>> +}

Maybe we can simply declare them with __attribute__ ((const)) in the 
previous patch?
See 
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes

Re: [PATCH 5/9] exec/address-spaces: Wrap address space singletons into functions

[Philippe Mathieu-Daudé]

On 20/9/22 01:17, Bernhard Beschow wrote:
> In the next steps, these singletons will be resolved by turning them
> into attributes of the system bus. The system bus is already accessible
> via the global current_machine variable which will be made use of later
> in the wrapper functions.
> 
> All changes have been performed with search-and-replace:
> * s/&address_space_memory/get_address_space_memory()/
> * s/&address_space_io/get_address_space_io()/
> The only exceptions were exec/address-spaces.h and softmmu/physmem.c
> which have been manually changed.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   accel/hvf/hvf-accel-ops.c            |  2 +-
>   accel/kvm/kvm-all.c                  | 12 ++++++------
>   hw/alpha/dp264.c                     |  4 ++--
>   hw/alpha/typhoon.c                   |  4 ++--
>   hw/arm/smmu-common.c                 |  4 ++--
>   hw/arm/smmuv3.c                      | 14 +++++++-------
>   hw/arm/virt.c                        |  2 +-
>   hw/char/goldfish_tty.c               |  4 ++--
>   hw/core/loader.c                     |  2 +-
>   hw/dma/pl330.c                       |  2 +-
>   hw/dma/rc4030.c                      |  2 +-
>   hw/dma/xlnx-zynq-devcfg.c            |  4 ++--
>   hw/dma/xlnx_dpdma.c                  |  8 ++++----
>   hw/hppa/machine.c                    |  4 ++--
>   hw/hyperv/hyperv.c                   |  2 +-
>   hw/hyperv/vmbus.c                    |  2 +-
>   hw/i386/amd_iommu.c                  | 18 +++++++++---------
>   hw/i386/fw_cfg.c                     |  2 +-
>   hw/i386/intel_iommu.c                | 24 ++++++++++++------------
>   hw/i386/microvm.c                    |  4 ++--
>   hw/i386/pc.c                         |  2 +-
>   hw/i386/xen/xen-hvm.c                |  4 ++--
>   hw/ide/ahci.c                        |  2 +-
>   hw/ide/macio.c                       | 10 +++++-----
>   hw/intc/apic.c                       |  2 +-
>   hw/intc/openpic_kvm.c                |  2 +-
>   hw/intc/pnv_xive.c                   |  6 +++---
>   hw/intc/pnv_xive2.c                  |  6 +++---
>   hw/intc/riscv_aplic.c                |  2 +-
>   hw/intc/spapr_xive.c                 |  2 +-
>   hw/intc/xive.c                       |  4 ++--
>   hw/intc/xive2.c                      |  4 ++--
>   hw/mips/jazz.c                       |  4 ++--
>   hw/misc/lasi.c                       |  2 +-
>   hw/misc/macio/mac_dbdma.c            |  8 ++++----
>   hw/net/ftgmac100.c                   | 16 ++++++++--------
>   hw/net/i82596.c                      | 24 ++++++++++++------------
>   hw/net/imx_fec.c                     | 22 +++++++++++-----------
>   hw/net/lasi_i82596.c                 |  2 +-
>   hw/net/npcm7xx_emc.c                 | 14 +++++++-------
>   hw/openrisc/boot.c                   |  2 +-
>   hw/pci-host/dino.c                   |  6 +++---
>   hw/pci-host/pnv_phb3.c               |  6 +++---
>   hw/pci-host/pnv_phb3_msi.c           |  6 +++---
>   hw/pci-host/pnv_phb4.c               | 10 +++++-----
>   hw/pci/pci.c                         |  2 +-
>   hw/ppc/pnv_psi.c                     |  2 +-
>   hw/ppc/spapr.c                       |  4 ++--
>   hw/ppc/spapr_events.c                |  2 +-
>   hw/ppc/spapr_hcall.c                 |  4 ++--
>   hw/ppc/spapr_iommu.c                 |  4 ++--
>   hw/ppc/spapr_ovec.c                  |  8 ++++----
>   hw/ppc/spapr_rtas.c                  |  2 +-
>   hw/remote/iommu.c                    |  2 +-
>   hw/remote/message.c                  |  4 ++--
>   hw/remote/proxy-memory-listener.c    |  2 +-
>   hw/riscv/boot.c                      |  6 +++---
>   hw/riscv/sifive_e.c                  |  2 +-
>   hw/riscv/sifive_u.c                  |  2 +-
>   hw/riscv/virt.c                      |  2 +-
>   hw/s390x/css.c                       | 16 ++++++++--------
>   hw/s390x/ipl.h                       |  2 +-
>   hw/s390x/s390-pci-bus.c              |  4 ++--
>   hw/s390x/s390-pci-inst.c             | 10 +++++-----
>   hw/s390x/s390-skeys.c                |  2 +-
>   hw/s390x/virtio-ccw.c                | 10 +++++-----
>   hw/sd/sdhci.c                        |  2 +-
>   hw/sh4/r2d.c                         |  4 ++--
>   hw/sparc/sun4m.c                     |  2 +-
>   hw/sparc/sun4m_iommu.c               |  4 ++--
>   hw/sparc64/sun4u_iommu.c             |  4 ++--
>   hw/timer/hpet.c                      |  2 +-
>   hw/usb/hcd-ehci-pci.c                |  2 +-
>   hw/usb/hcd-ehci-sysbus.c             |  2 +-
>   hw/usb/hcd-ohci.c                    |  2 +-
>   hw/usb/hcd-xhci-sysbus.c             |  2 +-
>   hw/vfio/ap.c                         |  2 +-
>   hw/vfio/ccw.c                        |  2 +-
>   hw/vfio/common.c                     |  8 ++++----
>   hw/vfio/platform.c                   |  2 +-
>   hw/virtio/vhost-vdpa.c               |  2 +-
>   hw/virtio/vhost.c                    |  2 +-
>   hw/virtio/virtio-bus.c               |  4 ++--
>   hw/virtio/virtio-iommu.c             |  6 +++---
>   hw/virtio/virtio-pci.c               |  2 +-
>   hw/xen/xen_pt.c                      |  4 ++--
>   include/exec/address-spaces.h        |  4 ++--
>   include/hw/elf_ops.h                 |  4 ++--
>   include/hw/ppc/spapr.h               |  5 +++--
>   include/hw/ppc/vof.h                 |  4 ++--
>   monitor/misc.c                       |  4 ++--
>   softmmu/ioport.c                     | 12 ++++++------
>   softmmu/memory_mapping.c             |  2 +-
>   softmmu/physmem.c                    | 17 ++++++++++++++---
>   target/arm/hvf/hvf.c                 |  4 ++--
>   target/arm/kvm.c                     |  4 ++--
>   target/avr/helper.c                  |  8 ++++----
>   target/i386/hax/hax-all.c            |  2 +-
>   target/i386/hax/hax-mem.c            |  2 +-
>   target/i386/hvf/hvf.c                |  2 +-
>   target/i386/hvf/vmx.h                |  2 +-
>   target/i386/hvf/x86_mmu.c            |  6 +++---
>   target/i386/nvmm/nvmm-all.c          |  4 ++--
>   target/i386/sev.c                    |  4 ++--
>   target/i386/tcg/sysemu/misc_helper.c | 12 ++++++------
>   target/i386/whpx/whpx-all.c          |  4 ++--
>   target/s390x/diag.c                  |  2 +-
>   target/s390x/mmu_helper.c            |  2 +-
>   target/s390x/sigp.c                  |  2 +-
>   target/xtensa/dbg_helper.c           |  2 +-
>   tests/qtest/fuzz/generic_fuzz.c      |  4 ++--
>   111 files changed, 285 insertions(+), 273 deletions(-)

Please consider using scripts/git.orderfile for tree-wide refactors,
it helps reviewers.

> diff --git a/include/exec/address-spaces.h b/include/exec/address-spaces.h
> index db8bfa9a92..d5c8cbd718 100644
> --- a/include/exec/address-spaces.h
> +++ b/include/exec/address-spaces.h
> @@ -33,8 +33,8 @@ MemoryRegion *get_system_memory(void);
>    */
>   MemoryRegion *get_system_io(void);
>   
> -extern AddressSpace address_space_memory;
> -extern AddressSpace address_space_io;
> +AddressSpace *get_address_space_memory(void);
> +AddressSpace *get_address_space_io(void);
>   
>   #endif

> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index 56e03e07b5..0ac920d446 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -89,8 +89,8 @@ RAMList ram_list = { .blocks = QLIST_HEAD_INITIALIZER(ram_list.blocks) };
>   static MemoryRegion *system_memory;
>   static MemoryRegion *system_io;
>   
> -AddressSpace address_space_io;
> -AddressSpace address_space_memory;
> +static AddressSpace address_space_io;
> +static AddressSpace address_space_memory;
>   
>   static MemoryRegion io_mem_unassigned;
>   
> @@ -2690,6 +2690,16 @@ MemoryRegion *get_system_io(void)
>       return system_io;
>   }
>   
> +AddressSpace *get_address_space_memory(void)
> +{
> +    return &address_space_memory;
> +}
> +
> +AddressSpace *get_address_space_io(void)
> +{
> +    return &address_space_io;
> +}

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 8/9] softmmu/physmem: Let SysBusState absorb memory region and address space singletons

[BALATON Zoltan]

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



On Tue, 20 Sep 2022, Philippe Mathieu-Daudé via wrote:

> On 20/9/22 01:17, Bernhard Beschow wrote:
>> These singletons are actually properties of the system bus but so far it
>> hasn't been modelled that way. Fix this to make this relationship very
>> obvious.
>> 
>> The idea of the patch is to restrain futher proliferation of the use of
>> get_system_memory() and get_system_io() which are "temprary interfaces"
>
> "further", "temporary"
>
>> "until a proper bus interface is available". This should now be the
>> case.
>> 
>> Note that the new attributes are values rather than a pointers. This
>> trades pointer dereferences for pointer arithmetic. The idea is to
>> reduce cache misses - a rule of thumb says that every pointer
>> dereference causes a cache miss while arithmetic is basically free.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   include/exec/address-spaces.h | 19 ++++++++++++---
>>   include/hw/sysbus.h           |  6 +++++
>>   softmmu/physmem.c             | 46 ++++++++++++++++++-----------------
>>   3 files changed, 45 insertions(+), 26 deletions(-)
>> 
>> diff --git a/include/exec/address-spaces.h b/include/exec/address-spaces.h
>> index d5c8cbd718..b31bd8dcf0 100644
>> --- a/include/exec/address-spaces.h
>> +++ b/include/exec/address-spaces.h
>> @@ -23,17 +23,28 @@
>>     #ifndef CONFIG_USER_ONLY
>>   -/* Get the root memory region.  This interface should only be used 
>> temporarily
>> - * until a proper bus interface is available.
>> +/**
>> + * Get the root memory region.  This is a legacy function, provided for
>> + * compatibility. Prefer using SysBusState::system_memory directly.
>>    */
>>   MemoryRegion *get_system_memory(void);
>
>> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
>> index 5bb3b88501..516e9091dc 100644
>> --- a/include/hw/sysbus.h
>> +++ b/include/hw/sysbus.h
>> @@ -17,6 +17,12 @@ struct SysBusState {
>>       /*< private >*/
>>       BusState parent_obj;
>>       /*< public >*/
>> +
>> +    MemoryRegion system_memory;
>> +    MemoryRegion system_io;
>> +
>> +    AddressSpace address_space_io;
>> +    AddressSpace address_space_memory;
>
> Alternatively (renaming doc accordingly):
>
>       struct {
>           MemoryRegion mr;
>           AddressSpace as;
>       } io, memory;

Do we really need that? Isn't mr just the same as as.root so it would be 
enough to store as only? Or is caching mr and not going through as to get 
it saves time in accessing these? Now we'll go through SysBusState anyway 
instead of accessing globals so is there a performance impact?

Regards,
BALATON Zoltan

>>   };
>>     #define TYPE_SYS_BUS_DEVICE "sys-bus-device"
>> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
>> index 0ac920d446..07e9a9171c 100644
>> --- a/softmmu/physmem.c
>> +++ b/softmmu/physmem.c
>> @@ -86,12 +86,6 @@
>>    */
>>   RAMList ram_list = { .blocks = QLIST_HEAD_INITIALIZER(ram_list.blocks) };
>>   -static MemoryRegion *system_memory;
>> -static MemoryRegion *system_io;
>> -
>> -static AddressSpace address_space_io;
>> -static AddressSpace address_space_memory;
>> -
>>   static MemoryRegion io_mem_unassigned;
>>     typedef struct PhysPageEntry PhysPageEntry;
>> @@ -146,7 +140,7 @@ typedef struct subpage_t {
>>   #define PHYS_SECTION_UNASSIGNED 0
>>     static void io_mem_init(void);
>> -static void memory_map_init(void);
>> +static void memory_map_init(SysBusState *sysbus);
>>   static void tcg_log_global_after_sync(MemoryListener *listener);
>>   static void tcg_commit(MemoryListener *listener);
>>   @@ -2667,37 +2661,45 @@ static void tcg_commit(MemoryListener *listener)
>>       tlb_flush(cpuas->cpu);
>>   }
>>   -static void memory_map_init(void)
>> +static void memory_map_init(SysBusState *sysbus)
>>   {
>
> No need to pass a singleton by argument.
>
>       assert(current_machine);
>
> You can use get_system_memory() and get_system_io() in place :)
>
> LGTM otherwise, great!
>
>> -    system_memory = g_malloc(sizeof(*system_memory));
>> +    MemoryRegion *system_memory = &sysbus->system_memory;
>> +    MemoryRegion *system_io = &sysbus->system_io;
>>         memory_region_init(system_memory, NULL, "system", UINT64_MAX);
>> -    address_space_init(&address_space_memory, system_memory, "memory");
>> +    address_space_init(&sysbus->address_space_memory, system_memory, 
>> "memory");
>>   -    system_io = g_malloc(sizeof(*system_io));
>>       memory_region_init_io(system_io, NULL, &unassigned_io_ops, NULL, 
>> "io",
>>                             65536);
>> -    address_space_init(&address_space_io, system_io, "I/O");
>> +    address_space_init(&sysbus->address_space_io, system_io, "I/O");
>>   }
>>     MemoryRegion *get_system_memory(void)
>>   {
>> -    return system_memory;
>> +    assert(current_machine);
>> +
>> +    return &current_machine->main_system_bus.system_memory;
>>   }
>>     MemoryRegion *get_system_io(void)
>>   {
>> -    return system_io;
>> +    assert(current_machine);
>> +
>> +    return &current_machine->main_system_bus.system_io;
>>   }
>>     AddressSpace *get_address_space_memory(void)
>>   {
>> -    return &address_space_memory;
>> +    assert(current_machine);
>> +
>> +    return &current_machine->main_system_bus.address_space_memory;
>>   }
>>     AddressSpace *get_address_space_io(void)
>>   {
>> -    return &address_space_io;
>> +    assert(current_machine);
>> +
>> +    return &current_machine->main_system_bus.address_space_io;
>>   }
>
>
>

Re: [PATCH 9/9] exec/address-spaces: Inline legacy functions

[BALATON Zoltan]

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



On Tue, 20 Sep 2022, Philippe Mathieu-Daudé via wrote:

> On 20/9/22 01:17, Bernhard Beschow wrote:
>> The functions just access a global pointer and perform some pointer
>> arithmetic on top. Allow the compiler to see through this by inlining.
>
> I thought about this while reviewing the previous patch, ...
>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   include/exec/address-spaces.h | 30 ++++++++++++++++++++++++++----
>>   softmmu/physmem.c             | 28 ----------------------------
>>   2 files changed, 26 insertions(+), 32 deletions(-)
>> 
>> diff --git a/include/exec/address-spaces.h b/include/exec/address-spaces.h
>> index b31bd8dcf0..182af27cad 100644
>> --- a/include/exec/address-spaces.h
>> +++ b/include/exec/address-spaces.h
>> @@ -23,29 +23,51 @@
>>     #ifndef CONFIG_USER_ONLY
>>   +#include "hw/boards.h"
>
> ... but I'm not a fan of including this header here. It is restricted to 
> system emulation, but still... Let see what the others think.

Had the same thought first if this would break user emulation but I don't 
know how that works (and this include is withing !CONFIG_USER_ONLY). I've 
checked in configure now and it seems that softmmu is enabled/disabled 
with system, which reminded me to a previous conversation where I've 
suggested renaming softmmu to sysemu as that better shows what it's really 
used for and maybe the real softmmu part should be split from it but I 
don't remember the details. If it still works with --enable-user 
--disable-system then maybe it's OK and only confusing because of 
misnaming sysemu as softmmu.

Reagrds,
BALATON Zoltan

>>   /**
>>    * Get the root memory region.  This is a legacy function, provided for
>>    * compatibility. Prefer using SysBusState::system_memory directly.
>>    */
>> -MemoryRegion *get_system_memory(void);
>> +inline MemoryRegion *get_system_memory(void)
>> +{
>> +    assert(current_machine);
>> +
>> +    return &current_machine->main_system_bus.system_memory;
>> +}
>>     /**
>>    * Get the root I/O port region.  This is a legacy function, provided for
>>    * compatibility. Prefer using SysBusState::system_io directly.
>>    */
>> -MemoryRegion *get_system_io(void);
>> +inline MemoryRegion *get_system_io(void)
>> +{
>> +    assert(current_machine);
>> +
>> +    return &current_machine->main_system_bus.system_io;
>> +}
>>     /**
>>    * Get the root memory address space.  This is a legacy function, 
>> provided for
>>    * compatibility. Prefer using SysBusState::address_space_memory 
>> directly.
>>    */
>> -AddressSpace *get_address_space_memory(void);
>> +inline AddressSpace *get_address_space_memory(void)
>> +{
>> +    assert(current_machine);
>> +
>> +    return &current_machine->main_system_bus.address_space_memory;
>> +}
>>     /**
>>    * Get the root I/O port address space.  This is a legacy function, 
>> provided
>>    * for compatibility. Prefer using SysBusState::address_space_io 
>> directly.
>>    */
>> -AddressSpace *get_address_space_io(void);
>> +inline AddressSpace *get_address_space_io(void)
>> +{
>> +    assert(current_machine);
>> +
>> +    return &current_machine->main_system_bus.address_space_io;
>> +}
>>     #endif
>>   diff --git a/softmmu/physmem.c b/softmmu/physmem.c
>> index 07e9a9171c..dce088f55c 100644
>> --- a/softmmu/physmem.c
>> +++ b/softmmu/physmem.c
>> @@ -2674,34 +2674,6 @@ static void memory_map_init(SysBusState *sysbus)
>>       address_space_init(&sysbus->address_space_io, system_io, "I/O");
>>   }
>>   -MemoryRegion *get_system_memory(void)
>> -{
>> -    assert(current_machine);
>> -
>> -    return &current_machine->main_system_bus.system_memory;
>> -}
>> -
>> -MemoryRegion *get_system_io(void)
>> -{
>> -    assert(current_machine);
>> -
>> -    return &current_machine->main_system_bus.system_io;
>> -}
>> -
>> -AddressSpace *get_address_space_memory(void)
>> -{
>> -    assert(current_machine);
>> -
>> -    return &current_machine->main_system_bus.address_space_memory;
>> -}
>> -
>> -AddressSpace *get_address_space_io(void)
>> -{
>> -    assert(current_machine);
>> -
>> -    return &current_machine->main_system_bus.address_space_io;
>> -}
>> -
>>   static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
>>                                        hwaddr length)
>>   {
>
>
>

Re: [PATCH 0/9] Deprecate sysbus_get_default() and get_system_memory() et. al

[Peter Maydell]

On Tue, 20 Sept 2022 at 00:18, Bernhard Beschow <shentey@gmail.com> wrote:
>
> In address-spaces.h it can be read that get_system_memory() and
> get_system_io() are temporary interfaces which "should only be used temporarily
> until a proper bus interface is available". This statement certainly extends to
> the address_space_memory and address_space_io singletons.

This is a long standing "we never really completed a cleanup"...

> This series attempts
> to stop further proliferation of their use by turning TYPE_SYSTEM_BUS into an
> object-oriented, "proper bus interface" inspired by PCIBus.
>
> While at it, also the main_system_bus singleton is turned into an attribute of
> MachineState. Together, this resolves five singletons in total, making the
> ownership relations much more obvious which helps comprehension.

...but I don't think this is the direction we want to go.
Overall the reason that the "system memory" and "system IO"
singletons are weird is that in theory they should not be necessary
at all -- board code should create devices and map them into an
entirely arbitrary MemoryRegion or set of MemoryRegions corresponding
to address space(s) for the CPU and for DMA-capable devices. But we
keep them around because
 (a) there is a ton of legacy code that assumes there's only one
     address space in the system and this is it
 (b) when modelling the kind of board where there really is only
     one address space, having the 'system memory' global makes
     the APIs for creating and connecting devices a lot simpler

Retaining the whole-system singleton but shoving it into MachineState
doesn't really change much, IMHO.

More generally, sysbus is rather weird because it isn't really a
bus. Every device in the system of TYPE_SYS_BUS_DEVICE is "on"
the unique TYPE_SYSTEM_BUS bus, but that doesn't mean they're
all in the same address space or that in real hardware they'd
all be on the same bus. sysbus has essentially degraded into a
hack for having devices get reset. I really really need to make
some time to have another look at reset handling. If we get that
right then I think it's probably possible to collapse the few
things TYPE_SYS_BUS_DEVICE does that TYPE_DEVICE does not down
into TYPE_DEVICE and get rid of sysbus altogether...

thanks
-- PMM

Re: [PATCH 4/9] hw/ppc/spapr: Fix code style problems reported by checkpatch

[Daniel Henrique Barboza]

On 9/19/22 20:17, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>


And I've queued it in gitlab.com/danielhb/qemu/tree/ppc-next since it's not
tied with the rest of the series. Thanks,


Daniel

>   include/hw/ppc/spapr.h | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 530d739b1d..04a95669ab 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -848,7 +848,8 @@ static inline uint64_t ppc64_phys_to_real(uint64_t addr)
>   
>   static inline uint32_t rtas_ld(target_ulong phys, int n)
>   {
> -    return ldl_be_phys(&address_space_memory, ppc64_phys_to_real(phys + 4*n));
> +    return ldl_be_phys(&address_space_memory,
> +                       ppc64_phys_to_real(phys + 4 * n));
>   }
>   
>   static inline uint64_t rtas_ldq(target_ulong phys, int n)
> @@ -858,7 +859,7 @@ static inline uint64_t rtas_ldq(target_ulong phys, int n)
>   
>   static inline void rtas_st(target_ulong phys, int n, uint32_t val)
>   {
> -    stl_be_phys(&address_space_memory, ppc64_phys_to_real(phys + 4*n), val);
> +    stl_be_phys(&address_space_memory, ppc64_phys_to_real(phys + 4 * n), val);
>   }
>   
>   typedef void (*spapr_rtas_fn)(PowerPCCPU *cpu, SpaprMachineState *sm,

Re: [PATCH 0/9] Deprecate sysbus_get_default() and get_system_memory() et. al

[Mark Cave-Ayland]

On 20/09/2022 10:55, Peter Maydell wrote:

> On Tue, 20 Sept 2022 at 00:18, Bernhard Beschow <shentey@gmail.com> wrote:
>>
>> In address-spaces.h it can be read that get_system_memory() and
>> get_system_io() are temporary interfaces which "should only be used temporarily
>> until a proper bus interface is available". This statement certainly extends to
>> the address_space_memory and address_space_io singletons.
> 
> This is a long standing "we never really completed a cleanup"...
> 
>> This series attempts
>> to stop further proliferation of their use by turning TYPE_SYSTEM_BUS into an
>> object-oriented, "proper bus interface" inspired by PCIBus.
>>
>> While at it, also the main_system_bus singleton is turned into an attribute of
>> MachineState. Together, this resolves five singletons in total, making the
>> ownership relations much more obvious which helps comprehension.
> 
> ...but I don't think this is the direction we want to go.
> Overall the reason that the "system memory" and "system IO"
> singletons are weird is that in theory they should not be necessary
> at all -- board code should create devices and map them into an
> entirely arbitrary MemoryRegion or set of MemoryRegions corresponding
> to address space(s) for the CPU and for DMA-capable devices. But we
> keep them around because
>   (a) there is a ton of legacy code that assumes there's only one
>       address space in the system and this is it
>   (b) when modelling the kind of board where there really is only
>       one address space, having the 'system memory' global makes
>       the APIs for creating and connecting devices a lot simpler
> 
> Retaining the whole-system singleton but shoving it into MachineState
> doesn't really change much, IMHO.
> 
> More generally, sysbus is rather weird because it isn't really a
> bus. Every device in the system of TYPE_SYS_BUS_DEVICE is "on"
> the unique TYPE_SYSTEM_BUS bus, but that doesn't mean they're
> all in the same address space or that in real hardware they'd
> all be on the same bus. sysbus has essentially degraded into a
> hack for having devices get reset. I really really need to make
> some time to have another look at reset handling. If we get that
> right then I think it's probably possible to collapse the few
> things TYPE_SYS_BUS_DEVICE does that TYPE_DEVICE does not down
> into TYPE_DEVICE and get rid of sysbus altogether...

Following on from one of the discussion points from Alex's KVM Forum BoF session: I 
think longer term what we need to aim for is for QEMU machines to define their own 
address spaces, and then bind those address spaces containing memory-mapped devices 
to one or more CPUs.

Once this in place, as Peter notes above it just remains to solve the reset problem 
and then it becomes possible to eliminate sysbus altogether as everything else can 
already be managed by qdev/QOM.


ATB,

Mark.

Re: [PATCH 0/9] Deprecate sysbus_get_default() and get_system_memory() et. al

[Bernhard Beschow]

Am 20. September 2022 09:55:37 UTC schrieb Peter Maydell <peter.maydell@linaro.org>:
>On Tue, 20 Sept 2022 at 00:18, Bernhard Beschow <shentey@gmail.com> wrote:
>>
>> In address-spaces.h it can be read that get_system_memory() and
>> get_system_io() are temporary interfaces which "should only be used temporarily
>> until a proper bus interface is available". This statement certainly extends to
>> the address_space_memory and address_space_io singletons.
>
>This is a long standing "we never really completed a cleanup"...
>
>> This series attempts
>> to stop further proliferation of their use by turning TYPE_SYSTEM_BUS into an
>> object-oriented, "proper bus interface" inspired by PCIBus.
>>
>> While at it, also the main_system_bus singleton is turned into an attribute of
>> MachineState. Together, this resolves five singletons in total, making the
>> ownership relations much more obvious which helps comprehension.
>
>...but I don't think this is the direction we want to go.
>Overall the reason that the "system memory" and "system IO"
>singletons are weird is that in theory they should not be necessary
>at all -- board code should create devices and map them into an
>entirely arbitrary MemoryRegion or set of MemoryRegions corresponding
>to address space(s) for the CPU and for DMA-capable devices.

My intention was to allow exactly that: By turning sytem memory and system IO into non-singletons, one could have many of them, thus allowing boards to create arbitrary mappings of memory and IO. Since QEMU currently assumes one set (memory and IO) of addresses, I for now instantiated the SysBus once in the machine class to preserve behavior.

>But we
>keep them around because
> (a) there is a ton of legacy code that assumes there's only one
>     address space in the system and this is it
> (b) when modelling the kind of board where there really is only
>     one address space, having the 'system memory' global makes
>     the APIs for creating and connecting devices a lot simpler

Indeed, the APIs may look simpler. The issue I see here though is that devices may make assumptions about these globals which makes the code hard to change in the long run. If devices are given their dependencies by the framework, they must make less assumptions, putting the framework into control. This makes the code more homogenious and therefore easier to change.

>Retaining the whole-system singleton but shoving it into MachineState
>doesn't really change much, IMHO.
>
>More generally, sysbus is rather weird because it isn't really a
>bus. Every device in the system of TYPE_SYS_BUS_DEVICE is "on"
>the unique TYPE_SYSTEM_BUS bus, but that doesn't mean they're
>all in the same address space or that in real hardware they'd
>all be on the same bus.

Again, having multiple SysBuses may solve that issue.

>sysbus has essentially degraded into a
>hack for having devices get reset. I really really need to make
>some time to have another look at reset handling. If we get that
>right then I think it's probably possible to collapse the few
>things TYPE_SYS_BUS_DEVICE does that TYPE_DEVICE does not down
>into TYPE_DEVICE and get rid of sysbus altogether...

There are many SysBusDevices which directly access the globals I intended to deprecate. If those devices could be changed to use the SysBus equivalents instead, this would put the boards in control of memory mappings.

Best regards,
Bernhard

>
>thanks
>-- PMM

Re: [PATCH 0/9] Deprecate sysbus_get_default() and get_system_memory() et. al

[Bernhard Beschow]

Am 20. September 2022 15:36:26 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>On 20/09/2022 10:55, Peter Maydell wrote:
>
>> On Tue, 20 Sept 2022 at 00:18, Bernhard Beschow <shentey@gmail.com> wrote:
>>> 
>>> In address-spaces.h it can be read that get_system_memory() and
>>> get_system_io() are temporary interfaces which "should only be used temporarily
>>> until a proper bus interface is available". This statement certainly extends to
>>> the address_space_memory and address_space_io singletons.
>> 
>> This is a long standing "we never really completed a cleanup"...
>> 
>>> This series attempts
>>> to stop further proliferation of their use by turning TYPE_SYSTEM_BUS into an
>>> object-oriented, "proper bus interface" inspired by PCIBus.
>>> 
>>> While at it, also the main_system_bus singleton is turned into an attribute of
>>> MachineState. Together, this resolves five singletons in total, making the
>>> ownership relations much more obvious which helps comprehension.
>> 
>> ...but I don't think this is the direction we want to go.
>> Overall the reason that the "system memory" and "system IO"
>> singletons are weird is that in theory they should not be necessary
>> at all -- board code should create devices and map them into an
>> entirely arbitrary MemoryRegion or set of MemoryRegions corresponding
>> to address space(s) for the CPU and for DMA-capable devices. But we
>> keep them around because
>>   (a) there is a ton of legacy code that assumes there's only one
>>       address space in the system and this is it
>>   (b) when modelling the kind of board where there really is only
>>       one address space, having the 'system memory' global makes
>>       the APIs for creating and connecting devices a lot simpler
>> 
>> Retaining the whole-system singleton but shoving it into MachineState
>> doesn't really change much, IMHO.
>> 
>> More generally, sysbus is rather weird because it isn't really a
>> bus. Every device in the system of TYPE_SYS_BUS_DEVICE is "on"
>> the unique TYPE_SYSTEM_BUS bus, but that doesn't mean they're
>> all in the same address space or that in real hardware they'd
>> all be on the same bus. sysbus has essentially degraded into a
>> hack for having devices get reset. I really really need to make
>> some time to have another look at reset handling. If we get that
>> right then I think it's probably possible to collapse the few
>> things TYPE_SYS_BUS_DEVICE does that TYPE_DEVICE does not down
>> into TYPE_DEVICE and get rid of sysbus altogether...
>
>Following on from one of the discussion points from Alex's KVM Forum BoF session: I think longer term what we need to aim for is for QEMU machines to define their own address spaces, and then bind those address spaces containing memory-mapped devices to one or more CPUs.

Isn't that more or less impossible with singletons?

>
>Once this in place, as Peter notes above it just remains to solve the reset problem and then it becomes possible to eliminate sysbus altogether as everything else can already be managed by qdev/QOM.

Also see my reply to Peter.

Thanks,
Bernhard
>
>
>ATB,
>
>Mark.

Re: [PATCH 2/9] exec/hwaddr.h: Add missing include

[Bernhard Beschow]

Am 20. September 2022 04:50:51 UTC schrieb "Philippe Mathieu-Daudé" <f4bug@amsat.org>:
>On 20/9/22 01:17, Bernhard Beschow wrote:
>> The next commit would not compile w/o the include directive.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   include/exec/hwaddr.h | 1 +
>>   1 file changed, 1 insertion(+)
>> 
>> diff --git a/include/exec/hwaddr.h b/include/exec/hwaddr.h
>> index 8f16d179a8..616255317c 100644
>> --- a/include/exec/hwaddr.h
>> +++ b/include/exec/hwaddr.h
>> @@ -3,6 +3,7 @@
>>   #ifndef HWADDR_H
>>   #define HWADDR_H
>>   +#include "qemu/osdep.h"
>
>NAck: This is an anti-pattern. "qemu/osdep.h" must not be included
>in .h, only in .c.
>
>Isn't including "hw/qdev-core.h" in "include/hw/boards.h" enough in
>the next patch?

Yes, this works just fine indeed! This patch could be dropped if in the next iteration, if any.

Thanks,
Bernhard

Re: [PATCH 8/9] softmmu/physmem: Let SysBusState absorb memory region and address space singletons

[Bernhard Beschow]

Am 20. September 2022 08:50:01 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>
>
>On Tue, 20 Sep 2022, Philippe Mathieu-Daudé via wrote:
>
>> On 20/9/22 01:17, Bernhard Beschow wrote:
>>> These singletons are actually properties of the system bus but so far it
>>> hasn't been modelled that way. Fix this to make this relationship very
>>> obvious.
>>> 
>>> The idea of the patch is to restrain futher proliferation of the use of
>>> get_system_memory() and get_system_io() which are "temprary interfaces"
>> 
>> "further", "temporary"
>> 
>>> "until a proper bus interface is available". This should now be the
>>> case.
>>> 
>>> Note that the new attributes are values rather than a pointers. This
>>> trades pointer dereferences for pointer arithmetic. The idea is to
>>> reduce cache misses - a rule of thumb says that every pointer
>>> dereference causes a cache miss while arithmetic is basically free.
>>> 
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>>   include/exec/address-spaces.h | 19 ++++++++++++---
>>>   include/hw/sysbus.h           |  6 +++++
>>>   softmmu/physmem.c             | 46 ++++++++++++++++++-----------------
>>>   3 files changed, 45 insertions(+), 26 deletions(-)
>>> 
>>> diff --git a/include/exec/address-spaces.h b/include/exec/address-spaces.h
>>> index d5c8cbd718..b31bd8dcf0 100644
>>> --- a/include/exec/address-spaces.h
>>> +++ b/include/exec/address-spaces.h
>>> @@ -23,17 +23,28 @@
>>>     #ifndef CONFIG_USER_ONLY
>>>   -/* Get the root memory region.  This interface should only be used temporarily
>>> - * until a proper bus interface is available.
>>> +/**
>>> + * Get the root memory region.  This is a legacy function, provided for
>>> + * compatibility. Prefer using SysBusState::system_memory directly.
>>>    */
>>>   MemoryRegion *get_system_memory(void);
>> 
>>> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
>>> index 5bb3b88501..516e9091dc 100644
>>> --- a/include/hw/sysbus.h
>>> +++ b/include/hw/sysbus.h
>>> @@ -17,6 +17,12 @@ struct SysBusState {
>>>       /*< private >*/
>>>       BusState parent_obj;
>>>       /*< public >*/
>>> +
>>> +    MemoryRegion system_memory;
>>> +    MemoryRegion system_io;
>>> +
>>> +    AddressSpace address_space_io;
>>> +    AddressSpace address_space_memory;
>> 
>> Alternatively (renaming doc accordingly):
>> 
>>       struct {
>>           MemoryRegion mr;
>>           AddressSpace as;
>>       } io, memory;
>
>Do we really need that? Isn't mr just the same as as.root so it would be enough to store as only? Or is caching mr and not going through as to get it saves time in accessing these?

as.root is just a pointer. That's why we need mr as a value as well.

> Now we'll go through SysBusState anyway instead of accessing globals so is there a performance impact?

Good question. Since both attributes are now next to each another I'd hope for an improvement ;-) That depends on on many things of course, such as if they are located in the same cache line. As written in the commit messages I tried to minimize pointer dereferences.

Best regards,
Bernhard
>
>Regards,
>BALATON Zoltan
>
>>>   };
>>>     #define TYPE_SYS_BUS_DEVICE "sys-bus-device"
>>> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
>>> index 0ac920d446..07e9a9171c 100644
>>> --- a/softmmu/physmem.c
>>> +++ b/softmmu/physmem.c
>>> @@ -86,12 +86,6 @@
>>>    */
>>>   RAMList ram_list = { .blocks = QLIST_HEAD_INITIALIZER(ram_list.blocks) };
>>>   -static MemoryRegion *system_memory;
>>> -static MemoryRegion *system_io;
>>> -
>>> -static AddressSpace address_space_io;
>>> -static AddressSpace address_space_memory;
>>> -
>>>   static MemoryRegion io_mem_unassigned;
>>>     typedef struct PhysPageEntry PhysPageEntry;
>>> @@ -146,7 +140,7 @@ typedef struct subpage_t {
>>>   #define PHYS_SECTION_UNASSIGNED 0
>>>     static void io_mem_init(void);
>>> -static void memory_map_init(void);
>>> +static void memory_map_init(SysBusState *sysbus);
>>>   static void tcg_log_global_after_sync(MemoryListener *listener);
>>>   static void tcg_commit(MemoryListener *listener);
>>>   @@ -2667,37 +2661,45 @@ static void tcg_commit(MemoryListener *listener)
>>>       tlb_flush(cpuas->cpu);
>>>   }
>>>   -static void memory_map_init(void)
>>> +static void memory_map_init(SysBusState *sysbus)
>>>   {
>> 
>> No need to pass a singleton by argument.
>> 
>>       assert(current_machine);
>> 
>> You can use get_system_memory() and get_system_io() in place :)
>> 
>> LGTM otherwise, great!
>> 
>>> -    system_memory = g_malloc(sizeof(*system_memory));
>>> +    MemoryRegion *system_memory = &sysbus->system_memory;
>>> +    MemoryRegion *system_io = &sysbus->system_io;
>>>         memory_region_init(system_memory, NULL, "system", UINT64_MAX);
>>> -    address_space_init(&address_space_memory, system_memory, "memory");
>>> +    address_space_init(&sysbus->address_space_memory, system_memory, "memory");
>>>   -    system_io = g_malloc(sizeof(*system_io));
>>>       memory_region_init_io(system_io, NULL, &unassigned_io_ops, NULL, "io",
>>>                             65536);
>>> -    address_space_init(&address_space_io, system_io, "I/O");
>>> +    address_space_init(&sysbus->address_space_io, system_io, "I/O");
>>>   }
>>>     MemoryRegion *get_system_memory(void)
>>>   {
>>> -    return system_memory;
>>> +    assert(current_machine);
>>> +
>>> +    return &current_machine->main_system_bus.system_memory;
>>>   }
>>>     MemoryRegion *get_system_io(void)
>>>   {
>>> -    return system_io;
>>> +    assert(current_machine);
>>> +
>>> +    return &current_machine->main_system_bus.system_io;
>>>   }
>>>     AddressSpace *get_address_space_memory(void)
>>>   {
>>> -    return &address_space_memory;
>>> +    assert(current_machine);
>>> +
>>> +    return &current_machine->main_system_bus.address_space_memory;
>>>   }
>>>     AddressSpace *get_address_space_io(void)
>>>   {
>>> -    return &address_space_io;
>>> +    assert(current_machine);
>>> +
>>> +    return &current_machine->main_system_bus.address_space_io;
>>>   }
>> 
>> 
>> 

Re: [PATCH 9/9] exec/address-spaces: Inline legacy functions

[Bernhard Beschow]

Am 20. September 2022 09:02:41 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>
>
>On Tue, 20 Sep 2022, Philippe Mathieu-Daudé via wrote:
>
>> On 20/9/22 01:17, Bernhard Beschow wrote:
>>> The functions just access a global pointer and perform some pointer
>>> arithmetic on top. Allow the compiler to see through this by inlining.
>> 
>> I thought about this while reviewing the previous patch, ...
>> 
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>>   include/exec/address-spaces.h | 30 ++++++++++++++++++++++++++----
>>>   softmmu/physmem.c             | 28 ----------------------------
>>>   2 files changed, 26 insertions(+), 32 deletions(-)
>>> 
>>> diff --git a/include/exec/address-spaces.h b/include/exec/address-spaces.h
>>> index b31bd8dcf0..182af27cad 100644
>>> --- a/include/exec/address-spaces.h
>>> +++ b/include/exec/address-spaces.h
>>> @@ -23,29 +23,51 @@
>>>     #ifndef CONFIG_USER_ONLY
>>>   +#include "hw/boards.h"
>> 
>> ... but I'm not a fan of including this header here. It is restricted to system emulation, but still... Let see what the others think.
>
>Had the same thought first if this would break user emulation but I don't know how that works (and this include is withing !CONFIG_USER_ONLY). I've checked in configure now and it seems that softmmu is enabled/disabled with system, which reminded me to a previous conversation where I've suggested renaming softmmu to sysemu as that better shows what it's really used for and maybe the real softmmu part should be split from it but I don't remember the details. If it still works with --enable-user --disable-system then maybe it's OK and only confusing because of misnaming sysemu as softmmu.

I've compiled all architectures w/o any --{enable,disable}-{user,system} flags and I had compile errors only when putting the include outside the guard. So this in particular doesn't seem to be a problem.

Best regards,
Bernhard
>
>Reagrds,
>BALATON Zoltan
>
>>>   /**
>>>    * Get the root memory region.  This is a legacy function, provided for
>>>    * compatibility. Prefer using SysBusState::system_memory directly.
>>>    */
>>> -MemoryRegion *get_system_memory(void);
>>> +inline MemoryRegion *get_system_memory(void)
>>> +{
>>> +    assert(current_machine);
>>> +
>>> +    return &current_machine->main_system_bus.system_memory;
>>> +}
>>>     /**
>>>    * Get the root I/O port region.  This is a legacy function, provided for
>>>    * compatibility. Prefer using SysBusState::system_io directly.
>>>    */
>>> -MemoryRegion *get_system_io(void);
>>> +inline MemoryRegion *get_system_io(void)
>>> +{
>>> +    assert(current_machine);
>>> +
>>> +    return &current_machine->main_system_bus.system_io;
>>> +}
>>>     /**
>>>    * Get the root memory address space.  This is a legacy function, provided for
>>>    * compatibility. Prefer using SysBusState::address_space_memory directly.
>>>    */
>>> -AddressSpace *get_address_space_memory(void);
>>> +inline AddressSpace *get_address_space_memory(void)
>>> +{
>>> +    assert(current_machine);
>>> +
>>> +    return &current_machine->main_system_bus.address_space_memory;
>>> +}
>>>     /**
>>>    * Get the root I/O port address space.  This is a legacy function, provided
>>>    * for compatibility. Prefer using SysBusState::address_space_io directly.
>>>    */
>>> -AddressSpace *get_address_space_io(void);
>>> +inline AddressSpace *get_address_space_io(void)
>>> +{
>>> +    assert(current_machine);
>>> +
>>> +    return &current_machine->main_system_bus.address_space_io;
>>> +}
>>>     #endif
>>>   diff --git a/softmmu/physmem.c b/softmmu/physmem.c
>>> index 07e9a9171c..dce088f55c 100644
>>> --- a/softmmu/physmem.c
>>> +++ b/softmmu/physmem.c
>>> @@ -2674,34 +2674,6 @@ static void memory_map_init(SysBusState *sysbus)
>>>       address_space_init(&sysbus->address_space_io, system_io, "I/O");
>>>   }
>>>   -MemoryRegion *get_system_memory(void)
>>> -{
>>> -    assert(current_machine);
>>> -
>>> -    return &current_machine->main_system_bus.system_memory;
>>> -}
>>> -
>>> -MemoryRegion *get_system_io(void)
>>> -{
>>> -    assert(current_machine);
>>> -
>>> -    return &current_machine->main_system_bus.system_io;
>>> -}
>>> -
>>> -AddressSpace *get_address_space_memory(void)
>>> -{
>>> -    assert(current_machine);
>>> -
>>> -    return &current_machine->main_system_bus.address_space_memory;
>>> -}
>>> -
>>> -AddressSpace *get_address_space_io(void)
>>> -{
>>> -    assert(current_machine);
>>> -
>>> -    return &current_machine->main_system_bus.address_space_io;
>>> -}
>>> -
>>>   static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
>>>                                        hwaddr length)
>>>   {
>> 
>> 
>> 

Re: [PATCH 1/9] hw/riscv/sifive_e: Fix inheritance of SiFiveEState

[Bernhard Beschow]

Am 20. September 2022 11:36:47 UTC schrieb Markus Armbruster <armbru@redhat.com>:
>Alistair Francis <alistair23@gmail.com> writes:
>
>> On Tue, Sep 20, 2022 at 9:18 AM Bernhard Beschow <shentey@gmail.com> wrote:
>>>
>>> SiFiveEState inherits from SysBusDevice while it's TypeInfo claims it to
>>> inherit from TYPE_MACHINE. This is an inconsistency which can cause
>>> undefined behavior such as memory corruption.
>>>
>>> Change SiFiveEState to inherit from MachineState since it is registered
>>> as a machine.
>>>
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>
>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>
>To the SiFive maintainers: since this is a bug fix, let's merge it right
>away.

I could repost this particular patch with the three new tags (incl. Fixes) if desired.

Best regards,
Bernhard
>

Re: [PATCH 0/9] Deprecate sysbus_get_default() and get_system_memory() et. al

[Peter Maydell]

On Tue, 20 Sept 2022 at 23:50, Bernhard Beschow <shentey@gmail.com> wrote:
>
> Am 20. September 2022 09:55:37 UTC schrieb Peter Maydell <peter.maydell@linaro.org>:
> >On Tue, 20 Sept 2022 at 00:18, Bernhard Beschow <shentey@gmail.com> wrote:
> >>
> >> In address-spaces.h it can be read that get_system_memory() and
> >> get_system_io() are temporary interfaces which "should only be used temporarily
> >> until a proper bus interface is available". This statement certainly extends to
> >> the address_space_memory and address_space_io singletons.
> >
> >This is a long standing "we never really completed a cleanup"...
> >
> >> This series attempts
> >> to stop further proliferation of their use by turning TYPE_SYSTEM_BUS into an
> >> object-oriented, "proper bus interface" inspired by PCIBus.
> >>
> >> While at it, also the main_system_bus singleton is turned into an attribute of
> >> MachineState. Together, this resolves five singletons in total, making the
> >> ownership relations much more obvious which helps comprehension.
> >
> >...but I don't think this is the direction we want to go.
> >Overall the reason that the "system memory" and "system IO"
> >singletons are weird is that in theory they should not be necessary
> >at all -- board code should create devices and map them into an
> >entirely arbitrary MemoryRegion or set of MemoryRegions corresponding
> >to address space(s) for the CPU and for DMA-capable devices.
>
> My intention was to allow exactly that: By turning sytem memory and system IO into non-singletons, one could have many of them, thus allowing boards to create arbitrary mappings of memory and IO. Since QEMU currently assumes one set (memory and IO) of addresses, I for now instantiated the SysBus once in the machine class to preserve behavior.

You can already create arbitrary mappings of memory and IO
(look at the virt board for an example). The existence of the
legacy singleton system-memory and system-io doesn't prevent that,
and stuffing the singletons into the MachineState doesn't do
anything to change the code that is relying on the singletons.

> >Retaining the whole-system singleton but shoving it into MachineState
> >doesn't really change much, IMHO.
> >
> >More generally, sysbus is rather weird because it isn't really a
> >bus. Every device in the system of TYPE_SYS_BUS_DEVICE is "on"
> >the unique TYPE_SYSTEM_BUS bus, but that doesn't mean they're
> >all in the same address space or that in real hardware they'd
> >all be on the same bus.
>
> Again, having multiple SysBuses may solve that issue.

We definitely don't want multiple sysbuses.

thanks
-- PMM

Re: [PATCH 1/9] hw/riscv/sifive_e: Fix inheritance of SiFiveEState

[B]

Am 21. September 2022 04:55:02 UTC schrieb Markus Armbruster <armbru@redhat.com>:
>Bernhard Beschow <shentey@gmail.com> writes:
>
>> Am 20. September 2022 11:36:47 UTC schrieb Markus Armbruster <armbru@redhat.com>:
>>>Alistair Francis <alistair23@gmail.com> writes:
>>>
>>>> On Tue, Sep 20, 2022 at 9:18 AM Bernhard Beschow <shentey@gmail.com> wrote:
>>>>>
>>>>> SiFiveEState inherits from SysBusDevice while it's TypeInfo claims it to
>>>>> inherit from TYPE_MACHINE. This is an inconsistency which can cause
>>>>> undefined behavior such as memory corruption.
>>>>>
>>>>> Change SiFiveEState to inherit from MachineState since it is registered
>>>>> as a machine.
>>>>>
>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>>
>>>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>>>
>>>To the SiFive maintainers: since this is a bug fix, let's merge it right
>>>away.
>>
>> I could repost this particular patch with the three new tags (incl. Fixes) if desired.
>
>Can't hurt, and could help the maintainers.

[X] Done.

Best regards,
Bernhard