Re: [PATCH v4 9/9] KVM: selftests: Add nested SVM DecodeAssists test

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Test logic codifies a KVM emulator bug regarding linear address wrapping in compatibility mode.
- [Medium] Guest assertion failures are silently masked by MMIO exit assertions.
--

--- Patch [9]: [PATCH v4 9/9] KVM: selftests: Add nested SVM DecodeAssists test ---
commit 05cea14bcd0202443d9a4c1a4e002383cf66eb03
Author: Tina Zhang <[email protected]>

    KVM: selftests: Add nested SVM DecodeAssists test
    
    Add a nested SVM selftest for DecodeAssists.  Verify that KVM exposes
    the feature to L1 and provides architectural exit state for MOV CR/DR,
    CLTS, LMSW, SMSW, INTn, INVLPG, and INVLPGA intercepts.

> diff --git a/tools/testing/selftests/kvm/x86/svm_nested_decode_assists_test.c b/tools/testing/selftests/kvm/x86/svm_nested_decode_assists_test.c
> new file mode 100644
> index 0000000000000..13511ca8473b2
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86/svm_nested_decode_assists_test.c
> @@ -0,0 +1,791 @@

[ ... ]

> +static void test_synthesized_npf_linear_wrap(struct svm_test_data *svm,
> +					     gpa_t npf_gpa)
> +{
> +	struct vmcb *vmcb = svm->vmcb;
> +	u16 cs_attrib = vmcb->save.cs.attrib;
> +	u64 cs_base = vmcb->save.cs.base;
> +	u32 cs_limit = vmcb->save.cs.limit;
> +	u32 outsb_eip = LINEAR_WRAP_CODE_GVA + LINEAR_WRAP_OUTSB_OFFSET -
> +			LINEAR_WRAP_CS_BASE;
> +	u32 setup_eip = LINEAR_WRAP_CODE_GVA + LINEAR_WRAP_SETUP_OFFSET -
> +			LINEAR_WRAP_CS_BASE;
> +
> +	vmcb->save.cs.attrib &= ~SVM_SELECTOR_L_MASK;
> +	vmcb->save.cs.attrib |= SVM_SELECTOR_DB_MASK;
> +	vmcb->save.cs.base = LINEAR_WRAP_CS_BASE;
> +	vmcb->save.cs.limit = UINT32_MAX;
> +	prepare_l2_for_vmrun(svm, setup_eip);
> +	run_guest(vmcb, svm->vmcb_gpa);
> +	GUEST_ASSERT_EQ(vmcb->control.exit_code, SVM_EXIT_NPF);
> +	GUEST_ASSERT_EQ(vmcb->control.exit_info_2, npf_gpa);
> +	GUEST_ASSERT_EQ(vmcb->save.rip, outsb_eip);
> +	GUEST_ASSERT_EQ(vmcb->control.insn_len,
> +			sizeof(linear_wrap_insn_bytes));
> +	GUEST_ASSERT(!memcmp(vmcb->control.insn_bytes,
> +			     linear_wrap_insn_bytes,
> +			     sizeof(linear_wrap_insn_bytes)));

[Severity: Medium]
Will this assertion artificially fail if KVM's address generation is fixed to
comply with the architecture?

In compatibility mode, the 32-bit effective address and 64-bit segment base
are added to form a 64-bit linear address, which does not wrap at 4GB
according to AMD APM Vol 2 and Intel SDM. KVM's emulator currently
incorrectly masks the linear address to 32 bits for instruction fetches
outside of 64-bit mode.

[ ... ]

> +static void prepare_linear_wrap_outsb_code(struct kvm_vm *vm)
> +{
> +	gva_t code_gva = vm_alloc_pages(vm, 2);
> +	u8 *high_page = addr_gva2hva(vm, code_gva);
> +	u8 *low_page = addr_gva2hva(vm, code_gva + PAGE_SIZE);
> +	gpa_t high_gpa = addr_gva2gpa(vm, code_gva);
> +	gpa_t low_gpa = addr_gva2gpa(vm, code_gva + PAGE_SIZE);
> +	u32 source = (u32)(u64)&npf_target;
> +	u8 *setup = &high_page[LINEAR_WRAP_SETUP_OFFSET];
> +
> +	TEST_ASSERT((u64)&npf_target <= UINT32_MAX,
> +		    "npf_target must be addressable from compatibility mode");
> +
> +	/* mov $npf_target, %esi; mov $0x80, %dx */
> +	setup[0] = 0xbe;
> +	memcpy(&setup[1], &source, sizeof(source));
> +	setup[5] = 0x66;
> +	setup[6] = 0xba;
> +	setup[7] = 0x80;
> +	setup[8] = 0x00;
> +
> +	memcpy(&high_page[LINEAR_WRAP_OUTSB_OFFSET],
> +	       linear_wrap_insn_bytes,
> +	       PAGE_SIZE - LINEAR_WRAP_OUTSB_OFFSET);
> +	memcpy(low_page,
> +	       &linear_wrap_insn_bytes[PAGE_SIZE - LINEAR_WRAP_OUTSB_OFFSET],
> +	       sizeof(linear_wrap_insn_bytes) -
> +	       (PAGE_SIZE - LINEAR_WRAP_OUTSB_OFFSET));
> +
> +	virt_map(vm, LINEAR_WRAP_CODE_GVA, high_gpa, 1);
> +	virt_map(vm, 0, low_gpa, 1);

[Severity: Medium]
Does mapping GVA 0 here explicitly rely on the emulator's non-architectural
32-bit wrap to provide the instruction bytes?

If the test is run on a fixed KVM, the true linear address at 0x100000000
will be unmapped.

[ ... ]

> +static void complete_mmio_read(struct kvm_vcpu *vcpu, gpa_t expected_gpa,
> +			       u8 value)
> +{
> +	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_MMIO);

[Severity: Medium]
Does this assert MMIO without checking for a prior guest assertion?

If a test fails and triggers a KVM_EXIT_IO with UCALL_ABORT, we will hit this
assertion and mask the true error payload.

[ ... ]

> +int main(int argc, char *argv[])
> +{
> +	gva_t svm_gva, npf_gva, boundary_page_gva, iopm_gva;
> +	gva_t canonical_code_gva;
> +	gpa_t npf_gpa, mmio_source_gpa, mmio_gpa, iopm_gpa;
> +	struct userspace_mem_region *region;
> +	struct kvm_vcpu *vcpu;
> +	struct kvm_vm *vm;
> +	u64 *pte;
> +
> +	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_SVM));
> +	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_NPT));
> +	TEST_REQUIRE(this_cpu_has(X86_FEATURE_DECODEASSISTS));
> +	TEST_ASSERT(kvm_cpu_has(X86_FEATURE_DECODEASSISTS),
> +		    "KVM failed to expose DecodeAssists");
> +	TEST_REQUIRE(kvm_has_cap(KVM_CAP_EXCEPTION_PAYLOAD));
> +
> +	vm = vm_create_with_one_vcpu(&vcpu, l1_guest_code);
> +	vm_enable_cap(vm, KVM_CAP_EXCEPTION_PAYLOAD, 1);
> +	prepare_boundary_outsb_code(vm);
> +	canonical_code_gva = prepare_canonical_boundary_outsb_code(vm);
> +	prepare_linear_wrap_outsb_code(vm);
> +	vm_enable_npt(vm);
> +	vcpu_alloc_svm(vm, &svm_gva);
> +	iopm_gva = vm_alloc_pages(vm, TEST_IOPM_SIZE / PAGE_SIZE);
> +	iopm_gpa = addr_gva2gpa(vm, iopm_gva);
> +	memset(addr_gva2hva(vm, iopm_gva), 0, TEST_IOPM_SIZE);
> +	npf_gva = (gva_t)&npf_target;
> +	npf_gpa = addr_gva2gpa(vm, npf_gva);
> +
> +	tdp_identity_map_default_memslots(vm);
> +	pte = tdp_get_pte(vm, npf_gpa);
> +	*pte &= ~PTE_PRESENT_MASK(&vm->stage2_mmu);
> +	region = memslot2region(vm, 0);
> +	mmio_gpa = region->region.guest_phys_addr +
> +		   region->region.memory_size + PAGE_SIZE;
> +	mmio_source_gpa = addr_gva2gpa(vm, (gva_t)&mmio_source);
> +	pte = tdp_get_pte(vm, mmio_source_gpa);
> +	*pte = (*pte & ~PHYSICAL_PAGE_MASK) | mmio_gpa;
> +
> +	boundary_page_gva = (gva_t)&boundary_outsb_code[PAGE_SIZE];
> +	pte = vm_get_pte(vm, boundary_page_gva);
> +	*pte &= ~PTE_PRESENT_MASK(&vm->mmu);
> +
> +	vcpu_args_set(vcpu, 4, svm_gva, npf_gpa, iopm_gpa,
> +		      canonical_code_gva);
> +
> +	/* Complete the MMIO source read in the MOVSB #NPF regression test. */
> +	vcpu_run(vcpu);
> +	complete_mmio_read(vcpu, mmio_gpa, 0xa5);

[Severity: Medium]
If any test preceding this MMIO read (e.g. test_hardware_npf()) fails, the
vcpu will exit early. Will that hit the generic MMIO check in
complete_mmio_read() and lose the REPORT_GUEST_ASSERT output, making it
difficult to debug?

Could the test explicitly check for UCALL_ABORT when encountering KVM_EXIT_IO
before asserting on MMIO?

> +
> +	/* Leave the second MMIO read pending while injecting #PF. */
> +	vcpu_run(vcpu);
> +	complete_mmio_read(vcpu, mmio_gpa, 0x5a);
> +	queue_userspace_pf(vcpu);
> +	vcpu_run(vcpu);
> +	assert_ucall_done(vcpu);
> +
> +	kvm_vm_free(vm);
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9