[PATCH 0/2] drm/virtio: introduce the HOST_PAGE_SIZE feature

[PATCH 0/2] drm/virtio: introduce the HOST_PAGE_SIZE feature

[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, each one,
with a different page size.

For what pertains to virtio-gpu, this is not a problem if the page size
of the guest happens to be bigger or equal than the host, but will
potentially lead to failures in memory allocations and/or mappings
otherwise.

To improve this situation, we introduce here the HOST_PAGE_SIZE feature.
This feature indicates that the host has an extended virtio_gpu_config
structure that include it's own page size a new field.

On the second commit, we also add a new 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.

Sergio Lopez (2):
  drm/virtio: introduce the HOST_PAGE_SIZE feature
  drm/virtio: add VIRTGPU_PARAM_HOST_PAGE_SIZE to params

 drivers/gpu/drm/virtio/virtgpu_drv.c   |  1 +
 drivers/gpu/drm/virtio/virtgpu_drv.h   |  2 ++
 drivers/gpu/drm/virtio/virtgpu_ioctl.c |  5 +++++
 drivers/gpu/drm/virtio/virtgpu_kms.c   | 13 ++++++++++---
 include/uapi/drm/virtgpu_drm.h         |  1 +
 include/uapi/linux/virtio_gpu.h        |  5 +++++
 6 files changed, 24 insertions(+), 3 deletions(-)

-- 
2.45.2

[PATCH 1/2] drm/virtio: introduce the HOST_PAGE_SIZE feature

[Sergio Lopez]

Introduce a new feature, HOST_PAGE_SIZE, that indicates the host
provides its page size as a value in virtio_gpu_config.

Signed-off-by: Sergio Lopez <slp@redhat.com>
---
 drivers/gpu/drm/virtio/virtgpu_drv.c |  1 +
 drivers/gpu/drm/virtio/virtgpu_drv.h |  2 ++
 drivers/gpu/drm/virtio/virtgpu_kms.c | 13 ++++++++++---
 include/uapi/linux/virtio_gpu.h      |  5 +++++
 4 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
index 188e126383c2..026fc061db6d 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -149,6 +149,7 @@ static unsigned int features[] = {
 	VIRTIO_GPU_F_RESOURCE_UUID,
 	VIRTIO_GPU_F_RESOURCE_BLOB,
 	VIRTIO_GPU_F_CONTEXT_INIT,
+	VIRTIO_GPU_F_HOST_PAGE_SIZE,
 };
 static struct virtio_driver virtio_gpu_driver = {
 	.feature_table = features,
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index bb7d86a0c6a1..5ce795deb1eb 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -249,6 +249,7 @@ struct virtio_gpu_device {
 	bool has_resource_blob;
 	bool has_host_visible;
 	bool has_context_init;
+	bool has_host_page_size;
 	struct virtio_shm_region host_visible_region;
 	struct drm_mm host_visible_mm;
 
@@ -262,6 +263,7 @@ struct virtio_gpu_device {
 	uint32_t num_capsets;
 	uint64_t capset_id_mask;
 	struct list_head cap_cache;
+	uint32_t host_page_size;
 
 	/* protects uuid state when exporting */
 	spinlock_t resource_export_lock;
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 5a3b5aaed1f3..11cccc3ca560 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -124,7 +124,7 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
 	struct virtio_gpu_device *vgdev;
 	/* this will expand later */
 	struct virtqueue *vqs[2];
-	u32 num_scanouts, num_capsets;
+	u32 num_scanouts, num_capsets, host_page_size;
 	int ret = 0;
 
 	if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
@@ -197,6 +197,12 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
 	if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_CONTEXT_INIT)) {
 		vgdev->has_context_init = true;
 	}
+	if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_HOST_PAGE_SIZE)) {
+		vgdev->has_host_page_size = true;
+		virtio_cread_le(vgdev->vdev, struct virtio_gpu_config,
+				host_page_size, &host_page_size);
+		vgdev->host_page_size = host_page_size;
+	}
 
 	DRM_INFO("features: %cvirgl %cedid %cresource_blob %chost_visible",
 		 vgdev->has_virgl_3d    ? '+' : '-',
@@ -204,8 +210,9 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
 		 vgdev->has_resource_blob ? '+' : '-',
 		 vgdev->has_host_visible ? '+' : '-');
 
-	DRM_INFO("features: %ccontext_init\n",
-		 vgdev->has_context_init ? '+' : '-');
+	DRM_INFO("features: %ccontext_init %chost_page_size\n",
+		 vgdev->has_context_init ? '+' : '-',
+		 vgdev->has_host_page_size ? '+' : '-');
 
 	ret = virtio_find_vqs(vgdev->vdev, 2, vqs, callbacks, names, NULL);
 	if (ret) {
diff --git a/include/uapi/linux/virtio_gpu.h b/include/uapi/linux/virtio_gpu.h
index 0e21f3998108..120e41bf83a5 100644
--- a/include/uapi/linux/virtio_gpu.h
+++ b/include/uapi/linux/virtio_gpu.h
@@ -64,6 +64,10 @@
  * context_init and multiple timelines
  */
 #define VIRTIO_GPU_F_CONTEXT_INIT        4
+/*
+ * Config struct contains host page size
+ */
+#define VIRTIO_GPU_F_HOST_PAGE_SIZE      5
 
 enum virtio_gpu_ctrl_type {
 	VIRTIO_GPU_UNDEFINED = 0,
@@ -363,6 +367,7 @@ struct virtio_gpu_config {
 	__le32 events_clear;
 	__le32 num_scanouts;
 	__le32 num_capsets;
+	__le32 host_page_size;
 };
 
 /* simple formats for fbcon/X use */
-- 
2.45.2

[PATCH 2/2] drm/virtio: add VIRTGPU_PARAM_HOST_PAGE_SIZE to params

[Sergio Lopez]

Add VIRTGPU_PARAM_HOST_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 e4f76f315550..c16cdf2d64b4 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_PAGE_SIZE:
+		if (!vgdev->has_host_page_size)
+			return -EINVAL;
+		value = vgdev->host_page_size;
+		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/include/uapi/drm/virtgpu_drm.h b/include/uapi/drm/virtgpu_drm.h
index c2ce71987e9b..505f87263a15 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_PAGE_SIZE 9 /* Host's page size */
 
 struct drm_virtgpu_getparam {
 	__u64 param;
-- 
2.45.2

Re: [PATCH 0/2] drm/virtio: introduce the HOST_PAGE_SIZE feature

[Dmitry Osipenko]

On 7/23/24 14:49, 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, each one,
> with a different page size.
> 
> For what pertains to virtio-gpu, this is not a problem if the page size
> of the guest happens to be bigger or equal than the host, but will
> potentially lead to failures in memory allocations and/or mappings
> otherwise.

Please describe concrete problem you're trying to solve. Guest memory
allocation consists of guest pages, I don't see how knowledge of host
page size helps anything in userspace.

I suspect you want this for host blobs, but then it should be
virtio_gpu_vram_create() that should use max(host_page_sz,
guest_page_size), AFAICT. It's kernel who is responsible for memory
management, userspace can't be trusted for doing that.

-- 
Best regards,
Dmitry

Re: [PATCH 0/2] drm/virtio: introduce the HOST_PAGE_SIZE feature

[Sergio Lopez Pascual]

Dmitry Osipenko <dmitry.osipenko@collabora.com> writes:

> On 7/23/24 14:49, 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, each one,
>> with a different page size.
>>
>> For what pertains to virtio-gpu, this is not a problem if the page size
>> of the guest happens to be bigger or equal than the host, but will
>> potentially lead to failures in memory allocations and/or mappings
>> otherwise.
>
> Please describe concrete problem you're trying to solve. Guest memory
> allocation consists of guest pages, I don't see how knowledge of host
> page size helps anything in userspace.
>
> I suspect you want this for host blobs, but then it should be
> virtio_gpu_vram_create() that should use max(host_page_sz,
> guest_page_size), AFAICT. It's kernel who is responsible for memory
> management, userspace can't be trusted for doing that.

Mesa's Vulkan/Venus uses CREATE_BLOB to request the host the creation
and mapping into the guest of device-backed memory and shmem regions.
The CREATE_BLOB ioctl doesn't update drm_virtgpu_resource_create->size,
so the guest kernel (and, as a consequence, the host kernel) can't
override the user's request.

I'd like Mesa's Vulkan/Venus in the guest to be able to obtain the host
page size to align the size of the CREATE_BLOB requests as required.

Thanks,
Sergio.

Re: [PATCH 0/2] drm/virtio: introduce the HOST_PAGE_SIZE feature

[Rob Clark]

On Wed, Jul 24, 2024 at 12:00 PM Dmitry Osipenko
<dmitry.osipenko@collabora.com> wrote:
>
> On 7/23/24 14:49, 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, each one,
> > with a different page size.
> >
> > For what pertains to virtio-gpu, this is not a problem if the page size
> > of the guest happens to be bigger or equal than the host, but will
> > potentially lead to failures in memory allocations and/or mappings
> > otherwise.
>
> Please describe concrete problem you're trying to solve. Guest memory
> allocation consists of guest pages, I don't see how knowledge of host
> page size helps anything in userspace.
>
> I suspect you want this for host blobs, but then it should be
> virtio_gpu_vram_create() that should use max(host_page_sz,
> guest_page_size), AFAICT. It's kernel who is responsible for memory
> management, userspace can't be trusted for doing that.

fwiw virtgpu native context would require this as well, mesa would
need to know the host page size to correctly align GPU VA allocations
(which must be a multiple of the host page size).

So a-b for adding this and exposing it to userspace.

BR,
-R

> --
> Best regards,
> Dmitry
>

Re: [PATCH 0/2] drm/virtio: introduce the HOST_PAGE_SIZE feature

[Rob Clark]

On Tue, Aug 6, 2024 at 9:15 AM Gurchetan Singh
<gurchetansingh@chromium.org> wrote:
>
>
>
> On Mon, Aug 5, 2024 at 2:14 AM Sergio Lopez Pascual <slp@redhat.com> wrote:
>>
>> Dmitry Osipenko <dmitry.osipenko@collabora.com> writes:
>>
>> > On 7/23/24 14:49, 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, each one,
>> >> with a different page size.
>> >>
>> >> For what pertains to virtio-gpu, this is not a problem if the page size
>> >> of the guest happens to be bigger or equal than the host, but will
>> >> potentially lead to failures in memory allocations and/or mappings
>> >> otherwise.
>> >
>> > Please describe concrete problem you're trying to solve. Guest memory
>> > allocation consists of guest pages, I don't see how knowledge of host
>> > page size helps anything in userspace.
>> >
>> > I suspect you want this for host blobs, but then it should be
>> > virtio_gpu_vram_create() that should use max(host_page_sz,
>> > guest_page_size), AFAICT. It's kernel who is responsible for memory
>> > management, userspace can't be trusted for doing that.
>>
>> Mesa's Vulkan/Venus uses CREATE_BLOB to request the host the creation
>> and mapping into the guest of device-backed memory and shmem regions.
>> The CREATE_BLOB ioctl doesn't update drm_virtgpu_resource_create->size,
>> so the guest kernel (and, as a consequence, the host kernel) can't
>> override the user's request.
>>
>> I'd like Mesa's Vulkan/Venus in the guest to be able to obtain the host
>> page size to align the size of the CREATE_BLOB requests as required.
>
>
> gfxstream solves this problem by putting the relevant information in the capabilities obtained from the host:
>
> https://android.googlesource.com/platform/hardware/google/gfxstream/+/refs/heads/main/host/virtio-gpu-gfxstream-renderer.cpp#1691
>
> If you want to be paranoid, you can also validate the ResourceCreateBlob::size is properly host-page aligned when that request reaches the host.
>
> So you can probably solve this problem using current interfaces.  Whether it's cleaner for all context types to use the capabilities, or have all VMMs to expose VIRTIO_GPU_F_HOST_PAGE_SIZE, would be the cost/benefit tradeoff.
>

I guess solving it in a context-type specific way is possible.  But I
think it is a relatively universal constraint.  And maybe it makes
sense for virtgpu guest kernel to enforce alignment (at least it can
return an error synchronously) in addition to the host.

BR,
-R

>>
>>
>> Thanks,
>> Sergio.
>>

Re: [PATCH 0/2] drm/virtio: introduce the HOST_PAGE_SIZE feature

[Sergio Lopez Pascual]

Gurchetan Singh <gurchetansingh@chromium.org> writes:

> On Tue, Aug 6, 2024 at 1:15 PM Rob Clark <robdclark@gmail.com> wrote:
>
>> On Tue, Aug 6, 2024 at 9:15 AM Gurchetan Singh
>> <gurchetansingh@chromium.org> wrote:
>> >
>> >
>> >
>> > On Mon, Aug 5, 2024 at 2:14 AM Sergio Lopez Pascual <slp@redhat.com>
>> wrote:
>> >>
>> >> Dmitry Osipenko <dmitry.osipenko@collabora.com> writes:
>> >>
>> >> > On 7/23/24 14:49, 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, each one,
>> >> >> with a different page size.
>> >> >>
>> >> >> For what pertains to virtio-gpu, this is not a problem if the page
>> size
>> >> >> of the guest happens to be bigger or equal than the host, but will
>> >> >> potentially lead to failures in memory allocations and/or mappings
>> >> >> otherwise.
>> >> >
>> >> > Please describe concrete problem you're trying to solve. Guest memory
>> >> > allocation consists of guest pages, I don't see how knowledge of host
>> >> > page size helps anything in userspace.
>> >> >
>> >> > I suspect you want this for host blobs, but then it should be
>> >> > virtio_gpu_vram_create() that should use max(host_page_sz,
>> >> > guest_page_size), AFAICT. It's kernel who is responsible for memory
>> >> > management, userspace can't be trusted for doing that.
>> >>
>> >> Mesa's Vulkan/Venus uses CREATE_BLOB to request the host the creation
>> >> and mapping into the guest of device-backed memory and shmem regions.
>> >> The CREATE_BLOB ioctl doesn't update drm_virtgpu_resource_create->size,
>> >> so the guest kernel (and, as a consequence, the host kernel) can't
>> >> override the user's request.
>> >>
>> >> I'd like Mesa's Vulkan/Venus in the guest to be able to obtain the host
>> >> page size to align the size of the CREATE_BLOB requests as required.
>> >
>> >
>> > gfxstream solves this problem by putting the relevant information in the
>> capabilities obtained from the host:
>> >
>> >
>> https://android.googlesource.com/platform/hardware/google/gfxstream/+/refs/heads/main/host/virtio-gpu-gfxstream-renderer.cpp#1691
>> >
>> > If you want to be paranoid, you can also validate the
>> ResourceCreateBlob::size is properly host-page aligned when that request
>> reaches the host.
>> >
>> > So you can probably solve this problem using current interfaces.
>> Whether it's cleaner for all context types to use the capabilities, or have
>> all VMMs to expose VIRTIO_GPU_F_HOST_PAGE_SIZE, would be the cost/benefit
>> tradeoff.
>> >
>>
>> I guess solving it in a context-type specific way is possible.  But I
>> think it is a relatively universal constraint.  And maybe it makes
>> sense for virtgpu guest kernel to enforce alignment (at least it can
>> return an error synchronously) in addition to the host.
>>
>
> virtio-media may have support for VIRTIO_MEDIA_CMD_MMAP too, so could run
> into this issue.
>
> https://github.com/chromeos/virtio-media?tab=readme-ov-file#shared-memory-regions
>
> virtio-fs also has the DAX window which uses the same memory mapping
> mechanism.
>
> https://virtio-fs.gitlab.io/design.html
>
> Maybe this should not be a virtio-gpu thing, but a virtio thing?

This is true, but finding a common place to put the page size is really
hard in practice. I don't think we can borrow space in the feature bits
for that (and that would probably be abusing its purpose quite a bit)
and extending the transport configuration registers is quite cumbersome
and, in general, undesirable.

That leaves us with the device-specific config space, and that implies a
device-specific feature bit as it's implemented in this series.

The Shared Memory Regions on the VIRTIO spec, while doesn't talk
specifically about page size, also gives us a hint about this being the
right direction:

"
2.10 Shared Memory Regions
(...)
Memory consistency rules vary depending on the region and the device
and they will be specified as required by each device."
"

Thanks,
Sergio.

Re: [PATCH 0/2] drm/virtio: introduce the HOST_PAGE_SIZE feature

[Dmitry Osipenko]

On 8/5/24 19:24, Rob Clark wrote:
> On Wed, Jul 24, 2024 at 12:00 PM Dmitry Osipenko
> <dmitry.osipenko@collabora.com> wrote:
>>
>> On 7/23/24 14:49, 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, each one,
>>> with a different page size.
>>>
>>> For what pertains to virtio-gpu, this is not a problem if the page size
>>> of the guest happens to be bigger or equal than the host, but will
>>> potentially lead to failures in memory allocations and/or mappings
>>> otherwise.
>>
>> Please describe concrete problem you're trying to solve. Guest memory
>> allocation consists of guest pages, I don't see how knowledge of host
>> page size helps anything in userspace.
>>
>> I suspect you want this for host blobs, but then it should be
>> virtio_gpu_vram_create() that should use max(host_page_sz,
>> guest_page_size), AFAICT. It's kernel who is responsible for memory
>> management, userspace can't be trusted for doing that.
> 
> fwiw virtgpu native context would require this as well, mesa would
> need to know the host page size to correctly align GPU VA allocations
> (which must be a multiple of the host page size).
> 
> So a-b for adding this and exposing it to userspace.

In general, GPU page size has no connection to the CPU page size. It
happens that MSM driver uses same page size for both GPU and CPU. Likely
you could configure a different GPU page size if you wanted. dGPUs would
often use 64k pages.

-- 
Best regards,
Dmitry

Re: [PATCH 0/2] drm/virtio: introduce the HOST_PAGE_SIZE feature

[Rob Clark]

On Thu, Aug 8, 2024 at 4:16 AM Dmitry Osipenko
<dmitry.osipenko@collabora.com> wrote:
>
> On 8/5/24 19:24, Rob Clark wrote:
> > On Wed, Jul 24, 2024 at 12:00 PM Dmitry Osipenko
> > <dmitry.osipenko@collabora.com> wrote:
> >>
> >> On 7/23/24 14:49, 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, each one,
> >>> with a different page size.
> >>>
> >>> For what pertains to virtio-gpu, this is not a problem if the page size
> >>> of the guest happens to be bigger or equal than the host, but will
> >>> potentially lead to failures in memory allocations and/or mappings
> >>> otherwise.
> >>
> >> Please describe concrete problem you're trying to solve. Guest memory
> >> allocation consists of guest pages, I don't see how knowledge of host
> >> page size helps anything in userspace.
> >>
> >> I suspect you want this for host blobs, but then it should be
> >> virtio_gpu_vram_create() that should use max(host_page_sz,
> >> guest_page_size), AFAICT. It's kernel who is responsible for memory
> >> management, userspace can't be trusted for doing that.
> >
> > fwiw virtgpu native context would require this as well, mesa would
> > need to know the host page size to correctly align GPU VA allocations
> > (which must be a multiple of the host page size).
> >
> > So a-b for adding this and exposing it to userspace.
>
> In general, GPU page size has no connection to the CPU page size. It
> happens that MSM driver uses same page size for both GPU and CPU. Likely
> you could configure a different GPU page size if you wanted. dGPUs would
> often use 64k pages.

The smmu actually supports various different page sizes (4k, 64k,
etc.. I think up to 2g), and will try to map larger contiguous sets of
pages using larger page sizes to reduce TLB pressure.  This
restriction about aligning to host page size is because the kernel
expects allocations and therefore (currently, pre-sparse) gpu mappings
to be a multiple of the host page size.

As far as whether this should be something outside of virtio-gpu, this
does feel a bit specific to how GEM buffer allocations work and how
host blob resources work.  Maybe other subsystems like media end up
with similar constraints for similar reasons, idk.  But it at least
feels like something applicable to all/most virtgpu context types.

BR,
-R

Re: [PATCH 0/2] drm/virtio: introduce the HOST_PAGE_SIZE feature

[Sergio Lopez Pascual]

Gurchetan Singh <gurchetansingh@chromium.org> writes:

> On Thu, Aug 8, 2024 at 3:38 AM Sergio Lopez Pascual <slp@redhat.com> wrote:
>
>> Gurchetan Singh <gurchetansingh@chromium.org> writes:
>>
>> > On Tue, Aug 6, 2024 at 1:15 PM Rob Clark <robdclark@gmail.com> wrote:
>> >
>> >> On Tue, Aug 6, 2024 at 9:15 AM Gurchetan Singh
>> >> <gurchetansingh@chromium.org> wrote:
>> >> >
>> >> >
>> >> >
>> >> > On Mon, Aug 5, 2024 at 2:14 AM Sergio Lopez Pascual <slp@redhat.com>
>> >> wrote:
>> >> >>
>> >> >> Dmitry Osipenko <dmitry.osipenko@collabora.com> writes:
>> >> >>
>> >> >> > On 7/23/24 14:49, 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, each
>> one,
>> >> >> >> with a different page size.
>> >> >> >>
>> >> >> >> For what pertains to virtio-gpu, this is not a problem if the page
>> >> size
>> >> >> >> of the guest happens to be bigger or equal than the host, but will
>> >> >> >> potentially lead to failures in memory allocations and/or mappings
>> >> >> >> otherwise.
>> >> >> >
>> >> >> > Please describe concrete problem you're trying to solve. Guest
>> memory
>> >> >> > allocation consists of guest pages, I don't see how knowledge of
>> host
>> >> >> > page size helps anything in userspace.
>> >> >> >
>> >> >> > I suspect you want this for host blobs, but then it should be
>> >> >> > virtio_gpu_vram_create() that should use max(host_page_sz,
>> >> >> > guest_page_size), AFAICT. It's kernel who is responsible for memory
>> >> >> > management, userspace can't be trusted for doing that.
>> >> >>
>> >> >> Mesa's Vulkan/Venus uses CREATE_BLOB to request the host the creation
>> >> >> and mapping into the guest of device-backed memory and shmem regions.
>> >> >> The CREATE_BLOB ioctl doesn't update
>> drm_virtgpu_resource_create->size,
>> >> >> so the guest kernel (and, as a consequence, the host kernel) can't
>> >> >> override the user's request.
>> >> >>
>> >> >> I'd like Mesa's Vulkan/Venus in the guest to be able to obtain the
>> host
>> >> >> page size to align the size of the CREATE_BLOB requests as required.
>> >> >
>> >> >
>> >> > gfxstream solves this problem by putting the relevant information in
>> the
>> >> capabilities obtained from the host:
>> >> >
>> >> >
>> >>
>> https://android.googlesource.com/platform/hardware/google/gfxstream/+/refs/heads/main/host/virtio-gpu-gfxstream-renderer.cpp#1691
>> >> >
>> >> > If you want to be paranoid, you can also validate the
>> >> ResourceCreateBlob::size is properly host-page aligned when that request
>> >> reaches the host.
>> >> >
>> >> > So you can probably solve this problem using current interfaces.
>> >> Whether it's cleaner for all context types to use the capabilities, or
>> have
>> >> all VMMs to expose VIRTIO_GPU_F_HOST_PAGE_SIZE, would be the
>> cost/benefit
>> >> tradeoff.
>> >> >
>> >>
>> >> I guess solving it in a context-type specific way is possible.  But I
>> >> think it is a relatively universal constraint.  And maybe it makes
>> >> sense for virtgpu guest kernel to enforce alignment (at least it can
>> >> return an error synchronously) in addition to the host.
>> >>
>> >
>> > virtio-media may have support for VIRTIO_MEDIA_CMD_MMAP too, so could run
>> > into this issue.
>> >
>> >
>> https://github.com/chromeos/virtio-media?tab=readme-ov-file#shared-memory-regions
>> >
>> > virtio-fs also has the DAX window which uses the same memory mapping
>> > mechanism.
>> >
>> > https://virtio-fs.gitlab.io/design.html
>> >
>> > Maybe this should not be a virtio-gpu thing, but a virtio thing?
>>
>> This is true, but finding a common place to put the page size is really
>> hard in practice. I don't think we can borrow space in the feature bits
>> for that (and that would probably be abusing its purpose quite a bit)
>> and extending the transport configuration registers is quite cumbersome
>> and, in general, undesirable.
>>
>> That leaves us with the device-specific config space, and that implies a
>> device-specific feature bit as it's implemented in this series.
>>
>> The Shared Memory Regions on the VIRTIO spec, while doesn't talk
>> specifically about page size, also gives us a hint about this being the
>> right direction:
>>
>
> Can't we just modify the Shared Memory region PCI capability to include
> page size?  We can either:
>
> 1) keep the same size struct + header (VIRTIO_PCI_CAP_SHARED_MEMORY_CFG),
> and just hijack one of the padding fields. If the padding field is zero, we
> can just say it's 4096.

Yes, we can turn that padding into "__le16 page_size_order" to store
"PAGE_SIZE >> 12". That should be enough to secure some future-proofing.
There's also some space in the "MMIO Device Register Layout" to store it
as a 16 bit or 32 bit value.

This would require proposing it as a change to the VIRTIO specs. Do you
want to do it yourself or should I take the initiative?

Thanks,
Sergio.

Re: [PATCH 0/2] drm/virtio: introduce the HOST_PAGE_SIZE feature

[Sergio Lopez Pascual]

Gurchetan Singh <gurchetansingh@chromium.org> writes:

> On Tue, Aug 6, 2024 at 1:15 PM Rob Clark <robdclark@gmail.com> wrote:
>
>> On Tue, Aug 6, 2024 at 9:15 AM Gurchetan Singh
>> <gurchetansingh@chromium.org> wrote:
>> >
>> >
>> >
>> > On Mon, Aug 5, 2024 at 2:14 AM Sergio Lopez Pascual <slp@redhat.com>
>> wrote:
>> >>
>> >> Dmitry Osipenko <dmitry.osipenko@collabora.com> writes:
>> >>
>> >> > On 7/23/24 14:49, 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, each one,
>> >> >> with a different page size.
>> >> >>
>> >> >> For what pertains to virtio-gpu, this is not a problem if the page
>> size
>> >> >> of the guest happens to be bigger or equal than the host, but will
>> >> >> potentially lead to failures in memory allocations and/or mappings
>> >> >> otherwise.
>> >> >
>> >> > Please describe concrete problem you're trying to solve. Guest memory
>> >> > allocation consists of guest pages, I don't see how knowledge of host
>> >> > page size helps anything in userspace.
>> >> >
>> >> > I suspect you want this for host blobs, but then it should be
>> >> > virtio_gpu_vram_create() that should use max(host_page_sz,
>> >> > guest_page_size), AFAICT. It's kernel who is responsible for memory
>> >> > management, userspace can't be trusted for doing that.
>> >>
>> >> Mesa's Vulkan/Venus uses CREATE_BLOB to request the host the creation
>> >> and mapping into the guest of device-backed memory and shmem regions.
>> >> The CREATE_BLOB ioctl doesn't update drm_virtgpu_resource_create->size,
>> >> so the guest kernel (and, as a consequence, the host kernel) can't
>> >> override the user's request.
>> >>
>> >> I'd like Mesa's Vulkan/Venus in the guest to be able to obtain the host
>> >> page size to align the size of the CREATE_BLOB requests as required.
>> >
>> >
>> > gfxstream solves this problem by putting the relevant information in the
>> capabilities obtained from the host:
>> >
>> >
>> https://android.googlesource.com/platform/hardware/google/gfxstream/+/refs/heads/main/host/virtio-gpu-gfxstream-renderer.cpp#1691
>> >
>> > If you want to be paranoid, you can also validate the
>> ResourceCreateBlob::size is properly host-page aligned when that request
>> reaches the host.
>> >
>> > So you can probably solve this problem using current interfaces.
>> Whether it's cleaner for all context types to use the capabilities, or have
>> all VMMs to expose VIRTIO_GPU_F_HOST_PAGE_SIZE, would be the cost/benefit
>> tradeoff.
>> >
>>
>> I guess solving it in a context-type specific way is possible.  But I
>> think it is a relatively universal constraint.  And maybe it makes
>> sense for virtgpu guest kernel to enforce alignment (at least it can
>> return an error synchronously) in addition to the host.
>>
>
> virtio-media may have support for VIRTIO_MEDIA_CMD_MMAP too, so could run
> into this issue.
>
> https://github.com/chromeos/virtio-media?tab=readme-ov-file#shared-memory-regions
>
> virtio-fs also has the DAX window which uses the same memory mapping
> mechanism.
>
> https://virtio-fs.gitlab.io/design.html
>
> Maybe this should not be a virtio-gpu thing, but a virtio thing?

There seem to be certain consensus that the information about alignment
restrictions must be shared in a device-specific way:

https://lore.kernel.org/virtio-comment/CY8PR12MB7195B5E575099CD9CA1F2F39DCAF2@CY8PR12MB7195.namprd12.prod.outlook.com/T/#t

There's also the precedent of virtio-fs already sharing the alignment
restrictions through FUSE_INIT (from VIRTIO SPECS 1.3, section
5.11.6.4).

---
The driver maps a file range into the DAX window using the
FUSE_SETUPMAPPING request. Alignment
constraints for FUSE_SETUPMAPPING and FUSE_REMOVEMAPPING requests are
communicated dur-
ing FUSE_INIT negotiation.
---

Given this information, I'm going to rebase and respin this series.

Thanks,
Sergio.