Re: [PATCH RFC v3 79/88] KVM: selftests: Verify BAR revocation and access gating in low power state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] KVM: selftests: low_power_bar_revocation() fails on direct
  VFIO variant
--

commit de038c6f26d71384fc01f1fa33e4f547141e9831
Author: Ackerley Tng <ackerleytng@google.com>

KVM: selftests: Verify BAR revocation and access gating in low power state

This commit adds a low_power_bar_revocation subtest to guest_memfd_vfio_test
to verify that entering D3hot revokes BAR mmap mappings and rejects
pread/pwrite accesses until returning to D0.

> diff --git a/tools/testing/selftests/kvm/x86/guest_memfd_vfio_test.c b/tools/testing/selftests/kvm/x86/guest_memfd_vfio_test.c
> index 03c324f7f618d..cf2d46a365504 100644
> --- a/tools/testing/selftests/kvm/x86/guest_memfd_vfio_test.c
> +++ b/tools/testing/selftests/kvm/x86/guest_memfd_vfio_test.c
> @@ -443,4 +443,90 @@ TEST_F(guest_memfd_vfio, device_reset)
>  	ASSERT_EQ(0, munmap(bar0, bar0_info.size));
>  }
>  
> +TEST_F(guest_memfd_vfio, low_power_bar_revocation)
> +{
[ ... ]
> +	/* Enter low-power D3hot state */
> +	feat.argsz = sizeof(feat);
> +	feat.flags = VFIO_DEVICE_FEATURE_SET |
> +		     VFIO_DEVICE_FEATURE_LOW_POWER_ENTRY;
> +	ret = ioctl(self->vfio_fd, VFIO_DEVICE_FEATURE, &feat);
> +	ASSERT_EQ(0, ret);
> +
> +	/*
> +	 * In low-power state, BARs are marked unavailable and mappings are
> +	 * revoked.
> +	 *
> +	 * - Host mmap accesses trigger SIGBUS because page tables were zapped
> +	 *   and get_pfn fails with -EFAULT while in low power state.
> +	 * - Host pread() and pwrite() on the BAR descriptor fail with -EIO.
> +	 */
> +	TEST_EXPECT_SIGBUS((void)READ_ONCE(*(uint32_t *)((char *)bar0 + E1000_RAL0)));
> +	TEST_EXPECT_SIGBUS(WRITE_ONCE(*(uint32_t *)((char *)bar0 + E1000_RAL0), 0x12345678));
> +
> +	val = 0;
> +	ret_bytes = pread(self->bar_fd, &val, sizeof(val), bar0_info.offset + E1000_STATUS);
> +	ASSERT_EQ(-1, ret_bytes);
> +	ASSERT_EQ(EIO, errno);
> +
> +	val = 0x12345678;
> +	ret_bytes = pwrite(self->bar_fd, &val, sizeof(val), bar0_info.offset + E1000_RAL0);
> +	ASSERT_EQ(-1, ret_bytes);
> +	ASSERT_EQ(EIO, errno);

[Severity: Medium]
Will these pread() and pwrite() assertions fail in the direct fixture
variant?

In the guest_memfd variant, self->bar_fd is self->gmem_fd, and
vfio_pci_core_gmem_rw() rejects the access with -EIO because
vdev->bars_available is false.  In the direct variant, however,
self->bar_fd is self->vfio_fd, and VFIO_DEVICE_FEATURE_LOW_POWER_ENTRY
wakes the device on any descriptor access through the VFIO uAPI:

drivers/vfio/pci/vfio_pci_core.c:vfio_pci_rw() {
	...
	ret = pm_runtime_resume_and_get(&vdev->pdev->dev);
	if (ret) {
		pci_info_ratelimited(vdev->pdev, "runtime resume failed %d\n",
				     ret);
		return -EIO;
	}

	switch (index) {
	...
	case VFIO_PCI_BAR0_REGION_INDEX ... VFIO_PCI_BAR5_REGION_INDEX:
		ret = vfio_pci_bar_rw(vdev, buf, count, ppos, iswrite);
		break;
}

Because vfio_pci_rw() resumes the device to PCI_D0 before calling
vfio_pci_bar_rw(), and __vfio_pci_memory_enabled() only checks
pdev->current_state < PCI_D3hot and PCI_COMMAND_MEMORY, pread() and
pwrite() succeed and return sizeof(val) instead of failing with EIO.
Should the pread()/pwrite() EIO checks be gated on variant->use_gmem, or
should D3hot be entered via PCI_PM_CTRL in config space so both variants
stay in D3hot across descriptor accesses?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-gmem-alternative-providers-v3-0-8484dde66a7e@google.com?part=79