[Devel] [PATCH hci-8.1 3/5] selftests/ve: check CPUID override right after entering a VE

[no-reply at sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] selftests/ve: faulting child in wait_go=0 case fails at ready-read, not diagnosed
- [Low] selftests/ve: armed_before_first_reschedule cannot guarantee no reschedule, may falsely pass
--

commit 56789e953f13472fc78d80df081028a09179dc56
Author: Konstantin Khorenko <khorenko at virtuozzo.com>

selftests/ve: check CPUID override right after entering a VE

This commit adds a test case in which the child executes CPUID straight
after unshare(CLONE_NEWVE), with no blocking in between. It expects the
table values. A new wait_go field in struct child_ctx selects between the
old blocking flow and the new flow.

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

> diff --git a/tools/testing/selftests/ve/ve_cpuid_override_test.c b/tools/testing/selftests/ve/ve_cpuid_override_test.c
> --- a/tools/testing/selftests/ve/ve_cpuid_override_test.c
> +++ b/tools/testing/selftests/ve/ve_cpuid_override_test.c

[ ... ]

> @@ -73,12 +74,18 @@ static int cpuid_child(struct child_ctx *ctx)
>  
>  	if (unshare(CLONE_NEWVE) < 0)
>  		return 21;
> -	if (write(ctx->result_pipe[1], "R", 1) != 1)
> -		return 22;
> -	if (read(ctx->go_pipe[0], &go, 1) != 1)
> -		return 23;
> -
> +	if (ctx->wait_go) {
> +		if (write(ctx->result_pipe[1], "R", 1) != 1)
> +			return 22;
> +		if (read(ctx->go_pipe[0], &go, 1) != 1)
> +			return 23;
> +	}
> +
> +	/* Without wait_go nothing wakes the parent before the CPUID. */
>  	do_cpuid(CPUID_LEAF, &r);
> +
> +	if (!ctx->wait_go && write(ctx->result_pipe[1], "R", 1) != 1)
> +		return 22;

[Severity: Low]
If the kernel does not emulate the CPUID fault, can the child die with
SIGSEGV inside do_cpuid() in the wait_go=0 case, before it writes the "R"
byte?

In that case the parent's read() of the ready byte in spawn_cpuid_child()
would see EOF, and ASSERT_EQ(read(...), 1) would abort the test there.

collect_cpuid_child() would then not be reached, so the "child killed by
signal" TH_LOG is not printed and waitpid() is never called. The child is
left as a zombie until the harness exits.

The test still fails, but with a misleading assertion. The later patch
"selftests/ve: check CPUID override emulation for a prefixed CPUID" keeps
the same ordering of "R" after CPUID for wait_go=0, so this does not appear
to be fixed later in the series.

>  	if (write(ctx->result_pipe[1], &r, sizeof(r)) != sizeof(r))
>  		return 24;
>  	return 0;

[ ... ]

> +/* No context switch between entering the VE and the CPUID. */
> +TEST_F(ve_cpuid_override, armed_before_first_reschedule)
> +{
> +	struct child_ctx ctx = { .wait_go = 0 };
> +	struct cpuid_regs r;
> +	pid_t pid;
> +
> +	ASSERT_EQ(write_cpuid_override(self->table), 0);
> +
> +	pid = spawn_cpuid_child(_metadata, self->cgv2_fd, self->ctid, &ctx);
> +	collect_cpuid_child(_metadata, pid, &ctx, &r);
> +
> +	EXPECT_EQ(r.edx, self->host.edx & ~self->bit);
> +}

[Severity: Low]
Can this test pass on a kernel without the immediate-arming fix?

The comment says "No context switch between entering the VE and the
CPUID", but cpuid_child() only avoids voluntary blocking between
unshare(CLONE_NEWVE) and do_cpuid(). Nothing pins the child to a CPU or
prevents preemption, a timer tick, or a migration in that window.

Any such context switch would apply the CPUID-faulting state through
__switch_to_xtra(). The child would then see the table values even without
the fix.

The test is a single run with no retries, so it can miss the bug on a
loaded host. The later patches "x86/traps: decode the faulting CPUID with
the instruction decoder" and "selftests/ve: check CPUID override emulation
for a prefixed CPUID" add neither iterations nor pinning to this case.
Should the comment be softened, or should the test repeat the sequence?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261006-b4-prep-khorenko-vstor-148656-cpuid-v1-0-741609f4f03a@virtuozzo.com?part=3