Before this change booting a -cpu max,sve=off would trigger and
assert:

  qemu-system-aarch64: ../../target/arm/helper.c:6647: sve_vqm1_for_el_sm: Assertion `sm' failed.

when the guest attempts to write to SMCR which shouldn't even exist if
SVE has been turned off.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 target/arm/cpu64.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 0e021960fb..a38d43421a 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -409,6 +409,13 @@ static void cpu_arm_set_sve(Object *obj, bool value, Error **errp)
     t = cpu->isar.id_aa64pfr0;
     t = FIELD_DP64(t, ID_AA64PFR0, SVE, value);
     cpu->isar.id_aa64pfr0 = t;
+
+    /* FEAT_SME requires SVE, so disable it if no SVE */
+    if (!value) {
+        t = cpu->isar.id_aa64pfr1;
+        t = FIELD_DP64(t, ID_AA64PFR1, SME, 0);
+        cpu->isar.id_aa64pfr1 = t;
+    }
 }
 
 void arm_cpu_sme_finalize(ARMCPU *cpu, Error **errp)
-- 
2.39.1

Alex Bennée <alex.bennee@linaro.org> writes:

> Before this change booting a -cpu max,sve=off would trigger and
> assert:
>
>   qemu-system-aarch64: ../../target/arm/helper.c:6647: sve_vqm1_for_el_sm: Assertion `sm' failed.
>
> when the guest attempts to write to SMCR which shouldn't even exist if
> SVE has been turned off.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  target/arm/cpu64.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 0e021960fb..a38d43421a 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -409,6 +409,13 @@ static void cpu_arm_set_sve(Object *obj, bool value, Error **errp)
>      t = cpu->isar.id_aa64pfr0;
>      t = FIELD_DP64(t, ID_AA64PFR0, SVE, value);
>      cpu->isar.id_aa64pfr0 = t;
> +
> +    /* FEAT_SME requires SVE, so disable it if no SVE */
> +    if (!value) {
> +        t = cpu->isar.id_aa64pfr1;
> +        t = FIELD_DP64(t, ID_AA64PFR1, SME, 0);
> +        cpu->isar.id_aa64pfr1 = t;
> +    }

What about -cpu max,sve=off,sme=on ?

Fabiano Rosas <farosas@suse.de> writes:

> Alex Bennée <alex.bennee@linaro.org> writes:
>
>> Before this change booting a -cpu max,sve=off would trigger and
>> assert:
>>
>>   qemu-system-aarch64: ../../target/arm/helper.c:6647: sve_vqm1_for_el_sm: Assertion `sm' failed.
>>
>> when the guest attempts to write to SMCR which shouldn't even exist if
>> SVE has been turned off.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>> ---
>>  target/arm/cpu64.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
>> index 0e021960fb..a38d43421a 100644
>> --- a/target/arm/cpu64.c
>> +++ b/target/arm/cpu64.c
>> @@ -409,6 +409,13 @@ static void cpu_arm_set_sve(Object *obj, bool value, Error **errp)
>>      t = cpu->isar.id_aa64pfr0;
>>      t = FIELD_DP64(t, ID_AA64PFR0, SVE, value);
>>      cpu->isar.id_aa64pfr0 = t;
>> +
>> +    /* FEAT_SME requires SVE, so disable it if no SVE */
>> +    if (!value) {
>> +        t = cpu->isar.id_aa64pfr1;
>> +        t = FIELD_DP64(t, ID_AA64PFR1, SME, 0);
>> +        cpu->isar.id_aa64pfr1 = t;
>> +    }
>
> What about -cpu max,sve=off,sme=on ?

Gah - I bet this is going to depend on ordering of parameters as well.

Markus,

Is there any way to represent optionA implies optionB in our argument parsing?

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Alex Bennée <alex.bennee@linaro.org> writes:

> Fabiano Rosas <farosas@suse.de> writes:
>
>> Alex Bennée <alex.bennee@linaro.org> writes:
>>
>>> Before this change booting a -cpu max,sve=off would trigger and
>>> assert:
>>>
>>>   qemu-system-aarch64: ../../target/arm/helper.c:6647: sve_vqm1_for_el_sm: Assertion `sm' failed.
>>>
>>> when the guest attempts to write to SMCR which shouldn't even exist if
>>> SVE has been turned off.
>>>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>> ---
>>>  target/arm/cpu64.c | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
>>> index 0e021960fb..a38d43421a 100644
>>> --- a/target/arm/cpu64.c
>>> +++ b/target/arm/cpu64.c
>>> @@ -409,6 +409,13 @@ static void cpu_arm_set_sve(Object *obj, bool value, Error **errp)
>>>      t = cpu->isar.id_aa64pfr0;
>>>      t = FIELD_DP64(t, ID_AA64PFR0, SVE, value);
>>>      cpu->isar.id_aa64pfr0 = t;
>>> +
>>> +    /* FEAT_SME requires SVE, so disable it if no SVE */
>>> +    if (!value) {
>>> +        t = cpu->isar.id_aa64pfr1;
>>> +        t = FIELD_DP64(t, ID_AA64PFR1, SME, 0);
>>> +        cpu->isar.id_aa64pfr1 = t;
>>> +    }
>>
>> What about -cpu max,sve=off,sme=on ?
>
> Gah - I bet this is going to depend on ordering of parameters as well.
>
> Markus,
>
> Is there any way to represent optionA implies optionB in our argument parsing?

You meant "in the one of our multitude of ways to parse arguments that
is being used here".

The commit message implicates -cpu.  Which is its own special case.
qemu_init() passes the option argument to parse_cpu_option(), which
splits it at the first ",", interprets the first part as CPU model name,
and passes the second part to the CPU type's ->parse_features()
callback.  Three implementations, all bespoke parsers[*].  ARM CPUs
appear to use cpu_common_parse_features().  As far as I can tell, it
parses the string as a sequence of CPU properties PROP=VAL,... and sets
the properties.

cpu_arm_set_sve() is the setter for property "sve".  Checking the value
of another property in such a setter is usually wrong, as Fabiano
pointed out for this case.  Check in the realize() method instead.

Questions?


[*] Yes, having in the order of twenty ad hoc option argument parsers is
an embarrassment, but it's what a decade or so of unsystematic interface
growth gets you.

On Fri, 10 Feb 2023 at 14:07, Markus Armbruster <armbru@redhat.com> wrote:
> cpu_arm_set_sve() is the setter for property "sve".  Checking the value
> of another property in such a setter is usually wrong, as Fabiano
> pointed out for this case.  Check in the realize() method instead.

Yep. Compare what we do with the "must have both VFP and Neon or neither"
check on those two properties, for instance.

thanks
-- PMM