Re: [PATCH] XTF: tests SPEC_CTRL added bits

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

On Tue, Jan 30, 2024 at 11:42:43AM +0100, Jan Beulich wrote:
> 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.

That's just my place holder for random XTF stuff.  I don't intend this
to be committed.

> > --- /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?

Just wanted a PV and an HVM context.

> > --- /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,

It will ensure a vmexit/trap, which is what I was after here.  Maybe the
comment should be "Trap into Xen." or some such.  It wasn't about
ensuring VM context switching.

Thanks, Roger.