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

[Bernhard Beschow <shentey@gmail.com>]

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