| Message ID | 20210117164813.4101761-4-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | accel: Restrict TCG-specific code | expand |
On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote: > tb_gen_code() is only called within TCG accelerator, > declare it locally. Is this used only in accel/tcg/cpu-exec.c ? Should it be a static function there? Ciao, Claudio > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > accel/tcg/internal.h | 5 +++++ > include/exec/exec-all.h | 5 ----- > accel/tcg/cpu-exec.c | 1 + > accel/tcg/user-exec.c | 1 + > 4 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/accel/tcg/internal.h b/accel/tcg/internal.h > index 4981d98dbfd..f7e18c3498b 100644 > --- a/accel/tcg/internal.h > +++ b/accel/tcg/internal.h > @@ -11,6 +11,11 @@ > > #include "exec/exec-all.h" > > +TranslationBlock *tb_gen_code(CPUState *cpu, > + target_ulong pc, target_ulong cs_base, > + uint32_t flags, > + int cflags); > + > void tb_flush_jmp_cache(CPUState *cpu, target_ulong addr); > > #endif > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h > index 1e3e7cf8e78..3acc7c2943a 100644 > --- a/include/exec/exec-all.h > +++ b/include/exec/exec-all.h > @@ -64,11 +64,6 @@ bool cpu_restore_state(CPUState *cpu, uintptr_t searched_pc, bool will_exit); > > void QEMU_NORETURN cpu_loop_exit_noexc(CPUState *cpu); > void QEMU_NORETURN cpu_io_recompile(CPUState *cpu, uintptr_t retaddr); > -TranslationBlock *tb_gen_code(CPUState *cpu, > - target_ulong pc, target_ulong cs_base, > - uint32_t flags, > - int cflags); > - > void QEMU_NORETURN cpu_loop_exit(CPUState *cpu); > void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc); > void QEMU_NORETURN cpu_loop_exit_atomic(CPUState *cpu, uintptr_t pc); > diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c > index e0df9b6a1dd..43676ae8d13 100644 > --- a/accel/tcg/cpu-exec.c > +++ b/accel/tcg/cpu-exec.c > @@ -41,6 +41,7 @@ > #include "exec/cpu-all.h" > #include "sysemu/cpu-timers.h" > #include "sysemu/replay.h" > +#include "internal.h" > > /* -icount align implementation. */ > > diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c > index 1215b55ca08..05f3c09cbf9 100644 > --- a/accel/tcg/user-exec.c > +++ b/accel/tcg/user-exec.c > @@ -28,6 +28,7 @@ > #include "qemu/atomic128.h" > #include "trace/trace-root.h" > #include "trace/mem.h" > +#include "internal.h" > > #undef EAX > #undef ECX >
On 1/17/21 11:12 PM, Claudio Fontana wrote: > On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote: >> tb_gen_code() is only called within TCG accelerator, >> declare it locally. > > Is this used only in accel/tcg/cpu-exec.c ? Should it be a static function there? Possibly, but there's a *lot* of code that would have to be moved. For now, queuing a slightly modified version of the patch. >> --- a/accel/tcg/user-exec.c >> +++ b/accel/tcg/user-exec.c >> @@ -28,6 +28,7 @@ >> #include "qemu/atomic128.h" >> #include "trace/trace-root.h" >> #include "trace/mem.h" >> +#include "internal.h" Not needed by this patch. r~
On 1/21/21 7:06 AM, Richard Henderson wrote: > On 1/17/21 11:12 PM, Claudio Fontana wrote: >> On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote: >>> tb_gen_code() is only called within TCG accelerator, >>> declare it locally. >> >> Is this used only in accel/tcg/cpu-exec.c ? Should it be a static function there? > > Possibly, but there's a *lot* of code that would have to be moved. For now, > queuing a slightly modified version of the patch. > >>> --- a/accel/tcg/user-exec.c >>> +++ b/accel/tcg/user-exec.c >>> @@ -28,6 +28,7 @@ >>> #include "qemu/atomic128.h" >>> #include "trace/trace-root.h" >>> #include "trace/mem.h" >>> +#include "internal.h" > > Not needed by this patch. > > > r~ > Hello, resurrecting this, and in reference to its commit: "c03f041f128301c6a6c32242846be08719cd4fc3", the name "internal.h" ends up polluting the include paths, so that when working for example on s390x, including "internal.h" ends up including this instead of the file in target/s390x/. I am not sure what exactly the right solution is, for this specific problem, and if we should look at the include paths settings in detail, but in my view calling files just "internal.h" or "internals.h" in general is not a good idea. I can see two issues with this naming: 1) it describes nothing about the actual intended contents, other that they should be "internal". Rather it would be better to know what the file is intended to contain, or we end up (as we end up) with very large files containing completely unrelated content. 2) we end up with clashes in our include paths if we are not super careful. Probably in this case, the target/s390x/internal.h could be given another name (s390x-internal.h) and then split up in the future (there is a whole bunch of unrelated suff). For accel/tcg/internal.h, maybe renaming it, or removing it altogether could both be good options? Thanks, Claudio
On 3/15/21 2:52 PM, Claudio Fontana wrote: > On 1/21/21 7:06 AM, Richard Henderson wrote: >> On 1/17/21 11:12 PM, Claudio Fontana wrote: >>> On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote: >>>> tb_gen_code() is only called within TCG accelerator, >>>> declare it locally. >>> >>> Is this used only in accel/tcg/cpu-exec.c ? Should it be a static function there? >> >> Possibly, but there's a *lot* of code that would have to be moved. For now, >> queuing a slightly modified version of the patch. >> >>>> --- a/accel/tcg/user-exec.c >>>> +++ b/accel/tcg/user-exec.c >>>> @@ -28,6 +28,7 @@ >>>> #include "qemu/atomic128.h" >>>> #include "trace/trace-root.h" >>>> #include "trace/mem.h" >>>> +#include "internal.h" >> >> Not needed by this patch. >> >> >> r~ >> > > Hello, > > resurrecting this, and in reference to its commit: "c03f041f128301c6a6c32242846be08719cd4fc3", > > the name "internal.h" ends up polluting the include paths, > so that when working for example on s390x, including "internal.h" ends up including this instead of the file in target/s390x/. > > I am not sure what exactly the right solution is, for this specific problem, > and if we should look at the include paths settings in detail, > > but in my view calling files just "internal.h" or "internals.h" in general is not a good idea. > > I can see two issues with this naming: > > 1) it describes nothing about the actual intended contents, other that they should be "internal". > Rather it would be better to know what the file is intended to contain, or we end up (as we end up) with very large files containing completely unrelated content. > > 2) we end up with clashes in our include paths if we are not super careful. > > Probably in this case, the target/s390x/internal.h could be given another name (s390x-internal.h) and then split up in the future (there is a whole bunch of unrelated suff). > > For accel/tcg/internal.h, maybe renaming it, or removing it altogether could both be good options? I think something like commit ed5bad39e57 is required, restricting the scope of: add_project_arguments('-iquote', meson.current_source_dir() / 'tcg' / tcg_arch, ... to tcg, and ... '-iquote', meson.current_source_dir() / 'accel/tcg', to accel. language: ['c', 'cpp', 'objc'])
diff --git a/accel/tcg/internal.h b/accel/tcg/internal.h index 4981d98dbfd..f7e18c3498b 100644 --- a/accel/tcg/internal.h +++ b/accel/tcg/internal.h @@ -11,6 +11,11 @@ #include "exec/exec-all.h" +TranslationBlock *tb_gen_code(CPUState *cpu, + target_ulong pc, target_ulong cs_base, + uint32_t flags, + int cflags); + void tb_flush_jmp_cache(CPUState *cpu, target_ulong addr); #endif diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h index 1e3e7cf8e78..3acc7c2943a 100644 --- a/include/exec/exec-all.h +++ b/include/exec/exec-all.h @@ -64,11 +64,6 @@ bool cpu_restore_state(CPUState *cpu, uintptr_t searched_pc, bool will_exit); void QEMU_NORETURN cpu_loop_exit_noexc(CPUState *cpu); void QEMU_NORETURN cpu_io_recompile(CPUState *cpu, uintptr_t retaddr); -TranslationBlock *tb_gen_code(CPUState *cpu, - target_ulong pc, target_ulong cs_base, - uint32_t flags, - int cflags); - void QEMU_NORETURN cpu_loop_exit(CPUState *cpu); void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc); void QEMU_NORETURN cpu_loop_exit_atomic(CPUState *cpu, uintptr_t pc); diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c index e0df9b6a1dd..43676ae8d13 100644 --- a/accel/tcg/cpu-exec.c +++ b/accel/tcg/cpu-exec.c @@ -41,6 +41,7 @@ #include "exec/cpu-all.h" #include "sysemu/cpu-timers.h" #include "sysemu/replay.h" +#include "internal.h" /* -icount align implementation. */ diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c index 1215b55ca08..05f3c09cbf9 100644 --- a/accel/tcg/user-exec.c +++ b/accel/tcg/user-exec.c @@ -28,6 +28,7 @@ #include "qemu/atomic128.h" #include "trace/trace-root.h" #include "trace/mem.h" +#include "internal.h" #undef EAX #undef ECX
tb_gen_code() is only called within TCG accelerator, declare it locally. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- accel/tcg/internal.h | 5 +++++ include/exec/exec-all.h | 5 ----- accel/tcg/cpu-exec.c | 1 + accel/tcg/user-exec.c | 1 + 4 files changed, 7 insertions(+), 5 deletions(-)