Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [Medium] The NVMe DMA test hardcodes the Amazon PCI Vendor ID (0x1d0f), causing false failures on generic hardware. - [Medium] Missing compiler memory barrier before ringing the NVMe Submission Queue doorbell allows dangerous compiler reordering. - [Medium] gmem_provider_iommufd_test fails to exercise the KVM guest_memfd private memory path because it does not set the KVM_MEMORY_ATTRIBUTE_PRIVATE attribute. - [Medium] The test claims to confirm a fresh IOMMU_IOAS_MAP_FILE succeeds after restoring the page, but does not actually issue the ioctl. - [Low] Comment incorrectly claims the NVMe completion phase bit is inverted, contradicting the code and the NVMe specification. --
--- Patch [10]: [RFC PATCH 10/11] selftests/kvm: gmem_provider iommufd tests --- commit 104877080caa9ea2c6214f716d67d2b54448bb37 Author: David Woodhouse <[email protected]> Date: Thu Jul 16 09:31:30 2026 +0000 selftests/kvm: gmem_provider iommufd tests Three selftests that exercise the guest_memfd provider ABI from the iommufd side, going progressively closer to a real assigned-device setup. > diff --git a/tools/testing/selftests/kvm/x86/gmem_provider_iommufd_test.c > b/tools/testing/selftests/kvm/x86/gmem_provider_iommufd_test.c > new file mode 100644 > index 0000000000000..0b2fc097b5a91 > --- /dev/null > +++ b/tools/testing/selftests/kvm/x86/gmem_provider_iommufd_test.c [ ... ] > + hva = mmap(NULL, DATA_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, > gmem_fd, 0); > + TEST_ASSERT(hva != MAP_FAILED, > + "provider mmap failed errno=%d", errno); > + r = __vm_set_user_memory_region2(vm, DATA_SLOT, KVM_MEM_GUEST_MEMFD, > + DATA_GPA, DATA_SIZE, hva, gmem_fd, 0); > + TEST_ASSERT(!r, "KVM_SET_USER_MEMORY_REGION2 failed r=%d errno=%d", r, > errno); > + virt_map(vm, DATA_GPA, DATA_GPA, 1); > + > + /* > + * 3) iommufd side: allocate IOAS [Severity: Medium] Does main() actually exercise the KVM guest_memfd private memory path? Because the KVM_MEMORY_ATTRIBUTE_PRIVATE attribute is never set on this GPA via vm_mem_set_private(), KVM will fault on the shared memory HVA. Does this completely bypass the KVM guest_memfd logic, with the guest write reaching the provider fd via the HVA mmap path instead? [ ... ] > diff --git a/tools/testing/selftests/kvm/x86/gmem_provider_nvme_dma_test.c > b/tools/testing/selftests/kvm/x86/gmem_provider_nvme_dma_test.c > new file mode 100644 > index 0000000000000..8c8a4b2e8ac96 > --- /dev/null > +++ b/tools/testing/selftests/kvm/x86/gmem_provider_nvme_dma_test.c [ ... ] > + /* Identify Controller SQE at slot 0. */ > + { > + uint32_t *s = (uint32_t *)sq_buf; > + memset(s, 0, 64); > + s[0] = 0x00010006; /* CID=1 << 16 | OPC=Identify */ > + s[1] = 0; /* NSID */ > + /* PRP1 (offset 24 in SQE = dword 6) */ > + s[6] = (uint32_t)IOVA_DST; > + s[7] = (uint32_t)(IOVA_DST >> 32); > + s[8] = 0; > + s[9] = 0; /* PRP2 */ > + s[10] = 0x01; /* CDW10: CNS = Identify Controller */ > + } > + > + /* Ring SQ0 tail doorbell. */ > + wr32(bar, NVME_SQ0TDBL, 1); [Severity: Medium] Is there a risk of the compiler reordering the SQE initialization after the doorbell ring in main()? Since the SQE is prepared via non-volatile stores and the doorbell is written via a volatile cast, the C standard permits reordering non-volatile stores after volatile stores. Could this result in the NVMe controller processing an incomplete or zeroed command? Would it be safer to add a compiler barrier before writing the doorbell? [ ... ] > + /* Provider region now holds Identify Controller data. VID/SSVID at > bytes 0..3 > + * must equal the device's PCI vendor + subsystem vendor (both are > Amazon here). */ > + { > + uint16_t vid = ((uint16_t *)provider_hva)[0]; > + uint16_t ssvid = ((uint16_t *)provider_hva)[1]; > + uint8_t first = ((uint8_t *)provider_hva)[0]; > + uint8_t poison_still = ((uint8_t *)provider_hva)[8]; > + char mn[41] = {}; > + memcpy(mn, (char *)provider_hva + 24, 40); > + fprintf(stderr, "MARKER: read done; byte0=0x%02x > byte8=0x%02x\n", > + first, poison_still); > + fflush(stderr); > + pr_info("Identify Controller in provider: VID=0x%04x > SSVID=0x%04x MN='%s'\n", > + vid, ssvid, mn); > + fflush(stdout); > + TEST_ASSERT(vid == 0x1d0f, > + "unexpected VID 0x%04x in provider region -- > Identify DMA landed elsewhere?\n" > + " poison=0x%02x byte0=0x%02x (poison intact=DMA > didn't land here)", > + vid, poison_still, first); [Severity: Medium] Does main() hardcode the Amazon PCI Vendor ID here? If this test is run on a non-Amazon NVMe drive, won't this assertion cause it to fail on generic hardware? Should we fetch the PCI Vendor ID from the config space instead of hardcoding it to 0x1d0f? [ ... ] > + /* Issue a second Identify Controller aimed at the revoked IOVA. > + * Use a new CID and place the SQE in slot 1; the SQ ring will > + * wrap for larger runs but we only ever use slots 0 and 1. > + */ > + memset(&s[16], 0, 64); > + s[16] = 0x00020006; /* CID=2 << 16 | > OPC=Identify */ > + s[16 + 6] = (uint32_t)IOVA_DST; > + s[16 + 7] = (uint32_t)(IOVA_DST >> 32); > + s[16 + 10] = 0x01; /* CNS = Identify > Controller */ > + > + wr32(bar, NVME_SQ0TDBL, 2); [Severity: Medium] Similar to the first command submission in main(), does this need a compiler barrier before the doorbell ring to prevent the compiler from sinking the SQE initialization? > + pr_info("Identify Controller (revoked target) submitted (SQ0 > tail=2)\n"); > + > + /* Poll for CQE at slot 1 with phase bit inverted from first > pass. */ > + for (i = 0; i < 5000; i++) { > + if ((c[4 + 3] & 0x10000)) /* CQE 1 P bit */ [Severity: Low] Is this comment accurate? Since this is only the second command in a 64-entry queue, the controller is still on its first pass. The phase bit remains 1 and does not invert, which matches the code's correct check for a 1 bit. [ ... ] > + /* Restore and confirm a fresh IOMMU_IOAS_MAP_FILE succeeds. */ > + rev.present = 1; > + ret = ioctl(gmem_fd, GMEM_PROVIDER_SET_PRESENT, &rev); > + TEST_ASSERT(ret == 0, "SET_PRESENT(restore) errno=%d", errno); > + pr_info("Restore + revoke round trip complete\n"); > + } [Severity: Medium] The comment in main() mentions confirming a fresh IOMMU_IOAS_MAP_FILE succeeds, but does the test actually attempt to issue this ioctl after restoring the page? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/f09647f427565c1789a6979470045aab78661563.1784194290.git.d...@amazon.co.uk?part=10
