qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] target/arm: Provide always-false kvm_arm_*_supported() stubs for usermode
@ 2025-07-14 13:51 Peter Maydell
  2025-07-14 15:31 ` Pierrick Bouvier
  0 siblings, 1 reply; 11+ messages in thread
From: Peter Maydell @ 2025-07-14 13:51 UTC (permalink / raw)
  To: qemu-arm, qemu-devel; +Cc: Richard Henderson, Pierrick Bouvier

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

diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
index b4cad051551..6a9b6374a6d 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -161,6 +161,14 @@ void kvm_arm_add_vcpu_properties(ARMCPU *cpu);
  */
 void kvm_arm_steal_time_finalize(ARMCPU *cpu, Error **errp);
 
+/*
+ * These "is some KVM subfeature enabled?" functions may be called
+ * when KVM support is not present, including in the user-mode
+ * emulators. The kvm-stub.c file is only built into the system
+ * emulators, so for user-mode emulation we provide "always false"
+ * stubs here.
+ */
+#ifndef CONFIG_USER_ONLY
 /**
  * kvm_arm_aarch32_supported:
  *
@@ -197,6 +205,33 @@ bool kvm_arm_mte_supported(void);
  * Returns true if KVM can enable EL2 and false otherwise.
  */
 bool kvm_arm_el2_supported(void);
+#else
+
+static inline bool kvm_arm_aarch32_supported(void)
+{
+    return false;
+}
+
+static inline bool kvm_arm_pmu_supported(void)
+{
+    return false;
+}
+
+static inline bool kvm_arm_sve_supported(void)
+{
+    return false;
+}
+
+static inline bool kvm_arm_mte_supported(void)
+{
+    return false;
+}
+
+static inline bool kvm_arm_el2_supported(void)
+{
+    return false;
+}
+#endif
 
 /**
  * kvm_arm_get_max_vm_ipa_size:
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] target/arm: Provide always-false kvm_arm_*_supported() stubs for usermode
  2025-07-14 13:51 [PATCH] target/arm: Provide always-false kvm_arm_*_supported() stubs for usermode Peter Maydell
@ 2025-07-14 15:31 ` Pierrick Bouvier
  2025-07-14 15:41   ` Pierrick Bouvier
  0 siblings, 1 reply; 11+ messages in thread
From: Pierrick Bouvier @ 2025-07-14 15:31 UTC (permalink / raw)
  To: Peter Maydell, qemu-arm, qemu-devel; +Cc: Richard Henderson

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.

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

Thanks,
Pierrick


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] target/arm: Provide always-false kvm_arm_*_supported() stubs for usermode
  2025-07-14 15:31 ` Pierrick Bouvier
@ 2025-07-14 15:41   ` Pierrick Bouvier
  2025-07-14 15:52     ` Richard Henderson
  2025-07-17 16:56     ` Peter Maydell
  0 siblings, 2 replies; 11+ messages in thread
From: Pierrick Bouvier @ 2025-07-14 15:41 UTC (permalink / raw)
  To: Peter Maydell, qemu-arm, qemu-devel; +Cc: Richard Henderson

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



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] target/arm: Provide always-false kvm_arm_*_supported() stubs for usermode
  2025-07-14 15:41   ` Pierrick Bouvier
@ 2025-07-14 15:52     ` Richard Henderson
  2025-07-14 17:38       ` Pierrick Bouvier
  2025-07-14 19:49       ` Philippe Mathieu-Daudé
  2025-07-17 16:56     ` Peter Maydell
  1 sibling, 2 replies; 11+ messages in thread
From: Richard Henderson @ 2025-07-14 15:52 UTC (permalink / raw)
  To: Pierrick Bouvier, Peter Maydell, qemu-arm, qemu-devel

On 7/14/25 09:41, Pierrick Bouvier wrote:
> 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.

It's probably worth filing a missed-optimization type bug, if that's available in clang's 
reporting system.

With my compiler hat on, I suspect that GCC generates IR like

   if (argc) {
     if (0) {
       f();
     }
   }

in order to get the short-circuting part of && correct, which Just So Happens to fold away 
exactly as we wish.

I'm not sure how clang expands the expression such that (x && 0) doesn't fold away, but (0 
&& x) does, as evidenced by

> +        if (kvm_enabled() && !kvm_arm_sve_supported()) { 


r~


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] target/arm: Provide always-false kvm_arm_*_supported() stubs for usermode
  2025-07-14 15:52     ` Richard Henderson
@ 2025-07-14 17:38       ` Pierrick Bouvier
  2025-07-15 20:43         ` Pierrick Bouvier
  2025-07-14 19:49       ` Philippe Mathieu-Daudé
  1 sibling, 1 reply; 11+ messages in thread
From: Pierrick Bouvier @ 2025-07-14 17:38 UTC (permalink / raw)
  To: Richard Henderson, Peter Maydell, qemu-arm, qemu-devel

On 7/14/25 8:52 AM, Richard Henderson wrote:
> On 7/14/25 09:41, Pierrick Bouvier wrote:
>> 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.
> 
> It's probably worth filing a missed-optimization type bug, if that's available in clang's
> reporting system.
>

Sure, I'll post a bug report on clang repository.

> With my compiler hat on, I suspect that GCC generates IR like
> 
>     if (argc) {
>       if (0) {
>         f();
>       }
>     }
> 
> in order to get the short-circuting part of && correct, which Just So Happens to fold away
> exactly as we wish.
> 
> I'm not sure how clang expands the expression such that (x && 0) doesn't fold away, but (0
> && x) does, as evidenced by
>

For gcc, the simple GIMPLE tree is identical for both.

int main (int argc)
{
   int D.2775;

   {
     if (0 != 0) goto <D.2773>; else goto <D.2774>;
     <D.2773>:
     f ();
     <D.2774>:
   }
   D.2775 = 0;
   return D.2775;
}

This is the LLVM IR difference between "(0 && argc)" and "(argc && 0)".

  ; Function Attrs: noinline nounwind optnone uwtable
  define dso_local i32 @main(i32 noundef %0) #0 {
    %2 = alloca i32, align 4
-  %3 = alloca i32, align 4
-  store i32 0, ptr %2, align 4
-  store i32 %0, ptr %3, align 4
-  %4 = load i32, ptr %3, align 4
-  %5 = icmp ne i32 %4, 0
-  br i1 %5, label %6, label %9
-
-6:                                                ; preds = %1
-  br i1 false, label %7, label %9
-
-7:                                                ; preds = %6
-  %8 = call i32 @f()
-  br label %9
-
-9:                                                ; preds = %7, %6, %1
-  %10 = load i32, ptr %2, align 4
-  ret i32 %10
+  store i32 %0, ptr %2, align 4
+  ret i32 0
  }

-declare i32 @f() #1
-

We can see it elides completely the if as expected, given the right 
order for expression.
On a side note, the first operand still needs to be evaluated if it has 
a side effect (especially a function call), before ensuring the shortcut 
properly applies.

>> +        if (kvm_enabled() && !kvm_arm_sve_supported()) {
> 
> 
> r~



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] target/arm: Provide always-false kvm_arm_*_supported() stubs for usermode
  2025-07-14 15:52     ` Richard Henderson
  2025-07-14 17:38       ` Pierrick Bouvier
@ 2025-07-14 19:49       ` Philippe Mathieu-Daudé
  1 sibling, 0 replies; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-07-14 19:49 UTC (permalink / raw)
  To: Richard Henderson, Pierrick Bouvier, Peter Maydell, qemu-arm,
	qemu-devel

On 14/7/25 17:52, Richard Henderson wrote:
> On 7/14/25 09:41, Pierrick Bouvier wrote:
>> 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.
> 
> It's probably worth filing a missed-optimization type bug, if that's 
> available in clang's reporting system.
> 
> With my compiler hat on, I suspect that GCC generates IR like
> 
>    if (argc) {
>      if (0) {
>        f();
>      }
>    }
> 
> in order to get the short-circuting part of && correct, which Just So 
> Happens to fold away exactly as we wish.
> 
> I'm not sure how clang expands the expression such that (x && 0) doesn't 
> fold away, but (0 && x) does, as evidenced by
> 
>> +        if (kvm_enabled() && !kvm_arm_sve_supported()) { 

I'd prefer move the kvm_enabled() call instead of re-introducing the stubs:

-- >8 --
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 26cf7e6dfa2..37c7cc7b18e 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 (kvm_enabled()) {
+        if (value && !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);
---


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] target/arm: Provide always-false kvm_arm_*_supported() stubs for usermode
  2025-07-14 17:38       ` Pierrick Bouvier
@ 2025-07-15 20:43         ` Pierrick Bouvier
  0 siblings, 0 replies; 11+ messages in thread
From: Pierrick Bouvier @ 2025-07-15 20:43 UTC (permalink / raw)
  To: Richard Henderson, Peter Maydell, qemu-arm, qemu-devel

On 7/14/25 10:38 AM, Pierrick Bouvier wrote:
> On 7/14/25 8:52 AM, Richard Henderson wrote:
>> On 7/14/25 09:41, Pierrick Bouvier wrote:
>>> 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.
>>
>> It's probably worth filing a missed-optimization type bug, if that's available in clang's
>> reporting system.
>>
> 
> Sure, I'll post a bug report on clang repository.
> 
>> With my compiler hat on, I suspect that GCC generates IR like
>>
>>      if (argc) {
>>        if (0) {
>>          f();
>>        }
>>      }
>>
>> in order to get the short-circuting part of && correct, which Just So Happens to fold away
>> exactly as we wish.
>>
>> I'm not sure how clang expands the expression such that (x && 0) doesn't fold away, but (0
>> && x) does, as evidenced by
>>
> 
> For gcc, the simple GIMPLE tree is identical for both.
> 
> int main (int argc)
> {
>     int D.2775;
> 
>     {
>       if (0 != 0) goto <D.2773>; else goto <D.2774>;
>       <D.2773>:
>       f ();
>       <D.2774>:
>     }
>     D.2775 = 0;
>     return D.2775;
> }
> 
> This is the LLVM IR difference between "(0 && argc)" and "(argc && 0)".
> 
>    ; Function Attrs: noinline nounwind optnone uwtable
>    define dso_local i32 @main(i32 noundef %0) #0 {
>      %2 = alloca i32, align 4
> -  %3 = alloca i32, align 4
> -  store i32 0, ptr %2, align 4
> -  store i32 %0, ptr %3, align 4
> -  %4 = load i32, ptr %3, align 4
> -  %5 = icmp ne i32 %4, 0
> -  br i1 %5, label %6, label %9
> -
> -6:                                                ; preds = %1
> -  br i1 false, label %7, label %9
> -
> -7:                                                ; preds = %6
> -  %8 = call i32 @f()
> -  br label %9
> -
> -9:                                                ; preds = %7, %6, %1
> -  %10 = load i32, ptr %2, align 4
> -  ret i32 %10
> +  store i32 %0, ptr %2, align 4
> +  ret i32 0
>    }
> 
> -declare i32 @f() #1
> -
> 
> We can see it elides completely the if as expected, given the right
> order for expression.
> On a side note, the first operand still needs to be evaluated if it has
> a side effect (especially a function call), before ensuring the shortcut
> properly applies.
>

For reference, this is the associated bug report:
https://github.com/llvm/llvm-project/issues/148955

>>> +        if (kvm_enabled() && !kvm_arm_sve_supported()) {
>>
>>
>> r~
> 



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] target/arm: Provide always-false kvm_arm_*_supported() stubs for usermode
  2025-07-14 15:41   ` Pierrick Bouvier
  2025-07-14 15:52     ` Richard Henderson
@ 2025-07-17 16:56     ` Peter Maydell
  2025-07-17 17:05       ` Pierrick Bouvier
  1 sibling, 1 reply; 11+ messages in thread
From: Peter Maydell @ 2025-07-17 16:56 UTC (permalink / raw)
  To: Pierrick Bouvier; +Cc: qemu-arm, qemu-devel, Richard Henderson

On Mon, 14 Jul 2025 at 16:41, Pierrick Bouvier
<pierrick.bouvier@linaro.org> wrote:
> 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).

I'm not really a fan of relying on the compiler to fold stuff
away -- it's fragile and there's no guarantee the compiler
will actually do it. In this example it would be really easy
for somebody coming along to tidy this up later to put the
nested if()s back into a single if() condition and reintroduce
the problem, for instance.

I've applied this patch to target-arm.next; thanks for the review.

-- PMM


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] target/arm: Provide always-false kvm_arm_*_supported() stubs for usermode
  2025-07-17 16:56     ` Peter Maydell
@ 2025-07-17 17:05       ` Pierrick Bouvier
  2025-07-17 17:12         ` Peter Maydell
  0 siblings, 1 reply; 11+ messages in thread
From: Pierrick Bouvier @ 2025-07-17 17:05 UTC (permalink / raw)
  To: Peter Maydell; +Cc: qemu-arm, qemu-devel, Richard Henderson

On 7/17/25 9:56 AM, Peter Maydell wrote:
> On Mon, 14 Jul 2025 at 16:41, Pierrick Bouvier
> <pierrick.bouvier@linaro.org> wrote:
>> 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).
> 
> I'm not really a fan of relying on the compiler to fold stuff
> away -- it's fragile and there's no guarantee the compiler
> will actually do it. In this example it would be really easy
> for somebody coming along to tidy this up later to put the
> nested if()s back into a single if() condition and reintroduce
> the problem, for instance.
>

There are various places where we already relied on that, including 
before the single-binary work ({accel}_allowed).

For the fragile aspect of it, that's why CI exists. Building all 
binaries with clang --enable-debug should ensure no regression can be 
introduced.

> I've applied this patch to target-arm.next; thanks for the review.
>

Thank you Peter.

> -- PMM


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] target/arm: Provide always-false kvm_arm_*_supported() stubs for usermode
  2025-07-17 17:05       ` Pierrick Bouvier
@ 2025-07-17 17:12         ` Peter Maydell
  2025-07-17 17:29           ` Pierrick Bouvier
  0 siblings, 1 reply; 11+ messages in thread
From: Peter Maydell @ 2025-07-17 17:12 UTC (permalink / raw)
  To: Pierrick Bouvier; +Cc: qemu-arm, qemu-devel, Richard Henderson

On Thu, 17 Jul 2025 at 18:05, Pierrick Bouvier
<pierrick.bouvier@linaro.org> wrote:
> On 7/17/25 9:56 AM, Peter Maydell wrote:
> > I'm not really a fan of relying on the compiler to fold stuff
> > away -- it's fragile and there's no guarantee the compiler
> > will actually do it. In this example it would be really easy
> > for somebody coming along to tidy this up later to put the
> > nested if()s back into a single if() condition and reintroduce
> > the problem, for instance.
> >
>
> There are various places where we already relied on that, including
> before the single-binary work ({accel}_allowed).
>
> For the fragile aspect of it, that's why CI exists. Building all
> binaries with clang --enable-debug should ensure no regression can be
> introduced.

But our CI can't check all versions of gcc and clang at
all optimisation levels. The reason it's fragile is because
there's never a guarantee that the compiler does it or what
situations it will work vs not work. It's not a feature
provided by the language or the compiler that you can fold
away references to functions that don't exist; it just
happens to work.

I know we already rely on constant-folding in various
places: I'm not super enthusiastic about those either.

-- PMM


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] target/arm: Provide always-false kvm_arm_*_supported() stubs for usermode
  2025-07-17 17:12         ` Peter Maydell
@ 2025-07-17 17:29           ` Pierrick Bouvier
  0 siblings, 0 replies; 11+ messages in thread
From: Pierrick Bouvier @ 2025-07-17 17:29 UTC (permalink / raw)
  To: Peter Maydell; +Cc: qemu-arm, qemu-devel, Richard Henderson

On 7/17/25 10:12 AM, Peter Maydell wrote:
> On Thu, 17 Jul 2025 at 18:05, Pierrick Bouvier
> <pierrick.bouvier@linaro.org> wrote:
>> On 7/17/25 9:56 AM, Peter Maydell wrote:
>>> I'm not really a fan of relying on the compiler to fold stuff
>>> away -- it's fragile and there's no guarantee the compiler
>>> will actually do it. In this example it would be really easy
>>> for somebody coming along to tidy this up later to put the
>>> nested if()s back into a single if() condition and reintroduce
>>> the problem, for instance.
>>>
>>
>> There are various places where we already relied on that, including
>> before the single-binary work ({accel}_allowed).
>>
>> For the fragile aspect of it, that's why CI exists. Building all
>> binaries with clang --enable-debug should ensure no regression can be
>> introduced.
> 
> But our CI can't check all versions of gcc and clang at
> all optimisation levels. The reason it's fragile is because
> there's never a guarantee that the compiler does it or what
> situations it will work vs not work. It's not a feature
> provided by the language or the compiler that you can fold
> away references to functions that don't exist; it just
> happens to work.
>

Sure, it's always a compromise, and it's overkill to try to cover all 
the {os, compiler, version} possibilities.

That said, checking debug builds with available clang version in latest 
ubuntu LTS + latest clang version published should already cover a lot 
of cases, including new warnings that might appear.

I would be happy to help contribute on QEMU CI side, but in my personal 
experience trying to approach concerned developers, either I didn't have 
any answer, or received a "No, our minutes are precious".
I know your point of view (Engineers are already paid, minutes are not), 
but it still doesn't make sense to me. I hope one day we can move away 
from this paradigm, and really make a good usage of cpu time instead of 
people time for testing things.

> I know we already rely on constant-folding in various
> places: I'm not super enthusiastic about those either.
> 
> -- PMM



^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2025-07-17 20:14 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-14 13:51 [PATCH] target/arm: Provide always-false kvm_arm_*_supported() stubs for usermode Peter Maydell
2025-07-14 15:31 ` Pierrick Bouvier
2025-07-14 15:41   ` Pierrick Bouvier
2025-07-14 15:52     ` Richard Henderson
2025-07-14 17:38       ` Pierrick Bouvier
2025-07-15 20:43         ` Pierrick Bouvier
2025-07-14 19:49       ` Philippe Mathieu-Daudé
2025-07-17 16:56     ` Peter Maydell
2025-07-17 17:05       ` Pierrick Bouvier
2025-07-17 17:12         ` Peter Maydell
2025-07-17 17:29           ` Pierrick Bouvier

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).