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

[BALATON Zoltan]

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)
> >   {