Re: [RFC PATCH] target/arm: disable FEAT_SME if we turn off SVE

[Markus Armbruster]

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.