From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 42314C54EE9 for ; Tue, 20 Sep 2022 08:50:29 +0000 (UTC) Received: from list by lists.xenproject.org with outflank-mailman.409226.652182 (Exim 4.92) (envelope-from ) id 1oaYxD-0008KT-8U; Tue, 20 Sep 2022 08:50:07 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 409226.652182; Tue, 20 Sep 2022 08:50:07 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1oaYxD-0008KM-4r; Tue, 20 Sep 2022 08:50:07 +0000 Received: by outflank-mailman (input) for mailman id 409226; Tue, 20 Sep 2022 08:50:06 +0000 Received: from se1-gles-sth1-in.inumbo.com ([159.253.27.254] helo=se1-gles-sth1.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1oaYxC-0008BU-DI for xen-devel@lists.xenproject.org; Tue, 20 Sep 2022 08:50:06 +0000 Received: from zero.eik.bme.hu (zero.eik.bme.hu [152.66.115.2]) by se1-gles-sth1.inumbo.com (Halon) with ESMTPS id 377ef738-38c1-11ed-9647-05401a9f4f97; Tue, 20 Sep 2022 10:50:04 +0200 (CEST) Received: from zero.eik.bme.hu (blah.eik.bme.hu [152.66.115.182]) by localhost (Postfix) with SMTP id 39177746335; Tue, 20 Sep 2022 10:50:02 +0200 (CEST) Received: by zero.eik.bme.hu (Postfix, from userid 432) id E0C2A74632C; Tue, 20 Sep 2022 10:50:01 +0200 (CEST) Received: from localhost (localhost [127.0.0.1]) by zero.eik.bme.hu (Postfix) with ESMTP id DBD6474632B; Tue, 20 Sep 2022 10:50:01 +0200 (CEST) X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list Sender: "Xen-devel" X-Inumbo-ID: 377ef738-38c1-11ed-9647-05401a9f4f97 Date: Tue, 20 Sep 2022 10:50:01 +0200 (CEST) From: BALATON Zoltan To: =?ISO-8859-15?Q?Philippe_Mathieu-Daud=E9?= cc: Bernhard Beschow , qemu-devel@nongnu.org, "Michael S. Tsirkin" , Magnus Damm , Aleksandar Rikalo , Bandan Das , Matthew Rosato , Daniel Henrique Barboza , Sergio Lopez , Alexey Kardashevskiy , Xiaojuan Yang , Cameron Esfahani , Michael Rolnik , Song Gao , Jagannathan Raman , Greg Kurz , Kamil Rytarowski , Peter Xu , Joel Stanley , Alistair Francis , "Dr. David Alan Gilbert" , Paolo Bonzini , haxm-team@intel.com, Roman Bolshakov , Markus Armbruster , Eric Auger , David Gibson , =?ISO-8859-15?Q?Daniel_P=2E_Berrang=E9?= , Christian Borntraeger , =?ISO-8859-15?Q?C=E9dric_Le_Goater?= , Stefan Hajnoczi , qemu-block@nongnu.org, Eduardo Habkost , =?ISO-8859-15?Q?Herv=E9_Poussineau?= , qemu-ppc@nongnu.org, Cornelia Huck , Palmer Dabbelt , Helge Deller , Stefano Stabellini , qemu-riscv@nongnu.org, Stafford Horne , Paul Durrant , Havard Skinnemoen , Elena Ufimtseva , Alexander Graf , Thomas Huth , Alex Williamson , Wenchao Wang , Tony Krowiak , Marcel Apfelbaum , qemu-s390x@nongnu.org, =?ISO-8859-15?Q?Marc-Andr=E9_Lureau?= , Mark Cave-Ayland , Eric Farman , Reinoud Zandijk , Alexander Bulekov , Yanan Wang , "Edgar E. Iglesias" , Gerd Hoffmann , Tyrone Ting , xen-devel@lists.xenproject.org, Yoshinori Sato , John Snow , Richard Henderson , Darren Kenny , kvm@vger.kernel.org, Qiuhao Li , John G Johnson , Bin Meng , Sunil Muthuswamy , Max Filippov , qemu-arm@nongnu.org, Marcelo Tosatti , Peter Maydell , Anthony Perard , Andrew Jeffery , Artyom Tarasenko , Halil Pasic , "Maciej S. Szmigiero" , Jason Wang , David Hildenbrand , Laurent Vivier , Alistair Francis , Jason Herne Subject: Re: [PATCH 8/9] softmmu/physmem: Let SysBusState absorb memory region and address space singletons In-Reply-To: Message-ID: References: <20220919231720.163121-1-shentey@gmail.com> <20220919231720.163121-9-shentey@gmail.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="3866299591-1142243070-1663663801=:43449" This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --3866299591-1142243070-1663663801=:43449 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8BIT 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 >> --- >> 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; >> } > > > --3866299591-1142243070-1663663801=:43449--