Re: [Devel] [PATCH hci-8.1 v4 1/5] selftests/ve: check CPUID override emulation for VE tasks

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] selftests/ve: stopped-child sync does not guarantee a context switch before SIGCONT
- [Low] selftests/ve: x86 gate admits 32-bit builds but do_cpuid() asm needs x86-64
--

commit 7b92cfc5a3a156b03d1e440c0f30bc0e6c9462a6
Author: Konstantin Khorenko <khorenko@virtuozzo.com>

selftests/ve: check CPUID override emulation for VE tasks

This adds a selftest that writes a one-entry /proc/vz/cpuid_override table
and compares the leaf 1 CPUID values seen by a task in a fresh VE with
those seen by the host. A second test flushes the table after the task was
armed and expects the hardware value back. The test is built on x86 only.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-148843

> diff --git a/tools/testing/selftests/ve/Makefile b/tools/testing/selftests/ve/Makefile
> --- a/tools/testing/selftests/ve/Makefile
> +++ b/tools/testing/selftests/ve/Makefile

[ ... ]

> +# ARCH stays untouched: lib.mk picks the clang target by its emptiness.
> +ifeq ($(ARCH),)
> +ve_arch := $(shell uname -m 2>/dev/null || echo not)
> +else
> +ve_arch := $(ARCH)
> +endif
> +ifeq ($(ve_arch),$(filter $(ve_arch),x86 x86_64))
> +TEST_GEN_PROGS += ve_cpuid_override_test
> +endif

[Severity: Low]
The selftests top-level Makefile maps both i386 and x86_64 to ARCH=x86, so
this gate also enables ve_cpuid_override_test for 32-bit x86 builds.

do_cpuid() in ve_cpuid_override_test.c uses an ebx output constraint, and
its own comment says it is x86-64 only because "there ebx is not the PIC
base":

        asm volatile("cpuid"
                     : "=a" (r->eax), "=b" (r->ebx), "=c" (r->ecx), "=d" (r->edx)
                     : "a" (leaf), "c" (0));

Can this break the build of the whole ve selftests directory with ARCH=x86
on a 32-bit PIC or PIE toolchain, where GCC reports "can't find a register
in class BREG"? Should the gate exclude 32-bit x86?

> diff --git a/tools/testing/selftests/ve/ve_cpuid_override_test.c b/tools/testing/selftests/ve/ve_cpuid_override_test.c
> new file mode 100644
> --- /dev/null
> +++ b/tools/testing/selftests/ve/ve_cpuid_override_test.c

[ ... ]

> +	ASSERT_GE(waitpid(pid, &status, WUNTRACED), 0);
> +	if (!WIFSTOPPED(status)) {

[ ... ]

> +static void resume_cpuid_child(struct __test_metadata *_metadata, pid_t pid)
> +{
> +	ASSERT_EQ(kill(pid, SIGCONT), 0);
> +}

[Severity: Medium]
Does this synchronization guarantee a context switch before SIGCONT? The
waitpid(WUNTRACED) in spawn_cpuid_child() only guarantees that the stop was
reported to the parent, not that the child has been switched out.

In the kernel stop path, do_signal_stop() sets TASK_STOPPED and calls
do_notify_parent_cldstop() before it calls schedule():

cpuid_child()
   unshare(CLONE_NEWVE);
   raise(SIGSTOP);
      do_signal_stop()
         set_special_state(TASK_STOPPED);
         do_notify_parent_cldstop();   <- parent's waitpid() can return here
         schedule();

If the parent runs resume_cpuid_child() inside that window, SIGCONT sets
the child back to TASK_RUNNING. When the child then reaches schedule(), it
is no longer in a sleeping state and is not deactivated, so it can keep
running on the same CPU with no context switch.

The commit message says faulting is switched on "at its next context
switch" and that "a stopped task is off the CPU for sure". Without the
switch, faulting is never armed for the child.

In ve_sees_table_host_sees_hardware, the child would then see the hardware
CPUID and the EXPECT_EQ on edx would fail on a correct kernel.

In flushed_table_falls_back_to_hardware, the child would see the hardware
CPUID, which is the expected value, so the test would pass even if the
kernel never armed faulting.

The window is narrow, since the parent must wake and finish waitpid()
first, but it appears to be structurally possible. Is there something in
the test that waits for the child to actually schedule out before SIGCONT
is sent?

I did not find a later fix for this in the series. The follow-up patches
"x86/ve: enable CPUID faulting at once when the current task enters a VE"
and "selftests/ve: check CPUID override right after entering a VE" change
the kernel to arm at once and add a test without the stop. The final-state
test file still uses the same waitpid(WUNTRACED) followed by kill(SIGCONT)
synchronization for the stop-based tests.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261007-b4-prep-khorenko-vstor-148656-cpuid-v4-0-bf16c29bea24@virtuozzo.com?part=1

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel