| Message ID | 20230203134433.31513-4-cohuck@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | arm: enable MTE for QEMU + kvm | expand |
Hi, On 2/3/23 14:44, Cornelia Huck wrote: > Acked-by: Thomas Huth <thuth@redhat.com> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Signed-off-by: Cornelia Huck <cohuck@redhat.com> Still as you need to respin I think adding a short commit msg wouldn't hurt ;-) Add new cpu MTE feature tests with TCG+virt tag memory and TCG-no tag memory (default) attempting to set cpu mte option on/off. No real test for KVM because ../.. > --- > tests/qtest/arm-cpu-features.c | 75 ++++++++++++++++++++++++++++++++++ > 1 file changed, 75 insertions(+) > > diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c > index 8691802950ca..c5dbf66e938a 100644 > --- a/tests/qtest/arm-cpu-features.c > +++ b/tests/qtest/arm-cpu-features.c > @@ -22,6 +22,7 @@ > > #define MACHINE "-machine virt,gic-version=max -accel tcg " > #define MACHINE_KVM "-machine virt,gic-version=max -accel kvm -accel tcg " > +#define MACHINE_MTE "-machine virt,gic-version=max,mte=on -accel tcg " > #define QUERY_HEAD "{ 'execute': 'query-cpu-model-expansion', " \ > " 'arguments': { 'type': 'full', " > #define QUERY_TAIL "}}" > @@ -156,6 +157,18 @@ static bool resp_get_feature(QDict *resp, const char *feature) > g_assert(qdict_get_bool(_props, feature) == (expected_value)); \ > }) > > +#define resp_assert_feature_str(resp, feature, expected_value) \ > +({ \ > + QDict *_props; \ > + \ > + g_assert(_resp); \ > + g_assert(resp_has_props(_resp)); \ > + _props = resp_get_props(_resp); \ > + g_assert(qdict_get(_props, feature)); \ > + g_assert_cmpstr(qdict_get_try_str(_props, feature), ==, \ > + expected_value); \ > +}) > + > #define assert_feature(qts, cpu_type, feature, expected_value) \ > ({ \ > QDict *_resp; \ > @@ -166,6 +179,16 @@ static bool resp_get_feature(QDict *resp, const char *feature) > qobject_unref(_resp); \ > }) > > +#define assert_feature_str(qts, cpu_type, feature, expected_value) \ > +({ \ > + QDict *_resp; \ > + \ > + _resp = do_query_no_props(qts, cpu_type); \ > + g_assert(_resp); \ > + resp_assert_feature_str(_resp, feature, expected_value); \ > + qobject_unref(_resp); \ > +}) > + > #define assert_set_feature(qts, cpu_type, feature, value) \ > ({ \ > const char *_fmt = (value) ? "{ %s: true }" : "{ %s: false }"; \ > @@ -177,6 +200,17 @@ static bool resp_get_feature(QDict *resp, const char *feature) > qobject_unref(_resp); \ > }) > Not really related to your series but those macros become increasingly difficult to follow. Especially the feature param versus format that are partly redundant look weird: "mte", "off", "{ 'mte': 'off' } Starting adding comments may help the reading. > +#define assert_set_feature_str(qts, cpu_type, feature, value, _fmt) \ > +({ \ > + const char *__fmt = _fmt; \ > + QDict *_resp; \ > + \ > + _resp = do_query(qts, cpu_type, __fmt, feature); \ > + g_assert(_resp); \ > + resp_assert_feature_str(_resp, feature, value); \ > + qobject_unref(_resp); \ > +}) > + > #define assert_has_feature_enabled(qts, cpu_type, feature) \ > assert_feature(qts, cpu_type, feature, true) > > @@ -413,6 +447,24 @@ static void sve_tests_sve_off_kvm(const void *data) > qtest_quit(qts); > } > > +static void mte_tests_tag_memory_on(const void *data) > +{ > + QTestState *qts; > + > + qts = qtest_init(MACHINE_MTE "-cpu max"); > + > + /* > + * With tag memory, "mte" should default to on, and explicitly specifying > + * either on or off should be fine. > + */ the above comment rather applies to assert_set_feature_str's, right? > + assert_has_feature(qts, "max", "mte"); > + > + assert_set_feature_str(qts, "max", "mte", "off", "{ 'mte': 'off' }"); > + assert_set_feature_str(qts, "max", "mte", "on", "{ 'mte': 'on' }"); > + > + qtest_quit(qts); > +} > + > static void pauth_tests_default(QTestState *qts, const char *cpu_type) > { > assert_has_feature_enabled(qts, cpu_type, "pauth"); > @@ -425,6 +477,19 @@ static void pauth_tests_default(QTestState *qts, const char *cpu_type) > "{ 'pauth': false, 'pauth-impdef': true }"); > } > > +static void mte_tests_default(QTestState *qts, const char *cpu_type) > +{ > + assert_has_feature(qts, cpu_type, "mte"); > + > + /* > + * Without tag memory, mte will be off under tcg. > + * Explicitly enabling it yields an error. > + */ > + assert_set_feature_str(qts, "max", "mte", "off", "{ 'mte': 'off' }"); > + assert_error(qts, cpu_type, "mte=on requires tag memory", > + "{ 'mte': 'on' }"); Sorry in v4 I reported I preferred the pauth msg, clarifying now: assert_error(qts, cpu_type, "cannot enable pauth-impdef without pauth", "{ 'pauth': false, 'pauth-impdef': true }"); Here would translate into cannot enable mte without tag memory. > +} > + > static void test_query_cpu_model_expansion(const void *data) > { > QTestState *qts; > @@ -474,6 +539,7 @@ static void test_query_cpu_model_expansion(const void *data) > > sve_tests_default(qts, "max"); > pauth_tests_default(qts, "max"); > + mte_tests_default(qts, "max"); > > /* Test that features that depend on KVM generate errors without. */ > assert_error(qts, "max", > @@ -517,6 +583,13 @@ static void test_query_cpu_model_expansion_kvm(const void *data) > assert_set_feature(qts, "host", "pmu", false); > assert_set_feature(qts, "host", "pmu", true); > > + /* > + * Unfortunately, there's no easy way to test whether this instance > + * of KVM supports MTE. So we can only assert that the feature > + * is present, but not whether it can be toggled. > + */ > + assert_has_feature(qts, "host", "mte"); I know you replied in v4 but I am still confused: What does (QEMU) query-cpu-model-expansion type=full model={"name":"host"} return on a MTE capable host and and on a non MTE capable host? If I remember correctly qmp_query_cpu_model_expansion loops over the advertised features and try to set them explicitly so if the host does not support it this should fail and the result should be different from the case where the host supports it (even if it is off by default) Does assert_has_feature_enabled() returns false? Thanks Eric > + > /* > * Some features would be enabled by default, but they're disabled > * because this instance of KVM doesn't support them. Test that the > @@ -631,6 +704,8 @@ int main(int argc, char **argv) > NULL, sve_tests_sve_off); > qtest_add_data_func("/arm/kvm/query-cpu-model-expansion/sve-off", > NULL, sve_tests_sve_off_kvm); > + qtest_add_data_func("/arm/max/query-cpu-model-expansion/tag-memory", > + NULL, mte_tests_tag_memory_on); > } > > return g_test_run();
On Mon, Feb 06 2023, Eric Auger <eauger@redhat.com> wrote: > Hi, > > On 2/3/23 14:44, Cornelia Huck wrote: >> Acked-by: Thomas Huth <thuth@redhat.com> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> Signed-off-by: Cornelia Huck <cohuck@redhat.com> > > Still as you need to respin I think adding a short commit msg wouldn't > hurt ;-) Add new cpu MTE feature tests with TCG+virt tag memory and > TCG-no tag memory (default) attempting to set cpu mte option on/off. No > real test for KVM because ../.. Ok, I'll add some lines :) >> --- >> tests/qtest/arm-cpu-features.c | 75 ++++++++++++++++++++++++++++++++++ >> 1 file changed, 75 insertions(+) (...) >> +static void mte_tests_default(QTestState *qts, const char *cpu_type) >> +{ >> + assert_has_feature(qts, cpu_type, "mte"); >> + >> + /* >> + * Without tag memory, mte will be off under tcg. >> + * Explicitly enabling it yields an error. >> + */ >> + assert_set_feature_str(qts, "max", "mte", "off", "{ 'mte': 'off' }"); >> + assert_error(qts, cpu_type, "mte=on requires tag memory", >> + "{ 'mte': 'on' }"); > Sorry in v4 I reported I preferred the pauth msg, clarifying now: > > assert_error(qts, cpu_type, "cannot enable pauth-impdef without pauth", > "{ 'pauth': false, 'pauth-impdef': true }"); > > Here would translate into cannot enable mte without tag memory. Oh, so you mean that I should adapt the message generated by the code? [did not get around to the rest of it this week, will try again next week]
On Mon, Feb 06 2023, Eric Auger <eauger@redhat.com> wrote: > Hi, > > On 2/3/23 14:44, Cornelia Huck wrote: >> @@ -517,6 +583,13 @@ static void test_query_cpu_model_expansion_kvm(const void *data) >> assert_set_feature(qts, "host", "pmu", false); >> assert_set_feature(qts, "host", "pmu", true); >> >> + /* >> + * Unfortunately, there's no easy way to test whether this instance >> + * of KVM supports MTE. So we can only assert that the feature >> + * is present, but not whether it can be toggled. >> + */ >> + assert_has_feature(qts, "host", "mte"); > I know you replied in v4 but I am still confused: > What does > (QEMU) query-cpu-model-expansion type=full model={"name":"host"} > return on a MTE capable host and and on a non MTE capable host? FWIW, it's "auto" in both cases, but the main problem is actually something else... > > If I remember correctly qmp_query_cpu_model_expansion loops over the > advertised features and try to set them explicitly so if the host does > not support it this should fail and the result should be different from > the case where the host supports it (even if it is off by default) > > Does assert_has_feature_enabled() returns false? I poked around a bit with qmp on a system (well, model) with MTE where starting a guest with MTE works just fine. I used the minimal setup described in docs/devel/writing-monitor-commands.rst, and trying to do a cpu model expansion with mte=on fails because the KVM ioctl fails with -EINVAL (as we haven't set up proper memory mappings). The qtest setup doesn't do any proper setup either AFAICS, so enabling MTE won't work even if KVM and the host support it. (Trying to enable MTE on a host that doesn't support it would also report an error, but a different one, as KVM would not support the MTE cap at all.) We don't really know beforehand what to expect ("auto" is not yet expanded, see above), so I'm not sure how to test this in a meaningful way, even if we did set up memory mappings (which seems like overkill for a feature test.) The comment describing this could be improved, though :)
Hi Connie, On 2/15/23 11:59, Cornelia Huck wrote: > On Mon, Feb 06 2023, Eric Auger <eauger@redhat.com> wrote: > >> Hi, >> >> On 2/3/23 14:44, Cornelia Huck wrote: >>> @@ -517,6 +583,13 @@ static void test_query_cpu_model_expansion_kvm(const void *data) >>> assert_set_feature(qts, "host", "pmu", false); >>> assert_set_feature(qts, "host", "pmu", true); >>> >>> + /* >>> + * Unfortunately, there's no easy way to test whether this instance >>> + * of KVM supports MTE. So we can only assert that the feature >>> + * is present, but not whether it can be toggled. >>> + */ >>> + assert_has_feature(qts, "host", "mte"); >> I know you replied in v4 but I am still confused: >> What does >> (QEMU) query-cpu-model-expansion type=full model={"name":"host"} >> return on a MTE capable host and and on a non MTE capable host? > > FWIW, it's "auto" in both cases, but the main problem is actually > something else... > >> >> If I remember correctly qmp_query_cpu_model_expansion loops over the >> advertised features and try to set them explicitly so if the host does >> not support it this should fail and the result should be different from >> the case where the host supports it (even if it is off by default) >> >> Does assert_has_feature_enabled() returns false? > > I poked around a bit with qmp on a system (well, model) with MTE where > starting a guest with MTE works just fine. I used the minimal setup > described in docs/devel/writing-monitor-commands.rst, and trying to do a > cpu model expansion with mte=on fails because the KVM ioctl fails with > -EINVAL (as we haven't set up proper memory mappings). The qtest setup > doesn't do any proper setup either AFAICS, so enabling MTE won't work > even if KVM and the host support it. (Trying to enable MTE on a host > that doesn't support it would also report an error, but a different one, > as KVM would not support the MTE cap at all.) We don't really know > beforehand what to expect ("auto" is not yet expanded, see above), so > I'm not sure how to test this in a meaningful way, even if we did set up > memory mappings (which seems like overkill for a feature test.) > > The comment describing this could be improved, though :) > OK fair enough, don't make it a blocking issue for the series and simply update the comment up to your knowledge. Thanks Eric
On Fri, Feb 10 2023, Cornelia Huck <cohuck@redhat.com> wrote: > On Mon, Feb 06 2023, Eric Auger <eauger@redhat.com> wrote: > >> Hi, >> >> On 2/3/23 14:44, Cornelia Huck wrote: >>> +static void mte_tests_default(QTestState *qts, const char *cpu_type) >>> +{ >>> + assert_has_feature(qts, cpu_type, "mte"); >>> + >>> + /* >>> + * Without tag memory, mte will be off under tcg. >>> + * Explicitly enabling it yields an error. >>> + */ >>> + assert_set_feature_str(qts, "max", "mte", "off", "{ 'mte': 'off' }"); >>> + assert_error(qts, cpu_type, "mte=on requires tag memory", >>> + "{ 'mte': 'on' }"); >> Sorry in v4 I reported I preferred the pauth msg, clarifying now: >> >> assert_error(qts, cpu_type, "cannot enable pauth-impdef without pauth", >> "{ 'pauth': false, 'pauth-impdef': true }"); >> >> Here would translate into cannot enable mte without tag memory. > > Oh, so you mean that I should adapt the message generated by the code? Friendly ping :) Did you mean to adapt the error messages in cpu64.c?
diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c index 8691802950ca..c5dbf66e938a 100644 --- a/tests/qtest/arm-cpu-features.c +++ b/tests/qtest/arm-cpu-features.c @@ -22,6 +22,7 @@ #define MACHINE "-machine virt,gic-version=max -accel tcg " #define MACHINE_KVM "-machine virt,gic-version=max -accel kvm -accel tcg " +#define MACHINE_MTE "-machine virt,gic-version=max,mte=on -accel tcg " #define QUERY_HEAD "{ 'execute': 'query-cpu-model-expansion', " \ " 'arguments': { 'type': 'full', " #define QUERY_TAIL "}}" @@ -156,6 +157,18 @@ static bool resp_get_feature(QDict *resp, const char *feature) g_assert(qdict_get_bool(_props, feature) == (expected_value)); \ }) +#define resp_assert_feature_str(resp, feature, expected_value) \ +({ \ + QDict *_props; \ + \ + g_assert(_resp); \ + g_assert(resp_has_props(_resp)); \ + _props = resp_get_props(_resp); \ + g_assert(qdict_get(_props, feature)); \ + g_assert_cmpstr(qdict_get_try_str(_props, feature), ==, \ + expected_value); \ +}) + #define assert_feature(qts, cpu_type, feature, expected_value) \ ({ \ QDict *_resp; \ @@ -166,6 +179,16 @@ static bool resp_get_feature(QDict *resp, const char *feature) qobject_unref(_resp); \ }) +#define assert_feature_str(qts, cpu_type, feature, expected_value) \ +({ \ + QDict *_resp; \ + \ + _resp = do_query_no_props(qts, cpu_type); \ + g_assert(_resp); \ + resp_assert_feature_str(_resp, feature, expected_value); \ + qobject_unref(_resp); \ +}) + #define assert_set_feature(qts, cpu_type, feature, value) \ ({ \ const char *_fmt = (value) ? "{ %s: true }" : "{ %s: false }"; \ @@ -177,6 +200,17 @@ static bool resp_get_feature(QDict *resp, const char *feature) qobject_unref(_resp); \ }) +#define assert_set_feature_str(qts, cpu_type, feature, value, _fmt) \ +({ \ + const char *__fmt = _fmt; \ + QDict *_resp; \ + \ + _resp = do_query(qts, cpu_type, __fmt, feature); \ + g_assert(_resp); \ + resp_assert_feature_str(_resp, feature, value); \ + qobject_unref(_resp); \ +}) + #define assert_has_feature_enabled(qts, cpu_type, feature) \ assert_feature(qts, cpu_type, feature, true) @@ -413,6 +447,24 @@ static void sve_tests_sve_off_kvm(const void *data) qtest_quit(qts); } +static void mte_tests_tag_memory_on(const void *data) +{ + QTestState *qts; + + qts = qtest_init(MACHINE_MTE "-cpu max"); + + /* + * With tag memory, "mte" should default to on, and explicitly specifying + * either on or off should be fine. + */ + assert_has_feature(qts, "max", "mte"); + + assert_set_feature_str(qts, "max", "mte", "off", "{ 'mte': 'off' }"); + assert_set_feature_str(qts, "max", "mte", "on", "{ 'mte': 'on' }"); + + qtest_quit(qts); +} + static void pauth_tests_default(QTestState *qts, const char *cpu_type) { assert_has_feature_enabled(qts, cpu_type, "pauth"); @@ -425,6 +477,19 @@ static void pauth_tests_default(QTestState *qts, const char *cpu_type) "{ 'pauth': false, 'pauth-impdef': true }"); } +static void mte_tests_default(QTestState *qts, const char *cpu_type) +{ + assert_has_feature(qts, cpu_type, "mte"); + + /* + * Without tag memory, mte will be off under tcg. + * Explicitly enabling it yields an error. + */ + assert_set_feature_str(qts, "max", "mte", "off", "{ 'mte': 'off' }"); + assert_error(qts, cpu_type, "mte=on requires tag memory", + "{ 'mte': 'on' }"); +} + static void test_query_cpu_model_expansion(const void *data) { QTestState *qts; @@ -474,6 +539,7 @@ static void test_query_cpu_model_expansion(const void *data) sve_tests_default(qts, "max"); pauth_tests_default(qts, "max"); + mte_tests_default(qts, "max"); /* Test that features that depend on KVM generate errors without. */ assert_error(qts, "max", @@ -517,6 +583,13 @@ static void test_query_cpu_model_expansion_kvm(const void *data) assert_set_feature(qts, "host", "pmu", false); assert_set_feature(qts, "host", "pmu", true); + /* + * Unfortunately, there's no easy way to test whether this instance + * of KVM supports MTE. So we can only assert that the feature + * is present, but not whether it can be toggled. + */ + assert_has_feature(qts, "host", "mte"); + /* * Some features would be enabled by default, but they're disabled * because this instance of KVM doesn't support them. Test that the @@ -631,6 +704,8 @@ int main(int argc, char **argv) NULL, sve_tests_sve_off); qtest_add_data_func("/arm/kvm/query-cpu-model-expansion/sve-off", NULL, sve_tests_sve_off_kvm); + qtest_add_data_func("/arm/max/query-cpu-model-expansion/tag-memory", + NULL, mte_tests_tag_memory_on); } return g_test_run();