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(+)

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();
+}
-- 
2.26.2

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

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.

-- 
Alex Bennée

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~