Re: [PATCH RESEND v2 6/6] target/arm: Add vCPU feature 'el2' test.

[Andrew Jones <drjones@redhat.com>]

On Thu, Apr 01, 2021 at 05:55:38AM -0700, Haibo Xu wrote:
> Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> ---
>  target/arm/monitor.c           | 2 +-
>  tests/qtest/arm-cpu-features.c | 9 +++++++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/monitor.c b/target/arm/monitor.c
> index 80c64fa355..6c39238925 100644
> --- a/target/arm/monitor.c
> +++ b/target/arm/monitor.c

The patch summary says we're adding a test, but here we're touching
monitor code. Adding 'el2' to this monitor list should happen in the
patch where el2 is introduced.

> @@ -90,7 +90,7 @@ QEMU_BUILD_BUG_ON(ARM_MAX_VQ > 16);
>   * then the order that considers those dependencies must be used.
>   */
>  static const char *cpu_model_advertised_features[] = {
> -    "aarch64", "pmu", "sve",
> +    "aarch64", "pmu", "sve", "el2",

It doesn't really matter, but I'd rather not add a new feature between
'sve' and 'sve128'. Why not just add it to the front or back of the list?

>      "sve128", "sve256", "sve384", "sve512",
>      "sve640", "sve768", "sve896", "sve1024", "sve1152", "sve1280",
>      "sve1408", "sve1536", "sve1664", "sve1792", "sve1920", "sve2048",
> diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
> index 8252b85bb8..be07bf0c76 100644
> --- a/tests/qtest/arm-cpu-features.c
> +++ b/tests/qtest/arm-cpu-features.c
> @@ -509,6 +509,7 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
>      if (g_str_equal(qtest_get_arch(), "aarch64")) {
>          bool kvm_supports_steal_time;
>          bool kvm_supports_sve;
> +        bool kvm_supports_el2;
>          char max_name[8], name[8];
>          uint32_t max_vq, vq;
>          uint64_t vls;
> @@ -533,10 +534,12 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
>           */
>          assert_has_feature(qts, "host", "kvm-steal-time");
>          assert_has_feature(qts, "host", "sve");
> +        assert_has_feature(qts, "host", "el2");
>  
>          resp = do_query_no_props(qts, "host");
>          kvm_supports_steal_time = resp_get_feature(resp, "kvm-steal-time");
>          kvm_supports_sve = resp_get_feature(resp, "sve");
> +        kvm_supports_el2 = resp_get_feature(resp, "el2");

Isn't this feature disabled by default whether the host supports it or
not? If so, this will always be false. I think the test should

 1) confirm the feature is disabled by default
 2) attempt to enable it without asserting on failure
 3) if it gets enabled, then attempt to disable it, asserting on failure

>          vls = resp_get_sve_vls(resp);
>          qobject_unref(resp);
>  
> @@ -602,11 +605,17 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
>          } else {
>              g_assert(vls == 0);
>          }
> +
> +        if (kvm_supports_el2) {
> +            assert_set_feature(qts, "host", "el2", false);
> +            assert_set_feature(qts, "host", "el2", true);

For the steps outlined above, you can drop this extra re-enabling test.

> +        }
>      } else {
>          assert_has_not_feature(qts, "host", "aarch64");
>          assert_has_not_feature(qts, "host", "pmu");
>          assert_has_not_feature(qts, "host", "sve");
>          assert_has_not_feature(qts, "host", "kvm-steal-time");
> +        assert_has_not_feature(qts, "host", "el2");
>      }
>  
>      qtest_quit(qts);
> -- 
> 2.17.1
> 
>

Thanks,
drew