Re: [RFC PATCH 10/11] selftests/kvm: gmem_provider iommufd tests

[sashiko-bot@kernel.org]

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 <dwmw@amazon.co.uk>
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.dwmw@amazon.co.uk?part=10