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
