Re: [PATCH 02/10] accel/tcg: move cpu_reloading_memory_map into cpu-exec-softmmu

[Claudio Fontana <cfontana@suse.de>]

On 3/20/23 14:32, Alex Bennée wrote:
> 
> Claudio Fontana <cfontana@suse.de> writes:
> 
>> How is this conditional on CONFIG_TCG? To me it looks like this breaks !CONFIG_TCG.
>> Careful, the meson.build in accel/tcg/meson.build is always recursed.
> 
> Surely it shouldn't be in accel/tcg then?


Hi Alex,

maybe we did not understand each other.

What I mean is that accel/tcg/meson.build is not conditionally read, it is _always_ read.

Therefore TCG-specific code needs to be conditionally included using the CONFIG_TCG.

Ciao,

Claudio

> 
>> This code was in tcg_ss before, why not simply add it to tcg_ss and
>> then to specific_ss along with the other tcg pieces?
> 
> tcg_ss is rebuilt for every target. So far the code in cpu-exec-softmmu
> should only need to for softmmu targets and hopefully share the same
> binary for all variants.
> 
> I guess I'll have to do something more in line with the recent
> re-factoring of gdbstub:
> 
>   # We build two versions of gdbstub, one for each mode
>   gdb_user_ss.add(files('gdbstub.c', 'user.c'))
>   gdb_softmmu_ss.add(files('gdbstub.c', 'softmmu.c'))
> 
>   gdb_user_ss = gdb_user_ss.apply(config_host, strict: false)
>   gdb_softmmu_ss = gdb_softmmu_ss.apply(config_host, strict: false)
> 
>   libgdb_user = static_library('gdb_user',
>                                gdb_user_ss.sources() + genh,
>                                name_suffix: 'fa',
>                                c_args: '-DCONFIG_USER_ONLY')
> 
>   libgdb_softmmu = static_library('gdb_softmmu',
>                                   gdb_softmmu_ss.sources() + genh,
>                                   name_suffix: 'fa')
> 
>   gdb_user = declare_dependency(link_whole: libgdb_user)
>   user_ss.add(gdb_user)
>   gdb_softmmu = declare_dependency(link_whole: libgdb_softmmu)
>   softmmu_ss.add(gdb_softmmu)
> 
> 
>>
>> Ciao,
>>
>> C
>>
>>
>> On 3/20/23 11:10, Alex Bennée wrote:
>>> This doesn't save much as cpu-exec-common still needs to be built
>>> per-target for its knowledge of CPUState but this helps with keeping
>>> things organised.
>>>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> ---
>>>  accel/tcg/cpu-exec-common.c  | 30 ----------------------
>>>  accel/tcg/cpu-exec-softmmu.c | 50 ++++++++++++++++++++++++++++++++++++
>>>  accel/tcg/meson.build        | 10 ++++++++
>>>  3 files changed, 60 insertions(+), 30 deletions(-)
>>>  create mode 100644 accel/tcg/cpu-exec-softmmu.c
>>>
>>> diff --git a/accel/tcg/cpu-exec-common.c b/accel/tcg/cpu-exec-common.c
>>> index e7962c9348..c6b0ad303e 100644
>>> --- a/accel/tcg/cpu-exec-common.c
>>> +++ b/accel/tcg/cpu-exec-common.c
>>> @@ -32,36 +32,6 @@ void cpu_loop_exit_noexc(CPUState *cpu)
>>>      cpu_loop_exit(cpu);
>>>  }
>>>  
>>> -#if defined(CONFIG_SOFTMMU)
>>> -void cpu_reloading_memory_map(void)
>>> -{
>>> -    if (qemu_in_vcpu_thread() && current_cpu->running) {
>>> -        /* The guest can in theory prolong the RCU critical section as long
>>> -         * as it feels like. The major problem with this is that because it
>>> -         * can do multiple reconfigurations of the memory map within the
>>> -         * critical section, we could potentially accumulate an unbounded
>>> -         * collection of memory data structures awaiting reclamation.
>>> -         *
>>> -         * Because the only thing we're currently protecting with RCU is the
>>> -         * memory data structures, it's sufficient to break the critical section
>>> -         * in this callback, which we know will get called every time the
>>> -         * memory map is rearranged.
>>> -         *
>>> -         * (If we add anything else in the system that uses RCU to protect
>>> -         * its data structures, we will need to implement some other mechanism
>>> -         * to force TCG CPUs to exit the critical section, at which point this
>>> -         * part of this callback might become unnecessary.)
>>> -         *
>>> -         * This pair matches cpu_exec's rcu_read_lock()/rcu_read_unlock(), which
>>> -         * only protects cpu->as->dispatch. Since we know our caller is about
>>> -         * to reload it, it's safe to split the critical section.
>>> -         */
>>> -        rcu_read_unlock();
>>> -        rcu_read_lock();
>>> -    }
>>> -}
>>> -#endif
>>> -
>>>  void cpu_loop_exit(CPUState *cpu)
>>>  {
>>>      /* Undo the setting in cpu_tb_exec.  */
>>> diff --git a/accel/tcg/cpu-exec-softmmu.c b/accel/tcg/cpu-exec-softmmu.c
>>> new file mode 100644
>>> index 0000000000..2318dd8c7d
>>> --- /dev/null
>>> +++ b/accel/tcg/cpu-exec-softmmu.c
>>> @@ -0,0 +1,50 @@
>>> +/*
>>> + *  Emulator main CPU execution loop, softmmu bits
>>> + *
>>> + *  Copyright (c) 2003-2005 Fabrice Bellard
>>> + *
>>> + * This library is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU Lesser General Public
>>> + * License as published by the Free Software Foundation; either
>>> + * version 2.1 of the License, or (at your option) any later version.
>>> + *
>>> + * This library is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>> + * Lesser General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU Lesser General Public
>>> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "hw/core/cpu.h"
>>> +#include "sysemu/cpus.h"
>>> +
>>> +void cpu_reloading_memory_map(void)
>>> +{
>>> +    if (qemu_in_vcpu_thread() && current_cpu->running) {
>>> +        /* The guest can in theory prolong the RCU critical section as long
>>> +         * as it feels like. The major problem with this is that because it
>>> +         * can do multiple reconfigurations of the memory map within the
>>> +         * critical section, we could potentially accumulate an unbounded
>>> +         * collection of memory data structures awaiting reclamation.
>>> +         *
>>> +         * Because the only thing we're currently protecting with RCU is the
>>> +         * memory data structures, it's sufficient to break the critical section
>>> +         * in this callback, which we know will get called every time the
>>> +         * memory map is rearranged.
>>> +         *
>>> +         * (If we add anything else in the system that uses RCU to protect
>>> +         * its data structures, we will need to implement some other mechanism
>>> +         * to force TCG CPUs to exit the critical section, at which point this
>>> +         * part of this callback might become unnecessary.)
>>> +         *
>>> +         * This pair matches cpu_exec's rcu_read_lock()/rcu_read_unlock(), which
>>> +         * only protects cpu->as->dispatch. Since we know our caller is about
>>> +         * to reload it, it's safe to split the critical section.
>>> +         */
>>> +        rcu_read_unlock();
>>> +        rcu_read_lock();
>>> +    }
>>> +}
>>> diff --git a/accel/tcg/meson.build b/accel/tcg/meson.build
>>> index aeb20a6ef0..bdc086b90d 100644
>>> --- a/accel/tcg/meson.build
>>> +++ b/accel/tcg/meson.build
>>> @@ -1,3 +1,9 @@
>>> +#
>>> +# Currently most things here end up in specific_ss eventually because
>>> +# they need knowledge of CPUState. Stuff that that doesn't can live in
>>> +# common user, softmmu or overall code
>>> +#
>>> +
>>>  tcg_ss = ss.source_set()
>>>  tcg_ss.add(files(
>>>    'tcg-all.c',
>>> @@ -9,6 +15,7 @@ tcg_ss.add(files(
>>>    'translate-all.c',
>>>    'translator.c',
>>>  ))
>>> +
>>>  tcg_ss.add(when: 'CONFIG_USER_ONLY', if_true: files('user-exec.c'))
>>>  tcg_ss.add(when: 'CONFIG_SOFTMMU', if_false: files('user-exec-stub.c'))
>>>  tcg_ss.add(when: 'CONFIG_PLUGIN', if_true: [files('plugin-gen.c')])
>>> @@ -27,3 +34,6 @@ tcg_module_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TCG'], if_true: files(
>>>    'tcg-accel-ops-icount.c',
>>>    'tcg-accel-ops-rr.c',
>>>  ))
>>> +
>>> +# Common softmmu code
>>> +softmmu_ss.add(files('cpu-exec-softmmu.c'))
> 
>