Re: [PATCH] KVM: selftests: riscv: Test the SBI STA shared memory lifecycle

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] KVM: selftests: riscv: malformed Signed-off-by trailer and
  author mismatch
- [Medium] KVM: selftests: riscv: uncleared dirty log in
  check_sta_lifecycle()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Medium] KVM: selftests: infinite loop on schedstat parsing failure in
  get_run_delay() [1]
--

commit 711407189204983601d5cf730f872165c9f4324d
Author: Bingyu.Xian <shanbeeyoo@gmail.com>

> KVM: selftests: riscv: Test the SBI STA shared memory lifecycle
>
> The RISC-V steal-time test checks the UAPI alignment constraints and
> compares stolen time with the host run delay. It does not exercise
> changes to the shared page after accounting has been enabled.
>
> Extend the existing test to check that disabling STA stops shared-page
> updates and that reconfiguring it leaves the old shared page unchanged.
> Place the second STA structure at the end of another page and verify
> that updates mark the appropriate page dirty.
>
> Discard the second backing page with MADV_DONTNEED and check that a
> subsequent KVM_RUN resumes STA updates on the replacement page and
> marks it dirty. Read the replacement page before KVM_RUN so that a
> stale write into the old backing page cannot satisfy the assertions.
>
> Keep these checks in steal_time instead of adding another test binary.
> The test does not depend on a gfn-to-pfn cache implementation.
>
> Assisted-by: YuanSheng: deepseek-v4-pro
> Co-developed-by: Quan Zhou <zhouquan@iscas.ac.cn>
> Signed-off-by: Quan Zhou <zhouquan@iscas.ac.cn>
> Signed-off-by: bingyu.xian <shanbeenyoo@gmail.com>

[Severity: High]
Both the Author header and the final Signed-off-by trailer format the name
as a dot-separated single token ("Bingyu.Xian" / "bingyu.xian") instead of a
space-separated full name ("Bingyu Xian"), and the email addresses do not
match (shanbeeyoo@gmail.com vs shanbeenyoo@gmail.com).

> diff --git a/tools/testing/selftests/kvm/steal_time.c b/tools/testing/selftests/kvm/steal_time.c
> index 82b94a82dd035..059f7fa60a207 100644
> --- a/tools/testing/selftests/kvm/steal_time.c
> +++ b/tools/testing/selftests/kvm/steal_time.c

[ ... ]

> @@ -329,6 +330,111 @@ static void steal_time_dump(struct kvm_vm *vm, u32 vcpu_idx)
>  	pr_info("\n");
>  }
>  
> +static gpa_t lifecycle_second_gpa;
> +
> +static void sta_lifecycle_guest(void)
> +{
> +	sta_set_shmem(ST_GPA_BASE, 0);
> +	GUEST_SYNC(0);
> +	/* The first update takes place on the next KVM_RUN. */
> +	GUEST_SYNC(1);
> +
> +	sta_set_shmem(INVALID_GPA, 0);
> +	GUEST_SYNC(2);
> +	GUEST_SYNC(3);
> +
> +	sta_set_shmem(lifecycle_second_gpa, 0);
> +	GUEST_SYNC(4);
> +	GUEST_SYNC(5);
> +
> +	/* Userspace discards the second backing page before resuming us. */
> +	GUEST_SYNC(6);
> +	sta_set_shmem(INVALID_GPA, 0);
> +	GUEST_DONE();
> +}

[ ... ]

> +static void check_sta_lifecycle(void)
> +{
> +	struct sta_struct snapshot, *first, *second;
> +	struct kvm_vcpu *vcpu;
> +	struct kvm_vm *vm;
> +	unsigned long dirty = 0;
> +	void *second_page;
> +
> +	vm = vm_create_with_one_vcpu(&vcpu, sta_lifecycle_guest);
> +	vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, ST_GPA_BASE,
> +				    1, 2, KVM_MEM_LOG_DIRTY_PAGES);
> +	/* Also exercise an STA structure ending exactly at a page boundary. */
> +	WRITE_AND_SYNC_TO_GUEST(vm, lifecycle_second_gpa,
> +				ST_GPA_BASE + 2 * vm->page_size - sizeof(struct sta_struct));
> +	first = addr_gpa2hva(vm, ST_GPA_BASE);
> +	second = addr_gpa2hva(vm, lifecycle_second_gpa);
> +	second_page = addr_gpa2hva(vm, ST_GPA_BASE + vm->page_size);
> +
> +	sta_lifecycle_run(vcpu, 0);
> +	sta_lifecycle_run(vcpu, 1);
> +	check_sta_page(first);
> +	kvm_vm_get_dirty_log(vm, 1, &dirty);
> +	TEST_ASSERT(dirty & BIT(0), "STA update did not dirty the first page");
> +	ksft_test_result_pass("STA shared page updates and is marked dirty\n");
> +
> +	sta_lifecycle_run(vcpu, 2);
> +	snapshot = *first;
> +	sta_lifecycle_run(vcpu, 3);
> +	TEST_ASSERT(!memcmp(first, &snapshot, sizeof(snapshot)),
> +		    "Disabled STA shared page was modified");
> +	ksft_test_result_pass("STA disable stops shared page updates\n");
> +
> +	sta_lifecycle_run(vcpu, 4);
> +	sta_lifecycle_run(vcpu, 5);
> +	check_sta_page(second);
> +	TEST_ASSERT(!memcmp(first, &snapshot, sizeof(snapshot)),
> +		    "Old STA shared page was modified after reconfiguration");
> +	dirty = 0;
> +	kvm_vm_get_dirty_log(vm, 1, &dirty);
> +	TEST_ASSERT(dirty & BIT(1), "STA update did not dirty the new page");

[Severity: Medium]
Can the dirty-log checks after stages 1 and 5 pass even if
kvm_riscv_vcpu_record_steal_time() fails to mark the STA page dirty?

During stages 0 and 4, sta_set_shmem() invokes
kvm_sbi_sta_steal_time_set_shmem(), which zeroes the structure via
kvm_vcpu_write_guest() -> __kvm_write_guest_page() ->
mark_page_dirty_in_slot().  Because the dirty log is not cleared after
stage 0 or stage 4, BIT(0) and BIT(1) are already set before the KVM_RUN
steal-time updates in stages 1 and 5 occur.  In addition, entering stage 2
updates and dirties page 0 before sta_set_shmem(INVALID_GPA, 0) runs, so
leaving the dirty bitmap uncleared after stage 2 also prevents stage 5 from
checking that BIT(0) stayed clear after reconfiguration.

[ ... ]

> @@ -535,9 +641,16 @@ int main(int ac, char **av)
>  
>  	ksft_print_header();
>  	TEST_REQUIRE(is_steal_time_supported(vcpus[0]));
> +#ifdef __riscv
> +	ksft_set_plan(NR_VCPUS + 4);
> +#else
>  	ksft_set_plan(NR_VCPUS);
> +#endif
>  
>  	check_steal_time_uapi();
> +#ifdef __riscv
> +	check_sta_lifecycle();
> +#endif
>  
>  	/* Run test on each VCPU */
>  	for (i = 0; i < NR_VCPUS; ++i) {

[Severity: Medium]
This problem wasn't introduced by this patch, but can the per-VCPU loop in
main() spin forever [1] when /proc/<pid>/schedstat is unavailable or
unreadable (for example when CONFIG_SCHED_INFO is disabled)?

tools/testing/selftests/kvm/lib/test_util.c:get_run_delay() {
	...
	/* Return MIN_RUN_DELAY_NS upon failure just to be safe */
	if (fscanf(fp, "%ld %ld ", &val[0], &val[1]) < 2)
		val[1] = MIN_RUN_DELAY_NS;
	...
	return val[1];
}

When fscanf() fails repeatedly, both the initial run_delay and every
subsequent get_run_delay() call in the polling loop return MIN_RUN_DELAY_NS,
so get_run_delay() - run_delay is always 0 and never reaches
MIN_RUN_DELAY_NS.

[1] https://sashiko.dev/#/bug/linux-a91f66d8-0fe1-40da-93b7-ac05da87bc12

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115085457.889873-1-shanbeeyoo@gmail.com?part=1