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

[Gurchetan Singh]

On Thu, Aug 8, 2024 at 3:38 AM Sergio Lopez Pascual <slp at redhat.com> wrote:

> Gurchetan Singh <gurchetansingh at chromium.org> writes:
>
> > On Tue, Aug 6, 2024 at 1:15 PM Rob Clark <robdclark at gmail.com> wrote:
> >
> >> On Tue, Aug 6, 2024 at 9:15 AM Gurchetan Singh
> >> <gurchetansingh at chromium.org> wrote:
> >> >
> >> >
> >> >
> >> > On Mon, Aug 5, 2024 at 2:14 AM Sergio Lopez Pascual <slp at redhat.com>
> >> wrote:
> >> >>
> >> >> Dmitry Osipenko <dmitry.osipenko at 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.

or

2) Or expand the size of the struct, with
VIRTIO_PCI_CAP_SHARED_MEMORY_CFG2.

(sketch here: crrev.com/c/5778179)

The benefit of this would work with virtio-fs (though I'm not sure anyone
uses the DAX window), and possibly virtio-media in the future.


>
> "
> 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.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20240809/bf24a563/attachment-0001.htm>