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

[Bernhard Beschow <shentey@xxxxxxxxx>]

Am 20. September 2022 08:50:01 UTC schrieb BALATON Zoltan <balaton@xxxxxxxxxx>:
>
>
>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@xxxxxxxxx>
>>> ---
>>>   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;
>>>   }
>> 
>> 
>>