| Message ID | 20210117164813.4101761-5-f4bug@amsat.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | accel: Restrict TCG-specific code | expand |
On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote: > cpu_loop_exit*() functions are declared in accel/tcg/cpu-exec-common.c, > and are not available when TCG accelerator is not built. Add stubs so > linking without TCG succeed. > > Problematic files: > > - hw/semihosting/console.c in qemu_semihosting_console_inc() > - hw/ppc/spapr_hcall.c in h_confer() > - hw/s390x/ipl.c in s390_ipl_reset_request() > - hw/misc/mips_itu.c > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > I suppose the s390x kvm-only build didn't catch this because > of compiler optimization: > > in s390_ipl_reset_request(): > > 640 if (tcg_enabled()) { > 641 cpu_loop_exit(cs); > 642 } Ciao Philippe, yes I have seen this a lot on x86 also, and seems to depend on the compiler used. On OpenSUSE 15.2, with gcc based on 7.5.0, I am getting this optimization also, and so no error happens in these cases. It is a bit inconvenient because I have to rely completely on the CI to catch these situations. Ciao, Claudio > > and "sysemu/tcg.h" is: > > 13 #ifdef CONFIG_TCG > 14 extern bool tcg_allowed; > 15 #define tcg_enabled() (tcg_allowed) > 16 #else > 17 #define tcg_enabled() 0 > 18 #endif > --- > accel/stubs/tcg-stub.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/accel/stubs/tcg-stub.c b/accel/stubs/tcg-stub.c > index 8c18d3eabdd..2304606f8e0 100644 > --- a/accel/stubs/tcg-stub.c > +++ b/accel/stubs/tcg-stub.c > @@ -28,3 +28,13 @@ void *probe_access(CPUArchState *env, target_ulong addr, int size, > /* Handled by hardware accelerator. */ > g_assert_not_reached(); > } > + > +void QEMU_NORETURN cpu_loop_exit(CPUState *cpu) > +{ > + g_assert_not_reached(); > +} > + > +void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc) > +{ > + g_assert_not_reached(); > +} >
On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote: > cpu_loop_exit*() functions are declared in accel/tcg/cpu-exec-common.c, > and are not available when TCG accelerator is not built. Add stubs so > linking without TCG succeed. The reason why stubs are needed here at all seems to be that that the code calling cpu_loop_exit is not refactored properly yet; if we look at the example of i386, after the refactoring moving tcg related code into target/i386/tcg/, (and really even before that I think), the code calling cpu_loop_exit is not built for non-TCG at all, and so we don't need stubs. I am ok with this anyway, just wanted to convey that I think we should look at stubs as a necessary evil until all code stops mixing tcg, kvm and other accels... Thanks, Claudio > > Problematic files: > > - hw/semihosting/console.c in qemu_semihosting_console_inc() > - hw/ppc/spapr_hcall.c in h_confer() > - hw/s390x/ipl.c in s390_ipl_reset_request() > - hw/misc/mips_itu.c > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > I suppose the s390x kvm-only build didn't catch this because > of compiler optimization: > > in s390_ipl_reset_request(): > > 640 if (tcg_enabled()) { > 641 cpu_loop_exit(cs); > 642 } > > and "sysemu/tcg.h" is: > > 13 #ifdef CONFIG_TCG > 14 extern bool tcg_allowed; > 15 #define tcg_enabled() (tcg_allowed) > 16 #else > 17 #define tcg_enabled() 0 > 18 #endif > --- > accel/stubs/tcg-stub.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/accel/stubs/tcg-stub.c b/accel/stubs/tcg-stub.c > index 8c18d3eabdd..2304606f8e0 100644 > --- a/accel/stubs/tcg-stub.c > +++ b/accel/stubs/tcg-stub.c > @@ -28,3 +28,13 @@ void *probe_access(CPUArchState *env, target_ulong addr, int size, > /* Handled by hardware accelerator. */ > g_assert_not_reached(); > } > + > +void QEMU_NORETURN cpu_loop_exit(CPUState *cpu) > +{ > + g_assert_not_reached(); > +} > + > +void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc) > +{ > + g_assert_not_reached(); > +} >
On 1/18/21 10:29 AM, Claudio Fontana wrote: > On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote: >> cpu_loop_exit*() functions are declared in accel/tcg/cpu-exec-common.c, >> and are not available when TCG accelerator is not built. Add stubs so >> linking without TCG succeed. > > The reason why stubs are needed here at all seems to be that that the code > calling cpu_loop_exit is not refactored properly yet; I agree ... > if we look at the example of i386, after the refactoring moving tcg related code into target/i386/tcg/, > (and really even before that I think), > the code calling cpu_loop_exit is not built for non-TCG at all, and so we don't need stubs. > > I am ok with this anyway, just wanted to convey that I think we should look at stubs as a necessary evil until all code stops mixing tcg, kvm and other accels... > > Thanks, > > Claudio > >> >> Problematic files: >> >> - hw/semihosting/console.c in qemu_semihosting_console_inc() >> - hw/ppc/spapr_hcall.c in h_confer() >> - hw/s390x/ipl.c in s390_ipl_reset_request() >> - hw/misc/mips_itu.c ... but I have no clue how to refactore these, as they are used in both KVM and TCG. How would you do? I'm stuck with the semihosting code dependency on ARM since 2 years... Phil.
On 1/18/21 10:39 AM, Philippe Mathieu-Daudé wrote: > On 1/18/21 10:29 AM, Claudio Fontana wrote: >> On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote: >>> cpu_loop_exit*() functions are declared in accel/tcg/cpu-exec-common.c, >>> and are not available when TCG accelerator is not built. Add stubs so >>> linking without TCG succeed. >> >> The reason why stubs are needed here at all seems to be that that the code >> calling cpu_loop_exit is not refactored properly yet; > > I agree ... > >> if we look at the example of i386, after the refactoring moving tcg related code into target/i386/tcg/, >> (and really even before that I think), >> the code calling cpu_loop_exit is not built for non-TCG at all, and so we don't need stubs. >> >> I am ok with this anyway, just wanted to convey that I think we should look at stubs as a necessary evil until all code stops mixing tcg, kvm and other accels... >> >> Thanks, >> >> Claudio >> >>> >>> Problematic files: >>> >>> - hw/semihosting/console.c in qemu_semihosting_console_inc() >>> - hw/ppc/spapr_hcall.c in h_confer() >>> - hw/s390x/ipl.c in s390_ipl_reset_request() >>> - hw/misc/mips_itu.c > > ... but I have no clue how to refactore these, as they > are used in both KVM and TCG. > > How would you do? I'm stuck with the semihosting code > dependency on ARM since 2 years... > > Phil. > Just naively looking at this, qemu_semihosting_console_inc seems called only by do_arm_semihosting in target/arm/arm-semi.c, which in turn is called by linux-user (TCG), target/arm/m_helper.c in arm_v7m_cpu_do_interrupt(), which I would assume is TCG only too, just waiting for the TCG/KVM refactoring in ARM, which I would assume would make cpu_tcg.c TCG-only, target/arm/helper.c in handle_semihosting, which is already wrapped in #ifdef CONFIG_TCG and is commented with: " * We only see semihosting exceptions in TCG only as they are not * trapped to the hypervisor in KVM. */ " So am I wrong in my assumption that as soon as we are able to separate TCG vs KVM in target/arm/ , the issue of hw/semihosting/console.c would be solved? Did not look at the other cases. Ciao! Claudio
On 1/17/21 6:48 AM, Philippe Mathieu-Daudé wrote: > cpu_loop_exit*() functions are declared in accel/tcg/cpu-exec-common.c, > and are not available when TCG accelerator is not built. Add stubs so > linking without TCG succeed. > > Problematic files: > > - hw/semihosting/console.c in qemu_semihosting_console_inc() > - hw/ppc/spapr_hcall.c in h_confer() > - hw/s390x/ipl.c in s390_ipl_reset_request() > - hw/misc/mips_itu.c > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- Queued to tcg-next. r~
Claudio Fontana <cfontana@suse.de> writes: > On 1/18/21 10:39 AM, Philippe Mathieu-Daudé wrote: >> On 1/18/21 10:29 AM, Claudio Fontana wrote: >>> On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote: >>>> cpu_loop_exit*() functions are declared in accel/tcg/cpu-exec-common.c, >>>> and are not available when TCG accelerator is not built. Add stubs so >>>> linking without TCG succeed. >>> >>> The reason why stubs are needed here at all seems to be that that the code >>> calling cpu_loop_exit is not refactored properly yet; >> >> I agree ... >> >>> if we look at the example of i386, after the refactoring moving tcg related code into target/i386/tcg/, >>> (and really even before that I think), >>> the code calling cpu_loop_exit is not built for non-TCG at all, and so we don't need stubs. >>> >>> I am ok with this anyway, just wanted to convey that I think we should look at stubs as a necessary evil until all code stops mixing tcg, kvm and other accels... >>> >>> Thanks, >>> >>> Claudio >>> >>>> >>>> Problematic files: >>>> >>>> - hw/semihosting/console.c in qemu_semihosting_console_inc() >>>> - hw/ppc/spapr_hcall.c in h_confer() >>>> - hw/s390x/ipl.c in s390_ipl_reset_request() >>>> - hw/misc/mips_itu.c >> >> ... but I have no clue how to refactore these, as they >> are used in both KVM and TCG. >> >> How would you do? I'm stuck with the semihosting code >> dependency on ARM since 2 years... >> >> Phil. >> > > Just naively looking at this, qemu_semihosting_console_inc seems called only by > do_arm_semihosting in target/arm/arm-semi.c, > > which in turn is called by linux-user (TCG), > > target/arm/m_helper.c in arm_v7m_cpu_do_interrupt(), > which I would assume is TCG only too, just waiting for the TCG/KVM refactoring in ARM, which I would assume would make cpu_tcg.c TCG-only, > > target/arm/helper.c in handle_semihosting, which is already wrapped in #ifdef CONFIG_TCG and is commented with: > > " > * We only see semihosting exceptions in TCG only as they are not > * trapped to the hypervisor in KVM. > */ > " > > So am I wrong in my assumption that as soon as we are able to separate > TCG vs KVM in target/arm/ , the issue of hw/semihosting/console.c > would be solved? I think it is - certainly for ARM. I don't know if real RiscV HW can trap semihosting calls to the kernel/hypervisor.
diff --git a/accel/stubs/tcg-stub.c b/accel/stubs/tcg-stub.c index 8c18d3eabdd..2304606f8e0 100644 --- a/accel/stubs/tcg-stub.c +++ b/accel/stubs/tcg-stub.c @@ -28,3 +28,13 @@ void *probe_access(CPUArchState *env, target_ulong addr, int size, /* Handled by hardware accelerator. */ g_assert_not_reached(); } + +void QEMU_NORETURN cpu_loop_exit(CPUState *cpu) +{ + g_assert_not_reached(); +} + +void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc) +{ + g_assert_not_reached(); +}
cpu_loop_exit*() functions are declared in accel/tcg/cpu-exec-common.c, and are not available when TCG accelerator is not built. Add stubs so linking without TCG succeed. Problematic files: - hw/semihosting/console.c in qemu_semihosting_console_inc() - hw/ppc/spapr_hcall.c in h_confer() - hw/s390x/ipl.c in s390_ipl_reset_request() - hw/misc/mips_itu.c Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- I suppose the s390x kvm-only build didn't catch this because of compiler optimization: in s390_ipl_reset_request(): 640 if (tcg_enabled()) { 641 cpu_loop_exit(cs); 642 } and "sysemu/tcg.h" is: 13 #ifdef CONFIG_TCG 14 extern bool tcg_allowed; 15 #define tcg_enabled() (tcg_allowed) 16 #else 17 #define tcg_enabled() 0 18 #endif --- accel/stubs/tcg-stub.c | 10 ++++++++++ 1 file changed, 10 insertions(+)