Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] The test does not check for partial completion of KVM_SEV_SNP_LAUNCH_UPDATE, leading to potentially silent incomplete memory initialization. - [Medium] The test fails to assert KVM_EXIT_MEMORY_FAULT on the revoke path because the validation is placed in an unreachable 'else' branch. --
--- Patch [9]: [RFC PATCH 09/11] selftests/kvm: gmem_provider KVM-only tests --- commit 93ae3986a0340a4d14084b20d7839ed583b0c01b Author: David Woodhouse <[email protected]> selftests/kvm: gmem_provider KVM-only tests Three selftests that exercise the guest_memfd provider ABI purely from the KVM side (no iommufd, no assigned devices). These tests verify the live-update survivability, hugepage mappings, and memory revocation. > diff --git a/tools/testing/selftests/kvm/x86/gmem_provider_revoke_test.c > b/tools/testing/selftests/kvm/x86/gmem_provider_revoke_test.c > index 0000000000000..7e711ea526748 100644 > --- /dev/null > +++ b/tools/testing/selftests/kvm/x86/gmem_provider_revoke_test.c [ ... ] > + /* 2) Revoke: mark absent and zap the guest NPT (provider->KVM). */ > + req = (struct gmem_provider_present){ .offset = 0, .len = 4096, > .present = 0 }; > + r = ioctl(gmem_fd, GMEM_PROVIDER_SET_PRESENT, &req); > + TEST_ASSERT(!r, "revoke ioctl failed, errno %d", errno); > + > + /* 3) Guest re-reads -> re-fault into absent get_pfn -> must NOT see > MAGIC. */ > + r = _vcpu_run(vcpu); > + if (r) { > + pr_info("revoked: vcpu_run failed (errno %d) -- guest faulted, > as expected\n", > + errno); > + } else { > + TEST_ASSERT(vcpu->run->exit_reason == KVM_EXIT_MEMORY_FAULT, > + "revoked: expected fault or KVM_EXIT_MEMORY_FAULT, > got %u (%s)", > + vcpu->run->exit_reason, > + exit_reason_str(vcpu->run->exit_reason)); > + pr_info("revoked: KVM_EXIT_MEMORY_FAULT as expected\n"); > + } [Severity: Medium] Since KVM_EXIT_MEMORY_FAULT always causes the KVM_RUN ioctl to return -1 with errno EFAULT, won't _vcpu_run(vcpu) return a non-zero value here? If so, this execution path would take the if (r) branch, completely bypassing the exit reason validation in the else block. Should this check be moved or adjusted to properly assert that the exit reason is indeed KVM_EXIT_MEMORY_FAULT? > + > + /* 4) Restore: mark present again. */ > + req.present = 1; > + r = ioctl(gmem_fd, GMEM_PROVIDER_SET_PRESENT, &req); > + TEST_ASSERT(!r, "restore ioctl failed, errno %d", errno); > + [ ... ] > diff --git a/tools/testing/selftests/kvm/x86/gmem_provider_test.c > b/tools/testing/selftests/kvm/x86/gmem_provider_test.c > index 0000000000000..d7caa11616dfe 100644 > --- /dev/null > +++ b/tools/testing/selftests/kvm/x86/gmem_provider_test.c [ ... ] > + struct kvm_sev_snp_launch_start start = { .policy = 0x30000 }; > + ret = sev_ioctl(vm_fd, sev_fd, KVM_SEV_SNP_LAUNCH_START, &start); > + if (ret) { perror("SNP_LAUNCH_START"); goto out; } > + > + printf("[VM%d] SNP_LAUNCH_UPDATE (code + zero, %luMB)\n", > + vm_num, GUEST_MEM_SIZE >> 20); > + struct kvm_sev_snp_launch_update update = { > + .gfn_start = 0, > + .uaddr = (uint64_t)(unsigned long)src, > + .len = PAGE_SIZE_4K, > + .type = KVM_SEV_SNP_PAGE_TYPE_NORMAL, > + }; > + ret = sev_ioctl(vm_fd, sev_fd, KVM_SEV_SNP_LAUNCH_UPDATE, &update); > + if (ret) { perror("SNP_LAUNCH_UPDATE code"); goto out; } [Severity: Medium] If a signal interrupts the KVM_SEV_SNP_LAUNCH_UPDATE ioctl, KVM might return 0 but leave update.len > 0 indicating a partial completion. Does this code need a loop to check update.len and retry the remaining length to prevent silent partial initialization? > + > + struct kvm_sev_snp_launch_update update_zero = { > + .gfn_start = 1, > + .uaddr = (uint64_t)(unsigned long)(src + PAGE_SIZE_4K), > + .len = GUEST_MEM_SIZE - PAGE_SIZE_4K, > + .type = KVM_SEV_SNP_PAGE_TYPE_ZERO, > + }; > + ret = sev_ioctl(vm_fd, sev_fd, KVM_SEV_SNP_LAUNCH_UPDATE, &update_zero); > + if (ret) { perror("SNP_LAUNCH_UPDATE zero"); goto out; } [Severity: Medium] Similarly, could a signal interrupting this update_zero ioctl cause a partial completion here as well? If the return value is 0 but update_zero.len is still greater than 0, the rest of the 16MB guest memory would be left uninitialized, potentially causing flaky test failures later. > + > + struct kvm_sev_snp_launch_finish finish = { 0 }; > + ret = sev_ioctl(vm_fd, sev_fd, KVM_SEV_SNP_LAUNCH_FINISH, &finish); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/f09647f427565c1789a6979470045aab78661563.1784194290.git.d...@amazon.co.uk?part=9
