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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 14/06/2021 14:31, Jan Beulich wrote:
> On 14.06.2021 12:47, Andrew Cooper wrote:
>> +.PHONY: distclean
>> +distclean: clean
>> +	$(RM) -f -- *~
>> +
>> +.PHONY: install
>> +install: all
>> +
>> +.PHONY: uninstall
>> +uninstall:
>> +
>> +CFLAGS += -Werror -std=gnu11
> Is this strictly necessary?

Appears not.  Dropped.

>
>> +            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.

Well - they work perfectly fine even with the Clang integrated assembler.

>
>> +    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.

That should have been &&, but I also appear to have lost a noclone
attribute too.

>
>> +    {
>> +        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?

I guess it was too much to hope that this would be compatible across the
BSDs too.

And the FreeBSD CI did notice it, but apparently didn't email me...

I'll try to make it BSD compatible.

>
>> +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.)

Well - I question if it even is "bad" precedent.

For array bounds which are constants, the compiler can (and does) do
better than anything we can write in C here, as it is arch-dependent
whether signed or unsigned is better to use.

Beyond that, it's just code volume.

~Andrew