[RFC PATCH 0/5] virtio: obtain SHM page size from device

[RFC PATCH 0/5] virtio: obtain SHM page size from device

[Sergio Lopez]

There's an incresing number of machines supporting multiple page sizes
and, on these machines, the host and a guest can be running with
different pages sizes.

In addition to this, there might be devices that have a required and/or
preferred page size for mapping memory.

In this series, we extend virtio_shm_region with a field to hold the
page size. This field has a 16-bit size to accommodate into the existing
padding virtio_pci_cap, simplifying the introduction of this additional
data into the structure. The device will provide the page size in format
PAGE_SIZE >> 12.

The series also extends the PCI and MMIO transports to obtain the
corresponding value from the device. For the PCI one, it should be safe
since we're using an existing 16-bit padding in the virtio_pci_cap
struct. For MMIO, we need to access a new register, so there's a risk
the VMM may overreact and crash the VM. I've checked libkrun,
firecracker, cloud-hypervisor and crosvm, and all of them should deal
with the unexpected MMIO read gracefully. QEMU doesn't support SHM for
the MMIO transport, so that isn't a concern either.

How the SHM page size information is used depends on each device. Some
may silently round up allocations, some may expose this information to
userspace. This series includes a patch that extends virtio-gpu to
expose the information via the VIRTGPU_GETPARAM ioctl, as an example of
the second approach.

This patch series is an RFC because it requires changes to the VIRTIO
specifications. This patch series will be used as a reference to
propose such changes.

Signed-off-by: Sergio Lopez <slp@redhat.com>
---
Sergio Lopez (5):
      virtio_config: add page_size field to virtio_shm_region
      virtio: introduce VIRTIO_F_SHM_PAGE_SIZE
      virtio-pci: extend virtio_pci_cap to hold page_size
      virtio-mmio: read shm region page size
      drm/virtio: add VIRTGPU_PARAM_HOST_SHM_PAGE_SIZE to params

 drivers/gpu/drm/virtio/virtgpu_ioctl.c |  5 +++++
 drivers/virtio/virtio_mmio.c           | 13 +++++++++++++
 drivers/virtio/virtio_pci_modern.c     | 31 ++++++++++++++++++++++++++++---
 drivers/virtio/virtio_ring.c           |  2 ++
 include/linux/virtio_config.h          |  1 +
 include/uapi/drm/virtgpu_drm.h         |  1 +
 include/uapi/linux/virtio_config.h     |  7 ++++++-
 include/uapi/linux/virtio_mmio.h       |  3 +++
 include/uapi/linux/virtio_pci.h        |  2 +-
 9 files changed, 60 insertions(+), 5 deletions(-)
---
base-commit: 4dc1d1bec89864d8076e5ab314f86f46442bfb02
change-id: 20250213-virtio-shm-page-size-6e9a08c7ded1

Best regards,
-- 
Sergio Lopez <slp@redhat.com>

[PATCH RFC 1/5] virtio_config: add page_size field to virtio_shm_region

[Sergio Lopez]

Add a page_size field to virtio_shm_region to store page size supported
by the device. This is required to support running VMs with a page size
smaller than the host, among other potential scenarios.

Signed-off-by: Sergio Lopez <slp@redhat.com>
---
 drivers/virtio/virtio_mmio.c       | 2 ++
 drivers/virtio/virtio_pci_modern.c | 1 +
 include/linux/virtio_config.h      | 1 +
 3 files changed, 4 insertions(+)

diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 5d78c2d572abfcfe2b84cdd82df622320fe97d5d..cd0a0407659517e6a318b117ba67258c59f1f983 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -560,6 +560,8 @@ static bool vm_get_shm_region(struct virtio_device *vdev,
 
 	region->addr = addr;
 
+	region->page_size = 4096 >> 12;
+
 	return true;
 }
 
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 5eaade7578606e4b02af0d66447417ad6aa11064..4e6c28aacd1b5dfc117337a689a25d668805e334 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -862,6 +862,7 @@ static bool vp_get_shm_region(struct virtio_device *vdev,
 
 	region->len = len;
 	region->addr = (u64) phys_addr + offset;
+	region->page_size = 4096 >> 12;
 
 	return true;
 }
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 169c7d367facb36dcabf9596068580ea8b8516c7..21ada8fcdf655894bb725045f54c9db3a1492b13 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -14,6 +14,7 @@ struct irq_affinity;
 struct virtio_shm_region {
 	u64 addr;
 	u64 len;
+	u16 page_size; /* PAGE_SIZE >> 12 */
 };
 
 typedef void vq_callback_t(struct virtqueue *);

-- 
2.48.1

[PATCH RFC 2/5] virtio: introduce VIRTIO_F_SHM_PAGE_SIZE

[Sergio Lopez]

Introduce VIRTIO_F_SHM_PAGE_SIZE, a feature bit which indicates that the
transport provides the page size for SHM regions.

Signed-off-by: Sergio Lopez <slp@redhat.com>
---
 drivers/virtio/virtio_pci_modern.c | 3 +++
 drivers/virtio/virtio_ring.c       | 2 ++
 include/uapi/linux/virtio_config.h | 7 ++++++-
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 4e6c28aacd1b5dfc117337a689a25d668805e334..79616ce5057bf3b2b88cae7e8fb7729efa9dd632 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -367,6 +367,9 @@ static void vp_transport_features(struct virtio_device *vdev, u64 features)
 
 	if (features & BIT_ULL(VIRTIO_F_ADMIN_VQ))
 		__virtio_set_bit(vdev, VIRTIO_F_ADMIN_VQ);
+
+	if (features & BIT_ULL(VIRTIO_F_SHM_PAGE_SIZE))
+		__virtio_set_bit(vdev, VIRTIO_F_SHM_PAGE_SIZE);
 }
 
 static int __vp_check_common_size_one_feature(struct virtio_device *vdev, u32 fbit,
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index fdd2d2b07b5a2aca864bc917306536685afb66a6..d853d5cf7e553be8bbe0ff461dc7312b258c6e58 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2983,6 +2983,8 @@ void vring_transport_features(struct virtio_device *vdev)
 			break;
 		case VIRTIO_F_NOTIFICATION_DATA:
 			break;
+		case VIRTIO_F_SHM_PAGE_SIZE:
+			break;
 		default:
 			/* We don't understand this bit. */
 			__virtio_clear_bit(vdev, i);
diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
index 2445f365bce74b4e926c6929322b269252ab6830..3171e3792263c2cd472ac09da18e593ab400751d 100644
--- a/include/uapi/linux/virtio_config.h
+++ b/include/uapi/linux/virtio_config.h
@@ -52,7 +52,7 @@
  * rest are per-device feature bits.
  */
 #define VIRTIO_TRANSPORT_F_START	28
-#define VIRTIO_TRANSPORT_F_END		42
+#define VIRTIO_TRANSPORT_F_END		43
 
 #ifndef VIRTIO_CONFIG_NO_LEGACY
 /* Do we get callbacks when the ring is completely used, even if we've
@@ -120,4 +120,9 @@
  */
 #define VIRTIO_F_ADMIN_VQ		41
 
+/*
+ * This feature indicates that the transport provides the SHM page size.
+ */
+#define VIRTIO_F_SHM_PAGE_SIZE		42
+
 #endif /* _UAPI_LINUX_VIRTIO_CONFIG_H */

-- 
2.48.1

[PATCH RFC 3/5] virtio-pci: extend virtio_pci_cap to hold page_size

[Sergio Lopez]

Turn the 16 bit padding into a page_size field to allow the device to
pass its required page size with format PAGE_SIZE >> 12.

Signed-off-by: Sergio Lopez <slp@redhat.com>
---
 drivers/virtio/virtio_pci_modern.c | 29 +++++++++++++++++++++++++----
 include/uapi/linux/virtio_pci.h    |  2 +-
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 79616ce5057bf3b2b88cae7e8fb7729efa9dd632..26e9cd5148c0f10209c34d12e65d64490a855d75 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -770,14 +770,16 @@ static void del_vq(struct virtio_pci_vq_info *info)
 	vring_del_virtqueue(vq);
 }
 
-static int virtio_pci_find_shm_cap(struct pci_dev *dev, u8 required_id,
-				   u8 *bar, u64 *offset, u64 *len)
+static int virtio_pci_find_shm_cap(struct virtio_device *vdev, struct pci_dev *dev,
+				   u8 required_id, u8 *bar, u64 *offset, u64 *len,
+				   u16 *page_size)
 {
 	int pos;
 
 	for (pos = pci_find_capability(dev, PCI_CAP_ID_VNDR); pos > 0;
 	     pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) {
 		u8 type, cap_len, id, res_bar;
+		u16 res_psize = 0;
 		u32 tmp32;
 		u64 res_offset, res_length;
 
@@ -808,6 +810,23 @@ static int virtio_pci_find_shm_cap(struct pci_dev *dev, u8 required_id,
 		 * Looks good.
 		 */
 
+		if (__virtio_test_bit(vdev, VIRTIO_F_SHM_PAGE_SIZE)) {
+			pci_read_config_word(dev, pos + offsetof(struct virtio_pci_cap,
+								 page_size), &res_psize);
+			if (!res_psize) {
+				dev_err(&dev->dev, "%s: shm cap with invalid page size on "
+					"a device with VIRTIO_F_SHM_PAGE_SIZE feature\n",
+					__func__);
+				res_psize = 4096 >> 12;
+			}
+		}
+
+		/* For backwards compatibility, if the device didn't specify a
+		 * page size, assume it to be 4096.
+		 */
+		if (!res_psize)
+			res_psize = 4096 >> 12;
+
 		/* Read the lower 32bit of length and offset */
 		pci_read_config_dword(dev, pos + offsetof(struct virtio_pci_cap,
 							  offset), &tmp32);
@@ -829,6 +848,7 @@ static int virtio_pci_find_shm_cap(struct pci_dev *dev, u8 required_id,
 		*bar = res_bar;
 		*offset = res_offset;
 		*len = res_length;
+		*page_size = res_psize;
 
 		return pos;
 	}
@@ -841,11 +861,12 @@ static bool vp_get_shm_region(struct virtio_device *vdev,
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	struct pci_dev *pci_dev = vp_dev->pci_dev;
 	u8 bar;
+	u16 page_size;
 	u64 offset, len;
 	phys_addr_t phys_addr;
 	size_t bar_len;
 
-	if (!virtio_pci_find_shm_cap(pci_dev, id, &bar, &offset, &len))
+	if (!virtio_pci_find_shm_cap(vdev, pci_dev, id, &bar, &offset, &len, &page_size))
 		return false;
 
 	phys_addr = pci_resource_start(pci_dev, bar);
@@ -865,7 +886,7 @@ static bool vp_get_shm_region(struct virtio_device *vdev,
 
 	region->len = len;
 	region->addr = (u64) phys_addr + offset;
-	region->page_size = 4096 >> 12;
+	region->page_size = page_size;
 
 	return true;
 }
diff --git a/include/uapi/linux/virtio_pci.h b/include/uapi/linux/virtio_pci.h
index 8549d4571257142ac6c9dad5c01369923791a85a..fb0ccb7a125d8178c1f78333c4d2f43540e1764b 100644
--- a/include/uapi/linux/virtio_pci.h
+++ b/include/uapi/linux/virtio_pci.h
@@ -127,7 +127,7 @@ struct virtio_pci_cap {
 	__u8 cfg_type;		/* Identifies the structure. */
 	__u8 bar;		/* Where to find it. */
 	__u8 id;		/* Multiple capabilities of the same type */
-	__u8 padding[2];	/* Pad to full dword. */
+	__u16 page_size;	/* Device page size (PAGE_SIZE >> 12). */
 	__le32 offset;		/* Offset within bar. */
 	__le32 length;		/* Length of the structure, in bytes. */
 };

-- 
2.48.1

[PATCH RFC 4/5] virtio-mmio: read shm region page size

[Sergio Lopez]

Use the newly introduced SHM_PAGE_SIZE register to read the page size
and store it into virtio_shm_region.

Signed-off-by: Sergio Lopez <slp@redhat.com>
---
 drivers/virtio/virtio_mmio.c     | 13 ++++++++++++-
 include/uapi/linux/virtio_mmio.h |  3 +++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index cd0a0407659517e6a318b117ba67258c59f1f983..92590c77901bee453d8c1a1ac60ad9cca90d1b59 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -537,6 +537,7 @@ static bool vm_get_shm_region(struct virtio_device *vdev,
 			      struct virtio_shm_region *region, u8 id)
 {
 	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
+	u16 page_size = 0;
 	u64 len, addr;
 
 	/* Select the region we're interested in */
@@ -560,7 +561,17 @@ static bool vm_get_shm_region(struct virtio_device *vdev,
 
 	region->addr = addr;
 
-	region->page_size = 4096 >> 12;
+	/* If supported by the device transport, read the region page size */
+	if (__virtio_test_bit(vdev, VIRTIO_F_SHM_PAGE_SIZE))
+		page_size = (u16) readl(vm_dev->base + VIRTIO_MMIO_SHM_PAGE_SIZE);
+
+	/* For backwards compatibility, if the device didn't specify a
+	 * page size, assume it to be 4096.
+	 */
+	if (page_size == 0)
+		page_size = 4096 >> 12;
+
+	region->page_size = page_size;
 
 	return true;
 }
diff --git a/include/uapi/linux/virtio_mmio.h b/include/uapi/linux/virtio_mmio.h
index 0650f91bea6c70f935764070d825d181a2379afb..c5a1cdad5a1c9bc954d7facfd8c05e2acbb28e96 100644
--- a/include/uapi/linux/virtio_mmio.h
+++ b/include/uapi/linux/virtio_mmio.h
@@ -133,6 +133,9 @@
 #define VIRTIO_MMIO_SHM_BASE_LOW        0x0b8
 #define VIRTIO_MMIO_SHM_BASE_HIGH       0x0bc
 
+/* Shared memory region page size, in format PAGE_SIZE >> 12 */
+#define VIRTIO_MMIO_SHM_PAGE_SIZE       0x0c4
+
 /* Configuration atomicity value */
 #define VIRTIO_MMIO_CONFIG_GENERATION	0x0fc
 

-- 
2.48.1

[PATCH RFC 5/5] drm/virtio: add VIRTGPU_PARAM_HOST_SHM_PAGE_SIZE to params

[Sergio Lopez]

Add VIRTGPU_PARAM_HOST_SHM_PAGE_SIZE as a param that can be read with
VIRTGPU_GETPARAM by userspace applications running in the guest to
obtain the host's page size and find out the right alignment to be used
in shared memory allocations.

Signed-off-by: Sergio Lopez <slp@redhat.com>
---
 drivers/gpu/drm/virtio/virtgpu_ioctl.c | 5 +++++
 include/uapi/drm/virtgpu_drm.h         | 1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index c33c057365f85a2ace536f91655c903036827312..4b49635b4fe1d4256f219823341cef8e5fa8f029 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -117,6 +117,11 @@ static int virtio_gpu_getparam_ioctl(struct drm_device *dev, void *data,
 	case VIRTGPU_PARAM_EXPLICIT_DEBUG_NAME:
 		value = vgdev->has_context_init ? 1 : 0;
 		break;
+	case VIRTGPU_PARAM_HOST_SHM_PAGE_SIZE:
+		if (!vgdev->has_host_visible)
+			return -EINVAL;
+		value = vgdev->host_visible_region.page_size;
+		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/include/uapi/drm/virtgpu_drm.h b/include/uapi/drm/virtgpu_drm.h
index c2ce71987e9bb816d13a300679336cb756f1cbcf..72db6b3339e0dcaf550acbf5ac4381a6e5c2216d 100644
--- a/include/uapi/drm/virtgpu_drm.h
+++ b/include/uapi/drm/virtgpu_drm.h
@@ -98,6 +98,7 @@ struct drm_virtgpu_execbuffer {
 #define VIRTGPU_PARAM_CONTEXT_INIT 6 /* DRM_VIRTGPU_CONTEXT_INIT */
 #define VIRTGPU_PARAM_SUPPORTED_CAPSET_IDs 7 /* Bitmask of supported capability set ids */
 #define VIRTGPU_PARAM_EXPLICIT_DEBUG_NAME 8 /* Ability to set debug name from userspace */
+#define VIRTGPU_PARAM_HOST_SHM_PAGE_SIZE 9 /* Host SHM page size, with format PAGE_SIZE >> 12 */
 
 struct drm_virtgpu_getparam {
 	__u64 param;

-- 
2.48.1

Re: [RFC PATCH 0/5] virtio: obtain SHM page size from device

[Michael S. Tsirkin]

On Thu, Feb 13, 2025 at 04:49:14PM +0100, Sergio Lopez wrote:
> There's an incresing number of machines supporting multiple page sizes
> and, on these machines, the host and a guest can be running with
> different pages sizes.
> 
> In addition to this, there might be devices that have a required and/or
> preferred page size for mapping memory.
> 
> In this series, we extend virtio_shm_region with a field to hold the
> page size. This field has a 16-bit size to accommodate into the existing
> padding virtio_pci_cap, simplifying the introduction of this additional
> data into the structure. The device will provide the page size in format
> PAGE_SIZE >> 12.
> 
> The series also extends the PCI and MMIO transports to obtain the
> corresponding value from the device. For the PCI one, it should be safe
> since we're using an existing 16-bit padding in the virtio_pci_cap
> struct. For MMIO, we need to access a new register, so there's a risk
> the VMM may overreact and crash the VM. I've checked libkrun,
> firecracker, cloud-hypervisor and crosvm, and all of them should deal
> with the unexpected MMIO read gracefully. QEMU doesn't support SHM for
> the MMIO transport, so that isn't a concern either.
> 
> How the SHM page size information is used depends on each device. Some
> may silently round up allocations, some may expose this information to
> userspace. This series includes a patch that extends virtio-gpu to
> expose the information via the VIRTGPU_GETPARAM ioctl, as an example of
> the second approach.
> 
> This patch series is an RFC because it requires changes to the VIRTIO
> specifications. This patch series will be used as a reference to
> propose such changes.
> 
> Signed-off-by: Sergio Lopez <slp@redhat.com>


don't you want to negotiate the page size with the
driver then?

> ---
> Sergio Lopez (5):
>       virtio_config: add page_size field to virtio_shm_region
>       virtio: introduce VIRTIO_F_SHM_PAGE_SIZE
>       virtio-pci: extend virtio_pci_cap to hold page_size
>       virtio-mmio: read shm region page size
>       drm/virtio: add VIRTGPU_PARAM_HOST_SHM_PAGE_SIZE to params
> 
>  drivers/gpu/drm/virtio/virtgpu_ioctl.c |  5 +++++
>  drivers/virtio/virtio_mmio.c           | 13 +++++++++++++
>  drivers/virtio/virtio_pci_modern.c     | 31 ++++++++++++++++++++++++++++---
>  drivers/virtio/virtio_ring.c           |  2 ++
>  include/linux/virtio_config.h          |  1 +
>  include/uapi/drm/virtgpu_drm.h         |  1 +
>  include/uapi/linux/virtio_config.h     |  7 ++++++-
>  include/uapi/linux/virtio_mmio.h       |  3 +++
>  include/uapi/linux/virtio_pci.h        |  2 +-
>  9 files changed, 60 insertions(+), 5 deletions(-)
> ---
> base-commit: 4dc1d1bec89864d8076e5ab314f86f46442bfb02
> change-id: 20250213-virtio-shm-page-size-6e9a08c7ded1
> 
> Best regards,
> -- 
> Sergio Lopez <slp@redhat.com>

Re: [PATCH RFC 3/5] virtio-pci: extend virtio_pci_cap to hold page_size

[Daniel Verkamp]

On Thu, Feb 13, 2025 at 7:54 AM Sergio Lopez <slp@redhat.com> wrote:
>
> Turn the 16 bit padding into a page_size field to allow the device to
> pass its required page size with format PAGE_SIZE >> 12.
>
> Signed-off-by: Sergio Lopez <slp@redhat.com>
> ---
>  drivers/virtio/virtio_pci_modern.c | 29 +++++++++++++++++++++++++----
>  include/uapi/linux/virtio_pci.h    |  2 +-
>  2 files changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index 79616ce5057bf3b2b88cae7e8fb7729efa9dd632..26e9cd5148c0f10209c34d12e65d64490a855d75 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
[...]
> +               if (__virtio_test_bit(vdev, VIRTIO_F_SHM_PAGE_SIZE)) {
> +                       pci_read_config_word(dev, pos + offsetof(struct virtio_pci_cap,
> +                                                                page_size), &res_psize);
> +                       if (!res_psize) {
> +                               dev_err(&dev->dev, "%s: shm cap with invalid page size on "
> +                                       "a device with VIRTIO_F_SHM_PAGE_SIZE feature\n",
> +                                       __func__);

Maybe this should also constrain the page size to be a power of 2?

[...]
> diff --git a/include/uapi/linux/virtio_pci.h b/include/uapi/linux/virtio_pci.h
> index 8549d4571257142ac6c9dad5c01369923791a85a..fb0ccb7a125d8178c1f78333c4d2f43540e1764b 100644
> --- a/include/uapi/linux/virtio_pci.h
> +++ b/include/uapi/linux/virtio_pci.h
> @@ -127,7 +127,7 @@ struct virtio_pci_cap {
>         __u8 cfg_type;          /* Identifies the structure. */
>         __u8 bar;               /* Where to find it. */
>         __u8 id;                /* Multiple capabilities of the same type */
> -       __u8 padding[2];        /* Pad to full dword. */
> +       __u16 page_size;        /* Device page size (PAGE_SIZE >> 12). */

This comment should probably clarify that the page_size field is only
valid when cfg_type is VIRTIO_PCI_CAP_SHARED_MEMORY_CFG, assuming
that's the case. Or should the device be expected to provide the
page_size for all capabilities regardless of type?

It seems like the name should also ideally make it clearer that this
is page_size/4096 rather than the actual page size to avoid confusing
device implementers.

>         __le32 offset;          /* Offset within bar. */
>         __le32 length;          /* Length of the structure, in bytes. */
>  };

Re: [PATCH RFC 3/5] virtio-pci: extend virtio_pci_cap to hold page_size

[Daniel Verkamp]

On Thu, Feb 13, 2025 at 11:22 AM Daniel Verkamp <dverkamp@chromium.org> wrote:
>
> On Thu, Feb 13, 2025 at 7:54 AM Sergio Lopez <slp@redhat.com> wrote:
> >
> > Turn the 16 bit padding into a page_size field to allow the device to
> > pass its required page size with format PAGE_SIZE >> 12.
> >
> > Signed-off-by: Sergio Lopez <slp@redhat.com>
> > ---
> >  drivers/virtio/virtio_pci_modern.c | 29 +++++++++++++++++++++++++----
> >  include/uapi/linux/virtio_pci.h    |  2 +-
> >  2 files changed, 26 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> > index 79616ce5057bf3b2b88cae7e8fb7729efa9dd632..26e9cd5148c0f10209c34d12e65d64490a855d75 100644
> > --- a/drivers/virtio/virtio_pci_modern.c
> > +++ b/drivers/virtio/virtio_pci_modern.c
> [...]
> > +               if (__virtio_test_bit(vdev, VIRTIO_F_SHM_PAGE_SIZE)) {
> > +                       pci_read_config_word(dev, pos + offsetof(struct virtio_pci_cap,
> > +                                                                page_size), &res_psize);
> > +                       if (!res_psize) {
> > +                               dev_err(&dev->dev, "%s: shm cap with invalid page size on "
> > +                                       "a device with VIRTIO_F_SHM_PAGE_SIZE feature\n",
> > +                                       __func__);
>
> Maybe this should also constrain the page size to be a power of 2?
>
> [...]
> > diff --git a/include/uapi/linux/virtio_pci.h b/include/uapi/linux/virtio_pci.h
> > index 8549d4571257142ac6c9dad5c01369923791a85a..fb0ccb7a125d8178c1f78333c4d2f43540e1764b 100644
> > --- a/include/uapi/linux/virtio_pci.h
> > +++ b/include/uapi/linux/virtio_pci.h
> > @@ -127,7 +127,7 @@ struct virtio_pci_cap {
> >         __u8 cfg_type;          /* Identifies the structure. */
> >         __u8 bar;               /* Where to find it. */
> >         __u8 id;                /* Multiple capabilities of the same type */
> > -       __u8 padding[2];        /* Pad to full dword. */
> > +       __u16 page_size;        /* Device page size (PAGE_SIZE >> 12). */
>
> This comment should probably clarify that the page_size field is only
> valid when cfg_type is VIRTIO_PCI_CAP_SHARED_MEMORY_CFG, assuming
> that's the case. Or should the device be expected to provide the
> page_size for all capabilities regardless of type?
>
> It seems like the name should also ideally make it clearer that this
> is page_size/4096 rather than the actual page size to avoid confusing
> device implementers.

Alternatively, this could be represented as a single u8 page_shift,
where page_size = 1 << (page_shift + 12), and then existing devices
would "just work" (assuming they filled the padding with 0). It is
probably still a good idea to guard it with a feature bit, though, so
it doesn't really make that much difference.

Thanks,
-- Daniel

Re: [PATCH RFC 3/5] virtio-pci: extend virtio_pci_cap to hold page_size

[Sergio Lopez Pascual]

Daniel Verkamp <dverkamp@chromium.org> writes:

> On Thu, Feb 13, 2025 at 7:54 AM Sergio Lopez <slp@redhat.com> wrote:
>>
>> Turn the 16 bit padding into a page_size field to allow the device to
>> pass its required page size with format PAGE_SIZE >> 12.
>>
>> Signed-off-by: Sergio Lopez <slp@redhat.com>
>> ---
>>  drivers/virtio/virtio_pci_modern.c | 29 +++++++++++++++++++++++++----
>>  include/uapi/linux/virtio_pci.h    |  2 +-
>>  2 files changed, 26 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
>> index 79616ce5057bf3b2b88cae7e8fb7729efa9dd632..26e9cd5148c0f10209c34d12e65d64490a855d75 100644
>> --- a/drivers/virtio/virtio_pci_modern.c
>> +++ b/drivers/virtio/virtio_pci_modern.c
> [...]
>> +               if (__virtio_test_bit(vdev, VIRTIO_F_SHM_PAGE_SIZE)) {
>> +                       pci_read_config_word(dev, pos + offsetof(struct virtio_pci_cap,
>> +                                                                page_size), &res_psize);
>> +                       if (!res_psize) {
>> +                               dev_err(&dev->dev, "%s: shm cap with invalid page size on "
>> +                                       "a device with VIRTIO_F_SHM_PAGE_SIZE feature\n",
>> +                                       __func__);
>
> Maybe this should also constrain the page size to be a power of 2?
>
> [...]
>> diff --git a/include/uapi/linux/virtio_pci.h b/include/uapi/linux/virtio_pci.h
>> index 8549d4571257142ac6c9dad5c01369923791a85a..fb0ccb7a125d8178c1f78333c4d2f43540e1764b 100644
>> --- a/include/uapi/linux/virtio_pci.h
>> +++ b/include/uapi/linux/virtio_pci.h
>> @@ -127,7 +127,7 @@ struct virtio_pci_cap {
>>         __u8 cfg_type;          /* Identifies the structure. */
>>         __u8 bar;               /* Where to find it. */
>>         __u8 id;                /* Multiple capabilities of the same type */
>> -       __u8 padding[2];        /* Pad to full dword. */
>> +       __u16 page_size;        /* Device page size (PAGE_SIZE >> 12). */
>
> This comment should probably clarify that the page_size field is only
> valid when cfg_type is VIRTIO_PCI_CAP_SHARED_MEMORY_CFG, assuming
> that's the case. Or should the device be expected to provide the
> page_size for all capabilities regardless of type?

Only for VIRTIO_PCI_CAP_SHARED_MEMORY_CFG, I'll clarify that in the
comment.

> It seems like the name should also ideally make it clearer that this
> is page_size/4096 rather than the actual page size to avoid confusing
> device implementers.

I like your suggestion in the other email of turning this into
page_shift, which clarifies the field contents and also addresses the
need check the constrains you highlighted above. I'll do that in v2.

Thanks!
Sergio.

Re: [PATCH RFC 3/5] virtio-pci: extend virtio_pci_cap to hold page_size

[Sergio Lopez Pascual]

Daniel Verkamp <dverkamp@chromium.org> writes:

> On Thu, Feb 13, 2025 at 11:22 AM Daniel Verkamp <dverkamp@chromium.org> wrote:
>>
>> On Thu, Feb 13, 2025 at 7:54 AM Sergio Lopez <slp@redhat.com> wrote:
>> >
>> > Turn the 16 bit padding into a page_size field to allow the device to
>> > pass its required page size with format PAGE_SIZE >> 12.
>> >
>> > Signed-off-by: Sergio Lopez <slp@redhat.com>
>> > ---
>> >  drivers/virtio/virtio_pci_modern.c | 29 +++++++++++++++++++++++++----
>> >  include/uapi/linux/virtio_pci.h    |  2 +-
>> >  2 files changed, 26 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
>> > index 79616ce5057bf3b2b88cae7e8fb7729efa9dd632..26e9cd5148c0f10209c34d12e65d64490a855d75 100644
>> > --- a/drivers/virtio/virtio_pci_modern.c
>> > +++ b/drivers/virtio/virtio_pci_modern.c
>> [...]
>> > +               if (__virtio_test_bit(vdev, VIRTIO_F_SHM_PAGE_SIZE)) {
>> > +                       pci_read_config_word(dev, pos + offsetof(struct virtio_pci_cap,
>> > +                                                                page_size), &res_psize);
>> > +                       if (!res_psize) {
>> > +                               dev_err(&dev->dev, "%s: shm cap with invalid page size on "
>> > +                                       "a device with VIRTIO_F_SHM_PAGE_SIZE feature\n",
>> > +                                       __func__);
>>
>> Maybe this should also constrain the page size to be a power of 2?
>>
>> [...]
>> > diff --git a/include/uapi/linux/virtio_pci.h b/include/uapi/linux/virtio_pci.h
>> > index 8549d4571257142ac6c9dad5c01369923791a85a..fb0ccb7a125d8178c1f78333c4d2f43540e1764b 100644
>> > --- a/include/uapi/linux/virtio_pci.h
>> > +++ b/include/uapi/linux/virtio_pci.h
>> > @@ -127,7 +127,7 @@ struct virtio_pci_cap {
>> >         __u8 cfg_type;          /* Identifies the structure. */
>> >         __u8 bar;               /* Where to find it. */
>> >         __u8 id;                /* Multiple capabilities of the same type */
>> > -       __u8 padding[2];        /* Pad to full dword. */
>> > +       __u16 page_size;        /* Device page size (PAGE_SIZE >> 12). */
>>
>> This comment should probably clarify that the page_size field is only
>> valid when cfg_type is VIRTIO_PCI_CAP_SHARED_MEMORY_CFG, assuming
>> that's the case. Or should the device be expected to provide the
>> page_size for all capabilities regardless of type?
>>
>> It seems like the name should also ideally make it clearer that this
>> is page_size/4096 rather than the actual page size to avoid confusing
>> device implementers.
>
> Alternatively, this could be represented as a single u8 page_shift,
> where page_size = 1 << (page_shift + 12), and then existing devices
> would "just work" (assuming they filled the padding with 0). It is
> probably still a good idea to guard it with a feature bit, though, so
> it doesn't really make that much difference.

I actually considered not using a feature bit and just behave like this,
but then I've noticed that, for the mmio transport, it would mean
reading from a new register. Some VMMs may overreact to the unexpected
read and crash or, even if they don't, it's not guaranteed they aren't
going to return garbage in response. So I think it's better being a bit
conservative and protect the behavior behind a feature bit.

Thanks,
Sergio.

Re: [RFC PATCH 0/5] virtio: obtain SHM page size from device

[Sergio Lopez Pascual]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Thu, Feb 13, 2025 at 04:49:14PM +0100, Sergio Lopez wrote:
>> There's an incresing number of machines supporting multiple page sizes
>> and, on these machines, the host and a guest can be running with
>> different pages sizes.
>>
>> In addition to this, there might be devices that have a required and/or
>> preferred page size for mapping memory.
>>
>> In this series, we extend virtio_shm_region with a field to hold the
>> page size. This field has a 16-bit size to accommodate into the existing
>> padding virtio_pci_cap, simplifying the introduction of this additional
>> data into the structure. The device will provide the page size in format
>> PAGE_SIZE >> 12.
>>
>> The series also extends the PCI and MMIO transports to obtain the
>> corresponding value from the device. For the PCI one, it should be safe
>> since we're using an existing 16-bit padding in the virtio_pci_cap
>> struct. For MMIO, we need to access a new register, so there's a risk
>> the VMM may overreact and crash the VM. I've checked libkrun,
>> firecracker, cloud-hypervisor and crosvm, and all of them should deal
>> with the unexpected MMIO read gracefully. QEMU doesn't support SHM for
>> the MMIO transport, so that isn't a concern either.
>>
>> How the SHM page size information is used depends on each device. Some
>> may silently round up allocations, some may expose this information to
>> userspace. This series includes a patch that extends virtio-gpu to
>> expose the information via the VIRTGPU_GETPARAM ioctl, as an example of
>> the second approach.
>>
>> This patch series is an RFC because it requires changes to the VIRTIO
>> specifications. This patch series will be used as a reference to
>> propose such changes.
>>
>> Signed-off-by: Sergio Lopez <slp@redhat.com>
>
>
> don't you want to negotiate the page size with the
> driver then?

It's not really a negotiation. If the device presents the feature, the
driver must honor the page size, either directly by rejecting or
rounding up allocations, indirectly by informing userspace, or both.

If the driver can't accomodate to the page size, it must refrain from
using the Shared Memory Region.

Thanks,
Sergio.