| Message ID | 20220919231720.163121-9-shentey@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Deprecate sysbus_get_default() and get_system_memory() et. al | expand |
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 ¤t_machine->main_system_bus.system_memory; > } > > MemoryRegion *get_system_io(void) > { > - return system_io; > + assert(current_machine); > + > + return ¤t_machine->main_system_bus.system_io; > } > > AddressSpace *get_address_space_memory(void) > { > - return &address_space_memory; > + assert(current_machine); > + > + return ¤t_machine->main_system_bus.address_space_memory; > } > > AddressSpace *get_address_space_io(void) > { > - return &address_space_io; > + assert(current_machine); > + > + return ¤t_machine->main_system_bus.address_space_io; > }
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 ¤t_machine->main_system_bus.system_memory; >> } >> MemoryRegion *get_system_io(void) >> { >> - return system_io; >> + assert(current_machine); >> + >> + return ¤t_machine->main_system_bus.system_io; >> } >> AddressSpace *get_address_space_memory(void) >> { >> - return &address_space_memory; >> + assert(current_machine); >> + >> + return ¤t_machine->main_system_bus.address_space_memory; >> } >> AddressSpace *get_address_space_io(void) >> { >> - return &address_space_io; >> + assert(current_machine); >> + >> + return ¤t_machine->main_system_bus.address_space_io; >> } > > >
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 ¤t_machine->main_system_bus.system_memory; >>> } >>> MemoryRegion *get_system_io(void) >>> { >>> - return system_io; >>> + assert(current_machine); >>> + >>> + return ¤t_machine->main_system_bus.system_io; >>> } >>> AddressSpace *get_address_space_memory(void) >>> { >>> - return &address_space_memory; >>> + assert(current_machine); >>> + >>> + return ¤t_machine->main_system_bus.address_space_memory; >>> } >>> AddressSpace *get_address_space_io(void) >>> { >>> - return &address_space_io; >>> + assert(current_machine); >>> + >>> + return ¤t_machine->main_system_bus.address_space_io; >>> } >> >> >>
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 ¤t_machine->main_system_bus.system_memory; } MemoryRegion *get_system_io(void) { - return system_io; + assert(current_machine); + + return ¤t_machine->main_system_bus.system_io; } AddressSpace *get_address_space_memory(void) { - return &address_space_memory; + assert(current_machine); + + return ¤t_machine->main_system_bus.address_space_memory; } AddressSpace *get_address_space_io(void) { - return &address_space_io; + assert(current_machine); + + return ¤t_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(¤t_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);
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(-)