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

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

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