Re: [PATCH v2 14/20] KVM: selftests: Add nested VMX APIC cache invalidation test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] KVM: selftests: missing virtualize APIC accesses check in main()
- [Medium] KVM: selftests: memslot move races with vmlaunch() and vmresume() in run()
- [Medium] KVM: selftests: vmx_apic_update_test fails to verify APIC-access exits
--

commit c3febf61f5d64e784b8c55b816b4a00a658a0208
Author: Fred Griffoul <fgriffo@amazon.co.uk>

KVM: selftests: Add nested VMX APIC cache invalidation test

This commit adds a selftest for nested VMX APIC-access page cache
invalidation while an L2 guest writes to the APIC-access page concurrently
with either madvise(MADV_DONTNEED) or memslot moves.

> diff --git a/tools/testing/selftests/kvm/x86/vmx_apic_update_test.c b/tools/testing/selftests/kvm/x86/vmx_apic_update_test.c
> new file mode 100644
> index 0000000000000..357e10c88cd0a
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86/vmx_apic_update_test.c
[ ... ]
> +static void l1_guest_code(struct vmx_pages *vmx_pages)
> +{
> +	uint32_t control, exit_reason;
> +
> +	prepare_for_vmx_operation(vmx_pages);
> +	load_vmcs(vmx_pages);
> +	prepare_vmcs(vmx_pages, l2_guest_code);
> +
> +	/* Enable APIC access */
> +	control = vmread(CPU_BASED_VM_EXEC_CONTROL);
> +	control |= CPU_BASED_ACTIVATE_SECONDARY_CONTROLS;
> +	vmwrite(CPU_BASED_VM_EXEC_CONTROL, control);
> +	control = vmread(SECONDARY_VM_EXEC_CONTROL);
> +	control |= SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES;
> +	vmwrite(SECONDARY_VM_EXEC_CONTROL, control);
> +	vmwrite(APIC_ACCESS_ADDR, VAPIC_GPA);
> +
> +	GUEST_SYNC1(0);
> +	vmlaunch();
> +again:
> +	exit_reason = vmread(VM_EXIT_REASON);
> +	if (exit_reason == EXIT_REASON_APIC_ACCESS) {
> +		uint64_t guest_rip = vmread(GUEST_RIP);
> +		uint64_t instr_len = vmread(VM_EXIT_INSTRUCTION_LEN);
> +
> +		vmwrite(GUEST_RIP, guest_rip + instr_len);
> +		vmresume();
> +		goto again;
> +	}
> +
> +	GUEST_SYNC1(exit_reason);
> +	GUEST_ASSERT(exit_reason == EXIT_REASON_VMCALL);
> +	GUEST_DONE();
> +}
[ ... ]
> +static void *update_move_memslot(void *arg)
> +{
> +	struct update_control *ctrl = arg;
> +	uint64_t gpa = VAPIC_GPA;
> +
> +	wait_for_start_signal(ctrl);
> +
> +	/*
> +	 * Alternate the slot between VAPIC_GPA and an alternate address,
> +	 * so that the GPA under test is repeatedly unbacked (L2's APIC
> +	 * access bounces to userspace as MMIO) and re-backed (KVM must
> +	 * re-latch the page). Moving strictly away would change
> +	 * vCPU-visible state only once.
> +	 */
> +	while (is_running(ctrl)) {
> +		usleep(update_period_ms * 1000);
> +		gpa = (gpa == VAPIC_GPA) ? VAPIC_GPA + 0x10000 : VAPIC_GPA;
> +		vm_mem_region_move(ctrl->vm, VAPIC_SLOT, gpa);
> +		ctrl->updates++;
> +	}
> +
> +	return NULL;
> +}
> +
> +static void run(void * (*update)(void *), const char *name, bool mmio_ok)
> +{
[ ... ]
> +		case KVM_EXIT_MMIO:
> +			/*
> +			 * While the slot is moved away, the GPA under test
> +			 * is unbacked and the access legitimately bounces
> +			 * to userspace as MMIO (until the updater moves it
> +			 * back). madvise() never unbacks the GPA, so there
> +			 * an MMIO exit means KVM lost the mapping: fail.
> +			 */
> +			TEST_ASSERT(mmio_ok,
> +				    "%s: unexpected APIC MMIO access at 0x%llx",
> +				    progname, vcpu->run->mmio.phys_addr);
> +			printf
> +			    ("%s: APIC MMIO access at 0x%llx (memslot move effect)\n",
> +			     progname, vcpu->run->mmio.phys_addr);
> +			break;

[Severity: Medium]
Does checking for the absence of KVM_EXIT_MMIO in run() actually verify that
APIC-access virtualization stayed active across invalidations?

In l2_guest_code(), L2 performs 8 writes to VAPIC_GPA + 0x80 followed by
vmcall(), while l1_guest_code() handles EXIT_REASON_APIC_ACCESS by advancing
GUEST_RIP and executing vmresume() without counting how many
EXIT_REASON_APIC_ACCESS exits occurred.

In the madvise(MADV_DONTNEED) test, VAPIC_SLOT remains registered at
VAPIC_GPA. After madvise() zaps the userspace PTE and EPT02 SPTE, L2's next
write faults in a new anonymous host page in EPT02 rather than triggering
KVM_EXIT_MMIO. If KVM fails to refresh vmcs02.APIC_ACCESS_ADDR to the new
HPA (or fails to re-enable SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES after
update_move_memslot() moves VAPIC_SLOT back to VAPIC_GPA), the EPT02 HPA
will not match vmcs02.APIC_ACCESS_ADDR and L2's writes will silently commit
to normal memory without triggering either EXIT_REASON_APIC_ACCESS or
KVM_EXIT_MMIO.

Should l1_guest_code() and run() count the EXIT_REASON_APIC_ACCESS exits and
assert that all 8 L2 writes are intercepted as EXIT_REASON_APIC_ACCESS in
the madvise case (and as either EXIT_REASON_APIC_ACCESS or KVM_EXIT_MMIO in
the memslot-move case)?

> +		default:
> +			TEST_FAIL("%s: Unexpected exit reason: %d (flags 0x%x)",
> +				  progname,
> +				  vcpu->run->exit_reason, vcpu->run->flags);
> +		}
> +	}

[Severity: Medium]
As reported by Sashiko on v1 [1], can update_move_memslot() race with
vmlaunch() and vmresume() in l1_guest_code() and cause run() to fail with
KVM_EXIT_INTERNAL_ERROR?

In l1_guest_code(), GUEST_SYNC1(0) starts the updater thread before
vmlaunch(), and each EXIT_REASON_APIC_ACCESS exit into L1 advances GUEST_RIP
and executes vmresume(). Concurrently, update_move_memslot() toggles
VAPIC_SLOT between VAPIC_GPA and VAPIC_GPA + 0x10000.

In KVM's nested VM-entry path:

  nested_vmx_run()
    nested_vmx_enter_non_root_mode()
      nested_get_vmcs12_pages(vcpu, false)

nested_get_vmcs12_pages() is called with lazy == false on both vmlaunch and
vmresume. Unlike lazy revalidation while L2 is already in guest mode, if
VAPIC_GPA is unbacked when L1 executes vmlaunch() or vmresume(),
nested_get_vmcs12_pages() treats the missing gfn as an error and exits to
userspace with KVM_EXIT_INTERNAL_ERROR, hitting the default TEST_FAIL() case
in run().

[ ... ]
> +int main(int argc, char *argv[])
> +{
> +	int opt_madvise = 0;
> +	int opt_memslot_move = 0;
> +
> +	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_VMX));
> +	TEST_REQUIRE(kvm_cpu_has_ept());

[Severity: Medium]
Should main() also check TEST_REQUIRE(kvm_cpu_has_vmx_virtualize_apic_accesses())
here, similar to vmx_apic_access_test.c?

In l1_guest_code(), the test unconditionally sets
SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES in SECONDARY_VM_EXEC_CONTROL before
calling vmlaunch(). On hosts or nested environments where virtualize APIC
accesses is unsupported or disabled (for example when flexpriority is
disabled), vmlaunch() will fail with a guest assertion failure instead of
skipping the test.

[1] https://lore.kernel.org/r/20260920211920.928306-13-dwmw2@infradead.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105020104.2507083-1-dwmw2@infradead.org?part=14