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