1. Patchew
  2. QEMU
  3. [PATCH 0/9] Deprecate sysbus_get_default() and get_system_memory() et. al
  4. View patch

[PATCH 9/9] exec/address-spaces: Inline legacy functions

Bernhard Beschow posted 9 patches 3 years, 4 months ago
Maintainers: Cameron Esfahani <dirty@apple.com>, Roman Bolshakov <r.bolshakov@yadro.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eric Auger <eric.auger@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, Laurent Vivier <laurent@vivier.eu>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Philippe Mathieu-Daudé" <f4bug@amsat.org>, Yanan Wang <wangyanan55@huawei.com>, "Hervé Poussineau" <hpoussin@reactos.org>, Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>, Alistair Francis <alistair@alistair23.me>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, Helge Deller <deller@gmx.de>, "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>, "Michael S. Tsirkin" <mst@redhat.com>, Peter Xu <peterx@redhat.com>, Jason Wang <jasowang@redhat.com>, Sergio Lopez <slp@redhat.com>, Stefano Stabellini <sstabellini@kernel.org>, Anthony Perard <anthony.perard@citrix.com>, Paul Durrant <paul@xen.org>, John Snow <jsnow@redhat.com>, "Cédric Le Goater" <clg@kaod.org>, Daniel Henrique Barboza <danielhb413@gmail.com>, David Gibson <david@gibson.dropbear.id.au>, Greg Kurz <groug@kaod.org>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Andrew Jeffery <andrew@aj.id.au>, Joel Stanley <joel@jms.id.au>, Havard Skinnemoen <hskinnemoen@google.com>, Tyrone Ting <kfting@nuvoton.com>, Stafford Horne <shorne@gmail.com>, Elena Ufimtseva <elena.ufimtseva@oracle.com>, Jagannathan Raman <jag.raman@oracle.com>, John G Johnson <john.g.johnson@oracle.com>, Palmer Dabbelt <palmer@dabbelt.com>, Bin Meng <bin.meng@windriver.com>, Halil Pasic <pasic@linux.ibm.com>, Christian Borntraeger <borntraeger@linux.ibm.com>, Eric Farman <farman@linux.ibm.com>, David Hildenbrand <david@redhat.com>, Cornelia Huck <cohuck@redhat.com>, Thomas Huth <thuth@redhat.com>, Matthew Rosato <mjrosato@linux.ibm.com>, Yoshinori Sato <ysato@users.sourceforge.jp>, Magnus Damm <magnus.damm@gmail.com>, Artyom Tarasenko <atar4qemu@gmail.com>, Gerd Hoffmann <kraxel@redhat.com>, Alex Williamson <alex.williamson@redhat.com>, Tony Krowiak <akrowiak@linux.ibm.com>, Jason Herne <jjherne@linux.ibm.com>, Alexey Kardashevskiy <aik@ozlabs.ru>, Markus Armbruster <armbru@redhat.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Alexander Graf <agraf@csgraf.de>, Michael Rolnik <mrolnik@gmail.com>, Wenchao Wang <wenchao.wang@intel.com>, Kamil Rytarowski <kamil@netbsd.org>, Reinoud Zandijk <reinoud@netbsd.org>, Marcelo Tosatti <mtosatti@redhat.com>, Sunil Muthuswamy <sunilmut@microsoft.com>, Song Gao <gaosong@loongson.cn>, Xiaojuan Yang <yangxiaojuan@loongson.cn>, Max Filippov <jcmvbkbc@gmail.com>, Alexander Bulekov <alxndr@bu.edu>, Bandan Das <bsd@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, Darren Kenny <darren.kenny@oracle.com>, Qiuhao Li <Qiuhao.Li@outlook.com>
[PATCH 9/9] exec/address-spaces: Inline legacy functions
Posted by Bernhard Beschow 3 years, 4 months ago 
The functions just access a global pointer and perform some pointer
arithmetic on top. Allow the compiler to see through this by inlining.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/exec/address-spaces.h | 30 ++++++++++++++++++++++++++----
 softmmu/physmem.c             | 28 ----------------------------
 2 files changed, 26 insertions(+), 32 deletions(-)

diff --git a/include/exec/address-spaces.h b/include/exec/address-spaces.h
index b31bd8dcf0..182af27cad 100644
--- a/include/exec/address-spaces.h
+++ b/include/exec/address-spaces.h
@@ -23,29 +23,51 @@
 
 #ifndef CONFIG_USER_ONLY
 
+#include "hw/boards.h"
+
 /**
  * Get the root memory region.  This is a legacy function, provided for
  * compatibility. Prefer using SysBusState::system_memory directly.
  */
-MemoryRegion *get_system_memory(void);
+inline MemoryRegion *get_system_memory(void)
+{
+    assert(current_machine);
+
+    return &current_machine->main_system_bus.system_memory;
+}
 
 /**
  * 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);
+inline MemoryRegion *get_system_io(void)
+{
+    assert(current_machine);
+
+    return &current_machine->main_system_bus.system_io;
+}
 
 /**
  * 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);
+inline AddressSpace *get_address_space_memory(void)
+{
+    assert(current_machine);
+
+    return &current_machine->main_system_bus.address_space_memory;
+}
 
 /**
  * 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);
+inline AddressSpace *get_address_space_io(void)
+{
+    assert(current_machine);
+
+    return &current_machine->main_system_bus.address_space_io;
+}
 
 #endif
 
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 07e9a9171c..dce088f55c 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -2674,34 +2674,6 @@ static void memory_map_init(SysBusState *sysbus)
     address_space_init(&sysbus->address_space_io, system_io, "I/O");
 }
 
-MemoryRegion *get_system_memory(void)
-{
-    assert(current_machine);
-
-    return &current_machine->main_system_bus.system_memory;
-}
-
-MemoryRegion *get_system_io(void)
-{
-    assert(current_machine);
-
-    return &current_machine->main_system_bus.system_io;
-}
-
-AddressSpace *get_address_space_memory(void)
-{
-    assert(current_machine);
-
-    return &current_machine->main_system_bus.address_space_memory;
-}
-
-AddressSpace *get_address_space_io(void)
-{
-    assert(current_machine);
-
-    return &current_machine->main_system_bus.address_space_io;
-}
-
 static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
                                      hwaddr length)
 {
-- 
2.37.3
Re: [PATCH 9/9] exec/address-spaces: Inline legacy functions
Posted by Philippe Mathieu-Daudé 3 years, 4 months ago 
On 20/9/22 01:17, Bernhard Beschow wrote:
> The functions just access a global pointer and perform some pointer
> arithmetic on top. Allow the compiler to see through this by inlining.

I thought about this while reviewing the previous patch, ...

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   include/exec/address-spaces.h | 30 ++++++++++++++++++++++++++----
>   softmmu/physmem.c             | 28 ----------------------------
>   2 files changed, 26 insertions(+), 32 deletions(-)
> 
> diff --git a/include/exec/address-spaces.h b/include/exec/address-spaces.h
> index b31bd8dcf0..182af27cad 100644
> --- a/include/exec/address-spaces.h
> +++ b/include/exec/address-spaces.h
> @@ -23,29 +23,51 @@
>   
>   #ifndef CONFIG_USER_ONLY
>   
> +#include "hw/boards.h"

... but I'm not a fan of including this header here. It is restricted to 
system emulation, but still... Let see what the others think.

>   /**
>    * Get the root memory region.  This is a legacy function, provided for
>    * compatibility. Prefer using SysBusState::system_memory directly.
>    */
> -MemoryRegion *get_system_memory(void);
> +inline MemoryRegion *get_system_memory(void)
> +{
> +    assert(current_machine);
> +
> +    return &current_machine->main_system_bus.system_memory;
> +}
>   
>   /**
>    * 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);
> +inline MemoryRegion *get_system_io(void)
> +{
> +    assert(current_machine);
> +
> +    return &current_machine->main_system_bus.system_io;
> +}
>   
>   /**
>    * 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);
> +inline AddressSpace *get_address_space_memory(void)
> +{
> +    assert(current_machine);
> +
> +    return &current_machine->main_system_bus.address_space_memory;
> +}
>   
>   /**
>    * 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);
> +inline AddressSpace *get_address_space_io(void)
> +{
> +    assert(current_machine);
> +
> +    return &current_machine->main_system_bus.address_space_io;
> +}
>   
>   #endif
>   
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index 07e9a9171c..dce088f55c 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -2674,34 +2674,6 @@ static void memory_map_init(SysBusState *sysbus)
>       address_space_init(&sysbus->address_space_io, system_io, "I/O");
>   }
>   
> -MemoryRegion *get_system_memory(void)
> -{
> -    assert(current_machine);
> -
> -    return &current_machine->main_system_bus.system_memory;
> -}
> -
> -MemoryRegion *get_system_io(void)
> -{
> -    assert(current_machine);
> -
> -    return &current_machine->main_system_bus.system_io;
> -}
> -
> -AddressSpace *get_address_space_memory(void)
> -{
> -    assert(current_machine);
> -
> -    return &current_machine->main_system_bus.address_space_memory;
> -}
> -
> -AddressSpace *get_address_space_io(void)
> -{
> -    assert(current_machine);
> -
> -    return &current_machine->main_system_bus.address_space_io;
> -}
> -
>   static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
>                                        hwaddr length)
>   {
Re: [PATCH 9/9] exec/address-spaces: Inline legacy functions
Posted by BALATON Zoltan 3 years, 4 months ago 

On Tue, 20 Sep 2022, Philippe Mathieu-Daudé via wrote:

> On 20/9/22 01:17, Bernhard Beschow wrote:
>> The functions just access a global pointer and perform some pointer
>> arithmetic on top. Allow the compiler to see through this by inlining.
>
> I thought about this while reviewing the previous patch, ...
>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   include/exec/address-spaces.h | 30 ++++++++++++++++++++++++++----
>>   softmmu/physmem.c             | 28 ----------------------------
>>   2 files changed, 26 insertions(+), 32 deletions(-)
>> 
>> diff --git a/include/exec/address-spaces.h b/include/exec/address-spaces.h
>> index b31bd8dcf0..182af27cad 100644
>> --- a/include/exec/address-spaces.h
>> +++ b/include/exec/address-spaces.h
>> @@ -23,29 +23,51 @@
>>     #ifndef CONFIG_USER_ONLY
>>   +#include "hw/boards.h"
>
> ... but I'm not a fan of including this header here. It is restricted to 
> system emulation, but still... Let see what the others think.

Had the same thought first if this would break user emulation but I don't 
know how that works (and this include is withing !CONFIG_USER_ONLY). I've 
checked in configure now and it seems that softmmu is enabled/disabled 
with system, which reminded me to a previous conversation where I've 
suggested renaming softmmu to sysemu as that better shows what it's really 
used for and maybe the real softmmu part should be split from it but I 
don't remember the details. If it still works with --enable-user 
--disable-system then maybe it's OK and only confusing because of 
misnaming sysemu as softmmu.

Reagrds,
BALATON Zoltan

>>   /**
>>    * Get the root memory region.  This is a legacy function, provided for
>>    * compatibility. Prefer using SysBusState::system_memory directly.
>>    */
>> -MemoryRegion *get_system_memory(void);
>> +inline MemoryRegion *get_system_memory(void)
>> +{
>> +    assert(current_machine);
>> +
>> +    return &current_machine->main_system_bus.system_memory;
>> +}
>>     /**
>>    * 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);
>> +inline MemoryRegion *get_system_io(void)
>> +{
>> +    assert(current_machine);
>> +
>> +    return &current_machine->main_system_bus.system_io;
>> +}
>>     /**
>>    * 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);
>> +inline AddressSpace *get_address_space_memory(void)
>> +{
>> +    assert(current_machine);
>> +
>> +    return &current_machine->main_system_bus.address_space_memory;
>> +}
>>     /**
>>    * 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);
>> +inline AddressSpace *get_address_space_io(void)
>> +{
>> +    assert(current_machine);
>> +
>> +    return &current_machine->main_system_bus.address_space_io;
>> +}
>>     #endif
>>   diff --git a/softmmu/physmem.c b/softmmu/physmem.c
>> index 07e9a9171c..dce088f55c 100644
>> --- a/softmmu/physmem.c
>> +++ b/softmmu/physmem.c
>> @@ -2674,34 +2674,6 @@ static void memory_map_init(SysBusState *sysbus)
>>       address_space_init(&sysbus->address_space_io, system_io, "I/O");
>>   }
>>   -MemoryRegion *get_system_memory(void)
>> -{
>> -    assert(current_machine);
>> -
>> -    return &current_machine->main_system_bus.system_memory;
>> -}
>> -
>> -MemoryRegion *get_system_io(void)
>> -{
>> -    assert(current_machine);
>> -
>> -    return &current_machine->main_system_bus.system_io;
>> -}
>> -
>> -AddressSpace *get_address_space_memory(void)
>> -{
>> -    assert(current_machine);
>> -
>> -    return &current_machine->main_system_bus.address_space_memory;
>> -}
>> -
>> -AddressSpace *get_address_space_io(void)
>> -{
>> -    assert(current_machine);
>> -
>> -    return &current_machine->main_system_bus.address_space_io;
>> -}
>> -
>>   static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
>>                                        hwaddr length)
>>   {
>
>
>
Re: [PATCH 9/9] exec/address-spaces: Inline legacy functions
Posted by Bernhard Beschow 3 years, 4 months ago 
Am 20. September 2022 09:02:41 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:
>>> The functions just access a global pointer and perform some pointer
>>> arithmetic on top. Allow the compiler to see through this by inlining.
>> 
>> I thought about this while reviewing the previous patch, ...
>> 
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>>   include/exec/address-spaces.h | 30 ++++++++++++++++++++++++++----
>>>   softmmu/physmem.c             | 28 ----------------------------
>>>   2 files changed, 26 insertions(+), 32 deletions(-)
>>> 
>>> diff --git a/include/exec/address-spaces.h b/include/exec/address-spaces.h
>>> index b31bd8dcf0..182af27cad 100644
>>> --- a/include/exec/address-spaces.h
>>> +++ b/include/exec/address-spaces.h
>>> @@ -23,29 +23,51 @@
>>>     #ifndef CONFIG_USER_ONLY
>>>   +#include "hw/boards.h"
>> 
>> ... but I'm not a fan of including this header here. It is restricted to system emulation, but still... Let see what the others think.
>
>Had the same thought first if this would break user emulation but I don't know how that works (and this include is withing !CONFIG_USER_ONLY). I've checked in configure now and it seems that softmmu is enabled/disabled with system, which reminded me to a previous conversation where I've suggested renaming softmmu to sysemu as that better shows what it's really used for and maybe the real softmmu part should be split from it but I don't remember the details. If it still works with --enable-user --disable-system then maybe it's OK and only confusing because of misnaming sysemu as softmmu.

I've compiled all architectures w/o any --{enable,disable}-{user,system} flags and I had compile errors only when putting the include outside the guard. So this in particular doesn't seem to be a problem.

Best regards,
Bernhard
>
>Reagrds,
>BALATON Zoltan
>
>>>   /**
>>>    * Get the root memory region.  This is a legacy function, provided for
>>>    * compatibility. Prefer using SysBusState::system_memory directly.
>>>    */
>>> -MemoryRegion *get_system_memory(void);
>>> +inline MemoryRegion *get_system_memory(void)
>>> +{
>>> +    assert(current_machine);
>>> +
>>> +    return &current_machine->main_system_bus.system_memory;
>>> +}
>>>     /**
>>>    * 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);
>>> +inline MemoryRegion *get_system_io(void)
>>> +{
>>> +    assert(current_machine);
>>> +
>>> +    return &current_machine->main_system_bus.system_io;
>>> +}
>>>     /**
>>>    * 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);
>>> +inline AddressSpace *get_address_space_memory(void)
>>> +{
>>> +    assert(current_machine);
>>> +
>>> +    return &current_machine->main_system_bus.address_space_memory;
>>> +}
>>>     /**
>>>    * 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);
>>> +inline AddressSpace *get_address_space_io(void)
>>> +{
>>> +    assert(current_machine);
>>> +
>>> +    return &current_machine->main_system_bus.address_space_io;
>>> +}
>>>     #endif
>>>   diff --git a/softmmu/physmem.c b/softmmu/physmem.c
>>> index 07e9a9171c..dce088f55c 100644
>>> --- a/softmmu/physmem.c
>>> +++ b/softmmu/physmem.c
>>> @@ -2674,34 +2674,6 @@ static void memory_map_init(SysBusState *sysbus)
>>>       address_space_init(&sysbus->address_space_io, system_io, "I/O");
>>>   }
>>>   -MemoryRegion *get_system_memory(void)
>>> -{
>>> -    assert(current_machine);
>>> -
>>> -    return &current_machine->main_system_bus.system_memory;
>>> -}
>>> -
>>> -MemoryRegion *get_system_io(void)
>>> -{
>>> -    assert(current_machine);
>>> -
>>> -    return &current_machine->main_system_bus.system_io;
>>> -}
>>> -
>>> -AddressSpace *get_address_space_memory(void)
>>> -{
>>> -    assert(current_machine);
>>> -
>>> -    return &current_machine->main_system_bus.address_space_memory;
>>> -}
>>> -
>>> -AddressSpace *get_address_space_io(void)
>>> -{
>>> -    assert(current_machine);
>>> -
>>> -    return &current_machine->main_system_bus.address_space_io;
>>> -}
>>> -
>>>   static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
>>>                                        hwaddr length)
>>>   {
>> 
>> 
>>
Re: [PATCH 9/9] exec/address-spaces: Inline legacy functions
Posted by Philippe Mathieu-Daudé 3 years, 4 months ago 
On 20/9/22 07:15, Philippe Mathieu-Daudé wrote:
> On 20/9/22 01:17, Bernhard Beschow wrote:
>> The functions just access a global pointer and perform some pointer
>> arithmetic on top. Allow the compiler to see through this by inlining.
> 
> I thought about this while reviewing the previous patch, ...
> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   include/exec/address-spaces.h | 30 ++++++++++++++++++++++++++----
>>   softmmu/physmem.c             | 28 ----------------------------
>>   2 files changed, 26 insertions(+), 32 deletions(-)
>>
>> diff --git a/include/exec/address-spaces.h 
>> b/include/exec/address-spaces.h
>> index b31bd8dcf0..182af27cad 100644
>> --- a/include/exec/address-spaces.h
>> +++ b/include/exec/address-spaces.h
>> @@ -23,29 +23,51 @@
>>   #ifndef CONFIG_USER_ONLY
>> +#include "hw/boards.h"
> 
> ... but I'm not a fan of including this header here. It is restricted to 
> system emulation, but still... Let see what the others think.
> 
>>   /**
>>    * Get the root memory region.  This is a legacy function, provided for
>>    * compatibility. Prefer using SysBusState::system_memory directly.
>>    */
>> -MemoryRegion *get_system_memory(void);
>> +inline MemoryRegion *get_system_memory(void)
>> +{
>> +    assert(current_machine);
>> +
>> +    return &current_machine->main_system_bus.system_memory;
>> +}

Maybe we can simply declare them with __attribute__ ((const)) in the 
previous patch?
See 
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.