Re: [PATCH v1.1 5/5] tests: Introduce a TSX test

[Jan Beulich <jbeulich@suse.com>]

On 14.06.2021 12:47, Andrew Cooper wrote:
> --- /dev/null
> +++ b/tools/tests/tsx/Makefile
> @@ -0,0 +1,43 @@
> +XEN_ROOT = $(CURDIR)/../../..
> +include $(XEN_ROOT)/tools/Rules.mk
> +
> +TARGET := test-tsx
> +
> +.PHONY: all
> +all: $(TARGET)
> +
> +.PHONY: run
> +run: $(TARGET)
> +	./$(TARGET)
> +
> +.PHONY: clean
> +clean:
> +	$(RM) -f -- *.o $(TARGET) $(DEPS_RM)

I'm surprised this is necessary, but indeed I can see it elsewhere too.

> +.PHONY: distclean
> +distclean: clean
> +	$(RM) -f -- *~
> +
> +.PHONY: install
> +install: all
> +
> +.PHONY: uninstall
> +uninstall:
> +
> +CFLAGS += -Werror -std=gnu11

Is this strictly necessary? It excludes a fair share of the gcc
versions that we claim the tree can be built with. If it is
necessary, then I think it needs arranging for that the tools/
build as a whole won't fail just because of this test not
building. We do something along these lines for the x86 emulator
harness, for example.

> +CFLAGS += $(CFLAGS_xeninclude)
> +CFLAGS += $(CFLAGS_libxenctrl)
> +CFLAGS += $(CFLAGS_libxenguest)
> +CFLAGS += -I$(XEN_ROOT)/tools/libs/ctrl -I$(XEN_ROOT)/tools/libs/guest
> +CFLAGS += $(APPEND_CFLAGS)
> +
> +LDFLAGS += $(LDLIBS_libxenctrl)
> +LDFLAGS += $(LDLIBS_libxenguest)
> +LDFLAGS += $(APPEND_LDFLAGS)
> +
> +test-tsx.o: Makefile
> +
> +test-tsx: test-tsx.o

Wouldn't you want to use $(TARGET) at least here?

> +/*
> + * Test a specific TSX MSR for consistency across the system, taking into
> + * account whether it ought to be accessable or not.
> + *
> + * We can't query offline CPUs, so skip those if encountered.  We don't care
> + * particularly for the exact MSR value, but we do care that it is the same
> + * everywhere.
> + */
> +static void test_tsx_msr_consistency(unsigned int msr, bool accessable)

Isn't it "accessible"?

> +{
> +    uint64_t cpu0_val = ~0;
> +
> +    for ( unsigned int cpu = 0; cpu <= physinfo.max_cpu_id; ++cpu )
> +    {
> +        xc_resource_entry_t ent = {
> +            .u.cmd = XEN_RESOURCE_OP_MSR_READ,
> +            .idx = msr,
> +        };
> +        xc_resource_op_t op = {
> +            .cpu = cpu,
> +            .entries = &ent,
> +            .nr_entries = 1,
> +        };
> +        int rc = xc_resource_op(xch, 1, &op);
> +
> +        if ( rc < 0 )
> +        {
> +            /* Don't emit a message for offline CPUs */
> +            if ( errno != ENODEV )
> +                fail("  xc_resource_op() for CPU%u failed: rc %d, errno %d - %s\n",
> +                     cpu, rc, errno, strerror(errno));
> +            continue;
> +        }
> +
> +        if ( accessable )
> +        {
> +            if ( rc != 1 )
> +            {
> +                fail("  Expected 1 result, got %u\n", rc);

%d

> +                continue;
> +            }
> +            if ( ent.u.ret != 0 )
> +            {
> +                fail("  Expected ok, got %d\n", ent.u.ret);
> +                continue;
> +            }
> +        }
> +        else
> +        {
> +            if ( rc != 0 )
> +                fail("  Expected 0 results, got %u\n", rc);
> +            else if ( ent.u.ret != -EPERM )
> +                fail("  Expected -EPERM, got %d\n", ent.u.ret);
> +            continue;
> +        }
> +
> +        if ( cpu == 0 )
> +        {
> +            cpu0_val = ent.val;
> +            printf("  CPU0 val %#"PRIx64"\n", cpu0_val);
> +        }
> +        else if ( ent.val != cpu0_val )
> +            fail("  CPU%u val %#"PRIx64" differes from CPU0 %#"PRIx64"\n",

Nit: differs?

> +/*
> + * Probe for how RTM behaves, deliberately not inspecting CPUID.
> + * Distinguishes between "no support at all" (i.e. XBEGIN suffers #UD),
> + * working ok, and appearing to always abort.
> + */
> +static enum rtm_behaviour probe_rtm_behaviour(void)
> +{
> +    for ( int i = 0; i < 1000; ++i )
> +    {
> +        /*
> +         * Opencoding the RTM infrastructure from immintrin.h, because we
> +         * still support older versions of GCC.  ALso so we can include #UD
> +         * detection logic.
> +         */
> +#define XBEGIN_STARTED -1
> +#define XBEGIN_UD      -2
> +        unsigned int status = XBEGIN_STARTED;
> +
> +        asm volatile (".Lxbegin: .byte 0xc7,0xf8,0,0,0,0" /* XBEGIN 1f; 1: */
> +                      : "+a" (status) :: "memory");
> +        if ( status == XBEGIN_STARTED )
> +        {
> +            asm volatile (".byte 0x0f,0x01,0xd5" ::: "memory"); /* XEND */

Nit: This otherwise following hypervisor style, the asm()s want more
blanks added (also again further down).

> +            return RTM_OK;
> +        }
> +        else if ( status == XBEGIN_UD )
> +            return RTM_UD;
> +    }
> +
> +    return RTM_ABORT;
> +}
> +
> +static struct sigaction old_sigill;
> +
> +static void sigill_handler(int signo, siginfo_t *info, void *extra)
> +{
> +    extern char xbegin_label[] asm(".Lxbegin");

Perhaps add const? I'm also not sure about .L names used for extern-s.

> +    if ( info->si_addr == xbegin_label ||
> +         memcmp(info->si_addr, "\xc7\xf8\x00\x00\x00\x00", 6) == 0 )

Why the || here? I could see you use && if you really wanted to be on
the safe side, but the way you have it I don't understand the
intentions.

> +    {
> +        ucontext_t *context = extra;
> +
> +        /*
> +         * Found the XBEGIN instruction.  Step over it, and update `status` to
> +         * signal #UD.
> +         */
> +#ifdef __x86_64__
> +        context->uc_mcontext.gregs[REG_RIP] += 6;
> +        context->uc_mcontext.gregs[REG_RAX] = XBEGIN_UD;
> +#else
> +        context->uc_mcontext.gregs[REG_EIP] += 6;
> +        context->uc_mcontext.gregs[REG_EAX] = XBEGIN_UD;
> +#endif

At the very least for this, don't you need to constrain the test to
just Linux?

> +static void test_tsx(void)
> +{
> +    int rc;
> +
> +    /* Read all policies except raw. */
> +    for ( int i = XEN_SYSCTL_cpu_policy_host;

To avoid having this as bad precedent, even though it's "just" testing
code: unsigned int? (I've first spotted this here, but later I've
found more places elsewhere.)

Jan