Re: [PATCH] XTF: tests SPEC_CTRL added bits

From: Jan Beulich <jbeulich@suse.com>
To: Roger Pau Monne <roger.pau@citrix.com>
Cc: xen-devel@lists.xenproject.org
Subject: Re: [PATCH] XTF: tests SPEC_CTRL added bits
Date: Tue, 30 Jan 2024 11:42:43 +0100	[thread overview]
Message-ID: <7f514c59-dbf1-44ea-a589-dbc43f0b4ee4@suse.com> (raw)
In-Reply-To: <20240130102719.51150-1-roger.pau@citrix.com>

On 30.01.2024 11:27, Roger Pau Monne wrote:
> Dummy set/clear tests for additional spec_ctrl bits.
> ---
>  docs/all-tests.dox  |   2 +
>  tests/test/Makefile |   9 ++++
>  tests/test/main.c   | 100 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 111 insertions(+)
>  create mode 100644 tests/test/Makefile
>  create mode 100644 tests/test/main.c

I'm puzzled: Why "test"? That doesn't describe in any way what this test
is about.

> --- /dev/null
> +++ b/tests/test/Makefile
> @@ -0,0 +1,9 @@
> +include $(ROOT)/build/common.mk
> +
> +NAME      := test
> +CATEGORY  := utility
> +TEST-ENVS := hvm32 pv64

Any reason for this limitation?

> --- /dev/null
> +++ b/tests/test/main.c
> @@ -0,0 +1,100 @@
> +/**
> + * @file tests/test/main.c
> + * @ref test-test
> + *
> + * @page test-test test
> + *
> + * @todo Docs for test-test
> + *
> + * @see tests/test/main.c
> + */
> +#include <xtf.h>
> +
> +#define MSR_SPEC_CTRL                       0x00000048
> +#define  SPEC_CTRL_IPRED_DIS_U              (_AC(1, ULL) <<  3)
> +#define  SPEC_CTRL_IPRED_DIS_S              (_AC(1, ULL) <<  4)
> +#define  SPEC_CTRL_RRSBA_DIS_U              (_AC(1, ULL) <<  5)
> +#define  SPEC_CTRL_RRSBA_DIS_S              (_AC(1, ULL) <<  6)
> +#define  SPEC_CTRL_DDP_DIS_U                (_AC(1, ULL) <<  8)
> +#define  SPEC_CTRL_BHI_DIS_S                (_AC(1, ULL) << 10)
> +
> +const char test_title[] = "SPEC_CTRL";
> +
> +static void update_spec_ctrl(uint64_t mask, bool set)
> +{
> +    uint64_t spec_ctrl = rdmsr(MSR_SPEC_CTRL);
> +
> +    if ( set )
> +        spec_ctrl |= mask;
> +    else
> +        spec_ctrl &= ~mask;
> +
> +    wrmsr(MSR_SPEC_CTRL, spec_ctrl);
> +}
> +
> +static void assert_spec_ctrl(uint64_t mask, bool set)
> +{
> +    uint64_t spec_ctrl = rdmsr(MSR_SPEC_CTRL);
> +
> +    if ( (spec_ctrl & mask) != (set ? mask : 0) )
> +    {
> +        xtf_failure("SPEC_CTRL expected: %#" PRIx64 " got: %#" PRIx64 "\n",
> +                    set ? (spec_ctrl | mask) : (spec_ctrl & ~mask),
> +                    spec_ctrl);
> +        xtf_exit();
> +    }
> +}
> +
> +static void test_loop(uint64_t mask)
> +{
> +    update_spec_ctrl(mask, true);
> +    assert_spec_ctrl(mask, true);
> +    /* Ensure context switch to Xen. */
> +    hypercall_yield();

I'm afraid yielding doesn't guarantee context switching in Xen, if the
system (or even just the one CPU) is otherwise idle. Hence at the very
least please don't say "ensure" in the comment. But perhaps more
reliable to e.g. use "poll" with a timeout. While I didn't post that
addition, I've used such for testing my vCPU-area-registration work:

        struct sched_poll poll = { .timeout = s + SECONDS(1) };
        rc = hypercall_sched_op(SCHEDOP_poll, &poll);
        if ( rc )
            xtf_error("Could not poll (%d)\n", rc);

(there also to ensure enough time passes for the time area to be
updated).

I actually found this to have another neat side effect: The guest then
can't go away so quickly that "xl console" doesn't manage to attach to
the guest (which otherwise I observe to work only about every other
time).

Jan