| Message ID | 20230203100551.2445547-1-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] target/arm: disable FEAT_SME if we turn off SVE | expand |
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 <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
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)
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(+)