Re: [PATCH] target/arm: Provide always-false kvm_arm_*_supported() stubs for usermode

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 7/14/25 8:31 AM, Pierrick Bouvier wrote:
> On 7/14/25 6:51 AM, Peter Maydell wrote:
>> If you try to build aarch64-linux-user with clang and --enable-debug then it
>> fails to compile:
>>
>>    ld: libqemu-aarch64-linux-user.a.p/target_arm_cpu64.c.o: in function `cpu_arm_set_sve':
>>    ../../target/arm/cpu64.c:321:(.text+0x1254): undefined reference to `kvm_arm_sve_supported'
>>
>> This is a regression introduced in commit f86d4220, which switched
>> the kvm-stub.c file away from being built for all arm targets to only
>> being built for system emulation binaries.  It doesn't affect gcc,
>> presumably because even at -O0 gcc folds away the always-false
>> kvm_enabled() condition but clang does not.
>>
>> We would prefer not to build kvm-stub.c once for usermode and once
>> for system-emulation binaries, and we can't build it just once for
>> both because it includes cpu.h.  So instead provide always-false
>> versions of the five functions that are valid to call without KVM
>> support in kvm_arm.h.
>>
>> Fixes: f86d42205c2eba ("target/arm/meson: accelerator files are not needed in user mode")
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3033
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> I'm never sure when we prefer to use stub-functions in separate C files
>> vs when we prefer to have ifdeffed stubs in headers. There are several
>> ways we could fix this compile error, so I just picked one...
>> ---
>>    target/arm/kvm_arm.h | 35 +++++++++++++++++++++++++++++++++++
>>    1 file changed, 35 insertions(+)
>>
> 
> Thanks Peter, clang with --enable-debug is indeed a combination I didn't
> try. I'll test this too now. Going through this topic, yes I noticed
> that gcc always folds the any if (0) condition, and, based on a Richard
> comment (I can't find now) it seems that we used to rely on that for
> other parts of the code.
> 
> The fix you propose works well (initial goal was just to remove
> CONFIG_KVM, so having CONFIG_USER_ONLY is ok), but I wonder if there is
> something specific affecting clang in this case and preventing the folding.
> 

Indeed, clang does not fold the condition "value && kvm_enabled() && 
!kvm_arm_sve_supported()". Looks like a missing case.
This code compiles with gcc -O0, but not clang -O0.

extern int f(void);
int main(int argc) {
     if (argc && 0)
         f();
}

As folding is not guaranteed by C standard, I'm not sure it's really 
possible to file a bug. However, since we rely on this behaviour in 
other parts, maybe it would be better to rewrite the condition on our side.

By changing the code to this, the folding happens as expected.

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 26cf7e6dfa2..af5788dafab 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -318,9 +318,11 @@ static void cpu_arm_set_sve(Object *obj, bool 
value, Error **errp)
  {
      ARMCPU *cpu = ARM_CPU(obj);

-    if (value && kvm_enabled() && !kvm_arm_sve_supported()) {
-        error_setg(errp, "'sve' feature not supported by KVM on this 
host");
-        return;
+    if (value) {
+        if (kvm_enabled() && !kvm_arm_sve_supported()) {
+            error_setg(errp, "'sve' feature not supported by KVM on 
this host");
+            return;
+        }
      }

      FIELD_DP64_IDREG(&cpu->isar, ID_AA64PFR0, SVE, value);

If you prefer keeping your patch, I'm ok, but fixing the condition looks 
better to me (as we already rely on constant folding in other places).

> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> 
> Thanks,
> Pierrick